[{"id":25265,"web_url":"https://patchwork.libcamera.org/comment/25265/","msgid":"","date":"2022-10-04T16:40:20","subject":"Re: [libcamera-devel] [PATCH v1 1/9] camera_sensor: Add minimum and\n\tmaximum line length to IPACameraSensorInfo","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Naush,\n\nThank you for the patch.\n\nOn Mon, Oct 03, 2022 at 09:39:27AM +0100, Naushir Patuck via libcamera-devel wrote:\n> Add fields for minimum and maximum line length (in units of pixels) to the\n> IPACameraSensorInfo structure. This replaces the existing lineLength field.\n> \n> Update the ipu3, raspberrypi and rkisp1 IPAs to use IPACameraSensorInfo::minLineLength\n> instead of IPACameraSensorInfo::lineLength, as logically we will always want to\n> use the fastest sensor readout by default.\n> \n> Signed-off-by: Naushir Patuck \n> ---\n> include/libcamera/ipa/core.mojom | 21 +++++++++++++++------\n> src/ipa/ipu3/ipu3.cpp | 2 +-\n> src/ipa/raspberrypi/raspberrypi.cpp | 2 +-\n> src/ipa/rkisp1/rkisp1.cpp | 2 +-\n> src/libcamera/camera_sensor.cpp | 6 ++++--\n\nThis causes a compilation failure in src/ipa/ipu3/ipu3.cpp due to a\nchange that has been merged recently. It's easy to fix with\n\ndiff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\nindex d66965e7d94e..c89f76c56ee3 100644\n--- a/src/ipa/ipu3/ipu3.cpp\n+++ b/src/ipa/ipu3/ipu3.cpp\n@@ -499,7 +500,8 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo,\n \tcontext_.frameContexts.clear();\n\n \t/* Initialise the sensor configuration. */\n-\tcontext_.configuration.sensor.lineDuration = sensorInfo_.lineLength * 1.0s / sensorInfo_.pixelRate;\n+\tcontext_.configuration.sensor.lineDuration = sensorInfo_.minLineLength\n+\t\t\t\t\t\t * 1.0s / sensorInfo_.pixelRate;\n\n \t/*\n \t * Compute the sensor V4L2 controls to be used by the algorithms and\n\nIf that's the only change needed in this series, I'll fix it myself,\notherwise you can include this in v2.\n\n> 5 files changed, 22 insertions(+), 11 deletions(-)\n> \n> diff --git a/include/libcamera/ipa/core.mojom b/include/libcamera/ipa/core.mojom\n> index 74f3339e56f2..2bc3028c22d6 100644\n> --- a/include/libcamera/ipa/core.mojom\n> +++ b/include/libcamera/ipa/core.mojom\n> @@ -172,10 +172,17 @@ module libcamera;\n> */\n> \n> /**\n> - * \\var IPACameraSensorInfo::lineLength\n> - * \\brief Total line length in pixels\n> + * \\var IPACameraSensorInfo::minLineLength\n> + * \\brief The minimum line length in pixels\n> *\n> - * The total line length in pixel clock periods, including blanking.\n> + * The minimum allowable line length in pixel clock periods, including blanking.\n> + */\n> +\n> +/**\n> + * \\var IPACameraSensorInfo::maxLineLength\n> + * \\brief The maximum line length in pixels\n> + *\n> + * The maximum allowable line length in pixel clock periods, including blanking.\n> */\n> \n> /**\n> @@ -189,7 +196,7 @@ module libcamera;\n> * To obtain the minimum frame duration:\n> *\n> * \\verbatim\n> - frameDuration(s) = minFrameLength(lines) * lineLength(pixels) / pixelRate(pixels per second)\n> + frameDuration(s) = minFrameLength(lines) * minLineLength(pixels) / pixelRate(pixels per second)\n> \\endverbatim\n> */\n> \n> @@ -204,7 +211,7 @@ module libcamera;\n> * To obtain the maximum frame duration:\n> *\n> * \\verbatim\n> - frameDuration(s) = maxFrameLength(lines) * lineLength(pixels) / pixelRate(pixels per second)\n> + frameDuration(s) = maxFrameLength(lines) * maxLineLength(pixels) / pixelRate(pixels per second)\n> \\endverbatim\n> */\n> struct IPACameraSensorInfo {\n> @@ -217,7 +224,9 @@ struct IPACameraSensorInfo {\n> \tSize outputSize;\n> \n> \tuint64 pixelRate;\n> -\tuint32 lineLength;\n> +\n> +\tuint32 minLineLength;\n> +\tuint32 maxLineLength;\n> \n> \tuint32 minFrameLength;\n> \tuint32 maxFrameLength;\n> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> index d1ea081d595d..da029571ba55 100644\n> --- a/src/ipa/ipu3/ipu3.cpp\n> +++ b/src/ipa/ipu3/ipu3.cpp\n> @@ -341,7 +341,7 @@ int IPAIPU3::init(const IPASettings &settings,\n> \n> \t/* Clean context */\n> \tcontext_.configuration = {};\n> -\tcontext_.configuration.sensor.lineDuration = sensorInfo.lineLength * 1.0s / sensorInfo.pixelRate;\n> +\tcontext_.configuration.sensor.lineDuration = sensorInfo.minLineLength * 1.0s / sensorInfo.pixelRate;\n\nThis is getting long, let's wrap it.\n\n\tcontext_.configuration.sensor.lineDuration = sensorInfo.minLineLength\n\t\t\t\t\t\t * 1.0s / sensorInfo.pixelRate;\n\n(I can also apply this change myself if no v2 is otherwise needed.)\n\n> \n> \t/* Load the tuning data file. */\n> \tFile file(settings.configurationFile);\n> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> index 8d731435764e..358a119da222 100644\n> --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> @@ -356,7 +356,7 @@ void IPARPi::setMode(const IPACameraSensorInfo &sensorInfo)\n> \t * Calculate the line length as the ratio between the line length in\n> \t * pixels and the pixel rate.\n> \t */\n> -\tmode_.lineLength = sensorInfo.lineLength * (1.0s / sensorInfo.pixelRate);\n> +\tmode_.lineLength = sensorInfo.minLineLength * (1.0s / sensorInfo.pixelRate);\n> \n> \t/*\n> \t * Set the frame length limits for the mode to ensure exposure and\n> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> index 32feb1682749..ddb22d98eb41 100644\n> --- a/src/ipa/rkisp1/rkisp1.cpp\n> +++ b/src/ipa/rkisp1/rkisp1.cpp\n> @@ -246,7 +246,7 @@ int IPARkISP1::configure([[maybe_unused]] const IPACameraSensorInfo &info,\n> \tcontext_.configuration.hw.revision = hwRevision_;\n> \n> \tcontext_.configuration.sensor.size = info.outputSize;\n> -\tcontext_.configuration.sensor.lineDuration = info.lineLength * 1.0s / info.pixelRate;\n> +\tcontext_.configuration.sensor.lineDuration = info.minLineLength * 1.0s / info.pixelRate;\n> \n> \t/*\n> \t * When the AGC computes the new exposure values for a frame, it needs\n> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> index 911fd0beae4e..c2c8d3d44e26 100644\n> --- a/src/libcamera/camera_sensor.cpp\n> +++ b/src/libcamera/camera_sensor.cpp\n> @@ -883,10 +883,12 @@ int CameraSensor::sensorInfo(IPACameraSensorInfo *info) const\n> \t\treturn -EINVAL;\n> \t}\n> \n> -\tint32_t hblank = ctrls.get(V4L2_CID_HBLANK).get();\n> -\tinfo->lineLength = info->outputSize.width + hblank;\n> \tinfo->pixelRate = ctrls.get(V4L2_CID_PIXEL_RATE).get();\n> \n> +\tconst ControlInfo hblank = ctrls.infoMap()->at(V4L2_CID_HBLANK);\n> +\tinfo->minLineLength = info->outputSize.width + hblank.min().get();\n> +\tinfo->maxLineLength = info->outputSize.width + hblank.max().get();\n\nHmmm... If the current value of V4L2_CID_HBLANK is different than the\nminimum, won't all the IPA modules compute an incorrect line length ?\nI'm sure this is fixed for the Raspberry Pi IPA module in subsequent\npatches, but the IPU3 and RkISP1 will likely be affected.\n\n> +\n> \tconst ControlInfo vblank = ctrls.infoMap()->at(V4L2_CID_VBLANK);\n> \tinfo->minFrameLength = info->outputSize.height + vblank.min().get();\n> \tinfo->maxFrameLength = info->outputSize.height + vblank.max().get();","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 D6F11C0DA4\n\tfor ;\n\tTue, 4 Oct 2022 16:40:26 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4353660AAC;\n\tTue, 4 Oct 2022 18:40:26 +0200 (CEST)","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 1DD4F601C7\n\tfor ;\n\tTue, 4 Oct 2022 18:40:24 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 756752D9;\n\tTue, 4 Oct 2022 18:40:23 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1664901626;\n\tbh=ad10kFUJZ87473YqnSwkwTJ8ovDKcmIRv5VXOk68xGw=;\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=WWrH1KbjI83GXjo06DAgZ6s0O+Lw2esE7sOai4wny9D3fTSdfZ/RuxR7GnlEs+hi8\n\t5icG7kf9gCryfmADNsDhfoIh9NGP5dqy5/k6dA8v1ZnY6CThR2aJ3npW3X+uNhOJxs\n\tELqXUkZDQo8VeNhKThkxW5aR/IwyL6p46zOEmG/PbSl0uvewb3Pskz2vieWLCTDVVo\n\tSCLge9neMc0TVWvKcmFFq2KwuKonOl3pP+VwI4azNCZV5gJq2dOxi5NZ9mQi9llDRm\n\tIW2P/bZoTwLpAkqYIiNrU+WesfOjcVmukOgnG4mEefNTiLbo1Z/bJ/u/nI6flE1oJK\n\ttLFO70+u5+jPg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1664901623;\n\tbh=ad10kFUJZ87473YqnSwkwTJ8ovDKcmIRv5VXOk68xGw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=K84MR8j1OKQInmHLIHCOqc4g6ymrndSThHKB0neqvp3zpdKLrjyzKCTUqI/4kCBue\n\tmadVMj1xJAmpWZcHMwfHkgljGW4bzL+oC2WNFt059eSEseq2t4fq5f/3ckJ6aYznd6\n\taHG5cNbtV9zkoMmOuFBib1834DY1/I6uEtSw+e0c="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"K84MR8j1\"; dkim-atps=neutral","Date":"Tue, 4 Oct 2022 19:40:20 +0300","To":"Naushir Patuck ","Message-ID":"","References":"<20221003083934.31629-1-naush@raspberrypi.com>\n\t<20221003083934.31629-2-naush@raspberrypi.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20221003083934.31629-2-naush@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH v1 1/9] camera_sensor: Add minimum and\n\tmaximum line length to IPACameraSensorInfo","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":25304,"web_url":"https://patchwork.libcamera.org/comment/25304/","msgid":"","date":"2022-10-05T12:01:41","subject":"Re: [libcamera-devel] [PATCH v1 1/9] camera_sensor: Add minimum and\n\tmaximum line length to IPACameraSensorInfo","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Laurent,\n\nThank you for your feedback.\n\nOn Tue, 4 Oct 2022 at 17:40, Laurent Pinchart <\nlaurent.pinchart@ideasonboard.com> wrote:\n\n> Hi Naush,\n>\n> Thank you for the patch.\n>\n> On Mon, Oct 03, 2022 at 09:39:27AM +0100, Naushir Patuck via\n> libcamera-devel wrote:\n> > Add fields for minimum and maximum line length (in units of pixels) to\n> the\n> > IPACameraSensorInfo structure. This replaces the existing lineLength\n> field.\n> >\n> > Update the ipu3, raspberrypi and rkisp1 IPAs to use\n> IPACameraSensorInfo::minLineLength\n> > instead of IPACameraSensorInfo::lineLength, as logically we will always\n> want to\n> > use the fastest sensor readout by default.\n> >\n> > Signed-off-by: Naushir Patuck \n> > ---\n> > include/libcamera/ipa/core.mojom | 21 +++++++++++++++------\n> > src/ipa/ipu3/ipu3.cpp | 2 +-\n> > src/ipa/raspberrypi/raspberrypi.cpp | 2 +-\n> > src/ipa/rkisp1/rkisp1.cpp | 2 +-\n> > src/libcamera/camera_sensor.cpp | 6 ++++--\n>\n> This causes a compilation failure in src/ipa/ipu3/ipu3.cpp due to a\n> change that has been merged recently. It's easy to fix with\n>\n> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> index d66965e7d94e..c89f76c56ee3 100644\n> --- a/src/ipa/ipu3/ipu3.cpp\n> +++ b/src/ipa/ipu3/ipu3.cpp\n> @@ -499,7 +500,8 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo,\n> context_.frameContexts.clear();\n>\n> /* Initialise the sensor configuration. */\n> - context_.configuration.sensor.lineDuration =\n> sensorInfo_.lineLength * 1.0s / sensorInfo_.pixelRate;\n> + context_.configuration.sensor.lineDuration =\n> sensorInfo_.minLineLength\n> + * 1.0s /\n> sensorInfo_.pixelRate;\n>\n> /*\n> * Compute the sensor V4L2 controls to be used by the algorithms\n> and\n>\n> If that's the only change needed in this series, I'll fix it myself,\n> otherwise you can include this in v2.\n>\n\nI'll fix this up in the next revision.\n\n\n> > 5 files changed, 22 insertions(+), 11 deletions(-)\n> >\n> > diff --git a/include/libcamera/ipa/core.mojom\n> b/include/libcamera/ipa/core.mojom\n> > index 74f3339e56f2..2bc3028c22d6 100644\n> > --- a/include/libcamera/ipa/core.mojom\n> > +++ b/include/libcamera/ipa/core.mojom\n> > @@ -172,10 +172,17 @@ module libcamera;\n> > */\n> >\n> > /**\n> > - * \\var IPACameraSensorInfo::lineLength\n> > - * \\brief Total line length in pixels\n> > + * \\var IPACameraSensorInfo::minLineLength\n> > + * \\brief The minimum line length in pixels\n> > *\n> > - * The total line length in pixel clock periods, including blanking.\n> > + * The minimum allowable line length in pixel clock periods, including\n> blanking.\n> > + */\n> > +\n> > +/**\n> > + * \\var IPACameraSensorInfo::maxLineLength\n> > + * \\brief The maximum line length in pixels\n> > + *\n> > + * The maximum allowable line length in pixel clock periods, including\n> blanking.\n> > */\n> >\n> > /**\n> > @@ -189,7 +196,7 @@ module libcamera;\n> > * To obtain the minimum frame duration:\n> > *\n> > * \\verbatim\n> > - frameDuration(s) = minFrameLength(lines) * lineLength(pixels) /\n> pixelRate(pixels per second)\n> > + frameDuration(s) = minFrameLength(lines) * minLineLength(pixels)\n> / pixelRate(pixels per second)\n> > \\endverbatim\n> > */\n> >\n> > @@ -204,7 +211,7 @@ module libcamera;\n> > * To obtain the maximum frame duration:\n> > *\n> > * \\verbatim\n> > - frameDuration(s) = maxFrameLength(lines) * lineLength(pixels) /\n> pixelRate(pixels per second)\n> > + frameDuration(s) = maxFrameLength(lines) * maxLineLength(pixels)\n> / pixelRate(pixels per second)\n> > \\endverbatim\n> > */\n> > struct IPACameraSensorInfo {\n> > @@ -217,7 +224,9 @@ struct IPACameraSensorInfo {\n> > Size outputSize;\n> >\n> > uint64 pixelRate;\n> > - uint32 lineLength;\n> > +\n> > + uint32 minLineLength;\n> > + uint32 maxLineLength;\n> >\n> > uint32 minFrameLength;\n> > uint32 maxFrameLength;\n> > diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> > index d1ea081d595d..da029571ba55 100644\n> > --- a/src/ipa/ipu3/ipu3.cpp\n> > +++ b/src/ipa/ipu3/ipu3.cpp\n> > @@ -341,7 +341,7 @@ int IPAIPU3::init(const IPASettings &settings,\n> >\n> > /* Clean context */\n> > context_.configuration = {};\n> > - context_.configuration.sensor.lineDuration = sensorInfo.lineLength\n> * 1.0s / sensorInfo.pixelRate;\n> > + context_.configuration.sensor.lineDuration =\n> sensorInfo.minLineLength * 1.0s / sensorInfo.pixelRate;\n>\n> This is getting long, let's wrap it.\n>\n> context_.configuration.sensor.lineDuration =\n> sensorInfo.minLineLength\n> * 1.0s /\n> sensorInfo.pixelRate;\n>\n> (I can also apply this change myself if no v2 is otherwise needed.)\n>\n> >\n> > /* Load the tuning data file. */\n> > File file(settings.configurationFile);\n> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp\n> b/src/ipa/raspberrypi/raspberrypi.cpp\n> > index 8d731435764e..358a119da222 100644\n> > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> > @@ -356,7 +356,7 @@ void IPARPi::setMode(const IPACameraSensorInfo\n> &sensorInfo)\n> > * Calculate the line length as the ratio between the line length\n> in\n> > * pixels and the pixel rate.\n> > */\n> > - mode_.lineLength = sensorInfo.lineLength * (1.0s /\n> sensorInfo.pixelRate);\n> > + mode_.lineLength = sensorInfo.minLineLength * (1.0s /\n> sensorInfo.pixelRate);\n> >\n> > /*\n> > * Set the frame length limits for the mode to ensure exposure and\n> > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> > index 32feb1682749..ddb22d98eb41 100644\n> > --- a/src/ipa/rkisp1/rkisp1.cpp\n> > +++ b/src/ipa/rkisp1/rkisp1.cpp\n> > @@ -246,7 +246,7 @@ int IPARkISP1::configure([[maybe_unused]] const\n> IPACameraSensorInfo &info,\n> > context_.configuration.hw.revision = hwRevision_;\n> >\n> > context_.configuration.sensor.size = info.outputSize;\n> > - context_.configuration.sensor.lineDuration = info.lineLength *\n> 1.0s / info.pixelRate;\n> > + context_.configuration.sensor.lineDuration = info.minLineLength *\n> 1.0s / info.pixelRate;\n> >\n> > /*\n> > * When the AGC computes the new exposure values for a frame, it\n> needs\n> > diff --git a/src/libcamera/camera_sensor.cpp\n> b/src/libcamera/camera_sensor.cpp\n> > index 911fd0beae4e..c2c8d3d44e26 100644\n> > --- a/src/libcamera/camera_sensor.cpp\n> > +++ b/src/libcamera/camera_sensor.cpp\n> > @@ -883,10 +883,12 @@ int CameraSensor::sensorInfo(IPACameraSensorInfo\n> *info) const\n> > return -EINVAL;\n> > }\n> >\n> > - int32_t hblank = ctrls.get(V4L2_CID_HBLANK).get();\n> > - info->lineLength = info->outputSize.width + hblank;\n> > info->pixelRate = ctrls.get(V4L2_CID_PIXEL_RATE).get();\n> >\n> > + const ControlInfo hblank = ctrls.infoMap()->at(V4L2_CID_HBLANK);\n> > + info->minLineLength = info->outputSize.width +\n> hblank.min().get();\n> > + info->maxLineLength = info->outputSize.width +\n> hblank.max().get();\n>\n> Hmmm... If the current value of V4L2_CID_HBLANK is different than the\n> minimum, won't all the IPA modules compute an incorrect line length ?\n> I'm sure this is fixed for the Raspberry Pi IPA module in subsequent\n> patches, but the IPU3 and RkISP1 will likely be affected.\n>\n\nThis is true - I am making the (perhaps incorrect) assumption that sensors\nthat\ndo not dynamically control hblank will have the control value set to the\nminimum.\nI made the change here so that the min/max LineDuration fields in\nIPACameraSensorInfo\nmatches with what min/max FrameLength conveys.\n\nI can make one of two changes to fix this:\n\n1) Revert the above code so that IPACameraSensorInfo continues to report\nlineLength as before. I will then have to update our RPi mode setup so that\nwe essentially do the above in our own mode structure.\n\n2) Keep the above change, and update rkisp1/ipu3/rpi IPAs to calculate line\nlength\ndirectly from the control value instead of from the fields in\nIPACameraSensorInfo.\n\nFor consistency, I would prefer (2), but happy to do either.\n\nRegards,\nNaush\n\n\n>\n> > +\n> > const ControlInfo vblank = ctrls.infoMap()->at(V4L2_CID_VBLANK);\n> > info->minFrameLength = info->outputSize.height +\n> vblank.min().get();\n> > info->maxFrameLength = info->outputSize.height +\n> vblank.max().get();\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\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 B76BCBD16B\n\tfor ;\n\tWed, 5 Oct 2022 12:01:59 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0A66A622F3;\n\tWed, 5 Oct 2022 14:01:59 +0200 (CEST)","from mail-lf1-x136.google.com (mail-lf1-x136.google.com\n\t[IPv6:2a00:1450:4864:20::136])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7B57F601C7\n\tfor ;\n\tWed, 5 Oct 2022 14:01:57 +0200 (CEST)","by mail-lf1-x136.google.com with SMTP id a29so2128355lfo.1\n\tfor ;\n\tWed, 05 Oct 2022 05:01:57 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1664971319;\n\tbh=fbsAeB+DlHu9OHLUgl36Zya6yVlekrll68Qs8D49NPo=;\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=0Sfk+iM5hWtqSrTqVAaSuwS+Aw/jUG/VwR4+Un2Lm2tqM6RiRXD/juEveqJwj30Yb\n\tp88Cn70z5+OW740q0SjMKAOusiZ+H5OJuHGfI2RkUo9qeHYudjyuIHoQ77ocSAZNUD\n\t9uY2qfZ211GC3AwXklR+3uy83aqjoksIdJdihtfC0DHRnrirdc30RXGA94mN7Xtkwv\n\tgKft0hkM03x+cAPyjwkT0huKE+pATtnQS2P1va3UYvrhSD8wFhltoKVzJdDuwTw/0s\n\txMXRigKUj9HR4n+CBcx2ohYG0aQ1kxuiWHvn1H6dPoQMH8xpYjoJ054Z5hhIzh4P7T\n\t+nYqoZqKvVwXQ==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc:subject:date;\n\tbh=Y7dD3h6McxvribmIM6FtC2DRN4p8uJZSR4A62GvuibY=;\n\tb=t3qZ8gtlPG5sggl6OqNhZ/XM/ngPQ8hBs87agw9JpQPZovlBV8rovExNGQBUUHnxX8\n\t2oUj5qJkbrGi6fVoe1P3UYf+qKA/WOqOkVFcw5a2BXazd8li60qmdm3VKR4amRkoM/S1\n\tzfj9+uaHMsNVtdRuKFhyvDbMQ70Q6vXG6h1WTbHRwf1hRH2xQxIckqx+5LpZqcyweGmA\n\tc/haVr2LrHISJoo36X4pm6Y7L4hbRbtN3HE06J7sNekZdJsFE0a03AfBVBCzbl0Fe/2h\n\tOyxR6WABVlbyMkYEKhRjf0FZu7Jzj2Ot736rbMejGRn7/TT8TKDWEjdXuA39hVe7DqGv\n\tqkfQ=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"t3qZ8gtl\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\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;\n\tbh=Y7dD3h6McxvribmIM6FtC2DRN4p8uJZSR4A62GvuibY=;\n\tb=bPFQK6Am3q6BKmeP4Hj9I2vlSm+P+ACZkFuJwZJ3sKbeIrKXnoyTASo02H4YgrQkDI\n\t2gZ4bSBv/EsUZby38ah602IDcSqVglQxoexlcwA1dFr9A6C+xoe2Fb18sd5q0bSDgd+r\n\tlsTdWMfQEep6gBr+WXiqV5hRDcpME9hIBk3sco4I8H4UZEWfnyJGarthqBYQ1p0d6JQp\n\tly8JfVzqRn/k0QOaGEfyOuJOgZsE3nng8YM5qeNvrS/UxZL/JQ7P6z0eqKYxtVCjMWd8\n\tbS2u6rwwa+H304sRV5S7bCJmREgz5iwBrszjaLAQgWAefJItU2uS9A2wEfkfgYsH8ssO\n\twtQg==","X-Gm-Message-State":"ACrzQf21bgHiEpavKAlcW5j9HAmfdEvq2zM6TVrsICbMJ+oLm+CepH3p\n\tT2Ih0PknU1kau9k1GCZx+cGZEVCUON39+bCNtZBOb+/2C06TUQ==","X-Google-Smtp-Source":"AMsMyM5d1kRX58e1OkwCQpSP8fOci+5uVRgedpiiP/ay237UI260J2DQoQVftt9sBcjLBMaSnzF/Aar7MTqWoPVQU7A=","X-Received":"by 2002:a05:6512:2616:b0:4a2:1723:cf40 with SMTP id\n\tbt22-20020a056512261600b004a21723cf40mr8286550lfb.354.1664971316837;\n\tWed, 05 Oct 2022 05:01:56 -0700 (PDT)","MIME-Version":"1.0","References":"<20221003083934.31629-1-naush@raspberrypi.com>\n\t<20221003083934.31629-2-naush@raspberrypi.com>\n\t","In-Reply-To":"","Date":"Wed, 5 Oct 2022 13:01:41 +0100","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"multipart/alternative; boundary=\"000000000000e6d5f205ea485758\"","Subject":"Re: [libcamera-devel] [PATCH v1 1/9] camera_sensor: Add minimum and\n\tmaximum line length to IPACameraSensorInfo","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":25307,"web_url":"https://patchwork.libcamera.org/comment/25307/","msgid":"","date":"2022-10-05T12:39:28","subject":"Re: [libcamera-devel] [PATCH v1 1/9] camera_sensor: Add minimum and\n\tmaximum line length to IPACameraSensorInfo","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Naush,\n\nOn Wed, Oct 05, 2022 at 01:01:41PM +0100, Naushir Patuck wrote:\n> On Tue, 4 Oct 2022 at 17:40, Laurent Pinchart wrote:\n> > On Mon, Oct 03, 2022 at 09:39:27AM +0100, Naushir Patuck via\n> > libcamera-devel wrote:\n> > > Add fields for minimum and maximum line length (in units of pixels) to the\n> > > IPACameraSensorInfo structure. This replaces the existing lineLength field.\n> > >\n> > > Update the ipu3, raspberrypi and rkisp1 IPAs to use IPACameraSensorInfo::minLineLength\n> > > instead of IPACameraSensorInfo::lineLength, as logically we will always want to\n> > > use the fastest sensor readout by default.\n> > >\n> > > Signed-off-by: Naushir Patuck \n> > > ---\n> > > include/libcamera/ipa/core.mojom | 21 +++++++++++++++------\n> > > src/ipa/ipu3/ipu3.cpp | 2 +-\n> > > src/ipa/raspberrypi/raspberrypi.cpp | 2 +-\n> > > src/ipa/rkisp1/rkisp1.cpp | 2 +-\n> > > src/libcamera/camera_sensor.cpp | 6 ++++--\n> >\n> > This causes a compilation failure in src/ipa/ipu3/ipu3.cpp due to a\n> > change that has been merged recently. It's easy to fix with\n> >\n> > diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> > index d66965e7d94e..c89f76c56ee3 100644\n> > --- a/src/ipa/ipu3/ipu3.cpp\n> > +++ b/src/ipa/ipu3/ipu3.cpp\n> > @@ -499,7 +500,8 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo,\n> > context_.frameContexts.clear();\n> >\n> > /* Initialise the sensor configuration. */\n> > - context_.configuration.sensor.lineDuration = sensorInfo_.lineLength * 1.0s / sensorInfo_.pixelRate;\n> > + context_.configuration.sensor.lineDuration = sensorInfo_.minLineLength\n> > + * 1.0s / sensorInfo_.pixelRate;\n> >\n> > /*\n> > * Compute the sensor V4L2 controls to be used by the algorithms and\n> >\n> > If that's the only change needed in this series, I'll fix it myself,\n> > otherwise you can include this in v2.\n> \n> I'll fix this up in the next revision.\n> \n> > > 5 files changed, 22 insertions(+), 11 deletions(-)\n> > >\n> > > diff --git a/include/libcamera/ipa/core.mojom b/include/libcamera/ipa/core.mojom\n> > > index 74f3339e56f2..2bc3028c22d6 100644\n> > > --- a/include/libcamera/ipa/core.mojom\n> > > +++ b/include/libcamera/ipa/core.mojom\n> > > @@ -172,10 +172,17 @@ module libcamera;\n> > > */\n> > >\n> > > /**\n> > > - * \\var IPACameraSensorInfo::lineLength\n> > > - * \\brief Total line length in pixels\n> > > + * \\var IPACameraSensorInfo::minLineLength\n> > > + * \\brief The minimum line length in pixels\n> > > *\n> > > - * The total line length in pixel clock periods, including blanking.\n> > > + * The minimum allowable line length in pixel clock periods, including blanking.\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\var IPACameraSensorInfo::maxLineLength\n> > > + * \\brief The maximum line length in pixels\n> > > + *\n> > > + * The maximum allowable line length in pixel clock periods, including blanking.\n> > > */\n> > >\n> > > /**\n> > > @@ -189,7 +196,7 @@ module libcamera;\n> > > * To obtain the minimum frame duration:\n> > > *\n> > > * \\verbatim\n> > > - frameDuration(s) = minFrameLength(lines) * lineLength(pixels) / pixelRate(pixels per second)\n> > > + frameDuration(s) = minFrameLength(lines) * minLineLength(pixels) / pixelRate(pixels per second)\n> > > \\endverbatim\n> > > */\n> > >\n> > > @@ -204,7 +211,7 @@ module libcamera;\n> > > * To obtain the maximum frame duration:\n> > > *\n> > > * \\verbatim\n> > > - frameDuration(s) = maxFrameLength(lines) * lineLength(pixels) / pixelRate(pixels per second)\n> > > + frameDuration(s) = maxFrameLength(lines) * maxLineLength(pixels) / pixelRate(pixels per second)\n> > > \\endverbatim\n> > > */\n> > > struct IPACameraSensorInfo {\n> > > @@ -217,7 +224,9 @@ struct IPACameraSensorInfo {\n> > > Size outputSize;\n> > >\n> > > uint64 pixelRate;\n> > > - uint32 lineLength;\n> > > +\n> > > + uint32 minLineLength;\n> > > + uint32 maxLineLength;\n> > >\n> > > uint32 minFrameLength;\n> > > uint32 maxFrameLength;\n> > > diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> > > index d1ea081d595d..da029571ba55 100644\n> > > --- a/src/ipa/ipu3/ipu3.cpp\n> > > +++ b/src/ipa/ipu3/ipu3.cpp\n> > > @@ -341,7 +341,7 @@ int IPAIPU3::init(const IPASettings &settings,\n> > >\n> > > /* Clean context */\n> > > context_.configuration = {};\n> > > - context_.configuration.sensor.lineDuration = sensorInfo.lineLength * 1.0s / sensorInfo.pixelRate;\n> > > + context_.configuration.sensor.lineDuration = sensorInfo.minLineLength * 1.0s / sensorInfo.pixelRate;\n> >\n> > This is getting long, let's wrap it.\n> >\n> > context_.configuration.sensor.lineDuration = sensorInfo.minLineLength\n> > * 1.0s / sensorInfo.pixelRate;\n> >\n> > (I can also apply this change myself if no v2 is otherwise needed.)\n> >\n> > >\n> > > /* Load the tuning data file. */\n> > > File file(settings.configurationFile);\n> > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> > > index 8d731435764e..358a119da222 100644\n> > > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> > > @@ -356,7 +356,7 @@ void IPARPi::setMode(const IPACameraSensorInfo &sensorInfo)\n> > > * Calculate the line length as the ratio between the line length in\n> > > * pixels and the pixel rate.\n> > > */\n> > > - mode_.lineLength = sensorInfo.lineLength * (1.0s / sensorInfo.pixelRate);\n> > > + mode_.lineLength = sensorInfo.minLineLength * (1.0s / sensorInfo.pixelRate);\n> > >\n> > > /*\n> > > * Set the frame length limits for the mode to ensure exposure and\n> > > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> > > index 32feb1682749..ddb22d98eb41 100644\n> > > --- a/src/ipa/rkisp1/rkisp1.cpp\n> > > +++ b/src/ipa/rkisp1/rkisp1.cpp\n> > > @@ -246,7 +246,7 @@ int IPARkISP1::configure([[maybe_unused]] const IPACameraSensorInfo &info,\n> > > context_.configuration.hw.revision = hwRevision_;\n> > >\n> > > context_.configuration.sensor.size = info.outputSize;\n> > > - context_.configuration.sensor.lineDuration = info.lineLength * 1.0s / info.pixelRate;\n> > > + context_.configuration.sensor.lineDuration = info.minLineLength * 1.0s / info.pixelRate;\n> > >\n> > > /*\n> > > * When the AGC computes the new exposure values for a frame, it needs\n> > > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> > > index 911fd0beae4e..c2c8d3d44e26 100644\n> > > --- a/src/libcamera/camera_sensor.cpp\n> > > +++ b/src/libcamera/camera_sensor.cpp\n> > > @@ -883,10 +883,12 @@ int CameraSensor::sensorInfo(IPACameraSensorInfo *info) const\n> > > return -EINVAL;\n> > > }\n> > >\n> > > - int32_t hblank = ctrls.get(V4L2_CID_HBLANK).get();\n> > > - info->lineLength = info->outputSize.width + hblank;\n> > > info->pixelRate = ctrls.get(V4L2_CID_PIXEL_RATE).get();\n> > >\n> > > + const ControlInfo hblank = ctrls.infoMap()->at(V4L2_CID_HBLANK);\n> > > + info->minLineLength = info->outputSize.width + hblank.min().get();\n> > > + info->maxLineLength = info->outputSize.width + hblank.max().get();\n> >\n> > Hmmm... If the current value of V4L2_CID_HBLANK is different than the\n> > minimum, won't all the IPA modules compute an incorrect line length ?\n> > I'm sure this is fixed for the Raspberry Pi IPA module in subsequent\n> > patches, but the IPU3 and RkISP1 will likely be affected.\n> \n> This is true - I am making the (perhaps incorrect) assumption that sensors that\n> do not dynamically control hblank will have the control value set to the minimum.\n> I made the change here so that the min/max LineDuration fields in IPACameraSensorInfo\n> matches with what min/max FrameLength conveys.\n> \n> I can make one of two changes to fix this:\n> \n> 1) Revert the above code so that IPACameraSensorInfo continues to report\n> lineLength as before. I will then have to update our RPi mode setup so that\n> we essentially do the above in our own mode structure.\n> \n> 2) Keep the above change, and update rkisp1/ipu3/rpi IPAs to calculate line length\n> directly from the control value instead of from the fields in IPACameraSensorInfo.\n> \n> For consistency, I would prefer (2), but happy to do either.\n\n3) Set V4L2_CID_HBLANK to the minimum when initializing the sensor.\n\nOr could this introduce breakages ?\n\n> > > +\n> > > const ControlInfo vblank = ctrls.infoMap()->at(V4L2_CID_VBLANK);\n> > > info->minFrameLength = info->outputSize.height + vblank.min().get();\n> > > info->maxFrameLength = info->outputSize.height + vblank.max().get();","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 1CB2BC0DA4\n\tfor ;\n\tWed, 5 Oct 2022 12:39:35 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 66FC56236C;\n\tWed, 5 Oct 2022 14:39:34 +0200 (CEST)","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 72539601C7\n\tfor ;\n\tWed, 5 Oct 2022 14:39:32 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id BBD2C997;\n\tWed, 5 Oct 2022 14:39:31 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1664973574;\n\tbh=GqQCPlpp1rKlgqUidGOFdmJEMJeO7U3oeSwyQRAvrHk=;\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=qLPtfTLy/N1axW3ykscOHkYmOHzdDhSmke6nDxzT9xAlUqiHOUeSQZ68ixmarEmXJ\n\tfilVaJbZ898KdMpsvEMSCKx0tnFnmIVSpvbJh0iFDr36+bQWWqf3Tfi64H9yZLtNKI\n\tZFHA90q/NhKRXq5RftgrDcJabByEWZouUAGpvDUodK7hZwOb8lvlru6+1uAkM3as1g\n\t+ptDiKl9HGgnZ0JaqkWtC0BuOBifCxTRPjuSOmDPTIEAVCToOcN/NP3h6KNOGDfBsb\n\tzd+Jt/H4UpNDSoje1F5fiZZhaJKKQY1cVS6Uw/GUdUqhEXv+iAnRDD2jGO+ayaLfJP\n\t5qzJJ6IkdqyoA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1664973572;\n\tbh=GqQCPlpp1rKlgqUidGOFdmJEMJeO7U3oeSwyQRAvrHk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=O/vSxiRbDF9jp6mQGBOAtWGsS1tEcCIqGEgpV8VkwioMEAiizJ/r4F9Mxcb3SRh2Y\n\tBhrDTJOc2B/4NEY28BjeXcOkaVlXZWWgWV2DJw86vTxBL99xxAx4eXHjaZsdmNhDXx\n\tnnrweeEW539cQN2FQ53wQz4P5DtBKcKv+iVqsp8g="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"O/vSxiRb\"; dkim-atps=neutral","Date":"Wed, 5 Oct 2022 15:39:28 +0300","To":"Naushir Patuck ","Message-ID":"","References":"<20221003083934.31629-1-naush@raspberrypi.com>\n\t<20221003083934.31629-2-naush@raspberrypi.com>\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 v1 1/9] camera_sensor: Add minimum and\n\tmaximum line length to IPACameraSensorInfo","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":25311,"web_url":"https://patchwork.libcamera.org/comment/25311/","msgid":"","date":"2022-10-05T13:16:04","subject":"Re: [libcamera-devel] [PATCH v1 1/9] camera_sensor: Add minimum and\n\tmaximum line length to IPACameraSensorInfo","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Laurent,\n\nOn Wed, 5 Oct 2022 at 13:39, Laurent Pinchart <\nlaurent.pinchart@ideasonboard.com> wrote:\n\n> Hi Naush,\n>\n> On Wed, Oct 05, 2022 at 01:01:41PM +0100, Naushir Patuck wrote:\n> > On Tue, 4 Oct 2022 at 17:40, Laurent Pinchart wrote:\n> > > On Mon, Oct 03, 2022 at 09:39:27AM +0100, Naushir Patuck via\n> > > libcamera-devel wrote:\n> > > > Add fields for minimum and maximum line length (in units of pixels)\n> to the\n> > > > IPACameraSensorInfo structure. This replaces the existing lineLength\n> field.\n> > > >\n> > > > Update the ipu3, raspberrypi and rkisp1 IPAs to use\n> IPACameraSensorInfo::minLineLength\n> > > > instead of IPACameraSensorInfo::lineLength, as logically we will\n> always want to\n> > > > use the fastest sensor readout by default.\n> > > >\n> > > > Signed-off-by: Naushir Patuck \n> > > > ---\n> > > > include/libcamera/ipa/core.mojom | 21 +++++++++++++++------\n> > > > src/ipa/ipu3/ipu3.cpp | 2 +-\n> > > > src/ipa/raspberrypi/raspberrypi.cpp | 2 +-\n> > > > src/ipa/rkisp1/rkisp1.cpp | 2 +-\n> > > > src/libcamera/camera_sensor.cpp | 6 ++++--\n> > >\n> > > This causes a compilation failure in src/ipa/ipu3/ipu3.cpp due to a\n> > > change that has been merged recently. It's easy to fix with\n> > >\n> > > diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> > > index d66965e7d94e..c89f76c56ee3 100644\n> > > --- a/src/ipa/ipu3/ipu3.cpp\n> > > +++ b/src/ipa/ipu3/ipu3.cpp\n> > > @@ -499,7 +500,8 @@ int IPAIPU3::configure(const IPAConfigInfo\n> &configInfo,\n> > > context_.frameContexts.clear();\n> > >\n> > > /* Initialise the sensor configuration. */\n> > > - context_.configuration.sensor.lineDuration =\n> sensorInfo_.lineLength * 1.0s / sensorInfo_.pixelRate;\n> > > + context_.configuration.sensor.lineDuration =\n> sensorInfo_.minLineLength\n> > > + * 1.0s /\n> sensorInfo_.pixelRate;\n> > >\n> > > /*\n> > > * Compute the sensor V4L2 controls to be used by the\n> algorithms and\n> > >\n> > > If that's the only change needed in this series, I'll fix it myself,\n> > > otherwise you can include this in v2.\n> >\n> > I'll fix this up in the next revision.\n> >\n> > > > 5 files changed, 22 insertions(+), 11 deletions(-)\n> > > >\n> > > > diff --git a/include/libcamera/ipa/core.mojom\n> b/include/libcamera/ipa/core.mojom\n> > > > index 74f3339e56f2..2bc3028c22d6 100644\n> > > > --- a/include/libcamera/ipa/core.mojom\n> > > > +++ b/include/libcamera/ipa/core.mojom\n> > > > @@ -172,10 +172,17 @@ module libcamera;\n> > > > */\n> > > >\n> > > > /**\n> > > > - * \\var IPACameraSensorInfo::lineLength\n> > > > - * \\brief Total line length in pixels\n> > > > + * \\var IPACameraSensorInfo::minLineLength\n> > > > + * \\brief The minimum line length in pixels\n> > > > *\n> > > > - * The total line length in pixel clock periods, including blanking.\n> > > > + * The minimum allowable line length in pixel clock periods,\n> including blanking.\n> > > > + */\n> > > > +\n> > > > +/**\n> > > > + * \\var IPACameraSensorInfo::maxLineLength\n> > > > + * \\brief The maximum line length in pixels\n> > > > + *\n> > > > + * The maximum allowable line length in pixel clock periods,\n> including blanking.\n> > > > */\n> > > >\n> > > > /**\n> > > > @@ -189,7 +196,7 @@ module libcamera;\n> > > > * To obtain the minimum frame duration:\n> > > > *\n> > > > * \\verbatim\n> > > > - frameDuration(s) = minFrameLength(lines) *\n> lineLength(pixels) / pixelRate(pixels per second)\n> > > > + frameDuration(s) = minFrameLength(lines) *\n> minLineLength(pixels) / pixelRate(pixels per second)\n> > > > \\endverbatim\n> > > > */\n> > > >\n> > > > @@ -204,7 +211,7 @@ module libcamera;\n> > > > * To obtain the maximum frame duration:\n> > > > *\n> > > > * \\verbatim\n> > > > - frameDuration(s) = maxFrameLength(lines) *\n> lineLength(pixels) / pixelRate(pixels per second)\n> > > > + frameDuration(s) = maxFrameLength(lines) *\n> maxLineLength(pixels) / pixelRate(pixels per second)\n> > > > \\endverbatim\n> > > > */\n> > > > struct IPACameraSensorInfo {\n> > > > @@ -217,7 +224,9 @@ struct IPACameraSensorInfo {\n> > > > Size outputSize;\n> > > >\n> > > > uint64 pixelRate;\n> > > > - uint32 lineLength;\n> > > > +\n> > > > + uint32 minLineLength;\n> > > > + uint32 maxLineLength;\n> > > >\n> > > > uint32 minFrameLength;\n> > > > uint32 maxFrameLength;\n> > > > diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> > > > index d1ea081d595d..da029571ba55 100644\n> > > > --- a/src/ipa/ipu3/ipu3.cpp\n> > > > +++ b/src/ipa/ipu3/ipu3.cpp\n> > > > @@ -341,7 +341,7 @@ int IPAIPU3::init(const IPASettings &settings,\n> > > >\n> > > > /* Clean context */\n> > > > context_.configuration = {};\n> > > > - context_.configuration.sensor.lineDuration =\n> sensorInfo.lineLength * 1.0s / sensorInfo.pixelRate;\n> > > > + context_.configuration.sensor.lineDuration =\n> sensorInfo.minLineLength * 1.0s / sensorInfo.pixelRate;\n> > >\n> > > This is getting long, let's wrap it.\n> > >\n> > > context_.configuration.sensor.lineDuration =\n> sensorInfo.minLineLength\n> > > * 1.0s /\n> sensorInfo.pixelRate;\n> > >\n> > > (I can also apply this change myself if no v2 is otherwise needed.)\n> > >\n> > > >\n> > > > /* Load the tuning data file. */\n> > > > File file(settings.configurationFile);\n> > > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp\n> b/src/ipa/raspberrypi/raspberrypi.cpp\n> > > > index 8d731435764e..358a119da222 100644\n> > > > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> > > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> > > > @@ -356,7 +356,7 @@ void IPARPi::setMode(const IPACameraSensorInfo\n> &sensorInfo)\n> > > > * Calculate the line length as the ratio between the line\n> length in\n> > > > * pixels and the pixel rate.\n> > > > */\n> > > > - mode_.lineLength = sensorInfo.lineLength * (1.0s /\n> sensorInfo.pixelRate);\n> > > > + mode_.lineLength = sensorInfo.minLineLength * (1.0s /\n> sensorInfo.pixelRate);\n> > > >\n> > > > /*\n> > > > * Set the frame length limits for the mode to ensure exposure\n> and\n> > > > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> > > > index 32feb1682749..ddb22d98eb41 100644\n> > > > --- a/src/ipa/rkisp1/rkisp1.cpp\n> > > > +++ b/src/ipa/rkisp1/rkisp1.cpp\n> > > > @@ -246,7 +246,7 @@ int IPARkISP1::configure([[maybe_unused]] const\n> IPACameraSensorInfo &info,\n> > > > context_.configuration.hw.revision = hwRevision_;\n> > > >\n> > > > context_.configuration.sensor.size = info.outputSize;\n> > > > - context_.configuration.sensor.lineDuration = info.lineLength *\n> 1.0s / info.pixelRate;\n> > > > + context_.configuration.sensor.lineDuration =\n> info.minLineLength * 1.0s / info.pixelRate;\n> > > >\n> > > > /*\n> > > > * When the AGC computes the new exposure values for a frame,\n> it needs\n> > > > diff --git a/src/libcamera/camera_sensor.cpp\n> b/src/libcamera/camera_sensor.cpp\n> > > > index 911fd0beae4e..c2c8d3d44e26 100644\n> > > > --- a/src/libcamera/camera_sensor.cpp\n> > > > +++ b/src/libcamera/camera_sensor.cpp\n> > > > @@ -883,10 +883,12 @@ int\n> CameraSensor::sensorInfo(IPACameraSensorInfo *info) const\n> > > > return -EINVAL;\n> > > > }\n> > > >\n> > > > - int32_t hblank = ctrls.get(V4L2_CID_HBLANK).get();\n> > > > - info->lineLength = info->outputSize.width + hblank;\n> > > > info->pixelRate =\n> ctrls.get(V4L2_CID_PIXEL_RATE).get();\n> > > >\n> > > > + const ControlInfo hblank =\n> ctrls.infoMap()->at(V4L2_CID_HBLANK);\n> > > > + info->minLineLength = info->outputSize.width +\n> hblank.min().get();\n> > > > + info->maxLineLength = info->outputSize.width +\n> hblank.max().get();\n> > >\n> > > Hmmm... If the current value of V4L2_CID_HBLANK is different than the\n> > > minimum, won't all the IPA modules compute an incorrect line length ?\n> > > I'm sure this is fixed for the Raspberry Pi IPA module in subsequent\n> > > patches, but the IPU3 and RkISP1 will likely be affected.\n> >\n> > This is true - I am making the (perhaps incorrect) assumption that\n> sensors that\n> > do not dynamically control hblank will have the control value set to the\n> minimum.\n> > I made the change here so that the min/max LineDuration fields in\n> IPACameraSensorInfo\n> > matches with what min/max FrameLength conveys.\n> >\n> > I can make one of two changes to fix this:\n> >\n> > 1) Revert the above code so that IPACameraSensorInfo continues to report\n> > lineLength as before. I will then have to update our RPi mode setup so\n> that\n> > we essentially do the above in our own mode structure.\n> >\n> > 2) Keep the above change, and update rkisp1/ipu3/rpi IPAs to calculate\n> line length\n> > directly from the control value instead of from the fields in\n> IPACameraSensorInfo.\n> >\n> > For consistency, I would prefer (2), but happy to do either.\n>\n> 3) Set V4L2_CID_HBLANK to the minimum when initializing the sensor.\n>\n> Or could this introduce breakages ?\n>\n\nThe only breakage I can think of is if a user was setting HBLANK outside of\nlibcamera (through v4l2-ctl) and expecting the IPA to just use that value\nwithout\nchange. I don't know how common that is, particularly given how few sensor\ndrivers actually seem to have read/write HBLANK controls.\n\nNaush\n\n\n>\n> > > > +\n> > > > const ControlInfo vblank =\n> ctrls.infoMap()->at(V4L2_CID_VBLANK);\n> > > > info->minFrameLength = info->outputSize.height +\n> vblank.min().get();\n> > > > info->maxFrameLength = info->outputSize.height +\n> vblank.max().get();\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\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 BDBF7BD16B\n\tfor ;\n\tWed, 5 Oct 2022 13:16:23 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 183B16239B;\n\tWed, 5 Oct 2022 15:16:23 +0200 (CEST)","from mail-lj1-x232.google.com (mail-lj1-x232.google.com\n\t[IPv6:2a00:1450:4864:20::232])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2D3E8601C7\n\tfor ;\n\tWed, 5 Oct 2022 15:16:21 +0200 (CEST)","by mail-lj1-x232.google.com with SMTP id by36so956295ljb.4\n\tfor ;\n\tWed, 05 Oct 2022 06:16:21 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1664975783;\n\tbh=lYMhwXkVmzRSHtjjB5/bNG9fP4eaDFweythRRWQNvf8=;\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=qRZJp1xW79agr4oZQxzJIfESzQCQsAbYKLoLJA15nd9f6K9bHQ9oxYFy1AL4ts8MF\n\tUI4GjI6sEhiNKwudo6xfC7V+lmx2QvGkClP7OEyQ+RWjwJse+e2Bl8Z4PHNabe8NuU\n\toglON4fmzij6b3NNsNiXk9/kfdA00wRytppsx3gMwCZqScsES6+gEe9w7i564/zpqX\n\ty4fgst72hC2+wKvxcHjX7Thq0hNE5NdKMWThRaCpmdojG1JrwYMcXVil7eUtxRCF/R\n\txp0H5Hyz995CRc3P1rt82mbJjkAMls1+4eFGH+0BulMscRLk46EEqVdSWC3tWGMz/Z\n\t7K0K5o5TfmGkA==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc:subject:date;\n\tbh=tx9JG62M6C8CmhVKJWK5OwoBP45KZ9BZtYZURVt1qC8=;\n\tb=qW7ghZYPrSl10Y0GBTE60kVSwavU0nthE9HR+AflLKaCfPSP4h6vW9A4lvE1wLYCnw\n\tMvrUoX4sdFCBIWXFeeNV+6wTmCz9PCHrZjJizH7ZR3kekwQTN48IBGkX9F5PXARTL95i\n\t7Ih8OBYjfnyD2Ognu6clS4pNEA/y586DP3UP0PgvQlNggb/i7+L4FWKzmo/zOdIIQVAt\n\tnWBp9V8fwZ4ZZp7iBSEPhfUJ7HR5VMUUElXiieXShPPHk0B2f7F9CLKhAOQMLDfDzinJ\n\tuet+3/T91ILpC4TzwgayoFlHypAdTWop+w/xg4EACXeZ77v/gvT7l18KJ0Pb3KzedXhb\n\tSnEg=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"qW7ghZYP\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\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;\n\tbh=tx9JG62M6C8CmhVKJWK5OwoBP45KZ9BZtYZURVt1qC8=;\n\tb=DHJsJ6Y141iciqTWwQ+8dXK0MclGaP/OMGtnqu0OS6JWl/Ahkesn0muN7XIJUghPEv\n\txeMe3RSWvNVNiSO/oqYOmKjlo1AvqxRAOAmyf8DZYCICMglsoEzd3BPv2nh26z4GcY4U\n\tRUxkB1rRf4pVVAwa+t5AyItHKHf3YE+JkFdbP/Rzt3J9k9ShfZOrLDBeR5/MotWuqoUu\n\tIfpksjFc5ifwa7ebgNYhTX6+ZPhuEow/PqFXtkiCWtdDp1JmsJF9BIjXV7P6SOzu81qD\n\tlw29FiJdtyrSNpUteQztoye6bWwlVhUN1q8airkpS8yTAPnvrssVQB1yNjtrVz9ScVGW\n\tEhBA==","X-Gm-Message-State":"ACrzQf3T6qdxIRxwCjUZQfr4xFQq443t5HSuC0D+sA/+oIDQQTrhMDzI\n\tMqUeMtNrcMRl+6fHn/UQHLrfDrYbijd9XnI6WrIL4VUBRhQ=","X-Google-Smtp-Source":"AMsMyM7Jigg7+E6mN3IVw1M1MOIwkRo2juUe2hwGflVwMA0SRlv6FaDTqD2dQzkmJcmi0O63p7XLwuIboImUionFySo=","X-Received":"by 2002:a05:651c:244:b0:26d:e2af:db15 with SMTP id\n\tx4-20020a05651c024400b0026de2afdb15mr4610826ljn.271.1664975780467;\n\tWed, 05 Oct 2022 06:16:20 -0700 (PDT)","MIME-Version":"1.0","References":"<20221003083934.31629-1-naush@raspberrypi.com>\n\t<20221003083934.31629-2-naush@raspberrypi.com>\n\t\n\t\n\t","In-Reply-To":"","Date":"Wed, 5 Oct 2022 14:16:04 +0100","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"multipart/alternative; boundary=\"000000000000f472a805ea49610e\"","Subject":"Re: [libcamera-devel] [PATCH v1 1/9] camera_sensor: Add minimum and\n\tmaximum line length to IPACameraSensorInfo","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\" "}}]