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

15699 diff mbox series

From: David Plowman <david.plowman@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@raspberrypi.com>
Signed-off-by: Naushir Patuck <naush@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(-)

Quoting Naushir Patuck via libcamera-devel (2022-04-21 16:11:17)
> From: David Plowman <david.plowman@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@raspberrypi.com>
> Signed-off-by: Naushir Patuck <naush@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@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
>