[{"id":21025,"web_url":"https://patchwork.libcamera.org/comment/21025/","msgid":"","date":"2021-11-19T11:27:43","subject":"Re: [libcamera-devel] [PATCH 2/3] pipeline: raspberrypi: Allow\n\tregistration of multiple cameras","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi,\n\nOn Fri, 19 Nov 2021 at 11:10, Naushir Patuck wrote:\n\n> Expand the pipeline handler camera registration to correctly handle\n> multiple\n> cameras attached to the platform. For example, Raspberry Pi Compute Module\n> platforms have two camera connectors, and this change would allow the user\n> to\n> select either of the two cameras to run.\n>\n> There are associated kernel driver changes for both Unicam and the ISP\n> needed\n> to correctly advertise multiple media devices and nodes for multi-camera\n> usage:\n>\n> https://github.com/raspberrypi/linux/pull/4140\n> https://github.com/raspberrypi/linux/pull/4709\n>\n> However, this change is backward compatible with kernel builds that do not\n> have\n> these changes for standard single camera usage.\n>\n> Signed-off-by: Naushir Patuck \n> ---\n> .../pipeline/raspberrypi/raspberrypi.cpp | 112 ++++++++++++------\n> 1 file changed, 77 insertions(+), 35 deletions(-)\n>\n> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> index 9aa7e9eef5e7..8ed2ebcaafe7 100644\n> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> @@ -311,7 +311,8 @@ private:\n> return static_cast(camera->_d());\n> }\n>\n> - bool registerCameras();\n> + int registerCameras(MediaDevice *unicam, MediaDevice *isp,\n> + const std::string &unicamIdStr, const\n> std::string &ispIdStr);\n> int queueAllBuffers(Camera *camera);\n> int prepareBuffers(Camera *camera);\n> void freeBuffers(Camera *camera);\n> @@ -993,49 +994,87 @@ int PipelineHandlerRPi::queueRequestDevice(Camera\n> *camera, Request *request)\n>\n> bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)\n> {\n> - DeviceMatch unicam(\"unicam\");\n> - DeviceMatch isp(\"bcm2835-isp\");\n> + unsigned int numCameras = 0;\n>\n> - unicam.add(\"unicam-image\");\n> + /*\n> + * String of indexes to append to the entity names when searching\n> for\n> + * the Unican media devices. The first string is empty\n> (un-indexed) to\n> + * to maintain backward compatability with old versions of the\n> Unicam\n> + * kernel driver that did not advertise instance indexes.\n> + */\n> + for (const std::string &unicamIdStr : { \"\", \"0\", \"1\" }) {\n> + MediaDevice *unicamDevice;\n>\n> - isp.add(\"bcm2835-isp0-output0\"); /* Input */\n> - isp.add(\"bcm2835-isp0-capture1\"); /* Output 0 */\n> - isp.add(\"bcm2835-isp0-capture2\"); /* Output 1 */\n> - isp.add(\"bcm2835-isp0-capture3\"); /* Stats */\n> + DeviceMatch unicam(\"unicam\");\n> + unicam.add(\"unicam\" + unicamIdStr + \"-image\");\n> + unicamDevice = acquireMediaDevice(enumerator, unicam);\n>\n> - unicam_ = acquireMediaDevice(enumerator, unicam);\n> - if (!unicam_)\n> - return false;\n> + if (!unicamDevice)\n> + continue;\n>\n> - isp_ = acquireMediaDevice(enumerator, isp);\n> - if (!isp_)\n> - return false;\n> + for (const std::string &ispIdStr : { \"0\", \"1\" }) {\n> + MediaDevice *ispDevice;\n> + int ret;\n> +\n> + DeviceMatch isp(\"bcm2835-isp\");\n> + isp.add(\"bcm2835-isp\" + ispIdStr + \"-output0\"); /*\n> Input */\n> + isp.add(\"bcm2835-isp\" + ispIdStr + \"-capture1\");\n> /* Output 0 */\n> + isp.add(\"bcm2835-isp\" + ispIdStr + \"-capture2\");\n> /* Output 0 */\n> + isp.add(\"bcm2835-isp\" + ispIdStr + \"-capture3\");\n> /* Stats */\n> + ispDevice = acquireMediaDevice(enumerator, isp);\n> +\n> + if (!ispDevice)\n> + continue;\n> +\n> + ret = registerCameras(unicamDevice, ispDevice,\n> unicamIdStr, ispIdStr);\n> + if (ret) {\n> + LOG(RPI, Error) << \"Failed to register\n> camera: \" << ret;\n> + ispDevice->release();\n> + unicamDevice->release();\n>\n\nI am not too sure if the above two lines are\nenough. PipelineHandler::acquireMediaDevice()\ndoes a MediaDevice::acquire() but also adds the media device internally to\na private member:\n\nstd::vector> mediaDevices_;\n\nPerhaps I ought to implement a new public\nfunction PipelineHandler::acquireMediaDevice(MediaDevice *)\nthat removes the entry in mediaDevices_ as well as does a\nMediaDevice::release(). Thoughts?\n\nNaush\n\n\n+ } else\n> + numCameras++;\n>\n> - return registerCameras();\n> + break;\n> + }\n> + }\n> +\n> + return !!numCameras;\n> }\n>\n> -bool PipelineHandlerRPi::registerCameras()\n> +int PipelineHandlerRPi::registerCameras(MediaDevice *unicam, MediaDevice\n> *isp,\n> + const std::string &unicamIdStr,\n> + const std::string &ispIdStr)\n> {\n> std::unique_ptr data =\n> std::make_unique(this);\n> +\n> if (!data->dmaHeap_.isValid())\n> - return false;\n> + return -ENOMEM;\n> +\n> + MediaEntity *unicamImage = unicam->getEntityByName(\"unicam\" +\n> unicamIdStr + \"-image\");\n> + MediaEntity *ispOutput0 = isp->getEntityByName(\"bcm2835-isp\" +\n> ispIdStr + \"-output0\");\n> + MediaEntity *ispCapture1 = isp->getEntityByName(\"bcm2835-isp\" +\n> ispIdStr + \"-capture1\");\n> + MediaEntity *ispCapture2 = isp->getEntityByName(\"bcm2835-isp\" +\n> ispIdStr + \"-capture2\");\n> + MediaEntity *ispCapture3 = isp->getEntityByName(\"bcm2835-isp\" +\n> ispIdStr + \"-capture3\");\n> +\n> + if (!unicamImage || !ispOutput0 || !ispCapture1 || !ispCapture2 ||\n> !ispCapture3)\n> + return -ENOENT;\n>\n> /* Locate and open the unicam video streams. */\n> - data->unicam_[Unicam::Image] = RPi::Stream(\"Unicam Image\",\n> unicam_->getEntityByName(\"unicam-image\"));\n> + data->unicam_[Unicam::Image] = RPi::Stream(\"Unicam Image\",\n> unicamImage);\n>\n> /* An embedded data node will not be present if the sensor does\n> not support it. */\n> - MediaEntity *embeddedEntity =\n> unicam_->getEntityByName(\"unicam-embedded\");\n> - if (embeddedEntity) {\n> - data->unicam_[Unicam::Embedded] = RPi::Stream(\"Unicam\n> Embedded\", embeddedEntity);\n> + MediaEntity *unicamEmbedded = unicam->getEntityByName(\"unicam\" +\n> unicamIdStr + \"-embedded\");\n> + if (unicamEmbedded) {\n> + data->unicam_[Unicam::Embedded] = RPi::Stream(\"Unicam\n> Embedded\", unicamEmbedded);\n>\n> data->unicam_[Unicam::Embedded].dev()->bufferReady.connect(data.get(),\n>\n> &RPiCameraData::unicamBufferDequeue);\n> }\n>\n> /* Tag the ISP input stream as an import stream. */\n> - data->isp_[Isp::Input] = RPi::Stream(\"ISP Input\",\n> isp_->getEntityByName(\"bcm2835-isp0-output0\"), true);\n> - data->isp_[Isp::Output0] = RPi::Stream(\"ISP Output0\",\n> isp_->getEntityByName(\"bcm2835-isp0-capture1\"));\n> - data->isp_[Isp::Output1] = RPi::Stream(\"ISP Output1\",\n> isp_->getEntityByName(\"bcm2835-isp0-capture2\"));\n> - data->isp_[Isp::Stats] = RPi::Stream(\"ISP Stats\",\n> isp_->getEntityByName(\"bcm2835-isp0-capture3\"));\n> + data->isp_[Isp::Input] = RPi::Stream(\"ISP Input\", ispOutput0,\n> true);\n> + data->isp_[Isp::Output0] = RPi::Stream(\"ISP Output0\", ispCapture1);\n> + data->isp_[Isp::Output1] = RPi::Stream(\"ISP Output1\", ispCapture2);\n> + data->isp_[Isp::Stats] = RPi::Stream(\"ISP Stats\", ispCapture3);\n>\n> /* Wire up all the buffer connections. */\n> data->unicam_[Unicam::Image].dev()->frameStart.connect(data.get(),\n> &RPiCameraData::frameStarted);\n> @@ -1046,7 +1085,7 @@ bool PipelineHandlerRPi::registerCameras()\n> data->isp_[Isp::Stats].dev()->bufferReady.connect(data.get(),\n> &RPiCameraData::ispOutputDequeue);\n>\n> /* Identify the sensor. */\n> - for (MediaEntity *entity : unicam_->entities()) {\n> + for (MediaEntity *entity : unicam->entities()) {\n> if (entity->function() == MEDIA_ENT_F_CAM_SENSOR) {\n> data->sensor_ =\n> std::make_unique(entity);\n> break;\n> @@ -1054,23 +1093,23 @@ bool PipelineHandlerRPi::registerCameras()\n> }\n>\n> if (!data->sensor_)\n> - return false;\n> + return -EINVAL;\n>\n> if (data->sensor_->init())\n> - return false;\n> + return -EINVAL;\n>\n> data->sensorFormats_ = populateSensorFormats(data->sensor_);\n>\n> ipa::RPi::SensorConfig sensorConfig;\n> if (data->loadIPA(&sensorConfig)) {\n> LOG(RPI, Error) << \"Failed to load a suitable IPA library\";\n> - return false;\n> + return -EINVAL;\n> }\n>\n> - if (sensorConfig.sensorMetadata ^ !!embeddedEntity) {\n> + if (sensorConfig.sensorMetadata ^ !!unicamEmbedded) {\n> LOG(RPI, Warning) << \"Mismatch between Unicam and\n> CamHelper for embedded data usage!\";\n> sensorConfig.sensorMetadata = false;\n> - if (embeddedEntity)\n> + if (unicamEmbedded)\n>\n> data->unicam_[Unicam::Embedded].dev()->bufferReady.disconnect();\n> }\n>\n> @@ -1091,12 +1130,12 @@ bool PipelineHandlerRPi::registerCameras()\n>\n> for (auto stream : data->streams_) {\n> if (stream->dev()->open())\n> - return false;\n> + continue;\n> }\n>\n> if\n> (!data->unicam_[Unicam::Image].dev()->caps().hasMediaController()) {\n> LOG(RPI, Error) << \"Unicam driver does not use the\n> MediaController, please update your kernel!\";\n> - return false;\n> + return -EINVAL;\n> }\n>\n> /*\n> @@ -1158,7 +1197,7 @@ bool PipelineHandlerRPi::registerCameras()\n>\n> if (!bayerFormat.isValid()) {\n> LOG(RPI, Error) << \"No Bayer format found\";\n> - return false;\n> + return -EINVAL;\n> }\n> data->nativeBayerOrder_ = bayerFormat.order;\n>\n> @@ -1178,7 +1217,10 @@ bool PipelineHandlerRPi::registerCameras()\n> Camera::create(std::move(data), id, streams);\n> registerCamera(std::move(camera));\n>\n> - return true;\n> + LOG(RPI, Info) << \"Registered camera \" << id\n> + << \" to instances \\\"unicam\" << unicamIdStr << \"\\\"\"\n> + << \" and \\\"isp\" << ispIdStr << \"\\\"\";\n> + return 0;\n> }\n>\n> int PipelineHandlerRPi::queueAllBuffers(Camera *camera)\n> --\n> 2.25.1\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 CA1E3BDB13\n\tfor ;\n\tFri, 19 Nov 2021 11:28:02 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 055B06037A;\n\tFri, 19 Nov 2021 12:28:02 +0100 (CET)","from mail-lf1-x12a.google.com (mail-lf1-x12a.google.com\n\t[IPv6:2a00:1450:4864:20::12a])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 957D260233\n\tfor ;\n\tFri, 19 Nov 2021 12:28:00 +0100 (CET)","by mail-lf1-x12a.google.com with SMTP id bu18so42238520lfb.0\n\tfor ;\n\tFri, 19 Nov 2021 03:28:00 -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=\"mfHTZa8R\"; 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=eeuxn6ojY4X42OkTpPgrw+Mjea35rH6b9DVLYp5KwGw=;\n\tb=mfHTZa8RQaCp/2EBjuzkcozV89JKpb5DQW4uJN6tnj0FyEGz0uRQLHgsSbxll3Zs7N\n\tn6kgziToPpqnR0KDz+GGbJ7k9RB7NWz0p+tn8bWD8I6rV2LKH+eyp4GjtgyMScQ5pZuf\n\tUy8+/xeooDdgjpKNLP+Jhla/xHdwSRBD6YTWVEAxHJjEXvhZxbopM+ruK0tnx2hc2zJW\n\tjJXeglB5PR0pVJBEmVy5AajvrT0Vc3U9XZphcMIevUF3zW+tdknPwKINXlcftLGeHDwu\n\tJBHO9MIsM09f/KFfqhPTeXcNZm3Hv0BsEnoagrvzo9fxPnXlRWFOozyCoD+GKI2aCzjG\n\t5sCw==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=eeuxn6ojY4X42OkTpPgrw+Mjea35rH6b9DVLYp5KwGw=;\n\tb=IkbIXIkTzGNbX2nOAzaQLYm0rgA+dg1YZ0G7tmEK5hkqWpEx6m8gCiIci0Y7TZuQFy\n\tXbsRlSvlfAju/LRj9caBIiD0GVVozRlZApVSzpSCFeFKuy7nWPNPKtWI//AiVyPJmsQG\n\tG7vWag7XdcLUDMxzAXXIcgBnta9HX4jL1cj6rN6O7r6wlrg3jzDtGU7HCaNSHnnF38rj\n\tHt7Gccpi4ldNPlbNmbyIfUdjQcYK8Lq+FVlOQBaIzYY2D3oDLtlCy5I/bm4niqYh+6L2\n\tEWDasOV63qZ60oyflawa1dh+8It6Fg8CeOFxtQPg6xQrkdWxkAh0gdt95ZsZYt9LB9gq\n\tO+FA==","X-Gm-Message-State":"AOAM531RZAK5g/EgOkrMbn0nAr6Rllr4WXCMP3LGPbQWO2MqIreHhwLa\n\tAq9M323lsVv6jdld+O0v5hb/0+pqspdRe48tK2jQEkXyl6/ZasQn","X-Google-Smtp-Source":"ABdhPJzC2IIszChwI/5bqVVdNuaCB+NmFZcQ5z/aJqn8JdNEnR2IGqulCgQR/I8cMzESJQfvcRhxpXAIVFtXAzcCCFI=","X-Received":"by 2002:ac2:5932:: with SMTP id\n\tv18mr31039538lfi.611.1637321279265; \n\tFri, 19 Nov 2021 03:27:59 -0800 (PST)","MIME-Version":"1.0","References":"<20211119110955.3137585-1-naush@raspberrypi.com>\n\t<20211119110955.3137585-2-naush@raspberrypi.com>","In-Reply-To":"<20211119110955.3137585-2-naush@raspberrypi.com>","From":"Naushir Patuck ","Date":"Fri, 19 Nov 2021 11:27:43 +0000","Message-ID":"","To":"libcamera devel ","Content-Type":"multipart/alternative; boundary=\"0000000000003befe105d1229152\"","Subject":"Re: [libcamera-devel] [PATCH 2/3] pipeline: raspberrypi: Allow\n\tregistration of multiple cameras","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\" "}},{"id":21026,"web_url":"https://patchwork.libcamera.org/comment/21026/","msgid":"","date":"2021-11-19T11:28:39","subject":"Re: [libcamera-devel] [PATCH 2/3] pipeline: raspberrypi: Allow\n\tregistration of multiple cameras","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"On Fri, 19 Nov 2021 at 11:27, Naushir Patuck wrote:\n\n> Hi,\n>\n> On Fri, 19 Nov 2021 at 11:10, Naushir Patuck \n> wrote:\n>\n>> Expand the pipeline handler camera registration to correctly handle\n>> multiple\n>> cameras attached to the platform. For example, Raspberry Pi Compute Module\n>> platforms have two camera connectors, and this change would allow the\n>> user to\n>> select either of the two cameras to run.\n>>\n>> There are associated kernel driver changes for both Unicam and the ISP\n>> needed\n>> to correctly advertise multiple media devices and nodes for multi-camera\n>> usage:\n>>\n>> https://github.com/raspberrypi/linux/pull/4140\n>> https://github.com/raspberrypi/linux/pull/4709\n>>\n>> However, this change is backward compatible with kernel builds that do\n>> not have\n>> these changes for standard single camera usage.\n>>\n>> Signed-off-by: Naushir Patuck \n>> ---\n>> .../pipeline/raspberrypi/raspberrypi.cpp | 112 ++++++++++++------\n>> 1 file changed, 77 insertions(+), 35 deletions(-)\n>>\n>> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n>> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n>> index 9aa7e9eef5e7..8ed2ebcaafe7 100644\n>> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n>> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n>> @@ -311,7 +311,8 @@ private:\n>> return static_cast(camera->_d());\n>> }\n>>\n>> - bool registerCameras();\n>> + int registerCameras(MediaDevice *unicam, MediaDevice *isp,\n>> + const std::string &unicamIdStr, const\n>> std::string &ispIdStr);\n>> int queueAllBuffers(Camera *camera);\n>> int prepareBuffers(Camera *camera);\n>> void freeBuffers(Camera *camera);\n>> @@ -993,49 +994,87 @@ int PipelineHandlerRPi::queueRequestDevice(Camera\n>> *camera, Request *request)\n>>\n>> bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)\n>> {\n>> - DeviceMatch unicam(\"unicam\");\n>> - DeviceMatch isp(\"bcm2835-isp\");\n>> + unsigned int numCameras = 0;\n>>\n>> - unicam.add(\"unicam-image\");\n>> + /*\n>> + * String of indexes to append to the entity names when searching\n>> for\n>> + * the Unican media devices. The first string is empty\n>> (un-indexed) to\n>> + * to maintain backward compatability with old versions of the\n>> Unicam\n>> + * kernel driver that did not advertise instance indexes.\n>> + */\n>> + for (const std::string &unicamIdStr : { \"\", \"0\", \"1\" }) {\n>> + MediaDevice *unicamDevice;\n>>\n>> - isp.add(\"bcm2835-isp0-output0\"); /* Input */\n>> - isp.add(\"bcm2835-isp0-capture1\"); /* Output 0 */\n>> - isp.add(\"bcm2835-isp0-capture2\"); /* Output 1 */\n>> - isp.add(\"bcm2835-isp0-capture3\"); /* Stats */\n>> + DeviceMatch unicam(\"unicam\");\n>> + unicam.add(\"unicam\" + unicamIdStr + \"-image\");\n>> + unicamDevice = acquireMediaDevice(enumerator, unicam);\n>>\n>> - unicam_ = acquireMediaDevice(enumerator, unicam);\n>> - if (!unicam_)\n>> - return false;\n>> + if (!unicamDevice)\n>> + continue;\n>>\n>> - isp_ = acquireMediaDevice(enumerator, isp);\n>> - if (!isp_)\n>> - return false;\n>> + for (const std::string &ispIdStr : { \"0\", \"1\" }) {\n>> + MediaDevice *ispDevice;\n>> + int ret;\n>> +\n>> + DeviceMatch isp(\"bcm2835-isp\");\n>> + isp.add(\"bcm2835-isp\" + ispIdStr + \"-output0\");\n>> /* Input */\n>> + isp.add(\"bcm2835-isp\" + ispIdStr + \"-capture1\");\n>> /* Output 0 */\n>> + isp.add(\"bcm2835-isp\" + ispIdStr + \"-capture2\");\n>> /* Output 0 */\n>> + isp.add(\"bcm2835-isp\" + ispIdStr + \"-capture3\");\n>> /* Stats */\n>> + ispDevice = acquireMediaDevice(enumerator, isp);\n>> +\n>> + if (!ispDevice)\n>> + continue;\n>> +\n>> + ret = registerCameras(unicamDevice, ispDevice,\n>> unicamIdStr, ispIdStr);\n>> + if (ret) {\n>> + LOG(RPI, Error) << \"Failed to register\n>> camera: \" << ret;\n>> + ispDevice->release();\n>> + unicamDevice->release();\n>>\n>\n> I am not too sure if the above two lines are\n> enough. PipelineHandler::acquireMediaDevice()\n> does a MediaDevice::acquire() but also adds the media device internally to\n> a private member:\n>\n> std::vector> mediaDevices_;\n>\n> Perhaps I ought to implement a new public\n> function PipelineHandler::acquireMediaDevice(MediaDevice *)\n>\n\nSorry, that should read PipelineHandler::releaseMediaDevice(MediaDevice *)\n:-(\n\n\n> that removes the entry in mediaDevices_ as well as does a\n> MediaDevice::release(). Thoughts?\n>\n> Naush\n>\n>\n> + } else\n>> + numCameras++;\n>>\n>> - return registerCameras();\n>> + break;\n>> + }\n>> + }\n>> +\n>> + return !!numCameras;\n>> }\n>>\n>> -bool PipelineHandlerRPi::registerCameras()\n>> +int PipelineHandlerRPi::registerCameras(MediaDevice *unicam, MediaDevice\n>> *isp,\n>> + const std::string &unicamIdStr,\n>> + const std::string &ispIdStr)\n>> {\n>> std::unique_ptr data =\n>> std::make_unique(this);\n>> +\n>> if (!data->dmaHeap_.isValid())\n>> - return false;\n>> + return -ENOMEM;\n>> +\n>> + MediaEntity *unicamImage = unicam->getEntityByName(\"unicam\" +\n>> unicamIdStr + \"-image\");\n>> + MediaEntity *ispOutput0 = isp->getEntityByName(\"bcm2835-isp\" +\n>> ispIdStr + \"-output0\");\n>> + MediaEntity *ispCapture1 = isp->getEntityByName(\"bcm2835-isp\" +\n>> ispIdStr + \"-capture1\");\n>> + MediaEntity *ispCapture2 = isp->getEntityByName(\"bcm2835-isp\" +\n>> ispIdStr + \"-capture2\");\n>> + MediaEntity *ispCapture3 = isp->getEntityByName(\"bcm2835-isp\" +\n>> ispIdStr + \"-capture3\");\n>> +\n>> + if (!unicamImage || !ispOutput0 || !ispCapture1 || !ispCapture2\n>> || !ispCapture3)\n>> + return -ENOENT;\n>>\n>> /* Locate and open the unicam video streams. */\n>> - data->unicam_[Unicam::Image] = RPi::Stream(\"Unicam Image\",\n>> unicam_->getEntityByName(\"unicam-image\"));\n>> + data->unicam_[Unicam::Image] = RPi::Stream(\"Unicam Image\",\n>> unicamImage);\n>>\n>> /* An embedded data node will not be present if the sensor does\n>> not support it. */\n>> - MediaEntity *embeddedEntity =\n>> unicam_->getEntityByName(\"unicam-embedded\");\n>> - if (embeddedEntity) {\n>> - data->unicam_[Unicam::Embedded] = RPi::Stream(\"Unicam\n>> Embedded\", embeddedEntity);\n>> + MediaEntity *unicamEmbedded = unicam->getEntityByName(\"unicam\" +\n>> unicamIdStr + \"-embedded\");\n>> + if (unicamEmbedded) {\n>> + data->unicam_[Unicam::Embedded] = RPi::Stream(\"Unicam\n>> Embedded\", unicamEmbedded);\n>>\n>> data->unicam_[Unicam::Embedded].dev()->bufferReady.connect(data.get(),\n>>\n>> &RPiCameraData::unicamBufferDequeue);\n>> }\n>>\n>> /* Tag the ISP input stream as an import stream. */\n>> - data->isp_[Isp::Input] = RPi::Stream(\"ISP Input\",\n>> isp_->getEntityByName(\"bcm2835-isp0-output0\"), true);\n>> - data->isp_[Isp::Output0] = RPi::Stream(\"ISP Output0\",\n>> isp_->getEntityByName(\"bcm2835-isp0-capture1\"));\n>> - data->isp_[Isp::Output1] = RPi::Stream(\"ISP Output1\",\n>> isp_->getEntityByName(\"bcm2835-isp0-capture2\"));\n>> - data->isp_[Isp::Stats] = RPi::Stream(\"ISP Stats\",\n>> isp_->getEntityByName(\"bcm2835-isp0-capture3\"));\n>> + data->isp_[Isp::Input] = RPi::Stream(\"ISP Input\", ispOutput0,\n>> true);\n>> + data->isp_[Isp::Output0] = RPi::Stream(\"ISP Output0\",\n>> ispCapture1);\n>> + data->isp_[Isp::Output1] = RPi::Stream(\"ISP Output1\",\n>> ispCapture2);\n>> + data->isp_[Isp::Stats] = RPi::Stream(\"ISP Stats\", ispCapture3);\n>>\n>> /* Wire up all the buffer connections. */\n>>\n>> data->unicam_[Unicam::Image].dev()->frameStart.connect(data.get(),\n>> &RPiCameraData::frameStarted);\n>> @@ -1046,7 +1085,7 @@ bool PipelineHandlerRPi::registerCameras()\n>> data->isp_[Isp::Stats].dev()->bufferReady.connect(data.get(),\n>> &RPiCameraData::ispOutputDequeue);\n>>\n>> /* Identify the sensor. */\n>> - for (MediaEntity *entity : unicam_->entities()) {\n>> + for (MediaEntity *entity : unicam->entities()) {\n>> if (entity->function() == MEDIA_ENT_F_CAM_SENSOR) {\n>> data->sensor_ =\n>> std::make_unique(entity);\n>> break;\n>> @@ -1054,23 +1093,23 @@ bool PipelineHandlerRPi::registerCameras()\n>> }\n>>\n>> if (!data->sensor_)\n>> - return false;\n>> + return -EINVAL;\n>>\n>> if (data->sensor_->init())\n>> - return false;\n>> + return -EINVAL;\n>>\n>> data->sensorFormats_ = populateSensorFormats(data->sensor_);\n>>\n>> ipa::RPi::SensorConfig sensorConfig;\n>> if (data->loadIPA(&sensorConfig)) {\n>> LOG(RPI, Error) << \"Failed to load a suitable IPA\n>> library\";\n>> - return false;\n>> + return -EINVAL;\n>> }\n>>\n>> - if (sensorConfig.sensorMetadata ^ !!embeddedEntity) {\n>> + if (sensorConfig.sensorMetadata ^ !!unicamEmbedded) {\n>> LOG(RPI, Warning) << \"Mismatch between Unicam and\n>> CamHelper for embedded data usage!\";\n>> sensorConfig.sensorMetadata = false;\n>> - if (embeddedEntity)\n>> + if (unicamEmbedded)\n>>\n>> data->unicam_[Unicam::Embedded].dev()->bufferReady.disconnect();\n>> }\n>>\n>> @@ -1091,12 +1130,12 @@ bool PipelineHandlerRPi::registerCameras()\n>>\n>> for (auto stream : data->streams_) {\n>> if (stream->dev()->open())\n>> - return false;\n>> + continue;\n>> }\n>>\n>> if\n>> (!data->unicam_[Unicam::Image].dev()->caps().hasMediaController()) {\n>> LOG(RPI, Error) << \"Unicam driver does not use the\n>> MediaController, please update your kernel!\";\n>> - return false;\n>> + return -EINVAL;\n>> }\n>>\n>> /*\n>> @@ -1158,7 +1197,7 @@ bool PipelineHandlerRPi::registerCameras()\n>>\n>> if (!bayerFormat.isValid()) {\n>> LOG(RPI, Error) << \"No Bayer format found\";\n>> - return false;\n>> + return -EINVAL;\n>> }\n>> data->nativeBayerOrder_ = bayerFormat.order;\n>>\n>> @@ -1178,7 +1217,10 @@ bool PipelineHandlerRPi::registerCameras()\n>> Camera::create(std::move(data), id, streams);\n>> registerCamera(std::move(camera));\n>>\n>> - return true;\n>> + LOG(RPI, Info) << \"Registered camera \" << id\n>> + << \" to instances \\\"unicam\" << unicamIdStr << \"\\\"\"\n>> + << \" and \\\"isp\" << ispIdStr << \"\\\"\";\n>> + return 0;\n>> }\n>>\n>> int PipelineHandlerRPi::queueAllBuffers(Camera *camera)\n>> --\n>> 2.25.1\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 A761ABF415\n\tfor ;\n\tFri, 19 Nov 2021 11:28:57 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 612B26037A;\n\tFri, 19 Nov 2021 12:28:57 +0100 (CET)","from mail-lf1-x12c.google.com (mail-lf1-x12c.google.com\n\t[IPv6:2a00:1450:4864:20::12c])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 85BB660233\n\tfor ;\n\tFri, 19 Nov 2021 12:28:56 +0100 (CET)","by mail-lf1-x12c.google.com with SMTP id b40so42080195lfv.10\n\tfor ;\n\tFri, 19 Nov 2021 03:28:56 -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=\"F2itEjt3\"; 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=TEx+M9ef8L3tTRijtg67lfe1z8BrTlpg6e8vHN/OvZc=;\n\tb=F2itEjt38jQv5SokMagwpdnQokrY5MxY8ekTA8VNrASXjeLaIEZeuDPmFnwLWddCn+\n\tHPB+e6BzEVzFTL/v/2qKt3B86DXIbE/Wn9+Hmhhq6a+/DuA8ONxHfN3kgFn8j0WuDaPb\n\t7UmbtXlStwajZVudhcdaj85WqOb/fzUVoSQla9f9rtvzrF97/voBygaXVZ8blMxsFhlB\n\tAK/UOtSL6vj8GZkTNHK5d13jJXRjBiR2onKaF5fijkxNmIWc0+C2RiOeCq8ZNmSB+hPT\n\tRBvgTkcIrWnv8UB2yGgsM/vUiHMG0mr+qDUwmNWa88EY0O//LSOrgSPGRA+bU8BC4fN7\n\tAVyA==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=TEx+M9ef8L3tTRijtg67lfe1z8BrTlpg6e8vHN/OvZc=;\n\tb=aK0IImrpcZspaYFVB/8ZEK/S4uLqIMknrntgxrLogRhsBDE9GmtYSBj8ctPPY3VWut\n\tnvAN0vAATI2HpnhNnlnMf18BKhB09rKAAMjQUOED4UYUhALYz9YQVogt3KT7MHBQEXJG\n\te4vj6T8aWXDQz6tQlP8MW+zTMCRMMOChgD79kKiSxspjY1QBoMf78K9Ri6A7lRk6CRpj\n\tvVi5XTriCGefz0lZzavTiowWBOCvZVyRSVziCUIG1Ca4T6LuuYDGYNKTSvWRoUtiKgO2\n\tzJCaXa76/Wqfir+IobCOWroIe6r7ZU1QRpjiWjkOqZaij5ZlJ52J6ngUPzccHmQmLPgd\n\tSKKg==","X-Gm-Message-State":"AOAM530682bPZ9btfaR4eWt/AAZ1dcWpKsLQRrkzrqIrgXXyQRQSJNyE\n\tUtClqLcDj9ogID8hLkRCwXo0w7ZKyRuDeZZD9JO3tTdeSEutj8uS","X-Google-Smtp-Source":"ABdhPJyfLvXQtySHbL5zFutKvn/h7eCSfB5dR2fRW5JbBHvUECuEyzKOvyPViTB3ivU0JNjQ/DqVMVs6OSiq1W/dslY=","X-Received":"by 2002:a05:6512:10d6:: with SMTP id\n\tk22mr31383523lfg.108.1637321335741; \n\tFri, 19 Nov 2021 03:28:55 -0800 (PST)","MIME-Version":"1.0","References":"<20211119110955.3137585-1-naush@raspberrypi.com>\n\t<20211119110955.3137585-2-naush@raspberrypi.com>\n\t","In-Reply-To":"","From":"Naushir Patuck ","Date":"Fri, 19 Nov 2021 11:28:39 +0000","Message-ID":"","To":"libcamera devel ","Content-Type":"multipart/alternative; boundary=\"00000000000099b6fd05d1229484\"","Subject":"Re: [libcamera-devel] [PATCH 2/3] pipeline: raspberrypi: Allow\n\tregistration of multiple cameras","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\" "}},{"id":21040,"web_url":"https://patchwork.libcamera.org/comment/21040/","msgid":"","date":"2021-11-19T13:03:11","subject":"Re: [libcamera-devel] [PATCH 2/3] pipeline: raspberrypi: Allow\n\tregistration of multiple cameras","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Naush,\n\nThank you for the patch.\n\nOn Fri, Nov 19, 2021 at 11:28:39AM +0000, Naushir Patuck wrote:\n> On Fri, 19 Nov 2021 at 11:27, Naushir Patuck wrote:\n> > On Fri, 19 Nov 2021 at 11:10, Naushir Patuck wrote:\n> >\n> >> Expand the pipeline handler camera registration to correctly handle multiple\n> >> cameras attached to the platform. For example, Raspberry Pi Compute Module\n> >> platforms have two camera connectors, and this change would allow the user to\n> >> select either of the two cameras to run.\n\nThat's interesting :-)\n\n> >> There are associated kernel driver changes for both Unicam and the ISP needed\n> >> to correctly advertise multiple media devices and nodes for multi-camera usage:\n> >>\n> >> https://github.com/raspberrypi/linux/pull/4140\n> >> https://github.com/raspberrypi/linux/pull/4709\n\nThere's still a single ISP instance. How does it work at the\nhardware/firmware level ? The kernel patch seems simple, how are ISP\noperations queued by users scheduled and arbitrated ? Can the two\ncameras be used concurrently, or are they mutually exclusive ?\n\n> >> However, this change is backward compatible with kernel builds that do not have\n> >> these changes for standard single camera usage.\n> >>\n> >> Signed-off-by: Naushir Patuck \n> >> ---\n> >> .../pipeline/raspberrypi/raspberrypi.cpp | 112 ++++++++++++------\n> >> 1 file changed, 77 insertions(+), 35 deletions(-)\n> >>\n> >> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> >> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> >> index 9aa7e9eef5e7..8ed2ebcaafe7 100644\n> >> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> >> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> >> @@ -311,7 +311,8 @@ private:\n> >> return static_cast(camera->_d());\n> >> }\n> >>\n> >> - bool registerCameras();\n> >> + int registerCameras(MediaDevice *unicam, MediaDevice *isp,\n> >> + const std::string &unicamIdStr, const std::string &ispIdStr);\n> >> int queueAllBuffers(Camera *camera);\n> >> int prepareBuffers(Camera *camera);\n> >> void freeBuffers(Camera *camera);\n> >> @@ -993,49 +994,87 @@ int PipelineHandlerRPi::queueRequestDevice(Camera *camera, Request *request)\n> >>\n> >> bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)\n> >> {\n> >> - DeviceMatch unicam(\"unicam\");\n> >> - DeviceMatch isp(\"bcm2835-isp\");\n> >> + unsigned int numCameras = 0;\n> >>\n> >> - unicam.add(\"unicam-image\");\n> >> + /*\n> >> + * String of indexes to append to the entity names when searching for\n> >> + * the Unican media devices. The first string is empty (un-indexed) to\n> >> + * to maintain backward compatability with old versions of the Unicam\n> >> + * kernel driver that did not advertise instance indexes.\n> >> + */\n> >> + for (const std::string &unicamIdStr : { \"\", \"0\", \"1\" }) {\n> >> + MediaDevice *unicamDevice;\n> >>\n> >> - isp.add(\"bcm2835-isp0-output0\"); /* Input */\n> >> - isp.add(\"bcm2835-isp0-capture1\"); /* Output 0 */\n> >> - isp.add(\"bcm2835-isp0-capture2\"); /* Output 1 */\n> >> - isp.add(\"bcm2835-isp0-capture3\"); /* Stats */\n> >> + DeviceMatch unicam(\"unicam\");\n> >> + unicam.add(\"unicam\" + unicamIdStr + \"-image\");\n> >> + unicamDevice = acquireMediaDevice(enumerator, unicam);\n> >>\n> >> - unicam_ = acquireMediaDevice(enumerator, unicam);\n> >> - if (!unicam_)\n> >> - return false;\n> >> + if (!unicamDevice)\n> >> + continue;\n> >>\n> >> - isp_ = acquireMediaDevice(enumerator, isp);\n> >> - if (!isp_)\n> >> - return false;\n> >> + for (const std::string &ispIdStr : { \"0\", \"1\" }) {\n\nI'm not sure to understand why you have nested loops, shouldn't you\nacquire the unicam and ISP devices separately ?\n\n> >> + MediaDevice *ispDevice;\n> >> + int ret;\n> >> +\n> >> + DeviceMatch isp(\"bcm2835-isp\");\n> >> + isp.add(\"bcm2835-isp\" + ispIdStr + \"-output0\"); /* Input */\n> >> + isp.add(\"bcm2835-isp\" + ispIdStr + \"-capture1\"); /* Output 0 */\n> >> + isp.add(\"bcm2835-isp\" + ispIdStr + \"-capture2\"); /* Output 0 */\n> >> + isp.add(\"bcm2835-isp\" + ispIdStr + \"-capture3\"); /* Stats */\n> >> + ispDevice = acquireMediaDevice(enumerator, isp);\n> >> +\n> >> + if (!ispDevice)\n> >> + continue;\n> >> +\n> >> + ret = registerCameras(unicamDevice, ispDevice, unicamIdStr, ispIdStr);\n> >> + if (ret) {\n> >> + LOG(RPI, Error) << \"Failed to register camera: \" << ret;\n> >> + ispDevice->release();\n> >> + unicamDevice->release();\n> >\n> > I am not too sure if the above two lines are\n> > enough. PipelineHandler::acquireMediaDevice()\n> > does a MediaDevice::acquire() but also adds the media device internally to\n> > a private member:\n> >\n> > std::vector> mediaDevices_;\n> >\n> > Perhaps I ought to implement a new public\n> > function PipelineHandler::acquireMediaDevice(MediaDevice *)\n> \n> Sorry, that should read PipelineHandler::releaseMediaDevice(MediaDevice *)\n> :-(\n> \n> > that removes the entry in mediaDevices_ as well as does a\n> > MediaDevice::release(). Thoughts?\n\nThe media device acquired by the pipeline handler are meant to be\nreleased automatically. Camera registration failure isn't supposed to\nhappen, in case one camera fails to register and the other doesn't, is\nthere any issue keeping the media devices for the failed camera acquired\n?\n\n> >> + } else\n> >> + numCameras++;\n> >>\n> >> - return registerCameras();\n> >> + break;\n> >> + }\n> >> + }\n> >> +\n> >> + return !!numCameras;\n> >> }\n> >>\n> >> -bool PipelineHandlerRPi::registerCameras()\n> >> +int PipelineHandlerRPi::registerCameras(MediaDevice *unicam, MediaDevice *isp,\n> >> + const std::string &unicamIdStr,\n> >> + const std::string &ispIdStr)\n> >> {\n> >> std::unique_ptr data = std::make_unique(this);\n> >> +\n> >> if (!data->dmaHeap_.isValid())\n> >> - return false;\n> >> + return -ENOMEM;\n> >> +\n> >> + MediaEntity *unicamImage = unicam->getEntityByName(\"unicam\" + unicamIdStr + \"-image\");\n> >> + MediaEntity *ispOutput0 = isp->getEntityByName(\"bcm2835-isp\" + ispIdStr + \"-output0\");\n> >> + MediaEntity *ispCapture1 = isp->getEntityByName(\"bcm2835-isp\" + ispIdStr + \"-capture1\");\n> >> + MediaEntity *ispCapture2 = isp->getEntityByName(\"bcm2835-isp\" + ispIdStr + \"-capture2\");\n> >> + MediaEntity *ispCapture3 = isp->getEntityByName(\"bcm2835-isp\" + ispIdStr + \"-capture3\");\n> >> +\n> >> + if (!unicamImage || !ispOutput0 || !ispCapture1 || !ispCapture2 || !ispCapture3)\n> >> + return -ENOENT;\n> >>\n> >> /* Locate and open the unicam video streams. */\n> >> - data->unicam_[Unicam::Image] = RPi::Stream(\"Unicam Image\", unicam_->getEntityByName(\"unicam-image\"));\n> >> + data->unicam_[Unicam::Image] = RPi::Stream(\"Unicam Image\", unicamImage);\n> >>\n> >> /* An embedded data node will not be present if the sensor does not support it. */\n> >> - MediaEntity *embeddedEntity = unicam_->getEntityByName(\"unicam-embedded\");\n> >> - if (embeddedEntity) {\n> >> - data->unicam_[Unicam::Embedded] = RPi::Stream(\"Unicam Embedded\", embeddedEntity);\n> >> + MediaEntity *unicamEmbedded = unicam->getEntityByName(\"unicam\" + unicamIdStr + \"-embedded\");\n> >> + if (unicamEmbedded) {\n> >> + data->unicam_[Unicam::Embedded] = RPi::Stream(\"Unicam Embedded\", unicamEmbedded);\n> >>\n> >> data->unicam_[Unicam::Embedded].dev()->bufferReady.connect(data.get(),\n> >> &RPiCameraData::unicamBufferDequeue);\n> >> }\n> >>\n> >> /* Tag the ISP input stream as an import stream. */\n> >> - data->isp_[Isp::Input] = RPi::Stream(\"ISP Input\", isp_->getEntityByName(\"bcm2835-isp0-output0\"), true);\n> >> - data->isp_[Isp::Output0] = RPi::Stream(\"ISP Output0\", isp_->getEntityByName(\"bcm2835-isp0-capture1\"));\n> >> - data->isp_[Isp::Output1] = RPi::Stream(\"ISP Output1\", isp_->getEntityByName(\"bcm2835-isp0-capture2\"));\n> >> - data->isp_[Isp::Stats] = RPi::Stream(\"ISP Stats\", isp_->getEntityByName(\"bcm2835-isp0-capture3\"));\n> >> + data->isp_[Isp::Input] = RPi::Stream(\"ISP Input\", ispOutput0, true);\n> >> + data->isp_[Isp::Output0] = RPi::Stream(\"ISP Output0\", ispCapture1);\n> >> + data->isp_[Isp::Output1] = RPi::Stream(\"ISP Output1\", ispCapture2);\n> >> + data->isp_[Isp::Stats] = RPi::Stream(\"ISP Stats\", ispCapture3);\n> >>\n> >> /* Wire up all the buffer connections. */\n> >> data->unicam_[Unicam::Image].dev()->frameStart.connect(data.get(), &RPiCameraData::frameStarted);\n> >> @@ -1046,7 +1085,7 @@ bool PipelineHandlerRPi::registerCameras()\n> >> data->isp_[Isp::Stats].dev()->bufferReady.connect(data.get(), &RPiCameraData::ispOutputDequeue);\n> >>\n> >> /* Identify the sensor. */\n> >> - for (MediaEntity *entity : unicam_->entities()) {\n> >> + for (MediaEntity *entity : unicam->entities()) {\n> >> if (entity->function() == MEDIA_ENT_F_CAM_SENSOR) {\n> >> data->sensor_ = std::make_unique(entity);\n> >> break;\n> >> @@ -1054,23 +1093,23 @@ bool PipelineHandlerRPi::registerCameras()\n> >> }\n> >>\n> >> if (!data->sensor_)\n> >> - return false;\n> >> + return -EINVAL;\n> >>\n> >> if (data->sensor_->init())\n> >> - return false;\n> >> + return -EINVAL;\n> >>\n> >> data->sensorFormats_ = populateSensorFormats(data->sensor_);\n> >>\n> >> ipa::RPi::SensorConfig sensorConfig;\n> >> if (data->loadIPA(&sensorConfig)) {\n> >> LOG(RPI, Error) << \"Failed to load a suitable IPA library\";\n> >> - return false;\n> >> + return -EINVAL;\n> >> }\n> >>\n> >> - if (sensorConfig.sensorMetadata ^ !!embeddedEntity) {\n> >> + if (sensorConfig.sensorMetadata ^ !!unicamEmbedded) {\n> >> LOG(RPI, Warning) << \"Mismatch between Unicam and CamHelper for embedded data usage!\";\n> >> sensorConfig.sensorMetadata = false;\n> >> - if (embeddedEntity)\n> >> + if (unicamEmbedded)\n> >> data->unicam_[Unicam::Embedded].dev()->bufferReady.disconnect();\n> >> }\n> >>\n> >> @@ -1091,12 +1130,12 @@ bool PipelineHandlerRPi::registerCameras()\n> >>\n> >> for (auto stream : data->streams_) {\n> >> if (stream->dev()->open())\n> >> - return false;\n> >> + continue;\n> >> }\n> >>\n> >> if (!data->unicam_[Unicam::Image].dev()->caps().hasMediaController()) {\n> >> LOG(RPI, Error) << \"Unicam driver does not use the MediaController, please update your kernel!\";\n> >> - return false;\n> >> + return -EINVAL;\n> >> }\n> >>\n> >> /*\n> >> @@ -1158,7 +1197,7 @@ bool PipelineHandlerRPi::registerCameras()\n> >>\n> >> if (!bayerFormat.isValid()) {\n> >> LOG(RPI, Error) << \"No Bayer format found\";\n> >> - return false;\n> >> + return -EINVAL;\n> >> }\n> >> data->nativeBayerOrder_ = bayerFormat.order;\n> >>\n> >> @@ -1178,7 +1217,10 @@ bool PipelineHandlerRPi::registerCameras()\n> >> Camera::create(std::move(data), id, streams);\n> >> registerCamera(std::move(camera));\n> >>\n> >> - return true;\n> >> + LOG(RPI, Info) << \"Registered camera \" << id\n> >> + << \" to instances \\\"unicam\" << unicamIdStr << \"\\\"\"\n> >> + << \" and \\\"isp\" << ispIdStr << \"\\\"\";\n> >> + return 0;\n> >> }\n> >>\n> >> int PipelineHandlerRPi::queueAllBuffers(Camera *camera)","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 C1C66BF415\n\tfor ;\n\tFri, 19 Nov 2021 13:03:36 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1EDFA6033C;\n\tFri, 19 Nov 2021 14:03:36 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 388AE600B5\n\tfor ;\n\tFri, 19 Nov 2021 14:03:34 +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 AF5D41959;\n\tFri, 19 Nov 2021 14:03:33 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"aZCPtizI\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1637327013;\n\tbh=LwWfAQ0VsKkk+OfS7hD8n1PIioXfhoDCROt69b8giCY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=aZCPtizIN7qJXKfzve2MnrQpNqGddCWjneav1o8hMz6CjVaaFKIwVEGKwWHw1Tk2/\n\tQYg3mm/zB9TEAc0CBacDKwB8bhV0AbPAr8w2Y6+wMPvoqbpqxc9B/rEe3aOKYCaB5s\n\tdyT+a7jE2/owaSCjOcwHNIL83lVndKupIOD24pac=","Date":"Fri, 19 Nov 2021 15:03:11 +0200","From":"Laurent Pinchart ","To":"Naushir Patuck ","Message-ID":"","References":"<20211119110955.3137585-1-naush@raspberrypi.com>\n\t<20211119110955.3137585-2-naush@raspberrypi.com>\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 2/3] pipeline: raspberrypi: Allow\n\tregistration of multiple cameras","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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":21062,"web_url":"https://patchwork.libcamera.org/comment/21062/","msgid":"","date":"2021-11-19T16:45:27","subject":"Re: [libcamera-devel] [PATCH 2/3] pipeline: raspberrypi: Allow\n\tregistration of multiple cameras","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 your review for this series.\n\nOn Fri, 19 Nov 2021 at 13:03, Laurent Pinchart <\nlaurent.pinchart@ideasonboard.com> wrote:\n\n> Hi Naush,\n>\n> Thank you for the patch.\n>\n> On Fri, Nov 19, 2021 at 11:28:39AM +0000, Naushir Patuck wrote:\n> > On Fri, 19 Nov 2021 at 11:27, Naushir Patuck wrote:\n> > > On Fri, 19 Nov 2021 at 11:10, Naushir Patuck wrote:\n> > >\n> > >> Expand the pipeline handler camera registration to correctly handle\n> multiple\n> > >> cameras attached to the platform. For example, Raspberry Pi Compute\n> Module\n> > >> platforms have two camera connectors, and this change would allow the\n> user to\n> > >> select either of the two cameras to run.\n>\n> That's interesting :-)\n>\n> > >> There are associated kernel driver changes for both Unicam and the\n> ISP needed\n> > >> to correctly advertise multiple media devices and nodes for\n> multi-camera usage:\n> > >>\n> > >> https://github.com/raspberrypi/linux/pull/4140\n> > >> https://github.com/raspberrypi/linux/pull/4709\n>\n> There's still a single ISP instance. How does it work at the\n> hardware/firmware level ? The kernel patch seems simple, how are ISP\n> operations queued by users scheduled and arbitrated ? Can the two\n> cameras be used concurrently, or are they mutually exclusive ?\n>\n\nThe isp driver change will allow concurrent usage of the ISP hardware.\nWhat's missing from your view is the shim in the firmware that handles\nthe arbitration between the many users ;-) This makes adding multi-user\nto the kernel driver mostly a duplication of the single-user instance.\n\nAs an aside, I have run into a few issues with actual concurrent usage\nin the pipeline handlers. I've been talking with Jacopo offline about\nthis, but we can keep that discussion as a separate thread to this work.\n\n\n>\n> > >> However, this change is backward compatible with kernel builds that\n> do not have\n> > >> these changes for standard single camera usage.\n> > >>\n> > >> Signed-off-by: Naushir Patuck \n> > >> ---\n> > >> .../pipeline/raspberrypi/raspberrypi.cpp | 112\n> ++++++++++++------\n> > >> 1 file changed, 77 insertions(+), 35 deletions(-)\n> > >>\n> > >> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > >> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > >> index 9aa7e9eef5e7..8ed2ebcaafe7 100644\n> > >> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > >> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > >> @@ -311,7 +311,8 @@ private:\n> > >> return static_cast(camera->_d());\n> > >> }\n> > >>\n> > >> - bool registerCameras();\n> > >> + int registerCameras(MediaDevice *unicam, MediaDevice *isp,\n> > >> + const std::string &unicamIdStr, const\n> std::string &ispIdStr);\n> > >> int queueAllBuffers(Camera *camera);\n> > >> int prepareBuffers(Camera *camera);\n> > >> void freeBuffers(Camera *camera);\n> > >> @@ -993,49 +994,87 @@ int\n> PipelineHandlerRPi::queueRequestDevice(Camera *camera, Request *request)\n> > >>\n> > >> bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)\n> > >> {\n> > >> - DeviceMatch unicam(\"unicam\");\n> > >> - DeviceMatch isp(\"bcm2835-isp\");\n> > >> + unsigned int numCameras = 0;\n> > >>\n> > >> - unicam.add(\"unicam-image\");\n> > >> + /*\n> > >> + * String of indexes to append to the entity names when\n> searching for\n> > >> + * the Unican media devices. The first string is empty\n> (un-indexed) to\n> > >> + * to maintain backward compatability with old versions of\n> the Unicam\n> > >> + * kernel driver that did not advertise instance indexes.\n> > >> + */\n> > >> + for (const std::string &unicamIdStr : { \"\", \"0\", \"1\" }) {\n> > >> + MediaDevice *unicamDevice;\n> > >>\n> > >> - isp.add(\"bcm2835-isp0-output0\"); /* Input */\n> > >> - isp.add(\"bcm2835-isp0-capture1\"); /* Output 0 */\n> > >> - isp.add(\"bcm2835-isp0-capture2\"); /* Output 1 */\n> > >> - isp.add(\"bcm2835-isp0-capture3\"); /* Stats */\n> > >> + DeviceMatch unicam(\"unicam\");\n> > >> + unicam.add(\"unicam\" + unicamIdStr + \"-image\");\n> > >> + unicamDevice = acquireMediaDevice(enumerator, unicam);\n> > >>\n> > >> - unicam_ = acquireMediaDevice(enumerator, unicam);\n> > >> - if (!unicam_)\n> > >> - return false;\n> > >> + if (!unicamDevice)\n> > >> + continue;\n> > >>\n> > >> - isp_ = acquireMediaDevice(enumerator, isp);\n> > >> - if (!isp_)\n> > >> - return false;\n> > >> + for (const std::string &ispIdStr : { \"0\", \"1\" }) {\n>\n> I'm not sure to understand why you have nested loops, shouldn't you\n> acquire the unicam and ISP devices separately ?\n>\n\nEach unicam instance must be matched with an isp instance, else\nthe camera cannot be registered.\n\nI used nested loops for convenience, it's not strictly necessary.\nWith the nested loops, I don't need to keep arrays of MediaDevice\npointers sticking around :-)\n\n\n>\n> > >> + MediaDevice *ispDevice;\n> > >> + int ret;\n> > >> +\n> > >> + DeviceMatch isp(\"bcm2835-isp\");\n> > >> + isp.add(\"bcm2835-isp\" + ispIdStr +\n> \"-output0\"); /* Input */\n> > >> + isp.add(\"bcm2835-isp\" + ispIdStr +\n> \"-capture1\"); /* Output 0 */\n> > >> + isp.add(\"bcm2835-isp\" + ispIdStr +\n> \"-capture2\"); /* Output 0 */\n> > >> + isp.add(\"bcm2835-isp\" + ispIdStr +\n> \"-capture3\"); /* Stats */\n> > >> + ispDevice = acquireMediaDevice(enumerator,\n> isp);\n> > >> +\n> > >> + if (!ispDevice)\n> > >> + continue;\n> > >> +\n> > >> + ret = registerCameras(unicamDevice,\n> ispDevice, unicamIdStr, ispIdStr);\n> > >> + if (ret) {\n> > >> + LOG(RPI, Error) << \"Failed to\n> register camera: \" << ret;\n> > >> + ispDevice->release();\n> > >> + unicamDevice->release();\n> > >\n> > > I am not too sure if the above two lines are\n> > > enough. PipelineHandler::acquireMediaDevice()\n> > > does a MediaDevice::acquire() but also adds the media device\n> internally to\n> > > a private member:\n> > >\n> > > std::vector> mediaDevices_;\n> > >\n> > > Perhaps I ought to implement a new public\n> > > function PipelineHandler::acquireMediaDevice(MediaDevice *)\n> >\n> > Sorry, that should read PipelineHandler::releaseMediaDevice(MediaDevice\n> *)\n> > :-(\n> >\n> > > that removes the entry in mediaDevices_ as well as does a\n> > > MediaDevice::release(). Thoughts?\n>\n> The media device acquired by the pipeline handler are meant to be\n> released automatically. Camera registration failure isn't supposed to\n> happen, in case one camera fails to register and the other doesn't, is\n> there any issue keeping the media devices for the failed camera acquired\n> ?\n>\n\nI suppose there is no harm in keeping the device acquired when the camera\ncannot\nbe registered, particularly as it cannot be used again. In which case I\ncan simplify the\nerror path logic.\n\nRegards,\nNaush\n\n\n>\n> > >> + } else\n> > >> + numCameras++;\n> > >>\n> > >> - return registerCameras();\n> > >> + break;\n> > >> + }\n> > >> + }\n> > >> +\n> > >> + return !!numCameras;\n> > >> }\n> > >>\n> > >> -bool PipelineHandlerRPi::registerCameras()\n> > >> +int PipelineHandlerRPi::registerCameras(MediaDevice *unicam,\n> MediaDevice *isp,\n> > >> + const std::string\n> &unicamIdStr,\n> > >> + const std::string &ispIdStr)\n> > >> {\n> > >> std::unique_ptr data =\n> std::make_unique(this);\n> > >> +\n> > >> if (!data->dmaHeap_.isValid())\n> > >> - return false;\n> > >> + return -ENOMEM;\n> > >> +\n> > >> + MediaEntity *unicamImage = unicam->getEntityByName(\"unicam\" +\n> unicamIdStr + \"-image\");\n> > >> + MediaEntity *ispOutput0 = isp->getEntityByName(\"bcm2835-isp\"\n> + ispIdStr + \"-output0\");\n> > >> + MediaEntity *ispCapture1 = isp->getEntityByName(\"bcm2835-isp\"\n> + ispIdStr + \"-capture1\");\n> > >> + MediaEntity *ispCapture2 = isp->getEntityByName(\"bcm2835-isp\"\n> + ispIdStr + \"-capture2\");\n> > >> + MediaEntity *ispCapture3 = isp->getEntityByName(\"bcm2835-isp\"\n> + ispIdStr + \"-capture3\");\n> > >> +\n> > >> + if (!unicamImage || !ispOutput0 || !ispCapture1 ||\n> !ispCapture2 || !ispCapture3)\n> > >> + return -ENOENT;\n> > >>\n> > >> /* Locate and open the unicam video streams. */\n> > >> - data->unicam_[Unicam::Image] = RPi::Stream(\"Unicam Image\",\n> unicam_->getEntityByName(\"unicam-image\"));\n> > >> + data->unicam_[Unicam::Image] = RPi::Stream(\"Unicam Image\",\n> unicamImage);\n> > >>\n> > >> /* An embedded data node will not be present if the sensor\n> does not support it. */\n> > >> - MediaEntity *embeddedEntity =\n> unicam_->getEntityByName(\"unicam-embedded\");\n> > >> - if (embeddedEntity) {\n> > >> - data->unicam_[Unicam::Embedded] = RPi::Stream(\"Unicam\n> Embedded\", embeddedEntity);\n> > >> + MediaEntity *unicamEmbedded =\n> unicam->getEntityByName(\"unicam\" + unicamIdStr + \"-embedded\");\n> > >> + if (unicamEmbedded) {\n> > >> + data->unicam_[Unicam::Embedded] = RPi::Stream(\"Unicam\n> Embedded\", unicamEmbedded);\n> > >>\n> > >>\n> data->unicam_[Unicam::Embedded].dev()->bufferReady.connect(data.get(),\n> > >>\n> &RPiCameraData::unicamBufferDequeue);\n> > >> }\n> > >>\n> > >> /* Tag the ISP input stream as an import stream. */\n> > >> - data->isp_[Isp::Input] = RPi::Stream(\"ISP Input\",\n> isp_->getEntityByName(\"bcm2835-isp0-output0\"), true);\n> > >> - data->isp_[Isp::Output0] = RPi::Stream(\"ISP Output0\",\n> isp_->getEntityByName(\"bcm2835-isp0-capture1\"));\n> > >> - data->isp_[Isp::Output1] = RPi::Stream(\"ISP Output1\",\n> isp_->getEntityByName(\"bcm2835-isp0-capture2\"));\n> > >> - data->isp_[Isp::Stats] = RPi::Stream(\"ISP Stats\",\n> isp_->getEntityByName(\"bcm2835-isp0-capture3\"));\n> > >> + data->isp_[Isp::Input] = RPi::Stream(\"ISP Input\", ispOutput0,\n> true);\n> > >> + data->isp_[Isp::Output0] = RPi::Stream(\"ISP Output0\",\n> ispCapture1);\n> > >> + data->isp_[Isp::Output1] = RPi::Stream(\"ISP Output1\",\n> ispCapture2);\n> > >> + data->isp_[Isp::Stats] = RPi::Stream(\"ISP Stats\",\n> ispCapture3);\n> > >>\n> > >> /* Wire up all the buffer connections. */\n> > >>\n> data->unicam_[Unicam::Image].dev()->frameStart.connect(data.get(),\n> &RPiCameraData::frameStarted);\n> > >> @@ -1046,7 +1085,7 @@ bool PipelineHandlerRPi::registerCameras()\n> > >> data->isp_[Isp::Stats].dev()->bufferReady.connect(data.get(),\n> &RPiCameraData::ispOutputDequeue);\n> > >>\n> > >> /* Identify the sensor. */\n> > >> - for (MediaEntity *entity : unicam_->entities()) {\n> > >> + for (MediaEntity *entity : unicam->entities()) {\n> > >> if (entity->function() == MEDIA_ENT_F_CAM_SENSOR) {\n> > >> data->sensor_ =\n> std::make_unique(entity);\n> > >> break;\n> > >> @@ -1054,23 +1093,23 @@ bool PipelineHandlerRPi::registerCameras()\n> > >> }\n> > >>\n> > >> if (!data->sensor_)\n> > >> - return false;\n> > >> + return -EINVAL;\n> > >>\n> > >> if (data->sensor_->init())\n> > >> - return false;\n> > >> + return -EINVAL;\n> > >>\n> > >> data->sensorFormats_ = populateSensorFormats(data->sensor_);\n> > >>\n> > >> ipa::RPi::SensorConfig sensorConfig;\n> > >> if (data->loadIPA(&sensorConfig)) {\n> > >> LOG(RPI, Error) << \"Failed to load a suitable IPA\n> library\";\n> > >> - return false;\n> > >> + return -EINVAL;\n> > >> }\n> > >>\n> > >> - if (sensorConfig.sensorMetadata ^ !!embeddedEntity) {\n> > >> + if (sensorConfig.sensorMetadata ^ !!unicamEmbedded) {\n> > >> LOG(RPI, Warning) << \"Mismatch between Unicam and\n> CamHelper for embedded data usage!\";\n> > >> sensorConfig.sensorMetadata = false;\n> > >> - if (embeddedEntity)\n> > >> + if (unicamEmbedded)\n> > >>\n> data->unicam_[Unicam::Embedded].dev()->bufferReady.disconnect();\n> > >> }\n> > >>\n> > >> @@ -1091,12 +1130,12 @@ bool PipelineHandlerRPi::registerCameras()\n> > >>\n> > >> for (auto stream : data->streams_) {\n> > >> if (stream->dev()->open())\n> > >> - return false;\n> > >> + continue;\n> > >> }\n> > >>\n> > >> if\n> (!data->unicam_[Unicam::Image].dev()->caps().hasMediaController()) {\n> > >> LOG(RPI, Error) << \"Unicam driver does not use the\n> MediaController, please update your kernel!\";\n> > >> - return false;\n> > >> + return -EINVAL;\n> > >> }\n> > >>\n> > >> /*\n> > >> @@ -1158,7 +1197,7 @@ bool PipelineHandlerRPi::registerCameras()\n> > >>\n> > >> if (!bayerFormat.isValid()) {\n> > >> LOG(RPI, Error) << \"No Bayer format found\";\n> > >> - return false;\n> > >> + return -EINVAL;\n> > >> }\n> > >> data->nativeBayerOrder_ = bayerFormat.order;\n> > >>\n> > >> @@ -1178,7 +1217,10 @@ bool PipelineHandlerRPi::registerCameras()\n> > >> Camera::create(std::move(data), id, streams);\n> > >> registerCamera(std::move(camera));\n> > >>\n> > >> - return true;\n> > >> + LOG(RPI, Info) << \"Registered camera \" << id\n> > >> + << \" to instances \\\"unicam\" << unicamIdStr <<\n> \"\\\"\"\n> > >> + << \" and \\\"isp\" << ispIdStr << \"\\\"\";\n> > >> + return 0;\n> > >> }\n> > >>\n> > >> int PipelineHandlerRPi::queueAllBuffers(Camera *camera)\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 AE4FCBDB13\n\tfor ;\n\tFri, 19 Nov 2021 16:45:47 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DE0CB60378;\n\tFri, 19 Nov 2021 17:45:46 +0100 (CET)","from mail-lf1-x130.google.com (mail-lf1-x130.google.com\n\t[IPv6:2a00:1450:4864:20::130])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 19788600B5\n\tfor ;\n\tFri, 19 Nov 2021 17:45:45 +0100 (CET)","by mail-lf1-x130.google.com with SMTP id bi37so46014719lfb.5\n\tfor ;\n\tFri, 19 Nov 2021 08:45:45 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"QHApV1Ko\"; 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=Md3AMsQT2JSQg5GkzLYXr1roF4J00w8A6q01NAUpcUY=;\n\tb=QHApV1Ko7eskzJHGCCWqEh459e4gaGMgoGifP43uu8MYbGRL8aKkz1JGULqoHA0ScL\n\t/Ol1IQ+bSQ7sBENgOmAyR4gXw3AeMez+G7JMLh6rQ/FHJV1mxtQw9IMkuLnJ9EeqvMW9\n\tnCEYwcHCY696EHNkuXfGFpznTpCrZYlyt6yQA1wGaBXXM7zzq9hrNrFku7Iwim0nXCoH\n\tmrxEwmzNsQVg71MybQ8LNTxboJ8E6rMDkFNUwVnEnY9/Utu2rt5bCSRzGR4Y1KnBcO5h\n\tw1xasY3D1VgQJYeV1Kyav7PDSL4KaXTSNfhHutMQVNFp8iX5P5jv8KD2zT5eOHgg6Gmq\n\t3Y8g==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=Md3AMsQT2JSQg5GkzLYXr1roF4J00w8A6q01NAUpcUY=;\n\tb=glszJcBL/zk4ECE6r7ch+a0CRamsPrcS58+BknFRZ/MxK2BTMMbNvlEHwRqDYKsqpM\n\tS+CNjQL/Dn4mimeFIFZexVOnC1HtkKaoYlJ4/975ajr4s9/mkX7RpWRAtPBUN+iJS4eN\n\tECuYA1sh4Q1RMU90iWj7tOJlJXgoe1L08aF9NYSaEaEaIMfHcJA69xOjFpuFt4dPQcom\n\ty6GHCiRX+0swYlMRoQOTXWwWUtgRMf6TtLc+dBhgafQg0IjyhyCCd6+GkQHovyiYzpI1\n\tv8+g8osXY9PXTqSdP165o+a7FyWt0/yXFmu/h3XbAuxFDFz4IDRvxKlsw2QWTvn2FiZJ\n\tDbKg==","X-Gm-Message-State":"AOAM533oYSUCk8yroOFSNeX0cN/yvpWVH/4q2oCaQTng2M/cm5WYc+ix\n\tDKObUPxQyHdRUBmeyLBhF+xBaSdq2s8uPB7nE8YgcQ==","X-Google-Smtp-Source":"ABdhPJyBPFBJCHL+kkHS4PGymDCK/ICRN8k2V1UOOhJEt3rQ4BHXlspqZi9xYFv/DkA8TD1hnBaj5qhCW3Dk6DGDuIA=","X-Received":"by 2002:ac2:5932:: with SMTP id\n\tv18mr33300286lfi.611.1637340343636; \n\tFri, 19 Nov 2021 08:45:43 -0800 (PST)","MIME-Version":"1.0","References":"<20211119110955.3137585-1-naush@raspberrypi.com>\n\t<20211119110955.3137585-2-naush@raspberrypi.com>\n\t\n\t\n\t","In-Reply-To":"","From":"Naushir Patuck ","Date":"Fri, 19 Nov 2021 16:45:27 +0000","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"multipart/alternative; boundary=\"0000000000008f259e05d127010a\"","Subject":"Re: [libcamera-devel] [PATCH 2/3] pipeline: raspberrypi: Allow\n\tregistration of multiple cameras","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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":21065,"web_url":"https://patchwork.libcamera.org/comment/21065/","msgid":"","date":"2021-11-19T20:52:55","subject":"Re: [libcamera-devel] [PATCH 2/3] pipeline: raspberrypi: Allow\n\tregistration of multiple cameras","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, Nov 19, 2021 at 04:45:27PM +0000, Naushir Patuck wrote:\n> On Fri, 19 Nov 2021 at 13:03, Laurent Pinchart wrote:\n> > On Fri, Nov 19, 2021 at 11:28:39AM +0000, Naushir Patuck wrote:\n> > > On Fri, 19 Nov 2021 at 11:27, Naushir Patuck wrote:\n> > > > On Fri, 19 Nov 2021 at 11:10, Naushir Patuck wrote:\n> > > >\n> > > >> Expand the pipeline handler camera registration to correctly handle multiple\n> > > >> cameras attached to the platform. For example, Raspberry Pi Compute Module\n> > > >> platforms have two camera connectors, and this change would allow the user to\n> > > >> select either of the two cameras to run.\n> >\n> > That's interesting :-)\n> >\n> > > >> There are associated kernel driver changes for both Unicam and the ISP needed\n> > > >> to correctly advertise multiple media devices and nodes for multi-camera usage:\n> > > >>\n> > > >> https://github.com/raspberrypi/linux/pull/4140\n> > > >> https://github.com/raspberrypi/linux/pull/4709\n> >\n> > There's still a single ISP instance. How does it work at the\n> > hardware/firmware level ? The kernel patch seems simple, how are ISP\n> > operations queued by users scheduled and arbitrated ? Can the two\n> > cameras be used concurrently, or are they mutually exclusive ?\n> \n> The isp driver change will allow concurrent usage of the ISP hardware.\n> What's missing from your view is the shim in the firmware that handles\n> the arbitration between the many users ;-) This makes adding multi-user\n> to the kernel driver mostly a duplication of the single-user instance.\n\nAre there advantages in doing so in the firmware instead of in the Linux\nkernel driver, or even in userspace in libcamera ? I suppose that if\nit's there already it's the simplest way forward, I'm not requesting a\nchange here, but I'm wondering what the design rationale is.\n\nHow should libcamera cope with limitations in terms of ISP bandwidth\nwith multiple instances ?\n\n> As an aside, I have run into a few issues with actual concurrent usage\n> in the pipeline handlers. I've been talking with Jacopo offline about\n> this, but we can keep that discussion as a separate thread to this work.\n\nhttps://git.libcamera.org/libcamera/pinchartl/libcamera.git/log/?h=mtk/multi-cam\nmay help already. We'll also need an API for pipeline handlers to report\nmutual exclusion constraints between camera, and a way to expose that to\napplications.\n\n> > > >> However, this change is backward compatible with kernel builds that do not have\n> > > >> these changes for standard single camera usage.\n> > > >>\n> > > >> Signed-off-by: Naushir Patuck \n> > > >> ---\n> > > >> .../pipeline/raspberrypi/raspberrypi.cpp | 112 ++++++++++++------\n> > > >> 1 file changed, 77 insertions(+), 35 deletions(-)\n> > > >>\n> > > >> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > >> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > >> index 9aa7e9eef5e7..8ed2ebcaafe7 100644\n> > > >> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > >> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > >> @@ -311,7 +311,8 @@ private:\n> > > >> return static_cast(camera->_d());\n> > > >> }\n> > > >>\n> > > >> - bool registerCameras();\n> > > >> + int registerCameras(MediaDevice *unicam, MediaDevice *isp,\n> > > >> + const std::string &unicamIdStr, const std::string &ispIdStr);\n> > > >> int queueAllBuffers(Camera *camera);\n> > > >> int prepareBuffers(Camera *camera);\n> > > >> void freeBuffers(Camera *camera);\n> > > >> @@ -993,49 +994,87 @@ int PipelineHandlerRPi::queueRequestDevice(Camera *camera, Request *request)\n> > > >>\n> > > >> bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)\n> > > >> {\n> > > >> - DeviceMatch unicam(\"unicam\");\n> > > >> - DeviceMatch isp(\"bcm2835-isp\");\n> > > >> + unsigned int numCameras = 0;\n> > > >>\n> > > >> - unicam.add(\"unicam-image\");\n> > > >> + /*\n> > > >> + * String of indexes to append to the entity names when searching for\n> > > >> + * the Unican media devices. The first string is empty (un-indexed) to\n> > > >> + * to maintain backward compatability with old versions of the Unicam\n> > > >> + * kernel driver that did not advertise instance indexes.\n> > > >> + */\n> > > >> + for (const std::string &unicamIdStr : { \"\", \"0\", \"1\" }) {\n> > > >> + MediaDevice *unicamDevice;\n> > > >>\n> > > >> - isp.add(\"bcm2835-isp0-output0\"); /* Input */\n> > > >> - isp.add(\"bcm2835-isp0-capture1\"); /* Output 0 */\n> > > >> - isp.add(\"bcm2835-isp0-capture2\"); /* Output 1 */\n> > > >> - isp.add(\"bcm2835-isp0-capture3\"); /* Stats */\n> > > >> + DeviceMatch unicam(\"unicam\");\n> > > >> + unicam.add(\"unicam\" + unicamIdStr + \"-image\");\n> > > >> + unicamDevice = acquireMediaDevice(enumerator, unicam);\n> > > >>\n> > > >> - unicam_ = acquireMediaDevice(enumerator, unicam);\n> > > >> - if (!unicam_)\n> > > >> - return false;\n> > > >> + if (!unicamDevice)\n> > > >> + continue;\n> > > >>\n> > > >> - isp_ = acquireMediaDevice(enumerator, isp);\n> > > >> - if (!isp_)\n> > > >> - return false;\n> > > >> + for (const std::string &ispIdStr : { \"0\", \"1\" }) {\n> >\n> > I'm not sure to understand why you have nested loops, shouldn't you\n> > acquire the unicam and ISP devices separately ?\n> \n> Each unicam instance must be matched with an isp instance, else\n> the camera cannot be registered.\n> \n> I used nested loops for convenience, it's not strictly necessary.\n> With the nested loops, I don't need to keep arrays of MediaDevice\n> pointers sticking around :-)\n\nIf the two ISP instances are identical, it sounds like you could support\nthis in a much simpler way, by acquiring one unicam instance and one ISP\ninstance only in the pipeline handler. match() will be called in a loop\nuntil it returns false, so you'll have two independent pipeline handler\ninstances, handling one camera each. Handling multiple cameras in a\nsingle pipeline handler instance is only required when cameras share\nresources.\n\n> > > >> + MediaDevice *ispDevice;\n> > > >> + int ret;\n> > > >> +\n> > > >> + DeviceMatch isp(\"bcm2835-isp\");\n> > > >> + isp.add(\"bcm2835-isp\" + ispIdStr + \"-output0\"); /* Input */\n> > > >> + isp.add(\"bcm2835-isp\" + ispIdStr + \"-capture1\"); /* Output 0 */\n> > > >> + isp.add(\"bcm2835-isp\" + ispIdStr + \"-capture2\"); /* Output 0 */\n> > > >> + isp.add(\"bcm2835-isp\" + ispIdStr + \"-capture3\"); /* Stats */\n> > > >> + ispDevice = acquireMediaDevice(enumerator, isp);\n> > > >> +\n> > > >> + if (!ispDevice)\n> > > >> + continue;\n> > > >> +\n> > > >> + ret = registerCameras(unicamDevice, ispDevice, unicamIdStr, ispIdStr);\n> > > >> + if (ret) {\n> > > >> + LOG(RPI, Error) << \"Failed to register camera: \" << ret;\n> > > >> + ispDevice->release();\n> > > >> + unicamDevice->release();\n> > > >\n> > > > I am not too sure if the above two lines are\n> > > > enough. PipelineHandler::acquireMediaDevice()\n> > > > does a MediaDevice::acquire() but also adds the media device internally to\n> > > > a private member:\n> > > >\n> > > > std::vector> mediaDevices_;\n> > > >\n> > > > Perhaps I ought to implement a new public\n> > > > function PipelineHandler::acquireMediaDevice(MediaDevice *)\n> > >\n> > > Sorry, that should read PipelineHandler::releaseMediaDevice(MediaDevice *)\n> > > :-(\n> > >\n> > > > that removes the entry in mediaDevices_ as well as does a\n> > > > MediaDevice::release(). Thoughts?\n> >\n> > The media device acquired by the pipeline handler are meant to be\n> > released automatically. Camera registration failure isn't supposed to\n> > happen, in case one camera fails to register and the other doesn't, is\n> > there any issue keeping the media devices for the failed camera acquired\n> > ?\n> \n> I suppose there is no harm in keeping the device acquired when the camera cannot\n> be registered, particularly as it cannot be used again. In which case I can simplify the\n> error path logic.\n> \n> > > >> + } else\n> > > >> + numCameras++;\n> > > >>\n> > > >> - return registerCameras();\n> > > >> + break;\n> > > >> + }\n> > > >> + }\n> > > >> +\n> > > >> + return !!numCameras;\n> > > >> }\n> > > >>\n> > > >> -bool PipelineHandlerRPi::registerCameras()\n> > > >> +int PipelineHandlerRPi::registerCameras(MediaDevice *unicam, MediaDevice *isp,\n> > > >> + const std::string &unicamIdStr,\n> > > >> + const std::string &ispIdStr)\n> > > >> {\n> > > >> std::unique_ptr data = std::make_unique(this);\n> > > >> +\n> > > >> if (!data->dmaHeap_.isValid())\n> > > >> - return false;\n> > > >> + return -ENOMEM;\n> > > >> +\n> > > >> + MediaEntity *unicamImage = unicam->getEntityByName(\"unicam\" + unicamIdStr + \"-image\");\n> > > >> + MediaEntity *ispOutput0 = isp->getEntityByName(\"bcm2835-isp\" + ispIdStr + \"-output0\");\n> > > >> + MediaEntity *ispCapture1 = isp->getEntityByName(\"bcm2835-isp\" + ispIdStr + \"-capture1\");\n> > > >> + MediaEntity *ispCapture2 = isp->getEntityByName(\"bcm2835-isp\" + ispIdStr + \"-capture2\");\n> > > >> + MediaEntity *ispCapture3 = isp->getEntityByName(\"bcm2835-isp\" + ispIdStr + \"-capture3\");\n> > > >> +\n> > > >> + if (!unicamImage || !ispOutput0 || !ispCapture1 || !ispCapture2 || !ispCapture3)\n> > > >> + return -ENOENT;\n> > > >>\n> > > >> /* Locate and open the unicam video streams. */\n> > > >> - data->unicam_[Unicam::Image] = RPi::Stream(\"Unicam Image\", unicam_->getEntityByName(\"unicam-image\"));\n> > > >> + data->unicam_[Unicam::Image] = RPi::Stream(\"Unicam Image\", unicamImage);\n> > > >>\n> > > >> /* An embedded data node will not be present if the sensor does not support it. */\n> > > >> - MediaEntity *embeddedEntity = unicam_->getEntityByName(\"unicam-embedded\");\n> > > >> - if (embeddedEntity) {\n> > > >> - data->unicam_[Unicam::Embedded] = RPi::Stream(\"Unicam Embedded\", embeddedEntity);\n> > > >> + MediaEntity *unicamEmbedded = unicam->getEntityByName(\"unicam\" + unicamIdStr + \"-embedded\");\n> > > >> + if (unicamEmbedded) {\n> > > >> + data->unicam_[Unicam::Embedded] = RPi::Stream(\"Unicam Embedded\", unicamEmbedded);\n> > > >>\n> > > >> data->unicam_[Unicam::Embedded].dev()->bufferReady.connect(data.get(),\n> > > >> &RPiCameraData::unicamBufferDequeue);\n> > > >> }\n> > > >>\n> > > >> /* Tag the ISP input stream as an import stream. */\n> > > >> - data->isp_[Isp::Input] = RPi::Stream(\"ISP Input\", isp_->getEntityByName(\"bcm2835-isp0-output0\"), true);\n> > > >> - data->isp_[Isp::Output0] = RPi::Stream(\"ISP Output0\", isp_->getEntityByName(\"bcm2835-isp0-capture1\"));\n> > > >> - data->isp_[Isp::Output1] = RPi::Stream(\"ISP Output1\", isp_->getEntityByName(\"bcm2835-isp0-capture2\"));\n> > > >> - data->isp_[Isp::Stats] = RPi::Stream(\"ISP Stats\", isp_->getEntityByName(\"bcm2835-isp0-capture3\"));\n> > > >> + data->isp_[Isp::Input] = RPi::Stream(\"ISP Input\", ispOutput0, true);\n> > > >> + data->isp_[Isp::Output0] = RPi::Stream(\"ISP Output0\", ispCapture1);\n> > > >> + data->isp_[Isp::Output1] = RPi::Stream(\"ISP Output1\", ispCapture2);\n> > > >> + data->isp_[Isp::Stats] = RPi::Stream(\"ISP Stats\", ispCapture3);\n> > > >>\n> > > >> /* Wire up all the buffer connections. */\n> > > >> data->unicam_[Unicam::Image].dev()->frameStart.connect(data.get(), &RPiCameraData::frameStarted);\n> > > >> @@ -1046,7 +1085,7 @@ bool PipelineHandlerRPi::registerCameras()\n> > > >> data->isp_[Isp::Stats].dev()->bufferReady.connect(data.get(), &RPiCameraData::ispOutputDequeue);\n> > > >>\n> > > >> /* Identify the sensor. */\n> > > >> - for (MediaEntity *entity : unicam_->entities()) {\n> > > >> + for (MediaEntity *entity : unicam->entities()) {\n> > > >> if (entity->function() == MEDIA_ENT_F_CAM_SENSOR) {\n> > > >> data->sensor_ = std::make_unique(entity);\n> > > >> break;\n> > > >> @@ -1054,23 +1093,23 @@ bool PipelineHandlerRPi::registerCameras()\n> > > >> }\n> > > >>\n> > > >> if (!data->sensor_)\n> > > >> - return false;\n> > > >> + return -EINVAL;\n> > > >>\n> > > >> if (data->sensor_->init())\n> > > >> - return false;\n> > > >> + return -EINVAL;\n> > > >>\n> > > >> data->sensorFormats_ = populateSensorFormats(data->sensor_);\n> > > >>\n> > > >> ipa::RPi::SensorConfig sensorConfig;\n> > > >> if (data->loadIPA(&sensorConfig)) {\n> > > >> LOG(RPI, Error) << \"Failed to load a suitable IPA library\";\n> > > >> - return false;\n> > > >> + return -EINVAL;\n> > > >> }\n> > > >>\n> > > >> - if (sensorConfig.sensorMetadata ^ !!embeddedEntity) {\n> > > >> + if (sensorConfig.sensorMetadata ^ !!unicamEmbedded) {\n> > > >> LOG(RPI, Warning) << \"Mismatch between Unicam and CamHelper for embedded data usage!\";\n> > > >> sensorConfig.sensorMetadata = false;\n> > > >> - if (embeddedEntity)\n> > > >> + if (unicamEmbedded)\n> > > >> data->unicam_[Unicam::Embedded].dev()->bufferReady.disconnect();\n> > > >> }\n> > > >>\n> > > >> @@ -1091,12 +1130,12 @@ bool PipelineHandlerRPi::registerCameras()\n> > > >>\n> > > >> for (auto stream : data->streams_) {\n> > > >> if (stream->dev()->open())\n> > > >> - return false;\n> > > >> + continue;\n> > > >> }\n> > > >>\n> > > >> if (!data->unicam_[Unicam::Image].dev()->caps().hasMediaController()) {\n> > > >> LOG(RPI, Error) << \"Unicam driver does not use the MediaController, please update your kernel!\";\n> > > >> - return false;\n> > > >> + return -EINVAL;\n> > > >> }\n> > > >>\n> > > >> /*\n> > > >> @@ -1158,7 +1197,7 @@ bool PipelineHandlerRPi::registerCameras()\n> > > >>\n> > > >> if (!bayerFormat.isValid()) {\n> > > >> LOG(RPI, Error) << \"No Bayer format found\";\n> > > >> - return false;\n> > > >> + return -EINVAL;\n> > > >> }\n> > > >> data->nativeBayerOrder_ = bayerFormat.order;\n> > > >>\n> > > >> @@ -1178,7 +1217,10 @@ bool PipelineHandlerRPi::registerCameras()\n> > > >> Camera::create(std::move(data), id, streams);\n> > > >> registerCamera(std::move(camera));\n> > > >>\n> > > >> - return true;\n> > > >> + LOG(RPI, Info) << \"Registered camera \" << id\n> > > >> + << \" to instances \\\"unicam\" << unicamIdStr << \"\\\"\"\n> > > >> + << \" and \\\"isp\" << ispIdStr << \"\\\"\";\n> > > >> + return 0;\n> > > >> }\n> > > >>\n> > > >> int PipelineHandlerRPi::queueAllBuffers(Camera *camera)","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 CCFBCBDB13\n\tfor ;\n\tFri, 19 Nov 2021 20:53:20 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1717660371;\n\tFri, 19 Nov 2021 21:53:20 +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 9B96C600B5\n\tfor ;\n\tFri, 19 Nov 2021 21:53:18 +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 13CE81C19;\n\tFri, 19 Nov 2021 21:53:17 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"j0dKgn1t\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1637355198;\n\tbh=O5wCK9Ab6oE9YqPhBOLJbcElumbwMHwBLjwre86M5rI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=j0dKgn1tOa73b6GjHT0KIFqL8Z7cqHdTvs2mp8uIC+SvnYj8Cqq6pvzwVCIk9aBF1\n\txtC5VKutok279ToI528e9nKxrV6WQq+PsC+YxJTTZ9R7ZpmKJxu4HNe2SeZetgXmN5\n\t5WLHFzHp7bmSZKViqLycOx7hTkTOnfWJRXXHFoyE=","Date":"Fri, 19 Nov 2021 22:52:55 +0200","From":"Laurent Pinchart ","To":"Naushir Patuck ","Message-ID":"","References":"<20211119110955.3137585-1-naush@raspberrypi.com>\n\t<20211119110955.3137585-2-naush@raspberrypi.com>\n\t\n\t\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 2/3] pipeline: raspberrypi: Allow\n\tregistration of multiple cameras","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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":21070,"web_url":"https://patchwork.libcamera.org/comment/21070/","msgid":"","date":"2021-11-20T07:28:19","subject":"Re: [libcamera-devel] [PATCH 2/3] pipeline: raspberrypi: Allow\n\tregistration of multiple cameras","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Laurent,\n\n\nOn Fri, 19 Nov 2021 at 20:53, Laurent Pinchart <\nlaurent.pinchart@ideasonboard.com> wrote:\n\n> Hi Naush,\n>\n> On Fri, Nov 19, 2021 at 04:45:27PM +0000, Naushir Patuck wrote:\n> > On Fri, 19 Nov 2021 at 13:03, Laurent Pinchart wrote:\n> > > On Fri, Nov 19, 2021 at 11:28:39AM +0000, Naushir Patuck wrote:\n> > > > On Fri, 19 Nov 2021 at 11:27, Naushir Patuck wrote:\n> > > > > On Fri, 19 Nov 2021 at 11:10, Naushir Patuck wrote:\n> > > > >\n> > > > >> Expand the pipeline handler camera registration to correctly\n> handle multiple\n> > > > >> cameras attached to the platform. For example, Raspberry Pi\n> Compute Module\n> > > > >> platforms have two camera connectors, and this change would allow\n> the user to\n> > > > >> select either of the two cameras to run.\n> > >\n> > > That's interesting :-)\n> > >\n> > > > >> There are associated kernel driver changes for both Unicam and\n> the ISP needed\n> > > > >> to correctly advertise multiple media devices and nodes for\n> multi-camera usage:\n> > > > >>\n> > > > >> https://github.com/raspberrypi/linux/pull/4140\n> > > > >> https://github.com/raspberrypi/linux/pull/4709\n> > >\n> > > There's still a single ISP instance. How does it work at the\n> > > hardware/firmware level ? The kernel patch seems simple, how are ISP\n> > > operations queued by users scheduled and arbitrated ? Can the two\n> > > cameras be used concurrently, or are they mutually exclusive ?\n> >\n> > The isp driver change will allow concurrent usage of the ISP hardware.\n> > What's missing from your view is the shim in the firmware that handles\n> > the arbitration between the many users ;-) This makes adding multi-user\n> > to the kernel driver mostly a duplication of the single-user instance.\n>\n> Are there advantages in doing so in the firmware instead of in the Linux\n> kernel driver, or even in userspace in libcamera ? I suppose that if\n> it's there already it's the simplest way forward, I'm not requesting a\n> change here, but I'm wondering what the design rationale is.\n>\n\nThe firmware already has support for concurrent usage as we need\nit to share the ISP HW with the codec driver where ISP is used to generate\na format that our codec block understands.\n\nAnother advantage is that it would be easier to context switch between\nframes\non a stripe level rather than at frame level. This could be duplicated in\nthe\nkernel driver but would require much more effort.\n\n\n>\n> How should libcamera cope with limitations in terms of ISP bandwidth\n> with multiple instances ?\n>\n\nThat's a good question, and I am not sure of the answer. Right now, our\npipeline handler will simply drop frames when it cannot keep up in the HW.\nIt would be easy enough to calculate expected throughput in the pipeline\nhandler\ninstance, but how do we know if/when other users outside of libcamera\nare also using the ISP?\n\n\n>\n> > As an aside, I have run into a few issues with actual concurrent usage\n> > in the pipeline handlers. I've been talking with Jacopo offline about\n> > this, but we can keep that discussion as a separate thread to this work.\n>\n>\n> https://git.libcamera.org/libcamera/pinchartl/libcamera.git/log/?h=mtk/multi-cam\n> may help already. We'll also need an API for pipeline handlers to report\n> mutual exclusion constraints between camera, and a way to expose that to\n> applications.\n>\n> > > > >> However, this change is backward compatible with kernel builds\n> that do not have\n> > > > >> these changes for standard single camera usage.\n> > > > >>\n> > > > >> Signed-off-by: Naushir Patuck \n> > > > >> ---\n> > > > >> .../pipeline/raspberrypi/raspberrypi.cpp | 112\n> ++++++++++++------\n> > > > >> 1 file changed, 77 insertions(+), 35 deletions(-)\n> > > > >>\n> > > > >> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > >> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > >> index 9aa7e9eef5e7..8ed2ebcaafe7 100644\n> > > > >> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > >> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > >> @@ -311,7 +311,8 @@ private:\n> > > > >> return static_cast(camera->_d());\n> > > > >> }\n> > > > >>\n> > > > >> - bool registerCameras();\n> > > > >> + int registerCameras(MediaDevice *unicam, MediaDevice *isp,\n> > > > >> + const std::string &unicamIdStr, const\n> std::string &ispIdStr);\n> > > > >> int queueAllBuffers(Camera *camera);\n> > > > >> int prepareBuffers(Camera *camera);\n> > > > >> void freeBuffers(Camera *camera);\n> > > > >> @@ -993,49 +994,87 @@ int\n> PipelineHandlerRPi::queueRequestDevice(Camera *camera, Request *request)\n> > > > >>\n> > > > >> bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)\n> > > > >> {\n> > > > >> - DeviceMatch unicam(\"unicam\");\n> > > > >> - DeviceMatch isp(\"bcm2835-isp\");\n> > > > >> + unsigned int numCameras = 0;\n> > > > >>\n> > > > >> - unicam.add(\"unicam-image\");\n> > > > >> + /*\n> > > > >> + * String of indexes to append to the entity names when\n> searching for\n> > > > >> + * the Unican media devices. The first string is empty\n> (un-indexed) to\n> > > > >> + * to maintain backward compatability with old versions\n> of the Unicam\n> > > > >> + * kernel driver that did not advertise instance indexes.\n> > > > >> + */\n> > > > >> + for (const std::string &unicamIdStr : { \"\", \"0\", \"1\" }) {\n> > > > >> + MediaDevice *unicamDevice;\n> > > > >>\n> > > > >> - isp.add(\"bcm2835-isp0-output0\"); /* Input */\n> > > > >> - isp.add(\"bcm2835-isp0-capture1\"); /* Output 0 */\n> > > > >> - isp.add(\"bcm2835-isp0-capture2\"); /* Output 1 */\n> > > > >> - isp.add(\"bcm2835-isp0-capture3\"); /* Stats */\n> > > > >> + DeviceMatch unicam(\"unicam\");\n> > > > >> + unicam.add(\"unicam\" + unicamIdStr + \"-image\");\n> > > > >> + unicamDevice = acquireMediaDevice(enumerator,\n> unicam);\n> > > > >>\n> > > > >> - unicam_ = acquireMediaDevice(enumerator, unicam);\n> > > > >> - if (!unicam_)\n> > > > >> - return false;\n> > > > >> + if (!unicamDevice)\n> > > > >> + continue;\n> > > > >>\n> > > > >> - isp_ = acquireMediaDevice(enumerator, isp);\n> > > > >> - if (!isp_)\n> > > > >> - return false;\n> > > > >> + for (const std::string &ispIdStr : { \"0\", \"1\" }) {\n> > >\n> > > I'm not sure to understand why you have nested loops, shouldn't you\n> > > acquire the unicam and ISP devices separately ?\n> >\n> > Each unicam instance must be matched with an isp instance, else\n> > the camera cannot be registered.\n> >\n> > I used nested loops for convenience, it's not strictly necessary.\n> > With the nested loops, I don't need to keep arrays of MediaDevice\n> > pointers sticking around :-)\n>\n> If the two ISP instances are identical, it sounds like you could support\n> this in a much simpler way, by acquiring one unicam instance and one ISP\n> instance only in the pipeline handler. match() will be called in a loop\n> until it returns false, so you'll have two independent pipeline handler\n> instances, handling one camera each. Handling multiple cameras in a\n> single pipeline handler instance is only required when cameras share\n> resources.\n>\n\nAh, this is what I was missing! I did not realise that match() would be\ncalled\nin a loop until it fails. So I can adjust this code to only register one\ncamera\nper call and things should be simpler! I'll make that change for the next\nversion of this series.\n\nRegards,\nNaush\n\n\n>\n> > > > >> + MediaDevice *ispDevice;\n> > > > >> + int ret;\n> > > > >> +\n> > > > >> + DeviceMatch isp(\"bcm2835-isp\");\n> > > > >> + isp.add(\"bcm2835-isp\" + ispIdStr +\n> \"-output0\"); /* Input */\n> > > > >> + isp.add(\"bcm2835-isp\" + ispIdStr +\n> \"-capture1\"); /* Output 0 */\n> > > > >> + isp.add(\"bcm2835-isp\" + ispIdStr +\n> \"-capture2\"); /* Output 0 */\n> > > > >> + isp.add(\"bcm2835-isp\" + ispIdStr +\n> \"-capture3\"); /* Stats */\n> > > > >> + ispDevice =\n> acquireMediaDevice(enumerator, isp);\n> > > > >> +\n> > > > >> + if (!ispDevice)\n> > > > >> + continue;\n> > > > >> +\n> > > > >> + ret = registerCameras(unicamDevice,\n> ispDevice, unicamIdStr, ispIdStr);\n> > > > >> + if (ret) {\n> > > > >> + LOG(RPI, Error) << \"Failed to\n> register camera: \" << ret;\n> > > > >> + ispDevice->release();\n> > > > >> + unicamDevice->release();\n> > > > >\n> > > > > I am not too sure if the above two lines are\n> > > > > enough. PipelineHandler::acquireMediaDevice()\n> > > > > does a MediaDevice::acquire() but also adds the media device\n> internally to\n> > > > > a private member:\n> > > > >\n> > > > > std::vector> mediaDevices_;\n> > > > >\n> > > > > Perhaps I ought to implement a new public\n> > > > > function PipelineHandler::acquireMediaDevice(MediaDevice *)\n> > > >\n> > > > Sorry, that should read\n> PipelineHandler::releaseMediaDevice(MediaDevice *)\n> > > > :-(\n> > > >\n> > > > > that removes the entry in mediaDevices_ as well as does a\n> > > > > MediaDevice::release(). Thoughts?\n> > >\n> > > The media device acquired by the pipeline handler are meant to be\n> > > released automatically. Camera registration failure isn't supposed to\n> > > happen, in case one camera fails to register and the other doesn't, is\n> > > there any issue keeping the media devices for the failed camera\n> acquired\n> > > ?\n> >\n> > I suppose there is no harm in keeping the device acquired when the\n> camera cannot\n> > be registered, particularly as it cannot be used again. In which case I\n> can simplify the\n> > error path logic.\n> >\n> > > > >> + } else\n> > > > >> + numCameras++;\n> > > > >>\n> > > > >> - return registerCameras();\n> > > > >> + break;\n> > > > >> + }\n> > > > >> + }\n> > > > >> +\n> > > > >> + return !!numCameras;\n> > > > >> }\n> > > > >>\n> > > > >> -bool PipelineHandlerRPi::registerCameras()\n> > > > >> +int PipelineHandlerRPi::registerCameras(MediaDevice *unicam,\n> MediaDevice *isp,\n> > > > >> + const std::string\n> &unicamIdStr,\n> > > > >> + const std::string\n> &ispIdStr)\n> > > > >> {\n> > > > >> std::unique_ptr data =\n> std::make_unique(this);\n> > > > >> +\n> > > > >> if (!data->dmaHeap_.isValid())\n> > > > >> - return false;\n> > > > >> + return -ENOMEM;\n> > > > >> +\n> > > > >> + MediaEntity *unicamImage =\n> unicam->getEntityByName(\"unicam\" + unicamIdStr + \"-image\");\n> > > > >> + MediaEntity *ispOutput0 =\n> isp->getEntityByName(\"bcm2835-isp\" + ispIdStr + \"-output0\");\n> > > > >> + MediaEntity *ispCapture1 =\n> isp->getEntityByName(\"bcm2835-isp\" + ispIdStr + \"-capture1\");\n> > > > >> + MediaEntity *ispCapture2 =\n> isp->getEntityByName(\"bcm2835-isp\" + ispIdStr + \"-capture2\");\n> > > > >> + MediaEntity *ispCapture3 =\n> isp->getEntityByName(\"bcm2835-isp\" + ispIdStr + \"-capture3\");\n> > > > >> +\n> > > > >> + if (!unicamImage || !ispOutput0 || !ispCapture1 ||\n> !ispCapture2 || !ispCapture3)\n> > > > >> + return -ENOENT;\n> > > > >>\n> > > > >> /* Locate and open the unicam video streams. */\n> > > > >> - data->unicam_[Unicam::Image] = RPi::Stream(\"Unicam\n> Image\", unicam_->getEntityByName(\"unicam-image\"));\n> > > > >> + data->unicam_[Unicam::Image] = RPi::Stream(\"Unicam\n> Image\", unicamImage);\n> > > > >>\n> > > > >> /* An embedded data node will not be present if the\n> sensor does not support it. */\n> > > > >> - MediaEntity *embeddedEntity =\n> unicam_->getEntityByName(\"unicam-embedded\");\n> > > > >> - if (embeddedEntity) {\n> > > > >> - data->unicam_[Unicam::Embedded] =\n> RPi::Stream(\"Unicam Embedded\", embeddedEntity);\n> > > > >> + MediaEntity *unicamEmbedded =\n> unicam->getEntityByName(\"unicam\" + unicamIdStr + \"-embedded\");\n> > > > >> + if (unicamEmbedded) {\n> > > > >> + data->unicam_[Unicam::Embedded] =\n> RPi::Stream(\"Unicam Embedded\", unicamEmbedded);\n> > > > >>\n> > > > >>\n> data->unicam_[Unicam::Embedded].dev()->bufferReady.connect(data.get(),\n> > > > >> &RPiCameraData::unicamBufferDequeue);\n> > > > >> }\n> > > > >>\n> > > > >> /* Tag the ISP input stream as an import stream. */\n> > > > >> - data->isp_[Isp::Input] = RPi::Stream(\"ISP Input\",\n> isp_->getEntityByName(\"bcm2835-isp0-output0\"), true);\n> > > > >> - data->isp_[Isp::Output0] = RPi::Stream(\"ISP Output0\",\n> isp_->getEntityByName(\"bcm2835-isp0-capture1\"));\n> > > > >> - data->isp_[Isp::Output1] = RPi::Stream(\"ISP Output1\",\n> isp_->getEntityByName(\"bcm2835-isp0-capture2\"));\n> > > > >> - data->isp_[Isp::Stats] = RPi::Stream(\"ISP Stats\",\n> isp_->getEntityByName(\"bcm2835-isp0-capture3\"));\n> > > > >> + data->isp_[Isp::Input] = RPi::Stream(\"ISP Input\",\n> ispOutput0, true);\n> > > > >> + data->isp_[Isp::Output0] = RPi::Stream(\"ISP Output0\",\n> ispCapture1);\n> > > > >> + data->isp_[Isp::Output1] = RPi::Stream(\"ISP Output1\",\n> ispCapture2);\n> > > > >> + data->isp_[Isp::Stats] = RPi::Stream(\"ISP Stats\",\n> ispCapture3);\n> > > > >>\n> > > > >> /* Wire up all the buffer connections. */\n> > > > >>\n> data->unicam_[Unicam::Image].dev()->frameStart.connect(data.get(),\n> &RPiCameraData::frameStarted);\n> > > > >> @@ -1046,7 +1085,7 @@ bool PipelineHandlerRPi::registerCameras()\n> > > > >>\n> data->isp_[Isp::Stats].dev()->bufferReady.connect(data.get(),\n> &RPiCameraData::ispOutputDequeue);\n> > > > >>\n> > > > >> /* Identify the sensor. */\n> > > > >> - for (MediaEntity *entity : unicam_->entities()) {\n> > > > >> + for (MediaEntity *entity : unicam->entities()) {\n> > > > >> if (entity->function() == MEDIA_ENT_F_CAM_SENSOR)\n> {\n> > > > >> data->sensor_ =\n> std::make_unique(entity);\n> > > > >> break;\n> > > > >> @@ -1054,23 +1093,23 @@ bool PipelineHandlerRPi::registerCameras()\n> > > > >> }\n> > > > >>\n> > > > >> if (!data->sensor_)\n> > > > >> - return false;\n> > > > >> + return -EINVAL;\n> > > > >>\n> > > > >> if (data->sensor_->init())\n> > > > >> - return false;\n> > > > >> + return -EINVAL;\n> > > > >>\n> > > > >> data->sensorFormats_ =\n> populateSensorFormats(data->sensor_);\n> > > > >>\n> > > > >> ipa::RPi::SensorConfig sensorConfig;\n> > > > >> if (data->loadIPA(&sensorConfig)) {\n> > > > >> LOG(RPI, Error) << \"Failed to load a suitable IPA\n> library\";\n> > > > >> - return false;\n> > > > >> + return -EINVAL;\n> > > > >> }\n> > > > >>\n> > > > >> - if (sensorConfig.sensorMetadata ^ !!embeddedEntity) {\n> > > > >> + if (sensorConfig.sensorMetadata ^ !!unicamEmbedded) {\n> > > > >> LOG(RPI, Warning) << \"Mismatch between Unicam and\n> CamHelper for embedded data usage!\";\n> > > > >> sensorConfig.sensorMetadata = false;\n> > > > >> - if (embeddedEntity)\n> > > > >> + if (unicamEmbedded)\n> > > > >> data->unicam_[Unicam::Embedded].dev()->bufferReady.disconnect();\n> > > > >> }\n> > > > >>\n> > > > >> @@ -1091,12 +1130,12 @@ bool PipelineHandlerRPi::registerCameras()\n> > > > >>\n> > > > >> for (auto stream : data->streams_) {\n> > > > >> if (stream->dev()->open())\n> > > > >> - return false;\n> > > > >> + continue;\n> > > > >> }\n> > > > >>\n> > > > >> if\n> (!data->unicam_[Unicam::Image].dev()->caps().hasMediaController()) {\n> > > > >> LOG(RPI, Error) << \"Unicam driver does not use\n> the MediaController, please update your kernel!\";\n> > > > >> - return false;\n> > > > >> + return -EINVAL;\n> > > > >> }\n> > > > >>\n> > > > >> /*\n> > > > >> @@ -1158,7 +1197,7 @@ bool PipelineHandlerRPi::registerCameras()\n> > > > >>\n> > > > >> if (!bayerFormat.isValid()) {\n> > > > >> LOG(RPI, Error) << \"No Bayer format found\";\n> > > > >> - return false;\n> > > > >> + return -EINVAL;\n> > > > >> }\n> > > > >> data->nativeBayerOrder_ = bayerFormat.order;\n> > > > >>\n> > > > >> @@ -1178,7 +1217,10 @@ bool PipelineHandlerRPi::registerCameras()\n> > > > >> Camera::create(std::move(data), id, streams);\n> > > > >> registerCamera(std::move(camera));\n> > > > >>\n> > > > >> - return true;\n> > > > >> + LOG(RPI, Info) << \"Registered camera \" << id\n> > > > >> + << \" to instances \\\"unicam\" << unicamIdStr\n> << \"\\\"\"\n> > > > >> + << \" and \\\"isp\" << ispIdStr << \"\\\"\";\n> > > > >> + return 0;\n> > > > >> }\n> > > > >>\n> > > > >> int PipelineHandlerRPi::queueAllBuffers(Camera *camera)\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 6DF34BF415\n\tfor ;\n\tSat, 20 Nov 2021 07:28:40 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A967E60378;\n\tSat, 20 Nov 2021 08:28:39 +0100 (CET)","from mail-lf1-x134.google.com (mail-lf1-x134.google.com\n\t[IPv6:2a00:1450:4864:20::134])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B7FA260231\n\tfor ;\n\tSat, 20 Nov 2021 08:28:37 +0100 (CET)","by mail-lf1-x134.google.com with SMTP id n12so54179163lfe.1\n\tfor ;\n\tFri, 19 Nov 2021 23:28:37 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"g0xX6lK6\"; 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=CjJkGJ73n+NsFtJAHn0btNHz5idjMgwPvgYxlQxw8JI=;\n\tb=g0xX6lK6pRhz5aq09HGO/OHLHAvehP/i/UHPmfM0ydUMbZTrRM4xfFqpDNwJncHwG1\n\tkcMgZbf49dM/rwQ6PsTt5OQgnCFJDMWVzseVg0gcMVgqK5O7/suCcvXF2JsgiLFcUF53\n\tSUWk/yqw8BnLuYHqhZHJMkvyAP5XWatyQnKFcsdzwjI1fNQRZQsEh17M/wcxSCFc8slQ\n\tXgnuLO6n3ZYJdSTxFEVaitTLe3oAWNaTZjP8jtrCNZWmRPUnn0SBg/35uAyUbwa1kude\n\tgPbcAe0kGx3XAgDULPhcVbAsVYuHVxj/cMH4JIIbFre53TArHW6boi+nW/nnU3d6r1k6\n\tknXQ==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=CjJkGJ73n+NsFtJAHn0btNHz5idjMgwPvgYxlQxw8JI=;\n\tb=3E/RS3oYiY5usiJNHs+fQ0UaPkWrq+2ahLWExwmyP2u+gzYFIKXW2bpCPJcJVAQlxE\n\tbJg7ymisBaEVoAZhVyRlJ01m3oBYhOuWu8G1Nou7P9x32JmvMUEtIErDZmZ9sn5+1k7/\n\tG6C1CVdFcUKyy6grnBrBEeKqX3hKDU6l9SOD7AFjJx4g/PhLQjXcAvvZ7jWgx/GVBsnY\n\t7fOf3TRG8TZFJqIeI0oOBGKCPg1aq9qMbeV/bKBDZZwZDtvRka/dgahTcSAbrPUqo4zl\n\tbIDoTDeJEBrdUyBUMbH2ImTydfm3qa4W3MHYEHrwSdaG8qmieG3rxwQv8c1tBq27qz2i\n\tossw==","X-Gm-Message-State":"AOAM531VnHYc/jsKrPNUoq/LxbNhU4lWYqRl9PIwRgIyAwefl5TMIfBp\n\ttnyU5ofdS0VC6faAwAsoAXDK/or4TvDu2pCf8zwyOw==","X-Google-Smtp-Source":"ABdhPJzP9MmwrciFCNEKWaeRn4s4Qoivz3+Ae28togRRyrJA4cUY3I5/KPvm+YJj+kkNNKkxYy8O9Q6/NGcs3UDjZFY=","X-Received":"by 2002:a19:6754:: with SMTP id\n\te20mr37741035lfj.122.1637393316501; \n\tFri, 19 Nov 2021 23:28:36 -0800 (PST)","MIME-Version":"1.0","References":"<20211119110955.3137585-1-naush@raspberrypi.com>\n\t<20211119110955.3137585-2-naush@raspberrypi.com>\n\t\n\t\n\t\n\t\n\t","In-Reply-To":"","From":"Naushir Patuck ","Date":"Sat, 20 Nov 2021 07:28:19 +0000","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"multipart/alternative; boundary=\"000000000000fcf47205d1335636\"","Subject":"Re: [libcamera-devel] [PATCH 2/3] pipeline: raspberrypi: Allow\n\tregistration of multiple cameras","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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":21084,"web_url":"https://patchwork.libcamera.org/comment/21084/","msgid":"","date":"2021-11-21T23:25:21","subject":"Re: [libcamera-devel] [PATCH 2/3] pipeline: raspberrypi: Allow\n\tregistration of multiple cameras","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Naush,\n\nOn Sat, Nov 20, 2021 at 07:28:19AM +0000, Naushir Patuck wrote:\n> On Fri, 19 Nov 2021 at 20:53, Laurent Pinchart wrote:\n> > On Fri, Nov 19, 2021 at 04:45:27PM +0000, Naushir Patuck wrote:\n> > > On Fri, 19 Nov 2021 at 13:03, Laurent Pinchart wrote:\n> > > > On Fri, Nov 19, 2021 at 11:28:39AM +0000, Naushir Patuck wrote:\n> > > > > On Fri, 19 Nov 2021 at 11:27, Naushir Patuck wrote:\n> > > > > > On Fri, 19 Nov 2021 at 11:10, Naushir Patuck wrote:\n> > > > > >\n> > > > > >> Expand the pipeline handler camera registration to correctly handle multiple\n> > > > > >> cameras attached to the platform. For example, Raspberry Pi Compute Module\n> > > > > >> platforms have two camera connectors, and this change would allow the user to\n> > > > > >> select either of the two cameras to run.\n> > > >\n> > > > That's interesting :-)\n> > > >\n> > > > > >> There are associated kernel driver changes for both Unicam and the ISP needed\n> > > > > >> to correctly advertise multiple media devices and nodes for multi-camera usage:\n> > > > > >>\n> > > > > >> https://github.com/raspberrypi/linux/pull/4140\n> > > > > >> https://github.com/raspberrypi/linux/pull/4709\n> > > >\n> > > > There's still a single ISP instance. How does it work at the\n> > > > hardware/firmware level ? The kernel patch seems simple, how are ISP\n> > > > operations queued by users scheduled and arbitrated ? Can the two\n> > > > cameras be used concurrently, or are they mutually exclusive ?\n> > >\n> > > The isp driver change will allow concurrent usage of the ISP hardware.\n> > > What's missing from your view is the shim in the firmware that handles\n> > > the arbitration between the many users ;-) This makes adding multi-user\n> > > to the kernel driver mostly a duplication of the single-user instance.\n> >\n> > Are there advantages in doing so in the firmware instead of in the Linux\n> > kernel driver, or even in userspace in libcamera ? I suppose that if\n> > it's there already it's the simplest way forward, I'm not requesting a\n> > change here, but I'm wondering what the design rationale is.\n> \n> The firmware already has support for concurrent usage as we need\n> it to share the ISP HW with the codec driver where ISP is used to generate\n> a format that our codec block understands.\n> \n> Another advantage is that it would be easier to context switch between frames\n> on a stripe level rather than at frame level. This could be duplicated in the\n> kernel driver but would require much more effort.\n> \n> > How should libcamera cope with limitations in terms of ISP bandwidth\n> > with multiple instances ?\n> \n> That's a good question, and I am not sure of the answer. Right now, our\n> pipeline handler will simply drop frames when it cannot keep up in the HW.\n> It would be easy enough to calculate expected throughput in the pipeline handler\n> instance, but how do we know if/when other users outside of libcamera\n> are also using the ISP?\n\nIf multiplexing is handled in the firmware, we'd need an API to\nnegotiate bandwidth allocation with the firmware I suppose.\n\n> > > As an aside, I have run into a few issues with actual concurrent usage\n> > > in the pipeline handlers. I've been talking with Jacopo offline about\n> > > this, but we can keep that discussion as a separate thread to this work.\n> >\n> > https://git.libcamera.org/libcamera/pinchartl/libcamera.git/log/?h=mtk/multi-cam\n> > may help already. We'll also need an API for pipeline handlers to report\n> > mutual exclusion constraints between camera, and a way to expose that to\n> > applications.\n> >\n> > > > > >> However, this change is backward compatible with kernel builds that do not have\n> > > > > >> these changes for standard single camera usage.\n> > > > > >>\n> > > > > >> Signed-off-by: Naushir Patuck \n> > > > > >> ---\n> > > > > >> .../pipeline/raspberrypi/raspberrypi.cpp | 112 ++++++++++++------\n> > > > > >> 1 file changed, 77 insertions(+), 35 deletions(-)\n> > > > > >>\n> > > > > >> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > > >> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > > >> index 9aa7e9eef5e7..8ed2ebcaafe7 100644\n> > > > > >> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > > >> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > > >> @@ -311,7 +311,8 @@ private:\n> > > > > >> return static_cast(camera->_d());\n> > > > > >> }\n> > > > > >>\n> > > > > >> - bool registerCameras();\n> > > > > >> + int registerCameras(MediaDevice *unicam, MediaDevice *isp,\n> > > > > >> + const std::string &unicamIdStr, const std::string &ispIdStr);\n> > > > > >> int queueAllBuffers(Camera *camera);\n> > > > > >> int prepareBuffers(Camera *camera);\n> > > > > >> void freeBuffers(Camera *camera);\n> > > > > >> @@ -993,49 +994,87 @@ int PipelineHandlerRPi::queueRequestDevice(Camera *camera, Request *request)\n> > > > > >>\n> > > > > >> bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)\n> > > > > >> {\n> > > > > >> - DeviceMatch unicam(\"unicam\");\n> > > > > >> - DeviceMatch isp(\"bcm2835-isp\");\n> > > > > >> + unsigned int numCameras = 0;\n> > > > > >>\n> > > > > >> - unicam.add(\"unicam-image\");\n> > > > > >> + /*\n> > > > > >> + * String of indexes to append to the entity names when searching for\n> > > > > >> + * the Unican media devices. The first string is empty (un-indexed) to\n> > > > > >> + * to maintain backward compatability with old versions of the Unicam\n> > > > > >> + * kernel driver that did not advertise instance indexes.\n> > > > > >> + */\n> > > > > >> + for (const std::string &unicamIdStr : { \"\", \"0\", \"1\" }) {\n> > > > > >> + MediaDevice *unicamDevice;\n> > > > > >>\n> > > > > >> - isp.add(\"bcm2835-isp0-output0\"); /* Input */\n> > > > > >> - isp.add(\"bcm2835-isp0-capture1\"); /* Output 0 */\n> > > > > >> - isp.add(\"bcm2835-isp0-capture2\"); /* Output 1 */\n> > > > > >> - isp.add(\"bcm2835-isp0-capture3\"); /* Stats */\n> > > > > >> + DeviceMatch unicam(\"unicam\");\n> > > > > >> + unicam.add(\"unicam\" + unicamIdStr + \"-image\");\n> > > > > >> + unicamDevice = acquireMediaDevice(enumerator, unicam);\n> > > > > >>\n> > > > > >> - unicam_ = acquireMediaDevice(enumerator, unicam);\n> > > > > >> - if (!unicam_)\n> > > > > >> - return false;\n> > > > > >> + if (!unicamDevice)\n> > > > > >> + continue;\n> > > > > >>\n> > > > > >> - isp_ = acquireMediaDevice(enumerator, isp);\n> > > > > >> - if (!isp_)\n> > > > > >> - return false;\n> > > > > >> + for (const std::string &ispIdStr : { \"0\", \"1\" }) {\n> > > >\n> > > > I'm not sure to understand why you have nested loops, shouldn't you\n> > > > acquire the unicam and ISP devices separately ?\n> > >\n> > > Each unicam instance must be matched with an isp instance, else\n> > > the camera cannot be registered.\n> > >\n> > > I used nested loops for convenience, it's not strictly necessary.\n> > > With the nested loops, I don't need to keep arrays of MediaDevice\n> > > pointers sticking around :-)\n> >\n> > If the two ISP instances are identical, it sounds like you could support\n> > this in a much simpler way, by acquiring one unicam instance and one ISP\n> > instance only in the pipeline handler. match() will be called in a loop\n> > until it returns false, so you'll have two independent pipeline handler\n> > instances, handling one camera each. Handling multiple cameras in a\n> > single pipeline handler instance is only required when cameras share\n> > resources.\n> \n> Ah, this is what I was missing! I did not realise that match() would be called\n> in a loop until it fails. So I can adjust this code to only register one camera\n> per call and things should be simpler! I'll make that change for the next\n> version of this series.\n\nYou may need to extend DeviceMatch to allow regexps, depending on how\nentities are named in the kernel.\n\n> > > > > >> + MediaDevice *ispDevice;\n> > > > > >> + int ret;\n> > > > > >> +\n> > > > > >> + DeviceMatch isp(\"bcm2835-isp\");\n> > > > > >> + isp.add(\"bcm2835-isp\" + ispIdStr + \"-output0\"); /* Input */\n> > > > > >> + isp.add(\"bcm2835-isp\" + ispIdStr + \"-capture1\"); /* Output 0 */\n> > > > > >> + isp.add(\"bcm2835-isp\" + ispIdStr + \"-capture2\"); /* Output 0 */\n> > > > > >> + isp.add(\"bcm2835-isp\" + ispIdStr + \"-capture3\"); /* Stats */\n> > > > > >> + ispDevice = acquireMediaDevice(enumerator, isp);\n> > > > > >> +\n> > > > > >> + if (!ispDevice)\n> > > > > >> + continue;\n> > > > > >> +\n> > > > > >> + ret = registerCameras(unicamDevice, ispDevice, unicamIdStr, ispIdStr);\n> > > > > >> + if (ret) {\n> > > > > >> + LOG(RPI, Error) << \"Failed to register camera: \" << ret;\n> > > > > >> + ispDevice->release();\n> > > > > >> + unicamDevice->release();\n> > > > > >\n> > > > > > I am not too sure if the above two lines are\n> > > > > > enough. PipelineHandler::acquireMediaDevice()\n> > > > > > does a MediaDevice::acquire() but also adds the media device internally to\n> > > > > > a private member:\n> > > > > >\n> > > > > > std::vector> mediaDevices_;\n> > > > > >\n> > > > > > Perhaps I ought to implement a new public\n> > > > > > function PipelineHandler::acquireMediaDevice(MediaDevice *)\n> > > > >\n> > > > > Sorry, that should read PipelineHandler::releaseMediaDevice(MediaDevice *)\n> > > > > :-(\n> > > > >\n> > > > > > that removes the entry in mediaDevices_ as well as does a\n> > > > > > MediaDevice::release(). Thoughts?\n> > > >\n> > > > The media device acquired by the pipeline handler are meant to be\n> > > > released automatically. Camera registration failure isn't supposed to\n> > > > happen, in case one camera fails to register and the other doesn't, is\n> > > > there any issue keeping the media devices for the failed camera acquired\n> > > > ?\n> > >\n> > > I suppose there is no harm in keeping the device acquired when the camera cannot\n> > > be registered, particularly as it cannot be used again. In which case I can simplify the\n> > > error path logic.\n> > >\n> > > > > >> + } else\n> > > > > >> + numCameras++;\n> > > > > >>\n> > > > > >> - return registerCameras();\n> > > > > >> + break;\n> > > > > >> + }\n> > > > > >> + }\n> > > > > >> +\n> > > > > >> + return !!numCameras;\n> > > > > >> }\n> > > > > >>\n> > > > > >> -bool PipelineHandlerRPi::registerCameras()\n> > > > > >> +int PipelineHandlerRPi::registerCameras(MediaDevice *unicam, MediaDevice *isp,\n> > > > > >> + const std::string &unicamIdStr,\n> > > > > >> + const std::string &ispIdStr)\n> > > > > >> {\n> > > > > >> std::unique_ptr data = std::make_unique(this);\n> > > > > >> +\n> > > > > >> if (!data->dmaHeap_.isValid())\n> > > > > >> - return false;\n> > > > > >> + return -ENOMEM;\n> > > > > >> +\n> > > > > >> + MediaEntity *unicamImage = unicam->getEntityByName(\"unicam\" + unicamIdStr + \"-image\");\n> > > > > >> + MediaEntity *ispOutput0 = isp->getEntityByName(\"bcm2835-isp\" + ispIdStr + \"-output0\");\n> > > > > >> + MediaEntity *ispCapture1 = isp->getEntityByName(\"bcm2835-isp\" + ispIdStr + \"-capture1\");\n> > > > > >> + MediaEntity *ispCapture2 = isp->getEntityByName(\"bcm2835-isp\" + ispIdStr + \"-capture2\");\n> > > > > >> + MediaEntity *ispCapture3 = isp->getEntityByName(\"bcm2835-isp\" + ispIdStr + \"-capture3\");\n> > > > > >> +\n> > > > > >> + if (!unicamImage || !ispOutput0 || !ispCapture1 || !ispCapture2 || !ispCapture3)\n> > > > > >> + return -ENOENT;\n> > > > > >>\n> > > > > >> /* Locate and open the unicam video streams. */\n> > > > > >> - data->unicam_[Unicam::Image] = RPi::Stream(\"Unicam Image\", unicam_->getEntityByName(\"unicam-image\"));\n> > > > > >> + data->unicam_[Unicam::Image] = RPi::Stream(\"Unicam Image\", unicamImage);\n> > > > > >>\n> > > > > >> /* An embedded data node will not be present if the sensor does not support it. */\n> > > > > >> - MediaEntity *embeddedEntity = unicam_->getEntityByName(\"unicam-embedded\");\n> > > > > >> - if (embeddedEntity) {\n> > > > > >> - data->unicam_[Unicam::Embedded] = RPi::Stream(\"Unicam Embedded\", embeddedEntity);\n> > > > > >> + MediaEntity *unicamEmbedded = unicam->getEntityByName(\"unicam\" + unicamIdStr + \"-embedded\");\n> > > > > >> + if (unicamEmbedded) {\n> > > > > >> + data->unicam_[Unicam::Embedded] = RPi::Stream(\"Unicam Embedded\", unicamEmbedded);\n> > > > > >>\n> > > > > >> data->unicam_[Unicam::Embedded].dev()->bufferReady.connect(data.get(),\n> > > > > >> &RPiCameraData::unicamBufferDequeue);\n> > > > > >> }\n> > > > > >>\n> > > > > >> /* Tag the ISP input stream as an import stream. */\n> > > > > >> - data->isp_[Isp::Input] = RPi::Stream(\"ISP Input\", isp_->getEntityByName(\"bcm2835-isp0-output0\"), true);\n> > > > > >> - data->isp_[Isp::Output0] = RPi::Stream(\"ISP Output0\", isp_->getEntityByName(\"bcm2835-isp0-capture1\"));\n> > > > > >> - data->isp_[Isp::Output1] = RPi::Stream(\"ISP Output1\", isp_->getEntityByName(\"bcm2835-isp0-capture2\"));\n> > > > > >> - data->isp_[Isp::Stats] = RPi::Stream(\"ISP Stats\", isp_->getEntityByName(\"bcm2835-isp0-capture3\"));\n> > > > > >> + data->isp_[Isp::Input] = RPi::Stream(\"ISP Input\", ispOutput0, true);\n> > > > > >> + data->isp_[Isp::Output0] = RPi::Stream(\"ISP Output0\", ispCapture1);\n> > > > > >> + data->isp_[Isp::Output1] = RPi::Stream(\"ISP Output1\", ispCapture2);\n> > > > > >> + data->isp_[Isp::Stats] = RPi::Stream(\"ISP Stats\", ispCapture3);\n> > > > > >>\n> > > > > >> /* Wire up all the buffer connections. */\n> > > > > >> data->unicam_[Unicam::Image].dev()->frameStart.connect(data.get(), &RPiCameraData::frameStarted);\n> > > > > >> @@ -1046,7 +1085,7 @@ bool PipelineHandlerRPi::registerCameras()\n> > > > > >> data->isp_[Isp::Stats].dev()->bufferReady.connect(data.get(), &RPiCameraData::ispOutputDequeue);\n> > > > > >>\n> > > > > >> /* Identify the sensor. */\n> > > > > >> - for (MediaEntity *entity : unicam_->entities()) {\n> > > > > >> + for (MediaEntity *entity : unicam->entities()) {\n> > > > > >> if (entity->function() == MEDIA_ENT_F_CAM_SENSOR) {\n> > > > > >> data->sensor_ = std::make_unique(entity);\n> > > > > >> break;\n> > > > > >> @@ -1054,23 +1093,23 @@ bool PipelineHandlerRPi::registerCameras()\n> > > > > >> }\n> > > > > >>\n> > > > > >> if (!data->sensor_)\n> > > > > >> - return false;\n> > > > > >> + return -EINVAL;\n> > > > > >>\n> > > > > >> if (data->sensor_->init())\n> > > > > >> - return false;\n> > > > > >> + return -EINVAL;\n> > > > > >>\n> > > > > >> data->sensorFormats_ = populateSensorFormats(data->sensor_);\n> > > > > >>\n> > > > > >> ipa::RPi::SensorConfig sensorConfig;\n> > > > > >> if (data->loadIPA(&sensorConfig)) {\n> > > > > >> LOG(RPI, Error) << \"Failed to load a suitable IPA library\";\n> > > > > >> - return false;\n> > > > > >> + return -EINVAL;\n> > > > > >> }\n> > > > > >>\n> > > > > >> - if (sensorConfig.sensorMetadata ^ !!embeddedEntity) {\n> > > > > >> + if (sensorConfig.sensorMetadata ^ !!unicamEmbedded) {\n> > > > > >> LOG(RPI, Warning) << \"Mismatch between Unicam and CamHelper for embedded data usage!\";\n> > > > > >> sensorConfig.sensorMetadata = false;\n> > > > > >> - if (embeddedEntity)\n> > > > > >> + if (unicamEmbedded)\n> > > > > >> data->unicam_[Unicam::Embedded].dev()->bufferReady.disconnect();\n> > > > > >> }\n> > > > > >>\n> > > > > >> @@ -1091,12 +1130,12 @@ bool PipelineHandlerRPi::registerCameras()\n> > > > > >>\n> > > > > >> for (auto stream : data->streams_) {\n> > > > > >> if (stream->dev()->open())\n> > > > > >> - return false;\n> > > > > >> + continue;\n> > > > > >> }\n> > > > > >>\n> > > > > >> if (!data->unicam_[Unicam::Image].dev()->caps().hasMediaController()) {\n> > > > > >> LOG(RPI, Error) << \"Unicam driver does not use the MediaController, please update your kernel!\";\n> > > > > >> - return false;\n> > > > > >> + return -EINVAL;\n> > > > > >> }\n> > > > > >>\n> > > > > >> /*\n> > > > > >> @@ -1158,7 +1197,7 @@ bool PipelineHandlerRPi::registerCameras()\n> > > > > >>\n> > > > > >> if (!bayerFormat.isValid()) {\n> > > > > >> LOG(RPI, Error) << \"No Bayer format found\";\n> > > > > >> - return false;\n> > > > > >> + return -EINVAL;\n> > > > > >> }\n> > > > > >> data->nativeBayerOrder_ = bayerFormat.order;\n> > > > > >>\n> > > > > >> @@ -1178,7 +1217,10 @@ bool PipelineHandlerRPi::registerCameras()\n> > > > > >> Camera::create(std::move(data), id, streams);\n> > > > > >> registerCamera(std::move(camera));\n> > > > > >>\n> > > > > >> - return true;\n> > > > > >> + LOG(RPI, Info) << \"Registered camera \" << id\n> > > > > >> + << \" to instances \\\"unicam\" << unicamIdStr << \"\\\"\"\n> > > > > >> + << \" and \\\"isp\" << ispIdStr << \"\\\"\";\n> > > > > >> + return 0;\n> > > > > >> }\n> > > > > >>\n> > > > > >> int PipelineHandlerRPi::queueAllBuffers(Camera *camera)","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 86C38BDB13\n\tfor ;\n\tSun, 21 Nov 2021 23:25:47 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 99CCA60378;\n\tMon, 22 Nov 2021 00:25:46 +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 0CE826032C\n\tfor ;\n\tMon, 22 Nov 2021 00:25:45 +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 82C76190C;\n\tMon, 22 Nov 2021 00:25:44 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"cd4jqDwG\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1637537144;\n\tbh=Qd+qZYz8wxd66s7rul/mgru/2lKSfq/OPMaWHKf0LHM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=cd4jqDwGTZkJTgztIT+zUpicXJPMSUGwfAfahK6ePgB6Sok925h5ZO+Bo5T79gX/8\n\tELG6wlBnm5GVVfrMD4f+Zmbq2XBprz89MhBfvcz7Me8N8Y/qBF4c2QZVYsNUBoask3\n\txd4l52LxJbat+gdJ52WfsIt3gU1vdKIT1N5PY630=","Date":"Mon, 22 Nov 2021 01:25:21 +0200","From":"Laurent Pinchart ","To":"Naushir Patuck ","Message-ID":"","References":"<20211119110955.3137585-1-naush@raspberrypi.com>\n\t<20211119110955.3137585-2-naush@raspberrypi.com>\n\t\n\t\n\t\n\t\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 2/3] pipeline: raspberrypi: Allow\n\tregistration of multiple cameras","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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":21094,"web_url":"https://patchwork.libcamera.org/comment/21094/","msgid":"","date":"2021-11-22T08:42:45","subject":"Re: [libcamera-devel] [PATCH 2/3] pipeline: raspberrypi: Allow\n\tregistration of multiple cameras","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Laurent,\n\nOn Sun, 21 Nov 2021 at 23:25, Laurent Pinchart <\nlaurent.pinchart@ideasonboard.com> wrote:\n\n> Hi Naush,\n>\n> On Sat, Nov 20, 2021 at 07:28:19AM +0000, Naushir Patuck wrote:\n> > On Fri, 19 Nov 2021 at 20:53, Laurent Pinchart wrote:\n> > > On Fri, Nov 19, 2021 at 04:45:27PM +0000, Naushir Patuck wrote:\n> > > > On Fri, 19 Nov 2021 at 13:03, Laurent Pinchart wrote:\n> > > > > On Fri, Nov 19, 2021 at 11:28:39AM +0000, Naushir Patuck wrote:\n> > > > > > On Fri, 19 Nov 2021 at 11:27, Naushir Patuck wrote:\n> > > > > > > On Fri, 19 Nov 2021 at 11:10, Naushir Patuck wrote:\n> > > > > > >\n> > > > > > >> Expand the pipeline handler camera registration to correctly\n> handle multiple\n> > > > > > >> cameras attached to the platform. For example, Raspberry Pi\n> Compute Module\n> > > > > > >> platforms have two camera connectors, and this change would\n> allow the user to\n> > > > > > >> select either of the two cameras to run.\n> > > > >\n> > > > > That's interesting :-)\n> > > > >\n> > > > > > >> There are associated kernel driver changes for both Unicam\n> and the ISP needed\n> > > > > > >> to correctly advertise multiple media devices and nodes for\n> multi-camera usage:\n> > > > > > >>\n> > > > > > >> https://github.com/raspberrypi/linux/pull/4140\n> > > > > > >> https://github.com/raspberrypi/linux/pull/4709\n> > > > >\n> > > > > There's still a single ISP instance. How does it work at the\n> > > > > hardware/firmware level ? The kernel patch seems simple, how are\n> ISP\n> > > > > operations queued by users scheduled and arbitrated ? Can the two\n> > > > > cameras be used concurrently, or are they mutually exclusive ?\n> > > >\n> > > > The isp driver change will allow concurrent usage of the ISP\n> hardware.\n> > > > What's missing from your view is the shim in the firmware that\n> handles\n> > > > the arbitration between the many users ;-) This makes adding\n> multi-user\n> > > > to the kernel driver mostly a duplication of the single-user\n> instance.\n> > >\n> > > Are there advantages in doing so in the firmware instead of in the\n> Linux\n> > > kernel driver, or even in userspace in libcamera ? I suppose that if\n> > > it's there already it's the simplest way forward, I'm not requesting a\n> > > change here, but I'm wondering what the design rationale is.\n> >\n> > The firmware already has support for concurrent usage as we need\n> > it to share the ISP HW with the codec driver where ISP is used to\n> generate\n> > a format that our codec block understands.\n> >\n> > Another advantage is that it would be easier to context switch between\n> frames\n> > on a stripe level rather than at frame level. This could be duplicated\n> in the\n> > kernel driver but would require much more effort.\n> >\n> > > How should libcamera cope with limitations in terms of ISP bandwidth\n> > > with multiple instances ?\n> >\n> > That's a good question, and I am not sure of the answer. Right now, our\n> > pipeline handler will simply drop frames when it cannot keep up in the\n> HW.\n> > It would be easy enough to calculate expected throughput in the pipeline\n> handler\n> > instance, but how do we know if/when other users outside of libcamera\n> > are also using the ISP?\n>\n> If multiplexing is handled in the firmware, we'd need an API to\n> negotiate bandwidth allocation with the firmware I suppose.\n>\n\nWhat do you think would be an acceptable failure route if the bandwidth\nallocation exceeds the HW limitations? Should the use-case fail straight\naway,\nor do we throw a warning and drop frames gracefully if/when it happens.\nWe currently do the latter.\n\n\n>\n> > > > As an aside, I have run into a few issues with actual concurrent\n> usage\n> > > > in the pipeline handlers. I've been talking with Jacopo offline\n> about\n> > > > this, but we can keep that discussion as a separate thread to this\n> work.\n> > >\n> > >\n> https://git.libcamera.org/libcamera/pinchartl/libcamera.git/log/?h=mtk/multi-cam\n> > > may help already. We'll also need an API for pipeline handlers to\n> report\n> > > mutual exclusion constraints between camera, and a way to expose that\n> to\n> > > applications.\n> > >\n> > > > > > >> However, this change is backward compatible with kernel\n> builds that do not have\n> > > > > > >> these changes for standard single camera usage.\n> > > > > > >>\n> > > > > > >> Signed-off-by: Naushir Patuck \n> > > > > > >> ---\n> > > > > > >> .../pipeline/raspberrypi/raspberrypi.cpp | 112\n> ++++++++++++------\n> > > > > > >> 1 file changed, 77 insertions(+), 35 deletions(-)\n> > > > > > >>\n> > > > > > >> diff --git\n> a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > > > >> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > > > >> index 9aa7e9eef5e7..8ed2ebcaafe7 100644\n> > > > > > >> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > > > >> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > > > >> @@ -311,7 +311,8 @@ private:\n> > > > > > >> return static_cast *>(camera->_d());\n> > > > > > >> }\n> > > > > > >>\n> > > > > > >> - bool registerCameras();\n> > > > > > >> + int registerCameras(MediaDevice *unicam, MediaDevice\n> *isp,\n> > > > > > >> + const std::string &unicamIdStr,\n> const std::string &ispIdStr);\n> > > > > > >> int queueAllBuffers(Camera *camera);\n> > > > > > >> int prepareBuffers(Camera *camera);\n> > > > > > >> void freeBuffers(Camera *camera);\n> > > > > > >> @@ -993,49 +994,87 @@ int\n> PipelineHandlerRPi::queueRequestDevice(Camera *camera, Request *request)\n> > > > > > >>\n> > > > > > >> bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)\n> > > > > > >> {\n> > > > > > >> - DeviceMatch unicam(\"unicam\");\n> > > > > > >> - DeviceMatch isp(\"bcm2835-isp\");\n> > > > > > >> + unsigned int numCameras = 0;\n> > > > > > >>\n> > > > > > >> - unicam.add(\"unicam-image\");\n> > > > > > >> + /*\n> > > > > > >> + * String of indexes to append to the entity names\n> when searching for\n> > > > > > >> + * the Unican media devices. The first string is\n> empty (un-indexed) to\n> > > > > > >> + * to maintain backward compatability with old\n> versions of the Unicam\n> > > > > > >> + * kernel driver that did not advertise instance\n> indexes.\n> > > > > > >> + */\n> > > > > > >> + for (const std::string &unicamIdStr : { \"\", \"0\", \"1\"\n> }) {\n> > > > > > >> + MediaDevice *unicamDevice;\n> > > > > > >>\n> > > > > > >> - isp.add(\"bcm2835-isp0-output0\"); /* Input */\n> > > > > > >> - isp.add(\"bcm2835-isp0-capture1\"); /* Output 0 */\n> > > > > > >> - isp.add(\"bcm2835-isp0-capture2\"); /* Output 1 */\n> > > > > > >> - isp.add(\"bcm2835-isp0-capture3\"); /* Stats */\n> > > > > > >> + DeviceMatch unicam(\"unicam\");\n> > > > > > >> + unicam.add(\"unicam\" + unicamIdStr + \"-image\");\n> > > > > > >> + unicamDevice = acquireMediaDevice(enumerator,\n> unicam);\n> > > > > > >>\n> > > > > > >> - unicam_ = acquireMediaDevice(enumerator, unicam);\n> > > > > > >> - if (!unicam_)\n> > > > > > >> - return false;\n> > > > > > >> + if (!unicamDevice)\n> > > > > > >> + continue;\n> > > > > > >>\n> > > > > > >> - isp_ = acquireMediaDevice(enumerator, isp);\n> > > > > > >> - if (!isp_)\n> > > > > > >> - return false;\n> > > > > > >> + for (const std::string &ispIdStr : { \"0\", \"1\"\n> }) {\n> > > > >\n> > > > > I'm not sure to understand why you have nested loops, shouldn't you\n> > > > > acquire the unicam and ISP devices separately ?\n> > > >\n> > > > Each unicam instance must be matched with an isp instance, else\n> > > > the camera cannot be registered.\n> > > >\n> > > > I used nested loops for convenience, it's not strictly necessary.\n> > > > With the nested loops, I don't need to keep arrays of MediaDevice\n> > > > pointers sticking around :-)\n> > >\n> > > If the two ISP instances are identical, it sounds like you could\n> support\n> > > this in a much simpler way, by acquiring one unicam instance and one\n> ISP\n> > > instance only in the pipeline handler. match() will be called in a loop\n> > > until it returns false, so you'll have two independent pipeline handler\n> > > instances, handling one camera each. Handling multiple cameras in a\n> > > single pipeline handler instance is only required when cameras share\n> > > resources.\n> >\n> > Ah, this is what I was missing! I did not realise that match() would be\n> called\n> > in a loop until it fails. So I can adjust this code to only register\n> one camera\n> > per call and things should be simpler! I'll make that change for the\n> next\n> > version of this series.\n>\n> You may need to extend DeviceMatch to allow regexps, depending on how\n> entities are named in the kernel.\n>\n\nI think this is not needed for our implementation, but will put a new\nversion\ntogether with my intended changes and reassess.\n\nRegards,\nNaush\n\n\n\n>\n> > > > > > >> + MediaDevice *ispDevice;\n> > > > > > >> + int ret;\n> > > > > > >> +\n> > > > > > >> + DeviceMatch isp(\"bcm2835-isp\");\n> > > > > > >> + isp.add(\"bcm2835-isp\" + ispIdStr +\n> \"-output0\"); /* Input */\n> > > > > > >> + isp.add(\"bcm2835-isp\" + ispIdStr +\n> \"-capture1\"); /* Output 0 */\n> > > > > > >> + isp.add(\"bcm2835-isp\" + ispIdStr +\n> \"-capture2\"); /* Output 0 */\n> > > > > > >> + isp.add(\"bcm2835-isp\" + ispIdStr +\n> \"-capture3\"); /* Stats */\n> > > > > > >> + ispDevice =\n> acquireMediaDevice(enumerator, isp);\n> > > > > > >> +\n> > > > > > >> + if (!ispDevice)\n> > > > > > >> + continue;\n> > > > > > >> +\n> > > > > > >> + ret = registerCameras(unicamDevice,\n> ispDevice, unicamIdStr, ispIdStr);\n> > > > > > >> + if (ret) {\n> > > > > > >> + LOG(RPI, Error) << \"Failed to\n> register camera: \" << ret;\n> > > > > > >> + ispDevice->release();\n> > > > > > >> + unicamDevice->release();\n> > > > > > >\n> > > > > > > I am not too sure if the above two lines are\n> > > > > > > enough. PipelineHandler::acquireMediaDevice()\n> > > > > > > does a MediaDevice::acquire() but also adds the media device\n> internally to\n> > > > > > > a private member:\n> > > > > > >\n> > > > > > > std::vector> mediaDevices_;\n> > > > > > >\n> > > > > > > Perhaps I ought to implement a new public\n> > > > > > > function PipelineHandler::acquireMediaDevice(MediaDevice *)\n> > > > > >\n> > > > > > Sorry, that should read\n> PipelineHandler::releaseMediaDevice(MediaDevice *)\n> > > > > > :-(\n> > > > > >\n> > > > > > > that removes the entry in mediaDevices_ as well as does a\n> > > > > > > MediaDevice::release(). Thoughts?\n> > > > >\n> > > > > The media device acquired by the pipeline handler are meant to be\n> > > > > released automatically. Camera registration failure isn't supposed\n> to\n> > > > > happen, in case one camera fails to register and the other\n> doesn't, is\n> > > > > there any issue keeping the media devices for the failed camera\n> acquired\n> > > > > ?\n> > > >\n> > > > I suppose there is no harm in keeping the device acquired when the\n> camera cannot\n> > > > be registered, particularly as it cannot be used again. In which\n> case I can simplify the\n> > > > error path logic.\n> > > >\n> > > > > > >> + } else\n> > > > > > >> + numCameras++;\n> > > > > > >>\n> > > > > > >> - return registerCameras();\n> > > > > > >> + break;\n> > > > > > >> + }\n> > > > > > >> + }\n> > > > > > >> +\n> > > > > > >> + return !!numCameras;\n> > > > > > >> }\n> > > > > > >>\n> > > > > > >> -bool PipelineHandlerRPi::registerCameras()\n> > > > > > >> +int PipelineHandlerRPi::registerCameras(MediaDevice *unicam,\n> MediaDevice *isp,\n> > > > > > >> + const std::string\n> &unicamIdStr,\n> > > > > > >> + const std::string\n> &ispIdStr)\n> > > > > > >> {\n> > > > > > >> std::unique_ptr data =\n> std::make_unique(this);\n> > > > > > >> +\n> > > > > > >> if (!data->dmaHeap_.isValid())\n> > > > > > >> - return false;\n> > > > > > >> + return -ENOMEM;\n> > > > > > >> +\n> > > > > > >> + MediaEntity *unicamImage =\n> unicam->getEntityByName(\"unicam\" + unicamIdStr + \"-image\");\n> > > > > > >> + MediaEntity *ispOutput0 =\n> isp->getEntityByName(\"bcm2835-isp\" + ispIdStr + \"-output0\");\n> > > > > > >> + MediaEntity *ispCapture1 =\n> isp->getEntityByName(\"bcm2835-isp\" + ispIdStr + \"-capture1\");\n> > > > > > >> + MediaEntity *ispCapture2 =\n> isp->getEntityByName(\"bcm2835-isp\" + ispIdStr + \"-capture2\");\n> > > > > > >> + MediaEntity *ispCapture3 =\n> isp->getEntityByName(\"bcm2835-isp\" + ispIdStr + \"-capture3\");\n> > > > > > >> +\n> > > > > > >> + if (!unicamImage || !ispOutput0 || !ispCapture1 ||\n> !ispCapture2 || !ispCapture3)\n> > > > > > >> + return -ENOENT;\n> > > > > > >>\n> > > > > > >> /* Locate and open the unicam video streams. */\n> > > > > > >> - data->unicam_[Unicam::Image] = RPi::Stream(\"Unicam\n> Image\", unicam_->getEntityByName(\"unicam-image\"));\n> > > > > > >> + data->unicam_[Unicam::Image] = RPi::Stream(\"Unicam\n> Image\", unicamImage);\n> > > > > > >>\n> > > > > > >> /* An embedded data node will not be present if the\n> sensor does not support it. */\n> > > > > > >> - MediaEntity *embeddedEntity =\n> unicam_->getEntityByName(\"unicam-embedded\");\n> > > > > > >> - if (embeddedEntity) {\n> > > > > > >> - data->unicam_[Unicam::Embedded] =\n> RPi::Stream(\"Unicam Embedded\", embeddedEntity);\n> > > > > > >> + MediaEntity *unicamEmbedded =\n> unicam->getEntityByName(\"unicam\" + unicamIdStr + \"-embedded\");\n> > > > > > >> + if (unicamEmbedded) {\n> > > > > > >> + data->unicam_[Unicam::Embedded] =\n> RPi::Stream(\"Unicam Embedded\", unicamEmbedded);\n> > > > > > >>\n> > > > > > >>\n> data->unicam_[Unicam::Embedded].dev()->bufferReady.connect(data.get(),\n> > > > > > >> &RPiCameraData::unicamBufferDequeue);\n> > > > > > >> }\n> > > > > > >>\n> > > > > > >> /* Tag the ISP input stream as an import stream. */\n> > > > > > >> - data->isp_[Isp::Input] = RPi::Stream(\"ISP Input\",\n> isp_->getEntityByName(\"bcm2835-isp0-output0\"), true);\n> > > > > > >> - data->isp_[Isp::Output0] = RPi::Stream(\"ISP Output0\",\n> isp_->getEntityByName(\"bcm2835-isp0-capture1\"));\n> > > > > > >> - data->isp_[Isp::Output1] = RPi::Stream(\"ISP Output1\",\n> isp_->getEntityByName(\"bcm2835-isp0-capture2\"));\n> > > > > > >> - data->isp_[Isp::Stats] = RPi::Stream(\"ISP Stats\",\n> isp_->getEntityByName(\"bcm2835-isp0-capture3\"));\n> > > > > > >> + data->isp_[Isp::Input] = RPi::Stream(\"ISP Input\",\n> ispOutput0, true);\n> > > > > > >> + data->isp_[Isp::Output0] = RPi::Stream(\"ISP Output0\",\n> ispCapture1);\n> > > > > > >> + data->isp_[Isp::Output1] = RPi::Stream(\"ISP Output1\",\n> ispCapture2);\n> > > > > > >> + data->isp_[Isp::Stats] = RPi::Stream(\"ISP Stats\",\n> ispCapture3);\n> > > > > > >>\n> > > > > > >> /* Wire up all the buffer connections. */\n> > > > > > >>\n> data->unicam_[Unicam::Image].dev()->frameStart.connect(data.get(),\n> &RPiCameraData::frameStarted);\n> > > > > > >> @@ -1046,7 +1085,7 @@ bool\n> PipelineHandlerRPi::registerCameras()\n> > > > > > >>\n> data->isp_[Isp::Stats].dev()->bufferReady.connect(data.get(),\n> &RPiCameraData::ispOutputDequeue);\n> > > > > > >>\n> > > > > > >> /* Identify the sensor. */\n> > > > > > >> - for (MediaEntity *entity : unicam_->entities()) {\n> > > > > > >> + for (MediaEntity *entity : unicam->entities()) {\n> > > > > > >> if (entity->function() ==\n> MEDIA_ENT_F_CAM_SENSOR) {\n> > > > > > >> data->sensor_ =\n> std::make_unique(entity);\n> > > > > > >> break;\n> > > > > > >> @@ -1054,23 +1093,23 @@ bool\n> PipelineHandlerRPi::registerCameras()\n> > > > > > >> }\n> > > > > > >>\n> > > > > > >> if (!data->sensor_)\n> > > > > > >> - return false;\n> > > > > > >> + return -EINVAL;\n> > > > > > >>\n> > > > > > >> if (data->sensor_->init())\n> > > > > > >> - return false;\n> > > > > > >> + return -EINVAL;\n> > > > > > >>\n> > > > > > >> data->sensorFormats_ =\n> populateSensorFormats(data->sensor_);\n> > > > > > >>\n> > > > > > >> ipa::RPi::SensorConfig sensorConfig;\n> > > > > > >> if (data->loadIPA(&sensorConfig)) {\n> > > > > > >> LOG(RPI, Error) << \"Failed to load a suitable\n> IPA library\";\n> > > > > > >> - return false;\n> > > > > > >> + return -EINVAL;\n> > > > > > >> }\n> > > > > > >>\n> > > > > > >> - if (sensorConfig.sensorMetadata ^ !!embeddedEntity) {\n> > > > > > >> + if (sensorConfig.sensorMetadata ^ !!unicamEmbedded) {\n> > > > > > >> LOG(RPI, Warning) << \"Mismatch between Unicam\n> and CamHelper for embedded data usage!\";\n> > > > > > >> sensorConfig.sensorMetadata = false;\n> > > > > > >> - if (embeddedEntity)\n> > > > > > >> + if (unicamEmbedded)\n> > > > > > >>\n> data->unicam_[Unicam::Embedded].dev()->bufferReady.disconnect();\n> > > > > > >> }\n> > > > > > >>\n> > > > > > >> @@ -1091,12 +1130,12 @@ bool\n> PipelineHandlerRPi::registerCameras()\n> > > > > > >>\n> > > > > > >> for (auto stream : data->streams_) {\n> > > > > > >> if (stream->dev()->open())\n> > > > > > >> - return false;\n> > > > > > >> + continue;\n> > > > > > >> }\n> > > > > > >>\n> > > > > > >> if\n> (!data->unicam_[Unicam::Image].dev()->caps().hasMediaController()) {\n> > > > > > >> LOG(RPI, Error) << \"Unicam driver does not\n> use the MediaController, please update your kernel!\";\n> > > > > > >> - return false;\n> > > > > > >> + return -EINVAL;\n> > > > > > >> }\n> > > > > > >>\n> > > > > > >> /*\n> > > > > > >> @@ -1158,7 +1197,7 @@ bool\n> PipelineHandlerRPi::registerCameras()\n> > > > > > >>\n> > > > > > >> if (!bayerFormat.isValid()) {\n> > > > > > >> LOG(RPI, Error) << \"No Bayer format found\";\n> > > > > > >> - return false;\n> > > > > > >> + return -EINVAL;\n> > > > > > >> }\n> > > > > > >> data->nativeBayerOrder_ = bayerFormat.order;\n> > > > > > >>\n> > > > > > >> @@ -1178,7 +1217,10 @@ bool\n> PipelineHandlerRPi::registerCameras()\n> > > > > > >> Camera::create(std::move(data), id, streams);\n> > > > > > >> registerCamera(std::move(camera));\n> > > > > > >>\n> > > > > > >> - return true;\n> > > > > > >> + LOG(RPI, Info) << \"Registered camera \" << id\n> > > > > > >> + << \" to instances \\\"unicam\" <<\n> unicamIdStr << \"\\\"\"\n> > > > > > >> + << \" and \\\"isp\" << ispIdStr << \"\\\"\";\n> > > > > > >> + return 0;\n> > > > > > >> }\n> > > > > > >>\n> > > > > > >> int PipelineHandlerRPi::queueAllBuffers(Camera *camera)\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 07425BDB13\n\tfor ;\n\tMon, 22 Nov 2021 08:43:06 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CCA5560376;\n\tMon, 22 Nov 2021 09:43:04 +0100 (CET)","from mail-lf1-x133.google.com (mail-lf1-x133.google.com\n\t[IPv6:2a00:1450:4864:20::133])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 17DE660120\n\tfor ;\n\tMon, 22 Nov 2021 09:43:02 +0100 (CET)","by mail-lf1-x133.google.com with SMTP id f18so77308361lfv.6\n\tfor ;\n\tMon, 22 Nov 2021 00:43:02 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"DhY14rVt\"; 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=ljE5CFumu2NOg0iQQA5RPXUJGbfwEAuor4YW6jpMuow=;\n\tb=DhY14rVt32K0uw3/G+bnHa39gRXTd6HInMbZ8CnEJLF+CA7aSJF8EPBTAog1BoCda9\n\tqghTATQfgw1qpngjHy9+puwpRM6/83oOtaFwlchHvt8EMZEh1hSEVSzl8c3R7Fm/mkk/\n\tYs6yZGD2HM4BFA6LdB+YnFMqapbDs2+LE3w9fvMomMxgbiqjLnZWNcRybkYgB1f1zsij\n\tQ94RSlEcM/CVaDyTCiC8tTlRMqFJgV86NgORb786MxNZX99le3DzAPjGMgG51aZ0lHGY\n\tWR11bhkpDg3qyRGfeapa2i+2DlN67inP0RGO5YRXGgAZ9nIoEfdiP0tEdeq2YSqmXvrI\n\tAEfg==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=ljE5CFumu2NOg0iQQA5RPXUJGbfwEAuor4YW6jpMuow=;\n\tb=jtCVRkDCSbAIUWtABJMcZLZk4dinEQeyLo4yMCblHxhGBrGca/GxeqbxPC4N7bhvxf\n\t0QQ4tNLcWPp0MQBEVoCb7zSwtYpVOB2AXdgwelEH4l5tUeAP7uJQpO25Mc43sIRiu/UN\n\tMdmUA3v1DGkvrH590ZSpE7Vvr2bELiAEchCrncHTaPTT99ILYewjwG5fj7vYzelrUS31\n\t5+iaoyRPwlWncYxdNdLmL5GFBrwZ6fa349YsoSU4xVw/it82aayP1OaS62E7CNiE/e9w\n\tuNH+zKcD+U5DuRwHxVS+swE9sbHOzNpnkUoo9ryuY0aYKBBXwBIFgB1m5UZer6riKsh8\n\topOg==","X-Gm-Message-State":"AOAM531HhT6UaaxEUy/BM8ZSLocqokfE/CRkQcvP4A0OYJryfq/ACdcc\n\tdmbuHBIzwkN6bTKfczitfvJEHALqHnb4eoE8wspZk22nIJ7fjQ==","X-Google-Smtp-Source":"ABdhPJyCHRU7NRDBJ/D/efHtBqrk46tsLHrXsZradRqDA+DcFkcJ40L8vJHgXCpnBmgRg9PjYaxwaBca9Ke2gxOdXF0=","X-Received":"by 2002:a05:6512:39c4:: with SMTP id\n\tk4mr55392181lfu.79.1637570580974; \n\tMon, 22 Nov 2021 00:43:00 -0800 (PST)","MIME-Version":"1.0","References":"<20211119110955.3137585-1-naush@raspberrypi.com>\n\t<20211119110955.3137585-2-naush@raspberrypi.com>\n\t\n\t\n\t\n\t\n\t\n\t\n\t","In-Reply-To":"","From":"Naushir Patuck ","Date":"Mon, 22 Nov 2021 08:42:45 +0000","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"multipart/alternative; boundary=\"000000000000c623f605d15c9c9b\"","Subject":"Re: [libcamera-devel] [PATCH 2/3] pipeline: raspberrypi: Allow\n\tregistration of multiple cameras","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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":21105,"web_url":"https://patchwork.libcamera.org/comment/21105/","msgid":"","date":"2021-11-22T23:51:12","subject":"Re: [libcamera-devel] [PATCH 2/3] pipeline: raspberrypi: Allow\n\tregistration of multiple cameras","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Naush,\n\nOn Mon, Nov 22, 2021 at 08:42:45AM +0000, Naushir Patuck wrote:\n> On Sun, 21 Nov 2021 at 23:25, Laurent Pinchart wrote:\n> > On Sat, Nov 20, 2021 at 07:28:19AM +0000, Naushir Patuck wrote:\n> > > On Fri, 19 Nov 2021 at 20:53, Laurent Pinchart wrote:\n> > > > On Fri, Nov 19, 2021 at 04:45:27PM +0000, Naushir Patuck wrote:\n> > > > > On Fri, 19 Nov 2021 at 13:03, Laurent Pinchart wrote:\n> > > > > > On Fri, Nov 19, 2021 at 11:28:39AM +0000, Naushir Patuck wrote:\n> > > > > > > On Fri, 19 Nov 2021 at 11:27, Naushir Patuck wrote:\n> > > > > > > > On Fri, 19 Nov 2021 at 11:10, Naushir Patuck wrote:\n> > > > > > > >\n> > > > > > > >> Expand the pipeline handler camera registration to correctly handle multiple\n> > > > > > > >> cameras attached to the platform. For example, Raspberry Pi Compute Module\n> > > > > > > >> platforms have two camera connectors, and this change would allow the user to\n> > > > > > > >> select either of the two cameras to run.\n> > > > > >\n> > > > > > That's interesting :-)\n> > > > > >\n> > > > > > > >> There are associated kernel driver changes for both Unicam and the ISP needed\n> > > > > > > >> to correctly advertise multiple media devices and nodes for multi-camera usage:\n> > > > > > > >>\n> > > > > > > >> https://github.com/raspberrypi/linux/pull/4140\n> > > > > > > >> https://github.com/raspberrypi/linux/pull/4709\n> > > > > >\n> > > > > > There's still a single ISP instance. How does it work at the\n> > > > > > hardware/firmware level ? The kernel patch seems simple, how are ISP\n> > > > > > operations queued by users scheduled and arbitrated ? Can the two\n> > > > > > cameras be used concurrently, or are they mutually exclusive ?\n> > > > >\n> > > > > The isp driver change will allow concurrent usage of the ISP hardware.\n> > > > > What's missing from your view is the shim in the firmware that handles\n> > > > > the arbitration between the many users ;-) This makes adding multi-user\n> > > > > to the kernel driver mostly a duplication of the single-user instance.\n> > > >\n> > > > Are there advantages in doing so in the firmware instead of in the Linux\n> > > > kernel driver, or even in userspace in libcamera ? I suppose that if\n> > > > it's there already it's the simplest way forward, I'm not requesting a\n> > > > change here, but I'm wondering what the design rationale is.\n> > >\n> > > The firmware already has support for concurrent usage as we need\n> > > it to share the ISP HW with the codec driver where ISP is used to generate\n> > > a format that our codec block understands.\n> > >\n> > > Another advantage is that it would be easier to context switch between frames\n> > > on a stripe level rather than at frame level. This could be duplicated in the\n> > > kernel driver but would require much more effort.\n> > >\n> > > > How should libcamera cope with limitations in terms of ISP bandwidth\n> > > > with multiple instances ?\n> > >\n> > > That's a good question, and I am not sure of the answer. Right now, our\n> > > pipeline handler will simply drop frames when it cannot keep up in the HW.\n> > > It would be easy enough to calculate expected throughput in the pipeline handler\n> > > instance, but how do we know if/when other users outside of libcamera\n> > > are also using the ISP?\n> >\n> > If multiplexing is handled in the firmware, we'd need an API to\n> > negotiate bandwidth allocation with the firmware I suppose.\n> \n> What do you think would be an acceptable failure route if the bandwidth\n> allocation exceeds the HW limitations? Should the use-case fail straight away,\n> or do we throw a warning and drop frames gracefully if/when it happens.\n> We currently do the latter.\n\nThat's a good question, and I'm not sure what the best option would be.\nIt can be nice for users to be informed of issues (especially when\nthey'll try to figure out why the frame rate is lower than expecting),\nbut at the same time, we shouldn't prevent this degraded mode from being\nused as it may be good enough for some use cases.\n\n> > > > > As an aside, I have run into a few issues with actual concurrent usage\n> > > > > in the pipeline handlers. I've been talking with Jacopo offline about\n> > > > > this, but we can keep that discussion as a separate thread to this work.\n> > > >\n> > > > https://git.libcamera.org/libcamera/pinchartl/libcamera.git/log/?h=mtk/multi-cam\n> > > > may help already. We'll also need an API for pipeline handlers to report\n> > > > mutual exclusion constraints between camera, and a way to expose that to\n> > > > applications.\n> > > >\n> > > > > > > >> However, this change is backward compatible with kernel builds that do not have\n> > > > > > > >> these changes for standard single camera usage.\n> > > > > > > >>\n> > > > > > > >> Signed-off-by: Naushir Patuck \n> > > > > > > >> ---\n> > > > > > > >> .../pipeline/raspberrypi/raspberrypi.cpp | 112 ++++++++++++------\n> > > > > > > >> 1 file changed, 77 insertions(+), 35 deletions(-)\n> > > > > > > >>\n> > > > > > > >> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > > > > >> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > > > > >> index 9aa7e9eef5e7..8ed2ebcaafe7 100644\n> > > > > > > >> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > > > > >> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > > > > >> @@ -311,7 +311,8 @@ private:\n> > > > > > > >> return static_cast(camera->_d());\n> > > > > > > >> }\n> > > > > > > >>\n> > > > > > > >> - bool registerCameras();\n> > > > > > > >> + int registerCameras(MediaDevice *unicam, MediaDevice *isp,\n> > > > > > > >> + const std::string &unicamIdStr, const std::string &ispIdStr);\n> > > > > > > >> int queueAllBuffers(Camera *camera);\n> > > > > > > >> int prepareBuffers(Camera *camera);\n> > > > > > > >> void freeBuffers(Camera *camera);\n> > > > > > > >> @@ -993,49 +994,87 @@ int PipelineHandlerRPi::queueRequestDevice(Camera *camera, Request *request)\n> > > > > > > >>\n> > > > > > > >> bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)\n> > > > > > > >> {\n> > > > > > > >> - DeviceMatch unicam(\"unicam\");\n> > > > > > > >> - DeviceMatch isp(\"bcm2835-isp\");\n> > > > > > > >> + unsigned int numCameras = 0;\n> > > > > > > >>\n> > > > > > > >> - unicam.add(\"unicam-image\");\n> > > > > > > >> + /*\n> > > > > > > >> + * String of indexes to append to the entity names when searching for\n> > > > > > > >> + * the Unican media devices. The first string is empty (un-indexed) to\n> > > > > > > >> + * to maintain backward compatability with old versions of the Unicam\n> > > > > > > >> + * kernel driver that did not advertise instance indexes.\n> > > > > > > >> + */\n> > > > > > > >> + for (const std::string &unicamIdStr : { \"\", \"0\", \"1\" }) {\n> > > > > > > >> + MediaDevice *unicamDevice;\n> > > > > > > >>\n> > > > > > > >> - isp.add(\"bcm2835-isp0-output0\"); /* Input */\n> > > > > > > >> - isp.add(\"bcm2835-isp0-capture1\"); /* Output 0 */\n> > > > > > > >> - isp.add(\"bcm2835-isp0-capture2\"); /* Output 1 */\n> > > > > > > >> - isp.add(\"bcm2835-isp0-capture3\"); /* Stats */\n> > > > > > > >> + DeviceMatch unicam(\"unicam\");\n> > > > > > > >> + unicam.add(\"unicam\" + unicamIdStr + \"-image\");\n> > > > > > > >> + unicamDevice = acquireMediaDevice(enumerator, unicam);\n> > > > > > > >>\n> > > > > > > >> - unicam_ = acquireMediaDevice(enumerator, unicam);\n> > > > > > > >> - if (!unicam_)\n> > > > > > > >> - return false;\n> > > > > > > >> + if (!unicamDevice)\n> > > > > > > >> + continue;\n> > > > > > > >>\n> > > > > > > >> - isp_ = acquireMediaDevice(enumerator, isp);\n> > > > > > > >> - if (!isp_)\n> > > > > > > >> - return false;\n> > > > > > > >> + for (const std::string &ispIdStr : { \"0\", \"1\" }) {\n> > > > > >\n> > > > > > I'm not sure to understand why you have nested loops, shouldn't you\n> > > > > > acquire the unicam and ISP devices separately ?\n> > > > >\n> > > > > Each unicam instance must be matched with an isp instance, else\n> > > > > the camera cannot be registered.\n> > > > >\n> > > > > I used nested loops for convenience, it's not strictly necessary.\n> > > > > With the nested loops, I don't need to keep arrays of MediaDevice\n> > > > > pointers sticking around :-)\n> > > >\n> > > > If the two ISP instances are identical, it sounds like you could support\n> > > > this in a much simpler way, by acquiring one unicam instance and one ISP\n> > > > instance only in the pipeline handler. match() will be called in a loop\n> > > > until it returns false, so you'll have two independent pipeline handler\n> > > > instances, handling one camera each. Handling multiple cameras in a\n> > > > single pipeline handler instance is only required when cameras share\n> > > > resources.\n> > >\n> > > Ah, this is what I was missing! I did not realise that match() would be called\n> > > in a loop until it fails. So I can adjust this code to only register one camera\n> > > per call and things should be simpler! I'll make that change for the next\n> > > version of this series.\n> >\n> > You may need to extend DeviceMatch to allow regexps, depending on how\n> > entities are named in the kernel.\n> \n> I think this is not needed for our implementation, but will put a new version\n> together with my intended changes and reassess.\n> \n> > > > > > > >> + MediaDevice *ispDevice;\n> > > > > > > >> + int ret;\n> > > > > > > >> +\n> > > > > > > >> + DeviceMatch isp(\"bcm2835-isp\");\n> > > > > > > >> + isp.add(\"bcm2835-isp\" + ispIdStr + \"-output0\"); /* Input */\n> > > > > > > >> + isp.add(\"bcm2835-isp\" + ispIdStr + \"-capture1\"); /* Output 0 */\n> > > > > > > >> + isp.add(\"bcm2835-isp\" + ispIdStr + \"-capture2\"); /* Output 0 */\n> > > > > > > >> + isp.add(\"bcm2835-isp\" + ispIdStr + \"-capture3\"); /* Stats */\n> > > > > > > >> + ispDevice = acquireMediaDevice(enumerator, isp);\n> > > > > > > >> +\n> > > > > > > >> + if (!ispDevice)\n> > > > > > > >> + continue;\n> > > > > > > >> +\n> > > > > > > >> + ret = registerCameras(unicamDevice, ispDevice, unicamIdStr, ispIdStr);\n> > > > > > > >> + if (ret) {\n> > > > > > > >> + LOG(RPI, Error) << \"Failed to register camera: \" << ret;\n> > > > > > > >> + ispDevice->release();\n> > > > > > > >> + unicamDevice->release();\n> > > > > > > >\n> > > > > > > > I am not too sure if the above two lines are\n> > > > > > > > enough. PipelineHandler::acquireMediaDevice()\n> > > > > > > > does a MediaDevice::acquire() but also adds the media device internally to\n> > > > > > > > a private member:\n> > > > > > > >\n> > > > > > > > std::vector> mediaDevices_;\n> > > > > > > >\n> > > > > > > > Perhaps I ought to implement a new public\n> > > > > > > > function PipelineHandler::acquireMediaDevice(MediaDevice *)\n> > > > > > >\n> > > > > > > Sorry, that should read PipelineHandler::releaseMediaDevice(MediaDevice *)\n> > > > > > > :-(\n> > > > > > >\n> > > > > > > > that removes the entry in mediaDevices_ as well as does a\n> > > > > > > > MediaDevice::release(). Thoughts?\n> > > > > >\n> > > > > > The media device acquired by the pipeline handler are meant to be\n> > > > > > released automatically. Camera registration failure isn't supposed to\n> > > > > > happen, in case one camera fails to register and the other doesn't, is\n> > > > > > there any issue keeping the media devices for the failed camera acquired\n> > > > > > ?\n> > > > >\n> > > > > I suppose there is no harm in keeping the device acquired when the camera cannot\n> > > > > be registered, particularly as it cannot be used again. In which case I can simplify the\n> > > > > error path logic.\n> > > > >\n> > > > > > > >> + } else\n> > > > > > > >> + numCameras++;\n> > > > > > > >>\n> > > > > > > >> - return registerCameras();\n> > > > > > > >> + break;\n> > > > > > > >> + }\n> > > > > > > >> + }\n> > > > > > > >> +\n> > > > > > > >> + return !!numCameras;\n> > > > > > > >> }\n> > > > > > > >>\n> > > > > > > >> -bool PipelineHandlerRPi::registerCameras()\n> > > > > > > >> +int PipelineHandlerRPi::registerCameras(MediaDevice *unicam, MediaDevice *isp,\n> > > > > > > >> + const std::string &unicamIdStr,\n> > > > > > > >> + const std::string &ispIdStr)\n> > > > > > > >> {\n> > > > > > > >> std::unique_ptr data = std::make_unique(this);\n> > > > > > > >> +\n> > > > > > > >> if (!data->dmaHeap_.isValid())\n> > > > > > > >> - return false;\n> > > > > > > >> + return -ENOMEM;\n> > > > > > > >> +\n> > > > > > > >> + MediaEntity *unicamImage = unicam->getEntityByName(\"unicam\" + unicamIdStr + \"-image\");\n> > > > > > > >> + MediaEntity *ispOutput0 = isp->getEntityByName(\"bcm2835-isp\" + ispIdStr + \"-output0\");\n> > > > > > > >> + MediaEntity *ispCapture1 = isp->getEntityByName(\"bcm2835-isp\" + ispIdStr + \"-capture1\");\n> > > > > > > >> + MediaEntity *ispCapture2 = isp->getEntityByName(\"bcm2835-isp\" + ispIdStr + \"-capture2\");\n> > > > > > > >> + MediaEntity *ispCapture3 = isp->getEntityByName(\"bcm2835-isp\" + ispIdStr + \"-capture3\");\n> > > > > > > >> +\n> > > > > > > >> + if (!unicamImage || !ispOutput0 || !ispCapture1 || !ispCapture2 || !ispCapture3)\n> > > > > > > >> + return -ENOENT;\n> > > > > > > >>\n> > > > > > > >> /* Locate and open the unicam video streams. */\n> > > > > > > >> - data->unicam_[Unicam::Image] = RPi::Stream(\"Unicam Image\", unicam_->getEntityByName(\"unicam-image\"));\n> > > > > > > >> + data->unicam_[Unicam::Image] = RPi::Stream(\"Unicam Image\", unicamImage);\n> > > > > > > >>\n> > > > > > > >> /* An embedded data node will not be present if the sensor does not support it. */\n> > > > > > > >> - MediaEntity *embeddedEntity = unicam_->getEntityByName(\"unicam-embedded\");\n> > > > > > > >> - if (embeddedEntity) {\n> > > > > > > >> - data->unicam_[Unicam::Embedded] = RPi::Stream(\"Unicam Embedded\", embeddedEntity);\n> > > > > > > >> + MediaEntity *unicamEmbedded = unicam->getEntityByName(\"unicam\" + unicamIdStr + \"-embedded\");\n> > > > > > > >> + if (unicamEmbedded) {\n> > > > > > > >> + data->unicam_[Unicam::Embedded] = RPi::Stream(\"Unicam Embedded\", unicamEmbedded);\n> > > > > > > >>\n> > > > > > > >> data->unicam_[Unicam::Embedded].dev()->bufferReady.connect(data.get(),\n> > > > > > > >> &RPiCameraData::unicamBufferDequeue);\n> > > > > > > >> }\n> > > > > > > >>\n> > > > > > > >> /* Tag the ISP input stream as an import stream. */\n> > > > > > > >> - data->isp_[Isp::Input] = RPi::Stream(\"ISP Input\", isp_->getEntityByName(\"bcm2835-isp0-output0\"), true);\n> > > > > > > >> - data->isp_[Isp::Output0] = RPi::Stream(\"ISP Output0\", isp_->getEntityByName(\"bcm2835-isp0-capture1\"));\n> > > > > > > >> - data->isp_[Isp::Output1] = RPi::Stream(\"ISP Output1\", isp_->getEntityByName(\"bcm2835-isp0-capture2\"));\n> > > > > > > >> - data->isp_[Isp::Stats] = RPi::Stream(\"ISP Stats\", isp_->getEntityByName(\"bcm2835-isp0-capture3\"));\n> > > > > > > >> + data->isp_[Isp::Input] = RPi::Stream(\"ISP Input\", ispOutput0, true);\n> > > > > > > >> + data->isp_[Isp::Output0] = RPi::Stream(\"ISP Output0\", ispCapture1);\n> > > > > > > >> + data->isp_[Isp::Output1] = RPi::Stream(\"ISP Output1\", ispCapture2);\n> > > > > > > >> + data->isp_[Isp::Stats] = RPi::Stream(\"ISP Stats\", ispCapture3);\n> > > > > > > >>\n> > > > > > > >> /* Wire up all the buffer connections. */\n> > > > > > > >> data->unicam_[Unicam::Image].dev()->frameStart.connect(data.get(), &RPiCameraData::frameStarted);\n> > > > > > > >> @@ -1046,7 +1085,7 @@ bool PipelineHandlerRPi::registerCameras()\n> > > > > > > >> data->isp_[Isp::Stats].dev()->bufferReady.connect(data.get(), &RPiCameraData::ispOutputDequeue);\n> > > > > > > >>\n> > > > > > > >> /* Identify the sensor. */\n> > > > > > > >> - for (MediaEntity *entity : unicam_->entities()) {\n> > > > > > > >> + for (MediaEntity *entity : unicam->entities()) {\n> > > > > > > >> if (entity->function() == MEDIA_ENT_F_CAM_SENSOR) {\n> > > > > > > >> data->sensor_ = std::make_unique(entity);\n> > > > > > > >> break;\n> > > > > > > >> @@ -1054,23 +1093,23 @@ bool PipelineHandlerRPi::registerCameras()\n> > > > > > > >> }\n> > > > > > > >>\n> > > > > > > >> if (!data->sensor_)\n> > > > > > > >> - return false;\n> > > > > > > >> + return -EINVAL;\n> > > > > > > >>\n> > > > > > > >> if (data->sensor_->init())\n> > > > > > > >> - return false;\n> > > > > > > >> + return -EINVAL;\n> > > > > > > >>\n> > > > > > > >> data->sensorFormats_ = populateSensorFormats(data->sensor_);\n> > > > > > > >>\n> > > > > > > >> ipa::RPi::SensorConfig sensorConfig;\n> > > > > > > >> if (data->loadIPA(&sensorConfig)) {\n> > > > > > > >> LOG(RPI, Error) << \"Failed to load a suitable IPA library\";\n> > > > > > > >> - return false;\n> > > > > > > >> + return -EINVAL;\n> > > > > > > >> }\n> > > > > > > >>\n> > > > > > > >> - if (sensorConfig.sensorMetadata ^ !!embeddedEntity) {\n> > > > > > > >> + if (sensorConfig.sensorMetadata ^ !!unicamEmbedded) {\n> > > > > > > >> LOG(RPI, Warning) << \"Mismatch between Unicam and CamHelper for embedded data usage!\";\n> > > > > > > >> sensorConfig.sensorMetadata = false;\n> > > > > > > >> - if (embeddedEntity)\n> > > > > > > >> + if (unicamEmbedded)\n> > > > > > > >> data->unicam_[Unicam::Embedded].dev()->bufferReady.disconnect();\n> > > > > > > >> }\n> > > > > > > >>\n> > > > > > > >> @@ -1091,12 +1130,12 @@ bool PipelineHandlerRPi::registerCameras()\n> > > > > > > >>\n> > > > > > > >> for (auto stream : data->streams_) {\n> > > > > > > >> if (stream->dev()->open())\n> > > > > > > >> - return false;\n> > > > > > > >> + continue;\n> > > > > > > >> }\n> > > > > > > >>\n> > > > > > > >> if (!data->unicam_[Unicam::Image].dev()->caps().hasMediaController()) {\n> > > > > > > >> LOG(RPI, Error) << \"Unicam driver does not use the MediaController, please update your kernel!\";\n> > > > > > > >> - return false;\n> > > > > > > >> + return -EINVAL;\n> > > > > > > >> }\n> > > > > > > >>\n> > > > > > > >> /*\n> > > > > > > >> @@ -1158,7 +1197,7 @@ bool PipelineHandlerRPi::registerCameras()\n> > > > > > > >>\n> > > > > > > >> if (!bayerFormat.isValid()) {\n> > > > > > > >> LOG(RPI, Error) << \"No Bayer format found\";\n> > > > > > > >> - return false;\n> > > > > > > >> + return -EINVAL;\n> > > > > > > >> }\n> > > > > > > >> data->nativeBayerOrder_ = bayerFormat.order;\n> > > > > > > >>\n> > > > > > > >> @@ -1178,7 +1217,10 @@ bool PipelineHandlerRPi::registerCameras()\n> > > > > > > >> Camera::create(std::move(data), id, streams);\n> > > > > > > >> registerCamera(std::move(camera));\n> > > > > > > >>\n> > > > > > > >> - return true;\n> > > > > > > >> + LOG(RPI, Info) << \"Registered camera \" << id\n> > > > > > > >> + << \" to instances \\\"unicam\" << unicamIdStr << \"\\\"\"\n> > > > > > > >> + << \" and \\\"isp\" << ispIdStr << \"\\\"\";\n> > > > > > > >> + return 0;\n> > > > > > > >> }\n> > > > > > > >>\n> > > > > > > >> int PipelineHandlerRPi::queueAllBuffers(Camera *camera)","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 30B44BF415\n\tfor ;\n\tMon, 22 Nov 2021 23:51:40 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3B3636038C;\n\tTue, 23 Nov 2021 00:51:39 +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 DA0C260233\n\tfor ;\n\tTue, 23 Nov 2021 00:51:36 +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 A87E292A;\n\tTue, 23 Nov 2021 00:51:35 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"Zj7rMzp0\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1637625095;\n\tbh=A+OUjKa7PMXouT15t8ZER26IZi8jlvzTWlSWPxJrImw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Zj7rMzp0qZf/lflIQG4Hhs6XLb38fUSwufPuG+8RypXA0HgxswWTQ/ojy8SRQj41K\n\teyCX+XCnpM9zp+vpvrQGBNezhu6qachhXGlQvA8H3+1D89ENY9lceXozUdCa0iKLKv\n\t8WzC1nWuFhVj7JhhyoGYE/fv9XjlnyBhHkB5/yTs=","Date":"Tue, 23 Nov 2021 01:51:12 +0200","From":"Laurent Pinchart ","To":"Naushir Patuck ","Message-ID":"","References":"<20211119110955.3137585-1-naush@raspberrypi.com>\n\t<20211119110955.3137585-2-naush@raspberrypi.com>\n\t\n\t\n\t\n\t\n\t\n\t\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 2/3] pipeline: raspberrypi: Allow\n\tregistration of multiple cameras","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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]