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

Message ID 20220816015414.7462-9-laurent.pinchart@ideasonboard.com
State Accepted
Headers show
Series
  • Add DPF tuning support for RkISP1
Related show

Commit Message

Laurent Pinchart Aug. 16, 2022, 1:54 a.m. UTC 
From: Florian Sylvestre <fsylvestre@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@baylibre.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Umang Jain <umang.jain@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(+)

Comments

Nicolas Dufresne via libcamera-devel Aug. 18, 2022, 1:18 p.m. UTC | #1 
Hi Laurent,

On Tue, Aug 16, 2022 at 04:54:13AM +0300, Laurent Pinchart via libcamera-devel wrote:
> From: Florian Sylvestre <fsylvestre@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@baylibre.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Reviewed-by: Umang Jain <umang.jain@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"


Paul


> + */
> +
>  /**
>   * \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;
Laurent Pinchart Aug. 18, 2022, 5:51 p.m. UTC | #2 
Hi Paul,

On Thu, Aug 18, 2022 at 10:18:03PM +0900, paul.elder@ideasonboard.com wrote:
> On Tue, Aug 16, 2022 at 04:54:13AM +0300, Laurent Pinchart via libcamera-devel wrote:
> > From: Florian Sylvestre <fsylvestre@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@baylibre.com>
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Reviewed-by: Umang Jain <umang.jain@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".

> > + */
> > +
> >  /**
> >   * \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;
Nicolas Dufresne via libcamera-devel Aug. 21, 2022, 10:12 a.m. UTC | #3 
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@ideasonboard.com wrote:
> > On Tue, Aug 16, 2022 at 04:54:13AM +0300, Laurent Pinchart via libcamera-devel wrote:
> > > From: Florian Sylvestre <fsylvestre@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@baylibre.com>
> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > Reviewed-by: Umang Jain <umang.jain@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@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;

Patch
diff mbox series 

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
+ */
+
 /**
  * \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;