Re: [PATCH v2 0/6] media: uvcvideo: Implement the Privacy GPIO as a subdevice

Hans de Goede
2024-11-09T15:37:16Z

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,