[REGRESSION] Re: [PATCH v10 11/21] media: uvcvideo: Set unique vdev name based in type

From: Nicolas Dufresne <nicolas@ndufresne.ca>
To: Ricardo Ribalda <ribalda@chromium.org>,
	Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
	Hans Verkuil <hverkuil-cisco@xs4all.nl>,
	Mauro Carvalho Chehab <mchehab@kernel.org>,
	Sergey Senozhatsky <sergey.senozhatsky@gmail.com>,
	linux-media@vger.kernel.org, linux-kernel@vger.kernel.org,
	tfiga@chromium.org
Cc: linux-kernel@vger.kernel.org, stable <stable@vger.kernel.org>
Subject: [REGRESSION] Re: [PATCH v10 11/21] media: uvcvideo: Set unique vdev name based in type
Date: Mon, 06 Dec 2021 14:05:06 -0500	[thread overview]
Message-ID: <b4bfa8b8f3d8f25c48a3b0b81a0e87dc90f111af.camel@ndufresne.ca> (raw)
In-Reply-To: <20210618122923.385938-12-ribalda@chromium.org>

Hi Ricard, 

Le vendredi 18 juin 2021 à 14:29 +0200, Ricardo Ribalda a écrit :
> All the entities must have a unique name. We can have a descriptive and
> unique name by appending the function and the entity->id.

Thanks for your work. The only issue is that unfortunately this change cause an
important regression for users. All UVC cameras in all UIs seems to no longer
include any information about the camera. As an example, I have two cameras on
my system and Firefox, Chrome, Cheese, Zoom and MS Team all agree that my camera
are now:

  Video Capture 4
  Video Capture 5

Previously they would be shown as something like:

  StreamCam
  Integrated

We should probably revert this change quickly before it get deployed more
widely, I have notice the backport being sent for 5.4, 5.10 and 5.14. I'm using
5.15 shipped by Fedora team.

As a proper solution, maybe I could suggest to keep using dev->name, but trim it
enough to fit the " N" string to guaranty that you have enough space in this
limited 32 char string and use that instead ? This should fit the uniqueness
requirement without the sacrifice of the only possibly useful information we had
in that limited string.

regards,
Nicolas

> 
> This is even resilent 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: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> ---
>  drivers/media/usb/uvc/uvc_driver.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> index 14b60792ffab..037bf80d1100 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_file_operations *fops,
>  			      const struct v4l2_ioctl_ops *ioctl_ops)
>  {
> +	const char *name;
>  	int ret;
>  
>  	/* Initialize the video buffers queue. */
> @@ -2222,16 +2223,20 @@ 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;
>  	}
>  
> -	strscpy(vdev->name, dev->name, sizeof(vdev->name));
> +	snprintf(vdev->name, sizeof(vdev->name), "%s %u", name,
> +		 stream->header.bTerminalLink);
>  
>  	/*
>  	 * Set the driver data before calling video_register_device, otherwise