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

[Laurent Pinchart]

Hi Stefan,

On Thu, Oct 17, 2024 at 09:36:21AM +0200, Stefan Klug wrote:
> 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.

I'm certainly not opposed to this patch, I don't want to cause rare
crashes for users :-) I'm only wondering how we could try to debug this
in case it's a driver issue, to get it fixed. Maybe an ERROR log message
would be a good option, to get it noticed ? If the issue is very rare,
it shouldn't cause much disturbance.

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