[v1] media: uvcvideo: use vb2 ioctl and fop helpers

[PATCH 3/3] media: uvcvideo: Remove stream->is_streaming field

Posted by Ricardo Ribalda 7 months ago

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.

Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/media/usb/uvc/uvc_v4l2.c | 12 +++++-------
 drivers/media/usb/uvc/uvcvideo.h |  1 -
 2 files changed, 5 insertions(+), 8 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
index 7a5ecbefa32c0a6b74c85d7f77a25b433598471e..51419f443f2c43dfd17a9782352bd2cde1094732 100644
--- a/drivers/media/usb/uvc/uvc_v4l2.c
+++ b/drivers/media/usb/uvc/uvc_v4l2.c
@@ -617,7 +617,8 @@ static int uvc_v4l2_release(struct file *file)
 
 	uvc_ctrl_cleanup_fh(handle);
 
-	if (handle->is_streaming)
+	if (stream->queue.queue.owner == file->private_data &&
+	    uvc_queue_streaming(&stream->queue))
 		uvc_pm_put(stream->dev);
 
 	/* Release the file handle. */
@@ -684,7 +685,7 @@ static int uvc_ioctl_streamon(struct file *file, void *fh,
 	struct uvc_streaming *stream = handle->stream;
 	int ret;
 
-	if (handle->is_streaming)
+	if (uvc_queue_streaming(&stream->queue))
 		return 0;
 
 	ret = uvc_pm_get(stream->dev);
@@ -697,8 +698,6 @@ static int uvc_ioctl_streamon(struct file *file, void *fh,
 		return ret;
 	}
 
-	handle->is_streaming = true;
-
 	return 0;
 }
 
@@ -707,16 +706,15 @@ static int uvc_ioctl_streamoff(struct file *file, void *fh,
 {
 	struct uvc_fh *handle = fh;
 	struct uvc_streaming *stream = handle->stream;
+	bool was_streaming = uvc_queue_streaming(&stream->queue);
 	int ret;
 
 	ret = vb2_ioctl_streamoff(file, fh, type);
 	if (ret)
 		return ret;
 
-	if (handle->is_streaming) {
-		handle->is_streaming = false;
+	if (was_streaming)
 		uvc_pm_put(stream->dev);
-	}
 
 	return 0;
 }
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.1151.ga128411c76-goog

Re: [PATCH 3/3] media: uvcvideo: Remove stream->is_streaming field

Posted by Hans Verkuil 6 months, 2 weeks ago

On 22/05/2025 19:58, 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.
> 
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
>  drivers/media/usb/uvc/uvc_v4l2.c | 12 +++++-------
>  drivers/media/usb/uvc/uvcvideo.h |  1 -
>  2 files changed, 5 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
> index 7a5ecbefa32c0a6b74c85d7f77a25b433598471e..51419f443f2c43dfd17a9782352bd2cde1094732 100644
> --- a/drivers/media/usb/uvc/uvc_v4l2.c
> +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> @@ -617,7 +617,8 @@ static int uvc_v4l2_release(struct file *file)
>  
>  	uvc_ctrl_cleanup_fh(handle);
>  
> -	if (handle->is_streaming)
> +	if (stream->queue.queue.owner == file->private_data &&

Use vb2_queue_is_busy(&stream->queue) instead of directly accessing the owner field.
But see below, since this can be dropped altogether.

> +	    uvc_queue_streaming(&stream->queue))
>  		uvc_pm_put(stream->dev);

I think patch 1/3 can be improved, which likely makes this patch obsolete.

The uvc_pm_get/put should be placed in the start/stop_streaming callbacks. That's
where you need them, and it avoids all these is_streaming tests. And it allows you to
use the vb2_ioctl_streamon/off helpers in patch 2, since the streamon/off functions
no longer mess with the uvc_pm_get/put functions.

Regards,

	Hans

>  
>  	/* Release the file handle. */
> @@ -684,7 +685,7 @@ static int uvc_ioctl_streamon(struct file *file, void *fh,
>  	struct uvc_streaming *stream = handle->stream;
>  	int ret;
>  
> -	if (handle->is_streaming)
> +	if (uvc_queue_streaming(&stream->queue))
>  		return 0;
>  
>  	ret = uvc_pm_get(stream->dev);
> @@ -697,8 +698,6 @@ static int uvc_ioctl_streamon(struct file *file, void *fh,
>  		return ret;
>  	}
>  
> -	handle->is_streaming = true;
> -
>  	return 0;
>  }
>  
> @@ -707,16 +706,15 @@ static int uvc_ioctl_streamoff(struct file *file, void *fh,
>  {
>  	struct uvc_fh *handle = fh;
>  	struct uvc_streaming *stream = handle->stream;
> +	bool was_streaming = uvc_queue_streaming(&stream->queue);
>  	int ret;
>  
>  	ret = vb2_ioctl_streamoff(file, fh, type);
>  	if (ret)
>  		return ret;
>  
> -	if (handle->is_streaming) {
> -		handle->is_streaming = false;
> +	if (was_streaming)
>  		uvc_pm_put(stream->dev);
> -	}
>  
>  	return 0;
>  }
> 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;
>  };
>  
>  /* ------------------------------------------------------------------------
>

Re: [PATCH 3/3] media: uvcvideo: Remove stream->is_streaming field

Posted by Ricardo Ribalda 6 months, 2 weeks ago

Hi Hans

On Mon, 2 Jun 2025 at 09:57, Hans Verkuil <hverkuil@xs4all.nl> wrote:
>
> On 22/05/2025 19:58, 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.
> >
> > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > ---
> >  drivers/media/usb/uvc/uvc_v4l2.c | 12 +++++-------
> >  drivers/media/usb/uvc/uvcvideo.h |  1 -
> >  2 files changed, 5 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
> > index 7a5ecbefa32c0a6b74c85d7f77a25b433598471e..51419f443f2c43dfd17a9782352bd2cde1094732 100644
> > --- a/drivers/media/usb/uvc/uvc_v4l2.c
> > +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> > @@ -617,7 +617,8 @@ static int uvc_v4l2_release(struct file *file)
> >
> >       uvc_ctrl_cleanup_fh(handle);
> >
> > -     if (handle->is_streaming)
> > +     if (stream->queue.queue.owner == file->private_data &&
>
> Use vb2_queue_is_busy(&stream->queue) instead of directly accessing the owner field.
> But see below, since this can be dropped altogether.
>
> > +         uvc_queue_streaming(&stream->queue))
> >               uvc_pm_put(stream->dev);
>
> I think patch 1/3 can be improved, which likely makes this patch obsolete.

If it is ok with you, I'd rather not touch 1/3. It has landed already
in the uvc tree.

I am reimplemeing 3/3 using  vb2's start_streaming stop_streaming.

BTW, I see that vb2 keeps track of the streaming state, so most of the
code is gone :)


>
> The uvc_pm_get/put should be placed in the start/stop_streaming callbacks. That's
> where you need them, and it avoids all these is_streaming tests. And it allows you to
> use the vb2_ioctl_streamon/off helpers in patch 2, since the streamon/off functions
> no longer mess with the uvc_pm_get/put functions.
>
> Regards,
>
>         Hans
>
> >
> >       /* Release the file handle. */
> > @@ -684,7 +685,7 @@ static int uvc_ioctl_streamon(struct file *file, void *fh,
> >       struct uvc_streaming *stream = handle->stream;
> >       int ret;
> >
> > -     if (handle->is_streaming)
> > +     if (uvc_queue_streaming(&stream->queue))
> >               return 0;
> >
> >       ret = uvc_pm_get(stream->dev);
> > @@ -697,8 +698,6 @@ static int uvc_ioctl_streamon(struct file *file, void *fh,
> >               return ret;
> >       }
> >
> > -     handle->is_streaming = true;
> > -
> >       return 0;
> >  }
> >
> > @@ -707,16 +706,15 @@ static int uvc_ioctl_streamoff(struct file *file, void *fh,
> >  {
> >       struct uvc_fh *handle = fh;
> >       struct uvc_streaming *stream = handle->stream;
> > +     bool was_streaming = uvc_queue_streaming(&stream->queue);
> >       int ret;
> >
> >       ret = vb2_ioctl_streamoff(file, fh, type);
> >       if (ret)
> >               return ret;
> >
> > -     if (handle->is_streaming) {
> > -             handle->is_streaming = false;
> > +     if (was_streaming)
> >               uvc_pm_put(stream->dev);
> > -     }
> >
> >       return 0;
> >  }
> > 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

[PATCH 1/3] media: uvcvideo: Refactor uvc_queue_streamon
[PATCH 2/3] media: uvcvideo: Use vb2 ioctl and fop helpers
[PATCH 3/3] media: uvcvideo: Remove stream->is_streaming field