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

[Laurent Pinchart]

On Fri, Mar 10, 2023 at 10:01:08AM +0000, Naushir Patuck wrote:
> On Thu, 9 Mar 2023 at 10:02, Laurent Pinchart wrote:
> > 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.
> 
> For my clarification, so process 1 will still acquire Unicam 0, but when it
> comes to camera.start(), will fail since process 0 will have Unicam 0 running in
> its process.  Is that right?

Nearly. Both processes will acquire the same unicam instances in
match(), being completely unaware of each other. It is only
Camera::acquire() (not Camera::start()) that has basic inter-process
locking.

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