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

[Hans de Goede <hdegoede@redhat.com>]

Hi Ricardo,

On 12-Nov-24 6:31 PM, Ricardo Ribalda wrote:
> Hi Hans
> 
> On Mon, 11 Nov 2024 at 13:59, Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> Hi Ricardo, Et al.,
>>
>> On 8-Nov-24 9:25 PM, Ricardo Ribalda wrote:
>>> Some notebooks have a button to disable the camera (not to be mistaken
>>> with the mechanical cover). This is a standard GPIO linked to the
>>> camera via the ACPI table.
>>>
>>> 4 years ago we added support for this button in UVC via the Privacy control.
>>> This has two issues:
>>> - If the camera has its own privacy control, it will be masked
>>> - We need to power-up the camera to read the privacy control gpio.
>>
>> Thinking more about this I think we need to start with looking at the userspace
>> API for privacy controls, define how we want that to look and then go from
>> there.
>>
>> The reason I'm writing this is because due to my work in drivers/platform/x86
>> (pdx86) on EC / ACPI / WMI drivers for non chromebooks I am aware of at least
>> 4 different methods camera on/off (aka privacy) toggles are being reported
>> to userspace at the moment. Adding a v4l2-ctrl on a subdev instead of directly
>> on /dev/video# would be adding a 5th method which seems highly undesirable.
>>
>> Instead I would like to first focus on fixing these userspace API
>> inconsistencies agreeing on a single API we want to use everywhere
>> going forward. We don't need to fix all drivers at once, but IMHO we
>> should agree on what the API should look like and document that and
>> any future drivers implementing camera privacy control related code
>> then must use the new API.
>>
>> Lets start with the 3 APIs I'm currently aware of:
>>
>> 1. uvcvideo driver exporting V4L2_CID_PRIVACY on /dev/video#
>> uvcvideo seems to be the only user of this CID (i)
>>
>> 2. pdx86 drivers exporting an input evdev with EV_SW,
>> SW_CAMERA_LENS_COVER. This is somewhat of a special case
>> for some Dell laptops with an electro-mechanical shutter
>> operated by the EC. But this is not also used by
>> hp-wmi.c where it does not necessarily indicate the
>> status of a mechanical cover, but also possibly simply
>> disconnecting the camera from the USB bus.
>>
>> 3. pdx86 drivers exporting an input evdev with EV_KEY,
>> KEY_CAMERA_ACCESS_ENABLE, KEY_CAMERA_ACCESS_DISABLE
>> These KEY codes are based on offical the HUTRR72 HID/HUT
>> extension and as such may also be send by USB/I2C/BT HID
>> devices.
>>
>> The only user outside of hid-input.c is the recently added
>> drivers/platform/x86/lenovo-wmi-camera.c driver and I'm
>> wondering if that should not use SW_CAMERA_LENS_COVER
>> instead. I'll ask the driver author about how this
>>
>> 4. pdx86 drivers exporting an input evdev with EV_KEY,
>> KEY_CAMERA. Note this 4th method lacks information on if
>> the camera was enabled or disabled. In many cases this
>> is send to indicate that the EC has either dropped
>> a UVC camera of the bus, or added it to the bus.
>> Ideally we would have some helper checking for internal
>> UVC camera presence and turn this into 2 or 3.
>>
>> TL;DR: it a mess.
>>
>> Circling back to this patch-set, note how 3 of the 4
>> currently in use variants today use in input evdev.
>>
>> I think that using an input evdev (shared with the
>> snapshot button if present) will give us a nice out for
>> the power-management issue with the V4L2_CID_PRIVACY,
>> while at the same time giving a nice opportunity to
>> standardize on a single userspace API.
>>
>> My proposal would be to standardize on SW_CAMERA_LENS_COVER
>> I realize that the GPIO does not always indicate a lens
>> cover, but the resulting black frames are the same result
>> as if there were a lens cover and looking at:
>>
>> https://support.hp.com/ie-en/document/ish_3960099-3335046-16
>>
>> and then the second picture when expanding "Locate and use
>> the webcam privacy switch" that does look like it may be
>> an actual cover which reports back its state through a GPIO.
>>
>> The reason why I'm not in favor of using
>> KEY_CAMERA_ACCESS_ENABLE + KEY_CAMERA_ACCESS_DISABLE is that
>> looking at the HUTRR72 it talks about:
>> "Enables programmatic access to camera device"
>> which suggests that it is a request to the OS / desktop-
>> environment to block camera access at the software level,
>> rather then reporting back that a hw-level block is in place.
>>
>> And since these may be used by any HID device we are not of
>> control in how these will be used.
>>
>> Ricardo, what do you think of instead of using a v4l-subdev,
>> using an input evdev (shared with the existing one) reporting
>> SW_CAMERA_LENS_COVER ?  The v4l-subdev approach will need
>> userspace changes anyways and if we are going to make userspace
>> changes we might as well use the best API available.
> 
> I just sent a patchset using SW_CAMERA_LENS_COVER

I'm glad that you like my proposal and thank you for immediately
implementing it and sending out a v3.

I was expecting us to first have a bit more discussion about
what the userspace API should look like and what we should do
wrt keeping / deprecating V4L2_CID_PRIVACY.

But I'm glad that you like the evdev SW_CAMERA_LENS_COVER idea,
at least I assume you like it since you went for it for v3 :)

I'll reply to your v3 cover-letter to discuss what we should do
wrt keeping / deprecating V4L2_CID_PRIVACY.

IMHO it would be good to hold of on sending out a v4 until we
have hashed out how we want this all to look userspace API wise,
otherwise you'll just spend a lot of time doing revisions
pursuing a moving target.

> I guess the internal uvc privacy (UVC_CT_PRIVACY_CONTROL) shall NOT be
> converted to evdev:
> - If we do so, we cannot differentiate external gpio and internal, for
> devices that have both
> - There is no warranty that we will get a uvc_event when the control
> changes, so we would have to constantly poll the device

These are good questions, lets also discuss this in a thread
with the v3 cover-letter as start to keep all discussion in one place.

Regards,

Hans