[{"id":34723,"web_url":"https://patchwork.libcamera.org/comment/34723/","msgid":"<20250629225855.GB20333@pendragon.ideasonboard.com>","date":"2025-06-29T22:58:55","subject":"Re: [PATCH v2 2/2] pipeline: imx8-isi: Add multicamera support","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Antoine,\n\nThank you for the patch.\n\nOn Wed, Jun 18, 2025 at 02:45:41PM +0200, Antoine Bouyer wrote:\n> ISI can be used to support multiple cameras at the same time in the media\n> device. A dedicated pad is assigned to each camera by HW (dts). We then\n> assign one (or more) source pad(s) depending on ISI constraints in the SoC.\n> \n> Since ISI may have different number of pipes depending on SoC, the number\n> of cameras is computed _before_ going through all pipeline's components,\n> to limit number of pipes that could be assigned to a camera.\n> \n> For instance, 3 is targeted as maximum amount of streams per camera as\n> defined by 'kNumStreams' constant. If 2 cameras are connected, with\n> only 5 ISI pipes, this target cannot be achieved. In such case, 2 streams\n> are assigned to each camera.\n> \n> On the other hand, if ISI has 8 source pads (from index 6 to 13) and 2\n> cameras, first three source pads [6:8] are assigned to first camera, and\n> the three next source pads [9:11] are assigned to the second camera. All\n> these pads (sink and sources) must have same format to prevent configuration\n> mismatch during start.\n> \n> However, since ISI routing must be performed _before_ any stream becomes\n> active, 'setRouting' is now executed during the 'match' step, instead of\n> 'configure'. Indeed, it is up to the application to decide when a camera\n> 'start' is executed: this could happen before or after the other camera\n> configuration is executed. Moving routing into the 'match' step makes sure\n> the routing is correctly applied before any 'start' is executed.\n> \n> Signed-off-by: Antoine Bouyer \n> ---\n> src/libcamera/pipeline/imx8-isi/imx8-isi.cpp | 112 +++++++++++++------\n> 1 file changed, 75 insertions(+), 37 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp b/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp\n> index 186b623df186..5a22d761aefe 100644\n> --- a/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp\n> +++ b/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp\n> @@ -55,7 +55,7 @@ public:\n> \n> \tunsigned int pipeIndex(const Stream *stream)\n> \t{\n> -\t\treturn stream - &*streams_.begin();\n> +\t\treturn stream - &*streams_.begin() + xbarSourceOffset_;\n> \t}\n> \n> \tunsigned int getRawMediaBusFormat(PixelFormat *pixelFormat) const;\n> @@ -69,7 +69,8 @@ public:\n> \n> \tstd::vector enabledStreams_;\n> \n> -\tunsigned int xbarSink_;\n> +\tunsigned int xbarSink_ = 0;\n> +\tunsigned int xbarSourceOffset_ = 0;\n> };\n> \n> class ISICameraConfiguration : public CameraConfiguration\n> @@ -809,34 +810,9 @@ int PipelineHandlerISI::configure(Camera *camera, CameraConfiguration *c)\n> \tISICameraConfiguration *camConfig = static_cast(c);\n> \tISICameraData *data = cameraData(camera);\n> \n> -\t/* All links are immutable except the sensor -> csis link. */\n> -\tconst MediaPad *sensorSrc = data->sensor_->entity()->getPadByIndex(0);\n> -\tsensorSrc->links()[0]->setEnabled(true);\n> -\n> -\t/*\n> -\t * Reset the crossbar switch routing and enable one route for each\n> -\t * requested stream configuration.\n> -\t *\n> -\t * \\todo Handle concurrent usage of multiple cameras by adjusting the\n> -\t * routing table instead of resetting it.\n> -\t */\n> -\tV4L2Subdevice::Routing routing = {};\n> -\tunsigned int xbarFirstSource = crossbar_->entity()->pads().size() / 2 + 1;\n> -\n> -\tfor (const auto &[idx, config] : utils::enumerate(*c)) {\n> -\t\tuint32_t sourcePad = xbarFirstSource + idx;\n> -\t\trouting.emplace_back(V4L2Subdevice::Stream{ data->xbarSink_, 0 },\n> -\t\t\t\t V4L2Subdevice::Stream{ sourcePad, 0 },\n> -\t\t\t\t V4L2_SUBDEV_ROUTE_FL_ACTIVE);\n> -\t}\n> -\n> -\tint ret = crossbar_->setRouting(&routing, V4L2Subdevice::ActiveFormat);\n> -\tif (ret)\n> -\t\treturn ret;\n> -\n> -\t/* Apply format to the sensor and CSIS receiver. */\n> +\t/* Apply format to the sensor, CSIS receiver and crossbar sink pad. */\n> \tV4L2SubdeviceFormat format = camConfig->sensorFormat_;\n> -\tret = data->sensor_->setFormat(&format);\n> +\tint ret = data->sensor_->setFormat(&format);\n> \tif (ret)\n> \t\treturn ret;\n> \n> @@ -848,10 +824,17 @@ int PipelineHandlerISI::configure(Camera *camera, CameraConfiguration *c)\n> \tif (ret)\n> \t\treturn ret;\n> \n> -\t/* Now configure the ISI and video node instances, one per stream. */\n> -\tdata->enabledStreams_.clear();\n> -\tfor (const auto &config : *c) {\n> -\t\tPipe *pipe = pipeFromStream(camera, config.stream());\n> +\t/*\n> +\t * As links on the output of the crossbar switch are immutable, the\n> +\t * routing table configured at match() time creates a media pipeline\n> +\t * that includes all the ISI pipelines corresponding to streams of this\n> +\t * camera, regardless of whether or not the streams are used in the\n> +\t * camera configuration. Set the format on the sink pad of all\n> +\t * corresponding ISI pipelines to avoid link validation failures when\n> +\t * starting streaming on the media pipeline.\n> +\t */\n> +\tfor (unsigned i = 0; i < data->streams_.size(); i++) {\n> +\t\tPipe *pipe = &pipes_.at(data->xbarSourceOffset_ + i);\n> \n> \t\t/*\n> \t\t * Set the format on the ISI sink pad: it must match what is\n> @@ -860,6 +843,15 @@ int PipelineHandlerISI::configure(Camera *camera, CameraConfiguration *c)\n> \t\tret = pipe->isi->setFormat(0, &format);\n> \t\tif (ret)\n> \t\t\treturn ret;\n> +\t}\n> +\n> +\t/*\n> +\t * Now configure the ISI pipeline source pad and video node instances,\n> +\t * one per enabled stream.\n> +\t */\n> +\tdata->enabledStreams_.clear();\n> +\tfor (const auto &config : *c) {\n> +\t\tPipe *pipe = pipeFromStream(camera, config.stream());\n> \n> \t\t/*\n> \t\t * Configure the ISI sink compose rectangle to downscale the\n> @@ -971,6 +963,20 @@ bool PipelineHandlerISI::match(DeviceEnumerator *enumerator)\n> \tif (!isiDev_)\n> \t\treturn false;\n> \n> +\t/* Count the number of sensors, to create one camera per sensor. */\n> +\tunsigned cameraCount = 0;\n> +\tfor (MediaEntity *entity : isiDev_->entities()) {\n> +\t\tif (entity->function() != MEDIA_ENT_F_CAM_SENSOR)\n> +\t\t\tcontinue;\n> +\n> +\t\tcameraCount++;\n> +\t}\n> +\n> +\tif (!cameraCount) {\n> +\t\tLOG(ISI, Error) << \"No camera sensor found\";\n> +\t\treturn false;\n> +\t}\n> +\n> \t/*\n> \t * Acquire the subdevs and video nodes for the crossbar switch and the\n> \t * processing pipelines.\n> @@ -1014,12 +1020,19 @@ bool PipelineHandlerISI::match(DeviceEnumerator *enumerator)\n> \t\treturn false;\n> \t}\n> \n> +\tif (cameraCount > pipes_.size()) {\n> +\t\tLOG(ISI, Error) << \"Too many cameras\";\n> +\t\treturn false;\n> +\t}\n> +\n> \t/*\n> \t * Loop over all the crossbar switch sink pads to find connected CSI-2\n> \t * receivers and camera sensors.\n> \t */\n> \tunsigned int numCameras = 0;\n> \tunsigned int numSinks = 0;\n> +\tunsigned int xbarFirstSource = crossbar_->entity()->pads().size() - pipes_.size();\n\nLet's make this const to clearly indicate it won't change later, that\nhelps reading the code.\n\n\tconst unsigned int xbarFirstSource = crossbar_->entity()->pads().size()\n\t\t\t\t\t - pipes_.size();\n\n> +\tV4L2Subdevice::Routing routing = {};\n\nA blank line here would be nice.\n\n> \tfor (MediaPad *pad : crossbar_->entity()->pads()) {\n> \t\tunsigned int sink = numSinks;\n> \n> @@ -1050,17 +1063,30 @@ bool PipelineHandlerISI::match(DeviceEnumerator *enumerator)\n> \t\t\tcontinue;\n> \t\t}\n> \n> -\t\t/* Create the camera data. */\n> +\t\t/* All links are immutable except the sensor -> csis link. */\n> +\t\tconst MediaPad *sensorSrc = sensor->getPadByIndex(0);\n> +\t\tsensorSrc->links()[0]->setEnabled(true);\n> +\n> \t\t/*\n> -\t\t * \\todo compute available pipes per camera instead of using\n> -\t\t * pipes_.size() for multi cameras case.\n> +\t\t * Define maximum amount of streams per camera. In case of\n> +\t\t * multiple cameras, limit maximum amount of streams to allow\n> +\t\t * all cameras to get at least one dedicated pipe.\n> \t\t */\n> +\t\tunsigned int maxStreams = pipes_.size() / cameraCount;\n\ncameraCount and pipes_.size() are constants through the loop, so you can\nmove this variable before the loop, just after xbarFirstSource (and make\nit const too).\n\nWith these small changes,\n\nReviewed-by: Laurent Pinchart \n\nI could make the modifications locally when applying the patches, but\nI'd appreciate if you could test them, if you don't mind sending a v3\n(with the R-b tags included).\n\n> +\n> +\t\t/* Create the camera data. */\n> \t\tstd::unique_ptr data =\n> -\t\t\tstd::make_unique(this, pipes_.size());\n> +\t\t\tstd::make_unique(this, maxStreams);\n> \n> \t\tdata->sensor_ = CameraSensorFactoryBase::create(sensor);\n> \t\tdata->csis_ = std::make_unique(csi);\n> \t\tdata->xbarSink_ = sink;\n> +\t\tdata->xbarSourceOffset_ = numCameras * data->streams_.size();\n> +\n> +\t\tLOG(ISI, Debug)\n> +\t\t\t<< \"cam\" << numCameras\n> +\t\t\t<< \" streams \" << data->streams_.size()\n> +\t\t\t<< \" offset \" << data->xbarSourceOffset_;\n> \n> \t\tret = data->init();\n> \t\tif (ret) {\n> @@ -1075,6 +1101,14 @@ bool PipelineHandlerISI::match(DeviceEnumerator *enumerator)\n> \t\t\t std::inserter(streams, streams.end()),\n> \t\t\t [](Stream &s) { return &s; });\n> \n> +\t\t/* Add routes to the crossbar switch routing table. */\n> +\t\tfor (unsigned i = 0; i < data->streams_.size(); i++) {\n> +\t\t\tunsigned int sourcePad = xbarFirstSource + data->xbarSourceOffset_ + i;\n> +\t\t\trouting.emplace_back(V4L2Subdevice::Stream{ data->xbarSink_, 0 },\n> +\t\t\t\t\t V4L2Subdevice::Stream{ sourcePad, 0 },\n> +\t\t\t\t\t V4L2_SUBDEV_ROUTE_FL_ACTIVE);\n> +\t\t}\n> +\n> \t\tstd::shared_ptr camera =\n> \t\t\tCamera::create(std::move(data), id, streams);\n> \n> @@ -1082,6 +1116,10 @@ bool PipelineHandlerISI::match(DeviceEnumerator *enumerator)\n> \t\tnumCameras++;\n> \t}\n> \n> +\tret = crossbar_->setRouting(&routing, V4L2Subdevice::ActiveFormat);\n> +\tif (ret)\n> +\t\treturn false;\n> +\n> \treturn numCameras > 0;\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 38DC4C3237\n\tfor ;\n\tSun, 29 Jun 2025 22:59:22 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CB8D368E05;\n\tMon, 30 Jun 2025 00:59:20 +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 C568468DF5\n\tfor ;\n\tMon, 30 Jun 2025 00:59:19 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with UTF8SMTPSA id 16E456A6;\n\tMon, 30 Jun 2025 00:58:57 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"L/65Xdie\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1751237938;\n\tbh=hbfJXMUEq0boVGQ6Gv9VXuOF0QQCJ9omiw2JR7/rQco=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=L/65Xdiek6xTh0rGz4/tYmXVvM5lNHtm7BCZx0Dy5ZBZ3osf+Nrn+mg8q/whfb1aY\n\tZFcRzMs3W5A6dh4sA3MQcJbG4oEG/GngcTMkWq+keRAjisaDCCxnfP3FEdmqwdvoG0\n\tHDXfZD1cUKP0koOVIB9RwrdMm6v7PiLhTlX67sq8=","Date":"Mon, 30 Jun 2025 01:58:55 +0300","From":"Laurent Pinchart ","To":"Antoine Bouyer ","Cc":"libcamera-devel@lists.libcamera.org, julien.vuillaumier@nxp.com","Subject":"Re: [PATCH v2 2/2] pipeline: imx8-isi: Add multicamera support","Message-ID":"<20250629225855.GB20333@pendragon.ideasonboard.com>","References":"<20250618124541.2340270-1-antoine.bouyer@nxp.com>\n\t<20250618124541.2340270-3-antoine.bouyer@nxp.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20250618124541.2340270-3-antoine.bouyer@nxp.com>","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]