[{"id":15540,"web_url":"https://patchwork.libcamera.org/comment/15540/","msgid":"","date":"2021-03-08T23:29:58","subject":"Re: [libcamera-devel] [PATCH v3 1/5] libcamera: ipu3: Initialize\n\tcontrols using sensor resolution","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nThank you for the patch.\n\nOn Mon, Feb 22, 2021 at 11:52:18AM +0100, Jacopo Mondi wrote:\n> The controls' limits initialized by the IPU3 pipeline handler depend\n> on the sensor configuration. In order to compute controls using a known\n> state apply to the sensor a configuration equal to its own resolution.\n> \n> Signed-off-by: Jacopo Mondi \n> ---\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 26 +++++++++++++-------------\n> 1 file changed, 13 insertions(+), 13 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index b8a655ce0b68..f867b5913b27 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -805,10 +805,21 @@ bool PipelineHandlerIPU3::match(DeviceEnumerator *enumerator)\n> */\n> int PipelineHandlerIPU3::initControls(IPU3CameraData *data)\n> {\n> +\t/*\n> +\t * \\todo The here intialized controls depend on sensor configuration.\n\ns/intialized/initialized/\n\n\"here initialized\" sounds weird to me, would \"The controls initialized\nherein depend on sensor configuration\" be better ?\n\nBut more than that, the todo should describe what needs to be done, not\njust the current state, otherwise we'll wonder in a few months from now\nwhat was meant.\n\nThe rest of the patch looks good, so it's really a minor comment.\n\nReviewed-by: Laurent Pinchart \n\n> +\t *\n> +\t * Initialize the sensor using its resolution and compute the control\n> +\t * limits.\n> +\t */\n> \tCameraSensor *sensor = data->cio2_.sensor();\n> -\tCameraSensorInfo sensorInfo{};\n> +\tV4L2SubdeviceFormat sensorFormat = {};\n> +\tsensorFormat.size = sensor->resolution();\n> +\tint ret = sensor->setFormat(&sensorFormat);\n> +\tif (ret)\n> +\t\treturn ret;\n> \n> -\tint ret = sensor->sensorInfo(&sensorInfo);\n> +\tCameraSensorInfo sensorInfo{};\n> +\tret = sensor->sensorInfo(&sensorInfo);\n> \tif (ret)\n> \t\treturn ret;\n> \n> @@ -851,17 +862,6 @@ int PipelineHandlerIPU3::initControls(IPU3CameraData *data)\n> \t * sensor's full frame as ImgU input).\n> \t */\n> \n> -\t/* Re-fetch the sensor info updated to use the largest resolution. */\n> -\tV4L2SubdeviceFormat sensorFormat = {};\n> -\tsensorFormat.size = sensor->resolution();\n> -\tret = sensor->setFormat(&sensorFormat);\n> -\tif (ret)\n> -\t\treturn ret;\n> -\n> -\tret = sensor->sensorInfo(&sensorInfo);\n> -\tif (ret)\n> -\t\treturn ret;\n> -\n> \t/*\n> \t * The maximum scaler crop rectangle is the analogue crop used to\n> \t * produce the maximum frame size.","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 6F540BD1F1\n\tfor ;\n\tMon, 8 Mar 2021 23:30:32 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E828B68A92;\n\tTue, 9 Mar 2021 00:30:31 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2ACE76051F\n\tfor ;\n\tTue, 9 Mar 2021 00:30:30 +0100 (CET)","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 8711199;\n\tTue, 9 Mar 2021 00:30:29 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"fu7sCG67\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1615246229;\n\tbh=k+W3jj8rPYd0aWPCYP+VYQgYTZ9L1R1VeKnPlOoP0NM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=fu7sCG67ezYlVYKacxQ2T5HjjiKs9AYs4TJw+p5+jHk+xXu8yjg1+6DNU1MzMPYIN\n\tntIt5fWHiCBk/aJw7TN3pWuQydSrmAMeeGvd1zZssTQkRLzBacNfiflRCzci8cM2Ey\n\tmmbzHBz8aK8Hi6/X5K+ImIMMpDiJC/xQh7/m0MIk=","Date":"Tue, 9 Mar 2021 01:29:58 +0200","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20210222105222.393677-1-jacopo@jmondi.org>\n\t<20210222105222.393677-2-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210222105222.393677-2-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH v3 1/5] libcamera: ipu3: Initialize\n\tcontrols using sensor resolution","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","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]