Message-ID: <20241126171836.GA23391@pendragon.ideasonboard.com>
Date: Tue, 26 Nov 2024 19:18:36 +0200
From: Laurent Pinchart <laurent.pinchart@...asonboard.com>
To: Ricardo Ribalda <ribalda@...omium.org>
Cc: Hans de Goede <hdegoede@...hat.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 Tue, Nov 26, 2024 at 05:22:20PM +0100, Ricardo Ribalda wrote:
> On Mon, 25 Nov 2024 at 15:02, Hans de Goede wrote:
> > On 25-Nov-24 2:39 PM, Ricardo Ribalda wrote:
> > > On Mon, 25 Nov 2024 at 13:25, Hans de Goede wrote:
> > >> On 9-Nov-24 5:29 PM, Ricardo Ribalda wrote:
> > >>
> > >> <snip>
> > >>
> > >>>> I have been discussing UVC power-management with Laurent, also
> > >>>> related to power-consumption issues caused by libcamera's pipeline
> > >>>> handler holding open the /dev/video# node as long as the camera
> > >>>> manager object exists.
> > >>
> > >> <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?
> > >>
> > >> Yes, actually looking at how complex this is when e.g. also taking
> > >> controls into account I think that taking small steps is a good idea.
> > >>
> > >> I have lately mostly been working on sensor drivers where delaying
> > >> applying format settings + all controls to stream-on is normal.
> > >>
> > >> So that is the mental model I'm applying to uvc here, but that might
> > >> not be entirely applicable.
> > >>
> > >>> Laurent mentioned that some cameras missbehave if a lot of controls
> > >>> are set during probing. I hope that this approach does not trigger
> > >>> those, and if it does it would be easier to revert if we do the work
> > >>> in two steps.
> > >>
> > >> Ack, taking small steps sounds like a good plan.
> > >>
> > >> <snip>
> > >>
> > >>>> This should also make camera enumeration faster for apps, since
> > >>>> most apps / frameworks do the whole 3 levels nested loop for this
> > >>>> on startup, for which atm we go out to the hw, which now instead
> > >>>> will come from the fmts cache and thus will be much much faster,
> > >>>> so this should lead to a noticeable speedup for apps accessing UVC
> > >>>> cameras which would be another nice win.
> > >>>>
> > >>>> Downside is that the initial probe will take longer see we do
> > >>>> all the tryfmt-s there now. But I think that taking a bit longer
> > >>>> to probe while the machine is booting should not be an issue.
> > >>>
> > >>> How do you pretend to handle the controls? Do you plan to power-up the
> > >>> device during s_ctrl() or set them only during streamon()?
> > >>> If we power-up the device during s_ctrl we need to take care of the
> > >>> asynchronous controls (typically pan/tilt/zoom), The device must be
> > >>> powered until the control finishes, and the device might never reply
> > >>> control_done if the firmware is not properly implemented.
> > >>> If we set the controls only during streamon, we will break some
> > >>> usecases. There are some video conferencing equipment that do homing
> > >>> during streamoff. That will be a serious API breakage.
> > >>
> > >> How to handle controls is a good idea.
> > >>
> > >> Based on my sensor experience my initial idea was to just cache them
> > >> all. Basically make set_ctrl succeed but do not actually do anyhing
> > >> when the camera is not already powered on and then on stream-on call
> > >> __v4l2_ctrl_handler_setup() to get all current values applied.
> > >>
> > >> But as you indicate that will likely not work well with async controls,
> > >> although we already have this issue when using v4l2-ctl from the cmdline
> > >> on such a control and that seems to work fine.
> > >
> > > -----
> > >> Just because we allow
> > >> the USB connection to sleep, does not mean that the camera cannot finish
> > >> doing applying the async control.
> > >>
> > > Not sure what you mean with this sentence. Could you explain it
> > > differently? Sorry
> > >
> > >> But I can see how some cameras might not like this and having 2 different
> > >> paths for different controls also is undesirable.
> > >>
> > >> Combine that with what Laurent said about devices not liking it when
> > >> you set too much controls in a short time and I do think we need to
> > >> immediately apply ctrls.
> > >>
> > >> I see 2 ways of doing that:
> > >>
> > >> 1. Use pm_runtime_set_autosuspend_delay() with a delay of say 1 second
> > >> and then on set_ctrl do a pm_runtime_get_sync() +
> > >> pm_runtime_put_autosuspend() giving the camera 1 second to finish
> > >> applying the async ctrl (which might not be enough for e.g homing) +
> > >> also avoid doing suspend + resume all the time if multiple ctrls are send
> > >
> > > What about 1.5:
> > >
> > > during s_ctrl():
> > > usb_autopm_get_interface()
> > > if the control is UVC_CTRL_FLAG_ASYNCHRONOUS.
> > >        usb_autopm_get_interface()
> > > set the actual control in the hardware
> > > usb_autopm_put_interface()
> > >
> > > during uvc_ctrl_status_event():
> > >    usb_autopm_put_interface()
> >
> > How do we match this to the usb_autopm_get_interface()
> > call ? At a minimum we would need some counter to
> > track pending (not acked through status interrupt urb)
> > async control requests and only do the put() if that
> > counter >= 1 (and then decrease the counter).
> >
> > We don't want to do unbalanced puts here in case of
> > buggy cameras sending unexpected / too many
> > ctrl status events.

We would need a counter indeed, which is a big red flag of bad
engineering. It will be fragile at best.

> > > during close():
> > >    send all the missing usb_autopm_put_interface()
> >
> > Except for my one remark this is an interesting
> > proposal.
> 
> I have just upload a patchset implementing this. I tried
> v4l2-compliance and using the camera app.
> 
> I think it looks promissing
> 
> Shall we move the discussion there?
> 
> https://lore.kernel.org/linux-media/20241126-uvc-granpower-ng-v1-0-6312bf26549c@chromium.org/T/#t

You're sending too many patch series too quickly, even before we can
come to an agreement on any item being discussed. Experimenting is
helpful, but if we keep moving the discussion from one series to the
next, that won't work. Let's keep it here, and focus on one problem at a
time, or the end result will be slower merging of the patches.

> > Maybe also do a dev_warn() if there are missing
> > usb_autopm_put_interface() calls pending on close() ?
> >
> > > This way:
> > > - we do not have an artificial delay that might not work for all the use cases
> > > - cameras with noncompliant async controls will have the same PM
> > > behaviour as now  (will be powered on until close() )
> > >
> > > We do the same with the rest of the actions that require hardware access, like:
> > > https://lore.kernel.org/linux-media/20220920-resend-powersave-v5-2-692e6df6c1e2@chromium.org/
> > >
> > > This way:
> > > - Apps that do not need to access the hardware, do not wake it up, and
> > > we do not break usecases.
> > >
> > > Next steps will be:
> > >  - cache the formats
> > >  - move the actual set_ctrl to streamon... but if we can do that I
> > > would argue than we can move completely to the control framework.
> >
> > Right I had forgotten that the UVC driver does not use the control
> > framework. I think moving to that would be a prerequisite for moving
> > the set_ctrl to stream_on.

-- 
Regards,

Laurent Pinchart

Powered by blists - more mailing lists