Message-ID: <CANiDSCsY_uCRyO2NAmxob12=DSa3h+CXg_MaMWLrjMFz1_Ru-g@mail.gmail.com>
Date: Tue, 26 Nov 2024 17:22:20 +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

Hi

On Mon, 25 Nov 2024 at 15:02, Hans de Goede <hdegoede@...hat.com> wrote:
>
> Hi Ricardo,
>
> On 25-Nov-24 2:39 PM, Ricardo Ribalda wrote:
> > On Mon, 25 Nov 2024 at 13:25, Hans de Goede <hdegoede@...hat.com> wrote:
> >>
> >> Hi Ricardo,
> >>
> >> 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.
>
> > 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


>
> 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,
>
> Hans
>


--
Ricardo Ribalda

Powered by blists - more mailing lists