[v3,14/19] pipeline: rkisp1: Query kernel for available params blocks
diff mbox series

Message ID 20250815102945.1602071-15-stefan.klug@ideasonboard.com
State New
Headers show
Series
  • Implement WDR algorithm
Related show

Commit Message

Stefan Klug Aug. 15, 2025, 10:29 a.m. UTC
Query the params device for RKISP1_CID_SUPPORTED_PARAMS_BLOCKS and
inject the information into the IPA hardware context for use by the
algorithms.

Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

---

Changes in v3:
- Removed unnecessary log statement
- Collected tags
- Added case for getControls() failing internally
---
 include/libcamera/ipa/rkisp1.mojom       |  2 +-
 src/ipa/rkisp1/ipa_context.h             |  1 +
 src/ipa/rkisp1/rkisp1.cpp                | 20 +++++++++++-----
 src/libcamera/pipeline/rkisp1/rkisp1.cpp | 29 ++++++++++++++++++++----
 4 files changed, 41 insertions(+), 11 deletions(-)

Comments

Dan Scally Aug. 15, 2025, 10:44 a.m. UTC | #1
Hi Stefan

On 15/08/2025 11:29, Stefan Klug wrote:
> Query the params device for RKISP1_CID_SUPPORTED_PARAMS_BLOCKS and
> inject the information into the IPA hardware context for use by the
> algorithms.
> 
> Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>
> ---
> 
> Changes in v3:
> - Removed unnecessary log statement
> - Collected tags
> - Added case for getControls() failing internally
> ---
>   include/libcamera/ipa/rkisp1.mojom       |  2 +-
>   src/ipa/rkisp1/ipa_context.h             |  1 +
>   src/ipa/rkisp1/rkisp1.cpp                | 20 +++++++++++-----
>   src/libcamera/pipeline/rkisp1/rkisp1.cpp | 29 ++++++++++++++++++++----
>   4 files changed, 41 insertions(+), 11 deletions(-)
> 
> diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom
> index 043ad27ea199..068e898848c4 100644
> --- a/include/libcamera/ipa/rkisp1.mojom
> +++ b/include/libcamera/ipa/rkisp1.mojom
> @@ -16,7 +16,7 @@ struct IPAConfigInfo {
>   
>   interface IPARkISP1Interface {
>   	init(libcamera.IPASettings settings,
> -	     uint32 hwRevision,
> +	     uint32 hwRevision, uint32 supportedBlocks,
>   	     libcamera.IPACameraSensorInfo sensorInfo,
>   	     libcamera.ControlInfoMap sensorControls)
>   		=> (int32 ret, libcamera.ControlInfoMap ipaControls);
> diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
> index 362ab2fda5fe..60cfab228edf 100644
> --- a/src/ipa/rkisp1/ipa_context.h
> +++ b/src/ipa/rkisp1/ipa_context.h
> @@ -36,6 +36,7 @@ struct IPAHwSettings {
>   	unsigned int numHistogramBins;
>   	unsigned int numHistogramWeights;
>   	unsigned int numGammaOutSamples;
> +	uint32_t supportedBlocks;
>   	bool compand;
>   };
>   
> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> index cf66d5553dcd..9ba0f0e6eb9d 100644
> --- a/src/ipa/rkisp1/rkisp1.cpp
> +++ b/src/ipa/rkisp1/rkisp1.cpp
> @@ -52,6 +52,7 @@ public:
>   	IPARkISP1();
>   
>   	int init(const IPASettings &settings, unsigned int hwRevision,
> +		 uint32_t supportedBlocks,
>   		 const IPACameraSensorInfo &sensorInfo,
>   		 const ControlInfoMap &sensorControls,
>   		 ControlInfoMap *ipaControls) override;
> @@ -89,27 +90,30 @@ private:
>   
>   namespace {
>   
> -const IPAHwSettings ipaHwSettingsV10{
> +IPAHwSettings ipaHwSettingsV10{
>   	RKISP1_CIF_ISP_AE_MEAN_MAX_V10,
>   	RKISP1_CIF_ISP_HIST_BIN_N_MAX_V10,
>   	RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE_V10,
>   	RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES_V10,
> +	0,
>   	false,
>   };
>   
> -const IPAHwSettings ipaHwSettingsIMX8MP{
> +IPAHwSettings ipaHwSettingsIMX8MP{
>   	RKISP1_CIF_ISP_AE_MEAN_MAX_V10,
>   	RKISP1_CIF_ISP_HIST_BIN_N_MAX_V10,
>   	RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE_V10,
>   	RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES_V10,
> +	0,
>   	true,
>   };
>   
> -const IPAHwSettings ipaHwSettingsV12{
> +IPAHwSettings ipaHwSettingsV12{
>   	RKISP1_CIF_ISP_AE_MEAN_MAX_V12,
>   	RKISP1_CIF_ISP_HIST_BIN_N_MAX_V12,
>   	RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE_V12,
>   	RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES_V12,
> +	0,
>   	false,
>   };
>   
> @@ -132,20 +136,22 @@ std::string IPARkISP1::logPrefix() const
>   }
>   
>   int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision,
> +		    uint32_t supportedBlocks,
>   		    const IPACameraSensorInfo &sensorInfo,
>   		    const ControlInfoMap &sensorControls,
>   		    ControlInfoMap *ipaControls)
>   {
> +	IPAHwSettings *hw = nullptr;
>   	/* \todo Add support for other revisions */
>   	switch (hwRevision) {
>   	case RKISP1_V10:
> -		context_.hw = &ipaHwSettingsV10;
> +		hw = &ipaHwSettingsV10;
>   		break;
>   	case RKISP1_V_IMX8MP:
> -		context_.hw = &ipaHwSettingsIMX8MP;
> +		hw = &ipaHwSettingsIMX8MP;
>   		break;
>   	case RKISP1_V12:
> -		context_.hw = &ipaHwSettingsV12;
> +		hw = &ipaHwSettingsV12;
>   		break;
>   	default:
>   		LOG(IPARkISP1, Error)
> @@ -153,6 +159,8 @@ int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision,
>   			<< " is currently not supported";
>   		return -ENODEV;
>   	}
> +	hw->supportedBlocks = supportedBlocks;
> +	context_.hw = hw;
>   
>   	LOG(IPARkISP1, Debug) << "Hardware revision is " << hwRevision;
>   
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index c4d4fdc6aa73..199f53d9e631 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -100,7 +100,7 @@ public:
>   
>   	PipelineHandlerRkISP1 *pipe();
>   	const PipelineHandlerRkISP1 *pipe() const;
> -	int loadIPA(unsigned int hwRevision);
> +	int loadIPA(unsigned int hwRevision, uint32_t supportedBlocks);
>   
>   	Stream mainPathStream_;
>   	Stream selfPathStream_;
> @@ -383,7 +383,7 @@ const PipelineHandlerRkISP1 *RkISP1CameraData::pipe() const
>   	return static_cast<const PipelineHandlerRkISP1 *>(Camera::Private::pipe());
>   }
>   
> -int RkISP1CameraData::loadIPA(unsigned int hwRevision)
> +int RkISP1CameraData::loadIPA(unsigned int hwRevision, uint32_t supportedBlocks)
>   {
>   	ipa_ = IPAManager::createIPA<ipa::rkisp1::IPAProxyRkISP1>(pipe(), 1, 1);
>   	if (!ipa_)
> @@ -405,7 +405,8 @@ int RkISP1CameraData::loadIPA(unsigned int hwRevision)
>   	}
>   
>   	ret = ipa_->init({ ipaTuningFile, sensor_->model() }, hwRevision,
> -			 sensorInfo, sensor_->controls(), &ipaControls_);
> +			 supportedBlocks, sensorInfo, sensor_->controls(),
> +			 &ipaControls_);
>   	if (ret < 0) {
>   		LOG(RkISP1, Error) << "IPA initialization failure";
>   		return ret;
> @@ -1311,6 +1312,12 @@ int PipelineHandlerRkISP1::updateControls(RkISP1CameraData *data)
>   	return 0;
>   }
>   
> +/*
> + * By default we assume all the blocks that were included in the first
> + * extensible parameters series are available. That is the lower 20bits.
> + */
> +const uint32_t kDefaultExtParamsBlocks = 0xfffff;
> +
>   int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)
>   {
>   	int ret;
> @@ -1348,7 +1355,21 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)
>   	isp_->frameStart.connect(data->delayedCtrls_.get(),
>   				 &DelayedControls::applyControls);
>   
> -	ret = data->loadIPA(media_->hwRevision());
> +	uint32_t supportedBlocks = kDefaultExtParamsBlocks;
> +
> +	auto &controls = param_->controls();
> +	if (controls.find(RKISP1_CID_SUPPORTED_PARAMS_BLOCKS) != controls.end()) {
> +		auto list = param_->getControls({ { RKISP1_CID_SUPPORTED_PARAMS_BLOCKS } });
> +		if (!list.empty())
> +			supportedBlocks = static_cast<uint32_t>(
> +				list.get(RKISP1_CID_SUPPORTED_PARAMS_BLOCKS)
> +					.get<int32_t>());
> +	} else {
> +		LOG(RkISP1, Error)
> +			<< "Failed to query supported params blocks. Falling back to defaults.";
> +	}
> +
> +	ret = data->loadIPA(media_->hwRevision(), supportedBlocks);
>   	if (ret)
>   		return ret;
>
Barnabás Pőcze Aug. 15, 2025, 2:52 p.m. UTC | #2
Hi

2025. 08. 15. 12:29 keltezéssel, Stefan Klug írta:
> Query the params device for RKISP1_CID_SUPPORTED_PARAMS_BLOCKS and
> inject the information into the IPA hardware context for use by the
> algorithms.
> 
> Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> ---
> 
> Changes in v3:
> - Removed unnecessary log statement
> - Collected tags
> - Added case for getControls() failing internally
> ---
>   include/libcamera/ipa/rkisp1.mojom       |  2 +-
>   src/ipa/rkisp1/ipa_context.h             |  1 +
>   src/ipa/rkisp1/rkisp1.cpp                | 20 +++++++++++-----
>   src/libcamera/pipeline/rkisp1/rkisp1.cpp | 29 ++++++++++++++++++++----
>   4 files changed, 41 insertions(+), 11 deletions(-)
> 
> diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom
> index 043ad27ea199..068e898848c4 100644
> --- a/include/libcamera/ipa/rkisp1.mojom
> +++ b/include/libcamera/ipa/rkisp1.mojom
> @@ -16,7 +16,7 @@ struct IPAConfigInfo {
>   
>   interface IPARkISP1Interface {
>   	init(libcamera.IPASettings settings,
> -	     uint32 hwRevision,
> +	     uint32 hwRevision, uint32 supportedBlocks,
>   	     libcamera.IPACameraSensorInfo sensorInfo,
>   	     libcamera.ControlInfoMap sensorControls)
>   		=> (int32 ret, libcamera.ControlInfoMap ipaControls);
> diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
> index 362ab2fda5fe..60cfab228edf 100644
> --- a/src/ipa/rkisp1/ipa_context.h
> +++ b/src/ipa/rkisp1/ipa_context.h
> @@ -36,6 +36,7 @@ struct IPAHwSettings {
>   	unsigned int numHistogramBins;
>   	unsigned int numHistogramWeights;
>   	unsigned int numGammaOutSamples;
> +	uint32_t supportedBlocks;
>   	bool compand;
>   };
>   
> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> index cf66d5553dcd..9ba0f0e6eb9d 100644
> --- a/src/ipa/rkisp1/rkisp1.cpp
> +++ b/src/ipa/rkisp1/rkisp1.cpp
> @@ -52,6 +52,7 @@ public:
>   	IPARkISP1();
>   
>   	int init(const IPASettings &settings, unsigned int hwRevision,
> +		 uint32_t supportedBlocks,
>   		 const IPACameraSensorInfo &sensorInfo,
>   		 const ControlInfoMap &sensorControls,
>   		 ControlInfoMap *ipaControls) override;
> @@ -89,27 +90,30 @@ private:
>   
>   namespace {
>   
> -const IPAHwSettings ipaHwSettingsV10{
> +IPAHwSettings ipaHwSettingsV10{
>   	RKISP1_CIF_ISP_AE_MEAN_MAX_V10,
>   	RKISP1_CIF_ISP_HIST_BIN_N_MAX_V10,
>   	RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE_V10,
>   	RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES_V10,
> +	0,
>   	false,
>   };
>   
> -const IPAHwSettings ipaHwSettingsIMX8MP{
> +IPAHwSettings ipaHwSettingsIMX8MP{
>   	RKISP1_CIF_ISP_AE_MEAN_MAX_V10,
>   	RKISP1_CIF_ISP_HIST_BIN_N_MAX_V10,
>   	RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE_V10,
>   	RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES_V10,
> +	0,
>   	true,
>   };
>   
> -const IPAHwSettings ipaHwSettingsV12{
> +IPAHwSettings ipaHwSettingsV12{
>   	RKISP1_CIF_ISP_AE_MEAN_MAX_V12,
>   	RKISP1_CIF_ISP_HIST_BIN_N_MAX_V12,
>   	RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE_V12,
>   	RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES_V12,
> +	0,
>   	false,
>   };
>   
> @@ -132,20 +136,22 @@ std::string IPARkISP1::logPrefix() const
>   }
>   
>   int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision,
> +		    uint32_t supportedBlocks,
>   		    const IPACameraSensorInfo &sensorInfo,
>   		    const ControlInfoMap &sensorControls,
>   		    ControlInfoMap *ipaControls)
>   {
> +	IPAHwSettings *hw = nullptr;
>   	/* \todo Add support for other revisions */
>   	switch (hwRevision) {
>   	case RKISP1_V10:
> -		context_.hw = &ipaHwSettingsV10;
> +		hw = &ipaHwSettingsV10;
>   		break;
>   	case RKISP1_V_IMX8MP:
> -		context_.hw = &ipaHwSettingsIMX8MP;
> +		hw = &ipaHwSettingsIMX8MP;
>   		break;
>   	case RKISP1_V12:
> -		context_.hw = &ipaHwSettingsV12;
> +		hw = &ipaHwSettingsV12;
>   		break;
>   	default:
>   		LOG(IPARkISP1, Error)
> @@ -153,6 +159,8 @@ int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision,
>   			<< " is currently not supported";
>   		return -ENODEV;
>   	}
> +	hw->supportedBlocks = supportedBlocks;

This seems to modify the global data. Is that going to be fine in the long run?


Regards,
Barnabás Pőcze


> +	context_.hw = hw;
>   
>   	LOG(IPARkISP1, Debug) << "Hardware revision is " << hwRevision;
>   
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index c4d4fdc6aa73..199f53d9e631 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -100,7 +100,7 @@ public:
>   
>   	PipelineHandlerRkISP1 *pipe();
>   	const PipelineHandlerRkISP1 *pipe() const;
> -	int loadIPA(unsigned int hwRevision);
> +	int loadIPA(unsigned int hwRevision, uint32_t supportedBlocks);
>   
>   	Stream mainPathStream_;
>   	Stream selfPathStream_;
> @@ -383,7 +383,7 @@ const PipelineHandlerRkISP1 *RkISP1CameraData::pipe() const
>   	return static_cast<const PipelineHandlerRkISP1 *>(Camera::Private::pipe());
>   }
>   
> -int RkISP1CameraData::loadIPA(unsigned int hwRevision)
> +int RkISP1CameraData::loadIPA(unsigned int hwRevision, uint32_t supportedBlocks)
>   {
>   	ipa_ = IPAManager::createIPA<ipa::rkisp1::IPAProxyRkISP1>(pipe(), 1, 1);
>   	if (!ipa_)
> @@ -405,7 +405,8 @@ int RkISP1CameraData::loadIPA(unsigned int hwRevision)
>   	}
>   
>   	ret = ipa_->init({ ipaTuningFile, sensor_->model() }, hwRevision,
> -			 sensorInfo, sensor_->controls(), &ipaControls_);
> +			 supportedBlocks, sensorInfo, sensor_->controls(),
> +			 &ipaControls_);
>   	if (ret < 0) {
>   		LOG(RkISP1, Error) << "IPA initialization failure";
>   		return ret;
> @@ -1311,6 +1312,12 @@ int PipelineHandlerRkISP1::updateControls(RkISP1CameraData *data)
>   	return 0;
>   }
>   
> +/*
> + * By default we assume all the blocks that were included in the first
> + * extensible parameters series are available. That is the lower 20bits.
> + */
> +const uint32_t kDefaultExtParamsBlocks = 0xfffff;
> +
>   int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)
>   {
>   	int ret;
> @@ -1348,7 +1355,21 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)
>   	isp_->frameStart.connect(data->delayedCtrls_.get(),
>   				 &DelayedControls::applyControls);
>   
> -	ret = data->loadIPA(media_->hwRevision());
> +	uint32_t supportedBlocks = kDefaultExtParamsBlocks;
> +
> +	auto &controls = param_->controls();
> +	if (controls.find(RKISP1_CID_SUPPORTED_PARAMS_BLOCKS) != controls.end()) {
> +		auto list = param_->getControls({ { RKISP1_CID_SUPPORTED_PARAMS_BLOCKS } });
> +		if (!list.empty())
> +			supportedBlocks = static_cast<uint32_t>(
> +				list.get(RKISP1_CID_SUPPORTED_PARAMS_BLOCKS)
> +					.get<int32_t>());
> +	} else {
> +		LOG(RkISP1, Error)
> +			<< "Failed to query supported params blocks. Falling back to defaults.";
> +	}
> +
> +	ret = data->loadIPA(media_->hwRevision(), supportedBlocks);
>   	if (ret)
>   		return ret;
>
Stefan Klug Aug. 18, 2025, 8:17 a.m. UTC | #3
Hi Barnabás,

Thanks for the comment.

Quoting Barnabás Pőcze (2025-08-15 16:52:53)
> Hi
> 
> 2025. 08. 15. 12:29 keltezéssel, Stefan Klug írta:
> > Query the params device for RKISP1_CID_SUPPORTED_PARAMS_BLOCKS and
> > inject the information into the IPA hardware context for use by the
> > algorithms.
> > 
> > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > 
> > ---
> > 
> > Changes in v3:
> > - Removed unnecessary log statement
> > - Collected tags
> > - Added case for getControls() failing internally
> > ---
> >   include/libcamera/ipa/rkisp1.mojom       |  2 +-
> >   src/ipa/rkisp1/ipa_context.h             |  1 +
> >   src/ipa/rkisp1/rkisp1.cpp                | 20 +++++++++++-----
> >   src/libcamera/pipeline/rkisp1/rkisp1.cpp | 29 ++++++++++++++++++++----
> >   4 files changed, 41 insertions(+), 11 deletions(-)
> > 
> > diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom
> > index 043ad27ea199..068e898848c4 100644
> > --- a/include/libcamera/ipa/rkisp1.mojom
> > +++ b/include/libcamera/ipa/rkisp1.mojom
> > @@ -16,7 +16,7 @@ struct IPAConfigInfo {
> >   
> >   interface IPARkISP1Interface {
> >       init(libcamera.IPASettings settings,
> > -          uint32 hwRevision,
> > +          uint32 hwRevision, uint32 supportedBlocks,
> >            libcamera.IPACameraSensorInfo sensorInfo,
> >            libcamera.ControlInfoMap sensorControls)
> >               => (int32 ret, libcamera.ControlInfoMap ipaControls);
> > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
> > index 362ab2fda5fe..60cfab228edf 100644
> > --- a/src/ipa/rkisp1/ipa_context.h
> > +++ b/src/ipa/rkisp1/ipa_context.h
> > @@ -36,6 +36,7 @@ struct IPAHwSettings {
> >       unsigned int numHistogramBins;
> >       unsigned int numHistogramWeights;
> >       unsigned int numGammaOutSamples;
> > +     uint32_t supportedBlocks;
> >       bool compand;
> >   };
> >   
> > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> > index cf66d5553dcd..9ba0f0e6eb9d 100644
> > --- a/src/ipa/rkisp1/rkisp1.cpp
> > +++ b/src/ipa/rkisp1/rkisp1.cpp
> > @@ -52,6 +52,7 @@ public:
> >       IPARkISP1();
> >   
> >       int init(const IPASettings &settings, unsigned int hwRevision,
> > +              uint32_t supportedBlocks,
> >                const IPACameraSensorInfo &sensorInfo,
> >                const ControlInfoMap &sensorControls,
> >                ControlInfoMap *ipaControls) override;
> > @@ -89,27 +90,30 @@ private:
> >   
> >   namespace {
> >   
> > -const IPAHwSettings ipaHwSettingsV10{
> > +IPAHwSettings ipaHwSettingsV10{
> >       RKISP1_CIF_ISP_AE_MEAN_MAX_V10,
> >       RKISP1_CIF_ISP_HIST_BIN_N_MAX_V10,
> >       RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE_V10,
> >       RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES_V10,
> > +     0,
> >       false,
> >   };
> >   
> > -const IPAHwSettings ipaHwSettingsIMX8MP{
> > +IPAHwSettings ipaHwSettingsIMX8MP{
> >       RKISP1_CIF_ISP_AE_MEAN_MAX_V10,
> >       RKISP1_CIF_ISP_HIST_BIN_N_MAX_V10,
> >       RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE_V10,
> >       RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES_V10,
> > +     0,
> >       true,
> >   };
> >   
> > -const IPAHwSettings ipaHwSettingsV12{
> > +IPAHwSettings ipaHwSettingsV12{
> >       RKISP1_CIF_ISP_AE_MEAN_MAX_V12,
> >       RKISP1_CIF_ISP_HIST_BIN_N_MAX_V12,
> >       RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE_V12,
> >       RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES_V12,
> > +     0,
> >       false,
> >   };
> >   
> > @@ -132,20 +136,22 @@ std::string IPARkISP1::logPrefix() const
> >   }
> >   
> >   int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision,
> > +                 uint32_t supportedBlocks,
> >                   const IPACameraSensorInfo &sensorInfo,
> >                   const ControlInfoMap &sensorControls,
> >                   ControlInfoMap *ipaControls)
> >   {
> > +     IPAHwSettings *hw = nullptr;
> >       /* \todo Add support for other revisions */
> >       switch (hwRevision) {
> >       case RKISP1_V10:
> > -             context_.hw = &ipaHwSettingsV10;
> > +             hw = &ipaHwSettingsV10;
> >               break;
> >       case RKISP1_V_IMX8MP:
> > -             context_.hw = &ipaHwSettingsIMX8MP;
> > +             hw = &ipaHwSettingsIMX8MP;
> >               break;
> >       case RKISP1_V12:
> > -             context_.hw = &ipaHwSettingsV12;
> > +             hw = &ipaHwSettingsV12;
> >               break;
> >       default:
> >               LOG(IPARkISP1, Error)
> > @@ -153,6 +159,8 @@ int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision,
> >                       << " is currently not supported";
> >               return -ENODEV;
> >       }
> > +     hw->supportedBlocks = supportedBlocks;
> 
> This seems to modify the global data. Is that going to be fine in the long run?

That depends on how many hardware variation we will see :-). But you are
right, this is not the typical way to solve it. I tried to do a minimal
invasive solution were the algorithms don't need any modification.

I now tried it the other way around and I think it is better. THe
relevant diff is then:

diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
index 618b6d9049e2..c333c095632f 100644
--- a/src/ipa/rkisp1/algorithms/agc.cpp
+++ b/src/ipa/rkisp1/algorithms/agc.cpp
@@ -56,7 +56,7 @@ int Agc::parseMeteringModes(IPAContext &context, const YamlObject &tuningData)

 		std::vector<uint8_t> weights =
 			value.getList<uint8_t>().value_or(std::vector<uint8_t>{});
-		if (weights.size() != context.hw->numHistogramWeights) {
+		if (weights.size() != context.hw.numHistogramWeights) {
 			LOG(RkISP1Agc, Warning)
 				<< "Failed to read metering mode'" << key << "'";
 			continue;
@@ -68,7 +68,7 @@ int Agc::parseMeteringModes(IPAContext &context, const YamlObject &tuningData)
 	if (meteringModes_.empty()) {
 		LOG(RkISP1Agc, Warning)
 			<< "No metering modes read from tuning file; defaulting to matrix";
-		std::vector<uint8_t> weights(context.hw->numHistogramWeights, 1);
+		std::vector<uint8_t> weights(context.hw.numHistogramWeights, 1);

 		meteringModes_[controls::MeteringMatrix] = weights;
 	}
@@ -417,7 +417,7 @@ void Agc::prepare(IPAContext &context, const uint32_t frame,

 	Span<uint8_t> weights{
 		hstConfig->hist_weight,
-		context.hw->numHistogramWeights
+		context.hw.numHistogramWeights
 	};
 	std::vector<uint8_t> &modeWeights = meteringModes_.at(frameContext.agc.meteringMode);
 	std::copy(modeWeights.begin(), modeWeights.end(), weights.begin());
@@ -555,9 +555,9 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
 	const rkisp1_cif_isp_stat *params = &stats->params;

 	/* The lower 4 bits are fractional and meant to be discarded. */
-	Histogram hist({ params->hist.hist_bins, context.hw->numHistogramBins },
+	Histogram hist({ params->hist.hist_bins, context.hw.numHistogramBins },
 		       [](uint32_t x) { return x >> 4; });
-	expMeans_ = { params->ae.exp_mean, context.hw->numAeCells };
+	expMeans_ = { params->ae.exp_mean, context.hw.numAeCells };
 	std::vector<uint8_t> &modeWeights = meteringModes_.at(frameContext.agc.meteringMode);
 	weights_ = { modeWeights.data(), modeWeights.size() };

diff --git a/src/ipa/rkisp1/algorithms/blc.cpp b/src/ipa/rkisp1/algorithms/blc.cpp
index 98cb7145e164..32fc44ffff92 100644
--- a/src/ipa/rkisp1/algorithms/blc.cpp
+++ b/src/ipa/rkisp1/algorithms/blc.cpp
@@ -114,7 +114,7 @@ int BlackLevelCorrection::configure(IPAContext &context,
 	 * of the extensible parameters format.
 	 */
 	supported_ = context.configuration.paramFormat == V4L2_META_FMT_RK_ISP1_EXT_PARAMS ||
-		     !context.hw->compand;
+		     !context.hw.compand;

 	if (!supported_)
 		LOG(RkISP1Blc, Warning)
@@ -140,7 +140,7 @@ void BlackLevelCorrection::prepare(IPAContext &context,
 	if (!supported_)
 		return;

-	if (context.hw->compand) {
+	if (context.hw.compand) {
 		auto config = params->block<BlockType::CompandBls>();
 		config.setEnabled(true);

diff --git a/src/ipa/rkisp1/algorithms/compress.cpp b/src/ipa/rkisp1/algorithms/compress.cpp
index e076727de4f1..84ecef577e9c 100644
--- a/src/ipa/rkisp1/algorithms/compress.cpp
+++ b/src/ipa/rkisp1/algorithms/compress.cpp
@@ -53,7 +53,7 @@ int Compress::configure(IPAContext &context,
 			[[maybe_unused]] const IPACameraSensorInfo &configInfo)
 {
 	if (context.configuration.paramFormat != V4L2_META_FMT_RK_ISP1_EXT_PARAMS ||
-	    !context.hw->compand) {
+	    !context.hw.compand) {
 		LOG(RkISP1Compress, Warning)
 			<< "Compression is not supported by the hardware or kernel.";
 		return 0;
diff --git a/src/ipa/rkisp1/algorithms/goc.cpp b/src/ipa/rkisp1/algorithms/goc.cpp
index a9493678dba7..a0e7030fe5db 100644
--- a/src/ipa/rkisp1/algorithms/goc.cpp
+++ b/src/ipa/rkisp1/algorithms/goc.cpp
@@ -50,7 +50,7 @@ const float kDefaultGamma = 2.2f;
  */
 int GammaOutCorrection::init(IPAContext &context, const YamlObject &tuningData)
 {
-	if (context.hw->numGammaOutSamples !=
+	if (context.hw.numGammaOutSamples !=
 	    RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES_V10) {
 		LOG(RkISP1Gamma, Error)
 			<< "Gamma is not implemented for RkISP1 V12";
@@ -101,7 +101,7 @@ void GammaOutCorrection::prepare(IPAContext &context,
 				 IPAFrameContext &frameContext,
 				 RkISP1Params *params)
 {
-	ASSERT(context.hw->numGammaOutSamples ==
+	ASSERT(context.hw.numGammaOutSamples ==
 	       RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES_V10);

 	if (!frameContext.goc.update)
diff --git a/src/ipa/rkisp1/algorithms/lux.cpp b/src/ipa/rkisp1/algorithms/lux.cpp
index dd05f18d5e94..e8da69810008 100644
--- a/src/ipa/rkisp1/algorithms/lux.cpp
+++ b/src/ipa/rkisp1/algorithms/lux.cpp
@@ -61,7 +61,7 @@ void Lux::process(IPAContext &context,

 	/* \todo Deduplicate the histogram calculation from AGC */
 	const rkisp1_cif_isp_stat *params = &stats->params;
-	Histogram yHist({ params->hist.hist_bins, context.hw->numHistogramBins },
+	Histogram yHist({ params->hist.hist_bins, context.hw.numHistogramBins },
 			[](uint32_t x) { return x >> 4; });

 	double lux = lux_.estimateLux(exposureTime, gain, 1.0, yHist);
diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
index 362ab2fda5fe..57decbd3e2e6 100644
--- a/src/ipa/rkisp1/ipa_context.h
+++ b/src/ipa/rkisp1/ipa_context.h
@@ -36,6 +36,7 @@ struct IPAHwSettings {
 	unsigned int numHistogramBins;
 	unsigned int numHistogramWeights;
 	unsigned int numGammaOutSamples;
+	uint32_t supportedBlocks;
 	bool compand;
 };

@@ -201,11 +202,11 @@ struct IPAFrameContext : public FrameContext {

 struct IPAContext {
 	IPAContext(unsigned int frameContextSize)
-		: hw(nullptr), frameContexts(frameContextSize)
+		: frameContexts(frameContextSize)
 	{
 	}

-	const IPAHwSettings *hw;
+	IPAHwSettings hw;
 	IPACameraSensorInfo sensorInfo;
 	IPASessionConfiguration configuration;
 	IPAActiveState activeState;
diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
index cf66d5553dcd..fa22bfc34904 100644
--- a/src/ipa/rkisp1/rkisp1.cpp
+++ b/src/ipa/rkisp1/rkisp1.cpp
@@ -52,6 +52,7 @@ public:
 	IPARkISP1();

 	int init(const IPASettings &settings, unsigned int hwRevision,
+		 uint32_t supportedBlocks,
 		 const IPACameraSensorInfo &sensorInfo,
 		 const ControlInfoMap &sensorControls,
 		 ControlInfoMap *ipaControls) override;
@@ -94,6 +95,7 @@ const IPAHwSettings ipaHwSettingsV10{
 	RKISP1_CIF_ISP_HIST_BIN_N_MAX_V10,
 	RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE_V10,
 	RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES_V10,
+	0,
 	false,
 };

@@ -102,6 +104,7 @@ const IPAHwSettings ipaHwSettingsIMX8MP{
 	RKISP1_CIF_ISP_HIST_BIN_N_MAX_V10,
 	RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE_V10,
 	RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES_V10,
+	0,
 	true,
 };

@@ -110,6 +113,7 @@ const IPAHwSettings ipaHwSettingsV12{
 	RKISP1_CIF_ISP_HIST_BIN_N_MAX_V12,
 	RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE_V12,
 	RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES_V12,
+	0,
 	false,
 };

@@ -132,6 +136,7 @@ std::string IPARkISP1::logPrefix() const
 }

 int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision,
+		    uint32_t supportedBlocks,
 		    const IPACameraSensorInfo &sensorInfo,
 		    const ControlInfoMap &sensorControls,
 		    ControlInfoMap *ipaControls)
@@ -139,13 +144,13 @@ int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision,
 	/* \todo Add support for other revisions */
 	switch (hwRevision) {
 	case RKISP1_V10:
-		context_.hw = &ipaHwSettingsV10;
+		context_.hw = ipaHwSettingsV10;
 		break;
 	case RKISP1_V_IMX8MP:
-		context_.hw = &ipaHwSettingsIMX8MP;
+		context_.hw = ipaHwSettingsIMX8MP;
 		break;
 	case RKISP1_V12:
-		context_.hw = &ipaHwSettingsV12;
+		context_.hw = ipaHwSettingsV12;
 		break;
 	default:
 		LOG(IPARkISP1, Error)
@@ -153,6 +158,7 @@ int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision,
 			<< " is currently not supported";
 		return -ENODEV;
 	}
+	context_.hw.supportedBlocks = supportedBlocks;

 	LOG(IPARkISP1, Debug) << "Hardware revision is " << hwRevision;


Which version do you like better?

Best regards,
Stefan


> 
> 
> Regards,
> Barnabás Pőcze
> 
> 
> > +     context_.hw = hw;
> >   
> >       LOG(IPARkISP1, Debug) << "Hardware revision is " << hwRevision;
> >   
> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > index c4d4fdc6aa73..199f53d9e631 100644
> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > @@ -100,7 +100,7 @@ public:
> >   
> >       PipelineHandlerRkISP1 *pipe();
> >       const PipelineHandlerRkISP1 *pipe() const;
> > -     int loadIPA(unsigned int hwRevision);
> > +     int loadIPA(unsigned int hwRevision, uint32_t supportedBlocks);
> >   
> >       Stream mainPathStream_;
> >       Stream selfPathStream_;
> > @@ -383,7 +383,7 @@ const PipelineHandlerRkISP1 *RkISP1CameraData::pipe() const
> >       return static_cast<const PipelineHandlerRkISP1 *>(Camera::Private::pipe());
> >   }
> >   
> > -int RkISP1CameraData::loadIPA(unsigned int hwRevision)
> > +int RkISP1CameraData::loadIPA(unsigned int hwRevision, uint32_t supportedBlocks)
> >   {
> >       ipa_ = IPAManager::createIPA<ipa::rkisp1::IPAProxyRkISP1>(pipe(), 1, 1);
> >       if (!ipa_)
> > @@ -405,7 +405,8 @@ int RkISP1CameraData::loadIPA(unsigned int hwRevision)
> >       }
> >   
> >       ret = ipa_->init({ ipaTuningFile, sensor_->model() }, hwRevision,
> > -                      sensorInfo, sensor_->controls(), &ipaControls_);
> > +                      supportedBlocks, sensorInfo, sensor_->controls(),
> > +                      &ipaControls_);
> >       if (ret < 0) {
> >               LOG(RkISP1, Error) << "IPA initialization failure";
> >               return ret;
> > @@ -1311,6 +1312,12 @@ int PipelineHandlerRkISP1::updateControls(RkISP1CameraData *data)
> >       return 0;
> >   }
> >   
> > +/*
> > + * By default we assume all the blocks that were included in the first
> > + * extensible parameters series are available. That is the lower 20bits.
> > + */
> > +const uint32_t kDefaultExtParamsBlocks = 0xfffff;
> > +
> >   int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)
> >   {
> >       int ret;
> > @@ -1348,7 +1355,21 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)
> >       isp_->frameStart.connect(data->delayedCtrls_.get(),
> >                                &DelayedControls::applyControls);
> >   
> > -     ret = data->loadIPA(media_->hwRevision());
> > +     uint32_t supportedBlocks = kDefaultExtParamsBlocks;
> > +
> > +     auto &controls = param_->controls();
> > +     if (controls.find(RKISP1_CID_SUPPORTED_PARAMS_BLOCKS) != controls.end()) {
> > +             auto list = param_->getControls({ { RKISP1_CID_SUPPORTED_PARAMS_BLOCKS } });
> > +             if (!list.empty())
> > +                     supportedBlocks = static_cast<uint32_t>(
> > +                             list.get(RKISP1_CID_SUPPORTED_PARAMS_BLOCKS)
> > +                                     .get<int32_t>());
> > +     } else {
> > +             LOG(RkISP1, Error)
> > +                     << "Failed to query supported params blocks. Falling back to defaults.";
> > +     }
> > +
> > +     ret = data->loadIPA(media_->hwRevision(), supportedBlocks);
> >       if (ret)
> >               return ret;
> >   
>
Kieran Bingham Aug. 18, 2025, 9:30 a.m. UTC | #4
Quoting Stefan Klug (2025-08-18 09:17:53)
> Hi Barnabás,
> 
> Thanks for the comment.
> 
> Quoting Barnabás Pőcze (2025-08-15 16:52:53)
> > Hi
> > 
> > 2025. 08. 15. 12:29 keltezéssel, Stefan Klug írta:
> > > Query the params device for RKISP1_CID_SUPPORTED_PARAMS_BLOCKS and
> > > inject the information into the IPA hardware context for use by the
> > > algorithms.
> > > 
> > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > > 
> > > ---
> > > 
> > > Changes in v3:
> > > - Removed unnecessary log statement
> > > - Collected tags
> > > - Added case for getControls() failing internally
> > > ---
> > >   include/libcamera/ipa/rkisp1.mojom       |  2 +-
> > >   src/ipa/rkisp1/ipa_context.h             |  1 +
> > >   src/ipa/rkisp1/rkisp1.cpp                | 20 +++++++++++-----
> > >   src/libcamera/pipeline/rkisp1/rkisp1.cpp | 29 ++++++++++++++++++++----
> > >   4 files changed, 41 insertions(+), 11 deletions(-)
> > > 
> > > diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom
> > > index 043ad27ea199..068e898848c4 100644
> > > --- a/include/libcamera/ipa/rkisp1.mojom
> > > +++ b/include/libcamera/ipa/rkisp1.mojom
> > > @@ -16,7 +16,7 @@ struct IPAConfigInfo {
> > >   
> > >   interface IPARkISP1Interface {
> > >       init(libcamera.IPASettings settings,
> > > -          uint32 hwRevision,
> > > +          uint32 hwRevision, uint32 supportedBlocks,
> > >            libcamera.IPACameraSensorInfo sensorInfo,
> > >            libcamera.ControlInfoMap sensorControls)
> > >               => (int32 ret, libcamera.ControlInfoMap ipaControls);
> > > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
> > > index 362ab2fda5fe..60cfab228edf 100644
> > > --- a/src/ipa/rkisp1/ipa_context.h
> > > +++ b/src/ipa/rkisp1/ipa_context.h
> > > @@ -36,6 +36,7 @@ struct IPAHwSettings {
> > >       unsigned int numHistogramBins;
> > >       unsigned int numHistogramWeights;
> > >       unsigned int numGammaOutSamples;
> > > +     uint32_t supportedBlocks;
> > >       bool compand;
> > >   };
> > >   
> > > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> > > index cf66d5553dcd..9ba0f0e6eb9d 100644
> > > --- a/src/ipa/rkisp1/rkisp1.cpp
> > > +++ b/src/ipa/rkisp1/rkisp1.cpp
> > > @@ -52,6 +52,7 @@ public:
> > >       IPARkISP1();
> > >   
> > >       int init(const IPASettings &settings, unsigned int hwRevision,
> > > +              uint32_t supportedBlocks,
> > >                const IPACameraSensorInfo &sensorInfo,
> > >                const ControlInfoMap &sensorControls,
> > >                ControlInfoMap *ipaControls) override;
> > > @@ -89,27 +90,30 @@ private:
> > >   
> > >   namespace {
> > >   
> > > -const IPAHwSettings ipaHwSettingsV10{
> > > +IPAHwSettings ipaHwSettingsV10{
> > >       RKISP1_CIF_ISP_AE_MEAN_MAX_V10,
> > >       RKISP1_CIF_ISP_HIST_BIN_N_MAX_V10,
> > >       RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE_V10,
> > >       RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES_V10,
> > > +     0,
> > >       false,
> > >   };
> > >   
> > > -const IPAHwSettings ipaHwSettingsIMX8MP{
> > > +IPAHwSettings ipaHwSettingsIMX8MP{
> > >       RKISP1_CIF_ISP_AE_MEAN_MAX_V10,
> > >       RKISP1_CIF_ISP_HIST_BIN_N_MAX_V10,
> > >       RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE_V10,
> > >       RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES_V10,
> > > +     0,
> > >       true,
> > >   };
> > >   
> > > -const IPAHwSettings ipaHwSettingsV12{
> > > +IPAHwSettings ipaHwSettingsV12{
> > >       RKISP1_CIF_ISP_AE_MEAN_MAX_V12,
> > >       RKISP1_CIF_ISP_HIST_BIN_N_MAX_V12,
> > >       RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE_V12,
> > >       RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES_V12,
> > > +     0,
> > >       false,
> > >   };
> > >   
> > > @@ -132,20 +136,22 @@ std::string IPARkISP1::logPrefix() const
> > >   }
> > >   
> > >   int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision,
> > > +                 uint32_t supportedBlocks,
> > >                   const IPACameraSensorInfo &sensorInfo,
> > >                   const ControlInfoMap &sensorControls,
> > >                   ControlInfoMap *ipaControls)
> > >   {
> > > +     IPAHwSettings *hw = nullptr;
> > >       /* \todo Add support for other revisions */
> > >       switch (hwRevision) {
> > >       case RKISP1_V10:
> > > -             context_.hw = &ipaHwSettingsV10;
> > > +             hw = &ipaHwSettingsV10;
> > >               break;
> > >       case RKISP1_V_IMX8MP:
> > > -             context_.hw = &ipaHwSettingsIMX8MP;
> > > +             hw = &ipaHwSettingsIMX8MP;
> > >               break;
> > >       case RKISP1_V12:
> > > -             context_.hw = &ipaHwSettingsV12;
> > > +             hw = &ipaHwSettingsV12;
> > >               break;
> > >       default:
> > >               LOG(IPARkISP1, Error)
> > > @@ -153,6 +159,8 @@ int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision,
> > >                       << " is currently not supported";
> > >               return -ENODEV;
> > >       }
> > > +     hw->supportedBlocks = supportedBlocks;
> > 
> > This seems to modify the global data. Is that going to be fine in the long run?
> 
> That depends on how many hardware variation we will see :-). But you are
> right, this is not the typical way to solve it. I tried to do a minimal
> invasive solution were the algorithms don't need any modification.
> 
> I now tried it the other way around and I think it is better. THe
> relevant diff is then:
> 
> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
> index 618b6d9049e2..c333c095632f 100644
> --- a/src/ipa/rkisp1/algorithms/agc.cpp
> +++ b/src/ipa/rkisp1/algorithms/agc.cpp
> @@ -56,7 +56,7 @@ int Agc::parseMeteringModes(IPAContext &context, const YamlObject &tuningData)
> 
>                 std::vector<uint8_t> weights =
>                         value.getList<uint8_t>().value_or(std::vector<uint8_t>{});
> -               if (weights.size() != context.hw->numHistogramWeights) {
> +               if (weights.size() != context.hw.numHistogramWeights) {
>                         LOG(RkISP1Agc, Warning)
>                                 << "Failed to read metering mode'" << key << "'";
>                         continue;
> @@ -68,7 +68,7 @@ int Agc::parseMeteringModes(IPAContext &context, const YamlObject &tuningData)
>         if (meteringModes_.empty()) {
>                 LOG(RkISP1Agc, Warning)
>                         << "No metering modes read from tuning file; defaulting to matrix";
> -               std::vector<uint8_t> weights(context.hw->numHistogramWeights, 1);
> +               std::vector<uint8_t> weights(context.hw.numHistogramWeights, 1);
> 
>                 meteringModes_[controls::MeteringMatrix] = weights;
>         }
> @@ -417,7 +417,7 @@ void Agc::prepare(IPAContext &context, const uint32_t frame,
> 
>         Span<uint8_t> weights{
>                 hstConfig->hist_weight,
> -               context.hw->numHistogramWeights
> +               context.hw.numHistogramWeights
>         };
>         std::vector<uint8_t> &modeWeights = meteringModes_.at(frameContext.agc.meteringMode);
>         std::copy(modeWeights.begin(), modeWeights.end(), weights.begin());
> @@ -555,9 +555,9 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
>         const rkisp1_cif_isp_stat *params = &stats->params;
> 
>         /* The lower 4 bits are fractional and meant to be discarded. */
> -       Histogram hist({ params->hist.hist_bins, context.hw->numHistogramBins },
> +       Histogram hist({ params->hist.hist_bins, context.hw.numHistogramBins },
>                        [](uint32_t x) { return x >> 4; });
> -       expMeans_ = { params->ae.exp_mean, context.hw->numAeCells };
> +       expMeans_ = { params->ae.exp_mean, context.hw.numAeCells };
>         std::vector<uint8_t> &modeWeights = meteringModes_.at(frameContext.agc.meteringMode);
>         weights_ = { modeWeights.data(), modeWeights.size() };
> 
> diff --git a/src/ipa/rkisp1/algorithms/blc.cpp b/src/ipa/rkisp1/algorithms/blc.cpp
> index 98cb7145e164..32fc44ffff92 100644
> --- a/src/ipa/rkisp1/algorithms/blc.cpp
> +++ b/src/ipa/rkisp1/algorithms/blc.cpp
> @@ -114,7 +114,7 @@ int BlackLevelCorrection::configure(IPAContext &context,
>          * of the extensible parameters format.
>          */
>         supported_ = context.configuration.paramFormat == V4L2_META_FMT_RK_ISP1_EXT_PARAMS ||
> -                    !context.hw->compand;
> +                    !context.hw.compand;
> 
>         if (!supported_)
>                 LOG(RkISP1Blc, Warning)
> @@ -140,7 +140,7 @@ void BlackLevelCorrection::prepare(IPAContext &context,
>         if (!supported_)
>                 return;
> 
> -       if (context.hw->compand) {
> +       if (context.hw.compand) {
>                 auto config = params->block<BlockType::CompandBls>();
>                 config.setEnabled(true);
> 
> diff --git a/src/ipa/rkisp1/algorithms/compress.cpp b/src/ipa/rkisp1/algorithms/compress.cpp
> index e076727de4f1..84ecef577e9c 100644
> --- a/src/ipa/rkisp1/algorithms/compress.cpp
> +++ b/src/ipa/rkisp1/algorithms/compress.cpp
> @@ -53,7 +53,7 @@ int Compress::configure(IPAContext &context,
>                         [[maybe_unused]] const IPACameraSensorInfo &configInfo)
>  {
>         if (context.configuration.paramFormat != V4L2_META_FMT_RK_ISP1_EXT_PARAMS ||
> -           !context.hw->compand) {
> +           !context.hw.compand) {
>                 LOG(RkISP1Compress, Warning)
>                         << "Compression is not supported by the hardware or kernel.";
>                 return 0;
> diff --git a/src/ipa/rkisp1/algorithms/goc.cpp b/src/ipa/rkisp1/algorithms/goc.cpp
> index a9493678dba7..a0e7030fe5db 100644
> --- a/src/ipa/rkisp1/algorithms/goc.cpp
> +++ b/src/ipa/rkisp1/algorithms/goc.cpp
> @@ -50,7 +50,7 @@ const float kDefaultGamma = 2.2f;
>   */
>  int GammaOutCorrection::init(IPAContext &context, const YamlObject &tuningData)
>  {
> -       if (context.hw->numGammaOutSamples !=
> +       if (context.hw.numGammaOutSamples !=
>             RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES_V10) {
>                 LOG(RkISP1Gamma, Error)
>                         << "Gamma is not implemented for RkISP1 V12";
> @@ -101,7 +101,7 @@ void GammaOutCorrection::prepare(IPAContext &context,
>                                  IPAFrameContext &frameContext,
>                                  RkISP1Params *params)
>  {
> -       ASSERT(context.hw->numGammaOutSamples ==
> +       ASSERT(context.hw.numGammaOutSamples ==
>                RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES_V10);
> 
>         if (!frameContext.goc.update)
> diff --git a/src/ipa/rkisp1/algorithms/lux.cpp b/src/ipa/rkisp1/algorithms/lux.cpp
> index dd05f18d5e94..e8da69810008 100644
> --- a/src/ipa/rkisp1/algorithms/lux.cpp
> +++ b/src/ipa/rkisp1/algorithms/lux.cpp
> @@ -61,7 +61,7 @@ void Lux::process(IPAContext &context,
> 
>         /* \todo Deduplicate the histogram calculation from AGC */
>         const rkisp1_cif_isp_stat *params = &stats->params;
> -       Histogram yHist({ params->hist.hist_bins, context.hw->numHistogramBins },
> +       Histogram yHist({ params->hist.hist_bins, context.hw.numHistogramBins },
>                         [](uint32_t x) { return x >> 4; });
> 
>         double lux = lux_.estimateLux(exposureTime, gain, 1.0, yHist);
> diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
> index 362ab2fda5fe..57decbd3e2e6 100644
> --- a/src/ipa/rkisp1/ipa_context.h
> +++ b/src/ipa/rkisp1/ipa_context.h
> @@ -36,6 +36,7 @@ struct IPAHwSettings {
>         unsigned int numHistogramBins;
>         unsigned int numHistogramWeights;
>         unsigned int numGammaOutSamples;
> +       uint32_t supportedBlocks;
>         bool compand;
>  };
> 
> @@ -201,11 +202,11 @@ struct IPAFrameContext : public FrameContext {
> 
>  struct IPAContext {
>         IPAContext(unsigned int frameContextSize)
> -               : hw(nullptr), frameContexts(frameContextSize)
> +               : frameContexts(frameContextSize)
>         {
>         }
> 
> -       const IPAHwSettings *hw;
> +       IPAHwSettings hw;
>         IPACameraSensorInfo sensorInfo;
>         IPASessionConfiguration configuration;
>         IPAActiveState activeState;
> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> index cf66d5553dcd..fa22bfc34904 100644
> --- a/src/ipa/rkisp1/rkisp1.cpp
> +++ b/src/ipa/rkisp1/rkisp1.cpp
> @@ -52,6 +52,7 @@ public:
>         IPARkISP1();
> 
>         int init(const IPASettings &settings, unsigned int hwRevision,
> +                uint32_t supportedBlocks,
>                  const IPACameraSensorInfo &sensorInfo,
>                  const ControlInfoMap &sensorControls,
>                  ControlInfoMap *ipaControls) override;
> @@ -94,6 +95,7 @@ const IPAHwSettings ipaHwSettingsV10{
>         RKISP1_CIF_ISP_HIST_BIN_N_MAX_V10,
>         RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE_V10,
>         RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES_V10,
> +       0,
>         false,
>  };
> 
> @@ -102,6 +104,7 @@ const IPAHwSettings ipaHwSettingsIMX8MP{
>         RKISP1_CIF_ISP_HIST_BIN_N_MAX_V10,
>         RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE_V10,
>         RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES_V10,
> +       0,
>         true,
>  };
> 
> @@ -110,6 +113,7 @@ const IPAHwSettings ipaHwSettingsV12{
>         RKISP1_CIF_ISP_HIST_BIN_N_MAX_V12,
>         RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE_V12,
>         RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES_V12,
> +       0,
>         false,
>  };
> 
> @@ -132,6 +136,7 @@ std::string IPARkISP1::logPrefix() const
>  }
> 
>  int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision,
> +                   uint32_t supportedBlocks,
>                     const IPACameraSensorInfo &sensorInfo,
>                     const ControlInfoMap &sensorControls,
>                     ControlInfoMap *ipaControls)
> @@ -139,13 +144,13 @@ int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision,
>         /* \todo Add support for other revisions */
>         switch (hwRevision) {
>         case RKISP1_V10:
> -               context_.hw = &ipaHwSettingsV10;
> +               context_.hw = ipaHwSettingsV10;
>                 break;
>         case RKISP1_V_IMX8MP:
> -               context_.hw = &ipaHwSettingsIMX8MP;
> +               context_.hw = ipaHwSettingsIMX8MP;
>                 break;
>         case RKISP1_V12:
> -               context_.hw = &ipaHwSettingsV12;
> +               context_.hw = ipaHwSettingsV12;
>                 break;
>         default:
>                 LOG(IPARkISP1, Error)
> @@ -153,6 +158,7 @@ int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision,
>                         << " is currently not supported";
>                 return -ENODEV;
>         }
> +       context_.hw.supportedBlocks = supportedBlocks;
> 
>         LOG(IPARkISP1, Debug) << "Hardware revision is " << hwRevision;
> 
> 
> Which version do you like better?

I think I like this new version where the context_.hw is initialised
from the static data - but doesn't modify the global.

Remembering to put the static initialisation data back to const of
course ;-)

--
Kieran

> 
> Best regards,
> Stefan
> 
> 
> > 
> > 
> > Regards,
> > Barnabás Pőcze
> > 
> > 
> > > +     context_.hw = hw;
> > >   
> > >       LOG(IPARkISP1, Debug) << "Hardware revision is " << hwRevision;
> > >   
> > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > index c4d4fdc6aa73..199f53d9e631 100644
> > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > @@ -100,7 +100,7 @@ public:
> > >   
> > >       PipelineHandlerRkISP1 *pipe();
> > >       const PipelineHandlerRkISP1 *pipe() const;
> > > -     int loadIPA(unsigned int hwRevision);
> > > +     int loadIPA(unsigned int hwRevision, uint32_t supportedBlocks);
> > >   
> > >       Stream mainPathStream_;
> > >       Stream selfPathStream_;
> > > @@ -383,7 +383,7 @@ const PipelineHandlerRkISP1 *RkISP1CameraData::pipe() const
> > >       return static_cast<const PipelineHandlerRkISP1 *>(Camera::Private::pipe());
> > >   }
> > >   
> > > -int RkISP1CameraData::loadIPA(unsigned int hwRevision)
> > > +int RkISP1CameraData::loadIPA(unsigned int hwRevision, uint32_t supportedBlocks)
> > >   {
> > >       ipa_ = IPAManager::createIPA<ipa::rkisp1::IPAProxyRkISP1>(pipe(), 1, 1);
> > >       if (!ipa_)
> > > @@ -405,7 +405,8 @@ int RkISP1CameraData::loadIPA(unsigned int hwRevision)
> > >       }
> > >   
> > >       ret = ipa_->init({ ipaTuningFile, sensor_->model() }, hwRevision,
> > > -                      sensorInfo, sensor_->controls(), &ipaControls_);
> > > +                      supportedBlocks, sensorInfo, sensor_->controls(),
> > > +                      &ipaControls_);
> > >       if (ret < 0) {
> > >               LOG(RkISP1, Error) << "IPA initialization failure";
> > >               return ret;
> > > @@ -1311,6 +1312,12 @@ int PipelineHandlerRkISP1::updateControls(RkISP1CameraData *data)
> > >       return 0;
> > >   }
> > >   
> > > +/*
> > > + * By default we assume all the blocks that were included in the first
> > > + * extensible parameters series are available. That is the lower 20bits.
> > > + */
> > > +const uint32_t kDefaultExtParamsBlocks = 0xfffff;
> > > +
> > >   int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)
> > >   {
> > >       int ret;
> > > @@ -1348,7 +1355,21 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)
> > >       isp_->frameStart.connect(data->delayedCtrls_.get(),
> > >                                &DelayedControls::applyControls);
> > >   
> > > -     ret = data->loadIPA(media_->hwRevision());
> > > +     uint32_t supportedBlocks = kDefaultExtParamsBlocks;
> > > +
> > > +     auto &controls = param_->controls();
> > > +     if (controls.find(RKISP1_CID_SUPPORTED_PARAMS_BLOCKS) != controls.end()) {
> > > +             auto list = param_->getControls({ { RKISP1_CID_SUPPORTED_PARAMS_BLOCKS } });
> > > +             if (!list.empty())
> > > +                     supportedBlocks = static_cast<uint32_t>(
> > > +                             list.get(RKISP1_CID_SUPPORTED_PARAMS_BLOCKS)
> > > +                                     .get<int32_t>());
> > > +     } else {
> > > +             LOG(RkISP1, Error)
> > > +                     << "Failed to query supported params blocks. Falling back to defaults.";
> > > +     }
> > > +
> > > +     ret = data->loadIPA(media_->hwRevision(), supportedBlocks);
> > >       if (ret)
> > >               return ret;
> > >   
> >
Paul Elder Sept. 5, 2025, 10:42 a.m. UTC | #5
Quoting Stefan Klug (2025-08-18 17:17:53)
> Hi Barnabás,
> 
> Thanks for the comment.
> 
> Quoting Barnabás Pőcze (2025-08-15 16:52:53)
> > Hi
> > 
> > 2025. 08. 15. 12:29 keltezéssel, Stefan Klug írta:
> > > Query the params device for RKISP1_CID_SUPPORTED_PARAMS_BLOCKS and
> > > inject the information into the IPA hardware context for use by the
> > > algorithms.
> > > 
> > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > > 
> > > ---
> > > 
> > > Changes in v3:
> > > - Removed unnecessary log statement
> > > - Collected tags
> > > - Added case for getControls() failing internally
> > > ---
> > >   include/libcamera/ipa/rkisp1.mojom       |  2 +-
> > >   src/ipa/rkisp1/ipa_context.h             |  1 +
> > >   src/ipa/rkisp1/rkisp1.cpp                | 20 +++++++++++-----
> > >   src/libcamera/pipeline/rkisp1/rkisp1.cpp | 29 ++++++++++++++++++++----
> > >   4 files changed, 41 insertions(+), 11 deletions(-)
> > > 
> > > diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom
> > > index 043ad27ea199..068e898848c4 100644
> > > --- a/include/libcamera/ipa/rkisp1.mojom
> > > +++ b/include/libcamera/ipa/rkisp1.mojom
> > > @@ -16,7 +16,7 @@ struct IPAConfigInfo {
> > >   
> > >   interface IPARkISP1Interface {
> > >       init(libcamera.IPASettings settings,
> > > -          uint32 hwRevision,
> > > +          uint32 hwRevision, uint32 supportedBlocks,
> > >            libcamera.IPACameraSensorInfo sensorInfo,
> > >            libcamera.ControlInfoMap sensorControls)
> > >               => (int32 ret, libcamera.ControlInfoMap ipaControls);
> > > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
> > > index 362ab2fda5fe..60cfab228edf 100644
> > > --- a/src/ipa/rkisp1/ipa_context.h
> > > +++ b/src/ipa/rkisp1/ipa_context.h
> > > @@ -36,6 +36,7 @@ struct IPAHwSettings {
> > >       unsigned int numHistogramBins;
> > >       unsigned int numHistogramWeights;
> > >       unsigned int numGammaOutSamples;
> > > +     uint32_t supportedBlocks;
> > >       bool compand;
> > >   };
> > >   
> > > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> > > index cf66d5553dcd..9ba0f0e6eb9d 100644
> > > --- a/src/ipa/rkisp1/rkisp1.cpp
> > > +++ b/src/ipa/rkisp1/rkisp1.cpp
> > > @@ -52,6 +52,7 @@ public:
> > >       IPARkISP1();
> > >   
> > >       int init(const IPASettings &settings, unsigned int hwRevision,
> > > +              uint32_t supportedBlocks,
> > >                const IPACameraSensorInfo &sensorInfo,
> > >                const ControlInfoMap &sensorControls,
> > >                ControlInfoMap *ipaControls) override;
> > > @@ -89,27 +90,30 @@ private:
> > >   
> > >   namespace {
> > >   
> > > -const IPAHwSettings ipaHwSettingsV10{
> > > +IPAHwSettings ipaHwSettingsV10{
> > >       RKISP1_CIF_ISP_AE_MEAN_MAX_V10,
> > >       RKISP1_CIF_ISP_HIST_BIN_N_MAX_V10,
> > >       RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE_V10,
> > >       RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES_V10,
> > > +     0,
> > >       false,
> > >   };
> > >   
> > > -const IPAHwSettings ipaHwSettingsIMX8MP{
> > > +IPAHwSettings ipaHwSettingsIMX8MP{
> > >       RKISP1_CIF_ISP_AE_MEAN_MAX_V10,
> > >       RKISP1_CIF_ISP_HIST_BIN_N_MAX_V10,
> > >       RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE_V10,
> > >       RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES_V10,
> > > +     0,
> > >       true,
> > >   };
> > >   
> > > -const IPAHwSettings ipaHwSettingsV12{
> > > +IPAHwSettings ipaHwSettingsV12{
> > >       RKISP1_CIF_ISP_AE_MEAN_MAX_V12,
> > >       RKISP1_CIF_ISP_HIST_BIN_N_MAX_V12,
> > >       RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE_V12,
> > >       RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES_V12,
> > > +     0,
> > >       false,
> > >   };
> > >   
> > > @@ -132,20 +136,22 @@ std::string IPARkISP1::logPrefix() const
> > >   }
> > >   
> > >   int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision,
> > > +                 uint32_t supportedBlocks,
> > >                   const IPACameraSensorInfo &sensorInfo,
> > >                   const ControlInfoMap &sensorControls,
> > >                   ControlInfoMap *ipaControls)
> > >   {
> > > +     IPAHwSettings *hw = nullptr;
> > >       /* \todo Add support for other revisions */
> > >       switch (hwRevision) {
> > >       case RKISP1_V10:
> > > -             context_.hw = &ipaHwSettingsV10;
> > > +             hw = &ipaHwSettingsV10;
> > >               break;
> > >       case RKISP1_V_IMX8MP:
> > > -             context_.hw = &ipaHwSettingsIMX8MP;
> > > +             hw = &ipaHwSettingsIMX8MP;
> > >               break;
> > >       case RKISP1_V12:
> > > -             context_.hw = &ipaHwSettingsV12;
> > > +             hw = &ipaHwSettingsV12;
> > >               break;
> > >       default:
> > >               LOG(IPARkISP1, Error)
> > > @@ -153,6 +159,8 @@ int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision,
> > >                       << " is currently not supported";
> > >               return -ENODEV;
> > >       }
> > > +     hw->supportedBlocks = supportedBlocks;
> > 
> > This seems to modify the global data. Is that going to be fine in the long run?
> 
> That depends on how many hardware variation we will see :-). But you are
> right, this is not the typical way to solve it. I tried to do a minimal
> invasive solution were the algorithms don't need any modification.
> 
> I now tried it the other way around and I think it is better. THe
> relevant diff is then:
> 
> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
> index 618b6d9049e2..c333c095632f 100644
> --- a/src/ipa/rkisp1/algorithms/agc.cpp
> +++ b/src/ipa/rkisp1/algorithms/agc.cpp
> @@ -56,7 +56,7 @@ int Agc::parseMeteringModes(IPAContext &context, const YamlObject &tuningData)
> 
>                 std::vector<uint8_t> weights =
>                         value.getList<uint8_t>().value_or(std::vector<uint8_t>{});
> -               if (weights.size() != context.hw->numHistogramWeights) {
> +               if (weights.size() != context.hw.numHistogramWeights) {
>                         LOG(RkISP1Agc, Warning)
>                                 << "Failed to read metering mode'" << key << "'";
>                         continue;
> @@ -68,7 +68,7 @@ int Agc::parseMeteringModes(IPAContext &context, const YamlObject &tuningData)
>         if (meteringModes_.empty()) {
>                 LOG(RkISP1Agc, Warning)
>                         << "No metering modes read from tuning file; defaulting to matrix";
> -               std::vector<uint8_t> weights(context.hw->numHistogramWeights, 1);
> +               std::vector<uint8_t> weights(context.hw.numHistogramWeights, 1);
> 
>                 meteringModes_[controls::MeteringMatrix] = weights;
>         }
> @@ -417,7 +417,7 @@ void Agc::prepare(IPAContext &context, const uint32_t frame,
> 
>         Span<uint8_t> weights{
>                 hstConfig->hist_weight,
> -               context.hw->numHistogramWeights
> +               context.hw.numHistogramWeights
>         };
>         std::vector<uint8_t> &modeWeights = meteringModes_.at(frameContext.agc.meteringMode);
>         std::copy(modeWeights.begin(), modeWeights.end(), weights.begin());
> @@ -555,9 +555,9 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
>         const rkisp1_cif_isp_stat *params = &stats->params;
> 
>         /* The lower 4 bits are fractional and meant to be discarded. */
> -       Histogram hist({ params->hist.hist_bins, context.hw->numHistogramBins },
> +       Histogram hist({ params->hist.hist_bins, context.hw.numHistogramBins },
>                        [](uint32_t x) { return x >> 4; });
> -       expMeans_ = { params->ae.exp_mean, context.hw->numAeCells };
> +       expMeans_ = { params->ae.exp_mean, context.hw.numAeCells };
>         std::vector<uint8_t> &modeWeights = meteringModes_.at(frameContext.agc.meteringMode);
>         weights_ = { modeWeights.data(), modeWeights.size() };
> 
> diff --git a/src/ipa/rkisp1/algorithms/blc.cpp b/src/ipa/rkisp1/algorithms/blc.cpp
> index 98cb7145e164..32fc44ffff92 100644
> --- a/src/ipa/rkisp1/algorithms/blc.cpp
> +++ b/src/ipa/rkisp1/algorithms/blc.cpp
> @@ -114,7 +114,7 @@ int BlackLevelCorrection::configure(IPAContext &context,
>          * of the extensible parameters format.
>          */
>         supported_ = context.configuration.paramFormat == V4L2_META_FMT_RK_ISP1_EXT_PARAMS ||
> -                    !context.hw->compand;
> +                    !context.hw.compand;
> 
>         if (!supported_)
>                 LOG(RkISP1Blc, Warning)
> @@ -140,7 +140,7 @@ void BlackLevelCorrection::prepare(IPAContext &context,
>         if (!supported_)
>                 return;
> 
> -       if (context.hw->compand) {
> +       if (context.hw.compand) {
>                 auto config = params->block<BlockType::CompandBls>();
>                 config.setEnabled(true);
> 
> diff --git a/src/ipa/rkisp1/algorithms/compress.cpp b/src/ipa/rkisp1/algorithms/compress.cpp
> index e076727de4f1..84ecef577e9c 100644
> --- a/src/ipa/rkisp1/algorithms/compress.cpp
> +++ b/src/ipa/rkisp1/algorithms/compress.cpp
> @@ -53,7 +53,7 @@ int Compress::configure(IPAContext &context,
>                         [[maybe_unused]] const IPACameraSensorInfo &configInfo)
>  {
>         if (context.configuration.paramFormat != V4L2_META_FMT_RK_ISP1_EXT_PARAMS ||
> -           !context.hw->compand) {
> +           !context.hw.compand) {
>                 LOG(RkISP1Compress, Warning)
>                         << "Compression is not supported by the hardware or kernel.";
>                 return 0;
> diff --git a/src/ipa/rkisp1/algorithms/goc.cpp b/src/ipa/rkisp1/algorithms/goc.cpp
> index a9493678dba7..a0e7030fe5db 100644
> --- a/src/ipa/rkisp1/algorithms/goc.cpp
> +++ b/src/ipa/rkisp1/algorithms/goc.cpp
> @@ -50,7 +50,7 @@ const float kDefaultGamma = 2.2f;
>   */
>  int GammaOutCorrection::init(IPAContext &context, const YamlObject &tuningData)
>  {
> -       if (context.hw->numGammaOutSamples !=
> +       if (context.hw.numGammaOutSamples !=
>             RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES_V10) {
>                 LOG(RkISP1Gamma, Error)
>                         << "Gamma is not implemented for RkISP1 V12";
> @@ -101,7 +101,7 @@ void GammaOutCorrection::prepare(IPAContext &context,
>                                  IPAFrameContext &frameContext,
>                                  RkISP1Params *params)
>  {
> -       ASSERT(context.hw->numGammaOutSamples ==
> +       ASSERT(context.hw.numGammaOutSamples ==
>                RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES_V10);
> 
>         if (!frameContext.goc.update)
> diff --git a/src/ipa/rkisp1/algorithms/lux.cpp b/src/ipa/rkisp1/algorithms/lux.cpp
> index dd05f18d5e94..e8da69810008 100644
> --- a/src/ipa/rkisp1/algorithms/lux.cpp
> +++ b/src/ipa/rkisp1/algorithms/lux.cpp
> @@ -61,7 +61,7 @@ void Lux::process(IPAContext &context,
> 
>         /* \todo Deduplicate the histogram calculation from AGC */
>         const rkisp1_cif_isp_stat *params = &stats->params;
> -       Histogram yHist({ params->hist.hist_bins, context.hw->numHistogramBins },
> +       Histogram yHist({ params->hist.hist_bins, context.hw.numHistogramBins },
>                         [](uint32_t x) { return x >> 4; });
> 
>         double lux = lux_.estimateLux(exposureTime, gain, 1.0, yHist);
> diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
> index 362ab2fda5fe..57decbd3e2e6 100644
> --- a/src/ipa/rkisp1/ipa_context.h
> +++ b/src/ipa/rkisp1/ipa_context.h
> @@ -36,6 +36,7 @@ struct IPAHwSettings {
>         unsigned int numHistogramBins;
>         unsigned int numHistogramWeights;
>         unsigned int numGammaOutSamples;
> +       uint32_t supportedBlocks;
>         bool compand;
>  };
> 
> @@ -201,11 +202,11 @@ struct IPAFrameContext : public FrameContext {
> 
>  struct IPAContext {
>         IPAContext(unsigned int frameContextSize)
> -               : hw(nullptr), frameContexts(frameContextSize)
> +               : frameContexts(frameContextSize)
>         {
>         }
> 
> -       const IPAHwSettings *hw;
> +       IPAHwSettings hw;
>         IPACameraSensorInfo sensorInfo;
>         IPASessionConfiguration configuration;
>         IPAActiveState activeState;
> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> index cf66d5553dcd..fa22bfc34904 100644
> --- a/src/ipa/rkisp1/rkisp1.cpp
> +++ b/src/ipa/rkisp1/rkisp1.cpp
> @@ -52,6 +52,7 @@ public:
>         IPARkISP1();
> 
>         int init(const IPASettings &settings, unsigned int hwRevision,
> +                uint32_t supportedBlocks,
>                  const IPACameraSensorInfo &sensorInfo,
>                  const ControlInfoMap &sensorControls,
>                  ControlInfoMap *ipaControls) override;
> @@ -94,6 +95,7 @@ const IPAHwSettings ipaHwSettingsV10{
>         RKISP1_CIF_ISP_HIST_BIN_N_MAX_V10,
>         RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE_V10,
>         RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES_V10,
> +       0,
>         false,
>  };
> 
> @@ -102,6 +104,7 @@ const IPAHwSettings ipaHwSettingsIMX8MP{
>         RKISP1_CIF_ISP_HIST_BIN_N_MAX_V10,
>         RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE_V10,
>         RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES_V10,
> +       0,
>         true,
>  };
> 
> @@ -110,6 +113,7 @@ const IPAHwSettings ipaHwSettingsV12{
>         RKISP1_CIF_ISP_HIST_BIN_N_MAX_V12,
>         RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE_V12,
>         RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES_V12,
> +       0,
>         false,
>  };
> 
> @@ -132,6 +136,7 @@ std::string IPARkISP1::logPrefix() const
>  }
> 
>  int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision,
> +                   uint32_t supportedBlocks,
>                     const IPACameraSensorInfo &sensorInfo,
>                     const ControlInfoMap &sensorControls,
>                     ControlInfoMap *ipaControls)
> @@ -139,13 +144,13 @@ int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision,
>         /* \todo Add support for other revisions */
>         switch (hwRevision) {
>         case RKISP1_V10:
> -               context_.hw = &ipaHwSettingsV10;
> +               context_.hw = ipaHwSettingsV10;
>                 break;
>         case RKISP1_V_IMX8MP:
> -               context_.hw = &ipaHwSettingsIMX8MP;
> +               context_.hw = ipaHwSettingsIMX8MP;
>                 break;
>         case RKISP1_V12:
> -               context_.hw = &ipaHwSettingsV12;
> +               context_.hw = ipaHwSettingsV12;
>                 break;
>         default:
>                 LOG(IPARkISP1, Error)
> @@ -153,6 +158,7 @@ int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision,
>                         << " is currently not supported";
>                 return -ENODEV;
>         }
> +       context_.hw.supportedBlocks = supportedBlocks;
> 
>         LOG(IPARkISP1, Debug) << "Hardware revision is " << hwRevision;
> 
> 
> Which version do you like better?

I like this version more too :)

Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>

> 
> Best regards,
> Stefan
> 
> 
> > 
> > 
> > Regards,
> > Barnabás Pőcze
> > 
> > 
> > > +     context_.hw = hw;
> > >   
> > >       LOG(IPARkISP1, Debug) << "Hardware revision is " << hwRevision;
> > >   
> > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > index c4d4fdc6aa73..199f53d9e631 100644
> > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > @@ -100,7 +100,7 @@ public:
> > >   
> > >       PipelineHandlerRkISP1 *pipe();
> > >       const PipelineHandlerRkISP1 *pipe() const;
> > > -     int loadIPA(unsigned int hwRevision);
> > > +     int loadIPA(unsigned int hwRevision, uint32_t supportedBlocks);
> > >   
> > >       Stream mainPathStream_;
> > >       Stream selfPathStream_;
> > > @@ -383,7 +383,7 @@ const PipelineHandlerRkISP1 *RkISP1CameraData::pipe() const
> > >       return static_cast<const PipelineHandlerRkISP1 *>(Camera::Private::pipe());
> > >   }
> > >   
> > > -int RkISP1CameraData::loadIPA(unsigned int hwRevision)
> > > +int RkISP1CameraData::loadIPA(unsigned int hwRevision, uint32_t supportedBlocks)
> > >   {
> > >       ipa_ = IPAManager::createIPA<ipa::rkisp1::IPAProxyRkISP1>(pipe(), 1, 1);
> > >       if (!ipa_)
> > > @@ -405,7 +405,8 @@ int RkISP1CameraData::loadIPA(unsigned int hwRevision)
> > >       }
> > >   
> > >       ret = ipa_->init({ ipaTuningFile, sensor_->model() }, hwRevision,
> > > -                      sensorInfo, sensor_->controls(), &ipaControls_);
> > > +                      supportedBlocks, sensorInfo, sensor_->controls(),
> > > +                      &ipaControls_);
> > >       if (ret < 0) {
> > >               LOG(RkISP1, Error) << "IPA initialization failure";
> > >               return ret;
> > > @@ -1311,6 +1312,12 @@ int PipelineHandlerRkISP1::updateControls(RkISP1CameraData *data)
> > >       return 0;
> > >   }
> > >   
> > > +/*
> > > + * By default we assume all the blocks that were included in the first
> > > + * extensible parameters series are available. That is the lower 20bits.
> > > + */
> > > +const uint32_t kDefaultExtParamsBlocks = 0xfffff;
> > > +
> > >   int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)
> > >   {
> > >       int ret;
> > > @@ -1348,7 +1355,21 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)
> > >       isp_->frameStart.connect(data->delayedCtrls_.get(),
> > >                                &DelayedControls::applyControls);
> > >   
> > > -     ret = data->loadIPA(media_->hwRevision());
> > > +     uint32_t supportedBlocks = kDefaultExtParamsBlocks;
> > > +
> > > +     auto &controls = param_->controls();
> > > +     if (controls.find(RKISP1_CID_SUPPORTED_PARAMS_BLOCKS) != controls.end()) {
> > > +             auto list = param_->getControls({ { RKISP1_CID_SUPPORTED_PARAMS_BLOCKS } });
> > > +             if (!list.empty())
> > > +                     supportedBlocks = static_cast<uint32_t>(
> > > +                             list.get(RKISP1_CID_SUPPORTED_PARAMS_BLOCKS)
> > > +                                     .get<int32_t>());
> > > +     } else {
> > > +             LOG(RkISP1, Error)
> > > +                     << "Failed to query supported params blocks. Falling back to defaults.";
> > > +     }
> > > +
> > > +     ret = data->loadIPA(media_->hwRevision(), supportedBlocks);
> > >       if (ret)
> > >               return ret;
> > >   
> >

Patch
diff mbox series

diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom
index 043ad27ea199..068e898848c4 100644
--- a/include/libcamera/ipa/rkisp1.mojom
+++ b/include/libcamera/ipa/rkisp1.mojom
@@ -16,7 +16,7 @@  struct IPAConfigInfo {
 
 interface IPARkISP1Interface {
 	init(libcamera.IPASettings settings,
-	     uint32 hwRevision,
+	     uint32 hwRevision, uint32 supportedBlocks,
 	     libcamera.IPACameraSensorInfo sensorInfo,
 	     libcamera.ControlInfoMap sensorControls)
 		=> (int32 ret, libcamera.ControlInfoMap ipaControls);
diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
index 362ab2fda5fe..60cfab228edf 100644
--- a/src/ipa/rkisp1/ipa_context.h
+++ b/src/ipa/rkisp1/ipa_context.h
@@ -36,6 +36,7 @@  struct IPAHwSettings {
 	unsigned int numHistogramBins;
 	unsigned int numHistogramWeights;
 	unsigned int numGammaOutSamples;
+	uint32_t supportedBlocks;
 	bool compand;
 };
 
diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
index cf66d5553dcd..9ba0f0e6eb9d 100644
--- a/src/ipa/rkisp1/rkisp1.cpp
+++ b/src/ipa/rkisp1/rkisp1.cpp
@@ -52,6 +52,7 @@  public:
 	IPARkISP1();
 
 	int init(const IPASettings &settings, unsigned int hwRevision,
+		 uint32_t supportedBlocks,
 		 const IPACameraSensorInfo &sensorInfo,
 		 const ControlInfoMap &sensorControls,
 		 ControlInfoMap *ipaControls) override;
@@ -89,27 +90,30 @@  private:
 
 namespace {
 
-const IPAHwSettings ipaHwSettingsV10{
+IPAHwSettings ipaHwSettingsV10{
 	RKISP1_CIF_ISP_AE_MEAN_MAX_V10,
 	RKISP1_CIF_ISP_HIST_BIN_N_MAX_V10,
 	RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE_V10,
 	RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES_V10,
+	0,
 	false,
 };
 
-const IPAHwSettings ipaHwSettingsIMX8MP{
+IPAHwSettings ipaHwSettingsIMX8MP{
 	RKISP1_CIF_ISP_AE_MEAN_MAX_V10,
 	RKISP1_CIF_ISP_HIST_BIN_N_MAX_V10,
 	RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE_V10,
 	RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES_V10,
+	0,
 	true,
 };
 
-const IPAHwSettings ipaHwSettingsV12{
+IPAHwSettings ipaHwSettingsV12{
 	RKISP1_CIF_ISP_AE_MEAN_MAX_V12,
 	RKISP1_CIF_ISP_HIST_BIN_N_MAX_V12,
 	RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE_V12,
 	RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES_V12,
+	0,
 	false,
 };
 
@@ -132,20 +136,22 @@  std::string IPARkISP1::logPrefix() const
 }
 
 int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision,
+		    uint32_t supportedBlocks,
 		    const IPACameraSensorInfo &sensorInfo,
 		    const ControlInfoMap &sensorControls,
 		    ControlInfoMap *ipaControls)
 {
+	IPAHwSettings *hw = nullptr;
 	/* \todo Add support for other revisions */
 	switch (hwRevision) {
 	case RKISP1_V10:
-		context_.hw = &ipaHwSettingsV10;
+		hw = &ipaHwSettingsV10;
 		break;
 	case RKISP1_V_IMX8MP:
-		context_.hw = &ipaHwSettingsIMX8MP;
+		hw = &ipaHwSettingsIMX8MP;
 		break;
 	case RKISP1_V12:
-		context_.hw = &ipaHwSettingsV12;
+		hw = &ipaHwSettingsV12;
 		break;
 	default:
 		LOG(IPARkISP1, Error)
@@ -153,6 +159,8 @@  int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision,
 			<< " is currently not supported";
 		return -ENODEV;
 	}
+	hw->supportedBlocks = supportedBlocks;
+	context_.hw = hw;
 
 	LOG(IPARkISP1, Debug) << "Hardware revision is " << hwRevision;
 
diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
index c4d4fdc6aa73..199f53d9e631 100644
--- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
+++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
@@ -100,7 +100,7 @@  public:
 
 	PipelineHandlerRkISP1 *pipe();
 	const PipelineHandlerRkISP1 *pipe() const;
-	int loadIPA(unsigned int hwRevision);
+	int loadIPA(unsigned int hwRevision, uint32_t supportedBlocks);
 
 	Stream mainPathStream_;
 	Stream selfPathStream_;
@@ -383,7 +383,7 @@  const PipelineHandlerRkISP1 *RkISP1CameraData::pipe() const
 	return static_cast<const PipelineHandlerRkISP1 *>(Camera::Private::pipe());
 }
 
-int RkISP1CameraData::loadIPA(unsigned int hwRevision)
+int RkISP1CameraData::loadIPA(unsigned int hwRevision, uint32_t supportedBlocks)
 {
 	ipa_ = IPAManager::createIPA<ipa::rkisp1::IPAProxyRkISP1>(pipe(), 1, 1);
 	if (!ipa_)
@@ -405,7 +405,8 @@  int RkISP1CameraData::loadIPA(unsigned int hwRevision)
 	}
 
 	ret = ipa_->init({ ipaTuningFile, sensor_->model() }, hwRevision,
-			 sensorInfo, sensor_->controls(), &ipaControls_);
+			 supportedBlocks, sensorInfo, sensor_->controls(),
+			 &ipaControls_);
 	if (ret < 0) {
 		LOG(RkISP1, Error) << "IPA initialization failure";
 		return ret;
@@ -1311,6 +1312,12 @@  int PipelineHandlerRkISP1::updateControls(RkISP1CameraData *data)
 	return 0;
 }
 
+/*
+ * By default we assume all the blocks that were included in the first
+ * extensible parameters series are available. That is the lower 20bits.
+ */
+const uint32_t kDefaultExtParamsBlocks = 0xfffff;
+
 int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)
 {
 	int ret;
@@ -1348,7 +1355,21 @@  int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)
 	isp_->frameStart.connect(data->delayedCtrls_.get(),
 				 &DelayedControls::applyControls);
 
-	ret = data->loadIPA(media_->hwRevision());
+	uint32_t supportedBlocks = kDefaultExtParamsBlocks;
+
+	auto &controls = param_->controls();
+	if (controls.find(RKISP1_CID_SUPPORTED_PARAMS_BLOCKS) != controls.end()) {
+		auto list = param_->getControls({ { RKISP1_CID_SUPPORTED_PARAMS_BLOCKS } });
+		if (!list.empty())
+			supportedBlocks = static_cast<uint32_t>(
+				list.get(RKISP1_CID_SUPPORTED_PARAMS_BLOCKS)
+					.get<int32_t>());
+	} else {
+		LOG(RkISP1, Error)
+			<< "Failed to query supported params blocks. Falling back to defaults.";
+	}
+
+	ret = data->loadIPA(media_->hwRevision(), supportedBlocks);
 	if (ret)
 		return ret;