[libcamera-devel,v5,17/33] ipa: libipa: Pass FCQueue size as template argument
diff mbox series

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

Commit Message

Laurent Pinchart Sept. 27, 2022, 2:36 a.m. UTC 
The frame context queue size is meant to be a compile-time constant, not
a variable that can change between runs. Pass it to the class as a
template argument.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 src/ipa/ipu3/ipa_context.h   |  5 ++++-
 src/ipa/ipu3/ipu3.cpp        |  5 +----
 src/ipa/libipa/fc_queue.cpp  |  6 +++---
 src/ipa/libipa/fc_queue.h    | 17 ++++++++---------
 src/ipa/rkisp1/ipa_context.h |  4 +++-
 src/ipa/rkisp1/rkisp1.cpp    |  4 +---
 6 files changed, 20 insertions(+), 21 deletions(-)

Comments

Jacopo Mondi Sept. 27, 2022, 9:17 a.m. UTC | #1 
Up to you if you want to take this patch in or not :)

On Tue, Sep 27, 2022 at 05:36:26AM +0300, Laurent Pinchart via libcamera-devel wrote:
> The frame context queue size is meant to be a compile-time constant, not
> a variable that can change between runs. Pass it to the class as a
> template argument.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  src/ipa/ipu3/ipa_context.h   |  5 ++++-
>  src/ipa/ipu3/ipu3.cpp        |  5 +----
>  src/ipa/libipa/fc_queue.cpp  |  6 +++---
>  src/ipa/libipa/fc_queue.h    | 17 ++++++++---------
>  src/ipa/rkisp1/ipa_context.h |  4 +++-
>  src/ipa/rkisp1/rkisp1.cpp    |  4 +---
>  6 files changed, 20 insertions(+), 21 deletions(-)
>
> diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h
> index 36099353e9f2..bdba3d07d13a 100644
> --- a/src/ipa/ipu3/ipa_context.h
> +++ b/src/ipa/ipu3/ipa_context.h
> @@ -79,11 +79,14 @@ struct IPAFrameContext : public FrameContext {
>  	} sensor;
>  };
>
> +/* Maximum number of frame contexts to be held */
> +static constexpr uint32_t kMaxFrameContexts = 16;
> +
>  struct IPAContext {
>  	IPASessionConfiguration configuration;
>  	IPAActiveState activeState;
>
> -	FCQueue<IPAFrameContext> frameContexts;
> +	FCQueue<IPAFrameContext, kMaxFrameContexts> frameContexts;
>  };
>
>  } /* namespace ipa::ipu3 */
> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> index d1ea081d595d..f5292a77e4d3 100644
> --- a/src/ipa/ipu3/ipu3.cpp
> +++ b/src/ipa/ipu3/ipu3.cpp
> @@ -55,9 +55,6 @@ static constexpr uint32_t kMinCellSizeLog2 = 3;
>  /* log2 of the maximum grid cell width and height, in pixels */
>  static constexpr uint32_t kMaxCellSizeLog2 = 6;
>
> -/* Maximum number of frame contexts to be held */
> -static constexpr uint32_t kMaxFrameContexts = 16;
> -
>  namespace libcamera {
>
>  LOG_DEFINE_CATEGORY(IPAIPU3)
> @@ -191,7 +188,7 @@ private:
>  };
>
>  IPAIPU3::IPAIPU3()
> -	: context_({ {}, {}, { kMaxFrameContexts } })
> +	: context_({ {}, {}, {} })
>  {
>  }
>
> diff --git a/src/ipa/libipa/fc_queue.cpp b/src/ipa/libipa/fc_queue.cpp
> index e812faa505a5..9b5702ec5eb0 100644
> --- a/src/ipa/libipa/fc_queue.cpp
> +++ b/src/ipa/libipa/fc_queue.cpp
> @@ -42,6 +42,7 @@ namespace ipa {
>   * \class FCQueue
>   * \brief A support class for managing FrameContext instances in IPA modules
>   * \tparam FrameContext The IPA module-specific FrameContext derived class type
> + * \tparam Size The number of contexts in the queue
>   *
>   * Along with the Module and Algorithm classes, the frame context queue is a
>   * core component of the libipa infrastructure. It stores per-frame contexts
> @@ -90,9 +91,8 @@ namespace ipa {
>   */
>
>  /**
> - * \fn FCQueue::FCQueue(unsigned int size)
> - * \brief Construct a frame contexts queue of a specified size
> - * \param[in] size The number of contexts in the queue
> + * \fn FCQueue::FCQueue()
> + * \brief Construct a frame contexts queue
>   */
>
>  /**
> diff --git a/src/ipa/libipa/fc_queue.h b/src/ipa/libipa/fc_queue.h
> index a589e7e1031b..b9c8c6fe5a62 100644
> --- a/src/ipa/libipa/fc_queue.h
> +++ b/src/ipa/libipa/fc_queue.h
> @@ -7,8 +7,8 @@
>
>  #pragma once
>
> +#include <array>
>  #include <stdint.h>
> -#include <vector>
>
>  #include <libcamera/base/log.h>
>
> @@ -18,21 +18,20 @@ LOG_DECLARE_CATEGORY(FCQueue)
>
>  namespace ipa {
>
> -template<typename FrameContext>
> +template<typename FrameContext, std::size_t Size>
>  class FCQueue;
>
>  struct FrameContext {
>  private:
> -	template<typename T> friend class FCQueue;
> +	template<typename T, std::size_t> friend class FCQueue;
>  	uint32_t frame;
>  };
>
> -template<typename FrameContext>
> +template<typename FrameContext, std::size_t Size>
>  class FCQueue
>  {
>  public:
> -	FCQueue(unsigned int size)
> -		: contexts_(size)
> +	FCQueue()
>  	{
>  	}
>
> @@ -44,7 +43,7 @@ public:
>
>  	FrameContext &alloc(const uint32_t frame)
>  	{
> -		FrameContext &frameContext = contexts_[frame % contexts_.size()];
> +		FrameContext &frameContext = contexts_[frame % Size];
>
>  		/*
>  		 * Do not re-initialise if a get() call has already fetched this
> @@ -68,7 +67,7 @@ public:
>
>  	FrameContext &get(uint32_t frame)
>  	{
> -		FrameContext &frameContext = contexts_[frame % contexts_.size()];
> +		FrameContext &frameContext = contexts_[frame % Size];
>
>  		/*
>  		 * If the IPA algorithms try to access a frame context slot which
> @@ -110,7 +109,7 @@ private:
>  		frameContext.frame = frame;
>  	}
>
> -	std::vector<FrameContext> contexts_;
> +	std::array<FrameContext, Size> contexts_;
>  };
>
>  } /* namespace ipa */
> diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
> index f6aaefffed52..4481bd2acde6 100644
> --- a/src/ipa/rkisp1/ipa_context.h
> +++ b/src/ipa/rkisp1/ipa_context.h
> @@ -94,11 +94,13 @@ struct IPAActiveState {
>  struct IPAFrameContext : public FrameContext {
>  };
>
> +static constexpr uint32_t kMaxFrameContexts = 16;
> +
>  struct IPAContext {
>  	IPASessionConfiguration configuration;
>  	IPAActiveState activeState;
>
> -	FCQueue<IPAFrameContext> frameContexts;
> +	FCQueue<IPAFrameContext, kMaxFrameContexts> frameContexts;
>  };
>
>  } /* namespace ipa::rkisp1 */
> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> index eb3481949897..b7c1ecfee4c8 100644
> --- a/src/ipa/rkisp1/rkisp1.cpp
> +++ b/src/ipa/rkisp1/rkisp1.cpp
> @@ -41,8 +41,6 @@ using namespace std::literals::chrono_literals;
>  namespace ipa::rkisp1 {
>
>  /* Maximum number of frame contexts to be held */
> -static constexpr uint32_t kMaxFrameContexts = 16;
> -
>  class IPARkISP1 : public IPARkISP1Interface, public Module
>  {
>  public:
> @@ -106,7 +104,7 @@ const ControlInfoMap::Map rkisp1Controls{
>  } /* namespace */
>
>  IPARkISP1::IPARkISP1()
> -	: context_({ {}, {}, { kMaxFrameContexts } })
> +	: context_({ {}, {}, {} })
>  {
>  }
>
> --
> Regards,
>
> Laurent Pinchart
>
Kieran Bingham Sept. 27, 2022, 9:46 a.m. UTC | #2 
Quoting Laurent Pinchart via libcamera-devel (2022-09-27 03:36:26)
> The frame context queue size is meant to be a compile-time constant, not
> a variable that can change between runs. Pass it to the class as a
> template argument.

This is fine with me.

If we really hit something that means we need to communicate a more
dynamic size at runtime, we'll handle it then.


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


> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  src/ipa/ipu3/ipa_context.h   |  5 ++++-
>  src/ipa/ipu3/ipu3.cpp        |  5 +----
>  src/ipa/libipa/fc_queue.cpp  |  6 +++---
>  src/ipa/libipa/fc_queue.h    | 17 ++++++++---------
>  src/ipa/rkisp1/ipa_context.h |  4 +++-
>  src/ipa/rkisp1/rkisp1.cpp    |  4 +---
>  6 files changed, 20 insertions(+), 21 deletions(-)
> 
> diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h
> index 36099353e9f2..bdba3d07d13a 100644
> --- a/src/ipa/ipu3/ipa_context.h
> +++ b/src/ipa/ipu3/ipa_context.h
> @@ -79,11 +79,14 @@ struct IPAFrameContext : public FrameContext {
>         } sensor;
>  };
>  
> +/* Maximum number of frame contexts to be held */
> +static constexpr uint32_t kMaxFrameContexts = 16;
> +
>  struct IPAContext {
>         IPASessionConfiguration configuration;
>         IPAActiveState activeState;
>  
> -       FCQueue<IPAFrameContext> frameContexts;
> +       FCQueue<IPAFrameContext, kMaxFrameContexts> frameContexts;
>  };
>  
>  } /* namespace ipa::ipu3 */
> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> index d1ea081d595d..f5292a77e4d3 100644
> --- a/src/ipa/ipu3/ipu3.cpp
> +++ b/src/ipa/ipu3/ipu3.cpp
> @@ -55,9 +55,6 @@ static constexpr uint32_t kMinCellSizeLog2 = 3;
>  /* log2 of the maximum grid cell width and height, in pixels */
>  static constexpr uint32_t kMaxCellSizeLog2 = 6;
>  
> -/* Maximum number of frame contexts to be held */
> -static constexpr uint32_t kMaxFrameContexts = 16;
> -
>  namespace libcamera {
>  
>  LOG_DEFINE_CATEGORY(IPAIPU3)
> @@ -191,7 +188,7 @@ private:
>  };
>  
>  IPAIPU3::IPAIPU3()
> -       : context_({ {}, {}, { kMaxFrameContexts } })
> +       : context_({ {}, {}, {} })
>  {
>  }
>  
> diff --git a/src/ipa/libipa/fc_queue.cpp b/src/ipa/libipa/fc_queue.cpp
> index e812faa505a5..9b5702ec5eb0 100644
> --- a/src/ipa/libipa/fc_queue.cpp
> +++ b/src/ipa/libipa/fc_queue.cpp
> @@ -42,6 +42,7 @@ namespace ipa {
>   * \class FCQueue
>   * \brief A support class for managing FrameContext instances in IPA modules
>   * \tparam FrameContext The IPA module-specific FrameContext derived class type
> + * \tparam Size The number of contexts in the queue
>   *
>   * Along with the Module and Algorithm classes, the frame context queue is a
>   * core component of the libipa infrastructure. It stores per-frame contexts
> @@ -90,9 +91,8 @@ namespace ipa {
>   */
>  
>  /**
> - * \fn FCQueue::FCQueue(unsigned int size)
> - * \brief Construct a frame contexts queue of a specified size
> - * \param[in] size The number of contexts in the queue
> + * \fn FCQueue::FCQueue()
> + * \brief Construct a frame contexts queue
>   */
>  
>  /**
> diff --git a/src/ipa/libipa/fc_queue.h b/src/ipa/libipa/fc_queue.h
> index a589e7e1031b..b9c8c6fe5a62 100644
> --- a/src/ipa/libipa/fc_queue.h
> +++ b/src/ipa/libipa/fc_queue.h
> @@ -7,8 +7,8 @@
>  
>  #pragma once
>  
> +#include <array>
>  #include <stdint.h>
> -#include <vector>
>  
>  #include <libcamera/base/log.h>
>  
> @@ -18,21 +18,20 @@ LOG_DECLARE_CATEGORY(FCQueue)
>  
>  namespace ipa {
>  
> -template<typename FrameContext>
> +template<typename FrameContext, std::size_t Size>
>  class FCQueue;
>  
>  struct FrameContext {
>  private:
> -       template<typename T> friend class FCQueue;
> +       template<typename T, std::size_t> friend class FCQueue;
>         uint32_t frame;
>  };
>  
> -template<typename FrameContext>
> +template<typename FrameContext, std::size_t Size>
>  class FCQueue
>  {
>  public:
> -       FCQueue(unsigned int size)
> -               : contexts_(size)
> +       FCQueue()
>         {
>         }
>  
> @@ -44,7 +43,7 @@ public:
>  
>         FrameContext &alloc(const uint32_t frame)
>         {
> -               FrameContext &frameContext = contexts_[frame % contexts_.size()];
> +               FrameContext &frameContext = contexts_[frame % Size];
>  
>                 /*
>                  * Do not re-initialise if a get() call has already fetched this
> @@ -68,7 +67,7 @@ public:
>  
>         FrameContext &get(uint32_t frame)
>         {
> -               FrameContext &frameContext = contexts_[frame % contexts_.size()];
> +               FrameContext &frameContext = contexts_[frame % Size];
>  
>                 /*
>                  * If the IPA algorithms try to access a frame context slot which
> @@ -110,7 +109,7 @@ private:
>                 frameContext.frame = frame;
>         }
>  
> -       std::vector<FrameContext> contexts_;
> +       std::array<FrameContext, Size> contexts_;
>  };
>  
>  } /* namespace ipa */
> diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
> index f6aaefffed52..4481bd2acde6 100644
> --- a/src/ipa/rkisp1/ipa_context.h
> +++ b/src/ipa/rkisp1/ipa_context.h
> @@ -94,11 +94,13 @@ struct IPAActiveState {
>  struct IPAFrameContext : public FrameContext {
>  };
>  
> +static constexpr uint32_t kMaxFrameContexts = 16;
> +
>  struct IPAContext {
>         IPASessionConfiguration configuration;
>         IPAActiveState activeState;
>  
> -       FCQueue<IPAFrameContext> frameContexts;
> +       FCQueue<IPAFrameContext, kMaxFrameContexts> frameContexts;
>  };
>  
>  } /* namespace ipa::rkisp1 */
> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> index eb3481949897..b7c1ecfee4c8 100644
> --- a/src/ipa/rkisp1/rkisp1.cpp
> +++ b/src/ipa/rkisp1/rkisp1.cpp
> @@ -41,8 +41,6 @@ using namespace std::literals::chrono_literals;
>  namespace ipa::rkisp1 {
>  
>  /* Maximum number of frame contexts to be held */
> -static constexpr uint32_t kMaxFrameContexts = 16;
> -
>  class IPARkISP1 : public IPARkISP1Interface, public Module
>  {
>  public:
> @@ -106,7 +104,7 @@ const ControlInfoMap::Map rkisp1Controls{
>  } /* namespace */
>  
>  IPARkISP1::IPARkISP1()
> -       : context_({ {}, {}, { kMaxFrameContexts } })
> +       : context_({ {}, {}, {} })
>  {
>  }
>  
> -- 
> Regards,
> 
> Laurent Pinchart
>

Patch
diff mbox series 

diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h
index 36099353e9f2..bdba3d07d13a 100644
--- a/src/ipa/ipu3/ipa_context.h
+++ b/src/ipa/ipu3/ipa_context.h
@@ -79,11 +79,14 @@  struct IPAFrameContext : public FrameContext {
 	} sensor;
 };
 
+/* Maximum number of frame contexts to be held */
+static constexpr uint32_t kMaxFrameContexts = 16;
+
 struct IPAContext {
 	IPASessionConfiguration configuration;
 	IPAActiveState activeState;
 
-	FCQueue<IPAFrameContext> frameContexts;
+	FCQueue<IPAFrameContext, kMaxFrameContexts> frameContexts;
 };
 
 } /* namespace ipa::ipu3 */
diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
index d1ea081d595d..f5292a77e4d3 100644
--- a/src/ipa/ipu3/ipu3.cpp
+++ b/src/ipa/ipu3/ipu3.cpp
@@ -55,9 +55,6 @@  static constexpr uint32_t kMinCellSizeLog2 = 3;
 /* log2 of the maximum grid cell width and height, in pixels */
 static constexpr uint32_t kMaxCellSizeLog2 = 6;
 
-/* Maximum number of frame contexts to be held */
-static constexpr uint32_t kMaxFrameContexts = 16;
-
 namespace libcamera {
 
 LOG_DEFINE_CATEGORY(IPAIPU3)
@@ -191,7 +188,7 @@  private:
 };
 
 IPAIPU3::IPAIPU3()
-	: context_({ {}, {}, { kMaxFrameContexts } })
+	: context_({ {}, {}, {} })
 {
 }
 
diff --git a/src/ipa/libipa/fc_queue.cpp b/src/ipa/libipa/fc_queue.cpp
index e812faa505a5..9b5702ec5eb0 100644
--- a/src/ipa/libipa/fc_queue.cpp
+++ b/src/ipa/libipa/fc_queue.cpp
@@ -42,6 +42,7 @@  namespace ipa {
  * \class FCQueue
  * \brief A support class for managing FrameContext instances in IPA modules
  * \tparam FrameContext The IPA module-specific FrameContext derived class type
+ * \tparam Size The number of contexts in the queue
  *
  * Along with the Module and Algorithm classes, the frame context queue is a
  * core component of the libipa infrastructure. It stores per-frame contexts
@@ -90,9 +91,8 @@  namespace ipa {
  */
 
 /**
- * \fn FCQueue::FCQueue(unsigned int size)
- * \brief Construct a frame contexts queue of a specified size
- * \param[in] size The number of contexts in the queue
+ * \fn FCQueue::FCQueue()
+ * \brief Construct a frame contexts queue
  */
 
 /**
diff --git a/src/ipa/libipa/fc_queue.h b/src/ipa/libipa/fc_queue.h
index a589e7e1031b..b9c8c6fe5a62 100644
--- a/src/ipa/libipa/fc_queue.h
+++ b/src/ipa/libipa/fc_queue.h
@@ -7,8 +7,8 @@ 
 
 #pragma once
 
+#include <array>
 #include <stdint.h>
-#include <vector>
 
 #include <libcamera/base/log.h>
 
@@ -18,21 +18,20 @@  LOG_DECLARE_CATEGORY(FCQueue)
 
 namespace ipa {
 
-template<typename FrameContext>
+template<typename FrameContext, std::size_t Size>
 class FCQueue;
 
 struct FrameContext {
 private:
-	template<typename T> friend class FCQueue;
+	template<typename T, std::size_t> friend class FCQueue;
 	uint32_t frame;
 };
 
-template<typename FrameContext>
+template<typename FrameContext, std::size_t Size>
 class FCQueue
 {
 public:
-	FCQueue(unsigned int size)
-		: contexts_(size)
+	FCQueue()
 	{
 	}
 
@@ -44,7 +43,7 @@  public:
 
 	FrameContext &alloc(const uint32_t frame)
 	{
-		FrameContext &frameContext = contexts_[frame % contexts_.size()];
+		FrameContext &frameContext = contexts_[frame % Size];
 
 		/*
 		 * Do not re-initialise if a get() call has already fetched this
@@ -68,7 +67,7 @@  public:
 
 	FrameContext &get(uint32_t frame)
 	{
-		FrameContext &frameContext = contexts_[frame % contexts_.size()];
+		FrameContext &frameContext = contexts_[frame % Size];
 
 		/*
 		 * If the IPA algorithms try to access a frame context slot which
@@ -110,7 +109,7 @@  private:
 		frameContext.frame = frame;
 	}
 
-	std::vector<FrameContext> contexts_;
+	std::array<FrameContext, Size> contexts_;
 };
 
 } /* namespace ipa */
diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
index f6aaefffed52..4481bd2acde6 100644
--- a/src/ipa/rkisp1/ipa_context.h
+++ b/src/ipa/rkisp1/ipa_context.h
@@ -94,11 +94,13 @@  struct IPAActiveState {
 struct IPAFrameContext : public FrameContext {
 };
 
+static constexpr uint32_t kMaxFrameContexts = 16;
+
 struct IPAContext {
 	IPASessionConfiguration configuration;
 	IPAActiveState activeState;
 
-	FCQueue<IPAFrameContext> frameContexts;
+	FCQueue<IPAFrameContext, kMaxFrameContexts> frameContexts;
 };
 
 } /* namespace ipa::rkisp1 */
diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
index eb3481949897..b7c1ecfee4c8 100644
--- a/src/ipa/rkisp1/rkisp1.cpp
+++ b/src/ipa/rkisp1/rkisp1.cpp
@@ -41,8 +41,6 @@  using namespace std::literals::chrono_literals;
 namespace ipa::rkisp1 {
 
 /* Maximum number of frame contexts to be held */
-static constexpr uint32_t kMaxFrameContexts = 16;
-
 class IPARkISP1 : public IPARkISP1Interface, public Module
 {
 public:
@@ -106,7 +104,7 @@  const ControlInfoMap::Map rkisp1Controls{
 } /* namespace */
 
 IPARkISP1::IPARkISP1()
-	: context_({ {}, {}, { kMaxFrameContexts } })
+	: context_({ {}, {}, {} })
 {
 }