| Message ID | 20241108-uvc-subdev-v2-0-85d8a051a3d3@chromium.org |
|---|---|
| Headers | show |
| Series | media: uvcvideo: Implement the Privacy GPIO as a subdevice | expand |
Em Fri, 08 Nov 2024 20:25:44 +0000 Ricardo Ribalda <ribalda@chromium.org> escreveu: > Some notebooks have a button to disable the camera (not to be mistaken > with the mechanical cover). This is a standard GPIO linked to the > camera via the ACPI table. > > 4 years ago we added support for this button in UVC via the Privacy control. > This has two issues: > - If the camera has its own privacy control, it will be masked > - We need to power-up the camera to read the privacy control gpio. > > We tried to fix the power-up issues implementing "granular power > saving" but it has been more complicated than anticipated.... > > Last year, we proposed a patchset to implement the privacy gpio as a > subdevice https://lore.kernel.org/linux-media/20230111-uvc_privacy_subdev-v1-0-f859ac9a01e3@chromium.org/ > > I think it is a pretty clean solution and makes sense to use a > subdevice for something that is a sub device of the camera :). > > This is an attempt to continue with that approach. > > Tested on gimble: > gimble-rev3 ~ # v4l2-ctl --all -d /dev/v4l-subdev0 No matter if internally implemented as a subdevice or not, UVC is not a MC-centric device[1]. It means that UVC can be compiled without media controller support, and that its functionality shall be visible via /dev/video* nodes. So, whatever internal implementation it is used, it shall not require config MEDIA_CONTROLLER and the control shall be visible via /dev/video*. Moving privacy control out of /dev/video would mean that this will break support for it on existing applications, which is a big nack. Now, it would be acceptable to have this visible via V4L2 and subdev APIs. [1] https://www.kernel.org/doc/html/latest/userspace-api/media/glossary.html#term-MC-centric Regards, Mauro > Driver Info: > Driver version : 6.6.56 > Capabilities : 0x00000000 > Media Driver Info: > Driver name : uvcvideo > Model : HP 5M Camera: HP 5M Camera > Serial : 0001 > Bus info : usb-0000:00:14.0-6 > Media version : 6.6.56 > Hardware revision: 0x00009601 (38401) > Driver version : 6.6.56 > Interface Info: > ID : 0x0300001d > Type : V4L Sub-Device > Entity Info: > ID : 0x00000013 (19) > Name : GPIO > Function : Unknown sub-device (00020006) > > Camera Controls > > privacy 0x009a0910 (bool) : default=0 value=0 flags=read-only, volatile > > gimble-rev3 ~ # media-ctl -p > Media controller API version 6.6.56 > > Media device information > ------------------------ > driver uvcvideo > model HP 5M Camera: HP 5M Camera > serial 0001 > bus info usb-0000:00:14.0-6 > hw revision 0x9601 > driver version 6.6.56 > > Device topology > - entity 1: HP 5M Camera: HP 5M Camera (1 pad, 1 link) > type Node subtype V4L flags 1 > device node name /dev/video0 > pad0: Sink > <- "Extension 8":1 [ENABLED,IMMUTABLE] > > - entity 4: HP 5M Camera: HP 5M Camera (0 pad, 0 link) > type Node subtype V4L flags 0 > device node name /dev/video1 > > - entity 8: Extension 8 (2 pads, 2 links, 0 routes) > type V4L2 subdev subtype Unknown flags 0 > pad0: Sink > <- "Extension 4":1 [ENABLED,IMMUTABLE] > pad1: Source > -> "HP 5M Camera: HP 5M Camera":0 [ENABLED,IMMUTABLE] > > - entity 11: Extension 4 (2 pads, 2 links, 0 routes) > type V4L2 subdev subtype Unknown flags 0 > pad0: Sink > <- "Processing 2":1 [ENABLED,IMMUTABLE] > pad1: Source > -> "Extension 8":0 [ENABLED,IMMUTABLE] > > - entity 14: Processing 2 (2 pads, 2 links, 0 routes) > type V4L2 subdev subtype Unknown flags 0 > pad0: Sink > <- "Camera 1":0 [ENABLED,IMMUTABLE] > pad1: Source > -> "Extension 4":0 [ENABLED,IMMUTABLE] > > - entity 17: Camera 1 (1 pad, 1 link, 0 routes) > type V4L2 subdev subtype Sensor flags 0 > pad0: Source > -> "Processing 2":0 [ENABLED,IMMUTABLE] > > - entity 19: GPIO (0 pad, 0 link, 0 routes) > type V4L2 subdev subtype Decoder flags 0 > device node name /dev/v4l-subdev0 > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > --- > Changes in v2: > - Rebase on top of https://patchwork.linuxtv.org/project/linux-media/patch/20241106-uvc-crashrmmod-v6-1-fbf9781c6e83@chromium.org/ > - Create uvc_gpio_cleanup and uvc_gpio_deinit > - Refactor quirk: do not disable irq > - Change define number for MEDIA_ENT_F_GPIO > - Link to v1: https://lore.kernel.org/r/20241031-uvc-subdev-v1-0-a68331cedd72@chromium.org > > --- > Ricardo Ribalda (5): > media: uvcvideo: Factor out gpio functions to its own file > Revert "media: uvcvideo: Allow entity-defined get_info and get_cur" > media: uvcvideo: Create ancillary link for GPIO subdevice > media: v4l2-core: Add new MEDIA_ENT_F_GPIO > media: uvcvideo: Use MEDIA_ENT_F_GPIO for the GPIO entity > > Yunke Cao (1): > media: uvcvideo: Re-implement privacy GPIO as a separate subdevice > > .../userspace-api/media/mediactl/media-types.rst | 4 + > drivers/media/usb/uvc/Makefile | 3 +- > drivers/media/usb/uvc/uvc_ctrl.c | 40 +---- > drivers/media/usb/uvc/uvc_driver.c | 123 +------------- > drivers/media/usb/uvc/uvc_entity.c | 20 ++- > drivers/media/usb/uvc/uvc_gpio.c | 187 +++++++++++++++++++++ > drivers/media/usb/uvc/uvc_video.c | 4 + > drivers/media/usb/uvc/uvcvideo.h | 34 ++-- > drivers/media/v4l2-core/v4l2-async.c | 3 +- > include/uapi/linux/media.h | 1 + > 10 files changed, 252 insertions(+), 167 deletions(-) > --- > base-commit: 4353256f5487e0c5c47e8ff764bf4f9e679fb525 > change-id: 20241030-uvc-subdev-89f4467a00b5 > > Best regards, Thanks, Mauro
Hi Mauro On Sat, 9 Nov 2024 at 15:05, Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote: > > Em Fri, 08 Nov 2024 20:25:44 +0000 > Ricardo Ribalda <ribalda@chromium.org> escreveu: > > > Some notebooks have a button to disable the camera (not to be mistaken > > with the mechanical cover). This is a standard GPIO linked to the > > camera via the ACPI table. > > > > 4 years ago we added support for this button in UVC via the Privacy control. > > This has two issues: > > - If the camera has its own privacy control, it will be masked > > - We need to power-up the camera to read the privacy control gpio. > > > > We tried to fix the power-up issues implementing "granular power > > saving" but it has been more complicated than anticipated.... > > > > Last year, we proposed a patchset to implement the privacy gpio as a > > subdevice https://lore.kernel.org/linux-media/20230111-uvc_privacy_subdev-v1-0-f859ac9a01e3@chromium.org/ > > > > I think it is a pretty clean solution and makes sense to use a > > subdevice for something that is a sub device of the camera :). > > > > This is an attempt to continue with that approach. > > > > Tested on gimble: > > gimble-rev3 ~ # v4l2-ctl --all -d /dev/v4l-subdev0 > > No matter if internally implemented as a subdevice or not, > UVC is not a MC-centric device[1]. > > It means that UVC can be compiled without media controller support, > and that its functionality shall be visible via /dev/video* nodes. > > So, whatever internal implementation it is used, it shall not require > config MEDIA_CONTROLLER and the control shall be visible via > /dev/video*. > > Moving privacy control out of /dev/video would mean that this will break > support for it on existing applications, which is a big nack. Now, it would > be acceptable to have this visible via V4L2 and subdev APIs. I have googled a bit, and it seems that the only users of this feature is ChromeOS, so I do not expect any existing applications to be impacted. I can keep the old API, but that does not solve the issue when the camera supports the privacy control and it is also attached to a GPIO. I do not see a big requirement to depend on the MEDIA_CONTROLLER to use the privacy GPIO. Remember that this feature is not part of the camera itself, it is an external GPIO. What about trying the subdevice approach, and if we break any app, implement both APIs (legacy and subdevice)? Please note that If a device has an internal Privacy control it will still work via /dev/videoX. > > [1] https://www.kernel.org/doc/html/latest/userspace-api/media/glossary.html#term-MC-centric > > Regards, > Mauro > > > Driver Info: > > Driver version : 6.6.56 > > Capabilities : 0x00000000 > > Media Driver Info: > > Driver name : uvcvideo > > Model : HP 5M Camera: HP 5M Camera > > Serial : 0001 > > Bus info : usb-0000:00:14.0-6 > > Media version : 6.6.56 > > Hardware revision: 0x00009601 (38401) > > Driver version : 6.6.56 > > Interface Info: > > ID : 0x0300001d > > Type : V4L Sub-Device > > Entity Info: > > ID : 0x00000013 (19) > > Name : GPIO > > Function : Unknown sub-device (00020006) > > > > Camera Controls > > > > privacy 0x009a0910 (bool) : default=0 value=0 flags=read-only, volatile > > > > gimble-rev3 ~ # media-ctl -p > > Media controller API version 6.6.56 > > > > Media device information > > ------------------------ > > driver uvcvideo > > model HP 5M Camera: HP 5M Camera > > serial 0001 > > bus info usb-0000:00:14.0-6 > > hw revision 0x9601 > > driver version 6.6.56 > > > > Device topology > > - entity 1: HP 5M Camera: HP 5M Camera (1 pad, 1 link) > > type Node subtype V4L flags 1 > > device node name /dev/video0 > > pad0: Sink > > <- "Extension 8":1 [ENABLED,IMMUTABLE] > > > > - entity 4: HP 5M Camera: HP 5M Camera (0 pad, 0 link) > > type Node subtype V4L flags 0 > > device node name /dev/video1 > > > > - entity 8: Extension 8 (2 pads, 2 links, 0 routes) > > type V4L2 subdev subtype Unknown flags 0 > > pad0: Sink > > <- "Extension 4":1 [ENABLED,IMMUTABLE] > > pad1: Source > > -> "HP 5M Camera: HP 5M Camera":0 [ENABLED,IMMUTABLE] > > > > - entity 11: Extension 4 (2 pads, 2 links, 0 routes) > > type V4L2 subdev subtype Unknown flags 0 > > pad0: Sink > > <- "Processing 2":1 [ENABLED,IMMUTABLE] > > pad1: Source > > -> "Extension 8":0 [ENABLED,IMMUTABLE] > > > > - entity 14: Processing 2 (2 pads, 2 links, 0 routes) > > type V4L2 subdev subtype Unknown flags 0 > > pad0: Sink > > <- "Camera 1":0 [ENABLED,IMMUTABLE] > > pad1: Source > > -> "Extension 4":0 [ENABLED,IMMUTABLE] > > > > - entity 17: Camera 1 (1 pad, 1 link, 0 routes) > > type V4L2 subdev subtype Sensor flags 0 > > pad0: Source > > -> "Processing 2":0 [ENABLED,IMMUTABLE] > > > > - entity 19: GPIO (0 pad, 0 link, 0 routes) > > type V4L2 subdev subtype Decoder flags 0 > > device node name /dev/v4l-subdev0 > > > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > > --- > > Changes in v2: > > - Rebase on top of https://patchwork.linuxtv.org/project/linux-media/patch/20241106-uvc-crashrmmod-v6-1-fbf9781c6e83@chromium.org/ > > - Create uvc_gpio_cleanup and uvc_gpio_deinit > > - Refactor quirk: do not disable irq > > - Change define number for MEDIA_ENT_F_GPIO > > - Link to v1: https://lore.kernel.org/r/20241031-uvc-subdev-v1-0-a68331cedd72@chromium.org > > > > --- > > Ricardo Ribalda (5): > > media: uvcvideo: Factor out gpio functions to its own file > > Revert "media: uvcvideo: Allow entity-defined get_info and get_cur" > > media: uvcvideo: Create ancillary link for GPIO subdevice > > media: v4l2-core: Add new MEDIA_ENT_F_GPIO > > media: uvcvideo: Use MEDIA_ENT_F_GPIO for the GPIO entity > > > > Yunke Cao (1): > > media: uvcvideo: Re-implement privacy GPIO as a separate subdevice > > > > .../userspace-api/media/mediactl/media-types.rst | 4 + > > drivers/media/usb/uvc/Makefile | 3 +- > > drivers/media/usb/uvc/uvc_ctrl.c | 40 +---- > > drivers/media/usb/uvc/uvc_driver.c | 123 +------------- > > drivers/media/usb/uvc/uvc_entity.c | 20 ++- > > drivers/media/usb/uvc/uvc_gpio.c | 187 +++++++++++++++++++++ > > drivers/media/usb/uvc/uvc_video.c | 4 + > > drivers/media/usb/uvc/uvcvideo.h | 34 ++-- > > drivers/media/v4l2-core/v4l2-async.c | 3 +- > > include/uapi/linux/media.h | 1 + > > 10 files changed, 252 insertions(+), 167 deletions(-) > > --- > > base-commit: 4353256f5487e0c5c47e8ff764bf4f9e679fb525 > > change-id: 20241030-uvc-subdev-89f4467a00b5 > > > > Best regards, > > > > Thanks, > Mauro
Hi Ricardo, FYI / some background: I have been asked to start helping / co-maintaining UVC with Laurent. I'll send out a patch adding myself as UVC maintainer soon. On 8-Nov-24 9:25 PM, Ricardo Ribalda wrote: > Some notebooks have a button to disable the camera (not to be mistaken > with the mechanical cover). This is a standard GPIO linked to the > camera via the ACPI table. > > 4 years ago we added support for this button in UVC via the Privacy control. > This has two issues: > - If the camera has its own privacy control, it will be masked > - We need to power-up the camera to read the privacy control gpio. > > We tried to fix the power-up issues implementing "granular power > saving" but it has been more complicated than anticipated.... I have been discussing UVC power-management with Laurent, also related to power-consumption issues caused by libcamera's pipeline handler holding open the /dev/video# node as long as the camera manager object exists. For now we have fixed this with some relatively small changes to libcamera's uvcvideo pipeline handler, but that is really meant as an interim solution and as this privacy control series shows the power-management issues are real. Combined with Mauro's remarks about how this is an userspace ABI break (1) I think we should maybe first take another look at the powermanagement issues in general rather then moving forward with this series. My apologies for this, I realize how annoying it can be when you are working on a patch series to fix a specific issue and a reviewer moves the goal-posts like this. But I do really think that just fixing the generic power-management issues would be better and I also think that this should be feasible / not too hard. Here is what I have in mind for this: 1. Assume that the results of trying a specific fmt do not change over time. 2. Only allow userspace to request fmts which match one of the enum-fmts -> enum-frame-sizes -> enum-frame-rates tripplet results (constrain what userspace requests to these) 3. Run the equivalent of tryfmt on all possible combinations (so the usaul 3 levels nested loop for this) on probe() and cache the results 4. Make try_fmt / set_fmt not poweron the device but instead constrain the requested fmt to one from our cached fmts 5. On stream-on do the actual power-on + set-fmt + verify that we get what we expect based on the cache, and otherwise return -EIO. I think that should sort the issue, assuming that 1. above holds true. One downside is that this stops UVC button presses from working when not streaming. But userspace will typically only open the /dev/video# node if it plans to stream anyways so there should not be much of a difference wrt button press behavior. This should also make camera enumeration faster for apps, since most apps / frameworks do the whole 3 levels nested loop for this on startup, for which atm we go out to the hw, which now instead will come from the fmts cache and thus will be much much faster, so this should lead to a noticeable speedup for apps accessing UVC cameras which would be another nice win. Downside is that the initial probe will take longer see we do all the tryfmt-s there now. But I think that taking a bit longer to probe while the machine is booting should not be an issue. Regards, Hans 1) Which is technically correct, but FWIW I agree with you that I think most userspace consumers will not care > Last year, we proposed a patchset to implement the privacy gpio as a > subdevice https://lore.kernel.org/linux-media/20230111-uvc_privacy_subdev-v1-0-f859ac9a01e3@chromium.org/ > > I think it is a pretty clean solution and makes sense to use a > subdevice for something that is a sub device of the camera :). > > This is an attempt to continue with that approach. > > Tested on gimble: > gimble-rev3 ~ # v4l2-ctl --all -d /dev/v4l-subdev0 > Driver Info: > Driver version : 6.6.56 > Capabilities : 0x00000000 > Media Driver Info: > Driver name : uvcvideo > Model : HP 5M Camera: HP 5M Camera > Serial : 0001 > Bus info : usb-0000:00:14.0-6 > Media version : 6.6.56 > Hardware revision: 0x00009601 (38401) > Driver version : 6.6.56 > Interface Info: > ID : 0x0300001d > Type : V4L Sub-Device > Entity Info: > ID : 0x00000013 (19) > Name : GPIO > Function : Unknown sub-device (00020006) > > Camera Controls > > privacy 0x009a0910 (bool) : default=0 value=0 flags=read-only, volatile > > gimble-rev3 ~ # media-ctl -p > Media controller API version 6.6.56 > > Media device information > ------------------------ > driver uvcvideo > model HP 5M Camera: HP 5M Camera > serial 0001 > bus info usb-0000:00:14.0-6 > hw revision 0x9601 > driver version 6.6.56 > > Device topology > - entity 1: HP 5M Camera: HP 5M Camera (1 pad, 1 link) > type Node subtype V4L flags 1 > device node name /dev/video0 > pad0: Sink > <- "Extension 8":1 [ENABLED,IMMUTABLE] > > - entity 4: HP 5M Camera: HP 5M Camera (0 pad, 0 link) > type Node subtype V4L flags 0 > device node name /dev/video1 > > - entity 8: Extension 8 (2 pads, 2 links, 0 routes) > type V4L2 subdev subtype Unknown flags 0 > pad0: Sink > <- "Extension 4":1 [ENABLED,IMMUTABLE] > pad1: Source > -> "HP 5M Camera: HP 5M Camera":0 [ENABLED,IMMUTABLE] > > - entity 11: Extension 4 (2 pads, 2 links, 0 routes) > type V4L2 subdev subtype Unknown flags 0 > pad0: Sink > <- "Processing 2":1 [ENABLED,IMMUTABLE] > pad1: Source > -> "Extension 8":0 [ENABLED,IMMUTABLE] > > - entity 14: Processing 2 (2 pads, 2 links, 0 routes) > type V4L2 subdev subtype Unknown flags 0 > pad0: Sink > <- "Camera 1":0 [ENABLED,IMMUTABLE] > pad1: Source > -> "Extension 4":0 [ENABLED,IMMUTABLE] > > - entity 17: Camera 1 (1 pad, 1 link, 0 routes) > type V4L2 subdev subtype Sensor flags 0 > pad0: Source > -> "Processing 2":0 [ENABLED,IMMUTABLE] > > - entity 19: GPIO (0 pad, 0 link, 0 routes) > type V4L2 subdev subtype Decoder flags 0 > device node name /dev/v4l-subdev0 > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > --- > Changes in v2: > - Rebase on top of https://patchwork.linuxtv.org/project/linux-media/patch/20241106-uvc-crashrmmod-v6-1-fbf9781c6e83@chromium.org/ > - Create uvc_gpio_cleanup and uvc_gpio_deinit > - Refactor quirk: do not disable irq > - Change define number for MEDIA_ENT_F_GPIO > - Link to v1: https://lore.kernel.org/r/20241031-uvc-subdev-v1-0-a68331cedd72@chromium.org > > --- > Ricardo Ribalda (5): > media: uvcvideo: Factor out gpio functions to its own file > Revert "media: uvcvideo: Allow entity-defined get_info and get_cur" > media: uvcvideo: Create ancillary link for GPIO subdevice > media: v4l2-core: Add new MEDIA_ENT_F_GPIO > media: uvcvideo: Use MEDIA_ENT_F_GPIO for the GPIO entity > > Yunke Cao (1): > media: uvcvideo: Re-implement privacy GPIO as a separate subdevice > > .../userspace-api/media/mediactl/media-types.rst | 4 + > drivers/media/usb/uvc/Makefile | 3 +- > drivers/media/usb/uvc/uvc_ctrl.c | 40 +---- > drivers/media/usb/uvc/uvc_driver.c | 123 +------------- > drivers/media/usb/uvc/uvc_entity.c | 20 ++- > drivers/media/usb/uvc/uvc_gpio.c | 187 +++++++++++++++++++++ > drivers/media/usb/uvc/uvc_video.c | 4 + > drivers/media/usb/uvc/uvcvideo.h | 34 ++-- > drivers/media/v4l2-core/v4l2-async.c | 3 +- > include/uapi/linux/media.h | 1 + > 10 files changed, 252 insertions(+), 167 deletions(-) > --- > base-commit: 4353256f5487e0c5c47e8ff764bf4f9e679fb525 > change-id: 20241030-uvc-subdev-89f4467a00b5 > > Best regards,
Hi Hans On Sat, 9 Nov 2024 at 16:37, Hans de Goede <hdegoede@redhat.com> wrote: > > Hi Ricardo, > > FYI / some background: I have been asked to start helping / > co-maintaining UVC with Laurent. I'll send out a patch adding > myself as UVC maintainer soon. Great! I talked with Laurent yesterday, I hope that we can maintain the driver the three of us in the near future. > > On 8-Nov-24 9:25 PM, Ricardo Ribalda wrote: > > Some notebooks have a button to disable the camera (not to be mistaken > > with the mechanical cover). This is a standard GPIO linked to the > > camera via the ACPI table. > > > > 4 years ago we added support for this button in UVC via the Privacy control. > > This has two issues: > > - If the camera has its own privacy control, it will be masked > > - We need to power-up the camera to read the privacy control gpio. > > > > We tried to fix the power-up issues implementing "granular power > > saving" but it has been more complicated than anticipated.... > > I have been discussing UVC power-management with Laurent, also > related to power-consumption issues caused by libcamera's pipeline > handler holding open the /dev/video# node as long as the camera > manager object exists. > > For now we have fixed this with some relatively small changes to > libcamera's uvcvideo pipeline handler, but that is really meant > as an interim solution and as this privacy control series shows > the power-management issues are real. Indeed, we have tried to fixed it before: https://lore.kernel.org/linux-media/20220920-resend-powersave-v5-2-692e6df6c1e2@chromium.org/ btw this recently landed patch was to work in this direction :) https://lore.kernel.org/linux-media/20240926-guenter-mini-v7-1-690441953d4a@chromium.org/ The more people interested in this problem the better. > > Combined with Mauro's remarks about how this is an userspace ABI break (1) > I think we should maybe first take another look at the powermanagement > issues in general rather then moving forward with this series. > > My apologies for this, I realize how annoying it can be when you are > working on a patch series to fix a specific issue and a reviewer > moves the goal-posts like this. But I do really think that just fixing > the generic power-management issues would be better and I also think > that this should be feasible / not too hard. > > Here is what I have in mind for this: > > 1. Assume that the results of trying a specific fmt do not change over time. > > 2. Only allow userspace to request fmts which match one of the enum-fmts -> > enum-frame-sizes -> enum-frame-rates tripplet results > (constrain what userspace requests to these) > > 3. Run the equivalent of tryfmt on all possible combinations (so the usaul > 3 levels nested loop for this) on probe() and cache the results > > 4. Make try_fmt / set_fmt not poweron the device but instead constrain > the requested fmt to one from our cached fmts > > 5. On stream-on do the actual power-on + set-fmt + verify that we get > what we expect based on the cache, and otherwise return -EIO. Can we start powering up the device during try/set fmt and then implement the format caching as an improvement? Laurent mentioned that some cameras missbehave if a lot of controls are set during probing. I hope that this approach does not trigger those, and if it does it would be easier to revert if we do the work in two steps. > > I think that should sort the issue, assuming that 1. above holds true. > > One downside is that this stops UVC button presses from working when > not streaming. But userspace will typically only open the /dev/video# > node if it plans to stream anyways so there should not be much of > a difference wrt button press behavior. I do not personally use the button, but it is currently implemented as a standard HID device. Making it only work during streamon() might be a bit weird. I am afraid that if there is a button we should keep the current behaviour. > > This should also make camera enumeration faster for apps, since > most apps / frameworks do the whole 3 levels nested loop for this > on startup, for which atm we go out to the hw, which now instead > will come from the fmts cache and thus will be much much faster, > so this should lead to a noticeable speedup for apps accessing UVC > cameras which would be another nice win. > > Downside is that the initial probe will take longer see we do > all the tryfmt-s there now. But I think that taking a bit longer > to probe while the machine is booting should not be an issue. How do you pretend to handle the controls? Do you plan to power-up the device during s_ctrl() or set them only during streamon()? If we power-up the device during s_ctrl we need to take care of the asynchronous controls (typically pan/tilt/zoom), The device must be powered until the control finishes, and the device might never reply control_done if the firmware is not properly implemented. If we set the controls only during streamon, we will break some usecases. There are some video conferencing equipment that do homing during streamoff. That will be a serious API breakage. This patchset is not only to fix the powersaving issues, but also to fix the issue when a camera has a gpio privacy switch and an internal Privacy control. 4 years ago I did not see any camera with Privacy control (in 100s of models), now they are common. Can we have both changes, gpio subdevice and granular power saving? > > Regards, > > Hans > > > 1) Which is technically correct, but FWIW I agree with you that I think > most userspace consumers will not care > > > > > > Last year, we proposed a patchset to implement the privacy gpio as a > > subdevice https://lore.kernel.org/linux-media/20230111-uvc_privacy_subdev-v1-0-f859ac9a01e3@chromium.org/ > > > > I think it is a pretty clean solution and makes sense to use a > > subdevice for something that is a sub device of the camera :). > > > > This is an attempt to continue with that approach. > > > > Tested on gimble: > > gimble-rev3 ~ # v4l2-ctl --all -d /dev/v4l-subdev0 > > Driver Info: > > Driver version : 6.6.56 > > Capabilities : 0x00000000 > > Media Driver Info: > > Driver name : uvcvideo > > Model : HP 5M Camera: HP 5M Camera > > Serial : 0001 > > Bus info : usb-0000:00:14.0-6 > > Media version : 6.6.56 > > Hardware revision: 0x00009601 (38401) > > Driver version : 6.6.56 > > Interface Info: > > ID : 0x0300001d > > Type : V4L Sub-Device > > Entity Info: > > ID : 0x00000013 (19) > > Name : GPIO > > Function : Unknown sub-device (00020006) > > > > Camera Controls > > > > privacy 0x009a0910 (bool) : default=0 value=0 flags=read-only, volatile > > > > gimble-rev3 ~ # media-ctl -p > > Media controller API version 6.6.56 > > > > Media device information > > ------------------------ > > driver uvcvideo > > model HP 5M Camera: HP 5M Camera > > serial 0001 > > bus info usb-0000:00:14.0-6 > > hw revision 0x9601 > > driver version 6.6.56 > > > > Device topology > > - entity 1: HP 5M Camera: HP 5M Camera (1 pad, 1 link) > > type Node subtype V4L flags 1 > > device node name /dev/video0 > > pad0: Sink > > <- "Extension 8":1 [ENABLED,IMMUTABLE] > > > > - entity 4: HP 5M Camera: HP 5M Camera (0 pad, 0 link) > > type Node subtype V4L flags 0 > > device node name /dev/video1 > > > > - entity 8: Extension 8 (2 pads, 2 links, 0 routes) > > type V4L2 subdev subtype Unknown flags 0 > > pad0: Sink > > <- "Extension 4":1 [ENABLED,IMMUTABLE] > > pad1: Source > > -> "HP 5M Camera: HP 5M Camera":0 [ENABLED,IMMUTABLE] > > > > - entity 11: Extension 4 (2 pads, 2 links, 0 routes) > > type V4L2 subdev subtype Unknown flags 0 > > pad0: Sink > > <- "Processing 2":1 [ENABLED,IMMUTABLE] > > pad1: Source > > -> "Extension 8":0 [ENABLED,IMMUTABLE] > > > > - entity 14: Processing 2 (2 pads, 2 links, 0 routes) > > type V4L2 subdev subtype Unknown flags 0 > > pad0: Sink > > <- "Camera 1":0 [ENABLED,IMMUTABLE] > > pad1: Source > > -> "Extension 4":0 [ENABLED,IMMUTABLE] > > > > - entity 17: Camera 1 (1 pad, 1 link, 0 routes) > > type V4L2 subdev subtype Sensor flags 0 > > pad0: Source > > -> "Processing 2":0 [ENABLED,IMMUTABLE] > > > > - entity 19: GPIO (0 pad, 0 link, 0 routes) > > type V4L2 subdev subtype Decoder flags 0 > > device node name /dev/v4l-subdev0 > > > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > > --- > > Changes in v2: > > - Rebase on top of https://patchwork.linuxtv.org/project/linux-media/patch/20241106-uvc-crashrmmod-v6-1-fbf9781c6e83@chromium.org/ > > - Create uvc_gpio_cleanup and uvc_gpio_deinit > > - Refactor quirk: do not disable irq > > - Change define number for MEDIA_ENT_F_GPIO > > - Link to v1: https://lore.kernel.org/r/20241031-uvc-subdev-v1-0-a68331cedd72@chromium.org > > > > --- > > Ricardo Ribalda (5): > > media: uvcvideo: Factor out gpio functions to its own file > > Revert "media: uvcvideo: Allow entity-defined get_info and get_cur" > > media: uvcvideo: Create ancillary link for GPIO subdevice > > media: v4l2-core: Add new MEDIA_ENT_F_GPIO > > media: uvcvideo: Use MEDIA_ENT_F_GPIO for the GPIO entity > > > > Yunke Cao (1): > > media: uvcvideo: Re-implement privacy GPIO as a separate subdevice > > > > .../userspace-api/media/mediactl/media-types.rst | 4 + > > drivers/media/usb/uvc/Makefile | 3 +- > > drivers/media/usb/uvc/uvc_ctrl.c | 40 +---- > > drivers/media/usb/uvc/uvc_driver.c | 123 +------------- > > drivers/media/usb/uvc/uvc_entity.c | 20 ++- > > drivers/media/usb/uvc/uvc_gpio.c | 187 +++++++++++++++++++++ > > drivers/media/usb/uvc/uvc_video.c | 4 + > > drivers/media/usb/uvc/uvcvideo.h | 34 ++-- > > drivers/media/v4l2-core/v4l2-async.c | 3 +- > > include/uapi/linux/media.h | 1 + > > 10 files changed, 252 insertions(+), 167 deletions(-) > > --- > > base-commit: 4353256f5487e0c5c47e8ff764bf4f9e679fb525 > > change-id: 20241030-uvc-subdev-89f4467a00b5 > > > > Best regards, >
Em Sat, 9 Nov 2024 17:29:54 +0100 Ricardo Ribalda <ribalda@chromium.org> escreveu: > > > > I think that should sort the issue, assuming that 1. above holds true. > > > > One downside is that this stops UVC button presses from working when > > not streaming. But userspace will typically only open the /dev/video# > > node if it plans to stream anyways so there should not be much of > > a difference wrt button press behavior. > > I do not personally use the button, but it is currently implemented as > a standard HID device. IMO, controlling the privacy via evdev is the best approach then. There's no need for a RW control neither at subdev or at device level. It could make sense a Read only to allow apps to read, but still it shall be up to the Kernel to protect the stream if the button is pressed. > Making it only work during streamon() might be > a bit weird. > I am afraid that if there is a button we should keep the current behaviour. Privacy matters only when streaming. IMO the Kernel check for it needs to be done at DQBUF time and at read() calls, as one can enable/disable the camera while doing videoconf calls. I do that a lot with app "soft" buttons, and on devices that physically support cutting the video. I don't trust myself privacy soft buttons, specially when handled in userspace, so what I have are webcam covers (and a small stick glued at a laptop camera that has a too small sensor for a webcam cover). I only remove the cover/stick when I want to participate on videoconf with video enabled with the builtin camera. Regards Thanks, Mauro
On 10/11/2024 11:02, Mauro Carvalho Chehab wrote: > Em Sat, 9 Nov 2024 17:29:54 +0100 > Ricardo Ribalda <ribalda@chromium.org> escreveu: > >>> >>> I think that should sort the issue, assuming that 1. above holds true. >>> >>> One downside is that this stops UVC button presses from working when >>> not streaming. But userspace will typically only open the /dev/video# >>> node if it plans to stream anyways so there should not be much of >>> a difference wrt button press behavior. >> >> I do not personally use the button, but it is currently implemented as >> a standard HID device. > > IMO, controlling the privacy via evdev is the best approach then. There's > no need for a RW control neither at subdev or at device level. It could > make sense a Read only to allow apps to read, but still it shall be up to > the Kernel to protect the stream if the button is pressed. > >> Making it only work during streamon() might be >> a bit weird. >> I am afraid that if there is a button we should keep the current behaviour. > > Privacy matters only when streaming. IMO the Kernel check for it needs to > be done at DQBUF time and at read() calls, as one can enable/disable the > camera while doing videoconf calls. I do that a lot with app "soft" buttons, > and on devices that physically support cutting the video. We could add a vb2_s_privacy(bool privacy) function to vb2 to tell vb2 if the privacy mode is on. And if so, take action. E.g. calling QBUF/DQBUF would return a -EACCES error. That will ensure consistent behavior for all drivers that have a privacy function. Note that there are two types of privacy GPIO: one that triggers when a physical cover is moved, blocking the sensor, and one that is a button relying on software to stop streaming video. In the first case it is informative, but you can keep streaming. Regards, Hans > > I don't trust myself privacy soft buttons, specially when handled in userspace, > so what I have are webcam covers (and a small stick glued at a laptop camera > that has a too small sensor for a webcam cover). I only remove the cover/stick > when I want to participate on videoconf with video enabled with the builtin > camera. > > Regards > > Thanks, > Mauro >
Hi Mauro On Sun, 10 Nov 2024 at 11:03, Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote: > > Em Sat, 9 Nov 2024 17:29:54 +0100 > Ricardo Ribalda <ribalda@chromium.org> escreveu: > > > > > > > I think that should sort the issue, assuming that 1. above holds true. > > > > > > One downside is that this stops UVC button presses from working when > > > not streaming. But userspace will typically only open the /dev/video# > > > node if it plans to stream anyways so there should not be much of > > > a difference wrt button press behavior. > > > > I do not personally use the button, but it is currently implemented as > > a standard HID device. > > IMO, controlling the privacy via evdev is the best approach then. There's > no need for a RW control neither at subdev or at device level. It could > make sense a Read only to allow apps to read, but still it shall be up to > the Kernel to protect the stream if the button is pressed. > > > Making it only work during streamon() might be > > a bit weird. > > I am afraid that if there is a button we should keep the current behaviour. > > Privacy matters only when streaming. IMO the Kernel check for it needs to > be done at DQBUF time and at read() calls, as one can enable/disable the > camera while doing videoconf calls. I do that a lot with app "soft" buttons, > and on devices that physically support cutting the video. > > I don't trust myself privacy soft buttons, specially when handled in userspace, > so what I have are webcam covers (and a small stick glued at a laptop camera > that has a too small sensor for a webcam cover). I only remove the cover/stick > when I want to participate on videoconf with video enabled with the builtin > camera. > > Regards I think we are mixing up concepts here. On one side we have the uvc button. You can see one here https://www.sellpy.dk/item/2Yk1ZULbki?utm_source=google&utm_medium=cpc&utm_campaign=17610409619&gad_source=1&gclid=Cj0KCQiA0MG5BhD1ARIsAEcZtwR9-09ZtTIVNbVknrZCtCd7ezVM8YFw1yQXfs81FWhofg9eW-iBrsIaAopVEALw_wcB That button is not represented as a hid device. We do not know how the user will use this button. They could even use it to start an app when pressed. On the other side we have the privacy gpio. The chassis has a switch that is connected to the camera and to the SOC. You can see one here: https://support.hp.com/ie-en/document/ish_3960099-3335046-16 .We link the camera with a gpio via the acpi table. When the user flips the button, the camera produces black frames and the SOC gets an IRQ. The IRQ is used to display a message like "Camera off" and the value of the GPIO can be checked when an app is running to tell the user: "Camera not available, flip the privacy button if you want to use it." Userspace cannot change the value of the gpio. It is read-only, userspace cannot override the privacy switch. The privacy gpio is represented with a control in /dev/videoX This patchset wants to move it to /dev/v4l2-subdevX To make things more complicated. Recently some cameras are starting to have their own privacy control without the need of an external gpio. This is also represented as a control in /dev/videoX. Now that we have these 3 concepts in place: Today a uvc camera is powered up when /dev/videoX is open(), not when it is streaming. This means that if we want to get an event for the privacy gpio we have to powerup the camera, which results in power consumption. This can be fixed by moving the control to a subdevice (technically the gpio is not part of the camera, so it makes sense). If we only powerup the camera during streamon we will break the uvc button, and the async controls. > > Thanks, > Mauro
On Sun, 10 Nov 2024 at 11:29, Hans Verkuil <hverkuil@xs4all.nl> wrote: > > On 10/11/2024 11:02, Mauro Carvalho Chehab wrote: > > Em Sat, 9 Nov 2024 17:29:54 +0100 > > Ricardo Ribalda <ribalda@chromium.org> escreveu: > > > >>> > >>> I think that should sort the issue, assuming that 1. above holds true. > >>> > >>> One downside is that this stops UVC button presses from working when > >>> not streaming. But userspace will typically only open the /dev/video# > >>> node if it plans to stream anyways so there should not be much of > >>> a difference wrt button press behavior. > >> > >> I do not personally use the button, but it is currently implemented as > >> a standard HID device. > > > > IMO, controlling the privacy via evdev is the best approach then. There's > > no need for a RW control neither at subdev or at device level. It could > > make sense a Read only to allow apps to read, but still it shall be up to > > the Kernel to protect the stream if the button is pressed. > > > >> Making it only work during streamon() might be > >> a bit weird. > >> I am afraid that if there is a button we should keep the current behaviour. > > > > Privacy matters only when streaming. IMO the Kernel check for it needs to > > be done at DQBUF time and at read() calls, as one can enable/disable the > > camera while doing videoconf calls. I do that a lot with app "soft" buttons, > > and on devices that physically support cutting the video. > > We could add a vb2_s_privacy(bool privacy) function to vb2 to tell vb2 if the privacy > mode is on. And if so, take action. E.g. calling QBUF/DQBUF would return a -EACCES error. > > That will ensure consistent behavior for all drivers that have a privacy function. I am not against adding a feature like this, but we still need a way to notify userspace about a change of the privacy state when the user presses it. Controls are great for this. > > Note that there are two types of privacy GPIO: one that triggers when a physical > cover is moved, blocking the sensor, and one that is a button relying on software > to stop streaming video. In the first case it is informative, but you can keep > streaming. I am curious who implements a software privacy switch. I would definitely use a physical cover in those devices. Chromebooks only support physical cover and hardware privacy switch. I have not seen software privacy switches. > > Regards, > > Hans > > > > > I don't trust myself privacy soft buttons, specially when handled in userspace, > > so what I have are webcam covers (and a small stick glued at a laptop camera > > that has a too small sensor for a webcam cover). I only remove the cover/stick > > when I want to participate on videoconf with video enabled with the builtin > > camera. > > > > Regards > > > > Thanks, > > Mauro > > >
On 10/11/2024 11:37, Ricardo Ribalda wrote: > On Sun, 10 Nov 2024 at 11:29, Hans Verkuil <hverkuil@xs4all.nl> wrote: >> >> On 10/11/2024 11:02, Mauro Carvalho Chehab wrote: >>> Em Sat, 9 Nov 2024 17:29:54 +0100 >>> Ricardo Ribalda <ribalda@chromium.org> escreveu: >>> >>>>> >>>>> I think that should sort the issue, assuming that 1. above holds true. >>>>> >>>>> One downside is that this stops UVC button presses from working when >>>>> not streaming. But userspace will typically only open the /dev/video# >>>>> node if it plans to stream anyways so there should not be much of >>>>> a difference wrt button press behavior. >>>> >>>> I do not personally use the button, but it is currently implemented as >>>> a standard HID device. >>> >>> IMO, controlling the privacy via evdev is the best approach then. There's >>> no need for a RW control neither at subdev or at device level. It could >>> make sense a Read only to allow apps to read, but still it shall be up to >>> the Kernel to protect the stream if the button is pressed. >>> >>>> Making it only work during streamon() might be >>>> a bit weird. >>>> I am afraid that if there is a button we should keep the current behaviour. >>> >>> Privacy matters only when streaming. IMO the Kernel check for it needs to >>> be done at DQBUF time and at read() calls, as one can enable/disable the >>> camera while doing videoconf calls. I do that a lot with app "soft" buttons, >>> and on devices that physically support cutting the video. >> >> We could add a vb2_s_privacy(bool privacy) function to vb2 to tell vb2 if the privacy >> mode is on. And if so, take action. E.g. calling QBUF/DQBUF would return a -EACCES error. >> >> That will ensure consistent behavior for all drivers that have a privacy function. > > I am not against adding a feature like this, but we still need a way > to notify userspace about a change of the privacy state when the user > presses it. > Controls are great for this. > >> >> Note that there are two types of privacy GPIO: one that triggers when a physical >> cover is moved, blocking the sensor, and one that is a button relying on software >> to stop streaming video. In the first case it is informative, but you can keep >> streaming. > > I am curious who implements a software privacy switch. I would > definitely use a physical cover in those devices. > > Chromebooks only support physical cover and hardware privacy switch. I > have not seen software privacy switches. I actually thought this discussion was about software privacy switched. I haven't seen any, but it wouldn't surprise me if there are webcams that do something like that. For proper privacy covers I don't think you need a vb2 implementation, even if you keep streaming you should just see black video, no need to refuse QBUF/DQBUF. Regards, Hans > >> >> Regards, >> >> Hans >> >>> >>> I don't trust myself privacy soft buttons, specially when handled in userspace, >>> so what I have are webcam covers (and a small stick glued at a laptop camera >>> that has a too small sensor for a webcam cover). I only remove the cover/stick >>> when I want to participate on videoconf with video enabled with the builtin >>> camera. >>> >>> Regards >>> >>> Thanks, >>> Mauro >>> >> > >
On Sun, 10 Nov 2024 at 11:32, Ricardo Ribalda <ribalda@chromium.org> wrote: > > Hi Mauro > > On Sun, 10 Nov 2024 at 11:03, Mauro Carvalho Chehab > <mchehab+huawei@kernel.org> wrote: > > > > Em Sat, 9 Nov 2024 17:29:54 +0100 > > Ricardo Ribalda <ribalda@chromium.org> escreveu: > > > > > > > > > > I think that should sort the issue, assuming that 1. above holds true. > > > > > > > > One downside is that this stops UVC button presses from working when > > > > not streaming. But userspace will typically only open the /dev/video# > > > > node if it plans to stream anyways so there should not be much of > > > > a difference wrt button press behavior. > > > > > > I do not personally use the button, but it is currently implemented as > > > a standard HID device. > > > > IMO, controlling the privacy via evdev is the best approach then. There's > > no need for a RW control neither at subdev or at device level. It could > > make sense a Read only to allow apps to read, but still it shall be up to > > the Kernel to protect the stream if the button is pressed. > > > > > Making it only work during streamon() might be > > > a bit weird. > > > I am afraid that if there is a button we should keep the current behaviour. > > > > Privacy matters only when streaming. IMO the Kernel check for it needs to > > be done at DQBUF time and at read() calls, as one can enable/disable the > > camera while doing videoconf calls. I do that a lot with app "soft" buttons, > > and on devices that physically support cutting the video. > > > > I don't trust myself privacy soft buttons, specially when handled in userspace, > > so what I have are webcam covers (and a small stick glued at a laptop camera > > that has a too small sensor for a webcam cover). I only remove the cover/stick > > when I want to participate on videoconf with video enabled with the builtin > > camera. > > > > Regards > > I think we are mixing up concepts here. > > On one side we have the uvc button. You can see one here > https://www.sellpy.dk/item/2Yk1ZULbki?utm_source=google&utm_medium=cpc&utm_campaign=17610409619&gad_source=1&gclid=Cj0KCQiA0MG5BhD1ARIsAEcZtwR9-09ZtTIVNbVknrZCtCd7ezVM8YFw1yQXfs81FWhofg9eW-iBrsIaAopVEALw_wcB > That button is not represented as a hid device. We do not know how the That button is NOW represented as a hid device. sorry :) > user will use this button. They could even use it to start an app when > pressed. > > On the other side we have the privacy gpio. The chassis has a switch > that is connected to the camera and to the SOC. You can see one here: > https://support.hp.com/ie-en/document/ish_3960099-3335046-16 .We link > the camera with a gpio via the acpi table. When the user flips the > button, the camera produces black frames and the SOC gets an IRQ. The > IRQ is used to display a message like "Camera off" and the value of > the GPIO can be checked when an app is running to tell the user: > "Camera not available, flip the privacy button if you want to use it." > Userspace cannot change the value of the gpio. It is read-only, > userspace cannot override the privacy switch. The privacy gpio is > represented with a control in /dev/videoX This patchset wants to move > it to /dev/v4l2-subdevX > > To make things more complicated. Recently some cameras are starting to > have their own privacy control without the need of an external gpio. > This is also represented as a control in /dev/videoX. > > > Now that we have these 3 concepts in place: > > Today a uvc camera is powered up when /dev/videoX is open(), not when > it is streaming. This means that if we want to get an event for the > privacy gpio we have to powerup the camera, which results in power > consumption. This can be fixed by moving the control to a subdevice > (technically the gpio is not part of the camera, so it makes sense). > > If we only powerup the camera during streamon we will break the uvc > button, and the async controls. > > > > > > > Thanks, > > Mauro > > > > -- > Ricardo Ribalda
Em Sun, 10 Nov 2024 11:32:16 +0100 Ricardo Ribalda <ribalda@chromium.org> escreveu: > Hi Mauro > > On Sun, 10 Nov 2024 at 11:03, Mauro Carvalho Chehab > <mchehab+huawei@kernel.org> wrote: > > > > Em Sat, 9 Nov 2024 17:29:54 +0100 > > Ricardo Ribalda <ribalda@chromium.org> escreveu: > > > > > > > > > > I think that should sort the issue, assuming that 1. above holds true. > > > > > > > > One downside is that this stops UVC button presses from working when > > > > not streaming. But userspace will typically only open the /dev/video# > > > > node if it plans to stream anyways so there should not be much of > > > > a difference wrt button press behavior. > > > > > > I do not personally use the button, but it is currently implemented as > > > a standard HID device. > > > > IMO, controlling the privacy via evdev is the best approach then. There's > > no need for a RW control neither at subdev or at device level. It could > > make sense a Read only to allow apps to read, but still it shall be up to > > the Kernel to protect the stream if the button is pressed. > > > > > Making it only work during streamon() might be > > > a bit weird. > > > I am afraid that if there is a button we should keep the current behaviour. > > > > Privacy matters only when streaming. IMO the Kernel check for it needs to > > be done at DQBUF time and at read() calls, as one can enable/disable the > > camera while doing videoconf calls. I do that a lot with app "soft" buttons, > > and on devices that physically support cutting the video. > > > > I don't trust myself privacy soft buttons, specially when handled in userspace, > > so what I have are webcam covers (and a small stick glued at a laptop camera > > that has a too small sensor for a webcam cover). I only remove the cover/stick > > when I want to participate on videoconf with video enabled with the builtin > > camera. > > > > Regards > > I think we are mixing up concepts here. > > On one side we have the uvc button. You can see one here > https://www.sellpy.dk/item/2Yk1ZULbki?utm_source=google&utm_medium=cpc&utm_campaign=17610409619&gad_source=1&gclid=Cj0KCQiA0MG5BhD1ARIsAEcZtwR9-09ZtTIVNbVknrZCtCd7ezVM8YFw1yQXfs81FWhofg9eW-iBrsIaAopVEALw_wcB > That button is not represented as a hid device. We do not know how the > user will use this button. They could even use it to start an app when > pressed. Old cameras have a <snapshot> button. Maybe that's the case of the device you're pointing, as it looks some non-uvc Logitech cameras I have myself. > On the other side we have the privacy gpio. The chassis has a switch > that is connected to the camera and to the SOC. You can see one here: > https://support.hp.com/ie-en/document/ish_3960099-3335046-16 .We link > the camera with a gpio via the acpi table. When the user flips the > button, the camera produces black frames and the SOC gets an IRQ. OK, so the hardware warrants black frames. Sounds a more secure implementation. > The IRQ is used to display a message like "Camera off" and the value of > the GPIO can be checked when an app is running to tell the user: > "Camera not available, flip the privacy button if you want to use it." So, it is not really a privacy gpio/control. It is instead a privacy notification control. I would better name it to clearly indicate what it is about. > Userspace cannot change the value of the gpio. It is read-only, > userspace cannot override the privacy switch. The privacy gpio is > represented with a control in /dev/videoX This patchset wants to move > it to /dev/v4l2-subdevX Well, if it is really a gpio pin, kernel (and eventually userspace) can force it to pullup (or pulldown) state, forcing one of the states. If, instead is an output-only pin, kernel/userspace can't control it at all. > To make things more complicated. Recently some cameras are starting to > have their own privacy control without the need of an external gpio. > This is also represented as a control in /dev/videoX. IMO, both privacy notification events shall be reported the same way, no matter if they use GPIO, an input pin or something else. > Now that we have these 3 concepts in place: > > Today a uvc camera is powered up when /dev/videoX is open(), not when > it is streaming. Ideally, the part of the hardware responsible for streaming shall be powered on only while streaming. I agree with Hans de Goede: better have this fixed before the privacy notification patches. > This means that if we want to get an event for the > privacy gpio we have to powerup the camera, which results in power > consumption. This can be fixed by moving the control to a subdevice > (technically the gpio is not part of the camera, so it makes sense). Ok, but as you said, not all cameras implement it as a separate gpio. > If we only powerup the camera during streamon we will break the uvc > button, and the async controls. Why? IMO, it shall use regmap in a way that the register settings will be sent to the device only when the camera control hardware is powered up. On a complex device, there are likely at least two power up hardware: the camera control logic and the streaming logic. Not sure if both are visible via UVC spec, though. Thanks, Mauro
Em Sun, 10 Nov 2024 11:37:46 +0100 Ricardo Ribalda <ribalda@chromium.org> escreveu: > > > Privacy matters only when streaming. IMO the Kernel check for it needs to > > > be done at DQBUF time and at read() calls, as one can enable/disable the > > > camera while doing videoconf calls. I do that a lot with app "soft" buttons, > > > and on devices that physically support cutting the video. > > > > We could add a vb2_s_privacy(bool privacy) function to vb2 to tell vb2 if the privacy > > mode is on. And if so, take action. E.g. calling QBUF/DQBUF would return a -EACCES error. > > > > That will ensure consistent behavior for all drivers that have a privacy function. > > I am not against adding a feature like this, but we still need a way > to notify userspace about a change of the privacy state when the user > presses it. > Controls are great for this. It doesn't sound a good idea. See, from users PoV, they want the stream to start with black frames when privacy is on, and dynamically being able to enable/disable actual frame output. So, the best is to have the stream running independently of the privacy being enabled or not. Thanks, Mauro
On Sat, Nov 09, 2024 at 04:37:10PM +0100, Hans de Goede wrote: > Hi Ricardo, > > FYI / some background: I have been asked to start helping / > co-maintaining UVC with Laurent. I'll send out a patch adding > myself as UVC maintainer soon. > > On 8-Nov-24 9:25 PM, Ricardo Ribalda wrote: > > Some notebooks have a button to disable the camera (not to be mistaken > > with the mechanical cover). This is a standard GPIO linked to the > > camera via the ACPI table. > > > > 4 years ago we added support for this button in UVC via the Privacy control. > > This has two issues: > > - If the camera has its own privacy control, it will be masked > > - We need to power-up the camera to read the privacy control gpio. > > > > We tried to fix the power-up issues implementing "granular power > > saving" but it has been more complicated than anticipated.... > > I have been discussing UVC power-management with Laurent, also > related to power-consumption issues caused by libcamera's pipeline > handler holding open the /dev/video# node as long as the camera > manager object exists. > > For now we have fixed this with some relatively small changes to > libcamera's uvcvideo pipeline handler, but that is really meant > as an interim solution and as this privacy control series shows > the power-management issues are real. > > Combined with Mauro's remarks about how this is an userspace ABI break (1) > I think we should maybe first take another look at the powermanagement > issues in general rather then moving forward with this series. > > My apologies for this, I realize how annoying it can be when you are > working on a patch series to fix a specific issue and a reviewer > moves the goal-posts like this. But I do really think that just fixing > the generic power-management issues would be better and I also think > that this should be feasible / not too hard. > > Here is what I have in mind for this: > > 1. Assume that the results of trying a specific fmt do not change over time. > > 2. Only allow userspace to request fmts which match one of the enum-fmts -> > enum-frame-sizes -> enum-frame-rates tripplet results > (constrain what userspace requests to these) > > 3. Run the equivalent of tryfmt on all possible combinations (so the usaul > 3 levels nested loop for this) on probe() and cache the results This is possibly problematic. If I recall correctly (it was a very long time ago), the gstreamer v4l2src element used to do this, and it resulted in very large delays (> 10s with some devices), and possibly in some devices crashing as well. > 4. Make try_fmt / set_fmt not poweron the device but instead constrain > the requested fmt to one from our cached fmts > > 5. On stream-on do the actual power-on + set-fmt + verify that we get > what we expect based on the cache, and otherwise return -EIO. > > I think that should sort the issue, assuming that 1. above holds true. > > One downside is that this stops UVC button presses from working when > not streaming. But userspace will typically only open the /dev/video# > node if it plans to stream anyways so there should not be much of > a difference wrt button press behavior. > > This should also make camera enumeration faster for apps, since > most apps / frameworks do the whole 3 levels nested loop for this > on startup, for which atm we go out to the hw, which now instead > will come from the fmts cache and thus will be much much faster, > so this should lead to a noticeable speedup for apps accessing UVC > cameras which would be another nice win. > > Downside is that the initial probe will take longer see we do > all the tryfmt-s there now. But I think that taking a bit longer > to probe while the machine is booting should not be an issue. It depends on how much "a bit" is. I don't think a >10s delay would be acceptable. > 1) Which is technically correct, but FWIW I agree with you that I think > most userspace consumers will not care > > > Last year, we proposed a patchset to implement the privacy gpio as a > > subdevice https://lore.kernel.org/linux-media/20230111-uvc_privacy_subdev-v1-0-f859ac9a01e3@chromium.org/ > > > > I think it is a pretty clean solution and makes sense to use a > > subdevice for something that is a sub device of the camera :). > > > > This is an attempt to continue with that approach. > > > > Tested on gimble: > > gimble-rev3 ~ # v4l2-ctl --all -d /dev/v4l-subdev0 > > Driver Info: > > Driver version : 6.6.56 > > Capabilities : 0x00000000 > > Media Driver Info: > > Driver name : uvcvideo > > Model : HP 5M Camera: HP 5M Camera > > Serial : 0001 > > Bus info : usb-0000:00:14.0-6 > > Media version : 6.6.56 > > Hardware revision: 0x00009601 (38401) > > Driver version : 6.6.56 > > Interface Info: > > ID : 0x0300001d > > Type : V4L Sub-Device > > Entity Info: > > ID : 0x00000013 (19) > > Name : GPIO > > Function : Unknown sub-device (00020006) > > > > Camera Controls > > > > privacy 0x009a0910 (bool) : default=0 value=0 flags=read-only, volatile > > > > gimble-rev3 ~ # media-ctl -p > > Media controller API version 6.6.56 > > > > Media device information > > ------------------------ > > driver uvcvideo > > model HP 5M Camera: HP 5M Camera > > serial 0001 > > bus info usb-0000:00:14.0-6 > > hw revision 0x9601 > > driver version 6.6.56 > > > > Device topology > > - entity 1: HP 5M Camera: HP 5M Camera (1 pad, 1 link) > > type Node subtype V4L flags 1 > > device node name /dev/video0 > > pad0: Sink > > <- "Extension 8":1 [ENABLED,IMMUTABLE] > > > > - entity 4: HP 5M Camera: HP 5M Camera (0 pad, 0 link) > > type Node subtype V4L flags 0 > > device node name /dev/video1 > > > > - entity 8: Extension 8 (2 pads, 2 links, 0 routes) > > type V4L2 subdev subtype Unknown flags 0 > > pad0: Sink > > <- "Extension 4":1 [ENABLED,IMMUTABLE] > > pad1: Source > > -> "HP 5M Camera: HP 5M Camera":0 [ENABLED,IMMUTABLE] > > > > - entity 11: Extension 4 (2 pads, 2 links, 0 routes) > > type V4L2 subdev subtype Unknown flags 0 > > pad0: Sink > > <- "Processing 2":1 [ENABLED,IMMUTABLE] > > pad1: Source > > -> "Extension 8":0 [ENABLED,IMMUTABLE] > > > > - entity 14: Processing 2 (2 pads, 2 links, 0 routes) > > type V4L2 subdev subtype Unknown flags 0 > > pad0: Sink > > <- "Camera 1":0 [ENABLED,IMMUTABLE] > > pad1: Source > > -> "Extension 4":0 [ENABLED,IMMUTABLE] > > > > - entity 17: Camera 1 (1 pad, 1 link, 0 routes) > > type V4L2 subdev subtype Sensor flags 0 > > pad0: Source > > -> "Processing 2":0 [ENABLED,IMMUTABLE] > > > > - entity 19: GPIO (0 pad, 0 link, 0 routes) > > type V4L2 subdev subtype Decoder flags 0 > > device node name /dev/v4l-subdev0 > > > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > > --- > > Changes in v2: > > - Rebase on top of https://patchwork.linuxtv.org/project/linux-media/patch/20241106-uvc-crashrmmod-v6-1-fbf9781c6e83@chromium.org/ > > - Create uvc_gpio_cleanup and uvc_gpio_deinit > > - Refactor quirk: do not disable irq > > - Change define number for MEDIA_ENT_F_GPIO > > - Link to v1: https://lore.kernel.org/r/20241031-uvc-subdev-v1-0-a68331cedd72@chromium.org > > > > --- > > Ricardo Ribalda (5): > > media: uvcvideo: Factor out gpio functions to its own file > > Revert "media: uvcvideo: Allow entity-defined get_info and get_cur" > > media: uvcvideo: Create ancillary link for GPIO subdevice > > media: v4l2-core: Add new MEDIA_ENT_F_GPIO > > media: uvcvideo: Use MEDIA_ENT_F_GPIO for the GPIO entity > > > > Yunke Cao (1): > > media: uvcvideo: Re-implement privacy GPIO as a separate subdevice > > > > .../userspace-api/media/mediactl/media-types.rst | 4 + > > drivers/media/usb/uvc/Makefile | 3 +- > > drivers/media/usb/uvc/uvc_ctrl.c | 40 +---- > > drivers/media/usb/uvc/uvc_driver.c | 123 +------------- > > drivers/media/usb/uvc/uvc_entity.c | 20 ++- > > drivers/media/usb/uvc/uvc_gpio.c | 187 +++++++++++++++++++++ > > drivers/media/usb/uvc/uvc_video.c | 4 + > > drivers/media/usb/uvc/uvcvideo.h | 34 ++-- > > drivers/media/v4l2-core/v4l2-async.c | 3 +- > > include/uapi/linux/media.h | 1 + > > 10 files changed, 252 insertions(+), 167 deletions(-) > > --- > > base-commit: 4353256f5487e0c5c47e8ff764bf4f9e679fb525 > > change-id: 20241030-uvc-subdev-89f4467a00b5
On Sat, Nov 09, 2024 at 05:29:54PM +0100, Ricardo Ribalda wrote: > On Sat, 9 Nov 2024 at 16:37, Hans de Goede <hdegoede@redhat.com> wrote: > > > > Hi Ricardo, > > > > FYI / some background: I have been asked to start helping / > > co-maintaining UVC with Laurent. I'll send out a patch adding > > myself as UVC maintainer soon. > > Great! I talked with Laurent yesterday, I hope that we can maintain > the driver the three of us in the near future. > > > On 8-Nov-24 9:25 PM, Ricardo Ribalda wrote: > > > Some notebooks have a button to disable the camera (not to be mistaken > > > with the mechanical cover). This is a standard GPIO linked to the > > > camera via the ACPI table. > > > > > > 4 years ago we added support for this button in UVC via the Privacy control. > > > This has two issues: > > > - If the camera has its own privacy control, it will be masked > > > - We need to power-up the camera to read the privacy control gpio. > > > > > > We tried to fix the power-up issues implementing "granular power > > > saving" but it has been more complicated than anticipated.... > > > > I have been discussing UVC power-management with Laurent, also > > related to power-consumption issues caused by libcamera's pipeline > > handler holding open the /dev/video# node as long as the camera > > manager object exists. > > > > For now we have fixed this with some relatively small changes to > > libcamera's uvcvideo pipeline handler, but that is really meant > > as an interim solution and as this privacy control series shows > > the power-management issues are real. > > Indeed, we have tried to fixed it before: > https://lore.kernel.org/linux-media/20220920-resend-powersave-v5-2-692e6df6c1e2@chromium.org/ > > btw this recently landed patch was to work in this direction :) > https://lore.kernel.org/linux-media/20240926-guenter-mini-v7-1-690441953d4a@chromium.org/ > > The more people interested in this problem the better. > > > Combined with Mauro's remarks about how this is an userspace ABI break (1) > > I think we should maybe first take another look at the powermanagement > > issues in general rather then moving forward with this series. > > > > My apologies for this, I realize how annoying it can be when you are > > working on a patch series to fix a specific issue and a reviewer > > moves the goal-posts like this. But I do really think that just fixing > > the generic power-management issues would be better and I also think > > that this should be feasible / not too hard. > > > > Here is what I have in mind for this: > > > > 1. Assume that the results of trying a specific fmt do not change over time. > > > > 2. Only allow userspace to request fmts which match one of the enum-fmts -> > > enum-frame-sizes -> enum-frame-rates tripplet results > > (constrain what userspace requests to these) > > > > 3. Run the equivalent of tryfmt on all possible combinations (so the usaul > > 3 levels nested loop for this) on probe() and cache the results > > > > 4. Make try_fmt / set_fmt not poweron the device but instead constrain > > the requested fmt to one from our cached fmts > > > > 5. On stream-on do the actual power-on + set-fmt + verify that we get > > what we expect based on the cache, and otherwise return -EIO. > > Can we start powering up the device during try/set fmt and then > implement the format caching as an improvement? This sounds worth trying. We'll need to test it on a wide range of devices though, both internal and external. > Laurent mentioned that some cameras missbehave if a lot of controls > are set during probing. I hope that this approach does not trigger > those, and if it does it would be easier to revert if we do the work > in two steps. > > > I think that should sort the issue, assuming that 1. above holds true. > > > > One downside is that this stops UVC button presses from working when > > not streaming. But userspace will typically only open the /dev/video# > > node if it plans to stream anyways so there should not be much of > > a difference wrt button press behavior. > > I do not personally use the button, but it is currently implemented as > a standard HID device. Making it only work during streamon() might be > a bit weird. > I am afraid that if there is a button we should keep the current behaviour. > > > > > This should also make camera enumeration faster for apps, since > > most apps / frameworks do the whole 3 levels nested loop for this > > on startup, for which atm we go out to the hw, which now instead > > will come from the fmts cache and thus will be much much faster, > > so this should lead to a noticeable speedup for apps accessing UVC > > cameras which would be another nice win. > > > > Downside is that the initial probe will take longer see we do > > all the tryfmt-s there now. But I think that taking a bit longer > > to probe while the machine is booting should not be an issue. > > How do you pretend to handle the controls? Do you plan to power-up the > device during s_ctrl() or set them only during streamon()? > If we power-up the device during s_ctrl we need to take care of the > asynchronous controls (typically pan/tilt/zoom), The device must be > powered until the control finishes, and the device might never reply > control_done if the firmware is not properly implemented. > If we set the controls only during streamon, we will break some > usecases. There are some video conferencing equipment that do homing > during streamoff. That will be a serious API breakage. > > This patchset is not only to fix the powersaving issues, but also to > fix the issue when a camera has a gpio privacy switch and an internal > Privacy control. 4 years ago I did not see any camera with Privacy > control (in 100s of models), now they are common. > Can we have both changes, gpio subdevice and granular power saving? > > > Regards, > > > > Hans > > > > 1) Which is technically correct, but FWIW I agree with you that I think > > most userspace consumers will not care > > > > > Last year, we proposed a patchset to implement the privacy gpio as a > > > subdevice https://lore.kernel.org/linux-media/20230111-uvc_privacy_subdev-v1-0-f859ac9a01e3@chromium.org/ > > > > > > I think it is a pretty clean solution and makes sense to use a > > > subdevice for something that is a sub device of the camera :). > > > > > > This is an attempt to continue with that approach. > > > > > > Tested on gimble: > > > gimble-rev3 ~ # v4l2-ctl --all -d /dev/v4l-subdev0 > > > Driver Info: > > > Driver version : 6.6.56 > > > Capabilities : 0x00000000 > > > Media Driver Info: > > > Driver name : uvcvideo > > > Model : HP 5M Camera: HP 5M Camera > > > Serial : 0001 > > > Bus info : usb-0000:00:14.0-6 > > > Media version : 6.6.56 > > > Hardware revision: 0x00009601 (38401) > > > Driver version : 6.6.56 > > > Interface Info: > > > ID : 0x0300001d > > > Type : V4L Sub-Device > > > Entity Info: > > > ID : 0x00000013 (19) > > > Name : GPIO > > > Function : Unknown sub-device (00020006) > > > > > > Camera Controls > > > > > > privacy 0x009a0910 (bool) : default=0 value=0 flags=read-only, volatile > > > > > > gimble-rev3 ~ # media-ctl -p > > > Media controller API version 6.6.56 > > > > > > Media device information > > > ------------------------ > > > driver uvcvideo > > > model HP 5M Camera: HP 5M Camera > > > serial 0001 > > > bus info usb-0000:00:14.0-6 > > > hw revision 0x9601 > > > driver version 6.6.56 > > > > > > Device topology > > > - entity 1: HP 5M Camera: HP 5M Camera (1 pad, 1 link) > > > type Node subtype V4L flags 1 > > > device node name /dev/video0 > > > pad0: Sink > > > <- "Extension 8":1 [ENABLED,IMMUTABLE] > > > > > > - entity 4: HP 5M Camera: HP 5M Camera (0 pad, 0 link) > > > type Node subtype V4L flags 0 > > > device node name /dev/video1 > > > > > > - entity 8: Extension 8 (2 pads, 2 links, 0 routes) > > > type V4L2 subdev subtype Unknown flags 0 > > > pad0: Sink > > > <- "Extension 4":1 [ENABLED,IMMUTABLE] > > > pad1: Source > > > -> "HP 5M Camera: HP 5M Camera":0 [ENABLED,IMMUTABLE] > > > > > > - entity 11: Extension 4 (2 pads, 2 links, 0 routes) > > > type V4L2 subdev subtype Unknown flags 0 > > > pad0: Sink > > > <- "Processing 2":1 [ENABLED,IMMUTABLE] > > > pad1: Source > > > -> "Extension 8":0 [ENABLED,IMMUTABLE] > > > > > > - entity 14: Processing 2 (2 pads, 2 links, 0 routes) > > > type V4L2 subdev subtype Unknown flags 0 > > > pad0: Sink > > > <- "Camera 1":0 [ENABLED,IMMUTABLE] > > > pad1: Source > > > -> "Extension 4":0 [ENABLED,IMMUTABLE] > > > > > > - entity 17: Camera 1 (1 pad, 1 link, 0 routes) > > > type V4L2 subdev subtype Sensor flags 0 > > > pad0: Source > > > -> "Processing 2":0 [ENABLED,IMMUTABLE] > > > > > > - entity 19: GPIO (0 pad, 0 link, 0 routes) > > > type V4L2 subdev subtype Decoder flags 0 > > > device node name /dev/v4l-subdev0 > > > > > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > > > --- > > > Changes in v2: > > > - Rebase on top of https://patchwork.linuxtv.org/project/linux-media/patch/20241106-uvc-crashrmmod-v6-1-fbf9781c6e83@chromium.org/ > > > - Create uvc_gpio_cleanup and uvc_gpio_deinit > > > - Refactor quirk: do not disable irq > > > - Change define number for MEDIA_ENT_F_GPIO > > > - Link to v1: https://lore.kernel.org/r/20241031-uvc-subdev-v1-0-a68331cedd72@chromium.org > > > > > > --- > > > Ricardo Ribalda (5): > > > media: uvcvideo: Factor out gpio functions to its own file > > > Revert "media: uvcvideo: Allow entity-defined get_info and get_cur" > > > media: uvcvideo: Create ancillary link for GPIO subdevice > > > media: v4l2-core: Add new MEDIA_ENT_F_GPIO > > > media: uvcvideo: Use MEDIA_ENT_F_GPIO for the GPIO entity > > > > > > Yunke Cao (1): > > > media: uvcvideo: Re-implement privacy GPIO as a separate subdevice > > > > > > .../userspace-api/media/mediactl/media-types.rst | 4 + > > > drivers/media/usb/uvc/Makefile | 3 +- > > > drivers/media/usb/uvc/uvc_ctrl.c | 40 +---- > > > drivers/media/usb/uvc/uvc_driver.c | 123 +------------- > > > drivers/media/usb/uvc/uvc_entity.c | 20 ++- > > > drivers/media/usb/uvc/uvc_gpio.c | 187 +++++++++++++++++++++ > > > drivers/media/usb/uvc/uvc_video.c | 4 + > > > drivers/media/usb/uvc/uvcvideo.h | 34 ++-- > > > drivers/media/v4l2-core/v4l2-async.c | 3 +- > > > include/uapi/linux/media.h | 1 + > > > 10 files changed, 252 insertions(+), 167 deletions(-) > > > --- > > > base-commit: 4353256f5487e0c5c47e8ff764bf4f9e679fb525 > > > change-id: 20241030-uvc-subdev-89f4467a00b5
On Sun, 10 Nov 2024 at 13:46, Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote: > > Em Sun, 10 Nov 2024 11:32:16 +0100 > Ricardo Ribalda <ribalda@chromium.org> escreveu: > > > Hi Mauro > > > > On Sun, 10 Nov 2024 at 11:03, Mauro Carvalho Chehab > > <mchehab+huawei@kernel.org> wrote: > > > > > > Em Sat, 9 Nov 2024 17:29:54 +0100 > > > Ricardo Ribalda <ribalda@chromium.org> escreveu: > > > > > > > > > > > > > I think that should sort the issue, assuming that 1. above holds true. > > > > > > > > > > One downside is that this stops UVC button presses from working when > > > > > not streaming. But userspace will typically only open the /dev/video# > > > > > node if it plans to stream anyways so there should not be much of > > > > > a difference wrt button press behavior. > > > > > > > > I do not personally use the button, but it is currently implemented as > > > > a standard HID device. > > > > > > IMO, controlling the privacy via evdev is the best approach then. There's > > > no need for a RW control neither at subdev or at device level. It could > > > make sense a Read only to allow apps to read, but still it shall be up to > > > the Kernel to protect the stream if the button is pressed. > > > > > > > Making it only work during streamon() might be > > > > a bit weird. > > > > I am afraid that if there is a button we should keep the current behaviour. > > > > > > Privacy matters only when streaming. IMO the Kernel check for it needs to > > > be done at DQBUF time and at read() calls, as one can enable/disable the > > > camera while doing videoconf calls. I do that a lot with app "soft" buttons, > > > and on devices that physically support cutting the video. > > > > > > I don't trust myself privacy soft buttons, specially when handled in userspace, > > > so what I have are webcam covers (and a small stick glued at a laptop camera > > > that has a too small sensor for a webcam cover). I only remove the cover/stick > > > when I want to participate on videoconf with video enabled with the builtin > > > camera. > > > > > > Regards > > > > I think we are mixing up concepts here. > > > > On one side we have the uvc button. You can see one here > > https://www.sellpy.dk/item/2Yk1ZULbki?utm_source=google&utm_medium=cpc&utm_campaign=17610409619&gad_source=1&gclid=Cj0KCQiA0MG5BhD1ARIsAEcZtwR9-09ZtTIVNbVknrZCtCd7ezVM8YFw1yQXfs81FWhofg9eW-iBrsIaAopVEALw_wcB > > That button is not represented as a hid device. We do not know how the > > user will use this button. They could even use it to start an app when > > pressed. > > Old cameras have a <snapshot> button. Maybe that's the case of the device > you're pointing, as it looks some non-uvc Logitech cameras I have myself. > > > On the other side we have the privacy gpio. The chassis has a switch > > that is connected to the camera and to the SOC. You can see one here: > > https://support.hp.com/ie-en/document/ish_3960099-3335046-16 .We link > > the camera with a gpio via the acpi table. When the user flips the > > button, the camera produces black frames and the SOC gets an IRQ. > > OK, so the hardware warrants black frames. Sounds a more secure > implementation. > > > The IRQ is used to display a message like "Camera off" and the value of > > the GPIO can be checked when an app is running to tell the user: > > "Camera not available, flip the privacy button if you want to use it." > > So, it is not really a privacy gpio/control. It is instead a privacy > notification control. > > I would better name it to clearly indicate what it is about. > > > Userspace cannot change the value of the gpio. It is read-only, > > userspace cannot override the privacy switch. The privacy gpio is > > represented with a control in /dev/videoX This patchset wants to move > > it to /dev/v4l2-subdevX > > Well, if it is really a gpio pin, kernel (and eventually userspace) can force > it to pullup (or pulldown) state, forcing one of the states. If, instead is > an output-only pin, kernel/userspace can't control it at all. > > > To make things more complicated. Recently some cameras are starting to > > have their own privacy control without the need of an external gpio. > > This is also represented as a control in /dev/videoX. > > IMO, both privacy notification events shall be reported the same way, > no matter if they use GPIO, an input pin or something else. How do we handle devices that have internal privacy and GPIO if we do not use a subdevice? > > > Now that we have these 3 concepts in place: > > > > Today a uvc camera is powered up when /dev/videoX is open(), not when > > it is streaming. > > Ideally, the part of the hardware responsible for streaming shall be > powered on only while streaming. I agree with Hans de Goede: better > have this fixed before the privacy notification patches. > > > This means that if we want to get an event for the > > privacy gpio we have to powerup the camera, which results in power > > consumption. This can be fixed by moving the control to a subdevice > > (technically the gpio is not part of the camera, so it makes sense). > > Ok, but as you said, not all cameras implement it as a separate gpio. For the device that are not a separate gpio you need to powerup the device to read it. > > > If we only powerup the camera during streamon we will break the uvc > > button, and the async controls. > > Why? IMO, it shall use regmap in a way that the register settings > will be sent to the device only when the camera control hardware is > powered up. On a complex device, there are likely at least two power > up hardware: the camera control logic and the streaming logic. > > Not sure if both are visible via UVC spec, though. There are controls that need to be accessed without streaming, like the ones that do homing (calibrate the motors), report occupancy of a room. If we modify the driver behaviour to read/write controls only during this streaming we will stop supporting current use cases and definately break applications. > > Thanks, > Mauro
On Sun, 10 Nov 2024 at 16:14, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > On Sat, Nov 09, 2024 at 05:29:54PM +0100, Ricardo Ribalda wrote: > > On Sat, 9 Nov 2024 at 16:37, Hans de Goede <hdegoede@redhat.com> wrote: > > > > > > Hi Ricardo, > > > > > > FYI / some background: I have been asked to start helping / > > > co-maintaining UVC with Laurent. I'll send out a patch adding > > > myself as UVC maintainer soon. > > > > Great! I talked with Laurent yesterday, I hope that we can maintain > > the driver the three of us in the near future. > > > > > On 8-Nov-24 9:25 PM, Ricardo Ribalda wrote: > > > > Some notebooks have a button to disable the camera (not to be mistaken > > > > with the mechanical cover). This is a standard GPIO linked to the > > > > camera via the ACPI table. > > > > > > > > 4 years ago we added support for this button in UVC via the Privacy control. > > > > This has two issues: > > > > - If the camera has its own privacy control, it will be masked > > > > - We need to power-up the camera to read the privacy control gpio. > > > > > > > > We tried to fix the power-up issues implementing "granular power > > > > saving" but it has been more complicated than anticipated.... > > > > > > I have been discussing UVC power-management with Laurent, also > > > related to power-consumption issues caused by libcamera's pipeline > > > handler holding open the /dev/video# node as long as the camera > > > manager object exists. > > > > > > For now we have fixed this with some relatively small changes to > > > libcamera's uvcvideo pipeline handler, but that is really meant > > > as an interim solution and as this privacy control series shows > > > the power-management issues are real. > > > > Indeed, we have tried to fixed it before: > > https://lore.kernel.org/linux-media/20220920-resend-powersave-v5-2-692e6df6c1e2@chromium.org/ > > > > btw this recently landed patch was to work in this direction :) > > https://lore.kernel.org/linux-media/20240926-guenter-mini-v7-1-690441953d4a@chromium.org/ > > > > The more people interested in this problem the better. > > > > > Combined with Mauro's remarks about how this is an userspace ABI break (1) > > > I think we should maybe first take another look at the powermanagement > > > issues in general rather then moving forward with this series. > > > > > > My apologies for this, I realize how annoying it can be when you are > > > working on a patch series to fix a specific issue and a reviewer > > > moves the goal-posts like this. But I do really think that just fixing > > > the generic power-management issues would be better and I also think > > > that this should be feasible / not too hard. > > > > > > Here is what I have in mind for this: > > > > > > 1. Assume that the results of trying a specific fmt do not change over time. > > > > > > 2. Only allow userspace to request fmts which match one of the enum-fmts -> > > > enum-frame-sizes -> enum-frame-rates tripplet results > > > (constrain what userspace requests to these) > > > > > > 3. Run the equivalent of tryfmt on all possible combinations (so the usaul > > > 3 levels nested loop for this) on probe() and cache the results > > > > > > 4. Make try_fmt / set_fmt not poweron the device but instead constrain > > > the requested fmt to one from our cached fmts > > > > > > 5. On stream-on do the actual power-on + set-fmt + verify that we get > > > what we expect based on the cache, and otherwise return -EIO. > > > > Can we start powering up the device during try/set fmt and then > > implement the format caching as an improvement? > > This sounds worth trying. We'll need to test it on a wide range of > devices though, both internal and external. We still need a plan for asynchronous controls. And we have to decide if we stop supporting the uvc button (maybe we can start by moving USB_VIDEO_CLASS_INPUT_EVDEV to staging and see what happens?) > > > Laurent mentioned that some cameras missbehave if a lot of controls > > are set during probing. I hope that this approach does not trigger > > those, and if it does it would be easier to revert if we do the work > > in two steps. > > > > > I think that should sort the issue, assuming that 1. above holds true. > > > > > > One downside is that this stops UVC button presses from working when > > > not streaming. But userspace will typically only open the /dev/video# > > > node if it plans to stream anyways so there should not be much of > > > a difference wrt button press behavior. > > > > I do not personally use the button, but it is currently implemented as > > a standard HID device. Making it only work during streamon() might be > > a bit weird. > > I am afraid that if there is a button we should keep the current behaviour. > > > > > > > > This should also make camera enumeration faster for apps, since > > > most apps / frameworks do the whole 3 levels nested loop for this > > > on startup, for which atm we go out to the hw, which now instead > > > will come from the fmts cache and thus will be much much faster, > > > so this should lead to a noticeable speedup for apps accessing UVC > > > cameras which would be another nice win. > > > > > > Downside is that the initial probe will take longer see we do > > > all the tryfmt-s there now. But I think that taking a bit longer > > > to probe while the machine is booting should not be an issue. > > > > How do you pretend to handle the controls? Do you plan to power-up the > > device during s_ctrl() or set them only during streamon()? > > If we power-up the device during s_ctrl we need to take care of the > > asynchronous controls (typically pan/tilt/zoom), The device must be > > powered until the control finishes, and the device might never reply > > control_done if the firmware is not properly implemented. > > If we set the controls only during streamon, we will break some > > usecases. There are some video conferencing equipment that do homing > > during streamoff. That will be a serious API breakage. > > > > This patchset is not only to fix the powersaving issues, but also to > > fix the issue when a camera has a gpio privacy switch and an internal > > Privacy control. 4 years ago I did not see any camera with Privacy > > control (in 100s of models), now they are common. > > Can we have both changes, gpio subdevice and granular power saving? > > > > > Regards, > > > > > > Hans > > > > > > 1) Which is technically correct, but FWIW I agree with you that I think > > > most userspace consumers will not care > > > > > > > Last year, we proposed a patchset to implement the privacy gpio as a > > > > subdevice https://lore.kernel.org/linux-media/20230111-uvc_privacy_subdev-v1-0-f859ac9a01e3@chromium.org/ > > > > > > > > I think it is a pretty clean solution and makes sense to use a > > > > subdevice for something that is a sub device of the camera :). > > > > > > > > This is an attempt to continue with that approach. > > > > > > > > Tested on gimble: > > > > gimble-rev3 ~ # v4l2-ctl --all -d /dev/v4l-subdev0 > > > > Driver Info: > > > > Driver version : 6.6.56 > > > > Capabilities : 0x00000000 > > > > Media Driver Info: > > > > Driver name : uvcvideo > > > > Model : HP 5M Camera: HP 5M Camera > > > > Serial : 0001 > > > > Bus info : usb-0000:00:14.0-6 > > > > Media version : 6.6.56 > > > > Hardware revision: 0x00009601 (38401) > > > > Driver version : 6.6.56 > > > > Interface Info: > > > > ID : 0x0300001d > > > > Type : V4L Sub-Device > > > > Entity Info: > > > > ID : 0x00000013 (19) > > > > Name : GPIO > > > > Function : Unknown sub-device (00020006) > > > > > > > > Camera Controls > > > > > > > > privacy 0x009a0910 (bool) : default=0 value=0 flags=read-only, volatile > > > > > > > > gimble-rev3 ~ # media-ctl -p > > > > Media controller API version 6.6.56 > > > > > > > > Media device information > > > > ------------------------ > > > > driver uvcvideo > > > > model HP 5M Camera: HP 5M Camera > > > > serial 0001 > > > > bus info usb-0000:00:14.0-6 > > > > hw revision 0x9601 > > > > driver version 6.6.56 > > > > > > > > Device topology > > > > - entity 1: HP 5M Camera: HP 5M Camera (1 pad, 1 link) > > > > type Node subtype V4L flags 1 > > > > device node name /dev/video0 > > > > pad0: Sink > > > > <- "Extension 8":1 [ENABLED,IMMUTABLE] > > > > > > > > - entity 4: HP 5M Camera: HP 5M Camera (0 pad, 0 link) > > > > type Node subtype V4L flags 0 > > > > device node name /dev/video1 > > > > > > > > - entity 8: Extension 8 (2 pads, 2 links, 0 routes) > > > > type V4L2 subdev subtype Unknown flags 0 > > > > pad0: Sink > > > > <- "Extension 4":1 [ENABLED,IMMUTABLE] > > > > pad1: Source > > > > -> "HP 5M Camera: HP 5M Camera":0 [ENABLED,IMMUTABLE] > > > > > > > > - entity 11: Extension 4 (2 pads, 2 links, 0 routes) > > > > type V4L2 subdev subtype Unknown flags 0 > > > > pad0: Sink > > > > <- "Processing 2":1 [ENABLED,IMMUTABLE] > > > > pad1: Source > > > > -> "Extension 8":0 [ENABLED,IMMUTABLE] > > > > > > > > - entity 14: Processing 2 (2 pads, 2 links, 0 routes) > > > > type V4L2 subdev subtype Unknown flags 0 > > > > pad0: Sink > > > > <- "Camera 1":0 [ENABLED,IMMUTABLE] > > > > pad1: Source > > > > -> "Extension 4":0 [ENABLED,IMMUTABLE] > > > > > > > > - entity 17: Camera 1 (1 pad, 1 link, 0 routes) > > > > type V4L2 subdev subtype Sensor flags 0 > > > > pad0: Source > > > > -> "Processing 2":0 [ENABLED,IMMUTABLE] > > > > > > > > - entity 19: GPIO (0 pad, 0 link, 0 routes) > > > > type V4L2 subdev subtype Decoder flags 0 > > > > device node name /dev/v4l-subdev0 > > > > > > > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > > > > --- > > > > Changes in v2: > > > > - Rebase on top of https://patchwork.linuxtv.org/project/linux-media/patch/20241106-uvc-crashrmmod-v6-1-fbf9781c6e83@chromium.org/ > > > > - Create uvc_gpio_cleanup and uvc_gpio_deinit > > > > - Refactor quirk: do not disable irq > > > > - Change define number for MEDIA_ENT_F_GPIO > > > > - Link to v1: https://lore.kernel.org/r/20241031-uvc-subdev-v1-0-a68331cedd72@chromium.org > > > > > > > > --- > > > > Ricardo Ribalda (5): > > > > media: uvcvideo: Factor out gpio functions to its own file > > > > Revert "media: uvcvideo: Allow entity-defined get_info and get_cur" > > > > media: uvcvideo: Create ancillary link for GPIO subdevice > > > > media: v4l2-core: Add new MEDIA_ENT_F_GPIO > > > > media: uvcvideo: Use MEDIA_ENT_F_GPIO for the GPIO entity > > > > > > > > Yunke Cao (1): > > > > media: uvcvideo: Re-implement privacy GPIO as a separate subdevice > > > > > > > > .../userspace-api/media/mediactl/media-types.rst | 4 + > > > > drivers/media/usb/uvc/Makefile | 3 +- > > > > drivers/media/usb/uvc/uvc_ctrl.c | 40 +---- > > > > drivers/media/usb/uvc/uvc_driver.c | 123 +------------- > > > > drivers/media/usb/uvc/uvc_entity.c | 20 ++- > > > > drivers/media/usb/uvc/uvc_gpio.c | 187 +++++++++++++++++++++ > > > > drivers/media/usb/uvc/uvc_video.c | 4 + > > > > drivers/media/usb/uvc/uvcvideo.h | 34 ++-- > > > > drivers/media/v4l2-core/v4l2-async.c | 3 +- > > > > include/uapi/linux/media.h | 1 + > > > > 10 files changed, 252 insertions(+), 167 deletions(-) > > > > --- > > > > base-commit: 4353256f5487e0c5c47e8ff764bf4f9e679fb525 > > > > change-id: 20241030-uvc-subdev-89f4467a00b5 > > -- > Regards, > > Laurent Pinchart
On Sun, 10 Nov 2024 at 16:14, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > On Sat, Nov 09, 2024 at 05:29:54PM +0100, Ricardo Ribalda wrote: > > On Sat, 9 Nov 2024 at 16:37, Hans de Goede <hdegoede@redhat.com> wrote: > > > > > > Hi Ricardo, > > > > > > FYI / some background: I have been asked to start helping / > > > co-maintaining UVC with Laurent. I'll send out a patch adding > > > myself as UVC maintainer soon. > > > > Great! I talked with Laurent yesterday, I hope that we can maintain > > the driver the three of us in the near future. > > > > > On 8-Nov-24 9:25 PM, Ricardo Ribalda wrote: > > > > Some notebooks have a button to disable the camera (not to be mistaken > > > > with the mechanical cover). This is a standard GPIO linked to the > > > > camera via the ACPI table. > > > > > > > > 4 years ago we added support for this button in UVC via the Privacy control. > > > > This has two issues: > > > > - If the camera has its own privacy control, it will be masked > > > > - We need to power-up the camera to read the privacy control gpio. > > > > > > > > We tried to fix the power-up issues implementing "granular power > > > > saving" but it has been more complicated than anticipated.... > > > > > > I have been discussing UVC power-management with Laurent, also > > > related to power-consumption issues caused by libcamera's pipeline > > > handler holding open the /dev/video# node as long as the camera > > > manager object exists. > > > > > > For now we have fixed this with some relatively small changes to > > > libcamera's uvcvideo pipeline handler, but that is really meant > > > as an interim solution and as this privacy control series shows > > > the power-management issues are real. > > > > Indeed, we have tried to fixed it before: > > https://lore.kernel.org/linux-media/20220920-resend-powersave-v5-2-692e6df6c1e2@chromium.org/ > > > > btw this recently landed patch was to work in this direction :) > > https://lore.kernel.org/linux-media/20240926-guenter-mini-v7-1-690441953d4a@chromium.org/ > > > > The more people interested in this problem the better. > > > > > Combined with Mauro's remarks about how this is an userspace ABI break (1) > > > I think we should maybe first take another look at the powermanagement > > > issues in general rather then moving forward with this series. > > > > > > My apologies for this, I realize how annoying it can be when you are > > > working on a patch series to fix a specific issue and a reviewer > > > moves the goal-posts like this. But I do really think that just fixing > > > the generic power-management issues would be better and I also think > > > that this should be feasible / not too hard. > > > > > > Here is what I have in mind for this: > > > > > > 1. Assume that the results of trying a specific fmt do not change over time. > > > > > > 2. Only allow userspace to request fmts which match one of the enum-fmts -> > > > enum-frame-sizes -> enum-frame-rates tripplet results > > > (constrain what userspace requests to these) > > > > > > 3. Run the equivalent of tryfmt on all possible combinations (so the usaul > > > 3 levels nested loop for this) on probe() and cache the results > > > > > > 4. Make try_fmt / set_fmt not poweron the device but instead constrain > > > the requested fmt to one from our cached fmts > > > > > > 5. On stream-on do the actual power-on + set-fmt + verify that we get > > > what we expect based on the cache, and otherwise return -EIO. > > > > Can we start powering up the device during try/set fmt and then > > implement the format caching as an improvement? > > This sounds worth trying. We'll need to test it on a wide range of > devices though, both internal and external. For what is worth, we have been running something similar to https://lore.kernel.org/linux-media/20220920-resend-powersave-v5-2-692e6df6c1e2@chromium.org/ in ChromeOS and it has worked fine.... But I am pretty sure that it has issues with async controls :S > > > Laurent mentioned that some cameras missbehave if a lot of controls > > are set during probing. I hope that this approach does not trigger > > those, and if it does it would be easier to revert if we do the work > > in two steps. > > > > > I think that should sort the issue, assuming that 1. above holds true. > > > > > > One downside is that this stops UVC button presses from working when > > > not streaming. But userspace will typically only open the /dev/video# > > > node if it plans to stream anyways so there should not be much of > > > a difference wrt button press behavior. > > > > I do not personally use the button, but it is currently implemented as > > a standard HID device. Making it only work during streamon() might be > > a bit weird. > > I am afraid that if there is a button we should keep the current behaviour. > > > > > > > > This should also make camera enumeration faster for apps, since > > > most apps / frameworks do the whole 3 levels nested loop for this > > > on startup, for which atm we go out to the hw, which now instead > > > will come from the fmts cache and thus will be much much faster, > > > so this should lead to a noticeable speedup for apps accessing UVC > > > cameras which would be another nice win. > > > > > > Downside is that the initial probe will take longer see we do > > > all the tryfmt-s there now. But I think that taking a bit longer > > > to probe while the machine is booting should not be an issue. > > > > How do you pretend to handle the controls? Do you plan to power-up the > > device during s_ctrl() or set them only during streamon()? > > If we power-up the device during s_ctrl we need to take care of the > > asynchronous controls (typically pan/tilt/zoom), The device must be > > powered until the control finishes, and the device might never reply > > control_done if the firmware is not properly implemented. > > If we set the controls only during streamon, we will break some > > usecases. There are some video conferencing equipment that do homing > > during streamoff. That will be a serious API breakage. > > > > This patchset is not only to fix the powersaving issues, but also to > > fix the issue when a camera has a gpio privacy switch and an internal > > Privacy control. 4 years ago I did not see any camera with Privacy > > control (in 100s of models), now they are common. > > Can we have both changes, gpio subdevice and granular power saving? > > > > > Regards, > > > > > > Hans > > > > > > 1) Which is technically correct, but FWIW I agree with you that I think > > > most userspace consumers will not care > > > > > > > Last year, we proposed a patchset to implement the privacy gpio as a > > > > subdevice https://lore.kernel.org/linux-media/20230111-uvc_privacy_subdev-v1-0-f859ac9a01e3@chromium.org/ > > > > > > > > I think it is a pretty clean solution and makes sense to use a > > > > subdevice for something that is a sub device of the camera :). > > > > > > > > This is an attempt to continue with that approach. > > > > > > > > Tested on gimble: > > > > gimble-rev3 ~ # v4l2-ctl --all -d /dev/v4l-subdev0 > > > > Driver Info: > > > > Driver version : 6.6.56 > > > > Capabilities : 0x00000000 > > > > Media Driver Info: > > > > Driver name : uvcvideo > > > > Model : HP 5M Camera: HP 5M Camera > > > > Serial : 0001 > > > > Bus info : usb-0000:00:14.0-6 > > > > Media version : 6.6.56 > > > > Hardware revision: 0x00009601 (38401) > > > > Driver version : 6.6.56 > > > > Interface Info: > > > > ID : 0x0300001d > > > > Type : V4L Sub-Device > > > > Entity Info: > > > > ID : 0x00000013 (19) > > > > Name : GPIO > > > > Function : Unknown sub-device (00020006) > > > > > > > > Camera Controls > > > > > > > > privacy 0x009a0910 (bool) : default=0 value=0 flags=read-only, volatile > > > > > > > > gimble-rev3 ~ # media-ctl -p > > > > Media controller API version 6.6.56 > > > > > > > > Media device information > > > > ------------------------ > > > > driver uvcvideo > > > > model HP 5M Camera: HP 5M Camera > > > > serial 0001 > > > > bus info usb-0000:00:14.0-6 > > > > hw revision 0x9601 > > > > driver version 6.6.56 > > > > > > > > Device topology > > > > - entity 1: HP 5M Camera: HP 5M Camera (1 pad, 1 link) > > > > type Node subtype V4L flags 1 > > > > device node name /dev/video0 > > > > pad0: Sink > > > > <- "Extension 8":1 [ENABLED,IMMUTABLE] > > > > > > > > - entity 4: HP 5M Camera: HP 5M Camera (0 pad, 0 link) > > > > type Node subtype V4L flags 0 > > > > device node name /dev/video1 > > > > > > > > - entity 8: Extension 8 (2 pads, 2 links, 0 routes) > > > > type V4L2 subdev subtype Unknown flags 0 > > > > pad0: Sink > > > > <- "Extension 4":1 [ENABLED,IMMUTABLE] > > > > pad1: Source > > > > -> "HP 5M Camera: HP 5M Camera":0 [ENABLED,IMMUTABLE] > > > > > > > > - entity 11: Extension 4 (2 pads, 2 links, 0 routes) > > > > type V4L2 subdev subtype Unknown flags 0 > > > > pad0: Sink > > > > <- "Processing 2":1 [ENABLED,IMMUTABLE] > > > > pad1: Source > > > > -> "Extension 8":0 [ENABLED,IMMUTABLE] > > > > > > > > - entity 14: Processing 2 (2 pads, 2 links, 0 routes) > > > > type V4L2 subdev subtype Unknown flags 0 > > > > pad0: Sink > > > > <- "Camera 1":0 [ENABLED,IMMUTABLE] > > > > pad1: Source > > > > -> "Extension 4":0 [ENABLED,IMMUTABLE] > > > > > > > > - entity 17: Camera 1 (1 pad, 1 link, 0 routes) > > > > type V4L2 subdev subtype Sensor flags 0 > > > > pad0: Source > > > > -> "Processing 2":0 [ENABLED,IMMUTABLE] > > > > > > > > - entity 19: GPIO (0 pad, 0 link, 0 routes) > > > > type V4L2 subdev subtype Decoder flags 0 > > > > device node name /dev/v4l-subdev0 > > > > > > > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > > > > --- > > > > Changes in v2: > > > > - Rebase on top of https://patchwork.linuxtv.org/project/linux-media/patch/20241106-uvc-crashrmmod-v6-1-fbf9781c6e83@chromium.org/ > > > > - Create uvc_gpio_cleanup and uvc_gpio_deinit > > > > - Refactor quirk: do not disable irq > > > > - Change define number for MEDIA_ENT_F_GPIO > > > > - Link to v1: https://lore.kernel.org/r/20241031-uvc-subdev-v1-0-a68331cedd72@chromium.org > > > > > > > > --- > > > > Ricardo Ribalda (5): > > > > media: uvcvideo: Factor out gpio functions to its own file > > > > Revert "media: uvcvideo: Allow entity-defined get_info and get_cur" > > > > media: uvcvideo: Create ancillary link for GPIO subdevice > > > > media: v4l2-core: Add new MEDIA_ENT_F_GPIO > > > > media: uvcvideo: Use MEDIA_ENT_F_GPIO for the GPIO entity > > > > > > > > Yunke Cao (1): > > > > media: uvcvideo: Re-implement privacy GPIO as a separate subdevice > > > > > > > > .../userspace-api/media/mediactl/media-types.rst | 4 + > > > > drivers/media/usb/uvc/Makefile | 3 +- > > > > drivers/media/usb/uvc/uvc_ctrl.c | 40 +---- > > > > drivers/media/usb/uvc/uvc_driver.c | 123 +------------- > > > > drivers/media/usb/uvc/uvc_entity.c | 20 ++- > > > > drivers/media/usb/uvc/uvc_gpio.c | 187 +++++++++++++++++++++ > > > > drivers/media/usb/uvc/uvc_video.c | 4 + > > > > drivers/media/usb/uvc/uvcvideo.h | 34 ++-- > > > > drivers/media/v4l2-core/v4l2-async.c | 3 +- > > > > include/uapi/linux/media.h | 1 + > > > > 10 files changed, 252 insertions(+), 167 deletions(-) > > > > --- > > > > base-commit: 4353256f5487e0c5c47e8ff764bf4f9e679fb525 > > > > change-id: 20241030-uvc-subdev-89f4467a00b5 > > -- > Regards, > > Laurent Pinchart
Hi Ricardo, On 9-Nov-24 5:29 PM, Ricardo Ribalda wrote: > Hi Hans > > On Sat, 9 Nov 2024 at 16:37, Hans de Goede <hdegoede@redhat.com> wrote: <snip> Note only replying to the button remark here, to try and disentangle that from the general power-management discussions. >> One downside is that this stops UVC button presses from working when >> not streaming. But userspace will typically only open the /dev/video# >> node if it plans to stream anyways so there should not be much of >> a difference wrt button press behavior. > > I do not personally use the button, but it is currently implemented as > a standard HID device. Making it only work during streamon() might be > a bit weird. > I am afraid that if there is a button we should keep the current behaviour. There are 2 sort of "snapshot" buttons on UVC cameras 1. Snapshot buttons handled through the UVC protocol / USB interface. These require the UVC interface to be powered on and the status interrupt URB to be submitted (uvc_status_start() called). These will only work if the /dev/video# node is open, otherwise the UVC interface is powered down and the status interrupt URB is not submitted. IOW most of the time these already do not work, since most of the time userspace will not have /dev/video# open (otherwise we would have the power-consumption issues this patch-series tries to fix everywhere). IMHO not having these working only when /dev/video# is open and instead only having them working when streaming is a not a big deal since usually userspace will only open /dev/video# to stream anyways (except for udev probing, but that is very short lived and does not help with the button). 2. Snapshot buttons which use a separate standard USB HID interface Since these use a separate USB interface, using the usb-hid driver. These do always work and these are handled completely independent of the UVC driver so it does not matter what we do in the UVC driver. I hope this helps clarify the button situation. Regards, Hans
Hi Ricardo, Et al., On 8-Nov-24 9:25 PM, Ricardo Ribalda wrote: > Some notebooks have a button to disable the camera (not to be mistaken > with the mechanical cover). This is a standard GPIO linked to the > camera via the ACPI table. > > 4 years ago we added support for this button in UVC via the Privacy control. > This has two issues: > - If the camera has its own privacy control, it will be masked > - We need to power-up the camera to read the privacy control gpio. Thinking more about this I think we need to start with looking at the userspace API for privacy controls, define how we want that to look and then go from there. The reason I'm writing this is because due to my work in drivers/platform/x86 (pdx86) on EC / ACPI / WMI drivers for non chromebooks I am aware of at least 4 different methods camera on/off (aka privacy) toggles are being reported to userspace at the moment. Adding a v4l2-ctrl on a subdev instead of directly on /dev/video# would be adding a 5th method which seems highly undesirable. Instead I would like to first focus on fixing these userspace API inconsistencies agreeing on a single API we want to use everywhere going forward. We don't need to fix all drivers at once, but IMHO we should agree on what the API should look like and document that and any future drivers implementing camera privacy control related code then must use the new API. Lets start with the 3 APIs I'm currently aware of: 1. uvcvideo driver exporting V4L2_CID_PRIVACY on /dev/video# uvcvideo seems to be the only user of this CID (i) 2. pdx86 drivers exporting an input evdev with EV_SW, SW_CAMERA_LENS_COVER. This is somewhat of a special case for some Dell laptops with an electro-mechanical shutter operated by the EC. But this is not also used by hp-wmi.c where it does not necessarily indicate the status of a mechanical cover, but also possibly simply disconnecting the camera from the USB bus. 3. pdx86 drivers exporting an input evdev with EV_KEY, KEY_CAMERA_ACCESS_ENABLE, KEY_CAMERA_ACCESS_DISABLE These KEY codes are based on offical the HUTRR72 HID/HUT extension and as such may also be send by USB/I2C/BT HID devices. The only user outside of hid-input.c is the recently added drivers/platform/x86/lenovo-wmi-camera.c driver and I'm wondering if that should not use SW_CAMERA_LENS_COVER instead. I'll ask the driver author about how this 4. pdx86 drivers exporting an input evdev with EV_KEY, KEY_CAMERA. Note this 4th method lacks information on if the camera was enabled or disabled. In many cases this is send to indicate that the EC has either dropped a UVC camera of the bus, or added it to the bus. Ideally we would have some helper checking for internal UVC camera presence and turn this into 2 or 3. TL;DR: it a mess. Circling back to this patch-set, note how 3 of the 4 currently in use variants today use in input evdev. I think that using an input evdev (shared with the snapshot button if present) will give us a nice out for the power-management issue with the V4L2_CID_PRIVACY, while at the same time giving a nice opportunity to standardize on a single userspace API. My proposal would be to standardize on SW_CAMERA_LENS_COVER I realize that the GPIO does not always indicate a lens cover, but the resulting black frames are the same result as if there were a lens cover and looking at: https://support.hp.com/ie-en/document/ish_3960099-3335046-16 and then the second picture when expanding "Locate and use the webcam privacy switch" that does look like it may be an actual cover which reports back its state through a GPIO. The reason why I'm not in favor of using KEY_CAMERA_ACCESS_ENABLE + KEY_CAMERA_ACCESS_DISABLE is that looking at the HUTRR72 it talks about: "Enables programmatic access to camera device" which suggests that it is a request to the OS / desktop- environment to block camera access at the software level, rather then reporting back that a hw-level block is in place. And since these may be used by any HID device we are not of control in how these will be used. Ricardo, what do you think of instead of using a v4l-subdev, using an input evdev (shared with the existing one) reporting SW_CAMERA_LENS_COVER ? The v4l-subdev approach will need userspace changes anyways and if we are going to make userspace changes we might as well use the best API available. One downside of going the evdev route is that it is a bit harder for userspace to map the evdev to a camera: 1. For the various WMI interfaces this already is impossible, and just to show a notification it is not necessary (using an external cam will make things weird though). 2. For UVC cameras mapping the evdev to the /dev/video# node can still be done by looking if they share a parent USB interface. This is e.g. already done in apps like xawtv looking at the PCI parent to pair up /dev/video# for video capture with the ALSA interface exposed for sound by bttv cards. 3. We can maybe do something at the media-controller level to help userspace linking a camera to its evdev node. This would also be helpful for the existing WMI interfaces. Regards, Hans i) With the exception of drivers/media/pci/intel/ivsc/mei_csi.c which has a V4L2_CID_PRIVACY control which always reads 0, so I guess we can / should probably drop that. p.s. I do plan to also get back to you on the actually powermanagement discussion. But only so many hours in a day, so it will probably be a couple of days.
p.s. On 11-Nov-24 1:59 PM, Hans de Goede wrote: > Hi Ricardo, Et al., > > On 8-Nov-24 9:25 PM, Ricardo Ribalda wrote: >> Some notebooks have a button to disable the camera (not to be mistaken >> with the mechanical cover). This is a standard GPIO linked to the >> camera via the ACPI table. >> >> 4 years ago we added support for this button in UVC via the Privacy control. >> This has two issues: >> - If the camera has its own privacy control, it will be masked >> - We need to power-up the camera to read the privacy control gpio. > > Thinking more about this I think we need to start with looking at the userspace > API for privacy controls, define how we want that to look and then go from > there. > > The reason I'm writing this is because due to my work in drivers/platform/x86 > (pdx86) on EC / ACPI / WMI drivers for non chromebooks I am aware of at least > 4 different methods camera on/off (aka privacy) toggles are being reported > to userspace at the moment. Adding a v4l2-ctrl on a subdev instead of directly > on /dev/video# would be adding a 5th method which seems highly undesirable. > > Instead I would like to first focus on fixing these userspace API > inconsistencies agreeing on a single API we want to use everywhere > going forward. We don't need to fix all drivers at once, but IMHO we > should agree on what the API should look like and document that and > any future drivers implementing camera privacy control related code > then must use the new API. > > Lets start with the 3 APIs I'm currently aware of: > > 1. uvcvideo driver exporting V4L2_CID_PRIVACY on /dev/video# > uvcvideo seems to be the only user of this CID (i) > > 2. pdx86 drivers exporting an input evdev with EV_SW, > SW_CAMERA_LENS_COVER. This is somewhat of a special case > for some Dell laptops with an electro-mechanical shutter > operated by the EC. But this is not also used by > hp-wmi.c where it does not necessarily indicate the s/not/now/ > status of a mechanical cover, but also possibly simply > disconnecting the camera from the USB bus. > > 3. pdx86 drivers exporting an input evdev with EV_KEY, > KEY_CAMERA_ACCESS_ENABLE, KEY_CAMERA_ACCESS_DISABLE > These KEY codes are based on offical the HUTRR72 HID/HUT > extension and as such may also be send by USB/I2C/BT HID > devices. > > The only user outside of hid-input.c is the recently added > drivers/platform/x86/lenovo-wmi-camera.c driver and I'm > wondering if that should not use SW_CAMERA_LENS_COVER > instead. I'll ask the driver author about how this I have send an email out about possibly switching this to SW_CAMERA_LENS_COVER : https://lore.kernel.org/platform-driver-x86/5666914c-e8c2-481d-8fdf-aff82865c228@redhat.com/ Regards, Hans > 4. pdx86 drivers exporting an input evdev with EV_KEY, > KEY_CAMERA. Note this 4th method lacks information on if > the camera was enabled or disabled. In many cases this > is send to indicate that the EC has either dropped > a UVC camera of the bus, or added it to the bus. > Ideally we would have some helper checking for internal > UVC camera presence and turn this into 2 or 3. > > TL;DR: it a mess. > > Circling back to this patch-set, note how 3 of the 4 > currently in use variants today use in input evdev. > > I think that using an input evdev (shared with the > snapshot button if present) will give us a nice out for > the power-management issue with the V4L2_CID_PRIVACY, > while at the same time giving a nice opportunity to > standardize on a single userspace API. > > My proposal would be to standardize on SW_CAMERA_LENS_COVER > I realize that the GPIO does not always indicate a lens > cover, but the resulting black frames are the same result > as if there were a lens cover and looking at: > > https://support.hp.com/ie-en/document/ish_3960099-3335046-16 > > and then the second picture when expanding "Locate and use > the webcam privacy switch" that does look like it may be > an actual cover which reports back its state through a GPIO. > > The reason why I'm not in favor of using > KEY_CAMERA_ACCESS_ENABLE + KEY_CAMERA_ACCESS_DISABLE is that > looking at the HUTRR72 it talks about: > "Enables programmatic access to camera device" > which suggests that it is a request to the OS / desktop- > environment to block camera access at the software level, > rather then reporting back that a hw-level block is in place. > > And since these may be used by any HID device we are not of > control in how these will be used. > > Ricardo, what do you think of instead of using a v4l-subdev, > using an input evdev (shared with the existing one) reporting > SW_CAMERA_LENS_COVER ? The v4l-subdev approach will need > userspace changes anyways and if we are going to make userspace > changes we might as well use the best API available. > > One downside of going the evdev route is that it is a bit > harder for userspace to map the evdev to a camera: > > 1. For the various WMI interfaces this already is impossible, > and just to show a notification it is not necessary (using > an external cam will make things weird though). > > 2. For UVC cameras mapping the evdev to the /dev/video# > node can still be done by looking if they share a parent > USB interface. This is e.g. already done in apps like > xawtv looking at the PCI parent to pair up /dev/video# > for video capture with the ALSA interface exposed for > sound by bttv cards. > > 3. We can maybe do something at the media-controller > level to help userspace linking a camera to its evdev node. > This would also be helpful for the existing WMI interfaces. > > Regards, > > Hans > > > > i) With the exception of drivers/media/pci/intel/ivsc/mei_csi.c > which has a V4L2_CID_PRIVACY control which always reads 0, so > I guess we can / should probably drop that. > > > > p.s. > > I do plan to also get back to you on the actually powermanagement > discussion. But only so many hours in a day, so it will probably > be a couple of days. > >
Hi Hans On Mon, 11 Nov 2024 at 13:59, Hans de Goede <hdegoede@redhat.com> wrote: > > Hi Ricardo, Et al., > > On 8-Nov-24 9:25 PM, Ricardo Ribalda wrote: > > Some notebooks have a button to disable the camera (not to be mistaken > > with the mechanical cover). This is a standard GPIO linked to the > > camera via the ACPI table. > > > > 4 years ago we added support for this button in UVC via the Privacy control. > > This has two issues: > > - If the camera has its own privacy control, it will be masked > > - We need to power-up the camera to read the privacy control gpio. > > Thinking more about this I think we need to start with looking at the userspace > API for privacy controls, define how we want that to look and then go from > there. > > The reason I'm writing this is because due to my work in drivers/platform/x86 > (pdx86) on EC / ACPI / WMI drivers for non chromebooks I am aware of at least > 4 different methods camera on/off (aka privacy) toggles are being reported > to userspace at the moment. Adding a v4l2-ctrl on a subdev instead of directly > on /dev/video# would be adding a 5th method which seems highly undesirable. > > Instead I would like to first focus on fixing these userspace API > inconsistencies agreeing on a single API we want to use everywhere > going forward. We don't need to fix all drivers at once, but IMHO we > should agree on what the API should look like and document that and > any future drivers implementing camera privacy control related code > then must use the new API. > > Lets start with the 3 APIs I'm currently aware of: > > 1. uvcvideo driver exporting V4L2_CID_PRIVACY on /dev/video# > uvcvideo seems to be the only user of this CID (i) > > 2. pdx86 drivers exporting an input evdev with EV_SW, > SW_CAMERA_LENS_COVER. This is somewhat of a special case > for some Dell laptops with an electro-mechanical shutter > operated by the EC. But this is not also used by > hp-wmi.c where it does not necessarily indicate the > status of a mechanical cover, but also possibly simply > disconnecting the camera from the USB bus. > > 3. pdx86 drivers exporting an input evdev with EV_KEY, > KEY_CAMERA_ACCESS_ENABLE, KEY_CAMERA_ACCESS_DISABLE > These KEY codes are based on offical the HUTRR72 HID/HUT > extension and as such may also be send by USB/I2C/BT HID > devices. > > The only user outside of hid-input.c is the recently added > drivers/platform/x86/lenovo-wmi-camera.c driver and I'm > wondering if that should not use SW_CAMERA_LENS_COVER > instead. I'll ask the driver author about how this > > 4. pdx86 drivers exporting an input evdev with EV_KEY, > KEY_CAMERA. Note this 4th method lacks information on if > the camera was enabled or disabled. In many cases this > is send to indicate that the EC has either dropped > a UVC camera of the bus, or added it to the bus. > Ideally we would have some helper checking for internal > UVC camera presence and turn this into 2 or 3. > > TL;DR: it a mess. > > Circling back to this patch-set, note how 3 of the 4 > currently in use variants today use in input evdev. > > I think that using an input evdev (shared with the > snapshot button if present) will give us a nice out for > the power-management issue with the V4L2_CID_PRIVACY, > while at the same time giving a nice opportunity to > standardize on a single userspace API. > > My proposal would be to standardize on SW_CAMERA_LENS_COVER > I realize that the GPIO does not always indicate a lens > cover, but the resulting black frames are the same result > as if there were a lens cover and looking at: > > https://support.hp.com/ie-en/document/ish_3960099-3335046-16 > > and then the second picture when expanding "Locate and use > the webcam privacy switch" that does look like it may be > an actual cover which reports back its state through a GPIO. > > The reason why I'm not in favor of using > KEY_CAMERA_ACCESS_ENABLE + KEY_CAMERA_ACCESS_DISABLE is that > looking at the HUTRR72 it talks about: > "Enables programmatic access to camera device" > which suggests that it is a request to the OS / desktop- > environment to block camera access at the software level, > rather then reporting back that a hw-level block is in place. > > And since these may be used by any HID device we are not of > control in how these will be used. > > Ricardo, what do you think of instead of using a v4l-subdev, > using an input evdev (shared with the existing one) reporting > SW_CAMERA_LENS_COVER ? The v4l-subdev approach will need > userspace changes anyways and if we are going to make userspace > changes we might as well use the best API available. I just sent a patchset using SW_CAMERA_LENS_COVER I guess the internal uvc privacy (UVC_CT_PRIVACY_CONTROL) shall NOT be converted to evdev: - If we do so, we cannot differentiate external gpio and internal, for devices that have both - There is no warranty that we will get a uvc_event when the control changes, so we would have to constantly poll the device Regards! > > One downside of going the evdev route is that it is a bit > harder for userspace to map the evdev to a camera: > > 1. For the various WMI interfaces this already is impossible, > and just to show a notification it is not necessary (using > an external cam will make things weird though). > > 2. For UVC cameras mapping the evdev to the /dev/video# > node can still be done by looking if they share a parent > USB interface. This is e.g. already done in apps like > xawtv looking at the PCI parent to pair up /dev/video# > for video capture with the ALSA interface exposed for > sound by bttv cards. > > 3. We can maybe do something at the media-controller > level to help userspace linking a camera to its evdev node. > This would also be helpful for the existing WMI interfaces. > > Regards, > > Hans > > > > i) With the exception of drivers/media/pci/intel/ivsc/mei_csi.c > which has a V4L2_CID_PRIVACY control which always reads 0, so > I guess we can / should probably drop that. > > > > p.s. > > I do plan to also get back to you on the actually powermanagement > discussion. But only so many hours in a day, so it will probably > be a couple of days. > > -- Ricardo Ribalda
Hi Ricardo, On 12-Nov-24 6:31 PM, Ricardo Ribalda wrote: > Hi Hans > > On Mon, 11 Nov 2024 at 13:59, Hans de Goede <hdegoede@redhat.com> wrote: >> >> Hi Ricardo, Et al., >> >> On 8-Nov-24 9:25 PM, Ricardo Ribalda wrote: >>> Some notebooks have a button to disable the camera (not to be mistaken >>> with the mechanical cover). This is a standard GPIO linked to the >>> camera via the ACPI table. >>> >>> 4 years ago we added support for this button in UVC via the Privacy control. >>> This has two issues: >>> - If the camera has its own privacy control, it will be masked >>> - We need to power-up the camera to read the privacy control gpio. >> >> Thinking more about this I think we need to start with looking at the userspace >> API for privacy controls, define how we want that to look and then go from >> there. >> >> The reason I'm writing this is because due to my work in drivers/platform/x86 >> (pdx86) on EC / ACPI / WMI drivers for non chromebooks I am aware of at least >> 4 different methods camera on/off (aka privacy) toggles are being reported >> to userspace at the moment. Adding a v4l2-ctrl on a subdev instead of directly >> on /dev/video# would be adding a 5th method which seems highly undesirable. >> >> Instead I would like to first focus on fixing these userspace API >> inconsistencies agreeing on a single API we want to use everywhere >> going forward. We don't need to fix all drivers at once, but IMHO we >> should agree on what the API should look like and document that and >> any future drivers implementing camera privacy control related code >> then must use the new API. >> >> Lets start with the 3 APIs I'm currently aware of: >> >> 1. uvcvideo driver exporting V4L2_CID_PRIVACY on /dev/video# >> uvcvideo seems to be the only user of this CID (i) >> >> 2. pdx86 drivers exporting an input evdev with EV_SW, >> SW_CAMERA_LENS_COVER. This is somewhat of a special case >> for some Dell laptops with an electro-mechanical shutter >> operated by the EC. But this is not also used by >> hp-wmi.c where it does not necessarily indicate the >> status of a mechanical cover, but also possibly simply >> disconnecting the camera from the USB bus. >> >> 3. pdx86 drivers exporting an input evdev with EV_KEY, >> KEY_CAMERA_ACCESS_ENABLE, KEY_CAMERA_ACCESS_DISABLE >> These KEY codes are based on offical the HUTRR72 HID/HUT >> extension and as such may also be send by USB/I2C/BT HID >> devices. >> >> The only user outside of hid-input.c is the recently added >> drivers/platform/x86/lenovo-wmi-camera.c driver and I'm >> wondering if that should not use SW_CAMERA_LENS_COVER >> instead. I'll ask the driver author about how this >> >> 4. pdx86 drivers exporting an input evdev with EV_KEY, >> KEY_CAMERA. Note this 4th method lacks information on if >> the camera was enabled or disabled. In many cases this >> is send to indicate that the EC has either dropped >> a UVC camera of the bus, or added it to the bus. >> Ideally we would have some helper checking for internal >> UVC camera presence and turn this into 2 or 3. >> >> TL;DR: it a mess. >> >> Circling back to this patch-set, note how 3 of the 4 >> currently in use variants today use in input evdev. >> >> I think that using an input evdev (shared with the >> snapshot button if present) will give us a nice out for >> the power-management issue with the V4L2_CID_PRIVACY, >> while at the same time giving a nice opportunity to >> standardize on a single userspace API. >> >> My proposal would be to standardize on SW_CAMERA_LENS_COVER >> I realize that the GPIO does not always indicate a lens >> cover, but the resulting black frames are the same result >> as if there were a lens cover and looking at: >> >> https://support.hp.com/ie-en/document/ish_3960099-3335046-16 >> >> and then the second picture when expanding "Locate and use >> the webcam privacy switch" that does look like it may be >> an actual cover which reports back its state through a GPIO. >> >> The reason why I'm not in favor of using >> KEY_CAMERA_ACCESS_ENABLE + KEY_CAMERA_ACCESS_DISABLE is that >> looking at the HUTRR72 it talks about: >> "Enables programmatic access to camera device" >> which suggests that it is a request to the OS / desktop- >> environment to block camera access at the software level, >> rather then reporting back that a hw-level block is in place. >> >> And since these may be used by any HID device we are not of >> control in how these will be used. >> >> Ricardo, what do you think of instead of using a v4l-subdev, >> using an input evdev (shared with the existing one) reporting >> SW_CAMERA_LENS_COVER ? The v4l-subdev approach will need >> userspace changes anyways and if we are going to make userspace >> changes we might as well use the best API available. > > I just sent a patchset using SW_CAMERA_LENS_COVER I'm glad that you like my proposal and thank you for immediately implementing it and sending out a v3. I was expecting us to first have a bit more discussion about what the userspace API should look like and what we should do wrt keeping / deprecating V4L2_CID_PRIVACY. But I'm glad that you like the evdev SW_CAMERA_LENS_COVER idea, at least I assume you like it since you went for it for v3 :) I'll reply to your v3 cover-letter to discuss what we should do wrt keeping / deprecating V4L2_CID_PRIVACY. IMHO it would be good to hold of on sending out a v4 until we have hashed out how we want this all to look userspace API wise, otherwise you'll just spend a lot of time doing revisions pursuing a moving target. > I guess the internal uvc privacy (UVC_CT_PRIVACY_CONTROL) shall NOT be > converted to evdev: > - If we do so, we cannot differentiate external gpio and internal, for > devices that have both > - There is no warranty that we will get a uvc_event when the control > changes, so we would have to constantly poll the device These are good questions, lets also discuss this in a thread with the v3 cover-letter as start to keep all discussion in one place. Regards, Hans
Hi Ricardo, On 9-Nov-24 5:29 PM, Ricardo Ribalda wrote: <snip> >> I have been discussing UVC power-management with Laurent, also >> related to power-consumption issues caused by libcamera's pipeline >> handler holding open the /dev/video# node as long as the camera >> manager object exists. <snip> >> Here is what I have in mind for this: >> >> 1. Assume that the results of trying a specific fmt do not change over time. >> >> 2. Only allow userspace to request fmts which match one of the enum-fmts -> >> enum-frame-sizes -> enum-frame-rates tripplet results >> (constrain what userspace requests to these) >> >> 3. Run the equivalent of tryfmt on all possible combinations (so the usaul >> 3 levels nested loop for this) on probe() and cache the results >> >> 4. Make try_fmt / set_fmt not poweron the device but instead constrain >> the requested fmt to one from our cached fmts >> >> 5. On stream-on do the actual power-on + set-fmt + verify that we get >> what we expect based on the cache, and otherwise return -EIO. > > Can we start powering up the device during try/set fmt and then > implement the format caching as an improvement? Yes, actually looking at how complex this is when e.g. also taking controls into account I think that taking small steps is a good idea. I have lately mostly been working on sensor drivers where delaying applying format settings + all controls to stream-on is normal. So that is the mental model I'm applying to uvc here, but that might not be entirely applicable. > Laurent mentioned that some cameras missbehave if a lot of controls > are set during probing. I hope that this approach does not trigger > those, and if it does it would be easier to revert if we do the work > in two steps. Ack, taking small steps sounds like a good plan. <snip> >> This should also make camera enumeration faster for apps, since >> most apps / frameworks do the whole 3 levels nested loop for this >> on startup, for which atm we go out to the hw, which now instead >> will come from the fmts cache and thus will be much much faster, >> so this should lead to a noticeable speedup for apps accessing UVC >> cameras which would be another nice win. >> >> Downside is that the initial probe will take longer see we do >> all the tryfmt-s there now. But I think that taking a bit longer >> to probe while the machine is booting should not be an issue. > > How do you pretend to handle the controls? Do you plan to power-up the > device during s_ctrl() or set them only during streamon()? > If we power-up the device during s_ctrl we need to take care of the > asynchronous controls (typically pan/tilt/zoom), The device must be > powered until the control finishes, and the device might never reply > control_done if the firmware is not properly implemented. > If we set the controls only during streamon, we will break some > usecases. There are some video conferencing equipment that do homing > during streamoff. That will be a serious API breakage. How to handle controls is a good idea. Based on my sensor experience my initial idea was to just cache them all. Basically make set_ctrl succeed but do not actually do anyhing when the camera is not already powered on and then on stream-on call __v4l2_ctrl_handler_setup() to get all current values applied. But as you indicate that will likely not work well with async controls, although we already have this issue when using v4l2-ctl from the cmdline on such a control and that seems to work fine. Just because we allow the USB connection to sleep, does not mean that the camera cannot finish doing applying the async control. But I can see how some cameras might not like this and having 2 different paths for different controls also is undesirable. Combine that with what Laurent said about devices not liking it when you set too much controls in a short time and I do think we need to immediately apply ctrls. I see 2 ways of doing that: 1. Use pm_runtime_set_autosuspend_delay() with a delay of say 1 second and then on set_ctrl do a pm_runtime_get_sync() + pm_runtime_put_autosuspend() giving the camera 1 second to finish applying the async ctrl (which might not be enough for e.g homing) + also avoid doing suspend + resume all the time if multiple ctrls are send 2. Instead of immediately powering on the camera on /dev/video# open track per fh if the camera has been powered on and then on the first set-ctrl, or the first other IOCTL like try/set-fmt which requires the camera to be powered on power it up and then keep it on until the fh is closed, since apps hitting these paths are likely to do more stuff which requires the camera to be powered on. This should avoid apps (like udev rules) just doing a simple get-cap query of powering on the camera, while at the same time preserving the current behavior for apps which want to actually do something with the camera, so the chance of regressions should be small. I guess the time between power-up and sending the first request to the camera will change slightly. But most apps which actually want to do stuff with the camera will likely already do so immediately after opening /dev/video# so the timing change should be negligible. I guess 2. is pretty similar to your proposal to delay power-on to the first set/try-fmt, which I assume also already does something similar wrt controls ? I think that 2. can work nicely and that would be nice to have, even though it does not fix the privacy-control + power-mgmt issue. Regards, Hans
Hi Ricardo, On 10-Nov-24 5:04 PM, Ricardo Ribalda wrote: > On Sun, 10 Nov 2024 at 16:14, Laurent Pinchart > <laurent.pinchart@ideasonboard.com> wrote: <snip> >>> Can we start powering up the device during try/set fmt and then >>> implement the format caching as an improvement? >> >> This sounds worth trying. We'll need to test it on a wide range of >> devices though, both internal and external. Ack, as mentioned in the other mail which I just send I think this is worth trying. > We still need a plan for asynchronous controls. As I mentioned in that other email I think we can do the same there. So basically delay powering up the camera from /dev/video# open till the first moment we actually need to communicate to the camera and track per file-handle if we did a usb_autopm_get_interface() for that file-handle and if yes, then do the put-interface on file-handle close. > And we have to decide if we stop supporting the uvc button (maybe we > can start by moving USB_VIDEO_CLASS_INPUT_EVDEV to staging and see > what happens?) As I mentioned in other threads I do not think that the button only working changing from: "only works when /dev/video# is open" to: "only works when streaming from /dev/video#" (or actually only works when some action on the camera which requires it to be powered-on has been done). is a big deal, since most apps which open /dev/video# for a longer time will almost always do so to actually do something with the camera, at which point the button will work just as before. And for apps which only do a short-lived open of /dev/video# the button does not work with the current code either. TL;DR: IMHO it is fine if the button only works when streaming. Regards, Hans
Hi Ricardo, On 10-Nov-24 5:07 PM, Ricardo Ribalda wrote: > On Sun, 10 Nov 2024 at 16:14, Laurent Pinchart > <laurent.pinchart@ideasonboard.com> wrote: <snip> >>>> Here is what I have in mind for this: >>>> >>>> 1. Assume that the results of trying a specific fmt do not change over time. >>>> >>>> 2. Only allow userspace to request fmts which match one of the enum-fmts -> >>>> enum-frame-sizes -> enum-frame-rates tripplet results >>>> (constrain what userspace requests to these) >>>> >>>> 3. Run the equivalent of tryfmt on all possible combinations (so the usaul >>>> 3 levels nested loop for this) on probe() and cache the results >>>> >>>> 4. Make try_fmt / set_fmt not poweron the device but instead constrain >>>> the requested fmt to one from our cached fmts >>>> >>>> 5. On stream-on do the actual power-on + set-fmt + verify that we get >>>> what we expect based on the cache, and otherwise return -EIO. >>> >>> Can we start powering up the device during try/set fmt and then >>> implement the format caching as an improvement? >> >> This sounds worth trying. We'll need to test it on a wide range of >> devices though, both internal and external. > > For what is worth, we have been running something similar to > https://lore.kernel.org/linux-media/20220920-resend-powersave-v5-2-692e6df6c1e2@chromium.org/ > in ChromeOS and it has worked fine.... > > But I am pretty sure that it has issues with async controls :S Interesting that is actually a lot more aggressive (as in doing a usb_autopm_put_interface() often) then what I expected when you said: "Can we start powering up the device during try/set fmt" As I mentioned in my other emails what I think would worth nicely is delay the initial usb_autopm_get_interface() till we need it and then just leave the camera on till /dev/video# gets closed. That idea is based on dividing apps in 2 groups: 1. Apps just temporarily opening /dev/video# nodes for discovery, where we ideally would not power-up the camera. 2. Apps (could even be the same app) opening /dev/video# for a longer time because it actually want to use the camera at the moment of opening and which close the /dev/video# node when done with the camera. Your code seems to also covers a 3th group of apps: 3. Apps opening /dev/video# for a long time, while not using it all the time. Although it would be nice to also cover those, IMHO those are not well behaved apps and I'm not sure if we need to cover those. Although looking back at the libcamera uvc pipeline handler issue I fixed recently, that was actually a case of 3. Regards, Hans
On Mon, Nov 25, 2024 at 01:25:41PM +0100, Hans de Goede wrote: > Hi Ricardo, > > On 9-Nov-24 5:29 PM, Ricardo Ribalda wrote: > > <snip> > > >> I have been discussing UVC power-management with Laurent, also > >> related to power-consumption issues caused by libcamera's pipeline > >> handler holding open the /dev/video# node as long as the camera > >> manager object exists. > > <snip> > > >> Here is what I have in mind for this: > >> > >> 1. Assume that the results of trying a specific fmt do not change over time. > >> > >> 2. Only allow userspace to request fmts which match one of the enum-fmts -> > >> enum-frame-sizes -> enum-frame-rates tripplet results > >> (constrain what userspace requests to these) > >> > >> 3. Run the equivalent of tryfmt on all possible combinations (so the usaul > >> 3 levels nested loop for this) on probe() and cache the results > >> > >> 4. Make try_fmt / set_fmt not poweron the device but instead constrain > >> the requested fmt to one from our cached fmts > >> > >> 5. On stream-on do the actual power-on + set-fmt + verify that we get > >> what we expect based on the cache, and otherwise return -EIO. > > > > Can we start powering up the device during try/set fmt and then > > implement the format caching as an improvement? > > Yes, actually looking at how complex this is when e.g. also taking > controls into account I think that taking small steps is a good idea. > > I have lately mostly been working on sensor drivers where delaying > applying format settings + all controls to stream-on is normal. > > So that is the mental model I'm applying to uvc here, but that might > not be entirely applicable. > > > Laurent mentioned that some cameras missbehave if a lot of controls > > are set during probing. I hope that this approach does not trigger > > those, and if it does it would be easier to revert if we do the work > > in two steps. > > Ack, taking small steps sounds like a good plan. > > <snip> > > >> This should also make camera enumeration faster for apps, since > >> most apps / frameworks do the whole 3 levels nested loop for this > >> on startup, for which atm we go out to the hw, which now instead > >> will come from the fmts cache and thus will be much much faster, > >> so this should lead to a noticeable speedup for apps accessing UVC > >> cameras which would be another nice win. > >> > >> Downside is that the initial probe will take longer see we do > >> all the tryfmt-s there now. But I think that taking a bit longer > >> to probe while the machine is booting should not be an issue. > > > > How do you pretend to handle the controls? Do you plan to power-up the > > device during s_ctrl() or set them only during streamon()? > > If we power-up the device during s_ctrl we need to take care of the > > asynchronous controls (typically pan/tilt/zoom), The device must be > > powered until the control finishes, and the device might never reply > > control_done if the firmware is not properly implemented. Talking about powering up is a bit misleading here. What this is about is resuming the USB device. The device stays powered when it is suspended, and the value of controls is retained unless the USB device gets reset. The .resume() handler does not restore controls, the .reset_resume() handler does. Some UVC cameras have the USB_QUIRK_RESET_RESUME quirk set, but the majority doesn't. Annoyingly there's a blanket entry for all Logitech UVC devices in drivers/usb/core/quirks.c, which has led the uvcvideo driver to grow a UVC_QUIRK_NO_RESET_RESUME quirk. > > If we set the controls only during streamon, we will break some > > usecases. There are some video conferencing equipment that do homing > > during streamoff. That will be a serious API breakage. > > How to handle controls is a good idea. > > Based on my sensor experience my initial idea was to just cache them > all. Basically make set_ctrl succeed but do not actually do anyhing > when the camera is not already powered on and then on stream-on call > __v4l2_ctrl_handler_setup() to get all current values applied. > > But as you indicate that will likely not work well with async controls, It's not all asynchronous controls, only the ones that have an impact on the device that can be observed without streaming. Mechanical pan/tilt is the main use case. We could handle those controls in a special way if needed. > although we already have this issue when using v4l2-ctl from the cmdline > on such a control and that seems to work fine. Just because we allow > the USB connection to sleep, does not mean that the camera cannot finish > doing applying the async control. > > But I can see how some cameras might not like this and having 2 different > paths for different controls also is undesirable. > > Combine that with what Laurent said about devices not liking it when > you set too much controls in a short time and I do think we need to > immediately apply ctrls. > > I see 2 ways of doing that: > > 1. Use pm_runtime_set_autosuspend_delay() with a delay of say 1 second > and then on set_ctrl do a pm_runtime_get_sync() + > pm_runtime_put_autosuspend() giving the camera 1 second to finish > applying the async ctrl (which might not be enough for e.g homing) + > also avoid doing suspend + resume all the time if multiple ctrls are send > > 2. Instead of immediately powering on the camera on /dev/video# open > track per fh if the camera has been powered on and then on the first > set-ctrl, or the first other IOCTL like try/set-fmt which requires > the camera to be powered on power it up and then keep it on until > the fh is closed, since apps hitting these paths are likely to do > more stuff which requires the camera to be powered on. A mode of operation where a userspace action causes a state change and the only way to change back to the previous state is to close the device often leads to problems. I'd rather not do this unless we have to completely rule out all other options. > This should avoid apps (like udev rules) just doing a simple get-cap > query of powering on the camera, while at the same time preserving > the current behavior for apps which want to actually do something > with the camera, so the chance of regressions should be small. > > I guess the time between power-up and sending the first request to > the camera will change slightly. But most apps which actually want > to do stuff with the camera will likely already do so immediately > after opening /dev/video# so the timing change should be negligible. > > I guess 2. is pretty similar to your proposal to delay power-on > to the first set/try-fmt, which I assume also already does > something similar wrt controls ? > > I think that 2. can work nicely and that would be nice to have, > even though it does not fix the privacy-control + power-mgmt issue.
On Mon, Nov 25, 2024 at 01:31:49PM +0100, Hans de Goede wrote: > Hi Ricardo, > > On 10-Nov-24 5:04 PM, Ricardo Ribalda wrote: > > On Sun, 10 Nov 2024 at 16:14, Laurent Pinchart wrote: > > <snip> > > >>> Can we start powering up the device during try/set fmt and then > >>> implement the format caching as an improvement? > >> > >> This sounds worth trying. We'll need to test it on a wide range of > >> devices though, both internal and external. > > Ack, as mentioned in the other mail which I just send I think > this is worth trying. > > > We still need a plan for asynchronous controls. > > As I mentioned in that other email I think we can do the same there. > > So basically delay powering up the camera from /dev/video# open till > the first moment we actually need to communicate to the camera and > track per file-handle if we did a usb_autopm_get_interface() for > that file-handle and if yes, then do the put-interface on file-handle > close. > > > And we have to decide if we stop supporting the uvc button (maybe we > > can start by moving USB_VIDEO_CLASS_INPUT_EVDEV to staging and see > > what happens?) > > As I mentioned in other threads I do not think that the button > only working changing from: > > "only works when /dev/video# is open" > > to: > > "only works when streaming from /dev/video#" > > (or actually only works when some action on the camera which > requires it to be powered-on has been done). > > is a big deal, since most apps which open /dev/video# for > a longer time will almost always do so to actually do something > with the camera, at which point the button will work just as > before. > > And for apps which only do a short-lived open of /dev/video# > the button does not work with the current code either. > > TL;DR: IMHO it is fine if the button only works when streaming. I'm fine with that, we can reconsider if people complain. It would be painful though, as it could mean reverting everything we'll build related to power management from now on until someone notices the new behaviour, which could easily take a year. The risk is low, but the consequences serious.
On Mon, Nov 25, 2024 at 01:39:05PM +0100, Hans de Goede wrote: > Hi Ricardo, > > On 10-Nov-24 5:07 PM, Ricardo Ribalda wrote: > > On Sun, 10 Nov 2024 at 16:14, Laurent Pinchart > > <laurent.pinchart@ideasonboard.com> wrote: > > <snip> > > >>>> Here is what I have in mind for this: > >>>> > >>>> 1. Assume that the results of trying a specific fmt do not change over time. > >>>> > >>>> 2. Only allow userspace to request fmts which match one of the enum-fmts -> > >>>> enum-frame-sizes -> enum-frame-rates tripplet results > >>>> (constrain what userspace requests to these) > >>>> > >>>> 3. Run the equivalent of tryfmt on all possible combinations (so the usaul > >>>> 3 levels nested loop for this) on probe() and cache the results > >>>> > >>>> 4. Make try_fmt / set_fmt not poweron the device but instead constrain > >>>> the requested fmt to one from our cached fmts > >>>> > >>>> 5. On stream-on do the actual power-on + set-fmt + verify that we get > >>>> what we expect based on the cache, and otherwise return -EIO. > >>> > >>> Can we start powering up the device during try/set fmt and then > >>> implement the format caching as an improvement? > >> > >> This sounds worth trying. We'll need to test it on a wide range of > >> devices though, both internal and external. > > > > For what is worth, we have been running something similar to > > https://lore.kernel.org/linux-media/20220920-resend-powersave-v5-2-692e6df6c1e2@chromium.org/ > > in ChromeOS and it has worked fine.... > > > > But I am pretty sure that it has issues with async controls :S > > Interesting that is actually a lot more aggressive (as in doing a > usb_autopm_put_interface() often) then what I expected when you said: > > "Can we start powering up the device during try/set fmt" > > As I mentioned in my other emails what I think would worth nicely > is delay the initial usb_autopm_get_interface() till we need it > and then just leave the camera on till /dev/video# gets closed. > > That idea is based on dividing apps in 2 groups: > > 1. Apps just temporarily opening /dev/video# nodes for discovery, > where we ideally would not power-up the camera. > > 2. Apps (could even be the same app) opening /dev/video# for > a longer time because it actually want to use the camera > at the moment of opening and which close the /dev/video# node > when done with the camera. > > Your code seems to also covers a 3th group of apps: > > 3. Apps opening /dev/video# for a long time, while not using > it all the time. > > Although it would be nice to also cover those, IMHO those are > not well behaved apps and I'm not sure if we need to cover those. Is that right ? Isn't it better for an application to keep the device open to avoid open delays or even open failures when it wants to use the device ? > Although looking back at the libcamera uvc pipeline handler issue > I fixed recently, that was actually a case of 3.
Hi Laurent, On 25-Nov-24 1:49 PM, Laurent Pinchart wrote: > On Mon, Nov 25, 2024 at 01:25:41PM +0100, Hans de Goede wrote: <snip> >> I see 2 ways of doing that: >> >> 1. Use pm_runtime_set_autosuspend_delay() with a delay of say 1 second >> and then on set_ctrl do a pm_runtime_get_sync() + >> pm_runtime_put_autosuspend() giving the camera 1 second to finish >> applying the async ctrl (which might not be enough for e.g homing) + >> also avoid doing suspend + resume all the time if multiple ctrls are send >> >> 2. Instead of immediately powering on the camera on /dev/video# open >> track per fh if the camera has been powered on and then on the first >> set-ctrl, or the first other IOCTL like try/set-fmt which requires >> the camera to be powered on power it up and then keep it on until >> the fh is closed, since apps hitting these paths are likely to do >> more stuff which requires the camera to be powered on. > > A mode of operation where a userspace action causes a state change and > the only way to change back to the previous state is to close the device > often leads to problems. I'd rather not do this unless we have to > completely rule out all other options. But we already have that today. We already do the usb_autopm_get_interface() as soon as /dev/video# gets opened and the only way to undo it is to close /dev/video#. What I'm suggesting is to no longer do the usb_autopm_get_interface() on all opens, but only on some. Where "some" are the ones where we come to the conclusion that we actually need to power-up the USB-bus / interface because we want to talk to the device. IOW delay the usb_autopm_get_interface() until the first action which actually requires it. This should be a very minimal change from the pov of USB interactions with the actual device, so a small change of regressions while at least not powering on the device during udev discovery. I guess one could argue that the cases where this is a win are so small that this is not worth it. Regards, Hans
Hi, On 25-Nov-24 1:56 PM, Laurent Pinchart wrote: > On Mon, Nov 25, 2024 at 01:31:49PM +0100, Hans de Goede wrote: >> Hi Ricardo, >> >> On 10-Nov-24 5:04 PM, Ricardo Ribalda wrote: >>> On Sun, 10 Nov 2024 at 16:14, Laurent Pinchart wrote: >> >> <snip> >> >>>>> Can we start powering up the device during try/set fmt and then >>>>> implement the format caching as an improvement? >>>> >>>> This sounds worth trying. We'll need to test it on a wide range of >>>> devices though, both internal and external. >> >> Ack, as mentioned in the other mail which I just send I think >> this is worth trying. >> >>> We still need a plan for asynchronous controls. >> >> As I mentioned in that other email I think we can do the same there. >> >> So basically delay powering up the camera from /dev/video# open till >> the first moment we actually need to communicate to the camera and >> track per file-handle if we did a usb_autopm_get_interface() for >> that file-handle and if yes, then do the put-interface on file-handle >> close. >> >>> And we have to decide if we stop supporting the uvc button (maybe we >>> can start by moving USB_VIDEO_CLASS_INPUT_EVDEV to staging and see >>> what happens?) >> >> As I mentioned in other threads I do not think that the button >> only working changing from: >> >> "only works when /dev/video# is open" >> >> to: >> >> "only works when streaming from /dev/video#" >> >> (or actually only works when some action on the camera which >> requires it to be powered-on has been done). >> >> is a big deal, since most apps which open /dev/video# for >> a longer time will almost always do so to actually do something >> with the camera, at which point the button will work just as >> before. >> >> And for apps which only do a short-lived open of /dev/video# >> the button does not work with the current code either. >> >> TL;DR: IMHO it is fine if the button only works when streaming. > > I'm fine with that, we can reconsider if people complain. It would be > painful though, as it could mean reverting everything we'll build > related to power management from now on until someone notices the new > behaviour, which could easily take a year. The risk is low, but the > consequences serious. I think that if some users complain we can just add a default off module option to restore the old behavior for use-cases which somehow depend on that. Doing an extra usb_autopm_get_interface() + usb_autopm_put_interface() at open/close() time if the option is set is easy, and that will just render out other get() and put() calls into no-ops. So we always have that route as an escape-hatch. Regards, Hans
On Mon, 25 Nov 2024 at 13:25, Hans de Goede <hdegoede@redhat.com> wrote: > > Hi Ricardo, > > On 9-Nov-24 5:29 PM, Ricardo Ribalda wrote: > > <snip> > > >> I have been discussing UVC power-management with Laurent, also > >> related to power-consumption issues caused by libcamera's pipeline > >> handler holding open the /dev/video# node as long as the camera > >> manager object exists. > > <snip> > > >> Here is what I have in mind for this: > >> > >> 1. Assume that the results of trying a specific fmt do not change over time. > >> > >> 2. Only allow userspace to request fmts which match one of the enum-fmts -> > >> enum-frame-sizes -> enum-frame-rates tripplet results > >> (constrain what userspace requests to these) > >> > >> 3. Run the equivalent of tryfmt on all possible combinations (so the usaul > >> 3 levels nested loop for this) on probe() and cache the results > >> > >> 4. Make try_fmt / set_fmt not poweron the device but instead constrain > >> the requested fmt to one from our cached fmts > >> > >> 5. On stream-on do the actual power-on + set-fmt + verify that we get > >> what we expect based on the cache, and otherwise return -EIO. > > > > Can we start powering up the device during try/set fmt and then > > implement the format caching as an improvement? > > Yes, actually looking at how complex this is when e.g. also taking > controls into account I think that taking small steps is a good idea. > > I have lately mostly been working on sensor drivers where delaying > applying format settings + all controls to stream-on is normal. > > So that is the mental model I'm applying to uvc here, but that might > not be entirely applicable. > > > Laurent mentioned that some cameras missbehave if a lot of controls > > are set during probing. I hope that this approach does not trigger > > those, and if it does it would be easier to revert if we do the work > > in two steps. > > Ack, taking small steps sounds like a good plan. > > <snip> > > >> This should also make camera enumeration faster for apps, since > >> most apps / frameworks do the whole 3 levels nested loop for this > >> on startup, for which atm we go out to the hw, which now instead > >> will come from the fmts cache and thus will be much much faster, > >> so this should lead to a noticeable speedup for apps accessing UVC > >> cameras which would be another nice win. > >> > >> Downside is that the initial probe will take longer see we do > >> all the tryfmt-s there now. But I think that taking a bit longer > >> to probe while the machine is booting should not be an issue. > > > > How do you pretend to handle the controls? Do you plan to power-up the > > device during s_ctrl() or set them only during streamon()? > > If we power-up the device during s_ctrl we need to take care of the > > asynchronous controls (typically pan/tilt/zoom), The device must be > > powered until the control finishes, and the device might never reply > > control_done if the firmware is not properly implemented. > > If we set the controls only during streamon, we will break some > > usecases. There are some video conferencing equipment that do homing > > during streamoff. That will be a serious API breakage. > > How to handle controls is a good idea. > > Based on my sensor experience my initial idea was to just cache them > all. Basically make set_ctrl succeed but do not actually do anyhing > when the camera is not already powered on and then on stream-on call > __v4l2_ctrl_handler_setup() to get all current values applied. > > But as you indicate that will likely not work well with async controls, > although we already have this issue when using v4l2-ctl from the cmdline > on such a control and that seems to work fine. ----- > Just because we allow > the USB connection to sleep, does not mean that the camera cannot finish > doing applying the async control. > Not sure what you mean with this sentence. Could you explain it differently? Sorry > But I can see how some cameras might not like this and having 2 different > paths for different controls also is undesirable. > > Combine that with what Laurent said about devices not liking it when > you set too much controls in a short time and I do think we need to > immediately apply ctrls. > > I see 2 ways of doing that: > > 1. Use pm_runtime_set_autosuspend_delay() with a delay of say 1 second > and then on set_ctrl do a pm_runtime_get_sync() + > pm_runtime_put_autosuspend() giving the camera 1 second to finish > applying the async ctrl (which might not be enough for e.g homing) + > also avoid doing suspend + resume all the time if multiple ctrls are send What about 1.5: during s_ctrl(): usb_autopm_get_interface() if the control is UVC_CTRL_FLAG_ASYNCHRONOUS. usb_autopm_get_interface() set the actual control in the hardware usb_autopm_put_interface() during uvc_ctrl_status_event(): usb_autopm_put_interface() during close(): send all the missing usb_autopm_put_interface() This way: - we do not have an artificial delay that might not work for all the use cases - cameras with noncompliant async controls will have the same PM behaviour as now (will be powered on until close() ) We do the same with the rest of the actions that require hardware access, like: https://lore.kernel.org/linux-media/20220920-resend-powersave-v5-2-692e6df6c1e2@chromium.org/ This way: - Apps that do not need to access the hardware, do not wake it up, and we do not break usecases. Next steps will be: - cache the formats - move the actual set_ctrl to streamon... but if we can do that I would argue than we can move completely to the control framework. > > 2. Instead of immediately powering on the camera on /dev/video# open > track per fh if the camera has been powered on and then on the first > set-ctrl, or the first other IOCTL like try/set-fmt which requires > the camera to be powered on power it up and then keep it on until > the fh is closed, since apps hitting these paths are likely to do > more stuff which requires the camera to be powered on. > > This should avoid apps (like udev rules) just doing a simple get-cap > query of powering on the camera, while at the same time preserving > the current behavior for apps which want to actually do something > with the camera, so the chance of regressions should be small. > > I guess the time between power-up and sending the first request to > the camera will change slightly. But most apps which actually want > to do stuff with the camera will likely already do so immediately > after opening /dev/video# so the timing change should be negligible. > > I guess 2. is pretty similar to your proposal to delay power-on > to the first set/try-fmt, which I assume also already does > something similar wrt controls ? > > I think that 2. can work nicely and that would be nice to have, > even though it does not fix the privacy-control + power-mgmt issue. > > Regards, > > Hans > > >
Hi, On 25-Nov-24 1:58 PM, Laurent Pinchart wrote: > On Mon, Nov 25, 2024 at 01:39:05PM +0100, Hans de Goede wrote: >> Hi Ricardo, >> >> On 10-Nov-24 5:07 PM, Ricardo Ribalda wrote: >>> On Sun, 10 Nov 2024 at 16:14, Laurent Pinchart >>> <laurent.pinchart@ideasonboard.com> wrote: >> >> <snip> >> >>>>>> Here is what I have in mind for this: >>>>>> >>>>>> 1. Assume that the results of trying a specific fmt do not change over time. >>>>>> >>>>>> 2. Only allow userspace to request fmts which match one of the enum-fmts -> >>>>>> enum-frame-sizes -> enum-frame-rates tripplet results >>>>>> (constrain what userspace requests to these) >>>>>> >>>>>> 3. Run the equivalent of tryfmt on all possible combinations (so the usaul >>>>>> 3 levels nested loop for this) on probe() and cache the results >>>>>> >>>>>> 4. Make try_fmt / set_fmt not poweron the device but instead constrain >>>>>> the requested fmt to one from our cached fmts >>>>>> >>>>>> 5. On stream-on do the actual power-on + set-fmt + verify that we get >>>>>> what we expect based on the cache, and otherwise return -EIO. >>>>> >>>>> Can we start powering up the device during try/set fmt and then >>>>> implement the format caching as an improvement? >>>> >>>> This sounds worth trying. We'll need to test it on a wide range of >>>> devices though, both internal and external. >>> >>> For what is worth, we have been running something similar to >>> https://lore.kernel.org/linux-media/20220920-resend-powersave-v5-2-692e6df6c1e2@chromium.org/ >>> in ChromeOS and it has worked fine.... >>> >>> But I am pretty sure that it has issues with async controls :S >> >> Interesting that is actually a lot more aggressive (as in doing a >> usb_autopm_put_interface() often) then what I expected when you said: >> >> "Can we start powering up the device during try/set fmt" >> >> As I mentioned in my other emails what I think would worth nicely >> is delay the initial usb_autopm_get_interface() till we need it >> and then just leave the camera on till /dev/video# gets closed. >> >> That idea is based on dividing apps in 2 groups: >> >> 1. Apps just temporarily opening /dev/video# nodes for discovery, >> where we ideally would not power-up the camera. >> >> 2. Apps (could even be the same app) opening /dev/video# for >> a longer time because it actually want to use the camera >> at the moment of opening and which close the /dev/video# node >> when done with the camera. >> >> Your code seems to also covers a 3th group of apps: >> >> 3. Apps opening /dev/video# for a long time, while not using >> it all the time. >> >> Although it would be nice to also cover those, IMHO those are >> not well behaved apps and I'm not sure if we need to cover those. > > Is that right ? Isn't it better for an application to keep the device > open to avoid open delays or even open failures when it wants to use the > device ? Keeping devices open has advantages and disadvantages. E.g. keeping /dev/input/event# nodes open will also typically lead to e.g. touchscreens staying powered on. Generally speaking it is not unheard of to expect userspace to behave in a certain way for things like this for power-consumption reasons. I guess the question is how far do we want to go inside the uvc driver to avoid userspace needing to close the /dev/video# nodes when unused. Ricardo's patch from here: https://lore.kernel.org/linux-media/20220920-resend-powersave-v5-2-692e6df6c1e2@chromium.org/ goes all the way and if I understand Ricardo correctly this is already in use in ChromeOS ? So we could also go with this. Maybe put it behind a Kconfig option for a while ? AFAICT the only thing which needs to be figured out there is async controls. I think we can simply set a long autosuspend delay on devices with async controls to deal with that ? I have a Logitech QuickCam Orbit (non AF) UVC camera which has pan + tilt controls and AFAICT these work fine with v4l2-ctl which immediately closes the /dev/video# node after the set-ctrl command. But I'm not sure if I have tested this without the camera streaming at the time. Anyways that is at least one camera I can test. Regards, Hans
On Mon, 25 Nov 2024 at 14:44, Hans de Goede <hdegoede@redhat.com> wrote: > > Hi, > > On 25-Nov-24 1:58 PM, Laurent Pinchart wrote: > > On Mon, Nov 25, 2024 at 01:39:05PM +0100, Hans de Goede wrote: > >> Hi Ricardo, > >> > >> On 10-Nov-24 5:07 PM, Ricardo Ribalda wrote: > >>> On Sun, 10 Nov 2024 at 16:14, Laurent Pinchart > >>> <laurent.pinchart@ideasonboard.com> wrote: > >> > >> <snip> > >> > >>>>>> Here is what I have in mind for this: > >>>>>> > >>>>>> 1. Assume that the results of trying a specific fmt do not change over time. > >>>>>> > >>>>>> 2. Only allow userspace to request fmts which match one of the enum-fmts -> > >>>>>> enum-frame-sizes -> enum-frame-rates tripplet results > >>>>>> (constrain what userspace requests to these) > >>>>>> > >>>>>> 3. Run the equivalent of tryfmt on all possible combinations (so the usaul > >>>>>> 3 levels nested loop for this) on probe() and cache the results > >>>>>> > >>>>>> 4. Make try_fmt / set_fmt not poweron the device but instead constrain > >>>>>> the requested fmt to one from our cached fmts > >>>>>> > >>>>>> 5. On stream-on do the actual power-on + set-fmt + verify that we get > >>>>>> what we expect based on the cache, and otherwise return -EIO. > >>>>> > >>>>> Can we start powering up the device during try/set fmt and then > >>>>> implement the format caching as an improvement? > >>>> > >>>> This sounds worth trying. We'll need to test it on a wide range of > >>>> devices though, both internal and external. > >>> > >>> For what is worth, we have been running something similar to > >>> https://lore.kernel.org/linux-media/20220920-resend-powersave-v5-2-692e6df6c1e2@chromium.org/ > >>> in ChromeOS and it has worked fine.... > >>> > >>> But I am pretty sure that it has issues with async controls :S > >> > >> Interesting that is actually a lot more aggressive (as in doing a > >> usb_autopm_put_interface() often) then what I expected when you said: > >> > >> "Can we start powering up the device during try/set fmt" > >> > >> As I mentioned in my other emails what I think would worth nicely > >> is delay the initial usb_autopm_get_interface() till we need it > >> and then just leave the camera on till /dev/video# gets closed. > >> > >> That idea is based on dividing apps in 2 groups: > >> > >> 1. Apps just temporarily opening /dev/video# nodes for discovery, > >> where we ideally would not power-up the camera. > >> > >> 2. Apps (could even be the same app) opening /dev/video# for > >> a longer time because it actually want to use the camera > >> at the moment of opening and which close the /dev/video# node > >> when done with the camera. > >> > >> Your code seems to also covers a 3th group of apps: > >> > >> 3. Apps opening /dev/video# for a long time, while not using > >> it all the time. > >> > >> Although it would be nice to also cover those, IMHO those are > >> not well behaved apps and I'm not sure if we need to cover those. > > > > Is that right ? Isn't it better for an application to keep the device > > open to avoid open delays or even open failures when it wants to use the > > device ? > > Keeping devices open has advantages and disadvantages. E.g. keeping > /dev/input/event# nodes open will also typically lead to e.g. > touchscreens staying powered on. > > Generally speaking it is not unheard of to expect userspace to > behave in a certain way for things like this for power-consumption > reasons. > > I guess the question is how far do we want to go inside the uvc > driver to avoid userspace needing to close the /dev/video# nodes > when unused. > > Ricardo's patch from here: > > https://lore.kernel.org/linux-media/20220920-resend-powersave-v5-2-692e6df6c1e2@chromium.org/ > > goes all the way and if I understand Ricardo correctly this is > already in use in ChromeOS ? It is in use on just some devices. I try to not diverge from upstream if I can. > > So we could also go with this. Maybe put it behind a Kconfig option > for a while ? I can try to work on a new version of the patch, including support for async controls the way I described in my previous email. Let me know what you think. > > AFAICT the only thing which needs to be figured out there is async > controls. > > I think we can simply set a long autosuspend delay on devices with > async controls to deal with that ? > > I have a Logitech QuickCam Orbit (non AF) UVC camera which has > pan + tilt controls and AFAICT these work fine with v4l2-ctl > which immediately closes the /dev/video# node after the set-ctrl > command. But I'm not sure if I have tested this without the camera > streaming at the time. Anyways that is at least one camera I can test. > > Regards, > > Hans > >
Hi Ricardo, On 25-Nov-24 2:39 PM, Ricardo Ribalda wrote: > On Mon, 25 Nov 2024 at 13:25, Hans de Goede <hdegoede@redhat.com> wrote: >> >> Hi Ricardo, >> >> On 9-Nov-24 5:29 PM, Ricardo Ribalda wrote: >> >> <snip> >> >>>> I have been discussing UVC power-management with Laurent, also >>>> related to power-consumption issues caused by libcamera's pipeline >>>> handler holding open the /dev/video# node as long as the camera >>>> manager object exists. >> >> <snip> >> >>>> Here is what I have in mind for this: >>>> >>>> 1. Assume that the results of trying a specific fmt do not change over time. >>>> >>>> 2. Only allow userspace to request fmts which match one of the enum-fmts -> >>>> enum-frame-sizes -> enum-frame-rates tripplet results >>>> (constrain what userspace requests to these) >>>> >>>> 3. Run the equivalent of tryfmt on all possible combinations (so the usaul >>>> 3 levels nested loop for this) on probe() and cache the results >>>> >>>> 4. Make try_fmt / set_fmt not poweron the device but instead constrain >>>> the requested fmt to one from our cached fmts >>>> >>>> 5. On stream-on do the actual power-on + set-fmt + verify that we get >>>> what we expect based on the cache, and otherwise return -EIO. >>> >>> Can we start powering up the device during try/set fmt and then >>> implement the format caching as an improvement? >> >> Yes, actually looking at how complex this is when e.g. also taking >> controls into account I think that taking small steps is a good idea. >> >> I have lately mostly been working on sensor drivers where delaying >> applying format settings + all controls to stream-on is normal. >> >> So that is the mental model I'm applying to uvc here, but that might >> not be entirely applicable. >> >>> Laurent mentioned that some cameras missbehave if a lot of controls >>> are set during probing. I hope that this approach does not trigger >>> those, and if it does it would be easier to revert if we do the work >>> in two steps. >> >> Ack, taking small steps sounds like a good plan. >> >> <snip> >> >>>> This should also make camera enumeration faster for apps, since >>>> most apps / frameworks do the whole 3 levels nested loop for this >>>> on startup, for which atm we go out to the hw, which now instead >>>> will come from the fmts cache and thus will be much much faster, >>>> so this should lead to a noticeable speedup for apps accessing UVC >>>> cameras which would be another nice win. >>>> >>>> Downside is that the initial probe will take longer see we do >>>> all the tryfmt-s there now. But I think that taking a bit longer >>>> to probe while the machine is booting should not be an issue. >>> >>> How do you pretend to handle the controls? Do you plan to power-up the >>> device during s_ctrl() or set them only during streamon()? >>> If we power-up the device during s_ctrl we need to take care of the >>> asynchronous controls (typically pan/tilt/zoom), The device must be >>> powered until the control finishes, and the device might never reply >>> control_done if the firmware is not properly implemented. >>> If we set the controls only during streamon, we will break some >>> usecases. There are some video conferencing equipment that do homing >>> during streamoff. That will be a serious API breakage. >> >> How to handle controls is a good idea. >> >> Based on my sensor experience my initial idea was to just cache them >> all. Basically make set_ctrl succeed but do not actually do anyhing >> when the camera is not already powered on and then on stream-on call >> __v4l2_ctrl_handler_setup() to get all current values applied. >> >> But as you indicate that will likely not work well with async controls, >> although we already have this issue when using v4l2-ctl from the cmdline >> on such a control and that seems to work fine. > > ----- >> Just because we allow >> the USB connection to sleep, does not mean that the camera cannot finish >> doing applying the async control. >> > Not sure what you mean with this sentence. Could you explain it > differently? Sorry > >> But I can see how some cameras might not like this and having 2 different >> paths for different controls also is undesirable. >> >> Combine that with what Laurent said about devices not liking it when >> you set too much controls in a short time and I do think we need to >> immediately apply ctrls. >> >> I see 2 ways of doing that: >> >> 1. Use pm_runtime_set_autosuspend_delay() with a delay of say 1 second >> and then on set_ctrl do a pm_runtime_get_sync() + >> pm_runtime_put_autosuspend() giving the camera 1 second to finish >> applying the async ctrl (which might not be enough for e.g homing) + >> also avoid doing suspend + resume all the time if multiple ctrls are send > > What about 1.5: > > during s_ctrl(): > usb_autopm_get_interface() > if the control is UVC_CTRL_FLAG_ASYNCHRONOUS. > usb_autopm_get_interface() > set the actual control in the hardware > usb_autopm_put_interface() > > during uvc_ctrl_status_event(): > usb_autopm_put_interface() How do we match this to the usb_autopm_get_interface() call ? At a minimum we would need some counter to track pending (not acked through status interrupt urb) async control requests and only do the put() if that counter >= 1 (and then decrease the counter). We don't want to do unbalanced puts here in case of buggy cameras sending unexpected / too many ctrl status events. > during close(): > send all the missing usb_autopm_put_interface() Except for my one remark this is an interesting proposal. Maybe also do a dev_warn() if there are missing usb_autopm_put_interface() calls pending on close() ? > This way: > - we do not have an artificial delay that might not work for all the use cases > - cameras with noncompliant async controls will have the same PM > behaviour as now (will be powered on until close() ) > > We do the same with the rest of the actions that require hardware access, like: > https://lore.kernel.org/linux-media/20220920-resend-powersave-v5-2-692e6df6c1e2@chromium.org/ > > This way: > - Apps that do not need to access the hardware, do not wake it up, and > we do not break usecases. > > Next steps will be: > - cache the formats > - move the actual set_ctrl to streamon... but if we can do that I > would argue than we can move completely to the control framework. Right I had forgotten that the UVC driver does not use the control framework. I think moving to that would be a prerequisite for moving the set_ctrl to stream_on. Regards, Hans
Hi On Mon, 25 Nov 2024 at 15:02, Hans de Goede <hdegoede@redhat.com> wrote: > > Hi Ricardo, > > On 25-Nov-24 2:39 PM, Ricardo Ribalda wrote: > > On Mon, 25 Nov 2024 at 13:25, Hans de Goede <hdegoede@redhat.com> wrote: > >> > >> Hi Ricardo, > >> > >> On 9-Nov-24 5:29 PM, Ricardo Ribalda wrote: > >> > >> <snip> > >> > >>>> I have been discussing UVC power-management with Laurent, also > >>>> related to power-consumption issues caused by libcamera's pipeline > >>>> handler holding open the /dev/video# node as long as the camera > >>>> manager object exists. > >> > >> <snip> > >> > >>>> Here is what I have in mind for this: > >>>> > >>>> 1. Assume that the results of trying a specific fmt do not change over time. > >>>> > >>>> 2. Only allow userspace to request fmts which match one of the enum-fmts -> > >>>> enum-frame-sizes -> enum-frame-rates tripplet results > >>>> (constrain what userspace requests to these) > >>>> > >>>> 3. Run the equivalent of tryfmt on all possible combinations (so the usaul > >>>> 3 levels nested loop for this) on probe() and cache the results > >>>> > >>>> 4. Make try_fmt / set_fmt not poweron the device but instead constrain > >>>> the requested fmt to one from our cached fmts > >>>> > >>>> 5. On stream-on do the actual power-on + set-fmt + verify that we get > >>>> what we expect based on the cache, and otherwise return -EIO. > >>> > >>> Can we start powering up the device during try/set fmt and then > >>> implement the format caching as an improvement? > >> > >> Yes, actually looking at how complex this is when e.g. also taking > >> controls into account I think that taking small steps is a good idea. > >> > >> I have lately mostly been working on sensor drivers where delaying > >> applying format settings + all controls to stream-on is normal. > >> > >> So that is the mental model I'm applying to uvc here, but that might > >> not be entirely applicable. > >> > >>> Laurent mentioned that some cameras missbehave if a lot of controls > >>> are set during probing. I hope that this approach does not trigger > >>> those, and if it does it would be easier to revert if we do the work > >>> in two steps. > >> > >> Ack, taking small steps sounds like a good plan. > >> > >> <snip> > >> > >>>> This should also make camera enumeration faster for apps, since > >>>> most apps / frameworks do the whole 3 levels nested loop for this > >>>> on startup, for which atm we go out to the hw, which now instead > >>>> will come from the fmts cache and thus will be much much faster, > >>>> so this should lead to a noticeable speedup for apps accessing UVC > >>>> cameras which would be another nice win. > >>>> > >>>> Downside is that the initial probe will take longer see we do > >>>> all the tryfmt-s there now. But I think that taking a bit longer > >>>> to probe while the machine is booting should not be an issue. > >>> > >>> How do you pretend to handle the controls? Do you plan to power-up the > >>> device during s_ctrl() or set them only during streamon()? > >>> If we power-up the device during s_ctrl we need to take care of the > >>> asynchronous controls (typically pan/tilt/zoom), The device must be > >>> powered until the control finishes, and the device might never reply > >>> control_done if the firmware is not properly implemented. > >>> If we set the controls only during streamon, we will break some > >>> usecases. There are some video conferencing equipment that do homing > >>> during streamoff. That will be a serious API breakage. > >> > >> How to handle controls is a good idea. > >> > >> Based on my sensor experience my initial idea was to just cache them > >> all. Basically make set_ctrl succeed but do not actually do anyhing > >> when the camera is not already powered on and then on stream-on call > >> __v4l2_ctrl_handler_setup() to get all current values applied. > >> > >> But as you indicate that will likely not work well with async controls, > >> although we already have this issue when using v4l2-ctl from the cmdline > >> on such a control and that seems to work fine. > > > > ----- > >> Just because we allow > >> the USB connection to sleep, does not mean that the camera cannot finish > >> doing applying the async control. > >> > > Not sure what you mean with this sentence. Could you explain it > > differently? Sorry > > > >> But I can see how some cameras might not like this and having 2 different > >> paths for different controls also is undesirable. > >> > >> Combine that with what Laurent said about devices not liking it when > >> you set too much controls in a short time and I do think we need to > >> immediately apply ctrls. > >> > >> I see 2 ways of doing that: > >> > >> 1. Use pm_runtime_set_autosuspend_delay() with a delay of say 1 second > >> and then on set_ctrl do a pm_runtime_get_sync() + > >> pm_runtime_put_autosuspend() giving the camera 1 second to finish > >> applying the async ctrl (which might not be enough for e.g homing) + > >> also avoid doing suspend + resume all the time if multiple ctrls are send > > > > What about 1.5: > > > > during s_ctrl(): > > usb_autopm_get_interface() > > if the control is UVC_CTRL_FLAG_ASYNCHRONOUS. > > usb_autopm_get_interface() > > set the actual control in the hardware > > usb_autopm_put_interface() > > > > during uvc_ctrl_status_event(): > > usb_autopm_put_interface() > > How do we match this to the usb_autopm_get_interface() > call ? At a minimum we would need some counter to > track pending (not acked through status interrupt urb) > async control requests and only do the put() if that > counter >= 1 (and then decrease the counter). > > We don't want to do unbalanced puts here in case of > buggy cameras sending unexpected / too many > ctrl status events. > > > during close(): > > send all the missing usb_autopm_put_interface() > > Except for my one remark this is an interesting > proposal. I have just upload a patchset implementing this. I tried v4l2-compliance and using the camera app. I think it looks promissing Shall we move the discussion there? https://lore.kernel.org/linux-media/20241126-uvc-granpower-ng-v1-0-6312bf26549c@chromium.org/T/#t > > Maybe also do a dev_warn() if there are missing > usb_autopm_put_interface() calls pending on close() ? > > > This way: > > - we do not have an artificial delay that might not work for all the use cases > > - cameras with noncompliant async controls will have the same PM > > behaviour as now (will be powered on until close() ) > > > > We do the same with the rest of the actions that require hardware access, like: > > https://lore.kernel.org/linux-media/20220920-resend-powersave-v5-2-692e6df6c1e2@chromium.org/ > > > > This way: > > - Apps that do not need to access the hardware, do not wake it up, and > > we do not break usecases. > > > > Next steps will be: > > - cache the formats > > - move the actual set_ctrl to streamon... but if we can do that I > > would argue than we can move completely to the control framework. > > Right I had forgotten that the UVC driver does not use the control > framework. I think moving to that would be a prerequisite for moving > the set_ctrl to stream_on. > > Regards, > > Hans > -- Ricardo Ribalda
On Tue, Nov 26, 2024 at 05:22:20PM +0100, Ricardo Ribalda wrote: > On Mon, 25 Nov 2024 at 15:02, Hans de Goede wrote: > > On 25-Nov-24 2:39 PM, Ricardo Ribalda wrote: > > > On Mon, 25 Nov 2024 at 13:25, Hans de Goede wrote: > > >> On 9-Nov-24 5:29 PM, Ricardo Ribalda wrote: > > >> > > >> <snip> > > >> > > >>>> I have been discussing UVC power-management with Laurent, also > > >>>> related to power-consumption issues caused by libcamera's pipeline > > >>>> handler holding open the /dev/video# node as long as the camera > > >>>> manager object exists. > > >> > > >> <snip> > > >> > > >>>> Here is what I have in mind for this: > > >>>> > > >>>> 1. Assume that the results of trying a specific fmt do not change over time. > > >>>> > > >>>> 2. Only allow userspace to request fmts which match one of the enum-fmts -> > > >>>> enum-frame-sizes -> enum-frame-rates tripplet results > > >>>> (constrain what userspace requests to these) > > >>>> > > >>>> 3. Run the equivalent of tryfmt on all possible combinations (so the usaul > > >>>> 3 levels nested loop for this) on probe() and cache the results > > >>>> > > >>>> 4. Make try_fmt / set_fmt not poweron the device but instead constrain > > >>>> the requested fmt to one from our cached fmts > > >>>> > > >>>> 5. On stream-on do the actual power-on + set-fmt + verify that we get > > >>>> what we expect based on the cache, and otherwise return -EIO. > > >>> > > >>> Can we start powering up the device during try/set fmt and then > > >>> implement the format caching as an improvement? > > >> > > >> Yes, actually looking at how complex this is when e.g. also taking > > >> controls into account I think that taking small steps is a good idea. > > >> > > >> I have lately mostly been working on sensor drivers where delaying > > >> applying format settings + all controls to stream-on is normal. > > >> > > >> So that is the mental model I'm applying to uvc here, but that might > > >> not be entirely applicable. > > >> > > >>> Laurent mentioned that some cameras missbehave if a lot of controls > > >>> are set during probing. I hope that this approach does not trigger > > >>> those, and if it does it would be easier to revert if we do the work > > >>> in two steps. > > >> > > >> Ack, taking small steps sounds like a good plan. > > >> > > >> <snip> > > >> > > >>>> This should also make camera enumeration faster for apps, since > > >>>> most apps / frameworks do the whole 3 levels nested loop for this > > >>>> on startup, for which atm we go out to the hw, which now instead > > >>>> will come from the fmts cache and thus will be much much faster, > > >>>> so this should lead to a noticeable speedup for apps accessing UVC > > >>>> cameras which would be another nice win. > > >>>> > > >>>> Downside is that the initial probe will take longer see we do > > >>>> all the tryfmt-s there now. But I think that taking a bit longer > > >>>> to probe while the machine is booting should not be an issue. > > >>> > > >>> How do you pretend to handle the controls? Do you plan to power-up the > > >>> device during s_ctrl() or set them only during streamon()? > > >>> If we power-up the device during s_ctrl we need to take care of the > > >>> asynchronous controls (typically pan/tilt/zoom), The device must be > > >>> powered until the control finishes, and the device might never reply > > >>> control_done if the firmware is not properly implemented. > > >>> If we set the controls only during streamon, we will break some > > >>> usecases. There are some video conferencing equipment that do homing > > >>> during streamoff. That will be a serious API breakage. > > >> > > >> How to handle controls is a good idea. > > >> > > >> Based on my sensor experience my initial idea was to just cache them > > >> all. Basically make set_ctrl succeed but do not actually do anyhing > > >> when the camera is not already powered on and then on stream-on call > > >> __v4l2_ctrl_handler_setup() to get all current values applied. > > >> > > >> But as you indicate that will likely not work well with async controls, > > >> although we already have this issue when using v4l2-ctl from the cmdline > > >> on such a control and that seems to work fine. > > > > > > ----- > > >> Just because we allow > > >> the USB connection to sleep, does not mean that the camera cannot finish > > >> doing applying the async control. > > >> > > > Not sure what you mean with this sentence. Could you explain it > > > differently? Sorry > > > > > >> But I can see how some cameras might not like this and having 2 different > > >> paths for different controls also is undesirable. > > >> > > >> Combine that with what Laurent said about devices not liking it when > > >> you set too much controls in a short time and I do think we need to > > >> immediately apply ctrls. > > >> > > >> I see 2 ways of doing that: > > >> > > >> 1. Use pm_runtime_set_autosuspend_delay() with a delay of say 1 second > > >> and then on set_ctrl do a pm_runtime_get_sync() + > > >> pm_runtime_put_autosuspend() giving the camera 1 second to finish > > >> applying the async ctrl (which might not be enough for e.g homing) + > > >> also avoid doing suspend + resume all the time if multiple ctrls are send > > > > > > What about 1.5: > > > > > > during s_ctrl(): > > > usb_autopm_get_interface() > > > if the control is UVC_CTRL_FLAG_ASYNCHRONOUS. > > > usb_autopm_get_interface() > > > set the actual control in the hardware > > > usb_autopm_put_interface() > > > > > > during uvc_ctrl_status_event(): > > > usb_autopm_put_interface() > > > > How do we match this to the usb_autopm_get_interface() > > call ? At a minimum we would need some counter to > > track pending (not acked through status interrupt urb) > > async control requests and only do the put() if that > > counter >= 1 (and then decrease the counter). > > > > We don't want to do unbalanced puts here in case of > > buggy cameras sending unexpected / too many > > ctrl status events. We would need a counter indeed, which is a big red flag of bad engineering. It will be fragile at best. > > > during close(): > > > send all the missing usb_autopm_put_interface() > > > > Except for my one remark this is an interesting > > proposal. > > I have just upload a patchset implementing this. I tried > v4l2-compliance and using the camera app. > > I think it looks promissing > > Shall we move the discussion there? > > https://lore.kernel.org/linux-media/20241126-uvc-granpower-ng-v1-0-6312bf26549c@chromium.org/T/#t You're sending too many patch series too quickly, even before we can come to an agreement on any item being discussed. Experimenting is helpful, but if we keep moving the discussion from one series to the next, that won't work. Let's keep it here, and focus on one problem at a time, or the end result will be slower merging of the patches. > > Maybe also do a dev_warn() if there are missing > > usb_autopm_put_interface() calls pending on close() ? > > > > > This way: > > > - we do not have an artificial delay that might not work for all the use cases > > > - cameras with noncompliant async controls will have the same PM > > > behaviour as now (will be powered on until close() ) > > > > > > We do the same with the rest of the actions that require hardware access, like: > > > https://lore.kernel.org/linux-media/20220920-resend-powersave-v5-2-692e6df6c1e2@chromium.org/ > > > > > > This way: > > > - Apps that do not need to access the hardware, do not wake it up, and > > > we do not break usecases. > > > > > > Next steps will be: > > > - cache the formats > > > - move the actual set_ctrl to streamon... but if we can do that I > > > would argue than we can move completely to the control framework. > > > > Right I had forgotten that the UVC driver does not use the control > > framework. I think moving to that would be a prerequisite for moving > > the set_ctrl to stream_on.
On Mon, Nov 25, 2024 at 02:29:51PM +0100, Hans de Goede wrote: > On 25-Nov-24 1:49 PM, Laurent Pinchart wrote: > > On Mon, Nov 25, 2024 at 01:25:41PM +0100, Hans de Goede wrote: > > <snip> > > >> I see 2 ways of doing that: > >> > >> 1. Use pm_runtime_set_autosuspend_delay() with a delay of say 1 second > >> and then on set_ctrl do a pm_runtime_get_sync() + > >> pm_runtime_put_autosuspend() giving the camera 1 second to finish > >> applying the async ctrl (which might not be enough for e.g homing) + > >> also avoid doing suspend + resume all the time if multiple ctrls are send > >> > >> 2. Instead of immediately powering on the camera on /dev/video# open > >> track per fh if the camera has been powered on and then on the first > >> set-ctrl, or the first other IOCTL like try/set-fmt which requires > >> the camera to be powered on power it up and then keep it on until > >> the fh is closed, since apps hitting these paths are likely to do > >> more stuff which requires the camera to be powered on. > > > > A mode of operation where a userspace action causes a state change and > > the only way to change back to the previous state is to close the device > > often leads to problems. I'd rather not do this unless we have to > > completely rule out all other options. > > But we already have that today. We already do the usb_autopm_get_interface() > as soon as /dev/video# gets opened and the only way to undo it is to close > /dev/video#. Yes, but close() is the counterpart of open(). Breaking the symmetry is what bothers me, it's not nice from an application point of view. It wouldn't force instance have solved the issue of keeping the device powered when used through libcamera (or anything else that keeps the device node open after using the camera, or just after querying some of its capabilities through TRY_FMT). > What I'm suggesting is to no longer do the usb_autopm_get_interface() > on all opens, but only on some. > > Where "some" are the ones where we come to the conclusion that we actually > need to power-up the USB-bus / interface because we want to talk to > the device. > > IOW delay the usb_autopm_get_interface() until the first action which > actually requires it. > > This should be a very minimal change from the pov of USB interactions > with the actual device, so a small change of regressions while at > least not powering on the device during udev discovery. > > I guess one could argue that the cases where this is a win are so > small that this is not worth it. Is there a significant enough gain through this approach, and are the other approaches impractical ? If so we can consider this.
On Tue, 26 Nov 2024 at 18:18, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > On Tue, Nov 26, 2024 at 05:22:20PM +0100, Ricardo Ribalda wrote: > > On Mon, 25 Nov 2024 at 15:02, Hans de Goede wrote: > > > On 25-Nov-24 2:39 PM, Ricardo Ribalda wrote: > > > > On Mon, 25 Nov 2024 at 13:25, Hans de Goede wrote: > > > >> On 9-Nov-24 5:29 PM, Ricardo Ribalda wrote: > > > >> > > > >> <snip> > > > >> > > > >>>> I have been discussing UVC power-management with Laurent, also > > > >>>> related to power-consumption issues caused by libcamera's pipeline > > > >>>> handler holding open the /dev/video# node as long as the camera > > > >>>> manager object exists. > > > >> > > > >> <snip> > > > >> > > > >>>> Here is what I have in mind for this: > > > >>>> > > > >>>> 1. Assume that the results of trying a specific fmt do not change over time. > > > >>>> > > > >>>> 2. Only allow userspace to request fmts which match one of the enum-fmts -> > > > >>>> enum-frame-sizes -> enum-frame-rates tripplet results > > > >>>> (constrain what userspace requests to these) > > > >>>> > > > >>>> 3. Run the equivalent of tryfmt on all possible combinations (so the usaul > > > >>>> 3 levels nested loop for this) on probe() and cache the results > > > >>>> > > > >>>> 4. Make try_fmt / set_fmt not poweron the device but instead constrain > > > >>>> the requested fmt to one from our cached fmts > > > >>>> > > > >>>> 5. On stream-on do the actual power-on + set-fmt + verify that we get > > > >>>> what we expect based on the cache, and otherwise return -EIO. > > > >>> > > > >>> Can we start powering up the device during try/set fmt and then > > > >>> implement the format caching as an improvement? > > > >> > > > >> Yes, actually looking at how complex this is when e.g. also taking > > > >> controls into account I think that taking small steps is a good idea. > > > >> > > > >> I have lately mostly been working on sensor drivers where delaying > > > >> applying format settings + all controls to stream-on is normal. > > > >> > > > >> So that is the mental model I'm applying to uvc here, but that might > > > >> not be entirely applicable. > > > >> > > > >>> Laurent mentioned that some cameras missbehave if a lot of controls > > > >>> are set during probing. I hope that this approach does not trigger > > > >>> those, and if it does it would be easier to revert if we do the work > > > >>> in two steps. > > > >> > > > >> Ack, taking small steps sounds like a good plan. > > > >> > > > >> <snip> > > > >> > > > >>>> This should also make camera enumeration faster for apps, since > > > >>>> most apps / frameworks do the whole 3 levels nested loop for this > > > >>>> on startup, for which atm we go out to the hw, which now instead > > > >>>> will come from the fmts cache and thus will be much much faster, > > > >>>> so this should lead to a noticeable speedup for apps accessing UVC > > > >>>> cameras which would be another nice win. > > > >>>> > > > >>>> Downside is that the initial probe will take longer see we do > > > >>>> all the tryfmt-s there now. But I think that taking a bit longer > > > >>>> to probe while the machine is booting should not be an issue. > > > >>> > > > >>> How do you pretend to handle the controls? Do you plan to power-up the > > > >>> device during s_ctrl() or set them only during streamon()? > > > >>> If we power-up the device during s_ctrl we need to take care of the > > > >>> asynchronous controls (typically pan/tilt/zoom), The device must be > > > >>> powered until the control finishes, and the device might never reply > > > >>> control_done if the firmware is not properly implemented. > > > >>> If we set the controls only during streamon, we will break some > > > >>> usecases. There are some video conferencing equipment that do homing > > > >>> during streamoff. That will be a serious API breakage. > > > >> > > > >> How to handle controls is a good idea. > > > >> > > > >> Based on my sensor experience my initial idea was to just cache them > > > >> all. Basically make set_ctrl succeed but do not actually do anyhing > > > >> when the camera is not already powered on and then on stream-on call > > > >> __v4l2_ctrl_handler_setup() to get all current values applied. > > > >> > > > >> But as you indicate that will likely not work well with async controls, > > > >> although we already have this issue when using v4l2-ctl from the cmdline > > > >> on such a control and that seems to work fine. > > > > > > > > ----- > > > >> Just because we allow > > > >> the USB connection to sleep, does not mean that the camera cannot finish > > > >> doing applying the async control. > > > >> > > > > Not sure what you mean with this sentence. Could you explain it > > > > differently? Sorry > > > > > > > >> But I can see how some cameras might not like this and having 2 different > > > >> paths for different controls also is undesirable. > > > >> > > > >> Combine that with what Laurent said about devices not liking it when > > > >> you set too much controls in a short time and I do think we need to > > > >> immediately apply ctrls. > > > >> > > > >> I see 2 ways of doing that: > > > >> > > > >> 1. Use pm_runtime_set_autosuspend_delay() with a delay of say 1 second > > > >> and then on set_ctrl do a pm_runtime_get_sync() + > > > >> pm_runtime_put_autosuspend() giving the camera 1 second to finish > > > >> applying the async ctrl (which might not be enough for e.g homing) + > > > >> also avoid doing suspend + resume all the time if multiple ctrls are send > > > > > > > > What about 1.5: > > > > > > > > during s_ctrl(): > > > > usb_autopm_get_interface() > > > > if the control is UVC_CTRL_FLAG_ASYNCHRONOUS. > > > > usb_autopm_get_interface() > > > > set the actual control in the hardware > > > > usb_autopm_put_interface() > > > > > > > > during uvc_ctrl_status_event(): > > > > usb_autopm_put_interface() > > > > > > How do we match this to the usb_autopm_get_interface() > > > call ? At a minimum we would need some counter to > > > track pending (not acked through status interrupt urb) > > > async control requests and only do the put() if that > > > counter >= 1 (and then decrease the counter). > > > > > > We don't want to do unbalanced puts here in case of > > > buggy cameras sending unexpected / too many > > > ctrl status events. > > We would need a counter indeed, which is a big red flag of bad > engineering. It will be fragile at best. > > > > > during close(): > > > > send all the missing usb_autopm_put_interface() > > > > > > Except for my one remark this is an interesting > > > proposal. > > > > I have just upload a patchset implementing this. I tried > > v4l2-compliance and using the camera app. > > > > I think it looks promissing > > > > Shall we move the discussion there? > > > > https://lore.kernel.org/linux-media/20241126-uvc-granpower-ng-v1-0-6312bf26549c@chromium.org/T/#t > > You're sending too many patch series too quickly, even before we can > come to an agreement on any item being discussed. Experimenting is > helpful, but if we keep moving the discussion from one series to the > next, that won't work. Let's keep it here, and focus on one problem at a > time, or the end result will be slower merging of the patches. I'd argue that it is better to discuss power management in a series called "uvcvideo: Implement Granular Power Saving" than in another called "media: uvcvideo: Implement the Privacy GPIO as a subdevice" Those two problems are orthogonal. I also believe that Hans agreed that that approach was worth exploring.... > > > > Maybe also do a dev_warn() if there are missing > > > usb_autopm_put_interface() calls pending on close() ? > > > > > > > This way: > > > > - we do not have an artificial delay that might not work for all the use cases > > > > - cameras with noncompliant async controls will have the same PM > > > > behaviour as now (will be powered on until close() ) > > > > > > > > We do the same with the rest of the actions that require hardware access, like: > > > > https://lore.kernel.org/linux-media/20220920-resend-powersave-v5-2-692e6df6c1e2@chromium.org/ > > > > > > > > This way: > > > > - Apps that do not need to access the hardware, do not wake it up, and > > > > we do not break usecases. > > > > > > > > Next steps will be: > > > > - cache the formats > > > > - move the actual set_ctrl to streamon... but if we can do that I > > > > would argue than we can move completely to the control framework. > > > > > > Right I had forgotten that the UVC driver does not use the control > > > framework. I think moving to that would be a prerequisite for moving > > > the set_ctrl to stream_on. > > -- > Regards, > > Laurent Pinchart
Some notebooks have a button to disable the camera (not to be mistaken with the mechanical cover). This is a standard GPIO linked to the camera via the ACPI table. 4 years ago we added support for this button in UVC via the Privacy control. This has two issues: - If the camera has its own privacy control, it will be masked - We need to power-up the camera to read the privacy control gpio. We tried to fix the power-up issues implementing "granular power saving" but it has been more complicated than anticipated.... Last year, we proposed a patchset to implement the privacy gpio as a subdevice https://lore.kernel.org/linux-media/20230111-uvc_privacy_subdev-v1-0-f859ac9a01e3@chromium.org/ I think it is a pretty clean solution and makes sense to use a subdevice for something that is a sub device of the camera :). This is an attempt to continue with that approach. Tested on gimble: gimble-rev3 ~ # v4l2-ctl --all -d /dev/v4l-subdev0 Driver Info: Driver version : 6.6.56 Capabilities : 0x00000000 Media Driver Info: Driver name : uvcvideo Model : HP 5M Camera: HP 5M Camera Serial : 0001 Bus info : usb-0000:00:14.0-6 Media version : 6.6.56 Hardware revision: 0x00009601 (38401) Driver version : 6.6.56 Interface Info: ID : 0x0300001d Type : V4L Sub-Device Entity Info: ID : 0x00000013 (19) Name : GPIO Function : Unknown sub-device (00020006) Camera Controls privacy 0x009a0910 (bool) : default=0 value=0 flags=read-only, volatile gimble-rev3 ~ # media-ctl -p Media controller API version 6.6.56 Media device information ------------------------ driver uvcvideo model HP 5M Camera: HP 5M Camera serial 0001 bus info usb-0000:00:14.0-6 hw revision 0x9601 driver version 6.6.56 Device topology - entity 1: HP 5M Camera: HP 5M Camera (1 pad, 1 link) type Node subtype V4L flags 1 device node name /dev/video0 pad0: Sink <- "Extension 8":1 [ENABLED,IMMUTABLE] - entity 4: HP 5M Camera: HP 5M Camera (0 pad, 0 link) type Node subtype V4L flags 0 device node name /dev/video1 - entity 8: Extension 8 (2 pads, 2 links, 0 routes) type V4L2 subdev subtype Unknown flags 0 pad0: Sink <- "Extension 4":1 [ENABLED,IMMUTABLE] pad1: Source -> "HP 5M Camera: HP 5M Camera":0 [ENABLED,IMMUTABLE] - entity 11: Extension 4 (2 pads, 2 links, 0 routes) type V4L2 subdev subtype Unknown flags 0 pad0: Sink <- "Processing 2":1 [ENABLED,IMMUTABLE] pad1: Source -> "Extension 8":0 [ENABLED,IMMUTABLE] - entity 14: Processing 2 (2 pads, 2 links, 0 routes) type V4L2 subdev subtype Unknown flags 0 pad0: Sink <- "Camera 1":0 [ENABLED,IMMUTABLE] pad1: Source -> "Extension 4":0 [ENABLED,IMMUTABLE] - entity 17: Camera 1 (1 pad, 1 link, 0 routes) type V4L2 subdev subtype Sensor flags 0 pad0: Source -> "Processing 2":0 [ENABLED,IMMUTABLE] - entity 19: GPIO (0 pad, 0 link, 0 routes) type V4L2 subdev subtype Decoder flags 0 device node name /dev/v4l-subdev0 Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> --- Changes in v2: - Rebase on top of https://patchwork.linuxtv.org/project/linux-media/patch/20241106-uvc-crashrmmod-v6-1-fbf9781c6e83@chromium.org/ - Create uvc_gpio_cleanup and uvc_gpio_deinit - Refactor quirk: do not disable irq - Change define number for MEDIA_ENT_F_GPIO - Link to v1: https://lore.kernel.org/r/20241031-uvc-subdev-v1-0-a68331cedd72@chromium.org --- Ricardo Ribalda (5): media: uvcvideo: Factor out gpio functions to its own file Revert "media: uvcvideo: Allow entity-defined get_info and get_cur" media: uvcvideo: Create ancillary link for GPIO subdevice media: v4l2-core: Add new MEDIA_ENT_F_GPIO media: uvcvideo: Use MEDIA_ENT_F_GPIO for the GPIO entity Yunke Cao (1): media: uvcvideo: Re-implement privacy GPIO as a separate subdevice .../userspace-api/media/mediactl/media-types.rst | 4 + drivers/media/usb/uvc/Makefile | 3 +- drivers/media/usb/uvc/uvc_ctrl.c | 40 +---- drivers/media/usb/uvc/uvc_driver.c | 123 +------------- drivers/media/usb/uvc/uvc_entity.c | 20 ++- drivers/media/usb/uvc/uvc_gpio.c | 187 +++++++++++++++++++++ drivers/media/usb/uvc/uvc_video.c | 4 + drivers/media/usb/uvc/uvcvideo.h | 34 ++-- drivers/media/v4l2-core/v4l2-async.c | 3 +- include/uapi/linux/media.h | 1 + 10 files changed, 252 insertions(+), 167 deletions(-) --- base-commit: 4353256f5487e0c5c47e8ff764bf4f9e679fb525 change-id: 20241030-uvc-subdev-89f4467a00b5 Best regards,