[libcamera-devel,17/22] ipa: ipu3: Use a IPAFrameContext pointer from the per-frame queue
diff mbox series

Message ID 20211108131350.130665-18-jeanmichel.hautbois@ideasonboard.com
State Superseded
Headers show
Series
  • IPA: IPU3: Introduce per-frame controls
Related show

Commit Message

Jean-Michel Hautbois Nov. 8, 2021, 1:13 p.m. UTC 
We have one frame context shared between the algorithms thanks to a
local cached context_ variable in IPAIPU3. Now that we can store the
frame contexts in a queue, implement all the needed functions for that
and convert the frame context to a pointer.

The algorithm are now using the values applied on the frame they are
processing, and not the latest one.

Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
---
 src/ipa/ipu3/algorithms/agc.cpp          | 12 ++++----
 src/ipa/ipu3/algorithms/agc.h            |  2 +-
 src/ipa/ipu3/algorithms/awb.cpp          | 14 ++++-----
 src/ipa/ipu3/algorithms/tone_mapping.cpp |  8 ++---
 src/ipa/ipu3/ipa_context.h               |  2 +-
 src/ipa/ipu3/ipu3.cpp                    | 38 +++++++++++++++++++-----
 6 files changed, 49 insertions(+), 27 deletions(-)

Comments

Jacopo Mondi Nov. 8, 2021, 2:10 p.m. UTC | #1 
Hi Jean-Michel

On Mon, Nov 08, 2021 at 02:13:45PM +0100, Jean-Michel Hautbois wrote:
> We have one frame context shared between the algorithms thanks to a
> local cached context_ variable in IPAIPU3. Now that we can store the
> frame contexts in a queue, implement all the needed functions for that
> and convert the frame context to a pointer.
>
> The algorithm are now using the values applied on the frame they are
> processing, and not the latest one.
>
> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
> ---
>  src/ipa/ipu3/algorithms/agc.cpp          | 12 ++++----
>  src/ipa/ipu3/algorithms/agc.h            |  2 +-
>  src/ipa/ipu3/algorithms/awb.cpp          | 14 ++++-----
>  src/ipa/ipu3/algorithms/tone_mapping.cpp |  8 ++---
>  src/ipa/ipu3/ipa_context.h               |  2 +-
>  src/ipa/ipu3/ipu3.cpp                    | 38 +++++++++++++++++++-----
>  6 files changed, 49 insertions(+), 27 deletions(-)
>
> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp
> index 52cf2753..2eee5b6b 100644
> --- a/src/ipa/ipu3/algorithms/agc.cpp
> +++ b/src/ipa/ipu3/algorithms/agc.cpp
> @@ -245,7 +245,7 @@ void Agc::computeExposure(uint32_t &exposure, double &analogueGain, double curre
>   * \param[in] stats The IPU3 statistics and ISP results
>   * \param[in] currentYGain The gain calculated on the current brightness level
>   */
> -double Agc::computeInitialY(IPAFrameContext &frameContext,
> +double Agc::computeInitialY(IPAFrameContext *frameContext,
>  			    const ipu3_uapi_grid_config &grid,
>  			    const ipu3_uapi_stats_3a *stats,
>  			    double currentYGain)
> @@ -271,9 +271,9 @@ double Agc::computeInitialY(IPAFrameContext &frameContext,
>  	 * Estimate the sum of the brightness values, weighted with the gains
>  	 * applied on the channels in AWB.
>  	 */
> -	double Y_sum = redSum * frameContext.awb.gains.red * .299 +
> -		       greenSum * frameContext.awb.gains.green * .587 +
> -		       blueSum * frameContext.awb.gains.blue * .114;
> +	double Y_sum = redSum * frameContext->awb.gains.red * .299 +
> +		       greenSum * frameContext->awb.gains.green * .587 +
> +		       blueSum * frameContext->awb.gains.blue * .114;
>
>  	/* And return the average brightness */
>  	return Y_sum / (grid.height * grid.width);
> @@ -291,8 +291,8 @@ double Agc::computeInitialY(IPAFrameContext &frameContext,
>  void Agc::process(IPAContext &context, const ipu3_uapi_stats_3a *stats)
>  {
>  	/* Get the latest exposure and gain applied */
> -	uint32_t &exposure = context.frameContext.agc.exposure;
> -	double &analogueGain = context.frameContext.agc.gain;
> +	uint32_t &exposure = context.frameContext->agc.exposure;
> +	double &analogueGain = context.frameContext->agc.gain;
>  	measureBrightness(stats, context.configuration.grid.bdsGrid);
>
>  	double currentYGain = 1.0;
> diff --git a/src/ipa/ipu3/algorithms/agc.h b/src/ipa/ipu3/algorithms/agc.h
> index 6f5d71e0..5d6bef9d 100644
> --- a/src/ipa/ipu3/algorithms/agc.h
> +++ b/src/ipa/ipu3/algorithms/agc.h
> @@ -35,7 +35,7 @@ private:
>  			       const ipu3_uapi_grid_config &grid);
>  	void filterExposure();
>  	void computeExposure(uint32_t &exposure, double &gain, double currentYGain);
> -	double computeInitialY(IPAFrameContext &frameContext,
> +	double computeInitialY(IPAFrameContext *frameContext,
>  			       const ipu3_uapi_grid_config &grid,
>  			       const ipu3_uapi_stats_3a *stats,
>  			       double currentYGain);
> diff --git a/src/ipa/ipu3/algorithms/awb.cpp b/src/ipa/ipu3/algorithms/awb.cpp
> index a4114659..bd55d377 100644
> --- a/src/ipa/ipu3/algorithms/awb.cpp
> +++ b/src/ipa/ipu3/algorithms/awb.cpp
> @@ -382,9 +382,9 @@ void Awb::process(IPAContext &context, const ipu3_uapi_stats_3a *stats)
>  	 * The results are cached, so if no results were calculated, we set the
>  	 * cached values from asyncResults_ here.
>  	 */
> -	context.frameContext.awb.gains.blue = asyncResults_.blueGain;
> -	context.frameContext.awb.gains.green = asyncResults_.greenGain;
> -	context.frameContext.awb.gains.red = asyncResults_.redGain;
> +	context.frameContext->awb.gains.blue = asyncResults_.blueGain;
> +	context.frameContext->awb.gains.green = asyncResults_.greenGain;
> +	context.frameContext->awb.gains.red = asyncResults_.redGain;
>  }
>
>  constexpr uint16_t Awb::threshold(float value)
> @@ -432,10 +432,10 @@ void Awb::prepare(IPAContext &context, ipu3_uapi_params *params)
>  	params->acc_param.bnr.opt_center_sqr.y_sqr_reset = params->acc_param.bnr.opt_center.y_reset
>  							* params->acc_param.bnr.opt_center.y_reset;
>  	/* Convert to u3.13 fixed point values */
> -	params->acc_param.bnr.wb_gains.gr = 8192 * context.frameContext.awb.gains.green;
> -	params->acc_param.bnr.wb_gains.r  = 8192 * context.frameContext.awb.gains.red;
> -	params->acc_param.bnr.wb_gains.b  = 8192 * context.frameContext.awb.gains.blue;
> -	params->acc_param.bnr.wb_gains.gb = 8192 * context.frameContext.awb.gains.green;
> +	params->acc_param.bnr.wb_gains.gr = 8192 * context.frameContext->awb.gains.green;
> +	params->acc_param.bnr.wb_gains.r  = 8192 * context.frameContext->awb.gains.red;
> +	params->acc_param.bnr.wb_gains.b  = 8192 * context.frameContext->awb.gains.blue;
> +	params->acc_param.bnr.wb_gains.gb = 8192 * context.frameContext->awb.gains.green;
>
>  	LOG(IPU3Awb, Debug) << "Color temperature estimated: " << asyncResults_.temperatureK;
>
> diff --git a/src/ipa/ipu3/algorithms/tone_mapping.cpp b/src/ipa/ipu3/algorithms/tone_mapping.cpp
> index 5d74c552..50498f41 100644
> --- a/src/ipa/ipu3/algorithms/tone_mapping.cpp
> +++ b/src/ipa/ipu3/algorithms/tone_mapping.cpp
> @@ -57,7 +57,7 @@ void ToneMapping::prepare([[maybe_unused]] IPAContext &context,
>  {
>  	/* Copy the calculated LUT into the parameters buffer. */
>  	memcpy(params->acc_param.gamma.gc_lut.lut,
> -	       context.frameContext.toneMapping.gammaCorrection.lut,
> +	       context.frameContext->toneMapping.gammaCorrection.lut,
>  	       IPU3_UAPI_GAMMA_CORR_LUT_ENTRIES *
>  	       sizeof(params->acc_param.gamma.gc_lut.lut[0]));
>
> @@ -84,11 +84,11 @@ void ToneMapping::process(IPAContext &context,
>  	 */
>  	gamma_ = 1.1;
>
> -	if (context.frameContext.toneMapping.gamma == gamma_)
> +	if (context.frameContext->toneMapping.gamma == gamma_)
>  		return;
>
>  	struct ipu3_uapi_gamma_corr_lut &lut =
> -		context.frameContext.toneMapping.gammaCorrection;
> +		context.frameContext->toneMapping.gammaCorrection;
>
>  	for (uint32_t i = 0; i < std::size(lut.lut); i++) {
>  		double j = static_cast<double>(i) / (std::size(lut.lut) - 1);
> @@ -98,7 +98,7 @@ void ToneMapping::process(IPAContext &context,
>  		lut.lut[i] = gamma * 8191;
>  	}
>
> -	context.frameContext.toneMapping.gamma = gamma_;
> +	context.frameContext->toneMapping.gamma = gamma_;
>  }
>
>  } /* namespace ipa::ipu3::algorithms */
> diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h
> index ee8f7b55..69780915 100644
> --- a/src/ipa/ipu3/ipa_context.h
> +++ b/src/ipa/ipu3/ipa_context.h
> @@ -57,7 +57,7 @@ struct IPAFrameContext {
>
>  struct IPAContext {
>  	IPASessionConfiguration configuration;
> -	IPAFrameContext frameContext;
> +	IPAFrameContext *frameContext;
>  };
>
>  } /* namespace ipa::ipu3 */
> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> index 177c5c2f..b60ab7e7 100644
> --- a/src/ipa/ipu3/ipu3.cpp
> +++ b/src/ipa/ipu3/ipu3.cpp
> @@ -522,10 +522,26 @@ void IPAIPU3::unmapBuffers(const std::vector<unsigned int> &ids)
>
>  void IPAIPU3::frameStarted([[maybe_unused]] unsigned int frame)
>  {
> +	/*
> +	 * Instantiate a new IPAFrameContext to push to the queue. The lifetime
> +	 * of this pointer is indirectly controlled by the
> +	 * ActionSetSensorControls event, as this is where we will delete it.
> +	 */
> +	struct IPAFrameContext *frameContext = new IPAFrameContext;
> +	frameContext->frameId = frame;
> +	frameContextQueue.push(frameContext);

Maybe a detail:

Wouldn't it be safer to store unique_ptr<IPAFrameContext> in the queue
instead of manually allocate/free ?

>  }
>
>  void IPAIPU3::frameCompleted([[maybe_unused]] unsigned int frame)
>  {
> +	/*
> +	 * Remove the pointer from the queue, it should not be accessed
> +	 * anymore and delete it.
> +	 */
> +	struct IPAFrameContext *frameContext = frameContextQueue.front();
> +	ASSERT(frameContext->frameId == frame);
> +	frameContextQueue.pop();
> +	delete frameContext;
>  }
>
>  /**
> @@ -579,10 +595,6 @@ void IPAIPU3::processEvent(const IPU3Event &event)
>  		const ipu3_uapi_stats_3a *stats =
>  			reinterpret_cast<ipu3_uapi_stats_3a *>(mem.data());
>
> -		/* \todo move those into processControls */
> -		context_.frameContext.agc.exposure = event.sensorControls.get(V4L2_CID_EXPOSURE).get<int32_t>();
> -		context_.frameContext.agc.gain = camHelper_->gain(event.sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>());
> -
>  		parseStatistics(event.frame, event.frameTimestamp, stats);
>  		/*
>  		* To save incurring extra IPC calls, we do not send explicit events
> @@ -610,9 +622,12 @@ void IPAIPU3::processEvent(const IPU3Event &event)
>   */
>  void IPAIPU3::processControls([[maybe_unused]] unsigned int frame,
>  			      [[maybe_unused]] const ControlList &controls,
> -			      [[maybe_unused]] const ControlList &sensorCtrls)
> +			      const ControlList &sensorCtrls)
>  {
> -	/* \todo Start processing for 'frame' based on 'controls'. */
> +	struct IPAFrameContext *frameContext = frameContextQueue.back();
> +	ASSERT(frameContext->frameId == frame);
> +	frameContext->agc.exposure = sensorCtrls.get(V4L2_CID_EXPOSURE).get<int32_t>();
> +	frameContext->agc.gain = camHelper_->gain(sensorCtrls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>());
>  }
>
>  /**
> @@ -636,6 +651,9 @@ void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params)
>  	 */
>  	params->use = {};
>
> +	/* Set the reference to the current frame context */
> +	context_.frameContext = frameContextQueue.front();
> +
>  	for (auto const &algo : algorithms_)
>  		algo->prepare(context_, params);
>
> @@ -661,6 +679,9 @@ void IPAIPU3::parseStatistics(unsigned int frame,
>  {
>  	ControlList ctrls(controls::controls);
>
> +	/* Set the reference to the current frame context */
> +	context_.frameContext = frameContextQueue.front();
> +
>  	for (auto const &algo : algorithms_)
>  		algo->process(context_, stats);
>
> @@ -690,8 +711,9 @@ void IPAIPU3::setControls(unsigned int frame)
>  	IPU3Action op;
>  	op.op = ActionSetSensorControls;
>
> -	exposure_ = context_.frameContext.agc.exposure;
> -	gain_ = camHelper_->gainCode(context_.frameContext.agc.gain);
> +	/* Apply the exposure and gain updated values */
> +	exposure_ = context_.frameContext->agc.exposure;
> +	gain_ = camHelper_->gainCode(context_.frameContext->agc.gain);
>
>  	ControlList ctrls(ctrls_);
>  	ctrls.set(V4L2_CID_EXPOSURE, static_cast<int32_t>(exposure_));
> --
> 2.32.0
>
Kieran Bingham Nov. 8, 2021, 4:09 p.m. UTC | #2 
Quoting Jacopo Mondi (2021-11-08 14:10:43)
> Hi Jean-Michel
> 
> On Mon, Nov 08, 2021 at 02:13:45PM +0100, Jean-Michel Hautbois wrote:
> > We have one frame context shared between the algorithms thanks to a
> > local cached context_ variable in IPAIPU3. Now that we can store the
> > frame contexts in a queue, implement all the needed functions for that
> > and convert the frame context to a pointer.
> >
> > The algorithm are now using the values applied on the frame they are
> > processing, and not the latest one.
> >
> > Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
> > ---
> >  src/ipa/ipu3/algorithms/agc.cpp          | 12 ++++----
> >  src/ipa/ipu3/algorithms/agc.h            |  2 +-
> >  src/ipa/ipu3/algorithms/awb.cpp          | 14 ++++-----
> >  src/ipa/ipu3/algorithms/tone_mapping.cpp |  8 ++---
> >  src/ipa/ipu3/ipa_context.h               |  2 +-
> >  src/ipa/ipu3/ipu3.cpp                    | 38 +++++++++++++++++++-----
> >  6 files changed, 49 insertions(+), 27 deletions(-)
> >
> > diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp
> > index 52cf2753..2eee5b6b 100644
> > --- a/src/ipa/ipu3/algorithms/agc.cpp
> > +++ b/src/ipa/ipu3/algorithms/agc.cpp
> > @@ -245,7 +245,7 @@ void Agc::computeExposure(uint32_t &exposure, double &analogueGain, double curre
> >   * \param[in] stats The IPU3 statistics and ISP results
> >   * \param[in] currentYGain The gain calculated on the current brightness level
> >   */
> > -double Agc::computeInitialY(IPAFrameContext &frameContext,
> > +double Agc::computeInitialY(IPAFrameContext *frameContext,
> >                           const ipu3_uapi_grid_config &grid,
> >                           const ipu3_uapi_stats_3a *stats,
> >                           double currentYGain)
> > @@ -271,9 +271,9 @@ double Agc::computeInitialY(IPAFrameContext &frameContext,
> >        * Estimate the sum of the brightness values, weighted with the gains
> >        * applied on the channels in AWB.
> >        */
> > -     double Y_sum = redSum * frameContext.awb.gains.red * .299 +
> > -                    greenSum * frameContext.awb.gains.green * .587 +
> > -                    blueSum * frameContext.awb.gains.blue * .114;
> > +     double Y_sum = redSum * frameContext->awb.gains.red * .299 +
> > +                    greenSum * frameContext->awb.gains.green * .587 +
> > +                    blueSum * frameContext->awb.gains.blue * .114;
> >
> >       /* And return the average brightness */
> >       return Y_sum / (grid.height * grid.width);
> > @@ -291,8 +291,8 @@ double Agc::computeInitialY(IPAFrameContext &frameContext,
> >  void Agc::process(IPAContext &context, const ipu3_uapi_stats_3a *stats)
> >  {
> >       /* Get the latest exposure and gain applied */
> > -     uint32_t &exposure = context.frameContext.agc.exposure;
> > -     double &analogueGain = context.frameContext.agc.gain;
> > +     uint32_t &exposure = context.frameContext->agc.exposure;
> > +     double &analogueGain = context.frameContext->agc.gain;
> >       measureBrightness(stats, context.configuration.grid.bdsGrid);
> >
> >       double currentYGain = 1.0;
> > diff --git a/src/ipa/ipu3/algorithms/agc.h b/src/ipa/ipu3/algorithms/agc.h
> > index 6f5d71e0..5d6bef9d 100644
> > --- a/src/ipa/ipu3/algorithms/agc.h
> > +++ b/src/ipa/ipu3/algorithms/agc.h
> > @@ -35,7 +35,7 @@ private:
> >                              const ipu3_uapi_grid_config &grid);
> >       void filterExposure();
> >       void computeExposure(uint32_t &exposure, double &gain, double currentYGain);
> > -     double computeInitialY(IPAFrameContext &frameContext,
> > +     double computeInitialY(IPAFrameContext *frameContext,
> >                              const ipu3_uapi_grid_config &grid,
> >                              const ipu3_uapi_stats_3a *stats,
> >                              double currentYGain);
> > diff --git a/src/ipa/ipu3/algorithms/awb.cpp b/src/ipa/ipu3/algorithms/awb.cpp
> > index a4114659..bd55d377 100644
> > --- a/src/ipa/ipu3/algorithms/awb.cpp
> > +++ b/src/ipa/ipu3/algorithms/awb.cpp
> > @@ -382,9 +382,9 @@ void Awb::process(IPAContext &context, const ipu3_uapi_stats_3a *stats)
> >        * The results are cached, so if no results were calculated, we set the
> >        * cached values from asyncResults_ here.
> >        */
> > -     context.frameContext.awb.gains.blue = asyncResults_.blueGain;
> > -     context.frameContext.awb.gains.green = asyncResults_.greenGain;
> > -     context.frameContext.awb.gains.red = asyncResults_.redGain;
> > +     context.frameContext->awb.gains.blue = asyncResults_.blueGain;
> > +     context.frameContext->awb.gains.green = asyncResults_.greenGain;
> > +     context.frameContext->awb.gains.red = asyncResults_.redGain;
> >  }
> >
> >  constexpr uint16_t Awb::threshold(float value)
> > @@ -432,10 +432,10 @@ void Awb::prepare(IPAContext &context, ipu3_uapi_params *params)
> >       params->acc_param.bnr.opt_center_sqr.y_sqr_reset = params->acc_param.bnr.opt_center.y_reset
> >                                                       * params->acc_param.bnr.opt_center.y_reset;
> >       /* Convert to u3.13 fixed point values */
> > -     params->acc_param.bnr.wb_gains.gr = 8192 * context.frameContext.awb.gains.green;
> > -     params->acc_param.bnr.wb_gains.r  = 8192 * context.frameContext.awb.gains.red;
> > -     params->acc_param.bnr.wb_gains.b  = 8192 * context.frameContext.awb.gains.blue;
> > -     params->acc_param.bnr.wb_gains.gb = 8192 * context.frameContext.awb.gains.green;
> > +     params->acc_param.bnr.wb_gains.gr = 8192 * context.frameContext->awb.gains.green;
> > +     params->acc_param.bnr.wb_gains.r  = 8192 * context.frameContext->awb.gains.red;
> > +     params->acc_param.bnr.wb_gains.b  = 8192 * context.frameContext->awb.gains.blue;
> > +     params->acc_param.bnr.wb_gains.gb = 8192 * context.frameContext->awb.gains.green;
> >
> >       LOG(IPU3Awb, Debug) << "Color temperature estimated: " << asyncResults_.temperatureK;
> >
> > diff --git a/src/ipa/ipu3/algorithms/tone_mapping.cpp b/src/ipa/ipu3/algorithms/tone_mapping.cpp
> > index 5d74c552..50498f41 100644
> > --- a/src/ipa/ipu3/algorithms/tone_mapping.cpp
> > +++ b/src/ipa/ipu3/algorithms/tone_mapping.cpp
> > @@ -57,7 +57,7 @@ void ToneMapping::prepare([[maybe_unused]] IPAContext &context,
> >  {
> >       /* Copy the calculated LUT into the parameters buffer. */
> >       memcpy(params->acc_param.gamma.gc_lut.lut,
> > -            context.frameContext.toneMapping.gammaCorrection.lut,
> > +            context.frameContext->toneMapping.gammaCorrection.lut,
> >              IPU3_UAPI_GAMMA_CORR_LUT_ENTRIES *
> >              sizeof(params->acc_param.gamma.gc_lut.lut[0]));
> >
> > @@ -84,11 +84,11 @@ void ToneMapping::process(IPAContext &context,
> >        */
> >       gamma_ = 1.1;
> >
> > -     if (context.frameContext.toneMapping.gamma == gamma_)
> > +     if (context.frameContext->toneMapping.gamma == gamma_)
> >               return;
> >
> >       struct ipu3_uapi_gamma_corr_lut &lut =
> > -             context.frameContext.toneMapping.gammaCorrection;
> > +             context.frameContext->toneMapping.gammaCorrection;
> >
> >       for (uint32_t i = 0; i < std::size(lut.lut); i++) {
> >               double j = static_cast<double>(i) / (std::size(lut.lut) - 1);
> > @@ -98,7 +98,7 @@ void ToneMapping::process(IPAContext &context,
> >               lut.lut[i] = gamma * 8191;
> >       }
> >
> > -     context.frameContext.toneMapping.gamma = gamma_;
> > +     context.frameContext->toneMapping.gamma = gamma_;
> >  }
> >
> >  } /* namespace ipa::ipu3::algorithms */
> > diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h
> > index ee8f7b55..69780915 100644
> > --- a/src/ipa/ipu3/ipa_context.h
> > +++ b/src/ipa/ipu3/ipa_context.h
> > @@ -57,7 +57,7 @@ struct IPAFrameContext {
> >
> >  struct IPAContext {
> >       IPASessionConfiguration configuration;
> > -     IPAFrameContext frameContext;
> > +     IPAFrameContext *frameContext;
> >  };
> >
> >  } /* namespace ipa::ipu3 */
> > diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> > index 177c5c2f..b60ab7e7 100644
> > --- a/src/ipa/ipu3/ipu3.cpp
> > +++ b/src/ipa/ipu3/ipu3.cpp
> > @@ -522,10 +522,26 @@ void IPAIPU3::unmapBuffers(const std::vector<unsigned int> &ids)
> >
> >  void IPAIPU3::frameStarted([[maybe_unused]] unsigned int frame)
> >  {
> > +     /*
> > +      * Instantiate a new IPAFrameContext to push to the queue. The lifetime
> > +      * of this pointer is indirectly controlled by the
> > +      * ActionSetSensorControls event, as this is where we will delete it.
> > +      */
> > +     struct IPAFrameContext *frameContext = new IPAFrameContext;
> > +     frameContext->frameId = frame;
> > +     frameContextQueue.push(frameContext);
> 
> Maybe a detail:
> 
> Wouldn't it be safer to store unique_ptr<IPAFrameContext> in the queue
> instead of manually allocate/free ?

I'm curious if we should even be storing pointers at all, and not put a
full IPAFrameContext on the queue instead.

Reading https://en.cppreference.com/w/cpp/container/deque (which is the
default queue implementation I believe) sounds like the implementation
will be able to reuse existing allocations then, rather than performing
a new/delete for every IPAFrameContext instance.

In essence, I think it can be a cheap-ish scalable ring buffer, which is
what we actually want here.

I think "[libcamera-devel] [PATCH 14/22] ipa: ipu3: Introduce a queue of
IPAFrameContext" should be squashed into the content of this patch
though. There's no point separating the implementation of the queue from
the addition of it to the IPU3. I think it's clearer together.


> >  }
> >
> >  void IPAIPU3::frameCompleted([[maybe_unused]] unsigned int frame)
> >  {
> > +     /*
> > +      * Remove the pointer from the queue, it should not be accessed
> > +      * anymore and delete it.
> > +      */
> > +     struct IPAFrameContext *frameContext = frameContextQueue.front();

Although - It's worth checking to see if .front() gives a reference...

https://www.cplusplus.com/reference/deque/deque/front/

.front() returns a reference, so we can put the whole IPAFrameContext on
there without making copies at points like this. (Had to check, as I was
concerned if this would be a reason to not put the whole structure on
the queue).




> > +     ASSERT(frameContext->frameId == frame);
> > +     frameContextQueue.pop();
> > +     delete frameContext;
> >  }
> >
> >  /**
> > @@ -579,10 +595,6 @@ void IPAIPU3::processEvent(const IPU3Event &event)
> >               const ipu3_uapi_stats_3a *stats =
> >                       reinterpret_cast<ipu3_uapi_stats_3a *>(mem.data());
> >
> > -             /* \todo move those into processControls */

Ok, I see why there was a todo now.

> > -             context_.frameContext.agc.exposure = event.sensorControls.get(V4L2_CID_EXPOSURE).get<int32_t>();
> > -             context_.frameContext.agc.gain = camHelper_->gain(event.sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>());
> > -
> >               parseStatistics(event.frame, event.frameTimestamp, stats);
> >               /*
> >               * To save incurring extra IPC calls, we do not send explicit events
> > @@ -610,9 +622,12 @@ void IPAIPU3::processEvent(const IPU3Event &event)
> >   */
> >  void IPAIPU3::processControls([[maybe_unused]] unsigned int frame,
> >                             [[maybe_unused]] const ControlList &controls,
> > -                           [[maybe_unused]] const ControlList &sensorCtrls)
> > +                           const ControlList &sensorCtrls)
> >  {
> > -     /* \todo Start processing for 'frame' based on 'controls'. */
> > +     struct IPAFrameContext *frameContext = frameContextQueue.back();
> > +     ASSERT(frameContext->frameId == frame);
> > +     frameContext->agc.exposure = sensorCtrls.get(V4L2_CID_EXPOSURE).get<int32_t>();
> > +     frameContext->agc.gain = camHelper_->gain(sensorCtrls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>());

I'm ... still concerned about how a sequencing diagram would show the
usage of these values.

ProcessControls is the 'first' event that occurs. I don't think I see
how the IPA should consider the exposure and gain of the sensor at that
point.

The IPA should be in charge of 'setting' the exposure and gain on frames
as they go out, and I think it should only care about what values were
set on the frame associated with the statistics that it processes.

> >  }
> >
> >  /**
> > @@ -636,6 +651,9 @@ void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params)
> >        */
> >       params->use = {};
> >
> > +     /* Set the reference to the current frame context */
> > +     context_.frameContext = frameContextQueue.front();
> > +
> >       for (auto const &algo : algorithms_)
> >               algo->prepare(context_, params);
> >
> > @@ -661,6 +679,9 @@ void IPAIPU3::parseStatistics(unsigned int frame,
> >  {
> >       ControlList ctrls(controls::controls);
> >
> > +     /* Set the reference to the current frame context */
> > +     context_.frameContext = frameContextQueue.front();
> > +

Seeing this set the reference in both fillParams and parseStatistics
feels risky/racy. It might not be right now - but it feels like it could
be in the future when those two events might have potential to happen in
parallel. Or maybe we would prevent that from happening.



> >       for (auto const &algo : algorithms_)
> >               algo->process(context_, stats);
> >
> > @@ -690,8 +711,9 @@ void IPAIPU3::setControls(unsigned int frame)
> >       IPU3Action op;
> >       op.op = ActionSetSensorControls;
> >
> > -     exposure_ = context_.frameContext.agc.exposure;
> > -     gain_ = camHelper_->gainCode(context_.frameContext.agc.gain);
> > +     /* Apply the exposure and gain updated values */
> > +     exposure_ = context_.frameContext->agc.exposure;
> > +     gain_ = camHelper_->gainCode(context_.frameContext->agc.gain);

If exposure_ and gain_ get set immediately here, do they need to be
stored as class variables? Can they be local? or do they get used
somewhere else? (I'd expect any other usages to be specific to a
FrameContext reference...)

> >
> >       ControlList ctrls(ctrls_);
> >       ctrls.set(V4L2_CID_EXPOSURE, static_cast<int32_t>(exposure_));
> > --
> > 2.32.0
> >

Patch
diff mbox series 

diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp
index 52cf2753..2eee5b6b 100644
--- a/src/ipa/ipu3/algorithms/agc.cpp
+++ b/src/ipa/ipu3/algorithms/agc.cpp
@@ -245,7 +245,7 @@  void Agc::computeExposure(uint32_t &exposure, double &analogueGain, double curre
  * \param[in] stats The IPU3 statistics and ISP results
  * \param[in] currentYGain The gain calculated on the current brightness level
  */
-double Agc::computeInitialY(IPAFrameContext &frameContext,
+double Agc::computeInitialY(IPAFrameContext *frameContext,
 			    const ipu3_uapi_grid_config &grid,
 			    const ipu3_uapi_stats_3a *stats,
 			    double currentYGain)
@@ -271,9 +271,9 @@  double Agc::computeInitialY(IPAFrameContext &frameContext,
 	 * Estimate the sum of the brightness values, weighted with the gains
 	 * applied on the channels in AWB.
 	 */
-	double Y_sum = redSum * frameContext.awb.gains.red * .299 +
-		       greenSum * frameContext.awb.gains.green * .587 +
-		       blueSum * frameContext.awb.gains.blue * .114;
+	double Y_sum = redSum * frameContext->awb.gains.red * .299 +
+		       greenSum * frameContext->awb.gains.green * .587 +
+		       blueSum * frameContext->awb.gains.blue * .114;
 
 	/* And return the average brightness */
 	return Y_sum / (grid.height * grid.width);
@@ -291,8 +291,8 @@  double Agc::computeInitialY(IPAFrameContext &frameContext,
 void Agc::process(IPAContext &context, const ipu3_uapi_stats_3a *stats)
 {
 	/* Get the latest exposure and gain applied */
-	uint32_t &exposure = context.frameContext.agc.exposure;
-	double &analogueGain = context.frameContext.agc.gain;
+	uint32_t &exposure = context.frameContext->agc.exposure;
+	double &analogueGain = context.frameContext->agc.gain;
 	measureBrightness(stats, context.configuration.grid.bdsGrid);
 
 	double currentYGain = 1.0;
diff --git a/src/ipa/ipu3/algorithms/agc.h b/src/ipa/ipu3/algorithms/agc.h
index 6f5d71e0..5d6bef9d 100644
--- a/src/ipa/ipu3/algorithms/agc.h
+++ b/src/ipa/ipu3/algorithms/agc.h
@@ -35,7 +35,7 @@  private:
 			       const ipu3_uapi_grid_config &grid);
 	void filterExposure();
 	void computeExposure(uint32_t &exposure, double &gain, double currentYGain);
-	double computeInitialY(IPAFrameContext &frameContext,
+	double computeInitialY(IPAFrameContext *frameContext,
 			       const ipu3_uapi_grid_config &grid,
 			       const ipu3_uapi_stats_3a *stats,
 			       double currentYGain);
diff --git a/src/ipa/ipu3/algorithms/awb.cpp b/src/ipa/ipu3/algorithms/awb.cpp
index a4114659..bd55d377 100644
--- a/src/ipa/ipu3/algorithms/awb.cpp
+++ b/src/ipa/ipu3/algorithms/awb.cpp
@@ -382,9 +382,9 @@  void Awb::process(IPAContext &context, const ipu3_uapi_stats_3a *stats)
 	 * The results are cached, so if no results were calculated, we set the
 	 * cached values from asyncResults_ here.
 	 */
-	context.frameContext.awb.gains.blue = asyncResults_.blueGain;
-	context.frameContext.awb.gains.green = asyncResults_.greenGain;
-	context.frameContext.awb.gains.red = asyncResults_.redGain;
+	context.frameContext->awb.gains.blue = asyncResults_.blueGain;
+	context.frameContext->awb.gains.green = asyncResults_.greenGain;
+	context.frameContext->awb.gains.red = asyncResults_.redGain;
 }
 
 constexpr uint16_t Awb::threshold(float value)
@@ -432,10 +432,10 @@  void Awb::prepare(IPAContext &context, ipu3_uapi_params *params)
 	params->acc_param.bnr.opt_center_sqr.y_sqr_reset = params->acc_param.bnr.opt_center.y_reset
 							* params->acc_param.bnr.opt_center.y_reset;
 	/* Convert to u3.13 fixed point values */
-	params->acc_param.bnr.wb_gains.gr = 8192 * context.frameContext.awb.gains.green;
-	params->acc_param.bnr.wb_gains.r  = 8192 * context.frameContext.awb.gains.red;
-	params->acc_param.bnr.wb_gains.b  = 8192 * context.frameContext.awb.gains.blue;
-	params->acc_param.bnr.wb_gains.gb = 8192 * context.frameContext.awb.gains.green;
+	params->acc_param.bnr.wb_gains.gr = 8192 * context.frameContext->awb.gains.green;
+	params->acc_param.bnr.wb_gains.r  = 8192 * context.frameContext->awb.gains.red;
+	params->acc_param.bnr.wb_gains.b  = 8192 * context.frameContext->awb.gains.blue;
+	params->acc_param.bnr.wb_gains.gb = 8192 * context.frameContext->awb.gains.green;
 
 	LOG(IPU3Awb, Debug) << "Color temperature estimated: " << asyncResults_.temperatureK;
 
diff --git a/src/ipa/ipu3/algorithms/tone_mapping.cpp b/src/ipa/ipu3/algorithms/tone_mapping.cpp
index 5d74c552..50498f41 100644
--- a/src/ipa/ipu3/algorithms/tone_mapping.cpp
+++ b/src/ipa/ipu3/algorithms/tone_mapping.cpp
@@ -57,7 +57,7 @@  void ToneMapping::prepare([[maybe_unused]] IPAContext &context,
 {
 	/* Copy the calculated LUT into the parameters buffer. */
 	memcpy(params->acc_param.gamma.gc_lut.lut,
-	       context.frameContext.toneMapping.gammaCorrection.lut,
+	       context.frameContext->toneMapping.gammaCorrection.lut,
 	       IPU3_UAPI_GAMMA_CORR_LUT_ENTRIES *
 	       sizeof(params->acc_param.gamma.gc_lut.lut[0]));
 
@@ -84,11 +84,11 @@  void ToneMapping::process(IPAContext &context,
 	 */
 	gamma_ = 1.1;
 
-	if (context.frameContext.toneMapping.gamma == gamma_)
+	if (context.frameContext->toneMapping.gamma == gamma_)
 		return;
 
 	struct ipu3_uapi_gamma_corr_lut &lut =
-		context.frameContext.toneMapping.gammaCorrection;
+		context.frameContext->toneMapping.gammaCorrection;
 
 	for (uint32_t i = 0; i < std::size(lut.lut); i++) {
 		double j = static_cast<double>(i) / (std::size(lut.lut) - 1);
@@ -98,7 +98,7 @@  void ToneMapping::process(IPAContext &context,
 		lut.lut[i] = gamma * 8191;
 	}
 
-	context.frameContext.toneMapping.gamma = gamma_;
+	context.frameContext->toneMapping.gamma = gamma_;
 }
 
 } /* namespace ipa::ipu3::algorithms */
diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h
index ee8f7b55..69780915 100644
--- a/src/ipa/ipu3/ipa_context.h
+++ b/src/ipa/ipu3/ipa_context.h
@@ -57,7 +57,7 @@  struct IPAFrameContext {
 
 struct IPAContext {
 	IPASessionConfiguration configuration;
-	IPAFrameContext frameContext;
+	IPAFrameContext *frameContext;
 };
 
 } /* namespace ipa::ipu3 */
diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
index 177c5c2f..b60ab7e7 100644
--- a/src/ipa/ipu3/ipu3.cpp
+++ b/src/ipa/ipu3/ipu3.cpp
@@ -522,10 +522,26 @@  void IPAIPU3::unmapBuffers(const std::vector<unsigned int> &ids)
 
 void IPAIPU3::frameStarted([[maybe_unused]] unsigned int frame)
 {
+	/*
+	 * Instantiate a new IPAFrameContext to push to the queue. The lifetime
+	 * of this pointer is indirectly controlled by the
+	 * ActionSetSensorControls event, as this is where we will delete it.
+	 */
+	struct IPAFrameContext *frameContext = new IPAFrameContext;
+	frameContext->frameId = frame;
+	frameContextQueue.push(frameContext);
 }
 
 void IPAIPU3::frameCompleted([[maybe_unused]] unsigned int frame)
 {
+	/*
+	 * Remove the pointer from the queue, it should not be accessed
+	 * anymore and delete it.
+	 */
+	struct IPAFrameContext *frameContext = frameContextQueue.front();
+	ASSERT(frameContext->frameId == frame);
+	frameContextQueue.pop();
+	delete frameContext;
 }
 
 /**
@@ -579,10 +595,6 @@  void IPAIPU3::processEvent(const IPU3Event &event)
 		const ipu3_uapi_stats_3a *stats =
 			reinterpret_cast<ipu3_uapi_stats_3a *>(mem.data());
 
-		/* \todo move those into processControls */
-		context_.frameContext.agc.exposure = event.sensorControls.get(V4L2_CID_EXPOSURE).get<int32_t>();
-		context_.frameContext.agc.gain = camHelper_->gain(event.sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>());
-
 		parseStatistics(event.frame, event.frameTimestamp, stats);
 		/*
 		* To save incurring extra IPC calls, we do not send explicit events
@@ -610,9 +622,12 @@  void IPAIPU3::processEvent(const IPU3Event &event)
  */
 void IPAIPU3::processControls([[maybe_unused]] unsigned int frame,
 			      [[maybe_unused]] const ControlList &controls,
-			      [[maybe_unused]] const ControlList &sensorCtrls)
+			      const ControlList &sensorCtrls)
 {
-	/* \todo Start processing for 'frame' based on 'controls'. */
+	struct IPAFrameContext *frameContext = frameContextQueue.back();
+	ASSERT(frameContext->frameId == frame);
+	frameContext->agc.exposure = sensorCtrls.get(V4L2_CID_EXPOSURE).get<int32_t>();
+	frameContext->agc.gain = camHelper_->gain(sensorCtrls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>());
 }
 
 /**
@@ -636,6 +651,9 @@  void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params)
 	 */
 	params->use = {};
 
+	/* Set the reference to the current frame context */
+	context_.frameContext = frameContextQueue.front();
+
 	for (auto const &algo : algorithms_)
 		algo->prepare(context_, params);
 
@@ -661,6 +679,9 @@  void IPAIPU3::parseStatistics(unsigned int frame,
 {
 	ControlList ctrls(controls::controls);
 
+	/* Set the reference to the current frame context */
+	context_.frameContext = frameContextQueue.front();
+
 	for (auto const &algo : algorithms_)
 		algo->process(context_, stats);
 
@@ -690,8 +711,9 @@  void IPAIPU3::setControls(unsigned int frame)
 	IPU3Action op;
 	op.op = ActionSetSensorControls;
 
-	exposure_ = context_.frameContext.agc.exposure;
-	gain_ = camHelper_->gainCode(context_.frameContext.agc.gain);
+	/* Apply the exposure and gain updated values */
+	exposure_ = context_.frameContext->agc.exposure;
+	gain_ = camHelper_->gainCode(context_.frameContext->agc.gain);
 
 	ControlList ctrls(ctrls_);
 	ctrls.set(V4L2_CID_EXPOSURE, static_cast<int32_t>(exposure_));