[libcamera-devel] [PATCH] libcamera: v4l2_videodevice: Fix V4L2 buffer cache entry availabity check

[Naushir Patuck]

Hi Niklas,

On Wed, 12 Feb 2020 at 15:07, Niklas Söderlund
<niklas.soderlund at ragnatech.se> wrote:
>
> Hi Naushir,
>
> On 2020-02-12 11:47:42 +0000, Naushir Patuck wrote:
> > Hi,
> >
> > Would anybody - hopefully Niklas :) - be able to do a quick review of
> > the below patch please?  It does seem we need it (or similar
> > alternative) for good performance on our platform.
>
> I had a quick look and I agree this fix solves a real issue. However I'm
> concerned it introduce another one. If more imported buffers are used
> then there are entries in the cache there is no eviction of the oldest
> occupied entry not in use.
>

Yes, this was my thoughts on the fix as well.  However, I noticed that
the cache size is always initialised to the number of buffers
allocated, so we would always have the same number of entries as
buffers - maybe my understanding is wrong?



> This is my fault as I have it on my todo list to write a proper test for
> the V4L2BufferCache and address any faults like the one you address in
> this change. If you don't beat me to it I hope to do so beginning next
> week.
>

Not a problem.  I am running locally with the patch below, so this is
not impacting me in any way.

> >
> > Regards,
> > Naush
> >
> > On Thu, 30 Jan 2020 at 15:43, <naush at raspberrypi.com> wrote:
> > >
> > > From: Naushir Patuck <naush at raspberrypi.com>
> > >
> > > The logic for keeping track of the cache holding dmabuf file descriptors
> > > did not distinguish between slots that are occuiped but free and slots
> > > that are not occupied.  In devices using imported dmabufs this would
> > > result in ping-ponging between the top two cache entries - and that in
> > > turn would result in unnecessary cache missies.
> > >
> > > Fix this by tracking if an entry is occupied separately from if an entry
> > > is free.
> > >
> > > Fixes: 9e71540ebbde ("libcamera: v4l2_videodevice: Add V4L2BufferCache to deal with index mapping")
> > > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> > > ---
> > >  src/libcamera/include/v4l2_videodevice.h | 1 +
> > >  src/libcamera/v4l2_videodevice.cpp       | 6 +++---
> > >  2 files changed, 4 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/src/libcamera/include/v4l2_videodevice.h b/src/libcamera/include/v4l2_videodevice.h
> > > index e4d35ab..a77b62e 100644
> > > --- a/src/libcamera/include/v4l2_videodevice.h
> > > +++ b/src/libcamera/include/v4l2_videodevice.h
> > > @@ -125,6 +125,7 @@ private:
> > >                 bool operator==(const FrameBuffer &buffer);
> > >
> > >                 bool free;
> > > +               bool occupied;
> > >
> > >         private:
> > >                 struct Plane {
> > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> > > index 37b8d33..b887a38 100644
> > > --- a/src/libcamera/v4l2_videodevice.cpp
> > > +++ b/src/libcamera/v4l2_videodevice.cpp
> > > @@ -212,7 +212,7 @@ int V4L2BufferCache::get(const FrameBuffer &buffer)
> > >                 if (!entry.free)
> > >                         continue;
> > >
> > > -               if (use < 0)
> > > +               if (use < 0 && !entry.occupied)
> > >                         use = index;
> > >
> > >                 /* Try to find a cache hit by comparing the planes. */
> > > @@ -245,12 +245,12 @@ void V4L2BufferCache::put(unsigned int index)
> > >  }
> > >
> > >  V4L2BufferCache::Entry::Entry()
> > > -       : free(true)
> > > +       : free(true), occupied(false)
> > >  {
> > >  }
> > >
> > >  V4L2BufferCache::Entry::Entry(bool free, const FrameBuffer &buffer)
> > > -       : free(free)
> > > +       : free(free), occupied(true)
> > >  {
> > >         for (const FrameBuffer::Plane &plane : buffer.planes())
> > >                 planes_.emplace_back(plane);
> > > --
> > > 2.17.1
> > >
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel at lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
>
> --
> Regards,
> Niklas Söderlund

Best regards,
Naush