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

[Naushir Patuck]

Hi Roman,

On Thu, 11 Nov 2021 at 08:36, Roman Stratiienko <r.stratiienko at gmail.com>
wrote:

> Naush,
>
> Please let me once again remind you that I have to set the
> configuration variable by downstream patch which determines the max.
> number of buffers for Android to allocate.
>
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -429,9 +429,9 @@ CameraConfiguration::Status
> RPiCameraConfiguration::validate()
>                         cfg.frameSize = unicamFormat.planes[0].size;
>                         rawCount++;
>                 } else {
> +                       cfg.bufferCount = 3;
>                         outSize[outCount] = std::make_pair(count,
> cfg.size);
>                         /* Record the largest resolution for fixups later.
> */
>                         if (maxSize < cfg.size) {
>
> So maybe this value could be used to calculate cache size. Sorry if I
> am talking about wrong things, I am new to the libcamera codebase.
>

I think this change effectively over allocates the buffer cache, and fixes
the original problem you were seeing.  The buffercount does indirectly
get used to size the buffer cache in the existing code.

I would not necessarily say this is the correct fix though, and we probably
need to understand the Android layer's buffer allocation mechanism better
for the Raspberry Pi pipeline handler.

Naush



>
> Roman.
>
> чт, 11 нояб. 2021 г. в 10:16, Naushir Patuck <naush at raspberrypi.com>:
> >
> > Hi Umang,
> >
> > Thank you for your reviews!
> >
> > On Wed, 10 Nov 2021 at 18:35, Umang Jain <umang.jain at ideasonboard.com>
> wrote:
> >>
> >> Hi Naush
> >>
> >> Thank you for the patch
> >>
> >> On 11/10/21 3:38 PM, Naushir Patuck wrote:
> >> > 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.
> >>
> >>
> >> One clarification pleease, does this mean applications can still
> >> allocate more buffers on the fly (i.e. the count can increase in
> >> future), even after RPi pipeline-handler has started?
> >
> >
> > My understanding is that the Android layer does exactly this.
> >
> > As pointed out by Kieran, one issue is that we may not know
> > the exact number of buffers allocated by the application.  Hence
> > we need a mechanism where the buffer cache sizing might have
> > to become dynamic to account for additional buffers.  For now,
> > over allocating the slots in the cache will be sufficient.
> >
> > Regards,
> > Naush
> >
> >>
> >>
> >> >
> >> > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> >>
> >> Patch looks good so,
> >>
> >> Reviewed-by: Umang Jain <umang.jain at ideasonboard.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.
> >> > +      */
> >> > +     if (isExternal())
> >> > +             count = count * 2;
> >> > +
> >> >       return dev_->importBuffers(count);
> >> >   }
> >> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20211111/570b8c91/attachment-0001.htm>