[libcamera-devel,v2,02/10] ipa: raspberrypi: Add hardware configuration to the controller

18471 diff mbox series

Add a new Controller::HardwareConfig structure that captures the
hardware statistics grid/histogram sizes and pipeline widths. This
ensures there is a single centralised places for these parameters.

Add a getHardwareConfig() helper function to retrieve these values for a
given hardware target.

Update the statistics populating routine in the IPA to use the values
from this structure instead of the hardcoded numbers.

Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
---
 src/ipa/raspberrypi/controller/algorithm.h    |  4 +++
 src/ipa/raspberrypi/controller/controller.cpp | 31 +++++++++++++++++++
 src/ipa/raspberrypi/controller/controller.h   | 11 +++++++
 src/ipa/raspberrypi/raspberrypi.cpp           | 21 +++++--------
 4 files changed, 54 insertions(+), 13 deletions(-)

Quoting Naushir Patuck via libcamera-devel (2023-03-27 13:20:22)
> Add a new Controller::HardwareConfig structure that captures the
> hardware statistics grid/histogram sizes and pipeline widths. This
> ensures there is a single centralised places for these parameters.
> 
> Add a getHardwareConfig() helper function to retrieve these values for a
> given hardware target.
> 
> Update the statistics populating routine in the IPA to use the values
> from this structure instead of the hardcoded numbers.
> 
> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> Reviewed-by: David Plowman <david.plowman@raspberrypi.com>

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

> ---
>  src/ipa/raspberrypi/controller/algorithm.h    |  4 +++
>  src/ipa/raspberrypi/controller/controller.cpp | 31 +++++++++++++++++++
>  src/ipa/raspberrypi/controller/controller.h   | 11 +++++++
>  src/ipa/raspberrypi/raspberrypi.cpp           | 21 +++++--------
>  4 files changed, 54 insertions(+), 13 deletions(-)
> 
> diff --git a/src/ipa/raspberrypi/controller/algorithm.h b/src/ipa/raspberrypi/controller/algorithm.h
> index 7c22fbe4945c..4aa814ebbebd 100644
> --- a/src/ipa/raspberrypi/controller/algorithm.h
> +++ b/src/ipa/raspberrypi/controller/algorithm.h
> @@ -45,6 +45,10 @@ public:
>         {
>                 return controller_->getTarget();
>         }
> +       const Controller::HardwareConfig &getHardwareConfig() const
> +       {
> +               return controller_->getHardwareConfig();
> +       }
>  
>  private:
>         Controller *controller_;
> diff --git a/src/ipa/raspberrypi/controller/controller.cpp b/src/ipa/raspberrypi/controller/controller.cpp
> index a6250ee140b0..fa1721130783 100644
> --- a/src/ipa/raspberrypi/controller/controller.cpp
> +++ b/src/ipa/raspberrypi/controller/controller.cpp
> @@ -20,6 +20,25 @@ using namespace libcamera;
>  
>  LOG_DEFINE_CATEGORY(RPiController)
>  
> +static const std::map<std::string, Controller::HardwareConfig> HardwareConfigMap = {
> +       {
> +               "bcm2835",
> +               {
> +                       /*
> +                        * There are only ever 15 AGC regions computed by the firmware
> +                        * due to zoning, but the HW defines AGC_REGIONS == 16!
> +                        */
> +                       .agcRegions = { 15 , 1 },
> +                       .agcZoneWeights = { 15 , 1 },
> +                       .awbRegions = { 16, 12 },
> +                       .focusRegions = { 4, 3 },
> +                       .numHistogramBins = 128,
> +                       .numGammaPoints = 33,
> +                       .pipelineWidth = 13
> +               }
> +       },
> +};
> +
>  Controller::Controller()
>         : switchModeCalled_(false)
>  {
> @@ -148,3 +167,15 @@ const std::string &Controller::getTarget() const
>  {
>         return target_;
>  }
> +
> +const Controller::HardwareConfig &Controller::getHardwareConfig() const
> +{
> +       auto cfg = HardwareConfigMap.find(getTarget());
> +
> +       /*
> +        * This really should not happen, the IPA ought to validate the target
> +        * on initialisation.
> +        */
> +       ASSERT(cfg != HardwareConfigMap.end());
> +       return cfg->second;
> +}
> diff --git a/src/ipa/raspberrypi/controller/controller.h b/src/ipa/raspberrypi/controller/controller.h
> index 24e02903d438..c6af5cd6c7d4 100644
> --- a/src/ipa/raspberrypi/controller/controller.h
> +++ b/src/ipa/raspberrypi/controller/controller.h
> @@ -37,6 +37,16 @@ typedef std::unique_ptr<Algorithm> AlgorithmPtr;
>  class Controller
>  {
>  public:
> +       struct HardwareConfig {
> +               libcamera::Size agcRegions;
> +               libcamera::Size agcZoneWeights;
> +               libcamera::Size awbRegions;
> +               libcamera::Size focusRegions;
> +               unsigned int numHistogramBins;
> +               unsigned int numGammaPoints;
> +               unsigned int pipelineWidth;
> +       };
> +
>         Controller();
>         ~Controller();
>         int read(char const *filename);
> @@ -47,6 +57,7 @@ public:
>         Metadata &getGlobalMetadata();
>         Algorithm *getAlgorithm(std::string const &name) const;
>         const std::string &getTarget() const;
> +       const HardwareConfig &getHardwareConfig() const;
>  
>  protected:
>         int createAlgorithm(const std::string &name, const libcamera::YamlObject &params);
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> index 86359538cf67..b64cb96e2dde 100644
> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> @@ -1392,20 +1392,19 @@ RPiController::StatisticsPtr IPARPi::fillStatistics(bcm2835_isp_stats *stats) co
>  {
>         using namespace RPiController;
>  
> +       const Controller::HardwareConfig &hw = controller_.getHardwareConfig();
>         unsigned int i;
>         StatisticsPtr statistics =
>                 std::make_unique<Statistics>(Statistics::AgcStatsPos::PreWb, Statistics::ColourStatsPos::PostLsc);
>  
>         /* RGB histograms are not used, so do not populate them. */
> -       statistics->yHist = RPiController::Histogram(stats->hist[0].g_hist, NUM_HISTOGRAM_BINS);
> +       statistics->yHist = RPiController::Histogram(stats->hist[0].g_hist,
> +                                                    hw.numHistogramBins);
>  
> -       /*
> -        * All region sums are based on a 13-bit pipeline bit-depth. Normalise
> -        * this to 16-bits for the AGC/AWB/ALSC algorithms.
> -        */
> -       constexpr unsigned int scale = Statistics::NormalisationFactorPow2 - 13;
> +       /* All region sums are based on a 16-bit normalised pipeline bit-depth. */
> +       unsigned int scale = Statistics::NormalisationFactorPow2 - hw.pipelineWidth;
>  
> -       statistics->awbRegions.init({ DEFAULT_AWB_REGIONS_X, DEFAULT_AWB_REGIONS_Y });
> +       statistics->awbRegions.init(hw.awbRegions);
>         for (i = 0; i < statistics->awbRegions.numRegions(); i++)
>                 statistics->awbRegions.set(i, { { stats->awb_stats[i].r_sum << scale,
>                                                   stats->awb_stats[i].g_sum << scale,
> @@ -1413,11 +1412,7 @@ RPiController::StatisticsPtr IPARPi::fillStatistics(bcm2835_isp_stats *stats) co
>                                                 stats->awb_stats[i].counted,
>                                                 stats->awb_stats[i].notcounted });
>  
> -       /*
> -        * There are only ever 15 regions computed by the firmware due to zoning,
> -        * but the HW defines AGC_REGIONS == 16!
> -        */
> -       statistics->agcRegions.init(15);
> +       statistics->agcRegions.init(hw.agcRegions);
>         for (i = 0; i < statistics->agcRegions.numRegions(); i++)
>                 statistics->agcRegions.set(i, { { stats->agc_stats[i].r_sum << scale,
>                                                   stats->agc_stats[i].g_sum << scale,
> @@ -1425,7 +1420,7 @@ RPiController::StatisticsPtr IPARPi::fillStatistics(bcm2835_isp_stats *stats) co
>                                                 stats->agc_stats[i].counted,
>                                                 stats->awb_stats[i].notcounted });
>  
> -       statistics->focusRegions.init({ 4, 3 });
> +       statistics->focusRegions.init(hw.focusRegions);
>         for (i = 0; i < statistics->focusRegions.numRegions(); i++)
>                 statistics->focusRegions.set(i, { stats->focus_stats[i].contrast_val[1][1] / 1000,
>                                                   stats->focus_stats[i].contrast_val_num[1][1],
> -- 
> 2.34.1
>