| 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
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,