[{"id":24047,"web_url":"https://patchwork.libcamera.org/comment/24047/","msgid":"","date":"2022-07-21T23:35:45","subject":"Re: [libcamera-devel] [RFC PATCH v2] pipeline: rkisp1: Query the\n\tdriver for formats","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 Thu, Jul 21, 2022 at 01:40:04AM +0900, Paul Elder via libcamera-devel wrote:\n> Query the driver for the output formats that it supports, instead of\n> hardcoding it. While at it, cache the framesizes as well.\n\nThe cached sizes are not used, and I don't foresee a use for them in the\nfuture. I would drop them, but I think you should use the enumerated\nsizes in RkISP1Path::populateFormats() to replace the minResolution_ and\nmaxResolution_ values.\n\n> This allows future-proofing for formats that are supported by some but\n> not all versions of the driver.\n> \n> As the rkisp1 driver currently does not support VIDIOC_ENUM_FRAMESIZES,\n> fallback to the hardcoded list of supported formats and framesizes. This\n> feature will be added to the driver in parallel, though we cannot\n> guarantee that users will have a new enough kernel for it.\n> \n> Signed-off-by: Paul Elder \n> \n> ---\n> Changes in v2:\n> - enumerate and cache framesizes as well\n> - massage generateConfiguration accordingly\n> - this lets us skip modifying V4L2VideoDevice::formats() to support\n> lack of ENUM_FRAMESIZES\n> - also requires us to keep the list of hardcoded formats for backward\n> compatibility\n> ---\n> src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 50 +++++++++++++++++--\n> src/libcamera/pipeline/rkisp1/rkisp1_path.h | 3 ++\n> 2 files changed, 48 insertions(+), 5 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n> index 6f175758..cf5feebe 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n> @@ -41,6 +41,8 @@ bool RkISP1Path::init(MediaDevice *media)\n> \tif (video_->open() < 0)\n> \t\treturn false;\n> \n> +\tpopulateFormats(video_);\n\nNo need to pass it to the function, you can access video_ internally.\n\n> +\n> \tlink_ = media->link(\"rkisp1_isp\", 2, resizer, 0);\n> \tif (!link_)\n> \t\treturn false;\n> @@ -48,15 +50,44 @@ bool RkISP1Path::init(MediaDevice *media)\n> \treturn true;\n> }\n> \n> +void RkISP1Path::populateFormats(std::unique_ptr &video)\n> +{\n> +\tV4L2VideoDevice::Formats v4l2Formats = video->formats();\n> +\tif (v4l2Formats.empty()) {\n> +\t\tLOG(RkISP1, Warning)\n> +\t\t\t<< \"Failed to enumerate framesizes. Loading default framesizes\";\n> +\n> +\t\tfor (const PixelFormat &format : formats_)\n> +\t\t\tstreamFormats_[format] = { { minResolution_, maxResolution_ } };\n> +\t\treturn;\n> +\t}\n> +\n> +\tstd::vector formats;\n> +\tfor (auto pair : v4l2Formats) {\n> +\t\tconst PixelFormat pixelFormat = pair.first.toPixelFormat();\n> +\t\tconst PixelFormatInfo &info = PixelFormatInfo::info(pixelFormat);\n> +\n> +\t\t/* \\todo Add support for RAW formats. */\n> +\t\tif (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW)\n> +\t\t\tcontinue;\n> +\n> +\t\tstreamFormats_[pixelFormat] = pair.second;\n> +\t}\n> +}\n> +\n> StreamConfiguration RkISP1Path::generateConfiguration(const Size &resolution)\n> {\n> \tSize maxResolution = maxResolution_.boundedToAspectRatio(resolution)\n> \t\t\t\t\t .boundedTo(resolution);\n> \tSize minResolution = minResolution_.expandedToAspectRatio(resolution);\n> \n> +\t/*\n> +\t * Can we use the global min/max resolutions here or does it need to be\n> +\t * resized to the same aspect ratio as the requested resolution?\n> +\t */\n\nIs this a comment that explains the code below, or a question to the\nreader to figure out if the code should be fixed ?\n\n> \tstd::map> streamFormats;\n> -\tfor (const PixelFormat &format : formats_)\n> -\t\tstreamFormats[format] = { { minResolution, maxResolution } };\n> +\tfor (auto pair : streamFormats_)\n\n\tfor (const auto &[format, sizes] : streamFormats_)\n\n> +\t\tstreamFormats[pair.first] = { { minResolution, maxResolution } };\n> \n> \tStreamFormats formats(streamFormats);\n> \tStreamConfiguration cfg(formats);\n> @@ -72,8 +103,14 @@ CameraConfiguration::Status RkISP1Path::validate(StreamConfiguration *cfg)\n> \tconst StreamConfiguration reqCfg = *cfg;\n> \tCameraConfiguration::Status status = CameraConfiguration::Valid;\n> \n> -\tif (std::find(formats_.begin(), formats_.end(), cfg->pixelFormat) ==\n> -\t formats_.end())\n> +\t/*\n> +\t * Default to NV12 if the requested format is not supported. All\n> +\t * versions of the ISP are guaranteed to support NV12 on both the main\n> +\t * and self paths.\n> +\t */\n> +\tif (std::find_if(streamFormats_.begin(), streamFormats_.end(),\n> +\t\t\t [cfg](auto pair) { return pair.first == cfg->pixelFormat; }) ==\n> +\t streamFormats_.end())\n\nIt's a map :-)\n\n\tif (!streamFormats.count(cfg->pixelFormat))\n\n> \t\tcfg->pixelFormat = formats::NV12;\n> \n> \tcfg->size.boundTo(maxResolution_);\n> @@ -207,6 +244,8 @@ void RkISP1Path::stop()\n> namespace {\n> constexpr Size RKISP1_RSZ_MP_SRC_MIN{ 32, 16 };\n> constexpr Size RKISP1_RSZ_MP_SRC_MAX{ 4416, 3312 };\n> +\n> +/* \\todo Remove this eventually. */\n\n/*\n * \\todo Remove the hardcoded resolutions and formats once all users will have \n * migrated to a recent enough kernel.\n */\n\nand you can move this just before the namespace, and drop the duplicated\n\\todo below.\n\n> constexpr std::array RKISP1_RSZ_MP_FORMATS{\n> \tformats::YUYV,\n> \tformats::NV16,\n> @@ -214,11 +253,12 @@ constexpr std::array RKISP1_RSZ_MP_FORMATS{\n> \tformats::NV21,\n> \tformats::NV12,\n> \tformats::R8,\n> -\t/* \\todo Add support for RAW formats. */\n> };\n> \n> constexpr Size RKISP1_RSZ_SP_SRC_MIN{ 32, 16 };\n> constexpr Size RKISP1_RSZ_SP_SRC_MAX{ 1920, 1920 };\n> +\n> +/* \\todo Remove this eventually. */\n> constexpr std::array RKISP1_RSZ_SP_FORMATS{\n> \tformats::YUYV,\n> \tformats::NV16,\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.h b/src/libcamera/pipeline/rkisp1/rkisp1_path.h\n> index f3f1ae39..42db158f 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.h\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.h\n> @@ -57,11 +57,14 @@ public:\n> \tSignal &bufferReady() { return video_->bufferReady; }\n> \n> private:\n> +\tvoid populateFormats(std::unique_ptr &video);\n> +\n> \tstatic constexpr unsigned int RKISP1_BUFFER_COUNT = 4;\n> \n> \tconst char *name_;\n> \tbool running_;\n> \n> +\tstd::map> streamFormats_;\n> \tconst Span formats_;\n> \tconst Size minResolution_;\n> \tconst Size maxResolution_;","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 2A43BBD1F1\n\tfor ;\n\tThu, 21 Jul 2022 23:35:50 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 67EE963311;\n\tFri, 22 Jul 2022 01:35:49 +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 83CEB60487\n\tfor ;\n\tFri, 22 Jul 2022 01:35: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 E424F6D5;\n\tFri, 22 Jul 2022 01:35:46 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1658446549;\n\tbh=LJimmYYxVGNXuLZWWmci59SeLU1Sb+d+YfgRmwaE02w=;\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=Wq3MAfW3LP3+buUF+tO9y7Vep5dRLu9CRUtiIMmsOy/fPPTa9BqDc6mMQ8pYXAuZq\n\thDYZDscjIWrTnmXcsAsGGee94RkKhUR/OBcRUDDLmDlomsbOK3UMAyXOdp3zfDts4l\n\toGcj6MXRA5DVjr+KCddWmmnh3udRmSS42QdfTWPqgK/TklEfXBNK7LncnSgqw2HbgM\n\t4+o3tsQ3ZGhwEK1BBOEopXVGTPXcYIXy0Yf1BQAiXPZhfOeJdof0Jk+TMNjMYtNHGA\n\tcKHQ0oebqN/L6+Dx/Yu0OQK1wHgmJdtoySCsXMjmbEZ1M7uMBZ8BtztEMd8dgyBbxJ\n\tLIm9Qwi2EcpgQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1658446547;\n\tbh=LJimmYYxVGNXuLZWWmci59SeLU1Sb+d+YfgRmwaE02w=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=T74rSIUPp5ZViOv4Vmz6LWzQY9xwES26CT8ay0Jieu0atQ4xOiHweFDjVgqAgGYOo\n\trrJpJNPeLk5RJX6FJUDIwjzd9+j2eHzCeeUY7ocgZaPaJXeYFasruUcWJDUBj6V1di\n\t2EmanT5YMeeggAum4WO65oNfcSbBcfshqVIT799U="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"T74rSIUP\"; dkim-atps=neutral","Date":"Fri, 22 Jul 2022 02:35:45 +0300","To":"Paul Elder ","Message-ID":"","References":"<20220720164004.3813194-1-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220720164004.3813194-1-paul.elder@ideasonboard.com>","Subject":"Re: [libcamera-devel] [RFC PATCH v2] pipeline: rkisp1: Query the\n\tdriver for formats","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\" "}}]