[v2,1/4] ipa: rkisp1: algorithms: agc: Check for correct stats type

21682 diff mbox series

Sometimes the ISP produces statistics only with a subset of statistic
types being valid. It doesn't happen normally, but was observed in the
wild. Check for the RKISP1_CIF_ISP_STAT_AUTOEXP bit to prevent using
invalid or outdated data. As it doesn't happen regularly add an error
message to get notified when it happens.

Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>

---
Changes in v2:
- Added error message
- Made condition more readable
- Collected tags
---
 src/ipa/rkisp1/algorithms/agc.cpp | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Quoting Stefan Klug (2024-10-18 15:58:35)
> Sometimes the ISP produces statistics only with a subset of statistic
> types being valid. It doesn't happen normally, but was observed in the
> wild. Check for the RKISP1_CIF_ISP_STAT_AUTOEXP bit to prevent using
> invalid or outdated data. As it doesn't happen regularly add an error
> message to get notified when it happens.
> 
> Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
> 
> ---
> Changes in v2:
> - Added error message
> - Made condition more readable
> - Collected tags
> ---
>  src/ipa/rkisp1/algorithms/agc.cpp | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
> index 17d074d9c03e..f03136836fd7 100644
> --- a/src/ipa/rkisp1/algorithms/agc.cpp
> +++ b/src/ipa/rkisp1/algorithms/agc.cpp
> @@ -398,8 +398,9 @@ 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)) {
>                 fillMetadata(context, frameContext, metadata);
> +               LOG(RkISP1Agc, Error) << "AUTOEXP data is missing in statistics";

Won't this now report an error for every frame on Raw streams?

>                 return;
>         }
>  
> @@ -412,7 +413,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);
>  
>         /* The lower 4 bits are fractional and meant to be discarded. */
>         Histogram hist({ params->hist.hist_bins, context.hw->numHistogramBins },
> -- 
> 2.43.0
>

On Fri, Oct 18, 2024 at 04:05:43PM +0100, Kieran Bingham wrote:
> Quoting Stefan Klug (2024-10-18 15:58:35)
> > Sometimes the ISP produces statistics only with a subset of statistic
> > types being valid. It doesn't happen normally, but was observed in the
> > wild. Check for the RKISP1_CIF_ISP_STAT_AUTOEXP bit to prevent using
> > invalid or outdated data. As it doesn't happen regularly add an error
> > message to get notified when it happens.
> > 
> > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
> > 
> > ---
> > Changes in v2:
> > - Added error message
> > - Made condition more readable
> > - Collected tags
> > ---
> >  src/ipa/rkisp1/algorithms/agc.cpp | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
> > index 17d074d9c03e..f03136836fd7 100644
> > --- a/src/ipa/rkisp1/algorithms/agc.cpp
> > +++ b/src/ipa/rkisp1/algorithms/agc.cpp
> > @@ -398,8 +398,9 @@ 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)) {
> >                 fillMetadata(context, frameContext, metadata);
> > +               LOG(RkISP1Agc, Error) << "AUTOEXP data is missing in statistics";
> 
> Won't this now report an error for every frame on Raw streams?

Ouch. v3 is on it's way.

> 
> >                 return;
> >         }
> >  
> > @@ -412,7 +413,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);
> >  
> >         /* The lower 4 bits are fractional and meant to be discarded. */
> >         Histogram hist({ params->hist.hist_bins, context.hw->numHistogramBins },
> > -- 
> > 2.43.0
> >