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

[William Vinnicombe]

Hi Kieran,

Thanks for your comments

On Mon, 18 Sept 2023 at 11:41, Kieran Bingham <
kieran.bingham at ideasonboard.com> wrote:

> 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)
>

12 was selected as an arbitrary number, which is more than the number of
input buffers (which is application dependent, but is typically 8-10). As
they are much smaller than the image buffers (1-2kB rather than MB), it's
much simpler here to just set it to 12 rather than querying the number of
input buffers, as the extra memory used is small. If that explanation
sounds good, I can submit a v2 with the comment edited to that effect?


> >                          */
> > -                       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
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20230921/f4c8571d/attachment.htm>