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
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: <[email protected]>
Fixes: e3f60e7e1a2b ("media: uvcvideo: Set unique vdev name based in type")
Reported-by: Nicolas Dufresne <[email protected]>
Signed-off-by: Ricardo Ribalda <[email protected]>
Reviewed-by: Laurent Pinchart <[email protected]>
---
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
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 <[email protected]>
Signed-off-by: Ricardo Ribalda <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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: <[email protected]>
> Fixes: e3f60e7e1a2b ("media: uvcvideo: Set unique vdev name based in type")
> Reported-by: Nicolas Dufresne <[email protected]>
> Signed-off-by: Ricardo Ribalda <[email protected]>
> Reviewed-by: Laurent Pinchart <[email protected]>
Reviewed-by: Hans Verkuil <[email protected]>
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
>
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 <[email protected]>
> Signed-off-by: Ricardo Ribalda <[email protected]>
Reviewed-by: Hans Verkuil <[email protected]>
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;
>
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 <[email protected]>
Reviewed-by: Hans Verkuil <[email protected]>
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
> ------------------
>
>
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 <[email protected]>
Reviewed-by: Hans Verkuil <[email protected]>
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.
>
Em Tue, 7 Dec 2021 01:38:36 +0100
Ricardo Ribalda <[email protected]> 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