[{"id":25127,"web_url":"https://patchwork.libcamera.org/comment/25127/","msgid":"","date":"2022-09-26T11:15:40","subject":"Re: [libcamera-devel] [PATCH 1/1] pipeline: rkisp1: Implement Bayer\n\tformats support","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Florian,\n\nThank you for the patch.\n\nOn Fri, Sep 23, 2022 at 02:55:46PM +0200, Florian Sylvestre via libcamera-devel wrote:\n> Implement raw mode for RkISP1:\n> - The ISP resizer doesn't support raw formats, so when in raw mode we force the\n> output resolution to be the same as the sensor one.\n> - In raw mode, the ISP is bypassed, so we never get statistics buffers.\n> This means that the IPA is never instructed to set the controls nor the\n> metadata.\n> Add a completeRaw() function to the IPA for the purpose of instructing the IPA\n> to set controls and metadata when a frame is ready, as opposed to when the\n> statistics are ready.\n> We also need to skip queueing the stats buffer when in raw mode to prevent the\n> statistics bufferReady slot to be triggered at stream off.\n> \n> Signed-off-by: Florian Sylvestre \n> Signed-off-by: Paul Elder \n> ---\n> include/libcamera/ipa/rkisp1.mojom | 1 +\n> src/ipa/rkisp1/rkisp1.cpp | 10 +++\n> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 63 ++++++++++++++++++-\n> src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 51 ++++++++++++++-\n> 4 files changed, 120 insertions(+), 5 deletions(-)\n> \n> diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom\n> index eaf3955e..931ef357 100644\n> --- a/include/libcamera/ipa/rkisp1.mojom\n> +++ b/include/libcamera/ipa/rkisp1.mojom\n> @@ -27,6 +27,7 @@ interface IPARkISP1Interface {\n> \t[async] fillParamsBuffer(uint32 frame, uint32 bufferId);\n> \t[async] processStatsBuffer(uint32 frame, uint32 bufferId,\n> \t\t\t\t libcamera.ControlList sensorControls);\n> +\t[async] completeRaw(uint32 frame);\n> };\n> \n> interface IPARkISP1EventInterface {\n> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> index 61a091e6..4c784e8f 100644\n> --- a/src/ipa/rkisp1/rkisp1.cpp\n> +++ b/src/ipa/rkisp1/rkisp1.cpp\n> @@ -63,6 +63,7 @@ public:\n> \tvoid fillParamsBuffer(const uint32_t frame, const uint32_t bufferId) override;\n> \tvoid processStatsBuffer(const uint32_t frame, const uint32_t bufferId,\n> \t\t\t\tconst ControlList &sensorControls) override;\n> +\tvoid completeRaw(const uint32_t frame) override;\n> \n> protected:\n> \tstd::string logPrefix() const override;\n> @@ -352,6 +353,15 @@ void IPARkISP1::processStatsBuffer(const uint32_t frame, const uint32_t bufferId\n> \tprepareMetadata(frame, aeState);\n> }\n> \n> +void IPARkISP1::completeRaw(const uint32_t frame)\n> +{\n> +\tunsigned int aeState = 0;\n> +\n> +\tsetControls(frame);\n> +\n> +\tprepareMetadata(frame, aeState);\n> +}\n> +\n\nIs this enough to ensure the IPA module will operate properly when\ncapturing raw frames ? The algorithms will never be run due to a lack of\nstatistics, so I don't think manual exposure and gain will be taken into\naccount correctly.\n\nThis should probably be fixed in a separate patch, possibly before this\none to prepare for raw support on the IPA side first and then enabling\nit on the pipeline handler side.\n\n> void IPARkISP1::setControls(unsigned int frame)\n> {\n> \tRkISP1FrameContext &frameContext = context_.frameContexts.get(frame);\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index 25fbf9f1..bade024d 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -182,6 +182,7 @@ private:\n> \tstd::unique_ptr csi_;\n> \n> \tbool hasSelfPath_;\n> +\tbool isRaw_;\n> \n> \tRkISP1MainPath mainPath_;\n> \tRkISP1SelfPath selfPath_;\n> @@ -363,7 +364,9 @@ void RkISP1CameraData::paramFilled(unsigned int frame)\n> \t\treturn;\n> \n> \tpipe->param_->queueBuffer(info->paramBuffer);\n> -\tpipe->stat_->queueBuffer(info->statBuffer);\n> +\n> +\tif (!pipe->isRaw_)\n> +\t\tpipe->stat_->queueBuffer(info->statBuffer);\n\nShouldn't you also skip queuing ISP parameters ?\n\n> \n> \tif (info->mainPathBuffer)\n> \t\tmainPath_->queueBuffer(info->mainPathBuffer);\n> @@ -413,6 +416,21 @@ bool RkISP1CameraConfiguration::fitsAllPaths(const StreamConfiguration &cfg)\n> \treturn true;\n> }\n> \n> +std::map rawFormats = {\n> +\t{ formats::SRGGB8, MEDIA_BUS_FMT_SRGGB8_1X8 },\n> +\t{ formats::SGRBG8, MEDIA_BUS_FMT_SGRBG8_1X8 },\n> +\t{ formats::SGBRG8, MEDIA_BUS_FMT_SGBRG8_1X8 },\n> +\t{ formats::SBGGR8, MEDIA_BUS_FMT_SBGGR8_1X8 },\n\nCould you please sort the bayer pattern alphabetically ? Same below\nwhere applicable.\n\n> +\t{ formats::SRGGB10, MEDIA_BUS_FMT_SRGGB10_1X10 },\n> +\t{ formats::SGRBG10, MEDIA_BUS_FMT_SGRBG10_1X10 },\n> +\t{ formats::SGBRG10, MEDIA_BUS_FMT_SGBRG10_1X10 },\n> +\t{ formats::SBGGR10, MEDIA_BUS_FMT_SBGGR10_1X10 },\n> +\t{ formats::SRGGB12, MEDIA_BUS_FMT_SRGGB12_1X12 },\n> +\t{ formats::SGRBG12, MEDIA_BUS_FMT_SGRBG12_1X12 },\n> +\t{ formats::SGBRG12, MEDIA_BUS_FMT_SGBRG12_1X12 },\n> +\t{ formats::SBGGR12, MEDIA_BUS_FMT_SBGGR12_1X12 },\n> +};\n> +\n> CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> {\n> \tconst CameraSensor *sensor = data_->sensor_.get();\n> @@ -504,8 +522,23 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> \n> \t/* Select the sensor format. */\n> \tSize maxSize;\n> -\tfor (const StreamConfiguration &cfg : config_)\n> +\tPixelFormat rawFormat;\n> +\tbool hasRawFormat = false;\n> +\tfor (StreamConfiguration &cfg : config_) {\n> +\t\tif (PixelFormatInfo::info(cfg.pixelFormat).colourEncoding ==\n> +\t\t PixelFormatInfo::ColourEncodingRAW) {\n> +\t\t\thasRawFormat = true;\n> +\t\t\trawFormat = cfg.pixelFormat;\n> +\n> +\t\t\t/* Raw format cannot be resized by ISP. */\n> +\t\t\tif (cfg.size != sensor->resolution()) {\n> +\t\t\t\tcfg.size = sensor->resolution();\n> +\t\t\t\tstatus = Adjusted;\n> +\t\t\t}\n> +\t\t}\n> +\n> \t\tmaxSize = std::max(maxSize, cfg.size);\n> +\t}\n\nIf an application requests two streams, I don't think this logic will\nwork correctly. We can't capture two raw streams, or one raw and one YUV\nstream.\n\n> \n> \tsensorFormat_ = sensor->getFormat({ MEDIA_BUS_FMT_SBGGR12_1X12,\n> \t\t\t\t\t MEDIA_BUS_FMT_SGBRG12_1X12,\n> @@ -520,6 +553,13 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> \t\t\t\t\t MEDIA_BUS_FMT_SGRBG8_1X8,\n> \t\t\t\t\t MEDIA_BUS_FMT_SRGGB8_1X8 },\n> \t\t\t\t\t maxSize);\n> +\n> +\tif (hasRawFormat) {\n> +\t\tauto mbus = rawFormats.find(rawFormat);\n> +\t\tif (mbus != rawFormats.end())\n> +\t\t\tsensorFormat_ = sensor->getFormat({ mbus->second }, maxSize);\n> +\t}\n> +\n> \tif (sensorFormat_.size.isNull())\n> \t\tsensorFormat_.size = sensor->resolution();\n> \n> @@ -659,8 +699,14 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)\n> \t\t<< \"ISP input pad configured with \" << format\n> \t\t<< \" crop \" << rect;\n> \n> +\tconst PixelFormat &streamFormat = config->at(0).pixelFormat;\n> +\tconst PixelFormatInfo &info = PixelFormatInfo::info(streamFormat);\n> +\tisRaw_ = info.colourEncoding == PixelFormatInfo::ColourEncodingRAW;\n> +\n> \t/* YUYV8_2X8 is required on the ISP source path pad for YUV output. */\n> -\tformat.mbus_code = MEDIA_BUS_FMT_YUYV8_2X8;\n> +\tif (!isRaw_)\n> +\t\tformat.mbus_code = MEDIA_BUS_FMT_YUYV8_2X8;\n> +\n> \tLOG(RkISP1, Debug)\n> \t\t<< \"Configuring ISP output pad with \" << format\n> \t\t<< \" crop \" << rect;\n> @@ -1152,6 +1198,17 @@ void PipelineHandlerRkISP1::bufferReady(FrameBuffer *buffer)\n> \trequest->metadata().set(controls::SensorTimestamp,\n> \t\t\t\tbuffer->metadata().timestamp);\n> \n> +\tif (isRaw_) {\n> +\t\tASSERT(activeCamera_);\n> +\t\tRkISP1CameraData *data = cameraData(activeCamera_);\n> +\n> +\t\tRkISP1FrameInfo *info = data->frameInfo_.find(buffer);\n> +\t\tif (!info)\n> +\t\t\treturn;\n> +\n> +\t\tdata->ipa_->completeRaw(info->frame);\n> +\t}\n> +\n> \tcompleteBuffer(request, buffer);\n> \ttryCompleteRequest(request);\n\nAs PipelineHandlerRkISP1::tryCompleteRequest() calls find() and all of\nits callers now do as well, I'd modify the function to take the info\npointer instead of the request pointer.\n\n> }\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n> index 2d38f0fb..0a33d9ed 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n> @@ -132,6 +132,42 @@ int RkISP1Path::configure(const StreamConfiguration &config,\n> \tcase formats::NV21:\n> \t\tispFormat.mbus_code = MEDIA_BUS_FMT_YUYV8_1_5X8;\n> \t\tbreak;\n> +\tcase formats::SRGGB8:\n> +\t\tispFormat.mbus_code = MEDIA_BUS_FMT_SRGGB8_1X8;\n> +\t\tbreak;\n> +\tcase formats::SGRBG8:\n> +\t\tispFormat.mbus_code = MEDIA_BUS_FMT_SGRBG8_1X8;\n> +\t\tbreak;\n> +\tcase formats::SGBRG8:\n> +\t\tispFormat.mbus_code = MEDIA_BUS_FMT_SGBRG8_1X8;\n> +\t\tbreak;\n> +\tcase formats::SBGGR8:\n> +\t\tispFormat.mbus_code = MEDIA_BUS_FMT_SBGGR8_1X8;\n> +\t\tbreak;\n> +\tcase formats::SRGGB10:\n> +\t\tispFormat.mbus_code = MEDIA_BUS_FMT_SRGGB10_1X10;\n> +\t\tbreak;\n> +\tcase formats::SGRBG10:\n> +\t\tispFormat.mbus_code = MEDIA_BUS_FMT_SGRBG10_1X10;\n> +\t\tbreak;\n> +\tcase formats::SGBRG10:\n> +\t\tispFormat.mbus_code = MEDIA_BUS_FMT_SGBRG10_1X10;\n> +\t\tbreak;\n> +\tcase formats::SBGGR10:\n> +\t\tispFormat.mbus_code = MEDIA_BUS_FMT_SBGGR10_1X10;\n> +\t\tbreak;\n> +\tcase formats::SRGGB12:\n> +\t\tispFormat.mbus_code = MEDIA_BUS_FMT_SRGGB12_1X12;\n> +\t\tbreak;\n> +\tcase formats::SGRBG12:\n> +\t\tispFormat.mbus_code = MEDIA_BUS_FMT_SGRBG12_1X12;\n> +\t\tbreak;\n> +\tcase formats::SGBRG12:\n> +\t\tispFormat.mbus_code = MEDIA_BUS_FMT_SGBRG12_1X12;\n> +\t\tbreak;\n> +\tcase formats::SBGGR12:\n> +\t\tispFormat.mbus_code = MEDIA_BUS_FMT_SBGGR12_1X12;\n> +\t\tbreak;\n> \tdefault:\n> \t\tispFormat.mbus_code = MEDIA_BUS_FMT_YUYV8_2X8;\n> \t\tbreak;\n\nThis is growing big, how about extending the rawFormats map to cover all\nformats, and using it here ?\n\n> @@ -207,14 +243,25 @@ 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> -constexpr std::array RKISP1_RSZ_MP_FORMATS{\n> +constexpr std::array RKISP1_RSZ_MP_FORMATS{\n> \tformats::YUYV,\n> \tformats::NV16,\n> \tformats::NV61,\n> \tformats::NV21,\n> \tformats::NV12,\n> \tformats::R8,\n> -\t/* \\todo Add support for RAW formats. */\n> +\tformats::SRGGB8,\n> +\tformats::SGRBG8,\n> +\tformats::SGBRG8,\n> +\tformats::SBGGR8,\n> +\tformats::SRGGB10,\n> +\tformats::SGRBG10,\n> +\tformats::SGBRG10,\n> +\tformats::SBGGR10,\n> +\tformats::SRGGB12,\n> +\tformats::SGRBG12,\n> +\tformats::SGBRG12,\n> +\tformats::SBGGR12,\n> };\n> \n> constexpr Size RKISP1_RSZ_SP_SRC_MIN{ 32, 16 };","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 01D8AC0DA4\n\tfor ;\n\tMon, 26 Sep 2022 11:15:59 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 472EC62254;\n\tMon, 26 Sep 2022 13:15:59 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C6BDF6218B\n\tfor ;\n\tMon, 26 Sep 2022 13:15:57 +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 279CA415;\n\tMon, 26 Sep 2022 13:15:57 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1664190959;\n\tbh=TD0dxI1ySuDU3BWfzfWseSxr+MB9PUZ1R+nhi3B5wnM=;\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=Sma+BTTH3gjvy+pFjVrOcyW4HnqDCWZvAVxCmNup1ihH07svf2JKL0KsL2s1EYrQJ\n\tIHZU1DSUAXcrE+DotWO3asrYUqtdWR2/eC8ezIa1VI44JSsPyWGgjlPMA7N8B2eos+\n\tyuEdnqWbHbETR/i3QYRWQhoGgGT+wxp0tukJpCXc+SkPW2+kyEYmimmdrDuZroDUaO\n\tegEiGmOhVce2FpjwfYjUvn9RkGzwwpQJhD79AaYeDud9RJR+qNyBZ4RJzbxc2AMxrJ\n\tEq/vQsUp7iDzN/GgK2/iGfbN/acIyvjCx4j3bJYuykvdH1CxZRkmpAEbB+ZpyfleiW\n\tpN3OcPHdquMKA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1664190957;\n\tbh=TD0dxI1ySuDU3BWfzfWseSxr+MB9PUZ1R+nhi3B5wnM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=F+T48IhMDpIFxh7VVIDFUnfCWaBctBRFU9sDaMONyFP8CJTba2qMay787pKumWuTw\n\t03bkJiHaVS+9DoyEnVgclgDiKUcJwhNGTcRNhboU1caLZHyei5VofQtGq0iY2VylMc\n\txAiDSSUDj80cx5Lpqx7eWuE4lMRAthEosBP4bcrI="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"F+T48IhM\"; dkim-atps=neutral","Date":"Mon, 26 Sep 2022 14:15:40 +0300","To":"Florian Sylvestre ","Message-ID":"","References":"<20220923125546.903671-1-fsylvestre@baylibre.com>\n\t<20220923125546.903671-2-fsylvestre@baylibre.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220923125546.903671-2-fsylvestre@baylibre.com>","Subject":"Re: [libcamera-devel] [PATCH 1/1] pipeline: rkisp1: Implement Bayer\n\tformats support","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\" "}}]