<div dir="ltr"><div dir="ltr">Hi Laurent,</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, 9 Mar 2023 at 10:02, Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com">laurent.pinchart@ideasonboard.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Naush,<br>
<br>
On Thu, Mar 09, 2023 at 08:04:47AM +0000, Naushir Patuck wrote:<br>
> On Thu, 9 Mar 2023 at 01:14, Laurent Pinchart wrote:<br>
> <br>
> > Hi Naush,<br>
> ><br>
> > Thank you for the patch.<br>
> ><br>
> > I know this has been merged, but I've noticed a few issues, which can be<br>
> > fixed in further patches.<br>
> ><br>
> > On Fri, Feb 24, 2023 at 07:30:23AM +0000, Naushir Patuck via libcamera-devel wrote:<br>
> > ><br>
> > > On Raspberry Pi Compute Module platforms, it is possible to attach a<br>
> > > single camera device only to the secondary Unicam port. The current<br>
> > > logic of PipelineHandlerRPi::match() will return a failure during<br>
> > > enumeration of the first Unicam media device (due to no sensor attached,<br>
> > > or sensor failure) and thus the second Unicam media device will never be<br>
> > > enumerated.<br>
> > ><br>
> > > Fix this by looping over all Unicam instances in PipelineHandlerRPi::match()<br>
> > > until a camera is correctly registered, or return a failure otherwise.<br>
> > ><br>
> > > Reported-on: <a href="https://github.com/raspberrypi/libcamera/issues/44" rel="noreferrer" target="_blank">https://github.com/raspberrypi/libcamera/issues/44</a><br>
> > > Signed-off-by: Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a>><br>
> > > ---<br>
> > > .../pipeline/raspberrypi/raspberrypi.cpp | 67 +++++++++++--------<br>
> > > 1 file changed, 40 insertions(+), 27 deletions(-)<br>
> > ><br>
> > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> > > index 841209548350..ef01b7e166ba 100644<br>
> > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> > > @@ -1246,41 +1246,54 @@ int PipelineHandlerRPi::queueRequestDevice(Camera *camera, Request *request)<br>
> > ><br>
> > > bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)<br>
> > > {<br>
> > > - DeviceMatch unicam("unicam");<br>
> > > - MediaDevice *unicamDevice = acquireMediaDevice(enumerator, unicam);<br>
> > > + constexpr unsigned int numUnicamDevices = 2;<br>
> ><br>
> > Constants should start with a k prefix, that is kNumUnicamDevices.<br>
> ><br>
> > ><br>
> > > - if (!unicamDevice) {<br>
> > > - LOG(RPI, Debug) << "Unable to acquire a Unicam instance";<br>
> > > - return false;<br>
> > > - }<br>
> > > + /*<br>
> > > + * Loop over all Unicam instances, but return out once a match is found.<br>
> > > + * This is to ensure we correctly enumrate the camera when an instance<br>
> > > + * of Unicam has registered with media controller, but has not registered<br>
> > > + * device nodes due to a sensor subdevice failure.<br>
> > > + */<br>
> > > + for (unsigned int i = 0; i < numUnicamDevices; i++) {<br>
> > > + DeviceMatch unicam("unicam");<br>
> > > + MediaDevice *unicamDevice = acquireMediaDevice(enumerator, unicam);<br>
> > ><br>
> > > - DeviceMatch isp("bcm2835-isp");<br>
> > > - MediaDevice *ispDevice = acquireMediaDevice(enumerator, isp);<br>
> > > + if (!unicamDevice) {<br>
> > > + LOG(RPI, Debug) << "Unable to acquire a Unicam instance";<br>
> > > + continue;<br>
> ><br>
> > This looks weird, if the unicam device can't be acquired, I don't see<br>
> > how the next iteration of the loop could successfully acquire another<br>
> > instance. I would thus break here.<br>
> <br>
> This is probably me not understanding how the media device enumeration stuff<br>
> works, but I thought the continue would be needed for situations where we want<br>
> simultaneous dual cameras running in separate processes. For example, process 0<br>
> acquires "Unicam 0" and starts running as normal. Process 1 starts and goes<br>
> through match() where "Unicam 0" still exists in the entity list, but fails to<br>
> acquire because it is locked by process 0. So we have to move on to "Unicam 1"<br>
> which is acquired correctly for process 1. Is that understanding wrong?<br>
<br>
The minimal inter-process locking support in libcamera only operates<br>
when trying to acquire a *camera* with Camera::acquire(). The<br>
acquireMediaDevice() function is a bit confusing, its name refers to the<br>
pipeline handler acquiring a MediaDevice from the DeviceEnumerator,<br>
guaranteeing that the pipeline handler gets ownership of the media<br>
device and no other pipeline handler *in the same process* will be able<br>
to acquire it. Two processes running libcamera will both get "Unicam 0"<br>
in the first iteration of the loop.<br></blockquote><div><br></div><div>For my clarification, so process 1 will still acquire Unicam 0, but when it<br>comes to camera.start(), will fail since process 0 will have Unicam 0 running in<br>its process. Is that right?<br></div><div><br></div><div>Naush</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> > > + }<br>
> > ><br>
> > > - if (!ispDevice) {<br>
> > > - LOG(RPI, Debug) << "Unable to acquire ISP instance";<br>
> > > - return false;<br>
> > > - }<br>
> > > + DeviceMatch isp("bcm2835-isp");<br>
> > > + MediaDevice *ispDevice = acquireMediaDevice(enumerator, isp);<br>
> > ><br>
> > > - /*<br>
> > > - * The loop below is used to register multiple cameras behind one or more<br>
> > > - * video mux devices that are attached to a particular Unicam instance.<br>
> > > - * Obviously these cameras cannot be used simultaneously.<br>
> > > - */<br>
> > > - unsigned int numCameras = 0;<br>
> > > - for (MediaEntity *entity : unicamDevice->entities()) {<br>
> > > - if (entity->function() != MEDIA_ENT_F_CAM_SENSOR)<br>
> > > + if (!ispDevice) {<br>
> > > + LOG(RPI, Debug) << "Unable to acquire ISP instance";<br>
> > > continue;<br>
> ><br>
> > Shouldn't you release the unicam device in this case ? I think it would<br>
> > be better to first loop over unicam instances, ignoring any instance<br>
> > than has no connected camera sensor, and then, if an instance with a<br>
> > connected sensor is found, acquire an ISP instance.<br>
> <br>
> I think we discussed this briefly in the github comments. There is no<br>
> compliment releaseMediaDevice() call that I can use to release the device.<br>
<br>
It's an issue indeed. The design idea was to release all acquired media<br>
devices automatically when the match() function returns false, but that<br>
doesn't allow releasing media device that have been acquired and turned<br>
out not to be needed.<br>
<br>
In this specific case, if you acquire a unicam instance that has no<br>
connected sensor, it's fine if it stays acquired as no other pipeline<br>
handler instance would be able to use it for a meaningful purpose<br>
anyway, but in general this is something we should probably fix.<br>
<br>
> Regarding the second part of the comment, yes, I could move the isp acquire bit<br>
> into the for (unicamDevice->entities()) loop to optimise this a bit.<br>
> <br>
> > > + }<br>
> > ><br>
> > > - int ret = registerCamera(unicamDevice, ispDevice, entity);<br>
> > > - if (ret)<br>
> > > - LOG(RPI, Error) << "Failed to register camera "<br>
> > > - << entity->name() << ": " << ret;<br>
> > > - else<br>
> > > - numCameras++;<br>
> > > + /*<br>
> > > + * The loop below is used to register multiple cameras behind one or more<br>
> > > + * video mux devices that are attached to a particular Unicam instance.<br>
> > > + * Obviously these cameras cannot be used simultaneously.<br>
> > > + */<br>
> > > + unsigned int numCameras = 0;<br>
> > > + for (MediaEntity *entity : unicamDevice->entities()) {<br>
> > > + if (entity->function() != MEDIA_ENT_F_CAM_SENSOR)<br>
> > > + continue;<br>
> > > +<br>
> > > + int ret = registerCamera(unicamDevice, ispDevice, entity);<br>
> > > + if (ret)<br>
> > > + LOG(RPI, Error) << "Failed to register camera "<br>
> > > + << entity->name() << ": " << ret;<br>
> > > + else<br>
> > > + numCameras++;<br>
> > > + }<br>
> > > +<br>
> > > + if (numCameras)<br>
> > > + return true;<br>
> > > }<br>
> > ><br>
> > > - return !!numCameras;<br>
> > > + return false;<br>
> > > }<br>
> > ><br>
> > > void PipelineHandlerRPi::releaseDevice(Camera *camera)<br>
<br>
-- <br>
Regards,<br>
<br>
Laurent Pinchart<br>
</blockquote></div></div>