[libcamera-devel] [PATCH 3/3] pipeline: raspberrypi: Increase the V4L2BufferCache slot allocations

[Laurent Pinchart]

On Wed, Nov 10, 2021 at 10:52:02AM +0000, Kieran Bingham wrote:
> Quoting Naushir Patuck (2021-11-10 10:42:58)
> > On Wed, 10 Nov 2021 at 10:33, Kieran Bingham wrote:
> > > Quoting Naushir Patuck (2021-11-10 10:08:02)
> > > > If a stream is marked as external, double the number of V4L2BufferCache slots
> > > > that are allocated. This is to account for additional buffers that may be
> > > > allocated directly by the application.
> > > >
> > > > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> > > > ---
> > > >  src/libcamera/pipeline/raspberrypi/rpi_stream.cpp | 8 ++++++++
> > > >  1 file changed, 8 insertions(+)
> > > >
> > > > diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
> > > > index b3265d0e8aab..67901936d6b6 100644
> > > > --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
> > > > +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
> > > > @@ -107,6 +107,14 @@ int Stream::prepareBuffers(unsigned int count)
> > > >                 count = bufferMap_.size();
> > > >         }
> > > >
> > > > +       /*
> > > > +        * If this is an external stream, we must allocate slots for buffers that
> > > > +        * might be externally allocated. We have no indication of how many buffers
> > > > +        * may be used, so this might overallocate slots in the buffer cache.
> > > > +        */
> > >
> > > Perhaps we'll need a better system here (not in this patch) to handle
> > > this ... but I think overallocating is cheap - and the whole point of
> > > the buffer cache is to try to re-use the same buffers where possible. So
> > > I /think/ overallocations is the right solution for now anyway.

Overallocation of V4L2 buffer slots is cheap, but we indeed can't know
in advance how much to overallocate. Note that there's a hardcoded limit
of 32 buffers in the kernel (VIDEO_MAX_FRAME).

> > I think this over allocation is the only change needed to "fix" Roman's original
> > issue.  But I did mean to tidy up the buffer allocations for some time now, so
> > why not :-)
> > 
> > > > +       if (isExternal())
> > > > +               count = count * 2;
> > >
> > > Of course it's hard to know /how/ far to overallocate ... But this will
> > > do until we figure it out ;-)
> > 
> > I wonder if we can perhaps allow the cache to grow in size dynamically
> > instead of relying on a fixed number of slots?
> 
> That's the bit that I wondered about as the "better system" - but it
> would require a matching number of 'buffers' on the v4l2 side - so
> dynamically growing the slots would have to interact with the V4L2
> buffer allcoations I think...
>
> I believe we can ask for more buffers some how - but I don't know if we
> can do so /while/ streaming...

There's VIDIOC_CREATE_BUFS for that. The question is when to create new
buffer slots, as if applications were to create new buffers all the
time, the cache would keep growing. We probably need some type of aging
system to decide when a previously cached entry would be old enough to
be reused.

> > > Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > >
> > > > +
> > > >         return dev_->importBuffers(count);
> > > >  }
> > > >

-- 
Regards,

Laurent Pinchart