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

[Kieran Bingham]

Quoting Naushir Patuck (2023-02-24 07:30:23)
> 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>

I'm not sure how we convey it, but I see 
Will Whang ( will127534 ) reports he has tested this.


> ---
>  .../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;
>  
> -       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";

I'm still a little weary about this way of doing it. I think now when
the RPi Pipeline handler is built and enabled on a non-rpi platform -
the debug logs will print this line twice.

But it's at the debug level, so it probably doesn't matter.

Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>

> +                       continue;
> +               }
>  
> -       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;
> +               }
>  
> -               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)
> -- 
> 2.25.1
>