[v1,1/4] ipa: rkisp1: algorithms: agc: Check for correct stats type
diff mbox series

Message ID 20241015203820.574326-2-stefan.klug@ideasonboard.com
State Accepted
Headers show
Series
  • A few small fixes
Related show

Commit Message

Stefan Klug Oct. 15, 2024, 8:38 p.m. UTC 
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@ideasonboard.com>
---
 src/ipa/rkisp1/algorithms/agc.cpp | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

Comments

Kieran Bingham Oct. 16, 2024, 1:33 p.m. UTC | #1 
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@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)) {
>                 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 ?
But I think it seems reasonable


Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

>  
>         /* The lower 4 bits are fractional and meant to be discarded. */
>         Histogram hist({ params->hist.hist_bins, context.hw->numHistogramBins },
> -- 
> 2.43.0
>
Laurent Pinchart Oct. 16, 2024, 2:36 p.m. UTC | #2 
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@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 ?

> But I think it seems reasonable
> 
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> >  
> >         /* The lower 4 bits are fractional and meant to be discarded. */
> >         Histogram hist({ params->hist.hist_bins, context.hw->numHistogramBins },
Paul Elder Oct. 17, 2024, 1:26 a.m. UTC | #3 
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@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)) {

+1


Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>

> 
> > >                 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 ?
> 
> > But I think it seems reasonable
> > 
> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@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
Stefan Klug Oct. 17, 2024, 7:36 a.m. UTC | #4 
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@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@ideasonboard.com>
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@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
Laurent Pinchart Oct. 17, 2024, 8:24 a.m. UTC | #5 
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@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@ideasonboard.com>
> > 
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > 
> > > >  
> > > >         /* The lower 4 bits are fractional and meant to be discarded. */
> > > >         Histogram hist({ params->hist.hist_bins, context.hw->numHistogramBins },

Patch
diff mbox series 

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