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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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 ?

> Although looking back at the libcamera uvc pipeline handler issue
> I fixed recently, that was actually a case of 3.

-- 
Regards,

Laurent Pinchart