[{"id":15217,"web_url":"https://patchwork.libcamera.org/comment/15217/","msgid":"<963a49a0-f91e-27ca-1a61-f1e2bf8916ac@ideasonboard.com>","date":"2021-02-18T11:35:27","subject":"Re: [libcamera-devel] [PATCH 3/3] pipeline: raspberrypi: Update\n\tlens shading control in-place","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Naush,\n\nOn 17/02/2021 10:08, Naushir Patuck wrote:\n> Add the dmabuf file descriptor to the lens shading control in-place\n> rather than taking a copy.\n> \n> Signed-off-by: Naushir Patuck \n> ---\n> src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 11 ++++++-----\n> 1 file changed, 6 insertions(+), 5 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> index 9dd4d112a907..a60415d93705 100644\n> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> @@ -1338,13 +1338,14 @@ void RPiCameraData::embeddedComplete(uint32_t bufferId)\n> \n> void RPiCameraData::setIspControls(const ControlList &controls)\n> {\n> -\tControlList ctrls = controls;\n> +\tControlList ctrls = std::move(controls);\n\nOh, can we do that on a const ControlList reference ?\nDoesn't that by definition modify the parameter that came in?\n\n\n> \n> \tif (ctrls.contains(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING)) {\n> -\t\tSpan s =\n> -\t\t\tctrls.get(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING).data();\n> -\t\tconst bcm2835_isp_lens_shading *ls =\n> -\t\t\treinterpret_cast(s.data());\n> +\t\tControlValue &value =\n> +\t\t\tconst_cast(ctrls.get(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING));\n\nCould you take a look at Laurent's V4L2 control series he posted recently?\n\n [PATCH/RFC 0/2] libcamera: Add Control instances for V4L2 controls\n\nBy putting the types into the Control, I think it helps simplify a lot\nof code, but I'm curious how it would affect controls which map to\nstructure arrays like this...\n\n> +\t\tSpan s = value.data();\n> +\t\tbcm2835_isp_lens_shading *ls =\n> +\t\t\treinterpret_cast(s.data());\n> \t\tls->dmabuf = lsTable_.fd();\n> \t}\n\nNow that you've moved controls to the local ctrls, when it reaches the\nend of this function, it would go out of scope and release the control\nlist won't it ?\n\nDoes that mean your now potentially pointing to a lens-shading table\nthat has been released? or is that not an issue?\n\n(Or is it used later in this function which the mail has not provided\ncontext for ?...)\n\n\n\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 16267BD1F1\n\tfor ;\n\tThu, 18 Feb 2021 11:35:33 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DC1B56835F;\n\tThu, 18 Feb 2021 12:35:32 +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 ED8CF637DA\n\tfor ;\n\tThu, 18 Feb 2021 12:35:30 +0100 (CET)","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 87A5F3E7;\n\tThu, 18 Feb 2021 12:35:30 +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=\"P0tUzusM\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1613648130;\n\tbh=Az8u/DFC48a8yuCwkiu81mkYJOh4rwbzz2zCUhxANqE=;\n\th=Reply-To:Subject:To:References:From:Date:In-Reply-To:From;\n\tb=P0tUzusMnDUx2EUmTi0yAeaHvWxHoygL135401XZcEuyxeDfWeDlP/j5B4BfyLmcW\n\t3DyhjwuCB6m8eNG2j2apdaPcTfAgc3CO80P+HzNDssaj6uTX21o7tqSR9Mz3eNN3/O\n\tIWB5LlEhgp8Izu5OiaKPhKD0Rdb/7z/VAJpxv5x8=","To":"Naushir Patuck ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20210217100852.1542397-1-naush@raspberrypi.com>\n\t<20210217100852.1542397-3-naush@raspberrypi.com>","From":"Kieran Bingham ","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAlcEEwEKAEECGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEWIQSQLdeYP70o/eNy1HqhHkZyEKRh/QUCXWTtygUJ\n\tCyJXZAAKCRChHkZyEKRh/f8dEACTDsbLN2nioNZMwyLuQRUAFcXNolDX48xcUXsWS2QjxaPm\n\tVsJx8Uy8aYkS85mdPBh0C83OovQR/OVbr8AxhGvYqBs3nQvbWuTl/+4od7DfK2VZOoKBAu5S\n\tQK2FYuUcikDqYcFWJ8DQnubxfE8dvzojHEkXw0sA4igINHDDFX3HJGZtLio+WpEFQtCbfTAG\n\tYZslasz1YZRbwEdSsmO3/kqy5eMnczlm8a21A3fKUo3g8oAZEFM+f4DUNzqIltg31OAB/kZS\n\tenKZQ/SWC8PmLg/ZXBrReYakxXtkP6w3FwMlzOlhGxqhIRNiAJfXJBaRhuUWzPOpEDE9q5YJ\n\tBmqQL2WJm1VSNNVxbXJHpaWMH1sA2R00vmvRrPXGwyIO0IPYeUYQa3gsy6k+En/aMQJd27dp\n\taScf9am9PFICPY5T4ppneeJLif2lyLojo0mcHOV+uyrds9XkLpp14GfTkeKPdPMrLLTsHRfH\n\tfA4I4OBpRrEPiGIZB/0im98MkGY/Mu6qxeZmYLCcgD6qz4idOvfgVOrNh+aA8HzIVR+RMW8H\n\tQGBN9f0E3kfwxuhl3omo6V7lDw8XOdmuWZNC9zPq1UfryVHANYbLGz9KJ4Aw6M+OgBC2JpkD\n\thXMdHUkC+d20dwXrwHTlrJi1YNp6rBc+xald3wsUPOZ5z8moTHUX/uPA/qhGsbkCDQRWBP1m\n\tARAAzijkb+Sau4hAncr1JjOY+KyFEdUNxRy+hqTJdJfaYihxyaj0Ee0P0zEi35CbE6lgU0Uz\n\ttih9fiUbSV3wfsWqg1Ut3/5rTKu7kLFp15kF7eqvV4uezXRD3Qu4yjv/rMmEJbbD4cTvGCYI\n\td6MDC417f7vK3hCbCVIZSp3GXxyC1LU+UQr3fFcOyCwmP9vDUR9JV0BSqHHxRDdpUXE26Dk6\n\tmhf0V1YkspE5St814ETXpEus2urZE5yJIUROlWPIL+hm3NEWfAP06vsQUyLvr/GtbOT79vXl\n\tEn1aulcYyu20dRRxhkQ6iILaURcxIAVJJKPi8dsoMnS8pB0QW12AHWuirPF0g6DiuUfPmrA5\n\tPKe56IGlpkjc8cO51lIxHkWTpCMWigRdPDexKX+Sb+W9QWK/0JjIc4t3KBaiG8O4yRX8ml2R\n\t+rxfAVKM6V769P/hWoRGdgUMgYHFpHGSgEt80OKK5HeUPy2cngDUXzwrqiM5Sz6Od0qw5pCk\n\tNlXqI0W/who0iSVM+8+RmyY0OEkxEcci7rRLsGnM15B5PjLJjh1f2ULYkv8s4SnDwMZ/kE04\n\t/UqCMK/KnX8pwXEMCjz0h6qWNpGwJ0/tYIgQJZh6bqkvBrDogAvuhf60Sogw+mH8b+PBlx1L\n\toeTK396wc+4c3BfiC6pNtUS5GpsPMMjYMk7kVvEAEQEAAYkCPAQYAQoAJgIbDBYhBJAt15g/\n\tvSj943LUeqEeRnIQpGH9BQJdizzIBQkLSKZiAAoJEKEeRnIQpGH9eYgQAJpjaWNgqNOnMTmD\n\tMJggbwjIotypzIXfhHNCeTkG7+qCDlSaBPclcPGYrTwCt0YWPU2TgGgJrVhYT20ierN8LUvj\n\t6qOPTd+Uk7NFzL65qkh80ZKNBFddx1AabQpSVQKbdcLb8OFs85kuSvFdgqZwgxA1vl4TFhNz\n\tPZ79NAmXLackAx3sOVFhk4WQaKRshCB7cSl+RIng5S/ThOBlwNlcKG7j7W2MC06BlTbdEkUp\n\tECzuuRBv8wX4OQl+hbWbB/VKIx5HKlLu1eypen/5lNVzSqMMIYkkZcjV2SWQyUGxSwq0O/sx\n\tS0A8/atCHUXOboUsn54qdxrVDaK+6jIAuo8JiRWctP16KjzUM7MO0/+4zllM8EY57rXrj48j\n\tsbEYX0YQnzaj+jO6kJtoZsIaYR7rMMq9aUAjyiaEZpmP1qF/2sYenDx0Fg2BSlLvLvXM0vU8\n\tpQk3kgDu7kb/7PRYrZvBsr21EIQoIjXbZxDz/o7z95frkP71EaICttZ6k9q5oxxA5WC6sTXc\n\tMW8zs8avFNuA9VpXt0YupJd2ijtZy2mpZNG02fFVXhIn4G807G7+9mhuC4XG5rKlBBUXTvPU\n\tAfYnB4JBDLmLzBFavQfvonSfbitgXwCG3vS+9HEwAjU30Bar1PEOmIbiAoMzuKeRm2LVpmq4\n\tWZw01QYHU/GUV/zHJSFk","Organization":"Ideas on Board","Message-ID":"<963a49a0-f91e-27ca-1a61-f1e2bf8916ac@ideasonboard.com>","Date":"Thu, 18 Feb 2021 11:35:27 +0000","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101\n\tThunderbird/68.10.0","MIME-Version":"1.0","In-Reply-To":"<20210217100852.1542397-3-naush@raspberrypi.com>","Content-Language":"en-GB","Subject":"Re: [libcamera-devel] [PATCH 3/3] pipeline: raspberrypi: Update\n\tlens shading control in-place","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","Reply-To":"kieran.bingham@ideasonboard.com","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":15220,"web_url":"https://patchwork.libcamera.org/comment/15220/","msgid":"","date":"2021-02-18T12:01:21","subject":"Re: [libcamera-devel] [PATCH 3/3] pipeline: raspberrypi: Update\n\tlens shading control in-place","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Kieran,\n\nOn Thu, 18 Feb 2021 at 11:35, Kieran Bingham <\nkieran.bingham@ideasonboard.com> wrote:\n\n> Hi Naush,\n>\n> On 17/02/2021 10:08, Naushir Patuck wrote:\n> > Add the dmabuf file descriptor to the lens shading control in-place\n> > rather than taking a copy.\n> >\n> > Signed-off-by: Naushir Patuck \n> > ---\n> > src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 11 ++++++-----\n> > 1 file changed, 6 insertions(+), 5 deletions(-)\n> >\n> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > index 9dd4d112a907..a60415d93705 100644\n> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > @@ -1338,13 +1338,14 @@ void RPiCameraData::embeddedComplete(uint32_t\n> bufferId)\n> >\n> > void RPiCameraData::setIspControls(const ControlList &controls)\n> > {\n> > - ControlList ctrls = controls;\n> > + ControlList ctrls = std::move(controls);\n>\n> Oh, can we do that on a const ControlList reference ?\n> Doesn't that by definition modify the parameter that came in?\n>\n\nThe compiler does not seem to complain, so I assume this works as we are\nnot (yet) modifying the elements of the map container.\n\n\n>\n>\n> >\n> > if (ctrls.contains(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING)) {\n> > - Span s =\n> > -\n> ctrls.get(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING).data();\n> > - const bcm2835_isp_lens_shading *ls =\n> > - reinterpret_cast *>(s.data());\n> > + ControlValue &value =\n> > + const_cast &>(ctrls.get(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING));\n>\n> Could you take a look at Laurent's V4L2 control series he posted recently?\n>\n> [PATCH/RFC 0/2] libcamera: Add Control instances for V4L2 controls\n>\n> By putting the types into the Control, I think it helps simplify a lot\n> of code, but I'm curious how it would affect controls which map to\n> structure arrays like this...\n>\n\nSure, will have a look at that.\n\n\n>\n> > + Span s = value.data();\n> > + bcm2835_isp_lens_shading *ls =\n> > + reinterpret_cast *>(s.data());\n> > ls->dmabuf = lsTable_.fd();\n> > }\n>\n> Now that you've moved controls to the local ctrls, when it reaches the\n> end of this function, it would go out of scope and release the control\n> list won't it ?\n>\n\nYes, the local ctrls will be out of scope at that point, but the call to\nsetControls() would have copied the\nbcm2835_isp_lens_shading struct internally, so that is not an issue.\n\n\n>\n> Does that mean your now potentially pointing to a lens-shading table\n> that has been released? or is that not an issue?\n>\n\nThe bcm2835_isp_lens_shading does not hold the table directly, rather a\nhandle to the dmabuf\nwhere the table is actually stored. So again, this is not an issue.\n\n\n>\n> (Or is it used later in this function which the mail has not provided\n> context for ?...)\n>\n\nThe setControls() call is the last line of the patch, so you may have\nmissed that :-)\n\nHowever, if you do feel this is still wrong, please shout!\n\n\n>\n>\n>\n> >\n> >\n>\n> --\n> Regards\n> --\n> Kieran\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 0CD27BD1F1\n\tfor ;\n\tThu, 18 Feb 2021 12:01:40 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7B58A683C6;\n\tThu, 18 Feb 2021 13:01:39 +0100 (CET)","from mail-lf1-x130.google.com (mail-lf1-x130.google.com\n\t[IPv6:2a00:1450:4864:20::130])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1F6676816A\n\tfor ;\n\tThu, 18 Feb 2021 13:01:38 +0100 (CET)","by mail-lf1-x130.google.com with SMTP id u4so6260420lfs.0\n\tfor ;\n\tThu, 18 Feb 2021 04:01:38 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"FsKrPioZ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=vGv2omLRuOgxFXt7nutRd5ll4/T19Cvb+S2LRr7xv/A=;\n\tb=FsKrPioZc6w5sdLg5/jKadyj+h+VhVAxR4E0yMdn9fTpdx/v519Mtqxo9GEzbwMvDH\n\tvyZnACPiWoDRCUVds3D3RUJRk4piOZbUBaMVatT9qEbMtAXB3I1V+QKGtDpMHJR6FaTW\n\tmY6jsLzQo4J4wBMRTvVdpFk9VR/fMdqtlWc+hUzJ9/rtYQusUPqty1JKfl6Whrqrqsfc\n\tePPpU8DPBP2IidU4c6dbFDUX6wWslL/gjhelIqtcZ4nv4oUQI4txS9167BHkWLv46b/z\n\tqvrs1DXfic4GgsPjh/gR1QtKzY76VCWqaQJymf0raLymq9hu51WaD2H99NjHvY2CPTqT\n\tJtfQ==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=vGv2omLRuOgxFXt7nutRd5ll4/T19Cvb+S2LRr7xv/A=;\n\tb=ZrgvRrdvphOF0413VLqi30BZbLYTU9YTO2aYjRD05w5DrIvaCoac9vNY78fVuSSo0W\n\tWtr0djdQILi8pnroSANGeTw5XqMESeYLTqdCYNVapuyFAN68DWTrxsfHaz8tw8VJZjdq\n\tFwkcqanCorxKHLzrtO/cwkOZjm9F+HDiCZE9w7668kuW1UYSyPckx2sHc73etswkbbjP\n\t+otAifNB79zhkv8HkwVMaoBqYtPRLm1hiBwABDRvt6T7fiJ3P0sUXLL1ucwGFN9kXmSP\n\t4cnNo2u/hgeN658aoJW9tVlnMmiBFo1mE5c3aQhl8m4xWZEeUKTJ7iILhE6T+16A3WL+\n\tkFjQ==","X-Gm-Message-State":"AOAM531Q+aAGvZKanF0kbqmw2rEawgMiP8xgQDW4oZ4o4clvdFOZl69n\n\t/kJoTP1g6IOapC3j+LFBFkJxHzUxJY4nSS1+2XpzmEDXI2fhuWfp","X-Google-Smtp-Source":"ABdhPJwaTfglAMfDe1JqF6QZx4F3aPmQc7VAjNz3OhjG4WrMUZJ/n4lvRoYihqOk+RSYVWjh245KXudX8L87QKzTrww=","X-Received":"by 2002:a19:cd1:: with SMTP id 200mr1009239lfm.171.1613649697527;\n\tThu, 18 Feb 2021 04:01:37 -0800 (PST)","MIME-Version":"1.0","References":"<20210217100852.1542397-1-naush@raspberrypi.com>\n\t<20210217100852.1542397-3-naush@raspberrypi.com>\n\t<963a49a0-f91e-27ca-1a61-f1e2bf8916ac@ideasonboard.com>","In-Reply-To":"<963a49a0-f91e-27ca-1a61-f1e2bf8916ac@ideasonboard.com>","From":"Naushir Patuck ","Date":"Thu, 18 Feb 2021 12:01:21 +0000","Message-ID":"","To":"Kieran Bingham ","Subject":"Re: [libcamera-devel] [PATCH 3/3] pipeline: raspberrypi: Update\n\tlens shading control in-place","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 ","Content-Type":"multipart/mixed;\n\tboundary=\"===============5814648712710290586==\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]