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

[Laurent Pinchart]

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.

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

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

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

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

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