[{"id":11200,"web_url":"https://patchwork.libcamera.org/comment/11200/","msgid":"<20200706093011.p5z5nsbcs7k6kjvj@uno.localdomain>","date":"2020-07-06T09:30:11","subject":"Re: [libcamera-devel] [PATCH v2 12/12] libcamera: pipeline:\n\traspberrypi: Start IPA when starting camera","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"On Sat, Jul 04, 2020 at 03:52:27AM +0300, Laurent Pinchart wrote:\n> The IPA is meant to be started when starting the camera, and stopped\n> when stopping it. It was so far start early in order to handle the\n\ns/start/started ?\n\n> IPAInterface::processEvent() call related to lens shading table\n> allocation before IPAInterface::configure() to pass the table to the\n> IPA. Now that the lens shading table is passed through configure(),\n> starting the IPA early isn't needed anymore.\n>\n> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>\n> ---\n>  .../pipeline/raspberrypi/raspberrypi.cpp      | 31 ++++++++++---------\n>  1 file changed, 17 insertions(+), 14 deletions(-)\n>\n> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> index 9babf9f4a19c..c877820a6e1e 100644\n> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> @@ -303,10 +303,6 @@ public:\n>  \t\t\tvcsm_.free(lsTable_);\n>  \t\t\tlsTable_ = nullptr;\n>  \t\t}\n> -\n> -\t\t/* Stop the IPA proxy thread. */\n> -\t\tif (ipa_)\n> -\t\t\tipa_->stop();\n>  \t}\n>\n>  \tvoid frameStarted(uint32_t sequence);\n> @@ -800,6 +796,16 @@ int PipelineHandlerRPi::start(Camera *camera)\n>  \tret = queueAllBuffers(camera);\n>  \tif (ret) {\n>  \t\tLOG(RPI, Error) << \"Failed to queue buffers\";\n> +\t\tstop(camera);\n> +\t\treturn ret;\n> +\t}\n> +\n> +\t/* Start the IPA. */\n> +\tret = data->ipa_->start();\n> +\tif (ret) {\n> +\t\tLOG(RPI, Error)\n> +\t\t\t<< \"Failed to start IPA for \" << camera->name();\n> +\t\tstop(camera);\n>  \t\treturn ret;\n>  \t}\n>\n> @@ -810,8 +816,10 @@ int PipelineHandlerRPi::start(Camera *camera)\n>  \tV4L2DeviceFormat sensorFormat;\n>  \tdata->unicam_[Unicam::Image].dev()->getFormat(&sensorFormat);\n>  \tret = data->isp_[Isp::Input].dev()->setFormat(&sensorFormat);\n> -\tif (ret)\n> +\tif (ret) {\n> +\t\tstop(camera);\n\nDo we want to stop the whole camera or just the IPA here ?\nI think you meant data->ipa_->stop(), right ?\n\nWith this fixed\nReviewed-by: Jacopo Mondi <jacopo@jmondi.org>\n\nThanks\n  j\n\n>  \t\treturn ret;\n> +\t}\n>\n>  \t/* Enable SOF event generation. */\n>  \tdata->unicam_[Unicam::Image].dev()->setFrameStartEnabled(true);\n> @@ -855,6 +863,9 @@ void PipelineHandlerRPi::stop(Camera *camera)\n>  \tdata->bayerQueue_ = std::queue<FrameBuffer *>{};\n>  \tdata->embeddedQueue_ = std::queue<FrameBuffer *>{};\n>\n> +\t/* Stop the IPA. */\n> +\tdata->ipa_->stop();\n> +\n>  \tfreeBuffers(camera);\n>  }\n>\n> @@ -1107,15 +1118,7 @@ int RPiCameraData::loadIPA()\n>  \t\t.configurationFile = ipa_->configurationFile(sensor_->model() + \".json\")\n>  \t};\n>\n> -\tipa_->init(settings);\n> -\n> -\t/*\n> -\t * Startup the IPA thread now. Without this call, none of the IPA API\n> -\t * functions will run.\n> -\t *\n> -\t * It only gets stopped in the class destructor.\n> -\t */\n> -\treturn ipa_->start();\n> +\treturn ipa_->init(settings);\n>  }\n>\n>  int RPiCameraData::configureIPA()\n> --\n> Regards,\n>\n> Laurent Pinchart\n>\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 08439BD790\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon,  6 Jul 2020 09:26:41 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9F26360C59;\n\tMon,  6 Jul 2020 11:26:40 +0200 (CEST)","from relay8-d.mail.gandi.net (relay8-d.mail.gandi.net\n\t[217.70.183.201])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A280C603B1\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon,  6 Jul 2020 11:26:39 +0200 (CEST)","from uno.localdomain (host-79-34-235-173.business.telecomitalia.it\n\t[79.34.235.173]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay8-d.mail.gandi.net (Postfix) with ESMTPSA id B66D01BF211;\n\tMon,  6 Jul 2020 09:26:38 +0000 (UTC)"],"X-Originating-IP":"79.34.235.173","Date":"Mon, 6 Jul 2020 11:30:11 +0200","From":"Jacopo Mondi <jacopo@jmondi.org>","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Message-ID":"<20200706093011.p5z5nsbcs7k6kjvj@uno.localdomain>","References":"<20200704005227.21782-1-laurent.pinchart@ideasonboard.com>\n\t<20200704005227.21782-13-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200704005227.21782-13-laurent.pinchart@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2 12/12] libcamera: pipeline:\n\traspberrypi: Start IPA when starting camera","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":11207,"web_url":"https://patchwork.libcamera.org/comment/11207/","msgid":"<20200706110857.GA5912@pendragon.ideasonboard.com>","date":"2020-07-06T11:08:57","subject":"Re: [libcamera-devel] [PATCH v2 12/12] libcamera: pipeline:\n\traspberrypi: Start IPA when starting camera","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Mon, Jul 06, 2020 at 11:30:11AM +0200, Jacopo Mondi wrote:\n> On Sat, Jul 04, 2020 at 03:52:27AM +0300, Laurent Pinchart wrote:\n> > The IPA is meant to be started when starting the camera, and stopped\n> > when stopping it. It was so far start early in order to handle the\n> \n> s/start/started ?\n\nIndeed.\n\n> > IPAInterface::processEvent() call related to lens shading table\n> > allocation before IPAInterface::configure() to pass the table to the\n> > IPA. Now that the lens shading table is passed through configure(),\n> > starting the IPA early isn't needed anymore.\n> >\n> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n> > Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>\n> > ---\n> >  .../pipeline/raspberrypi/raspberrypi.cpp      | 31 ++++++++++---------\n> >  1 file changed, 17 insertions(+), 14 deletions(-)\n> >\n> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > index 9babf9f4a19c..c877820a6e1e 100644\n> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > @@ -303,10 +303,6 @@ public:\n> >  \t\t\tvcsm_.free(lsTable_);\n> >  \t\t\tlsTable_ = nullptr;\n> >  \t\t}\n> > -\n> > -\t\t/* Stop the IPA proxy thread. */\n> > -\t\tif (ipa_)\n> > -\t\t\tipa_->stop();\n> >  \t}\n> >\n> >  \tvoid frameStarted(uint32_t sequence);\n> > @@ -800,6 +796,16 @@ int PipelineHandlerRPi::start(Camera *camera)\n> >  \tret = queueAllBuffers(camera);\n> >  \tif (ret) {\n> >  \t\tLOG(RPI, Error) << \"Failed to queue buffers\";\n> > +\t\tstop(camera);\n> > +\t\treturn ret;\n> > +\t}\n> > +\n> > +\t/* Start the IPA. */\n> > +\tret = data->ipa_->start();\n> > +\tif (ret) {\n> > +\t\tLOG(RPI, Error)\n> > +\t\t\t<< \"Failed to start IPA for \" << camera->name();\n> > +\t\tstop(camera);\n> >  \t\treturn ret;\n> >  \t}\n> >\n> > @@ -810,8 +816,10 @@ int PipelineHandlerRPi::start(Camera *camera)\n> >  \tV4L2DeviceFormat sensorFormat;\n> >  \tdata->unicam_[Unicam::Image].dev()->getFormat(&sensorFormat);\n> >  \tret = data->isp_[Isp::Input].dev()->setFormat(&sensorFormat);\n> > -\tif (ret)\n> > +\tif (ret) {\n> > +\t\tstop(camera);\n> \n> Do we want to stop the whole camera or just the IPA here ?\n> I think you meant data->ipa_->stop(), right ?\n\nThe idea is that the PipelineHandlerRPi::stop() function should clean up\neverything that start() may have done, and can be called in start() when\nan error occurs. To make this easier I need IPAProxy::stop() to be safe\nto call when the IPA is already stopped. I'll send a patch to do so.\n\n> With this fixed\n> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>\n> \n> >  \t\treturn ret;\n> > +\t}\n> >\n> >  \t/* Enable SOF event generation. */\n> >  \tdata->unicam_[Unicam::Image].dev()->setFrameStartEnabled(true);\n> > @@ -855,6 +863,9 @@ void PipelineHandlerRPi::stop(Camera *camera)\n> >  \tdata->bayerQueue_ = std::queue<FrameBuffer *>{};\n> >  \tdata->embeddedQueue_ = std::queue<FrameBuffer *>{};\n> >\n> > +\t/* Stop the IPA. */\n> > +\tdata->ipa_->stop();\n> > +\n> >  \tfreeBuffers(camera);\n> >  }\n> >\n> > @@ -1107,15 +1118,7 @@ int RPiCameraData::loadIPA()\n> >  \t\t.configurationFile = ipa_->configurationFile(sensor_->model() + \".json\")\n> >  \t};\n> >\n> > -\tipa_->init(settings);\n> > -\n> > -\t/*\n> > -\t * Startup the IPA thread now. Without this call, none of the IPA API\n> > -\t * functions will run.\n> > -\t *\n> > -\t * It only gets stopped in the class destructor.\n> > -\t */\n> > -\treturn ipa_->start();\n> > +\treturn ipa_->init(settings);\n> >  }\n> >\n> >  int RPiCameraData::configureIPA()","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 D8550BD790\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon,  6 Jul 2020 11:09:03 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A3F9060C59;\n\tMon,  6 Jul 2020 13:09:03 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0D7AA603AA\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon,  6 Jul 2020 13:09:03 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 5043097E;\n\tMon,  6 Jul 2020 13:09:02 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"OyZQW/8T\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1594033742;\n\tbh=zGwlFgtIjp4R54hm4vDpa88JzCh9zURWyiXkeFMPTts=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=OyZQW/8TAxViTViDPWYoI1PfcQ0RtB1JKHMe1PeNgnDNMXH+3ONgnMK9r52BS2xZT\n\t0LCHZqrOuwdaWilXBNIagXdeo9IqnJhUVRCKYD2knu5ipzgT92ssccNig/2yVmuBC2\n\t+QGfvHnQ48ODnvonBHWG5xHP5WlIlCE2HuqC/8yg=","Date":"Mon, 6 Jul 2020 14:08:57 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Jacopo Mondi <jacopo@jmondi.org>","Message-ID":"<20200706110857.GA5912@pendragon.ideasonboard.com>","References":"<20200704005227.21782-1-laurent.pinchart@ideasonboard.com>\n\t<20200704005227.21782-13-laurent.pinchart@ideasonboard.com>\n\t<20200706093011.p5z5nsbcs7k6kjvj@uno.localdomain>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200706093011.p5z5nsbcs7k6kjvj@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v2 12/12] libcamera: pipeline:\n\traspberrypi: Start IPA when starting camera","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":11212,"web_url":"https://patchwork.libcamera.org/comment/11212/","msgid":"<20200706113756.xpv67v2k34aogof4@uno.localdomain>","date":"2020-07-06T11:37:56","subject":"Re: [libcamera-devel] [PATCH v2 12/12] libcamera: pipeline:\n\traspberrypi: Start IPA when starting camera","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Mon, Jul 06, 2020 at 02:08:57PM +0300, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> On Mon, Jul 06, 2020 at 11:30:11AM +0200, Jacopo Mondi wrote:\n> > On Sat, Jul 04, 2020 at 03:52:27AM +0300, Laurent Pinchart wrote:\n> > > The IPA is meant to be started when starting the camera, and stopped\n> > > when stopping it. It was so far start early in order to handle the\n> >\n> > s/start/started ?\n>\n> Indeed.\n>\n> > > IPAInterface::processEvent() call related to lens shading table\n> > > allocation before IPAInterface::configure() to pass the table to the\n> > > IPA. Now that the lens shading table is passed through configure(),\n> > > starting the IPA early isn't needed anymore.\n> > >\n> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n> > > Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>\n> > > ---\n> > >  .../pipeline/raspberrypi/raspberrypi.cpp      | 31 ++++++++++---------\n> > >  1 file changed, 17 insertions(+), 14 deletions(-)\n> > >\n> > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > index 9babf9f4a19c..c877820a6e1e 100644\n> > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > @@ -303,10 +303,6 @@ public:\n> > >  \t\t\tvcsm_.free(lsTable_);\n> > >  \t\t\tlsTable_ = nullptr;\n> > >  \t\t}\n> > > -\n> > > -\t\t/* Stop the IPA proxy thread. */\n> > > -\t\tif (ipa_)\n> > > -\t\t\tipa_->stop();\n> > >  \t}\n> > >\n> > >  \tvoid frameStarted(uint32_t sequence);\n> > > @@ -800,6 +796,16 @@ int PipelineHandlerRPi::start(Camera *camera)\n> > >  \tret = queueAllBuffers(camera);\n> > >  \tif (ret) {\n> > >  \t\tLOG(RPI, Error) << \"Failed to queue buffers\";\n> > > +\t\tstop(camera);\n> > > +\t\treturn ret;\n> > > +\t}\n> > > +\n> > > +\t/* Start the IPA. */\n> > > +\tret = data->ipa_->start();\n> > > +\tif (ret) {\n> > > +\t\tLOG(RPI, Error)\n> > > +\t\t\t<< \"Failed to start IPA for \" << camera->name();\n> > > +\t\tstop(camera);\n> > >  \t\treturn ret;\n> > >  \t}\n> > >\n> > > @@ -810,8 +816,10 @@ int PipelineHandlerRPi::start(Camera *camera)\n> > >  \tV4L2DeviceFormat sensorFormat;\n> > >  \tdata->unicam_[Unicam::Image].dev()->getFormat(&sensorFormat);\n> > >  \tret = data->isp_[Isp::Input].dev()->setFormat(&sensorFormat);\n> > > -\tif (ret)\n> > > +\tif (ret) {\n> > > +\t\tstop(camera);\n> >\n> > Do we want to stop the whole camera or just the IPA here ?\n> > I think you meant data->ipa_->stop(), right ?\n>\n> The idea is that the PipelineHandlerRPi::stop() function should clean up\n> everything that start() may have done, and can be called in start() when\n> an error occurs. To make this easier I need IPAProxy::stop() to be safe\n> to call when the IPA is already stopped. I'll send a patch to do so.\n>\n\nDoes this mean all the error paths in the start() function should call\nstop() ?\n\n> > With this fixed\n> > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>\n> >\n\nPlease reain my tag here!\n\nThanks\n  j\n\n> > >  \t\treturn ret;\n> > > +\t}\n> > >\n> > >  \t/* Enable SOF event generation. */\n> > >  \tdata->unicam_[Unicam::Image].dev()->setFrameStartEnabled(true);\n> > > @@ -855,6 +863,9 @@ void PipelineHandlerRPi::stop(Camera *camera)\n> > >  \tdata->bayerQueue_ = std::queue<FrameBuffer *>{};\n> > >  \tdata->embeddedQueue_ = std::queue<FrameBuffer *>{};\n> > >\n> > > +\t/* Stop the IPA. */\n> > > +\tdata->ipa_->stop();\n> > > +\n> > >  \tfreeBuffers(camera);\n> > >  }\n> > >\n> > > @@ -1107,15 +1118,7 @@ int RPiCameraData::loadIPA()\n> > >  \t\t.configurationFile = ipa_->configurationFile(sensor_->model() + \".json\")\n> > >  \t};\n> > >\n> > > -\tipa_->init(settings);\n> > > -\n> > > -\t/*\n> > > -\t * Startup the IPA thread now. Without this call, none of the IPA API\n> > > -\t * functions will run.\n> > > -\t *\n> > > -\t * It only gets stopped in the class destructor.\n> > > -\t */\n> > > -\treturn ipa_->start();\n> > > +\treturn ipa_->init(settings);\n> > >  }\n> > >\n> > >  int RPiCameraData::configureIPA()\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 7C696BD790\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon,  6 Jul 2020 11:34:26 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1115660E09;\n\tMon,  6 Jul 2020 13:34:26 +0200 (CEST)","from relay3-d.mail.gandi.net (relay3-d.mail.gandi.net\n\t[217.70.183.195])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 244A5603AA\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon,  6 Jul 2020 13:34:24 +0200 (CEST)","from uno.localdomain (host-79-34-235-173.business.telecomitalia.it\n\t[79.34.235.173]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay3-d.mail.gandi.net (Postfix) with ESMTPSA id 4018B60014;\n\tMon,  6 Jul 2020 11:34:22 +0000 (UTC)"],"X-Originating-IP":"79.34.235.173","Date":"Mon, 6 Jul 2020 13:37:56 +0200","From":"Jacopo Mondi <jacopo@jmondi.org>","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Message-ID":"<20200706113756.xpv67v2k34aogof4@uno.localdomain>","References":"<20200704005227.21782-1-laurent.pinchart@ideasonboard.com>\n\t<20200704005227.21782-13-laurent.pinchart@ideasonboard.com>\n\t<20200706093011.p5z5nsbcs7k6kjvj@uno.localdomain>\n\t<20200706110857.GA5912@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200706110857.GA5912@pendragon.ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2 12/12] libcamera: pipeline:\n\traspberrypi: Start IPA when starting camera","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":11213,"web_url":"https://patchwork.libcamera.org/comment/11213/","msgid":"<20200706114325.GF5912@pendragon.ideasonboard.com>","date":"2020-07-06T11:43:25","subject":"Re: [libcamera-devel] [PATCH v2 12/12] libcamera: pipeline:\n\traspberrypi: Start IPA when starting camera","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Mon, Jul 06, 2020 at 01:37:56PM +0200, Jacopo Mondi wrote:\n> On Mon, Jul 06, 2020 at 02:08:57PM +0300, Laurent Pinchart wrote:\n> > On Mon, Jul 06, 2020 at 11:30:11AM +0200, Jacopo Mondi wrote:\n> >> On Sat, Jul 04, 2020 at 03:52:27AM +0300, Laurent Pinchart wrote:\n> >>> The IPA is meant to be started when starting the camera, and stopped\n> >>> when stopping it. It was so far start early in order to handle the\n> >>\n> >> s/start/started ?\n> >\n> > Indeed.\n> >\n> >>> IPAInterface::processEvent() call related to lens shading table\n> >>> allocation before IPAInterface::configure() to pass the table to the\n> >>> IPA. Now that the lens shading table is passed through configure(),\n> >>> starting the IPA early isn't needed anymore.\n> >>>\n> >>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n> >>> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>\n> >>> ---\n> >>>  .../pipeline/raspberrypi/raspberrypi.cpp      | 31 ++++++++++---------\n> >>>  1 file changed, 17 insertions(+), 14 deletions(-)\n> >>>\n> >>> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> >>> index 9babf9f4a19c..c877820a6e1e 100644\n> >>> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> >>> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> >>> @@ -303,10 +303,6 @@ public:\n> >>>  \t\t\tvcsm_.free(lsTable_);\n> >>>  \t\t\tlsTable_ = nullptr;\n> >>>  \t\t}\n> >>> -\n> >>> -\t\t/* Stop the IPA proxy thread. */\n> >>> -\t\tif (ipa_)\n> >>> -\t\t\tipa_->stop();\n> >>>  \t}\n> >>>\n> >>>  \tvoid frameStarted(uint32_t sequence);\n> >>> @@ -800,6 +796,16 @@ int PipelineHandlerRPi::start(Camera *camera)\n> >>>  \tret = queueAllBuffers(camera);\n> >>>  \tif (ret) {\n> >>>  \t\tLOG(RPI, Error) << \"Failed to queue buffers\";\n> >>> +\t\tstop(camera);\n> >>> +\t\treturn ret;\n> >>> +\t}\n> >>> +\n> >>> +\t/* Start the IPA. */\n> >>> +\tret = data->ipa_->start();\n> >>> +\tif (ret) {\n> >>> +\t\tLOG(RPI, Error)\n> >>> +\t\t\t<< \"Failed to start IPA for \" << camera->name();\n> >>> +\t\tstop(camera);\n> >>>  \t\treturn ret;\n> >>>  \t}\n> >>>\n> >>> @@ -810,8 +816,10 @@ int PipelineHandlerRPi::start(Camera *camera)\n> >>>  \tV4L2DeviceFormat sensorFormat;\n> >>>  \tdata->unicam_[Unicam::Image].dev()->getFormat(&sensorFormat);\n> >>>  \tret = data->isp_[Isp::Input].dev()->setFormat(&sensorFormat);\n> >>> -\tif (ret)\n> >>> +\tif (ret) {\n> >>> +\t\tstop(camera);\n> >>\n> >> Do we want to stop the whole camera or just the IPA here ?\n> >> I think you meant data->ipa_->stop(), right ?\n> >\n> > The idea is that the PipelineHandlerRPi::stop() function should clean up\n> > everything that start() may have done, and can be called in start() when\n> > an error occurs. To make this easier I need IPAProxy::stop() to be safe\n> > to call when the IPA is already stopped. I'll send a patch to do so.\n> \n> Does this mean all the error paths in the start() function should call\n> stop() ?\n\nYes, that's the idea.\n\n> >> With this fixed\n> >> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>\n> \n> Please reain my tag here!\n> \n> >>>  \t\treturn ret;\n> >>> +\t}\n> >>>\n> >>>  \t/* Enable SOF event generation. */\n> >>>  \tdata->unicam_[Unicam::Image].dev()->setFrameStartEnabled(true);\n> >>> @@ -855,6 +863,9 @@ void PipelineHandlerRPi::stop(Camera *camera)\n> >>>  \tdata->bayerQueue_ = std::queue<FrameBuffer *>{};\n> >>>  \tdata->embeddedQueue_ = std::queue<FrameBuffer *>{};\n> >>>\n> >>> +\t/* Stop the IPA. */\n> >>> +\tdata->ipa_->stop();\n> >>> +\n> >>>  \tfreeBuffers(camera);\n> >>>  }\n> >>>\n> >>> @@ -1107,15 +1118,7 @@ int RPiCameraData::loadIPA()\n> >>>  \t\t.configurationFile = ipa_->configurationFile(sensor_->model() + \".json\")\n> >>>  \t};\n> >>>\n> >>> -\tipa_->init(settings);\n> >>> -\n> >>> -\t/*\n> >>> -\t * Startup the IPA thread now. Without this call, none of the IPA API\n> >>> -\t * functions will run.\n> >>> -\t *\n> >>> -\t * It only gets stopped in the class destructor.\n> >>> -\t */\n> >>> -\treturn ipa_->start();\n> >>> +\treturn ipa_->init(settings);\n> >>>  }\n> >>>\n> >>>  int RPiCameraData::configureIPA()","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 836E3BD792\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon,  6 Jul 2020 11:43:32 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 20073603AB;\n\tMon,  6 Jul 2020 13:43:32 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5C13B603AA\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon,  6 Jul 2020 13:43:30 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id D73C097E;\n\tMon,  6 Jul 2020 13:43:29 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"d9CHylOT\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1594035810;\n\tbh=aVwPSDexU2UN/YgR1hn2yTFXtskuq2wqCXzR2fKi8Qs=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=d9CHylOTakhbeXZJj+fSFdbFJIgTrQ9CYBKy3o2rtJ069gw/R3TboHFG6d60QJUZ+\n\tBqJtmxfRpDtDlUVeRhjD/9nkfiTd6Zt1frS/CEznZfnL1r7BFFToB4dmNbkwoRPVcH\n\t9NgVtNMppbtE5iP7QAOsv54cBF7DgLlFw4VzwEeU=","Date":"Mon, 6 Jul 2020 14:43:25 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Jacopo Mondi <jacopo@jmondi.org>","Message-ID":"<20200706114325.GF5912@pendragon.ideasonboard.com>","References":"<20200704005227.21782-1-laurent.pinchart@ideasonboard.com>\n\t<20200704005227.21782-13-laurent.pinchart@ideasonboard.com>\n\t<20200706093011.p5z5nsbcs7k6kjvj@uno.localdomain>\n\t<20200706110857.GA5912@pendragon.ideasonboard.com>\n\t<20200706113756.xpv67v2k34aogof4@uno.localdomain>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200706113756.xpv67v2k34aogof4@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v2 12/12] libcamera: pipeline:\n\traspberrypi: Start IPA when starting camera","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":11243,"web_url":"https://patchwork.libcamera.org/comment/11243/","msgid":"<CAEmqJPr9mAhg37WXjomTKkWBA8C4JgrdaOZuBW5VRZak4GSPOg@mail.gmail.com>","date":"2020-07-08T11:12:58","subject":"Re: [libcamera-devel] [PATCH v2 12/12] libcamera: pipeline:\n\traspberrypi: Start IPA when starting camera","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Laurent,\n\nThank you for the patch.\n\nOn Sat, 4 Jul 2020 at 01:52, Laurent Pinchart\n<laurent.pinchart@ideasonboard.com> wrote:\n>\n> The IPA is meant to be started when starting the camera, and stopped\n> when stopping it. It was so far start early in order to handle the\n> IPAInterface::processEvent() call related to lens shading table\n> allocation before IPAInterface::configure() to pass the table to the\n> IPA. Now that the lens shading table is passed through configure(),\n> starting the IPA early isn't needed anymore.\n>\n> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>\n\nOne question, what is the overhead of ipa_->start() and ipa_->stop().\nIf we have to do this on a mode switch preparing for a capture (where\nwe call pipeline_handler::stop() and pipeline_handler::start()), it\nmight increase capture latency.  Perhaps this does not matter, or we\ncan address if it is indeed a problem.  Otherwise,\n\nReviewed-by: Naushir Patuck <naush@raspberrypi.com>\n\n> ---\n>  .../pipeline/raspberrypi/raspberrypi.cpp      | 31 ++++++++++---------\n>  1 file changed, 17 insertions(+), 14 deletions(-)\n>\n> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> index 9babf9f4a19c..c877820a6e1e 100644\n> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> @@ -303,10 +303,6 @@ public:\n>                         vcsm_.free(lsTable_);\n>                         lsTable_ = nullptr;\n>                 }\n> -\n> -               /* Stop the IPA proxy thread. */\n> -               if (ipa_)\n> -                       ipa_->stop();\n>         }\n>\n>         void frameStarted(uint32_t sequence);\n> @@ -800,6 +796,16 @@ int PipelineHandlerRPi::start(Camera *camera)\n>         ret = queueAllBuffers(camera);\n>         if (ret) {\n>                 LOG(RPI, Error) << \"Failed to queue buffers\";\n> +               stop(camera);\n> +               return ret;\n> +       }\n> +\n> +       /* Start the IPA. */\n> +       ret = data->ipa_->start();\n> +       if (ret) {\n> +               LOG(RPI, Error)\n> +                       << \"Failed to start IPA for \" << camera->name();\n> +               stop(camera);\n>                 return ret;\n>         }\n>\n> @@ -810,8 +816,10 @@ int PipelineHandlerRPi::start(Camera *camera)\n>         V4L2DeviceFormat sensorFormat;\n>         data->unicam_[Unicam::Image].dev()->getFormat(&sensorFormat);\n>         ret = data->isp_[Isp::Input].dev()->setFormat(&sensorFormat);\n> -       if (ret)\n> +       if (ret) {\n> +               stop(camera);\n>                 return ret;\n> +       }\n>\n>         /* Enable SOF event generation. */\n>         data->unicam_[Unicam::Image].dev()->setFrameStartEnabled(true);\n> @@ -855,6 +863,9 @@ void PipelineHandlerRPi::stop(Camera *camera)\n>         data->bayerQueue_ = std::queue<FrameBuffer *>{};\n>         data->embeddedQueue_ = std::queue<FrameBuffer *>{};\n>\n> +       /* Stop the IPA. */\n> +       data->ipa_->stop();\n> +\n>         freeBuffers(camera);\n>  }\n>\n> @@ -1107,15 +1118,7 @@ int RPiCameraData::loadIPA()\n>                 .configurationFile = ipa_->configurationFile(sensor_->model() + \".json\")\n>         };\n>\n> -       ipa_->init(settings);\n> -\n> -       /*\n> -        * Startup the IPA thread now. Without this call, none of the IPA API\n> -        * functions will run.\n> -        *\n> -        * It only gets stopped in the class destructor.\n> -        */\n> -       return ipa_->start();\n> +       return ipa_->init(settings);\n>  }\n>\n>  int RPiCameraData::configureIPA()\n> --\n> Regards,\n>\n> Laurent Pinchart\n>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 3A246BD790\n\tfor <parsemail@patchwork.libcamera.org>;\n\tWed,  8 Jul 2020 11:13:17 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C28D161102;\n\tWed,  8 Jul 2020 13:13:16 +0200 (CEST)","from mail-lj1-x241.google.com (mail-lj1-x241.google.com\n\t[IPv6:2a00:1450:4864:20::241])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3552A60E05\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed,  8 Jul 2020 13:13:15 +0200 (CEST)","by mail-lj1-x241.google.com with SMTP id f5so37850209ljj.10\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 08 Jul 2020 04:13:15 -0700 (PDT)"],"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=\"MxwzMuYd\"; 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:content-transfer-encoding;\n\tbh=BCefi1aW7l1W3Z3Yws+cW5VPN1FQ9GxHcxmi2QVuOrE=;\n\tb=MxwzMuYdCvxWCgNvczj/zovk84F3dnmFQ6iGlfBuWwiw481t3Ouiy1OY5SKMzm1583\n\tnyU2CnuT0LSdcesNkBzu91ZBSTK+xj8tWnPx/6nLrgYPjYZXd/XXLk8C3H837msN1nOF\n\t70dn8FXxGvrTZrgU5dEdpuxmXj15/GD+wSUr09nZzpo71N9CyqYnXa498f/+9MILFJFd\n\thrIKX8Mo8TQA9RSZJMhT4CqZ5FW8QUj3ptp9t76n8BSOJkSJHYUYwpxAEiaRmQQxSXn+\n\tppLgfdMrD8oFvWTvvoJKV2wrHNOXGeyaFDYt9RXOLqs8gu33gBlmF8KTMN75HrTL9FdT\n\ttLqA==","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:content-transfer-encoding;\n\tbh=BCefi1aW7l1W3Z3Yws+cW5VPN1FQ9GxHcxmi2QVuOrE=;\n\tb=CHQKLF1jOT1oGeDZEhZv8ZOmrHw5crSkSKTyZF9Kb+SwjxijDM7dJVBFcW9/Ct46AQ\n\tztCHCGR40GlBTHnHtPDawP6w9QfQNaMkid9hzohkp+xq6RoJKIlB/OckMQyYmiyidiFI\n\tar+HIFHCkWMt5dHWfGxdyCdmij9eEilMOabW+q5tcZyGmwghsCy1c/TLTb7jE671Kez3\n\ty1oiR46z1Ag5QlC036mMYflddtwFvxph+W0pxJ4KcGr2SK4gAyCSZDdZHQXDdVHoBajs\n\tZjy5ZYH6vgP4dbZRQg7pevNW4xfdmcm37DL4WKcm6MqsyF7ypxWhSNQGAhZy5O1ex9ZE\n\tgJDQ==","X-Gm-Message-State":"AOAM5301dSX8ZOcSt0Np02DrytJ8ZXBznY/UBZ0dYyYDUb1MXYIIzdxD\n\tE3vqxNWMKXFZBL7VBc6fdW12XkqcYlTC5EpTAP0ni9F14iQ=","X-Google-Smtp-Source":"ABdhPJx7LoTHAnRasnQZVKWZ592d9+cvJ4PsJ5SiiufiXpyydO9RQNkEnAoxM1CAHCPxjnarcEEV8rR/jDDDP7iwv/s=","X-Received":"by 2002:a2e:b042:: with SMTP id\n\td2mr22064683ljl.252.1594206794034; \n\tWed, 08 Jul 2020 04:13:14 -0700 (PDT)","MIME-Version":"1.0","References":"<20200704005227.21782-1-laurent.pinchart@ideasonboard.com>\n\t<20200704005227.21782-13-laurent.pinchart@ideasonboard.com>","In-Reply-To":"<20200704005227.21782-13-laurent.pinchart@ideasonboard.com>","From":"Naushir Patuck <naush@raspberrypi.com>","Date":"Wed, 8 Jul 2020 12:12:58 +0100","Message-ID":"<CAEmqJPr9mAhg37WXjomTKkWBA8C4JgrdaOZuBW5VRZak4GSPOg@mail.gmail.com>","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2 12/12] libcamera: pipeline:\n\traspberrypi: Start IPA when starting camera","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":11257,"web_url":"https://patchwork.libcamera.org/comment/11257/","msgid":"<20200708204652.GR20298@pendragon.ideasonboard.com>","date":"2020-07-08T20:46:52","subject":"Re: [libcamera-devel] [PATCH v2 12/12] libcamera: pipeline:\n\traspberrypi: Start IPA when starting camera","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Naush,\n\nOn Wed, Jul 08, 2020 at 12:12:58PM +0100, Naushir Patuck wrote:\n> On Sat, 4 Jul 2020 at 01:52, Laurent Pinchart wrote:\n> >\n> > The IPA is meant to be started when starting the camera, and stopped\n> > when stopping it. It was so far start early in order to handle the\n> > IPAInterface::processEvent() call related to lens shading table\n> > allocation before IPAInterface::configure() to pass the table to the\n> > IPA. Now that the lens shading table is passed through configure(),\n> > starting the IPA early isn't needed anymore.\n> >\n> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n> > Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>\n> \n> One question, what is the overhead of ipa_->start() and ipa_->stop().\n> If we have to do this on a mode switch preparing for a capture (where\n> we call pipeline_handler::stop() and pipeline_handler::start()), it\n> might increase capture latency.  Perhaps this does not matter, or we\n> can address if it is indeed a problem.  Otherwise,\n\nThere's a small overhead, but I think it's offset by\nIPAInterface::configure() becoming a direct call instead of a\ncross-thread call. It may be more of a problem for closed IPA modules\nisolated in a separate process though, it's a good point. I could\nreconsider that, but I'd rather first align all implementations on the\ncurrent model, and then rework start/stop (possibly with the addition of\nprepare/finish to handle the expensive operations that are not related\nto starting and stopping the camera itself) on top.\n\n> Reviewed-by: Naushir Patuck <naush@raspberrypi.com>\n> \n> > ---\n> >  .../pipeline/raspberrypi/raspberrypi.cpp      | 31 ++++++++++---------\n> >  1 file changed, 17 insertions(+), 14 deletions(-)\n> >\n> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > index 9babf9f4a19c..c877820a6e1e 100644\n> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > @@ -303,10 +303,6 @@ public:\n> >                         vcsm_.free(lsTable_);\n> >                         lsTable_ = nullptr;\n> >                 }\n> > -\n> > -               /* Stop the IPA proxy thread. */\n> > -               if (ipa_)\n> > -                       ipa_->stop();\n> >         }\n> >\n> >         void frameStarted(uint32_t sequence);\n> > @@ -800,6 +796,16 @@ int PipelineHandlerRPi::start(Camera *camera)\n> >         ret = queueAllBuffers(camera);\n> >         if (ret) {\n> >                 LOG(RPI, Error) << \"Failed to queue buffers\";\n> > +               stop(camera);\n> > +               return ret;\n> > +       }\n> > +\n> > +       /* Start the IPA. */\n> > +       ret = data->ipa_->start();\n> > +       if (ret) {\n> > +               LOG(RPI, Error)\n> > +                       << \"Failed to start IPA for \" << camera->name();\n> > +               stop(camera);\n> >                 return ret;\n> >         }\n> >\n> > @@ -810,8 +816,10 @@ int PipelineHandlerRPi::start(Camera *camera)\n> >         V4L2DeviceFormat sensorFormat;\n> >         data->unicam_[Unicam::Image].dev()->getFormat(&sensorFormat);\n> >         ret = data->isp_[Isp::Input].dev()->setFormat(&sensorFormat);\n> > -       if (ret)\n> > +       if (ret) {\n> > +               stop(camera);\n> >                 return ret;\n> > +       }\n> >\n> >         /* Enable SOF event generation. */\n> >         data->unicam_[Unicam::Image].dev()->setFrameStartEnabled(true);\n> > @@ -855,6 +863,9 @@ void PipelineHandlerRPi::stop(Camera *camera)\n> >         data->bayerQueue_ = std::queue<FrameBuffer *>{};\n> >         data->embeddedQueue_ = std::queue<FrameBuffer *>{};\n> >\n> > +       /* Stop the IPA. */\n> > +       data->ipa_->stop();\n> > +\n> >         freeBuffers(camera);\n> >  }\n> >\n> > @@ -1107,15 +1118,7 @@ int RPiCameraData::loadIPA()\n> >                 .configurationFile = ipa_->configurationFile(sensor_->model() + \".json\")\n> >         };\n> >\n> > -       ipa_->init(settings);\n> > -\n> > -       /*\n> > -        * Startup the IPA thread now. Without this call, none of the IPA API\n> > -        * functions will run.\n> > -        *\n> > -        * It only gets stopped in the class destructor.\n> > -        */\n> > -       return ipa_->start();\n> > +       return ipa_->init(settings);\n> >  }\n> >\n> >  int RPiCameraData::configureIPA()","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 D0BD8BD792\n\tfor <parsemail@patchwork.libcamera.org>;\n\tWed,  8 Jul 2020 20:47:00 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6545160E05;\n\tWed,  8 Jul 2020 22:47:00 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A3BB660E05\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed,  8 Jul 2020 22:46:58 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 28AF9543;\n\tWed,  8 Jul 2020 22:46:58 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"jVYlSyZX\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1594241218;\n\tbh=BFI6w0tKepnmAZHDjmefeJLYHs9HZ4aJVP6rmgmRTxU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=jVYlSyZXRJY4zgFZM2gEAP9llznCYISp0zclCGj9I1vBURWfZ+J17i6u3tY3PBf9K\n\thfDG3hzb9ciNAQD8A8hDUxo0G1ntOSfyjQYBWTyONW3X0lZho+kJQJi6AY6Sm12MGh\n\tSDFMVzmhrYQX8ey9nhS7TcwDVNCX/RqGwNe6ZHPI=","Date":"Wed, 8 Jul 2020 23:46:52 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Naushir Patuck <naush@raspberrypi.com>","Message-ID":"<20200708204652.GR20298@pendragon.ideasonboard.com>","References":"<20200704005227.21782-1-laurent.pinchart@ideasonboard.com>\n\t<20200704005227.21782-13-laurent.pinchart@ideasonboard.com>\n\t<CAEmqJPr9mAhg37WXjomTKkWBA8C4JgrdaOZuBW5VRZak4GSPOg@mail.gmail.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<CAEmqJPr9mAhg37WXjomTKkWBA8C4JgrdaOZuBW5VRZak4GSPOg@mail.gmail.com>","Subject":"Re: [libcamera-devel] [PATCH v2 12/12] libcamera: pipeline:\n\traspberrypi: Start IPA when starting camera","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]