| Message ID | 20241216153344.201302-1-stefan.klug@ideasonboard.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
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@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@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 >
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@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@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>();
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; - 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>();
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). Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> --- src/ipa/rkisp1/algorithms/lsc.cpp | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)