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

[Naushir Patuck]

Hi all,

On Thu, 21 Sept 2023 at 16:53, Kieran Bingham
<kieran.bingham at ideasonboard.com> wrote:
>
> Quoting Naushir Patuck (2023-09-21 16:14:40)
> > Hi Kieran,
> >
> > On Thu, 21 Sept 2023 at 16:10, Kieran Bingham via libcamera-devel
> > <libcamera-devel at lists.libcamera.org> wrote:
> > >
> > > Quoting William Vinnicombe (2023-09-21 14:44:38)
> > > > 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?
> > >
> > > Do we know how many application buffers there are? Is that a value we
> > > can determine when we import buffers? or during configuration?
> >
> > We can (sort of), but it's not entirely trivial hence keeping it
> > simple with a constant of 12 buffers with this change.
> >
> > >
> > > What happens if I have an application which allocates 16 buffers, and
> > > runs at 120 FPS? Will 12 still be sufficient?
> >
> > It should be sufficient even if we have > 12 RAW buffers.  This is
> > because the lifetime of the embedded buffers is much smaller than the
> > RAW buffers and they will be recycled quicker.  This is why the
> > current number of 4 worked for the most part, but not always at the
> > faster farmerates...
>
> Capturing the rationale that the lifetime of the embedded buffers is
> smaller than the raw buffers sounds helpful to capture in the comment
> too then. That's what I was looking for!

Just coming back to this one as it does not seem to be merged yet.  Is
it sufficient to update the commit message to get this merged, or do
we need anything more?

Regards,
Naush

>
>
> --
> Kieran
>
>
> >
> > Regards,
> > Naush
> >
> >
> > >
> > > --
> > > Kieran
> > >
> > >
> > >
> > > >
> > > >
> > > > > >                          */
> > > > > > -                       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
> > > > > >
> > > > >