[{"id":15721,"web_url":"https://patchwork.libcamera.org/comment/15721/","msgid":"<20210316154548.zcu5j2iin344l7xv@uno.localdomain>","date":"2021-03-16T15:45:48","subject":"Re: [libcamera-devel] [PATCH v4 1/2] libcamera: ipu3: Move ipa\n\tconfiguration from start() to configure()","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hello Jean-Michel,\n\nOn Tue, Mar 16, 2021 at 04:40:22PM +0100, Jean-Michel Hautbois wrote:\n> IPA was configured after all the pipeline devices were started,\n> including IPA itself...\n\ns/..././ :)\n\n> Move it at the end of configure() call.\n>\n> Signed-off-by: Jean-Michel Hautbois \n\nReviewed-by: Jacopo Mondi \n\nThanks\n j\n\n> ---\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 8 ++++----\n> 1 file changed, 4 insertions(+), 4 deletions(-)\n>\n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index 2ea13ec9..bb61ef4a 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -633,6 +633,10 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)\n> \t\treturn ret;\n> \t}\n>\n> +\tstd::map entityControls;\n> +\tentityControls.emplace(0, data->cio2_.sensor()->controls());\n> +\tdata->ipa_->configure(entityControls);\n> +\n> \treturn 0;\n> }\n>\n> @@ -717,7 +721,6 @@ int PipelineHandlerIPU3::freeBuffers(Camera *camera)\n>\n> int PipelineHandlerIPU3::start(Camera *camera, [[maybe_unused]] const ControlList *controls)\n> {\n> -\tstd::map entityControls;\n> \tIPU3CameraData *data = cameraData(camera);\n> \tCIO2Device *cio2 = &data->cio2_;\n> \tImgUDevice *imgu = data->imgu_;\n> @@ -744,9 +747,6 @@ int PipelineHandlerIPU3::start(Camera *camera, [[maybe_unused]] const ControlLis\n> \tif (ret)\n> \t\tgoto error;\n>\n> -\tentityControls.emplace(0, data->cio2_.sensor()->controls());\n> -\tdata->ipa_->configure(entityControls);\n> -\n> \treturn 0;\n>\n> error:\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 6A905BD80C\n\tfor ;\n\tTue, 16 Mar 2021 15:45:20 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E363568D4C;\n\tTue, 16 Mar 2021 16:45:19 +0100 (CET)","from relay4-d.mail.gandi.net (relay4-d.mail.gandi.net\n\t[217.70.183.196])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 473C568D48\n\tfor ;\n\tTue, 16 Mar 2021 16:45:18 +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 relay4-d.mail.gandi.net (Postfix) with ESMTPSA id 7A2B5E0016;\n\tTue, 16 Mar 2021 15:45:17 +0000 (UTC)"],"X-Originating-IP":"79.22.58.175","Date":"Tue, 16 Mar 2021 16:45:48 +0100","From":"Jacopo Mondi ","To":"Jean-Michel Hautbois ","Message-ID":"<20210316154548.zcu5j2iin344l7xv@uno.localdomain>","References":"<20210316154023.46033-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20210316154023.46033-2-jeanmichel.hautbois@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210316154023.46033-2-jeanmichel.hautbois@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v4 1/2] libcamera: ipu3: Move ipa\n\tconfiguration from start() to configure()","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":15730,"web_url":"https://patchwork.libcamera.org/comment/15730/","msgid":"","date":"2021-03-16T20:55:27","subject":"Re: [libcamera-devel] [PATCH v4 1/2] libcamera: ipu3: Move ipa\n\tconfiguration from start() to configure()","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 Tue, Mar 16, 2021 at 04:40:22PM +0100, Jean-Michel Hautbois wrote:\n> IPA was configured after all the pipeline devices were started,\n> including IPA itself...\n> Move it at the end of configure() call.\n> \n> Signed-off-by: Jean-Michel Hautbois \n\nThe change looks good. Could you make sure it doesn't break anything if\nwe call configure() twice without any start() in-between ? This check\nshould be added to lc-compliance (once we get it merged), in the\nmeantime, you can modify any test application to duplicate the\nconfigure() call. Bonus points if this can be run under valgrind too.\n\nOnce done,\n\nReviewed-by: Laurent Pinchart \n\n> ---\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 8 ++++----\n> 1 file changed, 4 insertions(+), 4 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index 2ea13ec9..bb61ef4a 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -633,6 +633,10 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)\n> \t\treturn ret;\n> \t}\n> \n> +\tstd::map entityControls;\n> +\tentityControls.emplace(0, data->cio2_.sensor()->controls());\n> +\tdata->ipa_->configure(entityControls);\n> +\n> \treturn 0;\n> }\n> \n> @@ -717,7 +721,6 @@ int PipelineHandlerIPU3::freeBuffers(Camera *camera)\n> \n> int PipelineHandlerIPU3::start(Camera *camera, [[maybe_unused]] const ControlList *controls)\n> {\n> -\tstd::map entityControls;\n> \tIPU3CameraData *data = cameraData(camera);\n> \tCIO2Device *cio2 = &data->cio2_;\n> \tImgUDevice *imgu = data->imgu_;\n> @@ -744,9 +747,6 @@ int PipelineHandlerIPU3::start(Camera *camera, [[maybe_unused]] const ControlLis\n> \tif (ret)\n> \t\tgoto error;\n> \n> -\tentityControls.emplace(0, data->cio2_.sensor()->controls());\n> -\tdata->ipa_->configure(entityControls);\n> -\n> \treturn 0;\n> \n> error:","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 7EB39C32E1\n\tfor ;\n\tTue, 16 Mar 2021 20:56:05 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DFF7468D4E;\n\tTue, 16 Mar 2021 21:56:04 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2437068D49\n\tfor ;\n\tTue, 16 Mar 2021 21:56:03 +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 87F45D8B;\n\tTue, 16 Mar 2021 21:56:02 +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=\"fyQzwy+n\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1615928162;\n\tbh=DsPLyDMnPrPLKEXxpIZ/JLQQpnT5Ual7LVijncKdL4M=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=fyQzwy+nj/nescHOoii3qHxnM5GL/X2+EctxnfGNFYoBAa07YjQ+JZxyo7BKS1b5b\n\tWmehpGZ7RpFP8jjrsddU6lEoj2McNRQ0ompWf+/Wo+7C1BNBZtPQnq7fZHjEq27Naa\n\tQS7ckr6qUrRVur/EZFYCnKY+wHmnxZwsqJoo2ASQ=","Date":"Tue, 16 Mar 2021 22:55:27 +0200","From":"Laurent Pinchart ","To":"Jean-Michel Hautbois ","Message-ID":"","References":"<20210316154023.46033-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20210316154023.46033-2-jeanmichel.hautbois@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210316154023.46033-2-jeanmichel.hautbois@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v4 1/2] libcamera: ipu3: Move ipa\n\tconfiguration from start() to configure()","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":15741,"web_url":"https://patchwork.libcamera.org/comment/15741/","msgid":"<8807cc13-4d54-a7c6-2173-d8bf85a7992e@ideasonboard.com>","date":"2021-03-17T12:59:56","subject":"Re: [libcamera-devel] [PATCH v4 1/2] libcamera: ipu3: Move ipa\n\tconfiguration from start() to configure()","submitter":{"id":75,"url":"https://patchwork.libcamera.org/api/people/75/","name":"Jean-Michel Hautbois","email":"jeanmichel.hautbois@ideasonboard.com"},"content":"Hi Laurent,\n\nOn 16/03/2021 21:55, Laurent Pinchart wrote:\n> Hi Jean-Michel,\n> \n> Thank you for the patch.\n> \n> On Tue, Mar 16, 2021 at 04:40:22PM +0100, Jean-Michel Hautbois wrote:\n>> IPA was configured after all the pipeline devices were started,\n>> including IPA itself...\n>> Move it at the end of configure() call.\n>>\n>> Signed-off-by: Jean-Michel Hautbois \n> \n> The change looks good. Could you make sure it doesn't break anything if\n> we call configure() twice without any start() in-between ? This check\n> should be added to lc-compliance (once we get it merged), in the\n> meantime, you can modify any test application to duplicate the\n> configure() call. Bonus points if this can be run under valgrind too.\n\nI have cheated with test/camera/capture.cpp, I get too configure() calls\nsuccessfully without any issue.\n\nAs I really like bonus points, I also passed valgrind on a 'cam' call:\n==28310== Using Valgrind-3.16.1 and LibVEX; rerun with -h for copyright info\n==28310== Command: cam -c1 -C100\n==28310==\n[29:33:24.510402583] [28310] INFO Camera camera_manager.cpp:293\nlibcamera v0.0.0+2409-d179b494-dirty (2021-03-17T13:55:04+01:00)\n[29:33:27.824662259] [28313] INFO IPU3 ipu3.cpp:1128 Registered\nCamera[0] \"\\_SB_.PCI0.LNK1\" connected to CSI-2 receiver 1\nUsing camera \\_SB_.PCI0.LNK1\n[29:33:28.243120313] [28310] INFO Camera camera.cpp:890 configuring\nstreams: (0) 1280x720-NV12\nCapture 100 frames\n106409.888872 (0.00 fps) stream0 seq: 020006 bytesused: 1382400\n106409.896575 (129.82 fps) stream0 seq: 020007 bytesused: 1382400\n\n106413.324588 (29.91 fps) stream0 seq: 020104 bytesused: 1382400\n106413.357822 (30.09 fps) stream0 seq: 020105 bytesused: 1382400\n==28310==\n==28310== HEAP SUMMARY:\n==28310== in use at exit: 0 bytes in 0 blocks\n==28310== total heap usage: 18,382 allocs, 18,382 frees, 2,117,494\nbytes allocated\n==28310==\n==28310== All heap blocks were freed -- no leaks are possible\n==28310==\n==28310== For lists of detected and suppressed errors, rerun with: -s\n==28310== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)\n\n> Once done,\n> \n> Reviewed-by: Laurent Pinchart \n\nThank you :-).\n\n> \n>> ---\n>> src/libcamera/pipeline/ipu3/ipu3.cpp | 8 ++++----\n>> 1 file changed, 4 insertions(+), 4 deletions(-)\n>>\n>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n>> index 2ea13ec9..bb61ef4a 100644\n>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n>> @@ -633,6 +633,10 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)\n>> \t\treturn ret;\n>> \t}\n>> \n>> +\tstd::map entityControls;\n>> +\tentityControls.emplace(0, data->cio2_.sensor()->controls());\n>> +\tdata->ipa_->configure(entityControls);\n>> +\n>> \treturn 0;\n>> }\n>> \n>> @@ -717,7 +721,6 @@ int PipelineHandlerIPU3::freeBuffers(Camera *camera)\n>> \n>> int PipelineHandlerIPU3::start(Camera *camera, [[maybe_unused]] const ControlList *controls)\n>> {\n>> -\tstd::map entityControls;\n>> \tIPU3CameraData *data = cameraData(camera);\n>> \tCIO2Device *cio2 = &data->cio2_;\n>> \tImgUDevice *imgu = data->imgu_;\n>> @@ -744,9 +747,6 @@ int PipelineHandlerIPU3::start(Camera *camera, [[maybe_unused]] const ControlLis\n>> \tif (ret)\n>> \t\tgoto error;\n>> \n>> -\tentityControls.emplace(0, data->cio2_.sensor()->controls());\n>> -\tdata->ipa_->configure(entityControls);\n>> -\n>> \treturn 0;\n>> \n>> error:\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 3510EC32E1\n\tfor ;\n\tWed, 17 Mar 2021 13:00:00 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7F44F68D62;\n\tWed, 17 Mar 2021 13:59:59 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 70314605B1\n\tfor ;\n\tWed, 17 Mar 2021 13:59:57 +0100 (CET)","from [IPv6:2a01:e0a:169:7140:b62c:b7e4:bec7:f024] (unknown\n\t[IPv6:2a01:e0a:169:7140:b62c:b7e4:bec7:f024])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id E28BC45D;\n\tWed, 17 Mar 2021 13:59:56 +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=\"PHZw8IPU\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1615985997;\n\tbh=6nqHErvvf8KG1K8c/CVwgfRZ1iKrZAZPmdLQ+6yCqXc=;\n\th=Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=PHZw8IPUz363KhOhvpy9onFBnv8cvrF2un9w7qZF3JsuM02rukbrWlLNPIZnOACAK\n\tdBUMhPi43KQ+YE16Fe6SO4eIy1ooFM0ilLGgPn10e7DPxas6gwG3ZXhNMWCL8Pyy8L\n\td4sxr5CrCIGtUmPHWCHMI8fYouX6q0CiHVrFxM+E=","To":"Laurent Pinchart ,\n\tJean-Michel Hautbois ","References":"<20210316154023.46033-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20210316154023.46033-2-jeanmichel.hautbois@ideasonboard.com>\n\t","From":"Jean-Michel Hautbois ","Message-ID":"<8807cc13-4d54-a7c6-2173-d8bf85a7992e@ideasonboard.com>","Date":"Wed, 17 Mar 2021 13:59:56 +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":"","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH v4 1/2] libcamera: ipu3: Move ipa\n\tconfiguration from start() to configure()","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\" "}}]