[libcamera-devel] [PATCH v2 1/2] pipeline: raspberrypi: Move sensor entity detection out of registerCamera()

[Laurent Pinchart]

Hi Naush,

Thank you for the patch.

On Wed, Dec 08, 2021 at 03:15:26PM +0000, Naushir Patuck wrote:
> Enumerate the sensor device entities in PipelineHandlerRPi::match() and loop
> over PipelineHandlerRPi::registerCamera() for each sensor found. This will
> allow the pipeline handler to register multiple cameras attached to a single
> Unicam instance with a Video Mux device.
> 
> Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> ---
>  .../pipeline/raspberrypi/raspberrypi.cpp      | 35 +++++++++++--------
>  1 file changed, 20 insertions(+), 15 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index 86851ac467ad..756878c70036 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -310,7 +310,7 @@ private:
>  		return static_cast<RPiCameraData *>(camera->_d());
>  	}
>  
> -	int registerCamera(MediaDevice *unicam, MediaDevice *isp);
> +	int registerCamera(MediaDevice *unicam, MediaDevice *isp, MediaEntity *sensorEntity);
>  	int queueAllBuffers(Camera *camera);
>  	int prepareBuffers(Camera *camera);
>  	void freeBuffers(Camera *camera);
> @@ -1029,16 +1029,28 @@ bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)
>  		return false;
>  	}
>  
> -	int ret = registerCamera(unicamDevice, ispDevice);
> -	if (ret) {
> -		LOG(RPI, Error) << "Failed to register camera: " << ret;
> -		return false;
> +	/*
> +	 * The loop below is used to register multiple cameras behind one or more
> +	 * video mux devices that are attaced to a particular Unicam instance.

s/attaced/attached/

> +	 * Obviously these cameras cannot be used simultaneously.

We need to expose mutual exclusion between cameras to applications
through the libcamera public API. It doesn't have to be a blocker for
the series, but should be done soon after, otherwise trying to use both
cameras will result in incorrect behaviour (and possibly even crashes).
Have you given any thought to how this could be done ?

> +	 */
> +	unsigned int numCameras = 0;
> +	for (MediaEntity *entity : unicamDevice->entities()) {
> +		if (entity->function() != MEDIA_ENT_F_CAM_SENSOR)
> +			continue;
> +
> +		int ret = registerCamera(unicamDevice, ispDevice, entity);
> +		if (ret)
> +			LOG(RPI, Error) << "Failed to register camera "
> +					<< entity->name() << ": " << ret;
> +		else
> +			numCameras++;

I tend to deal with error first, but that's a personal preference:

		int ret = registerCamera(unicamDevice, ispDevice, entity);
		if (ret) {
			LOG(RPI, Error) << "Failed to register camera "
					<< entity->name() << ": " << ret;
			continue;
		}

		numCameras++;

>  	}
>  
> -	return true;
> +	return !!numCameras;

This looks fine, but by itself may cause a regression. Patch 2/2 will be
merged with this one, so the problem would only occur during bisection,
but still, I think we should squash the two patches together.

>  }
>  
> -int PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp)
> +int PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp, MediaEntity *sensorEntity)
>  {
>  	std::unique_ptr<RPiCameraData> data = std::make_unique<RPiCameraData>(this);
>  
> @@ -1079,14 +1091,7 @@ int PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp)
>  	data->isp_[Isp::Output1].dev()->bufferReady.connect(data.get(), &RPiCameraData::ispOutputDequeue);
>  	data->isp_[Isp::Stats].dev()->bufferReady.connect(data.get(), &RPiCameraData::ispOutputDequeue);
>  
> -	/* Identify the sensor. */
> -	for (MediaEntity *entity : unicam->entities()) {
> -		if (entity->function() == MEDIA_ENT_F_CAM_SENSOR) {
> -			data->sensor_ = std::make_unique<CameraSensor>(entity);
> -			break;
> -		}
> -	}
> -
> +	data->sensor_ = std::make_unique<CameraSensor>(sensorEntity);
>  	if (!data->sensor_)
>  		return -EINVAL;
>  

-- 
Regards,

Laurent Pinchart