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

Message ID 20210702150940.226941-5-naush@raspberrypi.com
State Superseded
Headers show
Series
  • Raspberry Pi: Enable imx477 long exposure modes
Related show

Commit Message

Naushir Patuck July 2, 2021, 3:09 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>
---
 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(-)

Comments

David Plowman July 2, 2021, 4:09 p.m. UTC | #1 
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@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@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@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
>
Naushir Patuck July 2, 2021, 4:17 p.m. UTC | #2 
Hi David,

Thank you for your feedback.

On Fri, 2 Jul 2021 at 17:09, David Plowman <david.plowman@raspberrypi.com>
wrote:

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

Indeed it has :(
But frame length is probably important enough that we should record it down.


> On Fri, 2 Jul 2021 at 16:09, Naushir Patuck <naush@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@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.
>

With my last set of updates (i.e. C++ifying and generalising the SMIA
parser),
these registers can be listed in any order.


>
> (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!
>

Groan, yes, the first patch did this, and I went and used C++ comments
in this one.  Will fix it in the next revision.

Regards,
Naush


>
> With those little things addressed:
>
> Reviewed-by: David Plowman <david.plowman@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
> >
>

Patch
diff mbox series 

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