[v2] ipa: rkisp1: agc: Fix histogram construction
diff mbox series

Message ID 20240419055849.1086196-1-paul.elder@ideasonboard.com
State Accepted
Headers show
Series
  • [v2] ipa: rkisp1: agc: Fix histogram construction
Related show

Commit Message

Paul Elder April 19, 2024, 5:58 a.m. UTC
This histogram reported by the rkisp1 hardware is 20 bits, where the
upper 16 bits are meaningful integer data and the lower 4 bits are
fractional and meant to be discarded. Remove thse 4 bits when
construction the histogram.

Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>

---
This was meant to be in the same series as "[PATCH v2] ipa: libipa:
histogram: Add transform parameter to constructor"

This depends on v2 of "Centralise Agc into libipa".

Changes in v2:
- use a lambda function instead of the rshift parameter
---
 src/ipa/rkisp1/algorithms/agc.cpp | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Kieran Bingham April 19, 2024, 8:42 p.m. UTC | #1
Quoting Paul Elder (2024-04-19 06:58:49)
> This histogram reported by the rkisp1 hardware is 20 bits, where the
> upper 16 bits are meaningful integer data and the lower 4 bits are
> fractional and meant to be discarded. Remove thse 4 bits when
> construction the histogram.
> 
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> 
> ---
> This was meant to be in the same series as "[PATCH v2] ipa: libipa:
> histogram: Add transform parameter to constructor"
> 
> This depends on v2 of "Centralise Agc into libipa".

I assume you mean

"[PATCH v2] ipa: libipa: histogram: Add transform parameter to constructor" ?

> 
> Changes in v2:
> - use a lambda function instead of the rshift parameter
> ---
>  src/ipa/rkisp1/algorithms/agc.cpp | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
> index 27b6f2c1..26526f5e 100644
> --- a/src/ipa/rkisp1/algorithms/agc.cpp
> +++ b/src/ipa/rkisp1/algorithms/agc.cpp
> @@ -278,7 +278,8 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
>         const rkisp1_cif_isp_stat *params = &stats->params;
>         ASSERT(stats->meas_type & RKISP1_CIF_ISP_STAT_AUTOEXP);
>  
> -       Histogram hist({ params->hist.hist_bins, context.hw->numHistogramBins });
> +       Histogram hist({ params->hist.hist_bins, context.hw->numHistogramBins },
> +                      [](uint32_t x) { return x >> 4; });

It's a clean fix and more generalised and still optimised so works for
me.

I would have thought a comment above would be beneficial to explain why
we're shifting all the values right 4 here though.

Anyway.

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

>         expMeans_ = { params->ae.exp_mean, context.hw->numAeCells };
>  
>         /*
> -- 
> 2.39.2
>
Dan Scally April 22, 2024, 10:41 p.m. UTC | #2
Hi Paul

On 19/04/2024 06:58, Paul Elder wrote:
> This histogram reported by the rkisp1 hardware is 20 bits, where the
> upper 16 bits are meaningful integer data and the lower 4 bits are
> fractional and meant to be discarded. Remove thse 4 bits when
> construction the histogram.

"Remove those 4 bits when constructing the Histogram", if you end up sending another version, but 
otherwise:


Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>

> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
>
> ---
> This was meant to be in the same series as "[PATCH v2] ipa: libipa:
> histogram: Add transform parameter to constructor"
>
> This depends on v2 of "Centralise Agc into libipa".
>
> Changes in v2:
> - use a lambda function instead of the rshift parameter
> ---
>   src/ipa/rkisp1/algorithms/agc.cpp | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
> index 27b6f2c1..26526f5e 100644
> --- a/src/ipa/rkisp1/algorithms/agc.cpp
> +++ b/src/ipa/rkisp1/algorithms/agc.cpp
> @@ -278,7 +278,8 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
>   	const rkisp1_cif_isp_stat *params = &stats->params;
>   	ASSERT(stats->meas_type & RKISP1_CIF_ISP_STAT_AUTOEXP);
>   
> -	Histogram hist({ params->hist.hist_bins, context.hw->numHistogramBins });
> +	Histogram hist({ params->hist.hist_bins, context.hw->numHistogramBins },
> +		       [](uint32_t x) { return x >> 4; });
>   	expMeans_ = { params->ae.exp_mean, context.hw->numAeCells };
>   
>   	/*
Paul Elder April 26, 2024, 8:18 a.m. UTC | #3
On Fri, Apr 19, 2024 at 09:42:17PM +0100, Kieran Bingham wrote:
> Quoting Paul Elder (2024-04-19 06:58:49)
> > This histogram reported by the rkisp1 hardware is 20 bits, where the
> > upper 16 bits are meaningful integer data and the lower 4 bits are
> > fractional and meant to be discarded. Remove thse 4 bits when
> > construction the histogram.
> > 
> > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > 
> > ---
> > This was meant to be in the same series as "[PATCH v2] ipa: libipa:
> > histogram: Add transform parameter to constructor"
> > 
> > This depends on v2 of "Centralise Agc into libipa".
> 
> I assume you mean
> 
> "[PATCH v2] ipa: libipa: histogram: Add transform parameter to constructor" ?

I mean I depend on that too (see above "This was meant to be in the same
series...") but this specific patch also depends on Dan's agc series.

Or I could preempt his series :)


Paul

> 
> > 
> > Changes in v2:
> > - use a lambda function instead of the rshift parameter
> > ---
> >  src/ipa/rkisp1/algorithms/agc.cpp | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
> > index 27b6f2c1..26526f5e 100644
> > --- a/src/ipa/rkisp1/algorithms/agc.cpp
> > +++ b/src/ipa/rkisp1/algorithms/agc.cpp
> > @@ -278,7 +278,8 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
> >         const rkisp1_cif_isp_stat *params = &stats->params;
> >         ASSERT(stats->meas_type & RKISP1_CIF_ISP_STAT_AUTOEXP);
> >  
> > -       Histogram hist({ params->hist.hist_bins, context.hw->numHistogramBins });
> > +       Histogram hist({ params->hist.hist_bins, context.hw->numHistogramBins },
> > +                      [](uint32_t x) { return x >> 4; });
> 
> It's a clean fix and more generalised and still optimised so works for
> me.
> 
> I would have thought a comment above would be beneficial to explain why
> we're shifting all the values right 4 here though.
> 
> Anyway.
> 
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> >         expMeans_ = { params->ae.exp_mean, context.hw->numAeCells };
> >  
> >         /*
> > -- 
> > 2.39.2
> >
Laurent Pinchart May 3, 2024, 12:17 a.m. UTC | #4
On Fri, Apr 26, 2024 at 05:18:58PM +0900, Paul Elder wrote:
> On Fri, Apr 19, 2024 at 09:42:17PM +0100, Kieran Bingham wrote:
> > Quoting Paul Elder (2024-04-19 06:58:49)
> > > This histogram reported by the rkisp1 hardware is 20 bits, where the
> > > upper 16 bits are meaningful integer data and the lower 4 bits are
> > > fractional and meant to be discarded. Remove thse 4 bits when
> > > construction the histogram.
> > > 
> > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > > 
> > > ---
> > > This was meant to be in the same series as "[PATCH v2] ipa: libipa:
> > > histogram: Add transform parameter to constructor"
> > > 
> > > This depends on v2 of "Centralise Agc into libipa".
> > 
> > I assume you mean
> > 
> > "[PATCH v2] ipa: libipa: histogram: Add transform parameter to constructor" ?
> 
> I mean I depend on that too (see above "This was meant to be in the same
> series...") but this specific patch also depends on Dan's agc series.
> 
> Or I could preempt his series :)
> 
> > > 
> > > Changes in v2:
> > > - use a lambda function instead of the rshift parameter
> > > ---
> > >  src/ipa/rkisp1/algorithms/agc.cpp | 3 ++-
> > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
> > > index 27b6f2c1..26526f5e 100644
> > > --- a/src/ipa/rkisp1/algorithms/agc.cpp
> > > +++ b/src/ipa/rkisp1/algorithms/agc.cpp
> > > @@ -278,7 +278,8 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
> > >         const rkisp1_cif_isp_stat *params = &stats->params;
> > >         ASSERT(stats->meas_type & RKISP1_CIF_ISP_STAT_AUTOEXP);
> > >  
> > > -       Histogram hist({ params->hist.hist_bins, context.hw->numHistogramBins });
> > > +       Histogram hist({ params->hist.hist_bins, context.hw->numHistogramBins },
> > > +                      [](uint32_t x) { return x >> 4; });
> > 
> > It's a clean fix and more generalised and still optimised so works for
> > me.
> > 
> > I would have thought a comment above would be beneficial to explain why
> > we're shifting all the values right 4 here though.

Add

	/* The lower 4 bits are fractional and meant to be discarded. */

and get my

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

if you also fix the typo in the commit message.

> > 
> > Anyway.
> > 
> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > 
> > >         expMeans_ = { params->ae.exp_mean, context.hw->numAeCells };
> > >  
> > >         /*

Patch
diff mbox series

diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
index 27b6f2c1..26526f5e 100644
--- a/src/ipa/rkisp1/algorithms/agc.cpp
+++ b/src/ipa/rkisp1/algorithms/agc.cpp
@@ -278,7 +278,8 @@  void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
 	const rkisp1_cif_isp_stat *params = &stats->params;
 	ASSERT(stats->meas_type & RKISP1_CIF_ISP_STAT_AUTOEXP);
 
-	Histogram hist({ params->hist.hist_bins, context.hw->numHistogramBins });
+	Histogram hist({ params->hist.hist_bins, context.hw->numHistogramBins },
+		       [](uint32_t x) { return x >> 4; });
 	expMeans_ = { params->ae.exp_mean, context.hw->numAeCells };
 
 	/*