[libcamera-devel] [PATCH 2/8] Allow only one camera being started

[Cheng-Hao Yang]

> I think 'not having many usecases' for something isn't a good reason to
> forcibly disable it. If the hardware *can not* do it, then that's
> acceptable.

> Is there any use case where it is possible to capture from two cameras
> at the same time? I think it may simply not be possible on the IPU3 -
> but if that's the case, then that should be the documented rationale
> behind this patch.

>From Han-lin's perspective, basically there is no such a use case to enable
both cameras at the same time. Maybe it's only true for ipu3. I'm not sure.

I also thought that we can dynamically assign ImgUs: if only one camera is
being used, it can occupy both the ImgUs. However, when another camera
is enabled, the first camera needs to release one of the ImgU, and fall back
the StillCapture to VideoSnapshot.
Han-lin and I think that ideally it's possible, while we're not sure if
it's worth
the effort, as there's simply no such a use case, and there's very likely
to be
some lag during the transition (i.e. the first camera's stream might pause
when reconfiguring ISP).

> If we could for instance capture from both the IR sensor (which may not
> need the ISP?), and the image sensor at the same time on a front facing
> camera device like on the SGo2 - then I don't think we should be
> preventing that here.

IIUC the ipu3 pipeline handler prevents cases that use only StreamRole::Raw,
which means the ISP is necessary. Right?

--------------

For other suggestions, I'll fix them in the following patch. Let's wait for
others'
comments on all the patches to prevent the spam :)

Thanks for looking into this!

BR,
Harvey

On Thu, May 19, 2022 at 11:01 PM Kieran Bingham <
kieran.bingham at ideasonboard.com> wrote:

> Quoting Harvey Yang via libcamera-devel (2022-05-12 11:32:52)
> > 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
> > process frames from one single camera.
>
> I think 'not having many usecases' for something isn't a good reason to
> forcibly disable it. If the hardware *can not* do it, then that's
> acceptable.
>
> Is there any use case where it is possible to capture from two cameras
> at the same time? I think it may simply not be possible on the IPU3 -
> but if that's the case, then that should be the documented rationale
> behind this patch.
>
> If we could for instance capture from both the IR sensor (which may not
> need the ISP?), and the image sensor at the same time on a front facing
> camera device like on the SGo2 - then I don't think we should be
> preventing that here.
>
> > Signed-off-by: Harvey Yang <chenghaoyang at chromium.org>
> > ---
> >  src/libcamera/pipeline/ipu3/ipu3.cpp | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> >
> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp
> b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > index fd989e61..111ba053 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,10 @@ int PipelineHandlerIPU3::freeBuffers(Camera *camera)
> >
> >  int PipelineHandlerIPU3::start(Camera *camera, [[maybe_unused]] const
> ControlList *controls)
> >  {
> > +       // Deny second camera being started.
>
> libcamera code style here for a single line comment should be
>         /* Deny second camera being started. */
>
> But what about the third?
>
> If this is /not/ possible - then I think it should be more like:
>
>         /*
>          * Enforce that only a single camera can be used at a time due
>          * to the limitations of the IPU3 IMGU which can only ..
>          * <detailed of limitation here>.
>          */
>
> > +       if (inUseCamera_ && inUseCamera_ != camera)
> > +               return -1;
>
> Errnos rather than -1 ...
>
> Perhaps -EBUSY ...
>
> > +
> >         IPU3CameraData *data = cameraData(camera);
> >         CIO2Device *cio2 = &data->cio2_;
> >         ImgUDevice *imgu = data->imgu_;
> > @@ -781,6 +787,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 +816,8 @@ error:
> >         freeBuffers(camera);
> >         LOG(IPU3, Error) << "Failed to start camera " << camera->id();
> >
> > +       inUseCamera_ = nullptr;
> > +
> >         return ret;
> >  }
> >
> > @@ -826,6 +836,8 @@ void PipelineHandlerIPU3::stopDevice(Camera *camera)
> >                 LOG(IPU3, Warning) << "Failed to stop camera " <<
> camera->id();
> >
> >         freeBuffers(camera);
> > +
> > +       inUseCamera_ = nullptr;
> >  }
> >
> >  void IPU3CameraData::cancelPendingRequests()
> > --
> > 2.36.0.512.ge40c2bad7a-goog
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20220520/c67a5b20/attachment-0001.htm>