Message-ID: <CANiDSCvS1qEfS9oY=R05YhdRQJZmAjDCxVXxfVO4-=v4W1jTDg@mail.gmail.com>
Date: Mon, 18 Nov 2024 17:47:25 +0100
From: Ricardo Ribalda <ribalda@...omium.org>
To: Hans de Goede <hdegoede@...hat.com>
Cc: Laurent Pinchart <laurent.pinchart@...asonboard.com>, 
	Mauro Carvalho Chehab <mchehab@...nel.org>, Sakari Ailus <sakari.ailus@...ux.intel.com>, 
	Armin Wolf <W_Armin@....de>, linux-kernel@...r.kernel.org, linux-media@...r.kernel.org, 
	Yunke Cao <yunkec@...omium.org>, Hans Verkuil <hverkuil@...all.nl>, stable@...r.kernel.org, 
	Sergey Senozhatsky <senozhatsky@...omium.org>
Subject: Re: [PATCH v3 0/8] media: uvcvideo: Implement the Privacy GPIO as a evdev

Hi Hans

On Mon, 18 Nov 2024 at 16:43, Hans de Goede <hdegoede@...hat.com> wrote:
>
> Hi All,
>
> On 15-Nov-24 9:20 AM, Ricardo Ribalda wrote:
> > On Fri, 15 Nov 2024 at 00:06, Laurent Pinchart
> > <laurent.pinchart@...asonboard.com> wrote:
> >>
> >> Hello,
> >>
> >> On Thu, Nov 14, 2024 at 08:21:26PM +0100, Ricardo Ribalda wrote:
> >>> On Wed, 13 Nov 2024 at 18:57, Hans de Goede wrote:
> >>>> On 12-Nov-24 6:30 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 three 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.
> >>>>> - Other drivers have not followed this approach and have used evdev.
> >>>>>
> >>>>> We tried to fix the power-up issues implementing "granular power
> >>>>> saving" but it has been more complicated than anticipated...
> >>>>>
> >>>>> This patchset implements the Privacy GPIO as a evdev.
> >>>>>
> >>>>> The first patch of this set is already in Laurent's tree... but I
> >>>>> include it to get some CI coverage.
> >>>>>
> >>>>> Signed-off-by: Ricardo Ribalda <ribalda@...omium.org>
> >>>>> ---
> >>>>> Changes in v3:
> >>>>> - CodeStyle (Thanks Sakari)
> >>>>> - Re-implement as input device
> >>>>
> >>>> Thank you for your enthusiasm for my suggestion to implement this
> >>>> as an input device.
> >>>
> >>> I wanted to give it a try... and it turned out to be quite simple to
> >>> implement. I thought it could be a good idea to share it, so we can
> >>> have something tangible to talk about ;).
> >>>
> >>>> As I mentioned in my reply in the v2 thread, the goal of my
> >>>> enumeration of various way camera privacy-controls are exposed to
> >>>> userspace today is to try and get everyone to agree on a single
> >>>> userspace API for this.
> >>>>
> >>>> Except for this v3 patch-set, which I take as an implied vote
> >>>> from you (Ricardo) for the evdev SW_CAMERA_LENS_COVER approach,
> >>>> we have not heard anything on this subject from Sakari or Laurent
> >>>> yet. So for now I would like to first focus on / circle back to
> >>>> the userspace API discussion and then once we have a plan for
> >>>> the userspace API we can implement that for uvcvideo.
> >>>>
> >>>> First lets look at the API question top down, iow what use-cases
> >>>> do we expect there to be for information about the camera-privacy
> >>>> switch state:
> >>>>
> >>>> a) Having an app which is using (trying to use) the camera show
> >>>> a notification to the user that the camera is turned-off by
> >>>> a privacy switch .
> >>>>
> >>>> Ricardo, AFAICT this is the main use-case for chrome-os, do I have
> >>>> this right ?
> >>>
> >>> b) is as important as a) for us.  If you do not give instant feedback
> >>> to the user when they change the status of the camera they might not
> >>> be able to find the button later on :)
> >>
> >> How do you handle cameras that suffer from
> >> UVC_QUIRK_PRIVACY_DURING_STREAM ?
> >
> > For those b) does not work.
>
> I already suspected as much, but it is good to have this
> confirmed.
>
> I'm afraid that from a userspace API pov cameras with a GPIO
> which only works when powered-on need to be treated the same as
> cameras which only have UVC_CT_PRIVACY_CONTROL IOW in this case
> keep exporting V4L2_CID_PRIVACY instead of switching to evdev
> with SW_CAMERA_LENS_COVER.
>
> Unfortunately this will make the GPIO handling code in the UVC
> driver somewhat more involved since now we have both uAPI-s for
> GPIOs depending on UVC_QUIRK_PRIVACY_DURING_STREAM.
>
> But I think that this makes sense, this way we end up offering
> 2 uAPIs depending on the hw capabilities:
>
> 1. evdev with SW_CAMERA_LENS_COVER which always reports a reliable
> state + events on the state changing without needing to power-up
> the camera.
>
> 2. V4L2_CID_PRIVACY for the case where the camera needs to be
> powered-on (/dev/video opened) and where the ctrl possibly needs
> to be polled.
>
> Assuming we can all agree on this split based on hw capabilities
> I think that we must document this somewhere in the media subsystem
> documentation. We can then also write down there that
> SW_CAMERA_LENS_COVER only applies to internal cameras.

I do not think that it is worth it to keep UVC_CT_PRIVACY_CONTROL for
the two devices that have connected the GPIO's pull up to the wrong
power rail.
Now that the GPIO can be used from userspace, I expect that those
errors will be found early in the design process and never reach
production stage.


If we use UVC_CT_PRIVACY_CONTROL for thes two devices:
- userspace will have to implement two different APIs
- the driver will have to duplicate the code.
- all that code will be very difficult to test: there are only 2
devices affected and it requires manual intervention to properly test
it.

I think that UVC_QUIRK_PRIVACY_DURING_STREAM is a good compromise and
the main user handles it properly.


>
> >>>> b) Showing on on-screen-display (OSD) with a camera /
> >>>> crossed-out-camera icon when the switch is toggled, similar to how
> >>>> muting speakers/mic show an OSD . Laptop vendor Windows add-on
> >>>> software does this and I know that some users have been asking
> >>>> for this.
> >>>>
> >>>> Then lets look at the question bottom-up which hardware interfaces
> >>>> do we have exposing this information:
> >>>>
> >>>> 1. Internal UVC camera with an input privacy GPIO resource in
> >>>> the ACPI fwnode for the UVC camera, with the GPIO reporting
> >>>> the privacy-switch state. Found on some chrome-books
> >>
> >> Ricardo, is this found only in ACPI-based (x86) chromebooks, or also in
> >> DT-based chromebooks ?
> >
> > I am only aware of ACPI models using this feature today. But there
> > might be DT devices in the future that will use this feature.
> > AFAIK the code should support ACPI and DT.
> >
> >>
> >> Can we assume that the UVC module will not be powered off (and therefore
> >> disappear from USB) when the privacy switch is toggled to disable the
> >> camera ?
> >
> > That is true today, but I cannot be sure that some vendor becomes
> > creative and wire things in a weird way. We usually catch this things
> > early in the process and solve them, but I cannot predict the future
> > (yet :P)
>
> FWIW note that dropping the UVC module of the bus is definitely
> a thing on Windows laptops, but there the camera on/off events
> are handled by the embedded-controller and reported through
> some vendor WMI/ACPI interface rather then being handled by
> the UVC driver.
>
> So not really relevant to the discussion wrt the UVC driver,
> but still good to keep in mind.
>
> >>>> 2. Laptop firmware (EC/ACPI/WMI) which reports privacy-switch
> >>>> state, without a clear 1:1 relation between the reported state and
> >>>> which camera it applies to. In this case sometimes the whole UVC
> >>>> camera module (if it is UVC) is simply dropped of the bus when
> >>>> the camera is disabled through the privacy switch, removing
> >>>> the entire /dev/video# node for the camera. Found on many windows
> >>>> laptops.
> >>>>
> >>>> 3. UVC cameras which report privacy-switch status through
> >>>> a UVC_CT_PRIVACY_CONTROL. Found on ... ?
> >>>
> >>> Some logitech cameras and also internal ones.
> >>>
> >>>> Note this will only work while the camera is streaming and
> >>>> even then may require polling of the ctrl because not all
> >>>> cameras reliably send UVC status messages when it changes.
> >>>> This renders this hardware interface as not usable
> >>
> >> In general I agree, but maybe the situation is better with the UVC
> >> cameras that implement UVC_CT_PRIVACY_CONTROL ?
> >>
> >> Note that, in theory, and as far as I understand, it should be possible
> >> to get the UVC_CT_PRIVACY_CONTROL events when the camera is not
> >> streaming, if the device implement remote wakeup. In practice that's
> >> hardly ever the case, among the ~450 sets of USB descriptors I've
> >> collected over time, only 8 report support for remote wakeup in the
> >> configuration descriptor's bmAttributes field, and I'm not even sure we
> >> could trust those devices to implement this feature correctly.
> >
> > I would bet that they simply copied the descriptor from another
> > project and did not test it.
> >
> >>
> >> Ricardo, do you know if the internal UVC cameras used in chromebooks
> >> that implement UVC_CT_PRIVACY_CONTROL support remote wakeup to notify
> >> changes in the privacy control when the camera is suspended ?
> >
> > Today we only rely on the gpio privacy.
> >
> > Some camera vendors even emulate the control:
> > Instead of having a gpio and a sensor, they look at the frame and if
> > it is very dark, they zero it out completely and set
> > UVC_CT_PRIVACY_CONTROL to 1.
>
> My 2 cents here are that given the wide variety of hardware that
> even if some hw reliably provides status interrupts for
> UVC_CT_PRIVACY_CONTROL we cannot rely on that and we certainly
> cannot rely on remote wakeup being present *and* reliabe.
>
> So I really think that for UVC_CT_PRIVACY_CONTROL we should
> stick with V4L2_CID_PRIVACY.
>
> >>>> Currently there are 2 ways this info is being communicated
> >>>> to userspace, hw-interfaces 1. + 3. are exposed as a v4l2
> >>>> privacy-ctrl where as hw-if 2. uses and input evdev device.
> >>>>
> >>>> The advantage of the v4l2 privacy-ctrl is that it makes it
> >>>> very clear which camera is controlled by the camera
> >>>> privacy-switch.
> >>>>
> >>>> The disadvantage is that it will not work for hw-if 2,
> >>>> because the ACPI / WMI drivers have no v4l2 device to report
> >>>> the control on. We could try to add some magic glue code,
> >>>> but even then with e.g. IPU6 cameras it would still be
> >>>> unclear which v4l2(sub)device we should put the control on
> >>>> and if a UVC camera is just dropped from the bus there is
> >>>> no /dev/video# device at all.
> >>
> >> Is there any ACPI- or WMI-provided information that could assist with
> >> associating a privacy GPIO with a camera ?
> >>
> >>>> Using an input device does not has this disadvantage and
> >>>> it has the advantage of not requiring to power-up the camera
> >>>> as currently happens with a v4l2 ctrl on a UVC camera.
> >>
> >> API-wise, and with the current uvcvideo implementation, I agree. We
> >> could of course also try to improve the uvcvideo driver to not power the
> >> device unless it is streaming (depending on whether or not the known
> >> drawbacks are considered acceptable).
> >>
> >> Devices in the 3rd category will still need to be powered up to report
> >> the status of the privacy control, as well as some devices in the 1st
> >> category (see patch 8/8 in this series that introduces
> >> UVC_QUIRK_PRIVACY_DURING_STREAM).
> >>
> >>>> But using an input device makes it harder to determine
> >>>> which camera the privacy-switch applies to.
> >>
> >> We could include the evdev in the MC graph. That will of course only be
> >> possible if the kernel knows about that association in the first place.
> >> At least the 1st category of devices would benefit from this.
>
> Yes I was thinking about adding a link to the MC graph for this too.
>
> Ricardo I notice that in this v3 series you still create a v4l2-subdev
> for the GPIO handling and then add an ancillary link for the GPIO subdev
> to the mc-graph. But I'm not sure how that is helpful. Userspace would
> still need to do parent matching, but then match the evdev parent to
> the subdev after getting the subdev from the mc. In that case it might
> as well look at the physical (USB-interface) parent of the MC/video
> node and do parent matching on that avoiding the need to go through
> the MC at all.
>
> I think using the MC could still be useful by adding a new type of
> ancillary link to the MC API which provides a file-path as info to
> userspace rather then a mc-link and then just directly provide
> the /dev/input/event# path through this new API?
>
> I guess that extending the MC API like this might be a bit of
> a discussion. But it would already make sense to have this for
> the existing input device for the snapshot button.

The driver creates a v4l2-subdevice for every entity, and the gpio
today is modeled as an entity.
The patchset just adds an ancillary link as Sakari suggested.
I am not against removing the  gpio entity all together if it is not needed.

Now that we are brainstorming here... what about adding a control that
contains the name of the input device (eventX)? Is that a horrible
idea?

>
> >>>> We can specify
> >>>> that SW_CAMERA_LENS_COVER only applies to device internal
> >>>> cameras, but then it is up to userspace to determine which
> >>>> cameras that are.
> >>>
> >>> I am working on wiring up this to userspace right now.. I will report
> >>> back if it cannot do it.
>
> Ricardo, great, thank you!
>
> >>>> Another problem with using an input device is that it will
> >>>> not work for "UVC cameras which report privacy-switch status
> >>>> through a UVC_CT_PRIVACY_CONTROL." since those need the camera
> >>>> on and even then need to be polled to get a reliable reading.
> >>>>
> >>>> Taking this all into account my proposal would be to go
> >>>> with an input device and document that SW_CAMERA_LENS_COVER
> >>>> only applies to device internal cameras.
> >>>>
> >>>> This should work well for both use-cases a) and b) described
> >>>> above and also be easy to support for both hw interfaces
> >>>> 1. and 2.
> >>>>
> >>>> My proposal for hw-if 3. (UVC_CT_PRIVACY_CONTROL) would be
> >>>> to keep reporting this as V4L2_CID_PRIVACY. This means it
> >>>> will not work out of the box for userspace which expects
> >>>> the input device method, but giving the limitations of
> >>>> this hw interface I think that requiring userspace to have
> >>>> to explicitly support this use-case (and e.g. poll the
> >>>> control) is a good thing rather then a bad thing.
> >>>>
> >>>> Still before moving forward with switching the hw-if 1.
> >>>> case to an input device as this patch-series does I would
> >>>> like to hear input from others.
> >>>>
> >>>> Sakari, Laurent, any comments ?
> >>
> >> Assuming the kernel could report the association between an evdev and
> >> camera, we would need to report which evdev SW_CAMERA_LENS_COVER
> >> originates from all the way from the evdev to the consumer of the event.
> >> How well is that supported in standard Linux system architectures ? If
> >> I'm not mistaken libinput will report the originating device, but how
> >> far up the stack is it propagated ? And which component would we expect
> >> to consume those events, should the camera evdev be managed by e.g.
> >> libcamera ?
>
> Good questions. Looking back at our 2 primary use-cases:
>
> a) Having an app which is using (trying to use) the camera show
> a notification to the user that the camera is turned-off by
> a privacy switch .
>
> b) Showing on on-screen-display (OSD) with a camera /
> crossed-out-camera icon when the switch is toggled, similar to how
> muting speakers/mic show an OSD . Laptop vendor Windows add-on
> software does this and I know that some users have been asking
> for this.
>
> I think we have everything to do b) in current compositors
> like gnome-shell. Using an evdev with SW_CAMERA_LENS_COVER
> would even be a lot easier for b) then the current
> V4L2_CID_PRIVACY API.
>
> a) though is a lot harder. We could open up access to
> the relevant /dev/input/event# node using a udev uaccess
> tag so that users who can access /dev/video# nodes also
> get raw access to that /dev/input/event# node and then
> libcamera could indeed provide this information that way.
> I think that is probably the best option.
>
> At least for the cases where the camera on/off switch
> does not simply make the camera completely disappear.
>
> That case is harder. atm that case is not handled at all
> though. So even just getting b) to work for that case
> would be nice / an improvement.
>
> Eventually if we give libcamera access to event#
> nodes which advertise SW_CAMERA_LENS_COVER (and no other
> privacy sensitive information) then libcamera could even
> separately offer some API for apps to just get that value
> if there is no camera to associate it with.
>
> Actually thinking more about it libcamera probably might
> be the right place for some sort of "no cameras found
> have you tried hitting your camera privacy-switch" API.
> That is some API to query if such a message should be
> shown to the user. But that is very much future work.

Are standard apps expected to use libcamera directly or they should
use pipewire?
Maybe a) Should be pipewire's task?

>
> Regards,
>
> Hans
>
>
>


--
Ricardo Ribalda

Powered by blists - more mailing lists