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

[Kieran Bingham]

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
>