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(-)
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
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";name = "Video Output";name = "Metadata";
- 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
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";name = "Video Output";name = "Metadata";
- 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
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;
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;
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.
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";name = "Video Output";name = "Metadata";
- /*
* Many userspace apps identify the device with vdev->name, so we
s/apps/applications/
* cannot change its name for its function.*/+#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.
linux-stable-mirror@lists.linaro.org
-
Laurent Pinchart -
Ricardo Ribalda