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

[Kieran Bingham]

Quoting Naushir Patuck (2021-11-10 10:42:58)
> Hi Kieran,
> 
> Thanks for all the reviews!
> 
> On Wed, 10 Nov 2021 at 10:33, Kieran Bingham <
> kieran.bingham at ideasonboard.com> 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.
> >
> 
> 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...

--
Kieran

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