[{"id":26957,"web_url":"https://patchwork.libcamera.org/comment/26957/","msgid":"<26csbh5y5qh7r4rle34eyn3drneqvvifcqb65s5nppg5ifm6kv@ft5oy4xnkzu4>","date":"2023-04-27T08:55:57","subject":"Re: [libcamera-devel] [PATCH 06/13] pipeline: ipa: raspberrypi:\n\tRestructure the IPA mojom interface","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:50PM +0100, Naushir Patuck via libcamera-devel wrote:\n> Restructure the IPA mojom interface to be more consistent in the use\n> of the API. Function parameters are now grouped into *Params structures\n> and results are now returned in *Results structures.\n>\n> The following pipeline -> IPA interfaces have been removed:\n>\n> signalQueueRequest(libcamera.ControlList controls);\n> signalIspPrepare(ISPConfig data);\n> signalStatReady(uint32 bufferId, uint32 ipaContext);\n>\n> and replaced with:\n>\n> prepareIsp(PrepareParams params);\n> processStats(ProcessParams params);\n>\n> signalQueueRequest() is now encompassed within signalPrepareIsp().\n>\n> The following IPA -> pipeline interfaces have been removed:\n>\n> runIsp(uint32 bufferId);\n> embeddedComplete(uint32 bufferId);\n> statsMetadataComplete(uint32 bufferId, libcamera.ControlList controls);\n>\n> and replaced with the following async calls:\n>\n> prepareIspComplete(BufferIds buffers);\n> processStatsComplete(BufferIds buffers);\n> metadataReady(libcamera.ControlList metadata);\n>\n> Signed-off-by: Naushir Patuck \n> ---\n> include/libcamera/ipa/raspberrypi.mojom | 127 +++++---------\n> src/ipa/rpi/vc4/raspberrypi.cpp | 102 +++++-------\n> .../pipeline/rpi/vc4/raspberrypi.cpp | 155 +++++++++---------\n> 3 files changed, 165 insertions(+), 219 deletions(-)\n>\n> diff --git a/include/libcamera/ipa/raspberrypi.mojom b/include/libcamera/ipa/raspberrypi.mojom\n> index 80e0126618c8..7d56248f4ab3 100644\n> --- a/include/libcamera/ipa/raspberrypi.mojom\n> +++ b/include/libcamera/ipa/raspberrypi.mojom\n> @@ -8,7 +8,7 @@ module ipa.RPi;\n>\n> import \"include/libcamera/ipa/core.mojom\";\n>\n> -/* Size of the LS grid allocation. */\n> +/* Size of the LS grid allocation on VC4. */\n> const uint32 MaxLsGridSize = 0x8000;\n>\n> struct SensorConfig {\n> @@ -19,117 +19,78 @@ struct SensorConfig {\n> \tuint32 sensorMetadata;\n> };\n>\n> -struct IPAInitResult {\n> +struct InitParams {\n> +\tbool lensPresent;\n> +};\n> +\n> +struct InitResult {\n> \tSensorConfig sensorConfig;\n> \tlibcamera.ControlInfoMap controlInfo;\n> };\n>\n> -struct ISPConfig {\n> -\tuint32 embeddedBufferId;\n> -\tuint32 bayerBufferId;\n> -\tbool embeddedBufferPresent;\n> -\tlibcamera.ControlList controls;\n> -\tuint32 ipaContext;\n> -\tuint32 delayContext;\n> +struct BufferIds {\n> +\tuint32 bayer;\n> +\tuint32 embedded;\n> +\tuint32 stats;\n> };\n>\n> -struct IPAConfig {\n> +struct ConfigParams {\n> \tuint32 transform;\n> -\tlibcamera.SharedFD lsTableHandle;\n> \tlibcamera.ControlInfoMap sensorControls;\n> \tlibcamera.ControlInfoMap ispControls;\n> \tlibcamera.ControlInfoMap lensControls;\n> + /* VC4 specifc */\n> +\tlibcamera.SharedFD lsTableHandle;\n> };\n>\n> -struct IPAConfigResult {\n> - float modeSensitivity;\n> - libcamera.ControlInfoMap controlInfo;\n> +struct ConfigResult {\n> +\tfloat modeSensitivity;\n> +\tlibcamera.ControlInfoMap controlInfo;\n> +\tlibcamera.ControlList controls;\n> };\n>\n> -struct StartConfig {\n> +struct StartResult {\n> \tlibcamera.ControlList controls;\n> \tint32 dropFrameCount;\n> };\n>\n> +struct PrepareParams {\n> +\tBufferIds buffers;\n> +\tlibcamera.ControlList sensorControls;\n> +\tlibcamera.ControlList requestControls;\n> +\tuint32 ipaContext;\n> +\tuint32 delayContext;\n> +};\n> +\n> +struct ProcessParams {\n> +\tBufferIds buffers;\n> +\tuint32 ipaContext;\n> +};\n> +\n> interface IPARPiInterface {\n> -\tinit(libcamera.IPASettings settings, bool lensPresent)\n> -\t\t=> (int32 ret, IPAInitResult result);\n> -\tstart(libcamera.ControlList controls) => (StartConfig startConfig);\n> +\tinit(libcamera.IPASettings settings, InitParams params)\n> +\t\t=> (int32 ret, InitResult result);\n> +\n> +\tstart(libcamera.ControlList controls) => (StartResult result);\n> \tstop();\n>\n> -\t/**\n> -\t * \\fn configure()\n\nIs it intentional to drop documentation ? Was it used at all ?\n\n> -\t * \\brief Configure the IPA stream and sensor settings\n> -\t * \\param[in] sensorInfo Camera sensor information\n> -\t * \\param[in] ipaConfig Pipeline-handler-specific configuration data\n> -\t * \\param[out] controls Controls to apply by the pipeline entity\n> -\t * \\param[out] result Other results that the pipeline handler may require\n> -\t *\n> -\t * This function shall be called when the camera is configured to inform\n> -\t * the IPA of the camera's streams and the sensor settings.\n> -\t *\n> -\t * The \\a sensorInfo conveys information about the camera sensor settings that\n> -\t * the pipeline handler has selected for the configuration.\n> -\t *\n> -\t * The \\a ipaConfig and \\a controls parameters carry data passed by the\n> -\t * pipeline handler to the IPA and back.\n> -\t */\n> -\tconfigure(libcamera.IPACameraSensorInfo sensorInfo,\n> -\t\t IPAConfig ipaConfig)\n> -\t\t=> (int32 ret, libcamera.ControlList controls, IPAConfigResult result);\n> -\n> -\t/**\n> -\t * \\fn mapBuffers()\n> -\t * \\brief Map buffers shared between the pipeline handler and the IPA\n> -\t * \\param[in] buffers List of buffers to map\n> -\t *\n> -\t * This function informs the IPA module of memory buffers set up by the\n> -\t * pipeline handler that the IPA needs to access. It provides dmabuf\n> -\t * file handles for each buffer, and associates the buffers with unique\n> -\t * numerical IDs.\n> -\t *\n> -\t * IPAs shall map the dmabuf file handles to their address space and\n> -\t * keep a cache of the mappings, indexed by the buffer numerical IDs.\n> -\t * The IDs are used in all other IPA interface functions to refer to\n> -\t * buffers, including the unmapBuffers() function.\n> -\t *\n> -\t * All buffers that the pipeline handler wishes to share with an IPA\n> -\t * shall be mapped with this function. Buffers may be mapped all at once\n> -\t * with a single call, or mapped and unmapped dynamically at runtime,\n> -\t * depending on the IPA protocol. Regardless of the protocol, all\n> -\t * buffers mapped at a given time shall have unique numerical IDs.\n> -\t *\n> -\t * The numerical IDs have no meaning defined by the IPA interface, and\n> -\t * should be treated as opaque handles by IPAs, with the only exception\n> -\t * that ID zero is invalid.\n> -\t *\n> -\t * \\sa unmapBuffers()\n> -\t */\n> -\tmapBuffers(array buffers);\n> +\tconfigure(libcamera.IPACameraSensorInfo sensorInfo, ConfigParams params)\n> +\t\t=> (int32 ret, ConfigResult result);\n>\n> -\t/**\n> -\t * \\fn unmapBuffers()\n> -\t * \\brief Unmap buffers shared by the pipeline to the IPA\n> -\t * \\param[in] ids List of buffer IDs to unmap\n> -\t *\n> -\t * This function removes mappings set up with mapBuffers(). Numerical\n> -\t * IDs of unmapped buffers may be reused when mapping new buffers.\n> -\t *\n> -\t * \\sa mapBuffers()\n> -\t */\n> +\tmapBuffers(array buffers);\n> \tunmapBuffers(array ids);\n>\n> -\t[async] signalStatReady(uint32 bufferId, uint32 ipaContext);\n> -\t[async] signalQueueRequest(libcamera.ControlList controls);\n> -\t[async] signalIspPrepare(ISPConfig data);\n> +\t[async] prepareIsp(PrepareParams params);\n> +\t[async] processStats(ProcessParams params);\n> };\n>\n> interface IPARPiEventInterface {\n> -\tstatsMetadataComplete(uint32 bufferId, libcamera.ControlList controls);\n> -\trunIsp(uint32 bufferId);\n> -\tembeddedComplete(uint32 bufferId);\n> +\tprepareIspComplete(BufferIds buffers);\n> +\tprocessStatsComplete(BufferIds buffers);\n> +\tmetadataReady(libcamera.ControlList metadata);\n> \tsetIspControls(libcamera.ControlList controls);\n> \tsetDelayedControls(libcamera.ControlList controls, uint32 delayContext);\n> \tsetLensControls(libcamera.ControlList controls);\n> \tsetCameraTimeout(uint32 maxFrameLengthMs);\n> };\n> +\n\nAdditional blank line\n\nThe rest looks good\nReviewed-by: Jacopo Mondi \n\nThanks\n j\n\n> diff --git a/src/ipa/rpi/vc4/raspberrypi.cpp b/src/ipa/rpi/vc4/raspberrypi.cpp\n> index 841635ccde2b..d737f1d662a0 100644\n> --- a/src/ipa/rpi/vc4/raspberrypi.cpp\n> +++ b/src/ipa/rpi/vc4/raspberrypi.cpp\n> @@ -136,30 +136,28 @@ public:\n> \t\t\tmunmap(lsTable_, MaxLsGridSize);\n> \t}\n>\n> -\tint init(const IPASettings &settings, bool lensPresent, IPAInitResult *result) override;\n> -\tvoid start(const ControlList &controls, StartConfig *startConfig) override;\n> +\tint init(const IPASettings &settings, const InitParams ¶ms, InitResult *result) override;\n> +\tvoid start(const ControlList &controls, StartResult *result) override;\n> \tvoid stop() override {}\n>\n> -\tint configure(const IPACameraSensorInfo &sensorInfo, const IPAConfig &data,\n> -\t\t ControlList *controls, IPAConfigResult *result) override;\n> +\tint configure(const IPACameraSensorInfo &sensorInfo, const ConfigParams ¶ms,\n> +\t\t ConfigResult *result) override;\n> \tvoid mapBuffers(const std::vector &buffers) override;\n> \tvoid unmapBuffers(const std::vector &ids) override;\n> -\tvoid signalStatReady(const uint32_t bufferId, uint32_t ipaContext) override;\n> -\tvoid signalQueueRequest(const ControlList &controls) override;\n> -\tvoid signalIspPrepare(const ISPConfig &data) override;\n> +\tvoid prepareIsp(const PrepareParams ¶ms) override;\n> +\tvoid processStats(const ProcessParams ¶ms) override;\n>\n> private:\n> \tvoid setMode(const IPACameraSensorInfo &sensorInfo);\n> \tbool validateSensorControls();\n> \tbool validateIspControls();\n> \tbool validateLensControls();\n> -\tvoid queueRequest(const ControlList &controls);\n> -\tvoid returnEmbeddedBuffer(unsigned int bufferId);\n> -\tvoid prepareISP(const ISPConfig &data);\n> +\tvoid applyControls(const ControlList &controls);\n> +\tvoid prepare(const PrepareParams ¶ms);\n> \tvoid reportMetadata(unsigned int ipaContext);\n> \tvoid fillDeviceStatus(const ControlList &sensorControls, unsigned int ipaContext);\n> \tRPiController::StatisticsPtr fillStatistics(bcm2835_isp_stats *stats) const;\n> -\tvoid processStats(unsigned int bufferId, unsigned int ipaContext);\n> +\tvoid process(unsigned int bufferId, unsigned int ipaContext);\n> \tvoid setCameraTimeoutValue();\n> \tvoid applyFrameDurations(Duration minFrameDuration, Duration maxFrameDuration);\n> \tvoid applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls);\n> @@ -229,7 +227,7 @@ private:\n> \tDuration lastTimeout_;\n> };\n>\n> -int IPARPi::init(const IPASettings &settings, bool lensPresent, IPAInitResult *result)\n> +int IPARPi::init(const IPASettings &settings, const InitParams ¶ms, InitResult *result)\n> {\n> \t/*\n> \t * Load the \"helper\" for this sensor. This tells us all the device specific stuff\n> @@ -274,7 +272,7 @@ int IPARPi::init(const IPASettings &settings, bool lensPresent, IPAInitResult *r\n> \t\treturn -EINVAL;\n> \t}\n>\n> -\tlensPresent_ = lensPresent;\n> +\tlensPresent_ = params.lensPresent;\n>\n> \tcontroller_.initialise();\n>\n> @@ -287,14 +285,13 @@ int IPARPi::init(const IPASettings &settings, bool lensPresent, IPAInitResult *r\n> \treturn 0;\n> }\n>\n> -void IPARPi::start(const ControlList &controls, StartConfig *startConfig)\n> +void IPARPi::start(const ControlList &controls, StartResult *result)\n> {\n> \tRPiController::Metadata metadata;\n>\n> -\tASSERT(startConfig);\n> \tif (!controls.empty()) {\n> \t\t/* We have been given some controls to action before start. */\n> -\t\tqueueRequest(controls);\n> +\t\tapplyControls(controls);\n> \t}\n>\n> \tcontroller_.switchMode(mode_, &metadata);\n> @@ -313,7 +310,7 @@ void IPARPi::start(const ControlList &controls, StartConfig *startConfig)\n> \tif (agcStatus.shutterTime && agcStatus.analogueGain) {\n> \t\tControlList ctrls(sensorCtrls_);\n> \t\tapplyAGC(&agcStatus, ctrls);\n> -\t\tstartConfig->controls = std::move(ctrls);\n> +\t\tresult->controls = std::move(ctrls);\n> \t\tsetCameraTimeoutValue();\n> \t}\n>\n> @@ -360,7 +357,7 @@ void IPARPi::start(const ControlList &controls, StartConfig *startConfig)\n> \t\tmistrustCount_ = helper_->mistrustFramesModeSwitch();\n> \t}\n>\n> -\tstartConfig->dropFrameCount = dropFrameCount_;\n> +\tresult->dropFrameCount = dropFrameCount_;\n>\n> \tfirstStart_ = false;\n> \tlastRunTimestamp_ = 0;\n> @@ -435,11 +432,11 @@ void IPARPi::setMode(const IPACameraSensorInfo &sensorInfo)\n> \t\t\t mode_.minFrameDuration, mode_.maxFrameDuration);\n> }\n>\n> -int IPARPi::configure(const IPACameraSensorInfo &sensorInfo, const IPAConfig &ipaConfig,\n> -\t\t ControlList *controls, IPAConfigResult *result)\n> +int IPARPi::configure(const IPACameraSensorInfo &sensorInfo, const ConfigParams ¶ms,\n> +\t\t ConfigResult *result)\n> {\n> -\tsensorCtrls_ = ipaConfig.sensorControls;\n> -\tispCtrls_ = ipaConfig.ispControls;\n> +\tsensorCtrls_ = params.sensorControls;\n> +\tispCtrls_ = params.ispControls;\n>\n> \tif (!validateSensorControls()) {\n> \t\tLOG(IPARPI, Error) << \"Sensor control validation failed.\";\n> @@ -452,7 +449,7 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo, const IPAConfig &ip\n> \t}\n>\n> \tif (lensPresent_) {\n> -\t\tlensCtrls_ = ipaConfig.lensControls;\n> +\t\tlensCtrls_ = params.lensControls;\n> \t\tif (!validateLensControls()) {\n> \t\t\tLOG(IPARPI, Warning) << \"Lens validation failed, \"\n> \t\t\t\t\t << \"no lens control will be available.\";\n> @@ -466,10 +463,10 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo, const IPAConfig &ip\n> \t/* Re-assemble camera mode using the sensor info. */\n> \tsetMode(sensorInfo);\n>\n> -\tmode_.transform = static_cast(ipaConfig.transform);\n> +\tmode_.transform = static_cast(params.transform);\n>\n> \t/* Store the lens shading table pointer and handle if available. */\n> -\tif (ipaConfig.lsTableHandle.isValid()) {\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> @@ -477,7 +474,7 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo, const IPAConfig &ip\n> \t\t}\n>\n> \t\t/* Map the LS table buffer into user space. */\n> -\t\tlsTableHandle_ = std::move(ipaConfig.lsTableHandle);\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> @@ -512,8 +509,7 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo, const IPAConfig &ip\n> \t\tapplyAGC(&agcStatus, ctrls);\n> \t}\n>\n> -\tASSERT(controls);\n> -\t*controls = std::move(ctrls);\n> +\tresult->controls = std::move(ctrls);\n>\n> \t/*\n> \t * Apply the correct limits to the exposure, gain and frame duration controls\n> @@ -560,37 +556,34 @@ void IPARPi::unmapBuffers(const std::vector &ids)\n> \t}\n> }\n>\n> -void IPARPi::signalStatReady(uint32_t bufferId, uint32_t ipaContext)\n> +void IPARPi::processStats(const ProcessParams ¶ms)\n> {\n> -\tunsigned int context = ipaContext % rpiMetadata_.size();\n> +\tunsigned int context = params.ipaContext % rpiMetadata_.size();\n>\n> \tif (++checkCount_ != frameCount_) /* assert here? */\n> \t\tLOG(IPARPI, Error) << \"WARNING: Prepare/Process mismatch!!!\";\n> \tif (processPending_ && frameCount_ > mistrustCount_)\n> -\t\tprocessStats(bufferId, context);\n> +\t\tprocess(params.buffers.stats, context);\n>\n> \treportMetadata(context);\n> -\n> -\tstatsMetadataComplete.emit(bufferId, libcameraMetadata_);\n> +\tprocessStatsComplete.emit(params.buffers);\n> }\n>\n> -void IPARPi::signalQueueRequest(const ControlList &controls)\n> -{\n> -\tqueueRequest(controls);\n> -}\n>\n> -void IPARPi::signalIspPrepare(const ISPConfig &data)\n> +void IPARPi::prepareIsp(const PrepareParams ¶ms)\n> {\n> +\tapplyControls(params.requestControls);\n> +\n> \t/*\n> \t * At start-up, or after a mode-switch, we may want to\n> \t * avoid running the control algos for a few frames in case\n> \t * they are \"unreliable\".\n> \t */\n> -\tprepareISP(data);\n> +\tprepare(params);\n> \tframeCount_++;\n>\n> \t/* Ready to push the input buffer into the ISP. */\n> -\trunIsp.emit(data.bayerBufferId);\n> +\tprepareIspComplete.emit(params.buffers);\n> }\n>\n> void IPARPi::reportMetadata(unsigned int ipaContext)\n> @@ -703,6 +696,8 @@ void IPARPi::reportMetadata(unsigned int ipaContext)\n> \t\tlibcameraMetadata_.set(controls::AfState, s);\n> \t\tlibcameraMetadata_.set(controls::AfPauseState, p);\n> \t}\n> +\n> +\tmetadataReady.emit(libcameraMetadata_);\n> }\n>\n> bool IPARPi::validateSensorControls()\n> @@ -826,7 +821,7 @@ static const std::map AfPauseTable\n> \t{ controls::AfPauseResume, RPiController::AfAlgorithm::AfPauseResume },\n> };\n>\n> -void IPARPi::queueRequest(const ControlList &controls)\n> +void IPARPi::applyControls(const ControlList &controls)\n> {\n> \tusing RPiController::AfAlgorithm;\n>\n> @@ -1256,27 +1251,22 @@ void IPARPi::queueRequest(const ControlList &controls)\n> \t}\n> }\n>\n> -void IPARPi::returnEmbeddedBuffer(unsigned int bufferId)\n> +void IPARPi::prepare(const PrepareParams ¶ms)\n> {\n> -\tembeddedComplete.emit(bufferId);\n> -}\n> -\n> -void IPARPi::prepareISP(const ISPConfig &data)\n> -{\n> -\tint64_t frameTimestamp = data.controls.get(controls::SensorTimestamp).value_or(0);\n> -\tunsigned int ipaContext = data.ipaContext % rpiMetadata_.size();\n> +\tint64_t frameTimestamp = params.sensorControls.get(controls::SensorTimestamp).value_or(0);\n> +\tunsigned int ipaContext = params.ipaContext % rpiMetadata_.size();\n> \tRPiController::Metadata &rpiMetadata = rpiMetadata_[ipaContext];\n> \tSpan embeddedBuffer;\n>\n> \trpiMetadata.clear();\n> -\tfillDeviceStatus(data.controls, ipaContext);\n> +\tfillDeviceStatus(params.sensorControls, ipaContext);\n>\n> -\tif (data.embeddedBufferPresent) {\n> +\tif (params.buffers.embedded) {\n> \t\t/*\n> \t\t * Pipeline handler has supplied us with an embedded data buffer,\n> \t\t * we must pass it to the CamHelper for parsing.\n> \t\t */\n> -\t\tauto it = buffers_.find(data.embeddedBufferId);\n> +\t\tauto it = buffers_.find(params.buffers.embedded);\n> \t\tASSERT(it != buffers_.end());\n> \t\tembeddedBuffer = it->second.planes()[0];\n> \t}\n> @@ -1288,7 +1278,7 @@ void IPARPi::prepareISP(const ISPConfig &data)\n> \t * metadata.\n> \t */\n> \tAgcStatus agcStatus;\n> -\tRPiController::Metadata &delayedMetadata = rpiMetadata_[data.delayContext];\n> +\tRPiController::Metadata &delayedMetadata = rpiMetadata_[params.delayContext];\n> \tif (!delayedMetadata.get(\"agc.status\", agcStatus))\n> \t\trpiMetadata.set(\"agc.delayed_status\", agcStatus);\n>\n> @@ -1298,10 +1288,6 @@ void IPARPi::prepareISP(const ISPConfig &data)\n> \t */\n> \thelper_->prepare(embeddedBuffer, rpiMetadata);\n>\n> -\t/* Done with embedded data now, return to pipeline handler asap. */\n> -\tif (data.embeddedBufferPresent)\n> -\t\treturnEmbeddedBuffer(data.embeddedBufferId);\n> -\n> \t/* Allow a 10% margin on the comparison below. */\n> \tDuration delta = (frameTimestamp - lastRunTimestamp_) * 1.0ns;\n> \tif (lastRunTimestamp_ && frameCount_ > dropFrameCount_ &&\n> @@ -1445,7 +1431,7 @@ RPiController::StatisticsPtr IPARPi::fillStatistics(bcm2835_isp_stats *stats) co\n> \treturn statistics;\n> }\n>\n> -void IPARPi::processStats(unsigned int bufferId, unsigned int ipaContext)\n> +void IPARPi::process(unsigned int bufferId, unsigned int ipaContext)\n> {\n> \tRPiController::Metadata &rpiMetadata = rpiMetadata_[ipaContext];\n>\n> diff --git a/src/libcamera/pipeline/rpi/vc4/raspberrypi.cpp b/src/libcamera/pipeline/rpi/vc4/raspberrypi.cpp\n> index e54bf1ef2c17..30ce6feab0d1 100644\n> --- a/src/libcamera/pipeline/rpi/vc4/raspberrypi.cpp\n> +++ b/src/libcamera/pipeline/rpi/vc4/raspberrypi.cpp\n> @@ -200,15 +200,15 @@ public:\n> \tvoid freeBuffers();\n> \tvoid frameStarted(uint32_t sequence);\n>\n> -\tint loadIPA(ipa::RPi::IPAInitResult *result);\n> -\tint configureIPA(const CameraConfiguration *config, ipa::RPi::IPAConfigResult *result);\n> +\tint loadIPA(ipa::RPi::InitResult *result);\n> +\tint configureIPA(const CameraConfiguration *config, ipa::RPi::ConfigResult *result);\n> \tint loadPipelineConfiguration();\n>\n> \tvoid enumerateVideoDevices(MediaLink *link);\n>\n> -\tvoid statsMetadataComplete(uint32_t bufferId, const ControlList &controls);\n> -\tvoid runIsp(uint32_t bufferId);\n> -\tvoid embeddedComplete(uint32_t bufferId);\n> +\tvoid processStatsComplete(const ipa::RPi::BufferIds &buffers);\n> +\tvoid metadataReady(const ControlList &metadata);\n> +\tvoid prepareIspComplete(const ipa::RPi::BufferIds &buffers);\n> \tvoid setIspControls(const ControlList &controls);\n> \tvoid setDelayedControls(const ControlList &controls, uint32_t delayContext);\n> \tvoid setLensControls(const ControlList &controls);\n> @@ -238,7 +238,7 @@ public:\n> \t/* The vector below is just for convenience when iterating over all streams. */\n> \tstd::vector streams_;\n> \t/* Stores the ids of the buffers mapped in the IPA. */\n> -\tstd::unordered_set ipaBuffers_;\n> +\tstd::unordered_set bufferIds_;\n> \t/*\n> \t * Stores a cascade of Video Mux or Bridge devices between the sensor and\n> \t * Unicam together with media link across the entities.\n> @@ -1000,7 +1000,7 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)\n>\n> \tdata->isp_[Isp::Input].dev()->setSelection(V4L2_SEL_TGT_CROP, &crop);\n>\n> -\tipa::RPi::IPAConfigResult result;\n> +\tipa::RPi::ConfigResult result;\n> \tret = data->configureIPA(config, &result);\n> \tif (ret)\n> \t\tLOG(RPI, Error) << \"Failed to configure the IPA: \" << ret;\n> @@ -1117,17 +1117,17 @@ int PipelineHandlerRPi::start(Camera *camera, const ControlList *controls)\n> \t\tdata->applyScalerCrop(*controls);\n>\n> \t/* Start the IPA. */\n> -\tipa::RPi::StartConfig startConfig;\n> +\tipa::RPi::StartResult result;\n> \tdata->ipa_->start(controls ? *controls : ControlList{ controls::controls },\n> -\t\t\t &startConfig);\n> +\t\t\t &result);\n>\n> \t/* Apply any gain/exposure settings that the IPA may have passed back. */\n> -\tif (!startConfig.controls.empty())\n> -\t\tdata->setSensorControls(startConfig.controls);\n> +\tif (!result.controls.empty())\n> +\t\tdata->setSensorControls(result.controls);\n>\n> \t/* Configure the number of dropped frames required on startup. */\n> \tdata->dropFrameCount_ = data->config_.disableStartupFrameDrops\n> -\t\t\t ? 0 : startConfig.dropFrameCount;\n> +\t\t\t\t? 0 : result.dropFrameCount;\n>\n> \tfor (auto const stream : data->streams_)\n> \t\tstream->resetBuffers();\n> @@ -1358,7 +1358,7 @@ int PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp, Me\n>\n> \tdata->sensorFormats_ = populateSensorFormats(data->sensor_);\n>\n> -\tipa::RPi::IPAInitResult result;\n> +\tipa::RPi::InitResult result;\n> \tif (data->loadIPA(&result)) {\n> \t\tLOG(RPI, Error) << \"Failed to load a suitable IPA library\";\n> \t\treturn -EINVAL;\n> @@ -1599,7 +1599,7 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)\n> void PipelineHandlerRPi::mapBuffers(Camera *camera, const RPi::BufferMap &buffers, unsigned int mask)\n> {\n> \tRPiCameraData *data = cameraData(camera);\n> -\tstd::vector ipaBuffers;\n> +\tstd::vector bufferIds;\n> \t/*\n> \t * Link the FrameBuffers with the id (key value) in the map stored in\n> \t * the RPi stream object - along with an identifier mask.\n> @@ -1608,12 +1608,12 @@ void PipelineHandlerRPi::mapBuffers(Camera *camera, const RPi::BufferMap &buffer\n> \t * handler and the IPA.\n> \t */\n> \tfor (auto const &it : buffers) {\n> -\t\tipaBuffers.push_back(IPABuffer(mask | it.first,\n> +\t\tbufferIds.push_back(IPABuffer(mask | it.first,\n> \t\t\t\t\t it.second->planes()));\n> -\t\tdata->ipaBuffers_.insert(mask | it.first);\n> +\t\tdata->bufferIds_.insert(mask | it.first);\n> \t}\n>\n> -\tdata->ipa_->mapBuffers(ipaBuffers);\n> +\tdata->ipa_->mapBuffers(bufferIds);\n> }\n>\n> void RPiCameraData::freeBuffers()\n> @@ -1623,10 +1623,10 @@ void RPiCameraData::freeBuffers()\n> \t\t * Copy the buffer ids from the unordered_set to a vector to\n> \t\t * pass to the IPA.\n> \t\t */\n> -\t\tstd::vector ipaBuffers(ipaBuffers_.begin(),\n> -\t\t\t\t\t\t ipaBuffers_.end());\n> -\t\tipa_->unmapBuffers(ipaBuffers);\n> -\t\tipaBuffers_.clear();\n> +\t\tstd::vector bufferIds(bufferIds_.begin(),\n> +\t\t\t\t\t\t bufferIds_.end());\n> +\t\tipa_->unmapBuffers(bufferIds);\n> +\t\tbufferIds_.clear();\n> \t}\n>\n> \tfor (auto const stream : streams_)\n> @@ -1643,16 +1643,16 @@ void RPiCameraData::frameStarted(uint32_t sequence)\n> \tdelayedCtrls_->applyControls(sequence);\n> }\n>\n> -int RPiCameraData::loadIPA(ipa::RPi::IPAInitResult *result)\n> +int RPiCameraData::loadIPA(ipa::RPi::InitResult *result)\n> {\n> \tipa_ = IPAManager::createIPA(pipe(), 1, 1);\n>\n> \tif (!ipa_)\n> \t\treturn -ENOENT;\n>\n> -\tipa_->statsMetadataComplete.connect(this, &RPiCameraData::statsMetadataComplete);\n> -\tipa_->runIsp.connect(this, &RPiCameraData::runIsp);\n> -\tipa_->embeddedComplete.connect(this, &RPiCameraData::embeddedComplete);\n> +\tipa_->processStatsComplete.connect(this, &RPiCameraData::processStatsComplete);\n> +\tipa_->prepareIspComplete.connect(this, &RPiCameraData::prepareIspComplete);\n> +\tipa_->metadataReady.connect(this, &RPiCameraData::metadataReady);\n> \tipa_->setIspControls.connect(this, &RPiCameraData::setIspControls);\n> \tipa_->setDelayedControls.connect(this, &RPiCameraData::setDelayedControls);\n> \tipa_->setLensControls.connect(this, &RPiCameraData::setLensControls);\n> @@ -1674,23 +1674,25 @@ int RPiCameraData::loadIPA(ipa::RPi::IPAInitResult *result)\n> \t}\n>\n> \tIPASettings settings(configurationFile, sensor_->model());\n> +\tipa::RPi::InitParams params;\n>\n> -\treturn ipa_->init(settings, !!sensor_->focusLens(), result);\n> +\tparams.lensPresent = !!sensor_->focusLens();\n> +\treturn ipa_->init(settings, params, result);\n> }\n>\n> -int RPiCameraData::configureIPA(const CameraConfiguration *config, ipa::RPi::IPAConfigResult *result)\n> +int RPiCameraData::configureIPA(const CameraConfiguration *config, ipa::RPi::ConfigResult *result)\n> {\n> \tstd::map entityControls;\n> -\tipa::RPi::IPAConfig ipaConfig;\n> +\tipa::RPi::ConfigParams params;\n>\n> \t/* \\todo Move passing of ispControls and lensControls to ipa::init() */\n> -\tipaConfig.sensorControls = sensor_->controls();\n> -\tipaConfig.ispControls = isp_[Isp::Input].dev()->controls();\n> +\tparams.sensorControls = sensor_->controls();\n> +\tparams.ispControls = isp_[Isp::Input].dev()->controls();\n> \tif (sensor_->focusLens())\n> -\t\tipaConfig.lensControls = sensor_->focusLens()->controls();\n> +\t\tparams.lensControls = sensor_->focusLens()->controls();\n>\n> \t/* Always send the user transform to the IPA. */\n> -\tipaConfig.transform = static_cast(config->transform);\n> +\tparams.transform = static_cast(config->transform);\n>\n> \t/* Allocate the lens shading table via dmaHeap and pass to the IPA. */\n> \tif (!lsTable_.isValid()) {\n> @@ -1703,7 +1705,7 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config, ipa::RPi::IPA\n> \t\t * \\todo Investigate if mapping the lens shading table buffer\n> \t\t * could be handled with mapBuffers().\n> \t\t */\n> -\t\tipaConfig.lsTableHandle = lsTable_;\n> +\t\tparams.lsTableHandle = lsTable_;\n> \t}\n>\n> \t/* We store the IPACameraSensorInfo for digital zoom calculations. */\n> @@ -1714,15 +1716,14 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config, ipa::RPi::IPA\n> \t}\n>\n> \t/* Ready the IPA - it must know about the sensor resolution. */\n> -\tControlList controls;\n> -\tret = ipa_->configure(sensorInfo_, ipaConfig, &controls, result);\n> +\tret = ipa_->configure(sensorInfo_, params, result);\n> \tif (ret < 0) {\n> \t\tLOG(RPI, Error) << \"IPA configuration failed!\";\n> \t\treturn -EPIPE;\n> \t}\n>\n> -\tif (!controls.empty())\n> -\t\tsetSensorControls(controls);\n> +\tif (!result->controls.empty())\n> +\t\tsetSensorControls(result->controls);\n>\n> \treturn 0;\n> }\n> @@ -1883,24 +1884,32 @@ void RPiCameraData::enumerateVideoDevices(MediaLink *link)\n> \t}\n> }\n>\n> -void RPiCameraData::statsMetadataComplete(uint32_t bufferId, const ControlList &controls)\n> +void RPiCameraData::processStatsComplete(const ipa::RPi::BufferIds &buffers)\n> {\n> \tif (!isRunning())\n> \t\treturn;\n>\n> -\tFrameBuffer *buffer = isp_[Isp::Stats].getBuffers().at(bufferId & RPi::MaskID);\n> +\tFrameBuffer *buffer = isp_[Isp::Stats].getBuffers().at(buffers.stats & RPi::MaskID);\n>\n> \thandleStreamBuffer(buffer, &isp_[Isp::Stats]);\n> +\tstate_ = State::IpaComplete;\n> +\thandleState();\n> +}\n> +\n> +void RPiCameraData::metadataReady(const ControlList &metadata)\n> +{\n> +\tif (!isRunning())\n> +\t\treturn;\n>\n> \t/* Add to the Request metadata buffer what the IPA has provided. */\n> \tRequest *request = requestQueue_.front();\n> -\trequest->metadata().merge(controls);\n> +\trequest->metadata().merge(metadata);\n>\n> \t/*\n> \t * Inform the sensor of the latest colour gains if it has the\n> \t * V4L2_CID_NOTIFY_GAINS control (which means notifyGainsUnity_ is set).\n> \t */\n> -\tconst auto &colourGains = controls.get(libcamera::controls::ColourGains);\n> +\tconst auto &colourGains = metadata.get(libcamera::controls::ColourGains);\n> \tif (notifyGainsUnity_ && colourGains) {\n> \t\t/* The control wants linear gains in the order B, Gb, Gr, R. */\n> \t\tControlList ctrls(sensor_->controls());\n> @@ -1914,33 +1923,29 @@ void RPiCameraData::statsMetadataComplete(uint32_t bufferId, const ControlList &\n>\n> \t\tsensor_->setControls(&ctrls);\n> \t}\n> -\n> -\tstate_ = State::IpaComplete;\n> -\thandleState();\n> }\n>\n> -void RPiCameraData::runIsp(uint32_t bufferId)\n> +void RPiCameraData::prepareIspComplete(const ipa::RPi::BufferIds &buffers)\n> {\n> +\tunsigned int embeddedId = buffers.embedded & RPi::MaskID;\n> +\tunsigned int bayer = buffers.bayer & RPi::MaskID;\n> +\tFrameBuffer *buffer;\n> +\n> \tif (!isRunning())\n> \t\treturn;\n>\n> -\tFrameBuffer *buffer = unicam_[Unicam::Image].getBuffers().at(bufferId & RPi::MaskID);\n> -\n> -\tLOG(RPI, Debug) << \"Input re-queue to ISP, buffer id \" << (bufferId & RPi::MaskID)\n> +\tbuffer = unicam_[Unicam::Image].getBuffers().at(bayer & RPi::MaskID);\n> +\tLOG(RPI, Debug) << \"Input re-queue to ISP, buffer id \" << (bayer & RPi::MaskID)\n> \t\t\t<< \", timestamp: \" << buffer->metadata().timestamp;\n>\n> \tisp_[Isp::Input].queueBuffer(buffer);\n> \tispOutputCount_ = 0;\n> -\thandleState();\n> -}\n>\n> -void RPiCameraData::embeddedComplete(uint32_t bufferId)\n> -{\n> -\tif (!isRunning())\n> -\t\treturn;\n> +\tif (sensorMetadata_ && embeddedId) {\n> +\t\tbuffer = unicam_[Unicam::Embedded].getBuffers().at(embeddedId & RPi::MaskID);\n> +\t\thandleStreamBuffer(buffer, &unicam_[Unicam::Embedded]);\n> +\t}\n>\n> -\tFrameBuffer *buffer = unicam_[Unicam::Embedded].getBuffers().at(bufferId & RPi::MaskID);\n> -\thandleStreamBuffer(buffer, &unicam_[Unicam::Embedded]);\n> \thandleState();\n> }\n>\n> @@ -2116,8 +2121,10 @@ void RPiCameraData::ispOutputDequeue(FrameBuffer *buffer)\n> \t * application until after the IPA signals so.\n> \t */\n> \tif (stream == &isp_[Isp::Stats]) {\n> -\t\tipa_->signalStatReady(RPi::MaskStats | static_cast(index),\n> -\t\t\t\t requestQueue_.front()->sequence());\n> +\t\tipa::RPi::ProcessParams params;\n> +\t\tparams.buffers.stats = index | RPi::MaskStats;\n> +\t\tparams.ipaContext = requestQueue_.front()->sequence();\n> +\t\tipa_->processStats(params);\n> \t} else {\n> \t\t/* Any other ISP output can be handed back to the application now. */\n> \t\thandleStreamBuffer(buffer, stream);\n> @@ -2344,38 +2351,30 @@ void RPiCameraData::tryRunPipeline()\n> \trequest->metadata().clear();\n> \tfillRequestMetadata(bayerFrame.controls, request);\n>\n> -\t/*\n> -\t * Process all the user controls by the IPA. Once this is complete, we\n> -\t * queue the ISP output buffer listed in the request to start the HW\n> -\t * pipeline.\n> -\t */\n> -\tipa_->signalQueueRequest(request->controls());\n> -\n> \t/* Set our state to say the pipeline is active. */\n> \tstate_ = State::Busy;\n>\n> -\tunsigned int bayerId = unicam_[Unicam::Image].getBufferId(bayerFrame.buffer);\n> +\tunsigned int bayer = unicam_[Unicam::Image].getBufferId(bayerFrame.buffer);\n>\n> -\tLOG(RPI, Debug) << \"Signalling signalIspPrepare:\"\n> -\t\t\t<< \" Bayer buffer id: \" << bayerId;\n> +\tLOG(RPI, Debug) << \"Signalling prepareIsp:\"\n> +\t\t\t<< \" Bayer buffer id: \" << bayer;\n>\n> -\tipa::RPi::ISPConfig ispPrepare;\n> -\tispPrepare.bayerBufferId = RPi::MaskBayerData | bayerId;\n> -\tispPrepare.controls = std::move(bayerFrame.controls);\n> -\tispPrepare.ipaContext = request->sequence();\n> -\tispPrepare.delayContext = bayerFrame.delayContext;\n> +\tipa::RPi::PrepareParams params;\n> +\tparams.buffers.bayer = RPi::MaskBayerData | bayer;\n> +\tparams.sensorControls = std::move(bayerFrame.controls);\n> +\tparams.requestControls = request->controls();\n> +\tparams.ipaContext = request->sequence();\n> +\tparams.delayContext = bayerFrame.delayContext;\n>\n> \tif (embeddedBuffer) {\n> \t\tunsigned int embeddedId = unicam_[Unicam::Embedded].getBufferId(embeddedBuffer);\n>\n> -\t\tispPrepare.embeddedBufferId = RPi::MaskEmbeddedData | embeddedId;\n> -\t\tispPrepare.embeddedBufferPresent = true;\n> -\n> -\t\tLOG(RPI, Debug) << \"Signalling signalIspPrepare:\"\n> +\t\tparams.buffers.embedded = RPi::MaskEmbeddedData | embeddedId;\n> +\t\tLOG(RPI, Debug) << \"Signalling prepareIsp:\"\n> \t\t\t\t<< \" Embedded buffer id: \" << embeddedId;\n> \t}\n>\n> -\tipa_->signalIspPrepare(ispPrepare);\n> +\tipa_->prepareIsp(params);\n> }\n>\n> bool RPiCameraData::findMatchingBuffers(BayerFrame &bayerFrame, FrameBuffer *&embeddedBuffer)\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 05EA7BDCBD\n\tfor ;\n\tThu, 27 Apr 2023 08:56:03 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 57D90627D1;\n\tThu, 27 Apr 2023 10:56:02 +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 41259627D1\n\tfor ;\n\tThu, 27 Apr 2023 10:56:01 +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 1F4C6C7E;\n\tThu, 27 Apr 2023 10:55:49 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1682585762;\n\tbh=uNiluVFN3pq4uJE7+TPm4ybAZG0tfHXIwj/Y/Twa4WM=;\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=1hq7/Ci64K+UhaI1cOHQu6L1kzY1Pa6KEHxvTb6lDZBH5REsSiKuxXb8OapBnF54W\n\tQI2WBdg7kkypymlDsEgh+Xk5ma1vuTiDF2hKTVqzN+1rNf8U+tu8XAqtwshTzQLsD+\n\tBeq6B1r+bwO5p7Ohze5ZDCjv/q04sEKq6oDsGXyYzZF+pbqONX4nL5ts8PwgYt40qw\n\t4VuC1B8mppeSqiumB9wim1/mnH/0+ZR0DkYEuz1mah6YJliN0m29CELJX1mysiF+O0\n\teJNaM/ccML51lh4NKOlzXfbf0Z4tYXXjV95MHYLZtwD3PjdONk12s3inUxz5YdSkXr\n\tLJ47wfthQOskw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1682585749;\n\tbh=uNiluVFN3pq4uJE7+TPm4ybAZG0tfHXIwj/Y/Twa4WM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=fEGYlubX+cRMpKF9k9LQNRIRfYioeXDB1IFzPMXv3ynZZFaVDA7tzCGrEDr3rrmXe\n\tjqFXQ6wkzUiEpPIwMFrsPUDuFtOWXO+4kb2nJ3v/4DGjWQfH9xTIa1H/H5JJE2iVje\n\tghIx6/WWWZ3/WOiTug/ypU4rIJcq7gzYTimpukSA="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"fEGYlubX\"; dkim-atps=neutral","Date":"Thu, 27 Apr 2023 10:55:57 +0200","To":"Naushir Patuck ","Message-ID":"<26csbh5y5qh7r4rle34eyn3drneqvvifcqb65s5nppg5ifm6kv@ft5oy4xnkzu4>","References":"<20230426131057.21550-1-naush@raspberrypi.com>\n\t<20230426131057.21550-7-naush@raspberrypi.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20230426131057.21550-7-naush@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH 06/13] pipeline: ipa: raspberrypi:\n\tRestructure the IPA mojom interface","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":26958,"web_url":"https://patchwork.libcamera.org/comment/26958/","msgid":"","date":"2023-04-27T09:53:25","subject":"Re: [libcamera-devel] [PATCH 06/13] pipeline: ipa: raspberrypi:\n\tRestructure the IPA mojom interface","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Jacopo,\n\nOn Thu, 27 Apr 2023 at 09:56, Jacopo Mondi\n wrote:\n>\n> Hi Naush\n>\n> On Wed, Apr 26, 2023 at 02:10:50PM +0100, Naushir Patuck via libcamera-devel wrote:\n> > Restructure the IPA mojom interface to be more consistent in the use\n> > of the API. Function parameters are now grouped into *Params structures\n> > and results are now returned in *Results structures.\n> >\n> > The following pipeline -> IPA interfaces have been removed:\n> >\n> > signalQueueRequest(libcamera.ControlList controls);\n> > signalIspPrepare(ISPConfig data);\n> > signalStatReady(uint32 bufferId, uint32 ipaContext);\n> >\n> > and replaced with:\n> >\n> > prepareIsp(PrepareParams params);\n> > processStats(ProcessParams params);\n> >\n> > signalQueueRequest() is now encompassed within signalPrepareIsp().\n> >\n> > The following IPA -> pipeline interfaces have been removed:\n> >\n> > runIsp(uint32 bufferId);\n> > embeddedComplete(uint32 bufferId);\n> > statsMetadataComplete(uint32 bufferId, libcamera.ControlList controls);\n> >\n> > and replaced with the following async calls:\n> >\n> > prepareIspComplete(BufferIds buffers);\n> > processStatsComplete(BufferIds buffers);\n> > metadataReady(libcamera.ControlList metadata);\n> >\n> > Signed-off-by: Naushir Patuck \n> > ---\n> > include/libcamera/ipa/raspberrypi.mojom | 127 +++++---------\n> > src/ipa/rpi/vc4/raspberrypi.cpp | 102 +++++-------\n> > .../pipeline/rpi/vc4/raspberrypi.cpp | 155 +++++++++---------\n> > 3 files changed, 165 insertions(+), 219 deletions(-)\n> >\n> > diff --git a/include/libcamera/ipa/raspberrypi.mojom b/include/libcamera/ipa/raspberrypi.mojom\n> > index 80e0126618c8..7d56248f4ab3 100644\n> > --- a/include/libcamera/ipa/raspberrypi.mojom\n> > +++ b/include/libcamera/ipa/raspberrypi.mojom\n> > @@ -8,7 +8,7 @@ module ipa.RPi;\n> >\n> > import \"include/libcamera/ipa/core.mojom\";\n> >\n> > -/* Size of the LS grid allocation. */\n> > +/* Size of the LS grid allocation on VC4. */\n> > const uint32 MaxLsGridSize = 0x8000;\n> >\n> > struct SensorConfig {\n> > @@ -19,117 +19,78 @@ struct SensorConfig {\n> > uint32 sensorMetadata;\n> > };\n> >\n> > -struct IPAInitResult {\n> > +struct InitParams {\n> > + bool lensPresent;\n> > +};\n> > +\n> > +struct InitResult {\n> > SensorConfig sensorConfig;\n> > libcamera.ControlInfoMap controlInfo;\n> > };\n> >\n> > -struct ISPConfig {\n> > - uint32 embeddedBufferId;\n> > - uint32 bayerBufferId;\n> > - bool embeddedBufferPresent;\n> > - libcamera.ControlList controls;\n> > - uint32 ipaContext;\n> > - uint32 delayContext;\n> > +struct BufferIds {\n> > + uint32 bayer;\n> > + uint32 embedded;\n> > + uint32 stats;\n> > };\n> >\n> > -struct IPAConfig {\n> > +struct ConfigParams {\n> > uint32 transform;\n> > - libcamera.SharedFD lsTableHandle;\n> > libcamera.ControlInfoMap sensorControls;\n> > libcamera.ControlInfoMap ispControls;\n> > libcamera.ControlInfoMap lensControls;\n> > + /* VC4 specifc */\n> > + libcamera.SharedFD lsTableHandle;\n> > };\n> >\n> > -struct IPAConfigResult {\n> > - float modeSensitivity;\n> > - libcamera.ControlInfoMap controlInfo;\n> > +struct ConfigResult {\n> > + float modeSensitivity;\n> > + libcamera.ControlInfoMap controlInfo;\n> > + libcamera.ControlList controls;\n> > };\n> >\n> > -struct StartConfig {\n> > +struct StartResult {\n> > libcamera.ControlList controls;\n> > int32 dropFrameCount;\n> > };\n> >\n> > +struct PrepareParams {\n> > + BufferIds buffers;\n> > + libcamera.ControlList sensorControls;\n> > + libcamera.ControlList requestControls;\n> > + uint32 ipaContext;\n> > + uint32 delayContext;\n> > +};\n> > +\n> > +struct ProcessParams {\n> > + BufferIds buffers;\n> > + uint32 ipaContext;\n> > +};\n> > +\n> > interface IPARPiInterface {\n> > - init(libcamera.IPASettings settings, bool lensPresent)\n> > - => (int32 ret, IPAInitResult result);\n> > - start(libcamera.ControlList controls) => (StartConfig startConfig);\n> > + init(libcamera.IPASettings settings, InitParams params)\n> > + => (int32 ret, InitResult result);\n> > +\n> > + start(libcamera.ControlList controls) => (StartResult result);\n> > stop();\n> >\n> > - /**\n> > - * \\fn configure()\n>\n> Is it intentional to drop documentation ? Was it used at all ?\n\nI don't believe it is at all used. I looked at ipu3 and rkisp1 and they have\nno doc tags either.\n\n>\n> > - * \\brief Configure the IPA stream and sensor settings\n> > - * \\param[in] sensorInfo Camera sensor information\n> > - * \\param[in] ipaConfig Pipeline-handler-specific configuration data\n> > - * \\param[out] controls Controls to apply by the pipeline entity\n> > - * \\param[out] result Other results that the pipeline handler may require\n> > - *\n> > - * This function shall be called when the camera is configured to inform\n> > - * the IPA of the camera's streams and the sensor settings.\n> > - *\n> > - * The \\a sensorInfo conveys information about the camera sensor settings that\n> > - * the pipeline handler has selected for the configuration.\n> > - *\n> > - * The \\a ipaConfig and \\a controls parameters carry data passed by the\n> > - * pipeline handler to the IPA and back.\n> > - */\n> > - configure(libcamera.IPACameraSensorInfo sensorInfo,\n> > - IPAConfig ipaConfig)\n> > - => (int32 ret, libcamera.ControlList controls, IPAConfigResult result);\n> > -\n> > - /**\n> > - * \\fn mapBuffers()\n> > - * \\brief Map buffers shared between the pipeline handler and the IPA\n> > - * \\param[in] buffers List of buffers to map\n> > - *\n> > - * This function informs the IPA module of memory buffers set up by the\n> > - * pipeline handler that the IPA needs to access. It provides dmabuf\n> > - * file handles for each buffer, and associates the buffers with unique\n> > - * numerical IDs.\n> > - *\n> > - * IPAs shall map the dmabuf file handles to their address space and\n> > - * keep a cache of the mappings, indexed by the buffer numerical IDs.\n> > - * The IDs are used in all other IPA interface functions to refer to\n> > - * buffers, including the unmapBuffers() function.\n> > - *\n> > - * All buffers that the pipeline handler wishes to share with an IPA\n> > - * shall be mapped with this function. Buffers may be mapped all at once\n> > - * with a single call, or mapped and unmapped dynamically at runtime,\n> > - * depending on the IPA protocol. Regardless of the protocol, all\n> > - * buffers mapped at a given time shall have unique numerical IDs.\n> > - *\n> > - * The numerical IDs have no meaning defined by the IPA interface, and\n> > - * should be treated as opaque handles by IPAs, with the only exception\n> > - * that ID zero is invalid.\n> > - *\n> > - * \\sa unmapBuffers()\n> > - */\n> > - mapBuffers(array buffers);\n> > + configure(libcamera.IPACameraSensorInfo sensorInfo, ConfigParams params)\n> > + => (int32 ret, ConfigResult result);\n> >\n> > - /**\n> > - * \\fn unmapBuffers()\n> > - * \\brief Unmap buffers shared by the pipeline to the IPA\n> > - * \\param[in] ids List of buffer IDs to unmap\n> > - *\n> > - * This function removes mappings set up with mapBuffers(). Numerical\n> > - * IDs of unmapped buffers may be reused when mapping new buffers.\n> > - *\n> > - * \\sa mapBuffers()\n> > - */\n> > + mapBuffers(array buffers);\n> > unmapBuffers(array ids);\n> >\n> > - [async] signalStatReady(uint32 bufferId, uint32 ipaContext);\n> > - [async] signalQueueRequest(libcamera.ControlList controls);\n> > - [async] signalIspPrepare(ISPConfig data);\n> > + [async] prepareIsp(PrepareParams params);\n> > + [async] processStats(ProcessParams params);\n> > };\n> >\n> > interface IPARPiEventInterface {\n> > - statsMetadataComplete(uint32 bufferId, libcamera.ControlList controls);\n> > - runIsp(uint32 bufferId);\n> > - embeddedComplete(uint32 bufferId);\n> > + prepareIspComplete(BufferIds buffers);\n> > + processStatsComplete(BufferIds buffers);\n> > + metadataReady(libcamera.ControlList metadata);\n> > setIspControls(libcamera.ControlList controls);\n> > setDelayedControls(libcamera.ControlList controls, uint32 delayContext);\n> > setLensControls(libcamera.ControlList controls);\n> > setCameraTimeout(uint32 maxFrameLengthMs);\n> > };\n> > +\n>\n> Additional blank line\n\nAck\n\nRegards,\nNaush\n\n>\n> The rest looks good\n> Reviewed-by: Jacopo Mondi \n>\n> Thanks\n> j\n>\n> > diff --git a/src/ipa/rpi/vc4/raspberrypi.cpp b/src/ipa/rpi/vc4/raspberrypi.cpp\n> > index 841635ccde2b..d737f1d662a0 100644\n> > --- a/src/ipa/rpi/vc4/raspberrypi.cpp\n> > +++ b/src/ipa/rpi/vc4/raspberrypi.cpp\n> > @@ -136,30 +136,28 @@ public:\n> > munmap(lsTable_, MaxLsGridSize);\n> > }\n> >\n> > - int init(const IPASettings &settings, bool lensPresent, IPAInitResult *result) override;\n> > - void start(const ControlList &controls, StartConfig *startConfig) override;\n> > + int init(const IPASettings &settings, const InitParams ¶ms, InitResult *result) override;\n> > + void start(const ControlList &controls, StartResult *result) override;\n> > void stop() override {}\n> >\n> > - int configure(const IPACameraSensorInfo &sensorInfo, const IPAConfig &data,\n> > - ControlList *controls, IPAConfigResult *result) override;\n> > + int configure(const IPACameraSensorInfo &sensorInfo, const ConfigParams ¶ms,\n> > + ConfigResult *result) override;\n> > void mapBuffers(const std::vector &buffers) override;\n> > void unmapBuffers(const std::vector &ids) override;\n> > - void signalStatReady(const uint32_t bufferId, uint32_t ipaContext) override;\n> > - void signalQueueRequest(const ControlList &controls) override;\n> > - void signalIspPrepare(const ISPConfig &data) override;\n> > + void prepareIsp(const PrepareParams ¶ms) override;\n> > + void processStats(const ProcessParams ¶ms) override;\n> >\n> > private:\n> > void setMode(const IPACameraSensorInfo &sensorInfo);\n> > bool validateSensorControls();\n> > bool validateIspControls();\n> > bool validateLensControls();\n> > - void queueRequest(const ControlList &controls);\n> > - void returnEmbeddedBuffer(unsigned int bufferId);\n> > - void prepareISP(const ISPConfig &data);\n> > + void applyControls(const ControlList &controls);\n> > + void prepare(const PrepareParams ¶ms);\n> > void reportMetadata(unsigned int ipaContext);\n> > void fillDeviceStatus(const ControlList &sensorControls, unsigned int ipaContext);\n> > RPiController::StatisticsPtr fillStatistics(bcm2835_isp_stats *stats) const;\n> > - void processStats(unsigned int bufferId, unsigned int ipaContext);\n> > + void process(unsigned int bufferId, unsigned int ipaContext);\n> > void setCameraTimeoutValue();\n> > void applyFrameDurations(Duration minFrameDuration, Duration maxFrameDuration);\n> > void applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls);\n> > @@ -229,7 +227,7 @@ private:\n> > Duration lastTimeout_;\n> > };\n> >\n> > -int IPARPi::init(const IPASettings &settings, bool lensPresent, IPAInitResult *result)\n> > +int IPARPi::init(const IPASettings &settings, const InitParams ¶ms, InitResult *result)\n> > {\n> > /*\n> > * Load the \"helper\" for this sensor. This tells us all the device specific stuff\n> > @@ -274,7 +272,7 @@ int IPARPi::init(const IPASettings &settings, bool lensPresent, IPAInitResult *r\n> > return -EINVAL;\n> > }\n> >\n> > - lensPresent_ = lensPresent;\n> > + lensPresent_ = params.lensPresent;\n> >\n> > controller_.initialise();\n> >\n> > @@ -287,14 +285,13 @@ int IPARPi::init(const IPASettings &settings, bool lensPresent, IPAInitResult *r\n> > return 0;\n> > }\n> >\n> > -void IPARPi::start(const ControlList &controls, StartConfig *startConfig)\n> > +void IPARPi::start(const ControlList &controls, StartResult *result)\n> > {\n> > RPiController::Metadata metadata;\n> >\n> > - ASSERT(startConfig);\n> > if (!controls.empty()) {\n> > /* We have been given some controls to action before start. */\n> > - queueRequest(controls);\n> > + applyControls(controls);\n> > }\n> >\n> > controller_.switchMode(mode_, &metadata);\n> > @@ -313,7 +310,7 @@ void IPARPi::start(const ControlList &controls, StartConfig *startConfig)\n> > if (agcStatus.shutterTime && agcStatus.analogueGain) {\n> > ControlList ctrls(sensorCtrls_);\n> > applyAGC(&agcStatus, ctrls);\n> > - startConfig->controls = std::move(ctrls);\n> > + result->controls = std::move(ctrls);\n> > setCameraTimeoutValue();\n> > }\n> >\n> > @@ -360,7 +357,7 @@ void IPARPi::start(const ControlList &controls, StartConfig *startConfig)\n> > mistrustCount_ = helper_->mistrustFramesModeSwitch();\n> > }\n> >\n> > - startConfig->dropFrameCount = dropFrameCount_;\n> > + result->dropFrameCount = dropFrameCount_;\n> >\n> > firstStart_ = false;\n> > lastRunTimestamp_ = 0;\n> > @@ -435,11 +432,11 @@ void IPARPi::setMode(const IPACameraSensorInfo &sensorInfo)\n> > mode_.minFrameDuration, mode_.maxFrameDuration);\n> > }\n> >\n> > -int IPARPi::configure(const IPACameraSensorInfo &sensorInfo, const IPAConfig &ipaConfig,\n> > - ControlList *controls, IPAConfigResult *result)\n> > +int IPARPi::configure(const IPACameraSensorInfo &sensorInfo, const ConfigParams ¶ms,\n> > + ConfigResult *result)\n> > {\n> > - sensorCtrls_ = ipaConfig.sensorControls;\n> > - ispCtrls_ = ipaConfig.ispControls;\n> > + sensorCtrls_ = params.sensorControls;\n> > + ispCtrls_ = params.ispControls;\n> >\n> > if (!validateSensorControls()) {\n> > LOG(IPARPI, Error) << \"Sensor control validation failed.\";\n> > @@ -452,7 +449,7 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo, const IPAConfig &ip\n> > }\n> >\n> > if (lensPresent_) {\n> > - lensCtrls_ = ipaConfig.lensControls;\n> > + lensCtrls_ = params.lensControls;\n> > if (!validateLensControls()) {\n> > LOG(IPARPI, Warning) << \"Lens validation failed, \"\n> > << \"no lens control will be available.\";\n> > @@ -466,10 +463,10 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo, const IPAConfig &ip\n> > /* Re-assemble camera mode using the sensor info. */\n> > setMode(sensorInfo);\n> >\n> > - mode_.transform = static_cast(ipaConfig.transform);\n> > + mode_.transform = static_cast(params.transform);\n> >\n> > /* Store the lens shading table pointer and handle if available. */\n> > - if (ipaConfig.lsTableHandle.isValid()) {\n> > + if (params.lsTableHandle.isValid()) {\n> > /* Remove any previous table, if there was one. */\n> > if (lsTable_) {\n> > munmap(lsTable_, MaxLsGridSize);\n> > @@ -477,7 +474,7 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo, const IPAConfig &ip\n> > }\n> >\n> > /* Map the LS table buffer into user space. */\n> > - lsTableHandle_ = std::move(ipaConfig.lsTableHandle);\n> > + lsTableHandle_ = std::move(params.lsTableHandle);\n> > if (lsTableHandle_.isValid()) {\n> > lsTable_ = mmap(nullptr, MaxLsGridSize, PROT_READ | PROT_WRITE,\n> > MAP_SHARED, lsTableHandle_.get(), 0);\n> > @@ -512,8 +509,7 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo, const IPAConfig &ip\n> > applyAGC(&agcStatus, ctrls);\n> > }\n> >\n> > - ASSERT(controls);\n> > - *controls = std::move(ctrls);\n> > + result->controls = std::move(ctrls);\n> >\n> > /*\n> > * Apply the correct limits to the exposure, gain and frame duration controls\n> > @@ -560,37 +556,34 @@ void IPARPi::unmapBuffers(const std::vector &ids)\n> > }\n> > }\n> >\n> > -void IPARPi::signalStatReady(uint32_t bufferId, uint32_t ipaContext)\n> > +void IPARPi::processStats(const ProcessParams ¶ms)\n> > {\n> > - unsigned int context = ipaContext % rpiMetadata_.size();\n> > + unsigned int context = params.ipaContext % rpiMetadata_.size();\n> >\n> > if (++checkCount_ != frameCount_) /* assert here? */\n> > LOG(IPARPI, Error) << \"WARNING: Prepare/Process mismatch!!!\";\n> > if (processPending_ && frameCount_ > mistrustCount_)\n> > - processStats(bufferId, context);\n> > + process(params.buffers.stats, context);\n> >\n> > reportMetadata(context);\n> > -\n> > - statsMetadataComplete.emit(bufferId, libcameraMetadata_);\n> > + processStatsComplete.emit(params.buffers);\n> > }\n> >\n> > -void IPARPi::signalQueueRequest(const ControlList &controls)\n> > -{\n> > - queueRequest(controls);\n> > -}\n> >\n> > -void IPARPi::signalIspPrepare(const ISPConfig &data)\n> > +void IPARPi::prepareIsp(const PrepareParams ¶ms)\n> > {\n> > + applyControls(params.requestControls);\n> > +\n> > /*\n> > * At start-up, or after a mode-switch, we may want to\n> > * avoid running the control algos for a few frames in case\n> > * they are \"unreliable\".\n> > */\n> > - prepareISP(data);\n> > + prepare(params);\n> > frameCount_++;\n> >\n> > /* Ready to push the input buffer into the ISP. */\n> > - runIsp.emit(data.bayerBufferId);\n> > + prepareIspComplete.emit(params.buffers);\n> > }\n> >\n> > void IPARPi::reportMetadata(unsigned int ipaContext)\n> > @@ -703,6 +696,8 @@ void IPARPi::reportMetadata(unsigned int ipaContext)\n> > libcameraMetadata_.set(controls::AfState, s);\n> > libcameraMetadata_.set(controls::AfPauseState, p);\n> > }\n> > +\n> > + metadataReady.emit(libcameraMetadata_);\n> > }\n> >\n> > bool IPARPi::validateSensorControls()\n> > @@ -826,7 +821,7 @@ static const std::map AfPauseTable\n> > { controls::AfPauseResume, RPiController::AfAlgorithm::AfPauseResume },\n> > };\n> >\n> > -void IPARPi::queueRequest(const ControlList &controls)\n> > +void IPARPi::applyControls(const ControlList &controls)\n> > {\n> > using RPiController::AfAlgorithm;\n> >\n> > @@ -1256,27 +1251,22 @@ void IPARPi::queueRequest(const ControlList &controls)\n> > }\n> > }\n> >\n> > -void IPARPi::returnEmbeddedBuffer(unsigned int bufferId)\n> > +void IPARPi::prepare(const PrepareParams ¶ms)\n> > {\n> > - embeddedComplete.emit(bufferId);\n> > -}\n> > -\n> > -void IPARPi::prepareISP(const ISPConfig &data)\n> > -{\n> > - int64_t frameTimestamp = data.controls.get(controls::SensorTimestamp).value_or(0);\n> > - unsigned int ipaContext = data.ipaContext % rpiMetadata_.size();\n> > + int64_t frameTimestamp = params.sensorControls.get(controls::SensorTimestamp).value_or(0);\n> > + unsigned int ipaContext = params.ipaContext % rpiMetadata_.size();\n> > RPiController::Metadata &rpiMetadata = rpiMetadata_[ipaContext];\n> > Span embeddedBuffer;\n> >\n> > rpiMetadata.clear();\n> > - fillDeviceStatus(data.controls, ipaContext);\n> > + fillDeviceStatus(params.sensorControls, ipaContext);\n> >\n> > - if (data.embeddedBufferPresent) {\n> > + if (params.buffers.embedded) {\n> > /*\n> > * Pipeline handler has supplied us with an embedded data buffer,\n> > * we must pass it to the CamHelper for parsing.\n> > */\n> > - auto it = buffers_.find(data.embeddedBufferId);\n> > + auto it = buffers_.find(params.buffers.embedded);\n> > ASSERT(it != buffers_.end());\n> > embeddedBuffer = it->second.planes()[0];\n> > }\n> > @@ -1288,7 +1278,7 @@ void IPARPi::prepareISP(const ISPConfig &data)\n> > * metadata.\n> > */\n> > AgcStatus agcStatus;\n> > - RPiController::Metadata &delayedMetadata = rpiMetadata_[data.delayContext];\n> > + RPiController::Metadata &delayedMetadata = rpiMetadata_[params.delayContext];\n> > if (!delayedMetadata.get(\"agc.status\", agcStatus))\n> > rpiMetadata.set(\"agc.delayed_status\", agcStatus);\n> >\n> > @@ -1298,10 +1288,6 @@ void IPARPi::prepareISP(const ISPConfig &data)\n> > */\n> > helper_->prepare(embeddedBuffer, rpiMetadata);\n> >\n> > - /* Done with embedded data now, return to pipeline handler asap. */\n> > - if (data.embeddedBufferPresent)\n> > - returnEmbeddedBuffer(data.embeddedBufferId);\n> > -\n> > /* Allow a 10% margin on the comparison below. */\n> > Duration delta = (frameTimestamp - lastRunTimestamp_) * 1.0ns;\n> > if (lastRunTimestamp_ && frameCount_ > dropFrameCount_ &&\n> > @@ -1445,7 +1431,7 @@ RPiController::StatisticsPtr IPARPi::fillStatistics(bcm2835_isp_stats *stats) co\n> > return statistics;\n> > }\n> >\n> > -void IPARPi::processStats(unsigned int bufferId, unsigned int ipaContext)\n> > +void IPARPi::process(unsigned int bufferId, unsigned int ipaContext)\n> > {\n> > RPiController::Metadata &rpiMetadata = rpiMetadata_[ipaContext];\n> >\n> > diff --git a/src/libcamera/pipeline/rpi/vc4/raspberrypi.cpp b/src/libcamera/pipeline/rpi/vc4/raspberrypi.cpp\n> > index e54bf1ef2c17..30ce6feab0d1 100644\n> > --- a/src/libcamera/pipeline/rpi/vc4/raspberrypi.cpp\n> > +++ b/src/libcamera/pipeline/rpi/vc4/raspberrypi.cpp\n> > @@ -200,15 +200,15 @@ public:\n> > void freeBuffers();\n> > void frameStarted(uint32_t sequence);\n> >\n> > - int loadIPA(ipa::RPi::IPAInitResult *result);\n> > - int configureIPA(const CameraConfiguration *config, ipa::RPi::IPAConfigResult *result);\n> > + int loadIPA(ipa::RPi::InitResult *result);\n> > + int configureIPA(const CameraConfiguration *config, ipa::RPi::ConfigResult *result);\n> > int loadPipelineConfiguration();\n> >\n> > void enumerateVideoDevices(MediaLink *link);\n> >\n> > - void statsMetadataComplete(uint32_t bufferId, const ControlList &controls);\n> > - void runIsp(uint32_t bufferId);\n> > - void embeddedComplete(uint32_t bufferId);\n> > + void processStatsComplete(const ipa::RPi::BufferIds &buffers);\n> > + void metadataReady(const ControlList &metadata);\n> > + void prepareIspComplete(const ipa::RPi::BufferIds &buffers);\n> > void setIspControls(const ControlList &controls);\n> > void setDelayedControls(const ControlList &controls, uint32_t delayContext);\n> > void setLensControls(const ControlList &controls);\n> > @@ -238,7 +238,7 @@ public:\n> > /* The vector below is just for convenience when iterating over all streams. */\n> > std::vector streams_;\n> > /* Stores the ids of the buffers mapped in the IPA. */\n> > - std::unordered_set ipaBuffers_;\n> > + std::unordered_set bufferIds_;\n> > /*\n> > * Stores a cascade of Video Mux or Bridge devices between the sensor and\n> > * Unicam together with media link across the entities.\n> > @@ -1000,7 +1000,7 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)\n> >\n> > data->isp_[Isp::Input].dev()->setSelection(V4L2_SEL_TGT_CROP, &crop);\n> >\n> > - ipa::RPi::IPAConfigResult result;\n> > + ipa::RPi::ConfigResult result;\n> > ret = data->configureIPA(config, &result);\n> > if (ret)\n> > LOG(RPI, Error) << \"Failed to configure the IPA: \" << ret;\n> > @@ -1117,17 +1117,17 @@ int PipelineHandlerRPi::start(Camera *camera, const ControlList *controls)\n> > data->applyScalerCrop(*controls);\n> >\n> > /* Start the IPA. */\n> > - ipa::RPi::StartConfig startConfig;\n> > + ipa::RPi::StartResult result;\n> > data->ipa_->start(controls ? *controls : ControlList{ controls::controls },\n> > - &startConfig);\n> > + &result);\n> >\n> > /* Apply any gain/exposure settings that the IPA may have passed back. */\n> > - if (!startConfig.controls.empty())\n> > - data->setSensorControls(startConfig.controls);\n> > + if (!result.controls.empty())\n> > + data->setSensorControls(result.controls);\n> >\n> > /* Configure the number of dropped frames required on startup. */\n> > data->dropFrameCount_ = data->config_.disableStartupFrameDrops\n> > - ? 0 : startConfig.dropFrameCount;\n> > + ? 0 : result.dropFrameCount;\n> >\n> > for (auto const stream : data->streams_)\n> > stream->resetBuffers();\n> > @@ -1358,7 +1358,7 @@ int PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp, Me\n> >\n> > data->sensorFormats_ = populateSensorFormats(data->sensor_);\n> >\n> > - ipa::RPi::IPAInitResult result;\n> > + ipa::RPi::InitResult result;\n> > if (data->loadIPA(&result)) {\n> > LOG(RPI, Error) << \"Failed to load a suitable IPA library\";\n> > return -EINVAL;\n> > @@ -1599,7 +1599,7 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)\n> > void PipelineHandlerRPi::mapBuffers(Camera *camera, const RPi::BufferMap &buffers, unsigned int mask)\n> > {\n> > RPiCameraData *data = cameraData(camera);\n> > - std::vector ipaBuffers;\n> > + std::vector bufferIds;\n> > /*\n> > * Link the FrameBuffers with the id (key value) in the map stored in\n> > * the RPi stream object - along with an identifier mask.\n> > @@ -1608,12 +1608,12 @@ void PipelineHandlerRPi::mapBuffers(Camera *camera, const RPi::BufferMap &buffer\n> > * handler and the IPA.\n> > */\n> > for (auto const &it : buffers) {\n> > - ipaBuffers.push_back(IPABuffer(mask | it.first,\n> > + bufferIds.push_back(IPABuffer(mask | it.first,\n> > it.second->planes()));\n> > - data->ipaBuffers_.insert(mask | it.first);\n> > + data->bufferIds_.insert(mask | it.first);\n> > }\n> >\n> > - data->ipa_->mapBuffers(ipaBuffers);\n> > + data->ipa_->mapBuffers(bufferIds);\n> > }\n> >\n> > void RPiCameraData::freeBuffers()\n> > @@ -1623,10 +1623,10 @@ void RPiCameraData::freeBuffers()\n> > * Copy the buffer ids from the unordered_set to a vector to\n> > * pass to the IPA.\n> > */\n> > - std::vector ipaBuffers(ipaBuffers_.begin(),\n> > - ipaBuffers_.end());\n> > - ipa_->unmapBuffers(ipaBuffers);\n> > - ipaBuffers_.clear();\n> > + std::vector bufferIds(bufferIds_.begin(),\n> > + bufferIds_.end());\n> > + ipa_->unmapBuffers(bufferIds);\n> > + bufferIds_.clear();\n> > }\n> >\n> > for (auto const stream : streams_)\n> > @@ -1643,16 +1643,16 @@ void RPiCameraData::frameStarted(uint32_t sequence)\n> > delayedCtrls_->applyControls(sequence);\n> > }\n> >\n> > -int RPiCameraData::loadIPA(ipa::RPi::IPAInitResult *result)\n> > +int RPiCameraData::loadIPA(ipa::RPi::InitResult *result)\n> > {\n> > ipa_ = IPAManager::createIPA(pipe(), 1, 1);\n> >\n> > if (!ipa_)\n> > return -ENOENT;\n> >\n> > - ipa_->statsMetadataComplete.connect(this, &RPiCameraData::statsMetadataComplete);\n> > - ipa_->runIsp.connect(this, &RPiCameraData::runIsp);\n> > - ipa_->embeddedComplete.connect(this, &RPiCameraData::embeddedComplete);\n> > + ipa_->processStatsComplete.connect(this, &RPiCameraData::processStatsComplete);\n> > + ipa_->prepareIspComplete.connect(this, &RPiCameraData::prepareIspComplete);\n> > + ipa_->metadataReady.connect(this, &RPiCameraData::metadataReady);\n> > ipa_->setIspControls.connect(this, &RPiCameraData::setIspControls);\n> > ipa_->setDelayedControls.connect(this, &RPiCameraData::setDelayedControls);\n> > ipa_->setLensControls.connect(this, &RPiCameraData::setLensControls);\n> > @@ -1674,23 +1674,25 @@ int RPiCameraData::loadIPA(ipa::RPi::IPAInitResult *result)\n> > }\n> >\n> > IPASettings settings(configurationFile, sensor_->model());\n> > + ipa::RPi::InitParams params;\n> >\n> > - return ipa_->init(settings, !!sensor_->focusLens(), result);\n> > + params.lensPresent = !!sensor_->focusLens();\n> > + return ipa_->init(settings, params, result);\n> > }\n> >\n> > -int RPiCameraData::configureIPA(const CameraConfiguration *config, ipa::RPi::IPAConfigResult *result)\n> > +int RPiCameraData::configureIPA(const CameraConfiguration *config, ipa::RPi::ConfigResult *result)\n> > {\n> > std::map entityControls;\n> > - ipa::RPi::IPAConfig ipaConfig;\n> > + ipa::RPi::ConfigParams params;\n> >\n> > /* \\todo Move passing of ispControls and lensControls to ipa::init() */\n> > - ipaConfig.sensorControls = sensor_->controls();\n> > - ipaConfig.ispControls = isp_[Isp::Input].dev()->controls();\n> > + params.sensorControls = sensor_->controls();\n> > + params.ispControls = isp_[Isp::Input].dev()->controls();\n> > if (sensor_->focusLens())\n> > - ipaConfig.lensControls = sensor_->focusLens()->controls();\n> > + params.lensControls = sensor_->focusLens()->controls();\n> >\n> > /* Always send the user transform to the IPA. */\n> > - ipaConfig.transform = static_cast(config->transform);\n> > + params.transform = static_cast(config->transform);\n> >\n> > /* Allocate the lens shading table via dmaHeap and pass to the IPA. */\n> > if (!lsTable_.isValid()) {\n> > @@ -1703,7 +1705,7 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config, ipa::RPi::IPA\n> > * \\todo Investigate if mapping the lens shading table buffer\n> > * could be handled with mapBuffers().\n> > */\n> > - ipaConfig.lsTableHandle = lsTable_;\n> > + params.lsTableHandle = lsTable_;\n> > }\n> >\n> > /* We store the IPACameraSensorInfo for digital zoom calculations. */\n> > @@ -1714,15 +1716,14 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config, ipa::RPi::IPA\n> > }\n> >\n> > /* Ready the IPA - it must know about the sensor resolution. */\n> > - ControlList controls;\n> > - ret = ipa_->configure(sensorInfo_, ipaConfig, &controls, result);\n> > + ret = ipa_->configure(sensorInfo_, params, result);\n> > if (ret < 0) {\n> > LOG(RPI, Error) << \"IPA configuration failed!\";\n> > return -EPIPE;\n> > }\n> >\n> > - if (!controls.empty())\n> > - setSensorControls(controls);\n> > + if (!result->controls.empty())\n> > + setSensorControls(result->controls);\n> >\n> > return 0;\n> > }\n> > @@ -1883,24 +1884,32 @@ void RPiCameraData::enumerateVideoDevices(MediaLink *link)\n> > }\n> > }\n> >\n> > -void RPiCameraData::statsMetadataComplete(uint32_t bufferId, const ControlList &controls)\n> > +void RPiCameraData::processStatsComplete(const ipa::RPi::BufferIds &buffers)\n> > {\n> > if (!isRunning())\n> > return;\n> >\n> > - FrameBuffer *buffer = isp_[Isp::Stats].getBuffers().at(bufferId & RPi::MaskID);\n> > + FrameBuffer *buffer = isp_[Isp::Stats].getBuffers().at(buffers.stats & RPi::MaskID);\n> >\n> > handleStreamBuffer(buffer, &isp_[Isp::Stats]);\n> > + state_ = State::IpaComplete;\n> > + handleState();\n> > +}\n> > +\n> > +void RPiCameraData::metadataReady(const ControlList &metadata)\n> > +{\n> > + if (!isRunning())\n> > + return;\n> >\n> > /* Add to the Request metadata buffer what the IPA has provided. */\n> > Request *request = requestQueue_.front();\n> > - request->metadata().merge(controls);\n> > + request->metadata().merge(metadata);\n> >\n> > /*\n> > * Inform the sensor of the latest colour gains if it has the\n> > * V4L2_CID_NOTIFY_GAINS control (which means notifyGainsUnity_ is set).\n> > */\n> > - const auto &colourGains = controls.get(libcamera::controls::ColourGains);\n> > + const auto &colourGains = metadata.get(libcamera::controls::ColourGains);\n> > if (notifyGainsUnity_ && colourGains) {\n> > /* The control wants linear gains in the order B, Gb, Gr, R. */\n> > ControlList ctrls(sensor_->controls());\n> > @@ -1914,33 +1923,29 @@ void RPiCameraData::statsMetadataComplete(uint32_t bufferId, const ControlList &\n> >\n> > sensor_->setControls(&ctrls);\n> > }\n> > -\n> > - state_ = State::IpaComplete;\n> > - handleState();\n> > }\n> >\n> > -void RPiCameraData::runIsp(uint32_t bufferId)\n> > +void RPiCameraData::prepareIspComplete(const ipa::RPi::BufferIds &buffers)\n> > {\n> > + unsigned int embeddedId = buffers.embedded & RPi::MaskID;\n> > + unsigned int bayer = buffers.bayer & RPi::MaskID;\n> > + FrameBuffer *buffer;\n> > +\n> > if (!isRunning())\n> > return;\n> >\n> > - FrameBuffer *buffer = unicam_[Unicam::Image].getBuffers().at(bufferId & RPi::MaskID);\n> > -\n> > - LOG(RPI, Debug) << \"Input re-queue to ISP, buffer id \" << (bufferId & RPi::MaskID)\n> > + buffer = unicam_[Unicam::Image].getBuffers().at(bayer & RPi::MaskID);\n> > + LOG(RPI, Debug) << \"Input re-queue to ISP, buffer id \" << (bayer & RPi::MaskID)\n> > << \", timestamp: \" << buffer->metadata().timestamp;\n> >\n> > isp_[Isp::Input].queueBuffer(buffer);\n> > ispOutputCount_ = 0;\n> > - handleState();\n> > -}\n> >\n> > -void RPiCameraData::embeddedComplete(uint32_t bufferId)\n> > -{\n> > - if (!isRunning())\n> > - return;\n> > + if (sensorMetadata_ && embeddedId) {\n> > + buffer = unicam_[Unicam::Embedded].getBuffers().at(embeddedId & RPi::MaskID);\n> > + handleStreamBuffer(buffer, &unicam_[Unicam::Embedded]);\n> > + }\n> >\n> > - FrameBuffer *buffer = unicam_[Unicam::Embedded].getBuffers().at(bufferId & RPi::MaskID);\n> > - handleStreamBuffer(buffer, &unicam_[Unicam::Embedded]);\n> > handleState();\n> > }\n> >\n> > @@ -2116,8 +2121,10 @@ void RPiCameraData::ispOutputDequeue(FrameBuffer *buffer)\n> > * application until after the IPA signals so.\n> > */\n> > if (stream == &isp_[Isp::Stats]) {\n> > - ipa_->signalStatReady(RPi::MaskStats | static_cast(index),\n> > - requestQueue_.front()->sequence());\n> > + ipa::RPi::ProcessParams params;\n> > + params.buffers.stats = index | RPi::MaskStats;\n> > + params.ipaContext = requestQueue_.front()->sequence();\n> > + ipa_->processStats(params);\n> > } else {\n> > /* Any other ISP output can be handed back to the application now. */\n> > handleStreamBuffer(buffer, stream);\n> > @@ -2344,38 +2351,30 @@ void RPiCameraData::tryRunPipeline()\n> > request->metadata().clear();\n> > fillRequestMetadata(bayerFrame.controls, request);\n> >\n> > - /*\n> > - * Process all the user controls by the IPA. Once this is complete, we\n> > - * queue the ISP output buffer listed in the request to start the HW\n> > - * pipeline.\n> > - */\n> > - ipa_->signalQueueRequest(request->controls());\n> > -\n> > /* Set our state to say the pipeline is active. */\n> > state_ = State::Busy;\n> >\n> > - unsigned int bayerId = unicam_[Unicam::Image].getBufferId(bayerFrame.buffer);\n> > + unsigned int bayer = unicam_[Unicam::Image].getBufferId(bayerFrame.buffer);\n> >\n> > - LOG(RPI, Debug) << \"Signalling signalIspPrepare:\"\n> > - << \" Bayer buffer id: \" << bayerId;\n> > + LOG(RPI, Debug) << \"Signalling prepareIsp:\"\n> > + << \" Bayer buffer id: \" << bayer;\n> >\n> > - ipa::RPi::ISPConfig ispPrepare;\n> > - ispPrepare.bayerBufferId = RPi::MaskBayerData | bayerId;\n> > - ispPrepare.controls = std::move(bayerFrame.controls);\n> > - ispPrepare.ipaContext = request->sequence();\n> > - ispPrepare.delayContext = bayerFrame.delayContext;\n> > + ipa::RPi::PrepareParams params;\n> > + params.buffers.bayer = RPi::MaskBayerData | bayer;\n> > + params.sensorControls = std::move(bayerFrame.controls);\n> > + params.requestControls = request->controls();\n> > + params.ipaContext = request->sequence();\n> > + params.delayContext = bayerFrame.delayContext;\n> >\n> > if (embeddedBuffer) {\n> > unsigned int embeddedId = unicam_[Unicam::Embedded].getBufferId(embeddedBuffer);\n> >\n> > - ispPrepare.embeddedBufferId = RPi::MaskEmbeddedData | embeddedId;\n> > - ispPrepare.embeddedBufferPresent = true;\n> > -\n> > - LOG(RPI, Debug) << \"Signalling signalIspPrepare:\"\n> > + params.buffers.embedded = RPi::MaskEmbeddedData | embeddedId;\n> > + LOG(RPI, Debug) << \"Signalling prepareIsp:\"\n> > << \" Embedded buffer id: \" << embeddedId;\n> > }\n> >\n> > - ipa_->signalIspPrepare(ispPrepare);\n> > + ipa_->prepareIsp(params);\n> > }\n> >\n> > bool RPiCameraData::findMatchingBuffers(BayerFrame &bayerFrame, FrameBuffer *&embeddedBuffer)\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 92430C0DA4\n\tfor ;\n\tThu, 27 Apr 2023 09:53:39 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 14D92627D6;\n\tThu, 27 Apr 2023 11:53:39 +0200 (CEST)","from mail-yw1-x1129.google.com (mail-yw1-x1129.google.com\n\t[IPv6:2607:f8b0:4864:20::1129])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 70651627D1\n\tfor ;\n\tThu, 27 Apr 2023 11:53:36 +0200 (CEST)","by mail-yw1-x1129.google.com with SMTP id\n\t00721157ae682-54fb4c97d55so64499827b3.0\n\tfor ;\n\tThu, 27 Apr 2023 02:53:36 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1682589219;\n\tbh=17niR063Yr16BWL2vh0ZwMnqlgn1/d6UxF3D660Lqow=;\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=syYGuSEfO1Qbg41Iz3RZtcS7EOAYTNZVs7Aom6PR7bfmedu7fuE2r0qgQvQykS0wI\n\tJEmY0i0yuXCstIVK5HGQeQm90mL3cwftFZTGUTwUiGDAS7gGE7JyISJI0vvXNDz4RH\n\tz6vXoCOmqVA9EH2w1ZM82NHT3/lBvg5fNkwrY5FrwDTeUwKbpyJTCGtZoiI7l4FbDv\n\t6o6u9jYW0iAJVRdY8ka5evrhfj5kRCNGgZf9l2k+eM2XSmkYtwipNM5PuFTetPqKMh\n\tSQTKznZsYysmkkryey7mMlETM8Qlu9+9pXaghPiE49DaIbyvtZq8mGYT0SAiTQG0bz\n\tCbRJGGQl04FJw==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1682589215; x=1685181215;\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=ez/SruDMnnIXeQlslkcjjfJiMoHyo/5A52B2cCueO/Y=;\n\tb=Txlt3oBZXLV3B8yxa5ExohpNfl7MjFOBl8MWzBs6zorNXxvFqz90L9G3vQw46RNGLt\n\tpeMqMM9uvDw1SJhBW0JKYcVKYdycK/QPd20NnsTUg9jTBeRg//WRvIY8WxXgKjNZUlYz\n\tLVnLnPLat4p29XWCNezAWFmSpcGYA6j0iG7E35QDEd5nyxODsElReoWCTSNyfKWWeOkL\n\tE/ZnPCOrKH9RZRoFi9sq2vmkg4iBpD+uu5VCj6nhAyjNRGe95ggnsJ3ShYvu68TX4BzR\n\tAaDAKqGvYRuB+/tt3a6MdWNywjaIY8dVzH3EwNZiFUj6suByX/dAOHk72/MHl9JlNoKS\n\tbnrA=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"Txlt3oBZ\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20221208; t=1682589215; x=1685181215;\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=ez/SruDMnnIXeQlslkcjjfJiMoHyo/5A52B2cCueO/Y=;\n\tb=Lp0GD9y66s7YHSjkcef1/wUupv1C+hOpw6+Dtwvk0rksVtlz+en5hve0Pbtg2s5JqE\n\tUKG2V0TbkOxGN4SQs17P0QodzD3KsXBMr/yx1KwUQOEvTNTzxoL71Blgd64S9SPWa6Nh\n\theEoMkSwax9RqwrzP5LDTr3u28TP9X2OkynZUMWXQQCQK/aLJRGYPL4IE1m7IgHU5HII\n\tETfHCMtAKCQvhMWLynM4sB9sNscTgseQFEeUPiRt2InqrWSLZV8ZYDH2ALitu1/oLBUt\n\teDBQ+dL4tnFH14mwvOXBdrYoWZ5EgIVfbrWttJgOIpe0X2IDwAKHY1UZkbJtGIw5t6Ye\n\trd7g==","X-Gm-Message-State":"AC+VfDwf/+D4iJvy3FiVrZbjzpshv1pzFFdFl234bLUfmh8ozpoN6lC5\n\t331u5fdyMnk5Ho+a+rHusYnHI5oWJT4dTBWM0M3qeOHg32nRSwHEXdFIhQ==","X-Google-Smtp-Source":"ACHHUZ7uloiEuvB/E7yQrZYrpgYMuTZtD3dPAkuVszaTuNf+PDRW0RbD+jTYcJYXlFzUDvNiCmYFbPS/cn73B4luUio=","X-Received":"by 2002:a81:6c58:0:b0:552:9636:9f1f with SMTP id\n\th85-20020a816c58000000b0055296369f1fmr672510ywc.27.1682589214751;\n\tThu, 27 Apr 2023 02:53:34 -0700 (PDT)","MIME-Version":"1.0","References":"<20230426131057.21550-1-naush@raspberrypi.com>\n\t<20230426131057.21550-7-naush@raspberrypi.com>\n\t<26csbh5y5qh7r4rle34eyn3drneqvvifcqb65s5nppg5ifm6kv@ft5oy4xnkzu4>","In-Reply-To":"<26csbh5y5qh7r4rle34eyn3drneqvvifcqb65s5nppg5ifm6kv@ft5oy4xnkzu4>","Date":"Thu, 27 Apr 2023 10:53:25 +0100","Message-ID":"","To":"Jacopo Mondi ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH 06/13] pipeline: ipa: raspberrypi:\n\tRestructure the IPA mojom interface","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":26981,"web_url":"https://patchwork.libcamera.org/comment/26981/","msgid":"<20230427171521.GC26786@pendragon.ideasonboard.com>","date":"2023-04-27T17:15:21","subject":"Re: [libcamera-devel] [PATCH 06/13] pipeline: ipa: raspberrypi:\n\tRestructure the IPA mojom interface","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 Thu, Apr 27, 2023 at 10:53:25AM +0100, Naushir Patuck via libcamera-devel wrote:\n> On Thu, 27 Apr 2023 at 09:56, Jacopo Mondi wrote:\n> > On Wed, Apr 26, 2023 at 02:10:50PM +0100, Naushir Patuck via libcamera-devel wrote:\n> > > Restructure the IPA mojom interface to be more consistent in the use\n> > > of the API. Function parameters are now grouped into *Params structures\n> > > and results are now returned in *Results structures.\n> > >\n> > > The following pipeline -> IPA interfaces have been removed:\n> > >\n> > > signalQueueRequest(libcamera.ControlList controls);\n> > > signalIspPrepare(ISPConfig data);\n> > > signalStatReady(uint32 bufferId, uint32 ipaContext);\n> > >\n> > > and replaced with:\n> > >\n> > > prepareIsp(PrepareParams params);\n> > > processStats(ProcessParams params);\n> > >\n> > > signalQueueRequest() is now encompassed within signalPrepareIsp().\n\nI think you meant \"within prepareIsp()\" here.\n\n> > >\n> > > The following IPA -> pipeline interfaces have been removed:\n> > >\n> > > runIsp(uint32 bufferId);\n> > > embeddedComplete(uint32 bufferId);\n> > > statsMetadataComplete(uint32 bufferId, libcamera.ControlList controls);\n> > >\n> > > and replaced with the following async calls:\n> > >\n> > > prepareIspComplete(BufferIds buffers);\n> > > processStatsComplete(BufferIds buffers);\n> > > metadataReady(libcamera.ControlList metadata);\n> > >\n> > > Signed-off-by: Naushir Patuck \n> > > ---\n> > > include/libcamera/ipa/raspberrypi.mojom | 127 +++++---------\n> > > src/ipa/rpi/vc4/raspberrypi.cpp | 102 +++++-------\n> > > .../pipeline/rpi/vc4/raspberrypi.cpp | 155 +++++++++---------\n> > > 3 files changed, 165 insertions(+), 219 deletions(-)\n> > >\n> > > diff --git a/include/libcamera/ipa/raspberrypi.mojom b/include/libcamera/ipa/raspberrypi.mojom\n> > > index 80e0126618c8..7d56248f4ab3 100644\n> > > --- a/include/libcamera/ipa/raspberrypi.mojom\n> > > +++ b/include/libcamera/ipa/raspberrypi.mojom\n> > > @@ -8,7 +8,7 @@ module ipa.RPi;\n> > >\n> > > import \"include/libcamera/ipa/core.mojom\";\n> > >\n> > > -/* Size of the LS grid allocation. */\n> > > +/* Size of the LS grid allocation on VC4. */\n> > > const uint32 MaxLsGridSize = 0x8000;\n> > >\n> > > struct SensorConfig {\n> > > @@ -19,117 +19,78 @@ struct SensorConfig {\n> > > uint32 sensorMetadata;\n> > > };\n> > >\n> > > -struct IPAInitResult {\n> > > +struct InitParams {\n> > > + bool lensPresent;\n> > > +};\n> > > +\n> > > +struct InitResult {\n> > > SensorConfig sensorConfig;\n> > > libcamera.ControlInfoMap controlInfo;\n> > > };\n> > >\n> > > -struct ISPConfig {\n> > > - uint32 embeddedBufferId;\n> > > - uint32 bayerBufferId;\n> > > - bool embeddedBufferPresent;\n> > > - libcamera.ControlList controls;\n> > > - uint32 ipaContext;\n> > > - uint32 delayContext;\n> > > +struct BufferIds {\n> > > + uint32 bayer;\n> > > + uint32 embedded;\n> > > + uint32 stats;\n> > > };\n> > >\n> > > -struct IPAConfig {\n> > > +struct ConfigParams {\n> > > uint32 transform;\n> > > - libcamera.SharedFD lsTableHandle;\n> > > libcamera.ControlInfoMap sensorControls;\n> > > libcamera.ControlInfoMap ispControls;\n> > > libcamera.ControlInfoMap lensControls;\n> > > + /* VC4 specifc */\n> > > + libcamera.SharedFD lsTableHandle;\n> > > };\n> > >\n> > > -struct IPAConfigResult {\n> > > - float modeSensitivity;\n> > > - libcamera.ControlInfoMap controlInfo;\n> > > +struct ConfigResult {\n> > > + float modeSensitivity;\n> > > + libcamera.ControlInfoMap controlInfo;\n> > > + libcamera.ControlList controls;\n> > > };\n> > >\n> > > -struct StartConfig {\n> > > +struct StartResult {\n> > > libcamera.ControlList controls;\n> > > int32 dropFrameCount;\n> > > };\n> > >\n> > > +struct PrepareParams {\n> > > + BufferIds buffers;\n\nThe only part that bothers me a bit is usage of BufferIds here, as the\nstats buffer isn't used.\n\n> > > + libcamera.ControlList sensorControls;\n> > > + libcamera.ControlList requestControls;\n> > > + uint32 ipaContext;\n> > > + uint32 delayContext;\n> > > +};\n> > > +\n> > > +struct ProcessParams {\n> > > + BufferIds buffers;\n> > > + uint32 ipaContext;\n> > > +};\n> > > +\n> > > interface IPARPiInterface {\n> > > - init(libcamera.IPASettings settings, bool lensPresent)\n> > > - => (int32 ret, IPAInitResult result);\n> > > - start(libcamera.ControlList controls) => (StartConfig startConfig);\n> > > + init(libcamera.IPASettings settings, InitParams params)\n> > > + => (int32 ret, InitResult result);\n> > > +\n> > > + start(libcamera.ControlList controls) => (StartResult result);\n> > > stop();\n> > >\n> > > - /**\n> > > - * \\fn configure()\n> >\n> > Is it intentional to drop documentation ? Was it used at all ?\n> \n> I don't believe it is at all used. I looked at ipu3 and rkisp1 and they have\n> no doc tags either.\n\nIt's not mandatory, but it's nice to have. Could you keep it if that's\nnot too much trouble ?\n\n> > > - * \\brief Configure the IPA stream and sensor settings\n> > > - * \\param[in] sensorInfo Camera sensor information\n> > > - * \\param[in] ipaConfig Pipeline-handler-specific configuration data\n> > > - * \\param[out] controls Controls to apply by the pipeline entity\n> > > - * \\param[out] result Other results that the pipeline handler may require\n> > > - *\n> > > - * This function shall be called when the camera is configured to inform\n> > > - * the IPA of the camera's streams and the sensor settings.\n> > > - *\n> > > - * The \\a sensorInfo conveys information about the camera sensor settings that\n> > > - * the pipeline handler has selected for the configuration.\n> > > - *\n> > > - * The \\a ipaConfig and \\a controls parameters carry data passed by the\n> > > - * pipeline handler to the IPA and back.\n> > > - */\n> > > - configure(libcamera.IPACameraSensorInfo sensorInfo,\n> > > - IPAConfig ipaConfig)\n> > > - => (int32 ret, libcamera.ControlList controls, IPAConfigResult result);\n> > > -\n> > > - /**\n> > > - * \\fn mapBuffers()\n> > > - * \\brief Map buffers shared between the pipeline handler and the IPA\n> > > - * \\param[in] buffers List of buffers to map\n> > > - *\n> > > - * This function informs the IPA module of memory buffers set up by the\n> > > - * pipeline handler that the IPA needs to access. It provides dmabuf\n> > > - * file handles for each buffer, and associates the buffers with unique\n> > > - * numerical IDs.\n> > > - *\n> > > - * IPAs shall map the dmabuf file handles to their address space and\n> > > - * keep a cache of the mappings, indexed by the buffer numerical IDs.\n> > > - * The IDs are used in all other IPA interface functions to refer to\n> > > - * buffers, including the unmapBuffers() function.\n> > > - *\n> > > - * All buffers that the pipeline handler wishes to share with an IPA\n> > > - * shall be mapped with this function. Buffers may be mapped all at once\n> > > - * with a single call, or mapped and unmapped dynamically at runtime,\n> > > - * depending on the IPA protocol. Regardless of the protocol, all\n> > > - * buffers mapped at a given time shall have unique numerical IDs.\n> > > - *\n> > > - * The numerical IDs have no meaning defined by the IPA interface, and\n> > > - * should be treated as opaque handles by IPAs, with the only exception\n> > > - * that ID zero is invalid.\n> > > - *\n> > > - * \\sa unmapBuffers()\n> > > - */\n> > > - mapBuffers(array buffers);\n> > > + configure(libcamera.IPACameraSensorInfo sensorInfo, ConfigParams params)\n> > > + => (int32 ret, ConfigResult result);\n> > >\n> > > - /**\n> > > - * \\fn unmapBuffers()\n> > > - * \\brief Unmap buffers shared by the pipeline to the IPA\n> > > - * \\param[in] ids List of buffer IDs to unmap\n> > > - *\n> > > - * This function removes mappings set up with mapBuffers(). Numerical\n> > > - * IDs of unmapped buffers may be reused when mapping new buffers.\n> > > - *\n> > > - * \\sa mapBuffers()\n> > > - */\n> > > + mapBuffers(array buffers);\n> > > unmapBuffers(array ids);\n> > >\n> > > - [async] signalStatReady(uint32 bufferId, uint32 ipaContext);\n> > > - [async] signalQueueRequest(libcamera.ControlList controls);\n> > > - [async] signalIspPrepare(ISPConfig data);\n> > > + [async] prepareIsp(PrepareParams params);\n> > > + [async] processStats(ProcessParams params);\n> > > };\n> > >\n> > > interface IPARPiEventInterface {\n> > > - statsMetadataComplete(uint32 bufferId, libcamera.ControlList controls);\n> > > - runIsp(uint32 bufferId);\n> > > - embeddedComplete(uint32 bufferId);\n> > > + prepareIspComplete(BufferIds buffers);\n> > > + processStatsComplete(BufferIds buffers);\n> > > + metadataReady(libcamera.ControlList metadata);\n> > > setIspControls(libcamera.ControlList controls);\n> > > setDelayedControls(libcamera.ControlList controls, uint32 delayContext);\n> > > setLensControls(libcamera.ControlList controls);\n> > > setCameraTimeout(uint32 maxFrameLengthMs);\n> > > };\n> > > +\n> >\n> > Additional blank line\n> \n> Ack\n> \n> > The rest looks good\n> > Reviewed-by: Jacopo Mondi \n> >\n> > > diff --git a/src/ipa/rpi/vc4/raspberrypi.cpp b/src/ipa/rpi/vc4/raspberrypi.cpp\n> > > index 841635ccde2b..d737f1d662a0 100644\n> > > --- a/src/ipa/rpi/vc4/raspberrypi.cpp\n> > > +++ b/src/ipa/rpi/vc4/raspberrypi.cpp\n> > > @@ -136,30 +136,28 @@ public:\n> > > munmap(lsTable_, MaxLsGridSize);\n> > > }\n> > >\n> > > - int init(const IPASettings &settings, bool lensPresent, IPAInitResult *result) override;\n> > > - void start(const ControlList &controls, StartConfig *startConfig) override;\n> > > + int init(const IPASettings &settings, const InitParams ¶ms, InitResult *result) override;\n> > > + void start(const ControlList &controls, StartResult *result) override;\n> > > void stop() override {}\n> > >\n> > > - int configure(const IPACameraSensorInfo &sensorInfo, const IPAConfig &data,\n> > > - ControlList *controls, IPAConfigResult *result) override;\n> > > + int configure(const IPACameraSensorInfo &sensorInfo, const ConfigParams ¶ms,\n> > > + ConfigResult *result) override;\n> > > void mapBuffers(const std::vector &buffers) override;\n> > > void unmapBuffers(const std::vector &ids) override;\n> > > - void signalStatReady(const uint32_t bufferId, uint32_t ipaContext) override;\n> > > - void signalQueueRequest(const ControlList &controls) override;\n> > > - void signalIspPrepare(const ISPConfig &data) override;\n> > > + void prepareIsp(const PrepareParams ¶ms) override;\n> > > + void processStats(const ProcessParams ¶ms) override;\n> > >\n> > > private:\n> > > void setMode(const IPACameraSensorInfo &sensorInfo);\n> > > bool validateSensorControls();\n> > > bool validateIspControls();\n> > > bool validateLensControls();\n> > > - void queueRequest(const ControlList &controls);\n> > > - void returnEmbeddedBuffer(unsigned int bufferId);\n> > > - void prepareISP(const ISPConfig &data);\n> > > + void applyControls(const ControlList &controls);\n> > > + void prepare(const PrepareParams ¶ms);\n> > > void reportMetadata(unsigned int ipaContext);\n> > > void fillDeviceStatus(const ControlList &sensorControls, unsigned int ipaContext);\n> > > RPiController::StatisticsPtr fillStatistics(bcm2835_isp_stats *stats) const;\n> > > - void processStats(unsigned int bufferId, unsigned int ipaContext);\n> > > + void process(unsigned int bufferId, unsigned int ipaContext);\n> > > void setCameraTimeoutValue();\n> > > void applyFrameDurations(Duration minFrameDuration, Duration maxFrameDuration);\n> > > void applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls);\n> > > @@ -229,7 +227,7 @@ private:\n> > > Duration lastTimeout_;\n> > > };\n> > >\n> > > -int IPARPi::init(const IPASettings &settings, bool lensPresent, IPAInitResult *result)\n> > > +int IPARPi::init(const IPASettings &settings, const InitParams ¶ms, InitResult *result)\n> > > {\n> > > /*\n> > > * Load the \"helper\" for this sensor. This tells us all the device specific stuff\n> > > @@ -274,7 +272,7 @@ int IPARPi::init(const IPASettings &settings, bool lensPresent, IPAInitResult *r\n> > > return -EINVAL;\n> > > }\n> > >\n> > > - lensPresent_ = lensPresent;\n> > > + lensPresent_ = params.lensPresent;\n> > >\n> > > controller_.initialise();\n> > >\n> > > @@ -287,14 +285,13 @@ int IPARPi::init(const IPASettings &settings, bool lensPresent, IPAInitResult *r\n> > > return 0;\n> > > }\n> > >\n> > > -void IPARPi::start(const ControlList &controls, StartConfig *startConfig)\n> > > +void IPARPi::start(const ControlList &controls, StartResult *result)\n> > > {\n> > > RPiController::Metadata metadata;\n> > >\n> > > - ASSERT(startConfig);\n> > > if (!controls.empty()) {\n> > > /* We have been given some controls to action before start. */\n> > > - queueRequest(controls);\n> > > + applyControls(controls);\n> > > }\n> > >\n> > > controller_.switchMode(mode_, &metadata);\n> > > @@ -313,7 +310,7 @@ void IPARPi::start(const ControlList &controls, StartConfig *startConfig)\n> > > if (agcStatus.shutterTime && agcStatus.analogueGain) {\n> > > ControlList ctrls(sensorCtrls_);\n> > > applyAGC(&agcStatus, ctrls);\n> > > - startConfig->controls = std::move(ctrls);\n> > > + result->controls = std::move(ctrls);\n> > > setCameraTimeoutValue();\n> > > }\n> > >\n> > > @@ -360,7 +357,7 @@ void IPARPi::start(const ControlList &controls, StartConfig *startConfig)\n> > > mistrustCount_ = helper_->mistrustFramesModeSwitch();\n> > > }\n> > >\n> > > - startConfig->dropFrameCount = dropFrameCount_;\n> > > + result->dropFrameCount = dropFrameCount_;\n> > >\n> > > firstStart_ = false;\n> > > lastRunTimestamp_ = 0;\n> > > @@ -435,11 +432,11 @@ void IPARPi::setMode(const IPACameraSensorInfo &sensorInfo)\n> > > mode_.minFrameDuration, mode_.maxFrameDuration);\n> > > }\n> > >\n> > > -int IPARPi::configure(const IPACameraSensorInfo &sensorInfo, const IPAConfig &ipaConfig,\n> > > - ControlList *controls, IPAConfigResult *result)\n> > > +int IPARPi::configure(const IPACameraSensorInfo &sensorInfo, const ConfigParams ¶ms,\n> > > + ConfigResult *result)\n> > > {\n> > > - sensorCtrls_ = ipaConfig.sensorControls;\n> > > - ispCtrls_ = ipaConfig.ispControls;\n> > > + sensorCtrls_ = params.sensorControls;\n> > > + ispCtrls_ = params.ispControls;\n> > >\n> > > if (!validateSensorControls()) {\n> > > LOG(IPARPI, Error) << \"Sensor control validation failed.\";\n> > > @@ -452,7 +449,7 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo, const IPAConfig &ip\n> > > }\n> > >\n> > > if (lensPresent_) {\n> > > - lensCtrls_ = ipaConfig.lensControls;\n> > > + lensCtrls_ = params.lensControls;\n> > > if (!validateLensControls()) {\n> > > LOG(IPARPI, Warning) << \"Lens validation failed, \"\n> > > << \"no lens control will be available.\";\n> > > @@ -466,10 +463,10 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo, const IPAConfig &ip\n> > > /* Re-assemble camera mode using the sensor info. */\n> > > setMode(sensorInfo);\n> > >\n> > > - mode_.transform = static_cast(ipaConfig.transform);\n> > > + mode_.transform = static_cast(params.transform);\n> > >\n> > > /* Store the lens shading table pointer and handle if available. */\n> > > - if (ipaConfig.lsTableHandle.isValid()) {\n> > > + if (params.lsTableHandle.isValid()) {\n> > > /* Remove any previous table, if there was one. */\n> > > if (lsTable_) {\n> > > munmap(lsTable_, MaxLsGridSize);\n> > > @@ -477,7 +474,7 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo, const IPAConfig &ip\n> > > }\n> > >\n> > > /* Map the LS table buffer into user space. */\n> > > - lsTableHandle_ = std::move(ipaConfig.lsTableHandle);\n> > > + lsTableHandle_ = std::move(params.lsTableHandle);\n> > > if (lsTableHandle_.isValid()) {\n> > > lsTable_ = mmap(nullptr, MaxLsGridSize, PROT_READ | PROT_WRITE,\n> > > MAP_SHARED, lsTableHandle_.get(), 0);\n> > > @@ -512,8 +509,7 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo, const IPAConfig &ip\n> > > applyAGC(&agcStatus, ctrls);\n> > > }\n> > >\n> > > - ASSERT(controls);\n> > > - *controls = std::move(ctrls);\n> > > + result->controls = std::move(ctrls);\n> > >\n> > > /*\n> > > * Apply the correct limits to the exposure, gain and frame duration controls\n> > > @@ -560,37 +556,34 @@ void IPARPi::unmapBuffers(const std::vector &ids)\n> > > }\n> > > }\n> > >\n> > > -void IPARPi::signalStatReady(uint32_t bufferId, uint32_t ipaContext)\n> > > +void IPARPi::processStats(const ProcessParams ¶ms)\n> > > {\n> > > - unsigned int context = ipaContext % rpiMetadata_.size();\n> > > + unsigned int context = params.ipaContext % rpiMetadata_.size();\n> > >\n> > > if (++checkCount_ != frameCount_) /* assert here? */\n> > > LOG(IPARPI, Error) << \"WARNING: Prepare/Process mismatch!!!\";\n> > > if (processPending_ && frameCount_ > mistrustCount_)\n> > > - processStats(bufferId, context);\n> > > + process(params.buffers.stats, context);\n> > >\n> > > reportMetadata(context);\n> > > -\n> > > - statsMetadataComplete.emit(bufferId, libcameraMetadata_);\n> > > + processStatsComplete.emit(params.buffers);\n> > > }\n> > >\n> > > -void IPARPi::signalQueueRequest(const ControlList &controls)\n> > > -{\n> > > - queueRequest(controls);\n> > > -}\n> > >\n> > > -void IPARPi::signalIspPrepare(const ISPConfig &data)\n> > > +void IPARPi::prepareIsp(const PrepareParams ¶ms)\n> > > {\n> > > + applyControls(params.requestControls);\n> > > +\n> > > /*\n> > > * At start-up, or after a mode-switch, we may want to\n> > > * avoid running the control algos for a few frames in case\n> > > * they are \"unreliable\".\n> > > */\n> > > - prepareISP(data);\n> > > + prepare(params);\n> > > frameCount_++;\n> > >\n> > > /* Ready to push the input buffer into the ISP. */\n> > > - runIsp.emit(data.bayerBufferId);\n> > > + prepareIspComplete.emit(params.buffers);\n> > > }\n> > >\n> > > void IPARPi::reportMetadata(unsigned int ipaContext)\n> > > @@ -703,6 +696,8 @@ void IPARPi::reportMetadata(unsigned int ipaContext)\n> > > libcameraMetadata_.set(controls::AfState, s);\n> > > libcameraMetadata_.set(controls::AfPauseState, p);\n> > > }\n> > > +\n> > > + metadataReady.emit(libcameraMetadata_);\n> > > }\n> > >\n> > > bool IPARPi::validateSensorControls()\n> > > @@ -826,7 +821,7 @@ static const std::map AfPauseTable\n> > > { controls::AfPauseResume, RPiController::AfAlgorithm::AfPauseResume },\n> > > };\n> > >\n> > > -void IPARPi::queueRequest(const ControlList &controls)\n> > > +void IPARPi::applyControls(const ControlList &controls)\n> > > {\n> > > using RPiController::AfAlgorithm;\n> > >\n> > > @@ -1256,27 +1251,22 @@ void IPARPi::queueRequest(const ControlList &controls)\n> > > }\n> > > }\n> > >\n> > > -void IPARPi::returnEmbeddedBuffer(unsigned int bufferId)\n> > > +void IPARPi::prepare(const PrepareParams ¶ms)\n> > > {\n> > > - embeddedComplete.emit(bufferId);\n> > > -}\n> > > -\n> > > -void IPARPi::prepareISP(const ISPConfig &data)\n> > > -{\n> > > - int64_t frameTimestamp = data.controls.get(controls::SensorTimestamp).value_or(0);\n> > > - unsigned int ipaContext = data.ipaContext % rpiMetadata_.size();\n> > > + int64_t frameTimestamp = params.sensorControls.get(controls::SensorTimestamp).value_or(0);\n> > > + unsigned int ipaContext = params.ipaContext % rpiMetadata_.size();\n> > > RPiController::Metadata &rpiMetadata = rpiMetadata_[ipaContext];\n> > > Span embeddedBuffer;\n> > >\n> > > rpiMetadata.clear();\n> > > - fillDeviceStatus(data.controls, ipaContext);\n> > > + fillDeviceStatus(params.sensorControls, ipaContext);\n> > >\n> > > - if (data.embeddedBufferPresent) {\n> > > + if (params.buffers.embedded) {\n> > > /*\n> > > * Pipeline handler has supplied us with an embedded data buffer,\n> > > * we must pass it to the CamHelper for parsing.\n> > > */\n> > > - auto it = buffers_.find(data.embeddedBufferId);\n> > > + auto it = buffers_.find(params.buffers.embedded);\n> > > ASSERT(it != buffers_.end());\n> > > embeddedBuffer = it->second.planes()[0];\n> > > }\n> > > @@ -1288,7 +1278,7 @@ void IPARPi::prepareISP(const ISPConfig &data)\n> > > * metadata.\n> > > */\n> > > AgcStatus agcStatus;\n> > > - RPiController::Metadata &delayedMetadata = rpiMetadata_[data.delayContext];\n> > > + RPiController::Metadata &delayedMetadata = rpiMetadata_[params.delayContext];\n> > > if (!delayedMetadata.get(\"agc.status\", agcStatus))\n> > > rpiMetadata.set(\"agc.delayed_status\", agcStatus);\n> > >\n> > > @@ -1298,10 +1288,6 @@ void IPARPi::prepareISP(const ISPConfig &data)\n> > > */\n> > > helper_->prepare(embeddedBuffer, rpiMetadata);\n> > >\n> > > - /* Done with embedded data now, return to pipeline handler asap. */\n> > > - if (data.embeddedBufferPresent)\n> > > - returnEmbeddedBuffer(data.embeddedBufferId);\n> > > -\n> > > /* Allow a 10% margin on the comparison below. */\n> > > Duration delta = (frameTimestamp - lastRunTimestamp_) * 1.0ns;\n> > > if (lastRunTimestamp_ && frameCount_ > dropFrameCount_ &&\n> > > @@ -1445,7 +1431,7 @@ RPiController::StatisticsPtr IPARPi::fillStatistics(bcm2835_isp_stats *stats) co\n> > > return statistics;\n> > > }\n> > >\n> > > -void IPARPi::processStats(unsigned int bufferId, unsigned int ipaContext)\n> > > +void IPARPi::process(unsigned int bufferId, unsigned int ipaContext)\n> > > {\n> > > RPiController::Metadata &rpiMetadata = rpiMetadata_[ipaContext];\n> > >\n> > > diff --git a/src/libcamera/pipeline/rpi/vc4/raspberrypi.cpp b/src/libcamera/pipeline/rpi/vc4/raspberrypi.cpp\n> > > index e54bf1ef2c17..30ce6feab0d1 100644\n> > > --- a/src/libcamera/pipeline/rpi/vc4/raspberrypi.cpp\n> > > +++ b/src/libcamera/pipeline/rpi/vc4/raspberrypi.cpp\n> > > @@ -200,15 +200,15 @@ public:\n> > > void freeBuffers();\n> > > void frameStarted(uint32_t sequence);\n> > >\n> > > - int loadIPA(ipa::RPi::IPAInitResult *result);\n> > > - int configureIPA(const CameraConfiguration *config, ipa::RPi::IPAConfigResult *result);\n> > > + int loadIPA(ipa::RPi::InitResult *result);\n> > > + int configureIPA(const CameraConfiguration *config, ipa::RPi::ConfigResult *result);\n> > > int loadPipelineConfiguration();\n> > >\n> > > void enumerateVideoDevices(MediaLink *link);\n> > >\n> > > - void statsMetadataComplete(uint32_t bufferId, const ControlList &controls);\n> > > - void runIsp(uint32_t bufferId);\n> > > - void embeddedComplete(uint32_t bufferId);\n> > > + void processStatsComplete(const ipa::RPi::BufferIds &buffers);\n> > > + void metadataReady(const ControlList &metadata);\n> > > + void prepareIspComplete(const ipa::RPi::BufferIds &buffers);\n> > > void setIspControls(const ControlList &controls);\n> > > void setDelayedControls(const ControlList &controls, uint32_t delayContext);\n> > > void setLensControls(const ControlList &controls);\n> > > @@ -238,7 +238,7 @@ public:\n> > > /* The vector below is just for convenience when iterating over all streams. */\n> > > std::vector streams_;\n> > > /* Stores the ids of the buffers mapped in the IPA. */\n> > > - std::unordered_set ipaBuffers_;\n> > > + std::unordered_set bufferIds_;\n> > > /*\n> > > * Stores a cascade of Video Mux or Bridge devices between the sensor and\n> > > * Unicam together with media link across the entities.\n> > > @@ -1000,7 +1000,7 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)\n> > >\n> > > data->isp_[Isp::Input].dev()->setSelection(V4L2_SEL_TGT_CROP, &crop);\n> > >\n> > > - ipa::RPi::IPAConfigResult result;\n> > > + ipa::RPi::ConfigResult result;\n> > > ret = data->configureIPA(config, &result);\n> > > if (ret)\n> > > LOG(RPI, Error) << \"Failed to configure the IPA: \" << ret;\n> > > @@ -1117,17 +1117,17 @@ int PipelineHandlerRPi::start(Camera *camera, const ControlList *controls)\n> > > data->applyScalerCrop(*controls);\n> > >\n> > > /* Start the IPA. */\n> > > - ipa::RPi::StartConfig startConfig;\n> > > + ipa::RPi::StartResult result;\n> > > data->ipa_->start(controls ? *controls : ControlList{ controls::controls },\n> > > - &startConfig);\n> > > + &result);\n> > >\n> > > /* Apply any gain/exposure settings that the IPA may have passed back. */\n> > > - if (!startConfig.controls.empty())\n> > > - data->setSensorControls(startConfig.controls);\n> > > + if (!result.controls.empty())\n> > > + data->setSensorControls(result.controls);\n> > >\n> > > /* Configure the number of dropped frames required on startup. */\n> > > data->dropFrameCount_ = data->config_.disableStartupFrameDrops\n> > > - ? 0 : startConfig.dropFrameCount;\n> > > + ? 0 : result.dropFrameCount;\n> > >\n> > > for (auto const stream : data->streams_)\n> > > stream->resetBuffers();\n> > > @@ -1358,7 +1358,7 @@ int PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp, Me\n> > >\n> > > data->sensorFormats_ = populateSensorFormats(data->sensor_);\n> > >\n> > > - ipa::RPi::IPAInitResult result;\n> > > + ipa::RPi::InitResult result;\n> > > if (data->loadIPA(&result)) {\n> > > LOG(RPI, Error) << \"Failed to load a suitable IPA library\";\n> > > return -EINVAL;\n> > > @@ -1599,7 +1599,7 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)\n> > > void PipelineHandlerRPi::mapBuffers(Camera *camera, const RPi::BufferMap &buffers, unsigned int mask)\n> > > {\n> > > RPiCameraData *data = cameraData(camera);\n> > > - std::vector ipaBuffers;\n> > > + std::vector bufferIds;\n> > > /*\n> > > * Link the FrameBuffers with the id (key value) in the map stored in\n> > > * the RPi stream object - along with an identifier mask.\n> > > @@ -1608,12 +1608,12 @@ void PipelineHandlerRPi::mapBuffers(Camera *camera, const RPi::BufferMap &buffer\n> > > * handler and the IPA.\n> > > */\n> > > for (auto const &it : buffers) {\n> > > - ipaBuffers.push_back(IPABuffer(mask | it.first,\n> > > + bufferIds.push_back(IPABuffer(mask | it.first,\n> > > it.second->planes()));\n> > > - data->ipaBuffers_.insert(mask | it.first);\n> > > + data->bufferIds_.insert(mask | it.first);\n> > > }\n> > >\n> > > - data->ipa_->mapBuffers(ipaBuffers);\n> > > + data->ipa_->mapBuffers(bufferIds);\n> > > }\n> > >\n> > > void RPiCameraData::freeBuffers()\n> > > @@ -1623,10 +1623,10 @@ void RPiCameraData::freeBuffers()\n> > > * Copy the buffer ids from the unordered_set to a vector to\n> > > * pass to the IPA.\n> > > */\n> > > - std::vector ipaBuffers(ipaBuffers_.begin(),\n> > > - ipaBuffers_.end());\n> > > - ipa_->unmapBuffers(ipaBuffers);\n> > > - ipaBuffers_.clear();\n> > > + std::vector bufferIds(bufferIds_.begin(),\n> > > + bufferIds_.end());\n> > > + ipa_->unmapBuffers(bufferIds);\n> > > + bufferIds_.clear();\n> > > }\n> > >\n> > > for (auto const stream : streams_)\n> > > @@ -1643,16 +1643,16 @@ void RPiCameraData::frameStarted(uint32_t sequence)\n> > > delayedCtrls_->applyControls(sequence);\n> > > }\n> > >\n> > > -int RPiCameraData::loadIPA(ipa::RPi::IPAInitResult *result)\n> > > +int RPiCameraData::loadIPA(ipa::RPi::InitResult *result)\n> > > {\n> > > ipa_ = IPAManager::createIPA(pipe(), 1, 1);\n> > >\n> > > if (!ipa_)\n> > > return -ENOENT;\n> > >\n> > > - ipa_->statsMetadataComplete.connect(this, &RPiCameraData::statsMetadataComplete);\n> > > - ipa_->runIsp.connect(this, &RPiCameraData::runIsp);\n> > > - ipa_->embeddedComplete.connect(this, &RPiCameraData::embeddedComplete);\n> > > + ipa_->processStatsComplete.connect(this, &RPiCameraData::processStatsComplete);\n> > > + ipa_->prepareIspComplete.connect(this, &RPiCameraData::prepareIspComplete);\n> > > + ipa_->metadataReady.connect(this, &RPiCameraData::metadataReady);\n> > > ipa_->setIspControls.connect(this, &RPiCameraData::setIspControls);\n> > > ipa_->setDelayedControls.connect(this, &RPiCameraData::setDelayedControls);\n> > > ipa_->setLensControls.connect(this, &RPiCameraData::setLensControls);\n> > > @@ -1674,23 +1674,25 @@ int RPiCameraData::loadIPA(ipa::RPi::IPAInitResult *result)\n> > > }\n> > >\n> > > IPASettings settings(configurationFile, sensor_->model());\n> > > + ipa::RPi::InitParams params;\n> > >\n> > > - return ipa_->init(settings, !!sensor_->focusLens(), result);\n> > > + params.lensPresent = !!sensor_->focusLens();\n> > > + return ipa_->init(settings, params, result);\n> > > }\n> > >\n> > > -int RPiCameraData::configureIPA(const CameraConfiguration *config, ipa::RPi::IPAConfigResult *result)\n> > > +int RPiCameraData::configureIPA(const CameraConfiguration *config, ipa::RPi::ConfigResult *result)\n> > > {\n> > > std::map entityControls;\n> > > - ipa::RPi::IPAConfig ipaConfig;\n> > > + ipa::RPi::ConfigParams params;\n> > >\n> > > /* \\todo Move passing of ispControls and lensControls to ipa::init() */\n> > > - ipaConfig.sensorControls = sensor_->controls();\n> > > - ipaConfig.ispControls = isp_[Isp::Input].dev()->controls();\n> > > + params.sensorControls = sensor_->controls();\n> > > + params.ispControls = isp_[Isp::Input].dev()->controls();\n> > > if (sensor_->focusLens())\n> > > - ipaConfig.lensControls = sensor_->focusLens()->controls();\n> > > + params.lensControls = sensor_->focusLens()->controls();\n> > >\n> > > /* Always send the user transform to the IPA. */\n> > > - ipaConfig.transform = static_cast(config->transform);\n> > > + params.transform = static_cast(config->transform);\n> > >\n> > > /* Allocate the lens shading table via dmaHeap and pass to the IPA. */\n> > > if (!lsTable_.isValid()) {\n> > > @@ -1703,7 +1705,7 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config, ipa::RPi::IPA\n> > > * \\todo Investigate if mapping the lens shading table buffer\n> > > * could be handled with mapBuffers().\n> > > */\n> > > - ipaConfig.lsTableHandle = lsTable_;\n> > > + params.lsTableHandle = lsTable_;\n> > > }\n> > >\n> > > /* We store the IPACameraSensorInfo for digital zoom calculations. */\n> > > @@ -1714,15 +1716,14 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config, ipa::RPi::IPA\n> > > }\n> > >\n> > > /* Ready the IPA - it must know about the sensor resolution. */\n> > > - ControlList controls;\n> > > - ret = ipa_->configure(sensorInfo_, ipaConfig, &controls, result);\n> > > + ret = ipa_->configure(sensorInfo_, params, result);\n> > > if (ret < 0) {\n> > > LOG(RPI, Error) << \"IPA configuration failed!\";\n> > > return -EPIPE;\n> > > }\n> > >\n> > > - if (!controls.empty())\n> > > - setSensorControls(controls);\n> > > + if (!result->controls.empty())\n> > > + setSensorControls(result->controls);\n> > >\n> > > return 0;\n> > > }\n> > > @@ -1883,24 +1884,32 @@ void RPiCameraData::enumerateVideoDevices(MediaLink *link)\n> > > }\n> > > }\n> > >\n> > > -void RPiCameraData::statsMetadataComplete(uint32_t bufferId, const ControlList &controls)\n> > > +void RPiCameraData::processStatsComplete(const ipa::RPi::BufferIds &buffers)\n> > > {\n> > > if (!isRunning())\n> > > return;\n> > >\n> > > - FrameBuffer *buffer = isp_[Isp::Stats].getBuffers().at(bufferId & RPi::MaskID);\n> > > + FrameBuffer *buffer = isp_[Isp::Stats].getBuffers().at(buffers.stats & RPi::MaskID);\n> > >\n> > > handleStreamBuffer(buffer, &isp_[Isp::Stats]);\n> > > + state_ = State::IpaComplete;\n> > > + handleState();\n> > > +}\n> > > +\n> > > +void RPiCameraData::metadataReady(const ControlList &metadata)\n> > > +{\n> > > + if (!isRunning())\n> > > + return;\n> > >\n> > > /* Add to the Request metadata buffer what the IPA has provided. */\n> > > Request *request = requestQueue_.front();\n> > > - request->metadata().merge(controls);\n> > > + request->metadata().merge(metadata);\n> > >\n> > > /*\n> > > * Inform the sensor of the latest colour gains if it has the\n> > > * V4L2_CID_NOTIFY_GAINS control (which means notifyGainsUnity_ is set).\n> > > */\n> > > - const auto &colourGains = controls.get(libcamera::controls::ColourGains);\n> > > + const auto &colourGains = metadata.get(libcamera::controls::ColourGains);\n> > > if (notifyGainsUnity_ && colourGains) {\n> > > /* The control wants linear gains in the order B, Gb, Gr, R. */\n> > > ControlList ctrls(sensor_->controls());\n> > > @@ -1914,33 +1923,29 @@ void RPiCameraData::statsMetadataComplete(uint32_t bufferId, const ControlList &\n> > >\n> > > sensor_->setControls(&ctrls);\n> > > }\n> > > -\n> > > - state_ = State::IpaComplete;\n> > > - handleState();\n> > > }\n> > >\n> > > -void RPiCameraData::runIsp(uint32_t bufferId)\n> > > +void RPiCameraData::prepareIspComplete(const ipa::RPi::BufferIds &buffers)\n> > > {\n> > > + unsigned int embeddedId = buffers.embedded & RPi::MaskID;\n> > > + unsigned int bayer = buffers.bayer & RPi::MaskID;\n> > > + FrameBuffer *buffer;\n> > > +\n> > > if (!isRunning())\n> > > return;\n> > >\n> > > - FrameBuffer *buffer = unicam_[Unicam::Image].getBuffers().at(bufferId & RPi::MaskID);\n> > > -\n> > > - LOG(RPI, Debug) << \"Input re-queue to ISP, buffer id \" << (bufferId & RPi::MaskID)\n> > > + buffer = unicam_[Unicam::Image].getBuffers().at(bayer & RPi::MaskID);\n> > > + LOG(RPI, Debug) << \"Input re-queue to ISP, buffer id \" << (bayer & RPi::MaskID)\n> > > << \", timestamp: \" << buffer->metadata().timestamp;\n> > >\n> > > isp_[Isp::Input].queueBuffer(buffer);\n> > > ispOutputCount_ = 0;\n> > > - handleState();\n> > > -}\n> > >\n> > > -void RPiCameraData::embeddedComplete(uint32_t bufferId)\n> > > -{\n> > > - if (!isRunning())\n> > > - return;\n> > > + if (sensorMetadata_ && embeddedId) {\n> > > + buffer = unicam_[Unicam::Embedded].getBuffers().at(embeddedId & RPi::MaskID);\n> > > + handleStreamBuffer(buffer, &unicam_[Unicam::Embedded]);\n> > > + }\n> > >\n> > > - FrameBuffer *buffer = unicam_[Unicam::Embedded].getBuffers().at(bufferId & RPi::MaskID);\n> > > - handleStreamBuffer(buffer, &unicam_[Unicam::Embedded]);\n> > > handleState();\n> > > }\n> > >\n> > > @@ -2116,8 +2121,10 @@ void RPiCameraData::ispOutputDequeue(FrameBuffer *buffer)\n> > > * application until after the IPA signals so.\n> > > */\n> > > if (stream == &isp_[Isp::Stats]) {\n> > > - ipa_->signalStatReady(RPi::MaskStats | static_cast(index),\n> > > - requestQueue_.front()->sequence());\n> > > + ipa::RPi::ProcessParams params;\n> > > + params.buffers.stats = index | RPi::MaskStats;\n> > > + params.ipaContext = requestQueue_.front()->sequence();\n> > > + ipa_->processStats(params);\n> > > } else {\n> > > /* Any other ISP output can be handed back to the application now. */\n> > > handleStreamBuffer(buffer, stream);\n> > > @@ -2344,38 +2351,30 @@ void RPiCameraData::tryRunPipeline()\n> > > request->metadata().clear();\n> > > fillRequestMetadata(bayerFrame.controls, request);\n> > >\n> > > - /*\n> > > - * Process all the user controls by the IPA. Once this is complete, we\n> > > - * queue the ISP output buffer listed in the request to start the HW\n> > > - * pipeline.\n> > > - */\n> > > - ipa_->signalQueueRequest(request->controls());\n> > > -\n> > > /* Set our state to say the pipeline is active. */\n> > > state_ = State::Busy;\n> > >\n> > > - unsigned int bayerId = unicam_[Unicam::Image].getBufferId(bayerFrame.buffer);\n> > > + unsigned int bayer = unicam_[Unicam::Image].getBufferId(bayerFrame.buffer);\n> > >\n> > > - LOG(RPI, Debug) << \"Signalling signalIspPrepare:\"\n> > > - << \" Bayer buffer id: \" << bayerId;\n> > > + LOG(RPI, Debug) << \"Signalling prepareIsp:\"\n> > > + << \" Bayer buffer id: \" << bayer;\n> > >\n> > > - ipa::RPi::ISPConfig ispPrepare;\n> > > - ispPrepare.bayerBufferId = RPi::MaskBayerData | bayerId;\n> > > - ispPrepare.controls = std::move(bayerFrame.controls);\n> > > - ispPrepare.ipaContext = request->sequence();\n> > > - ispPrepare.delayContext = bayerFrame.delayContext;\n> > > + ipa::RPi::PrepareParams params;\n> > > + params.buffers.bayer = RPi::MaskBayerData | bayer;\n> > > + params.sensorControls = std::move(bayerFrame.controls);\n> > > + params.requestControls = request->controls();\n> > > + params.ipaContext = request->sequence();\n> > > + params.delayContext = bayerFrame.delayContext;\n> > >\n> > > if (embeddedBuffer) {\n> > > unsigned int embeddedId = unicam_[Unicam::Embedded].getBufferId(embeddedBuffer);\n> > >\n> > > - ispPrepare.embeddedBufferId = RPi::MaskEmbeddedData | embeddedId;\n> > > - ispPrepare.embeddedBufferPresent = true;\n> > > -\n> > > - LOG(RPI, Debug) << \"Signalling signalIspPrepare:\"\n> > > + params.buffers.embedded = RPi::MaskEmbeddedData | embeddedId;\n> > > + LOG(RPI, Debug) << \"Signalling prepareIsp:\"\n> > > << \" Embedded buffer id: \" << embeddedId;\n> > > }\n> > >\n> > > - ipa_->signalIspPrepare(ispPrepare);\n> > > + ipa_->prepareIsp(params);\n> > > }\n> > >\n> > > bool RPiCameraData::findMatchingBuffers(BayerFrame &bayerFrame, FrameBuffer *&embeddedBuffer)","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 D3121BDCBD\n\tfor ;\n\tThu, 27 Apr 2023 17:15:12 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 468DE627DF;\n\tThu, 27 Apr 2023 19:15:12 +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 ABAC6627B7\n\tfor ;\n\tThu, 27 Apr 2023 19:15:10 +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 2CF5FC7E;\n\tThu, 27 Apr 2023 19:14:56 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1682615712;\n\tbh=VKtbGZZw9zlEJAXm13F1YGaKJTpLux48E7N5I+56YRo=;\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=bsa2JwthcKAj2YWPgLQz3M+Cyvix5IVRpTOk0P9MaMoBMoG1PK+7DeqDOyzp2KIth\n\tsFvC/wGLcxmZM23CycOUiF49Z2rme2FME+BnXygVNHauo9BhKQhFfJ4kZbou1RKj8Q\n\tAYwfpCyIWT6f+obE4kZY4/2DTAEXbCTL3/9CN/XBhMucRUJmiBjb0cqwe5ZvdBbxO4\n\tulpEhD0pzReBG5f3u4LqxHDF6ljoypp8QaeG2FoYA1hRGJMYEpkMS85sYI90LAgz3D\n\tmVjJaktyfI7iGsI5GAPORCiFNZL6G8bm7eMZh++mz5vjMDiX9zULV4nR9mysIEmLFb\n\tajWT7zeIBGsYw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1682615698;\n\tbh=VKtbGZZw9zlEJAXm13F1YGaKJTpLux48E7N5I+56YRo=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Ap7dF6ujiMn5UJwCNXZqJCpbhUlvw2xPv4fpWYHen7+6MJ/nltcsGDZGGuha2jpPz\n\tmw2RkRwyg0di6Uvrq3qIs6Jf1jSw+vvdWnvvt39HN6dJ/yLQTnQvs0U9sSPYRyCQOu\n\tI99egaoxsuDe5YBamYbEQX5xHvxtW5Vb0cFU/bWQ="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"Ap7dF6uj\"; dkim-atps=neutral","Date":"Thu, 27 Apr 2023 20:15:21 +0300","To":"Naushir Patuck ","Message-ID":"<20230427171521.GC26786@pendragon.ideasonboard.com>","References":"<20230426131057.21550-1-naush@raspberrypi.com>\n\t<20230426131057.21550-7-naush@raspberrypi.com>\n\t<26csbh5y5qh7r4rle34eyn3drneqvvifcqb65s5nppg5ifm6kv@ft5oy4xnkzu4>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 06/13] pipeline: ipa: raspberrypi:\n\tRestructure the IPA mojom interface","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":26989,"web_url":"https://patchwork.libcamera.org/comment/26989/","msgid":"","date":"2023-04-28T09:43:23","subject":"Re: [libcamera-devel] [PATCH 06/13] pipeline: ipa: raspberrypi:\n\tRestructure the IPA mojom interface","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Laurent,\n\nOn Thu, 27 Apr 2023 at 18:15, Laurent Pinchart\n wrote:\n>\n> Hi Naush,\n>\n> Thank you for the patch.\n>\n> On Thu, Apr 27, 2023 at 10:53:25AM +0100, Naushir Patuck via libcamera-devel wrote:\n> > On Thu, 27 Apr 2023 at 09:56, Jacopo Mondi wrote:\n> > > On Wed, Apr 26, 2023 at 02:10:50PM +0100, Naushir Patuck via libcamera-devel wrote:\n> > > > Restructure the IPA mojom interface to be more consistent in the use\n> > > > of the API. Function parameters are now grouped into *Params structures\n> > > > and results are now returned in *Results structures.\n> > > >\n> > > > The following pipeline -> IPA interfaces have been removed:\n> > > >\n> > > > signalQueueRequest(libcamera.ControlList controls);\n> > > > signalIspPrepare(ISPConfig data);\n> > > > signalStatReady(uint32 bufferId, uint32 ipaContext);\n> > > >\n> > > > and replaced with:\n> > > >\n> > > > prepareIsp(PrepareParams params);\n> > > > processStats(ProcessParams params);\n> > > >\n> > > > signalQueueRequest() is now encompassed within signalPrepareIsp().\n>\n> I think you meant \"within prepareIsp()\" here.\n>\n> > > >\n> > > > The following IPA -> pipeline interfaces have been removed:\n> > > >\n> > > > runIsp(uint32 bufferId);\n> > > > embeddedComplete(uint32 bufferId);\n> > > > statsMetadataComplete(uint32 bufferId, libcamera.ControlList controls);\n> > > >\n> > > > and replaced with the following async calls:\n> > > >\n> > > > prepareIspComplete(BufferIds buffers);\n> > > > processStatsComplete(BufferIds buffers);\n> > > > metadataReady(libcamera.ControlList metadata);\n> > > >\n> > > > Signed-off-by: Naushir Patuck \n> > > > ---\n> > > > include/libcamera/ipa/raspberrypi.mojom | 127 +++++---------\n> > > > src/ipa/rpi/vc4/raspberrypi.cpp | 102 +++++-------\n> > > > .../pipeline/rpi/vc4/raspberrypi.cpp | 155 +++++++++---------\n> > > > 3 files changed, 165 insertions(+), 219 deletions(-)\n> > > >\n> > > > diff --git a/include/libcamera/ipa/raspberrypi.mojom b/include/libcamera/ipa/raspberrypi.mojom\n> > > > index 80e0126618c8..7d56248f4ab3 100644\n> > > > --- a/include/libcamera/ipa/raspberrypi.mojom\n> > > > +++ b/include/libcamera/ipa/raspberrypi.mojom\n> > > > @@ -8,7 +8,7 @@ module ipa.RPi;\n> > > >\n> > > > import \"include/libcamera/ipa/core.mojom\";\n> > > >\n> > > > -/* Size of the LS grid allocation. */\n> > > > +/* Size of the LS grid allocation on VC4. */\n> > > > const uint32 MaxLsGridSize = 0x8000;\n> > > >\n> > > > struct SensorConfig {\n> > > > @@ -19,117 +19,78 @@ struct SensorConfig {\n> > > > uint32 sensorMetadata;\n> > > > };\n> > > >\n> > > > -struct IPAInitResult {\n> > > > +struct InitParams {\n> > > > + bool lensPresent;\n> > > > +};\n> > > > +\n> > > > +struct InitResult {\n> > > > SensorConfig sensorConfig;\n> > > > libcamera.ControlInfoMap controlInfo;\n> > > > };\n> > > >\n> > > > -struct ISPConfig {\n> > > > - uint32 embeddedBufferId;\n> > > > - uint32 bayerBufferId;\n> > > > - bool embeddedBufferPresent;\n> > > > - libcamera.ControlList controls;\n> > > > - uint32 ipaContext;\n> > > > - uint32 delayContext;\n> > > > +struct BufferIds {\n> > > > + uint32 bayer;\n> > > > + uint32 embedded;\n> > > > + uint32 stats;\n> > > > };\n> > > >\n> > > > -struct IPAConfig {\n> > > > +struct ConfigParams {\n> > > > uint32 transform;\n> > > > - libcamera.SharedFD lsTableHandle;\n> > > > libcamera.ControlInfoMap sensorControls;\n> > > > libcamera.ControlInfoMap ispControls;\n> > > > libcamera.ControlInfoMap lensControls;\n> > > > + /* VC4 specifc */\n> > > > + libcamera.SharedFD lsTableHandle;\n> > > > };\n> > > >\n> > > > -struct IPAConfigResult {\n> > > > - float modeSensitivity;\n> > > > - libcamera.ControlInfoMap controlInfo;\n> > > > +struct ConfigResult {\n> > > > + float modeSensitivity;\n> > > > + libcamera.ControlInfoMap controlInfo;\n> > > > + libcamera.ControlList controls;\n> > > > };\n> > > >\n> > > > -struct StartConfig {\n> > > > +struct StartResult {\n> > > > libcamera.ControlList controls;\n> > > > int32 dropFrameCount;\n> > > > };\n> > > >\n> > > > +struct PrepareParams {\n> > > > + BufferIds buffers;\n>\n> The only part that bothers me a bit is usage of BufferIds here, as the\n> stats buffer isn't used.\n\nAdding the stats buffer to BufferIds allows me to generalise the structure and\nuse it throughout the interface.\n\nIt is also sort-of ' used right now, in that we see the absence of the stats\nbuffer to \"know\" that the stats are not in-line with prepareISP. However, this\nis not intended to stay that way, we will have a separate parameter to signal\nthis if needed.\n\nRegards,\nNaush\n\n>\n> > > > + libcamera.ControlList sensorControls;\n> > > > + libcamera.ControlList requestControls;\n> > > > + uint32 ipaContext;\n> > > > + uint32 delayContext;\n> > > > +};\n> > > > +\n> > > > +struct ProcessParams {\n> > > > + BufferIds buffers;\n> > > > + uint32 ipaContext;\n> > > > +};\n> > > > +\n> > > > interface IPARPiInterface {\n> > > > - init(libcamera.IPASettings settings, bool lensPresent)\n> > > > - => (int32 ret, IPAInitResult result);\n> > > > - start(libcamera.ControlList controls) => (StartConfig startConfig);\n> > > > + init(libcamera.IPASettings settings, InitParams params)\n> > > > + => (int32 ret, InitResult result);\n> > > > +\n> > > > + start(libcamera.ControlList controls) => (StartResult result);\n> > > > stop();\n> > > >\n> > > > - /**\n> > > > - * \\fn configure()\n> > >\n> > > Is it intentional to drop documentation ? Was it used at all ?\n> >\n> > I don't believe it is at all used. I looked at ipu3 and rkisp1 and they have\n> > no doc tags either.\n>\n> It's not mandatory, but it's nice to have. Could you keep it if that's\n> not too much trouble ?\n\nAck\n\n>\n> > > > - * \\brief Configure the IPA stream and sensor settings\n> > > > - * \\param[in] sensorInfo Camera sensor information\n> > > > - * \\param[in] ipaConfig Pipeline-handler-specific configuration data\n> > > > - * \\param[out] controls Controls to apply by the pipeline entity\n> > > > - * \\param[out] result Other results that the pipeline handler may require\n> > > > - *\n> > > > - * This function shall be called when the camera is configured to inform\n> > > > - * the IPA of the camera's streams and the sensor settings.\n> > > > - *\n> > > > - * The \\a sensorInfo conveys information about the camera sensor settings that\n> > > > - * the pipeline handler has selected for the configuration.\n> > > > - *\n> > > > - * The \\a ipaConfig and \\a controls parameters carry data passed by the\n> > > > - * pipeline handler to the IPA and back.\n> > > > - */\n> > > > - configure(libcamera.IPACameraSensorInfo sensorInfo,\n> > > > - IPAConfig ipaConfig)\n> > > > - => (int32 ret, libcamera.ControlList controls, IPAConfigResult result);\n> > > > -\n> > > > - /**\n> > > > - * \\fn mapBuffers()\n> > > > - * \\brief Map buffers shared between the pipeline handler and the IPA\n> > > > - * \\param[in] buffers List of buffers to map\n> > > > - *\n> > > > - * This function informs the IPA module of memory buffers set up by the\n> > > > - * pipeline handler that the IPA needs to access. It provides dmabuf\n> > > > - * file handles for each buffer, and associates the buffers with unique\n> > > > - * numerical IDs.\n> > > > - *\n> > > > - * IPAs shall map the dmabuf file handles to their address space and\n> > > > - * keep a cache of the mappings, indexed by the buffer numerical IDs.\n> > > > - * The IDs are used in all other IPA interface functions to refer to\n> > > > - * buffers, including the unmapBuffers() function.\n> > > > - *\n> > > > - * All buffers that the pipeline handler wishes to share with an IPA\n> > > > - * shall be mapped with this function. Buffers may be mapped all at once\n> > > > - * with a single call, or mapped and unmapped dynamically at runtime,\n> > > > - * depending on the IPA protocol. Regardless of the protocol, all\n> > > > - * buffers mapped at a given time shall have unique numerical IDs.\n> > > > - *\n> > > > - * The numerical IDs have no meaning defined by the IPA interface, and\n> > > > - * should be treated as opaque handles by IPAs, with the only exception\n> > > > - * that ID zero is invalid.\n> > > > - *\n> > > > - * \\sa unmapBuffers()\n> > > > - */\n> > > > - mapBuffers(array buffers);\n> > > > + configure(libcamera.IPACameraSensorInfo sensorInfo, ConfigParams params)\n> > > > + => (int32 ret, ConfigResult result);\n> > > >\n> > > > - /**\n> > > > - * \\fn unmapBuffers()\n> > > > - * \\brief Unmap buffers shared by the pipeline to the IPA\n> > > > - * \\param[in] ids List of buffer IDs to unmap\n> > > > - *\n> > > > - * This function removes mappings set up with mapBuffers(). Numerical\n> > > > - * IDs of unmapped buffers may be reused when mapping new buffers.\n> > > > - *\n> > > > - * \\sa mapBuffers()\n> > > > - */\n> > > > + mapBuffers(array buffers);\n> > > > unmapBuffers(array ids);\n> > > >\n> > > > - [async] signalStatReady(uint32 bufferId, uint32 ipaContext);\n> > > > - [async] signalQueueRequest(libcamera.ControlList controls);\n> > > > - [async] signalIspPrepare(ISPConfig data);\n> > > > + [async] prepareIsp(PrepareParams params);\n> > > > + [async] processStats(ProcessParams params);\n> > > > };\n> > > >\n> > > > interface IPARPiEventInterface {\n> > > > - statsMetadataComplete(uint32 bufferId, libcamera.ControlList controls);\n> > > > - runIsp(uint32 bufferId);\n> > > > - embeddedComplete(uint32 bufferId);\n> > > > + prepareIspComplete(BufferIds buffers);\n> > > > + processStatsComplete(BufferIds buffers);\n> > > > + metadataReady(libcamera.ControlList metadata);\n> > > > setIspControls(libcamera.ControlList controls);\n> > > > setDelayedControls(libcamera.ControlList controls, uint32 delayContext);\n> > > > setLensControls(libcamera.ControlList controls);\n> > > > setCameraTimeout(uint32 maxFrameLengthMs);\n> > > > };\n> > > > +\n> > >\n> > > Additional blank line\n> >\n> > Ack\n> >\n> > > The rest looks good\n> > > Reviewed-by: Jacopo Mondi \n> > >\n> > > > diff --git a/src/ipa/rpi/vc4/raspberrypi.cpp b/src/ipa/rpi/vc4/raspberrypi.cpp\n> > > > index 841635ccde2b..d737f1d662a0 100644\n> > > > --- a/src/ipa/rpi/vc4/raspberrypi.cpp\n> > > > +++ b/src/ipa/rpi/vc4/raspberrypi.cpp\n> > > > @@ -136,30 +136,28 @@ public:\n> > > > munmap(lsTable_, MaxLsGridSize);\n> > > > }\n> > > >\n> > > > - int init(const IPASettings &settings, bool lensPresent, IPAInitResult *result) override;\n> > > > - void start(const ControlList &controls, StartConfig *startConfig) override;\n> > > > + int init(const IPASettings &settings, const InitParams ¶ms, InitResult *result) override;\n> > > > + void start(const ControlList &controls, StartResult *result) override;\n> > > > void stop() override {}\n> > > >\n> > > > - int configure(const IPACameraSensorInfo &sensorInfo, const IPAConfig &data,\n> > > > - ControlList *controls, IPAConfigResult *result) override;\n> > > > + int configure(const IPACameraSensorInfo &sensorInfo, const ConfigParams ¶ms,\n> > > > + ConfigResult *result) override;\n> > > > void mapBuffers(const std::vector &buffers) override;\n> > > > void unmapBuffers(const std::vector &ids) override;\n> > > > - void signalStatReady(const uint32_t bufferId, uint32_t ipaContext) override;\n> > > > - void signalQueueRequest(const ControlList &controls) override;\n> > > > - void signalIspPrepare(const ISPConfig &data) override;\n> > > > + void prepareIsp(const PrepareParams ¶ms) override;\n> > > > + void processStats(const ProcessParams ¶ms) override;\n> > > >\n> > > > private:\n> > > > void setMode(const IPACameraSensorInfo &sensorInfo);\n> > > > bool validateSensorControls();\n> > > > bool validateIspControls();\n> > > > bool validateLensControls();\n> > > > - void queueRequest(const ControlList &controls);\n> > > > - void returnEmbeddedBuffer(unsigned int bufferId);\n> > > > - void prepareISP(const ISPConfig &data);\n> > > > + void applyControls(const ControlList &controls);\n> > > > + void prepare(const PrepareParams ¶ms);\n> > > > void reportMetadata(unsigned int ipaContext);\n> > > > void fillDeviceStatus(const ControlList &sensorControls, unsigned int ipaContext);\n> > > > RPiController::StatisticsPtr fillStatistics(bcm2835_isp_stats *stats) const;\n> > > > - void processStats(unsigned int bufferId, unsigned int ipaContext);\n> > > > + void process(unsigned int bufferId, unsigned int ipaContext);\n> > > > void setCameraTimeoutValue();\n> > > > void applyFrameDurations(Duration minFrameDuration, Duration maxFrameDuration);\n> > > > void applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls);\n> > > > @@ -229,7 +227,7 @@ private:\n> > > > Duration lastTimeout_;\n> > > > };\n> > > >\n> > > > -int IPARPi::init(const IPASettings &settings, bool lensPresent, IPAInitResult *result)\n> > > > +int IPARPi::init(const IPASettings &settings, const InitParams ¶ms, InitResult *result)\n> > > > {\n> > > > /*\n> > > > * Load the \"helper\" for this sensor. This tells us all the device specific stuff\n> > > > @@ -274,7 +272,7 @@ int IPARPi::init(const IPASettings &settings, bool lensPresent, IPAInitResult *r\n> > > > return -EINVAL;\n> > > > }\n> > > >\n> > > > - lensPresent_ = lensPresent;\n> > > > + lensPresent_ = params.lensPresent;\n> > > >\n> > > > controller_.initialise();\n> > > >\n> > > > @@ -287,14 +285,13 @@ int IPARPi::init(const IPASettings &settings, bool lensPresent, IPAInitResult *r\n> > > > return 0;\n> > > > }\n> > > >\n> > > > -void IPARPi::start(const ControlList &controls, StartConfig *startConfig)\n> > > > +void IPARPi::start(const ControlList &controls, StartResult *result)\n> > > > {\n> > > > RPiController::Metadata metadata;\n> > > >\n> > > > - ASSERT(startConfig);\n> > > > if (!controls.empty()) {\n> > > > /* We have been given some controls to action before start. */\n> > > > - queueRequest(controls);\n> > > > + applyControls(controls);\n> > > > }\n> > > >\n> > > > controller_.switchMode(mode_, &metadata);\n> > > > @@ -313,7 +310,7 @@ void IPARPi::start(const ControlList &controls, StartConfig *startConfig)\n> > > > if (agcStatus.shutterTime && agcStatus.analogueGain) {\n> > > > ControlList ctrls(sensorCtrls_);\n> > > > applyAGC(&agcStatus, ctrls);\n> > > > - startConfig->controls = std::move(ctrls);\n> > > > + result->controls = std::move(ctrls);\n> > > > setCameraTimeoutValue();\n> > > > }\n> > > >\n> > > > @@ -360,7 +357,7 @@ void IPARPi::start(const ControlList &controls, StartConfig *startConfig)\n> > > > mistrustCount_ = helper_->mistrustFramesModeSwitch();\n> > > > }\n> > > >\n> > > > - startConfig->dropFrameCount = dropFrameCount_;\n> > > > + result->dropFrameCount = dropFrameCount_;\n> > > >\n> > > > firstStart_ = false;\n> > > > lastRunTimestamp_ = 0;\n> > > > @@ -435,11 +432,11 @@ void IPARPi::setMode(const IPACameraSensorInfo &sensorInfo)\n> > > > mode_.minFrameDuration, mode_.maxFrameDuration);\n> > > > }\n> > > >\n> > > > -int IPARPi::configure(const IPACameraSensorInfo &sensorInfo, const IPAConfig &ipaConfig,\n> > > > - ControlList *controls, IPAConfigResult *result)\n> > > > +int IPARPi::configure(const IPACameraSensorInfo &sensorInfo, const ConfigParams ¶ms,\n> > > > + ConfigResult *result)\n> > > > {\n> > > > - sensorCtrls_ = ipaConfig.sensorControls;\n> > > > - ispCtrls_ = ipaConfig.ispControls;\n> > > > + sensorCtrls_ = params.sensorControls;\n> > > > + ispCtrls_ = params.ispControls;\n> > > >\n> > > > if (!validateSensorControls()) {\n> > > > LOG(IPARPI, Error) << \"Sensor control validation failed.\";\n> > > > @@ -452,7 +449,7 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo, const IPAConfig &ip\n> > > > }\n> > > >\n> > > > if (lensPresent_) {\n> > > > - lensCtrls_ = ipaConfig.lensControls;\n> > > > + lensCtrls_ = params.lensControls;\n> > > > if (!validateLensControls()) {\n> > > > LOG(IPARPI, Warning) << \"Lens validation failed, \"\n> > > > << \"no lens control will be available.\";\n> > > > @@ -466,10 +463,10 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo, const IPAConfig &ip\n> > > > /* Re-assemble camera mode using the sensor info. */\n> > > > setMode(sensorInfo);\n> > > >\n> > > > - mode_.transform = static_cast(ipaConfig.transform);\n> > > > + mode_.transform = static_cast(params.transform);\n> > > >\n> > > > /* Store the lens shading table pointer and handle if available. */\n> > > > - if (ipaConfig.lsTableHandle.isValid()) {\n> > > > + if (params.lsTableHandle.isValid()) {\n> > > > /* Remove any previous table, if there was one. */\n> > > > if (lsTable_) {\n> > > > munmap(lsTable_, MaxLsGridSize);\n> > > > @@ -477,7 +474,7 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo, const IPAConfig &ip\n> > > > }\n> > > >\n> > > > /* Map the LS table buffer into user space. */\n> > > > - lsTableHandle_ = std::move(ipaConfig.lsTableHandle);\n> > > > + lsTableHandle_ = std::move(params.lsTableHandle);\n> > > > if (lsTableHandle_.isValid()) {\n> > > > lsTable_ = mmap(nullptr, MaxLsGridSize, PROT_READ | PROT_WRITE,\n> > > > MAP_SHARED, lsTableHandle_.get(), 0);\n> > > > @@ -512,8 +509,7 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo, const IPAConfig &ip\n> > > > applyAGC(&agcStatus, ctrls);\n> > > > }\n> > > >\n> > > > - ASSERT(controls);\n> > > > - *controls = std::move(ctrls);\n> > > > + result->controls = std::move(ctrls);\n> > > >\n> > > > /*\n> > > > * Apply the correct limits to the exposure, gain and frame duration controls\n> > > > @@ -560,37 +556,34 @@ void IPARPi::unmapBuffers(const std::vector &ids)\n> > > > }\n> > > > }\n> > > >\n> > > > -void IPARPi::signalStatReady(uint32_t bufferId, uint32_t ipaContext)\n> > > > +void IPARPi::processStats(const ProcessParams ¶ms)\n> > > > {\n> > > > - unsigned int context = ipaContext % rpiMetadata_.size();\n> > > > + unsigned int context = params.ipaContext % rpiMetadata_.size();\n> > > >\n> > > > if (++checkCount_ != frameCount_) /* assert here? */\n> > > > LOG(IPARPI, Error) << \"WARNING: Prepare/Process mismatch!!!\";\n> > > > if (processPending_ && frameCount_ > mistrustCount_)\n> > > > - processStats(bufferId, context);\n> > > > + process(params.buffers.stats, context);\n> > > >\n> > > > reportMetadata(context);\n> > > > -\n> > > > - statsMetadataComplete.emit(bufferId, libcameraMetadata_);\n> > > > + processStatsComplete.emit(params.buffers);\n> > > > }\n> > > >\n> > > > -void IPARPi::signalQueueRequest(const ControlList &controls)\n> > > > -{\n> > > > - queueRequest(controls);\n> > > > -}\n> > > >\n> > > > -void IPARPi::signalIspPrepare(const ISPConfig &data)\n> > > > +void IPARPi::prepareIsp(const PrepareParams ¶ms)\n> > > > {\n> > > > + applyControls(params.requestControls);\n> > > > +\n> > > > /*\n> > > > * At start-up, or after a mode-switch, we may want to\n> > > > * avoid running the control algos for a few frames in case\n> > > > * they are \"unreliable\".\n> > > > */\n> > > > - prepareISP(data);\n> > > > + prepare(params);\n> > > > frameCount_++;\n> > > >\n> > > > /* Ready to push the input buffer into the ISP. */\n> > > > - runIsp.emit(data.bayerBufferId);\n> > > > + prepareIspComplete.emit(params.buffers);\n> > > > }\n> > > >\n> > > > void IPARPi::reportMetadata(unsigned int ipaContext)\n> > > > @@ -703,6 +696,8 @@ void IPARPi::reportMetadata(unsigned int ipaContext)\n> > > > libcameraMetadata_.set(controls::AfState, s);\n> > > > libcameraMetadata_.set(controls::AfPauseState, p);\n> > > > }\n> > > > +\n> > > > + metadataReady.emit(libcameraMetadata_);\n> > > > }\n> > > >\n> > > > bool IPARPi::validateSensorControls()\n> > > > @@ -826,7 +821,7 @@ static const std::map AfPauseTable\n> > > > { controls::AfPauseResume, RPiController::AfAlgorithm::AfPauseResume },\n> > > > };\n> > > >\n> > > > -void IPARPi::queueRequest(const ControlList &controls)\n> > > > +void IPARPi::applyControls(const ControlList &controls)\n> > > > {\n> > > > using RPiController::AfAlgorithm;\n> > > >\n> > > > @@ -1256,27 +1251,22 @@ void IPARPi::queueRequest(const ControlList &controls)\n> > > > }\n> > > > }\n> > > >\n> > > > -void IPARPi::returnEmbeddedBuffer(unsigned int bufferId)\n> > > > +void IPARPi::prepare(const PrepareParams ¶ms)\n> > > > {\n> > > > - embeddedComplete.emit(bufferId);\n> > > > -}\n> > > > -\n> > > > -void IPARPi::prepareISP(const ISPConfig &data)\n> > > > -{\n> > > > - int64_t frameTimestamp = data.controls.get(controls::SensorTimestamp).value_or(0);\n> > > > - unsigned int ipaContext = data.ipaContext % rpiMetadata_.size();\n> > > > + int64_t frameTimestamp = params.sensorControls.get(controls::SensorTimestamp).value_or(0);\n> > > > + unsigned int ipaContext = params.ipaContext % rpiMetadata_.size();\n> > > > RPiController::Metadata &rpiMetadata = rpiMetadata_[ipaContext];\n> > > > Span embeddedBuffer;\n> > > >\n> > > > rpiMetadata.clear();\n> > > > - fillDeviceStatus(data.controls, ipaContext);\n> > > > + fillDeviceStatus(params.sensorControls, ipaContext);\n> > > >\n> > > > - if (data.embeddedBufferPresent) {\n> > > > + if (params.buffers.embedded) {\n> > > > /*\n> > > > * Pipeline handler has supplied us with an embedded data buffer,\n> > > > * we must pass it to the CamHelper for parsing.\n> > > > */\n> > > > - auto it = buffers_.find(data.embeddedBufferId);\n> > > > + auto it = buffers_.find(params.buffers.embedded);\n> > > > ASSERT(it != buffers_.end());\n> > > > embeddedBuffer = it->second.planes()[0];\n> > > > }\n> > > > @@ -1288,7 +1278,7 @@ void IPARPi::prepareISP(const ISPConfig &data)\n> > > > * metadata.\n> > > > */\n> > > > AgcStatus agcStatus;\n> > > > - RPiController::Metadata &delayedMetadata = rpiMetadata_[data.delayContext];\n> > > > + RPiController::Metadata &delayedMetadata = rpiMetadata_[params.delayContext];\n> > > > if (!delayedMetadata.get(\"agc.status\", agcStatus))\n> > > > rpiMetadata.set(\"agc.delayed_status\", agcStatus);\n> > > >\n> > > > @@ -1298,10 +1288,6 @@ void IPARPi::prepareISP(const ISPConfig &data)\n> > > > */\n> > > > helper_->prepare(embeddedBuffer, rpiMetadata);\n> > > >\n> > > > - /* Done with embedded data now, return to pipeline handler asap. */\n> > > > - if (data.embeddedBufferPresent)\n> > > > - returnEmbeddedBuffer(data.embeddedBufferId);\n> > > > -\n> > > > /* Allow a 10% margin on the comparison below. */\n> > > > Duration delta = (frameTimestamp - lastRunTimestamp_) * 1.0ns;\n> > > > if (lastRunTimestamp_ && frameCount_ > dropFrameCount_ &&\n> > > > @@ -1445,7 +1431,7 @@ RPiController::StatisticsPtr IPARPi::fillStatistics(bcm2835_isp_stats *stats) co\n> > > > return statistics;\n> > > > }\n> > > >\n> > > > -void IPARPi::processStats(unsigned int bufferId, unsigned int ipaContext)\n> > > > +void IPARPi::process(unsigned int bufferId, unsigned int ipaContext)\n> > > > {\n> > > > RPiController::Metadata &rpiMetadata = rpiMetadata_[ipaContext];\n> > > >\n> > > > diff --git a/src/libcamera/pipeline/rpi/vc4/raspberrypi.cpp b/src/libcamera/pipeline/rpi/vc4/raspberrypi.cpp\n> > > > index e54bf1ef2c17..30ce6feab0d1 100644\n> > > > --- a/src/libcamera/pipeline/rpi/vc4/raspberrypi.cpp\n> > > > +++ b/src/libcamera/pipeline/rpi/vc4/raspberrypi.cpp\n> > > > @@ -200,15 +200,15 @@ public:\n> > > > void freeBuffers();\n> > > > void frameStarted(uint32_t sequence);\n> > > >\n> > > > - int loadIPA(ipa::RPi::IPAInitResult *result);\n> > > > - int configureIPA(const CameraConfiguration *config, ipa::RPi::IPAConfigResult *result);\n> > > > + int loadIPA(ipa::RPi::InitResult *result);\n> > > > + int configureIPA(const CameraConfiguration *config, ipa::RPi::ConfigResult *result);\n> > > > int loadPipelineConfiguration();\n> > > >\n> > > > void enumerateVideoDevices(MediaLink *link);\n> > > >\n> > > > - void statsMetadataComplete(uint32_t bufferId, const ControlList &controls);\n> > > > - void runIsp(uint32_t bufferId);\n> > > > - void embeddedComplete(uint32_t bufferId);\n> > > > + void processStatsComplete(const ipa::RPi::BufferIds &buffers);\n> > > > + void metadataReady(const ControlList &metadata);\n> > > > + void prepareIspComplete(const ipa::RPi::BufferIds &buffers);\n> > > > void setIspControls(const ControlList &controls);\n> > > > void setDelayedControls(const ControlList &controls, uint32_t delayContext);\n> > > > void setLensControls(const ControlList &controls);\n> > > > @@ -238,7 +238,7 @@ public:\n> > > > /* The vector below is just for convenience when iterating over all streams. */\n> > > > std::vector streams_;\n> > > > /* Stores the ids of the buffers mapped in the IPA. */\n> > > > - std::unordered_set ipaBuffers_;\n> > > > + std::unordered_set bufferIds_;\n> > > > /*\n> > > > * Stores a cascade of Video Mux or Bridge devices between the sensor and\n> > > > * Unicam together with media link across the entities.\n> > > > @@ -1000,7 +1000,7 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)\n> > > >\n> > > > data->isp_[Isp::Input].dev()->setSelection(V4L2_SEL_TGT_CROP, &crop);\n> > > >\n> > > > - ipa::RPi::IPAConfigResult result;\n> > > > + ipa::RPi::ConfigResult result;\n> > > > ret = data->configureIPA(config, &result);\n> > > > if (ret)\n> > > > LOG(RPI, Error) << \"Failed to configure the IPA: \" << ret;\n> > > > @@ -1117,17 +1117,17 @@ int PipelineHandlerRPi::start(Camera *camera, const ControlList *controls)\n> > > > data->applyScalerCrop(*controls);\n> > > >\n> > > > /* Start the IPA. */\n> > > > - ipa::RPi::StartConfig startConfig;\n> > > > + ipa::RPi::StartResult result;\n> > > > data->ipa_->start(controls ? *controls : ControlList{ controls::controls },\n> > > > - &startConfig);\n> > > > + &result);\n> > > >\n> > > > /* Apply any gain/exposure settings that the IPA may have passed back. */\n> > > > - if (!startConfig.controls.empty())\n> > > > - data->setSensorControls(startConfig.controls);\n> > > > + if (!result.controls.empty())\n> > > > + data->setSensorControls(result.controls);\n> > > >\n> > > > /* Configure the number of dropped frames required on startup. */\n> > > > data->dropFrameCount_ = data->config_.disableStartupFrameDrops\n> > > > - ? 0 : startConfig.dropFrameCount;\n> > > > + ? 0 : result.dropFrameCount;\n> > > >\n> > > > for (auto const stream : data->streams_)\n> > > > stream->resetBuffers();\n> > > > @@ -1358,7 +1358,7 @@ int PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp, Me\n> > > >\n> > > > data->sensorFormats_ = populateSensorFormats(data->sensor_);\n> > > >\n> > > > - ipa::RPi::IPAInitResult result;\n> > > > + ipa::RPi::InitResult result;\n> > > > if (data->loadIPA(&result)) {\n> > > > LOG(RPI, Error) << \"Failed to load a suitable IPA library\";\n> > > > return -EINVAL;\n> > > > @@ -1599,7 +1599,7 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)\n> > > > void PipelineHandlerRPi::mapBuffers(Camera *camera, const RPi::BufferMap &buffers, unsigned int mask)\n> > > > {\n> > > > RPiCameraData *data = cameraData(camera);\n> > > > - std::vector ipaBuffers;\n> > > > + std::vector bufferIds;\n> > > > /*\n> > > > * Link the FrameBuffers with the id (key value) in the map stored in\n> > > > * the RPi stream object - along with an identifier mask.\n> > > > @@ -1608,12 +1608,12 @@ void PipelineHandlerRPi::mapBuffers(Camera *camera, const RPi::BufferMap &buffer\n> > > > * handler and the IPA.\n> > > > */\n> > > > for (auto const &it : buffers) {\n> > > > - ipaBuffers.push_back(IPABuffer(mask | it.first,\n> > > > + bufferIds.push_back(IPABuffer(mask | it.first,\n> > > > it.second->planes()));\n> > > > - data->ipaBuffers_.insert(mask | it.first);\n> > > > + data->bufferIds_.insert(mask | it.first);\n> > > > }\n> > > >\n> > > > - data->ipa_->mapBuffers(ipaBuffers);\n> > > > + data->ipa_->mapBuffers(bufferIds);\n> > > > }\n> > > >\n> > > > void RPiCameraData::freeBuffers()\n> > > > @@ -1623,10 +1623,10 @@ void RPiCameraData::freeBuffers()\n> > > > * Copy the buffer ids from the unordered_set to a vector to\n> > > > * pass to the IPA.\n> > > > */\n> > > > - std::vector ipaBuffers(ipaBuffers_.begin(),\n> > > > - ipaBuffers_.end());\n> > > > - ipa_->unmapBuffers(ipaBuffers);\n> > > > - ipaBuffers_.clear();\n> > > > + std::vector bufferIds(bufferIds_.begin(),\n> > > > + bufferIds_.end());\n> > > > + ipa_->unmapBuffers(bufferIds);\n> > > > + bufferIds_.clear();\n> > > > }\n> > > >\n> > > > for (auto const stream : streams_)\n> > > > @@ -1643,16 +1643,16 @@ void RPiCameraData::frameStarted(uint32_t sequence)\n> > > > delayedCtrls_->applyControls(sequence);\n> > > > }\n> > > >\n> > > > -int RPiCameraData::loadIPA(ipa::RPi::IPAInitResult *result)\n> > > > +int RPiCameraData::loadIPA(ipa::RPi::InitResult *result)\n> > > > {\n> > > > ipa_ = IPAManager::createIPA(pipe(), 1, 1);\n> > > >\n> > > > if (!ipa_)\n> > > > return -ENOENT;\n> > > >\n> > > > - ipa_->statsMetadataComplete.connect(this, &RPiCameraData::statsMetadataComplete);\n> > > > - ipa_->runIsp.connect(this, &RPiCameraData::runIsp);\n> > > > - ipa_->embeddedComplete.connect(this, &RPiCameraData::embeddedComplete);\n> > > > + ipa_->processStatsComplete.connect(this, &RPiCameraData::processStatsComplete);\n> > > > + ipa_->prepareIspComplete.connect(this, &RPiCameraData::prepareIspComplete);\n> > > > + ipa_->metadataReady.connect(this, &RPiCameraData::metadataReady);\n> > > > ipa_->setIspControls.connect(this, &RPiCameraData::setIspControls);\n> > > > ipa_->setDelayedControls.connect(this, &RPiCameraData::setDelayedControls);\n> > > > ipa_->setLensControls.connect(this, &RPiCameraData::setLensControls);\n> > > > @@ -1674,23 +1674,25 @@ int RPiCameraData::loadIPA(ipa::RPi::IPAInitResult *result)\n> > > > }\n> > > >\n> > > > IPASettings settings(configurationFile, sensor_->model());\n> > > > + ipa::RPi::InitParams params;\n> > > >\n> > > > - return ipa_->init(settings, !!sensor_->focusLens(), result);\n> > > > + params.lensPresent = !!sensor_->focusLens();\n> > > > + return ipa_->init(settings, params, result);\n> > > > }\n> > > >\n> > > > -int RPiCameraData::configureIPA(const CameraConfiguration *config, ipa::RPi::IPAConfigResult *result)\n> > > > +int RPiCameraData::configureIPA(const CameraConfiguration *config, ipa::RPi::ConfigResult *result)\n> > > > {\n> > > > std::map entityControls;\n> > > > - ipa::RPi::IPAConfig ipaConfig;\n> > > > + ipa::RPi::ConfigParams params;\n> > > >\n> > > > /* \\todo Move passing of ispControls and lensControls to ipa::init() */\n> > > > - ipaConfig.sensorControls = sensor_->controls();\n> > > > - ipaConfig.ispControls = isp_[Isp::Input].dev()->controls();\n> > > > + params.sensorControls = sensor_->controls();\n> > > > + params.ispControls = isp_[Isp::Input].dev()->controls();\n> > > > if (sensor_->focusLens())\n> > > > - ipaConfig.lensControls = sensor_->focusLens()->controls();\n> > > > + params.lensControls = sensor_->focusLens()->controls();\n> > > >\n> > > > /* Always send the user transform to the IPA. */\n> > > > - ipaConfig.transform = static_cast(config->transform);\n> > > > + params.transform = static_cast(config->transform);\n> > > >\n> > > > /* Allocate the lens shading table via dmaHeap and pass to the IPA. */\n> > > > if (!lsTable_.isValid()) {\n> > > > @@ -1703,7 +1705,7 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config, ipa::RPi::IPA\n> > > > * \\todo Investigate if mapping the lens shading table buffer\n> > > > * could be handled with mapBuffers().\n> > > > */\n> > > > - ipaConfig.lsTableHandle = lsTable_;\n> > > > + params.lsTableHandle = lsTable_;\n> > > > }\n> > > >\n> > > > /* We store the IPACameraSensorInfo for digital zoom calculations. */\n> > > > @@ -1714,15 +1716,14 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config, ipa::RPi::IPA\n> > > > }\n> > > >\n> > > > /* Ready the IPA - it must know about the sensor resolution. */\n> > > > - ControlList controls;\n> > > > - ret = ipa_->configure(sensorInfo_, ipaConfig, &controls, result);\n> > > > + ret = ipa_->configure(sensorInfo_, params, result);\n> > > > if (ret < 0) {\n> > > > LOG(RPI, Error) << \"IPA configuration failed!\";\n> > > > return -EPIPE;\n> > > > }\n> > > >\n> > > > - if (!controls.empty())\n> > > > - setSensorControls(controls);\n> > > > + if (!result->controls.empty())\n> > > > + setSensorControls(result->controls);\n> > > >\n> > > > return 0;\n> > > > }\n> > > > @@ -1883,24 +1884,32 @@ void RPiCameraData::enumerateVideoDevices(MediaLink *link)\n> > > > }\n> > > > }\n> > > >\n> > > > -void RPiCameraData::statsMetadataComplete(uint32_t bufferId, const ControlList &controls)\n> > > > +void RPiCameraData::processStatsComplete(const ipa::RPi::BufferIds &buffers)\n> > > > {\n> > > > if (!isRunning())\n> > > > return;\n> > > >\n> > > > - FrameBuffer *buffer = isp_[Isp::Stats].getBuffers().at(bufferId & RPi::MaskID);\n> > > > + FrameBuffer *buffer = isp_[Isp::Stats].getBuffers().at(buffers.stats & RPi::MaskID);\n> > > >\n> > > > handleStreamBuffer(buffer, &isp_[Isp::Stats]);\n> > > > + state_ = State::IpaComplete;\n> > > > + handleState();\n> > > > +}\n> > > > +\n> > > > +void RPiCameraData::metadataReady(const ControlList &metadata)\n> > > > +{\n> > > > + if (!isRunning())\n> > > > + return;\n> > > >\n> > > > /* Add to the Request metadata buffer what the IPA has provided. */\n> > > > Request *request = requestQueue_.front();\n> > > > - request->metadata().merge(controls);\n> > > > + request->metadata().merge(metadata);\n> > > >\n> > > > /*\n> > > > * Inform the sensor of the latest colour gains if it has the\n> > > > * V4L2_CID_NOTIFY_GAINS control (which means notifyGainsUnity_ is set).\n> > > > */\n> > > > - const auto &colourGains = controls.get(libcamera::controls::ColourGains);\n> > > > + const auto &colourGains = metadata.get(libcamera::controls::ColourGains);\n> > > > if (notifyGainsUnity_ && colourGains) {\n> > > > /* The control wants linear gains in the order B, Gb, Gr, R. */\n> > > > ControlList ctrls(sensor_->controls());\n> > > > @@ -1914,33 +1923,29 @@ void RPiCameraData::statsMetadataComplete(uint32_t bufferId, const ControlList &\n> > > >\n> > > > sensor_->setControls(&ctrls);\n> > > > }\n> > > > -\n> > > > - state_ = State::IpaComplete;\n> > > > - handleState();\n> > > > }\n> > > >\n> > > > -void RPiCameraData::runIsp(uint32_t bufferId)\n> > > > +void RPiCameraData::prepareIspComplete(const ipa::RPi::BufferIds &buffers)\n> > > > {\n> > > > + unsigned int embeddedId = buffers.embedded & RPi::MaskID;\n> > > > + unsigned int bayer = buffers.bayer & RPi::MaskID;\n> > > > + FrameBuffer *buffer;\n> > > > +\n> > > > if (!isRunning())\n> > > > return;\n> > > >\n> > > > - FrameBuffer *buffer = unicam_[Unicam::Image].getBuffers().at(bufferId & RPi::MaskID);\n> > > > -\n> > > > - LOG(RPI, Debug) << \"Input re-queue to ISP, buffer id \" << (bufferId & RPi::MaskID)\n> > > > + buffer = unicam_[Unicam::Image].getBuffers().at(bayer & RPi::MaskID);\n> > > > + LOG(RPI, Debug) << \"Input re-queue to ISP, buffer id \" << (bayer & RPi::MaskID)\n> > > > << \", timestamp: \" << buffer->metadata().timestamp;\n> > > >\n> > > > isp_[Isp::Input].queueBuffer(buffer);\n> > > > ispOutputCount_ = 0;\n> > > > - handleState();\n> > > > -}\n> > > >\n> > > > -void RPiCameraData::embeddedComplete(uint32_t bufferId)\n> > > > -{\n> > > > - if (!isRunning())\n> > > > - return;\n> > > > + if (sensorMetadata_ && embeddedId) {\n> > > > + buffer = unicam_[Unicam::Embedded].getBuffers().at(embeddedId & RPi::MaskID);\n> > > > + handleStreamBuffer(buffer, &unicam_[Unicam::Embedded]);\n> > > > + }\n> > > >\n> > > > - FrameBuffer *buffer = unicam_[Unicam::Embedded].getBuffers().at(bufferId & RPi::MaskID);\n> > > > - handleStreamBuffer(buffer, &unicam_[Unicam::Embedded]);\n> > > > handleState();\n> > > > }\n> > > >\n> > > > @@ -2116,8 +2121,10 @@ void RPiCameraData::ispOutputDequeue(FrameBuffer *buffer)\n> > > > * application until after the IPA signals so.\n> > > > */\n> > > > if (stream == &isp_[Isp::Stats]) {\n> > > > - ipa_->signalStatReady(RPi::MaskStats | static_cast(index),\n> > > > - requestQueue_.front()->sequence());\n> > > > + ipa::RPi::ProcessParams params;\n> > > > + params.buffers.stats = index | RPi::MaskStats;\n> > > > + params.ipaContext = requestQueue_.front()->sequence();\n> > > > + ipa_->processStats(params);\n> > > > } else {\n> > > > /* Any other ISP output can be handed back to the application now. */\n> > > > handleStreamBuffer(buffer, stream);\n> > > > @@ -2344,38 +2351,30 @@ void RPiCameraData::tryRunPipeline()\n> > > > request->metadata().clear();\n> > > > fillRequestMetadata(bayerFrame.controls, request);\n> > > >\n> > > > - /*\n> > > > - * Process all the user controls by the IPA. Once this is complete, we\n> > > > - * queue the ISP output buffer listed in the request to start the HW\n> > > > - * pipeline.\n> > > > - */\n> > > > - ipa_->signalQueueRequest(request->controls());\n> > > > -\n> > > > /* Set our state to say the pipeline is active. */\n> > > > state_ = State::Busy;\n> > > >\n> > > > - unsigned int bayerId = unicam_[Unicam::Image].getBufferId(bayerFrame.buffer);\n> > > > + unsigned int bayer = unicam_[Unicam::Image].getBufferId(bayerFrame.buffer);\n> > > >\n> > > > - LOG(RPI, Debug) << \"Signalling signalIspPrepare:\"\n> > > > - << \" Bayer buffer id: \" << bayerId;\n> > > > + LOG(RPI, Debug) << \"Signalling prepareIsp:\"\n> > > > + << \" Bayer buffer id: \" << bayer;\n> > > >\n> > > > - ipa::RPi::ISPConfig ispPrepare;\n> > > > - ispPrepare.bayerBufferId = RPi::MaskBayerData | bayerId;\n> > > > - ispPrepare.controls = std::move(bayerFrame.controls);\n> > > > - ispPrepare.ipaContext = request->sequence();\n> > > > - ispPrepare.delayContext = bayerFrame.delayContext;\n> > > > + ipa::RPi::PrepareParams params;\n> > > > + params.buffers.bayer = RPi::MaskBayerData | bayer;\n> > > > + params.sensorControls = std::move(bayerFrame.controls);\n> > > > + params.requestControls = request->controls();\n> > > > + params.ipaContext = request->sequence();\n> > > > + params.delayContext = bayerFrame.delayContext;\n> > > >\n> > > > if (embeddedBuffer) {\n> > > > unsigned int embeddedId = unicam_[Unicam::Embedded].getBufferId(embeddedBuffer);\n> > > >\n> > > > - ispPrepare.embeddedBufferId = RPi::MaskEmbeddedData | embeddedId;\n> > > > - ispPrepare.embeddedBufferPresent = true;\n> > > > -\n> > > > - LOG(RPI, Debug) << \"Signalling signalIspPrepare:\"\n> > > > + params.buffers.embedded = RPi::MaskEmbeddedData | embeddedId;\n> > > > + LOG(RPI, Debug) << \"Signalling prepareIsp:\"\n> > > > << \" Embedded buffer id: \" << embeddedId;\n> > > > }\n> > > >\n> > > > - ipa_->signalIspPrepare(ispPrepare);\n> > > > + ipa_->prepareIsp(params);\n> > > > }\n> > > >\n> > > > bool RPiCameraData::findMatchingBuffers(BayerFrame &bayerFrame, FrameBuffer *&embeddedBuffer)\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 AFC82C0DA4\n\tfor ;\n\tFri, 28 Apr 2023 09:43:34 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 09D02627D4;\n\tFri, 28 Apr 2023 11:43:34 +0200 (CEST)","from mail-yb1-xb36.google.com (mail-yb1-xb36.google.com\n\t[IPv6:2607:f8b0:4864:20::b36])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E5ABF627CD\n\tfor ;\n\tFri, 28 Apr 2023 11:43:31 +0200 (CEST)","by mail-yb1-xb36.google.com with SMTP id\n\t3f1490d57ef6-b97ec4bbc5aso7705431276.3\n\tfor ;\n\tFri, 28 Apr 2023 02:43:31 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1682675014;\n\tbh=YsxfXjfO0vheJK9wjqgFlKQBjcnO8n/OsIaQoQMrmu8=;\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=zsjNv8ZhEgZHQrIMcvJ5LmdV0/IDz7llIDwQHrPFJgKBzJEdMqXQZA+5wahRqmBxy\n\tQDWAzLuRgPUKloYwtt7Gdf8V6TycZOJhgiZIXZjzjAWWPsiyoVQCeoyyBFcvmmBe1F\n\tMRICBzDNUTgR+NN+Om9gx5PHJTMbKf8wbnkO9+I1/OR5elVxLk/JM23/zvZH79BS9v\n\t7ycaI4MdMdtVritHLtL9woOJjLrK1gHYatcsY9CTDo2NpOLr2xz0sY0+HUJjFj0vGY\n\tStyGgcwHBC6zJAgHn8ty6PA0m/BM7IAJrkqy2zm3pf89d1IzXPfOVXVKkrfcxnIAZb\n\tX6sm8/Usa0Nbw==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1682675010; x=1685267010;\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=ycjoX1Ql903txCNJTXGGabNfnmMB64Tl3N08WqVk2/k=;\n\tb=A56quc/syQODlQMZGWvxugImIxALIOlXVVjl5/00zlN4w1XIx4hPr90SDy3kfwU87f\n\tdvI0cpJJPtOE5cURxlVNGEbq8A4ACM17jjGn4gNCiHYHn+8YgrWTIz/SN80LBP4+lLqr\n\tAHZ95lcPhwWlUEnuB+4pWAzeHIMAJi7LAp4UGhRR1DVMeBh7bpcVBxtkjn+ut/AX5MIP\n\tEBDk7J5yFiDjcn1EhkJJhBTz9FDfM+IfoGte+NSCGeMMqSsnI3LH91ujmMcnaM1uJTvb\n\tRPX2npck+4zNGjWInAJZBfttMvds0TXogT5d8Qg/wj6RK7Bh3GRi7xrpRwyv+J/thK1G\n\t126g=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"A56quc/s\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20221208; t=1682675010; x=1685267010;\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=ycjoX1Ql903txCNJTXGGabNfnmMB64Tl3N08WqVk2/k=;\n\tb=ImgYQWEoUHysYYCEsG/ZQc7yhwQaNRuESG0m/tDFv5KHC1g85Ouos9Hm2x9GmpNM11\n\tEcDU+Blod6AFmkVkX3DZSHqahM3mOfQEs9mk9YKVyza9laTEhJhta/iVQuHDSeQqOOwG\n\tkJQifnk8xQpiclnJvUXjAK3ZNnGEN8umqKee/XoZI9NHTlt8xCjVkEEOG5xxhmGaVjRh\n\tEz5xRhJ/FWJDscvYczQQi/pvaqigmW+7aN1qpgu1j675Tlx+k+6Z078u7gtFgWmN8ADl\n\tdmE40NklkcrzMTPx5V/N29v2MupQcyd+99KPKNorpMB/pIKNmwLfh40a0v1PY59WtQEO\n\t/yGg==","X-Gm-Message-State":"AC+VfDwJEPYO0BUj4baBGJoHX71dqfw/wLL/unfm6kPm0dQNYC4LBVQw\n\tzFMhV0EiILQ0BInQoCH1P8ffWYLd10/dzqtFLuGH33fmuj3lZpD1tIhbZg==","X-Google-Smtp-Source":"ACHHUZ5Mhx+BJpqPm81U0BJhxAagAEkXSUBt7uYPG+CKOBRcFUVAFnrCoGHB8tkz3cNBrUQjjzN4DmGU1u6dIy38rJY=","X-Received":"by 2002:a25:ada3:0:b0:b99:f279:10dc with SMTP id\n\tz35-20020a25ada3000000b00b99f27910dcmr3442771ybi.28.1682675010296;\n\tFri, 28 Apr 2023 02:43:30 -0700 (PDT)","MIME-Version":"1.0","References":"<20230426131057.21550-1-naush@raspberrypi.com>\n\t<20230426131057.21550-7-naush@raspberrypi.com>\n\t<26csbh5y5qh7r4rle34eyn3drneqvvifcqb65s5nppg5ifm6kv@ft5oy4xnkzu4>\n\t\n\t<20230427171521.GC26786@pendragon.ideasonboard.com>","In-Reply-To":"<20230427171521.GC26786@pendragon.ideasonboard.com>","Date":"Fri, 28 Apr 2023 10:43:23 +0100","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH 06/13] pipeline: ipa: raspberrypi:\n\tRestructure the IPA mojom interface","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":27012,"web_url":"https://patchwork.libcamera.org/comment/27012/","msgid":"<20230502153730.GC22691@pendragon.ideasonboard.com>","date":"2023-05-02T15:37:30","subject":"Re: [libcamera-devel] [PATCH 06/13] pipeline: ipa: raspberrypi:\n\tRestructure the IPA mojom interface","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Naush,\n\nOn Fri, Apr 28, 2023 at 10:43:23AM +0100, Naushir Patuck wrote:\n> On Thu, 27 Apr 2023 at 18:15, Laurent Pinchart wrote:\n> > On Thu, Apr 27, 2023 at 10:53:25AM +0100, Naushir Patuck via libcamera-devel wrote:\n> > > On Thu, 27 Apr 2023 at 09:56, Jacopo Mondi wrote:\n> > > > On Wed, Apr 26, 2023 at 02:10:50PM +0100, Naushir Patuck via libcamera-devel wrote:\n> > > > > Restructure the IPA mojom interface to be more consistent in the use\n> > > > > of the API. Function parameters are now grouped into *Params structures\n> > > > > and results are now returned in *Results structures.\n> > > > >\n> > > > > The following pipeline -> IPA interfaces have been removed:\n> > > > >\n> > > > > signalQueueRequest(libcamera.ControlList controls);\n> > > > > signalIspPrepare(ISPConfig data);\n> > > > > signalStatReady(uint32 bufferId, uint32 ipaContext);\n> > > > >\n> > > > > and replaced with:\n> > > > >\n> > > > > prepareIsp(PrepareParams params);\n> > > > > processStats(ProcessParams params);\n> > > > >\n> > > > > signalQueueRequest() is now encompassed within signalPrepareIsp().\n> >\n> > I think you meant \"within prepareIsp()\" here.\n> >\n> > > > > The following IPA -> pipeline interfaces have been removed:\n> > > > >\n> > > > > runIsp(uint32 bufferId);\n> > > > > embeddedComplete(uint32 bufferId);\n> > > > > statsMetadataComplete(uint32 bufferId, libcamera.ControlList controls);\n> > > > >\n> > > > > and replaced with the following async calls:\n> > > > >\n> > > > > prepareIspComplete(BufferIds buffers);\n> > > > > processStatsComplete(BufferIds buffers);\n> > > > > metadataReady(libcamera.ControlList metadata);\n> > > > >\n> > > > > Signed-off-by: Naushir Patuck \n> > > > > ---\n> > > > > include/libcamera/ipa/raspberrypi.mojom | 127 +++++---------\n> > > > > src/ipa/rpi/vc4/raspberrypi.cpp | 102 +++++-------\n> > > > > .../pipeline/rpi/vc4/raspberrypi.cpp | 155 +++++++++---------\n> > > > > 3 files changed, 165 insertions(+), 219 deletions(-)\n> > > > >\n> > > > > diff --git a/include/libcamera/ipa/raspberrypi.mojom b/include/libcamera/ipa/raspberrypi.mojom\n> > > > > index 80e0126618c8..7d56248f4ab3 100644\n> > > > > --- a/include/libcamera/ipa/raspberrypi.mojom\n> > > > > +++ b/include/libcamera/ipa/raspberrypi.mojom\n> > > > > @@ -8,7 +8,7 @@ module ipa.RPi;\n> > > > >\n> > > > > import \"include/libcamera/ipa/core.mojom\";\n> > > > >\n> > > > > -/* Size of the LS grid allocation. */\n> > > > > +/* Size of the LS grid allocation on VC4. */\n> > > > > const uint32 MaxLsGridSize = 0x8000;\n> > > > >\n> > > > > struct SensorConfig {\n> > > > > @@ -19,117 +19,78 @@ struct SensorConfig {\n> > > > > uint32 sensorMetadata;\n> > > > > };\n> > > > >\n> > > > > -struct IPAInitResult {\n> > > > > +struct InitParams {\n> > > > > + bool lensPresent;\n> > > > > +};\n> > > > > +\n> > > > > +struct InitResult {\n> > > > > SensorConfig sensorConfig;\n> > > > > libcamera.ControlInfoMap controlInfo;\n> > > > > };\n> > > > >\n> > > > > -struct ISPConfig {\n> > > > > - uint32 embeddedBufferId;\n> > > > > - uint32 bayerBufferId;\n> > > > > - bool embeddedBufferPresent;\n> > > > > - libcamera.ControlList controls;\n> > > > > - uint32 ipaContext;\n> > > > > - uint32 delayContext;\n> > > > > +struct BufferIds {\n> > > > > + uint32 bayer;\n> > > > > + uint32 embedded;\n> > > > > + uint32 stats;\n> > > > > };\n> > > > >\n> > > > > -struct IPAConfig {\n> > > > > +struct ConfigParams {\n> > > > > uint32 transform;\n> > > > > - libcamera.SharedFD lsTableHandle;\n> > > > > libcamera.ControlInfoMap sensorControls;\n> > > > > libcamera.ControlInfoMap ispControls;\n> > > > > libcamera.ControlInfoMap lensControls;\n> > > > > + /* VC4 specifc */\n> > > > > + libcamera.SharedFD lsTableHandle;\n> > > > > };\n> > > > >\n> > > > > -struct IPAConfigResult {\n> > > > > - float modeSensitivity;\n> > > > > - libcamera.ControlInfoMap controlInfo;\n> > > > > +struct ConfigResult {\n> > > > > + float modeSensitivity;\n> > > > > + libcamera.ControlInfoMap controlInfo;\n> > > > > + libcamera.ControlList controls;\n> > > > > };\n> > > > >\n> > > > > -struct StartConfig {\n> > > > > +struct StartResult {\n> > > > > libcamera.ControlList controls;\n> > > > > int32 dropFrameCount;\n> > > > > };\n> > > > >\n> > > > > +struct PrepareParams {\n> > > > > + BufferIds buffers;\n> >\n> > The only part that bothers me a bit is usage of BufferIds here, as the\n> > stats buffer isn't used.\n> \n> Adding the stats buffer to BufferIds allows me to generalise the structure and\n> use it throughout the interface.\n> \n> It is also sort-of ' used right now, in that we see the absence of the stats\n> buffer to \"know\" that the stats are not in-line with prepareISP. However, this\n> is not intended to stay that way, we will have a separate parameter to signal\n> this if needed.\n\nI suppose we can keep it as-is for now, even if it feels a bit like\nshoe-horning a single IPA interface for different needs. The IPA\ninterface mechanism was designed to make it easy for pipeline handlers\nand IPA modules to communicate with custom interfaces :-)\n\n> > > > > + libcamera.ControlList sensorControls;\n> > > > > + libcamera.ControlList requestControls;\n> > > > > + uint32 ipaContext;\n> > > > > + uint32 delayContext;\n> > > > > +};\n> > > > > +\n> > > > > +struct ProcessParams {\n> > > > > + BufferIds buffers;\n> > > > > + uint32 ipaContext;\n> > > > > +};\n> > > > > +\n> > > > > interface IPARPiInterface {\n> > > > > - init(libcamera.IPASettings settings, bool lensPresent)\n> > > > > - => (int32 ret, IPAInitResult result);\n> > > > > - start(libcamera.ControlList controls) => (StartConfig startConfig);\n> > > > > + init(libcamera.IPASettings settings, InitParams params)\n> > > > > + => (int32 ret, InitResult result);\n> > > > > +\n> > > > > + start(libcamera.ControlList controls) => (StartResult result);\n> > > > > stop();\n> > > > >\n> > > > > - /**\n> > > > > - * \\fn configure()\n> > > >\n> > > > Is it intentional to drop documentation ? Was it used at all ?\n> > >\n> > > I don't believe it is at all used. I looked at ipu3 and rkisp1 and they have\n> > > no doc tags either.\n> >\n> > It's not mandatory, but it's nice to have. Could you keep it if that's\n> > not too much trouble ?\n> \n> Ack\n> \n> > > > > - * \\brief Configure the IPA stream and sensor settings\n> > > > > - * \\param[in] sensorInfo Camera sensor information\n> > > > > - * \\param[in] ipaConfig Pipeline-handler-specific configuration data\n> > > > > - * \\param[out] controls Controls to apply by the pipeline entity\n> > > > > - * \\param[out] result Other results that the pipeline handler may require\n> > > > > - *\n> > > > > - * This function shall be called when the camera is configured to inform\n> > > > > - * the IPA of the camera's streams and the sensor settings.\n> > > > > - *\n> > > > > - * The \\a sensorInfo conveys information about the camera sensor settings that\n> > > > > - * the pipeline handler has selected for the configuration.\n> > > > > - *\n> > > > > - * The \\a ipaConfig and \\a controls parameters carry data passed by the\n> > > > > - * pipeline handler to the IPA and back.\n> > > > > - */\n> > > > > - configure(libcamera.IPACameraSensorInfo sensorInfo,\n> > > > > - IPAConfig ipaConfig)\n> > > > > - => (int32 ret, libcamera.ControlList controls, IPAConfigResult result);\n> > > > > -\n> > > > > - /**\n> > > > > - * \\fn mapBuffers()\n> > > > > - * \\brief Map buffers shared between the pipeline handler and the IPA\n> > > > > - * \\param[in] buffers List of buffers to map\n> > > > > - *\n> > > > > - * This function informs the IPA module of memory buffers set up by the\n> > > > > - * pipeline handler that the IPA needs to access. It provides dmabuf\n> > > > > - * file handles for each buffer, and associates the buffers with unique\n> > > > > - * numerical IDs.\n> > > > > - *\n> > > > > - * IPAs shall map the dmabuf file handles to their address space and\n> > > > > - * keep a cache of the mappings, indexed by the buffer numerical IDs.\n> > > > > - * The IDs are used in all other IPA interface functions to refer to\n> > > > > - * buffers, including the unmapBuffers() function.\n> > > > > - *\n> > > > > - * All buffers that the pipeline handler wishes to share with an IPA\n> > > > > - * shall be mapped with this function. Buffers may be mapped all at once\n> > > > > - * with a single call, or mapped and unmapped dynamically at runtime,\n> > > > > - * depending on the IPA protocol. Regardless of the protocol, all\n> > > > > - * buffers mapped at a given time shall have unique numerical IDs.\n> > > > > - *\n> > > > > - * The numerical IDs have no meaning defined by the IPA interface, and\n> > > > > - * should be treated as opaque handles by IPAs, with the only exception\n> > > > > - * that ID zero is invalid.\n> > > > > - *\n> > > > > - * \\sa unmapBuffers()\n> > > > > - */\n> > > > > - mapBuffers(array buffers);\n> > > > > + configure(libcamera.IPACameraSensorInfo sensorInfo, ConfigParams params)\n> > > > > + => (int32 ret, ConfigResult result);\n> > > > >\n> > > > > - /**\n> > > > > - * \\fn unmapBuffers()\n> > > > > - * \\brief Unmap buffers shared by the pipeline to the IPA\n> > > > > - * \\param[in] ids List of buffer IDs to unmap\n> > > > > - *\n> > > > > - * This function removes mappings set up with mapBuffers(). Numerical\n> > > > > - * IDs of unmapped buffers may be reused when mapping new buffers.\n> > > > > - *\n> > > > > - * \\sa mapBuffers()\n> > > > > - */\n> > > > > + mapBuffers(array buffers);\n> > > > > unmapBuffers(array ids);\n> > > > >\n> > > > > - [async] signalStatReady(uint32 bufferId, uint32 ipaContext);\n> > > > > - [async] signalQueueRequest(libcamera.ControlList controls);\n> > > > > - [async] signalIspPrepare(ISPConfig data);\n> > > > > + [async] prepareIsp(PrepareParams params);\n> > > > > + [async] processStats(ProcessParams params);\n> > > > > };\n> > > > >\n> > > > > interface IPARPiEventInterface {\n> > > > > - statsMetadataComplete(uint32 bufferId, libcamera.ControlList controls);\n> > > > > - runIsp(uint32 bufferId);\n> > > > > - embeddedComplete(uint32 bufferId);\n> > > > > + prepareIspComplete(BufferIds buffers);\n> > > > > + processStatsComplete(BufferIds buffers);\n> > > > > + metadataReady(libcamera.ControlList metadata);\n> > > > > setIspControls(libcamera.ControlList controls);\n> > > > > setDelayedControls(libcamera.ControlList controls, uint32 delayContext);\n> > > > > setLensControls(libcamera.ControlList controls);\n> > > > > setCameraTimeout(uint32 maxFrameLengthMs);\n> > > > > };\n> > > > > +\n> > > >\n> > > > Additional blank line\n> > >\n> > > Ack\n> > >\n> > > > The rest looks good\n> > > > Reviewed-by: Jacopo Mondi \n> > > >\n> > > > > diff --git a/src/ipa/rpi/vc4/raspberrypi.cpp b/src/ipa/rpi/vc4/raspberrypi.cpp\n> > > > > index 841635ccde2b..d737f1d662a0 100644\n> > > > > --- a/src/ipa/rpi/vc4/raspberrypi.cpp\n> > > > > +++ b/src/ipa/rpi/vc4/raspberrypi.cpp\n> > > > > @@ -136,30 +136,28 @@ public:\n> > > > > munmap(lsTable_, MaxLsGridSize);\n> > > > > }\n> > > > >\n> > > > > - int init(const IPASettings &settings, bool lensPresent, IPAInitResult *result) override;\n> > > > > - void start(const ControlList &controls, StartConfig *startConfig) override;\n> > > > > + int init(const IPASettings &settings, const InitParams ¶ms, InitResult *result) override;\n> > > > > + void start(const ControlList &controls, StartResult *result) override;\n> > > > > void stop() override {}\n> > > > >\n> > > > > - int configure(const IPACameraSensorInfo &sensorInfo, const IPAConfig &data,\n> > > > > - ControlList *controls, IPAConfigResult *result) override;\n> > > > > + int configure(const IPACameraSensorInfo &sensorInfo, const ConfigParams ¶ms,\n> > > > > + ConfigResult *result) override;\n> > > > > void mapBuffers(const std::vector &buffers) override;\n> > > > > void unmapBuffers(const std::vector &ids) override;\n> > > > > - void signalStatReady(const uint32_t bufferId, uint32_t ipaContext) override;\n> > > > > - void signalQueueRequest(const ControlList &controls) override;\n> > > > > - void signalIspPrepare(const ISPConfig &data) override;\n> > > > > + void prepareIsp(const PrepareParams ¶ms) override;\n> > > > > + void processStats(const ProcessParams ¶ms) override;\n> > > > >\n> > > > > private:\n> > > > > void setMode(const IPACameraSensorInfo &sensorInfo);\n> > > > > bool validateSensorControls();\n> > > > > bool validateIspControls();\n> > > > > bool validateLensControls();\n> > > > > - void queueRequest(const ControlList &controls);\n> > > > > - void returnEmbeddedBuffer(unsigned int bufferId);\n> > > > > - void prepareISP(const ISPConfig &data);\n> > > > > + void applyControls(const ControlList &controls);\n> > > > > + void prepare(const PrepareParams ¶ms);\n> > > > > void reportMetadata(unsigned int ipaContext);\n> > > > > void fillDeviceStatus(const ControlList &sensorControls, unsigned int ipaContext);\n> > > > > RPiController::StatisticsPtr fillStatistics(bcm2835_isp_stats *stats) const;\n> > > > > - void processStats(unsigned int bufferId, unsigned int ipaContext);\n> > > > > + void process(unsigned int bufferId, unsigned int ipaContext);\n> > > > > void setCameraTimeoutValue();\n> > > > > void applyFrameDurations(Duration minFrameDuration, Duration maxFrameDuration);\n> > > > > void applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls);\n> > > > > @@ -229,7 +227,7 @@ private:\n> > > > > Duration lastTimeout_;\n> > > > > };\n> > > > >\n> > > > > -int IPARPi::init(const IPASettings &settings, bool lensPresent, IPAInitResult *result)\n> > > > > +int IPARPi::init(const IPASettings &settings, const InitParams ¶ms, InitResult *result)\n> > > > > {\n> > > > > /*\n> > > > > * Load the \"helper\" for this sensor. This tells us all the device specific stuff\n> > > > > @@ -274,7 +272,7 @@ int IPARPi::init(const IPASettings &settings, bool lensPresent, IPAInitResult *r\n> > > > > return -EINVAL;\n> > > > > }\n> > > > >\n> > > > > - lensPresent_ = lensPresent;\n> > > > > + lensPresent_ = params.lensPresent;\n> > > > >\n> > > > > controller_.initialise();\n> > > > >\n> > > > > @@ -287,14 +285,13 @@ int IPARPi::init(const IPASettings &settings, bool lensPresent, IPAInitResult *r\n> > > > > return 0;\n> > > > > }\n> > > > >\n> > > > > -void IPARPi::start(const ControlList &controls, StartConfig *startConfig)\n> > > > > +void IPARPi::start(const ControlList &controls, StartResult *result)\n> > > > > {\n> > > > > RPiController::Metadata metadata;\n> > > > >\n> > > > > - ASSERT(startConfig);\n> > > > > if (!controls.empty()) {\n> > > > > /* We have been given some controls to action before start. */\n> > > > > - queueRequest(controls);\n> > > > > + applyControls(controls);\n> > > > > }\n> > > > >\n> > > > > controller_.switchMode(mode_, &metadata);\n> > > > > @@ -313,7 +310,7 @@ void IPARPi::start(const ControlList &controls, StartConfig *startConfig)\n> > > > > if (agcStatus.shutterTime && agcStatus.analogueGain) {\n> > > > > ControlList ctrls(sensorCtrls_);\n> > > > > applyAGC(&agcStatus, ctrls);\n> > > > > - startConfig->controls = std::move(ctrls);\n> > > > > + result->controls = std::move(ctrls);\n> > > > > setCameraTimeoutValue();\n> > > > > }\n> > > > >\n> > > > > @@ -360,7 +357,7 @@ void IPARPi::start(const ControlList &controls, StartConfig *startConfig)\n> > > > > mistrustCount_ = helper_->mistrustFramesModeSwitch();\n> > > > > }\n> > > > >\n> > > > > - startConfig->dropFrameCount = dropFrameCount_;\n> > > > > + result->dropFrameCount = dropFrameCount_;\n> > > > >\n> > > > > firstStart_ = false;\n> > > > > lastRunTimestamp_ = 0;\n> > > > > @@ -435,11 +432,11 @@ void IPARPi::setMode(const IPACameraSensorInfo &sensorInfo)\n> > > > > mode_.minFrameDuration, mode_.maxFrameDuration);\n> > > > > }\n> > > > >\n> > > > > -int IPARPi::configure(const IPACameraSensorInfo &sensorInfo, const IPAConfig &ipaConfig,\n> > > > > - ControlList *controls, IPAConfigResult *result)\n> > > > > +int IPARPi::configure(const IPACameraSensorInfo &sensorInfo, const ConfigParams ¶ms,\n> > > > > + ConfigResult *result)\n> > > > > {\n> > > > > - sensorCtrls_ = ipaConfig.sensorControls;\n> > > > > - ispCtrls_ = ipaConfig.ispControls;\n> > > > > + sensorCtrls_ = params.sensorControls;\n> > > > > + ispCtrls_ = params.ispControls;\n> > > > >\n> > > > > if (!validateSensorControls()) {\n> > > > > LOG(IPARPI, Error) << \"Sensor control validation failed.\";\n> > > > > @@ -452,7 +449,7 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo, const IPAConfig &ip\n> > > > > }\n> > > > >\n> > > > > if (lensPresent_) {\n> > > > > - lensCtrls_ = ipaConfig.lensControls;\n> > > > > + lensCtrls_ = params.lensControls;\n> > > > > if (!validateLensControls()) {\n> > > > > LOG(IPARPI, Warning) << \"Lens validation failed, \"\n> > > > > << \"no lens control will be available.\";\n> > > > > @@ -466,10 +463,10 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo, const IPAConfig &ip\n> > > > > /* Re-assemble camera mode using the sensor info. */\n> > > > > setMode(sensorInfo);\n> > > > >\n> > > > > - mode_.transform = static_cast(ipaConfig.transform);\n> > > > > + mode_.transform = static_cast(params.transform);\n> > > > >\n> > > > > /* Store the lens shading table pointer and handle if available. */\n> > > > > - if (ipaConfig.lsTableHandle.isValid()) {\n> > > > > + if (params.lsTableHandle.isValid()) {\n> > > > > /* Remove any previous table, if there was one. */\n> > > > > if (lsTable_) {\n> > > > > munmap(lsTable_, MaxLsGridSize);\n> > > > > @@ -477,7 +474,7 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo, const IPAConfig &ip\n> > > > > }\n> > > > >\n> > > > > /* Map the LS table buffer into user space. */\n> > > > > - lsTableHandle_ = std::move(ipaConfig.lsTableHandle);\n> > > > > + lsTableHandle_ = std::move(params.lsTableHandle);\n> > > > > if (lsTableHandle_.isValid()) {\n> > > > > lsTable_ = mmap(nullptr, MaxLsGridSize, PROT_READ | PROT_WRITE,\n> > > > > MAP_SHARED, lsTableHandle_.get(), 0);\n> > > > > @@ -512,8 +509,7 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo, const IPAConfig &ip\n> > > > > applyAGC(&agcStatus, ctrls);\n> > > > > }\n> > > > >\n> > > > > - ASSERT(controls);\n> > > > > - *controls = std::move(ctrls);\n> > > > > + result->controls = std::move(ctrls);\n> > > > >\n> > > > > /*\n> > > > > * Apply the correct limits to the exposure, gain and frame duration controls\n> > > > > @@ -560,37 +556,34 @@ void IPARPi::unmapBuffers(const std::vector &ids)\n> > > > > }\n> > > > > }\n> > > > >\n> > > > > -void IPARPi::signalStatReady(uint32_t bufferId, uint32_t ipaContext)\n> > > > > +void IPARPi::processStats(const ProcessParams ¶ms)\n> > > > > {\n> > > > > - unsigned int context = ipaContext % rpiMetadata_.size();\n> > > > > + unsigned int context = params.ipaContext % rpiMetadata_.size();\n> > > > >\n> > > > > if (++checkCount_ != frameCount_) /* assert here? */\n> > > > > LOG(IPARPI, Error) << \"WARNING: Prepare/Process mismatch!!!\";\n> > > > > if (processPending_ && frameCount_ > mistrustCount_)\n> > > > > - processStats(bufferId, context);\n> > > > > + process(params.buffers.stats, context);\n> > > > >\n> > > > > reportMetadata(context);\n> > > > > -\n> > > > > - statsMetadataComplete.emit(bufferId, libcameraMetadata_);\n> > > > > + processStatsComplete.emit(params.buffers);\n> > > > > }\n> > > > >\n> > > > > -void IPARPi::signalQueueRequest(const ControlList &controls)\n> > > > > -{\n> > > > > - queueRequest(controls);\n> > > > > -}\n> > > > >\n> > > > > -void IPARPi::signalIspPrepare(const ISPConfig &data)\n> > > > > +void IPARPi::prepareIsp(const PrepareParams ¶ms)\n> > > > > {\n> > > > > + applyControls(params.requestControls);\n> > > > > +\n> > > > > /*\n> > > > > * At start-up, or after a mode-switch, we may want to\n> > > > > * avoid running the control algos for a few frames in case\n> > > > > * they are \"unreliable\".\n> > > > > */\n> > > > > - prepareISP(data);\n> > > > > + prepare(params);\n> > > > > frameCount_++;\n> > > > >\n> > > > > /* Ready to push the input buffer into the ISP. */\n> > > > > - runIsp.emit(data.bayerBufferId);\n> > > > > + prepareIspComplete.emit(params.buffers);\n> > > > > }\n> > > > >\n> > > > > void IPARPi::reportMetadata(unsigned int ipaContext)\n> > > > > @@ -703,6 +696,8 @@ void IPARPi::reportMetadata(unsigned int ipaContext)\n> > > > > libcameraMetadata_.set(controls::AfState, s);\n> > > > > libcameraMetadata_.set(controls::AfPauseState, p);\n> > > > > }\n> > > > > +\n> > > > > + metadataReady.emit(libcameraMetadata_);\n> > > > > }\n> > > > >\n> > > > > bool IPARPi::validateSensorControls()\n> > > > > @@ -826,7 +821,7 @@ static const std::map AfPauseTable\n> > > > > { controls::AfPauseResume, RPiController::AfAlgorithm::AfPauseResume },\n> > > > > };\n> > > > >\n> > > > > -void IPARPi::queueRequest(const ControlList &controls)\n> > > > > +void IPARPi::applyControls(const ControlList &controls)\n> > > > > {\n> > > > > using RPiController::AfAlgorithm;\n> > > > >\n> > > > > @@ -1256,27 +1251,22 @@ void IPARPi::queueRequest(const ControlList &controls)\n> > > > > }\n> > > > > }\n> > > > >\n> > > > > -void IPARPi::returnEmbeddedBuffer(unsigned int bufferId)\n> > > > > +void IPARPi::prepare(const PrepareParams ¶ms)\n> > > > > {\n> > > > > - embeddedComplete.emit(bufferId);\n> > > > > -}\n> > > > > -\n> > > > > -void IPARPi::prepareISP(const ISPConfig &data)\n> > > > > -{\n> > > > > - int64_t frameTimestamp = data.controls.get(controls::SensorTimestamp).value_or(0);\n> > > > > - unsigned int ipaContext = data.ipaContext % rpiMetadata_.size();\n> > > > > + int64_t frameTimestamp = params.sensorControls.get(controls::SensorTimestamp).value_or(0);\n> > > > > + unsigned int ipaContext = params.ipaContext % rpiMetadata_.size();\n> > > > > RPiController::Metadata &rpiMetadata = rpiMetadata_[ipaContext];\n> > > > > Span embeddedBuffer;\n> > > > >\n> > > > > rpiMetadata.clear();\n> > > > > - fillDeviceStatus(data.controls, ipaContext);\n> > > > > + fillDeviceStatus(params.sensorControls, ipaContext);\n> > > > >\n> > > > > - if (data.embeddedBufferPresent) {\n> > > > > + if (params.buffers.embedded) {\n> > > > > /*\n> > > > > * Pipeline handler has supplied us with an embedded data buffer,\n> > > > > * we must pass it to the CamHelper for parsing.\n> > > > > */\n> > > > > - auto it = buffers_.find(data.embeddedBufferId);\n> > > > > + auto it = buffers_.find(params.buffers.embedded);\n> > > > > ASSERT(it != buffers_.end());\n> > > > > embeddedBuffer = it->second.planes()[0];\n> > > > > }\n> > > > > @@ -1288,7 +1278,7 @@ void IPARPi::prepareISP(const ISPConfig &data)\n> > > > > * metadata.\n> > > > > */\n> > > > > AgcStatus agcStatus;\n> > > > > - RPiController::Metadata &delayedMetadata = rpiMetadata_[data.delayContext];\n> > > > > + RPiController::Metadata &delayedMetadata = rpiMetadata_[params.delayContext];\n> > > > > if (!delayedMetadata.get(\"agc.status\", agcStatus))\n> > > > > rpiMetadata.set(\"agc.delayed_status\", agcStatus);\n> > > > >\n> > > > > @@ -1298,10 +1288,6 @@ void IPARPi::prepareISP(const ISPConfig &data)\n> > > > > */\n> > > > > helper_->prepare(embeddedBuffer, rpiMetadata);\n> > > > >\n> > > > > - /* Done with embedded data now, return to pipeline handler asap. */\n> > > > > - if (data.embeddedBufferPresent)\n> > > > > - returnEmbeddedBuffer(data.embeddedBufferId);\n> > > > > -\n> > > > > /* Allow a 10% margin on the comparison below. */\n> > > > > Duration delta = (frameTimestamp - lastRunTimestamp_) * 1.0ns;\n> > > > > if (lastRunTimestamp_ && frameCount_ > dropFrameCount_ &&\n> > > > > @@ -1445,7 +1431,7 @@ RPiController::StatisticsPtr IPARPi::fillStatistics(bcm2835_isp_stats *stats) co\n> > > > > return statistics;\n> > > > > }\n> > > > >\n> > > > > -void IPARPi::processStats(unsigned int bufferId, unsigned int ipaContext)\n> > > > > +void IPARPi::process(unsigned int bufferId, unsigned int ipaContext)\n> > > > > {\n> > > > > RPiController::Metadata &rpiMetadata = rpiMetadata_[ipaContext];\n> > > > >\n> > > > > diff --git a/src/libcamera/pipeline/rpi/vc4/raspberrypi.cpp b/src/libcamera/pipeline/rpi/vc4/raspberrypi.cpp\n> > > > > index e54bf1ef2c17..30ce6feab0d1 100644\n> > > > > --- a/src/libcamera/pipeline/rpi/vc4/raspberrypi.cpp\n> > > > > +++ b/src/libcamera/pipeline/rpi/vc4/raspberrypi.cpp\n> > > > > @@ -200,15 +200,15 @@ public:\n> > > > > void freeBuffers();\n> > > > > void frameStarted(uint32_t sequence);\n> > > > >\n> > > > > - int loadIPA(ipa::RPi::IPAInitResult *result);\n> > > > > - int configureIPA(const CameraConfiguration *config, ipa::RPi::IPAConfigResult *result);\n> > > > > + int loadIPA(ipa::RPi::InitResult *result);\n> > > > > + int configureIPA(const CameraConfiguration *config, ipa::RPi::ConfigResult *result);\n> > > > > int loadPipelineConfiguration();\n> > > > >\n> > > > > void enumerateVideoDevices(MediaLink *link);\n> > > > >\n> > > > > - void statsMetadataComplete(uint32_t bufferId, const ControlList &controls);\n> > > > > - void runIsp(uint32_t bufferId);\n> > > > > - void embeddedComplete(uint32_t bufferId);\n> > > > > + void processStatsComplete(const ipa::RPi::BufferIds &buffers);\n> > > > > + void metadataReady(const ControlList &metadata);\n> > > > > + void prepareIspComplete(const ipa::RPi::BufferIds &buffers);\n> > > > > void setIspControls(const ControlList &controls);\n> > > > > void setDelayedControls(const ControlList &controls, uint32_t delayContext);\n> > > > > void setLensControls(const ControlList &controls);\n> > > > > @@ -238,7 +238,7 @@ public:\n> > > > > /* The vector below is just for convenience when iterating over all streams. */\n> > > > > std::vector streams_;\n> > > > > /* Stores the ids of the buffers mapped in the IPA. */\n> > > > > - std::unordered_set ipaBuffers_;\n> > > > > + std::unordered_set bufferIds_;\n> > > > > /*\n> > > > > * Stores a cascade of Video Mux or Bridge devices between the sensor and\n> > > > > * Unicam together with media link across the entities.\n> > > > > @@ -1000,7 +1000,7 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)\n> > > > >\n> > > > > data->isp_[Isp::Input].dev()->setSelection(V4L2_SEL_TGT_CROP, &crop);\n> > > > >\n> > > > > - ipa::RPi::IPAConfigResult result;\n> > > > > + ipa::RPi::ConfigResult result;\n> > > > > ret = data->configureIPA(config, &result);\n> > > > > if (ret)\n> > > > > LOG(RPI, Error) << \"Failed to configure the IPA: \" << ret;\n> > > > > @@ -1117,17 +1117,17 @@ int PipelineHandlerRPi::start(Camera *camera, const ControlList *controls)\n> > > > > data->applyScalerCrop(*controls);\n> > > > >\n> > > > > /* Start the IPA. */\n> > > > > - ipa::RPi::StartConfig startConfig;\n> > > > > + ipa::RPi::StartResult result;\n> > > > > data->ipa_->start(controls ? *controls : ControlList{ controls::controls },\n> > > > > - &startConfig);\n> > > > > + &result);\n> > > > >\n> > > > > /* Apply any gain/exposure settings that the IPA may have passed back. */\n> > > > > - if (!startConfig.controls.empty())\n> > > > > - data->setSensorControls(startConfig.controls);\n> > > > > + if (!result.controls.empty())\n> > > > > + data->setSensorControls(result.controls);\n> > > > >\n> > > > > /* Configure the number of dropped frames required on startup. */\n> > > > > data->dropFrameCount_ = data->config_.disableStartupFrameDrops\n> > > > > - ? 0 : startConfig.dropFrameCount;\n> > > > > + ? 0 : result.dropFrameCount;\n> > > > >\n> > > > > for (auto const stream : data->streams_)\n> > > > > stream->resetBuffers();\n> > > > > @@ -1358,7 +1358,7 @@ int PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp, Me\n> > > > >\n> > > > > data->sensorFormats_ = populateSensorFormats(data->sensor_);\n> > > > >\n> > > > > - ipa::RPi::IPAInitResult result;\n> > > > > + ipa::RPi::InitResult result;\n> > > > > if (data->loadIPA(&result)) {\n> > > > > LOG(RPI, Error) << \"Failed to load a suitable IPA library\";\n> > > > > return -EINVAL;\n> > > > > @@ -1599,7 +1599,7 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)\n> > > > > void PipelineHandlerRPi::mapBuffers(Camera *camera, const RPi::BufferMap &buffers, unsigned int mask)\n> > > > > {\n> > > > > RPiCameraData *data = cameraData(camera);\n> > > > > - std::vector ipaBuffers;\n> > > > > + std::vector bufferIds;\n> > > > > /*\n> > > > > * Link the FrameBuffers with the id (key value) in the map stored in\n> > > > > * the RPi stream object - along with an identifier mask.\n> > > > > @@ -1608,12 +1608,12 @@ void PipelineHandlerRPi::mapBuffers(Camera *camera, const RPi::BufferMap &buffer\n> > > > > * handler and the IPA.\n> > > > > */\n> > > > > for (auto const &it : buffers) {\n> > > > > - ipaBuffers.push_back(IPABuffer(mask | it.first,\n> > > > > + bufferIds.push_back(IPABuffer(mask | it.first,\n> > > > > it.second->planes()));\n> > > > > - data->ipaBuffers_.insert(mask | it.first);\n> > > > > + data->bufferIds_.insert(mask | it.first);\n> > > > > }\n> > > > >\n> > > > > - data->ipa_->mapBuffers(ipaBuffers);\n> > > > > + data->ipa_->mapBuffers(bufferIds);\n> > > > > }\n> > > > >\n> > > > > void RPiCameraData::freeBuffers()\n> > > > > @@ -1623,10 +1623,10 @@ void RPiCameraData::freeBuffers()\n> > > > > * Copy the buffer ids from the unordered_set to a vector to\n> > > > > * pass to the IPA.\n> > > > > */\n> > > > > - std::vector ipaBuffers(ipaBuffers_.begin(),\n> > > > > - ipaBuffers_.end());\n> > > > > - ipa_->unmapBuffers(ipaBuffers);\n> > > > > - ipaBuffers_.clear();\n> > > > > + std::vector bufferIds(bufferIds_.begin(),\n> > > > > + bufferIds_.end());\n> > > > > + ipa_->unmapBuffers(bufferIds);\n> > > > > + bufferIds_.clear();\n> > > > > }\n> > > > >\n> > > > > for (auto const stream : streams_)\n> > > > > @@ -1643,16 +1643,16 @@ void RPiCameraData::frameStarted(uint32_t sequence)\n> > > > > delayedCtrls_->applyControls(sequence);\n> > > > > }\n> > > > >\n> > > > > -int RPiCameraData::loadIPA(ipa::RPi::IPAInitResult *result)\n> > > > > +int RPiCameraData::loadIPA(ipa::RPi::InitResult *result)\n> > > > > {\n> > > > > ipa_ = IPAManager::createIPA(pipe(), 1, 1);\n> > > > >\n> > > > > if (!ipa_)\n> > > > > return -ENOENT;\n> > > > >\n> > > > > - ipa_->statsMetadataComplete.connect(this, &RPiCameraData::statsMetadataComplete);\n> > > > > - ipa_->runIsp.connect(this, &RPiCameraData::runIsp);\n> > > > > - ipa_->embeddedComplete.connect(this, &RPiCameraData::embeddedComplete);\n> > > > > + ipa_->processStatsComplete.connect(this, &RPiCameraData::processStatsComplete);\n> > > > > + ipa_->prepareIspComplete.connect(this, &RPiCameraData::prepareIspComplete);\n> > > > > + ipa_->metadataReady.connect(this, &RPiCameraData::metadataReady);\n> > > > > ipa_->setIspControls.connect(this, &RPiCameraData::setIspControls);\n> > > > > ipa_->setDelayedControls.connect(this, &RPiCameraData::setDelayedControls);\n> > > > > ipa_->setLensControls.connect(this, &RPiCameraData::setLensControls);\n> > > > > @@ -1674,23 +1674,25 @@ int RPiCameraData::loadIPA(ipa::RPi::IPAInitResult *result)\n> > > > > }\n> > > > >\n> > > > > IPASettings settings(configurationFile, sensor_->model());\n> > > > > + ipa::RPi::InitParams params;\n> > > > >\n> > > > > - return ipa_->init(settings, !!sensor_->focusLens(), result);\n> > > > > + params.lensPresent = !!sensor_->focusLens();\n> > > > > + return ipa_->init(settings, params, result);\n> > > > > }\n> > > > >\n> > > > > -int RPiCameraData::configureIPA(const CameraConfiguration *config, ipa::RPi::IPAConfigResult *result)\n> > > > > +int RPiCameraData::configureIPA(const CameraConfiguration *config, ipa::RPi::ConfigResult *result)\n> > > > > {\n> > > > > std::map entityControls;\n> > > > > - ipa::RPi::IPAConfig ipaConfig;\n> > > > > + ipa::RPi::ConfigParams params;\n> > > > >\n> > > > > /* \\todo Move passing of ispControls and lensControls to ipa::init() */\n> > > > > - ipaConfig.sensorControls = sensor_->controls();\n> > > > > - ipaConfig.ispControls = isp_[Isp::Input].dev()->controls();\n> > > > > + params.sensorControls = sensor_->controls();\n> > > > > + params.ispControls = isp_[Isp::Input].dev()->controls();\n> > > > > if (sensor_->focusLens())\n> > > > > - ipaConfig.lensControls = sensor_->focusLens()->controls();\n> > > > > + params.lensControls = sensor_->focusLens()->controls();\n> > > > >\n> > > > > /* Always send the user transform to the IPA. */\n> > > > > - ipaConfig.transform = static_cast(config->transform);\n> > > > > + params.transform = static_cast(config->transform);\n> > > > >\n> > > > > /* Allocate the lens shading table via dmaHeap and pass to the IPA. */\n> > > > > if (!lsTable_.isValid()) {\n> > > > > @@ -1703,7 +1705,7 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config, ipa::RPi::IPA\n> > > > > * \\todo Investigate if mapping the lens shading table buffer\n> > > > > * could be handled with mapBuffers().\n> > > > > */\n> > > > > - ipaConfig.lsTableHandle = lsTable_;\n> > > > > + params.lsTableHandle = lsTable_;\n> > > > > }\n> > > > >\n> > > > > /* We store the IPACameraSensorInfo for digital zoom calculations. */\n> > > > > @@ -1714,15 +1716,14 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config, ipa::RPi::IPA\n> > > > > }\n> > > > >\n> > > > > /* Ready the IPA - it must know about the sensor resolution. */\n> > > > > - ControlList controls;\n> > > > > - ret = ipa_->configure(sensorInfo_, ipaConfig, &controls, result);\n> > > > > + ret = ipa_->configure(sensorInfo_, params, result);\n> > > > > if (ret < 0) {\n> > > > > LOG(RPI, Error) << \"IPA configuration failed!\";\n> > > > > return -EPIPE;\n> > > > > }\n> > > > >\n> > > > > - if (!controls.empty())\n> > > > > - setSensorControls(controls);\n> > > > > + if (!result->controls.empty())\n> > > > > + setSensorControls(result->controls);\n> > > > >\n> > > > > return 0;\n> > > > > }\n> > > > > @@ -1883,24 +1884,32 @@ void RPiCameraData::enumerateVideoDevices(MediaLink *link)\n> > > > > }\n> > > > > }\n> > > > >\n> > > > > -void RPiCameraData::statsMetadataComplete(uint32_t bufferId, const ControlList &controls)\n> > > > > +void RPiCameraData::processStatsComplete(const ipa::RPi::BufferIds &buffers)\n> > > > > {\n> > > > > if (!isRunning())\n> > > > > return;\n> > > > >\n> > > > > - FrameBuffer *buffer = isp_[Isp::Stats].getBuffers().at(bufferId & RPi::MaskID);\n> > > > > + FrameBuffer *buffer = isp_[Isp::Stats].getBuffers().at(buffers.stats & RPi::MaskID);\n> > > > >\n> > > > > handleStreamBuffer(buffer, &isp_[Isp::Stats]);\n> > > > > + state_ = State::IpaComplete;\n> > > > > + handleState();\n> > > > > +}\n> > > > > +\n> > > > > +void RPiCameraData::metadataReady(const ControlList &metadata)\n> > > > > +{\n> > > > > + if (!isRunning())\n> > > > > + return;\n> > > > >\n> > > > > /* Add to the Request metadata buffer what the IPA has provided. */\n> > > > > Request *request = requestQueue_.front();\n> > > > > - request->metadata().merge(controls);\n> > > > > + request->metadata().merge(metadata);\n> > > > >\n> > > > > /*\n> > > > > * Inform the sensor of the latest colour gains if it has the\n> > > > > * V4L2_CID_NOTIFY_GAINS control (which means notifyGainsUnity_ is set).\n> > > > > */\n> > > > > - const auto &colourGains = controls.get(libcamera::controls::ColourGains);\n> > > > > + const auto &colourGains = metadata.get(libcamera::controls::ColourGains);\n> > > > > if (notifyGainsUnity_ && colourGains) {\n> > > > > /* The control wants linear gains in the order B, Gb, Gr, R. */\n> > > > > ControlList ctrls(sensor_->controls());\n> > > > > @@ -1914,33 +1923,29 @@ void RPiCameraData::statsMetadataComplete(uint32_t bufferId, const ControlList &\n> > > > >\n> > > > > sensor_->setControls(&ctrls);\n> > > > > }\n> > > > > -\n> > > > > - state_ = State::IpaComplete;\n> > > > > - handleState();\n> > > > > }\n> > > > >\n> > > > > -void RPiCameraData::runIsp(uint32_t bufferId)\n> > > > > +void RPiCameraData::prepareIspComplete(const ipa::RPi::BufferIds &buffers)\n> > > > > {\n> > > > > + unsigned int embeddedId = buffers.embedded & RPi::MaskID;\n> > > > > + unsigned int bayer = buffers.bayer & RPi::MaskID;\n> > > > > + FrameBuffer *buffer;\n> > > > > +\n> > > > > if (!isRunning())\n> > > > > return;\n> > > > >\n> > > > > - FrameBuffer *buffer = unicam_[Unicam::Image].getBuffers().at(bufferId & RPi::MaskID);\n> > > > > -\n> > > > > - LOG(RPI, Debug) << \"Input re-queue to ISP, buffer id \" << (bufferId & RPi::MaskID)\n> > > > > + buffer = unicam_[Unicam::Image].getBuffers().at(bayer & RPi::MaskID);\n> > > > > + LOG(RPI, Debug) << \"Input re-queue to ISP, buffer id \" << (bayer & RPi::MaskID)\n> > > > > << \", timestamp: \" << buffer->metadata().timestamp;\n> > > > >\n> > > > > isp_[Isp::Input].queueBuffer(buffer);\n> > > > > ispOutputCount_ = 0;\n> > > > > - handleState();\n> > > > > -}\n> > > > >\n> > > > > -void RPiCameraData::embeddedComplete(uint32_t bufferId)\n> > > > > -{\n> > > > > - if (!isRunning())\n> > > > > - return;\n> > > > > + if (sensorMetadata_ && embeddedId) {\n> > > > > + buffer = unicam_[Unicam::Embedded].getBuffers().at(embeddedId & RPi::MaskID);\n> > > > > + handleStreamBuffer(buffer, &unicam_[Unicam::Embedded]);\n> > > > > + }\n> > > > >\n> > > > > - FrameBuffer *buffer = unicam_[Unicam::Embedded].getBuffers().at(bufferId & RPi::MaskID);\n> > > > > - handleStreamBuffer(buffer, &unicam_[Unicam::Embedded]);\n> > > > > handleState();\n> > > > > }\n> > > > >\n> > > > > @@ -2116,8 +2121,10 @@ void RPiCameraData::ispOutputDequeue(FrameBuffer *buffer)\n> > > > > * application until after the IPA signals so.\n> > > > > */\n> > > > > if (stream == &isp_[Isp::Stats]) {\n> > > > > - ipa_->signalStatReady(RPi::MaskStats | static_cast(index),\n> > > > > - requestQueue_.front()->sequence());\n> > > > > + ipa::RPi::ProcessParams params;\n> > > > > + params.buffers.stats = index | RPi::MaskStats;\n> > > > > + params.ipaContext = requestQueue_.front()->sequence();\n> > > > > + ipa_->processStats(params);\n> > > > > } else {\n> > > > > /* Any other ISP output can be handed back to the application now. */\n> > > > > handleStreamBuffer(buffer, stream);\n> > > > > @@ -2344,38 +2351,30 @@ void RPiCameraData::tryRunPipeline()\n> > > > > request->metadata().clear();\n> > > > > fillRequestMetadata(bayerFrame.controls, request);\n> > > > >\n> > > > > - /*\n> > > > > - * Process all the user controls by the IPA. Once this is complete, we\n> > > > > - * queue the ISP output buffer listed in the request to start the HW\n> > > > > - * pipeline.\n> > > > > - */\n> > > > > - ipa_->signalQueueRequest(request->controls());\n> > > > > -\n> > > > > /* Set our state to say the pipeline is active. */\n> > > > > state_ = State::Busy;\n> > > > >\n> > > > > - unsigned int bayerId = unicam_[Unicam::Image].getBufferId(bayerFrame.buffer);\n> > > > > + unsigned int bayer = unicam_[Unicam::Image].getBufferId(bayerFrame.buffer);\n> > > > >\n> > > > > - LOG(RPI, Debug) << \"Signalling signalIspPrepare:\"\n> > > > > - << \" Bayer buffer id: \" << bayerId;\n> > > > > + LOG(RPI, Debug) << \"Signalling prepareIsp:\"\n> > > > > + << \" Bayer buffer id: \" << bayer;\n> > > > >\n> > > > > - ipa::RPi::ISPConfig ispPrepare;\n> > > > > - ispPrepare.bayerBufferId = RPi::MaskBayerData | bayerId;\n> > > > > - ispPrepare.controls = std::move(bayerFrame.controls);\n> > > > > - ispPrepare.ipaContext = request->sequence();\n> > > > > - ispPrepare.delayContext = bayerFrame.delayContext;\n> > > > > + ipa::RPi::PrepareParams params;\n> > > > > + params.buffers.bayer = RPi::MaskBayerData | bayer;\n> > > > > + params.sensorControls = std::move(bayerFrame.controls);\n> > > > > + params.requestControls = request->controls();\n> > > > > + params.ipaContext = request->sequence();\n> > > > > + params.delayContext = bayerFrame.delayContext;\n> > > > >\n> > > > > if (embeddedBuffer) {\n> > > > > unsigned int embeddedId = unicam_[Unicam::Embedded].getBufferId(embeddedBuffer);\n> > > > >\n> > > > > - ispPrepare.embeddedBufferId = RPi::MaskEmbeddedData | embeddedId;\n> > > > > - ispPrepare.embeddedBufferPresent = true;\n> > > > > -\n> > > > > - LOG(RPI, Debug) << \"Signalling signalIspPrepare:\"\n> > > > > + params.buffers.embedded = RPi::MaskEmbeddedData | embeddedId;\n> > > > > + LOG(RPI, Debug) << \"Signalling prepareIsp:\"\n> > > > > << \" Embedded buffer id: \" << embeddedId;\n> > > > > }\n> > > > >\n> > > > > - ipa_->signalIspPrepare(ispPrepare);\n> > > > > + ipa_->prepareIsp(params);\n> > > > > }\n> > > > >\n> > > > > bool RPiCameraData::findMatchingBuffers(BayerFrame &bayerFrame, FrameBuffer *&embeddedBuffer)","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 33291C0DA4\n\tfor ;\n\tTue, 2 May 2023 15:37:21 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9DE67633AC;\n\tTue, 2 May 2023 17:37:20 +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 63187627DC\n\tfor ;\n\tTue, 2 May 2023 17:37:19 +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 42220800;\n\tTue, 2 May 2023 17:37:15 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1683041840;\n\tbh=9D2/US8fpWCmSXaRbwITIt2Q1i7Uek41Ca30ZLFc+cQ=;\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=KNxYcS9nOooBAjCi+DwALHkUPn9e328buAePsD/xs1sJgGdvEy1oTdY8chfqMUc90\n\tqhq8j8KvJvCEaT5nEl1yfTzF9i9HYpbBpp6gWBvDBtW47UT/6WT1aNHCGkY+AJXwVD\n\tMk92Ps90YXnBPHROyusTLv8UaL37Z8b7FJL1r82BxKvVT98Hfgg1F0QcX52Yot+tG5\n\tZVXZSuWFz0M6mo13fUSwout0sNPnyAX58xLrBf/urf632Vf6ymfX4vlk3RJNRRU5bH\n\tdOz9In25mbZBCkZ+ENGjAU2rWnPgnGqlcbchzrf5j7BeEolx5RwUmSUfXHkcGw7kgH\n\tURDFsPSIDvUUw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1683041837;\n\tbh=9D2/US8fpWCmSXaRbwITIt2Q1i7Uek41Ca30ZLFc+cQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Ife2UmN7hZlOaVhvMwv33vaLx8oJh1ZmIeQc+lhkyAAdqDjbeD5SWhzPg6ZJb7E8x\n\tyiSNje2GIO54sJmCtjRnrrKvEUOhfBt19qh6thB+UZtmSwMoPwEsuTrpPlbrkIxjiZ\n\tPPr8EBA+/7ofTFNmiDpHQAdCkSh+shMhtz03XZdE="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"Ife2UmN7\"; dkim-atps=neutral","Date":"Tue, 2 May 2023 18:37:30 +0300","To":"Naushir Patuck ","Message-ID":"<20230502153730.GC22691@pendragon.ideasonboard.com>","References":"<20230426131057.21550-1-naush@raspberrypi.com>\n\t<20230426131057.21550-7-naush@raspberrypi.com>\n\t<26csbh5y5qh7r4rle34eyn3drneqvvifcqb65s5nppg5ifm6kv@ft5oy4xnkzu4>\n\t\n\t<20230427171521.GC26786@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 06/13] pipeline: ipa: raspberrypi:\n\tRestructure the IPA mojom interface","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\" "}}]