Message-ID: <CANiDSCu9fiAXEer-TROhks+Hn2=bZp2jb_Zm+nvkzW=6yMtcUg@mail.gmail.com>
Date: Sun, 10 Nov 2024 17:01:50 +0100
From: Ricardo Ribalda <ribalda@...omium.org>
To: Mauro Carvalho Chehab <mchehab+huawei@...nel.org>
Cc: Hans de Goede <hdegoede@...hat.com>, 
	Laurent Pinchart <laurent.pinchart@...asonboard.com>, 
	Mauro Carvalho Chehab <mchehab@...nel.org>, Sakari Ailus <sakari.ailus@...ux.intel.com>, 
	linux-kernel@...r.kernel.org, linux-media@...r.kernel.org, 
	Yunke Cao <yunkec@...omium.org>, Hans Verkuil <hverkuil@...all.nl>
Subject: Re: [PATCH v2 0/6] media: uvcvideo: Implement the Privacy GPIO as a subdevice

On Sun, 10 Nov 2024 at 13:46, Mauro Carvalho Chehab
<mchehab+huawei@...nel.org> wrote:
>
> Em Sun, 10 Nov 2024 11:32:16 +0100
> Ricardo Ribalda <ribalda@...omium.org> escreveu:
>
> > Hi Mauro
> >
> > On Sun, 10 Nov 2024 at 11:03, Mauro Carvalho Chehab
> > <mchehab+huawei@...nel.org> wrote:
> > >
> > > Em Sat, 9 Nov 2024 17:29:54 +0100
> > > Ricardo Ribalda <ribalda@...omium.org> escreveu:
> > >
> > > > >
> > > > > I think that should sort the issue, assuming that 1. above holds true.
> > > > >
> > > > > One downside is that this stops UVC button presses from working when
> > > > > not streaming. But userspace will typically only open the /dev/video#
> > > > > node if it plans to stream anyways so there should not be much of
> > > > > a difference wrt button press behavior.
> > > >
> > > > I do not personally use the button, but it is currently implemented as
> > > > a standard HID device.
> > >
> > > IMO, controlling the privacy via evdev is the best approach then. There's
> > > no need for a RW control neither at subdev or at device level. It could
> > > make sense a Read only to allow apps to read, but still it shall be up to
> > > the Kernel to protect the stream if the button is pressed.
> > >
> > > > Making it only work during streamon() might be
> > > > a bit weird.
> > > > I am afraid that if there is a button we should keep the current behaviour.
> > >
> > > Privacy matters only when streaming. IMO the Kernel check for it needs to
> > > be done at DQBUF time and at read() calls, as one can enable/disable the
> > > camera while doing videoconf calls. I do that a lot with app "soft" buttons,
> > > and on devices that physically support cutting the video.
> > >
> > > I don't trust myself privacy soft buttons, specially when handled in userspace,
> > > so what I have are webcam covers (and a small stick glued at a laptop camera
> > > that has a too small sensor for a webcam cover). I only remove the cover/stick
> > > when I want to participate on videoconf with video enabled with the builtin
> > > camera.
> > >
> > > Regards
> >
> > I think we are mixing up concepts here.
> >
> > On one side we have the uvc button. You can see one here
> > https://www.sellpy.dk/item/2Yk1ZULbki?utm_source=google&utm_medium=cpc&utm_campaign=17610409619&gad_source=1&gclid=Cj0KCQiA0MG5BhD1ARIsAEcZtwR9-09ZtTIVNbVknrZCtCd7ezVM8YFw1yQXfs81FWhofg9eW-iBrsIaAopVEALw_wcB
> > That button is not represented as a hid device. We do not know how the
> > user will use this button. They could even use it to start an app when
> > pressed.
>
> Old cameras have a <snapshot> button. Maybe that's the case of the device
> you're pointing, as it looks some non-uvc Logitech cameras I have myself.
>
> > On the other side we have  the privacy gpio. The chassis has a switch
> > that is connected to the camera and to the SOC. You can see one here:
> > https://support.hp.com/ie-en/document/ish_3960099-3335046-16 .We link
> > the camera with a gpio via the acpi table. When the user flips the
> > button, the camera produces black frames and the SOC gets an IRQ.
>
> OK, so the hardware warrants black frames. Sounds a more secure
> implementation.
>
> > The IRQ is used to display a message like "Camera off" and the value of
> > the GPIO can be checked when an app is running to tell the user:
> > "Camera not available, flip the privacy button if you want to use it."
>
> So, it is not really a privacy gpio/control. It is instead a privacy
> notification control.
>
> I would better name it to clearly indicate what it is about.
>
> > Userspace cannot change the value of the gpio. It is read-only,
> > userspace cannot override the privacy switch. The privacy gpio is
> > represented with a control in /dev/videoX This patchset wants to move
> > it to /dev/v4l2-subdevX
>
> Well, if it is really a gpio pin, kernel (and eventually userspace) can force
> it to pullup (or pulldown) state, forcing one of the states. If, instead is
> an output-only pin, kernel/userspace can't control it at all.
>
> > To make things more complicated. Recently some cameras are starting to
> > have their own privacy control without the need of an external gpio.
> > This is also represented as a control in /dev/videoX.
>
> IMO, both privacy notification events shall be reported the same way,
> no matter if they use GPIO, an input pin or something else.

How do we handle devices that have internal privacy and GPIO if we do
not use a subdevice?

>
> > Now that we have these 3 concepts in place:
> >
> > Today a uvc camera is powered up when /dev/videoX is open(), not when
> > it is streaming.
>
> Ideally, the part of the hardware responsible for streaming shall be
> powered on only while streaming. I agree with Hans de Goede: better
> have this fixed before the privacy notification patches.
>
> > This means that if we want to get an event for the
> > privacy gpio we have to powerup the camera, which results in power
> > consumption. This can be fixed by moving the control to a subdevice
> > (technically the gpio is not part of the camera, so it makes sense).
>
> Ok, but as you said, not all cameras implement it as a separate gpio.

For the device that are not a separate gpio you need to powerup the
device to read it.


>
> > If we only powerup the camera during streamon we will break the uvc
> > button, and the async controls.
>
> Why? IMO, it shall use regmap in a way that the register settings
> will be sent to the device only when the camera control hardware is
> powered up. On a complex device, there are likely at least two power
> up hardware: the camera control logic and the streaming logic.
>
> Not sure if both are visible via UVC spec, though.

There are controls that need to be accessed without streaming, like
the ones that do homing (calibrate the motors), report occupancy of a
room.

If we modify the driver behaviour to  read/write controls only during
this streaming we will stop supporting current use cases and
definately break applications.

>
> Thanks,
> Mauro



-- 
Ricardo Ribalda

Powered by blists - more mailing lists