[{"id":26959,"web_url":"https://patchwork.libcamera.org/comment/26959/","msgid":"<6a34pfgd7zfbupsonwttin6qo5kvlgjo2guqrqksvj6c5ivs35@sm2iazri3glv>","date":"2023-04-27T10:00:37","subject":"Re: [libcamera-devel] [PATCH 08/13] ipa: raspberrypi: Introduce\n\tIpaBase class","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Naush\n\nOn Wed, Apr 26, 2023 at 02:10:52PM +0100, Naushir Patuck via libcamera-devel wrote:\n> Create a new IpaBase class that handles general purpose housekeeping\n> duties for the Raspberry Pi IPA. Code the implementation of new class is\n> essentially pulled from the existing ipa/rpi/vc4/raspberrypi.cpp\n> file with a minimal amount of refactoring.\n>\n> Create a derived IpaVc4 class from IpaBase that handles the VC4 pipeline\n> specific tasks of the IPA. Again, code for this class implementation is\n> taken from the existing ipa/rpi/vc4/raspberrypi.cpp with a\n> minimal amount of refactoring.\n>\n> The goal of this change is to allow third parties to implement their own\n> IPA running on the Raspberry Pi without duplicating all of the IPA\n> housekeeping tasks.\n>\n> Signed-off-by: Naushir Patuck \n> ---\n> src/ipa/meson.build | 5 +-\n> .../raspberrypi.cpp => common/ipa_base.cpp} | 1219 +++++------------\n> src/ipa/rpi/common/ipa_base.h | 125 ++\n> src/ipa/rpi/common/meson.build | 7 +\n> src/ipa/rpi/vc4/meson.build | 8 +-\n> src/ipa/rpi/vc4/vc4.cpp | 540 ++++++++\n> 6 files changed, 1012 insertions(+), 892 deletions(-)\n> rename src/ipa/rpi/{vc4/raspberrypi.cpp => common/ipa_base.cpp} (65%)\n> create mode 100644 src/ipa/rpi/common/ipa_base.h\n> create mode 100644 src/ipa/rpi/common/meson.build\n> create mode 100644 src/ipa/rpi/vc4/vc4.cpp\n>\n\n[snip]\n\n>\n> vc4_ipa_includes += include_directories('..')\n> -vc4_ipa_sources += [rpi_ipa_cam_helper_sources, rpi_ipa_controller_sources]\n> +vc4_ipa_sources += [\n> + rpi_ipa_cam_helper_sources,\n> + rpi_ipa_common_sources,\n> + rpi_ipa_controller_sources,\n> +]\n>\n> mod = shared_module(ipa_name,\n> [vc4_ipa_sources, libcamera_generated_ipa_headers],\n> diff --git a/src/ipa/rpi/vc4/vc4.cpp b/src/ipa/rpi/vc4/vc4.cpp\n> new file mode 100644\n> index 000000000000..0d929cda6c4a\n> --- /dev/null\n> +++ b/src/ipa/rpi/vc4/vc4.cpp\n> @@ -0,0 +1,540 @@\n> +/* SPDX-License-Identifier: BSD-2-Clause */\n> +/*\n> + * Copyright (C) 2019-2021, Raspberry Pi Ltd\n> + *\n> + * rpi.cpp - Raspberry Pi VC4/BCM2835 ISP IPA.\n> + */\n> +\n> +#include \n> +#include \n> +\n> +#include \n> +\n> +#include \n> +\n> +#include \"common/ipa_base.h\"\n> +#include \"controller/af_status.h\"\n> +#include \"controller/agc_algorithm.h\"\n> +#include \"controller/alsc_status.h\"\n> +#include \"controller/awb_status.h\"\n> +#include \"controller/black_level_status.h\"\n> +#include \"controller/ccm_status.h\"\n> +#include \"controller/contrast_status.h\"\n> +#include \"controller/denoise_algorithm.h\"\n> +#include \"controller/denoise_status.h\"\n> +#include \"controller/dpc_status.h\"\n> +#include \"controller/geq_status.h\"\n> +#include \"controller/lux_status.h\"\n> +#include \"controller/noise_status.h\"\n> +#include \"controller/sharpen_status.h\"\n> +\n> +namespace libcamera {\n> +\n> +LOG_DECLARE_CATEGORY(IPARPI)\n> +\n> +namespace ipa::RPi {\n> +\n> +class IpaVc4 final : public IpaBase\n> +{\n> +public:\n> +\tIpaVc4()\n> +\t\t: IpaBase(), lsTable_(nullptr)\n> +\t{\n> +\t}\n> +\n> +\t~IpaVc4()\n> +\t{\n> +\t\tif (lsTable_)\n> +\t\t\tmunmap(lsTable_, MaxLsGridSize);\n> +\t}\n> +\n> +\n\nAdditional blank line\n\nThe rest looks good, I haven't gone in detail in the code as it's\nmostly copied from the existing implementations but the class desig\nlooks correct to me\n\nReviewed-by: Jacopo Mondi \n\nThanks\n j\n\n> +private:\n> +\tint32_t platformInit(const InitParams ¶ms, InitResult *result) override;\n> +\tint32_t platformConfigure(const ConfigParams ¶ms, ConfigResult *result) override;\n> +\n> +\tvoid platformPrepareIsp(const PrepareParams ¶ms, RPiController::Metadata &rpiMetadata) override;\n> +\tRPiController::StatisticsPtr platformProcessStats(Span mem) override;\n> +\n> +\tvoid handleControls(const ControlList &controls) override;\n> +\tbool validateIspControls();\n> +\n> +\tvoid applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls);\n> +\tvoid applyDG(const struct AgcStatus *dgStatus, ControlList &ctrls);\n> +\tvoid applyCCM(const struct CcmStatus *ccmStatus, ControlList &ctrls);\n> +\tvoid applyBlackLevel(const struct BlackLevelStatus *blackLevelStatus, ControlList &ctrls);\n> +\tvoid applyGamma(const struct ContrastStatus *contrastStatus, ControlList &ctrls);\n> +\tvoid applyGEQ(const struct GeqStatus *geqStatus, ControlList &ctrls);\n> +\tvoid applyDenoise(const struct DenoiseStatus *denoiseStatus, ControlList &ctrls);\n> +\tvoid applySharpen(const struct SharpenStatus *sharpenStatus, ControlList &ctrls);\n> +\tvoid applyDPC(const struct DpcStatus *dpcStatus, ControlList &ctrls);\n> +\tvoid applyLS(const struct AlscStatus *lsStatus, ControlList &ctrls);\n> +\tvoid applyAF(const struct AfStatus *afStatus, ControlList &lensCtrls);\n> +\tvoid resampleTable(uint16_t dest[], const std::vector &src, int destW, int destH);\n> +\n> +\t/* VC4 ISP controls. */\n> +\tControlInfoMap ispCtrls_;\n> +\n> +\t/* LS table allocation passed in from the pipeline handler. */\n> +\tSharedFD lsTableHandle_;\n> +\tvoid *lsTable_;\n> +};\n> +\n> +int32_t IpaVc4::platformInit([[maybe_unused]] const InitParams ¶ms, [[maybe_unused]] InitResult *result)\n> +{\n> +\tconst std::string &target = controller_.getTarget();\n> +\n> +\tif (target != \"bcm2835\") {\n> +\t\tLOG(IPARPI, Error)\n> +\t\t\t<< \"Tuning data file target returned \\\"\" << target << \"\\\"\"\n> +\t\t\t<< \", expected \\\"bcm2835\\\"\";\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\treturn 0;\n> +}\n> +\n> +int32_t IpaVc4::platformConfigure(const ConfigParams ¶ms, [[maybe_unused]] ConfigResult *result)\n> +{\n> +\tispCtrls_ = params.ispControls;\n> +\tif (!validateIspControls()) {\n> +\t\tLOG(IPARPI, Error) << \"ISP control validation failed.\";\n> +\t\treturn -1;\n> +\t}\n> +\n> +\t/* Store the lens shading table pointer and handle if available. */\n> +\tif (params.lsTableHandle.isValid()) {\n> +\t\t/* Remove any previous table, if there was one. */\n> +\t\tif (lsTable_) {\n> +\t\t\tmunmap(lsTable_, MaxLsGridSize);\n> +\t\t\tlsTable_ = nullptr;\n> +\t\t}\n> +\n> +\t\t/* Map the LS table buffer into user space. */\n> +\t\tlsTableHandle_ = std::move(params.lsTableHandle);\n> +\t\tif (lsTableHandle_.isValid()) {\n> +\t\t\tlsTable_ = mmap(nullptr, MaxLsGridSize, PROT_READ | PROT_WRITE,\n> +\t\t\t\t\tMAP_SHARED, lsTableHandle_.get(), 0);\n> +\n> +\t\t\tif (lsTable_ == MAP_FAILED) {\n> +\t\t\t\tLOG(IPARPI, Error) << \"dmaHeap mmap failure for LS table.\";\n> +\t\t\t\tlsTable_ = nullptr;\n> +\t\t\t}\n> +\t\t}\n> +\t}\n> +\n> +\treturn 0;\n> +}\n> +\n> +void IpaVc4::platformPrepareIsp([[maybe_unused]] const PrepareParams ¶ms,\n> +\t\t\t\tRPiController::Metadata &rpiMetadata)\n> +{\n> +\tControlList ctrls(ispCtrls_);\n> +\n> +\t/* Lock the metadata buffer to avoid constant locks/unlocks. */\n> +\tstd::unique_lock lock(rpiMetadata);\n> +\n> +\tAwbStatus *awbStatus = rpiMetadata.getLocked(\"awb.status\");\n> +\tif (awbStatus)\n> +\t\tapplyAWB(awbStatus, ctrls);\n> +\n> +\tCcmStatus *ccmStatus = rpiMetadata.getLocked(\"ccm.status\");\n> +\tif (ccmStatus)\n> +\t\tapplyCCM(ccmStatus, ctrls);\n> +\n> +\tAgcStatus *dgStatus = rpiMetadata.getLocked(\"agc.status\");\n> +\tif (dgStatus)\n> +\t\tapplyDG(dgStatus, ctrls);\n> +\n> +\tAlscStatus *lsStatus = rpiMetadata.getLocked(\"alsc.status\");\n> +\tif (lsStatus)\n> +\t\tapplyLS(lsStatus, ctrls);\n> +\n> +\tContrastStatus *contrastStatus = rpiMetadata.getLocked(\"contrast.status\");\n> +\tif (contrastStatus)\n> +\t\tapplyGamma(contrastStatus, ctrls);\n> +\n> +\tBlackLevelStatus *blackLevelStatus = rpiMetadata.getLocked(\"black_level.status\");\n> +\tif (blackLevelStatus)\n> +\t\tapplyBlackLevel(blackLevelStatus, ctrls);\n> +\n> +\tGeqStatus *geqStatus = rpiMetadata.getLocked(\"geq.status\");\n> +\tif (geqStatus)\n> +\t\tapplyGEQ(geqStatus, ctrls);\n> +\n> +\tDenoiseStatus *denoiseStatus = rpiMetadata.getLocked(\"denoise.status\");\n> +\tif (denoiseStatus)\n> +\t\tapplyDenoise(denoiseStatus, ctrls);\n> +\n> +\tSharpenStatus *sharpenStatus = rpiMetadata.getLocked(\"sharpen.status\");\n> +\tif (sharpenStatus)\n> +\t\tapplySharpen(sharpenStatus, ctrls);\n> +\n> +\tDpcStatus *dpcStatus = rpiMetadata.getLocked(\"dpc.status\");\n> +\tif (dpcStatus)\n> +\t\tapplyDPC(dpcStatus, ctrls);\n> +\n> +\tconst AfStatus *afStatus = rpiMetadata.getLocked(\"af.status\");\n> +\tif (afStatus) {\n> +\t\tControlList lensctrls(lensCtrls_);\n> +\t\tapplyAF(afStatus, lensctrls);\n> +\t\tif (!lensctrls.empty())\n> +\t\t\tsetLensControls.emit(lensctrls);\n> +\t}\n> +\n> +\tif (!ctrls.empty())\n> +\t\tsetIspControls.emit(ctrls);\n> +}\n> +\n> +RPiController::StatisticsPtr IpaVc4::platformProcessStats(Span mem)\n> +{\n> +\tusing namespace RPiController;\n> +\n> +\tconst bcm2835_isp_stats *stats = reinterpret_cast(mem.data());\n> +\tStatisticsPtr statistics = std::make_unique(Statistics::AgcStatsPos::PreWb,\n> +\t\t\t\t\t\t\t\tStatistics::ColourStatsPos::PostLsc);\n> +\tconst Controller::HardwareConfig &hw = controller_.getHardwareConfig();\n> +\tunsigned int i;\n> +\n> +\t/* RGB histograms are not used, so do not populate them. */\n> +\tstatistics->yHist = RPiController::Histogram(stats->hist[0].g_hist,\n> +\t\t\t\t\t\t hw.numHistogramBins);\n> +\n> +\t/* All region sums are based on a 16-bit normalised pipeline bit-depth. */\n> +\tunsigned int scale = Statistics::NormalisationFactorPow2 - hw.pipelineWidth;\n> +\n> +\tstatistics->awbRegions.init(hw.awbRegions);\n> +\tfor (i = 0; i < statistics->awbRegions.numRegions(); i++)\n> +\t\tstatistics->awbRegions.set(i, { { stats->awb_stats[i].r_sum << scale,\n> +\t\t\t\t\t\t stats->awb_stats[i].g_sum << scale,\n> +\t\t\t\t\t\t stats->awb_stats[i].b_sum << scale },\n> +\t\t\t\t\t\tstats->awb_stats[i].counted,\n> +\t\t\t\t\t\tstats->awb_stats[i].notcounted });\n> +\n> +\tstatistics->agcRegions.init(hw.agcRegions);\n> +\tfor (i = 0; i < statistics->agcRegions.numRegions(); i++)\n> +\t\tstatistics->agcRegions.set(i, { { stats->agc_stats[i].r_sum << scale,\n> +\t\t\t\t\t\t stats->agc_stats[i].g_sum << scale,\n> +\t\t\t\t\t\t stats->agc_stats[i].b_sum << scale },\n> +\t\t\t\t\t\tstats->agc_stats[i].counted,\n> +\t\t\t\t\t\tstats->awb_stats[i].notcounted });\n> +\n> +\tstatistics->focusRegions.init(hw.focusRegions);\n> +\tfor (i = 0; i < statistics->focusRegions.numRegions(); i++)\n> +\t\tstatistics->focusRegions.set(i, { stats->focus_stats[i].contrast_val[1][1] / 1000,\n> +\t\t\t\t\t\t stats->focus_stats[i].contrast_val_num[1][1],\n> +\t\t\t\t\t\t stats->focus_stats[i].contrast_val_num[1][0] });\n> +\n> +\treturn statistics;\n> +}\n> +\n> +void IpaVc4::handleControls([[maybe_unused]] const ControlList &controls)\n> +{\n> +\t/* No controls require any special updates to the hardware configuration. */\n> +}\n> +\n> +bool IpaVc4::validateIspControls()\n> +{\n> +\tstatic const uint32_t ctrls[] = {\n> +\t\tV4L2_CID_RED_BALANCE,\n> +\t\tV4L2_CID_BLUE_BALANCE,\n> +\t\tV4L2_CID_DIGITAL_GAIN,\n> +\t\tV4L2_CID_USER_BCM2835_ISP_CC_MATRIX,\n> +\t\tV4L2_CID_USER_BCM2835_ISP_GAMMA,\n> +\t\tV4L2_CID_USER_BCM2835_ISP_BLACK_LEVEL,\n> +\t\tV4L2_CID_USER_BCM2835_ISP_GEQ,\n> +\t\tV4L2_CID_USER_BCM2835_ISP_DENOISE,\n> +\t\tV4L2_CID_USER_BCM2835_ISP_SHARPEN,\n> +\t\tV4L2_CID_USER_BCM2835_ISP_DPC,\n> +\t\tV4L2_CID_USER_BCM2835_ISP_LENS_SHADING,\n> +\t\tV4L2_CID_USER_BCM2835_ISP_CDN,\n> +\t};\n> +\n> +\tfor (auto c : ctrls) {\n> +\t\tif (ispCtrls_.find(c) == ispCtrls_.end()) {\n> +\t\t\tLOG(IPARPI, Error) << \"Unable to find ISP control \"\n> +\t\t\t\t\t << utils::hex(c);\n> +\t\t\treturn false;\n> +\t\t}\n> +\t}\n> +\n> +\treturn true;\n> +}\n> +\n> +void IpaVc4::applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls)\n> +{\n> +\tLOG(IPARPI, Debug) << \"Applying WB R: \" << awbStatus->gainR << \" B: \"\n> +\t\t\t << awbStatus->gainB;\n> +\n> +\tctrls.set(V4L2_CID_RED_BALANCE,\n> +\t\t static_cast(awbStatus->gainR * 1000));\n> +\tctrls.set(V4L2_CID_BLUE_BALANCE,\n> +\t\t static_cast(awbStatus->gainB * 1000));\n> +}\n> +\n> +void IpaVc4::applyDG(const struct AgcStatus *dgStatus, ControlList &ctrls)\n> +{\n> +\tctrls.set(V4L2_CID_DIGITAL_GAIN,\n> +\t\t static_cast(dgStatus->digitalGain * 1000));\n> +}\n> +\n> +void IpaVc4::applyCCM(const struct CcmStatus *ccmStatus, ControlList &ctrls)\n> +{\n> +\tbcm2835_isp_custom_ccm ccm;\n> +\n> +\tfor (int i = 0; i < 9; i++) {\n> +\t\tccm.ccm.ccm[i / 3][i % 3].den = 1000;\n> +\t\tccm.ccm.ccm[i / 3][i % 3].num = 1000 * ccmStatus->matrix[i];\n> +\t}\n> +\n> +\tccm.enabled = 1;\n> +\tccm.ccm.offsets[0] = ccm.ccm.offsets[1] = ccm.ccm.offsets[2] = 0;\n> +\n> +\tControlValue c(Span{ reinterpret_cast(&ccm),\n> +\t\t\t\t\t sizeof(ccm) });\n> +\tctrls.set(V4L2_CID_USER_BCM2835_ISP_CC_MATRIX, c);\n> +}\n> +\n> +void IpaVc4::applyBlackLevel(const struct BlackLevelStatus *blackLevelStatus, ControlList &ctrls)\n> +{\n> +\tbcm2835_isp_black_level blackLevel;\n> +\n> +\tblackLevel.enabled = 1;\n> +\tblackLevel.black_level_r = blackLevelStatus->blackLevelR;\n> +\tblackLevel.black_level_g = blackLevelStatus->blackLevelG;\n> +\tblackLevel.black_level_b = blackLevelStatus->blackLevelB;\n> +\n> +\tControlValue c(Span{ reinterpret_cast(&blackLevel),\n> +\t\t\t\t\t sizeof(blackLevel) });\n> +\tctrls.set(V4L2_CID_USER_BCM2835_ISP_BLACK_LEVEL, c);\n> +}\n> +\n> +void IpaVc4::applyGamma(const struct ContrastStatus *contrastStatus, ControlList &ctrls)\n> +{\n> +\tconst unsigned int numGammaPoints = controller_.getHardwareConfig().numGammaPoints;\n> +\tstruct bcm2835_isp_gamma gamma;\n> +\n> +\tfor (unsigned int i = 0; i < numGammaPoints - 1; i++) {\n> +\t\tint x = i < 16 ? i * 1024\n> +\t\t\t : (i < 24 ? (i - 16) * 2048 + 16384\n> +\t\t\t\t\t : (i - 24) * 4096 + 32768);\n> +\t\tgamma.x[i] = x;\n> +\t\tgamma.y[i] = std::min(65535, contrastStatus->gammaCurve.eval(x));\n> +\t}\n> +\n> +\tgamma.x[numGammaPoints - 1] = 65535;\n> +\tgamma.y[numGammaPoints - 1] = 65535;\n> +\tgamma.enabled = 1;\n> +\n> +\tControlValue c(Span{ reinterpret_cast(&gamma),\n> +\t\t\t\t\t sizeof(gamma) });\n> +\tctrls.set(V4L2_CID_USER_BCM2835_ISP_GAMMA, c);\n> +}\n> +\n> +void IpaVc4::applyGEQ(const struct GeqStatus *geqStatus, ControlList &ctrls)\n> +{\n> +\tbcm2835_isp_geq geq;\n> +\n> +\tgeq.enabled = 1;\n> +\tgeq.offset = geqStatus->offset;\n> +\tgeq.slope.den = 1000;\n> +\tgeq.slope.num = 1000 * geqStatus->slope;\n> +\n> +\tControlValue c(Span{ reinterpret_cast(&geq),\n> +\t\t\t\t\t sizeof(geq) });\n> +\tctrls.set(V4L2_CID_USER_BCM2835_ISP_GEQ, c);\n> +}\n> +\n> +void IpaVc4::applyDenoise(const struct DenoiseStatus *denoiseStatus, ControlList &ctrls)\n> +{\n> +\tusing RPiController::DenoiseMode;\n> +\n> +\tbcm2835_isp_denoise denoise;\n> +\tDenoiseMode mode = static_cast(denoiseStatus->mode);\n> +\n> +\tdenoise.enabled = mode != DenoiseMode::Off;\n> +\tdenoise.constant = denoiseStatus->noiseConstant;\n> +\tdenoise.slope.num = 1000 * denoiseStatus->noiseSlope;\n> +\tdenoise.slope.den = 1000;\n> +\tdenoise.strength.num = 1000 * denoiseStatus->strength;\n> +\tdenoise.strength.den = 1000;\n> +\n> +\t/* Set the CDN mode to match the SDN operating mode. */\n> +\tbcm2835_isp_cdn cdn;\n> +\tswitch (mode) {\n> +\tcase DenoiseMode::ColourFast:\n> +\t\tcdn.enabled = 1;\n> +\t\tcdn.mode = CDN_MODE_FAST;\n> +\t\tbreak;\n> +\tcase DenoiseMode::ColourHighQuality:\n> +\t\tcdn.enabled = 1;\n> +\t\tcdn.mode = CDN_MODE_HIGH_QUALITY;\n> +\t\tbreak;\n> +\tdefault:\n> +\t\tcdn.enabled = 0;\n> +\t}\n> +\n> +\tControlValue c(Span{ reinterpret_cast(&denoise),\n> +\t\t\t\t\t sizeof(denoise) });\n> +\tctrls.set(V4L2_CID_USER_BCM2835_ISP_DENOISE, c);\n> +\n> +\tc = ControlValue(Span{ reinterpret_cast(&cdn),\n> +\t\t\t\t\t sizeof(cdn) });\n> +\tctrls.set(V4L2_CID_USER_BCM2835_ISP_CDN, c);\n> +}\n> +\n> +void IpaVc4::applySharpen(const struct SharpenStatus *sharpenStatus, ControlList &ctrls)\n> +{\n> +\tbcm2835_isp_sharpen sharpen;\n> +\n> +\tsharpen.enabled = 1;\n> +\tsharpen.threshold.num = 1000 * sharpenStatus->threshold;\n> +\tsharpen.threshold.den = 1000;\n> +\tsharpen.strength.num = 1000 * sharpenStatus->strength;\n> +\tsharpen.strength.den = 1000;\n> +\tsharpen.limit.num = 1000 * sharpenStatus->limit;\n> +\tsharpen.limit.den = 1000;\n> +\n> +\tControlValue c(Span{ reinterpret_cast(&sharpen),\n> +\t\t\t\t\t sizeof(sharpen) });\n> +\tctrls.set(V4L2_CID_USER_BCM2835_ISP_SHARPEN, c);\n> +}\n> +\n> +void IpaVc4::applyDPC(const struct DpcStatus *dpcStatus, ControlList &ctrls)\n> +{\n> +\tbcm2835_isp_dpc dpc;\n> +\n> +\tdpc.enabled = 1;\n> +\tdpc.strength = dpcStatus->strength;\n> +\n> +\tControlValue c(Span{ reinterpret_cast(&dpc),\n> +\t\t\t\t\t sizeof(dpc) });\n> +\tctrls.set(V4L2_CID_USER_BCM2835_ISP_DPC, c);\n> +}\n> +\n> +void IpaVc4::applyLS(const struct AlscStatus *lsStatus, ControlList &ctrls)\n> +{\n> +\t/*\n> +\t * Program lens shading tables into pipeline.\n> +\t * Choose smallest cell size that won't exceed 63x48 cells.\n> +\t */\n> +\tconst int cellSizes[] = { 16, 32, 64, 128, 256 };\n> +\tunsigned int numCells = std::size(cellSizes);\n> +\tunsigned int i, w, h, cellSize;\n> +\tfor (i = 0; i < numCells; i++) {\n> +\t\tcellSize = cellSizes[i];\n> +\t\tw = (mode_.width + cellSize - 1) / cellSize;\n> +\t\th = (mode_.height + cellSize - 1) / cellSize;\n> +\t\tif (w < 64 && h <= 48)\n> +\t\t\tbreak;\n> +\t}\n> +\n> +\tif (i == numCells) {\n> +\t\tLOG(IPARPI, Error) << \"Cannot find cell size\";\n> +\t\treturn;\n> +\t}\n> +\n> +\t/* We're going to supply corner sampled tables, 16 bit samples. */\n> +\tw++, h++;\n> +\tbcm2835_isp_lens_shading ls = {\n> +\t\t.enabled = 1,\n> +\t\t.grid_cell_size = cellSize,\n> +\t\t.grid_width = w,\n> +\t\t.grid_stride = w,\n> +\t\t.grid_height = h,\n> +\t\t/* .dmabuf will be filled in by pipeline handler. */\n> +\t\t.dmabuf = 0,\n> +\t\t.ref_transform = 0,\n> +\t\t.corner_sampled = 1,\n> +\t\t.gain_format = GAIN_FORMAT_U4P10\n> +\t};\n> +\n> +\tif (!lsTable_ || w * h * 4 * sizeof(uint16_t) > MaxLsGridSize) {\n> +\t\tLOG(IPARPI, Error) << \"Do not have a correctly allocate lens shading table!\";\n> +\t\treturn;\n> +\t}\n> +\n> +\tif (lsStatus) {\n> +\t\t/* Format will be u4.10 */\n> +\t\tuint16_t *grid = static_cast(lsTable_);\n> +\n> +\t\tresampleTable(grid, lsStatus->r, w, h);\n> +\t\tresampleTable(grid + w * h, lsStatus->g, w, h);\n> +\t\tmemcpy(grid + 2 * w * h, grid + w * h, w * h * sizeof(uint16_t));\n> +\t\tresampleTable(grid + 3 * w * h, lsStatus->b, w, h);\n> +\t}\n> +\n> +\tControlValue c(Span{ reinterpret_cast(&ls),\n> +\t\t\t\t\t sizeof(ls) });\n> +\tctrls.set(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING, c);\n> +}\n> +\n> +void IpaVc4::applyAF(const struct AfStatus *afStatus, ControlList &lensCtrls)\n> +{\n> +\tif (afStatus->lensSetting) {\n> +\t\tControlValue v(afStatus->lensSetting.value());\n> +\t\tlensCtrls.set(V4L2_CID_FOCUS_ABSOLUTE, v);\n> +\t}\n> +}\n> +\n> +/*\n> + * Resamples a 16x12 table with central sampling to destW x destH with corner\n> + * sampling.\n> + */\n> +void IpaVc4::resampleTable(uint16_t dest[], const std::vector &src,\n> +\t\t\t int destW, int destH)\n> +{\n> +\t/*\n> +\t * Precalculate and cache the x sampling locations and phases to\n> +\t * save recomputing them on every row.\n> +\t */\n> +\tassert(destW > 1 && destH > 1 && destW <= 64);\n> +\tint xLo[64], xHi[64];\n> +\tdouble xf[64];\n> +\tdouble x = -0.5, xInc = 16.0 / (destW - 1);\n> +\tfor (int i = 0; i < destW; i++, x += xInc) {\n> +\t\txLo[i] = floor(x);\n> +\t\txf[i] = x - xLo[i];\n> +\t\txHi[i] = xLo[i] < 15 ? xLo[i] + 1 : 15;\n> +\t\txLo[i] = xLo[i] > 0 ? xLo[i] : 0;\n> +\t}\n> +\n> +\t/* Now march over the output table generating the new values. */\n> +\tdouble y = -0.5, yInc = 12.0 / (destH - 1);\n> +\tfor (int j = 0; j < destH; j++, y += yInc) {\n> +\t\tint yLo = floor(y);\n> +\t\tdouble yf = y - yLo;\n> +\t\tint yHi = yLo < 11 ? yLo + 1 : 11;\n> +\t\tyLo = yLo > 0 ? yLo : 0;\n> +\t\tdouble const *rowAbove = src.data() + yLo * 16;\n> +\t\tdouble const *rowBelow = src.data() + yHi * 16;\n> +\t\tfor (int i = 0; i < destW; i++) {\n> +\t\t\tdouble above = rowAbove[xLo[i]] * (1 - xf[i]) + rowAbove[xHi[i]] * xf[i];\n> +\t\t\tdouble below = rowBelow[xLo[i]] * (1 - xf[i]) + rowBelow[xHi[i]] * xf[i];\n> +\t\t\tint result = floor(1024 * (above * (1 - yf) + below * yf) + .5);\n> +\t\t\t*(dest++) = result > 16383 ? 16383 : result; /* want u4.10 */\n> +\t\t}\n> +\t}\n> +}\n> +\n> +} /* namespace ipa::RPi */\n> +\n> +/*\n> + * External IPA module interface\n> + */\n> +extern \"C\" {\n> +const struct IPAModuleInfo ipaModuleInfo = {\n> +\tIPA_MODULE_API_VERSION,\n> +\t1,\n> +\t\"PipelineHandlerRPi\",\n> +\t\"vc4\",\n> +};\n> +\n> +IPAInterface *ipaCreate()\n> +{\n> +\treturn new ipa::RPi::IpaVc4();\n> +}\n> +\n> +} /* extern \"C\" */\n> +\n> +} /* namespace libcamera */\n> --\n> 2.34.1\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 03528BDCBD\n\tfor ;\n\tThu, 27 Apr 2023 10:00:43 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 99BB0627DE;\n\tThu, 27 Apr 2023 12:00:42 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E9B42627B7\n\tfor ;\n\tThu, 27 Apr 2023 12:00:40 +0200 (CEST)","from ideasonboard.com (unknown\n\t[IPv6:2001:b07:5d2e:52c9:1cf0:b3bc:c785:4625])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id C33889DE;\n\tThu, 27 Apr 2023 12:00:28 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1682589642;\n\tbh=wuyyWu41371fvFetoDU6X+1dq3pwKJ660B7/R5fmUrE=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=W9X9+Xx8vnPKBosGYFAZ+ZK1ZLLSklE25R5Tiz61mnE1iKBU8bWn4LC76ZPykBSSz\n\th9Ppln5bIuulCssnCwjZIQ/LcVIZd8TmFosWIr9vAaPFYWPK/A3aQxgopYV6rBYe1n\n\tmUW2OCMaIxRKi9Xq/x3Ev4CLQDC1RMgHfjxfdRtyMozk1KXXz4jb2vX/qtu6H3xqaQ\n\twacA0CnvMV9hjLw3nXd+t7eAoy0/IazFug5ZudgAy9kXfmYrWqP5Cvn5508beIN4dL\n\tleOgCArl9N8QF03rVTgWqNETTbv7wUekcBGjeN3c4WT+fDDe0XuBnke7LhA8AXdfeQ\n\tSHMk/CP5VsFPw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1682589629;\n\tbh=wuyyWu41371fvFetoDU6X+1dq3pwKJ660B7/R5fmUrE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=P+2RYhGigCkKfWLrL1y7pYnA5bkMC9TpxoGTNQkZhBrDf1f5GjX/X9CXCma9Js0Uo\n\tOHwwyJeLYVba/2PuYnqtg/YS8GxaTjn727RX3dZ36JlQeH98oW4OcEeb9/HWX9BRHr\n\tEIqLmq8Z2wz/+Q+kNNs2bl4S9EPo55x1dHGiWQN4="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"P+2RYhGi\"; dkim-atps=neutral","Date":"Thu, 27 Apr 2023 12:00:37 +0200","To":"Naushir Patuck ","Message-ID":"<6a34pfgd7zfbupsonwttin6qo5kvlgjo2guqrqksvj6c5ivs35@sm2iazri3glv>","References":"<20230426131057.21550-1-naush@raspberrypi.com>\n\t<20230426131057.21550-9-naush@raspberrypi.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20230426131057.21550-9-naush@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH 08/13] ipa: raspberrypi: Introduce\n\tIpaBase class","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Jacopo Mondi via libcamera-devel ","Reply-To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":26976,"web_url":"https://patchwork.libcamera.org/comment/26976/","msgid":"<20230427155415.GI28489@pendragon.ideasonboard.com>","date":"2023-04-27T15:54:15","subject":"Re: [libcamera-devel] [PATCH 08/13] ipa: raspberrypi: Introduce\n\tIpaBase class","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Naush,\n\nThank you for the patch.\n\nOn Wed, Apr 26, 2023 at 02:10:52PM +0100, Naushir Patuck via libcamera-devel wrote:\n> Create a new IpaBase class that handles general purpose housekeeping\n> duties for the Raspberry Pi IPA. Code the implementation of new class is\n\nDid you mean s/Code the implementation/The implementation/ and\ns/of new class/of the new class/ ?\n\n> essentially pulled from the existing ipa/rpi/vc4/raspberrypi.cpp\n> file with a minimal amount of refactoring.\n> \n> Create a derived IpaVc4 class from IpaBase that handles the VC4 pipeline\n> specific tasks of the IPA. Again, code for this class implementation is\n> taken from the existing ipa/rpi/vc4/raspberrypi.cpp with a\n> minimal amount of refactoring.\n> \n> The goal of this change is to allow third parties to implement their own\n> IPA running on the Raspberry Pi without duplicating all of the IPA\n> housekeeping tasks.\n> \n> Signed-off-by: Naushir Patuck \n> ---\n> src/ipa/meson.build | 5 +-\n> .../raspberrypi.cpp => common/ipa_base.cpp} | 1219 +++++------------\n> src/ipa/rpi/common/ipa_base.h | 125 ++\n> src/ipa/rpi/common/meson.build | 7 +\n> src/ipa/rpi/vc4/meson.build | 8 +-\n> src/ipa/rpi/vc4/vc4.cpp | 540 ++++++++\n> 6 files changed, 1012 insertions(+), 892 deletions(-)\n> rename src/ipa/rpi/{vc4/raspberrypi.cpp => common/ipa_base.cpp} (65%)\n> create mode 100644 src/ipa/rpi/common/ipa_base.h\n> create mode 100644 src/ipa/rpi/common/meson.build\n> create mode 100644 src/ipa/rpi/vc4/vc4.cpp\n> \n> diff --git a/src/ipa/meson.build b/src/ipa/meson.build\n> index 10d3b44ca7b6..0c622c38a4a0 100644\n> --- a/src/ipa/meson.build\n> +++ b/src/ipa/meson.build\n> @@ -37,13 +37,14 @@ endif\n> \n> enabled_ipa_modules = []\n> \n> -# If the Raspberry Pi VC4 IPA is enabled, ensure we include the rpi/cam_helper\n> -# and rpi/controller subdirectories in the build.\n> +# If the Raspberry Pi VC4 IPA is enabled, ensure we include the cam_helper,\n> +# common and controller subdirectories in the build.\n> #\n> # This is done here and not within rpi/vc4/meson.build as meson does not\n> # allow the subdir command to traverse up the directory tree.\n> if pipelines.contains('rpi/vc4')\n> subdir('rpi/cam_helper')\n> + subdir('rpi/common')\n> subdir('rpi/controller')\n> endif\n> \n> diff --git a/src/ipa/rpi/vc4/raspberrypi.cpp b/src/ipa/rpi/common/ipa_base.cpp\n> similarity index 65%\n> rename from src/ipa/rpi/vc4/raspberrypi.cpp\n> rename to src/ipa/rpi/common/ipa_base.cpp\n> index e3ca8e2f7cbe..becada5973ad 100644\n> --- a/src/ipa/rpi/vc4/raspberrypi.cpp\n> +++ b/src/ipa/rpi/common/ipa_base.cpp\n> @@ -1,60 +1,30 @@\n\n[snip]\n\n> @@ -62,8 +32,7 @@ namespace libcamera {\n> using namespace std::literals::chrono_literals;\n> using utils::Duration;\n> \n> -/* Number of metadata objects available in the context list. */\n> -constexpr unsigned int numMetadataContexts = 16;\n> +namespace {\n> \n> /* Number of frame length times to hold in the queue. */\n> constexpr unsigned int FrameLengthsQueueSize = 10;\n> @@ -80,7 +49,7 @@ constexpr Duration defaultMaxFrameDuration = 250.0s;\n> * we rate-limit the controller Prepare() and Process() calls to lower than or\n> * equal to this rate.\n> */\n> -constexpr Duration controllerMinFrameDuration = 1.0s / 30.0;\n> +static constexpr Duration controllerMinFrameDuration = 1.0s / 30.0;\n\nEverything contained in an anonymous namespace becomes static. You can\ndrop the static keyword from other variables.\n\n> \n> /* List of controls handled by the Raspberry Pi IPA */\n> static const ControlInfoMap::Map ipaControls{\n> @@ -116,118 +85,23 @@ static const ControlInfoMap::Map ipaAfControls{\n> \t{ &controls::LensPosition, ControlInfo(0.0f, 32.0f, 1.0f) }\n> };\n> \n> +} /* namespace */\n> +\n\n[snip]\n\n> diff --git a/src/ipa/rpi/common/ipa_base.h b/src/ipa/rpi/common/ipa_base.h\n> new file mode 100644\n> index 000000000000..b9fbf9414d63\n> --- /dev/null\n> +++ b/src/ipa/rpi/common/ipa_base.h\n> @@ -0,0 +1,125 @@\n> +/* SPDX-License-Identifier: BSD-2-Clause */\n> +/*\n> + * Copyright (C) 2023, Raspberry Pi Ltd\n> + *\n> + * ipa_base.cpp - Raspberry Pi IPA base class\n\nipa_base.h\n\n> + */\n> +#pragma once\n> +\n> +#include \n> +#include \n> +#include \n\nYou need here, but can drop as it's only used to\nimplement the interface of the base class, and so is provided by\nlibcamera/ipa/ipa_interface.h. You also need stdint.h.\n\n> +\n\n#include \n\n> +#include \n> +\n> +#include \n> +\n> +#include \n> +#include \n\nNot needed.\n\n> +#include \n> +\n> +#include \"libcamera/internal/mapped_framebuffer.h\"\n> +\n> +#include \"cam_helper/cam_helper.h\"\n> +#include \"controller/agc_status.h\"\n> +#include \"controller/camera_mode.h\"\n> +#include \"controller/controller.h\"\n> +#include \"controller/metadata.h\"\n> +\n> +namespace libcamera {\n> +\n> +namespace ipa::RPi {\n> +\n> +class IpaBase : public IPARPiInterface\n> +{\n> +public:\n> +\tIpaBase();\n> +\tvirtual ~IpaBase() = 0;\n\nThat's a bit weird, why do you need a virtual abstract destructor ?\n\n> +\n> +\tint32_t init(const IPASettings &settings, const InitParams ¶ms, InitResult *result) override;\n> +\tint32_t configure(const IPACameraSensorInfo &sensorInfo, const ConfigParams ¶ms,\n> +\t\t\t ConfigResult *result) override;\n> +\n> +\tvoid start(const ControlList &controls, StartResult *result) override;\n> +\tvoid stop() override {}\n> +\n> +\tvoid mapBuffers(const std::vector &buffers) override;\n> +\tvoid unmapBuffers(const std::vector &ids) override;\n> +\n> +\tvoid prepareIsp(const PrepareParams ¶ms) override;\n> +\tvoid processStats(const ProcessParams ¶ms) override;\n> +\n> +\tvoid reportMetadata(unsigned int ipaContext, ControlList *controls) override;\n> +\n> +private:\n> +\t/* Number of metadata objects available in the context list. */\n> +\tstatic constexpr unsigned int numMetadataContexts = 16;\n> +\n> +\tvirtual int32_t platformInit(const InitParams ¶ms, InitResult *result) = 0;\n\nThis enforces a particular sequence of calls between the base class and\nthe derived class, dictated by the base class design. It's best to\nhandle this in the derived class instead. You can override the init()\nfunction in the derived class, call the init() function of the base\nclass, and then perform all the work you're currently doing in\nIpaVc4::platformInit().\n\nThe same comment applies to the other platform*() functions (some may\nrequire a little bit of refactoring). The handleControls() function can\nalso be dropped by making applyControls() a protected virtual function\nthat derived classes may override.\n\n> +\tvirtual int32_t platformConfigure(const ConfigParams ¶ms, ConfigResult *result) = 0;\n> +\n> +\tvirtual void platformPrepareIsp(const PrepareParams ¶ms,\n> +\t\t\t\t\tRPiController::Metadata &rpiMetadata) = 0;\n> +\tvirtual RPiController::StatisticsPtr platformProcessStats(Span mem) = 0;\n> +\n> +\tvoid setMode(const IPACameraSensorInfo &sensorInfo);\n> +\tvoid setCameraTimeoutValue();\n> +\tbool validateSensorControls();\n> +\tbool validateLensControls();\n> +\tvoid applyControls(const ControlList &controls);\n> +\tvirtual void handleControls(const ControlList &controls) = 0;\n> +\tvoid fillDeviceStatus(const ControlList &sensorControls, unsigned int ipaContext);\n> +\tvoid applyFrameDurations(utils::Duration minFrameDuration, utils::Duration maxFrameDuration);\n> +\tvoid applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls);\n> +\n> +\tstd::map buffers_;\n> +\n> +\tbool lensPresent_;\n> +\tControlList libcameraMetadata_;\n> +\n> +\tstd::array rpiMetadata_;\n> +\n> +\t/*\n> +\t * We count frames to decide if the frame must be hidden (e.g. from\n> +\t * display) or mistrusted (i.e. not given to the control algos).\n> +\t */\n> +\tuint64_t frameCount_;\n> +\n> +\t/* How many frames we should avoid running control algos on. */\n> +\tunsigned int mistrustCount_;\n> +\n> +\t/* Number of frames that need to be dropped on startup. */\n> +\tunsigned int dropFrameCount_;\n> +\n> +\t/* Frame timestamp for the last run of the controller. */\n> +\tuint64_t lastRunTimestamp_;\n> +\n> +\t/* Do we run a Controller::process() for this frame? */\n> +\tbool processPending_;\n> +\n> +\t/* Distinguish the first camera start from others. */\n> +\tbool firstStart_;\n> +\n> +\t/* Frame duration (1/fps) limits. */\n> +\tutils::Duration minFrameDuration_;\n> +\tutils::Duration maxFrameDuration_;\n> +\n> +protected:\n\nWe usually put protected members before private members.\n\n> +\t/* Raspberry Pi controller specific defines. */\n> +\tstd::unique_ptr helper_;\n> +\tRPiController::Controller controller_;\n> +\n> +\tControlInfoMap sensorCtrls_;\n> +\tControlInfoMap lensCtrls_;\n> +\n> +\t/* Camera sensor params. */\n> +\tCameraMode mode_;\n> +\n> +\t/* Track the frame length times over FrameLengthsQueueSize frames. */\n> +\tstd::deque frameLengths_;\n> +\tutils::Duration lastTimeout_;\n> +};\n> +\n> +} /* namespace ipa::RPi */\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/ipa/rpi/common/meson.build b/src/ipa/rpi/common/meson.build\n> new file mode 100644\n> index 000000000000..a7189f8389af\n> --- /dev/null\n> +++ b/src/ipa/rpi/common/meson.build\n> @@ -0,0 +1,7 @@\n> +# SPDX-License-Identifier: CC0-1.0\n> +\n> +# SPDX-License-Identifier: CC0-1.0\n\nOne SPDX header is enough :-)\n\n> +\n> +rpi_ipa_common_sources = files([\n> + 'ipa_base.cpp',\n> +])\n> diff --git a/src/ipa/rpi/vc4/meson.build b/src/ipa/rpi/vc4/meson.build\n> index 992d0f1ab5a7..b581391586b3 100644\n> --- a/src/ipa/rpi/vc4/meson.build\n> +++ b/src/ipa/rpi/vc4/meson.build\n> @@ -13,11 +13,15 @@ vc4_ipa_includes = [\n> ]\n> \n> vc4_ipa_sources = files([\n> - 'raspberrypi.cpp',\n> + 'vc4.cpp',\n> ])\n> \n> vc4_ipa_includes += include_directories('..')\n> -vc4_ipa_sources += [rpi_ipa_cam_helper_sources, rpi_ipa_controller_sources]\n> +vc4_ipa_sources += [\n> + rpi_ipa_cam_helper_sources,\n> + rpi_ipa_common_sources,\n> + rpi_ipa_controller_sources,\n\nCould we avoid compiling all these source files multiple times, once for\nthe IpaVc4 IPA module, and once for the other IPA modules that may be\nimplemented in the future ? This can be done with a static library, see\nsrc/apps/common/meson.build and src/apps/cam/meson.build for instance.\n\n> +]\n> \n> mod = shared_module(ipa_name,\n> [vc4_ipa_sources, libcamera_generated_ipa_headers],\n\n[snip]","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 933B3C0DA4\n\tfor ;\n\tThu, 27 Apr 2023 15:54:07 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D03A1627B7;\n\tThu, 27 Apr 2023 17:54:06 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6BE27627B7\n\tfor ;\n\tThu, 27 Apr 2023 17:54:05 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(133-32-181-51.west.xps.vectant.ne.jp [133.32.181.51])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id BC2BC802;\n\tThu, 27 Apr 2023 17:53:52 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1682610846;\n\tbh=JlXHNqlzzEzJ+KVgmnhvVjdYrKAAKwKV8sd/kx8kaa0=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=H6nJaoQje2Q2yL9/p0it3YtE9raSP1VsuqhI5bOuLyy072hkREIvetqx9ikujcFZ2\n\tDEnOpKH3W+ZUD08wDp8pYPg2c9EFOY0qklhZqvA+nylgtWeW0l8+IOgeNXAzlQV2fY\n\t8DyFsN8UvSptnV1vR5YsFQwix5C7q9c/P4wu4FpyWUT32GGq02RxoQ8pJ0oab+s8tQ\n\t98M9BdrL/XNvP5+5vdKq0hfal53SCcNh9DLX3v7yTN/M4HksbQMgcvNOiePUsMFgTc\n\t2NhS9RbX7pt3K/c/FnxW8AmywyOpcszEQ5EQBWlMWrHSmsrRArICPfKNSDzDpElckW\n\twJ7DCAtwiYWhA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1682610833;\n\tbh=JlXHNqlzzEzJ+KVgmnhvVjdYrKAAKwKV8sd/kx8kaa0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=eic9kV9Hw08vkXOifHHnwUe+ay4jlSooIKyyy+W5nUPunQBHLibkVXf3+wgujdLom\n\tIb/UPfMAMSoyPhOmJx3NJipzbFr7SYOCypYT8wfDWTfns2WiYChPwyOQS+WzaSeFwB\n\tYo3wKfhEyGfr2ko9SsMnMZMIjUIq0G3hAaduXVWY="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"eic9kV9H\"; dkim-atps=neutral","Date":"Thu, 27 Apr 2023 18:54:15 +0300","To":"Naushir Patuck ","Message-ID":"<20230427155415.GI28489@pendragon.ideasonboard.com>","References":"<20230426131057.21550-1-naush@raspberrypi.com>\n\t<20230426131057.21550-9-naush@raspberrypi.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20230426131057.21550-9-naush@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH 08/13] ipa: raspberrypi: Introduce\n\tIpaBase class","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":26985,"web_url":"https://patchwork.libcamera.org/comment/26985/","msgid":"","date":"2023-04-28T08:35:51","subject":"Re: [libcamera-devel] [PATCH 08/13] ipa: raspberrypi: Introduce\n\tIpaBase class","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Laurent,\n\nThank you for your feedback.\n\nOn Thu, 27 Apr 2023 at 16:54, Laurent Pinchart\n wrote:\n>\n> Hi Naush,\n>\n> Thank you for the patch.\n>\n> On Wed, Apr 26, 2023 at 02:10:52PM +0100, Naushir Patuck via libcamera-devel wrote:\n> > Create a new IpaBase class that handles general purpose housekeeping\n> > duties for the Raspberry Pi IPA. Code the implementation of new class is\n>\n> Did you mean s/Code the implementation/The implementation/ and\n> s/of new class/of the new class/ ?\n\nI actually meant \"Code for the implementation...\", but I'll replace it\nwith your wording.\n\n>\n> > essentially pulled from the existing ipa/rpi/vc4/raspberrypi.cpp\n> > file with a minimal amount of refactoring.\n> >\n> > Create a derived IpaVc4 class from IpaBase that handles the VC4 pipeline\n> > specific tasks of the IPA. Again, code for this class implementation is\n> > taken from the existing ipa/rpi/vc4/raspberrypi.cpp with a\n> > minimal amount of refactoring.\n> >\n> > The goal of this change is to allow third parties to implement their own\n> > IPA running on the Raspberry Pi without duplicating all of the IPA\n> > housekeeping tasks.\n> >\n> > Signed-off-by: Naushir Patuck \n> > ---\n> > src/ipa/meson.build | 5 +-\n> > .../raspberrypi.cpp => common/ipa_base.cpp} | 1219 +++++------------\n> > src/ipa/rpi/common/ipa_base.h | 125 ++\n> > src/ipa/rpi/common/meson.build | 7 +\n> > src/ipa/rpi/vc4/meson.build | 8 +-\n> > src/ipa/rpi/vc4/vc4.cpp | 540 ++++++++\n> > 6 files changed, 1012 insertions(+), 892 deletions(-)\n> > rename src/ipa/rpi/{vc4/raspberrypi.cpp => common/ipa_base.cpp} (65%)\n> > create mode 100644 src/ipa/rpi/common/ipa_base.h\n> > create mode 100644 src/ipa/rpi/common/meson.build\n> > create mode 100644 src/ipa/rpi/vc4/vc4.cpp\n> >\n> > diff --git a/src/ipa/meson.build b/src/ipa/meson.build\n> > index 10d3b44ca7b6..0c622c38a4a0 100644\n> > --- a/src/ipa/meson.build\n> > +++ b/src/ipa/meson.build\n> > @@ -37,13 +37,14 @@ endif\n> >\n> > enabled_ipa_modules = []\n> >\n> > -# If the Raspberry Pi VC4 IPA is enabled, ensure we include the rpi/cam_helper\n> > -# and rpi/controller subdirectories in the build.\n> > +# If the Raspberry Pi VC4 IPA is enabled, ensure we include the cam_helper,\n> > +# common and controller subdirectories in the build.\n> > #\n> > # This is done here and not within rpi/vc4/meson.build as meson does not\n> > # allow the subdir command to traverse up the directory tree.\n> > if pipelines.contains('rpi/vc4')\n> > subdir('rpi/cam_helper')\n> > + subdir('rpi/common')\n> > subdir('rpi/controller')\n> > endif\n> >\n> > diff --git a/src/ipa/rpi/vc4/raspberrypi.cpp b/src/ipa/rpi/common/ipa_base.cpp\n> > similarity index 65%\n> > rename from src/ipa/rpi/vc4/raspberrypi.cpp\n> > rename to src/ipa/rpi/common/ipa_base.cpp\n> > index e3ca8e2f7cbe..becada5973ad 100644\n> > --- a/src/ipa/rpi/vc4/raspberrypi.cpp\n> > +++ b/src/ipa/rpi/common/ipa_base.cpp\n> > @@ -1,60 +1,30 @@\n>\n> [snip]\n>\n> > @@ -62,8 +32,7 @@ namespace libcamera {\n> > using namespace std::literals::chrono_literals;\n> > using utils::Duration;\n> >\n> > -/* Number of metadata objects available in the context list. */\n> > -constexpr unsigned int numMetadataContexts = 16;\n> > +namespace {\n> >\n> > /* Number of frame length times to hold in the queue. */\n> > constexpr unsigned int FrameLengthsQueueSize = 10;\n> > @@ -80,7 +49,7 @@ constexpr Duration defaultMaxFrameDuration = 250.0s;\n> > * we rate-limit the controller Prepare() and Process() calls to lower than or\n> > * equal to this rate.\n> > */\n> > -constexpr Duration controllerMinFrameDuration = 1.0s / 30.0;\n> > +static constexpr Duration controllerMinFrameDuration = 1.0s / 30.0;\n>\n> Everything contained in an anonymous namespace becomes static. You can\n> drop the static keyword from other variables.\n>\n> >\n> > /* List of controls handled by the Raspberry Pi IPA */\n> > static const ControlInfoMap::Map ipaControls{\n> > @@ -116,118 +85,23 @@ static const ControlInfoMap::Map ipaAfControls{\n> > { &controls::LensPosition, ControlInfo(0.0f, 32.0f, 1.0f) }\n> > };\n> >\n> > +} /* namespace */\n> > +\n>\n> [snip]\n>\n> > diff --git a/src/ipa/rpi/common/ipa_base.h b/src/ipa/rpi/common/ipa_base.h\n> > new file mode 100644\n> > index 000000000000..b9fbf9414d63\n> > --- /dev/null\n> > +++ b/src/ipa/rpi/common/ipa_base.h\n> > @@ -0,0 +1,125 @@\n> > +/* SPDX-License-Identifier: BSD-2-Clause */\n> > +/*\n> > + * Copyright (C) 2023, Raspberry Pi Ltd\n> > + *\n> > + * ipa_base.cpp - Raspberry Pi IPA base class\n>\n> ipa_base.h\n>\n> > + */\n> > +#pragma once\n> > +\n> > +#include \n> > +#include \n> > +#include \n>\n> You need here, but can drop as it's only used to\n> implement the interface of the base class, and so is provided by\n> libcamera/ipa/ipa_interface.h. You also need stdint.h.\n>\n> > +\n>\n> #include \n>\n> > +#include \n> > +\n> > +#include \n> > +\n> > +#include \n> > +#include \n>\n> Not needed.\n>\n> > +#include \n> > +\n> > +#include \"libcamera/internal/mapped_framebuffer.h\"\n> > +\n> > +#include \"cam_helper/cam_helper.h\"\n> > +#include \"controller/agc_status.h\"\n> > +#include \"controller/camera_mode.h\"\n> > +#include \"controller/controller.h\"\n> > +#include \"controller/metadata.h\"\n> > +\n> > +namespace libcamera {\n> > +\n> > +namespace ipa::RPi {\n> > +\n> > +class IpaBase : public IPARPiInterface\n> > +{\n> > +public:\n> > + IpaBase();\n> > + virtual ~IpaBase() = 0;\n>\n> That's a bit weird, why do you need a virtual abstract destructor ?\n\nThis was a mistake, I'll remove it.\n\n>\n> > +\n> > + int32_t init(const IPASettings &settings, const InitParams ¶ms, InitResult *result) override;\n> > + int32_t configure(const IPACameraSensorInfo &sensorInfo, const ConfigParams ¶ms,\n> > + ConfigResult *result) override;\n> > +\n> > + void start(const ControlList &controls, StartResult *result) override;\n> > + void stop() override {}\n> > +\n> > + void mapBuffers(const std::vector &buffers) override;\n> > + void unmapBuffers(const std::vector &ids) override;\n> > +\n> > + void prepareIsp(const PrepareParams ¶ms) override;\n> > + void processStats(const ProcessParams ¶ms) override;\n> > +\n> > + void reportMetadata(unsigned int ipaContext, ControlList *controls) override;\n> > +\n> > +private:\n> > + /* Number of metadata objects available in the context list. */\n> > + static constexpr unsigned int numMetadataContexts = 16;\n> > +\n> > + virtual int32_t platformInit(const InitParams ¶ms, InitResult *result) = 0;\n>\n> This enforces a particular sequence of calls between the base class and\n> the derived class, dictated by the base class design. It's best to\n> handle this in the derived class instead. You can override the init()\n> function in the derived class, call the init() function of the base\n> class, and then perform all the work you're currently doing in\n> IpaVc4::platformInit().\n\nOne of the earlier iterations had exactly this implementing. After\ninternal discussions, we decided to do it the other way round (i.e. the base\nclass implements the init() override). Out of curiosity, what advantages would\none way have over the other?\n\n>\n> The same comment applies to the other platform*() functions (some may\n> require a little bit of refactoring). The handleControls() function can\n> also be dropped by making applyControls() a protected virtual function\n> that derived classes may override.\n>\n> > + virtual int32_t platformConfigure(const ConfigParams ¶ms, ConfigResult *result) = 0;\n> > +\n> > + virtual void platformPrepareIsp(const PrepareParams ¶ms,\n> > + RPiController::Metadata &rpiMetadata) = 0;\n> > + virtual RPiController::StatisticsPtr platformProcessStats(Span mem) = 0;\n> > +\n> > + void setMode(const IPACameraSensorInfo &sensorInfo);\n> > + void setCameraTimeoutValue();\n> > + bool validateSensorControls();\n> > + bool validateLensControls();\n> > + void applyControls(const ControlList &controls);\n> > + virtual void handleControls(const ControlList &controls) = 0;\n> > + void fillDeviceStatus(const ControlList &sensorControls, unsigned int ipaContext);\n> > + void applyFrameDurations(utils::Duration minFrameDuration, utils::Duration maxFrameDuration);\n> > + void applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls);\n> > +\n> > + std::map buffers_;\n> > +\n> > + bool lensPresent_;\n> > + ControlList libcameraMetadata_;\n> > +\n> > + std::array rpiMetadata_;\n> > +\n> > + /*\n> > + * We count frames to decide if the frame must be hidden (e.g. from\n> > + * display) or mistrusted (i.e. not given to the control algos).\n> > + */\n> > + uint64_t frameCount_;\n> > +\n> > + /* How many frames we should avoid running control algos on. */\n> > + unsigned int mistrustCount_;\n> > +\n> > + /* Number of frames that need to be dropped on startup. */\n> > + unsigned int dropFrameCount_;\n> > +\n> > + /* Frame timestamp for the last run of the controller. */\n> > + uint64_t lastRunTimestamp_;\n> > +\n> > + /* Do we run a Controller::process() for this frame? */\n> > + bool processPending_;\n> > +\n> > + /* Distinguish the first camera start from others. */\n> > + bool firstStart_;\n> > +\n> > + /* Frame duration (1/fps) limits. */\n> > + utils::Duration minFrameDuration_;\n> > + utils::Duration maxFrameDuration_;\n> > +\n> > +protected:\n>\n> We usually put protected members before private members.\n>\n> > + /* Raspberry Pi controller specific defines. */\n> > + std::unique_ptr helper_;\n> > + RPiController::Controller controller_;\n> > +\n> > + ControlInfoMap sensorCtrls_;\n> > + ControlInfoMap lensCtrls_;\n> > +\n> > + /* Camera sensor params. */\n> > + CameraMode mode_;\n> > +\n> > + /* Track the frame length times over FrameLengthsQueueSize frames. */\n> > + std::deque frameLengths_;\n> > + utils::Duration lastTimeout_;\n> > +};\n> > +\n> > +} /* namespace ipa::RPi */\n> > +\n> > +} /* namespace libcamera */\n> > diff --git a/src/ipa/rpi/common/meson.build b/src/ipa/rpi/common/meson.build\n> > new file mode 100644\n> > index 000000000000..a7189f8389af\n> > --- /dev/null\n> > +++ b/src/ipa/rpi/common/meson.build\n> > @@ -0,0 +1,7 @@\n> > +# SPDX-License-Identifier: CC0-1.0\n> > +\n> > +# SPDX-License-Identifier: CC0-1.0\n>\n> One SPDX header is enough :-)\n>\n> > +\n> > +rpi_ipa_common_sources = files([\n> > + 'ipa_base.cpp',\n> > +])\n> > diff --git a/src/ipa/rpi/vc4/meson.build b/src/ipa/rpi/vc4/meson.build\n> > index 992d0f1ab5a7..b581391586b3 100644\n> > --- a/src/ipa/rpi/vc4/meson.build\n> > +++ b/src/ipa/rpi/vc4/meson.build\n> > @@ -13,11 +13,15 @@ vc4_ipa_includes = [\n> > ]\n> >\n> > vc4_ipa_sources = files([\n> > - 'raspberrypi.cpp',\n> > + 'vc4.cpp',\n> > ])\n> >\n> > vc4_ipa_includes += include_directories('..')\n> > -vc4_ipa_sources += [rpi_ipa_cam_helper_sources, rpi_ipa_controller_sources]\n> > +vc4_ipa_sources += [\n> > + rpi_ipa_cam_helper_sources,\n> > + rpi_ipa_common_sources,\n> > + rpi_ipa_controller_sources,\n>\n> Could we avoid compiling all these source files multiple times, once for\n> the IpaVc4 IPA module, and once for the other IPA modules that may be\n> implemented in the future ? This can be done with a static library, see\n> src/apps/common/meson.build and src/apps/cam/meson.build for instance.\n>\n> > +]\n> >\n> > mod = shared_module(ipa_name,\n> > [vc4_ipa_sources, libcamera_generated_ipa_headers],\n>\n\nSure, I'll fix this up.\n\nNaush\n\n> [snip]\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 62EB3C0DA4\n\tfor ;\n\tFri, 28 Apr 2023 08:36:02 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A7FD0627DF;\n\tFri, 28 Apr 2023 10:36:01 +0200 (CEST)","from mail-yw1-x112c.google.com (mail-yw1-x112c.google.com\n\t[IPv6:2607:f8b0:4864:20::112c])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 186EC627CD\n\tfor ;\n\tFri, 28 Apr 2023 10:35:59 +0200 (CEST)","by mail-yw1-x112c.google.com with SMTP id\n\t00721157ae682-54fc6949475so110437967b3.3\n\tfor ;\n\tFri, 28 Apr 2023 01:35:59 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1682670961;\n\tbh=hMD/Yr1v+/VtTysyg17K5lmMdq4Bv1bsQukTsXqLNxk=;\n\th=References:In-Reply-To:Date:To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=Ab0W1EpoLgQHOysbjrbPhZdwUDZZJJTuVW7rhdxtC6wdvTziS7kEUV2Sjy476saIm\n\tIW0JMytiFGvqzvidVZdQOIq8j5a/DjDIdOJMi0WAyqm/zvSaCS+/84kH+6eLH3GCMz\n\tOc5Nq9iLIoEskuGCnDnwYfdD2AW3034tGAXhCvEAPH6P/JbIET0wfJ5lwvEEcOUzbe\n\tTWh7oytZcDcjIabM5qQ+INy2FkOcIXuK53Z9EM49QCTdcG+iAMpBGjHVumdr5RHjGc\n\t29B8BepbZdK84AfJ8n0TmHLv+4GUZ9xu+dyayuDTwTyM+86cnMb+gzr1X//QOgumd2\n\t/ssd8ORHubfBg==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1682670959; x=1685262959;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc:subject:date:message-id:reply-to;\n\tbh=rTmc87JPMRHoTQ75VS4PefAKM3LD8m9GsONeyVIOzxc=;\n\tb=lnPQ7sLWEWRlgtX+296NCn//dtz+epo+Nmmw7GWDY+5z0O+GzmUq4nszGHB3ILLIyf\n\tY94AZ2pjRKaSoNC1+OO+9KqCK3xTkz2GwG4uAFdr+rce0/+q7mKF9EQYEBO2hJvUx8jz\n\tHPEN5f0hb9GPvPFVowVC/C4V795s7HyVatyvouRKbMIPSN6glPeXSG9qJtQvE2/U6Gat\n\tsTgYD3RaPNQY1d3h/FpsQWiIVvk14Kfpwm8FiwVDFQt9DzjbzzrKbSLu1t//8YTM5s7r\n\t9toookmfOC5LH+Ux5IXyCGNFSl4QZQCZmtG0zzD59lZekzKDJVt9tfHbogI8tnAOjnDh\n\tiw1g=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"lnPQ7sLW\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20221208; t=1682670959; x=1685262959;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-message-state:from:to:cc:subject:date:message-id\n\t:reply-to;\n\tbh=rTmc87JPMRHoTQ75VS4PefAKM3LD8m9GsONeyVIOzxc=;\n\tb=X//xmk6k+ApKn3tuzgFV2XdnTh5G43/4x4PmyzENdX8O+rR3RYoPGNmS9Vcp0e7HPG\n\tC8Xq5gr4ADecT1zQI34dGImypvYWkLEJjVi0L2lpldFYR6OCqQtm8gvlBmnrX3ErI/V7\n\ttGGvXtKccSWGtCgcgUNLyNmA4+IBrsrbDORx9E8pYFzGWt69rpKgHz1Bp/UmfxGa5NQr\n\tCSjq7ixbK3CpxpogKa5dE3MJqMK/jVVvrMjCT/k9JJ8oKkWDOpOfExNgqcye1gidQLMB\n\tzU5BaT8N5U8mF0+9RskI57r2pLAKWoK4nuCC+s7fgSc2ajL9vHyEBPNGfqP8cg/fUkuJ\n\tshAg==","X-Gm-Message-State":"AC+VfDzyAdPBAaA6cCey+gjce3bm5sqYK5Dv+klHqx3ST8jHXwF1BeLH\n\t8ATUErUeQ0HfpXwkkFH9vkUO8Y9Th9bQ3psAj8rtG1h5Ty187msGZs7R/A==","X-Google-Smtp-Source":"ACHHUZ6AXcddbLZWR+QDkMzl59npMdPLpUXN7eGaDs5EJHjbJUSXh5DtqyNmonLO5be3iu+512RGpfpTT3iY0fyGKU4=","X-Received":"by 2002:a81:6a85:0:b0:54c:272b:ee42 with SMTP id\n\tf127-20020a816a85000000b0054c272bee42mr3047224ywc.48.1682670958628;\n\tFri, 28 Apr 2023 01:35:58 -0700 (PDT)","MIME-Version":"1.0","References":"<20230426131057.21550-1-naush@raspberrypi.com>\n\t<20230426131057.21550-9-naush@raspberrypi.com>\n\t<20230427155415.GI28489@pendragon.ideasonboard.com>","In-Reply-To":"<20230427155415.GI28489@pendragon.ideasonboard.com>","Date":"Fri, 28 Apr 2023 09:35:51 +0100","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH 08/13] ipa: raspberrypi: Introduce\n\tIpaBase class","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Naushir Patuck via libcamera-devel\n\t","Reply-To":"Naushir Patuck ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":26988,"web_url":"https://patchwork.libcamera.org/comment/26988/","msgid":"","date":"2023-04-28T09:33:14","subject":"Re: [libcamera-devel] [PATCH 08/13] ipa: raspberrypi: Introduce\n\tIpaBase class","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Naush\n\nOn Fri, Apr 28, 2023 at 09:35:51AM +0100, Naushir Patuck via libcamera-devel wrote:\n> Hi Laurent,\n>\n> Thank you for your feedback.\n>\n> On Thu, 27 Apr 2023 at 16:54, Laurent Pinchart\n> wrote:\n> >\n> > Hi Naush,\n> >\n> > Thank you for the patch.\n> >\n> > On Wed, Apr 26, 2023 at 02:10:52PM +0100, Naushir Patuck via libcamera-devel wrote:\n> > > Create a new IpaBase class that handles general purpose housekeeping\n> > > duties for the Raspberry Pi IPA. Code the implementation of new class is\n> >\n> > Did you mean s/Code the implementation/The implementation/ and\n> > s/of new class/of the new class/ ?\n>\n> I actually meant \"Code for the implementation...\", but I'll replace it\n> with your wording.\n>\n> >\n> > > essentially pulled from the existing ipa/rpi/vc4/raspberrypi.cpp\n> > > file with a minimal amount of refactoring.\n> > >\n> > > Create a derived IpaVc4 class from IpaBase that handles the VC4 pipeline\n> > > specific tasks of the IPA. Again, code for this class implementation is\n> > > taken from the existing ipa/rpi/vc4/raspberrypi.cpp with a\n> > > minimal amount of refactoring.\n> > >\n> > > The goal of this change is to allow third parties to implement their own\n> > > IPA running on the Raspberry Pi without duplicating all of the IPA\n> > > housekeeping tasks.\n> > >\n> > > Signed-off-by: Naushir Patuck \n> > > ---\n> > > src/ipa/meson.build | 5 +-\n> > > .../raspberrypi.cpp => common/ipa_base.cpp} | 1219 +++++------------\n> > > src/ipa/rpi/common/ipa_base.h | 125 ++\n> > > src/ipa/rpi/common/meson.build | 7 +\n> > > src/ipa/rpi/vc4/meson.build | 8 +-\n> > > src/ipa/rpi/vc4/vc4.cpp | 540 ++++++++\n> > > 6 files changed, 1012 insertions(+), 892 deletions(-)\n> > > rename src/ipa/rpi/{vc4/raspberrypi.cpp => common/ipa_base.cpp} (65%)\n> > > create mode 100644 src/ipa/rpi/common/ipa_base.h\n> > > create mode 100644 src/ipa/rpi/common/meson.build\n> > > create mode 100644 src/ipa/rpi/vc4/vc4.cpp\n> > >\n> > > diff --git a/src/ipa/meson.build b/src/ipa/meson.build\n> > > index 10d3b44ca7b6..0c622c38a4a0 100644\n> > > --- a/src/ipa/meson.build\n> > > +++ b/src/ipa/meson.build\n> > > @@ -37,13 +37,14 @@ endif\n> > >\n> > > enabled_ipa_modules = []\n> > >\n> > > -# If the Raspberry Pi VC4 IPA is enabled, ensure we include the rpi/cam_helper\n> > > -# and rpi/controller subdirectories in the build.\n> > > +# If the Raspberry Pi VC4 IPA is enabled, ensure we include the cam_helper,\n> > > +# common and controller subdirectories in the build.\n> > > #\n> > > # This is done here and not within rpi/vc4/meson.build as meson does not\n> > > # allow the subdir command to traverse up the directory tree.\n> > > if pipelines.contains('rpi/vc4')\n> > > subdir('rpi/cam_helper')\n> > > + subdir('rpi/common')\n> > > subdir('rpi/controller')\n> > > endif\n> > >\n> > > diff --git a/src/ipa/rpi/vc4/raspberrypi.cpp b/src/ipa/rpi/common/ipa_base.cpp\n> > > similarity index 65%\n> > > rename from src/ipa/rpi/vc4/raspberrypi.cpp\n> > > rename to src/ipa/rpi/common/ipa_base.cpp\n> > > index e3ca8e2f7cbe..becada5973ad 100644\n> > > --- a/src/ipa/rpi/vc4/raspberrypi.cpp\n> > > +++ b/src/ipa/rpi/common/ipa_base.cpp\n> > > @@ -1,60 +1,30 @@\n> >\n> > [snip]\n> >\n> > > @@ -62,8 +32,7 @@ namespace libcamera {\n> > > using namespace std::literals::chrono_literals;\n> > > using utils::Duration;\n> > >\n> > > -/* Number of metadata objects available in the context list. */\n> > > -constexpr unsigned int numMetadataContexts = 16;\n> > > +namespace {\n> > >\n> > > /* Number of frame length times to hold in the queue. */\n> > > constexpr unsigned int FrameLengthsQueueSize = 10;\n> > > @@ -80,7 +49,7 @@ constexpr Duration defaultMaxFrameDuration = 250.0s;\n> > > * we rate-limit the controller Prepare() and Process() calls to lower than or\n> > > * equal to this rate.\n> > > */\n> > > -constexpr Duration controllerMinFrameDuration = 1.0s / 30.0;\n> > > +static constexpr Duration controllerMinFrameDuration = 1.0s / 30.0;\n> >\n> > Everything contained in an anonymous namespace becomes static. You can\n> > drop the static keyword from other variables.\n> >\n> > >\n> > > /* List of controls handled by the Raspberry Pi IPA */\n> > > static const ControlInfoMap::Map ipaControls{\n> > > @@ -116,118 +85,23 @@ static const ControlInfoMap::Map ipaAfControls{\n> > > { &controls::LensPosition, ControlInfo(0.0f, 32.0f, 1.0f) }\n> > > };\n> > >\n> > > +} /* namespace */\n> > > +\n> >\n> > [snip]\n> >\n> > > diff --git a/src/ipa/rpi/common/ipa_base.h b/src/ipa/rpi/common/ipa_base.h\n> > > new file mode 100644\n> > > index 000000000000..b9fbf9414d63\n> > > --- /dev/null\n> > > +++ b/src/ipa/rpi/common/ipa_base.h\n> > > @@ -0,0 +1,125 @@\n> > > +/* SPDX-License-Identifier: BSD-2-Clause */\n> > > +/*\n> > > + * Copyright (C) 2023, Raspberry Pi Ltd\n> > > + *\n> > > + * ipa_base.cpp - Raspberry Pi IPA base class\n> >\n> > ipa_base.h\n> >\n> > > + */\n> > > +#pragma once\n> > > +\n> > > +#include \n> > > +#include \n> > > +#include \n> >\n> > You need here, but can drop as it's only used to\n> > implement the interface of the base class, and so is provided by\n> > libcamera/ipa/ipa_interface.h. You also need stdint.h.\n> >\n> > > +\n> >\n> > #include \n> >\n> > > +#include \n> > > +\n> > > +#include \n> > > +\n> > > +#include \n> > > +#include \n> >\n> > Not needed.\n> >\n> > > +#include \n> > > +\n> > > +#include \"libcamera/internal/mapped_framebuffer.h\"\n> > > +\n> > > +#include \"cam_helper/cam_helper.h\"\n> > > +#include \"controller/agc_status.h\"\n> > > +#include \"controller/camera_mode.h\"\n> > > +#include \"controller/controller.h\"\n> > > +#include \"controller/metadata.h\"\n> > > +\n> > > +namespace libcamera {\n> > > +\n> > > +namespace ipa::RPi {\n> > > +\n> > > +class IpaBase : public IPARPiInterface\n> > > +{\n> > > +public:\n> > > + IpaBase();\n> > > + virtual ~IpaBase() = 0;\n> >\n> > That's a bit weird, why do you need a virtual abstract destructor ?\n>\n> This was a mistake, I'll remove it.\n>\n> >\n> > > +\n> > > + int32_t init(const IPASettings &settings, const InitParams ¶ms, InitResult *result) override;\n> > > + int32_t configure(const IPACameraSensorInfo &sensorInfo, const ConfigParams ¶ms,\n> > > + ConfigResult *result) override;\n> > > +\n> > > + void start(const ControlList &controls, StartResult *result) override;\n> > > + void stop() override {}\n> > > +\n> > > + void mapBuffers(const std::vector &buffers) override;\n> > > + void unmapBuffers(const std::vector &ids) override;\n> > > +\n> > > + void prepareIsp(const PrepareParams ¶ms) override;\n> > > + void processStats(const ProcessParams ¶ms) override;\n> > > +\n> > > + void reportMetadata(unsigned int ipaContext, ControlList *controls) override;\n> > > +\n> > > +private:\n> > > + /* Number of metadata objects available in the context list. */\n> > > + static constexpr unsigned int numMetadataContexts = 16;\n> > > +\n> > > + virtual int32_t platformInit(const InitParams ¶ms, InitResult *result) = 0;\n> >\n> > This enforces a particular sequence of calls between the base class and\n> > the derived class, dictated by the base class design. It's best to\n> > handle this in the derived class instead. You can override the init()\n> > function in the derived class, call the init() function of the base\n> > class, and then perform all the work you're currently doing in\n> > IpaVc4::platformInit().\n>\n> One of the earlier iterations had exactly this implementing. After\n> internal discussions, we decided to do it the other way round (i.e. the base\n> class implements the init() override). Out of curiosity, what advantages would\n> one way have over the other?\n>\n\nI admit that it was my suggestion to pick this style as originally\nthere was a mix of the two. Seeing an override calling into the base\nclass' overridden method felt a bit weird. I admit I've researched\n(briefly) what was the suggested way to handle methods composition in\nC++ and found nothing relevant, so my thought was that it was better\nto clearly define what was the interface that derived classes need to\nimplement, as the PipelineHandler/IPAInterface is implemented in the\nbase classes.\n\nWhen it comes to the call order, it's the base class driving the\nprocesing flow, while dispatching calls to the derived classes at the\nright time. Moving to your suggestion would allow derived classes to\ncall the base class method, which means most of the control is moved\nto the derived classes with possible code duplication.\n\nI think to actually decide where to go, it is useful to analyze what\nclass implements the operations flow control\n\nIn one case\n\n Base::method() Derived::platformMethod()\n\n doSomeWork();\n ....\n platformMethod() --> doPlaformWork();\n ...\n finalizeWork();\n\nWhile in the other\n\n Base::method() Derived::method()\n\n doSomeWork();\n ....\n method() <-- Base::method()\n ...\n finalizeWork();\n\nIs this an over-simplification ?\n\nNaush, do you expect the derived classes to control the operation\nflow, or will this be common to both and could be implemented in the\nbase classes ?\n\n> >\n> > The same comment applies to the other platform*() functions (some may\n> > require a little bit of refactoring). The handleControls() function can\n> > also be dropped by making applyControls() a protected virtual function\n> > that derived classes may override.\n> >\n> > > + virtual int32_t platformConfigure(const ConfigParams ¶ms, ConfigResult *result) = 0;\n> > > +\n> > > + virtual void platformPrepareIsp(const PrepareParams ¶ms,\n> > > + RPiController::Metadata &rpiMetadata) = 0;\n> > > + virtual RPiController::StatisticsPtr platformProcessStats(Span mem) = 0;\n> > > +\n> > > + void setMode(const IPACameraSensorInfo &sensorInfo);\n> > > + void setCameraTimeoutValue();\n> > > + bool validateSensorControls();\n> > > + bool validateLensControls();\n> > > + void applyControls(const ControlList &controls);\n> > > + virtual void handleControls(const ControlList &controls) = 0;\n> > > + void fillDeviceStatus(const ControlList &sensorControls, unsigned int ipaContext);\n> > > + void applyFrameDurations(utils::Duration minFrameDuration, utils::Duration maxFrameDuration);\n> > > + void applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls);\n> > > +\n> > > + std::map buffers_;\n> > > +\n> > > + bool lensPresent_;\n> > > + ControlList libcameraMetadata_;\n> > > +\n> > > + std::array rpiMetadata_;\n> > > +\n> > > + /*\n> > > + * We count frames to decide if the frame must be hidden (e.g. from\n> > > + * display) or mistrusted (i.e. not given to the control algos).\n> > > + */\n> > > + uint64_t frameCount_;\n> > > +\n> > > + /* How many frames we should avoid running control algos on. */\n> > > + unsigned int mistrustCount_;\n> > > +\n> > > + /* Number of frames that need to be dropped on startup. */\n> > > + unsigned int dropFrameCount_;\n> > > +\n> > > + /* Frame timestamp for the last run of the controller. */\n> > > + uint64_t lastRunTimestamp_;\n> > > +\n> > > + /* Do we run a Controller::process() for this frame? */\n> > > + bool processPending_;\n> > > +\n> > > + /* Distinguish the first camera start from others. */\n> > > + bool firstStart_;\n> > > +\n> > > + /* Frame duration (1/fps) limits. */\n> > > + utils::Duration minFrameDuration_;\n> > > + utils::Duration maxFrameDuration_;\n> > > +\n> > > +protected:\n> >\n> > We usually put protected members before private members.\n> >\n> > > + /* Raspberry Pi controller specific defines. */\n> > > + std::unique_ptr helper_;\n> > > + RPiController::Controller controller_;\n> > > +\n> > > + ControlInfoMap sensorCtrls_;\n> > > + ControlInfoMap lensCtrls_;\n> > > +\n> > > + /* Camera sensor params. */\n> > > + CameraMode mode_;\n> > > +\n> > > + /* Track the frame length times over FrameLengthsQueueSize frames. */\n> > > + std::deque frameLengths_;\n> > > + utils::Duration lastTimeout_;\n> > > +};\n> > > +\n> > > +} /* namespace ipa::RPi */\n> > > +\n> > > +} /* namespace libcamera */\n> > > diff --git a/src/ipa/rpi/common/meson.build b/src/ipa/rpi/common/meson.build\n> > > new file mode 100644\n> > > index 000000000000..a7189f8389af\n> > > --- /dev/null\n> > > +++ b/src/ipa/rpi/common/meson.build\n> > > @@ -0,0 +1,7 @@\n> > > +# SPDX-License-Identifier: CC0-1.0\n> > > +\n> > > +# SPDX-License-Identifier: CC0-1.0\n> >\n> > One SPDX header is enough :-)\n> >\n> > > +\n> > > +rpi_ipa_common_sources = files([\n> > > + 'ipa_base.cpp',\n> > > +])\n> > > diff --git a/src/ipa/rpi/vc4/meson.build b/src/ipa/rpi/vc4/meson.build\n> > > index 992d0f1ab5a7..b581391586b3 100644\n> > > --- a/src/ipa/rpi/vc4/meson.build\n> > > +++ b/src/ipa/rpi/vc4/meson.build\n> > > @@ -13,11 +13,15 @@ vc4_ipa_includes = [\n> > > ]\n> > >\n> > > vc4_ipa_sources = files([\n> > > - 'raspberrypi.cpp',\n> > > + 'vc4.cpp',\n> > > ])\n> > >\n> > > vc4_ipa_includes += include_directories('..')\n> > > -vc4_ipa_sources += [rpi_ipa_cam_helper_sources, rpi_ipa_controller_sources]\n> > > +vc4_ipa_sources += [\n> > > + rpi_ipa_cam_helper_sources,\n> > > + rpi_ipa_common_sources,\n> > > + rpi_ipa_controller_sources,\n> >\n> > Could we avoid compiling all these source files multiple times, once for\n> > the IpaVc4 IPA module, and once for the other IPA modules that may be\n> > implemented in the future ? This can be done with a static library, see\n> > src/apps/common/meson.build and src/apps/cam/meson.build for instance.\n> >\n> > > +]\n> > >\n> > > mod = shared_module(ipa_name,\n> > > [vc4_ipa_sources, libcamera_generated_ipa_headers],\n> >\n>\n> Sure, I'll fix this up.\n>\n> Naush\n>\n> > [snip]\n> >\n> > --\n> > Regards,\n> >\n> > Laurent Pinchart","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 6165CBDCBD\n\tfor ;\n\tFri, 28 Apr 2023 09:33:20 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D69B3627D4;\n\tFri, 28 Apr 2023 11:33:19 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A78B7627CD\n\tfor ;\n\tFri, 28 Apr 2023 11:33:18 +0200 (CEST)","from ideasonboard.com (93-61-96-190.ip145.fastwebnet.it\n\t[93.61.96.190])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id D22CB9B9;\n\tFri, 28 Apr 2023 11:33:05 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1682674399;\n\tbh=i/67ncZgnZH0mVSze2+Hd1T4r7z6Biw0xL59QriJGFk=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=sWZNar6SCFliG/JB0xrS7Eg0VbGoPbvr9/69NAE3sJ7el30V8VTBeWGRo0qptoK/f\n\t/nllD27ECWbXV4OzSCAt8rUaFG0Ri49unTgb3pKWRzQr9jYZCWe8B4x8I/amiXNtb9\n\tKs7Aw1rNNRy43zli5MI5DqAUGfzDcLGujaY5ECkDZA5WQ3DxtapNG9HLjY0iiKQ5Mb\n\tvHk39lJGtPaWqgxTJhnUWq6csY4OuNlW1odbP1e3LKex+1NgnY+cO5WDTjV5TVyVvY\n\tsJgESGDfRylPnOYnQ36kBEhlxt3AooUqecOd1xzLXsLyRi58vz3Mwv3IeQw+kA4qUN\n\tSeXKbBa305w4A==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1682674386;\n\tbh=i/67ncZgnZH0mVSze2+Hd1T4r7z6Biw0xL59QriJGFk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=ldU4SzGqoK1/TNomI4Vm3qLk69V3RAkiB0mBqAnDy7M9Yg8cTNMSb2oOACdtsHsZR\n\tEXsq++6tXciD4TsUYNF/ZBftOE5/d++Tq8uvgEsnm/SIO91Xlswq31p86roSh6Y+A1\n\tonAPb5skc/dvJsXFf71Wj/7VbcE6TzXEwI/GinXo="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"ldU4SzGq\"; dkim-atps=neutral","Date":"Fri, 28 Apr 2023 11:33:14 +0200","To":"Naushir Patuck ","Message-ID":"","References":"<20230426131057.21550-1-naush@raspberrypi.com>\n\t<20230426131057.21550-9-naush@raspberrypi.com>\n\t<20230427155415.GI28489@pendragon.ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 08/13] ipa: raspberrypi: Introduce\n\tIpaBase class","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Jacopo Mondi via libcamera-devel ","Reply-To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":26991,"web_url":"https://patchwork.libcamera.org/comment/26991/","msgid":"","date":"2023-04-28T13:57:47","subject":"Re: [libcamera-devel] [PATCH 08/13] ipa: raspberrypi: Introduce\n\tIpaBase class","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Jacopo,\n\nOn Fri, 28 Apr 2023 at 10:33, Jacopo Mondi\n wrote:\n>\n> Hi Naush\n>\n> On Fri, Apr 28, 2023 at 09:35:51AM +0100, Naushir Patuck via libcamera-devel wrote:\n> > Hi Laurent,\n> >\n> > Thank you for your feedback.\n> >\n> > On Thu, 27 Apr 2023 at 16:54, Laurent Pinchart\n> > wrote:\n> > >\n> > > Hi Naush,\n> > >\n> > > Thank you for the patch.\n> > >\n> > > On Wed, Apr 26, 2023 at 02:10:52PM +0100, Naushir Patuck via libcamera-devel wrote:\n> > > > Create a new IpaBase class that handles general purpose housekeeping\n> > > > duties for the Raspberry Pi IPA. Code the implementation of new class is\n> > >\n> > > Did you mean s/Code the implementation/The implementation/ and\n> > > s/of new class/of the new class/ ?\n> >\n> > I actually meant \"Code for the implementation...\", but I'll replace it\n> > with your wording.\n> >\n> > >\n> > > > essentially pulled from the existing ipa/rpi/vc4/raspberrypi.cpp\n> > > > file with a minimal amount of refactoring.\n> > > >\n> > > > Create a derived IpaVc4 class from IpaBase that handles the VC4 pipeline\n> > > > specific tasks of the IPA. Again, code for this class implementation is\n> > > > taken from the existing ipa/rpi/vc4/raspberrypi.cpp with a\n> > > > minimal amount of refactoring.\n> > > >\n> > > > The goal of this change is to allow third parties to implement their own\n> > > > IPA running on the Raspberry Pi without duplicating all of the IPA\n> > > > housekeeping tasks.\n> > > >\n> > > > Signed-off-by: Naushir Patuck \n> > > > ---\n> > > > src/ipa/meson.build | 5 +-\n> > > > .../raspberrypi.cpp => common/ipa_base.cpp} | 1219 +++++------------\n> > > > src/ipa/rpi/common/ipa_base.h | 125 ++\n> > > > src/ipa/rpi/common/meson.build | 7 +\n> > > > src/ipa/rpi/vc4/meson.build | 8 +-\n> > > > src/ipa/rpi/vc4/vc4.cpp | 540 ++++++++\n> > > > 6 files changed, 1012 insertions(+), 892 deletions(-)\n> > > > rename src/ipa/rpi/{vc4/raspberrypi.cpp => common/ipa_base.cpp} (65%)\n> > > > create mode 100644 src/ipa/rpi/common/ipa_base.h\n> > > > create mode 100644 src/ipa/rpi/common/meson.build\n> > > > create mode 100644 src/ipa/rpi/vc4/vc4.cpp\n> > > >\n> > > > diff --git a/src/ipa/meson.build b/src/ipa/meson.build\n> > > > index 10d3b44ca7b6..0c622c38a4a0 100644\n> > > > --- a/src/ipa/meson.build\n> > > > +++ b/src/ipa/meson.build\n> > > > @@ -37,13 +37,14 @@ endif\n> > > >\n> > > > enabled_ipa_modules = []\n> > > >\n> > > > -# If the Raspberry Pi VC4 IPA is enabled, ensure we include the rpi/cam_helper\n> > > > -# and rpi/controller subdirectories in the build.\n> > > > +# If the Raspberry Pi VC4 IPA is enabled, ensure we include the cam_helper,\n> > > > +# common and controller subdirectories in the build.\n> > > > #\n> > > > # This is done here and not within rpi/vc4/meson.build as meson does not\n> > > > # allow the subdir command to traverse up the directory tree.\n> > > > if pipelines.contains('rpi/vc4')\n> > > > subdir('rpi/cam_helper')\n> > > > + subdir('rpi/common')\n> > > > subdir('rpi/controller')\n> > > > endif\n> > > >\n> > > > diff --git a/src/ipa/rpi/vc4/raspberrypi.cpp b/src/ipa/rpi/common/ipa_base.cpp\n> > > > similarity index 65%\n> > > > rename from src/ipa/rpi/vc4/raspberrypi.cpp\n> > > > rename to src/ipa/rpi/common/ipa_base.cpp\n> > > > index e3ca8e2f7cbe..becada5973ad 100644\n> > > > --- a/src/ipa/rpi/vc4/raspberrypi.cpp\n> > > > +++ b/src/ipa/rpi/common/ipa_base.cpp\n> > > > @@ -1,60 +1,30 @@\n> > >\n> > > [snip]\n> > >\n> > > > @@ -62,8 +32,7 @@ namespace libcamera {\n> > > > using namespace std::literals::chrono_literals;\n> > > > using utils::Duration;\n> > > >\n> > > > -/* Number of metadata objects available in the context list. */\n> > > > -constexpr unsigned int numMetadataContexts = 16;\n> > > > +namespace {\n> > > >\n> > > > /* Number of frame length times to hold in the queue. */\n> > > > constexpr unsigned int FrameLengthsQueueSize = 10;\n> > > > @@ -80,7 +49,7 @@ constexpr Duration defaultMaxFrameDuration = 250.0s;\n> > > > * we rate-limit the controller Prepare() and Process() calls to lower than or\n> > > > * equal to this rate.\n> > > > */\n> > > > -constexpr Duration controllerMinFrameDuration = 1.0s / 30.0;\n> > > > +static constexpr Duration controllerMinFrameDuration = 1.0s / 30.0;\n> > >\n> > > Everything contained in an anonymous namespace becomes static. You can\n> > > drop the static keyword from other variables.\n> > >\n> > > >\n> > > > /* List of controls handled by the Raspberry Pi IPA */\n> > > > static const ControlInfoMap::Map ipaControls{\n> > > > @@ -116,118 +85,23 @@ static const ControlInfoMap::Map ipaAfControls{\n> > > > { &controls::LensPosition, ControlInfo(0.0f, 32.0f, 1.0f) }\n> > > > };\n> > > >\n> > > > +} /* namespace */\n> > > > +\n> > >\n> > > [snip]\n> > >\n> > > > diff --git a/src/ipa/rpi/common/ipa_base.h b/src/ipa/rpi/common/ipa_base.h\n> > > > new file mode 100644\n> > > > index 000000000000..b9fbf9414d63\n> > > > --- /dev/null\n> > > > +++ b/src/ipa/rpi/common/ipa_base.h\n> > > > @@ -0,0 +1,125 @@\n> > > > +/* SPDX-License-Identifier: BSD-2-Clause */\n> > > > +/*\n> > > > + * Copyright (C) 2023, Raspberry Pi Ltd\n> > > > + *\n> > > > + * ipa_base.cpp - Raspberry Pi IPA base class\n> > >\n> > > ipa_base.h\n> > >\n> > > > + */\n> > > > +#pragma once\n> > > > +\n> > > > +#include \n> > > > +#include \n> > > > +#include \n> > >\n> > > You need here, but can drop as it's only used to\n> > > implement the interface of the base class, and so is provided by\n> > > libcamera/ipa/ipa_interface.h. You also need stdint.h.\n> > >\n> > > > +\n> > >\n> > > #include \n> > >\n> > > > +#include \n> > > > +\n> > > > +#include \n> > > > +\n> > > > +#include \n> > > > +#include \n> > >\n> > > Not needed.\n> > >\n> > > > +#include \n> > > > +\n> > > > +#include \"libcamera/internal/mapped_framebuffer.h\"\n> > > > +\n> > > > +#include \"cam_helper/cam_helper.h\"\n> > > > +#include \"controller/agc_status.h\"\n> > > > +#include \"controller/camera_mode.h\"\n> > > > +#include \"controller/controller.h\"\n> > > > +#include \"controller/metadata.h\"\n> > > > +\n> > > > +namespace libcamera {\n> > > > +\n> > > > +namespace ipa::RPi {\n> > > > +\n> > > > +class IpaBase : public IPARPiInterface\n> > > > +{\n> > > > +public:\n> > > > + IpaBase();\n> > > > + virtual ~IpaBase() = 0;\n> > >\n> > > That's a bit weird, why do you need a virtual abstract destructor ?\n> >\n> > This was a mistake, I'll remove it.\n> >\n> > >\n> > > > +\n> > > > + int32_t init(const IPASettings &settings, const InitParams ¶ms, InitResult *result) override;\n> > > > + int32_t configure(const IPACameraSensorInfo &sensorInfo, const ConfigParams ¶ms,\n> > > > + ConfigResult *result) override;\n> > > > +\n> > > > + void start(const ControlList &controls, StartResult *result) override;\n> > > > + void stop() override {}\n> > > > +\n> > > > + void mapBuffers(const std::vector &buffers) override;\n> > > > + void unmapBuffers(const std::vector &ids) override;\n> > > > +\n> > > > + void prepareIsp(const PrepareParams ¶ms) override;\n> > > > + void processStats(const ProcessParams ¶ms) override;\n> > > > +\n> > > > + void reportMetadata(unsigned int ipaContext, ControlList *controls) override;\n> > > > +\n> > > > +private:\n> > > > + /* Number of metadata objects available in the context list. */\n> > > > + static constexpr unsigned int numMetadataContexts = 16;\n> > > > +\n> > > > + virtual int32_t platformInit(const InitParams ¶ms, InitResult *result) = 0;\n> > >\n> > > This enforces a particular sequence of calls between the base class and\n> > > the derived class, dictated by the base class design. It's best to\n> > > handle this in the derived class instead. You can override the init()\n> > > function in the derived class, call the init() function of the base\n> > > class, and then perform all the work you're currently doing in\n> > > IpaVc4::platformInit().\n> >\n> > One of the earlier iterations had exactly this implementing. After\n> > internal discussions, we decided to do it the other way round (i.e. the base\n> > class implements the init() override). Out of curiosity, what advantages would\n> > one way have over the other?\n> >\n>\n> I admit that it was my suggestion to pick this style as originally\n> there was a mix of the two. Seeing an override calling into the base\n> class' overridden method felt a bit weird. I admit I've researched\n> (briefly) what was the suggested way to handle methods composition in\n> C++ and found nothing relevant, so my thought was that it was better\n> to clearly define what was the interface that derived classes need to\n> implement, as the PipelineHandler/IPAInterface is implemented in the\n> base classes.\n>\n> When it comes to the call order, it's the base class driving the\n> procesing flow, while dispatching calls to the derived classes at the\n> right time. Moving to your suggestion would allow derived classes to\n> call the base class method, which means most of the control is moved\n> to the derived classes with possible code duplication.\n>\n> I think to actually decide where to go, it is useful to analyze what\n> class implements the operations flow control\n>\n> In one case\n>\n> Base::method() Derived::platformMethod()\n>\n> doSomeWork();\n> ....\n> platformMethod() --> doPlaformWork();\n> ...\n> finalizeWork();\n>\n> While in the other\n>\n> Base::method() Derived::method()\n>\n> doSomeWork();\n> ....\n> method() <-- Base::method()\n> ...\n> finalizeWork();\n>\n> Is this an over-simplification ?\n>\n> Naush, do you expect the derived classes to control the operation\n> flow, or will this be common to both and could be implemented in the\n> base classes ?\n\nNo, I see no explicit need for operation flow to be dictated by the derived\nclass. Hence changing things around to your suggestion and making things more\nconsistent.\n\nRegards,\nNaush\n\n\n>\n> > >\n> > > The same comment applies to the other platform*() functions (some may\n> > > require a little bit of refactoring). The handleControls() function can\n> > > also be dropped by making applyControls() a protected virtual function\n> > > that derived classes may override.\n> > >\n> > > > + virtual int32_t platformConfigure(const ConfigParams ¶ms, ConfigResult *result) = 0;\n> > > > +\n> > > > + virtual void platformPrepareIsp(const PrepareParams ¶ms,\n> > > > + RPiController::Metadata &rpiMetadata) = 0;\n> > > > + virtual RPiController::StatisticsPtr platformProcessStats(Span mem) = 0;\n> > > > +\n> > > > + void setMode(const IPACameraSensorInfo &sensorInfo);\n> > > > + void setCameraTimeoutValue();\n> > > > + bool validateSensorControls();\n> > > > + bool validateLensControls();\n> > > > + void applyControls(const ControlList &controls);\n> > > > + virtual void handleControls(const ControlList &controls) = 0;\n> > > > + void fillDeviceStatus(const ControlList &sensorControls, unsigned int ipaContext);\n> > > > + void applyFrameDurations(utils::Duration minFrameDuration, utils::Duration maxFrameDuration);\n> > > > + void applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls);\n> > > > +\n> > > > + std::map buffers_;\n> > > > +\n> > > > + bool lensPresent_;\n> > > > + ControlList libcameraMetadata_;\n> > > > +\n> > > > + std::array rpiMetadata_;\n> > > > +\n> > > > + /*\n> > > > + * We count frames to decide if the frame must be hidden (e.g. from\n> > > > + * display) or mistrusted (i.e. not given to the control algos).\n> > > > + */\n> > > > + uint64_t frameCount_;\n> > > > +\n> > > > + /* How many frames we should avoid running control algos on. */\n> > > > + unsigned int mistrustCount_;\n> > > > +\n> > > > + /* Number of frames that need to be dropped on startup. */\n> > > > + unsigned int dropFrameCount_;\n> > > > +\n> > > > + /* Frame timestamp for the last run of the controller. */\n> > > > + uint64_t lastRunTimestamp_;\n> > > > +\n> > > > + /* Do we run a Controller::process() for this frame? */\n> > > > + bool processPending_;\n> > > > +\n> > > > + /* Distinguish the first camera start from others. */\n> > > > + bool firstStart_;\n> > > > +\n> > > > + /* Frame duration (1/fps) limits. */\n> > > > + utils::Duration minFrameDuration_;\n> > > > + utils::Duration maxFrameDuration_;\n> > > > +\n> > > > +protected:\n> > >\n> > > We usually put protected members before private members.\n> > >\n> > > > + /* Raspberry Pi controller specific defines. */\n> > > > + std::unique_ptr helper_;\n> > > > + RPiController::Controller controller_;\n> > > > +\n> > > > + ControlInfoMap sensorCtrls_;\n> > > > + ControlInfoMap lensCtrls_;\n> > > > +\n> > > > + /* Camera sensor params. */\n> > > > + CameraMode mode_;\n> > > > +\n> > > > + /* Track the frame length times over FrameLengthsQueueSize frames. */\n> > > > + std::deque frameLengths_;\n> > > > + utils::Duration lastTimeout_;\n> > > > +};\n> > > > +\n> > > > +} /* namespace ipa::RPi */\n> > > > +\n> > > > +} /* namespace libcamera */\n> > > > diff --git a/src/ipa/rpi/common/meson.build b/src/ipa/rpi/common/meson.build\n> > > > new file mode 100644\n> > > > index 000000000000..a7189f8389af\n> > > > --- /dev/null\n> > > > +++ b/src/ipa/rpi/common/meson.build\n> > > > @@ -0,0 +1,7 @@\n> > > > +# SPDX-License-Identifier: CC0-1.0\n> > > > +\n> > > > +# SPDX-License-Identifier: CC0-1.0\n> > >\n> > > One SPDX header is enough :-)\n> > >\n> > > > +\n> > > > +rpi_ipa_common_sources = files([\n> > > > + 'ipa_base.cpp',\n> > > > +])\n> > > > diff --git a/src/ipa/rpi/vc4/meson.build b/src/ipa/rpi/vc4/meson.build\n> > > > index 992d0f1ab5a7..b581391586b3 100644\n> > > > --- a/src/ipa/rpi/vc4/meson.build\n> > > > +++ b/src/ipa/rpi/vc4/meson.build\n> > > > @@ -13,11 +13,15 @@ vc4_ipa_includes = [\n> > > > ]\n> > > >\n> > > > vc4_ipa_sources = files([\n> > > > - 'raspberrypi.cpp',\n> > > > + 'vc4.cpp',\n> > > > ])\n> > > >\n> > > > vc4_ipa_includes += include_directories('..')\n> > > > -vc4_ipa_sources += [rpi_ipa_cam_helper_sources, rpi_ipa_controller_sources]\n> > > > +vc4_ipa_sources += [\n> > > > + rpi_ipa_cam_helper_sources,\n> > > > + rpi_ipa_common_sources,\n> > > > + rpi_ipa_controller_sources,\n> > >\n> > > Could we avoid compiling all these source files multiple times, once for\n> > > the IpaVc4 IPA module, and once for the other IPA modules that may be\n> > > implemented in the future ? This can be done with a static library, see\n> > > src/apps/common/meson.build and src/apps/cam/meson.build for instance.\n> > >\n> > > > +]\n> > > >\n> > > > mod = shared_module(ipa_name,\n> > > > [vc4_ipa_sources, libcamera_generated_ipa_headers],\n> > >\n> >\n> > Sure, I'll fix this up.\n> >\n> > Naush\n> >\n> > > [snip]\n> > >\n> > > --\n> > > Regards,\n> > >\n> > > Laurent Pinchart","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id BDBB5BDCBD\n\tfor ;\n\tFri, 28 Apr 2023 13:58:06 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3777E627D4;\n\tFri, 28 Apr 2023 15:58:06 +0200 (CEST)","from mail-yw1-x112b.google.com (mail-yw1-x112b.google.com\n\t[IPv6:2607:f8b0:4864:20::112b])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D0553627D3\n\tfor ;\n\tFri, 28 Apr 2023 15:58:03 +0200 (CEST)","by mail-yw1-x112b.google.com with SMTP id\n\t00721157ae682-54f8b46d031so114120867b3.3\n\tfor ;\n\tFri, 28 Apr 2023 06:58:03 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1682690286;\n\tbh=Iu5qb0zLUSFr/7rZGGV5b+IyE0RV499N9tX6BCssjRQ=;\n\th=References:In-Reply-To:Date:To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=anetZDb+bKBhuUO28XQWslsoeN6YZUNJA82YnQoTUQQR17Sj6RbQO+F8kNMg+iBX5\n\tZnrSxj10YZy6Lqr+SE/lqElQ/qUTLdE0xE0KDO3Syz2Mq3AWU+BM88YY2107HxDZze\n\tnv+y4BIfomBt/4UcP8dWYo173tNjyxrwMsei6ew1QpGOCD07749OrqBJH2CkaOSsZM\n\t9AJgrH33hwvX4zJnLs5EbEhoozLsESmLzUQEvKvg6+lvClGQRrNCN/tb9V1RQ2SUXx\n\t4dllnPE+lrEfSWxBX/e5x0o2RlpTlpOGom5XfR9qaLXTzCCigk5dxSdjDqBxBNBA4r\n\tSjiqnxLXmdJLg==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1682690282; x=1685282282;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc:subject:date:message-id:reply-to;\n\tbh=UQogB4JmLwYqjCgidCJoIA4IxOZ8QzBqrDa/43NhYKE=;\n\tb=FfyMoMut1kHThsIdy1LXvxXNyiAgHU/4X5YbXSf5KHtlBNKSqwOvXKc94IJvZVQvj4\n\t8aTNiSpYfzRHryBWHBj4qx0VI4oItLpfau/FMokihVBR7GfR2Qu4kJzCZOh0FqgIcL3q\n\tzQ1t61UyxQYZmiKoWIjPxfVK/qPXtRWV1CsfrY4/FGBLODPYoDJaDmuuHA07OwHSRzCp\n\tYUUN5G43u/r7MZAqoZgWkBuUegSqAxtAoNe3bXhbiOt3AyKM/6buzaMcHDtGgk7fEBNJ\n\tLxKSEV41+guljuUgrUS7LqWbqR8c2pwOFCfUUREnZDYHqUDMQHO+61b3+XCEr7trhrft\n\tV1bA=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"FfyMoMut\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20221208; t=1682690282; x=1685282282;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-message-state:from:to:cc:subject:date:message-id\n\t:reply-to;\n\tbh=UQogB4JmLwYqjCgidCJoIA4IxOZ8QzBqrDa/43NhYKE=;\n\tb=j1981WV7DWBed3BND/ZHg1c9z8g67K8J9GX7yMKNKVMifl5RD42d+Ric53DyTuVQtQ\n\taoRyLkfbudkPxB69KAoXjMRCSLnbhvB64aPgZv/EqmrGMjgjOv2LOw0Fw012A086RVRf\n\ttRFSUTB6eZVQItFQ4eCem6Fp/cdLjpe0FbfR47FpEEU+uXV1KfOWUll1W/M68PiRE54c\n\tKn/25KSL5mbekU/C1PPAUx4m/Ks7m5c1ljUBqhVrdLvLTYfb6Cy7GcaR6zkwLZ9dtn1Z\n\tuMINVWxGVj+UjK1PGDG4tGgZ7Gl4NV+MFMeyeS/vcJXgQQvYuIVV8CUP8Ykye0EjGVHo\n\tnfEg==","X-Gm-Message-State":"AC+VfDzwjCh3ZIZ3zOjG8qn2aogCQALDR5zPg90qYE4uciy1Mf08TF2a\n\tsNMUSUxhrqobJ8EWdbCdZHzcRXZjyNRbo9/L/cFkDj5xYfLtcyBwWTazWQ==","X-Google-Smtp-Source":"ACHHUZ7hCi80sPLiv/yZLAnAu0Os4m4zSBrXHudi0CfXgfhP97qDPCW7ZPasP31Np+dJ7pm4YUuAuFW6P80EVrPpnj0=","X-Received":"by 2002:a0d:d74f:0:b0:552:e1d1:bdb0 with SMTP id\n\tz76-20020a0dd74f000000b00552e1d1bdb0mr4000831ywd.43.1682690282553;\n\tFri, 28 Apr 2023 06:58:02 -0700 (PDT)","MIME-Version":"1.0","References":"<20230426131057.21550-1-naush@raspberrypi.com>\n\t<20230426131057.21550-9-naush@raspberrypi.com>\n\t<20230427155415.GI28489@pendragon.ideasonboard.com>\n\t\n\t","In-Reply-To":"","Date":"Fri, 28 Apr 2023 14:57:47 +0100","Message-ID":"","To":"Jacopo Mondi ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH 08/13] ipa: raspberrypi: Introduce\n\tIpaBase class","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Naushir Patuck via libcamera-devel\n\t","Reply-To":"Naushir Patuck ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":27008,"web_url":"https://patchwork.libcamera.org/comment/27008/","msgid":"","date":"2023-05-02T14:54:16","subject":"Re: [libcamera-devel] [PATCH 08/13] ipa: raspberrypi: Introduce\n\tIpaBase class","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Laurent and Jacopo,\n\nOn Fri, 28 Apr 2023 at 14:57, Naushir Patuck wrote:\n>\n> Hi Jacopo,\n>\n\n \n\n> >\n> > When it comes to the call order, it's the base class driving the\n> > procesing flow, while dispatching calls to the derived classes at the\n> > right time. Moving to your suggestion would allow derived classes to\n> > call the base class method, which means most of the control is moved\n> > to the derived classes with possible code duplication.\n> >\n> > I think to actually decide where to go, it is useful to analyze what\n> > class implements the operations flow control\n> >\n> > In one case\n> >\n> > Base::method() Derived::platformMethod()\n> >\n> > doSomeWork();\n> > ....\n> > platformMethod() --> doPlaformWork();\n> > ...\n> > finalizeWork();\n> >\n> > While in the other\n> >\n> > Base::method() Derived::method()\n> >\n> > doSomeWork();\n> > ....\n> > method() <-- Base::method()\n> > ...\n> > finalizeWork();\n> >\n> > Is this an over-simplification ?\n> >\n> > Naush, do you expect the derived classes to control the operation\n> > flow, or will this be common to both and could be implemented in the\n> > base classes ?\n>\n> No, I see no explicit need for operation flow to be dictated by the derived\n> class. Hence changing things around to your suggestion and making things more\n> consistent.\n\nHaving looked further into this, I'm a bit more conflicted. I initially thought\nthat I could simply revert to the original mechanism of having the control flow\nstart from the derived class and call into the base class, as the 2nd case\ndrawn above. This does work neatly for the ipa::init() and ipa::configure().\nHowever platformProcessStats() and platformPrepareIsp() require the control flow\nfrom base class to derived class, as in the 1st case drawn above.\n\nI don't think it's sensible to choose flow 1 for some functions and flow 2 for\nothers. So that suggests to me that flow 1 (i.e. the existing flow) is probably\nthe right approach for the IPA changes. Do folks think that makes sense?\nAgain, I am happy to change things if it seems more appropriate.\n\nRegards,\nNaush","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 48FF4C0DA4\n\tfor ;\n\tTue, 2 May 2023 14:54:35 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A199D627DC;\n\tTue, 2 May 2023 16:54:34 +0200 (CEST)","from mail-yb1-xb2d.google.com (mail-yb1-xb2d.google.com\n\t[IPv6:2607:f8b0:4864:20::b2d])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 06E3560538\n\tfor ;\n\tTue, 2 May 2023 16:54:31 +0200 (CEST)","by mail-yb1-xb2d.google.com with SMTP id\n\t3f1490d57ef6-b9e2b65d006so3049948276.3\n\tfor ;\n\tTue, 02 May 2023 07:54:31 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1683039274;\n\tbh=lX9ZINaoOKUWgayttGiZ3i34L12OiOv3j9c4sIa6M4I=;\n\th=References:In-Reply-To:Date:To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=ggbnklx1LbonccDkGzW2869ntn4WnS4SBn7eJ4X+SyRczOWD8aKb+OkmPVdgduDd2\n\tAR9WKf7frCb3qjCW2ZO/geX3DMlv/rccMUMsHDrPl5p7RIx/lF4ed+D7cL92c4xR7f\n\tVqFHH+U32BbsLjXnlk+P+JJ53jxkRKMcpS/fOp+BcDls/ZPgZNbM/BCDztKr78SgfN\n\tUWSpkCTBwriCDxDL+aYkKaZ+u7TfnGNYG0jPQMRonov86C1Fgd+w8HvJ4z5cpdwEvY\n\t32pOoAvHFaqite15bm5qs1+v0hkzXW6nUnihKucRF6B1KKmAv7mCmZj7mWfN9BUJR5\n\tykqmlPVEvn4vA==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1683039270; x=1685631270;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc:subject:date:message-id:reply-to;\n\tbh=BEHJKDAPihNAZ8VkMVnaqXTEZnNSw+nrJwpUpJS9q0U=;\n\tb=aZofL/+eAHcCHM8ZVkn0UAHt0W865niVmXKlcyjMFN80QnlvduH04OVq+WVKPrdudY\n\t3pZ5j2vR3aaqSvU4IZYCwNYI6txarr99lFNNkX0crunfoOgvbV50CehjvHkT5ANwaGs5\n\tVLelT7YjHIm/Yb0gJ7dea5zqnjeolki1k0hqFliKeFVM/mLoUPQqLFjXF8XnCBcuDms0\n\tni89WfiiF9OICalt+cQWqiTU6qCV/h9a5KvAfIBbi/w7SYqU/I2Spu3VEszUAq8mIT2B\n\tjQNtEKLMmfwgeFhRbnMWqI1P+X3bR/KKUpjvKkNz9LDpUd/+U6rYY+kJ03eY8g1TdeNd\n\t5nuA=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"aZofL/+e\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20221208; t=1683039270; x=1685631270;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-message-state:from:to:cc:subject:date:message-id\n\t:reply-to;\n\tbh=BEHJKDAPihNAZ8VkMVnaqXTEZnNSw+nrJwpUpJS9q0U=;\n\tb=bnPeWwSAW9OYW5+8iFvCUmwZ0Lztg1BIIsWUN7gKyrJfzGJbXwnAi6XClBlYzhPZBD\n\t5nlhVF5Eu6BDP0g2AHSJ6rf9ZcIFCbY19Zdd7pwVOfVrB2qLmiMta6LbHhTfjlH9n1bw\n\tkN7+4m/OovS0guedrBizzs7QeRxEwzJSI+p42kwRqXw58X7NbiQmRwKrZVr4u6v5gqII\n\t9m39oqP3IetzWRelPavs3WqlXNMErgTENZdNSQHcrsClDexai3wpssugQnzoiwICinso\n\t3KCZ+wAwxbOYj1F6z72V1RgB+dm9Yv/SQOZ/zp4Q58G4q4tgVmuaWCMyHGztGSe96ASB\n\tcG1Q==","X-Gm-Message-State":"AC+VfDxsLoMKCHcj5ukHsBMmlPI9phS5Jtbwg+s/4aiIn2MpCb4Iul+X\n\truRvROKY0VHmaaOC3rZtm77Zh1Ka6PbZCASUcyoLbrb47BruYU1K+oIMBw==","X-Google-Smtp-Source":"ACHHUZ7+SiFPlg+if7ZZOcrtJmEk1j+pS3ifm6foCmHSTwv27XbjcwOV1B7QYdndYJP5LR/9HHeD0rYyK4uTsDo6PF8=","X-Received":"by 2002:a25:69c7:0:b0:b9e:9515:4581 with SMTP id\n\te190-20020a2569c7000000b00b9e95154581mr360177ybc.56.1683039270569;\n\tTue, 02 May 2023 07:54:30 -0700 (PDT)","MIME-Version":"1.0","References":"<20230426131057.21550-1-naush@raspberrypi.com>\n\t<20230426131057.21550-9-naush@raspberrypi.com>\n\t<20230427155415.GI28489@pendragon.ideasonboard.com>\n\t\n\t\n\t","In-Reply-To":"","Date":"Tue, 2 May 2023 15:54:16 +0100","Message-ID":"","To":"Jacopo Mondi ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH 08/13] ipa: raspberrypi: Introduce\n\tIpaBase class","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Naushir Patuck via libcamera-devel\n\t","Reply-To":"Naushir Patuck ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":27009,"web_url":"https://patchwork.libcamera.org/comment/27009/","msgid":"<20230502151643.GA22691@pendragon.ideasonboard.com>","date":"2023-05-02T15:16:43","subject":"Re: [libcamera-devel] [PATCH 08/13] ipa: raspberrypi: Introduce\n\tIpaBase class","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Naush,\n\nOn Tue, May 02, 2023 at 03:54:16PM +0100, Naushir Patuck wrote:\n> Hi Laurent and Jacopo,\n> \n> On Fri, 28 Apr 2023 at 14:57, Naushir Patuck wrote:\n> >\n> > Hi Jacopo,\n> \n> \n> \n> > > When it comes to the call order, it's the base class driving the\n> > > procesing flow, while dispatching calls to the derived classes at the\n> > > right time. Moving to your suggestion would allow derived classes to\n> > > call the base class method, which means most of the control is moved\n> > > to the derived classes with possible code duplication.\n> > >\n> > > I think to actually decide where to go, it is useful to analyze what\n> > > class implements the operations flow control\n> > >\n> > > In one case\n> > >\n> > > Base::method() Derived::platformMethod()\n> > >\n> > > doSomeWork();\n> > > ....\n> > > platformMethod() --> doPlaformWork();\n> > > ...\n> > > finalizeWork();\n> > >\n> > > While in the other\n> > >\n> > > Base::method() Derived::method()\n> > >\n> > > doSomeWork();\n> > > ....\n> > > method() <-- Base::method()\n> > > ...\n> > > finalizeWork();\n> > >\n> > > Is this an over-simplification ?\n> > >\n> > > Naush, do you expect the derived classes to control the operation\n> > > flow, or will this be common to both and could be implemented in the\n> > > base classes ?\n> >\n> > No, I see no explicit need for operation flow to be dictated by the derived\n> > class. Hence changing things around to your suggestion and making things more\n> > consistent.\n> \n> Having looked further into this, I'm a bit more conflicted. I initially thought\n> that I could simply revert to the original mechanism of having the control flow\n> start from the derived class and call into the base class, as the 2nd case\n> drawn above. This does work neatly for the ipa::init() and ipa::configure().\n> However platformProcessStats() and platformPrepareIsp() require the control flow\n> from base class to derived class, as in the 1st case drawn above.\n> \n> I don't think it's sensible to choose flow 1 for some functions and flow 2 for\n> others. So that suggests to me that flow 1 (i.e. the existing flow) is probably\n> the right approach for the IPA changes. Do folks think that makes sense?\n> Again, I am happy to change things if it seems more appropriate.\n\nAs mentioned in a different conversation, I'm fine keeping the existing\ncode flow, even if I think it would be best to reverse it. We can\nimprove this incrementally on top.","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 332A7BD16B\n\tfor ;\n\tTue, 2 May 2023 15:16:36 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B019B627DC;\n\tTue, 2 May 2023 17:16:35 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E67B860538\n\tfor ;\n\tTue, 2 May 2023 17:16:33 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(133-32-181-51.west.xps.vectant.ne.jp [133.32.181.51])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id EFD83800;\n\tTue, 2 May 2023 17:16:30 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1683040595;\n\tbh=PioYFdUs0ok/ps47bYGlqejh0AUMitkHvJtPcSqJ4u4=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=ns7bZ3sUIebD+A4pxh4uRbcKM0VU2xJgtaPH0IlOMLF3tcAUjvdQP2m9s+xwmUE6h\n\tbsbOjSwkb4uPwcSyuhamZ4gdYHJNGwN/+kpbTXm1BPfXwTtMPSf4KJBWWNYqJ0TCMf\n\tHLrdaKyVUhyZsg+JuLKVNw/Dd91R+IF3qeClQ47vgz7Pmv9XlVe00thSohLWtPiNrz\n\tX6cmhvduZbYID1ifjuWCD9gqjOg08TtrTmDI4V53Z3VSaCeSXsT7bUTeALxIqRw1Dc\n\tR369Pt9NOEcsZdF4J9VEEuTiI48KPMroV6W9Q65qSmKLqMcaP+dipH3ooHcu3/LDxS\n\tw548huNIXDuTw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1683040591;\n\tbh=PioYFdUs0ok/ps47bYGlqejh0AUMitkHvJtPcSqJ4u4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=TWc+xtNxHdNlKkWqwEY1ofJSEdId5u9Vcq/WBo0d/qSlRlnRqSUQtglF7OxqN+WxQ\n\tKkHk4nD0ZfncV1pnOxm+YxMZ8YlZ4DlaMvjc4/Mhty7Eo2CViCFaqW0Ey/VmjkwRUW\n\t7hLHWux1ZLRiz7up1R7YfzHdDwg0Gobq6p2CIxsU="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"TWc+xtNx\"; dkim-atps=neutral","Date":"Tue, 2 May 2023 18:16:43 +0300","To":"Naushir Patuck ","Message-ID":"<20230502151643.GA22691@pendragon.ideasonboard.com>","References":"<20230426131057.21550-1-naush@raspberrypi.com>\n\t<20230426131057.21550-9-naush@raspberrypi.com>\n\t<20230427155415.GI28489@pendragon.ideasonboard.com>\n\t\n\t\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 08/13] ipa: raspberrypi: Introduce\n\tIpaBase class","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"Jacopo Mondi ,\n\tlibcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":27010,"web_url":"https://patchwork.libcamera.org/comment/27010/","msgid":"","date":"2023-05-02T15:27:23","subject":"Re: [libcamera-devel] [PATCH 08/13] ipa: raspberrypi: Introduce\n\tIpaBase class","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Laurent,\n\nOn Tue, 2 May 2023 at 16:16, Laurent Pinchart\n wrote:\n>\n> Hi Naush,\n>\n> On Tue, May 02, 2023 at 03:54:16PM +0100, Naushir Patuck wrote:\n> > Hi Laurent and Jacopo,\n> >\n> > On Fri, 28 Apr 2023 at 14:57, Naushir Patuck wrote:\n> > >\n> > > Hi Jacopo,\n> >\n> > \n> >\n> > > > When it comes to the call order, it's the base class driving the\n> > > > procesing flow, while dispatching calls to the derived classes at the\n> > > > right time. Moving to your suggestion would allow derived classes to\n> > > > call the base class method, which means most of the control is moved\n> > > > to the derived classes with possible code duplication.\n> > > >\n> > > > I think to actually decide where to go, it is useful to analyze what\n> > > > class implements the operations flow control\n> > > >\n> > > > In one case\n> > > >\n> > > > Base::method() Derived::platformMethod()\n> > > >\n> > > > doSomeWork();\n> > > > ....\n> > > > platformMethod() --> doPlaformWork();\n> > > > ...\n> > > > finalizeWork();\n> > > >\n> > > > While in the other\n> > > >\n> > > > Base::method() Derived::method()\n> > > >\n> > > > doSomeWork();\n> > > > ....\n> > > > method() <-- Base::method()\n> > > > ...\n> > > > finalizeWork();\n> > > >\n> > > > Is this an over-simplification ?\n> > > >\n> > > > Naush, do you expect the derived classes to control the operation\n> > > > flow, or will this be common to both and could be implemented in the\n> > > > base classes ?\n> > >\n> > > No, I see no explicit need for operation flow to be dictated by the derived\n> > > class. Hence changing things around to your suggestion and making things more\n> > > consistent.\n> >\n> > Having looked further into this, I'm a bit more conflicted. I initially thought\n> > that I could simply revert to the original mechanism of having the control flow\n> > start from the derived class and call into the base class, as the 2nd case\n> > drawn above. This does work neatly for the ipa::init() and ipa::configure().\n> > However platformProcessStats() and platformPrepareIsp() require the control flow\n> > from base class to derived class, as in the 1st case drawn above.\n> >\n> > I don't think it's sensible to choose flow 1 for some functions and flow 2 for\n> > others. So that suggests to me that flow 1 (i.e. the existing flow) is probably\n> > the right approach for the IPA changes. Do folks think that makes sense?\n> > Again, I am happy to change things if it seems more appropriate.\n>\n> As mentioned in a different conversation, I'm fine keeping the existing\n> code flow, even if I think it would be best to reverse it. We can\n> improve this incrementally on top.\n\nAgree. I'll keep the existing flow and change if needed in the future.\n\nRegards,\nNaush\n\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 9D06DC0DA4\n\tfor ;\n\tTue, 2 May 2023 15:27:41 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 06F80627DC;\n\tTue, 2 May 2023 17:27:41 +0200 (CEST)","from mail-yw1-x1134.google.com (mail-yw1-x1134.google.com\n\t[IPv6:2607:f8b0:4864:20::1134])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 77F4460538\n\tfor ;\n\tTue, 2 May 2023 17:27:38 +0200 (CEST)","by mail-yw1-x1134.google.com with SMTP id\n\t00721157ae682-55a1462f9f6so27216337b3.3\n\tfor ;\n\tTue, 02 May 2023 08:27:38 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1683041261;\n\tbh=Sq8GAPe8mtlDxW7rbkEILkSc3PEefK7prGV+6D0wQFQ=;\n\th=References:In-Reply-To:Date:To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=jvwQrfTzLKmxlh3XycI36rOTL1rwI3KeJuue8WuvmvF//iZO/FFJs2xtxnBmHiIsY\n\t3zPL8Bfgbp4eqFEA7PM1TAtgQQ7A2JkGkvq5qnGeY464b2lpmhWEBPCh13jaFc5pOl\n\tjg/0O5nxXKp3It6hesdztdGElty7PIJKXHFyRJ/mFri5Ih5NSgUOaUrf/Ozast4XED\n\tRfyJHZCWRGHuAIZrWMRV/y2VlKwJ2yeIT62CwbzPkYHEzGgQswzXHanYtRQ3S6/mKk\n\tEmZ8HWaRGOcH6QSuVtVCv5hX4DrEf6AJr8uSoGg9wWKdWkaIkMxinbWnRsGBJTosdR\n\taGyo3T1mmFYxg==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1683041257; x=1685633257;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc:subject:date:message-id:reply-to;\n\tbh=7i+MWNgr4iKqK5gYgFlQqAkzu1aww5lI9ojmJ+NsuFk=;\n\tb=EGF1OkyUGcns7XUyHAnJIHJckvxJPkwOG/JhhkqItETbvYmpiK+WUscDjoIjzZewm0\n\t1s3LH3T+JKwigSuTNxqOEbqN7j0rxVq/jcyS9H7tvd9UqmInHpmaHXP36o1U3LbwwI6z\n\tI7t8rA5yAQ8ZkU2PGPKcudwrfTk96WlXG81TbL+KyylGs7RHQqzSr9qOfjfH97BFOuvf\n\tcqFFQHq71Bwfu3VdhHbinmskjhGsjSyn0l57l7Hpt45KumAwATRgpRpx9HlGPq8Wi6Bo\n\ta2h4MON38b32ZMMjCBzTa7sYjgzXcsw9Rh6lutlqdiNS3FcEgVge69rXo0KD9I35Tzwv\n\tOD2A=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"EGF1OkyU\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20221208; t=1683041257; x=1685633257;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-message-state:from:to:cc:subject:date:message-id\n\t:reply-to;\n\tbh=7i+MWNgr4iKqK5gYgFlQqAkzu1aww5lI9ojmJ+NsuFk=;\n\tb=Y8H/DSomRVDmE9sGf/AlDtlXNF9jhLlsSmVB75kLimlcMndAxSUjdR1wCfOYpnqn+l\n\tbqFC/bAZMPaemFYpc48Ql3RxIzD4rDzlk5RKyPh2eO5I2C89TaT1uvRnVGDb8p3wPlX7\n\t4FlZumPRVc5Gk1hlT7rIBkh3UwEInBcb3SXWUSDQXJSo0oGjdG7DD0uMUcnpGgDEhuE/\n\tiDQzMu9C/akTNIqHySb/kaoptEIiLglhB2hywqMxHx8LodkzYyJstAd8z/YFdQ0EsPMg\n\tsyZ319GxNIvvZZDdMd0OEfUPzhVoUvMyyTeoVwmyefH50p7OOfFPUVSwCSH9OseeGw1Q\n\taCig==","X-Gm-Message-State":"AC+VfDwO14solP13Mjq3xtogUP0UtPFjibj4DONYHHuosUBYbt37ROHW\n\t0QHKBtVgXS9uFluIJrIsNYGeZun9c9IPa+DZh+4ZHboVemjHdxdSrfx2tQ==","X-Google-Smtp-Source":"ACHHUZ5Pk5Qky9xtiEHFBnIGajF0fkLmNCcM3V1qZWLXEY9F8jFkC3baSfofF0CUGSLM0rmKD3m4OMjOGLEbP85TJck=","X-Received":"by 2002:a25:34cc:0:b0:b99:f0ea:dae5 with SMTP id\n\tb195-20020a2534cc000000b00b99f0eadae5mr13802021yba.64.1683041257191;\n\tTue, 02 May 2023 08:27:37 -0700 (PDT)","MIME-Version":"1.0","References":"<20230426131057.21550-1-naush@raspberrypi.com>\n\t<20230426131057.21550-9-naush@raspberrypi.com>\n\t<20230427155415.GI28489@pendragon.ideasonboard.com>\n\t\n\t\n\t\n\t\n\t<20230502151643.GA22691@pendragon.ideasonboard.com>","In-Reply-To":"<20230502151643.GA22691@pendragon.ideasonboard.com>","Date":"Tue, 2 May 2023 16:27:23 +0100","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH 08/13] ipa: raspberrypi: Introduce\n\tIpaBase class","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Naushir Patuck via libcamera-devel\n\t","Reply-To":"Naushir Patuck ","Cc":"Jacopo Mondi ,\n\tlibcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":27011,"web_url":"https://patchwork.libcamera.org/comment/27011/","msgid":"<20230502153511.GB22691@pendragon.ideasonboard.com>","date":"2023-05-02T15:35:11","subject":"Re: [libcamera-devel] [PATCH 08/13] ipa: raspberrypi: Introduce\n\tIpaBase class","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hello,\n\nOn Fri, Apr 28, 2023 at 11:33:14AM +0200, Jacopo Mondi wrote:\n> On Fri, Apr 28, 2023 at 09:35:51AM +0100, Naushir Patuck via libcamera-devel wrote:\n> > On Thu, 27 Apr 2023 at 16:54, Laurent Pinchart wrote:\n> > > On Wed, Apr 26, 2023 at 02:10:52PM +0100, Naushir Patuck via libcamera-devel wrote:\n> > > > Create a new IpaBase class that handles general purpose housekeeping\n> > > > duties for the Raspberry Pi IPA. Code the implementation of new class is\n> > >\n> > > Did you mean s/Code the implementation/The implementation/ and\n> > > s/of new class/of the new class/ ?\n> >\n> > I actually meant \"Code for the implementation...\", but I'll replace it\n> > with your wording.\n> >\n> > > > essentially pulled from the existing ipa/rpi/vc4/raspberrypi.cpp\n> > > > file with a minimal amount of refactoring.\n> > > >\n> > > > Create a derived IpaVc4 class from IpaBase that handles the VC4 pipeline\n> > > > specific tasks of the IPA. Again, code for this class implementation is\n> > > > taken from the existing ipa/rpi/vc4/raspberrypi.cpp with a\n> > > > minimal amount of refactoring.\n> > > >\n> > > > The goal of this change is to allow third parties to implement their own\n> > > > IPA running on the Raspberry Pi without duplicating all of the IPA\n> > > > housekeeping tasks.\n> > > >\n> > > > Signed-off-by: Naushir Patuck \n> > > > ---\n> > > > src/ipa/meson.build | 5 +-\n> > > > .../raspberrypi.cpp => common/ipa_base.cpp} | 1219 +++++------------\n> > > > src/ipa/rpi/common/ipa_base.h | 125 ++\n> > > > src/ipa/rpi/common/meson.build | 7 +\n> > > > src/ipa/rpi/vc4/meson.build | 8 +-\n> > > > src/ipa/rpi/vc4/vc4.cpp | 540 ++++++++\n> > > > 6 files changed, 1012 insertions(+), 892 deletions(-)\n> > > > rename src/ipa/rpi/{vc4/raspberrypi.cpp => common/ipa_base.cpp} (65%)\n> > > > create mode 100644 src/ipa/rpi/common/ipa_base.h\n> > > > create mode 100644 src/ipa/rpi/common/meson.build\n> > > > create mode 100644 src/ipa/rpi/vc4/vc4.cpp\n> > > >\n> > > > diff --git a/src/ipa/meson.build b/src/ipa/meson.build\n> > > > index 10d3b44ca7b6..0c622c38a4a0 100644\n> > > > --- a/src/ipa/meson.build\n> > > > +++ b/src/ipa/meson.build\n> > > > @@ -37,13 +37,14 @@ endif\n> > > >\n> > > > enabled_ipa_modules = []\n> > > >\n> > > > -# If the Raspberry Pi VC4 IPA is enabled, ensure we include the rpi/cam_helper\n> > > > -# and rpi/controller subdirectories in the build.\n> > > > +# If the Raspberry Pi VC4 IPA is enabled, ensure we include the cam_helper,\n> > > > +# common and controller subdirectories in the build.\n> > > > #\n> > > > # This is done here and not within rpi/vc4/meson.build as meson does not\n> > > > # allow the subdir command to traverse up the directory tree.\n> > > > if pipelines.contains('rpi/vc4')\n> > > > subdir('rpi/cam_helper')\n> > > > + subdir('rpi/common')\n> > > > subdir('rpi/controller')\n> > > > endif\n> > > >\n> > > > diff --git a/src/ipa/rpi/vc4/raspberrypi.cpp b/src/ipa/rpi/common/ipa_base.cpp\n> > > > similarity index 65%\n> > > > rename from src/ipa/rpi/vc4/raspberrypi.cpp\n> > > > rename to src/ipa/rpi/common/ipa_base.cpp\n> > > > index e3ca8e2f7cbe..becada5973ad 100644\n> > > > --- a/src/ipa/rpi/vc4/raspberrypi.cpp\n> > > > +++ b/src/ipa/rpi/common/ipa_base.cpp\n> > > > @@ -1,60 +1,30 @@\n> > >\n> > > [snip]\n> > >\n> > > > @@ -62,8 +32,7 @@ namespace libcamera {\n> > > > using namespace std::literals::chrono_literals;\n> > > > using utils::Duration;\n> > > >\n> > > > -/* Number of metadata objects available in the context list. */\n> > > > -constexpr unsigned int numMetadataContexts = 16;\n> > > > +namespace {\n> > > >\n> > > > /* Number of frame length times to hold in the queue. */\n> > > > constexpr unsigned int FrameLengthsQueueSize = 10;\n> > > > @@ -80,7 +49,7 @@ constexpr Duration defaultMaxFrameDuration = 250.0s;\n> > > > * we rate-limit the controller Prepare() and Process() calls to lower than or\n> > > > * equal to this rate.\n> > > > */\n> > > > -constexpr Duration controllerMinFrameDuration = 1.0s / 30.0;\n> > > > +static constexpr Duration controllerMinFrameDuration = 1.0s / 30.0;\n> > >\n> > > Everything contained in an anonymous namespace becomes static. You can\n> > > drop the static keyword from other variables.\n> > >\n> > > >\n> > > > /* List of controls handled by the Raspberry Pi IPA */\n> > > > static const ControlInfoMap::Map ipaControls{\n> > > > @@ -116,118 +85,23 @@ static const ControlInfoMap::Map ipaAfControls{\n> > > > { &controls::LensPosition, ControlInfo(0.0f, 32.0f, 1.0f) }\n> > > > };\n> > > >\n> > > > +} /* namespace */\n> > > > +\n> > >\n> > > [snip]\n> > >\n> > > > diff --git a/src/ipa/rpi/common/ipa_base.h b/src/ipa/rpi/common/ipa_base.h\n> > > > new file mode 100644\n> > > > index 000000000000..b9fbf9414d63\n> > > > --- /dev/null\n> > > > +++ b/src/ipa/rpi/common/ipa_base.h\n> > > > @@ -0,0 +1,125 @@\n> > > > +/* SPDX-License-Identifier: BSD-2-Clause */\n> > > > +/*\n> > > > + * Copyright (C) 2023, Raspberry Pi Ltd\n> > > > + *\n> > > > + * ipa_base.cpp - Raspberry Pi IPA base class\n> > >\n> > > ipa_base.h\n> > >\n> > > > + */\n> > > > +#pragma once\n> > > > +\n> > > > +#include \n> > > > +#include \n> > > > +#include \n> > >\n> > > You need here, but can drop as it's only used to\n> > > implement the interface of the base class, and so is provided by\n> > > libcamera/ipa/ipa_interface.h. You also need stdint.h.\n> > >\n> > > > +\n> > >\n> > > #include \n> > >\n> > > > +#include \n> > > > +\n> > > > +#include \n> > > > +\n> > > > +#include \n> > > > +#include \n> > >\n> > > Not needed.\n> > >\n> > > > +#include \n> > > > +\n> > > > +#include \"libcamera/internal/mapped_framebuffer.h\"\n> > > > +\n> > > > +#include \"cam_helper/cam_helper.h\"\n> > > > +#include \"controller/agc_status.h\"\n> > > > +#include \"controller/camera_mode.h\"\n> > > > +#include \"controller/controller.h\"\n> > > > +#include \"controller/metadata.h\"\n> > > > +\n> > > > +namespace libcamera {\n> > > > +\n> > > > +namespace ipa::RPi {\n> > > > +\n> > > > +class IpaBase : public IPARPiInterface\n> > > > +{\n> > > > +public:\n> > > > + IpaBase();\n> > > > + virtual ~IpaBase() = 0;\n> > >\n> > > That's a bit weird, why do you need a virtual abstract destructor ?\n> >\n> > This was a mistake, I'll remove it.\n> >\n> > > > +\n> > > > + int32_t init(const IPASettings &settings, const InitParams ¶ms, InitResult *result) override;\n> > > > + int32_t configure(const IPACameraSensorInfo &sensorInfo, const ConfigParams ¶ms,\n> > > > + ConfigResult *result) override;\n> > > > +\n> > > > + void start(const ControlList &controls, StartResult *result) override;\n> > > > + void stop() override {}\n> > > > +\n> > > > + void mapBuffers(const std::vector &buffers) override;\n> > > > + void unmapBuffers(const std::vector &ids) override;\n> > > > +\n> > > > + void prepareIsp(const PrepareParams ¶ms) override;\n> > > > + void processStats(const ProcessParams ¶ms) override;\n> > > > +\n> > > > + void reportMetadata(unsigned int ipaContext, ControlList *controls) override;\n> > > > +\n> > > > +private:\n> > > > + /* Number of metadata objects available in the context list. */\n> > > > + static constexpr unsigned int numMetadataContexts = 16;\n> > > > +\n> > > > + virtual int32_t platformInit(const InitParams ¶ms, InitResult *result) = 0;\n> > >\n> > > This enforces a particular sequence of calls between the base class and\n> > > the derived class, dictated by the base class design. It's best to\n> > > handle this in the derived class instead. You can override the init()\n> > > function in the derived class, call the init() function of the base\n> > > class, and then perform all the work you're currently doing in\n> > > IpaVc4::platformInit().\n> >\n> > One of the earlier iterations had exactly this implementing. After\n> > internal discussions, we decided to do it the other way round (i.e. the base\n> > class implements the init() override). Out of curiosity, what advantages would\n> > one way have over the other?\n> \n> I admit that it was my suggestion to pick this style as originally\n> there was a mix of the two. Seeing an override calling into the base\n> class' overridden method felt a bit weird.\n\nThat's a normal pattern in C++, base classes shouldn't, when possible,\nimply a particular implementation of derived classes. Calling the base\nclass implementation at the beginning or end of a derived class'\nfunction is perfectly fine and normal.\n\nThe other pattern, calling into a protected virtual function implemented\nby the derived class, is an acceptable use case too.\n\n> I admit I've researched\n> (briefly) what was the suggested way to handle methods composition in\n\nNote that composition and inheritance are two different things.\n\n> C++ and found nothing relevant, so my thought was that it was better\n> to clearly define what was the interface that derived classes need to\n> implement, as the PipelineHandler/IPAInterface is implemented in the\n> base classes.\n> \n> When it comes to the call order, it's the base class driving the\n> procesing flow, while dispatching calls to the derived classes at the\n> right time. Moving to your suggestion would allow derived classes to\n> call the base class method, which means most of the control is moved\n> to the derived classes with possible code duplication.\n\nI think having the derived class in control is desirable, as only the\nderived class knows about its specific needs. Otherwise you could have\nto over-complicate the design of the base class to cover the needs of\ndifferent derived classes.\n\n> I think to actually decide where to go, it is useful to analyze what\n> class implements the operations flow control\n> \n> In one case\n> \n> Base::method() Derived::platformMethod()\n> \n> doSomeWork();\n> ....\n> platformMethod() --> doPlaformWork();\n> ...\n> finalizeWork();\n> \n> While in the other\n> \n> Base::method() Derived::method()\n> \n> doSomeWork();\n> ....\n> method() <-- Base::method()\n> ...\n> finalizeWork();\n> \n> Is this an over-simplification ?\n> \n> Naush, do you expect the derived classes to control the operation\n> flow, or will this be common to both and could be implemented in the\n> base classes ?\n> \n> > > The same comment applies to the other platform*() functions (some may\n> > > require a little bit of refactoring). The handleControls() function can\n> > > also be dropped by making applyControls() a protected virtual function\n> > > that derived classes may override.\n> > >\n> > > > + virtual int32_t platformConfigure(const ConfigParams ¶ms, ConfigResult *result) = 0;\n> > > > +\n> > > > + virtual void platformPrepareIsp(const PrepareParams ¶ms,\n> > > > + RPiController::Metadata &rpiMetadata) = 0;\n> > > > + virtual RPiController::StatisticsPtr platformProcessStats(Span mem) = 0;\n> > > > +\n> > > > + void setMode(const IPACameraSensorInfo &sensorInfo);\n> > > > + void setCameraTimeoutValue();\n> > > > + bool validateSensorControls();\n> > > > + bool validateLensControls();\n> > > > + void applyControls(const ControlList &controls);\n> > > > + virtual void handleControls(const ControlList &controls) = 0;\n> > > > + void fillDeviceStatus(const ControlList &sensorControls, unsigned int ipaContext);\n> > > > + void applyFrameDurations(utils::Duration minFrameDuration, utils::Duration maxFrameDuration);\n> > > > + void applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls);\n> > > > +\n> > > > + std::map buffers_;\n> > > > +\n> > > > + bool lensPresent_;\n> > > > + ControlList libcameraMetadata_;\n> > > > +\n> > > > + std::array rpiMetadata_;\n> > > > +\n> > > > + /*\n> > > > + * We count frames to decide if the frame must be hidden (e.g. from\n> > > > + * display) or mistrusted (i.e. not given to the control algos).\n> > > > + */\n> > > > + uint64_t frameCount_;\n> > > > +\n> > > > + /* How many frames we should avoid running control algos on. */\n> > > > + unsigned int mistrustCount_;\n> > > > +\n> > > > + /* Number of frames that need to be dropped on startup. */\n> > > > + unsigned int dropFrameCount_;\n> > > > +\n> > > > + /* Frame timestamp for the last run of the controller. */\n> > > > + uint64_t lastRunTimestamp_;\n> > > > +\n> > > > + /* Do we run a Controller::process() for this frame? */\n> > > > + bool processPending_;\n> > > > +\n> > > > + /* Distinguish the first camera start from others. */\n> > > > + bool firstStart_;\n> > > > +\n> > > > + /* Frame duration (1/fps) limits. */\n> > > > + utils::Duration minFrameDuration_;\n> > > > + utils::Duration maxFrameDuration_;\n> > > > +\n> > > > +protected:\n> > >\n> > > We usually put protected members before private members.\n> > >\n> > > > + /* Raspberry Pi controller specific defines. */\n> > > > + std::unique_ptr helper_;\n> > > > + RPiController::Controller controller_;\n> > > > +\n> > > > + ControlInfoMap sensorCtrls_;\n> > > > + ControlInfoMap lensCtrls_;\n> > > > +\n> > > > + /* Camera sensor params. */\n> > > > + CameraMode mode_;\n> > > > +\n> > > > + /* Track the frame length times over FrameLengthsQueueSize frames. */\n> > > > + std::deque frameLengths_;\n> > > > + utils::Duration lastTimeout_;\n> > > > +};\n> > > > +\n> > > > +} /* namespace ipa::RPi */\n> > > > +\n> > > > +} /* namespace libcamera */\n> > > > diff --git a/src/ipa/rpi/common/meson.build b/src/ipa/rpi/common/meson.build\n> > > > new file mode 100644\n> > > > index 000000000000..a7189f8389af\n> > > > --- /dev/null\n> > > > +++ b/src/ipa/rpi/common/meson.build\n> > > > @@ -0,0 +1,7 @@\n> > > > +# SPDX-License-Identifier: CC0-1.0\n> > > > +\n> > > > +# SPDX-License-Identifier: CC0-1.0\n> > >\n> > > One SPDX header is enough :-)\n> > >\n> > > > +\n> > > > +rpi_ipa_common_sources = files([\n> > > > + 'ipa_base.cpp',\n> > > > +])\n> > > > diff --git a/src/ipa/rpi/vc4/meson.build b/src/ipa/rpi/vc4/meson.build\n> > > > index 992d0f1ab5a7..b581391586b3 100644\n> > > > --- a/src/ipa/rpi/vc4/meson.build\n> > > > +++ b/src/ipa/rpi/vc4/meson.build\n> > > > @@ -13,11 +13,15 @@ vc4_ipa_includes = [\n> > > > ]\n> > > >\n> > > > vc4_ipa_sources = files([\n> > > > - 'raspberrypi.cpp',\n> > > > + 'vc4.cpp',\n> > > > ])\n> > > >\n> > > > vc4_ipa_includes += include_directories('..')\n> > > > -vc4_ipa_sources += [rpi_ipa_cam_helper_sources, rpi_ipa_controller_sources]\n> > > > +vc4_ipa_sources += [\n> > > > + rpi_ipa_cam_helper_sources,\n> > > > + rpi_ipa_common_sources,\n> > > > + rpi_ipa_controller_sources,\n> > >\n> > > Could we avoid compiling all these source files multiple times, once for\n> > > the IpaVc4 IPA module, and once for the other IPA modules that may be\n> > > implemented in the future ? This can be done with a static library, see\n> > > src/apps/common/meson.build and src/apps/cam/meson.build for instance.\n> > >\n> > > > +]\n> > > >\n> > > > mod = shared_module(ipa_name,\n> > > > [vc4_ipa_sources, libcamera_generated_ipa_headers],\n> > >\n> >\n> > Sure, I'll fix this up.\n> >\n> > Naush\n> >\n> > > [snip]","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 9E4ADBD16B\n\tfor ;\n\tTue, 2 May 2023 15:35:03 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 02608627DC;\n\tTue, 2 May 2023 17:35:03 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A719B60538\n\tfor ;\n\tTue, 2 May 2023 17:35:01 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(133-32-181-51.west.xps.vectant.ne.jp [133.32.181.51])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id A1FA8800;\n\tTue, 2 May 2023 17:34:58 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1683041703;\n\tbh=slYdKqw5gujfhbE8Qlxcjloi5WqaAwTL6VtYCu6ez8k=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=efN1zzRcmYDaNsocA15nOs6Pd4UyJ3Lz83fKYDAith+4GF4fzuEgoghXZLa6Sstmh\n\tsGr5H03C/HKyDH4UZ3ViE+8K3KxHGH64D/HHE0wCJzBI2g/tOFm3QbEZgVqCovHK3m\n\tuoH0FRO6Q6MGEp8htTfp5hJlmdEMTPNEGr89uiYUZYUe071vE4xy2Od7BfI2Ze1cc6\n\tzk3ocbARfwzm9l+TsFNeoLJnroA0JGBUFOI6/WqH5MVlsBlbdrDgioLd0SHAwn6DlN\n\t2wU3dsUsILQpGEFNHMCf6tpDlVRCq/aDa04AIcEbPmaVQ9nn3bscqwX47DbOW3Qr/1\n\tELfnOvx2Yx3MQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1683041699;\n\tbh=slYdKqw5gujfhbE8Qlxcjloi5WqaAwTL6VtYCu6ez8k=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=DtSGnq+eCwmCV+VjVqIexqMTCbmxDscq3vhY0L98O9l68Kw/fdFJ41qTILpEOpB2b\n\t+qwe+qx1NtSHuGlv5ECL+XUz3nSdL1TZ+QitY55S0ytIPNXCEzC68j2DOBBgbfg9Mp\n\tBbLVppV6dXJa/w++l6pefBdCqENClt+cFYwEKSFU="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"DtSGnq+e\"; dkim-atps=neutral","Date":"Tue, 2 May 2023 18:35:11 +0300","To":"Jacopo Mondi ","Message-ID":"<20230502153511.GB22691@pendragon.ideasonboard.com>","References":"<20230426131057.21550-1-naush@raspberrypi.com>\n\t<20230426131057.21550-9-naush@raspberrypi.com>\n\t<20230427155415.GI28489@pendragon.ideasonboard.com>\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 08/13] ipa: raspberrypi: Introduce\n\tIpaBase class","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]