Message-ID: <55c76c99-dc86-41b2-84c6-d2e844530f67@redhat.com>
Date: Mon, 25 Nov 2024 13:25:41 +0100
From: Hans de Goede <hdegoede@...hat.com>
To: Ricardo Ribalda <ribalda@...omium.org>
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

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.

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.

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

Powered by blists - more mailing lists