[{"id":14109,"web_url":"https://patchwork.libcamera.org/comment/14109/","msgid":"","date":"2020-12-07T23:35:44","subject":"Re: [libcamera-devel] [PATCH v3 0/3] Pass controls on camera:start()","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Naush,\n\nOn Fri, Dec 04, 2020 at 03:31:18PM +0000, Naushir Patuck wrote:\n> Hi,\n> \n> Here is patchset v3 for the work on passing controls on\n> camera::start(). I have addressed Jacopo's review comments, and\n> rebased to the top of master.\n\nThis looks good to me. I've replied with minor comments on 1/3 and 2/3\nwhich I could address when applying the series. For 3/3, there's an open\nquestion of whether we should drop the initial control set at\nconfigure() time. If you send a v4 with this change, could you\nincorporate the review feedback for the other patches too ?\n\n> Naushir Patuck (3):\n> libcamera: pipeline: Pass libcamera controls into\n> pipeline_handler::start()\n> libcamera: ipa: Pass a set of controls and return results from\n> ipa::start()\n> pipeline: ipa: raspberrypi: Pass controls to IPA on start\n> \n> Documentation/guides/pipeline-handler.rst | 4 +-\n> include/libcamera/camera.h | 2 +-\n> .../libcamera/internal/ipa_context_wrapper.h | 3 +-\n> include/libcamera/internal/pipeline_handler.h | 2 +-\n> include/libcamera/ipa/ipa_interface.h | 3 +-\n> include/libcamera/ipa/raspberrypi.h | 1 +\n> src/ipa/libipa/ipa_interface_wrapper.cpp | 4 +-\n> src/ipa/raspberrypi/raspberrypi.cpp | 55 ++++++++++++-------\n> src/ipa/rkisp1/rkisp1.cpp | 3 +-\n> src/ipa/vimc/vimc.cpp | 6 +-\n> src/libcamera/camera.cpp | 11 ++--\n> src/libcamera/ipa_context_wrapper.cpp | 5 +-\n> src/libcamera/ipa_interface.cpp | 7 +++\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 4 +-\n> .../pipeline/raspberrypi/raspberrypi.cpp | 22 ++++++--\n> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 7 ++-\n> src/libcamera/pipeline/simple/simple.cpp | 4 +-\n> src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 4 +-\n> src/libcamera/pipeline/vimc/vimc.cpp | 7 ++-\n> src/libcamera/pipeline_handler.cpp | 1 +\n> src/libcamera/proxy/ipa_proxy_linux.cpp | 3 +-\n> src/libcamera/proxy/ipa_proxy_thread.cpp | 13 +++--\n> test/ipa/ipa_interface_test.cpp | 3 +-\n> test/ipa/ipa_wrappers_test.cpp | 5 +-\n> 24 files changed, 117 insertions(+), 62 deletions(-)","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 5795FBDB20\n\tfor ;\n\tMon, 7 Dec 2020 23:35:50 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CD35E67E6C;\n\tTue, 8 Dec 2020 00:35:49 +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 C8EA667E6B\n\tfor ;\n\tTue, 8 Dec 2020 00:35:47 +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 384248D;\n\tTue, 8 Dec 2020 00:35:47 +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=\"kxfGxoSQ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1607384147;\n\tbh=S+JwGy7zdmXK9AZRnjomA6CRoWyhrC2eIvbc2Cf2Eno=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=kxfGxoSQ6Gn5su0HhHiGZimEjNGLcu1kvbXujK8/6c9rqL99HPMv+Ja3k1yP6THbD\n\tQrEn/H1QdEk3u60OaEBB3iQM92Piu2rRG36iQfjyubRv4i1bHYpSayrRreRc4nA/sj\n\tpN/ai29fhmUvYvWoge6QGzmbXQ8BhJ7L2L7TBbO0=","Date":"Tue, 8 Dec 2020 01:35:44 +0200","From":"Laurent Pinchart ","To":"Naushir Patuck ","Message-ID":"","References":"<20201204153121.66136-1-naush@raspberrypi.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201204153121.66136-1-naush@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH v3 0/3] Pass controls on camera:start()","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":14124,"web_url":"https://patchwork.libcamera.org/comment/14124/","msgid":"","date":"2020-12-08T09:57:17","subject":"Re: [libcamera-devel] [PATCH v3 0/3] Pass controls on camera:start()","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Laurent,\n\nOn Mon, 7 Dec 2020 at 23:35, Laurent Pinchart <\nlaurent.pinchart@ideasonboard.com> wrote:\n\n> Hi Naush,\n>\n> On Fri, Dec 04, 2020 at 03:31:18PM +0000, Naushir Patuck wrote:\n> > Hi,\n> >\n> > Here is patchset v3 for the work on passing controls on\n> > camera::start(). I have addressed Jacopo's review comments, and\n> > rebased to the top of master.\n>\n> This looks good to me. I've replied with minor comments on 1/3 and 2/3\n> which I could address when applying the series. For 3/3, there's an open\n> question of whether we should drop the initial control set at\n> configure() time. If you send a v4 with this change, could you\n> incorporate the review feedback for the other patches too ?\n>\n\nThanks for the review feedback for all the patches. Regarding the question\nof removing the initial control set at configure() time, I think it still\nis needed. That set at configure() applies the default exposure time/gain\nto the sensor at start of today, whereas the set at start() only applies\nthe exposure/gain if the controller wants a change to those values. On the\nfirst run, the controller will have no state history, so will not provide\nvalues to set in start(). The only time this is not true is if a user has\nprovided some manual exposure/gain controls for the agc to use. Then we do\nthe RPi::IPA_CONFIG_SENSOR twice, once with the default values, then\noverriding that with the user values.\n\nIf you are ok with everything else, feel free to apply all your suggested\nchanges to the patches and merge. If instead you would prefer me to create\na v4 with all the changes, happy to do so as well, let me know.\n\nRegards,\nNaush\n\n\n\n> > Naushir Patuck (3):\n> > libcamera: pipeline: Pass libcamera controls into\n> > pipeline_handler::start()\n> > libcamera: ipa: Pass a set of controls and return results from\n> > ipa::start()\n> > pipeline: ipa: raspberrypi: Pass controls to IPA on start\n> >\n> > Documentation/guides/pipeline-handler.rst | 4 +-\n> > include/libcamera/camera.h | 2 +-\n> > .../libcamera/internal/ipa_context_wrapper.h | 3 +-\n> > include/libcamera/internal/pipeline_handler.h | 2 +-\n> > include/libcamera/ipa/ipa_interface.h | 3 +-\n> > include/libcamera/ipa/raspberrypi.h | 1 +\n> > src/ipa/libipa/ipa_interface_wrapper.cpp | 4 +-\n> > src/ipa/raspberrypi/raspberrypi.cpp | 55 ++++++++++++-------\n> > src/ipa/rkisp1/rkisp1.cpp | 3 +-\n> > src/ipa/vimc/vimc.cpp | 6 +-\n> > src/libcamera/camera.cpp | 11 ++--\n> > src/libcamera/ipa_context_wrapper.cpp | 5 +-\n> > src/libcamera/ipa_interface.cpp | 7 +++\n> > src/libcamera/pipeline/ipu3/ipu3.cpp | 4 +-\n> > .../pipeline/raspberrypi/raspberrypi.cpp | 22 ++++++--\n> > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 7 ++-\n> > src/libcamera/pipeline/simple/simple.cpp | 4 +-\n> > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 4 +-\n> > src/libcamera/pipeline/vimc/vimc.cpp | 7 ++-\n> > src/libcamera/pipeline_handler.cpp | 1 +\n> > src/libcamera/proxy/ipa_proxy_linux.cpp | 3 +-\n> > src/libcamera/proxy/ipa_proxy_thread.cpp | 13 +++--\n> > test/ipa/ipa_interface_test.cpp | 3 +-\n> > test/ipa/ipa_wrappers_test.cpp | 5 +-\n> > 24 files changed, 117 insertions(+), 62 deletions(-)\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\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 63F3EBDB20\n\tfor ;\n\tTue, 8 Dec 2020 09:57:39 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D9F7267E6E;\n\tTue, 8 Dec 2020 10:57:38 +0100 (CET)","from mail-lj1-x229.google.com (mail-lj1-x229.google.com\n\t[IPv6:2a00:1450:4864:20::229])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 59927635F2\n\tfor ;\n\tTue, 8 Dec 2020 10:57:37 +0100 (CET)","by mail-lj1-x229.google.com with SMTP id a1so17074807ljq.3\n\tfor ;\n\tTue, 08 Dec 2020 01:57:37 -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=\"K9gKyyAs\"; 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=PCS3niyU6DQi5QhY2j0r7MRoCjNiAC6yYoZvCkcCpEg=;\n\tb=K9gKyyAsQv4edWra3ct5JSFqUKrXMRc2WmtKM8OwOuGNwrjBbLXhWg0/HDN4Ss7so/\n\twKnNuAWmo++Fs5o970A8+Ra8Qkj2gHFtrDc13KNdG91oPIAcNTfxVVJI2/IpeZxX8PbQ\n\tQ0S0ylnu/6/ZpVD9/B3vSge+lXb0K+CP8sTbUD9Gk7T4MNKEBBabO/qOEEGz7XgCz4cG\n\tkuzT7fKzGPCYHxm8+8RGQqX0XBiuEdVVucajIAnsUmJyLFp1pjlT3wSJqRfCxiFATOUi\n\tNMvs3gityDGVNoqSMm6HjZw3rGrNhJ9W1tlyaLmrvL29UEXxdiUl4iS7cuznJ81cmxCU\n\t217w==","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=PCS3niyU6DQi5QhY2j0r7MRoCjNiAC6yYoZvCkcCpEg=;\n\tb=qjZMh85muI5TqavwNs61p84VaB1Bv98HFUKGD5/Mp/ESBbP35u80fi8Y/fGoyQdgak\n\tx8ac4KMLn6QwlEGqPdMfMU4Ws5liJJcw+cugLovs7l0CdlaHh00WdegXph18za3VXZem\n\tL8z4YEM1/8CiQAhTNO9JRw13CSS2lB0LFx3zBx/rkG4FROFMWhCx3HqDU+zgmLjR3Zoz\n\tgAfmmQH3OrIvGZejMRTrll9WEL2+4dmWy9v7wZ9JSUqNHIQ9JH1glIrUwTjyL30fxeJk\n\tYbcRKtOXUqpjEGX3zJ9XFMwuGSdDPdYMtdxmUuAk34K6FD+rrVjd4PwUz7pkMbxrfL+t\n\tPBww==","X-Gm-Message-State":"AOAM531KIyjHikrIiWLYpCf4d29BO5aGC1u+2W3i0tXC4ciJA44ewOYq\n\ti4CeYqMW1L6eTP24oIRq65TTZjoXmg0SadaJslTytA==","X-Google-Smtp-Source":"ABdhPJzcStl+Mbb7wXWSIqMyKNoBT6Me/pY16Et/V5OjsvuLCb1ABBojQWLNQ5Sx25brn/0kZYCgLaps/u9NjvVDbXs=","X-Received":"by 2002:a2e:3a10:: with SMTP id\n\th16mr10839761lja.400.1607421456544; \n\tTue, 08 Dec 2020 01:57:36 -0800 (PST)","MIME-Version":"1.0","References":"<20201204153121.66136-1-naush@raspberrypi.com>\n\t","In-Reply-To":"","From":"Naushir Patuck ","Date":"Tue, 8 Dec 2020 09:57:17 +0000","Message-ID":"","To":"Laurent Pinchart ","Subject":"Re: [libcamera-devel] [PATCH v3 0/3] Pass controls on camera:start()","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=\"===============6056110184151841307==\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":14133,"web_url":"https://patchwork.libcamera.org/comment/14133/","msgid":"","date":"2020-12-08T11:07:02","subject":"Re: [libcamera-devel] [PATCH v3 0/3] Pass controls on camera:start()","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Naush,\n\nOn Tue, Dec 08, 2020 at 09:57:17AM +0000, Naushir Patuck wrote:\n> On Mon, 7 Dec 2020 at 23:35, Laurent Pinchart wrote:\n> > On Fri, Dec 04, 2020 at 03:31:18PM +0000, Naushir Patuck wrote:\n> > > Hi,\n> > >\n> > > Here is patchset v3 for the work on passing controls on\n> > > camera::start(). I have addressed Jacopo's review comments, and\n> > > rebased to the top of master.\n> >\n> > This looks good to me. I've replied with minor comments on 1/3 and 2/3\n> > which I could address when applying the series. For 3/3, there's an open\n> > question of whether we should drop the initial control set at\n> > configure() time. If you send a v4 with this change, could you\n> > incorporate the review feedback for the other patches too ?\n> \n> Thanks for the review feedback for all the patches. Regarding the question\n> of removing the initial control set at configure() time, I think it still\n> is needed. That set at configure() applies the default exposure time/gain\n> to the sensor at start of today, whereas the set at start() only applies\n> the exposure/gain if the controller wants a change to those values. On the\n> first run, the controller will have no state history, so will not provide\n> values to set in start(). The only time this is not true is if a user has\n> provided some manual exposure/gain controls for the agc to use. Then we do\n> the RPi::IPA_CONFIG_SENSOR twice, once with the default values, then\n> overriding that with the user values.\n\nRight, I understand that. My question was whether we shouldn't instead\nskip RPi::IPA_CONFIG_SENSOR at configure() time, and do it\nunconditionally at start() time. Now that I've read your answer, I\nunderstand we don't necessarily want to apply new control values at\nstart() time when restarting the camera without reconfiguring it, but\nI'm concerned about how this will interact with staggered writes (or the\nnew DelayedControls class). For instance, if an application calls\nCamera::configure() multiple times in a row before Camera::start(),\nthere will be multiple sensor control writes enqueued in the staggered\nwrites mechanism, which I think could create issues. Wouldn't it be\nsimpler, from a pipeline handler point of view, if every time the camera\nis started the IPA provided initial sensor controls once and once only ?\n\n> If you are ok with everything else, feel free to apply all your suggested\n> changes to the patches and merge. If instead you would prefer me to create\n> a v4 with all the changes, happy to do so as well, let me know.\n> \n> > > Naushir Patuck (3):\n> > > libcamera: pipeline: Pass libcamera controls into\n> > > pipeline_handler::start()\n> > > libcamera: ipa: Pass a set of controls and return results from\n> > > ipa::start()\n> > > pipeline: ipa: raspberrypi: Pass controls to IPA on start\n> > >\n> > > Documentation/guides/pipeline-handler.rst | 4 +-\n> > > include/libcamera/camera.h | 2 +-\n> > > .../libcamera/internal/ipa_context_wrapper.h | 3 +-\n> > > include/libcamera/internal/pipeline_handler.h | 2 +-\n> > > include/libcamera/ipa/ipa_interface.h | 3 +-\n> > > include/libcamera/ipa/raspberrypi.h | 1 +\n> > > src/ipa/libipa/ipa_interface_wrapper.cpp | 4 +-\n> > > src/ipa/raspberrypi/raspberrypi.cpp | 55 ++++++++++++-------\n> > > src/ipa/rkisp1/rkisp1.cpp | 3 +-\n> > > src/ipa/vimc/vimc.cpp | 6 +-\n> > > src/libcamera/camera.cpp | 11 ++--\n> > > src/libcamera/ipa_context_wrapper.cpp | 5 +-\n> > > src/libcamera/ipa_interface.cpp | 7 +++\n> > > src/libcamera/pipeline/ipu3/ipu3.cpp | 4 +-\n> > > .../pipeline/raspberrypi/raspberrypi.cpp | 22 ++++++--\n> > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 7 ++-\n> > > src/libcamera/pipeline/simple/simple.cpp | 4 +-\n> > > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 4 +-\n> > > src/libcamera/pipeline/vimc/vimc.cpp | 7 ++-\n> > > src/libcamera/pipeline_handler.cpp | 1 +\n> > > src/libcamera/proxy/ipa_proxy_linux.cpp | 3 +-\n> > > src/libcamera/proxy/ipa_proxy_thread.cpp | 13 +++--\n> > > test/ipa/ipa_interface_test.cpp | 3 +-\n> > > test/ipa/ipa_wrappers_test.cpp | 5 +-\n> > > 24 files changed, 117 insertions(+), 62 deletions(-)","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 0436CBDB20\n\tfor ;\n\tTue, 8 Dec 2020 11:07:08 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 67C0367E74;\n\tTue, 8 Dec 2020 12:07:07 +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 50B4E67E12\n\tfor ;\n\tTue, 8 Dec 2020 12:07:06 +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 C283EDD;\n\tTue, 8 Dec 2020 12:07:05 +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=\"i5kp3N+C\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1607425626;\n\tbh=f+faw26DEcoy1ABELqTSA0F++U78KX57e9ttJDWq5U4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=i5kp3N+CcYrWHne6ROdMraClLNhz49do8zmtpreQ7frHcExNz+zFwy8wwARmJGkBR\n\tQ75npTusxNrJQwIUj63xoz+3I5Ya7Q+wA5CH49Qb7MFNTedFLm3lrRz3sD8eAWe9qA\n\t4GZH2lU8h2hvmCX15UjcBuykAN5Qi3vuEado29qs=","Date":"Tue, 8 Dec 2020 13:07:02 +0200","From":"Laurent Pinchart ","To":"Naushir Patuck ","Message-ID":"","References":"<20201204153121.66136-1-naush@raspberrypi.com>\n\t\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v3 0/3] Pass controls on camera:start()","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":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":14134,"web_url":"https://patchwork.libcamera.org/comment/14134/","msgid":"","date":"2020-12-08T11:20:56","subject":"Re: [libcamera-devel] [PATCH v3 0/3] Pass controls on camera:start()","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Laurent,\n\nOn Tue, 8 Dec 2020 at 11:07, Laurent Pinchart <\nlaurent.pinchart@ideasonboard.com> wrote:\n\n> Hi Naush,\n>\n> On Tue, Dec 08, 2020 at 09:57:17AM +0000, Naushir Patuck wrote:\n> > On Mon, 7 Dec 2020 at 23:35, Laurent Pinchart wrote:\n> > > On Fri, Dec 04, 2020 at 03:31:18PM +0000, Naushir Patuck wrote:\n> > > > Hi,\n> > > >\n> > > > Here is patchset v3 for the work on passing controls on\n> > > > camera::start(). I have addressed Jacopo's review comments, and\n> > > > rebased to the top of master.\n> > >\n> > > This looks good to me. I've replied with minor comments on 1/3 and 2/3\n> > > which I could address when applying the series. For 3/3, there's an\n> open\n> > > question of whether we should drop the initial control set at\n> > > configure() time. If you send a v4 with this change, could you\n> > > incorporate the review feedback for the other patches too ?\n> >\n> > Thanks for the review feedback for all the patches. Regarding the\n> question\n> > of removing the initial control set at configure() time, I think it still\n> > is needed. That set at configure() applies the default exposure\n> time/gain\n> > to the sensor at start of today, whereas the set at start() only applies\n> > the exposure/gain if the controller wants a change to those values. On\n> the\n> > first run, the controller will have no state history, so will not provide\n> > values to set in start(). The only time this is not true is if a user\n> has\n> > provided some manual exposure/gain controls for the agc to use. Then we\n> do\n> > the RPi::IPA_CONFIG_SENSOR twice, once with the default values, then\n> > overriding that with the user values.\n>\n> Right, I understand that. My question was whether we shouldn't instead\n> skip RPi::IPA_CONFIG_SENSOR at configure() time, and do it\n> unconditionally at start() time. Now that I've read your answer, I\n> understand we don't necessarily want to apply new control values at\n> start() time when restarting the camera without reconfiguring it, but\n> I'm concerned about how this will interact with staggered writes (or the\n> new DelayedControls class). For instance, if an application calls\n> Camera::configure() multiple times in a row before Camera::start(),\n> there will be multiple sensor control writes enqueued in the staggered\n> writes mechanism, which I think could create issues. Wouldn't it be\n> simpler, from a pipeline handler point of view, if every time the camera\n> is started the IPA provided initial sensor controls once and once only ?\n>\n\nYes, this is a valid concern. I get around this issue in the staggered\nwriter by having a reset() mechanism. Just before stream on, I call\nstaggered write reset(), and this method write the last set control values\nto the device, and throw away all earlier values. This means that on\nstart(), there will only ever be one (the latest) set of controls to use.\nDelayedControls has implemented a similar reset() mechanism. However, in\nDelayedControls::reset(), it gets given a list of controls as an argument\nto replace the existing controls in the queue. Niklas, please correct me\nif this is wrong :) I did talk about a flush() mechanism that mirrors\ncloser to what the staggered writer does as well.\n\nRegards,\nNaush\n\n\n>\n> > If you are ok with everything else, feel free to apply all your suggested\n> > changes to the patches and merge. If instead you would prefer me to\n> create\n> > a v4 with all the changes, happy to do so as well, let me know.\n> >\n> > > > Naushir Patuck (3):\n> > > > libcamera: pipeline: Pass libcamera controls into\n> > > > pipeline_handler::start()\n> > > > libcamera: ipa: Pass a set of controls and return results from\n> > > > ipa::start()\n> > > > pipeline: ipa: raspberrypi: Pass controls to IPA on start\n> > > >\n> > > > Documentation/guides/pipeline-handler.rst | 4 +-\n> > > > include/libcamera/camera.h | 2 +-\n> > > > .../libcamera/internal/ipa_context_wrapper.h | 3 +-\n> > > > include/libcamera/internal/pipeline_handler.h | 2 +-\n> > > > include/libcamera/ipa/ipa_interface.h | 3 +-\n> > > > include/libcamera/ipa/raspberrypi.h | 1 +\n> > > > src/ipa/libipa/ipa_interface_wrapper.cpp | 4 +-\n> > > > src/ipa/raspberrypi/raspberrypi.cpp | 55\n> ++++++++++++-------\n> > > > src/ipa/rkisp1/rkisp1.cpp | 3 +-\n> > > > src/ipa/vimc/vimc.cpp | 6 +-\n> > > > src/libcamera/camera.cpp | 11 ++--\n> > > > src/libcamera/ipa_context_wrapper.cpp | 5 +-\n> > > > src/libcamera/ipa_interface.cpp | 7 +++\n> > > > src/libcamera/pipeline/ipu3/ipu3.cpp | 4 +-\n> > > > .../pipeline/raspberrypi/raspberrypi.cpp | 22 ++++++--\n> > > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 7 ++-\n> > > > src/libcamera/pipeline/simple/simple.cpp | 4 +-\n> > > > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 4 +-\n> > > > src/libcamera/pipeline/vimc/vimc.cpp | 7 ++-\n> > > > src/libcamera/pipeline_handler.cpp | 1 +\n> > > > src/libcamera/proxy/ipa_proxy_linux.cpp | 3 +-\n> > > > src/libcamera/proxy/ipa_proxy_thread.cpp | 13 +++--\n> > > > test/ipa/ipa_interface_test.cpp | 3 +-\n> > > > test/ipa/ipa_wrappers_test.cpp | 5 +-\n> > > > 24 files changed, 117 insertions(+), 62 deletions(-)\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\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 A2469BDB1F\n\tfor ;\n\tTue, 8 Dec 2020 11:21:16 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0414F67E6D;\n\tTue, 8 Dec 2020 12:21:16 +0100 (CET)","from mail-lj1-x236.google.com (mail-lj1-x236.google.com\n\t[IPv6:2a00:1450:4864:20::236])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id EC57A600FF\n\tfor ;\n\tTue, 8 Dec 2020 12:21:13 +0100 (CET)","by mail-lj1-x236.google.com with SMTP id f11so6458987ljm.8\n\tfor ;\n\tTue, 08 Dec 2020 03:21:13 -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=\"HQvw7ooe\"; 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=EpQpApNqCZsW7QU5rjid7IIkd7O8pyOfmvACvs17/Jg=;\n\tb=HQvw7ooeD6aQTgRoQJCRN4HXcMuGRG7R0gFUqN0JDis6c0NTVXNKaNng67dmqXofFR\n\tk5Laeab/gGe6O5Iyz1xuKE19WSWo4zbAjqJPgRY3jqmbT9gFnHeWJkC2xGKOok5TMHT1\n\tIu13wdhjVuooyK8D+LPQUhoWr3vUoX1pSC3QCZY7Dukn+BSne74c17YRYGeCTC7aN6jJ\n\t4E17jmUeIRNDtGqb8iOUkQj2BK5qhJfkjkkcd4IeN1vZ/ntHX52aX7PmkYcK1a/IFMQa\n\tvnfjmTuxaAGBd9oHwjCs1Zv1qzkwFHbw6hjuMtkACI9N5tPX3agpdVbc/rc15todKa65\n\tFVUQ==","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=EpQpApNqCZsW7QU5rjid7IIkd7O8pyOfmvACvs17/Jg=;\n\tb=rnQCo7o2lJzIOBRpqolIZpRznzdC36DILKXx8X2Sij8VEaFrs4VU1UkB99zP54jY9n\n\tlaii/yD5wdXvn2CYhj92FFT9eNOwLgRonChHczvblGYCqI+BFpYaskSZfGzIeuQZ3VAC\n\tHqGQF6TUWGsipCjD1iEVLDXcGZiDGLPzYR2ZJzO9kmbrUgFIn0ZQvYXJbcBM43/wlR2M\n\txtc39zlLXqvqYELVuHHc3s4dcJrDF4StAx4+yqQTriLFwyj3FoOE1PqRtzNZo/VLnKWE\n\tewS4rhpbXjam60pqcOc6qP098qSIcA2En7qKXCYMxtOstW2n55upb7hBeCYBAxvS0yCA\n\tNq2Q==","X-Gm-Message-State":"AOAM531m/0OZ9TDxnM4PoJQNj6LRXddw8nOQ9kdUXRkID8xlqJec+ZKJ\n\t6kKLfzLhWzXVmnJ8dHUxk87oUg86NwuZwSzr8z76tA==","X-Google-Smtp-Source":"ABdhPJzxQR9yJc7k/ufbjpelT/QMjcAAbDaTi5v+2rVSJMWq7s3GEvRC/S3J2LbStHQhF2PTEEN1itfxGrMqllT8VjI=","X-Received":"by 2002:a2e:88d1:: with SMTP id\n\ta17mr5685917ljk.299.1607426473381; \n\tTue, 08 Dec 2020 03:21:13 -0800 (PST)","MIME-Version":"1.0","References":"<20201204153121.66136-1-naush@raspberrypi.com>\n\t\n\t\n\t","In-Reply-To":"","From":"Naushir Patuck ","Date":"Tue, 8 Dec 2020 11:20:56 +0000","Message-ID":"","To":"Laurent Pinchart ","Subject":"Re: [libcamera-devel] [PATCH v3 0/3] Pass controls on camera:start()","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=\"===============2879810787743556938==\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":14146,"web_url":"https://patchwork.libcamera.org/comment/14146/","msgid":"","date":"2020-12-08T12:49:52","subject":"Re: [libcamera-devel] [PATCH v3 0/3] Pass controls on camera:start()","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Naush,\n\nOn Tue, Dec 08, 2020 at 11:20:56AM +0000, Naushir Patuck wrote:\n> On Tue, 8 Dec 2020 at 11:07, Laurent Pinchart wrote:\n> > On Tue, Dec 08, 2020 at 09:57:17AM +0000, Naushir Patuck wrote:\n> > > On Mon, 7 Dec 2020 at 23:35, Laurent Pinchart wrote:\n> > > > On Fri, Dec 04, 2020 at 03:31:18PM +0000, Naushir Patuck wrote:\n> > > > > Hi,\n> > > > >\n> > > > > Here is patchset v3 for the work on passing controls on\n> > > > > camera::start(). I have addressed Jacopo's review comments, and\n> > > > > rebased to the top of master.\n> > > >\n> > > > This looks good to me. I've replied with minor comments on 1/3 and 2/3\n> > > > which I could address when applying the series. For 3/3, there's an open\n> > > > question of whether we should drop the initial control set at\n> > > > configure() time. If you send a v4 with this change, could you\n> > > > incorporate the review feedback for the other patches too ?\n> > >\n> > > Thanks for the review feedback for all the patches. Regarding the question\n> > > of removing the initial control set at configure() time, I think it still\n> > > is needed. That set at configure() applies the default exposure time/gain\n> > > to the sensor at start of today, whereas the set at start() only applies\n> > > the exposure/gain if the controller wants a change to those values. On the\n> > > first run, the controller will have no state history, so will not provide\n> > > values to set in start(). The only time this is not true is if a user has\n> > > provided some manual exposure/gain controls for the agc to use. Then we do\n> > > the RPi::IPA_CONFIG_SENSOR twice, once with the default values, then\n> > > overriding that with the user values.\n> >\n> > Right, I understand that. My question was whether we shouldn't instead\n> > skip RPi::IPA_CONFIG_SENSOR at configure() time, and do it\n> > unconditionally at start() time. Now that I've read your answer, I\n> > understand we don't necessarily want to apply new control values at\n> > start() time when restarting the camera without reconfiguring it, but\n> > I'm concerned about how this will interact with staggered writes (or the\n> > new DelayedControls class). For instance, if an application calls\n> > Camera::configure() multiple times in a row before Camera::start(),\n> > there will be multiple sensor control writes enqueued in the staggered\n> > writes mechanism, which I think could create issues. Wouldn't it be\n> > simpler, from a pipeline handler point of view, if every time the camera\n> > is started the IPA provided initial sensor controls once and once only ?\n> \n> Yes, this is a valid concern. I get around this issue in the staggered\n> writer by having a reset() mechanism. Just before stream on, I call\n> staggered write reset(), and this method write the last set control values\n> to the device, and throw away all earlier values. This means that on\n> start(), there will only ever be one (the latest) set of controls to use.\n> DelayedControls has implemented a similar reset() mechanism. However, in\n> DelayedControls::reset(), it gets given a list of controls as an argument\n> to replace the existing controls in the queue. Niklas, please correct me\n> if this is wrong :) I did talk about a flush() mechanism that mirrors\n> closer to what the staggered writer does as well.\n\nThank you for the explanation. My understanding is that there are\nmultiple ways to address this issue, with different pending patch series\nthat effects this, and it hasn't been decided yet how it will all look\nlike in the end. There's thus no need to change the way controls are\nhandled in this series, we can rework that as part of the work on\nDelayedControls, if needed.\n\nI've pushed the series with the small suggested changes.\n\n> > > If you are ok with everything else, feel free to apply all your suggested\n> > > changes to the patches and merge. If instead you would prefer me to create\n> > > a v4 with all the changes, happy to do so as well, let me know.\n> > >\n> > > > > Naushir Patuck (3):\n> > > > > libcamera: pipeline: Pass libcamera controls into\n> > > > > pipeline_handler::start()\n> > > > > libcamera: ipa: Pass a set of controls and return results from\n> > > > > ipa::start()\n> > > > > pipeline: ipa: raspberrypi: Pass controls to IPA on start\n> > > > >\n> > > > > Documentation/guides/pipeline-handler.rst | 4 +-\n> > > > > include/libcamera/camera.h | 2 +-\n> > > > > .../libcamera/internal/ipa_context_wrapper.h | 3 +-\n> > > > > include/libcamera/internal/pipeline_handler.h | 2 +-\n> > > > > include/libcamera/ipa/ipa_interface.h | 3 +-\n> > > > > include/libcamera/ipa/raspberrypi.h | 1 +\n> > > > > src/ipa/libipa/ipa_interface_wrapper.cpp | 4 +-\n> > > > > src/ipa/raspberrypi/raspberrypi.cpp | 55 ++++++++++++-------\n> > > > > src/ipa/rkisp1/rkisp1.cpp | 3 +-\n> > > > > src/ipa/vimc/vimc.cpp | 6 +-\n> > > > > src/libcamera/camera.cpp | 11 ++--\n> > > > > src/libcamera/ipa_context_wrapper.cpp | 5 +-\n> > > > > src/libcamera/ipa_interface.cpp | 7 +++\n> > > > > src/libcamera/pipeline/ipu3/ipu3.cpp | 4 +-\n> > > > > .../pipeline/raspberrypi/raspberrypi.cpp | 22 ++++++--\n> > > > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 7 ++-\n> > > > > src/libcamera/pipeline/simple/simple.cpp | 4 +-\n> > > > > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 4 +-\n> > > > > src/libcamera/pipeline/vimc/vimc.cpp | 7 ++-\n> > > > > src/libcamera/pipeline_handler.cpp | 1 +\n> > > > > src/libcamera/proxy/ipa_proxy_linux.cpp | 3 +-\n> > > > > src/libcamera/proxy/ipa_proxy_thread.cpp | 13 +++--\n> > > > > test/ipa/ipa_interface_test.cpp | 3 +-\n> > > > > test/ipa/ipa_wrappers_test.cpp | 5 +-\n> > > > > 24 files changed, 117 insertions(+), 62 deletions(-)","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 6DC75BDB20\n\tfor ;\n\tTue, 8 Dec 2020 12:49:58 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DE1FF67F09;\n\tTue, 8 Dec 2020 13:49:57 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 63B0967E4D\n\tfor ;\n\tTue, 8 Dec 2020 13:49:56 +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 D9EA5DD;\n\tTue, 8 Dec 2020 13:49:55 +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=\"qk+GJ64J\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1607431796;\n\tbh=4erUA0D7qnpMYhzy4lap9vvkA+Vo9/aPggtMr3DNFrg=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=qk+GJ64JreRmNEVjeG/w2bmCZSmPT4Jb1qFbk45GT98I3CqBms9SgzJAIzyXpDhdf\n\tIwtFetKtEAZrz1KfQ729Ag/NlDHasXJc/NjtWVZu1L9DO9luzu+iNcJNNieewVFa7a\n\t6Uo0ILKo0stbkQKrpDWQVys2MLXThvwVKBbOx0dE=","Date":"Tue, 8 Dec 2020 14:49:52 +0200","From":"Laurent Pinchart ","To":"Naushir Patuck ","Message-ID":"","References":"<20201204153121.66136-1-naush@raspberrypi.com>\n\t\n\t\n\t\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v3 0/3] Pass controls on camera:start()","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":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]