[libcamera-devel] [PATCH] pipeline: raspberrypi: Iterate over all Unicam instances in match()
Naushir Patuck
naush at raspberrypi.com
Fri Mar 10 11:49:29 CET 2023
On Fri, 10 Mar 2023 at 10:33, Laurent Pinchart <
laurent.pinchart at ideasonboard.com> wrote:
> Hi Naush,
>
> On Fri, Mar 10, 2023 at 10:22:01AM +0000, Naushir Patuck wrote:
> > One more thing....
> >
> > On Fri, 10 Mar 2023 at 10:01, 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?
> > >
> > >> > > > + }
> > >> > > >
> > >> > > > - 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.
> >
> > Actually, moving the ISP acquire into the inner loop would be the wrong
> thing to
> > do. The inner loop is to identify multiple cameras attached to a single
> Unicam
> > port through a mux/bridge chip. As such, only a single sensor can be
> active at
> > any time, and only a single ISP can service them. So I think the ISP
> acquiring
> > code is in the right place.
>
> Yes, but that's not what I meant :-)
>
> foreach (unicam instances) {
> unicam = acquireMediaDevice(instance);
> if (!unicam)
> /* We've exhausted all unicam instances */
> return false;
>
> if (unicam instance has connected source)
> break;
> }
>
> DeviceMatch isp("bcm2835-isp");
> MediaDevice *ispDevice = acquireMediaDevice(enumerator, isp);
>
> if (!ispDevice)
> return false;
>
>
Ah, sorry I get you now. However, this would not work again for the case
where
we have multiple sensors attached to one Unicam port via a mux. We need a
second inter loop iterating over "connected sources".
I could refactor the entire match() function to work round this, but I don't
really see any benefit in making such a big change. As you said, it's ok to
have the ISP stay acquired as no other pipeline handler instance would be
able
to use it either.
Regards,
Naush
> > >> > 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20230310/9608421c/attachment.htm>
More information about the libcamera-devel
mailing list