Message-ID: <20241125131428.GD32280@pendragon.ideasonboard.com>
Date: Mon, 25 Nov 2024 15:14:28 +0200
From: Laurent Pinchart <laurent.pinchart@...asonboard.com>
To: Hans de Goede <hdegoede@...hat.com>
Cc: Ricardo Ribalda <ribalda@...omium.org>,
	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

On Mon, Nov 25, 2024 at 01:01:14PM +0100, Hans de Goede wrote:
> On 18-Nov-24 5:47 PM, Ricardo Ribalda wrote:
> > On Mon, 18 Nov 2024 at 16:43, Hans de Goede wrote:
> >> On 15-Nov-24 9:20 AM, Ricardo Ribalda wrote:
> >>> On Fri, 15 Nov 2024 at 00:06, Laurent Pinchart wrote:
> 
> <snip>
> 
> >>>> 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.
> 
> Ok, as you wish. Lets go with using SW_CAMERA_LENS_COVER for the 2 models with
> UVC_QUIRK_PRIVACY_DURING_STREAM too.
> 
> <snip>
> 
> >>>> Is there any ACPI- or WMI-provided information that could assist with
> >>>> associating a privacy GPIO with a camera ?
> 
> I just realized I did not answer this question from Laurent
> in my previous reply.
> 
> No unfortunately there is no ACPI- or WMI-provided information that
> could assist with associating ACPI/WMI camera privacy controls with
> a specific camera. Note that these are typically not exposed as a GPIO,
> but rather as some vendor firmware interface.
> 
> Thinking more about this I'm starting to believe more and more
> that the privacy-control stuff should be handled by libcamera
> and then specifically by the pipeline-handler, with some helper
> code to share functionality where possible.
> 
> E.g. on IPU6 equipped Windows laptops there may be some ACPI/WMI
> driver which provides a /dev/input/event# SW_CAMERA_LENS_COVER node.

Using an event device means that the user would need permissions to
access it. Would distributions be able to tell the device apart from
other event devices such as mouse/keyboard, where a logged user may not
have permission to access all event devices in a multi-seat system ?
Would compositors be able to ignore the device to let libcamera handle
it ?

> So I would expect the IPU6 pipeline-handler to search for such a
> /dev/input/event# node and then expose that to users of the camera
> through a to-be-defined API (I'm thinking a read-only control).
> 
> The code to find the event node can be shared, because this would
> e.g. likely also apply to some IPU3 designs as well as upcoming
> IPU7 designs.
> 
> <snip>
> 
> >>>> 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 don't think we need that. MC can model any type of entity and report
the device major:minor. That plus ancillary links should give us most of
what we need, the only required addition should be a new MC entity
function.

> >> 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.
> 
> Ok I see that explains why the subdevice is there, thank you.
> 
> > 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.
> 
> Right unlike other entities which are really part of the UVC
> specification, the GPIO is not a "real" UVC entity.
> 
> So I wonder if, after switching to SW_CAMERA_LENS_COVER, having
> this as a v4l2-subdevice buys us anything ? If not I think removing
> it might be a good idea.
> 
> As for the ancillary link, that was useful to have when the API
> was a v4l2-ctrl on the subdevice. Just like I doubt if having
> the subdevice at all gives us any added value, I also doubt if
> having the ancillary link gives us any added value.
> 
> > 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?
> 
> I don't know, my initial reaction is that does not feel right to me.
> 
> >>>>>> 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!
> 
> Ricardo, any status update on this ?
> 
> <snip>
> 
> >>>> 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?
> 
> Standard apps are supposed to use pipewire, but IMHO this is
> really too low-level for pipewire to handle itself.
> 
> Also see my remarks above about how I think this needs to
> be part of the pipeline handler. Since e.g. associating
> a /dev/input/event# SW_CAMERA_LENS_COVER node with a specific
> UVC camera is going to be UVC specific solution.
> 
> For other pipeline-handlers combined with vendor fw-interfaces
> offering SW_CAMERA_LENS_COVER support I do not think that there
> is going to be a way to actually associate the 2. So we will
> likely simply have the pipeline handler for e.g. IPU6 simply
> associate any SW_CAMERA_LENS_COVER with the normal (non IR)
> user facing camera.
> 
> Since we need this different ways to map a /dev/input/event#
> SW_CAMERA_LENS_COVER node to a specific camera this really
> needs to be done in libcamera IMHO.
> 
> And I think this also solves the question about needing
> a kernel  API to associate the /dev/input/event# with
> a specific /dev/video#. At least for now I think we don't
> need an API and instead we can simply walk sysfs to find
> the common USB-interface parent to associate the 2.
> 
> See how xawtv associates the alsa and /dev/video# parts
> of tv-grabber cards for an example.

-- 
Regards,

Laurent Pinchart

Powered by blists - more mailing lists