| Message ID | 20250815102945.1602071-15-stefan.klug@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
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; >
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; >
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; > > >
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; > > > > >
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; > > > > >
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;