On Mon, 25 Nov 2024 at 14:44, Hans de Goede <hdegoede@xxxxxxxxxx> wrote: > > 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 ? It is in use on just some devices. I try to not diverge from upstream if I can. > > So we could also go with this. Maybe put it behind a Kconfig option > for a while ? I can try to work on a new version of the patch, including support for async controls the way I described in my previous email. Let me know what you think. > > 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 > > -- Ricardo Ribalda
