[v2,1/4] ipa: rpi: pisp: Add decompand support using PiSP hardware block

24558 diff mbox series

From: Sena Asotani <aso.fam429@gmail.com>

This patch integrates a new decompand algorithm that utilizes the PiSP
FE hardware block available on Raspberry Pi 5. The implementation
enables conversion of companded sensor data into linear format prior to
ISP processing.

Changes include:
- Implementation of decompand logic for controller and pipe interfaces
- Enabling decompand block by "rpi.decompand" in tuning.json

Signed-off-by: Sena Asotani <aso.fam429@gmail.com>
Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
Tested-by: Nick Hollinghurst <nick.hollinghurst@raspberrypi.com>
---
 src/ipa/rpi/controller/decompand_status.h |  8 ++++
 src/ipa/rpi/controller/meson.build        |  1 +
 src/ipa/rpi/controller/rpi/decompand.cpp  | 58 +++++++++++++++++++++++
 src/ipa/rpi/controller/rpi/decompand.h    | 31 ++++++++++++
 src/ipa/rpi/pisp/pisp.cpp                 | 38 +++++++++++++++
 5 files changed, 136 insertions(+)
 create mode 100644 src/ipa/rpi/controller/decompand_status.h
 create mode 100644 src/ipa/rpi/controller/rpi/decompand.cpp
 create mode 100644 src/ipa/rpi/controller/rpi/decompand.h

Hi Naush

Thanks for the patch.

On Fri, 3 Oct 2025 at 13:18, Naushir Patuck <naush@raspberrypi.com> wrote:
>
> From: Sena Asotani <aso.fam429@gmail.com>
>
> This patch integrates a new decompand algorithm that utilizes the PiSP
> FE hardware block available on Raspberry Pi 5. The implementation
> enables conversion of companded sensor data into linear format prior to
> ISP processing.
>
> Changes include:
> - Implementation of decompand logic for controller and pipe interfaces
> - Enabling decompand block by "rpi.decompand" in tuning.json
>
> Signed-off-by: Sena Asotani <aso.fam429@gmail.com>
> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> Tested-by: Nick Hollinghurst <nick.hollinghurst@raspberrypi.com>
> ---
>  src/ipa/rpi/controller/decompand_status.h |  8 ++++
>  src/ipa/rpi/controller/meson.build        |  1 +
>  src/ipa/rpi/controller/rpi/decompand.cpp  | 58 +++++++++++++++++++++++
>  src/ipa/rpi/controller/rpi/decompand.h    | 31 ++++++++++++
>  src/ipa/rpi/pisp/pisp.cpp                 | 38 +++++++++++++++
>  5 files changed, 136 insertions(+)
>  create mode 100644 src/ipa/rpi/controller/decompand_status.h
>  create mode 100644 src/ipa/rpi/controller/rpi/decompand.cpp
>  create mode 100644 src/ipa/rpi/controller/rpi/decompand.h
>
> diff --git a/src/ipa/rpi/controller/decompand_status.h b/src/ipa/rpi/controller/decompand_status.h
> new file mode 100644
> index 000000000000..2d9888dca4f3
> --- /dev/null
> +++ b/src/ipa/rpi/controller/decompand_status.h
> @@ -0,0 +1,8 @@
> +#pragma once
> +
> +#include "libipa/pwl.h"
> +
> +struct DecompandStatus {
> +       uint32_t bitdepth;

I wonder if a short comment somewhere explaining how we use the
bitdepth would be useful? It wasn't immediately clear to me that we're
expecting a single mode with a single bit depth to need decompanding,
or all the modes (when bitdepth is zero).

But this minor point notwithstanding:

Reviewed-by: David Plowman <david.plowman@raspberrypi.com>

Thanks!
David

> +       libcamera::ipa::Pwl decompandCurve;
> +};
> diff --git a/src/ipa/rpi/controller/meson.build b/src/ipa/rpi/controller/meson.build
> index 74b74888bbff..c13c48539d10 100644
> --- a/src/ipa/rpi/controller/meson.build
> +++ b/src/ipa/rpi/controller/meson.build
> @@ -14,6 +14,7 @@ rpi_ipa_controller_sources = files([
>      'rpi/cac.cpp',
>      'rpi/ccm.cpp',
>      'rpi/contrast.cpp',
> +    'rpi/decompand.cpp',
>      'rpi/denoise.cpp',
>      'rpi/dpc.cpp',
>      'rpi/geq.cpp',
> diff --git a/src/ipa/rpi/controller/rpi/decompand.cpp b/src/ipa/rpi/controller/rpi/decompand.cpp
> new file mode 100644
> index 000000000000..2036750f82f4
> --- /dev/null
> +++ b/src/ipa/rpi/controller/rpi/decompand.cpp
> @@ -0,0 +1,58 @@
> +#include "decompand.h"
> +
> +#include <libcamera/base/log.h>
> +
> +#include "../decompand_status.h"
> +#include "../histogram.h"
> +
> +using namespace RPiController;
> +using namespace libcamera;
> +
> +LOG_DEFINE_CATEGORY(RPiDecompand)
> +
> +#define NAME "rpi.decompand"
> +
> +Decompand::Decompand(Controller *controller)
> +       : Algorithm(controller)
> +{
> +}
> +
> +char const *Decompand::name() const
> +{
> +       return NAME;
> +}
> +
> +int Decompand::read(const libcamera::YamlObject &params)
> +{
> +       config_.bitdepth = params["bitdepth"].get<uint32_t>(0);
> +       config_.decompandCurve = params["decompand_curve"].get<ipa::Pwl>(ipa::Pwl{});
> +       return config_.decompandCurve.empty() ? -EINVAL : 0;
> +}
> +
> +void Decompand::initialise()
> +{
> +}
> +
> +void Decompand::switchMode(CameraMode const &cameraMode,
> +                          [[maybe_unused]] Metadata *metadata)
> +{
> +       mode_ = cameraMode;
> +}
> +
> +void Decompand::prepare(Metadata *imageMetadata)
> +{
> +       DecompandStatus decompandStatus;
> +
> +       if (config_.bitdepth == 0 || mode_.bitdepth == config_.bitdepth) {
> +               decompandStatus.decompandCurve = config_.decompandCurve;
> +               imageMetadata->set("decompand.status", decompandStatus);
> +       }
> +}
> +
> +/* Register algorithm with the system. */
> +static Algorithm *create(Controller *controller)
> +{
> +       return new Decompand(controller);
> +}
> +
> +static RegisterAlgorithm reg(NAME, &create);
> diff --git a/src/ipa/rpi/controller/rpi/decompand.h b/src/ipa/rpi/controller/rpi/decompand.h
> new file mode 100644
> index 000000000000..38b26a21e6d5
> --- /dev/null
> +++ b/src/ipa/rpi/controller/rpi/decompand.h
> @@ -0,0 +1,31 @@
> +#pragma once
> +
> +#include <libipa/pwl.h>
> +
> +#include "../decompand_status.h"
> +
> +#include "algorithm.h"
> +
> +namespace RPiController {
> +
> +struct DecompandConfig {
> +       uint32_t bitdepth;
> +       libcamera::ipa::Pwl decompandCurve;
> +};
> +
> +class Decompand : public Algorithm
> +{
> +public:
> +       Decompand(Controller *controller = nullptr);
> +       char const *name() const override;
> +       int read(const libcamera::YamlObject &params) override;
> +       void initialise() override;
> +       void switchMode(CameraMode const &cameraMode, Metadata *metadata) override;
> +       void prepare(Metadata *imageMetadata) override;
> +
> +private:
> +       CameraMode mode_;
> +       DecompandConfig config_;
> +};
> +
> +} /* namespace RPiController */
> diff --git a/src/ipa/rpi/pisp/pisp.cpp b/src/ipa/rpi/pisp/pisp.cpp
> index 829b91258522..14ece12b0895 100644
> --- a/src/ipa/rpi/pisp/pisp.cpp
> +++ b/src/ipa/rpi/pisp/pisp.cpp
> @@ -32,6 +32,7 @@
>  #include "controller/cac_status.h"
>  #include "controller/ccm_status.h"
>  #include "controller/contrast_status.h"
> +#include "controller/decompand_status.h"
>  #include "controller/denoise_algorithm.h"
>  #include "controller/denoise_status.h"
>  #include "controller/dpc_status.h"
> @@ -113,6 +114,25 @@ int generateLut(const ipa::Pwl &pwl, uint32_t *lut, std::size_t lutSize,
>         return 0;
>  }
>
> +int generateDecompandLut(const ipa::Pwl &pwl, Span<uint16_t> lut)
> +{
> +       if (pwl.empty())
> +               return -EINVAL;
> +
> +       constexpr int step = 1024;
> +       for (std::size_t i = 0; i < lut.size(); ++i) {
> +               int x = i * step;
> +
> +               int y = pwl.eval(x);
> +               if (y < 0)
> +                       return -1;
> +
> +               lut[i] = static_cast<uint16_t>(std::min(y, 65535));
> +       }
> +
> +       return 0;
> +}
> +
>  void packLscLut(uint32_t packed[NumLscVertexes][NumLscVertexes],
>                 double const rgb[3][NumLscVertexes][NumLscVertexes])
>  {
> @@ -236,6 +256,7 @@ private:
>         void applyLensShading(const AlscStatus *alscStatus,
>                               pisp_be_global_config &global);
>         void applyDPC(const DpcStatus *dpcStatus, pisp_be_global_config &global);
> +       void applyDecompand(const DecompandStatus *decompandStatus);
>         void applySdn(const SdnStatus *sdnStatus, pisp_be_global_config &global);
>         void applyTdn(const TdnStatus *tdnStatus, const DeviceStatus *deviceStatus,
>                       pisp_be_global_config &global);
> @@ -351,6 +372,11 @@ void IpaPiSP::platformPrepareIsp([[maybe_unused]] const PrepareParams &params,
>                 if (noiseStatus)
>                         applyFocusStats(noiseStatus);
>
> +               DecompandStatus *decompandStatus =
> +                       rpiMetadata.getLocked<DecompandStatus>("decompand.status");
> +               if (decompandStatus)
> +                       applyDecompand(decompandStatus);
> +
>                 BlackLevelStatus *blackLevelStatus =
>                         rpiMetadata.getLocked<BlackLevelStatus>("black_level.status");
>                 if (blackLevelStatus)
> @@ -702,6 +728,18 @@ void IpaPiSP::applyDPC(const DpcStatus *dpcStatus, pisp_be_global_config &global
>         be_->SetDpc(dpc);
>  }
>
> +void IpaPiSP::applyDecompand(const DecompandStatus *decompandStatus)
> +{
> +       pisp_fe_global_config feGlobal;
> +       pisp_fe_decompand_config decompand = {};
> +
> +       if (!generateDecompandLut(decompandStatus->decompandCurve, decompand.lut)) {
> +               fe_->SetDecompand(decompand);
> +               feGlobal.enables |= PISP_FE_ENABLE_DECOMPAND;
> +               fe_->SetGlobal(feGlobal);
> +       }
> +}
> +
>  void IpaPiSP::applySdn(const SdnStatus *sdnStatus, pisp_be_global_config &global)
>  {
>         pisp_be_sdn_config sdn = {};
> --
> 2.43.0
>

Hello Naush, Sena,

On Fri, Oct 03, 2025 at 01:15:52PM +0100, Naushir Patuck wrote:
> From: Sena Asotani <aso.fam429@gmail.com>
> 
> This patch integrates a new decompand algorithm that utilizes the PiSP
> FE hardware block available on Raspberry Pi 5. The implementation
> enables conversion of companded sensor data into linear format prior to
> ISP processing.
> 
> Changes include:
> - Implementation of decompand logic for controller and pipe interfaces
> - Enabling decompand block by "rpi.decompand" in tuning.json

I've worked on something similar for i.MX8MP, with a different approach.

The companding curve is an intrinsic property of the sensor. Depending
on the sensor, it can be

- hardcoded or configurable
- when configurable, specified in different ways (e.g. piecewise linear
  function, parametric formula or selectable from presets)
- always enabled or controllable

This creates a relatively large problem space. libcamera needs to know
when a compression curve is applied by the sensor, and what curve is
being applied. Additionally, it may require the ability to
enable/disable compression, as well as the ability to configure the
curve.

Specifying the curve in a tuning file doesn't seem to be a good
solution. I've opted to support hardcoded curves only to start with (in
order to simplify the problem), and report the curve from the sensor
helper. I've also added a V4L2 control ([1]) to enable and disable
compression, with support in the AR0144 driver([2]).

Do you plan to implement proper companding support for the RPi pipeline
handler and IPA module ?

[1] https://lore.kernel.org/linux-media/20240905225308.11267-9-laurent.pinchart@ideasonboard.com/
[2] https://lore.kernel.org/linux-media/20240905225308.11267-10-laurent.pinchart@ideasonboard.com/

> Signed-off-by: Sena Asotani <aso.fam429@gmail.com>
> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> Tested-by: Nick Hollinghurst <nick.hollinghurst@raspberrypi.com>
> ---
>  src/ipa/rpi/controller/decompand_status.h |  8 ++++
>  src/ipa/rpi/controller/meson.build        |  1 +
>  src/ipa/rpi/controller/rpi/decompand.cpp  | 58 +++++++++++++++++++++++
>  src/ipa/rpi/controller/rpi/decompand.h    | 31 ++++++++++++
>  src/ipa/rpi/pisp/pisp.cpp                 | 38 +++++++++++++++
>  5 files changed, 136 insertions(+)
>  create mode 100644 src/ipa/rpi/controller/decompand_status.h
>  create mode 100644 src/ipa/rpi/controller/rpi/decompand.cpp
>  create mode 100644 src/ipa/rpi/controller/rpi/decompand.h
> 
> diff --git a/src/ipa/rpi/controller/decompand_status.h b/src/ipa/rpi/controller/decompand_status.h
> new file mode 100644
> index 000000000000..2d9888dca4f3
> --- /dev/null
> +++ b/src/ipa/rpi/controller/decompand_status.h
> @@ -0,0 +1,8 @@
> +#pragma once
> +
> +#include "libipa/pwl.h"
> +
> +struct DecompandStatus {
> +	uint32_t bitdepth;
> +	libcamera::ipa::Pwl decompandCurve;
> +};
> diff --git a/src/ipa/rpi/controller/meson.build b/src/ipa/rpi/controller/meson.build
> index 74b74888bbff..c13c48539d10 100644
> --- a/src/ipa/rpi/controller/meson.build
> +++ b/src/ipa/rpi/controller/meson.build
> @@ -14,6 +14,7 @@ rpi_ipa_controller_sources = files([
>      'rpi/cac.cpp',
>      'rpi/ccm.cpp',
>      'rpi/contrast.cpp',
> +    'rpi/decompand.cpp',
>      'rpi/denoise.cpp',
>      'rpi/dpc.cpp',
>      'rpi/geq.cpp',
> diff --git a/src/ipa/rpi/controller/rpi/decompand.cpp b/src/ipa/rpi/controller/rpi/decompand.cpp
> new file mode 100644
> index 000000000000..2036750f82f4
> --- /dev/null
> +++ b/src/ipa/rpi/controller/rpi/decompand.cpp
> @@ -0,0 +1,58 @@
> +#include "decompand.h"
> +
> +#include <libcamera/base/log.h>
> +
> +#include "../decompand_status.h"
> +#include "../histogram.h"
> +
> +using namespace RPiController;
> +using namespace libcamera;
> +
> +LOG_DEFINE_CATEGORY(RPiDecompand)
> +
> +#define NAME "rpi.decompand"
> +
> +Decompand::Decompand(Controller *controller)
> +	: Algorithm(controller)
> +{
> +}
> +
> +char const *Decompand::name() const
> +{
> +	return NAME;
> +}
> +
> +int Decompand::read(const libcamera::YamlObject &params)
> +{
> +	config_.bitdepth = params["bitdepth"].get<uint32_t>(0);
> +	config_.decompandCurve = params["decompand_curve"].get<ipa::Pwl>(ipa::Pwl{});
> +	return config_.decompandCurve.empty() ? -EINVAL : 0;
> +}
> +
> +void Decompand::initialise()
> +{
> +}
> +
> +void Decompand::switchMode(CameraMode const &cameraMode,
> +			   [[maybe_unused]] Metadata *metadata)
> +{
> +	mode_ = cameraMode;
> +}
> +
> +void Decompand::prepare(Metadata *imageMetadata)
> +{
> +	DecompandStatus decompandStatus;
> +
> +	if (config_.bitdepth == 0 || mode_.bitdepth == config_.bitdepth) {
> +		decompandStatus.decompandCurve = config_.decompandCurve;
> +		imageMetadata->set("decompand.status", decompandStatus);
> +	}
> +}
> +
> +/* Register algorithm with the system. */
> +static Algorithm *create(Controller *controller)
> +{
> +	return new Decompand(controller);
> +}
> +
> +static RegisterAlgorithm reg(NAME, &create);
> diff --git a/src/ipa/rpi/controller/rpi/decompand.h b/src/ipa/rpi/controller/rpi/decompand.h
> new file mode 100644
> index 000000000000..38b26a21e6d5
> --- /dev/null
> +++ b/src/ipa/rpi/controller/rpi/decompand.h
> @@ -0,0 +1,31 @@
> +#pragma once
> +
> +#include <libipa/pwl.h>
> +
> +#include "../decompand_status.h"
> +
> +#include "algorithm.h"
> +
> +namespace RPiController {
> +
> +struct DecompandConfig {
> +	uint32_t bitdepth;
> +	libcamera::ipa::Pwl decompandCurve;
> +};
> +
> +class Decompand : public Algorithm
> +{
> +public:
> +	Decompand(Controller *controller = nullptr);
> +	char const *name() const override;
> +	int read(const libcamera::YamlObject &params) override;
> +	void initialise() override;
> +	void switchMode(CameraMode const &cameraMode, Metadata *metadata) override;
> +	void prepare(Metadata *imageMetadata) override;
> +
> +private:
> +	CameraMode mode_;
> +	DecompandConfig config_;
> +};
> +
> +} /* namespace RPiController */
> diff --git a/src/ipa/rpi/pisp/pisp.cpp b/src/ipa/rpi/pisp/pisp.cpp
> index 829b91258522..14ece12b0895 100644
> --- a/src/ipa/rpi/pisp/pisp.cpp
> +++ b/src/ipa/rpi/pisp/pisp.cpp
> @@ -32,6 +32,7 @@
>  #include "controller/cac_status.h"
>  #include "controller/ccm_status.h"
>  #include "controller/contrast_status.h"
> +#include "controller/decompand_status.h"
>  #include "controller/denoise_algorithm.h"
>  #include "controller/denoise_status.h"
>  #include "controller/dpc_status.h"
> @@ -113,6 +114,25 @@ int generateLut(const ipa::Pwl &pwl, uint32_t *lut, std::size_t lutSize,
>  	return 0;
>  }
>  
> +int generateDecompandLut(const ipa::Pwl &pwl, Span<uint16_t> lut)
> +{
> +	if (pwl.empty())
> +		return -EINVAL;
> +
> +	constexpr int step = 1024;
> +	for (std::size_t i = 0; i < lut.size(); ++i) {
> +		int x = i * step;
> +
> +		int y = pwl.eval(x);
> +		if (y < 0)
> +			return -1;
> +
> +		lut[i] = static_cast<uint16_t>(std::min(y, 65535));
> +	}
> +
> +	return 0;
> +}
> +
>  void packLscLut(uint32_t packed[NumLscVertexes][NumLscVertexes],
>  		double const rgb[3][NumLscVertexes][NumLscVertexes])
>  {
> @@ -236,6 +256,7 @@ private:
>  	void applyLensShading(const AlscStatus *alscStatus,
>  			      pisp_be_global_config &global);
>  	void applyDPC(const DpcStatus *dpcStatus, pisp_be_global_config &global);
> +	void applyDecompand(const DecompandStatus *decompandStatus);
>  	void applySdn(const SdnStatus *sdnStatus, pisp_be_global_config &global);
>  	void applyTdn(const TdnStatus *tdnStatus, const DeviceStatus *deviceStatus,
>  		      pisp_be_global_config &global);
> @@ -351,6 +372,11 @@ void IpaPiSP::platformPrepareIsp([[maybe_unused]] const PrepareParams &params,
>  		if (noiseStatus)
>  			applyFocusStats(noiseStatus);
>  
> +		DecompandStatus *decompandStatus =
> +			rpiMetadata.getLocked<DecompandStatus>("decompand.status");
> +		if (decompandStatus)
> +			applyDecompand(decompandStatus);
> +
>  		BlackLevelStatus *blackLevelStatus =
>  			rpiMetadata.getLocked<BlackLevelStatus>("black_level.status");
>  		if (blackLevelStatus)
> @@ -702,6 +728,18 @@ void IpaPiSP::applyDPC(const DpcStatus *dpcStatus, pisp_be_global_config &global
>  	be_->SetDpc(dpc);
>  }
>  
> +void IpaPiSP::applyDecompand(const DecompandStatus *decompandStatus)
> +{
> +	pisp_fe_global_config feGlobal;
> +	pisp_fe_decompand_config decompand = {};
> +
> +	if (!generateDecompandLut(decompandStatus->decompandCurve, decompand.lut)) {
> +		fe_->SetDecompand(decompand);
> +		feGlobal.enables |= PISP_FE_ENABLE_DECOMPAND;
> +		fe_->SetGlobal(feGlobal);
> +	}
> +}
> +
>  void IpaPiSP::applySdn(const SdnStatus *sdnStatus, pisp_be_global_config &global)
>  {
>  	pisp_be_sdn_config sdn = {};

Hi Laurent,

On Mon, 13 Oct 2025 at 15:50, Laurent Pinchart <
laurent.pinchart@ideasonboard.com> wrote:

> Hello Naush, Sena,
>
> On Fri, Oct 03, 2025 at 01:15:52PM +0100, Naushir Patuck wrote:
> > From: Sena Asotani <aso.fam429@gmail.com>
> >
> > This patch integrates a new decompand algorithm that utilizes the PiSP
> > FE hardware block available on Raspberry Pi 5. The implementation
> > enables conversion of companded sensor data into linear format prior to
> > ISP processing.
> >
> > Changes include:
> > - Implementation of decompand logic for controller and pipe interfaces
> > - Enabling decompand block by "rpi.decompand" in tuning.json
>
> I've worked on something similar for i.MX8MP, with a different approach.
>
> The companding curve is an intrinsic property of the sensor. Depending
> on the sensor, it can be
>
> - hardcoded or configurable
> - when configurable, specified in different ways (e.g. piecewise linear
>   function, parametric formula or selectable from presets)
> - always enabled or controllable
>
> This creates a relatively large problem space. libcamera needs to know
> when a compression curve is applied by the sensor, and what curve is
> being applied. Additionally, it may require the ability to
> enable/disable compression, as well as the ability to configure the
> curve.
>
> Specifying the curve in a tuning file doesn't seem to be a good
> solution. I've opted to support hardcoded curves only to start with (in
> order to simplify the problem), and report the curve from the sensor
> helper. I've also added a V4L2 control ([1]) to enable and disable
> compression, with support in the AR0144 driver([2]).
>

This work for implementing decompanding was done by Sena (cc'ed) for the
IMX585 which is not yet upstreamed.  I've not worked on any sensors myself
that use this feature, so I can't really tell if a hard-coded curve vs a
user programmable one is the best approach.  Having said that, I would
agree that a V4L2 control to enable/disable companding (+ a libcamera
control of course) on the sensor is the right approach!


> Do you plan to implement proper companding support for the RPi pipeline
> handler and IPA module ?
>

Do you mean adding the relevant libcamera controls for use by an
application?  Apart from that, and having a user programmable curve, this
patch set does include everything else required for the IPA/PH to enable
the "decomand" feature in the frontend.  It seems to do the right thing for
the IMX585 as reported by Sena.

Regards,
Naush


> [1]
> https://lore.kernel.org/linux-media/20240905225308.11267-9-laurent.pinchart@ideasonboard.com/
> [2]
> https://lore.kernel.org/linux-media/20240905225308.11267-10-laurent.pinchart@ideasonboard.com/
>
> > Signed-off-by: Sena Asotani <aso.fam429@gmail.com>
> > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> > Tested-by: Nick Hollinghurst <nick.hollinghurst@raspberrypi.com>
> > ---
> >  src/ipa/rpi/controller/decompand_status.h |  8 ++++
> >  src/ipa/rpi/controller/meson.build        |  1 +
> >  src/ipa/rpi/controller/rpi/decompand.cpp  | 58 +++++++++++++++++++++++
> >  src/ipa/rpi/controller/rpi/decompand.h    | 31 ++++++++++++
> >  src/ipa/rpi/pisp/pisp.cpp                 | 38 +++++++++++++++
> >  5 files changed, 136 insertions(+)
> >  create mode 100644 src/ipa/rpi/controller/decompand_status.h
> >  create mode 100644 src/ipa/rpi/controller/rpi/decompand.cpp
> >  create mode 100644 src/ipa/rpi/controller/rpi/decompand.h
> >
> > diff --git a/src/ipa/rpi/controller/decompand_status.h
> b/src/ipa/rpi/controller/decompand_status.h
> > new file mode 100644
> > index 000000000000..2d9888dca4f3
> > --- /dev/null
> > +++ b/src/ipa/rpi/controller/decompand_status.h
> > @@ -0,0 +1,8 @@
> > +#pragma once
> > +
> > +#include "libipa/pwl.h"
> > +
> > +struct DecompandStatus {
> > +     uint32_t bitdepth;
> > +     libcamera::ipa::Pwl decompandCurve;
> > +};
> > diff --git a/src/ipa/rpi/controller/meson.build
> b/src/ipa/rpi/controller/meson.build
> > index 74b74888bbff..c13c48539d10 100644
> > --- a/src/ipa/rpi/controller/meson.build
> > +++ b/src/ipa/rpi/controller/meson.build
> > @@ -14,6 +14,7 @@ rpi_ipa_controller_sources = files([
> >      'rpi/cac.cpp',
> >      'rpi/ccm.cpp',
> >      'rpi/contrast.cpp',
> > +    'rpi/decompand.cpp',
> >      'rpi/denoise.cpp',
> >      'rpi/dpc.cpp',
> >      'rpi/geq.cpp',
> > diff --git a/src/ipa/rpi/controller/rpi/decompand.cpp
> b/src/ipa/rpi/controller/rpi/decompand.cpp
> > new file mode 100644
> > index 000000000000..2036750f82f4
> > --- /dev/null
> > +++ b/src/ipa/rpi/controller/rpi/decompand.cpp
> > @@ -0,0 +1,58 @@
> > +#include "decompand.h"
> > +
> > +#include <libcamera/base/log.h>
> > +
> > +#include "../decompand_status.h"
> > +#include "../histogram.h"
> > +
> > +using namespace RPiController;
> > +using namespace libcamera;
> > +
> > +LOG_DEFINE_CATEGORY(RPiDecompand)
> > +
> > +#define NAME "rpi.decompand"
> > +
> > +Decompand::Decompand(Controller *controller)
> > +     : Algorithm(controller)
> > +{
> > +}
> > +
> > +char const *Decompand::name() const
> > +{
> > +     return NAME;
> > +}
> > +
> > +int Decompand::read(const libcamera::YamlObject &params)
> > +{
> > +     config_.bitdepth = params["bitdepth"].get<uint32_t>(0);
> > +     config_.decompandCurve =
> params["decompand_curve"].get<ipa::Pwl>(ipa::Pwl{});
> > +     return config_.decompandCurve.empty() ? -EINVAL : 0;
> > +}
> > +
> > +void Decompand::initialise()
> > +{
> > +}
> > +
> > +void Decompand::switchMode(CameraMode const &cameraMode,
> > +                        [[maybe_unused]] Metadata *metadata)
> > +{
> > +     mode_ = cameraMode;
> > +}
> > +
> > +void Decompand::prepare(Metadata *imageMetadata)
> > +{
> > +     DecompandStatus decompandStatus;
> > +
> > +     if (config_.bitdepth == 0 || mode_.bitdepth == config_.bitdepth) {
> > +             decompandStatus.decompandCurve = config_.decompandCurve;
> > +             imageMetadata->set("decompand.status", decompandStatus);
> > +     }
> > +}
> > +
> > +/* Register algorithm with the system. */
> > +static Algorithm *create(Controller *controller)
> > +{
> > +     return new Decompand(controller);
> > +}
> > +
> > +static RegisterAlgorithm reg(NAME, &create);
> > diff --git a/src/ipa/rpi/controller/rpi/decompand.h
> b/src/ipa/rpi/controller/rpi/decompand.h
> > new file mode 100644
> > index 000000000000..38b26a21e6d5
> > --- /dev/null
> > +++ b/src/ipa/rpi/controller/rpi/decompand.h
> > @@ -0,0 +1,31 @@
> > +#pragma once
> > +
> > +#include <libipa/pwl.h>
> > +
> > +#include "../decompand_status.h"
> > +
> > +#include "algorithm.h"
> > +
> > +namespace RPiController {
> > +
> > +struct DecompandConfig {
> > +     uint32_t bitdepth;
> > +     libcamera::ipa::Pwl decompandCurve;
> > +};
> > +
> > +class Decompand : public Algorithm
> > +{
> > +public:
> > +     Decompand(Controller *controller = nullptr);
> > +     char const *name() const override;
> > +     int read(const libcamera::YamlObject &params) override;
> > +     void initialise() override;
> > +     void switchMode(CameraMode const &cameraMode, Metadata *metadata)
> override;
> > +     void prepare(Metadata *imageMetadata) override;
> > +
> > +private:
> > +     CameraMode mode_;
> > +     DecompandConfig config_;
> > +};
> > +
> > +} /* namespace RPiController */
> > diff --git a/src/ipa/rpi/pisp/pisp.cpp b/src/ipa/rpi/pisp/pisp.cpp
> > index 829b91258522..14ece12b0895 100644
> > --- a/src/ipa/rpi/pisp/pisp.cpp
> > +++ b/src/ipa/rpi/pisp/pisp.cpp
> > @@ -32,6 +32,7 @@
> >  #include "controller/cac_status.h"
> >  #include "controller/ccm_status.h"
> >  #include "controller/contrast_status.h"
> > +#include "controller/decompand_status.h"
> >  #include "controller/denoise_algorithm.h"
> >  #include "controller/denoise_status.h"
> >  #include "controller/dpc_status.h"
> > @@ -113,6 +114,25 @@ int generateLut(const ipa::Pwl &pwl, uint32_t *lut,
> std::size_t lutSize,
> >       return 0;
> >  }
> >
> > +int generateDecompandLut(const ipa::Pwl &pwl, Span<uint16_t> lut)
> > +{
> > +     if (pwl.empty())
> > +             return -EINVAL;
> > +
> > +     constexpr int step = 1024;
> > +     for (std::size_t i = 0; i < lut.size(); ++i) {
> > +             int x = i * step;
> > +
> > +             int y = pwl.eval(x);
> > +             if (y < 0)
> > +                     return -1;
> > +
> > +             lut[i] = static_cast<uint16_t>(std::min(y, 65535));
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> >  void packLscLut(uint32_t packed[NumLscVertexes][NumLscVertexes],
> >               double const rgb[3][NumLscVertexes][NumLscVertexes])
> >  {
> > @@ -236,6 +256,7 @@ private:
> >       void applyLensShading(const AlscStatus *alscStatus,
> >                             pisp_be_global_config &global);
> >       void applyDPC(const DpcStatus *dpcStatus, pisp_be_global_config
> &global);
> > +     void applyDecompand(const DecompandStatus *decompandStatus);
> >       void applySdn(const SdnStatus *sdnStatus, pisp_be_global_config
> &global);
> >       void applyTdn(const TdnStatus *tdnStatus, const DeviceStatus
> *deviceStatus,
> >                     pisp_be_global_config &global);
> > @@ -351,6 +372,11 @@ void IpaPiSP::platformPrepareIsp([[maybe_unused]]
> const PrepareParams &params,
> >               if (noiseStatus)
> >                       applyFocusStats(noiseStatus);
> >
> > +             DecompandStatus *decompandStatus =
> > +
>  rpiMetadata.getLocked<DecompandStatus>("decompand.status");
> > +             if (decompandStatus)
> > +                     applyDecompand(decompandStatus);
> > +
> >               BlackLevelStatus *blackLevelStatus =
> >
>  rpiMetadata.getLocked<BlackLevelStatus>("black_level.status");
> >               if (blackLevelStatus)
> > @@ -702,6 +728,18 @@ void IpaPiSP::applyDPC(const DpcStatus *dpcStatus,
> pisp_be_global_config &global
> >       be_->SetDpc(dpc);
> >  }
> >
> > +void IpaPiSP::applyDecompand(const DecompandStatus *decompandStatus)
> > +{
> > +     pisp_fe_global_config feGlobal;
> > +     pisp_fe_decompand_config decompand = {};
> > +
> > +     if (!generateDecompandLut(decompandStatus->decompandCurve,
> decompand.lut)) {
> > +             fe_->SetDecompand(decompand);
> > +             feGlobal.enables |= PISP_FE_ENABLE_DECOMPAND;
> > +             fe_->SetGlobal(feGlobal);
> > +     }
> > +}
> > +
> >  void IpaPiSP::applySdn(const SdnStatus *sdnStatus,
> pisp_be_global_config &global)
> >  {
> >       pisp_be_sdn_config sdn = {};
>
> --
> Regards,
>
> Laurent Pinchart
>

Hi Naush,

On Tue, Oct 14, 2025 at 08:26:33AM +0100, Naushir Patuck wrote:
> On Mon, 13 Oct 2025 at 15:50, Laurent Pinchart wrote:
> > On Fri, Oct 03, 2025 at 01:15:52PM +0100, Naushir Patuck wrote:
> > > From: Sena Asotani <aso.fam429@gmail.com>
> > >
> > > This patch integrates a new decompand algorithm that utilizes the PiSP
> > > FE hardware block available on Raspberry Pi 5. The implementation
> > > enables conversion of companded sensor data into linear format prior to
> > > ISP processing.
> > >
> > > Changes include:
> > > - Implementation of decompand logic for controller and pipe interfaces
> > > - Enabling decompand block by "rpi.decompand" in tuning.json
> >
> > I've worked on something similar for i.MX8MP, with a different approach.
> >
> > The companding curve is an intrinsic property of the sensor. Depending
> > on the sensor, it can be
> >
> > - hardcoded or configurable
> > - when configurable, specified in different ways (e.g. piecewise linear
> >   function, parametric formula or selectable from presets)
> > - always enabled or controllable
> >
> > This creates a relatively large problem space. libcamera needs to know
> > when a compression curve is applied by the sensor, and what curve is
> > being applied. Additionally, it may require the ability to
> > enable/disable compression, as well as the ability to configure the
> > curve.
> >
> > Specifying the curve in a tuning file doesn't seem to be a good
> > solution. I've opted to support hardcoded curves only to start with (in
> > order to simplify the problem), and report the curve from the sensor
> > helper. I've also added a V4L2 control ([1]) to enable and disable
> > compression, with support in the AR0144 driver([2]).
> 
> This work for implementing decompanding was done by Sena (cc'ed) for the
> IMX585 which is not yet upstreamed.  I've not worked on any sensors myself
> that use this feature, so I can't really tell if a hard-coded curve vs a
> user programmable one is the best approach.

I've seen both cases implemented in sensors, so we need to support
hardcoded curves, and may want to support configurable curves.

> Having said that, I would
> agree that a V4L2 control to enable/disable companding (+ a libcamera
> control of course) on the sensor is the right approach!

On the camera side I expect it would be a SensorConfiguration parameter
rather than a control.

> > Do you plan to implement proper companding support for the RPi pipeline
> > handler and IPA module ?
> 
> Do you mean adding the relevant libcamera controls for use by an
> application?  Apart from that, and having a user programmable curve, this
> patch set does include everything else required for the IPA/PH to enable
> the "decomand" feature in the frontend.  It seems to do the right thing for
> the IMX585 as reported by Sena.

I meant

- Making companding dynamically selectable by the user
- Optionally supporting user-specific curves
- Moving the curve from tuning files to sensor helpers

> > [1] https://lore.kernel.org/linux-media/20240905225308.11267-9-laurent.pinchart@ideasonboard.com/
> > [2] https://lore.kernel.org/linux-media/20240905225308.11267-10-laurent.pinchart@ideasonboard.com/
> >
> > > Signed-off-by: Sena Asotani <aso.fam429@gmail.com>
> > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> > > Tested-by: Nick Hollinghurst <nick.hollinghurst@raspberrypi.com>
> > > ---
> > >  src/ipa/rpi/controller/decompand_status.h |  8 ++++
> > >  src/ipa/rpi/controller/meson.build        |  1 +
> > >  src/ipa/rpi/controller/rpi/decompand.cpp  | 58 +++++++++++++++++++++++
> > >  src/ipa/rpi/controller/rpi/decompand.h    | 31 ++++++++++++
> > >  src/ipa/rpi/pisp/pisp.cpp                 | 38 +++++++++++++++
> > >  5 files changed, 136 insertions(+)
> > >  create mode 100644 src/ipa/rpi/controller/decompand_status.h
> > >  create mode 100644 src/ipa/rpi/controller/rpi/decompand.cpp
> > >  create mode 100644 src/ipa/rpi/controller/rpi/decompand.h
> > >
> > > diff --git a/src/ipa/rpi/controller/decompand_status.h
> > b/src/ipa/rpi/controller/decompand_status.h
> > > new file mode 100644
> > > index 000000000000..2d9888dca4f3
> > > --- /dev/null
> > > +++ b/src/ipa/rpi/controller/decompand_status.h
> > > @@ -0,0 +1,8 @@
> > > +#pragma once
> > > +
> > > +#include "libipa/pwl.h"
> > > +
> > > +struct DecompandStatus {
> > > +     uint32_t bitdepth;
> > > +     libcamera::ipa::Pwl decompandCurve;
> > > +};
> > > diff --git a/src/ipa/rpi/controller/meson.build
> > b/src/ipa/rpi/controller/meson.build
> > > index 74b74888bbff..c13c48539d10 100644
> > > --- a/src/ipa/rpi/controller/meson.build
> > > +++ b/src/ipa/rpi/controller/meson.build
> > > @@ -14,6 +14,7 @@ rpi_ipa_controller_sources = files([
> > >      'rpi/cac.cpp',
> > >      'rpi/ccm.cpp',
> > >      'rpi/contrast.cpp',
> > > +    'rpi/decompand.cpp',
> > >      'rpi/denoise.cpp',
> > >      'rpi/dpc.cpp',
> > >      'rpi/geq.cpp',
> > > diff --git a/src/ipa/rpi/controller/rpi/decompand.cpp
> > b/src/ipa/rpi/controller/rpi/decompand.cpp
> > > new file mode 100644
> > > index 000000000000..2036750f82f4
> > > --- /dev/null
> > > +++ b/src/ipa/rpi/controller/rpi/decompand.cpp
> > > @@ -0,0 +1,58 @@
> > > +#include "decompand.h"
> > > +
> > > +#include <libcamera/base/log.h>
> > > +
> > > +#include "../decompand_status.h"
> > > +#include "../histogram.h"
> > > +
> > > +using namespace RPiController;
> > > +using namespace libcamera;
> > > +
> > > +LOG_DEFINE_CATEGORY(RPiDecompand)
> > > +
> > > +#define NAME "rpi.decompand"
> > > +
> > > +Decompand::Decompand(Controller *controller)
> > > +     : Algorithm(controller)
> > > +{
> > > +}
> > > +
> > > +char const *Decompand::name() const
> > > +{
> > > +     return NAME;
> > > +}
> > > +
> > > +int Decompand::read(const libcamera::YamlObject &params)
> > > +{
> > > +     config_.bitdepth = params["bitdepth"].get<uint32_t>(0);
> > > +     config_.decompandCurve =
> > params["decompand_curve"].get<ipa::Pwl>(ipa::Pwl{});
> > > +     return config_.decompandCurve.empty() ? -EINVAL : 0;
> > > +}
> > > +
> > > +void Decompand::initialise()
> > > +{
> > > +}
> > > +
> > > +void Decompand::switchMode(CameraMode const &cameraMode,
> > > +                        [[maybe_unused]] Metadata *metadata)
> > > +{
> > > +     mode_ = cameraMode;
> > > +}
> > > +
> > > +void Decompand::prepare(Metadata *imageMetadata)
> > > +{
> > > +     DecompandStatus decompandStatus;
> > > +
> > > +     if (config_.bitdepth == 0 || mode_.bitdepth == config_.bitdepth) {
> > > +             decompandStatus.decompandCurve = config_.decompandCurve;
> > > +             imageMetadata->set("decompand.status", decompandStatus);
> > > +     }
> > > +}
> > > +
> > > +/* Register algorithm with the system. */
> > > +static Algorithm *create(Controller *controller)
> > > +{
> > > +     return new Decompand(controller);
> > > +}
> > > +
> > > +static RegisterAlgorithm reg(NAME, &create);
> > > diff --git a/src/ipa/rpi/controller/rpi/decompand.h
> > b/src/ipa/rpi/controller/rpi/decompand.h
> > > new file mode 100644
> > > index 000000000000..38b26a21e6d5
> > > --- /dev/null
> > > +++ b/src/ipa/rpi/controller/rpi/decompand.h
> > > @@ -0,0 +1,31 @@
> > > +#pragma once
> > > +
> > > +#include <libipa/pwl.h>
> > > +
> > > +#include "../decompand_status.h"
> > > +
> > > +#include "algorithm.h"
> > > +
> > > +namespace RPiController {
> > > +
> > > +struct DecompandConfig {
> > > +     uint32_t bitdepth;
> > > +     libcamera::ipa::Pwl decompandCurve;
> > > +};
> > > +
> > > +class Decompand : public Algorithm
> > > +{
> > > +public:
> > > +     Decompand(Controller *controller = nullptr);
> > > +     char const *name() const override;
> > > +     int read(const libcamera::YamlObject &params) override;
> > > +     void initialise() override;
> > > +     void switchMode(CameraMode const &cameraMode, Metadata *metadata)
> > override;
> > > +     void prepare(Metadata *imageMetadata) override;
> > > +
> > > +private:
> > > +     CameraMode mode_;
> > > +     DecompandConfig config_;
> > > +};
> > > +
> > > +} /* namespace RPiController */
> > > diff --git a/src/ipa/rpi/pisp/pisp.cpp b/src/ipa/rpi/pisp/pisp.cpp
> > > index 829b91258522..14ece12b0895 100644
> > > --- a/src/ipa/rpi/pisp/pisp.cpp
> > > +++ b/src/ipa/rpi/pisp/pisp.cpp
> > > @@ -32,6 +32,7 @@
> > >  #include "controller/cac_status.h"
> > >  #include "controller/ccm_status.h"
> > >  #include "controller/contrast_status.h"
> > > +#include "controller/decompand_status.h"
> > >  #include "controller/denoise_algorithm.h"
> > >  #include "controller/denoise_status.h"
> > >  #include "controller/dpc_status.h"
> > > @@ -113,6 +114,25 @@ int generateLut(const ipa::Pwl &pwl, uint32_t *lut,
> > std::size_t lutSize,
> > >       return 0;
> > >  }
> > >
> > > +int generateDecompandLut(const ipa::Pwl &pwl, Span<uint16_t> lut)
> > > +{
> > > +     if (pwl.empty())
> > > +             return -EINVAL;
> > > +
> > > +     constexpr int step = 1024;
> > > +     for (std::size_t i = 0; i < lut.size(); ++i) {
> > > +             int x = i * step;
> > > +
> > > +             int y = pwl.eval(x);
> > > +             if (y < 0)
> > > +                     return -1;
> > > +
> > > +             lut[i] = static_cast<uint16_t>(std::min(y, 65535));
> > > +     }
> > > +
> > > +     return 0;
> > > +}
> > > +
> > >  void packLscLut(uint32_t packed[NumLscVertexes][NumLscVertexes],
> > >               double const rgb[3][NumLscVertexes][NumLscVertexes])
> > >  {
> > > @@ -236,6 +256,7 @@ private:
> > >       void applyLensShading(const AlscStatus *alscStatus,
> > >                             pisp_be_global_config &global);
> > >       void applyDPC(const DpcStatus *dpcStatus, pisp_be_global_config
> > &global);
> > > +     void applyDecompand(const DecompandStatus *decompandStatus);
> > >       void applySdn(const SdnStatus *sdnStatus, pisp_be_global_config
> > &global);
> > >       void applyTdn(const TdnStatus *tdnStatus, const DeviceStatus
> > *deviceStatus,
> > >                     pisp_be_global_config &global);
> > > @@ -351,6 +372,11 @@ void IpaPiSP::platformPrepareIsp([[maybe_unused]]
> > const PrepareParams &params,
> > >               if (noiseStatus)
> > >                       applyFocusStats(noiseStatus);
> > >
> > > +             DecompandStatus *decompandStatus =
> > > +
> >  rpiMetadata.getLocked<DecompandStatus>("decompand.status");
> > > +             if (decompandStatus)
> > > +                     applyDecompand(decompandStatus);
> > > +
> > >               BlackLevelStatus *blackLevelStatus =
> > >
> >  rpiMetadata.getLocked<BlackLevelStatus>("black_level.status");
> > >               if (blackLevelStatus)
> > > @@ -702,6 +728,18 @@ void IpaPiSP::applyDPC(const DpcStatus *dpcStatus,
> > pisp_be_global_config &global
> > >       be_->SetDpc(dpc);
> > >  }
> > >
> > > +void IpaPiSP::applyDecompand(const DecompandStatus *decompandStatus)
> > > +{
> > > +     pisp_fe_global_config feGlobal;
> > > +     pisp_fe_decompand_config decompand = {};
> > > +
> > > +     if (!generateDecompandLut(decompandStatus->decompandCurve,
> > decompand.lut)) {
> > > +             fe_->SetDecompand(decompand);
> > > +             feGlobal.enables |= PISP_FE_ENABLE_DECOMPAND;
> > > +             fe_->SetGlobal(feGlobal);
> > > +     }
> > > +}
> > > +
> > >  void IpaPiSP::applySdn(const SdnStatus *sdnStatus,
> > pisp_be_global_config &global)
> > >  {
> > >       pisp_be_sdn_config sdn = {};

Hi Laurent,

On Tue, 14 Oct 2025 at 09:00, Laurent Pinchart <
laurent.pinchart@ideasonboard.com> wrote:

> Hi Naush,
>
> On Tue, Oct 14, 2025 at 08:26:33AM +0100, Naushir Patuck wrote:
> > On Mon, 13 Oct 2025 at 15:50, Laurent Pinchart wrote:
> > > On Fri, Oct 03, 2025 at 01:15:52PM +0100, Naushir Patuck wrote:
> > > > From: Sena Asotani <aso.fam429@gmail.com>
> > > >
> > > > This patch integrates a new decompand algorithm that utilizes the
> PiSP
> > > > FE hardware block available on Raspberry Pi 5. The implementation
> > > > enables conversion of companded sensor data into linear format prior
> to
> > > > ISP processing.
> > > >
> > > > Changes include:
> > > > - Implementation of decompand logic for controller and pipe
> interfaces
> > > > - Enabling decompand block by "rpi.decompand" in tuning.json
> > >
> > > I've worked on something similar for i.MX8MP, with a different
> approach.
> > >
> > > The companding curve is an intrinsic property of the sensor. Depending
> > > on the sensor, it can be
> > >
> > > - hardcoded or configurable
> > > - when configurable, specified in different ways (e.g. piecewise linear
> > >   function, parametric formula or selectable from presets)
> > > - always enabled or controllable
> > >
> > > This creates a relatively large problem space. libcamera needs to know
> > > when a compression curve is applied by the sensor, and what curve is
> > > being applied. Additionally, it may require the ability to
> > > enable/disable compression, as well as the ability to configure the
> > > curve.
> > >
> > > Specifying the curve in a tuning file doesn't seem to be a good
> > > solution. I've opted to support hardcoded curves only to start with (in
> > > order to simplify the problem), and report the curve from the sensor
> > > helper. I've also added a V4L2 control ([1]) to enable and disable
> > > compression, with support in the AR0144 driver([2]).
> >
> > This work for implementing decompanding was done by Sena (cc'ed) for the
> > IMX585 which is not yet upstreamed.  I've not worked on any sensors
> myself
> > that use this feature, so I can't really tell if a hard-coded curve vs a
> > user programmable one is the best approach.
>
> I've seen both cases implemented in sensors, so we need to support
> hardcoded curves, and may want to support configurable curves.
>
> > Having said that, I would
> > agree that a V4L2 control to enable/disable companding (+ a libcamera
> > control of course) on the sensor is the right approach!
>
> On the camera side I expect it would be a SensorConfiguration parameter
> rather than a control.
>
> > > Do you plan to implement proper companding support for the RPi pipeline
> > > handler and IPA module ?
> >
> > Do you mean adding the relevant libcamera controls for use by an
> > application?  Apart from that, and having a user programmable curve, this
> > patch set does include everything else required for the IPA/PH to enable
> > the "decomand" feature in the frontend.  It seems to do the right thing
> for
> > the IMX585 as reported by Sena.
>
> I meant
>
> - Making companding dynamically selectable by the user
> - Optionally supporting user-specific curves
> - Moving the curve from tuning files to sensor helpers
>

I don't have any experience with using companding in sensors.  I don't even
have access to the IMX585 that this feature is developed for.  So a few
basic questions to help my understanding :)

- Should this be a user accessible control(s)?  Do you see a user needing
to enable/disable this?  Perhaps this should be a decision made only by the
pipeline handler.
- Should the curve (if adjustable) be perhaps defined in the sensor driver
and extracted to userland via the sensor helper?
- I'm struggling to see why a sensor device would allow adjustable
companding curves, is there a use-case you know about where this is needed?

Regards,
Naush


>
> > > [1]
> https://lore.kernel.org/linux-media/20240905225308.11267-9-laurent.pinchart@ideasonboard.com/
> > > [2]
> https://lore.kernel.org/linux-media/20240905225308.11267-10-laurent.pinchart@ideasonboard.com/
> > >
> > > > Signed-off-by: Sena Asotani <aso.fam429@gmail.com>
> > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> > > > Tested-by: Nick Hollinghurst <nick.hollinghurst@raspberrypi.com>
> > > > ---
> > > >  src/ipa/rpi/controller/decompand_status.h |  8 ++++
> > > >  src/ipa/rpi/controller/meson.build        |  1 +
> > > >  src/ipa/rpi/controller/rpi/decompand.cpp  | 58
> +++++++++++++++++++++++
> > > >  src/ipa/rpi/controller/rpi/decompand.h    | 31 ++++++++++++
> > > >  src/ipa/rpi/pisp/pisp.cpp                 | 38 +++++++++++++++
> > > >  5 files changed, 136 insertions(+)
> > > >  create mode 100644 src/ipa/rpi/controller/decompand_status.h
> > > >  create mode 100644 src/ipa/rpi/controller/rpi/decompand.cpp
> > > >  create mode 100644 src/ipa/rpi/controller/rpi/decompand.h
> > > >
> > > > diff --git a/src/ipa/rpi/controller/decompand_status.h
> > > b/src/ipa/rpi/controller/decompand_status.h
> > > > new file mode 100644
> > > > index 000000000000..2d9888dca4f3
> > > > --- /dev/null
> > > > +++ b/src/ipa/rpi/controller/decompand_status.h
> > > > @@ -0,0 +1,8 @@
> > > > +#pragma once
> > > > +
> > > > +#include "libipa/pwl.h"
> > > > +
> > > > +struct DecompandStatus {
> > > > +     uint32_t bitdepth;
> > > > +     libcamera::ipa::Pwl decompandCurve;
> > > > +};
> > > > diff --git a/src/ipa/rpi/controller/meson.build
> > > b/src/ipa/rpi/controller/meson.build
> > > > index 74b74888bbff..c13c48539d10 100644
> > > > --- a/src/ipa/rpi/controller/meson.build
> > > > +++ b/src/ipa/rpi/controller/meson.build
> > > > @@ -14,6 +14,7 @@ rpi_ipa_controller_sources = files([
> > > >      'rpi/cac.cpp',
> > > >      'rpi/ccm.cpp',
> > > >      'rpi/contrast.cpp',
> > > > +    'rpi/decompand.cpp',
> > > >      'rpi/denoise.cpp',
> > > >      'rpi/dpc.cpp',
> > > >      'rpi/geq.cpp',
> > > > diff --git a/src/ipa/rpi/controller/rpi/decompand.cpp
> > > b/src/ipa/rpi/controller/rpi/decompand.cpp
> > > > new file mode 100644
> > > > index 000000000000..2036750f82f4
> > > > --- /dev/null
> > > > +++ b/src/ipa/rpi/controller/rpi/decompand.cpp
> > > > @@ -0,0 +1,58 @@
> > > > +#include "decompand.h"
> > > > +
> > > > +#include <libcamera/base/log.h>
> > > > +
> > > > +#include "../decompand_status.h"
> > > > +#include "../histogram.h"
> > > > +
> > > > +using namespace RPiController;
> > > > +using namespace libcamera;
> > > > +
> > > > +LOG_DEFINE_CATEGORY(RPiDecompand)
> > > > +
> > > > +#define NAME "rpi.decompand"
> > > > +
> > > > +Decompand::Decompand(Controller *controller)
> > > > +     : Algorithm(controller)
> > > > +{
> > > > +}
> > > > +
> > > > +char const *Decompand::name() const
> > > > +{
> > > > +     return NAME;
> > > > +}
> > > > +
> > > > +int Decompand::read(const libcamera::YamlObject &params)
> > > > +{
> > > > +     config_.bitdepth = params["bitdepth"].get<uint32_t>(0);
> > > > +     config_.decompandCurve =
> > > params["decompand_curve"].get<ipa::Pwl>(ipa::Pwl{});
> > > > +     return config_.decompandCurve.empty() ? -EINVAL : 0;
> > > > +}
> > > > +
> > > > +void Decompand::initialise()
> > > > +{
> > > > +}
> > > > +
> > > > +void Decompand::switchMode(CameraMode const &cameraMode,
> > > > +                        [[maybe_unused]] Metadata *metadata)
> > > > +{
> > > > +     mode_ = cameraMode;
> > > > +}
> > > > +
> > > > +void Decompand::prepare(Metadata *imageMetadata)
> > > > +{
> > > > +     DecompandStatus decompandStatus;
> > > > +
> > > > +     if (config_.bitdepth == 0 || mode_.bitdepth ==
> config_.bitdepth) {
> > > > +             decompandStatus.decompandCurve =
> config_.decompandCurve;
> > > > +             imageMetadata->set("decompand.status",
> decompandStatus);
> > > > +     }
> > > > +}
> > > > +
> > > > +/* Register algorithm with the system. */
> > > > +static Algorithm *create(Controller *controller)
> > > > +{
> > > > +     return new Decompand(controller);
> > > > +}
> > > > +
> > > > +static RegisterAlgorithm reg(NAME, &create);
> > > > diff --git a/src/ipa/rpi/controller/rpi/decompand.h
> > > b/src/ipa/rpi/controller/rpi/decompand.h
> > > > new file mode 100644
> > > > index 000000000000..38b26a21e6d5
> > > > --- /dev/null
> > > > +++ b/src/ipa/rpi/controller/rpi/decompand.h
> > > > @@ -0,0 +1,31 @@
> > > > +#pragma once
> > > > +
> > > > +#include <libipa/pwl.h>
> > > > +
> > > > +#include "../decompand_status.h"
> > > > +
> > > > +#include "algorithm.h"
> > > > +
> > > > +namespace RPiController {
> > > > +
> > > > +struct DecompandConfig {
> > > > +     uint32_t bitdepth;
> > > > +     libcamera::ipa::Pwl decompandCurve;
> > > > +};
> > > > +
> > > > +class Decompand : public Algorithm
> > > > +{
> > > > +public:
> > > > +     Decompand(Controller *controller = nullptr);
> > > > +     char const *name() const override;
> > > > +     int read(const libcamera::YamlObject &params) override;
> > > > +     void initialise() override;
> > > > +     void switchMode(CameraMode const &cameraMode, Metadata
> *metadata)
> > > override;
> > > > +     void prepare(Metadata *imageMetadata) override;
> > > > +
> > > > +private:
> > > > +     CameraMode mode_;
> > > > +     DecompandConfig config_;
> > > > +};
> > > > +
> > > > +} /* namespace RPiController */
> > > > diff --git a/src/ipa/rpi/pisp/pisp.cpp b/src/ipa/rpi/pisp/pisp.cpp
> > > > index 829b91258522..14ece12b0895 100644
> > > > --- a/src/ipa/rpi/pisp/pisp.cpp
> > > > +++ b/src/ipa/rpi/pisp/pisp.cpp
> > > > @@ -32,6 +32,7 @@
> > > >  #include "controller/cac_status.h"
> > > >  #include "controller/ccm_status.h"
> > > >  #include "controller/contrast_status.h"
> > > > +#include "controller/decompand_status.h"
> > > >  #include "controller/denoise_algorithm.h"
> > > >  #include "controller/denoise_status.h"
> > > >  #include "controller/dpc_status.h"
> > > > @@ -113,6 +114,25 @@ int generateLut(const ipa::Pwl &pwl, uint32_t
> *lut,
> > > std::size_t lutSize,
> > > >       return 0;
> > > >  }
> > > >
> > > > +int generateDecompandLut(const ipa::Pwl &pwl, Span<uint16_t> lut)
> > > > +{
> > > > +     if (pwl.empty())
> > > > +             return -EINVAL;
> > > > +
> > > > +     constexpr int step = 1024;
> > > > +     for (std::size_t i = 0; i < lut.size(); ++i) {
> > > > +             int x = i * step;
> > > > +
> > > > +             int y = pwl.eval(x);
> > > > +             if (y < 0)
> > > > +                     return -1;
> > > > +
> > > > +             lut[i] = static_cast<uint16_t>(std::min(y, 65535));
> > > > +     }
> > > > +
> > > > +     return 0;
> > > > +}
> > > > +
> > > >  void packLscLut(uint32_t packed[NumLscVertexes][NumLscVertexes],
> > > >               double const rgb[3][NumLscVertexes][NumLscVertexes])
> > > >  {
> > > > @@ -236,6 +256,7 @@ private:
> > > >       void applyLensShading(const AlscStatus *alscStatus,
> > > >                             pisp_be_global_config &global);
> > > >       void applyDPC(const DpcStatus *dpcStatus, pisp_be_global_config
> > > &global);
> > > > +     void applyDecompand(const DecompandStatus *decompandStatus);
> > > >       void applySdn(const SdnStatus *sdnStatus, pisp_be_global_config
> > > &global);
> > > >       void applyTdn(const TdnStatus *tdnStatus, const DeviceStatus
> > > *deviceStatus,
> > > >                     pisp_be_global_config &global);
> > > > @@ -351,6 +372,11 @@ void
> IpaPiSP::platformPrepareIsp([[maybe_unused]]
> > > const PrepareParams &params,
> > > >               if (noiseStatus)
> > > >                       applyFocusStats(noiseStatus);
> > > >
> > > > +             DecompandStatus *decompandStatus =
> > > > +
> > >  rpiMetadata.getLocked<DecompandStatus>("decompand.status");
> > > > +             if (decompandStatus)
> > > > +                     applyDecompand(decompandStatus);
> > > > +
> > > >               BlackLevelStatus *blackLevelStatus =
> > > >
> > >  rpiMetadata.getLocked<BlackLevelStatus>("black_level.status");
> > > >               if (blackLevelStatus)
> > > > @@ -702,6 +728,18 @@ void IpaPiSP::applyDPC(const DpcStatus
> *dpcStatus,
> > > pisp_be_global_config &global
> > > >       be_->SetDpc(dpc);
> > > >  }
> > > >
> > > > +void IpaPiSP::applyDecompand(const DecompandStatus *decompandStatus)
> > > > +{
> > > > +     pisp_fe_global_config feGlobal;
> > > > +     pisp_fe_decompand_config decompand = {};
> > > > +
> > > > +     if (!generateDecompandLut(decompandStatus->decompandCurve,
> > > decompand.lut)) {
> > > > +             fe_->SetDecompand(decompand);
> > > > +             feGlobal.enables |= PISP_FE_ENABLE_DECOMPAND;
> > > > +             fe_->SetGlobal(feGlobal);
> > > > +     }
> > > > +}
> > > > +
> > > >  void IpaPiSP::applySdn(const SdnStatus *sdnStatus,
> > > pisp_be_global_config &global)
> > > >  {
> > > >       pisp_be_sdn_config sdn = {};
>
> --
> Regards,
>
> Laurent Pinchart
>