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

[David Plowman]

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?

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