The is_streaming field is used by modular PM to know if the device is
currently streaming or not.
With the transition to vb2 and fop helpers, we can use vb2 functions for
the same functionality. The great benefit is that vb2 already takes
track of the streaming state for us.
Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
drivers/media/usb/uvc/uvc_queue.c | 11 ++++++++-
drivers/media/usb/uvc/uvc_v4l2.c | 51 ++-------------------------------------
drivers/media/usb/uvc/uvcvideo.h | 1 -
3 files changed, 12 insertions(+), 51 deletions(-)
diff --git a/drivers/media/usb/uvc/uvc_queue.c b/drivers/media/usb/uvc/uvc_queue.c
index 72c5494dee9f46ff61072e7d293bfaddda40e615..dff93bec204428b8aebc09332e0322fa68823fa4 100644
--- a/drivers/media/usb/uvc/uvc_queue.c
+++ b/drivers/media/usb/uvc/uvc_queue.c
@@ -165,12 +165,18 @@ static int uvc_start_streaming(struct vb2_queue *vq, unsigned int count)
lockdep_assert_irqs_enabled();
+ ret = uvc_pm_get(stream->dev);
+ if (ret)
+ return ret;
+
queue->buf_used = 0;
ret = uvc_video_start_streaming(stream);
if (ret == 0)
return 0;
+ uvc_pm_put(stream->dev);
+
spin_lock_irq(&queue->irqlock);
uvc_queue_return_buffers(queue, UVC_BUF_STATE_QUEUED);
spin_unlock_irq(&queue->irqlock);
@@ -181,11 +187,14 @@ static int uvc_start_streaming(struct vb2_queue *vq, unsigned int count)
static void uvc_stop_streaming(struct vb2_queue *vq)
{
struct uvc_video_queue *queue = vb2_get_drv_priv(vq);
+ struct uvc_streaming *stream = uvc_queue_to_stream(queue);
lockdep_assert_irqs_enabled();
- if (vq->type != V4L2_BUF_TYPE_META_CAPTURE)
+ if (vq->type != V4L2_BUF_TYPE_META_CAPTURE) {
+ uvc_pm_put(stream->dev);
uvc_video_stop_streaming(uvc_queue_to_stream(queue));
+ }
spin_lock_irq(&queue->irqlock);
uvc_queue_return_buffers(queue, UVC_BUF_STATE_ERROR);
diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
index 7a5ecbefa32c0a6b74c85d7f77a25b433598471e..d4bee0d4334b764c0cf02363b573b55fb44eb228 100644
--- a/drivers/media/usb/uvc/uvc_v4l2.c
+++ b/drivers/media/usb/uvc/uvc_v4l2.c
@@ -617,9 +617,6 @@ static int uvc_v4l2_release(struct file *file)
uvc_ctrl_cleanup_fh(handle);
- if (handle->is_streaming)
- uvc_pm_put(stream->dev);
-
/* Release the file handle. */
vb2_fop_release(file);
file->private_data = NULL;
@@ -677,50 +674,6 @@ static int uvc_ioctl_try_fmt(struct file *file, void *fh,
return uvc_v4l2_try_format(stream, fmt, &probe, NULL, NULL);
}
-static int uvc_ioctl_streamon(struct file *file, void *fh,
- enum v4l2_buf_type type)
-{
- struct uvc_fh *handle = fh;
- struct uvc_streaming *stream = handle->stream;
- int ret;
-
- if (handle->is_streaming)
- return 0;
-
- ret = uvc_pm_get(stream->dev);
- if (ret)
- return ret;
-
- ret = vb2_ioctl_streamon(file, fh, type);
- if (ret) {
- uvc_pm_put(stream->dev);
- return ret;
- }
-
- handle->is_streaming = true;
-
- return 0;
-}
-
-static int uvc_ioctl_streamoff(struct file *file, void *fh,
- enum v4l2_buf_type type)
-{
- struct uvc_fh *handle = fh;
- struct uvc_streaming *stream = handle->stream;
- int ret;
-
- ret = vb2_ioctl_streamoff(file, fh, type);
- if (ret)
- return ret;
-
- if (handle->is_streaming) {
- handle->is_streaming = false;
- uvc_pm_put(stream->dev);
- }
-
- return 0;
-}
-
static int uvc_ioctl_enum_input(struct file *file, void *fh,
struct v4l2_input *input)
{
@@ -1323,8 +1276,8 @@ const struct v4l2_ioctl_ops uvc_ioctl_ops = {
.vidioc_expbuf = vb2_ioctl_expbuf,
.vidioc_dqbuf = vb2_ioctl_dqbuf,
.vidioc_create_bufs = vb2_ioctl_create_bufs,
- .vidioc_streamon = uvc_ioctl_streamon,
- .vidioc_streamoff = uvc_ioctl_streamoff,
+ .vidioc_streamon = vb2_ioctl_streamon,
+ .vidioc_streamoff = vb2_ioctl_streamoff,
.vidioc_enum_input = uvc_ioctl_enum_input,
.vidioc_g_input = uvc_ioctl_g_input,
.vidioc_s_input = uvc_ioctl_s_input,
diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
index 3ddbf065a2cbae40ee48cb06f84ca8f0052990c4..f895f690f7cdc1af942d5f3a5f10e9dd1c956a35 100644
--- a/drivers/media/usb/uvc/uvcvideo.h
+++ b/drivers/media/usb/uvc/uvcvideo.h
@@ -626,7 +626,6 @@ struct uvc_fh {
struct uvc_video_chain *chain;
struct uvc_streaming *stream;
unsigned int pending_async_ctrls;
- bool is_streaming;
};
/* ------------------------------------------------------------------------
--
2.49.0.1266.g31b7d2e469-googOn 02/06/2025 14:59, Ricardo Ribalda wrote:
> The is_streaming field is used by modular PM to know if the device is
> currently streaming or not.
>
> With the transition to vb2 and fop helpers, we can use vb2 functions for
> the same functionality. The great benefit is that vb2 already takes
> track of the streaming state for us.
>
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
> drivers/media/usb/uvc/uvc_queue.c | 11 ++++++++-
> drivers/media/usb/uvc/uvc_v4l2.c | 51 ++-------------------------------------
> drivers/media/usb/uvc/uvcvideo.h | 1 -
> 3 files changed, 12 insertions(+), 51 deletions(-)
>
> diff --git a/drivers/media/usb/uvc/uvc_queue.c b/drivers/media/usb/uvc/uvc_queue.c
> index 72c5494dee9f46ff61072e7d293bfaddda40e615..dff93bec204428b8aebc09332e0322fa68823fa4 100644
> --- a/drivers/media/usb/uvc/uvc_queue.c
> +++ b/drivers/media/usb/uvc/uvc_queue.c
> @@ -165,12 +165,18 @@ static int uvc_start_streaming(struct vb2_queue *vq, unsigned int count)
>
> lockdep_assert_irqs_enabled();
>
> + ret = uvc_pm_get(stream->dev);
> + if (ret)
> + return ret;
> +
> queue->buf_used = 0;
>
> ret = uvc_video_start_streaming(stream);
I'm not sure this is correct. See comments below.
> if (ret == 0)
> return 0;
>
> + uvc_pm_put(stream->dev);
> +
> spin_lock_irq(&queue->irqlock);
> uvc_queue_return_buffers(queue, UVC_BUF_STATE_QUEUED);
> spin_unlock_irq(&queue->irqlock);
> @@ -181,11 +187,14 @@ static int uvc_start_streaming(struct vb2_queue *vq, unsigned int count)
> static void uvc_stop_streaming(struct vb2_queue *vq)
> {
> struct uvc_video_queue *queue = vb2_get_drv_priv(vq);
> + struct uvc_streaming *stream = uvc_queue_to_stream(queue);
>
> lockdep_assert_irqs_enabled();
>
> - if (vq->type != V4L2_BUF_TYPE_META_CAPTURE)
> + if (vq->type != V4L2_BUF_TYPE_META_CAPTURE) {
> + uvc_pm_put(stream->dev);
This doesn't look right, for both video and metadata uvc_pm_get is called,
but only for video is put called.
> uvc_video_stop_streaming(uvc_queue_to_stream(queue));
And this is odd too.
> + }
My assumption is that uvc_video_start_streaming and uvc_video_stop_streaming
are valid for both video and meta: i.e. the first time you start streaming
(either video or meta) you call uvc_video_start_streaming. If you were already
streaming for e.g. video, then start streaming metadata (or vice versa), then
you don't need to do anything in start_streaming.
Same for stop_streaming: only if both video and metadata stopped streaming
is uvc_video_stop_streaming called.
Please correct me if I am wrong.
In any case, if I am right, then you have to rework this code accordingly.
Regardless, you need to test various sequences of streaming video and metadata
in different orders and make sure this is handled correctly.
Regards,
Hans
>
> spin_lock_irq(&queue->irqlock);
> uvc_queue_return_buffers(queue, UVC_BUF_STATE_ERROR);
> diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
> index 7a5ecbefa32c0a6b74c85d7f77a25b433598471e..d4bee0d4334b764c0cf02363b573b55fb44eb228 100644
> --- a/drivers/media/usb/uvc/uvc_v4l2.c
> +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> @@ -617,9 +617,6 @@ static int uvc_v4l2_release(struct file *file)
>
> uvc_ctrl_cleanup_fh(handle);
>
> - if (handle->is_streaming)
> - uvc_pm_put(stream->dev);
> -
> /* Release the file handle. */
> vb2_fop_release(file);
> file->private_data = NULL;
> @@ -677,50 +674,6 @@ static int uvc_ioctl_try_fmt(struct file *file, void *fh,
> return uvc_v4l2_try_format(stream, fmt, &probe, NULL, NULL);
> }
>
> -static int uvc_ioctl_streamon(struct file *file, void *fh,
> - enum v4l2_buf_type type)
> -{
> - struct uvc_fh *handle = fh;
> - struct uvc_streaming *stream = handle->stream;
> - int ret;
> -
> - if (handle->is_streaming)
> - return 0;
> -
> - ret = uvc_pm_get(stream->dev);
> - if (ret)
> - return ret;
> -
> - ret = vb2_ioctl_streamon(file, fh, type);
> - if (ret) {
> - uvc_pm_put(stream->dev);
> - return ret;
> - }
> -
> - handle->is_streaming = true;
> -
> - return 0;
> -}
> -
> -static int uvc_ioctl_streamoff(struct file *file, void *fh,
> - enum v4l2_buf_type type)
> -{
> - struct uvc_fh *handle = fh;
> - struct uvc_streaming *stream = handle->stream;
> - int ret;
> -
> - ret = vb2_ioctl_streamoff(file, fh, type);
> - if (ret)
> - return ret;
> -
> - if (handle->is_streaming) {
> - handle->is_streaming = false;
> - uvc_pm_put(stream->dev);
> - }
> -
> - return 0;
> -}
> -
> static int uvc_ioctl_enum_input(struct file *file, void *fh,
> struct v4l2_input *input)
> {
> @@ -1323,8 +1276,8 @@ const struct v4l2_ioctl_ops uvc_ioctl_ops = {
> .vidioc_expbuf = vb2_ioctl_expbuf,
> .vidioc_dqbuf = vb2_ioctl_dqbuf,
> .vidioc_create_bufs = vb2_ioctl_create_bufs,
> - .vidioc_streamon = uvc_ioctl_streamon,
> - .vidioc_streamoff = uvc_ioctl_streamoff,
> + .vidioc_streamon = vb2_ioctl_streamon,
> + .vidioc_streamoff = vb2_ioctl_streamoff,
> .vidioc_enum_input = uvc_ioctl_enum_input,
> .vidioc_g_input = uvc_ioctl_g_input,
> .vidioc_s_input = uvc_ioctl_s_input,
> diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> index 3ddbf065a2cbae40ee48cb06f84ca8f0052990c4..f895f690f7cdc1af942d5f3a5f10e9dd1c956a35 100644
> --- a/drivers/media/usb/uvc/uvcvideo.h
> +++ b/drivers/media/usb/uvc/uvcvideo.h
> @@ -626,7 +626,6 @@ struct uvc_fh {
> struct uvc_video_chain *chain;
> struct uvc_streaming *stream;
> unsigned int pending_async_ctrls;
> - bool is_streaming;
> };
>
> /* ------------------------------------------------------------------------
>
Hi Hans
On Mon, 2 Jun 2025 at 15:23, Hans Verkuil <hans@jjverkuil.nl> wrote:
>
> On 02/06/2025 14:59, Ricardo Ribalda wrote:
> > The is_streaming field is used by modular PM to know if the device is
> > currently streaming or not.
> >
> > With the transition to vb2 and fop helpers, we can use vb2 functions for
> > the same functionality. The great benefit is that vb2 already takes
> > track of the streaming state for us.
> >
> > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > ---
> > drivers/media/usb/uvc/uvc_queue.c | 11 ++++++++-
> > drivers/media/usb/uvc/uvc_v4l2.c | 51 ++-------------------------------------
> > drivers/media/usb/uvc/uvcvideo.h | 1 -
> > 3 files changed, 12 insertions(+), 51 deletions(-)
> >
> > diff --git a/drivers/media/usb/uvc/uvc_queue.c b/drivers/media/usb/uvc/uvc_queue.c
> > index 72c5494dee9f46ff61072e7d293bfaddda40e615..dff93bec204428b8aebc09332e0322fa68823fa4 100644
> > --- a/drivers/media/usb/uvc/uvc_queue.c
> > +++ b/drivers/media/usb/uvc/uvc_queue.c
> > @@ -165,12 +165,18 @@ static int uvc_start_streaming(struct vb2_queue *vq, unsigned int count)
> >
> > lockdep_assert_irqs_enabled();
> >
> > + ret = uvc_pm_get(stream->dev);
> > + if (ret)
> > + return ret;
> > +
> > queue->buf_used = 0;
> >
> > ret = uvc_video_start_streaming(stream);
>
> I'm not sure this is correct. See comments below.
>
> > if (ret == 0)
> > return 0;
> >
> > + uvc_pm_put(stream->dev);
> > +
> > spin_lock_irq(&queue->irqlock);
> > uvc_queue_return_buffers(queue, UVC_BUF_STATE_QUEUED);
> > spin_unlock_irq(&queue->irqlock);
> > @@ -181,11 +187,14 @@ static int uvc_start_streaming(struct vb2_queue *vq, unsigned int count)
> > static void uvc_stop_streaming(struct vb2_queue *vq)
> > {
> > struct uvc_video_queue *queue = vb2_get_drv_priv(vq);
> > + struct uvc_streaming *stream = uvc_queue_to_stream(queue);
> >
> > lockdep_assert_irqs_enabled();
> >
> > - if (vq->type != V4L2_BUF_TYPE_META_CAPTURE)
> > + if (vq->type != V4L2_BUF_TYPE_META_CAPTURE) {
> > + uvc_pm_put(stream->dev);
>
> This doesn't look right, for both video and metadata uvc_pm_get is called,
> but only for video is put called.
Please take a look at
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/media/usb/uvc/uvc_queue.c#n195
start_streaming is not called for metadata nodes, only for video nodes.
>
> > uvc_video_stop_streaming(uvc_queue_to_stream(queue));
>
> And this is odd too.
>
> > + }
>
> My assumption is that uvc_video_start_streaming and uvc_video_stop_streaming
> are valid for both video and meta: i.e. the first time you start streaming
> (either video or meta) you call uvc_video_start_streaming. If you were already
> streaming for e.g. video, then start streaming metadata (or vice versa), then
> you don't need to do anything in start_streaming.
>
> Same for stop_streaming: only if both video and metadata stopped streaming
> is uvc_video_stop_streaming called.
>
> Please correct me if I am wrong.
>
> In any case, if I am right, then you have to rework this code accordingly.
>
> Regardless, you need to test various sequences of streaming video and metadata
> in different orders and make sure this is handled correctly.
I have tried streaming and getting frames. After some seconds the
device turns off as expected.
>
> Regards,
>
> Hans
>
> >
> > spin_lock_irq(&queue->irqlock);
> > uvc_queue_return_buffers(queue, UVC_BUF_STATE_ERROR);
> > diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
> > index 7a5ecbefa32c0a6b74c85d7f77a25b433598471e..d4bee0d4334b764c0cf02363b573b55fb44eb228 100644
> > --- a/drivers/media/usb/uvc/uvc_v4l2.c
> > +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> > @@ -617,9 +617,6 @@ static int uvc_v4l2_release(struct file *file)
> >
> > uvc_ctrl_cleanup_fh(handle);
> >
> > - if (handle->is_streaming)
> > - uvc_pm_put(stream->dev);
> > -
> > /* Release the file handle. */
> > vb2_fop_release(file);
> > file->private_data = NULL;
> > @@ -677,50 +674,6 @@ static int uvc_ioctl_try_fmt(struct file *file, void *fh,
> > return uvc_v4l2_try_format(stream, fmt, &probe, NULL, NULL);
> > }
> >
> > -static int uvc_ioctl_streamon(struct file *file, void *fh,
> > - enum v4l2_buf_type type)
> > -{
> > - struct uvc_fh *handle = fh;
> > - struct uvc_streaming *stream = handle->stream;
> > - int ret;
> > -
> > - if (handle->is_streaming)
> > - return 0;
> > -
> > - ret = uvc_pm_get(stream->dev);
> > - if (ret)
> > - return ret;
> > -
> > - ret = vb2_ioctl_streamon(file, fh, type);
> > - if (ret) {
> > - uvc_pm_put(stream->dev);
> > - return ret;
> > - }
> > -
> > - handle->is_streaming = true;
> > -
> > - return 0;
> > -}
> > -
> > -static int uvc_ioctl_streamoff(struct file *file, void *fh,
> > - enum v4l2_buf_type type)
> > -{
> > - struct uvc_fh *handle = fh;
> > - struct uvc_streaming *stream = handle->stream;
> > - int ret;
> > -
> > - ret = vb2_ioctl_streamoff(file, fh, type);
> > - if (ret)
> > - return ret;
> > -
> > - if (handle->is_streaming) {
> > - handle->is_streaming = false;
> > - uvc_pm_put(stream->dev);
> > - }
> > -
> > - return 0;
> > -}
> > -
> > static int uvc_ioctl_enum_input(struct file *file, void *fh,
> > struct v4l2_input *input)
> > {
> > @@ -1323,8 +1276,8 @@ const struct v4l2_ioctl_ops uvc_ioctl_ops = {
> > .vidioc_expbuf = vb2_ioctl_expbuf,
> > .vidioc_dqbuf = vb2_ioctl_dqbuf,
> > .vidioc_create_bufs = vb2_ioctl_create_bufs,
> > - .vidioc_streamon = uvc_ioctl_streamon,
> > - .vidioc_streamoff = uvc_ioctl_streamoff,
> > + .vidioc_streamon = vb2_ioctl_streamon,
> > + .vidioc_streamoff = vb2_ioctl_streamoff,
> > .vidioc_enum_input = uvc_ioctl_enum_input,
> > .vidioc_g_input = uvc_ioctl_g_input,
> > .vidioc_s_input = uvc_ioctl_s_input,
> > diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> > index 3ddbf065a2cbae40ee48cb06f84ca8f0052990c4..f895f690f7cdc1af942d5f3a5f10e9dd1c956a35 100644
> > --- a/drivers/media/usb/uvc/uvcvideo.h
> > +++ b/drivers/media/usb/uvc/uvcvideo.h
> > @@ -626,7 +626,6 @@ struct uvc_fh {
> > struct uvc_video_chain *chain;
> > struct uvc_streaming *stream;
> > unsigned int pending_async_ctrls;
> > - bool is_streaming;
> > };
> >
> > /* ------------------------------------------------------------------------
> >
>
--
Ricardo Ribalda
On 02/06/2025 15:33, Ricardo Ribalda wrote:
> Hi Hans
>
> On Mon, 2 Jun 2025 at 15:23, Hans Verkuil <hans@jjverkuil.nl> wrote:
>>
>> On 02/06/2025 14:59, Ricardo Ribalda wrote:
>>> The is_streaming field is used by modular PM to know if the device is
>>> currently streaming or not.
>>>
>>> With the transition to vb2 and fop helpers, we can use vb2 functions for
>>> the same functionality. The great benefit is that vb2 already takes
>>> track of the streaming state for us.
>>>
>>> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
>>> ---
>>> drivers/media/usb/uvc/uvc_queue.c | 11 ++++++++-
>>> drivers/media/usb/uvc/uvc_v4l2.c | 51 ++-------------------------------------
>>> drivers/media/usb/uvc/uvcvideo.h | 1 -
>>> 3 files changed, 12 insertions(+), 51 deletions(-)
>>>
>>> diff --git a/drivers/media/usb/uvc/uvc_queue.c b/drivers/media/usb/uvc/uvc_queue.c
>>> index 72c5494dee9f46ff61072e7d293bfaddda40e615..dff93bec204428b8aebc09332e0322fa68823fa4 100644
>>> --- a/drivers/media/usb/uvc/uvc_queue.c
>>> +++ b/drivers/media/usb/uvc/uvc_queue.c
>>> @@ -165,12 +165,18 @@ static int uvc_start_streaming(struct vb2_queue *vq, unsigned int count)
>>>
>>> lockdep_assert_irqs_enabled();
>>>
>>> + ret = uvc_pm_get(stream->dev);
>>> + if (ret)
>>> + return ret;
>>> +
>>> queue->buf_used = 0;
>>>
>>> ret = uvc_video_start_streaming(stream);
>>
>> I'm not sure this is correct. See comments below.
>>
>>> if (ret == 0)
>>> return 0;
>>>
>>> + uvc_pm_put(stream->dev);
>>> +
>>> spin_lock_irq(&queue->irqlock);
>>> uvc_queue_return_buffers(queue, UVC_BUF_STATE_QUEUED);
>>> spin_unlock_irq(&queue->irqlock);
>>> @@ -181,11 +187,14 @@ static int uvc_start_streaming(struct vb2_queue *vq, unsigned int count)
>>> static void uvc_stop_streaming(struct vb2_queue *vq)
>>> {
>>> struct uvc_video_queue *queue = vb2_get_drv_priv(vq);
>>> + struct uvc_streaming *stream = uvc_queue_to_stream(queue);
>>>
>>> lockdep_assert_irqs_enabled();
>>>
>>> - if (vq->type != V4L2_BUF_TYPE_META_CAPTURE)
>>> + if (vq->type != V4L2_BUF_TYPE_META_CAPTURE) {
>>> + uvc_pm_put(stream->dev);
>>
>> This doesn't look right, for both video and metadata uvc_pm_get is called,
>> but only for video is put called.
>
> Please take a look at
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/media/usb/uvc/uvc_queue.c#n195
>
> start_streaming is not called for metadata nodes, only for video nodes.
So when you start streaming metadata and no video is streaming, then nothing
happens. I noticed this before, in fact. Only after you also start to stream
video will the metadata start to arrive. And it stops again as soon as you
stop streaming video.
That's not really how it is supposed to work: whoever starts streaming first
is the one that calls uvc_video_start_streaming. And only when nobody is streaming
should uvc_video_stop_streaming be called. That's how it works in other drivers
(e.g. those that stream both video and vbi, or even more different types of data).
Fixing this would change the behavior of uvc, and I'm not sure if this is
something we want. I leave that to Laurent and Hans.
If this isn't fixed, then at least add a comment explaining why you test for
!= V4L2_BUF_TYPE_META_CAPTURE before calling uvc_pm_put. It's not obvious.
Regards,
Hans
>
>
>
>>
>>> uvc_video_stop_streaming(uvc_queue_to_stream(queue));
>>
>> And this is odd too.
>>
>>> + }
>>
>> My assumption is that uvc_video_start_streaming and uvc_video_stop_streaming
>> are valid for both video and meta: i.e. the first time you start streaming
>> (either video or meta) you call uvc_video_start_streaming. If you were already
>> streaming for e.g. video, then start streaming metadata (or vice versa), then
>> you don't need to do anything in start_streaming.
>>
>> Same for stop_streaming: only if both video and metadata stopped streaming
>> is uvc_video_stop_streaming called.
>>
>> Please correct me if I am wrong.
>>
>> In any case, if I am right, then you have to rework this code accordingly.
>>
>> Regardless, you need to test various sequences of streaming video and metadata
>> in different orders and make sure this is handled correctly.
>
> I have tried streaming and getting frames. After some seconds the
> device turns off as expected.
>
>>
>> Regards,
>>
>> Hans
>>
>>>
>>> spin_lock_irq(&queue->irqlock);
>>> uvc_queue_return_buffers(queue, UVC_BUF_STATE_ERROR);
>>> diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
>>> index 7a5ecbefa32c0a6b74c85d7f77a25b433598471e..d4bee0d4334b764c0cf02363b573b55fb44eb228 100644
>>> --- a/drivers/media/usb/uvc/uvc_v4l2.c
>>> +++ b/drivers/media/usb/uvc/uvc_v4l2.c
>>> @@ -617,9 +617,6 @@ static int uvc_v4l2_release(struct file *file)
>>>
>>> uvc_ctrl_cleanup_fh(handle);
>>>
>>> - if (handle->is_streaming)
>>> - uvc_pm_put(stream->dev);
>>> -
>>> /* Release the file handle. */
>>> vb2_fop_release(file);
>>> file->private_data = NULL;
>>> @@ -677,50 +674,6 @@ static int uvc_ioctl_try_fmt(struct file *file, void *fh,
>>> return uvc_v4l2_try_format(stream, fmt, &probe, NULL, NULL);
>>> }
>>>
>>> -static int uvc_ioctl_streamon(struct file *file, void *fh,
>>> - enum v4l2_buf_type type)
>>> -{
>>> - struct uvc_fh *handle = fh;
>>> - struct uvc_streaming *stream = handle->stream;
>>> - int ret;
>>> -
>>> - if (handle->is_streaming)
>>> - return 0;
>>> -
>>> - ret = uvc_pm_get(stream->dev);
>>> - if (ret)
>>> - return ret;
>>> -
>>> - ret = vb2_ioctl_streamon(file, fh, type);
>>> - if (ret) {
>>> - uvc_pm_put(stream->dev);
>>> - return ret;
>>> - }
>>> -
>>> - handle->is_streaming = true;
>>> -
>>> - return 0;
>>> -}
>>> -
>>> -static int uvc_ioctl_streamoff(struct file *file, void *fh,
>>> - enum v4l2_buf_type type)
>>> -{
>>> - struct uvc_fh *handle = fh;
>>> - struct uvc_streaming *stream = handle->stream;
>>> - int ret;
>>> -
>>> - ret = vb2_ioctl_streamoff(file, fh, type);
>>> - if (ret)
>>> - return ret;
>>> -
>>> - if (handle->is_streaming) {
>>> - handle->is_streaming = false;
>>> - uvc_pm_put(stream->dev);
>>> - }
>>> -
>>> - return 0;
>>> -}
>>> -
>>> static int uvc_ioctl_enum_input(struct file *file, void *fh,
>>> struct v4l2_input *input)
>>> {
>>> @@ -1323,8 +1276,8 @@ const struct v4l2_ioctl_ops uvc_ioctl_ops = {
>>> .vidioc_expbuf = vb2_ioctl_expbuf,
>>> .vidioc_dqbuf = vb2_ioctl_dqbuf,
>>> .vidioc_create_bufs = vb2_ioctl_create_bufs,
>>> - .vidioc_streamon = uvc_ioctl_streamon,
>>> - .vidioc_streamoff = uvc_ioctl_streamoff,
>>> + .vidioc_streamon = vb2_ioctl_streamon,
>>> + .vidioc_streamoff = vb2_ioctl_streamoff,
>>> .vidioc_enum_input = uvc_ioctl_enum_input,
>>> .vidioc_g_input = uvc_ioctl_g_input,
>>> .vidioc_s_input = uvc_ioctl_s_input,
>>> diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
>>> index 3ddbf065a2cbae40ee48cb06f84ca8f0052990c4..f895f690f7cdc1af942d5f3a5f10e9dd1c956a35 100644
>>> --- a/drivers/media/usb/uvc/uvcvideo.h
>>> +++ b/drivers/media/usb/uvc/uvcvideo.h
>>> @@ -626,7 +626,6 @@ struct uvc_fh {
>>> struct uvc_video_chain *chain;
>>> struct uvc_streaming *stream;
>>> unsigned int pending_async_ctrls;
>>> - bool is_streaming;
>>> };
>>>
>>> /* ------------------------------------------------------------------------
>>>
>>
>
>
On Mon, Jun 02, 2025 at 03:47:50PM +0200, Hans Verkuil wrote:
> On 02/06/2025 15:33, Ricardo Ribalda wrote:
> > On Mon, 2 Jun 2025 at 15:23, Hans Verkuil wrote:
> >> On 02/06/2025 14:59, Ricardo Ribalda wrote:
> >>> The is_streaming field is used by modular PM to know if the device is
> >>> currently streaming or not.
> >>>
> >>> With the transition to vb2 and fop helpers, we can use vb2 functions for
> >>> the same functionality. The great benefit is that vb2 already takes
> >>> track of the streaming state for us.
> >>>
> >>> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> >>> ---
> >>> drivers/media/usb/uvc/uvc_queue.c | 11 ++++++++-
> >>> drivers/media/usb/uvc/uvc_v4l2.c | 51 ++-------------------------------------
> >>> drivers/media/usb/uvc/uvcvideo.h | 1 -
> >>> 3 files changed, 12 insertions(+), 51 deletions(-)
> >>>
> >>> diff --git a/drivers/media/usb/uvc/uvc_queue.c b/drivers/media/usb/uvc/uvc_queue.c
> >>> index 72c5494dee9f46ff61072e7d293bfaddda40e615..dff93bec204428b8aebc09332e0322fa68823fa4 100644
> >>> --- a/drivers/media/usb/uvc/uvc_queue.c
> >>> +++ b/drivers/media/usb/uvc/uvc_queue.c
> >>> @@ -165,12 +165,18 @@ static int uvc_start_streaming(struct vb2_queue *vq, unsigned int count)
> >>>
> >>> lockdep_assert_irqs_enabled();
> >>>
> >>> + ret = uvc_pm_get(stream->dev);
> >>> + if (ret)
> >>> + return ret;
> >>> +
> >>> queue->buf_used = 0;
> >>>
> >>> ret = uvc_video_start_streaming(stream);
> >>
> >> I'm not sure this is correct. See comments below.
> >>
> >>> if (ret == 0)
> >>> return 0;
> >>>
> >>> + uvc_pm_put(stream->dev);
> >>> +
> >>> spin_lock_irq(&queue->irqlock);
> >>> uvc_queue_return_buffers(queue, UVC_BUF_STATE_QUEUED);
> >>> spin_unlock_irq(&queue->irqlock);
> >>> @@ -181,11 +187,14 @@ static int uvc_start_streaming(struct vb2_queue *vq, unsigned int count)
> >>> static void uvc_stop_streaming(struct vb2_queue *vq)
> >>> {
> >>> struct uvc_video_queue *queue = vb2_get_drv_priv(vq);
> >>> + struct uvc_streaming *stream = uvc_queue_to_stream(queue);
> >>>
> >>> lockdep_assert_irqs_enabled();
> >>>
> >>> - if (vq->type != V4L2_BUF_TYPE_META_CAPTURE)
> >>> + if (vq->type != V4L2_BUF_TYPE_META_CAPTURE) {
> >>> + uvc_pm_put(stream->dev);
> >>
> >> This doesn't look right, for both video and metadata uvc_pm_get is called,
> >> but only for video is put called.
> >
> > Please take a look at
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/media/usb/uvc/uvc_queue.c#n195
> >
> > start_streaming is not called for metadata nodes, only for video nodes.
>
> So when you start streaming metadata and no video is streaming, then nothing
> happens. I noticed this before, in fact. Only after you also start to stream
> video will the metadata start to arrive. And it stops again as soon as you
> stop streaming video.
>
> That's not really how it is supposed to work: whoever starts streaming first
> is the one that calls uvc_video_start_streaming. And only when nobody is streaming
> should uvc_video_stop_streaming be called. That's how it works in other drivers
> (e.g. those that stream both video and vbi, or even more different types of data).
>
> Fixing this would change the behavior of uvc, and I'm not sure if this is
> something we want. I leave that to Laurent and Hans.
I don't see a use case for capturing metadata only, so I think we can
keep the behaviour as-is.
> If this isn't fixed, then at least add a comment explaining why you test for
> != V4L2_BUF_TYPE_META_CAPTURE before calling uvc_pm_put. It's not obvious.
Agreed.
> >>> uvc_video_stop_streaming(uvc_queue_to_stream(queue));
> >>
> >> And this is odd too.
> >>
> >>> + }
> >>
> >> My assumption is that uvc_video_start_streaming and uvc_video_stop_streaming
> >> are valid for both video and meta: i.e. the first time you start streaming
> >> (either video or meta) you call uvc_video_start_streaming. If you were already
> >> streaming for e.g. video, then start streaming metadata (or vice versa), then
> >> you don't need to do anything in start_streaming.
> >>
> >> Same for stop_streaming: only if both video and metadata stopped streaming
> >> is uvc_video_stop_streaming called.
> >>
> >> Please correct me if I am wrong.
> >>
> >> In any case, if I am right, then you have to rework this code accordingly.
> >>
> >> Regardless, you need to test various sequences of streaming video and metadata
> >> in different orders and make sure this is handled correctly.
> >
> > I have tried streaming and getting frames. After some seconds the
> > device turns off as expected.
> >
> >>> spin_lock_irq(&queue->irqlock);
> >>> uvc_queue_return_buffers(queue, UVC_BUF_STATE_ERROR);
> >>> diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
> >>> index 7a5ecbefa32c0a6b74c85d7f77a25b433598471e..d4bee0d4334b764c0cf02363b573b55fb44eb228 100644
> >>> --- a/drivers/media/usb/uvc/uvc_v4l2.c
> >>> +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> >>> @@ -617,9 +617,6 @@ static int uvc_v4l2_release(struct file *file)
> >>>
> >>> uvc_ctrl_cleanup_fh(handle);
> >>>
> >>> - if (handle->is_streaming)
> >>> - uvc_pm_put(stream->dev);
> >>> -
> >>> /* Release the file handle. */
> >>> vb2_fop_release(file);
> >>> file->private_data = NULL;
> >>> @@ -677,50 +674,6 @@ static int uvc_ioctl_try_fmt(struct file *file, void *fh,
> >>> return uvc_v4l2_try_format(stream, fmt, &probe, NULL, NULL);
> >>> }
> >>>
> >>> -static int uvc_ioctl_streamon(struct file *file, void *fh,
> >>> - enum v4l2_buf_type type)
> >>> -{
> >>> - struct uvc_fh *handle = fh;
> >>> - struct uvc_streaming *stream = handle->stream;
> >>> - int ret;
> >>> -
> >>> - if (handle->is_streaming)
> >>> - return 0;
> >>> -
> >>> - ret = uvc_pm_get(stream->dev);
> >>> - if (ret)
> >>> - return ret;
> >>> -
> >>> - ret = vb2_ioctl_streamon(file, fh, type);
> >>> - if (ret) {
> >>> - uvc_pm_put(stream->dev);
> >>> - return ret;
> >>> - }
> >>> -
> >>> - handle->is_streaming = true;
> >>> -
> >>> - return 0;
> >>> -}
> >>> -
> >>> -static int uvc_ioctl_streamoff(struct file *file, void *fh,
> >>> - enum v4l2_buf_type type)
> >>> -{
> >>> - struct uvc_fh *handle = fh;
> >>> - struct uvc_streaming *stream = handle->stream;
> >>> - int ret;
> >>> -
> >>> - ret = vb2_ioctl_streamoff(file, fh, type);
> >>> - if (ret)
> >>> - return ret;
> >>> -
> >>> - if (handle->is_streaming) {
> >>> - handle->is_streaming = false;
> >>> - uvc_pm_put(stream->dev);
> >>> - }
> >>> -
> >>> - return 0;
> >>> -}
> >>> -
> >>> static int uvc_ioctl_enum_input(struct file *file, void *fh,
> >>> struct v4l2_input *input)
> >>> {
> >>> @@ -1323,8 +1276,8 @@ const struct v4l2_ioctl_ops uvc_ioctl_ops = {
> >>> .vidioc_expbuf = vb2_ioctl_expbuf,
> >>> .vidioc_dqbuf = vb2_ioctl_dqbuf,
> >>> .vidioc_create_bufs = vb2_ioctl_create_bufs,
> >>> - .vidioc_streamon = uvc_ioctl_streamon,
> >>> - .vidioc_streamoff = uvc_ioctl_streamoff,
> >>> + .vidioc_streamon = vb2_ioctl_streamon,
> >>> + .vidioc_streamoff = vb2_ioctl_streamoff,
> >>> .vidioc_enum_input = uvc_ioctl_enum_input,
> >>> .vidioc_g_input = uvc_ioctl_g_input,
> >>> .vidioc_s_input = uvc_ioctl_s_input,
> >>> diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> >>> index 3ddbf065a2cbae40ee48cb06f84ca8f0052990c4..f895f690f7cdc1af942d5f3a5f10e9dd1c956a35 100644
> >>> --- a/drivers/media/usb/uvc/uvcvideo.h
> >>> +++ b/drivers/media/usb/uvc/uvcvideo.h
> >>> @@ -626,7 +626,6 @@ struct uvc_fh {
> >>> struct uvc_video_chain *chain;
> >>> struct uvc_streaming *stream;
> >>> unsigned int pending_async_ctrls;
> >>> - bool is_streaming;
> >>> };
> >>>
> >>> /* ------------------------------------------------------------------------
--
Regards,
Laurent Pinchart
Hi Laurent, Hi Hans
> > If this isn't fixed, then at least add a comment explaining why you test for
> > != V4L2_BUF_TYPE_META_CAPTURE before calling uvc_pm_put. It's not obvious.
>
> Agreed.
Maybe this is better than a comment?
diff --git a/drivers/media/usb/uvc/uvc_queue.c
b/drivers/media/usb/uvc/uvc_queue.c
index 72c5494dee9f..7f9d731df32c 100644
--- a/drivers/media/usb/uvc/uvc_queue.c
+++ b/drivers/media/usb/uvc/uvc_queue.c
@@ -39,8 +39,6 @@ static inline struct uvc_buffer
*uvc_vbuf_to_buffer(struct vb2_v4l2_buffer *buf)
/*
* Return all queued buffers to videobuf2 in the requested state.
- *
- * This function must be called with the queue spinlock held.
*/
static void uvc_queue_return_buffers(struct uvc_video_queue *queue,
enum uvc_buffer_state state)
@@ -49,6 +47,8 @@ static void uvc_queue_return_buffers(struct
uvc_video_queue *queue,
? VB2_BUF_STATE_ERROR
: VB2_BUF_STATE_QUEUED;
+ spin_lock_irq(&queue->irqlock);
+
while (!list_empty(&queue->irqqueue)) {
struct uvc_buffer *buf = list_first_entry(&queue->irqqueue,
struct uvc_buffer,
@@ -57,6 +57,8 @@ static void uvc_queue_return_buffers(struct
uvc_video_queue *queue,
buf->state = state;
vb2_buffer_done(&buf->buf.vb2_buf, vb2_state);
}
+
+ spin_unlock_irq(&queue->irqlock);
}
/* -----------------------------------------------------------------------------
@@ -157,7 +159,7 @@ static void uvc_buffer_finish(struct vb2_buffer *vb)
uvc_video_clock_update(stream, vbuf, buf);
}
-static int uvc_start_streaming(struct vb2_queue *vq, unsigned int count)
+static int uvc_start_streaming_video(struct vb2_queue *vq, unsigned int count)
{
struct uvc_video_queue *queue = vb2_get_drv_priv(vq);
struct uvc_streaming *stream = uvc_queue_to_stream(queue);
@@ -171,25 +173,29 @@ static int uvc_start_streaming(struct vb2_queue
*vq, unsigned int count)
if (ret == 0)
return 0;
- spin_lock_irq(&queue->irqlock);
uvc_queue_return_buffers(queue, UVC_BUF_STATE_QUEUED);
- spin_unlock_irq(&queue->irqlock);
return ret;
}
-static void uvc_stop_streaming(struct vb2_queue *vq)
+static void uvc_stop_streaming_meta(struct vb2_queue *vq)
{
struct uvc_video_queue *queue = vb2_get_drv_priv(vq);
lockdep_assert_irqs_enabled();
- if (vq->type != V4L2_BUF_TYPE_META_CAPTURE)
- uvc_video_stop_streaming(uvc_queue_to_stream(queue));
+ uvc_queue_return_buffers(queue, UVC_BUF_STATE_ERROR);
+}
+
+static void uvc_stop_streaming_video(struct vb2_queue *vq)
+{
+ struct uvc_video_queue *queue = vb2_get_drv_priv(vq);
+
+ lockdep_assert_irqs_enabled();
+
+ uvc_video_stop_streaming(uvc_queue_to_stream(queue));
- spin_lock_irq(&queue->irqlock);
uvc_queue_return_buffers(queue, UVC_BUF_STATE_ERROR);
- spin_unlock_irq(&queue->irqlock);
}
static const struct vb2_ops uvc_queue_qops = {
@@ -197,15 +203,15 @@ static const struct vb2_ops uvc_queue_qops = {
.buf_prepare = uvc_buffer_prepare,
.buf_queue = uvc_buffer_queue,
.buf_finish = uvc_buffer_finish,
- .start_streaming = uvc_start_streaming,
- .stop_streaming = uvc_stop_streaming,
+ .start_streaming = uvc_start_streaming_video,
+ .stop_streaming = uvc_stop_streaming_video,
};
static const struct vb2_ops uvc_meta_queue_qops = {
.queue_setup = uvc_queue_setup,
.buf_prepare = uvc_buffer_prepare,
.buf_queue = uvc_buffer_queue,
- .stop_streaming = uvc_stop_streaming,
+ .stop_streaming = uvc_stop_streaming_meta,
};
int uvc_queue_init(struct uvc_video_queue *queue, enum v4l2_buf_type type)
--
Ricardo Ribalda
On 02/06/2025 16:38, Ricardo Ribalda wrote:
> Hi Laurent, Hi Hans
>>> If this isn't fixed, then at least add a comment explaining why you test for
>>> != V4L2_BUF_TYPE_META_CAPTURE before calling uvc_pm_put. It's not obvious.
>>
>> Agreed.
>
> Maybe this is better than a comment?
Yes, that's better.
Regards,
Hans
>
> diff --git a/drivers/media/usb/uvc/uvc_queue.c
> b/drivers/media/usb/uvc/uvc_queue.c
> index 72c5494dee9f..7f9d731df32c 100644
> --- a/drivers/media/usb/uvc/uvc_queue.c
> +++ b/drivers/media/usb/uvc/uvc_queue.c
> @@ -39,8 +39,6 @@ static inline struct uvc_buffer
> *uvc_vbuf_to_buffer(struct vb2_v4l2_buffer *buf)
>
> /*
> * Return all queued buffers to videobuf2 in the requested state.
> - *
> - * This function must be called with the queue spinlock held.
> */
> static void uvc_queue_return_buffers(struct uvc_video_queue *queue,
> enum uvc_buffer_state state)
> @@ -49,6 +47,8 @@ static void uvc_queue_return_buffers(struct
> uvc_video_queue *queue,
> ? VB2_BUF_STATE_ERROR
> : VB2_BUF_STATE_QUEUED;
>
> + spin_lock_irq(&queue->irqlock);
> +
> while (!list_empty(&queue->irqqueue)) {
> struct uvc_buffer *buf = list_first_entry(&queue->irqqueue,
> struct uvc_buffer,
> @@ -57,6 +57,8 @@ static void uvc_queue_return_buffers(struct
> uvc_video_queue *queue,
> buf->state = state;
> vb2_buffer_done(&buf->buf.vb2_buf, vb2_state);
> }
> +
> + spin_unlock_irq(&queue->irqlock);
> }
>
> /* -----------------------------------------------------------------------------
> @@ -157,7 +159,7 @@ static void uvc_buffer_finish(struct vb2_buffer *vb)
> uvc_video_clock_update(stream, vbuf, buf);
> }
>
> -static int uvc_start_streaming(struct vb2_queue *vq, unsigned int count)
> +static int uvc_start_streaming_video(struct vb2_queue *vq, unsigned int count)
> {
> struct uvc_video_queue *queue = vb2_get_drv_priv(vq);
> struct uvc_streaming *stream = uvc_queue_to_stream(queue);
> @@ -171,25 +173,29 @@ static int uvc_start_streaming(struct vb2_queue
> *vq, unsigned int count)
> if (ret == 0)
> return 0;
>
> - spin_lock_irq(&queue->irqlock);
> uvc_queue_return_buffers(queue, UVC_BUF_STATE_QUEUED);
> - spin_unlock_irq(&queue->irqlock);
>
> return ret;
> }
>
> -static void uvc_stop_streaming(struct vb2_queue *vq)
> +static void uvc_stop_streaming_meta(struct vb2_queue *vq)
> {
> struct uvc_video_queue *queue = vb2_get_drv_priv(vq);
>
> lockdep_assert_irqs_enabled();
>
> - if (vq->type != V4L2_BUF_TYPE_META_CAPTURE)
> - uvc_video_stop_streaming(uvc_queue_to_stream(queue));
> + uvc_queue_return_buffers(queue, UVC_BUF_STATE_ERROR);
> +}
> +
> +static void uvc_stop_streaming_video(struct vb2_queue *vq)
> +{
> + struct uvc_video_queue *queue = vb2_get_drv_priv(vq);
> +
> + lockdep_assert_irqs_enabled();
> +
> + uvc_video_stop_streaming(uvc_queue_to_stream(queue));
>
> - spin_lock_irq(&queue->irqlock);
> uvc_queue_return_buffers(queue, UVC_BUF_STATE_ERROR);
> - spin_unlock_irq(&queue->irqlock);
> }
>
> static const struct vb2_ops uvc_queue_qops = {
> @@ -197,15 +203,15 @@ static const struct vb2_ops uvc_queue_qops = {
> .buf_prepare = uvc_buffer_prepare,
> .buf_queue = uvc_buffer_queue,
> .buf_finish = uvc_buffer_finish,
> - .start_streaming = uvc_start_streaming,
> - .stop_streaming = uvc_stop_streaming,
> + .start_streaming = uvc_start_streaming_video,
> + .stop_streaming = uvc_stop_streaming_video,
> };
>
> static const struct vb2_ops uvc_meta_queue_qops = {
> .queue_setup = uvc_queue_setup,
> .buf_prepare = uvc_buffer_prepare,
> .buf_queue = uvc_buffer_queue,
> - .stop_streaming = uvc_stop_streaming,
> + .stop_streaming = uvc_stop_streaming_meta,
> };
>
> int uvc_queue_init(struct uvc_video_queue *queue, enum v4l2_buf_type type)
>
>
>
© 2016 - 2026 Red Hat, Inc.