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

[Umang Jain]

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

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