| Message ID | 20210812165243.276977-5-jeanmichel.hautbois@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
On 12/08/2021 17:52, Jean-Michel Hautbois wrote: > When a new ipu3_uapi_params buffer is ready, IPAIPU3 receives a > EventFillParams. When this is called, the algorithms should fill every > field in the parameter structure they need to update. > > Demonstrate it by adding a loop on the prepare() call in IPAIPU3 and let I don't think we need to demonstrate it, we 'implement' it ;-) > the Grid algorithm update the grid field. > This leads to removing this same update from the Awb algorithm. > > Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> > --- > src/ipa/ipu3/algorithms/algorithm.h | 2 ++ > src/ipa/ipu3/algorithms/grid.cpp | 6 ++++++ > src/ipa/ipu3/algorithms/grid.h | 1 + > src/ipa/ipu3/ipu3.cpp | 3 +++ > src/ipa/ipu3/ipu3_awb.cpp | 1 - > 5 files changed, 12 insertions(+), 1 deletion(-) > > diff --git a/src/ipa/ipu3/algorithms/algorithm.h b/src/ipa/ipu3/algorithms/algorithm.h > index c1b37276..b571f55a 100644 > --- a/src/ipa/ipu3/algorithms/algorithm.h > +++ b/src/ipa/ipu3/algorithms/algorithm.h > @@ -26,6 +26,8 @@ public: > { > return 0; > } > + > + virtual void prepare([[maybe_unused]] IPAContext &context, [[maybe_unused]] ipu3_uapi_params ¶ms) {} > }; > > } /* namespace ipa::ipu3 */ > diff --git a/src/ipa/ipu3/algorithms/grid.cpp b/src/ipa/ipu3/algorithms/grid.cpp > index 3578f41b..e42a760d 100644 > --- a/src/ipa/ipu3/algorithms/grid.cpp > +++ b/src/ipa/ipu3/algorithms/grid.cpp > @@ -78,6 +78,12 @@ int Grid::configure(IPAContext &context, const IPAConfigInfo &configInfo) > return 0; > } > > +void Grid::prepare(IPAContext &context, ipu3_uapi_params ¶ms) > +{ > + /* Update the IPU3 parameters with new calculated grid */ I don't think it's a new calculated grid, as the grid is only calculated during the configure phase. I would say: Update the IPU3 parameters with the configured grid Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > + params.acc_param.awb.config.grid = context.configuration.grid.bdsGrid; > +} > + > } /* namespace ipa::ipu3::algorithms */ > > } /* namespace libcamera */ > diff --git a/src/ipa/ipu3/algorithms/grid.h b/src/ipa/ipu3/algorithms/grid.h > index b4a51b42..89399bd2 100644 > --- a/src/ipa/ipu3/algorithms/grid.h > +++ b/src/ipa/ipu3/algorithms/grid.h > @@ -19,6 +19,7 @@ public: > ~Grid() = default; > > int configure(IPAContext &context, const IPAConfigInfo &configInfo) override; > + void prepare(IPAContext &context, ipu3_uapi_params ¶ms) override; > }; > > } /* namespace ipa::ipu3::algorithms */ > diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp > index ef7fec86..394a7a45 100644 > --- a/src/ipa/ipu3/ipu3.cpp > +++ b/src/ipa/ipu3/ipu3.cpp > @@ -305,6 +305,9 @@ void IPAIPU3::processControls([[maybe_unused]] unsigned int frame, > > void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params) > { > + for (auto const &algo : algorithms_) > + algo->prepare(context_, params_); > + > if (agcAlgo_->updateControls()) > awbAlgo_->updateWbParameters(params_, agcAlgo_->gamma()); > > diff --git a/src/ipa/ipu3/ipu3_awb.cpp b/src/ipa/ipu3/ipu3_awb.cpp > index 4ee5ee6f..911760b3 100644 > --- a/src/ipa/ipu3/ipu3_awb.cpp > +++ b/src/ipa/ipu3/ipu3_awb.cpp > @@ -159,7 +159,6 @@ void IPU3Awb::initialise(ipu3_uapi_params ¶ms, const Size &bdsOutputSize, st > params.acc_param.awb.config.rgbs_thr_r = 8191; > params.acc_param.awb.config.rgbs_thr_gb = 8191; > params.acc_param.awb.config.rgbs_thr_b = 8191 | IPU3_UAPI_AWB_RGBS_THR_B_EN | IPU3_UAPI_AWB_RGBS_THR_B_INCL_SAT; > - params.acc_param.awb.config.grid = bdsGrid; > awbGrid_ = bdsGrid; > > params.use.acc_bnr = 1; >
Hi Jean-Michel, Thank you for the patch. On Thu, Aug 12, 2021 at 06:52:37PM +0200, Jean-Michel Hautbois wrote: > When a new ipu3_uapi_params buffer is ready, IPAIPU3 receives a > EventFillParams. When this is called, the algorithms should fill every > field in the parameter structure they need to update. > > Demonstrate it by adding a loop on the prepare() call in IPAIPU3 and let > the Grid algorithm update the grid field. > This leads to removing this same update from the Awb algorithm. > > Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> > --- > src/ipa/ipu3/algorithms/algorithm.h | 2 ++ > src/ipa/ipu3/algorithms/grid.cpp | 6 ++++++ > src/ipa/ipu3/algorithms/grid.h | 1 + > src/ipa/ipu3/ipu3.cpp | 3 +++ > src/ipa/ipu3/ipu3_awb.cpp | 1 - > 5 files changed, 12 insertions(+), 1 deletion(-) > > diff --git a/src/ipa/ipu3/algorithms/algorithm.h b/src/ipa/ipu3/algorithms/algorithm.h > index c1b37276..b571f55a 100644 > --- a/src/ipa/ipu3/algorithms/algorithm.h > +++ b/src/ipa/ipu3/algorithms/algorithm.h > @@ -26,6 +26,8 @@ public: > { > return 0; > } > + > + virtual void prepare([[maybe_unused]] IPAContext &context, [[maybe_unused]] ipu3_uapi_params ¶ms) {} Line wrap. Could you add a patch after 02/10 that adds all functions (configure(), prepare() and process()) to the Algorithm class, and call them in the IPAIPU3 class as appropriate ? That patch should include the documentation of the functions, to explain how they're used. After that the rest of the series can begin moving the algorithms to the new class. > }; > > } /* namespace ipa::ipu3 */ > diff --git a/src/ipa/ipu3/algorithms/grid.cpp b/src/ipa/ipu3/algorithms/grid.cpp > index 3578f41b..e42a760d 100644 > --- a/src/ipa/ipu3/algorithms/grid.cpp > +++ b/src/ipa/ipu3/algorithms/grid.cpp > @@ -78,6 +78,12 @@ int Grid::configure(IPAContext &context, const IPAConfigInfo &configInfo) > return 0; > } > > +void Grid::prepare(IPAContext &context, ipu3_uapi_params ¶ms) > +{ > + /* Update the IPU3 parameters with new calculated grid */ > + params.acc_param.awb.config.grid = context.configuration.grid.bdsGrid; And this is where the spaghetti dish begins :-( It should be the responsibility of the AWB algorithm to set params.acc_param.awb. > +} > + > } /* namespace ipa::ipu3::algorithms */ > > } /* namespace libcamera */ > diff --git a/src/ipa/ipu3/algorithms/grid.h b/src/ipa/ipu3/algorithms/grid.h > index b4a51b42..89399bd2 100644 > --- a/src/ipa/ipu3/algorithms/grid.h > +++ b/src/ipa/ipu3/algorithms/grid.h > @@ -19,6 +19,7 @@ public: > ~Grid() = default; > > int configure(IPAContext &context, const IPAConfigInfo &configInfo) override; > + void prepare(IPAContext &context, ipu3_uapi_params ¶ms) override; > }; > > } /* namespace ipa::ipu3::algorithms */ > diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp > index ef7fec86..394a7a45 100644 > --- a/src/ipa/ipu3/ipu3.cpp > +++ b/src/ipa/ipu3/ipu3.cpp > @@ -305,6 +305,9 @@ void IPAIPU3::processControls([[maybe_unused]] unsigned int frame, > > void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params) > { > + for (auto const &algo : algorithms_) > + algo->prepare(context_, params_); > + > if (agcAlgo_->updateControls()) > awbAlgo_->updateWbParameters(params_, agcAlgo_->gamma()); > > diff --git a/src/ipa/ipu3/ipu3_awb.cpp b/src/ipa/ipu3/ipu3_awb.cpp > index 4ee5ee6f..911760b3 100644 > --- a/src/ipa/ipu3/ipu3_awb.cpp > +++ b/src/ipa/ipu3/ipu3_awb.cpp > @@ -159,7 +159,6 @@ void IPU3Awb::initialise(ipu3_uapi_params ¶ms, const Size &bdsOutputSize, st > params.acc_param.awb.config.rgbs_thr_r = 8191; > params.acc_param.awb.config.rgbs_thr_gb = 8191; > params.acc_param.awb.config.rgbs_thr_b = 8191 | IPU3_UAPI_AWB_RGBS_THR_B_EN | IPU3_UAPI_AWB_RGBS_THR_B_INCL_SAT; > - params.acc_param.awb.config.grid = bdsGrid; > awbGrid_ = bdsGrid; > > params.use.acc_bnr = 1;
Hi Laurent, On 15/08/2021 20:02, Laurent Pinchart wrote: > Hi Jean-Michel, > > Thank you for the patch. > > On Thu, Aug 12, 2021 at 06:52:37PM +0200, Jean-Michel Hautbois wrote: >> When a new ipu3_uapi_params buffer is ready, IPAIPU3 receives a >> EventFillParams. When this is called, the algorithms should fill every >> field in the parameter structure they need to update. >> >> Demonstrate it by adding a loop on the prepare() call in IPAIPU3 and let >> the Grid algorithm update the grid field. >> This leads to removing this same update from the Awb algorithm. >> >> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> >> --- >> src/ipa/ipu3/algorithms/algorithm.h | 2 ++ >> src/ipa/ipu3/algorithms/grid.cpp | 6 ++++++ >> src/ipa/ipu3/algorithms/grid.h | 1 + >> src/ipa/ipu3/ipu3.cpp | 3 +++ >> src/ipa/ipu3/ipu3_awb.cpp | 1 - >> 5 files changed, 12 insertions(+), 1 deletion(-) >> >> diff --git a/src/ipa/ipu3/algorithms/algorithm.h b/src/ipa/ipu3/algorithms/algorithm.h >> index c1b37276..b571f55a 100644 >> --- a/src/ipa/ipu3/algorithms/algorithm.h >> +++ b/src/ipa/ipu3/algorithms/algorithm.h >> @@ -26,6 +26,8 @@ public: >> { >> return 0; >> } >> + >> + virtual void prepare([[maybe_unused]] IPAContext &context, [[maybe_unused]] ipu3_uapi_params ¶ms) {} > > Line wrap. > > Could you add a patch after 02/10 that adds all functions (configure(), > prepare() and process()) to the Algorithm class, and call them in the > IPAIPU3 class as appropriate ? That patch should include the > documentation of the functions, to explain how they're used. After that > the rest of the series can begin moving the algorithms to the new class. > I did that in the first place, and it was a bit of a heavy patch. So I decided to split it to introduce each of configure/prepare/process calls step by step... >> }; >> >> } /* namespace ipa::ipu3 */ >> diff --git a/src/ipa/ipu3/algorithms/grid.cpp b/src/ipa/ipu3/algorithms/grid.cpp >> index 3578f41b..e42a760d 100644 >> --- a/src/ipa/ipu3/algorithms/grid.cpp >> +++ b/src/ipa/ipu3/algorithms/grid.cpp >> @@ -78,6 +78,12 @@ int Grid::configure(IPAContext &context, const IPAConfigInfo &configInfo) >> return 0; >> } >> >> +void Grid::prepare(IPAContext &context, ipu3_uapi_params ¶ms) >> +{ >> + /* Update the IPU3 parameters with new calculated grid */ >> + params.acc_param.awb.config.grid = context.configuration.grid.bdsGrid; > > And this is where the spaghetti dish begins :-( It should be the > responsibility of the AWB algorithm to set params.acc_param.awb. > Yes, I moved it at least 5 times while writing the patches as I could not decide where it should be :-p. >> +} >> + >> } /* namespace ipa::ipu3::algorithms */ >> >> } /* namespace libcamera */ >> diff --git a/src/ipa/ipu3/algorithms/grid.h b/src/ipa/ipu3/algorithms/grid.h >> index b4a51b42..89399bd2 100644 >> --- a/src/ipa/ipu3/algorithms/grid.h >> +++ b/src/ipa/ipu3/algorithms/grid.h >> @@ -19,6 +19,7 @@ public: >> ~Grid() = default; >> >> int configure(IPAContext &context, const IPAConfigInfo &configInfo) override; >> + void prepare(IPAContext &context, ipu3_uapi_params ¶ms) override; >> }; >> >> } /* namespace ipa::ipu3::algorithms */ >> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp >> index ef7fec86..394a7a45 100644 >> --- a/src/ipa/ipu3/ipu3.cpp >> +++ b/src/ipa/ipu3/ipu3.cpp >> @@ -305,6 +305,9 @@ void IPAIPU3::processControls([[maybe_unused]] unsigned int frame, >> >> void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params) >> { >> + for (auto const &algo : algorithms_) >> + algo->prepare(context_, params_); >> + >> if (agcAlgo_->updateControls()) >> awbAlgo_->updateWbParameters(params_, agcAlgo_->gamma()); >> >> diff --git a/src/ipa/ipu3/ipu3_awb.cpp b/src/ipa/ipu3/ipu3_awb.cpp >> index 4ee5ee6f..911760b3 100644 >> --- a/src/ipa/ipu3/ipu3_awb.cpp >> +++ b/src/ipa/ipu3/ipu3_awb.cpp >> @@ -159,7 +159,6 @@ void IPU3Awb::initialise(ipu3_uapi_params ¶ms, const Size &bdsOutputSize, st >> params.acc_param.awb.config.rgbs_thr_r = 8191; >> params.acc_param.awb.config.rgbs_thr_gb = 8191; >> params.acc_param.awb.config.rgbs_thr_b = 8191 | IPU3_UAPI_AWB_RGBS_THR_B_EN | IPU3_UAPI_AWB_RGBS_THR_B_INCL_SAT; >> - params.acc_param.awb.config.grid = bdsGrid; >> awbGrid_ = bdsGrid; >> >> params.use.acc_bnr = 1; >
Hi Jean-Michel, On Mon, Aug 16, 2021 at 10:01:14AM +0200, Jean-Michel Hautbois wrote: > On 15/08/2021 20:02, Laurent Pinchart wrote: > > On Thu, Aug 12, 2021 at 06:52:37PM +0200, Jean-Michel Hautbois wrote: > >> When a new ipu3_uapi_params buffer is ready, IPAIPU3 receives a > >> EventFillParams. When this is called, the algorithms should fill every > >> field in the parameter structure they need to update. > >> > >> Demonstrate it by adding a loop on the prepare() call in IPAIPU3 and let > >> the Grid algorithm update the grid field. > >> This leads to removing this same update from the Awb algorithm. > >> > >> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> > >> --- > >> src/ipa/ipu3/algorithms/algorithm.h | 2 ++ > >> src/ipa/ipu3/algorithms/grid.cpp | 6 ++++++ > >> src/ipa/ipu3/algorithms/grid.h | 1 + > >> src/ipa/ipu3/ipu3.cpp | 3 +++ > >> src/ipa/ipu3/ipu3_awb.cpp | 1 - > >> 5 files changed, 12 insertions(+), 1 deletion(-) > >> > >> diff --git a/src/ipa/ipu3/algorithms/algorithm.h b/src/ipa/ipu3/algorithms/algorithm.h > >> index c1b37276..b571f55a 100644 > >> --- a/src/ipa/ipu3/algorithms/algorithm.h > >> +++ b/src/ipa/ipu3/algorithms/algorithm.h > >> @@ -26,6 +26,8 @@ public: > >> { > >> return 0; > >> } > >> + > >> + virtual void prepare([[maybe_unused]] IPAContext &context, [[maybe_unused]] ipu3_uapi_params ¶ms) {} > > > > Line wrap. > > > > Could you add a patch after 02/10 that adds all functions (configure(), > > prepare() and process()) to the Algorithm class, and call them in the > > IPAIPU3 class as appropriate ? That patch should include the > > documentation of the functions, to explain how they're used. After that > > the rest of the series can begin moving the algorithms to the new class. > > I did that in the first place, and it was a bit of a heavy patch. So I > decided to split it to introduce each of configure/prepare/process calls > step by step... The issue with this approach is that it's harder to have an overview of the proposed API. Is it *that* heavy to define a class with three abstract functions and their documentation ? > >> }; > >> > >> } /* namespace ipa::ipu3 */ > >> diff --git a/src/ipa/ipu3/algorithms/grid.cpp b/src/ipa/ipu3/algorithms/grid.cpp > >> index 3578f41b..e42a760d 100644 > >> --- a/src/ipa/ipu3/algorithms/grid.cpp > >> +++ b/src/ipa/ipu3/algorithms/grid.cpp > >> @@ -78,6 +78,12 @@ int Grid::configure(IPAContext &context, const IPAConfigInfo &configInfo) > >> return 0; > >> } > >> > >> +void Grid::prepare(IPAContext &context, ipu3_uapi_params ¶ms) > >> +{ > >> + /* Update the IPU3 parameters with new calculated grid */ > >> + params.acc_param.awb.config.grid = context.configuration.grid.bdsGrid; > > > > And this is where the spaghetti dish begins :-( It should be the > > responsibility of the AWB algorithm to set params.acc_param.awb. > > Yes, I moved it at least 5 times while writing the patches as I could > not decide where it should be :-p. As commented in patch 03/10, I don't think "grid" should be an algorithm. > >> +} > >> + > >> } /* namespace ipa::ipu3::algorithms */ > >> > >> } /* namespace libcamera */ > >> diff --git a/src/ipa/ipu3/algorithms/grid.h b/src/ipa/ipu3/algorithms/grid.h > >> index b4a51b42..89399bd2 100644 > >> --- a/src/ipa/ipu3/algorithms/grid.h > >> +++ b/src/ipa/ipu3/algorithms/grid.h > >> @@ -19,6 +19,7 @@ public: > >> ~Grid() = default; > >> > >> int configure(IPAContext &context, const IPAConfigInfo &configInfo) override; > >> + void prepare(IPAContext &context, ipu3_uapi_params ¶ms) override; > >> }; > >> > >> } /* namespace ipa::ipu3::algorithms */ > >> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp > >> index ef7fec86..394a7a45 100644 > >> --- a/src/ipa/ipu3/ipu3.cpp > >> +++ b/src/ipa/ipu3/ipu3.cpp > >> @@ -305,6 +305,9 @@ void IPAIPU3::processControls([[maybe_unused]] unsigned int frame, > >> > >> void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params) > >> { > >> + for (auto const &algo : algorithms_) > >> + algo->prepare(context_, params_); > >> + > >> if (agcAlgo_->updateControls()) > >> awbAlgo_->updateWbParameters(params_, agcAlgo_->gamma()); > >> > >> diff --git a/src/ipa/ipu3/ipu3_awb.cpp b/src/ipa/ipu3/ipu3_awb.cpp > >> index 4ee5ee6f..911760b3 100644 > >> --- a/src/ipa/ipu3/ipu3_awb.cpp > >> +++ b/src/ipa/ipu3/ipu3_awb.cpp > >> @@ -159,7 +159,6 @@ void IPU3Awb::initialise(ipu3_uapi_params ¶ms, const Size &bdsOutputSize, st > >> params.acc_param.awb.config.rgbs_thr_r = 8191; > >> params.acc_param.awb.config.rgbs_thr_gb = 8191; > >> params.acc_param.awb.config.rgbs_thr_b = 8191 | IPU3_UAPI_AWB_RGBS_THR_B_EN | IPU3_UAPI_AWB_RGBS_THR_B_INCL_SAT; > >> - params.acc_param.awb.config.grid = bdsGrid; > >> awbGrid_ = bdsGrid; > >> > >> params.use.acc_bnr = 1;
Hi Laurent, On 16/08/2021 11:41, Laurent Pinchart wrote: > Hi Jean-Michel, > > On Mon, Aug 16, 2021 at 10:01:14AM +0200, Jean-Michel Hautbois wrote: >> On 15/08/2021 20:02, Laurent Pinchart wrote: >>> On Thu, Aug 12, 2021 at 06:52:37PM +0200, Jean-Michel Hautbois wrote: >>>> When a new ipu3_uapi_params buffer is ready, IPAIPU3 receives a >>>> EventFillParams. When this is called, the algorithms should fill every >>>> field in the parameter structure they need to update. >>>> >>>> Demonstrate it by adding a loop on the prepare() call in IPAIPU3 and let >>>> the Grid algorithm update the grid field. >>>> This leads to removing this same update from the Awb algorithm. >>>> >>>> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> >>>> --- >>>> src/ipa/ipu3/algorithms/algorithm.h | 2 ++ >>>> src/ipa/ipu3/algorithms/grid.cpp | 6 ++++++ >>>> src/ipa/ipu3/algorithms/grid.h | 1 + >>>> src/ipa/ipu3/ipu3.cpp | 3 +++ >>>> src/ipa/ipu3/ipu3_awb.cpp | 1 - >>>> 5 files changed, 12 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/src/ipa/ipu3/algorithms/algorithm.h b/src/ipa/ipu3/algorithms/algorithm.h >>>> index c1b37276..b571f55a 100644 >>>> --- a/src/ipa/ipu3/algorithms/algorithm.h >>>> +++ b/src/ipa/ipu3/algorithms/algorithm.h >>>> @@ -26,6 +26,8 @@ public: >>>> { >>>> return 0; >>>> } >>>> + >>>> + virtual void prepare([[maybe_unused]] IPAContext &context, [[maybe_unused]] ipu3_uapi_params ¶ms) {} >>> >>> Line wrap. >>> >>> Could you add a patch after 02/10 that adds all functions (configure(), >>> prepare() and process()) to the Algorithm class, and call them in the >>> IPAIPU3 class as appropriate ? That patch should include the >>> documentation of the functions, to explain how they're used. After that >>> the rest of the series can begin moving the algorithms to the new class. >> >> I did that in the first place, and it was a bit of a heavy patch. So I >> decided to split it to introduce each of configure/prepare/process calls >> step by step... > > The issue with this approach is that it's harder to have an overview of > the proposed API. Is it *that* heavy to define a class with three > abstract functions and their documentation ? > No, but as we already have a process() call in AGC it must be rewritten accordingly. >>>> }; >>>> >>>> } /* namespace ipa::ipu3 */ >>>> diff --git a/src/ipa/ipu3/algorithms/grid.cpp b/src/ipa/ipu3/algorithms/grid.cpp >>>> index 3578f41b..e42a760d 100644 >>>> --- a/src/ipa/ipu3/algorithms/grid.cpp >>>> +++ b/src/ipa/ipu3/algorithms/grid.cpp >>>> @@ -78,6 +78,12 @@ int Grid::configure(IPAContext &context, const IPAConfigInfo &configInfo) >>>> return 0; >>>> } >>>> >>>> +void Grid::prepare(IPAContext &context, ipu3_uapi_params ¶ms) >>>> +{ >>>> + /* Update the IPU3 parameters with new calculated grid */ >>>> + params.acc_param.awb.config.grid = context.configuration.grid.bdsGrid; >>> >>> And this is where the spaghetti dish begins :-( It should be the >>> responsibility of the AWB algorithm to set params.acc_param.awb. >> >> Yes, I moved it at least 5 times while writing the patches as I could >> not decide where it should be :-p. > > As commented in patch 03/10, I don't think "grid" should be an > algorithm. > >>>> +} >>>> + >>>> } /* namespace ipa::ipu3::algorithms */ >>>> >>>> } /* namespace libcamera */ >>>> diff --git a/src/ipa/ipu3/algorithms/grid.h b/src/ipa/ipu3/algorithms/grid.h >>>> index b4a51b42..89399bd2 100644 >>>> --- a/src/ipa/ipu3/algorithms/grid.h >>>> +++ b/src/ipa/ipu3/algorithms/grid.h >>>> @@ -19,6 +19,7 @@ public: >>>> ~Grid() = default; >>>> >>>> int configure(IPAContext &context, const IPAConfigInfo &configInfo) override; >>>> + void prepare(IPAContext &context, ipu3_uapi_params ¶ms) override; >>>> }; >>>> >>>> } /* namespace ipa::ipu3::algorithms */ >>>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp >>>> index ef7fec86..394a7a45 100644 >>>> --- a/src/ipa/ipu3/ipu3.cpp >>>> +++ b/src/ipa/ipu3/ipu3.cpp >>>> @@ -305,6 +305,9 @@ void IPAIPU3::processControls([[maybe_unused]] unsigned int frame, >>>> >>>> void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params) >>>> { >>>> + for (auto const &algo : algorithms_) >>>> + algo->prepare(context_, params_); >>>> + >>>> if (agcAlgo_->updateControls()) >>>> awbAlgo_->updateWbParameters(params_, agcAlgo_->gamma()); >>>> >>>> diff --git a/src/ipa/ipu3/ipu3_awb.cpp b/src/ipa/ipu3/ipu3_awb.cpp >>>> index 4ee5ee6f..911760b3 100644 >>>> --- a/src/ipa/ipu3/ipu3_awb.cpp >>>> +++ b/src/ipa/ipu3/ipu3_awb.cpp >>>> @@ -159,7 +159,6 @@ void IPU3Awb::initialise(ipu3_uapi_params ¶ms, const Size &bdsOutputSize, st >>>> params.acc_param.awb.config.rgbs_thr_r = 8191; >>>> params.acc_param.awb.config.rgbs_thr_gb = 8191; >>>> params.acc_param.awb.config.rgbs_thr_b = 8191 | IPU3_UAPI_AWB_RGBS_THR_B_EN | IPU3_UAPI_AWB_RGBS_THR_B_INCL_SAT; >>>> - params.acc_param.awb.config.grid = bdsGrid; >>>> awbGrid_ = bdsGrid; >>>> >>>> params.use.acc_bnr = 1; >
Hi Jean-Michel, On Mon, Aug 16, 2021 at 11:43:54AM +0200, Jean-Michel Hautbois wrote: > On 16/08/2021 11:41, Laurent Pinchart wrote: > > On Mon, Aug 16, 2021 at 10:01:14AM +0200, Jean-Michel Hautbois wrote: > >> On 15/08/2021 20:02, Laurent Pinchart wrote: > >>> On Thu, Aug 12, 2021 at 06:52:37PM +0200, Jean-Michel Hautbois wrote: > >>>> When a new ipu3_uapi_params buffer is ready, IPAIPU3 receives a > >>>> EventFillParams. When this is called, the algorithms should fill every > >>>> field in the parameter structure they need to update. > >>>> > >>>> Demonstrate it by adding a loop on the prepare() call in IPAIPU3 and let > >>>> the Grid algorithm update the grid field. > >>>> This leads to removing this same update from the Awb algorithm. > >>>> > >>>> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> > >>>> --- > >>>> src/ipa/ipu3/algorithms/algorithm.h | 2 ++ > >>>> src/ipa/ipu3/algorithms/grid.cpp | 6 ++++++ > >>>> src/ipa/ipu3/algorithms/grid.h | 1 + > >>>> src/ipa/ipu3/ipu3.cpp | 3 +++ > >>>> src/ipa/ipu3/ipu3_awb.cpp | 1 - > >>>> 5 files changed, 12 insertions(+), 1 deletion(-) > >>>> > >>>> diff --git a/src/ipa/ipu3/algorithms/algorithm.h b/src/ipa/ipu3/algorithms/algorithm.h > >>>> index c1b37276..b571f55a 100644 > >>>> --- a/src/ipa/ipu3/algorithms/algorithm.h > >>>> +++ b/src/ipa/ipu3/algorithms/algorithm.h > >>>> @@ -26,6 +26,8 @@ public: > >>>> { > >>>> return 0; > >>>> } > >>>> + > >>>> + virtual void prepare([[maybe_unused]] IPAContext &context, [[maybe_unused]] ipu3_uapi_params ¶ms) {} > >>> > >>> Line wrap. > >>> > >>> Could you add a patch after 02/10 that adds all functions (configure(), > >>> prepare() and process()) to the Algorithm class, and call them in the > >>> IPAIPU3 class as appropriate ? That patch should include the > >>> documentation of the functions, to explain how they're used. After that > >>> the rest of the series can begin moving the algorithms to the new class. > >> > >> I did that in the first place, and it was a bit of a heavy patch. So I > >> decided to split it to introduce each of configure/prepare/process calls > >> step by step... > > > > The issue with this approach is that it's harder to have an overview of > > the proposed API. Is it *that* heavy to define a class with three > > abstract functions and their documentation ? > > No, but as we already have a process() call in AGC it must be rewritten > accordingly. The existing process() function takes different parameters, so the compiler shouldn't complain. Worst case it could be renamed in a small patch at the beginning of the series to move it out of the way. I agree that adapting the algorithm is best done in a patch separate from the introduction of the new Algorithm structure. > >>>> }; > >>>> > >>>> } /* namespace ipa::ipu3 */ > >>>> diff --git a/src/ipa/ipu3/algorithms/grid.cpp b/src/ipa/ipu3/algorithms/grid.cpp > >>>> index 3578f41b..e42a760d 100644 > >>>> --- a/src/ipa/ipu3/algorithms/grid.cpp > >>>> +++ b/src/ipa/ipu3/algorithms/grid.cpp > >>>> @@ -78,6 +78,12 @@ int Grid::configure(IPAContext &context, const IPAConfigInfo &configInfo) > >>>> return 0; > >>>> } > >>>> > >>>> +void Grid::prepare(IPAContext &context, ipu3_uapi_params ¶ms) > >>>> +{ > >>>> + /* Update the IPU3 parameters with new calculated grid */ > >>>> + params.acc_param.awb.config.grid = context.configuration.grid.bdsGrid; > >>> > >>> And this is where the spaghetti dish begins :-( It should be the > >>> responsibility of the AWB algorithm to set params.acc_param.awb. > >> > >> Yes, I moved it at least 5 times while writing the patches as I could > >> not decide where it should be :-p. > > > > As commented in patch 03/10, I don't think "grid" should be an > > algorithm. > > > >>>> +} > >>>> + > >>>> } /* namespace ipa::ipu3::algorithms */ > >>>> > >>>> } /* namespace libcamera */ > >>>> diff --git a/src/ipa/ipu3/algorithms/grid.h b/src/ipa/ipu3/algorithms/grid.h > >>>> index b4a51b42..89399bd2 100644 > >>>> --- a/src/ipa/ipu3/algorithms/grid.h > >>>> +++ b/src/ipa/ipu3/algorithms/grid.h > >>>> @@ -19,6 +19,7 @@ public: > >>>> ~Grid() = default; > >>>> > >>>> int configure(IPAContext &context, const IPAConfigInfo &configInfo) override; > >>>> + void prepare(IPAContext &context, ipu3_uapi_params ¶ms) override; > >>>> }; > >>>> > >>>> } /* namespace ipa::ipu3::algorithms */ > >>>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp > >>>> index ef7fec86..394a7a45 100644 > >>>> --- a/src/ipa/ipu3/ipu3.cpp > >>>> +++ b/src/ipa/ipu3/ipu3.cpp > >>>> @@ -305,6 +305,9 @@ void IPAIPU3::processControls([[maybe_unused]] unsigned int frame, > >>>> > >>>> void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params) > >>>> { > >>>> + for (auto const &algo : algorithms_) > >>>> + algo->prepare(context_, params_); > >>>> + > >>>> if (agcAlgo_->updateControls()) > >>>> awbAlgo_->updateWbParameters(params_, agcAlgo_->gamma()); > >>>> > >>>> diff --git a/src/ipa/ipu3/ipu3_awb.cpp b/src/ipa/ipu3/ipu3_awb.cpp > >>>> index 4ee5ee6f..911760b3 100644 > >>>> --- a/src/ipa/ipu3/ipu3_awb.cpp > >>>> +++ b/src/ipa/ipu3/ipu3_awb.cpp > >>>> @@ -159,7 +159,6 @@ void IPU3Awb::initialise(ipu3_uapi_params ¶ms, const Size &bdsOutputSize, st > >>>> params.acc_param.awb.config.rgbs_thr_r = 8191; > >>>> params.acc_param.awb.config.rgbs_thr_gb = 8191; > >>>> params.acc_param.awb.config.rgbs_thr_b = 8191 | IPU3_UAPI_AWB_RGBS_THR_B_EN | IPU3_UAPI_AWB_RGBS_THR_B_INCL_SAT; > >>>> - params.acc_param.awb.config.grid = bdsGrid; > >>>> awbGrid_ = bdsGrid; > >>>> > >>>> params.use.acc_bnr = 1;
Hi Laurent, On 16/08/2021 11:49, Laurent Pinchart wrote: > Hi Jean-Michel, > > On Mon, Aug 16, 2021 at 11:43:54AM +0200, Jean-Michel Hautbois wrote: >> On 16/08/2021 11:41, Laurent Pinchart wrote: >>> On Mon, Aug 16, 2021 at 10:01:14AM +0200, Jean-Michel Hautbois wrote: >>>> On 15/08/2021 20:02, Laurent Pinchart wrote: >>>>> On Thu, Aug 12, 2021 at 06:52:37PM +0200, Jean-Michel Hautbois wrote: >>>>>> When a new ipu3_uapi_params buffer is ready, IPAIPU3 receives a >>>>>> EventFillParams. When this is called, the algorithms should fill every >>>>>> field in the parameter structure they need to update. >>>>>> >>>>>> Demonstrate it by adding a loop on the prepare() call in IPAIPU3 and let >>>>>> the Grid algorithm update the grid field. >>>>>> This leads to removing this same update from the Awb algorithm. >>>>>> >>>>>> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> >>>>>> --- >>>>>> src/ipa/ipu3/algorithms/algorithm.h | 2 ++ >>>>>> src/ipa/ipu3/algorithms/grid.cpp | 6 ++++++ >>>>>> src/ipa/ipu3/algorithms/grid.h | 1 + >>>>>> src/ipa/ipu3/ipu3.cpp | 3 +++ >>>>>> src/ipa/ipu3/ipu3_awb.cpp | 1 - >>>>>> 5 files changed, 12 insertions(+), 1 deletion(-) >>>>>> >>>>>> diff --git a/src/ipa/ipu3/algorithms/algorithm.h b/src/ipa/ipu3/algorithms/algorithm.h >>>>>> index c1b37276..b571f55a 100644 >>>>>> --- a/src/ipa/ipu3/algorithms/algorithm.h >>>>>> +++ b/src/ipa/ipu3/algorithms/algorithm.h >>>>>> @@ -26,6 +26,8 @@ public: >>>>>> { >>>>>> return 0; >>>>>> } >>>>>> + >>>>>> + virtual void prepare([[maybe_unused]] IPAContext &context, [[maybe_unused]] ipu3_uapi_params ¶ms) {} >>>>> >>>>> Line wrap. >>>>> >>>>> Could you add a patch after 02/10 that adds all functions (configure(), >>>>> prepare() and process()) to the Algorithm class, and call them in the >>>>> IPAIPU3 class as appropriate ? That patch should include the >>>>> documentation of the functions, to explain how they're used. After that >>>>> the rest of the series can begin moving the algorithms to the new class. >>>> >>>> I did that in the first place, and it was a bit of a heavy patch. So I >>>> decided to split it to introduce each of configure/prepare/process calls >>>> step by step... >>> >>> The issue with this approach is that it's harder to have an overview of >>> the proposed API. Is it *that* heavy to define a class with three >>> abstract functions and their documentation ? >> >> No, but as we already have a process() call in AGC it must be rewritten >> accordingly. > > The existing process() function takes different parameters, so the > compiler shouldn't complain. Worst case it could be renamed in a small > patch at the beginning of the series to move it out of the way. I agree > that adapting the algorithm is best done in a patch separate from the > introduction of the new Algorithm structure. > The compiler does not agree with you :-). ../src/ipa/ipu3/ipu3_agc.h:33:7: error: 'libcamera::ipa::ipu3::IPU3Agc::process' hides overloaded virtual function [-Werror,-Woverloaded-virtual] void process(const ipu3_uapi_stats_3a *stats, uint32_t &exposure, double &gain); ^ ../src/ipa/ipu3/algorithms/algorithm.h:26:15: note: hidden overloaded virtual function 'libcamera::ipa::ipu3::Algorithm::process' declared here: different number of parameters (2 vs 3) virtual void process(IPAContext &context, ipu3_uapi_stats_3a &stats); >>>>>> }; >>>>>> >>>>>> } /* namespace ipa::ipu3 */ >>>>>> diff --git a/src/ipa/ipu3/algorithms/grid.cpp b/src/ipa/ipu3/algorithms/grid.cpp >>>>>> index 3578f41b..e42a760d 100644 >>>>>> --- a/src/ipa/ipu3/algorithms/grid.cpp >>>>>> +++ b/src/ipa/ipu3/algorithms/grid.cpp >>>>>> @@ -78,6 +78,12 @@ int Grid::configure(IPAContext &context, const IPAConfigInfo &configInfo) >>>>>> return 0; >>>>>> } >>>>>> >>>>>> +void Grid::prepare(IPAContext &context, ipu3_uapi_params ¶ms) >>>>>> +{ >>>>>> + /* Update the IPU3 parameters with new calculated grid */ >>>>>> + params.acc_param.awb.config.grid = context.configuration.grid.bdsGrid; >>>>> >>>>> And this is where the spaghetti dish begins :-( It should be the >>>>> responsibility of the AWB algorithm to set params.acc_param.awb. >>>> >>>> Yes, I moved it at least 5 times while writing the patches as I could >>>> not decide where it should be :-p. >>> >>> As commented in patch 03/10, I don't think "grid" should be an >>> algorithm. >>> >>>>>> +} >>>>>> + >>>>>> } /* namespace ipa::ipu3::algorithms */ >>>>>> >>>>>> } /* namespace libcamera */ >>>>>> diff --git a/src/ipa/ipu3/algorithms/grid.h b/src/ipa/ipu3/algorithms/grid.h >>>>>> index b4a51b42..89399bd2 100644 >>>>>> --- a/src/ipa/ipu3/algorithms/grid.h >>>>>> +++ b/src/ipa/ipu3/algorithms/grid.h >>>>>> @@ -19,6 +19,7 @@ public: >>>>>> ~Grid() = default; >>>>>> >>>>>> int configure(IPAContext &context, const IPAConfigInfo &configInfo) override; >>>>>> + void prepare(IPAContext &context, ipu3_uapi_params ¶ms) override; >>>>>> }; >>>>>> >>>>>> } /* namespace ipa::ipu3::algorithms */ >>>>>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp >>>>>> index ef7fec86..394a7a45 100644 >>>>>> --- a/src/ipa/ipu3/ipu3.cpp >>>>>> +++ b/src/ipa/ipu3/ipu3.cpp >>>>>> @@ -305,6 +305,9 @@ void IPAIPU3::processControls([[maybe_unused]] unsigned int frame, >>>>>> >>>>>> void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params) >>>>>> { >>>>>> + for (auto const &algo : algorithms_) >>>>>> + algo->prepare(context_, params_); >>>>>> + >>>>>> if (agcAlgo_->updateControls()) >>>>>> awbAlgo_->updateWbParameters(params_, agcAlgo_->gamma()); >>>>>> >>>>>> diff --git a/src/ipa/ipu3/ipu3_awb.cpp b/src/ipa/ipu3/ipu3_awb.cpp >>>>>> index 4ee5ee6f..911760b3 100644 >>>>>> --- a/src/ipa/ipu3/ipu3_awb.cpp >>>>>> +++ b/src/ipa/ipu3/ipu3_awb.cpp >>>>>> @@ -159,7 +159,6 @@ void IPU3Awb::initialise(ipu3_uapi_params ¶ms, const Size &bdsOutputSize, st >>>>>> params.acc_param.awb.config.rgbs_thr_r = 8191; >>>>>> params.acc_param.awb.config.rgbs_thr_gb = 8191; >>>>>> params.acc_param.awb.config.rgbs_thr_b = 8191 | IPU3_UAPI_AWB_RGBS_THR_B_EN | IPU3_UAPI_AWB_RGBS_THR_B_INCL_SAT; >>>>>> - params.acc_param.awb.config.grid = bdsGrid; >>>>>> awbGrid_ = bdsGrid; >>>>>> >>>>>> params.use.acc_bnr = 1; >
Hi Jean-Michel, On Wed, Aug 18, 2021 at 08:47:28AM +0200, Jean-Michel Hautbois wrote: > On 16/08/2021 11:49, Laurent Pinchart wrote: > > On Mon, Aug 16, 2021 at 11:43:54AM +0200, Jean-Michel Hautbois wrote: > >> On 16/08/2021 11:41, Laurent Pinchart wrote: > >>> On Mon, Aug 16, 2021 at 10:01:14AM +0200, Jean-Michel Hautbois wrote: > >>>> On 15/08/2021 20:02, Laurent Pinchart wrote: > >>>>> On Thu, Aug 12, 2021 at 06:52:37PM +0200, Jean-Michel Hautbois wrote: > >>>>>> When a new ipu3_uapi_params buffer is ready, IPAIPU3 receives a > >>>>>> EventFillParams. When this is called, the algorithms should fill every > >>>>>> field in the parameter structure they need to update. > >>>>>> > >>>>>> Demonstrate it by adding a loop on the prepare() call in IPAIPU3 and let > >>>>>> the Grid algorithm update the grid field. > >>>>>> This leads to removing this same update from the Awb algorithm. > >>>>>> > >>>>>> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> > >>>>>> --- > >>>>>> src/ipa/ipu3/algorithms/algorithm.h | 2 ++ > >>>>>> src/ipa/ipu3/algorithms/grid.cpp | 6 ++++++ > >>>>>> src/ipa/ipu3/algorithms/grid.h | 1 + > >>>>>> src/ipa/ipu3/ipu3.cpp | 3 +++ > >>>>>> src/ipa/ipu3/ipu3_awb.cpp | 1 - > >>>>>> 5 files changed, 12 insertions(+), 1 deletion(-) > >>>>>> > >>>>>> diff --git a/src/ipa/ipu3/algorithms/algorithm.h b/src/ipa/ipu3/algorithms/algorithm.h > >>>>>> index c1b37276..b571f55a 100644 > >>>>>> --- a/src/ipa/ipu3/algorithms/algorithm.h > >>>>>> +++ b/src/ipa/ipu3/algorithms/algorithm.h > >>>>>> @@ -26,6 +26,8 @@ public: > >>>>>> { > >>>>>> return 0; > >>>>>> } > >>>>>> + > >>>>>> + virtual void prepare([[maybe_unused]] IPAContext &context, [[maybe_unused]] ipu3_uapi_params ¶ms) {} > >>>>> > >>>>> Line wrap. > >>>>> > >>>>> Could you add a patch after 02/10 that adds all functions (configure(), > >>>>> prepare() and process()) to the Algorithm class, and call them in the > >>>>> IPAIPU3 class as appropriate ? That patch should include the > >>>>> documentation of the functions, to explain how they're used. After that > >>>>> the rest of the series can begin moving the algorithms to the new class. > >>>> > >>>> I did that in the first place, and it was a bit of a heavy patch. So I > >>>> decided to split it to introduce each of configure/prepare/process calls > >>>> step by step... > >>> > >>> The issue with this approach is that it's harder to have an overview of > >>> the proposed API. Is it *that* heavy to define a class with three > >>> abstract functions and their documentation ? > >> > >> No, but as we already have a process() call in AGC it must be rewritten > >> accordingly. > > > > The existing process() function takes different parameters, so the > > compiler shouldn't complain. Worst case it could be renamed in a small > > patch at the beginning of the series to move it out of the way. I agree > > that adapting the algorithm is best done in a patch separate from the > > introduction of the new Algorithm structure. > > The compiler does not agree with you :-). > > ../src/ipa/ipu3/ipu3_agc.h:33:7: error: > 'libcamera::ipa::ipu3::IPU3Agc::process' hides overloaded virtual > function [-Werror,-Woverloaded-virtual] It's a warning that we treat as an error, as all warnings. You could disable the warning temporarily and then restore it as part of the series to keep patches from getting large. > void process(const ipu3_uapi_stats_3a *stats, uint32_t > &exposure, double &gain); > ^ > ../src/ipa/ipu3/algorithms/algorithm.h:26:15: note: hidden overloaded > virtual function 'libcamera::ipa::ipu3::Algorithm::process' declared > here: different number of parameters (2 vs 3) > virtual void process(IPAContext &context, ipu3_uapi_stats_3a > &stats); > > >>>>>> }; > >>>>>> > >>>>>> } /* namespace ipa::ipu3 */ > >>>>>> diff --git a/src/ipa/ipu3/algorithms/grid.cpp b/src/ipa/ipu3/algorithms/grid.cpp > >>>>>> index 3578f41b..e42a760d 100644 > >>>>>> --- a/src/ipa/ipu3/algorithms/grid.cpp > >>>>>> +++ b/src/ipa/ipu3/algorithms/grid.cpp > >>>>>> @@ -78,6 +78,12 @@ int Grid::configure(IPAContext &context, const IPAConfigInfo &configInfo) > >>>>>> return 0; > >>>>>> } > >>>>>> > >>>>>> +void Grid::prepare(IPAContext &context, ipu3_uapi_params ¶ms) > >>>>>> +{ > >>>>>> + /* Update the IPU3 parameters with new calculated grid */ > >>>>>> + params.acc_param.awb.config.grid = context.configuration.grid.bdsGrid; > >>>>> > >>>>> And this is where the spaghetti dish begins :-( It should be the > >>>>> responsibility of the AWB algorithm to set params.acc_param.awb. > >>>> > >>>> Yes, I moved it at least 5 times while writing the patches as I could > >>>> not decide where it should be :-p. > >>> > >>> As commented in patch 03/10, I don't think "grid" should be an > >>> algorithm. > >>> > >>>>>> +} > >>>>>> + > >>>>>> } /* namespace ipa::ipu3::algorithms */ > >>>>>> > >>>>>> } /* namespace libcamera */ > >>>>>> diff --git a/src/ipa/ipu3/algorithms/grid.h b/src/ipa/ipu3/algorithms/grid.h > >>>>>> index b4a51b42..89399bd2 100644 > >>>>>> --- a/src/ipa/ipu3/algorithms/grid.h > >>>>>> +++ b/src/ipa/ipu3/algorithms/grid.h > >>>>>> @@ -19,6 +19,7 @@ public: > >>>>>> ~Grid() = default; > >>>>>> > >>>>>> int configure(IPAContext &context, const IPAConfigInfo &configInfo) override; > >>>>>> + void prepare(IPAContext &context, ipu3_uapi_params ¶ms) override; > >>>>>> }; > >>>>>> > >>>>>> } /* namespace ipa::ipu3::algorithms */ > >>>>>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp > >>>>>> index ef7fec86..394a7a45 100644 > >>>>>> --- a/src/ipa/ipu3/ipu3.cpp > >>>>>> +++ b/src/ipa/ipu3/ipu3.cpp > >>>>>> @@ -305,6 +305,9 @@ void IPAIPU3::processControls([[maybe_unused]] unsigned int frame, > >>>>>> > >>>>>> void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params) > >>>>>> { > >>>>>> + for (auto const &algo : algorithms_) > >>>>>> + algo->prepare(context_, params_); > >>>>>> + > >>>>>> if (agcAlgo_->updateControls()) > >>>>>> awbAlgo_->updateWbParameters(params_, agcAlgo_->gamma()); > >>>>>> > >>>>>> diff --git a/src/ipa/ipu3/ipu3_awb.cpp b/src/ipa/ipu3/ipu3_awb.cpp > >>>>>> index 4ee5ee6f..911760b3 100644 > >>>>>> --- a/src/ipa/ipu3/ipu3_awb.cpp > >>>>>> +++ b/src/ipa/ipu3/ipu3_awb.cpp > >>>>>> @@ -159,7 +159,6 @@ void IPU3Awb::initialise(ipu3_uapi_params ¶ms, const Size &bdsOutputSize, st > >>>>>> params.acc_param.awb.config.rgbs_thr_r = 8191; > >>>>>> params.acc_param.awb.config.rgbs_thr_gb = 8191; > >>>>>> params.acc_param.awb.config.rgbs_thr_b = 8191 | IPU3_UAPI_AWB_RGBS_THR_B_EN | IPU3_UAPI_AWB_RGBS_THR_B_INCL_SAT; > >>>>>> - params.acc_param.awb.config.grid = bdsGrid; > >>>>>> awbGrid_ = bdsGrid; > >>>>>> > >>>>>> params.use.acc_bnr = 1;
diff --git a/src/ipa/ipu3/algorithms/algorithm.h b/src/ipa/ipu3/algorithms/algorithm.h index c1b37276..b571f55a 100644 --- a/src/ipa/ipu3/algorithms/algorithm.h +++ b/src/ipa/ipu3/algorithms/algorithm.h @@ -26,6 +26,8 @@ public: { return 0; } + + virtual void prepare([[maybe_unused]] IPAContext &context, [[maybe_unused]] ipu3_uapi_params ¶ms) {} }; } /* namespace ipa::ipu3 */ diff --git a/src/ipa/ipu3/algorithms/grid.cpp b/src/ipa/ipu3/algorithms/grid.cpp index 3578f41b..e42a760d 100644 --- a/src/ipa/ipu3/algorithms/grid.cpp +++ b/src/ipa/ipu3/algorithms/grid.cpp @@ -78,6 +78,12 @@ int Grid::configure(IPAContext &context, const IPAConfigInfo &configInfo) return 0; } +void Grid::prepare(IPAContext &context, ipu3_uapi_params ¶ms) +{ + /* Update the IPU3 parameters with new calculated grid */ + params.acc_param.awb.config.grid = context.configuration.grid.bdsGrid; +} + } /* namespace ipa::ipu3::algorithms */ } /* namespace libcamera */ diff --git a/src/ipa/ipu3/algorithms/grid.h b/src/ipa/ipu3/algorithms/grid.h index b4a51b42..89399bd2 100644 --- a/src/ipa/ipu3/algorithms/grid.h +++ b/src/ipa/ipu3/algorithms/grid.h @@ -19,6 +19,7 @@ public: ~Grid() = default; int configure(IPAContext &context, const IPAConfigInfo &configInfo) override; + void prepare(IPAContext &context, ipu3_uapi_params ¶ms) override; }; } /* namespace ipa::ipu3::algorithms */ diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp index ef7fec86..394a7a45 100644 --- a/src/ipa/ipu3/ipu3.cpp +++ b/src/ipa/ipu3/ipu3.cpp @@ -305,6 +305,9 @@ void IPAIPU3::processControls([[maybe_unused]] unsigned int frame, void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params) { + for (auto const &algo : algorithms_) + algo->prepare(context_, params_); + if (agcAlgo_->updateControls()) awbAlgo_->updateWbParameters(params_, agcAlgo_->gamma()); diff --git a/src/ipa/ipu3/ipu3_awb.cpp b/src/ipa/ipu3/ipu3_awb.cpp index 4ee5ee6f..911760b3 100644 --- a/src/ipa/ipu3/ipu3_awb.cpp +++ b/src/ipa/ipu3/ipu3_awb.cpp @@ -159,7 +159,6 @@ void IPU3Awb::initialise(ipu3_uapi_params ¶ms, const Size &bdsOutputSize, st params.acc_param.awb.config.rgbs_thr_r = 8191; params.acc_param.awb.config.rgbs_thr_gb = 8191; params.acc_param.awb.config.rgbs_thr_b = 8191 | IPU3_UAPI_AWB_RGBS_THR_B_EN | IPU3_UAPI_AWB_RGBS_THR_B_INCL_SAT; - params.acc_param.awb.config.grid = bdsGrid; awbGrid_ = bdsGrid; params.use.acc_bnr = 1;
When a new ipu3_uapi_params buffer is ready, IPAIPU3 receives a EventFillParams. When this is called, the algorithms should fill every field in the parameter structure they need to update. Demonstrate it by adding a loop on the prepare() call in IPAIPU3 and let the Grid algorithm update the grid field. This leads to removing this same update from the Awb algorithm. Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> --- src/ipa/ipu3/algorithms/algorithm.h | 2 ++ src/ipa/ipu3/algorithms/grid.cpp | 6 ++++++ src/ipa/ipu3/algorithms/grid.h | 1 + src/ipa/ipu3/ipu3.cpp | 3 +++ src/ipa/ipu3/ipu3_awb.cpp | 1 - 5 files changed, 12 insertions(+), 1 deletion(-)