[{"id":15641,"web_url":"https://patchwork.libcamera.org/comment/15641/","msgid":"<20210313083109.bkytd3osqglytcbg@uno.localdomain>","date":"2021-03-13T08:31:09","subject":"Re: [libcamera-devel] [PATCH 1/2] libcamera: ipu3: Move Imgu\n\tconfiguration to IPU3CameraData","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hello Jean-Micheal,\n\nOn Fri, Mar 12, 2021 at 02:03:03PM +0100, Jean-Michel Hautbois wrote:\n> The IPU3 IPA needs to know the BayerDownScaler (BDS) configuration to\n> calculate the statistics grids.\n> This patch makes it possible for PipelineHandlerIPU3::start() to access\n> the BDS configuration and later pass the rectangle to the IPU3 IPA\n> configure method.\n>\n\nThe alternative would be having the pipe configuration retrieved from\nthe ImgU device at start() which would require having the ImgUDevice\nstateful, which will be very painful to implement due to the awful\npipe configuration procedure.\n\nRemoving 'const' from the data_ field doesn't worry me too much, but\nsmells like we're taking a shortcut. I'm fine with that for the time\nbeing.\n\n> Signed-off-by: Jean-Michel Hautbois \n> ---\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 13 +++++++------\n> 1 file changed, 7 insertions(+), 6 deletions(-)\n>\n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index 00da2bb2..0d133031 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -70,6 +70,8 @@ public:\n> \tCIO2Device cio2_;\n> \tImgUDevice *imgu_;\n>\n> +\tImgUDevice::PipeConfig pipeConfig_;\n> +\n> \tStream outStream_;\n> \tStream vfStream_;\n> \tStream rawStream_;\n> @@ -97,7 +99,6 @@ public:\n> \tStatus validate() override;\n>\n> \tconst StreamConfiguration &cio2Format() const { return cio2Configuration_; }\n> -\tconst ImgUDevice::PipeConfig imguConfig() const { return pipeConfig_; }\n>\n> \t/* Cache the combinedTransform_ that will be applied to the sensor */\n> \tTransform combinedTransform_;\n> @@ -108,10 +109,9 @@ private:\n> \t * corresponding Camera instance is valid. In order to borrow a\n> \t * reference to the camera data, store a new reference to the camera.\n> \t */\n> -\tconst IPU3CameraData *data_;\n> +\tIPU3CameraData *data_;\n>\n> \tStreamConfiguration cio2Configuration_;\n> -\tImgUDevice::PipeConfig pipeConfig_;\n> };\n>\n> class PipelineHandlerIPU3 : public PipelineHandler\n> @@ -375,12 +375,13 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()\n>\n> \t/* Only compute the ImgU configuration if a YUV stream has been requested. */\n> \tif (yuvCount) {\n> -\t\tpipeConfig_ = data_->imgu_->calculatePipeConfig(&pipe);\n> -\t\tif (pipeConfig_.isNull()) {\n> +\t\tImgUDevice::PipeConfig imguConfig = data_->imgu_->calculatePipeConfig(&pipe);\n\nWhy do you need to go through an intermediate variable ?\nCan't you assign data_->pipeConfig_ directly here ?\n\nThanks\n j\n\n> +\t\tif (imguConfig.isNull()) {\n> \t\t\tLOG(IPU3, Error) << \"Failed to calculate pipe configuration: \"\n> \t\t\t\t\t << \"unsupported resolutions.\";\n> \t\t\treturn Invalid;\n> \t\t}\n> +\t\tdata_->pipeConfig_ = imguConfig;\n> \t}\n>\n> \treturn status;\n> @@ -576,7 +577,7 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)\n> \t * stream has been requested: return here to skip the ImgU configuration\n> \t * part.\n> \t */\n> -\tImgUDevice::PipeConfig imguConfig = config->imguConfig();\n> +\tImgUDevice::PipeConfig imguConfig = data->pipeConfig_;\n> \tif (imguConfig.isNull())\n> \t\treturn 0;\n>\n> --\n> 2.27.0\n>\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","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 B28EBBD1F1\n\tfor ;\n\tSat, 13 Mar 2021 08:30:40 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2AA4068C67;\n\tSat, 13 Mar 2021 09:30:40 +0100 (CET)","from relay11.mail.gandi.net (relay11.mail.gandi.net\n\t[217.70.178.231])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id EA342602E3\n\tfor ;\n\tSat, 13 Mar 2021 09:30:38 +0100 (CET)","from uno.localdomain (host-79-22-58-175.retail.telecomitalia.it\n\t[79.22.58.175]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay11.mail.gandi.net (Postfix) with ESMTPSA id 1B96C100004;\n\tSat, 13 Mar 2021 08:30:37 +0000 (UTC)"],"Date":"Sat, 13 Mar 2021 09:31:09 +0100","From":"Jacopo Mondi ","To":"Jean-Michel Hautbois ","Message-ID":"<20210313083109.bkytd3osqglytcbg@uno.localdomain>","References":"<20210312130304.34698-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20210312130304.34698-2-jeanmichel.hautbois@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210312130304.34698-2-jeanmichel.hautbois@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 1/2] libcamera: ipu3: Move Imgu\n\tconfiguration to IPU3CameraData","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":15677,"web_url":"https://patchwork.libcamera.org/comment/15677/","msgid":"<64d22214-add0-21f5-19e4-ce9a278b8c29@gmail.com>","date":"2021-03-14T19:19:01","subject":"Re: [libcamera-devel] [PATCH 1/2] libcamera: ipu3: Move Imgu\n\tconfiguration to IPU3CameraData","submitter":{"id":74,"url":"https://patchwork.libcamera.org/api/people/74/","name":"Jean-Michel Hautbois","email":"jeanmichel.hautbois@gmail.com"},"content":"Hi Jacopo,\n\nOn 13/03/2021 09:31, Jacopo Mondi wrote:\n> Hello Jean-Micheal,\n> \n> On Fri, Mar 12, 2021 at 02:03:03PM +0100, Jean-Michel Hautbois wrote:\n>> The IPU3 IPA needs to know the BayerDownScaler (BDS) configuration to\n>> calculate the statistics grids.\n>> This patch makes it possible for PipelineHandlerIPU3::start() to access\n>> the BDS configuration and later pass the rectangle to the IPU3 IPA\n>> configure method.\n>>\n> \n> The alternative would be having the pipe configuration retrieved from\n> the ImgU device at start() which would require having the ImgUDevice\n> stateful, which will be very painful to implement due to the awful\n> pipe configuration procedure.\n> \n> Removing 'const' from the data_ field doesn't worry me too much, but\n> smells like we're taking a shortcut. I'm fine with that for the time\n> being.\n\nYes, there is no real easy straightforward way unfortunately...\n\n>> Signed-off-by: Jean-Michel Hautbois \n>> ---\n>> src/libcamera/pipeline/ipu3/ipu3.cpp | 13 +++++++------\n>> 1 file changed, 7 insertions(+), 6 deletions(-)\n>>\n>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n>> index 00da2bb2..0d133031 100644\n>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n>> @@ -70,6 +70,8 @@ public:\n>> \tCIO2Device cio2_;\n>> \tImgUDevice *imgu_;\n>>\n>> +\tImgUDevice::PipeConfig pipeConfig_;\n>> +\n>> \tStream outStream_;\n>> \tStream vfStream_;\n>> \tStream rawStream_;\n>> @@ -97,7 +99,6 @@ public:\n>> \tStatus validate() override;\n>>\n>> \tconst StreamConfiguration &cio2Format() const { return cio2Configuration_; }\n>> -\tconst ImgUDevice::PipeConfig imguConfig() const { return pipeConfig_; }\n>>\n>> \t/* Cache the combinedTransform_ that will be applied to the sensor */\n>> \tTransform combinedTransform_;\n>> @@ -108,10 +109,9 @@ private:\n>> \t * corresponding Camera instance is valid. In order to borrow a\n>> \t * reference to the camera data, store a new reference to the camera.\n>> \t */\n>> -\tconst IPU3CameraData *data_;\n>> +\tIPU3CameraData *data_;\n>>\n>> \tStreamConfiguration cio2Configuration_;\n>> -\tImgUDevice::PipeConfig pipeConfig_;\n>> };\n>>\n>> class PipelineHandlerIPU3 : public PipelineHandler\n>> @@ -375,12 +375,13 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()\n>>\n>> \t/* Only compute the ImgU configuration if a YUV stream has been requested. */\n>> \tif (yuvCount) {\n>> -\t\tpipeConfig_ = data_->imgu_->calculatePipeConfig(&pipe);\n>> -\t\tif (pipeConfig_.isNull()) {\n>> +\t\tImgUDevice::PipeConfig imguConfig = data_->imgu_->calculatePipeConfig(&pipe);\n> \n> Why do you need to go through an intermediate variable ?\n> Can't you assign data_->pipeConfig_ directly here ?\n\nObviously yes, I can, thanks :-).\n\n> Thanks\n> j\n> \n>> +\t\tif (imguConfig.isNull()) {\n>> \t\t\tLOG(IPU3, Error) << \"Failed to calculate pipe configuration: \"\n>> \t\t\t\t\t << \"unsupported resolutions.\";\n>> \t\t\treturn Invalid;\n>> \t\t}\n>> +\t\tdata_->pipeConfig_ = imguConfig;\n>> \t}\n>>\n>> \treturn status;\n>> @@ -576,7 +577,7 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)\n>> \t * stream has been requested: return here to skip the ImgU configuration\n>> \t * part.\n>> \t */\n>> -\tImgUDevice::PipeConfig imguConfig = config->imguConfig();\n>> +\tImgUDevice::PipeConfig imguConfig = data->pipeConfig_;\n>> \tif (imguConfig.isNull())\n>> \t\treturn 0;\n>>\n>> --\n>> 2.27.0\n>>\n>> _______________________________________________\n>> libcamera-devel mailing list\n>> libcamera-devel@lists.libcamera.org\n>> https://lists.libcamera.org/listinfo/libcamera-devel\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel\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 6A521BD80C\n\tfor ;\n\tSun, 14 Mar 2021 19:19:05 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8B205602E4;\n\tSun, 14 Mar 2021 20:19:04 +0100 (CET)","from mail-wr1-x431.google.com (mail-wr1-x431.google.com\n\t[IPv6:2a00:1450:4864:20::431])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 26822602E3\n\tfor ;\n\tSun, 14 Mar 2021 20:19:03 +0100 (CET)","by mail-wr1-x431.google.com with SMTP id e10so7595045wro.12\n\tfor ;\n\tSun, 14 Mar 2021 12:19:03 -0700 (PDT)","from ?IPv6:2a01:e0a:169:7140:7083:d73a:c175:a9f3?\n\t([2a01:e0a:169:7140:7083:d73a:c175:a9f3])\n\tby smtp.gmail.com with ESMTPSA id\n\tr2sm15776107wrt.8.2021.03.14.12.19.01\n\t(version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128);\n\tSun, 14 Mar 2021 12:19:01 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=gmail.com header.i=@gmail.com\n\theader.b=\"tv4jVeEN\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025;\n\th=subject:to:cc:references:from:message-id:date:user-agent\n\t:mime-version:in-reply-to:content-language:content-transfer-encoding; \n\tbh=w5KZB41OhGUdn7H7aKDUYW8xm7pDLC3nD9m4X+LDtFY=;\n\tb=tv4jVeENLLMKINHIqVDqQoR47id8JmeU5BuRxI6XEacSTX/4Bmu2UabL8lpAFjaV7z\n\tD98x5xKKglOHX9r0lhNoSD1yeR5ywVkAiHiUqvi5ymBe0uZ0QYPAOisqYN/xY1H59VLG\n\tuVed5F2JwDUPhwCvTNzayyFn10IOcKVXi/WSIxQqwHdAGogXdg1KlXS406JoOFfVjhpH\n\ti9fG0pAKDUKnV5nI8UXyi2DBAYda5h3eMAzOkURjCPf/eG6GwXPRnVUZnFodSHWRj0CK\n\tJHjyN0RAS9/F0vmengmcGjfhy+RCy83rkI7Ng/w+146Fz8MG1k7f1e5t+F7U3F+VTinK\n\tyv7A==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:subject:to:cc:references:from:message-id:date\n\t:user-agent:mime-version:in-reply-to:content-language\n\t:content-transfer-encoding;\n\tbh=w5KZB41OhGUdn7H7aKDUYW8xm7pDLC3nD9m4X+LDtFY=;\n\tb=SlfhUy8T34+DDTw4qp1iTClFr9s+M13jR8esHns0dlxvY+Cd+6wbwtF9KMCb2w3BVp\n\tWtK/bLhGT9cLfDMsuFI/MicbxtsIzBXnhFfqSYx7YvmDvcsMJgt4bmRnlQSWTrVsFBDt\n\tBXklp6OkpapuK9Pob6ZorgIuI/3x4U3azu+6znkHTHCXBH9PR445K0vqIuYT2pIDB800\n\tU/N5hCKZfd0xl9I7/bbUtHMOq6Jit7Fm0dmnuSsNI+vMfF8J9rdXzAmNBEWIjg4MHwHH\n\tmQkz4HMC4K76Rgxo7Hz94tD7niJOEwpki0BU2bstqLsF3SgTGI8BxfIGfHIJUMVas23u\n\tNASg==","X-Gm-Message-State":"AOAM5336UUpf6PVskTzViKg9Z4ikup3kiL2/qCS+M43BmxuOdX2eRlAp\n\tgYPnkcN+2TsLGs8xFVd56Fc6B/9ETpNQWw==","X-Google-Smtp-Source":"ABdhPJxRX6AArRnh/2EWXp2F8gMFFCmk0peqK3EfczB7YGOWq8slbQfNguaN5WSVt66cLj8zgJ4caQ==","X-Received":"by 2002:a5d:6989:: with SMTP id\n\tg9mr25250049wru.198.1615749542422; \n\tSun, 14 Mar 2021 12:19:02 -0700 (PDT)","To":"Jacopo Mondi ,\n\tJean-Michel Hautbois ","References":"<20210312130304.34698-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20210312130304.34698-2-jeanmichel.hautbois@ideasonboard.com>\n\t<20210313083109.bkytd3osqglytcbg@uno.localdomain>","From":"Jean-Michel Hautbois ","Message-ID":"<64d22214-add0-21f5-19e4-ce9a278b8c29@gmail.com>","Date":"Sun, 14 Mar 2021 20:19:01 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.7.1","MIME-Version":"1.0","In-Reply-To":"<20210313083109.bkytd3osqglytcbg@uno.localdomain>","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH 1/2] libcamera: ipu3: Move Imgu\n\tconfiguration to IPU3CameraData","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":15690,"web_url":"https://patchwork.libcamera.org/comment/15690/","msgid":"","date":"2021-03-15T02:53:36","subject":"Re: [libcamera-devel] [PATCH 1/2] libcamera: ipu3: Move Imgu\n\tconfiguration to IPU3CameraData","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jean-Michel,\n\nThank you for the patch.\n\nOn Sun, Mar 14, 2021 at 08:19:01PM +0100, Jean-Michel Hautbois wrote:\n> On 13/03/2021 09:31, Jacopo Mondi wrote:\n> > On Fri, Mar 12, 2021 at 02:03:03PM +0100, Jean-Michel Hautbois wrote:\n> >> The IPU3 IPA needs to know the BayerDownScaler (BDS) configuration to\n> >> calculate the statistics grids.\n\nBlank line.\n\n> >> This patch makes it possible for PipelineHandlerIPU3::start() to access\n> >> the BDS configuration and later pass the rectangle to the IPU3 IPA\n> >> configure method.\n> > \n> > The alternative would be having the pipe configuration retrieved from\n> > the ImgU device at start() which would require having the ImgUDevice\n> > stateful, which will be very painful to implement due to the awful\n> > pipe configuration procedure.\n> > \n> > Removing 'const' from the data_ field doesn't worry me too much, but\n> > smells like we're taking a shortcut. I'm fine with that for the time\n> > being.\n> \n> Yes, there is no real easy straightforward way unfortunately...\n> \n> >> Signed-off-by: Jean-Michel Hautbois \n> >> ---\n> >> src/libcamera/pipeline/ipu3/ipu3.cpp | 13 +++++++------\n> >> 1 file changed, 7 insertions(+), 6 deletions(-)\n> >>\n> >> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> >> index 00da2bb2..0d133031 100644\n> >> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> >> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> >> @@ -70,6 +70,8 @@ public:\n> >> \tCIO2Device cio2_;\n> >> \tImgUDevice *imgu_;\n> >>\n> >> +\tImgUDevice::PipeConfig pipeConfig_;\n> >> +\n> >> \tStream outStream_;\n> >> \tStream vfStream_;\n> >> \tStream rawStream_;\n> >> @@ -97,7 +99,6 @@ public:\n> >> \tStatus validate() override;\n> >>\n> >> \tconst StreamConfiguration &cio2Format() const { return cio2Configuration_; }\n> >> -\tconst ImgUDevice::PipeConfig imguConfig() const { return pipeConfig_; }\n> >>\n> >> \t/* Cache the combinedTransform_ that will be applied to the sensor */\n> >> \tTransform combinedTransform_;\n> >> @@ -108,10 +109,9 @@ private:\n> >> \t * corresponding Camera instance is valid. In order to borrow a\n> >> \t * reference to the camera data, store a new reference to the camera.\n> >> \t */\n> >> -\tconst IPU3CameraData *data_;\n> >> +\tIPU3CameraData *data_;\n> >>\n> >> \tStreamConfiguration cio2Configuration_;\n> >> -\tImgUDevice::PipeConfig pipeConfig_;\n> >> };\n> >>\n> >> class PipelineHandlerIPU3 : public PipelineHandler\n> >> @@ -375,12 +375,13 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()\n> >>\n> >> \t/* Only compute the ImgU configuration if a YUV stream has been requested. */\n> >> \tif (yuvCount) {\n> >> -\t\tpipeConfig_ = data_->imgu_->calculatePipeConfig(&pipe);\n> >> -\t\tif (pipeConfig_.isNull()) {\n> >> +\t\tImgUDevice::PipeConfig imguConfig = data_->imgu_->calculatePipeConfig(&pipe);\n> > \n> > Why do you need to go through an intermediate variable ?\n> > Can't you assign data_->pipeConfig_ directly here ?\n> \n> Obviously yes, I can, thanks :-).\n> \n> >> +\t\tif (imguConfig.isNull()) {\n> >> \t\t\tLOG(IPU3, Error) << \"Failed to calculate pipe configuration: \"\n> >> \t\t\t\t\t << \"unsupported resolutions.\";\n> >> \t\t\treturn Invalid;\n> >> \t\t}\n> >> +\t\tdata_->pipeConfig_ = imguConfig;\n\nI'm sorry, that's a no-go. validate() isn't supposed to modify the\ncurrent state of the camera. An application can configure the camera,\ngenerate a new configuration, validate it, and then throw it away. You\nwould end up storing the second configuration here.\n\nYou could instead store the pipe config in IPU3CameraData in\nPipelineHandlerIPU3::configure(), that would be simpler. A better option\nwould be to move the IPA configure() call from\nPipelineHandlerIPU3::start() to PipelineHandlerIPU3::configure().\n\n> >> \t}\n> >>\n> >> \treturn status;\n> >> @@ -576,7 +577,7 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)\n> >> \t * stream has been requested: return here to skip the ImgU configuration\n> >> \t * part.\n> >> \t */\n> >> -\tImgUDevice::PipeConfig imguConfig = config->imguConfig();\n> >> +\tImgUDevice::PipeConfig imguConfig = data->pipeConfig_;\n> >> \tif (imguConfig.isNull())\n> >> \t\treturn 0;\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 285C9BD80C\n\tfor ;\n\tMon, 15 Mar 2021 02:54:14 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9E321602E4;\n\tMon, 15 Mar 2021 03:54:13 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5BC6E602E0\n\tfor ;\n\tMon, 15 Mar 2021 03:54:12 +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 960DE87A;\n\tMon, 15 Mar 2021 03:54:11 +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=\"MDtwMMU4\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1615776851;\n\tbh=dE5aPzxsai8NvZHRjYPk35j6oADXG2XAG0j1201a7Eg=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=MDtwMMU4I8xodMBUyyNZp0ohlMv1HCw+dTlK6MbuBtBzBZdLDSFbhpgyZqMaD/dL/\n\t0NEvYq2lqo/gSvf3C+aXs38LAip9pfFySw5rIfKIMjtt2uxP80XrSwt0wI3h8ApJ9/\n\td3+BFpiWl6VKLW/IfguSn/PGynydWx7jYYyl6KoQ=","Date":"Mon, 15 Mar 2021 04:53:36 +0200","From":"Laurent Pinchart ","To":"Jean-Michel Hautbois ","Message-ID":"","References":"<20210312130304.34698-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20210312130304.34698-2-jeanmichel.hautbois@ideasonboard.com>\n\t<20210313083109.bkytd3osqglytcbg@uno.localdomain>\n\t<64d22214-add0-21f5-19e4-ce9a278b8c29@gmail.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<64d22214-add0-21f5-19e4-ce9a278b8c29@gmail.com>","Subject":"Re: [libcamera-devel] [PATCH 1/2] libcamera: ipu3: Move Imgu\n\tconfiguration to IPU3CameraData","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":15703,"web_url":"https://patchwork.libcamera.org/comment/15703/","msgid":"<1e20148d-5327-83bf-6f39-cdf2975cff41@ideasonboard.com>","date":"2021-03-15T13:08:14","subject":"Re: [libcamera-devel] [PATCH 1/2] libcamera: ipu3: Move Imgu\n\tconfiguration to IPU3CameraData","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi All,\n\nOn 15/03/2021 02:53, Laurent Pinchart wrote:\n> Hi Jean-Michel,\n> \n> Thank you for the patch.\n> \n> On Sun, Mar 14, 2021 at 08:19:01PM +0100, Jean-Michel Hautbois wrote:\n>> On 13/03/2021 09:31, Jacopo Mondi wrote:\n>>> On Fri, Mar 12, 2021 at 02:03:03PM +0100, Jean-Michel Hautbois wrote:\n>>>> The IPU3 IPA needs to know the BayerDownScaler (BDS) configuration to\n>>>> calculate the statistics grids.\n> \n> Blank line.\n> \n>>>> This patch makes it possible for PipelineHandlerIPU3::start() to access\n>>>> the BDS configuration and later pass the rectangle to the IPU3 IPA\n>>>> configure method.\n>>>\n>>> The alternative would be having the pipe configuration retrieved from\n>>> the ImgU device at start() which would require having the ImgUDevice\n>>> stateful, which will be very painful to implement due to the awful\n>>> pipe configuration procedure.\n>>>\n>>> Removing 'const' from the data_ field doesn't worry me too much, but\n>>> smells like we're taking a shortcut. I'm fine with that for the time\n>>> being.\n>>\n>> Yes, there is no real easy straightforward way unfortunately...\n>>\n>>>> Signed-off-by: Jean-Michel Hautbois \n>>>> ---\n>>>> src/libcamera/pipeline/ipu3/ipu3.cpp | 13 +++++++------\n>>>> 1 file changed, 7 insertions(+), 6 deletions(-)\n>>>>\n>>>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n>>>> index 00da2bb2..0d133031 100644\n>>>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n>>>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n>>>> @@ -70,6 +70,8 @@ public:\n>>>> \tCIO2Device cio2_;\n>>>> \tImgUDevice *imgu_;\n>>>>\n>>>> +\tImgUDevice::PipeConfig pipeConfig_;\n>>>> +\n>>>> \tStream outStream_;\n>>>> \tStream vfStream_;\n>>>> \tStream rawStream_;\n>>>> @@ -97,7 +99,6 @@ public:\n>>>> \tStatus validate() override;\n>>>>\n>>>> \tconst StreamConfiguration &cio2Format() const { return cio2Configuration_; }\n>>>> -\tconst ImgUDevice::PipeConfig imguConfig() const { return pipeConfig_; }\n>>>>\n>>>> \t/* Cache the combinedTransform_ that will be applied to the sensor */\n>>>> \tTransform combinedTransform_;\n>>>> @@ -108,10 +109,9 @@ private:\n>>>> \t * corresponding Camera instance is valid. In order to borrow a\n>>>> \t * reference to the camera data, store a new reference to the camera.\n>>>> \t */\n>>>> -\tconst IPU3CameraData *data_;\n>>>> +\tIPU3CameraData *data_;\n>>>>\n>>>> \tStreamConfiguration cio2Configuration_;\n>>>> -\tImgUDevice::PipeConfig pipeConfig_;\n>>>> };\n>>>>\n>>>> class PipelineHandlerIPU3 : public PipelineHandler\n>>>> @@ -375,12 +375,13 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()\n>>>>\n>>>> \t/* Only compute the ImgU configuration if a YUV stream has been requested. */\n>>>> \tif (yuvCount) {\n>>>> -\t\tpipeConfig_ = data_->imgu_->calculatePipeConfig(&pipe);\n>>>> -\t\tif (pipeConfig_.isNull()) {\n>>>> +\t\tImgUDevice::PipeConfig imguConfig = data_->imgu_->calculatePipeConfig(&pipe);\n>>>\n>>> Why do you need to go through an intermediate variable ?\n>>> Can't you assign data_->pipeConfig_ directly here ?\n>>\n>> Obviously yes, I can, thanks :-).\n>>\n>>>> +\t\tif (imguConfig.isNull()) {\n>>>> \t\t\tLOG(IPU3, Error) << \"Failed to calculate pipe configuration: \"\n>>>> \t\t\t\t\t << \"unsupported resolutions.\";\n>>>> \t\t\treturn Invalid;\n>>>> \t\t}\n>>>> +\t\tdata_->pipeConfig_ = imguConfig;\n> \n> I'm sorry, that's a no-go. validate() isn't supposed to modify the\n> current state of the camera. An application can configure the camera,\n> generate a new configuration, validate it, and then throw it away. You\n> would end up storing the second configuration here.\n> \n> You could instead store the pipe config in IPU3CameraData in\n> PipelineHandlerIPU3::configure(), that would be simpler. A better option\n> would be to move the IPA configure() call from\n> PipelineHandlerIPU3::start() to PipelineHandlerIPU3::configure().\n\nI think this is the right way forwards.\n\nOur interfaces throughout should be consistent, so we should configure\nin configure().\n\nIn this instance I think I recall a message from Laurent highlighting\nthat the IPU3 is currently buggy because it calls configure after it has\nstarted which is incorrect.\n\nSo first we need to move the call to configure the IPA before we start\nit, and then ensure we configure everything correctly before starting\nanything ;-)\n\n--\nKieran\n\n> \n>>>> \t}\n>>>>\n>>>> \treturn status;\n>>>> @@ -576,7 +577,7 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)\n>>>> \t * stream has been requested: return here to skip the ImgU configuration\n>>>> \t * part.\n>>>> \t */\n>>>> -\tImgUDevice::PipeConfig imguConfig = config->imguConfig();\n>>>> +\tImgUDevice::PipeConfig imguConfig = data->pipeConfig_;\n>>>> \tif (imguConfig.isNull())\n>>>> \t\treturn 0;\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 44978BD80E\n\tfor ;\n\tMon, 15 Mar 2021 13:08:19 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A7EBB68D41;\n\tMon, 15 Mar 2021 14:08:18 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B7DC668D3E\n\tfor ;\n\tMon, 15 Mar 2021 14:08:17 +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 17999556;\n\tMon, 15 Mar 2021 14:08:17 +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=\"R86pfXdf\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1615813697;\n\tbh=3TRFdCc4AGWEYHJRM1hkVWnfDKDJvTPi2/utn+Y/Ewo=;\n\th=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=R86pfXdfCJKzmwEnXkA+tD0UZzdqOmLk9LavAyx3p9R8XyVMlcKJ198KbVCl5Gmea\n\ttX6RAfO4VusRRknba/8IYxur7v3OfG3AwgIdVFCTH2Kp7ZSClGxoqWjRNRG4q2K16N\n\ttG7OWiV2HeBCl50EWWhqcLq21PMkK5Pn2vymayOc=","To":"Laurent Pinchart ,\n\tJean-Michel Hautbois ","References":"<20210312130304.34698-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20210312130304.34698-2-jeanmichel.hautbois@ideasonboard.com>\n\t<20210313083109.bkytd3osqglytcbg@uno.localdomain>\n\t<64d22214-add0-21f5-19e4-ce9a278b8c29@gmail.com>\n\t","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":"<1e20148d-5327-83bf-6f39-cdf2975cff41@ideasonboard.com>","Date":"Mon, 15 Mar 2021 13:08:14 +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":"","Content-Language":"en-GB","Subject":"Re: [libcamera-devel] [PATCH 1/2] libcamera: ipu3: Move Imgu\n\tconfiguration to IPU3CameraData","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","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\" "}}]