[libcamera-devel] [PATCH v1 3/5] ipa: raspberrypi: awb: Delay release of the statistics buffer

[Naushir Patuck]

Hi David,

Thank you for the review!

On Wed, 23 Nov 2022 at 13:52, David Plowman <david.plowman at raspberrypi.com>
wrote:

> Hi Naush
>
> Thanks for the patch.
>
> On Tue, 22 Nov 2022 at 11:22, Naushir Patuck via libcamera-devel
> <libcamera-devel at lists.libcamera.org> wrote:
> >
> > Release the statistics buffer after running the through the AWB
> calculations.
> > Only the "counted" statistics are copied out to a local structure, so
> keeping
> > the statistics buffer allows the algorithm to see the "uncounted"
> statistics as
> > well.
>
> I guess I still feel a bit weird about this. The code used to copy out
> everything it needed so that it could release the buffer as soon as
> possible. Maybe that's not a meaningful concern any more...?
>

In the original code, we used to make a copy of the stats dmabuf, and this
copy (through a shared_ptr) was what the controller algorithms would get.
We now copy values out of the stats dmabuf into the new stats structure,
and pass this in as a shared_ptr.

As such, moving the reset() to release the buffer to later does not have any
impact on recycling the stats dmabuf back to the ISP.  The alternative to
this
change would be to cache the stats grid sizes (that's what we really need)
instead of keeping the shared_ptr referenced for a bit longer?

Naush


>
> >
> > This is currently handled by hard-coding the total number of statistics
> regions
> > regions based on the structure definition in the bcm2835_isp_stats
> structure.
> >
> > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
>
> In spite of my probably unnecessary weird feelings, it's probably of
> no consequence, so:
>
> Reviewed-by: David Plowman <david.plowman at raspberrypi.com>
>
> Thanks!
> David
>
> > ---
> >  src/ipa/raspberrypi/controller/rpi/awb.cpp | 10 +++++-----
> >  1 file changed, 5 insertions(+), 5 deletions(-)
> >
> > diff --git a/src/ipa/raspberrypi/controller/rpi/awb.cpp
> b/src/ipa/raspberrypi/controller/rpi/awb.cpp
> > index 8d8ddf0913f7..861022014896 100644
> > --- a/src/ipa/raspberrypi/controller/rpi/awb.cpp
> > +++ b/src/ipa/raspberrypi/controller/rpi/awb.cpp
> > @@ -432,11 +432,6 @@ void Awb::prepareStats()
> >          */
> >         generateStats(zones_, statistics_->awb_stats, config_.minPixels,
> >                       config_.minG);
> > -       /*
> > -        * we're done with these; we may as well relinquish our hold on
> the
> > -        * pointer.
> > -        */
> > -       statistics_.reset();
> >         /*
> >          * apply sensitivities, so values appear to come from our
> "canonical"
> >          * sensor.
> > @@ -733,6 +728,11 @@ void Awb::doAwb()
> >                         << " with gains r " << asyncResults_.gainR
> >                         << " and b " << asyncResults_.gainB;
> >         }
> > +       /*
> > +        * we're done with these; we may as well relinquish our hold on
> the
> > +        * pointer.
> > +        */
> > +       statistics_.reset();
> >  }
> >
> >  /* Register algorithm with the system. */
> > --
> > 2.25.1
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20221123/2ee5710a/attachment.htm>