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

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

Hi,

On 25-Nov-24 1:58 PM, Laurent Pinchart wrote:
> On Mon, Nov 25, 2024 at 01:39:05PM +0100, Hans de Goede wrote:
>> Hi Ricardo,
>>
>> On 10-Nov-24 5:07 PM, Ricardo Ribalda wrote:
>>> On Sun, 10 Nov 2024 at 16:14, Laurent Pinchart
>>> <laurent.pinchart@ideasonboard.com> wrote:
>>
>> <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?
>>>>
>>>> This sounds worth trying. We'll need to test it on a wide range of
>>>> devices though, both internal and external.
>>>
>>> For what is worth, we have been running something similar to
>>> https://lore.kernel.org/linux-media/20220920-resend-powersave-v5-2-692e6df6c1e2@chromium.org/
>>> in ChromeOS and it has worked fine....
>>>
>>> But I am pretty sure that it has issues with async controls :S
>>
>> Interesting that is actually a lot more aggressive (as in doing a
>> usb_autopm_put_interface() often) then what I expected when you said:
>>
>> "Can we start powering up the device during try/set fmt"
>>
>> As I mentioned in my other emails what I think would worth nicely
>> is delay the initial usb_autopm_get_interface() till we need it
>> and then just leave the camera on till /dev/video# gets closed.
>>
>> That idea is based on dividing apps in 2 groups:
>>
>> 1. Apps just temporarily opening /dev/video# nodes for discovery,
>> where we ideally would not power-up the camera.
>>
>> 2. Apps (could even be the same app) opening /dev/video# for
>> a longer time because it actually want to use the camera
>> at the moment of opening and which close the /dev/video# node
>> when done with the camera.
>>
>> Your code seems to also covers a 3th group of apps:
>>
>> 3. Apps opening /dev/video# for a long time, while not using
>> it all the time.
>>
>> Although it would be nice to also cover those, IMHO those are
>> not well behaved apps and I'm not sure if we need to cover those.
> 
> Is that right ? Isn't it better for an application to keep the device
> open to avoid open delays or even open failures when it wants to use the
> device ?

Keeping devices open has advantages and disadvantages. E.g. keeping
/dev/input/event# nodes open will also typically lead to e.g.
touchscreens staying powered on.

Generally speaking it is not unheard of to expect userspace to
behave in a certain way for things like this for power-consumption
reasons.

I guess the question is how far do we want to go inside the uvc
driver to avoid userspace needing to close the /dev/video# nodes
when unused.

Ricardo's patch from here:

https://lore.kernel.org/linux-media/20220920-resend-powersave-v5-2-692e6df6c1e2@chromium.org/

goes all the way and if I understand Ricardo correctly this is
already in use in ChromeOS ?

So we could also go with this. Maybe put it behind a Kconfig option
for a while ?

AFAICT the only thing which needs to be figured out there is async
controls.

I think we can simply set a long autosuspend delay on devices with
async controls to deal with that ?

I have a Logitech QuickCam Orbit (non AF) UVC camera which has
pan + tilt controls and AFAICT these work fine with v4l2-ctl
which immediately closes the /dev/video# node after the set-ctrl
command. But I'm not sure if I have tested this without the camera
streaming at the time. Anyways that is at least one camera I can test.

Regards,

Hans