Message-ID: <CANiDSCuRbOEhWi8WtJpJSm5SOjzTRzpk=OTOV_jwbhUQMoXszw@mail.gmail.com>
Date: Sun, 10 Nov 2024 17:04:46 +0100
From: Ricardo Ribalda <ribalda@...omium.org>
To: Laurent Pinchart <laurent.pinchart@...asonboard.com>
Cc: Hans de Goede <hdegoede@...hat.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 16:14, Laurent Pinchart
<laurent.pinchart@...asonboard.com> wrote:
>
> On Sat, Nov 09, 2024 at 05:29:54PM +0100, Ricardo Ribalda wrote:
> > On Sat, 9 Nov 2024 at 16:37, Hans de Goede <hdegoede@...hat.com> wrote:
> > >
> > > Hi Ricardo,
> > >
> > > FYI / some background: I have been asked to start helping /
> > > co-maintaining UVC with Laurent. I'll send out a patch adding
> > > myself as UVC maintainer soon.
> >
> > Great! I talked with Laurent yesterday, I hope that we can maintain
> > the driver the three of us in the near future.
> >
> > > 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.
> > > >
> > > > We tried to fix the power-up issues implementing "granular power
> > > > saving" but it has been more complicated than anticipated....
> > >
> > > I have been discussing UVC power-management with Laurent, also
> > > related to power-consumption issues caused by libcamera's pipeline
> > > handler holding open the /dev/video# node as long as the camera
> > > manager object exists.
> > >
> > > For now we have fixed this with some relatively small changes to
> > > libcamera's uvcvideo pipeline handler, but that is really meant
> > > as an interim solution and as this privacy control series shows
> > > the power-management issues are real.
> >
> > Indeed, we have tried to fixed it before:
> > https://lore.kernel.org/linux-media/20220920-resend-powersave-v5-2-692e6df6c1e2@chromium.org/
> >
> > btw this recently landed patch was to work in this direction :)
> > https://lore.kernel.org/linux-media/20240926-guenter-mini-v7-1-690441953d4a@chromium.org/
> >
> > The more people interested in this problem the better.
> >
> > > Combined with Mauro's remarks about how this is an userspace ABI break (1)
> > > I think we should maybe first take another look at the powermanagement
> > > issues in general rather then moving forward with this series.
> > >
> > > My apologies for this, I realize how annoying it can be when you are
> > > working on a patch series to fix a specific issue and a reviewer
> > > moves the goal-posts like this. But I do really think that just fixing
> > > the generic power-management issues would be better and I also think
> > > that this should be feasible / not too hard.
> > >
> > > Here is what I have in mind for this:
> > >
> > > 1. Assume that the results of trying a specific fmt do not change over time.
> > >
> > > 2. Only allow userspace to request fmts which match one of the enum-fmts ->
> > >    enum-frame-sizes -> enum-frame-rates tripplet results
> > >    (constrain what userspace requests to these)
> > >
> > > 3. Run the equivalent of tryfmt on all possible combinations (so the usaul
> > >    3 levels nested loop for this) on probe() and cache the results
> > >
> > > 4. Make try_fmt / set_fmt not poweron the device but instead constrain
> > >    the requested fmt to one from our cached fmts
> > >
> > > 5. On stream-on do the actual power-on + set-fmt + verify that we get
> > >    what we expect based on the cache, and otherwise return -EIO.
> >
> > Can we start powering up the device during try/set fmt and then
> > implement the format caching as an improvement?
>
> This sounds worth trying. We'll need to test it on a wide range of
> devices though, both internal and external.

We still need a plan for asynchronous controls.

And we have to decide if we stop supporting the uvc button (maybe we
can start by moving USB_VIDEO_CLASS_INPUT_EVDEV to staging and see
what happens?)


>
> > Laurent mentioned that some cameras missbehave if a lot of controls
> > are set during probing. I hope that this approach does not trigger
> > those, and if it does it would be easier to revert if we do the work
> > in two steps.
> >
> > > 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. 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.
> >
> > >
> > > This should also make camera enumeration faster for apps, since
> > > most apps / frameworks do the whole 3 levels nested loop for this
> > > on startup, for which atm we go out to the hw, which now instead
> > > will come from the fmts cache and thus will be much much faster,
> > > so this should lead to a noticeable speedup for apps accessing UVC
> > > cameras which would be another nice win.
> > >
> > > Downside is that the initial probe will take longer see we do
> > > all the tryfmt-s there now. But I think that taking a bit longer
> > > to probe while the machine is booting should not be an issue.
> >
> > How do you pretend to handle the controls? Do you plan to power-up the
> > device during s_ctrl() or set them only during streamon()?
> > If we power-up the device during s_ctrl we need to take care of the
> > asynchronous controls (typically pan/tilt/zoom), The device must be
> > powered until the control finishes, and the device might never reply
> > control_done if the firmware is not properly implemented.
> > If we set the controls only during streamon, we will break some
> > usecases. There are some video conferencing equipment that do homing
> > during streamoff. That will be a serious API breakage.
> >
> > This patchset is not only to fix the powersaving issues, but also to
> > fix the issue when a camera has  a gpio privacy switch and an internal
> > Privacy control.  4 years ago I did not see any camera with Privacy
> > control (in 100s of models), now they are common.
> > Can we have both changes, gpio subdevice and granular power saving?
> >
> > > Regards,
> > >
> > > Hans
> > >
> > > 1) Which is technically correct, but FWIW I agree with you that I think
> > > most userspace consumers will not care
> > >
> > > > Last year, we proposed a patchset to implement the privacy gpio as a
> > > > subdevice https://lore.kernel.org/linux-media/20230111-uvc_privacy_subdev-v1-0-f859ac9a01e3@chromium.org/
> > > >
> > > > I think it is a pretty clean solution and makes sense to use a
> > > > subdevice for something that is a sub device of the camera :).
> > > >
> > > > This is an attempt to continue with that approach.
> > > >
> > > > Tested on gimble:
> > > > gimble-rev3 ~ # v4l2-ctl --all -d /dev/v4l-subdev0
> > > > Driver Info:
> > > >         Driver version   : 6.6.56
> > > >         Capabilities     : 0x00000000
> > > > Media Driver Info:
> > > >         Driver name      : uvcvideo
> > > >         Model            : HP 5M Camera: HP 5M Camera
> > > >         Serial           : 0001
> > > >         Bus info         : usb-0000:00:14.0-6
> > > >         Media version    : 6.6.56
> > > >         Hardware revision: 0x00009601 (38401)
> > > >         Driver version   : 6.6.56
> > > > Interface Info:
> > > >         ID               : 0x0300001d
> > > >         Type             : V4L Sub-Device
> > > > Entity Info:
> > > >         ID               : 0x00000013 (19)
> > > >         Name             : GPIO
> > > >         Function         : Unknown sub-device (00020006)
> > > >
> > > > Camera Controls
> > > >
> > > >                         privacy 0x009a0910 (bool)   : default=0 value=0 flags=read-only, volatile
> > > >
> > > > gimble-rev3 ~ # media-ctl  -p
> > > > Media controller API version 6.6.56
> > > >
> > > > Media device information
> > > > ------------------------
> > > > driver          uvcvideo
> > > > model           HP 5M Camera: HP 5M Camera
> > > > serial          0001
> > > > bus info        usb-0000:00:14.0-6
> > > > hw revision     0x9601
> > > > driver version  6.6.56
> > > >
> > > > Device topology
> > > > - entity 1: HP 5M Camera: HP 5M Camera (1 pad, 1 link)
> > > >             type Node subtype V4L flags 1
> > > >             device node name /dev/video0
> > > >         pad0: Sink
> > > >                 <- "Extension 8":1 [ENABLED,IMMUTABLE]
> > > >
> > > > - entity 4: HP 5M Camera: HP 5M Camera (0 pad, 0 link)
> > > >             type Node subtype V4L flags 0
> > > >             device node name /dev/video1
> > > >
> > > > - entity 8: Extension 8 (2 pads, 2 links, 0 routes)
> > > >             type V4L2 subdev subtype Unknown flags 0
> > > >         pad0: Sink
> > > >                 <- "Extension 4":1 [ENABLED,IMMUTABLE]
> > > >         pad1: Source
> > > >                 -> "HP 5M Camera: HP 5M Camera":0 [ENABLED,IMMUTABLE]
> > > >
> > > > - entity 11: Extension 4 (2 pads, 2 links, 0 routes)
> > > >              type V4L2 subdev subtype Unknown flags 0
> > > >         pad0: Sink
> > > >                 <- "Processing 2":1 [ENABLED,IMMUTABLE]
> > > >         pad1: Source
> > > >                 -> "Extension 8":0 [ENABLED,IMMUTABLE]
> > > >
> > > > - entity 14: Processing 2 (2 pads, 2 links, 0 routes)
> > > >              type V4L2 subdev subtype Unknown flags 0
> > > >         pad0: Sink
> > > >                 <- "Camera 1":0 [ENABLED,IMMUTABLE]
> > > >         pad1: Source
> > > >                 -> "Extension 4":0 [ENABLED,IMMUTABLE]
> > > >
> > > > - entity 17: Camera 1 (1 pad, 1 link, 0 routes)
> > > >              type V4L2 subdev subtype Sensor flags 0
> > > >         pad0: Source
> > > >                 -> "Processing 2":0 [ENABLED,IMMUTABLE]
> > > >
> > > > - entity 19: GPIO (0 pad, 0 link, 0 routes)
> > > >              type V4L2 subdev subtype Decoder flags 0
> > > >              device node name /dev/v4l-subdev0
> > > >
> > > > Signed-off-by: Ricardo Ribalda <ribalda@...omium.org>
> > > > ---
> > > > Changes in v2:
> > > > - Rebase on top of https://patchwork.linuxtv.org/project/linux-media/patch/20241106-uvc-crashrmmod-v6-1-fbf9781c6e83@chromium.org/
> > > > - Create uvc_gpio_cleanup and uvc_gpio_deinit
> > > > - Refactor quirk: do not disable irq
> > > > - Change define number for MEDIA_ENT_F_GPIO
> > > > - Link to v1: https://lore.kernel.org/r/20241031-uvc-subdev-v1-0-a68331cedd72@chromium.org
> > > >
> > > > ---
> > > > Ricardo Ribalda (5):
> > > >       media: uvcvideo: Factor out gpio functions to its own file
> > > >       Revert "media: uvcvideo: Allow entity-defined get_info and get_cur"
> > > >       media: uvcvideo: Create ancillary link for GPIO subdevice
> > > >       media: v4l2-core: Add new MEDIA_ENT_F_GPIO
> > > >       media: uvcvideo: Use MEDIA_ENT_F_GPIO for the GPIO entity
> > > >
> > > > Yunke Cao (1):
> > > >       media: uvcvideo: Re-implement privacy GPIO as a separate subdevice
> > > >
> > > >  .../userspace-api/media/mediactl/media-types.rst   |   4 +
> > > >  drivers/media/usb/uvc/Makefile                     |   3 +-
> > > >  drivers/media/usb/uvc/uvc_ctrl.c                   |  40 +----
> > > >  drivers/media/usb/uvc/uvc_driver.c                 | 123 +-------------
> > > >  drivers/media/usb/uvc/uvc_entity.c                 |  20 ++-
> > > >  drivers/media/usb/uvc/uvc_gpio.c                   | 187 +++++++++++++++++++++
> > > >  drivers/media/usb/uvc/uvc_video.c                  |   4 +
> > > >  drivers/media/usb/uvc/uvcvideo.h                   |  34 ++--
> > > >  drivers/media/v4l2-core/v4l2-async.c               |   3 +-
> > > >  include/uapi/linux/media.h                         |   1 +
> > > >  10 files changed, 252 insertions(+), 167 deletions(-)
> > > > ---
> > > > base-commit: 4353256f5487e0c5c47e8ff764bf4f9e679fb525
> > > > change-id: 20241030-uvc-subdev-89f4467a00b5
>
> --
> Regards,
>
> Laurent Pinchart



-- 
Ricardo Ribalda

Powered by blists - more mailing lists