[libcamera-devel] [PATCH v2] libcamera: device_enumerator: ensure deviceNode is not empty

[Kieran Bingham]

Quoting Benjamin Bara via libcamera-devel (2023-06-09 07:14:32)
> From: Benjamin Bara <benjamin.bara at skidata.com>
> 
> When activating both ISP nodes on the i.MX8MP, but only connecting one
> camera sensor, libcamera aborts because it couldn't find the chosen
> entity's device node:

This sounds annoying indeed. If there is a camera there - we should be
able to use it!


> 
> [37:54:40.779902250] [3631] DEBUG DeviceEnumerator device_enumerator.cpp:252 Added device /dev/media1: rkisp1
> [37:54:40.780196750] [3631] DEBUG DeviceEnumerator device_enumerator_udev.cpp:322 All dependencies for media device /dev/media0 found
> [37:54:40.780237875] [3631] DEBUG DeviceEnumerator device_enumerator.cpp:252 Added device /dev/media0: rkisp1
> [37:54:40.780505125] [3631] DEBUG Camera camera_manager.cpp:152 Found registered pipeline handler 'PipelineHandlerRkISP1'
> [37:54:40.780599875] [3631] DEBUG DeviceEnumerator device_enumerator.cpp:312 Successful match for media device "rkisp1"
> [37:54:40.780731375] [3631] ERROR V4L2 v4l2_device.cpp:93 'rkisp1_isp': Failed to open V4L2 device '': No such file or directory
> 
> Fix this by skipping empty device nodes:
> 
> [37:49:05.172672000] [3603] DEBUG DeviceEnumerator device_enumerator_udev.cpp:322 All dependencies for media device /dev/media1 found
> [37:49:05.172720625] [3603] DEBUG DeviceEnumerator device_enumerator.cpp:256 Added device /dev/media1: rkisp1
> [37:49:05.172973875] [3603] DEBUG DeviceEnumerator device_enumerator_udev.cpp:322 All dependencies for media device /dev/media0 found
> [37:49:05.173012125] [3603] DEBUG DeviceEnumerator device_enumerator.cpp:256 Added device /dev/media0: rkisp1
> [37:49:05.173281625] [3603] DEBUG Camera camera_manager.cpp:152 Found registered pipeline handler 'PipelineHandlerRkISP1'
> [37:49:05.173376875] [3603] DEBUG DeviceEnumerator device_enumerator.cpp:107 Skip rkisp1_isp: no device node
> [37:49:05.173414375] [3603] DEBUG DeviceEnumerator device_enumerator.cpp:316 Successful match for media device "rkisp1"
> [37:49:05.173671250] [3603] DEBUG V4L2 v4l2_videodevice.cpp:632 /dev/video1[15:cap]: Opened device platform:rkisp1: rkisp1: rkisp1_stats
> [37:49:05.173775125] [3603] DEBUG V4L2 v4l2_videodevice.cpp:632 /dev/video2[16:out]: Opened device platform:rkisp1: rkisp1: rkisp1_params
> [37:49:05.173880500] [3603] DEBUG V4L2 v4l2_videodevice.cpp:632 /dev/video0[18:cap]: Opened device platform:rkisp1: rkisp1: rkisp1
> 
> Signed-off-by: Benjamin Bara <benjamin.bara at skidata.com>
> ---
> Hi!
> 
> Seems like something went wrong with the format of the log in V1 when
> copying it.
> ---
> Changes in v2:
> - adapt commit message
> - Link to v1: https://lists.libcamera.org/pipermail/libcamera-devel/2023-June/038181.html
> ---
>  src/libcamera/device_enumerator.cpp | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/src/libcamera/device_enumerator.cpp b/src/libcamera/device_enumerator.cpp
> index f2e055de..f2d943d1 100644
> --- a/src/libcamera/device_enumerator.cpp
> +++ b/src/libcamera/device_enumerator.cpp
> @@ -100,9 +100,13 @@ bool DeviceMatch::match(const MediaDevice *device) const
>                 bool found = false;
>  
>                 for (const MediaEntity *entity : device->entities()) {
> -                       if (name == entity->name()) {
> +                       if (name == entity->name() && !entity->deviceNode().empty()) {
>                                 found = true;
>                                 break;
> +                       } else if (name == entity->name()) {
> +                               LOG(DeviceEnumerator, Debug)
> +                                       << "Skip " << entity->name()
> +                                       << ": no device node";

I think I'd be tempted to merge these two like this:

			if (name == entity->name()) {
				if (!entity->deviceNode().empty()) {
					found = true;
					break;
				} else {
					LOG(DeviceEnumerator, Debug)
						<< "Skip " << entity->name()
						<< ": no device node";
				}
			}


Just because that doesn't duplicate the if (name == entity->name())

This seems reasonable to me though - but it would help to see a media
graph to see what it's handling here.

Could you share the output of :

  media-ctl -d /dev/media0 --print-dot

please? (Where /dev/media0 might be different on your device)

My only worry is to be sure that this will be applicable for all
platforms in the same way without breaking anything but it seems
reasonable at the moment so:


Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>


>                         }
>                 }
>  
> 
> ---
> base-commit: 0ee9339331c648232e87d2de2ccd5a92cc61cab2
> change-id: 20230609-empty-devnode-36dd2647cde0
> 
> Best regards,
> -- 
> Benjamin Bara <benjamin.bara at skidata.com>
>