[PATCH 0/3] uvc: Restore old vdev name

[PATCH 0/3] uvc: Restore old vdev name

[Ricardo Ribalda]

In order to have unique entity names, we decided to change the name of
the video devices with their functionality.

This has resulted in some (all?) GUIs showing not useful names.

This patchset reverts the original patch and introduces a new one to
allow having different entity and vdev names.

Since some distros have ported the reverted patch to their stable
kernels, it would be great if we can get this sent asap, to avoid making
more people angry ;).


Ricardo Ribalda (3):
  Revert "media: uvcvideo: Set unique vdev name based in type"
  media: v4l2-dev.c: Allow driver-defined entity names
  media: uvcvideo: Set unique entity name based in type

 drivers/media/usb/uvc/uvc_driver.c | 14 +++++++++++---
 drivers/media/v4l2-core/v4l2-dev.c |  4 +++-
 2 files changed, 14 insertions(+), 4 deletions(-)

-- 
2.33.0

[PATCH 1/3] Revert "media: uvcvideo: Set unique vdev name based in type"

[Ricardo Ribalda]

A lot of userspace depends on a descriptive name for vdev. Without this
patch, users have a hard time figuring out which camera shall they use
for their video conferencing.

This reverts commit e3f60e7e1a2b451f538f9926763432249bcf39c4.

Cc: <stable@vger.kernel.org>
Fixes: e3f60e7e1a2b ("media: uvcvideo: Set unique vdev name based in type")
Reported-by: Nicolas Dufresne <nicolas@ndufresne.ca>
Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/media/usb/uvc/uvc_driver.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
index 7c007426e082..058d28a0344b 100644
--- a/drivers/media/usb/uvc/uvc_driver.c
+++ b/drivers/media/usb/uvc/uvc_driver.c
@@ -2193,7 +2193,6 @@ int uvc_register_video_device(struct uvc_device *dev,
 			      const struct v4l2_file_operations *fops,
 			      const struct v4l2_ioctl_ops *ioctl_ops)
 {
-	const char *name;
 	int ret;
 
 	/* Initialize the video buffers queue. */
@@ -2222,20 +2221,16 @@ int uvc_register_video_device(struct uvc_device *dev,
 	case V4L2_BUF_TYPE_VIDEO_CAPTURE:
 	default:
 		vdev->device_caps = V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_STREAMING;
-		name = "Video Capture";
 		break;
 	case V4L2_BUF_TYPE_VIDEO_OUTPUT:
 		vdev->device_caps = V4L2_CAP_VIDEO_OUTPUT | V4L2_CAP_STREAMING;
-		name = "Video Output";
 		break;
 	case V4L2_BUF_TYPE_META_CAPTURE:
 		vdev->device_caps = V4L2_CAP_META_CAPTURE | V4L2_CAP_STREAMING;
-		name = "Metadata";
 		break;
 	}
 
-	snprintf(vdev->name, sizeof(vdev->name), "%s %u", name,
-		 stream->header.bTerminalLink);
+	strscpy(vdev->name, dev->name, sizeof(vdev->name));
 
 	/*
 	 * Set the driver data before calling video_register_device, otherwise
-- 
2.33.0

Re: [PATCH 1/3] Revert "media: uvcvideo: Set unique vdev name based in type"

[Laurent Pinchart]

Hi Ricardo,

Thank you for the patch.

On Tue, Dec 07, 2021 at 01:06:27AM +0100, Ricardo Ribalda wrote:
> A lot of userspace depends on a descriptive name for vdev. Without this
> patch, users have a hard time figuring out which camera shall they use
> for their video conferencing.
> 
> This reverts commit e3f60e7e1a2b451f538f9926763432249bcf39c4.
> 
> Cc: <stable@vger.kernel.org>
> Fixes: e3f60e7e1a2b ("media: uvcvideo: Set unique vdev name based in type")
> Reported-by: Nicolas Dufresne <nicolas@ndufresne.ca>
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/media/usb/uvc/uvc_driver.c | 7 +------
>  1 file changed, 1 insertion(+), 6 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> index 7c007426e082..058d28a0344b 100644
> --- a/drivers/media/usb/uvc/uvc_driver.c
> +++ b/drivers/media/usb/uvc/uvc_driver.c
> @@ -2193,7 +2193,6 @@ int uvc_register_video_device(struct uvc_device *dev,
>  			      const struct v4l2_file_operations *fops,
>  			      const struct v4l2_ioctl_ops *ioctl_ops)
>  {
> -	const char *name;
>  	int ret;
>  
>  	/* Initialize the video buffers queue. */
> @@ -2222,20 +2221,16 @@ int uvc_register_video_device(struct uvc_device *dev,
>  	case V4L2_BUF_TYPE_VIDEO_CAPTURE:
>  	default:
>  		vdev->device_caps = V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_STREAMING;
> -		name = "Video Capture";
>  		break;
>  	case V4L2_BUF_TYPE_VIDEO_OUTPUT:
>  		vdev->device_caps = V4L2_CAP_VIDEO_OUTPUT | V4L2_CAP_STREAMING;
> -		name = "Video Output";
>  		break;
>  	case V4L2_BUF_TYPE_META_CAPTURE:
>  		vdev->device_caps = V4L2_CAP_META_CAPTURE | V4L2_CAP_STREAMING;
> -		name = "Metadata";
>  		break;
>  	}
>  
> -	snprintf(vdev->name, sizeof(vdev->name), "%s %u", name,
> -		 stream->header.bTerminalLink);
> +	strscpy(vdev->name, dev->name, sizeof(vdev->name));
>  
>  	/*
>  	 * Set the driver data before calling video_register_device, otherwise

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/3] Revert "media: uvcvideo: Set unique vdev name based in type"

[Laurent Pinchart]

On Tue, Dec 07, 2021 at 02:14:01AM +0200, Laurent Pinchart wrote:
> Hi Ricardo,
> 
> Thank you for the patch.
> 
> On Tue, Dec 07, 2021 at 01:06:27AM +0100, Ricardo Ribalda wrote:
> > A lot of userspace depends on a descriptive name for vdev. Without this
> > patch, users have a hard time figuring out which camera shall they use
> > for their video conferencing.
> > 
> > This reverts commit e3f60e7e1a2b451f538f9926763432249bcf39c4.
> > 
> > Cc: <stable@vger.kernel.org>
> > Fixes: e3f60e7e1a2b ("media: uvcvideo: Set unique vdev name based in type")
> > Reported-by: Nicolas Dufresne <nicolas@ndufresne.ca>
> > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Mauro, is it possible to queue this as a fix for v5.16 ?

> > ---
> >  drivers/media/usb/uvc/uvc_driver.c | 7 +------
> >  1 file changed, 1 insertion(+), 6 deletions(-)
> > 
> > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> > index 7c007426e082..058d28a0344b 100644
> > --- a/drivers/media/usb/uvc/uvc_driver.c
> > +++ b/drivers/media/usb/uvc/uvc_driver.c
> > @@ -2193,7 +2193,6 @@ int uvc_register_video_device(struct uvc_device *dev,
> >  			      const struct v4l2_file_operations *fops,
> >  			      const struct v4l2_ioctl_ops *ioctl_ops)
> >  {
> > -	const char *name;
> >  	int ret;
> >  
> >  	/* Initialize the video buffers queue. */
> > @@ -2222,20 +2221,16 @@ int uvc_register_video_device(struct uvc_device *dev,
> >  	case V4L2_BUF_TYPE_VIDEO_CAPTURE:
> >  	default:
> >  		vdev->device_caps = V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_STREAMING;
> > -		name = "Video Capture";
> >  		break;
> >  	case V4L2_BUF_TYPE_VIDEO_OUTPUT:
> >  		vdev->device_caps = V4L2_CAP_VIDEO_OUTPUT | V4L2_CAP_STREAMING;
> > -		name = "Video Output";
> >  		break;
> >  	case V4L2_BUF_TYPE_META_CAPTURE:
> >  		vdev->device_caps = V4L2_CAP_META_CAPTURE | V4L2_CAP_STREAMING;
> > -		name = "Metadata";
> >  		break;
> >  	}
> >  
> > -	snprintf(vdev->name, sizeof(vdev->name), "%s %u", name,
> > -		 stream->header.bTerminalLink);
> > +	strscpy(vdev->name, dev->name, sizeof(vdev->name));
> >  
> >  	/*
> >  	 * Set the driver data before calling video_register_device, otherwise

-- 
Regards,

Laurent Pinchart

[PATCH 2/3] media: v4l2-dev.c: Allow driver-defined entity names

[Ricardo Ribalda]

If the driver provides an name for an entity, use it.
This is particularly useful for drivers that export multiple video
devices for the same hardware (i.e. metadata and data).

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

diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
index d03ace324db0..4c00503b9349 100644
--- a/drivers/media/v4l2-core/v4l2-dev.c
+++ b/drivers/media/v4l2-core/v4l2-dev.c
@@ -832,7 +832,9 @@ static int video_register_media_controller(struct video_device *vdev)
 	}
 
 	if (vdev->entity.function != MEDIA_ENT_F_UNKNOWN) {
-		vdev->entity.name = vdev->name;
+		/* Use entity names provided by the driver, if available. */
+		if (!vdev->entity.name)
+			vdev->entity.name = vdev->name;
 
 		/* Needed just for backward compatibility with legacy MC API */
 		vdev->entity.info.dev.major = VIDEO_MAJOR;
-- 
2.33.0

Re: [PATCH 2/3] media: v4l2-dev.c: Allow driver-defined entity names

[Laurent Pinchart]

Hi Ricardo,

Thank you for the patch.

On Tue, Dec 07, 2021 at 01:06:28AM +0100, Ricardo Ribalda wrote:
> If the driver provides an name for an entity, use it.
> This is particularly useful for drivers that export multiple video
> devices for the same hardware (i.e. metadata and data).

This seems reasonable (especially given that I've proposed it), but I
may be missing unintented consequences. Other reviews would be useful.

> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
>  drivers/media/v4l2-core/v4l2-dev.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
> index d03ace324db0..4c00503b9349 100644
> --- a/drivers/media/v4l2-core/v4l2-dev.c
> +++ b/drivers/media/v4l2-core/v4l2-dev.c
> @@ -832,7 +832,9 @@ static int video_register_media_controller(struct video_device *vdev)
>  	}
>  
>  	if (vdev->entity.function != MEDIA_ENT_F_UNKNOWN) {
> -		vdev->entity.name = vdev->name;
> +		/* Use entity names provided by the driver, if available. */
> +		if (!vdev->entity.name)
> +			vdev->entity.name = vdev->name;

We need to document this.

>  
>  		/* Needed just for backward compatibility with legacy MC API */
>  		vdev->entity.info.dev.major = VIDEO_MAJOR;

-- 
Regards,

Laurent Pinchart

[PATCH 3/3] media: uvcvideo: Set unique entity name based in type

[Ricardo Ribalda]

All the entities must have a unique name. We can have a descriptive and
unique name by appending the function to their terminal link.

This is even resilient to multi chain 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

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

diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
index 058d28a0344b..3700e61a8701 100644
--- a/drivers/media/usb/uvc/uvc_driver.c
+++ b/drivers/media/usb/uvc/uvc_driver.c
@@ -2194,6 +2194,7 @@ int uvc_register_video_device(struct uvc_device *dev,
 			      const struct v4l2_ioctl_ops *ioctl_ops)
 {
 	int ret;
+	const char *name;
 
 	/* Initialize the video buffers queue. */
 	ret = uvc_queue_init(queue, type, !uvc_no_drop_param);
@@ -2221,17 +2222,29 @@ int uvc_register_video_device(struct uvc_device *dev,
 	case V4L2_BUF_TYPE_VIDEO_CAPTURE:
 	default:
 		vdev->device_caps = V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_STREAMING;
+		name = "Video Capture";
 		break;
 	case V4L2_BUF_TYPE_VIDEO_OUTPUT:
 		vdev->device_caps = V4L2_CAP_VIDEO_OUTPUT | V4L2_CAP_STREAMING;
+		name = "Video Output";
 		break;
 	case V4L2_BUF_TYPE_META_CAPTURE:
 		vdev->device_caps = V4L2_CAP_META_CAPTURE | V4L2_CAP_STREAMING;
+		name = "Metadata";
 		break;
 	}
 
+	/*
+	 * Many userspace apps identify the device with vdev->name, so we
+	 * cannot change its name for its function.
+	 */
 	strscpy(vdev->name, dev->name, sizeof(vdev->name));
 
+#if defined(CONFIG_MEDIA_CONTROLLER)
+	vdev->entity.name = devm_kasprintf(&stream->intf->dev, GFP_KERNEL,
+				"%s %u", name, stream->header.bTerminalLink);
+#endif
+
 	/*
 	 * Set the driver data before calling video_register_device, otherwise
 	 * the file open() handler might race us.
-- 
2.33.0

Re: [PATCH 3/3] media: uvcvideo: Set unique entity name based in type

[Laurent Pinchart]

Hi Ricardo,

Thank you for the patch.

On Tue, Dec 07, 2021 at 01:06:29AM +0100, Ricardo Ribalda wrote:
> All the entities must have a unique name. We can have a descriptive and
> unique name by appending the function to their terminal link.
> 
> This is even resilient to multi chain 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
> 
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
>  drivers/media/usb/uvc/uvc_driver.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> index 058d28a0344b..3700e61a8701 100644
> --- a/drivers/media/usb/uvc/uvc_driver.c
> +++ b/drivers/media/usb/uvc/uvc_driver.c
> @@ -2194,6 +2194,7 @@ int uvc_register_video_device(struct uvc_device *dev,
>  			      const struct v4l2_ioctl_ops *ioctl_ops)
>  {
>  	int ret;
> +	const char *name;

Please swap those two lines.

>  
>  	/* Initialize the video buffers queue. */
>  	ret = uvc_queue_init(queue, type, !uvc_no_drop_param);
> @@ -2221,17 +2222,29 @@ int uvc_register_video_device(struct uvc_device *dev,
>  	case V4L2_BUF_TYPE_VIDEO_CAPTURE:
>  	default:
>  		vdev->device_caps = V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_STREAMING;
> +		name = "Video Capture";
>  		break;
>  	case V4L2_BUF_TYPE_VIDEO_OUTPUT:
>  		vdev->device_caps = V4L2_CAP_VIDEO_OUTPUT | V4L2_CAP_STREAMING;
> +		name = "Video Output";
>  		break;
>  	case V4L2_BUF_TYPE_META_CAPTURE:
>  		vdev->device_caps = V4L2_CAP_META_CAPTURE | V4L2_CAP_STREAMING;
> +		name = "Metadata";
>  		break;
>  	}
>  
> +	/*
> +	 * Many userspace apps identify the device with vdev->name, so we

s/apps/applications/

> +	 * cannot change its name for its function.
> +	 */
>  	strscpy(vdev->name, dev->name, sizeof(vdev->name));
>  
> +#if defined(CONFIG_MEDIA_CONTROLLER)
> +	vdev->entity.name = devm_kasprintf(&stream->intf->dev, GFP_KERNEL,
> +				"%s %u", name, stream->header.bTerminalLink);

Won't the compiler warn about a set but unused variable when
!CONFIG_MEDIA_CONTROLLER ?

> +#endif
> +
>  	/*
>  	 * Set the driver data before calling video_register_device, otherwise
>  	 * the file open() handler might race us.

-- 
Regards,

Laurent Pinchart