| Message ID | mailman.19.1677223831.25031.libcamera-devel@lists.libcamera.org |
|---|---|
| State | Accepted |
| Commit | 2a261d911f50d925be7b8921c1af58cde8a7f545 |
| Headers | show |
| Series |
|
| Related | show |
Hi Naush Thanks for the patch. On Fri, 24 Feb 2023 at 07:30, Naushir Patuck via libcamera-devel <libcamera-devel@lists.libcamera.org> 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@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; > > - 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 s/enumrate/enumerate/ > + * 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; > + } > > - 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; > + } > > - 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; > } Looks fine to me! Reviewed-by: David Plowman <david.plowman@raspberrypi.com> Thanks! David > > void PipelineHandlerRPi::releaseDevice(Camera *camera) > -- > 2.25.1 >
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@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. > + } > > - 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. > + } > > - 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)
Hi Laurent, Thank you for your feedback. On Thu, 9 Mar 2023 at 01:14, Laurent Pinchart < laurent.pinchart@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@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 >
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@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. > > > + } > > > > > > - 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)
Hi Laurent, On Thu, 9 Mar 2023 at 10:02, Laurent Pinchart < laurent.pinchart@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@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. > > > > 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 >
One more thing.... On Fri, 10 Mar 2023 at 10:01, Naushir Patuck <naush@raspberrypi.com> wrote: > Hi Laurent, > > On Thu, 9 Mar 2023 at 10:02, Laurent Pinchart < > laurent.pinchart@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@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 >> >
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@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)
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@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; > >> > 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)
On Fri, 10 Mar 2023 at 10:33, Laurent Pinchart < laurent.pinchart@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@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 >
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; - 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; + } - 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; + } - 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)
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@raspberrypi.com> --- .../pipeline/raspberrypi/raspberrypi.cpp | 67 +++++++++++-------- 1 file changed, 40 insertions(+), 27 deletions(-)