Unicam instance without camera will cause the second unicam instance with camera not detected #44
Comments
|
I don't know enough about the overlay setup to be able to give a definitive answer. |
|
I think its' the way we're handling pipeline matching in libcamera/rpi pipeline handler. the PH is probably examining the first 'media device' (/dev/media4) and seeing no cameras, so it returns back that there are no devices supported by the pipeline handler and doesn't get asked to try again with /dev/media5 ? |
|
Ahhh, yes that's possibly what's happening! So nothing to do with devicetree/overlays. I'll have a look and see exactly what we do in our matching. |
|
One question I have for @6by9, should we register a media device if we cannot find a sensor device on that port? Given that /dev/video* nodes don't get registered, perhaps the same needs to be done for /dev/media*? Back to the pipeline handler, I'm not really sure what the "right" thing to do here is? The match() needs to fail if a camera cannot be enumerated/registered. Once the fail happens, the caller code will not re-iterate into the match() again. Do I need to do an internal second iteration of the media devices if the first call to PipelineHandlerRPi::match() fails? |
|
Something like this is what I had in mind: diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
index d752911ddfff..bce12388e503 100644
--- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
+++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
@@ -1248,41 +1248,46 @@ int PipelineHandlerRPi::queueRequestDevice(Camera *camera, Request *request)
bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)
{
- DeviceMatch unicam("unicam");
- MediaDevice *unicamDevice = acquireMediaDevice(enumerator, unicam);
+ for (unsigned int i = 0; i < 2; i++) {
+ DeviceMatch unicam("unicam");
+ MediaDevice *unicamDevice = acquireMediaDevice(enumerator, unicam);
- if (!unicamDevice) {
- LOG(RPI, Debug) << "Unable to acquire a Unicam instance";
- return false;
- }
-
- 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;
} |
|
Yes, I think the PH will have to cover all devices that it can handle. I'm not sure this was how it was originally envisaged, so a patch on the list for Laurent to consider will help. I'd probably break the function into two now - and have match first identify the components? then decide what to do with them? I don't think you should hardcode the assumption of 2 unicams though. |
From memory you have to register the media device early for the subdevices to then bind to, but you mustn't register the video device until everything is present as otherwise userspace can start messing with it in an unsafe manner. If you're looking at how the PH enumerates cameras then I've also had an issue flagged that |
I'll scream "Fault tolerance" as loud as I can every time I see this topic ;-) |
I was expecting you to make that comment :-) For individual cameras it doesn't make any real difference, but it is annoying with things like the CSI2 mux devices where if any 1 camera fails to probe, then the device sits there dumbly. |
|
Absolutely - and it should be able to support any that are working correctly ;-) |
Unless I am mistaken, I don't think we can do this unfortunately. In
Because the code above acquires the devices on start, scenario 3 and 4 would not enumerate correctly. I think the change might have to be closer to what I proposed, minus the hardcoded |
This is an easy one. libcamera-apps only set the tuning file env variable after the |
Looking at this a bit further, I can't see any way to do this cleanly other than a hard-coded |
|
Ah, but if we abuse |
|
Yes, that's what I meant - have this match handle all devices it can. That's the part I mentioned might need to be checked through with Laurent. But I think it's ok, and if it makes match() simpler in the rpi pipeline handler, and handles everything in one go - I think that's better. |
|
(we can of course always add a releaseMediaDevice if we need to if it's better) |
|
TL;DR; - I think this bug is a one line fix ;-) The key differentiator is whether each unicam instance should have it's own pipeline handler or can all be managed from a single. But it's a moot point, and having chatted with Laurent I think I've spotted a clearer fix. "Only" the following hunk applied should solve this I think. The issue is that on the 'empty' unicam we're returning false (!!0) to say no cameras were identified - however it did acquire a unicam instance from the enumerator. If instead it keeps that acquisition, (without creating a camera, as there is none), and then returns true - the camera manager/pipeilne handler factory will call match with a new pipeline handler instance. This will then correctly populate the second unicam instance, and acquire an available ISP. Finally, this will return (true) to say it has completed registration of that media device, and any cameras - and then finally - the CameraManager will call ->match() on the raspberry pi handler a third, and final time - where it will try to acquire a unicam instance, but it will not find one - so it will return 'false'. We might want also to remove the log print, which would otherwise always print: |
What does "each unicam instance should have it's own pipeline handler" entail? Specifically, does this mean that each "camera" will run in its own thread? If so, that is probably a desirable thing no? And presumably it does imply if we do enumerate all cameras in a single call to match(), they will all share a single pipeline handler thread? |
|
It's just the difference that each call to match constructs a full instance of a PipelineHandler object, so each match call has it's own context and private pipeline handler data. See CameraManager::Private::createPipelineHandlers at |
|
I don't think there's any specific threading differences here though ? |
I'll take your word for it :-) So can I conclude there is no performance penalty for doing all camera enumerations in one |
I can see that this would solve the problem, but are we abusing the API? Retuning true implies we have created/registered a Camera object. Will this come back to bite us? |
I don't believe so. The only key difference is that you would share a pipeline handler instance. But in Raspberry Pi pipeline handler there is no private data - so theres' nothing shared anyway (https://github.com/raspberrypi/libcamera/blob/main/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp#L325..L356) ... Aha - but there is shared data in the base PipelineHandler ... That waitingRequests_ might be important not to share if two unicams want to operate at the same time (maybe this should be in CameraData ... and likewise with the useCount_ and lock_. ... |
|
Ok, so I'm still unsure what to do here @kbingham. Given the API documentation and your comment about shared state, perhaps I keep the existing approach and only register one "active Unicam instance" per call to |
|
I still think the only change required here is: src/libcamera/pipeline/raspberrypi/raspberrypi.cpp: Just needs testing. I don't think it will bite us, as the only use of that return value is to tell the camera manager when to stop calling match() on this type. The only possible side effect I can see is that the unused unicam would have a pipeline handler constructed, which won't get registered to a camera so it should get 'free'd (due to the sharedptr) when it goes out of scope in the camera manager. I expect you could make some updates to the documentation at PipelineHandler::match here too if it helps. @will127534 Would you be able to test that one line change please to make sure it does actually solve the issue? |
|
On Thu, Feb 23, 2023 at 02:09:36AM -0800, Kieran Bingham wrote:
I still think the only change required here is:
src/libcamera/pipeline/raspberrypi/raspberrypi.cpp:
src/libcamera/pipeline/raspberrypi/raspberrypi.cpp:
bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)
{
...
- return !!numCameras;
+ return true;
}
Just needs testing. I don't think it will bite us, as the only use of that
return value is to tell the camera manager when to stop calling match() on this
type. The only possible side effect I can see is that the unused unicam would
have a pipeline handler constructed, which won't get registered to a camera so
it should get 'free'd (due to the sharedptr) when it goes out of scope in the
camera manager.
I'm not sure I like that a lot :-) It could possibly work, but not by
design, and may cause issues later. Could we instead skip unicam
instances that have no sensors in match() ? Instead of picking the first
unicam instead, loop over them all, and pick the first one that has a
sensor. If there are no unicam instances left, return false.
… I expect you could make some updates to the documentation at
PipelineHandler::match here too if it helps.
@will127534 Would you be able to test that one line change please to make sure
it does actually solve the issue?
--
Regards,
Laurent Pinchart
|
|
That returns us back to the original suggested fix. I'll post a patch then maybe we discuss from there... |
@kbingham, No problem It doesn't seems like it fix the issue, here is the output of libcamera app will hang when opening camera: On the same note, |
|
I've mailed my patch for discussion on the ML. @will127534 would you be able to test it out? You can download it from here. |
The patch works for me, listing camera & opening camera without issues |
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: raspberrypi/libcamera#44 Signed-off-by: Naushir Patuck <naush@raspberrypi.com> Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
|
This fix is now merged in upstream libcamera. |
If I have both unicam port enabled but only attach camera on CAM1 interface, libcamera will show
ERROR: *** no cameras available ***even though it shows up in v4l2v4l2-ctl --list-devices output:
If I remove the unused camera interface as the following
v4l2-ctl --list-devicesoutput shows:It is now showing up in libcamera --list
The text was updated successfully, but these errors were encountered: