[libcamera-devel,v2,2/2] libcamera: raspberrypi: Fetch correct value for sensor's modeSensitivity
diff mbox series

Message ID 20210922132915.14881-3-david.plowman@raspberrypi.com
State Superseded
Headers show
Series
  • Application support for per-mode sensitivities
Related show

Commit Message

David Plowman Sept. 22, 2021, 1:29 p.m. UTC 
These changes retrieve the correct value for sensitivity of the mode
selected for the sensor. This value is known to the CamHelper which
passes it across to the pipeline handler so that it can be set
correctly in the CameraConfiguration.

Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
---
 include/libcamera/ipa/raspberrypi.mojom            |  7 ++++++-
 src/ipa/raspberrypi/raspberrypi.cpp                |  7 +++++--
 src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 10 +++++++---
 3 files changed, 18 insertions(+), 6 deletions(-)

Comments

Kieran Bingham Jan. 4, 2022, 11:13 a.m. UTC | #1 
Quoting David Plowman (2021-09-22 14:29:15)
> These changes retrieve the correct value for sensitivity of the mode
> selected for the sensor. This value is known to the CamHelper which
> passes it across to the pipeline handler so that it can be set
> correctly in the CameraConfiguration.
> 
> Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
> ---
>  include/libcamera/ipa/raspberrypi.mojom            |  7 ++++++-
>  src/ipa/raspberrypi/raspberrypi.cpp                |  7 +++++--
>  src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 10 +++++++---
>  3 files changed, 18 insertions(+), 6 deletions(-)
> 
> diff --git a/include/libcamera/ipa/raspberrypi.mojom b/include/libcamera/ipa/raspberrypi.mojom
> index e453d46c..a92a76f8 100644
> --- a/include/libcamera/ipa/raspberrypi.mojom
> +++ b/include/libcamera/ipa/raspberrypi.mojom
> @@ -38,6 +38,10 @@ struct IPAConfig {
>         libcamera.FileDescriptor lsTableHandle;
>  };
>  
> +struct IPAConfigResult {
> +       float modeSensitivity;
> +};
> +
>  struct StartConfig {
>         libcamera.ControlList controls;
>         int32 dropFrameCount;
> @@ -57,6 +61,7 @@ interface IPARPiInterface {
>          * \param[in] entityControls Controls provided by the pipeline entities
>          * \param[in] ipaConfig Pipeline-handler-specific configuration data
>          * \param[out] controls Controls to apply by the pipeline entity
> +        * \param[out] result Other results that the pipeline handler may require
>          *
>          * This function shall be called when the camera is configured to inform
>          * the IPA of the camera's streams and the sensor settings.
> @@ -71,7 +76,7 @@ interface IPARPiInterface {
>                   map<uint32, libcamera.IPAStream> streamConfig,
>                   map<uint32, libcamera.ControlInfoMap> entityControls,
>                   IPAConfig ipaConfig)
> -               => (int32 ret, libcamera.ControlList controls);
> +               => (int32 ret, libcamera.ControlList controls, IPAConfigResult result);
>  
>         /**
>          * \fn mapBuffers()
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> index 047123ab..796e6d15 100644
> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> @@ -97,7 +97,7 @@ public:
>                       const std::map<unsigned int, IPAStream> &streamConfig,
>                       const std::map<unsigned int, ControlInfoMap> &entityControls,
>                       const ipa::RPi::IPAConfig &data,
> -                     ControlList *controls) override;
> +                     ControlList *controls, ipa::RPi::IPAConfigResult *result) override;
>         void mapBuffers(const std::vector<IPABuffer> &buffers) override;
>         void unmapBuffers(const std::vector<unsigned int> &ids) override;
>         void signalStatReady(const uint32_t bufferId) override;
> @@ -337,7 +337,7 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo,
>                       [[maybe_unused]] const std::map<unsigned int, IPAStream> &streamConfig,
>                       const std::map<unsigned int, ControlInfoMap> &entityControls,
>                       const ipa::RPi::IPAConfig &ipaConfig,
> -                     ControlList *controls)
> +                     ControlList *controls, ipa::RPi::IPAConfigResult *result)
>  {
>         if (entityControls.size() != 2) {
>                 LOG(IPARPI, Error) << "No ISP or sensor controls found.";
> @@ -389,6 +389,9 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo,
>         /* Pass the camera mode to the CamHelper to setup algorithms. */
>         helper_->SetCameraMode(mode_);
>  
> +       /* The pipeline handler passes out the mode's sensitivity. */
> +       result->modeSensitivity = mode_.sensitivity;
> +
>         if (firstStart_) {
>                 /* Supply initial values for frame durations. */
>                 applyFrameDurations(defaultMinFrameDuration, defaultMaxFrameDuration);
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index 0bdfa727..caf0030e 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -147,7 +147,7 @@ public:
>         void frameStarted(uint32_t sequence);
>  
>         int loadIPA(ipa::RPi::SensorConfig *sensorConfig);
> -       int configureIPA(const CameraConfiguration *config);
> +       int configureIPA(CameraConfiguration *config);
>  
>         void statsMetadataComplete(uint32_t bufferId, const ControlList &controls);
>         void runIsp(uint32_t bufferId);
> @@ -1250,7 +1250,7 @@ int RPiCameraData::loadIPA(ipa::RPi::SensorConfig *sensorConfig)
>         return ipa_->init(settings, sensorConfig);
>  }
>  
> -int RPiCameraData::configureIPA(const CameraConfiguration *config)
> +int RPiCameraData::configureIPA(CameraConfiguration *config)
>  {
>         /* We know config must be an RPiCameraConfiguration. */
>         const RPiCameraConfiguration *rpiConfig =
> @@ -1299,13 +1299,17 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config)
>  
>         /* Ready the IPA - it must know about the sensor resolution. */
>         ControlList controls;
> +       ipa::RPi::IPAConfigResult result;
>         ret = ipa_->configure(sensorInfo_, streamConfig, entityControls, ipaConfig,
> -                             &controls);
> +                             &controls, &result);

This should probably be being passed back as a control/property here.
But I think right now the &controls is a set of V4L2 Controls, and not
libcamera controls.

We've been discussing about changing that so that the IPAs pass
libcamera::controls instead. Currently Isolated IPAs will break when we
try to pass lens controls because that's 'yet another infomap' that will
have to be serialised.

If we instead use only libcamera controls in the interface between the
pipeline handler and the IPA this would be resolved, and it would mean
that list could also pass the sensitivity property.

Is there any reason you're aware of as to why we could or couldn't use
libcamera controls here? I know Laurent has been concerned in the past
about perhaps if there was any precision issues or rounding that the IPA
might need to know about that would otherwise occur in the pipeline
handler. Do you see this as an issue? or would we be able to
successfully convert the interfaces to use libcamera control lists only
(and keep the V4L2'ness' hidden in the V4L2 layer / pipeline handler
only).

All that said, changing the controls interface there is still possible
on top, even with this patch, so I don't object to passing it directly
back for now.


Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

--
Kieran


>         if (ret < 0) {
>                 LOG(RPI, Error) << "IPA configuration failed!";
>                 return -EPIPE;
>         }
>  
> +       /* Store the mode sensitivity for the application. */
> +       config->modeSensitivity = result.modeSensitivity;
> +
>         /*
>          * Configure the H/V flip controls based on the combination of
>          * the sensor and user transform.
> -- 
> 2.20.1
>
David Plowman Jan. 4, 2022, 1:33 p.m. UTC | #2 
Hi again

On Tue, 4 Jan 2022 at 11:13, Kieran Bingham
<kieran.bingham@ideasonboard.com> wrote:
>
> Quoting David Plowman (2021-09-22 14:29:15)
> > These changes retrieve the correct value for sensitivity of the mode
> > selected for the sensor. This value is known to the CamHelper which
> > passes it across to the pipeline handler so that it can be set
> > correctly in the CameraConfiguration.
> >
> > Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
> > ---
> >  include/libcamera/ipa/raspberrypi.mojom            |  7 ++++++-
> >  src/ipa/raspberrypi/raspberrypi.cpp                |  7 +++++--
> >  src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 10 +++++++---
> >  3 files changed, 18 insertions(+), 6 deletions(-)
> >
> > diff --git a/include/libcamera/ipa/raspberrypi.mojom b/include/libcamera/ipa/raspberrypi.mojom
> > index e453d46c..a92a76f8 100644
> > --- a/include/libcamera/ipa/raspberrypi.mojom
> > +++ b/include/libcamera/ipa/raspberrypi.mojom
> > @@ -38,6 +38,10 @@ struct IPAConfig {
> >         libcamera.FileDescriptor lsTableHandle;
> >  };
> >
> > +struct IPAConfigResult {
> > +       float modeSensitivity;
> > +};
> > +
> >  struct StartConfig {
> >         libcamera.ControlList controls;
> >         int32 dropFrameCount;
> > @@ -57,6 +61,7 @@ interface IPARPiInterface {
> >          * \param[in] entityControls Controls provided by the pipeline entities
> >          * \param[in] ipaConfig Pipeline-handler-specific configuration data
> >          * \param[out] controls Controls to apply by the pipeline entity
> > +        * \param[out] result Other results that the pipeline handler may require
> >          *
> >          * This function shall be called when the camera is configured to inform
> >          * the IPA of the camera's streams and the sensor settings.
> > @@ -71,7 +76,7 @@ interface IPARPiInterface {
> >                   map<uint32, libcamera.IPAStream> streamConfig,
> >                   map<uint32, libcamera.ControlInfoMap> entityControls,
> >                   IPAConfig ipaConfig)
> > -               => (int32 ret, libcamera.ControlList controls);
> > +               => (int32 ret, libcamera.ControlList controls, IPAConfigResult result);
> >
> >         /**
> >          * \fn mapBuffers()
> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> > index 047123ab..796e6d15 100644
> > --- a/src/ipa/raspberrypi/raspberrypi.cpp
> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> > @@ -97,7 +97,7 @@ public:
> >                       const std::map<unsigned int, IPAStream> &streamConfig,
> >                       const std::map<unsigned int, ControlInfoMap> &entityControls,
> >                       const ipa::RPi::IPAConfig &data,
> > -                     ControlList *controls) override;
> > +                     ControlList *controls, ipa::RPi::IPAConfigResult *result) override;
> >         void mapBuffers(const std::vector<IPABuffer> &buffers) override;
> >         void unmapBuffers(const std::vector<unsigned int> &ids) override;
> >         void signalStatReady(const uint32_t bufferId) override;
> > @@ -337,7 +337,7 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo,
> >                       [[maybe_unused]] const std::map<unsigned int, IPAStream> &streamConfig,
> >                       const std::map<unsigned int, ControlInfoMap> &entityControls,
> >                       const ipa::RPi::IPAConfig &ipaConfig,
> > -                     ControlList *controls)
> > +                     ControlList *controls, ipa::RPi::IPAConfigResult *result)
> >  {
> >         if (entityControls.size() != 2) {
> >                 LOG(IPARPI, Error) << "No ISP or sensor controls found.";
> > @@ -389,6 +389,9 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo,
> >         /* Pass the camera mode to the CamHelper to setup algorithms. */
> >         helper_->SetCameraMode(mode_);
> >
> > +       /* The pipeline handler passes out the mode's sensitivity. */
> > +       result->modeSensitivity = mode_.sensitivity;
> > +
> >         if (firstStart_) {
> >                 /* Supply initial values for frame durations. */
> >                 applyFrameDurations(defaultMinFrameDuration, defaultMaxFrameDuration);
> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > index 0bdfa727..caf0030e 100644
> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > @@ -147,7 +147,7 @@ public:
> >         void frameStarted(uint32_t sequence);
> >
> >         int loadIPA(ipa::RPi::SensorConfig *sensorConfig);
> > -       int configureIPA(const CameraConfiguration *config);
> > +       int configureIPA(CameraConfiguration *config);
> >
> >         void statsMetadataComplete(uint32_t bufferId, const ControlList &controls);
> >         void runIsp(uint32_t bufferId);
> > @@ -1250,7 +1250,7 @@ int RPiCameraData::loadIPA(ipa::RPi::SensorConfig *sensorConfig)
> >         return ipa_->init(settings, sensorConfig);
> >  }
> >
> > -int RPiCameraData::configureIPA(const CameraConfiguration *config)
> > +int RPiCameraData::configureIPA(CameraConfiguration *config)
> >  {
> >         /* We know config must be an RPiCameraConfiguration. */
> >         const RPiCameraConfiguration *rpiConfig =
> > @@ -1299,13 +1299,17 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config)
> >
> >         /* Ready the IPA - it must know about the sensor resolution. */
> >         ControlList controls;
> > +       ipa::RPi::IPAConfigResult result;
> >         ret = ipa_->configure(sensorInfo_, streamConfig, entityControls, ipaConfig,
> > -                             &controls);
> > +                             &controls, &result);
>
> This should probably be being passed back as a control/property here.
> But I think right now the &controls is a set of V4L2 Controls, and not
> libcamera controls.
>
> We've been discussing about changing that so that the IPAs pass
> libcamera::controls instead. Currently Isolated IPAs will break when we
> try to pass lens controls because that's 'yet another infomap' that will
> have to be serialised.
>
> If we instead use only libcamera controls in the interface between the
> pipeline handler and the IPA this would be resolved, and it would mean
> that list could also pass the sensitivity property.
>
> Is there any reason you're aware of as to why we could or couldn't use
> libcamera controls here? I know Laurent has been concerned in the past
> about perhaps if there was any precision issues or rounding that the IPA
> might need to know about that would otherwise occur in the pipeline
> handler. Do you see this as an issue? or would we be able to
> successfully convert the interfaces to use libcamera control lists only
> (and keep the V4L2'ness' hidden in the V4L2 layer / pipeline handler
> only).

I can't think of any precision issues that particularly bother me
(which of course doesn't mean there aren't any). As I said in my other
reply, I do need this value to be available from the call to
configure(), before you get to call start(), so it's not clear to me
at the minute where I would read this value back - but I'm guessing
someone has some ideas?

David

>
> All that said, changing the controls interface there is still possible
> on top, even with this patch, so I don't object to passing it directly
> back for now.
>
>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>
> --
> Kieran
>
>
> >         if (ret < 0) {
> >                 LOG(RPI, Error) << "IPA configuration failed!";
> >                 return -EPIPE;
> >         }
> >
> > +       /* Store the mode sensitivity for the application. */
> > +       config->modeSensitivity = result.modeSensitivity;
> > +
> >         /*
> >          * Configure the H/V flip controls based on the combination of
> >          * the sensor and user transform.
> > --
> > 2.20.1
> >
Kieran Bingham Feb. 9, 2022, 12:27 p.m. UTC | #3 
Quoting David Plowman (2022-01-04 13:33:05)
> Hi again
> 
> On Tue, 4 Jan 2022 at 11:13, Kieran Bingham
> <kieran.bingham@ideasonboard.com> wrote:
> >
> > Quoting David Plowman (2021-09-22 14:29:15)
> > > These changes retrieve the correct value for sensitivity of the mode
> > > selected for the sensor. This value is known to the CamHelper which
> > > passes it across to the pipeline handler so that it can be set
> > > correctly in the CameraConfiguration.
> > >
> > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
> > > ---
> > >  include/libcamera/ipa/raspberrypi.mojom            |  7 ++++++-
> > >  src/ipa/raspberrypi/raspberrypi.cpp                |  7 +++++--
> > >  src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 10 +++++++---
> > >  3 files changed, 18 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/include/libcamera/ipa/raspberrypi.mojom b/include/libcamera/ipa/raspberrypi.mojom
> > > index e453d46c..a92a76f8 100644
> > > --- a/include/libcamera/ipa/raspberrypi.mojom
> > > +++ b/include/libcamera/ipa/raspberrypi.mojom
> > > @@ -38,6 +38,10 @@ struct IPAConfig {
> > >         libcamera.FileDescriptor lsTableHandle;
> > >  };
> > >
> > > +struct IPAConfigResult {
> > > +       float modeSensitivity;
> > > +};
> > > +
> > >  struct StartConfig {
> > >         libcamera.ControlList controls;
> > >         int32 dropFrameCount;
> > > @@ -57,6 +61,7 @@ interface IPARPiInterface {
> > >          * \param[in] entityControls Controls provided by the pipeline entities
> > >          * \param[in] ipaConfig Pipeline-handler-specific configuration data
> > >          * \param[out] controls Controls to apply by the pipeline entity
> > > +        * \param[out] result Other results that the pipeline handler may require
> > >          *
> > >          * This function shall be called when the camera is configured to inform
> > >          * the IPA of the camera's streams and the sensor settings.
> > > @@ -71,7 +76,7 @@ interface IPARPiInterface {
> > >                   map<uint32, libcamera.IPAStream> streamConfig,
> > >                   map<uint32, libcamera.ControlInfoMap> entityControls,
> > >                   IPAConfig ipaConfig)
> > > -               => (int32 ret, libcamera.ControlList controls);
> > > +               => (int32 ret, libcamera.ControlList controls, IPAConfigResult result);
> > >
> > >         /**
> > >          * \fn mapBuffers()
> > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> > > index 047123ab..796e6d15 100644
> > > --- a/src/ipa/raspberrypi/raspberrypi.cpp
> > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> > > @@ -97,7 +97,7 @@ public:
> > >                       const std::map<unsigned int, IPAStream> &streamConfig,
> > >                       const std::map<unsigned int, ControlInfoMap> &entityControls,
> > >                       const ipa::RPi::IPAConfig &data,
> > > -                     ControlList *controls) override;
> > > +                     ControlList *controls, ipa::RPi::IPAConfigResult *result) override;
> > >         void mapBuffers(const std::vector<IPABuffer> &buffers) override;
> > >         void unmapBuffers(const std::vector<unsigned int> &ids) override;
> > >         void signalStatReady(const uint32_t bufferId) override;
> > > @@ -337,7 +337,7 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo,
> > >                       [[maybe_unused]] const std::map<unsigned int, IPAStream> &streamConfig,
> > >                       const std::map<unsigned int, ControlInfoMap> &entityControls,
> > >                       const ipa::RPi::IPAConfig &ipaConfig,
> > > -                     ControlList *controls)
> > > +                     ControlList *controls, ipa::RPi::IPAConfigResult *result)
> > >  {
> > >         if (entityControls.size() != 2) {
> > >                 LOG(IPARPI, Error) << "No ISP or sensor controls found.";
> > > @@ -389,6 +389,9 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo,
> > >         /* Pass the camera mode to the CamHelper to setup algorithms. */
> > >         helper_->SetCameraMode(mode_);
> > >
> > > +       /* The pipeline handler passes out the mode's sensitivity. */
> > > +       result->modeSensitivity = mode_.sensitivity;
> > > +
> > >         if (firstStart_) {
> > >                 /* Supply initial values for frame durations. */
> > >                 applyFrameDurations(defaultMinFrameDuration, defaultMaxFrameDuration);
> > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > index 0bdfa727..caf0030e 100644
> > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > @@ -147,7 +147,7 @@ public:
> > >         void frameStarted(uint32_t sequence);
> > >
> > >         int loadIPA(ipa::RPi::SensorConfig *sensorConfig);
> > > -       int configureIPA(const CameraConfiguration *config);
> > > +       int configureIPA(CameraConfiguration *config);
> > >
> > >         void statsMetadataComplete(uint32_t bufferId, const ControlList &controls);
> > >         void runIsp(uint32_t bufferId);
> > > @@ -1250,7 +1250,7 @@ int RPiCameraData::loadIPA(ipa::RPi::SensorConfig *sensorConfig)
> > >         return ipa_->init(settings, sensorConfig);
> > >  }
> > >
> > > -int RPiCameraData::configureIPA(const CameraConfiguration *config)
> > > +int RPiCameraData::configureIPA(CameraConfiguration *config)
> > >  {
> > >         /* We know config must be an RPiCameraConfiguration. */
> > >         const RPiCameraConfiguration *rpiConfig =
> > > @@ -1299,13 +1299,17 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config)
> > >
> > >         /* Ready the IPA - it must know about the sensor resolution. */
> > >         ControlList controls;
> > > +       ipa::RPi::IPAConfigResult result;
> > >         ret = ipa_->configure(sensorInfo_, streamConfig, entityControls, ipaConfig,
> > > -                             &controls);
> > > +                             &controls, &result);
> >
> > This should probably be being passed back as a control/property here.
> > But I think right now the &controls is a set of V4L2 Controls, and not
> > libcamera controls.
> >
> > We've been discussing about changing that so that the IPAs pass
> > libcamera::controls instead. Currently Isolated IPAs will break when we
> > try to pass lens controls because that's 'yet another infomap' that will
> > have to be serialised.
> >
> > If we instead use only libcamera controls in the interface between the
> > pipeline handler and the IPA this would be resolved, and it would mean
> > that list could also pass the sensitivity property.
> >
> > Is there any reason you're aware of as to why we could or couldn't use
> > libcamera controls here? I know Laurent has been concerned in the past
> > about perhaps if there was any precision issues or rounding that the IPA
> > might need to know about that would otherwise occur in the pipeline
> > handler. Do you see this as an issue? or would we be able to
> > successfully convert the interfaces to use libcamera control lists only
> > (and keep the V4L2'ness' hidden in the V4L2 layer / pipeline handler
> > only).
> 
> I can't think of any precision issues that particularly bother me
> (which of course doesn't mean there aren't any). As I said in my other
> reply, I do need this value to be available from the call to
> configure(), before you get to call start(), so it's not clear to me
> at the minute where I would read this value back - but I'm guessing
> someone has some ideas?

That discussion above was more about the type of the Control rather than
where it gets returned.

I'm confused here if the start() and configure() you are referencing is
the public API, or the IPA interface API?

--
Kieran


> 
> David
> 
> >
> > All that said, changing the controls interface there is still possible
> > on top, even with this patch, so I don't object to passing it directly
> > back for now.
> >
> >
> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> >
> > --
> > Kieran
> >
> >
> > >         if (ret < 0) {
> > >                 LOG(RPI, Error) << "IPA configuration failed!";
> > >                 return -EPIPE;
> > >         }
> > >
> > > +       /* Store the mode sensitivity for the application. */
> > > +       config->modeSensitivity = result.modeSensitivity;
> > > +
> > >         /*
> > >          * Configure the H/V flip controls based on the combination of
> > >          * the sensor and user transform.
> > > --
> > > 2.20.1
> > >
David Plowman Feb. 9, 2022, 1 p.m. UTC | #4 
Hi Kieran

On Wed, 9 Feb 2022 at 12:27, Kieran Bingham
<kieran.bingham@ideasonboard.com> wrote:
>
> Quoting David Plowman (2022-01-04 13:33:05)
> > Hi again
> >
> > On Tue, 4 Jan 2022 at 11:13, Kieran Bingham
> > <kieran.bingham@ideasonboard.com> wrote:
> > >
> > > Quoting David Plowman (2021-09-22 14:29:15)
> > > > These changes retrieve the correct value for sensitivity of the mode
> > > > selected for the sensor. This value is known to the CamHelper which
> > > > passes it across to the pipeline handler so that it can be set
> > > > correctly in the CameraConfiguration.
> > > >
> > > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
> > > > ---
> > > >  include/libcamera/ipa/raspberrypi.mojom            |  7 ++++++-
> > > >  src/ipa/raspberrypi/raspberrypi.cpp                |  7 +++++--
> > > >  src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 10 +++++++---
> > > >  3 files changed, 18 insertions(+), 6 deletions(-)
> > > >
> > > > diff --git a/include/libcamera/ipa/raspberrypi.mojom b/include/libcamera/ipa/raspberrypi.mojom
> > > > index e453d46c..a92a76f8 100644
> > > > --- a/include/libcamera/ipa/raspberrypi.mojom
> > > > +++ b/include/libcamera/ipa/raspberrypi.mojom
> > > > @@ -38,6 +38,10 @@ struct IPAConfig {
> > > >         libcamera.FileDescriptor lsTableHandle;
> > > >  };
> > > >
> > > > +struct IPAConfigResult {
> > > > +       float modeSensitivity;
> > > > +};
> > > > +
> > > >  struct StartConfig {
> > > >         libcamera.ControlList controls;
> > > >         int32 dropFrameCount;
> > > > @@ -57,6 +61,7 @@ interface IPARPiInterface {
> > > >          * \param[in] entityControls Controls provided by the pipeline entities
> > > >          * \param[in] ipaConfig Pipeline-handler-specific configuration data
> > > >          * \param[out] controls Controls to apply by the pipeline entity
> > > > +        * \param[out] result Other results that the pipeline handler may require
> > > >          *
> > > >          * This function shall be called when the camera is configured to inform
> > > >          * the IPA of the camera's streams and the sensor settings.
> > > > @@ -71,7 +76,7 @@ interface IPARPiInterface {
> > > >                   map<uint32, libcamera.IPAStream> streamConfig,
> > > >                   map<uint32, libcamera.ControlInfoMap> entityControls,
> > > >                   IPAConfig ipaConfig)
> > > > -               => (int32 ret, libcamera.ControlList controls);
> > > > +               => (int32 ret, libcamera.ControlList controls, IPAConfigResult result);
> > > >
> > > >         /**
> > > >          * \fn mapBuffers()
> > > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> > > > index 047123ab..796e6d15 100644
> > > > --- a/src/ipa/raspberrypi/raspberrypi.cpp
> > > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> > > > @@ -97,7 +97,7 @@ public:
> > > >                       const std::map<unsigned int, IPAStream> &streamConfig,
> > > >                       const std::map<unsigned int, ControlInfoMap> &entityControls,
> > > >                       const ipa::RPi::IPAConfig &data,
> > > > -                     ControlList *controls) override;
> > > > +                     ControlList *controls, ipa::RPi::IPAConfigResult *result) override;
> > > >         void mapBuffers(const std::vector<IPABuffer> &buffers) override;
> > > >         void unmapBuffers(const std::vector<unsigned int> &ids) override;
> > > >         void signalStatReady(const uint32_t bufferId) override;
> > > > @@ -337,7 +337,7 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo,
> > > >                       [[maybe_unused]] const std::map<unsigned int, IPAStream> &streamConfig,
> > > >                       const std::map<unsigned int, ControlInfoMap> &entityControls,
> > > >                       const ipa::RPi::IPAConfig &ipaConfig,
> > > > -                     ControlList *controls)
> > > > +                     ControlList *controls, ipa::RPi::IPAConfigResult *result)
> > > >  {
> > > >         if (entityControls.size() != 2) {
> > > >                 LOG(IPARPI, Error) << "No ISP or sensor controls found.";
> > > > @@ -389,6 +389,9 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo,
> > > >         /* Pass the camera mode to the CamHelper to setup algorithms. */
> > > >         helper_->SetCameraMode(mode_);
> > > >
> > > > +       /* The pipeline handler passes out the mode's sensitivity. */
> > > > +       result->modeSensitivity = mode_.sensitivity;
> > > > +
> > > >         if (firstStart_) {
> > > >                 /* Supply initial values for frame durations. */
> > > >                 applyFrameDurations(defaultMinFrameDuration, defaultMaxFrameDuration);
> > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > > index 0bdfa727..caf0030e 100644
> > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > > @@ -147,7 +147,7 @@ public:
> > > >         void frameStarted(uint32_t sequence);
> > > >
> > > >         int loadIPA(ipa::RPi::SensorConfig *sensorConfig);
> > > > -       int configureIPA(const CameraConfiguration *config);
> > > > +       int configureIPA(CameraConfiguration *config);
> > > >
> > > >         void statsMetadataComplete(uint32_t bufferId, const ControlList &controls);
> > > >         void runIsp(uint32_t bufferId);
> > > > @@ -1250,7 +1250,7 @@ int RPiCameraData::loadIPA(ipa::RPi::SensorConfig *sensorConfig)
> > > >         return ipa_->init(settings, sensorConfig);
> > > >  }
> > > >
> > > > -int RPiCameraData::configureIPA(const CameraConfiguration *config)
> > > > +int RPiCameraData::configureIPA(CameraConfiguration *config)
> > > >  {
> > > >         /* We know config must be an RPiCameraConfiguration. */
> > > >         const RPiCameraConfiguration *rpiConfig =
> > > > @@ -1299,13 +1299,17 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config)
> > > >
> > > >         /* Ready the IPA - it must know about the sensor resolution. */
> > > >         ControlList controls;
> > > > +       ipa::RPi::IPAConfigResult result;
> > > >         ret = ipa_->configure(sensorInfo_, streamConfig, entityControls, ipaConfig,
> > > > -                             &controls);
> > > > +                             &controls, &result);
> > >
> > > This should probably be being passed back as a control/property here.
> > > But I think right now the &controls is a set of V4L2 Controls, and not
> > > libcamera controls.
> > >
> > > We've been discussing about changing that so that the IPAs pass
> > > libcamera::controls instead. Currently Isolated IPAs will break when we
> > > try to pass lens controls because that's 'yet another infomap' that will
> > > have to be serialised.
> > >
> > > If we instead use only libcamera controls in the interface between the
> > > pipeline handler and the IPA this would be resolved, and it would mean
> > > that list could also pass the sensitivity property.
> > >
> > > Is there any reason you're aware of as to why we could or couldn't use
> > > libcamera controls here? I know Laurent has been concerned in the past
> > > about perhaps if there was any precision issues or rounding that the IPA
> > > might need to know about that would otherwise occur in the pipeline
> > > handler. Do you see this as an issue? or would we be able to
> > > successfully convert the interfaces to use libcamera control lists only
> > > (and keep the V4L2'ness' hidden in the V4L2 layer / pipeline handler
> > > only).
> >
> > I can't think of any precision issues that particularly bother me
> > (which of course doesn't mean there aren't any). As I said in my other
> > reply, I do need this value to be available from the call to
> > configure(), before you get to call start(), so it's not clear to me
> > at the minute where I would read this value back - but I'm guessing
> > someone has some ideas?
>
> That discussion above was more about the type of the Control rather than
> where it gets returned.
>
> I'm confused here if the start() and configure() you are referencing is
> the public API, or the IPA interface API?

I'm referring to the public Camera API. It's applications that will
want to know this number.

David

>
> --
> Kieran
>
>
> >
> > David
> >
> > >
> > > All that said, changing the controls interface there is still possible
> > > on top, even with this patch, so I don't object to passing it directly
> > > back for now.
> > >
> > >
> > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > >
> > > --
> > > Kieran
> > >
> > >
> > > >         if (ret < 0) {
> > > >                 LOG(RPI, Error) << "IPA configuration failed!";
> > > >                 return -EPIPE;
> > > >         }
> > > >
> > > > +       /* Store the mode sensitivity for the application. */
> > > > +       config->modeSensitivity = result.modeSensitivity;
> > > > +
> > > >         /*
> > > >          * Configure the H/V flip controls based on the combination of
> > > >          * the sensor and user transform.
> > > > --
> > > > 2.20.1
> > > >
Kieran Bingham Feb. 9, 2022, 1:17 p.m. UTC | #5 
Quoting David Plowman (2022-02-09 13:00:57)
> Hi Kieran
> 
> On Wed, 9 Feb 2022 at 12:27, Kieran Bingham
> <kieran.bingham@ideasonboard.com> wrote:
> >
> > Quoting David Plowman (2022-01-04 13:33:05)
> > > Hi again
> > >
> > > On Tue, 4 Jan 2022 at 11:13, Kieran Bingham
> > > <kieran.bingham@ideasonboard.com> wrote:
> > > >
> > > > Quoting David Plowman (2021-09-22 14:29:15)
> > > > > These changes retrieve the correct value for sensitivity of the mode
> > > > > selected for the sensor. This value is known to the CamHelper which
> > > > > passes it across to the pipeline handler so that it can be set
> > > > > correctly in the CameraConfiguration.
> > > > >
> > > > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
> > > > > ---
> > > > >  include/libcamera/ipa/raspberrypi.mojom            |  7 ++++++-
> > > > >  src/ipa/raspberrypi/raspberrypi.cpp                |  7 +++++--
> > > > >  src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 10 +++++++---
> > > > >  3 files changed, 18 insertions(+), 6 deletions(-)
> > > > >
> > > > > diff --git a/include/libcamera/ipa/raspberrypi.mojom b/include/libcamera/ipa/raspberrypi.mojom
> > > > > index e453d46c..a92a76f8 100644
> > > > > --- a/include/libcamera/ipa/raspberrypi.mojom
> > > > > +++ b/include/libcamera/ipa/raspberrypi.mojom
> > > > > @@ -38,6 +38,10 @@ struct IPAConfig {
> > > > >         libcamera.FileDescriptor lsTableHandle;
> > > > >  };
> > > > >
> > > > > +struct IPAConfigResult {
> > > > > +       float modeSensitivity;
> > > > > +};
> > > > > +
> > > > >  struct StartConfig {
> > > > >         libcamera.ControlList controls;
> > > > >         int32 dropFrameCount;
> > > > > @@ -57,6 +61,7 @@ interface IPARPiInterface {
> > > > >          * \param[in] entityControls Controls provided by the pipeline entities
> > > > >          * \param[in] ipaConfig Pipeline-handler-specific configuration data
> > > > >          * \param[out] controls Controls to apply by the pipeline entity
> > > > > +        * \param[out] result Other results that the pipeline handler may require
> > > > >          *
> > > > >          * This function shall be called when the camera is configured to inform
> > > > >          * the IPA of the camera's streams and the sensor settings.
> > > > > @@ -71,7 +76,7 @@ interface IPARPiInterface {
> > > > >                   map<uint32, libcamera.IPAStream> streamConfig,
> > > > >                   map<uint32, libcamera.ControlInfoMap> entityControls,
> > > > >                   IPAConfig ipaConfig)
> > > > > -               => (int32 ret, libcamera.ControlList controls);
> > > > > +               => (int32 ret, libcamera.ControlList controls, IPAConfigResult result);
> > > > >
> > > > >         /**
> > > > >          * \fn mapBuffers()
> > > > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> > > > > index 047123ab..796e6d15 100644
> > > > > --- a/src/ipa/raspberrypi/raspberrypi.cpp
> > > > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> > > > > @@ -97,7 +97,7 @@ public:
> > > > >                       const std::map<unsigned int, IPAStream> &streamConfig,
> > > > >                       const std::map<unsigned int, ControlInfoMap> &entityControls,
> > > > >                       const ipa::RPi::IPAConfig &data,
> > > > > -                     ControlList *controls) override;
> > > > > +                     ControlList *controls, ipa::RPi::IPAConfigResult *result) override;
> > > > >         void mapBuffers(const std::vector<IPABuffer> &buffers) override;
> > > > >         void unmapBuffers(const std::vector<unsigned int> &ids) override;
> > > > >         void signalStatReady(const uint32_t bufferId) override;
> > > > > @@ -337,7 +337,7 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo,
> > > > >                       [[maybe_unused]] const std::map<unsigned int, IPAStream> &streamConfig,
> > > > >                       const std::map<unsigned int, ControlInfoMap> &entityControls,
> > > > >                       const ipa::RPi::IPAConfig &ipaConfig,
> > > > > -                     ControlList *controls)
> > > > > +                     ControlList *controls, ipa::RPi::IPAConfigResult *result)
> > > > >  {
> > > > >         if (entityControls.size() != 2) {
> > > > >                 LOG(IPARPI, Error) << "No ISP or sensor controls found.";
> > > > > @@ -389,6 +389,9 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo,
> > > > >         /* Pass the camera mode to the CamHelper to setup algorithms. */
> > > > >         helper_->SetCameraMode(mode_);
> > > > >
> > > > > +       /* The pipeline handler passes out the mode's sensitivity. */
> > > > > +       result->modeSensitivity = mode_.sensitivity;
> > > > > +
> > > > >         if (firstStart_) {
> > > > >                 /* Supply initial values for frame durations. */
> > > > >                 applyFrameDurations(defaultMinFrameDuration, defaultMaxFrameDuration);
> > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > > > index 0bdfa727..caf0030e 100644
> > > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > > > @@ -147,7 +147,7 @@ public:
> > > > >         void frameStarted(uint32_t sequence);
> > > > >
> > > > >         int loadIPA(ipa::RPi::SensorConfig *sensorConfig);
> > > > > -       int configureIPA(const CameraConfiguration *config);
> > > > > +       int configureIPA(CameraConfiguration *config);
> > > > >
> > > > >         void statsMetadataComplete(uint32_t bufferId, const ControlList &controls);
> > > > >         void runIsp(uint32_t bufferId);
> > > > > @@ -1250,7 +1250,7 @@ int RPiCameraData::loadIPA(ipa::RPi::SensorConfig *sensorConfig)
> > > > >         return ipa_->init(settings, sensorConfig);
> > > > >  }
> > > > >
> > > > > -int RPiCameraData::configureIPA(const CameraConfiguration *config)
> > > > > +int RPiCameraData::configureIPA(CameraConfiguration *config)
> > > > >  {
> > > > >         /* We know config must be an RPiCameraConfiguration. */
> > > > >         const RPiCameraConfiguration *rpiConfig =
> > > > > @@ -1299,13 +1299,17 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config)
> > > > >
> > > > >         /* Ready the IPA - it must know about the sensor resolution. */
> > > > >         ControlList controls;
> > > > > +       ipa::RPi::IPAConfigResult result;
> > > > >         ret = ipa_->configure(sensorInfo_, streamConfig, entityControls, ipaConfig,
> > > > > -                             &controls);
> > > > > +                             &controls, &result);
> > > >
> > > > This should probably be being passed back as a control/property here.
> > > > But I think right now the &controls is a set of V4L2 Controls, and not
> > > > libcamera controls.
> > > >
> > > > We've been discussing about changing that so that the IPAs pass
> > > > libcamera::controls instead. Currently Isolated IPAs will break when we
> > > > try to pass lens controls because that's 'yet another infomap' that will
> > > > have to be serialised.
> > > >
> > > > If we instead use only libcamera controls in the interface between the
> > > > pipeline handler and the IPA this would be resolved, and it would mean
> > > > that list could also pass the sensitivity property.
> > > >
> > > > Is there any reason you're aware of as to why we could or couldn't use
> > > > libcamera controls here? I know Laurent has been concerned in the past
> > > > about perhaps if there was any precision issues or rounding that the IPA
> > > > might need to know about that would otherwise occur in the pipeline
> > > > handler. Do you see this as an issue? or would we be able to
> > > > successfully convert the interfaces to use libcamera control lists only
> > > > (and keep the V4L2'ness' hidden in the V4L2 layer / pipeline handler
> > > > only).
> > >
> > > I can't think of any precision issues that particularly bother me
> > > (which of course doesn't mean there aren't any). As I said in my other
> > > reply, I do need this value to be available from the call to
> > > configure(), before you get to call start(), so it's not clear to me
> > > at the minute where I would read this value back - but I'm guessing
> > > someone has some ideas?
> >
> > That discussion above was more about the type of the Control rather than
> > where it gets returned.
> >
> > I'm confused here if the start() and configure() you are referencing is
> > the public API, or the IPA interface API?
> 
> I'm referring to the public Camera API. It's applications that will
> want to know this number.

Ok, so it can certainly be updated by the configure operation, but I
wonder if it's something that the model would expect to be determined
during validate()?

Configure() is supposed to be a 'non-changing' operation ... but this is
more like a return value. Would an application expect to know the mode
sensitivity after they've validated a configuration? (to check it is
what they expect?) before even calling configure?

--
Kieran

> 
> David
> 
> >
> > --
> > Kieran
> >
> >
> > >
> > > David
> > >
> > > >
> > > > All that said, changing the controls interface there is still possible
> > > > on top, even with this patch, so I don't object to passing it directly
> > > > back for now.
> > > >
> > > >
> > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > > >
> > > > --
> > > > Kieran
> > > >
> > > >
> > > > >         if (ret < 0) {
> > > > >                 LOG(RPI, Error) << "IPA configuration failed!";
> > > > >                 return -EPIPE;
> > > > >         }
> > > > >
> > > > > +       /* Store the mode sensitivity for the application. */
> > > > > +       config->modeSensitivity = result.modeSensitivity;
> > > > > +
> > > > >         /*
> > > > >          * Configure the H/V flip controls based on the combination of
> > > > >          * the sensor and user transform.
> > > > > --
> > > > > 2.20.1
> > > > >
Naushir Patuck April 13, 2022, 9:13 a.m. UTC | #6 
Hi David,

Thank you for your patch.

On Wed, 22 Sept 2021 at 14:29, David Plowman <david.plowman@raspberrypi.com>
wrote:

> These changes retrieve the correct value for sensitivity of the mode
> selected for the sensor. This value is known to the CamHelper which
> passes it across to the pipeline handler so that it can be set
> correctly in the CameraConfiguration.
>
> Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
>

I agree with Kieran that we might want to consider passing libcamera
controls from the IPA
instead of v4l2 controls.  The mode sensitivity can then be put into a
control list. But that is
not for this series.

Reviewed-by: Naushir Patuck <naush@raspberrypi.com>


> ---
>  include/libcamera/ipa/raspberrypi.mojom            |  7 ++++++-
>  src/ipa/raspberrypi/raspberrypi.cpp                |  7 +++++--
>  src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 10 +++++++---
>  3 files changed, 18 insertions(+), 6 deletions(-)
>
> diff --git a/include/libcamera/ipa/raspberrypi.mojom
> b/include/libcamera/ipa/raspberrypi.mojom
> index e453d46c..a92a76f8 100644
> --- a/include/libcamera/ipa/raspberrypi.mojom
> +++ b/include/libcamera/ipa/raspberrypi.mojom
> @@ -38,6 +38,10 @@ struct IPAConfig {
>         libcamera.FileDescriptor lsTableHandle;
>  };
>
> +struct IPAConfigResult {
> +       float modeSensitivity;
> +};
> +
>  struct StartConfig {
>         libcamera.ControlList controls;
>         int32 dropFrameCount;
> @@ -57,6 +61,7 @@ interface IPARPiInterface {
>          * \param[in] entityControls Controls provided by the pipeline
> entities
>          * \param[in] ipaConfig Pipeline-handler-specific configuration
> data
>          * \param[out] controls Controls to apply by the pipeline entity
> +        * \param[out] result Other results that the pipeline handler may
> require
>          *
>          * This function shall be called when the camera is configured to
> inform
>          * the IPA of the camera's streams and the sensor settings.
> @@ -71,7 +76,7 @@ interface IPARPiInterface {
>                   map<uint32, libcamera.IPAStream> streamConfig,
>                   map<uint32, libcamera.ControlInfoMap> entityControls,
>                   IPAConfig ipaConfig)
> -               => (int32 ret, libcamera.ControlList controls);
> +               => (int32 ret, libcamera.ControlList controls,
> IPAConfigResult result);
>
>         /**
>          * \fn mapBuffers()
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp
> b/src/ipa/raspberrypi/raspberrypi.cpp
> index 047123ab..796e6d15 100644
> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> @@ -97,7 +97,7 @@ public:
>                       const std::map<unsigned int, IPAStream>
> &streamConfig,
>                       const std::map<unsigned int, ControlInfoMap>
> &entityControls,
>                       const ipa::RPi::IPAConfig &data,
> -                     ControlList *controls) override;
> +                     ControlList *controls, ipa::RPi::IPAConfigResult
> *result) override;
>         void mapBuffers(const std::vector<IPABuffer> &buffers) override;
>         void unmapBuffers(const std::vector<unsigned int> &ids) override;
>         void signalStatReady(const uint32_t bufferId) override;
> @@ -337,7 +337,7 @@ int IPARPi::configure(const IPACameraSensorInfo
> &sensorInfo,
>                       [[maybe_unused]] const std::map<unsigned int,
> IPAStream> &streamConfig,
>                       const std::map<unsigned int, ControlInfoMap>
> &entityControls,
>                       const ipa::RPi::IPAConfig &ipaConfig,
> -                     ControlList *controls)
> +                     ControlList *controls, ipa::RPi::IPAConfigResult
> *result)
>  {
>         if (entityControls.size() != 2) {
>                 LOG(IPARPI, Error) << "No ISP or sensor controls found.";
> @@ -389,6 +389,9 @@ int IPARPi::configure(const IPACameraSensorInfo
> &sensorInfo,
>         /* Pass the camera mode to the CamHelper to setup algorithms. */
>         helper_->SetCameraMode(mode_);
>
> +       /* The pipeline handler passes out the mode's sensitivity. */
> +       result->modeSensitivity = mode_.sensitivity;
> +
>         if (firstStart_) {
>                 /* Supply initial values for frame durations. */
>                 applyFrameDurations(defaultMinFrameDuration,
> defaultMaxFrameDuration);
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index 0bdfa727..caf0030e 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -147,7 +147,7 @@ public:
>         void frameStarted(uint32_t sequence);
>
>         int loadIPA(ipa::RPi::SensorConfig *sensorConfig);
> -       int configureIPA(const CameraConfiguration *config);
> +       int configureIPA(CameraConfiguration *config);
>
>         void statsMetadataComplete(uint32_t bufferId, const ControlList
> &controls);
>         void runIsp(uint32_t bufferId);
> @@ -1250,7 +1250,7 @@ int RPiCameraData::loadIPA(ipa::RPi::SensorConfig
> *sensorConfig)
>         return ipa_->init(settings, sensorConfig);
>  }
>
> -int RPiCameraData::configureIPA(const CameraConfiguration *config)
> +int RPiCameraData::configureIPA(CameraConfiguration *config)
>  {
>         /* We know config must be an RPiCameraConfiguration. */
>         const RPiCameraConfiguration *rpiConfig =
> @@ -1299,13 +1299,17 @@ int RPiCameraData::configureIPA(const
> CameraConfiguration *config)
>
>         /* Ready the IPA - it must know about the sensor resolution. */
>         ControlList controls;
> +       ipa::RPi::IPAConfigResult result;
>         ret = ipa_->configure(sensorInfo_, streamConfig, entityControls,
> ipaConfig,
> -                             &controls);
> +                             &controls, &result);
>         if (ret < 0) {
>                 LOG(RPI, Error) << "IPA configuration failed!";
>                 return -EPIPE;
>         }
>
> +       /* Store the mode sensitivity for the application. */
> +       config->modeSensitivity = result.modeSensitivity;
> +
>         /*
>          * Configure the H/V flip controls based on the combination of
>          * the sensor and user transform.
> --
> 2.20.1
>
>
Kieran Bingham April 13, 2022, 9:38 a.m. UTC | #7 
Hi David, Naush,

Quoting David Plowman (2021-09-22 14:29:15)
> These changes retrieve the correct value for sensitivity of the mode
> selected for the sensor. This value is known to the CamHelper which
> passes it across to the pipeline handler so that it can be set
> correctly in the CameraConfiguration.
> 
> Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
> ---
>  include/libcamera/ipa/raspberrypi.mojom            |  7 ++++++-
>  src/ipa/raspberrypi/raspberrypi.cpp                |  7 +++++--
>  src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 10 +++++++---
>  3 files changed, 18 insertions(+), 6 deletions(-)
> 
> diff --git a/include/libcamera/ipa/raspberrypi.mojom b/include/libcamera/ipa/raspberrypi.mojom
> index e453d46c..a92a76f8 100644
> --- a/include/libcamera/ipa/raspberrypi.mojom
> +++ b/include/libcamera/ipa/raspberrypi.mojom
> @@ -38,6 +38,10 @@ struct IPAConfig {
>         libcamera.FileDescriptor lsTableHandle;
>  };
>  
> +struct IPAConfigResult {
> +       float modeSensitivity;
> +};
> +
>  struct StartConfig {
>         libcamera.ControlList controls;
>         int32 dropFrameCount;
> @@ -57,6 +61,7 @@ interface IPARPiInterface {
>          * \param[in] entityControls Controls provided by the pipeline entities
>          * \param[in] ipaConfig Pipeline-handler-specific configuration data
>          * \param[out] controls Controls to apply by the pipeline entity
> +        * \param[out] result Other results that the pipeline handler may require
>          *
>          * This function shall be called when the camera is configured to inform
>          * the IPA of the camera's streams and the sensor settings.
> @@ -71,7 +76,7 @@ interface IPARPiInterface {
>                   map<uint32, libcamera.IPAStream> streamConfig,
>                   map<uint32, libcamera.ControlInfoMap> entityControls,
>                   IPAConfig ipaConfig)
> -               => (int32 ret, libcamera.ControlList controls);
> +               => (int32 ret, libcamera.ControlList controls, IPAConfigResult result);
>  
>         /**
>          * \fn mapBuffers()
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> index 047123ab..796e6d15 100644
> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> @@ -97,7 +97,7 @@ public:
>                       const std::map<unsigned int, IPAStream> &streamConfig,
>                       const std::map<unsigned int, ControlInfoMap> &entityControls,
>                       const ipa::RPi::IPAConfig &data,
> -                     ControlList *controls) override;
> +                     ControlList *controls, ipa::RPi::IPAConfigResult *result) override;
>         void mapBuffers(const std::vector<IPABuffer> &buffers) override;
>         void unmapBuffers(const std::vector<unsigned int> &ids) override;
>         void signalStatReady(const uint32_t bufferId) override;
> @@ -337,7 +337,7 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo,
>                       [[maybe_unused]] const std::map<unsigned int, IPAStream> &streamConfig,
>                       const std::map<unsigned int, ControlInfoMap> &entityControls,
>                       const ipa::RPi::IPAConfig &ipaConfig,
> -                     ControlList *controls)
> +                     ControlList *controls, ipa::RPi::IPAConfigResult *result)
>  {
>         if (entityControls.size() != 2) {
>                 LOG(IPARPI, Error) << "No ISP or sensor controls found.";
> @@ -389,6 +389,9 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo,
>         /* Pass the camera mode to the CamHelper to setup algorithms. */
>         helper_->SetCameraMode(mode_);
>  
> +       /* The pipeline handler passes out the mode's sensitivity. */
> +       result->modeSensitivity = mode_.sensitivity;
> +
>         if (firstStart_) {
>                 /* Supply initial values for frame durations. */
>                 applyFrameDurations(defaultMinFrameDuration, defaultMaxFrameDuration);
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index 0bdfa727..caf0030e 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -147,7 +147,7 @@ public:
>         void frameStarted(uint32_t sequence);
>  
>         int loadIPA(ipa::RPi::SensorConfig *sensorConfig);
> -       int configureIPA(const CameraConfiguration *config);
> +       int configureIPA(CameraConfiguration *config);
>  
>         void statsMetadataComplete(uint32_t bufferId, const ControlList &controls);
>         void runIsp(uint32_t bufferId);
> @@ -1250,7 +1250,7 @@ int RPiCameraData::loadIPA(ipa::RPi::SensorConfig *sensorConfig)
>         return ipa_->init(settings, sensorConfig);
>  }
>  
> -int RPiCameraData::configureIPA(const CameraConfiguration *config)
> +int RPiCameraData::configureIPA(CameraConfiguration *config)
>  {
>         /* We know config must be an RPiCameraConfiguration. */
>         const RPiCameraConfiguration *rpiConfig =
> @@ -1299,13 +1299,17 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config)
>  
>         /* Ready the IPA - it must know about the sensor resolution. */
>         ControlList controls;
> +       ipa::RPi::IPAConfigResult result;
>         ret = ipa_->configure(sensorInfo_, streamConfig, entityControls, ipaConfig,
> -                             &controls);
> +                             &controls, &result);

I've just rebased this series to master to facilitate merging, and with
fresh eyes I can't help but wonder if this value shouldn't be returned
in the validate() phase. (Not sure if this has been asked before / yet).

Is there anything that prevents us adding a validate() to the IPA
interface to allow validating the configuration and at that point,
setting the mode sensitivity? Or can this value /only/ be determined
when configuring?

Then the validate phase should be able to return the mode sensitivity of
the configuration that is being validated.

Doing that we could keep the configure calls using a const parameter for
their config structures.

>         if (ret < 0) {
>                 LOG(RPI, Error) << "IPA configuration failed!";
>                 return -EPIPE;
>         }
>  
> +       /* Store the mode sensitivity for the application. */
> +       config->modeSensitivity = result.modeSensitivity;
> +
>         /*
>          * Configure the H/V flip controls based on the combination of
>          * the sensor and user transform.
> -- 
> 2.20.1
>
Kieran Bingham April 13, 2022, 9:39 a.m. UTC | #8 
Quoting Kieran Bingham (2022-02-09 13:17:42)
> Quoting David Plowman (2022-02-09 13:00:57)
> > Hi Kieran
> > 
> > On Wed, 9 Feb 2022 at 12:27, Kieran Bingham
> > <kieran.bingham@ideasonboard.com> wrote:
> > >
> > > Quoting David Plowman (2022-01-04 13:33:05)
> > > > Hi again
> > > >
> > > > On Tue, 4 Jan 2022 at 11:13, Kieran Bingham
> > > > <kieran.bingham@ideasonboard.com> wrote:
> > > > >
> > > > > Quoting David Plowman (2021-09-22 14:29:15)
> > > > > > These changes retrieve the correct value for sensitivity of the mode
> > > > > > selected for the sensor. This value is known to the CamHelper which
> > > > > > passes it across to the pipeline handler so that it can be set
> > > > > > correctly in the CameraConfiguration.
> > > > > >
> > > > > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
> > > > > > ---
> > > > > >  include/libcamera/ipa/raspberrypi.mojom            |  7 ++++++-
> > > > > >  src/ipa/raspberrypi/raspberrypi.cpp                |  7 +++++--
> > > > > >  src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 10 +++++++---
> > > > > >  3 files changed, 18 insertions(+), 6 deletions(-)
> > > > > >
> > > > > > diff --git a/include/libcamera/ipa/raspberrypi.mojom b/include/libcamera/ipa/raspberrypi.mojom
> > > > > > index e453d46c..a92a76f8 100644
> > > > > > --- a/include/libcamera/ipa/raspberrypi.mojom
> > > > > > +++ b/include/libcamera/ipa/raspberrypi.mojom
> > > > > > @@ -38,6 +38,10 @@ struct IPAConfig {
> > > > > >         libcamera.FileDescriptor lsTableHandle;
> > > > > >  };
> > > > > >
> > > > > > +struct IPAConfigResult {
> > > > > > +       float modeSensitivity;
> > > > > > +};
> > > > > > +
> > > > > >  struct StartConfig {
> > > > > >         libcamera.ControlList controls;
> > > > > >         int32 dropFrameCount;
> > > > > > @@ -57,6 +61,7 @@ interface IPARPiInterface {
> > > > > >          * \param[in] entityControls Controls provided by the pipeline entities
> > > > > >          * \param[in] ipaConfig Pipeline-handler-specific configuration data
> > > > > >          * \param[out] controls Controls to apply by the pipeline entity
> > > > > > +        * \param[out] result Other results that the pipeline handler may require
> > > > > >          *
> > > > > >          * This function shall be called when the camera is configured to inform
> > > > > >          * the IPA of the camera's streams and the sensor settings.
> > > > > > @@ -71,7 +76,7 @@ interface IPARPiInterface {
> > > > > >                   map<uint32, libcamera.IPAStream> streamConfig,
> > > > > >                   map<uint32, libcamera.ControlInfoMap> entityControls,
> > > > > >                   IPAConfig ipaConfig)
> > > > > > -               => (int32 ret, libcamera.ControlList controls);
> > > > > > +               => (int32 ret, libcamera.ControlList controls, IPAConfigResult result);
> > > > > >
> > > > > >         /**
> > > > > >          * \fn mapBuffers()
> > > > > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> > > > > > index 047123ab..796e6d15 100644
> > > > > > --- a/src/ipa/raspberrypi/raspberrypi.cpp
> > > > > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> > > > > > @@ -97,7 +97,7 @@ public:
> > > > > >                       const std::map<unsigned int, IPAStream> &streamConfig,
> > > > > >                       const std::map<unsigned int, ControlInfoMap> &entityControls,
> > > > > >                       const ipa::RPi::IPAConfig &data,
> > > > > > -                     ControlList *controls) override;
> > > > > > +                     ControlList *controls, ipa::RPi::IPAConfigResult *result) override;
> > > > > >         void mapBuffers(const std::vector<IPABuffer> &buffers) override;
> > > > > >         void unmapBuffers(const std::vector<unsigned int> &ids) override;
> > > > > >         void signalStatReady(const uint32_t bufferId) override;
> > > > > > @@ -337,7 +337,7 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo,
> > > > > >                       [[maybe_unused]] const std::map<unsigned int, IPAStream> &streamConfig,
> > > > > >                       const std::map<unsigned int, ControlInfoMap> &entityControls,
> > > > > >                       const ipa::RPi::IPAConfig &ipaConfig,
> > > > > > -                     ControlList *controls)
> > > > > > +                     ControlList *controls, ipa::RPi::IPAConfigResult *result)
> > > > > >  {
> > > > > >         if (entityControls.size() != 2) {
> > > > > >                 LOG(IPARPI, Error) << "No ISP or sensor controls found.";
> > > > > > @@ -389,6 +389,9 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo,
> > > > > >         /* Pass the camera mode to the CamHelper to setup algorithms. */
> > > > > >         helper_->SetCameraMode(mode_);
> > > > > >
> > > > > > +       /* The pipeline handler passes out the mode's sensitivity. */
> > > > > > +       result->modeSensitivity = mode_.sensitivity;
> > > > > > +
> > > > > >         if (firstStart_) {
> > > > > >                 /* Supply initial values for frame durations. */
> > > > > >                 applyFrameDurations(defaultMinFrameDuration, defaultMaxFrameDuration);
> > > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > > > > index 0bdfa727..caf0030e 100644
> > > > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > > > > @@ -147,7 +147,7 @@ public:
> > > > > >         void frameStarted(uint32_t sequence);
> > > > > >
> > > > > >         int loadIPA(ipa::RPi::SensorConfig *sensorConfig);
> > > > > > -       int configureIPA(const CameraConfiguration *config);
> > > > > > +       int configureIPA(CameraConfiguration *config);
> > > > > >
> > > > > >         void statsMetadataComplete(uint32_t bufferId, const ControlList &controls);
> > > > > >         void runIsp(uint32_t bufferId);
> > > > > > @@ -1250,7 +1250,7 @@ int RPiCameraData::loadIPA(ipa::RPi::SensorConfig *sensorConfig)
> > > > > >         return ipa_->init(settings, sensorConfig);
> > > > > >  }
> > > > > >
> > > > > > -int RPiCameraData::configureIPA(const CameraConfiguration *config)
> > > > > > +int RPiCameraData::configureIPA(CameraConfiguration *config)
> > > > > >  {
> > > > > >         /* We know config must be an RPiCameraConfiguration. */
> > > > > >         const RPiCameraConfiguration *rpiConfig =
> > > > > > @@ -1299,13 +1299,17 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config)
> > > > > >
> > > > > >         /* Ready the IPA - it must know about the sensor resolution. */
> > > > > >         ControlList controls;
> > > > > > +       ipa::RPi::IPAConfigResult result;
> > > > > >         ret = ipa_->configure(sensorInfo_, streamConfig, entityControls, ipaConfig,
> > > > > > -                             &controls);
> > > > > > +                             &controls, &result);
> > > > >
> > > > > This should probably be being passed back as a control/property here.
> > > > > But I think right now the &controls is a set of V4L2 Controls, and not
> > > > > libcamera controls.
> > > > >
> > > > > We've been discussing about changing that so that the IPAs pass
> > > > > libcamera::controls instead. Currently Isolated IPAs will break when we
> > > > > try to pass lens controls because that's 'yet another infomap' that will
> > > > > have to be serialised.
> > > > >
> > > > > If we instead use only libcamera controls in the interface between the
> > > > > pipeline handler and the IPA this would be resolved, and it would mean
> > > > > that list could also pass the sensitivity property.
> > > > >
> > > > > Is there any reason you're aware of as to why we could or couldn't use
> > > > > libcamera controls here? I know Laurent has been concerned in the past
> > > > > about perhaps if there was any precision issues or rounding that the IPA
> > > > > might need to know about that would otherwise occur in the pipeline
> > > > > handler. Do you see this as an issue? or would we be able to
> > > > > successfully convert the interfaces to use libcamera control lists only
> > > > > (and keep the V4L2'ness' hidden in the V4L2 layer / pipeline handler
> > > > > only).
> > > >
> > > > I can't think of any precision issues that particularly bother me
> > > > (which of course doesn't mean there aren't any). As I said in my other
> > > > reply, I do need this value to be available from the call to
> > > > configure(), before you get to call start(), so it's not clear to me
> > > > at the minute where I would read this value back - but I'm guessing
> > > > someone has some ideas?
> > >
> > > That discussion above was more about the type of the Control rather than
> > > where it gets returned.
> > >
> > > I'm confused here if the start() and configure() you are referencing is
> > > the public API, or the IPA interface API?
> > 
> > I'm referring to the public Camera API. It's applications that will
> > want to know this number.
> 
> Ok, so it can certainly be updated by the configure operation, but I
> wonder if it's something that the model would expect to be determined
> during validate()?
> 
> Configure() is supposed to be a 'non-changing' operation ... but this is
> more like a return value. Would an application expect to know the mode
> sensitivity after they've validated a configuration? (to check it is
> what they expect?) before even calling configure?

Aha - this is the question I'd like answered before merging ;-)

--
Kieran


> 
> --
> Kieran
> 
> > 
> > David
> > 
> > >
> > > --
> > > Kieran
> > >
> > >
> > > >
> > > > David
> > > >
> > > > >
> > > > > All that said, changing the controls interface there is still possible
> > > > > on top, even with this patch, so I don't object to passing it directly
> > > > > back for now.
> > > > >
> > > > >
> > > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > > > >
> > > > > --
> > > > > Kieran
> > > > >
> > > > >
> > > > > >         if (ret < 0) {
> > > > > >                 LOG(RPI, Error) << "IPA configuration failed!";
> > > > > >                 return -EPIPE;
> > > > > >         }
> > > > > >
> > > > > > +       /* Store the mode sensitivity for the application. */
> > > > > > +       config->modeSensitivity = result.modeSensitivity;
> > > > > > +
> > > > > >         /*
> > > > > >          * Configure the H/V flip controls based on the combination of
> > > > > >          * the sensor and user transform.
> > > > > > --
> > > > > > 2.20.1
> > > > > >
Naushir Patuck April 13, 2022, 10:14 a.m. UTC | #9 
Hi Kieran,

On Wed, 13 Apr 2022 at 10:38, Kieran Bingham <
kieran.bingham@ideasonboard.com> wrote:

> Hi David, Naush,
>
> Quoting David Plowman (2021-09-22 14:29:15)
> > These changes retrieve the correct value for sensitivity of the mode
> > selected for the sensor. This value is known to the CamHelper which
> > passes it across to the pipeline handler so that it can be set
> > correctly in the CameraConfiguration.
> >
> > Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
> > ---
> >  include/libcamera/ipa/raspberrypi.mojom            |  7 ++++++-
> >  src/ipa/raspberrypi/raspberrypi.cpp                |  7 +++++--
> >  src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 10 +++++++---
> >  3 files changed, 18 insertions(+), 6 deletions(-)
> >
> > diff --git a/include/libcamera/ipa/raspberrypi.mojom
> b/include/libcamera/ipa/raspberrypi.mojom
> > index e453d46c..a92a76f8 100644
> > --- a/include/libcamera/ipa/raspberrypi.mojom
> > +++ b/include/libcamera/ipa/raspberrypi.mojom
> > @@ -38,6 +38,10 @@ struct IPAConfig {
> >         libcamera.FileDescriptor lsTableHandle;
> >  };
> >
> > +struct IPAConfigResult {
> > +       float modeSensitivity;
> > +};
> > +
> >  struct StartConfig {
> >         libcamera.ControlList controls;
> >         int32 dropFrameCount;
> > @@ -57,6 +61,7 @@ interface IPARPiInterface {
> >          * \param[in] entityControls Controls provided by the pipeline
> entities
> >          * \param[in] ipaConfig Pipeline-handler-specific configuration
> data
> >          * \param[out] controls Controls to apply by the pipeline entity
> > +        * \param[out] result Other results that the pipeline handler
> may require
> >          *
> >          * This function shall be called when the camera is configured
> to inform
> >          * the IPA of the camera's streams and the sensor settings.
> > @@ -71,7 +76,7 @@ interface IPARPiInterface {
> >                   map<uint32, libcamera.IPAStream> streamConfig,
> >                   map<uint32, libcamera.ControlInfoMap> entityControls,
> >                   IPAConfig ipaConfig)
> > -               => (int32 ret, libcamera.ControlList controls);
> > +               => (int32 ret, libcamera.ControlList controls,
> IPAConfigResult result);
> >
> >         /**
> >          * \fn mapBuffers()
> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp
> b/src/ipa/raspberrypi/raspberrypi.cpp
> > index 047123ab..796e6d15 100644
> > --- a/src/ipa/raspberrypi/raspberrypi.cpp
> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> > @@ -97,7 +97,7 @@ public:
> >                       const std::map<unsigned int, IPAStream>
> &streamConfig,
> >                       const std::map<unsigned int, ControlInfoMap>
> &entityControls,
> >                       const ipa::RPi::IPAConfig &data,
> > -                     ControlList *controls) override;
> > +                     ControlList *controls, ipa::RPi::IPAConfigResult
> *result) override;
> >         void mapBuffers(const std::vector<IPABuffer> &buffers) override;
> >         void unmapBuffers(const std::vector<unsigned int> &ids) override;
> >         void signalStatReady(const uint32_t bufferId) override;
> > @@ -337,7 +337,7 @@ int IPARPi::configure(const IPACameraSensorInfo
> &sensorInfo,
> >                       [[maybe_unused]] const std::map<unsigned int,
> IPAStream> &streamConfig,
> >                       const std::map<unsigned int, ControlInfoMap>
> &entityControls,
> >                       const ipa::RPi::IPAConfig &ipaConfig,
> > -                     ControlList *controls)
> > +                     ControlList *controls, ipa::RPi::IPAConfigResult
> *result)
> >  {
> >         if (entityControls.size() != 2) {
> >                 LOG(IPARPI, Error) << "No ISP or sensor controls found.";
> > @@ -389,6 +389,9 @@ int IPARPi::configure(const IPACameraSensorInfo
> &sensorInfo,
> >         /* Pass the camera mode to the CamHelper to setup algorithms. */
> >         helper_->SetCameraMode(mode_);
> >
> > +       /* The pipeline handler passes out the mode's sensitivity. */
> > +       result->modeSensitivity = mode_.sensitivity;
> > +
> >         if (firstStart_) {
> >                 /* Supply initial values for frame durations. */
> >                 applyFrameDurations(defaultMinFrameDuration,
> defaultMaxFrameDuration);
> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > index 0bdfa727..caf0030e 100644
> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > @@ -147,7 +147,7 @@ public:
> >         void frameStarted(uint32_t sequence);
> >
> >         int loadIPA(ipa::RPi::SensorConfig *sensorConfig);
> > -       int configureIPA(const CameraConfiguration *config);
> > +       int configureIPA(CameraConfiguration *config);
> >
> >         void statsMetadataComplete(uint32_t bufferId, const ControlList
> &controls);
> >         void runIsp(uint32_t bufferId);
> > @@ -1250,7 +1250,7 @@ int RPiCameraData::loadIPA(ipa::RPi::SensorConfig
> *sensorConfig)
> >         return ipa_->init(settings, sensorConfig);
> >  }
> >
> > -int RPiCameraData::configureIPA(const CameraConfiguration *config)
> > +int RPiCameraData::configureIPA(CameraConfiguration *config)
> >  {
> >         /* We know config must be an RPiCameraConfiguration. */
> >         const RPiCameraConfiguration *rpiConfig =
> > @@ -1299,13 +1299,17 @@ int RPiCameraData::configureIPA(const
> CameraConfiguration *config)
> >
> >         /* Ready the IPA - it must know about the sensor resolution. */
> >         ControlList controls;
> > +       ipa::RPi::IPAConfigResult result;
> >         ret = ipa_->configure(sensorInfo_, streamConfig, entityControls,
> ipaConfig,
> > -                             &controls);
> > +                             &controls, &result);
>
> I've just rebased this series to master to facilitate merging, and with
> fresh eyes I can't help but wonder if this value shouldn't be returned
> in the validate() phase. (Not sure if this has been asked before / yet).
>
> Is there anything that prevents us adding a validate() to the IPA
> interface to allow validating the configuration and at that point,
> setting the mode sensitivity? Or can this value /only/ be determined
> when configuring?
>

I can't think of any reason that this couldn't be done in validate(), but
David
might have some reasons.  However, is an application required to call
CameraConfiguration::validate() directly?  I can see that it does get called
in Camera::configure(), just before the call to
PipelineHandler::configure().
If there is no requirement to call CameraConfiguration::validate(), would it
matter where the result gets set?

Regards,
Naush


>
> Then the validate phase should be able to return the mode sensitivity of
> the configuration that is being validated.
>
> Doing that we could keep the configure calls using a const parameter for
> their config structures.
>
> >         if (ret < 0) {
> >                 LOG(RPI, Error) << "IPA configuration failed!";
> >                 return -EPIPE;
> >         }
> >
> > +       /* Store the mode sensitivity for the application. */
> > +       config->modeSensitivity = result.modeSensitivity;
> > +
> >         /*
> >          * Configure the H/V flip controls based on the combination of
> >          * the sensor and user transform.
> > --
> > 2.20.1
> >
>
Kieran Bingham April 13, 2022, 10:20 a.m. UTC | #10 
Quoting Naushir Patuck (2022-04-13 11:14:01)
> Hi Kieran,
> 
> On Wed, 13 Apr 2022 at 10:38, Kieran Bingham <
> kieran.bingham@ideasonboard.com> wrote:
> 
> > Hi David, Naush,
> >
> > Quoting David Plowman (2021-09-22 14:29:15)
> > > These changes retrieve the correct value for sensitivity of the mode
> > > selected for the sensor. This value is known to the CamHelper which
> > > passes it across to the pipeline handler so that it can be set
> > > correctly in the CameraConfiguration.
> > >
> > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
> > > ---
> > >  include/libcamera/ipa/raspberrypi.mojom            |  7 ++++++-
> > >  src/ipa/raspberrypi/raspberrypi.cpp                |  7 +++++--
> > >  src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 10 +++++++---
> > >  3 files changed, 18 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/include/libcamera/ipa/raspberrypi.mojom
> > b/include/libcamera/ipa/raspberrypi.mojom
> > > index e453d46c..a92a76f8 100644
> > > --- a/include/libcamera/ipa/raspberrypi.mojom
> > > +++ b/include/libcamera/ipa/raspberrypi.mojom
> > > @@ -38,6 +38,10 @@ struct IPAConfig {
> > >         libcamera.FileDescriptor lsTableHandle;
> > >  };
> > >
> > > +struct IPAConfigResult {
> > > +       float modeSensitivity;
> > > +};
> > > +
> > >  struct StartConfig {
> > >         libcamera.ControlList controls;
> > >         int32 dropFrameCount;
> > > @@ -57,6 +61,7 @@ interface IPARPiInterface {
> > >          * \param[in] entityControls Controls provided by the pipeline
> > entities
> > >          * \param[in] ipaConfig Pipeline-handler-specific configuration
> > data
> > >          * \param[out] controls Controls to apply by the pipeline entity
> > > +        * \param[out] result Other results that the pipeline handler
> > may require
> > >          *
> > >          * This function shall be called when the camera is configured
> > to inform
> > >          * the IPA of the camera's streams and the sensor settings.
> > > @@ -71,7 +76,7 @@ interface IPARPiInterface {
> > >                   map<uint32, libcamera.IPAStream> streamConfig,
> > >                   map<uint32, libcamera.ControlInfoMap> entityControls,
> > >                   IPAConfig ipaConfig)
> > > -               => (int32 ret, libcamera.ControlList controls);
> > > +               => (int32 ret, libcamera.ControlList controls,
> > IPAConfigResult result);
> > >
> > >         /**
> > >          * \fn mapBuffers()
> > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp
> > b/src/ipa/raspberrypi/raspberrypi.cpp
> > > index 047123ab..796e6d15 100644
> > > --- a/src/ipa/raspberrypi/raspberrypi.cpp
> > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> > > @@ -97,7 +97,7 @@ public:
> > >                       const std::map<unsigned int, IPAStream>
> > &streamConfig,
> > >                       const std::map<unsigned int, ControlInfoMap>
> > &entityControls,
> > >                       const ipa::RPi::IPAConfig &data,
> > > -                     ControlList *controls) override;
> > > +                     ControlList *controls, ipa::RPi::IPAConfigResult
> > *result) override;
> > >         void mapBuffers(const std::vector<IPABuffer> &buffers) override;
> > >         void unmapBuffers(const std::vector<unsigned int> &ids) override;
> > >         void signalStatReady(const uint32_t bufferId) override;
> > > @@ -337,7 +337,7 @@ int IPARPi::configure(const IPACameraSensorInfo
> > &sensorInfo,
> > >                       [[maybe_unused]] const std::map<unsigned int,
> > IPAStream> &streamConfig,
> > >                       const std::map<unsigned int, ControlInfoMap>
> > &entityControls,
> > >                       const ipa::RPi::IPAConfig &ipaConfig,
> > > -                     ControlList *controls)
> > > +                     ControlList *controls, ipa::RPi::IPAConfigResult
> > *result)
> > >  {
> > >         if (entityControls.size() != 2) {
> > >                 LOG(IPARPI, Error) << "No ISP or sensor controls found.";
> > > @@ -389,6 +389,9 @@ int IPARPi::configure(const IPACameraSensorInfo
> > &sensorInfo,
> > >         /* Pass the camera mode to the CamHelper to setup algorithms. */
> > >         helper_->SetCameraMode(mode_);
> > >
> > > +       /* The pipeline handler passes out the mode's sensitivity. */
> > > +       result->modeSensitivity = mode_.sensitivity;
> > > +
> > >         if (firstStart_) {
> > >                 /* Supply initial values for frame durations. */
> > >                 applyFrameDurations(defaultMinFrameDuration,
> > defaultMaxFrameDuration);
> > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > index 0bdfa727..caf0030e 100644
> > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > @@ -147,7 +147,7 @@ public:
> > >         void frameStarted(uint32_t sequence);
> > >
> > >         int loadIPA(ipa::RPi::SensorConfig *sensorConfig);
> > > -       int configureIPA(const CameraConfiguration *config);
> > > +       int configureIPA(CameraConfiguration *config);
> > >
> > >         void statsMetadataComplete(uint32_t bufferId, const ControlList
> > &controls);
> > >         void runIsp(uint32_t bufferId);
> > > @@ -1250,7 +1250,7 @@ int RPiCameraData::loadIPA(ipa::RPi::SensorConfig
> > *sensorConfig)
> > >         return ipa_->init(settings, sensorConfig);
> > >  }
> > >
> > > -int RPiCameraData::configureIPA(const CameraConfiguration *config)
> > > +int RPiCameraData::configureIPA(CameraConfiguration *config)
> > >  {
> > >         /* We know config must be an RPiCameraConfiguration. */
> > >         const RPiCameraConfiguration *rpiConfig =
> > > @@ -1299,13 +1299,17 @@ int RPiCameraData::configureIPA(const
> > CameraConfiguration *config)
> > >
> > >         /* Ready the IPA - it must know about the sensor resolution. */
> > >         ControlList controls;
> > > +       ipa::RPi::IPAConfigResult result;
> > >         ret = ipa_->configure(sensorInfo_, streamConfig, entityControls,
> > ipaConfig,
> > > -                             &controls);
> > > +                             &controls, &result);
> >
> > I've just rebased this series to master to facilitate merging, and with
> > fresh eyes I can't help but wonder if this value shouldn't be returned
> > in the validate() phase. (Not sure if this has been asked before / yet).
> >
> > Is there anything that prevents us adding a validate() to the IPA
> > interface to allow validating the configuration and at that point,
> > setting the mode sensitivity? Or can this value /only/ be determined
> > when configuring?
> >
> 
> I can't think of any reason that this couldn't be done in validate(), but
> David
> might have some reasons.  However, is an application required to call
> CameraConfiguration::validate() directly?  I can see that it does get called
> in Camera::configure(), just before the call to
> PipelineHandler::configure().
> If there is no requirement to call CameraConfiguration::validate(), would it
> matter where the result gets set?

There isn't a 'requirement' to do so as such, but it's recommended. If
the call to configure can't be satisfied 'precisely' then the configure
call should fail. It should not make any changes or adjustments to the
configuration.

Validate will tell you if the pipeline handler made any changes, and
more than that - the validate before configure /must/ report that no
changes were made.

When an application calls validate() - the pipeline handler can make
changes to the Configuration (such as filling in the ModeSensitivity
here) and if a change was made - it can return 'ConfigurationAdjusted' -
which then means the application should check to see what was different
from what it requested - to what the pipeline handler will deliver.

I would like there to be a way to highlight 'what' changes were made if
the validate returns 'Adjusted' - but I'm not sure how we could easily
implement that yet.

--
Kieran


> Regards,
> Naush
> 
> 
> >
> > Then the validate phase should be able to return the mode sensitivity of
> > the configuration that is being validated.
> >
> > Doing that we could keep the configure calls using a const parameter for
> > their config structures.
> >
> > >         if (ret < 0) {
> > >                 LOG(RPI, Error) << "IPA configuration failed!";
> > >                 return -EPIPE;
> > >         }
> > >
> > > +       /* Store the mode sensitivity for the application. */
> > > +       config->modeSensitivity = result.modeSensitivity;
> > > +
> > >         /*
> > >          * Configure the H/V flip controls based on the combination of
> > >          * the sensor and user transform.
> > > --
> > > 2.20.1
> > >
> >
Kieran Bingham April 13, 2022, 10:59 a.m. UTC | #11 
Quoting Naushir Patuck (2022-04-13 11:44:14)
> Hi Kieran,
> 
> On Wed, 13 Apr 2022 at 11:20, Kieran Bingham <
> kieran.bingham@ideasonboard.com> wrote:
> 
> > Quoting Naushir Patuck (2022-04-13 11:14:01)
> > > Hi Kieran,
> > >
> > > On Wed, 13 Apr 2022 at 10:38, Kieran Bingham <
> > > kieran.bingham@ideasonboard.com> wrote:
> > >
> > > > Hi David, Naush,
> > > >
> > > > Quoting David Plowman (2021-09-22 14:29:15)
> > > > > These changes retrieve the correct value for sensitivity of the mode
> > > > > selected for the sensor. This value is known to the CamHelper which
> > > > > passes it across to the pipeline handler so that it can be set
> > > > > correctly in the CameraConfiguration.
> > > > >
> > > > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
> > > > > ---
> > > > >  include/libcamera/ipa/raspberrypi.mojom            |  7 ++++++-
> > > > >  src/ipa/raspberrypi/raspberrypi.cpp                |  7 +++++--
> > > > >  src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 10 +++++++---
> > > > >  3 files changed, 18 insertions(+), 6 deletions(-)
> > > > >
> > > > > diff --git a/include/libcamera/ipa/raspberrypi.mojom
> > > > b/include/libcamera/ipa/raspberrypi.mojom
> > > > > index e453d46c..a92a76f8 100644
> > > > > --- a/include/libcamera/ipa/raspberrypi.mojom
> > > > > +++ b/include/libcamera/ipa/raspberrypi.mojom
> > > > > @@ -38,6 +38,10 @@ struct IPAConfig {
> > > > >         libcamera.FileDescriptor lsTableHandle;
> > > > >  };
> > > > >
> > > > > +struct IPAConfigResult {
> > > > > +       float modeSensitivity;
> > > > > +};
> > > > > +
> > > > >  struct StartConfig {
> > > > >         libcamera.ControlList controls;
> > > > >         int32 dropFrameCount;
> > > > > @@ -57,6 +61,7 @@ interface IPARPiInterface {
> > > > >          * \param[in] entityControls Controls provided by the
> > pipeline
> > > > entities
> > > > >          * \param[in] ipaConfig Pipeline-handler-specific
> > configuration
> > > > data
> > > > >          * \param[out] controls Controls to apply by the pipeline
> > entity
> > > > > +        * \param[out] result Other results that the pipeline handler
> > > > may require
> > > > >          *
> > > > >          * This function shall be called when the camera is
> > configured
> > > > to inform
> > > > >          * the IPA of the camera's streams and the sensor settings.
> > > > > @@ -71,7 +76,7 @@ interface IPARPiInterface {
> > > > >                   map<uint32, libcamera.IPAStream> streamConfig,
> > > > >                   map<uint32, libcamera.ControlInfoMap>
> > entityControls,
> > > > >                   IPAConfig ipaConfig)
> > > > > -               => (int32 ret, libcamera.ControlList controls);
> > > > > +               => (int32 ret, libcamera.ControlList controls,
> > > > IPAConfigResult result);
> > > > >
> > > > >         /**
> > > > >          * \fn mapBuffers()
> > > > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp
> > > > b/src/ipa/raspberrypi/raspberrypi.cpp
> > > > > index 047123ab..796e6d15 100644
> > > > > --- a/src/ipa/raspberrypi/raspberrypi.cpp
> > > > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> > > > > @@ -97,7 +97,7 @@ public:
> > > > >                       const std::map<unsigned int, IPAStream>
> > > > &streamConfig,
> > > > >                       const std::map<unsigned int, ControlInfoMap>
> > > > &entityControls,
> > > > >                       const ipa::RPi::IPAConfig &data,
> > > > > -                     ControlList *controls) override;
> > > > > +                     ControlList *controls,
> > ipa::RPi::IPAConfigResult
> > > > *result) override;
> > > > >         void mapBuffers(const std::vector<IPABuffer> &buffers)
> > override;
> > > > >         void unmapBuffers(const std::vector<unsigned int> &ids)
> > override;
> > > > >         void signalStatReady(const uint32_t bufferId) override;
> > > > > @@ -337,7 +337,7 @@ int IPARPi::configure(const IPACameraSensorInfo
> > > > &sensorInfo,
> > > > >                       [[maybe_unused]] const std::map<unsigned int,
> > > > IPAStream> &streamConfig,
> > > > >                       const std::map<unsigned int, ControlInfoMap>
> > > > &entityControls,
> > > > >                       const ipa::RPi::IPAConfig &ipaConfig,
> > > > > -                     ControlList *controls)
> > > > > +                     ControlList *controls,
> > ipa::RPi::IPAConfigResult
> > > > *result)
> > > > >  {
> > > > >         if (entityControls.size() != 2) {
> > > > >                 LOG(IPARPI, Error) << "No ISP or sensor controls
> > found.";
> > > > > @@ -389,6 +389,9 @@ int IPARPi::configure(const IPACameraSensorInfo
> > > > &sensorInfo,
> > > > >         /* Pass the camera mode to the CamHelper to setup
> > algorithms. */
> > > > >         helper_->SetCameraMode(mode_);
> > > > >
> > > > > +       /* The pipeline handler passes out the mode's sensitivity. */
> > > > > +       result->modeSensitivity = mode_.sensitivity;
> > > > > +
> > > > >         if (firstStart_) {
> > > > >                 /* Supply initial values for frame durations. */
> > > > >                 applyFrameDurations(defaultMinFrameDuration,
> > > > defaultMaxFrameDuration);
> > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > > > index 0bdfa727..caf0030e 100644
> > > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > > > @@ -147,7 +147,7 @@ public:
> > > > >         void frameStarted(uint32_t sequence);
> > > > >
> > > > >         int loadIPA(ipa::RPi::SensorConfig *sensorConfig);
> > > > > -       int configureIPA(const CameraConfiguration *config);
> > > > > +       int configureIPA(CameraConfiguration *config);
> > > > >
> > > > >         void statsMetadataComplete(uint32_t bufferId, const
> > ControlList
> > > > &controls);
> > > > >         void runIsp(uint32_t bufferId);
> > > > > @@ -1250,7 +1250,7 @@ int
> > RPiCameraData::loadIPA(ipa::RPi::SensorConfig
> > > > *sensorConfig)
> > > > >         return ipa_->init(settings, sensorConfig);
> > > > >  }
> > > > >
> > > > > -int RPiCameraData::configureIPA(const CameraConfiguration *config)
> > > > > +int RPiCameraData::configureIPA(CameraConfiguration *config)
> > > > >  {
> > > > >         /* We know config must be an RPiCameraConfiguration. */
> > > > >         const RPiCameraConfiguration *rpiConfig =
> > > > > @@ -1299,13 +1299,17 @@ int RPiCameraData::configureIPA(const
> > > > CameraConfiguration *config)
> > > > >
> > > > >         /* Ready the IPA - it must know about the sensor resolution.
> > */
> > > > >         ControlList controls;
> > > > > +       ipa::RPi::IPAConfigResult result;
> > > > >         ret = ipa_->configure(sensorInfo_, streamConfig,
> > entityControls,
> > > > ipaConfig,
> > > > > -                             &controls);
> > > > > +                             &controls, &result);
> > > >
> > > > I've just rebased this series to master to facilitate merging, and with
> > > > fresh eyes I can't help but wonder if this value shouldn't be returned
> > > > in the validate() phase. (Not sure if this has been asked before /
> > yet).
> > > >
> > > > Is there anything that prevents us adding a validate() to the IPA
> > > > interface to allow validating the configuration and at that point,
> > > > setting the mode sensitivity? Or can this value /only/ be determined
> > > > when configuring?
> > > >
> > >
> > > I can't think of any reason that this couldn't be done in validate(), but
> > > David
> > > might have some reasons.  However, is an application required to call
> > > CameraConfiguration::validate() directly?  I can see that it does get
> > called
> > > in Camera::configure(), just before the call to
> > > PipelineHandler::configure().
> > > If there is no requirement to call CameraConfiguration::validate(),
> > would it
> > > matter where the result gets set?
> >
> > There isn't a 'requirement' to do so as such, but it's recommended. If
> > the call to configure can't be satisfied 'precisely' then the configure
> > call should fail. It should not make any changes or adjustments to the
> > configuration.
> >
> > Validate will tell you if the pipeline handler made any changes, and
> > more than that - the validate before configure /must/ report that no
> > changes were made.
> >
> 
> I think this is the part that may be troublesome.  Assume an application
> did not call CameraConfiguration::validate() directly, but it does get
> called
> from PipelineHandler::configure(). The validate() will then adjust the
> sensitivity value and return a Status::Adjusted.  This will then fail the
> PipelineHandler::configure() call.  Perhaps CameraConfiguration::validate()
> should be a mandatory call from the application? Or if we adjust the
> sensitivity in CameraConfiguration::validate(), we don't return
> Status::Adjusted?
> 

I'm sorry - I was wrong to say there isn't a requirement to do so.

https://www.libcamera.org/api-html/classlibcamera_1_1CameraConfiguration.html#details

documents that applications 'shall' be validated by a call to
validate().

"""
The CameraConfiguration holds an ordered list of stream configurations.
It supports iterators and operates as a vector of StreamConfiguration
instances. The stream configurations are inserted by addConfiguration(),
and the at() function or operator[] return a reference to the
StreamConfiguration based on its insertion index. Accessing a stream
configuration with an invalid index results in undefined behaviour.

CameraConfiguration instances are retrieved from the camera with
Camera::generateConfiguration(). Applications may then inspect the
configuration, modify it, and possibly add new stream configuration
entries with addConfiguration(). Once the camera configuration satisfies
the application, it shall be validated by a call to validate(). The
validation implements "try" semantics: it adjusts invalid configurations
to the closest achievable parameters instead of rejecting them
completely. Applications then decide whether to accept the modified
configuration, or try again with a different set of parameters. Once the
configuration is valid, it is passed to Camera::configure().

"""

--
Kieran


> Naush
> 
> 
> >
> > When an application calls validate() - the pipeline handler can make
> > changes to the Configuration (such as filling in the ModeSensitivity
> > here) and if a change was made - it can return 'ConfigurationAdjusted' -
> > which then means the application should check to see what was different
> > from what it requested - to what the pipeline handler will deliver.
> >
> > I would like there to be a way to highlight 'what' changes were made if
> > the validate returns 'Adjusted' - but I'm not sure how we could easily
> > implement that yet.
> >
> > --
> > Kieran
> >
> >
> > > Regards,
> > > Naush
> > >
> > >
> > > >
> > > > Then the validate phase should be able to return the mode sensitivity
> > of
> > > > the configuration that is being validated.
> > > >
> > > > Doing that we could keep the configure calls using a const parameter
> > for
> > > > their config structures.
> > > >
> > > > >         if (ret < 0) {
> > > > >                 LOG(RPI, Error) << "IPA configuration failed!";
> > > > >                 return -EPIPE;
> > > > >         }
> > > > >
> > > > > +       /* Store the mode sensitivity for the application. */
> > > > > +       config->modeSensitivity = result.modeSensitivity;
> > > > > +
> > > > >         /*
> > > > >          * Configure the H/V flip controls based on the combination
> > of
> > > > >          * the sensor and user transform.
> > > > > --
> > > > > 2.20.1
> > > > >
> > > >
> >
Naushir Patuck April 13, 2022, 12:36 p.m. UTC | #12 
On Wed, 13 Apr 2022 at 11:59, Kieran Bingham <
kieran.bingham@ideasonboard.com> wrote:

> Quoting Naushir Patuck (2022-04-13 11:44:14)
> > Hi Kieran,
> >
> > On Wed, 13 Apr 2022 at 11:20, Kieran Bingham <
> > kieran.bingham@ideasonboard.com> wrote:
> >
> > > Quoting Naushir Patuck (2022-04-13 11:14:01)
> > > > Hi Kieran,
> > > >
> > > > On Wed, 13 Apr 2022 at 10:38, Kieran Bingham <
> > > > kieran.bingham@ideasonboard.com> wrote:
> > > >
> > > > > Hi David, Naush,
> > > > >
> > > > > Quoting David Plowman (2021-09-22 14:29:15)
> > > > > > These changes retrieve the correct value for sensitivity of the
> mode
> > > > > > selected for the sensor. This value is known to the CamHelper
> which
> > > > > > passes it across to the pipeline handler so that it can be set
> > > > > > correctly in the CameraConfiguration.
> > > > > >
> > > > > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
> > > > > > ---
> > > > > >  include/libcamera/ipa/raspberrypi.mojom            |  7 ++++++-
> > > > > >  src/ipa/raspberrypi/raspberrypi.cpp                |  7 +++++--
> > > > > >  src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 10
> +++++++---
> > > > > >  3 files changed, 18 insertions(+), 6 deletions(-)
> > > > > >
> > > > > > diff --git a/include/libcamera/ipa/raspberrypi.mojom
> > > > > b/include/libcamera/ipa/raspberrypi.mojom
> > > > > > index e453d46c..a92a76f8 100644
> > > > > > --- a/include/libcamera/ipa/raspberrypi.mojom
> > > > > > +++ b/include/libcamera/ipa/raspberrypi.mojom
> > > > > > @@ -38,6 +38,10 @@ struct IPAConfig {
> > > > > >         libcamera.FileDescriptor lsTableHandle;
> > > > > >  };
> > > > > >
> > > > > > +struct IPAConfigResult {
> > > > > > +       float modeSensitivity;
> > > > > > +};
> > > > > > +
> > > > > >  struct StartConfig {
> > > > > >         libcamera.ControlList controls;
> > > > > >         int32 dropFrameCount;
> > > > > > @@ -57,6 +61,7 @@ interface IPARPiInterface {
> > > > > >          * \param[in] entityControls Controls provided by the
> > > pipeline
> > > > > entities
> > > > > >          * \param[in] ipaConfig Pipeline-handler-specific
> > > configuration
> > > > > data
> > > > > >          * \param[out] controls Controls to apply by the pipeline
> > > entity
> > > > > > +        * \param[out] result Other results that the pipeline
> handler
> > > > > may require
> > > > > >          *
> > > > > >          * This function shall be called when the camera is
> > > configured
> > > > > to inform
> > > > > >          * the IPA of the camera's streams and the sensor
> settings.
> > > > > > @@ -71,7 +76,7 @@ interface IPARPiInterface {
> > > > > >                   map<uint32, libcamera.IPAStream> streamConfig,
> > > > > >                   map<uint32, libcamera.ControlInfoMap>
> > > entityControls,
> > > > > >                   IPAConfig ipaConfig)
> > > > > > -               => (int32 ret, libcamera.ControlList controls);
> > > > > > +               => (int32 ret, libcamera.ControlList controls,
> > > > > IPAConfigResult result);
> > > > > >
> > > > > >         /**
> > > > > >          * \fn mapBuffers()
> > > > > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp
> > > > > b/src/ipa/raspberrypi/raspberrypi.cpp
> > > > > > index 047123ab..796e6d15 100644
> > > > > > --- a/src/ipa/raspberrypi/raspberrypi.cpp
> > > > > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> > > > > > @@ -97,7 +97,7 @@ public:
> > > > > >                       const std::map<unsigned int, IPAStream>
> > > > > &streamConfig,
> > > > > >                       const std::map<unsigned int,
> ControlInfoMap>
> > > > > &entityControls,
> > > > > >                       const ipa::RPi::IPAConfig &data,
> > > > > > -                     ControlList *controls) override;
> > > > > > +                     ControlList *controls,
> > > ipa::RPi::IPAConfigResult
> > > > > *result) override;
> > > > > >         void mapBuffers(const std::vector<IPABuffer> &buffers)
> > > override;
> > > > > >         void unmapBuffers(const std::vector<unsigned int> &ids)
> > > override;
> > > > > >         void signalStatReady(const uint32_t bufferId) override;
> > > > > > @@ -337,7 +337,7 @@ int IPARPi::configure(const
> IPACameraSensorInfo
> > > > > &sensorInfo,
> > > > > >                       [[maybe_unused]] const std::map<unsigned
> int,
> > > > > IPAStream> &streamConfig,
> > > > > >                       const std::map<unsigned int,
> ControlInfoMap>
> > > > > &entityControls,
> > > > > >                       const ipa::RPi::IPAConfig &ipaConfig,
> > > > > > -                     ControlList *controls)
> > > > > > +                     ControlList *controls,
> > > ipa::RPi::IPAConfigResult
> > > > > *result)
> > > > > >  {
> > > > > >         if (entityControls.size() != 2) {
> > > > > >                 LOG(IPARPI, Error) << "No ISP or sensor controls
> > > found.";
> > > > > > @@ -389,6 +389,9 @@ int IPARPi::configure(const
> IPACameraSensorInfo
> > > > > &sensorInfo,
> > > > > >         /* Pass the camera mode to the CamHelper to setup
> > > algorithms. */
> > > > > >         helper_->SetCameraMode(mode_);
> > > > > >
> > > > > > +       /* The pipeline handler passes out the mode's
> sensitivity. */
> > > > > > +       result->modeSensitivity = mode_.sensitivity;
> > > > > > +
> > > > > >         if (firstStart_) {
> > > > > >                 /* Supply initial values for frame durations. */
> > > > > >                 applyFrameDurations(defaultMinFrameDuration,
> > > > > defaultMaxFrameDuration);
> > > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > > > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > > > > index 0bdfa727..caf0030e 100644
> > > > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > > > > @@ -147,7 +147,7 @@ public:
> > > > > >         void frameStarted(uint32_t sequence);
> > > > > >
> > > > > >         int loadIPA(ipa::RPi::SensorConfig *sensorConfig);
> > > > > > -       int configureIPA(const CameraConfiguration *config);
> > > > > > +       int configureIPA(CameraConfiguration *config);
> > > > > >
> > > > > >         void statsMetadataComplete(uint32_t bufferId, const
> > > ControlList
> > > > > &controls);
> > > > > >         void runIsp(uint32_t bufferId);
> > > > > > @@ -1250,7 +1250,7 @@ int
> > > RPiCameraData::loadIPA(ipa::RPi::SensorConfig
> > > > > *sensorConfig)
> > > > > >         return ipa_->init(settings, sensorConfig);
> > > > > >  }
> > > > > >
> > > > > > -int RPiCameraData::configureIPA(const CameraConfiguration
> *config)
> > > > > > +int RPiCameraData::configureIPA(CameraConfiguration *config)
> > > > > >  {
> > > > > >         /* We know config must be an RPiCameraConfiguration. */
> > > > > >         const RPiCameraConfiguration *rpiConfig =
> > > > > > @@ -1299,13 +1299,17 @@ int RPiCameraData::configureIPA(const
> > > > > CameraConfiguration *config)
> > > > > >
> > > > > >         /* Ready the IPA - it must know about the sensor
> resolution.
> > > */
> > > > > >         ControlList controls;
> > > > > > +       ipa::RPi::IPAConfigResult result;
> > > > > >         ret = ipa_->configure(sensorInfo_, streamConfig,
> > > entityControls,
> > > > > ipaConfig,
> > > > > > -                             &controls);
> > > > > > +                             &controls, &result);
> > > > >
> > > > > I've just rebased this series to master to facilitate merging, and
> with
> > > > > fresh eyes I can't help but wonder if this value shouldn't be
> returned
> > > > > in the validate() phase. (Not sure if this has been asked before /
> > > yet).
> > > > >
> > > > > Is there anything that prevents us adding a validate() to the IPA
> > > > > interface to allow validating the configuration and at that point,
> > > > > setting the mode sensitivity? Or can this value /only/ be
> determined
> > > > > when configuring?
> > > > >
> > > >
> > > > I can't think of any reason that this couldn't be done in
> validate(), but
> > > > David
> > > > might have some reasons.  However, is an application required to call
> > > > CameraConfiguration::validate() directly?  I can see that it does get
> > > called
> > > > in Camera::configure(), just before the call to
> > > > PipelineHandler::configure().
> > > > If there is no requirement to call CameraConfiguration::validate(),
> > > would it
> > > > matter where the result gets set?
> > >
> > > There isn't a 'requirement' to do so as such, but it's recommended. If
> > > the call to configure can't be satisfied 'precisely' then the configure
> > > call should fail. It should not make any changes or adjustments to the
> > > configuration.
> > >
> > > Validate will tell you if the pipeline handler made any changes, and
> > > more than that - the validate before configure /must/ report that no
> > > changes were made.
> > >
> >
> > I think this is the part that may be troublesome.  Assume an application
> > did not call CameraConfiguration::validate() directly, but it does get
> > called
> > from PipelineHandler::configure(). The validate() will then adjust the
> > sensitivity value and return a Status::Adjusted.  This will then fail the
> > PipelineHandler::configure() call.  Perhaps
> CameraConfiguration::validate()
> > should be a mandatory call from the application? Or if we adjust the
> > sensitivity in CameraConfiguration::validate(), we don't return
> > Status::Adjusted?
> >
>
> I'm sorry - I was wrong to say there isn't a requirement to do so.
>
>
> https://www.libcamera.org/api-html/classlibcamera_1_1CameraConfiguration.html#details
>
> documents that applications 'shall' be validated by a call to
> validate().
>

Ah, that makes things less ambiguous then.  I'll let David comment further
when he is back, but I see no reason not to switch this to validate() now.

Regards,
Naush


>
> """
> The CameraConfiguration holds an ordered list of stream configurations.
> It supports iterators and operates as a vector of StreamConfiguration
> instances. The stream configurations are inserted by addConfiguration(),
> and the at() function or operator[] return a reference to the
> StreamConfiguration based on its insertion index. Accessing a stream
> configuration with an invalid index results in undefined behaviour.
>
> CameraConfiguration instances are retrieved from the camera with
> Camera::generateConfiguration(). Applications may then inspect the
> configuration, modify it, and possibly add new stream configuration
> entries with addConfiguration(). Once the camera configuration satisfies
> the application, it shall be validated by a call to validate(). The
> validation implements "try" semantics: it adjusts invalid configurations
> to the closest achievable parameters instead of rejecting them
> completely. Applications then decide whether to accept the modified
> configuration, or try again with a different set of parameters. Once the
> configuration is valid, it is passed to Camera::configure().
>
> """
>
> --
> Kieran
>
>
> > Naush
> >
> >
> > >
> > > When an application calls validate() - the pipeline handler can make
> > > changes to the Configuration (such as filling in the ModeSensitivity
> > > here) and if a change was made - it can return 'ConfigurationAdjusted'
> -
> > > which then means the application should check to see what was different
> > > from what it requested - to what the pipeline handler will deliver.
> > >
> > > I would like there to be a way to highlight 'what' changes were made if
> > > the validate returns 'Adjusted' - but I'm not sure how we could easily
> > > implement that yet.
> > >
> > > --
> > > Kieran
> > >
> > >
> > > > Regards,
> > > > Naush
> > > >
> > > >
> > > > >
> > > > > Then the validate phase should be able to return the mode
> sensitivity
> > > of
> > > > > the configuration that is being validated.
> > > > >
> > > > > Doing that we could keep the configure calls using a const
> parameter
> > > for
> > > > > their config structures.
> > > > >
> > > > > >         if (ret < 0) {
> > > > > >                 LOG(RPI, Error) << "IPA configuration failed!";
> > > > > >                 return -EPIPE;
> > > > > >         }
> > > > > >
> > > > > > +       /* Store the mode sensitivity for the application. */
> > > > > > +       config->modeSensitivity = result.modeSensitivity;
> > > > > > +
> > > > > >         /*
> > > > > >          * Configure the H/V flip controls based on the
> combination
> > > of
> > > > > >          * the sensor and user transform.
> > > > > > --
> > > > > > 2.20.1
> > > > > >
> > > > >
> > >
>
Naushir Patuck April 21, 2022, 1:13 p.m. UTC | #13 
Hi Kieran,

On Wed, 13 Apr 2022 at 13:36, Naushir Patuck <naush@raspberrypi.com> wrote:

>
>
> On Wed, 13 Apr 2022 at 11:59, Kieran Bingham <
> kieran.bingham@ideasonboard.com> wrote:
>
>> Quoting Naushir Patuck (2022-04-13 11:44:14)
>> > Hi Kieran,
>> >
>> > On Wed, 13 Apr 2022 at 11:20, Kieran Bingham <
>> > kieran.bingham@ideasonboard.com> wrote:
>> >
>> > > Quoting Naushir Patuck (2022-04-13 11:14:01)
>> > > > Hi Kieran,
>> > > >
>> > > > On Wed, 13 Apr 2022 at 10:38, Kieran Bingham <
>> > > > kieran.bingham@ideasonboard.com> wrote:
>> > > >
>> > > > > Hi David, Naush,
>> > > > >
>> > > > > Quoting David Plowman (2021-09-22 14:29:15)
>> > > > > > These changes retrieve the correct value for sensitivity of the
>> mode
>> > > > > > selected for the sensor. This value is known to the CamHelper
>> which
>> > > > > > passes it across to the pipeline handler so that it can be set
>> > > > > > correctly in the CameraConfiguration.
>> > > > > >
>> > > > > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
>> > > > > > ---
>> > > > > >  include/libcamera/ipa/raspberrypi.mojom            |  7 ++++++-
>> > > > > >  src/ipa/raspberrypi/raspberrypi.cpp                |  7 +++++--
>> > > > > >  src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 10
>> +++++++---
>> > > > > >  3 files changed, 18 insertions(+), 6 deletions(-)
>> > > > > >
>> > > > > > diff --git a/include/libcamera/ipa/raspberrypi.mojom
>> > > > > b/include/libcamera/ipa/raspberrypi.mojom
>> > > > > > index e453d46c..a92a76f8 100644
>> > > > > > --- a/include/libcamera/ipa/raspberrypi.mojom
>> > > > > > +++ b/include/libcamera/ipa/raspberrypi.mojom
>> > > > > > @@ -38,6 +38,10 @@ struct IPAConfig {
>> > > > > >         libcamera.FileDescriptor lsTableHandle;
>> > > > > >  };
>> > > > > >
>> > > > > > +struct IPAConfigResult {
>> > > > > > +       float modeSensitivity;
>> > > > > > +};
>> > > > > > +
>> > > > > >  struct StartConfig {
>> > > > > >         libcamera.ControlList controls;
>> > > > > >         int32 dropFrameCount;
>> > > > > > @@ -57,6 +61,7 @@ interface IPARPiInterface {
>> > > > > >          * \param[in] entityControls Controls provided by the
>> > > pipeline
>> > > > > entities
>> > > > > >          * \param[in] ipaConfig Pipeline-handler-specific
>> > > configuration
>> > > > > data
>> > > > > >          * \param[out] controls Controls to apply by the
>> pipeline
>> > > entity
>> > > > > > +        * \param[out] result Other results that the pipeline
>> handler
>> > > > > may require
>> > > > > >          *
>> > > > > >          * This function shall be called when the camera is
>> > > configured
>> > > > > to inform
>> > > > > >          * the IPA of the camera's streams and the sensor
>> settings.
>> > > > > > @@ -71,7 +76,7 @@ interface IPARPiInterface {
>> > > > > >                   map<uint32, libcamera.IPAStream> streamConfig,
>> > > > > >                   map<uint32, libcamera.ControlInfoMap>
>> > > entityControls,
>> > > > > >                   IPAConfig ipaConfig)
>> > > > > > -               => (int32 ret, libcamera.ControlList controls);
>> > > > > > +               => (int32 ret, libcamera.ControlList controls,
>> > > > > IPAConfigResult result);
>> > > > > >
>> > > > > >         /**
>> > > > > >          * \fn mapBuffers()
>> > > > > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp
>> > > > > b/src/ipa/raspberrypi/raspberrypi.cpp
>> > > > > > index 047123ab..796e6d15 100644
>> > > > > > --- a/src/ipa/raspberrypi/raspberrypi.cpp
>> > > > > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp
>> > > > > > @@ -97,7 +97,7 @@ public:
>> > > > > >                       const std::map<unsigned int, IPAStream>
>> > > > > &streamConfig,
>> > > > > >                       const std::map<unsigned int,
>> ControlInfoMap>
>> > > > > &entityControls,
>> > > > > >                       const ipa::RPi::IPAConfig &data,
>> > > > > > -                     ControlList *controls) override;
>> > > > > > +                     ControlList *controls,
>> > > ipa::RPi::IPAConfigResult
>> > > > > *result) override;
>> > > > > >         void mapBuffers(const std::vector<IPABuffer> &buffers)
>> > > override;
>> > > > > >         void unmapBuffers(const std::vector<unsigned int> &ids)
>> > > override;
>> > > > > >         void signalStatReady(const uint32_t bufferId) override;
>> > > > > > @@ -337,7 +337,7 @@ int IPARPi::configure(const
>> IPACameraSensorInfo
>> > > > > &sensorInfo,
>> > > > > >                       [[maybe_unused]] const std::map<unsigned
>> int,
>> > > > > IPAStream> &streamConfig,
>> > > > > >                       const std::map<unsigned int,
>> ControlInfoMap>
>> > > > > &entityControls,
>> > > > > >                       const ipa::RPi::IPAConfig &ipaConfig,
>> > > > > > -                     ControlList *controls)
>> > > > > > +                     ControlList *controls,
>> > > ipa::RPi::IPAConfigResult
>> > > > > *result)
>> > > > > >  {
>> > > > > >         if (entityControls.size() != 2) {
>> > > > > >                 LOG(IPARPI, Error) << "No ISP or sensor controls
>> > > found.";
>> > > > > > @@ -389,6 +389,9 @@ int IPARPi::configure(const
>> IPACameraSensorInfo
>> > > > > &sensorInfo,
>> > > > > >         /* Pass the camera mode to the CamHelper to setup
>> > > algorithms. */
>> > > > > >         helper_->SetCameraMode(mode_);
>> > > > > >
>> > > > > > +       /* The pipeline handler passes out the mode's
>> sensitivity. */
>> > > > > > +       result->modeSensitivity = mode_.sensitivity;
>> > > > > > +
>> > > > > >         if (firstStart_) {
>> > > > > >                 /* Supply initial values for frame durations. */
>> > > > > >                 applyFrameDurations(defaultMinFrameDuration,
>> > > > > defaultMaxFrameDuration);
>> > > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
>> > > > > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
>> > > > > > index 0bdfa727..caf0030e 100644
>> > > > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
>> > > > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
>> > > > > > @@ -147,7 +147,7 @@ public:
>> > > > > >         void frameStarted(uint32_t sequence);
>> > > > > >
>> > > > > >         int loadIPA(ipa::RPi::SensorConfig *sensorConfig);
>> > > > > > -       int configureIPA(const CameraConfiguration *config);
>> > > > > > +       int configureIPA(CameraConfiguration *config);
>> > > > > >
>> > > > > >         void statsMetadataComplete(uint32_t bufferId, const
>> > > ControlList
>> > > > > &controls);
>> > > > > >         void runIsp(uint32_t bufferId);
>> > > > > > @@ -1250,7 +1250,7 @@ int
>> > > RPiCameraData::loadIPA(ipa::RPi::SensorConfig
>> > > > > *sensorConfig)
>> > > > > >         return ipa_->init(settings, sensorConfig);
>> > > > > >  }
>> > > > > >
>> > > > > > -int RPiCameraData::configureIPA(const CameraConfiguration
>> *config)
>> > > > > > +int RPiCameraData::configureIPA(CameraConfiguration *config)
>> > > > > >  {
>> > > > > >         /* We know config must be an RPiCameraConfiguration. */
>> > > > > >         const RPiCameraConfiguration *rpiConfig =
>> > > > > > @@ -1299,13 +1299,17 @@ int RPiCameraData::configureIPA(const
>> > > > > CameraConfiguration *config)
>> > > > > >
>> > > > > >         /* Ready the IPA - it must know about the sensor
>> resolution.
>> > > */
>> > > > > >         ControlList controls;
>> > > > > > +       ipa::RPi::IPAConfigResult result;
>> > > > > >         ret = ipa_->configure(sensorInfo_, streamConfig,
>> > > entityControls,
>> > > > > ipaConfig,
>> > > > > > -                             &controls);
>> > > > > > +                             &controls, &result);
>> > > > >
>> > > > > I've just rebased this series to master to facilitate merging,
>> and with
>> > > > > fresh eyes I can't help but wonder if this value shouldn't be
>> returned
>> > > > > in the validate() phase. (Not sure if this has been asked before /
>> > > yet).
>> > > > >
>> > > > > Is there anything that prevents us adding a validate() to the IPA
>> > > > > interface to allow validating the configuration and at that point,
>> > > > > setting the mode sensitivity? Or can this value /only/ be
>> determined
>> > > > > when configuring?
>> > > > >
>> > > >
>> > > > I can't think of any reason that this couldn't be done in
>> validate(), but
>> > > > David
>> > > > might have some reasons.  However, is an application required to
>> call
>> > > > CameraConfiguration::validate() directly?  I can see that it does
>> get
>> > > called
>> > > > in Camera::configure(), just before the call to
>> > > > PipelineHandler::configure().
>> > > > If there is no requirement to call CameraConfiguration::validate(),
>> > > would it
>> > > > matter where the result gets set?
>> > >
>> > > There isn't a 'requirement' to do so as such, but it's recommended. If
>> > > the call to configure can't be satisfied 'precisely' then the
>> configure
>> > > call should fail. It should not make any changes or adjustments to the
>> > > configuration.
>> > >
>> > > Validate will tell you if the pipeline handler made any changes, and
>> > > more than that - the validate before configure /must/ report that no
>> > > changes were made.
>> > >
>> >
>> > I think this is the part that may be troublesome.  Assume an application
>> > did not call CameraConfiguration::validate() directly, but it does get
>> > called
>> > from PipelineHandler::configure(). The validate() will then adjust the
>> > sensitivity value and return a Status::Adjusted.  This will then fail
>> the
>> > PipelineHandler::configure() call.  Perhaps
>> CameraConfiguration::validate()
>> > should be a mandatory call from the application? Or if we adjust the
>> > sensitivity in CameraConfiguration::validate(), we don't return
>> > Status::Adjusted?
>> >
>>
>> I'm sorry - I was wrong to say there isn't a requirement to do so.
>>
>>
>> https://www.libcamera.org/api-html/classlibcamera_1_1CameraConfiguration.html#details
>>
>> documents that applications 'shall' be validated by a call to
>> validate().
>>
>
> Ah, that makes things less ambiguous then.  I'll let David comment further
> when he is back, but I see no reason not to switch this to validate() now.
>

After having prototyped an ipa::validate() to pass the sensitivity back to
the
PH, I've changed my mind on the above statement :-)

The reason for this is that things just feel a bit more awkward in the
validate
call.For instance, we have not yet set up a sensor mode in the IPA to use
for
deducing the mode sensitivity.  If we move the mode setup into
ipa::validate(),
there are further things associated with the mode that need to happen there
(e.g.
helper->set_mode())that do not feel right to me.

Instead of doing things this way, I wonder if we should look to pass the
sensitivity out via the Camera::properties_ ControlList.  My initial
reservation
suggesting this was the assumption that properties were static through the
lifetime of the camera.However, this is not the case, as we have
properties::ScalerCropMaximum that changes on every call to configure().
What if
we added a properties::SensorSensitivity that also gets updated in the same
way?
This will remove the need for CameraConfiguration::modeSensitivityfield
entirely.

Thanks,
Naush


>
> Regards,
> Naush
>
>
>>
>> """
>> The CameraConfiguration holds an ordered list of stream configurations.
>> It supports iterators and operates as a vector of StreamConfiguration
>> instances. The stream configurations are inserted by addConfiguration(),
>> and the at() function or operator[] return a reference to the
>> StreamConfiguration based on its insertion index. Accessing a stream
>> configuration with an invalid index results in undefined behaviour.
>>
>> CameraConfiguration instances are retrieved from the camera with
>> Camera::generateConfiguration(). Applications may then inspect the
>> configuration, modify it, and possibly add new stream configuration
>> entries with addConfiguration(). Once the camera configuration satisfies
>> the application, it shall be validated by a call to validate(). The
>> validation implements "try" semantics: it adjusts invalid configurations
>> to the closest achievable parameters instead of rejecting them
>> completely. Applications then decide whether to accept the modified
>> configuration, or try again with a different set of parameters. Once the
>> configuration is valid, it is passed to Camera::configure().
>>
>> """
>>
>> --
>> Kieran
>>
>>
>> > Naush
>> >
>> >
>> > >
>> > > When an application calls validate() - the pipeline handler can make
>> > > changes to the Configuration (such as filling in the ModeSensitivity
>> > > here) and if a change was made - it can return
>> 'ConfigurationAdjusted' -
>> > > which then means the application should check to see what was
>> different
>> > > from what it requested - to what the pipeline handler will deliver.
>> > >
>> > > I would like there to be a way to highlight 'what' changes were made
>> if
>> > > the validate returns 'Adjusted' - but I'm not sure how we could easily
>> > > implement that yet.
>> > >
>> > > --
>> > > Kieran
>> > >
>> > >
>> > > > Regards,
>> > > > Naush
>> > > >
>> > > >
>> > > > >
>> > > > > Then the validate phase should be able to return the mode
>> sensitivity
>> > > of
>> > > > > the configuration that is being validated.
>> > > > >
>> > > > > Doing that we could keep the configure calls using a const
>> parameter
>> > > for
>> > > > > their config structures.
>> > > > >
>> > > > > >         if (ret < 0) {
>> > > > > >                 LOG(RPI, Error) << "IPA configuration failed!";
>> > > > > >                 return -EPIPE;
>> > > > > >         }
>> > > > > >
>> > > > > > +       /* Store the mode sensitivity for the application. */
>> > > > > > +       config->modeSensitivity = result.modeSensitivity;
>> > > > > > +
>> > > > > >         /*
>> > > > > >          * Configure the H/V flip controls based on the
>> combination
>> > > of
>> > > > > >          * the sensor and user transform.
>> > > > > > --
>> > > > > > 2.20.1
>> > > > > >
>> > > > >
>> > >
>>
>
David Plowman April 21, 2022, 1:32 p.m. UTC | #14 
Yes, I recall that there was discussion that this number would be
better as a property, and wasn't the conclusion that we should move it
(even if not right now)? So doing it like this is certainly fine with
me.

David

On Thu, 21 Apr 2022 at 14:14, Naushir Patuck <naush@raspberrypi.com> wrote:
>
> Hi Kieran,
>
> On Wed, 13 Apr 2022 at 13:36, Naushir Patuck <naush@raspberrypi.com> wrote:
>>
>>
>>
>> On Wed, 13 Apr 2022 at 11:59, Kieran Bingham <kieran.bingham@ideasonboard.com> wrote:
>>>
>>> Quoting Naushir Patuck (2022-04-13 11:44:14)
>>> > Hi Kieran,
>>> >
>>> > On Wed, 13 Apr 2022 at 11:20, Kieran Bingham <
>>> > kieran.bingham@ideasonboard.com> wrote:
>>> >
>>> > > Quoting Naushir Patuck (2022-04-13 11:14:01)
>>> > > > Hi Kieran,
>>> > > >
>>> > > > On Wed, 13 Apr 2022 at 10:38, Kieran Bingham <
>>> > > > kieran.bingham@ideasonboard.com> wrote:
>>> > > >
>>> > > > > Hi David, Naush,
>>> > > > >
>>> > > > > Quoting David Plowman (2021-09-22 14:29:15)
>>> > > > > > These changes retrieve the correct value for sensitivity of the mode
>>> > > > > > selected for the sensor. This value is known to the CamHelper which
>>> > > > > > passes it across to the pipeline handler so that it can be set
>>> > > > > > correctly in the CameraConfiguration.
>>> > > > > >
>>> > > > > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
>>> > > > > > ---
>>> > > > > >  include/libcamera/ipa/raspberrypi.mojom            |  7 ++++++-
>>> > > > > >  src/ipa/raspberrypi/raspberrypi.cpp                |  7 +++++--
>>> > > > > >  src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 10 +++++++---
>>> > > > > >  3 files changed, 18 insertions(+), 6 deletions(-)
>>> > > > > >
>>> > > > > > diff --git a/include/libcamera/ipa/raspberrypi.mojom
>>> > > > > b/include/libcamera/ipa/raspberrypi.mojom
>>> > > > > > index e453d46c..a92a76f8 100644
>>> > > > > > --- a/include/libcamera/ipa/raspberrypi.mojom
>>> > > > > > +++ b/include/libcamera/ipa/raspberrypi.mojom
>>> > > > > > @@ -38,6 +38,10 @@ struct IPAConfig {
>>> > > > > >         libcamera.FileDescriptor lsTableHandle;
>>> > > > > >  };
>>> > > > > >
>>> > > > > > +struct IPAConfigResult {
>>> > > > > > +       float modeSensitivity;
>>> > > > > > +};
>>> > > > > > +
>>> > > > > >  struct StartConfig {
>>> > > > > >         libcamera.ControlList controls;
>>> > > > > >         int32 dropFrameCount;
>>> > > > > > @@ -57,6 +61,7 @@ interface IPARPiInterface {
>>> > > > > >          * \param[in] entityControls Controls provided by the
>>> > > pipeline
>>> > > > > entities
>>> > > > > >          * \param[in] ipaConfig Pipeline-handler-specific
>>> > > configuration
>>> > > > > data
>>> > > > > >          * \param[out] controls Controls to apply by the pipeline
>>> > > entity
>>> > > > > > +        * \param[out] result Other results that the pipeline handler
>>> > > > > may require
>>> > > > > >          *
>>> > > > > >          * This function shall be called when the camera is
>>> > > configured
>>> > > > > to inform
>>> > > > > >          * the IPA of the camera's streams and the sensor settings.
>>> > > > > > @@ -71,7 +76,7 @@ interface IPARPiInterface {
>>> > > > > >                   map<uint32, libcamera.IPAStream> streamConfig,
>>> > > > > >                   map<uint32, libcamera.ControlInfoMap>
>>> > > entityControls,
>>> > > > > >                   IPAConfig ipaConfig)
>>> > > > > > -               => (int32 ret, libcamera.ControlList controls);
>>> > > > > > +               => (int32 ret, libcamera.ControlList controls,
>>> > > > > IPAConfigResult result);
>>> > > > > >
>>> > > > > >         /**
>>> > > > > >          * \fn mapBuffers()
>>> > > > > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp
>>> > > > > b/src/ipa/raspberrypi/raspberrypi.cpp
>>> > > > > > index 047123ab..796e6d15 100644
>>> > > > > > --- a/src/ipa/raspberrypi/raspberrypi.cpp
>>> > > > > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp
>>> > > > > > @@ -97,7 +97,7 @@ public:
>>> > > > > >                       const std::map<unsigned int, IPAStream>
>>> > > > > &streamConfig,
>>> > > > > >                       const std::map<unsigned int, ControlInfoMap>
>>> > > > > &entityControls,
>>> > > > > >                       const ipa::RPi::IPAConfig &data,
>>> > > > > > -                     ControlList *controls) override;
>>> > > > > > +                     ControlList *controls,
>>> > > ipa::RPi::IPAConfigResult
>>> > > > > *result) override;
>>> > > > > >         void mapBuffers(const std::vector<IPABuffer> &buffers)
>>> > > override;
>>> > > > > >         void unmapBuffers(const std::vector<unsigned int> &ids)
>>> > > override;
>>> > > > > >         void signalStatReady(const uint32_t bufferId) override;
>>> > > > > > @@ -337,7 +337,7 @@ int IPARPi::configure(const IPACameraSensorInfo
>>> > > > > &sensorInfo,
>>> > > > > >                       [[maybe_unused]] const std::map<unsigned int,
>>> > > > > IPAStream> &streamConfig,
>>> > > > > >                       const std::map<unsigned int, ControlInfoMap>
>>> > > > > &entityControls,
>>> > > > > >                       const ipa::RPi::IPAConfig &ipaConfig,
>>> > > > > > -                     ControlList *controls)
>>> > > > > > +                     ControlList *controls,
>>> > > ipa::RPi::IPAConfigResult
>>> > > > > *result)
>>> > > > > >  {
>>> > > > > >         if (entityControls.size() != 2) {
>>> > > > > >                 LOG(IPARPI, Error) << "No ISP or sensor controls
>>> > > found.";
>>> > > > > > @@ -389,6 +389,9 @@ int IPARPi::configure(const IPACameraSensorInfo
>>> > > > > &sensorInfo,
>>> > > > > >         /* Pass the camera mode to the CamHelper to setup
>>> > > algorithms. */
>>> > > > > >         helper_->SetCameraMode(mode_);
>>> > > > > >
>>> > > > > > +       /* The pipeline handler passes out the mode's sensitivity. */
>>> > > > > > +       result->modeSensitivity = mode_.sensitivity;
>>> > > > > > +
>>> > > > > >         if (firstStart_) {
>>> > > > > >                 /* Supply initial values for frame durations. */
>>> > > > > >                 applyFrameDurations(defaultMinFrameDuration,
>>> > > > > defaultMaxFrameDuration);
>>> > > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
>>> > > > > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
>>> > > > > > index 0bdfa727..caf0030e 100644
>>> > > > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
>>> > > > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
>>> > > > > > @@ -147,7 +147,7 @@ public:
>>> > > > > >         void frameStarted(uint32_t sequence);
>>> > > > > >
>>> > > > > >         int loadIPA(ipa::RPi::SensorConfig *sensorConfig);
>>> > > > > > -       int configureIPA(const CameraConfiguration *config);
>>> > > > > > +       int configureIPA(CameraConfiguration *config);
>>> > > > > >
>>> > > > > >         void statsMetadataComplete(uint32_t bufferId, const
>>> > > ControlList
>>> > > > > &controls);
>>> > > > > >         void runIsp(uint32_t bufferId);
>>> > > > > > @@ -1250,7 +1250,7 @@ int
>>> > > RPiCameraData::loadIPA(ipa::RPi::SensorConfig
>>> > > > > *sensorConfig)
>>> > > > > >         return ipa_->init(settings, sensorConfig);
>>> > > > > >  }
>>> > > > > >
>>> > > > > > -int RPiCameraData::configureIPA(const CameraConfiguration *config)
>>> > > > > > +int RPiCameraData::configureIPA(CameraConfiguration *config)
>>> > > > > >  {
>>> > > > > >         /* We know config must be an RPiCameraConfiguration. */
>>> > > > > >         const RPiCameraConfiguration *rpiConfig =
>>> > > > > > @@ -1299,13 +1299,17 @@ int RPiCameraData::configureIPA(const
>>> > > > > CameraConfiguration *config)
>>> > > > > >
>>> > > > > >         /* Ready the IPA - it must know about the sensor resolution.
>>> > > */
>>> > > > > >         ControlList controls;
>>> > > > > > +       ipa::RPi::IPAConfigResult result;
>>> > > > > >         ret = ipa_->configure(sensorInfo_, streamConfig,
>>> > > entityControls,
>>> > > > > ipaConfig,
>>> > > > > > -                             &controls);
>>> > > > > > +                             &controls, &result);
>>> > > > >
>>> > > > > I've just rebased this series to master to facilitate merging, and with
>>> > > > > fresh eyes I can't help but wonder if this value shouldn't be returned
>>> > > > > in the validate() phase. (Not sure if this has been asked before /
>>> > > yet).
>>> > > > >
>>> > > > > Is there anything that prevents us adding a validate() to the IPA
>>> > > > > interface to allow validating the configuration and at that point,
>>> > > > > setting the mode sensitivity? Or can this value /only/ be determined
>>> > > > > when configuring?
>>> > > > >
>>> > > >
>>> > > > I can't think of any reason that this couldn't be done in validate(), but
>>> > > > David
>>> > > > might have some reasons.  However, is an application required to call
>>> > > > CameraConfiguration::validate() directly?  I can see that it does get
>>> > > called
>>> > > > in Camera::configure(), just before the call to
>>> > > > PipelineHandler::configure().
>>> > > > If there is no requirement to call CameraConfiguration::validate(),
>>> > > would it
>>> > > > matter where the result gets set?
>>> > >
>>> > > There isn't a 'requirement' to do so as such, but it's recommended. If
>>> > > the call to configure can't be satisfied 'precisely' then the configure
>>> > > call should fail. It should not make any changes or adjustments to the
>>> > > configuration.
>>> > >
>>> > > Validate will tell you if the pipeline handler made any changes, and
>>> > > more than that - the validate before configure /must/ report that no
>>> > > changes were made.
>>> > >
>>> >
>>> > I think this is the part that may be troublesome.  Assume an application
>>> > did not call CameraConfiguration::validate() directly, but it does get
>>> > called
>>> > from PipelineHandler::configure(). The validate() will then adjust the
>>> > sensitivity value and return a Status::Adjusted.  This will then fail the
>>> > PipelineHandler::configure() call.  Perhaps CameraConfiguration::validate()
>>> > should be a mandatory call from the application? Or if we adjust the
>>> > sensitivity in CameraConfiguration::validate(), we don't return
>>> > Status::Adjusted?
>>> >
>>>
>>> I'm sorry - I was wrong to say there isn't a requirement to do so.
>>>
>>> https://www.libcamera.org/api-html/classlibcamera_1_1CameraConfiguration.html#details
>>>
>>> documents that applications 'shall' be validated by a call to
>>> validate().
>>
>>
>> Ah, that makes things less ambiguous then.  I'll let David comment further
>> when he is back, but I see no reason not to switch this to validate() now.
>
>
> After having prototyped an ipa::validate() to pass the sensitivity back to the
> PH, I've changed my mind on the above statement :-)
>
> The reason for this is that things just feel a bit more awkward in the validate
> call.For instance, we have not yet set up a sensor mode in the IPA to use for
> deducing the mode sensitivity.  If we move the mode setup into ipa::validate(),
> there are further things associated with the mode that need to happen there (e.g.
> helper->set_mode())that do not feel right to me.
>
> Instead of doing things this way, I wonder if we should look to pass the
> sensitivity out via the Camera::properties_ ControlList.  My initial reservation
> suggesting this was the assumption that properties were static through the
> lifetime of the camera.However, this is not the case, as we have
> properties::ScalerCropMaximum that changes on every call to configure(). What if
> we added a properties::SensorSensitivity that also gets updated in the same way?
> This will remove the need for CameraConfiguration::modeSensitivityfield entirely.
>
> Thanks,
> Naush
>
>>
>>
>> Regards,
>> Naush
>>
>>>
>>>
>>> """
>>> The CameraConfiguration holds an ordered list of stream configurations.
>>> It supports iterators and operates as a vector of StreamConfiguration
>>> instances. The stream configurations are inserted by addConfiguration(),
>>> and the at() function or operator[] return a reference to the
>>> StreamConfiguration based on its insertion index. Accessing a stream
>>> configuration with an invalid index results in undefined behaviour.
>>>
>>> CameraConfiguration instances are retrieved from the camera with
>>> Camera::generateConfiguration(). Applications may then inspect the
>>> configuration, modify it, and possibly add new stream configuration
>>> entries with addConfiguration(). Once the camera configuration satisfies
>>> the application, it shall be validated by a call to validate(). The
>>> validation implements "try" semantics: it adjusts invalid configurations
>>> to the closest achievable parameters instead of rejecting them
>>> completely. Applications then decide whether to accept the modified
>>> configuration, or try again with a different set of parameters. Once the
>>> configuration is valid, it is passed to Camera::configure().
>>>
>>> """
>>>
>>> --
>>> Kieran
>>>
>>>
>>> > Naush
>>> >
>>> >
>>> > >
>>> > > When an application calls validate() - the pipeline handler can make
>>> > > changes to the Configuration (such as filling in the ModeSensitivity
>>> > > here) and if a change was made - it can return 'ConfigurationAdjusted' -
>>> > > which then means the application should check to see what was different
>>> > > from what it requested - to what the pipeline handler will deliver.
>>> > >
>>> > > I would like there to be a way to highlight 'what' changes were made if
>>> > > the validate returns 'Adjusted' - but I'm not sure how we could easily
>>> > > implement that yet.
>>> > >
>>> > > --
>>> > > Kieran
>>> > >
>>> > >
>>> > > > Regards,
>>> > > > Naush
>>> > > >
>>> > > >
>>> > > > >
>>> > > > > Then the validate phase should be able to return the mode sensitivity
>>> > > of
>>> > > > > the configuration that is being validated.
>>> > > > >
>>> > > > > Doing that we could keep the configure calls using a const parameter
>>> > > for
>>> > > > > their config structures.
>>> > > > >
>>> > > > > >         if (ret < 0) {
>>> > > > > >                 LOG(RPI, Error) << "IPA configuration failed!";
>>> > > > > >                 return -EPIPE;
>>> > > > > >         }
>>> > > > > >
>>> > > > > > +       /* Store the mode sensitivity for the application. */
>>> > > > > > +       config->modeSensitivity = result.modeSensitivity;
>>> > > > > > +
>>> > > > > >         /*
>>> > > > > >          * Configure the H/V flip controls based on the combination
>>> > > of
>>> > > > > >          * the sensor and user transform.
>>> > > > > > --
>>> > > > > > 2.20.1
>>> > > > > >
>>> > > > >
>>> > >
Kieran Bingham April 21, 2022, 1:58 p.m. UTC | #15 
Hi Naush, David,

Quoting Naushir Patuck (2022-04-21 14:13:45)
> Hi Kieran,
> 
> On Wed, 13 Apr 2022 at 13:36, Naushir Patuck <naush@raspberrypi.com> wrote:
> 
> >
> >
> > On Wed, 13 Apr 2022 at 11:59, Kieran Bingham <
> > kieran.bingham@ideasonboard.com> wrote:
> >
> >> Quoting Naushir Patuck (2022-04-13 11:44:14)
> >> > Hi Kieran,
> >> >
> >> > On Wed, 13 Apr 2022 at 11:20, Kieran Bingham <
> >> > kieran.bingham@ideasonboard.com> wrote:
> >> >
> >> > > Quoting Naushir Patuck (2022-04-13 11:14:01)
> >> > > > Hi Kieran,
> >> > > >
> >> > > > On Wed, 13 Apr 2022 at 10:38, Kieran Bingham <
> >> > > > kieran.bingham@ideasonboard.com> wrote:
> >> > > >
> >> > > > > Hi David, Naush,
> >> > > > >
> >> > > > > Quoting David Plowman (2021-09-22 14:29:15)
> >> > > > > > These changes retrieve the correct value for sensitivity of the
> >> mode
> >> > > > > > selected for the sensor. This value is known to the CamHelper
> >> which
> >> > > > > > passes it across to the pipeline handler so that it can be set
> >> > > > > > correctly in the CameraConfiguration.
> >> > > > > >
> >> > > > > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
> >> > > > > > ---
> >> > > > > >  include/libcamera/ipa/raspberrypi.mojom            |  7 ++++++-
> >> > > > > >  src/ipa/raspberrypi/raspberrypi.cpp                |  7 +++++--
> >> > > > > >  src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 10
> >> +++++++---
> >> > > > > >  3 files changed, 18 insertions(+), 6 deletions(-)
> >> > > > > >
> >> > > > > > diff --git a/include/libcamera/ipa/raspberrypi.mojom
> >> > > > > b/include/libcamera/ipa/raspberrypi.mojom
> >> > > > > > index e453d46c..a92a76f8 100644
> >> > > > > > --- a/include/libcamera/ipa/raspberrypi.mojom
> >> > > > > > +++ b/include/libcamera/ipa/raspberrypi.mojom
> >> > > > > > @@ -38,6 +38,10 @@ struct IPAConfig {
> >> > > > > >         libcamera.FileDescriptor lsTableHandle;
> >> > > > > >  };
> >> > > > > >
> >> > > > > > +struct IPAConfigResult {
> >> > > > > > +       float modeSensitivity;
> >> > > > > > +};
> >> > > > > > +
> >> > > > > >  struct StartConfig {
> >> > > > > >         libcamera.ControlList controls;
> >> > > > > >         int32 dropFrameCount;
> >> > > > > > @@ -57,6 +61,7 @@ interface IPARPiInterface {
> >> > > > > >          * \param[in] entityControls Controls provided by the
> >> > > pipeline
> >> > > > > entities
> >> > > > > >          * \param[in] ipaConfig Pipeline-handler-specific
> >> > > configuration
> >> > > > > data
> >> > > > > >          * \param[out] controls Controls to apply by the
> >> pipeline
> >> > > entity
> >> > > > > > +        * \param[out] result Other results that the pipeline
> >> handler
> >> > > > > may require
> >> > > > > >          *
> >> > > > > >          * This function shall be called when the camera is
> >> > > configured
> >> > > > > to inform
> >> > > > > >          * the IPA of the camera's streams and the sensor
> >> settings.
> >> > > > > > @@ -71,7 +76,7 @@ interface IPARPiInterface {
> >> > > > > >                   map<uint32, libcamera.IPAStream> streamConfig,
> >> > > > > >                   map<uint32, libcamera.ControlInfoMap>
> >> > > entityControls,
> >> > > > > >                   IPAConfig ipaConfig)
> >> > > > > > -               => (int32 ret, libcamera.ControlList controls);
> >> > > > > > +               => (int32 ret, libcamera.ControlList controls,
> >> > > > > IPAConfigResult result);
> >> > > > > >
> >> > > > > >         /**
> >> > > > > >          * \fn mapBuffers()
> >> > > > > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp
> >> > > > > b/src/ipa/raspberrypi/raspberrypi.cpp
> >> > > > > > index 047123ab..796e6d15 100644
> >> > > > > > --- a/src/ipa/raspberrypi/raspberrypi.cpp
> >> > > > > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> >> > > > > > @@ -97,7 +97,7 @@ public:
> >> > > > > >                       const std::map<unsigned int, IPAStream>
> >> > > > > &streamConfig,
> >> > > > > >                       const std::map<unsigned int,
> >> ControlInfoMap>
> >> > > > > &entityControls,
> >> > > > > >                       const ipa::RPi::IPAConfig &data,
> >> > > > > > -                     ControlList *controls) override;
> >> > > > > > +                     ControlList *controls,
> >> > > ipa::RPi::IPAConfigResult
> >> > > > > *result) override;
> >> > > > > >         void mapBuffers(const std::vector<IPABuffer> &buffers)
> >> > > override;
> >> > > > > >         void unmapBuffers(const std::vector<unsigned int> &ids)
> >> > > override;
> >> > > > > >         void signalStatReady(const uint32_t bufferId) override;
> >> > > > > > @@ -337,7 +337,7 @@ int IPARPi::configure(const
> >> IPACameraSensorInfo
> >> > > > > &sensorInfo,
> >> > > > > >                       [[maybe_unused]] const std::map<unsigned
> >> int,
> >> > > > > IPAStream> &streamConfig,
> >> > > > > >                       const std::map<unsigned int,
> >> ControlInfoMap>
> >> > > > > &entityControls,
> >> > > > > >                       const ipa::RPi::IPAConfig &ipaConfig,
> >> > > > > > -                     ControlList *controls)
> >> > > > > > +                     ControlList *controls,
> >> > > ipa::RPi::IPAConfigResult
> >> > > > > *result)
> >> > > > > >  {
> >> > > > > >         if (entityControls.size() != 2) {
> >> > > > > >                 LOG(IPARPI, Error) << "No ISP or sensor controls
> >> > > found.";
> >> > > > > > @@ -389,6 +389,9 @@ int IPARPi::configure(const
> >> IPACameraSensorInfo
> >> > > > > &sensorInfo,
> >> > > > > >         /* Pass the camera mode to the CamHelper to setup
> >> > > algorithms. */
> >> > > > > >         helper_->SetCameraMode(mode_);
> >> > > > > >
> >> > > > > > +       /* The pipeline handler passes out the mode's
> >> sensitivity. */
> >> > > > > > +       result->modeSensitivity = mode_.sensitivity;
> >> > > > > > +
> >> > > > > >         if (firstStart_) {
> >> > > > > >                 /* Supply initial values for frame durations. */
> >> > > > > >                 applyFrameDurations(defaultMinFrameDuration,
> >> > > > > defaultMaxFrameDuration);
> >> > > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> >> > > > > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> >> > > > > > index 0bdfa727..caf0030e 100644
> >> > > > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> >> > > > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> >> > > > > > @@ -147,7 +147,7 @@ public:
> >> > > > > >         void frameStarted(uint32_t sequence);
> >> > > > > >
> >> > > > > >         int loadIPA(ipa::RPi::SensorConfig *sensorConfig);
> >> > > > > > -       int configureIPA(const CameraConfiguration *config);
> >> > > > > > +       int configureIPA(CameraConfiguration *config);
> >> > > > > >
> >> > > > > >         void statsMetadataComplete(uint32_t bufferId, const
> >> > > ControlList
> >> > > > > &controls);
> >> > > > > >         void runIsp(uint32_t bufferId);
> >> > > > > > @@ -1250,7 +1250,7 @@ int
> >> > > RPiCameraData::loadIPA(ipa::RPi::SensorConfig
> >> > > > > *sensorConfig)
> >> > > > > >         return ipa_->init(settings, sensorConfig);
> >> > > > > >  }
> >> > > > > >
> >> > > > > > -int RPiCameraData::configureIPA(const CameraConfiguration
> >> *config)
> >> > > > > > +int RPiCameraData::configureIPA(CameraConfiguration *config)
> >> > > > > >  {
> >> > > > > >         /* We know config must be an RPiCameraConfiguration. */
> >> > > > > >         const RPiCameraConfiguration *rpiConfig =
> >> > > > > > @@ -1299,13 +1299,17 @@ int RPiCameraData::configureIPA(const
> >> > > > > CameraConfiguration *config)
> >> > > > > >
> >> > > > > >         /* Ready the IPA - it must know about the sensor
> >> resolution.
> >> > > */
> >> > > > > >         ControlList controls;
> >> > > > > > +       ipa::RPi::IPAConfigResult result;
> >> > > > > >         ret = ipa_->configure(sensorInfo_, streamConfig,
> >> > > entityControls,
> >> > > > > ipaConfig,
> >> > > > > > -                             &controls);
> >> > > > > > +                             &controls, &result);
> >> > > > >
> >> > > > > I've just rebased this series to master to facilitate merging,
> >> and with
> >> > > > > fresh eyes I can't help but wonder if this value shouldn't be
> >> returned
> >> > > > > in the validate() phase. (Not sure if this has been asked before /
> >> > > yet).
> >> > > > >
> >> > > > > Is there anything that prevents us adding a validate() to the IPA
> >> > > > > interface to allow validating the configuration and at that point,
> >> > > > > setting the mode sensitivity? Or can this value /only/ be
> >> determined
> >> > > > > when configuring?
> >> > > > >
> >> > > >
> >> > > > I can't think of any reason that this couldn't be done in
> >> validate(), but
> >> > > > David
> >> > > > might have some reasons.  However, is an application required to
> >> call
> >> > > > CameraConfiguration::validate() directly?  I can see that it does
> >> get
> >> > > called
> >> > > > in Camera::configure(), just before the call to
> >> > > > PipelineHandler::configure().
> >> > > > If there is no requirement to call CameraConfiguration::validate(),
> >> > > would it
> >> > > > matter where the result gets set?
> >> > >
> >> > > There isn't a 'requirement' to do so as such, but it's recommended. If
> >> > > the call to configure can't be satisfied 'precisely' then the
> >> configure
> >> > > call should fail. It should not make any changes or adjustments to the
> >> > > configuration.
> >> > >
> >> > > Validate will tell you if the pipeline handler made any changes, and
> >> > > more than that - the validate before configure /must/ report that no
> >> > > changes were made.
> >> > >
> >> >
> >> > I think this is the part that may be troublesome.  Assume an application
> >> > did not call CameraConfiguration::validate() directly, but it does get
> >> > called
> >> > from PipelineHandler::configure(). The validate() will then adjust the
> >> > sensitivity value and return a Status::Adjusted.  This will then fail
> >> the
> >> > PipelineHandler::configure() call.  Perhaps
> >> CameraConfiguration::validate()
> >> > should be a mandatory call from the application? Or if we adjust the
> >> > sensitivity in CameraConfiguration::validate(), we don't return
> >> > Status::Adjusted?
> >> >
> >>
> >> I'm sorry - I was wrong to say there isn't a requirement to do so.
> >>
> >>
> >> https://www.libcamera.org/api-html/classlibcamera_1_1CameraConfiguration.html#details
> >>
> >> documents that applications 'shall' be validated by a call to
> >> validate().
> >>
> >
> > Ah, that makes things less ambiguous then.  I'll let David comment further
> > when he is back, but I see no reason not to switch this to validate() now.
> >
> 
> After having prototyped an ipa::validate() to pass the sensitivity back to
> the
> PH, I've changed my mind on the above statement :-)
> 
> The reason for this is that things just feel a bit more awkward in the
> validate
> call.For instance, we have not yet set up a sensor mode in the IPA to use
> for
> deducing the mode sensitivity.  If we move the mode setup into
> ipa::validate(),
> there are further things associated with the mode that need to happen there
> (e.g.
> helper->set_mode())that do not feel right to me.
> 
> Instead of doing things this way, I wonder if we should look to pass the
> sensitivity out via the Camera::properties_ ControlList.  My initial
> reservation
> suggesting this was the assumption that properties were static through the
> lifetime of the camera.However, this is not the case, as we have
> properties::ScalerCropMaximum that changes on every call to configure().
> What if
> we added a properties::SensorSensitivity that also gets updated in the same
> way?
> This will remove the need for CameraConfiguration::modeSensitivityfield
> entirely.

That sounds ok to me too. I wonder if 'ModeSensitivity' is still the
right name in that instance ...

I think that would mean that it's only readable after the camera is
configured though - is that ok?

--
Kieran

> 
> Thanks,
> Naush
> 
> 
> >
> > Regards,
> > Naush
> >
> >
> >>
> >> """
> >> The CameraConfiguration holds an ordered list of stream configurations.
> >> It supports iterators and operates as a vector of StreamConfiguration
> >> instances. The stream configurations are inserted by addConfiguration(),
> >> and the at() function or operator[] return a reference to the
> >> StreamConfiguration based on its insertion index. Accessing a stream
> >> configuration with an invalid index results in undefined behaviour.
> >>
> >> CameraConfiguration instances are retrieved from the camera with
> >> Camera::generateConfiguration(). Applications may then inspect the
> >> configuration, modify it, and possibly add new stream configuration
> >> entries with addConfiguration(). Once the camera configuration satisfies
> >> the application, it shall be validated by a call to validate(). The
> >> validation implements "try" semantics: it adjusts invalid configurations
> >> to the closest achievable parameters instead of rejecting them
> >> completely. Applications then decide whether to accept the modified
> >> configuration, or try again with a different set of parameters. Once the
> >> configuration is valid, it is passed to Camera::configure().
> >>
> >> """
> >>
> >> --
> >> Kieran
> >>
> >>
> >> > Naush
> >> >
> >> >
> >> > >
> >> > > When an application calls validate() - the pipeline handler can make
> >> > > changes to the Configuration (such as filling in the ModeSensitivity
> >> > > here) and if a change was made - it can return
> >> 'ConfigurationAdjusted' -
> >> > > which then means the application should check to see what was
> >> different
> >> > > from what it requested - to what the pipeline handler will deliver.
> >> > >
> >> > > I would like there to be a way to highlight 'what' changes were made
> >> if
> >> > > the validate returns 'Adjusted' - but I'm not sure how we could easily
> >> > > implement that yet.
> >> > >
> >> > > --
> >> > > Kieran
> >> > >
> >> > >
> >> > > > Regards,
> >> > > > Naush
> >> > > >
> >> > > >
> >> > > > >
> >> > > > > Then the validate phase should be able to return the mode
> >> sensitivity
> >> > > of
> >> > > > > the configuration that is being validated.
> >> > > > >
> >> > > > > Doing that we could keep the configure calls using a const
> >> parameter
> >> > > for
> >> > > > > their config structures.
> >> > > > >
> >> > > > > >         if (ret < 0) {
> >> > > > > >                 LOG(RPI, Error) << "IPA configuration failed!";
> >> > > > > >                 return -EPIPE;
> >> > > > > >         }
> >> > > > > >
> >> > > > > > +       /* Store the mode sensitivity for the application. */
> >> > > > > > +       config->modeSensitivity = result.modeSensitivity;
> >> > > > > > +
> >> > > > > >         /*
> >> > > > > >          * Configure the H/V flip controls based on the
> >> combination
> >> > > of
> >> > > > > >          * the sensor and user transform.
> >> > > > > > --
> >> > > > > > 2.20.1
> >> > > > > >
> >> > > > >
> >> > >
> >>
> >
Naushir Patuck April 21, 2022, 2:10 p.m. UTC | #16 
On Thu, 21 Apr 2022 at 14:58, Kieran Bingham <
kieran.bingham@ideasonboard.com> wrote:

> Hi Naush, David,
>
> Quoting Naushir Patuck (2022-04-21 14:13:45)
> > Hi Kieran,
> >
> > On Wed, 13 Apr 2022 at 13:36, Naushir Patuck <naush@raspberrypi.com>
> wrote:
> >
> > >
> > >
> > > On Wed, 13 Apr 2022 at 11:59, Kieran Bingham <
> > > kieran.bingham@ideasonboard.com> wrote:
> > >
> > >> Quoting Naushir Patuck (2022-04-13 11:44:14)
> > >> > Hi Kieran,
> > >> >
> > >> > On Wed, 13 Apr 2022 at 11:20, Kieran Bingham <
> > >> > kieran.bingham@ideasonboard.com> wrote:
> > >> >
> > >> > > Quoting Naushir Patuck (2022-04-13 11:14:01)
> > >> > > > Hi Kieran,
> > >> > > >
> > >> > > > On Wed, 13 Apr 2022 at 10:38, Kieran Bingham <
> > >> > > > kieran.bingham@ideasonboard.com> wrote:
> > >> > > >
> > >> > > > > Hi David, Naush,
> > >> > > > >
> > >> > > > > Quoting David Plowman (2021-09-22 14:29:15)
> > >> > > > > > These changes retrieve the correct value for sensitivity of
> the
> > >> mode
> > >> > > > > > selected for the sensor. This value is known to the
> CamHelper
> > >> which
> > >> > > > > > passes it across to the pipeline handler so that it can be
> set
> > >> > > > > > correctly in the CameraConfiguration.
> > >> > > > > >
> > >> > > > > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com
> >
> > >> > > > > > ---
> > >> > > > > >  include/libcamera/ipa/raspberrypi.mojom            |  7
> ++++++-
> > >> > > > > >  src/ipa/raspberrypi/raspberrypi.cpp                |  7
> +++++--
> > >> > > > > >  src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 10
> > >> +++++++---
> > >> > > > > >  3 files changed, 18 insertions(+), 6 deletions(-)
> > >> > > > > >
> > >> > > > > > diff --git a/include/libcamera/ipa/raspberrypi.mojom
> > >> > > > > b/include/libcamera/ipa/raspberrypi.mojom
> > >> > > > > > index e453d46c..a92a76f8 100644
> > >> > > > > > --- a/include/libcamera/ipa/raspberrypi.mojom
> > >> > > > > > +++ b/include/libcamera/ipa/raspberrypi.mojom
> > >> > > > > > @@ -38,6 +38,10 @@ struct IPAConfig {
> > >> > > > > >         libcamera.FileDescriptor lsTableHandle;
> > >> > > > > >  };
> > >> > > > > >
> > >> > > > > > +struct IPAConfigResult {
> > >> > > > > > +       float modeSensitivity;
> > >> > > > > > +};
> > >> > > > > > +
> > >> > > > > >  struct StartConfig {
> > >> > > > > >         libcamera.ControlList controls;
> > >> > > > > >         int32 dropFrameCount;
> > >> > > > > > @@ -57,6 +61,7 @@ interface IPARPiInterface {
> > >> > > > > >          * \param[in] entityControls Controls provided by
> the
> > >> > > pipeline
> > >> > > > > entities
> > >> > > > > >          * \param[in] ipaConfig Pipeline-handler-specific
> > >> > > configuration
> > >> > > > > data
> > >> > > > > >          * \param[out] controls Controls to apply by the
> > >> pipeline
> > >> > > entity
> > >> > > > > > +        * \param[out] result Other results that the
> pipeline
> > >> handler
> > >> > > > > may require
> > >> > > > > >          *
> > >> > > > > >          * This function shall be called when the camera is
> > >> > > configured
> > >> > > > > to inform
> > >> > > > > >          * the IPA of the camera's streams and the sensor
> > >> settings.
> > >> > > > > > @@ -71,7 +76,7 @@ interface IPARPiInterface {
> > >> > > > > >                   map<uint32, libcamera.IPAStream>
> streamConfig,
> > >> > > > > >                   map<uint32, libcamera.ControlInfoMap>
> > >> > > entityControls,
> > >> > > > > >                   IPAConfig ipaConfig)
> > >> > > > > > -               => (int32 ret, libcamera.ControlList
> controls);
> > >> > > > > > +               => (int32 ret, libcamera.ControlList
> controls,
> > >> > > > > IPAConfigResult result);
> > >> > > > > >
> > >> > > > > >         /**
> > >> > > > > >          * \fn mapBuffers()
> > >> > > > > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp
> > >> > > > > b/src/ipa/raspberrypi/raspberrypi.cpp
> > >> > > > > > index 047123ab..796e6d15 100644
> > >> > > > > > --- a/src/ipa/raspberrypi/raspberrypi.cpp
> > >> > > > > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> > >> > > > > > @@ -97,7 +97,7 @@ public:
> > >> > > > > >                       const std::map<unsigned int,
> IPAStream>
> > >> > > > > &streamConfig,
> > >> > > > > >                       const std::map<unsigned int,
> > >> ControlInfoMap>
> > >> > > > > &entityControls,
> > >> > > > > >                       const ipa::RPi::IPAConfig &data,
> > >> > > > > > -                     ControlList *controls) override;
> > >> > > > > > +                     ControlList *controls,
> > >> > > ipa::RPi::IPAConfigResult
> > >> > > > > *result) override;
> > >> > > > > >         void mapBuffers(const std::vector<IPABuffer>
> &buffers)
> > >> > > override;
> > >> > > > > >         void unmapBuffers(const std::vector<unsigned int>
> &ids)
> > >> > > override;
> > >> > > > > >         void signalStatReady(const uint32_t bufferId)
> override;
> > >> > > > > > @@ -337,7 +337,7 @@ int IPARPi::configure(const
> > >> IPACameraSensorInfo
> > >> > > > > &sensorInfo,
> > >> > > > > >                       [[maybe_unused]] const
> std::map<unsigned
> > >> int,
> > >> > > > > IPAStream> &streamConfig,
> > >> > > > > >                       const std::map<unsigned int,
> > >> ControlInfoMap>
> > >> > > > > &entityControls,
> > >> > > > > >                       const ipa::RPi::IPAConfig &ipaConfig,
> > >> > > > > > -                     ControlList *controls)
> > >> > > > > > +                     ControlList *controls,
> > >> > > ipa::RPi::IPAConfigResult
> > >> > > > > *result)
> > >> > > > > >  {
> > >> > > > > >         if (entityControls.size() != 2) {
> > >> > > > > >                 LOG(IPARPI, Error) << "No ISP or sensor
> controls
> > >> > > found.";
> > >> > > > > > @@ -389,6 +389,9 @@ int IPARPi::configure(const
> > >> IPACameraSensorInfo
> > >> > > > > &sensorInfo,
> > >> > > > > >         /* Pass the camera mode to the CamHelper to setup
> > >> > > algorithms. */
> > >> > > > > >         helper_->SetCameraMode(mode_);
> > >> > > > > >
> > >> > > > > > +       /* The pipeline handler passes out the mode's
> > >> sensitivity. */
> > >> > > > > > +       result->modeSensitivity = mode_.sensitivity;
> > >> > > > > > +
> > >> > > > > >         if (firstStart_) {
> > >> > > > > >                 /* Supply initial values for frame
> durations. */
> > >> > > > > >                 applyFrameDurations(defaultMinFrameDuration,
> > >> > > > > defaultMaxFrameDuration);
> > >> > > > > > diff --git
> a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > >> > > > > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > >> > > > > > index 0bdfa727..caf0030e 100644
> > >> > > > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > >> > > > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > >> > > > > > @@ -147,7 +147,7 @@ public:
> > >> > > > > >         void frameStarted(uint32_t sequence);
> > >> > > > > >
> > >> > > > > >         int loadIPA(ipa::RPi::SensorConfig *sensorConfig);
> > >> > > > > > -       int configureIPA(const CameraConfiguration *config);
> > >> > > > > > +       int configureIPA(CameraConfiguration *config);
> > >> > > > > >
> > >> > > > > >         void statsMetadataComplete(uint32_t bufferId, const
> > >> > > ControlList
> > >> > > > > &controls);
> > >> > > > > >         void runIsp(uint32_t bufferId);
> > >> > > > > > @@ -1250,7 +1250,7 @@ int
> > >> > > RPiCameraData::loadIPA(ipa::RPi::SensorConfig
> > >> > > > > *sensorConfig)
> > >> > > > > >         return ipa_->init(settings, sensorConfig);
> > >> > > > > >  }
> > >> > > > > >
> > >> > > > > > -int RPiCameraData::configureIPA(const CameraConfiguration
> > >> *config)
> > >> > > > > > +int RPiCameraData::configureIPA(CameraConfiguration
> *config)
> > >> > > > > >  {
> > >> > > > > >         /* We know config must be an
> RPiCameraConfiguration. */
> > >> > > > > >         const RPiCameraConfiguration *rpiConfig =
> > >> > > > > > @@ -1299,13 +1299,17 @@ int
> RPiCameraData::configureIPA(const
> > >> > > > > CameraConfiguration *config)
> > >> > > > > >
> > >> > > > > >         /* Ready the IPA - it must know about the sensor
> > >> resolution.
> > >> > > */
> > >> > > > > >         ControlList controls;
> > >> > > > > > +       ipa::RPi::IPAConfigResult result;
> > >> > > > > >         ret = ipa_->configure(sensorInfo_, streamConfig,
> > >> > > entityControls,
> > >> > > > > ipaConfig,
> > >> > > > > > -                             &controls);
> > >> > > > > > +                             &controls, &result);
> > >> > > > >
> > >> > > > > I've just rebased this series to master to facilitate merging,
> > >> and with
> > >> > > > > fresh eyes I can't help but wonder if this value shouldn't be
> > >> returned
> > >> > > > > in the validate() phase. (Not sure if this has been asked
> before /
> > >> > > yet).
> > >> > > > >
> > >> > > > > Is there anything that prevents us adding a validate() to the
> IPA
> > >> > > > > interface to allow validating the configuration and at that
> point,
> > >> > > > > setting the mode sensitivity? Or can this value /only/ be
> > >> determined
> > >> > > > > when configuring?
> > >> > > > >
> > >> > > >
> > >> > > > I can't think of any reason that this couldn't be done in
> > >> validate(), but
> > >> > > > David
> > >> > > > might have some reasons.  However, is an application required to
> > >> call
> > >> > > > CameraConfiguration::validate() directly?  I can see that it
> does
> > >> get
> > >> > > called
> > >> > > > in Camera::configure(), just before the call to
> > >> > > > PipelineHandler::configure().
> > >> > > > If there is no requirement to call
> CameraConfiguration::validate(),
> > >> > > would it
> > >> > > > matter where the result gets set?
> > >> > >
> > >> > > There isn't a 'requirement' to do so as such, but it's
> recommended. If
> > >> > > the call to configure can't be satisfied 'precisely' then the
> > >> configure
> > >> > > call should fail. It should not make any changes or adjustments
> to the
> > >> > > configuration.
> > >> > >
> > >> > > Validate will tell you if the pipeline handler made any changes,
> and
> > >> > > more than that - the validate before configure /must/ report that
> no
> > >> > > changes were made.
> > >> > >
> > >> >
> > >> > I think this is the part that may be troublesome.  Assume an
> application
> > >> > did not call CameraConfiguration::validate() directly, but it does
> get
> > >> > called
> > >> > from PipelineHandler::configure(). The validate() will then adjust
> the
> > >> > sensitivity value and return a Status::Adjusted.  This will then
> fail
> > >> the
> > >> > PipelineHandler::configure() call.  Perhaps
> > >> CameraConfiguration::validate()
> > >> > should be a mandatory call from the application? Or if we adjust the
> > >> > sensitivity in CameraConfiguration::validate(), we don't return
> > >> > Status::Adjusted?
> > >> >
> > >>
> > >> I'm sorry - I was wrong to say there isn't a requirement to do so.
> > >>
> > >>
> > >>
> https://www.libcamera.org/api-html/classlibcamera_1_1CameraConfiguration.html#details
> > >>
> > >> documents that applications 'shall' be validated by a call to
> > >> validate().
> > >>
> > >
> > > Ah, that makes things less ambiguous then.  I'll let David comment
> further
> > > when he is back, but I see no reason not to switch this to validate()
> now.
> > >
> >
> > After having prototyped an ipa::validate() to pass the sensitivity back
> to
> > the
> > PH, I've changed my mind on the above statement :-)
> >
> > The reason for this is that things just feel a bit more awkward in the
> > validate
> > call.For instance, we have not yet set up a sensor mode in the IPA to use
> > for
> > deducing the mode sensitivity.  If we move the mode setup into
> > ipa::validate(),
> > there are further things associated with the mode that need to happen
> there
> > (e.g.
> > helper->set_mode())that do not feel right to me.
> >
> > Instead of doing things this way, I wonder if we should look to pass the
> > sensitivity out via the Camera::properties_ ControlList.  My initial
> > reservation
> > suggesting this was the assumption that properties were static through
> the
> > lifetime of the camera.However, this is not the case, as we have
> > properties::ScalerCropMaximum that changes on every call to configure().
> > What if
> > we added a properties::SensorSensitivity that also gets updated in the
> same
> > way?
> > This will remove the need for CameraConfiguration::modeSensitivityfield
> > entirely.
>
> That sounds ok to me too. I wonder if 'ModeSensitivity' is still the
> right name in that instance ...
>

We could use SensorSensitivity?


>
> I think that would mean that it's only readable after the camera is
> configured though - is that ok?
>

I think that is unavoidable in this case, but equivalent to how we would
deal with properties::ScalerCropMaximum.

Naush



>
> --
> Kieran
>
> >
> > Thanks,
> > Naush
> >
> >
> > >
> > > Regards,
> > > Naush
> > >
> > >
> > >>
> > >> """
> > >> The CameraConfiguration holds an ordered list of stream
> configurations.
> > >> It supports iterators and operates as a vector of StreamConfiguration
> > >> instances. The stream configurations are inserted by
> addConfiguration(),
> > >> and the at() function or operator[] return a reference to the
> > >> StreamConfiguration based on its insertion index. Accessing a stream
> > >> configuration with an invalid index results in undefined behaviour.
> > >>
> > >> CameraConfiguration instances are retrieved from the camera with
> > >> Camera::generateConfiguration(). Applications may then inspect the
> > >> configuration, modify it, and possibly add new stream configuration
> > >> entries with addConfiguration(). Once the camera configuration
> satisfies
> > >> the application, it shall be validated by a call to validate(). The
> > >> validation implements "try" semantics: it adjusts invalid
> configurations
> > >> to the closest achievable parameters instead of rejecting them
> > >> completely. Applications then decide whether to accept the modified
> > >> configuration, or try again with a different set of parameters. Once
> the
> > >> configuration is valid, it is passed to Camera::configure().
> > >>
> > >> """
> > >>
> > >> --
> > >> Kieran
> > >>
> > >>
> > >> > Naush
> > >> >
> > >> >
> > >> > >
> > >> > > When an application calls validate() - the pipeline handler can
> make
> > >> > > changes to the Configuration (such as filling in the
> ModeSensitivity
> > >> > > here) and if a change was made - it can return
> > >> 'ConfigurationAdjusted' -
> > >> > > which then means the application should check to see what was
> > >> different
> > >> > > from what it requested - to what the pipeline handler will
> deliver.
> > >> > >
> > >> > > I would like there to be a way to highlight 'what' changes were
> made
> > >> if
> > >> > > the validate returns 'Adjusted' - but I'm not sure how we could
> easily
> > >> > > implement that yet.
> > >> > >
> > >> > > --
> > >> > > Kieran
> > >> > >
> > >> > >
> > >> > > > Regards,
> > >> > > > Naush
> > >> > > >
> > >> > > >
> > >> > > > >
> > >> > > > > Then the validate phase should be able to return the mode
> > >> sensitivity
> > >> > > of
> > >> > > > > the configuration that is being validated.
> > >> > > > >
> > >> > > > > Doing that we could keep the configure calls using a const
> > >> parameter
> > >> > > for
> > >> > > > > their config structures.
> > >> > > > >
> > >> > > > > >         if (ret < 0) {
> > >> > > > > >                 LOG(RPI, Error) << "IPA configuration
> failed!";
> > >> > > > > >                 return -EPIPE;
> > >> > > > > >         }
> > >> > > > > >
> > >> > > > > > +       /* Store the mode sensitivity for the application.
> */
> > >> > > > > > +       config->modeSensitivity = result.modeSensitivity;
> > >> > > > > > +
> > >> > > > > >         /*
> > >> > > > > >          * Configure the H/V flip controls based on the
> > >> combination
> > >> > > of
> > >> > > > > >          * the sensor and user transform.
> > >> > > > > > --
> > >> > > > > > 2.20.1
> > >> > > > > >
> > >> > > > >
> > >> > >
> > >>
> > >
>
David Plowman April 21, 2022, 2:16 p.m. UTC | #17 
Hi Kieran

On Thu, 21 Apr 2022 at 14:58, Kieran Bingham
<kieran.bingham@ideasonboard.com> wrote:
>
> Hi Naush, David,
>
> Quoting Naushir Patuck (2022-04-21 14:13:45)
> > Hi Kieran,
> >
> > On Wed, 13 Apr 2022 at 13:36, Naushir Patuck <naush@raspberrypi.com> wrote:
> >
> > >
> > >
> > > On Wed, 13 Apr 2022 at 11:59, Kieran Bingham <
> > > kieran.bingham@ideasonboard.com> wrote:
> > >
> > >> Quoting Naushir Patuck (2022-04-13 11:44:14)
> > >> > Hi Kieran,
> > >> >
> > >> > On Wed, 13 Apr 2022 at 11:20, Kieran Bingham <
> > >> > kieran.bingham@ideasonboard.com> wrote:
> > >> >
> > >> > > Quoting Naushir Patuck (2022-04-13 11:14:01)
> > >> > > > Hi Kieran,
> > >> > > >
> > >> > > > On Wed, 13 Apr 2022 at 10:38, Kieran Bingham <
> > >> > > > kieran.bingham@ideasonboard.com> wrote:
> > >> > > >
> > >> > > > > Hi David, Naush,
> > >> > > > >
> > >> > > > > Quoting David Plowman (2021-09-22 14:29:15)
> > >> > > > > > These changes retrieve the correct value for sensitivity of the
> > >> mode
> > >> > > > > > selected for the sensor. This value is known to the CamHelper
> > >> which
> > >> > > > > > passes it across to the pipeline handler so that it can be set
> > >> > > > > > correctly in the CameraConfiguration.
> > >> > > > > >
> > >> > > > > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
> > >> > > > > > ---
> > >> > > > > >  include/libcamera/ipa/raspberrypi.mojom            |  7 ++++++-
> > >> > > > > >  src/ipa/raspberrypi/raspberrypi.cpp                |  7 +++++--
> > >> > > > > >  src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 10
> > >> +++++++---
> > >> > > > > >  3 files changed, 18 insertions(+), 6 deletions(-)
> > >> > > > > >
> > >> > > > > > diff --git a/include/libcamera/ipa/raspberrypi.mojom
> > >> > > > > b/include/libcamera/ipa/raspberrypi.mojom
> > >> > > > > > index e453d46c..a92a76f8 100644
> > >> > > > > > --- a/include/libcamera/ipa/raspberrypi.mojom
> > >> > > > > > +++ b/include/libcamera/ipa/raspberrypi.mojom
> > >> > > > > > @@ -38,6 +38,10 @@ struct IPAConfig {
> > >> > > > > >         libcamera.FileDescriptor lsTableHandle;
> > >> > > > > >  };
> > >> > > > > >
> > >> > > > > > +struct IPAConfigResult {
> > >> > > > > > +       float modeSensitivity;
> > >> > > > > > +};
> > >> > > > > > +
> > >> > > > > >  struct StartConfig {
> > >> > > > > >         libcamera.ControlList controls;
> > >> > > > > >         int32 dropFrameCount;
> > >> > > > > > @@ -57,6 +61,7 @@ interface IPARPiInterface {
> > >> > > > > >          * \param[in] entityControls Controls provided by the
> > >> > > pipeline
> > >> > > > > entities
> > >> > > > > >          * \param[in] ipaConfig Pipeline-handler-specific
> > >> > > configuration
> > >> > > > > data
> > >> > > > > >          * \param[out] controls Controls to apply by the
> > >> pipeline
> > >> > > entity
> > >> > > > > > +        * \param[out] result Other results that the pipeline
> > >> handler
> > >> > > > > may require
> > >> > > > > >          *
> > >> > > > > >          * This function shall be called when the camera is
> > >> > > configured
> > >> > > > > to inform
> > >> > > > > >          * the IPA of the camera's streams and the sensor
> > >> settings.
> > >> > > > > > @@ -71,7 +76,7 @@ interface IPARPiInterface {
> > >> > > > > >                   map<uint32, libcamera.IPAStream> streamConfig,
> > >> > > > > >                   map<uint32, libcamera.ControlInfoMap>
> > >> > > entityControls,
> > >> > > > > >                   IPAConfig ipaConfig)
> > >> > > > > > -               => (int32 ret, libcamera.ControlList controls);
> > >> > > > > > +               => (int32 ret, libcamera.ControlList controls,
> > >> > > > > IPAConfigResult result);
> > >> > > > > >
> > >> > > > > >         /**
> > >> > > > > >          * \fn mapBuffers()
> > >> > > > > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp
> > >> > > > > b/src/ipa/raspberrypi/raspberrypi.cpp
> > >> > > > > > index 047123ab..796e6d15 100644
> > >> > > > > > --- a/src/ipa/raspberrypi/raspberrypi.cpp
> > >> > > > > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> > >> > > > > > @@ -97,7 +97,7 @@ public:
> > >> > > > > >                       const std::map<unsigned int, IPAStream>
> > >> > > > > &streamConfig,
> > >> > > > > >                       const std::map<unsigned int,
> > >> ControlInfoMap>
> > >> > > > > &entityControls,
> > >> > > > > >                       const ipa::RPi::IPAConfig &data,
> > >> > > > > > -                     ControlList *controls) override;
> > >> > > > > > +                     ControlList *controls,
> > >> > > ipa::RPi::IPAConfigResult
> > >> > > > > *result) override;
> > >> > > > > >         void mapBuffers(const std::vector<IPABuffer> &buffers)
> > >> > > override;
> > >> > > > > >         void unmapBuffers(const std::vector<unsigned int> &ids)
> > >> > > override;
> > >> > > > > >         void signalStatReady(const uint32_t bufferId) override;
> > >> > > > > > @@ -337,7 +337,7 @@ int IPARPi::configure(const
> > >> IPACameraSensorInfo
> > >> > > > > &sensorInfo,
> > >> > > > > >                       [[maybe_unused]] const std::map<unsigned
> > >> int,
> > >> > > > > IPAStream> &streamConfig,
> > >> > > > > >                       const std::map<unsigned int,
> > >> ControlInfoMap>
> > >> > > > > &entityControls,
> > >> > > > > >                       const ipa::RPi::IPAConfig &ipaConfig,
> > >> > > > > > -                     ControlList *controls)
> > >> > > > > > +                     ControlList *controls,
> > >> > > ipa::RPi::IPAConfigResult
> > >> > > > > *result)
> > >> > > > > >  {
> > >> > > > > >         if (entityControls.size() != 2) {
> > >> > > > > >                 LOG(IPARPI, Error) << "No ISP or sensor controls
> > >> > > found.";
> > >> > > > > > @@ -389,6 +389,9 @@ int IPARPi::configure(const
> > >> IPACameraSensorInfo
> > >> > > > > &sensorInfo,
> > >> > > > > >         /* Pass the camera mode to the CamHelper to setup
> > >> > > algorithms. */
> > >> > > > > >         helper_->SetCameraMode(mode_);
> > >> > > > > >
> > >> > > > > > +       /* The pipeline handler passes out the mode's
> > >> sensitivity. */
> > >> > > > > > +       result->modeSensitivity = mode_.sensitivity;
> > >> > > > > > +
> > >> > > > > >         if (firstStart_) {
> > >> > > > > >                 /* Supply initial values for frame durations. */
> > >> > > > > >                 applyFrameDurations(defaultMinFrameDuration,
> > >> > > > > defaultMaxFrameDuration);
> > >> > > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > >> > > > > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > >> > > > > > index 0bdfa727..caf0030e 100644
> > >> > > > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > >> > > > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > >> > > > > > @@ -147,7 +147,7 @@ public:
> > >> > > > > >         void frameStarted(uint32_t sequence);
> > >> > > > > >
> > >> > > > > >         int loadIPA(ipa::RPi::SensorConfig *sensorConfig);
> > >> > > > > > -       int configureIPA(const CameraConfiguration *config);
> > >> > > > > > +       int configureIPA(CameraConfiguration *config);
> > >> > > > > >
> > >> > > > > >         void statsMetadataComplete(uint32_t bufferId, const
> > >> > > ControlList
> > >> > > > > &controls);
> > >> > > > > >         void runIsp(uint32_t bufferId);
> > >> > > > > > @@ -1250,7 +1250,7 @@ int
> > >> > > RPiCameraData::loadIPA(ipa::RPi::SensorConfig
> > >> > > > > *sensorConfig)
> > >> > > > > >         return ipa_->init(settings, sensorConfig);
> > >> > > > > >  }
> > >> > > > > >
> > >> > > > > > -int RPiCameraData::configureIPA(const CameraConfiguration
> > >> *config)
> > >> > > > > > +int RPiCameraData::configureIPA(CameraConfiguration *config)
> > >> > > > > >  {
> > >> > > > > >         /* We know config must be an RPiCameraConfiguration. */
> > >> > > > > >         const RPiCameraConfiguration *rpiConfig =
> > >> > > > > > @@ -1299,13 +1299,17 @@ int RPiCameraData::configureIPA(const
> > >> > > > > CameraConfiguration *config)
> > >> > > > > >
> > >> > > > > >         /* Ready the IPA - it must know about the sensor
> > >> resolution.
> > >> > > */
> > >> > > > > >         ControlList controls;
> > >> > > > > > +       ipa::RPi::IPAConfigResult result;
> > >> > > > > >         ret = ipa_->configure(sensorInfo_, streamConfig,
> > >> > > entityControls,
> > >> > > > > ipaConfig,
> > >> > > > > > -                             &controls);
> > >> > > > > > +                             &controls, &result);
> > >> > > > >
> > >> > > > > I've just rebased this series to master to facilitate merging,
> > >> and with
> > >> > > > > fresh eyes I can't help but wonder if this value shouldn't be
> > >> returned
> > >> > > > > in the validate() phase. (Not sure if this has been asked before /
> > >> > > yet).
> > >> > > > >
> > >> > > > > Is there anything that prevents us adding a validate() to the IPA
> > >> > > > > interface to allow validating the configuration and at that point,
> > >> > > > > setting the mode sensitivity? Or can this value /only/ be
> > >> determined
> > >> > > > > when configuring?
> > >> > > > >
> > >> > > >
> > >> > > > I can't think of any reason that this couldn't be done in
> > >> validate(), but
> > >> > > > David
> > >> > > > might have some reasons.  However, is an application required to
> > >> call
> > >> > > > CameraConfiguration::validate() directly?  I can see that it does
> > >> get
> > >> > > called
> > >> > > > in Camera::configure(), just before the call to
> > >> > > > PipelineHandler::configure().
> > >> > > > If there is no requirement to call CameraConfiguration::validate(),
> > >> > > would it
> > >> > > > matter where the result gets set?
> > >> > >
> > >> > > There isn't a 'requirement' to do so as such, but it's recommended. If
> > >> > > the call to configure can't be satisfied 'precisely' then the
> > >> configure
> > >> > > call should fail. It should not make any changes or adjustments to the
> > >> > > configuration.
> > >> > >
> > >> > > Validate will tell you if the pipeline handler made any changes, and
> > >> > > more than that - the validate before configure /must/ report that no
> > >> > > changes were made.
> > >> > >
> > >> >
> > >> > I think this is the part that may be troublesome.  Assume an application
> > >> > did not call CameraConfiguration::validate() directly, but it does get
> > >> > called
> > >> > from PipelineHandler::configure(). The validate() will then adjust the
> > >> > sensitivity value and return a Status::Adjusted.  This will then fail
> > >> the
> > >> > PipelineHandler::configure() call.  Perhaps
> > >> CameraConfiguration::validate()
> > >> > should be a mandatory call from the application? Or if we adjust the
> > >> > sensitivity in CameraConfiguration::validate(), we don't return
> > >> > Status::Adjusted?
> > >> >
> > >>
> > >> I'm sorry - I was wrong to say there isn't a requirement to do so.
> > >>
> > >>
> > >> https://www.libcamera.org/api-html/classlibcamera_1_1CameraConfiguration.html#details
> > >>
> > >> documents that applications 'shall' be validated by a call to
> > >> validate().
> > >>
> > >
> > > Ah, that makes things less ambiguous then.  I'll let David comment further
> > > when he is back, but I see no reason not to switch this to validate() now.
> > >
> >
> > After having prototyped an ipa::validate() to pass the sensitivity back to
> > the
> > PH, I've changed my mind on the above statement :-)
> >
> > The reason for this is that things just feel a bit more awkward in the
> > validate
> > call.For instance, we have not yet set up a sensor mode in the IPA to use
> > for
> > deducing the mode sensitivity.  If we move the mode setup into
> > ipa::validate(),
> > there are further things associated with the mode that need to happen there
> > (e.g.
> > helper->set_mode())that do not feel right to me.
> >
> > Instead of doing things this way, I wonder if we should look to pass the
> > sensitivity out via the Camera::properties_ ControlList.  My initial
> > reservation
> > suggesting this was the assumption that properties were static through the
> > lifetime of the camera.However, this is not the case, as we have
> > properties::ScalerCropMaximum that changes on every call to configure().
> > What if
> > we added a properties::SensorSensitivity that also gets updated in the same
> > way?
> > This will remove the need for CameraConfiguration::modeSensitivityfield
> > entirely.
>
> That sounds ok to me too. I wonder if 'ModeSensitivity' is still the
> right name in that instance ...
>
> I think that would mean that it's only readable after the camera is
> configured though - is that ok?

Yes. The use-case is if you want to calculate exposure/gain values for
a new camera mode from values that you obtained from a different mode.
So you need to have these values before you call camera.start(). So
this is fine.

For anyone interested, it's exactly this
https://github.com/raspberrypi/picamera2/blob/main/examples/opencv_mertens_merge.py
that will go wrong. "exposure_normal" needs to be adjusted by the
ratio of the mode sensitivities.

David

>
> --
> Kieran
>
> >
> > Thanks,
> > Naush
> >
> >
> > >
> > > Regards,
> > > Naush
> > >
> > >
> > >>
> > >> """
> > >> The CameraConfiguration holds an ordered list of stream configurations.
> > >> It supports iterators and operates as a vector of StreamConfiguration
> > >> instances. The stream configurations are inserted by addConfiguration(),
> > >> and the at() function or operator[] return a reference to the
> > >> StreamConfiguration based on its insertion index. Accessing a stream
> > >> configuration with an invalid index results in undefined behaviour.
> > >>
> > >> CameraConfiguration instances are retrieved from the camera with
> > >> Camera::generateConfiguration(). Applications may then inspect the
> > >> configuration, modify it, and possibly add new stream configuration
> > >> entries with addConfiguration(). Once the camera configuration satisfies
> > >> the application, it shall be validated by a call to validate(). The
> > >> validation implements "try" semantics: it adjusts invalid configurations
> > >> to the closest achievable parameters instead of rejecting them
> > >> completely. Applications then decide whether to accept the modified
> > >> configuration, or try again with a different set of parameters. Once the
> > >> configuration is valid, it is passed to Camera::configure().
> > >>
> > >> """
> > >>
> > >> --
> > >> Kieran
> > >>
> > >>
> > >> > Naush
> > >> >
> > >> >
> > >> > >
> > >> > > When an application calls validate() - the pipeline handler can make
> > >> > > changes to the Configuration (such as filling in the ModeSensitivity
> > >> > > here) and if a change was made - it can return
> > >> 'ConfigurationAdjusted' -
> > >> > > which then means the application should check to see what was
> > >> different
> > >> > > from what it requested - to what the pipeline handler will deliver.
> > >> > >
> > >> > > I would like there to be a way to highlight 'what' changes were made
> > >> if
> > >> > > the validate returns 'Adjusted' - but I'm not sure how we could easily
> > >> > > implement that yet.
> > >> > >
> > >> > > --
> > >> > > Kieran
> > >> > >
> > >> > >
> > >> > > > Regards,
> > >> > > > Naush
> > >> > > >
> > >> > > >
> > >> > > > >
> > >> > > > > Then the validate phase should be able to return the mode
> > >> sensitivity
> > >> > > of
> > >> > > > > the configuration that is being validated.
> > >> > > > >
> > >> > > > > Doing that we could keep the configure calls using a const
> > >> parameter
> > >> > > for
> > >> > > > > their config structures.
> > >> > > > >
> > >> > > > > >         if (ret < 0) {
> > >> > > > > >                 LOG(RPI, Error) << "IPA configuration failed!";
> > >> > > > > >                 return -EPIPE;
> > >> > > > > >         }
> > >> > > > > >
> > >> > > > > > +       /* Store the mode sensitivity for the application. */
> > >> > > > > > +       config->modeSensitivity = result.modeSensitivity;
> > >> > > > > > +
> > >> > > > > >         /*
> > >> > > > > >          * Configure the H/V flip controls based on the
> > >> combination
> > >> > > of
> > >> > > > > >          * the sensor and user transform.
> > >> > > > > > --
> > >> > > > > > 2.20.1
> > >> > > > > >
> > >> > > > >
> > >> > >
> > >>
> > >

Patch
diff mbox series 

diff --git a/include/libcamera/ipa/raspberrypi.mojom b/include/libcamera/ipa/raspberrypi.mojom
index e453d46c..a92a76f8 100644
--- a/include/libcamera/ipa/raspberrypi.mojom
+++ b/include/libcamera/ipa/raspberrypi.mojom
@@ -38,6 +38,10 @@  struct IPAConfig {
 	libcamera.FileDescriptor lsTableHandle;
 };
 
+struct IPAConfigResult {
+       float modeSensitivity;
+};
+
 struct StartConfig {
 	libcamera.ControlList controls;
 	int32 dropFrameCount;
@@ -57,6 +61,7 @@  interface IPARPiInterface {
 	 * \param[in] entityControls Controls provided by the pipeline entities
 	 * \param[in] ipaConfig Pipeline-handler-specific configuration data
 	 * \param[out] controls Controls to apply by the pipeline entity
+	 * \param[out] result Other results that the pipeline handler may require
 	 *
 	 * This function shall be called when the camera is configured to inform
 	 * the IPA of the camera's streams and the sensor settings.
@@ -71,7 +76,7 @@  interface IPARPiInterface {
 		  map<uint32, libcamera.IPAStream> streamConfig,
 		  map<uint32, libcamera.ControlInfoMap> entityControls,
 		  IPAConfig ipaConfig)
-		=> (int32 ret, libcamera.ControlList controls);
+		=> (int32 ret, libcamera.ControlList controls, IPAConfigResult result);
 
 	/**
 	 * \fn mapBuffers()
diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
index 047123ab..796e6d15 100644
--- a/src/ipa/raspberrypi/raspberrypi.cpp
+++ b/src/ipa/raspberrypi/raspberrypi.cpp
@@ -97,7 +97,7 @@  public:
 		      const std::map<unsigned int, IPAStream> &streamConfig,
 		      const std::map<unsigned int, ControlInfoMap> &entityControls,
 		      const ipa::RPi::IPAConfig &data,
-		      ControlList *controls) override;
+		      ControlList *controls, ipa::RPi::IPAConfigResult *result) override;
 	void mapBuffers(const std::vector<IPABuffer> &buffers) override;
 	void unmapBuffers(const std::vector<unsigned int> &ids) override;
 	void signalStatReady(const uint32_t bufferId) override;
@@ -337,7 +337,7 @@  int IPARPi::configure(const IPACameraSensorInfo &sensorInfo,
 		      [[maybe_unused]] const std::map<unsigned int, IPAStream> &streamConfig,
 		      const std::map<unsigned int, ControlInfoMap> &entityControls,
 		      const ipa::RPi::IPAConfig &ipaConfig,
-		      ControlList *controls)
+		      ControlList *controls, ipa::RPi::IPAConfigResult *result)
 {
 	if (entityControls.size() != 2) {
 		LOG(IPARPI, Error) << "No ISP or sensor controls found.";
@@ -389,6 +389,9 @@  int IPARPi::configure(const IPACameraSensorInfo &sensorInfo,
 	/* Pass the camera mode to the CamHelper to setup algorithms. */
 	helper_->SetCameraMode(mode_);
 
+	/* The pipeline handler passes out the mode's sensitivity. */
+	result->modeSensitivity = mode_.sensitivity;
+
 	if (firstStart_) {
 		/* Supply initial values for frame durations. */
 		applyFrameDurations(defaultMinFrameDuration, defaultMaxFrameDuration);
diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
index 0bdfa727..caf0030e 100644
--- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
+++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
@@ -147,7 +147,7 @@  public:
 	void frameStarted(uint32_t sequence);
 
 	int loadIPA(ipa::RPi::SensorConfig *sensorConfig);
-	int configureIPA(const CameraConfiguration *config);
+	int configureIPA(CameraConfiguration *config);
 
 	void statsMetadataComplete(uint32_t bufferId, const ControlList &controls);
 	void runIsp(uint32_t bufferId);
@@ -1250,7 +1250,7 @@  int RPiCameraData::loadIPA(ipa::RPi::SensorConfig *sensorConfig)
 	return ipa_->init(settings, sensorConfig);
 }
 
-int RPiCameraData::configureIPA(const CameraConfiguration *config)
+int RPiCameraData::configureIPA(CameraConfiguration *config)
 {
 	/* We know config must be an RPiCameraConfiguration. */
 	const RPiCameraConfiguration *rpiConfig =
@@ -1299,13 +1299,17 @@  int RPiCameraData::configureIPA(const CameraConfiguration *config)
 
 	/* Ready the IPA - it must know about the sensor resolution. */
 	ControlList controls;
+	ipa::RPi::IPAConfigResult result;
 	ret = ipa_->configure(sensorInfo_, streamConfig, entityControls, ipaConfig,
-			      &controls);
+			      &controls, &result);
 	if (ret < 0) {
 		LOG(RPI, Error) << "IPA configuration failed!";
 		return -EPIPE;
 	}
 
+	/* Store the mode sensitivity for the application. */
+	config->modeSensitivity = result.modeSensitivity;
+
 	/*
 	 * Configure the H/V flip controls based on the combination of
 	 * the sensor and user transform.