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

[Kieran Bingham]

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 at ideasonboard.com>


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