| Message ID | 20230426131057.21550-9-naush@raspberrypi.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Naush On Wed, Apr 26, 2023 at 02:10:52PM +0100, Naushir Patuck via libcamera-devel wrote: > Create a new IpaBase class that handles general purpose housekeeping > duties for the Raspberry Pi IPA. Code the implementation of new class is > essentially pulled from the existing ipa/rpi/vc4/raspberrypi.cpp > file with a minimal amount of refactoring. > > Create a derived IpaVc4 class from IpaBase that handles the VC4 pipeline > specific tasks of the IPA. Again, code for this class implementation is > taken from the existing ipa/rpi/vc4/raspberrypi.cpp with a > minimal amount of refactoring. > > The goal of this change is to allow third parties to implement their own > IPA running on the Raspberry Pi without duplicating all of the IPA > housekeeping tasks. > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > --- > src/ipa/meson.build | 5 +- > .../raspberrypi.cpp => common/ipa_base.cpp} | 1219 +++++------------ > src/ipa/rpi/common/ipa_base.h | 125 ++ > src/ipa/rpi/common/meson.build | 7 + > src/ipa/rpi/vc4/meson.build | 8 +- > src/ipa/rpi/vc4/vc4.cpp | 540 ++++++++ > 6 files changed, 1012 insertions(+), 892 deletions(-) > rename src/ipa/rpi/{vc4/raspberrypi.cpp => common/ipa_base.cpp} (65%) > create mode 100644 src/ipa/rpi/common/ipa_base.h > create mode 100644 src/ipa/rpi/common/meson.build > create mode 100644 src/ipa/rpi/vc4/vc4.cpp > [snip] > > vc4_ipa_includes += include_directories('..') > -vc4_ipa_sources += [rpi_ipa_cam_helper_sources, rpi_ipa_controller_sources] > +vc4_ipa_sources += [ > + rpi_ipa_cam_helper_sources, > + rpi_ipa_common_sources, > + rpi_ipa_controller_sources, > +] > > mod = shared_module(ipa_name, > [vc4_ipa_sources, libcamera_generated_ipa_headers], > diff --git a/src/ipa/rpi/vc4/vc4.cpp b/src/ipa/rpi/vc4/vc4.cpp > new file mode 100644 > index 000000000000..0d929cda6c4a > --- /dev/null > +++ b/src/ipa/rpi/vc4/vc4.cpp > @@ -0,0 +1,540 @@ > +/* SPDX-License-Identifier: BSD-2-Clause */ > +/* > + * Copyright (C) 2019-2021, Raspberry Pi Ltd > + * > + * rpi.cpp - Raspberry Pi VC4/BCM2835 ISP IPA. > + */ > + > +#include <string.h> > +#include <sys/mman.h> > + > +#include <linux/bcm2835-isp.h> > + > +#include <libcamera/base/log.h> > + > +#include "common/ipa_base.h" > +#include "controller/af_status.h" > +#include "controller/agc_algorithm.h" > +#include "controller/alsc_status.h" > +#include "controller/awb_status.h" > +#include "controller/black_level_status.h" > +#include "controller/ccm_status.h" > +#include "controller/contrast_status.h" > +#include "controller/denoise_algorithm.h" > +#include "controller/denoise_status.h" > +#include "controller/dpc_status.h" > +#include "controller/geq_status.h" > +#include "controller/lux_status.h" > +#include "controller/noise_status.h" > +#include "controller/sharpen_status.h" > + > +namespace libcamera { > + > +LOG_DECLARE_CATEGORY(IPARPI) > + > +namespace ipa::RPi { > + > +class IpaVc4 final : public IpaBase > +{ > +public: > + IpaVc4() > + : IpaBase(), lsTable_(nullptr) > + { > + } > + > + ~IpaVc4() > + { > + if (lsTable_) > + munmap(lsTable_, MaxLsGridSize); > + } > + > + Additional blank line The rest looks good, I haven't gone in detail in the code as it's mostly copied from the existing implementations but the class desig looks correct to me Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> Thanks j > +private: > + int32_t platformInit(const InitParams ¶ms, InitResult *result) override; > + int32_t platformConfigure(const ConfigParams ¶ms, ConfigResult *result) override; > + > + void platformPrepareIsp(const PrepareParams ¶ms, RPiController::Metadata &rpiMetadata) override; > + RPiController::StatisticsPtr platformProcessStats(Span<uint8_t> mem) override; > + > + void handleControls(const ControlList &controls) override; > + bool validateIspControls(); > + > + void applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls); > + void applyDG(const struct AgcStatus *dgStatus, ControlList &ctrls); > + void applyCCM(const struct CcmStatus *ccmStatus, ControlList &ctrls); > + void applyBlackLevel(const struct BlackLevelStatus *blackLevelStatus, ControlList &ctrls); > + void applyGamma(const struct ContrastStatus *contrastStatus, ControlList &ctrls); > + void applyGEQ(const struct GeqStatus *geqStatus, ControlList &ctrls); > + void applyDenoise(const struct DenoiseStatus *denoiseStatus, ControlList &ctrls); > + void applySharpen(const struct SharpenStatus *sharpenStatus, ControlList &ctrls); > + void applyDPC(const struct DpcStatus *dpcStatus, ControlList &ctrls); > + void applyLS(const struct AlscStatus *lsStatus, ControlList &ctrls); > + void applyAF(const struct AfStatus *afStatus, ControlList &lensCtrls); > + void resampleTable(uint16_t dest[], const std::vector<double> &src, int destW, int destH); > + > + /* VC4 ISP controls. */ > + ControlInfoMap ispCtrls_; > + > + /* LS table allocation passed in from the pipeline handler. */ > + SharedFD lsTableHandle_; > + void *lsTable_; > +}; > + > +int32_t IpaVc4::platformInit([[maybe_unused]] const InitParams ¶ms, [[maybe_unused]] InitResult *result) > +{ > + const std::string &target = controller_.getTarget(); > + > + if (target != "bcm2835") { > + LOG(IPARPI, Error) > + << "Tuning data file target returned \"" << target << "\"" > + << ", expected \"bcm2835\""; > + return -EINVAL; > + } > + > + return 0; > +} > + > +int32_t IpaVc4::platformConfigure(const ConfigParams ¶ms, [[maybe_unused]] ConfigResult *result) > +{ > + ispCtrls_ = params.ispControls; > + if (!validateIspControls()) { > + LOG(IPARPI, Error) << "ISP control validation failed."; > + return -1; > + } > + > + /* Store the lens shading table pointer and handle if available. */ > + if (params.lsTableHandle.isValid()) { > + /* Remove any previous table, if there was one. */ > + if (lsTable_) { > + munmap(lsTable_, MaxLsGridSize); > + lsTable_ = nullptr; > + } > + > + /* Map the LS table buffer into user space. */ > + lsTableHandle_ = std::move(params.lsTableHandle); > + if (lsTableHandle_.isValid()) { > + lsTable_ = mmap(nullptr, MaxLsGridSize, PROT_READ | PROT_WRITE, > + MAP_SHARED, lsTableHandle_.get(), 0); > + > + if (lsTable_ == MAP_FAILED) { > + LOG(IPARPI, Error) << "dmaHeap mmap failure for LS table."; > + lsTable_ = nullptr; > + } > + } > + } > + > + return 0; > +} > + > +void IpaVc4::platformPrepareIsp([[maybe_unused]] const PrepareParams ¶ms, > + RPiController::Metadata &rpiMetadata) > +{ > + ControlList ctrls(ispCtrls_); > + > + /* Lock the metadata buffer to avoid constant locks/unlocks. */ > + std::unique_lock<RPiController::Metadata> lock(rpiMetadata); > + > + AwbStatus *awbStatus = rpiMetadata.getLocked<AwbStatus>("awb.status"); > + if (awbStatus) > + applyAWB(awbStatus, ctrls); > + > + CcmStatus *ccmStatus = rpiMetadata.getLocked<CcmStatus>("ccm.status"); > + if (ccmStatus) > + applyCCM(ccmStatus, ctrls); > + > + AgcStatus *dgStatus = rpiMetadata.getLocked<AgcStatus>("agc.status"); > + if (dgStatus) > + applyDG(dgStatus, ctrls); > + > + AlscStatus *lsStatus = rpiMetadata.getLocked<AlscStatus>("alsc.status"); > + if (lsStatus) > + applyLS(lsStatus, ctrls); > + > + ContrastStatus *contrastStatus = rpiMetadata.getLocked<ContrastStatus>("contrast.status"); > + if (contrastStatus) > + applyGamma(contrastStatus, ctrls); > + > + BlackLevelStatus *blackLevelStatus = rpiMetadata.getLocked<BlackLevelStatus>("black_level.status"); > + if (blackLevelStatus) > + applyBlackLevel(blackLevelStatus, ctrls); > + > + GeqStatus *geqStatus = rpiMetadata.getLocked<GeqStatus>("geq.status"); > + if (geqStatus) > + applyGEQ(geqStatus, ctrls); > + > + DenoiseStatus *denoiseStatus = rpiMetadata.getLocked<DenoiseStatus>("denoise.status"); > + if (denoiseStatus) > + applyDenoise(denoiseStatus, ctrls); > + > + SharpenStatus *sharpenStatus = rpiMetadata.getLocked<SharpenStatus>("sharpen.status"); > + if (sharpenStatus) > + applySharpen(sharpenStatus, ctrls); > + > + DpcStatus *dpcStatus = rpiMetadata.getLocked<DpcStatus>("dpc.status"); > + if (dpcStatus) > + applyDPC(dpcStatus, ctrls); > + > + const AfStatus *afStatus = rpiMetadata.getLocked<AfStatus>("af.status"); > + if (afStatus) { > + ControlList lensctrls(lensCtrls_); > + applyAF(afStatus, lensctrls); > + if (!lensctrls.empty()) > + setLensControls.emit(lensctrls); > + } > + > + if (!ctrls.empty()) > + setIspControls.emit(ctrls); > +} > + > +RPiController::StatisticsPtr IpaVc4::platformProcessStats(Span<uint8_t> mem) > +{ > + using namespace RPiController; > + > + const bcm2835_isp_stats *stats = reinterpret_cast<bcm2835_isp_stats *>(mem.data()); > + StatisticsPtr statistics = std::make_unique<Statistics>(Statistics::AgcStatsPos::PreWb, > + Statistics::ColourStatsPos::PostLsc); > + const Controller::HardwareConfig &hw = controller_.getHardwareConfig(); > + unsigned int i; > + > + /* RGB histograms are not used, so do not populate them. */ > + statistics->yHist = RPiController::Histogram(stats->hist[0].g_hist, > + hw.numHistogramBins); > + > + /* All region sums are based on a 16-bit normalised pipeline bit-depth. */ > + unsigned int scale = Statistics::NormalisationFactorPow2 - hw.pipelineWidth; > + > + statistics->awbRegions.init(hw.awbRegions); > + for (i = 0; i < statistics->awbRegions.numRegions(); i++) > + statistics->awbRegions.set(i, { { stats->awb_stats[i].r_sum << scale, > + stats->awb_stats[i].g_sum << scale, > + stats->awb_stats[i].b_sum << scale }, > + stats->awb_stats[i].counted, > + stats->awb_stats[i].notcounted }); > + > + statistics->agcRegions.init(hw.agcRegions); > + for (i = 0; i < statistics->agcRegions.numRegions(); i++) > + statistics->agcRegions.set(i, { { stats->agc_stats[i].r_sum << scale, > + stats->agc_stats[i].g_sum << scale, > + stats->agc_stats[i].b_sum << scale }, > + stats->agc_stats[i].counted, > + stats->awb_stats[i].notcounted }); > + > + statistics->focusRegions.init(hw.focusRegions); > + for (i = 0; i < statistics->focusRegions.numRegions(); i++) > + statistics->focusRegions.set(i, { stats->focus_stats[i].contrast_val[1][1] / 1000, > + stats->focus_stats[i].contrast_val_num[1][1], > + stats->focus_stats[i].contrast_val_num[1][0] }); > + > + return statistics; > +} > + > +void IpaVc4::handleControls([[maybe_unused]] const ControlList &controls) > +{ > + /* No controls require any special updates to the hardware configuration. */ > +} > + > +bool IpaVc4::validateIspControls() > +{ > + static const uint32_t ctrls[] = { > + V4L2_CID_RED_BALANCE, > + V4L2_CID_BLUE_BALANCE, > + V4L2_CID_DIGITAL_GAIN, > + V4L2_CID_USER_BCM2835_ISP_CC_MATRIX, > + V4L2_CID_USER_BCM2835_ISP_GAMMA, > + V4L2_CID_USER_BCM2835_ISP_BLACK_LEVEL, > + V4L2_CID_USER_BCM2835_ISP_GEQ, > + V4L2_CID_USER_BCM2835_ISP_DENOISE, > + V4L2_CID_USER_BCM2835_ISP_SHARPEN, > + V4L2_CID_USER_BCM2835_ISP_DPC, > + V4L2_CID_USER_BCM2835_ISP_LENS_SHADING, > + V4L2_CID_USER_BCM2835_ISP_CDN, > + }; > + > + for (auto c : ctrls) { > + if (ispCtrls_.find(c) == ispCtrls_.end()) { > + LOG(IPARPI, Error) << "Unable to find ISP control " > + << utils::hex(c); > + return false; > + } > + } > + > + return true; > +} > + > +void IpaVc4::applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls) > +{ > + LOG(IPARPI, Debug) << "Applying WB R: " << awbStatus->gainR << " B: " > + << awbStatus->gainB; > + > + ctrls.set(V4L2_CID_RED_BALANCE, > + static_cast<int32_t>(awbStatus->gainR * 1000)); > + ctrls.set(V4L2_CID_BLUE_BALANCE, > + static_cast<int32_t>(awbStatus->gainB * 1000)); > +} > + > +void IpaVc4::applyDG(const struct AgcStatus *dgStatus, ControlList &ctrls) > +{ > + ctrls.set(V4L2_CID_DIGITAL_GAIN, > + static_cast<int32_t>(dgStatus->digitalGain * 1000)); > +} > + > +void IpaVc4::applyCCM(const struct CcmStatus *ccmStatus, ControlList &ctrls) > +{ > + bcm2835_isp_custom_ccm ccm; > + > + for (int i = 0; i < 9; i++) { > + ccm.ccm.ccm[i / 3][i % 3].den = 1000; > + ccm.ccm.ccm[i / 3][i % 3].num = 1000 * ccmStatus->matrix[i]; > + } > + > + ccm.enabled = 1; > + ccm.ccm.offsets[0] = ccm.ccm.offsets[1] = ccm.ccm.offsets[2] = 0; > + > + ControlValue c(Span<const uint8_t>{ reinterpret_cast<uint8_t *>(&ccm), > + sizeof(ccm) }); > + ctrls.set(V4L2_CID_USER_BCM2835_ISP_CC_MATRIX, c); > +} > + > +void IpaVc4::applyBlackLevel(const struct BlackLevelStatus *blackLevelStatus, ControlList &ctrls) > +{ > + bcm2835_isp_black_level blackLevel; > + > + blackLevel.enabled = 1; > + blackLevel.black_level_r = blackLevelStatus->blackLevelR; > + blackLevel.black_level_g = blackLevelStatus->blackLevelG; > + blackLevel.black_level_b = blackLevelStatus->blackLevelB; > + > + ControlValue c(Span<const uint8_t>{ reinterpret_cast<uint8_t *>(&blackLevel), > + sizeof(blackLevel) }); > + ctrls.set(V4L2_CID_USER_BCM2835_ISP_BLACK_LEVEL, c); > +} > + > +void IpaVc4::applyGamma(const struct ContrastStatus *contrastStatus, ControlList &ctrls) > +{ > + const unsigned int numGammaPoints = controller_.getHardwareConfig().numGammaPoints; > + struct bcm2835_isp_gamma gamma; > + > + for (unsigned int i = 0; i < numGammaPoints - 1; i++) { > + int x = i < 16 ? i * 1024 > + : (i < 24 ? (i - 16) * 2048 + 16384 > + : (i - 24) * 4096 + 32768); > + gamma.x[i] = x; > + gamma.y[i] = std::min<uint16_t>(65535, contrastStatus->gammaCurve.eval(x)); > + } > + > + gamma.x[numGammaPoints - 1] = 65535; > + gamma.y[numGammaPoints - 1] = 65535; > + gamma.enabled = 1; > + > + ControlValue c(Span<const uint8_t>{ reinterpret_cast<uint8_t *>(&gamma), > + sizeof(gamma) }); > + ctrls.set(V4L2_CID_USER_BCM2835_ISP_GAMMA, c); > +} > + > +void IpaVc4::applyGEQ(const struct GeqStatus *geqStatus, ControlList &ctrls) > +{ > + bcm2835_isp_geq geq; > + > + geq.enabled = 1; > + geq.offset = geqStatus->offset; > + geq.slope.den = 1000; > + geq.slope.num = 1000 * geqStatus->slope; > + > + ControlValue c(Span<const uint8_t>{ reinterpret_cast<uint8_t *>(&geq), > + sizeof(geq) }); > + ctrls.set(V4L2_CID_USER_BCM2835_ISP_GEQ, c); > +} > + > +void IpaVc4::applyDenoise(const struct DenoiseStatus *denoiseStatus, ControlList &ctrls) > +{ > + using RPiController::DenoiseMode; > + > + bcm2835_isp_denoise denoise; > + DenoiseMode mode = static_cast<DenoiseMode>(denoiseStatus->mode); > + > + denoise.enabled = mode != DenoiseMode::Off; > + denoise.constant = denoiseStatus->noiseConstant; > + denoise.slope.num = 1000 * denoiseStatus->noiseSlope; > + denoise.slope.den = 1000; > + denoise.strength.num = 1000 * denoiseStatus->strength; > + denoise.strength.den = 1000; > + > + /* Set the CDN mode to match the SDN operating mode. */ > + bcm2835_isp_cdn cdn; > + switch (mode) { > + case DenoiseMode::ColourFast: > + cdn.enabled = 1; > + cdn.mode = CDN_MODE_FAST; > + break; > + case DenoiseMode::ColourHighQuality: > + cdn.enabled = 1; > + cdn.mode = CDN_MODE_HIGH_QUALITY; > + break; > + default: > + cdn.enabled = 0; > + } > + > + ControlValue c(Span<const uint8_t>{ reinterpret_cast<uint8_t *>(&denoise), > + sizeof(denoise) }); > + ctrls.set(V4L2_CID_USER_BCM2835_ISP_DENOISE, c); > + > + c = ControlValue(Span<const uint8_t>{ reinterpret_cast<uint8_t *>(&cdn), > + sizeof(cdn) }); > + ctrls.set(V4L2_CID_USER_BCM2835_ISP_CDN, c); > +} > + > +void IpaVc4::applySharpen(const struct SharpenStatus *sharpenStatus, ControlList &ctrls) > +{ > + bcm2835_isp_sharpen sharpen; > + > + sharpen.enabled = 1; > + sharpen.threshold.num = 1000 * sharpenStatus->threshold; > + sharpen.threshold.den = 1000; > + sharpen.strength.num = 1000 * sharpenStatus->strength; > + sharpen.strength.den = 1000; > + sharpen.limit.num = 1000 * sharpenStatus->limit; > + sharpen.limit.den = 1000; > + > + ControlValue c(Span<const uint8_t>{ reinterpret_cast<uint8_t *>(&sharpen), > + sizeof(sharpen) }); > + ctrls.set(V4L2_CID_USER_BCM2835_ISP_SHARPEN, c); > +} > + > +void IpaVc4::applyDPC(const struct DpcStatus *dpcStatus, ControlList &ctrls) > +{ > + bcm2835_isp_dpc dpc; > + > + dpc.enabled = 1; > + dpc.strength = dpcStatus->strength; > + > + ControlValue c(Span<const uint8_t>{ reinterpret_cast<uint8_t *>(&dpc), > + sizeof(dpc) }); > + ctrls.set(V4L2_CID_USER_BCM2835_ISP_DPC, c); > +} > + > +void IpaVc4::applyLS(const struct AlscStatus *lsStatus, ControlList &ctrls) > +{ > + /* > + * Program lens shading tables into pipeline. > + * Choose smallest cell size that won't exceed 63x48 cells. > + */ > + const int cellSizes[] = { 16, 32, 64, 128, 256 }; > + unsigned int numCells = std::size(cellSizes); > + unsigned int i, w, h, cellSize; > + for (i = 0; i < numCells; i++) { > + cellSize = cellSizes[i]; > + w = (mode_.width + cellSize - 1) / cellSize; > + h = (mode_.height + cellSize - 1) / cellSize; > + if (w < 64 && h <= 48) > + break; > + } > + > + if (i == numCells) { > + LOG(IPARPI, Error) << "Cannot find cell size"; > + return; > + } > + > + /* We're going to supply corner sampled tables, 16 bit samples. */ > + w++, h++; > + bcm2835_isp_lens_shading ls = { > + .enabled = 1, > + .grid_cell_size = cellSize, > + .grid_width = w, > + .grid_stride = w, > + .grid_height = h, > + /* .dmabuf will be filled in by pipeline handler. */ > + .dmabuf = 0, > + .ref_transform = 0, > + .corner_sampled = 1, > + .gain_format = GAIN_FORMAT_U4P10 > + }; > + > + if (!lsTable_ || w * h * 4 * sizeof(uint16_t) > MaxLsGridSize) { > + LOG(IPARPI, Error) << "Do not have a correctly allocate lens shading table!"; > + return; > + } > + > + if (lsStatus) { > + /* Format will be u4.10 */ > + uint16_t *grid = static_cast<uint16_t *>(lsTable_); > + > + resampleTable(grid, lsStatus->r, w, h); > + resampleTable(grid + w * h, lsStatus->g, w, h); > + memcpy(grid + 2 * w * h, grid + w * h, w * h * sizeof(uint16_t)); > + resampleTable(grid + 3 * w * h, lsStatus->b, w, h); > + } > + > + ControlValue c(Span<const uint8_t>{ reinterpret_cast<uint8_t *>(&ls), > + sizeof(ls) }); > + ctrls.set(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING, c); > +} > + > +void IpaVc4::applyAF(const struct AfStatus *afStatus, ControlList &lensCtrls) > +{ > + if (afStatus->lensSetting) { > + ControlValue v(afStatus->lensSetting.value()); > + lensCtrls.set(V4L2_CID_FOCUS_ABSOLUTE, v); > + } > +} > + > +/* > + * Resamples a 16x12 table with central sampling to destW x destH with corner > + * sampling. > + */ > +void IpaVc4::resampleTable(uint16_t dest[], const std::vector<double> &src, > + int destW, int destH) > +{ > + /* > + * Precalculate and cache the x sampling locations and phases to > + * save recomputing them on every row. > + */ > + assert(destW > 1 && destH > 1 && destW <= 64); > + int xLo[64], xHi[64]; > + double xf[64]; > + double x = -0.5, xInc = 16.0 / (destW - 1); > + for (int i = 0; i < destW; i++, x += xInc) { > + xLo[i] = floor(x); > + xf[i] = x - xLo[i]; > + xHi[i] = xLo[i] < 15 ? xLo[i] + 1 : 15; > + xLo[i] = xLo[i] > 0 ? xLo[i] : 0; > + } > + > + /* Now march over the output table generating the new values. */ > + double y = -0.5, yInc = 12.0 / (destH - 1); > + for (int j = 0; j < destH; j++, y += yInc) { > + int yLo = floor(y); > + double yf = y - yLo; > + int yHi = yLo < 11 ? yLo + 1 : 11; > + yLo = yLo > 0 ? yLo : 0; > + double const *rowAbove = src.data() + yLo * 16; > + double const *rowBelow = src.data() + yHi * 16; > + for (int i = 0; i < destW; i++) { > + double above = rowAbove[xLo[i]] * (1 - xf[i]) + rowAbove[xHi[i]] * xf[i]; > + double below = rowBelow[xLo[i]] * (1 - xf[i]) + rowBelow[xHi[i]] * xf[i]; > + int result = floor(1024 * (above * (1 - yf) + below * yf) + .5); > + *(dest++) = result > 16383 ? 16383 : result; /* want u4.10 */ > + } > + } > +} > + > +} /* namespace ipa::RPi */ > + > +/* > + * External IPA module interface > + */ > +extern "C" { > +const struct IPAModuleInfo ipaModuleInfo = { > + IPA_MODULE_API_VERSION, > + 1, > + "PipelineHandlerRPi", > + "vc4", > +}; > + > +IPAInterface *ipaCreate() > +{ > + return new ipa::RPi::IpaVc4(); > +} > + > +} /* extern "C" */ > + > +} /* namespace libcamera */ > -- > 2.34.1 >
Hi Naush, Thank you for the patch. On Wed, Apr 26, 2023 at 02:10:52PM +0100, Naushir Patuck via libcamera-devel wrote: > Create a new IpaBase class that handles general purpose housekeeping > duties for the Raspberry Pi IPA. Code the implementation of new class is Did you mean s/Code the implementation/The implementation/ and s/of new class/of the new class/ ? > essentially pulled from the existing ipa/rpi/vc4/raspberrypi.cpp > file with a minimal amount of refactoring. > > Create a derived IpaVc4 class from IpaBase that handles the VC4 pipeline > specific tasks of the IPA. Again, code for this class implementation is > taken from the existing ipa/rpi/vc4/raspberrypi.cpp with a > minimal amount of refactoring. > > The goal of this change is to allow third parties to implement their own > IPA running on the Raspberry Pi without duplicating all of the IPA > housekeeping tasks. > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > --- > src/ipa/meson.build | 5 +- > .../raspberrypi.cpp => common/ipa_base.cpp} | 1219 +++++------------ > src/ipa/rpi/common/ipa_base.h | 125 ++ > src/ipa/rpi/common/meson.build | 7 + > src/ipa/rpi/vc4/meson.build | 8 +- > src/ipa/rpi/vc4/vc4.cpp | 540 ++++++++ > 6 files changed, 1012 insertions(+), 892 deletions(-) > rename src/ipa/rpi/{vc4/raspberrypi.cpp => common/ipa_base.cpp} (65%) > create mode 100644 src/ipa/rpi/common/ipa_base.h > create mode 100644 src/ipa/rpi/common/meson.build > create mode 100644 src/ipa/rpi/vc4/vc4.cpp > > diff --git a/src/ipa/meson.build b/src/ipa/meson.build > index 10d3b44ca7b6..0c622c38a4a0 100644 > --- a/src/ipa/meson.build > +++ b/src/ipa/meson.build > @@ -37,13 +37,14 @@ endif > > enabled_ipa_modules = [] > > -# If the Raspberry Pi VC4 IPA is enabled, ensure we include the rpi/cam_helper > -# and rpi/controller subdirectories in the build. > +# If the Raspberry Pi VC4 IPA is enabled, ensure we include the cam_helper, > +# common and controller subdirectories in the build. > # > # This is done here and not within rpi/vc4/meson.build as meson does not > # allow the subdir command to traverse up the directory tree. > if pipelines.contains('rpi/vc4') > subdir('rpi/cam_helper') > + subdir('rpi/common') > subdir('rpi/controller') > endif > > diff --git a/src/ipa/rpi/vc4/raspberrypi.cpp b/src/ipa/rpi/common/ipa_base.cpp > similarity index 65% > rename from src/ipa/rpi/vc4/raspberrypi.cpp > rename to src/ipa/rpi/common/ipa_base.cpp > index e3ca8e2f7cbe..becada5973ad 100644 > --- a/src/ipa/rpi/vc4/raspberrypi.cpp > +++ b/src/ipa/rpi/common/ipa_base.cpp > @@ -1,60 +1,30 @@ [snip] > @@ -62,8 +32,7 @@ namespace libcamera { > using namespace std::literals::chrono_literals; > using utils::Duration; > > -/* Number of metadata objects available in the context list. */ > -constexpr unsigned int numMetadataContexts = 16; > +namespace { > > /* Number of frame length times to hold in the queue. */ > constexpr unsigned int FrameLengthsQueueSize = 10; > @@ -80,7 +49,7 @@ constexpr Duration defaultMaxFrameDuration = 250.0s; > * we rate-limit the controller Prepare() and Process() calls to lower than or > * equal to this rate. > */ > -constexpr Duration controllerMinFrameDuration = 1.0s / 30.0; > +static constexpr Duration controllerMinFrameDuration = 1.0s / 30.0; Everything contained in an anonymous namespace becomes static. You can drop the static keyword from other variables. > > /* List of controls handled by the Raspberry Pi IPA */ > static const ControlInfoMap::Map ipaControls{ > @@ -116,118 +85,23 @@ static const ControlInfoMap::Map ipaAfControls{ > { &controls::LensPosition, ControlInfo(0.0f, 32.0f, 1.0f) } > }; > > +} /* namespace */ > + [snip] > diff --git a/src/ipa/rpi/common/ipa_base.h b/src/ipa/rpi/common/ipa_base.h > new file mode 100644 > index 000000000000..b9fbf9414d63 > --- /dev/null > +++ b/src/ipa/rpi/common/ipa_base.h > @@ -0,0 +1,125 @@ > +/* SPDX-License-Identifier: BSD-2-Clause */ > +/* > + * Copyright (C) 2023, Raspberry Pi Ltd > + * > + * ipa_base.cpp - Raspberry Pi IPA base class ipa_base.h > + */ > +#pragma once > + > +#include <array> > +#include <deque> > +#include <vector> You need <map> here, but can drop <vector> as it's only used to implement the interface of the base class, and so is provided by libcamera/ipa/ipa_interface.h. You also need stdint.h. > + #include <libcamera/base/span.h> > +#include <libcamera/base/utils.h> > + > +#include <libcamera/controls.h> > + > +#include <libcamera/ipa/ipa_interface.h> > +#include <libcamera/ipa/ipa_module_info.h> Not needed. > +#include <libcamera/ipa/raspberrypi_ipa_interface.h> > + > +#include "libcamera/internal/mapped_framebuffer.h" > + > +#include "cam_helper/cam_helper.h" > +#include "controller/agc_status.h" > +#include "controller/camera_mode.h" > +#include "controller/controller.h" > +#include "controller/metadata.h" > + > +namespace libcamera { > + > +namespace ipa::RPi { > + > +class IpaBase : public IPARPiInterface > +{ > +public: > + IpaBase(); > + virtual ~IpaBase() = 0; That's a bit weird, why do you need a virtual abstract destructor ? > + > + int32_t init(const IPASettings &settings, const InitParams ¶ms, InitResult *result) override; > + int32_t configure(const IPACameraSensorInfo &sensorInfo, const ConfigParams ¶ms, > + ConfigResult *result) override; > + > + void start(const ControlList &controls, StartResult *result) override; > + void stop() override {} > + > + void mapBuffers(const std::vector<IPABuffer> &buffers) override; > + void unmapBuffers(const std::vector<unsigned int> &ids) override; > + > + void prepareIsp(const PrepareParams ¶ms) override; > + void processStats(const ProcessParams ¶ms) override; > + > + void reportMetadata(unsigned int ipaContext, ControlList *controls) override; > + > +private: > + /* Number of metadata objects available in the context list. */ > + static constexpr unsigned int numMetadataContexts = 16; > + > + virtual int32_t platformInit(const InitParams ¶ms, InitResult *result) = 0; This enforces a particular sequence of calls between the base class and the derived class, dictated by the base class design. It's best to handle this in the derived class instead. You can override the init() function in the derived class, call the init() function of the base class, and then perform all the work you're currently doing in IpaVc4::platformInit(). The same comment applies to the other platform*() functions (some may require a little bit of refactoring). The handleControls() function can also be dropped by making applyControls() a protected virtual function that derived classes may override. > + virtual int32_t platformConfigure(const ConfigParams ¶ms, ConfigResult *result) = 0; > + > + virtual void platformPrepareIsp(const PrepareParams ¶ms, > + RPiController::Metadata &rpiMetadata) = 0; > + virtual RPiController::StatisticsPtr platformProcessStats(Span<uint8_t> mem) = 0; > + > + void setMode(const IPACameraSensorInfo &sensorInfo); > + void setCameraTimeoutValue(); > + bool validateSensorControls(); > + bool validateLensControls(); > + void applyControls(const ControlList &controls); > + virtual void handleControls(const ControlList &controls) = 0; > + void fillDeviceStatus(const ControlList &sensorControls, unsigned int ipaContext); > + void applyFrameDurations(utils::Duration minFrameDuration, utils::Duration maxFrameDuration); > + void applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls); > + > + std::map<unsigned int, MappedFrameBuffer> buffers_; > + > + bool lensPresent_; > + ControlList libcameraMetadata_; > + > + std::array<RPiController::Metadata, numMetadataContexts> rpiMetadata_; > + > + /* > + * We count frames to decide if the frame must be hidden (e.g. from > + * display) or mistrusted (i.e. not given to the control algos). > + */ > + uint64_t frameCount_; > + > + /* How many frames we should avoid running control algos on. */ > + unsigned int mistrustCount_; > + > + /* Number of frames that need to be dropped on startup. */ > + unsigned int dropFrameCount_; > + > + /* Frame timestamp for the last run of the controller. */ > + uint64_t lastRunTimestamp_; > + > + /* Do we run a Controller::process() for this frame? */ > + bool processPending_; > + > + /* Distinguish the first camera start from others. */ > + bool firstStart_; > + > + /* Frame duration (1/fps) limits. */ > + utils::Duration minFrameDuration_; > + utils::Duration maxFrameDuration_; > + > +protected: We usually put protected members before private members. > + /* Raspberry Pi controller specific defines. */ > + std::unique_ptr<RPiController::CamHelper> helper_; > + RPiController::Controller controller_; > + > + ControlInfoMap sensorCtrls_; > + ControlInfoMap lensCtrls_; > + > + /* Camera sensor params. */ > + CameraMode mode_; > + > + /* Track the frame length times over FrameLengthsQueueSize frames. */ > + std::deque<utils::Duration> frameLengths_; > + utils::Duration lastTimeout_; > +}; > + > +} /* namespace ipa::RPi */ > + > +} /* namespace libcamera */ > diff --git a/src/ipa/rpi/common/meson.build b/src/ipa/rpi/common/meson.build > new file mode 100644 > index 000000000000..a7189f8389af > --- /dev/null > +++ b/src/ipa/rpi/common/meson.build > @@ -0,0 +1,7 @@ > +# SPDX-License-Identifier: CC0-1.0 > + > +# SPDX-License-Identifier: CC0-1.0 One SPDX header is enough :-) > + > +rpi_ipa_common_sources = files([ > + 'ipa_base.cpp', > +]) > diff --git a/src/ipa/rpi/vc4/meson.build b/src/ipa/rpi/vc4/meson.build > index 992d0f1ab5a7..b581391586b3 100644 > --- a/src/ipa/rpi/vc4/meson.build > +++ b/src/ipa/rpi/vc4/meson.build > @@ -13,11 +13,15 @@ vc4_ipa_includes = [ > ] > > vc4_ipa_sources = files([ > - 'raspberrypi.cpp', > + 'vc4.cpp', > ]) > > vc4_ipa_includes += include_directories('..') > -vc4_ipa_sources += [rpi_ipa_cam_helper_sources, rpi_ipa_controller_sources] > +vc4_ipa_sources += [ > + rpi_ipa_cam_helper_sources, > + rpi_ipa_common_sources, > + rpi_ipa_controller_sources, Could we avoid compiling all these source files multiple times, once for the IpaVc4 IPA module, and once for the other IPA modules that may be implemented in the future ? This can be done with a static library, see src/apps/common/meson.build and src/apps/cam/meson.build for instance. > +] > > mod = shared_module(ipa_name, > [vc4_ipa_sources, libcamera_generated_ipa_headers], [snip]
Hi Laurent, Thank you for your feedback. On Thu, 27 Apr 2023 at 16:54, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Naush, > > Thank you for the patch. > > On Wed, Apr 26, 2023 at 02:10:52PM +0100, Naushir Patuck via libcamera-devel wrote: > > Create a new IpaBase class that handles general purpose housekeeping > > duties for the Raspberry Pi IPA. Code the implementation of new class is > > Did you mean s/Code the implementation/The implementation/ and > s/of new class/of the new class/ ? I actually meant "Code for the implementation...", but I'll replace it with your wording. > > > essentially pulled from the existing ipa/rpi/vc4/raspberrypi.cpp > > file with a minimal amount of refactoring. > > > > Create a derived IpaVc4 class from IpaBase that handles the VC4 pipeline > > specific tasks of the IPA. Again, code for this class implementation is > > taken from the existing ipa/rpi/vc4/raspberrypi.cpp with a > > minimal amount of refactoring. > > > > The goal of this change is to allow third parties to implement their own > > IPA running on the Raspberry Pi without duplicating all of the IPA > > housekeeping tasks. > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > --- > > src/ipa/meson.build | 5 +- > > .../raspberrypi.cpp => common/ipa_base.cpp} | 1219 +++++------------ > > src/ipa/rpi/common/ipa_base.h | 125 ++ > > src/ipa/rpi/common/meson.build | 7 + > > src/ipa/rpi/vc4/meson.build | 8 +- > > src/ipa/rpi/vc4/vc4.cpp | 540 ++++++++ > > 6 files changed, 1012 insertions(+), 892 deletions(-) > > rename src/ipa/rpi/{vc4/raspberrypi.cpp => common/ipa_base.cpp} (65%) > > create mode 100644 src/ipa/rpi/common/ipa_base.h > > create mode 100644 src/ipa/rpi/common/meson.build > > create mode 100644 src/ipa/rpi/vc4/vc4.cpp > > > > diff --git a/src/ipa/meson.build b/src/ipa/meson.build > > index 10d3b44ca7b6..0c622c38a4a0 100644 > > --- a/src/ipa/meson.build > > +++ b/src/ipa/meson.build > > @@ -37,13 +37,14 @@ endif > > > > enabled_ipa_modules = [] > > > > -# If the Raspberry Pi VC4 IPA is enabled, ensure we include the rpi/cam_helper > > -# and rpi/controller subdirectories in the build. > > +# If the Raspberry Pi VC4 IPA is enabled, ensure we include the cam_helper, > > +# common and controller subdirectories in the build. > > # > > # This is done here and not within rpi/vc4/meson.build as meson does not > > # allow the subdir command to traverse up the directory tree. > > if pipelines.contains('rpi/vc4') > > subdir('rpi/cam_helper') > > + subdir('rpi/common') > > subdir('rpi/controller') > > endif > > > > diff --git a/src/ipa/rpi/vc4/raspberrypi.cpp b/src/ipa/rpi/common/ipa_base.cpp > > similarity index 65% > > rename from src/ipa/rpi/vc4/raspberrypi.cpp > > rename to src/ipa/rpi/common/ipa_base.cpp > > index e3ca8e2f7cbe..becada5973ad 100644 > > --- a/src/ipa/rpi/vc4/raspberrypi.cpp > > +++ b/src/ipa/rpi/common/ipa_base.cpp > > @@ -1,60 +1,30 @@ > > [snip] > > > @@ -62,8 +32,7 @@ namespace libcamera { > > using namespace std::literals::chrono_literals; > > using utils::Duration; > > > > -/* Number of metadata objects available in the context list. */ > > -constexpr unsigned int numMetadataContexts = 16; > > +namespace { > > > > /* Number of frame length times to hold in the queue. */ > > constexpr unsigned int FrameLengthsQueueSize = 10; > > @@ -80,7 +49,7 @@ constexpr Duration defaultMaxFrameDuration = 250.0s; > > * we rate-limit the controller Prepare() and Process() calls to lower than or > > * equal to this rate. > > */ > > -constexpr Duration controllerMinFrameDuration = 1.0s / 30.0; > > +static constexpr Duration controllerMinFrameDuration = 1.0s / 30.0; > > Everything contained in an anonymous namespace becomes static. You can > drop the static keyword from other variables. > > > > > /* List of controls handled by the Raspberry Pi IPA */ > > static const ControlInfoMap::Map ipaControls{ > > @@ -116,118 +85,23 @@ static const ControlInfoMap::Map ipaAfControls{ > > { &controls::LensPosition, ControlInfo(0.0f, 32.0f, 1.0f) } > > }; > > > > +} /* namespace */ > > + > > [snip] > > > diff --git a/src/ipa/rpi/common/ipa_base.h b/src/ipa/rpi/common/ipa_base.h > > new file mode 100644 > > index 000000000000..b9fbf9414d63 > > --- /dev/null > > +++ b/src/ipa/rpi/common/ipa_base.h > > @@ -0,0 +1,125 @@ > > +/* SPDX-License-Identifier: BSD-2-Clause */ > > +/* > > + * Copyright (C) 2023, Raspberry Pi Ltd > > + * > > + * ipa_base.cpp - Raspberry Pi IPA base class > > ipa_base.h > > > + */ > > +#pragma once > > + > > +#include <array> > > +#include <deque> > > +#include <vector> > > You need <map> here, but can drop <vector> as it's only used to > implement the interface of the base class, and so is provided by > libcamera/ipa/ipa_interface.h. You also need stdint.h. > > > + > > #include <libcamera/base/span.h> > > > +#include <libcamera/base/utils.h> > > + > > +#include <libcamera/controls.h> > > + > > +#include <libcamera/ipa/ipa_interface.h> > > +#include <libcamera/ipa/ipa_module_info.h> > > Not needed. > > > +#include <libcamera/ipa/raspberrypi_ipa_interface.h> > > + > > +#include "libcamera/internal/mapped_framebuffer.h" > > + > > +#include "cam_helper/cam_helper.h" > > +#include "controller/agc_status.h" > > +#include "controller/camera_mode.h" > > +#include "controller/controller.h" > > +#include "controller/metadata.h" > > + > > +namespace libcamera { > > + > > +namespace ipa::RPi { > > + > > +class IpaBase : public IPARPiInterface > > +{ > > +public: > > + IpaBase(); > > + virtual ~IpaBase() = 0; > > That's a bit weird, why do you need a virtual abstract destructor ? This was a mistake, I'll remove it. > > > + > > + int32_t init(const IPASettings &settings, const InitParams ¶ms, InitResult *result) override; > > + int32_t configure(const IPACameraSensorInfo &sensorInfo, const ConfigParams ¶ms, > > + ConfigResult *result) override; > > + > > + void start(const ControlList &controls, StartResult *result) override; > > + void stop() override {} > > + > > + void mapBuffers(const std::vector<IPABuffer> &buffers) override; > > + void unmapBuffers(const std::vector<unsigned int> &ids) override; > > + > > + void prepareIsp(const PrepareParams ¶ms) override; > > + void processStats(const ProcessParams ¶ms) override; > > + > > + void reportMetadata(unsigned int ipaContext, ControlList *controls) override; > > + > > +private: > > + /* Number of metadata objects available in the context list. */ > > + static constexpr unsigned int numMetadataContexts = 16; > > + > > + virtual int32_t platformInit(const InitParams ¶ms, InitResult *result) = 0; > > This enforces a particular sequence of calls between the base class and > the derived class, dictated by the base class design. It's best to > handle this in the derived class instead. You can override the init() > function in the derived class, call the init() function of the base > class, and then perform all the work you're currently doing in > IpaVc4::platformInit(). One of the earlier iterations had exactly this implementing. After internal discussions, we decided to do it the other way round (i.e. the base class implements the init() override). Out of curiosity, what advantages would one way have over the other? > > The same comment applies to the other platform*() functions (some may > require a little bit of refactoring). The handleControls() function can > also be dropped by making applyControls() a protected virtual function > that derived classes may override. > > > + virtual int32_t platformConfigure(const ConfigParams ¶ms, ConfigResult *result) = 0; > > + > > + virtual void platformPrepareIsp(const PrepareParams ¶ms, > > + RPiController::Metadata &rpiMetadata) = 0; > > + virtual RPiController::StatisticsPtr platformProcessStats(Span<uint8_t> mem) = 0; > > + > > + void setMode(const IPACameraSensorInfo &sensorInfo); > > + void setCameraTimeoutValue(); > > + bool validateSensorControls(); > > + bool validateLensControls(); > > + void applyControls(const ControlList &controls); > > + virtual void handleControls(const ControlList &controls) = 0; > > + void fillDeviceStatus(const ControlList &sensorControls, unsigned int ipaContext); > > + void applyFrameDurations(utils::Duration minFrameDuration, utils::Duration maxFrameDuration); > > + void applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls); > > + > > + std::map<unsigned int, MappedFrameBuffer> buffers_; > > + > > + bool lensPresent_; > > + ControlList libcameraMetadata_; > > + > > + std::array<RPiController::Metadata, numMetadataContexts> rpiMetadata_; > > + > > + /* > > + * We count frames to decide if the frame must be hidden (e.g. from > > + * display) or mistrusted (i.e. not given to the control algos). > > + */ > > + uint64_t frameCount_; > > + > > + /* How many frames we should avoid running control algos on. */ > > + unsigned int mistrustCount_; > > + > > + /* Number of frames that need to be dropped on startup. */ > > + unsigned int dropFrameCount_; > > + > > + /* Frame timestamp for the last run of the controller. */ > > + uint64_t lastRunTimestamp_; > > + > > + /* Do we run a Controller::process() for this frame? */ > > + bool processPending_; > > + > > + /* Distinguish the first camera start from others. */ > > + bool firstStart_; > > + > > + /* Frame duration (1/fps) limits. */ > > + utils::Duration minFrameDuration_; > > + utils::Duration maxFrameDuration_; > > + > > +protected: > > We usually put protected members before private members. > > > + /* Raspberry Pi controller specific defines. */ > > + std::unique_ptr<RPiController::CamHelper> helper_; > > + RPiController::Controller controller_; > > + > > + ControlInfoMap sensorCtrls_; > > + ControlInfoMap lensCtrls_; > > + > > + /* Camera sensor params. */ > > + CameraMode mode_; > > + > > + /* Track the frame length times over FrameLengthsQueueSize frames. */ > > + std::deque<utils::Duration> frameLengths_; > > + utils::Duration lastTimeout_; > > +}; > > + > > +} /* namespace ipa::RPi */ > > + > > +} /* namespace libcamera */ > > diff --git a/src/ipa/rpi/common/meson.build b/src/ipa/rpi/common/meson.build > > new file mode 100644 > > index 000000000000..a7189f8389af > > --- /dev/null > > +++ b/src/ipa/rpi/common/meson.build > > @@ -0,0 +1,7 @@ > > +# SPDX-License-Identifier: CC0-1.0 > > + > > +# SPDX-License-Identifier: CC0-1.0 > > One SPDX header is enough :-) > > > + > > +rpi_ipa_common_sources = files([ > > + 'ipa_base.cpp', > > +]) > > diff --git a/src/ipa/rpi/vc4/meson.build b/src/ipa/rpi/vc4/meson.build > > index 992d0f1ab5a7..b581391586b3 100644 > > --- a/src/ipa/rpi/vc4/meson.build > > +++ b/src/ipa/rpi/vc4/meson.build > > @@ -13,11 +13,15 @@ vc4_ipa_includes = [ > > ] > > > > vc4_ipa_sources = files([ > > - 'raspberrypi.cpp', > > + 'vc4.cpp', > > ]) > > > > vc4_ipa_includes += include_directories('..') > > -vc4_ipa_sources += [rpi_ipa_cam_helper_sources, rpi_ipa_controller_sources] > > +vc4_ipa_sources += [ > > + rpi_ipa_cam_helper_sources, > > + rpi_ipa_common_sources, > > + rpi_ipa_controller_sources, > > Could we avoid compiling all these source files multiple times, once for > the IpaVc4 IPA module, and once for the other IPA modules that may be > implemented in the future ? This can be done with a static library, see > src/apps/common/meson.build and src/apps/cam/meson.build for instance. > > > +] > > > > mod = shared_module(ipa_name, > > [vc4_ipa_sources, libcamera_generated_ipa_headers], > Sure, I'll fix this up. Naush > [snip] > > -- > Regards, > > Laurent Pinchart
Hi Naush On Fri, Apr 28, 2023 at 09:35:51AM +0100, Naushir Patuck via libcamera-devel wrote: > Hi Laurent, > > Thank you for your feedback. > > On Thu, 27 Apr 2023 at 16:54, Laurent Pinchart > <laurent.pinchart@ideasonboard.com> wrote: > > > > Hi Naush, > > > > Thank you for the patch. > > > > On Wed, Apr 26, 2023 at 02:10:52PM +0100, Naushir Patuck via libcamera-devel wrote: > > > Create a new IpaBase class that handles general purpose housekeeping > > > duties for the Raspberry Pi IPA. Code the implementation of new class is > > > > Did you mean s/Code the implementation/The implementation/ and > > s/of new class/of the new class/ ? > > I actually meant "Code for the implementation...", but I'll replace it > with your wording. > > > > > > essentially pulled from the existing ipa/rpi/vc4/raspberrypi.cpp > > > file with a minimal amount of refactoring. > > > > > > Create a derived IpaVc4 class from IpaBase that handles the VC4 pipeline > > > specific tasks of the IPA. Again, code for this class implementation is > > > taken from the existing ipa/rpi/vc4/raspberrypi.cpp with a > > > minimal amount of refactoring. > > > > > > The goal of this change is to allow third parties to implement their own > > > IPA running on the Raspberry Pi without duplicating all of the IPA > > > housekeeping tasks. > > > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > > --- > > > src/ipa/meson.build | 5 +- > > > .../raspberrypi.cpp => common/ipa_base.cpp} | 1219 +++++------------ > > > src/ipa/rpi/common/ipa_base.h | 125 ++ > > > src/ipa/rpi/common/meson.build | 7 + > > > src/ipa/rpi/vc4/meson.build | 8 +- > > > src/ipa/rpi/vc4/vc4.cpp | 540 ++++++++ > > > 6 files changed, 1012 insertions(+), 892 deletions(-) > > > rename src/ipa/rpi/{vc4/raspberrypi.cpp => common/ipa_base.cpp} (65%) > > > create mode 100644 src/ipa/rpi/common/ipa_base.h > > > create mode 100644 src/ipa/rpi/common/meson.build > > > create mode 100644 src/ipa/rpi/vc4/vc4.cpp > > > > > > diff --git a/src/ipa/meson.build b/src/ipa/meson.build > > > index 10d3b44ca7b6..0c622c38a4a0 100644 > > > --- a/src/ipa/meson.build > > > +++ b/src/ipa/meson.build > > > @@ -37,13 +37,14 @@ endif > > > > > > enabled_ipa_modules = [] > > > > > > -# If the Raspberry Pi VC4 IPA is enabled, ensure we include the rpi/cam_helper > > > -# and rpi/controller subdirectories in the build. > > > +# If the Raspberry Pi VC4 IPA is enabled, ensure we include the cam_helper, > > > +# common and controller subdirectories in the build. > > > # > > > # This is done here and not within rpi/vc4/meson.build as meson does not > > > # allow the subdir command to traverse up the directory tree. > > > if pipelines.contains('rpi/vc4') > > > subdir('rpi/cam_helper') > > > + subdir('rpi/common') > > > subdir('rpi/controller') > > > endif > > > > > > diff --git a/src/ipa/rpi/vc4/raspberrypi.cpp b/src/ipa/rpi/common/ipa_base.cpp > > > similarity index 65% > > > rename from src/ipa/rpi/vc4/raspberrypi.cpp > > > rename to src/ipa/rpi/common/ipa_base.cpp > > > index e3ca8e2f7cbe..becada5973ad 100644 > > > --- a/src/ipa/rpi/vc4/raspberrypi.cpp > > > +++ b/src/ipa/rpi/common/ipa_base.cpp > > > @@ -1,60 +1,30 @@ > > > > [snip] > > > > > @@ -62,8 +32,7 @@ namespace libcamera { > > > using namespace std::literals::chrono_literals; > > > using utils::Duration; > > > > > > -/* Number of metadata objects available in the context list. */ > > > -constexpr unsigned int numMetadataContexts = 16; > > > +namespace { > > > > > > /* Number of frame length times to hold in the queue. */ > > > constexpr unsigned int FrameLengthsQueueSize = 10; > > > @@ -80,7 +49,7 @@ constexpr Duration defaultMaxFrameDuration = 250.0s; > > > * we rate-limit the controller Prepare() and Process() calls to lower than or > > > * equal to this rate. > > > */ > > > -constexpr Duration controllerMinFrameDuration = 1.0s / 30.0; > > > +static constexpr Duration controllerMinFrameDuration = 1.0s / 30.0; > > > > Everything contained in an anonymous namespace becomes static. You can > > drop the static keyword from other variables. > > > > > > > > /* List of controls handled by the Raspberry Pi IPA */ > > > static const ControlInfoMap::Map ipaControls{ > > > @@ -116,118 +85,23 @@ static const ControlInfoMap::Map ipaAfControls{ > > > { &controls::LensPosition, ControlInfo(0.0f, 32.0f, 1.0f) } > > > }; > > > > > > +} /* namespace */ > > > + > > > > [snip] > > > > > diff --git a/src/ipa/rpi/common/ipa_base.h b/src/ipa/rpi/common/ipa_base.h > > > new file mode 100644 > > > index 000000000000..b9fbf9414d63 > > > --- /dev/null > > > +++ b/src/ipa/rpi/common/ipa_base.h > > > @@ -0,0 +1,125 @@ > > > +/* SPDX-License-Identifier: BSD-2-Clause */ > > > +/* > > > + * Copyright (C) 2023, Raspberry Pi Ltd > > > + * > > > + * ipa_base.cpp - Raspberry Pi IPA base class > > > > ipa_base.h > > > > > + */ > > > +#pragma once > > > + > > > +#include <array> > > > +#include <deque> > > > +#include <vector> > > > > You need <map> here, but can drop <vector> as it's only used to > > implement the interface of the base class, and so is provided by > > libcamera/ipa/ipa_interface.h. You also need stdint.h. > > > > > + > > > > #include <libcamera/base/span.h> > > > > > +#include <libcamera/base/utils.h> > > > + > > > +#include <libcamera/controls.h> > > > + > > > +#include <libcamera/ipa/ipa_interface.h> > > > +#include <libcamera/ipa/ipa_module_info.h> > > > > Not needed. > > > > > +#include <libcamera/ipa/raspberrypi_ipa_interface.h> > > > + > > > +#include "libcamera/internal/mapped_framebuffer.h" > > > + > > > +#include "cam_helper/cam_helper.h" > > > +#include "controller/agc_status.h" > > > +#include "controller/camera_mode.h" > > > +#include "controller/controller.h" > > > +#include "controller/metadata.h" > > > + > > > +namespace libcamera { > > > + > > > +namespace ipa::RPi { > > > + > > > +class IpaBase : public IPARPiInterface > > > +{ > > > +public: > > > + IpaBase(); > > > + virtual ~IpaBase() = 0; > > > > That's a bit weird, why do you need a virtual abstract destructor ? > > This was a mistake, I'll remove it. > > > > > > + > > > + int32_t init(const IPASettings &settings, const InitParams ¶ms, InitResult *result) override; > > > + int32_t configure(const IPACameraSensorInfo &sensorInfo, const ConfigParams ¶ms, > > > + ConfigResult *result) override; > > > + > > > + void start(const ControlList &controls, StartResult *result) override; > > > + void stop() override {} > > > + > > > + void mapBuffers(const std::vector<IPABuffer> &buffers) override; > > > + void unmapBuffers(const std::vector<unsigned int> &ids) override; > > > + > > > + void prepareIsp(const PrepareParams ¶ms) override; > > > + void processStats(const ProcessParams ¶ms) override; > > > + > > > + void reportMetadata(unsigned int ipaContext, ControlList *controls) override; > > > + > > > +private: > > > + /* Number of metadata objects available in the context list. */ > > > + static constexpr unsigned int numMetadataContexts = 16; > > > + > > > + virtual int32_t platformInit(const InitParams ¶ms, InitResult *result) = 0; > > > > This enforces a particular sequence of calls between the base class and > > the derived class, dictated by the base class design. It's best to > > handle this in the derived class instead. You can override the init() > > function in the derived class, call the init() function of the base > > class, and then perform all the work you're currently doing in > > IpaVc4::platformInit(). > > One of the earlier iterations had exactly this implementing. After > internal discussions, we decided to do it the other way round (i.e. the base > class implements the init() override). Out of curiosity, what advantages would > one way have over the other? > I admit that it was my suggestion to pick this style as originally there was a mix of the two. Seeing an override calling into the base class' overridden method felt a bit weird. I admit I've researched (briefly) what was the suggested way to handle methods composition in C++ and found nothing relevant, so my thought was that it was better to clearly define what was the interface that derived classes need to implement, as the PipelineHandler/IPAInterface is implemented in the base classes. When it comes to the call order, it's the base class driving the procesing flow, while dispatching calls to the derived classes at the right time. Moving to your suggestion would allow derived classes to call the base class method, which means most of the control is moved to the derived classes with possible code duplication. I think to actually decide where to go, it is useful to analyze what class implements the operations flow control In one case Base::method() Derived::platformMethod() doSomeWork(); .... platformMethod() --> doPlaformWork(); ... finalizeWork(); While in the other Base::method() Derived::method() doSomeWork(); .... method() <-- Base::method() ... finalizeWork(); Is this an over-simplification ? Naush, do you expect the derived classes to control the operation flow, or will this be common to both and could be implemented in the base classes ? > > > > The same comment applies to the other platform*() functions (some may > > require a little bit of refactoring). The handleControls() function can > > also be dropped by making applyControls() a protected virtual function > > that derived classes may override. > > > > > + virtual int32_t platformConfigure(const ConfigParams ¶ms, ConfigResult *result) = 0; > > > + > > > + virtual void platformPrepareIsp(const PrepareParams ¶ms, > > > + RPiController::Metadata &rpiMetadata) = 0; > > > + virtual RPiController::StatisticsPtr platformProcessStats(Span<uint8_t> mem) = 0; > > > + > > > + void setMode(const IPACameraSensorInfo &sensorInfo); > > > + void setCameraTimeoutValue(); > > > + bool validateSensorControls(); > > > + bool validateLensControls(); > > > + void applyControls(const ControlList &controls); > > > + virtual void handleControls(const ControlList &controls) = 0; > > > + void fillDeviceStatus(const ControlList &sensorControls, unsigned int ipaContext); > > > + void applyFrameDurations(utils::Duration minFrameDuration, utils::Duration maxFrameDuration); > > > + void applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls); > > > + > > > + std::map<unsigned int, MappedFrameBuffer> buffers_; > > > + > > > + bool lensPresent_; > > > + ControlList libcameraMetadata_; > > > + > > > + std::array<RPiController::Metadata, numMetadataContexts> rpiMetadata_; > > > + > > > + /* > > > + * We count frames to decide if the frame must be hidden (e.g. from > > > + * display) or mistrusted (i.e. not given to the control algos). > > > + */ > > > + uint64_t frameCount_; > > > + > > > + /* How many frames we should avoid running control algos on. */ > > > + unsigned int mistrustCount_; > > > + > > > + /* Number of frames that need to be dropped on startup. */ > > > + unsigned int dropFrameCount_; > > > + > > > + /* Frame timestamp for the last run of the controller. */ > > > + uint64_t lastRunTimestamp_; > > > + > > > + /* Do we run a Controller::process() for this frame? */ > > > + bool processPending_; > > > + > > > + /* Distinguish the first camera start from others. */ > > > + bool firstStart_; > > > + > > > + /* Frame duration (1/fps) limits. */ > > > + utils::Duration minFrameDuration_; > > > + utils::Duration maxFrameDuration_; > > > + > > > +protected: > > > > We usually put protected members before private members. > > > > > + /* Raspberry Pi controller specific defines. */ > > > + std::unique_ptr<RPiController::CamHelper> helper_; > > > + RPiController::Controller controller_; > > > + > > > + ControlInfoMap sensorCtrls_; > > > + ControlInfoMap lensCtrls_; > > > + > > > + /* Camera sensor params. */ > > > + CameraMode mode_; > > > + > > > + /* Track the frame length times over FrameLengthsQueueSize frames. */ > > > + std::deque<utils::Duration> frameLengths_; > > > + utils::Duration lastTimeout_; > > > +}; > > > + > > > +} /* namespace ipa::RPi */ > > > + > > > +} /* namespace libcamera */ > > > diff --git a/src/ipa/rpi/common/meson.build b/src/ipa/rpi/common/meson.build > > > new file mode 100644 > > > index 000000000000..a7189f8389af > > > --- /dev/null > > > +++ b/src/ipa/rpi/common/meson.build > > > @@ -0,0 +1,7 @@ > > > +# SPDX-License-Identifier: CC0-1.0 > > > + > > > +# SPDX-License-Identifier: CC0-1.0 > > > > One SPDX header is enough :-) > > > > > + > > > +rpi_ipa_common_sources = files([ > > > + 'ipa_base.cpp', > > > +]) > > > diff --git a/src/ipa/rpi/vc4/meson.build b/src/ipa/rpi/vc4/meson.build > > > index 992d0f1ab5a7..b581391586b3 100644 > > > --- a/src/ipa/rpi/vc4/meson.build > > > +++ b/src/ipa/rpi/vc4/meson.build > > > @@ -13,11 +13,15 @@ vc4_ipa_includes = [ > > > ] > > > > > > vc4_ipa_sources = files([ > > > - 'raspberrypi.cpp', > > > + 'vc4.cpp', > > > ]) > > > > > > vc4_ipa_includes += include_directories('..') > > > -vc4_ipa_sources += [rpi_ipa_cam_helper_sources, rpi_ipa_controller_sources] > > > +vc4_ipa_sources += [ > > > + rpi_ipa_cam_helper_sources, > > > + rpi_ipa_common_sources, > > > + rpi_ipa_controller_sources, > > > > Could we avoid compiling all these source files multiple times, once for > > the IpaVc4 IPA module, and once for the other IPA modules that may be > > implemented in the future ? This can be done with a static library, see > > src/apps/common/meson.build and src/apps/cam/meson.build for instance. > > > > > +] > > > > > > mod = shared_module(ipa_name, > > > [vc4_ipa_sources, libcamera_generated_ipa_headers], > > > > Sure, I'll fix this up. > > Naush > > > [snip] > > > > -- > > Regards, > > > > Laurent Pinchart
Hi Jacopo, On Fri, 28 Apr 2023 at 10:33, Jacopo Mondi <jacopo.mondi@ideasonboard.com> wrote: > > Hi Naush > > On Fri, Apr 28, 2023 at 09:35:51AM +0100, Naushir Patuck via libcamera-devel wrote: > > Hi Laurent, > > > > Thank you for your feedback. > > > > On Thu, 27 Apr 2023 at 16:54, Laurent Pinchart > > <laurent.pinchart@ideasonboard.com> wrote: > > > > > > Hi Naush, > > > > > > Thank you for the patch. > > > > > > On Wed, Apr 26, 2023 at 02:10:52PM +0100, Naushir Patuck via libcamera-devel wrote: > > > > Create a new IpaBase class that handles general purpose housekeeping > > > > duties for the Raspberry Pi IPA. Code the implementation of new class is > > > > > > Did you mean s/Code the implementation/The implementation/ and > > > s/of new class/of the new class/ ? > > > > I actually meant "Code for the implementation...", but I'll replace it > > with your wording. > > > > > > > > > essentially pulled from the existing ipa/rpi/vc4/raspberrypi.cpp > > > > file with a minimal amount of refactoring. > > > > > > > > Create a derived IpaVc4 class from IpaBase that handles the VC4 pipeline > > > > specific tasks of the IPA. Again, code for this class implementation is > > > > taken from the existing ipa/rpi/vc4/raspberrypi.cpp with a > > > > minimal amount of refactoring. > > > > > > > > The goal of this change is to allow third parties to implement their own > > > > IPA running on the Raspberry Pi without duplicating all of the IPA > > > > housekeeping tasks. > > > > > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > > > --- > > > > src/ipa/meson.build | 5 +- > > > > .../raspberrypi.cpp => common/ipa_base.cpp} | 1219 +++++------------ > > > > src/ipa/rpi/common/ipa_base.h | 125 ++ > > > > src/ipa/rpi/common/meson.build | 7 + > > > > src/ipa/rpi/vc4/meson.build | 8 +- > > > > src/ipa/rpi/vc4/vc4.cpp | 540 ++++++++ > > > > 6 files changed, 1012 insertions(+), 892 deletions(-) > > > > rename src/ipa/rpi/{vc4/raspberrypi.cpp => common/ipa_base.cpp} (65%) > > > > create mode 100644 src/ipa/rpi/common/ipa_base.h > > > > create mode 100644 src/ipa/rpi/common/meson.build > > > > create mode 100644 src/ipa/rpi/vc4/vc4.cpp > > > > > > > > diff --git a/src/ipa/meson.build b/src/ipa/meson.build > > > > index 10d3b44ca7b6..0c622c38a4a0 100644 > > > > --- a/src/ipa/meson.build > > > > +++ b/src/ipa/meson.build > > > > @@ -37,13 +37,14 @@ endif > > > > > > > > enabled_ipa_modules = [] > > > > > > > > -# If the Raspberry Pi VC4 IPA is enabled, ensure we include the rpi/cam_helper > > > > -# and rpi/controller subdirectories in the build. > > > > +# If the Raspberry Pi VC4 IPA is enabled, ensure we include the cam_helper, > > > > +# common and controller subdirectories in the build. > > > > # > > > > # This is done here and not within rpi/vc4/meson.build as meson does not > > > > # allow the subdir command to traverse up the directory tree. > > > > if pipelines.contains('rpi/vc4') > > > > subdir('rpi/cam_helper') > > > > + subdir('rpi/common') > > > > subdir('rpi/controller') > > > > endif > > > > > > > > diff --git a/src/ipa/rpi/vc4/raspberrypi.cpp b/src/ipa/rpi/common/ipa_base.cpp > > > > similarity index 65% > > > > rename from src/ipa/rpi/vc4/raspberrypi.cpp > > > > rename to src/ipa/rpi/common/ipa_base.cpp > > > > index e3ca8e2f7cbe..becada5973ad 100644 > > > > --- a/src/ipa/rpi/vc4/raspberrypi.cpp > > > > +++ b/src/ipa/rpi/common/ipa_base.cpp > > > > @@ -1,60 +1,30 @@ > > > > > > [snip] > > > > > > > @@ -62,8 +32,7 @@ namespace libcamera { > > > > using namespace std::literals::chrono_literals; > > > > using utils::Duration; > > > > > > > > -/* Number of metadata objects available in the context list. */ > > > > -constexpr unsigned int numMetadataContexts = 16; > > > > +namespace { > > > > > > > > /* Number of frame length times to hold in the queue. */ > > > > constexpr unsigned int FrameLengthsQueueSize = 10; > > > > @@ -80,7 +49,7 @@ constexpr Duration defaultMaxFrameDuration = 250.0s; > > > > * we rate-limit the controller Prepare() and Process() calls to lower than or > > > > * equal to this rate. > > > > */ > > > > -constexpr Duration controllerMinFrameDuration = 1.0s / 30.0; > > > > +static constexpr Duration controllerMinFrameDuration = 1.0s / 30.0; > > > > > > Everything contained in an anonymous namespace becomes static. You can > > > drop the static keyword from other variables. > > > > > > > > > > > /* List of controls handled by the Raspberry Pi IPA */ > > > > static const ControlInfoMap::Map ipaControls{ > > > > @@ -116,118 +85,23 @@ static const ControlInfoMap::Map ipaAfControls{ > > > > { &controls::LensPosition, ControlInfo(0.0f, 32.0f, 1.0f) } > > > > }; > > > > > > > > +} /* namespace */ > > > > + > > > > > > [snip] > > > > > > > diff --git a/src/ipa/rpi/common/ipa_base.h b/src/ipa/rpi/common/ipa_base.h > > > > new file mode 100644 > > > > index 000000000000..b9fbf9414d63 > > > > --- /dev/null > > > > +++ b/src/ipa/rpi/common/ipa_base.h > > > > @@ -0,0 +1,125 @@ > > > > +/* SPDX-License-Identifier: BSD-2-Clause */ > > > > +/* > > > > + * Copyright (C) 2023, Raspberry Pi Ltd > > > > + * > > > > + * ipa_base.cpp - Raspberry Pi IPA base class > > > > > > ipa_base.h > > > > > > > + */ > > > > +#pragma once > > > > + > > > > +#include <array> > > > > +#include <deque> > > > > +#include <vector> > > > > > > You need <map> here, but can drop <vector> as it's only used to > > > implement the interface of the base class, and so is provided by > > > libcamera/ipa/ipa_interface.h. You also need stdint.h. > > > > > > > + > > > > > > #include <libcamera/base/span.h> > > > > > > > +#include <libcamera/base/utils.h> > > > > + > > > > +#include <libcamera/controls.h> > > > > + > > > > +#include <libcamera/ipa/ipa_interface.h> > > > > +#include <libcamera/ipa/ipa_module_info.h> > > > > > > Not needed. > > > > > > > +#include <libcamera/ipa/raspberrypi_ipa_interface.h> > > > > + > > > > +#include "libcamera/internal/mapped_framebuffer.h" > > > > + > > > > +#include "cam_helper/cam_helper.h" > > > > +#include "controller/agc_status.h" > > > > +#include "controller/camera_mode.h" > > > > +#include "controller/controller.h" > > > > +#include "controller/metadata.h" > > > > + > > > > +namespace libcamera { > > > > + > > > > +namespace ipa::RPi { > > > > + > > > > +class IpaBase : public IPARPiInterface > > > > +{ > > > > +public: > > > > + IpaBase(); > > > > + virtual ~IpaBase() = 0; > > > > > > That's a bit weird, why do you need a virtual abstract destructor ? > > > > This was a mistake, I'll remove it. > > > > > > > > > + > > > > + int32_t init(const IPASettings &settings, const InitParams ¶ms, InitResult *result) override; > > > > + int32_t configure(const IPACameraSensorInfo &sensorInfo, const ConfigParams ¶ms, > > > > + ConfigResult *result) override; > > > > + > > > > + void start(const ControlList &controls, StartResult *result) override; > > > > + void stop() override {} > > > > + > > > > + void mapBuffers(const std::vector<IPABuffer> &buffers) override; > > > > + void unmapBuffers(const std::vector<unsigned int> &ids) override; > > > > + > > > > + void prepareIsp(const PrepareParams ¶ms) override; > > > > + void processStats(const ProcessParams ¶ms) override; > > > > + > > > > + void reportMetadata(unsigned int ipaContext, ControlList *controls) override; > > > > + > > > > +private: > > > > + /* Number of metadata objects available in the context list. */ > > > > + static constexpr unsigned int numMetadataContexts = 16; > > > > + > > > > + virtual int32_t platformInit(const InitParams ¶ms, InitResult *result) = 0; > > > > > > This enforces a particular sequence of calls between the base class and > > > the derived class, dictated by the base class design. It's best to > > > handle this in the derived class instead. You can override the init() > > > function in the derived class, call the init() function of the base > > > class, and then perform all the work you're currently doing in > > > IpaVc4::platformInit(). > > > > One of the earlier iterations had exactly this implementing. After > > internal discussions, we decided to do it the other way round (i.e. the base > > class implements the init() override). Out of curiosity, what advantages would > > one way have over the other? > > > > I admit that it was my suggestion to pick this style as originally > there was a mix of the two. Seeing an override calling into the base > class' overridden method felt a bit weird. I admit I've researched > (briefly) what was the suggested way to handle methods composition in > C++ and found nothing relevant, so my thought was that it was better > to clearly define what was the interface that derived classes need to > implement, as the PipelineHandler/IPAInterface is implemented in the > base classes. > > When it comes to the call order, it's the base class driving the > procesing flow, while dispatching calls to the derived classes at the > right time. Moving to your suggestion would allow derived classes to > call the base class method, which means most of the control is moved > to the derived classes with possible code duplication. > > I think to actually decide where to go, it is useful to analyze what > class implements the operations flow control > > In one case > > Base::method() Derived::platformMethod() > > doSomeWork(); > .... > platformMethod() --> doPlaformWork(); > ... > finalizeWork(); > > While in the other > > Base::method() Derived::method() > > doSomeWork(); > .... > method() <-- Base::method() > ... > finalizeWork(); > > Is this an over-simplification ? > > Naush, do you expect the derived classes to control the operation > flow, or will this be common to both and could be implemented in the > base classes ? No, I see no explicit need for operation flow to be dictated by the derived class. Hence changing things around to your suggestion and making things more consistent. Regards, Naush > > > > > > > The same comment applies to the other platform*() functions (some may > > > require a little bit of refactoring). The handleControls() function can > > > also be dropped by making applyControls() a protected virtual function > > > that derived classes may override. > > > > > > > + virtual int32_t platformConfigure(const ConfigParams ¶ms, ConfigResult *result) = 0; > > > > + > > > > + virtual void platformPrepareIsp(const PrepareParams ¶ms, > > > > + RPiController::Metadata &rpiMetadata) = 0; > > > > + virtual RPiController::StatisticsPtr platformProcessStats(Span<uint8_t> mem) = 0; > > > > + > > > > + void setMode(const IPACameraSensorInfo &sensorInfo); > > > > + void setCameraTimeoutValue(); > > > > + bool validateSensorControls(); > > > > + bool validateLensControls(); > > > > + void applyControls(const ControlList &controls); > > > > + virtual void handleControls(const ControlList &controls) = 0; > > > > + void fillDeviceStatus(const ControlList &sensorControls, unsigned int ipaContext); > > > > + void applyFrameDurations(utils::Duration minFrameDuration, utils::Duration maxFrameDuration); > > > > + void applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls); > > > > + > > > > + std::map<unsigned int, MappedFrameBuffer> buffers_; > > > > + > > > > + bool lensPresent_; > > > > + ControlList libcameraMetadata_; > > > > + > > > > + std::array<RPiController::Metadata, numMetadataContexts> rpiMetadata_; > > > > + > > > > + /* > > > > + * We count frames to decide if the frame must be hidden (e.g. from > > > > + * display) or mistrusted (i.e. not given to the control algos). > > > > + */ > > > > + uint64_t frameCount_; > > > > + > > > > + /* How many frames we should avoid running control algos on. */ > > > > + unsigned int mistrustCount_; > > > > + > > > > + /* Number of frames that need to be dropped on startup. */ > > > > + unsigned int dropFrameCount_; > > > > + > > > > + /* Frame timestamp for the last run of the controller. */ > > > > + uint64_t lastRunTimestamp_; > > > > + > > > > + /* Do we run a Controller::process() for this frame? */ > > > > + bool processPending_; > > > > + > > > > + /* Distinguish the first camera start from others. */ > > > > + bool firstStart_; > > > > + > > > > + /* Frame duration (1/fps) limits. */ > > > > + utils::Duration minFrameDuration_; > > > > + utils::Duration maxFrameDuration_; > > > > + > > > > +protected: > > > > > > We usually put protected members before private members. > > > > > > > + /* Raspberry Pi controller specific defines. */ > > > > + std::unique_ptr<RPiController::CamHelper> helper_; > > > > + RPiController::Controller controller_; > > > > + > > > > + ControlInfoMap sensorCtrls_; > > > > + ControlInfoMap lensCtrls_; > > > > + > > > > + /* Camera sensor params. */ > > > > + CameraMode mode_; > > > > + > > > > + /* Track the frame length times over FrameLengthsQueueSize frames. */ > > > > + std::deque<utils::Duration> frameLengths_; > > > > + utils::Duration lastTimeout_; > > > > +}; > > > > + > > > > +} /* namespace ipa::RPi */ > > > > + > > > > +} /* namespace libcamera */ > > > > diff --git a/src/ipa/rpi/common/meson.build b/src/ipa/rpi/common/meson.build > > > > new file mode 100644 > > > > index 000000000000..a7189f8389af > > > > --- /dev/null > > > > +++ b/src/ipa/rpi/common/meson.build > > > > @@ -0,0 +1,7 @@ > > > > +# SPDX-License-Identifier: CC0-1.0 > > > > + > > > > +# SPDX-License-Identifier: CC0-1.0 > > > > > > One SPDX header is enough :-) > > > > > > > + > > > > +rpi_ipa_common_sources = files([ > > > > + 'ipa_base.cpp', > > > > +]) > > > > diff --git a/src/ipa/rpi/vc4/meson.build b/src/ipa/rpi/vc4/meson.build > > > > index 992d0f1ab5a7..b581391586b3 100644 > > > > --- a/src/ipa/rpi/vc4/meson.build > > > > +++ b/src/ipa/rpi/vc4/meson.build > > > > @@ -13,11 +13,15 @@ vc4_ipa_includes = [ > > > > ] > > > > > > > > vc4_ipa_sources = files([ > > > > - 'raspberrypi.cpp', > > > > + 'vc4.cpp', > > > > ]) > > > > > > > > vc4_ipa_includes += include_directories('..') > > > > -vc4_ipa_sources += [rpi_ipa_cam_helper_sources, rpi_ipa_controller_sources] > > > > +vc4_ipa_sources += [ > > > > + rpi_ipa_cam_helper_sources, > > > > + rpi_ipa_common_sources, > > > > + rpi_ipa_controller_sources, > > > > > > Could we avoid compiling all these source files multiple times, once for > > > the IpaVc4 IPA module, and once for the other IPA modules that may be > > > implemented in the future ? This can be done with a static library, see > > > src/apps/common/meson.build and src/apps/cam/meson.build for instance. > > > > > > > +] > > > > > > > > mod = shared_module(ipa_name, > > > > [vc4_ipa_sources, libcamera_generated_ipa_headers], > > > > > > > Sure, I'll fix this up. > > > > Naush > > > > > [snip] > > > > > > -- > > > Regards, > > > > > > Laurent Pinchart
Hi Laurent and Jacopo, On Fri, 28 Apr 2023 at 14:57, Naushir Patuck <naush@raspberrypi.com> wrote: > > Hi Jacopo, > <snip> > > > > When it comes to the call order, it's the base class driving the > > procesing flow, while dispatching calls to the derived classes at the > > right time. Moving to your suggestion would allow derived classes to > > call the base class method, which means most of the control is moved > > to the derived classes with possible code duplication. > > > > I think to actually decide where to go, it is useful to analyze what > > class implements the operations flow control > > > > In one case > > > > Base::method() Derived::platformMethod() > > > > doSomeWork(); > > .... > > platformMethod() --> doPlaformWork(); > > ... > > finalizeWork(); > > > > While in the other > > > > Base::method() Derived::method() > > > > doSomeWork(); > > .... > > method() <-- Base::method() > > ... > > finalizeWork(); > > > > Is this an over-simplification ? > > > > Naush, do you expect the derived classes to control the operation > > flow, or will this be common to both and could be implemented in the > > base classes ? > > No, I see no explicit need for operation flow to be dictated by the derived > class. Hence changing things around to your suggestion and making things more > consistent. Having looked further into this, I'm a bit more conflicted. I initially thought that I could simply revert to the original mechanism of having the control flow start from the derived class and call into the base class, as the 2nd case drawn above. This does work neatly for the ipa::init() and ipa::configure(). However platformProcessStats() and platformPrepareIsp() require the control flow from base class to derived class, as in the 1st case drawn above. I don't think it's sensible to choose flow 1 for some functions and flow 2 for others. So that suggests to me that flow 1 (i.e. the existing flow) is probably the right approach for the IPA changes. Do folks think that makes sense? Again, I am happy to change things if it seems more appropriate. Regards, Naush
Hi Naush, On Tue, May 02, 2023 at 03:54:16PM +0100, Naushir Patuck wrote: > Hi Laurent and Jacopo, > > On Fri, 28 Apr 2023 at 14:57, Naushir Patuck wrote: > > > > Hi Jacopo, > > <snip> > > > > When it comes to the call order, it's the base class driving the > > > procesing flow, while dispatching calls to the derived classes at the > > > right time. Moving to your suggestion would allow derived classes to > > > call the base class method, which means most of the control is moved > > > to the derived classes with possible code duplication. > > > > > > I think to actually decide where to go, it is useful to analyze what > > > class implements the operations flow control > > > > > > In one case > > > > > > Base::method() Derived::platformMethod() > > > > > > doSomeWork(); > > > .... > > > platformMethod() --> doPlaformWork(); > > > ... > > > finalizeWork(); > > > > > > While in the other > > > > > > Base::method() Derived::method() > > > > > > doSomeWork(); > > > .... > > > method() <-- Base::method() > > > ... > > > finalizeWork(); > > > > > > Is this an over-simplification ? > > > > > > Naush, do you expect the derived classes to control the operation > > > flow, or will this be common to both and could be implemented in the > > > base classes ? > > > > No, I see no explicit need for operation flow to be dictated by the derived > > class. Hence changing things around to your suggestion and making things more > > consistent. > > Having looked further into this, I'm a bit more conflicted. I initially thought > that I could simply revert to the original mechanism of having the control flow > start from the derived class and call into the base class, as the 2nd case > drawn above. This does work neatly for the ipa::init() and ipa::configure(). > However platformProcessStats() and platformPrepareIsp() require the control flow > from base class to derived class, as in the 1st case drawn above. > > I don't think it's sensible to choose flow 1 for some functions and flow 2 for > others. So that suggests to me that flow 1 (i.e. the existing flow) is probably > the right approach for the IPA changes. Do folks think that makes sense? > Again, I am happy to change things if it seems more appropriate. As mentioned in a different conversation, I'm fine keeping the existing code flow, even if I think it would be best to reverse it. We can improve this incrementally on top.
Hi Laurent, On Tue, 2 May 2023 at 16:16, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Naush, > > On Tue, May 02, 2023 at 03:54:16PM +0100, Naushir Patuck wrote: > > Hi Laurent and Jacopo, > > > > On Fri, 28 Apr 2023 at 14:57, Naushir Patuck wrote: > > > > > > Hi Jacopo, > > > > <snip> > > > > > > When it comes to the call order, it's the base class driving the > > > > procesing flow, while dispatching calls to the derived classes at the > > > > right time. Moving to your suggestion would allow derived classes to > > > > call the base class method, which means most of the control is moved > > > > to the derived classes with possible code duplication. > > > > > > > > I think to actually decide where to go, it is useful to analyze what > > > > class implements the operations flow control > > > > > > > > In one case > > > > > > > > Base::method() Derived::platformMethod() > > > > > > > > doSomeWork(); > > > > .... > > > > platformMethod() --> doPlaformWork(); > > > > ... > > > > finalizeWork(); > > > > > > > > While in the other > > > > > > > > Base::method() Derived::method() > > > > > > > > doSomeWork(); > > > > .... > > > > method() <-- Base::method() > > > > ... > > > > finalizeWork(); > > > > > > > > Is this an over-simplification ? > > > > > > > > Naush, do you expect the derived classes to control the operation > > > > flow, or will this be common to both and could be implemented in the > > > > base classes ? > > > > > > No, I see no explicit need for operation flow to be dictated by the derived > > > class. Hence changing things around to your suggestion and making things more > > > consistent. > > > > Having looked further into this, I'm a bit more conflicted. I initially thought > > that I could simply revert to the original mechanism of having the control flow > > start from the derived class and call into the base class, as the 2nd case > > drawn above. This does work neatly for the ipa::init() and ipa::configure(). > > However platformProcessStats() and platformPrepareIsp() require the control flow > > from base class to derived class, as in the 1st case drawn above. > > > > I don't think it's sensible to choose flow 1 for some functions and flow 2 for > > others. So that suggests to me that flow 1 (i.e. the existing flow) is probably > > the right approach for the IPA changes. Do folks think that makes sense? > > Again, I am happy to change things if it seems more appropriate. > > As mentioned in a different conversation, I'm fine keeping the existing > code flow, even if I think it would be best to reverse it. We can > improve this incrementally on top. Agree. I'll keep the existing flow and change if needed in the future. Regards, Naush > > -- > Regards, > > Laurent Pinchart
Hello, On Fri, Apr 28, 2023 at 11:33:14AM +0200, Jacopo Mondi wrote: > On Fri, Apr 28, 2023 at 09:35:51AM +0100, Naushir Patuck via libcamera-devel wrote: > > On Thu, 27 Apr 2023 at 16:54, Laurent Pinchart wrote: > > > On Wed, Apr 26, 2023 at 02:10:52PM +0100, Naushir Patuck via libcamera-devel wrote: > > > > Create a new IpaBase class that handles general purpose housekeeping > > > > duties for the Raspberry Pi IPA. Code the implementation of new class is > > > > > > Did you mean s/Code the implementation/The implementation/ and > > > s/of new class/of the new class/ ? > > > > I actually meant "Code for the implementation...", but I'll replace it > > with your wording. > > > > > > essentially pulled from the existing ipa/rpi/vc4/raspberrypi.cpp > > > > file with a minimal amount of refactoring. > > > > > > > > Create a derived IpaVc4 class from IpaBase that handles the VC4 pipeline > > > > specific tasks of the IPA. Again, code for this class implementation is > > > > taken from the existing ipa/rpi/vc4/raspberrypi.cpp with a > > > > minimal amount of refactoring. > > > > > > > > The goal of this change is to allow third parties to implement their own > > > > IPA running on the Raspberry Pi without duplicating all of the IPA > > > > housekeeping tasks. > > > > > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > > > --- > > > > src/ipa/meson.build | 5 +- > > > > .../raspberrypi.cpp => common/ipa_base.cpp} | 1219 +++++------------ > > > > src/ipa/rpi/common/ipa_base.h | 125 ++ > > > > src/ipa/rpi/common/meson.build | 7 + > > > > src/ipa/rpi/vc4/meson.build | 8 +- > > > > src/ipa/rpi/vc4/vc4.cpp | 540 ++++++++ > > > > 6 files changed, 1012 insertions(+), 892 deletions(-) > > > > rename src/ipa/rpi/{vc4/raspberrypi.cpp => common/ipa_base.cpp} (65%) > > > > create mode 100644 src/ipa/rpi/common/ipa_base.h > > > > create mode 100644 src/ipa/rpi/common/meson.build > > > > create mode 100644 src/ipa/rpi/vc4/vc4.cpp > > > > > > > > diff --git a/src/ipa/meson.build b/src/ipa/meson.build > > > > index 10d3b44ca7b6..0c622c38a4a0 100644 > > > > --- a/src/ipa/meson.build > > > > +++ b/src/ipa/meson.build > > > > @@ -37,13 +37,14 @@ endif > > > > > > > > enabled_ipa_modules = [] > > > > > > > > -# If the Raspberry Pi VC4 IPA is enabled, ensure we include the rpi/cam_helper > > > > -# and rpi/controller subdirectories in the build. > > > > +# If the Raspberry Pi VC4 IPA is enabled, ensure we include the cam_helper, > > > > +# common and controller subdirectories in the build. > > > > # > > > > # This is done here and not within rpi/vc4/meson.build as meson does not > > > > # allow the subdir command to traverse up the directory tree. > > > > if pipelines.contains('rpi/vc4') > > > > subdir('rpi/cam_helper') > > > > + subdir('rpi/common') > > > > subdir('rpi/controller') > > > > endif > > > > > > > > diff --git a/src/ipa/rpi/vc4/raspberrypi.cpp b/src/ipa/rpi/common/ipa_base.cpp > > > > similarity index 65% > > > > rename from src/ipa/rpi/vc4/raspberrypi.cpp > > > > rename to src/ipa/rpi/common/ipa_base.cpp > > > > index e3ca8e2f7cbe..becada5973ad 100644 > > > > --- a/src/ipa/rpi/vc4/raspberrypi.cpp > > > > +++ b/src/ipa/rpi/common/ipa_base.cpp > > > > @@ -1,60 +1,30 @@ > > > > > > [snip] > > > > > > > @@ -62,8 +32,7 @@ namespace libcamera { > > > > using namespace std::literals::chrono_literals; > > > > using utils::Duration; > > > > > > > > -/* Number of metadata objects available in the context list. */ > > > > -constexpr unsigned int numMetadataContexts = 16; > > > > +namespace { > > > > > > > > /* Number of frame length times to hold in the queue. */ > > > > constexpr unsigned int FrameLengthsQueueSize = 10; > > > > @@ -80,7 +49,7 @@ constexpr Duration defaultMaxFrameDuration = 250.0s; > > > > * we rate-limit the controller Prepare() and Process() calls to lower than or > > > > * equal to this rate. > > > > */ > > > > -constexpr Duration controllerMinFrameDuration = 1.0s / 30.0; > > > > +static constexpr Duration controllerMinFrameDuration = 1.0s / 30.0; > > > > > > Everything contained in an anonymous namespace becomes static. You can > > > drop the static keyword from other variables. > > > > > > > > > > > /* List of controls handled by the Raspberry Pi IPA */ > > > > static const ControlInfoMap::Map ipaControls{ > > > > @@ -116,118 +85,23 @@ static const ControlInfoMap::Map ipaAfControls{ > > > > { &controls::LensPosition, ControlInfo(0.0f, 32.0f, 1.0f) } > > > > }; > > > > > > > > +} /* namespace */ > > > > + > > > > > > [snip] > > > > > > > diff --git a/src/ipa/rpi/common/ipa_base.h b/src/ipa/rpi/common/ipa_base.h > > > > new file mode 100644 > > > > index 000000000000..b9fbf9414d63 > > > > --- /dev/null > > > > +++ b/src/ipa/rpi/common/ipa_base.h > > > > @@ -0,0 +1,125 @@ > > > > +/* SPDX-License-Identifier: BSD-2-Clause */ > > > > +/* > > > > + * Copyright (C) 2023, Raspberry Pi Ltd > > > > + * > > > > + * ipa_base.cpp - Raspberry Pi IPA base class > > > > > > ipa_base.h > > > > > > > + */ > > > > +#pragma once > > > > + > > > > +#include <array> > > > > +#include <deque> > > > > +#include <vector> > > > > > > You need <map> here, but can drop <vector> as it's only used to > > > implement the interface of the base class, and so is provided by > > > libcamera/ipa/ipa_interface.h. You also need stdint.h. > > > > > > > + > > > > > > #include <libcamera/base/span.h> > > > > > > > +#include <libcamera/base/utils.h> > > > > + > > > > +#include <libcamera/controls.h> > > > > + > > > > +#include <libcamera/ipa/ipa_interface.h> > > > > +#include <libcamera/ipa/ipa_module_info.h> > > > > > > Not needed. > > > > > > > +#include <libcamera/ipa/raspberrypi_ipa_interface.h> > > > > + > > > > +#include "libcamera/internal/mapped_framebuffer.h" > > > > + > > > > +#include "cam_helper/cam_helper.h" > > > > +#include "controller/agc_status.h" > > > > +#include "controller/camera_mode.h" > > > > +#include "controller/controller.h" > > > > +#include "controller/metadata.h" > > > > + > > > > +namespace libcamera { > > > > + > > > > +namespace ipa::RPi { > > > > + > > > > +class IpaBase : public IPARPiInterface > > > > +{ > > > > +public: > > > > + IpaBase(); > > > > + virtual ~IpaBase() = 0; > > > > > > That's a bit weird, why do you need a virtual abstract destructor ? > > > > This was a mistake, I'll remove it. > > > > > > + > > > > + int32_t init(const IPASettings &settings, const InitParams ¶ms, InitResult *result) override; > > > > + int32_t configure(const IPACameraSensorInfo &sensorInfo, const ConfigParams ¶ms, > > > > + ConfigResult *result) override; > > > > + > > > > + void start(const ControlList &controls, StartResult *result) override; > > > > + void stop() override {} > > > > + > > > > + void mapBuffers(const std::vector<IPABuffer> &buffers) override; > > > > + void unmapBuffers(const std::vector<unsigned int> &ids) override; > > > > + > > > > + void prepareIsp(const PrepareParams ¶ms) override; > > > > + void processStats(const ProcessParams ¶ms) override; > > > > + > > > > + void reportMetadata(unsigned int ipaContext, ControlList *controls) override; > > > > + > > > > +private: > > > > + /* Number of metadata objects available in the context list. */ > > > > + static constexpr unsigned int numMetadataContexts = 16; > > > > + > > > > + virtual int32_t platformInit(const InitParams ¶ms, InitResult *result) = 0; > > > > > > This enforces a particular sequence of calls between the base class and > > > the derived class, dictated by the base class design. It's best to > > > handle this in the derived class instead. You can override the init() > > > function in the derived class, call the init() function of the base > > > class, and then perform all the work you're currently doing in > > > IpaVc4::platformInit(). > > > > One of the earlier iterations had exactly this implementing. After > > internal discussions, we decided to do it the other way round (i.e. the base > > class implements the init() override). Out of curiosity, what advantages would > > one way have over the other? > > I admit that it was my suggestion to pick this style as originally > there was a mix of the two. Seeing an override calling into the base > class' overridden method felt a bit weird. That's a normal pattern in C++, base classes shouldn't, when possible, imply a particular implementation of derived classes. Calling the base class implementation at the beginning or end of a derived class' function is perfectly fine and normal. The other pattern, calling into a protected virtual function implemented by the derived class, is an acceptable use case too. > I admit I've researched > (briefly) what was the suggested way to handle methods composition in Note that composition and inheritance are two different things. > C++ and found nothing relevant, so my thought was that it was better > to clearly define what was the interface that derived classes need to > implement, as the PipelineHandler/IPAInterface is implemented in the > base classes. > > When it comes to the call order, it's the base class driving the > procesing flow, while dispatching calls to the derived classes at the > right time. Moving to your suggestion would allow derived classes to > call the base class method, which means most of the control is moved > to the derived classes with possible code duplication. I think having the derived class in control is desirable, as only the derived class knows about its specific needs. Otherwise you could have to over-complicate the design of the base class to cover the needs of different derived classes. > I think to actually decide where to go, it is useful to analyze what > class implements the operations flow control > > In one case > > Base::method() Derived::platformMethod() > > doSomeWork(); > .... > platformMethod() --> doPlaformWork(); > ... > finalizeWork(); > > While in the other > > Base::method() Derived::method() > > doSomeWork(); > .... > method() <-- Base::method() > ... > finalizeWork(); > > Is this an over-simplification ? > > Naush, do you expect the derived classes to control the operation > flow, or will this be common to both and could be implemented in the > base classes ? > > > > The same comment applies to the other platform*() functions (some may > > > require a little bit of refactoring). The handleControls() function can > > > also be dropped by making applyControls() a protected virtual function > > > that derived classes may override. > > > > > > > + virtual int32_t platformConfigure(const ConfigParams ¶ms, ConfigResult *result) = 0; > > > > + > > > > + virtual void platformPrepareIsp(const PrepareParams ¶ms, > > > > + RPiController::Metadata &rpiMetadata) = 0; > > > > + virtual RPiController::StatisticsPtr platformProcessStats(Span<uint8_t> mem) = 0; > > > > + > > > > + void setMode(const IPACameraSensorInfo &sensorInfo); > > > > + void setCameraTimeoutValue(); > > > > + bool validateSensorControls(); > > > > + bool validateLensControls(); > > > > + void applyControls(const ControlList &controls); > > > > + virtual void handleControls(const ControlList &controls) = 0; > > > > + void fillDeviceStatus(const ControlList &sensorControls, unsigned int ipaContext); > > > > + void applyFrameDurations(utils::Duration minFrameDuration, utils::Duration maxFrameDuration); > > > > + void applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls); > > > > + > > > > + std::map<unsigned int, MappedFrameBuffer> buffers_; > > > > + > > > > + bool lensPresent_; > > > > + ControlList libcameraMetadata_; > > > > + > > > > + std::array<RPiController::Metadata, numMetadataContexts> rpiMetadata_; > > > > + > > > > + /* > > > > + * We count frames to decide if the frame must be hidden (e.g. from > > > > + * display) or mistrusted (i.e. not given to the control algos). > > > > + */ > > > > + uint64_t frameCount_; > > > > + > > > > + /* How many frames we should avoid running control algos on. */ > > > > + unsigned int mistrustCount_; > > > > + > > > > + /* Number of frames that need to be dropped on startup. */ > > > > + unsigned int dropFrameCount_; > > > > + > > > > + /* Frame timestamp for the last run of the controller. */ > > > > + uint64_t lastRunTimestamp_; > > > > + > > > > + /* Do we run a Controller::process() for this frame? */ > > > > + bool processPending_; > > > > + > > > > + /* Distinguish the first camera start from others. */ > > > > + bool firstStart_; > > > > + > > > > + /* Frame duration (1/fps) limits. */ > > > > + utils::Duration minFrameDuration_; > > > > + utils::Duration maxFrameDuration_; > > > > + > > > > +protected: > > > > > > We usually put protected members before private members. > > > > > > > + /* Raspberry Pi controller specific defines. */ > > > > + std::unique_ptr<RPiController::CamHelper> helper_; > > > > + RPiController::Controller controller_; > > > > + > > > > + ControlInfoMap sensorCtrls_; > > > > + ControlInfoMap lensCtrls_; > > > > + > > > > + /* Camera sensor params. */ > > > > + CameraMode mode_; > > > > + > > > > + /* Track the frame length times over FrameLengthsQueueSize frames. */ > > > > + std::deque<utils::Duration> frameLengths_; > > > > + utils::Duration lastTimeout_; > > > > +}; > > > > + > > > > +} /* namespace ipa::RPi */ > > > > + > > > > +} /* namespace libcamera */ > > > > diff --git a/src/ipa/rpi/common/meson.build b/src/ipa/rpi/common/meson.build > > > > new file mode 100644 > > > > index 000000000000..a7189f8389af > > > > --- /dev/null > > > > +++ b/src/ipa/rpi/common/meson.build > > > > @@ -0,0 +1,7 @@ > > > > +# SPDX-License-Identifier: CC0-1.0 > > > > + > > > > +# SPDX-License-Identifier: CC0-1.0 > > > > > > One SPDX header is enough :-) > > > > > > > + > > > > +rpi_ipa_common_sources = files([ > > > > + 'ipa_base.cpp', > > > > +]) > > > > diff --git a/src/ipa/rpi/vc4/meson.build b/src/ipa/rpi/vc4/meson.build > > > > index 992d0f1ab5a7..b581391586b3 100644 > > > > --- a/src/ipa/rpi/vc4/meson.build > > > > +++ b/src/ipa/rpi/vc4/meson.build > > > > @@ -13,11 +13,15 @@ vc4_ipa_includes = [ > > > > ] > > > > > > > > vc4_ipa_sources = files([ > > > > - 'raspberrypi.cpp', > > > > + 'vc4.cpp', > > > > ]) > > > > > > > > vc4_ipa_includes += include_directories('..') > > > > -vc4_ipa_sources += [rpi_ipa_cam_helper_sources, rpi_ipa_controller_sources] > > > > +vc4_ipa_sources += [ > > > > + rpi_ipa_cam_helper_sources, > > > > + rpi_ipa_common_sources, > > > > + rpi_ipa_controller_sources, > > > > > > Could we avoid compiling all these source files multiple times, once for > > > the IpaVc4 IPA module, and once for the other IPA modules that may be > > > implemented in the future ? This can be done with a static library, see > > > src/apps/common/meson.build and src/apps/cam/meson.build for instance. > > > > > > > +] > > > > > > > > mod = shared_module(ipa_name, > > > > [vc4_ipa_sources, libcamera_generated_ipa_headers], > > > > > > > Sure, I'll fix this up. > > > > Naush > > > > > [snip]
diff --git a/src/ipa/meson.build b/src/ipa/meson.build index 10d3b44ca7b6..0c622c38a4a0 100644 --- a/src/ipa/meson.build +++ b/src/ipa/meson.build @@ -37,13 +37,14 @@ endif enabled_ipa_modules = [] -# If the Raspberry Pi VC4 IPA is enabled, ensure we include the rpi/cam_helper -# and rpi/controller subdirectories in the build. +# If the Raspberry Pi VC4 IPA is enabled, ensure we include the cam_helper, +# common and controller subdirectories in the build. # # This is done here and not within rpi/vc4/meson.build as meson does not # allow the subdir command to traverse up the directory tree. if pipelines.contains('rpi/vc4') subdir('rpi/cam_helper') + subdir('rpi/common') subdir('rpi/controller') endif diff --git a/src/ipa/rpi/vc4/raspberrypi.cpp b/src/ipa/rpi/common/ipa_base.cpp similarity index 65% rename from src/ipa/rpi/vc4/raspberrypi.cpp rename to src/ipa/rpi/common/ipa_base.cpp index e3ca8e2f7cbe..becada5973ad 100644 --- a/src/ipa/rpi/vc4/raspberrypi.cpp +++ b/src/ipa/rpi/common/ipa_base.cpp @@ -1,60 +1,30 @@ /* SPDX-License-Identifier: BSD-2-Clause */ /* - * Copyright (C) 2019-2021, Raspberry Pi Ltd + * Copyright (C) 2019-2023, Raspberry Pi Ltd * - * rpi.cpp - Raspberry Pi Image Processing Algorithms + * ipa_base.cpp - Raspberry Pi IPA base class */ -#include <algorithm> -#include <array> -#include <cstring> -#include <deque> -#include <fcntl.h> -#include <math.h> -#include <stdint.h> -#include <string.h> -#include <sys/mman.h> -#include <vector> +#include "ipa_base.h" -#include <linux/bcm2835-isp.h> +#include <cmath> #include <libcamera/base/log.h> -#include <libcamera/base/shared_fd.h> #include <libcamera/base/span.h> - #include <libcamera/control_ids.h> -#include <libcamera/controls.h> -#include <libcamera/framebuffer.h> -#include <libcamera/request.h> - -#include <libcamera/ipa/ipa_interface.h> -#include <libcamera/ipa/ipa_module_info.h> -#include <libcamera/ipa/raspberrypi_ipa_interface.h> -#include "libcamera/internal/mapped_framebuffer.h" - -#include "cam_helper/cam_helper.h" #include "controller/af_algorithm.h" #include "controller/af_status.h" #include "controller/agc_algorithm.h" -#include "controller/agc_status.h" -#include "controller/alsc_status.h" #include "controller/awb_algorithm.h" #include "controller/awb_status.h" #include "controller/black_level_status.h" #include "controller/ccm_algorithm.h" #include "controller/ccm_status.h" #include "controller/contrast_algorithm.h" -#include "controller/contrast_status.h" -#include "controller/controller.h" #include "controller/denoise_algorithm.h" -#include "controller/denoise_status.h" -#include "controller/dpc_status.h" -#include "controller/geq_status.h" #include "controller/lux_status.h" -#include "controller/metadata.h" #include "controller/sharpen_algorithm.h" -#include "controller/sharpen_status.h" #include "controller/statistics.h" namespace libcamera { @@ -62,8 +32,7 @@ namespace libcamera { using namespace std::literals::chrono_literals; using utils::Duration; -/* Number of metadata objects available in the context list. */ -constexpr unsigned int numMetadataContexts = 16; +namespace { /* Number of frame length times to hold in the queue. */ constexpr unsigned int FrameLengthsQueueSize = 10; @@ -80,7 +49,7 @@ constexpr Duration defaultMaxFrameDuration = 250.0s; * we rate-limit the controller Prepare() and Process() calls to lower than or * equal to this rate. */ -constexpr Duration controllerMinFrameDuration = 1.0s / 30.0; +static constexpr Duration controllerMinFrameDuration = 1.0s / 30.0; /* List of controls handled by the Raspberry Pi IPA */ static const ControlInfoMap::Map ipaControls{ @@ -116,118 +85,23 @@ static const ControlInfoMap::Map ipaAfControls{ { &controls::LensPosition, ControlInfo(0.0f, 32.0f, 1.0f) } }; +} /* namespace */ + LOG_DEFINE_CATEGORY(IPARPI) namespace ipa::RPi { -class IPARPi : public IPARPiInterface +IpaBase::IpaBase() + : frameCount_(0), mistrustCount_(0), lastRunTimestamp_(0), firstStart_(true), + controller_() { -public: - IPARPi() - : controller_(), frameCount_(0), checkCount_(0), mistrustCount_(0), - lastRunTimestamp_(0), lsTable_(nullptr), firstStart_(true), - lastTimeout_(0s) - { - } - - ~IPARPi() - { - if (lsTable_) - munmap(lsTable_, MaxLsGridSize); - } - - int init(const IPASettings &settings, const InitParams ¶ms, InitResult *result) override; - void start(const ControlList &controls, StartResult *result) override; - void stop() override {} - - int configure(const IPACameraSensorInfo &sensorInfo, const ConfigParams ¶ms, - ConfigResult *result) override; - void mapBuffers(const std::vector<IPABuffer> &buffers) override; - void unmapBuffers(const std::vector<unsigned int> &ids) override; - void prepareIsp(const PrepareParams ¶ms) override; - void processStats(const ProcessParams ¶ms) override; - -private: - void setMode(const IPACameraSensorInfo &sensorInfo); - bool validateSensorControls(); - bool validateIspControls(); - bool validateLensControls(); - void applyControls(const ControlList &controls); - void prepare(const PrepareParams ¶ms); - void reportMetadata(unsigned int ipaContext, ControlList *controls) override; - void fillDeviceStatus(const ControlList &sensorControls, unsigned int ipaContext); - RPiController::StatisticsPtr fillStatistics(bcm2835_isp_stats *stats) const; - void process(unsigned int bufferId, unsigned int ipaContext); - void setCameraTimeoutValue(); - void applyFrameDurations(Duration minFrameDuration, Duration maxFrameDuration); - void applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls); - void applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls); - void applyDG(const struct AgcStatus *dgStatus, ControlList &ctrls); - void applyCCM(const struct CcmStatus *ccmStatus, ControlList &ctrls); - void applyBlackLevel(const struct BlackLevelStatus *blackLevelStatus, ControlList &ctrls); - void applyGamma(const struct ContrastStatus *contrastStatus, ControlList &ctrls); - void applyGEQ(const struct GeqStatus *geqStatus, ControlList &ctrls); - void applyDenoise(const struct DenoiseStatus *denoiseStatus, ControlList &ctrls); - void applySharpen(const struct SharpenStatus *sharpenStatus, ControlList &ctrls); - void applyDPC(const struct DpcStatus *dpcStatus, ControlList &ctrls); - void applyLS(const struct AlscStatus *lsStatus, ControlList &ctrls); - void applyAF(const struct AfStatus *afStatus, ControlList &lensCtrls); - void resampleTable(uint16_t dest[], const std::vector<double> &src, int destW, int destH); - - std::map<unsigned int, MappedFrameBuffer> buffers_; - - ControlInfoMap sensorCtrls_; - ControlInfoMap ispCtrls_; - ControlInfoMap lensCtrls_; - bool lensPresent_; - ControlList libcameraMetadata_; - - /* Camera sensor params. */ - CameraMode mode_; - - /* Raspberry Pi controller specific defines. */ - std::unique_ptr<RPiController::CamHelper> helper_; - RPiController::Controller controller_; - std::array<RPiController::Metadata, numMetadataContexts> rpiMetadata_; - - /* - * We count frames to decide if the frame must be hidden (e.g. from - * display) or mistrusted (i.e. not given to the control algos). - */ - uint64_t frameCount_; - - /* For checking the sequencing of Prepare/Process calls. */ - uint64_t checkCount_; - - /* How many frames we should avoid running control algos on. */ - unsigned int mistrustCount_; - - /* Number of frames that need to be dropped on startup. */ - unsigned int dropFrameCount_; - - /* Frame timestamp for the last run of the controller. */ - uint64_t lastRunTimestamp_; - - /* Do we run a Controller::process() for this frame? */ - bool processPending_; - - /* LS table allocation passed in from the pipeline handler. */ - SharedFD lsTableHandle_; - void *lsTable_; - - /* Distinguish the first camera start from others. */ - bool firstStart_; - - /* Frame duration (1/fps) limits. */ - Duration minFrameDuration_; - Duration maxFrameDuration_; +} - /* Track the frame length times over FrameLengthsQueueSize frames. */ - std::deque<Duration> frameLengths_; - Duration lastTimeout_; -}; +IpaBase::~IpaBase() +{ +} -int IPARPi::init(const IPASettings &settings, const InitParams ¶ms, InitResult *result) +int32_t IpaBase::init(const IPASettings &settings, const InitParams ¶ms, InitResult *result) { /* * Load the "helper" for this sensor. This tells us all the device specific stuff @@ -264,14 +138,6 @@ int IPARPi::init(const IPASettings &settings, const InitParams ¶ms, InitResu return ret; } - const std::string &target = controller_.getTarget(); - if (target != "bcm2835") { - LOG(IPARPI, Error) - << "Tuning data file target returned \"" << target << "\"" - << ", expected \"bcm2835\""; - return -EINVAL; - } - lensPresent_ = params.lensPresent; controller_.initialise(); @@ -282,10 +148,88 @@ int IPARPi::init(const IPASettings &settings, const InitParams ¶ms, InitResu ctrlMap.merge(ControlInfoMap::Map(ipaAfControls)); result->controlInfo = ControlInfoMap(std::move(ctrlMap), controls::controls); - return 0; + return platformInit(params, result); } -void IPARPi::start(const ControlList &controls, StartResult *result) +int32_t IpaBase::configure(const IPACameraSensorInfo &sensorInfo, const ConfigParams ¶ms, + ConfigResult *result) +{ + sensorCtrls_ = params.sensorControls; + + if (!validateSensorControls()) { + LOG(IPARPI, Error) << "Sensor control validation failed."; + return -1; + } + + if (lensPresent_) { + lensCtrls_ = params.lensControls; + if (!validateLensControls()) { + LOG(IPARPI, Warning) << "Lens validation failed, " + << "no lens control will be available."; + lensPresent_ = false; + } + } + + /* Setup a metadata ControlList to output metadata. */ + libcameraMetadata_ = ControlList(controls::controls); + + /* Re-assemble camera mode using the sensor info. */ + setMode(sensorInfo); + + mode_.transform = static_cast<libcamera::Transform>(params.transform); + + /* Pass the camera mode to the CamHelper to setup algorithms. */ + helper_->setCameraMode(mode_); + + /* + * Initialise this ControlList correctly, even if empty, in case the IPA is + * running is isolation mode (passing the ControlList through the IPC layer). + */ + ControlList ctrls(sensorCtrls_); + + /* The pipeline handler passes out the mode's sensitivity. */ + result->modeSensitivity = mode_.sensitivity; + + if (firstStart_) { + /* Supply initial values for frame durations. */ + applyFrameDurations(defaultMinFrameDuration, defaultMaxFrameDuration); + + /* Supply initial values for gain and exposure. */ + AgcStatus agcStatus; + agcStatus.shutterTime = defaultExposureTime; + agcStatus.analogueGain = defaultAnalogueGain; + applyAGC(&agcStatus, ctrls); + } + + result->controls = std::move(ctrls); + + /* + * Apply the correct limits to the exposure, gain and frame duration controls + * based on the current sensor mode. + */ + ControlInfoMap::Map ctrlMap = ipaControls; + ctrlMap[&controls::FrameDurationLimits] = + ControlInfo(static_cast<int64_t>(mode_.minFrameDuration.get<std::micro>()), + static_cast<int64_t>(mode_.maxFrameDuration.get<std::micro>())); + + ctrlMap[&controls::AnalogueGain] = + ControlInfo(static_cast<float>(mode_.minAnalogueGain), + static_cast<float>(mode_.maxAnalogueGain)); + + ctrlMap[&controls::ExposureTime] = + ControlInfo(static_cast<int32_t>(mode_.minShutter.get<std::micro>()), + static_cast<int32_t>(mode_.maxShutter.get<std::micro>())); + + /* Declare Autofocus controls, only if we have a controllable lens */ + if (lensPresent_) + ctrlMap.merge(ControlInfoMap::Map(ipaAfControls)); + + result->controlInfo = ControlInfoMap(std::move(ctrlMap), controls::controls); + + return platformConfigure(params, result); +} + +void IpaBase::start(const ControlList &controls, StartResult *result) { RPiController::Metadata metadata; @@ -320,7 +264,6 @@ void IPARPi::start(const ControlList &controls, StartResult *result) * or merely a mode switch in a running system. */ frameCount_ = 0; - checkCount_ = 0; if (firstStart_) { dropFrameCount_ = helper_->hideFramesStartup(); mistrustCount_ = helper_->mistrustFramesStartup(); @@ -363,229 +306,141 @@ void IPARPi::start(const ControlList &controls, StartResult *result) lastRunTimestamp_ = 0; } -void IPARPi::setMode(const IPACameraSensorInfo &sensorInfo) +void IpaBase::mapBuffers(const std::vector<IPABuffer> &buffers) { - mode_.bitdepth = sensorInfo.bitsPerPixel; - mode_.width = sensorInfo.outputSize.width; - mode_.height = sensorInfo.outputSize.height; - mode_.sensorWidth = sensorInfo.activeAreaSize.width; - mode_.sensorHeight = sensorInfo.activeAreaSize.height; - mode_.cropX = sensorInfo.analogCrop.x; - mode_.cropY = sensorInfo.analogCrop.y; - mode_.pixelRate = sensorInfo.pixelRate; - - /* - * Calculate scaling parameters. The scale_[xy] factors are determined - * by the ratio between the crop rectangle size and the output size. - */ - mode_.scaleX = sensorInfo.analogCrop.width / sensorInfo.outputSize.width; - mode_.scaleY = sensorInfo.analogCrop.height / sensorInfo.outputSize.height; - - /* - * We're not told by the pipeline handler how scaling is split between - * binning and digital scaling. For now, as a heuristic, assume that - * downscaling up to 2 is achieved through binning, and that any - * additional scaling is achieved through digital scaling. - * - * \todo Get the pipeline handle to provide the full data - */ - mode_.binX = std::min(2, static_cast<int>(mode_.scaleX)); - mode_.binY = std::min(2, static_cast<int>(mode_.scaleY)); - - /* The noise factor is the square root of the total binning factor. */ - mode_.noiseFactor = sqrt(mode_.binX * mode_.binY); - - /* - * Calculate the line length as the ratio between the line length in - * pixels and the pixel rate. - */ - mode_.minLineLength = sensorInfo.minLineLength * (1.0s / sensorInfo.pixelRate); - mode_.maxLineLength = sensorInfo.maxLineLength * (1.0s / sensorInfo.pixelRate); - - /* - * Set the frame length limits for the mode to ensure exposure and - * framerate calculations are clipped appropriately. - */ - mode_.minFrameLength = sensorInfo.minFrameLength; - mode_.maxFrameLength = sensorInfo.maxFrameLength; - - /* Store these for convenience. */ - mode_.minFrameDuration = mode_.minFrameLength * mode_.minLineLength; - mode_.maxFrameDuration = mode_.maxFrameLength * mode_.maxLineLength; - - /* - * Some sensors may have different sensitivities in different modes; - * the CamHelper will know the correct value. - */ - mode_.sensitivity = helper_->getModeSensitivity(mode_); - - const ControlInfo &gainCtrl = sensorCtrls_.at(V4L2_CID_ANALOGUE_GAIN); - const ControlInfo &shutterCtrl = sensorCtrls_.at(V4L2_CID_EXPOSURE); - - mode_.minAnalogueGain = helper_->gain(gainCtrl.min().get<int32_t>()); - mode_.maxAnalogueGain = helper_->gain(gainCtrl.max().get<int32_t>()); - - /* Shutter speed is calculated based on the limits of the frame durations. */ - mode_.minShutter = helper_->exposure(shutterCtrl.min().get<int32_t>(), mode_.minLineLength); - mode_.maxShutter = Duration::max(); - helper_->getBlanking(mode_.maxShutter, - mode_.minFrameDuration, mode_.maxFrameDuration); + for (const IPABuffer &buffer : buffers) { + const FrameBuffer fb(buffer.planes); + buffers_.emplace(buffer.id, + MappedFrameBuffer(&fb, MappedFrameBuffer::MapFlag::ReadWrite)); + } } -int IPARPi::configure(const IPACameraSensorInfo &sensorInfo, const ConfigParams ¶ms, - ConfigResult *result) +void IpaBase::unmapBuffers(const std::vector<unsigned int> &ids) { - sensorCtrls_ = params.sensorControls; - ispCtrls_ = params.ispControls; - - if (!validateSensorControls()) { - LOG(IPARPI, Error) << "Sensor control validation failed."; - return -1; - } - - if (!validateIspControls()) { - LOG(IPARPI, Error) << "ISP control validation failed."; - return -1; - } + for (unsigned int id : ids) { + auto it = buffers_.find(id); + if (it == buffers_.end()) + continue; - if (lensPresent_) { - lensCtrls_ = params.lensControls; - if (!validateLensControls()) { - LOG(IPARPI, Warning) << "Lens validation failed, " - << "no lens control will be available."; - lensPresent_ = false; - } + buffers_.erase(id); } +} - /* Setup a metadata ControlList to output metadata. */ - libcameraMetadata_ = ControlList(controls::controls); - - /* Re-assemble camera mode using the sensor info. */ - setMode(sensorInfo); - - mode_.transform = static_cast<libcamera::Transform>(params.transform); +void IpaBase::prepareIsp(const PrepareParams ¶ms) +{ + applyControls(params.requestControls); - /* Store the lens shading table pointer and handle if available. */ - if (params.lsTableHandle.isValid()) { - /* Remove any previous table, if there was one. */ - if (lsTable_) { - munmap(lsTable_, MaxLsGridSize); - lsTable_ = nullptr; - } + /* + * At start-up, or after a mode-switch, we may want to + * avoid running the control algos for a few frames in case + * they are "unreliable". + */ + int64_t frameTimestamp = params.sensorControls.get(controls::SensorTimestamp).value_or(0); + unsigned int ipaContext = params.ipaContext % rpiMetadata_.size(); + RPiController::Metadata &rpiMetadata = rpiMetadata_[ipaContext]; + Span<uint8_t> embeddedBuffer; - /* Map the LS table buffer into user space. */ - lsTableHandle_ = std::move(params.lsTableHandle); - if (lsTableHandle_.isValid()) { - lsTable_ = mmap(nullptr, MaxLsGridSize, PROT_READ | PROT_WRITE, - MAP_SHARED, lsTableHandle_.get(), 0); + rpiMetadata.clear(); + fillDeviceStatus(params.sensorControls, ipaContext); - if (lsTable_ == MAP_FAILED) { - LOG(IPARPI, Error) << "dmaHeap mmap failure for LS table."; - lsTable_ = nullptr; - } - } + if (params.buffers.embedded) { + /* + * Pipeline handler has supplied us with an embedded data buffer, + * we must pass it to the CamHelper for parsing. + */ + auto it = buffers_.find(params.buffers.embedded); + ASSERT(it != buffers_.end()); + embeddedBuffer = it->second.planes()[0]; } - /* Pass the camera mode to the CamHelper to setup algorithms. */ - helper_->setCameraMode(mode_); - /* - * Initialise this ControlList correctly, even if empty, in case the IPA is - * running is isolation mode (passing the ControlList through the IPC layer). + * AGC wants to know the algorithm status from the time it actioned the + * sensor exposure/gain changes. So fetch it from the metadata list + * indexed by the IPA cookie returned, and put it in the current frame + * metadata. */ - ControlList ctrls(sensorCtrls_); - - /* The pipeline handler passes out the mode's sensitivity. */ - result->modeSensitivity = mode_.sensitivity; + AgcStatus agcStatus; + RPiController::Metadata &delayedMetadata = rpiMetadata_[params.delayContext]; + if (!delayedMetadata.get<AgcStatus>("agc.status", agcStatus)) + rpiMetadata.set("agc.delayed_status", agcStatus); - if (firstStart_) { - /* Supply initial values for frame durations. */ - applyFrameDurations(defaultMinFrameDuration, defaultMaxFrameDuration); + /* + * This may overwrite the DeviceStatus using values from the sensor + * metadata, and may also do additional custom processing. + */ + helper_->prepare(embeddedBuffer, rpiMetadata); - /* Supply initial values for gain and exposure. */ - AgcStatus agcStatus; - agcStatus.shutterTime = defaultExposureTime; - agcStatus.analogueGain = defaultAnalogueGain; - applyAGC(&agcStatus, ctrls); + /* Allow a 10% margin on the comparison below. */ + Duration delta = (frameTimestamp - lastRunTimestamp_) * 1.0ns; + if (lastRunTimestamp_ && frameCount_ > dropFrameCount_ && + delta < controllerMinFrameDuration * 0.9) { + /* + * Ensure we merge the previous frame's metadata with the current + * frame. This will not overwrite exposure/gain values for the + * current frame, or any other bits of metadata that were added + * in helper_->Prepare(). + */ + RPiController::Metadata &lastMetadata = + rpiMetadata_[(ipaContext ? ipaContext : rpiMetadata_.size()) - 1]; + rpiMetadata.mergeCopy(lastMetadata); + processPending_ = false; + } else { + processPending_ = true; + lastRunTimestamp_ = frameTimestamp; } - result->controls = std::move(ctrls); - /* - * Apply the correct limits to the exposure, gain and frame duration controls - * based on the current sensor mode. + * If a statistics buffer has been passed in, call processStats + * directly now before prepare() since the statistics are available in-line + * with the Bayer frame. */ - ControlInfoMap::Map ctrlMap = ipaControls; - ctrlMap[&controls::FrameDurationLimits] = - ControlInfo(static_cast<int64_t>(mode_.minFrameDuration.get<std::micro>()), - static_cast<int64_t>(mode_.maxFrameDuration.get<std::micro>())); - - ctrlMap[&controls::AnalogueGain] = - ControlInfo(static_cast<float>(mode_.minAnalogueGain), - static_cast<float>(mode_.maxAnalogueGain)); - - ctrlMap[&controls::ExposureTime] = - ControlInfo(static_cast<int32_t>(mode_.minShutter.get<std::micro>()), - static_cast<int32_t>(mode_.maxShutter.get<std::micro>())); - - /* Declare Autofocus controls, only if we have a controllable lens */ - if (lensPresent_) - ctrlMap.merge(ControlInfoMap::Map(ipaAfControls)); - - result->controlInfo = ControlInfoMap(std::move(ctrlMap), controls::controls); - return 0; -} - -void IPARPi::mapBuffers(const std::vector<IPABuffer> &buffers) -{ - for (const IPABuffer &buffer : buffers) { - const FrameBuffer fb(buffer.planes); - buffers_.emplace(buffer.id, - MappedFrameBuffer(&fb, MappedFrameBuffer::MapFlag::ReadWrite)); + if (params.buffers.stats) + processStats({ params.buffers, params.ipaContext }); + + /* Do we need/want to call prepare? */ + if (processPending_) { + controller_.prepare(&rpiMetadata); + /* Actually prepare the ISP parameters for the frame. */ + platformPrepareIsp(params, rpiMetadata); } -} -void IPARPi::unmapBuffers(const std::vector<unsigned int> &ids) -{ - for (unsigned int id : ids) { - auto it = buffers_.find(id); - if (it == buffers_.end()) - continue; + frameCount_++; - buffers_.erase(id); - } + /* Ready to push the input buffer into the ISP. */ + prepareIspComplete.emit(params.buffers); } -void IPARPi::processStats(const ProcessParams ¶ms) +void IpaBase::processStats(const ProcessParams ¶ms) { - unsigned int context = params.ipaContext % rpiMetadata_.size(); + unsigned int ipaContext = params.ipaContext % rpiMetadata_.size(); - if (++checkCount_ != frameCount_) /* assert here? */ - LOG(IPARPI, Error) << "WARNING: Prepare/Process mismatch!!!"; - if (processPending_ && frameCount_ > mistrustCount_) - process(params.buffers.stats, context); + if (processPending_ && frameCount_ > mistrustCount_) { + RPiController::Metadata &rpiMetadata = rpiMetadata_[ipaContext]; - processStatsComplete.emit(params.buffers); -} + auto it = buffers_.find(params.buffers.stats); + if (it == buffers_.end()) { + LOG(IPARPI, Error) << "Could not find stats buffer!"; + return; + } + RPiController::StatisticsPtr statistics = platformProcessStats(it->second.planes()[0]); -void IPARPi::prepareIsp(const PrepareParams ¶ms) -{ - applyControls(params.requestControls); + helper_->process(statistics, rpiMetadata); + controller_.process(statistics, &rpiMetadata); - /* - * At start-up, or after a mode-switch, we may want to - * avoid running the control algos for a few frames in case - * they are "unreliable". - */ - prepare(params); - frameCount_++; + struct AgcStatus agcStatus; + if (rpiMetadata.get("agc.status", agcStatus) == 0) { + ControlList ctrls(sensorCtrls_); + applyAGC(&agcStatus, ctrls); + setDelayedControls.emit(ctrls, ipaContext); + setCameraTimeoutValue(); + } + } - /* Ready to push the input buffer into the ISP. */ - prepareIspComplete.emit(params.buffers); + processStatsComplete.emit(params.buffers); } -void IPARPi::reportMetadata(unsigned int ipaContext, ControlList *controls) +void IpaBase::reportMetadata(unsigned int ipaContext, ControlList *metadata) { RPiController::Metadata &rpiMetadata = rpiMetadata_[ipaContext % rpiMetadata_.size()]; std::unique_lock<RPiController::Metadata> lock(rpiMetadata); @@ -658,48 +513,132 @@ void IPARPi::reportMetadata(unsigned int ipaContext, ControlList *controls) libcameraMetadata_.set(controls::FocusFoM, focusFoM); } - CcmStatus *ccmStatus = rpiMetadata.getLocked<CcmStatus>("ccm.status"); - if (ccmStatus) { - float m[9]; - for (unsigned int i = 0; i < 9; i++) - m[i] = ccmStatus->matrix[i]; - libcameraMetadata_.set(controls::ColourCorrectionMatrix, m); - } + CcmStatus *ccmStatus = rpiMetadata.getLocked<CcmStatus>("ccm.status"); + if (ccmStatus) { + float m[9]; + for (unsigned int i = 0; i < 9; i++) + m[i] = ccmStatus->matrix[i]; + libcameraMetadata_.set(controls::ColourCorrectionMatrix, m); + } + + const AfStatus *afStatus = rpiMetadata.getLocked<AfStatus>("af.status"); + if (afStatus) { + int32_t s, p; + switch (afStatus->state) { + case AfState::Scanning: + s = controls::AfStateScanning; + break; + case AfState::Focused: + s = controls::AfStateFocused; + break; + case AfState::Failed: + s = controls::AfStateFailed; + break; + default: + s = controls::AfStateIdle; + } + switch (afStatus->pauseState) { + case AfPauseState::Pausing: + p = controls::AfPauseStatePausing; + break; + case AfPauseState::Paused: + p = controls::AfPauseStatePaused; + break; + default: + p = controls::AfPauseStateRunning; + } + libcameraMetadata_.set(controls::AfState, s); + libcameraMetadata_.set(controls::AfPauseState, p); + } + + *metadata = std::move(libcameraMetadata_); +} + +void IpaBase::setMode(const IPACameraSensorInfo &sensorInfo) +{ + mode_.bitdepth = sensorInfo.bitsPerPixel; + mode_.width = sensorInfo.outputSize.width; + mode_.height = sensorInfo.outputSize.height; + mode_.sensorWidth = sensorInfo.activeAreaSize.width; + mode_.sensorHeight = sensorInfo.activeAreaSize.height; + mode_.cropX = sensorInfo.analogCrop.x; + mode_.cropY = sensorInfo.analogCrop.y; + mode_.pixelRate = sensorInfo.pixelRate; + + /* + * Calculate scaling parameters. The scale_[xy] factors are determined + * by the ratio between the crop rectangle size and the output size. + */ + mode_.scaleX = sensorInfo.analogCrop.width / sensorInfo.outputSize.width; + mode_.scaleY = sensorInfo.analogCrop.height / sensorInfo.outputSize.height; + + /* + * We're not told by the pipeline handler how scaling is split between + * binning and digital scaling. For now, as a heuristic, assume that + * downscaling up to 2 is achieved through binning, and that any + * additional scaling is achieved through digital scaling. + * + * \todo Get the pipeline handle to provide the full data + */ + mode_.binX = std::min(2, static_cast<int>(mode_.scaleX)); + mode_.binY = std::min(2, static_cast<int>(mode_.scaleY)); + + /* The noise factor is the square root of the total binning factor. */ + mode_.noiseFactor = std::sqrt(mode_.binX * mode_.binY); + + /* + * Calculate the line length as the ratio between the line length in + * pixels and the pixel rate. + */ + mode_.minLineLength = sensorInfo.minLineLength * (1.0s / sensorInfo.pixelRate); + mode_.maxLineLength = sensorInfo.maxLineLength * (1.0s / sensorInfo.pixelRate); + + /* + * Set the frame length limits for the mode to ensure exposure and + * framerate calculations are clipped appropriately. + */ + mode_.minFrameLength = sensorInfo.minFrameLength; + mode_.maxFrameLength = sensorInfo.maxFrameLength; + + /* Store these for convenience. */ + mode_.minFrameDuration = mode_.minFrameLength * mode_.minLineLength; + mode_.maxFrameDuration = mode_.maxFrameLength * mode_.maxLineLength; + + /* + * Some sensors may have different sensitivities in different modes; + * the CamHelper will know the correct value. + */ + mode_.sensitivity = helper_->getModeSensitivity(mode_); + + const ControlInfo &gainCtrl = sensorCtrls_.at(V4L2_CID_ANALOGUE_GAIN); + const ControlInfo &shutterCtrl = sensorCtrls_.at(V4L2_CID_EXPOSURE); + + mode_.minAnalogueGain = helper_->gain(gainCtrl.min().get<int32_t>()); + mode_.maxAnalogueGain = helper_->gain(gainCtrl.max().get<int32_t>()); + + /* Shutter speed is calculated based on the limits of the frame durations. */ + mode_.minShutter = helper_->exposure(shutterCtrl.min().get<int32_t>(), mode_.minLineLength); + mode_.maxShutter = Duration::max(); + helper_->getBlanking(mode_.maxShutter, + mode_.minFrameDuration, mode_.maxFrameDuration); +} + +void IpaBase::setCameraTimeoutValue() +{ + /* + * Take the maximum value of the exposure queue as the camera timeout + * value to pass back to the pipeline handler. Only signal if it has changed + * from the last set value. + */ + auto max = std::max_element(frameLengths_.begin(), frameLengths_.end()); - const AfStatus *afStatus = rpiMetadata.getLocked<AfStatus>("af.status"); - if (afStatus) { - int32_t s, p; - switch (afStatus->state) { - case AfState::Scanning: - s = controls::AfStateScanning; - break; - case AfState::Focused: - s = controls::AfStateFocused; - break; - case AfState::Failed: - s = controls::AfStateFailed; - break; - default: - s = controls::AfStateIdle; - } - switch (afStatus->pauseState) { - case AfPauseState::Pausing: - p = controls::AfPauseStatePausing; - break; - case AfPauseState::Paused: - p = controls::AfPauseStatePaused; - break; - default: - p = controls::AfPauseStateRunning; - } - libcameraMetadata_.set(controls::AfState, s); - libcameraMetadata_.set(controls::AfPauseState, p); + if (*max != lastTimeout_) { + setCameraTimeout.emit(max->get<std::milli>()); + lastTimeout_ = *max; } - - *controls = std::move(libcameraMetadata_); } -bool IPARPi::validateSensorControls() +bool IpaBase::validateSensorControls() { static const uint32_t ctrls[] = { V4L2_CID_ANALOGUE_GAIN, @@ -719,35 +658,7 @@ bool IPARPi::validateSensorControls() return true; } -bool IPARPi::validateIspControls() -{ - static const uint32_t ctrls[] = { - V4L2_CID_RED_BALANCE, - V4L2_CID_BLUE_BALANCE, - V4L2_CID_DIGITAL_GAIN, - V4L2_CID_USER_BCM2835_ISP_CC_MATRIX, - V4L2_CID_USER_BCM2835_ISP_GAMMA, - V4L2_CID_USER_BCM2835_ISP_BLACK_LEVEL, - V4L2_CID_USER_BCM2835_ISP_GEQ, - V4L2_CID_USER_BCM2835_ISP_DENOISE, - V4L2_CID_USER_BCM2835_ISP_SHARPEN, - V4L2_CID_USER_BCM2835_ISP_DPC, - V4L2_CID_USER_BCM2835_ISP_LENS_SHADING, - V4L2_CID_USER_BCM2835_ISP_CDN, - }; - - for (auto c : ctrls) { - if (ispCtrls_.find(c) == ispCtrls_.end()) { - LOG(IPARPI, Error) << "Unable to find ISP control " - << utils::hex(c); - return false; - } - } - - return true; -} - -bool IPARPi::validateLensControls() +bool IpaBase::validateLensControls() { if (lensCtrls_.find(V4L2_CID_FOCUS_ABSOLUTE) == lensCtrls_.end()) { LOG(IPARPI, Error) << "Unable to find Lens control V4L2_CID_FOCUS_ABSOLUTE"; @@ -820,7 +731,7 @@ static const std::map<int32_t, RPiController::AfAlgorithm::AfPause> AfPauseTable { controls::AfPauseResume, RPiController::AfAlgorithm::AfPauseResume }, }; -void IPARPi::applyControls(const ControlList &controls) +void IpaBase::applyControls(const ControlList &controls) { using RPiController::AfAlgorithm; @@ -840,7 +751,7 @@ void IPARPi::applyControls(const ControlList &controls) if (mode == AfModeTable.end()) { LOG(IPARPI, Error) << "AF mode " << idx << " not recognised"; - } else + } else if (af) af->setMode(mode->second); } @@ -1112,6 +1023,10 @@ void IPARPi::applyControls(const ControlList &controls) case controls::NOISE_REDUCTION_MODE: { RPiController::DenoiseAlgorithm *sdn = dynamic_cast<RPiController::DenoiseAlgorithm *>( controller_.getAlgorithm("SDN")); + /* Some platforms may have a combined "denoise" algorithm instead. */ + if (!sdn) + sdn = dynamic_cast<RPiController::DenoiseAlgorithm *>( + controller_.getAlgorithm("denoise")); if (!sdn) { LOG(IPARPI, Warning) << "Could not set NOISE_REDUCTION_MODE - no SDN algorithm"; @@ -1248,125 +1163,12 @@ void IPARPi::applyControls(const ControlList &controls) break; } } -} - -void IPARPi::prepare(const PrepareParams ¶ms) -{ - int64_t frameTimestamp = params.sensorControls.get(controls::SensorTimestamp).value_or(0); - unsigned int ipaContext = params.ipaContext % rpiMetadata_.size(); - RPiController::Metadata &rpiMetadata = rpiMetadata_[ipaContext]; - Span<uint8_t> embeddedBuffer; - - rpiMetadata.clear(); - fillDeviceStatus(params.sensorControls, ipaContext); - - if (params.buffers.embedded) { - /* - * Pipeline handler has supplied us with an embedded data buffer, - * we must pass it to the CamHelper for parsing. - */ - auto it = buffers_.find(params.buffers.embedded); - ASSERT(it != buffers_.end()); - embeddedBuffer = it->second.planes()[0]; - } - - /* - * AGC wants to know the algorithm status from the time it actioned the - * sensor exposure/gain changes. So fetch it from the metadata list - * indexed by the IPA cookie returned, and put it in the current frame - * metadata. - */ - AgcStatus agcStatus; - RPiController::Metadata &delayedMetadata = rpiMetadata_[params.delayContext]; - if (!delayedMetadata.get<AgcStatus>("agc.status", agcStatus)) - rpiMetadata.set("agc.delayed_status", agcStatus); - - /* - * This may overwrite the DeviceStatus using values from the sensor - * metadata, and may also do additional custom processing. - */ - helper_->prepare(embeddedBuffer, rpiMetadata); - - /* Allow a 10% margin on the comparison below. */ - Duration delta = (frameTimestamp - lastRunTimestamp_) * 1.0ns; - if (lastRunTimestamp_ && frameCount_ > dropFrameCount_ && - delta < controllerMinFrameDuration * 0.9) { - /* - * Ensure we merge the previous frame's metadata with the current - * frame. This will not overwrite exposure/gain values for the - * current frame, or any other bits of metadata that were added - * in helper_->Prepare(). - */ - RPiController::Metadata &lastMetadata = - rpiMetadata_[(ipaContext ? ipaContext : rpiMetadata_.size()) - 1]; - rpiMetadata.mergeCopy(lastMetadata); - processPending_ = false; - return; - } - - lastRunTimestamp_ = frameTimestamp; - processPending_ = true; - - ControlList ctrls(ispCtrls_); - - controller_.prepare(&rpiMetadata); - - /* Lock the metadata buffer to avoid constant locks/unlocks. */ - std::unique_lock<RPiController::Metadata> lock(rpiMetadata); - - AwbStatus *awbStatus = rpiMetadata.getLocked<AwbStatus>("awb.status"); - if (awbStatus) - applyAWB(awbStatus, ctrls); - - CcmStatus *ccmStatus = rpiMetadata.getLocked<CcmStatus>("ccm.status"); - if (ccmStatus) - applyCCM(ccmStatus, ctrls); - - AgcStatus *dgStatus = rpiMetadata.getLocked<AgcStatus>("agc.status"); - if (dgStatus) - applyDG(dgStatus, ctrls); - - AlscStatus *lsStatus = rpiMetadata.getLocked<AlscStatus>("alsc.status"); - if (lsStatus) - applyLS(lsStatus, ctrls); - - ContrastStatus *contrastStatus = rpiMetadata.getLocked<ContrastStatus>("contrast.status"); - if (contrastStatus) - applyGamma(contrastStatus, ctrls); - - BlackLevelStatus *blackLevelStatus = rpiMetadata.getLocked<BlackLevelStatus>("black_level.status"); - if (blackLevelStatus) - applyBlackLevel(blackLevelStatus, ctrls); - - GeqStatus *geqStatus = rpiMetadata.getLocked<GeqStatus>("geq.status"); - if (geqStatus) - applyGEQ(geqStatus, ctrls); - - DenoiseStatus *denoiseStatus = rpiMetadata.getLocked<DenoiseStatus>("denoise.status"); - if (denoiseStatus) - applyDenoise(denoiseStatus, ctrls); - - SharpenStatus *sharpenStatus = rpiMetadata.getLocked<SharpenStatus>("sharpen.status"); - if (sharpenStatus) - applySharpen(sharpenStatus, ctrls); - - DpcStatus *dpcStatus = rpiMetadata.getLocked<DpcStatus>("dpc.status"); - if (dpcStatus) - applyDPC(dpcStatus, ctrls); - - const AfStatus *afStatus = rpiMetadata.getLocked<AfStatus>("af.status"); - if (afStatus) { - ControlList lensctrls(lensCtrls_); - applyAF(afStatus, lensctrls); - if (!lensctrls.empty()) - setLensControls.emit(lensctrls); - } - if (!ctrls.empty()) - setIspControls.emit(ctrls); + /* Give derived classes a chance to examine the new controls. */ + handleControls(controls); } -void IPARPi::fillDeviceStatus(const ControlList &sensorControls, unsigned int ipaContext) +void IpaBase::fillDeviceStatus(const ControlList &sensorControls, unsigned int ipaContext) { DeviceStatus deviceStatus = {}; @@ -1390,103 +1192,7 @@ void IPARPi::fillDeviceStatus(const ControlList &sensorControls, unsigned int ip rpiMetadata_[ipaContext].set("device.status", deviceStatus); } -RPiController::StatisticsPtr IPARPi::fillStatistics(bcm2835_isp_stats *stats) const -{ - using namespace RPiController; - - const Controller::HardwareConfig &hw = controller_.getHardwareConfig(); - unsigned int i; - StatisticsPtr statistics = - std::make_unique<Statistics>(Statistics::AgcStatsPos::PreWb, Statistics::ColourStatsPos::PostLsc); - - /* RGB histograms are not used, so do not populate them. */ - statistics->yHist = RPiController::Histogram(stats->hist[0].g_hist, - hw.numHistogramBins); - - /* All region sums are based on a 16-bit normalised pipeline bit-depth. */ - unsigned int scale = Statistics::NormalisationFactorPow2 - hw.pipelineWidth; - - statistics->awbRegions.init(hw.awbRegions); - for (i = 0; i < statistics->awbRegions.numRegions(); i++) - statistics->awbRegions.set(i, { { stats->awb_stats[i].r_sum << scale, - stats->awb_stats[i].g_sum << scale, - stats->awb_stats[i].b_sum << scale }, - stats->awb_stats[i].counted, - stats->awb_stats[i].notcounted }); - - statistics->agcRegions.init(hw.agcRegions); - for (i = 0; i < statistics->agcRegions.numRegions(); i++) - statistics->agcRegions.set(i, { { stats->agc_stats[i].r_sum << scale, - stats->agc_stats[i].g_sum << scale, - stats->agc_stats[i].b_sum << scale }, - stats->agc_stats[i].counted, - stats->awb_stats[i].notcounted }); - - statistics->focusRegions.init(hw.focusRegions); - for (i = 0; i < statistics->focusRegions.numRegions(); i++) - statistics->focusRegions.set(i, { stats->focus_stats[i].contrast_val[1][1] / 1000, - stats->focus_stats[i].contrast_val_num[1][1], - stats->focus_stats[i].contrast_val_num[1][0] }); - return statistics; -} - -void IPARPi::process(unsigned int bufferId, unsigned int ipaContext) -{ - RPiController::Metadata &rpiMetadata = rpiMetadata_[ipaContext]; - - auto it = buffers_.find(bufferId); - if (it == buffers_.end()) { - LOG(IPARPI, Error) << "Could not find stats buffer!"; - return; - } - - Span<uint8_t> mem = it->second.planes()[0]; - bcm2835_isp_stats *stats = reinterpret_cast<bcm2835_isp_stats *>(mem.data()); - RPiController::StatisticsPtr statistics = fillStatistics(stats); - - /* Save the focus stats in the metadata structure to report out later. */ - rpiMetadata_[ipaContext].set("focus.status", statistics->focusRegions); - - helper_->process(statistics, rpiMetadata); - controller_.process(statistics, &rpiMetadata); - - struct AgcStatus agcStatus; - if (rpiMetadata.get("agc.status", agcStatus) == 0) { - ControlList ctrls(sensorCtrls_); - applyAGC(&agcStatus, ctrls); - - setDelayedControls.emit(ctrls, ipaContext); - setCameraTimeoutValue(); - } -} - -void IPARPi::setCameraTimeoutValue() -{ - /* - * Take the maximum value of the exposure queue as the camera timeout - * value to pass back to the pipeline handler. Only signal if it has changed - * from the last set value. - */ - auto max = std::max_element(frameLengths_.begin(), frameLengths_.end()); - - if (*max != lastTimeout_) { - setCameraTimeout.emit(max->get<std::milli>()); - lastTimeout_ = *max; - } -} - -void IPARPi::applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls) -{ - LOG(IPARPI, Debug) << "Applying WB R: " << awbStatus->gainR << " B: " - << awbStatus->gainB; - - ctrls.set(V4L2_CID_RED_BALANCE, - static_cast<int32_t>(awbStatus->gainR * 1000)); - ctrls.set(V4L2_CID_BLUE_BALANCE, - static_cast<int32_t>(awbStatus->gainB * 1000)); -} - -void IPARPi::applyFrameDurations(Duration minFrameDuration, Duration maxFrameDuration) +void IpaBase::applyFrameDurations(Duration minFrameDuration, Duration maxFrameDuration) { /* * This will only be applied once AGC recalculations occur. @@ -1518,7 +1224,7 @@ void IPARPi::applyFrameDurations(Duration minFrameDuration, Duration maxFrameDur agc->setMaxShutter(maxShutter); } -void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls) +void IpaBase::applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls) { const int32_t minGainCode = helper_->gainCode(mode_.minAnalogueGain); const int32_t maxGainCode = helper_->gainCode(mode_.maxAnalogueGain); @@ -1570,269 +1276,6 @@ void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls) helper_->hblankToLineLength(hblank))); } -void IPARPi::applyDG(const struct AgcStatus *dgStatus, ControlList &ctrls) -{ - ctrls.set(V4L2_CID_DIGITAL_GAIN, - static_cast<int32_t>(dgStatus->digitalGain * 1000)); -} - -void IPARPi::applyCCM(const struct CcmStatus *ccmStatus, ControlList &ctrls) -{ - bcm2835_isp_custom_ccm ccm; - - for (int i = 0; i < 9; i++) { - ccm.ccm.ccm[i / 3][i % 3].den = 1000; - ccm.ccm.ccm[i / 3][i % 3].num = 1000 * ccmStatus->matrix[i]; - } - - ccm.enabled = 1; - ccm.ccm.offsets[0] = ccm.ccm.offsets[1] = ccm.ccm.offsets[2] = 0; - - ControlValue c(Span<const uint8_t>{ reinterpret_cast<uint8_t *>(&ccm), - sizeof(ccm) }); - ctrls.set(V4L2_CID_USER_BCM2835_ISP_CC_MATRIX, c); -} - -void IPARPi::applyGamma(const struct ContrastStatus *contrastStatus, ControlList &ctrls) -{ - const unsigned int numGammaPoints = controller_.getHardwareConfig().numGammaPoints; - struct bcm2835_isp_gamma gamma; - - for (unsigned int i = 0; i < numGammaPoints - 1; i++) { - int x = i < 16 ? i * 1024 - : (i < 24 ? (i - 16) * 2048 + 16384 - : (i - 24) * 4096 + 32768); - gamma.x[i] = x; - gamma.y[i] = std::min<uint16_t>(65535, contrastStatus->gammaCurve.eval(x)); - } - - gamma.x[numGammaPoints - 1] = 65535; - gamma.y[numGammaPoints - 1] = 65535; - gamma.enabled = 1; - - ControlValue c(Span<const uint8_t>{ reinterpret_cast<uint8_t *>(&gamma), - sizeof(gamma) }); - ctrls.set(V4L2_CID_USER_BCM2835_ISP_GAMMA, c); -} - -void IPARPi::applyBlackLevel(const struct BlackLevelStatus *blackLevelStatus, ControlList &ctrls) -{ - bcm2835_isp_black_level blackLevel; - - blackLevel.enabled = 1; - blackLevel.black_level_r = blackLevelStatus->blackLevelR; - blackLevel.black_level_g = blackLevelStatus->blackLevelG; - blackLevel.black_level_b = blackLevelStatus->blackLevelB; - - ControlValue c(Span<const uint8_t>{ reinterpret_cast<uint8_t *>(&blackLevel), - sizeof(blackLevel) }); - ctrls.set(V4L2_CID_USER_BCM2835_ISP_BLACK_LEVEL, c); -} - -void IPARPi::applyGEQ(const struct GeqStatus *geqStatus, ControlList &ctrls) -{ - bcm2835_isp_geq geq; - - geq.enabled = 1; - geq.offset = geqStatus->offset; - geq.slope.den = 1000; - geq.slope.num = 1000 * geqStatus->slope; - - ControlValue c(Span<const uint8_t>{ reinterpret_cast<uint8_t *>(&geq), - sizeof(geq) }); - ctrls.set(V4L2_CID_USER_BCM2835_ISP_GEQ, c); -} - -void IPARPi::applyDenoise(const struct DenoiseStatus *denoiseStatus, ControlList &ctrls) -{ - using RPiController::DenoiseMode; - - bcm2835_isp_denoise denoise; - DenoiseMode mode = static_cast<DenoiseMode>(denoiseStatus->mode); - - denoise.enabled = mode != DenoiseMode::Off; - denoise.constant = denoiseStatus->noiseConstant; - denoise.slope.num = 1000 * denoiseStatus->noiseSlope; - denoise.slope.den = 1000; - denoise.strength.num = 1000 * denoiseStatus->strength; - denoise.strength.den = 1000; - - /* Set the CDN mode to match the SDN operating mode. */ - bcm2835_isp_cdn cdn; - switch (mode) { - case DenoiseMode::ColourFast: - cdn.enabled = 1; - cdn.mode = CDN_MODE_FAST; - break; - case DenoiseMode::ColourHighQuality: - cdn.enabled = 1; - cdn.mode = CDN_MODE_HIGH_QUALITY; - break; - default: - cdn.enabled = 0; - } - - ControlValue c(Span<const uint8_t>{ reinterpret_cast<uint8_t *>(&denoise), - sizeof(denoise) }); - ctrls.set(V4L2_CID_USER_BCM2835_ISP_DENOISE, c); - - c = ControlValue(Span<const uint8_t>{ reinterpret_cast<uint8_t *>(&cdn), - sizeof(cdn) }); - ctrls.set(V4L2_CID_USER_BCM2835_ISP_CDN, c); -} - -void IPARPi::applySharpen(const struct SharpenStatus *sharpenStatus, ControlList &ctrls) -{ - bcm2835_isp_sharpen sharpen; - - sharpen.enabled = 1; - sharpen.threshold.num = 1000 * sharpenStatus->threshold; - sharpen.threshold.den = 1000; - sharpen.strength.num = 1000 * sharpenStatus->strength; - sharpen.strength.den = 1000; - sharpen.limit.num = 1000 * sharpenStatus->limit; - sharpen.limit.den = 1000; - - ControlValue c(Span<const uint8_t>{ reinterpret_cast<uint8_t *>(&sharpen), - sizeof(sharpen) }); - ctrls.set(V4L2_CID_USER_BCM2835_ISP_SHARPEN, c); -} - -void IPARPi::applyDPC(const struct DpcStatus *dpcStatus, ControlList &ctrls) -{ - bcm2835_isp_dpc dpc; - - dpc.enabled = 1; - dpc.strength = dpcStatus->strength; - - ControlValue c(Span<const uint8_t>{ reinterpret_cast<uint8_t *>(&dpc), - sizeof(dpc) }); - ctrls.set(V4L2_CID_USER_BCM2835_ISP_DPC, c); -} - -void IPARPi::applyLS(const struct AlscStatus *lsStatus, ControlList &ctrls) -{ - /* - * Program lens shading tables into pipeline. - * Choose smallest cell size that won't exceed 63x48 cells. - */ - const int cellSizes[] = { 16, 32, 64, 128, 256 }; - unsigned int numCells = std::size(cellSizes); - unsigned int i, w, h, cellSize; - for (i = 0; i < numCells; i++) { - cellSize = cellSizes[i]; - w = (mode_.width + cellSize - 1) / cellSize; - h = (mode_.height + cellSize - 1) / cellSize; - if (w < 64 && h <= 48) - break; - } - - if (i == numCells) { - LOG(IPARPI, Error) << "Cannot find cell size"; - return; - } - - /* We're going to supply corner sampled tables, 16 bit samples. */ - w++, h++; - bcm2835_isp_lens_shading ls = { - .enabled = 1, - .grid_cell_size = cellSize, - .grid_width = w, - .grid_stride = w, - .grid_height = h, - /* .dmabuf will be filled in by pipeline handler. */ - .dmabuf = 0, - .ref_transform = 0, - .corner_sampled = 1, - .gain_format = GAIN_FORMAT_U4P10 - }; - - if (!lsTable_ || w * h * 4 * sizeof(uint16_t) > MaxLsGridSize) { - LOG(IPARPI, Error) << "Do not have a correctly allocate lens shading table!"; - return; - } - - if (lsStatus) { - /* Format will be u4.10 */ - uint16_t *grid = static_cast<uint16_t *>(lsTable_); - - resampleTable(grid, lsStatus->r, w, h); - resampleTable(grid + w * h, lsStatus->g, w, h); - std::memcpy(grid + 2 * w * h, grid + w * h, w * h * sizeof(uint16_t)); - resampleTable(grid + 3 * w * h, lsStatus->b, w, h); - } - - ControlValue c(Span<const uint8_t>{ reinterpret_cast<uint8_t *>(&ls), - sizeof(ls) }); - ctrls.set(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING, c); -} - -void IPARPi::applyAF(const struct AfStatus *afStatus, ControlList &lensCtrls) -{ - if (afStatus->lensSetting) { - ControlValue v(afStatus->lensSetting.value()); - lensCtrls.set(V4L2_CID_FOCUS_ABSOLUTE, v); - } -} - -/* - * Resamples a 16x12 table with central sampling to destW x destH with corner - * sampling. - */ -void IPARPi::resampleTable(uint16_t dest[], const std::vector<double> &src, - int destW, int destH) -{ - /* - * Precalculate and cache the x sampling locations and phases to - * save recomputing them on every row. - */ - assert(destW > 1 && destH > 1 && destW <= 64); - int xLo[64], xHi[64]; - double xf[64]; - double x = -0.5, xInc = 16.0 / (destW - 1); - for (int i = 0; i < destW; i++, x += xInc) { - xLo[i] = floor(x); - xf[i] = x - xLo[i]; - xHi[i] = xLo[i] < 15 ? xLo[i] + 1 : 15; - xLo[i] = xLo[i] > 0 ? xLo[i] : 0; - } - - /* Now march over the output table generating the new values. */ - double y = -0.5, yInc = 12.0 / (destH - 1); - for (int j = 0; j < destH; j++, y += yInc) { - int yLo = floor(y); - double yf = y - yLo; - int yHi = yLo < 11 ? yLo + 1 : 11; - yLo = yLo > 0 ? yLo : 0; - double const *rowAbove = src.data() + yLo * 16; - double const *rowBelow = src.data() + yHi * 16; - for (int i = 0; i < destW; i++) { - double above = rowAbove[xLo[i]] * (1 - xf[i]) + rowAbove[xHi[i]] * xf[i]; - double below = rowBelow[xLo[i]] * (1 - xf[i]) + rowBelow[xHi[i]] * xf[i]; - int result = floor(1024 * (above * (1 - yf) + below * yf) + .5); - *(dest++) = result > 16383 ? 16383 : result; /* want u4.10 */ - } - } -} - } /* namespace ipa::RPi */ -/* - * External IPA module interface - */ -extern "C" { -const struct IPAModuleInfo ipaModuleInfo = { - IPA_MODULE_API_VERSION, - 1, - "PipelineHandlerRPi", - "vc4", -}; - -IPAInterface *ipaCreate() -{ - return new ipa::RPi::IPARPi(); -} - -} /* extern "C" */ - } /* namespace libcamera */ diff --git a/src/ipa/rpi/common/ipa_base.h b/src/ipa/rpi/common/ipa_base.h new file mode 100644 index 000000000000..b9fbf9414d63 --- /dev/null +++ b/src/ipa/rpi/common/ipa_base.h @@ -0,0 +1,125 @@ +/* SPDX-License-Identifier: BSD-2-Clause */ +/* + * Copyright (C) 2023, Raspberry Pi Ltd + * + * ipa_base.cpp - Raspberry Pi IPA base class + */ +#pragma once + +#include <array> +#include <deque> +#include <vector> + +#include <libcamera/base/utils.h> + +#include <libcamera/controls.h> + +#include <libcamera/ipa/ipa_interface.h> +#include <libcamera/ipa/ipa_module_info.h> +#include <libcamera/ipa/raspberrypi_ipa_interface.h> + +#include "libcamera/internal/mapped_framebuffer.h" + +#include "cam_helper/cam_helper.h" +#include "controller/agc_status.h" +#include "controller/camera_mode.h" +#include "controller/controller.h" +#include "controller/metadata.h" + +namespace libcamera { + +namespace ipa::RPi { + +class IpaBase : public IPARPiInterface +{ +public: + IpaBase(); + virtual ~IpaBase() = 0; + + int32_t init(const IPASettings &settings, const InitParams ¶ms, InitResult *result) override; + int32_t configure(const IPACameraSensorInfo &sensorInfo, const ConfigParams ¶ms, + ConfigResult *result) override; + + void start(const ControlList &controls, StartResult *result) override; + void stop() override {} + + void mapBuffers(const std::vector<IPABuffer> &buffers) override; + void unmapBuffers(const std::vector<unsigned int> &ids) override; + + void prepareIsp(const PrepareParams ¶ms) override; + void processStats(const ProcessParams ¶ms) override; + + void reportMetadata(unsigned int ipaContext, ControlList *controls) override; + +private: + /* Number of metadata objects available in the context list. */ + static constexpr unsigned int numMetadataContexts = 16; + + virtual int32_t platformInit(const InitParams ¶ms, InitResult *result) = 0; + virtual int32_t platformConfigure(const ConfigParams ¶ms, ConfigResult *result) = 0; + + virtual void platformPrepareIsp(const PrepareParams ¶ms, + RPiController::Metadata &rpiMetadata) = 0; + virtual RPiController::StatisticsPtr platformProcessStats(Span<uint8_t> mem) = 0; + + void setMode(const IPACameraSensorInfo &sensorInfo); + void setCameraTimeoutValue(); + bool validateSensorControls(); + bool validateLensControls(); + void applyControls(const ControlList &controls); + virtual void handleControls(const ControlList &controls) = 0; + void fillDeviceStatus(const ControlList &sensorControls, unsigned int ipaContext); + void applyFrameDurations(utils::Duration minFrameDuration, utils::Duration maxFrameDuration); + void applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls); + + std::map<unsigned int, MappedFrameBuffer> buffers_; + + bool lensPresent_; + ControlList libcameraMetadata_; + + std::array<RPiController::Metadata, numMetadataContexts> rpiMetadata_; + + /* + * We count frames to decide if the frame must be hidden (e.g. from + * display) or mistrusted (i.e. not given to the control algos). + */ + uint64_t frameCount_; + + /* How many frames we should avoid running control algos on. */ + unsigned int mistrustCount_; + + /* Number of frames that need to be dropped on startup. */ + unsigned int dropFrameCount_; + + /* Frame timestamp for the last run of the controller. */ + uint64_t lastRunTimestamp_; + + /* Do we run a Controller::process() for this frame? */ + bool processPending_; + + /* Distinguish the first camera start from others. */ + bool firstStart_; + + /* Frame duration (1/fps) limits. */ + utils::Duration minFrameDuration_; + utils::Duration maxFrameDuration_; + +protected: + /* Raspberry Pi controller specific defines. */ + std::unique_ptr<RPiController::CamHelper> helper_; + RPiController::Controller controller_; + + ControlInfoMap sensorCtrls_; + ControlInfoMap lensCtrls_; + + /* Camera sensor params. */ + CameraMode mode_; + + /* Track the frame length times over FrameLengthsQueueSize frames. */ + std::deque<utils::Duration> frameLengths_; + utils::Duration lastTimeout_; +}; + +} /* namespace ipa::RPi */ + +} /* namespace libcamera */ diff --git a/src/ipa/rpi/common/meson.build b/src/ipa/rpi/common/meson.build new file mode 100644 index 000000000000..a7189f8389af --- /dev/null +++ b/src/ipa/rpi/common/meson.build @@ -0,0 +1,7 @@ +# SPDX-License-Identifier: CC0-1.0 + +# SPDX-License-Identifier: CC0-1.0 + +rpi_ipa_common_sources = files([ + 'ipa_base.cpp', +]) diff --git a/src/ipa/rpi/vc4/meson.build b/src/ipa/rpi/vc4/meson.build index 992d0f1ab5a7..b581391586b3 100644 --- a/src/ipa/rpi/vc4/meson.build +++ b/src/ipa/rpi/vc4/meson.build @@ -13,11 +13,15 @@ vc4_ipa_includes = [ ] vc4_ipa_sources = files([ - 'raspberrypi.cpp', + 'vc4.cpp', ]) vc4_ipa_includes += include_directories('..') -vc4_ipa_sources += [rpi_ipa_cam_helper_sources, rpi_ipa_controller_sources] +vc4_ipa_sources += [ + rpi_ipa_cam_helper_sources, + rpi_ipa_common_sources, + rpi_ipa_controller_sources, +] mod = shared_module(ipa_name, [vc4_ipa_sources, libcamera_generated_ipa_headers], diff --git a/src/ipa/rpi/vc4/vc4.cpp b/src/ipa/rpi/vc4/vc4.cpp new file mode 100644 index 000000000000..0d929cda6c4a --- /dev/null +++ b/src/ipa/rpi/vc4/vc4.cpp @@ -0,0 +1,540 @@ +/* SPDX-License-Identifier: BSD-2-Clause */ +/* + * Copyright (C) 2019-2021, Raspberry Pi Ltd + * + * rpi.cpp - Raspberry Pi VC4/BCM2835 ISP IPA. + */ + +#include <string.h> +#include <sys/mman.h> + +#include <linux/bcm2835-isp.h> + +#include <libcamera/base/log.h> + +#include "common/ipa_base.h" +#include "controller/af_status.h" +#include "controller/agc_algorithm.h" +#include "controller/alsc_status.h" +#include "controller/awb_status.h" +#include "controller/black_level_status.h" +#include "controller/ccm_status.h" +#include "controller/contrast_status.h" +#include "controller/denoise_algorithm.h" +#include "controller/denoise_status.h" +#include "controller/dpc_status.h" +#include "controller/geq_status.h" +#include "controller/lux_status.h" +#include "controller/noise_status.h" +#include "controller/sharpen_status.h" + +namespace libcamera { + +LOG_DECLARE_CATEGORY(IPARPI) + +namespace ipa::RPi { + +class IpaVc4 final : public IpaBase +{ +public: + IpaVc4() + : IpaBase(), lsTable_(nullptr) + { + } + + ~IpaVc4() + { + if (lsTable_) + munmap(lsTable_, MaxLsGridSize); + } + + +private: + int32_t platformInit(const InitParams ¶ms, InitResult *result) override; + int32_t platformConfigure(const ConfigParams ¶ms, ConfigResult *result) override; + + void platformPrepareIsp(const PrepareParams ¶ms, RPiController::Metadata &rpiMetadata) override; + RPiController::StatisticsPtr platformProcessStats(Span<uint8_t> mem) override; + + void handleControls(const ControlList &controls) override; + bool validateIspControls(); + + void applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls); + void applyDG(const struct AgcStatus *dgStatus, ControlList &ctrls); + void applyCCM(const struct CcmStatus *ccmStatus, ControlList &ctrls); + void applyBlackLevel(const struct BlackLevelStatus *blackLevelStatus, ControlList &ctrls); + void applyGamma(const struct ContrastStatus *contrastStatus, ControlList &ctrls); + void applyGEQ(const struct GeqStatus *geqStatus, ControlList &ctrls); + void applyDenoise(const struct DenoiseStatus *denoiseStatus, ControlList &ctrls); + void applySharpen(const struct SharpenStatus *sharpenStatus, ControlList &ctrls); + void applyDPC(const struct DpcStatus *dpcStatus, ControlList &ctrls); + void applyLS(const struct AlscStatus *lsStatus, ControlList &ctrls); + void applyAF(const struct AfStatus *afStatus, ControlList &lensCtrls); + void resampleTable(uint16_t dest[], const std::vector<double> &src, int destW, int destH); + + /* VC4 ISP controls. */ + ControlInfoMap ispCtrls_; + + /* LS table allocation passed in from the pipeline handler. */ + SharedFD lsTableHandle_; + void *lsTable_; +}; + +int32_t IpaVc4::platformInit([[maybe_unused]] const InitParams ¶ms, [[maybe_unused]] InitResult *result) +{ + const std::string &target = controller_.getTarget(); + + if (target != "bcm2835") { + LOG(IPARPI, Error) + << "Tuning data file target returned \"" << target << "\"" + << ", expected \"bcm2835\""; + return -EINVAL; + } + + return 0; +} + +int32_t IpaVc4::platformConfigure(const ConfigParams ¶ms, [[maybe_unused]] ConfigResult *result) +{ + ispCtrls_ = params.ispControls; + if (!validateIspControls()) { + LOG(IPARPI, Error) << "ISP control validation failed."; + return -1; + } + + /* Store the lens shading table pointer and handle if available. */ + if (params.lsTableHandle.isValid()) { + /* Remove any previous table, if there was one. */ + if (lsTable_) { + munmap(lsTable_, MaxLsGridSize); + lsTable_ = nullptr; + } + + /* Map the LS table buffer into user space. */ + lsTableHandle_ = std::move(params.lsTableHandle); + if (lsTableHandle_.isValid()) { + lsTable_ = mmap(nullptr, MaxLsGridSize, PROT_READ | PROT_WRITE, + MAP_SHARED, lsTableHandle_.get(), 0); + + if (lsTable_ == MAP_FAILED) { + LOG(IPARPI, Error) << "dmaHeap mmap failure for LS table."; + lsTable_ = nullptr; + } + } + } + + return 0; +} + +void IpaVc4::platformPrepareIsp([[maybe_unused]] const PrepareParams ¶ms, + RPiController::Metadata &rpiMetadata) +{ + ControlList ctrls(ispCtrls_); + + /* Lock the metadata buffer to avoid constant locks/unlocks. */ + std::unique_lock<RPiController::Metadata> lock(rpiMetadata); + + AwbStatus *awbStatus = rpiMetadata.getLocked<AwbStatus>("awb.status"); + if (awbStatus) + applyAWB(awbStatus, ctrls); + + CcmStatus *ccmStatus = rpiMetadata.getLocked<CcmStatus>("ccm.status"); + if (ccmStatus) + applyCCM(ccmStatus, ctrls); + + AgcStatus *dgStatus = rpiMetadata.getLocked<AgcStatus>("agc.status"); + if (dgStatus) + applyDG(dgStatus, ctrls); + + AlscStatus *lsStatus = rpiMetadata.getLocked<AlscStatus>("alsc.status"); + if (lsStatus) + applyLS(lsStatus, ctrls); + + ContrastStatus *contrastStatus = rpiMetadata.getLocked<ContrastStatus>("contrast.status"); + if (contrastStatus) + applyGamma(contrastStatus, ctrls); + + BlackLevelStatus *blackLevelStatus = rpiMetadata.getLocked<BlackLevelStatus>("black_level.status"); + if (blackLevelStatus) + applyBlackLevel(blackLevelStatus, ctrls); + + GeqStatus *geqStatus = rpiMetadata.getLocked<GeqStatus>("geq.status"); + if (geqStatus) + applyGEQ(geqStatus, ctrls); + + DenoiseStatus *denoiseStatus = rpiMetadata.getLocked<DenoiseStatus>("denoise.status"); + if (denoiseStatus) + applyDenoise(denoiseStatus, ctrls); + + SharpenStatus *sharpenStatus = rpiMetadata.getLocked<SharpenStatus>("sharpen.status"); + if (sharpenStatus) + applySharpen(sharpenStatus, ctrls); + + DpcStatus *dpcStatus = rpiMetadata.getLocked<DpcStatus>("dpc.status"); + if (dpcStatus) + applyDPC(dpcStatus, ctrls); + + const AfStatus *afStatus = rpiMetadata.getLocked<AfStatus>("af.status"); + if (afStatus) { + ControlList lensctrls(lensCtrls_); + applyAF(afStatus, lensctrls); + if (!lensctrls.empty()) + setLensControls.emit(lensctrls); + } + + if (!ctrls.empty()) + setIspControls.emit(ctrls); +} + +RPiController::StatisticsPtr IpaVc4::platformProcessStats(Span<uint8_t> mem) +{ + using namespace RPiController; + + const bcm2835_isp_stats *stats = reinterpret_cast<bcm2835_isp_stats *>(mem.data()); + StatisticsPtr statistics = std::make_unique<Statistics>(Statistics::AgcStatsPos::PreWb, + Statistics::ColourStatsPos::PostLsc); + const Controller::HardwareConfig &hw = controller_.getHardwareConfig(); + unsigned int i; + + /* RGB histograms are not used, so do not populate them. */ + statistics->yHist = RPiController::Histogram(stats->hist[0].g_hist, + hw.numHistogramBins); + + /* All region sums are based on a 16-bit normalised pipeline bit-depth. */ + unsigned int scale = Statistics::NormalisationFactorPow2 - hw.pipelineWidth; + + statistics->awbRegions.init(hw.awbRegions); + for (i = 0; i < statistics->awbRegions.numRegions(); i++) + statistics->awbRegions.set(i, { { stats->awb_stats[i].r_sum << scale, + stats->awb_stats[i].g_sum << scale, + stats->awb_stats[i].b_sum << scale }, + stats->awb_stats[i].counted, + stats->awb_stats[i].notcounted }); + + statistics->agcRegions.init(hw.agcRegions); + for (i = 0; i < statistics->agcRegions.numRegions(); i++) + statistics->agcRegions.set(i, { { stats->agc_stats[i].r_sum << scale, + stats->agc_stats[i].g_sum << scale, + stats->agc_stats[i].b_sum << scale }, + stats->agc_stats[i].counted, + stats->awb_stats[i].notcounted }); + + statistics->focusRegions.init(hw.focusRegions); + for (i = 0; i < statistics->focusRegions.numRegions(); i++) + statistics->focusRegions.set(i, { stats->focus_stats[i].contrast_val[1][1] / 1000, + stats->focus_stats[i].contrast_val_num[1][1], + stats->focus_stats[i].contrast_val_num[1][0] }); + + return statistics; +} + +void IpaVc4::handleControls([[maybe_unused]] const ControlList &controls) +{ + /* No controls require any special updates to the hardware configuration. */ +} + +bool IpaVc4::validateIspControls() +{ + static const uint32_t ctrls[] = { + V4L2_CID_RED_BALANCE, + V4L2_CID_BLUE_BALANCE, + V4L2_CID_DIGITAL_GAIN, + V4L2_CID_USER_BCM2835_ISP_CC_MATRIX, + V4L2_CID_USER_BCM2835_ISP_GAMMA, + V4L2_CID_USER_BCM2835_ISP_BLACK_LEVEL, + V4L2_CID_USER_BCM2835_ISP_GEQ, + V4L2_CID_USER_BCM2835_ISP_DENOISE, + V4L2_CID_USER_BCM2835_ISP_SHARPEN, + V4L2_CID_USER_BCM2835_ISP_DPC, + V4L2_CID_USER_BCM2835_ISP_LENS_SHADING, + V4L2_CID_USER_BCM2835_ISP_CDN, + }; + + for (auto c : ctrls) { + if (ispCtrls_.find(c) == ispCtrls_.end()) { + LOG(IPARPI, Error) << "Unable to find ISP control " + << utils::hex(c); + return false; + } + } + + return true; +} + +void IpaVc4::applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls) +{ + LOG(IPARPI, Debug) << "Applying WB R: " << awbStatus->gainR << " B: " + << awbStatus->gainB; + + ctrls.set(V4L2_CID_RED_BALANCE, + static_cast<int32_t>(awbStatus->gainR * 1000)); + ctrls.set(V4L2_CID_BLUE_BALANCE, + static_cast<int32_t>(awbStatus->gainB * 1000)); +} + +void IpaVc4::applyDG(const struct AgcStatus *dgStatus, ControlList &ctrls) +{ + ctrls.set(V4L2_CID_DIGITAL_GAIN, + static_cast<int32_t>(dgStatus->digitalGain * 1000)); +} + +void IpaVc4::applyCCM(const struct CcmStatus *ccmStatus, ControlList &ctrls) +{ + bcm2835_isp_custom_ccm ccm; + + for (int i = 0; i < 9; i++) { + ccm.ccm.ccm[i / 3][i % 3].den = 1000; + ccm.ccm.ccm[i / 3][i % 3].num = 1000 * ccmStatus->matrix[i]; + } + + ccm.enabled = 1; + ccm.ccm.offsets[0] = ccm.ccm.offsets[1] = ccm.ccm.offsets[2] = 0; + + ControlValue c(Span<const uint8_t>{ reinterpret_cast<uint8_t *>(&ccm), + sizeof(ccm) }); + ctrls.set(V4L2_CID_USER_BCM2835_ISP_CC_MATRIX, c); +} + +void IpaVc4::applyBlackLevel(const struct BlackLevelStatus *blackLevelStatus, ControlList &ctrls) +{ + bcm2835_isp_black_level blackLevel; + + blackLevel.enabled = 1; + blackLevel.black_level_r = blackLevelStatus->blackLevelR; + blackLevel.black_level_g = blackLevelStatus->blackLevelG; + blackLevel.black_level_b = blackLevelStatus->blackLevelB; + + ControlValue c(Span<const uint8_t>{ reinterpret_cast<uint8_t *>(&blackLevel), + sizeof(blackLevel) }); + ctrls.set(V4L2_CID_USER_BCM2835_ISP_BLACK_LEVEL, c); +} + +void IpaVc4::applyGamma(const struct ContrastStatus *contrastStatus, ControlList &ctrls) +{ + const unsigned int numGammaPoints = controller_.getHardwareConfig().numGammaPoints; + struct bcm2835_isp_gamma gamma; + + for (unsigned int i = 0; i < numGammaPoints - 1; i++) { + int x = i < 16 ? i * 1024 + : (i < 24 ? (i - 16) * 2048 + 16384 + : (i - 24) * 4096 + 32768); + gamma.x[i] = x; + gamma.y[i] = std::min<uint16_t>(65535, contrastStatus->gammaCurve.eval(x)); + } + + gamma.x[numGammaPoints - 1] = 65535; + gamma.y[numGammaPoints - 1] = 65535; + gamma.enabled = 1; + + ControlValue c(Span<const uint8_t>{ reinterpret_cast<uint8_t *>(&gamma), + sizeof(gamma) }); + ctrls.set(V4L2_CID_USER_BCM2835_ISP_GAMMA, c); +} + +void IpaVc4::applyGEQ(const struct GeqStatus *geqStatus, ControlList &ctrls) +{ + bcm2835_isp_geq geq; + + geq.enabled = 1; + geq.offset = geqStatus->offset; + geq.slope.den = 1000; + geq.slope.num = 1000 * geqStatus->slope; + + ControlValue c(Span<const uint8_t>{ reinterpret_cast<uint8_t *>(&geq), + sizeof(geq) }); + ctrls.set(V4L2_CID_USER_BCM2835_ISP_GEQ, c); +} + +void IpaVc4::applyDenoise(const struct DenoiseStatus *denoiseStatus, ControlList &ctrls) +{ + using RPiController::DenoiseMode; + + bcm2835_isp_denoise denoise; + DenoiseMode mode = static_cast<DenoiseMode>(denoiseStatus->mode); + + denoise.enabled = mode != DenoiseMode::Off; + denoise.constant = denoiseStatus->noiseConstant; + denoise.slope.num = 1000 * denoiseStatus->noiseSlope; + denoise.slope.den = 1000; + denoise.strength.num = 1000 * denoiseStatus->strength; + denoise.strength.den = 1000; + + /* Set the CDN mode to match the SDN operating mode. */ + bcm2835_isp_cdn cdn; + switch (mode) { + case DenoiseMode::ColourFast: + cdn.enabled = 1; + cdn.mode = CDN_MODE_FAST; + break; + case DenoiseMode::ColourHighQuality: + cdn.enabled = 1; + cdn.mode = CDN_MODE_HIGH_QUALITY; + break; + default: + cdn.enabled = 0; + } + + ControlValue c(Span<const uint8_t>{ reinterpret_cast<uint8_t *>(&denoise), + sizeof(denoise) }); + ctrls.set(V4L2_CID_USER_BCM2835_ISP_DENOISE, c); + + c = ControlValue(Span<const uint8_t>{ reinterpret_cast<uint8_t *>(&cdn), + sizeof(cdn) }); + ctrls.set(V4L2_CID_USER_BCM2835_ISP_CDN, c); +} + +void IpaVc4::applySharpen(const struct SharpenStatus *sharpenStatus, ControlList &ctrls) +{ + bcm2835_isp_sharpen sharpen; + + sharpen.enabled = 1; + sharpen.threshold.num = 1000 * sharpenStatus->threshold; + sharpen.threshold.den = 1000; + sharpen.strength.num = 1000 * sharpenStatus->strength; + sharpen.strength.den = 1000; + sharpen.limit.num = 1000 * sharpenStatus->limit; + sharpen.limit.den = 1000; + + ControlValue c(Span<const uint8_t>{ reinterpret_cast<uint8_t *>(&sharpen), + sizeof(sharpen) }); + ctrls.set(V4L2_CID_USER_BCM2835_ISP_SHARPEN, c); +} + +void IpaVc4::applyDPC(const struct DpcStatus *dpcStatus, ControlList &ctrls) +{ + bcm2835_isp_dpc dpc; + + dpc.enabled = 1; + dpc.strength = dpcStatus->strength; + + ControlValue c(Span<const uint8_t>{ reinterpret_cast<uint8_t *>(&dpc), + sizeof(dpc) }); + ctrls.set(V4L2_CID_USER_BCM2835_ISP_DPC, c); +} + +void IpaVc4::applyLS(const struct AlscStatus *lsStatus, ControlList &ctrls) +{ + /* + * Program lens shading tables into pipeline. + * Choose smallest cell size that won't exceed 63x48 cells. + */ + const int cellSizes[] = { 16, 32, 64, 128, 256 }; + unsigned int numCells = std::size(cellSizes); + unsigned int i, w, h, cellSize; + for (i = 0; i < numCells; i++) { + cellSize = cellSizes[i]; + w = (mode_.width + cellSize - 1) / cellSize; + h = (mode_.height + cellSize - 1) / cellSize; + if (w < 64 && h <= 48) + break; + } + + if (i == numCells) { + LOG(IPARPI, Error) << "Cannot find cell size"; + return; + } + + /* We're going to supply corner sampled tables, 16 bit samples. */ + w++, h++; + bcm2835_isp_lens_shading ls = { + .enabled = 1, + .grid_cell_size = cellSize, + .grid_width = w, + .grid_stride = w, + .grid_height = h, + /* .dmabuf will be filled in by pipeline handler. */ + .dmabuf = 0, + .ref_transform = 0, + .corner_sampled = 1, + .gain_format = GAIN_FORMAT_U4P10 + }; + + if (!lsTable_ || w * h * 4 * sizeof(uint16_t) > MaxLsGridSize) { + LOG(IPARPI, Error) << "Do not have a correctly allocate lens shading table!"; + return; + } + + if (lsStatus) { + /* Format will be u4.10 */ + uint16_t *grid = static_cast<uint16_t *>(lsTable_); + + resampleTable(grid, lsStatus->r, w, h); + resampleTable(grid + w * h, lsStatus->g, w, h); + memcpy(grid + 2 * w * h, grid + w * h, w * h * sizeof(uint16_t)); + resampleTable(grid + 3 * w * h, lsStatus->b, w, h); + } + + ControlValue c(Span<const uint8_t>{ reinterpret_cast<uint8_t *>(&ls), + sizeof(ls) }); + ctrls.set(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING, c); +} + +void IpaVc4::applyAF(const struct AfStatus *afStatus, ControlList &lensCtrls) +{ + if (afStatus->lensSetting) { + ControlValue v(afStatus->lensSetting.value()); + lensCtrls.set(V4L2_CID_FOCUS_ABSOLUTE, v); + } +} + +/* + * Resamples a 16x12 table with central sampling to destW x destH with corner + * sampling. + */ +void IpaVc4::resampleTable(uint16_t dest[], const std::vector<double> &src, + int destW, int destH) +{ + /* + * Precalculate and cache the x sampling locations and phases to + * save recomputing them on every row. + */ + assert(destW > 1 && destH > 1 && destW <= 64); + int xLo[64], xHi[64]; + double xf[64]; + double x = -0.5, xInc = 16.0 / (destW - 1); + for (int i = 0; i < destW; i++, x += xInc) { + xLo[i] = floor(x); + xf[i] = x - xLo[i]; + xHi[i] = xLo[i] < 15 ? xLo[i] + 1 : 15; + xLo[i] = xLo[i] > 0 ? xLo[i] : 0; + } + + /* Now march over the output table generating the new values. */ + double y = -0.5, yInc = 12.0 / (destH - 1); + for (int j = 0; j < destH; j++, y += yInc) { + int yLo = floor(y); + double yf = y - yLo; + int yHi = yLo < 11 ? yLo + 1 : 11; + yLo = yLo > 0 ? yLo : 0; + double const *rowAbove = src.data() + yLo * 16; + double const *rowBelow = src.data() + yHi * 16; + for (int i = 0; i < destW; i++) { + double above = rowAbove[xLo[i]] * (1 - xf[i]) + rowAbove[xHi[i]] * xf[i]; + double below = rowBelow[xLo[i]] * (1 - xf[i]) + rowBelow[xHi[i]] * xf[i]; + int result = floor(1024 * (above * (1 - yf) + below * yf) + .5); + *(dest++) = result > 16383 ? 16383 : result; /* want u4.10 */ + } + } +} + +} /* namespace ipa::RPi */ + +/* + * External IPA module interface + */ +extern "C" { +const struct IPAModuleInfo ipaModuleInfo = { + IPA_MODULE_API_VERSION, + 1, + "PipelineHandlerRPi", + "vc4", +}; + +IPAInterface *ipaCreate() +{ + return new ipa::RPi::IpaVc4(); +} + +} /* extern "C" */ + +} /* namespace libcamera */
Create a new IpaBase class that handles general purpose housekeeping duties for the Raspberry Pi IPA. Code the implementation of new class is essentially pulled from the existing ipa/rpi/vc4/raspberrypi.cpp file with a minimal amount of refactoring. Create a derived IpaVc4 class from IpaBase that handles the VC4 pipeline specific tasks of the IPA. Again, code for this class implementation is taken from the existing ipa/rpi/vc4/raspberrypi.cpp with a minimal amount of refactoring. The goal of this change is to allow third parties to implement their own IPA running on the Raspberry Pi without duplicating all of the IPA housekeeping tasks. Signed-off-by: Naushir Patuck <naush@raspberrypi.com> --- src/ipa/meson.build | 5 +- .../raspberrypi.cpp => common/ipa_base.cpp} | 1219 +++++------------ src/ipa/rpi/common/ipa_base.h | 125 ++ src/ipa/rpi/common/meson.build | 7 + src/ipa/rpi/vc4/meson.build | 8 +- src/ipa/rpi/vc4/vc4.cpp | 540 ++++++++ 6 files changed, 1012 insertions(+), 892 deletions(-) rename src/ipa/rpi/{vc4/raspberrypi.cpp => common/ipa_base.cpp} (65%) create mode 100644 src/ipa/rpi/common/ipa_base.h create mode 100644 src/ipa/rpi/common/meson.build create mode 100644 src/ipa/rpi/vc4/vc4.cpp