[v1,11/17] ipa: rkisp1: algorithms: dpf: detect DPF dev overrides

24881 diff mbox series

Introduce checkDevModeOverridesChanged() to detect when developer-mode
controls have been modified. This function compares override values against
the current hardware configuration for:
- Spatial coefficients (green and RB)
- RB filter size
- NLL coefficients and scale mode

Updates queueRequest() to trigger hardware updates whenever dev-mode
overrides differ from active settings. This ensures spatial kernels, NLL
tables, and filter size changes are applied immediately in manual mode.

Signed-off-by: Rui Wang <rui.wang@ideasonboard.com>
---
 src/ipa/rkisp1/algorithms/dpf.cpp | 51 +++++++++++++++++++++++++++++++
 src/ipa/rkisp1/algorithms/dpf.h   |  5 +++
 2 files changed, 56 insertions(+)

Quoting Rui Wang (2025-10-28 21:17:44)
> Introduce checkDevModeOverridesChanged() to detect when developer-mode
> controls have been modified. This function compares override values against
> the current hardware configuration for:
> - Spatial coefficients (green and RB)
> - RB filter size
> - NLL coefficients and scale mode
> 
> Updates queueRequest() to trigger hardware updates whenever dev-mode
> overrides differ from active settings. This ensures spatial kernels, NLL
> tables, and filter size changes are applied immediately in manual mode.
> 
> Signed-off-by: Rui Wang <rui.wang@ideasonboard.com>
> ---
>  src/ipa/rkisp1/algorithms/dpf.cpp | 51 +++++++++++++++++++++++++++++++
>  src/ipa/rkisp1/algorithms/dpf.h   |  5 +++
>  2 files changed, 56 insertions(+)
> 
> diff --git a/src/ipa/rkisp1/algorithms/dpf.cpp b/src/ipa/rkisp1/algorithms/dpf.cpp
> index 145f10b7..cefa5da5 100644
> --- a/src/ipa/rkisp1/algorithms/dpf.cpp
> +++ b/src/ipa/rkisp1/algorithms/dpf.cpp
> @@ -290,6 +290,54 @@ void Dpf::collectManualOverrides(const ControlList &controls)
>         }
>  }
>  
> +bool Dpf::checkDevModeOverridesChanged()
> +{
> +       if (!isDevMode())
> +               return false;
> +
> +       bool changed = false;
> +       if (overrides_.spatialGreen) {
> +               bool coeffsChanged = false;
> +               for (unsigned i = 0; i < RKISP1_CIF_ISP_DPF_MAX_SPATIAL_COEFFS; ++i) {
> +                       if (overrides_.spatialGreen->coeffs[i] != config_.g_flt.spatial_coeff[i]) {
> +                               coeffsChanged = true;
> +                               break;
> +                       }
> +               }
> +               if (coeffsChanged) {
> +                       changed = true;

Having two bools here seems redudnant.

> +               }

Single statements do not need { }.

> +       }


Please add blank lines in between conditional blocks to ease/improve
readability. 

> +       if (overrides_.spatialRb) {
> +               bool coeffsChanged = false;
> +               for (unsigned i = 0; i < RKISP1_CIF_ISP_DPF_MAX_SPATIAL_COEFFS; ++i) {
> +                       if (overrides_.spatialRb->coeffs[i] != config_.rb_flt.spatial_coeff[i]) {
> +                               coeffsChanged = true;
> +                               break;
> +                       }
> +               }
> +               if (coeffsChanged) {
> +                       changed = true;
> +               }
> +       }
> +       if (overrides_.rbSize && *overrides_.rbSize != (config_.rb_flt.fltsize == RKISP1_CIF_ISP_DPF_RB_FILTERSIZE_13x9 ? 1 : 0)) {
> +               changed = true;
> +       }

Where necessary we can support long lines, but try to aim for 80 and
really try to keep under 120 unless theres something more like a table
forcing us to go longer.

	if (overrides_.rbSize && 
	    *overrides_.rbSize != (config_.rb_flt.fltsize == RKISP1_CIF_ISP_DPF_RB_FILTERSIZE_13x9 ? 1 : 0)
	    	changed = true;

But even that's awkward, there must be somethign better we can do here.

Why isn't this just something like

	if (overrides_.rbSize && *overrides_.rbSize != config_.rb_flt.fltsize)
		changed = true;




> +       if (overrides_.nll) {
> +               bool coeffsChanged = false;
> +               for (unsigned i = 0; i < RKISP1_CIF_ISP_DPF_MAX_NLF_COEFFS; ++i) {
> +                       if (overrides_.nll->coeffs[i] != config_.nll.coeff[i]) {
> +                               coeffsChanged = true;
> +                               break;
> +                       }
> +               }
> +               if (coeffsChanged || overrides_.nll->scaleMode != (config_.nll.scale_mode == RKISP1_CIF_ISP_NLL_SCALE_LOGARITHMIC ? 1 : 0)) {
> +                       changed = true;
> +               }
> +       }
> +       return changed;
> +}
> +
>  /**
>   * \copydoc libcamera::ipa::Algorithm::queueRequest
>   */
> @@ -310,6 +358,9 @@ void Dpf::queueRequest(IPAContext &context,
>                      overrides_.strength->b != strengthConfig_.b)) {
>                         frameContext.dpf.update = true;
>                 }
> +               if (checkDevModeOverridesChanged()) {
> +                       frameContext.dpf.update = true;
> +               }
>         }
>  }
>  
> diff --git a/src/ipa/rkisp1/algorithms/dpf.h b/src/ipa/rkisp1/algorithms/dpf.h
> index 1a33a8c4..b971619b 100644
> --- a/src/ipa/rkisp1/algorithms/dpf.h
> +++ b/src/ipa/rkisp1/algorithms/dpf.h
> @@ -80,8 +80,13 @@ private:
>         bool enableDpf_ = true; /* YAML master enable */
>  
>         void handleEnableControl(const ControlList &controls, IPAFrameContext &frameContext, IPAContext &context) override;
> +
>         void collectManualOverrides(const ControlList &controls) override;
> +
> +       bool checkDevModeOverridesChanged();
> +
>         bool parseConfig(const YamlObject &tuningData) override;
> +
>         bool parseSingleConfig(const YamlObject &config,
>                                rkisp1_cif_isp_dpf_config &cfg,
>                                rkisp1_cif_isp_dpf_strength_config &strength);
> -- 
> 2.43.0
>