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@xxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxx/ >>> 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@xxxxxxxxxxxx/ 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
