| Message ID | 20250327-uvc-granpower-ng-v6-5-35a2357ff348@chromium.org |
|---|---|
| State | New |
| Headers | show |
| Series | media: uvcvideo: Implement Granular Power Saving | expand |
On 27/03/2025 22:05, Ricardo Ribalda wrote: > There are some ioctls that do not need to turn on the camera. Do not > call uvc_pm_get in those cases. > > Reviewed-by: Hans de Goede <hdegoede@redhat.com> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > --- > drivers/media/usb/uvc/uvc_v4l2.c | 20 ++++++++++++++++++++ > 1 file changed, 20 insertions(+) > > diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c > index 0f1ed0387b2611c8d21e211afe21a35101071d93..668a4e9d772c6d91f045ca75e2744b3a6c69da6b 100644 > --- a/drivers/media/usb/uvc/uvc_v4l2.c > +++ b/drivers/media/usb/uvc/uvc_v4l2.c > @@ -1440,6 +1440,26 @@ static long uvc_v4l2_unlocked_ioctl(struct file *file, > struct uvc_fh *handle = file->private_data; > int ret; > > + /* The following IOCTLs do not need to turn on the camera. */ > + switch (cmd) { > + 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: Wouldn't it be better to check against the ioctls that DO need to turn on the camera? That is more future proof IMHO. If a new ioctl is created, and uvc implements it and that needs to turn on the camera, then presumably you will realize that when you add that ioctl in uvc. If a new ioctl is created and uvc does not need to turn on the camera, then you will almost certainly forget to add it to this list. I'm not blocking this patch, but I think it will be hard to keep this list up to date. Inverting the test is probably much easier to handle in the future. Apologies if this has been discussed before, if so, just point to that discussion so I can read through it. Regards, Hans > + return video_ioctl2(file, cmd, arg); > + } > + > ret = uvc_pm_get(handle->stream->dev); > if (ret) > return ret; >
Hi Hans On Fri, 9 May 2025 at 15:44, Hans Verkuil <hans@jjverkuil.nl> wrote: > > On 27/03/2025 22:05, Ricardo Ribalda wrote: > > There are some ioctls that do not need to turn on the camera. Do not > > call uvc_pm_get in those cases. > > > > Reviewed-by: Hans de Goede <hdegoede@redhat.com> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > > --- > > drivers/media/usb/uvc/uvc_v4l2.c | 20 ++++++++++++++++++++ > > 1 file changed, 20 insertions(+) > > > > diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c > > index 0f1ed0387b2611c8d21e211afe21a35101071d93..668a4e9d772c6d91f045ca75e2744b3a6c69da6b 100644 > > --- a/drivers/media/usb/uvc/uvc_v4l2.c > > +++ b/drivers/media/usb/uvc/uvc_v4l2.c > > @@ -1440,6 +1440,26 @@ static long uvc_v4l2_unlocked_ioctl(struct file *file, > > struct uvc_fh *handle = file->private_data; > > int ret; > > > > + /* The following IOCTLs do not need to turn on the camera. */ > > + switch (cmd) { > > + 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: > > Wouldn't it be better to check against the ioctls that DO need to turn on the camera? I thought it was safer this way. I will look into inverting the logic in a follow-up patch. Regards! > > That is more future proof IMHO. > > If a new ioctl is created, and uvc implements it and that needs to turn on the camera, > then presumably you will realize that when you add that ioctl in uvc. > > If a new ioctl is created and uvc does not need to turn on the camera, then you will > almost certainly forget to add it to this list. > > I'm not blocking this patch, but I think it will be hard to keep this list up to date. > Inverting the test is probably much easier to handle in the future. > > Apologies if this has been discussed before, if so, just point to that discussion so I > can read through it. > > Regards, > > Hans > > > + return video_ioctl2(file, cmd, arg); > > + } > > + > > ret = uvc_pm_get(handle->stream->dev); > > if (ret) > > return ret; > > >
diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c index 0f1ed0387b2611c8d21e211afe21a35101071d93..668a4e9d772c6d91f045ca75e2744b3a6c69da6b 100644 --- a/drivers/media/usb/uvc/uvc_v4l2.c +++ b/drivers/media/usb/uvc/uvc_v4l2.c @@ -1440,6 +1440,26 @@ static long uvc_v4l2_unlocked_ioctl(struct file *file, struct uvc_fh *handle = file->private_data; int ret; + /* The following IOCTLs do not need to turn on the camera. */ + switch (cmd) { + 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;