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

[Laurent Pinchart]

Hi Naush,

On Thu, Mar 09, 2023 at 08:04:47AM +0000, Naushir Patuck wrote:
> On Thu, 9 Mar 2023 at 01:14, Laurent Pinchart wrote:
> 
> > 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.
> 
> This is probably me not understanding how the media device enumeration stuff
> works, but I thought the continue would be needed for situations where we want
> simultaneous dual cameras running in separate processes.  For example, process 0
> acquires "Unicam 0" and starts running as normal.  Process 1 starts and goes
> through match() where "Unicam 0" still exists in the entity list, but fails to
> acquire because it is locked by process 0.  So we have to move on to "Unicam 1"
> which is acquired correctly for process 1.  Is that understanding wrong?

The minimal inter-process locking support in libcamera only operates
when trying to acquire a *camera* with Camera::acquire(). The
acquireMediaDevice() function is a bit confusing, its name refers to the
pipeline handler acquiring a MediaDevice from the DeviceEnumerator,
guaranteeing that the pipeline handler gets ownership of the media
device and no other pipeline handler *in the same process* will be able
to acquire it. Two processes running libcamera will both get "Unicam 0"
in the first iteration of the loop.

> > > +             }
> > >
> > > -     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.
> 
> I think we discussed this briefly in the github comments.  There is no
> compliment releaseMediaDevice() call that I can use to release the device.

It's an issue indeed. The design idea was to release all acquired media
devices automatically when the match() function returns false, but that
doesn't allow releasing media device that have been acquired and turned
out not to be needed.

In this specific case, if you acquire a unicam instance that has no
connected sensor, it's fine if it stays acquired as no other pipeline
handler instance would be able to use it for a meaningful purpose
anyway, but in general this is something we should probably fix.

> Regarding the second part of the comment, yes, I could move the isp acquire bit
> into the for (unicamDevice->entities()) loop to optimise this a bit.
> 
> > > +             }
> > >
> > > -             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