diff mbox series

[v4,1/3] media: v4l2-dev.c: Add Meta: to the name of metadata devices

Message ID 20220920-resend-meta-v4-1-3ac355b66723@chromium.org
State New
Headers
Series Add Meta: to Metadata devices |

Commit Message

Ricardo Ribalda Dec. 2, 2022, 5:08 p.m. UTC 
  Devices with Metadata output (like uvc), create two video devices, one
for the data itself and another one for the metadata.

Add a "Meta: " to the beginning of the device name, as suggested by Mauro,
to avoid having multiple devices with the same name.

Fixes v4l2-compliance:
Media Controller ioctls:
     fail: v4l2-test-media.cpp(205): v2_entity_names_set.find(key) != v2_entity_names_set.end()
   test MEDIA_IOC_G_TOPOLOGY: FAIL
     fail: v4l2-test-media.cpp(394): num_data_links != num_links
   test MEDIA_IOC_ENUM_ENTITIES/LINKS: FAIL

Suggested-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/media/v4l2-core/v4l2-dev.c | 9 +++++++++
 1 file changed, 9 insertions(+)

Comments

Yunke Cao Dec. 5, 2022, 5:02 a.m. UTC | #1 
On Sat, Dec 3, 2022 at 2:08 AM Ricardo Ribalda <ribalda@chromium.org> wrote:
>
> Devices with Metadata output (like uvc), create two video devices, one
> for the data itself and another one for the metadata.
>
> Add a "Meta: " to the beginning of the device name, as suggested by Mauro,
> to avoid having multiple devices with the same name.
>
> Fixes v4l2-compliance:
> Media Controller ioctls:
>      fail: v4l2-test-media.cpp(205): v2_entity_names_set.find(key) != v2_entity_names_set.end()
>    test MEDIA_IOC_G_TOPOLOGY: FAIL
>      fail: v4l2-test-media.cpp(394): num_data_links != num_links
>    test MEDIA_IOC_ENUM_ENTITIES/LINKS: FAIL
>
> Suggested-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
>  drivers/media/v4l2-core/v4l2-dev.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
> index 397d553177fa..5c2c9ebb6b96 100644
> --- a/drivers/media/v4l2-core/v4l2-dev.c
> +++ b/drivers/media/v4l2-core/v4l2-dev.c
> @@ -901,6 +901,15 @@ int __video_register_device(struct video_device *vdev,
>         if (WARN_ON(type != VFL_TYPE_SUBDEV && !vdev->device_caps))
>                 return -EINVAL;
>
> +       /* Add Meta: to metadata device names */
> +       if (vdev->device_caps &
> +           (V4L2_CAP_META_CAPTURE | V4L2_CAP_META_OUTPUT)) {
> +               char aux[sizeof(vdev->name)];
> +
> +               snprintf(aux, sizeof(aux), "Meta: %s", vdev->name);
> +               strscpy(vdev->name, aux, sizeof(aux));
> +       }
> +
>         /* v4l2_fh support */
>         spin_lock_init(&vdev->fh_lock);
>         INIT_LIST_HEAD(&vdev->fh_list);
>
> --
> 2.39.0.rc0.267.gcb52ba06e7-goog-b4-0.11.0-dev-696ae

Reviewed-by: Yunke Cao <yunkec@chromium.org>
Laurent Pinchart Dec. 5, 2022, 10:03 p.m. UTC | #2 
Hi Ricardo,

Thank you for the patch.

On Fri, Dec 02, 2022 at 06:08:17PM +0100, Ricardo Ribalda wrote:
> Devices with Metadata output (like uvc), create two video devices, one
> for the data itself and another one for the metadata.
> 
> Add a "Meta: " to the beginning of the device name, as suggested by Mauro,
> to avoid having multiple devices with the same name.
> 
> Fixes v4l2-compliance:
> Media Controller ioctls:
>      fail: v4l2-test-media.cpp(205): v2_entity_names_set.find(key) != v2_entity_names_set.end()
>    test MEDIA_IOC_G_TOPOLOGY: FAIL
>      fail: v4l2-test-media.cpp(394): num_data_links != num_links
>    test MEDIA_IOC_ENUM_ENTITIES/LINKS: FAIL
> 
> Suggested-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
>  drivers/media/v4l2-core/v4l2-dev.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
> index 397d553177fa..5c2c9ebb6b96 100644
> --- a/drivers/media/v4l2-core/v4l2-dev.c
> +++ b/drivers/media/v4l2-core/v4l2-dev.c
> @@ -901,6 +901,15 @@ int __video_register_device(struct video_device *vdev,
>  	if (WARN_ON(type != VFL_TYPE_SUBDEV && !vdev->device_caps))
>  		return -EINVAL;
>  
> +	/* Add Meta: to metadata device names */
> +	if (vdev->device_caps &
> +	    (V4L2_CAP_META_CAPTURE | V4L2_CAP_META_OUTPUT)) {
> +		char aux[sizeof(vdev->name)];
> +
> +		snprintf(aux, sizeof(aux), "Meta: %s", vdev->name);

This will break userspace I'm afraid. libcamera looks up video devices
by entity names for multiple platforms. I wouldn't be surprised if other
userspace applications did something similar.

> +		strscpy(vdev->name, aux, sizeof(aux));
> +	}
> +
>  	/* v4l2_fh support */
>  	spin_lock_init(&vdev->fh_lock);
>  	INIT_LIST_HEAD(&vdev->fh_list);
>
Mauro Carvalho Chehab Dec. 21, 2022, 10:16 p.m. UTC | #3 
Em Fri, 02 Dec 2022 18:08:17 +0100
Ricardo Ribalda <ribalda@chromium.org> escreveu:

> Devices with Metadata output (like uvc), create two video devices, one
> for the data itself and another one for the metadata.
> 
> Add a "Meta: " to the beginning of the device name, as suggested by Mauro,
> to avoid having multiple devices with the same name.

Hmm... I don't remember when I suggested it, but I suspect that such
change could cause regressions on userspace.

I double-checked: the two apps I know that have code to detect meta
devices (Camorama and ZBar) won't be affected by name changes, as they
always open v4l2 devnodes before start seeing them as capture devices,
and run VIDIOC_QUERYCAP to check for V4L2_CAP_VIDEO_CAPTURE:

   if (first_device < 0) {
        fd = open(file, O_RDWR);
        if (fd < 0) {
            devices[n_devices].is_valid = FALSE;
        } else {
            if (ioctl(fd, VIDIOC_QUERYCAP, &vid_cap) == -1) {
                devices[n_devices].is_valid = FALSE;
            } else if (!(vid_cap.device_caps & V4L2_CAP_VIDEO_CAPTURE)) {
                devices[n_devices].is_valid = FALSE;
            } else {
                n_valid_devices++;
                devices[n_devices].is_valid = TRUE;
            }
        }

        close(fd);
    } else {
        devices[n_devices].is_valid = devices[first_device].is_valid;
    }

You need to double-check if this won't cause userspace regressions, specially
on browsers, as they have more complex logic, and seem to have already some
logic to detect and remove meta files.

> 
> Fixes v4l2-compliance:
> Media Controller ioctls:
>      fail: v4l2-test-media.cpp(205): v2_entity_names_set.find(key) != v2_entity_names_set.end()
>    test MEDIA_IOC_G_TOPOLOGY: FAIL
>      fail: v4l2-test-media.cpp(394): num_data_links != num_links
>    test MEDIA_IOC_ENUM_ENTITIES/LINKS: FAIL
> 
> Suggested-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
>  drivers/media/v4l2-core/v4l2-dev.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
> index 397d553177fa..5c2c9ebb6b96 100644
> --- a/drivers/media/v4l2-core/v4l2-dev.c
> +++ b/drivers/media/v4l2-core/v4l2-dev.c
> @@ -901,6 +901,15 @@ int __video_register_device(struct video_device *vdev,
>  	if (WARN_ON(type != VFL_TYPE_SUBDEV && !vdev->device_caps))
>  		return -EINVAL;
>  
> +	/* Add Meta: to metadata device names */
> +	if (vdev->device_caps &
> +	    (V4L2_CAP_META_CAPTURE | V4L2_CAP_META_OUTPUT)) {
> +		char aux[sizeof(vdev->name)];
> +
> +		snprintf(aux, sizeof(aux), "Meta: %s", vdev->name);
> +		strscpy(vdev->name, aux, sizeof(aux));
> +	}
> +
>  	/* v4l2_fh support */
>  	spin_lock_init(&vdev->fh_lock);
>  	INIT_LIST_HEAD(&vdev->fh_list);
>
Laurent Pinchart Dec. 21, 2022, 10:21 p.m. UTC | #4 
Hi Mauro,

On Wed, Dec 21, 2022 at 10:16:01PM +0000, Mauro Carvalho Chehab wrote:
> Em Fri, 02 Dec 2022 18:08:17 +0100 Ricardo Ribalda escreveu:
> 
> > Devices with Metadata output (like uvc), create two video devices, one
> > for the data itself and another one for the metadata.
> > 
> > Add a "Meta: " to the beginning of the device name, as suggested by Mauro,
> > to avoid having multiple devices with the same name.
> 
> Hmm... I don't remember when I suggested it, but I suspect that such
> change could cause regressions on userspace.
> 
> I double-checked: the two apps I know that have code to detect meta
> devices (Camorama and ZBar) won't be affected by name changes, as they
> always open v4l2 devnodes before start seeing them as capture devices,
> and run VIDIOC_QUERYCAP to check for V4L2_CAP_VIDEO_CAPTURE:
> 
>    if (first_device < 0) {
>         fd = open(file, O_RDWR);
>         if (fd < 0) {
>             devices[n_devices].is_valid = FALSE;
>         } else {
>             if (ioctl(fd, VIDIOC_QUERYCAP, &vid_cap) == -1) {
>                 devices[n_devices].is_valid = FALSE;
>             } else if (!(vid_cap.device_caps & V4L2_CAP_VIDEO_CAPTURE)) {
>                 devices[n_devices].is_valid = FALSE;
>             } else {
>                 n_valid_devices++;
>                 devices[n_devices].is_valid = TRUE;
>             }
>         }
> 
>         close(fd);
>     } else {
>         devices[n_devices].is_valid = devices[first_device].is_valid;
>     }
> 
> You need to double-check if this won't cause userspace regressions, specially
> on browsers, as they have more complex logic, and seem to have already some
> logic to detect and remove meta files.

As commented separately in the same mail thread, this change would break
libcamera. For drivers that have well-known names for entities, we look
up some entities by name.

For the uvcvideo driver, which is affected by the v4l2-compliance
failure, a prefix (or suffix, or anything else) would be fine, but we
can't do that unconditionally for all devices.

> > Fixes v4l2-compliance:
> > Media Controller ioctls:
> >      fail: v4l2-test-media.cpp(205): v2_entity_names_set.find(key) != v2_entity_names_set.end()
> >    test MEDIA_IOC_G_TOPOLOGY: FAIL
> >      fail: v4l2-test-media.cpp(394): num_data_links != num_links
> >    test MEDIA_IOC_ENUM_ENTITIES/LINKS: FAIL
> > 
> > Suggested-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > ---
> >  drivers/media/v4l2-core/v4l2-dev.c | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> > 
> > diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
> > index 397d553177fa..5c2c9ebb6b96 100644
> > --- a/drivers/media/v4l2-core/v4l2-dev.c
> > +++ b/drivers/media/v4l2-core/v4l2-dev.c
> > @@ -901,6 +901,15 @@ int __video_register_device(struct video_device *vdev,
> >  	if (WARN_ON(type != VFL_TYPE_SUBDEV && !vdev->device_caps))
> >  		return -EINVAL;
> >  
> > +	/* Add Meta: to metadata device names */
> > +	if (vdev->device_caps &
> > +	    (V4L2_CAP_META_CAPTURE | V4L2_CAP_META_OUTPUT)) {
> > +		char aux[sizeof(vdev->name)];
> > +
> > +		snprintf(aux, sizeof(aux), "Meta: %s", vdev->name);
> > +		strscpy(vdev->name, aux, sizeof(aux));
> > +	}
> > +
> >  	/* v4l2_fh support */
> >  	spin_lock_init(&vdev->fh_lock);
> >  	INIT_LIST_HEAD(&vdev->fh_list);
diff mbox series

Patch

diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
index 397d553177fa..5c2c9ebb6b96 100644
--- a/drivers/media/v4l2-core/v4l2-dev.c
+++ b/drivers/media/v4l2-core/v4l2-dev.c
@@ -901,6 +901,15 @@  int __video_register_device(struct video_device *vdev,
 	if (WARN_ON(type != VFL_TYPE_SUBDEV && !vdev->device_caps))
 		return -EINVAL;
 
+	/* Add Meta: to metadata device names */
+	if (vdev->device_caps &
+	    (V4L2_CAP_META_CAPTURE | V4L2_CAP_META_OUTPUT)) {
+		char aux[sizeof(vdev->name)];
+
+		snprintf(aux, sizeof(aux), "Meta: %s", vdev->name);
+		strscpy(vdev->name, aux, sizeof(aux));
+	}
+
 	/* v4l2_fh support */
 	spin_lock_init(&vdev->fh_lock);
 	INIT_LIST_HEAD(&vdev->fh_list);