[RFC] delayed_controls: avoid reading undefined control value

[Stanislaw Gruszka]

On Wed, Dec 11, 2024 at 09:03:39AM +0000, Naushir Patuck wrote:
> On Wed, 11 Dec 2024 at 07:43, Laurent Pinchart
> <laurent.pinchart at ideasonboard.com> wrote:
> >
> > (CC'ing Naush for the question related to the Raspberry Pi
> > implementation)
> >
> > On Tue, Dec 10, 2024 at 10:39:50PM +0000, Kieran Bingham wrote:
> > > Quoting Stanislaw Gruszka (2024-12-06 11:27:43)
> > > > Limit ControlRingBuffer index by number of queued entries in order do
> > > > avoid reading undefined control value with type ControlTypeNone .
> > > >
> > > > It can happen at the begging of streaming when ControlRingBuffer is
> > >
> > > s/begging/beginning/
> > >
> > > > not yet filled and there are errors on CSI-2 bus resulting dropping
> > > > frames. Then we can call to DelayedControls::get() with frame number
> > > > that exceed number of saved entries and get below assertion failure:
> > > >
> > > > ../src/libcamera/delayed_controls.cpp:227:
> > > > libcamera::ControlList libcamera::DelayedControls::get(uint32_t):
> > > > Assertion `info.type() != ControlTypeNone' failed
> > > >
> > > > Bug: https://bugs.libcamera.org/show_bug.cgi?id=241
> > > > Signed-off-by: Stanislaw Gruszka <stanislaw.gruszka at linux.intel.com>
> > > > ---
> > > > Notes: When debugging this I notice that the push() and get() are
> > > > done in different threads. Maybe mutex should be
> > > > used? Not sure how synchronization works for mojom signals
> > > > handlers.
> >
> > The comment related to the SoftwareISP::inputBufferReady signal, in the
> > SimpleCameraData::init() function, possibly applies to other signals of
> > the SoftwareISP class, although I wouldn't expect it would affect the
> > setSensorControls signal. Stanislaw, could you provide more information
> > about which threads the push and get functions are called from ?
> > Everything should be called from the camera manager thread.
> >
> > Looking at this more deeply, I think the outputBufferReady and
> > statsReady signals may be mishandled, as they seem to be emitted from
> > the soft ISP worker thread but not treated the same way as the
> > inputBufferReady signal. We shouldn't have to deal with it when
> > connecting the signals in the user, the SoftwareISP class should handle
> > thread crossing, and emit all its signals from the camera manager
> > thread. Milan, is this something you could address ?
> >
> > > I thought they would be in the same thread as the CameraManager event
> > > loop...  Perhaps we could/should have an assertion that the thread is
> > > the same - otherwise - yes it would probably need a lock if there's a
> > > reader and a writer on both ends of the queue.
> > >
> > > I'd double check the threading first.
> > >
> > > > Also I'm not sure if similar change like in this patch
> > > > is not needed for rpi DelayedControls
> > >
> > > Probably. There's very little different between the two implementations,
> > > so we should probably keep them aligned with bugfixes at least until
> > > someone tries to realign them both again.
> >
> > Naush, could you check this ?
> 
> Theoretically if our kernel would somehow dequeue buffers without
> triggering the frame start event, this could hit us.  However, in that

After debug this further on my setup, this is actually wrong for simple
pipeline. We do not correctly subscribe for frameStart events and
never call DelayedControls::applyControls(uint32_t sequence).

I'll debug this a bit more and provide more details and answers
to questions.

Regards
Stanislaw

> indexing into the ring buffer.  I don't recall any user raising an
> issue like this so I doubt it's a problem for us in practice.  I'm ok
> to add a fix in our DelayedControls implementation, but I would have a
> slight preference on changing the fix to seed the entire ring buffer
> with initial values rather than clamp the index.
> 
> Naush
> 
> 
> >
> > > It looks like the cookie feature was easier to fork than to add, I can't
> > > remember all the details ;_(
> > >
> > >
> > > > Alternative fix would be to pre-fill ControlRingBuffer with
> > > > valid control values. Should we care about possibility
> > > > of queueCount_ overflowing ?
> > >
> > > I'm not sure what 'valid' control values would be ? I think just
> > > clamping to the most recent data is probably the best we can do ? And 'I
> > > think' would reflect the most up to date information we have about
> > > what's configured on the sensor - so I think it's still even the 'most
> > > correct' (and probably even 'is' correct) data we can return in the
> > > get() call in this event.
> > >
> > >
> > > I think I can already throw this on here, it seems like a sane input
> > > parameter validation:
> > >
> > >
> > > Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > >
> > > >  src/libcamera/delayed_controls.cpp | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/src/libcamera/delayed_controls.cpp b/src/libcamera/delayed_controls.cpp
> > > > index 94d0a575..78b0b8f7 100644
> > > > --- a/src/libcamera/delayed_controls.cpp
> > > > +++ b/src/libcamera/delayed_controls.cpp
> > > > @@ -202,7 +202,7 @@ bool DelayedControls::push(const ControlList &controls)
> > > >   */
> > > >  ControlList DelayedControls::get(uint32_t sequence)
> > > >  {
> > > > -       unsigned int index = std::max<int>(0, sequence - maxDelay_);
> > > > +       unsigned int index = std::clamp<int>(sequence - maxDelay_, 0, queueCount_ - 1);
> > > >
> > > >         ControlList out(device_->controls());
> > > >         for (const auto &ctrl : values_) {
> >
> > --
> > Regards,
> >
> > Laurent Pinchart