[libcamera-devel] Problems with getControls() and listControls()

[David Plowman]

Hi everyone

Sorry I've not been following up on this topic over the last week or two!

On Tue, 9 Mar 2021 at 17:09, David Plowman
<david.plowman at raspberrypi.com> wrote:
>
> Hi Laurent, everyone
>
> Thanks for the detailed analysis.
>
> On Mon, 8 Mar 2021 at 02:52, Laurent Pinchart
> <laurent.pinchart at ideasonboard.com> wrote:
> >
> > Hello everybody,
> >
> > On Fri, Mar 05, 2021 at 03:29:53PM +0000, Kieran Bingham wrote:
> > > On 05/03/2021 15:23, David Plowman wrote:
> > > > On Fri, 5 Mar 2021 at 11:10, Kieran Bingham wrote:
> > > >> On 05/03/2021 11:06, Kieran Bingham wrote:
> > > >>> On 05/03/2021 10:47, David Plowman wrote:
> > > >>>> To fill in the details on some of this, the controls that can have
> > > >>>> their ranges altered (looking at current drivers) are:
> > > >>>>
> > > >>>> Exposure: this range can change whenever the vblanking is updated, so
> > > >>>> it can happen every frame. Luckily I don't think anyone cares about
> > > >>>> the ranges here so everything seems to work as expected...
> > > >>>>
> > > >>>> The remaining ones normally get updated only when the camera mode
> > > >>>> is changed.
> > > >>>>
> > > >>>> Pixel Rate
> > > >>>> Hblanking
> > > >>>> Vblanking
> > > >>>>
> > > >>>> As far as I'm aware, only vblanking is a problem as we use those
> > > >>>> limits to calculate possible frame timings. For the pixel rate and
> > > >>>> hblanking we use the value, not the limits, so I believe we get the
> > > >>>> right numbers back there.
> >
> > Agreed, those should be fine.
> >
> > > >>>> I'm inclined to agree with Kieran that watching for changes to values
> > > >>>> probably won't work. A modification of the range (I take it there's no
> > > >>>> event for that directly..?) doesn't necessarily cause a change in the
> > > >>>> value. Pragmatically speaking, regenerating the ControlInfo list after
> > > >>>> a setFormat doesn't sound like a terrible idea. Alternatively, and
> > > >>>> 100% safe, perhaps we should give up caching max/min values and
> > > >>>> re-fetch them every time they're wanted? Do we query these limits
> > > >>>> often?
> > > >>>
> > > >>> I'd also be a bit weary of some sort of race between when the kernel
> > > >>> updates us, to when we process the control event update, vs when some
> > > >>> other code would read it.
> > > >>
> > > >> So I've just been told there is an event V4L2_EVENT_CTRL_CH_RANGE which
> > > >> we can subscribe to, to know when a controls range is changed.
> > > >>
> > > >> So we can handle these updates 'automagically' ...
> > > >>
> > > >> The next question is ... is that the right way ;-) or will that be racy...
> > > >
> > > > I think this is probably a question for the libcamera team. If it were
> > > > solely down to me, I feel that events, callbacks and race conditions
> > > > are starting to get just a little fussy for something that seemed like
> > > > it should have been simple, and I would  be inclined just to query
> > > > them every time I want them. But this is taking us away from the
> > > > original idea of (cached lists of) ControlInfo objects, and I'm not
> > > > really clear how much use other pipeline handlers are making of them,
> > > > which leaves me hesitating...
> > >
> > > Yup, I spoke with Laurent earlier, this thread is on his todo list - but
> > > equally he doesn't think parsing the updates through events would be
> > > right from my understanding.
> > >
> > > So we'll have to make sure we update the value when we need to.
> >
> > From the point of view of a pipeline handler (which is all that matters
> > here, as IPAs don't have direct access to the devices), everything runs
> > in a single thread, with an event loop. When you call
> > V4L2VideoDevice::setFormat(), the resulting control range change events
> > that are queued by the kernel will not be processed until control
> > returns to the event loop. This prevents race conditions, but will not
> > give us updated information early enough.
> >
> > I can think of multiple ways to address this issue:
> >
> > - Removing the range cache. That's both simple and drastic. It will slow
> >   things down, put perhaps not to an extent that would negatively impact
> >   us.
>
> (Let's call this option 1.)
>
> Does the performance concerns relate mainly to querying the ranges
> while the sensor is streaming (rather than during setup)? The Pi
> pipeline handler doesn't do this - do we think other implementations
> would?
>
> Having said that, the idea of not using ControlInfos with v4l2
> controls (or treating them differently) could sit awkwardly with the
> fact that other controls (our IPA ones, for example) do use them. I
> also wonder if there's overlap with the issue of "dynamic controls",
> which might mean this is a bigger topic?
>
> >
> > - Manually updating ranges at the end of V4L2VideoDevice::setFormat().
> >   This is what you've proposed with making listControls() protected. It
> >   will in practice be a bit more complicated than that, as
> >   listControls() would replace the existing ControlInfoMap, which would
> >   invalidate all existing references, leading to hard to debug issues.
> >   We would have to update it instead.
>
> (Option 2)
>
> In practice I think this would work, and probably involves changing
> the least amount of stuff. Would others agree? Might this be a
> stop-gap until something involving "dynamic controls" has been figured
> out?

I was wondering how folks would feel about giving this solution a try,
at least for the time being. It has some benefits:

* It seems like a relatively contained change, probably the smallest
of any discussed here.
* I don't think it makes the deeper problem any harder to solve, or
make subsequent (better) solutions more difficult.
* And it will - for the realistic scenarios we're aware of - fix the problem.

If there's a general sentiment in favour I'd be happy to give it a
try, but please let me know!

Thanks
David

>
> >
> > - Processing (V4L2) control event changes manually at the end of
> >   V4L2VideoDevice::setFormat(). The (libcamera) event handling
> >   infrastructure needs to be extended a bit here, to allow for selective
> >   event processing (we don't want to handle other type of events that
> >   would call back into the pipeline handler and create race conditions),
> >   but that shouldn't be too hard to do.
>
> (Option 3)
>
> I'm not familiar with these bits of code, so probably best if I defer
> to others there!
>
> >
> > - Improving V4L2 to avoid changes in ranges when the sensor mode is
> >   modified. The root cause of the issue is that we're exposing a VBLANK
> >   control to userspace, when sensors will have a fixed vertical total
> >   size. The vertical blanking limits then vary as the maximum value is
> >   essentially max total vertical size minus active height.
> >
> >   This would be a much more intrusive change as it would require changes
> >   on the kernel side, so I'm not sure it would really be realistic, but
> >   we could then address at the same time the annoying dependency between
> >   EXPOSURE and VBLANK.
>
> (Option 4)
>
> Agree that this doesn't sound like a route to a fix any time in the
> very near future! Also sounds like it could mean revisiting many
> camera drivers... :(
>
> >
> > - Giving up on the broken kernel API, and calculating the limits in
> >   libcamera based on values stored in a sensor database. This is also a
> >   quite drastic option, but it could simplify the implementation quite a
> >   lot.
>
> (Option 5)
>
> Similar comments to option 4, I think.
>
> So to summarise that lot, I'm starting to wonder whether there's an
> interim kind of solution where we don't change too much (perhaps
> something like option 2 or 3), which will at least make the cameras
> run reliably at the correct rate. Subsequently there might be a more
> comprehensive solution, perhaps involving "dynamic controls" and so
> on...?
>
> Anyone have any other thoughts?
>
> Thanks again!
>
> David
>
> >
> > > >>> What about marking some controls as 'volatile' such that it must always
> > > >>> get the full control info, (or whatever it needs), or would 'volatile'
> > > >>> be too much?
> > > >>>
> > > >>> I don't know if it's something we need to update all values once per
> > > >>> frame, or if we only need 'some' ?
> > > >>>
> > > >>> Otherwise we could have an explicit function to manually reload min/max
> > > >>> values on a per-control basis ?
> > > >>>  (if there's only 'one' that needs to be reloaded?)
> > > >>>
> > > >>>> On Fri, 5 Mar 2021 at 09:11, Naushir Patuck wrote:
> > > >>>>> On Thu, 4 Mar 2021 at 20:05, Kieran Bingham wrote:
> > > >>>>>> On 03/03/2021 15:57, David Plowman wrote:
> > > >>>>>>> Hi everyone
> > > >>>>>>>
> > > >>>>>>> I've been chasing down problems with a camera driver (sometimes) not
> > > >>>>>>> running at the correct framerates. It seems that I've tracked it down
> > > >>>>>>> to libcamera's control list mechanism. Please correct me if I've got
> > > >>>>>>> anything wrong in my explanation below.
> > > >>>>>>>
> > > >>>>>>> V4L2 devices produce a list of all their controls, including min and
> > > >>>>>>> max values, when they are opened - as the V4L2Device constructor calls
> > > >>>>>>> the private listControls() method.
> > > >>>>>>>
> > > >>>>>>> The CameraInfo::sensorInfo() method calculates the minimum and maximum
> > > >>>>>>> frame lengths using the min and max vblank values. These values were
> > > >>>>>>> read and stored when the device was opened. Since then, however, the
> > > >>>>>>> format requested from the sensor may well have changed which may mean
> > > >>>>>>> these values are now wrong.
> > > >>>>>>
> > > >>>>>> This sounds troublesome indeed.
> > > >>>>>>
> > > >>>>>>> So firstly, have I understood this correctly? And secondly, what do
> > > >>>>>>> you think would be the preferred way to fix it? Perhaps the
> > > >>>>>>> listControls() method should be made "protected" and called at the end
> > > >>>>>>> of V4L2Subdevice::setFormat? Or should getControls() always update the
> > > >>>>>>> ranges..?
> > > >>>>>>
> > > >>>>>> I suspect the 'right' way to do this is to get the kernel to tell us if
> > > >>>>>> the control has changed. V4L2 devices can expose events:
> > > >>>>>>
> > > >>>>>> https://www.kernel.org/doc/html/latest/driver-api/media/v4l2-event.html
> > > >>>>>>
> > > >>>>>> We should be able to receive events for control changes through
> > > >>>>>> V4L2_EVENT_CTRL:
> > > >>>>>>
> > > >>>>>> https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/vidioc-dqevent.html#event-type
> > > >>>>>>
> > > >>>>>> We already have a means to subscribe to events in our V4L2Device and
> > > >>>>>> V4L2VideoDevice classes, so handling control updates there should be a
> > > >>>>>> case of extending from there.
> > > >>>>>>
> > > >>>>>> However, I have one concern on your reports above.
> > > >>>>>>
> > > >>>>>> It seems it's not the value that has changed, but the minimum and
> > > >>>>>> maximums? It looks like the events are reported when the 'value'
> > > >>>>>> changes, but I don't know if the min/max can be changed? I thought those
> > > >>>>>> were initialised at control creation time. But perhaps I've just never
> > > >>>>>> done it ;-)
> > > >>>>>
> > > >>>>> This is correct.  The VBLANK min/max limits change when we change
> > > >>>>> sensor modes.  When we call V4L2Subdevice::getControl(VBLANK), it
> > > >>>>> does return the correct "value", but the min/max remain unchanged.
> > > >>>>> In the current codebase, the min/max values are only updated on a
> > > >>>>> V4L2Device:listControl() call, which only happens when you open the device.
> > > >>>>>
> > > >>>>>> Hope this helps (or at least bumps the discussions).
> >
> > --
> > Regards,
> >
> > Laurent Pinchart