Message-ID: <CANiDSCtvLB=tWb7ZCFCw9gn26R2xHnOf=yTLj+M_4AuQKYvgOQ@mail.gmail.com>
Date: Mon, 25 Nov 2024 14:39:01 +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>, 
	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, 25 Nov 2024 at 13:25, Hans de Goede <hdegoede@...hat.com> 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.
> > 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,
> 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.
>
Not sure what you mean with this sentence. Could you explain it
differently? Sorry

> 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

What about 1.5:

during s_ctrl():
usb_autopm_get_interface()
if the control is UVC_CTRL_FLAG_ASYNCHRONOUS.
       usb_autopm_get_interface()
set the actual control in the hardware
usb_autopm_put_interface()

during uvc_ctrl_status_event():
   usb_autopm_put_interface()

during close():
   send all the missing usb_autopm_put_interface()


This way:
- we do not have an artificial delay that might not work for all the use cases
- cameras with noncompliant async controls will have the same PM
behaviour as now  (will be powered on until close() )

We do the same with the rest of the actions that require hardware access, like:
https://lore.kernel.org/linux-media/20220920-resend-powersave-v5-2-692e6df6c1e2@chromium.org/

This way:
- Apps that do not need to access the hardware, do not wake it up, and
we do not break usecases.

Next steps will be:
 - cache the formats
 - move the actual set_ctrl to streamon... but if we can do that I
would argue than we can move completely to the control framework.



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


-- 
Ricardo Ribalda

Powered by blists - more mailing lists