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

[Kieran Bingham]

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!


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