[libcamera-devel] [PATCH v3 2/9] ipu3: Allow only one camera being started

[Hanlin Chen]

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.
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()