| Message ID | 69d2363e-49bb-4c7f-9c91-6a22b0c6e9e3@xs4all.nl (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC] media: uvc: ensure video device entities have a unique name | expand |
Hi Hans On Thu, 17 Oct 2024 at 18:25, Hans Verkuil <hverkuil@xs4all.nl> wrote: > > The video device names (also used as entity names) are identical > for the video capture and metadata capture devices. > > This series ensures that the video device names are unique. > > It also fixes a bug where a metadata device would be created > for video output devices, but that's for video capture devices > only. You probably want to split that fix in a separate patch, but be aware that there might be userspace depending on the extra metadata device :( > > This fixes a compliance failure when running > 'v4l2-compliance -M /dev/mediaX'. > > Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> > --- > RFC for now. > > Based on the code I assume one UVC device can have multiple > capture and/or output devices, so I added an instance index. I believe we tried something similar and we ended up reverting it :( https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=f66dcb32af19faf49cc4a9222c3152b10c6ec84a Maybe, as you suggest, dropping the Cap prefix for the first inst is enough, if my memory is correct I think I also tried with that but we ended up abandoning the patch https://lore.kernel.org/linux-media/20220920-resend-meta-v5-0-3385df697370@chromium.org/ https://lore.kernel.org/linux-media/Y6OP%2Fqz9R4BgXi4o@pendragon.ideasonboard.com/ > > But I may be wrong about that. > > I also assume that there is no metadata for UVC video outputs. > > It might also be safer to drop the 'Cap0 ' prefix if inst == 0, > just in case someone relies on the name. > --- > drivers/media/usb/uvc/uvc_driver.c | 34 +++++++++++++++++++--------- > drivers/media/usb/uvc/uvc_metadata.c | 4 ++-- > drivers/media/usb/uvc/uvcvideo.h | 3 ++- > 3 files changed, 27 insertions(+), 14 deletions(-) > > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c > index ab9cdb50e74e..59e5769166f2 100644 > --- a/drivers/media/usb/uvc/uvc_driver.c > +++ b/drivers/media/usb/uvc/uvc_driver.c > @@ -1956,6 +1956,7 @@ static void uvc_unregister_video(struct uvc_device *dev) > > int uvc_register_video_device(struct uvc_device *dev, > struct uvc_streaming *stream, > + unsigned int inst, > struct video_device *vdev, > struct uvc_video_queue *queue, > enum v4l2_buf_type type, > @@ -1990,17 +1991,18 @@ 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; > + snprintf(vdev->name, sizeof(vdev->name), "Cap%u %s", inst, dev->name); > break; > case V4L2_BUF_TYPE_VIDEO_OUTPUT: > vdev->device_caps = V4L2_CAP_VIDEO_OUTPUT | V4L2_CAP_STREAMING; > + snprintf(vdev->name, sizeof(vdev->name), "Out%u %s", inst, dev->name); > break; > case V4L2_BUF_TYPE_META_CAPTURE: > vdev->device_caps = V4L2_CAP_META_CAPTURE | V4L2_CAP_STREAMING; > + snprintf(vdev->name, sizeof(vdev->name), "Meta%u %s", inst, dev->name); > break; > } > > - strscpy(vdev->name, dev->name, sizeof(vdev->name)); > - > /* > * Set the driver data before calling video_register_device, otherwise > * the file open() handler might race us. > @@ -2020,7 +2022,7 @@ int uvc_register_video_device(struct uvc_device *dev, > } > > static int uvc_register_video(struct uvc_device *dev, > - struct uvc_streaming *stream) > + struct uvc_streaming *stream, unsigned int inst) > { > int ret; > > @@ -2041,7 +2043,7 @@ static int uvc_register_video(struct uvc_device *dev, > uvc_debugfs_init_stream(stream); > > /* Register the device with V4L. */ > - return uvc_register_video_device(dev, stream, &stream->vdev, > + return uvc_register_video_device(dev, stream, inst, &stream->vdev, > &stream->queue, stream->type, > &uvc_fops, &uvc_ioctl_ops); > } > @@ -2054,9 +2056,13 @@ static int uvc_register_terms(struct uvc_device *dev, > { > struct uvc_streaming *stream; > struct uvc_entity *term; > + unsigned int inst_cap = 0; > + unsigned int inst_out = 0; > int ret; > > list_for_each_entry(term, &chain->entities, chain) { > + bool is_capture; > + > if (UVC_ENTITY_TYPE(term) != UVC_TT_STREAMING) > continue; > > @@ -2069,16 +2075,22 @@ static int uvc_register_terms(struct uvc_device *dev, > } > > stream->chain = chain; > - ret = uvc_register_video(dev, stream); > + is_capture = stream->type == V4L2_BUF_TYPE_VIDEO_CAPTURE; > + ret = uvc_register_video(dev, stream, > + is_capture ? inst_cap : inst_out); > if (ret < 0) > return ret; > > - /* > - * Register a metadata node, but ignore a possible failure, > - * complete registration of video nodes anyway. > - */ > - uvc_meta_register(stream); > - > + if (is_capture) { > + /* > + * Register a metadata node, but ignore a possible failure, > + * complete registration of video nodes anyway. > + */ > + uvc_meta_register(stream, inst_cap); > + inst_cap++; > + } else { > + inst_out++; > + } > term->vdev = &stream->vdev; > } > > diff --git a/drivers/media/usb/uvc/uvc_metadata.c b/drivers/media/usb/uvc/uvc_metadata.c > index 82de7781f5b6..685182302602 100644 > --- a/drivers/media/usb/uvc/uvc_metadata.c > +++ b/drivers/media/usb/uvc/uvc_metadata.c > @@ -156,7 +156,7 @@ static const struct v4l2_file_operations uvc_meta_fops = { > .mmap = vb2_fop_mmap, > }; > > -int uvc_meta_register(struct uvc_streaming *stream) > +int uvc_meta_register(struct uvc_streaming *stream, unsigned int inst) > { > struct uvc_device *dev = stream->dev; > struct video_device *vdev = &stream->meta.vdev; > @@ -170,7 +170,7 @@ int uvc_meta_register(struct uvc_streaming *stream) > */ > vdev->queue = &queue->queue; > > - return uvc_register_video_device(dev, stream, vdev, queue, > + return uvc_register_video_device(dev, stream, inst, vdev, queue, > V4L2_BUF_TYPE_META_CAPTURE, > &uvc_meta_fops, &uvc_meta_ioctl_ops); > } > diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h > index 07f9921d83f2..92c72ef6ea49 100644 > --- a/drivers/media/usb/uvc/uvcvideo.h > +++ b/drivers/media/usb/uvc/uvcvideo.h > @@ -738,10 +738,11 @@ int uvc_query_ctrl(struct uvc_device *dev, u8 query, u8 unit, > void uvc_video_clock_update(struct uvc_streaming *stream, > struct vb2_v4l2_buffer *vbuf, > struct uvc_buffer *buf); > -int uvc_meta_register(struct uvc_streaming *stream); > +int uvc_meta_register(struct uvc_streaming *stream, unsigned int inst); > > int uvc_register_video_device(struct uvc_device *dev, > struct uvc_streaming *stream, > + unsigned int inst, > struct video_device *vdev, > struct uvc_video_queue *queue, > enum v4l2_buf_type type, > -- > 2.43.0 > >
On 17/10/2024 16:48, Ricardo Ribalda wrote: > Hi Hans > > On Thu, 17 Oct 2024 at 18:25, Hans Verkuil <hverkuil@xs4all.nl> wrote: >> >> The video device names (also used as entity names) are identical >> for the video capture and metadata capture devices. >> >> This series ensures that the video device names are unique. >> >> It also fixes a bug where a metadata device would be created >> for video output devices, but that's for video capture devices >> only. > You probably want to split that fix in a separate patch, but be aware > that there might be userspace depending on the extra metadata device > :( > >> >> This fixes a compliance failure when running >> 'v4l2-compliance -M /dev/mediaX'. >> >> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> >> --- >> RFC for now. >> >> Based on the code I assume one UVC device can have multiple >> capture and/or output devices, so I added an instance index. > > I believe we tried something similar and we ended up reverting it :( > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=f66dcb32af19faf49cc4a9222c3152b10c6ec84a > > Maybe, as you suggest, dropping the Cap prefix for the first inst is > enough, if my memory is correct I think I also tried with that but we > ended up abandoning the patch > https://lore.kernel.org/linux-media/20220920-resend-meta-v5-0-3385df697370@chromium.org/ > https://lore.kernel.org/linux-media/Y6OP%2Fqz9R4BgXi4o@pendragon.ideasonboard.com/ >> >> But I may be wrong about that. >> >> I also assume that there is no metadata for UVC video outputs. >> >> It might also be safer to drop the 'Cap0 ' prefix if inst == 0, >> just in case someone relies on the name. Right, the suggestion was to try to just fix the entity name, so I'll see if I can figure out how to do this. Regards, Hans >> --- >> drivers/media/usb/uvc/uvc_driver.c | 34 +++++++++++++++++++--------- >> drivers/media/usb/uvc/uvc_metadata.c | 4 ++-- >> drivers/media/usb/uvc/uvcvideo.h | 3 ++- >> 3 files changed, 27 insertions(+), 14 deletions(-) >> >> diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c >> index ab9cdb50e74e..59e5769166f2 100644 >> --- a/drivers/media/usb/uvc/uvc_driver.c >> +++ b/drivers/media/usb/uvc/uvc_driver.c >> @@ -1956,6 +1956,7 @@ static void uvc_unregister_video(struct uvc_device *dev) >> >> int uvc_register_video_device(struct uvc_device *dev, >> struct uvc_streaming *stream, >> + unsigned int inst, >> struct video_device *vdev, >> struct uvc_video_queue *queue, >> enum v4l2_buf_type type, >> @@ -1990,17 +1991,18 @@ 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; >> + snprintf(vdev->name, sizeof(vdev->name), "Cap%u %s", inst, dev->name); >> break; >> case V4L2_BUF_TYPE_VIDEO_OUTPUT: >> vdev->device_caps = V4L2_CAP_VIDEO_OUTPUT | V4L2_CAP_STREAMING; >> + snprintf(vdev->name, sizeof(vdev->name), "Out%u %s", inst, dev->name); >> break; >> case V4L2_BUF_TYPE_META_CAPTURE: >> vdev->device_caps = V4L2_CAP_META_CAPTURE | V4L2_CAP_STREAMING; >> + snprintf(vdev->name, sizeof(vdev->name), "Meta%u %s", inst, dev->name); >> break; >> } >> >> - strscpy(vdev->name, dev->name, sizeof(vdev->name)); >> - >> /* >> * Set the driver data before calling video_register_device, otherwise >> * the file open() handler might race us. >> @@ -2020,7 +2022,7 @@ int uvc_register_video_device(struct uvc_device *dev, >> } >> >> static int uvc_register_video(struct uvc_device *dev, >> - struct uvc_streaming *stream) >> + struct uvc_streaming *stream, unsigned int inst) >> { >> int ret; >> >> @@ -2041,7 +2043,7 @@ static int uvc_register_video(struct uvc_device *dev, >> uvc_debugfs_init_stream(stream); >> >> /* Register the device with V4L. */ >> - return uvc_register_video_device(dev, stream, &stream->vdev, >> + return uvc_register_video_device(dev, stream, inst, &stream->vdev, >> &stream->queue, stream->type, >> &uvc_fops, &uvc_ioctl_ops); >> } >> @@ -2054,9 +2056,13 @@ static int uvc_register_terms(struct uvc_device *dev, >> { >> struct uvc_streaming *stream; >> struct uvc_entity *term; >> + unsigned int inst_cap = 0; >> + unsigned int inst_out = 0; >> int ret; >> >> list_for_each_entry(term, &chain->entities, chain) { >> + bool is_capture; >> + >> if (UVC_ENTITY_TYPE(term) != UVC_TT_STREAMING) >> continue; >> >> @@ -2069,16 +2075,22 @@ static int uvc_register_terms(struct uvc_device *dev, >> } >> >> stream->chain = chain; >> - ret = uvc_register_video(dev, stream); >> + is_capture = stream->type == V4L2_BUF_TYPE_VIDEO_CAPTURE; >> + ret = uvc_register_video(dev, stream, >> + is_capture ? inst_cap : inst_out); >> if (ret < 0) >> return ret; >> >> - /* >> - * Register a metadata node, but ignore a possible failure, >> - * complete registration of video nodes anyway. >> - */ >> - uvc_meta_register(stream); >> - >> + if (is_capture) { >> + /* >> + * Register a metadata node, but ignore a possible failure, >> + * complete registration of video nodes anyway. >> + */ >> + uvc_meta_register(stream, inst_cap); >> + inst_cap++; >> + } else { >> + inst_out++; >> + } >> term->vdev = &stream->vdev; >> } >> >> diff --git a/drivers/media/usb/uvc/uvc_metadata.c b/drivers/media/usb/uvc/uvc_metadata.c >> index 82de7781f5b6..685182302602 100644 >> --- a/drivers/media/usb/uvc/uvc_metadata.c >> +++ b/drivers/media/usb/uvc/uvc_metadata.c >> @@ -156,7 +156,7 @@ static const struct v4l2_file_operations uvc_meta_fops = { >> .mmap = vb2_fop_mmap, >> }; >> >> -int uvc_meta_register(struct uvc_streaming *stream) >> +int uvc_meta_register(struct uvc_streaming *stream, unsigned int inst) >> { >> struct uvc_device *dev = stream->dev; >> struct video_device *vdev = &stream->meta.vdev; >> @@ -170,7 +170,7 @@ int uvc_meta_register(struct uvc_streaming *stream) >> */ >> vdev->queue = &queue->queue; >> >> - return uvc_register_video_device(dev, stream, vdev, queue, >> + return uvc_register_video_device(dev, stream, inst, vdev, queue, >> V4L2_BUF_TYPE_META_CAPTURE, >> &uvc_meta_fops, &uvc_meta_ioctl_ops); >> } >> diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h >> index 07f9921d83f2..92c72ef6ea49 100644 >> --- a/drivers/media/usb/uvc/uvcvideo.h >> +++ b/drivers/media/usb/uvc/uvcvideo.h >> @@ -738,10 +738,11 @@ int uvc_query_ctrl(struct uvc_device *dev, u8 query, u8 unit, >> void uvc_video_clock_update(struct uvc_streaming *stream, >> struct vb2_v4l2_buffer *vbuf, >> struct uvc_buffer *buf); >> -int uvc_meta_register(struct uvc_streaming *stream); >> +int uvc_meta_register(struct uvc_streaming *stream, unsigned int inst); >> >> int uvc_register_video_device(struct uvc_device *dev, >> struct uvc_streaming *stream, >> + unsigned int inst, >> struct video_device *vdev, >> struct uvc_video_queue *queue, >> enum v4l2_buf_type type, >> -- >> 2.43.0 >> >> > >
diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c index ab9cdb50e74e..59e5769166f2 100644 --- a/drivers/media/usb/uvc/uvc_driver.c +++ b/drivers/media/usb/uvc/uvc_driver.c @@ -1956,6 +1956,7 @@ static void uvc_unregister_video(struct uvc_device *dev) int uvc_register_video_device(struct uvc_device *dev, struct uvc_streaming *stream, + unsigned int inst, struct video_device *vdev, struct uvc_video_queue *queue, enum v4l2_buf_type type, @@ -1990,17 +1991,18 @@ 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; + snprintf(vdev->name, sizeof(vdev->name), "Cap%u %s", inst, dev->name); break; case V4L2_BUF_TYPE_VIDEO_OUTPUT: vdev->device_caps = V4L2_CAP_VIDEO_OUTPUT | V4L2_CAP_STREAMING; + snprintf(vdev->name, sizeof(vdev->name), "Out%u %s", inst, dev->name); break; case V4L2_BUF_TYPE_META_CAPTURE: vdev->device_caps = V4L2_CAP_META_CAPTURE | V4L2_CAP_STREAMING; + snprintf(vdev->name, sizeof(vdev->name), "Meta%u %s", inst, dev->name); break; } - strscpy(vdev->name, dev->name, sizeof(vdev->name)); - /* * Set the driver data before calling video_register_device, otherwise * the file open() handler might race us. @@ -2020,7 +2022,7 @@ int uvc_register_video_device(struct uvc_device *dev, } static int uvc_register_video(struct uvc_device *dev, - struct uvc_streaming *stream) + struct uvc_streaming *stream, unsigned int inst) { int ret; @@ -2041,7 +2043,7 @@ static int uvc_register_video(struct uvc_device *dev, uvc_debugfs_init_stream(stream); /* Register the device with V4L. */ - return uvc_register_video_device(dev, stream, &stream->vdev, + return uvc_register_video_device(dev, stream, inst, &stream->vdev, &stream->queue, stream->type, &uvc_fops, &uvc_ioctl_ops); } @@ -2054,9 +2056,13 @@ static int uvc_register_terms(struct uvc_device *dev, { struct uvc_streaming *stream; struct uvc_entity *term; + unsigned int inst_cap = 0; + unsigned int inst_out = 0; int ret; list_for_each_entry(term, &chain->entities, chain) { + bool is_capture; + if (UVC_ENTITY_TYPE(term) != UVC_TT_STREAMING) continue; @@ -2069,16 +2075,22 @@ static int uvc_register_terms(struct uvc_device *dev, } stream->chain = chain; - ret = uvc_register_video(dev, stream); + is_capture = stream->type == V4L2_BUF_TYPE_VIDEO_CAPTURE; + ret = uvc_register_video(dev, stream, + is_capture ? inst_cap : inst_out); if (ret < 0) return ret; - /* - * Register a metadata node, but ignore a possible failure, - * complete registration of video nodes anyway. - */ - uvc_meta_register(stream); - + if (is_capture) { + /* + * Register a metadata node, but ignore a possible failure, + * complete registration of video nodes anyway. + */ + uvc_meta_register(stream, inst_cap); + inst_cap++; + } else { + inst_out++; + } term->vdev = &stream->vdev; } diff --git a/drivers/media/usb/uvc/uvc_metadata.c b/drivers/media/usb/uvc/uvc_metadata.c index 82de7781f5b6..685182302602 100644 --- a/drivers/media/usb/uvc/uvc_metadata.c +++ b/drivers/media/usb/uvc/uvc_metadata.c @@ -156,7 +156,7 @@ static const struct v4l2_file_operations uvc_meta_fops = { .mmap = vb2_fop_mmap, }; -int uvc_meta_register(struct uvc_streaming *stream) +int uvc_meta_register(struct uvc_streaming *stream, unsigned int inst) { struct uvc_device *dev = stream->dev; struct video_device *vdev = &stream->meta.vdev; @@ -170,7 +170,7 @@ int uvc_meta_register(struct uvc_streaming *stream) */ vdev->queue = &queue->queue; - return uvc_register_video_device(dev, stream, vdev, queue, + return uvc_register_video_device(dev, stream, inst, vdev, queue, V4L2_BUF_TYPE_META_CAPTURE, &uvc_meta_fops, &uvc_meta_ioctl_ops); } diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h index 07f9921d83f2..92c72ef6ea49 100644 --- a/drivers/media/usb/uvc/uvcvideo.h +++ b/drivers/media/usb/uvc/uvcvideo.h @@ -738,10 +738,11 @@ int uvc_query_ctrl(struct uvc_device *dev, u8 query, u8 unit, void uvc_video_clock_update(struct uvc_streaming *stream, struct vb2_v4l2_buffer *vbuf, struct uvc_buffer *buf); -int uvc_meta_register(struct uvc_streaming *stream); +int uvc_meta_register(struct uvc_streaming *stream, unsigned int inst); int uvc_register_video_device(struct uvc_device *dev, struct uvc_streaming *stream, + unsigned int inst, struct video_device *vdev, struct uvc_video_queue *queue, enum v4l2_buf_type type,
The video device names (also used as entity names) are identical for the video capture and metadata capture devices. This series ensures that the video device names are unique. It also fixes a bug where a metadata device would be created for video output devices, but that's for video capture devices only. This fixes a compliance failure when running 'v4l2-compliance -M /dev/mediaX'. Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> --- RFC for now. Based on the code I assume one UVC device can have multiple capture and/or output devices, so I added an instance index. But I may be wrong about that. I also assume that there is no metadata for UVC video outputs. It might also be safer to drop the 'Cap0 ' prefix if inst == 0, just in case someone relies on the name. --- drivers/media/usb/uvc/uvc_driver.c | 34 +++++++++++++++++++--------- drivers/media/usb/uvc/uvc_metadata.c | 4 ++-- drivers/media/usb/uvc/uvcvideo.h | 3 ++- 3 files changed, 27 insertions(+), 14 deletions(-)