Hi On Mon, 25 Nov 2024 at 15:02, Hans de Goede <hdegoede@xxxxxxxxxx> 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@xxxxxxxxxx> 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@xxxxxxxxxxxx/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@xxxxxxxxxxxx/ > > > > 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
