[PATCH v1 1/4] ipa: rkisp1: algorithms: agc: Check for correct stats type

[Stefan Klug]

Hi Laurent, hi Kieran,

Thank you for the review. 

On Wed, Oct 16, 2024 at 05:36:44PM +0300, Laurent Pinchart wrote:
> On Wed, Oct 16, 2024 at 02:33:06PM +0100, Kieran Bingham wrote:
> > Quoting Stefan Klug (2024-10-15 21:38:12)
> > > Sometimes the ISP produces statistics only with a subset of statistic
> > > types being valid. It doesn't happen often, but it happens.  Check for
> > > the RKISP1_CIF_ISP_STAT_AUTOEXP bit to prevent using invalid or outdated
> > > data.
> > > 
> > > Signed-off-by: Stefan Klug <stefan.klug at ideasonboard.com>
> > > ---
> > >  src/ipa/rkisp1/algorithms/agc.cpp | 3 +--
> > >  1 file changed, 1 insertion(+), 2 deletions(-)
> > > 
> > > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
> > > index 17d074d9c03e..5683707fa91a 100644
> > > --- a/src/ipa/rkisp1/algorithms/agc.cpp
> > > +++ b/src/ipa/rkisp1/algorithms/agc.cpp
> > > @@ -398,7 +398,7 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
> > >                   IPAFrameContext &frameContext, const rkisp1_stat_buffer *stats,
> > >                   ControlList &metadata)
> > >  {
> > > -       if (!stats) {
> > > +       if (!(stats && stats->meas_type & RKISP1_CIF_ISP_STAT_AUTOEXP)) {
> 
> I'd write
> 
> 	if (!stats || !(stats->meas_type & RKISP1_CIF_ISP_STAT_AUTOEXP)) {
> 
> > >                 fillMetadata(context, frameContext, metadata);
> > >                 return;
> > >         }
> > > @@ -412,7 +412,6 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
> > >          */
> > >  
> > >         const rkisp1_cif_isp_stat *params = &stats->params;
> > > -       ASSERT(stats->meas_type & RKISP1_CIF_ISP_STAT_AUTOEXP);
> > 
> > Did you hit this ?
> 
> I'm surprised. Is there a reliable way to trigger that ? What is it
> caused by ?

I saw that while experimenting with the dewarper. I had a setup, that
repeatedly crashed inside libcamera. Investigating that it turned out to
be due to the stat type not beeing checked. Currently with my system in a
proper state I am not able to reproduce it.

I don't want to invest too much time, trying to find that point again. I
added a debug error on my side for now, to see if it pops up again.
Would you prefer to drop the patches? Imho it is technically correct to
check for the bit and I needed to add it to progress with my development
but it seems to be a rare case otherwise.

Best regards,
Stefan

> 
> > But I think it seems reasonable
> > 
> > Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> 
> > >  
> > >         /* The lower 4 bits are fractional and meant to be discarded. */
> > >         Histogram hist({ params->hist.hist_bins, context.hw->numHistogramBins },
> 
> -- 
> Regards,
> 
> Laurent Pinchart