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

17832 diff mbox series

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.

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@raspberrypi.com>
---
 src/ipa/raspberrypi/controller/rpi/awb.cpp | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

Hi Naush

Thanks for the patch.

On Tue, 22 Nov 2022 at 11:22, Naushir Patuck via libcamera-devel
<libcamera-devel@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...?

>
> 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@raspberrypi.com>

In spite of my probably unnecessary weird feelings, it's probably of
no consequence, so:

Reviewed-by: David Plowman <david.plowman@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
>

Hi David,

Thank you for the review!

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

> Hi Naush
>
> Thanks for the patch.
>
> On Tue, 22 Nov 2022 at 11:22, Naushir Patuck via libcamera-devel
> <libcamera-devel@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@raspberrypi.com>
>
> In spite of my probably unnecessary weird feelings, it's probably of
> no consequence, so:
>
> Reviewed-by: David Plowman <david.plowman@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
> >
>