[RFC PATCH] ipa: rkisp1: Initialize lsc tables on first frame

[Kieran Bingham]

Quoting Stefan Klug (2024-12-16 15:33:02)
> When sending the parameters for lens shading correction to the ISP on
> frame x (in the test mostly frame 4) stalls of the ISP were observed.
> From userland the symptom is that no buffers get returned anymore.
> Kernel wise the ISP constantly produces interrupts with
> RKISP1_CIF_ISP_V_START being set but no interrupts with
> RKISP1_CIF_ISP_FRAME. It turned out that this behavior wasn't observed
> when the lens shading correction parameters were written before
> receiving the first frame. Ensure that behavior in the lsc algorithm by
> unconditionally filling the params buffer on frame 0.
> 
> Unfortunately there were also cases were no stalls were observed even
> though the lsc tables were not written on frame 0. So it is not clear
> if this fix fixes the actual root cause.
> 
> Another reason for that change is that it is sensible to write the lsc
> data on frame 0, to minimize the flicker in the image when the first
> statistics come in (where they would have been written otherwise).

I think it's reasonable to write an LSC calibration table from the
start. It might be difficult to ascertain 'which one' if we have an
option to interpolate this based on color temperature, but I would
suspect any of the pre-calibrated tables with the wrong temperature are
better than an identity mapping for the starting configuration.

> Signed-off-by: Stefan Klug <stefan.klug at ideasonboard.com>
> ---
> 
>  src/ipa/rkisp1/algorithms/lsc.cpp | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/src/ipa/rkisp1/algorithms/lsc.cpp b/src/ipa/rkisp1/algorithms/lsc.cpp
> index e47aa2f0727e..15a43799ae27 100644
> --- a/src/ipa/rkisp1/algorithms/lsc.cpp
> +++ b/src/ipa/rkisp1/algorithms/lsc.cpp
> @@ -410,12 +410,20 @@ void LensShadingCorrection::prepare(IPAContext &context,
>                                     RkISP1Params *params)
>  {
>         uint32_t ct = context.activeState.awb.temperatureK;

On frame zero - what is this initialised to ? Is it something sane? Is
it '0' ? And mostly is it something that won't badly affect the code below?

If the LSC interpolations get clamped to sane ranges, then I would be
happy with this - but I can't tell from the context of this patch alone,
however I do suspect that's the case so:


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

> -       if (std::abs(static_cast<int>(ct) - static_cast<int>(lastAppliedCt_)) <
> +       /*
> +        * The ISP in the imx8mp using linux 6.12 has a high probability for a
> +        * stall when the lsc module isn't configured in the first frame.
> +        * Therefore ensure that a lsc table is written unconditionally at the
> +        * first frame.
> +       */
> +       if (frame > 0 &&
> +           std::abs(static_cast<int>(ct) - static_cast<int>(lastAppliedCt_)) <
>             kColourTemperatureChangeThreshhold)
>                 return;
> +
>         unsigned int quantizedCt;
>         const Components &set = sets_.getInterpolated(ct, &quantizedCt);
> -       if (lastAppliedQuantizedCt_ == quantizedCt)
> +       if (frame > 0 && lastAppliedQuantizedCt_ == quantizedCt)
>                 return;
>  
>         auto config = params->block<BlockType::Lsc>();
> -- 
> 2.43.0
>