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

[Kieran Bingham]

Quoting Naushir Patuck (2023-10-18 13:16:40)
> 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?

My suggestion was to capture some of the rationale above into the comment
where the value is used so that it's not just an opaque "12 is a magic
number here".

If you'd rather leave this as is, let me know and I can merge. You've
already provided an RB tag.

--
Kieran


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