[libcamera-devel] [PATCH v3 2/9] ipu3: Allow only one camera being started
Cheng-Hao Yang
chenghaoyang at chromium.org
Tue Aug 2 12:30:19 CEST 2022
Thanks Umang and Han-lin,
An interesting fact is that the current patch (v3) actually passes
android.hardware.cts.CameraTest#testMultipleCameras. Maybe it only opens
the cameras without actually starting them and getting frames?
Nonetheless, I updated Camera::acquire() and Camera::release() to check
with the pipeline handler, and still makes PipelineHandlerIPU3 support only
one camera being acquired at a time. Please check.
> > int PipelineHandlerIPU3::start(Camera *camera, [[maybe_unused]] const
ControlList *controls)
> > {
> > + /*
> > + * Enforce that only a single camera can be used at a time to use
both
> > + * ImgUs on the camera, so that StillCapture stream can adopt
another
> > + * set of configuration.
> > + */
> > + if (inUseCamera_ && inUseCamera_ != camera)
> > + return -EBUSY;
> I think this is a wrong place to return -EBUSY.
> After Camera::acquire() success, we fail to start the camera reporting
> -EBUSY, is not a good behavior IMO
Right, how about returning -EUSERS as well? With v4, it actually should
never happen.
On Tue, Jul 26, 2022 at 11:48 PM Umang Jain <umang.jain at ideasonboard.com>
wrote:
> Hi Hanlin,
>
> On 7/26/22 17:50, Hanlin Chen wrote:
> > On Tue, Jul 26, 2022 at 3:16 PM Umang Jain via libcamera-devel
> > <libcamera-devel at lists.libcamera.org> wrote:
> >> Hi Harvey,
> >>
> >> Thank you for the patch.
> >>
> >> On 6/29/22 16:00, Harvey Yang via libcamera-devel wrote:
> >>> From: Harvey Yang <chenghaoyang at chromium.org>
> >>>
> >>> As we hardly have use cases/applications that need both cameras at the
> >>> same time, this patch adds a rule that only one camera can be started
> >>> one time. This also allows the following patches that use both imgus to
> >>
> >> Before I go any further, I believe there are CTS tests that require
> >> multiple streaming cameras at same point.
> >>
> >> For e.g. I see android.hardware.cts.CameraTest#testMultipleCameras [1]
> >>
> >> Is my understanding correct that streaming multiple cameras at the same
> >> is a CTS requirement ? Doesn't this patch violate that?
> >>
> >> I also see a related patch on the list under `[PATCH] libcamera: Allow
> >> concurrent use of cameras from same pipeline handler` subject. Would you
> >> like to review/discuss or clarify the situation here please?
> >>
> >> [1]
> >>
> https://android.googlesource.com/platform/cts/+/refs/heads/master/tests/camera/src/android/hardware/camera2/cts/CameraManagerTest.java#337
> >>
> > Hi Umang and Harvey,
> >
> > I think the test accept MAX_CAMERAS_IN_USE error as a pass situation,
> > which means HAL rejects the open due to internal resource limit. The
> > error is set when CameraDevice::open() returns -EUSER:
> > * -EUSERS: The maximal number of camera devices that can be opened
> > concurrently were opened already, either by this method or the
> > open_legacy method.
> > The problem here is that we didn't return -EUSERS on Camera::acquire()
> > which is called by CameraDevice::open().
> > Instead of checking this in PipelineHandlerIPU3::start(),
> > Camera::acquire() could call into the pipeline handler to check the
> > resource limit and return the -EUSER error directly, since only the
> > pipeline handler knows how to manage its resources.
>
>
> I agree with the premise here, but currently I think pipeline-handlers
> don't have a concept of "resource limiting" being implemented; based on
> which we decide whether a camera can be acquired or not.
>
> The patch in discussion below, implements a acquire() per
> pipeline-handler so it might be good to extend that to satisfy this case
> for IPU3 platform..
>
>
> > See:
> >
> https://android.googlesource.com/platform/cts/+/refs/heads/master/tests/camera/src/android/hardware/camera2/cts/CameraManagerTest.java#409
> >
> https://android.googlesource.com/platform/hardware/libhardware/+/master/include/hardware/camera_common.h#872
> >
> > I think "[PATCH] libcamera: Allow concurrent use of cameras from same
> > pipeline handler" is resolving a different problem. It allows opening
> > multi cameras if resources are adequate. Our case is to report the
> > error accordingly when resources are inadequate.
> >
> >>> process frames from one single camera.
> >>>
> >>> Signed-off-by: Harvey Yang <chenghaoyang at chromium.org>
> >>> ---
> >>> src/libcamera/pipeline/ipu3/ipu3.cpp | 16 ++++++++++++++++
> >>> 1 file changed, 16 insertions(+)
> >>>
> >>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp
> b/src/libcamera/pipeline/ipu3/ipu3.cpp
> >>> index fd989e61..c943ee6a 100644
> >>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> >>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> >>> @@ -166,6 +166,8 @@ private:
> >>> MediaDevice *cio2MediaDev_;
> >>> MediaDevice *imguMediaDev_;
> >>>
> >>> + Camera *inUseCamera_ = nullptr;
> >>> +
> >>> std::vector<IPABuffer> ipaBuffers_;
> >>> };
> >>>
> >>> @@ -765,6 +767,14 @@ int PipelineHandlerIPU3::freeBuffers(Camera
> *camera)
> >>>
> >>> int PipelineHandlerIPU3::start(Camera *camera, [[maybe_unused]]
> const ControlList *controls)
> >>> {
> >>> + /*
> >>> + * Enforce that only a single camera can be used at a time to
> use both
> >>> + * ImgUs on the camera, so that StillCapture stream can adopt
> another
> >>> + * set of configuration.
> >>> + */
> >>> + if (inUseCamera_ && inUseCamera_ != camera)
> >>> + return -EBUSY;
> >>
> >> I think this is a wrong place to return -EBUSY.
> >>
> >> After Camera::acquire() success, we fail to start the camera reporting
> >> -EBUSY, is not a good behavior IMO
> >>
> >>> +
> >>> IPU3CameraData *data = cameraData(camera);
> >>> CIO2Device *cio2 = &data->cio2_;
> >>> ImgUDevice *imgu = data->imgu_;
> >>> @@ -781,6 +791,8 @@ int PipelineHandlerIPU3::start(Camera *camera,
> [[maybe_unused]] const ControlLis
> >>> if (ret)
> >>> return ret;
> >>>
> >>> + inUseCamera_ = camera;
> >>> +
> >>> ret = data->ipa_->start();
> >>> if (ret)
> >>> goto error;
> >>> @@ -808,6 +820,8 @@ error:
> >>> freeBuffers(camera);
> >>> LOG(IPU3, Error) << "Failed to start camera " << camera->id();
> >>>
> >>> + inUseCamera_ = nullptr;
> >>> +
> >>> return ret;
> >>> }
> >>>
> >>> @@ -826,6 +840,8 @@ void PipelineHandlerIPU3::stopDevice(Camera
> *camera)
> >>> LOG(IPU3, Warning) << "Failed to stop camera " <<
> camera->id();
> >>>
> >>> freeBuffers(camera);
> >>> +
> >>> + inUseCamera_ = nullptr;
> >>> }
> >>>
> >>> void IPU3CameraData::cancelPendingRequests()
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20220802/cc966cd4/attachment.htm>
More information about the libcamera-devel
mailing list