[{"id":12835,"web_url":"https://patchwork.libcamera.org/comment/12835/","msgid":"<20200928225320.GX23539@pendragon.ideasonboard.com>","date":"2020-09-28T22:53:20","subject":"Re: [libcamera-devel] [PATCH v3 21/22] libcamera: pipeline: rkisp1:\n\tUse the media link to track if a path is enabled","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Niklas,\n\nThank you for the patch.\n\nOn Fri, Sep 25, 2020 at 03:42:06AM +0200, Niklas Söderlund wrote:\n> Instead of manually track if a path is enable or not use the media graph\n\ns/track/tracking/\n\n> link status. This enables RkISP1Path::start() to check the link status\n> thus allowing it to always be called from PipelineHandlerRkISP1::start()\n> making the code simpler. There is no functional changed.\n\ns/changed/change/\n\n> Signed-off-by: Niklas Söderlund \n> ---\n> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 82 +++++++++-----------\n> src/libcamera/pipeline/rkisp1/rkisp1path.cpp | 3 +\n> src/libcamera/pipeline/rkisp1/rkisp1path.h | 1 +\n> 3 files changed, 39 insertions(+), 47 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index e41a9a51bda576b0..c6c2e3aa3dc6d0dc 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -122,8 +122,7 @@ public:\n> \tRkISP1CameraData(PipelineHandler *pipe, RkISP1MainPath *mainPath,\n> \t\t\t RkISP1SelfPath *selfPath)\n> \t\t: CameraData(pipe), sensor_(nullptr), frame_(0),\n> -\t\t frameInfo_(pipe), mainPath_(mainPath), selfPath_(selfPath),\n> -\t\t mainPathActive_(false), selfPathActive_(false)\n> +\t\t frameInfo_(pipe), mainPath_(mainPath), selfPath_(selfPath)\n> \t{\n> \t}\n> \n> @@ -145,9 +144,6 @@ public:\n> \tRkISP1MainPath *mainPath_;\n> \tRkISP1SelfPath *selfPath_;\n> \n> -\tbool mainPathActive_;\n> -\tbool selfPathActive_;\n> -\n> private:\n> \tvoid queueFrameAction(unsigned int frame,\n> \t\t\t const IPAOperationData &action);\n> @@ -714,20 +710,14 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)\n> \n> \tLOG(RkISP1, Debug) << \"ISP output pad configured with \" << format.toString();\n> \n> -\tdata->mainPathActive_ = false;\n> -\tdata->selfPathActive_ = false;\n> \tfor (const StreamConfiguration &cfg : *config) {\n> -\t\tif (cfg.stream() == &data->mainPathStream_) {\n> +\t\tif (cfg.stream() == &data->mainPathStream_)\n> \t\t\tret = mainPath_.configure(cfg, format);\n> -\t\t\tif (ret)\n> -\t\t\t\treturn ret;\n> -\t\t\tdata->mainPathActive_ = true;\n> -\t\t} else {\n> +\t\telse\n> \t\t\tret = selfPath_.configure(cfg, format);\n> -\t\t\tif (ret)\n> -\t\t\t\treturn ret;\n> -\t\t\tdata->selfPathActive_ = true;\n> -\t\t}\n> +\n> +\t\tif (ret)\n> +\t\t\treturn ret;\n> \t}\n> \n> \tV4L2DeviceFormat paramFormat = {};\n> @@ -871,39 +861,23 @@ int PipelineHandlerRkISP1::start(Camera *camera)\n> \t\treturn ret;\n> \t}\n> \n> -\tstd::map streamConfig;\n> -\n> -\tif (data->mainPathActive_) {\n> -\t\tret = mainPath_.start();\n> -\t\tif (ret) {\n> -\t\t\tparam_->streamOff();\n> -\t\t\tstat_->streamOff();\n> -\t\t\tdata->ipa_->stop();\n> -\t\t\tfreeBuffers(camera);\n> -\t\t\treturn ret;\n> -\t\t}\n> -\n> -\t\tstreamConfig[0] = {\n> -\t\t\t.pixelFormat = data->mainPathStream_.configuration().pixelFormat,\n> -\t\t\t.size = data->mainPathStream_.configuration().size,\n> -\t\t};\n\nI think I'd prefer keeping a mainPath_.isEnabled() check here, as the\ncode would look confusing for the casual reader. You need such a check\nfor the streamConfig handling below anyway, so just replacing\ndata->*PathActive_ with *Path_.isEnabled() should be enough.\n\n> +\tret = mainPath_.start();\n> +\tif (ret) {\n> +\t\tparam_->streamOff();\n> +\t\tstat_->streamOff();\n> +\t\tdata->ipa_->stop();\n> +\t\tfreeBuffers(camera);\n> +\t\treturn ret;\n> \t}\n> \n> -\tif (data->selfPathActive_) {\n> -\t\tret = selfPath_.start();\n> -\t\tif (ret) {\n> -\t\t\tmainPath_.stop();\n> -\t\t\tparam_->streamOff();\n> -\t\t\tstat_->streamOff();\n> -\t\t\tdata->ipa_->stop();\n> -\t\t\tfreeBuffers(camera);\n> -\t\t\treturn ret;\n> -\t\t}\n> -\n> -\t\tstreamConfig[1] = {\n> -\t\t\t.pixelFormat = data->selfPathStream_.configuration().pixelFormat,\n> -\t\t\t.size = data->selfPathStream_.configuration().size,\n> -\t\t};\n> +\tret = selfPath_.start();\n> +\tif (ret) {\n> +\t\tmainPath_.stop();\n> +\t\tparam_->streamOff();\n> +\t\tstat_->streamOff();\n> +\t\tdata->ipa_->stop();\n> +\t\tfreeBuffers(camera);\n> +\t\treturn ret;\n> \t}\n> \n> \tactiveCamera_ = camera;\n> @@ -918,6 +892,20 @@ int PipelineHandlerRkISP1::start(Camera *camera)\n> \t\tret = 0;\n> \t}\n> \n> +\tstd::map streamConfig;\n> +\tif (data->mainPath_->isEnabled()) {\n> +\t\tstreamConfig[0] = {\n> +\t\t\t.pixelFormat = data->mainPathStream_.configuration().pixelFormat,\n> +\t\t\t.size = data->mainPathStream_.configuration().size,\n> +\t\t};\n> +\t}\n> +\tif (data->selfPath_->isEnabled()) {\n> +\t\tstreamConfig[1] = {\n> +\t\t\t.pixelFormat = data->selfPathStream_.configuration().pixelFormat,\n> +\t\t\t.size = data->selfPathStream_.configuration().size,\n> +\t\t};\n> +\t}\n> +\n> \tstd::map entityControls;\n> \tentityControls.emplace(0, data->sensor_->controls());\n> \n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1path.cpp\n> index 8010cb92423c8269..623ee91888acd983 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1path.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1path.cpp\n> @@ -163,6 +163,9 @@ int RkISP1Path::start()\n> {\n> \tint ret;\n> \n> +\tif (!isEnabled())\n> +\t\treturn 0;\n> +\n> \tif (running_)\n> \t\treturn -EBUSY;\n> \n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1path.h b/src/libcamera/pipeline/rkisp1/rkisp1path.h\n> index f57f4b646c3a3164..01f9c751f9e54c64 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1path.h\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1path.h\n> @@ -34,6 +34,7 @@ public:\n> \tbool init(MediaDevice *media);\n> \n> \tint enable() { return link_->setEnabled(true); }\n> +\tbool isEnabled() { return link_->flags() & MEDIA_LNK_FL_ENABLED; }\n\nIt could look a bit confusing to the reader that there's no disable()\ncall anywhere. Maybe a comment somewhere would be useful.\n\n> \n> \tStreamConfiguration generateConfiguration(const Size &resolution);\n> \tCameraConfiguration::Status validate(StreamConfiguration *cfg);","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 F181BC3B5C\n\tfor ;\n\tMon, 28 Sep 2020 22:53:56 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7D5856138F;\n\tTue, 29 Sep 2020 00:53:56 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6333260394\n\tfor ;\n\tTue, 29 Sep 2020 00:53:55 +0200 (CEST)","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 E1F4AA58;\n\tTue, 29 Sep 2020 00:53:54 +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=\"ZQe6mqBf\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1601333635;\n\tbh=ZKb0NC8mnvvu3A6ufS3yQ1TLWxZKDT1t4+Wkf4Zk1X8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=ZQe6mqBflwlA957JnHSMkeL4hOXIsmyk8xuAKOMpFJXDQo1VoEhy/NkOSAD0y24Hg\n\t4wkMTqC88cHZADRp89ojOM7s3O8Wke5A315885thNnGSeW+G+4ZoGwoKLTKG8ETqV3\n\t99YWaLn8msQodPbiHdSXCKm34FgsIOKeNlN5cnas=","Date":"Tue, 29 Sep 2020 01:53:20 +0300","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Message-ID":"<20200928225320.GX23539@pendragon.ideasonboard.com>","References":"<20200925014207.1455796-1-niklas.soderlund@ragnatech.se>\n\t<20200925014207.1455796-22-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200925014207.1455796-22-niklas.soderlund@ragnatech.se>","Subject":"Re: [libcamera-devel] [PATCH v3 21/22] libcamera: pipeline: rkisp1:\n\tUse the media link to track if a path is enabled","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=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]