Message-ID: <20241125124942.GA32280@pendragon.ideasonboard.com>
Date: Mon, 25 Nov 2024 14:49:42 +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>,
	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 Mon, Nov 25, 2024 at 01:25:41PM +0100, Hans de Goede wrote:
> Hi Ricardo,
> 
> On 9-Nov-24 5:29 PM, Ricardo Ribalda wrote:
> 
> <snip>
> 
> >> 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.
> 
> <snip>
> 
> >> 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?
> 
> Yes, actually looking at how complex this is when e.g. also taking
> controls into account I think that taking small steps is a good idea.
> 
> I have lately mostly been working on sensor drivers where delaying
> applying format settings + all controls to stream-on is normal.
> 
> So that is the mental model I'm applying to uvc here, but that might
> not be entirely applicable.
> 
> > 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.
> 
> Ack, taking small steps sounds like a good plan.
> 
> <snip>
> 
> >> 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.

Talking about powering up is a bit misleading here. What this is about
is resuming the USB device. The device stays powered when it is
suspended, and the value of controls is retained unless the USB device
gets reset. The .resume() handler does not restore controls, the
.reset_resume() handler does.

Some UVC cameras have the USB_QUIRK_RESET_RESUME quirk set, but the
majority doesn't. Annoyingly there's a blanket entry for all Logitech
UVC devices in drivers/usb/core/quirks.c, which has led the uvcvideo
driver to grow a UVC_QUIRK_NO_RESET_RESUME quirk.

> > 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.
> 
> How to handle controls is a good idea.
> 
> Based on my sensor experience my initial idea was to just cache them
> all. Basically make set_ctrl succeed but do not actually do anyhing
> when the camera is not already powered on and then on stream-on call
> __v4l2_ctrl_handler_setup() to get all current values applied.
> 
> But as you indicate that will likely not work well with async controls,

It's not all asynchronous controls, only the ones that have an impact on
the device that can be observed without streaming. Mechanical pan/tilt
is the main use case. We could handle those controls in a special way if
needed.

> although we already have this issue when using v4l2-ctl from the cmdline
> on such a control and that seems to work fine. Just because we allow
> the USB connection to sleep, does not mean that the camera cannot finish
> doing applying the async control.
> 
> But I can see how some cameras might not like this and having 2 different
> paths for different controls also is undesirable.
> 
> Combine that with what Laurent said about devices not liking it when
> you set too much controls in a short time and I do think we need to
> immediately apply ctrls.
> 
> I see 2 ways of doing that:
> 
> 1. Use pm_runtime_set_autosuspend_delay() with a delay of say 1 second
> and then on set_ctrl do a pm_runtime_get_sync() +
> pm_runtime_put_autosuspend() giving the camera 1 second to finish
> applying the async ctrl (which might not be enough for e.g homing) +
> also avoid doing suspend + resume all the time if multiple ctrls are send
> 
> 2. Instead of immediately powering on the camera on /dev/video# open
> track per fh if the camera has been powered on and then on the first
> set-ctrl, or the first other IOCTL like try/set-fmt which requires
> the camera to be powered on power it up and then keep it on until
> the fh is closed, since apps hitting these paths are likely to do
> more stuff which requires the camera to be powered on.

A mode of operation where a userspace action causes a state change and
the only way to change back to the previous state is to close the device
often leads to problems. I'd rather not do this unless we have to
completely rule out all other options.

> This should avoid apps (like udev rules) just doing a simple get-cap
> query of powering on the camera, while at the same time preserving
> the current behavior for apps which want to actually do something
> with the camera, so the chance of regressions should be small.
> 
> I guess the time between power-up and sending the first request to
> the camera will change slightly. But most apps which actually want
> to do stuff with the camera will likely already do so immediately
> after opening /dev/video# so the timing change should be negligible.
> 
> I guess 2. is pretty similar to your proposal to delay power-on
> to the first set/try-fmt, which I assume also already does
> something similar wrt controls ?
> 
> I think that 2. can work nicely and that would be nice to have,
> even though it does not fix the privacy-control + power-mgmt issue.

-- 
Regards,

Laurent Pinchart

Powered by blists - more mailing lists