[libcamera-devel,07/20] ipa: rpi: Add statsInline to the Controller hardware description
diff mbox series

Message ID 20231006132000.23504-8-naush@raspberrypi.com
State Superseded
Headers show
Series
  • Raspberry Pi: Preliminary PiSP support
Related show

Commit Message

Naushir Patuck Oct. 6, 2023, 1:19 p.m. UTC 
Add a new boolean field (statsInline) to Controller::HardwareConfigMap.
This field indicates where the statistics are generated in the hardware
ISP pipeline. For statsInline == true, statistics are generated before
the frame is processed (e.g. the PiSP case), and statsInline == false
indicates statistics are generated after the frame is processed (e.g.
the VC4 case).

Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
---
 src/ipa/rpi/common/ipa_base.cpp       | 19 ++++++++++++++-----
 src/ipa/rpi/controller/controller.cpp |  3 ++-
 src/ipa/rpi/controller/controller.h   |  1 +
 3 files changed, 17 insertions(+), 6 deletions(-)

Comments

Jacopo Mondi Oct. 12, 2023, 8:30 a.m. UTC | #1 
On Fri, Oct 06, 2023 at 02:19:47PM +0100, Naushir Patuck via libcamera-devel wrote:
> Add a new boolean field (statsInline) to Controller::HardwareConfigMap.
> This field indicates where the statistics are generated in the hardware
> ISP pipeline. For statsInline == true, statistics are generated before
> the frame is processed (e.g. the PiSP case), and statsInline == false
> indicates statistics are generated after the frame is processed (e.g.
> the VC4 case).
>
> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
> ---
>  src/ipa/rpi/common/ipa_base.cpp       | 19 ++++++++++++++-----
>  src/ipa/rpi/controller/controller.cpp |  3 ++-
>  src/ipa/rpi/controller/controller.h   |  1 +
>  3 files changed, 17 insertions(+), 6 deletions(-)
>
> diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp
> index 97f647a9e53e..f28eb36b7a44 100644
> --- a/src/ipa/rpi/common/ipa_base.cpp
> +++ b/src/ipa/rpi/common/ipa_base.cpp
> @@ -429,11 +429,10 @@ void IpaBase::prepareIsp(const PrepareParams &params)
>  	}
>
>  	/*
> -	 * If a statistics buffer has been passed in, call processStats
> -	 * directly now before prepare() since the statistics are available in-line
> -	 * with the Bayer frame.
> +	 * If the statistics are inline (i.e. already available with the Bayer
> +	 * frame), call processStats() now before prepare().
>  	 */
> -	if (params.buffers.stats)
> +	if (controller_.getHardwareConfig().statsInline)
>  		processStats({ params.buffers, params.ipaContext });
>
>  	/* Do we need/want to call prepare? */
> @@ -445,6 +444,10 @@ void IpaBase::prepareIsp(const PrepareParams &params)
>
>  	frameCount_++;
>
> +	/* If the statistics are inline the metadata can be returned early. */
> +	if (controller_.getHardwareConfig().statsInline)
> +		reportMetadata(ipaContext);
> +

Can't you drop this one and unconditionally call reportMetadata() in
processStats() ?

>  	/* Ready to push the input buffer into the ISP. */
>  	prepareIspComplete.emit(params.buffers, false);
>  }
> @@ -479,7 +482,13 @@ void IpaBase::processStats(const ProcessParams &params)
>  		}
>  	}
>
> -	reportMetadata(ipaContext);
> +	/*
> +	 * If the statistics are not inline the metadata must be returned now,
> +	 * before the processStatsComplete signal.
> +	 */
> +	if (!controller_.getHardwareConfig().statsInline)
> +		reportMetadata(ipaContext);
> +
>  	processStatsComplete.emit(params.buffers);
>  }
>
> diff --git a/src/ipa/rpi/controller/controller.cpp b/src/ipa/rpi/controller/controller.cpp
> index 14d245da2ce7..4b6f82b41916 100644
> --- a/src/ipa/rpi/controller/controller.cpp
> +++ b/src/ipa/rpi/controller/controller.cpp
> @@ -34,7 +34,8 @@ static const std::map<std::string, Controller::HardwareConfig> HardwareConfigMap
>  			.focusRegions = { 4, 3 },
>  			.numHistogramBins = 128,
>  			.numGammaPoints = 33,
> -			.pipelineWidth = 13
> +			.pipelineWidth = 13,
> +			.statsInline = false,
>  		}
>  	},
>  };
> diff --git a/src/ipa/rpi/controller/controller.h b/src/ipa/rpi/controller/controller.h
> index c6af5cd6c7d4..a8bc61880ab4 100644
> --- a/src/ipa/rpi/controller/controller.h
> +++ b/src/ipa/rpi/controller/controller.h
> @@ -45,6 +45,7 @@ public:
>  		unsigned int numHistogramBins;
>  		unsigned int numGammaPoints;
>  		unsigned int pipelineWidth;
> +		bool statsInline;
>  	};
>
>  	Controller();
> --
> 2.34.1
>
Naushir Patuck Oct. 12, 2023, 8:37 a.m. UTC | #2 
Hi Jacopo,

On Thu, 12 Oct 2023 at 09:30, Jacopo Mondi
<jacopo.mondi@ideasonboard.com> wrote:
>
>
> On Fri, Oct 06, 2023 at 02:19:47PM +0100, Naushir Patuck via libcamera-devel wrote:
> > Add a new boolean field (statsInline) to Controller::HardwareConfigMap.
> > This field indicates where the statistics are generated in the hardware
> > ISP pipeline. For statsInline == true, statistics are generated before
> > the frame is processed (e.g. the PiSP case), and statsInline == false
> > indicates statistics are generated after the frame is processed (e.g.
> > the VC4 case).
> >
> > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> > Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
> > ---
> >  src/ipa/rpi/common/ipa_base.cpp       | 19 ++++++++++++++-----
> >  src/ipa/rpi/controller/controller.cpp |  3 ++-
> >  src/ipa/rpi/controller/controller.h   |  1 +
> >  3 files changed, 17 insertions(+), 6 deletions(-)
> >
> > diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp
> > index 97f647a9e53e..f28eb36b7a44 100644
> > --- a/src/ipa/rpi/common/ipa_base.cpp
> > +++ b/src/ipa/rpi/common/ipa_base.cpp
> > @@ -429,11 +429,10 @@ void IpaBase::prepareIsp(const PrepareParams &params)
> >       }
> >
> >       /*
> > -      * If a statistics buffer has been passed in, call processStats
> > -      * directly now before prepare() since the statistics are available in-line
> > -      * with the Bayer frame.
> > +      * If the statistics are inline (i.e. already available with the Bayer
> > +      * frame), call processStats() now before prepare().
> >        */
> > -     if (params.buffers.stats)
> > +     if (controller_.getHardwareConfig().statsInline)
> >               processStats({ params.buffers, params.ipaContext });
> >
> >       /* Do we need/want to call prepare? */
> > @@ -445,6 +444,10 @@ void IpaBase::prepareIsp(const PrepareParams &params)
> >
> >       frameCount_++;
> >
> > +     /* If the statistics are inline the metadata can be returned early. */
> > +     if (controller_.getHardwareConfig().statsInline)
> > +             reportMetadata(ipaContext);
> > +
>
> Can't you drop this one and unconditionally call reportMetadata() in
> processStats() ?

Sadly not (well, not without a massive amount of rework).  The
contents of the metadata get collated in platformPrepareIsp(), and
this run *after* processStats() in statsInline (pisp) case.  So I have
to do this conditionally as above.

The rework would be to update all the algorithms to report metadata in
the processStats() phase, but I don't have the stomach for that big
change :)

Regards,
Naush


>
> >       /* Ready to push the input buffer into the ISP. */
> >       prepareIspComplete.emit(params.buffers, false);
> >  }
> > @@ -479,7 +482,13 @@ void IpaBase::processStats(const ProcessParams &params)
> >               }
> >       }
> >
> > -     reportMetadata(ipaContext);
> > +     /*
> > +      * If the statistics are not inline the metadata must be returned now,
> > +      * before the processStatsComplete signal.
> > +      */
> > +     if (!controller_.getHardwareConfig().statsInline)
> > +             reportMetadata(ipaContext);
> > +
> >       processStatsComplete.emit(params.buffers);
> >  }
> >
> > diff --git a/src/ipa/rpi/controller/controller.cpp b/src/ipa/rpi/controller/controller.cpp
> > index 14d245da2ce7..4b6f82b41916 100644
> > --- a/src/ipa/rpi/controller/controller.cpp
> > +++ b/src/ipa/rpi/controller/controller.cpp
> > @@ -34,7 +34,8 @@ static const std::map<std::string, Controller::HardwareConfig> HardwareConfigMap
> >                       .focusRegions = { 4, 3 },
> >                       .numHistogramBins = 128,
> >                       .numGammaPoints = 33,
> > -                     .pipelineWidth = 13
> > +                     .pipelineWidth = 13,
> > +                     .statsInline = false,
> >               }
> >       },
> >  };
> > diff --git a/src/ipa/rpi/controller/controller.h b/src/ipa/rpi/controller/controller.h
> > index c6af5cd6c7d4..a8bc61880ab4 100644
> > --- a/src/ipa/rpi/controller/controller.h
> > +++ b/src/ipa/rpi/controller/controller.h
> > @@ -45,6 +45,7 @@ public:
> >               unsigned int numHistogramBins;
> >               unsigned int numGammaPoints;
> >               unsigned int pipelineWidth;
> > +             bool statsInline;
> >       };
> >
> >       Controller();
> > --
> > 2.34.1
> >
Jacopo Mondi Oct. 12, 2023, 8:51 a.m. UTC | #3 
Hi Naush

On Thu, Oct 12, 2023 at 09:37:43AM +0100, Naushir Patuck wrote:
> Hi Jacopo,
>
> On Thu, 12 Oct 2023 at 09:30, Jacopo Mondi
> <jacopo.mondi@ideasonboard.com> wrote:
> >
> >
> > On Fri, Oct 06, 2023 at 02:19:47PM +0100, Naushir Patuck via libcamera-devel wrote:
> > > Add a new boolean field (statsInline) to Controller::HardwareConfigMap.
> > > This field indicates where the statistics are generated in the hardware
> > > ISP pipeline. For statsInline == true, statistics are generated before
> > > the frame is processed (e.g. the PiSP case), and statsInline == false
> > > indicates statistics are generated after the frame is processed (e.g.
> > > the VC4 case).
> > >
> > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> > > Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
> > > ---
> > >  src/ipa/rpi/common/ipa_base.cpp       | 19 ++++++++++++++-----
> > >  src/ipa/rpi/controller/controller.cpp |  3 ++-
> > >  src/ipa/rpi/controller/controller.h   |  1 +
> > >  3 files changed, 17 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp
> > > index 97f647a9e53e..f28eb36b7a44 100644
> > > --- a/src/ipa/rpi/common/ipa_base.cpp
> > > +++ b/src/ipa/rpi/common/ipa_base.cpp
> > > @@ -429,11 +429,10 @@ void IpaBase::prepareIsp(const PrepareParams &params)
> > >       }
> > >
> > >       /*
> > > -      * If a statistics buffer has been passed in, call processStats
> > > -      * directly now before prepare() since the statistics are available in-line
> > > -      * with the Bayer frame.
> > > +      * If the statistics are inline (i.e. already available with the Bayer
> > > +      * frame), call processStats() now before prepare().
> > >        */
> > > -     if (params.buffers.stats)
> > > +     if (controller_.getHardwareConfig().statsInline)
> > >               processStats({ params.buffers, params.ipaContext });
> > >
> > >       /* Do we need/want to call prepare? */
> > > @@ -445,6 +444,10 @@ void IpaBase::prepareIsp(const PrepareParams &params)
> > >
> > >       frameCount_++;
> > >
> > > +     /* If the statistics are inline the metadata can be returned early. */
> > > +     if (controller_.getHardwareConfig().statsInline)
> > > +             reportMetadata(ipaContext);
> > > +
> >
> > Can't you drop this one and unconditionally call reportMetadata() in
> > processStats() ?
>
> Sadly not (well, not without a massive amount of rework).  The
> contents of the metadata get collated in platformPrepareIsp(), and
> this run *after* processStats() in statsInline (pisp) case.  So I have
> to do this conditionally as above.
>
> The rework would be to update all the algorithms to report metadata in
> the processStats() phase, but I don't have the stomach for that big
> change :)
>

I see! Thanks for explaining!

So this looks good indeed!
Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

Thanks
  j

> Regards,
> Naush
>
>
> >
> > >       /* Ready to push the input buffer into the ISP. */
> > >       prepareIspComplete.emit(params.buffers, false);
> > >  }
> > > @@ -479,7 +482,13 @@ void IpaBase::processStats(const ProcessParams &params)
> > >               }
> > >       }
> > >
> > > -     reportMetadata(ipaContext);
> > > +     /*
> > > +      * If the statistics are not inline the metadata must be returned now,
> > > +      * before the processStatsComplete signal.
> > > +      */
> > > +     if (!controller_.getHardwareConfig().statsInline)
> > > +             reportMetadata(ipaContext);
> > > +
> > >       processStatsComplete.emit(params.buffers);
> > >  }
> > >
> > > diff --git a/src/ipa/rpi/controller/controller.cpp b/src/ipa/rpi/controller/controller.cpp
> > > index 14d245da2ce7..4b6f82b41916 100644
> > > --- a/src/ipa/rpi/controller/controller.cpp
> > > +++ b/src/ipa/rpi/controller/controller.cpp
> > > @@ -34,7 +34,8 @@ static const std::map<std::string, Controller::HardwareConfig> HardwareConfigMap
> > >                       .focusRegions = { 4, 3 },
> > >                       .numHistogramBins = 128,
> > >                       .numGammaPoints = 33,
> > > -                     .pipelineWidth = 13
> > > +                     .pipelineWidth = 13,
> > > +                     .statsInline = false,
> > >               }
> > >       },
> > >  };
> > > diff --git a/src/ipa/rpi/controller/controller.h b/src/ipa/rpi/controller/controller.h
> > > index c6af5cd6c7d4..a8bc61880ab4 100644
> > > --- a/src/ipa/rpi/controller/controller.h
> > > +++ b/src/ipa/rpi/controller/controller.h
> > > @@ -45,6 +45,7 @@ public:
> > >               unsigned int numHistogramBins;
> > >               unsigned int numGammaPoints;
> > >               unsigned int pipelineWidth;
> > > +             bool statsInline;
> > >       };
> > >
> > >       Controller();
> > > --
> > > 2.34.1
> > >

Patch
diff mbox series 

diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp
index 97f647a9e53e..f28eb36b7a44 100644
--- a/src/ipa/rpi/common/ipa_base.cpp
+++ b/src/ipa/rpi/common/ipa_base.cpp
@@ -429,11 +429,10 @@  void IpaBase::prepareIsp(const PrepareParams &params)
 	}
 
 	/*
-	 * If a statistics buffer has been passed in, call processStats
-	 * directly now before prepare() since the statistics are available in-line
-	 * with the Bayer frame.
+	 * If the statistics are inline (i.e. already available with the Bayer
+	 * frame), call processStats() now before prepare().
 	 */
-	if (params.buffers.stats)
+	if (controller_.getHardwareConfig().statsInline)
 		processStats({ params.buffers, params.ipaContext });
 
 	/* Do we need/want to call prepare? */
@@ -445,6 +444,10 @@  void IpaBase::prepareIsp(const PrepareParams &params)
 
 	frameCount_++;
 
+	/* If the statistics are inline the metadata can be returned early. */
+	if (controller_.getHardwareConfig().statsInline)
+		reportMetadata(ipaContext);
+
 	/* Ready to push the input buffer into the ISP. */
 	prepareIspComplete.emit(params.buffers, false);
 }
@@ -479,7 +482,13 @@  void IpaBase::processStats(const ProcessParams &params)
 		}
 	}
 
-	reportMetadata(ipaContext);
+	/*
+	 * If the statistics are not inline the metadata must be returned now,
+	 * before the processStatsComplete signal.
+	 */
+	if (!controller_.getHardwareConfig().statsInline)
+		reportMetadata(ipaContext);
+
 	processStatsComplete.emit(params.buffers);
 }
 
diff --git a/src/ipa/rpi/controller/controller.cpp b/src/ipa/rpi/controller/controller.cpp
index 14d245da2ce7..4b6f82b41916 100644
--- a/src/ipa/rpi/controller/controller.cpp
+++ b/src/ipa/rpi/controller/controller.cpp
@@ -34,7 +34,8 @@  static const std::map<std::string, Controller::HardwareConfig> HardwareConfigMap
 			.focusRegions = { 4, 3 },
 			.numHistogramBins = 128,
 			.numGammaPoints = 33,
-			.pipelineWidth = 13
+			.pipelineWidth = 13,
+			.statsInline = false,
 		}
 	},
 };
diff --git a/src/ipa/rpi/controller/controller.h b/src/ipa/rpi/controller/controller.h
index c6af5cd6c7d4..a8bc61880ab4 100644
--- a/src/ipa/rpi/controller/controller.h
+++ b/src/ipa/rpi/controller/controller.h
@@ -45,6 +45,7 @@  public:
 		unsigned int numHistogramBins;
 		unsigned int numGammaPoints;
 		unsigned int pipelineWidth;
+		bool statsInline;
 	};
 
 	Controller();