Message-ID: <CANiDSCsxkhZ0Dsxv9ZhkOk5JnZmSrFxq+tUoskxfHJDmSnXM5w@mail.gmail.com>
Date: Mon, 25 Nov 2024 16:05:15 +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

On Mon, 25 Nov 2024 at 13:01, Hans de Goede <hdegoede@...hat.com> wrote:
>
> Hi Ricardo,
>
> On 18-Nov-24 5:47 PM, Ricardo Ribalda wrote:
> > 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:
>
> <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.
>
> 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 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 ?

I still have not wired it to ChromeOS. But I do not expect to have any
issues. it is relative simple to go from vdev to evdev and the other
way around

 # ls -la /sys/class/video4linux/video0/device/input/input*/
drwxr-xr-x. 3 root  root     0 Nov 25 06:56 event11

 # ls -la /sys/class/input/event11/device/device/video4linux/
drwxr-xr-x. 3 root root 0 Nov 25 06:56 video0
drwxr-xr-x. 3 root root 0 Nov 25 06:56 video1


>
> <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,
>
> Hans
>
>
>


-- 
Ricardo Ribalda

Powered by blists - more mailing lists