[libcamera-devel] [PATCH v3 4/8] ipa: raspberrypi: Add frame_length to DeviceStatus

[David Plowman]

Hi Naush

Thanks for this update - I can see that this change has become more
troublesome than initially expected!

On Fri, 2 Jul 2021 at 16:09, Naushir Patuck <naush at raspberrypi.com> wrote:
>
> Update the IPA to fill frame length into the DeviceStatus struct from the
> VBLANK Control value passed from DelayedControls.
>
> Update imx477 and imx219 CamHelper classes to extract the frame length from the
> embedded data buffer.
>
> Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> ---
>  src/ipa/raspberrypi/cam_helper_imx219.cpp      | 6 +++++-
>  src/ipa/raspberrypi/cam_helper_imx477.cpp      | 6 +++++-
>  src/ipa/raspberrypi/controller/device_status.h | 5 ++++-
>  src/ipa/raspberrypi/raspberrypi.cpp            | 2 ++
>  4 files changed, 16 insertions(+), 3 deletions(-)
>
> diff --git a/src/ipa/raspberrypi/cam_helper_imx219.cpp b/src/ipa/raspberrypi/cam_helper_imx219.cpp
> index 4d68e01fce71..a3caab714602 100644
> --- a/src/ipa/raspberrypi/cam_helper_imx219.cpp
> +++ b/src/ipa/raspberrypi/cam_helper_imx219.cpp
> @@ -30,7 +30,10 @@ using namespace RPiController;
>  constexpr uint32_t gainReg = 0x157;
>  constexpr uint32_t expHiReg = 0x15a;
>  constexpr uint32_t expLoReg = 0x15b;
> -constexpr std::initializer_list<uint32_t> registerList [[maybe_unused]] = { expHiReg, expLoReg, gainReg };
> +constexpr uint32_t frameLengthHiReg = 0x160;
> +constexpr uint32_t frameLengthLoReg = 0x161;
> +constexpr std::initializer_list<uint32_t> registerList [[maybe_unused]]
> +       = { expHiReg, expLoReg, gainReg, frameLengthHiReg, frameLengthLoReg };

Didn't we have a thing about these registers having to be in numerical
order? So gainReg would have to go first. Though I'm assuming that we
still aren't using metadata for the imx219 so it doesn't actually
matter - but we should probably get it the right way round.

(Should probably have spotted that on a previous review - oh well, sorry!)

>
>  class CamHelperImx219 : public CamHelper
>  {
> @@ -93,6 +96,7 @@ void CamHelperImx219::PopulateMetadata(const MdParser::RegisterMap &registers,
>
>         deviceStatus.shutter_speed = Exposure(registers.at(expHiReg) * 256 + registers.at(expLoReg));
>         deviceStatus.analogue_gain = Gain(registers.at(gainReg));
> +       deviceStatus.frame_length = registers.at(frameLengthHiReg) * 256 + registers.at(frameLengthLoReg);
>
>         metadata.Set("device.status", deviceStatus);
>  }
> diff --git a/src/ipa/raspberrypi/cam_helper_imx477.cpp b/src/ipa/raspberrypi/cam_helper_imx477.cpp
> index 4098fde6f322..91d05d9226ff 100644
> --- a/src/ipa/raspberrypi/cam_helper_imx477.cpp
> +++ b/src/ipa/raspberrypi/cam_helper_imx477.cpp
> @@ -23,7 +23,10 @@ constexpr uint32_t expHiReg = 0x0202;
>  constexpr uint32_t expLoReg = 0x0203;
>  constexpr uint32_t gainHiReg = 0x0204;
>  constexpr uint32_t gainLoReg = 0x0205;
> -constexpr std::initializer_list<uint32_t> registerList [[maybe_unused]] = { expHiReg, expLoReg, gainHiReg, gainLoReg };
> +constexpr uint32_t frameLengthHiReg = 0x0340;
> +constexpr uint32_t frameLengthLoReg = 0x0341;
> +constexpr std::initializer_list<uint32_t> registerList =
> +       { expHiReg, expLoReg, gainHiReg, gainLoReg, frameLengthHiReg, frameLengthLoReg  };
>
>  class CamHelperImx477 : public CamHelper
>  {
> @@ -81,6 +84,7 @@ void CamHelperImx477::PopulateMetadata(const MdParser::RegisterMap &registers,
>
>         deviceStatus.shutter_speed = Exposure(registers.at(expHiReg) * 256 + registers.at(expLoReg));
>         deviceStatus.analogue_gain = Gain(registers.at(gainHiReg) * 256 + registers.at(gainLoReg));
> +       deviceStatus.frame_length = registers.at(frameLengthHiReg) * 256 + registers.at(frameLengthLoReg);
>
>         metadata.Set("device.status", deviceStatus);
>  }
> diff --git a/src/ipa/raspberrypi/controller/device_status.h b/src/ipa/raspberrypi/controller/device_status.h
> index e2511d19b96d..916471ab258b 100644
> --- a/src/ipa/raspberrypi/controller/device_status.h
> +++ b/src/ipa/raspberrypi/controller/device_status.h
> @@ -17,7 +17,7 @@
>
>  struct DeviceStatus {
>         DeviceStatus()
> -               : shutter_speed(std::chrono::seconds(0)), analogue_gain(0.0),
> +               : shutter_speed(std::chrono::seconds(0)), frame_length(0), analogue_gain(0.0),
>                   lens_position(0.0), aperture(0.0), flash_intensity(0.0)
>         {
>         }
> @@ -28,6 +28,7 @@ struct DeviceStatus {
>
>                 out << "Exposure: " << d.shutter_speed
>                     << " Gain: " << d.analogue_gain
> +                   << " Frame Length: " << d.frame_length
>                     << " Aperture: " << d.aperture
>                     << " Lens: " << d.lens_position
>                     << " Flash: " << d.flash_intensity;
> @@ -37,6 +38,8 @@ struct DeviceStatus {
>
>         /* time shutter is open */
>         libcamera::utils::Duration shutter_speed;
> +       // frame length given in number of lines

Didn't I see a patch go by where you carefully replaced C++ comments
by C-style ones?  :)
Or perhaps that was a later patch... I can't even remember!

With those little things addressed:

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

Thanks!
David

> +       uint32_t frame_length;
>         double analogue_gain;
>         /* 1.0/distance-in-metres, or 0 if unknown */
>         double lens_position;
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> index f51c970befb5..db103a885b7a 100644
> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> @@ -1015,9 +1015,11 @@ void IPARPi::fillDeviceStatus(const ControlList &sensorControls)
>
>         int32_t exposureLines = sensorControls.get(V4L2_CID_EXPOSURE).get<int32_t>();
>         int32_t gainCode = sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>();
> +       int32_t vblank = sensorControls.get(V4L2_CID_VBLANK).get<int32_t>();
>
>         deviceStatus.shutter_speed = helper_->Exposure(exposureLines);
>         deviceStatus.analogue_gain = helper_->Gain(gainCode);
> +       deviceStatus.frame_length = mode_.height + vblank;
>
>         LOG(IPARPI, Debug) << "Metadata - " << deviceStatus;
>
> --
> 2.25.1
>