[v2] media: uvcvideo: Invert granular PM logic + PM fix

[PATCH v2 7/9] media: uvcvideo: uvc_v4l2_unlocked_ioctl: Invert PM logic

Posted by Ricardo Ribalda 8 months, 1 week ago

Instead of listing the IOCTLs that do not need to turn on the camera,
list the IOCTLs that need to turn it on. This makes the code more
maintainable.

This patch changes the behaviour for unsupported IOCTLs. Those IOCTLs
will not turn on the camera.

Suggested-by: Hans Verkuil <hans@jjverkuil.nl>
Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/media/usb/uvc/uvc_v4l2.c | 61 ++++++++++++++++++++++++----------------
 1 file changed, 36 insertions(+), 25 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
index 020def11b60e00ca2875dd96f23ef9591fed11d9..13388879091c46ff74582226146521b5b5eb3d10 100644
--- a/drivers/media/usb/uvc/uvc_v4l2.c
+++ b/drivers/media/usb/uvc/uvc_v4l2.c
@@ -1219,43 +1219,54 @@ static long uvc_v4l2_compat_ioctl32(struct file *file,
 }
 #endif
 
-static long uvc_v4l2_unlocked_ioctl(struct file *file,
-				    unsigned int cmd, unsigned long arg)
+static long uvc_v4l2_pm_ioctl(struct file *file,
+			      unsigned int cmd, unsigned long arg)
 {
 	struct uvc_fh *handle = file->private_data;
 	int ret;
 
-	/* The following IOCTLs do not need to turn on the camera. */
-	switch (cmd) {
-	case UVCIOC_CTRL_MAP:
-	case VIDIOC_CREATE_BUFS:
-	case VIDIOC_DQBUF:
-	case VIDIOC_ENUM_FMT:
-	case VIDIOC_ENUM_FRAMEINTERVALS:
-	case VIDIOC_ENUM_FRAMESIZES:
-	case VIDIOC_ENUMINPUT:
-	case VIDIOC_EXPBUF:
-	case VIDIOC_G_FMT:
-	case VIDIOC_G_PARM:
-	case VIDIOC_G_SELECTION:
-	case VIDIOC_QBUF:
-	case VIDIOC_QUERYCAP:
-	case VIDIOC_REQBUFS:
-	case VIDIOC_SUBSCRIBE_EVENT:
-	case VIDIOC_UNSUBSCRIBE_EVENT:
-		return video_ioctl2(file, cmd, arg);
-	}
-
 	ret = uvc_pm_get(handle->stream->dev);
 	if (ret)
 		return ret;
-
 	ret = video_ioctl2(file, cmd, arg);
-
 	uvc_pm_put(handle->stream->dev);
+
 	return ret;
 }
 
+static long uvc_v4l2_unlocked_ioctl(struct file *file,
+				    unsigned int cmd, unsigned long arg)
+{
+	/*
+	 * For now, we do not support granular power saving for compat
+	 * syscalls.
+	 */
+	if (in_compat_syscall())
+		return uvc_v4l2_pm_ioctl(file, cmd, arg);
+
+	/* The following IOCTLs do need to turn on the camera. */
+	switch (cmd) {
+	case UVCIOC_CTRL_QUERY:
+	case VIDIOC_G_CTRL:
+	case VIDIOC_G_EXT_CTRLS:
+	case VIDIOC_G_INPUT:
+	case VIDIOC_QUERYCTRL:
+	case VIDIOC_QUERYMENU:
+	case VIDIOC_QUERY_EXT_CTRL:
+	case VIDIOC_S_CTRL:
+	case VIDIOC_S_EXT_CTRLS:
+	case VIDIOC_S_FMT:
+	case VIDIOC_S_INPUT:
+	case VIDIOC_S_PARM:
+	case VIDIOC_TRY_EXT_CTRLS:
+	case VIDIOC_TRY_FMT:
+		return uvc_v4l2_pm_ioctl(file, cmd, arg);
+	}
+
+	/* The other IOCTLs can run with the camera off. */
+	return video_ioctl2(file, cmd, arg);
+}
+
 const struct v4l2_ioctl_ops uvc_ioctl_ops = {
 	.vidioc_g_fmt_vid_cap = uvc_ioctl_g_fmt,
 	.vidioc_g_fmt_vid_out = uvc_ioctl_g_fmt,

-- 
2.49.0.1266.g31b7d2e469-goog

Re: [PATCH v2 7/9] media: uvcvideo: uvc_v4l2_unlocked_ioctl: Invert PM logic

Posted by Hans Verkuil 7 months, 2 weeks ago

Hi Ricardo,

These last three patches are a bit messy.

It is much better to combine patch 7 and 9, and move patch 7 as the first
one (but see my upcoming comment for that one, you should export a different
function).

More comments below:

On 02/06/2025 15:06, Ricardo Ribalda wrote:
> Instead of listing the IOCTLs that do not need to turn on the camera,
> list the IOCTLs that need to turn it on. This makes the code more
> maintainable.
> 
> This patch changes the behaviour for unsupported IOCTLs. Those IOCTLs
> will not turn on the camera.
> 
> Suggested-by: Hans Verkuil <hans@jjverkuil.nl>
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
>  drivers/media/usb/uvc/uvc_v4l2.c | 61 ++++++++++++++++++++++++----------------
>  1 file changed, 36 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
> index 020def11b60e00ca2875dd96f23ef9591fed11d9..13388879091c46ff74582226146521b5b5eb3d10 100644
> --- a/drivers/media/usb/uvc/uvc_v4l2.c
> +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> @@ -1219,43 +1219,54 @@ static long uvc_v4l2_compat_ioctl32(struct file *file,
>  }
>  #endif
>  
> -static long uvc_v4l2_unlocked_ioctl(struct file *file,
> -				    unsigned int cmd, unsigned long arg)
> +static long uvc_v4l2_pm_ioctl(struct file *file,
> +			      unsigned int cmd, unsigned long arg)

You don't need this function...

>  {
>  	struct uvc_fh *handle = file->private_data;
>  	int ret;
>  
> -	/* The following IOCTLs do not need to turn on the camera. */
> -	switch (cmd) {
> -	case UVCIOC_CTRL_MAP:
> -	case VIDIOC_CREATE_BUFS:
> -	case VIDIOC_DQBUF:
> -	case VIDIOC_ENUM_FMT:
> -	case VIDIOC_ENUM_FRAMEINTERVALS:
> -	case VIDIOC_ENUM_FRAMESIZES:
> -	case VIDIOC_ENUMINPUT:
> -	case VIDIOC_EXPBUF:
> -	case VIDIOC_G_FMT:
> -	case VIDIOC_G_PARM:
> -	case VIDIOC_G_SELECTION:
> -	case VIDIOC_QBUF:
> -	case VIDIOC_QUERYCAP:
> -	case VIDIOC_REQBUFS:
> -	case VIDIOC_SUBSCRIBE_EVENT:
> -	case VIDIOC_UNSUBSCRIBE_EVENT:
> -		return video_ioctl2(file, cmd, arg);
> -	}
> -
>  	ret = uvc_pm_get(handle->stream->dev);
>  	if (ret)
>  		return ret;
> -
>  	ret = video_ioctl2(file, cmd, arg);
> -
>  	uvc_pm_put(handle->stream->dev);
> +
>  	return ret;
>  }
>  
> +static long uvc_v4l2_unlocked_ioctl(struct file *file,
> +				    unsigned int cmd, unsigned long arg)
> +{
> +	/*
> +	 * For now, we do not support granular power saving for compat
> +	 * syscalls.
> +	 */
> +	if (in_compat_syscall())
> +		return uvc_v4l2_pm_ioctl(file, cmd, arg);

...you can drop this and just obtain the translated cmd (no need to
check in_compat_syscall for that)...

> +
> +	/* The following IOCTLs do need to turn on the camera. */
> +	switch (cmd) {
> +	case UVCIOC_CTRL_QUERY:
> +	case VIDIOC_G_CTRL:
> +	case VIDIOC_G_EXT_CTRLS:
> +	case VIDIOC_G_INPUT:
> +	case VIDIOC_QUERYCTRL:
> +	case VIDIOC_QUERYMENU:
> +	case VIDIOC_QUERY_EXT_CTRL:
> +	case VIDIOC_S_CTRL:
> +	case VIDIOC_S_EXT_CTRLS:
> +	case VIDIOC_S_FMT:
> +	case VIDIOC_S_INPUT:
> +	case VIDIOC_S_PARM:
> +	case VIDIOC_TRY_EXT_CTRLS:
> +	case VIDIOC_TRY_FMT:
> +		return uvc_v4l2_pm_ioctl(file, cmd, arg);

...and here you call uvc_pm_get/video_ioctl2/uvc_pm_put. It keeps everything nicely
localized, no need to look up what happens in a different function.

> +	}
> +
> +	/* The other IOCTLs can run with the camera off. */
> +	return video_ioctl2(file, cmd, arg);
> +}
> +
>  const struct v4l2_ioctl_ops uvc_ioctl_ops = {
>  	.vidioc_g_fmt_vid_cap = uvc_ioctl_g_fmt,
>  	.vidioc_g_fmt_vid_out = uvc_ioctl_g_fmt,
> 

Regards,

	Hans

Re: [PATCH v2 7/9] media: uvcvideo: uvc_v4l2_unlocked_ioctl: Invert PM logic

Posted by Hans de Goede 7 months, 4 weeks ago

Hi,

On 2-Jun-25 15:06, Ricardo Ribalda wrote:
> Instead of listing the IOCTLs that do not need to turn on the camera,
> list the IOCTLs that need to turn it on. This makes the code more
> maintainable.
> 
> This patch changes the behaviour for unsupported IOCTLs. Those IOCTLs
> will not turn on the camera.
> 
> Suggested-by: Hans Verkuil <hans@jjverkuil.nl>
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
>  drivers/media/usb/uvc/uvc_v4l2.c | 61 ++++++++++++++++++++++++----------------
>  1 file changed, 36 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
> index 020def11b60e00ca2875dd96f23ef9591fed11d9..13388879091c46ff74582226146521b5b5eb3d10 100644
> --- a/drivers/media/usb/uvc/uvc_v4l2.c
> +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> @@ -1219,43 +1219,54 @@ static long uvc_v4l2_compat_ioctl32(struct file *file,
>  }
>  #endif
>  
> -static long uvc_v4l2_unlocked_ioctl(struct file *file,
> -				    unsigned int cmd, unsigned long arg)
> +static long uvc_v4l2_pm_ioctl(struct file *file,
> +			      unsigned int cmd, unsigned long arg)
>  {
>  	struct uvc_fh *handle = file->private_data;
>  	int ret;
>  
> -	/* The following IOCTLs do not need to turn on the camera. */

s/do need/need/

otherwise this looks good to me:

Reviewed-by: Hans de Goede <hansg@kernel.org>

Regards,

Hans


> -	switch (cmd) {
> -	case UVCIOC_CTRL_MAP:
> -	case VIDIOC_CREATE_BUFS:
> -	case VIDIOC_DQBUF:
> -	case VIDIOC_ENUM_FMT:
> -	case VIDIOC_ENUM_FRAMEINTERVALS:
> -	case VIDIOC_ENUM_FRAMESIZES:
> -	case VIDIOC_ENUMINPUT:
> -	case VIDIOC_EXPBUF:
> -	case VIDIOC_G_FMT:
> -	case VIDIOC_G_PARM:
> -	case VIDIOC_G_SELECTION:
> -	case VIDIOC_QBUF:
> -	case VIDIOC_QUERYCAP:
> -	case VIDIOC_REQBUFS:
> -	case VIDIOC_SUBSCRIBE_EVENT:
> -	case VIDIOC_UNSUBSCRIBE_EVENT:
> -		return video_ioctl2(file, cmd, arg);
> -	}
> -
>  	ret = uvc_pm_get(handle->stream->dev);
>  	if (ret)
>  		return ret;
> -
>  	ret = video_ioctl2(file, cmd, arg);
> -
>  	uvc_pm_put(handle->stream->dev);
> +
>  	return ret;
>  }
>  
> +static long uvc_v4l2_unlocked_ioctl(struct file *file,
> +				    unsigned int cmd, unsigned long arg)
> +{
> +	/*
> +	 * For now, we do not support granular power saving for compat
> +	 * syscalls.
> +	 */
> +	if (in_compat_syscall())
> +		return uvc_v4l2_pm_ioctl(file, cmd, arg);
> +
> +	/* The following IOCTLs do need to turn on the camera. */
> +	switch (cmd) {
> +	case UVCIOC_CTRL_QUERY:
> +	case VIDIOC_G_CTRL:
> +	case VIDIOC_G_EXT_CTRLS:
> +	case VIDIOC_G_INPUT:
> +	case VIDIOC_QUERYCTRL:
> +	case VIDIOC_QUERYMENU:
> +	case VIDIOC_QUERY_EXT_CTRL:
> +	case VIDIOC_S_CTRL:
> +	case VIDIOC_S_EXT_CTRLS:
> +	case VIDIOC_S_FMT:
> +	case VIDIOC_S_INPUT:
> +	case VIDIOC_S_PARM:
> +	case VIDIOC_TRY_EXT_CTRLS:
> +	case VIDIOC_TRY_FMT:
> +		return uvc_v4l2_pm_ioctl(file, cmd, arg);
> +	}
> +
> +	/* The other IOCTLs can run with the camera off. */
> +	return video_ioctl2(file, cmd, arg);
> +}
> +
>  const struct v4l2_ioctl_ops uvc_ioctl_ops = {
>  	.vidioc_g_fmt_vid_cap = uvc_ioctl_g_fmt,
>  	.vidioc_g_fmt_vid_out = uvc_ioctl_g_fmt,
>

Re: [PATCH v2 7/9] media: uvcvideo: uvc_v4l2_unlocked_ioctl: Invert PM logic

Posted by Hans de Goede 7 months, 2 weeks ago

Hi,

On 16-Jun-25 4:14 PM, Hans de Goede wrote:
> Hi,
> 
> On 2-Jun-25 15:06, Ricardo Ribalda wrote:
>> Instead of listing the IOCTLs that do not need to turn on the camera,
>> list the IOCTLs that need to turn it on. This makes the code more
>> maintainable.
>>
>> This patch changes the behaviour for unsupported IOCTLs. Those IOCTLs
>> will not turn on the camera.
>>
>> Suggested-by: Hans Verkuil <hans@jjverkuil.nl>
>> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
>> ---
>>  drivers/media/usb/uvc/uvc_v4l2.c | 61 ++++++++++++++++++++++++----------------
>>  1 file changed, 36 insertions(+), 25 deletions(-)
>>
>> diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
>> index 020def11b60e00ca2875dd96f23ef9591fed11d9..13388879091c46ff74582226146521b5b5eb3d10 100644
>> --- a/drivers/media/usb/uvc/uvc_v4l2.c
>> +++ b/drivers/media/usb/uvc/uvc_v4l2.c
>> @@ -1219,43 +1219,54 @@ static long uvc_v4l2_compat_ioctl32(struct file *file,
>>  }
>>  #endif
>>  
>> -static long uvc_v4l2_unlocked_ioctl(struct file *file,
>> -				    unsigned int cmd, unsigned long arg)
>> +static long uvc_v4l2_pm_ioctl(struct file *file,
>> +			      unsigned int cmd, unsigned long arg)
>>  {
>>  	struct uvc_fh *handle = file->private_data;
>>  	int ret;
>>  
>> -	/* The following IOCTLs do not need to turn on the camera. */
> 
> s/do need/need/

Actually this review-remark belongs to when this comment is re-added
below ...

>> -	switch (cmd) {
>> -	case UVCIOC_CTRL_MAP:
>> -	case VIDIOC_CREATE_BUFS:
>> -	case VIDIOC_DQBUF:
>> -	case VIDIOC_ENUM_FMT:
>> -	case VIDIOC_ENUM_FRAMEINTERVALS:
>> -	case VIDIOC_ENUM_FRAMESIZES:
>> -	case VIDIOC_ENUMINPUT:
>> -	case VIDIOC_EXPBUF:
>> -	case VIDIOC_G_FMT:
>> -	case VIDIOC_G_PARM:
>> -	case VIDIOC_G_SELECTION:
>> -	case VIDIOC_QBUF:
>> -	case VIDIOC_QUERYCAP:
>> -	case VIDIOC_REQBUFS:
>> -	case VIDIOC_SUBSCRIBE_EVENT:
>> -	case VIDIOC_UNSUBSCRIBE_EVENT:
>> -		return video_ioctl2(file, cmd, arg);
>> -	}
>> -
>>  	ret = uvc_pm_get(handle->stream->dev);
>>  	if (ret)
>>  		return ret;
>> -
>>  	ret = video_ioctl2(file, cmd, arg);
>> -
>>  	uvc_pm_put(handle->stream->dev);
>> +
>>  	return ret;
>>  }
>>  
>> +static long uvc_v4l2_unlocked_ioctl(struct file *file,
>> +				    unsigned int cmd, unsigned long arg)
>> +{
>> +	/*
>> +	 * For now, we do not support granular power saving for compat
>> +	 * syscalls.
>> +	 */
>> +	if (in_compat_syscall())
>> +		return uvc_v4l2_pm_ioctl(file, cmd, arg);
>> +
>> +	/* The following IOCTLs do need to turn on the camera. */

So the 's/do need/need/' should be done here. I can fix this
up while merging. All that is necessary to merge this is an
ack from Hans V for merging the EXPORT in the core through
the UVC git tree.

Regards,

Hans



>> +	switch (cmd) {
>> +	case UVCIOC_CTRL_QUERY:
>> +	case VIDIOC_G_CTRL:
>> +	case VIDIOC_G_EXT_CTRLS:
>> +	case VIDIOC_G_INPUT:
>> +	case VIDIOC_QUERYCTRL:
>> +	case VIDIOC_QUERYMENU:
>> +	case VIDIOC_QUERY_EXT_CTRL:
>> +	case VIDIOC_S_CTRL:
>> +	case VIDIOC_S_EXT_CTRLS:
>> +	case VIDIOC_S_FMT:
>> +	case VIDIOC_S_INPUT:
>> +	case VIDIOC_S_PARM:
>> +	case VIDIOC_TRY_EXT_CTRLS:
>> +	case VIDIOC_TRY_FMT:
>> +		return uvc_v4l2_pm_ioctl(file, cmd, arg);
>> +	}
>> +
>> +	/* The other IOCTLs can run with the camera off. */
>> +	return video_ioctl2(file, cmd, arg);
>> +}
>> +
>>  const struct v4l2_ioctl_ops uvc_ioctl_ops = {
>>  	.vidioc_g_fmt_vid_cap = uvc_ioctl_g_fmt,
>>  	.vidioc_g_fmt_vid_out = uvc_ioctl_g_fmt,
>>
>