[libcamera-devel] [PATCH v3 5/5] libcamera: v4l2_videodevice: Empty the V4L2 buffer cache on streamOff()

[Laurent Pinchart]

On Mon, Mar 21, 2022 at 01:37:11PM +0000, Naushir Patuck via libcamera-devel wrote:
> On Mon, 21 Mar 2022 at 13:34, Kieran Bingham wrote:
> > Quoting Naushir Patuck (2022-03-21 10:27:02)
> > > When streamOff() is called, ensure the cache entires for the remaining queued
> > > buffers are freed since this will not happen via the dequeueBuffer() mechanism.
> > >
> > > Additionally, add a V4L2BufferCache::isEmpty() function and assert that the
> > > cache is empty at the end of the streamOff() call.
> > >
> > > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> > > Tested-by: David Plowman <david.plowman at raspberrypi.com>
> > > Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> >
> > Ohh I love an assert addition. It catches things. I wonder if these are
> > good things or bad things to catch:
> 
> Hmmm... How can this be possible?
> Presumably on streamOff(), all buffers must have been dequeued out of the device
> driver, and the cache has to be empty.  Is that an invalid assumption?

It doesn't sound invalid, but there's clearly a problem :-) You can
reproduce this by running unit tests with "ninja test" (the assertion
fails when running tests on an x86 machine with vimc, vim2m and vidid).

And while at that, I forgot to note during review that the buffer cache
unit test (test/v4l2_videodevice/buffer_cache.cpp) should be extended to
test the new isEmpty() function.

> > Processed 31 frames
> > Buffer received
> > Buffer received
> > Buffer received
> > Buffer received
> > Buffer received
> > Buffer received
> > Buffer received
> > Buffer received
> > stderr:
> > [339:43:53.887319781] [3693294]  WARN CameraSensorProperties camera_sensor_properties.cpp:148 No static properties available for 'Sensor A'
> > [339:43:53.887388240] [3693294]  WARN CameraSensorProperties camera_sensor_properties.cpp:150 Please consider updating the camera sensor properties database
> > [339:43:53.887416152] [3693294]  WARN CameraSensor camera_sensor.cpp:411 'Sensor A': Failed to retrieve the camera location
> > [339:43:54.491916393] [3693294] FATAL default v4l2_videodevice.cpp:1854 /dev/video6[7:cap]: assertion "cache_->isEmpty()" failed in streamOff()
> > Backtrace:
> > libcamera::V4L2VideoDevice::streamOff()+0x14c2 (../../src/libcamera/src/libcamera/v4l2_videodevice.cpp:1854)
> > CaptureAsyncTest::run()+0x2dbb (../../src/libcamera/test/v4l2_videodevice/capture_async.cpp:82)
> > Test::execute()+0x472 (../../src/libcamera/test/libtest/test.cpp:33)
> > main+0x2d2 (../../src/libcamera/test/v4l2_videodevice/capture_async.cpp:93)
> > __libc_start_call_main+0x80 (../sysdeps/nptl/libc_start_call_main.h:58)
> > __libc_start_main@@GLIBC_2.34+0x7d (../csu/libc-start.c:128)
> > _start+0x25 (/home/kbingham/iob/libcamera/ci/integrator/builds/unit-tests/test/v4l2_videodevice/capture_async
> > [0x000055a86d271fb5])
> >
> > ――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――
> >
> > 38/67 libcamera:v4l2_videodevice / buffer_sharing                OK
> >       6.31s
> > 39/67 libcamera:v4l2_videodevice / v4l2_m2mdevice                FAIL
> >       1.58s   killed by signal 6 SIGABRT
> > >>> MALLOC_PERTURB_=171
> > /home/kbingham/iob/libcamera/ci/integrator/builds/unit-tests/test/v4l2_videodevice/v4l2_m2mdevice
> > ――――――――――――――――――――――――――――――――――――― ✀
> > ―――――――――――――――――――――――――――――――――――――
> > stdout:
> >
> >
> >
> > Received capture buffer
> > Received capture buffer
> > Received capture buffer
> > Received capture buffer
> > stderr:
> > Output 31 frames
> > Captured 31 frames
> > [339:44:02.406962689] [3693463] FATAL default v4l2_videodevice.cpp:1854 /dev/video9[9:cap]: assertion "cache_->isEmpty()" failed in streamOff()
> > Backtrace:
> > libcamera::V4L2VideoDevice::streamOff()+0x14c2 (../../src/libcamera/src/libcamera/v4l2_videodevice.cpp:1854)
> > V4L2M2MDeviceTest::run()+0x523d (../../src/libcamera/test/v4l2_videodevice/v4l2_m2mdevice.cpp:173)
> > Test::execute()+0x472 (../../src/libcamera/test/libtest/test.cpp:33)
> > main+0x2bb (../../src/libcamera/test/v4l2_videodevice/v4l2_m2mdevice.cpp:205)
> > __libc_start_call_main+0x80 (../sysdeps/nptl/libc_start_call_main.h:58)
> > __libc_start_main@@GLIBC_2.34+0x7d (../csu/libc-start.c:128)
> > _start+0x25 (/home/kbingham/iob/libcamera/ci/integrator/builds/unit-tests/test/v4l2_videodevice/v4l2_m2mdevice
> > [0x000055b106567015])
> >
> > ――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――
> >
> > > ---
> > >  include/libcamera/internal/v4l2_videodevice.h |  1 +
> > >  src/libcamera/v4l2_videodevice.cpp            | 16 ++++++++++++++++
> > >  2 files changed, 17 insertions(+)
> > >
> > > diff --git a/include/libcamera/internal/v4l2_videodevice.h
> > b/include/libcamera/internal/v4l2_videodevice.h
> > > index 2d2ccc477c91..37747c0b2f27 100644
> > > --- a/include/libcamera/internal/v4l2_videodevice.h
> > > +++ b/include/libcamera/internal/v4l2_videodevice.h
> > > @@ -126,6 +126,7 @@ public:
> > >
> > >         int get(const FrameBuffer &buffer);
> > >         void put(unsigned int index);
> > > +       bool isEmpty() const;
> > >
> > >  private:
> > >         class Entry
> > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> > > index 5f36ee20710d..9da82697e7f0 100644
> > > --- a/src/libcamera/v4l2_videodevice.cpp
> > > +++ b/src/libcamera/v4l2_videodevice.cpp
> > > @@ -262,6 +262,19 @@ void V4L2BufferCache::put(unsigned int index)
> > >         cache_[index].free_ = true;
> > >  }
> > >
> > > +/**
> > > + * \brief Check if all the entries in the cache are unused
> > > + */
> > > +bool V4L2BufferCache::isEmpty() const
> > > +{
> > > +       for (auto const &entry : cache_) {
> > > +               if (!entry.free_)
> > > +                       return false;
> > > +       }
> > > +
> > > +       return true;
> > > +}
> > > +
> > >  V4L2BufferCache::Entry::Entry()
> > >         : free_(true), lastUsed_(0)
> > >  {
> > > @@ -1832,10 +1845,13 @@ int V4L2VideoDevice::streamOff()
> > >         for (auto it : queuedBuffers_) {
> > >                 FrameBuffer *buffer = it.second;
> > >
> > > +               cache_->put(it.first);
> > >                 buffer->metadata_.status = FrameMetadata::FrameCancelled;
> > >                 bufferReady.emit(buffer);
> > >         }
> > >
> > > +       ASSERT(cache_->isEmpty());
> > > +
> > >         queuedBuffers_.clear();
> > >         fdBufferNotifier_->setEnabled(false);
> > >         streaming_ = false;


-- 
Regards,

Laurent Pinchart