[libcamera-devel,v4,10/32] ipa: ipu3: Use the FCQueue
diff mbox series

Message ID 20220908014200.28728-11-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 
Replace the manual ring buffer implementation with the FCQueue class
from libipa.

Signed-off-by: Laurent Pinchart <laurent.pinchart@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(-)

Comments

Kieran Bingham Sept. 20, 2022, 2:28 p.m. UTC | #1 
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@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@ideasonboard.com>

>  }
>  
>  /**
> -- 
> Regards,
> 
> Laurent Pinchart
>
Laurent Pinchart Sept. 20, 2022, 7:43 p.m. UTC | #2 
On Tue, Sep 20, 2022 at 03:28:44PM +0100, Kieran Bingham wrote:
> 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@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 ...

The queue is cleared on stop(), so that part should be fine. The
configuration isn't meant to be modified at runtime, so it should be
fine too. The active state may be problematic, but that's not introduced
by this patch, it was previously cleared in IPAIPU3::configure() and
only moved to IPAIPU3::updateSessionConfiguration() 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@ideasonboard.com>
> 
> >  }
> >  
> >  /**
Jacopo Mondi Sept. 21, 2022, 6:13 p.m. UTC | #3 
This answers most of my previous questions...

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

Thanks
  j

On Thu, Sep 08, 2022 at 04:41:38AM +0300, Laurent Pinchart via libcamera-devel wrote:
> Replace the manual ring buffer implementation with the FCQueue class
> from libipa.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@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();
> +
>  	/*
>  	 * 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;
>  }
>
>  /**
> --
> Regards,
>
> Laurent Pinchart
>

Patch
diff mbox series 

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();
+
 	/*
 	 * 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;
 }
 
 /**