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

[David Plowman]

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:

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
>