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

[Naushir Patuck]

Hi Laurent,

Thank you for your feedback.

On Thu, 9 Mar 2023 at 01:14, Laurent Pinchart <
laurent.pinchart at ideasonboard.com> 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?


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

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.

Regards,
Naush


> > +             }
> >
> > -             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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20230309/a7ae7cbf/attachment.htm>