[libcamera-devel] ipa: rkisp1: Add support for manual gain and exposure

17380 diff mbox series

Add support for manual gain and exposure in the rkisp1 IPA.

Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>

---
This patch depends on v4 of the series: "ipa: Frame context queue, IPU3
& RkISP consolidation, and RkISP1 improvements"
---
 src/ipa/rkisp1/algorithms/agc.cpp | 59 +++++++++++++++++++++++++++----
 src/ipa/rkisp1/algorithms/agc.h   |  4 +++
 src/ipa/rkisp1/ipa_context.h      | 13 +++++--
 src/ipa/rkisp1/rkisp1.cpp         |  3 ++
 4 files changed, 71 insertions(+), 8 deletions(-)

Quoting Paul Elder via libcamera-devel (2022-09-19 07:37:43)
> Add support for manual gain and exposure in the rkisp1 IPA.
> 
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> 
> ---
> This patch depends on v4 of the series: "ipa: Frame context queue, IPU3
> & RkISP consolidation, and RkISP1 improvements"

Will you plan to add the same support to the IPU3 to keep them
consistent? Or is that no something you've looked at yet ? (It's fine to
wait for this to be figured out, but I don't want IPU3 and RKISP to have
different behaviours if we can avoid it, so it might be that we should
update both at the same time)



> ---
>  src/ipa/rkisp1/algorithms/agc.cpp | 59 +++++++++++++++++++++++++++----
>  src/ipa/rkisp1/algorithms/agc.h   |  4 +++
>  src/ipa/rkisp1/ipa_context.h      | 13 +++++--
>  src/ipa/rkisp1/rkisp1.cpp         |  3 ++
>  4 files changed, 71 insertions(+), 8 deletions(-)
> 
> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
> index bc764142..d26b35ed 100644
> --- a/src/ipa/rkisp1/algorithms/agc.cpp
> +++ b/src/ipa/rkisp1/algorithms/agc.cpp
> @@ -14,6 +14,7 @@
>  #include <libcamera/base/log.h>
>  #include <libcamera/base/utils.h>
>  
> +#include <libcamera/control_ids.h>
>  #include <libcamera/ipa/core_ipa_interface.h>
>  
>  #include "libipa/histogram.h"
> @@ -73,8 +74,11 @@ Agc::Agc()
>  int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)
>  {
>         /* Configure the default exposure and gain. */
> -       context.activeState.agc.gain = std::max(context.configuration.agc.minAnalogueGain, kMinAnalogueGain);
> -       context.activeState.agc.exposure = 10ms / context.configuration.sensor.lineDuration;
> +       context.activeState.agc.automatic.gain = std::max(context.configuration.agc.minAnalogueGain, kMinAnalogueGain);
> +       context.activeState.agc.automatic.exposure = 10ms / context.configuration.sensor.lineDuration;
> +       context.activeState.agc.manual.gain = context.activeState.agc.automatic.gain;
> +       context.activeState.agc.manual.exposure = context.activeState.agc.automatic.exposure;
> +       context.activeState.agc.autoEnabled = true;
>  
>         /*
>          * According to the RkISP1 documentation:
> @@ -221,8 +225,8 @@ void Agc::computeExposure(IPAContext &context, RkISP1FrameContext &frameContext,
>                               << stepGain;
>  
>         /* Update the estimated exposure and gain. */
> -       activeState.agc.exposure = shutterTime / configuration.sensor.lineDuration;
> -       activeState.agc.gain = stepGain;
> +       activeState.agc.automatic.exposure = shutterTime / configuration.sensor.lineDuration;
> +       activeState.agc.automatic.gain = stepGain;
>  }
>  
>  /**
> @@ -331,8 +335,10 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
>  void Agc::prepare(IPAContext &context, const uint32_t frame,
>                   RkISP1FrameContext &frameContext, rkisp1_params_cfg *params)
>  {
> -       frameContext.agc.exposure = context.activeState.agc.exposure;
> -       frameContext.agc.gain = context.activeState.agc.gain;
> +       if (frameContext.agc.autoEnabled) {
> +               frameContext.agc.exposure = context.activeState.agc.automatic.exposure;
> +               frameContext.agc.gain = context.activeState.agc.automatic.gain;
> +       }

I'm not quite sure if this is right yet. Will prepare be called at a
time that we know these are the exposure and gains applied to the sensor
in time for the frame associated with frameContext?

If not, we can still do this as a transitionary change - but we'd need
to add a todo/comment here.

The frameContext should reflect what will be/is actually configured for the
frame. But I know that's not easy right now in regards to the
delayedControls implementations...




>  
>         if (frame > 0)
>                 return;
> @@ -365,6 +371,47 @@ void Agc::prepare(IPAContext &context, const uint32_t frame,
>         params->module_en_update |= RKISP1_CIF_ISP_MODULE_HST;
>  }
>  
> +/**
> + * \copydoc libcamera::ipa::Algorithm::queueRequest
> + */
> +void Agc::queueRequest(IPAContext &context,
> +                      [[maybe_unused]] const uint32_t frame,
> +                      RkISP1FrameContext &frameContext,
> +                      const ControlList &controls)
> +{
> +       auto &agc = context.activeState.agc;
> +
> +       const auto &agcEnable = controls.get(controls::AeEnable);
> +       if (agcEnable && *agcEnable != agc.autoEnabled) {
> +               agc.autoEnabled = *agcEnable;
> +
> +               LOG(RkISP1Agc, Debug)
> +                       << (*agcEnable ? "Enabling" : "Disabling") << " AGC";
> +       }
> +
> +       const auto &exposure = controls.get(controls::ExposureTime);
> +       if (exposure && !agc.autoEnabled) {

I think this shouldn't check !agc.autoEnabled. If a new manual exposure
time is passed in, we can update the agc.manual.exposure accordingly.

Or do we explicitly disallow setting manual exposures when AeEnable is
enabled?

What would 

 Frame 1: AeEnabled = true; ManualExposure = 15ms; 
 Frame 2: AeEnabled = false;

Be expected to use for ManualExposure?



> +               agc.manual.exposure = *exposure;
> +
> +               LOG(RkISP1Agc, Debug)
> +                       << "Set exposure to: " << agc.manual.exposure;
> +       }
> +
> +       const auto &gain = controls.get(controls::AnalogueGain);
> +       if (gain && !agc.autoEnabled) {

Same here.... They will only be set to the FrameContext if autoEnabled
is set below...

But I guess that means we are either auto AGC or manual exposure &&
gain.


> +               agc.manual.gain = *gain;
> +
> +               LOG(RkISP1Agc, Debug) << "Set gain to: " << agc.manual.gain;
> +       }
> +
> +       frameContext.agc.autoEnabled = agc.autoEnabled;

Yes! The FC will certainly take the state of this at this point.


> +
> +       if (!frameContext.agc.autoEnabled) {
> +               frameContext.agc.exposure = agc.manual.exposure;
> +               frameContext.agc.gain = agc.manual.gain;

I think this shows we don't yet support automatic gain but fixed
exposure... (or vice-versa)


Is that something we need to consider later? or is there anything we
need to do now for that ?



> +       }
> +}
> +
>  REGISTER_IPA_ALGORITHM(Agc, "Agc")
>  
>  } /* namespace ipa::rkisp1::algorithms */
> diff --git a/src/ipa/rkisp1/algorithms/agc.h b/src/ipa/rkisp1/algorithms/agc.h
> index d6c6fb13..e624f101 100644
> --- a/src/ipa/rkisp1/algorithms/agc.h
> +++ b/src/ipa/rkisp1/algorithms/agc.h
> @@ -32,6 +32,10 @@ public:
>         void process(IPAContext &context, const uint32_t frame,
>                      RkISP1FrameContext &frameContext,
>                      const rkisp1_stat_buffer *stats) override;
> +       void queueRequest(IPAContext &context,
> +                         const uint32_t frame,
> +                         RkISP1FrameContext &frameContext,
> +                         const ControlList &controls) override;
>  
>  private:
>         void computeExposure(IPAContext &Context, RkISP1FrameContext &frameContext,
> diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
> index d0bc9090..df72ec87 100644
> --- a/src/ipa/rkisp1/ipa_context.h
> +++ b/src/ipa/rkisp1/ipa_context.h
> @@ -50,8 +50,16 @@ struct IPASessionConfiguration {
>  
>  struct IPAActiveState {
>         struct {
> -               uint32_t exposure;
> -               double gain;
> +               struct {
> +                       uint32_t exposure;
> +                       double gain;
> +               } manual;
> +               struct {
> +                       uint32_t exposure;
> +                       double gain;
> +               } automatic;
> +
> +               bool autoEnabled;
>         } agc;
>  
>         struct {
> @@ -92,6 +100,7 @@ struct RkISP1FrameContext : public FrameContext {
>         struct {
>                 uint32_t exposure;
>                 double gain;
> +               bool autoEnabled;
>         } agc;
>  
>         struct {
> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> index 75370ac8..0092d0a1 100644
> --- a/src/ipa/rkisp1/rkisp1.cpp
> +++ b/src/ipa/rkisp1/rkisp1.cpp
> @@ -92,8 +92,11 @@ private:
>  namespace {
>  
>  /* List of controls handled by the RkISP1 IPA */
> +/* \todo Report more accurate limits for exposure and gain */

I'd also like to see this table get moved so that each algorithm
generates the entries... which might have to be another callback on the
module or handled at init() time or such perhaps. But not required for
this patch.


>  const ControlInfoMap::Map rkisp1Controls{
>         { &controls::AeEnable, ControlInfo(false, true) },
> +       { &controls::ExposureTime, ControlInfo(0, 66666) },
> +       { &controls::AnalogueGain, ControlInfo(1.0f, 16.0f) },
>         { &controls::AwbEnable, ControlInfo(false, true) },
>         { &controls::ColourGains, ControlInfo(0.0f, 3.996f, 1.0f) },
>         { &controls::Brightness, ControlInfo(-1.0f, 0.993f) },
> -- 
> 2.30.2
>