[libcamera-devel] [PATCH v4 22/32] ipa: rkisp1: dpf: Store per-frame information in frame context
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Thu Sep 22 22:16:00 CEST 2022
Hi Jacopo,
On Wed, Sep 21, 2022 at 10:40:41PM +0200, Jacopo Mondi wrote:
> On Thu, Sep 08, 2022 at 04:41:50AM +0300, Laurent Pinchart via libcamera-devel wrote:
> > Rework the algorithm's usage of the active state, to store the value of
> > controls for the last queued request in the queueRequest() function, and
> > store a copy of the values in the corresponding frame context. The
> > latter is used in the prepare() function to populate the ISP parameters
> > with values corresponding to the right frame.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > ---
> > src/ipa/rkisp1/algorithms/dpf.cpp | 22 +++++++++++-----------
> > src/ipa/rkisp1/ipa_context.cpp | 15 ++++++++++++---
> > src/ipa/rkisp1/ipa_context.h | 6 +++++-
> > 3 files changed, 28 insertions(+), 15 deletions(-)
> >
> > diff --git a/src/ipa/rkisp1/algorithms/dpf.cpp b/src/ipa/rkisp1/algorithms/dpf.cpp
> > index 94c35c36570c..5d44480c5059 100644
> > --- a/src/ipa/rkisp1/algorithms/dpf.cpp
> > +++ b/src/ipa/rkisp1/algorithms/dpf.cpp
> > @@ -176,10 +176,11 @@ int Dpf::init([[maybe_unused]] IPAContext &context,
> > */
> > void Dpf::queueRequest(IPAContext &context,
> > [[maybe_unused]] const uint32_t frame,
> > - [[maybe_unused]] RkISP1FrameContext &frameContext,
> > + RkISP1FrameContext &frameContext,
> > const ControlList &controls)
> > {
> > auto &dpf = context.activeState.dpf;
> > + bool update = false;
> >
> > const auto &denoise = controls.get(controls::draft::NoiseReductionMode);
> > if (denoise) {
> > @@ -188,34 +189,35 @@ void Dpf::queueRequest(IPAContext &context,
> > switch (*denoise) {
> > case controls::draft::NoiseReductionModeOff:
> > dpf.denoise = false;
> > - dpf.updateParams = true;
> > + update = true;
>
> Do we update unconditionally or should we check if the state has
> changed since last time and the application is not re-sending the same
> control. As this is not compliant with the API assumptions that
> controls should be sent only when they change, I think it's fair to
> assume we need to update ?
No, you're right, I think it's useful to check if the value has changed
an only update when it does. It's just a few CPU cycles here, and will
save way more cycles in the kernel.
> If that's the case
> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
>
> > break;
> > case controls::draft::NoiseReductionModeMinimal:
> > case controls::draft::NoiseReductionModeHighQuality:
> > case controls::draft::NoiseReductionModeFast:
> > dpf.denoise = true;
> > - dpf.updateParams = true;
> > + update = true;
> > break;
> > default:
> > LOG(RkISP1Dpf, Error)
> > << "Unsupported denoise value "
> > << *denoise;
> > + break;
> > }
> > }
> > +
> > + frameContext.dpf.denoise = dpf.denoise;
> > + frameContext.dpf.update = update;
> > }
> >
> > /**
> > * \copydoc libcamera::ipa::Algorithm::prepare
> > */
> > void Dpf::prepare(IPAContext &context, const uint32_t frame,
> > - [[maybe_unused]] RkISP1FrameContext &frameContext,
> > - rkisp1_params_cfg *params)
> > + RkISP1FrameContext &frameContext, rkisp1_params_cfg *params)
> > {
> > if (!initialized_)
> > return;
> >
> > - auto &dpf = context.activeState.dpf;
> > -
> > if (frame == 0) {
> > params->others.dpf_config = config_;
> > params->others.dpf_strength_config = strengthConfig_;
> > @@ -245,12 +247,10 @@ void Dpf::prepare(IPAContext &context, const uint32_t frame,
> > RKISP1_CIF_ISP_MODULE_DPF_STRENGTH;
> > }
> >
> > - if (dpf.updateParams) {
> > + if (frameContext.dpf.update) {
> > params->module_en_update |= RKISP1_CIF_ISP_MODULE_DPF;
> > - if (dpf.denoise)
> > + if (frameContext.dpf.denoise)
> > params->module_ens |= RKISP1_CIF_ISP_MODULE_DPF;
> > -
> > - dpf.updateParams = false;
> > }
> > }
> >
> > diff --git a/src/ipa/rkisp1/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp
> > index 936b77709417..b0210a978559 100644
> > --- a/src/ipa/rkisp1/ipa_context.cpp
> > +++ b/src/ipa/rkisp1/ipa_context.cpp
> > @@ -168,9 +168,6 @@ namespace libcamera::ipa::rkisp1 {
> > *
> > * \var IPAActiveState::dpf.denoise
> > * \brief Indicates if denoise is activated
> > - *
> > - * \var IPAActiveState::dpf.updateParams
> > - * \brief Indicates if ISP parameters need to be updated
> > */
> >
> > /**
> > @@ -251,6 +248,18 @@ namespace libcamera::ipa::rkisp1 {
> > * compared to the previous frame
> > */
> >
> > +/**
> > + * \var RkISP1FrameContext::dpf
> > + * \brief Denoise Pre-Filter parameters for this frame
> > + *
> > + * \var RkISP1FrameContext::dpf.denoise
> > + * \brief Indicates if denoise is activated
> > + *
> > + * \var RkISP1FrameContext::dpf.update
> > + * \brief Indicates if the denoise pre-filter parameters have been updated
> > + * compared to the previous frame
> > + */
> > +
> > /**
> > * \var RkISP1FrameContext::sensor
> > * \brief Sensor configuration that used been used for this frame
> > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
> > index 78edb607d039..c22e1f099c23 100644
> > --- a/src/ipa/rkisp1/ipa_context.h
> > +++ b/src/ipa/rkisp1/ipa_context.h
> > @@ -79,7 +79,6 @@ struct IPAActiveState {
> >
> > struct {
> > bool denoise;
> > - bool updateParams;
> > } dpf;
> >
> > struct {
> > @@ -113,6 +112,11 @@ struct RkISP1FrameContext : public FrameContext {
> > bool update;
> > } cproc;
> >
> > + struct {
> > + bool denoise;
> > + bool update;
> > + } dpf;
> > +
> > struct {
> > uint32_t exposure;
> > double gain;
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list