[libcamera-devel] [PATCH v1 2/2] pipeline: raspberrypi: Fix the buffer count calculation for the ISP input stream

[Kieran Bingham]

Quoting David Plowman (2022-02-01 09:47:09)
> Hi Naush
> 
> Thanks for this patch. Weird how we never noticed this problem before!
> 
> On Tue, 1 Feb 2022 at 09:27, Naushir Patuck <naush at raspberrypi.com> wrote:
> >
> > The ISP input stream currently only allocates a single slot in the
> > V4L2VideoDevice cache as it follows the number of buffers allocated for use.
> > However, this is wrong as the ISP input stream imports buffers from Unicam
> > image stream.  As a consequence of this, only one cache slot was used during
> > runtime for the ISP input stream, and if multiple buffers were to be queued
> > simultaneously, the queue operation would return a failure.
> >
> > Fix this by passing the same number of RAW buffers available from the Unicam
> > image stream. Additionally, double this count in the cases where buffers could
> > be allocated externally from the application.
> 
> I agree this sounds like it wants a better solution, but is definitely
> beyond the scope of this (relatively small but important) fix. So:

Did you ever dig further into what the maximum number of V4L2 buffers
can be allocated are?

Should we change the V4L2Device to always just allocate the maximum
there instead?

These are just 'structures' to be able to pass the buffer information
right ? They're not the actual expensive memory backing?

That would also then give V4L2BufferCache the best chance of supporting
more external buffers?

--
Kieran


> Reviewed-by: David Plowman <david.plowman at raspberrypi.com>
> 
> Thanks!
> David
> 
> >
> > Bug: https://github.com/raspberrypi/libcamera-apps/issues/236
> >      https://github.com/raspberrypi/libcamera-apps/issues/238
> >      https://github.com/raspberrypi/libcamera-apps/issues/240
> > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> > ---
> >  src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 8 ++++++++
> >  src/libcamera/pipeline/raspberrypi/rpi_stream.cpp  | 3 ++-
> >  2 files changed, 10 insertions(+), 1 deletion(-)
> >
> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > index 6a46e6bc89fa..49af56edc1f9 100644
> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > @@ -1382,6 +1382,14 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)
> >                          * minimise frame drops.
> >                          */
> >                         numBuffers = std::max<int>(2, minBuffers - numRawBuffers);
> > +               } else if (stream == &data->isp_[Isp::Input]) {
> > +                       /*
> > +                        * ISP input buffers are imported from Unicam, so follow
> > +                        * similar logic as above to count all the RAW buffers
> > +                        * available.
> > +                        */
> > +                       numBuffers = numRawBuffers + std::max<int>(2, minBuffers - numRawBuffers);
> > +
> >                 } else if (stream == &data->unicam_[Unicam::Embedded]) {
> >                         /*
> >                          * Embedded data buffers are (currently) for internal use,
> > diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
> > index a4159e20b068..a421ad09ba50 100644
> > --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
> > +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
> > @@ -111,11 +111,12 @@ int Stream::prepareBuffers(unsigned int count)
> >          * 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.
> > +        * Similarly, if this stream is only importing buffers, we do the same.
> >          *
> >          * \todo Find a better heuristic, or, even better, an exact solution to
> >          * this issue.
> >          */
> > -       if (isExternal())
> > +       if (isExternal() || importOnly_)
> >                 count = count * 2;
> >
> >         return dev_->importBuffers(count);
> > --
> > 2.25.1
> >