[libcamera-devel,v2,3/3] ipa: ipu3: Introduce a IPAFrameContext queue

15810 diff mbox series

Introduce a queue of IPAFrameContext which shall maintain the data
required to track each frame. Currently, it is storing only the
sensor controls values applied for the frame but new structures
can be introduced in IPAFrameContext to extend any frame-related
contexts (either in-processing or new frames being queued).

For example, this patch provides the foundation for the extension
of IPAFrameContext to store incoming request controls.

Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
---
 src/ipa/ipu3/algorithms/agc.cpp |  2 +-
 src/ipa/ipu3/ipa_context.cpp    | 16 ++++++++++++++--
 src/ipa/ipu3/ipa_context.h      |  8 +++++++-
 src/ipa/ipu3/ipu3.cpp           | 31 ++++++++++++++++++++++++++-----
 4 files changed, 48 insertions(+), 9 deletions(-)

Quoting Umang Jain via libcamera-devel (2022-05-06 10:53:07)
> Introduce a queue of IPAFrameContext which shall maintain the data
> required to track each frame. Currently, it is storing only the
> sensor controls values applied for the frame but new structures
> can be introduced in IPAFrameContext to extend any frame-related
> contexts (either in-processing or new frames being queued).

Technically I'm not sure those even need to be stored in the
FrameContext as they get set and then used in the same function I think.

But ... I think it's fine for now, even as an example for the data - and
what will be more relevant is the controls that get set per-frame, and
anything that needs to be tracked to adapt to those.

However that said, if we rework DelayedControls - then being able to
associate the expected Gain/Exposures to what was set is probably
useful to debug and check the implementation.


 
> For example, this patch provides the foundation for the extension
> of IPAFrameContext to store incoming request controls.
> 
> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> ---
>  src/ipa/ipu3/algorithms/agc.cpp |  2 +-
>  src/ipa/ipu3/ipa_context.cpp    | 16 ++++++++++++++--
>  src/ipa/ipu3/ipa_context.h      |  8 +++++++-
>  src/ipa/ipu3/ipu3.cpp           | 31 ++++++++++++++++++++++++++-----
>  4 files changed, 48 insertions(+), 9 deletions(-)
> 
> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp
> index fdeec09d..4784af00 100644
> --- a/src/ipa/ipu3/algorithms/agc.cpp
> +++ b/src/ipa/ipu3/algorithms/agc.cpp
> @@ -187,7 +187,7 @@ void Agc::computeExposure(IPAContext &context, double yGain,
>                           double iqMeanGain)
>  {
>         const IPASessionConfiguration &configuration = context.configuration;
> -       IPAFrameContext &frameContext = context.frameContext;
> +       IPAFrameContext &frameContext = context.frameContextQueue.front();
>         /* Get the effective exposure and gain applied on the sensor. */
>         uint32_t exposure = frameContext.sensor.exposure;
>         double analogueGain = frameContext.sensor.gain;
> diff --git a/src/ipa/ipu3/ipa_context.cpp b/src/ipa/ipu3/ipa_context.cpp
> index 06eb2776..fb48bc9b 100644
> --- a/src/ipa/ipu3/ipa_context.cpp
> +++ b/src/ipa/ipu3/ipa_context.cpp
> @@ -58,8 +58,8 @@ namespace libcamera::ipa::ipu3 {
>   * \var IPAContext::configuration
>   * \brief The IPA session configuration, immutable during the session
>   *
> - * \var IPAContext::frameContext
> - * \brief The frame context for the frame being processed
> + * \var IPAContext::frameContextQueue
> + * \brief FIFO container of IPAFrameContext for all the frames being queued
>   *
>   * \var IPAContext::activeState
>   * \brief The current state of IPA algorithms
> @@ -182,6 +182,15 @@ namespace libcamera::ipa::ipu3 {
>   * <linux/intel-ipu3.h> struct ipu3_uapi_gamma_corr_lut for further details.
>   */
>  
> +/**
> + * \brief Construct a IPAFrameContext instance
> + */
> +IPAFrameContext::IPAFrameContext(uint32_t frame)
> +       : frame(frame)
> +{
> +       sensor = {};
> +}
> +
>  /**
>   * \var IPAFrameContext::sensor
>   * \brief Effective sensor values that were applied for the frame
> @@ -191,6 +200,9 @@ namespace libcamera::ipa::ipu3 {
>   *
>   * \var IPAFrameContext::sensor.gain
>   * \brief Analogue gain multiplier
> + *
> + * \var IPAFrameContext::frame
> + * \brief The frame number associated with this IPAFrameContext
>   */
>  
>  } /* namespace libcamera::ipa::ipu3 */
> diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h
> index 8d681131..20cccc97 100644
> --- a/src/ipa/ipu3/ipa_context.h
> +++ b/src/ipa/ipu3/ipa_context.h
> @@ -8,6 +8,8 @@
>  
>  #pragma once
>  
> +#include <queue>
> +
>  #include <linux/intel-ipu3.h>
>  
>  #include <libcamera/base/utils.h>
> @@ -71,17 +73,21 @@ struct IPAActiveState {
>  };
>  
>  struct IPAFrameContext {
> +       IPAFrameContext(uint32_t frame);
> +
>         struct {
>                 uint32_t exposure;
>                 double gain;
>         } sensor;
> +
> +       uint32_t frame;
>  };
>  
>  struct IPAContext {
>         IPASessionConfiguration configuration;
>         IPAActiveState activeState;
>  
> -       IPAFrameContext frameContext;
> +       std::queue<IPAFrameContext> frameContextQueue;
>  };
>  
>  } /* namespace ipa::ipu3 */
> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> index 268c8f61..1b566f14 100644
> --- a/src/ipa/ipu3/ipu3.cpp
> +++ b/src/ipa/ipu3/ipu3.cpp
> @@ -350,6 +350,8 @@ int IPAIPU3::start()
>   */
>  void IPAIPU3::stop()
>  {
> +       while (!context_.frameContextQueue.empty())
> +               context_.frameContextQueue.pop();

Does context_.frameContextQueue.clear(); work here?


>  }
>  
>  /**
> @@ -458,7 +460,7 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo,
>  
>         /* Clean IPAActiveState at each reconfiguration. */
>         context_.activeState = {};
> -       context_.frameContext = {};
> +       context_.frameContextQueue = {};
>  
>         if (!validateSensorControls()) {
>                 LOG(IPAIPU3, Error) << "Sensor control validation failed.";
> @@ -510,6 +512,13 @@ void IPAIPU3::unmapBuffers(const std::vector<unsigned int> &ids)
>  
>  void IPAIPU3::frameCompleted([[maybe_unused]] const uint32_t frame)

I suspect patch 1/3 could be squashed with this patch ... There's not a
lot of distinction on the change - but I don't mind the split otherwise.


>  {
> +       while (!context_.frameContextQueue.empty()) {
> +               auto &fc = context_.frameContextQueue.front();

I'm almost tempted to say we should have a warning/debug print in here
if (fc.frame < frame). If it's the current frame (== frame) it's
expected, but if it's earlier - then that's where some sort of frame
drop has occurred - and we need to make sure that any request is still
handled correctly if we're going to associated requests that get
returned to notify a dropped frame.

But I expect any frame drop handling would have already been handled
before this frameCompleted gets called.

> +               if (fc.frame <= frame)
> +                       context_.frameContextQueue.pop();
> +               else
> +                       break;
> +       }
>  }
>  
>  /**
> @@ -574,8 +583,18 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame,
>         const ipu3_uapi_stats_3a *stats =
>                 reinterpret_cast<ipu3_uapi_stats_3a *>(mem.data());
>  
> -       context_.frameContext.sensor.exposure = sensorControls.get(V4L2_CID_EXPOSURE).get<int32_t>();
> -       context_.frameContext.sensor.gain = camHelper_->gain(sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>());
> +       auto &frameContext = context_.frameContextQueue.front();
> +
> +       /*
> +        * An assert might be too harsh here. We want to know the cases
> +        * where the front of the queue (implies the current frame in processing,
> +        * diverges from the frame parameter of this function
> +        * \todo Identify those cases - e.g. frame drop?
> +        */
> +       ASSERT(context_.frameContextQueue.front().frame == frame);

This is certainly too hard I think. It would almost certainly cause
libcamera to close on a frame drop (which could be very easy to cause to
occur). So I don't think I'd like to merge this line.


> +
> +       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>());
>  
>         double lineDuration = context_.configuration.sensor.lineDuration.get<std::micro>();
>         int32_t vBlank = context_.configuration.sensor.defVBlank;
> @@ -590,11 +609,11 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame,
>         int64_t frameDuration = (vBlank + sensorInfo_.outputSize.height) * lineDuration;
>         ctrls.set(controls::FrameDuration, frameDuration);
>  
> -       ctrls.set(controls::AnalogueGain, context_.frameContext.sensor.gain);
> +       ctrls.set(controls::AnalogueGain, frameContext.sensor.gain);
>  
>         ctrls.set(controls::ColourTemperature, context_.activeState.awb.temperatureK);
>  
> -       ctrls.set(controls::ExposureTime, context_.frameContext.sensor.exposure * lineDuration);
> +       ctrls.set(controls::ExposureTime, frameContext.sensor.exposure * lineDuration);
>  
>         /*
>          * \todo The Metadata provides a path to getting extended data
> @@ -621,6 +640,8 @@ void IPAIPU3::queueRequest([[maybe_unused]] const uint32_t frame,
>                            [[maybe_unused]] const ControlList &controls)
>  {
>         /* \todo Start processing for 'frame' based on 'controls'. */
> +
> +       context_.frameContextQueue.emplace(frame);

But I think this is good - from here we can start parsing the request
and controls and start doing something about request controls.


These patches are more or less ok to go in on their own I think (except
that ASSERT which is far too hard) - but I'd be really keen to start
seeing how we handle controls here soon.

--
Kieran


>  }
>  
>  /**
> -- 
> 2.31.1
>

Hi Kieran,

On 5/6/22 17:22, Kieran Bingham wrote:
> Quoting Umang Jain via libcamera-devel (2022-05-06 10:53:07)
>> Introduce a queue of IPAFrameContext which shall maintain the data
>> required to track each frame. Currently, it is storing only the
>> sensor controls values applied for the frame but new structures
>> can be introduced in IPAFrameContext to extend any frame-related
>> contexts (either in-processing or new frames being queued).
> Technically I'm not sure those even need to be stored in the
> FrameContext as they get set and then used in the same function I think.
>
> But ... I think it's fine for now, even as an example for the data - and
> what will be more relevant is the controls that get set per-frame, and
> anything that needs to be tracked to adapt to those.
>
> However that said, if we rework DelayedControls - then being able to
> associate the expected Gain/Exposures to what was set is probably
> useful to debug and check the implementation.


I have a few ideas here, let me prepare a prototype branch and share it 
with you.

>
>
>   
>> For example, this patch provides the foundation for the extension
>> of IPAFrameContext to store incoming request controls.
>>
>> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
>> ---
>>   src/ipa/ipu3/algorithms/agc.cpp |  2 +-
>>   src/ipa/ipu3/ipa_context.cpp    | 16 ++++++++++++++--
>>   src/ipa/ipu3/ipa_context.h      |  8 +++++++-
>>   src/ipa/ipu3/ipu3.cpp           | 31 ++++++++++++++++++++++++++-----
>>   4 files changed, 48 insertions(+), 9 deletions(-)
>>
>> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp
>> index fdeec09d..4784af00 100644
>> --- a/src/ipa/ipu3/algorithms/agc.cpp
>> +++ b/src/ipa/ipu3/algorithms/agc.cpp
>> @@ -187,7 +187,7 @@ void Agc::computeExposure(IPAContext &context, double yGain,
>>                            double iqMeanGain)
>>   {
>>          const IPASessionConfiguration &configuration = context.configuration;
>> -       IPAFrameContext &frameContext = context.frameContext;
>> +       IPAFrameContext &frameContext = context.frameContextQueue.front();
>>          /* Get the effective exposure and gain applied on the sensor. */
>>          uint32_t exposure = frameContext.sensor.exposure;
>>          double analogueGain = frameContext.sensor.gain;
>> diff --git a/src/ipa/ipu3/ipa_context.cpp b/src/ipa/ipu3/ipa_context.cpp
>> index 06eb2776..fb48bc9b 100644
>> --- a/src/ipa/ipu3/ipa_context.cpp
>> +++ b/src/ipa/ipu3/ipa_context.cpp
>> @@ -58,8 +58,8 @@ namespace libcamera::ipa::ipu3 {
>>    * \var IPAContext::configuration
>>    * \brief The IPA session configuration, immutable during the session
>>    *
>> - * \var IPAContext::frameContext
>> - * \brief The frame context for the frame being processed
>> + * \var IPAContext::frameContextQueue
>> + * \brief FIFO container of IPAFrameContext for all the frames being queued
>>    *
>>    * \var IPAContext::activeState
>>    * \brief The current state of IPA algorithms
>> @@ -182,6 +182,15 @@ namespace libcamera::ipa::ipu3 {
>>    * <linux/intel-ipu3.h> struct ipu3_uapi_gamma_corr_lut for further details.
>>    */
>>   
>> +/**
>> + * \brief Construct a IPAFrameContext instance
>> + */
>> +IPAFrameContext::IPAFrameContext(uint32_t frame)
>> +       : frame(frame)
>> +{
>> +       sensor = {};
>> +}
>> +
>>   /**
>>    * \var IPAFrameContext::sensor
>>    * \brief Effective sensor values that were applied for the frame
>> @@ -191,6 +200,9 @@ namespace libcamera::ipa::ipu3 {
>>    *
>>    * \var IPAFrameContext::sensor.gain
>>    * \brief Analogue gain multiplier
>> + *
>> + * \var IPAFrameContext::frame
>> + * \brief The frame number associated with this IPAFrameContext
>>    */
>>   
>>   } /* namespace libcamera::ipa::ipu3 */
>> diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h
>> index 8d681131..20cccc97 100644
>> --- a/src/ipa/ipu3/ipa_context.h
>> +++ b/src/ipa/ipu3/ipa_context.h
>> @@ -8,6 +8,8 @@
>>   
>>   #pragma once
>>   
>> +#include <queue>
>> +
>>   #include <linux/intel-ipu3.h>
>>   
>>   #include <libcamera/base/utils.h>
>> @@ -71,17 +73,21 @@ struct IPAActiveState {
>>   };
>>   
>>   struct IPAFrameContext {
>> +       IPAFrameContext(uint32_t frame);
>> +
>>          struct {
>>                  uint32_t exposure;
>>                  double gain;
>>          } sensor;
>> +
>> +       uint32_t frame;
>>   };
>>   
>>   struct IPAContext {
>>          IPASessionConfiguration configuration;
>>          IPAActiveState activeState;
>>   
>> -       IPAFrameContext frameContext;
>> +       std::queue<IPAFrameContext> frameContextQueue;
>>   };
>>   
>>   } /* namespace ipa::ipu3 */
>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
>> index 268c8f61..1b566f14 100644
>> --- a/src/ipa/ipu3/ipu3.cpp
>> +++ b/src/ipa/ipu3/ipu3.cpp
>> @@ -350,6 +350,8 @@ int IPAIPU3::start()
>>    */
>>   void IPAIPU3::stop()
>>   {
>> +       while (!context_.frameContextQueue.empty())
>> +               context_.frameContextQueue.pop();
> Does context_.frameContextQueue.clear(); work here?


no, std::queue::clear() non-existent, however we can use

     context_.frameContextQueue = {};

to have the same effect.

>
>
>>   }
>>   
>>   /**
>> @@ -458,7 +460,7 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo,
>>   
>>          /* Clean IPAActiveState at each reconfiguration. */
>>          context_.activeState = {};
>> -       context_.frameContext = {};
>> +       context_.frameContextQueue = {};
>>   
>>          if (!validateSensorControls()) {
>>                  LOG(IPAIPU3, Error) << "Sensor control validation failed.";
>> @@ -510,6 +512,13 @@ void IPAIPU3::unmapBuffers(const std::vector<unsigned int> &ids)
>>   
>>   void IPAIPU3::frameCompleted([[maybe_unused]] const uint32_t frame)
> I suspect patch 1/3 could be squashed with this patch ... There's not a
> lot of distinction on the change - but I don't mind the split otherwise.


Ack. I'll keep separate for now.

>
>
>>   {
>> +       while (!context_.frameContextQueue.empty()) {
>> +               auto &fc = context_.frameContextQueue.front();
> I'm almost tempted to say we should have a warning/debug print in here
> if (fc.frame < frame). If it's the current frame (== frame) it's
> expected, but if it's earlier - then that's where some sort of frame
> drop has occurred - and we need to make sure that any request is still


Nice and good observation. It might not be 'true' frame drop detection I 
think,
but yes, having a warn if it's < currentFrame, is a marker that 
something went wrong.

> handled correctly if we're going to associated requests that get
> returned to notify a dropped frame.
>
> But I expect any frame drop handling would have already been handled
> before this frameCompleted gets called.


Frame drop handling seems a growing pit-fire, we need to address the 
mechanism separately :-)

>
>> +               if (fc.frame <= frame)
>> +                       context_.frameContextQueue.pop();
>> +               else
>> +                       break;
>> +       }
>>   }
>>   
>>   /**
>> @@ -574,8 +583,18 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame,
>>          const ipu3_uapi_stats_3a *stats =
>>                  reinterpret_cast<ipu3_uapi_stats_3a *>(mem.data());
>>   
>> -       context_.frameContext.sensor.exposure = sensorControls.get(V4L2_CID_EXPOSURE).get<int32_t>();
>> -       context_.frameContext.sensor.gain = camHelper_->gain(sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>());
>> +       auto &frameContext = context_.frameContextQueue.front();
>> +
>> +       /*
>> +        * An assert might be too harsh here. We want to know the cases
>> +        * where the front of the queue (implies the current frame in processing,
>> +        * diverges from the frame parameter of this function
>> +        * \todo Identify those cases - e.g. frame drop?
>> +        */
>> +       ASSERT(context_.frameContextQueue.front().frame == frame);
> This is certainly too hard I think. It would almost certainly cause
> libcamera to close on a frame drop (which could be very easy to cause to
> occur). So I don't think I'd like to merge this line.


I am not very keen on it either. Either we guarantee and eliminate all 
the situations where
frameContextQueue.front() != frame (which will involve listing down the 
possible edge scenarios
for e.g. frame-drop handling, and have grace failure paths for each of them)

Or we have this assert (or toned-down version of assert) to
let us know, that the front of the queue doesn't match up with the frame 
being processed.

>
>
>> +
>> +       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>());
>>   
>>          double lineDuration = context_.configuration.sensor.lineDuration.get<std::micro>();
>>          int32_t vBlank = context_.configuration.sensor.defVBlank;
>> @@ -590,11 +609,11 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame,
>>          int64_t frameDuration = (vBlank + sensorInfo_.outputSize.height) * lineDuration;
>>          ctrls.set(controls::FrameDuration, frameDuration);
>>   
>> -       ctrls.set(controls::AnalogueGain, context_.frameContext.sensor.gain);
>> +       ctrls.set(controls::AnalogueGain, frameContext.sensor.gain);
>>   
>>          ctrls.set(controls::ColourTemperature, context_.activeState.awb.temperatureK);
>>   
>> -       ctrls.set(controls::ExposureTime, context_.frameContext.sensor.exposure * lineDuration);
>> +       ctrls.set(controls::ExposureTime, frameContext.sensor.exposure * lineDuration);
>>   
>>          /*
>>           * \todo The Metadata provides a path to getting extended data
>> @@ -621,6 +640,8 @@ void IPAIPU3::queueRequest([[maybe_unused]] const uint32_t frame,
>>                             [[maybe_unused]] const ControlList &controls)
>>   {
>>          /* \todo Start processing for 'frame' based on 'controls'. */
>> +
>> +       context_.frameContextQueue.emplace(frame);
> But I think this is good - from here we can start parsing the request
> and controls and start doing something about request controls.


Ack.

>
>
> These patches are more or less ok to go in on their own I think (except
> that ASSERT which is far too hard) - but I'd be really keen to start
> seeing how we handle controls here soon.
>
> --
> Kieran
>
>
>>   }
>>   
>>   /**
>> -- 
>> 2.31.1
>>

Quoting Umang Jain (2022-05-06 14:48:34)
> Hi Kieran,
> 
> On 5/6/22 17:22, Kieran Bingham wrote:
> > Quoting Umang Jain via libcamera-devel (2022-05-06 10:53:07)
> >> Introduce a queue of IPAFrameContext which shall maintain the data
> >> required to track each frame. Currently, it is storing only the
> >> sensor controls values applied for the frame but new structures
> >> can be introduced in IPAFrameContext to extend any frame-related
> >> contexts (either in-processing or new frames being queued).
> > Technically I'm not sure those even need to be stored in the
> > FrameContext as they get set and then used in the same function I think.
> >
> > But ... I think it's fine for now, even as an example for the data - and
> > what will be more relevant is the controls that get set per-frame, and
> > anything that needs to be tracked to adapt to those.
> >
> > However that said, if we rework DelayedControls - then being able to
> > associate the expected Gain/Exposures to what was set is probably
> > useful to debug and check the implementation.
> 
> 
> I have a few ideas here, let me prepare a prototype branch and share it 
> with you.
> 
> >
> >
> >   
> >> For example, this patch provides the foundation for the extension
> >> of IPAFrameContext to store incoming request controls.
> >>
> >> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> >> ---
> >>   src/ipa/ipu3/algorithms/agc.cpp |  2 +-
> >>   src/ipa/ipu3/ipa_context.cpp    | 16 ++++++++++++++--
> >>   src/ipa/ipu3/ipa_context.h      |  8 +++++++-
> >>   src/ipa/ipu3/ipu3.cpp           | 31 ++++++++++++++++++++++++++-----
> >>   4 files changed, 48 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp
> >> index fdeec09d..4784af00 100644
> >> --- a/src/ipa/ipu3/algorithms/agc.cpp
> >> +++ b/src/ipa/ipu3/algorithms/agc.cpp
> >> @@ -187,7 +187,7 @@ void Agc::computeExposure(IPAContext &context, double yGain,
> >>                            double iqMeanGain)
> >>   {
> >>          const IPASessionConfiguration &configuration = context.configuration;
> >> -       IPAFrameContext &frameContext = context.frameContext;
> >> +       IPAFrameContext &frameContext = context.frameContextQueue.front();
> >>          /* Get the effective exposure and gain applied on the sensor. */
> >>          uint32_t exposure = frameContext.sensor.exposure;
> >>          double analogueGain = frameContext.sensor.gain;
> >> diff --git a/src/ipa/ipu3/ipa_context.cpp b/src/ipa/ipu3/ipa_context.cpp
> >> index 06eb2776..fb48bc9b 100644
> >> --- a/src/ipa/ipu3/ipa_context.cpp
> >> +++ b/src/ipa/ipu3/ipa_context.cpp
> >> @@ -58,8 +58,8 @@ namespace libcamera::ipa::ipu3 {
> >>    * \var IPAContext::configuration
> >>    * \brief The IPA session configuration, immutable during the session
> >>    *
> >> - * \var IPAContext::frameContext
> >> - * \brief The frame context for the frame being processed
> >> + * \var IPAContext::frameContextQueue
> >> + * \brief FIFO container of IPAFrameContext for all the frames being queued
> >>    *
> >>    * \var IPAContext::activeState
> >>    * \brief The current state of IPA algorithms
> >> @@ -182,6 +182,15 @@ namespace libcamera::ipa::ipu3 {
> >>    * <linux/intel-ipu3.h> struct ipu3_uapi_gamma_corr_lut for further details.
> >>    */
> >>   
> >> +/**
> >> + * \brief Construct a IPAFrameContext instance
> >> + */
> >> +IPAFrameContext::IPAFrameContext(uint32_t frame)
> >> +       : frame(frame)
> >> +{
> >> +       sensor = {};
> >> +}
> >> +
> >>   /**
> >>    * \var IPAFrameContext::sensor
> >>    * \brief Effective sensor values that were applied for the frame
> >> @@ -191,6 +200,9 @@ namespace libcamera::ipa::ipu3 {
> >>    *
> >>    * \var IPAFrameContext::sensor.gain
> >>    * \brief Analogue gain multiplier
> >> + *
> >> + * \var IPAFrameContext::frame
> >> + * \brief The frame number associated with this IPAFrameContext
> >>    */
> >>   
> >>   } /* namespace libcamera::ipa::ipu3 */
> >> diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h
> >> index 8d681131..20cccc97 100644
> >> --- a/src/ipa/ipu3/ipa_context.h
> >> +++ b/src/ipa/ipu3/ipa_context.h
> >> @@ -8,6 +8,8 @@
> >>   
> >>   #pragma once
> >>   
> >> +#include <queue>
> >> +
> >>   #include <linux/intel-ipu3.h>
> >>   
> >>   #include <libcamera/base/utils.h>
> >> @@ -71,17 +73,21 @@ struct IPAActiveState {
> >>   };
> >>   
> >>   struct IPAFrameContext {
> >> +       IPAFrameContext(uint32_t frame);
> >> +
> >>          struct {
> >>                  uint32_t exposure;
> >>                  double gain;
> >>          } sensor;
> >> +
> >> +       uint32_t frame;
> >>   };
> >>   
> >>   struct IPAContext {
> >>          IPASessionConfiguration configuration;
> >>          IPAActiveState activeState;
> >>   
> >> -       IPAFrameContext frameContext;
> >> +       std::queue<IPAFrameContext> frameContextQueue;
> >>   };
> >>   
> >>   } /* namespace ipa::ipu3 */
> >> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> >> index 268c8f61..1b566f14 100644
> >> --- a/src/ipa/ipu3/ipu3.cpp
> >> +++ b/src/ipa/ipu3/ipu3.cpp
> >> @@ -350,6 +350,8 @@ int IPAIPU3::start()
> >>    */
> >>   void IPAIPU3::stop()
> >>   {
> >> +       while (!context_.frameContextQueue.empty())
> >> +               context_.frameContextQueue.pop();
> > Does context_.frameContextQueue.clear(); work here?
> 
> 
> no, std::queue::clear() non-existent, however we can use
> 
>      context_.frameContextQueue = {};
> 
> to have the same effect.
> 
> >
> >
> >>   }
> >>   
> >>   /**
> >> @@ -458,7 +460,7 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo,
> >>   
> >>          /* Clean IPAActiveState at each reconfiguration. */
> >>          context_.activeState = {};
> >> -       context_.frameContext = {};
> >> +       context_.frameContextQueue = {};
> >>   
> >>          if (!validateSensorControls()) {
> >>                  LOG(IPAIPU3, Error) << "Sensor control validation failed.";
> >> @@ -510,6 +512,13 @@ void IPAIPU3::unmapBuffers(const std::vector<unsigned int> &ids)
> >>   
> >>   void IPAIPU3::frameCompleted([[maybe_unused]] const uint32_t frame)
> > I suspect patch 1/3 could be squashed with this patch ... There's not a
> > lot of distinction on the change - but I don't mind the split otherwise.
> 
> 
> Ack. I'll keep separate for now.
> 
> >
> >
> >>   {
> >> +       while (!context_.frameContextQueue.empty()) {
> >> +               auto &fc = context_.frameContextQueue.front();
> > I'm almost tempted to say we should have a warning/debug print in here
> > if (fc.frame < frame). If it's the current frame (== frame) it's
> > expected, but if it's earlier - then that's where some sort of frame
> > drop has occurred - and we need to make sure that any request is still
> 
> 
> Nice and good observation. It might not be 'true' frame drop detection I 
> think,
> but yes, having a warn if it's < currentFrame, is a marker that 
> something went wrong.
> 
> > handled correctly if we're going to associated requests that get
> > returned to notify a dropped frame.
> >
> > But I expect any frame drop handling would have already been handled
> > before this frameCompleted gets called.
> 
> 
> Frame drop handling seems a growing pit-fire, we need to address the 
> mechanism separately :-)
> 
> >
> >> +               if (fc.frame <= frame)
> >> +                       context_.frameContextQueue.pop();
> >> +               else
> >> +                       break;
> >> +       }
> >>   }
> >>   
> >>   /**
> >> @@ -574,8 +583,18 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame,
> >>          const ipu3_uapi_stats_3a *stats =
> >>                  reinterpret_cast<ipu3_uapi_stats_3a *>(mem.data());
> >>   
> >> -       context_.frameContext.sensor.exposure = sensorControls.get(V4L2_CID_EXPOSURE).get<int32_t>();
> >> -       context_.frameContext.sensor.gain = camHelper_->gain(sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>());
> >> +       auto &frameContext = context_.frameContextQueue.front();
> >> +
> >> +       /*
> >> +        * An assert might be too harsh here. We want to know the cases
> >> +        * where the front of the queue (implies the current frame in processing,
> >> +        * diverges from the frame parameter of this function
> >> +        * \todo Identify those cases - e.g. frame drop?
> >> +        */
> >> +       ASSERT(context_.frameContextQueue.front().frame == frame);
> > This is certainly too hard I think. It would almost certainly cause
> > libcamera to close on a frame drop (which could be very easy to cause to
> > occur). So I don't think I'd like to merge this line.
> 
> 
> I am not very keen on it either. Either we guarantee and eliminate all 
> the situations where
> frameContextQueue.front() != frame (which will involve listing down the 
> possible edge scenarios
> for e.g. frame-drop handling, and have grace failure paths for each of them)
> 
> Or we have this assert (or toned-down version of assert) to
> let us know, that the front of the queue doesn't match up with the frame 
> being processed.

I think if we're processing a stats buffer for a frame, that means we
successfully had the request queued in time to keep everything flowing -
but there could still have been a hole or gap that means a request
wasn't queued in time to satisfy the completion of the CIO2 - and we
would probably in that case have to drop a frame/request - and not
process it through the ISP. I think that means we'd have a 'gap' in the
sequence numbers here - so it may not be the front of the queue - in
which case we have to drop (and already complete) frameContexts up to
that point.

Perhaps that would have already happened befroe getting into this
function though - It might be clearer to map out on a sequence diagram.

> >
> >
> >> +
> >> +       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>());
> >>   
> >>          double lineDuration = context_.configuration.sensor.lineDuration.get<std::micro>();
> >>          int32_t vBlank = context_.configuration.sensor.defVBlank;
> >> @@ -590,11 +609,11 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame,
> >>          int64_t frameDuration = (vBlank + sensorInfo_.outputSize.height) * lineDuration;
> >>          ctrls.set(controls::FrameDuration, frameDuration);
> >>   
> >> -       ctrls.set(controls::AnalogueGain, context_.frameContext.sensor.gain);
> >> +       ctrls.set(controls::AnalogueGain, frameContext.sensor.gain);
> >>   
> >>          ctrls.set(controls::ColourTemperature, context_.activeState.awb.temperatureK);
> >>   
> >> -       ctrls.set(controls::ExposureTime, context_.frameContext.sensor.exposure * lineDuration);
> >> +       ctrls.set(controls::ExposureTime, frameContext.sensor.exposure * lineDuration);
> >>   
> >>          /*
> >>           * \todo The Metadata provides a path to getting extended data
> >> @@ -621,6 +640,8 @@ void IPAIPU3::queueRequest([[maybe_unused]] const uint32_t frame,
> >>                             [[maybe_unused]] const ControlList &controls)
> >>   {
> >>          /* \todo Start processing for 'frame' based on 'controls'. */
> >> +
> >> +       context_.frameContextQueue.emplace(frame);
> > But I think this is good - from here we can start parsing the request
> > and controls and start doing something about request controls.
> 
> 
> Ack.
> 
> >
> >
> > These patches are more or less ok to go in on their own I think (except
> > that ASSERT which is far too hard) - but I'd be really keen to start
> > seeing how we handle controls here soon.
> >
> > --
> > Kieran
> >
> >
> >>   }
> >>   
> >>   /**
> >> -- 
> >> 2.31.1
> >>

Quoting Kieran Bingham (2022-05-06 19:12:44)
> Quoting Umang Jain (2022-05-06 14:48:34)
> > Hi Kieran,
> > 
> > On 5/6/22 17:22, Kieran Bingham wrote:
> > > Quoting Umang Jain via libcamera-devel (2022-05-06 10:53:07)
> > >> Introduce a queue of IPAFrameContext which shall maintain the data
> > >> required to track each frame. Currently, it is storing only the
> > >> sensor controls values applied for the frame but new structures
> > >> can be introduced in IPAFrameContext to extend any frame-related
> > >> contexts (either in-processing or new frames being queued).
> > > Technically I'm not sure those even need to be stored in the
> > > FrameContext as they get set and then used in the same function I think.
> > >
> > > But ... I think it's fine for now, even as an example for the data - and
> > > what will be more relevant is the controls that get set per-frame, and
> > > anything that needs to be tracked to adapt to those.

So having looked closer, I think these are actually worth keeping in
this structure. Technically they could be brought into the IPA and
stored in the FrameContext during the call/time point of
"fillParamsBuffer()", as this is the earliest time that the frame has
completed, and we can know what was set on the sensor.

But I don't believe the values are used until the time of
processStatsBuffer() - and currently DelayedControls is storing the
information for us on the PipelineHandler side.



> > >
> > > However that said, if we rework DelayedControls - then being able to
> > > associate the expected Gain/Exposures to what was set is probably
> > > useful to debug and check the implementation.
> > 
> > 
> > I have a few ideas here, let me prepare a prototype branch and share it 
> > with you.
> > 
> > >
> > >
> > >   
> > >> For example, this patch provides the foundation for the extension
> > >> of IPAFrameContext to store incoming request controls.
> > >>
> > >> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> > >> ---
> > >>   src/ipa/ipu3/algorithms/agc.cpp |  2 +-
> > >>   src/ipa/ipu3/ipa_context.cpp    | 16 ++++++++++++++--
> > >>   src/ipa/ipu3/ipa_context.h      |  8 +++++++-
> > >>   src/ipa/ipu3/ipu3.cpp           | 31 ++++++++++++++++++++++++++-----
> > >>   4 files changed, 48 insertions(+), 9 deletions(-)
> > >>
> > >> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp
> > >> index fdeec09d..4784af00 100644
> > >> --- a/src/ipa/ipu3/algorithms/agc.cpp
> > >> +++ b/src/ipa/ipu3/algorithms/agc.cpp
> > >> @@ -187,7 +187,7 @@ void Agc::computeExposure(IPAContext &context, double yGain,
> > >>                            double iqMeanGain)
> > >>   {
> > >>          const IPASessionConfiguration &configuration = context.configuration;
> > >> -       IPAFrameContext &frameContext = context.frameContext;
> > >> +       IPAFrameContext &frameContext = context.frameContextQueue.front();
> > >>          /* Get the effective exposure and gain applied on the sensor. */
> > >>          uint32_t exposure = frameContext.sensor.exposure;
> > >>          double analogueGain = frameContext.sensor.gain;
> > >> diff --git a/src/ipa/ipu3/ipa_context.cpp b/src/ipa/ipu3/ipa_context.cpp
> > >> index 06eb2776..fb48bc9b 100644
> > >> --- a/src/ipa/ipu3/ipa_context.cpp
> > >> +++ b/src/ipa/ipu3/ipa_context.cpp
> > >> @@ -58,8 +58,8 @@ namespace libcamera::ipa::ipu3 {
> > >>    * \var IPAContext::configuration
> > >>    * \brief The IPA session configuration, immutable during the session
> > >>    *
> > >> - * \var IPAContext::frameContext
> > >> - * \brief The frame context for the frame being processed
> > >> + * \var IPAContext::frameContextQueue
> > >> + * \brief FIFO container of IPAFrameContext for all the frames being queued
> > >>    *
> > >>    * \var IPAContext::activeState
> > >>    * \brief The current state of IPA algorithms
> > >> @@ -182,6 +182,15 @@ namespace libcamera::ipa::ipu3 {
> > >>    * <linux/intel-ipu3.h> struct ipu3_uapi_gamma_corr_lut for further details.
> > >>    */
> > >>   
> > >> +/**
> > >> + * \brief Construct a IPAFrameContext instance
> > >> + */
> > >> +IPAFrameContext::IPAFrameContext(uint32_t frame)
> > >> +       : frame(frame)
> > >> +{
> > >> +       sensor = {};
> > >> +}
> > >> +
> > >>   /**
> > >>    * \var IPAFrameContext::sensor
> > >>    * \brief Effective sensor values that were applied for the frame
> > >> @@ -191,6 +200,9 @@ namespace libcamera::ipa::ipu3 {
> > >>    *
> > >>    * \var IPAFrameContext::sensor.gain
> > >>    * \brief Analogue gain multiplier
> > >> + *
> > >> + * \var IPAFrameContext::frame
> > >> + * \brief The frame number associated with this IPAFrameContext
> > >>    */
> > >>   
> > >>   } /* namespace libcamera::ipa::ipu3 */
> > >> diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h
> > >> index 8d681131..20cccc97 100644
> > >> --- a/src/ipa/ipu3/ipa_context.h
> > >> +++ b/src/ipa/ipu3/ipa_context.h
> > >> @@ -8,6 +8,8 @@
> > >>   
> > >>   #pragma once
> > >>   
> > >> +#include <queue>
> > >> +
> > >>   #include <linux/intel-ipu3.h>
> > >>   
> > >>   #include <libcamera/base/utils.h>
> > >> @@ -71,17 +73,21 @@ struct IPAActiveState {
> > >>   };
> > >>   
> > >>   struct IPAFrameContext {
> > >> +       IPAFrameContext(uint32_t frame);
> > >> +
> > >>          struct {
> > >>                  uint32_t exposure;
> > >>                  double gain;
> > >>          } sensor;
> > >> +
> > >> +       uint32_t frame;
> > >>   };
> > >>   
> > >>   struct IPAContext {
> > >>          IPASessionConfiguration configuration;
> > >>          IPAActiveState activeState;
> > >>   
> > >> -       IPAFrameContext frameContext;
> > >> +       std::queue<IPAFrameContext> frameContextQueue;

Further discussions have identified that we might want to use an array
or vector here to implement a fixed size ring buffer which could be more
efficient.

I think that could also be done on top, as an implementation detail
though.


> > >>   };
> > >>   
> > >>   } /* namespace ipa::ipu3 */
> > >> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> > >> index 268c8f61..1b566f14 100644
> > >> --- a/src/ipa/ipu3/ipu3.cpp
> > >> +++ b/src/ipa/ipu3/ipu3.cpp
> > >> @@ -350,6 +350,8 @@ int IPAIPU3::start()
> > >>    */
> > >>   void IPAIPU3::stop()
> > >>   {
> > >> +       while (!context_.frameContextQueue.empty())
> > >> +               context_.frameContextQueue.pop();
> > > Does context_.frameContextQueue.clear(); work here?
> > 
> > 
> > no, std::queue::clear() non-existent, however we can use
> > 
> >      context_.frameContextQueue = {};
> > 
> > to have the same effect.

Ok.

I wonder if we'll end up with a custom template class to handle
RingBuffers which will clean this up anyway.



> > 
> > >
> > >
> > >>   }
> > >>   
> > >>   /**
> > >> @@ -458,7 +460,7 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo,
> > >>   
> > >>          /* Clean IPAActiveState at each reconfiguration. */
> > >>          context_.activeState = {};
> > >> -       context_.frameContext = {};
> > >> +       context_.frameContextQueue = {};
> > >>   
> > >>          if (!validateSensorControls()) {
> > >>                  LOG(IPAIPU3, Error) << "Sensor control validation failed.";
> > >> @@ -510,6 +512,13 @@ void IPAIPU3::unmapBuffers(const std::vector<unsigned int> &ids)
> > >>   
> > >>   void IPAIPU3::frameCompleted([[maybe_unused]] const uint32_t frame)
> > > I suspect patch 1/3 could be squashed with this patch ... There's not a
> > > lot of distinction on the change - but I don't mind the split otherwise.
> > 
> > 
> > Ack. I'll keep separate for now.

Fine with me.


> > 
> > >
> > >
> > >>   {
> > >> +       while (!context_.frameContextQueue.empty()) {
> > >> +               auto &fc = context_.frameContextQueue.front();
> > > I'm almost tempted to say we should have a warning/debug print in here
> > > if (fc.frame < frame). If it's the current frame (== frame) it's
> > > expected, but if it's earlier - then that's where some sort of frame
> > > drop has occurred - and we need to make sure that any request is still
> > 
> > 
> > Nice and good observation. It might not be 'true' frame drop detection I 
> > think,
> > but yes, having a warn if it's < currentFrame, is a marker that 
> > something went wrong.
> > 
> > > handled correctly if we're going to associated requests that get
> > > returned to notify a dropped frame.
> > >
> > > But I expect any frame drop handling would have already been handled
> > > before this frameCompleted gets called.
> > 
> > 
> > Frame drop handling seems a growing pit-fire, we need to address the 
> > mechanism separately :-)

I think it became more clear that we need to have a function that can
'get' a FrameContext by frame id. So this should/could be a helper (or
part of a ring buffer class or such?)



> > 
> > >
> > >> +               if (fc.frame <= frame)
> > >> +                       context_.frameContextQueue.pop();
> > >> +               else
> > >> +                       break;
> > >> +       }
> > >>   }
> > >>   
> > >>   /**
> > >> @@ -574,8 +583,18 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame,
> > >>          const ipu3_uapi_stats_3a *stats =
> > >>                  reinterpret_cast<ipu3_uapi_stats_3a *>(mem.data());
> > >>   
> > >> -       context_.frameContext.sensor.exposure = sensorControls.get(V4L2_CID_EXPOSURE).get<int32_t>();
> > >> -       context_.frameContext.sensor.gain = camHelper_->gain(sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>());
> > >> +       auto &frameContext = context_.frameContextQueue.front();
> > >> +
> > >> +       /*
> > >> +        * An assert might be too harsh here. We want to know the cases
> > >> +        * where the front of the queue (implies the current frame in processing,
> > >> +        * diverges from the frame parameter of this function
> > >> +        * \todo Identify those cases - e.g. frame drop?
> > >> +        */
> > >> +       ASSERT(context_.frameContextQueue.front().frame == frame);
> > > This is certainly too hard I think. It would almost certainly cause
> > > libcamera to close on a frame drop (which could be very easy to cause to
> > > occur). So I don't think I'd like to merge this line.
> > 
> > 
> > I am not very keen on it either. Either we guarantee and eliminate all 
> > the situations where
> > frameContextQueue.front() != frame (which will involve listing down the 
> > possible edge scenarios
> > for e.g. frame-drop handling, and have grace failure paths for each of them)
> > 
> > Or we have this assert (or toned-down version of assert) to
> > let us know, that the front of the queue doesn't match up with the frame 
> > being processed.
> 
> I think if we're processing a stats buffer for a frame, that means we
> successfully had the request queued in time to keep everything flowing -
> but there could still have been a hole or gap that means a request
> wasn't queued in time to satisfy the completion of the CIO2 - and we
> would probably in that case have to drop a frame/request - and not
> process it through the ISP. I think that means we'd have a 'gap' in the
> sequence numbers here - so it may not be the front of the queue - in
> which case we have to drop (and already complete) frameContexts up to
> that point.
> 
> Perhaps that would have already happened befroe getting into this
> function though - It might be clearer to map out on a sequence diagram.
> 

I think switching to a helper that 'gets' the frameContext by FrameID
will clear this all up anyway, and then we can handle any earlier
frame's that aren't yet completed in sequence when relevant.


> > >
> > >
> > >> +
> > >> +       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>());
> > >>   
> > >>          double lineDuration = context_.configuration.sensor.lineDuration.get<std::micro>();
> > >>          int32_t vBlank = context_.configuration.sensor.defVBlank;
> > >> @@ -590,11 +609,11 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame,
> > >>          int64_t frameDuration = (vBlank + sensorInfo_.outputSize.height) * lineDuration;
> > >>          ctrls.set(controls::FrameDuration, frameDuration);
> > >>   
> > >> -       ctrls.set(controls::AnalogueGain, context_.frameContext.sensor.gain);
> > >> +       ctrls.set(controls::AnalogueGain, frameContext.sensor.gain);
> > >>   
> > >>          ctrls.set(controls::ColourTemperature, context_.activeState.awb.temperatureK);
> > >>   
> > >> -       ctrls.set(controls::ExposureTime, context_.frameContext.sensor.exposure * lineDuration);
> > >> +       ctrls.set(controls::ExposureTime, frameContext.sensor.exposure * lineDuration);
> > >>   
> > >>          /*
> > >>           * \todo The Metadata provides a path to getting extended data
> > >> @@ -621,6 +640,8 @@ void IPAIPU3::queueRequest([[maybe_unused]] const uint32_t frame,
> > >>                             [[maybe_unused]] const ControlList &controls)
> > >>   {
> > >>          /* \todo Start processing for 'frame' based on 'controls'. */
> > >> +
> > >> +       context_.frameContextQueue.emplace(frame);
> > > But I think this is good - from here we can start parsing the request
> > > and controls and start doing something about request controls.
> > 
> > 
> > Ack.


It might simply be enough to take a copy of the ControlList &controls
and put it in the FrameContext (for each frame) here ... but it depends
how much processing is involved in working through the controls, and how
much we need to manage that across algorithsm, or handling cases where a
control is or is not set.



> > 
> > >
> > >
> > > These patches are more or less ok to go in on their own I think (except
> > > that ASSERT which is far too hard) - but I'd be really keen to start
> > > seeing how we handle controls here soon.

Removing the ASSERT and having a helper to get a frame context by ID I
think is the next key step... lets see if there's any more comments from
the others.

--
Kieran



> > >
> > > --
> > > Kieran
> > >
> > >
> > >>   }
> > >>   
> > >>   /**
> > >> -- 
> > >> 2.31.1
> > >>