[{"id":19611,"web_url":"https://patchwork.libcamera.org/comment/19611/","msgid":"<48d176f5-ce3d-7475-b864-755a9975ab95@ideasonboard.com>","date":"2021-09-10T15:57:52","subject":"Re: [libcamera-devel] [RFC PATCH] ipa: ipu3: Clear incoming\n\tparameter use flags","submitter":{"id":75,"url":"https://patchwork.libcamera.org/api/people/75/","name":"Jean-Michel Hautbois","email":"jeanmichel.hautbois@ideasonboard.com"},"content":"Hi Kieran,\n\nOn 10/09/2021 17:49, Kieran Bingham wrote:\n> The incoming params buffer may contain uninitialised data, or the\n> parameters of previously queued frames. Clearing the entire buffer\n> may be an expensive operation, and the kernel will only read from\n> structures which have their associated use-flag set.\n> \n> It is the responsibility of the algorithms to set the use flags\n> accordingly for any data structure they update during prepare().\n> \n> Signed-off-by: Kieran Bingham \n\nThanks for the patch !\nReviewed-by: Jean-Michel Hautbois \nTested-by: Jean-Michel Hautbois \n\n> \n> ---\n> \n> Yes, the commit message is the same as the comment before the line.\n> I felt the text was worthy of documenting the clearing of the flags, and\n> ensuring that it's documented in the code that the algorithms are\n> responsible for setting their use flag of any structure they modify.\n> \n> Note that this is sent compile tested only, as it's something I noticed,\n> while writing documentation and wanted to check.\n> \n> src/ipa/ipu3/ipu3.cpp | 11 +++++++++++\n> 1 file changed, 11 insertions(+)\n> \n> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> index c925cf642611..30d2a53903ec 100644\n> --- a/src/ipa/ipu3/ipu3.cpp\n> +++ b/src/ipa/ipu3/ipu3.cpp\n> @@ -457,6 +457,17 @@ void IPAIPU3::processControls([[maybe_unused]] unsigned int frame,\n> \n> void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params)\n> {\n> +\t/*\n> +\t * The incoming params buffer may contain uninitialised data, or the\n> +\t * parameters of previously queued frames. Clearing the entire buffer\n> +\t * may be an expensive operation, and the kernel will only read from\n> +\t * structures which have their associated use-flag set.\n> +\t *\n> +\t * It is the responsibility of the algorithms to set the use flags\n> +\t * accordingly for any data structure they update during prepare().\n> +\t */\n> +\tparams->use = {};\n> +\n> \tfor (auto const &algo : algorithms_)\n> \t\talgo->prepare(context_, params);\n> \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 DD9DDBDB1D\n\tfor ;\n\tFri, 10 Sep 2021 15:57:57 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5A91669170;\n\tFri, 10 Sep 2021 17:57:57 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 48BAB69169\n\tfor ;\n\tFri, 10 Sep 2021 17:57:56 +0200 (CEST)","from tatooine.ideasonboard.com (unknown\n\t[IPv6:2a01:e0a:169:7140:b830:1690:884b:271c])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id D5947883;\n\tFri, 10 Sep 2021 17:57:55 +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=\"p6FrspuR\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1631289475;\n\tbh=L6XLhNYAGAXVERkrjYLt8iRVjcBCpaP06ixkkMQKgJY=;\n\th=Subject:To:References:From:Date:In-Reply-To:From;\n\tb=p6FrspuRUo896jxp+4Okl1bfBEGmXKD2orarIwtjezJRMCi0VWTlZsDtqhShKY7qm\n\tk8DbNKa9D9hrrU9845I8CUWf++JdqJZD0DfTyLr5Ia3YLocGbRvBuXL1eXRgj/4PVx\n\tdwUr0Oo646MKyEKBqhvE6PdMvwz58S76Rhci2htc=","To":"Kieran Bingham ,\n\tlibcamera devel ","References":"<20210910154924.117857-1-kieran.bingham@ideasonboard.com>","From":"Jean-Michel Hautbois ","Message-ID":"<48d176f5-ce3d-7475-b864-755a9975ab95@ideasonboard.com>","Date":"Fri, 10 Sep 2021 17:57:52 +0200","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.13.0","MIME-Version":"1.0","In-Reply-To":"<20210910154924.117857-1-kieran.bingham@ideasonboard.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-US","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [RFC PATCH] ipa: ipu3: Clear incoming\n\tparameter use flags","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19617,"web_url":"https://patchwork.libcamera.org/comment/19617/","msgid":"","date":"2021-09-10T16:24:12","subject":"Re: [libcamera-devel] [RFC PATCH] ipa: ipu3: Clear incoming\n\tparameter use flags","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"On 10/09/2021 16:49, Kieran Bingham wrote:\n> The incoming params buffer may contain uninitialised data, or the\n> parameters of previously queued frames. Clearing the entire buffer\n> may be an expensive operation, and the kernel will only read from\n> structures which have their associated use-flag set.\n> \n> It is the responsibility of the algorithms to set the use flags\n> accordingly for any data structure they update during prepare().\n\nTo extend this commit message from the introduction:\n\n\"Clear the use flags of the parameter buffer before passing the buffer\nto the algorithms during their prepare() operations.\"\n\n\n> \n> Signed-off-by: Kieran Bingham \n> \n> ---\n> \n> Yes, the commit message is the same as the comment before the line.\n> I felt the text was worthy of documenting the clearing of the flags, and\n> ensuring that it's documented in the code that the algorithms are\n> responsible for setting their use flag of any structure they modify.\n> \n> Note that this is sent compile tested only, as it's something I noticed,\n> while writing documentation and wanted to check.\n> \n> src/ipa/ipu3/ipu3.cpp | 11 +++++++++++\n> 1 file changed, 11 insertions(+)\n> \n> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> index c925cf642611..30d2a53903ec 100644\n> --- a/src/ipa/ipu3/ipu3.cpp\n> +++ b/src/ipa/ipu3/ipu3.cpp\n> @@ -457,6 +457,17 @@ void IPAIPU3::processControls([[maybe_unused]] unsigned int frame,\n> \n> void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params)\n> {\n> +\t/*\n> +\t * The incoming params buffer may contain uninitialised data, or the\n> +\t * parameters of previously queued frames. Clearing the entire buffer\n> +\t * may be an expensive operation, and the kernel will only read from\n> +\t * structures which have their associated use-flag set.\n> +\t *\n> +\t * It is the responsibility of the algorithms to set the use flags\n> +\t * accordingly for any data structure they update during prepare().\n> +\t */\n> +\tparams->use = {};\n> +\n> \tfor (auto const &algo : algorithms_)\n> \t\talgo->prepare(context_, params);\n> \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 5AA18BDB1D\n\tfor ;\n\tFri, 10 Sep 2021 16:24:17 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 248906917C;\n\tFri, 10 Sep 2021 18:24:17 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E71A469169\n\tfor ;\n\tFri, 10 Sep 2021 18:24:14 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 6FC5A883;\n\tFri, 10 Sep 2021 18:24:14 +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=\"nolGjIrV\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1631291054;\n\tbh=428JC19v9M8sLg5+yiMcNOUlSXN6kRpdOVC3+irLOtk=;\n\th=From:To:References:Subject:Date:In-Reply-To:From;\n\tb=nolGjIrVfDIcOtmlK7YJdVTvKf9Lbltd179CjXqVRuegyNO5DP9Ir4MdQaihz/p1T\n\tTTWZAQC1sWa7ijFYDmkkOemeWzXdpyHw9Xmlhg9CATYL7zEwU02UIWO+7G7LD6kSfh\n\tSnWIL2M1lz6OjBRR6djTKy85jy/dqyb0PnsEUD3Q=","From":"Kieran Bingham ","To":"libcamera devel ,\n\tJean-Michel Hautbois ","References":"<20210910154924.117857-1-kieran.bingham@ideasonboard.com>","Message-ID":"","Date":"Fri, 10 Sep 2021 17:24:12 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.11.0","MIME-Version":"1.0","In-Reply-To":"<20210910154924.117857-1-kieran.bingham@ideasonboard.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [RFC PATCH] ipa: ipu3: Clear incoming\n\tparameter use flags","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19619,"web_url":"https://patchwork.libcamera.org/comment/19619/","msgid":"","date":"2021-09-10T16:38:31","subject":"Re: [libcamera-devel] [RFC PATCH] ipa: ipu3: Clear incoming\n\tparameter use flags","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nThank you for the patch.\n\nOn Fri, Sep 10, 2021 at 05:24:12PM +0100, Kieran Bingham wrote:\n> On 10/09/2021 16:49, Kieran Bingham wrote:\n> > The incoming params buffer may contain uninitialised data, or the\n> > parameters of previously queued frames. Clearing the entire buffer\n> > may be an expensive operation, and the kernel will only read from\n> > structures which have their associated use-flag set.\n> > \n> > It is the responsibility of the algorithms to set the use flags\n> > accordingly for any data structure they update during prepare().\n> \n> To extend this commit message from the introduction:\n> \n> \"Clear the use flags of the parameter buffer before passing the buffer\n> to the algorithms during their prepare() operations.\"\n>\n> > Signed-off-by: Kieran Bingham \n\nReviewed-by: Laurent Pinchart \n\nBut... this has led me to check ipu3_uapi_flags. It's defined as\nfollows:\n\nstruct ipu3_uapi_flags {\n\t__u32 gdc:1;\n\t__u32 obgrid:1;\n\t__u32 reserved1:30;\n\n\t__u32 acc_bnr:1;\n\t__u32 acc_green_disparity:1;\n\t__u32 acc_dm:1;\n\t__u32 acc_ccm:1;\n\t__u32 acc_gamma:1;\n\t__u32 acc_csc:1;\n\t__u32 acc_cds:1;\n\t__u32 acc_shd:1;\n\t__u32 reserved2:2;\n\t__u32 acc_iefd:1;\n\t__u32 acc_yds_c0:1;\n\t__u32 acc_chnr_c0:1;\n\t__u32 acc_y_ee_nr:1;\n\t__u32 acc_yds:1;\n\t__u32 acc_chnr:1;\n\t__u32 acc_ytm:1;\n\t__u32 acc_yds2:1;\n\t__u32 acc_tcc:1;\n\t__u32 acc_dpc:1;\n\t__u32 acc_bds:1;\n\t__u32 acc_anr:1;\n\t__u32 acc_awb_fr:1;\n\t__u32 acc_ae:1;\n\t__u32 acc_af:1;\n\t__u32 acc_awb:1;\n\t__u32 reserved3:4;\n\n\t__u32 lin_vmem_params:1;\n\t__u32 tnr3_vmem_params:1;\n\t__u32 xnr3_vmem_params:1;\n\t__u32 tnr3_dmem_params:1;\n\t__u32 xnr3_dmem_params:1;\n\t__u32 reserved4:1;\n\t__u32 obgrid_param:1;\n\t__u32 reserved5:25;\n} __attribute__((packed));\n\nThe first group spans 32 bits (1+1+32). So does the last group\n(1+1+1+1+1+1+1+25). Guess how many bits the second group spans? Hint:\nit's not 32. I wonder if this operates as expected.\n\n> > ---\n> > \n> > Yes, the commit message is the same as the comment before the line.\n> > I felt the text was worthy of documenting the clearing of the flags, and\n> > ensuring that it's documented in the code that the algorithms are\n> > responsible for setting their use flag of any structure they modify.\n> > \n> > Note that this is sent compile tested only, as it's something I noticed,\n> > while writing documentation and wanted to check.\n> > \n> > src/ipa/ipu3/ipu3.cpp | 11 +++++++++++\n> > 1 file changed, 11 insertions(+)\n> > \n> > diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> > index c925cf642611..30d2a53903ec 100644\n> > --- a/src/ipa/ipu3/ipu3.cpp\n> > +++ b/src/ipa/ipu3/ipu3.cpp\n> > @@ -457,6 +457,17 @@ void IPAIPU3::processControls([[maybe_unused]] unsigned int frame,\n> > \n> > void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params)\n> > {\n> > +\t/*\n> > +\t * The incoming params buffer may contain uninitialised data, or the\n> > +\t * parameters of previously queued frames. Clearing the entire buffer\n> > +\t * may be an expensive operation, and the kernel will only read from\n> > +\t * structures which have their associated use-flag set.\n> > +\t *\n> > +\t * It is the responsibility of the algorithms to set the use flags\n> > +\t * accordingly for any data structure they update during prepare().\n> > +\t */\n> > +\tparams->use = {};\n> > +\n> > \tfor (auto const &algo : algorithms_)\n> > \t\talgo->prepare(context_, params);\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 11ACEBDB1D\n\tfor ;\n\tFri, 10 Sep 2021 16:38:55 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 68B8F6917D;\n\tFri, 10 Sep 2021 18:38: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 A797E69169\n\tfor ;\n\tFri, 10 Sep 2021 18:38: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 152D2883;\n\tFri, 10 Sep 2021 18:38:53 +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=\"b0eXz8dh\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1631291933;\n\tbh=rbKhbzmO76eApO8tkNoihWRVCYeiqtnGx0Le/Fq2mxs=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=b0eXz8dh8omyqMrs5NjPiDJ1NHpi57U3NreNqKx+PlDftCZR1i+zNd+TUEtAEvbp1\n\t2k6eQ5jc5m2ey2M4BJzktt2hSumxCYs20YbtbikmIFAPqXXaw3NwtfRxqQ5mdbEr8o\n\tQQl/HcsImTIxaABF1H+yEVmY5VOxqUk08pluHRRs=","Date":"Fri, 10 Sep 2021 19:38:31 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Message-ID":"","References":"<20210910154924.117857-1-kieran.bingham@ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [RFC PATCH] ipa: ipu3: Clear incoming\n\tparameter use flags","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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19620,"web_url":"https://patchwork.libcamera.org/comment/19620/","msgid":"<1d8f23f8-3cc3-1cd8-bb3f-18e1e6cf4c99@ideasonboard.com>","date":"2021-09-10T20:05:42","subject":"Re: [libcamera-devel] [RFC PATCH] ipa: ipu3: Clear incoming\n\tparameter use flags","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Laurent,\n\nOn 10/09/2021 17:38, Laurent Pinchart wrote:\n> Hi Kieran,\n> \n> Thank you for the patch.\n> \n> On Fri, Sep 10, 2021 at 05:24:12PM +0100, Kieran Bingham wrote:\n>> On 10/09/2021 16:49, Kieran Bingham wrote:\n>>> The incoming params buffer may contain uninitialised data, or the\n>>> parameters of previously queued frames. Clearing the entire buffer\n>>> may be an expensive operation, and the kernel will only read from\n>>> structures which have their associated use-flag set.\n>>>\n>>> It is the responsibility of the algorithms to set the use flags\n>>> accordingly for any data structure they update during prepare().\n>>\n>> To extend this commit message from the introduction:\n>>\n>> \"Clear the use flags of the parameter buffer before passing the buffer\n>> to the algorithms during their prepare() operations.\"\n>>\n>>> Signed-off-by: Kieran Bingham \n> \n> Reviewed-by: Laurent Pinchart \n> \n> But... this has led me to check ipu3_uapi_flags. It's defined as\n> follows:\n> \n> struct ipu3_uapi_flags {\n> \t__u32 gdc:1;\n> \t__u32 obgrid:1;\n> \t__u32 reserved1:30;\n> \n> \t__u32 acc_bnr:1;\n> \t__u32 acc_green_disparity:1;\n> \t__u32 acc_dm:1;\n> \t__u32 acc_ccm:1;\n> \t__u32 acc_gamma:1;\n> \t__u32 acc_csc:1;\n> \t__u32 acc_cds:1;\n> \t__u32 acc_shd:1;\n> \t__u32 reserved2:2;\n> \t__u32 acc_iefd:1;\n> \t__u32 acc_yds_c0:1;\n> \t__u32 acc_chnr_c0:1;\n> \t__u32 acc_y_ee_nr:1;\n> \t__u32 acc_yds:1;\n> \t__u32 acc_chnr:1;\n> \t__u32 acc_ytm:1;\n> \t__u32 acc_yds2:1;\n> \t__u32 acc_tcc:1;\n> \t__u32 acc_dpc:1;\n> \t__u32 acc_bds:1;\n> \t__u32 acc_anr:1;\n> \t__u32 acc_awb_fr:1;\n> \t__u32 acc_ae:1;\n> \t__u32 acc_af:1;\n> \t__u32 acc_awb:1;\n> \t__u32 reserved3:4;\n> \n> \t__u32 lin_vmem_params:1;\n> \t__u32 tnr3_vmem_params:1;\n> \t__u32 xnr3_vmem_params:1;\n> \t__u32 tnr3_dmem_params:1;\n> \t__u32 xnr3_dmem_params:1;\n> \t__u32 reserved4:1;\n> \t__u32 obgrid_param:1;\n> \t__u32 reserved5:25;\n> } __attribute__((packed));\n\npahole gives:\n\n> struct ipu3_uapi_flags {\n> \t__u32 gdc:1; /* 0: 0 4 */\n> \t__u32 obgrid:1; /* 0: 1 4 */\n> \t__u32 reserved1:30; /* 0: 2 4 */\n> \t__u32 acc_bnr:1; /* 4: 0 4 */\n> \t__u32 acc_green_disparity:1; /* 4: 1 4 */\n> \t__u32 acc_dm:1; /* 4: 2 4 */\n> \t__u32 acc_ccm:1; /* 4: 3 4 */\n> \t__u32 acc_gamma:1; /* 4: 4 4 */\n> \t__u32 acc_csc:1; /* 4: 5 4 */\n> \t__u32 acc_cds:1; /* 4: 6 4 */\n> \t__u32 acc_shd:1; /* 4: 7 4 */\n> \t__u32 reserved2:2; /* 4: 8 4 */\n> \t__u32 acc_iefd:1; /* 4:10 4 */\n> \t__u32 acc_yds_c0:1; /* 4:11 4 */\n> \t__u32 acc_chnr_c0:1; /* 4:12 4 */\n> \t__u32 acc_y_ee_nr:1; /* 4:13 4 */\n> \t__u32 acc_yds:1; /* 4:14 4 */\n> \t__u32 acc_chnr:1; /* 4:15 4 */\n> \t__u32 acc_ytm:1; /* 4:16 4 */\n> \t__u32 acc_yds2:1; /* 4:17 4 */\n> \t__u32 acc_tcc:1; /* 4:18 4 */\n> \t__u32 acc_dpc:1; /* 4:19 4 */\n> \t__u32 acc_bds:1; /* 4:20 4 */\n> \t__u32 acc_anr:1; /* 4:21 4 */\n> \t__u32 acc_awb_fr:1; /* 4:22 4 */\n> \t__u32 acc_ae:1; /* 4:23 4 */\n> \t__u32 acc_af:1; /* 4:24 4 */\n> \t__u32 acc_awb:1; /* 4:25 4 */\n> \t__u32 reserved3:4; /* 4:26 4 */\n> \t__u32 lin_vmem_params:1; /* 4:30 4 */\n> \t__u32 tnr3_vmem_params:1; /* 4:31 4 */\n> \t__u32 xnr3_vmem_params:1; /* 8: 0 4 */\n> \t__u32 tnr3_dmem_params:1; /* 8: 1 4 */\n> \t__u32 xnr3_dmem_params:1; /* 8: 2 4 */\n> \t__u32 reserved4:1; /* 8: 3 4 */\n> \t__u32 obgrid_param:1; /* 8: 4 4 */\n> \t__u32 reserved5:25; /* 8: 5 4 */\n> \n> \t/* size: 12, cachelines: 1, members: 37 */\n> \t/* bit_padding: 2 bits */\n> \t/* last cacheline: 12 bytes */\n> };\n\nand it's used in the params structure:\n\n> struct ipu3_uapi_params {\n> \tstruct ipu3_uapi_flags use __attribute__((__aligned__(32))); /* 0 12 */\n> \tstruct ipu3_uapi_acc_param acc_param __attribute__((__aligned__(1))); /* 12 37408 */\n> \n> \t/* XXX last struct has 12 bytes of padding */\n> \n> \t/* --- cacheline 584 boundary (37376 bytes) was 44 bytes ago --- */\n\n\nSo indeed, it's visibly only 12 bytes\n\n>\n> The first group spans 32 bits (1+1+32). So does the last group\n> (1+1+1+1+1+1+1+25). Guess how many bits the second group spans? Hint:\n> it's not 32. I wonder if this operates as expected.\n\nThis structure is read by the kernel, and userspace, not hardware as far\nas I'm aware.\n\nSo both sides will be using the same offsets/alignments and padding to\nparse the structures.\n\nWhat part do you anticipate is not operating as expected?\n\nThere are 2 bits of padding exposed by the structure. Is that what you\nare worried about?\n\nI would indeed expect that last reserved5 to be reserved5:27 if someone\nwas trying to explicitly pad, but as long as the structures are\nidentical on the user and kernel side they should use the same offsets.\n\n--\nKieran\n\n\n\n\n> \n>>> ---\n>>>\n>>> Yes, the commit message is the same as the comment before the line.\n>>> I felt the text was worthy of documenting the clearing of the flags, and\n>>> ensuring that it's documented in the code that the algorithms are\n>>> responsible for setting their use flag of any structure they modify.\n>>>\n>>> Note that this is sent compile tested only, as it's something I noticed,\n>>> while writing documentation and wanted to check.\n>>>\n>>> src/ipa/ipu3/ipu3.cpp | 11 +++++++++++\n>>> 1 file changed, 11 insertions(+)\n>>>\n>>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n>>> index c925cf642611..30d2a53903ec 100644\n>>> --- a/src/ipa/ipu3/ipu3.cpp\n>>> +++ b/src/ipa/ipu3/ipu3.cpp\n>>> @@ -457,6 +457,17 @@ void IPAIPU3::processControls([[maybe_unused]] unsigned int frame,\n>>> \n>>> void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params)\n>>> {\n>>> +\t/*\n>>> +\t * The incoming params buffer may contain uninitialised data, or the\n>>> +\t * parameters of previously queued frames. Clearing the entire buffer\n>>> +\t * may be an expensive operation, and the kernel will only read from\n>>> +\t * structures which have their associated use-flag set.\n>>> +\t *\n>>> +\t * It is the responsibility of the algorithms to set the use flags\n>>> +\t * accordingly for any data structure they update during prepare().\n>>> +\t */\n>>> +\tparams->use = {};\n>>> +\n>>> \tfor (auto const &algo : algorithms_)\n>>> \t\talgo->prepare(context_, params);\n>>> \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 9CE6ABDC71\n\tfor ;\n\tFri, 10 Sep 2021 20:05:48 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E161E6916A;\n\tFri, 10 Sep 2021 22:05:47 +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 5F1B669169\n\tfor ;\n\tFri, 10 Sep 2021 22:05:46 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id C9133883;\n\tFri, 10 Sep 2021 22:05:45 +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=\"jPuMOgea\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1631304346;\n\tbh=vIdFNQgzCkuHVdG0W0uBfUVgCSuyrp8ulsNm+rJvCwg=;\n\th=From:To:Cc:References:Subject:Date:In-Reply-To:From;\n\tb=jPuMOgeadLp6f6fMl1Dl3SbmM1YP457j1fulDUuM8k2KkggMxl6f3dWG8It+FJWoK\n\t+QCmN/CwhCXkhM7NQWbwsD+iZasvFXzgY6u5XI+lVZXoucD1hrgMJmeTCdVIYduS5/\n\tmqL8vYrC51F/71MnoQAICGhQ5jToYEZzJ9ePk5jo=","From":"Kieran Bingham ","To":"Laurent Pinchart ","References":"<20210910154924.117857-1-kieran.bingham@ideasonboard.com>\n\t\n\t","Message-ID":"<1d8f23f8-3cc3-1cd8-bb3f-18e1e6cf4c99@ideasonboard.com>","Date":"Fri, 10 Sep 2021 21:05:42 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.11.0","MIME-Version":"1.0","In-Reply-To":"","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [RFC PATCH] ipa: ipu3: Clear incoming\n\tparameter use flags","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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19621,"web_url":"https://patchwork.libcamera.org/comment/19621/","msgid":"","date":"2021-09-10T20:15:23","subject":"Re: [libcamera-devel] [RFC PATCH] ipa: ipu3: Clear incoming\n\tparameter use flags","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nOn Fri, Sep 10, 2021 at 09:05:42PM +0100, Kieran Bingham wrote:\n> On 10/09/2021 17:38, Laurent Pinchart wrote:\n> > On Fri, Sep 10, 2021 at 05:24:12PM +0100, Kieran Bingham wrote:\n> >> On 10/09/2021 16:49, Kieran Bingham wrote:\n> >>> The incoming params buffer may contain uninitialised data, or the\n> >>> parameters of previously queued frames. Clearing the entire buffer\n> >>> may be an expensive operation, and the kernel will only read from\n> >>> structures which have their associated use-flag set.\n> >>>\n> >>> It is the responsibility of the algorithms to set the use flags\n> >>> accordingly for any data structure they update during prepare().\n> >>\n> >> To extend this commit message from the introduction:\n> >>\n> >> \"Clear the use flags of the parameter buffer before passing the buffer\n> >> to the algorithms during their prepare() operations.\"\n> >>\n> >>> Signed-off-by: Kieran Bingham \n> > \n> > Reviewed-by: Laurent Pinchart \n> > \n> > But... this has led me to check ipu3_uapi_flags. It's defined as\n> > follows:\n> > \n> > struct ipu3_uapi_flags {\n> > \t__u32 gdc:1;\n> > \t__u32 obgrid:1;\n> > \t__u32 reserved1:30;\n> > \n> > \t__u32 acc_bnr:1;\n> > \t__u32 acc_green_disparity:1;\n> > \t__u32 acc_dm:1;\n> > \t__u32 acc_ccm:1;\n> > \t__u32 acc_gamma:1;\n> > \t__u32 acc_csc:1;\n> > \t__u32 acc_cds:1;\n> > \t__u32 acc_shd:1;\n> > \t__u32 reserved2:2;\n> > \t__u32 acc_iefd:1;\n> > \t__u32 acc_yds_c0:1;\n> > \t__u32 acc_chnr_c0:1;\n> > \t__u32 acc_y_ee_nr:1;\n> > \t__u32 acc_yds:1;\n> > \t__u32 acc_chnr:1;\n> > \t__u32 acc_ytm:1;\n> > \t__u32 acc_yds2:1;\n> > \t__u32 acc_tcc:1;\n> > \t__u32 acc_dpc:1;\n> > \t__u32 acc_bds:1;\n> > \t__u32 acc_anr:1;\n> > \t__u32 acc_awb_fr:1;\n> > \t__u32 acc_ae:1;\n> > \t__u32 acc_af:1;\n> > \t__u32 acc_awb:1;\n> > \t__u32 reserved3:4;\n> > \n> > \t__u32 lin_vmem_params:1;\n> > \t__u32 tnr3_vmem_params:1;\n> > \t__u32 xnr3_vmem_params:1;\n> > \t__u32 tnr3_dmem_params:1;\n> > \t__u32 xnr3_dmem_params:1;\n> > \t__u32 reserved4:1;\n> > \t__u32 obgrid_param:1;\n> > \t__u32 reserved5:25;\n> > } __attribute__((packed));\n> \n> pahole gives:\n> \n> > struct ipu3_uapi_flags {\n> > \t__u32 gdc:1; /* 0: 0 4 */\n> > \t__u32 obgrid:1; /* 0: 1 4 */\n> > \t__u32 reserved1:30; /* 0: 2 4 */\n> > \t__u32 acc_bnr:1; /* 4: 0 4 */\n> > \t__u32 acc_green_disparity:1; /* 4: 1 4 */\n> > \t__u32 acc_dm:1; /* 4: 2 4 */\n> > \t__u32 acc_ccm:1; /* 4: 3 4 */\n> > \t__u32 acc_gamma:1; /* 4: 4 4 */\n> > \t__u32 acc_csc:1; /* 4: 5 4 */\n> > \t__u32 acc_cds:1; /* 4: 6 4 */\n> > \t__u32 acc_shd:1; /* 4: 7 4 */\n> > \t__u32 reserved2:2; /* 4: 8 4 */\n> > \t__u32 acc_iefd:1; /* 4:10 4 */\n> > \t__u32 acc_yds_c0:1; /* 4:11 4 */\n> > \t__u32 acc_chnr_c0:1; /* 4:12 4 */\n> > \t__u32 acc_y_ee_nr:1; /* 4:13 4 */\n> > \t__u32 acc_yds:1; /* 4:14 4 */\n> > \t__u32 acc_chnr:1; /* 4:15 4 */\n> > \t__u32 acc_ytm:1; /* 4:16 4 */\n> > \t__u32 acc_yds2:1; /* 4:17 4 */\n> > \t__u32 acc_tcc:1; /* 4:18 4 */\n> > \t__u32 acc_dpc:1; /* 4:19 4 */\n> > \t__u32 acc_bds:1; /* 4:20 4 */\n> > \t__u32 acc_anr:1; /* 4:21 4 */\n> > \t__u32 acc_awb_fr:1; /* 4:22 4 */\n> > \t__u32 acc_ae:1; /* 4:23 4 */\n> > \t__u32 acc_af:1; /* 4:24 4 */\n> > \t__u32 acc_awb:1; /* 4:25 4 */\n> > \t__u32 reserved3:4; /* 4:26 4 */\n> > \t__u32 lin_vmem_params:1; /* 4:30 4 */\n> > \t__u32 tnr3_vmem_params:1; /* 4:31 4 */\n> > \t__u32 xnr3_vmem_params:1; /* 8: 0 4 */\n> > \t__u32 tnr3_dmem_params:1; /* 8: 1 4 */\n> > \t__u32 xnr3_dmem_params:1; /* 8: 2 4 */\n> > \t__u32 reserved4:1; /* 8: 3 4 */\n> > \t__u32 obgrid_param:1; /* 8: 4 4 */\n> > \t__u32 reserved5:25; /* 8: 5 4 */\n> > \n> > \t/* size: 12, cachelines: 1, members: 37 */\n> > \t/* bit_padding: 2 bits */\n> > \t/* last cacheline: 12 bytes */\n> > };\n> \n> and it's used in the params structure:\n> \n> > struct ipu3_uapi_params {\n> > \tstruct ipu3_uapi_flags use __attribute__((__aligned__(32))); /* 0 12 */\n> > \tstruct ipu3_uapi_acc_param acc_param __attribute__((__aligned__(1))); /* 12 37408 */\n> > \n> > \t/* XXX last struct has 12 bytes of padding */\n> > \n> > \t/* --- cacheline 584 boundary (37376 bytes) was 44 bytes ago --- */\n> \n> So indeed, it's visibly only 12 bytes\n> \n> > The first group spans 32 bits (1+1+32). So does the last group\n> > (1+1+1+1+1+1+1+25). Guess how many bits the second group spans? Hint:\n> > it's not 32. I wonder if this operates as expected.\n> \n> This structure is read by the kernel, and userspace, not hardware as far\n> as I'm aware.\n> \n> So both sides will be using the same offsets/alignments and padding to\n> parse the structures.\n> \n> What part do you anticipate is not operating as expected?\n\nAh I was missing that part. I think the structure was intended to store\nlin_vmem_params in the first bit of the last u32. But if it's not parsed\nby hardware (or firmware), it's probably harmless.\n\n> There are 2 bits of padding exposed by the structure. Is that what you\n> are worried about?\n> \n> I would indeed expect that last reserved5 to be reserved5:27 if someone\n> was trying to explicitly pad, but as long as the structures are\n> identical on the user and kernel side they should use the same offsets.\n\nI'd expect reserved3 to be 6 bits long instead. We could fix it, but\nthat will break the ABI.\n\n> >>> ---\n> >>>\n> >>> Yes, the commit message is the same as the comment before the line.\n> >>> I felt the text was worthy of documenting the clearing of the flags, and\n> >>> ensuring that it's documented in the code that the algorithms are\n> >>> responsible for setting their use flag of any structure they modify.\n> >>>\n> >>> Note that this is sent compile tested only, as it's something I noticed,\n> >>> while writing documentation and wanted to check.\n> >>>\n> >>> src/ipa/ipu3/ipu3.cpp | 11 +++++++++++\n> >>> 1 file changed, 11 insertions(+)\n> >>>\n> >>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> >>> index c925cf642611..30d2a53903ec 100644\n> >>> --- a/src/ipa/ipu3/ipu3.cpp\n> >>> +++ b/src/ipa/ipu3/ipu3.cpp\n> >>> @@ -457,6 +457,17 @@ void IPAIPU3::processControls([[maybe_unused]] unsigned int frame,\n> >>> \n> >>> void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params)\n> >>> {\n> >>> +\t/*\n> >>> +\t * The incoming params buffer may contain uninitialised data, or the\n> >>> +\t * parameters of previously queued frames. Clearing the entire buffer\n> >>> +\t * may be an expensive operation, and the kernel will only read from\n> >>> +\t * structures which have their associated use-flag set.\n> >>> +\t *\n> >>> +\t * It is the responsibility of the algorithms to set the use flags\n> >>> +\t * accordingly for any data structure they update during prepare().\n> >>> +\t */\n> >>> +\tparams->use = {};\n> >>> +\n> >>> \tfor (auto const &algo : algorithms_)\n> >>> \t\talgo->prepare(context_, params);\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 B73B7BDB1D\n\tfor ;\n\tFri, 10 Sep 2021 20:15:48 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 105BF6917D;\n\tFri, 10 Sep 2021 22:15:48 +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 4648F69169\n\tfor ;\n\tFri, 10 Sep 2021 22:15:46 +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 A6B13883;\n\tFri, 10 Sep 2021 22:15:45 +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=\"rXk+3RMK\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1631304945;\n\tbh=0aKyfhtEzRMNVGEeW3pi5LWr3wvPwWD1RD+NyR5iKf8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=rXk+3RMKUmVSdIryik3njrV6XbxuhRDyaesVVz93VTAcMY76+FwDY2argH9LbGKcb\n\t83/v3PlhMbSqLNL2vKx6bW6ijsI+3R4lBGZuChexlT43oRpOAfOPO8NG3QvlP5b1IT\n\tcBUwE4SOwx1RDQCLlBjNuRmrNQWNOeLM4Vw8rnmQ=","Date":"Fri, 10 Sep 2021 23:15:23 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Message-ID":"","References":"<20210910154924.117857-1-kieran.bingham@ideasonboard.com>\n\t\n\t\n\t<1d8f23f8-3cc3-1cd8-bb3f-18e1e6cf4c99@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<1d8f23f8-3cc3-1cd8-bb3f-18e1e6cf4c99@ideasonboard.com>","Subject":"Re: [libcamera-devel] [RFC PATCH] ipa: ipu3: Clear incoming\n\tparameter use flags","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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]