Re: [PATCH v2 0/6] media: uvcvideo: Implement the Privacy GPIO as a subdevice

[Hans de Goede <hdegoede@redhat.com>]

Hi Ricardo,

On 25-Nov-24 2:39 PM, Ricardo Ribalda wrote:
> On Mon, 25 Nov 2024 at 13:25, Hans de Goede <hdegoede@redhat.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()

How do we match this to the usb_autopm_get_interface()
call ? At a minimum we would need some counter to
track pending (not acked through status interrupt urb)
async control requests and only do the put() if that
counter >= 1 (and then decrease the counter).

We don't want to do unbalanced puts here in case of
buggy cameras sending unexpected / too many
ctrl status events.

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

Except for my one remark this is an interesting
proposal.

Maybe also do a dev_warn() if there are missing
usb_autopm_put_interface() calls pending on close() ?

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

Right I had forgotten that the UVC driver does not use the control
framework. I think moving to that would be a prerequisite for moving
the set_ctrl to stream_on.

Regards,

Hans