[libcamera-devel] [PATCH] pipeline: raspberrypi: Iterate over all Unicam instances in match()

[Laurent Pinchart]

Hi Naush,

Thank you for the patch.

I know this has been merged, but I've noticed a few issues, which can be
fixed in further patches.

On Fri, Feb 24, 2023 at 07:30:23AM +0000, Naushir Patuck via libcamera-devel wrote:
> 
> On Raspberry Pi Compute Module platforms, it is possible to attach a
> single camera device only to the secondary Unicam port. The current
> logic of PipelineHandlerRPi::match() will return a failure during
> enumeration of the first Unicam media device (due to no sensor attached,
> or sensor failure) and thus the second Unicam media device will never be
> enumerated.
> 
> Fix this by looping over all Unicam instances in PipelineHandlerRPi::match()
> until a camera is correctly registered, or return a failure otherwise.
> 
> Reported-on: https://github.com/raspberrypi/libcamera/issues/44
> Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> ---
>  .../pipeline/raspberrypi/raspberrypi.cpp      | 67 +++++++++++--------
>  1 file changed, 40 insertions(+), 27 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index 841209548350..ef01b7e166ba 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -1246,41 +1246,54 @@ int PipelineHandlerRPi::queueRequestDevice(Camera *camera, Request *request)
>  
>  bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)
>  {
> -	DeviceMatch unicam("unicam");
> -	MediaDevice *unicamDevice = acquireMediaDevice(enumerator, unicam);
> +	constexpr unsigned int numUnicamDevices = 2;

Constants should start with a k prefix, that is kNumUnicamDevices.

>  
> -	if (!unicamDevice) {
> -		LOG(RPI, Debug) << "Unable to acquire a Unicam instance";
> -		return false;
> -	}
> +	/*
> +	 * Loop over all Unicam instances, but return out once a match is found.
> +	 * This is to ensure we correctly enumrate the camera when an instance
> +	 * of Unicam has registered with media controller, but has not registered
> +	 * device nodes due to a sensor subdevice failure.
> +	 */
> +	for (unsigned int i = 0; i < numUnicamDevices; i++) {
> +		DeviceMatch unicam("unicam");
> +		MediaDevice *unicamDevice = acquireMediaDevice(enumerator, unicam);
>  
> -	DeviceMatch isp("bcm2835-isp");
> -	MediaDevice *ispDevice = acquireMediaDevice(enumerator, isp);
> +		if (!unicamDevice) {
> +			LOG(RPI, Debug) << "Unable to acquire a Unicam instance";
> +			continue;

This looks weird, if the unicam device can't be acquired, I don't see
how the next iteration of the loop could successfully acquire another
instance. I would thus break here.

> +		}
>  
> -	if (!ispDevice) {
> -		LOG(RPI, Debug) << "Unable to acquire ISP instance";
> -		return false;
> -	}
> +		DeviceMatch isp("bcm2835-isp");
> +		MediaDevice *ispDevice = acquireMediaDevice(enumerator, isp);
>  
> -	/*
> -	 * The loop below is used to register multiple cameras behind one or more
> -	 * video mux devices that are attached to a particular Unicam instance.
> -	 * Obviously these cameras cannot be used simultaneously.
> -	 */
> -	unsigned int numCameras = 0;
> -	for (MediaEntity *entity : unicamDevice->entities()) {
> -		if (entity->function() != MEDIA_ENT_F_CAM_SENSOR)
> +		if (!ispDevice) {
> +			LOG(RPI, Debug) << "Unable to acquire ISP instance";
>  			continue;

Shouldn't you release the unicam device in this case ? I think it would
be better to first loop over unicam instances, ignoring any instance
than has no connected camera sensor, and then, if an instance with a
connected sensor is found, acquire an ISP instance.

> +		}
>  
> -		int ret = registerCamera(unicamDevice, ispDevice, entity);
> -		if (ret)
> -			LOG(RPI, Error) << "Failed to register camera "
> -					<< entity->name() << ": " << ret;
> -		else
> -			numCameras++;
> +		/*
> +		 * The loop below is used to register multiple cameras behind one or more
> +		 * video mux devices that are attached to a particular Unicam instance.
> +		 * Obviously these cameras cannot be used simultaneously.
> +		 */
> +		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++;
> +		}
> +
> +		if (numCameras)
> +			return true;
>  	}
>  
> -	return !!numCameras;
> +	return false;
>  }
>  
>  void PipelineHandlerRPi::releaseDevice(Camera *camera)

-- 
Regards,

Laurent Pinchart