[{"id":32184,"web_url":"https://patchwork.libcamera.org/comment/32184/","msgid":"<173167276675.4187655.7009143180199354227@ping.linuxembedded.co.uk>","date":"2024-11-15T12:12:46","subject":"Re: [PATCH v3 6/6] libcamera: pipelines: Draw control delays from\n\tCameraSensor properties","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:28)\n> Rather than hard coding default delays for control values in the\n> pipeline handlers, pick up the ones defined in the CameraSensor\n> properties.\n> \n> Signed-off-by: Daniel Scally \n> ---\n> Changes in v3:\n> \n> - Fixed some broken alignment \n> \n> Changes in v2:\n> \n> - Switched to use the new getSensorDelays() function\n> \n> src/libcamera/pipeline/ipu3/ipu3.cpp | 12 +++++-------\n> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 13 ++++++-------\n> src/libcamera/pipeline/simple/simple.cpp | 8 ++++++--\n> 3 files changed, 17 insertions(+), 16 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index 0069d5e2..4d6b86b5 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -1077,14 +1077,12 @@ int PipelineHandlerIPU3::registerCameras()\n> if (ret)\n> continue;\n> \n> - /*\n> - * \\todo Read delay values from the sensor itself or from a\n> - * a sensor database. For now use generic values taken from\n> - * the Raspberry Pi and listed as 'generic values'.\n> - */\n> + uint8_t exposureDelay, gainDelay, vblankDelay, hblankDelay;\n> + cio2->sensor()->getSensorDelays(exposureDelay, gainDelay,\n> + vblankDelay, hblankDelay);\n\nThis probably makes me think passing the struct would be easier!\n\n> std::unordered_map params = {\n> - { V4L2_CID_ANALOGUE_GAIN, { 1, false } },\n> - { V4L2_CID_EXPOSURE, { 2, false } },\n> + { V4L2_CID_ANALOGUE_GAIN, { gainDelay, false } },\n> + { V4L2_CID_EXPOSURE, { exposureDelay, false } },\n\nShould we map all known control delays already even if they're not used ?\n\nSame comments apply below too.\n\n--\nKieran\n\n> };\n> \n> data->delayedCtrls_ =\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index 6c6d711f..42600908 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -24,6 +24,7 @@\n> #include \n> #include \n> #include \n> +#include \n> #include \n> #include \n> #include \n> @@ -1239,14 +1240,12 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)\n> /* Initialize the camera properties. */\n> data->properties_ = data->sensor_->properties();\n> \n> - /*\n> - * \\todo Read delay values from the sensor itself or from a\n> - * a sensor database. For now use generic values taken from\n> - * the Raspberry Pi and listed as generic values.\n> - */\n> + uint8_t exposureDelay, gainDelay, vblankDelay, hblankDelay;\n> + data->sensor_->getSensorDelays(exposureDelay, gainDelay, vblankDelay,\n> + hblankDelay);\n> std::unordered_map params = {\n> - { V4L2_CID_ANALOGUE_GAIN, { 1, false } },\n> - { V4L2_CID_EXPOSURE, { 2, false } },\n> + { V4L2_CID_ANALOGUE_GAIN, { gainDelay, false } },\n> + { V4L2_CID_EXPOSURE, { exposureDelay, false } },\n> };\n> \n> data->delayedCtrls_ =\n> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n> index 41fdf84c..20f395fb 100644\n> --- a/src/libcamera/pipeline/simple/simple.cpp\n> +++ b/src/libcamera/pipeline/simple/simple.cpp\n> @@ -26,6 +26,7 @@\n> \n> #include \n> #include \n> +#include \n> #include \n> #include \n> \n> @@ -1290,9 +1291,12 @@ int SimplePipelineHandler::configure(Camera *camera, CameraConfiguration *c)\n> if (outputCfgs.empty())\n> return 0;\n> \n> + uint8_t exposureDelay, gainDelay, vblankDelay, hblankDelay;\n> + data->sensor_->getSensorDelays(exposureDelay, gainDelay, vblankDelay,\n> + hblankDelay);\n> std::unordered_map params = {\n> - { V4L2_CID_ANALOGUE_GAIN, { 2, false } },\n> - { V4L2_CID_EXPOSURE, { 2, false } },\n> + { V4L2_CID_ANALOGUE_GAIN, { gainDelay, false } },\n> + { V4L2_CID_EXPOSURE, { exposureDelay, false } },\n> };\n> data->delayedCtrls_ =\n> std::make_unique(data->sensor_->device(),\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 62D32C0F1B\n\tfor ;\n\tFri, 15 Nov 2024 12:12:52 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3A2DE6587B;\n\tFri, 15 Nov 2024 13:12:51 +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 F2A696580A\n\tfor ;\n\tFri, 15 Nov 2024 13:12:49 +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 35627496;\n\tFri, 15 Nov 2024 13:12: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=\"bolaDJX9\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1731672755;\n\tbh=stD84/z6Ai7BT49HRRpPO1UVtbW4uq1L01/agf7bF+E=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=bolaDJX9wgaEd9vF/0dufQ6OY2fMyGK9cNPUQksW/usN9ou9Y7k6TjIH9wPi+FTTR\n\thSmA+/g5+w/gbOSbceOcfundhKDVD2uliJdA0jDJZAg6m4V7Hv5O0dMS92LuSyYCrh\n\tD5uFBzuj+i926ND94M8XexAGr6ftAwbMGQAhMhZ0=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20241115074628.417215-7-dan.scally@ideasonboard.com>","References":"<20241115074628.417215-1-dan.scally@ideasonboard.com>\n\t<20241115074628.417215-7-dan.scally@ideasonboard.com>","Subject":"Re: [PATCH v3 6/6] libcamera: pipelines: Draw control delays from\n\tCameraSensor properties","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:12:46 +0000","Message-ID":"<173167276675.4187655.7009143180199354227@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":32208,"web_url":"https://patchwork.libcamera.org/comment/32208/","msgid":"<20241118013850.GJ30787@pendragon.ideasonboard.com>","date":"2024-11-18T01:38:50","subject":"Re: [PATCH v3 6/6] libcamera: pipelines: Draw control delays from\n\tCameraSensor properties","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Fri, Nov 15, 2024 at 12:12:46PM +0000, Kieran Bingham wrote:\n> Quoting Daniel Scally (2024-11-15 07:46:28)\n> > Rather than hard coding default delays for control values in the\n> > pipeline handlers, pick up the ones defined in the CameraSensor\n> > properties.\n> > \n> > Signed-off-by: Daniel Scally \n> > ---\n> > Changes in v3:\n> > \n> > - Fixed some broken alignment \n> > \n> > Changes in v2:\n> > \n> > - Switched to use the new getSensorDelays() function\n> > \n> > src/libcamera/pipeline/ipu3/ipu3.cpp | 12 +++++-------\n> > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 13 ++++++-------\n> > src/libcamera/pipeline/simple/simple.cpp | 8 ++++++--\n> > 3 files changed, 17 insertions(+), 16 deletions(-)\n> > \n> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > index 0069d5e2..4d6b86b5 100644\n> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > @@ -1077,14 +1077,12 @@ int PipelineHandlerIPU3::registerCameras()\n> > if (ret)\n> > continue;\n> > \n> > - /*\n> > - * \\todo Read delay values from the sensor itself or from a\n> > - * a sensor database. For now use generic values taken from\n> > - * the Raspberry Pi and listed as 'generic values'.\n> > - */\n> > + uint8_t exposureDelay, gainDelay, vblankDelay, hblankDelay;\n> > + cio2->sensor()->getSensorDelays(exposureDelay, gainDelay,\n> > + vblankDelay, hblankDelay);\n> \n> This probably makes me think passing the struct would be easier!\n> \n> > std::unordered_map params = {\n> > - { V4L2_CID_ANALOGUE_GAIN, { 1, false } },\n> > - { V4L2_CID_EXPOSURE, { 2, false } },\n> > + { V4L2_CID_ANALOGUE_GAIN, { gainDelay, false } },\n> > + { V4L2_CID_EXPOSURE, { exposureDelay, false } },\n> \n> Should we map all known control delays already even if they're not used ?\n> \n> Same comments apply below too.\n> \n> > };\n\nShould the CameraSensor class report the map instead of just the delays\n?\n\n> > \n> > data->delayedCtrls_ =\n> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > index 6c6d711f..42600908 100644\n> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > @@ -24,6 +24,7 @@\n> > #include \n> > #include \n> > #include \n> > +#include \n> > #include \n> > #include \n> > #include \n> > @@ -1239,14 +1240,12 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)\n> > /* Initialize the camera properties. */\n> > data->properties_ = data->sensor_->properties();\n> > \n> > - /*\n> > - * \\todo Read delay values from the sensor itself or from a\n> > - * a sensor database. For now use generic values taken from\n> > - * the Raspberry Pi and listed as generic values.\n> > - */\n> > + uint8_t exposureDelay, gainDelay, vblankDelay, hblankDelay;\n> > + data->sensor_->getSensorDelays(exposureDelay, gainDelay, vblankDelay,\n> > + hblankDelay);\n> > std::unordered_map params = {\n> > - { V4L2_CID_ANALOGUE_GAIN, { 1, false } },\n> > - { V4L2_CID_EXPOSURE, { 2, false } },\n> > + { V4L2_CID_ANALOGUE_GAIN, { gainDelay, false } },\n> > + { V4L2_CID_EXPOSURE, { exposureDelay, false } },\n> > };\n> > \n> > data->delayedCtrls_ =\n> > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n> > index 41fdf84c..20f395fb 100644\n> > --- a/src/libcamera/pipeline/simple/simple.cpp\n> > +++ b/src/libcamera/pipeline/simple/simple.cpp\n> > @@ -26,6 +26,7 @@\n> > \n> > #include \n> > #include \n> > +#include \n> > #include \n> > #include \n> > \n> > @@ -1290,9 +1291,12 @@ int SimplePipelineHandler::configure(Camera *camera, CameraConfiguration *c)\n> > if (outputCfgs.empty())\n> > return 0;\n> > \n> > + uint8_t exposureDelay, gainDelay, vblankDelay, hblankDelay;\n> > + data->sensor_->getSensorDelays(exposureDelay, gainDelay, vblankDelay,\n> > + hblankDelay);\n> > std::unordered_map params = {\n> > - { V4L2_CID_ANALOGUE_GAIN, { 2, false } },\n> > - { V4L2_CID_EXPOSURE, { 2, false } },\n> > + { V4L2_CID_ANALOGUE_GAIN, { gainDelay, false } },\n> > + { V4L2_CID_EXPOSURE, { exposureDelay, false } },\n> > };\n> > data->delayedCtrls_ =\n> > std::make_unique(data->sensor_->device(),","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 46536C32E0\n\tfor ;\n\tMon, 18 Nov 2024 01:39:03 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4FEB2658BA;\n\tMon, 18 Nov 2024 02:39:02 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E141D600F2\n\tfor ;\n\tMon, 18 Nov 2024 02:38:59 +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 296995B3;\n\tMon, 18 Nov 2024 02:38:43 +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=\"kCb+AcW0\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1731893923;\n\tbh=iYuDJ/yPZzA/cladePkcS6R6dX+aKV5/ajizhqs2tR4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=kCb+AcW0NkIfQQG0q0qNXvQCPQbzwp7TrX6UVUlvdLvOQVkz7XNiHaXw3x4EvYJaH\n\tNcFqfrdwCUkoZQM55Cv2qN2WwwmrsgVMxu1HqfpbG668PlB2vRF2z3jr1n4WvHQui5\n\tqlfnG2vOjW9bFx3xCQv99slw8K/KHGslD5vEB5hY=","Date":"Mon, 18 Nov 2024 03:38:50 +0200","From":"Laurent Pinchart ","To":"Kieran Bingham ","Cc":"Daniel Scally ,\n\tlibcamera-devel@lists.libcamera.org, mike.rudenko@gmail.com","Subject":"Re: [PATCH v3 6/6] libcamera: pipelines: Draw control delays from\n\tCameraSensor properties","Message-ID":"<20241118013850.GJ30787@pendragon.ideasonboard.com>","References":"<20241115074628.417215-1-dan.scally@ideasonboard.com>\n\t<20241115074628.417215-7-dan.scally@ideasonboard.com>\n\t<173167276675.4187655.7009143180199354227@ping.linuxembedded.co.uk>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<173167276675.4187655.7009143180199354227@ping.linuxembedded.co.uk>","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\" "}}]