<div dir="ltr"><div dir="ltr">Hi Jacopo,<div><br></div><div>Thank you for your review!</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, 23 Mar 2023 at 16:11, Jacopo Mondi <<a href="mailto:jacopo.mondi@ideasonboard.com">jacopo.mondi@ideasonboard.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Naush<br>
<br>
On Wed, Mar 22, 2023 at 01:06:04PM +0000, Naushir Patuck via libcamera-devel wrote:<br>
> Add a new Controller::HardwareConfig structure that captures the<br>
> hardware statistics grid/histogram sizes and pipeline widths. This<br>
> ensures there is a single centralised places for these parameters.<br>
><br>
> Add a getHardwareConfig() helper function to retrieve these values for a<br>
> given hardware target.<br>
><br>
> Update the statistics populating routine in the IPA to use the values<br>
> from this structure instead of the hardcoded numbers.<br>
><br>
> Signed-off-by: Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a>><br>
> Reviewed-by: David Plowman <<a href="mailto:david.plowman@raspberrypi.com" target="_blank">david.plowman@raspberrypi.com</a>><br>
> ---<br>
> src/ipa/raspberrypi/controller/algorithm.h | 4 ++<br>
> src/ipa/raspberrypi/controller/controller.cpp | 39 +++++++++++++++++++<br>
> src/ipa/raspberrypi/controller/controller.h | 11 ++++++<br>
> src/ipa/raspberrypi/raspberrypi.cpp | 21 ++++------<br>
> 4 files changed, 62 insertions(+), 13 deletions(-)<br>
><br>
> diff --git a/src/ipa/raspberrypi/controller/algorithm.h b/src/ipa/raspberrypi/controller/algorithm.h<br>
> index 7c22fbe4945c..4aa814ebbebd 100644<br>
> --- a/src/ipa/raspberrypi/controller/algorithm.h<br>
> +++ b/src/ipa/raspberrypi/controller/algorithm.h<br>
> @@ -45,6 +45,10 @@ public:<br>
> {<br>
> return controller_->getTarget();<br>
> }<br>
> + const Controller::HardwareConfig &getHardwareConfig() const<br>
> + {<br>
> + return controller_->getHardwareConfig();<br>
> + }<br>
><br>
<br>
Will this be used ?<br></blockquote><div><br></div><div>Yes, in subsequent patches :)</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> private:<br>
> Controller *controller_;<br>
> diff --git a/src/ipa/raspberrypi/controller/controller.cpp b/src/ipa/raspberrypi/controller/controller.cpp<br>
> index a6250ee140b0..2c7517aec6b4 100644<br>
> --- a/src/ipa/raspberrypi/controller/controller.cpp<br>
> +++ b/src/ipa/raspberrypi/controller/controller.cpp<br>
> @@ -20,6 +20,31 @@ using namespace libcamera;<br>
><br>
> LOG_DEFINE_CATEGORY(RPiController)<br>
><br>
> +static const std::map<std::string, Controller::HardwareConfig> HardwareConfigMap = {<br>
> + {<br>
> + "bcm2835",<br>
> + {<br>
> + /*<br>
> + * There are only ever 15 AGC regions computed by the firmware<br>
> + * due to zoning, but the HW defines AGC_REGIONS == 16!<br>
> + */<br>
> + .agcRegions = { 15 , 1 },<br>
> + .agcZoneWeights = { 15 , 1 },<br>
> + .awbRegions = { 16, 12 },<br>
> + .focusRegions = { 4, 3 },<br>
> + .numHistogramBins = 128,<br>
> + .numGammaPoints = 33,<br>
> + .pipelineWidth = 13<br>
> + }<br>
> + },<br>
> + /* For error handling. */<br>
> + {<br>
> + "error",<br>
> + {<br>
> + }<br>
> + }<br>
<br>
Do you need this ?<br></blockquote><div><br></div><div>Good question. I think the answer is no. </div><div>We validate the platform on ipa init, so really we cannot get into this error state.</div><div><br></div><div>I'll remove it in the next version.</div><div><br></div><div>Regards,</div><div>Naush</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> +};<br>
> +<br>
> Controller::Controller()<br>
> : switchModeCalled_(false)<br>
> {<br>
> @@ -148,3 +173,17 @@ const std::string &Controller::getTarget() const<br>
> {<br>
> return target_;<br>
> }<br>
> +<br>
> +const Controller::HardwareConfig &Controller::getHardwareConfig() const<br>
> +{<br>
> + auto cfg = HardwareConfigMap.find(getTarget());<br>
> +<br>
> + /*<br>
> + * This really should not happen, the IPA ought to validate the target<br>
> + * on initialisation.<br>
> + */<br>
> + if (cfg == HardwareConfigMap.end())<br>
> + return HardwareConfigMap.at("error");<br>
<br>
Could you just return {} here and remove "error" from the map ?<br>
<br>
> +<br>
> + return cfg->second;<br>
> +}<br>
> diff --git a/src/ipa/raspberrypi/controller/controller.h b/src/ipa/raspberrypi/controller/controller.h<br>
> index 24e02903d438..c6af5cd6c7d4 100644<br>
> --- a/src/ipa/raspberrypi/controller/controller.h<br>
> +++ b/src/ipa/raspberrypi/controller/controller.h<br>
> @@ -37,6 +37,16 @@ typedef std::unique_ptr<Algorithm> AlgorithmPtr;<br>
> class Controller<br>
> {<br>
> public:<br>
> + struct HardwareConfig {<br>
> + libcamera::Size agcRegions;<br>
> + libcamera::Size agcZoneWeights;<br>
> + libcamera::Size awbRegions;<br>
> + libcamera::Size focusRegions;<br>
> + unsigned int numHistogramBins;<br>
> + unsigned int numGammaPoints;<br>
> + unsigned int pipelineWidth;<br>
> + };<br>
> +<br>
> Controller();<br>
> ~Controller();<br>
> int read(char const *filename);<br>
> @@ -47,6 +57,7 @@ public:<br>
> Metadata &getGlobalMetadata();<br>
> Algorithm *getAlgorithm(std::string const &name) const;<br>
> const std::string &getTarget() const;<br>
> + const HardwareConfig &getHardwareConfig() const;<br>
><br>
> protected:<br>
> int createAlgorithm(const std::string &name, const libcamera::YamlObject ¶ms);<br>
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp<br>
> index 86359538cf67..b64cb96e2dde 100644<br>
> --- a/src/ipa/raspberrypi/raspberrypi.cpp<br>
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp<br>
> @@ -1392,20 +1392,19 @@ RPiController::StatisticsPtr IPARPi::fillStatistics(bcm2835_isp_stats *stats) co<br>
> {<br>
> using namespace RPiController;<br>
><br>
> + const Controller::HardwareConfig &hw = controller_.getHardwareConfig();<br>
> unsigned int i;<br>
> StatisticsPtr statistics =<br>
> std::make_unique<Statistics>(Statistics::AgcStatsPos::PreWb, Statistics::ColourStatsPos::PostLsc);<br>
><br>
> /* RGB histograms are not used, so do not populate them. */<br>
> - statistics->yHist = RPiController::Histogram(stats->hist[0].g_hist, NUM_HISTOGRAM_BINS);<br>
> + statistics->yHist = RPiController::Histogram(stats->hist[0].g_hist,<br>
> + hw.numHistogramBins);<br>
><br>
> - /*<br>
> - * All region sums are based on a 13-bit pipeline bit-depth. Normalise<br>
> - * this to 16-bits for the AGC/AWB/ALSC algorithms.<br>
> - */<br>
> - constexpr unsigned int scale = Statistics::NormalisationFactorPow2 - 13;<br>
> + /* All region sums are based on a 16-bit normalised pipeline bit-depth. */<br>
> + unsigned int scale = Statistics::NormalisationFactorPow2 - hw.pipelineWidth;<br>
><br>
> - statistics->awbRegions.init({ DEFAULT_AWB_REGIONS_X, DEFAULT_AWB_REGIONS_Y });<br>
> + statistics->awbRegions.init(hw.awbRegions);<br>
> for (i = 0; i < statistics->awbRegions.numRegions(); i++)<br>
> statistics->awbRegions.set(i, { { stats->awb_stats[i].r_sum << scale,<br>
> stats->awb_stats[i].g_sum << scale,<br>
> @@ -1413,11 +1412,7 @@ RPiController::StatisticsPtr IPARPi::fillStatistics(bcm2835_isp_stats *stats) co<br>
> stats->awb_stats[i].counted,<br>
> stats->awb_stats[i].notcounted });<br>
><br>
> - /*<br>
> - * There are only ever 15 regions computed by the firmware due to zoning,<br>
> - * but the HW defines AGC_REGIONS == 16!<br>
> - */<br>
> - statistics->agcRegions.init(15);<br>
> + statistics->agcRegions.init(hw.agcRegions);<br>
> for (i = 0; i < statistics->agcRegions.numRegions(); i++)<br>
> statistics->agcRegions.set(i, { { stats->agc_stats[i].r_sum << scale,<br>
> stats->agc_stats[i].g_sum << scale,<br>
> @@ -1425,7 +1420,7 @@ RPiController::StatisticsPtr IPARPi::fillStatistics(bcm2835_isp_stats *stats) co<br>
> stats->agc_stats[i].counted,<br>
> stats->awb_stats[i].notcounted });<br>
><br>
> - statistics->focusRegions.init({ 4, 3 });<br>
> + statistics->focusRegions.init(hw.focusRegions);<br>
> for (i = 0; i < statistics->focusRegions.numRegions(); i++)<br>
> statistics->focusRegions.set(i, { stats->focus_stats[i].contrast_val[1][1] / 1000,<br>
> stats->focus_stats[i].contrast_val_num[1][1],<br>
> --<br>
> 2.34.1<br>
><br>
</blockquote></div></div>