[libcamera-devel] [PATCH v3 2/2] libcamera: raspberrypi: Fetch correct value for SensorSensitivity

[Kieran Bingham]

Quoting Naushir Patuck via libcamera-devel (2022-04-21 16:11:17)
> From: David Plowman <david.plowman at raspberrypi.com>
> 
> 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 camera properties.
> 
> Signed-off-by: David Plowman <david.plowman at raspberrypi.com>
> Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> ---
>  include/libcamera/ipa/raspberrypi.mojom            |  7 ++++++-
>  src/ipa/raspberrypi/raspberrypi.cpp                |  7 +++++--
>  src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 12 ++++++++----
>  3 files changed, 19 insertions(+), 7 deletions(-)
> 
> diff --git a/include/libcamera/ipa/raspberrypi.mojom b/include/libcamera/ipa/raspberrypi.mojom
> index 5a228b75cd2f..a60c3bb43d3c 100644
> --- a/include/libcamera/ipa/raspberrypi.mojom
> +++ b/include/libcamera/ipa/raspberrypi.mojom
> @@ -38,6 +38,10 @@ struct IPAConfig {
>         libcamera.SharedFD lsTableHandle;
>  };
>  
> +struct IPAConfigResult {
> +       float modeSensitivity;
> +};
> +
>  struct StartConfig {
>         libcamera.ControlList controls;
>         int32 dropFrameCount;
> @@ -58,6 +62,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.
> @@ -72,7 +77,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 5efd051bdd13..fd66cfeee087 100644
> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> @@ -99,7 +99,7 @@ public:
>                       const std::map<unsigned int, IPAStream> &streamConfig,
>                       const std::map<unsigned int, ControlInfoMap> &entityControls,
>                       const IPAConfig &data,
> -                     ControlList *controls) override;
> +                     ControlList *controls, 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;
> @@ -344,7 +344,7 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo,
>                       [[maybe_unused]] const std::map<unsigned int, IPAStream> &streamConfig,
>                       const std::map<unsigned int, ControlInfoMap> &entityControls,
>                       const IPAConfig &ipaConfig,
> -                     ControlList *controls)
> +                     ControlList *controls, IPAConfigResult *result)
>  {
>         if (entityControls.size() != 2) {
>                 LOG(IPARPI, Error) << "No ISP or sensor controls found.";
> @@ -404,6 +404,9 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo,
>          */
>         ControlList ctrls(sensorCtrls_);
>  
> +       /* 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 acc0becaa5c0..b2489005120c 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -200,7 +200,7 @@ public:
>         void frameStarted(uint32_t sequence);
>  
>         int loadIPA(ipa::RPi::SensorConfig *sensorConfig);
> -       int configureIPA(const CameraConfiguration *config);
> +       int configureIPA(const CameraConfiguration *config, ipa::RPi::IPAConfigResult *result);
>  
>         void enumerateVideoDevices(MediaLink *link);
>  
> @@ -898,7 +898,8 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)
>  
>         data->isp_[Isp::Input].dev()->setSelection(V4L2_SEL_TGT_CROP, &crop);
>  
> -       ret = data->configureIPA(config);
> +       ipa::RPi::IPAConfigResult result;
> +       ret = data->configureIPA(config, &result);
>         if (ret)
>                 LOG(RPI, Error) << "Failed to configure the IPA: " << ret;
>  
> @@ -937,6 +938,9 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)
>          */
>         data->properties_.set(properties::ScalerCropMaximum, data->sensorInfo_.analogCrop);
>  
> +       /* Store the mode sensitivity for the application. */
> +       data->properties_.set(properties::SensorSensitivity, result.modeSensitivity);
> +

I think this is good. I think in the future we might need better ways to
determine what 'would' be the sensitivity when validating a
configuration - but that would involve more rework, and we know we need
to do a bigger rework on configuration phase - so I think this is a good
way forwards now.

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

>         /* Setup the Video Mux/Bridge entities. */
>         for (auto &[device, link] : data->bridgeDevices_) {
>                 /*
> @@ -1528,7 +1532,7 @@ int RPiCameraData::loadIPA(ipa::RPi::SensorConfig *sensorConfig)
>         return ipa_->init(settings, sensorConfig);
>  }
>  
> -int RPiCameraData::configureIPA(const CameraConfiguration *config)
> +int RPiCameraData::configureIPA(const CameraConfiguration *config, ipa::RPi::IPAConfigResult *result)
>  {
>         std::map<unsigned int, IPAStream> streamConfig;
>         std::map<unsigned int, ControlInfoMap> entityControls;
> @@ -1574,7 +1578,7 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config)
>         /* Ready the IPA - it must know about the sensor resolution. */
>         ControlList controls;
>         ret = ipa_->configure(sensorInfo_, streamConfig, entityControls, ipaConfig,
> -                             &controls);
> +                             &controls, result);
>         if (ret < 0) {
>                 LOG(RPI, Error) << "IPA configuration failed!";
>                 return -EPIPE;
> -- 
> 2.25.1
>