[libcamera-devel] [PATCH v2 2/2] libcamera: raspberrypi: Fetch correct value for sensor's modeSensitivity

[Kieran Bingham]

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 at 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 at 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
>