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

[Laurent Pinchart]

On Tue, Dec 17, 2024 at 04:29:38PM +0000, Kieran Bingham wrote:
> 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.

I agree. However, I'm concerned that the kernel makes it possible to
hand the ISP so easily.

> > 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

s/imx8mp/i.MX8MP/
s/linux/Linux/

> > +        * stall when the lsc module isn't configured in the first frame.

s/lsc/LSC/

Maybe expand it to "is configured after starting the ISP if it hasn't
previously been configured for the first frame".

> > +        * Therefore ensure that a lsc table is written unconditionally at the

Ditto.

> > +        * 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>();

-- 
Regards,

Laurent Pinchart