[{"id":23522,"web_url":"https://patchwork.libcamera.org/comment/23522/","msgid":"<165588725204.1149771.6518262442621291445@Monstersaurus>","date":"2022-06-22T08:40:52","subject":"Re: [libcamera-devel] [PATCH] pipeline: rkisp1: Support media graph\n\twith separate CSI RX","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Paul Elder via libcamera-devel (2022-06-22 08:46:38)\n> The rkisp1 hardware supports a parallel interface, where the sensor\n> would be connected directly, as well as a CSI receiver. Although at the\n> moment the rkisp1 driver doesn't expose the CSI receiver as a separate\n> subdev, this patch allows the rkisp1 pipeline handler to continue\n> working even in the event that it eventually does.\n> \n> Signed-off-by: Paul Elder \n> ---\n> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 35 ++++++++++++++++++++++--\n> 1 file changed, 33 insertions(+), 2 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index 43b76e14..a233c961 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -178,6 +178,7 @@ private:\n> std::unique_ptr isp_;\n> std::unique_ptr param_;\n> std::unique_ptr stat_;\n> + std::unique_ptr csi_;\n> \n> RkISP1MainPath mainPath_;\n> RkISP1SelfPath selfPath_;\n> @@ -591,6 +592,12 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)\n> \n> LOG(RkISP1, Debug) << \"Sensor configured with \" << format;\n> \n> + if (csi_) {\n> + ret = csi_->setFormat(0, &format);\n> + if (ret < 0)\n> + return ret;\n> + }\n> +\n> ret = isp_->setFormat(0, &format);\n> if (ret < 0)\n> return ret;\n> @@ -892,7 +899,7 @@ int PipelineHandlerRkISP1::initLinks(Camera *camera,\n> * Configure the sensor links: enable the link corresponding to this\n> * camera.\n> */\n> - const MediaPad *pad = isp_->entity()->getPadByIndex(0);\n> + const MediaPad *pad = (csi_ ? csi_ : isp_)->entity()->getPadByIndex(0);\n> for (MediaLink *link : pad->links()) {\n> if (link->source()->entity() != sensor->entity())\n> continue;\n> @@ -907,6 +914,18 @@ int PipelineHandlerRkISP1::initLinks(Camera *camera,\n> return ret;\n> }\n> \n> + if (csi_) {\n> + const MediaPad *ispPad = isp_->entity()->getPadByIndex(0);\n> + for (MediaLink *link : ispPad->links()) {\n> + if (link->source()->entity() != csi_->entity())\n> + continue;\n> +\n> + ret = link->setEnabled(true);\n> + if (ret < 0)\n> + return ret;\n> + }\n> + }\n> +\n> for (const StreamConfiguration &cfg : config) {\n> if (cfg.stream() == &data->mainPathStream_)\n> ret = data->mainPath_->setEnabled(true);\n> @@ -1005,6 +1024,18 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)\n> if (isp_->open() < 0)\n> return false;\n> \n> + pad = isp_->entity()->getPadByIndex(0);\n> + if (!pad || pad->links().size() != 1)\n> + return false;\n> +\n> + csi_ = std::make_unique(pad->links().at(0)->source()->entity());\n> + if (!csi_ || csi_->open() < 0)\n> + return false;\n\nYour commit message talks about how currently there is no CSI subdev, and this\npatch allows it to use it if there is one.\n\nBut doesn't this statement mean it will fail to fully match if there is\nno CSI device? \n\n> +\n> + /* If the media device has no 1th pad, it's the sensor */\n> + if (!csi_->entity()->getPadByIndex(1))\n> + csi_ = nullptr;\n> +\n> /* Locate and open the stats and params video nodes. */\n> stat_ = V4L2VideoDevice::fromEntityName(media_, \"rkisp1_stats\");\n\nLike for instance, this code would not be executed anymore if !(csi_)\n\n--\nKieran\n\n> if (stat_->open() < 0)\n> @@ -1030,7 +1061,7 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)\n> * Enumerate all sensors connected to the ISP and create one\n> * camera instance for each of them.\n> */\n> - pad = isp_->entity()->getPadByIndex(0);\n> + pad = (csi_ ? csi_ : isp_)->entity()->getPadByIndex(0);\n> if (!pad)\n> return false;\n> \n> -- \n> 2.30.2\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 70D20BE173\n\tfor ;\n\tWed, 22 Jun 2022 08:40:57 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CAE5365632;\n\tWed, 22 Jun 2022 10:40: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 3F28B6559A\n\tfor ;\n\tWed, 22 Jun 2022 10:40:55 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id C548ADD;\n\tWed, 22 Jun 2022 10:40:54 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1655887256;\n\tbh=pGKy/2hKfoM664qiUfLif2Xn8wfcKyXbzqvslu/+Vyw=;\n\th=In-Reply-To:References:To:Date:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:\n\tFrom;\n\tb=GP+Ci/IoQpoIxM+RUhRzadKfOGJLybKC9JcIJDaR8H8U39fjXBn7FdDygncNcdt7g\n\tSK0n1DST+PRcgEPZMCpzPvP+Sjg/aYc5TPOZdo52FnlsQtKaZ+Gn2c1A0A7JBO3y92\n\tLmMOXa9AkLWvqqkknz+O1F2oiRUbu+c0Kekxs1GQXFj81jrt8aBUx14twrrmmNTss+\n\tD4RTruouKK02TJVWy0YMhhtsPFLon3ynzvi6+QpPej/842l4RK+Nntxm+B1RYlSKN+\n\tFbOvNRWoct0XSrYjIfhfIe78Hns5i4dzaWY8ZY/5oEeRsJi6RsqHXDMwSNuchntjo3\n\taXA80jVzkFpSw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1655887254;\n\tbh=pGKy/2hKfoM664qiUfLif2Xn8wfcKyXbzqvslu/+Vyw=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=rZEcWVIEB1XAA0Mu95HN+Zq1EQPq/JWA4UJWDVjJzyicUP7WIYUAc8jwitZIMtO+f\n\tIkHs7XmN5fj3sbMmP+hyfBxuWpxbjzdAXsehPisMyXkmPCgtPBj+weRovarsA3aI6p\n\tV3XFX+gRasYPpdB51vx3YbCQsZXmsBZphC8ioblY="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"rZEcWVIE\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20220622074638.144636-1-paul.elder@ideasonboard.com>","References":"<20220622074638.144636-1-paul.elder@ideasonboard.com>","To":"Paul Elder ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Wed, 22 Jun 2022 09:40:52 +0100","Message-ID":"<165588725204.1149771.6518262442621291445@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH] pipeline: rkisp1: Support media graph\n\twith separate CSI RX","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","From":"Kieran Bingham via libcamera-devel\n\t","Reply-To":"Kieran Bingham ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":23523,"web_url":"https://patchwork.libcamera.org/comment/23523/","msgid":"","date":"2022-06-22T08:46:57","subject":"Re: [libcamera-devel] [PATCH] pipeline: rkisp1: Support media graph\n\twith separate CSI RX","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Paul,\n\nThank you for the patch.\n\nOn Wed, Jun 22, 2022 at 04:46:38PM +0900, Paul Elder via libcamera-devel wrote:\n> The rkisp1 hardware supports a parallel interface, where the sensor\n> would be connected directly, as well as a CSI receiver. Although at the\n> moment the rkisp1 driver doesn't expose the CSI receiver as a separate\n> subdev, this patch allows the rkisp1 pipeline handler to continue\n> working even in the event that it eventually does.\n\nThe rkisp1 hardware supports both a CSI-2 input and a parallel input,\nwhere the sensor is connected directly to the ISP. On RK3399, the CSI-2\nreceiver is internal, but on the i.MX8MP, the CSI-2 receiver is a\nseparate IP core, connected to the parallel input of the ISP, and gets\nexposed to userspace as a V4L2 subdev. To prepare for this, handle an\noptional CSI-2 receiver subdev in the pipeline.\n\n> Signed-off-by: Paul Elder \n> ---\n> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 35 ++++++++++++++++++++++--\n> 1 file changed, 33 insertions(+), 2 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index 43b76e14..a233c961 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -178,6 +178,7 @@ private:\n> \tstd::unique_ptr isp_;\n> \tstd::unique_ptr param_;\n> \tstd::unique_ptr stat_;\n> +\tstd::unique_ptr csi_;\n> \n> \tRkISP1MainPath mainPath_;\n> \tRkISP1SelfPath selfPath_;\n> @@ -591,6 +592,12 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)\n> \n> \tLOG(RkISP1, Debug) << \"Sensor configured with \" << format;\n> \n> +\tif (csi_) {\n> +\t\tret = csi_->setFormat(0, &format);\n> +\t\tif (ret < 0)\n> +\t\t\treturn ret;\n> +\t}\n> +\n> \tret = isp_->setFormat(0, &format);\n> \tif (ret < 0)\n> \t\treturn ret;\n> @@ -892,7 +899,7 @@ int PipelineHandlerRkISP1::initLinks(Camera *camera,\n> \t * Configure the sensor links: enable the link corresponding to this\n> \t * camera.\n> \t */\n> -\tconst MediaPad *pad = isp_->entity()->getPadByIndex(0);\n> +\tconst MediaPad *pad = (csi_ ? csi_ : isp_)->entity()->getPadByIndex(0);\n> \tfor (MediaLink *link : pad->links()) {\n> \t\tif (link->source()->entity() != sensor->entity())\n> \t\t\tcontinue;\n> @@ -907,6 +914,18 @@ int PipelineHandlerRkISP1::initLinks(Camera *camera,\n> \t\t\treturn ret;\n> \t}\n> \n> +\tif (csi_) {\n> +\t\tconst MediaPad *ispPad = isp_->entity()->getPadByIndex(0);\n> +\t\tfor (MediaLink *link : ispPad->links()) {\n> +\t\t\tif (link->source()->entity() != csi_->entity())\n> +\t\t\t\tcontinue;\n> +\n> +\t\t\tret = link->setEnabled(true);\n> +\t\t\tif (ret < 0)\n> +\t\t\t\treturn ret;\n\nI think you can break here.\n\n> +\t\t}\n> +\t}\n> +\n> \tfor (const StreamConfiguration &cfg : config) {\n> \t\tif (cfg.stream() == &data->mainPathStream_)\n> \t\t\tret = data->mainPath_->setEnabled(true);\n> @@ -1005,6 +1024,18 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)\n> \tif (isp_->open() < 0)\n> \t\treturn false;\n> \n> +\tpad = isp_->entity()->getPadByIndex(0);\n> +\tif (!pad || pad->links().size() != 1)\n\nThe second check will break Scarlet, as it has two sensors attached\ndirectly to the ISP sink pad. Let's replace it with a\npad->links().empty() check.\n\n> +\t\treturn false;\n> +\n> +\tcsi_ = std::make_unique(pad->links().at(0)->source()->entity());\n> +\tif (!csi_ || csi_->open() < 0)\n> +\t\treturn false;\n> +\n> +\t/* If the media device has no 1th pad, it's the sensor */\n> +\tif (!csi_->entity()->getPadByIndex(1))\n\nWe could have a sensor with multiple subdevs, so this isn't very\nfuture-proof. How about testing the entity function ? The CSI-2 receiver\nsubdev uses MEDIA_ENT_F_VID_IF_BRIDGE.\n\n> +\t\tcsi_ = nullptr;\n\nCould you avoid creating the csi_ to destroy it right after in that case\n? Something like\n\n\t/* Locate and open the optional CSI-2 receiver. */\n\tMediaPad *ispSink = isp_->entity()->getPadByIndex(0);\n\tif (!ispSink || ispSink->links().empty())\n\t\treturn false;\n\n\tpad = ispSink->links().at(0)->source();\n\tif (pad->entity()->function() == MEDIA_ENT_F_VID_IF_BRIDGE) {\n\t\tcsi_ = std::make_unique(pad->entity());\n\t\tif (!csi_ || csi_->open() < 0)\n\t\t\treturn false;\n\n\t\tispSink = csi_->entity()->getPadByIndex(0);\n\t\tif (!ispSink)\n\t\t\treturn false;\n\t}\n\n> +\n> \t/* Locate and open the stats and params video nodes. */\n> \tstat_ = V4L2VideoDevice::fromEntityName(media_, \"rkisp1_stats\");\n> \tif (stat_->open() < 0)\n> @@ -1030,7 +1061,7 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)\n> \t * Enumerate all sensors connected to the ISP and create one\n> \t * camera instance for each of them.\n> \t */\n> -\tpad = isp_->entity()->getPadByIndex(0);\n> +\tpad = (csi_ ? csi_ : isp_)->entity()->getPadByIndex(0);\n> \tif (!pad)\n> \t\treturn false;\n\nAnd you can then drop this, and use ispSink instead of pad in the loop\nbelow.\n\nI'm actually tempted to make ispSink a member variable, so you could\nthen use it in PipelineHandlerRkISP1::initLinks() instead of duplicating\nlogic.\n\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 6EB42BD808\n\tfor ;\n\tWed, 22 Jun 2022 08:47:15 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BE0EE65638;\n\tWed, 22 Jun 2022 10:47:14 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5988A6559A\n\tfor ;\n\tWed, 22 Jun 2022 10:47:13 +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 B5FDC31A;\n\tWed, 22 Jun 2022 10:47:12 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1655887634;\n\tbh=zvK1c+r3N8iAYvmzn6pA7nk6hgrTJ+oUblEycMHjVME=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=k3eGpiqjjvBvJHjI/fbILhLVvj597vjzm81I/ll7pDTT8SmhKjXvCC0IQblRW5m5u\n\tbFf3I31ChDsD/SuCpkON+xpc2N7MkwAg4ZYRy6nLNH4eq9MjREN2DXXMyjbsxm4HH0\n\t6htoJ1nDxqe9BgtOR5MYH9Q5dlTJPbjSn7KaZirCTJ1/uwlsDAqr5HD8lU14xI5TvA\n\tdAXvnzX63nh8X0JJ3TL5H2etBT3yR354Y25m8jrDGGFB9/wLLoId5+KUNCM0CYZW+1\n\tCr/M9fLdy+V7G0nsrb3WOY9Q7ZfHolmpCfrpBn9amGGNPUk1rDC47+YawM5reeP92v\n\t62q7LAZX73F/Q==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1655887632;\n\tbh=zvK1c+r3N8iAYvmzn6pA7nk6hgrTJ+oUblEycMHjVME=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=URzhnsRdoRe+KhA4Mig347y/qYgj/h0RzGhVPfMJVHK2A1KUl957yXtZILRk/0pDo\n\t4iyEAh++EFtTGDlhx8TWucTF2p4j0yVbV1QanR5oGGQlcNIZjuio5UjYm0ibYaaOic\n\tAoK0RC9UW/YT6BVKqsUneGXWifaAEmNlIdhclbHo="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"URzhnsRd\"; dkim-atps=neutral","Date":"Wed, 22 Jun 2022 11:46:57 +0300","To":"Paul Elder ","Message-ID":"","References":"<20220622074638.144636-1-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220622074638.144636-1-paul.elder@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH] pipeline: rkisp1: Support media graph\n\twith separate CSI RX","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","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]