[PATCH v2] ipa: rpi: Apply default ControlInfo values for sensor controls

[David Plowman]

Hi Naush

Thanks for the revised version.

On Thu, 6 Feb 2025 at 11:33, Kieran Bingham
<kieran.bingham at ideasonboard.com> wrote:
>
> Quoting Naushir Patuck (2025-02-06 09:24:36)
> > The existing IPA initialisation code did not set default values for
> > some sensor related controls. This caused a crash using libcamerify
> > when the it was trying to access the default value for
> > controls::FrameDurationLimits as part of a recent change.
> >
> > Ensure controls::FrameDurationLimits, controls::AnalogueGain and
> > controls::ExposureTime advertise default values along with the existing
> > min/max values. The default is set to the defaults defined in the IPA
> > set during initialisation.
> >
> > Bug: https://bugs.libcamera.org/show_bug.cgi?id=253
> > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>

Reviewed-by: David Plowman <david.plowman at raspberrypi.com>

David

> > ---
> >  src/ipa/rpi/common/ipa_base.cpp | 9 ++++++---
> >  1 file changed, 6 insertions(+), 3 deletions(-)
> >
> > diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp
> > index bd3c22000df5..3c77665e6ab4 100644
> > --- a/src/ipa/rpi/common/ipa_base.cpp
> > +++ b/src/ipa/rpi/common/ipa_base.cpp
> > @@ -259,15 +259,18 @@ int32_t IpaBase::configure(const IPACameraSensorInfo &sensorInfo, const ConfigPa
> >         ControlInfoMap::Map ctrlMap = ipaControls;
> >         ctrlMap[&controls::FrameDurationLimits] =
> >                 ControlInfo(static_cast<int64_t>(mode_.minFrameDuration.get<std::micro>()),
> > -                           static_cast<int64_t>(mode_.maxFrameDuration.get<std::micro>()));
> > +                           static_cast<int64_t>(mode_.maxFrameDuration.get<std::micro>()),
> > +                           static_cast<int64_t>(defaultMinFrameDuration.get<std::micro>()));
> >
>
> Is there any risk or concern now that the default could be reported
> outside of the min/max values when configured?
>
> But reading the if (firstStart_) {} code block, I think these are the
> values that will be used on start so I expect they are at least the
> current representation of the defaults.
>
>
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>
> >         ctrlMap[&controls::AnalogueGain] =
> >                 ControlInfo(static_cast<float>(mode_.minAnalogueGain),
> > -                           static_cast<float>(mode_.maxAnalogueGain));
> > +                           static_cast<float>(mode_.maxAnalogueGain),
> > +                           static_cast<float>(defaultAnalogueGain));
> >
> >         ctrlMap[&controls::ExposureTime] =
> >                 ControlInfo(static_cast<int32_t>(mode_.minExposureTime.get<std::micro>()),
> > -                           static_cast<int32_t>(mode_.maxExposureTime.get<std::micro>()));
> > +                           static_cast<int32_t>(mode_.maxExposureTime.get<std::micro>()),
> > +                           static_cast<int32_t>(defaultExposureTime.get<std::micro>()));
> >
> >         /* Declare colour processing related controls for non-mono sensors. */
> >         if (!monoSensor_)
> > --
> > 2.43.0
> >