Message-ID: <CANiDSCv0U=i2ZSd0abfP1JYrY-HVKa5t_7qAea+9KCog5P2PVg@mail.gmail.com>
Date: Mon, 25 Nov 2024 14:50:40 +0100
From: Ricardo Ribalda <ribalda@...omium.org>
To: Hans de Goede <hdegoede@...hat.com>
Cc: Laurent Pinchart <laurent.pinchart@...asonboard.com>, 
	Mauro Carvalho Chehab <mchehab@...nel.org>, Sakari Ailus <sakari.ailus@...ux.intel.com>, 
	linux-kernel@...r.kernel.org, linux-media@...r.kernel.org, 
	Yunke Cao <yunkec@...omium.org>, Hans Verkuil <hverkuil@...all.nl>
Subject: Re: [PATCH v2 0/6] media: uvcvideo: Implement the Privacy GPIO as a subdevice

On Mon, 25 Nov 2024 at 14:44, Hans de Goede <hdegoede@...hat.com> 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@...asonboard.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 ?

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

Powered by blists - more mailing lists