[libcamera-devel] [PATCH v5 17/33] ipa: libipa: Pass FCQueue size as template argument

[Jacopo Mondi]

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 at 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
>