[{"id":28367,"web_url":"https://patchwork.libcamera.org/comment/28367/","msgid":"","date":"2024-01-02T12:15:36","subject":"Re: [libcamera-devel] [PATCH] ipa: rpi: Add hardware line rate\n\tconstraints","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi Naush\n\nThanks for the patch.\n\nOn Tue, 19 Dec 2023 at 12:57, Naushir Patuck via libcamera-devel\n wrote:\n>\n> Advertise hardware constraints on the pixel processing rate through the\n> Controller::HardwareConfig structure. When calculating the minimum line\n> length during a configure() operation, ensure that we don't exceed this\n> constraint.\n>\n> If we do exceed the hardware constraints, increase the modes's minimum\n> line length so the pixel processing rate falls below the hardware limit.\n> If this is not possible, throw a loud error message in the logs.\n>\n> Signed-off-by: Naushir Patuck \n\nThe change and code here seem reasonable to me. I guess the only minor\nconcern is to understand exactly what the constraint really is. Is it\njust about the line time, or is there a constraint on some number of\nblanking pixels? Or maybe the latter is equivalent to the former by\nvirtue of the fifos?\n\nSo it's an \"OK\" from me, but we should probably ask Nick for chapter and verse!\n\nReviewed-by: David Plowman \n\nThanks!\nDavid\n\n> ---\n> src/ipa/rpi/common/ipa_base.cpp | 26 ++++++++++++++++++++++++++\n> src/ipa/rpi/controller/controller.cpp | 3 +++\n> src/ipa/rpi/controller/controller.h | 2 ++\n> 3 files changed, 31 insertions(+)\n>\n> diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp\n> index 6ac9d5de2f88..419f19f120da 100644\n> --- a/src/ipa/rpi/common/ipa_base.cpp\n> +++ b/src/ipa/rpi/common/ipa_base.cpp\n> @@ -531,6 +531,32 @@ void IpaBase::setMode(const IPACameraSensorInfo &sensorInfo)\n> mode_.minLineLength = sensorInfo.minLineLength * (1.0s / sensorInfo.pixelRate);\n> mode_.maxLineLength = sensorInfo.maxLineLength * (1.0s / sensorInfo.pixelRate);\n>\n> + /*\n> + * Ensure that the maximum pixel processing rate does not exceed the ISP\n> + * hardware capabilities. If it does, try adjusting the minimum line\n> + * length to compensate if possible.\n> + */\n> + Duration minPixelTime = controller_.getHardwareConfig().minPixelProcessingTime;\n> + Duration pixelTime = mode_.minLineLength / mode_.width;\n> + if (minPixelTime && pixelTime < minPixelTime) {\n> + Duration adjustedLineLength = minPixelTime * mode_.width;\n> + if (adjustedLineLength <= mode_.maxLineLength) {\n> + LOG(IPARPI, Info)\n> + << \"Adjusting mode minimum line length from \" << mode_.minLineLength\n> + << \" to \" << adjustedLineLength << \" because of HW constraints.\";\n> + mode_.minLineLength = adjustedLineLength;\n> + } else {\n> + LOG(IPARPI, Error)\n> + << \"Sensor minimum line length of \" << pixelTime * mode_.width\n> + << \" (\" << 1us / pixelTime << \" MPix/s)\"\n> + << \" is below the minimum allowable HW limit of \" << minPixelTime * mode_.width\n> + << \" (\" << 1us / minPixelTime << \" MPix/s) \";\n> + LOG(IPARPI, Error)\n> + << \"THIS WILL CAUSE IMAGE CORRUPTION!!! \"\n> + << \"Please update the driver to allow more horizontal blanking control.\";\n> + }\n> + }\n> +\n> /*\n> * Set the frame length limits for the mode to ensure exposure and\n> * framerate calculations are clipped appropriately.\n> diff --git a/src/ipa/rpi/controller/controller.cpp b/src/ipa/rpi/controller/controller.cpp\n> index e62becd87e85..f81edf769736 100644\n> --- a/src/ipa/rpi/controller/controller.cpp\n> +++ b/src/ipa/rpi/controller/controller.cpp\n> @@ -17,6 +17,7 @@\n>\n> using namespace RPiController;\n> using namespace libcamera;\n> +using namespace std::literals::chrono_literals;\n>\n> LOG_DEFINE_CATEGORY(RPiController)\n>\n> @@ -37,6 +38,7 @@ static const std::map HardwareConfigMap\n> .numGammaPoints = 33,\n> .pipelineWidth = 13,\n> .statsInline = false,\n> + .minPixelProcessingTime = 0s,\n> }\n> },\n> {\n> @@ -51,6 +53,7 @@ static const std::map HardwareConfigMap\n> .numGammaPoints = 64,\n> .pipelineWidth = 16,\n> .statsInline = true,\n> + .minPixelProcessingTime = 1.0us / 380, /* 380 MPix/s */\n> }\n> },\n> };\n> diff --git a/src/ipa/rpi/controller/controller.h b/src/ipa/rpi/controller/controller.h\n> index 6e5f595284fd..170aea740789 100644\n> --- a/src/ipa/rpi/controller/controller.h\n> +++ b/src/ipa/rpi/controller/controller.h\n> @@ -15,6 +15,7 @@\n> #include \n> #include \n>\n> +#include \n> #include \"libcamera/internal/yaml_parser.h\"\n>\n> #include \"camera_mode.h\"\n> @@ -47,6 +48,7 @@ public:\n> unsigned int numGammaPoints;\n> unsigned int pipelineWidth;\n> bool statsInline;\n> + libcamera::utils::Duration minPixelProcessingTime;\n> };\n>\n> Controller();\n> --\n> 2.34.1\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 BB54ABDB1D\n\tfor ;\n\tTue, 2 Jan 2024 12:15:50 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2B6C962B41;\n\tTue, 2 Jan 2024 13:15:50 +0100 (CET)","from mail-qv1-xf36.google.com (mail-qv1-xf36.google.com\n\t[IPv6:2607:f8b0:4864:20::f36])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8B5A162B32\n\tfor ;\n\tTue, 2 Jan 2024 13:15:48 +0100 (CET)","by mail-qv1-xf36.google.com with SMTP id\n\t6a1803df08f44-680adbf077dso16765066d6.1\n\tfor ;\n\tTue, 02 Jan 2024 04:15:48 -0800 (PST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1704197750;\n\tbh=5mn82JE6icrDK8ngdtaKP3qyw7wcDwFY3sgcQChRrHg=;\n\th=References:In-Reply-To:Date:To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=AubTdFqYHJ6mw+5kytxkgR7q0R4EgGtlfvs+gOSvOU5uoL37Nxl9q487dzyZDLjts\n\tI634sfDtnhPnJjxRJxwUnUqDdkndyZlZac0wiKoJW8TbQc3RhYArd91FinMU7Ddhnk\n\tXamrXW9diO6Ji805xbSVnxsd6ow/aqj+buFeOhi7obZwuNvSMzNEcIXdW/Cfj3yMCW\n\tfvMDMvhKLRUR1WqObgAr4SyjGfMoRs1eqTjAKaM8I487J/67jxObAGxEft+2HpPZQy\n\t9lmA3g7Y8ti/mafzmw5kF01W0FkmCeHfgT5dWRwJ+zqqQ+5NT0cdlJhAhR3Rp8bDyQ\n\tA3W7BGAe8Dxyw==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1704197747; x=1704802547;\n\tdarn=lists.libcamera.org; \n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc:subject:date:message-id:reply-to;\n\tbh=fdLzcr9+Ufpe0j6CVmBc4JR+lMMTWk33P3Wi5SH7cFQ=;\n\tb=gSPG4r6j2ubgf+DuHL81qJFQBNZKA1meXeVABCav0ku+ACxlGX/HyttkZY/VoQzv8+\n\tVumcx1nhaJ+3dHxAGAK0RsLjg4Pis4Gqv0EVB0emnnyNB85Kv34x9JdH+ultIbrXnm2e\n\tfFNXDwGOAwZf5LeTVy2QcsxYtVO4QRTsZysgtCJUgUZcphm/q6r6dtMxYZv4A39+182o\n\tcl10H2jC4qIWg8C1+1+xIZE5ymiSCowWcfc+Cro7vnmIqdmuf2I4MV2+7nbwRowOAtTV\n\t+3Pn/W7TqXku56GM0WiTpjHwiZU9DeTSqJroJp0BU0/gYMsEF2yr+lfxA6kF4pRNfVfI\n\t+E1A=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"gSPG4r6j\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1704197747; x=1704802547;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-message-state:from:to:cc:subject:date:message-id\n\t:reply-to;\n\tbh=fdLzcr9+Ufpe0j6CVmBc4JR+lMMTWk33P3Wi5SH7cFQ=;\n\tb=bVUNLnwYUFSuhSEjc2M7QO1RUDN63M6GvuzuPBgh7hBw8tRhrrPIJn4XARpUPTx4F+\n\tQT6KNTAaJEn/n2ASyfBAiWinER93uDmi8/HqSm3McTQ3M+tp8Ykp0OPM5TuIue2dYU78\n\tnf6JY6NPfGbcgfImVvyDbNkTH91NqunHmMmuqJJlJPuXTsQCyt6c5J0gks+H/Zjr/O40\n\tj5JGBCXPBGM4pav/V8Gw9xYWe8jCFq4Asfwrcmag7lqYX2Ol/25UOEH3PyXpVjkdN7pf\n\tYdHP3Ot67uKXomtSDVScK7Bseb1IjU4NAnIulnujJcHajqCTjY2JBK3UCYQD2Ms5y13V\n\tutaQ==","X-Gm-Message-State":"AOJu0YwoNATWhrOQy/427hCYViV1mdZtwnQbN56qBMW12l1BCRTscuV0\n\thlBSuLiIYEZzRta4O/T6jJh39gbsMlgNuTOhNCoSsxuf/Fw3x0zjYYn3plp6","X-Google-Smtp-Source":"AGHT+IFNjNyQuO1BfIH1DEC2QbAU2+1UH4wtiOkKDq0fIFpu96BNjofXfGewyHdp3nDwXM8os5sg4txPvvqQ3HoH8V0=","X-Received":"by 2002:a05:6214:2506:b0:67f:22ee:5b0f with SMTP id\n\tgf6-20020a056214250600b0067f22ee5b0fmr32532982qvb.35.1704197747136;\n\tTue, 02 Jan 2024 04:15:47 -0800 (PST)","MIME-Version":"1.0","References":"<20231219125749.14362-1-naush@raspberrypi.com>","In-Reply-To":"<20231219125749.14362-1-naush@raspberrypi.com>","Date":"Tue, 2 Jan 2024 12:15:36 +0000","Message-ID":"","To":"Naushir Patuck ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH] ipa: rpi: Add hardware line rate\n\tconstraints","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","From":"David Plowman via libcamera-devel ","Reply-To":"David Plowman ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":28371,"web_url":"https://patchwork.libcamera.org/comment/28371/","msgid":"","date":"2024-01-03T14:17:15","subject":"Re: [libcamera-devel] [PATCH] ipa: rpi: Add hardware line rate\n\tconstraints","submitter":{"id":130,"url":"https://patchwork.libcamera.org/api/people/130/","name":"Nick Hollinghurst","email":"nick.hollinghurst@raspberrypi.com"},"content":"Hi all,\n\n> From: David Plowman \n> Date: Tue, 2 Jan 2024 at 12:15\n> Subject: Re: [libcamera-devel] [PATCH] ipa: rpi: Add hardware line\n> rate constraints\n> To: Naushir Patuck \n> Cc: \n>\n>\n> Hi Naush\n>\n> Thanks for the patch.\n>\n> On Tue, 19 Dec 2023 at 12:57, Naushir Patuck via libcamera-devel\n> wrote:\n> >\n> > Advertise hardware constraints on the pixel processing rate through the\n> > Controller::HardwareConfig structure. When calculating the minimum line\n> > length during a configure() operation, ensure that we don't exceed this\n> > constraint.\n> >\n> > If we do exceed the hardware constraints, increase the modes's minimum\n> > line length so the pixel processing rate falls below the hardware limit.\n> > If this is not possible, throw a loud error message in the logs.\n> >\n> > Signed-off-by: Naushir Patuck \n>\n> The change and code here seem reasonable to me. I guess the only minor\n> concern is to understand exactly what the constraint really is. Is it\n> just about the line time, or is there a constraint on some number of\n> blanking pixels? Or maybe the latter is equivalent to the former by\n> virtue of the fifos?\n>\n> So it's an \"OK\" from me, but we should probably ask Nick for chapter and verse!\n>\n> Reviewed-by: David Plowman \n>\n> Thanks!\n> David\n\nLGTM\n\nThe constraint is on the rate of pixels going from CSI2 peripheral to\nISP-FE (400Mpix/s, plus tiny overheads per scanline, for which\n380Mpix/s is a conservative bound).\n\nThere is a 64kbit data FIFO before the bottleneck, which means that in\nall reasonable cases the constraint applies at a timescale >= 1\nscanline, so adding horizontal blanking can prevent loss.\n\nIf the backlog were to grow beyond 64kbit during a single scanline,\nthere could still be loss. This could happen using 4 lanes at 1.5Gbps\nat 10bpp with frames wider than ~16,000 pixels. We don't expect to\nreach that with any known camera, so the change is good.\n\n Nick\n\nReviewed-by: Nick Hollinghurst \n\n> > ---\n> > src/ipa/rpi/common/ipa_base.cpp | 26 ++++++++++++++++++++++++++\n> > src/ipa/rpi/controller/controller.cpp | 3 +++\n> > src/ipa/rpi/controller/controller.h | 2 ++\n> > 3 files changed, 31 insertions(+)\n> >\n> > diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp\n> > index 6ac9d5de2f88..419f19f120da 100644\n> > --- a/src/ipa/rpi/common/ipa_base.cpp\n> > +++ b/src/ipa/rpi/common/ipa_base.cpp\n> > @@ -531,6 +531,32 @@ void IpaBase::setMode(const IPACameraSensorInfo &sensorInfo)\n> > mode_.minLineLength = sensorInfo.minLineLength * (1.0s / sensorInfo.pixelRate);\n> > mode_.maxLineLength = sensorInfo.maxLineLength * (1.0s / sensorInfo.pixelRate);\n> >\n> > + /*\n> > + * Ensure that the maximum pixel processing rate does not exceed the ISP\n> > + * hardware capabilities. If it does, try adjusting the minimum line\n> > + * length to compensate if possible.\n> > + */\n> > + Duration minPixelTime = controller_.getHardwareConfig().minPixelProcessingTime;\n> > + Duration pixelTime = mode_.minLineLength / mode_.width;\n> > + if (minPixelTime && pixelTime < minPixelTime) {\n> > + Duration adjustedLineLength = minPixelTime * mode_.width;\n> > + if (adjustedLineLength <= mode_.maxLineLength) {\n> > + LOG(IPARPI, Info)\n> > + << \"Adjusting mode minimum line length from \" << mode_.minLineLength\n> > + << \" to \" << adjustedLineLength << \" because of HW constraints.\";\n> > + mode_.minLineLength = adjustedLineLength;\n> > + } else {\n> > + LOG(IPARPI, Error)\n> > + << \"Sensor minimum line length of \" << pixelTime * mode_.width\n> > + << \" (\" << 1us / pixelTime << \" MPix/s)\"\n> > + << \" is below the minimum allowable HW limit of \" << minPixelTime * mode_.width\n> > + << \" (\" << 1us / minPixelTime << \" MPix/s) \";\n> > + LOG(IPARPI, Error)\n> > + << \"THIS WILL CAUSE IMAGE CORRUPTION!!! \"\n> > + << \"Please update the driver to allow more horizontal blanking control.\";\n> > + }\n> > + }\n> > +\n> > /*\n> > * Set the frame length limits for the mode to ensure exposure and\n> > * framerate calculations are clipped appropriately.\n> > diff --git a/src/ipa/rpi/controller/controller.cpp b/src/ipa/rpi/controller/controller.cpp\n> > index e62becd87e85..f81edf769736 100644\n> > --- a/src/ipa/rpi/controller/controller.cpp\n> > +++ b/src/ipa/rpi/controller/controller.cpp\n> > @@ -17,6 +17,7 @@\n> >\n> > using namespace RPiController;\n> > using namespace libcamera;\n> > +using namespace std::literals::chrono_literals;\n> >\n> > LOG_DEFINE_CATEGORY(RPiController)\n> >\n> > @@ -37,6 +38,7 @@ static const std::map HardwareConfigMap\n> > .numGammaPoints = 33,\n> > .pipelineWidth = 13,\n> > .statsInline = false,\n> > + .minPixelProcessingTime = 0s,\n> > }\n> > },\n> > {\n> > @@ -51,6 +53,7 @@ static const std::map HardwareConfigMap\n> > .numGammaPoints = 64,\n> > .pipelineWidth = 16,\n> > .statsInline = true,\n> > + .minPixelProcessingTime = 1.0us / 380, /* 380 MPix/s */\n> > }\n> > },\n> > };\n> > diff --git a/src/ipa/rpi/controller/controller.h b/src/ipa/rpi/controller/controller.h\n> > index 6e5f595284fd..170aea740789 100644\n> > --- a/src/ipa/rpi/controller/controller.h\n> > +++ b/src/ipa/rpi/controller/controller.h\n> > @@ -15,6 +15,7 @@\n> > #include \n> > #include \n> >\n> > +#include \n> > #include \"libcamera/internal/yaml_parser.h\"\n> >\n> > #include \"camera_mode.h\"\n> > @@ -47,6 +48,7 @@ public:\n> > unsigned int numGammaPoints;\n> > unsigned int pipelineWidth;\n> > bool statsInline;\n> > + libcamera::utils::Duration minPixelProcessingTime;\n> > };\n> >\n> > Controller();\n> > --\n> > 2.34.1\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 1CD52BE175\n\tfor ;\n\tWed, 3 Jan 2024 14:17:30 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5507F61D85;\n\tWed, 3 Jan 2024 15:17:29 +0100 (CET)","from mail-ej1-x62a.google.com (mail-ej1-x62a.google.com\n\t[IPv6:2a00:1450:4864:20::62a])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6CACB61D83\n\tfor ;\n\tWed, 3 Jan 2024 15:17:27 +0100 (CET)","by mail-ej1-x62a.google.com with SMTP id\n\ta640c23a62f3a-a2888d65f1fso93196466b.1\n\tfor ;\n\tWed, 03 Jan 2024 06:17:27 -0800 (PST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1704291449;\n\tbh=u5KK9ogO1YX3nkEqvfVW4x7dU3y25R1tiwfVHvS6GMg=;\n\th=References:In-Reply-To:Date:To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:\n\tFrom;\n\tb=zrrZL8r3tyV1EfNFHFG8jaJh6/kKvbTVg1+7e246uCESsAqbqG/FMcajjwthBs/Dm\n\tjwGku0adB9pm+alY4u2FwLkXWbKhKSEun3ppOivzubxB8vElwAvBwONqbbSimLSiSj\n\t3TOoXWuIUBuxNe8WEF6v11s5YMmW9PpbkscLgYO8lBTaZOqbCt57gM/V/x24UMhc5H\n\tGQaONj1bpBTKfQ5ZJKJeuXRri3bisG/Spuj5DUP8sGHpy+iv9JzgriVf1WKQl4pKbC\n\tlow/zLtS64MktwpCAxdnIXYYrU3cLm0ptBnbUkNWJNi73/+MENK3zV3z6yPyFHrJZJ\n\tjocmdWcKrYoQA==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1704291447; x=1704896247;\n\tdarn=lists.libcamera.org; \n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc:subject:date:message-id:reply-to;\n\tbh=0FneEnUVh7Lq5qcOeaxbmSSKwkGJAEuFz8yfTAg3RYk=;\n\tb=haTDOwCcffYPTGaFSMzi3eTbVVZ0yoal+Kgn2RKTHYyOEe5/Qvbkw5AHedglC9S9RY\n\tt1JYydUp2uM6hpeSY0pHd4PQi3iJhOx56acZDs/cjkLRnqjwhAjreWnfiqmvSXysOzUH\n\tbPxzHfKB5orFCLVf82eYtXpjhkagaF7IuELolxYXcF4AjTHokfClilXxKQDVfM2TGeBx\n\tZjHTuP40dcnp10omODZVIAkomG0MRgHlCN2G6DQ4AUCdcKtdyLZmGy43boZZpd+BDo08\n\tpCco1hWHHDJB5Ouel/4hUP8bZDR1saDE5HTC8Tveb5ocYir7ifzcF6+PixIH6lw1oNLY\n\tPMOg=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"haTDOwCc\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1704291447; x=1704896247;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-message-state:from:to:cc:subject:date:message-id\n\t:reply-to;\n\tbh=0FneEnUVh7Lq5qcOeaxbmSSKwkGJAEuFz8yfTAg3RYk=;\n\tb=oQgIo7zfoZjPQ9SdGJo16Zvq4tzjDP8z/htM9qHw9OgjrQEEflfolZrTpZ0ZSawG/I\n\tRKDnZLirlhgCMG2+z1PesH+2i6Avt5sk9rHqGen70N2O36dktVPCzVz7uc5u5F7vIMh0\n\t9bPyzTmASVLA6uDnbFXtKBjTAgJmXl/C+hWQFxjCgIlrGouvqzBni6WnbD54ZtV5hV9k\n\tdDAQEkbqlj/9isgEOX0lb5SMXtBH0eiqzMEhsOKxJMUj59LvuVquMSOpLPhsG6CXhQvx\n\t7XBrbfu/QzrKrl6YgYFwnLU9zKkojOjt8LIuLpDrUpyZ91dPMDX34iMH8AhAzZrgFnx9\n\trwWg==","X-Gm-Message-State":"AOJu0Yw0bdxgktMMozfIb9s2cyRS9XRQyUdld8MaqQcfbcOq6cyBQg+d\n\tCXzgxu1Lk5skYlkPfoyGUHBQV7pq1sQF//27QKulwIb33Zh6VGLVSXn6R9+FAby/zw==","X-Google-Smtp-Source":"AGHT+IEzm4gZtZU+9GO9wYyBxxyQN9DS7YIGpk8QK4/A5MNOLe0emDksQZr540D/aSrIvzVvvld29VN7Q5YmDDJE2/w=","X-Received":"by 2002:a17:907:2813:b0:a23:46f2:d8e4 with SMTP id\n\teb19-20020a170907281300b00a2346f2d8e4mr7780205ejc.51.1704291446676;\n\tWed, 03 Jan 2024 06:17:26 -0800 (PST)","MIME-Version":"1.0","References":"<20231219125749.14362-1-naush@raspberrypi.com>\n\t\n\t\n\t","In-Reply-To":"","Date":"Wed, 3 Jan 2024 14:17:15 +0000","Message-ID":"","To":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH] ipa: rpi: Add hardware line rate\n\tconstraints","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","From":"Nick Hollinghurst via libcamera-devel\n\t","Reply-To":"Nick Hollinghurst ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":28372,"web_url":"https://patchwork.libcamera.org/comment/28372/","msgid":"<20240103180413.GB17142@pendragon.ideasonboard.com>","date":"2024-01-03T18:04:13","subject":"Re: [libcamera-devel] [PATCH] ipa: rpi: Add hardware line rate\n\tconstraints","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Wed, Jan 03, 2024 at 02:17:15PM +0000, Nick Hollinghurst via libcamera-devel wrote:\n> On Tue, 2 Jan 2024 at 12:15, David Plowman wrote:\n> > On Tue, 19 Dec 2023 at 12:57, Naushir Patuck via libcamera-devel wrote:\n> > >\n> > > Advertise hardware constraints on the pixel processing rate through the\n> > > Controller::HardwareConfig structure. When calculating the minimum line\n> > > length during a configure() operation, ensure that we don't exceed this\n> > > constraint.\n> > >\n> > > If we do exceed the hardware constraints, increase the modes's minimum\n> > > line length so the pixel processing rate falls below the hardware limit.\n> > > If this is not possible, throw a loud error message in the logs.\n> > >\n> > > Signed-off-by: Naushir Patuck \n> >\n> > The change and code here seem reasonable to me. I guess the only minor\n> > concern is to understand exactly what the constraint really is. Is it\n> > just about the line time, or is there a constraint on some number of\n> > blanking pixels? Or maybe the latter is equivalent to the former by\n> > virtue of the fifos?\n> >\n> > So it's an \"OK\" from me, but we should probably ask Nick for chapter and verse!\n> >\n> > Reviewed-by: David Plowman \n> \n> LGTM\n> \n> The constraint is on the rate of pixels going from CSI2 peripheral to\n> ISP-FE (400Mpix/s, plus tiny overheads per scanline, for which\n> 380Mpix/s is a conservative bound).\n> \n> There is a 64kbit data FIFO before the bottleneck, which means that in\n> all reasonable cases the constraint applies at a timescale >= 1\n> scanline, so adding horizontal blanking can prevent loss.\n> \n> If the backlog were to grow beyond 64kbit during a single scanline,\n> there could still be loss. This could happen using 4 lanes at 1.5Gbps\n> at 10bpp with frames wider than ~16,000 pixels. We don't expect to\n> reach that with any known camera, so the change is good.\n\nThis is a useful explanation, could it be added to the comments in the\nsource code ?\n\n> Reviewed-by: Nick Hollinghurst \n> \n> > > ---\n> > > src/ipa/rpi/common/ipa_base.cpp | 26 ++++++++++++++++++++++++++\n> > > src/ipa/rpi/controller/controller.cpp | 3 +++\n> > > src/ipa/rpi/controller/controller.h | 2 ++\n> > > 3 files changed, 31 insertions(+)\n> > >\n> > > diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp\n> > > index 6ac9d5de2f88..419f19f120da 100644\n> > > --- a/src/ipa/rpi/common/ipa_base.cpp\n> > > +++ b/src/ipa/rpi/common/ipa_base.cpp\n> > > @@ -531,6 +531,32 @@ void IpaBase::setMode(const IPACameraSensorInfo &sensorInfo)\n> > > mode_.minLineLength = sensorInfo.minLineLength * (1.0s / sensorInfo.pixelRate);\n> > > mode_.maxLineLength = sensorInfo.maxLineLength * (1.0s / sensorInfo.pixelRate);\n> > >\n> > > + /*\n> > > + * Ensure that the maximum pixel processing rate does not exceed the ISP\n> > > + * hardware capabilities. If it does, try adjusting the minimum line\n> > > + * length to compensate if possible.\n> > > + */\n> > > + Duration minPixelTime = controller_.getHardwareConfig().minPixelProcessingTime;\n> > > + Duration pixelTime = mode_.minLineLength / mode_.width;\n> > > + if (minPixelTime && pixelTime < minPixelTime) {\n> > > + Duration adjustedLineLength = minPixelTime * mode_.width;\n> > > + if (adjustedLineLength <= mode_.maxLineLength) {\n> > > + LOG(IPARPI, Info)\n> > > + << \"Adjusting mode minimum line length from \" << mode_.minLineLength\n> > > + << \" to \" << adjustedLineLength << \" because of HW constraints.\";\n\nDoes this need to be an info message or would debug be enough ?\n\n> > > + mode_.minLineLength = adjustedLineLength;\n> > > + } else {\n> > > + LOG(IPARPI, Error)\n> > > + << \"Sensor minimum line length of \" << pixelTime * mode_.width\n> > > + << \" (\" << 1us / pixelTime << \" MPix/s)\"\n\nTechnically, as pixelTime is a time, 1us / pixelTime is a unit-less\nvalue. It could be written as '1 / pixelTime.get()', but\nadding the rouding, that would give\n\n\tstd::lround(1 / pixelTime.get())\n\nwhich I suppose isn't very nice to read. I'll stop being pedantic here\n:-)\n\n> > > + << \" is below the minimum allowable HW limit of \" << minPixelTime * mode_.width\n\nI would write \"ISP limit\" instead of \"HW limit\" to make it clear where\nthe constraint comes from.\n\nPlease add a line break before '<< minPixelTime'.\n\n> > > + << \" (\" << 1us / minPixelTime << \" MPix/s) \";\n> > > + LOG(IPARPI, Error)\n> > > + << \"THIS WILL CAUSE IMAGE CORRUPTION!!! \"\n> > > + << \"Please update the driver to allow more horizontal blanking control.\";\n\nSimilarly, \"Please update the camera sensor driver ...\".\n\nAs corruption is pretty much a given, should we error out ?\n\n> > > + }\n> > > + }\n> > > +\n> > > /*\n> > > * Set the frame length limits for the mode to ensure exposure and\n> > > * framerate calculations are clipped appropriately.\n> > > diff --git a/src/ipa/rpi/controller/controller.cpp b/src/ipa/rpi/controller/controller.cpp\n> > > index e62becd87e85..f81edf769736 100644\n> > > --- a/src/ipa/rpi/controller/controller.cpp\n> > > +++ b/src/ipa/rpi/controller/controller.cpp\n> > > @@ -17,6 +17,7 @@\n> > >\n> > > using namespace RPiController;\n> > > using namespace libcamera;\n> > > +using namespace std::literals::chrono_literals;\n> > >\n> > > LOG_DEFINE_CATEGORY(RPiController)\n> > >\n> > > @@ -37,6 +38,7 @@ static const std::map HardwareConfigMap\n> > > .numGammaPoints = 33,\n> > > .pipelineWidth = 13,\n> > > .statsInline = false,\n> > > + .minPixelProcessingTime = 0s,\n> > > }\n> > > },\n> > > {\n> > > @@ -51,6 +53,7 @@ static const std::map HardwareConfigMap\n> > > .numGammaPoints = 64,\n> > > .pipelineWidth = 16,\n> > > .statsInline = true,\n> > > + .minPixelProcessingTime = 1.0us / 380, /* 380 MPix/s */\n> > > }\n> > > },\n> > > };\n> > > diff --git a/src/ipa/rpi/controller/controller.h b/src/ipa/rpi/controller/controller.h\n> > > index 6e5f595284fd..170aea740789 100644\n> > > --- a/src/ipa/rpi/controller/controller.h\n> > > +++ b/src/ipa/rpi/controller/controller.h\n> > > @@ -15,6 +15,7 @@\n> > > #include \n> > > #include \n> > >\n> > > +#include \n> > > #include \"libcamera/internal/yaml_parser.h\"\n> > >\n> > > #include \"camera_mode.h\"\n> > > @@ -47,6 +48,7 @@ public:\n> > > unsigned int numGammaPoints;\n> > > unsigned int pipelineWidth;\n> > > bool statsInline;\n> > > + libcamera::utils::Duration minPixelProcessingTime;\n> > > };\n> > >\n> > > Controller();","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 BEB6BBDB1D\n\tfor ;\n\tWed, 3 Jan 2024 18:04:07 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E0E9161D85;\n\tWed, 3 Jan 2024 19:04: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 EEBF061D83\n\tfor ;\n\tWed, 3 Jan 2024 19:04:04 +0100 (CET)","from pendragon.ideasonboard.com (213-243-189-158.bb.dnainternet.fi\n\t[213.243.189.158])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 8906F3D5;\n\tWed, 3 Jan 2024 19:03:04 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1704305046;\n\tbh=zE2ST8D8iKqK1uH1uoyxAKvkfVLvKOvKFGbG0+mftIA=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=eeSZsGTSZNa5OYzom1g18NqyeO3AkTORALST0201qiMq2wKNCjuwrTcXZnM1JHxYD\n\ttOLQ+zbpfHudtoIzcVeCsIyk50drnbd5t9r1UPu4swZcGx2ScbWY7+xbXVKT0FEU/O\n\tiLM0aHIvREOOym6xT5jL/rEGPtr8RRZGcfu/VVr2JAT/uIq20ZYVLAWMp5AgaCkIWC\n\tZWfnL7MT4h/P3DQNI97XLOt2ulHyk/vR7hco4xxQaZYBqVfNBljZ1kwT988710oFcq\n\tX3k6TtRdUfDlX+DT2ATCMp3NFIExcHN2Fy+cS68OZd9+d57dou1pdts1ve8TvyMq7P\n\tGwmozemB9qlng==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1704304984;\n\tbh=zE2ST8D8iKqK1uH1uoyxAKvkfVLvKOvKFGbG0+mftIA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=h1E+Nvn7RjpUG3Ol+3/V1S28gMGU/+Y9QAeVhrEQDMtK6Tex3bzHs+qUB4pW5T/5J\n\tQ2KRlaHbknwGxDJRPKHdMxv0PbrzA1IN4kiU9umgT8txmPe08A1uf5d7qFNN2DaodN\n\tV3NDryo/4Om88ls32HeBOpEqriI17hN2QXl1Kwyo="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"h1E+Nvn7\"; dkim-atps=neutral","Date":"Wed, 3 Jan 2024 20:04:13 +0200","To":"Nick Hollinghurst ","Message-ID":"<20240103180413.GB17142@pendragon.ideasonboard.com>","References":"<20231219125749.14362-1-naush@raspberrypi.com>\n\t\n\t\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH] ipa: rpi: Add hardware line rate\n\tconstraints","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","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":28373,"web_url":"https://patchwork.libcamera.org/comment/28373/","msgid":"","date":"2024-01-04T08:28:10","subject":"Re: [libcamera-devel] [PATCH] ipa: rpi: Add hardware line rate\n\tconstraints","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Laurent,\n\nOn Wed, 3 Jan 2024 at 18:04, Laurent Pinchart via libcamera-devel\n wrote:\n>\n> On Wed, Jan 03, 2024 at 02:17:15PM +0000, Nick Hollinghurst via libcamera-devel wrote:\n> > On Tue, 2 Jan 2024 at 12:15, David Plowman wrote:\n> > > On Tue, 19 Dec 2023 at 12:57, Naushir Patuck via libcamera-devel wrote:\n> > > >\n> > > > Advertise hardware constraints on the pixel processing rate through the\n> > > > Controller::HardwareConfig structure. When calculating the minimum line\n> > > > length during a configure() operation, ensure that we don't exceed this\n> > > > constraint.\n> > > >\n> > > > If we do exceed the hardware constraints, increase the modes's minimum\n> > > > line length so the pixel processing rate falls below the hardware limit.\n> > > > If this is not possible, throw a loud error message in the logs.\n> > > >\n> > > > Signed-off-by: Naushir Patuck \n> > >\n> > > The change and code here seem reasonable to me. I guess the only minor\n> > > concern is to understand exactly what the constraint really is. Is it\n> > > just about the line time, or is there a constraint on some number of\n> > > blanking pixels? Or maybe the latter is equivalent to the former by\n> > > virtue of the fifos?\n> > >\n> > > So it's an \"OK\" from me, but we should probably ask Nick for chapter and verse!\n> > >\n> > > Reviewed-by: David Plowman \n> >\n> > LGTM\n> >\n> > The constraint is on the rate of pixels going from CSI2 peripheral to\n> > ISP-FE (400Mpix/s, plus tiny overheads per scanline, for which\n> > 380Mpix/s is a conservative bound).\n> >\n> > There is a 64kbit data FIFO before the bottleneck, which means that in\n> > all reasonable cases the constraint applies at a timescale >= 1\n> > scanline, so adding horizontal blanking can prevent loss.\n> >\n> > If the backlog were to grow beyond 64kbit during a single scanline,\n> > there could still be loss. This could happen using 4 lanes at 1.5Gbps\n> > at 10bpp with frames wider than ~16,000 pixels. We don't expect to\n> > reach that with any known camera, so the change is good.\n>\n> This is a useful explanation, could it be added to the comments in the\n> source code ?\n\nSure I can add that.\n\n>\n> > Reviewed-by: Nick Hollinghurst \n> >\n> > > > ---\n> > > > src/ipa/rpi/common/ipa_base.cpp | 26 ++++++++++++++++++++++++++\n> > > > src/ipa/rpi/controller/controller.cpp | 3 +++\n> > > > src/ipa/rpi/controller/controller.h | 2 ++\n> > > > 3 files changed, 31 insertions(+)\n> > > >\n> > > > diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp\n> > > > index 6ac9d5de2f88..419f19f120da 100644\n> > > > --- a/src/ipa/rpi/common/ipa_base.cpp\n> > > > +++ b/src/ipa/rpi/common/ipa_base.cpp\n> > > > @@ -531,6 +531,32 @@ void IpaBase::setMode(const IPACameraSensorInfo &sensorInfo)\n> > > > mode_.minLineLength = sensorInfo.minLineLength * (1.0s / sensorInfo.pixelRate);\n> > > > mode_.maxLineLength = sensorInfo.maxLineLength * (1.0s / sensorInfo.pixelRate);\n> > > >\n> > > > + /*\n> > > > + * Ensure that the maximum pixel processing rate does not exceed the ISP\n> > > > + * hardware capabilities. If it does, try adjusting the minimum line\n> > > > + * length to compensate if possible.\n> > > > + */\n> > > > + Duration minPixelTime = controller_.getHardwareConfig().minPixelProcessingTime;\n> > > > + Duration pixelTime = mode_.minLineLength / mode_.width;\n> > > > + if (minPixelTime && pixelTime < minPixelTime) {\n> > > > + Duration adjustedLineLength = minPixelTime * mode_.width;\n> > > > + if (adjustedLineLength <= mode_.maxLineLength) {\n> > > > + LOG(IPARPI, Info)\n> > > > + << \"Adjusting mode minimum line length from \" << mode_.minLineLength\n> > > > + << \" to \" << adjustedLineLength << \" because of HW constraints.\";\n>\n> Does this need to be an info message or would debug be enough ?\n\nI would prefer an info message personally so it shows up in the logs\nby default. This will help when users ask why their camera is not\nrunning as fast as they want.\n\n>\n> > > > + mode_.minLineLength = adjustedLineLength;\n> > > > + } else {\n> > > > + LOG(IPARPI, Error)\n> > > > + << \"Sensor minimum line length of \" << pixelTime * mode_.width\n> > > > + << \" (\" << 1us / pixelTime << \" MPix/s)\"\n>\n> Technically, as pixelTime is a time, 1us / pixelTime is a unit-less\n> value. It could be written as '1 / pixelTime.get()', but\n> adding the rouding, that would give\n>\n> std::lround(1 / pixelTime.get())\n>\n> which I suppose isn't very nice to read. I'll stop being pedantic here\n> :-)\n>\n> > > > + << \" is below the minimum allowable HW limit of \" << minPixelTime * mode_.width\n>\n> I would write \"ISP limit\" instead of \"HW limit\" to make it clear where\n> the constraint comes from.\n>\n> Please add a line break before '<< minPixelTime'.\n>\n> > > > + << \" (\" << 1us / minPixelTime << \" MPix/s) \";\n> > > > + LOG(IPARPI, Error)\n> > > > + << \"THIS WILL CAUSE IMAGE CORRUPTION!!! \"\n> > > > + << \"Please update the driver to allow more horizontal blanking control.\";\n>\n> Similarly, \"Please update the camera sensor driver ...\".\n>\n> As corruption is pretty much a given, should we error out ?\n\nMy preference would be not to, and allow the user to continue running.\nThe reason is there are mitigations we might ask users to try out\n(e.g. upclocking RP1) in certain scenarios. Having this not fail will\nallow the user to try such things without having to recompile\nlibcamera (but of course, the error message will still be displayed).\n\nRegards,\nNaush\n\n>\n> > > > + }\n> > > > + }\n> > > > +\n> > > > /*\n> > > > * Set the frame length limits for the mode to ensure exposure and\n> > > > * framerate calculations are clipped appropriately.\n> > > > diff --git a/src/ipa/rpi/controller/controller.cpp b/src/ipa/rpi/controller/controller.cpp\n> > > > index e62becd87e85..f81edf769736 100644\n> > > > --- a/src/ipa/rpi/controller/controller.cpp\n> > > > +++ b/src/ipa/rpi/controller/controller.cpp\n> > > > @@ -17,6 +17,7 @@\n> > > >\n> > > > using namespace RPiController;\n> > > > using namespace libcamera;\n> > > > +using namespace std::literals::chrono_literals;\n> > > >\n> > > > LOG_DEFINE_CATEGORY(RPiController)\n> > > >\n> > > > @@ -37,6 +38,7 @@ static const std::map HardwareConfigMap\n> > > > .numGammaPoints = 33,\n> > > > .pipelineWidth = 13,\n> > > > .statsInline = false,\n> > > > + .minPixelProcessingTime = 0s,\n> > > > }\n> > > > },\n> > > > {\n> > > > @@ -51,6 +53,7 @@ static const std::map HardwareConfigMap\n> > > > .numGammaPoints = 64,\n> > > > .pipelineWidth = 16,\n> > > > .statsInline = true,\n> > > > + .minPixelProcessingTime = 1.0us / 380, /* 380 MPix/s */\n> > > > }\n> > > > },\n> > > > };\n> > > > diff --git a/src/ipa/rpi/controller/controller.h b/src/ipa/rpi/controller/controller.h\n> > > > index 6e5f595284fd..170aea740789 100644\n> > > > --- a/src/ipa/rpi/controller/controller.h\n> > > > +++ b/src/ipa/rpi/controller/controller.h\n> > > > @@ -15,6 +15,7 @@\n> > > > #include \n> > > > #include \n> > > >\n> > > > +#include \n> > > > #include \"libcamera/internal/yaml_parser.h\"\n> > > >\n> > > > #include \"camera_mode.h\"\n> > > > @@ -47,6 +48,7 @@ public:\n> > > > unsigned int numGammaPoints;\n> > > > unsigned int pipelineWidth;\n> > > > bool statsInline;\n> > > > + libcamera::utils::Duration minPixelProcessingTime;\n> > > > };\n> > > >\n> > > > Controller();\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","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 453CEC323E\n\tfor ;\n\tThu, 4 Jan 2024 08:28:51 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4F70A61D80;\n\tThu, 4 Jan 2024 09:28:50 +0100 (CET)","from mail-yb1-xb35.google.com (mail-yb1-xb35.google.com\n\t[IPv6:2607:f8b0:4864:20::b35])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2113361D80\n\tfor ;\n\tThu, 4 Jan 2024 09:28:48 +0100 (CET)","by mail-yb1-xb35.google.com with SMTP id\n\t3f1490d57ef6-db3a09e96daso233694276.3\n\tfor ;\n\tThu, 04 Jan 2024 00:28:48 -0800 (PST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1704356930;\n\tbh=QAwIQcChwUzhdEYPAs/U98R34BiylR9yE1pCrxzyE1E=;\n\th=References:In-Reply-To:Date:To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=T+fM7BkGXvbQPz+cURhMoiAEHkpXsQg7DqznRH5pXZwGiQXp971iy4SP3iqkR+O+t\n\tChrcEB4wF0mqrCjJ07FLwlxu4v3/uVyDPRMs+EYzwAWcFVo3Mi8B8ab4E60M6rqPMm\n\tlmt1eLrTsOMgeC4e3TzkOPHYotZNKY/yltwxezpqyv2ObwcT5TD5Z05FOtgyi6VdPQ\n\t3G6IAfwQ3+/vUKhdz7MMeFO0jotinPogUg5ZirEnQkoReRoA3BsEhTXqtmAZOji0CS\n\t+8CVF31oWujR1MCh4AxIWO6MntL8MoDChiFen6o25fLPCZDHpMZlNtUbIPtpBzh/kx\n\t0srr7KaY/1hIw==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1704356927; x=1704961727;\n\tdarn=lists.libcamera.org; \n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc:subject:date:message-id:reply-to;\n\tbh=zrisVGXV1LWxZYBjgbds8qPyMrkhL7iXUqtNtRAXg+g=;\n\tb=fMDknG5GfPniNWfD5cxZ7qC2u0NldA0SkCvWJpldzvNCUx4vhV3NRwn3o7lGFDRiWv\n\tYrHYpRhLCjzhehOgrK8yuy64/Cskg7Vizw27i42nEepptCnM9b1nx+bFUqb9EhGlZcrZ\n\tfMOpEG6BmeUqHyuFqPHMQ8/qitwB9iZ9oGg6HSbNlNhFVHTPMRqthaLGyq/oFv0hlZZR\n\tfHDdhYSD/Sf4eknjr47JCmc6mWhcMq40g+DgUfZYhV73YhKoi7N9VCWuQyVCB72tT8Ew\n\tqq2lwpG0nNIJIzRdFJ4K01fZE4NB6bdK6/c0w1KHNJrzdyBvyFvOWsj2zsu5XYJZ+AqH\n\tB9pg=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"fMDknG5G\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1704356927; x=1704961727;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-message-state:from:to:cc:subject:date:message-id\n\t:reply-to;\n\tbh=zrisVGXV1LWxZYBjgbds8qPyMrkhL7iXUqtNtRAXg+g=;\n\tb=RmwN+hD0kSnYTFxCTaGm/08xVQ1MUo28Cayp9bcWDOjCHtLBfIFzpOaGKFZtBZWSWz\n\tWoC/jT/15XefyUqdX0OYa2kJVxmdIv5njeYL8UoD0hpFiNbAC9kFe8/8suCbaSyeYteV\n\tz2+CKRBV3ooeftRxHXwwDUDYqqfz3rzH/fWLyGzrQdJIZr8RB/FrDTvP3QEP2BbG7c0Q\n\tc39COJwH4YMS1xOhEB8arldkoAoXbMYo4PfymzzUjDMFQtk1WuR8Djk1KS9qhnPAfQ8U\n\t6Sqfuje3DKWst3hWtWIJ/XO//kF8Bu+W3nbvkdw5pungpNdqGOGwNWq69hwqObyjhfT0\n\tmBRA==","X-Gm-Message-State":"AOJu0Yyum/cHdP+2XFsbcDTrO5NEIBAznYPtPu2WJzesVcdpHscIwELr\n\tihebQVbRv4atUBtej7d1uQEclRREDglOKp8Plh/CcfbryhnhmA==","X-Google-Smtp-Source":"AGHT+IF2Nnkshz/CHWnBl8T5pIVZzDBoM4hYQirZgNP1tbO3cEFKMTkYzV4NDvyoC/g+vsp4OqSusuewEVD1fqlI5Xg=","X-Received":"by 2002:a25:ad57:0:b0:dbe:8bcd:42ae with SMTP id\n\tl23-20020a25ad57000000b00dbe8bcd42aemr184992ybe.16.1704356926632;\n\tThu, 04 Jan 2024 00:28:46 -0800 (PST)","MIME-Version":"1.0","References":"<20231219125749.14362-1-naush@raspberrypi.com>\n\t\n\t\n\t\n\t\n\t<20240103180413.GB17142@pendragon.ideasonboard.com>","In-Reply-To":"<20240103180413.GB17142@pendragon.ideasonboard.com>","Date":"Thu, 4 Jan 2024 08:28:10 +0000","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH] ipa: rpi: Add hardware line rate\n\tconstraints","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","From":"Naushir Patuck via libcamera-devel\n\t","Reply-To":"Naushir Patuck ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":28374,"web_url":"https://patchwork.libcamera.org/comment/28374/","msgid":"<20240104222027.GA14342@pendragon.ideasonboard.com>","date":"2024-01-04T22:20:27","subject":"Re: [libcamera-devel] [PATCH] ipa: rpi: Add hardware line rate\n\tconstraints","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Thu, Jan 04, 2024 at 08:28:10AM +0000, Naushir Patuck wrote:\n> On Wed, 3 Jan 2024 at 18:04, Laurent Pinchart via libcamera-devel wrote:\n> > On Wed, Jan 03, 2024 at 02:17:15PM +0000, Nick Hollinghurst via libcamera-devel wrote:\n> > > On Tue, 2 Jan 2024 at 12:15, David Plowman wrote:\n> > > > On Tue, 19 Dec 2023 at 12:57, Naushir Patuck via libcamera-devel wrote:\n> > > > >\n> > > > > Advertise hardware constraints on the pixel processing rate through the\n> > > > > Controller::HardwareConfig structure. When calculating the minimum line\n> > > > > length during a configure() operation, ensure that we don't exceed this\n> > > > > constraint.\n> > > > >\n> > > > > If we do exceed the hardware constraints, increase the modes's minimum\n> > > > > line length so the pixel processing rate falls below the hardware limit.\n> > > > > If this is not possible, throw a loud error message in the logs.\n> > > > >\n> > > > > Signed-off-by: Naushir Patuck \n> > > >\n> > > > The change and code here seem reasonable to me. I guess the only minor\n> > > > concern is to understand exactly what the constraint really is. Is it\n> > > > just about the line time, or is there a constraint on some number of\n> > > > blanking pixels? Or maybe the latter is equivalent to the former by\n> > > > virtue of the fifos?\n> > > >\n> > > > So it's an \"OK\" from me, but we should probably ask Nick for chapter and verse!\n> > > >\n> > > > Reviewed-by: David Plowman \n> > >\n> > > LGTM\n> > >\n> > > The constraint is on the rate of pixels going from CSI2 peripheral to\n> > > ISP-FE (400Mpix/s, plus tiny overheads per scanline, for which\n> > > 380Mpix/s is a conservative bound).\n> > >\n> > > There is a 64kbit data FIFO before the bottleneck, which means that in\n> > > all reasonable cases the constraint applies at a timescale >= 1\n> > > scanline, so adding horizontal blanking can prevent loss.\n> > >\n> > > If the backlog were to grow beyond 64kbit during a single scanline,\n> > > there could still be loss. This could happen using 4 lanes at 1.5Gbps\n> > > at 10bpp with frames wider than ~16,000 pixels. We don't expect to\n> > > reach that with any known camera, so the change is good.\n> >\n> > This is a useful explanation, could it be added to the comments in the\n> > source code ?\n> \n> Sure I can add that.\n> \n> > > Reviewed-by: Nick Hollinghurst \n> > >\n> > > > > ---\n> > > > > src/ipa/rpi/common/ipa_base.cpp | 26 ++++++++++++++++++++++++++\n> > > > > src/ipa/rpi/controller/controller.cpp | 3 +++\n> > > > > src/ipa/rpi/controller/controller.h | 2 ++\n> > > > > 3 files changed, 31 insertions(+)\n> > > > >\n> > > > > diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp\n> > > > > index 6ac9d5de2f88..419f19f120da 100644\n> > > > > --- a/src/ipa/rpi/common/ipa_base.cpp\n> > > > > +++ b/src/ipa/rpi/common/ipa_base.cpp\n> > > > > @@ -531,6 +531,32 @@ void IpaBase::setMode(const IPACameraSensorInfo &sensorInfo)\n> > > > > mode_.minLineLength = sensorInfo.minLineLength * (1.0s / sensorInfo.pixelRate);\n> > > > > mode_.maxLineLength = sensorInfo.maxLineLength * (1.0s / sensorInfo.pixelRate);\n> > > > >\n> > > > > + /*\n> > > > > + * Ensure that the maximum pixel processing rate does not exceed the ISP\n> > > > > + * hardware capabilities. If it does, try adjusting the minimum line\n> > > > > + * length to compensate if possible.\n> > > > > + */\n> > > > > + Duration minPixelTime = controller_.getHardwareConfig().minPixelProcessingTime;\n> > > > > + Duration pixelTime = mode_.minLineLength / mode_.width;\n> > > > > + if (minPixelTime && pixelTime < minPixelTime) {\n> > > > > + Duration adjustedLineLength = minPixelTime * mode_.width;\n> > > > > + if (adjustedLineLength <= mode_.maxLineLength) {\n> > > > > + LOG(IPARPI, Info)\n> > > > > + << \"Adjusting mode minimum line length from \" << mode_.minLineLength\n> > > > > + << \" to \" << adjustedLineLength << \" because of HW constraints.\";\n> >\n> > Does this need to be an info message or would debug be enough ?\n> \n> I would prefer an info message personally so it shows up in the logs\n> by default. This will help when users ask why their camera is not\n> running as fast as they want.\n\nOK.\n\n> > > > > + mode_.minLineLength = adjustedLineLength;\n> > > > > + } else {\n> > > > > + LOG(IPARPI, Error)\n> > > > > + << \"Sensor minimum line length of \" << pixelTime * mode_.width\n> > > > > + << \" (\" << 1us / pixelTime << \" MPix/s)\"\n> >\n> > Technically, as pixelTime is a time, 1us / pixelTime is a unit-less\n> > value. It could be written as '1 / pixelTime.get()', but\n> > adding the rouding, that would give\n> >\n> > std::lround(1 / pixelTime.get())\n> >\n> > which I suppose isn't very nice to read. I'll stop being pedantic here\n> > :-)\n> >\n> > > > > + << \" is below the minimum allowable HW limit of \" << minPixelTime * mode_.width\n> >\n> > I would write \"ISP limit\" instead of \"HW limit\" to make it clear where\n> > the constraint comes from.\n> >\n> > Please add a line break before '<< minPixelTime'.\n> >\n> > > > > + << \" (\" << 1us / minPixelTime << \" MPix/s) \";\n> > > > > + LOG(IPARPI, Error)\n> > > > > + << \"THIS WILL CAUSE IMAGE CORRUPTION!!! \"\n> > > > > + << \"Please update the driver to allow more horizontal blanking control.\";\n> >\n> > Similarly, \"Please update the camera sensor driver ...\".\n> >\n> > As corruption is pretty much a given, should we error out ?\n> \n> My preference would be not to, and allow the user to continue running.\n> The reason is there are mitigations we might ask users to try out\n> (e.g. upclocking RP1) in certain scenarios. Having this not fail will\n> allow the user to try such things without having to recompile\n> libcamera (but of course, the error message will still be displayed).\n\nGood point.\n\nBy the way, have you considered a solution where the upclocking would be\ndone automatically ?\n\n> > > > > + }\n> > > > > + }\n> > > > > +\n> > > > > /*\n> > > > > * Set the frame length limits for the mode to ensure exposure and\n> > > > > * framerate calculations are clipped appropriately.\n> > > > > diff --git a/src/ipa/rpi/controller/controller.cpp b/src/ipa/rpi/controller/controller.cpp\n> > > > > index e62becd87e85..f81edf769736 100644\n> > > > > --- a/src/ipa/rpi/controller/controller.cpp\n> > > > > +++ b/src/ipa/rpi/controller/controller.cpp\n> > > > > @@ -17,6 +17,7 @@\n> > > > >\n> > > > > using namespace RPiController;\n> > > > > using namespace libcamera;\n> > > > > +using namespace std::literals::chrono_literals;\n> > > > >\n> > > > > LOG_DEFINE_CATEGORY(RPiController)\n> > > > >\n> > > > > @@ -37,6 +38,7 @@ static const std::map HardwareConfigMap\n> > > > > .numGammaPoints = 33,\n> > > > > .pipelineWidth = 13,\n> > > > > .statsInline = false,\n> > > > > + .minPixelProcessingTime = 0s,\n> > > > > }\n> > > > > },\n> > > > > {\n> > > > > @@ -51,6 +53,7 @@ static const std::map HardwareConfigMap\n> > > > > .numGammaPoints = 64,\n> > > > > .pipelineWidth = 16,\n> > > > > .statsInline = true,\n> > > > > + .minPixelProcessingTime = 1.0us / 380, /* 380 MPix/s */\n> > > > > }\n> > > > > },\n> > > > > };\n> > > > > diff --git a/src/ipa/rpi/controller/controller.h b/src/ipa/rpi/controller/controller.h\n> > > > > index 6e5f595284fd..170aea740789 100644\n> > > > > --- a/src/ipa/rpi/controller/controller.h\n> > > > > +++ b/src/ipa/rpi/controller/controller.h\n> > > > > @@ -15,6 +15,7 @@\n> > > > > #include \n> > > > > #include \n> > > > >\n> > > > > +#include \n> > > > > #include \"libcamera/internal/yaml_parser.h\"\n> > > > >\n> > > > > #include \"camera_mode.h\"\n> > > > > @@ -47,6 +48,7 @@ public:\n> > > > > unsigned int numGammaPoints;\n> > > > > unsigned int pipelineWidth;\n> > > > > bool statsInline;\n> > > > > + libcamera::utils::Duration minPixelProcessingTime;\n> > > > > };\n> > > > >\n> > > > > Controller();","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 17EE7BDB1D\n\tfor ;\n\tThu, 4 Jan 2024 22:20:22 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6C4BA61D82;\n\tThu, 4 Jan 2024 23:20:21 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8B8A661D82\n\tfor ;\n\tThu, 4 Jan 2024 23:20:19 +0100 (CET)","from pendragon.ideasonboard.com (213-243-189-158.bb.dnainternet.fi\n\t[213.243.189.158])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 647F5B3;\n\tThu, 4 Jan 2024 23:19:18 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1704406821;\n\tbh=B+lvlDEKKrwO2aBV2swGlVR5CWZLUsN4Tbtuy+cxKns=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=iYFsDd1sOQs9U1DuSVoIu0xZdhYFB0pQn5hCMeDZqkmSGdkJ0/8KrcofcnOPuoOva\n\taxVQJgTN78Lm4h5MD56uSx+CDqrgWD57y0PRunQt+XIoEuWFPIkL8zqmpPLbv8WUXK\n\tT7Kel8kIW3JC7pJdx1nPr2EJtf63c+3E5y03qiG2VJlCOG4Bttj0EaEPH3pM3lCTGr\n\tgWzgeO3NDp7E4T8x0mxlR5vW96Tz+cd7NVhorhCzsfd7nUGeVKkOAAteHIp9MhUTxM\n\tyH2sbLgvr1HEAwTBKfNCqpl9MHAuVnWj0gjLjewRAW3MHBFJNoyO1alfoXGjcrBCD0\n\tQZFThEmsJpabg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1704406758;\n\tbh=B+lvlDEKKrwO2aBV2swGlVR5CWZLUsN4Tbtuy+cxKns=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=NpQeM66Pt2F+EaV9j/c/msUO/V6d8S05Hnc6gleTstECCwl4Afq3KTTrmiwkVFM66\n\tjUn8GBLqFb9D3MiKPUWYgLGM1KF9FT36VG0QKpixGDlk8ia9RWI28fElnURD1I038Z\n\tb6iaFBYjWD0hpRZyWzcr5cxhjBIx1aFhTnlOxJ24="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"NpQeM66P\"; dkim-atps=neutral","Date":"Fri, 5 Jan 2024 00:20:27 +0200","To":"Naushir Patuck ","Message-ID":"<20240104222027.GA14342@pendragon.ideasonboard.com>","References":"<20231219125749.14362-1-naush@raspberrypi.com>\n\t\n\t\n\t\n\t\n\t<20240103180413.GB17142@pendragon.ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH] ipa: rpi: Add hardware line rate\n\tconstraints","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","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":28377,"web_url":"https://patchwork.libcamera.org/comment/28377/","msgid":"","date":"2024-01-05T08:30:16","subject":"Re: [libcamera-devel] [PATCH] ipa: rpi: Add hardware line rate\n\tconstraints","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Laurent,\n\nOn Thu, 4 Jan 2024 at 22:20, Laurent Pinchart\n wrote:\n>\n> On Thu, Jan 04, 2024 at 08:28:10AM +0000, Naushir Patuck wrote:\n> > On Wed, 3 Jan 2024 at 18:04, Laurent Pinchart via libcamera-devel wrote:\n> > > On Wed, Jan 03, 2024 at 02:17:15PM +0000, Nick Hollinghurst via libcamera-devel wrote:\n> > > > On Tue, 2 Jan 2024 at 12:15, David Plowman wrote:\n> > > > > On Tue, 19 Dec 2023 at 12:57, Naushir Patuck via libcamera-devel wrote:\n> > > > > >\n> > > > > > Advertise hardware constraints on the pixel processing rate through the\n> > > > > > Controller::HardwareConfig structure. When calculating the minimum line\n> > > > > > length during a configure() operation, ensure that we don't exceed this\n> > > > > > constraint.\n> > > > > >\n> > > > > > If we do exceed the hardware constraints, increase the modes's minimum\n> > > > > > line length so the pixel processing rate falls below the hardware limit.\n> > > > > > If this is not possible, throw a loud error message in the logs.\n> > > > > >\n> > > > > > Signed-off-by: Naushir Patuck \n> > > > >\n> > > > > The change and code here seem reasonable to me. I guess the only minor\n> > > > > concern is to understand exactly what the constraint really is. Is it\n> > > > > just about the line time, or is there a constraint on some number of\n> > > > > blanking pixels? Or maybe the latter is equivalent to the former by\n> > > > > virtue of the fifos?\n> > > > >\n> > > > > So it's an \"OK\" from me, but we should probably ask Nick for chapter and verse!\n> > > > >\n> > > > > Reviewed-by: David Plowman \n> > > >\n> > > > LGTM\n> > > >\n> > > > The constraint is on the rate of pixels going from CSI2 peripheral to\n> > > > ISP-FE (400Mpix/s, plus tiny overheads per scanline, for which\n> > > > 380Mpix/s is a conservative bound).\n> > > >\n> > > > There is a 64kbit data FIFO before the bottleneck, which means that in\n> > > > all reasonable cases the constraint applies at a timescale >= 1\n> > > > scanline, so adding horizontal blanking can prevent loss.\n> > > >\n> > > > If the backlog were to grow beyond 64kbit during a single scanline,\n> > > > there could still be loss. This could happen using 4 lanes at 1.5Gbps\n> > > > at 10bpp with frames wider than ~16,000 pixels. We don't expect to\n> > > > reach that with any known camera, so the change is good.\n> > >\n> > > This is a useful explanation, could it be added to the comments in the\n> > > source code ?\n> >\n> > Sure I can add that.\n> >\n> > > > Reviewed-by: Nick Hollinghurst \n> > > >\n> > > > > > ---\n> > > > > > src/ipa/rpi/common/ipa_base.cpp | 26 ++++++++++++++++++++++++++\n> > > > > > src/ipa/rpi/controller/controller.cpp | 3 +++\n> > > > > > src/ipa/rpi/controller/controller.h | 2 ++\n> > > > > > 3 files changed, 31 insertions(+)\n> > > > > >\n> > > > > > diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp\n> > > > > > index 6ac9d5de2f88..419f19f120da 100644\n> > > > > > --- a/src/ipa/rpi/common/ipa_base.cpp\n> > > > > > +++ b/src/ipa/rpi/common/ipa_base.cpp\n> > > > > > @@ -531,6 +531,32 @@ void IpaBase::setMode(const IPACameraSensorInfo &sensorInfo)\n> > > > > > mode_.minLineLength = sensorInfo.minLineLength * (1.0s / sensorInfo.pixelRate);\n> > > > > > mode_.maxLineLength = sensorInfo.maxLineLength * (1.0s / sensorInfo.pixelRate);\n> > > > > >\n> > > > > > + /*\n> > > > > > + * Ensure that the maximum pixel processing rate does not exceed the ISP\n> > > > > > + * hardware capabilities. If it does, try adjusting the minimum line\n> > > > > > + * length to compensate if possible.\n> > > > > > + */\n> > > > > > + Duration minPixelTime = controller_.getHardwareConfig().minPixelProcessingTime;\n> > > > > > + Duration pixelTime = mode_.minLineLength / mode_.width;\n> > > > > > + if (minPixelTime && pixelTime < minPixelTime) {\n> > > > > > + Duration adjustedLineLength = minPixelTime * mode_.width;\n> > > > > > + if (adjustedLineLength <= mode_.maxLineLength) {\n> > > > > > + LOG(IPARPI, Info)\n> > > > > > + << \"Adjusting mode minimum line length from \" << mode_.minLineLength\n> > > > > > + << \" to \" << adjustedLineLength << \" because of HW constraints.\";\n> > >\n> > > Does this need to be an info message or would debug be enough ?\n> >\n> > I would prefer an info message personally so it shows up in the logs\n> > by default. This will help when users ask why their camera is not\n> > running as fast as they want.\n>\n> OK.\n>\n> > > > > > + mode_.minLineLength = adjustedLineLength;\n> > > > > > + } else {\n> > > > > > + LOG(IPARPI, Error)\n> > > > > > + << \"Sensor minimum line length of \" << pixelTime * mode_.width\n> > > > > > + << \" (\" << 1us / pixelTime << \" MPix/s)\"\n> > >\n> > > Technically, as pixelTime is a time, 1us / pixelTime is a unit-less\n> > > value. It could be written as '1 / pixelTime.get()', but\n> > > adding the rouding, that would give\n> > >\n> > > std::lround(1 / pixelTime.get())\n> > >\n> > > which I suppose isn't very nice to read. I'll stop being pedantic here\n> > > :-)\n> > >\n> > > > > > + << \" is below the minimum allowable HW limit of \" << minPixelTime * mode_.width\n> > >\n> > > I would write \"ISP limit\" instead of \"HW limit\" to make it clear where\n> > > the constraint comes from.\n> > >\n> > > Please add a line break before '<< minPixelTime'.\n> > >\n> > > > > > + << \" (\" << 1us / minPixelTime << \" MPix/s) \";\n> > > > > > + LOG(IPARPI, Error)\n> > > > > > + << \"THIS WILL CAUSE IMAGE CORRUPTION!!! \"\n> > > > > > + << \"Please update the driver to allow more horizontal blanking control.\";\n> > >\n> > > Similarly, \"Please update the camera sensor driver ...\".\n> > >\n> > > As corruption is pretty much a given, should we error out ?\n> >\n> > My preference would be not to, and allow the user to continue running.\n> > The reason is there are mitigations we might ask users to try out\n> > (e.g. upclocking RP1) in certain scenarios. Having this not fail will\n> > allow the user to try such things without having to recompile\n> > libcamera (but of course, the error message will still be displayed).\n>\n> Good point.\n>\n> By the way, have you considered a solution where the upclocking would be\n> done automatically ?\n\nThat's the goal, but a difficult one I fear. We need to do quite a\nbit of qualification for the higher clock rates as the clock tree\nstructure spans over many blocks on RP1.\n\nRegards,\nNaush\n\n>\n> > > > > > + }\n> > > > > > + }\n> > > > > > +\n> > > > > > /*\n> > > > > > * Set the frame length limits for the mode to ensure exposure and\n> > > > > > * framerate calculations are clipped appropriately.\n> > > > > > diff --git a/src/ipa/rpi/controller/controller.cpp b/src/ipa/rpi/controller/controller.cpp\n> > > > > > index e62becd87e85..f81edf769736 100644\n> > > > > > --- a/src/ipa/rpi/controller/controller.cpp\n> > > > > > +++ b/src/ipa/rpi/controller/controller.cpp\n> > > > > > @@ -17,6 +17,7 @@\n> > > > > >\n> > > > > > using namespace RPiController;\n> > > > > > using namespace libcamera;\n> > > > > > +using namespace std::literals::chrono_literals;\n> > > > > >\n> > > > > > LOG_DEFINE_CATEGORY(RPiController)\n> > > > > >\n> > > > > > @@ -37,6 +38,7 @@ static const std::map HardwareConfigMap\n> > > > > > .numGammaPoints = 33,\n> > > > > > .pipelineWidth = 13,\n> > > > > > .statsInline = false,\n> > > > > > + .minPixelProcessingTime = 0s,\n> > > > > > }\n> > > > > > },\n> > > > > > {\n> > > > > > @@ -51,6 +53,7 @@ static const std::map HardwareConfigMap\n> > > > > > .numGammaPoints = 64,\n> > > > > > .pipelineWidth = 16,\n> > > > > > .statsInline = true,\n> > > > > > + .minPixelProcessingTime = 1.0us / 380, /* 380 MPix/s */\n> > > > > > }\n> > > > > > },\n> > > > > > };\n> > > > > > diff --git a/src/ipa/rpi/controller/controller.h b/src/ipa/rpi/controller/controller.h\n> > > > > > index 6e5f595284fd..170aea740789 100644\n> > > > > > --- a/src/ipa/rpi/controller/controller.h\n> > > > > > +++ b/src/ipa/rpi/controller/controller.h\n> > > > > > @@ -15,6 +15,7 @@\n> > > > > > #include \n> > > > > > #include \n> > > > > >\n> > > > > > +#include \n> > > > > > #include \"libcamera/internal/yaml_parser.h\"\n> > > > > >\n> > > > > > #include \"camera_mode.h\"\n> > > > > > @@ -47,6 +48,7 @@ public:\n> > > > > > unsigned int numGammaPoints;\n> > > > > > unsigned int pipelineWidth;\n> > > > > > bool statsInline;\n> > > > > > + libcamera::utils::Duration minPixelProcessingTime;\n> > > > > > };\n> > > > > >\n> > > > > > Controller();\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","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 E9BA7C323E\n\tfor ;\n\tFri, 5 Jan 2024 08:30:55 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5F9B262B41;\n\tFri, 5 Jan 2024 09:30:55 +0100 (CET)","from mail-yb1-xb2a.google.com (mail-yb1-xb2a.google.com\n\t[IPv6:2607:f8b0:4864:20::b2a])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A261B61D7F\n\tfor ;\n\tFri, 5 Jan 2024 09:30:53 +0100 (CET)","by mail-yb1-xb2a.google.com with SMTP id\n\t3f1490d57ef6-dafe04717baso1133886276.1\n\tfor ;\n\tFri, 05 Jan 2024 00:30:53 -0800 (PST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1704443455;\n\tbh=knGipkjKLdmDVCswK/Yk/k2xCeJRzvnT7Y98TvGFg1U=;\n\th=References:In-Reply-To:Date:To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=QIQr++KOX1Hzd53wS2fMRGy0zUlOI+u5D2ZnvnC6l7STwAzIJgor9fl8QQPVdHvxH\n\tpw1dhyYeA8ds0EcL3MJQK2rqmyN9CujC1wqCcHqNXrgrIYMiSKT98dF+ndN5bs3V+m\n\t/L2pB4OXmD5gytstXBNcd/blU2vhwXqA2fdFMaDOq7cYg39oLBnvKHHOECRuD1FZjc\n\ty72AgwWzwjK74c7DOIxUfxgASzbMVGls59gpSO94dR3+GLLRtRIYcrsLLJuSAzdYkD\n\tdEBqhbzCAyy7ZW4mf5WljMI4Z6QZhoi5d2zrfO8J13vX6n4yr0PGPJ7jvrNWaxbmpx\n\twctARsmtE8XXg==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1704443452; x=1705048252;\n\tdarn=lists.libcamera.org; \n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc:subject:date:message-id:reply-to;\n\tbh=TsRb+/6lGnlEL0hukg6vsn4CkIPdshEMgegogoa3t6g=;\n\tb=YtJ5XVHe0FfvPXb82ykFVlEQxjI8ONPRF53ltHLhHudY2+TCRUbWpGtbVYzfcvr5cS\n\t1JjlnHbRAMwz+9uu95vKpvLRxM3xSZhKZPxTUf1ji8nVUNJJ7cOmzflCQ233Txw/0HGo\n\tvAbKOQ+MWo8O2SkkfxacBEMpRpnPkxULibNFcGdvYN/EP5HUhEaZj3zEi2uEevDdFY95\n\tpE/FCWkmLiwWNeUJ/8mEPPGZD5wD9e6Mp6mUZuQL53fSAmcTSnj6N+19ymcp6MAsre61\n\tPK81QElUfqcp66SvJLzqXfPzPf7uqz5LiOF2KjohD6wY8DuVSWy7Nh6kVbkkqowpnMds\n\tM83w=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"YtJ5XVHe\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1704443452; x=1705048252;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-message-state:from:to:cc:subject:date:message-id\n\t:reply-to;\n\tbh=TsRb+/6lGnlEL0hukg6vsn4CkIPdshEMgegogoa3t6g=;\n\tb=GdYN7iCAytJKEM45RJcWovW4l6pMkh81ssuNb4MwxLmF3/Lus6vNtFF516fnkTB9IU\n\t0ADp303lTM8j8lGqxSQUUjVkFBb+UGrdTXSHfQtkJwH7ytfYs5LXmO6rTRlwkBq1EFXB\n\tzfyFNk4KoGrMXjtAn7rMcKKYwukKqCvT9F3uku3r5yZjpysalB+fIsdE7W4w3q0P5ege\n\twihMg70nw19SF6zZRe2JhDSbBHdOs3lXUmjv1mgd1Ctn6LD0VyLkQPbe/j+BwNe6R2a+\n\tDygaqa8OLOyV5U+VmJQYwgyeqd1dp35OaCtNCMH2mM0EDK41Wpeyz8me3dA0UTBlfPrc\n\tiDBQ==","X-Gm-Message-State":"AOJu0YwkAewU685DJkznMOQFCdAvGMG3GfpDU86XJ0q6Qw1G2lGS6O+2\n\tU8quhksXziMylCCqddFIvyIJvFTpaDtMF8NECOTxZO0+C2l/wA==","X-Google-Smtp-Source":"AGHT+IEisWFnMLH3ETn5FGqTep++Hr1BumyELHuHcr8CP4DGli1dIT23ldo6dEvx7CfNLFkO9PT9D0yzKYzxIlvcH5E=","X-Received":"by 2002:a5b:85:0:b0:db7:8141:e65b with SMTP id\n\tb5-20020a5b0085000000b00db78141e65bmr1539206ybp.64.1704443452326; \n\tFri, 05 Jan 2024 00:30:52 -0800 (PST)","MIME-Version":"1.0","References":"<20231219125749.14362-1-naush@raspberrypi.com>\n\t\n\t\n\t\n\t\n\t<20240103180413.GB17142@pendragon.ideasonboard.com>\n\t\n\t<20240104222027.GA14342@pendragon.ideasonboard.com>","In-Reply-To":"<20240104222027.GA14342@pendragon.ideasonboard.com>","Date":"Fri, 5 Jan 2024 08:30:16 +0000","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH] ipa: rpi: Add hardware line rate\n\tconstraints","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","From":"Naushir Patuck via libcamera-devel\n\t","Reply-To":"Naushir Patuck ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]