[libcamera-devel] [PATCH] pipeline: rpi: vc4: Allocate more embedded data buffers

[Kieran Bingham]

Quoting William Vinnicombe via libcamera-devel (2023-09-18 10:30:16)
> If the pipeline runs out of embedded data buffers, then it will pass
> the frame to the IPA without the metadata. The IPA then has to use the
> delayed controls as inputs to the algorithms. This can cause problems
> with the subsequent algorithms if the sensor did not action the
> controls, especially with the autofocus as that doesn't have controls
> which can be passed in lieu of the metadata.

I assume the implication here is that you've detected this happening at
times ?

> Reduce the likelihood of this by increasing the number of embedded data
> buffers, as they are small so a generous number can be allocated.
> 
> Signed-off-by: William Vinnicombe <william.vinnicombe at raspberrypi.com>
> ---
>  src/libcamera/pipeline/rpi/vc4/vc4.cpp | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/rpi/vc4/vc4.cpp b/src/libcamera/pipeline/rpi/vc4/vc4.cpp
> index 018cf488..6777c697 100644
> --- a/src/libcamera/pipeline/rpi/vc4/vc4.cpp
> +++ b/src/libcamera/pipeline/rpi/vc4/vc4.cpp
> @@ -262,9 +262,9 @@ int PipelineHandlerVc4::prepareBuffers(Camera *camera)
>                 } else if (stream == &data->unicam_[Unicam::Embedded]) {
>                         /*
>                          * Embedded data buffers are (currently) for internal use,
> -                        * so allocate the minimum required to avoid frame drops.
> +                        * so allocate a generous number as they are small.

Can we improve on 'Generous number' at all? What aspects can we consider
here?

The pipeline depth should be a known value (4 frames?) so doubling that
would perhaps already be 'generous'?

Or basing it on the number of raw stream buffers we may have available
to queue to unicam? We can't ever capture embedded data without also
capturing a raw image can we ? (or maybe we can? )

"""
 * Embedded data buffers are (currently) for internal use, and are small
 * enough ( ~ 1kb -> 2 kb ) that we can allocate them generously to avoid
 * causing problems in the IPA when we can not supply the metadata.
 *
 * We may have up to 6 raw buffers in the pipeline, so use double this
 * value to ensure we always have sufficient resources.
"""

(Of course I just made up the comment details so I don't expect it to be
correct at all, just an example)

>                          */
> -                       numBuffers = minBuffers;
> +                       numBuffers = 12;

Normally we'd put constants like this into a constexpr too and define at
the top of the file - but I think with a large comment like above that
would be overkill here. Best to keep the code in one place in this
instance IMO.

>                 } else {
>                         /*
>                          * Since the ISP runs synchronous with the IPA and requests,
> -- 
> 2.39.2
>