[libcamera-devel] [PATCH] libcamera: device_enumerator: Don't stop if one device fails
Kieran Bingham
kieran.bingham at ideasonboard.com
Mon Feb 17 11:47:02 CET 2020
Hi Laurent,
On 17/02/2020 10:43, Laurent Pinchart wrote:
> Hi Kieran,
>
> On Mon, Feb 17, 2020 at 10:41:23AM +0000, Kieran Bingham wrote:
>> On 17/02/2020 08:23, Kieran Bingham wrote:
>>> On 14/02/2020 12:37, Laurent Pinchart wrote:
>>>> If one device fails to enumerate, which isn't supposed to happen under
>>>> normal conditions, both the sysfs and the udev enumerators stop
>>>> enumeration of further devices. This potentially prevents working
>>>> devices from being detected and handled. Fix it by skipping the faulty
>>>> device.
>>>
>>> Thank you,
>>>
>>> This does indeed allow the system to continue and function with the
>>> available cameras.
>>>
>>> Interestingly, in the particular case that I was investigating - it
>>> wasn't the newly added device that failed ... it was the existing camera
>>> interface.
>>>
>>> That can be investigated separately.
>>>
>>>> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>>
>> Thanks, indeed this now continues to function and produces an
>> appropriate message for the failing device components:
>>
>>> [2:20:54.425174891] [3209] ERR MediaDevice media_device.cpp:242 Failed to enumerate topology: Bad address
>>> [2:20:54.425281647] [3209] INFO DeviceEnumerator device_enumerator.cpp:217 Unable to populate media device /dev/media0 (Invalid argument), skipping
>>> [2:20:54.425393663] [3209] WARN DeviceEnumerator device_enumerator_udev.cpp:149 Failed to add device for '/sys/devices/platform/soc/fe801000.csi/media0', skipping
>>> Using camera VF0520 Live! Cam Sync: VF0520 L
>>
>> Tested-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>>
>> And with one small comment considered below:
>>
>> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>>
>>>> ---
>>>> src/libcamera/device_enumerator_sysfs.cpp | 15 +++++++--------
>>>> src/libcamera/device_enumerator_udev.cpp | 8 +++++---
>>>> 2 files changed, 12 insertions(+), 11 deletions(-)
>>>>
>>>> diff --git a/src/libcamera/device_enumerator_sysfs.cpp b/src/libcamera/device_enumerator_sysfs.cpp
>>>> index ad26affb38a3..bde736226f95 100644
>>>> --- a/src/libcamera/device_enumerator_sysfs.cpp
>>>> +++ b/src/libcamera/device_enumerator_sysfs.cpp
>>>> @@ -33,7 +33,6 @@ int DeviceEnumeratorSysfs::enumerate()
>>>> {
>>>> struct dirent *ent;
>>>> DIR *dir;
>>>> - int ret = 0;
>>>>
>>>> static const char * const sysfs_dirs[] = {
>>>> "/sys/subsystem/media/devices",
>>>> @@ -74,14 +73,14 @@ int DeviceEnumeratorSysfs::enumerate()
>>>> }
>>>>
>>>> std::shared_ptr<MediaDevice> media = createDevice(devnode);
>>>> - if (!media) {
>>>> - ret = -ENODEV;
>>>> - break;
>>>> - }
>>>> + if (!media)
>>>> + continue;
>>>>
>>>> if (populateMediaDevice(media) < 0) {
>>>> - ret = -ENODEV;
>>>> - break;
>>>> + LOG(DeviceEnumerator, Warning)
>>>> + << "Failed to populate media device /dev/media"
>>>> + << idx << ", skipping";
>>
>> Shouldn't this use something like media->deviceNode() ?
>>
>> (Perhaps even printing media->driver() and media->model for guidance too?)
>
> That's a good idea, I'll submit a v2 for you to test :-)
There's also the local variable available which was used to construct
the MediaDevice; - devnode;
But getting the strings from the MediaDevice and being more verbose
about the failure seems a bit more beneficial all the same.
--
Kieran
>
>>>> + continue;
>>>> }
>>>>
>>>> addDevice(media);
>>>> @@ -89,7 +88,7 @@ int DeviceEnumeratorSysfs::enumerate()
>>>>
>>>> closedir(dir);
>>>>
>>>> - return ret;
>>>> + return 0;
>>>> }
>>>>
>>>> int DeviceEnumeratorSysfs::populateMediaDevice(const std::shared_ptr<MediaDevice> &media)
>>>> diff --git a/src/libcamera/device_enumerator_udev.cpp b/src/libcamera/device_enumerator_udev.cpp
>>>> index b2c5fd221dcd..3837aaf97e6a 100644
>>>> --- a/src/libcamera/device_enumerator_udev.cpp
>>>> +++ b/src/libcamera/device_enumerator_udev.cpp
>>>> @@ -145,10 +145,12 @@ int DeviceEnumeratorUdev::enumerate()
>>>> goto done;
>>>> }
>>>>
>>>> - ret = addUdevDevice(dev);
>>>> + if (addUdevDevice(dev) < 0)
>>>> + LOG(DeviceEnumerator, Warning)
>>>> + << "Failed to add device for '"
>>>> + << syspath << "', skipping";
>>>> +
>>>> udev_device_unref(dev);
>>>> - if (ret < 0)
>>>> - break;
>>>> }
>>>>
>>>> done:
>
--
Regards
--
Kieran
More information about the libcamera-devel
mailing list