[{"id":23807,"web_url":"https://patchwork.libcamera.org/comment/23807/","msgid":"","date":"2022-07-08T21:56:53","subject":"Re: [libcamera-devel] [PATCH v2] 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, Jun 28, 2022 at 05:33:43PM +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 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 | 40 ++++++++++++++----------\n> 1 file changed, 24 insertions(+), 16 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index a233c961..2c457f0e 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> @@ -344,7 +346,7 @@ void RkISP1CameraData::paramFilled(unsigned int frame)\n> \tif (info->mainPathBuffer)\n> \t\tmainPath_->queueBuffer(info->mainPathBuffer);\n> \n> -\tif (info->selfPathBuffer)\n> +\tif (info->selfPathBuffer && selfPath_)\n\nI don't think selfPathBuffer can be non-null if selfPath_ is null, but\nit doesn't really hurt to check. I probably would have put the selfPath_\ncheck first as that's what you do everywhere else.\n\n> \t\tselfPath_->queueBuffer(info->selfPathBuffer);\n> }\n> \n> @@ -383,7 +385,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> @@ -393,6 +395,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> {\n> \tconst CameraSensor *sensor = data_->sensor_.get();\n> \tStatus status = Valid;\n> +\tunsigned int pathCount = data_->selfPath_ ? 2 : 1;\n\nMove it one line up ?\n\n> \n> \tif (config_.empty())\n> \t\treturn Invalid;\n> @@ -403,8 +406,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> @@ -421,7 +424,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 = (pathCount == 2);\n\nNo need for parentheses, and I would actually write\n\n\tbool selfPathAvailable = data->selfPath_;\n\nas it's more logical (and it's what you do below too). You could then\npossibly move the pathCount variable just before the check that uses it,\nup to you.\n\n> \tfor (unsigned int index : order) {\n> \t\tStreamConfiguration &cfg = config_[index];\n> \n> @@ -500,7 +503,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> @@ -527,7 +530,7 @@ CameraConfiguration *PipelineHandlerRkISP1::generateConfiguration(Camera *camera\n> \t}\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\nThis won't be enough. If you have two StillCapture roles, the first one\nwill use the main path and the second one the self path. If you have two\nViewFinder or VideoRecording roles, both will try to use the main path.\nIf you have more than two roles it's even worse. The logic is already\nbroken, you may want to fix it in a separate patch first, and maybe this\nchange will then be enough.\n\n> \n> @@ -636,10 +639,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\nShould never happen, but looks safer.\n\n> \t\t}\n> \n> \t\tif (ret)\n> @@ -687,7 +692,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> @@ -816,7 +821,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> @@ -843,7 +848,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\nThere's one additional usage of selfPath_ in\nPipelineHandlerRkISP1::initLinks():\n\n\t\telse if (cfg.stream() == &data->selfPathStream_)\n\t\t\tret = data->selfPath_->setEnabled(true);\n\nI don't think it can get triggered, as the configuration has been\nvalidated and the stream check thus can't pass, but as there's another\ncase above where a hasSelfPath_ check is added while not strictly\nneeded, I'd add one here too just to be safe.\n\n> @@ -946,7 +952,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> @@ -1002,9 +1009,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> @@ -1019,6 +1024,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> @@ -1049,11 +1056,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 ECF4EBE173\n\tfor ;\n\tFri, 8 Jul 2022 21:57:22 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 34FBB63312;\n\tFri, 8 Jul 2022 23:57:22 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 069446330D\n\tfor ;\n\tFri, 8 Jul 2022 23:57:21 +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 5990E56D;\n\tFri, 8 Jul 2022 23:57:20 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1657317442;\n\tbh=GdgxUm6oI33v6Zcxq8ZAtzIhr2qd5xbEBzcbOvHLyjI=;\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=Vz+uCBTW7nTn7cE+w8R8rv7w59U7sdNNUmGMp+B+TKF2hXYKgDfx73UKfd+sH1C5X\n\ti2lreYsFNrkoa2i253ICaBBJ23lEK3c/FIQis8Sx1wd2K6x5ZtwioN5oG2Ioc1sH6r\n\tqFRnG+PEvfVuQmWvzEIc6Di3jisGoyDy/w3Bn5sM8doRIybTnNrjj4puhvzq0HXmvm\n\tOw3cgVkM4Sc3lG2GJFQUMN4MHqLbWLrChnu5nJ09pe2oSUnEdq9xdIXTM97+tmENXm\n\t0ba74nVp7QFYpQdYPZcKws+R/MS5Cw0YWtwV6AzJlZTmlN+DEz6z+SwadMwchruRGf\n\tsEpr+M+KUoG3g==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1657317440;\n\tbh=GdgxUm6oI33v6Zcxq8ZAtzIhr2qd5xbEBzcbOvHLyjI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=SsochvIKdO8yPKGEr1Qe6cLdt+4fbQcOncNNtqq3b50f8l+ELrfuPIU6Z4XJd1V26\n\tmRlZwuCBkpJcL/GM4xDvRIR/5RjJjCq+ej2AuOBapjz3N2gg6Oni9RmB0l+wHnSYJQ\n\ti0l1sJUK6M5HBbqGkGxqdBbzOgQ/flVJDKHhsOhI="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"SsochvIK\"; dkim-atps=neutral","Date":"Sat, 9 Jul 2022 00:56:53 +0300","To":"Paul Elder ","Message-ID":"","References":"<20220628083343.1992147-1-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220628083343.1992147-1-paul.elder@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2] 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\" "}}]