[PATCH v2 0/4] uvc: Restore old vdev name

[PATCH v2 0/4] 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 ;).

v2:
 - Add Documentation
 - Mark maybe unused variables as __maybe_unused
 - Add Suggested-by

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

 Documentation/driver-api/media/v4l2-dev.rst |  4 ++++
 drivers/media/usb/uvc/uvc_driver.c          | 14 +++++++++++---
 drivers/media/v4l2-core/v4l2-dev.c          |  4 +++-
 3 files changed, 18 insertions(+), 4 deletions(-)

-- 
2.34.1.400.ga245620fadb-goog

[PATCH v2 1/4] 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>
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
-- 
2.34.1.400.ga245620fadb-goog

[PATCH v2 2/4] 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).

Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
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.34.1.400.ga245620fadb-goog

[PATCH v2 3/4] media: Documentation/driver-api: Document entity name

[Ricardo Ribalda]

Now the entity name can be configured by the driver to a value different
than vdev->name. Document it accordingly.

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

diff --git a/Documentation/driver-api/media/v4l2-dev.rst b/Documentation/driver-api/media/v4l2-dev.rst
index 99e3b5fa7444..22120b60b0a9 100644
--- a/Documentation/driver-api/media/v4l2-dev.rst
+++ b/Documentation/driver-api/media/v4l2-dev.rst
@@ -134,6 +134,10 @@ manually set the struct media_entity type and name fields.
 A reference to the entity will be automatically acquired/released when the
 video device is opened/closed.
 
+The entity name can be configured by setting the `vdev->entity.name` pointer
+to the desired value. If it is set to NULL, the entity will be named as the
+video device.
+
 ioctls and locking
 ------------------
 
-- 
2.34.1.400.ga245620fadb-goog

[PATCH v2 4/4] 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..8efbde981480 100644
--- a/drivers/media/usb/uvc/uvc_driver.c
+++ b/drivers/media/usb/uvc/uvc_driver.c
@@ -2193,6 +2193,7 @@ int uvc_register_video_device(struct uvc_device *dev,
 			      const struct v4l2_file_operations *fops,
 			      const struct v4l2_ioctl_ops *ioctl_ops)
 {
+	const char __maybe_unused *name;
 	int ret;
 
 	/* Initialize the video buffers queue. */
@@ -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 applications 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.34.1.400.ga245620fadb-goog

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

[Hans Verkuil]

On 07/12/2021 01:38, 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>

Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>

Thanks!

	Hans

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

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

[Hans Verkuil]

On 07/12/2021 01:38, 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).
> 
> Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>

Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>

Thanks!

	Hans

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

Re: [PATCH v2 3/4] media: Documentation/driver-api: Document entity name

[Hans Verkuil]

On 07/12/2021 01:38, Ricardo Ribalda wrote:
> Now the entity name can be configured by the driver to a value different
> than vdev->name. Document it accordingly.
> 
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>

Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>

Thanks!

	Hans

> ---
>  Documentation/driver-api/media/v4l2-dev.rst | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/Documentation/driver-api/media/v4l2-dev.rst b/Documentation/driver-api/media/v4l2-dev.rst
> index 99e3b5fa7444..22120b60b0a9 100644
> --- a/Documentation/driver-api/media/v4l2-dev.rst
> +++ b/Documentation/driver-api/media/v4l2-dev.rst
> @@ -134,6 +134,10 @@ manually set the struct media_entity type and name fields.
>  A reference to the entity will be automatically acquired/released when the
>  video device is opened/closed.
>  
> +The entity name can be configured by setting the `vdev->entity.name` pointer
> +to the desired value. If it is set to NULL, the entity will be named as the
> +video device.
> +
>  ioctls and locking
>  ------------------
>  
> 

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

[Hans Verkuil]

On 07/12/2021 01:38, 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>

Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>

Thanks!

	Hans

> ---
>  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..8efbde981480 100644
> --- a/drivers/media/usb/uvc/uvc_driver.c
> +++ b/drivers/media/usb/uvc/uvc_driver.c
> @@ -2193,6 +2193,7 @@ int uvc_register_video_device(struct uvc_device *dev,
>  			      const struct v4l2_file_operations *fops,
>  			      const struct v4l2_ioctl_ops *ioctl_ops)
>  {
> +	const char __maybe_unused *name;
>  	int ret;
>  
>  	/* Initialize the video buffers queue. */
> @@ -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 applications 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.
> 

Re: [PATCH v2 0/4] uvc: Restore old vdev name

[Mauro Carvalho Chehab]

Em Tue,  7 Dec 2021 01:38:36 +0100
Ricardo Ribalda <ribalda@chromium.org> escreveu:

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

Yeah, patch 1 of this series makes a lot sense. Reporting a camera
as "Video Capture" doesn't seem too nice, specially if multiple
UVC cameras are present.

Yet, I'm a little in doubt about patch 4/4, for a couple of reasons:

1. IMO, on *all* devices (not only uvc), it makes sense to add a "Metadata" 
   at the name string for the metadata devnodes.

   So, I would implement such logic at V4L2 core instead.

2. Such metadata string should be there not only for the entity name,
   but also for vdev->name;

3. I would, instead, set the device name as:

	vdev->name = "Meta: <foo>"

   for the meta devnodes, as the string size is limited.

4. As almost all devices have either video capture or video
   output, I can't see any value to unconditionally add
   "Video Capture"/"Video Output" strings. It would only make
   sense to have them on devices that report having both.

Regards,
Mauro

> 
> v2:
>  - Add Documentation
>  - Mark maybe unused variables as __maybe_unused
>  - Add Suggested-by
> 
> Ricardo Ribalda (4):
>   Revert "media: uvcvideo: Set unique vdev name based in type"
>   media: v4l2-dev.c: Allow driver-defined entity names
>   media: Documentation/driver-api: Document entity name
>   media: uvcvideo: Set unique entity name based in type
> 
>  Documentation/driver-api/media/v4l2-dev.rst |  4 ++++
>  drivers/media/usb/uvc/uvc_driver.c          | 14 +++++++++++---
>  drivers/media/v4l2-core/v4l2-dev.c          |  4 +++-
>  3 files changed, 18 insertions(+), 4 deletions(-

Thanks,
Mauro