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

[Naushir Patuck]

Hi Kieran,

Thanks for the review!

On Wed, 1 Mar 2023 at 00:42, Kieran Bingham
<kieran.bingham at ideasonboard.com> wrote:
>
> 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.

I could remove the log messages if that's desired?

Naush


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