[libcamera-devel] [PATCH v4 10/32] ipa: ipu3: Use the FCQueue

[Kieran Bingham]

Quoting Laurent Pinchart via libcamera-devel (2022-09-08 02:41:38)
> Replace the manual ring buffer implementation with the FCQueue class
> from libipa.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> ---
>  src/ipa/ipu3/ipa_context.cpp | 14 --------------
>  src/ipa/ipu3/ipa_context.h   | 10 +---------
>  src/ipa/ipu3/ipu3.cpp        | 32 ++++++++++++++++++++++++--------
>  3 files changed, 25 insertions(+), 31 deletions(-)
> 
> diff --git a/src/ipa/ipu3/ipa_context.cpp b/src/ipa/ipu3/ipa_context.cpp
> index 9cfca0db3a0d..6904ccbbdf8b 100644
> --- a/src/ipa/ipu3/ipa_context.cpp
> +++ b/src/ipa/ipu3/ipa_context.cpp
> @@ -164,20 +164,6 @@ namespace libcamera::ipa::ipu3 {
>   * <linux/intel-ipu3.h> struct ipu3_uapi_gamma_corr_lut for further details.
>   */
>  
> -/**
> - * \brief Default constructor for IPAFrameContext
> - */
> -IPAFrameContext::IPAFrameContext() = default;
> -
> -/**
> - * \brief Construct a IPAFrameContext instance
> - */
> -IPAFrameContext::IPAFrameContext(const ControlList &reqControls)
> -       : frameControls(reqControls)
> -{
> -       sensor = {};
> -}
> -
>  /**
>   * \struct IPAFrameContext
>   * \brief IPU3-specific FrameContext
> diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h
> index e8fc42769075..bfc0196e098a 100644
> --- a/src/ipa/ipu3/ipa_context.h
> +++ b/src/ipa/ipu3/ipa_context.h
> @@ -8,8 +8,6 @@
>  
>  #pragma once
>  
> -#include <array>
> -
>  #include <linux/intel-ipu3.h>
>  
>  #include <libcamera/base/utils.h>
> @@ -23,9 +21,6 @@ namespace libcamera {
>  
>  namespace ipa::ipu3 {
>  
> -/* Maximum number of frame contexts to be held */
> -static constexpr uint32_t kMaxFrameContexts = 16;
> -
>  struct IPASessionConfiguration {
>         struct {
>                 ipu3_uapi_grid_config bdsGrid;
> @@ -79,9 +74,6 @@ struct IPAActiveState {
>  };
>  
>  struct IPAFrameContext : public FrameContext {
> -       IPAFrameContext();
> -       IPAFrameContext(const ControlList &reqControls);
> -
>         struct {
>                 uint32_t exposure;
>                 double gain;
> @@ -94,7 +86,7 @@ struct IPAContext {
>         IPASessionConfiguration configuration;
>         IPAActiveState activeState;
>  
> -       std::array<IPAFrameContext, kMaxFrameContexts> frameContexts;
> +       FCQueue<IPAFrameContext> frameContexts;
>  };
>  
>  } /* namespace ipa::ipu3 */
> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> index b1b23fd8f927..8158ca0883e8 100644
> --- a/src/ipa/ipu3/ipu3.cpp
> +++ b/src/ipa/ipu3/ipu3.cpp
> @@ -40,6 +40,8 @@
>  #include "algorithms/tone_mapping.h"
>  #include "libipa/camera_sensor_helper.h"
>  
> +#include "ipa_context.h"
> +
>  /* Minimum grid width, expressed as a number of cells */
>  static constexpr uint32_t kMinGridWidth = 16;
>  /* Maximum grid width, expressed as a number of cells */
> @@ -53,6 +55,9 @@ 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)
> @@ -135,6 +140,8 @@ namespace ipa::ipu3 {
>  class IPAIPU3 : public IPAIPU3Interface, public Module
>  {
>  public:
> +       IPAIPU3();
> +
>         int init(const IPASettings &settings,
>                  const IPACameraSensorInfo &sensorInfo,
>                  const ControlInfoMap &sensorControls,
> @@ -183,6 +190,11 @@ private:
>         struct IPAContext context_;
>  };
>  
> +IPAIPU3::IPAIPU3()
> +       : context_({ {}, {}, { kMaxFrameContexts } })
> +{
> +}
> +
>  std::string IPAIPU3::logPrefix() const
>  {
>         return "ipu3";
> @@ -205,6 +217,11 @@ void IPAIPU3::updateSessionConfiguration(const ControlInfoMap &sensorControls)
>         int32_t minGain = v4l2Gain.min().get<int32_t>();
>         int32_t maxGain = v4l2Gain.max().get<int32_t>();
>  
> +       /* Clear the IPA context before the streaming session. */
> +       context_.configuration = {};
> +       context_.activeState = {};
> +       context_.frameContexts.clear();

I'm scared about stop / configure / start bugs here ...

( scared or scarred, or both :D )

> +
>         /*
>          * When the AGC computes the new exposure values for a frame, it needs
>          * to know the limits for shutter speed and analogue gain.
> @@ -382,6 +399,7 @@ int IPAIPU3::start()
>   */
>  void IPAIPU3::stop()
>  {
> +       context_.frameContexts.clear();
>  }
>  
>  /**
> @@ -488,11 +506,6 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo,
>  
>         calculateBdsGrid(configInfo.bdsOutputSize);
>  
> -       /* Clean IPAActiveState at each reconfiguration. */
> -       context_.activeState = {};
> -       IPAFrameContext initFrameContext;
> -       context_.frameContexts.fill(initFrameContext);
> -
>         if (!validateSensorControls()) {
>                 LOG(IPAIPU3, Error) << "Sensor control validation failed.";
>                 return -EINVAL;
> @@ -572,7 +585,7 @@ void IPAIPU3::fillParamsBuffer(const uint32_t frame, const uint32_t bufferId)
>          */
>         params->use = {};
>  
> -       IPAFrameContext &frameContext = context_.frameContexts[frame % kMaxFrameContexts];
> +       IPAFrameContext &frameContext = context_.frameContexts.get(frame);
>  
>         for (auto const &algo : algorithms())
>                 algo->prepare(context_, frame, frameContext, params);
> @@ -605,7 +618,7 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame,
>         const ipu3_uapi_stats_3a *stats =
>                 reinterpret_cast<ipu3_uapi_stats_3a *>(mem.data());
>  
> -       IPAFrameContext &frameContext = context_.frameContexts[frame % kMaxFrameContexts];
> +       IPAFrameContext &frameContext = context_.frameContexts.get(frame);
>  
>         frameContext.sensor.exposure = sensorControls.get(V4L2_CID_EXPOSURE).get<int32_t>();
>         frameContext.sensor.gain = camHelper_->gain(sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>());
> @@ -651,7 +664,10 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame,
>  void IPAIPU3::queueRequest(const uint32_t frame, const ControlList &controls)
>  {
>         /* \todo Start processing for 'frame' based on 'controls'. */
> -       context_.frameContexts[frame % kMaxFrameContexts] = { controls };
> +       IPAFrameContext &frameContext = context_.frameContexts.init(frame);
> +
> +       /* \todo Implement queueRequest to each algorithm. */
> +       frameContext.frameControls = controls;

Ok - all looks good enough to build upon...


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

>  }
>  
>  /**
> -- 
> Regards,
> 
> Laurent Pinchart
>