[libcamera-devel] [PATCH] ipa: rpi: imx296: Enable embedded data

[Laurent Pinchart]

Hi Naush,

Thank you for the patch.

On Wed, Jun 21, 2023 at 01:37:12PM +0100, Naushir Patuck via libcamera-devel wrote:
> Enable embedded data parising for the imx296 sensor.

s/parising/parsing/

> However, the imx296 has a quirk where the embedded data is ahead by a
> single frame, i.e. embedded data in frame N corresponds to the image
> data in frame N+1. So make a copy of the embedded data buffer stored in
> the camera helper state, and use that copy in the next frame.

We wouldn't make a copy if this was image data, but would instead keep
the buffer until the next frame. Would it make sense to do that even if
the amount of data is smaller ?

> Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> ---
>  src/ipa/rpi/cam_helper/cam_helper_imx296.cpp | 61 +++++++++++++++++++-
>  1 file changed, 60 insertions(+), 1 deletion(-)
> 
> diff --git a/src/ipa/rpi/cam_helper/cam_helper_imx296.cpp b/src/ipa/rpi/cam_helper/cam_helper_imx296.cpp
> index ecb845e76e12..8f06981b500e 100644
> --- a/src/ipa/rpi/cam_helper/cam_helper_imx296.cpp
> +++ b/src/ipa/rpi/cam_helper/cam_helper_imx296.cpp
> @@ -7,6 +7,7 @@
>  
>  #include <algorithm>
>  #include <cmath>
> +#include <string.h>
>  #include <stddef.h>
>  
>  #include "cam_helper.h"
> @@ -15,6 +16,17 @@ using namespace RPiController;
>  using libcamera::utils::Duration;
>  using namespace std::literals::chrono_literals;
>  
> +constexpr uint32_t gainReg = 0x41ba;
> +constexpr uint32_t shsLoReg = 0x41b4;
> +constexpr uint32_t shsMidReg = 0x41b5;
> +constexpr uint32_t shsHiReg = 0x41b6;
> +constexpr uint32_t vmaxLoReg = 0x41cc;
> +constexpr uint32_t vmaxMidReg = 0x41cd;
> +constexpr uint32_t vmaxHiReg = 0x41ce;
> +constexpr uint32_t tempReg = 0x41da;
> +constexpr std::initializer_list<uint32_t> registerList =
> +	{ gainReg, shsLoReg, shsMidReg, shsHiReg, vmaxLoReg, vmaxMidReg, vmaxHiReg, tempReg };
> +
>  class CamHelperImx296 : public CamHelper
>  {
>  public:
> @@ -23,8 +35,12 @@ public:
>  	double gain(uint32_t gainCode) const override;
>  	uint32_t exposureLines(const Duration exposure, const Duration lineLength) const override;
>  	Duration exposure(uint32_t exposureLines, const Duration lineLength) const override;
> +	void prepare(libcamera::Span<const uint8_t> buffer, Metadata &metadata) override;
>  	void getDelays(int &exposureDelay, int &gainDelay,
>  		       int &vblankDelay, int &hblankDelay) const override;
> +	bool sensorEmbeddedDataPresent() const override;
> +	void populateMetadata(const MdParser::RegisterMap &registers,
> +			      Metadata &metadata) const override;
>  
>  private:
>  	static constexpr uint32_t minExposureLines = 1;
> @@ -36,10 +52,13 @@ private:
>  	 * in units of lines.
>  	 */
>  	static constexpr int frameIntegrationDiff = 4;
> +
> +	std::unique_ptr<uint8_t[]> lastEmbeddedBuffer_;
> +	libcamera::Span<uint8_t> embeddedBufferSpan_;
>  };
>  
>  CamHelperImx296::CamHelperImx296()
> -	: CamHelper(nullptr, frameIntegrationDiff)
> +	: CamHelper(std::make_unique<MdParserSmia>(registerList), frameIntegrationDiff)
>  {
>  }
>  
> @@ -66,6 +85,24 @@ Duration CamHelperImx296::exposure(uint32_t exposureLines,
>  	return std::max<uint32_t>(minExposureLines, exposureLines) * timePerLine + 14.26us;
>  }
>  
> +void CamHelperImx296::prepare(libcamera::Span<const uint8_t> buffer, Metadata &metadata)
> +{
> +	/*
> +	 * The imx296 embedded data is ahead by a single frame, i.e. embedded
> +	 * data in frame N corresponds to the image data in frame N+1. So make
> +	 * a copy of the embedded data buffer and use it as normal for the next
> +	 * frame.
> +	 */
> +	CamHelper::prepare(embeddedBufferSpan_, metadata);
> +
> +	if (embeddedBufferSpan_.size() != buffer.size()) {
> +		lastEmbeddedBuffer_ = std::make_unique<uint8_t[]>(buffer.size());
> +		embeddedBufferSpan_ = libcamera::Span(lastEmbeddedBuffer_.get(), buffer.size());
> +	}
> +
> +	memcpy(embeddedBufferSpan_.data(), buffer.data(), embeddedBufferSpan_.size());

If you made lastEmbeddedBuffer_ a std::vector<uint8_t> you could
simplify this code and drop embeddedBufferSpan_.

> +}
> +
>  void CamHelperImx296::getDelays(int &exposureDelay, int &gainDelay,
>  				int &vblankDelay, int &hblankDelay) const
>  {
> @@ -75,6 +112,28 @@ void CamHelperImx296::getDelays(int &exposureDelay, int &gainDelay,
>  	hblankDelay = 2;
>  }
>  
> +bool CamHelperImx296::sensorEmbeddedDataPresent() const
> +{
> +	return true;
> +}
> +
> +void CamHelperImx296::populateMetadata(const MdParser::RegisterMap &registers,
> +				       Metadata &metadata) const
> +{
> +	uint32_t shs = registers.at(shsLoReg) + (registers.at(shsMidReg) << 8) +
> +		       (registers.at(shsHiReg) << 16);
> +	uint32_t vmax = registers.at(vmaxLoReg) + (registers.at(vmaxMidReg) << 8) +
> +			(registers.at(vmaxHiReg) << 16);
> +
> +	DeviceStatus deviceStatus;
> +	deviceStatus.lineLength = timePerLine;
> +	deviceStatus.frameLength = vmax;
> +	deviceStatus.shutterSpeed = exposure(vmax - shs, timePerLine);
> +	deviceStatus.analogueGain = gain(registers.at(gainReg));
> +
> +	metadata.set("device.status", deviceStatus);
> +}
> +
>  static CamHelper *create()
>  {
>  	return new CamHelperImx296();

-- 
Regards,

Laurent Pinchart