[{"id":13390,"web_url":"https://patchwork.libcamera.org/comment/13390/","msgid":"<20201022025506.GB3942@pendragon.ideasonboard.com>","date":"2020-10-22T02:55:06","subject":"Re: [libcamera-devel] [PATCH v3 09/14] libcamera: ipu3: Register\n\tcamera controls","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 Wed, Oct 21, 2020 at 04:36:30PM +0200, Jacopo Mondi wrote:\n> Register controls for the IPU3 pipeline handler. The only supported\n> Camera control is currently the pipeline depth control.\n> \n> Report the minimum and maximum values the pipeline handler supports\n> for the pipeline processing stages.\n> \n> Reviewed-by: Kieran Bingham \n> Signed-off-by: Jacopo Mondi \n\nHow about squashing this with 10/14 ?\n\n> ---\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 8 ++++++++\n> 1 file changed, 8 insertions(+)\n> \n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index af47739d8d4f..a90683ea523a 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -12,6 +12,7 @@\n> #include \n> \n> #include \n> +#include \n> #include \n> #include \n> #include \n> @@ -40,6 +41,10 @@ static constexpr unsigned int IMGU_OUTPUT_HEIGHT_ALIGN = 4;\n> static constexpr unsigned int IMGU_OUTPUT_WIDTH_MARGIN = 64;\n> static constexpr unsigned int IMGU_OUTPUT_HEIGHT_MARGIN = 32;\n> \n> +static const ControlInfoMap IPU3Controls = {\n> +\t{ &controls::draft::PipelineDepth, ControlInfo(2, 3) },\n> +};\n> +\n> class IPU3CameraData : public CameraData\n> {\n> public:\n> @@ -771,6 +776,9 @@ int PipelineHandlerIPU3::registerCameras()\n> \t\t/* Initialize the camera properties. */\n> \t\tdata->properties_ = cio2->sensor()->properties();\n> \n> +\t\t/* Initialze the camera controls. */\n> +\t\tdata->controlInfo_ = IPU3Controls;\n\nHmmmm... This control can only be reported in metadata, it can't be set\nin requests. This isn't something that needs to be addressed in this\nseries, but do you have any idea on how we should handle that to avoid\ngiving the impression to applications that they can set the pipeline\ndepth ? Or maybe we shouldn't handle this at compile time or runtime,\njust document it ?\n\n> +\n> \t\t/**\n> \t\t * \\todo Dynamically assign ImgU and output devices to each\n> \t\t * stream and camera; as of now, limit support to two cameras","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 008E2BDB13\n\tfor ;\n\tThu, 22 Oct 2020 02:55:54 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6D9AE61059;\n\tThu, 22 Oct 2020 04:55:54 +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 AE24A6034F\n\tfor ;\n\tThu, 22 Oct 2020 04:55:52 +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 1F93153;\n\tThu, 22 Oct 2020 04:55:52 +0200 (CEST)"],"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=\"W7hR9d4V\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1603335352;\n\tbh=z/WI6WV5NI47qPoOS30/TbyD2/zxr9DBo9p0wKZHXe8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=W7hR9d4Vt5wkFlHELEQ33z9ofJ5nx7wTys5zUKy/W1dEe5rnDj+jddKrVP88RcGQa\n\t04YQ6fVv5OC5a7XfhageTE6ssgWWiZk0dUiABERgNDLqpRxFunoeuFJUNp58/qlZ65\n\tERpw+61iEdX2qDINHQUPUsy7udfv68A9Orwg5qrI=","Date":"Thu, 22 Oct 2020 05:55:06 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"<20201022025506.GB3942@pendragon.ideasonboard.com>","References":"<20201021143635.22846-1-jacopo@jmondi.org>\n\t<20201021143635.22846-10-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201021143635.22846-10-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH v3 09/14] libcamera: ipu3: Register\n\tcamera controls","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\" "}},{"id":13447,"web_url":"https://patchwork.libcamera.org/comment/13447/","msgid":"<20201023170645.gahlispgklrtaq2k@uno.localdomain>","date":"2020-10-23T17:06:45","subject":"Re: [libcamera-devel] [PATCH v3 09/14] libcamera: ipu3: Register\n\tcamera controls","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Thu, Oct 22, 2020 at 05:55:06AM +0300, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> Thank you for the patch.\n>\n> On Wed, Oct 21, 2020 at 04:36:30PM +0200, Jacopo Mondi wrote:\n> > Register controls for the IPU3 pipeline handler. The only supported\n> > Camera control is currently the pipeline depth control.\n> >\n> > Report the minimum and maximum values the pipeline handler supports\n> > for the pipeline processing stages.\n> >\n> > Reviewed-by: Kieran Bingham \n> > Signed-off-by: Jacopo Mondi \n>\n> How about squashing this with 10/14 ?\n>\n> > ---\n> > src/libcamera/pipeline/ipu3/ipu3.cpp | 8 ++++++++\n> > 1 file changed, 8 insertions(+)\n> >\n> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > index af47739d8d4f..a90683ea523a 100644\n> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > @@ -12,6 +12,7 @@\n> > #include \n> >\n> > #include \n> > +#include \n> > #include \n> > #include \n> > #include \n> > @@ -40,6 +41,10 @@ static constexpr unsigned int IMGU_OUTPUT_HEIGHT_ALIGN = 4;\n> > static constexpr unsigned int IMGU_OUTPUT_WIDTH_MARGIN = 64;\n> > static constexpr unsigned int IMGU_OUTPUT_HEIGHT_MARGIN = 32;\n> >\n> > +static const ControlInfoMap IPU3Controls = {\n> > +\t{ &controls::draft::PipelineDepth, ControlInfo(2, 3) },\n> > +};\n> > +\n> > class IPU3CameraData : public CameraData\n> > {\n> > public:\n> > @@ -771,6 +776,9 @@ int PipelineHandlerIPU3::registerCameras()\n> > \t\t/* Initialize the camera properties. */\n> > \t\tdata->properties_ = cio2->sensor()->properties();\n> >\n> > +\t\t/* Initialze the camera controls. */\n> > +\t\tdata->controlInfo_ = IPU3Controls;\n>\n> Hmmmm... This control can only be reported in metadata, it can't be set\n> in requests. This isn't something that needs to be addressed in this\n> series, but do you have any idea on how we should handle that to avoid\n> giving the impression to applications that they can set the pipeline\n> depth ? Or maybe we shouldn't handle this at compile time or runtime,\n> just document it ?\n\nWe consciously decided to have a single namespace for controls and\nmetdata as you very well know. So far we've handled that at the\ndocumentation level but I see the need to report to application what\nthey should expect to be able to modify and what can only be inspected\nupon request completion.\n\nThe easiest way possible would be to add a data->metadataInfo_ where\nto report ControlInfo for the metadata. There might be an overlap\nbetween data->controlInfo_, but it might be expected.\n\nThanks\n j\n\n>\n> > +\n> > \t\t/**\n> > \t\t * \\todo Dynamically assign ImgU and output devices to each\n> > \t\t * stream and camera; as of now, limit support to two cameras\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id A999EC3D3C\n\tfor ;\n\tFri, 23 Oct 2020 17:06:49 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 258A6615D3;\n\tFri, 23 Oct 2020 19:06:49 +0200 (CEST)","from relay3-d.mail.gandi.net (relay3-d.mail.gandi.net\n\t[217.70.183.195])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id F394C60350\n\tfor ;\n\tFri, 23 Oct 2020 19:06:47 +0200 (CEST)","from uno.localdomain (93-34-118-233.ip49.fastwebnet.it\n\t[93.34.118.233]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay3-d.mail.gandi.net (Postfix) with ESMTPSA id 0EDF660004;\n\tFri, 23 Oct 2020 17:06:46 +0000 (UTC)"],"X-Originating-IP":"93.34.118.233","Date":"Fri, 23 Oct 2020 19:06:45 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Message-ID":"<20201023170645.gahlispgklrtaq2k@uno.localdomain>","References":"<20201021143635.22846-1-jacopo@jmondi.org>\n\t<20201021143635.22846-10-jacopo@jmondi.org>\n\t<20201022025506.GB3942@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201022025506.GB3942@pendragon.ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v3 09/14] libcamera: ipu3: Register\n\tcamera controls","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\" "}},{"id":13454,"web_url":"https://patchwork.libcamera.org/comment/13454/","msgid":"<20201023202608.GF5979@pendragon.ideasonboard.com>","date":"2020-10-23T20:26:08","subject":"Re: [libcamera-devel] [PATCH v3 09/14] libcamera: ipu3: Register\n\tcamera controls","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Fri, Oct 23, 2020 at 07:06:45PM +0200, Jacopo Mondi wrote:\n> On Thu, Oct 22, 2020 at 05:55:06AM +0300, Laurent Pinchart wrote:\n> > On Wed, Oct 21, 2020 at 04:36:30PM +0200, Jacopo Mondi wrote:\n> > > Register controls for the IPU3 pipeline handler. The only supported\n> > > Camera control is currently the pipeline depth control.\n> > >\n> > > Report the minimum and maximum values the pipeline handler supports\n> > > for the pipeline processing stages.\n> > >\n> > > Reviewed-by: Kieran Bingham \n> > > Signed-off-by: Jacopo Mondi \n> >\n> > How about squashing this with 10/14 ?\n> >\n> > > ---\n> > > src/libcamera/pipeline/ipu3/ipu3.cpp | 8 ++++++++\n> > > 1 file changed, 8 insertions(+)\n> > >\n> > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > index af47739d8d4f..a90683ea523a 100644\n> > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > @@ -12,6 +12,7 @@\n> > > #include \n> > >\n> > > #include \n> > > +#include \n> > > #include \n> > > #include \n> > > #include \n> > > @@ -40,6 +41,10 @@ static constexpr unsigned int IMGU_OUTPUT_HEIGHT_ALIGN = 4;\n> > > static constexpr unsigned int IMGU_OUTPUT_WIDTH_MARGIN = 64;\n> > > static constexpr unsigned int IMGU_OUTPUT_HEIGHT_MARGIN = 32;\n> > >\n> > > +static const ControlInfoMap IPU3Controls = {\n> > > +\t{ &controls::draft::PipelineDepth, ControlInfo(2, 3) },\n> > > +};\n> > > +\n> > > class IPU3CameraData : public CameraData\n> > > {\n> > > public:\n> > > @@ -771,6 +776,9 @@ int PipelineHandlerIPU3::registerCameras()\n> > > \t\t/* Initialize the camera properties. */\n> > > \t\tdata->properties_ = cio2->sensor()->properties();\n> > >\n> > > +\t\t/* Initialze the camera controls. */\n> > > +\t\tdata->controlInfo_ = IPU3Controls;\n> >\n> > Hmmmm... This control can only be reported in metadata, it can't be set\n> > in requests. This isn't something that needs to be addressed in this\n> > series, but do you have any idea on how we should handle that to avoid\n> > giving the impression to applications that they can set the pipeline\n> > depth ? Or maybe we shouldn't handle this at compile time or runtime,\n> > just document it ?\n> \n> We consciously decided to have a single namespace for controls and\n> metdata as you very well know. So far we've handled that at the\n> documentation level but I see the need to report to application what\n> they should expect to be able to modify and what can only be inspected\n> upon request completion.\n> \n> The easiest way possible would be to add a data->metadataInfo_ where\n> to report ControlInfo for the metadata. There might be an overlap\n> between data->controlInfo_, but it might be expected.\n\nThanks for sharing your ideas, that's more or less what I had in mind\ntoo. Let's keep it in mind for later.\n\n> > > +\n> > > \t\t/**\n> > > \t\t * \\todo Dynamically assign ImgU and output devices to each\n> > > \t\t * stream and camera; as of now, limit support to two cameras","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 EE359BDB13\n\tfor ;\n\tFri, 23 Oct 2020 20:26:56 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BDA89619AF;\n\tFri, 23 Oct 2020 22:26:56 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 74E52603C1\n\tfor ;\n\tFri, 23 Oct 2020 22:26:55 +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 0A775B26;\n\tFri, 23 Oct 2020 22:26:54 +0200 (CEST)"],"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=\"dUwFLDDJ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1603484815;\n\tbh=PmVkqh/Fine58kBwUPxUXmree2KfqSEdrM2hUtUudao=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=dUwFLDDJadEz8lwGswLETYRxFUXfXQ5VRHBB0vHr4YYRlqt2e5M/LMkll9+fr5jGW\n\tEb5E/R+fvfWnjbSk7/F9BkxSfM/Fn5SFbFj6vaSA8q5xOdzf3Al9Uf485tYBobIAAq\n\tjryux5s57GXyWDTu6jYL3mW+n/kr1xwfOTpo7aIQ=","Date":"Fri, 23 Oct 2020 23:26:08 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"<20201023202608.GF5979@pendragon.ideasonboard.com>","References":"<20201021143635.22846-1-jacopo@jmondi.org>\n\t<20201021143635.22846-10-jacopo@jmondi.org>\n\t<20201022025506.GB3942@pendragon.ideasonboard.com>\n\t<20201023170645.gahlispgklrtaq2k@uno.localdomain>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201023170645.gahlispgklrtaq2k@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v3 09/14] libcamera: ipu3: Register\n\tcamera controls","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\" "}}]