[{"id":32834,"web_url":"https://patchwork.libcamera.org/comment/32834/","msgid":"<20241217164437.GG20432@pendragon.ideasonboard.com>","date":"2024-12-17T16:44:37","subject":"Re: [PATCH 2/2] libcamera: rpi: Draw sensor delays from\n\tCameraSensorProperties","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Wed, Nov 27, 2024 at 01:32:33PM +0000, Daniel Scally wrote:\n> Now that we have camera sensor control application delay values in\n> the CameraSensorProperties class, remove the duplicated definitions\n> in the RPi IPA's CameraSensorHelpers and update the pipeline handler\n> to use the values from CameraSensorProperties.\n> \n> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>\n\nReviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n\n> ---\n>  include/libcamera/ipa/raspberrypi.mojom           |  4 ----\n>  src/ipa/rpi/cam_helper/cam_helper.cpp             | 13 -------------\n>  src/ipa/rpi/cam_helper/cam_helper.h               |  7 -------\n>  src/ipa/rpi/cam_helper/cam_helper_imx283.cpp      | 12 ------------\n>  src/ipa/rpi/cam_helper/cam_helper_imx290.cpp      | 11 -----------\n>  src/ipa/rpi/cam_helper/cam_helper_imx296.cpp      | 11 -----------\n>  src/ipa/rpi/cam_helper/cam_helper_imx477.cpp      | 11 -----------\n>  src/ipa/rpi/cam_helper/cam_helper_imx519.cpp      | 11 -----------\n>  src/ipa/rpi/cam_helper/cam_helper_imx708.cpp      | 11 -----------\n>  src/ipa/rpi/cam_helper/cam_helper_ov5647.cpp      | 15 ---------------\n>  src/ipa/rpi/cam_helper/cam_helper_ov64a40.cpp     | 12 ------------\n>  src/ipa/rpi/cam_helper/cam_helper_ov7251.cpp      | 12 ------------\n>  src/ipa/rpi/cam_helper/cam_helper_ov9281.cpp      | 12 ------------\n>  src/ipa/rpi/common/ipa_base.cpp                   | 14 ++------------\n>  .../pipeline/rpi/common/pipeline_base.cpp         |  9 +++++----\n>  15 files changed, 7 insertions(+), 158 deletions(-)\n> \n> diff --git a/include/libcamera/ipa/raspberrypi.mojom b/include/libcamera/ipa/raspberrypi.mojom\n> index 0b92587d..e30c70bd 100644\n> --- a/include/libcamera/ipa/raspberrypi.mojom\n> +++ b/include/libcamera/ipa/raspberrypi.mojom\n> @@ -12,10 +12,6 @@ import \"include/libcamera/ipa/core.mojom\";\n>  const uint32 MaxLsGridSize = 0x8000;\n>  \n>  struct SensorConfig {\n> -\tuint32 gainDelay;\n> -\tuint32 exposureDelay;\n> -\tuint32 vblankDelay;\n> -\tuint32 hblankDelay;\n>  \tuint32 sensorMetadata;\n>  };\n>  \n> diff --git a/src/ipa/rpi/cam_helper/cam_helper.cpp b/src/ipa/rpi/cam_helper/cam_helper.cpp\n> index 6493e882..8c720652 100644\n> --- a/src/ipa/rpi/cam_helper/cam_helper.cpp\n> +++ b/src/ipa/rpi/cam_helper/cam_helper.cpp\n> @@ -156,19 +156,6 @@ void CamHelper::setCameraMode(const CameraMode &mode)\n>  \t}\n>  }\n>  \n> -void CamHelper::getDelays(int &exposureDelay, int &gainDelay,\n> -\t\t\t  int &vblankDelay, int &hblankDelay) const\n> -{\n> -\t/*\n> -\t * These values are correct for many sensors. Other sensors will\n> -\t * need to over-ride this function.\n> -\t */\n> -\texposureDelay = 2;\n> -\tgainDelay = 1;\n> -\tvblankDelay = 2;\n> -\thblankDelay = 2;\n> -}\n> -\n>  bool CamHelper::sensorEmbeddedDataPresent() const\n>  {\n>  \treturn false;\n> diff --git a/src/ipa/rpi/cam_helper/cam_helper.h b/src/ipa/rpi/cam_helper/cam_helper.h\n> index 4a4ab5e6..29371bdb 100644\n> --- a/src/ipa/rpi/cam_helper/cam_helper.h\n> +++ b/src/ipa/rpi/cam_helper/cam_helper.h\n> @@ -36,11 +36,6 @@ namespace RPiController {\n>   * exposure time, and to convert between the sensor's gain codes and actual\n>   * gains.\n>   *\n> - * A function to return the number of frames of delay between updating exposure,\n> - * analogue gain and vblanking, and for the changes to take effect. For many\n> - * sensors these take the values 2, 1 and 2 respectively, but sensors that are\n> - * different will need to over-ride the default function provided.\n> - *\n>   * A function to query if the sensor outputs embedded data that can be parsed.\n>   *\n>   * A function to return the sensitivity of a given camera mode.\n> @@ -91,8 +86,6 @@ public:\n>  \tlibcamera::utils::Duration lineLengthPckToDuration(uint32_t lineLengthPck) const;\n>  \tvirtual uint32_t gainCode(double gain) const = 0;\n>  \tvirtual double gain(uint32_t gainCode) const = 0;\n> -\tvirtual void getDelays(int &exposureDelay, int &gainDelay,\n> -\t\t\t       int &vblankDelay, int &hblankDelay) const;\n>  \tvirtual bool sensorEmbeddedDataPresent() const;\n>  \tvirtual double getModeSensitivity(const CameraMode &mode) const;\n>  \tvirtual unsigned int hideFramesStartup() const;\n> diff --git a/src/ipa/rpi/cam_helper/cam_helper_imx283.cpp b/src/ipa/rpi/cam_helper/cam_helper_imx283.cpp\n> index cb0be72a..efc03193 100644\n> --- a/src/ipa/rpi/cam_helper/cam_helper_imx283.cpp\n> +++ b/src/ipa/rpi/cam_helper/cam_helper_imx283.cpp\n> @@ -17,8 +17,6 @@ public:\n>  \tCamHelperImx283();\n>  \tuint32_t gainCode(double gain) const override;\n>  \tdouble gain(uint32_t gainCode) const override;\n> -\tvoid getDelays(int &exposureDelay, int &gainDelay,\n> -\t\t       int &vblankDelay, int &hblankDelay) const override;\n>  \tunsigned int hideFramesModeSwitch() const override;\n>  \n>  private:\n> @@ -49,16 +47,6 @@ double CamHelperImx283::gain(uint32_t gainCode) const\n>  \treturn static_cast<double>(2048.0 / (2048 - gainCode));\n>  }\n>  \n> -void CamHelperImx283::getDelays(int &exposureDelay, int &gainDelay,\n> -\t\t\t\tint &vblankDelay, int &hblankDelay) const\n> -{\n> -\t/* The driver appears to behave as follows: */\n> -\texposureDelay = 2;\n> -\tgainDelay = 2;\n> -\tvblankDelay = 2;\n> -\thblankDelay = 2;\n> -}\n> -\n>  unsigned int CamHelperImx283::hideFramesModeSwitch() const\n>  {\n>  \t/* After a mode switch, we seem to get 1 bad frame. */\n> diff --git a/src/ipa/rpi/cam_helper/cam_helper_imx290.cpp b/src/ipa/rpi/cam_helper/cam_helper_imx290.cpp\n> index 3b87751e..c1aa8528 100644\n> --- a/src/ipa/rpi/cam_helper/cam_helper_imx290.cpp\n> +++ b/src/ipa/rpi/cam_helper/cam_helper_imx290.cpp\n> @@ -17,8 +17,6 @@ public:\n>  \tCamHelperImx290();\n>  \tuint32_t gainCode(double gain) const override;\n>  \tdouble gain(uint32_t gainCode) const override;\n> -\tvoid getDelays(int &exposureDelay, int &gainDelay,\n> -\t\t       int &vblankDelay, int &hblankDelay) const override;\n>  \tunsigned int hideFramesStartup() const override;\n>  \tunsigned int hideFramesModeSwitch() const override;\n>  \n> @@ -46,15 +44,6 @@ double CamHelperImx290::gain(uint32_t gainCode) const\n>  \treturn std::pow(10, 0.015 * gainCode);\n>  }\n>  \n> -void CamHelperImx290::getDelays(int &exposureDelay, int &gainDelay,\n> -\t\t\t\tint &vblankDelay, int &hblankDelay) const\n> -{\n> -\texposureDelay = 2;\n> -\tgainDelay = 2;\n> -\tvblankDelay = 2;\n> -\thblankDelay = 2;\n> -}\n> -\n>  unsigned int CamHelperImx290::hideFramesStartup() const\n>  {\n>  \t/* On startup, we seem to get 1 bad frame. */\n> diff --git a/src/ipa/rpi/cam_helper/cam_helper_imx296.cpp b/src/ipa/rpi/cam_helper/cam_helper_imx296.cpp\n> index d4a4fa79..ac7ee2ea 100644\n> --- a/src/ipa/rpi/cam_helper/cam_helper_imx296.cpp\n> +++ b/src/ipa/rpi/cam_helper/cam_helper_imx296.cpp\n> @@ -23,8 +23,6 @@ public:\n>  \tdouble gain(uint32_t gainCode) const override;\n>  \tuint32_t exposureLines(const Duration exposure, const Duration lineLength) const override;\n>  \tDuration exposure(uint32_t exposureLines, const Duration lineLength) const override;\n> -\tvoid getDelays(int &exposureDelay, int &gainDelay,\n> -\t\t       int &vblankDelay, int &hblankDelay) const override;\n>  \n>  private:\n>  \tstatic constexpr uint32_t minExposureLines = 1;\n> @@ -66,15 +64,6 @@ Duration CamHelperImx296::exposure(uint32_t exposureLines,\n>  \treturn std::max<uint32_t>(minExposureLines, exposureLines) * timePerLine + 14.26us;\n>  }\n>  \n> -void CamHelperImx296::getDelays(int &exposureDelay, int &gainDelay,\n> -\t\t\t\tint &vblankDelay, int &hblankDelay) const\n> -{\n> -\texposureDelay = 2;\n> -\tgainDelay = 2;\n> -\tvblankDelay = 2;\n> -\thblankDelay = 2;\n> -}\n> -\n>  static CamHelper *create()\n>  {\n>  \treturn new CamHelperImx296();\n> diff --git a/src/ipa/rpi/cam_helper/cam_helper_imx477.cpp b/src/ipa/rpi/cam_helper/cam_helper_imx477.cpp\n> index a53c40cd..a72ac67d 100644\n> --- a/src/ipa/rpi/cam_helper/cam_helper_imx477.cpp\n> +++ b/src/ipa/rpi/cam_helper/cam_helper_imx477.cpp\n> @@ -51,8 +51,6 @@ public:\n>  \tvoid prepare(libcamera::Span<const uint8_t> buffer, Metadata &metadata) override;\n>  \tstd::pair<uint32_t, uint32_t> getBlanking(Duration &exposure, Duration minFrameDuration,\n>  \t\t\t\t\t\t  Duration maxFrameDuration) const override;\n> -\tvoid getDelays(int &exposureDelay, int &gainDelay,\n> -\t\t       int &vblankDelay, int &hblankDelay) const override;\n>  \tbool sensorEmbeddedDataPresent() const override;\n>  \n>  private:\n> @@ -159,15 +157,6 @@ std::pair<uint32_t, uint32_t> CamHelperImx477::getBlanking(Duration &exposure,\n>  \treturn { frameLength - mode_.height, hblank };\n>  }\n>  \n> -void CamHelperImx477::getDelays(int &exposureDelay, int &gainDelay,\n> -\t\t\t\tint &vblankDelay, int &hblankDelay) const\n> -{\n> -\texposureDelay = 2;\n> -\tgainDelay = 2;\n> -\tvblankDelay = 3;\n> -\thblankDelay = 3;\n> -}\n> -\n>  bool CamHelperImx477::sensorEmbeddedDataPresent() const\n>  {\n>  \treturn true;\n> diff --git a/src/ipa/rpi/cam_helper/cam_helper_imx519.cpp b/src/ipa/rpi/cam_helper/cam_helper_imx519.cpp\n> index 2ff08653..10cbea48 100644\n> --- a/src/ipa/rpi/cam_helper/cam_helper_imx519.cpp\n> +++ b/src/ipa/rpi/cam_helper/cam_helper_imx519.cpp\n> @@ -51,8 +51,6 @@ public:\n>  \tvoid prepare(libcamera::Span<const uint8_t> buffer, Metadata &metadata) override;\n>  \tstd::pair<uint32_t, uint32_t> getBlanking(Duration &exposure, Duration minFrameDuration,\n>  \t\t\t\t\t\t  Duration maxFrameDuration) const override;\n> -\tvoid getDelays(int &exposureDelay, int &gainDelay,\n> -\t\t       int &vblankDelay, int &hblankDelay) const override;\n>  \tbool sensorEmbeddedDataPresent() const override;\n>  \n>  private:\n> @@ -159,15 +157,6 @@ std::pair<uint32_t, uint32_t> CamHelperImx519::getBlanking(Duration &exposure,\n>  \treturn { frameLength - mode_.height, hblank };\n>  }\n>  \n> -void CamHelperImx519::getDelays(int &exposureDelay, int &gainDelay,\n> -\t\t\t\tint &vblankDelay, int &hblankDelay) const\n> -{\n> -\texposureDelay = 2;\n> -\tgainDelay = 2;\n> -\tvblankDelay = 3;\n> -\thblankDelay = 3;\n> -}\n> -\n>  bool CamHelperImx519::sensorEmbeddedDataPresent() const\n>  {\n>  \treturn true;\n> diff --git a/src/ipa/rpi/cam_helper/cam_helper_imx708.cpp b/src/ipa/rpi/cam_helper/cam_helper_imx708.cpp\n> index ec83d9fd..24ffc846 100644\n> --- a/src/ipa/rpi/cam_helper/cam_helper_imx708.cpp\n> +++ b/src/ipa/rpi/cam_helper/cam_helper_imx708.cpp\n> @@ -54,8 +54,6 @@ public:\n>  \tvoid process(StatisticsPtr &stats, Metadata &metadata) override;\n>  \tstd::pair<uint32_t, uint32_t> getBlanking(Duration &exposure, Duration minFrameDuration,\n>  \t\t\t\t\t\t  Duration maxFrameDuration) const override;\n> -\tvoid getDelays(int &exposureDelay, int &gainDelay,\n> -\t\t       int &vblankDelay, int &hblankDelay) const override;\n>  \tbool sensorEmbeddedDataPresent() const override;\n>  \tdouble getModeSensitivity(const CameraMode &mode) const override;\n>  \tunsigned int hideFramesModeSwitch() const override;\n> @@ -208,15 +206,6 @@ std::pair<uint32_t, uint32_t> CamHelperImx708::getBlanking(Duration &exposure,\n>  \treturn { frameLength - mode_.height, hblank };\n>  }\n>  \n> -void CamHelperImx708::getDelays(int &exposureDelay, int &gainDelay,\n> -\t\t\t\tint &vblankDelay, int &hblankDelay) const\n> -{\n> -\texposureDelay = 2;\n> -\tgainDelay = 2;\n> -\tvblankDelay = 3;\n> -\thblankDelay = 3;\n> -}\n> -\n>  bool CamHelperImx708::sensorEmbeddedDataPresent() const\n>  {\n>  \treturn true;\n> diff --git a/src/ipa/rpi/cam_helper/cam_helper_ov5647.cpp b/src/ipa/rpi/cam_helper/cam_helper_ov5647.cpp\n> index c30b017c..40d6b6d7 100644\n> --- a/src/ipa/rpi/cam_helper/cam_helper_ov5647.cpp\n> +++ b/src/ipa/rpi/cam_helper/cam_helper_ov5647.cpp\n> @@ -17,8 +17,6 @@ public:\n>  \tCamHelperOv5647();\n>  \tuint32_t gainCode(double gain) const override;\n>  \tdouble gain(uint32_t gainCode) const override;\n> -\tvoid getDelays(int &exposureDelay, int &gainDelay,\n> -\t\t       int &vblankDelay, int &hblankDelay) const override;\n>  \tunsigned int hideFramesStartup() const override;\n>  \tunsigned int hideFramesModeSwitch() const override;\n>  \tunsigned int mistrustFramesStartup() const override;\n> @@ -52,19 +50,6 @@ double CamHelperOv5647::gain(uint32_t gainCode) const\n>  \treturn static_cast<double>(gainCode) / 16.0;\n>  }\n>  \n> -void CamHelperOv5647::getDelays(int &exposureDelay, int &gainDelay,\n> -\t\t\t\tint &vblankDelay, int &hblankDelay) const\n> -{\n> -\t/*\n> -\t * We run this sensor in a mode where the gain delay is bumped up to\n> -\t * 2. It seems to be the only way to make the delays \"predictable\".\n> -\t */\n> -\texposureDelay = 2;\n> -\tgainDelay = 2;\n> -\tvblankDelay = 2;\n> -\thblankDelay = 2;\n> -}\n> -\n>  unsigned int CamHelperOv5647::hideFramesStartup() const\n>  {\n>  \t/*\n> diff --git a/src/ipa/rpi/cam_helper/cam_helper_ov64a40.cpp b/src/ipa/rpi/cam_helper/cam_helper_ov64a40.cpp\n> index a8efd389..980495a8 100644\n> --- a/src/ipa/rpi/cam_helper/cam_helper_ov64a40.cpp\n> +++ b/src/ipa/rpi/cam_helper/cam_helper_ov64a40.cpp\n> @@ -18,8 +18,6 @@ public:\n>  \tCamHelperOv64a40();\n>  \tuint32_t gainCode(double gain) const override;\n>  \tdouble gain(uint32_t gainCode) const override;\n> -\tvoid getDelays(int &exposureDelay, int &gainDelay,\n> -\t\t       int &vblankDelay, int &hblankDelay) const override;\n>  \tdouble getModeSensitivity(const CameraMode &mode) const override;\n>  \n>  private:\n> @@ -45,16 +43,6 @@ double CamHelperOv64a40::gain(uint32_t gainCode) const\n>  \treturn static_cast<double>(gainCode) / 128.0;\n>  }\n>  \n> -void CamHelperOv64a40::getDelays(int &exposureDelay, int &gainDelay,\n> -\t\t\t\t int &vblankDelay, int &hblankDelay) const\n> -{\n> -\t/* The driver appears to behave as follows: */\n> -\texposureDelay = 2;\n> -\tgainDelay = 2;\n> -\tvblankDelay = 2;\n> -\thblankDelay = 2;\n> -}\n> -\n>  double CamHelperOv64a40::getModeSensitivity(const CameraMode &mode) const\n>  {\n>  \tif (mode.binX >= 2 && mode.scaleX >= 4) {\n> diff --git a/src/ipa/rpi/cam_helper/cam_helper_ov7251.cpp b/src/ipa/rpi/cam_helper/cam_helper_ov7251.cpp\n> index 7b12c445..fc7b999f 100644\n> --- a/src/ipa/rpi/cam_helper/cam_helper_ov7251.cpp\n> +++ b/src/ipa/rpi/cam_helper/cam_helper_ov7251.cpp\n> @@ -17,8 +17,6 @@ public:\n>  \tCamHelperOv7251();\n>  \tuint32_t gainCode(double gain) const override;\n>  \tdouble gain(uint32_t gainCode) const override;\n> -\tvoid getDelays(int &exposureDelay, int &gainDelay,\n> -\t\t       int &vblankDelay, int &hblankDelay) const override;\n>  \n>  private:\n>  \t/*\n> @@ -48,16 +46,6 @@ double CamHelperOv7251::gain(uint32_t gainCode) const\n>  \treturn static_cast<double>(gainCode) / 16.0;\n>  }\n>  \n> -void CamHelperOv7251::getDelays(int &exposureDelay, int &gainDelay,\n> -\t\t\t\tint &vblankDelay, int &hblankDelay) const\n> -{\n> -\t/* The driver appears to behave as follows: */\n> -\texposureDelay = 2;\n> -\tgainDelay = 2;\n> -\tvblankDelay = 2;\n> -\thblankDelay = 2;\n> -}\n> -\n>  static CamHelper *create()\n>  {\n>  \treturn new CamHelperOv7251();\n> diff --git a/src/ipa/rpi/cam_helper/cam_helper_ov9281.cpp b/src/ipa/rpi/cam_helper/cam_helper_ov9281.cpp\n> index a65c8ac0..e56868e4 100644\n> --- a/src/ipa/rpi/cam_helper/cam_helper_ov9281.cpp\n> +++ b/src/ipa/rpi/cam_helper/cam_helper_ov9281.cpp\n> @@ -17,8 +17,6 @@ public:\n>  \tCamHelperOv9281();\n>  \tuint32_t gainCode(double gain) const override;\n>  \tdouble gain(uint32_t gainCode) const override;\n> -\tvoid getDelays(int &exposureDelay, int &gainDelay,\n> -\t\t       int &vblankDelay, int &hblankDelay) const override;\n>  \n>  private:\n>  \t/*\n> @@ -48,16 +46,6 @@ double CamHelperOv9281::gain(uint32_t gainCode) const\n>  \treturn static_cast<double>(gainCode) / 16.0;\n>  }\n>  \n> -void CamHelperOv9281::getDelays(int &exposureDelay, int &gainDelay,\n> -\t\t\t\tint &vblankDelay, int &hblankDelay) const\n> -{\n> -\t/* The driver appears to behave as follows: */\n> -\texposureDelay = 2;\n> -\tgainDelay = 2;\n> -\tvblankDelay = 2;\n> -\thblankDelay = 2;\n> -}\n> -\n>  static CamHelper *create()\n>  {\n>  \treturn new CamHelperOv9281();\n> diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp\n> index 5fce17e6..45e2a1d7 100644\n> --- a/src/ipa/rpi/common/ipa_base.cpp\n> +++ b/src/ipa/rpi/common/ipa_base.cpp\n> @@ -134,18 +134,8 @@ int32_t IpaBase::init(const IPASettings &settings, const InitParams &params, Ini\n>  \t\treturn -EINVAL;\n>  \t}\n>  \n> -\t/*\n> -\t * Pass out the sensor config to the pipeline handler in order\n> -\t * to setup the staggered writer class.\n> -\t */\n> -\tint gainDelay, exposureDelay, vblankDelay, hblankDelay, sensorMetadata;\n> -\thelper_->getDelays(exposureDelay, gainDelay, vblankDelay, hblankDelay);\n> -\tsensorMetadata = helper_->sensorEmbeddedDataPresent();\n> -\n> -\tresult->sensorConfig.gainDelay = gainDelay;\n> -\tresult->sensorConfig.exposureDelay = exposureDelay;\n> -\tresult->sensorConfig.vblankDelay = vblankDelay;\n> -\tresult->sensorConfig.hblankDelay = hblankDelay;\n> +\t/* Pass out the sensor metadata to the pipeline handler */\n> +\tint sensorMetadata = helper_->sensorEmbeddedDataPresent();\n>  \tresult->sensorConfig.sensorMetadata = sensorMetadata;\n>  \n>  \t/* Load the tuning file for this sensor. */\n> diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> index 9e2d9d23..398a0280 100644\n> --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> @@ -816,11 +816,12 @@ int PipelineHandlerBase::registerCamera(std::unique_ptr<RPi::CameraData> &camera\n>  \t * Setup our delayed control writer with the sensor default\n>  \t * gain and exposure delays. Mark VBLANK for priority write.\n>  \t */\n> +\tconst CameraSensorProperties::SensorDelays &delays = data->sensor_->sensorDelays();\n>  \tstd::unordered_map<uint32_t, RPi::DelayedControls::ControlParams> params = {\n> -\t\t{ V4L2_CID_ANALOGUE_GAIN, { result.sensorConfig.gainDelay, false } },\n> -\t\t{ V4L2_CID_EXPOSURE, { result.sensorConfig.exposureDelay, false } },\n> -\t\t{ V4L2_CID_HBLANK, { result.sensorConfig.hblankDelay, false } },\n> -\t\t{ V4L2_CID_VBLANK, { result.sensorConfig.vblankDelay, true } }\n> +\t\t{ V4L2_CID_ANALOGUE_GAIN, { delays.gainDelay, false } },\n> +\t\t{ V4L2_CID_EXPOSURE, { delays.exposureDelay, false } },\n> +\t\t{ V4L2_CID_HBLANK, { delays.hblankDelay, false } },\n> +\t\t{ V4L2_CID_VBLANK, { delays.vblankDelay, true } }\n>  \t};\n>  \tdata->delayedCtrls_ = std::make_unique<RPi::DelayedControls>(data->sensor_->device(), params);\n>  \tdata->sensorMetadata_ = result.sensorConfig.sensorMetadata;","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 764F8BD1F1\n\tfor <parsemail@patchwork.libcamera.org>;\n\tTue, 17 Dec 2024 16:44:41 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9AE1A67FE9;\n\tTue, 17 Dec 2024 17:44:40 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E0B5E67FD3\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue, 17 Dec 2024 17:44:39 +0100 (CET)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 5295B75A;\n\tTue, 17 Dec 2024 17:44:02 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"smPlAbKK\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1734453842;\n\tbh=+TBm0cnLRqOg4O1grmKRpEqCL2C3wMnxj0qeI7HwCo8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=smPlAbKKyVbqYesXdSD6a5K76nDz/OU3DcwJMHTfl6zm+OcnSxO2S+s9PKpIoJmdO\n\tFBUKgKM2r1aNiliznk8M3eBSvEZlew2kNunMPqy8Wo4c7do9yUuWVbJynJ9ollunpV\n\tXPsvtvyR0y9de68aCHMBfawsgos6myaSO9rYUuiw=","Date":"Tue, 17 Dec 2024 18:44:37 +0200","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Daniel Scally <dan.scally@ideasonboard.com>","Cc":"libcamera-devel@lists.libcamera.org, david.plowman@raspberrypi.com,\n\tnaush@raspberrypi.com","Subject":"Re: [PATCH 2/2] libcamera: rpi: Draw sensor delays from\n\tCameraSensorProperties","Message-ID":"<20241217164437.GG20432@pendragon.ideasonboard.com>","References":"<20241127133233.247977-1-dan.scally@ideasonboard.com>\n\t<20241127133233.247977-3-dan.scally@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20241127133233.247977-3-dan.scally@ideasonboard.com>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":32836,"web_url":"https://patchwork.libcamera.org/comment/32836/","msgid":"<173445401236.32744.1757422668793523225@ping.linuxembedded.co.uk>","date":"2024-12-17T16:46:52","subject":"Re: [PATCH 2/2] libcamera: rpi: Draw sensor delays from\n\tCameraSensorProperties","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Daniel Scally (2024-11-27 13:32:33)\n> Now that we have camera sensor control application delay values in\n> the CameraSensorProperties class, remove the duplicated definitions\n> in the RPi IPA's CameraSensorHelpers and update the pipeline handler\n> to use the values from CameraSensorProperties.\n> \n> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>\n> ---\n>  include/libcamera/ipa/raspberrypi.mojom           |  4 ----\n>  src/ipa/rpi/cam_helper/cam_helper.cpp             | 13 -------------\n>  src/ipa/rpi/cam_helper/cam_helper.h               |  7 -------\n>  src/ipa/rpi/cam_helper/cam_helper_imx283.cpp      | 12 ------------\n>  src/ipa/rpi/cam_helper/cam_helper_imx290.cpp      | 11 -----------\n>  src/ipa/rpi/cam_helper/cam_helper_imx296.cpp      | 11 -----------\n>  src/ipa/rpi/cam_helper/cam_helper_imx477.cpp      | 11 -----------\n>  src/ipa/rpi/cam_helper/cam_helper_imx519.cpp      | 11 -----------\n>  src/ipa/rpi/cam_helper/cam_helper_imx708.cpp      | 11 -----------\n>  src/ipa/rpi/cam_helper/cam_helper_ov5647.cpp      | 15 ---------------\n>  src/ipa/rpi/cam_helper/cam_helper_ov64a40.cpp     | 12 ------------\n>  src/ipa/rpi/cam_helper/cam_helper_ov7251.cpp      | 12 ------------\n>  src/ipa/rpi/cam_helper/cam_helper_ov9281.cpp      | 12 ------------\n>  src/ipa/rpi/common/ipa_base.cpp                   | 14 ++------------\n>  .../pipeline/rpi/common/pipeline_base.cpp         |  9 +++++----\n>  15 files changed, 7 insertions(+), 158 deletions(-)\n> \n> diff --git a/include/libcamera/ipa/raspberrypi.mojom b/include/libcamera/ipa/raspberrypi.mojom\n> index 0b92587d..e30c70bd 100644\n> --- a/include/libcamera/ipa/raspberrypi.mojom\n> +++ b/include/libcamera/ipa/raspberrypi.mojom\n> @@ -12,10 +12,6 @@ import \"include/libcamera/ipa/core.mojom\";\n>  const uint32 MaxLsGridSize = 0x8000;\n>  \n>  struct SensorConfig {\n> -       uint32 gainDelay;\n> -       uint32 exposureDelay;\n> -       uint32 vblankDelay;\n> -       uint32 hblankDelay;\n>         uint32 sensorMetadata;\n>  };\n>  \n> diff --git a/src/ipa/rpi/cam_helper/cam_helper.cpp b/src/ipa/rpi/cam_helper/cam_helper.cpp\n> index 6493e882..8c720652 100644\n> --- a/src/ipa/rpi/cam_helper/cam_helper.cpp\n> +++ b/src/ipa/rpi/cam_helper/cam_helper.cpp\n> @@ -156,19 +156,6 @@ void CamHelper::setCameraMode(const CameraMode &mode)\n>         }\n>  }\n>  \n> -void CamHelper::getDelays(int &exposureDelay, int &gainDelay,\n> -                         int &vblankDelay, int &hblankDelay) const\n> -{\n> -       /*\n> -        * These values are correct for many sensors. Other sensors will\n> -        * need to over-ride this function.\n> -        */\n> -       exposureDelay = 2;\n> -       gainDelay = 1;\n> -       vblankDelay = 2;\n> -       hblankDelay = 2;\n> -}\n\nThis matches \"CameraSensorProperties::SensorDelays defaultSensorDelays\"\nin CameraSensorLegacy, so Ack here.\n\n<snip because large diff>\n\n> -void CamHelperOv5647::getDelays(int &exposureDelay, int &gainDelay,\n> -                               int &vblankDelay, int &hblankDelay) const\n> -{\n> -       /*\n> -        * We run this sensor in a mode where the gain delay is bumped up to\n> -        * 2. It seems to be the only way to make the delays \"predictable\".\n> -        */\n\nWe lose this comment as it's not in CameraSensorProperties, but the\nvalues match and I think it's ok.\n\n> -       exposureDelay = 2;\n> -       gainDelay = 2;\n> -       vblankDelay = 2;\n> -       hblankDelay = 2;\n> -}\n> -\n\n<snip>\n\n> diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp\n> index 5fce17e6..45e2a1d7 100644\n> --- a/src/ipa/rpi/common/ipa_base.cpp\n> +++ b/src/ipa/rpi/common/ipa_base.cpp\n> @@ -134,18 +134,8 @@ int32_t IpaBase::init(const IPASettings &settings, const InitParams &params, Ini\n>                 return -EINVAL;\n>         }\n>  \n> -       /*\n> -        * Pass out the sensor config to the pipeline handler in order\n> -        * to setup the staggered writer class.\n> -        */\n> -       int gainDelay, exposureDelay, vblankDelay, hblankDelay, sensorMetadata;\n> -       helper_->getDelays(exposureDelay, gainDelay, vblankDelay, hblankDelay);\n> -       sensorMetadata = helper_->sensorEmbeddedDataPresent();\n> -\n> -       result->sensorConfig.gainDelay = gainDelay;\n> -       result->sensorConfig.exposureDelay = exposureDelay;\n> -       result->sensorConfig.vblankDelay = vblankDelay;\n> -       result->sensorConfig.hblankDelay = hblankDelay;\n> +       /* Pass out the sensor metadata to the pipeline handler */\n> +       int sensorMetadata = helper_->sensorEmbeddedDataPresent();\n>         result->sensorConfig.sensorMetadata = sensorMetadata;\n>  \n>         /* Load the tuning file for this sensor. */\n> diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> index 9e2d9d23..398a0280 100644\n> --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> @@ -816,11 +816,12 @@ int PipelineHandlerBase::registerCamera(std::unique_ptr<RPi::CameraData> &camera\n>          * Setup our delayed control writer with the sensor default\n>          * gain and exposure delays. Mark VBLANK for priority write.\n>          */\n> +       const CameraSensorProperties::SensorDelays &delays = data->sensor_->sensorDelays();\n>         std::unordered_map<uint32_t, RPi::DelayedControls::ControlParams> params = {\n> -               { V4L2_CID_ANALOGUE_GAIN, { result.sensorConfig.gainDelay, false } },\n> -               { V4L2_CID_EXPOSURE, { result.sensorConfig.exposureDelay, false } },\n> -               { V4L2_CID_HBLANK, { result.sensorConfig.hblankDelay, false } },\n> -               { V4L2_CID_VBLANK, { result.sensorConfig.vblankDelay, true } }\n> +               { V4L2_CID_ANALOGUE_GAIN, { delays.gainDelay, false } },\n> +               { V4L2_CID_EXPOSURE, { delays.exposureDelay, false } },\n> +               { V4L2_CID_HBLANK, { delays.hblankDelay, false } },\n> +               { V4L2_CID_VBLANK, { delays.vblankDelay, true } }\n\nAnd that's all simplified and now unified so we have a single table of\ntruth for sensor delay values.\n\nso:\n\n\nReviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n\n\n>         };\n>         data->delayedCtrls_ = std::make_unique<RPi::DelayedControls>(data->sensor_->device(), params);\n>         data->sensorMetadata_ = result.sensorConfig.sensorMetadata;\n> -- \n> 2.34.1\n>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id E0D33BD1F1\n\tfor <parsemail@patchwork.libcamera.org>;\n\tTue, 17 Dec 2024 16:46:57 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9EFB167FF2;\n\tTue, 17 Dec 2024 17:46:57 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E586367FEF\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue, 17 Dec 2024 17:46:54 +0100 (CET)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 7BB684C7;\n\tTue, 17 Dec 2024 17:46:17 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"vm2Ho326\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1734453977;\n\tbh=C4y8t9kDLk9BLNGmV8qcDpvKnQ1IT5fvi2Gq5qDHlbU=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=vm2Ho326LIMF508mHO9uG4Bz/DvKhIWcEhgj3G53iOVRD89rVW9rwaq3LFzmWvzb/\n\tcQaTPF9bEUb8lzX+6whpxuJk5ACWGHQUhnFLhRYPuZsMxgwhbOFwnigDNCQGYdVqzc\n\t35yX568UTGGsIim0J26x7UZU6BQJcBXeC95+Rh0Q=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20241127133233.247977-3-dan.scally@ideasonboard.com>","References":"<20241127133233.247977-1-dan.scally@ideasonboard.com>\n\t<20241127133233.247977-3-dan.scally@ideasonboard.com>","Subject":"Re: [PATCH 2/2] libcamera: rpi: Draw sensor delays from\n\tCameraSensorProperties","From":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Cc":"david.plowman@raspberrypi.com, naush@raspberrypi.com,\n\tDaniel Scally <dan.scally@ideasonboard.com>","To":"Daniel Scally <dan.scally@ideasonboard.com>,\n\tlibcamera-devel@lists.libcamera.org","Date":"Tue, 17 Dec 2024 16:46:52 +0000","Message-ID":"<173445401236.32744.1757422668793523225@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":32838,"web_url":"https://patchwork.libcamera.org/comment/32838/","msgid":"<20241217165612.GJ20432@pendragon.ideasonboard.com>","date":"2024-12-17T16:56:12","subject":"Re: [PATCH 2/2] libcamera: rpi: Draw sensor delays from\n\tCameraSensorProperties","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Tue, Dec 17, 2024 at 04:46:52PM +0000, Kieran Bingham wrote:\n> Quoting Daniel Scally (2024-11-27 13:32:33)\n> > Now that we have camera sensor control application delay values in\n> > the CameraSensorProperties class, remove the duplicated definitions\n> > in the RPi IPA's CameraSensorHelpers and update the pipeline handler\n> > to use the values from CameraSensorProperties.\n> > \n> > Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>\n> > ---\n> >  include/libcamera/ipa/raspberrypi.mojom           |  4 ----\n> >  src/ipa/rpi/cam_helper/cam_helper.cpp             | 13 -------------\n> >  src/ipa/rpi/cam_helper/cam_helper.h               |  7 -------\n> >  src/ipa/rpi/cam_helper/cam_helper_imx283.cpp      | 12 ------------\n> >  src/ipa/rpi/cam_helper/cam_helper_imx290.cpp      | 11 -----------\n> >  src/ipa/rpi/cam_helper/cam_helper_imx296.cpp      | 11 -----------\n> >  src/ipa/rpi/cam_helper/cam_helper_imx477.cpp      | 11 -----------\n> >  src/ipa/rpi/cam_helper/cam_helper_imx519.cpp      | 11 -----------\n> >  src/ipa/rpi/cam_helper/cam_helper_imx708.cpp      | 11 -----------\n> >  src/ipa/rpi/cam_helper/cam_helper_ov5647.cpp      | 15 ---------------\n> >  src/ipa/rpi/cam_helper/cam_helper_ov64a40.cpp     | 12 ------------\n> >  src/ipa/rpi/cam_helper/cam_helper_ov7251.cpp      | 12 ------------\n> >  src/ipa/rpi/cam_helper/cam_helper_ov9281.cpp      | 12 ------------\n> >  src/ipa/rpi/common/ipa_base.cpp                   | 14 ++------------\n> >  .../pipeline/rpi/common/pipeline_base.cpp         |  9 +++++----\n> >  15 files changed, 7 insertions(+), 158 deletions(-)\n> > \n> > diff --git a/include/libcamera/ipa/raspberrypi.mojom b/include/libcamera/ipa/raspberrypi.mojom\n> > index 0b92587d..e30c70bd 100644\n> > --- a/include/libcamera/ipa/raspberrypi.mojom\n> > +++ b/include/libcamera/ipa/raspberrypi.mojom\n> > @@ -12,10 +12,6 @@ import \"include/libcamera/ipa/core.mojom\";\n> >  const uint32 MaxLsGridSize = 0x8000;\n> >  \n> >  struct SensorConfig {\n> > -       uint32 gainDelay;\n> > -       uint32 exposureDelay;\n> > -       uint32 vblankDelay;\n> > -       uint32 hblankDelay;\n> >         uint32 sensorMetadata;\n> >  };\n> >  \n> > diff --git a/src/ipa/rpi/cam_helper/cam_helper.cpp b/src/ipa/rpi/cam_helper/cam_helper.cpp\n> > index 6493e882..8c720652 100644\n> > --- a/src/ipa/rpi/cam_helper/cam_helper.cpp\n> > +++ b/src/ipa/rpi/cam_helper/cam_helper.cpp\n> > @@ -156,19 +156,6 @@ void CamHelper::setCameraMode(const CameraMode &mode)\n> >         }\n> >  }\n> >  \n> > -void CamHelper::getDelays(int &exposureDelay, int &gainDelay,\n> > -                         int &vblankDelay, int &hblankDelay) const\n> > -{\n> > -       /*\n> > -        * These values are correct for many sensors. Other sensors will\n> > -        * need to over-ride this function.\n> > -        */\n> > -       exposureDelay = 2;\n> > -       gainDelay = 1;\n> > -       vblankDelay = 2;\n> > -       hblankDelay = 2;\n> > -}\n> \n> This matches \"CameraSensorProperties::SensorDelays defaultSensorDelays\"\n> in CameraSensorLegacy, so Ack here.\n> \n> <snip because large diff>\n> \n> > -void CamHelperOv5647::getDelays(int &exposureDelay, int &gainDelay,\n> > -                               int &vblankDelay, int &hblankDelay) const\n> > -{\n> > -       /*\n> > -        * We run this sensor in a mode where the gain delay is bumped up to\n> > -        * 2. It seems to be the only way to make the delays \"predictable\".\n> > -        */\n> \n> We lose this comment as it's not in CameraSensorProperties, but the\n> values match and I think it's ok.\n\nIt could be worth keeping the comment. I think AR0521 suffers from a\nsimilar issue (but the driver doesn't currently run it in a mode where\nthe gain delay is predictable).\n\n> > -       exposureDelay = 2;\n> > -       gainDelay = 2;\n> > -       vblankDelay = 2;\n> > -       hblankDelay = 2;\n> > -}\n> > -\n> \n> <snip>\n> \n> > diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp\n> > index 5fce17e6..45e2a1d7 100644\n> > --- a/src/ipa/rpi/common/ipa_base.cpp\n> > +++ b/src/ipa/rpi/common/ipa_base.cpp\n> > @@ -134,18 +134,8 @@ int32_t IpaBase::init(const IPASettings &settings, const InitParams &params, Ini\n> >                 return -EINVAL;\n> >         }\n> >  \n> > -       /*\n> > -        * Pass out the sensor config to the pipeline handler in order\n> > -        * to setup the staggered writer class.\n> > -        */\n> > -       int gainDelay, exposureDelay, vblankDelay, hblankDelay, sensorMetadata;\n> > -       helper_->getDelays(exposureDelay, gainDelay, vblankDelay, hblankDelay);\n> > -       sensorMetadata = helper_->sensorEmbeddedDataPresent();\n> > -\n> > -       result->sensorConfig.gainDelay = gainDelay;\n> > -       result->sensorConfig.exposureDelay = exposureDelay;\n> > -       result->sensorConfig.vblankDelay = vblankDelay;\n> > -       result->sensorConfig.hblankDelay = hblankDelay;\n> > +       /* Pass out the sensor metadata to the pipeline handler */\n> > +       int sensorMetadata = helper_->sensorEmbeddedDataPresent();\n> >         result->sensorConfig.sensorMetadata = sensorMetadata;\n> >  \n> >         /* Load the tuning file for this sensor. */\n> > diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> > index 9e2d9d23..398a0280 100644\n> > --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> > +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> > @@ -816,11 +816,12 @@ int PipelineHandlerBase::registerCamera(std::unique_ptr<RPi::CameraData> &camera\n> >          * Setup our delayed control writer with the sensor default\n> >          * gain and exposure delays. Mark VBLANK for priority write.\n> >          */\n> > +       const CameraSensorProperties::SensorDelays &delays = data->sensor_->sensorDelays();\n> >         std::unordered_map<uint32_t, RPi::DelayedControls::ControlParams> params = {\n> > -               { V4L2_CID_ANALOGUE_GAIN, { result.sensorConfig.gainDelay, false } },\n> > -               { V4L2_CID_EXPOSURE, { result.sensorConfig.exposureDelay, false } },\n> > -               { V4L2_CID_HBLANK, { result.sensorConfig.hblankDelay, false } },\n> > -               { V4L2_CID_VBLANK, { result.sensorConfig.vblankDelay, true } }\n> > +               { V4L2_CID_ANALOGUE_GAIN, { delays.gainDelay, false } },\n> > +               { V4L2_CID_EXPOSURE, { delays.exposureDelay, false } },\n> > +               { V4L2_CID_HBLANK, { delays.hblankDelay, false } },\n> > +               { V4L2_CID_VBLANK, { delays.vblankDelay, true } }\n> \n> And that's all simplified and now unified so we have a single table of\n> truth for sensor delay values.\n> \n> so:\n> \n> \n> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n> \n> \n> >         };\n> >         data->delayedCtrls_ = std::make_unique<RPi::DelayedControls>(data->sensor_->device(), params);\n> >         data->sensorMetadata_ = result.sensorConfig.sensorMetadata;","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id B0FC1C32F6\n\tfor <parsemail@patchwork.libcamera.org>;\n\tTue, 17 Dec 2024 16:56:16 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 63EB167FF1;\n\tTue, 17 Dec 2024 17:56:16 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id BB13267FE2\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue, 17 Dec 2024 17:56:14 +0100 (CET)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 4E9E04C7;\n\tTue, 17 Dec 2024 17:55:37 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"pN2uNwzN\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1734454537;\n\tbh=q2NDj8NJCCgwFrPfKmuhyKYd3lGpq3RvZYSz9DXl1AA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=pN2uNwzNxMVovDXDrIzjzryI8Om1nxIa+Np5c6I79xp3N1535bk1Mm0RbEI/F3wY/\n\tlxOd3Mc6Xq2eOI1V/txRiMMmkc/dWKfxISNhFbaSowpMucmyNWezQJc+aivljk6/Rf\n\tQyzMrjpZi1TMrb/CYO+CdwtOCh0tYbwDMYIJbieQ=","Date":"Tue, 17 Dec 2024 18:56:12 +0200","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Cc":"Daniel Scally <dan.scally@ideasonboard.com>,\n\tlibcamera-devel@lists.libcamera.org, david.plowman@raspberrypi.com,\n\tnaush@raspberrypi.com","Subject":"Re: [PATCH 2/2] libcamera: rpi: Draw sensor delays from\n\tCameraSensorProperties","Message-ID":"<20241217165612.GJ20432@pendragon.ideasonboard.com>","References":"<20241127133233.247977-1-dan.scally@ideasonboard.com>\n\t<20241127133233.247977-3-dan.scally@ideasonboard.com>\n\t<173445401236.32744.1757422668793523225@ping.linuxembedded.co.uk>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<173445401236.32744.1757422668793523225@ping.linuxembedded.co.uk>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":32855,"web_url":"https://patchwork.libcamera.org/comment/32855/","msgid":"<7f14da10-42b3-4ada-ac73-c14b2f09a19d@ideasonboard.com>","date":"2024-12-17T22:46:01","subject":"Re: [PATCH 2/2] libcamera: rpi: Draw sensor delays from\n\tCameraSensorProperties","submitter":{"id":156,"url":"https://patchwork.libcamera.org/api/people/156/","name":"Dan Scally","email":"dan.scally@ideasonboard.com"},"content":"Hi Naush, David\n\nOn 17/12/2024 16:56, Laurent Pinchart wrote:\n> On Tue, Dec 17, 2024 at 04:46:52PM +0000, Kieran Bingham wrote:\n>> Quoting Daniel Scally (2024-11-27 13:32:33)\n>>> Now that we have camera sensor control application delay values in\n>>> the CameraSensorProperties class, remove the duplicated definitions\n>>> in the RPi IPA's CameraSensorHelpers and update the pipeline handler\n>>> to use the values from CameraSensorProperties.\n>>>\n>>> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>\n>>> ---\n>>>   include/libcamera/ipa/raspberrypi.mojom           |  4 ----\n>>>   src/ipa/rpi/cam_helper/cam_helper.cpp             | 13 -------------\n>>>   src/ipa/rpi/cam_helper/cam_helper.h               |  7 -------\n>>>   src/ipa/rpi/cam_helper/cam_helper_imx283.cpp      | 12 ------------\n>>>   src/ipa/rpi/cam_helper/cam_helper_imx290.cpp      | 11 -----------\n>>>   src/ipa/rpi/cam_helper/cam_helper_imx296.cpp      | 11 -----------\n>>>   src/ipa/rpi/cam_helper/cam_helper_imx477.cpp      | 11 -----------\n>>>   src/ipa/rpi/cam_helper/cam_helper_imx519.cpp      | 11 -----------\n>>>   src/ipa/rpi/cam_helper/cam_helper_imx708.cpp      | 11 -----------\n>>>   src/ipa/rpi/cam_helper/cam_helper_ov5647.cpp      | 15 ---------------\n>>>   src/ipa/rpi/cam_helper/cam_helper_ov64a40.cpp     | 12 ------------\n>>>   src/ipa/rpi/cam_helper/cam_helper_ov7251.cpp      | 12 ------------\n>>>   src/ipa/rpi/cam_helper/cam_helper_ov9281.cpp      | 12 ------------\n>>>   src/ipa/rpi/common/ipa_base.cpp                   | 14 ++------------\n>>>   .../pipeline/rpi/common/pipeline_base.cpp         |  9 +++++----\n>>>   15 files changed, 7 insertions(+), 158 deletions(-)\n>>>\n>>> diff --git a/include/libcamera/ipa/raspberrypi.mojom b/include/libcamera/ipa/raspberrypi.mojom\n>>> index 0b92587d..e30c70bd 100644\n>>> --- a/include/libcamera/ipa/raspberrypi.mojom\n>>> +++ b/include/libcamera/ipa/raspberrypi.mojom\n>>> @@ -12,10 +12,6 @@ import \"include/libcamera/ipa/core.mojom\";\n>>>   const uint32 MaxLsGridSize = 0x8000;\n>>>   \n>>>   struct SensorConfig {\n>>> -       uint32 gainDelay;\n>>> -       uint32 exposureDelay;\n>>> -       uint32 vblankDelay;\n>>> -       uint32 hblankDelay;\n>>>          uint32 sensorMetadata;\n>>>   };\n>>>   \n>>> diff --git a/src/ipa/rpi/cam_helper/cam_helper.cpp b/src/ipa/rpi/cam_helper/cam_helper.cpp\n>>> index 6493e882..8c720652 100644\n>>> --- a/src/ipa/rpi/cam_helper/cam_helper.cpp\n>>> +++ b/src/ipa/rpi/cam_helper/cam_helper.cpp\n>>> @@ -156,19 +156,6 @@ void CamHelper::setCameraMode(const CameraMode &mode)\n>>>          }\n>>>   }\n>>>   \n>>> -void CamHelper::getDelays(int &exposureDelay, int &gainDelay,\n>>> -                         int &vblankDelay, int &hblankDelay) const\n>>> -{\n>>> -       /*\n>>> -        * These values are correct for many sensors. Other sensors will\n>>> -        * need to over-ride this function.\n>>> -        */\n>>> -       exposureDelay = 2;\n>>> -       gainDelay = 1;\n>>> -       vblankDelay = 2;\n>>> -       hblankDelay = 2;\n>>> -}\n>> This matches \"CameraSensorProperties::SensorDelays defaultSensorDelays\"\n>> in CameraSensorLegacy, so Ack here.\n>>\n>> <snip because large diff>\n>>\n>>> -void CamHelperOv5647::getDelays(int &exposureDelay, int &gainDelay,\n>>> -                               int &vblankDelay, int &hblankDelay) const\n>>> -{\n>>> -       /*\n>>> -        * We run this sensor in a mode where the gain delay is bumped up to\n>>> -        * 2. It seems to be the only way to make the delays \"predictable\".\n>>> -        */\n>> We lose this comment as it's not in CameraSensorProperties, but the\n>> values match and I think it's ok.\n> It could be worth keeping the comment. I think AR0521 suffers from a\n> similar issue (but the driver doesn't currently run it in a mode where\n> the gain delay is predictable).\n\n\nLaurent's eagle-eyes spotted that actually the OV5647 driver doesn't seem to configure a 2 frame \ndelay. The datasheet says bits [5:4] of register 0x3503 control the delay, with 11 being the setting \nfor 2 frames but in the driver those are set to 00 (which should be no delay). Do you happen to know \nwhere this 2-frame delay value comes from?\n\n>\n>>> -       exposureDelay = 2;\n>>> -       gainDelay = 2;\n>>> -       vblankDelay = 2;\n>>> -       hblankDelay = 2;\n>>> -}\n>>> -\n>> <snip>\n>>\n>>> diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp\n>>> index 5fce17e6..45e2a1d7 100644\n>>> --- a/src/ipa/rpi/common/ipa_base.cpp\n>>> +++ b/src/ipa/rpi/common/ipa_base.cpp\n>>> @@ -134,18 +134,8 @@ int32_t IpaBase::init(const IPASettings &settings, const InitParams &params, Ini\n>>>                  return -EINVAL;\n>>>          }\n>>>   \n>>> -       /*\n>>> -        * Pass out the sensor config to the pipeline handler in order\n>>> -        * to setup the staggered writer class.\n>>> -        */\n>>> -       int gainDelay, exposureDelay, vblankDelay, hblankDelay, sensorMetadata;\n>>> -       helper_->getDelays(exposureDelay, gainDelay, vblankDelay, hblankDelay);\n>>> -       sensorMetadata = helper_->sensorEmbeddedDataPresent();\n>>> -\n>>> -       result->sensorConfig.gainDelay = gainDelay;\n>>> -       result->sensorConfig.exposureDelay = exposureDelay;\n>>> -       result->sensorConfig.vblankDelay = vblankDelay;\n>>> -       result->sensorConfig.hblankDelay = hblankDelay;\n>>> +       /* Pass out the sensor metadata to the pipeline handler */\n>>> +       int sensorMetadata = helper_->sensorEmbeddedDataPresent();\n>>>          result->sensorConfig.sensorMetadata = sensorMetadata;\n>>>   \n>>>          /* Load the tuning file for this sensor. */\n>>> diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n>>> index 9e2d9d23..398a0280 100644\n>>> --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n>>> +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n>>> @@ -816,11 +816,12 @@ int PipelineHandlerBase::registerCamera(std::unique_ptr<RPi::CameraData> &camera\n>>>           * Setup our delayed control writer with the sensor default\n>>>           * gain and exposure delays. Mark VBLANK for priority write.\n>>>           */\n>>> +       const CameraSensorProperties::SensorDelays &delays = data->sensor_->sensorDelays();\n>>>          std::unordered_map<uint32_t, RPi::DelayedControls::ControlParams> params = {\n>>> -               { V4L2_CID_ANALOGUE_GAIN, { result.sensorConfig.gainDelay, false } },\n>>> -               { V4L2_CID_EXPOSURE, { result.sensorConfig.exposureDelay, false } },\n>>> -               { V4L2_CID_HBLANK, { result.sensorConfig.hblankDelay, false } },\n>>> -               { V4L2_CID_VBLANK, { result.sensorConfig.vblankDelay, true } }\n>>> +               { V4L2_CID_ANALOGUE_GAIN, { delays.gainDelay, false } },\n>>> +               { V4L2_CID_EXPOSURE, { delays.exposureDelay, false } },\n>>> +               { V4L2_CID_HBLANK, { delays.hblankDelay, false } },\n>>> +               { V4L2_CID_VBLANK, { delays.vblankDelay, true } }\n>> And that's all simplified and now unified so we have a single table of\n>> truth for sensor delay values.\n>>\n>> so:\n>>\n>>\n>> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n>>\n>>\n>>>          };\n>>>          data->delayedCtrls_ = std::make_unique<RPi::DelayedControls>(data->sensor_->device(), params);\n>>>          data->sensorMetadata_ = result.sensorConfig.sensorMetadata;","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id E5B8FC3301\n\tfor <parsemail@patchwork.libcamera.org>;\n\tTue, 17 Dec 2024 22:46:07 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 39DDF67FD3;\n\tTue, 17 Dec 2024 23:46:07 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E397567FD3\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue, 17 Dec 2024 23:46:04 +0100 (CET)","from [192.168.0.43]\n\t(cpc141996-chfd3-2-0-cust928.12-3.cable.virginm.net [86.13.91.161])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 190054C7;\n\tTue, 17 Dec 2024 23:45:27 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"Bx2/kjxM\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1734475527;\n\tbh=B2FVZNH4sKippz7u79DR/vn6OeSd7GQF+T1zPa3s4EA=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=Bx2/kjxMf/9NpOjEiLmkMgdl4ccSQzEV+dIuhp2d30SMwUQgFC141mvzwjlUtb2gM\n\tkul/5fpMH6f5OsWOOq6DZouE3QmArw07em8x3lMW9fwTIu5ooU179rta7miBBL2cly\n\t/vzMrwR3J1aGGK0Zc+yJ8smct/r0jPIHseI0gpGI=","Message-ID":"<7f14da10-42b3-4ada-ac73-c14b2f09a19d@ideasonboard.com>","Date":"Tue, 17 Dec 2024 22:46:01 +0000","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH 2/2] libcamera: rpi: Draw sensor delays from\n\tCameraSensorProperties","To":"david.plowman@raspberrypi.com, Naushir Patuck <naush@raspberrypi.com>","Cc":"libcamera-devel@lists.libcamera.org,\n\tKieran Bingham <kieran.bingham@ideasonboard.com>,\n\tLaurent Pinchart <laurent.pinchart@ideasonboard.com>","References":"<20241127133233.247977-1-dan.scally@ideasonboard.com>\n\t<20241127133233.247977-3-dan.scally@ideasonboard.com>\n\t<173445401236.32744.1757422668793523225@ping.linuxembedded.co.uk>\n\t<20241217165612.GJ20432@pendragon.ideasonboard.com>","Content-Language":"en-US","From":"Dan Scally <dan.scally@ideasonboard.com>","Autocrypt":"addr=dan.scally@ideasonboard.com; keydata=\n\txsFNBGLydlEBEADa5O2s0AbUguprfvXOQun/0a8y2Vk6BqkQALgeD6KnXSWwaoCULp18etYW\n\tB31bfgrdphXQ5kUQibB0ADK8DERB4wrzrUb5CMxLBFE7mQty+v5NsP0OFNK9XTaAOcmD+Ove\n\teIjYvqurAaro91jrRVrS1gBRxIFqyPgNvwwL+alMZhn3/2jU2uvBmuRrgnc/e9cHKiuT3Dtq\n\tMHGPKL2m+plk+7tjMoQFfexoQ1JKugHAjxAhJfrkXh6uS6rc01bYCyo7ybzg53m1HLFJdNGX\n\tsUKR+dQpBs3SY4s66tc1sREJqdYyTsSZf80HjIeJjU/hRunRo4NjRIJwhvnK1GyjOvvuCKVU\n\tRWpY8dNjNu5OeAfdrlvFJOxIE9M8JuYCQTMULqd1NuzbpFMjc9524U3Cngs589T7qUMPb1H1\n\tNTA81LmtJ6Y+IV5/kiTUANflpzBwhu18Ok7kGyCq2a2jsOcVmk8gZNs04gyjuj8JziYwwLbf\n\tvzABwpFVcS8aR+nHIZV1HtOzyw8CsL8OySc3K9y+Y0NRpziMRvutrppzgyMb9V+N31mK9Mxl\n\t1YkgaTl4ciNWpdfUe0yxH03OCuHi3922qhPLF4XX5LN+NaVw5Xz2o3eeWklXdouxwV7QlN33\n\tu4+u2FWzKxDqO6WLQGjxPE0mVB4Gh5Pa1Vb0ct9Ctg0qElvtGQARAQABzShEYW4gU2NhbGx5\n\tIDxkYW4uc2NhbGx5QGlkZWFzb25ib2FyZC5jb20+wsGNBBMBCAA3FiEEsdtt8OWP7+8SNfQe\n\tkiQuh/L+GMQFAmLydlIFCQWjmoACGwMECwkIBwUVCAkKCwUWAgMBAAAKCRCSJC6H8v4YxDI2\n\tEAC2Gz0iyaXJkPInyshrREEWbo0CA6v5KKf3I/HlMPqkZ48bmGoYm4mEQGFWZJAT3K4ir8bg\n\tcEfs9V54gpbrZvdwS4abXbUK4WjKwEs8HK3XJv1WXUN2bsz5oEJWZUImh9gD3naiLLI9QMMm\n\tw/aZkT+NbN5/2KvChRWhdcha7+2Te4foOY66nIM+pw2FZM6zIkInLLUik2zXOhaZtqdeJZQi\n\tHSPU9xu7TRYN4cvdZAnSpG7gQqmLm5/uGZN1/sB3kHTustQtSXKMaIcD/DMNI3JN/t+RJVS7\n\tc0Jh/ThzTmhHyhxx3DRnDIy7kwMI4CFvmhkVC2uNs9kWsj1DuX5kt8513mvfw2OcX9UnNKmZ\n\tnhNCuF6DxVrL8wjOPuIpiEj3V+K7DFF1Cxw1/yrLs8dYdYh8T8vCY2CHBMsqpESROnTazboh\n\tAiQ2xMN1cyXtX11Qwqm5U3sykpLbx2BcmUUUEAKNsM//Zn81QXKG8vOx0ZdMfnzsCaCzt8f6\n\t9dcDBBI3tJ0BI9ByiocqUoL6759LM8qm18x3FYlxvuOs4wSGPfRVaA4yh0pgI+ModVC2Pu3y\n\tejE/IxeatGqJHh6Y+iJzskdi27uFkRixl7YJZvPJAbEn7kzSi98u/5ReEA8Qhc8KO/B7wprj\n\txjNMZNYd0Eth8+WkixHYj752NT5qshKJXcyUU87BTQRi8nZSARAAx0BJayh1Fhwbf4zoY56x\n\txHEpT6DwdTAYAetd3yiKClLVJadYxOpuqyWa1bdfQWPb+h4MeXbWw/53PBgn7gI2EA7ebIRC\n\tPJJhAIkeym7hHZoxqDQTGDJjxFEL11qF+U3rhWiL2Zt0Pl+zFq0eWYYVNiXjsIS4FI2+4m16\n\ttPbDWZFJnSZ828VGtRDQdhXfx3zyVX21lVx1bX4/OZvIET7sVUufkE4hrbqrrufre7wsjD1t\n\t8MQKSapVrr1RltpzPpScdoxknOSBRwOvpp57pJJe5A0L7+WxJ+vQoQXj0j+5tmIWOAV1qBQp\n\thyoyUk9JpPfntk2EKnZHWaApFp5TcL6c5LhUvV7F6XwOjGPuGlZQCWXee9dr7zym8iR3irWT\n\t+49bIh5PMlqSLXJDYbuyFQHFxoiNdVvvf7etvGfqFYVMPVjipqfEQ38ST2nkzx+KBICz7uwj\n\tJwLBdTXzGFKHQNckGMl7F5QdO/35An/QcxBnHVMXqaSd12tkJmoRVWduwuuoFfkTY5mUV3uX\n\txGj3iVCK4V+ezOYA7c2YolfRCNMTza6vcK/P4tDjjsyBBZrCCzhBvd4VVsnnlZhVaIxoky4K\n\taL+AP+zcQrUZmXmgZjXOLryGnsaeoVrIFyrU6ly90s1y3KLoPsDaTBMtnOdwxPmo1xisH8oL\n\ta/VRgpFBfojLPxMAEQEAAcLBfAQYAQgAJhYhBLHbbfDlj+/vEjX0HpIkLofy/hjEBQJi8nZT\n\tBQkFo5qAAhsMAAoJEJIkLofy/hjEXPcQAMIPNqiWiz/HKu9W4QIf1OMUpKn3YkVIj3p3gvfM\n\tRes4fGX94Ji599uLNrPoxKyaytC4R6BTxVriTJjWK8mbo9jZIRM4vkwkZZ2bu98EweSucxbp\n\tvjESsvMXGgxniqV/RQ/3T7LABYRoIUutARYq58p5HwSP0frF0fdFHYdTa2g7MYZl1ur2JzOC\n\tFHRpGadlNzKDE3fEdoMobxHB3Lm6FDml5GyBAA8+dQYVI0oDwJ3gpZPZ0J5Vx9RbqXe8RDuR\n\tdu90hvCJkq7/tzSQ0GeD3BwXb9/R/A4dVXhaDd91Q1qQXidI+2jwhx8iqiYxbT+DoAUkQRQy\n\txBtoCM1CxH7u45URUgD//fxYr3D4B1SlonA6vdaEdHZOGwECnDpTxecENMbz/Bx7qfrmd901\n\tD+N9SjIwrbVhhSyUXYnSUb8F+9g2RDY42Sk7GcYxIeON4VzKqWM7hpkXZ47pkK0YodO+dRKM\n\tyMcoUWrTK0Uz6UzUGKoJVbxmSW/EJLEGoI5p3NWxWtScEVv8mO49gqQdrRIOheZycDmHnItt\n\t9Qjv00uFhEwv2YfiyGk6iGF2W40s2pH2t6oeuGgmiZ7g6d0MEK8Ql/4zPItvr1c1rpwpXUC1\n\tu1kQWgtnNjFHX3KiYdqjcZeRBiry1X0zY+4Y24wUU0KsEewJwjhmCKAsju1RpdlPg2kC","In-Reply-To":"<20241217165612.GJ20432@pendragon.ideasonboard.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":32866,"web_url":"https://patchwork.libcamera.org/comment/32866/","msgid":"<CAEmqJPrO-KM2tqgqDiN1frpkD36mdr3np14jXv45+G1LVfNkYQ@mail.gmail.com>","date":"2024-12-18T08:33:41","subject":"Re: [PATCH 2/2] libcamera: rpi: Draw sensor delays from\n\tCameraSensorProperties","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Dan,\n\nOn Tue, 17 Dec 2024 at 22:46, Dan Scally <dan.scally@ideasonboard.com> wrote:\n>\n> Hi Naush, David\n>\n> On 17/12/2024 16:56, Laurent Pinchart wrote:\n> > On Tue, Dec 17, 2024 at 04:46:52PM +0000, Kieran Bingham wrote:\n> >> Quoting Daniel Scally (2024-11-27 13:32:33)\n> >>> Now that we have camera sensor control application delay values in\n> >>> the CameraSensorProperties class, remove the duplicated definitions\n> >>> in the RPi IPA's CameraSensorHelpers and update the pipeline handler\n> >>> to use the values from CameraSensorProperties.\n> >>>\n> >>> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>\n> >>> ---\n> >>>   include/libcamera/ipa/raspberrypi.mojom           |  4 ----\n> >>>   src/ipa/rpi/cam_helper/cam_helper.cpp             | 13 -------------\n> >>>   src/ipa/rpi/cam_helper/cam_helper.h               |  7 -------\n> >>>   src/ipa/rpi/cam_helper/cam_helper_imx283.cpp      | 12 ------------\n> >>>   src/ipa/rpi/cam_helper/cam_helper_imx290.cpp      | 11 -----------\n> >>>   src/ipa/rpi/cam_helper/cam_helper_imx296.cpp      | 11 -----------\n> >>>   src/ipa/rpi/cam_helper/cam_helper_imx477.cpp      | 11 -----------\n> >>>   src/ipa/rpi/cam_helper/cam_helper_imx519.cpp      | 11 -----------\n> >>>   src/ipa/rpi/cam_helper/cam_helper_imx708.cpp      | 11 -----------\n> >>>   src/ipa/rpi/cam_helper/cam_helper_ov5647.cpp      | 15 ---------------\n> >>>   src/ipa/rpi/cam_helper/cam_helper_ov64a40.cpp     | 12 ------------\n> >>>   src/ipa/rpi/cam_helper/cam_helper_ov7251.cpp      | 12 ------------\n> >>>   src/ipa/rpi/cam_helper/cam_helper_ov9281.cpp      | 12 ------------\n> >>>   src/ipa/rpi/common/ipa_base.cpp                   | 14 ++------------\n> >>>   .../pipeline/rpi/common/pipeline_base.cpp         |  9 +++++----\n> >>>   15 files changed, 7 insertions(+), 158 deletions(-)\n> >>>\n> >>> diff --git a/include/libcamera/ipa/raspberrypi.mojom b/include/libcamera/ipa/raspberrypi.mojom\n> >>> index 0b92587d..e30c70bd 100644\n> >>> --- a/include/libcamera/ipa/raspberrypi.mojom\n> >>> +++ b/include/libcamera/ipa/raspberrypi.mojom\n> >>> @@ -12,10 +12,6 @@ import \"include/libcamera/ipa/core.mojom\";\n> >>>   const uint32 MaxLsGridSize = 0x8000;\n> >>>\n> >>>   struct SensorConfig {\n> >>> -       uint32 gainDelay;\n> >>> -       uint32 exposureDelay;\n> >>> -       uint32 vblankDelay;\n> >>> -       uint32 hblankDelay;\n> >>>          uint32 sensorMetadata;\n> >>>   };\n> >>>\n> >>> diff --git a/src/ipa/rpi/cam_helper/cam_helper.cpp b/src/ipa/rpi/cam_helper/cam_helper.cpp\n> >>> index 6493e882..8c720652 100644\n> >>> --- a/src/ipa/rpi/cam_helper/cam_helper.cpp\n> >>> +++ b/src/ipa/rpi/cam_helper/cam_helper.cpp\n> >>> @@ -156,19 +156,6 @@ void CamHelper::setCameraMode(const CameraMode &mode)\n> >>>          }\n> >>>   }\n> >>>\n> >>> -void CamHelper::getDelays(int &exposureDelay, int &gainDelay,\n> >>> -                         int &vblankDelay, int &hblankDelay) const\n> >>> -{\n> >>> -       /*\n> >>> -        * These values are correct for many sensors. Other sensors will\n> >>> -        * need to over-ride this function.\n> >>> -        */\n> >>> -       exposureDelay = 2;\n> >>> -       gainDelay = 1;\n> >>> -       vblankDelay = 2;\n> >>> -       hblankDelay = 2;\n> >>> -}\n> >> This matches \"CameraSensorProperties::SensorDelays defaultSensorDelays\"\n> >> in CameraSensorLegacy, so Ack here.\n> >>\n> >> <snip because large diff>\n> >>\n> >>> -void CamHelperOv5647::getDelays(int &exposureDelay, int &gainDelay,\n> >>> -                               int &vblankDelay, int &hblankDelay) const\n> >>> -{\n> >>> -       /*\n> >>> -        * We run this sensor in a mode where the gain delay is bumped up to\n> >>> -        * 2. It seems to be the only way to make the delays \"predictable\".\n> >>> -        */\n> >> We lose this comment as it's not in CameraSensorProperties, but the\n> >> values match and I think it's ok.\n> > It could be worth keeping the comment. I think AR0521 suffers from a\n> > similar issue (but the driver doesn't currently run it in a mode where\n> > the gain delay is predictable).\n>\n>\n> Laurent's eagle-eyes spotted that actually the OV5647 driver doesn't seem to configure a 2 frame\n> delay. The datasheet says bits [5:4] of register 0x3503 control the delay, with 11 being the setting\n> for 2 frames but in the driver those are set to 00 (which should be no delay). Do you happen to know\n> where this 2-frame delay value comes from?\n\nThis is one where the datasheet and the sensor behavior simply don't\nseem to match.  We got to these values empirically by adjusting\nsettings and looking for expected changes in the statistics.  Not sure\nwhat a 0 delay as described by the datasheet would even mean for a\nrolling shutter sensor!\n\nRegards,\nNaush\n\n\n>\n> >\n> >>> -       exposureDelay = 2;\n> >>> -       gainDelay = 2;\n> >>> -       vblankDelay = 2;\n> >>> -       hblankDelay = 2;\n> >>> -}\n> >>> -\n> >> <snip>\n> >>\n> >>> diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp\n> >>> index 5fce17e6..45e2a1d7 100644\n> >>> --- a/src/ipa/rpi/common/ipa_base.cpp\n> >>> +++ b/src/ipa/rpi/common/ipa_base.cpp\n> >>> @@ -134,18 +134,8 @@ int32_t IpaBase::init(const IPASettings &settings, const InitParams &params, Ini\n> >>>                  return -EINVAL;\n> >>>          }\n> >>>\n> >>> -       /*\n> >>> -        * Pass out the sensor config to the pipeline handler in order\n> >>> -        * to setup the staggered writer class.\n> >>> -        */\n> >>> -       int gainDelay, exposureDelay, vblankDelay, hblankDelay, sensorMetadata;\n> >>> -       helper_->getDelays(exposureDelay, gainDelay, vblankDelay, hblankDelay);\n> >>> -       sensorMetadata = helper_->sensorEmbeddedDataPresent();\n> >>> -\n> >>> -       result->sensorConfig.gainDelay = gainDelay;\n> >>> -       result->sensorConfig.exposureDelay = exposureDelay;\n> >>> -       result->sensorConfig.vblankDelay = vblankDelay;\n> >>> -       result->sensorConfig.hblankDelay = hblankDelay;\n> >>> +       /* Pass out the sensor metadata to the pipeline handler */\n> >>> +       int sensorMetadata = helper_->sensorEmbeddedDataPresent();\n> >>>          result->sensorConfig.sensorMetadata = sensorMetadata;\n> >>>\n> >>>          /* Load the tuning file for this sensor. */\n> >>> diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> >>> index 9e2d9d23..398a0280 100644\n> >>> --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> >>> +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> >>> @@ -816,11 +816,12 @@ int PipelineHandlerBase::registerCamera(std::unique_ptr<RPi::CameraData> &camera\n> >>>           * Setup our delayed control writer with the sensor default\n> >>>           * gain and exposure delays. Mark VBLANK for priority write.\n> >>>           */\n> >>> +       const CameraSensorProperties::SensorDelays &delays = data->sensor_->sensorDelays();\n> >>>          std::unordered_map<uint32_t, RPi::DelayedControls::ControlParams> params = {\n> >>> -               { V4L2_CID_ANALOGUE_GAIN, { result.sensorConfig.gainDelay, false } },\n> >>> -               { V4L2_CID_EXPOSURE, { result.sensorConfig.exposureDelay, false } },\n> >>> -               { V4L2_CID_HBLANK, { result.sensorConfig.hblankDelay, false } },\n> >>> -               { V4L2_CID_VBLANK, { result.sensorConfig.vblankDelay, true } }\n> >>> +               { V4L2_CID_ANALOGUE_GAIN, { delays.gainDelay, false } },\n> >>> +               { V4L2_CID_EXPOSURE, { delays.exposureDelay, false } },\n> >>> +               { V4L2_CID_HBLANK, { delays.hblankDelay, false } },\n> >>> +               { V4L2_CID_VBLANK, { delays.vblankDelay, true } }\n> >> And that's all simplified and now unified so we have a single table of\n> >> truth for sensor delay values.\n> >>\n> >> so:\n> >>\n> >>\n> >> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n> >>\n> >>\n> >>>          };\n> >>>          data->delayedCtrls_ = std::make_unique<RPi::DelayedControls>(data->sensor_->device(), params);\n> >>>          data->sensorMetadata_ = result.sensorConfig.sensorMetadata;","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 77FB9C3301\n\tfor <parsemail@patchwork.libcamera.org>;\n\tWed, 18 Dec 2024 08:34:20 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 69B9A68085;\n\tWed, 18 Dec 2024 09:34:19 +0100 (CET)","from mail-yw1-x112c.google.com (mail-yw1-x112c.google.com\n\t[IPv6:2607:f8b0:4864:20::112c])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 802E167F24\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 18 Dec 2024 09:34:17 +0100 (CET)","by mail-yw1-x112c.google.com with SMTP id\n\t00721157ae682-6eec9d2c54bso12571567b3.2\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 18 Dec 2024 00:34:17 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"dtvBZqHW\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1734510856; x=1735115656;\n\tdarn=lists.libcamera.org; \n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc:subject:date:message-id:reply-to;\n\tbh=aGWrXoQiA3VrBQEOsfdhkwJXI9VZTTCiNhDQU4wwqUg=;\n\tb=dtvBZqHWc2CIUw0O/QpPDX87cPKjWvc/bWP+U2wAFrlxnOc5SNowUvkafs1XUCNcIG\n\tQj9+f5EcocKSDs/CV/q1lP6pJ8rS89F6qRvISkjNm7FmN6HzB/Wq25BwWa7D6Gv6JHH1\n\td0Mv54DZY4TMWn+LrHI7/O2zY2a9kGT0l6ksgUprJvoSayP/2CaUuSxigzM4Iwhrwr08\n\tw4HlGn+jcDVveUnEgPMLvlkCZ6uWQ3AOd7FOZUmN0iWSMTHdt01/8RDWmliBWkHs7u7a\n\tu10RAj5hntrkoIfpPk1UDU/WQ/icpoPwvWt+d9bSz7unhKNm6o9uXJHGGuNBvnwP+5HY\n\t/ANA==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1734510856; x=1735115656;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-message-state:from:to:cc:subject:date:message-id\n\t:reply-to;\n\tbh=aGWrXoQiA3VrBQEOsfdhkwJXI9VZTTCiNhDQU4wwqUg=;\n\tb=tZAIyzIOzz6/MZTDx/1tlUh3wEOfsfgfmDOXcKxW4iakOFO+ayouK5rY5o3bx0aRdf\n\tUQ5ggrK08dHPxZ05r0fzAG9En7w0AWEJzW8zX4iFnLZDlQklb7P/8L+hrqnPk+LPM6qW\n\tbCDiPnZo8JXJlzKqkieXn70wb/44R2raC+PQyKaW7/+WUbo0dVG7QEtWMv83oRdA1qfF\n\txgbeingRxLlS7l8KCjRastUHH802pMugK5GwxsKYh0iG3LKDsMlA5MrX0MomiYdI/jmt\n\teG/FdWa6+ssnqQD6EnC0myJofIkajNmVHt9QmR/xR21eAgglPsaA5INewsOHdmhR7Q8X\n\t7KvA==","X-Forwarded-Encrypted":"i=1;\n\tAJvYcCUvyuCLA4qWr6BWaz9UwN9NCF3qD6PGOWW1PeeHIR+yGyikCecj+oqUb0svzbD2sI5JLlT7/kYG0TIFBlPtfOo=@lists.libcamera.org","X-Gm-Message-State":"AOJu0YyZnoE21HnxmWsd0CD+RzfifjGq8Ob81NbaRL1h79YAfOPIjipe\n\tjUeuin5ttLu5Cd74sTfihIGpItZ9jKX52ElyBGvQgszv+7dp4UNYKqjaMbk/M/3UiF+jKlU7sut\n\tr0FgvEtESz7d+AYeeCOygqx1FSyu/vouDZQeMAg==","X-Gm-Gg":"ASbGncs162HU/a6mULQqmZCdWR7vu6CVG03sLWO1+ujy+LeChG+q7lN1X1XK0IRYTu8\n\tDt8kxn1fU5DcjRKqsQs4FjQQihrMS1jBhxo1+JKqqp6mfY+vcsl3iubiZpbQhP0qJ+mwr1A==","X-Google-Smtp-Source":"AGHT+IFvUo+ltr5czqsNUrXZoiUHdFWO3l86YSBz2JjZmQQIOrLPyy5Zcrz9mDnp9qcRfUoMIuf75aNxJib6c6A5ZG4=","X-Received":"by 2002:a05:6902:2786:b0:e38:2041:e9c4 with SMTP id\n\t3f1490d57ef6-e53621b144emr653500276.4.1734510856216; Wed, 18 Dec 2024\n\t00:34:16 -0800 (PST)","MIME-Version":"1.0","References":"<20241127133233.247977-1-dan.scally@ideasonboard.com>\n\t<20241127133233.247977-3-dan.scally@ideasonboard.com>\n\t<173445401236.32744.1757422668793523225@ping.linuxembedded.co.uk>\n\t<20241217165612.GJ20432@pendragon.ideasonboard.com>\n\t<7f14da10-42b3-4ada-ac73-c14b2f09a19d@ideasonboard.com>","In-Reply-To":"<7f14da10-42b3-4ada-ac73-c14b2f09a19d@ideasonboard.com>","From":"Naushir Patuck <naush@raspberrypi.com>","Date":"Wed, 18 Dec 2024 08:33:41 +0000","Message-ID":"<CAEmqJPrO-KM2tqgqDiN1frpkD36mdr3np14jXv45+G1LVfNkYQ@mail.gmail.com>","Subject":"Re: [PATCH 2/2] libcamera: rpi: Draw sensor delays from\n\tCameraSensorProperties","To":"Dan Scally <dan.scally@ideasonboard.com>","Cc":"david.plowman@raspberrypi.com, libcamera-devel@lists.libcamera.org, \n\tKieran Bingham <kieran.bingham@ideasonboard.com>, \n\tLaurent Pinchart <laurent.pinchart@ideasonboard.com>","Content-Type":"text/plain; charset=\"UTF-8\"","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]