Unicam instance without camera will cause the second unicam instance with camera not detected about libcamera HOT 31 CLOSED

I'll scream "Fault tolerance" as loud as I can every time I see this issue ;-)
(But as you were at the media-meeting @6by9, you'll know this is my pet-peeve / hate hehe)

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.

from libcamera.

I don't know enough about the overlay setup to be able to give a definitive answer.
@6by9 any thoughts on this one?

from libcamera.

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 ?

from libcamera.

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.

from libcamera.

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?

from libcamera.

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;
 }

from libcamera.

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?
It kind of makes the whole 'returning back true or false' quite redundant, as the PH should never return false?

I don't think you should hardcode the assumption of 2 unicams though.
Would something like this help? <pure psuedo code, not compiled>

std::vector<MediaDevice*> unicamDevices;
std::vector<MediaDevice*> ispDevices;

/* Find all Unicam instances */
while(MediaDevice *device = acquireMediaDevice(enumerator, "unicam"))
    unicamDevices.push_back(device);

if (unicamDevices.empty())
    return true; /* No compatible devices */

/* Find all available ISPs */
while (MediaDevice *device = acquireMediaDevice(enumerator, "bcm2835-isp"))
    ispDevices.push_back(device);

/* Iterate each unicam and if it has any cameras, obtain an ISP */
while (MediaDevice *unicam = unicamDevices.pop()) {
    if (ispDevices.empty()) {
        LOG(Error) << "Not enough ISP devices to manage unicam";
        return true;
    }

    /* Only consume the ISP if cameras were registered against it */
    unsigned int cameras = registerCameras(unicam, ispDevices.front());
    if (cameras)
        ispDevices.pop();
    else
        LOG(Info) << "Unicam instance " << unicam.toString() << " has detected no cameras.";
}

from libcamera.

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*?

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 --list-cameras ignores cameras that have no IPA tuning file, even if one is provided on the command line. This was on an IMX290 mono sensor when there is only a merged tuning for IMX290 colour.

from libcamera.

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.

I'll scream "Fault tolerance" as loud as I can every time I see this topic ;-)
(But as you were at the media-meeting @6by9, you'll know this is my pet-peeve / hate hehe)

from libcamera.

Absolutely - and it should be able to support any that are working correctly ;-)

from libcamera.

Would something like this help? <pure psuedo code, not compiled>

std::vector<MediaDevice*> unicamDevices;
std::vector<MediaDevice*> ispDevices;

/* Find all Unicam instances */
while(MediaDevice *device = acquireMediaDevice(enumerator, "unicam"))
    unicamDevices.push_back(device);

if (unicamDevices.empty())
    return true; /* No compatible devices */

/* Find all available ISPs */
while (MediaDevice *device = acquireMediaDevice(enumerator, "bcm2835-isp"))
    ispDevices.push_back(device);

/* Iterate each unicam and if it has any cameras, obtain an ISP */
while (MediaDevice *unicam = unicamDevices.pop()) {
    if (ispDevices.empty()) {
        LOG(Error) << "Not enough ISP devices to manage unicam";
        return true;
    }

    /* Only consume the ISP if cameras were registered against it */
    unsigned int cameras = registerCameras(unicam, ispDevices.front());
    if (cameras)
        ispDevices.pop();
    else
        LOG(Info) << "Unicam instance " << unicam.toString() << " has detected no cameras.";
}

Unless I am mistaken, I don't think we can do this unfortunately. In match(), we need to account for the following two scenarios:

1. Enumerating a single camera connected either Unicam 0/1 port.
2. Enumerating multiple cameras connected to a single Unicam port through some mux.
3. Enumerating multiple cameras connected to both Unicam 0/1 ports.
4. Combination of 2 + 3 :-)

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 for loop. Perhaps a simple while(true) will be sufficient?

from libcamera.

If you're looking at how the PH enumerates cameras then I've also had an issue flagged that --list-cameras ignores cameras that have no IPA tuning file, even if one is provided on the command line. This was on an IMX290 mono sensor when there is only a merged tuning for IMX290 colour.

This is an easy one. libcamera-apps only set the tuning file env variable after the --list-cameras routine. I'll move it to earlier in the function and that should fix this. PR is available at raspberrypi/rpicam-apps#476.

from libcamera.

I think the change might have to be closer to what I proposed, minus the hardcoded for loop. Perhaps a simple while(true) will be sufficient?

Looking at this a bit further, I can't see any way to do this cleanly other than a hard-coded for loop. The problem is with PipelineHandler::acquireMediaDevice(). We don't have an equivalent releaseMediaDevice(), so we can never correctly enumerate multiple cameras connected to both Unicam 0/1 ports (scenario 2) across multiple calls to match().

from libcamera.

Ah, but if we abuse match() to enumerate all possible camera instances in one call (so always return false at the end), that would work. Is that going to be ok from a libcamera point of view?

from libcamera.

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.

from libcamera.

(we can of course always add a releaseMediaDevice if we need to if it's better)

from libcamera.

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.

        }

-       return !!numCameras;
+       return true;
 }

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:

        if (!unicamDevice) {
-               LOG(RPI, Debug) << "Unable to acquire a Unicam instance";
                return false;
        }

from libcamera.

The key differentiator is whether each unicam instance should have it's own pipeline handler or can all be managed from a single.

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?

from libcamera.

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

https://github.com/raspberrypi/libcamera/blob/9b860a664e9cd7ca4889c6f8d7f0e8d402199de3/src/libcamera/camera_manager.cpp#L138..L170

from libcamera.

I don't think there's any specific threading differences here though ?

from libcamera.

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 match() call vs multiple match() calls?

from libcamera.

TL;DR; - I think this bug is a one line fix ;-)

I can see that this would solve the problem, but are we abusing the API?

/**
 * \fn PipelineHandler::match(DeviceEnumerator *enumerator)
 * \brief Match media devices and create camera instances
 * \param[in] enumerator The enumerator providing all media devices found in the
 * system

 blah...blah...

 * \return true if media devices have been acquired and camera instances
 * created, or false otherwise

Retuning true implies we have created/registered a Camera object. Will this come back to bite us?

from libcamera.

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 match() call vs multiple match() calls?

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 ...

https://github.com/raspberrypi/libcamera/blob/main/include/libcamera/internal/pipeline_handler.h#L90...L98

	std::vector<std::shared_ptr<MediaDevice>> mediaDevices_;
	std::vector<std::weak_ptr<Camera>> cameras_;

	std::queue<Request *> waitingRequests_;

	const char *name_;

	Mutex lock_;
	unsigned int useCount_ LIBCAMERA_TSA_GUARDED_BY(lock_);

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_. ...

from libcamera.

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 match(). This means I do end up with an outer for loop enumerating over all available Unicam entities. Thoughts?

from libcamera.

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 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?

from libcamera.

from libcamera.

That returns us back to the original suggested fix. I'll post a patch then maybe we discuss from there...

from libcamera.

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?

@kbingham, No problem

It doesn't seems like it fix the issue, here is the output of v4l2-ctl --list-devices
with the following dtoverlay:

dtoverlay=imx283
dtoverlay=imx219,cam0    <==== This one doesn't exists on the actual HW

Dmesg:
[    7.564614] imx219 0-0010: failed to read chip id 219
[    7.565753] imx219: probe of 0-0010 failed with error -5 <==== Because it doesn't exists on the actual HW
[    7.658518] imx283 10-001a: Device found
[    7.659139] imx283 10-001a: Consider updating driver imx283 to match on endpoints

bcm2835-codec-decode (platform:bcm2835-codec):
        /dev/video10
        /dev/video11
        /dev/video12
        /dev/video18
        /dev/video31
        /dev/media3

bcm2835-isp (platform:bcm2835-isp):
        /dev/video13
        /dev/video14
        /dev/video15
        /dev/video16
        /dev/video20
        /dev/video21
        /dev/video22
        /dev/video23
        /dev/media1
        /dev/media2

unicam (platform:fe800000.csi):
        /dev/media4

unicam (platform:fe801000.csi):
        /dev/video0
        /dev/video1
        /dev/media5

rpivid (platform:rpivid):
        /dev/video19
        /dev/media0

libcamera app will hang when opening camera:

pi@camera:~ $ libcamera-hello -t 0 --fullscreen on --verbose
Options:
    verbose: 2
    info_text:#%frame (%fps fps) exp %exp ag %ag dg %dg
    timeout: 0
    width: 0
    height: 0
    output:
    post_process_file:
    rawfull: 0
    preview: fullscreen
    qt-preview: 0
    transform: identity
    roi: all
    metering: centre
    exposure: normal
    ev: 0
    awb: auto
    flush: false
    wrap: 0
    brightness: 0
    contrast: 1
    saturation: 1
    sharpness: 1
    framerate: 30
    denoise: auto
    viewfinder-width: 0
    viewfinder-height: 0
    tuning-file: (libcamera)
    lores-width: 0
    lores-height: 0
    autofocus-range: normal
    autofocus-speed: normal
    autofocus-window: all
    mode: unspecified
    viewfinder-mode: unspecified
    metadata:
    metadata-format: json
No connector ID specified.  Choosing default from list:
Connector 32 (crtc 0): type 11, 0x0
Connector 42 (crtc 107): type 11, 1920x1080 (chosen)
Made DRM preview window
Opening camera...
[0:39:14.509524670] [949]  INFO Camera camera_manager.cpp:299 libcamera v0.0.4+19-58e0b6e1-dirty (2023-02-23T17:14:06-08:00)  <===== Hanging here 

On the same note, libcamera-hello --list-cameras --verbose also hangs.

from libcamera.

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.

from libcamera.

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

from libcamera.

This fix is now merged in upstream libcamera.

from libcamera.