| Message ID | 20240419055849.1086196-1-paul.elder@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
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 >
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 }; > > /*
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 > >
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 }; > > > > > > /*
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 }; /*
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(-)