[libcamera-devel] [PATCH v4 8/9] ipa: rkisp1: Add enable field for LSC algorithm in IPA context

[paul.elder at ideasonboard.com]

Hi Laurent,

On Thu, Aug 18, 2022 at 08:51:49PM +0300, Laurent Pinchart wrote:
> Hi Paul,
> 
> On Thu, Aug 18, 2022 at 10:18:03PM +0900, paul.elder at ideasonboard.com wrote:
> > On Tue, Aug 16, 2022 at 04:54:13AM +0300, Laurent Pinchart via libcamera-devel wrote:
> > > From: Florian Sylvestre <fsylvestre at baylibre.com>
> > > 
> > > Add an enable variable in the lsc struct in IPASessionConfiguration which
> > > indicates if the lsc algorithm has been configured. This will allow other
> > > algorithms to retrieve this information.
> > > 
> > > Signed-off-by: Florian Sylvestre <fsylvestre at baylibre.com>
> > > Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > > Reviewed-by: Umang Jain <umang.jain at ideasonboard.com>
> > > ---
> > > Changes since v1:
> > > 
> > > - Improve documentation
> > > ---
> > >  src/ipa/rkisp1/algorithms/lsc.cpp | 10 ++++++++++
> > >  src/ipa/rkisp1/algorithms/lsc.h   |  1 +
> > >  src/ipa/rkisp1/ipa_context.cpp    |  8 ++++++++
> > >  src/ipa/rkisp1/ipa_context.h      |  4 ++++
> > >  4 files changed, 23 insertions(+)
> > > 
> > > diff --git a/src/ipa/rkisp1/algorithms/lsc.cpp b/src/ipa/rkisp1/algorithms/lsc.cpp
> > > index 05c8c0dab5c8..da287ac7af75 100644
> > > --- a/src/ipa/rkisp1/algorithms/lsc.cpp
> > > +++ b/src/ipa/rkisp1/algorithms/lsc.cpp
> > > @@ -119,6 +119,16 @@ int LensShadingCorrection::init([[maybe_unused]] IPAContext &context,
> > >  	return 0;
> > >  }
> > >  
> > > +/**
> > > + * \copydoc libcamera::ipa::Algorithm::configure
> > > + */
> > > +int LensShadingCorrection::configure(IPAContext &context,
> > > +				     [[maybe_unused]] const IPACameraSensorInfo &configInfo)
> > > +{
> > > +	context.configuration.lsc.enabled = initialized_;
> > > +	return 0;
> > > +}
> > > +
> > >  /**
> > >   * \copydoc libcamera::ipa::Algorithm::prepare
> > >   */
> > > diff --git a/src/ipa/rkisp1/algorithms/lsc.h b/src/ipa/rkisp1/algorithms/lsc.h
> > > index fdb2ec1dd27d..f68602c005c4 100644
> > > --- a/src/ipa/rkisp1/algorithms/lsc.h
> > > +++ b/src/ipa/rkisp1/algorithms/lsc.h
> > > @@ -20,6 +20,7 @@ public:
> > >  	~LensShadingCorrection() = default;
> > >  
> > >  	int init(IPAContext &context, const YamlObject &tuningData) override;
> > > +	int configure(IPAContext &context, const IPACameraSensorInfo &configInfo) override;
> > >  	void prepare(IPAContext &context, rkisp1_params_cfg *params) override;
> > >  
> > >  private:
> > > diff --git a/src/ipa/rkisp1/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp
> > > index 23a63f8c6e25..1a549c092d73 100644
> > > --- a/src/ipa/rkisp1/ipa_context.cpp
> > > +++ b/src/ipa/rkisp1/ipa_context.cpp
> > > @@ -92,6 +92,14 @@ namespace libcamera::ipa::rkisp1 {
> > >   * \brief Indicates if the AWB hardware is enabled to apply colour gains
> > >   */
> > >  
> > > +/**
> > > + * \var IPASessionConfiguration::lsc
> > > + * \brief Lens Shading Correction configuration of the IPA
> > > + *
> > > + * \var IPASessionConfiguration::lsc.enabled
> > > + * \brief Indicates if the LSC hardware is enabled
> > 
> > imo this doesn't match the intent that's described in the changelog:
> > 
> > "Add an enable variable <snip> which indicates if the lsc algorithm has been configured"
> 
> I'll update the commit message to "[...] indicates if the LSC hardware
> module is enabled".

Sounds good.

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

> 
> > > + */
> > > +
> > >  /**
> > >   * \var IPASessionConfiguration::sensor
> > >   * \brief Sensor-specific configuration of the IPA
> > > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
> > > index 7f7b3e4d88fa..0cd6aadb83ed 100644
> > > --- a/src/ipa/rkisp1/ipa_context.h
> > > +++ b/src/ipa/rkisp1/ipa_context.h
> > > @@ -32,6 +32,10 @@ struct IPASessionConfiguration {
> > >  		bool enabled;
> > >  	} awb;
> > >  
> > > +	struct {
> > > +		bool enabled;
> > > +	} lsc;
> > > +
> > >  	struct {
> > >  		utils::Duration lineDuration;
> > >  		Size size;