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

[Kieran Bingham]

Hi Naush,

On 09/07/2021 10:56, Naushir Patuck 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>
> Reviewed-by: David Plowman <david.plowman at raspberrypi.com>
> ---
>  src/ipa/raspberrypi/cam_helper.cpp             | 1 +
>  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 ++
>  5 files changed, 17 insertions(+), 3 deletions(-)
> 
> diff --git a/src/ipa/raspberrypi/cam_helper.cpp b/src/ipa/raspberrypi/cam_helper.cpp
> index 1ec3f03e1aa3..3c6afce7572b 100644
> --- a/src/ipa/raspberrypi/cam_helper.cpp
> +++ b/src/ipa/raspberrypi/cam_helper.cpp
> @@ -187,6 +187,7 @@ void CamHelper::parseEmbeddedData(Span<const uint8_t> buffer,
>  
>  	deviceStatus.shutter_speed = parsedDeviceStatus.shutter_speed;
>  	deviceStatus.analogue_gain = parsedDeviceStatus.analogue_gain;
> +	deviceStatus.frame_length = parsedDeviceStatus.frame_length;
>  
>  	LOG(IPARPI, Debug) << "Metadata updated - " << deviceStatus;
>  
> 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]]

is the maybe_unused still applicable?
If it's not used - what is the declaration for ?


> +	= { expHiReg, expLoReg, gainReg, frameLengthHiReg, frameLengthLoReg };
>  
>  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 efd1a5893db8..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 = { 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

It's always better to be doing this in one place isn't it ;-)


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

Mixing C / C++ comment style here ?

> +	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;
>  
>