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

[Kieran Bingham]

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?

What happens if I have an application which allocates 16 buffers, and
runs at 120 FPS? Will 12 still be sufficient?

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