Hi Mauro On Sat, 9 Nov 2024 at 15:05, Mauro Carvalho Chehab <mchehab+huawei@xxxxxxxxxx> wrote: > > Em Fri, 08 Nov 2024 20:25:44 +0000 > Ricardo Ribalda <ribalda@xxxxxxxxxxxx> 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@xxxxxxxxxxxx/ > > > > 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@xxxxxxxxxxxx> > > --- > > Changes in v2: > > - Rebase on top of https://patchwork.linuxtv.org/project/linux-media/patch/20241106-uvc-crashrmmod-v6-1-fbf9781c6e83@xxxxxxxxxxxx/ > > - 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@xxxxxxxxxxxx > > > > --- > > 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 -- Ricardo Ribalda
