[libcamera-devel] [PATCH] pipeline: raspberrypi: Iterate over all Unicam instances in match()
Naushir Patuck
naush at raspberrypi.com
Fri Mar 10 11:22:01 CET 2023
One more thing....
On Fri, 10 Mar 2023 at 10:01, Naushir Patuck <naush at raspberrypi.com> wrote:
> Hi Laurent,
>
> On Thu, 9 Mar 2023 at 10:02, Laurent Pinchart <
> laurent.pinchart at ideasonboard.com> wrote:
>
>> 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.
>>
>
> 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?
>
> Naush
>
>
>> > > > + }
>> > > >
>> > > > - 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.
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/3ca75489/attachment.htm>
More information about the libcamera-devel
mailing list