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@xxxxxxxxxx> 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
