[libcamera-devel] [PATCH v1 02/10] ipa: raspberrypi: Add hardware configuration to the controller
Naushir Patuck
naush at raspberrypi.com
Fri Mar 24 08:52:42 CET 2023
Hi Jacopo,
Thank you for your review!
On Thu, 23 Mar 2023 at 16:11, Jacopo Mondi <jacopo.mondi at ideasonboard.com>
wrote:
> Hi Naush
>
> On Wed, Mar 22, 2023 at 01:06:04PM +0000, Naushir Patuck via
> libcamera-devel wrote:
> > 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 at raspberrypi.com>
> > Reviewed-by: David Plowman <david.plowman at raspberrypi.com>
> > ---
> > src/ipa/raspberrypi/controller/algorithm.h | 4 ++
> > src/ipa/raspberrypi/controller/controller.cpp | 39 +++++++++++++++++++
> > src/ipa/raspberrypi/controller/controller.h | 11 ++++++
> > src/ipa/raspberrypi/raspberrypi.cpp | 21 ++++------
> > 4 files changed, 62 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();
> > + }
> >
>
> Will this be used ?
>
Yes, in subsequent patches :)
>
> > private:
> > Controller *controller_;
> > diff --git a/src/ipa/raspberrypi/controller/controller.cpp
> b/src/ipa/raspberrypi/controller/controller.cpp
> > index a6250ee140b0..2c7517aec6b4 100644
> > --- a/src/ipa/raspberrypi/controller/controller.cpp
> > +++ b/src/ipa/raspberrypi/controller/controller.cpp
> > @@ -20,6 +20,31 @@ 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
> > + }
> > + },
> > + /* For error handling. */
> > + {
> > + "error",
> > + {
> > + }
> > + }
>
> Do you need this ?
>
Good question. I think the answer is no.
We validate the platform on ipa init, so really we cannot get into this
error state.
I'll remove it in the next version.
Regards,
Naush
>
> > +};
> > +
> > Controller::Controller()
> > : switchModeCalled_(false)
> > {
> > @@ -148,3 +173,17 @@ 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.
> > + */
> > + if (cfg == HardwareConfigMap.end())
> > + return HardwareConfigMap.at("error");
>
> Could you just return {} here and remove "error" from the map ?
>
> > +
> > + 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 ¶ms);
> > 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
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20230324/0999b953/attachment.htm>
More information about the libcamera-devel
mailing list