[{"id":32183,"web_url":"https://patchwork.libcamera.org/comment/32183/","msgid":"<173167262754.4187655.11344145749616706346@ping.linuxembedded.co.uk>","date":"2024-11-15T12:10:27","subject":"Re: [PATCH v3 5/6] libcamera: camera_sensor_properties: Add sensor\n\tcontrol delays","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-15 07:46:27)\n> Add properties covering the sensor control application delays to both\n> the static CameraSensorProperties definitions. The values used are the\n> defaults that're in use across the library, with deviations from that\n> taken from Raspberry Pi's CamHelper class definitions.\n> \n> Signed-off-by: Daniel Scally \n> ---\n> Changes in v3:\n> \n> - Rebased on top of the CameraSensorFactory introduction\n> - Some rephrasing\n> - Defined the sensorDelays member as empty where Raspberry Pi didn't\n> have any specific values. Check for the empty struct in\n> getSensorDelays() and return the defaults from there with a warning.\n\nSounds resonable to me.\n\n\n> \n> Changes in v2:\n> \n> - Rather than adding the delays to the properties ControlList, added a\n> new function in CameraSensor that allows PipelineHandlers to retreive\n> the delay values.\n> \n> include/libcamera/internal/camera_sensor.h | 2 +\n> .../internal/camera_sensor_properties.h | 9 +++\n> src/libcamera/sensor/camera_sensor.cpp | 13 +++\n> src/libcamera/sensor/camera_sensor_legacy.cpp | 33 ++++++++\n> .../sensor/camera_sensor_properties.cpp | 79 +++++++++++++++++++\n> 5 files changed, 136 insertions(+)\n> \n> diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h\n> index 8aafd82e..a9b2d494 100644\n> --- a/include/libcamera/internal/camera_sensor.h\n> +++ b/include/libcamera/internal/camera_sensor.h\n> @@ -73,6 +73,8 @@ public:\n> virtual const std::vector &\n> testPatternModes() const = 0;\n> virtual int setTestPatternMode(controls::draft::TestPatternModeEnum mode) = 0;\n> + virtual void getSensorDelays(uint8_t &exposureDelay, uint8_t &gainDelay,\n> + uint8_t &vblankDelay, uint8_t &hblankDelay) = 0;\n\nCould/Should this return a const pointer to a struct SensorDelays now ?\n\n> };\n> \n> class CameraSensorFactoryBase\n> diff --git a/include/libcamera/internal/camera_sensor_properties.h b/include/libcamera/internal/camera_sensor_properties.h\n> index 480ac121..56d5c15d 100644\n> --- a/include/libcamera/internal/camera_sensor_properties.h\n> +++ b/include/libcamera/internal/camera_sensor_properties.h\n> @@ -10,6 +10,8 @@\n> #include \n> #include \n> \n> +#include \n> +\n> #include \n> #include \n> \n> @@ -20,6 +22,13 @@ struct CameraSensorProperties {\n> \n> Size unitCellSize;\n> std::map testPatternModes;\n> +\n> + struct {\n> + uint8_t exposureDelay;\n> + uint8_t gainDelay;\n> + uint8_t vblankDelay;\n> + uint8_t hblankDelay;\n> + } sensorDelays;\n> };\n> \n> } /* namespace libcamera */\n> diff --git a/src/libcamera/sensor/camera_sensor.cpp b/src/libcamera/sensor/camera_sensor.cpp\n> index 54cf98b2..61420873 100644\n> --- a/src/libcamera/sensor/camera_sensor.cpp\n> +++ b/src/libcamera/sensor/camera_sensor.cpp\n> @@ -336,6 +336,19 @@ CameraSensor::~CameraSensor() = default;\n> * pattern mode for every frame thus incurs no performance penalty.\n> */\n> \n> +/**\n> + * \\fn CameraSensor::getSensorDelays()\n> + * \\brief Fetch the sensor delay values\n> + * \\param[out] exposureDelay The exposure delay\n> + * \\param[out] gainDelay The analogue gain delay\n> + * \\param[out] vblankDelay The vblank delay\n> + * \\param[out] hblankDelay The hblank delay\n> + *\n> + * This function fills in sensor control delays for pipeline handlers to use to\n> + * inform the DelayedControls. If no static properties are available it fills in\n> + * some widely applicable default values.\n> + */\n> +\n> /**\n> * \\class CameraSensorFactoryBase\n> * \\brief Base class for camera sensor factories\n> diff --git a/src/libcamera/sensor/camera_sensor_legacy.cpp b/src/libcamera/sensor/camera_sensor_legacy.cpp\n> index a9b15c03..84972f02 100644\n> --- a/src/libcamera/sensor/camera_sensor_legacy.cpp\n> +++ b/src/libcamera/sensor/camera_sensor_legacy.cpp\n> @@ -95,6 +95,8 @@ public:\n> const std::vector &\n> testPatternModes() const override { return testPatternModes_; }\n> int setTestPatternMode(controls::draft::TestPatternModeEnum mode) override;\n> + void getSensorDelays(uint8_t &exposureDelay, uint8_t &gainDelay,\n> + uint8_t &vblankDelay, uint8_t &hblankDelay) override;\n> \n> protected:\n> std::string logPrefix() const override;\n> @@ -482,6 +484,37 @@ void CameraSensorLegacy::initStaticProperties()\n> initTestPatternModes();\n> }\n> \n> +void CameraSensorLegacy::getSensorDelays(uint8_t &exposureDelay, uint8_t &gainDelay,\n> + uint8_t &vblankDelay, uint8_t &hblankDelay)\n> +{\n> +\n> + /*\n> + * These defaults are applicable to many sensors, however more specific\n> + * values can be added to the CameraSensorProperties for a sensor if\n> + * required.\n> + */\n> + if (!staticProps_ ||\n> + (!staticProps_->sensorDelays.exposureDelay &&\n> + !staticProps_->sensorDelays.gainDelay &&\n> + !staticProps_->sensorDelays.vblankDelay &&\n> + !staticProps_->sensorDelays.hblankDelay)) {\n> + LOG(CameraSensor, Warning)\n> + << \"No sensor delays found in static properties. \"\n> + \"Assuming unverified defaults.\";\n> +\n> + exposureDelay = 2;\n> + gainDelay = 1;\n> + vblankDelay = 2;\n> + hblankDelay = 2;\n> + return;\n> + }\n> +\n> + exposureDelay = staticProps_->sensorDelays.exposureDelay;\n> + gainDelay = staticProps_->sensorDelays.gainDelay;\n> + vblankDelay = staticProps_->sensorDelays.vblankDelay;\n> + hblankDelay = staticProps_->sensorDelays.hblankDelay;\n\nWhich would make this just\n\n\treturn staticProps_->sensorDelays;\n\n> +}\n\nI'm glad this function prints a warning!\n\n> +\n> void CameraSensorLegacy::initTestPatternModes()\n> {\n> const auto &v4l2TestPattern = controls().find(V4L2_CID_TEST_PATTERN);\n> diff --git a/src/libcamera/sensor/camera_sensor_properties.cpp b/src/libcamera/sensor/camera_sensor_properties.cpp\n> index 6d4136d0..6dda7ba9 100644\n> --- a/src/libcamera/sensor/camera_sensor_properties.cpp\n> +++ b/src/libcamera/sensor/camera_sensor_properties.cpp\n> @@ -41,6 +41,13 @@ LOG_DEFINE_CATEGORY(CameraSensorProperties)\n> * \\brief Map that associates the TestPattern control value with the indexes of\n> * the corresponding sensor test pattern modes as returned by\n> * V4L2_CID_TEST_PATTERN.\n> + *\n> + * \\var CameraSensorProperties::sensorDelays\n> + * \\brief Holds the delays, expressed in number of frames, between the time a\n> + * control is applied to the sensor and the time it actually takes effect.\n> + * Delays are recorded for the exposure time, analogue gain, vertical and\n> + * horizontal blanking. These values may be defined as empty, in which case the\n> + * CameraSensor derivative should provide default values.\n> */\n> \n> /**\n> @@ -60,6 +67,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n> { controls::draft::TestPatternModeColorBars, 2 },\n> { controls::draft::TestPatternModeColorBarsFadeToGray, 3 },\n> },\n> + .sensorDelays = { },\n> } },\n> { \"ar0521\", {\n> .unitCellSize = { 2200, 2200 },\n> @@ -69,6 +77,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n> { controls::draft::TestPatternModeColorBars, 2 },\n> { controls::draft::TestPatternModeColorBarsFadeToGray, 3 },\n> },\n> + .sensorDelays = { },\n> } },\n> { \"hi846\", {\n> .unitCellSize = { 1120, 1120 },\n> @@ -87,6 +96,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n> * 9: \"Resolution Pattern\"\n> */\n> },\n> + .sensorDelays = { },\n> } },\n> { \"imx214\", {\n> .unitCellSize = { 1120, 1120 },\n> @@ -97,6 +107,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n> { controls::draft::TestPatternModeColorBarsFadeToGray, 3 },\n> { controls::draft::TestPatternModePn9, 4 },\n> },\n> + .sensorDelays = { },\n> } },\n> { \"imx219\", {\n> .unitCellSize = { 1120, 1120 },\n> @@ -107,6 +118,12 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n> { controls::draft::TestPatternModeColorBarsFadeToGray, 3 },\n> { controls::draft::TestPatternModePn9, 4 },\n> },\n> + .sensorDelays = {\n> + .exposureDelay = 2,\n> + .gainDelay = 1,\n> + .vblankDelay = 2,\n> + .hblankDelay = 2\n> + },\n> } },\n> { \"imx258\", {\n> .unitCellSize = { 1120, 1120 },\n> @@ -117,34 +134,62 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n> { controls::draft::TestPatternModeColorBarsFadeToGray, 3 },\n> { controls::draft::TestPatternModePn9, 4 },\n> },\n> + .sensorDelays = { },\n> } },\n> { \"imx283\", {\n> .unitCellSize = { 2400, 2400 },\n> .testPatternModes = {},\n> + .sensorDelays = {\n> + .exposureDelay = 2,\n> + .gainDelay = 2,\n> + .vblankDelay = 2,\n> + .hblankDelay = 2\n> + },\n> } },\n> { \"imx290\", {\n> .unitCellSize = { 2900, 2900 },\n> .testPatternModes = {},\n> + .sensorDelays = {\n> + .exposureDelay = 2,\n> + .gainDelay = 2,\n> + .vblankDelay = 2,\n> + .hblankDelay = 2\n> + },\n> } },\n> { \"imx296\", {\n> .unitCellSize = { 3450, 3450 },\n> .testPatternModes = {},\n> + .sensorDelays = {\n> + .exposureDelay = 2,\n> + .gainDelay = 2,\n> + .vblankDelay = 2,\n> + .hblankDelay = 2\n> + },\n> } },\n> { \"imx327\", {\n> .unitCellSize = { 2900, 2900 },\n> .testPatternModes = {},\n> + .sensorDelays = { },\n> } },\n> { \"imx335\", {\n> .unitCellSize = { 2000, 2000 },\n> .testPatternModes = {},\n> + .sensorDelays = { },\n> } },\n> { \"imx415\", {\n> .unitCellSize = { 1450, 1450 },\n> .testPatternModes = {},\n> + .sensorDelays = { },\n> } },\n> { \"imx477\", {\n> .unitCellSize = { 1550, 1550 },\n> .testPatternModes = {},\n> + .sensorDelays = {\n> + .exposureDelay = 2,\n> + .gainDelay = 2,\n> + .vblankDelay = 3,\n> + .hblankDelay = 3\n> + },\n> } },\n> { \"imx519\", {\n> .unitCellSize = { 1220, 1220 },\n> @@ -157,6 +202,12 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n> * these two patterns do not comply with MIPI CCS v1.1 (Section 10.1).\n> */\n> },\n> + .sensorDelays = {\n> + .exposureDelay = 2,\n> + .gainDelay = 2,\n> + .vblankDelay = 3,\n> + .hblankDelay = 3\n> + },\n> } },\n> { \"imx708\", {\n> .unitCellSize = { 1400, 1400 },\n> @@ -167,6 +218,12 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n> { controls::draft::TestPatternModeColorBarsFadeToGray, 3 },\n> { controls::draft::TestPatternModePn9, 4 },\n> },\n> + .sensorDelays = {\n> + .exposureDelay = 2,\n> + .gainDelay = 2,\n> + .vblankDelay = 3,\n> + .hblankDelay = 3\n> + },\n> } },\n> { \"ov2685\", {\n> .unitCellSize = { 1750, 1750 },\n> @@ -181,6 +238,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n> * 5: \"Color Square\"\n> */\n> },\n> + .sensorDelays = { },\n> } },\n> { \"ov2740\", {\n> .unitCellSize = { 1400, 1400 },\n> @@ -188,6 +246,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n> { controls::draft::TestPatternModeOff, 0 },\n> { controls::draft::TestPatternModeColorBars, 1},\n> },\n> + .sensorDelays = { },\n> } },\n> { \"ov4689\", {\n> .unitCellSize = { 2000, 2000 },\n> @@ -201,6 +260,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n> * colorBarType2 and colorBarType3.\n> */\n> },\n> + .sensorDelays = { },\n> } },\n> { \"ov5640\", {\n> .unitCellSize = { 1400, 1400 },\n> @@ -208,10 +268,17 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n> { controls::draft::TestPatternModeOff, 0 },\n> { controls::draft::TestPatternModeColorBars, 1 },\n> },\n> + .sensorDelays = { },\n> } },\n> { \"ov5647\", {\n> .unitCellSize = { 1400, 1400 },\n> .testPatternModes = {},\n> + .sensorDelays = {\n> + .exposureDelay = 2,\n> + .gainDelay = 2,\n> + .vblankDelay = 2,\n> + .hblankDelay = 2\n> + },\n> } },\n> { \"ov5670\", {\n> .unitCellSize = { 1120, 1120 },\n> @@ -219,6 +286,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n> { controls::draft::TestPatternModeOff, 0 },\n> { controls::draft::TestPatternModeColorBars, 1 },\n> },\n> + .sensorDelays = { },\n> } },\n> { \"ov5675\", {\n> .unitCellSize = { 1120, 1120 },\n> @@ -226,6 +294,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n> { controls::draft::TestPatternModeOff, 0 },\n> { controls::draft::TestPatternModeColorBars, 1 },\n> },\n> + .sensorDelays = { },\n> } },\n> { \"ov5693\", {\n> .unitCellSize = { 1400, 1400 },\n> @@ -238,6 +307,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n> * Rolling Bar\".\n> */\n> },\n> + .sensorDelays = { },\n> } },\n> { \"ov64a40\", {\n> .unitCellSize = { 1008, 1008 },\n> @@ -251,6 +321,12 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n> * 4: \"Vertical Color Bar Type 4\"\n> */\n> },\n> + .sensorDelays = {\n> + .exposureDelay = 2,\n> + .gainDelay = 2,\n> + .vblankDelay = 2,\n> + .hblankDelay = 2\n> + },\n> } },\n> { \"ov8858\", {\n> .unitCellSize = { 1120, 1120 },\n> @@ -264,6 +340,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n> * 4: \"Vertical Color Bar Type 4\"\n> */\n> },\n> + .sensorDelays = { },\n> } },\n> { \"ov8865\", {\n> .unitCellSize = { 1400, 1400 },\n> @@ -278,6 +355,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n> * 5: \"Color squares with rolling bar\"\n> */\n> },\n> + .sensorDelays = { },\n> } },\n> { \"ov13858\", {\n> .unitCellSize = { 1120, 1120 },\n> @@ -285,6 +363,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n> { controls::draft::TestPatternModeOff, 0 },\n> { controls::draft::TestPatternModeColorBars, 1 },\n> },\n> + .sensorDelays = { },\n> } },\n> };\n> \n> -- \n> 2.30.2\n>","headers":{"Return-Path":"","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 30502C0F1B\n\tfor ;\n\tFri, 15 Nov 2024 12:10:32 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 595DF65873;\n\tFri, 15 Nov 2024 13:10:31 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 815606580A\n\tfor ;\n\tFri, 15 Nov 2024 13:10:30 +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 B1522496;\n\tFri, 15 Nov 2024 13:10:15 +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=\"le7uPfBE\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1731672615;\n\tbh=1JdbXVRtzOvpintmm2sySRGvV9D6rOZ/x4dcks5+Ywc=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=le7uPfBEGfb4ezpDfmnsgQ/KRSdo+uRXBcN1TQQX7OZq4UPG8nzzUjcbSzoiLvjnt\n\t8bdigaPwd7p5BzOQIX7iBSFGagMsxzG5dSoStGvpLB4B2jFLZiaZIkQlBtiobNyOZa\n\tNjxdrT+ytcQK0fpb04enlZzevPTPcEnwGz2o2qiE=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20241115074628.417215-6-dan.scally@ideasonboard.com>","References":"<20241115074628.417215-1-dan.scally@ideasonboard.com>\n\t<20241115074628.417215-6-dan.scally@ideasonboard.com>","Subject":"Re: [PATCH v3 5/6] libcamera: camera_sensor_properties: Add sensor\n\tcontrol delays","From":"Kieran Bingham ","Cc":"mike.rudenko@gmail.com, Daniel Scally ","To":"Daniel Scally ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Fri, 15 Nov 2024 12:10:27 +0000","Message-ID":"<173167262754.4187655.11344145749616706346@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":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":32191,"web_url":"https://patchwork.libcamera.org/comment/32191/","msgid":"","date":"2024-11-15T13:35:54","subject":"Re: [PATCH v3 5/6] libcamera: camera_sensor_properties: Add sensor\n\tcontrol delays","submitter":{"id":156,"url":"https://patchwork.libcamera.org/api/people/156/","name":"Dan Scally","email":"dan.scally@ideasonboard.com"},"content":"Hi Kieran\n\nOn 15/11/2024 12:10, Kieran Bingham wrote:\n> Quoting Daniel Scally (2024-11-15 07:46:27)\n>> Add properties covering the sensor control application delays to both\n>> the static CameraSensorProperties definitions. The values used are the\n>> defaults that're in use across the library, with deviations from that\n>> taken from Raspberry Pi's CamHelper class definitions.\n>>\n>> Signed-off-by: Daniel Scally \n>> ---\n>> Changes in v3:\n>>\n>> - Rebased on top of the CameraSensorFactory introduction\n>> - Some rephrasing\n>> - Defined the sensorDelays member as empty where Raspberry Pi didn't\n>> have any specific values. Check for the empty struct in\n>> getSensorDelays() and return the defaults from there with a warning.\n> Sounds resonable to me.\n>\n>\n>> Changes in v2:\n>>\n>> - Rather than adding the delays to the properties ControlList, added a\n>> new function in CameraSensor that allows PipelineHandlers to retreive\n>> the delay values.\n>>\n>> include/libcamera/internal/camera_sensor.h | 2 +\n>> .../internal/camera_sensor_properties.h | 9 +++\n>> src/libcamera/sensor/camera_sensor.cpp | 13 +++\n>> src/libcamera/sensor/camera_sensor_legacy.cpp | 33 ++++++++\n>> .../sensor/camera_sensor_properties.cpp | 79 +++++++++++++++++++\n>> 5 files changed, 136 insertions(+)\n>>\n>> diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h\n>> index 8aafd82e..a9b2d494 100644\n>> --- a/include/libcamera/internal/camera_sensor.h\n>> +++ b/include/libcamera/internal/camera_sensor.h\n>> @@ -73,6 +73,8 @@ public:\n>> virtual const std::vector &\n>> testPatternModes() const = 0;\n>> virtual int setTestPatternMode(controls::draft::TestPatternModeEnum mode) = 0;\n>> + virtual void getSensorDelays(uint8_t &exposureDelay, uint8_t &gainDelay,\n>> + uint8_t &vblankDelay, uint8_t &hblankDelay) = 0;\n> Could/Should this return a const pointer to a struct SensorDelays now ?\nYeah maybe...that would add a requirement for the struct to be available to CameraSensor (which is a \nFactory class now) - I don't really have any strong feelings either way, so as long as that's fine \nthen I'll make the change.\n>\n>> };\n>> \n>> class CameraSensorFactoryBase\n>> diff --git a/include/libcamera/internal/camera_sensor_properties.h b/include/libcamera/internal/camera_sensor_properties.h\n>> index 480ac121..56d5c15d 100644\n>> --- a/include/libcamera/internal/camera_sensor_properties.h\n>> +++ b/include/libcamera/internal/camera_sensor_properties.h\n>> @@ -10,6 +10,8 @@\n>> #include \n>> #include \n>> \n>> +#include \n>> +\n>> #include \n>> #include \n>> \n>> @@ -20,6 +22,13 @@ struct CameraSensorProperties {\n>> \n>> Size unitCellSize;\n>> std::map testPatternModes;\n>> +\n>> + struct {\n>> + uint8_t exposureDelay;\n>> + uint8_t gainDelay;\n>> + uint8_t vblankDelay;\n>> + uint8_t hblankDelay;\n>> + } sensorDelays;\n>> };\n>> \n>> } /* namespace libcamera */\n>> diff --git a/src/libcamera/sensor/camera_sensor.cpp b/src/libcamera/sensor/camera_sensor.cpp\n>> index 54cf98b2..61420873 100644\n>> --- a/src/libcamera/sensor/camera_sensor.cpp\n>> +++ b/src/libcamera/sensor/camera_sensor.cpp\n>> @@ -336,6 +336,19 @@ CameraSensor::~CameraSensor() = default;\n>> * pattern mode for every frame thus incurs no performance penalty.\n>> */\n>> \n>> +/**\n>> + * \\fn CameraSensor::getSensorDelays()\n>> + * \\brief Fetch the sensor delay values\n>> + * \\param[out] exposureDelay The exposure delay\n>> + * \\param[out] gainDelay The analogue gain delay\n>> + * \\param[out] vblankDelay The vblank delay\n>> + * \\param[out] hblankDelay The hblank delay\n>> + *\n>> + * This function fills in sensor control delays for pipeline handlers to use to\n>> + * inform the DelayedControls. If no static properties are available it fills in\n>> + * some widely applicable default values.\n>> + */\n>> +\n>> /**\n>> * \\class CameraSensorFactoryBase\n>> * \\brief Base class for camera sensor factories\n>> diff --git a/src/libcamera/sensor/camera_sensor_legacy.cpp b/src/libcamera/sensor/camera_sensor_legacy.cpp\n>> index a9b15c03..84972f02 100644\n>> --- a/src/libcamera/sensor/camera_sensor_legacy.cpp\n>> +++ b/src/libcamera/sensor/camera_sensor_legacy.cpp\n>> @@ -95,6 +95,8 @@ public:\n>> const std::vector &\n>> testPatternModes() const override { return testPatternModes_; }\n>> int setTestPatternMode(controls::draft::TestPatternModeEnum mode) override;\n>> + void getSensorDelays(uint8_t &exposureDelay, uint8_t &gainDelay,\n>> + uint8_t &vblankDelay, uint8_t &hblankDelay) override;\n>> \n>> protected:\n>> std::string logPrefix() const override;\n>> @@ -482,6 +484,37 @@ void CameraSensorLegacy::initStaticProperties()\n>> initTestPatternModes();\n>> }\n>> \n>> +void CameraSensorLegacy::getSensorDelays(uint8_t &exposureDelay, uint8_t &gainDelay,\n>> + uint8_t &vblankDelay, uint8_t &hblankDelay)\n>> +{\n>> +\n>> + /*\n>> + * These defaults are applicable to many sensors, however more specific\n>> + * values can be added to the CameraSensorProperties for a sensor if\n>> + * required.\n>> + */\n>> + if (!staticProps_ ||\n>> + (!staticProps_->sensorDelays.exposureDelay &&\n>> + !staticProps_->sensorDelays.gainDelay &&\n>> + !staticProps_->sensorDelays.vblankDelay &&\n>> + !staticProps_->sensorDelays.hblankDelay)) {\n>> + LOG(CameraSensor, Warning)\n>> + << \"No sensor delays found in static properties. \"\n>> + \"Assuming unverified defaults.\";\n>> +\n>> + exposureDelay = 2;\n>> + gainDelay = 1;\n>> + vblankDelay = 2;\n>> + hblankDelay = 2;\n>> + return;\n>> + }\n>> +\n>> + exposureDelay = staticProps_->sensorDelays.exposureDelay;\n>> + gainDelay = staticProps_->sensorDelays.gainDelay;\n>> + vblankDelay = staticProps_->sensorDelays.vblankDelay;\n>> + hblankDelay = staticProps_->sensorDelays.hblankDelay;\n> Which would make this just\n>\n> \treturn staticProps_->sensorDelays;\n>\n>> +}\n> I'm glad this function prints a warning!\n>\n>> +\n>> void CameraSensorLegacy::initTestPatternModes()\n>> {\n>> const auto &v4l2TestPattern = controls().find(V4L2_CID_TEST_PATTERN);\n>> diff --git a/src/libcamera/sensor/camera_sensor_properties.cpp b/src/libcamera/sensor/camera_sensor_properties.cpp\n>> index 6d4136d0..6dda7ba9 100644\n>> --- a/src/libcamera/sensor/camera_sensor_properties.cpp\n>> +++ b/src/libcamera/sensor/camera_sensor_properties.cpp\n>> @@ -41,6 +41,13 @@ LOG_DEFINE_CATEGORY(CameraSensorProperties)\n>> * \\brief Map that associates the TestPattern control value with the indexes of\n>> * the corresponding sensor test pattern modes as returned by\n>> * V4L2_CID_TEST_PATTERN.\n>> + *\n>> + * \\var CameraSensorProperties::sensorDelays\n>> + * \\brief Holds the delays, expressed in number of frames, between the time a\n>> + * control is applied to the sensor and the time it actually takes effect.\n>> + * Delays are recorded for the exposure time, analogue gain, vertical and\n>> + * horizontal blanking. These values may be defined as empty, in which case the\n>> + * CameraSensor derivative should provide default values.\n>> */\n>> \n>> /**\n>> @@ -60,6 +67,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n>> { controls::draft::TestPatternModeColorBars, 2 },\n>> { controls::draft::TestPatternModeColorBarsFadeToGray, 3 },\n>> },\n>> + .sensorDelays = { },\n>> } },\n>> { \"ar0521\", {\n>> .unitCellSize = { 2200, 2200 },\n>> @@ -69,6 +77,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n>> { controls::draft::TestPatternModeColorBars, 2 },\n>> { controls::draft::TestPatternModeColorBarsFadeToGray, 3 },\n>> },\n>> + .sensorDelays = { },\n>> } },\n>> { \"hi846\", {\n>> .unitCellSize = { 1120, 1120 },\n>> @@ -87,6 +96,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n>> * 9: \"Resolution Pattern\"\n>> */\n>> },\n>> + .sensorDelays = { },\n>> } },\n>> { \"imx214\", {\n>> .unitCellSize = { 1120, 1120 },\n>> @@ -97,6 +107,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n>> { controls::draft::TestPatternModeColorBarsFadeToGray, 3 },\n>> { controls::draft::TestPatternModePn9, 4 },\n>> },\n>> + .sensorDelays = { },\n>> } },\n>> { \"imx219\", {\n>> .unitCellSize = { 1120, 1120 },\n>> @@ -107,6 +118,12 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n>> { controls::draft::TestPatternModeColorBarsFadeToGray, 3 },\n>> { controls::draft::TestPatternModePn9, 4 },\n>> },\n>> + .sensorDelays = {\n>> + .exposureDelay = 2,\n>> + .gainDelay = 1,\n>> + .vblankDelay = 2,\n>> + .hblankDelay = 2\n>> + },\n>> } },\n>> { \"imx258\", {\n>> .unitCellSize = { 1120, 1120 },\n>> @@ -117,34 +134,62 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n>> { controls::draft::TestPatternModeColorBarsFadeToGray, 3 },\n>> { controls::draft::TestPatternModePn9, 4 },\n>> },\n>> + .sensorDelays = { },\n>> } },\n>> { \"imx283\", {\n>> .unitCellSize = { 2400, 2400 },\n>> .testPatternModes = {},\n>> + .sensorDelays = {\n>> + .exposureDelay = 2,\n>> + .gainDelay = 2,\n>> + .vblankDelay = 2,\n>> + .hblankDelay = 2\n>> + },\n>> } },\n>> { \"imx290\", {\n>> .unitCellSize = { 2900, 2900 },\n>> .testPatternModes = {},\n>> + .sensorDelays = {\n>> + .exposureDelay = 2,\n>> + .gainDelay = 2,\n>> + .vblankDelay = 2,\n>> + .hblankDelay = 2\n>> + },\n>> } },\n>> { \"imx296\", {\n>> .unitCellSize = { 3450, 3450 },\n>> .testPatternModes = {},\n>> + .sensorDelays = {\n>> + .exposureDelay = 2,\n>> + .gainDelay = 2,\n>> + .vblankDelay = 2,\n>> + .hblankDelay = 2\n>> + },\n>> } },\n>> { \"imx327\", {\n>> .unitCellSize = { 2900, 2900 },\n>> .testPatternModes = {},\n>> + .sensorDelays = { },\n>> } },\n>> { \"imx335\", {\n>> .unitCellSize = { 2000, 2000 },\n>> .testPatternModes = {},\n>> + .sensorDelays = { },\n>> } },\n>> { \"imx415\", {\n>> .unitCellSize = { 1450, 1450 },\n>> .testPatternModes = {},\n>> + .sensorDelays = { },\n>> } },\n>> { \"imx477\", {\n>> .unitCellSize = { 1550, 1550 },\n>> .testPatternModes = {},\n>> + .sensorDelays = {\n>> + .exposureDelay = 2,\n>> + .gainDelay = 2,\n>> + .vblankDelay = 3,\n>> + .hblankDelay = 3\n>> + },\n>> } },\n>> { \"imx519\", {\n>> .unitCellSize = { 1220, 1220 },\n>> @@ -157,6 +202,12 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n>> * these two patterns do not comply with MIPI CCS v1.1 (Section 10.1).\n>> */\n>> },\n>> + .sensorDelays = {\n>> + .exposureDelay = 2,\n>> + .gainDelay = 2,\n>> + .vblankDelay = 3,\n>> + .hblankDelay = 3\n>> + },\n>> } },\n>> { \"imx708\", {\n>> .unitCellSize = { 1400, 1400 },\n>> @@ -167,6 +218,12 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n>> { controls::draft::TestPatternModeColorBarsFadeToGray, 3 },\n>> { controls::draft::TestPatternModePn9, 4 },\n>> },\n>> + .sensorDelays = {\n>> + .exposureDelay = 2,\n>> + .gainDelay = 2,\n>> + .vblankDelay = 3,\n>> + .hblankDelay = 3\n>> + },\n>> } },\n>> { \"ov2685\", {\n>> .unitCellSize = { 1750, 1750 },\n>> @@ -181,6 +238,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n>> * 5: \"Color Square\"\n>> */\n>> },\n>> + .sensorDelays = { },\n>> } },\n>> { \"ov2740\", {\n>> .unitCellSize = { 1400, 1400 },\n>> @@ -188,6 +246,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n>> { controls::draft::TestPatternModeOff, 0 },\n>> { controls::draft::TestPatternModeColorBars, 1},\n>> },\n>> + .sensorDelays = { },\n>> } },\n>> { \"ov4689\", {\n>> .unitCellSize = { 2000, 2000 },\n>> @@ -201,6 +260,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n>> * colorBarType2 and colorBarType3.\n>> */\n>> },\n>> + .sensorDelays = { },\n>> } },\n>> { \"ov5640\", {\n>> .unitCellSize = { 1400, 1400 },\n>> @@ -208,10 +268,17 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n>> { controls::draft::TestPatternModeOff, 0 },\n>> { controls::draft::TestPatternModeColorBars, 1 },\n>> },\n>> + .sensorDelays = { },\n>> } },\n>> { \"ov5647\", {\n>> .unitCellSize = { 1400, 1400 },\n>> .testPatternModes = {},\n>> + .sensorDelays = {\n>> + .exposureDelay = 2,\n>> + .gainDelay = 2,\n>> + .vblankDelay = 2,\n>> + .hblankDelay = 2\n>> + },\n>> } },\n>> { \"ov5670\", {\n>> .unitCellSize = { 1120, 1120 },\n>> @@ -219,6 +286,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n>> { controls::draft::TestPatternModeOff, 0 },\n>> { controls::draft::TestPatternModeColorBars, 1 },\n>> },\n>> + .sensorDelays = { },\n>> } },\n>> { \"ov5675\", {\n>> .unitCellSize = { 1120, 1120 },\n>> @@ -226,6 +294,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n>> { controls::draft::TestPatternModeOff, 0 },\n>> { controls::draft::TestPatternModeColorBars, 1 },\n>> },\n>> + .sensorDelays = { },\n>> } },\n>> { \"ov5693\", {\n>> .unitCellSize = { 1400, 1400 },\n>> @@ -238,6 +307,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n>> * Rolling Bar\".\n>> */\n>> },\n>> + .sensorDelays = { },\n>> } },\n>> { \"ov64a40\", {\n>> .unitCellSize = { 1008, 1008 },\n>> @@ -251,6 +321,12 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n>> * 4: \"Vertical Color Bar Type 4\"\n>> */\n>> },\n>> + .sensorDelays = {\n>> + .exposureDelay = 2,\n>> + .gainDelay = 2,\n>> + .vblankDelay = 2,\n>> + .hblankDelay = 2\n>> + },\n>> } },\n>> { \"ov8858\", {\n>> .unitCellSize = { 1120, 1120 },\n>> @@ -264,6 +340,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n>> * 4: \"Vertical Color Bar Type 4\"\n>> */\n>> },\n>> + .sensorDelays = { },\n>> } },\n>> { \"ov8865\", {\n>> .unitCellSize = { 1400, 1400 },\n>> @@ -278,6 +355,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n>> * 5: \"Color squares with rolling bar\"\n>> */\n>> },\n>> + .sensorDelays = { },\n>> } },\n>> { \"ov13858\", {\n>> .unitCellSize = { 1120, 1120 },\n>> @@ -285,6 +363,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n>> { controls::draft::TestPatternModeOff, 0 },\n>> { controls::draft::TestPatternModeColorBars, 1 },\n>> },\n>> + .sensorDelays = { },\n>> } },\n>> };\n>> \n>> -- \n>> 2.30.2\n>>","headers":{"Return-Path":"","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 B7D31C3257\n\tfor ;\n\tFri, 15 Nov 2024 13:36:00 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9732065890;\n\tFri, 15 Nov 2024 14:35:59 +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 99D7D65882\n\tfor ;\n\tFri, 15 Nov 2024 14:35:57 +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 859879CE;\n\tFri, 15 Nov 2024 14:35:42 +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=\"MRezoFLw\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1731677742;\n\tbh=gFTPuYXucs2ggXh5K0JvqS6Ew+glvLNnrwnh/ZZimho=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=MRezoFLwsY9Dd3g6/JPuK2wvEO5SoKbtV7oa9UD//T9edK/xL70VhhLDpxFd9lWCe\n\tC54mwKB5mubbG9ctdzajGe5LJRyV0xDWzB4C02oy7YMcRWomkfgmCm6tjjtb/79B0b\n\tzRWvbD44C9nkYWCMjubfJ0SlZJCm0uGp3Qe5oCWs=","Message-ID":"","Date":"Fri, 15 Nov 2024 13:35:54 +0000","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v3 5/6] libcamera: camera_sensor_properties: Add sensor\n\tcontrol delays","To":"Kieran Bingham ,\n\tlibcamera-devel@lists.libcamera.org","Cc":"mike.rudenko@gmail.com","References":"<20241115074628.417215-1-dan.scally@ideasonboard.com>\n\t<20241115074628.417215-6-dan.scally@ideasonboard.com>\n\t<173167262754.4187655.11344145749616706346@ping.linuxembedded.co.uk>","Content-Language":"en-US","From":"Dan Scally ","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":"<173167262754.4187655.11344145749616706346@ping.linuxembedded.co.uk>","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":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":32198,"web_url":"https://patchwork.libcamera.org/comment/32198/","msgid":"<173167997331.576258.7909958730811573777@ping.linuxembedded.co.uk>","date":"2024-11-15T14:12:53","subject":"Re: [PATCH v3 5/6] libcamera: camera_sensor_properties: Add sensor\n\tcontrol delays","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Dan Scally (2024-11-15 13:35:54)\n> Hi Kieran\n> \n> On 15/11/2024 12:10, Kieran Bingham wrote:\n> > Quoting Daniel Scally (2024-11-15 07:46:27)\n> >> Add properties covering the sensor control application delays to both\n> >> the static CameraSensorProperties definitions. The values used are the\n> >> defaults that're in use across the library, with deviations from that\n> >> taken from Raspberry Pi's CamHelper class definitions.\n> >>\n> >> Signed-off-by: Daniel Scally \n> >> ---\n> >> Changes in v3:\n> >>\n> >> - Rebased on top of the CameraSensorFactory introduction\n> >> - Some rephrasing\n> >> - Defined the sensorDelays member as empty where Raspberry Pi didn't\n> >> have any specific values. Check for the empty struct in\n> >> getSensorDelays() and return the defaults from there with a warning.\n> > Sounds resonable to me.\n> >\n> >\n> >> Changes in v2:\n> >>\n> >> - Rather than adding the delays to the properties ControlList, added a\n> >> new function in CameraSensor that allows PipelineHandlers to retreive\n> >> the delay values.\n> >>\n> >> include/libcamera/internal/camera_sensor.h | 2 +\n> >> .../internal/camera_sensor_properties.h | 9 +++\n> >> src/libcamera/sensor/camera_sensor.cpp | 13 +++\n> >> src/libcamera/sensor/camera_sensor_legacy.cpp | 33 ++++++++\n> >> .../sensor/camera_sensor_properties.cpp | 79 +++++++++++++++++++\n> >> 5 files changed, 136 insertions(+)\n> >>\n> >> diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h\n> >> index 8aafd82e..a9b2d494 100644\n> >> --- a/include/libcamera/internal/camera_sensor.h\n> >> +++ b/include/libcamera/internal/camera_sensor.h\n> >> @@ -73,6 +73,8 @@ public:\n> >> virtual const std::vector &\n> >> testPatternModes() const = 0;\n> >> virtual int setTestPatternMode(controls::draft::TestPatternModeEnum mode) = 0;\n> >> + virtual void getSensorDelays(uint8_t &exposureDelay, uint8_t &gainDelay,\n> >> + uint8_t &vblankDelay, uint8_t &hblankDelay) = 0;\n> > Could/Should this return a const pointer to a struct SensorDelays now ?\n> >\n> Yeah maybe...that would add a requirement for the struct to be available to CameraSensor (which is a \n> Factory class now) - I don't really have any strong feelings either way, so as long as that's fine \n> then I'll make the change.\n\nI think it's reasonable....\n\n--\nKieran","headers":{"Return-Path":"","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 63D69C0F1B\n\tfor ;\n\tFri, 15 Nov 2024 14:12:58 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A811265895;\n\tFri, 15 Nov 2024 15:12:57 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B04DD6580A\n\tfor ;\n\tFri, 15 Nov 2024 15:12:55 +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 DEEDB291;\n\tFri, 15 Nov 2024 15:12:40 +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=\"PZc9Fr1j\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1731679961;\n\tbh=fxkVv10hZ1FMlfznP5u2fDDfp4e8D17BcB+sDOvXfVc=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=PZc9Fr1joFursNEdTpVW9s9MLlSaRA6tFbGcVqspOAIoOxc71if13AKBONDysCSFw\n\t0Hdh6znNcd3epl6MG0OJ6rvZU0gfIg9XqqZ0IZu56rL8bl5PcVMmyYOCdMt452n68m\n\tw31Qg09o/F9t93pribZnUxN0JEAgJ9dLlkk6R1Ec=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"","References":"<20241115074628.417215-1-dan.scally@ideasonboard.com>\n\t<20241115074628.417215-6-dan.scally@ideasonboard.com>\n\t<173167262754.4187655.11344145749616706346@ping.linuxembedded.co.uk>\n\t","Subject":"Re: [PATCH v3 5/6] libcamera: camera_sensor_properties: Add sensor\n\tcontrol delays","From":"Kieran Bingham ","Cc":"mike.rudenko@gmail.com","To":"Dan Scally ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Fri, 15 Nov 2024 14:12:53 +0000","Message-ID":"<173167997331.576258.7909958730811573777@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":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":32207,"web_url":"https://patchwork.libcamera.org/comment/32207/","msgid":"<20241118013653.GI30787@pendragon.ideasonboard.com>","date":"2024-11-18T01:36:53","subject":"Re: [PATCH v3 5/6] libcamera: camera_sensor_properties: Add sensor\n\tcontrol delays","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Dan,\n\nOn Fri, Nov 15, 2024 at 01:35:54PM +0000, Daniel Scally wrote:\n> On 15/11/2024 12:10, Kieran Bingham wrote:\n> > Quoting Daniel Scally (2024-11-15 07:46:27)\n> >> Add properties covering the sensor control application delays to both\n> >> the static CameraSensorProperties definitions. The values used are the\n> >> defaults that're in use across the library, with deviations from that\n\nI had never seen \"that're\" before.\n\n> >> taken from Raspberry Pi's CamHelper class definitions.\n> >>\n> >> Signed-off-by: Daniel Scally \n> >> ---\n> >> Changes in v3:\n> >>\n> >> - Rebased on top of the CameraSensorFactory introduction\n> >> - Some rephrasing\n> >> - Defined the sensorDelays member as empty where Raspberry Pi didn't\n> >> have any specific values. Check for the empty struct in\n> >> getSensorDelays() and return the defaults from there with a warning.\n> >\n> > Sounds resonable to me.\n> >\n> >> Changes in v2:\n> >>\n> >> - Rather than adding the delays to the properties ControlList, added a\n> >> new function in CameraSensor that allows PipelineHandlers to retreive\n> >> the delay values.\n> >>\n> >> include/libcamera/internal/camera_sensor.h | 2 +\n> >> .../internal/camera_sensor_properties.h | 9 +++\n> >> src/libcamera/sensor/camera_sensor.cpp | 13 +++\n> >> src/libcamera/sensor/camera_sensor_legacy.cpp | 33 ++++++++\n> >> .../sensor/camera_sensor_properties.cpp | 79 +++++++++++++++++++\n> >> 5 files changed, 136 insertions(+)\n> >>\n> >> diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h\n> >> index 8aafd82e..a9b2d494 100644\n> >> --- a/include/libcamera/internal/camera_sensor.h\n> >> +++ b/include/libcamera/internal/camera_sensor.h\n> >> @@ -73,6 +73,8 @@ public:\n> >> virtual const std::vector &\n> >> testPatternModes() const = 0;\n> >> virtual int setTestPatternMode(controls::draft::TestPatternModeEnum mode) = 0;\n> >> + virtual void getSensorDelays(uint8_t &exposureDelay, uint8_t &gainDelay,\n> >> + uint8_t &vblankDelay, uint8_t &hblankDelay) = 0;\n> >\n> > Could/Should this return a const pointer to a struct SensorDelays now ?\n>\n> Yeah maybe...that would add a requirement for the struct to be available to CameraSensor (which is a \n> Factory class now) - I don't really have any strong feelings either way, so as long as that's fine \n> then I'll make the change.\n\nI think it would make the API nicer.\n\n> >> };\n> >> \n> >> class CameraSensorFactoryBase\n> >> diff --git a/include/libcamera/internal/camera_sensor_properties.h b/include/libcamera/internal/camera_sensor_properties.h\n> >> index 480ac121..56d5c15d 100644\n> >> --- a/include/libcamera/internal/camera_sensor_properties.h\n> >> +++ b/include/libcamera/internal/camera_sensor_properties.h\n> >> @@ -10,6 +10,8 @@\n> >> #include \n> >> #include \n> >> \n> >> +#include \n\nstdint.h should go just before string, we mix the C and C++ headers.\n\n> >> +\n> >> #include \n> >> #include \n> >> \n> >> @@ -20,6 +22,13 @@ struct CameraSensorProperties {\n> >> \n> >> Size unitCellSize;\n> >> std::map testPatternModes;\n> >> +\n> >> + struct {\n> >> + uint8_t exposureDelay;\n> >> + uint8_t gainDelay;\n> >> + uint8_t vblankDelay;\n> >> + uint8_t hblankDelay;\n> >> + } sensorDelays;\n> >> };\n> >> \n> >> } /* namespace libcamera */\n> >> diff --git a/src/libcamera/sensor/camera_sensor.cpp b/src/libcamera/sensor/camera_sensor.cpp\n> >> index 54cf98b2..61420873 100644\n> >> --- a/src/libcamera/sensor/camera_sensor.cpp\n> >> +++ b/src/libcamera/sensor/camera_sensor.cpp\n> >> @@ -336,6 +336,19 @@ CameraSensor::~CameraSensor() = default;\n> >> * pattern mode for every frame thus incurs no performance penalty.\n> >> */\n> >> \n> >> +/**\n> >> + * \\fn CameraSensor::getSensorDelays()\n\nWe don't usually prefix getters with \"get\".\n\n> >> + * \\brief Fetch the sensor delay values\n> >> + * \\param[out] exposureDelay The exposure delay\n> >> + * \\param[out] gainDelay The analogue gain delay\n> >> + * \\param[out] vblankDelay The vblank delay\n> >> + * \\param[out] hblankDelay The hblank delay\n> >> + *\n> >> + * This function fills in sensor control delays for pipeline handlers to use to\n> >> + * inform the DelayedControls. If no static properties are available it fills in\n> >> + * some widely applicable default values.\n\nWe need to return some default values until all sensors provide the\ninformation we need, but I wouldn't call those \"widely applicable\ndefault values\". It makes it sound those defaults will have a high\nchance of working, while that's not specifically the case.\n\n> >> + */\n> >> +\n> >> /**\n> >> * \\class CameraSensorFactoryBase\n> >> * \\brief Base class for camera sensor factories\n> >> diff --git a/src/libcamera/sensor/camera_sensor_legacy.cpp b/src/libcamera/sensor/camera_sensor_legacy.cpp\n> >> index a9b15c03..84972f02 100644\n> >> --- a/src/libcamera/sensor/camera_sensor_legacy.cpp\n> >> +++ b/src/libcamera/sensor/camera_sensor_legacy.cpp\n> >> @@ -95,6 +95,8 @@ public:\n> >> const std::vector &\n> >> testPatternModes() const override { return testPatternModes_; }\n> >> int setTestPatternMode(controls::draft::TestPatternModeEnum mode) override;\n> >> + void getSensorDelays(uint8_t &exposureDelay, uint8_t &gainDelay,\n> >> + uint8_t &vblankDelay, uint8_t &hblankDelay) override;\n> >> \n> >> protected:\n> >> std::string logPrefix() const override;\n> >> @@ -482,6 +484,37 @@ void CameraSensorLegacy::initStaticProperties()\n> >> initTestPatternModes();\n> >> }\n> >> \n> >> +void CameraSensorLegacy::getSensorDelays(uint8_t &exposureDelay, uint8_t &gainDelay,\n> >> + uint8_t &vblankDelay, uint8_t &hblankDelay)\n> >> +{\n> >> +\n> >> + /*\n> >> + * These defaults are applicable to many sensors, however more specific\n> >> + * values can be added to the CameraSensorProperties for a sensor if\n> >> + * required.\n> >> + */\n> >> + if (!staticProps_ ||\n> >> + (!staticProps_->sensorDelays.exposureDelay &&\n> >> + !staticProps_->sensorDelays.gainDelay &&\n> >> + !staticProps_->sensorDelays.vblankDelay &&\n> >> + !staticProps_->sensorDelays.hblankDelay)) {\n> >> + LOG(CameraSensor, Warning)\n> >> + << \"No sensor delays found in static properties. \"\n> >> + \"Assuming unverified defaults.\";\n\nHere the lack of sensor-specific values is reported with a more\nappropriate seriousness. Let's update the comments to match.\n\n> >> +\n> >> + exposureDelay = 2;\n> >> + gainDelay = 1;\n> >> + vblankDelay = 2;\n> >> + hblankDelay = 2;\n> >> + return;\n> >> + }\n> >> +\n> >> + exposureDelay = staticProps_->sensorDelays.exposureDelay;\n> >> + gainDelay = staticProps_->sensorDelays.gainDelay;\n> >> + vblankDelay = staticProps_->sensorDelays.vblankDelay;\n> >> + hblankDelay = staticProps_->sensorDelays.hblankDelay;\n> >\n> > Which would make this just\n> >\n> > \treturn staticProps_->sensorDelays;\n> >\n> >> +}\n> >\n> > I'm glad this function prints a warning!\n> >\n> >> +\n> >> void CameraSensorLegacy::initTestPatternModes()\n> >> {\n> >> const auto &v4l2TestPattern = controls().find(V4L2_CID_TEST_PATTERN);\n> >> diff --git a/src/libcamera/sensor/camera_sensor_properties.cpp b/src/libcamera/sensor/camera_sensor_properties.cpp\n> >> index 6d4136d0..6dda7ba9 100644\n> >> --- a/src/libcamera/sensor/camera_sensor_properties.cpp\n> >> +++ b/src/libcamera/sensor/camera_sensor_properties.cpp\n> >> @@ -41,6 +41,13 @@ LOG_DEFINE_CATEGORY(CameraSensorProperties)\n> >> * \\brief Map that associates the TestPattern control value with the indexes of\n> >> * the corresponding sensor test pattern modes as returned by\n> >> * V4L2_CID_TEST_PATTERN.\n> >> + *\n> >> + * \\var CameraSensorProperties::sensorDelays\n> >> + * \\brief Holds the delays, expressed in number of frames, between the time a\n> >> + * control is applied to the sensor and the time it actually takes effect.\n> >> + * Delays are recorded for the exposure time, analogue gain, vertical and\n> >> + * horizontal blanking. These values may be defined as empty, in which case the\n> >> + * CameraSensor derivative should provide default values.\n\nA \\brief should be brief :-) Make it a single short sentence, and you\ncan add more information in the body of the documentation.\n\n> >> */\n> >> \n> >> /**\n> >> @@ -60,6 +67,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n> >> { controls::draft::TestPatternModeColorBars, 2 },\n> >> { controls::draft::TestPatternModeColorBarsFadeToGray, 3 },\n> >> },\n> >> + .sensorDelays = { },\n\nThe AR0144 has a 2 frames delay for all parameters. Assuming that \"2\nframes delay\" means that parameters set during frame N will take effect\nfor frame N+2 (so a delay of 0 means the parameter is applied\nimmediately to the current frame). Maybe this should be documented more\nclearly above.\n\nNote that the sensor can also be programmed to apply a one frame delay\ninstead of two frames for the analog gain. The value is therefore\ndriver-dependent, and could even be changed dynamically. That's\nsomething to worry about later.\n\n> >> } },\n> >> { \"ar0521\", {\n> >> .unitCellSize = { 2200, 2200 },\n> >> @@ -69,6 +77,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n> >> { controls::draft::TestPatternModeColorBars, 2 },\n> >> { controls::draft::TestPatternModeColorBarsFadeToGray, 3 },\n> >> },\n> >> + .sensorDelays = { },\n\nThe AR0521 will be interesting. The exposure time has a 2 frames delay,\nwhile the analog gain will have a 1 frame delay if it is set alone, and\na 2 frames delay if the exposure time is also set during the same frame.\nThis is something we should consider when we'll develop a tool to\nmeasure delays.\n\nThe sensor can be configured to have a fixed 2 frames delay for the\nanalog gain, but that's not how the driver currently configures it.\n\nThe delay for the blanking values doesn't seem to be documented.\n\n> >> } },\n> >> { \"hi846\", {\n> >> .unitCellSize = { 1120, 1120 },\n> >> @@ -87,6 +96,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n> >> * 9: \"Resolution Pattern\"\n> >> */\n> >> },\n> >> + .sensorDelays = { },\n> >> } },\n> >> { \"imx214\", {\n> >> .unitCellSize = { 1120, 1120 },\n> >> @@ -97,6 +107,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n> >> { controls::draft::TestPatternModeColorBarsFadeToGray, 3 },\n> >> { controls::draft::TestPatternModePn9, 4 },\n> >> },\n> >> + .sensorDelays = { },\n> >> } },\n> >> { \"imx219\", {\n> >> .unitCellSize = { 1120, 1120 },\n> >> @@ -107,6 +118,12 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n> >> { controls::draft::TestPatternModeColorBarsFadeToGray, 3 },\n> >> { controls::draft::TestPatternModePn9, 4 },\n> >> },\n> >> + .sensorDelays = {\n> >> + .exposureDelay = 2,\n> >> + .gainDelay = 1,\n> >> + .vblankDelay = 2,\n> >> + .hblankDelay = 2\n> >> + },\n> >> } },\n> >> { \"imx258\", {\n> >> .unitCellSize = { 1120, 1120 },\n> >> @@ -117,34 +134,62 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n> >> { controls::draft::TestPatternModeColorBarsFadeToGray, 3 },\n> >> { controls::draft::TestPatternModePn9, 4 },\n> >> },\n> >> + .sensorDelays = { },\n> >> } },\n> >> { \"imx283\", {\n> >> .unitCellSize = { 2400, 2400 },\n> >> .testPatternModes = {},\n> >> + .sensorDelays = {\n> >> + .exposureDelay = 2,\n> >> + .gainDelay = 2,\n> >> + .vblankDelay = 2,\n> >> + .hblankDelay = 2\n> >> + },\n> >> } },\n> >> { \"imx290\", {\n> >> .unitCellSize = { 2900, 2900 },\n> >> .testPatternModes = {},\n> >> + .sensorDelays = {\n> >> + .exposureDelay = 2,\n> >> + .gainDelay = 2,\n> >> + .vblankDelay = 2,\n> >> + .hblankDelay = 2\n> >> + },\n> >> } },\n> >> { \"imx296\", {\n> >> .unitCellSize = { 3450, 3450 },\n> >> .testPatternModes = {},\n> >> + .sensorDelays = {\n> >> + .exposureDelay = 2,\n> >> + .gainDelay = 2,\n> >> + .vblankDelay = 2,\n> >> + .hblankDelay = 2\n> >> + },\n> >> } },\n> >> { \"imx327\", {\n> >> .unitCellSize = { 2900, 2900 },\n> >> .testPatternModes = {},\n> >> + .sensorDelays = { },\n> >> } },\n> >> { \"imx335\", {\n> >> .unitCellSize = { 2000, 2000 },\n> >> .testPatternModes = {},\n> >> + .sensorDelays = { },\n> >> } },\n> >> { \"imx415\", {\n> >> .unitCellSize = { 1450, 1450 },\n> >> .testPatternModes = {},\n> >> + .sensorDelays = { },\n> >> } },\n> >> { \"imx477\", {\n> >> .unitCellSize = { 1550, 1550 },\n> >> .testPatternModes = {},\n> >> + .sensorDelays = {\n> >> + .exposureDelay = 2,\n> >> + .gainDelay = 2,\n> >> + .vblankDelay = 3,\n> >> + .hblankDelay = 3\n> >> + },\n> >> } },\n> >> { \"imx519\", {\n> >> .unitCellSize = { 1220, 1220 },\n> >> @@ -157,6 +202,12 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n> >> * these two patterns do not comply with MIPI CCS v1.1 (Section 10.1).\n> >> */\n> >> },\n> >> + .sensorDelays = {\n> >> + .exposureDelay = 2,\n> >> + .gainDelay = 2,\n> >> + .vblankDelay = 3,\n> >> + .hblankDelay = 3\n> >> + },\n> >> } },\n> >> { \"imx708\", {\n> >> .unitCellSize = { 1400, 1400 },\n> >> @@ -167,6 +218,12 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n> >> { controls::draft::TestPatternModeColorBarsFadeToGray, 3 },\n> >> { controls::draft::TestPatternModePn9, 4 },\n> >> },\n> >> + .sensorDelays = {\n> >> + .exposureDelay = 2,\n> >> + .gainDelay = 2,\n> >> + .vblankDelay = 3,\n> >> + .hblankDelay = 3\n> >> + },\n> >> } },\n> >> { \"ov2685\", {\n> >> .unitCellSize = { 1750, 1750 },\n> >> @@ -181,6 +238,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n> >> * 5: \"Color Square\"\n> >> */\n> >> },\n> >> + .sensorDelays = { },\n> >> } },\n> >> { \"ov2740\", {\n> >> .unitCellSize = { 1400, 1400 },\n> >> @@ -188,6 +246,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n> >> { controls::draft::TestPatternModeOff, 0 },\n> >> { controls::draft::TestPatternModeColorBars, 1},\n> >> },\n> >> + .sensorDelays = { },\n> >> } },\n> >> { \"ov4689\", {\n> >> .unitCellSize = { 2000, 2000 },\n> >> @@ -201,6 +260,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n> >> * colorBarType2 and colorBarType3.\n> >> */\n> >> },\n> >> + .sensorDelays = { },\n> >> } },\n> >> { \"ov5640\", {\n> >> .unitCellSize = { 1400, 1400 },\n> >> @@ -208,10 +268,17 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n> >> { controls::draft::TestPatternModeOff, 0 },\n> >> { controls::draft::TestPatternModeColorBars, 1 },\n> >> },\n> >> + .sensorDelays = { },\n> >> } },\n> >> { \"ov5647\", {\n> >> .unitCellSize = { 1400, 1400 },\n> >> .testPatternModes = {},\n> >> + .sensorDelays = {\n> >> + .exposureDelay = 2,\n> >> + .gainDelay = 2,\n> >> + .vblankDelay = 2,\n> >> + .hblankDelay = 2\n> >> + },\n> >> } },\n> >> { \"ov5670\", {\n> >> .unitCellSize = { 1120, 1120 },\n> >> @@ -219,6 +286,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n> >> { controls::draft::TestPatternModeOff, 0 },\n> >> { controls::draft::TestPatternModeColorBars, 1 },\n> >> },\n> >> + .sensorDelays = { },\n> >> } },\n> >> { \"ov5675\", {\n> >> .unitCellSize = { 1120, 1120 },\n> >> @@ -226,6 +294,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n> >> { controls::draft::TestPatternModeOff, 0 },\n> >> { controls::draft::TestPatternModeColorBars, 1 },\n> >> },\n> >> + .sensorDelays = { },\n> >> } },\n> >> { \"ov5693\", {\n> >> .unitCellSize = { 1400, 1400 },\n> >> @@ -238,6 +307,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n> >> * Rolling Bar\".\n> >> */\n> >> },\n> >> + .sensorDelays = { },\n> >> } },\n> >> { \"ov64a40\", {\n> >> .unitCellSize = { 1008, 1008 },\n> >> @@ -251,6 +321,12 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n> >> * 4: \"Vertical Color Bar Type 4\"\n> >> */\n> >> },\n> >> + .sensorDelays = {\n> >> + .exposureDelay = 2,\n> >> + .gainDelay = 2,\n> >> + .vblankDelay = 2,\n> >> + .hblankDelay = 2\n> >> + },\n> >> } },\n> >> { \"ov8858\", {\n> >> .unitCellSize = { 1120, 1120 },\n> >> @@ -264,6 +340,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n> >> * 4: \"Vertical Color Bar Type 4\"\n> >> */\n> >> },\n> >> + .sensorDelays = { },\n> >> } },\n> >> { \"ov8865\", {\n> >> .unitCellSize = { 1400, 1400 },\n> >> @@ -278,6 +355,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n> >> * 5: \"Color squares with rolling bar\"\n> >> */\n> >> },\n> >> + .sensorDelays = { },\n> >> } },\n> >> { \"ov13858\", {\n> >> .unitCellSize = { 1120, 1120 },\n> >> @@ -285,6 +363,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n> >> { controls::draft::TestPatternModeOff, 0 },\n> >> { controls::draft::TestPatternModeColorBars, 1 },\n> >> },\n> >> + .sensorDelays = { },\n> >> } },\n> >> };\n> >>","headers":{"Return-Path":"","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 914B8C32E0\n\tfor ;\n\tMon, 18 Nov 2024 01:37:06 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 59466658B0;\n\tMon, 18 Nov 2024 02:37:05 +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 628F1600F2\n\tfor ;\n\tMon, 18 Nov 2024 02:37:03 +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 8E6FA5B3;\n\tMon, 18 Nov 2024 02:36:46 +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=\"UmlOLmI0\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1731893806;\n\tbh=XTpKPTU0WPkgqd1XH5X9dRZA43OqXuhJYTuxxAv92tE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=UmlOLmI0VfdOUHnT9Y+Y29h5YMzSHpwfAFljx3rMwg1+yXhHIQZXX1cdD4qMgUWuf\n\tj+O1/8viRWBt984q6CtUwMXK6CkQpWqNCLTrUcDUDvRaw71Yeqs8ou2AwPDPpqQODC\n\tXumGunmLRuSMGXhnYaMmYZhrgLY/3RryxCsFwKtg=","Date":"Mon, 18 Nov 2024 03:36:53 +0200","From":"Laurent Pinchart ","To":"Dan Scally ","Cc":"Kieran Bingham ,\n\tlibcamera-devel@lists.libcamera.org, mike.rudenko@gmail.com","Subject":"Re: [PATCH v3 5/6] libcamera: camera_sensor_properties: Add sensor\n\tcontrol delays","Message-ID":"<20241118013653.GI30787@pendragon.ideasonboard.com>","References":"<20241115074628.417215-1-dan.scally@ideasonboard.com>\n\t<20241115074628.417215-6-dan.scally@ideasonboard.com>\n\t<173167262754.4187655.11344145749616706346@ping.linuxembedded.co.uk>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":32209,"web_url":"https://patchwork.libcamera.org/comment/32209/","msgid":"<20241118014143.GL12409@pendragon.ideasonboard.com>","date":"2024-11-18T01:41:43","subject":"Re: [PATCH v3 5/6] libcamera: camera_sensor_properties: Add sensor\n\tcontrol delays","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Mon, Nov 18, 2024 at 03:36:53AM +0200, Laurent Pinchart wrote:\n> Hi Dan,\n> \n> On Fri, Nov 15, 2024 at 01:35:54PM +0000, Daniel Scally wrote:\n> > On 15/11/2024 12:10, Kieran Bingham wrote:\n> > > Quoting Daniel Scally (2024-11-15 07:46:27)\n> > >> Add properties covering the sensor control application delays to both\n> > >> the static CameraSensorProperties definitions. The values used are the\n> > >> defaults that're in use across the library, with deviations from that\n> \n> I had never seen \"that're\" before.\n> \n> > >> taken from Raspberry Pi's CamHelper class definitions.\n> > >>\n> > >> Signed-off-by: Daniel Scally \n> > >> ---\n> > >> Changes in v3:\n> > >>\n> > >> - Rebased on top of the CameraSensorFactory introduction\n> > >> - Some rephrasing\n> > >> - Defined the sensorDelays member as empty where Raspberry Pi didn't\n> > >> have any specific values. Check for the empty struct in\n> > >> getSensorDelays() and return the defaults from there with a warning.\n> > >\n> > > Sounds resonable to me.\n> > >\n> > >> Changes in v2:\n> > >>\n> > >> - Rather than adding the delays to the properties ControlList, added a\n> > >> new function in CameraSensor that allows PipelineHandlers to retreive\n> > >> the delay values.\n> > >>\n> > >> include/libcamera/internal/camera_sensor.h | 2 +\n> > >> .../internal/camera_sensor_properties.h | 9 +++\n> > >> src/libcamera/sensor/camera_sensor.cpp | 13 +++\n> > >> src/libcamera/sensor/camera_sensor_legacy.cpp | 33 ++++++++\n> > >> .../sensor/camera_sensor_properties.cpp | 79 +++++++++++++++++++\n> > >> 5 files changed, 136 insertions(+)\n> > >>\n> > >> diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h\n> > >> index 8aafd82e..a9b2d494 100644\n> > >> --- a/include/libcamera/internal/camera_sensor.h\n> > >> +++ b/include/libcamera/internal/camera_sensor.h\n> > >> @@ -73,6 +73,8 @@ public:\n> > >> virtual const std::vector &\n> > >> testPatternModes() const = 0;\n> > >> virtual int setTestPatternMode(controls::draft::TestPatternModeEnum mode) = 0;\n> > >> + virtual void getSensorDelays(uint8_t &exposureDelay, uint8_t &gainDelay,\n> > >> + uint8_t &vblankDelay, uint8_t &hblankDelay) = 0;\n> > >\n> > > Could/Should this return a const pointer to a struct SensorDelays now ?\n> >\n> > Yeah maybe...that would add a requirement for the struct to be available to CameraSensor (which is a \n> > Factory class now) - I don't really have any strong feelings either way, so as long as that's fine \n> > then I'll make the change.\n> \n> I think it would make the API nicer.\n> \n> > >> };\n> > >> \n> > >> class CameraSensorFactoryBase\n> > >> diff --git a/include/libcamera/internal/camera_sensor_properties.h b/include/libcamera/internal/camera_sensor_properties.h\n> > >> index 480ac121..56d5c15d 100644\n> > >> --- a/include/libcamera/internal/camera_sensor_properties.h\n> > >> +++ b/include/libcamera/internal/camera_sensor_properties.h\n> > >> @@ -10,6 +10,8 @@\n> > >> #include \n> > >> #include \n> > >> \n> > >> +#include \n> \n> stdint.h should go just before string, we mix the C and C++ headers.\n> \n> > >> +\n> > >> #include \n> > >> #include \n> > >> \n> > >> @@ -20,6 +22,13 @@ struct CameraSensorProperties {\n> > >> \n> > >> Size unitCellSize;\n> > >> std::map testPatternModes;\n> > >> +\n> > >> + struct {\n> > >> + uint8_t exposureDelay;\n> > >> + uint8_t gainDelay;\n> > >> + uint8_t vblankDelay;\n> > >> + uint8_t hblankDelay;\n> > >> + } sensorDelays;\n> > >> };\n> > >> \n> > >> } /* namespace libcamera */\n> > >> diff --git a/src/libcamera/sensor/camera_sensor.cpp b/src/libcamera/sensor/camera_sensor.cpp\n> > >> index 54cf98b2..61420873 100644\n> > >> --- a/src/libcamera/sensor/camera_sensor.cpp\n> > >> +++ b/src/libcamera/sensor/camera_sensor.cpp\n> > >> @@ -336,6 +336,19 @@ CameraSensor::~CameraSensor() = default;\n> > >> * pattern mode for every frame thus incurs no performance penalty.\n> > >> */\n> > >> \n> > >> +/**\n> > >> + * \\fn CameraSensor::getSensorDelays()\n> \n> We don't usually prefix getters with \"get\".\n> \n> > >> + * \\brief Fetch the sensor delay values\n> > >> + * \\param[out] exposureDelay The exposure delay\n> > >> + * \\param[out] gainDelay The analogue gain delay\n> > >> + * \\param[out] vblankDelay The vblank delay\n> > >> + * \\param[out] hblankDelay The hblank delay\n> > >> + *\n> > >> + * This function fills in sensor control delays for pipeline handlers to use to\n> > >> + * inform the DelayedControls. If no static properties are available it fills in\n> > >> + * some widely applicable default values.\n> \n> We need to return some default values until all sensors provide the\n> information we need, but I wouldn't call those \"widely applicable\n> default values\". It makes it sound those defaults will have a high\n> chance of working, while that's not specifically the case.\n> \n> > >> + */\n> > >> +\n> > >> /**\n> > >> * \\class CameraSensorFactoryBase\n> > >> * \\brief Base class for camera sensor factories\n> > >> diff --git a/src/libcamera/sensor/camera_sensor_legacy.cpp b/src/libcamera/sensor/camera_sensor_legacy.cpp\n> > >> index a9b15c03..84972f02 100644\n> > >> --- a/src/libcamera/sensor/camera_sensor_legacy.cpp\n> > >> +++ b/src/libcamera/sensor/camera_sensor_legacy.cpp\n> > >> @@ -95,6 +95,8 @@ public:\n> > >> const std::vector &\n> > >> testPatternModes() const override { return testPatternModes_; }\n> > >> int setTestPatternMode(controls::draft::TestPatternModeEnum mode) override;\n> > >> + void getSensorDelays(uint8_t &exposureDelay, uint8_t &gainDelay,\n> > >> + uint8_t &vblankDelay, uint8_t &hblankDelay) override;\n> > >> \n> > >> protected:\n> > >> std::string logPrefix() const override;\n> > >> @@ -482,6 +484,37 @@ void CameraSensorLegacy::initStaticProperties()\n> > >> initTestPatternModes();\n> > >> }\n> > >> \n> > >> +void CameraSensorLegacy::getSensorDelays(uint8_t &exposureDelay, uint8_t &gainDelay,\n> > >> + uint8_t &vblankDelay, uint8_t &hblankDelay)\n> > >> +{\n> > >> +\n> > >> + /*\n> > >> + * These defaults are applicable to many sensors, however more specific\n> > >> + * values can be added to the CameraSensorProperties for a sensor if\n> > >> + * required.\n> > >> + */\n> > >> + if (!staticProps_ ||\n> > >> + (!staticProps_->sensorDelays.exposureDelay &&\n> > >> + !staticProps_->sensorDelays.gainDelay &&\n> > >> + !staticProps_->sensorDelays.vblankDelay &&\n> > >> + !staticProps_->sensorDelays.hblankDelay)) {\n> > >> + LOG(CameraSensor, Warning)\n> > >> + << \"No sensor delays found in static properties. \"\n> > >> + \"Assuming unverified defaults.\";\n> \n> Here the lack of sensor-specific values is reported with a more\n> appropriate seriousness. Let's update the comments to match.\n> \n> > >> +\n> > >> + exposureDelay = 2;\n> > >> + gainDelay = 1;\n> > >> + vblankDelay = 2;\n> > >> + hblankDelay = 2;\n> > >> + return;\n> > >> + }\n> > >> +\n> > >> + exposureDelay = staticProps_->sensorDelays.exposureDelay;\n> > >> + gainDelay = staticProps_->sensorDelays.gainDelay;\n> > >> + vblankDelay = staticProps_->sensorDelays.vblankDelay;\n> > >> + hblankDelay = staticProps_->sensorDelays.hblankDelay;\n> > >\n> > > Which would make this just\n> > >\n> > > \treturn staticProps_->sensorDelays;\n> > >\n> > >> +}\n> > >\n> > > I'm glad this function prints a warning!\n> > >\n> > >> +\n> > >> void CameraSensorLegacy::initTestPatternModes()\n> > >> {\n> > >> const auto &v4l2TestPattern = controls().find(V4L2_CID_TEST_PATTERN);\n> > >> diff --git a/src/libcamera/sensor/camera_sensor_properties.cpp b/src/libcamera/sensor/camera_sensor_properties.cpp\n> > >> index 6d4136d0..6dda7ba9 100644\n> > >> --- a/src/libcamera/sensor/camera_sensor_properties.cpp\n> > >> +++ b/src/libcamera/sensor/camera_sensor_properties.cpp\n> > >> @@ -41,6 +41,13 @@ LOG_DEFINE_CATEGORY(CameraSensorProperties)\n> > >> * \\brief Map that associates the TestPattern control value with the indexes of\n> > >> * the corresponding sensor test pattern modes as returned by\n> > >> * V4L2_CID_TEST_PATTERN.\n> > >> + *\n> > >> + * \\var CameraSensorProperties::sensorDelays\n> > >> + * \\brief Holds the delays, expressed in number of frames, between the time a\n> > >> + * control is applied to the sensor and the time it actually takes effect.\n> > >> + * Delays are recorded for the exposure time, analogue gain, vertical and\n> > >> + * horizontal blanking. These values may be defined as empty, in which case the\n> > >> + * CameraSensor derivative should provide default values.\n> \n> A \\brief should be brief :-) Make it a single short sentence, and you\n> can add more information in the body of the documentation.\n> \n> > >> */\n> > >> \n> > >> /**\n> > >> @@ -60,6 +67,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n> > >> { controls::draft::TestPatternModeColorBars, 2 },\n> > >> { controls::draft::TestPatternModeColorBarsFadeToGray, 3 },\n> > >> },\n> > >> + .sensorDelays = { },\n> \n> The AR0144 has a 2 frames delay for all parameters. Assuming that \"2\n> frames delay\" means that parameters set during frame N will take effect\n> for frame N+2 (so a delay of 0 means the parameter is applied\n> immediately to the current frame). Maybe this should be documented more\n> clearly above.\n> \n> Note that the sensor can also be programmed to apply a one frame delay\n> instead of two frames for the analog gain. The value is therefore\n> driver-dependent, and could even be changed dynamically. That's\n> something to worry about later.\n> \n> > >> } },\n> > >> { \"ar0521\", {\n> > >> .unitCellSize = { 2200, 2200 },\n> > >> @@ -69,6 +77,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n> > >> { controls::draft::TestPatternModeColorBars, 2 },\n> > >> { controls::draft::TestPatternModeColorBarsFadeToGray, 3 },\n> > >> },\n> > >> + .sensorDelays = { },\n> \n> The AR0521 will be interesting. The exposure time has a 2 frames delay,\n> while the analog gain will have a 1 frame delay if it is set alone, and\n> a 2 frames delay if the exposure time is also set during the same frame.\n> This is something we should consider when we'll develop a tool to\n> measure delays.\n> \n> The sensor can be configured to have a fixed 2 frames delay for the\n> analog gain, but that's not how the driver currently configures it.\n> \n> The delay for the blanking values doesn't seem to be documented.\n> \n> > >> } },\n> > >> { \"hi846\", {\n> > >> .unitCellSize = { 1120, 1120 },\n> > >> @@ -87,6 +96,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n> > >> * 9: \"Resolution Pattern\"\n> > >> */\n> > >> },\n> > >> + .sensorDelays = { },\n> > >> } },\n> > >> { \"imx214\", {\n> > >> .unitCellSize = { 1120, 1120 },\n> > >> @@ -97,6 +107,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n> > >> { controls::draft::TestPatternModeColorBarsFadeToGray, 3 },\n> > >> { controls::draft::TestPatternModePn9, 4 },\n> > >> },\n> > >> + .sensorDelays = { },\n> > >> } },\n> > >> { \"imx219\", {\n> > >> .unitCellSize = { 1120, 1120 },\n> > >> @@ -107,6 +118,12 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n> > >> { controls::draft::TestPatternModeColorBarsFadeToGray, 3 },\n> > >> { controls::draft::TestPatternModePn9, 4 },\n> > >> },\n> > >> + .sensorDelays = {\n> > >> + .exposureDelay = 2,\n> > >> + .gainDelay = 1,\n> > >> + .vblankDelay = 2,\n> > >> + .hblankDelay = 2\n> > >> + },\n> > >> } },\n> > >> { \"imx258\", {\n> > >> .unitCellSize = { 1120, 1120 },\n> > >> @@ -117,34 +134,62 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n> > >> { controls::draft::TestPatternModeColorBarsFadeToGray, 3 },\n> > >> { controls::draft::TestPatternModePn9, 4 },\n> > >> },\n> > >> + .sensorDelays = { },\n> > >> } },\n> > >> { \"imx283\", {\n> > >> .unitCellSize = { 2400, 2400 },\n> > >> .testPatternModes = {},\n> > >> + .sensorDelays = {\n> > >> + .exposureDelay = 2,\n> > >> + .gainDelay = 2,\n> > >> + .vblankDelay = 2,\n> > >> + .hblankDelay = 2\n> > >> + },\n> > >> } },\n> > >> { \"imx290\", {\n> > >> .unitCellSize = { 2900, 2900 },\n> > >> .testPatternModes = {},\n> > >> + .sensorDelays = {\n> > >> + .exposureDelay = 2,\n> > >> + .gainDelay = 2,\n> > >> + .vblankDelay = 2,\n> > >> + .hblankDelay = 2\n> > >> + },\n> > >> } },\n> > >> { \"imx296\", {\n> > >> .unitCellSize = { 3450, 3450 },\n> > >> .testPatternModes = {},\n> > >> + .sensorDelays = {\n> > >> + .exposureDelay = 2,\n> > >> + .gainDelay = 2,\n> > >> + .vblankDelay = 2,\n> > >> + .hblankDelay = 2\n> > >> + },\n> > >> } },\n> > >> { \"imx327\", {\n> > >> .unitCellSize = { 2900, 2900 },\n> > >> .testPatternModes = {},\n> > >> + .sensorDelays = { },\n> > >> } },\n> > >> { \"imx335\", {\n> > >> .unitCellSize = { 2000, 2000 },\n> > >> .testPatternModes = {},\n> > >> + .sensorDelays = { },\n> > >> } },\n> > >> { \"imx415\", {\n> > >> .unitCellSize = { 1450, 1450 },\n> > >> .testPatternModes = {},\n> > >> + .sensorDelays = { },\n> > >> } },\n> > >> { \"imx477\", {\n> > >> .unitCellSize = { 1550, 1550 },\n> > >> .testPatternModes = {},\n> > >> + .sensorDelays = {\n> > >> + .exposureDelay = 2,\n> > >> + .gainDelay = 2,\n> > >> + .vblankDelay = 3,\n> > >> + .hblankDelay = 3\n\nI forgot to mention that this will be \"interesting\" to handle. The\nexposure time is bound by the frame time (with a sensor-specific\nmargin), different delays for the exposure time and the blanking values\nwill probably require careful handling.\n\n> > >> + },\n> > >> } },\n> > >> { \"imx519\", {\n> > >> .unitCellSize = { 1220, 1220 },\n> > >> @@ -157,6 +202,12 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n> > >> * these two patterns do not comply with MIPI CCS v1.1 (Section 10.1).\n> > >> */\n> > >> },\n> > >> + .sensorDelays = {\n> > >> + .exposureDelay = 2,\n> > >> + .gainDelay = 2,\n> > >> + .vblankDelay = 3,\n> > >> + .hblankDelay = 3\n> > >> + },\n> > >> } },\n> > >> { \"imx708\", {\n> > >> .unitCellSize = { 1400, 1400 },\n> > >> @@ -167,6 +218,12 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n> > >> { controls::draft::TestPatternModeColorBarsFadeToGray, 3 },\n> > >> { controls::draft::TestPatternModePn9, 4 },\n> > >> },\n> > >> + .sensorDelays = {\n> > >> + .exposureDelay = 2,\n> > >> + .gainDelay = 2,\n> > >> + .vblankDelay = 3,\n> > >> + .hblankDelay = 3\n> > >> + },\n> > >> } },\n> > >> { \"ov2685\", {\n> > >> .unitCellSize = { 1750, 1750 },\n> > >> @@ -181,6 +238,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n> > >> * 5: \"Color Square\"\n> > >> */\n> > >> },\n> > >> + .sensorDelays = { },\n> > >> } },\n> > >> { \"ov2740\", {\n> > >> .unitCellSize = { 1400, 1400 },\n> > >> @@ -188,6 +246,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n> > >> { controls::draft::TestPatternModeOff, 0 },\n> > >> { controls::draft::TestPatternModeColorBars, 1},\n> > >> },\n> > >> + .sensorDelays = { },\n> > >> } },\n> > >> { \"ov4689\", {\n> > >> .unitCellSize = { 2000, 2000 },\n> > >> @@ -201,6 +260,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n> > >> * colorBarType2 and colorBarType3.\n> > >> */\n> > >> },\n> > >> + .sensorDelays = { },\n> > >> } },\n> > >> { \"ov5640\", {\n> > >> .unitCellSize = { 1400, 1400 },\n> > >> @@ -208,10 +268,17 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n> > >> { controls::draft::TestPatternModeOff, 0 },\n> > >> { controls::draft::TestPatternModeColorBars, 1 },\n> > >> },\n> > >> + .sensorDelays = { },\n> > >> } },\n> > >> { \"ov5647\", {\n> > >> .unitCellSize = { 1400, 1400 },\n> > >> .testPatternModes = {},\n> > >> + .sensorDelays = {\n> > >> + .exposureDelay = 2,\n> > >> + .gainDelay = 2,\n> > >> + .vblankDelay = 2,\n> > >> + .hblankDelay = 2\n> > >> + },\n> > >> } },\n> > >> { \"ov5670\", {\n> > >> .unitCellSize = { 1120, 1120 },\n> > >> @@ -219,6 +286,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n> > >> { controls::draft::TestPatternModeOff, 0 },\n> > >> { controls::draft::TestPatternModeColorBars, 1 },\n> > >> },\n> > >> + .sensorDelays = { },\n> > >> } },\n> > >> { \"ov5675\", {\n> > >> .unitCellSize = { 1120, 1120 },\n> > >> @@ -226,6 +294,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n> > >> { controls::draft::TestPatternModeOff, 0 },\n> > >> { controls::draft::TestPatternModeColorBars, 1 },\n> > >> },\n> > >> + .sensorDelays = { },\n> > >> } },\n> > >> { \"ov5693\", {\n> > >> .unitCellSize = { 1400, 1400 },\n> > >> @@ -238,6 +307,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n> > >> * Rolling Bar\".\n> > >> */\n> > >> },\n> > >> + .sensorDelays = { },\n> > >> } },\n> > >> { \"ov64a40\", {\n> > >> .unitCellSize = { 1008, 1008 },\n> > >> @@ -251,6 +321,12 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n> > >> * 4: \"Vertical Color Bar Type 4\"\n> > >> */\n> > >> },\n> > >> + .sensorDelays = {\n> > >> + .exposureDelay = 2,\n> > >> + .gainDelay = 2,\n> > >> + .vblankDelay = 2,\n> > >> + .hblankDelay = 2\n> > >> + },\n> > >> } },\n> > >> { \"ov8858\", {\n> > >> .unitCellSize = { 1120, 1120 },\n> > >> @@ -264,6 +340,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n> > >> * 4: \"Vertical Color Bar Type 4\"\n> > >> */\n> > >> },\n> > >> + .sensorDelays = { },\n> > >> } },\n> > >> { \"ov8865\", {\n> > >> .unitCellSize = { 1400, 1400 },\n> > >> @@ -278,6 +355,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n> > >> * 5: \"Color squares with rolling bar\"\n> > >> */\n> > >> },\n> > >> + .sensorDelays = { },\n> > >> } },\n> > >> { \"ov13858\", {\n> > >> .unitCellSize = { 1120, 1120 },\n> > >> @@ -285,6 +363,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n> > >> { controls::draft::TestPatternModeOff, 0 },\n> > >> { controls::draft::TestPatternModeColorBars, 1 },\n> > >> },\n> > >> + .sensorDelays = { },\n> > >> } },\n> > >> };\n> > >>","headers":{"Return-Path":"","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 2561CC32DD\n\tfor ;\n\tMon, 18 Nov 2024 01:41:56 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id EF95B658C1;\n\tMon, 18 Nov 2024 02:41:54 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id CB973600F2\n\tfor ;\n\tMon, 18 Nov 2024 02:41:52 +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 090565B3;\n\tMon, 18 Nov 2024 02:41:35 +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=\"cK39h9H9\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1731894096;\n\tbh=Wkc4XtRBy4cDEHYkn/s0jPdXTQ2C/egtrHt+h4gHouQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=cK39h9H9V5tr4c316r4bTCGSRhv/eJyQxOHxw7KvIG5sdrYRfQVGph3dWomQo5iJH\n\t5bhEcDeEHenGAATxMx3Nb00A1bskJhCwaCm2bL/kOM2Pw4fmlRJB3dIJ0XTUrfX1OS\n\tXJR8moYdwFyfs174tDKBI9/cqNz31Xd46SzzsaVg=","Date":"Mon, 18 Nov 2024 03:41:43 +0200","From":"Laurent Pinchart ","To":"Dan Scally ","Cc":"Kieran Bingham ,\n\tlibcamera-devel@lists.libcamera.org, mike.rudenko@gmail.com","Subject":"Re: [PATCH v3 5/6] libcamera: camera_sensor_properties: Add sensor\n\tcontrol delays","Message-ID":"<20241118014143.GL12409@pendragon.ideasonboard.com>","References":"<20241115074628.417215-1-dan.scally@ideasonboard.com>\n\t<20241115074628.417215-6-dan.scally@ideasonboard.com>\n\t<173167262754.4187655.11344145749616706346@ping.linuxembedded.co.uk>\n\t\n\t<20241118013653.GI30787@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20241118013653.GI30787@pendragon.ideasonboard.com>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":32260,"web_url":"https://patchwork.libcamera.org/comment/32260/","msgid":"","date":"2024-11-19T09:46:17","subject":"Re: [PATCH v3 5/6] libcamera: camera_sensor_properties: Add sensor\n\tcontrol delays","submitter":{"id":156,"url":"https://patchwork.libcamera.org/api/people/156/","name":"Dan Scally","email":"dan.scally@ideasonboard.com"},"content":"Hi Laurent\n\nOn 18/11/2024 01:36, Laurent Pinchart wrote:\n> Hi Dan,\n>\n> On Fri, Nov 15, 2024 at 01:35:54PM +0000, Daniel Scally wrote:\n>> On 15/11/2024 12:10, Kieran Bingham wrote:\n>>> Quoting Daniel Scally (2024-11-15 07:46:27)\n>>>> Add properties covering the sensor control application delays to both\n>>>> the static CameraSensorProperties definitions. The values used are the\n>>>> defaults that're in use across the library, with deviations from that\n> I had never seen \"that're\" before.\nEnglish is ever surprising (I'm not even sure if it's proper usage tbh...)\n>\n>>>> taken from Raspberry Pi's CamHelper class definitions.\n>>>>\n>>>> Signed-off-by: Daniel Scally \n>>>> ---\n>>>> Changes in v3:\n>>>>\n>>>> - Rebased on top of the CameraSensorFactory introduction\n>>>> - Some rephrasing\n>>>> - Defined the sensorDelays member as empty where Raspberry Pi didn't\n>>>> have any specific values. Check for the empty struct in\n>>>> getSensorDelays() and return the defaults from there with a warning.\n>>> Sounds resonable to me.\n>>>\n>>>> Changes in v2:\n>>>>\n>>>> - Rather than adding the delays to the properties ControlList, added a\n>>>> new function in CameraSensor that allows PipelineHandlers to retreive\n>>>> the delay values.\n>>>>\n>>>> include/libcamera/internal/camera_sensor.h | 2 +\n>>>> .../internal/camera_sensor_properties.h | 9 +++\n>>>> src/libcamera/sensor/camera_sensor.cpp | 13 +++\n>>>> src/libcamera/sensor/camera_sensor_legacy.cpp | 33 ++++++++\n>>>> .../sensor/camera_sensor_properties.cpp | 79 +++++++++++++++++++\n>>>> 5 files changed, 136 insertions(+)\n>>>>\n>>>> diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h\n>>>> index 8aafd82e..a9b2d494 100644\n>>>> --- a/include/libcamera/internal/camera_sensor.h\n>>>> +++ b/include/libcamera/internal/camera_sensor.h\n>>>> @@ -73,6 +73,8 @@ public:\n>>>> virtual const std::vector &\n>>>> testPatternModes() const = 0;\n>>>> virtual int setTestPatternMode(controls::draft::TestPatternModeEnum mode) = 0;\n>>>> + virtual void getSensorDelays(uint8_t &exposureDelay, uint8_t &gainDelay,\n>>>> + uint8_t &vblankDelay, uint8_t &hblankDelay) = 0;\n>>> Could/Should this return a const pointer to a struct SensorDelays now ?\n>> Yeah maybe...that would add a requirement for the struct to be available to CameraSensor (which is a\n>> Factory class now) - I don't really have any strong feelings either way, so as long as that's fine\n>> then I'll make the change.\n> I think it would make the API nicer.\n>\n>>>> };\n>>>> \n>>>> class CameraSensorFactoryBase\n>>>> diff --git a/include/libcamera/internal/camera_sensor_properties.h b/include/libcamera/internal/camera_sensor_properties.h\n>>>> index 480ac121..56d5c15d 100644\n>>>> --- a/include/libcamera/internal/camera_sensor_properties.h\n>>>> +++ b/include/libcamera/internal/camera_sensor_properties.h\n>>>> @@ -10,6 +10,8 @@\n>>>> #include \n>>>> #include \n>>>> \n>>>> +#include \n> stdint.h should go just before string, we mix the C and C++ headers.\n>\n>>>> +\n>>>> #include \n>>>> #include \n>>>> \n>>>> @@ -20,6 +22,13 @@ struct CameraSensorProperties {\n>>>> \n>>>> Size unitCellSize;\n>>>> std::map testPatternModes;\n>>>> +\n>>>> + struct {\n>>>> + uint8_t exposureDelay;\n>>>> + uint8_t gainDelay;\n>>>> + uint8_t vblankDelay;\n>>>> + uint8_t hblankDelay;\n>>>> + } sensorDelays;\n>>>> };\n>>>> \n>>>> } /* namespace libcamera */\n>>>> diff --git a/src/libcamera/sensor/camera_sensor.cpp b/src/libcamera/sensor/camera_sensor.cpp\n>>>> index 54cf98b2..61420873 100644\n>>>> --- a/src/libcamera/sensor/camera_sensor.cpp\n>>>> +++ b/src/libcamera/sensor/camera_sensor.cpp\n>>>> @@ -336,6 +336,19 @@ CameraSensor::~CameraSensor() = default;\n>>>> * pattern mode for every frame thus incurs no performance penalty.\n>>>> */\n>>>> \n>>>> +/**\n>>>> + * \\fn CameraSensor::getSensorDelays()\n> We don't usually prefix getters with \"get\".\n>\n>>>> + * \\brief Fetch the sensor delay values\n>>>> + * \\param[out] exposureDelay The exposure delay\n>>>> + * \\param[out] gainDelay The analogue gain delay\n>>>> + * \\param[out] vblankDelay The vblank delay\n>>>> + * \\param[out] hblankDelay The hblank delay\n>>>> + *\n>>>> + * This function fills in sensor control delays for pipeline handlers to use to\n>>>> + * inform the DelayedControls. If no static properties are available it fills in\n>>>> + * some widely applicable default values.\n> We need to return some default values until all sensors provide the\n> information we need, but I wouldn't call those \"widely applicable\n> default values\". It makes it sound those defaults will have a high\n> chance of working, while that's not specifically the case.\nAck\n>\n>>>> + */\n>>>> +\n>>>> /**\n>>>> * \\class CameraSensorFactoryBase\n>>>> * \\brief Base class for camera sensor factories\n>>>> diff --git a/src/libcamera/sensor/camera_sensor_legacy.cpp b/src/libcamera/sensor/camera_sensor_legacy.cpp\n>>>> index a9b15c03..84972f02 100644\n>>>> --- a/src/libcamera/sensor/camera_sensor_legacy.cpp\n>>>> +++ b/src/libcamera/sensor/camera_sensor_legacy.cpp\n>>>> @@ -95,6 +95,8 @@ public:\n>>>> const std::vector &\n>>>> testPatternModes() const override { return testPatternModes_; }\n>>>> int setTestPatternMode(controls::draft::TestPatternModeEnum mode) override;\n>>>> + void getSensorDelays(uint8_t &exposureDelay, uint8_t &gainDelay,\n>>>> + uint8_t &vblankDelay, uint8_t &hblankDelay) override;\n>>>> \n>>>> protected:\n>>>> std::string logPrefix() const override;\n>>>> @@ -482,6 +484,37 @@ void CameraSensorLegacy::initStaticProperties()\n>>>> initTestPatternModes();\n>>>> }\n>>>> \n>>>> +void CameraSensorLegacy::getSensorDelays(uint8_t &exposureDelay, uint8_t &gainDelay,\n>>>> + uint8_t &vblankDelay, uint8_t &hblankDelay)\n>>>> +{\n>>>> +\n>>>> + /*\n>>>> + * These defaults are applicable to many sensors, however more specific\n>>>> + * values can be added to the CameraSensorProperties for a sensor if\n>>>> + * required.\n>>>> + */\n>>>> + if (!staticProps_ ||\n>>>> + (!staticProps_->sensorDelays.exposureDelay &&\n>>>> + !staticProps_->sensorDelays.gainDelay &&\n>>>> + !staticProps_->sensorDelays.vblankDelay &&\n>>>> + !staticProps_->sensorDelays.hblankDelay)) {\n>>>> + LOG(CameraSensor, Warning)\n>>>> + << \"No sensor delays found in static properties. \"\n>>>> + \"Assuming unverified defaults.\";\n> Here the lack of sensor-specific values is reported with a more\n> appropriate seriousness. Let's update the comments to match.\n>\n>>>> +\n>>>> + exposureDelay = 2;\n>>>> + gainDelay = 1;\n>>>> + vblankDelay = 2;\n>>>> + hblankDelay = 2;\n>>>> + return;\n>>>> + }\n>>>> +\n>>>> + exposureDelay = staticProps_->sensorDelays.exposureDelay;\n>>>> + gainDelay = staticProps_->sensorDelays.gainDelay;\n>>>> + vblankDelay = staticProps_->sensorDelays.vblankDelay;\n>>>> + hblankDelay = staticProps_->sensorDelays.hblankDelay;\n>>> Which would make this just\n>>>\n>>> \treturn staticProps_->sensorDelays;\n>>>\n>>>> +}\n>>> I'm glad this function prints a warning!\n>>>\n>>>> +\n>>>> void CameraSensorLegacy::initTestPatternModes()\n>>>> {\n>>>> const auto &v4l2TestPattern = controls().find(V4L2_CID_TEST_PATTERN);\n>>>> diff --git a/src/libcamera/sensor/camera_sensor_properties.cpp b/src/libcamera/sensor/camera_sensor_properties.cpp\n>>>> index 6d4136d0..6dda7ba9 100644\n>>>> --- a/src/libcamera/sensor/camera_sensor_properties.cpp\n>>>> +++ b/src/libcamera/sensor/camera_sensor_properties.cpp\n>>>> @@ -41,6 +41,13 @@ LOG_DEFINE_CATEGORY(CameraSensorProperties)\n>>>> * \\brief Map that associates the TestPattern control value with the indexes of\n>>>> * the corresponding sensor test pattern modes as returned by\n>>>> * V4L2_CID_TEST_PATTERN.\n>>>> + *\n>>>> + * \\var CameraSensorProperties::sensorDelays\n>>>> + * \\brief Holds the delays, expressed in number of frames, between the time a\n>>>> + * control is applied to the sensor and the time it actually takes effect.\n>>>> + * Delays are recorded for the exposure time, analogue gain, vertical and\n>>>> + * horizontal blanking. These values may be defined as empty, in which case the\n>>>> + * CameraSensor derivative should provide default values.\n> A \\brief should be brief :-) Make it a single short sentence, and you\n> can add more information in the body of the documentation.\n>\n>>>> */\n>>>> \n>>>> /**\n>>>> @@ -60,6 +67,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n>>>> { controls::draft::TestPatternModeColorBars, 2 },\n>>>> { controls::draft::TestPatternModeColorBarsFadeToGray, 3 },\n>>>> },\n>>>> + .sensorDelays = { },\n> The AR0144 has a 2 frames delay for all parameters. Assuming that \"2\n> frames delay\" means that parameters set during frame N will take effect\n> for frame N+2 (so a delay of 0 means the parameter is applied\n> immediately to the current frame). Maybe this should be documented more\n> clearly above.\n>\n> Note that the sensor can also be programmed to apply a one frame delay\n> instead of two frames for the analog gain. The value is therefore\n> driver-dependent, and could even be changed dynamically. That's\n> something to worry about later.\nOof, in that case we probably need sensor controls in the drivers to report those values\n>\n>>>> } },\n>>>> { \"ar0521\", {\n>>>> .unitCellSize = { 2200, 2200 },\n>>>> @@ -69,6 +77,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n>>>> { controls::draft::TestPatternModeColorBars, 2 },\n>>>> { controls::draft::TestPatternModeColorBarsFadeToGray, 3 },\n>>>> },\n>>>> + .sensorDelays = { },\n> The AR0521 will be interesting. The exposure time has a 2 frames delay,\n> while the analog gain will have a 1 frame delay if it is set alone, and\n> a 2 frames delay if the exposure time is also set during the same frame.\n> This is something we should consider when we'll develop a tool to\n> measure delays.\n>\n> The sensor can be configured to have a fixed 2 frames delay for the\n> analog gain, but that's not how the driver currently configures it.\n>\n> The delay for the blanking values doesn't seem to be documented.\n>\n>>>> } },\n>>>> { \"hi846\", {\n>>>> .unitCellSize = { 1120, 1120 },\n>>>> @@ -87,6 +96,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n>>>> * 9: \"Resolution Pattern\"\n>>>> */\n>>>> },\n>>>> + .sensorDelays = { },\n>>>> } },\n>>>> { \"imx214\", {\n>>>> .unitCellSize = { 1120, 1120 },\n>>>> @@ -97,6 +107,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n>>>> { controls::draft::TestPatternModeColorBarsFadeToGray, 3 },\n>>>> { controls::draft::TestPatternModePn9, 4 },\n>>>> },\n>>>> + .sensorDelays = { },\n>>>> } },\n>>>> { \"imx219\", {\n>>>> .unitCellSize = { 1120, 1120 },\n>>>> @@ -107,6 +118,12 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n>>>> { controls::draft::TestPatternModeColorBarsFadeToGray, 3 },\n>>>> { controls::draft::TestPatternModePn9, 4 },\n>>>> },\n>>>> + .sensorDelays = {\n>>>> + .exposureDelay = 2,\n>>>> + .gainDelay = 1,\n>>>> + .vblankDelay = 2,\n>>>> + .hblankDelay = 2\n>>>> + },\n>>>> } },\n>>>> { \"imx258\", {\n>>>> .unitCellSize = { 1120, 1120 },\n>>>> @@ -117,34 +134,62 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n>>>> { controls::draft::TestPatternModeColorBarsFadeToGray, 3 },\n>>>> { controls::draft::TestPatternModePn9, 4 },\n>>>> },\n>>>> + .sensorDelays = { },\n>>>> } },\n>>>> { \"imx283\", {\n>>>> .unitCellSize = { 2400, 2400 },\n>>>> .testPatternModes = {},\n>>>> + .sensorDelays = {\n>>>> + .exposureDelay = 2,\n>>>> + .gainDelay = 2,\n>>>> + .vblankDelay = 2,\n>>>> + .hblankDelay = 2\n>>>> + },\n>>>> } },\n>>>> { \"imx290\", {\n>>>> .unitCellSize = { 2900, 2900 },\n>>>> .testPatternModes = {},\n>>>> + .sensorDelays = {\n>>>> + .exposureDelay = 2,\n>>>> + .gainDelay = 2,\n>>>> + .vblankDelay = 2,\n>>>> + .hblankDelay = 2\n>>>> + },\n>>>> } },\n>>>> { \"imx296\", {\n>>>> .unitCellSize = { 3450, 3450 },\n>>>> .testPatternModes = {},\n>>>> + .sensorDelays = {\n>>>> + .exposureDelay = 2,\n>>>> + .gainDelay = 2,\n>>>> + .vblankDelay = 2,\n>>>> + .hblankDelay = 2\n>>>> + },\n>>>> } },\n>>>> { \"imx327\", {\n>>>> .unitCellSize = { 2900, 2900 },\n>>>> .testPatternModes = {},\n>>>> + .sensorDelays = { },\n>>>> } },\n>>>> { \"imx335\", {\n>>>> .unitCellSize = { 2000, 2000 },\n>>>> .testPatternModes = {},\n>>>> + .sensorDelays = { },\n>>>> } },\n>>>> { \"imx415\", {\n>>>> .unitCellSize = { 1450, 1450 },\n>>>> .testPatternModes = {},\n>>>> + .sensorDelays = { },\n>>>> } },\n>>>> { \"imx477\", {\n>>>> .unitCellSize = { 1550, 1550 },\n>>>> .testPatternModes = {},\n>>>> + .sensorDelays = {\n>>>> + .exposureDelay = 2,\n>>>> + .gainDelay = 2,\n>>>> + .vblankDelay = 3,\n>>>> + .hblankDelay = 3\n>>>> + },\n>>>> } },\n>>>> { \"imx519\", {\n>>>> .unitCellSize = { 1220, 1220 },\n>>>> @@ -157,6 +202,12 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n>>>> * these two patterns do not comply with MIPI CCS v1.1 (Section 10.1).\n>>>> */\n>>>> },\n>>>> + .sensorDelays = {\n>>>> + .exposureDelay = 2,\n>>>> + .gainDelay = 2,\n>>>> + .vblankDelay = 3,\n>>>> + .hblankDelay = 3\n>>>> + },\n>>>> } },\n>>>> { \"imx708\", {\n>>>> .unitCellSize = { 1400, 1400 },\n>>>> @@ -167,6 +218,12 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n>>>> { controls::draft::TestPatternModeColorBarsFadeToGray, 3 },\n>>>> { controls::draft::TestPatternModePn9, 4 },\n>>>> },\n>>>> + .sensorDelays = {\n>>>> + .exposureDelay = 2,\n>>>> + .gainDelay = 2,\n>>>> + .vblankDelay = 3,\n>>>> + .hblankDelay = 3\n>>>> + },\n>>>> } },\n>>>> { \"ov2685\", {\n>>>> .unitCellSize = { 1750, 1750 },\n>>>> @@ -181,6 +238,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n>>>> * 5: \"Color Square\"\n>>>> */\n>>>> },\n>>>> + .sensorDelays = { },\n>>>> } },\n>>>> { \"ov2740\", {\n>>>> .unitCellSize = { 1400, 1400 },\n>>>> @@ -188,6 +246,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n>>>> { controls::draft::TestPatternModeOff, 0 },\n>>>> { controls::draft::TestPatternModeColorBars, 1},\n>>>> },\n>>>> + .sensorDelays = { },\n>>>> } },\n>>>> { \"ov4689\", {\n>>>> .unitCellSize = { 2000, 2000 },\n>>>> @@ -201,6 +260,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n>>>> * colorBarType2 and colorBarType3.\n>>>> */\n>>>> },\n>>>> + .sensorDelays = { },\n>>>> } },\n>>>> { \"ov5640\", {\n>>>> .unitCellSize = { 1400, 1400 },\n>>>> @@ -208,10 +268,17 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n>>>> { controls::draft::TestPatternModeOff, 0 },\n>>>> { controls::draft::TestPatternModeColorBars, 1 },\n>>>> },\n>>>> + .sensorDelays = { },\n>>>> } },\n>>>> { \"ov5647\", {\n>>>> .unitCellSize = { 1400, 1400 },\n>>>> .testPatternModes = {},\n>>>> + .sensorDelays = {\n>>>> + .exposureDelay = 2,\n>>>> + .gainDelay = 2,\n>>>> + .vblankDelay = 2,\n>>>> + .hblankDelay = 2\n>>>> + },\n>>>> } },\n>>>> { \"ov5670\", {\n>>>> .unitCellSize = { 1120, 1120 },\n>>>> @@ -219,6 +286,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n>>>> { controls::draft::TestPatternModeOff, 0 },\n>>>> { controls::draft::TestPatternModeColorBars, 1 },\n>>>> },\n>>>> + .sensorDelays = { },\n>>>> } },\n>>>> { \"ov5675\", {\n>>>> .unitCellSize = { 1120, 1120 },\n>>>> @@ -226,6 +294,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n>>>> { controls::draft::TestPatternModeOff, 0 },\n>>>> { controls::draft::TestPatternModeColorBars, 1 },\n>>>> },\n>>>> + .sensorDelays = { },\n>>>> } },\n>>>> { \"ov5693\", {\n>>>> .unitCellSize = { 1400, 1400 },\n>>>> @@ -238,6 +307,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n>>>> * Rolling Bar\".\n>>>> */\n>>>> },\n>>>> + .sensorDelays = { },\n>>>> } },\n>>>> { \"ov64a40\", {\n>>>> .unitCellSize = { 1008, 1008 },\n>>>> @@ -251,6 +321,12 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n>>>> * 4: \"Vertical Color Bar Type 4\"\n>>>> */\n>>>> },\n>>>> + .sensorDelays = {\n>>>> + .exposureDelay = 2,\n>>>> + .gainDelay = 2,\n>>>> + .vblankDelay = 2,\n>>>> + .hblankDelay = 2\n>>>> + },\n>>>> } },\n>>>> { \"ov8858\", {\n>>>> .unitCellSize = { 1120, 1120 },\n>>>> @@ -264,6 +340,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n>>>> * 4: \"Vertical Color Bar Type 4\"\n>>>> */\n>>>> },\n>>>> + .sensorDelays = { },\n>>>> } },\n>>>> { \"ov8865\", {\n>>>> .unitCellSize = { 1400, 1400 },\n>>>> @@ -278,6 +355,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n>>>> * 5: \"Color squares with rolling bar\"\n>>>> */\n>>>> },\n>>>> + .sensorDelays = { },\n>>>> } },\n>>>> { \"ov13858\", {\n>>>> .unitCellSize = { 1120, 1120 },\n>>>> @@ -285,6 +363,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n>>>> { controls::draft::TestPatternModeOff, 0 },\n>>>> { controls::draft::TestPatternModeColorBars, 1 },\n>>>> },\n>>>> + .sensorDelays = { },\n>>>> } },\n>>>> };\n>>>>","headers":{"Return-Path":"","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 27427C326B\n\tfor ;\n\tTue, 19 Nov 2024 09:46:25 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 312AF65EE8;\n\tTue, 19 Nov 2024 10:46:24 +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 2B6366589D\n\tfor ;\n\tTue, 19 Nov 2024 10:46:22 +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 05E983A2;\n\tTue, 19 Nov 2024 10:46:03 +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=\"myUxn1ye\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1732009564;\n\tbh=WWLlD1HNY0E8ut4ekRiDtYwAfO+oFuGf9tXjfsgGO9U=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=myUxn1yeDwOxCh8fCjE/u05klj1ublzUFr35ItXRMYw9sz6DGWcko+gkKHoRQrlqd\n\tjRUM9KSXJ8Thx/1YiqWM+N9XJHPAH64IwPkPpyYRBwQSKu5FZVq2hUqXaRlZ5BkJrX\n\td1MagIKmGhACP+u3XeIBZBCufEd7b1rkSYCqOd5E=","Message-ID":"","Date":"Tue, 19 Nov 2024 09:46:17 +0000","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v3 5/6] libcamera: camera_sensor_properties: Add sensor\n\tcontrol delays","To":"Laurent Pinchart ","Cc":"Kieran Bingham ,\n\tlibcamera-devel@lists.libcamera.org, mike.rudenko@gmail.com","References":"<20241115074628.417215-1-dan.scally@ideasonboard.com>\n\t<20241115074628.417215-6-dan.scally@ideasonboard.com>\n\t<173167262754.4187655.11344145749616706346@ping.linuxembedded.co.uk>\n\t\n\t<20241118013653.GI30787@pendragon.ideasonboard.com>","Content-Language":"en-US","From":"Dan Scally ","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":"<20241118013653.GI30787@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":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":32264,"web_url":"https://patchwork.libcamera.org/comment/32264/","msgid":"<6f48f10d-cb9c-4a85-85b5-97d2e6faff07@ideasonboard.com>","date":"2024-11-19T10:25:00","subject":"Re: [PATCH v3 5/6] libcamera: camera_sensor_properties: Add sensor\n\tcontrol delays","submitter":{"id":156,"url":"https://patchwork.libcamera.org/api/people/156/","name":"Dan Scally","email":"dan.scally@ideasonboard.com"},"content":"Hi Laurent, Kieran\n\nOn 18/11/2024 01:36, Laurent Pinchart wrote:\n> Hi Dan,\n>\n> On Fri, Nov 15, 2024 at 01:35:54PM +0000, Daniel Scally wrote:\n>> On 15/11/2024 12:10, Kieran Bingham wrote:\n>>> Quoting Daniel Scally (2024-11-15 07:46:27)\n>>>> Add properties covering the sensor control application delays to both\n>>>> the static CameraSensorProperties definitions. The values used are the\n>>>> defaults that're in use across the library, with deviations from that\n> I had never seen \"that're\" before.\n>\n>>>> taken from Raspberry Pi's CamHelper class definitions.\n>>>>\n>>>> Signed-off-by: Daniel Scally \n>>>> ---\n>>>> Changes in v3:\n>>>>\n>>>> - Rebased on top of the CameraSensorFactory introduction\n>>>> - Some rephrasing\n>>>> - Defined the sensorDelays member as empty where Raspberry Pi didn't\n>>>> have any specific values. Check for the empty struct in\n>>>> getSensorDelays() and return the defaults from there with a warning.\n>>> Sounds resonable to me.\n>>>\n>>>> Changes in v2:\n>>>>\n>>>> - Rather than adding the delays to the properties ControlList, added a\n>>>> new function in CameraSensor that allows PipelineHandlers to retreive\n>>>> the delay values.\n>>>>\n>>>> include/libcamera/internal/camera_sensor.h | 2 +\n>>>> .../internal/camera_sensor_properties.h | 9 +++\n>>>> src/libcamera/sensor/camera_sensor.cpp | 13 +++\n>>>> src/libcamera/sensor/camera_sensor_legacy.cpp | 33 ++++++++\n>>>> .../sensor/camera_sensor_properties.cpp | 79 +++++++++++++++++++\n>>>> 5 files changed, 136 insertions(+)\n>>>>\n>>>> diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h\n>>>> index 8aafd82e..a9b2d494 100644\n>>>> --- a/include/libcamera/internal/camera_sensor.h\n>>>> +++ b/include/libcamera/internal/camera_sensor.h\n>>>> @@ -73,6 +73,8 @@ public:\n>>>> virtual const std::vector &\n>>>> testPatternModes() const = 0;\n>>>> virtual int setTestPatternMode(controls::draft::TestPatternModeEnum mode) = 0;\n>>>> + virtual void getSensorDelays(uint8_t &exposureDelay, uint8_t &gainDelay,\n>>>> + uint8_t &vblankDelay, uint8_t &hblankDelay) = 0;\n>>> Could/Should this return a const pointer to a struct SensorDelays now ?\n>> Yeah maybe...that would add a requirement for the struct to be available to CameraSensor (which is a\n>> Factory class now) - I don't really have any strong feelings either way, so as long as that's fine\n>> then I'll make the change.\n> I think it would make the API nicer.\n>\nHm, how do we represent the defaults then? A global instance of the struct holding defaults?\n>>>> };\n>>>> \n>>>> class CameraSensorFactoryBase\n>>>> diff --git a/include/libcamera/internal/camera_sensor_properties.h b/include/libcamera/internal/camera_sensor_properties.h\n>>>> index 480ac121..56d5c15d 100644\n>>>> --- a/include/libcamera/internal/camera_sensor_properties.h\n>>>> +++ b/include/libcamera/internal/camera_sensor_properties.h\n>>>> @@ -10,6 +10,8 @@\n>>>> #include \n>>>> #include \n>>>> \n>>>> +#include \n> stdint.h should go just before string, we mix the C and C++ headers.\n>\n>>>> +\n>>>> #include \n>>>> #include \n>>>> \n>>>> @@ -20,6 +22,13 @@ struct CameraSensorProperties {\n>>>> \n>>>> Size unitCellSize;\n>>>> std::map testPatternModes;\n>>>> +\n>>>> + struct {\n>>>> + uint8_t exposureDelay;\n>>>> + uint8_t gainDelay;\n>>>> + uint8_t vblankDelay;\n>>>> + uint8_t hblankDelay;\n>>>> + } sensorDelays;\n>>>> };\n>>>> \n>>>> } /* namespace libcamera */\n>>>> diff --git a/src/libcamera/sensor/camera_sensor.cpp b/src/libcamera/sensor/camera_sensor.cpp\n>>>> index 54cf98b2..61420873 100644\n>>>> --- a/src/libcamera/sensor/camera_sensor.cpp\n>>>> +++ b/src/libcamera/sensor/camera_sensor.cpp\n>>>> @@ -336,6 +336,19 @@ CameraSensor::~CameraSensor() = default;\n>>>> * pattern mode for every frame thus incurs no performance penalty.\n>>>> */\n>>>> \n>>>> +/**\n>>>> + * \\fn CameraSensor::getSensorDelays()\n> We don't usually prefix getters with \"get\".\n>\n>>>> + * \\brief Fetch the sensor delay values\n>>>> + * \\param[out] exposureDelay The exposure delay\n>>>> + * \\param[out] gainDelay The analogue gain delay\n>>>> + * \\param[out] vblankDelay The vblank delay\n>>>> + * \\param[out] hblankDelay The hblank delay\n>>>> + *\n>>>> + * This function fills in sensor control delays for pipeline handlers to use to\n>>>> + * inform the DelayedControls. If no static properties are available it fills in\n>>>> + * some widely applicable default values.\n> We need to return some default values until all sensors provide the\n> information we need, but I wouldn't call those \"widely applicable\n> default values\". It makes it sound those defaults will have a high\n> chance of working, while that's not specifically the case.\n>\n>>>> + */\n>>>> +\n>>>> /**\n>>>> * \\class CameraSensorFactoryBase\n>>>> * \\brief Base class for camera sensor factories\n>>>> diff --git a/src/libcamera/sensor/camera_sensor_legacy.cpp b/src/libcamera/sensor/camera_sensor_legacy.cpp\n>>>> index a9b15c03..84972f02 100644\n>>>> --- a/src/libcamera/sensor/camera_sensor_legacy.cpp\n>>>> +++ b/src/libcamera/sensor/camera_sensor_legacy.cpp\n>>>> @@ -95,6 +95,8 @@ public:\n>>>> const std::vector &\n>>>> testPatternModes() const override { return testPatternModes_; }\n>>>> int setTestPatternMode(controls::draft::TestPatternModeEnum mode) override;\n>>>> + void getSensorDelays(uint8_t &exposureDelay, uint8_t &gainDelay,\n>>>> + uint8_t &vblankDelay, uint8_t &hblankDelay) override;\n>>>> \n>>>> protected:\n>>>> std::string logPrefix() const override;\n>>>> @@ -482,6 +484,37 @@ void CameraSensorLegacy::initStaticProperties()\n>>>> initTestPatternModes();\n>>>> }\n>>>> \n>>>> +void CameraSensorLegacy::getSensorDelays(uint8_t &exposureDelay, uint8_t &gainDelay,\n>>>> + uint8_t &vblankDelay, uint8_t &hblankDelay)\n>>>> +{\n>>>> +\n>>>> + /*\n>>>> + * These defaults are applicable to many sensors, however more specific\n>>>> + * values can be added to the CameraSensorProperties for a sensor if\n>>>> + * required.\n>>>> + */\n>>>> + if (!staticProps_ ||\n>>>> + (!staticProps_->sensorDelays.exposureDelay &&\n>>>> + !staticProps_->sensorDelays.gainDelay &&\n>>>> + !staticProps_->sensorDelays.vblankDelay &&\n>>>> + !staticProps_->sensorDelays.hblankDelay)) {\n>>>> + LOG(CameraSensor, Warning)\n>>>> + << \"No sensor delays found in static properties. \"\n>>>> + \"Assuming unverified defaults.\";\n> Here the lack of sensor-specific values is reported with a more\n> appropriate seriousness. Let's update the comments to match.\n>\n>>>> +\n>>>> + exposureDelay = 2;\n>>>> + gainDelay = 1;\n>>>> + vblankDelay = 2;\n>>>> + hblankDelay = 2;\n>>>> + return;\n>>>> + }\n>>>> +\n>>>> + exposureDelay = staticProps_->sensorDelays.exposureDelay;\n>>>> + gainDelay = staticProps_->sensorDelays.gainDelay;\n>>>> + vblankDelay = staticProps_->sensorDelays.vblankDelay;\n>>>> + hblankDelay = staticProps_->sensorDelays.hblankDelay;\n>>> Which would make this just\n>>>\n>>> \treturn staticProps_->sensorDelays;\n>>>\n>>>> +}\n>>> I'm glad this function prints a warning!\n>>>\n>>>> +\n>>>> void CameraSensorLegacy::initTestPatternModes()\n>>>> {\n>>>> const auto &v4l2TestPattern = controls().find(V4L2_CID_TEST_PATTERN);\n>>>> diff --git a/src/libcamera/sensor/camera_sensor_properties.cpp b/src/libcamera/sensor/camera_sensor_properties.cpp\n>>>> index 6d4136d0..6dda7ba9 100644\n>>>> --- a/src/libcamera/sensor/camera_sensor_properties.cpp\n>>>> +++ b/src/libcamera/sensor/camera_sensor_properties.cpp\n>>>> @@ -41,6 +41,13 @@ LOG_DEFINE_CATEGORY(CameraSensorProperties)\n>>>> * \\brief Map that associates the TestPattern control value with the indexes of\n>>>> * the corresponding sensor test pattern modes as returned by\n>>>> * V4L2_CID_TEST_PATTERN.\n>>>> + *\n>>>> + * \\var CameraSensorProperties::sensorDelays\n>>>> + * \\brief Holds the delays, expressed in number of frames, between the time a\n>>>> + * control is applied to the sensor and the time it actually takes effect.\n>>>> + * Delays are recorded for the exposure time, analogue gain, vertical and\n>>>> + * horizontal blanking. These values may be defined as empty, in which case the\n>>>> + * CameraSensor derivative should provide default values.\n> A \\brief should be brief :-) Make it a single short sentence, and you\n> can add more information in the body of the documentation.\n>\n>>>> */\n>>>> \n>>>> /**\n>>>> @@ -60,6 +67,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n>>>> { controls::draft::TestPatternModeColorBars, 2 },\n>>>> { controls::draft::TestPatternModeColorBarsFadeToGray, 3 },\n>>>> },\n>>>> + .sensorDelays = { },\n> The AR0144 has a 2 frames delay for all parameters. Assuming that \"2\n> frames delay\" means that parameters set during frame N will take effect\n> for frame N+2 (so a delay of 0 means the parameter is applied\n> immediately to the current frame). Maybe this should be documented more\n> clearly above.\n>\n> Note that the sensor can also be programmed to apply a one frame delay\n> instead of two frames for the analog gain. The value is therefore\n> driver-dependent, and could even be changed dynamically. That's\n> something to worry about later.\n>\n>>>> } },\n>>>> { \"ar0521\", {\n>>>> .unitCellSize = { 2200, 2200 },\n>>>> @@ -69,6 +77,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n>>>> { controls::draft::TestPatternModeColorBars, 2 },\n>>>> { controls::draft::TestPatternModeColorBarsFadeToGray, 3 },\n>>>> },\n>>>> + .sensorDelays = { },\n> The AR0521 will be interesting. The exposure time has a 2 frames delay,\n> while the analog gain will have a 1 frame delay if it is set alone, and\n> a 2 frames delay if the exposure time is also set during the same frame.\n> This is something we should consider when we'll develop a tool to\n> measure delays.\n>\n> The sensor can be configured to have a fixed 2 frames delay for the\n> analog gain, but that's not how the driver currently configures it.\n>\n> The delay for the blanking values doesn't seem to be documented.\n>\n>>>> } },\n>>>> { \"hi846\", {\n>>>> .unitCellSize = { 1120, 1120 },\n>>>> @@ -87,6 +96,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n>>>> * 9: \"Resolution Pattern\"\n>>>> */\n>>>> },\n>>>> + .sensorDelays = { },\n>>>> } },\n>>>> { \"imx214\", {\n>>>> .unitCellSize = { 1120, 1120 },\n>>>> @@ -97,6 +107,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n>>>> { controls::draft::TestPatternModeColorBarsFadeToGray, 3 },\n>>>> { controls::draft::TestPatternModePn9, 4 },\n>>>> },\n>>>> + .sensorDelays = { },\n>>>> } },\n>>>> { \"imx219\", {\n>>>> .unitCellSize = { 1120, 1120 },\n>>>> @@ -107,6 +118,12 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n>>>> { controls::draft::TestPatternModeColorBarsFadeToGray, 3 },\n>>>> { controls::draft::TestPatternModePn9, 4 },\n>>>> },\n>>>> + .sensorDelays = {\n>>>> + .exposureDelay = 2,\n>>>> + .gainDelay = 1,\n>>>> + .vblankDelay = 2,\n>>>> + .hblankDelay = 2\n>>>> + },\n>>>> } },\n>>>> { \"imx258\", {\n>>>> .unitCellSize = { 1120, 1120 },\n>>>> @@ -117,34 +134,62 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n>>>> { controls::draft::TestPatternModeColorBarsFadeToGray, 3 },\n>>>> { controls::draft::TestPatternModePn9, 4 },\n>>>> },\n>>>> + .sensorDelays = { },\n>>>> } },\n>>>> { \"imx283\", {\n>>>> .unitCellSize = { 2400, 2400 },\n>>>> .testPatternModes = {},\n>>>> + .sensorDelays = {\n>>>> + .exposureDelay = 2,\n>>>> + .gainDelay = 2,\n>>>> + .vblankDelay = 2,\n>>>> + .hblankDelay = 2\n>>>> + },\n>>>> } },\n>>>> { \"imx290\", {\n>>>> .unitCellSize = { 2900, 2900 },\n>>>> .testPatternModes = {},\n>>>> + .sensorDelays = {\n>>>> + .exposureDelay = 2,\n>>>> + .gainDelay = 2,\n>>>> + .vblankDelay = 2,\n>>>> + .hblankDelay = 2\n>>>> + },\n>>>> } },\n>>>> { \"imx296\", {\n>>>> .unitCellSize = { 3450, 3450 },\n>>>> .testPatternModes = {},\n>>>> + .sensorDelays = {\n>>>> + .exposureDelay = 2,\n>>>> + .gainDelay = 2,\n>>>> + .vblankDelay = 2,\n>>>> + .hblankDelay = 2\n>>>> + },\n>>>> } },\n>>>> { \"imx327\", {\n>>>> .unitCellSize = { 2900, 2900 },\n>>>> .testPatternModes = {},\n>>>> + .sensorDelays = { },\n>>>> } },\n>>>> { \"imx335\", {\n>>>> .unitCellSize = { 2000, 2000 },\n>>>> .testPatternModes = {},\n>>>> + .sensorDelays = { },\n>>>> } },\n>>>> { \"imx415\", {\n>>>> .unitCellSize = { 1450, 1450 },\n>>>> .testPatternModes = {},\n>>>> + .sensorDelays = { },\n>>>> } },\n>>>> { \"imx477\", {\n>>>> .unitCellSize = { 1550, 1550 },\n>>>> .testPatternModes = {},\n>>>> + .sensorDelays = {\n>>>> + .exposureDelay = 2,\n>>>> + .gainDelay = 2,\n>>>> + .vblankDelay = 3,\n>>>> + .hblankDelay = 3\n>>>> + },\n>>>> } },\n>>>> { \"imx519\", {\n>>>> .unitCellSize = { 1220, 1220 },\n>>>> @@ -157,6 +202,12 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n>>>> * these two patterns do not comply with MIPI CCS v1.1 (Section 10.1).\n>>>> */\n>>>> },\n>>>> + .sensorDelays = {\n>>>> + .exposureDelay = 2,\n>>>> + .gainDelay = 2,\n>>>> + .vblankDelay = 3,\n>>>> + .hblankDelay = 3\n>>>> + },\n>>>> } },\n>>>> { \"imx708\", {\n>>>> .unitCellSize = { 1400, 1400 },\n>>>> @@ -167,6 +218,12 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n>>>> { controls::draft::TestPatternModeColorBarsFadeToGray, 3 },\n>>>> { controls::draft::TestPatternModePn9, 4 },\n>>>> },\n>>>> + .sensorDelays = {\n>>>> + .exposureDelay = 2,\n>>>> + .gainDelay = 2,\n>>>> + .vblankDelay = 3,\n>>>> + .hblankDelay = 3\n>>>> + },\n>>>> } },\n>>>> { \"ov2685\", {\n>>>> .unitCellSize = { 1750, 1750 },\n>>>> @@ -181,6 +238,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n>>>> * 5: \"Color Square\"\n>>>> */\n>>>> },\n>>>> + .sensorDelays = { },\n>>>> } },\n>>>> { \"ov2740\", {\n>>>> .unitCellSize = { 1400, 1400 },\n>>>> @@ -188,6 +246,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n>>>> { controls::draft::TestPatternModeOff, 0 },\n>>>> { controls::draft::TestPatternModeColorBars, 1},\n>>>> },\n>>>> + .sensorDelays = { },\n>>>> } },\n>>>> { \"ov4689\", {\n>>>> .unitCellSize = { 2000, 2000 },\n>>>> @@ -201,6 +260,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n>>>> * colorBarType2 and colorBarType3.\n>>>> */\n>>>> },\n>>>> + .sensorDelays = { },\n>>>> } },\n>>>> { \"ov5640\", {\n>>>> .unitCellSize = { 1400, 1400 },\n>>>> @@ -208,10 +268,17 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n>>>> { controls::draft::TestPatternModeOff, 0 },\n>>>> { controls::draft::TestPatternModeColorBars, 1 },\n>>>> },\n>>>> + .sensorDelays = { },\n>>>> } },\n>>>> { \"ov5647\", {\n>>>> .unitCellSize = { 1400, 1400 },\n>>>> .testPatternModes = {},\n>>>> + .sensorDelays = {\n>>>> + .exposureDelay = 2,\n>>>> + .gainDelay = 2,\n>>>> + .vblankDelay = 2,\n>>>> + .hblankDelay = 2\n>>>> + },\n>>>> } },\n>>>> { \"ov5670\", {\n>>>> .unitCellSize = { 1120, 1120 },\n>>>> @@ -219,6 +286,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n>>>> { controls::draft::TestPatternModeOff, 0 },\n>>>> { controls::draft::TestPatternModeColorBars, 1 },\n>>>> },\n>>>> + .sensorDelays = { },\n>>>> } },\n>>>> { \"ov5675\", {\n>>>> .unitCellSize = { 1120, 1120 },\n>>>> @@ -226,6 +294,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n>>>> { controls::draft::TestPatternModeOff, 0 },\n>>>> { controls::draft::TestPatternModeColorBars, 1 },\n>>>> },\n>>>> + .sensorDelays = { },\n>>>> } },\n>>>> { \"ov5693\", {\n>>>> .unitCellSize = { 1400, 1400 },\n>>>> @@ -238,6 +307,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n>>>> * Rolling Bar\".\n>>>> */\n>>>> },\n>>>> + .sensorDelays = { },\n>>>> } },\n>>>> { \"ov64a40\", {\n>>>> .unitCellSize = { 1008, 1008 },\n>>>> @@ -251,6 +321,12 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n>>>> * 4: \"Vertical Color Bar Type 4\"\n>>>> */\n>>>> },\n>>>> + .sensorDelays = {\n>>>> + .exposureDelay = 2,\n>>>> + .gainDelay = 2,\n>>>> + .vblankDelay = 2,\n>>>> + .hblankDelay = 2\n>>>> + },\n>>>> } },\n>>>> { \"ov8858\", {\n>>>> .unitCellSize = { 1120, 1120 },\n>>>> @@ -264,6 +340,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n>>>> * 4: \"Vertical Color Bar Type 4\"\n>>>> */\n>>>> },\n>>>> + .sensorDelays = { },\n>>>> } },\n>>>> { \"ov8865\", {\n>>>> .unitCellSize = { 1400, 1400 },\n>>>> @@ -278,6 +355,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n>>>> * 5: \"Color squares with rolling bar\"\n>>>> */\n>>>> },\n>>>> + .sensorDelays = { },\n>>>> } },\n>>>> { \"ov13858\", {\n>>>> .unitCellSize = { 1120, 1120 },\n>>>> @@ -285,6 +363,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n>>>> { controls::draft::TestPatternModeOff, 0 },\n>>>> { controls::draft::TestPatternModeColorBars, 1 },\n>>>> },\n>>>> + .sensorDelays = { },\n>>>> } },\n>>>> };\n>>>>","headers":{"Return-Path":"","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 A72D9C326B\n\tfor ;\n\tTue, 19 Nov 2024 10:25:06 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4454065EF2;\n\tTue, 19 Nov 2024 11:25:06 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 77BBF65EEB\n\tfor ;\n\tTue, 19 Nov 2024 11:25: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 218E1B3;\n\tTue, 19 Nov 2024 11:24:46 +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=\"N4vlSLGp\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1732011886;\n\tbh=K2RBvvMRgKUEBda4U97Wx6kKwsn9T0jylWh1Z7nPOno=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=N4vlSLGpcZxJhy9AGYMouJJK0y1sFfC0BgTpPrVr8Z4q4A3Jcyzv4VvWkXmMY7DE/\n\t3jsr5+Kx6nCJqfW8gOfPQdPsMIbJl0zwbpwYuCbjxWLIPngHpARosrlxzQDTZYXn5v\n\tPLvN8UIE9J3qI32144zHWxQ8NhF+u3qWrY7yD3DE=","Message-ID":"<6f48f10d-cb9c-4a85-85b5-97d2e6faff07@ideasonboard.com>","Date":"Tue, 19 Nov 2024 10:25:00 +0000","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v3 5/6] libcamera: camera_sensor_properties: Add sensor\n\tcontrol delays","To":"Laurent Pinchart ","Cc":"Kieran Bingham ,\n\tlibcamera-devel@lists.libcamera.org, mike.rudenko@gmail.com","References":"<20241115074628.417215-1-dan.scally@ideasonboard.com>\n\t<20241115074628.417215-6-dan.scally@ideasonboard.com>\n\t<173167262754.4187655.11344145749616706346@ping.linuxembedded.co.uk>\n\t\n\t<20241118013653.GI30787@pendragon.ideasonboard.com>","Content-Language":"en-US","From":"Dan Scally ","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":"<20241118013653.GI30787@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":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":32269,"web_url":"https://patchwork.libcamera.org/comment/32269/","msgid":"<20241119104859.GG31681@pendragon.ideasonboard.com>","date":"2024-11-19T10:48:59","subject":"Re: [PATCH v3 5/6] libcamera: camera_sensor_properties: Add sensor\n\tcontrol delays","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Dan,\n\nOn Tue, Nov 19, 2024 at 10:25:00AM +0000, Daniel Scally wrote:\n> On 18/11/2024 01:36, Laurent Pinchart wrote:\n> > On Fri, Nov 15, 2024 at 01:35:54PM +0000, Daniel Scally wrote:\n> >> On 15/11/2024 12:10, Kieran Bingham wrote:\n> >>> Quoting Daniel Scally (2024-11-15 07:46:27)\n> >>>> Add properties covering the sensor control application delays to both\n> >>>> the static CameraSensorProperties definitions. The values used are the\n> >>>> defaults that're in use across the library, with deviations from that\n> >\n> > I had never seen \"that're\" before.\n> >\n> >>>> taken from Raspberry Pi's CamHelper class definitions.\n> >>>>\n> >>>> Signed-off-by: Daniel Scally \n> >>>> ---\n> >>>> Changes in v3:\n> >>>>\n> >>>> - Rebased on top of the CameraSensorFactory introduction\n> >>>> - Some rephrasing\n> >>>> - Defined the sensorDelays member as empty where Raspberry Pi didn't\n> >>>> have any specific values. Check for the empty struct in\n> >>>> getSensorDelays() and return the defaults from there with a warning.\n> >>> Sounds resonable to me.\n> >>>\n> >>>> Changes in v2:\n> >>>>\n> >>>> - Rather than adding the delays to the properties ControlList, added a\n> >>>> new function in CameraSensor that allows PipelineHandlers to retreive\n> >>>> the delay values.\n> >>>>\n> >>>> include/libcamera/internal/camera_sensor.h | 2 +\n> >>>> .../internal/camera_sensor_properties.h | 9 +++\n> >>>> src/libcamera/sensor/camera_sensor.cpp | 13 +++\n> >>>> src/libcamera/sensor/camera_sensor_legacy.cpp | 33 ++++++++\n> >>>> .../sensor/camera_sensor_properties.cpp | 79 +++++++++++++++++++\n> >>>> 5 files changed, 136 insertions(+)\n> >>>>\n> >>>> diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h\n> >>>> index 8aafd82e..a9b2d494 100644\n> >>>> --- a/include/libcamera/internal/camera_sensor.h\n> >>>> +++ b/include/libcamera/internal/camera_sensor.h\n> >>>> @@ -73,6 +73,8 @@ public:\n> >>>> virtual const std::vector &\n> >>>> testPatternModes() const = 0;\n> >>>> virtual int setTestPatternMode(controls::draft::TestPatternModeEnum mode) = 0;\n> >>>> + virtual void getSensorDelays(uint8_t &exposureDelay, uint8_t &gainDelay,\n> >>>> + uint8_t &vblankDelay, uint8_t &hblankDelay) = 0;\n> >>>\n> >>> Could/Should this return a const pointer to a struct SensorDelays now ?\n> >>\n> >> Yeah maybe...that would add a requirement for the struct to be available to CameraSensor (which is a\n> >> Factory class now) - I don't really have any strong feelings either way, so as long as that's fine\n> >> then I'll make the change.\n> >\n> > I think it would make the API nicer.\n>\n> Hm, how do we represent the defaults then? A global instance of the\n> struct holding defaults?\n\nYou can make it a static const variable, yes.\n\n> >>>> };\n> >>>> \n> >>>> class CameraSensorFactoryBase\n> >>>> diff --git a/include/libcamera/internal/camera_sensor_properties.h b/include/libcamera/internal/camera_sensor_properties.h\n> >>>> index 480ac121..56d5c15d 100644\n> >>>> --- a/include/libcamera/internal/camera_sensor_properties.h\n> >>>> +++ b/include/libcamera/internal/camera_sensor_properties.h\n> >>>> @@ -10,6 +10,8 @@\n> >>>> #include \n> >>>> #include \n> >>>> \n> >>>> +#include \n> >\n> > stdint.h should go just before string, we mix the C and C++ headers.\n> >\n> >>>> +\n> >>>> #include \n> >>>> #include \n> >>>> \n> >>>> @@ -20,6 +22,13 @@ struct CameraSensorProperties {\n> >>>> \n> >>>> Size unitCellSize;\n> >>>> std::map testPatternModes;\n> >>>> +\n> >>>> + struct {\n> >>>> + uint8_t exposureDelay;\n> >>>> + uint8_t gainDelay;\n> >>>> + uint8_t vblankDelay;\n> >>>> + uint8_t hblankDelay;\n> >>>> + } sensorDelays;\n> >>>> };\n> >>>> \n> >>>> } /* namespace libcamera */\n> >>>> diff --git a/src/libcamera/sensor/camera_sensor.cpp b/src/libcamera/sensor/camera_sensor.cpp\n> >>>> index 54cf98b2..61420873 100644\n> >>>> --- a/src/libcamera/sensor/camera_sensor.cpp\n> >>>> +++ b/src/libcamera/sensor/camera_sensor.cpp\n> >>>> @@ -336,6 +336,19 @@ CameraSensor::~CameraSensor() = default;\n> >>>> * pattern mode for every frame thus incurs no performance penalty.\n> >>>> */\n> >>>> \n> >>>> +/**\n> >>>> + * \\fn CameraSensor::getSensorDelays()\n> >\n> > We don't usually prefix getters with \"get\".\n> >\n> >>>> + * \\brief Fetch the sensor delay values\n> >>>> + * \\param[out] exposureDelay The exposure delay\n> >>>> + * \\param[out] gainDelay The analogue gain delay\n> >>>> + * \\param[out] vblankDelay The vblank delay\n> >>>> + * \\param[out] hblankDelay The hblank delay\n> >>>> + *\n> >>>> + * This function fills in sensor control delays for pipeline handlers to use to\n> >>>> + * inform the DelayedControls. If no static properties are available it fills in\n> >>>> + * some widely applicable default values.\n> >\n> > We need to return some default values until all sensors provide the\n> > information we need, but I wouldn't call those \"widely applicable\n> > default values\". It makes it sound those defaults will have a high\n> > chance of working, while that's not specifically the case.\n> >\n> >>>> + */\n> >>>> +\n> >>>> /**\n> >>>> * \\class CameraSensorFactoryBase\n> >>>> * \\brief Base class for camera sensor factories\n> >>>> diff --git a/src/libcamera/sensor/camera_sensor_legacy.cpp b/src/libcamera/sensor/camera_sensor_legacy.cpp\n> >>>> index a9b15c03..84972f02 100644\n> >>>> --- a/src/libcamera/sensor/camera_sensor_legacy.cpp\n> >>>> +++ b/src/libcamera/sensor/camera_sensor_legacy.cpp\n> >>>> @@ -95,6 +95,8 @@ public:\n> >>>> const std::vector &\n> >>>> testPatternModes() const override { return testPatternModes_; }\n> >>>> int setTestPatternMode(controls::draft::TestPatternModeEnum mode) override;\n> >>>> + void getSensorDelays(uint8_t &exposureDelay, uint8_t &gainDelay,\n> >>>> + uint8_t &vblankDelay, uint8_t &hblankDelay) override;\n> >>>> \n> >>>> protected:\n> >>>> std::string logPrefix() const override;\n> >>>> @@ -482,6 +484,37 @@ void CameraSensorLegacy::initStaticProperties()\n> >>>> initTestPatternModes();\n> >>>> }\n> >>>> \n> >>>> +void CameraSensorLegacy::getSensorDelays(uint8_t &exposureDelay, uint8_t &gainDelay,\n> >>>> + uint8_t &vblankDelay, uint8_t &hblankDelay)\n> >>>> +{\n> >>>> +\n> >>>> + /*\n> >>>> + * These defaults are applicable to many sensors, however more specific\n> >>>> + * values can be added to the CameraSensorProperties for a sensor if\n> >>>> + * required.\n> >>>> + */\n> >>>> + if (!staticProps_ ||\n> >>>> + (!staticProps_->sensorDelays.exposureDelay &&\n> >>>> + !staticProps_->sensorDelays.gainDelay &&\n> >>>> + !staticProps_->sensorDelays.vblankDelay &&\n> >>>> + !staticProps_->sensorDelays.hblankDelay)) {\n> >>>> + LOG(CameraSensor, Warning)\n> >>>> + << \"No sensor delays found in static properties. \"\n> >>>> + \"Assuming unverified defaults.\";\n> >\n> > Here the lack of sensor-specific values is reported with a more\n> > appropriate seriousness. Let's update the comments to match.\n> >\n> >>>> +\n> >>>> + exposureDelay = 2;\n> >>>> + gainDelay = 1;\n> >>>> + vblankDelay = 2;\n> >>>> + hblankDelay = 2;\n> >>>> + return;\n> >>>> + }\n> >>>> +\n> >>>> + exposureDelay = staticProps_->sensorDelays.exposureDelay;\n> >>>> + gainDelay = staticProps_->sensorDelays.gainDelay;\n> >>>> + vblankDelay = staticProps_->sensorDelays.vblankDelay;\n> >>>> + hblankDelay = staticProps_->sensorDelays.hblankDelay;\n> >>>\n> >>> Which would make this just\n> >>>\n> >>> \treturn staticProps_->sensorDelays;\n> >>>\n> >>>> +}\n> >>>\n> >>> I'm glad this function prints a warning!\n> >>>\n> >>>> +\n> >>>> void CameraSensorLegacy::initTestPatternModes()\n> >>>> {\n> >>>> const auto &v4l2TestPattern = controls().find(V4L2_CID_TEST_PATTERN);\n> >>>> diff --git a/src/libcamera/sensor/camera_sensor_properties.cpp b/src/libcamera/sensor/camera_sensor_properties.cpp\n> >>>> index 6d4136d0..6dda7ba9 100644\n> >>>> --- a/src/libcamera/sensor/camera_sensor_properties.cpp\n> >>>> +++ b/src/libcamera/sensor/camera_sensor_properties.cpp\n> >>>> @@ -41,6 +41,13 @@ LOG_DEFINE_CATEGORY(CameraSensorProperties)\n> >>>> * \\brief Map that associates the TestPattern control value with the indexes of\n> >>>> * the corresponding sensor test pattern modes as returned by\n> >>>> * V4L2_CID_TEST_PATTERN.\n> >>>> + *\n> >>>> + * \\var CameraSensorProperties::sensorDelays\n> >>>> + * \\brief Holds the delays, expressed in number of frames, between the time a\n> >>>> + * control is applied to the sensor and the time it actually takes effect.\n> >>>> + * Delays are recorded for the exposure time, analogue gain, vertical and\n> >>>> + * horizontal blanking. These values may be defined as empty, in which case the\n> >>>> + * CameraSensor derivative should provide default values.\n> >\n> > A \\brief should be brief :-) Make it a single short sentence, and you\n> > can add more information in the body of the documentation.\n> >\n> >>>> */\n> >>>> \n> >>>> /**\n> >>>> @@ -60,6 +67,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n> >>>> { controls::draft::TestPatternModeColorBars, 2 },\n> >>>> { controls::draft::TestPatternModeColorBarsFadeToGray, 3 },\n> >>>> },\n> >>>> + .sensorDelays = { },\n> >\n> > The AR0144 has a 2 frames delay for all parameters. Assuming that \"2\n> > frames delay\" means that parameters set during frame N will take effect\n> > for frame N+2 (so a delay of 0 means the parameter is applied\n> > immediately to the current frame). Maybe this should be documented more\n> > clearly above.\n> >\n> > Note that the sensor can also be programmed to apply a one frame delay\n> > instead of two frames for the analog gain. The value is therefore\n> > driver-dependent, and could even be changed dynamically. That's\n> > something to worry about later.\n> >\n> >>>> } },\n> >>>> { \"ar0521\", {\n> >>>> .unitCellSize = { 2200, 2200 },\n> >>>> @@ -69,6 +77,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n> >>>> { controls::draft::TestPatternModeColorBars, 2 },\n> >>>> { controls::draft::TestPatternModeColorBarsFadeToGray, 3 },\n> >>>> },\n> >>>> + .sensorDelays = { },\n> >\n> > The AR0521 will be interesting. The exposure time has a 2 frames delay,\n> > while the analog gain will have a 1 frame delay if it is set alone, and\n> > a 2 frames delay if the exposure time is also set during the same frame.\n> > This is something we should consider when we'll develop a tool to\n> > measure delays.\n> >\n> > The sensor can be configured to have a fixed 2 frames delay for the\n> > analog gain, but that's not how the driver currently configures it.\n> >\n> > The delay for the blanking values doesn't seem to be documented.\n> >\n> >>>> } },\n> >>>> { \"hi846\", {\n> >>>> .unitCellSize = { 1120, 1120 },\n> >>>> @@ -87,6 +96,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n> >>>> * 9: \"Resolution Pattern\"\n> >>>> */\n> >>>> },\n> >>>> + .sensorDelays = { },\n> >>>> } },\n> >>>> { \"imx214\", {\n> >>>> .unitCellSize = { 1120, 1120 },\n> >>>> @@ -97,6 +107,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n> >>>> { controls::draft::TestPatternModeColorBarsFadeToGray, 3 },\n> >>>> { controls::draft::TestPatternModePn9, 4 },\n> >>>> },\n> >>>> + .sensorDelays = { },\n> >>>> } },\n> >>>> { \"imx219\", {\n> >>>> .unitCellSize = { 1120, 1120 },\n> >>>> @@ -107,6 +118,12 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n> >>>> { controls::draft::TestPatternModeColorBarsFadeToGray, 3 },\n> >>>> { controls::draft::TestPatternModePn9, 4 },\n> >>>> },\n> >>>> + .sensorDelays = {\n> >>>> + .exposureDelay = 2,\n> >>>> + .gainDelay = 1,\n> >>>> + .vblankDelay = 2,\n> >>>> + .hblankDelay = 2\n> >>>> + },\n> >>>> } },\n> >>>> { \"imx258\", {\n> >>>> .unitCellSize = { 1120, 1120 },\n> >>>> @@ -117,34 +134,62 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n> >>>> { controls::draft::TestPatternModeColorBarsFadeToGray, 3 },\n> >>>> { controls::draft::TestPatternModePn9, 4 },\n> >>>> },\n> >>>> + .sensorDelays = { },\n> >>>> } },\n> >>>> { \"imx283\", {\n> >>>> .unitCellSize = { 2400, 2400 },\n> >>>> .testPatternModes = {},\n> >>>> + .sensorDelays = {\n> >>>> + .exposureDelay = 2,\n> >>>> + .gainDelay = 2,\n> >>>> + .vblankDelay = 2,\n> >>>> + .hblankDelay = 2\n> >>>> + },\n> >>>> } },\n> >>>> { \"imx290\", {\n> >>>> .unitCellSize = { 2900, 2900 },\n> >>>> .testPatternModes = {},\n> >>>> + .sensorDelays = {\n> >>>> + .exposureDelay = 2,\n> >>>> + .gainDelay = 2,\n> >>>> + .vblankDelay = 2,\n> >>>> + .hblankDelay = 2\n> >>>> + },\n> >>>> } },\n> >>>> { \"imx296\", {\n> >>>> .unitCellSize = { 3450, 3450 },\n> >>>> .testPatternModes = {},\n> >>>> + .sensorDelays = {\n> >>>> + .exposureDelay = 2,\n> >>>> + .gainDelay = 2,\n> >>>> + .vblankDelay = 2,\n> >>>> + .hblankDelay = 2\n> >>>> + },\n> >>>> } },\n> >>>> { \"imx327\", {\n> >>>> .unitCellSize = { 2900, 2900 },\n> >>>> .testPatternModes = {},\n> >>>> + .sensorDelays = { },\n> >>>> } },\n> >>>> { \"imx335\", {\n> >>>> .unitCellSize = { 2000, 2000 },\n> >>>> .testPatternModes = {},\n> >>>> + .sensorDelays = { },\n> >>>> } },\n> >>>> { \"imx415\", {\n> >>>> .unitCellSize = { 1450, 1450 },\n> >>>> .testPatternModes = {},\n> >>>> + .sensorDelays = { },\n> >>>> } },\n> >>>> { \"imx477\", {\n> >>>> .unitCellSize = { 1550, 1550 },\n> >>>> .testPatternModes = {},\n> >>>> + .sensorDelays = {\n> >>>> + .exposureDelay = 2,\n> >>>> + .gainDelay = 2,\n> >>>> + .vblankDelay = 3,\n> >>>> + .hblankDelay = 3\n> >>>> + },\n> >>>> } },\n> >>>> { \"imx519\", {\n> >>>> .unitCellSize = { 1220, 1220 },\n> >>>> @@ -157,6 +202,12 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n> >>>> * these two patterns do not comply with MIPI CCS v1.1 (Section 10.1).\n> >>>> */\n> >>>> },\n> >>>> + .sensorDelays = {\n> >>>> + .exposureDelay = 2,\n> >>>> + .gainDelay = 2,\n> >>>> + .vblankDelay = 3,\n> >>>> + .hblankDelay = 3\n> >>>> + },\n> >>>> } },\n> >>>> { \"imx708\", {\n> >>>> .unitCellSize = { 1400, 1400 },\n> >>>> @@ -167,6 +218,12 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n> >>>> { controls::draft::TestPatternModeColorBarsFadeToGray, 3 },\n> >>>> { controls::draft::TestPatternModePn9, 4 },\n> >>>> },\n> >>>> + .sensorDelays = {\n> >>>> + .exposureDelay = 2,\n> >>>> + .gainDelay = 2,\n> >>>> + .vblankDelay = 3,\n> >>>> + .hblankDelay = 3\n> >>>> + },\n> >>>> } },\n> >>>> { \"ov2685\", {\n> >>>> .unitCellSize = { 1750, 1750 },\n> >>>> @@ -181,6 +238,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n> >>>> * 5: \"Color Square\"\n> >>>> */\n> >>>> },\n> >>>> + .sensorDelays = { },\n> >>>> } },\n> >>>> { \"ov2740\", {\n> >>>> .unitCellSize = { 1400, 1400 },\n> >>>> @@ -188,6 +246,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n> >>>> { controls::draft::TestPatternModeOff, 0 },\n> >>>> { controls::draft::TestPatternModeColorBars, 1},\n> >>>> },\n> >>>> + .sensorDelays = { },\n> >>>> } },\n> >>>> { \"ov4689\", {\n> >>>> .unitCellSize = { 2000, 2000 },\n> >>>> @@ -201,6 +260,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n> >>>> * colorBarType2 and colorBarType3.\n> >>>> */\n> >>>> },\n> >>>> + .sensorDelays = { },\n> >>>> } },\n> >>>> { \"ov5640\", {\n> >>>> .unitCellSize = { 1400, 1400 },\n> >>>> @@ -208,10 +268,17 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n> >>>> { controls::draft::TestPatternModeOff, 0 },\n> >>>> { controls::draft::TestPatternModeColorBars, 1 },\n> >>>> },\n> >>>> + .sensorDelays = { },\n> >>>> } },\n> >>>> { \"ov5647\", {\n> >>>> .unitCellSize = { 1400, 1400 },\n> >>>> .testPatternModes = {},\n> >>>> + .sensorDelays = {\n> >>>> + .exposureDelay = 2,\n> >>>> + .gainDelay = 2,\n> >>>> + .vblankDelay = 2,\n> >>>> + .hblankDelay = 2\n> >>>> + },\n> >>>> } },\n> >>>> { \"ov5670\", {\n> >>>> .unitCellSize = { 1120, 1120 },\n> >>>> @@ -219,6 +286,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n> >>>> { controls::draft::TestPatternModeOff, 0 },\n> >>>> { controls::draft::TestPatternModeColorBars, 1 },\n> >>>> },\n> >>>> + .sensorDelays = { },\n> >>>> } },\n> >>>> { \"ov5675\", {\n> >>>> .unitCellSize = { 1120, 1120 },\n> >>>> @@ -226,6 +294,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n> >>>> { controls::draft::TestPatternModeOff, 0 },\n> >>>> { controls::draft::TestPatternModeColorBars, 1 },\n> >>>> },\n> >>>> + .sensorDelays = { },\n> >>>> } },\n> >>>> { \"ov5693\", {\n> >>>> .unitCellSize = { 1400, 1400 },\n> >>>> @@ -238,6 +307,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n> >>>> * Rolling Bar\".\n> >>>> */\n> >>>> },\n> >>>> + .sensorDelays = { },\n> >>>> } },\n> >>>> { \"ov64a40\", {\n> >>>> .unitCellSize = { 1008, 1008 },\n> >>>> @@ -251,6 +321,12 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n> >>>> * 4: \"Vertical Color Bar Type 4\"\n> >>>> */\n> >>>> },\n> >>>> + .sensorDelays = {\n> >>>> + .exposureDelay = 2,\n> >>>> + .gainDelay = 2,\n> >>>> + .vblankDelay = 2,\n> >>>> + .hblankDelay = 2\n> >>>> + },\n> >>>> } },\n> >>>> { \"ov8858\", {\n> >>>> .unitCellSize = { 1120, 1120 },\n> >>>> @@ -264,6 +340,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n> >>>> * 4: \"Vertical Color Bar Type 4\"\n> >>>> */\n> >>>> },\n> >>>> + .sensorDelays = { },\n> >>>> } },\n> >>>> { \"ov8865\", {\n> >>>> .unitCellSize = { 1400, 1400 },\n> >>>> @@ -278,6 +355,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n> >>>> * 5: \"Color squares with rolling bar\"\n> >>>> */\n> >>>> },\n> >>>> + .sensorDelays = { },\n> >>>> } },\n> >>>> { \"ov13858\", {\n> >>>> .unitCellSize = { 1120, 1120 },\n> >>>> @@ -285,6 +363,7 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen\n> >>>> { controls::draft::TestPatternModeOff, 0 },\n> >>>> { controls::draft::TestPatternModeColorBars, 1 },\n> >>>> },\n> >>>> + .sensorDelays = { },\n> >>>> } },\n> >>>> };\n> >>>>","headers":{"Return-Path":"","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 53E5CC32F1\n\tfor ;\n\tTue, 19 Nov 2024 10:49:11 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D849B65EF4;\n\tTue, 19 Nov 2024 11:49:10 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id BDDEC6589D\n\tfor ;\n\tTue, 19 Nov 2024 11:49:08 +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 121CD22E;\n\tTue, 19 Nov 2024 11:48:51 +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=\"KloyBAIv\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1732013331;\n\tbh=OePSn3BxZPKOSEJDD8bf/cBbZQKWs2ytDBR2FFLDzAU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=KloyBAIvKiwtCyRnQQLzm2VczCOSVcIa7jMiNFHRyq5pYoIZxilynejXEF4K8P2Ep\n\tPqf+itK04BMlFwp3g2A+8NiByHWz2lrY5tc8VwUXMZ+6ge82QHmlX+SdleqPDmHYXi\n\tk2AiM/DJ6o3v8RHFDae4lhE0eJAJbkA1PJYQeqbM=","Date":"Tue, 19 Nov 2024 12:48:59 +0200","From":"Laurent Pinchart ","To":"Dan Scally ","Cc":"Kieran Bingham ,\n\tlibcamera-devel@lists.libcamera.org, mike.rudenko@gmail.com","Subject":"Re: [PATCH v3 5/6] libcamera: camera_sensor_properties: Add sensor\n\tcontrol delays","Message-ID":"<20241119104859.GG31681@pendragon.ideasonboard.com>","References":"<20241115074628.417215-1-dan.scally@ideasonboard.com>\n\t<20241115074628.417215-6-dan.scally@ideasonboard.com>\n\t<173167262754.4187655.11344145749616706346@ping.linuxembedded.co.uk>\n\t\n\t<20241118013653.GI30787@pendragon.ideasonboard.com>\n\t<6f48f10d-cb9c-4a85-85b5-97d2e6faff07@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<6f48f10d-cb9c-4a85-85b5-97d2e6faff07@ideasonboard.com>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]