[{"id":23981,"web_url":"https://patchwork.libcamera.org/comment/23981/","msgid":"","date":"2022-07-19T22:49:32","subject":"Re: [libcamera-devel] [PATCH v3] pipeline: rkisp1: Support devices\n\twithout self path","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Paul,\n\nThank you for the patch.\n\nOn Tue, Jul 19, 2022 at 04:30:12PM +0900, Paul Elder via libcamera-devel wrote:\n> Some hardware supported by the rkisp1 driver, such as the ISP in the\n> i.MX8MP, don't have a self path. Although at the moment the driver still\n> exposes the self path, prepare the rkisp1 pipeline handler for when the\n> self path will be removed for devices that don't support it.\n> \n> Signed-off-by: Paul Elder \n\nReviewed-by: Laurent Pinchart \n\n> ---\n> Changes in v3:\n> - add more guards\n> - add pathCount guard to generateConfiguration to allow making self path\n> unavailable Just Work\n> \n> Changes in v2:\n> - simplify matching\n> - clean up self path availability in validate()\n> - fix configure(), return -ENODEV if multiple roles but no self path\n> ---\n> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 49 ++++++++++++++++--------\n> 1 file changed, 32 insertions(+), 17 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index 212fc76a..99eecd44 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -180,6 +180,8 @@ private:\n> \tstd::unique_ptr stat_;\n> \tstd::unique_ptr csi_;\n> \n> +\tbool hasSelfPath_;\n> +\n> \tRkISP1MainPath mainPath_;\n> \tRkISP1SelfPath selfPath_;\n> \n> @@ -364,7 +366,7 @@ void RkISP1CameraData::paramFilled(unsigned int frame)\n> \tif (info->mainPathBuffer)\n> \t\tmainPath_->queueBuffer(info->mainPathBuffer);\n> \n> -\tif (info->selfPathBuffer)\n> +\tif (selfPath_ && info->selfPathBuffer)\n> \t\tselfPath_->queueBuffer(info->selfPathBuffer);\n> }\n> \n> @@ -403,7 +405,7 @@ bool RkISP1CameraConfiguration::fitsAllPaths(const StreamConfiguration &cfg)\n> \t\treturn false;\n> \n> \tconfig = cfg;\n> -\tif (data_->selfPath_->validate(&config) != Valid)\n> +\tif (data_->selfPath_ && data_->selfPath_->validate(&config) != Valid)\n> \t\treturn false;\n> \n> \treturn true;\n> @@ -412,6 +414,7 @@ bool RkISP1CameraConfiguration::fitsAllPaths(const StreamConfiguration &cfg)\n> CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> {\n> \tconst CameraSensor *sensor = data_->sensor_.get();\n> +\tunsigned int pathCount = data_->selfPath_ ? 2 : 1;\n> \tStatus status = Valid;\n> \n> \tif (config_.empty())\n> @@ -423,8 +426,8 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> \t}\n> \n> \t/* Cap the number of entries to the available streams. */\n> -\tif (config_.size() > 2) {\n> -\t\tconfig_.resize(2);\n> +\tif (config_.size() > pathCount) {\n> +\t\tconfig_.resize(pathCount);\n> \t\tstatus = Adjusted;\n> \t}\n> \n> @@ -441,7 +444,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> \t\tstd::reverse(order.begin(), order.end());\n> \n> \tbool mainPathAvailable = true;\n> -\tbool selfPathAvailable = true;\n> +\tbool selfPathAvailable = data_->selfPath_;\n> \tfor (unsigned int index : order) {\n> \t\tStreamConfiguration &cfg = config_[index];\n> \n> @@ -520,7 +523,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> }\n> \n> PipelineHandlerRkISP1::PipelineHandlerRkISP1(CameraManager *manager)\n> -\t: PipelineHandler(manager)\n> +\t: PipelineHandler(manager), hasSelfPath_(true)\n> {\n> }\n> \n> @@ -532,12 +535,19 @@ CameraConfiguration *PipelineHandlerRkISP1::generateConfiguration(Camera *camera\n> \tconst StreamRoles &roles)\n> {\n> \tRkISP1CameraData *data = cameraData(camera);\n> +\n> +\tunsigned int pathCount = data->selfPath_ ? 2 : 1;\n> +\tif (roles.size() > pathCount) {\n> +\t\tLOG(RkISP1, Error) << \"Too many stream roles requested\";\n> +\t\treturn nullptr;\n> +\t}\n> +\n> \tCameraConfiguration *config = new RkISP1CameraConfiguration(camera, data);\n> \tif (roles.empty())\n> \t\treturn config;\n> \n> \tbool mainPathAvailable = true;\n> -\tbool selfPathAvailable = true;\n> +\tbool selfPathAvailable = data->selfPath_;\n> \tfor (const StreamRole role : roles) {\n> \t\tbool useMainPath;\n> \n> @@ -646,10 +656,12 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)\n> \t\t\tret = mainPath_.configure(cfg, format);\n> \t\t\tstreamConfig[0] = IPAStream(cfg.pixelFormat,\n> \t\t\t\t\t\t cfg.size);\n> -\t\t} else {\n> +\t\t} else if (hasSelfPath_) {\n> \t\t\tret = selfPath_.configure(cfg, format);\n> \t\t\tstreamConfig[1] = IPAStream(cfg.pixelFormat,\n> \t\t\t\t\t\t cfg.size);\n> +\t\t} else {\n> +\t\t\treturn -ENODEV;\n> \t\t}\n> \n> \t\tif (ret)\n> @@ -697,7 +709,7 @@ int PipelineHandlerRkISP1::exportFrameBuffers([[maybe_unused]] Camera *camera, S\n> \n> \tif (stream == &data->mainPathStream_)\n> \t\treturn mainPath_.exportBuffers(count, buffers);\n> -\telse if (stream == &data->selfPathStream_)\n> +\telse if (hasSelfPath_ && stream == &data->selfPathStream_)\n> \t\treturn selfPath_.exportBuffers(count, buffers);\n> \n> \treturn -EINVAL;\n> @@ -826,7 +838,7 @@ int PipelineHandlerRkISP1::start(Camera *camera, [[maybe_unused]] const ControlL\n> \t\t}\n> \t}\n> \n> -\tif (data->selfPath_->isEnabled()) {\n> +\tif (hasSelfPath_ && data->selfPath_->isEnabled()) {\n> \t\tret = selfPath_.start();\n> \t\tif (ret) {\n> \t\t\tmainPath_.stop();\n> @@ -853,7 +865,8 @@ void PipelineHandlerRkISP1::stopDevice(Camera *camera)\n> \n> \tdata->ipa_->stop();\n> \n> -\tselfPath_.stop();\n> +\tif (hasSelfPath_)\n> +\t\tselfPath_.stop();\n> \tmainPath_.stop();\n> \n> \tret = stat_->streamOff();\n> @@ -934,7 +947,7 @@ int PipelineHandlerRkISP1::initLinks(Camera *camera,\n> \tfor (const StreamConfiguration &cfg : config) {\n> \t\tif (cfg.stream() == &data->mainPathStream_)\n> \t\t\tret = data->mainPath_->setEnabled(true);\n> -\t\telse if (cfg.stream() == &data->selfPathStream_)\n> +\t\telse if (hasSelfPath_ && cfg.stream() == &data->selfPathStream_)\n> \t\t\tret = data->selfPath_->setEnabled(true);\n> \t\telse\n> \t\t\treturn -EINVAL;\n> @@ -951,7 +964,8 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)\n> \tint ret;\n> \n> \tstd::unique_ptr data =\n> -\t\tstd::make_unique(this, &mainPath_, &selfPath_);\n> +\t\tstd::make_unique(this, &mainPath_,\n> +\t\t\t\t\t\t hasSelfPath_ ? &selfPath_ : nullptr);\n> \n> \tControlInfoMap::Map ctrls;\n> \tctrls.emplace(std::piecewise_construct,\n> @@ -1007,9 +1021,7 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)\n> \n> \tDeviceMatch dm(\"rkisp1\");\n> \tdm.add(\"rkisp1_isp\");\n> -\tdm.add(\"rkisp1_resizer_selfpath\");\n> \tdm.add(\"rkisp1_resizer_mainpath\");\n> -\tdm.add(\"rkisp1_selfpath\");\n> \tdm.add(\"rkisp1_mainpath\");\n> \tdm.add(\"rkisp1_stats\");\n> \tdm.add(\"rkisp1_params\");\n> @@ -1024,6 +1036,8 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)\n> \t\treturn false;\n> \t}\n> \n> +\thasSelfPath_ = !!media_->getEntityByName(\"rkisp1_selfpath\");\n> +\n> \t/* Create the V4L2 subdevices we will need. */\n> \tisp_ = V4L2Subdevice::fromEntityName(media_, \"rkisp1_isp\");\n> \tif (isp_->open() < 0)\n> @@ -1058,11 +1072,12 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)\n> \tif (!mainPath_.init(media_))\n> \t\treturn false;\n> \n> -\tif (!selfPath_.init(media_))\n> +\tif (hasSelfPath_ && !selfPath_.init(media_))\n> \t\treturn false;\n> \n> \tmainPath_.bufferReady().connect(this, &PipelineHandlerRkISP1::bufferReady);\n> -\tselfPath_.bufferReady().connect(this, &PipelineHandlerRkISP1::bufferReady);\n> +\tif (hasSelfPath_)\n> +\t\tselfPath_.bufferReady().connect(this, &PipelineHandlerRkISP1::bufferReady);\n> \tstat_->bufferReady.connect(this, &PipelineHandlerRkISP1::statReady);\n> \tparam_->bufferReady.connect(this, &PipelineHandlerRkISP1::paramReady);\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 41BAFBE173\n\tfor ;\n\tTue, 19 Jul 2022 22:50:08 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7F5BA63312;\n\tWed, 20 Jul 2022 00:50:07 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 28976603F1\n\tfor ;\n\tWed, 20 Jul 2022 00:50:06 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 8C7C86DB;\n\tWed, 20 Jul 2022 00:50:05 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1658271007;\n\tbh=li0TH132+fOQP/O65frM8t6mKgiFF1afmzd9GoK+qRk=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=C+tgecvtTDoMT8t7NtFb/DH3V7bIWqDgsm+jDzMTZLUDSMcfoCbCuXEH3kEjXY6tB\n\tNSnX39LUr9lTMms9JTM2J8h29TWRLYiWVDxU/KQq/ZXTbCd3Kq3MF4SIDCKThzThWr\n\t+e64Zmj5rqGetTvp8wpBcMLyHV0AAJVdjbQjV5cNeFBdvSTSQvJf+3uPaHBqxJzplg\n\tJwYg1Rt0Xcdwgd7iKMyHU0oMbNip38u1s2EZX3j0hDz8nLxReCc+D5wDHpBDtdGnff\n\tBeiI+Aks66lRyuE7yHpKC1aNvVsL9xkyxbpca/MNNfPGuiBm0TENdpp61V8nxCXwBc\n\tdpoaKGhqlFZ6A==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1658271005;\n\tbh=li0TH132+fOQP/O65frM8t6mKgiFF1afmzd9GoK+qRk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=vlTqGH0BaK4Lmp7icgHJlYD2czaFM0qBRoYDYXvVPQguEmLeV2jOfmHxI/yypz2jB\n\tfXyVHKLVRd0znHOZNpRNdGz4PVkfh/psiRDCKG6Pl+pkGywKlmDGlomYEjER48Bbo1\n\tih7UTxBqXBSnuIY6IWnznCsMur5H1hlZdY/n1akE="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"vlTqGH0B\"; dkim-atps=neutral","Date":"Wed, 20 Jul 2022 01:49:32 +0300","To":"Paul Elder ","Message-ID":"","References":"<20220719073012.810184-1-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220719073012.810184-1-paul.elder@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v3] pipeline: rkisp1: Support devices\n\twithout self path","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":23984,"web_url":"https://patchwork.libcamera.org/comment/23984/","msgid":"<20220720083657.myrlba2n2rlxayep@uno.localdomain>","date":"2022-07-20T08:36:57","subject":"Re: [libcamera-devel] [PATCH v3] pipeline: rkisp1: Support devices\n\twithout self path","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Paul,\n\nOn Tue, Jul 19, 2022 at 04:30:12PM +0900, Paul Elder via libcamera-devel wrote:\n> Some hardware supported by the rkisp1 driver, such as the ISP in the\n> i.MX8MP, don't have a self path. Although at the moment the driver still\n> exposes the self path, prepare the rkisp1 pipeline handler for when the\n> self path will be removed for devices that don't support it.\n>\n> Signed-off-by: Paul Elder \n>\n> ---\n> Changes in v3:\n> - add more guards\n> - add pathCount guard to generateConfiguration to allow making self path\n> unavailable Just Work\n>\n> Changes in v2:\n> - simplify matching\n> - clean up self path availability in validate()\n> - fix configure(), return -ENODEV if multiple roles but no self path\n> ---\n> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 49 ++++++++++++++++--------\n> 1 file changed, 32 insertions(+), 17 deletions(-)\n>\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index 212fc76a..99eecd44 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -180,6 +180,8 @@ private:\n> \tstd::unique_ptr stat_;\n> \tstd::unique_ptr csi_;\n>\n> +\tbool hasSelfPath_;\n> +\n> \tRkISP1MainPath mainPath_;\n> \tRkISP1SelfPath selfPath_;\n>\n> @@ -364,7 +366,7 @@ void RkISP1CameraData::paramFilled(unsigned int frame)\n> \tif (info->mainPathBuffer)\n> \t\tmainPath_->queueBuffer(info->mainPathBuffer);\n>\n> -\tif (info->selfPathBuffer)\n> +\tif (selfPath_ && info->selfPathBuffer)\n> \t\tselfPath_->queueBuffer(info->selfPathBuffer);\n> }\n>\n> @@ -403,7 +405,7 @@ bool RkISP1CameraConfiguration::fitsAllPaths(const StreamConfiguration &cfg)\n> \t\treturn false;\n>\n> \tconfig = cfg;\n> -\tif (data_->selfPath_->validate(&config) != Valid)\n> +\tif (data_->selfPath_ && data_->selfPath_->validate(&config) != Valid)\n> \t\treturn false;\n>\n> \treturn true;\n> @@ -412,6 +414,7 @@ bool RkISP1CameraConfiguration::fitsAllPaths(const StreamConfiguration &cfg)\n> CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> {\n> \tconst CameraSensor *sensor = data_->sensor_.get();\n> +\tunsigned int pathCount = data_->selfPath_ ? 2 : 1;\n> \tStatus status = Valid;\n>\n> \tif (config_.empty())\n> @@ -423,8 +426,8 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> \t}\n>\n> \t/* Cap the number of entries to the available streams. */\n> -\tif (config_.size() > 2) {\n> -\t\tconfig_.resize(2);\n> +\tif (config_.size() > pathCount) {\n> +\t\tconfig_.resize(pathCount);\n> \t\tstatus = Adjusted;\n> \t}\n>\n> @@ -441,7 +444,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> \t\tstd::reverse(order.begin(), order.end());\n>\n> \tbool mainPathAvailable = true;\n> -\tbool selfPathAvailable = true;\n> +\tbool selfPathAvailable = data_->selfPath_;\n> \tfor (unsigned int index : order) {\n> \t\tStreamConfiguration &cfg = config_[index];\n>\n> @@ -520,7 +523,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> }\n>\n> PipelineHandlerRkISP1::PipelineHandlerRkISP1(CameraManager *manager)\n> -\t: PipelineHandler(manager)\n> +\t: PipelineHandler(manager), hasSelfPath_(true)\n> {\n> }\n>\n> @@ -532,12 +535,19 @@ CameraConfiguration *PipelineHandlerRkISP1::generateConfiguration(Camera *camera\n> \tconst StreamRoles &roles)\n> {\n> \tRkISP1CameraData *data = cameraData(camera);\n> +\n> +\tunsigned int pathCount = data->selfPath_ ? 2 : 1;\n> +\tif (roles.size() > pathCount) {\n> +\t\tLOG(RkISP1, Error) << \"Too many stream roles requested\";\n> +\t\treturn nullptr;\n> +\t}\n> +\n\nIf I'm not mistaken, the IPU3 works differently. It accepts all roles,\nand then lets validate() to shrink it down to the number of actually\nsupported ones.\n\n> \tCameraConfiguration *config = new RkISP1CameraConfiguration(camera, data);\n> \tif (roles.empty())\n> \t\treturn config;\n>\n> \tbool mainPathAvailable = true;\n> -\tbool selfPathAvailable = true;\n> +\tbool selfPathAvailable = data->selfPath_;\n> \tfor (const StreamRole role : roles) {\n> \t\tbool useMainPath;\n>\n> @@ -646,10 +656,12 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)\n> \t\t\tret = mainPath_.configure(cfg, format);\n> \t\t\tstreamConfig[0] = IPAStream(cfg.pixelFormat,\n> \t\t\t\t\t\t cfg.size);\n> -\t\t} else {\n> +\t\t} else if (hasSelfPath_) {\n> \t\t\tret = selfPath_.configure(cfg, format);\n> \t\t\tstreamConfig[1] = IPAStream(cfg.pixelFormat,\n> \t\t\t\t\t\t cfg.size);\n> +\t\t} else {\n> +\t\t\treturn -ENODEV;\n> \t\t}\n\nThis -shouldn't- be necessary. Configurations are validated before\nbeing passed to Camera::configure() and the number of streams has\nalready been reduced to 1 if !selfPath, and the only stream available\nshould be the mainPath one.\n\n>\n> \t\tif (ret)\n> @@ -697,7 +709,7 @@ int PipelineHandlerRkISP1::exportFrameBuffers([[maybe_unused]] Camera *camera, S\n>\n> \tif (stream == &data->mainPathStream_)\n> \t\treturn mainPath_.exportBuffers(count, buffers);\n> -\telse if (stream == &data->selfPathStream_)\n> +\telse if (hasSelfPath_ && stream == &data->selfPathStream_)\n> \t\treturn selfPath_.exportBuffers(count, buffers);\n>\n> \treturn -EINVAL;\n> @@ -826,7 +838,7 @@ int PipelineHandlerRkISP1::start(Camera *camera, [[maybe_unused]] const ControlL\n> \t\t}\n> \t}\n>\n> -\tif (data->selfPath_->isEnabled()) {\n> +\tif (hasSelfPath_ && data->selfPath_->isEnabled()) {\n\nSame reasoning, a validate configuration shouldn't have selfPath_\nenabled ? Have I missed how this can happen ?\n\n> \t\tret = selfPath_.start();\n> \t\tif (ret) {\n> \t\t\tmainPath_.stop();\n> @@ -853,7 +865,8 @@ void PipelineHandlerRkISP1::stopDevice(Camera *camera)\n>\n> \tdata->ipa_->stop();\n>\n> -\tselfPath_.stop();\n> +\tif (hasSelfPath_)\n> +\t\tselfPath_.stop();\n> \tmainPath_.stop();\n>\n> \tret = stat_->streamOff();\n> @@ -934,7 +947,7 @@ int PipelineHandlerRkISP1::initLinks(Camera *camera,\n> \tfor (const StreamConfiguration &cfg : config) {\n> \t\tif (cfg.stream() == &data->mainPathStream_)\n> \t\t\tret = data->mainPath_->setEnabled(true);\n> -\t\telse if (cfg.stream() == &data->selfPathStream_)\n> +\t\telse if (hasSelfPath_ && cfg.stream() == &data->selfPathStream_)\n> \t\t\tret = data->selfPath_->setEnabled(true);\n> \t\telse\n> \t\t\treturn -EINVAL;\n> @@ -951,7 +964,8 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)\n> \tint ret;\n>\n> \tstd::unique_ptr data =\n> -\t\tstd::make_unique(this, &mainPath_, &selfPath_);\n> +\t\tstd::make_unique(this, &mainPath_,\n> +\t\t\t\t\t\t hasSelfPath_ ? &selfPath_ : nullptr);\n>\n> \tControlInfoMap::Map ctrls;\n> \tctrls.emplace(std::piecewise_construct,\n> @@ -1007,9 +1021,7 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)\n>\n> \tDeviceMatch dm(\"rkisp1\");\n> \tdm.add(\"rkisp1_isp\");\n> -\tdm.add(\"rkisp1_resizer_selfpath\");\n> \tdm.add(\"rkisp1_resizer_mainpath\");\n> -\tdm.add(\"rkisp1_selfpath\");\n\nIt's a shame we can't verify in advance if the platform has a\nself-path and add the above entities to the DeviceMatch conditionally.\n\n> \tdm.add(\"rkisp1_mainpath\");\n> \tdm.add(\"rkisp1_stats\");\n> \tdm.add(\"rkisp1_params\");\n> @@ -1024,6 +1036,8 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)\n> \t\treturn false;\n> \t}\n>\n> +\thasSelfPath_ = !!media_->getEntityByName(\"rkisp1_selfpath\");\n> +\n> \t/* Create the V4L2 subdevices we will need. */\n> \tisp_ = V4L2Subdevice::fromEntityName(media_, \"rkisp1_isp\");\n> \tif (isp_->open() < 0)\n> @@ -1058,11 +1072,12 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)\n> \tif (!mainPath_.init(media_))\n> \t\treturn false;\n>\n> -\tif (!selfPath_.init(media_))\n> +\tif (hasSelfPath_ && !selfPath_.init(media_))\n> \t\treturn false;\n>\n> \tmainPath_.bufferReady().connect(this, &PipelineHandlerRkISP1::bufferReady);\n> -\tselfPath_.bufferReady().connect(this, &PipelineHandlerRkISP1::bufferReady);\n> +\tif (hasSelfPath_)\n> +\t\tselfPath_.bufferReady().connect(this, &PipelineHandlerRkISP1::bufferReady);\n> \tstat_->bufferReady.connect(this, &PipelineHandlerRkISP1::statReady);\n> \tparam_->bufferReady.connect(this, &PipelineHandlerRkISP1::paramReady);\n>\n> --\n> 2.30.2\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id DB0A5BD1F1\n\tfor ;\n\tWed, 20 Jul 2022 08:37:01 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 531CF63313;\n\tWed, 20 Jul 2022 10:37:01 +0200 (CEST)","from relay7-d.mail.gandi.net (relay7-d.mail.gandi.net\n\t[IPv6:2001:4b98:dc4:8::227])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6DC3760488\n\tfor ;\n\tWed, 20 Jul 2022 10:36:59 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby mail.gandi.net (Postfix) with ESMTPSA id E9F182000C;\n\tWed, 20 Jul 2022 08:36:58 +0000 (UTC)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1658306221;\n\tbh=lJJTek+pQjXsy7cbatOO2/LJz8q4rHZuHHwVoFOgf5U=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=gtzHxWnqSAYARMBNGFSzyR1Fr7M68/zOXpDcchPSSAi8zKJ5UEuD35qEz576Rb0sm\n\tpuPHX7K7YxL//RdRqn+VrESS9ktVGMVq7CygNkH/VP1b4lya3fnzWnNOo9ViKKgUNI\n\tAvj4aGsC/IedREgWoIBqbVZOOVFBGfR42Y5Dddrdr7OUZBqzho606C8wwMHacpspIJ\n\ta2evb1bc3HmeAolmdkYVF2cdRpTjAvdqzQESbh7Xb4zEHO0wsPaKLbN4aAQNwEOJrA\n\t5QHhk0+j3k1ROwU2UVLd3B9plobh5C9VWsYoIPmljOG2Ck0BwqRBCiqzLoRHv+TXoU\n\tPx1bqvMzWT46Q==","Date":"Wed, 20 Jul 2022 10:36:57 +0200","To":"Paul Elder ","Message-ID":"<20220720083657.myrlba2n2rlxayep@uno.localdomain>","References":"<20220719073012.810184-1-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220719073012.810184-1-paul.elder@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v3] pipeline: rkisp1: Support devices\n\twithout self path","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Jacopo Mondi via libcamera-devel ","Reply-To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":23986,"web_url":"https://patchwork.libcamera.org/comment/23986/","msgid":"","date":"2022-07-20T08:53:56","subject":"Re: [libcamera-devel] [PATCH v3] pipeline: rkisp1: Support devices\n\twithout self path","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Wed, Jul 20, 2022 at 10:36:57AM +0200, Jacopo Mondi via libcamera-devel wrote:\n> On Tue, Jul 19, 2022 at 04:30:12PM +0900, Paul Elder via libcamera-devel wrote:\n> > Some hardware supported by the rkisp1 driver, such as the ISP in the\n> > i.MX8MP, don't have a self path. Although at the moment the driver still\n> > exposes the self path, prepare the rkisp1 pipeline handler for when the\n> > self path will be removed for devices that don't support it.\n> >\n> > Signed-off-by: Paul Elder \n> >\n> > ---\n> > Changes in v3:\n> > - add more guards\n> > - add pathCount guard to generateConfiguration to allow making self path\n> > unavailable Just Work\n> >\n> > Changes in v2:\n> > - simplify matching\n> > - clean up self path availability in validate()\n> > - fix configure(), return -ENODEV if multiple roles but no self path\n> > ---\n> > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 49 ++++++++++++++++--------\n> > 1 file changed, 32 insertions(+), 17 deletions(-)\n> >\n> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > index 212fc76a..99eecd44 100644\n> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > @@ -180,6 +180,8 @@ private:\n> > \tstd::unique_ptr stat_;\n> > \tstd::unique_ptr csi_;\n> >\n> > +\tbool hasSelfPath_;\n> > +\n> > \tRkISP1MainPath mainPath_;\n> > \tRkISP1SelfPath selfPath_;\n> >\n> > @@ -364,7 +366,7 @@ void RkISP1CameraData::paramFilled(unsigned int frame)\n> > \tif (info->mainPathBuffer)\n> > \t\tmainPath_->queueBuffer(info->mainPathBuffer);\n> >\n> > -\tif (info->selfPathBuffer)\n> > +\tif (selfPath_ && info->selfPathBuffer)\n> > \t\tselfPath_->queueBuffer(info->selfPathBuffer);\n> > }\n> >\n> > @@ -403,7 +405,7 @@ bool RkISP1CameraConfiguration::fitsAllPaths(const StreamConfiguration &cfg)\n> > \t\treturn false;\n> >\n> > \tconfig = cfg;\n> > -\tif (data_->selfPath_->validate(&config) != Valid)\n> > +\tif (data_->selfPath_ && data_->selfPath_->validate(&config) != Valid)\n> > \t\treturn false;\n> >\n> > \treturn true;\n> > @@ -412,6 +414,7 @@ bool RkISP1CameraConfiguration::fitsAllPaths(const StreamConfiguration &cfg)\n> > CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> > {\n> > \tconst CameraSensor *sensor = data_->sensor_.get();\n> > +\tunsigned int pathCount = data_->selfPath_ ? 2 : 1;\n> > \tStatus status = Valid;\n> >\n> > \tif (config_.empty())\n> > @@ -423,8 +426,8 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> > \t}\n> >\n> > \t/* Cap the number of entries to the available streams. */\n> > -\tif (config_.size() > 2) {\n> > -\t\tconfig_.resize(2);\n> > +\tif (config_.size() > pathCount) {\n> > +\t\tconfig_.resize(pathCount);\n> > \t\tstatus = Adjusted;\n> > \t}\n> >\n> > @@ -441,7 +444,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> > \t\tstd::reverse(order.begin(), order.end());\n> >\n> > \tbool mainPathAvailable = true;\n> > -\tbool selfPathAvailable = true;\n> > +\tbool selfPathAvailable = data_->selfPath_;\n> > \tfor (unsigned int index : order) {\n> > \t\tStreamConfiguration &cfg = config_[index];\n> >\n> > @@ -520,7 +523,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> > }\n> >\n> > PipelineHandlerRkISP1::PipelineHandlerRkISP1(CameraManager *manager)\n> > -\t: PipelineHandler(manager)\n> > +\t: PipelineHandler(manager), hasSelfPath_(true)\n> > {\n> > }\n> >\n> > @@ -532,12 +535,19 @@ CameraConfiguration *PipelineHandlerRkISP1::generateConfiguration(Camera *camera\n> > \tconst StreamRoles &roles)\n> > {\n> > \tRkISP1CameraData *data = cameraData(camera);\n> > +\n> > +\tunsigned int pathCount = data->selfPath_ ? 2 : 1;\n> > +\tif (roles.size() > pathCount) {\n> > +\t\tLOG(RkISP1, Error) << \"Too many stream roles requested\";\n> > +\t\treturn nullptr;\n> > +\t}\n> > +\n> \n> If I'm not mistaken, the IPU3 works differently. It accepts all roles,\n> and then lets validate() to shrink it down to the number of actually\n> supported ones.\n\nI thought about this, but didn't check the IPU3 implementation when\nreviewing this patch. The generateConfiguration() documentation states\n\n * \\brief Generate a default camera configuration according to stream roles\n * \\param[in] roles A list of stream roles\n *\n * Generate a camera configuration for a set of desired stream roles. The caller\n * specifies a list of stream roles and the camera returns a configuration\n * containing suitable streams and their suggested default configurations. An\n * empty list of roles is valid, and will generate an empty configuration that\n * can be filled by the caller.\n *\n * \\context This function is \\threadsafe.\n *\n * \\return A CameraConfiguration if the requested roles can be satisfied, or a\n * null pointer otherwise. The ownership of the returned configuration is\n * passed to the caller.\n\nThe expected behaviour isn't very explicitly documented, but the\ndocumentation of the return value hints that if the requested roles\ncan't be satisfied, nullptr should be returned. Maybe we should modify\nthe IPU3 pipeline handler to match this ? Or modify the documentation ?\n\n> > \tCameraConfiguration *config = new RkISP1CameraConfiguration(camera, data);\n> > \tif (roles.empty())\n> > \t\treturn config;\n> >\n> > \tbool mainPathAvailable = true;\n> > -\tbool selfPathAvailable = true;\n> > +\tbool selfPathAvailable = data->selfPath_;\n> > \tfor (const StreamRole role : roles) {\n> > \t\tbool useMainPath;\n> >\n> > @@ -646,10 +656,12 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)\n> > \t\t\tret = mainPath_.configure(cfg, format);\n> > \t\t\tstreamConfig[0] = IPAStream(cfg.pixelFormat,\n> > \t\t\t\t\t\t cfg.size);\n> > -\t\t} else {\n> > +\t\t} else if (hasSelfPath_) {\n> > \t\t\tret = selfPath_.configure(cfg, format);\n> > \t\t\tstreamConfig[1] = IPAStream(cfg.pixelFormat,\n> > \t\t\t\t\t\t cfg.size);\n> > +\t\t} else {\n> > +\t\t\treturn -ENODEV;\n> > \t\t}\n> \n> This -shouldn't- be necessary. Configurations are validated before\n> being passed to Camera::configure() and the number of streams has\n> already been reduced to 1 if !selfPath, and the only stream available\n> should be the mainPath one.\n> \n> >\n> > \t\tif (ret)\n> > @@ -697,7 +709,7 @@ int PipelineHandlerRkISP1::exportFrameBuffers([[maybe_unused]] Camera *camera, S\n> >\n> > \tif (stream == &data->mainPathStream_)\n> > \t\treturn mainPath_.exportBuffers(count, buffers);\n> > -\telse if (stream == &data->selfPathStream_)\n> > +\telse if (hasSelfPath_ && stream == &data->selfPathStream_)\n> > \t\treturn selfPath_.exportBuffers(count, buffers);\n> >\n> > \treturn -EINVAL;\n> > @@ -826,7 +838,7 @@ int PipelineHandlerRkISP1::start(Camera *camera, [[maybe_unused]] const ControlL\n> > \t\t}\n> > \t}\n> >\n> > -\tif (data->selfPath_->isEnabled()) {\n> > +\tif (hasSelfPath_ && data->selfPath_->isEnabled()) {\n> \n> Same reasoning, a validate configuration shouldn't have selfPath_\n> enabled ? Have I missed how this can happen ?\n\nI think I commented in the review of the previous version that those two\ncases should never happen :-) I don't mind keeping the checks, it's up\nto you and Paul.\n\n> > \t\tret = selfPath_.start();\n> > \t\tif (ret) {\n> > \t\t\tmainPath_.stop();\n> > @@ -853,7 +865,8 @@ void PipelineHandlerRkISP1::stopDevice(Camera *camera)\n> >\n> > \tdata->ipa_->stop();\n> >\n> > -\tselfPath_.stop();\n> > +\tif (hasSelfPath_)\n> > +\t\tselfPath_.stop();\n> > \tmainPath_.stop();\n> >\n> > \tret = stat_->streamOff();\n> > @@ -934,7 +947,7 @@ int PipelineHandlerRkISP1::initLinks(Camera *camera,\n> > \tfor (const StreamConfiguration &cfg : config) {\n> > \t\tif (cfg.stream() == &data->mainPathStream_)\n> > \t\t\tret = data->mainPath_->setEnabled(true);\n> > -\t\telse if (cfg.stream() == &data->selfPathStream_)\n> > +\t\telse if (hasSelfPath_ && cfg.stream() == &data->selfPathStream_)\n> > \t\t\tret = data->selfPath_->setEnabled(true);\n> > \t\telse\n> > \t\t\treturn -EINVAL;\n> > @@ -951,7 +964,8 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)\n> > \tint ret;\n> >\n> > \tstd::unique_ptr data =\n> > -\t\tstd::make_unique(this, &mainPath_, &selfPath_);\n> > +\t\tstd::make_unique(this, &mainPath_,\n> > +\t\t\t\t\t\t hasSelfPath_ ? &selfPath_ : nullptr);\n> >\n> > \tControlInfoMap::Map ctrls;\n> > \tctrls.emplace(std::piecewise_construct,\n> > @@ -1007,9 +1021,7 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)\n> >\n> > \tDeviceMatch dm(\"rkisp1\");\n> > \tdm.add(\"rkisp1_isp\");\n> > -\tdm.add(\"rkisp1_resizer_selfpath\");\n> > \tdm.add(\"rkisp1_resizer_mainpath\");\n> > -\tdm.add(\"rkisp1_selfpath\");\n> \n> It's a shame we can't verify in advance if the platform has a\n> self-path and add the above entities to the DeviceMatch conditionally.\n\nTrue, but I don't think there's a risk of a false positive match here\nwhen removing the self path.\n\n> > \tdm.add(\"rkisp1_mainpath\");\n> > \tdm.add(\"rkisp1_stats\");\n> > \tdm.add(\"rkisp1_params\");\n> > @@ -1024,6 +1036,8 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)\n> > \t\treturn false;\n> > \t}\n> >\n> > +\thasSelfPath_ = !!media_->getEntityByName(\"rkisp1_selfpath\");\n> > +\n> > \t/* Create the V4L2 subdevices we will need. */\n> > \tisp_ = V4L2Subdevice::fromEntityName(media_, \"rkisp1_isp\");\n> > \tif (isp_->open() < 0)\n> > @@ -1058,11 +1072,12 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)\n> > \tif (!mainPath_.init(media_))\n> > \t\treturn false;\n> >\n> > -\tif (!selfPath_.init(media_))\n> > +\tif (hasSelfPath_ && !selfPath_.init(media_))\n> > \t\treturn false;\n> >\n> > \tmainPath_.bufferReady().connect(this, &PipelineHandlerRkISP1::bufferReady);\n> > -\tselfPath_.bufferReady().connect(this, &PipelineHandlerRkISP1::bufferReady);\n> > +\tif (hasSelfPath_)\n> > +\t\tselfPath_.bufferReady().connect(this, &PipelineHandlerRkISP1::bufferReady);\n> > \tstat_->bufferReady.connect(this, &PipelineHandlerRkISP1::statReady);\n> > \tparam_->bufferReady.connect(this, &PipelineHandlerRkISP1::paramReady);\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 2BF2FBD1F1\n\tfor ;\n\tWed, 20 Jul 2022 08:54:33 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 747F263313;\n\tWed, 20 Jul 2022 10:54:32 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id F107060488\n\tfor ;\n\tWed, 20 Jul 2022 10:54:30 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 453656DB;\n\tWed, 20 Jul 2022 10:54:30 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1658307272;\n\tbh=RrIFbUoSZr4QxQ1N07DE70XbiJzYkEh44RI3pw9gKk0=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=U4S1mCgDl9iD3YH9FGmRWktk3A/s3kaJYf1ah82dm2erMLOYEvb+jG8o/429z2IYb\n\t3R1ilL6FO2WYpEOQ3/ch+PhYyk6QZ6QgPCdZXBy7e3X9Tast3J4B0QOgLLU5ON7VCV\n\tj8/x9XqolACZN4G7tEolb5CZ+MsWnFPwHNdmjywC07hmPgDSaazNC7sNwRfomo31yD\n\ttx6W8I7KCnTYWZynl0Spj4cA/eURNf5j7XR0VCIK02+jDaQkdVm0dLr13JGsaXgTry\n\tGBOFkWnif5OJ8J2HPTSsUISn/c+2ucJ3ZACYGSrf9ofN5+dOs0SBNUGMM8batsCnLX\n\tlEoLMv3LMLabw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1658307270;\n\tbh=RrIFbUoSZr4QxQ1N07DE70XbiJzYkEh44RI3pw9gKk0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=NYvGagihY0DMikZEu8cwZfqofFyFXiMQI1CuyAS2UXHlEF4l1vrTarIOwqrQ9dNpS\n\tegU8bP5jor4AGn2YqBNqCvWyrPLLG88Zhfz4pK+pKujnrwMflCv57h73cqtnqjWO5F\n\tP9lAPwIs+pJuXd/bzq1+l7On27HCq+dD4qFvL1mM="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"NYvGagih\"; dkim-atps=neutral","Date":"Wed, 20 Jul 2022 11:53:56 +0300","To":"Jacopo Mondi ","Message-ID":"","References":"<20220719073012.810184-1-paul.elder@ideasonboard.com>\n\t<20220720083657.myrlba2n2rlxayep@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220720083657.myrlba2n2rlxayep@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v3] pipeline: rkisp1: Support devices\n\twithout self path","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":23989,"web_url":"https://patchwork.libcamera.org/comment/23989/","msgid":"<20220720090704.af2ajf2izr5xvepz@uno.localdomain>","date":"2022-07-20T09:07:04","subject":"Re: [libcamera-devel] [PATCH v3] pipeline: rkisp1: Support devices\n\twithout self path","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent\n\nOn Wed, Jul 20, 2022 at 11:53:56AM +0300, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> On Wed, Jul 20, 2022 at 10:36:57AM +0200, Jacopo Mondi via libcamera-devel wrote:\n> > On Tue, Jul 19, 2022 at 04:30:12PM +0900, Paul Elder via libcamera-devel wrote:\n> > > Some hardware supported by the rkisp1 driver, such as the ISP in the\n> > > i.MX8MP, don't have a self path. Although at the moment the driver still\n> > > exposes the self path, prepare the rkisp1 pipeline handler for when the\n> > > self path will be removed for devices that don't support it.\n> > >\n> > > Signed-off-by: Paul Elder \n> > >\n> > > ---\n> > > Changes in v3:\n> > > - add more guards\n> > > - add pathCount guard to generateConfiguration to allow making self path\n> > > unavailable Just Work\n> > >\n> > > Changes in v2:\n> > > - simplify matching\n> > > - clean up self path availability in validate()\n> > > - fix configure(), return -ENODEV if multiple roles but no self path\n> > > ---\n> > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 49 ++++++++++++++++--------\n> > > 1 file changed, 32 insertions(+), 17 deletions(-)\n> > >\n> > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > index 212fc76a..99eecd44 100644\n> > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > @@ -180,6 +180,8 @@ private:\n> > > \tstd::unique_ptr stat_;\n> > > \tstd::unique_ptr csi_;\n> > >\n> > > +\tbool hasSelfPath_;\n> > > +\n> > > \tRkISP1MainPath mainPath_;\n> > > \tRkISP1SelfPath selfPath_;\n> > >\n> > > @@ -364,7 +366,7 @@ void RkISP1CameraData::paramFilled(unsigned int frame)\n> > > \tif (info->mainPathBuffer)\n> > > \t\tmainPath_->queueBuffer(info->mainPathBuffer);\n> > >\n> > > -\tif (info->selfPathBuffer)\n> > > +\tif (selfPath_ && info->selfPathBuffer)\n> > > \t\tselfPath_->queueBuffer(info->selfPathBuffer);\n> > > }\n> > >\n> > > @@ -403,7 +405,7 @@ bool RkISP1CameraConfiguration::fitsAllPaths(const StreamConfiguration &cfg)\n> > > \t\treturn false;\n> > >\n> > > \tconfig = cfg;\n> > > -\tif (data_->selfPath_->validate(&config) != Valid)\n> > > +\tif (data_->selfPath_ && data_->selfPath_->validate(&config) != Valid)\n> > > \t\treturn false;\n> > >\n> > > \treturn true;\n> > > @@ -412,6 +414,7 @@ bool RkISP1CameraConfiguration::fitsAllPaths(const StreamConfiguration &cfg)\n> > > CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> > > {\n> > > \tconst CameraSensor *sensor = data_->sensor_.get();\n> > > +\tunsigned int pathCount = data_->selfPath_ ? 2 : 1;\n> > > \tStatus status = Valid;\n> > >\n> > > \tif (config_.empty())\n> > > @@ -423,8 +426,8 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> > > \t}\n> > >\n> > > \t/* Cap the number of entries to the available streams. */\n> > > -\tif (config_.size() > 2) {\n> > > -\t\tconfig_.resize(2);\n> > > +\tif (config_.size() > pathCount) {\n> > > +\t\tconfig_.resize(pathCount);\n> > > \t\tstatus = Adjusted;\n> > > \t}\n> > >\n> > > @@ -441,7 +444,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> > > \t\tstd::reverse(order.begin(), order.end());\n> > >\n> > > \tbool mainPathAvailable = true;\n> > > -\tbool selfPathAvailable = true;\n> > > +\tbool selfPathAvailable = data_->selfPath_;\n> > > \tfor (unsigned int index : order) {\n> > > \t\tStreamConfiguration &cfg = config_[index];\n> > >\n> > > @@ -520,7 +523,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> > > }\n> > >\n> > > PipelineHandlerRkISP1::PipelineHandlerRkISP1(CameraManager *manager)\n> > > -\t: PipelineHandler(manager)\n> > > +\t: PipelineHandler(manager), hasSelfPath_(true)\n> > > {\n> > > }\n> > >\n> > > @@ -532,12 +535,19 @@ CameraConfiguration *PipelineHandlerRkISP1::generateConfiguration(Camera *camera\n> > > \tconst StreamRoles &roles)\n> > > {\n> > > \tRkISP1CameraData *data = cameraData(camera);\n> > > +\n> > > +\tunsigned int pathCount = data->selfPath_ ? 2 : 1;\n> > > +\tif (roles.size() > pathCount) {\n> > > +\t\tLOG(RkISP1, Error) << \"Too many stream roles requested\";\n> > > +\t\treturn nullptr;\n> > > +\t}\n> > > +\n> >\n> > If I'm not mistaken, the IPU3 works differently. It accepts all roles,\n> > and then lets validate() to shrink it down to the number of actually\n> > supported ones.\n>\n> I thought about this, but didn't check the IPU3 implementation when\n> reviewing this patch. The generateConfiguration() documentation states\n>\n> * \\brief Generate a default camera configuration according to stream roles\n> * \\param[in] roles A list of stream roles\n> *\n> * Generate a camera configuration for a set of desired stream roles. The caller\n> * specifies a list of stream roles and the camera returns a configuration\n> * containing suitable streams and their suggested default configurations. An\n> * empty list of roles is valid, and will generate an empty configuration that\n> * can be filled by the caller.\n> *\n> * \\context This function is \\threadsafe.\n> *\n> * \\return A CameraConfiguration if the requested roles can be satisfied, or a\n> * null pointer otherwise. The ownership of the returned configuration is\n> * passed to the caller.\n>\n> The expected behaviour isn't very explicitly documented, but the\n> documentation of the return value hints that if the requested roles\n> can't be satisfied, nullptr should be returned. Maybe we should modify\n> the IPU3 pipeline handler to match this ? Or modify the documentation ?\n>\n\nFYI raspberrypi and simple behave like IPU3, if I'm not mistaken.\n\nI'm debated, I like the idea of returning a valid configuration, but\nthe Adjusted flag can be easily ignored and the application might find\nitself dealing with something it doesn't expect.\n\n> > > \tCameraConfiguration *config = new RkISP1CameraConfiguration(camera, data);\n> > > \tif (roles.empty())\n> > > \t\treturn config;\n> > >\n> > > \tbool mainPathAvailable = true;\n> > > -\tbool selfPathAvailable = true;\n> > > +\tbool selfPathAvailable = data->selfPath_;\n> > > \tfor (const StreamRole role : roles) {\n> > > \t\tbool useMainPath;\n> > >\n> > > @@ -646,10 +656,12 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)\n> > > \t\t\tret = mainPath_.configure(cfg, format);\n> > > \t\t\tstreamConfig[0] = IPAStream(cfg.pixelFormat,\n> > > \t\t\t\t\t\t cfg.size);\n> > > -\t\t} else {\n> > > +\t\t} else if (hasSelfPath_) {\n> > > \t\t\tret = selfPath_.configure(cfg, format);\n> > > \t\t\tstreamConfig[1] = IPAStream(cfg.pixelFormat,\n> > > \t\t\t\t\t\t cfg.size);\n> > > +\t\t} else {\n> > > +\t\t\treturn -ENODEV;\n> > > \t\t}\n> >\n> > This -shouldn't- be necessary. Configurations are validated before\n> > being passed to Camera::configure() and the number of streams has\n> > already been reduced to 1 if !selfPath, and the only stream available\n> > should be the mainPath one.\n> >\n> > >\n> > > \t\tif (ret)\n> > > @@ -697,7 +709,7 @@ int PipelineHandlerRkISP1::exportFrameBuffers([[maybe_unused]] Camera *camera, S\n> > >\n> > > \tif (stream == &data->mainPathStream_)\n> > > \t\treturn mainPath_.exportBuffers(count, buffers);\n> > > -\telse if (stream == &data->selfPathStream_)\n> > > +\telse if (hasSelfPath_ && stream == &data->selfPathStream_)\n> > > \t\treturn selfPath_.exportBuffers(count, buffers);\n> > >\n> > > \treturn -EINVAL;\n> > > @@ -826,7 +838,7 @@ int PipelineHandlerRkISP1::start(Camera *camera, [[maybe_unused]] const ControlL\n> > > \t\t}\n> > > \t}\n> > >\n> > > -\tif (data->selfPath_->isEnabled()) {\n> > > +\tif (hasSelfPath_ && data->selfPath_->isEnabled()) {\n> >\n> > Same reasoning, a validate configuration shouldn't have selfPath_\n> > enabled ? Have I missed how this can happen ?\n>\n> I think I commented in the review of the previous version that those two\n> cases should never happen :-) I don't mind keeping the checks, it's up\n> to you and Paul.\n>\n\nDon't know, I don't feel strong on this. Generally avoiding testing\nfor conditions known to be false could save the reader from wondering why\nthe check is there and chase a code path that doesn't exist.\n\nBut this is minimal so I'll let Paul decide.\n\n\n> > > \t\tret = selfPath_.start();\n> > > \t\tif (ret) {\n> > > \t\t\tmainPath_.stop();\n> > > @@ -853,7 +865,8 @@ void PipelineHandlerRkISP1::stopDevice(Camera *camera)\n> > >\n> > > \tdata->ipa_->stop();\n> > >\n> > > -\tselfPath_.stop();\n> > > +\tif (hasSelfPath_)\n> > > +\t\tselfPath_.stop();\n> > > \tmainPath_.stop();\n> > >\n> > > \tret = stat_->streamOff();\n> > > @@ -934,7 +947,7 @@ int PipelineHandlerRkISP1::initLinks(Camera *camera,\n> > > \tfor (const StreamConfiguration &cfg : config) {\n> > > \t\tif (cfg.stream() == &data->mainPathStream_)\n> > > \t\t\tret = data->mainPath_->setEnabled(true);\n> > > -\t\telse if (cfg.stream() == &data->selfPathStream_)\n> > > +\t\telse if (hasSelfPath_ && cfg.stream() == &data->selfPathStream_)\n> > > \t\t\tret = data->selfPath_->setEnabled(true);\n> > > \t\telse\n> > > \t\t\treturn -EINVAL;\n> > > @@ -951,7 +964,8 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)\n> > > \tint ret;\n> > >\n> > > \tstd::unique_ptr data =\n> > > -\t\tstd::make_unique(this, &mainPath_, &selfPath_);\n> > > +\t\tstd::make_unique(this, &mainPath_,\n> > > +\t\t\t\t\t\t hasSelfPath_ ? &selfPath_ : nullptr);\n> > >\n> > > \tControlInfoMap::Map ctrls;\n> > > \tctrls.emplace(std::piecewise_construct,\n> > > @@ -1007,9 +1021,7 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)\n> > >\n> > > \tDeviceMatch dm(\"rkisp1\");\n> > > \tdm.add(\"rkisp1_isp\");\n> > > -\tdm.add(\"rkisp1_resizer_selfpath\");\n> > > \tdm.add(\"rkisp1_resizer_mainpath\");\n> > > -\tdm.add(\"rkisp1_selfpath\");\n> >\n> > It's a shame we can't verify in advance if the platform has a\n> > self-path and add the above entities to the DeviceMatch conditionally.\n>\n> True, but I don't think there's a risk of a false positive match here\n> when removing the self path.\n>\n\nYeah, and unless we introduce something similar to\ndevice_get_match_data() where to hardcode that information there's no\nway we can know that in advance.\n\nThanks\n j\n\n> > > \tdm.add(\"rkisp1_mainpath\");\n> > > \tdm.add(\"rkisp1_stats\");\n> > > \tdm.add(\"rkisp1_params\");\n> > > @@ -1024,6 +1036,8 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)\n> > > \t\treturn false;\n> > > \t}\n> > >\n> > > +\thasSelfPath_ = !!media_->getEntityByName(\"rkisp1_selfpath\");\n> > > +\n> > > \t/* Create the V4L2 subdevices we will need. */\n> > > \tisp_ = V4L2Subdevice::fromEntityName(media_, \"rkisp1_isp\");\n> > > \tif (isp_->open() < 0)\n> > > @@ -1058,11 +1072,12 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)\n> > > \tif (!mainPath_.init(media_))\n> > > \t\treturn false;\n> > >\n> > > -\tif (!selfPath_.init(media_))\n> > > +\tif (hasSelfPath_ && !selfPath_.init(media_))\n> > > \t\treturn false;\n> > >\n> > > \tmainPath_.bufferReady().connect(this, &PipelineHandlerRkISP1::bufferReady);\n> > > -\tselfPath_.bufferReady().connect(this, &PipelineHandlerRkISP1::bufferReady);\n> > > +\tif (hasSelfPath_)\n> > > +\t\tselfPath_.bufferReady().connect(this, &PipelineHandlerRkISP1::bufferReady);\n> > > \tstat_->bufferReady.connect(this, &PipelineHandlerRkISP1::statReady);\n> > > \tparam_->bufferReady.connect(this, &PipelineHandlerRkISP1::paramReady);\n> > >\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","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 E2CE1BD1F1\n\tfor ;\n\tWed, 20 Jul 2022 09:07:07 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6CFC063313;\n\tWed, 20 Jul 2022 11:07:07 +0200 (CEST)","from relay12.mail.gandi.net (relay12.mail.gandi.net\n\t[217.70.178.232])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 538EB60488\n\tfor ;\n\tWed, 20 Jul 2022 11:07:06 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby mail.gandi.net (Postfix) with ESMTPSA id C20B7200009;\n\tWed, 20 Jul 2022 09:07:05 +0000 (UTC)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1658308027;\n\tbh=hraB/e/fGy+WkZnE+zRfFe7HyWJgpfZtZX9ZRPLwXeI=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=I1+FZMekBwEo2IrHyyHfK5untWe2H07FxWwEpe/cYACJMCWRpJEJqc7qPSnMGcjaQ\n\t1A0iNLhjxrq3qC5NUftf6kAB1P3laQb6AM5S5eMsPx7KKu643oRqYypFdYQbJ+e0Dp\n\t0c+A7KWo8JNzTSsUMdH2Wl8TXorBmetshuxnHujVy/pdFV5kND6oOaM9L7KjhW+wk+\n\tbvHiC5zbEJ1CVZtsWZWdSLPsSqvYg5F7uTErcAItXE549u6zTVs5tw8yoBz8RdKMUm\n\tpDYl2ORGagnuc0CqyGyYvlRdCyNvPGyop/8dzqTNDyLYh1B9wWy9FnMJbcS4IBuDxT\n\tMrrEgUg+alxbA==","Date":"Wed, 20 Jul 2022 11:07:04 +0200","To":"Laurent Pinchart ","Message-ID":"<20220720090704.af2ajf2izr5xvepz@uno.localdomain>","References":"<20220719073012.810184-1-paul.elder@ideasonboard.com>\n\t<20220720083657.myrlba2n2rlxayep@uno.localdomain>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v3] pipeline: rkisp1: Support devices\n\twithout self path","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Jacopo Mondi via libcamera-devel ","Reply-To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":23993,"web_url":"https://patchwork.libcamera.org/comment/23993/","msgid":"<20220720091951.GD3984498@pyrite.rasen.tech>","date":"2022-07-20T09:19:51","subject":"Re: [libcamera-devel] [PATCH v3] pipeline: rkisp1: Support devices\n\twithout self path","submitter":{"id":97,"url":"https://patchwork.libcamera.org/api/people/97/","name":"Nicolas Dufresne via libcamera-devel","email":"libcamera-devel@lists.libcamera.org"},"content":"Hi,\n\nOn Wed, Jul 20, 2022 at 11:07:04AM +0200, Jacopo Mondi wrote:\n> Hi Laurent\n> \n> On Wed, Jul 20, 2022 at 11:53:56AM +0300, Laurent Pinchart wrote:\n> > Hi Jacopo,\n> >\n> > On Wed, Jul 20, 2022 at 10:36:57AM +0200, Jacopo Mondi via libcamera-devel wrote:\n> > > On Tue, Jul 19, 2022 at 04:30:12PM +0900, Paul Elder via libcamera-devel wrote:\n> > > > Some hardware supported by the rkisp1 driver, such as the ISP in the\n> > > > i.MX8MP, don't have a self path. Although at the moment the driver still\n> > > > exposes the self path, prepare the rkisp1 pipeline handler for when the\n> > > > self path will be removed for devices that don't support it.\n> > > >\n> > > > Signed-off-by: Paul Elder \n> > > >\n> > > > ---\n> > > > Changes in v3:\n> > > > - add more guards\n> > > > - add pathCount guard to generateConfiguration to allow making self path\n> > > > unavailable Just Work\n> > > >\n> > > > Changes in v2:\n> > > > - simplify matching\n> > > > - clean up self path availability in validate()\n> > > > - fix configure(), return -ENODEV if multiple roles but no self path\n> > > > ---\n> > > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 49 ++++++++++++++++--------\n> > > > 1 file changed, 32 insertions(+), 17 deletions(-)\n> > > >\n> > > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > > index 212fc76a..99eecd44 100644\n> > > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > > @@ -180,6 +180,8 @@ private:\n> > > > \tstd::unique_ptr stat_;\n> > > > \tstd::unique_ptr csi_;\n> > > >\n> > > > +\tbool hasSelfPath_;\n> > > > +\n> > > > \tRkISP1MainPath mainPath_;\n> > > > \tRkISP1SelfPath selfPath_;\n> > > >\n> > > > @@ -364,7 +366,7 @@ void RkISP1CameraData::paramFilled(unsigned int frame)\n> > > > \tif (info->mainPathBuffer)\n> > > > \t\tmainPath_->queueBuffer(info->mainPathBuffer);\n> > > >\n> > > > -\tif (info->selfPathBuffer)\n> > > > +\tif (selfPath_ && info->selfPathBuffer)\n> > > > \t\tselfPath_->queueBuffer(info->selfPathBuffer);\n> > > > }\n> > > >\n> > > > @@ -403,7 +405,7 @@ bool RkISP1CameraConfiguration::fitsAllPaths(const StreamConfiguration &cfg)\n> > > > \t\treturn false;\n> > > >\n> > > > \tconfig = cfg;\n> > > > -\tif (data_->selfPath_->validate(&config) != Valid)\n> > > > +\tif (data_->selfPath_ && data_->selfPath_->validate(&config) != Valid)\n> > > > \t\treturn false;\n> > > >\n> > > > \treturn true;\n> > > > @@ -412,6 +414,7 @@ bool RkISP1CameraConfiguration::fitsAllPaths(const StreamConfiguration &cfg)\n> > > > CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> > > > {\n> > > > \tconst CameraSensor *sensor = data_->sensor_.get();\n> > > > +\tunsigned int pathCount = data_->selfPath_ ? 2 : 1;\n> > > > \tStatus status = Valid;\n> > > >\n> > > > \tif (config_.empty())\n> > > > @@ -423,8 +426,8 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> > > > \t}\n> > > >\n> > > > \t/* Cap the number of entries to the available streams. */\n> > > > -\tif (config_.size() > 2) {\n> > > > -\t\tconfig_.resize(2);\n> > > > +\tif (config_.size() > pathCount) {\n> > > > +\t\tconfig_.resize(pathCount);\n> > > > \t\tstatus = Adjusted;\n> > > > \t}\n> > > >\n> > > > @@ -441,7 +444,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> > > > \t\tstd::reverse(order.begin(), order.end());\n> > > >\n> > > > \tbool mainPathAvailable = true;\n> > > > -\tbool selfPathAvailable = true;\n> > > > +\tbool selfPathAvailable = data_->selfPath_;\n> > > > \tfor (unsigned int index : order) {\n> > > > \t\tStreamConfiguration &cfg = config_[index];\n> > > >\n> > > > @@ -520,7 +523,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> > > > }\n> > > >\n> > > > PipelineHandlerRkISP1::PipelineHandlerRkISP1(CameraManager *manager)\n> > > > -\t: PipelineHandler(manager)\n> > > > +\t: PipelineHandler(manager), hasSelfPath_(true)\n> > > > {\n> > > > }\n> > > >\n> > > > @@ -532,12 +535,19 @@ CameraConfiguration *PipelineHandlerRkISP1::generateConfiguration(Camera *camera\n> > > > \tconst StreamRoles &roles)\n> > > > {\n> > > > \tRkISP1CameraData *data = cameraData(camera);\n> > > > +\n> > > > +\tunsigned int pathCount = data->selfPath_ ? 2 : 1;\n> > > > +\tif (roles.size() > pathCount) {\n> > > > +\t\tLOG(RkISP1, Error) << \"Too many stream roles requested\";\n> > > > +\t\treturn nullptr;\n> > > > +\t}\n> > > > +\n> > >\n> > > If I'm not mistaken, the IPU3 works differently. It accepts all roles,\n> > > and then lets validate() to shrink it down to the number of actually\n> > > supported ones.\n> >\n> > I thought about this, but didn't check the IPU3 implementation when\n> > reviewing this patch. The generateConfiguration() documentation states\n> >\n> > * \\brief Generate a default camera configuration according to stream roles\n> > * \\param[in] roles A list of stream roles\n> > *\n> > * Generate a camera configuration for a set of desired stream roles. The caller\n> > * specifies a list of stream roles and the camera returns a configuration\n> > * containing suitable streams and their suggested default configurations. An\n> > * empty list of roles is valid, and will generate an empty configuration that\n> > * can be filled by the caller.\n> > *\n> > * \\context This function is \\threadsafe.\n> > *\n> > * \\return A CameraConfiguration if the requested roles can be satisfied, or a\n> > * null pointer otherwise. The ownership of the returned configuration is\n> > * passed to the caller.\n> >\n> > The expected behaviour isn't very explicitly documented, but the\n> > documentation of the return value hints that if the requested roles\n> > can't be satisfied, nullptr should be returned. Maybe we should modify\n> > the IPU3 pipeline handler to match this ? Or modify the documentation ?\n> >\n> \n> FYI raspberrypi and simple behave like IPU3, if I'm not mistaken.\n\nI looked at raspberrypi while doing this. They return invalid/nullptr if\nmore streams than supported are requested ((rawCount > 1 || outCount > 2)),\nso this was meant to be analogous to that.\n\n> I'm debated, I like the idea of returning a valid configuration, but\n> the Adjusted flag can be easily ignored and the application might find\n> itself dealing with something it doesn't expect.\n> \n> > > > \tCameraConfiguration *config = new RkISP1CameraConfiguration(camera, data);\n> > > > \tif (roles.empty())\n> > > > \t\treturn config;\n> > > >\n> > > > \tbool mainPathAvailable = true;\n> > > > -\tbool selfPathAvailable = true;\n> > > > +\tbool selfPathAvailable = data->selfPath_;\n> > > > \tfor (const StreamRole role : roles) {\n> > > > \t\tbool useMainPath;\n> > > >\n> > > > @@ -646,10 +656,12 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)\n> > > > \t\t\tret = mainPath_.configure(cfg, format);\n> > > > \t\t\tstreamConfig[0] = IPAStream(cfg.pixelFormat,\n> > > > \t\t\t\t\t\t cfg.size);\n> > > > -\t\t} else {\n> > > > +\t\t} else if (hasSelfPath_) {\n> > > > \t\t\tret = selfPath_.configure(cfg, format);\n> > > > \t\t\tstreamConfig[1] = IPAStream(cfg.pixelFormat,\n> > > > \t\t\t\t\t\t cfg.size);\n> > > > +\t\t} else {\n> > > > +\t\t\treturn -ENODEV;\n> > > > \t\t}\n> > >\n> > > This -shouldn't- be necessary. Configurations are validated before\n> > > being passed to Camera::configure() and the number of streams has\n> > > already been reduced to 1 if !selfPath, and the only stream available\n> > > should be the mainPath one.\n> > >\n> > > >\n> > > > \t\tif (ret)\n> > > > @@ -697,7 +709,7 @@ int PipelineHandlerRkISP1::exportFrameBuffers([[maybe_unused]] Camera *camera, S\n> > > >\n> > > > \tif (stream == &data->mainPathStream_)\n> > > > \t\treturn mainPath_.exportBuffers(count, buffers);\n> > > > -\telse if (stream == &data->selfPathStream_)\n> > > > +\telse if (hasSelfPath_ && stream == &data->selfPathStream_)\n> > > > \t\treturn selfPath_.exportBuffers(count, buffers);\n> > > >\n> > > > \treturn -EINVAL;\n> > > > @@ -826,7 +838,7 @@ int PipelineHandlerRkISP1::start(Camera *camera, [[maybe_unused]] const ControlL\n> > > > \t\t}\n> > > > \t}\n> > > >\n> > > > -\tif (data->selfPath_->isEnabled()) {\n> > > > +\tif (hasSelfPath_ && data->selfPath_->isEnabled()) {\n> > >\n> > > Same reasoning, a validate configuration shouldn't have selfPath_\n> > > enabled ? Have I missed how this can happen ?\n> >\n> > I think I commented in the review of the previous version that those two\n> > cases should never happen :-) I don't mind keeping the checks, it's up\n> > to you and Paul.\n> >\n> \n> Don't know, I don't feel strong on this. Generally avoiding testing\n> for conditions known to be false could save the reader from wondering why\n> the check is there and chase a code path that doesn't exist.\n> \n> But this is minimal so I'll let Paul decide.\n\nI was just being extra safe...\n\nPlus it's pushed anyway soooo...\n\n\nThanks,\n\nPaul\n\n> \n> \n> > > > \t\tret = selfPath_.start();\n> > > > \t\tif (ret) {\n> > > > \t\t\tmainPath_.stop();\n> > > > @@ -853,7 +865,8 @@ void PipelineHandlerRkISP1::stopDevice(Camera *camera)\n> > > >\n> > > > \tdata->ipa_->stop();\n> > > >\n> > > > -\tselfPath_.stop();\n> > > > +\tif (hasSelfPath_)\n> > > > +\t\tselfPath_.stop();\n> > > > \tmainPath_.stop();\n> > > >\n> > > > \tret = stat_->streamOff();\n> > > > @@ -934,7 +947,7 @@ int PipelineHandlerRkISP1::initLinks(Camera *camera,\n> > > > \tfor (const StreamConfiguration &cfg : config) {\n> > > > \t\tif (cfg.stream() == &data->mainPathStream_)\n> > > > \t\t\tret = data->mainPath_->setEnabled(true);\n> > > > -\t\telse if (cfg.stream() == &data->selfPathStream_)\n> > > > +\t\telse if (hasSelfPath_ && cfg.stream() == &data->selfPathStream_)\n> > > > \t\t\tret = data->selfPath_->setEnabled(true);\n> > > > \t\telse\n> > > > \t\t\treturn -EINVAL;\n> > > > @@ -951,7 +964,8 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)\n> > > > \tint ret;\n> > > >\n> > > > \tstd::unique_ptr data =\n> > > > -\t\tstd::make_unique(this, &mainPath_, &selfPath_);\n> > > > +\t\tstd::make_unique(this, &mainPath_,\n> > > > +\t\t\t\t\t\t hasSelfPath_ ? &selfPath_ : nullptr);\n> > > >\n> > > > \tControlInfoMap::Map ctrls;\n> > > > \tctrls.emplace(std::piecewise_construct,\n> > > > @@ -1007,9 +1021,7 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)\n> > > >\n> > > > \tDeviceMatch dm(\"rkisp1\");\n> > > > \tdm.add(\"rkisp1_isp\");\n> > > > -\tdm.add(\"rkisp1_resizer_selfpath\");\n> > > > \tdm.add(\"rkisp1_resizer_mainpath\");\n> > > > -\tdm.add(\"rkisp1_selfpath\");\n> > >\n> > > It's a shame we can't verify in advance if the platform has a\n> > > self-path and add the above entities to the DeviceMatch conditionally.\n> >\n> > True, but I don't think there's a risk of a false positive match here\n> > when removing the self path.\n> >\n> \n> Yeah, and unless we introduce something similar to\n> device_get_match_data() where to hardcode that information there's no\n> way we can know that in advance.\n> \n> Thanks\n> j\n> \n> > > > \tdm.add(\"rkisp1_mainpath\");\n> > > > \tdm.add(\"rkisp1_stats\");\n> > > > \tdm.add(\"rkisp1_params\");\n> > > > @@ -1024,6 +1036,8 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)\n> > > > \t\treturn false;\n> > > > \t}\n> > > >\n> > > > +\thasSelfPath_ = !!media_->getEntityByName(\"rkisp1_selfpath\");\n> > > > +\n> > > > \t/* Create the V4L2 subdevices we will need. */\n> > > > \tisp_ = V4L2Subdevice::fromEntityName(media_, \"rkisp1_isp\");\n> > > > \tif (isp_->open() < 0)\n> > > > @@ -1058,11 +1072,12 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)\n> > > > \tif (!mainPath_.init(media_))\n> > > > \t\treturn false;\n> > > >\n> > > > -\tif (!selfPath_.init(media_))\n> > > > +\tif (hasSelfPath_ && !selfPath_.init(media_))\n> > > > \t\treturn false;\n> > > >\n> > > > \tmainPath_.bufferReady().connect(this, &PipelineHandlerRkISP1::bufferReady);\n> > > > -\tselfPath_.bufferReady().connect(this, &PipelineHandlerRkISP1::bufferReady);\n> > > > +\tif (hasSelfPath_)\n> > > > +\t\tselfPath_.bufferReady().connect(this, &PipelineHandlerRkISP1::bufferReady);\n> > > > \tstat_->bufferReady.connect(this, &PipelineHandlerRkISP1::statReady);\n> > > > \tparam_->bufferReady.connect(this, &PipelineHandlerRkISP1::paramReady);\n> > > >\n> >\n> > --\n> > Regards,\n> >\n> > Laurent Pinchart","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 72873BD1F1\n\tfor ;\n\tWed, 20 Jul 2022 09:19:59 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id F241E63313;\n\tWed, 20 Jul 2022 11:19:58 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 27A4560488\n\tfor ;\n\tWed, 20 Jul 2022 11:19:58 +0200 (CEST)","from pyrite.rasen.tech (softbank036240121080.bbtec.net\n\t[36.240.121.80])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 492636DB;\n\tWed, 20 Jul 2022 11:19:55 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1658308799;\n\tbh=U5C9ufUvRVGGZAmcdWM/B/nLpKipKpwlGRJIiu1Ic7w=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=aiYnt/oRjPrhVYtg3U6ggAP/uk1LZRiBEi0QMbt9RIWjiSPTeazZFObyJR7jKLFL/\n\tdJQqOPye804LCIHJhvQoAeVrrhpmVOIGoRpKgyUMIzlr65kZ3fD+qTIj5XNz67ebx/\n\t6yOL1tguZtmER5Cht2ZxMQeSOm1f2PyvbxRzkBQURGULVtInhcfkaatDkTiK8U0gJw\n\tVtw7vAccMuoSm/Y2+iztVOT2RAiIZ3meAwTaMFk4KdbixzgYhv63dblm+Fq+yqVvVx\n\tunv0S65eW+MZWPRrDuemcctby+CTfHg4fnT6nIQqP957V96pjpdWvRjVRgajH+ur5v\n\twpGixMRlx2gUw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1658308797;\n\tbh=U5C9ufUvRVGGZAmcdWM/B/nLpKipKpwlGRJIiu1Ic7w=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=KApqw5MDTT/kt0glvKBHR0ZOcuhNZoh0Ecx1MmUTXg7n/ue0tCXNkthnMwjpBfvA1\n\tpmrP6cbXuwDZFOqQi4o1XibBFg9cZy7Su7CSKkrGjTP0RS+7rRF/aCZjfbO8ztwN1c\n\tcGycy7doLjrZeWuSvrx/x2E3I2EKt1T0EJXSYE9c="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"KApqw5MD\"; dkim-atps=neutral","Date":"Wed, 20 Jul 2022 18:19:51 +0900","To":"Jacopo Mondi ","Message-ID":"<20220720091951.GD3984498@pyrite.rasen.tech>","References":"<20220719073012.810184-1-paul.elder@ideasonboard.com>\n\t<20220720083657.myrlba2n2rlxayep@uno.localdomain>\n\t\n\t<20220720090704.af2ajf2izr5xvepz@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20220720090704.af2ajf2izr5xvepz@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v3] pipeline: rkisp1: Support devices\n\twithout self path","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Paul Elder via libcamera-devel ","Reply-To":"paul.elder@ideasonboard.com","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":23996,"web_url":"https://patchwork.libcamera.org/comment/23996/","msgid":"","date":"2022-07-20T09:32:57","subject":"Re: [libcamera-devel] [PATCH v3] pipeline: rkisp1: Support devices\n\twithout self path","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Wed, Jul 20, 2022 at 11:07:04AM +0200, Jacopo Mondi wrote:\n> On Wed, Jul 20, 2022 at 11:53:56AM +0300, Laurent Pinchart wrote:\n> > On Wed, Jul 20, 2022 at 10:36:57AM +0200, Jacopo Mondi via libcamera-devel wrote:\n> > > On Tue, Jul 19, 2022 at 04:30:12PM +0900, Paul Elder via libcamera-devel wrote:\n> > > > Some hardware supported by the rkisp1 driver, such as the ISP in the\n> > > > i.MX8MP, don't have a self path. Although at the moment the driver still\n> > > > exposes the self path, prepare the rkisp1 pipeline handler for when the\n> > > > self path will be removed for devices that don't support it.\n> > > >\n> > > > Signed-off-by: Paul Elder \n> > > >\n> > > > ---\n> > > > Changes in v3:\n> > > > - add more guards\n> > > > - add pathCount guard to generateConfiguration to allow making self path\n> > > > unavailable Just Work\n> > > >\n> > > > Changes in v2:\n> > > > - simplify matching\n> > > > - clean up self path availability in validate()\n> > > > - fix configure(), return -ENODEV if multiple roles but no self path\n> > > > ---\n> > > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 49 ++++++++++++++++--------\n> > > > 1 file changed, 32 insertions(+), 17 deletions(-)\n> > > >\n> > > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > > index 212fc76a..99eecd44 100644\n> > > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > > @@ -180,6 +180,8 @@ private:\n> > > > \tstd::unique_ptr stat_;\n> > > > \tstd::unique_ptr csi_;\n> > > >\n> > > > +\tbool hasSelfPath_;\n> > > > +\n> > > > \tRkISP1MainPath mainPath_;\n> > > > \tRkISP1SelfPath selfPath_;\n> > > >\n> > > > @@ -364,7 +366,7 @@ void RkISP1CameraData::paramFilled(unsigned int frame)\n> > > > \tif (info->mainPathBuffer)\n> > > > \t\tmainPath_->queueBuffer(info->mainPathBuffer);\n> > > >\n> > > > -\tif (info->selfPathBuffer)\n> > > > +\tif (selfPath_ && info->selfPathBuffer)\n> > > > \t\tselfPath_->queueBuffer(info->selfPathBuffer);\n> > > > }\n> > > >\n> > > > @@ -403,7 +405,7 @@ bool RkISP1CameraConfiguration::fitsAllPaths(const StreamConfiguration &cfg)\n> > > > \t\treturn false;\n> > > >\n> > > > \tconfig = cfg;\n> > > > -\tif (data_->selfPath_->validate(&config) != Valid)\n> > > > +\tif (data_->selfPath_ && data_->selfPath_->validate(&config) != Valid)\n> > > > \t\treturn false;\n> > > >\n> > > > \treturn true;\n> > > > @@ -412,6 +414,7 @@ bool RkISP1CameraConfiguration::fitsAllPaths(const StreamConfiguration &cfg)\n> > > > CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> > > > {\n> > > > \tconst CameraSensor *sensor = data_->sensor_.get();\n> > > > +\tunsigned int pathCount = data_->selfPath_ ? 2 : 1;\n> > > > \tStatus status = Valid;\n> > > >\n> > > > \tif (config_.empty())\n> > > > @@ -423,8 +426,8 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> > > > \t}\n> > > >\n> > > > \t/* Cap the number of entries to the available streams. */\n> > > > -\tif (config_.size() > 2) {\n> > > > -\t\tconfig_.resize(2);\n> > > > +\tif (config_.size() > pathCount) {\n> > > > +\t\tconfig_.resize(pathCount);\n> > > > \t\tstatus = Adjusted;\n> > > > \t}\n> > > >\n> > > > @@ -441,7 +444,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> > > > \t\tstd::reverse(order.begin(), order.end());\n> > > >\n> > > > \tbool mainPathAvailable = true;\n> > > > -\tbool selfPathAvailable = true;\n> > > > +\tbool selfPathAvailable = data_->selfPath_;\n> > > > \tfor (unsigned int index : order) {\n> > > > \t\tStreamConfiguration &cfg = config_[index];\n> > > >\n> > > > @@ -520,7 +523,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> > > > }\n> > > >\n> > > > PipelineHandlerRkISP1::PipelineHandlerRkISP1(CameraManager *manager)\n> > > > -\t: PipelineHandler(manager)\n> > > > +\t: PipelineHandler(manager), hasSelfPath_(true)\n> > > > {\n> > > > }\n> > > >\n> > > > @@ -532,12 +535,19 @@ CameraConfiguration *PipelineHandlerRkISP1::generateConfiguration(Camera *camera\n> > > > \tconst StreamRoles &roles)\n> > > > {\n> > > > \tRkISP1CameraData *data = cameraData(camera);\n> > > > +\n> > > > +\tunsigned int pathCount = data->selfPath_ ? 2 : 1;\n> > > > +\tif (roles.size() > pathCount) {\n> > > > +\t\tLOG(RkISP1, Error) << \"Too many stream roles requested\";\n> > > > +\t\treturn nullptr;\n> > > > +\t}\n> > > > +\n> > >\n> > > If I'm not mistaken, the IPU3 works differently. It accepts all roles,\n> > > and then lets validate() to shrink it down to the number of actually\n> > > supported ones.\n> >\n> > I thought about this, but didn't check the IPU3 implementation when\n> > reviewing this patch. The generateConfiguration() documentation states\n> >\n> > * \\brief Generate a default camera configuration according to stream roles\n> > * \\param[in] roles A list of stream roles\n> > *\n> > * Generate a camera configuration for a set of desired stream roles. The caller\n> > * specifies a list of stream roles and the camera returns a configuration\n> > * containing suitable streams and their suggested default configurations. An\n> > * empty list of roles is valid, and will generate an empty configuration that\n> > * can be filled by the caller.\n> > *\n> > * \\context This function is \\threadsafe.\n> > *\n> > * \\return A CameraConfiguration if the requested roles can be satisfied, or a\n> > * null pointer otherwise. The ownership of the returned configuration is\n> > * passed to the caller.\n> >\n> > The expected behaviour isn't very explicitly documented, but the\n> > documentation of the return value hints that if the requested roles\n> > can't be satisfied, nullptr should be returned. Maybe we should modify\n> > the IPU3 pipeline handler to match this ? Or modify the documentation ?\n> \n> FYI raspberrypi and simple behave like IPU3, if I'm not mistaken.\n> \n> I'm debated, I like the idea of returning a valid configuration, but\n> the Adjusted flag can be easily ignored and the application might find\n> itself dealing with something it doesn't expect.\n\nI'm also debated. Note that there's no Adjusted flag for\ngenerateConfiguration(), only validate(). Applications can of course\ncheck the size of the returned configuration.\n\n> > > > \tCameraConfiguration *config = new RkISP1CameraConfiguration(camera, data);\n> > > > \tif (roles.empty())\n> > > > \t\treturn config;\n> > > >\n> > > > \tbool mainPathAvailable = true;\n> > > > -\tbool selfPathAvailable = true;\n> > > > +\tbool selfPathAvailable = data->selfPath_;\n> > > > \tfor (const StreamRole role : roles) {\n> > > > \t\tbool useMainPath;\n> > > >\n> > > > @@ -646,10 +656,12 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)\n> > > > \t\t\tret = mainPath_.configure(cfg, format);\n> > > > \t\t\tstreamConfig[0] = IPAStream(cfg.pixelFormat,\n> > > > \t\t\t\t\t\t cfg.size);\n> > > > -\t\t} else {\n> > > > +\t\t} else if (hasSelfPath_) {\n> > > > \t\t\tret = selfPath_.configure(cfg, format);\n> > > > \t\t\tstreamConfig[1] = IPAStream(cfg.pixelFormat,\n> > > > \t\t\t\t\t\t cfg.size);\n> > > > +\t\t} else {\n> > > > +\t\t\treturn -ENODEV;\n> > > > \t\t}\n> > >\n> > > This -shouldn't- be necessary. Configurations are validated before\n> > > being passed to Camera::configure() and the number of streams has\n> > > already been reduced to 1 if !selfPath, and the only stream available\n> > > should be the mainPath one.\n> > >\n> > > >\n> > > > \t\tif (ret)\n> > > > @@ -697,7 +709,7 @@ int PipelineHandlerRkISP1::exportFrameBuffers([[maybe_unused]] Camera *camera, S\n> > > >\n> > > > \tif (stream == &data->mainPathStream_)\n> > > > \t\treturn mainPath_.exportBuffers(count, buffers);\n> > > > -\telse if (stream == &data->selfPathStream_)\n> > > > +\telse if (hasSelfPath_ && stream == &data->selfPathStream_)\n> > > > \t\treturn selfPath_.exportBuffers(count, buffers);\n> > > >\n> > > > \treturn -EINVAL;\n> > > > @@ -826,7 +838,7 @@ int PipelineHandlerRkISP1::start(Camera *camera, [[maybe_unused]] const ControlL\n> > > > \t\t}\n> > > > \t}\n> > > >\n> > > > -\tif (data->selfPath_->isEnabled()) {\n> > > > +\tif (hasSelfPath_ && data->selfPath_->isEnabled()) {\n> > >\n> > > Same reasoning, a validate configuration shouldn't have selfPath_\n> > > enabled ? Have I missed how this can happen ?\n> >\n> > I think I commented in the review of the previous version that those two\n> > cases should never happen :-) I don't mind keeping the checks, it's up\n> > to you and Paul.\n> \n> Don't know, I don't feel strong on this. Generally avoiding testing\n> for conditions known to be false could save the reader from wondering why\n> the check is there and chase a code path that doesn't exist.\n> \n> But this is minimal so I'll let Paul decide.\n> \n> > > > \t\tret = selfPath_.start();\n> > > > \t\tif (ret) {\n> > > > \t\t\tmainPath_.stop();\n> > > > @@ -853,7 +865,8 @@ void PipelineHandlerRkISP1::stopDevice(Camera *camera)\n> > > >\n> > > > \tdata->ipa_->stop();\n> > > >\n> > > > -\tselfPath_.stop();\n> > > > +\tif (hasSelfPath_)\n> > > > +\t\tselfPath_.stop();\n> > > > \tmainPath_.stop();\n> > > >\n> > > > \tret = stat_->streamOff();\n> > > > @@ -934,7 +947,7 @@ int PipelineHandlerRkISP1::initLinks(Camera *camera,\n> > > > \tfor (const StreamConfiguration &cfg : config) {\n> > > > \t\tif (cfg.stream() == &data->mainPathStream_)\n> > > > \t\t\tret = data->mainPath_->setEnabled(true);\n> > > > -\t\telse if (cfg.stream() == &data->selfPathStream_)\n> > > > +\t\telse if (hasSelfPath_ && cfg.stream() == &data->selfPathStream_)\n> > > > \t\t\tret = data->selfPath_->setEnabled(true);\n> > > > \t\telse\n> > > > \t\t\treturn -EINVAL;\n> > > > @@ -951,7 +964,8 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)\n> > > > \tint ret;\n> > > >\n> > > > \tstd::unique_ptr data =\n> > > > -\t\tstd::make_unique(this, &mainPath_, &selfPath_);\n> > > > +\t\tstd::make_unique(this, &mainPath_,\n> > > > +\t\t\t\t\t\t hasSelfPath_ ? &selfPath_ : nullptr);\n> > > >\n> > > > \tControlInfoMap::Map ctrls;\n> > > > \tctrls.emplace(std::piecewise_construct,\n> > > > @@ -1007,9 +1021,7 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)\n> > > >\n> > > > \tDeviceMatch dm(\"rkisp1\");\n> > > > \tdm.add(\"rkisp1_isp\");\n> > > > -\tdm.add(\"rkisp1_resizer_selfpath\");\n> > > > \tdm.add(\"rkisp1_resizer_mainpath\");\n> > > > -\tdm.add(\"rkisp1_selfpath\");\n> > >\n> > > It's a shame we can't verify in advance if the platform has a\n> > > self-path and add the above entities to the DeviceMatch conditionally.\n> >\n> > True, but I don't think there's a risk of a false positive match here\n> > when removing the self path.\n> \n> Yeah, and unless we introduce something similar to\n> device_get_match_data() where to hardcode that information there's no\n> way we can know that in advance.\n> \n> > > > \tdm.add(\"rkisp1_mainpath\");\n> > > > \tdm.add(\"rkisp1_stats\");\n> > > > \tdm.add(\"rkisp1_params\");\n> > > > @@ -1024,6 +1036,8 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)\n> > > > \t\treturn false;\n> > > > \t}\n> > > >\n> > > > +\thasSelfPath_ = !!media_->getEntityByName(\"rkisp1_selfpath\");\n> > > > +\n> > > > \t/* Create the V4L2 subdevices we will need. */\n> > > > \tisp_ = V4L2Subdevice::fromEntityName(media_, \"rkisp1_isp\");\n> > > > \tif (isp_->open() < 0)\n> > > > @@ -1058,11 +1072,12 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)\n> > > > \tif (!mainPath_.init(media_))\n> > > > \t\treturn false;\n> > > >\n> > > > -\tif (!selfPath_.init(media_))\n> > > > +\tif (hasSelfPath_ && !selfPath_.init(media_))\n> > > > \t\treturn false;\n> > > >\n> > > > \tmainPath_.bufferReady().connect(this, &PipelineHandlerRkISP1::bufferReady);\n> > > > -\tselfPath_.bufferReady().connect(this, &PipelineHandlerRkISP1::bufferReady);\n> > > > +\tif (hasSelfPath_)\n> > > > +\t\tselfPath_.bufferReady().connect(this, &PipelineHandlerRkISP1::bufferReady);\n> > > > \tstat_->bufferReady.connect(this, &PipelineHandlerRkISP1::statReady);\n> > > > \tparam_->bufferReady.connect(this, &PipelineHandlerRkISP1::paramReady);\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 3E534BE173\n\tfor ;\n\tWed, 20 Jul 2022 09:33:34 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8836D63313;\n\tWed, 20 Jul 2022 11:33:33 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8C9D560488\n\tfor ;\n\tWed, 20 Jul 2022 11:33:32 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id DB6146DB;\n\tWed, 20 Jul 2022 11:33:31 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1658309613;\n\tbh=UvzBR2iAn7euoU4Zub6Dejms64pxY7gm1EKiVTYwZcM=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=z6QJh0oYJr2NTXJ55djzmFBPtOd2gACQ7sUgt3cCOuxTrzsfdwOFpR6DCN+QSfFww\n\tKlFJ5+S87HTIDUuRY0ksueNKiaT6VmLk8EtKNv7oIOxI01AgFPaFrrJ87J8TdBn2Hb\n\tzY6yDcmLThFP8P2jR0R5OCPlzgofNZQM8YL9PQaeuU1QeBT9hJ5hLGb1sJkNdH5R9z\n\tS1QcjzRnGRvuDJuApMcI/FLRcSEvher/8Js8lSwZ+UUphXGwlFam63pohTCs4R5pao\n\t8Pnra/0KITkHfb4VHzkcEBxMvMy/LfaHUz6ecj6E9R58IhWJ/TwYqHdmH9d9SyqUs8\n\tBqHoFrR6U+KHg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1658309612;\n\tbh=UvzBR2iAn7euoU4Zub6Dejms64pxY7gm1EKiVTYwZcM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=UNJ4Tr0i8hwfZtfnzH/Km8O8mrOD1QPhMC+h88jNiozcwFtJoAFGHQ5eyldRCaODM\n\t/dckqn/xIudoPNEfZFGlvYjPLgPp8mCXPeADkOtJ8P8OMFEPSfM5yemNeAqNwC3LTa\n\tH0W4naO2WmVx2nLHpjSms/fRJLlGiA/7E9zSS//c="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"UNJ4Tr0i\"; dkim-atps=neutral","Date":"Wed, 20 Jul 2022 12:32:57 +0300","To":"Jacopo Mondi ","Message-ID":"","References":"<20220719073012.810184-1-paul.elder@ideasonboard.com>\n\t<20220720083657.myrlba2n2rlxayep@uno.localdomain>\n\t\n\t<20220720090704.af2ajf2izr5xvepz@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220720090704.af2ajf2izr5xvepz@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v3] pipeline: rkisp1: Support devices\n\twithout self path","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":23997,"web_url":"https://patchwork.libcamera.org/comment/23997/","msgid":"","date":"2022-07-20T09:34:13","subject":"Re: [libcamera-devel] [PATCH v3] pipeline: rkisp1: Support devices\n\twithout self path","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Wed, Jul 20, 2022 at 06:19:51PM +0900, paul.elder@ideasonboard.com wrote:\n> On Wed, Jul 20, 2022 at 11:07:04AM +0200, Jacopo Mondi wrote:\n> > On Wed, Jul 20, 2022 at 11:53:56AM +0300, Laurent Pinchart wrote:\n> > > On Wed, Jul 20, 2022 at 10:36:57AM +0200, Jacopo Mondi via libcamera-devel wrote:\n> > > > On Tue, Jul 19, 2022 at 04:30:12PM +0900, Paul Elder via libcamera-devel wrote:\n> > > > > Some hardware supported by the rkisp1 driver, such as the ISP in the\n> > > > > i.MX8MP, don't have a self path. Although at the moment the driver still\n> > > > > exposes the self path, prepare the rkisp1 pipeline handler for when the\n> > > > > self path will be removed for devices that don't support it.\n> > > > >\n> > > > > Signed-off-by: Paul Elder \n> > > > >\n> > > > > ---\n> > > > > Changes in v3:\n> > > > > - add more guards\n> > > > > - add pathCount guard to generateConfiguration to allow making self path\n> > > > > unavailable Just Work\n> > > > >\n> > > > > Changes in v2:\n> > > > > - simplify matching\n> > > > > - clean up self path availability in validate()\n> > > > > - fix configure(), return -ENODEV if multiple roles but no self path\n> > > > > ---\n> > > > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 49 ++++++++++++++++--------\n> > > > > 1 file changed, 32 insertions(+), 17 deletions(-)\n> > > > >\n> > > > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > > > index 212fc76a..99eecd44 100644\n> > > > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > > > @@ -180,6 +180,8 @@ private:\n> > > > > \tstd::unique_ptr stat_;\n> > > > > \tstd::unique_ptr csi_;\n> > > > >\n> > > > > +\tbool hasSelfPath_;\n> > > > > +\n> > > > > \tRkISP1MainPath mainPath_;\n> > > > > \tRkISP1SelfPath selfPath_;\n> > > > >\n> > > > > @@ -364,7 +366,7 @@ void RkISP1CameraData::paramFilled(unsigned int frame)\n> > > > > \tif (info->mainPathBuffer)\n> > > > > \t\tmainPath_->queueBuffer(info->mainPathBuffer);\n> > > > >\n> > > > > -\tif (info->selfPathBuffer)\n> > > > > +\tif (selfPath_ && info->selfPathBuffer)\n> > > > > \t\tselfPath_->queueBuffer(info->selfPathBuffer);\n> > > > > }\n> > > > >\n> > > > > @@ -403,7 +405,7 @@ bool RkISP1CameraConfiguration::fitsAllPaths(const StreamConfiguration &cfg)\n> > > > > \t\treturn false;\n> > > > >\n> > > > > \tconfig = cfg;\n> > > > > -\tif (data_->selfPath_->validate(&config) != Valid)\n> > > > > +\tif (data_->selfPath_ && data_->selfPath_->validate(&config) != Valid)\n> > > > > \t\treturn false;\n> > > > >\n> > > > > \treturn true;\n> > > > > @@ -412,6 +414,7 @@ bool RkISP1CameraConfiguration::fitsAllPaths(const StreamConfiguration &cfg)\n> > > > > CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> > > > > {\n> > > > > \tconst CameraSensor *sensor = data_->sensor_.get();\n> > > > > +\tunsigned int pathCount = data_->selfPath_ ? 2 : 1;\n> > > > > \tStatus status = Valid;\n> > > > >\n> > > > > \tif (config_.empty())\n> > > > > @@ -423,8 +426,8 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> > > > > \t}\n> > > > >\n> > > > > \t/* Cap the number of entries to the available streams. */\n> > > > > -\tif (config_.size() > 2) {\n> > > > > -\t\tconfig_.resize(2);\n> > > > > +\tif (config_.size() > pathCount) {\n> > > > > +\t\tconfig_.resize(pathCount);\n> > > > > \t\tstatus = Adjusted;\n> > > > > \t}\n> > > > >\n> > > > > @@ -441,7 +444,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> > > > > \t\tstd::reverse(order.begin(), order.end());\n> > > > >\n> > > > > \tbool mainPathAvailable = true;\n> > > > > -\tbool selfPathAvailable = true;\n> > > > > +\tbool selfPathAvailable = data_->selfPath_;\n> > > > > \tfor (unsigned int index : order) {\n> > > > > \t\tStreamConfiguration &cfg = config_[index];\n> > > > >\n> > > > > @@ -520,7 +523,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> > > > > }\n> > > > >\n> > > > > PipelineHandlerRkISP1::PipelineHandlerRkISP1(CameraManager *manager)\n> > > > > -\t: PipelineHandler(manager)\n> > > > > +\t: PipelineHandler(manager), hasSelfPath_(true)\n> > > > > {\n> > > > > }\n> > > > >\n> > > > > @@ -532,12 +535,19 @@ CameraConfiguration *PipelineHandlerRkISP1::generateConfiguration(Camera *camera\n> > > > > \tconst StreamRoles &roles)\n> > > > > {\n> > > > > \tRkISP1CameraData *data = cameraData(camera);\n> > > > > +\n> > > > > +\tunsigned int pathCount = data->selfPath_ ? 2 : 1;\n> > > > > +\tif (roles.size() > pathCount) {\n> > > > > +\t\tLOG(RkISP1, Error) << \"Too many stream roles requested\";\n> > > > > +\t\treturn nullptr;\n> > > > > +\t}\n> > > > > +\n> > > >\n> > > > If I'm not mistaken, the IPU3 works differently. It accepts all roles,\n> > > > and then lets validate() to shrink it down to the number of actually\n> > > > supported ones.\n> > >\n> > > I thought about this, but didn't check the IPU3 implementation when\n> > > reviewing this patch. The generateConfiguration() documentation states\n> > >\n> > > * \\brief Generate a default camera configuration according to stream roles\n> > > * \\param[in] roles A list of stream roles\n> > > *\n> > > * Generate a camera configuration for a set of desired stream roles. The caller\n> > > * specifies a list of stream roles and the camera returns a configuration\n> > > * containing suitable streams and their suggested default configurations. An\n> > > * empty list of roles is valid, and will generate an empty configuration that\n> > > * can be filled by the caller.\n> > > *\n> > > * \\context This function is \\threadsafe.\n> > > *\n> > > * \\return A CameraConfiguration if the requested roles can be satisfied, or a\n> > > * null pointer otherwise. The ownership of the returned configuration is\n> > > * passed to the caller.\n> > >\n> > > The expected behaviour isn't very explicitly documented, but the\n> > > documentation of the return value hints that if the requested roles\n> > > can't be satisfied, nullptr should be returned. Maybe we should modify\n> > > the IPU3 pipeline handler to match this ? Or modify the documentation ?\n> > >\n> > \n> > FYI raspberrypi and simple behave like IPU3, if I'm not mistaken.\n> \n> I looked at raspberrypi while doing this. They return invalid/nullptr if\n> more streams than supported are requested ((rawCount > 1 || outCount > 2)),\n> so this was meant to be analogous to that.\n> \n> > I'm debated, I like the idea of returning a valid configuration, but\n> > the Adjusted flag can be easily ignored and the application might find\n> > itself dealing with something it doesn't expect.\n> > \n> > > > > \tCameraConfiguration *config = new RkISP1CameraConfiguration(camera, data);\n> > > > > \tif (roles.empty())\n> > > > > \t\treturn config;\n> > > > >\n> > > > > \tbool mainPathAvailable = true;\n> > > > > -\tbool selfPathAvailable = true;\n> > > > > +\tbool selfPathAvailable = data->selfPath_;\n> > > > > \tfor (const StreamRole role : roles) {\n> > > > > \t\tbool useMainPath;\n> > > > >\n> > > > > @@ -646,10 +656,12 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)\n> > > > > \t\t\tret = mainPath_.configure(cfg, format);\n> > > > > \t\t\tstreamConfig[0] = IPAStream(cfg.pixelFormat,\n> > > > > \t\t\t\t\t\t cfg.size);\n> > > > > -\t\t} else {\n> > > > > +\t\t} else if (hasSelfPath_) {\n> > > > > \t\t\tret = selfPath_.configure(cfg, format);\n> > > > > \t\t\tstreamConfig[1] = IPAStream(cfg.pixelFormat,\n> > > > > \t\t\t\t\t\t cfg.size);\n> > > > > +\t\t} else {\n> > > > > +\t\t\treturn -ENODEV;\n> > > > > \t\t}\n> > > >\n> > > > This -shouldn't- be necessary. Configurations are validated before\n> > > > being passed to Camera::configure() and the number of streams has\n> > > > already been reduced to 1 if !selfPath, and the only stream available\n> > > > should be the mainPath one.\n> > > >\n> > > > >\n> > > > > \t\tif (ret)\n> > > > > @@ -697,7 +709,7 @@ int PipelineHandlerRkISP1::exportFrameBuffers([[maybe_unused]] Camera *camera, S\n> > > > >\n> > > > > \tif (stream == &data->mainPathStream_)\n> > > > > \t\treturn mainPath_.exportBuffers(count, buffers);\n> > > > > -\telse if (stream == &data->selfPathStream_)\n> > > > > +\telse if (hasSelfPath_ && stream == &data->selfPathStream_)\n> > > > > \t\treturn selfPath_.exportBuffers(count, buffers);\n> > > > >\n> > > > > \treturn -EINVAL;\n> > > > > @@ -826,7 +838,7 @@ int PipelineHandlerRkISP1::start(Camera *camera, [[maybe_unused]] const ControlL\n> > > > > \t\t}\n> > > > > \t}\n> > > > >\n> > > > > -\tif (data->selfPath_->isEnabled()) {\n> > > > > +\tif (hasSelfPath_ && data->selfPath_->isEnabled()) {\n> > > >\n> > > > Same reasoning, a validate configuration shouldn't have selfPath_\n> > > > enabled ? Have I missed how this can happen ?\n> > >\n> > > I think I commented in the review of the previous version that those two\n> > > cases should never happen :-) I don't mind keeping the checks, it's up\n> > > to you and Paul.\n> > >\n> > \n> > Don't know, I don't feel strong on this. Generally avoiding testing\n> > for conditions known to be false could save the reader from wondering why\n> > the check is there and chase a code path that doesn't exist.\n> > \n> > But this is minimal so I'll let Paul decide.\n> \n> I was just being extra safe...\n> \n> Plus it's pushed anyway soooo...\n\nSoooo... it could be addressed in subsequent patches if needed :-) I\ndon't call for that though.\n\n> > > > > \t\tret = selfPath_.start();\n> > > > > \t\tif (ret) {\n> > > > > \t\t\tmainPath_.stop();\n> > > > > @@ -853,7 +865,8 @@ void PipelineHandlerRkISP1::stopDevice(Camera *camera)\n> > > > >\n> > > > > \tdata->ipa_->stop();\n> > > > >\n> > > > > -\tselfPath_.stop();\n> > > > > +\tif (hasSelfPath_)\n> > > > > +\t\tselfPath_.stop();\n> > > > > \tmainPath_.stop();\n> > > > >\n> > > > > \tret = stat_->streamOff();\n> > > > > @@ -934,7 +947,7 @@ int PipelineHandlerRkISP1::initLinks(Camera *camera,\n> > > > > \tfor (const StreamConfiguration &cfg : config) {\n> > > > > \t\tif (cfg.stream() == &data->mainPathStream_)\n> > > > > \t\t\tret = data->mainPath_->setEnabled(true);\n> > > > > -\t\telse if (cfg.stream() == &data->selfPathStream_)\n> > > > > +\t\telse if (hasSelfPath_ && cfg.stream() == &data->selfPathStream_)\n> > > > > \t\t\tret = data->selfPath_->setEnabled(true);\n> > > > > \t\telse\n> > > > > \t\t\treturn -EINVAL;\n> > > > > @@ -951,7 +964,8 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)\n> > > > > \tint ret;\n> > > > >\n> > > > > \tstd::unique_ptr data =\n> > > > > -\t\tstd::make_unique(this, &mainPath_, &selfPath_);\n> > > > > +\t\tstd::make_unique(this, &mainPath_,\n> > > > > +\t\t\t\t\t\t hasSelfPath_ ? &selfPath_ : nullptr);\n> > > > >\n> > > > > \tControlInfoMap::Map ctrls;\n> > > > > \tctrls.emplace(std::piecewise_construct,\n> > > > > @@ -1007,9 +1021,7 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)\n> > > > >\n> > > > > \tDeviceMatch dm(\"rkisp1\");\n> > > > > \tdm.add(\"rkisp1_isp\");\n> > > > > -\tdm.add(\"rkisp1_resizer_selfpath\");\n> > > > > \tdm.add(\"rkisp1_resizer_mainpath\");\n> > > > > -\tdm.add(\"rkisp1_selfpath\");\n> > > >\n> > > > It's a shame we can't verify in advance if the platform has a\n> > > > self-path and add the above entities to the DeviceMatch conditionally.\n> > >\n> > > True, but I don't think there's a risk of a false positive match here\n> > > when removing the self path.\n> > \n> > Yeah, and unless we introduce something similar to\n> > device_get_match_data() where to hardcode that information there's no\n> > way we can know that in advance.\n> > \n> > > > > \tdm.add(\"rkisp1_mainpath\");\n> > > > > \tdm.add(\"rkisp1_stats\");\n> > > > > \tdm.add(\"rkisp1_params\");\n> > > > > @@ -1024,6 +1036,8 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)\n> > > > > \t\treturn false;\n> > > > > \t}\n> > > > >\n> > > > > +\thasSelfPath_ = !!media_->getEntityByName(\"rkisp1_selfpath\");\n> > > > > +\n> > > > > \t/* Create the V4L2 subdevices we will need. */\n> > > > > \tisp_ = V4L2Subdevice::fromEntityName(media_, \"rkisp1_isp\");\n> > > > > \tif (isp_->open() < 0)\n> > > > > @@ -1058,11 +1072,12 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)\n> > > > > \tif (!mainPath_.init(media_))\n> > > > > \t\treturn false;\n> > > > >\n> > > > > -\tif (!selfPath_.init(media_))\n> > > > > +\tif (hasSelfPath_ && !selfPath_.init(media_))\n> > > > > \t\treturn false;\n> > > > >\n> > > > > \tmainPath_.bufferReady().connect(this, &PipelineHandlerRkISP1::bufferReady);\n> > > > > -\tselfPath_.bufferReady().connect(this, &PipelineHandlerRkISP1::bufferReady);\n> > > > > +\tif (hasSelfPath_)\n> > > > > +\t\tselfPath_.bufferReady().connect(this, &PipelineHandlerRkISP1::bufferReady);\n> > > > > \tstat_->bufferReady.connect(this, &PipelineHandlerRkISP1::statReady);\n> > > > > \tparam_->bufferReady.connect(this, &PipelineHandlerRkISP1::paramReady);\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 EDF3DBE173\n\tfor ;\n\tWed, 20 Jul 2022 09:34:49 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9F8D863313;\n\tWed, 20 Jul 2022 11:34:49 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id EFA7260488\n\tfor ;\n\tWed, 20 Jul 2022 11:34:47 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 8691D6DB;\n\tWed, 20 Jul 2022 11:34:47 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1658309689;\n\tbh=DOyvaLgVhWR4jtvZxmgngG5ZvC/07lPjN1XLJ5lBZXw=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=q1Bp9k6p0JKX7wig6RA5whIqiWEx6tSDX/2NwWbBYAobtK2tTPHUyeKKRqyP9MW69\n\t9fJE5Z+tRUmDiDsbZ31k+J8nuFwde7YxmELlQwrjjYwZHLi/1p2akUcvR+PlJjR5cn\n\t7Qgr7Hama6s1h8axCXDTqnI3xvx3g/Q5XK8MLiu5BtdK57wY+rTj5ZQvMIkQy8/Ht6\n\thpz8vkdnuBQoPWiC61n6Ohj5TwMgNaz+vkq5EbAeFZpVvctZPZwS4XPtlGVmvUckdk\n\tzyOBj8gAjSR3zhjIoetXL6CqCOebngvXzt7kEJ8s2gCKaFRdsj7eO67a8uNR6VgnUz\n\txsFPRIovkdgEA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1658309687;\n\tbh=DOyvaLgVhWR4jtvZxmgngG5ZvC/07lPjN1XLJ5lBZXw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=WFlCX06Nl9n7wlO3bb1mboxvuqCkskEZschy8ZzikvJXXkBsDKa/AAzprvBGG8T0A\n\tqDzEr8ht4HeGBoNx748HAtFrzYr0qd0pZeE+3BbC52AQs9bitgyN52E1I3k7t1VyKU\n\t5PyAE4n/boZuLs+gMXiluWnh8vWZLMpdrvfQVnHs="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"WFlCX06N\"; dkim-atps=neutral","Date":"Wed, 20 Jul 2022 12:34:13 +0300","To":"paul.elder@ideasonboard.com","Message-ID":"","References":"<20220719073012.810184-1-paul.elder@ideasonboard.com>\n\t<20220720083657.myrlba2n2rlxayep@uno.localdomain>\n\t\n\t<20220720090704.af2ajf2izr5xvepz@uno.localdomain>\n\t<20220720091951.GD3984498@pyrite.rasen.tech>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220720091951.GD3984498@pyrite.rasen.tech>","Subject":"Re: [libcamera-devel] [PATCH v3] pipeline: rkisp1: Support devices\n\twithout self path","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":23998,"web_url":"https://patchwork.libcamera.org/comment/23998/","msgid":"<20220720094907.xv4x6k3v7fdxf5ge@uno.localdomain>","date":"2022-07-20T09:49:07","subject":"Re: [libcamera-devel] [PATCH v3] pipeline: rkisp1: Support devices\n\twithout self path","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Paul, Laurent,\n\nOn Wed, Jul 20, 2022 at 06:19:51PM +0900, paul.elder@ideasonboard.com wrote:\n> Hi,\n>\n> On Wed, Jul 20, 2022 at 11:07:04AM +0200, Jacopo Mondi wrote:\n> > Hi Laurent\n> >\n> > On Wed, Jul 20, 2022 at 11:53:56AM +0300, Laurent Pinchart wrote:\n> > > Hi Jacopo,\n> > >\n> > > On Wed, Jul 20, 2022 at 10:36:57AM +0200, Jacopo Mondi via libcamera-devel wrote:\n> > > > On Tue, Jul 19, 2022 at 04:30:12PM +0900, Paul Elder via libcamera-devel wrote:\n> > > > > Some hardware supported by the rkisp1 driver, such as the ISP in the\n> > > > > i.MX8MP, don't have a self path. Although at the moment the driver still\n> > > > > exposes the self path, prepare the rkisp1 pipeline handler for when the\n> > > > > self path will be removed for devices that don't support it.\n> > > > >\n> > > > > Signed-off-by: Paul Elder \n> > > > >\n> > > > > ---\n> > > > > Changes in v3:\n> > > > > - add more guards\n> > > > > - add pathCount guard to generateConfiguration to allow making self path\n> > > > > unavailable Just Work\n> > > > >\n> > > > > Changes in v2:\n> > > > > - simplify matching\n> > > > > - clean up self path availability in validate()\n> > > > > - fix configure(), return -ENODEV if multiple roles but no self path\n> > > > > ---\n> > > > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 49 ++++++++++++++++--------\n> > > > > 1 file changed, 32 insertions(+), 17 deletions(-)\n> > > > >\n> > > > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > > > index 212fc76a..99eecd44 100644\n> > > > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > > > @@ -180,6 +180,8 @@ private:\n> > > > > \tstd::unique_ptr stat_;\n> > > > > \tstd::unique_ptr csi_;\n> > > > >\n> > > > > +\tbool hasSelfPath_;\n> > > > > +\n> > > > > \tRkISP1MainPath mainPath_;\n> > > > > \tRkISP1SelfPath selfPath_;\n> > > > >\n> > > > > @@ -364,7 +366,7 @@ void RkISP1CameraData::paramFilled(unsigned int frame)\n> > > > > \tif (info->mainPathBuffer)\n> > > > > \t\tmainPath_->queueBuffer(info->mainPathBuffer);\n> > > > >\n> > > > > -\tif (info->selfPathBuffer)\n> > > > > +\tif (selfPath_ && info->selfPathBuffer)\n> > > > > \t\tselfPath_->queueBuffer(info->selfPathBuffer);\n> > > > > }\n> > > > >\n> > > > > @@ -403,7 +405,7 @@ bool RkISP1CameraConfiguration::fitsAllPaths(const StreamConfiguration &cfg)\n> > > > > \t\treturn false;\n> > > > >\n> > > > > \tconfig = cfg;\n> > > > > -\tif (data_->selfPath_->validate(&config) != Valid)\n> > > > > +\tif (data_->selfPath_ && data_->selfPath_->validate(&config) != Valid)\n> > > > > \t\treturn false;\n> > > > >\n> > > > > \treturn true;\n> > > > > @@ -412,6 +414,7 @@ bool RkISP1CameraConfiguration::fitsAllPaths(const StreamConfiguration &cfg)\n> > > > > CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> > > > > {\n> > > > > \tconst CameraSensor *sensor = data_->sensor_.get();\n> > > > > +\tunsigned int pathCount = data_->selfPath_ ? 2 : 1;\n> > > > > \tStatus status = Valid;\n> > > > >\n> > > > > \tif (config_.empty())\n> > > > > @@ -423,8 +426,8 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> > > > > \t}\n> > > > >\n> > > > > \t/* Cap the number of entries to the available streams. */\n> > > > > -\tif (config_.size() > 2) {\n> > > > > -\t\tconfig_.resize(2);\n> > > > > +\tif (config_.size() > pathCount) {\n> > > > > +\t\tconfig_.resize(pathCount);\n> > > > > \t\tstatus = Adjusted;\n> > > > > \t}\n> > > > >\n> > > > > @@ -441,7 +444,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> > > > > \t\tstd::reverse(order.begin(), order.end());\n> > > > >\n> > > > > \tbool mainPathAvailable = true;\n> > > > > -\tbool selfPathAvailable = true;\n> > > > > +\tbool selfPathAvailable = data_->selfPath_;\n> > > > > \tfor (unsigned int index : order) {\n> > > > > \t\tStreamConfiguration &cfg = config_[index];\n> > > > >\n> > > > > @@ -520,7 +523,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> > > > > }\n> > > > >\n> > > > > PipelineHandlerRkISP1::PipelineHandlerRkISP1(CameraManager *manager)\n> > > > > -\t: PipelineHandler(manager)\n> > > > > +\t: PipelineHandler(manager), hasSelfPath_(true)\n> > > > > {\n> > > > > }\n> > > > >\n> > > > > @@ -532,12 +535,19 @@ CameraConfiguration *PipelineHandlerRkISP1::generateConfiguration(Camera *camera\n> > > > > \tconst StreamRoles &roles)\n> > > > > {\n> > > > > \tRkISP1CameraData *data = cameraData(camera);\n> > > > > +\n> > > > > +\tunsigned int pathCount = data->selfPath_ ? 2 : 1;\n> > > > > +\tif (roles.size() > pathCount) {\n> > > > > +\t\tLOG(RkISP1, Error) << \"Too many stream roles requested\";\n> > > > > +\t\treturn nullptr;\n> > > > > +\t}\n> > > > > +\n> > > >\n> > > > If I'm not mistaken, the IPU3 works differently. It accepts all roles,\n> > > > and then lets validate() to shrink it down to the number of actually\n> > > > supported ones.\n> > >\n> > > I thought about this, but didn't check the IPU3 implementation when\n> > > reviewing this patch. The generateConfiguration() documentation states\n> > >\n> > > * \\brief Generate a default camera configuration according to stream roles\n> > > * \\param[in] roles A list of stream roles\n> > > *\n> > > * Generate a camera configuration for a set of desired stream roles. The caller\n> > > * specifies a list of stream roles and the camera returns a configuration\n> > > * containing suitable streams and their suggested default configurations. An\n> > > * empty list of roles is valid, and will generate an empty configuration that\n> > > * can be filled by the caller.\n> > > *\n> > > * \\context This function is \\threadsafe.\n> > > *\n> > > * \\return A CameraConfiguration if the requested roles can be satisfied, or a\n> > > * null pointer otherwise. The ownership of the returned configuration is\n> > > * passed to the caller.\n> > >\n> > > The expected behaviour isn't very explicitly documented, but the\n> > > documentation of the return value hints that if the requested roles\n> > > can't be satisfied, nullptr should be returned. Maybe we should modify\n> > > the IPU3 pipeline handler to match this ? Or modify the documentation ?\n> > >\n> >\n> > FYI raspberrypi and simple behave like IPU3, if I'm not mistaken.\n>\n> I looked at raspberrypi while doing this. They return invalid/nullptr if\n> more streams than supported are requested ((rawCount > 1 || outCount > 2)),\n> so this was meant to be analogous to that.\n>\n\nOh I missed\n\n\t\tif (rawCount > 1 || outCount > 2) {\n\t\t\tLOG(RPI, Error) << \"Invalid stream roles requested\";\n\t\t\tdelete config;\n\t\t\treturn nullptr;\n\t\t}\n\nIn generateConfiguration().\n\nAlso for IPU3, the configuration is not adjusted but rather Invalidated during\nvalidate():\n\n\tfor (const StreamConfiguration &cfg : config_) {\n\t\tconst PixelFormatInfo &info = PixelFormatInfo::info(cfg.pixelFormat);\n\n\t\tif (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW) {\n\t\t\trawCount++;\n\t\t\trawSize.expandTo(cfg.size);\n\t\t} else {\n\t\t\tyuvCount++;\n\t\t\tmaxYuvSize.expandTo(cfg.size);\n\t\t}\n\t}\n\n\tif (rawCount > 1 || yuvCount > 2) {\n\t\tLOG(IPU3, Debug) << \"Camera configuration not supported\";\n\t\treturn Invalid;\n }\n\nAnd generateConfiguration() detects that\n\n\tif (config->validate() == CameraConfiguration::Invalid)\n\t\treturn {};\n\nSo yeah, sorry for the noise\n\nWith all the previous comments clarified\nReviewed-by: Jacopo Mondi \n\nwhich doesn't matter as the patch is pushed :)\n\n\n> > I'm debated, I like the idea of returning a valid configuration, but\n> > the Adjusted flag can be easily ignored and the application might find\n> > itself dealing with something it doesn't expect.\n> >\n> > > > > \tCameraConfiguration *config = new RkISP1CameraConfiguration(camera, data);\n> > > > > \tif (roles.empty())\n> > > > > \t\treturn config;\n> > > > >\n> > > > > \tbool mainPathAvailable = true;\n> > > > > -\tbool selfPathAvailable = true;\n> > > > > +\tbool selfPathAvailable = data->selfPath_;\n> > > > > \tfor (const StreamRole role : roles) {\n> > > > > \t\tbool useMainPath;\n> > > > >\n> > > > > @@ -646,10 +656,12 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)\n> > > > > \t\t\tret = mainPath_.configure(cfg, format);\n> > > > > \t\t\tstreamConfig[0] = IPAStream(cfg.pixelFormat,\n> > > > > \t\t\t\t\t\t cfg.size);\n> > > > > -\t\t} else {\n> > > > > +\t\t} else if (hasSelfPath_) {\n> > > > > \t\t\tret = selfPath_.configure(cfg, format);\n> > > > > \t\t\tstreamConfig[1] = IPAStream(cfg.pixelFormat,\n> > > > > \t\t\t\t\t\t cfg.size);\n> > > > > +\t\t} else {\n> > > > > +\t\t\treturn -ENODEV;\n> > > > > \t\t}\n> > > >\n> > > > This -shouldn't- be necessary. Configurations are validated before\n> > > > being passed to Camera::configure() and the number of streams has\n> > > > already been reduced to 1 if !selfPath, and the only stream available\n> > > > should be the mainPath one.\n> > > >\n> > > > >\n> > > > > \t\tif (ret)\n> > > > > @@ -697,7 +709,7 @@ int PipelineHandlerRkISP1::exportFrameBuffers([[maybe_unused]] Camera *camera, S\n> > > > >\n> > > > > \tif (stream == &data->mainPathStream_)\n> > > > > \t\treturn mainPath_.exportBuffers(count, buffers);\n> > > > > -\telse if (stream == &data->selfPathStream_)\n> > > > > +\telse if (hasSelfPath_ && stream == &data->selfPathStream_)\n> > > > > \t\treturn selfPath_.exportBuffers(count, buffers);\n> > > > >\n> > > > > \treturn -EINVAL;\n> > > > > @@ -826,7 +838,7 @@ int PipelineHandlerRkISP1::start(Camera *camera, [[maybe_unused]] const ControlL\n> > > > > \t\t}\n> > > > > \t}\n> > > > >\n> > > > > -\tif (data->selfPath_->isEnabled()) {\n> > > > > +\tif (hasSelfPath_ && data->selfPath_->isEnabled()) {\n> > > >\n> > > > Same reasoning, a validate configuration shouldn't have selfPath_\n> > > > enabled ? Have I missed how this can happen ?\n> > >\n> > > I think I commented in the review of the previous version that those two\n> > > cases should never happen :-) I don't mind keeping the checks, it's up\n> > > to you and Paul.\n> > >\n> >\n> > Don't know, I don't feel strong on this. Generally avoiding testing\n> > for conditions known to be false could save the reader from wondering why\n> > the check is there and chase a code path that doesn't exist.\n> >\n> > But this is minimal so I'll let Paul decide.\n>\n> I was just being extra safe...\n>\n> Plus it's pushed anyway soooo...\n>\n>\n> Thanks,\n>\n> Paul\n>\n> >\n> >\n> > > > > \t\tret = selfPath_.start();\n> > > > > \t\tif (ret) {\n> > > > > \t\t\tmainPath_.stop();\n> > > > > @@ -853,7 +865,8 @@ void PipelineHandlerRkISP1::stopDevice(Camera *camera)\n> > > > >\n> > > > > \tdata->ipa_->stop();\n> > > > >\n> > > > > -\tselfPath_.stop();\n> > > > > +\tif (hasSelfPath_)\n> > > > > +\t\tselfPath_.stop();\n> > > > > \tmainPath_.stop();\n> > > > >\n> > > > > \tret = stat_->streamOff();\n> > > > > @@ -934,7 +947,7 @@ int PipelineHandlerRkISP1::initLinks(Camera *camera,\n> > > > > \tfor (const StreamConfiguration &cfg : config) {\n> > > > > \t\tif (cfg.stream() == &data->mainPathStream_)\n> > > > > \t\t\tret = data->mainPath_->setEnabled(true);\n> > > > > -\t\telse if (cfg.stream() == &data->selfPathStream_)\n> > > > > +\t\telse if (hasSelfPath_ && cfg.stream() == &data->selfPathStream_)\n> > > > > \t\t\tret = data->selfPath_->setEnabled(true);\n> > > > > \t\telse\n> > > > > \t\t\treturn -EINVAL;\n> > > > > @@ -951,7 +964,8 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)\n> > > > > \tint ret;\n> > > > >\n> > > > > \tstd::unique_ptr data =\n> > > > > -\t\tstd::make_unique(this, &mainPath_, &selfPath_);\n> > > > > +\t\tstd::make_unique(this, &mainPath_,\n> > > > > +\t\t\t\t\t\t hasSelfPath_ ? &selfPath_ : nullptr);\n> > > > >\n> > > > > \tControlInfoMap::Map ctrls;\n> > > > > \tctrls.emplace(std::piecewise_construct,\n> > > > > @@ -1007,9 +1021,7 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)\n> > > > >\n> > > > > \tDeviceMatch dm(\"rkisp1\");\n> > > > > \tdm.add(\"rkisp1_isp\");\n> > > > > -\tdm.add(\"rkisp1_resizer_selfpath\");\n> > > > > \tdm.add(\"rkisp1_resizer_mainpath\");\n> > > > > -\tdm.add(\"rkisp1_selfpath\");\n> > > >\n> > > > It's a shame we can't verify in advance if the platform has a\n> > > > self-path and add the above entities to the DeviceMatch conditionally.\n> > >\n> > > True, but I don't think there's a risk of a false positive match here\n> > > when removing the self path.\n> > >\n> >\n> > Yeah, and unless we introduce something similar to\n> > device_get_match_data() where to hardcode that information there's no\n> > way we can know that in advance.\n> >\n> > Thanks\n> > j\n> >\n> > > > > \tdm.add(\"rkisp1_mainpath\");\n> > > > > \tdm.add(\"rkisp1_stats\");\n> > > > > \tdm.add(\"rkisp1_params\");\n> > > > > @@ -1024,6 +1036,8 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)\n> > > > > \t\treturn false;\n> > > > > \t}\n> > > > >\n> > > > > +\thasSelfPath_ = !!media_->getEntityByName(\"rkisp1_selfpath\");\n> > > > > +\n> > > > > \t/* Create the V4L2 subdevices we will need. */\n> > > > > \tisp_ = V4L2Subdevice::fromEntityName(media_, \"rkisp1_isp\");\n> > > > > \tif (isp_->open() < 0)\n> > > > > @@ -1058,11 +1072,12 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)\n> > > > > \tif (!mainPath_.init(media_))\n> > > > > \t\treturn false;\n> > > > >\n> > > > > -\tif (!selfPath_.init(media_))\n> > > > > +\tif (hasSelfPath_ && !selfPath_.init(media_))\n> > > > > \t\treturn false;\n> > > > >\n> > > > > \tmainPath_.bufferReady().connect(this, &PipelineHandlerRkISP1::bufferReady);\n> > > > > -\tselfPath_.bufferReady().connect(this, &PipelineHandlerRkISP1::bufferReady);\n> > > > > +\tif (hasSelfPath_)\n> > > > > +\t\tselfPath_.bufferReady().connect(this, &PipelineHandlerRkISP1::bufferReady);\n> > > > > \tstat_->bufferReady.connect(this, &PipelineHandlerRkISP1::statReady);\n> > > > > \tparam_->bufferReady.connect(this, &PipelineHandlerRkISP1::paramReady);\n> > > > >\n> > >\n> > > --\n> > > Regards,\n> > >\n> > > Laurent Pinchart","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 F0E45BD1F1\n\tfor ;\n\tWed, 20 Jul 2022 09:49:10 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5A48063313;\n\tWed, 20 Jul 2022 11:49:10 +0200 (CEST)","from relay3-d.mail.gandi.net (relay3-d.mail.gandi.net\n\t[IPv6:2001:4b98:dc4:8::223])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6EAB560488\n\tfor ;\n\tWed, 20 Jul 2022 11:49:09 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby mail.gandi.net (Postfix) with ESMTPSA id 7E64560012;\n\tWed, 20 Jul 2022 09:49:08 +0000 (UTC)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1658310550;\n\tbh=ZuMBCDAIRvENDsI7hNBmxa3477usL6mT2d8etJsimTU=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=3TKR+uuCPagMOmE14b77APBj54E/TG/uFPdc7+yZ3/GwXYXIWv+Zuv0jKkIlc2hfZ\n\tCehOPhzjWPM1b7wE0NHzY9XoUnsL9gZY3E2k5tTXgCqXtZ/74U4/+XEzdyZL/PxIJv\n\tCQEnRSv2v2JPx3WLAjeNo+5WuRVMH00aKPzaxVuUbfW4BQyXogw3r+1v31r8DA+jna\n\tDg/isVROoC4ZUaghIRttqBL+arCy46qR0AhWw4/k8wR3WiaRTVspJffeHwlefy+qb0\n\t16G0ibnj7JOVwnh/ig6k+9EPMklWGqY1nL6vpPp3s3nTqEinWESZtdFfb6sdIJMBrh\n\ttqenAM4/jA71Q==","Date":"Wed, 20 Jul 2022 11:49:07 +0200","To":"paul.elder@ideasonboard.com","Message-ID":"<20220720094907.xv4x6k3v7fdxf5ge@uno.localdomain>","References":"<20220719073012.810184-1-paul.elder@ideasonboard.com>\n\t<20220720083657.myrlba2n2rlxayep@uno.localdomain>\n\t\n\t<20220720090704.af2ajf2izr5xvepz@uno.localdomain>\n\t<20220720091951.GD3984498@pyrite.rasen.tech>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220720091951.GD3984498@pyrite.rasen.tech>","Subject":"Re: [libcamera-devel] [PATCH v3] pipeline: rkisp1: Support devices\n\twithout self path","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Jacopo Mondi via libcamera-devel ","Reply-To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]