[libcamera-devel,v4,23/32] ipa: rkisp1: filter: Store per-frame information in frame context
diff mbox series

Message ID 20220908014200.28728-24-laurent.pinchart@ideasonboard.com
State Superseded
Headers show
Series
  • ipa: Frame context queue, IPU3 & RkISP consolidation, and RkISP1 improvements
Related show

Commit Message

Laurent Pinchart Sept. 8, 2022, 1:41 a.m. UTC 
Rework the algorithm's usage of the active state, to store the value of
controls for the last queued request in the queueRequest() function, and
store a copy of the values in the corresponding frame context. The
latter is used in the prepare() function to populate the ISP parameters
with values corresponding to the right frame.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 src/ipa/rkisp1/algorithms/filter.cpp | 30 +++++++++++++++-------------
 src/ipa/rkisp1/ipa_context.cpp       | 18 ++++++++++++++---
 src/ipa/rkisp1/ipa_context.h         |  7 ++++++-
 3 files changed, 37 insertions(+), 18 deletions(-)

Comments

Kieran Bingham Sept. 20, 2022, 11:09 p.m. UTC | #1 
Quoting Laurent Pinchart via libcamera-devel (2022-09-08 02:41:51)
> Rework the algorithm's usage of the active state, to store the value of
> controls for the last queued request in the queueRequest() function, and
> store a copy of the values in the corresponding frame context. The
> latter is used in the prepare() function to populate the ISP parameters
> with values corresponding to the right frame.
> 

Looks good again. I like that the default case with the unsupported
denoise value clearly leaves us such that we assign the most recent /
activeState to the frame context, and mark that no update will occur.

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

> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  src/ipa/rkisp1/algorithms/filter.cpp | 30 +++++++++++++++-------------
>  src/ipa/rkisp1/ipa_context.cpp       | 18 ++++++++++++++---
>  src/ipa/rkisp1/ipa_context.h         |  7 ++++++-
>  3 files changed, 37 insertions(+), 18 deletions(-)
> 
> diff --git a/src/ipa/rkisp1/algorithms/filter.cpp b/src/ipa/rkisp1/algorithms/filter.cpp
> index e64bd6a6d68f..ac743729e247 100644
> --- a/src/ipa/rkisp1/algorithms/filter.cpp
> +++ b/src/ipa/rkisp1/algorithms/filter.cpp
> @@ -44,15 +44,16 @@ static constexpr uint32_t kFiltModeDefault = 0x000004f2;
>   */
>  void Filter::queueRequest(IPAContext &context,
>                           [[maybe_unused]] const uint32_t frame,
> -                         [[maybe_unused]] RkISP1FrameContext &frameContext,
> +                         RkISP1FrameContext &frameContext,
>                           const ControlList &controls)
>  {
>         auto &filter = context.activeState.filter;
> +       bool update = false;
>  
>         const auto &sharpness = controls.get(controls::Sharpness);
>         if (sharpness) {
>                 filter.sharpness = std::round(std::clamp(*sharpness, 0.0f, 10.0f));
> -               filter.updateParams = true;
> +               update = true;
>  
>                 LOG(RkISP1Filter, Debug) << "Set sharpness to " << *sharpness;
>         }
> @@ -64,41 +65,42 @@ void Filter::queueRequest(IPAContext &context,
>                 switch (*denoise) {
>                 case controls::draft::NoiseReductionModeOff:
>                         filter.denoise = 0;
> -                       filter.updateParams = true;
> +                       update = true;
>                         break;
>                 case controls::draft::NoiseReductionModeMinimal:
>                         filter.denoise = 1;
> -                       filter.updateParams = true;
> +                       update = true;
>                         break;
>                 case controls::draft::NoiseReductionModeHighQuality:
>                 case controls::draft::NoiseReductionModeFast:
>                         filter.denoise = 3;
> -                       filter.updateParams = true;
> +                       update = true;
>                         break;
>                 default:
>                         LOG(RkISP1Filter, Error)
>                                 << "Unsupported denoise value "
>                                 << *denoise;
> +                       break;
>                 }
>         }
> +
> +       frameContext.filter.denoise = filter.denoise;
> +       frameContext.filter.sharpness = filter.sharpness;
> +       frameContext.filter.update = update;
>  }
>  
>  /**
>   * \copydoc libcamera::ipa::Algorithm::prepare
>   */
> -void Filter::prepare(IPAContext &context,
> +void Filter::prepare([[maybe_unused]] IPAContext &context,
>                      [[maybe_unused]] const uint32_t frame,
> -                    [[maybe_unused]] RkISP1FrameContext &frameContext,
> +                    RkISP1FrameContext &frameContext,
>                      rkisp1_params_cfg *params)
>  {
> -       auto &filter = context.activeState.filter;
> -
>         /* Check if the algorithm configuration has been updated. */
> -       if (!filter.updateParams)
> +       if (!frameContext.filter.update)
>                 return;
>  
> -       filter.updateParams = false;
> -
>         static constexpr uint16_t filt_fac_sh0[] = {
>                 0x04, 0x07, 0x0a, 0x0c, 0x10, 0x14, 0x1a, 0x1e, 0x24, 0x2a, 0x30
>         };
> @@ -147,8 +149,8 @@ void Filter::prepare(IPAContext &context,
>                 0, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3
>         };
>  
> -       uint8_t denoise = filter.denoise;
> -       uint8_t sharpness = filter.sharpness;
> +       uint8_t denoise = frameContext.filter.denoise;
> +       uint8_t sharpness = frameContext.filter.sharpness;
>         auto &flt_config = params->others.flt_config;
>  
>         flt_config.fac_sh0 = filt_fac_sh0[sharpness];
> diff --git a/src/ipa/rkisp1/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp
> index b0210a978559..b2628ef73d49 100644
> --- a/src/ipa/rkisp1/ipa_context.cpp
> +++ b/src/ipa/rkisp1/ipa_context.cpp
> @@ -179,9 +179,6 @@ namespace libcamera::ipa::rkisp1 {
>   *
>   * \var IPAActiveState::filter.sharpness
>   * \brief Sharpness level
> - *
> - * \var IPAActiveState::filter.updateParams
> - * \brief Indicates if ISP parameters need to be updated
>   */
>  
>  /**
> @@ -260,6 +257,21 @@ namespace libcamera::ipa::rkisp1 {
>   * compared to the previous frame
>   */
>  
> +/**
> + * \var RkISP1FrameContext::filter
> + * \brief Filter parameters for this frame
> + *
> + * \struct RkISP1FrameContext::filter.denoise
> + * \brief Denoising level
> + *
> + * \var RkISP1FrameContext::filter.sharpness
> + * \brief Sharpness level
> + *
> + * \var RkISP1FrameContext::filter.updateParams
> + * \brief Indicates if the filter parameters have been updated compared to the
> + * previous frame
> + */
> +
>  /**
>   * \var RkISP1FrameContext::sensor
>   * \brief Sensor configuration that used been used for this frame
> diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
> index c22e1f099c23..c15b908afcc8 100644
> --- a/src/ipa/rkisp1/ipa_context.h
> +++ b/src/ipa/rkisp1/ipa_context.h
> @@ -84,7 +84,6 @@ struct IPAActiveState {
>         struct {
>                 uint8_t denoise;
>                 uint8_t sharpness;
> -               bool updateParams;
>         } filter;
>  };
>  
> @@ -117,6 +116,12 @@ struct RkISP1FrameContext : public FrameContext {
>                 bool update;
>         } dpf;
>  
> +       struct {
> +               uint8_t denoise;
> +               uint8_t sharpness;
> +               bool update;
> +       } filter;
> +
>         struct {
>                 uint32_t exposure;
>                 double gain;
> -- 
> Regards,
> 
> Laurent Pinchart
>
Jacopo Mondi Sept. 21, 2022, 8:45 p.m. UTC | #2 
Same question as the previous one and same assumption this is
intentional

Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>


On Thu, Sep 08, 2022 at 04:41:51AM +0300, Laurent Pinchart via libcamera-devel wrote:
> Rework the algorithm's usage of the active state, to store the value of
> controls for the last queued request in the queueRequest() function, and
> store a copy of the values in the corresponding frame context. The
> latter is used in the prepare() function to populate the ISP parameters
> with values corresponding to the right frame.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  src/ipa/rkisp1/algorithms/filter.cpp | 30 +++++++++++++++-------------
>  src/ipa/rkisp1/ipa_context.cpp       | 18 ++++++++++++++---
>  src/ipa/rkisp1/ipa_context.h         |  7 ++++++-
>  3 files changed, 37 insertions(+), 18 deletions(-)
>
> diff --git a/src/ipa/rkisp1/algorithms/filter.cpp b/src/ipa/rkisp1/algorithms/filter.cpp
> index e64bd6a6d68f..ac743729e247 100644
> --- a/src/ipa/rkisp1/algorithms/filter.cpp
> +++ b/src/ipa/rkisp1/algorithms/filter.cpp
> @@ -44,15 +44,16 @@ static constexpr uint32_t kFiltModeDefault = 0x000004f2;
>   */
>  void Filter::queueRequest(IPAContext &context,
>  			  [[maybe_unused]] const uint32_t frame,
> -			  [[maybe_unused]] RkISP1FrameContext &frameContext,
> +			  RkISP1FrameContext &frameContext,
>  			  const ControlList &controls)
>  {
>  	auto &filter = context.activeState.filter;
> +	bool update = false;
>
>  	const auto &sharpness = controls.get(controls::Sharpness);
>  	if (sharpness) {
>  		filter.sharpness = std::round(std::clamp(*sharpness, 0.0f, 10.0f));
> -		filter.updateParams = true;
> +		update = true;
>
>  		LOG(RkISP1Filter, Debug) << "Set sharpness to " << *sharpness;
>  	}
> @@ -64,41 +65,42 @@ void Filter::queueRequest(IPAContext &context,
>  		switch (*denoise) {
>  		case controls::draft::NoiseReductionModeOff:
>  			filter.denoise = 0;
> -			filter.updateParams = true;
> +			update = true;
>  			break;
>  		case controls::draft::NoiseReductionModeMinimal:
>  			filter.denoise = 1;
> -			filter.updateParams = true;
> +			update = true;
>  			break;
>  		case controls::draft::NoiseReductionModeHighQuality:
>  		case controls::draft::NoiseReductionModeFast:
>  			filter.denoise = 3;
> -			filter.updateParams = true;
> +			update = true;
>  			break;
>  		default:
>  			LOG(RkISP1Filter, Error)
>  				<< "Unsupported denoise value "
>  				<< *denoise;
> +			break;
>  		}
>  	}
> +
> +	frameContext.filter.denoise = filter.denoise;
> +	frameContext.filter.sharpness = filter.sharpness;
> +	frameContext.filter.update = update;
>  }
>
>  /**
>   * \copydoc libcamera::ipa::Algorithm::prepare
>   */
> -void Filter::prepare(IPAContext &context,
> +void Filter::prepare([[maybe_unused]] IPAContext &context,
>  		     [[maybe_unused]] const uint32_t frame,
> -		     [[maybe_unused]] RkISP1FrameContext &frameContext,
> +		     RkISP1FrameContext &frameContext,
>  		     rkisp1_params_cfg *params)
>  {
> -	auto &filter = context.activeState.filter;
> -
>  	/* Check if the algorithm configuration has been updated. */
> -	if (!filter.updateParams)
> +	if (!frameContext.filter.update)
>  		return;
>
> -	filter.updateParams = false;
> -
>  	static constexpr uint16_t filt_fac_sh0[] = {
>  		0x04, 0x07, 0x0a, 0x0c, 0x10, 0x14, 0x1a, 0x1e, 0x24, 0x2a, 0x30
>  	};
> @@ -147,8 +149,8 @@ void Filter::prepare(IPAContext &context,
>  		0, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3
>  	};
>
> -	uint8_t denoise = filter.denoise;
> -	uint8_t sharpness = filter.sharpness;
> +	uint8_t denoise = frameContext.filter.denoise;
> +	uint8_t sharpness = frameContext.filter.sharpness;
>  	auto &flt_config = params->others.flt_config;
>
>  	flt_config.fac_sh0 = filt_fac_sh0[sharpness];
> diff --git a/src/ipa/rkisp1/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp
> index b0210a978559..b2628ef73d49 100644
> --- a/src/ipa/rkisp1/ipa_context.cpp
> +++ b/src/ipa/rkisp1/ipa_context.cpp
> @@ -179,9 +179,6 @@ namespace libcamera::ipa::rkisp1 {
>   *
>   * \var IPAActiveState::filter.sharpness
>   * \brief Sharpness level
> - *
> - * \var IPAActiveState::filter.updateParams
> - * \brief Indicates if ISP parameters need to be updated
>   */
>
>  /**
> @@ -260,6 +257,21 @@ namespace libcamera::ipa::rkisp1 {
>   * compared to the previous frame
>   */
>
> +/**
> + * \var RkISP1FrameContext::filter
> + * \brief Filter parameters for this frame
> + *
> + * \struct RkISP1FrameContext::filter.denoise
> + * \brief Denoising level
> + *
> + * \var RkISP1FrameContext::filter.sharpness
> + * \brief Sharpness level
> + *
> + * \var RkISP1FrameContext::filter.updateParams
> + * \brief Indicates if the filter parameters have been updated compared to the
> + * previous frame
> + */
> +
>  /**
>   * \var RkISP1FrameContext::sensor
>   * \brief Sensor configuration that used been used for this frame
> diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
> index c22e1f099c23..c15b908afcc8 100644
> --- a/src/ipa/rkisp1/ipa_context.h
> +++ b/src/ipa/rkisp1/ipa_context.h
> @@ -84,7 +84,6 @@ struct IPAActiveState {
>  	struct {
>  		uint8_t denoise;
>  		uint8_t sharpness;
> -		bool updateParams;
>  	} filter;
>  };
>
> @@ -117,6 +116,12 @@ struct RkISP1FrameContext : public FrameContext {
>  		bool update;
>  	} dpf;
>
> +	struct {
> +		uint8_t denoise;
> +		uint8_t sharpness;
> +		bool update;
> +	} filter;
> +
>  	struct {
>  		uint32_t exposure;
>  		double gain;
> --
> Regards,
>
> Laurent Pinchart
>

Patch
diff mbox series 

diff --git a/src/ipa/rkisp1/algorithms/filter.cpp b/src/ipa/rkisp1/algorithms/filter.cpp
index e64bd6a6d68f..ac743729e247 100644
--- a/src/ipa/rkisp1/algorithms/filter.cpp
+++ b/src/ipa/rkisp1/algorithms/filter.cpp
@@ -44,15 +44,16 @@  static constexpr uint32_t kFiltModeDefault = 0x000004f2;
  */
 void Filter::queueRequest(IPAContext &context,
 			  [[maybe_unused]] const uint32_t frame,
-			  [[maybe_unused]] RkISP1FrameContext &frameContext,
+			  RkISP1FrameContext &frameContext,
 			  const ControlList &controls)
 {
 	auto &filter = context.activeState.filter;
+	bool update = false;
 
 	const auto &sharpness = controls.get(controls::Sharpness);
 	if (sharpness) {
 		filter.sharpness = std::round(std::clamp(*sharpness, 0.0f, 10.0f));
-		filter.updateParams = true;
+		update = true;
 
 		LOG(RkISP1Filter, Debug) << "Set sharpness to " << *sharpness;
 	}
@@ -64,41 +65,42 @@  void Filter::queueRequest(IPAContext &context,
 		switch (*denoise) {
 		case controls::draft::NoiseReductionModeOff:
 			filter.denoise = 0;
-			filter.updateParams = true;
+			update = true;
 			break;
 		case controls::draft::NoiseReductionModeMinimal:
 			filter.denoise = 1;
-			filter.updateParams = true;
+			update = true;
 			break;
 		case controls::draft::NoiseReductionModeHighQuality:
 		case controls::draft::NoiseReductionModeFast:
 			filter.denoise = 3;
-			filter.updateParams = true;
+			update = true;
 			break;
 		default:
 			LOG(RkISP1Filter, Error)
 				<< "Unsupported denoise value "
 				<< *denoise;
+			break;
 		}
 	}
+
+	frameContext.filter.denoise = filter.denoise;
+	frameContext.filter.sharpness = filter.sharpness;
+	frameContext.filter.update = update;
 }
 
 /**
  * \copydoc libcamera::ipa::Algorithm::prepare
  */
-void Filter::prepare(IPAContext &context,
+void Filter::prepare([[maybe_unused]] IPAContext &context,
 		     [[maybe_unused]] const uint32_t frame,
-		     [[maybe_unused]] RkISP1FrameContext &frameContext,
+		     RkISP1FrameContext &frameContext,
 		     rkisp1_params_cfg *params)
 {
-	auto &filter = context.activeState.filter;
-
 	/* Check if the algorithm configuration has been updated. */
-	if (!filter.updateParams)
+	if (!frameContext.filter.update)
 		return;
 
-	filter.updateParams = false;
-
 	static constexpr uint16_t filt_fac_sh0[] = {
 		0x04, 0x07, 0x0a, 0x0c, 0x10, 0x14, 0x1a, 0x1e, 0x24, 0x2a, 0x30
 	};
@@ -147,8 +149,8 @@  void Filter::prepare(IPAContext &context,
 		0, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3
 	};
 
-	uint8_t denoise = filter.denoise;
-	uint8_t sharpness = filter.sharpness;
+	uint8_t denoise = frameContext.filter.denoise;
+	uint8_t sharpness = frameContext.filter.sharpness;
 	auto &flt_config = params->others.flt_config;
 
 	flt_config.fac_sh0 = filt_fac_sh0[sharpness];
diff --git a/src/ipa/rkisp1/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp
index b0210a978559..b2628ef73d49 100644
--- a/src/ipa/rkisp1/ipa_context.cpp
+++ b/src/ipa/rkisp1/ipa_context.cpp
@@ -179,9 +179,6 @@  namespace libcamera::ipa::rkisp1 {
  *
  * \var IPAActiveState::filter.sharpness
  * \brief Sharpness level
- *
- * \var IPAActiveState::filter.updateParams
- * \brief Indicates if ISP parameters need to be updated
  */
 
 /**
@@ -260,6 +257,21 @@  namespace libcamera::ipa::rkisp1 {
  * compared to the previous frame
  */
 
+/**
+ * \var RkISP1FrameContext::filter
+ * \brief Filter parameters for this frame
+ *
+ * \struct RkISP1FrameContext::filter.denoise
+ * \brief Denoising level
+ *
+ * \var RkISP1FrameContext::filter.sharpness
+ * \brief Sharpness level
+ *
+ * \var RkISP1FrameContext::filter.updateParams
+ * \brief Indicates if the filter parameters have been updated compared to the
+ * previous frame
+ */
+
 /**
  * \var RkISP1FrameContext::sensor
  * \brief Sensor configuration that used been used for this frame
diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
index c22e1f099c23..c15b908afcc8 100644
--- a/src/ipa/rkisp1/ipa_context.h
+++ b/src/ipa/rkisp1/ipa_context.h
@@ -84,7 +84,6 @@  struct IPAActiveState {
 	struct {
 		uint8_t denoise;
 		uint8_t sharpness;
-		bool updateParams;
 	} filter;
 };
 
@@ -117,6 +116,12 @@  struct RkISP1FrameContext : public FrameContext {
 		bool update;
 	} dpf;
 
+	struct {
+		uint8_t denoise;
+		uint8_t sharpness;
+		bool update;
+	} filter;
+
 	struct {
 		uint32_t exposure;
 		double gain;