[PATCH v4 0/3] Add Meta: to Metadata devices

[PATCH v4 0/3] Add Meta: to Metadata devices

[Ricardo Ribalda]

Metadata devices usually companion "real" devices. In order to
distinguish them properly, add the Meta: prefix to their names.

Also, add a unique suffix to all the uvc devices, to multisensor cameras
do not show the same names for all their devices (IR, RBG....).

v4:
- Rebase to latest master

v3:

- Add the meta logic to the core

v2: uvc: Restore old vdev name

To: Mauro Carvalho Chehab <mchehab@kernel.org>
To: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To: Yunke Cao <yunkec@chromium.org>
To: Sergey Senozhatsky <senozhatsky@chromium.org>
Cc: linux-media@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>

---
Changes in v4:
- Link to v3: https://lore.kernel.org/r/20220920-resend-meta-v1-0-dfcfba923204@chromium.org

---
Ricardo Ribalda (3):
      media: v4l2-dev.c: Add Meta: to the name of metadata devices
      media: Documentation/driver-api: Document device name
      media: uvcvideo: Add a unique suffix to camera names

 Documentation/driver-api/media/v4l2-dev.rst | 4 +++-
 drivers/media/usb/uvc/uvc_driver.c          | 3 ++-
 drivers/media/v4l2-core/v4l2-dev.c          | 9 +++++++++
 3 files changed, 14 insertions(+), 2 deletions(-)
---
base-commit: a4412fdd49dc011bcc2c0d81ac4cab7457092650
change-id: 20220920-resend-meta-435c30209235

Best regards,
-- 
Ricardo Ribalda <ribalda@chromium.org>

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

[Ricardo Ribalda]

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

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

[Yunke Cao]

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>

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

[Laurent Pinchart]

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

-- 
Regards,

Laurent Pinchart

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

[Mauro Carvalho Chehab]

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

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

[Laurent Pinchart]

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

-- 
Regards,

Laurent Pinchart

[PATCH v4 2/3] media: Documentation/driver-api: Document device name

[Ricardo Ribalda]

Document how the name of the metadata devices is modified.

Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 Documentation/driver-api/media/v4l2-dev.rst | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/Documentation/driver-api/media/v4l2-dev.rst b/Documentation/driver-api/media/v4l2-dev.rst
index 99e3b5fa7444..935a46e29c5e 100644
--- a/Documentation/driver-api/media/v4l2-dev.rst
+++ b/Documentation/driver-api/media/v4l2-dev.rst
@@ -42,7 +42,9 @@ You should also set these fields of :c:type:`video_device`:
 - :c:type:`video_device`->v4l2_dev: must be set to the :c:type:`v4l2_device`
   parent device.
 
-- :c:type:`video_device`->name: set to something descriptive and unique.
+- :c:type:`video_device`->name: set to something descriptive and unique. If the
+  device has the `V4L2_CAP_META_CAPTURE` or `V4L2_CAP_META_OUTPUT` capabilities,
+  the string `Meta:` will be inserted before the original name.
 
 - :c:type:`video_device`->vfl_dir: set this to ``VFL_DIR_RX`` for capture
   devices (``VFL_DIR_RX`` has value 0, so this is normally already the

-- 
2.39.0.rc0.267.gcb52ba06e7-goog-b4-0.11.0-dev-696ae

Re: [PATCH v4 2/3] media: Documentation/driver-api: Document device name

[Yunke Cao]

On Sat, Dec 3, 2022 at 2:08 AM Ricardo Ribalda <ribalda@chromium.org> wrote:
>
> Document how the name of the metadata devices is modified.
>
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
>  Documentation/driver-api/media/v4l2-dev.rst | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/driver-api/media/v4l2-dev.rst b/Documentation/driver-api/media/v4l2-dev.rst
> index 99e3b5fa7444..935a46e29c5e 100644
> --- a/Documentation/driver-api/media/v4l2-dev.rst
> +++ b/Documentation/driver-api/media/v4l2-dev.rst
> @@ -42,7 +42,9 @@ You should also set these fields of :c:type:`video_device`:
>  - :c:type:`video_device`->v4l2_dev: must be set to the :c:type:`v4l2_device`
>    parent device.
>
> -- :c:type:`video_device`->name: set to something descriptive and unique.
> +- :c:type:`video_device`->name: set to something descriptive and unique. If the
> +  device has the `V4L2_CAP_META_CAPTURE` or `V4L2_CAP_META_OUTPUT` capabilities,
> +  the string `Meta:` will be inserted before the original name.
>
>  - :c:type:`video_device`->vfl_dir: set this to ``VFL_DIR_RX`` for capture
>    devices (``VFL_DIR_RX`` has value 0, so this is normally already the
>
> --
> 2.39.0.rc0.267.gcb52ba06e7-goog-b4-0.11.0-dev-696ae
Reviewed-by: Yunke Cao <yunkec@chromium.org>

[PATCH v4 3/3] media: uvcvideo: Add a unique suffix to camera names

[Ricardo Ribalda]

Some cameras have multiple data inputs (i.e. IR sensor and RGB sensor),
append a unique number to the device 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

Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/media/usb/uvc/uvc_driver.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
index 215fb483efb0..f4032ebb3689 100644
--- a/drivers/media/usb/uvc/uvc_driver.c
+++ b/drivers/media/usb/uvc/uvc_driver.c
@@ -1963,7 +1963,8 @@ int uvc_register_video_device(struct uvc_device *dev,
 		break;
 	}
 
-	strscpy(vdev->name, dev->name, sizeof(vdev->name));
+	snprintf(vdev->name, sizeof(vdev->name), "%s %u", dev->name,
+		 stream->header.bTerminalLink);
 
 	/*
 	 * Set the driver data before calling video_register_device, otherwise

-- 
2.39.0.rc0.267.gcb52ba06e7-goog-b4-0.11.0-dev-696ae

Re: [PATCH v4 3/3] media: uvcvideo: Add a unique suffix to camera names

[Yunke Cao]

On Sat, Dec 3, 2022 at 2:08 AM Ricardo Ribalda <ribalda@chromium.org> wrote:
>
> Some cameras have multiple data inputs (i.e. IR sensor and RGB sensor),
> append a unique number to the device 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
>
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
>  drivers/media/usb/uvc/uvc_driver.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> index 215fb483efb0..f4032ebb3689 100644
> --- a/drivers/media/usb/uvc/uvc_driver.c
> +++ b/drivers/media/usb/uvc/uvc_driver.c
> @@ -1963,7 +1963,8 @@ int uvc_register_video_device(struct uvc_device *dev,
>                 break;
>         }
>
> -       strscpy(vdev->name, dev->name, sizeof(vdev->name));
> +       snprintf(vdev->name, sizeof(vdev->name), "%s %u", dev->name,
> +                stream->header.bTerminalLink);
>
>         /*
>          * Set the driver data before calling video_register_device, otherwise
>
> --
> 2.39.0.rc0.267.gcb52ba06e7-goog-b4-0.11.0-dev-696ae

Reviewed-by: Yunke Cao <yunkec@chromium.org>

Re: [PATCH v4 3/3] media: uvcvideo: Add a unique suffix to camera names

[Laurent Pinchart]

Hi Ricardo,

Thank you for the patch.

On Fri, Dec 02, 2022 at 06:08:19PM +0100, Ricardo Ribalda wrote:
> Some cameras have multiple data inputs (i.e. IR sensor and RGB sensor),
> append a unique number to the device 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
> 
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
>  drivers/media/usb/uvc/uvc_driver.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> index 215fb483efb0..f4032ebb3689 100644
> --- a/drivers/media/usb/uvc/uvc_driver.c
> +++ b/drivers/media/usb/uvc/uvc_driver.c
> @@ -1963,7 +1963,8 @@ int uvc_register_video_device(struct uvc_device *dev,
>  		break;
>  	}
>  
> -	strscpy(vdev->name, dev->name, sizeof(vdev->name));
> +	snprintf(vdev->name, sizeof(vdev->name), "%s %u", dev->name,
> +		 stream->header.bTerminalLink);

This won't be perfect as the string is not guaranteed to fit in
vdev->name, but I suppose it will help as a quick fix for some devices.
How about the other devices ? Won't they still exhibit the above
v4l2-compliance failure ? Isn't that something that will still affect
Chrome OS devices ?

The change should not cause any regression as big as in patch 1/3.
However, unless I'm mistaken users will notice a device name change,
especially when selecting a device in their web browser. Could that be a
problem ?

>  	/*
>  	 * Set the driver data before calling video_register_device, otherwise
> 

-- 
Regards,

Laurent Pinchart

Re: [PATCH v4 3/3] media: uvcvideo: Add a unique suffix to camera names

[Ricardo Ribalda]

Hi Laurent

On Mon, 5 Dec 2022 at 23:16, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Ricardo,
>
> Thank you for the patch.
>
> On Fri, Dec 02, 2022 at 06:08:19PM +0100, Ricardo Ribalda wrote:
> > Some cameras have multiple data inputs (i.e. IR sensor and RGB sensor),
> > append a unique number to the device 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
> >
> > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > ---
> >  drivers/media/usb/uvc/uvc_driver.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> > index 215fb483efb0..f4032ebb3689 100644
> > --- a/drivers/media/usb/uvc/uvc_driver.c
> > +++ b/drivers/media/usb/uvc/uvc_driver.c
> > @@ -1963,7 +1963,8 @@ int uvc_register_video_device(struct uvc_device *dev,
> >               break;
> >       }
> >
> > -     strscpy(vdev->name, dev->name, sizeof(vdev->name));
> > +     snprintf(vdev->name, sizeof(vdev->name), "%s %u", dev->name,
> > +              stream->header.bTerminalLink);
>
> This won't be perfect as the string is not guaranteed to fit in
> vdev->name, but I suppose it will help as a quick fix for some devices.
> How about the other devices ? Won't they still exhibit the above
> v4l2-compliance failure ? Isn't that something that will still affect
> Chrome OS devices ?

We could place the id first... but that will look bad: Eg:

1- My favorite camera

Another option is to remove the last chars to fit the id. Eg:

My favorite came-1

If you prefer any of those options or have a better idea I can implement that.


>
> The change should not cause any regression as big as in patch 1/3.
> However, unless I'm mistaken users will notice a device name change,
> especially when selecting a device in their web browser. Could that be a
> problem ?

I think the only side effect is that the first time that the kernel
changes the naming convention, if there are more than one camera on
the system, the video conference might pick a different camera.
The good news is that the user will be presented with cameras with
different names. Now some cameras show very confusing names:

ribalda@alco:~/work/linux$ for a in /dev/video* ; do yavta -l $a| grep
"Dell Webcam"; done
Device `Dell Webcam WB7022' on `usb-0000:2d:00.0-1.2.3.1' (driver
'uvcvideo') supports video, capture, without mplanes.
Device `Dell Webcam WB7022' on `usb-0000:2d:00.0-1.2.3.1' (driver
'uvcvideo') supports meta-data, capture, without mplanes.
Device `Dell Webcam WB7022' on `usb-0000:2d:00.0-1.2.3.1' (driver
'uvcvideo') supports video, capture, without mplanes.
Device `Dell Webcam WB7022' on `usb-0000:2d:00.0-1.2.3.1' (driver
'uvcvideo') supports meta-data, capture, without mplanes.



>
> >       /*
> >        * Set the driver data before calling video_register_device, otherwise
> >
>
> --
> Regards,
>
> Laurent Pinchart



-- 
Ricardo Ribalda

Re: [PATCH v4 3/3] media: uvcvideo: Add a unique suffix to camera names

[Ricardo Ribalda]

Hi

On Tue, 6 Dec 2022 at 00:02, Ricardo Ribalda <ribalda@chromium.org> wrote:
>
> Hi Laurent
>
> On Mon, 5 Dec 2022 at 23:16, Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> wrote:
> >
> > Hi Ricardo,
> >
> > Thank you for the patch.
> >
> > On Fri, Dec 02, 2022 at 06:08:19PM +0100, Ricardo Ribalda wrote:
> > > Some cameras have multiple data inputs (i.e. IR sensor and RGB sensor),
> > > append a unique number to the device 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
> > >
> > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > > ---
> > >  drivers/media/usb/uvc/uvc_driver.c | 3 ++-
> > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> > > index 215fb483efb0..f4032ebb3689 100644
> > > --- a/drivers/media/usb/uvc/uvc_driver.c
> > > +++ b/drivers/media/usb/uvc/uvc_driver.c
> > > @@ -1963,7 +1963,8 @@ int uvc_register_video_device(struct uvc_device *dev,
> > >               break;
> > >       }
> > >
> > > -     strscpy(vdev->name, dev->name, sizeof(vdev->name));
> > > +     snprintf(vdev->name, sizeof(vdev->name), "%s %u", dev->name,
> > > +              stream->header.bTerminalLink);
> >
> > This won't be perfect as the string is not guaranteed to fit in
> > vdev->name, but I suppose it will help as a quick fix for some devices.
> > How about the other devices ? Won't they still exhibit the above
> > v4l2-compliance failure ? Isn't that something that will still affect
> > Chrome OS devices ?
>
> We could place the id first... but that will look bad: Eg:
>
> 1- My favorite camera
>
> Another option is to remove the last chars to fit the id. Eg:
>
> My favorite came-1
>
> If you prefer any of those options or have a better idea I can implement that.

@Laurent

Any preference here?

Thanks!

>
>
> >
> > The change should not cause any regression as big as in patch 1/3.
> > However, unless I'm mistaken users will notice a device name change,
> > especially when selecting a device in their web browser. Could that be a
> > problem ?
>
> I think the only side effect is that the first time that the kernel
> changes the naming convention, if there are more than one camera on
> the system, the video conference might pick a different camera.
> The good news is that the user will be presented with cameras with
> different names. Now some cameras show very confusing names:
>
> ribalda@alco:~/work/linux$ for a in /dev/video* ; do yavta -l $a| grep
> "Dell Webcam"; done
> Device `Dell Webcam WB7022' on `usb-0000:2d:00.0-1.2.3.1' (driver
> 'uvcvideo') supports video, capture, without mplanes.
> Device `Dell Webcam WB7022' on `usb-0000:2d:00.0-1.2.3.1' (driver
> 'uvcvideo') supports meta-data, capture, without mplanes.
> Device `Dell Webcam WB7022' on `usb-0000:2d:00.0-1.2.3.1' (driver
> 'uvcvideo') supports video, capture, without mplanes.
> Device `Dell Webcam WB7022' on `usb-0000:2d:00.0-1.2.3.1' (driver
> 'uvcvideo') supports meta-data, capture, without mplanes.
>
>
>
> >
> > >       /*
> > >        * Set the driver data before calling video_register_device, otherwise
> > >
> >
> > --
> > Regards,
> >
> > Laurent Pinchart
>
>
>
> --
> Ricardo Ribalda



-- 
Ricardo Ribalda

Re: [PATCH v4 3/3] media: uvcvideo: Add a unique suffix to camera names

[Laurent Pinchart]

Hi Ricardo,

On Wed, Dec 21, 2022 at 12:00:58AM +0100, Ricardo Ribalda wrote:
> On Tue, 6 Dec 2022 at 00:02, Ricardo Ribalda wrote:
> > On Mon, 5 Dec 2022 at 23:16, Laurent Pinchart wrote:
> > > On Fri, Dec 02, 2022 at 06:08:19PM +0100, Ricardo Ribalda wrote:
> > > > Some cameras have multiple data inputs (i.e. IR sensor and RGB sensor),

Did you mean "data outputs" by the way ?

> > > > append a unique number to the device 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
> > > >
> > > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > > > ---
> > > >  drivers/media/usb/uvc/uvc_driver.c | 3 ++-
> > > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> > > > index 215fb483efb0..f4032ebb3689 100644
> > > > --- a/drivers/media/usb/uvc/uvc_driver.c
> > > > +++ b/drivers/media/usb/uvc/uvc_driver.c
> > > > @@ -1963,7 +1963,8 @@ int uvc_register_video_device(struct uvc_device *dev,
> > > >               break;
> > > >       }
> > > >
> > > > -     strscpy(vdev->name, dev->name, sizeof(vdev->name));
> > > > +     snprintf(vdev->name, sizeof(vdev->name), "%s %u", dev->name,
> > > > +              stream->header.bTerminalLink);
> > >
> > > This won't be perfect as the string is not guaranteed to fit in
> > > vdev->name, but I suppose it will help as a quick fix for some devices.
> > > How about the other devices ? Won't they still exhibit the above
> > > v4l2-compliance failure ? Isn't that something that will still affect
> > > Chrome OS devices ?
> >
> > We could place the id first... but that will look bad: Eg:
> >
> > 1- My favorite camera
> >
> > Another option is to remove the last chars to fit the id. Eg:
> >
> > My favorite came-1
> >
> > If you prefer any of those options or have a better idea I can implement that.
> 
> @Laurent
> 
> Any preference here?

I think the latter is better. Could we do so only when there are
multiple video capture devices (excluding the metadata device) though ?
That way we won't have a weird "-n" suffix in the majority of use cases.

> > > The change should not cause any regression as big as in patch 1/3.
> > > However, unless I'm mistaken users will notice a device name change,
> > > especially when selecting a device in their web browser. Could that be a
> > > problem ?
> >
> > I think the only side effect is that the first time that the kernel
> > changes the naming convention, if there are more than one camera on
> > the system, the video conference might pick a different camera.
> > The good news is that the user will be presented with cameras with
> > different names. Now some cameras show very confusing names:
> >
> > ribalda@alco:~/work/linux$ for a in /dev/video* ; do yavta -l $a| grep
> > "Dell Webcam"; done
> > Device `Dell Webcam WB7022' on `usb-0000:2d:00.0-1.2.3.1' (driver
> > 'uvcvideo') supports video, capture, without mplanes.
> > Device `Dell Webcam WB7022' on `usb-0000:2d:00.0-1.2.3.1' (driver
> > 'uvcvideo') supports meta-data, capture, without mplanes.
> > Device `Dell Webcam WB7022' on `usb-0000:2d:00.0-1.2.3.1' (driver
> > 'uvcvideo') supports video, capture, without mplanes.
> > Device `Dell Webcam WB7022' on `usb-0000:2d:00.0-1.2.3.1' (driver
> > 'uvcvideo') supports meta-data, capture, without mplanes.

I'm tempted to add a new model read-only string control to overcome the
length limitation. It could then be combined with other information
(such as the location and supported pixel formats) to create a
user-friendly camera name by applications.

> > > >       /*
> > > >        * Set the driver data before calling video_register_device, otherwise
> > > >

-- 
Regards,

Laurent Pinchart

Re: [PATCH v4 3/3] media: uvcvideo: Add a unique suffix to camera names

[Ricardo Ribalda]

Hi Laurent

On Wed, 21 Dec 2022 at 11:55, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Ricardo,
>
> On Wed, Dec 21, 2022 at 12:00:58AM +0100, Ricardo Ribalda wrote:
> > On Tue, 6 Dec 2022 at 00:02, Ricardo Ribalda wrote:
> > > On Mon, 5 Dec 2022 at 23:16, Laurent Pinchart wrote:
> > > > On Fri, Dec 02, 2022 at 06:08:19PM +0100, Ricardo Ribalda wrote:
> > > > > Some cameras have multiple data inputs (i.e. IR sensor and RGB sensor),
>
> Did you mean "data outputs" by the way ?
>
> > > > > append a unique number to the device 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
> > > > >
> > > > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > > > > ---
> > > > >  drivers/media/usb/uvc/uvc_driver.c | 3 ++-
> > > > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> > > > > index 215fb483efb0..f4032ebb3689 100644
> > > > > --- a/drivers/media/usb/uvc/uvc_driver.c
> > > > > +++ b/drivers/media/usb/uvc/uvc_driver.c
> > > > > @@ -1963,7 +1963,8 @@ int uvc_register_video_device(struct uvc_device *dev,
> > > > >               break;
> > > > >       }
> > > > >
> > > > > -     strscpy(vdev->name, dev->name, sizeof(vdev->name));
> > > > > +     snprintf(vdev->name, sizeof(vdev->name), "%s %u", dev->name,
> > > > > +              stream->header.bTerminalLink);
> > > >
> > > > This won't be perfect as the string is not guaranteed to fit in
> > > > vdev->name, but I suppose it will help as a quick fix for some devices.
> > > > How about the other devices ? Won't they still exhibit the above
> > > > v4l2-compliance failure ? Isn't that something that will still affect
> > > > Chrome OS devices ?
> > >
> > > We could place the id first... but that will look bad: Eg:
> > >
> > > 1- My favorite camera
> > >
> > > Another option is to remove the last chars to fit the id. Eg:
> > >
> > > My favorite came-1
> > >
> > > If you prefer any of those options or have a better idea I can implement that.
> >
> > @Laurent
> >
> > Any preference here?
>
> I think the latter is better. Could we do so only when there are
> multiple video capture devices (excluding the metadata device) though ?
> That way we won't have a weird "-n" suffix in the majority of use cases.
>
> > > > The change should not cause any regression as big as in patch 1/3.
> > > > However, unless I'm mistaken users will notice a device name change,
> > > > especially when selecting a device in their web browser. Could that be a
> > > > problem ?
> > >
> > > I think the only side effect is that the first time that the kernel
> > > changes the naming convention, if there are more than one camera on
> > > the system, the video conference might pick a different camera.
> > > The good news is that the user will be presented with cameras with
> > > different names. Now some cameras show very confusing names:
> > >
> > > ribalda@alco:~/work/linux$ for a in /dev/video* ; do yavta -l $a| grep
> > > "Dell Webcam"; done
> > > Device `Dell Webcam WB7022' on `usb-0000:2d:00.0-1.2.3.1' (driver
> > > 'uvcvideo') supports video, capture, without mplanes.
> > > Device `Dell Webcam WB7022' on `usb-0000:2d:00.0-1.2.3.1' (driver
> > > 'uvcvideo') supports meta-data, capture, without mplanes.
> > > Device `Dell Webcam WB7022' on `usb-0000:2d:00.0-1.2.3.1' (driver
> > > 'uvcvideo') supports video, capture, without mplanes.
> > > Device `Dell Webcam WB7022' on `usb-0000:2d:00.0-1.2.3.1' (driver
> > > 'uvcvideo') supports meta-data, capture, without mplanes.
>
> I'm tempted to add a new model read-only string control to overcome the
> length limitation. It could then be combined with other information
> (such as the location and supported pixel formats) to create a
> user-friendly camera name by applications.

Adding the vid:pid would be really useful! Mapping a /dev/videoX to
vid:pid is kind of complicated now.

Thanks!
>
> > > > >       /*
> > > > >        * Set the driver data before calling video_register_device, otherwise
> > > > >
>
> --
> Regards,
>
> Laurent Pinchart



-- 
Ricardo Ribalda

Re: [PATCH v4 3/3] media: uvcvideo: Add a unique suffix to camera names

[Laurent Pinchart]

Hi Ricardo,

On Wed, Dec 21, 2022 at 11:57:48AM +0100, Ricardo Ribalda wrote:
> On Wed, 21 Dec 2022 at 11:55, Laurent Pinchart wrote:
> > On Wed, Dec 21, 2022 at 12:00:58AM +0100, Ricardo Ribalda wrote:
> > > On Tue, 6 Dec 2022 at 00:02, Ricardo Ribalda wrote:
> > > > On Mon, 5 Dec 2022 at 23:16, Laurent Pinchart wrote:
> > > > > On Fri, Dec 02, 2022 at 06:08:19PM +0100, Ricardo Ribalda wrote:
> > > > > > Some cameras have multiple data inputs (i.e. IR sensor and RGB sensor),
> >
> > Did you mean "data outputs" by the way ?
> >
> > > > > > append a unique number to the device 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
> > > > > >
> > > > > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > > > > > ---
> > > > > >  drivers/media/usb/uvc/uvc_driver.c | 3 ++-
> > > > > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> > > > > > index 215fb483efb0..f4032ebb3689 100644
> > > > > > --- a/drivers/media/usb/uvc/uvc_driver.c
> > > > > > +++ b/drivers/media/usb/uvc/uvc_driver.c
> > > > > > @@ -1963,7 +1963,8 @@ int uvc_register_video_device(struct uvc_device *dev,
> > > > > >               break;
> > > > > >       }
> > > > > >
> > > > > > -     strscpy(vdev->name, dev->name, sizeof(vdev->name));
> > > > > > +     snprintf(vdev->name, sizeof(vdev->name), "%s %u", dev->name,
> > > > > > +              stream->header.bTerminalLink);
> > > > >
> > > > > This won't be perfect as the string is not guaranteed to fit in
> > > > > vdev->name, but I suppose it will help as a quick fix for some devices.
> > > > > How about the other devices ? Won't they still exhibit the above
> > > > > v4l2-compliance failure ? Isn't that something that will still affect
> > > > > Chrome OS devices ?
> > > >
> > > > We could place the id first... but that will look bad: Eg:
> > > >
> > > > 1- My favorite camera
> > > >
> > > > Another option is to remove the last chars to fit the id. Eg:
> > > >
> > > > My favorite came-1
> > > >
> > > > If you prefer any of those options or have a better idea I can implement that.
> > >
> > > @Laurent
> > >
> > > Any preference here?
> >
> > I think the latter is better. Could we do so only when there are
> > multiple video capture devices (excluding the metadata device) though ?
> > That way we won't have a weird "-n" suffix in the majority of use cases.
> >
> > > > > The change should not cause any regression as big as in patch 1/3.
> > > > > However, unless I'm mistaken users will notice a device name change,
> > > > > especially when selecting a device in their web browser. Could that be a
> > > > > problem ?
> > > >
> > > > I think the only side effect is that the first time that the kernel
> > > > changes the naming convention, if there are more than one camera on
> > > > the system, the video conference might pick a different camera.
> > > > The good news is that the user will be presented with cameras with
> > > > different names. Now some cameras show very confusing names:
> > > >
> > > > ribalda@alco:~/work/linux$ for a in /dev/video* ; do yavta -l $a| grep
> > > > "Dell Webcam"; done
> > > > Device `Dell Webcam WB7022' on `usb-0000:2d:00.0-1.2.3.1' (driver
> > > > 'uvcvideo') supports video, capture, without mplanes.
> > > > Device `Dell Webcam WB7022' on `usb-0000:2d:00.0-1.2.3.1' (driver
> > > > 'uvcvideo') supports meta-data, capture, without mplanes.
> > > > Device `Dell Webcam WB7022' on `usb-0000:2d:00.0-1.2.3.1' (driver
> > > > 'uvcvideo') supports video, capture, without mplanes.
> > > > Device `Dell Webcam WB7022' on `usb-0000:2d:00.0-1.2.3.1' (driver
> > > > 'uvcvideo') supports meta-data, capture, without mplanes.
> >
> > I'm tempted to add a new model read-only string control to overcome the
> > length limitation. It could then be combined with other information
> > (such as the location and supported pixel formats) to create a
> > user-friendly camera name by applications.
> 
> Adding the vid:pid would be really useful! Mapping a /dev/videoX to
> vid:pid is kind of complicated now.

libcamera can help there ;-) We already extract the vendor and product
ID. They are only used to create the camera ID at the moment, they are
not exposed to applications independently. That would be a good
addition.

Overall, device naming is something that we have decided *not* to handle
in libcamera. We provide information to applications to help them
construct a meaningful name, but don't create the name ourselves. This
was decided because naming schemes are dependent on application use
cases, and in many cases should be localized (e.g. "Front camera" and
"Back camera" vs. "Etukamera" and "Takakamera").

> > > > > >       /*
> > > > > >        * Set the driver data before calling video_register_device, otherwise
> > > > > >

-- 
Regards,

Laurent Pinchart

Re: [PATCH v4 3/3] media: uvcvideo: Add a unique suffix to camera names

[Ricardo Ribalda]

Hi Laurent

On Wed, 21 Dec 2022 at 12:23, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Ricardo,
>
> On Wed, Dec 21, 2022 at 11:57:48AM +0100, Ricardo Ribalda wrote:
> > On Wed, 21 Dec 2022 at 11:55, Laurent Pinchart wrote:
> > > On Wed, Dec 21, 2022 at 12:00:58AM +0100, Ricardo Ribalda wrote:
> > > > On Tue, 6 Dec 2022 at 00:02, Ricardo Ribalda wrote:
> > > > > On Mon, 5 Dec 2022 at 23:16, Laurent Pinchart wrote:
> > > > > > On Fri, Dec 02, 2022 at 06:08:19PM +0100, Ricardo Ribalda wrote:
> > > > > > > Some cameras have multiple data inputs (i.e. IR sensor and RGB sensor),
> > >
> > > Did you mean "data outputs" by the way ?
> > >
> > > > > > > append a unique number to the device 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
> > > > > > >
> > > > > > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > > > > > > ---
> > > > > > >  drivers/media/usb/uvc/uvc_driver.c | 3 ++-
> > > > > > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > > > > >
> > > > > > > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> > > > > > > index 215fb483efb0..f4032ebb3689 100644
> > > > > > > --- a/drivers/media/usb/uvc/uvc_driver.c
> > > > > > > +++ b/drivers/media/usb/uvc/uvc_driver.c
> > > > > > > @@ -1963,7 +1963,8 @@ int uvc_register_video_device(struct uvc_device *dev,
> > > > > > >               break;
> > > > > > >       }
> > > > > > >
> > > > > > > -     strscpy(vdev->name, dev->name, sizeof(vdev->name));
> > > > > > > +     snprintf(vdev->name, sizeof(vdev->name), "%s %u", dev->name,
> > > > > > > +              stream->header.bTerminalLink);
> > > > > >
> > > > > > This won't be perfect as the string is not guaranteed to fit in
> > > > > > vdev->name, but I suppose it will help as a quick fix for some devices.
> > > > > > How about the other devices ? Won't they still exhibit the above
> > > > > > v4l2-compliance failure ? Isn't that something that will still affect
> > > > > > Chrome OS devices ?
> > > > >
> > > > > We could place the id first... but that will look bad: Eg:
> > > > >
> > > > > 1- My favorite camera
> > > > >
> > > > > Another option is to remove the last chars to fit the id. Eg:
> > > > >
> > > > > My favorite came-1
> > > > >
> > > > > If you prefer any of those options or have a better idea I can implement that.
> > > >
> > > > @Laurent
> > > >
> > > > Any preference here?
> > >
> > > I think the latter is better. Could we do so only when there are
> > > multiple video capture devices (excluding the metadata device) though ?
> > > That way we won't have a weird "-n" suffix in the majority of use cases.
> > >
> > > > > > The change should not cause any regression as big as in patch 1/3.
> > > > > > However, unless I'm mistaken users will notice a device name change,
> > > > > > especially when selecting a device in their web browser. Could that be a
> > > > > > problem ?
> > > > >
> > > > > I think the only side effect is that the first time that the kernel
> > > > > changes the naming convention, if there are more than one camera on
> > > > > the system, the video conference might pick a different camera.
> > > > > The good news is that the user will be presented with cameras with
> > > > > different names. Now some cameras show very confusing names:
> > > > >
> > > > > ribalda@alco:~/work/linux$ for a in /dev/video* ; do yavta -l $a| grep
> > > > > "Dell Webcam"; done
> > > > > Device `Dell Webcam WB7022' on `usb-0000:2d:00.0-1.2.3.1' (driver
> > > > > 'uvcvideo') supports video, capture, without mplanes.
> > > > > Device `Dell Webcam WB7022' on `usb-0000:2d:00.0-1.2.3.1' (driver
> > > > > 'uvcvideo') supports meta-data, capture, without mplanes.
> > > > > Device `Dell Webcam WB7022' on `usb-0000:2d:00.0-1.2.3.1' (driver
> > > > > 'uvcvideo') supports video, capture, without mplanes.
> > > > > Device `Dell Webcam WB7022' on `usb-0000:2d:00.0-1.2.3.1' (driver
> > > > > 'uvcvideo') supports meta-data, capture, without mplanes.
> > >
> > > I'm tempted to add a new model read-only string control to overcome the
> > > length limitation. It could then be combined with other information
> > > (such as the location and supported pixel formats) to create a
> > > user-friendly camera name by applications.
> >
> > Adding the vid:pid would be really useful! Mapping a /dev/videoX to
> > vid:pid is kind of complicated now.
>
> libcamera can help there ;-) We already extract the vendor and product
> ID. They are only used to create the camera ID at the moment, they are
> not exposed to applications independently. That would be a good
> addition.
>
> Overall, device naming is something that we have decided *not* to handle
> in libcamera. We provide information to applications to help them
> construct a meaningful name, but don't create the name ourselves. This
> was decided because naming schemes are dependent on application use
> cases, and in many cases should be localized (e.g. "Front camera" and
> "Back camera" vs. "Etukamera" and "Takakamera").

Thanks for the explanation!

My use case is: vendor apps for doing provisioning or low level
testing. Libcamera would be a bit too much for that ;)

Most of the time they do not get an image, but set fancy controls.


Regards!


>
> > > > > > >       /*
> > > > > > >        * Set the driver data before calling video_register_device, otherwise
> > > > > > >
>
> --
> Regards,
>
> Laurent Pinchart



-- 
Ricardo Ribalda

Re: [PATCH v4 3/3] media: uvcvideo: Add a unique suffix to camera names

[Laurent Pinchart]

Hi Ricardo,

On Wed, Dec 21, 2022 at 09:21:57PM +0100, Ricardo Ribalda wrote:
> On Wed, 21 Dec 2022 at 12:23, Laurent Pinchart wrote:
> > On Wed, Dec 21, 2022 at 11:57:48AM +0100, Ricardo Ribalda wrote:
> > > On Wed, 21 Dec 2022 at 11:55, Laurent Pinchart wrote:
> > > > On Wed, Dec 21, 2022 at 12:00:58AM +0100, Ricardo Ribalda wrote:
> > > > > On Tue, 6 Dec 2022 at 00:02, Ricardo Ribalda wrote:
> > > > > > On Mon, 5 Dec 2022 at 23:16, Laurent Pinchart wrote:
> > > > > > > On Fri, Dec 02, 2022 at 06:08:19PM +0100, Ricardo Ribalda wrote:
> > > > > > > > Some cameras have multiple data inputs (i.e. IR sensor and RGB sensor),
> > > >
> > > > Did you mean "data outputs" by the way ?
> > > >
> > > > > > > > append a unique number to the device 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
> > > > > > > >
> > > > > > > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > > > > > > > ---
> > > > > > > >  drivers/media/usb/uvc/uvc_driver.c | 3 ++-
> > > > > > > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > > > > > >
> > > > > > > > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> > > > > > > > index 215fb483efb0..f4032ebb3689 100644
> > > > > > > > --- a/drivers/media/usb/uvc/uvc_driver.c
> > > > > > > > +++ b/drivers/media/usb/uvc/uvc_driver.c
> > > > > > > > @@ -1963,7 +1963,8 @@ int uvc_register_video_device(struct uvc_device *dev,
> > > > > > > >               break;
> > > > > > > >       }
> > > > > > > >
> > > > > > > > -     strscpy(vdev->name, dev->name, sizeof(vdev->name));
> > > > > > > > +     snprintf(vdev->name, sizeof(vdev->name), "%s %u", dev->name,
> > > > > > > > +              stream->header.bTerminalLink);
> > > > > > >
> > > > > > > This won't be perfect as the string is not guaranteed to fit in
> > > > > > > vdev->name, but I suppose it will help as a quick fix for some devices.
> > > > > > > How about the other devices ? Won't they still exhibit the above
> > > > > > > v4l2-compliance failure ? Isn't that something that will still affect
> > > > > > > Chrome OS devices ?
> > > > > >
> > > > > > We could place the id first... but that will look bad: Eg:
> > > > > >
> > > > > > 1- My favorite camera
> > > > > >
> > > > > > Another option is to remove the last chars to fit the id. Eg:
> > > > > >
> > > > > > My favorite came-1
> > > > > >
> > > > > > If you prefer any of those options or have a better idea I can implement that.
> > > > >
> > > > > @Laurent
> > > > >
> > > > > Any preference here?
> > > >
> > > > I think the latter is better. Could we do so only when there are
> > > > multiple video capture devices (excluding the metadata device) though ?
> > > > That way we won't have a weird "-n" suffix in the majority of use cases.
> > > >
> > > > > > > The change should not cause any regression as big as in patch 1/3.
> > > > > > > However, unless I'm mistaken users will notice a device name change,
> > > > > > > especially when selecting a device in their web browser. Could that be a
> > > > > > > problem ?
> > > > > >
> > > > > > I think the only side effect is that the first time that the kernel
> > > > > > changes the naming convention, if there are more than one camera on
> > > > > > the system, the video conference might pick a different camera.
> > > > > > The good news is that the user will be presented with cameras with
> > > > > > different names. Now some cameras show very confusing names:
> > > > > >
> > > > > > ribalda@alco:~/work/linux$ for a in /dev/video* ; do yavta -l $a| grep
> > > > > > "Dell Webcam"; done
> > > > > > Device `Dell Webcam WB7022' on `usb-0000:2d:00.0-1.2.3.1' (driver
> > > > > > 'uvcvideo') supports video, capture, without mplanes.
> > > > > > Device `Dell Webcam WB7022' on `usb-0000:2d:00.0-1.2.3.1' (driver
> > > > > > 'uvcvideo') supports meta-data, capture, without mplanes.
> > > > > > Device `Dell Webcam WB7022' on `usb-0000:2d:00.0-1.2.3.1' (driver
> > > > > > 'uvcvideo') supports video, capture, without mplanes.
> > > > > > Device `Dell Webcam WB7022' on `usb-0000:2d:00.0-1.2.3.1' (driver
> > > > > > 'uvcvideo') supports meta-data, capture, without mplanes.
> > > >
> > > > I'm tempted to add a new model read-only string control to overcome the
> > > > length limitation. It could then be combined with other information
> > > > (such as the location and supported pixel formats) to create a
> > > > user-friendly camera name by applications.
> > >
> > > Adding the vid:pid would be really useful! Mapping a /dev/videoX to
> > > vid:pid is kind of complicated now.
> >
> > libcamera can help there ;-) We already extract the vendor and product
> > ID. They are only used to create the camera ID at the moment, they are
> > not exposed to applications independently. That would be a good
> > addition.
> >
> > Overall, device naming is something that we have decided *not* to handle
> > in libcamera. We provide information to applications to help them
> > construct a meaningful name, but don't create the name ourselves. This
> > was decided because naming schemes are dependent on application use
> > cases, and in many cases should be localized (e.g. "Front camera" and
> > "Back camera" vs. "Etukamera" and "Takakamera").
> 
> Thanks for the explanation!
> 
> My use case is: vendor apps for doing provisioning or low level
> testing. Libcamera would be a bit too much for that ;)
> 
> Most of the time they do not get an image, but set fancy controls.

I see what you mean. libcamera isn't the right tool for that indeed, but
maybe the additional complexity of looking up the VID:PID isn't such a
big deal in those cases ? It's not that complicated using sysfs, and can
even be done quite simply with a shell script. Maybe a standard simple
script in v4l-utils (or in another location) could help ?

> > > > > > > >       /*
> > > > > > > >        * Set the driver data before calling video_register_device, otherwise
> > > > > > > >

-- 
Regards,

Laurent Pinchart

Re: [PATCH v4 3/3] media: uvcvideo: Add a unique suffix to camera names

[Mauro Carvalho Chehab]

Em Tue, 6 Dec 2022 00:02:22 +0100
Ricardo Ribalda <ribalda@chromium.org> escreveu:

> Hi Laurent
> 
> On Mon, 5 Dec 2022 at 23:16, Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> wrote:
> >
> > Hi Ricardo,
> >
> > Thank you for the patch.
> >
> > On Fri, Dec 02, 2022 at 06:08:19PM +0100, Ricardo Ribalda wrote:  
> > > Some cameras have multiple data inputs (i.e. IR sensor and RGB sensor),
> > > append a unique number to the device 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
> > >
> > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > > ---
> > >  drivers/media/usb/uvc/uvc_driver.c | 3 ++-
> > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> > > index 215fb483efb0..f4032ebb3689 100644
> > > --- a/drivers/media/usb/uvc/uvc_driver.c
> > > +++ b/drivers/media/usb/uvc/uvc_driver.c
> > > @@ -1963,7 +1963,8 @@ int uvc_register_video_device(struct uvc_device *dev,
> > >               break;
> > >       }
> > >
> > > -     strscpy(vdev->name, dev->name, sizeof(vdev->name));
> > > +     snprintf(vdev->name, sizeof(vdev->name), "%s %u", dev->name,
> > > +              stream->header.bTerminalLink);  
> >
> > This won't be perfect as the string is not guaranteed to fit in
> > vdev->name, but I suppose it will help as a quick fix for some devices.
> > How about the other devices ? Won't they still exhibit the above
> > v4l2-compliance failure ? Isn't that something that will still affect
> > Chrome OS devices ?  
> 
> We could place the id first... but that will look bad: Eg:
> 
> 1- My favorite camera
> 
> Another option is to remove the last chars to fit the id. Eg:
> 
> My favorite came-1
> 
> If you prefer any of those options or have a better idea I can implement that.
> 
> 
> >
> > The change should not cause any regression as big as in patch 1/3.
> > However, unless I'm mistaken users will notice a device name change,
> > especially when selecting a device in their web browser. Could that be a
> > problem ?  
> 
> I think the only side effect is that the first time that the kernel
> changes the naming convention, if there are more than one camera on
> the system, the video conference might pick a different camera.
> The good news is that the user will be presented with cameras with
> different names. Now some cameras show very confusing names:
> 
> ribalda@alco:~/work/linux$ for a in /dev/video* ; do yavta -l $a| grep
> "Dell Webcam"; done
> Device `Dell Webcam WB7022' on `usb-0000:2d:00.0-1.2.3.1' (driver
> 'uvcvideo') supports video, capture, without mplanes.
> Device `Dell Webcam WB7022' on `usb-0000:2d:00.0-1.2.3.1' (driver
> 'uvcvideo') supports meta-data, capture, without mplanes.
> Device `Dell Webcam WB7022' on `usb-0000:2d:00.0-1.2.3.1' (driver
> 'uvcvideo') supports video, capture, without mplanes.
> Device `Dell Webcam WB7022' on `usb-0000:2d:00.0-1.2.3.1' (driver
> 'uvcvideo') supports meta-data, capture, without mplanes.

That is bad, as some apps like camorama actually store the user
preferences (last used resolution and fps), preserving them the next
time the camera with the same name is used.

In the specific case of camorama, it uses the by-id filename, which is
derived from the name, as stored at:

	$ ls -la /dev/v4l/by-id/usb-Quanta_HD_Camera_0001-video-index*
	lrwxrwxrwx 1 root root 12 dez 21 09:59 /dev/v4l/by-id/usb-Quanta_HD_Camera_0001-video-index0 -> ../../video0
	lrwxrwxrwx 1 root root 12 dez 21 09:59 /dev/v4l/by-id/usb-Quanta_HD_Camera_0001-video-index1 -> ../../video1

With this change, not only the camera name will become bigger (and may
end losing the index0/index1 data), but it will also lost the stored 
preferences on apps like camorama, causing regressions.

It sounds a lot easier to teach udev to change the name on an unique
way, on machines where you need it.

Regards,
Mauro

Re: [PATCH v4 3/3] media: uvcvideo: Add a unique suffix to camera names

[Laurent Pinchart]

Hi Mauro,

On Wed, Dec 21, 2022 at 10:02:48PM +0000, Mauro Carvalho Chehab wrote:
> Em Tue, 6 Dec 2022 00:02:22 +0100 Ricardo Ribalda escreveu:
> > On Mon, 5 Dec 2022 at 23:16, Laurent Pinchart wrote:
> > > On Fri, Dec 02, 2022 at 06:08:19PM +0100, Ricardo Ribalda wrote:  
> > > > Some cameras have multiple data inputs (i.e. IR sensor and RGB sensor),
> > > > append a unique number to the device 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
> > > >
> > > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > > > ---
> > > >  drivers/media/usb/uvc/uvc_driver.c | 3 ++-
> > > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> > > > index 215fb483efb0..f4032ebb3689 100644
> > > > --- a/drivers/media/usb/uvc/uvc_driver.c
> > > > +++ b/drivers/media/usb/uvc/uvc_driver.c
> > > > @@ -1963,7 +1963,8 @@ int uvc_register_video_device(struct uvc_device *dev,
> > > >               break;
> > > >       }
> > > >
> > > > -     strscpy(vdev->name, dev->name, sizeof(vdev->name));
> > > > +     snprintf(vdev->name, sizeof(vdev->name), "%s %u", dev->name,
> > > > +              stream->header.bTerminalLink);  
> > >
> > > This won't be perfect as the string is not guaranteed to fit in
> > > vdev->name, but I suppose it will help as a quick fix for some devices.
> > > How about the other devices ? Won't they still exhibit the above
> > > v4l2-compliance failure ? Isn't that something that will still affect
> > > Chrome OS devices ?  
> > 
> > We could place the id first... but that will look bad: Eg:
> > 
> > 1- My favorite camera
> > 
> > Another option is to remove the last chars to fit the id. Eg:
> > 
> > My favorite came-1
> > 
> > If you prefer any of those options or have a better idea I can implement that.
> > 
> > > The change should not cause any regression as big as in patch 1/3.
> > > However, unless I'm mistaken users will notice a device name change,
> > > especially when selecting a device in their web browser. Could that be a
> > > problem ?  
> > 
> > I think the only side effect is that the first time that the kernel
> > changes the naming convention, if there are more than one camera on
> > the system, the video conference might pick a different camera.
> > The good news is that the user will be presented with cameras with
> > different names. Now some cameras show very confusing names:
> > 
> > ribalda@alco:~/work/linux$ for a in /dev/video* ; do yavta -l $a| grep
> > "Dell Webcam"; done
> > Device `Dell Webcam WB7022' on `usb-0000:2d:00.0-1.2.3.1' (driver
> > 'uvcvideo') supports video, capture, without mplanes.
> > Device `Dell Webcam WB7022' on `usb-0000:2d:00.0-1.2.3.1' (driver
> > 'uvcvideo') supports meta-data, capture, without mplanes.
> > Device `Dell Webcam WB7022' on `usb-0000:2d:00.0-1.2.3.1' (driver
> > 'uvcvideo') supports video, capture, without mplanes.
> > Device `Dell Webcam WB7022' on `usb-0000:2d:00.0-1.2.3.1' (driver
> > 'uvcvideo') supports meta-data, capture, without mplanes.
> 
> That is bad, as some apps like camorama actually store the user
> preferences (last used resolution and fps), preserving them the next
> time the camera with the same name is used.
> 
> In the specific case of camorama, it uses the by-id filename, which is
> derived from the name, as stored at:
> 
> 	$ ls -la /dev/v4l/by-id/usb-Quanta_HD_Camera_0001-video-index*
> 	lrwxrwxrwx 1 root root 12 dez 21 09:59 /dev/v4l/by-id/usb-Quanta_HD_Camera_0001-video-index0 -> ../../video0
> 	lrwxrwxrwx 1 root root 12 dez 21 09:59 /dev/v4l/by-id/usb-Quanta_HD_Camera_0001-video-index1 -> ../../video1
> 
> With this change, not only the camera name will become bigger (and may
> end losing the index0/index1 data), but it will also lost the stored 
> preferences on apps like camorama, causing regressions.
> 
> It sounds a lot easier to teach udev to change the name on an unique
> way, on machines where you need it.

It's not a udev issue, it's an API compliance problem, entity names in a
media controller graph must be unique, and they are not.

Losing the stored preference is possibly inconvenient in some cases, but
I don't think it's a real blocker.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v4 3/3] media: uvcvideo: Add a unique suffix to camera names

[Mauro Carvalho Chehab]

Em Thu, 22 Dec 2022 00:24:44 +0200
Laurent Pinchart <laurent.pinchart@ideasonboard.com> escreveu:

> Hi Mauro,
> 
> On Wed, Dec 21, 2022 at 10:02:48PM +0000, Mauro Carvalho Chehab wrote:
> > Em Tue, 6 Dec 2022 00:02:22 +0100 Ricardo Ribalda escreveu:  
> > > On Mon, 5 Dec 2022 at 23:16, Laurent Pinchart wrote:  
> > > > On Fri, Dec 02, 2022 at 06:08:19PM +0100, Ricardo Ribalda wrote:    
> > > > > Some cameras have multiple data inputs (i.e. IR sensor and RGB sensor),
> > > > > append a unique number to the device 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
> > > > >
> > > > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > > > > ---
> > > > >  drivers/media/usb/uvc/uvc_driver.c | 3 ++-
> > > > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> > > > > index 215fb483efb0..f4032ebb3689 100644
> > > > > --- a/drivers/media/usb/uvc/uvc_driver.c
> > > > > +++ b/drivers/media/usb/uvc/uvc_driver.c
> > > > > @@ -1963,7 +1963,8 @@ int uvc_register_video_device(struct uvc_device *dev,
> > > > >               break;
> > > > >       }
> > > > >
> > > > > -     strscpy(vdev->name, dev->name, sizeof(vdev->name));
> > > > > +     snprintf(vdev->name, sizeof(vdev->name), "%s %u", dev->name,
> > > > > +              stream->header.bTerminalLink);    
> > > >
> > > > This won't be perfect as the string is not guaranteed to fit in
> > > > vdev->name, but I suppose it will help as a quick fix for some devices.
> > > > How about the other devices ? Won't they still exhibit the above
> > > > v4l2-compliance failure ? Isn't that something that will still affect
> > > > Chrome OS devices ?    
> > > 
> > > We could place the id first... but that will look bad: Eg:
> > > 
> > > 1- My favorite camera
> > > 
> > > Another option is to remove the last chars to fit the id. Eg:
> > > 
> > > My favorite came-1
> > > 
> > > If you prefer any of those options or have a better idea I can implement that.
> > >   
> > > > The change should not cause any regression as big as in patch 1/3.
> > > > However, unless I'm mistaken users will notice a device name change,
> > > > especially when selecting a device in their web browser. Could that be a
> > > > problem ?    
> > > 
> > > I think the only side effect is that the first time that the kernel
> > > changes the naming convention, if there are more than one camera on
> > > the system, the video conference might pick a different camera.
> > > The good news is that the user will be presented with cameras with
> > > different names. Now some cameras show very confusing names:
> > > 
> > > ribalda@alco:~/work/linux$ for a in /dev/video* ; do yavta -l $a| grep
> > > "Dell Webcam"; done
> > > Device `Dell Webcam WB7022' on `usb-0000:2d:00.0-1.2.3.1' (driver
> > > 'uvcvideo') supports video, capture, without mplanes.
> > > Device `Dell Webcam WB7022' on `usb-0000:2d:00.0-1.2.3.1' (driver
> > > 'uvcvideo') supports meta-data, capture, without mplanes.
> > > Device `Dell Webcam WB7022' on `usb-0000:2d:00.0-1.2.3.1' (driver
> > > 'uvcvideo') supports video, capture, without mplanes.
> > > Device `Dell Webcam WB7022' on `usb-0000:2d:00.0-1.2.3.1' (driver
> > > 'uvcvideo') supports meta-data, capture, without mplanes.  
> > 
> > That is bad, as some apps like camorama actually store the user
> > preferences (last used resolution and fps), preserving them the next
> > time the camera with the same name is used.
> > 
> > In the specific case of camorama, it uses the by-id filename, which is
> > derived from the name, as stored at:
> > 
> > 	$ ls -la /dev/v4l/by-id/usb-Quanta_HD_Camera_0001-video-index*
> > 	lrwxrwxrwx 1 root root 12 dez 21 09:59 /dev/v4l/by-id/usb-Quanta_HD_Camera_0001-video-index0 -> ../../video0
> > 	lrwxrwxrwx 1 root root 12 dez 21 09:59 /dev/v4l/by-id/usb-Quanta_HD_Camera_0001-video-index1 -> ../../video1
> > 
> > With this change, not only the camera name will become bigger (and may
> > end losing the index0/index1 data), but it will also lost the stored 
> > preferences on apps like camorama, causing regressions.
> > 
> > It sounds a lot easier to teach udev to change the name on an unique
> > way, on machines where you need it.  
> 
> It's not a udev issue, it's an API compliance problem, entity names in a
> media controller graph must be unique, and they are not.

Then the right fix would be inside the media controller naming
logic, in a way that it will ensure unique names. It sounds to me
that mc core should then check, at device's register time, if the
name was already used. If so, change it to be unique.

> Losing the stored preference is possibly inconvenient in some cases, but
> I don't think it's a real blocker.

If it causes userspace regressions, then it is a blocker.

Regards,
Mauro

Re: [PATCH v4 3/3] media: uvcvideo: Add a unique suffix to camera names

[Laurent Pinchart]

On Wed, Dec 21, 2022 at 10:54:50PM +0000, Mauro Carvalho Chehab wrote:
> Em Thu, 22 Dec 2022 00:24:44 +0200
> Laurent Pinchart <laurent.pinchart@ideasonboard.com> escreveu:
> 
> > Hi Mauro,
> > 
> > On Wed, Dec 21, 2022 at 10:02:48PM +0000, Mauro Carvalho Chehab wrote:
> > > Em Tue, 6 Dec 2022 00:02:22 +0100 Ricardo Ribalda escreveu:  
> > > > On Mon, 5 Dec 2022 at 23:16, Laurent Pinchart wrote:  
> > > > > On Fri, Dec 02, 2022 at 06:08:19PM +0100, Ricardo Ribalda wrote:    
> > > > > > Some cameras have multiple data inputs (i.e. IR sensor and RGB sensor),
> > > > > > append a unique number to the device 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
> > > > > >
> > > > > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > > > > > ---
> > > > > >  drivers/media/usb/uvc/uvc_driver.c | 3 ++-
> > > > > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> > > > > > index 215fb483efb0..f4032ebb3689 100644
> > > > > > --- a/drivers/media/usb/uvc/uvc_driver.c
> > > > > > +++ b/drivers/media/usb/uvc/uvc_driver.c
> > > > > > @@ -1963,7 +1963,8 @@ int uvc_register_video_device(struct uvc_device *dev,
> > > > > >               break;
> > > > > >       }
> > > > > >
> > > > > > -     strscpy(vdev->name, dev->name, sizeof(vdev->name));
> > > > > > +     snprintf(vdev->name, sizeof(vdev->name), "%s %u", dev->name,
> > > > > > +              stream->header.bTerminalLink);    
> > > > >
> > > > > This won't be perfect as the string is not guaranteed to fit in
> > > > > vdev->name, but I suppose it will help as a quick fix for some devices.
> > > > > How about the other devices ? Won't they still exhibit the above
> > > > > v4l2-compliance failure ? Isn't that something that will still affect
> > > > > Chrome OS devices ?    
> > > > 
> > > > We could place the id first... but that will look bad: Eg:
> > > > 
> > > > 1- My favorite camera
> > > > 
> > > > Another option is to remove the last chars to fit the id. Eg:
> > > > 
> > > > My favorite came-1
> > > > 
> > > > If you prefer any of those options or have a better idea I can implement that.
> > > >   
> > > > > The change should not cause any regression as big as in patch 1/3.
> > > > > However, unless I'm mistaken users will notice a device name change,
> > > > > especially when selecting a device in their web browser. Could that be a
> > > > > problem ?    
> > > > 
> > > > I think the only side effect is that the first time that the kernel
> > > > changes the naming convention, if there are more than one camera on
> > > > the system, the video conference might pick a different camera.
> > > > The good news is that the user will be presented with cameras with
> > > > different names. Now some cameras show very confusing names:
> > > > 
> > > > ribalda@alco:~/work/linux$ for a in /dev/video* ; do yavta -l $a| grep
> > > > "Dell Webcam"; done
> > > > Device `Dell Webcam WB7022' on `usb-0000:2d:00.0-1.2.3.1' (driver
> > > > 'uvcvideo') supports video, capture, without mplanes.
> > > > Device `Dell Webcam WB7022' on `usb-0000:2d:00.0-1.2.3.1' (driver
> > > > 'uvcvideo') supports meta-data, capture, without mplanes.
> > > > Device `Dell Webcam WB7022' on `usb-0000:2d:00.0-1.2.3.1' (driver
> > > > 'uvcvideo') supports video, capture, without mplanes.
> > > > Device `Dell Webcam WB7022' on `usb-0000:2d:00.0-1.2.3.1' (driver
> > > > 'uvcvideo') supports meta-data, capture, without mplanes.  
> > > 
> > > That is bad, as some apps like camorama actually store the user
> > > preferences (last used resolution and fps), preserving them the next
> > > time the camera with the same name is used.
> > > 
> > > In the specific case of camorama, it uses the by-id filename, which is
> > > derived from the name, as stored at:
> > > 
> > > 	$ ls -la /dev/v4l/by-id/usb-Quanta_HD_Camera_0001-video-index*
> > > 	lrwxrwxrwx 1 root root 12 dez 21 09:59 /dev/v4l/by-id/usb-Quanta_HD_Camera_0001-video-index0 -> ../../video0
> > > 	lrwxrwxrwx 1 root root 12 dez 21 09:59 /dev/v4l/by-id/usb-Quanta_HD_Camera_0001-video-index1 -> ../../video1
> > > 
> > > With this change, not only the camera name will become bigger (and may
> > > end losing the index0/index1 data), but it will also lost the stored 
> > > preferences on apps like camorama, causing regressions.
> > > 
> > > It sounds a lot easier to teach udev to change the name on an unique
> > > way, on machines where you need it.  
> > 
> > It's not a udev issue, it's an API compliance problem, entity names in a
> > media controller graph must be unique, and they are not.
> 
> Then the right fix would be inside the media controller naming
> logic, in a way that it will ensure unique names. It sounds to me
> that mc core should then check, at device's register time, if the
> name was already used. If so, change it to be unique.

The entity name and video device name are one and the same.

> > Losing the stored preference is possibly inconvenient in some cases, but
> > I don't think it's a real blocker.
> 
> If it causes userspace regressions, then it is a blocker.

I wouldn't necessarily call this a regression.

-- 
Regards,

Laurent Pinchart