[libcamera-devel] [PATCH 2/3] pipeline: raspberrypi: Rework the internal buffer allocation scheme

[Kieran Bingham]

Quoting Naushir Patuck (2021-11-10 10:08:01)
> For simplicity, the pipeline handler would look at the maximum number of
> buffers set in the StreamConfiguration and allocate the same number of internal
> buffers for all device nodes. This would likely overallocate buffers for some
> nodes. Rework this logic to try and minimise overallcations without compromising
> performance.
> 
> For ISP nodes, we only ever need 1 set of internal buffers, as the hardware runs
> synchronous with the requests and IPA.
> 
> For Unicam nodes, allocate a minimum for 4 buffers (exported + internal), but
> also require at least 1 internal buffer.
> 
> Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> ---
>  .../pipeline/raspberrypi/raspberrypi.cpp      | 44 ++++++++++++++-----
>  1 file changed, 33 insertions(+), 11 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index 11d3c2b120dd..5f0f00aacd59 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -1211,21 +1211,43 @@ int PipelineHandlerRPi::queueAllBuffers(Camera *camera)
>  int PipelineHandlerRPi::prepareBuffers(Camera *camera)
>  {
>         RPiCameraData *data = cameraData(camera);
> +       unsigned int numBuffers;
> +       bool rawStream = false;
>         int ret;
>  
> -       /*
> -        * Decide how many internal buffers to allocate. For now, simply look
> -        * at how many external buffers will be provided. We'll need to improve
> -        * this logic. However, we really must have all streams allocate the same
> -        * number of buffers to simplify error handling in queueRequestDevice().
> -        */
> -       unsigned int maxBuffers = 0;
> -       for (const Stream *s : camera->streams())
> -               if (static_cast<const RPi::Stream *>(s)->isExternal())
> -                       maxBuffers = std::max(maxBuffers, s->configuration().bufferCount);
> +       for (Stream *s : camera->streams()) {
> +               if (isRaw(s->configuration().pixelFormat)) {
> +                       numBuffers = s->configuration().bufferCount;
> +                       rawStream = true;
> +                       break;
> +               }
> +       }
>  
> +       /* Decide how many internal buffers to allocate. */
>         for (auto const stream : data->streams_) {
> -               ret = stream->prepareBuffers(maxBuffers);
> +               if (stream == &data->unicam_[Unicam::Image] ||
> +                   stream == &data->unicam_[Unicam::Embedded]) {
> +                       /*
> +                        * For Unicam, allocate a minimum of 4 buffers as we want
> +                        * to avoid any frame drops. If an application has configured
> +                        * a RAW stream, allocate additional buffers to make up the
> +                        * minimum, but ensure we have at least 1 set of internal
> +                        * buffers to use to minimise frame drops.
> +                        */
> +                       constexpr unsigned int minBuffers = 4;
> +                       numBuffers = rawStream ? std::max<int>(1, minBuffers - numBuffers)
> +                                              : minBuffers;
> +               } else {
> +                       /*
> +                        * Since the ISP runs synchronous with the IPA and requests,
> +                        * we only ever need one set of internal buffers. Any buffers
> +                        * the application wants to hold onto will already be exported
> +                        * through PipelineHandlerRPi::exportFrameBuffers().
> +                        */

Ok, I see this is for the 4 device streams of the ISP. These don't
represent the buffers that go to the user application though do they?

Oh - I see - it's /just/ any extra internal allocation. If a user buffer
is sent in, it would be used on Output0, Output1 ... so we're not going
to starve here.

Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>

> +                       numBuffers = 1;
> +               }
> +
> +               ret = stream->prepareBuffers(numBuffers);
>                 if (ret < 0)
>                         return ret;
>         }
> -- 
> 2.25.1
>