[{"id":25350,"web_url":"https://patchwork.libcamera.org/comment/25350/","msgid":"","date":"2022-10-07T22:28:47","subject":"Re: [libcamera-devel] [PATCH v2 01/10] camera_sensor: Add minimum\n\tand maximum 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 Thu, Oct 06, 2022 at 02:17:35PM +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> Since the IPAs now use minLineLength for their calculations, set the starting\n> value of the V4L2_CID_HBLANK control to its minimum in CameraSensor::init().\n> \n> Signed-off-by: Naushir Patuck \n> Tested-by: Dave Stevenson \n> ---\n> include/libcamera/ipa/core.mojom | 21 +++++++++++++++------\n> src/ipa/ipu3/ipu3.cpp | 6 ++++--\n> src/ipa/raspberrypi/raspberrypi.cpp | 2 +-\n> src/ipa/rkisp1/rkisp1.cpp | 2 +-\n> src/libcamera/camera_sensor.cpp | 15 +++++++++++++--\n> 5 files changed, 34 insertions(+), 12 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 b93a09d40c39..7e26fc5639c2 100644\n> --- a/src/ipa/ipu3/ipu3.cpp\n> +++ b/src/ipa/ipu3/ipu3.cpp\n> @@ -336,7 +336,8 @@ 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\n> +\t\t\t\t\t\t * 1.0s / sensorInfo.pixelRate;\n\nThe * should be aligned below the =. Same below.\n\n> \n> \t/* Load the tuning data file. */\n> \tFile file(settings.configurationFile);\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> 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..83f81d655395 100644\n> --- a/src/libcamera/camera_sensor.cpp\n> +++ b/src/libcamera/camera_sensor.cpp\n> @@ -176,6 +176,15 @@ int CameraSensor::init()\n> \tif (ret)\n> \t\treturn ret;\n> \n> +\t/* Set HBLANK to the minimum as a starting value. */\n\nI would capture the rationale here.\n\n\t/*\n\t * Set HBLANK to the minimum to start with a well-defined line length,\n\t * allowing IPA modules that do not modify HBLANK to use the sensor\n\t * minimum line length in their calculations.\n\t */\n\nReviewed-by: Laurent Pinchart \n\nNo need to resubmit if only those small changes are required.\n\n> +\tconst ControlInfo hblank = ctrls.infoMap()->at(V4L2_CID_HBLANK);\n> +\tControlList ctrl(subdev_->controls());\n> +\n> +\tctrl.set(V4L2_CID_HBLANK, hblank.min().get());\n> +\tret = subdev_->setControls(&ctrl);\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> \treturn applyTestPatternMode(controls::draft::TestPatternModeEnum::TestPatternModeOff);\n> }\n> \n> @@ -883,10 +892,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> +\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 A86D0BD16B\n\tfor ;\n\tFri, 7 Oct 2022 22:28:55 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C8A3562D2A;\n\tSat, 8 Oct 2022 00:28:54 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 13D4E62CEC\n\tfor ;\n\tSat, 8 Oct 2022 00:28:53 +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 67CD24E6;\n\tSat, 8 Oct 2022 00:28:52 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1665181734;\n\tbh=yrZHNVkRekfhkzv/oHoHYVHojskKrSsStNHwC7MSGRw=;\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=Clw2sCT755qKT8ZE+ODYmo6J5B2jY6TxiIFXZJPh/7/PEzLD6rfHX24qzGwemJ8rm\n\t9uqvRttGvyFX/lqvJD7XH/SECyx/oPW5prLPirydBFG8KZnqLDTG99N39hDfANaeAq\n\tXTOjhh/Gr4VIm8GB63qM2HWa5KUzUs0ILNezB9tjabasrWPpMAh1QAv0BS30izCxnk\n\tUgm/Jf3MAq+rMx3smcBAFaUffBPsPSApE3fJzlWILzA/z4wz2SYH68cwdBbUJaRkIC\n\t8+ieMu0BVqBTVTMt6SkANTRDc3r6jGIgYGpkfnez9oXdi3W0RtpR01cu06kycDnfVR\n\ti5YqPJ250FgOA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1665181732;\n\tbh=yrZHNVkRekfhkzv/oHoHYVHojskKrSsStNHwC7MSGRw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=QUsKjzQpT8MkaKbNS8q1xaHWJPUFLZnePsgTkv4mfGdHv2wWa1cxQiPmuWjwhlEvR\n\tP2c6Thz5qtHuKlPXwn/K8zxDj/82+xqMpTnEuVpsjthLIbr14JSUczSwiBF6u1RCsw\n\tUMubVyVeqj/lBorlmByiDNMr3KTrZlykGcyapnhw="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"QUsKjzQp\"; dkim-atps=neutral","Date":"Sat, 8 Oct 2022 01:28:47 +0300","To":"Naushir Patuck ","Message-ID":"","References":"<20221006131744.5179-1-naush@raspberrypi.com>\n\t<20221006131744.5179-2-naush@raspberrypi.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20221006131744.5179-2-naush@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH v2 01/10] camera_sensor: Add minimum\n\tand maximum 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":25355,"web_url":"https://patchwork.libcamera.org/comment/25355/","msgid":"","date":"2022-10-08T06:12:55","subject":"Re: [libcamera-devel] [PATCH v2 01/10] camera_sensor: Add minimum\n\tand maximum 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\n\nOn Fri, 7 Oct 2022 at 23:28, Laurent Pinchart <\nlaurent.pinchart@ideasonboard.com> wrote:\n\n> Hi Naush,\n>\n> Thank you for the patch.\n>\n> On Thu, Oct 06, 2022 at 02:17:35PM +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> > Since the IPAs now use minLineLength for their calculations, set the\n> starting\n> > value of the V4L2_CID_HBLANK control to its minimum in\n> CameraSensor::init().\n> >\n> > Signed-off-by: Naushir Patuck \n> > Tested-by: Dave Stevenson \n> > ---\n> > include/libcamera/ipa/core.mojom | 21 +++++++++++++++------\n> > src/ipa/ipu3/ipu3.cpp | 6 ++++--\n> > src/ipa/raspberrypi/raspberrypi.cpp | 2 +-\n> > src/ipa/rkisp1/rkisp1.cpp | 2 +-\n> > src/libcamera/camera_sensor.cpp | 15 +++++++++++++--\n> > 5 files changed, 34 insertions(+), 12 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 b93a09d40c39..7e26fc5639c2 100644\n> > --- a/src/ipa/ipu3/ipu3.cpp\n> > +++ b/src/ipa/ipu3/ipu3.cpp\n> > @@ -336,7 +336,8 @@ 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\n> > + * 1.0s /\n> sensorInfo.pixelRate;\n>\n> The * should be aligned below the =. Same below.\n>\n> >\n> > /* Load the tuning data file. */\n> > File file(settings.configurationFile);\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 algorithms\n> and\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..83f81d655395 100644\n> > --- a/src/libcamera/camera_sensor.cpp\n> > +++ b/src/libcamera/camera_sensor.cpp\n> > @@ -176,6 +176,15 @@ int CameraSensor::init()\n> > if (ret)\n> > return ret;\n> >\n> > + /* Set HBLANK to the minimum as a starting value. */\n>\n> I would capture the rationale here.\n>\n> /*\n> * Set HBLANK to the minimum to start with a well-defined line\n> length,\n> * allowing IPA modules that do not modify HBLANK to use the sensor\n> * minimum line length in their calculations.\n> */\n>\n> Reviewed-by: Laurent Pinchart \n>\n> No need to resubmit if only those small changes are required.\n>\n> > + const ControlInfo hblank = ctrls.infoMap()->at(V4L2_CID_HBLANK);\n> > + ControlList ctrl(subdev_->controls());\n> > +\n> > + ctrl.set(V4L2_CID_HBLANK, hblank.min().get());\n> > + ret = subdev_->setControls(&ctrl);\n> > + if (ret)\n> > + return ret;\n>\n\nActually, doing the setControls here unconditionally is probably incorrect.\n\nIf the control is read-only, we will return an error. We probably want to\ndo something like what I did for the IPA where we test for\nHBLANK::min() == HBLANK::max(), and if true, assume the control is\nread-only and don't do the setControl().\n\nSorry I should have caught that, but my testing was done on sensors\nwith adjustable HBLANK controls. I'll update the statement and post\na revised patch first thing on Monday.\n\nRegards,\nNaush\n\n\n\n> > +\n> > return\n> applyTestPatternMode(controls::draft::TestPatternModeEnum::TestPatternModeOff);\n> > }\n> >\n> > @@ -883,10 +892,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> > 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 47926BD16B\n\tfor ;\n\tSat, 8 Oct 2022 06:13:19 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8734462D47;\n\tSat, 8 Oct 2022 08:13:18 +0200 (CEST)","from mail-lf1-x129.google.com (mail-lf1-x129.google.com\n\t[IPv6:2a00:1450:4864:20::129])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7A5AA603D4\n\tfor ;\n\tSat, 8 Oct 2022 08:13:16 +0200 (CEST)","by mail-lf1-x129.google.com with SMTP id b1so5005840lfs.7\n\tfor ;\n\tFri, 07 Oct 2022 23:13:16 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1665209598;\n\tbh=xhd4s1auoSKXBGCc8VzDLV0qZuHMIF8CXbVO/0rnaO0=;\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=vraiW+ZWThd6/zIxfC5vgufOBTDJqMSWvwq9zU9i7VjGO1knW7I6JQHnFX9csjveD\n\ttSqR7ncJMg4j0ajJxvakoLkxthmkft6r4XFAGVlJEEkLUVqsOPIig8MHThrCWJTpys\n\tBhLZsOv8xY5vkXZ4+cEcQ9vr2/UKbM9spiJMbsoeWnxJiDyA3cMjIJ6Vi/4QbkoUjK\n\thZICGF/HqOxdDcAudfDTuMMdIrb13LVc3EH1wPSncgFGzEFQKWeXG6JL4TqH0CG6Em\n\tbVjwmZgL1p8Xvm6cYZ87qHWJsjx7y30hBwI5UAyuxP8xrBS+4nCJOHjeDnaINtxe+g\n\tE6CeOAY6avBjA==","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:message-id:reply-to;\n\tbh=lZQn+4JEfy4yhPVdlccbA3uVae1p4V92bqY3glmKnB4=;\n\tb=I+rlSMY1/HTpq011rSSpZXhokhh0BMyfWHhBNvVt4tzIH6NScVF+z3gXlz3IYl5Frq\n\t0BV31J+g/vy5EzOuyRYR/3h3ehZXULRJLfbWtkj/aZBvC/p+o1agkkeo/sSV76EjFI30\n\tIzkTNguLn//lgvqYCnidlZdl6/tY2pAeOi0T7lNNy7OKFT91cLnKyZjcX0YWXSVZbLTN\n\t/e8J6WLr2j88uWd3SW1hFWuaXyywTsc72T/cqVWg3cdiKwb+wicsREo4VOgQhpmwwaTM\n\t4Kn7iN9cS8boq9eEaJMMt23zCpe+hR3E60jH92wisnb08ek0Ly6AznJZKol1Q6fqaea0\n\tblXQ=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"I+rlSMY1\"; 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:message-id\n\t:reply-to;\n\tbh=lZQn+4JEfy4yhPVdlccbA3uVae1p4V92bqY3glmKnB4=;\n\tb=eIzgDDQy+kyK4aD59AD1ypqCM5SPoe023TfoO7dw70Sx4OZ2WHgR4r7wltug2inYHC\n\t+BI8YDl4KbDsxrhMPOQnIvCKR7Q9UeFLuoiTJxJN9Mt/S3gcvOZZUbEt23FiHu6hW6su\n\tWgdzjAqMAAs41mJON6DUJPSRwGoCnnXKYtrXVA5qtzFq4VENo076fMrmRF5yAS2TTX9X\n\tXXFwVx3bbMxFh8vGMzJOMK2eGAmFIcGjfgafVHoYWzg/zNflIgb3fuAISr9fl8hbdYmd\n\toqxkdWmgqCGU4LTLfPN1AlFzqhjmI2ESVXjYzDzjRenUiKE85mCpST37QNDwvjD7QEgK\n\t9JVA==","X-Gm-Message-State":"ACrzQf2ayGJte5FbfvEqjQCCAI/17BuW+r6ez672NHT7tJkwaWpr4DDK\n\tABjCI0XF2p75PIHcmPotPGpSGitKE2csdiRtHdGMByx0GU0=","X-Google-Smtp-Source":"AMsMyM5OSjhI9DFrgDgDXr35w4x74hbu5NJtltK5i6/irB/Q4k9Whx7CnwXZMlj/6UiJjfFhrcuN+ZoBPi7x/H8fzKk=","X-Received":"by 2002:ac2:4c03:0:b0:4a2:2273:89c6 with SMTP id\n\tt3-20020ac24c03000000b004a2227389c6mr2715081lfq.245.1665209595565;\n\tFri, 07 Oct 2022 23:13:15 -0700 (PDT)","MIME-Version":"1.0","References":"<20221006131744.5179-1-naush@raspberrypi.com>\n\t<20221006131744.5179-2-naush@raspberrypi.com>\n\t","In-Reply-To":"","Date":"Sat, 8 Oct 2022 07:12:55 +0100","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"multipart/alternative; boundary=\"0000000000006baf9405ea7fd239\"","Subject":"Re: [libcamera-devel] [PATCH v2 01/10] camera_sensor: Add minimum\n\tand maximum 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":25369,"web_url":"https://patchwork.libcamera.org/comment/25369/","msgid":"<20221010111052.5ofbhwp4irsi6aiq@uno.localdomain>","date":"2022-10-10T11:10:52","subject":"Re: [libcamera-devel] [PATCH v2 01/10] camera_sensor: Add minimum\n\tand maximum line length to IPACameraSensorInfo","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi\n\nOn Sat, Oct 08, 2022 at 07:12:55AM +0100, Naushir Patuck via libcamera-devel wrote:\n> Hi Laurent,\n>\n>\n> On Fri, 7 Oct 2022 at 23:28, Laurent Pinchart <\n> laurent.pinchart@ideasonboard.com> wrote:\n>\n> > Hi Naush,\n> >\n> > Thank you for the patch.\n> >\n> > On Thu, Oct 06, 2022 at 02:17:35PM +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> > > Since the IPAs now use minLineLength for their calculations, set the\n> > starting\n> > > value of the V4L2_CID_HBLANK control to its minimum in\n> > CameraSensor::init().\n> > >\n> > > Signed-off-by: Naushir Patuck \n> > > Tested-by: Dave Stevenson \n> > > ---\n> > > include/libcamera/ipa/core.mojom | 21 +++++++++++++++------\n> > > src/ipa/ipu3/ipu3.cpp | 6 ++++--\n> > > src/ipa/raspberrypi/raspberrypi.cpp | 2 +-\n> > > src/ipa/rkisp1/rkisp1.cpp | 2 +-\n> > > src/libcamera/camera_sensor.cpp | 15 +++++++++++++--\n> > > 5 files changed, 34 insertions(+), 12 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 b93a09d40c39..7e26fc5639c2 100644\n> > > --- a/src/ipa/ipu3/ipu3.cpp\n> > > +++ b/src/ipa/ipu3/ipu3.cpp\n> > > @@ -336,7 +336,8 @@ 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\n> > > + * 1.0s /\n> > sensorInfo.pixelRate;\n> >\n> > The * should be aligned below the =. Same below.\n> >\n> > >\n> > > /* Load the tuning data file. */\n> > > File file(settings.configurationFile);\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 algorithms\n> > and\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..83f81d655395 100644\n> > > --- a/src/libcamera/camera_sensor.cpp\n> > > +++ b/src/libcamera/camera_sensor.cpp\n> > > @@ -176,6 +176,15 @@ int CameraSensor::init()\n> > > if (ret)\n> > > return ret;\n> > >\n> > > + /* Set HBLANK to the minimum as a starting value. */\n> >\n> > I would capture the rationale here.\n> >\n> > /*\n> > * Set HBLANK to the minimum to start with a well-defined line\n> > length,\n> > * allowing IPA modules that do not modify HBLANK to use the sensor\n> > * minimum line length in their calculations.\n> > */\n> >\n> > Reviewed-by: Laurent Pinchart \n> >\n> > No need to resubmit if only those small changes are required.\n> >\n> > > + const ControlInfo hblank = ctrls.infoMap()->at(V4L2_CID_HBLANK);\n> > > + ControlList ctrl(subdev_->controls());\n> > > +\n> > > + ctrl.set(V4L2_CID_HBLANK, hblank.min().get());\n> > > + ret = subdev_->setControls(&ctrl);\n> > > + if (ret)\n> > > + return ret;\n> >\n>\n> Actually, doing the setControls here unconditionally is probably incorrect.\n>\n> If the control is read-only, we will return an error. We probably want to\n> do something like what I did for the IPA where we test for\n> HBLANK::min() == HBLANK::max(), and if true, assume the control is\n> read-only and don't do the setControl().\n>\n> Sorry I should have caught that, but my testing was done on sensors\n> with adjustable HBLANK controls. I'll update the statement and post\n> a revised patch first thing on Monday.\n>\n\nI wonder if we shouldn't use (for hblank as well as per vblank) the\ncontrol's default value in the IPA calculations instead of assuming\n(and here forcing) the minium value. Specifically, during the\nIPA::configure() where we intialize values using the minium value\nseems rather an arbitrary assumptions. After IPA::configure() the IPA\nshould be free to change H/VBlank as they like in the min/max ranges,\nbut for intiialization...\n\nAlso most drivers (should?) update the blanking values to a sensible\ndefaul when a new mode is applied. Shouldn't we rely on their\nselection ?\n\n\n> Regards,\n> Naush\n>\n>\n>\n> > > +\n> > > return\n> > applyTestPatternMode(controls::draft::TestPatternModeEnum::TestPatternModeOff);\n> > > }\n> > >\n> > > @@ -883,10 +892,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> > > 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 9CA59C0DA4\n\tfor ;\n\tMon, 10 Oct 2022 11:10:56 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0AE4062D58;\n\tMon, 10 Oct 2022 13:10:56 +0200 (CEST)","from relay10.mail.gandi.net (relay10.mail.gandi.net\n\t[IPv6:2001:4b98:dc4:8::230])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id EC57662272\n\tfor ;\n\tMon, 10 Oct 2022 13:10:54 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby mail.gandi.net (Postfix) with ESMTPSA id 380AD240005;\n\tMon, 10 Oct 2022 11:10:53 +0000 (UTC)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1665400256;\n\tbh=XR7NJxAA/uSHkavm7mdoJlLy1AlpRmG4grjflwyIbt4=;\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=iD8r0Kp6FV/REhCjUDwqiqNvqCKsW9mksZnK6We6F7CrjafjlDqgT0TmcwE3EQiY2\n\tZVhyn13ER7EoGNCa3p7NLY11mPkefEDpboiNi6oyCudA9QhDMWaXwFa7QE94xDQRLh\n\t1V1jwO8Uzco7ZKtRV+DHphuZ6Y/3MwmZfhficGmIfRM+Kdd1J7hso6awFPCtiKPkui\n\tfWLjhk5mhvT5c1PIRN9EF8ML1Tw9IHOZB9zUbLoJhiEOEpaqXsa9wI03cRdnMZAUpL\n\tdYGTzb1tOy8EsSHqXO+1dNYNnzuzl7GZJgjRe+ZzbviaLL1lhijgS9xKkAn7ICMrjq\n\tK2xn5wNOFJS5A==","Date":"Mon, 10 Oct 2022 13:10:52 +0200","To":"Naushir Patuck ","Message-ID":"<20221010111052.5ofbhwp4irsi6aiq@uno.localdomain>","References":"<20221006131744.5179-1-naush@raspberrypi.com>\n\t<20221006131744.5179-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 v2 01/10] camera_sensor: Add minimum\n\tand maximum 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":"Jacopo Mondi via libcamera-devel ","Reply-To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":25374,"web_url":"https://patchwork.libcamera.org/comment/25374/","msgid":"","date":"2022-10-10T13:44:41","subject":"Re: [libcamera-devel] [PATCH v2 01/10] camera_sensor: Add minimum\n\tand maximum line length to IPACameraSensorInfo","submitter":{"id":27,"url":"https://patchwork.libcamera.org/api/people/27/","name":"Dave Stevenson","email":"dave.stevenson@raspberrypi.com"},"content":"Hi Jacopo\n\nOn Mon, 10 Oct 2022 at 12:10, Jacopo Mondi via libcamera-devel\n wrote:\n>\n> Hi\n>\n> On Sat, Oct 08, 2022 at 07:12:55AM +0100, Naushir Patuck via libcamera-devel wrote:\n> > Hi Laurent,\n> >\n> >\n> > On Fri, 7 Oct 2022 at 23:28, Laurent Pinchart <\n> > laurent.pinchart@ideasonboard.com> wrote:\n> >\n> > > Hi Naush,\n> > >\n> > > Thank you for the patch.\n> > >\n> > > On Thu, Oct 06, 2022 at 02:17:35PM +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> > > > Since the IPAs now use minLineLength for their calculations, set the\n> > > starting\n> > > > value of the V4L2_CID_HBLANK control to its minimum in\n> > > CameraSensor::init().\n> > > >\n> > > > Signed-off-by: Naushir Patuck \n> > > > Tested-by: Dave Stevenson \n> > > > ---\n> > > > include/libcamera/ipa/core.mojom | 21 +++++++++++++++------\n> > > > src/ipa/ipu3/ipu3.cpp | 6 ++++--\n> > > > src/ipa/raspberrypi/raspberrypi.cpp | 2 +-\n> > > > src/ipa/rkisp1/rkisp1.cpp | 2 +-\n> > > > src/libcamera/camera_sensor.cpp | 15 +++++++++++++--\n> > > > 5 files changed, 34 insertions(+), 12 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 b93a09d40c39..7e26fc5639c2 100644\n> > > > --- a/src/ipa/ipu3/ipu3.cpp\n> > > > +++ b/src/ipa/ipu3/ipu3.cpp\n> > > > @@ -336,7 +336,8 @@ 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\n> > > > + * 1.0s /\n> > > sensorInfo.pixelRate;\n> > >\n> > > The * should be aligned below the =. Same below.\n> > >\n> > > >\n> > > > /* Load the tuning data file. */\n> > > > File file(settings.configurationFile);\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 algorithms\n> > > and\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..83f81d655395 100644\n> > > > --- a/src/libcamera/camera_sensor.cpp\n> > > > +++ b/src/libcamera/camera_sensor.cpp\n> > > > @@ -176,6 +176,15 @@ int CameraSensor::init()\n> > > > if (ret)\n> > > > return ret;\n> > > >\n> > > > + /* Set HBLANK to the minimum as a starting value. */\n> > >\n> > > I would capture the rationale here.\n> > >\n> > > /*\n> > > * Set HBLANK to the minimum to start with a well-defined line\n> > > length,\n> > > * allowing IPA modules that do not modify HBLANK to use the sensor\n> > > * minimum line length in their calculations.\n> > > */\n> > >\n> > > Reviewed-by: Laurent Pinchart \n> > >\n> > > No need to resubmit if only those small changes are required.\n> > >\n> > > > + const ControlInfo hblank = ctrls.infoMap()->at(V4L2_CID_HBLANK);\n> > > > + ControlList ctrl(subdev_->controls());\n> > > > +\n> > > > + ctrl.set(V4L2_CID_HBLANK, hblank.min().get());\n> > > > + ret = subdev_->setControls(&ctrl);\n> > > > + if (ret)\n> > > > + return ret;\n> > >\n> >\n> > Actually, doing the setControls here unconditionally is probably incorrect.\n> >\n> > If the control is read-only, we will return an error. We probably want to\n> > do something like what I did for the IPA where we test for\n> > HBLANK::min() == HBLANK::max(), and if true, assume the control is\n> > read-only and don't do the setControl().\n> >\n> > Sorry I should have caught that, but my testing was done on sensors\n> > with adjustable HBLANK controls. I'll update the statement and post\n> > a revised patch first thing on Monday.\n> >\n>\n> I wonder if we shouldn't use (for hblank as well as per vblank) the\n> control's default value in the IPA calculations instead of assuming\n> (and here forcing) the minium value. Specifically, during the\n> IPA::configure() where we intialize values using the minium value\n> seems rather an arbitrary assumptions. After IPA::configure() the IPA\n> should be free to change H/VBlank as they like in the min/max ranges,\n> but for intiialization...\n>\n> Also most drivers (should?) update the blanking values to a sensible\n> defaul when a new mode is applied. Shouldn't we rely on their\n> selection ?\n\nIs it correct behaviour for a sensor driver to change a r/w HBLANK\nwhen changing mode? Changing limits for HBLANK is almost certainly the\ncase, but AIUI generally controls shouldn't change values unless they\nare then invalid.\n\nI'll agree that not updating the value has annoyances when the client\ndoesn't automatically compute and set HBLANK, but conversely updating\nit is annoying for cases where the app doesn't support it and you have\ndeliberately set it to a long value (any mode change resets your\nchosen value, despite it being valid).\n\nlibcamera always selecting the mode (even if the same) leading to a\nreset when I was trying to test configurable HBLANK was where I\nencountered this ambiguity.\n\n Dave\n\n> > Regards,\n> > Naush\n> >\n> >\n> >\n> > > > +\n> > > > return\n> > > applyTestPatternMode(controls::draft::TestPatternModeEnum::TestPatternModeOff);\n> > > > }\n> > > >\n> > > > @@ -883,10 +892,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> > > > 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 83C73C0DA4\n\tfor ;\n\tMon, 10 Oct 2022 13:45:00 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E0F3562D63;\n\tMon, 10 Oct 2022 15:44:59 +0200 (CEST)","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 42EE262272\n\tfor ;\n\tMon, 10 Oct 2022 15:44:58 +0200 (CEST)","by mail-ej1-x62a.google.com with SMTP id sc25so18863854ejc.12\n\tfor ;\n\tMon, 10 Oct 2022 06:44:58 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1665409499;\n\tbh=goBvXxAWYRXmEaYfCNYdrTN40VDGiKZ9NGeVl3Jp0R4=;\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=xAnpRqOHErtocjM/S/HdM7wUng1mTjwweVyZq0MC5NeTBJVfAdFczzMxwUzCkz5Jn\n\tdg3LmXksex1Yky2U9Pql5krw+qs8nsMNl9gtR1fHbsh9+cqu6G5qhtOma0Qll8Aqz9\n\tMQvUwup2doy/C733puxKDLCMO4romNLh3DG8edoTc+owRDjwQVLShhxLYFnjSMRlRt\n\tZN7a6gKNVjwb/88h2e560Cs3I4VDYOr59J2B7Bj+Z1tZ243UWn8mG62Uf4vlrG+fP0\n\tUKP8YXQNQbDM+PgbI30/oCUk55aGuTETECnxA2dovu5DjU6cHsqVdMGnWbRJ9jNKIg\n\t9DM0w3qTwOJyQ==","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:message-id:reply-to;\n\tbh=vZOCmnTPdZwXiptSc53ZYO8+QH+5K1AhPQbDiyiLE2c=;\n\tb=n8xptbykZuciF+OPPcGg5Qk71VaX9M6/wa7ZJ19i3Kp8u6BpLafhXo/e9SIsTzrOAg\n\tR6/FSXxFN/22DSZwbs+lVjxZnGczz2xSECUju7QqFq1GfmJ95ZXoBWOO42TQu7TtZvdu\n\te9HxdCTIQn84jJZTxlZKPJZxocL7ucqW4xbM5RvSPT8aMniKvWn11WmAMqn3rTJgenqn\n\tKY0JGFrwRyYMIf8MdUE/owmOWRLtAuOfGdeslJeKH7xJ3aUR+KWeWJUK8kZLx/s/wxcx\n\t7wusAFlvFhao3hjYub1KTcyaJVtdysdeyvJ16EpbfLZus8P754qhUW0R7abMWtNw2I83\n\tfzUg=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"n8xptbyk\"; 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:message-id\n\t:reply-to;\n\tbh=vZOCmnTPdZwXiptSc53ZYO8+QH+5K1AhPQbDiyiLE2c=;\n\tb=HuinMI6Lsch2jDsnnLJiyKelXgCZGzAiu4HCIx9eOeAI0IVQEhFf1xtXHS4EIRdpCP\n\tsmZg08qnvill/xL2V53AwwYGsre5JeFqNyyYYg66BA3Bn+xsqsWzdY+FNCHlwboB4JY8\n\t1k55zbPPNNzcsH09kSCxgiCaqc1hKFZGkUThyb77jW3eLmvBLJU9ASO/nNpipc+DM3QW\n\tYGgcLVyknZbHZUd25z4Q2HW/PDndkoedxf45oLuiNhFlagD5l5g6TBGa3SPmqpWJSKDh\n\tWta/SeWG5/U5F1pvZ8Vp6iq4NA7NWKVGVcj6HpZkz4+6i/AihyFtjFlWCgy3Nu5rURQ0\n\tQcNQ==","X-Gm-Message-State":"ACrzQf1WbLxCmgRTm3ZGrdyIypa6KyVBcDo5XJNA8roEq7zoRzJK8mtv\n\tnwPQYT6fIvwy7gm6Ppw9w4KQsC7YV1KLL0xoHcZkcCBM/wY=","X-Google-Smtp-Source":"AMsMyM498EXsbHYTWF70ts9yUV2L8HtbovGhjtNuCxU8lue23ImbMlKmJDfg3Jl4y8WOv6pkHfzMhj0iM09JXtdyb64=","X-Received":"by 2002:a17:907:d15:b0:781:e347:723 with SMTP id\n\tgn21-20020a1709070d1500b00781e3470723mr14854680ejc.723.1665409497722;\n\tMon, 10 Oct 2022 06:44:57 -0700 (PDT)","MIME-Version":"1.0","References":"<20221006131744.5179-1-naush@raspberrypi.com>\n\t<20221006131744.5179-2-naush@raspberrypi.com>\n\t\n\t\n\t<20221010111052.5ofbhwp4irsi6aiq@uno.localdomain>","In-Reply-To":"<20221010111052.5ofbhwp4irsi6aiq@uno.localdomain>","Date":"Mon, 10 Oct 2022 14:44:41 +0100","Message-ID":"","To":"Jacopo Mondi ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v2 01/10] camera_sensor: Add minimum\n\tand maximum 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":"Dave Stevenson via libcamera-devel\n\t","Reply-To":"Dave Stevenson ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":25376,"web_url":"https://patchwork.libcamera.org/comment/25376/","msgid":"<20221010140856.tm3sp64pblqhwmfp@uno.localdomain>","date":"2022-10-10T14:08:56","subject":"Re: [libcamera-devel] [PATCH v2 01/10] camera_sensor: Add minimum\n\tand maximum line length to IPACameraSensorInfo","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Dave\n\nOn Mon, Oct 10, 2022 at 02:44:41PM +0100, Dave Stevenson wrote:\n> Hi Jacopo\n>\n> On Mon, 10 Oct 2022 at 12:10, Jacopo Mondi via libcamera-devel\n> wrote:\n> >\n> > Hi\n> >\n> > On Sat, Oct 08, 2022 at 07:12:55AM +0100, Naushir Patuck via libcamera-devel wrote:\n> > > Hi Laurent,\n> > >\n> > >\n> > > On Fri, 7 Oct 2022 at 23:28, Laurent Pinchart <\n> > > laurent.pinchart@ideasonboard.com> wrote:\n> > >\n> > > > Hi Naush,\n> > > >\n> > > > Thank you for the patch.\n> > > >\n> > > > On Thu, Oct 06, 2022 at 02:17:35PM +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> > > > > Since the IPAs now use minLineLength for their calculations, set the\n> > > > starting\n> > > > > value of the V4L2_CID_HBLANK control to its minimum in\n> > > > CameraSensor::init().\n> > > > >\n> > > > > Signed-off-by: Naushir Patuck \n> > > > > Tested-by: Dave Stevenson \n> > > > > ---\n> > > > > include/libcamera/ipa/core.mojom | 21 +++++++++++++++------\n> > > > > src/ipa/ipu3/ipu3.cpp | 6 ++++--\n> > > > > src/ipa/raspberrypi/raspberrypi.cpp | 2 +-\n> > > > > src/ipa/rkisp1/rkisp1.cpp | 2 +-\n> > > > > src/libcamera/camera_sensor.cpp | 15 +++++++++++++--\n> > > > > 5 files changed, 34 insertions(+), 12 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 b93a09d40c39..7e26fc5639c2 100644\n> > > > > --- a/src/ipa/ipu3/ipu3.cpp\n> > > > > +++ b/src/ipa/ipu3/ipu3.cpp\n> > > > > @@ -336,7 +336,8 @@ 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\n> > > > > + * 1.0s /\n> > > > sensorInfo.pixelRate;\n> > > >\n> > > > The * should be aligned below the =. Same below.\n> > > >\n> > > > >\n> > > > > /* Load the tuning data file. */\n> > > > > File file(settings.configurationFile);\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 algorithms\n> > > > and\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..83f81d655395 100644\n> > > > > --- a/src/libcamera/camera_sensor.cpp\n> > > > > +++ b/src/libcamera/camera_sensor.cpp\n> > > > > @@ -176,6 +176,15 @@ int CameraSensor::init()\n> > > > > if (ret)\n> > > > > return ret;\n> > > > >\n> > > > > + /* Set HBLANK to the minimum as a starting value. */\n> > > >\n> > > > I would capture the rationale here.\n> > > >\n> > > > /*\n> > > > * Set HBLANK to the minimum to start with a well-defined line\n> > > > length,\n> > > > * allowing IPA modules that do not modify HBLANK to use the sensor\n> > > > * minimum line length in their calculations.\n> > > > */\n> > > >\n> > > > Reviewed-by: Laurent Pinchart \n> > > >\n> > > > No need to resubmit if only those small changes are required.\n> > > >\n> > > > > + const ControlInfo hblank = ctrls.infoMap()->at(V4L2_CID_HBLANK);\n> > > > > + ControlList ctrl(subdev_->controls());\n> > > > > +\n> > > > > + ctrl.set(V4L2_CID_HBLANK, hblank.min().get());\n> > > > > + ret = subdev_->setControls(&ctrl);\n> > > > > + if (ret)\n> > > > > + return ret;\n> > > >\n> > >\n> > > Actually, doing the setControls here unconditionally is probably incorrect.\n> > >\n> > > If the control is read-only, we will return an error. We probably want to\n> > > do something like what I did for the IPA where we test for\n> > > HBLANK::min() == HBLANK::max(), and if true, assume the control is\n> > > read-only and don't do the setControl().\n> > >\n> > > Sorry I should have caught that, but my testing was done on sensors\n> > > with adjustable HBLANK controls. I'll update the statement and post\n> > > a revised patch first thing on Monday.\n> > >\n> >\n> > I wonder if we shouldn't use (for hblank as well as per vblank) the\n> > control's default value in the IPA calculations instead of assuming\n> > (and here forcing) the minium value. Specifically, during the\n> > IPA::configure() where we intialize values using the minium value\n> > seems rather an arbitrary assumptions. After IPA::configure() the IPA\n> > should be free to change H/VBlank as they like in the min/max ranges,\n> > but for intiialization...\n> >\n> > Also most drivers (should?) update the blanking values to a sensible\n> > defaul when a new mode is applied. Shouldn't we rely on their\n> > selection ?\n>\n> Is it correct behaviour for a sensor driver to change a r/w HBLANK\n> when changing mode? Changing limits for HBLANK is almost certainly the\n> case, but AIUI generally controls shouldn't change values unless they\n> are then invalid.\n>\n> I'll agree that not updating the value has annoyances when the client\n> doesn't automatically compute and set HBLANK, but conversely updating\n> it is annoying for cases where the app doesn't support it and you have\n> deliberately set it to a long value (any mode change resets your\n> chosen value, despite it being valid).\n>\n\nWe've discussed the same in your review of my ar0521 changes...\nI actually think it makes sense for a driver to reset blankings when a\nnew mode is applied to something sensible, possibily something that\nmaximizes the frame rate ?\n\nYou have a point, that users not aware of this change will suddenly\nfound the blankings to be modified, but at the same time when it comes\nto user-facing features, no changing blankings when changing mode\ncould result in a relevant change in the produced FPS rate..\n\nI do expect an \"uninformed\" userspace not to change blankings, stream\nat a reasonable frame rate, change mode and maintain the same frame\nrate.\n\n\"Informed\" userspace should be capable of chaning blankings as they\nlike, so I don't expect this to impact it.\n\nHowever, I understand this again is on the topic \"userspace knows\neverthing\" vs \"drivers do the right thing\".\n\nAnyway, I think we need to define a policy and align to that, whatever\nit ends up being..\n\n> libcamera always selecting the mode (even if the same) leading to a\n> reset when I was trying to test configurable HBLANK was where I\n> encountered this ambiguity.\n>\n\nI understand, but you're setting HBLANK outside of libcamera and\nexpect that not to be changed. It's annoying for debugging and\ndeveloping, I understand, but isn't it expected that the library takes\nover and overrides pre-existing settings ?\n\n> Dave\n>\n> > > Regards,\n> > > Naush\n> > >\n> > >\n> > >\n> > > > > +\n> > > > > return\n> > > > applyTestPatternMode(controls::draft::TestPatternModeEnum::TestPatternModeOff);\n> > > > > }\n> > > > >\n> > > > > @@ -883,10 +892,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> > > > > 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 15E62C0DA4\n\tfor ;\n\tMon, 10 Oct 2022 14:09:02 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9032F62D65;\n\tMon, 10 Oct 2022 16:09:01 +0200 (CEST)","from relay1-d.mail.gandi.net (relay1-d.mail.gandi.net\n\t[IPv6:2001:4b98:dc4:8::221])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1068462272\n\tfor ;\n\tMon, 10 Oct 2022 16:09:00 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby mail.gandi.net (Postfix) with ESMTPSA id 0D21E240018;\n\tMon, 10 Oct 2022 14:08:58 +0000 (UTC)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1665410941;\n\tbh=nCzuAb/uwgsMleklc1aGtBisjDjSZ/7WOX4uBSAwiX4=;\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=Uabh+ysCnzs4jEW84JD6mqMFvHoaQbA3BkxBsERod8oe3mozpyAthbT6FKmkHarDc\n\tIMOblZ0M7yhlC1myhhETg6bXR3dZjdTJrPvn8yRTKzBrHH1VQ9kTBjSHcCKQ8UE3Pz\n\tZW3kvdLGHpfXfgj6l81KkEHydbpRBA4jJiO4EU3zCbCTZDP4r2+M0+E66rYguxp5ac\n\t5/j5z761mWoKY0/hkoNXzPmaO6XIzpkLZbNJrLw+kDzeh8b95Z6llpjnrjGXKQI9Db\n\t0fIYbT11qOFptEztdP25/NchENFL38TVnhGsikwjd/jnR2/nVNKlaVx9grOJL6ZCj3\n\tvD1oztRASDsVQ==","Date":"Mon, 10 Oct 2022 16:08:56 +0200","To":"Dave Stevenson ","Message-ID":"<20221010140856.tm3sp64pblqhwmfp@uno.localdomain>","References":"<20221006131744.5179-1-naush@raspberrypi.com>\n\t<20221006131744.5179-2-naush@raspberrypi.com>\n\t\n\t\n\t<20221010111052.5ofbhwp4irsi6aiq@uno.localdomain>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v2 01/10] camera_sensor: Add minimum\n\tand maximum 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":"Jacopo Mondi via libcamera-devel ","Reply-To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":25439,"web_url":"https://patchwork.libcamera.org/comment/25439/","msgid":"","date":"2022-10-18T00:07:53","subject":"Re: [libcamera-devel] [PATCH v2 01/10] camera_sensor: Add minimum\n\tand maximum line length to IPACameraSensorInfo","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hello,\n\nOn Mon, Oct 10, 2022 at 04:08:56PM +0200, Jacopo Mondi wrote:\n> On Mon, Oct 10, 2022 at 02:44:41PM +0100, Dave Stevenson wrote:\n> > On Mon, 10 Oct 2022 at 12:10, Jacopo Mondi wrote:\n> > > On Sat, Oct 08, 2022 at 07:12:55AM +0100, Naushir Patuck wrote:\n> > > > On Fri, 7 Oct 2022 at 23:28, Laurent Pinchart wrote:\n> > > > > On Thu, Oct 06, 2022 at 02:17:35PM +0100, Naushir Patuck 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> > > > > > Since the IPAs now use minLineLength for their calculations, set the starting\n> > > > > > value of the V4L2_CID_HBLANK control to its minimum in CameraSensor::init().\n> > > > > >\n> > > > > > Signed-off-by: Naushir Patuck \n> > > > > > Tested-by: Dave Stevenson \n> > > > > > ---\n> > > > > > include/libcamera/ipa/core.mojom | 21 +++++++++++++++------\n> > > > > > src/ipa/ipu3/ipu3.cpp | 6 ++++--\n> > > > > > src/ipa/raspberrypi/raspberrypi.cpp | 2 +-\n> > > > > > src/ipa/rkisp1/rkisp1.cpp | 2 +-\n> > > > > > src/libcamera/camera_sensor.cpp | 15 +++++++++++++--\n> > > > > > 5 files changed, 34 insertions(+), 12 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 b93a09d40c39..7e26fc5639c2 100644\n> > > > > > --- a/src/ipa/ipu3/ipu3.cpp\n> > > > > > +++ b/src/ipa/ipu3/ipu3.cpp\n> > > > > > @@ -336,7 +336,8 @@ 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\n> > > > > > + * 1.0s / sensorInfo.pixelRate;\n> > > > >\n> > > > > The * should be aligned below the =. Same below.\n> > > > >\n> > > > > >\n> > > > > > /* Load the tuning data file. */\n> > > > > > File file(settings.configurationFile);\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> > > > > > 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..83f81d655395 100644\n> > > > > > --- a/src/libcamera/camera_sensor.cpp\n> > > > > > +++ b/src/libcamera/camera_sensor.cpp\n> > > > > > @@ -176,6 +176,15 @@ int CameraSensor::init()\n> > > > > > if (ret)\n> > > > > > return ret;\n> > > > > >\n> > > > > > + /* Set HBLANK to the minimum as a starting value. */\n> > > > >\n> > > > > I would capture the rationale here.\n> > > > >\n> > > > > /*\n> > > > > * Set HBLANK to the minimum to start with a well-defined line length,\n> > > > > * allowing IPA modules that do not modify HBLANK to use the sensor\n> > > > > * minimum line length in their calculations.\n> > > > > */\n> > > > >\n> > > > > Reviewed-by: Laurent Pinchart \n> > > > >\n> > > > > No need to resubmit if only those small changes are required.\n> > > > >\n> > > > > > + const ControlInfo hblank = ctrls.infoMap()->at(V4L2_CID_HBLANK);\n> > > > > > + ControlList ctrl(subdev_->controls());\n> > > > > > +\n> > > > > > + ctrl.set(V4L2_CID_HBLANK, hblank.min().get());\n> > > > > > + ret = subdev_->setControls(&ctrl);\n> > > > > > + if (ret)\n> > > > > > + return ret;\n> > > >\n> > > > Actually, doing the setControls here unconditionally is probably incorrect.\n> > > >\n> > > > If the control is read-only, we will return an error. We probably want to\n> > > > do something like what I did for the IPA where we test for\n> > > > HBLANK::min() == HBLANK::max(), and if true, assume the control is\n> > > > read-only and don't do the setControl().\n> > > >\n> > > > Sorry I should have caught that, but my testing was done on sensors\n> > > > with adjustable HBLANK controls. I'll update the statement and post\n> > > > a revised patch first thing on Monday.\n> > >\n> > > I wonder if we shouldn't use (for hblank as well as per vblank) the\n> > > control's default value in the IPA calculations instead of assuming\n> > > (and here forcing) the minium value. Specifically, during the\n> > > IPA::configure() where we intialize values using the minium value\n> > > seems rather an arbitrary assumptions. After IPA::configure() the IPA\n> > > should be free to change H/VBlank as they like in the min/max ranges,\n> > > but for intiialization...\n\nIt is arbitrary, but I think it will lead to less abitrary results than\nusing the default value reported by the sensor driver. There's no policy\nfor the hblank default in the V4L2 API, so you would essentially get a\ncompletely unpredictable behaviour. With the minimum it gets a bit\nbetter. IPA modules should control hblank in any case, so it shouldn't\nmatter too much what we do here in the long run. This discussion is\nabout the transition, to avoid breaking IPU3 (which Kieran has\nsuccessfully tested on Soraka) and RkISP1 (which I'm about to test).\n\n> > > Also most drivers (should?) update the blanking values to a sensible\n> > > defaul when a new mode is applied. Shouldn't we rely on their\n> > > selection ?\n> >\n> > Is it correct behaviour for a sensor driver to change a r/w HBLANK\n> > when changing mode? Changing limits for HBLANK is almost certainly the\n> > case, but AIUI generally controls shouldn't change values unless they\n> > are then invalid.\n> >\n> > I'll agree that not updating the value has annoyances when the client\n> > doesn't automatically compute and set HBLANK, but conversely updating\n> > it is annoying for cases where the app doesn't support it and you have\n> > deliberately set it to a long value (any mode change resets your\n> > chosen value, despite it being valid).\n> \n> We've discussed the same in your review of my ar0521 changes...\n> I actually think it makes sense for a driver to reset blankings when a\n> new mode is applied to something sensible, possibily something that\n> maximizes the frame rate ?\n> \n> You have a point, that users not aware of this change will suddenly\n> found the blankings to be modified, but at the same time when it comes\n> to user-facing features, no changing blankings when changing mode\n> could result in a relevant change in the produced FPS rate..\n\nNot only in the FPS. Consider a case with two modes, lores and hires. As\nsensors typically express horizontal blanking as a horizontal total\nsize, the hblank control max value will be larger for lores than hires.\nIf the sensor were to set the default hblank to a value valid for lores\nbut not hires, switching from lores -> hires -> lores would change the\neffective hblank value even if userspace doesn't touch the hblank\ncontrol at all.\n\n> I do expect an \"uninformed\" userspace not to change blankings, stream\n> at a reasonable frame rate, change mode and maintain the same frame\n> rate.\n> \n> \"Informed\" userspace should be capable of chaning blankings as they\n> like, so I don't expect this to impact it.\n\nLots of mode-based drivers hardcode the blanking values. This may result\nin the same or different frames rates for different modes, but the frame\nrate is usually a \"standard\" round value (e.g. 25fps, 30fps, 60fps, ...\nnot 34.12 fps). When such a driver is moved to configurable h/v blank,\nif we don't reset the current h/v blank values when changing the mode,\nthe frame rates will indeed become quite random. I thus think resetting\nthe values make sense.\n\nOn the other hand, V4L2 doesn't generally reset controls that have a\nvalid valud on mode change. This could lead to applications getting\nconfused, but I agree that, if an application has set the h/v blank\nvalues explicitly, it should likely be able to set them after a mode set\ntoo, and have full control of the device. The issue raised by Dave below\n(*) is relevant though.\n\n> However, I understand this again is on the topic \"userspace knows\n> everthing\" vs \"drivers do the right thing\".\n> \n> Anyway, I think we need to define a policy and align to that, whatever\n> it ends up being..\n\nAgreed.\n\n> > libcamera always selecting the mode (even if the same) leading to a\n> > reset when I was trying to test configurable HBLANK was where I\n> > encountered this ambiguity.\n> \n> I understand, but you're setting HBLANK outside of libcamera and\n> expect that not to be changed. It's annoying for debugging and\n> developing, I understand, but isn't it expected that the library takes\n> over and overrides pre-existing settings ?\n\n(*) It's not just about libcamera. There are lots of applications that\ndon't touch h/v blank, and being able to set them with a separate tool\n(v4l2-ctl for instance) before running those applications is a useful\nfeature. V4L2 retains control values when a device is closed for this\nvery purpose. It can be useful during development, but also during\nnormal operation. I would argue it was a bit of a historical mistake to\nallow this for complex cameras though, it's a fragile hack at best. The\nquestion here is whether we can disable this hack for blanking controls\nor not. At this point, I would tend to say yes.\n\nI think the h/v blank issues should be discussed on the linux-media\nmailing list, and I'd bring up in the discussion the idea of defining\nnew HTS/VTS controls. How to transition existing sensor drivers without\nbreaking anything would be interesting to explore.\n\n> > > > > > +\n> > > > > > return applyTestPatternMode(controls::draft::TestPatternModeEnum::TestPatternModeOff);\n> > > > > > }\n> > > > > >\n> > > > > > @@ -883,10 +892,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> > > > > > 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 1F9EBBD16B\n\tfor ;\n\tTue, 18 Oct 2022 00:08:20 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 028D662E06;\n\tTue, 18 Oct 2022 02:08:19 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6374961F55\n\tfor ;\n\tTue, 18 Oct 2022 02:08:17 +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 B54368CC;\n\tTue, 18 Oct 2022 02:08:16 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1666051699;\n\tbh=rJFaHU8vHbx/TzQmDzXdDPJ1zNzhL9XlgdYbtP49eMQ=;\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=pOsNyvhKXCrm1XdI2ridFhYof735rcjUCHrSc3PxsBoQVXhKx4p+iqrpKP26wavbE\n\tNXCHmt/RhbIi5TZe7Y947/wzWsFLtsWePEZJUP+buLqP24rphjSe7eU74QbWvzIiOg\n\tF6/yj9VQ/UQBN635JSciW4pPpadQEWadvoKSZ20JLRG2SclfLuG0HAyNms1jw6go91\n\tKgFvq8lx0jiN37ahbKPz1SrZn0DFk8hMQFGgyPuB5t796L593//yWICeuGw32UfQiK\n\tVF03FPz+CL5RG6b3F1IHimJZM13nXWomZnO7xeIpPhHAgHjSYDt9A/HCb4FrTOC+xo\n\t2veDfgafBMT+g==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1666051697;\n\tbh=rJFaHU8vHbx/TzQmDzXdDPJ1zNzhL9XlgdYbtP49eMQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=dXkM3+iq9pPyXaazALNAbiRnvSPQml/TohUlKf0IsTEK8YpYcgXPA5u+imzVyHJ6/\n\te+zrFZbZvYrRAr7BM05/b1n6HxBb/lcAepUjZ5gZcH9OjTntyzY6zCLjdWhNGkuVK8\n\tf5zFVE+64zLCMAiRHD4BexyQ9Cmx1tzSXCtEIcuA="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"dXkM3+iq\"; dkim-atps=neutral","Date":"Tue, 18 Oct 2022 03:07:53 +0300","To":"Jacopo Mondi ","Message-ID":"","References":"<20221006131744.5179-1-naush@raspberrypi.com>\n\t<20221006131744.5179-2-naush@raspberrypi.com>\n\t\n\t\n\t<20221010111052.5ofbhwp4irsi6aiq@uno.localdomain>\n\t\n\t<20221010140856.tm3sp64pblqhwmfp@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20221010140856.tm3sp64pblqhwmfp@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v2 01/10] camera_sensor: Add minimum\n\tand maximum 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\" "}}]