[{"id":36076,"web_url":"https://patchwork.libcamera.org/comment/36076/","msgid":"<6aff1ba1-1813-4759-8b89-8f21a7336aa4@ideasonboard.com>","date":"2025-10-02T14:15:29","subject":"Re: [PATCH v1 1/4] ipa: rpi: pisp: Add decompand support using PiSP\n\thardware block","submitter":{"id":216,"url":"https://patchwork.libcamera.org/api/people/216/","name":"Barnabás Pőcze","email":"barnabas.pocze@ideasonboard.com"},"content":"Hi\n\n\nJust a couple quick code style remarks.\n\n2025. 10. 02. 15:26 keltezéssel, Naushir Patuck írta:\n> From: Sena Asotani \n> \n> This patch integrates a new decompand algorithm that utilizes the PiSP\n> FE hardware block available on Raspberry Pi 5. The implementation\n> enables conversion of companded sensor data into linear format prior to\n> ISP processing.\n> \n> Changes include:\n> - Implementation of decompand logic for controller and pipe interfaces\n> - Enabling decompand block by \"rpi.decompand\" in tuning.json\n> \n> Signed-off-by: Sena Asotani \n> Signed-off-by: Naushir Patuck \n> ---\n> src/ipa/rpi/controller/decompand_status.h | 8 ++++\n> src/ipa/rpi/controller/meson.build | 1 +\n> src/ipa/rpi/controller/rpi/decompand.cpp | 58 +++++++++++++++++++++++\n> src/ipa/rpi/controller/rpi/decompand.h | 31 ++++++++++++\n> src/ipa/rpi/pisp/pisp.cpp | 40 ++++++++++++++++\n> 5 files changed, 138 insertions(+)\n> create mode 100644 src/ipa/rpi/controller/decompand_status.h\n> create mode 100644 src/ipa/rpi/controller/rpi/decompand.cpp\n> create mode 100644 src/ipa/rpi/controller/rpi/decompand.h\n> \n> diff --git a/src/ipa/rpi/controller/decompand_status.h b/src/ipa/rpi/controller/decompand_status.h\n> new file mode 100644\n> index 000000000000..2d9888dca4f3\n> --- /dev/null\n> +++ b/src/ipa/rpi/controller/decompand_status.h\n> @@ -0,0 +1,8 @@\n> +#pragma once\n> +\n> +#include \"libipa/pwl.h\"\n> +\n> +struct DecompandStatus {\n> +\tuint32_t bitdepth;\n> +\tlibcamera::ipa::Pwl decompandCurve;\n> +};\n> diff --git a/src/ipa/rpi/controller/meson.build b/src/ipa/rpi/controller/meson.build\n> index 74b74888bbff..c13c48539d10 100644\n> --- a/src/ipa/rpi/controller/meson.build\n> +++ b/src/ipa/rpi/controller/meson.build\n> @@ -14,6 +14,7 @@ rpi_ipa_controller_sources = files([\n> 'rpi/cac.cpp',\n> 'rpi/ccm.cpp',\n> 'rpi/contrast.cpp',\n> + 'rpi/decompand.cpp',\n> 'rpi/denoise.cpp',\n> 'rpi/dpc.cpp',\n> 'rpi/geq.cpp',\n> diff --git a/src/ipa/rpi/controller/rpi/decompand.cpp b/src/ipa/rpi/controller/rpi/decompand.cpp\n> new file mode 100644\n> index 000000000000..911b04bc0da0\n> --- /dev/null\n> +++ b/src/ipa/rpi/controller/rpi/decompand.cpp\n> @@ -0,0 +1,58 @@\n> +#include \"decompand.h\"\n> +\n> +#include \n> +\n> +#include \"../decompand_status.h\"\n> +#include \"../histogram.h\"\n> +\n> +using namespace RPiController;\n> +using namespace libcamera;\n> +\n> +LOG_DEFINE_CATEGORY(RPiDecompand)\n> +\n> +#define NAME \"rpi.decompand\"\n> +\n> +Decompand::Decompand(Controller *controller)\n> +\t: Algorithm(controller)\n> +{\n> +}\n> +\n> +char const *Decompand::name() const\n> +{\n> +\treturn NAME;\n> +}\n> +\n> +int Decompand::read(const libcamera::YamlObject ¶ms)\n> +{\n> +\tconfig_.bitdepth = params[\"bitdepth\"].get(0);\n> +\tconfig_.decompandCurve = params[\"decompand_curve\"].get(ipa::Pwl{});\n> +\treturn config_.decompandCurve.empty() ? -EINVAL : 0;\n> +}\n> +\n> +void Decompand::initialise()\n> +{\n> +}\n> +\n> +void Decompand::switchMode([[maybe_unused]] CameraMode const &cameraMode,\n\nPlease remove the `maybe_unused` since it is always used.\n\n\n> +\t\t\t [[maybe_unused]] Metadata *metadata)\n> +{\n> +\tmode_ = cameraMode;\n> +}\n> +\n> +void Decompand::prepare(Metadata *imageMetadata)\n> +{\n> +\tDecompandStatus decompandStatus;\n> +\n> +\tif (config_.bitdepth == 0 || mode_.bitdepth == config_.bitdepth) {\n> +\t\tdecompandStatus.decompandCurve = config_.decompandCurve;\n> +\t\timageMetadata->set(\"decompand.status\", decompandStatus);\n> +\t}\n> +}\n> +\n> +/* Register algorithm with the system. */\n> +static Algorithm *create(Controller *controller)\n> +{\n> +\treturn (Algorithm *)new Decompand(controller);\n\nPlease remove the explicit cast here.\n\n\n> +}\n> +\n> +static RegisterAlgorithm reg(NAME, &create);\n> diff --git a/src/ipa/rpi/controller/rpi/decompand.h b/src/ipa/rpi/controller/rpi/decompand.h\n> new file mode 100644\n> index 000000000000..5ef35946efa5\n> --- /dev/null\n> +++ b/src/ipa/rpi/controller/rpi/decompand.h\n> @@ -0,0 +1,31 @@\n> +#pragma once\n> +\n> +#include \n> +\n> +#include \"../decompand_status.h\"\n> +\n> +#include \"algorithm.h\"\n> +\n> +namespace RPiController {\n> +\n> +struct DecompandConfig {\n> +\tuint32_t bitdepth;\n> +\tlibcamera::ipa::Pwl decompandCurve;\n> +};\n> +\n> +class Decompand : public Algorithm\n> +{\n> +public:\n> +\tDecompand(Controller *controller = NULL);\n\nI know it goes against the other algorithms, but I think it would\nbe nice to converge towards `nullptr`.\n\n\n> +\tchar const *name() const override;\n> +\tint read(const libcamera::YamlObject ¶ms) override;\n> +\tvoid initialise() override;\n> +\tvoid switchMode(CameraMode const &cameraMode, Metadata *metadata) override;\n> +\tvoid prepare(Metadata *imageMetadata) override;\n> +\n> +private:\n> +\tCameraMode mode_;\n> +\tDecompandConfig config_;\n> +};\n> +\n> +} /* namespace RPiController */\n> diff --git a/src/ipa/rpi/pisp/pisp.cpp b/src/ipa/rpi/pisp/pisp.cpp\n> index 829b91258522..e75c87df1924 100644\n> --- a/src/ipa/rpi/pisp/pisp.cpp\n> +++ b/src/ipa/rpi/pisp/pisp.cpp\n> @@ -32,6 +32,7 @@\n> #include \"controller/cac_status.h\"\n> #include \"controller/ccm_status.h\"\n> #include \"controller/contrast_status.h\"\n> +#include \"controller/decompand_status.h\"\n> #include \"controller/denoise_algorithm.h\"\n> #include \"controller/denoise_status.h\"\n> #include \"controller/dpc_status.h\"\n> @@ -113,6 +114,25 @@ int generateLut(const ipa::Pwl &pwl, uint32_t *lut, std::size_t lutSize,\n> \treturn 0;\n> }\n> \n> +int generateDecompandLut(const ipa::Pwl &pwl, uint16_t *lut, std::size_t lutSize = 65)\n\nPlease use a `Span lut` here. That will automatically pick up the size\nof the `decompand.lut` array in the call.\n\n\nRegards,\nBarnabás Pőcze\n\n\n> +{\n> +\tif (pwl.empty())\n> +\t\treturn -EINVAL;\n> +\n> +\tconstexpr int step = 1024;\n> +\tfor (std::size_t i = 0; i < lutSize; ++i) {\n> +\t\tint x = i * step;\n> +\n> +\t\tint y = pwl.eval(x);\n> +\t\tif (y < 0)\n> +\t\t\treturn -1;\n> +\n> +\t\tlut[i] = static_cast(std::min(y, 65535));\n> +\t}\n> +\n> +\treturn 0;\n> +}\n> +\n> void packLscLut(uint32_t packed[NumLscVertexes][NumLscVertexes],\n> \t\tdouble const rgb[3][NumLscVertexes][NumLscVertexes])\n> {\n> @@ -236,6 +256,7 @@ private:\n> \tvoid applyLensShading(const AlscStatus *alscStatus,\n> \t\t\t pisp_be_global_config &global);\n> \tvoid applyDPC(const DpcStatus *dpcStatus, pisp_be_global_config &global);\n> +\tvoid applyDecompand(const DecompandStatus *decompandStatus);\n> \tvoid applySdn(const SdnStatus *sdnStatus, pisp_be_global_config &global);\n> \tvoid applyTdn(const TdnStatus *tdnStatus, const DeviceStatus *deviceStatus,\n> \t\t pisp_be_global_config &global);\n> @@ -351,6 +372,11 @@ void IpaPiSP::platformPrepareIsp([[maybe_unused]] const PrepareParams ¶ms,\n> \t\tif (noiseStatus)\n> \t\t\tapplyFocusStats(noiseStatus);\n> \n> +\t\tDecompandStatus *decompandStatus =\n> +\t\t\trpiMetadata.getLocked(\"decompand.status\");\n> +\t\tif (decompandStatus)\n> +\t\t\tapplyDecompand(decompandStatus);\n> +\n> \t\tBlackLevelStatus *blackLevelStatus =\n> \t\t\trpiMetadata.getLocked(\"black_level.status\");\n> \t\tif (blackLevelStatus)\n> @@ -702,6 +728,20 @@ void IpaPiSP::applyDPC(const DpcStatus *dpcStatus, pisp_be_global_config &global\n> \tbe_->SetDpc(dpc);\n> }\n> \n> +void IpaPiSP::applyDecompand(const DecompandStatus *decompandStatus)\n> +{\n> +\tpisp_fe_global_config feGlobal;\n> +\tpisp_fe_decompand_config decompand = {};\n> +\n> +\tfe_->GetGlobal(feGlobal);\n> +\n> +\tif (!generateDecompandLut(decompandStatus->decompandCurve, decompand.lut, PISP_FE_DECOMPAND_LUT_SIZE)) {\n> +\t\tfe_->SetDecompand(decompand);\n> +\t\tfeGlobal.enables |= PISP_FE_ENABLE_DECOMPAND;\n> +\t\tfe_->SetGlobal(feGlobal);\n> +\t}\n> +}\n> +\n> void IpaPiSP::applySdn(const SdnStatus *sdnStatus, pisp_be_global_config &global)\n> {\n> \tpisp_be_sdn_config sdn = {};","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 1D4BFC328C\n\tfor ;\n\tThu, 2 Oct 2025 14:15:36 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 10E1C6B5AA;\n\tThu, 2 Oct 2025 16:15:35 +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 90D826B5A2\n\tfor ;\n\tThu, 2 Oct 2025 16:15:33 +0200 (CEST)","from [192.168.33.12] (185.221.142.146.nat.pool.zt.hu\n\t[185.221.142.146])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 2A148122A;\n\tThu, 2 Oct 2025 16:14:03 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"bQ00Q+qd\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1759414443;\n\tbh=lJYu3sKTFkzW6tXhf8iyIsuZmiSruTVxjR13896/ABM=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=bQ00Q+qd7RIVAeGYBjg/XjlfX8QXn9wI4IElM1rLU7e2tVeM6hipHTlrfvrUMDeFS\n\t2sHlHTP2qtyid1z2OHvrVml8F6sBmsv+jEs+ThTI3da6QkMPf0frseLSBAkpnq1AYJ\n\tgV+PRNJNSGT7l+mJtJQqyI1zKIYoQHppsl7AzVgg=","Message-ID":"<6aff1ba1-1813-4759-8b89-8f21a7336aa4@ideasonboard.com>","Date":"Thu, 2 Oct 2025 16:15:29 +0200","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v1 1/4] ipa: rpi: pisp: Add decompand support using PiSP\n\thardware block","To":"Naushir Patuck ,\n\tlibcamera-devel@lists.libcamera.org","Cc":"David Plowman ,\n\tNick Hollinghurst ,\n\tSena Asotani ","References":"<20251002133523.293413-1-naush@raspberrypi.com>\n\t<20251002133523.293413-2-naush@raspberrypi.com>","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Content-Language":"en-US, hu-HU","In-Reply-To":"<20251002133523.293413-2-naush@raspberrypi.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]