[libcamera-devel,v6,4/8] ipa: raspberrypi: Add frame_length to DeviceStatus
diff mbox series

Message ID 20210709145825.2943443-5-naush@raspberrypi.com
State Accepted
Headers show
Series
  • ipa: raspberrypi: Allow long exposure modes for imx477.
Related show

Commit Message

Naushir Patuck July 9, 2021, 2:58 p.m. UTC 
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@raspberrypi.com>
Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.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(-)

Comments

Jacopo Mondi July 9, 2021, 4:15 p.m. UTC | #1 
Hi Naush,

On Fri, Jul 09, 2021 at 03:58:21PM +0100, 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@raspberrypi.com>
> Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

Seems good!

Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks
   j
> ---
>  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]]
> +	= { 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..7bb8852375bd 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 */
> +	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
>
Laurent Pinchart July 11, 2021, 6:58 p.m. UTC | #2 
Hi Naush,

Thank you for the patch.

On Fri, Jul 09, 2021 at 03:58:21PM +0100, Naushir Patuck wrote:
> Update the IPA to fill frame length into the DeviceStatus struct from the
> VBLANK Control value passed from DelayedControls.

That's only for the case where no embedded data is present, right ? The
following commit message would seem clearer to me:

Store the frame length into the DeviceStatus struct. The value is
extracted from embedded data when available, or calculated from the
VBLANK value passed from DelayedControls otherwise.

> Update imx477 and imx219 CamHelper classes to extract the frame length from the
> embedded data buffer.
> 
> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.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]]
> +	= { 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..7bb8852375bd 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),

A line wrap may be nice.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

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

Patch
diff mbox series 

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]]
+	= { 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..7bb8852375bd 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 */
+	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;