[{"id":22650,"web_url":"https://patchwork.libcamera.org/comment/22650/","msgid":"<164936949446.22830.7307396101214349800@Monstersaurus>","date":"2022-04-07T22:11:34","subject":"Re: [libcamera-devel] [PATCH v5 1/6] ipa: ipu3: Replace event-based\n\tops with dedicated functions","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Umang Jain via libcamera-devel (2022-04-06 15:17:04)\n> The IPAIPU3 interface currently uses event-type based structures in\n> order to communicate with the pipeline-handler (and vice-versa).\n> Replace the event based structures with dedicated functions associated\n> to each operation.\n> \n> The translated naming scheme of operations to dedicated functions:\n\nSo I think these without () are signals? If so we should state that here?\n\nThe Actions are translated to signals:\n> ActionSetSensorControls => setSensorControls\n> ActionParamFilled => paramsBufferReady\n> ActionMetadataReady => metadataReady\n\nAnd Events are translated to dedicated functions:\n\n> EventProcessControls => queueRequest()\n> EventStatReady => processStatsBuffer()\n> EventFillParams => fillParamsBuffer()\n> \n\nIs there really a distinction between the use of signals and functions\nin either direction? or is it ultimately just signals both ways? (Or is\nit signals connected to functions of the same name? in both ways?)\n\nAnyway - that bit doesn't really matter it's just the commit message.\n\nIt's so hard to parse this patch. We really need to make sure something\nlike this is split from the start next time.\n\nI have one question about paramsBufferReady below - but I can't see\nanything else standing out so with that resolved or otherwise:\n\nReviewed-by: Kieran Bingham \n\n> Signed-off-by: Umang Jain \n> Reviewed-by: Laurent Pinchart \n> ---\n> include/libcamera/ipa/ipu3.mojom | 36 ++-----\n> src/ipa/ipu3/ipu3-ipa-design-guide.rst | 27 +++---\n> src/ipa/ipu3/ipu3.cpp | 129 +++++++++++--------------\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 122 ++++++++++-------------\n> 4 files changed, 130 insertions(+), 184 deletions(-)\n> \n> diff --git a/include/libcamera/ipa/ipu3.mojom b/include/libcamera/ipa/ipu3.mojom\n> index 18cdc744..d1b1c6b8 100644\n> --- a/include/libcamera/ipa/ipu3.mojom\n> +++ b/include/libcamera/ipa/ipu3.mojom\n> @@ -8,32 +8,6 @@ module ipa.ipu3;\n> \n> import \"include/libcamera/ipa/core.mojom\";\n> \n> -enum IPU3Operations {\n> - ActionSetSensorControls = 1,\n> - ActionParamFilled = 2,\n> - ActionMetadataReady = 3,\n> - EventProcessControls = 4,\n> - EventStatReady = 5,\n> - EventFillParams = 6,\n> -};\n> -\n> -struct IPU3Event {\n> - IPU3Operations op;\n> - uint32 frame;\n> - int64 frameTimestamp;\n> - uint32 bufferId;\n> - libcamera.ControlList controls;\n> - libcamera.ControlList sensorControls;\n> - libcamera.ControlList lensControls;\n> -};\n> -\n> -struct IPU3Action {\n> - IPU3Operations op;\n> - libcamera.ControlList controls;\n> - libcamera.ControlList sensorControls;\n> - libcamera.ControlList lensControls;\n> -};\n> -\n> struct IPAConfigInfo {\n> libcamera.IPACameraSensorInfo sensorInfo;\n> libcamera.ControlInfoMap sensorControls;\n> @@ -56,9 +30,15 @@ interface IPAIPU3Interface {\n> mapBuffers(array buffers);\n> unmapBuffers(array ids);\n> \n> - [async] processEvent(IPU3Event ev);\n> + [async] queueRequest(uint32 frame, libcamera.ControlList controls);\n> + [async] fillParamsBuffer(uint32 frame, uint32 bufferId);\n> + [async] processStatsBuffer(uint32 frame, int64 frameTimestamp,\n> + uint32 bufferId, libcamera.ControlList sensorControls);\n> };\n> \n> interface IPAIPU3EventInterface {\n> - queueFrameAction(uint32 frame, IPU3Action action);\n> + setSensorControls(uint32 frame, libcamera.ControlList sensorControls,\n> + libcamera.ControlList lensControls);\n> + paramsBufferReady(uint32 frame);\n> + metadataReady(uint32 frame, libcamera.ControlList metadata);\n> };\n> diff --git a/src/ipa/ipu3/ipu3-ipa-design-guide.rst b/src/ipa/ipu3/ipu3-ipa-design-guide.rst\n> index 89c71108..e724fdda 100644\n> --- a/src/ipa/ipu3/ipu3-ipa-design-guide.rst\n> +++ b/src/ipa/ipu3/ipu3-ipa-design-guide.rst\n> @@ -25,7 +25,8 @@ from applications, and managing events from the pipeline handler.\n> └─┬───┬───┬──────┬────┬────┬────┬─┴────▼─┬──┘ 1: init()\n> │ │ │ │ ▲ │ ▲ │ ▲ │ ▲ │ 2: configure()\n> │1 │2 │3 │4│ │4│ │4│ │4│ │5 3: mapBuffers(), start()\n> - ▼ ▼ ▼ ▼ │ ▼ │ ▼ │ ▼ │ ▼ 4: processEvent()\n> + │ │ │ │ │ │ │ │ │ │ │ │ 4: (▼) queueRequest(), fillParamsBuffer(), processStatsBuffer()\n> + ▼ ▼ ▼ ▼ │ ▼ │ ▼ │ ▼ │ ▼ (▲) setSensorControls, paramsBufferReady, metadataReady Signals\n\nI like that this is more descriptive about the direction and operations\nthat occur in 4.\n\n> ┌──────────────────┴────┴────┴────┴─────────┐ 5: stop(), unmapBuffers()\n> │ IPU3 IPA │\n> │ ┌───────────────────────┐ │\n> @@ -100,8 +101,9 @@ There are three main interactions with the algorithms for the IPU3 IPA\n> to operate when running:\n> \n> - configure()\n> -- processEvent(``EventFillParams``)\n> -- processEvent(``EventStatReady``)\n> +- queueRequest()\n> +- fillParamsBuffer()\n> +- processStatsBuffer()\n> \n> The configuration phase allows the pipeline-handler to inform the IPA of\n> the current stream configurations, which is then passed into each\n> @@ -114,8 +116,8 @@ Pre-frame preparation\n> When configured, the IPA is notified by the pipeline handler of the\n> Camera ``start()`` event, after which incoming requests will be queued\n> for processing, requiring a parameter buffer (``ipu3_uapi_params``) to\n> -be populated for the ImgU. This is given to the IPA through the\n> -``EventFillParams`` event, and then passed directly to each algorithm\n> +be populated for the ImgU. This is given to the IPA through\n> +``fillParamsBuffer()``, and then passed directly to each algorithm\n> through the ``prepare()`` call allowing the ISP configuration to be\n> updated for the needs of each component that the algorithm is\n> responsible for.\n> @@ -125,7 +127,7 @@ structure that it modifies, and it should take care to ensure that any\n> structure set by a use flag is fully initialised to suitable values.\n> \n> The parameter buffer is returned to the pipeline handler through the\n> -``ActionParamFilled`` event, and from there queued to the ImgU along\n> +``paramsBufferReady`` signal, and from there queued to the ImgU along\n> with a raw frame captured with the CIO2.\n> \n> Post-frame completion\n> @@ -133,8 +135,8 @@ Post-frame completion\n> \n> When the capture of an image is completed, and successfully processed\n> through the ImgU, the generated statistics buffer\n> -(``ipu3_uapi_stats_3a``) is given to the IPA through the\n> -``EventStatReady`` event. This provides the IPA with an opportunity to\n> +(``ipu3_uapi_stats_3a``) is given to the IPA through\n> +``processStatsBuffer()``. This provides the IPA with an opportunity to\n> examine the results of the ISP and run the calculations required by each\n> algorithm on the new data. The algorithms may require context from the\n> operations of other algorithms, for example, the AWB might choose to use\n> @@ -145,11 +147,14 @@ before they are needed.\n> The ordering of the algorithm processing is determined by their\n> placement in the ``IPU3::algorithms_`` ordered list.\n> \n> +Finally, the IPA metadata for the completed frame is returned back via\n> +the ``metadataReady`` signal.\n> +\n> Sensor Controls\n> ~~~~~~~~~~~~~~~\n> \n> The AutoExposure and AutoGain (AGC) algorithm differs slightly from the\n> others as it requires operating directly on the sensor, as opposed to\n> -through the ImgU ISP. To support this, there is a dedicated action\n> -`ActionSetSensorControls` to allow the IPA to request controls to be set\n> -on the camera sensor through the pipeline handler.\n> +through the ImgU ISP. To support this, there is a ``setSensorControls``\n> +signal to allow the IPA to request controls to be set on the camera\n> +sensor through the pipeline handler.\n> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> index 50b52d8b..9a59f80f 100644\n> --- a/src/ipa/ipu3/ipu3.cpp\n> +++ b/src/ipa/ipu3/ipu3.cpp\n> @@ -65,8 +65,9 @@ namespace ipa::ipu3 {\n> * The IPU3 Pipeline defines an IPU3-specific interface for communication\n> * between the PipelineHandler and the IPA module.\n> *\n> - * We extend the IPAIPU3Interface to implement our algorithms and handle events\n> - * from the IPU3 PipelineHandler to satisfy requests from the application.\n> + * We extend the IPAIPU3Interface to implement our algorithms and handle\n> + * calls from the IPU3 PipelineHandler to satisfy requests from the\n> + * application.\n> *\n> * At initialisation time, a CameraSensorHelper is instantiated to support\n> * camera-specific calculations, while the default controls are computed, and\n> @@ -81,14 +82,14 @@ namespace ipa::ipu3 {\n> * parameter buffer, and adapting the settings of the sensor attached to the\n> * IPU3 CIO2 through sensor-specific V4L2 controls.\n> *\n> - * When the event \\a EventFillParams occurs we populate the ImgU parameter\n> - * buffer with settings to configure the device in preparation for handling the\n> - * frame queued in the Request.\n> + * In fillParamsBuffer(), we populate the ImgU parameter buffer with\n> + * settings to configure the device in preparation for handling the frame\n> + * queued in the Request.\n> *\n> * When the frame has completed processing, the ImgU will generate a statistics\n> - * buffer which is given to the IPA as part of the \\a EventStatReady event. At\n> - * this event we run the algorithms to parse the statistics and cache any\n> - * results for the next \\a EventFillParams event.\n> + * buffer which is given to the IPA with processStatsBuffer(). In this we run the\n> + * algorithms to parse the statistics and cache any results for the next\n> + * fillParamsBuffer() call.\n> *\n> * The individual algorithms are split into modular components that are called\n> * iteratively to allow them to process statistics from the ImgU in a defined\n> @@ -143,14 +144,18 @@ public:\n> \n> void mapBuffers(const std::vector &buffers) override;\n> void unmapBuffers(const std::vector &ids) override;\n> - void processEvent(const IPU3Event &event) override;\n> \n> + void queueRequest(const uint32_t frame, const ControlList &controls) override;\n> + void fillParamsBuffer(const uint32_t frame, const uint32_t bufferId) override;\n> + void processStatsBuffer(const uint32_t frame, const int64_t frameTimestamp,\n> + const uint32_t bufferId,\n> + const ControlList &sensorControls) override;\n> private:\n> void updateControls(const IPACameraSensorInfo &sensorInfo,\n> const ControlInfoMap &sensorControls,\n> ControlInfoMap *ipaControls);\n> void updateSessionConfiguration(const ControlInfoMap &sensorControls);\n> - void processControls(unsigned int frame, const ControlList &controls);\n> +\n> void fillParams(unsigned int frame, ipu3_uapi_params *params);\n> void parseStatistics(unsigned int frame,\n> int64_t frameTimestamp,\n> @@ -505,73 +510,61 @@ void IPAIPU3::unmapBuffers(const std::vector &ids)\n> }\n> \n> /**\n> - * \\brief Process an event generated by the pipeline handler\n> - * \\param[in] event The event sent from pipeline handler\n> - *\n> - * The expected event handling over the lifetime of a Request has\n> - * the following sequence:\n> - *\n> - * - EventProcessControls : Handle controls from a new Request\n> - * - EventFillParams : Prepare the ISP to process the Request\n> - * - EventStatReady : Process statistics after ISP completion\n> + * \\brief Fill and return a buffer with ISP processing parameters for a frame\n> + * \\param[in] frame The frame number\n> + * \\param[in] bufferId ID of the parameter buffer to fill\n> */\n> -void IPAIPU3::processEvent(const IPU3Event &event)\n> +void IPAIPU3::fillParamsBuffer(const uint32_t frame, const uint32_t bufferId)\n> {\n> - switch (event.op) {\n> - case EventProcessControls: {\n> - processControls(event.frame, event.controls);\n> - break;\n> + auto it = buffers_.find(bufferId);\n> + if (it == buffers_.end()) {\n> + LOG(IPAIPU3, Error) << \"Could not find param buffer!\";\n> + return;\n> }\n> - case EventFillParams: {\n> - auto it = buffers_.find(event.bufferId);\n> - if (it == buffers_.end()) {\n> - LOG(IPAIPU3, Error) << \"Could not find param buffer!\";\n> - return;\n> - }\n> \n> - Span mem = it->second.planes()[0];\n> - ipu3_uapi_params *params =\n> - reinterpret_cast(mem.data());\n> + Span mem = it->second.planes()[0];\n> + ipu3_uapi_params *params =\n> + reinterpret_cast(mem.data());\n> \n> - fillParams(event.frame, params);\n> - break;\n> - }\n> - case EventStatReady: {\n> - auto it = buffers_.find(event.bufferId);\n> - if (it == buffers_.end()) {\n> - LOG(IPAIPU3, Error) << \"Could not find stats buffer!\";\n> - return;\n> - }\n> + fillParams(frame, params);\n> +}\n> \n> - Span mem = it->second.planes()[0];\n> - const ipu3_uapi_stats_3a *stats =\n> - reinterpret_cast(mem.data());\n> +/**\n> + * \\brief Process statistics after ISP completion\n> + * \\param[in] frame The frame number\n> + * \\param[in] frameTimestamp Timestamp of the frame\n> + * \\param[in] bufferId ID of the statistics buffer\n> + * \\param[in] sensorControls Sensor controls\n> + */\n> +void IPAIPU3::processStatsBuffer(const uint32_t frame, const int64_t frameTimestamp,\n> + const uint32_t bufferId, const ControlList &sensorControls)\n> +{\n> + auto it = buffers_.find(bufferId);\n> + if (it == buffers_.end()) {\n> + LOG(IPAIPU3, Error) << \"Could not find stats buffer!\";\n> + return;\n> + }\n> \n> - int32_t exposure = event.sensorControls.get(V4L2_CID_EXPOSURE).get();\n> - int32_t gain = event.sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get();\n> + Span mem = it->second.planes()[0];\n> + const ipu3_uapi_stats_3a *stats =\n> + reinterpret_cast(mem.data());\n> \n> - context_.frameContext.sensor.exposure = exposure;\n> - context_.frameContext.sensor.gain = camHelper_->gain(gain);\n> + context_.frameContext.sensor.exposure = sensorControls.get(V4L2_CID_EXPOSURE).get();\n> + context_.frameContext.sensor.gain = camHelper_->gain(sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get());\n> \n> - parseStatistics(event.frame, event.frameTimestamp, stats);\n> - break;\n> - }\n> - default:\n> - LOG(IPAIPU3, Error) << \"Unknown event \" << event.op;\n> - break;\n> - }\n> + parseStatistics(frame, frameTimestamp, stats);\n> }\n> \n> /**\n> - * \\brief Process a control list for a request from the application\n> + * \\brief Queue a request and process the control list from the application\n> * \\param[in] frame The number of the frame which will be processed next\n> * \\param[in] controls The controls for the \\a frame\n> *\n> * Parse the request to handle any IPA-managed controls that were set from the\n> * application such as manual sensor settings.\n> */\n> -void IPAIPU3::processControls([[maybe_unused]] unsigned int frame,\n> - [[maybe_unused]] const ControlList &controls)\n> +void IPAIPU3::queueRequest(const uint32_t frame,\n> + [[maybe_unused]] const ControlList &controls)\n> {\n> /* \\todo Start processing for 'frame' based on 'controls'. */\n> }\n> @@ -600,10 +593,7 @@ void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params)\n> for (auto const &algo : algorithms_)\n> algo->prepare(context_, params);\n> \n> - IPU3Action op;\n> - op.op = ActionParamFilled;\n> -\n> - queueFrameAction.emit(frame, op);\n> + paramsBufferReady.emit(frame);\n> }\n> \n> /**\n> @@ -647,11 +637,7 @@ void IPAIPU3::parseStatistics(unsigned int frame,\n> * likely want to avoid putting platform specific metadata in.\n> */\n> \n> - IPU3Action op;\n> - op.op = ActionMetadataReady;\n> - op.controls = ctrls;\n> -\n> - queueFrameAction.emit(frame, op);\n> + metadataReady.emit(frame, ctrls);\n> }\n> \n> /**\n> @@ -663,23 +649,18 @@ void IPAIPU3::parseStatistics(unsigned int frame,\n> */\n> void IPAIPU3::setControls(unsigned int frame)\n> {\n> - IPU3Action op;\n> - op.op = ActionSetSensorControls;\n> -\n> int32_t exposure = context_.frameContext.agc.exposure;\n> int32_t gain = camHelper_->gainCode(context_.frameContext.agc.gain);\n> \n> ControlList ctrls(sensorCtrls_);\n> ctrls.set(V4L2_CID_EXPOSURE, exposure);\n> ctrls.set(V4L2_CID_ANALOGUE_GAIN, gain);\n> - op.sensorControls = ctrls;\n> \n> ControlList lensCtrls(lensCtrls_);\n> lensCtrls.set(V4L2_CID_FOCUS_ABSOLUTE,\n> static_cast(context_.frameContext.af.focus));\n> - op.lensControls = lensCtrls;\n> \n> - queueFrameAction.emit(frame, op);\n> + setSensorControls.emit(frame, ctrls, lensCtrls);\n> }\n> \n> } /* namespace ipa::ipu3 */\n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index 60e01917..59d7e9c0 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -86,8 +86,10 @@ public:\n> ControlInfoMap ipaControls_;\n> \n> private:\n> - void queueFrameAction(unsigned int id,\n> - const ipa::ipu3::IPU3Action &action);\n> + void metadataReady(unsigned int id, const ControlList &metadata);\n> + void paramsFilled(unsigned int id);\n\nThis is confusing. ActionParamFilled becomes paramsBufferReady in the\ncommit message. Is this function used?\n\n> + void setSensorControls(unsigned int id, const ControlList &sensorControls,\n> + const ControlList &lensControls);\n> };\n> \n> class IPU3CameraConfiguration : public CameraConfiguration\n> @@ -871,11 +873,7 @@ void IPU3CameraData::queuePendingRequests()\n> \n> info->rawBuffer = rawBuffer;\n> \n> - ipa::ipu3::IPU3Event ev;\n> - ev.op = ipa::ipu3::EventProcessControls;\n> - ev.frame = info->id;\n> - ev.controls = request->controls();\n> - ipa_->processEvent(ev);\n> + ipa_->queueRequest(info->id, request->controls());\n> \n> pendingRequests_.pop();\n> processingRequests_.push(request);\n> @@ -1218,7 +1216,9 @@ int IPU3CameraData::loadIPA()\n> if (!ipa_)\n> return -ENOENT;\n> \n> - ipa_->queueFrameAction.connect(this, &IPU3CameraData::queueFrameAction);\n> + ipa_->setSensorControls.connect(this, &IPU3CameraData::setSensorControls);\n> + ipa_->paramsBufferReady.connect(this, &IPU3CameraData::paramsFilled);\n\nOh - I see - the signals are connected to functions of the same name\nwithin the IPU3CameraData.\n\nIs there a reason why \n queueFrameAction => queueFrameAction,\n setSensorControls => setSensorControls,\n metadataReady => metadataReady,\nbut\n paramsBufferReady => paramsFilled?\n\n\n\n> + ipa_->metadataReady.connect(this, &IPU3CameraData::metadataReady);\n> \n> /*\n> * Pass the sensor info to the IPA to initialize controls.\n> @@ -1253,69 +1253,58 @@ int IPU3CameraData::loadIPA()\n> return 0;\n> }\n> \n> -void IPU3CameraData::queueFrameAction(unsigned int id,\n> - const ipa::ipu3::IPU3Action &action)\n> +void IPU3CameraData::setSensorControls([[maybe_unused]] unsigned int id,\n> + const ControlList &sensorControls,\n> + const ControlList &lensControls)\n> {\n> - switch (action.op) {\n> - case ipa::ipu3::ActionSetSensorControls: {\n> - const ControlList &sensorControls = action.sensorControls;\n> - delayedCtrls_->push(sensorControls);\n> + delayedCtrls_->push(sensorControls);\n> \n> - CameraLens *focusLens = cio2_.sensor()->focusLens();\n> - if (!focusLens)\n> - break;\n> -\n> - const ControlList lensControls = action.lensControls;\n> - if (!lensControls.contains(V4L2_CID_FOCUS_ABSOLUTE))\n> - break;\n> + CameraLens *focusLens = cio2_.sensor()->focusLens();\n> + if (!focusLens)\n> + return;\n> \n> - const ControlValue &focusValue =\n> - lensControls.get(V4L2_CID_FOCUS_ABSOLUTE);\n> + if (!lensControls.contains(V4L2_CID_FOCUS_ABSOLUTE))\n> + return;\n> \n> - focusLens->setFocusPosition(focusValue.get());\n> + const ControlValue &focusValue = lensControls.get(V4L2_CID_FOCUS_ABSOLUTE);\n> \n> - break;\n> - }\n> - case ipa::ipu3::ActionParamFilled: {\n> - IPU3Frames::Info *info = frameInfos_.find(id);\n> - if (!info)\n> - break;\n> -\n> - /* Queue all buffers from the request aimed for the ImgU. */\n> - for (auto it : info->request->buffers()) {\n> - const Stream *stream = it.first;\n> - FrameBuffer *outbuffer = it.second;\n> + focusLens->setFocusPosition(focusValue.get());\n> +}\n> \n> - if (stream == &outStream_)\n> - imgu_->output_->queueBuffer(outbuffer);\n> - else if (stream == &vfStream_)\n> - imgu_->viewfinder_->queueBuffer(outbuffer);\n> - }\n> +void IPU3CameraData::paramsFilled(unsigned int id)\n> +{\n> + IPU3Frames::Info *info = frameInfos_.find(id);\n> + if (!info)\n> + return;\n> \n> - imgu_->param_->queueBuffer(info->paramBuffer);\n> - imgu_->stat_->queueBuffer(info->statBuffer);\n> - imgu_->input_->queueBuffer(info->rawBuffer);\n> + /* Queue all buffers from the request aimed for the ImgU. */\n> + for (auto it : info->request->buffers()) {\n> + const Stream *stream = it.first;\n> + FrameBuffer *outbuffer = it.second;\n> \n> - break;\n> + if (stream == &outStream_)\n> + imgu_->output_->queueBuffer(outbuffer);\n> + else if (stream == &vfStream_)\n> + imgu_->viewfinder_->queueBuffer(outbuffer);\n> }\n> - case ipa::ipu3::ActionMetadataReady: {\n> - IPU3Frames::Info *info = frameInfos_.find(id);\n> - if (!info)\n> - break;\n> \n> - Request *request = info->request;\n> - request->metadata().merge(action.controls);\n> + imgu_->param_->queueBuffer(info->paramBuffer);\n> + imgu_->stat_->queueBuffer(info->statBuffer);\n> + imgu_->input_->queueBuffer(info->rawBuffer);\n> +}\n> \n> - info->metadataProcessed = true;\n> - if (frameInfos_.tryComplete(info))\n> - pipe()->completeRequest(request);\n> +void IPU3CameraData::metadataReady(unsigned int id, const ControlList &metadata)\n> +{\n> + IPU3Frames::Info *info = frameInfos_.find(id);\n> + if (!info)\n> + return;\n> \n> - break;\n> - }\n> - default:\n> - LOG(IPU3, Error) << \"Unknown action \" << action.op;\n> - break;\n> - }\n> + Request *request = info->request;\n> + request->metadata().merge(metadata);\n> +\n> + info->metadataProcessed = true;\n> + if (frameInfos_.tryComplete(info))\n> + pipe()->completeRequest(request);\n> }\n> \n> /* -----------------------------------------------------------------------------\n> @@ -1390,11 +1379,7 @@ void IPU3CameraData::cio2BufferReady(FrameBuffer *buffer)\n> if (request->findBuffer(&rawStream_))\n> pipe()->completeBuffer(request, buffer);\n> \n> - ipa::ipu3::IPU3Event ev;\n> - ev.op = ipa::ipu3::EventFillParams;\n> - ev.frame = info->id;\n> - ev.bufferId = info->paramBuffer->cookie();\n> - ipa_->processEvent(ev);\n> + ipa_->fillParamsBuffer(info->id, info->paramBuffer->cookie());\n> }\n> \n> void IPU3CameraData::paramBufferReady(FrameBuffer *buffer)\n> @@ -1438,13 +1423,8 @@ void IPU3CameraData::statBufferReady(FrameBuffer *buffer)\n> return;\n> }\n> \n> - ipa::ipu3::IPU3Event ev;\n> - ev.op = ipa::ipu3::EventStatReady;\n> - ev.frame = info->id;\n> - ev.bufferId = info->statBuffer->cookie();\n> - ev.frameTimestamp = request->metadata().get(controls::SensorTimestamp);\n> - ev.sensorControls = info->effectiveSensorControls;\n> - ipa_->processEvent(ev);\n> + ipa_->processStatsBuffer(info->id, request->metadata().get(controls::SensorTimestamp),\n> + info->statBuffer->cookie(), info->effectiveSensorControls);\n> }\n> \n> /*\n> -- \n> 2.31.0\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 079ACC3256\n\tfor ;\n\tThu, 7 Apr 2022 22:11:40 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5186065644;\n\tFri, 8 Apr 2022 00:11:39 +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 370B4604B8\n\tfor ;\n\tFri, 8 Apr 2022 00:11:37 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id A51CF499;\n\tFri, 8 Apr 2022 00:11:36 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1649369499;\n\tbh=vgmqzOjFTg4F5CdvkYD7ntChNwkB4CpqfiJXovojHs4=;\n\th=In-Reply-To:References:To:Date:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:\n\tFrom;\n\tb=qmMkCfkL5eUWzoqwlzEfroVvOvsyIhVsux0EpUbB1BDTlebKe9DU4Hk7xID5LX3Bg\n\t/U1a9avQRtgvCVPCEgFRQcuhvXKJubl81Ek9iWpdeXPtQMdkEnP3KFdJj9x+u5Exe6\n\tyf9EETuKceV52+liKHQiV49s7cU8QvRSaG4PyRADkDlOLjDFdqTobVm9Ti0Zexr5UW\n\tVVzvxa1TcX3HGPJZ6q4YgztCe4FKfcMiiYJGgfyIO99h2eL7qWJQBPo849sr7Z15kM\n\th16uJAFHpRyx14iqXAoW7bXd5dh9TLUZr/U0g55XHgiZ6jjZoivUkcjxs1ytPXTZnb\n\t+zN8LwKRD4EGg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1649369496;\n\tbh=vgmqzOjFTg4F5CdvkYD7ntChNwkB4CpqfiJXovojHs4=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=oO2WrLMkr6MeRqlWYeBhaVIiDai5F0FmsjmI2w2da9fuZfuFckkxLbFhjatiL5LA5\n\tzQIP8iNIDCBMABq/NxteCEel/MzI1YlXcgdpiVNWznzN1wXNOS0gwXoSaUlFaM6Hs0\n\tK3cwSbjd4uHzGvGhpTgRDcNriwjhw0fMV/MGGako="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"oO2WrLMk\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20220406141709.164794-2-umang.jain@ideasonboard.com>","References":"<20220406141709.164794-1-umang.jain@ideasonboard.com>\n\t<20220406141709.164794-2-umang.jain@ideasonboard.com>","To":"Umang Jain ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Thu, 07 Apr 2022 23:11:34 +0100","Message-ID":"<164936949446.22830.7307396101214349800@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v5 1/6] ipa: ipu3: Replace event-based\n\tops with dedicated functions","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":"Kieran Bingham via libcamera-devel\n\t","Reply-To":"Kieran Bingham ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":22658,"web_url":"https://patchwork.libcamera.org/comment/22658/","msgid":"<20220408065529.GL3237525@pyrite.rasen.tech>","date":"2022-04-08T06:55:29","subject":"Re: [libcamera-devel] [PATCH v5 1/6] ipa: ipu3: Replace event-based\n\tops with dedicated functions","submitter":{"id":97,"url":"https://patchwork.libcamera.org/api/people/97/","name":"Nicolas Dufresne via libcamera-devel","email":"libcamera-devel@lists.libcamera.org"},"content":"Hi Umang,\n\nOn Wed, Apr 06, 2022 at 07:47:04PM +0530, Umang Jain via libcamera-devel wrote:\n> The IPAIPU3 interface currently uses event-type based structures in\n> order to communicate with the pipeline-handler (and vice-versa).\n> Replace the event based structures with dedicated functions associated\n> to each operation.\n> \n> The translated naming scheme of operations to dedicated functions:\n> ActionSetSensorControls => setSensorControls\n> ActionParamFilled => paramsBufferReady\n> ActionMetadataReady => metadataReady\n> EventProcessControls => queueRequest()\n\nDo you need to mention a bit on why just this one seems symmetrical? I\nknow what's going on since you recently had a similar change, but for\nothers perhaps not?\n\n> EventStatReady => processStatsBuffer()\n> EventFillParams => fillParamsBuffer()\n> \n> Signed-off-by: Umang Jain \n> Reviewed-by: Laurent Pinchart \n\nReviewed-by: Paul Elder \n\n> ---\n> include/libcamera/ipa/ipu3.mojom | 36 ++-----\n> src/ipa/ipu3/ipu3-ipa-design-guide.rst | 27 +++---\n> src/ipa/ipu3/ipu3.cpp | 129 +++++++++++--------------\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 122 ++++++++++-------------\n> 4 files changed, 130 insertions(+), 184 deletions(-)\n> \n> diff --git a/include/libcamera/ipa/ipu3.mojom b/include/libcamera/ipa/ipu3.mojom\n> index 18cdc744..d1b1c6b8 100644\n> --- a/include/libcamera/ipa/ipu3.mojom\n> +++ b/include/libcamera/ipa/ipu3.mojom\n> @@ -8,32 +8,6 @@ module ipa.ipu3;\n> \n> import \"include/libcamera/ipa/core.mojom\";\n> \n> -enum IPU3Operations {\n> -\tActionSetSensorControls = 1,\n> -\tActionParamFilled = 2,\n> -\tActionMetadataReady = 3,\n> -\tEventProcessControls = 4,\n> -\tEventStatReady = 5,\n> -\tEventFillParams = 6,\n> -};\n> -\n> -struct IPU3Event {\n> -\tIPU3Operations op;\n> -\tuint32 frame;\n> -\tint64 frameTimestamp;\n> -\tuint32 bufferId;\n> -\tlibcamera.ControlList controls;\n> -\tlibcamera.ControlList sensorControls;\n> -\tlibcamera.ControlList lensControls;\n> -};\n> -\n> -struct IPU3Action {\n> -\tIPU3Operations op;\n> -\tlibcamera.ControlList controls;\n> -\tlibcamera.ControlList sensorControls;\n> -\tlibcamera.ControlList lensControls;\n> -};\n> -\n> struct IPAConfigInfo {\n> \tlibcamera.IPACameraSensorInfo sensorInfo;\n> \tlibcamera.ControlInfoMap sensorControls;\n> @@ -56,9 +30,15 @@ interface IPAIPU3Interface {\n> \tmapBuffers(array buffers);\n> \tunmapBuffers(array ids);\n> \n> -\t[async] processEvent(IPU3Event ev);\n> +\t[async] queueRequest(uint32 frame, libcamera.ControlList controls);\n> +\t[async] fillParamsBuffer(uint32 frame, uint32 bufferId);\n> +\t[async] processStatsBuffer(uint32 frame, int64 frameTimestamp,\n> +\t\t\t\t uint32 bufferId, libcamera.ControlList sensorControls);\n> };\n> \n> interface IPAIPU3EventInterface {\n> -\tqueueFrameAction(uint32 frame, IPU3Action action);\n> +\tsetSensorControls(uint32 frame, libcamera.ControlList sensorControls,\n> +\t\t\t libcamera.ControlList lensControls);\n> +\tparamsBufferReady(uint32 frame);\n> +\tmetadataReady(uint32 frame, libcamera.ControlList metadata);\n> };\n> diff --git a/src/ipa/ipu3/ipu3-ipa-design-guide.rst b/src/ipa/ipu3/ipu3-ipa-design-guide.rst\n> index 89c71108..e724fdda 100644\n> --- a/src/ipa/ipu3/ipu3-ipa-design-guide.rst\n> +++ b/src/ipa/ipu3/ipu3-ipa-design-guide.rst\n> @@ -25,7 +25,8 @@ from applications, and managing events from the pipeline handler.\n> └─┬───┬───┬──────┬────┬────┬────┬─┴────▼─┬──┘ 1: init()\n> │ │ │ │ ▲ │ ▲ │ ▲ │ ▲ │ 2: configure()\n> │1 │2 │3 │4│ │4│ │4│ │4│ │5 3: mapBuffers(), start()\n> - ▼ ▼ ▼ ▼ │ ▼ │ ▼ │ ▼ │ ▼ 4: processEvent()\n> + │ │ │ │ │ │ │ │ │ │ │ │ 4: (▼) queueRequest(), fillParamsBuffer(), processStatsBuffer()\n> + ▼ ▼ ▼ ▼ │ ▼ │ ▼ │ ▼ │ ▼ (▲) setSensorControls, paramsBufferReady, metadataReady Signals\n> ┌──────────────────┴────┴────┴────┴─────────┐ 5: stop(), unmapBuffers()\n> │ IPU3 IPA │\n> │ ┌───────────────────────┐ │\n> @@ -100,8 +101,9 @@ There are three main interactions with the algorithms for the IPU3 IPA\n> to operate when running:\n> \n> - configure()\n> -- processEvent(``EventFillParams``)\n> -- processEvent(``EventStatReady``)\n> +- queueRequest()\n> +- fillParamsBuffer()\n> +- processStatsBuffer()\n> \n> The configuration phase allows the pipeline-handler to inform the IPA of\n> the current stream configurations, which is then passed into each\n> @@ -114,8 +116,8 @@ Pre-frame preparation\n> When configured, the IPA is notified by the pipeline handler of the\n> Camera ``start()`` event, after which incoming requests will be queued\n> for processing, requiring a parameter buffer (``ipu3_uapi_params``) to\n> -be populated for the ImgU. This is given to the IPA through the\n> -``EventFillParams`` event, and then passed directly to each algorithm\n> +be populated for the ImgU. This is given to the IPA through\n> +``fillParamsBuffer()``, and then passed directly to each algorithm\n> through the ``prepare()`` call allowing the ISP configuration to be\n> updated for the needs of each component that the algorithm is\n> responsible for.\n> @@ -125,7 +127,7 @@ structure that it modifies, and it should take care to ensure that any\n> structure set by a use flag is fully initialised to suitable values.\n> \n> The parameter buffer is returned to the pipeline handler through the\n> -``ActionParamFilled`` event, and from there queued to the ImgU along\n> +``paramsBufferReady`` signal, and from there queued to the ImgU along\n> with a raw frame captured with the CIO2.\n> \n> Post-frame completion\n> @@ -133,8 +135,8 @@ Post-frame completion\n> \n> When the capture of an image is completed, and successfully processed\n> through the ImgU, the generated statistics buffer\n> -(``ipu3_uapi_stats_3a``) is given to the IPA through the\n> -``EventStatReady`` event. This provides the IPA with an opportunity to\n> +(``ipu3_uapi_stats_3a``) is given to the IPA through\n> +``processStatsBuffer()``. This provides the IPA with an opportunity to\n> examine the results of the ISP and run the calculations required by each\n> algorithm on the new data. The algorithms may require context from the\n> operations of other algorithms, for example, the AWB might choose to use\n> @@ -145,11 +147,14 @@ before they are needed.\n> The ordering of the algorithm processing is determined by their\n> placement in the ``IPU3::algorithms_`` ordered list.\n> \n> +Finally, the IPA metadata for the completed frame is returned back via\n> +the ``metadataReady`` signal.\n> +\n> Sensor Controls\n> ~~~~~~~~~~~~~~~\n> \n> The AutoExposure and AutoGain (AGC) algorithm differs slightly from the\n> others as it requires operating directly on the sensor, as opposed to\n> -through the ImgU ISP. To support this, there is a dedicated action\n> -`ActionSetSensorControls` to allow the IPA to request controls to be set\n> -on the camera sensor through the pipeline handler.\n> +through the ImgU ISP. To support this, there is a ``setSensorControls``\n> +signal to allow the IPA to request controls to be set on the camera\n> +sensor through the pipeline handler.\n> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> index 50b52d8b..9a59f80f 100644\n> --- a/src/ipa/ipu3/ipu3.cpp\n> +++ b/src/ipa/ipu3/ipu3.cpp\n> @@ -65,8 +65,9 @@ namespace ipa::ipu3 {\n> * The IPU3 Pipeline defines an IPU3-specific interface for communication\n> * between the PipelineHandler and the IPA module.\n> *\n> - * We extend the IPAIPU3Interface to implement our algorithms and handle events\n> - * from the IPU3 PipelineHandler to satisfy requests from the application.\n> + * We extend the IPAIPU3Interface to implement our algorithms and handle\n> + * calls from the IPU3 PipelineHandler to satisfy requests from the\n> + * application.\n> *\n> * At initialisation time, a CameraSensorHelper is instantiated to support\n> * camera-specific calculations, while the default controls are computed, and\n> @@ -81,14 +82,14 @@ namespace ipa::ipu3 {\n> * parameter buffer, and adapting the settings of the sensor attached to the\n> * IPU3 CIO2 through sensor-specific V4L2 controls.\n> *\n> - * When the event \\a EventFillParams occurs we populate the ImgU parameter\n> - * buffer with settings to configure the device in preparation for handling the\n> - * frame queued in the Request.\n> + * In fillParamsBuffer(), we populate the ImgU parameter buffer with\n> + * settings to configure the device in preparation for handling the frame\n> + * queued in the Request.\n> *\n> * When the frame has completed processing, the ImgU will generate a statistics\n> - * buffer which is given to the IPA as part of the \\a EventStatReady event. At\n> - * this event we run the algorithms to parse the statistics and cache any\n> - * results for the next \\a EventFillParams event.\n> + * buffer which is given to the IPA with processStatsBuffer(). In this we run the\n> + * algorithms to parse the statistics and cache any results for the next\n> + * fillParamsBuffer() call.\n> *\n> * The individual algorithms are split into modular components that are called\n> * iteratively to allow them to process statistics from the ImgU in a defined\n> @@ -143,14 +144,18 @@ public:\n> \n> \tvoid mapBuffers(const std::vector &buffers) override;\n> \tvoid unmapBuffers(const std::vector &ids) override;\n> -\tvoid processEvent(const IPU3Event &event) override;\n> \n> +\tvoid queueRequest(const uint32_t frame, const ControlList &controls) override;\n> +\tvoid fillParamsBuffer(const uint32_t frame, const uint32_t bufferId) override;\n> +\tvoid processStatsBuffer(const uint32_t frame, const int64_t frameTimestamp,\n> +\t\t\t\tconst uint32_t bufferId,\n> +\t\t\t\tconst ControlList &sensorControls) override;\n> private:\n> \tvoid updateControls(const IPACameraSensorInfo &sensorInfo,\n> \t\t\t const ControlInfoMap &sensorControls,\n> \t\t\t ControlInfoMap *ipaControls);\n> \tvoid updateSessionConfiguration(const ControlInfoMap &sensorControls);\n> -\tvoid processControls(unsigned int frame, const ControlList &controls);\n> +\n> \tvoid fillParams(unsigned int frame, ipu3_uapi_params *params);\n> \tvoid parseStatistics(unsigned int frame,\n> \t\t\t int64_t frameTimestamp,\n> @@ -505,73 +510,61 @@ void IPAIPU3::unmapBuffers(const std::vector &ids)\n> }\n> \n> /**\n> - * \\brief Process an event generated by the pipeline handler\n> - * \\param[in] event The event sent from pipeline handler\n> - *\n> - * The expected event handling over the lifetime of a Request has\n> - * the following sequence:\n> - *\n> - * - EventProcessControls : Handle controls from a new Request\n> - * - EventFillParams : Prepare the ISP to process the Request\n> - * - EventStatReady : Process statistics after ISP completion\n> + * \\brief Fill and return a buffer with ISP processing parameters for a frame\n> + * \\param[in] frame The frame number\n> + * \\param[in] bufferId ID of the parameter buffer to fill\n> */\n> -void IPAIPU3::processEvent(const IPU3Event &event)\n> +void IPAIPU3::fillParamsBuffer(const uint32_t frame, const uint32_t bufferId)\n> {\n> -\tswitch (event.op) {\n> -\tcase EventProcessControls: {\n> -\t\tprocessControls(event.frame, event.controls);\n> -\t\tbreak;\n> +\tauto it = buffers_.find(bufferId);\n> +\tif (it == buffers_.end()) {\n> +\t\tLOG(IPAIPU3, Error) << \"Could not find param buffer!\";\n> +\t\treturn;\n> \t}\n> -\tcase EventFillParams: {\n> -\t\tauto it = buffers_.find(event.bufferId);\n> -\t\tif (it == buffers_.end()) {\n> -\t\t\tLOG(IPAIPU3, Error) << \"Could not find param buffer!\";\n> -\t\t\treturn;\n> -\t\t}\n> \n> -\t\tSpan mem = it->second.planes()[0];\n> -\t\tipu3_uapi_params *params =\n> -\t\t\treinterpret_cast(mem.data());\n> +\tSpan mem = it->second.planes()[0];\n> +\tipu3_uapi_params *params =\n> +\t\treinterpret_cast(mem.data());\n> \n> -\t\tfillParams(event.frame, params);\n> -\t\tbreak;\n> -\t}\n> -\tcase EventStatReady: {\n> -\t\tauto it = buffers_.find(event.bufferId);\n> -\t\tif (it == buffers_.end()) {\n> -\t\t\tLOG(IPAIPU3, Error) << \"Could not find stats buffer!\";\n> -\t\t\treturn;\n> -\t\t}\n> +\tfillParams(frame, params);\n> +}\n> \n> -\t\tSpan mem = it->second.planes()[0];\n> -\t\tconst ipu3_uapi_stats_3a *stats =\n> -\t\t\treinterpret_cast(mem.data());\n> +/**\n> + * \\brief Process statistics after ISP completion\n> + * \\param[in] frame The frame number\n> + * \\param[in] frameTimestamp Timestamp of the frame\n> + * \\param[in] bufferId ID of the statistics buffer\n> + * \\param[in] sensorControls Sensor controls\n> + */\n> +void IPAIPU3::processStatsBuffer(const uint32_t frame, const int64_t frameTimestamp,\n> +\t\t\t\t const uint32_t bufferId, const ControlList &sensorControls)\n> +{\n> +\tauto it = buffers_.find(bufferId);\n> +\tif (it == buffers_.end()) {\n> +\t\tLOG(IPAIPU3, Error) << \"Could not find stats buffer!\";\n> +\t\treturn;\n> +\t}\n> \n> -\t\tint32_t exposure = event.sensorControls.get(V4L2_CID_EXPOSURE).get();\n> -\t\tint32_t gain = event.sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get();\n> +\tSpan mem = it->second.planes()[0];\n> +\tconst ipu3_uapi_stats_3a *stats =\n> +\t\treinterpret_cast(mem.data());\n> \n> -\t\tcontext_.frameContext.sensor.exposure = exposure;\n> -\t\tcontext_.frameContext.sensor.gain = camHelper_->gain(gain);\n> +\tcontext_.frameContext.sensor.exposure = sensorControls.get(V4L2_CID_EXPOSURE).get();\n> +\tcontext_.frameContext.sensor.gain = camHelper_->gain(sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get());\n> \n> -\t\tparseStatistics(event.frame, event.frameTimestamp, stats);\n> -\t\tbreak;\n> -\t}\n> -\tdefault:\n> -\t\tLOG(IPAIPU3, Error) << \"Unknown event \" << event.op;\n> -\t\tbreak;\n> -\t}\n> +\tparseStatistics(frame, frameTimestamp, stats);\n> }\n> \n> /**\n> - * \\brief Process a control list for a request from the application\n> + * \\brief Queue a request and process the control list from the application\n> * \\param[in] frame The number of the frame which will be processed next\n> * \\param[in] controls The controls for the \\a frame\n> *\n> * Parse the request to handle any IPA-managed controls that were set from the\n> * application such as manual sensor settings.\n> */\n> -void IPAIPU3::processControls([[maybe_unused]] unsigned int frame,\n> -\t\t\t [[maybe_unused]] const ControlList &controls)\n> +void IPAIPU3::queueRequest(const uint32_t frame,\n> +\t\t\t [[maybe_unused]] const ControlList &controls)\n> {\n> \t/* \\todo Start processing for 'frame' based on 'controls'. */\n> }\n> @@ -600,10 +593,7 @@ void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params)\n> \tfor (auto const &algo : algorithms_)\n> \t\talgo->prepare(context_, params);\n> \n> -\tIPU3Action op;\n> -\top.op = ActionParamFilled;\n> -\n> -\tqueueFrameAction.emit(frame, op);\n> +\tparamsBufferReady.emit(frame);\n> }\n> \n> /**\n> @@ -647,11 +637,7 @@ void IPAIPU3::parseStatistics(unsigned int frame,\n> \t * likely want to avoid putting platform specific metadata in.\n> \t */\n> \n> -\tIPU3Action op;\n> -\top.op = ActionMetadataReady;\n> -\top.controls = ctrls;\n> -\n> -\tqueueFrameAction.emit(frame, op);\n> +\tmetadataReady.emit(frame, ctrls);\n> }\n> \n> /**\n> @@ -663,23 +649,18 @@ void IPAIPU3::parseStatistics(unsigned int frame,\n> */\n> void IPAIPU3::setControls(unsigned int frame)\n> {\n> -\tIPU3Action op;\n> -\top.op = ActionSetSensorControls;\n> -\n> \tint32_t exposure = context_.frameContext.agc.exposure;\n> \tint32_t gain = camHelper_->gainCode(context_.frameContext.agc.gain);\n> \n> \tControlList ctrls(sensorCtrls_);\n> \tctrls.set(V4L2_CID_EXPOSURE, exposure);\n> \tctrls.set(V4L2_CID_ANALOGUE_GAIN, gain);\n> -\top.sensorControls = ctrls;\n> \n> \tControlList lensCtrls(lensCtrls_);\n> \tlensCtrls.set(V4L2_CID_FOCUS_ABSOLUTE,\n> \t\t static_cast(context_.frameContext.af.focus));\n> -\top.lensControls = lensCtrls;\n> \n> -\tqueueFrameAction.emit(frame, op);\n> +\tsetSensorControls.emit(frame, ctrls, lensCtrls);\n> }\n> \n> } /* namespace ipa::ipu3 */\n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index 60e01917..59d7e9c0 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -86,8 +86,10 @@ public:\n> \tControlInfoMap ipaControls_;\n> \n> private:\n> -\tvoid queueFrameAction(unsigned int id,\n> -\t\t\t const ipa::ipu3::IPU3Action &action);\n> +\tvoid metadataReady(unsigned int id, const ControlList &metadata);\n> +\tvoid paramsFilled(unsigned int id);\n> +\tvoid setSensorControls(unsigned int id, const ControlList &sensorControls,\n> +\t\t\t const ControlList &lensControls);\n> };\n> \n> class IPU3CameraConfiguration : public CameraConfiguration\n> @@ -871,11 +873,7 @@ void IPU3CameraData::queuePendingRequests()\n> \n> \t\tinfo->rawBuffer = rawBuffer;\n> \n> -\t\tipa::ipu3::IPU3Event ev;\n> -\t\tev.op = ipa::ipu3::EventProcessControls;\n> -\t\tev.frame = info->id;\n> -\t\tev.controls = request->controls();\n> -\t\tipa_->processEvent(ev);\n> +\t\tipa_->queueRequest(info->id, request->controls());\n> \n> \t\tpendingRequests_.pop();\n> \t\tprocessingRequests_.push(request);\n> @@ -1218,7 +1216,9 @@ int IPU3CameraData::loadIPA()\n> \tif (!ipa_)\n> \t\treturn -ENOENT;\n> \n> -\tipa_->queueFrameAction.connect(this, &IPU3CameraData::queueFrameAction);\n> +\tipa_->setSensorControls.connect(this, &IPU3CameraData::setSensorControls);\n> +\tipa_->paramsBufferReady.connect(this, &IPU3CameraData::paramsFilled);\n> +\tipa_->metadataReady.connect(this, &IPU3CameraData::metadataReady);\n> \n> \t/*\n> \t * Pass the sensor info to the IPA to initialize controls.\n> @@ -1253,69 +1253,58 @@ int IPU3CameraData::loadIPA()\n> \treturn 0;\n> }\n> \n> -void IPU3CameraData::queueFrameAction(unsigned int id,\n> -\t\t\t\t const ipa::ipu3::IPU3Action &action)\n> +void IPU3CameraData::setSensorControls([[maybe_unused]] unsigned int id,\n> +\t\t\t\t const ControlList &sensorControls,\n> +\t\t\t\t const ControlList &lensControls)\n> {\n> -\tswitch (action.op) {\n> -\tcase ipa::ipu3::ActionSetSensorControls: {\n> -\t\tconst ControlList &sensorControls = action.sensorControls;\n> -\t\tdelayedCtrls_->push(sensorControls);\n> +\tdelayedCtrls_->push(sensorControls);\n> \n> -\t\tCameraLens *focusLens = cio2_.sensor()->focusLens();\n> -\t\tif (!focusLens)\n> -\t\t\tbreak;\n> -\n> -\t\tconst ControlList lensControls = action.lensControls;\n> -\t\tif (!lensControls.contains(V4L2_CID_FOCUS_ABSOLUTE))\n> -\t\t\tbreak;\n> +\tCameraLens *focusLens = cio2_.sensor()->focusLens();\n> +\tif (!focusLens)\n> +\t\treturn;\n> \n> -\t\tconst ControlValue &focusValue =\n> -\t\t\tlensControls.get(V4L2_CID_FOCUS_ABSOLUTE);\n> +\tif (!lensControls.contains(V4L2_CID_FOCUS_ABSOLUTE))\n> +\t\treturn;\n> \n> -\t\tfocusLens->setFocusPosition(focusValue.get());\n> +\tconst ControlValue &focusValue = lensControls.get(V4L2_CID_FOCUS_ABSOLUTE);\n> \n> -\t\tbreak;\n> -\t}\n> -\tcase ipa::ipu3::ActionParamFilled: {\n> -\t\tIPU3Frames::Info *info = frameInfos_.find(id);\n> -\t\tif (!info)\n> -\t\t\tbreak;\n> -\n> -\t\t/* Queue all buffers from the request aimed for the ImgU. */\n> -\t\tfor (auto it : info->request->buffers()) {\n> -\t\t\tconst Stream *stream = it.first;\n> -\t\t\tFrameBuffer *outbuffer = it.second;\n> +\tfocusLens->setFocusPosition(focusValue.get());\n> +}\n> \n> -\t\t\tif (stream == &outStream_)\n> -\t\t\t\timgu_->output_->queueBuffer(outbuffer);\n> -\t\t\telse if (stream == &vfStream_)\n> -\t\t\t\timgu_->viewfinder_->queueBuffer(outbuffer);\n> -\t\t}\n> +void IPU3CameraData::paramsFilled(unsigned int id)\n> +{\n> +\tIPU3Frames::Info *info = frameInfos_.find(id);\n> +\tif (!info)\n> +\t\treturn;\n> \n> -\t\timgu_->param_->queueBuffer(info->paramBuffer);\n> -\t\timgu_->stat_->queueBuffer(info->statBuffer);\n> -\t\timgu_->input_->queueBuffer(info->rawBuffer);\n> +\t/* Queue all buffers from the request aimed for the ImgU. */\n> +\tfor (auto it : info->request->buffers()) {\n> +\t\tconst Stream *stream = it.first;\n> +\t\tFrameBuffer *outbuffer = it.second;\n> \n> -\t\tbreak;\n> +\t\tif (stream == &outStream_)\n> +\t\t\timgu_->output_->queueBuffer(outbuffer);\n> +\t\telse if (stream == &vfStream_)\n> +\t\t\timgu_->viewfinder_->queueBuffer(outbuffer);\n> \t}\n> -\tcase ipa::ipu3::ActionMetadataReady: {\n> -\t\tIPU3Frames::Info *info = frameInfos_.find(id);\n> -\t\tif (!info)\n> -\t\t\tbreak;\n> \n> -\t\tRequest *request = info->request;\n> -\t\trequest->metadata().merge(action.controls);\n> +\timgu_->param_->queueBuffer(info->paramBuffer);\n> +\timgu_->stat_->queueBuffer(info->statBuffer);\n> +\timgu_->input_->queueBuffer(info->rawBuffer);\n> +}\n> \n> -\t\tinfo->metadataProcessed = true;\n> -\t\tif (frameInfos_.tryComplete(info))\n> -\t\t\tpipe()->completeRequest(request);\n> +void IPU3CameraData::metadataReady(unsigned int id, const ControlList &metadata)\n> +{\n> +\tIPU3Frames::Info *info = frameInfos_.find(id);\n> +\tif (!info)\n> +\t\treturn;\n> \n> -\t\tbreak;\n> -\t}\n> -\tdefault:\n> -\t\tLOG(IPU3, Error) << \"Unknown action \" << action.op;\n> -\t\tbreak;\n> -\t}\n> +\tRequest *request = info->request;\n> +\trequest->metadata().merge(metadata);\n> +\n> +\tinfo->metadataProcessed = true;\n> +\tif (frameInfos_.tryComplete(info))\n> +\t\tpipe()->completeRequest(request);\n> }\n> \n> /* -----------------------------------------------------------------------------\n> @@ -1390,11 +1379,7 @@ void IPU3CameraData::cio2BufferReady(FrameBuffer *buffer)\n> \tif (request->findBuffer(&rawStream_))\n> \t\tpipe()->completeBuffer(request, buffer);\n> \n> -\tipa::ipu3::IPU3Event ev;\n> -\tev.op = ipa::ipu3::EventFillParams;\n> -\tev.frame = info->id;\n> -\tev.bufferId = info->paramBuffer->cookie();\n> -\tipa_->processEvent(ev);\n> +\tipa_->fillParamsBuffer(info->id, info->paramBuffer->cookie());\n> }\n> \n> void IPU3CameraData::paramBufferReady(FrameBuffer *buffer)\n> @@ -1438,13 +1423,8 @@ void IPU3CameraData::statBufferReady(FrameBuffer *buffer)\n> \t\treturn;\n> \t}\n> \n> -\tipa::ipu3::IPU3Event ev;\n> -\tev.op = ipa::ipu3::EventStatReady;\n> -\tev.frame = info->id;\n> -\tev.bufferId = info->statBuffer->cookie();\n> -\tev.frameTimestamp = request->metadata().get(controls::SensorTimestamp);\n> -\tev.sensorControls = info->effectiveSensorControls;\n> -\tipa_->processEvent(ev);\n> +\tipa_->processStatsBuffer(info->id, request->metadata().get(controls::SensorTimestamp),\n> +\t\t\t\t info->statBuffer->cookie(), info->effectiveSensorControls);\n> }\n> \n> /*\n> -- \n> 2.31.0\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 81D22C3256\n\tfor ;\n\tFri, 8 Apr 2022 06:55:40 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7990B65644;\n\tFri, 8 Apr 2022 08:55:39 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4629A604B4\n\tfor ;\n\tFri, 8 Apr 2022 08:55:37 +0200 (CEST)","from pyrite.rasen.tech (softbank036240056250.bbtec.net\n\t[36.240.56.250])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 42E02499;\n\tFri, 8 Apr 2022 08:55:34 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1649400939;\n\tbh=ktdS/tK31OT2AKik43FsQHM4V5MujzqBGypOH181/qs=;\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=r0oYmhoEo26GGOZR4g798dMjWSRmPRwbFkXNuYVEGTMZHNa+M0UqBqfmMcLPzC1gq\n\tJC2j1YGua4qVDEOjhirAJIzR3uOFt/Ourc+NAEbIGhL7bf8UhAgNkFpDJKYyxLrqQI\n\tHaJVZmheqcsjMdl2qw52hCHiLgFj/NuL/h1r5I0TW9xQPsd/SsIHBYZjb+fZ6ofzS2\n\t9dYAV3XYPfjCoHo35gH1HPSAY5Cng4xOg5PjM6iOY4hHbxv8UpPBeu53yEpiUuSJU1\n\tW4laGCtofRz6v/+Zuf0HXtGmQrWHoZRPJS+1t8oK+NffixUc14U3hiQ6879w3WFFHj\n\teUjV4gRNykA+Q==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1649400936;\n\tbh=ktdS/tK31OT2AKik43FsQHM4V5MujzqBGypOH181/qs=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=lmM2Bs5xHgw8lQkwAhbH4LgwmGYcZoXcF1QIsWBUX33+/R80eTkjPQtvsKftlY8Dw\n\tJVtKhfQPfHsqA0O/l1DLqRiz40JuRtSE+OVME/TfRjvwFfcX4KvffhnDi/Mb71Glv6\n\t58Q6Hi0I0Bmb3nuzfNTs0T9aBwlIrMyoYZP5neBc="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"lmM2Bs5x\"; dkim-atps=neutral","Date":"Fri, 8 Apr 2022 15:55:29 +0900","To":"Umang Jain ","Message-ID":"<20220408065529.GL3237525@pyrite.rasen.tech>","References":"<20220406141709.164794-1-umang.jain@ideasonboard.com>\n\t<20220406141709.164794-2-umang.jain@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20220406141709.164794-2-umang.jain@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v5 1/6] ipa: ipu3: Replace event-based\n\tops with dedicated functions","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":"Paul Elder via libcamera-devel ","Reply-To":"paul.elder@ideasonboard.com","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":22659,"web_url":"https://patchwork.libcamera.org/comment/22659/","msgid":"<20220408070119.GM3237525@pyrite.rasen.tech>","date":"2022-04-08T07:01:19","subject":"Re: [libcamera-devel] [PATCH v5 1/6] ipa: ipu3: Replace event-based\n\tops with dedicated functions","submitter":{"id":97,"url":"https://patchwork.libcamera.org/api/people/97/","name":"Nicolas Dufresne via libcamera-devel","email":"libcamera-devel@lists.libcamera.org"},"content":"Hi Kieran,\n\nOn Thu, Apr 07, 2022 at 11:11:34PM +0100, Kieran Bingham via libcamera-devel wrote:\n> Quoting Umang Jain via libcamera-devel (2022-04-06 15:17:04)\n> > The IPAIPU3 interface currently uses event-type based structures in\n> > order to communicate with the pipeline-handler (and vice-versa).\n> > Replace the event based structures with dedicated functions associated\n> > to each operation.\n> > \n> > The translated naming scheme of operations to dedicated functions:\n> \n> So I think these without () are signals? If so we should state that here?\n> \n> The Actions are translated to signals:\n> > ActionSetSensorControls => setSensorControls\n> > ActionParamFilled => paramsBufferReady\n> > ActionMetadataReady => metadataReady\n> \n> And Events are translated to dedicated functions:\n> \n> > EventProcessControls => queueRequest()\n> > EventStatReady => processStatsBuffer()\n> > EventFillParams => fillParamsBuffer()\n> > \n> \n> Is there really a distinction between the use of signals and functions\n> in either direction? or is it ultimately just signals both ways? (Or is\n> it signals connected to functions of the same name? in both ways?)\n\nPipeline handler -> IPA calls are functions calls, from both the\nperspective of the pipeline handler and the IPA. That is, the pipeline\nhandler can do ipa_->queueRequest() and the IPA implements a\nqueueRequest function. Under the hood it's either invokeMethod or IPC\nsend, so it's not a signal.\n\nIPA -> pipeline handler is signal emission. The IPA emits a signal and\nthe pipeline handler handles it with a slot.\n\n> Anyway - that bit doesn't really matter it's just the commit message.\n> \n> It's so hard to parse this patch. We really need to make sure something\n> like this is split from the start next time.\n\ntbh yeah it is. I'm used to this pattern of patch though so it wasn't\nthat bad :p\n\n\nPaul\n\n> \n> I have one question about paramsBufferReady below - but I can't see\n> anything else standing out so with that resolved or otherwise:\n> \n> Reviewed-by: Kieran Bingham \n> \n> > Signed-off-by: Umang Jain \n> > Reviewed-by: Laurent Pinchart \n> > ---\n> > include/libcamera/ipa/ipu3.mojom | 36 ++-----\n> > src/ipa/ipu3/ipu3-ipa-design-guide.rst | 27 +++---\n> > src/ipa/ipu3/ipu3.cpp | 129 +++++++++++--------------\n> > src/libcamera/pipeline/ipu3/ipu3.cpp | 122 ++++++++++-------------\n> > 4 files changed, 130 insertions(+), 184 deletions(-)\n> > \n> > diff --git a/include/libcamera/ipa/ipu3.mojom b/include/libcamera/ipa/ipu3.mojom\n> > index 18cdc744..d1b1c6b8 100644\n> > --- a/include/libcamera/ipa/ipu3.mojom\n> > +++ b/include/libcamera/ipa/ipu3.mojom\n> > @@ -8,32 +8,6 @@ module ipa.ipu3;\n> > \n> > import \"include/libcamera/ipa/core.mojom\";\n> > \n> > -enum IPU3Operations {\n> > - ActionSetSensorControls = 1,\n> > - ActionParamFilled = 2,\n> > - ActionMetadataReady = 3,\n> > - EventProcessControls = 4,\n> > - EventStatReady = 5,\n> > - EventFillParams = 6,\n> > -};\n> > -\n> > -struct IPU3Event {\n> > - IPU3Operations op;\n> > - uint32 frame;\n> > - int64 frameTimestamp;\n> > - uint32 bufferId;\n> > - libcamera.ControlList controls;\n> > - libcamera.ControlList sensorControls;\n> > - libcamera.ControlList lensControls;\n> > -};\n> > -\n> > -struct IPU3Action {\n> > - IPU3Operations op;\n> > - libcamera.ControlList controls;\n> > - libcamera.ControlList sensorControls;\n> > - libcamera.ControlList lensControls;\n> > -};\n> > -\n> > struct IPAConfigInfo {\n> > libcamera.IPACameraSensorInfo sensorInfo;\n> > libcamera.ControlInfoMap sensorControls;\n> > @@ -56,9 +30,15 @@ interface IPAIPU3Interface {\n> > mapBuffers(array buffers);\n> > unmapBuffers(array ids);\n> > \n> > - [async] processEvent(IPU3Event ev);\n> > + [async] queueRequest(uint32 frame, libcamera.ControlList controls);\n> > + [async] fillParamsBuffer(uint32 frame, uint32 bufferId);\n> > + [async] processStatsBuffer(uint32 frame, int64 frameTimestamp,\n> > + uint32 bufferId, libcamera.ControlList sensorControls);\n> > };\n> > \n> > interface IPAIPU3EventInterface {\n> > - queueFrameAction(uint32 frame, IPU3Action action);\n> > + setSensorControls(uint32 frame, libcamera.ControlList sensorControls,\n> > + libcamera.ControlList lensControls);\n> > + paramsBufferReady(uint32 frame);\n> > + metadataReady(uint32 frame, libcamera.ControlList metadata);\n> > };\n> > diff --git a/src/ipa/ipu3/ipu3-ipa-design-guide.rst b/src/ipa/ipu3/ipu3-ipa-design-guide.rst\n> > index 89c71108..e724fdda 100644\n> > --- a/src/ipa/ipu3/ipu3-ipa-design-guide.rst\n> > +++ b/src/ipa/ipu3/ipu3-ipa-design-guide.rst\n> > @@ -25,7 +25,8 @@ from applications, and managing events from the pipeline handler.\n> > └─┬───┬───┬──────┬────┬────┬────┬─┴────▼─┬──┘ 1: init()\n> > │ │ │ │ ▲ │ ▲ │ ▲ │ ▲ │ 2: configure()\n> > │1 │2 │3 │4│ │4│ │4│ │4│ │5 3: mapBuffers(), start()\n> > - ▼ ▼ ▼ ▼ │ ▼ │ ▼ │ ▼ │ ▼ 4: processEvent()\n> > + │ │ │ │ │ │ │ │ │ │ │ │ 4: (▼) queueRequest(), fillParamsBuffer(), processStatsBuffer()\n> > + ▼ ▼ ▼ ▼ │ ▼ │ ▼ │ ▼ │ ▼ (▲) setSensorControls, paramsBufferReady, metadataReady Signals\n> \n> I like that this is more descriptive about the direction and operations\n> that occur in 4.\n> \n> > ┌──────────────────┴────┴────┴────┴─────────┐ 5: stop(), unmapBuffers()\n> > │ IPU3 IPA │\n> > │ ┌───────────────────────┐ │\n> > @@ -100,8 +101,9 @@ There are three main interactions with the algorithms for the IPU3 IPA\n> > to operate when running:\n> > \n> > - configure()\n> > -- processEvent(``EventFillParams``)\n> > -- processEvent(``EventStatReady``)\n> > +- queueRequest()\n> > +- fillParamsBuffer()\n> > +- processStatsBuffer()\n> > \n> > The configuration phase allows the pipeline-handler to inform the IPA of\n> > the current stream configurations, which is then passed into each\n> > @@ -114,8 +116,8 @@ Pre-frame preparation\n> > When configured, the IPA is notified by the pipeline handler of the\n> > Camera ``start()`` event, after which incoming requests will be queued\n> > for processing, requiring a parameter buffer (``ipu3_uapi_params``) to\n> > -be populated for the ImgU. This is given to the IPA through the\n> > -``EventFillParams`` event, and then passed directly to each algorithm\n> > +be populated for the ImgU. This is given to the IPA through\n> > +``fillParamsBuffer()``, and then passed directly to each algorithm\n> > through the ``prepare()`` call allowing the ISP configuration to be\n> > updated for the needs of each component that the algorithm is\n> > responsible for.\n> > @@ -125,7 +127,7 @@ structure that it modifies, and it should take care to ensure that any\n> > structure set by a use flag is fully initialised to suitable values.\n> > \n> > The parameter buffer is returned to the pipeline handler through the\n> > -``ActionParamFilled`` event, and from there queued to the ImgU along\n> > +``paramsBufferReady`` signal, and from there queued to the ImgU along\n> > with a raw frame captured with the CIO2.\n> > \n> > Post-frame completion\n> > @@ -133,8 +135,8 @@ Post-frame completion\n> > \n> > When the capture of an image is completed, and successfully processed\n> > through the ImgU, the generated statistics buffer\n> > -(``ipu3_uapi_stats_3a``) is given to the IPA through the\n> > -``EventStatReady`` event. This provides the IPA with an opportunity to\n> > +(``ipu3_uapi_stats_3a``) is given to the IPA through\n> > +``processStatsBuffer()``. This provides the IPA with an opportunity to\n> > examine the results of the ISP and run the calculations required by each\n> > algorithm on the new data. The algorithms may require context from the\n> > operations of other algorithms, for example, the AWB might choose to use\n> > @@ -145,11 +147,14 @@ before they are needed.\n> > The ordering of the algorithm processing is determined by their\n> > placement in the ``IPU3::algorithms_`` ordered list.\n> > \n> > +Finally, the IPA metadata for the completed frame is returned back via\n> > +the ``metadataReady`` signal.\n> > +\n> > Sensor Controls\n> > ~~~~~~~~~~~~~~~\n> > \n> > The AutoExposure and AutoGain (AGC) algorithm differs slightly from the\n> > others as it requires operating directly on the sensor, as opposed to\n> > -through the ImgU ISP. To support this, there is a dedicated action\n> > -`ActionSetSensorControls` to allow the IPA to request controls to be set\n> > -on the camera sensor through the pipeline handler.\n> > +through the ImgU ISP. To support this, there is a ``setSensorControls``\n> > +signal to allow the IPA to request controls to be set on the camera\n> > +sensor through the pipeline handler.\n> > diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> > index 50b52d8b..9a59f80f 100644\n> > --- a/src/ipa/ipu3/ipu3.cpp\n> > +++ b/src/ipa/ipu3/ipu3.cpp\n> > @@ -65,8 +65,9 @@ namespace ipa::ipu3 {\n> > * The IPU3 Pipeline defines an IPU3-specific interface for communication\n> > * between the PipelineHandler and the IPA module.\n> > *\n> > - * We extend the IPAIPU3Interface to implement our algorithms and handle events\n> > - * from the IPU3 PipelineHandler to satisfy requests from the application.\n> > + * We extend the IPAIPU3Interface to implement our algorithms and handle\n> > + * calls from the IPU3 PipelineHandler to satisfy requests from the\n> > + * application.\n> > *\n> > * At initialisation time, a CameraSensorHelper is instantiated to support\n> > * camera-specific calculations, while the default controls are computed, and\n> > @@ -81,14 +82,14 @@ namespace ipa::ipu3 {\n> > * parameter buffer, and adapting the settings of the sensor attached to the\n> > * IPU3 CIO2 through sensor-specific V4L2 controls.\n> > *\n> > - * When the event \\a EventFillParams occurs we populate the ImgU parameter\n> > - * buffer with settings to configure the device in preparation for handling the\n> > - * frame queued in the Request.\n> > + * In fillParamsBuffer(), we populate the ImgU parameter buffer with\n> > + * settings to configure the device in preparation for handling the frame\n> > + * queued in the Request.\n> > *\n> > * When the frame has completed processing, the ImgU will generate a statistics\n> > - * buffer which is given to the IPA as part of the \\a EventStatReady event. At\n> > - * this event we run the algorithms to parse the statistics and cache any\n> > - * results for the next \\a EventFillParams event.\n> > + * buffer which is given to the IPA with processStatsBuffer(). In this we run the\n> > + * algorithms to parse the statistics and cache any results for the next\n> > + * fillParamsBuffer() call.\n> > *\n> > * The individual algorithms are split into modular components that are called\n> > * iteratively to allow them to process statistics from the ImgU in a defined\n> > @@ -143,14 +144,18 @@ public:\n> > \n> > void mapBuffers(const std::vector &buffers) override;\n> > void unmapBuffers(const std::vector &ids) override;\n> > - void processEvent(const IPU3Event &event) override;\n> > \n> > + void queueRequest(const uint32_t frame, const ControlList &controls) override;\n> > + void fillParamsBuffer(const uint32_t frame, const uint32_t bufferId) override;\n> > + void processStatsBuffer(const uint32_t frame, const int64_t frameTimestamp,\n> > + const uint32_t bufferId,\n> > + const ControlList &sensorControls) override;\n> > private:\n> > void updateControls(const IPACameraSensorInfo &sensorInfo,\n> > const ControlInfoMap &sensorControls,\n> > ControlInfoMap *ipaControls);\n> > void updateSessionConfiguration(const ControlInfoMap &sensorControls);\n> > - void processControls(unsigned int frame, const ControlList &controls);\n> > +\n> > void fillParams(unsigned int frame, ipu3_uapi_params *params);\n> > void parseStatistics(unsigned int frame,\n> > int64_t frameTimestamp,\n> > @@ -505,73 +510,61 @@ void IPAIPU3::unmapBuffers(const std::vector &ids)\n> > }\n> > \n> > /**\n> > - * \\brief Process an event generated by the pipeline handler\n> > - * \\param[in] event The event sent from pipeline handler\n> > - *\n> > - * The expected event handling over the lifetime of a Request has\n> > - * the following sequence:\n> > - *\n> > - * - EventProcessControls : Handle controls from a new Request\n> > - * - EventFillParams : Prepare the ISP to process the Request\n> > - * - EventStatReady : Process statistics after ISP completion\n> > + * \\brief Fill and return a buffer with ISP processing parameters for a frame\n> > + * \\param[in] frame The frame number\n> > + * \\param[in] bufferId ID of the parameter buffer to fill\n> > */\n> > -void IPAIPU3::processEvent(const IPU3Event &event)\n> > +void IPAIPU3::fillParamsBuffer(const uint32_t frame, const uint32_t bufferId)\n> > {\n> > - switch (event.op) {\n> > - case EventProcessControls: {\n> > - processControls(event.frame, event.controls);\n> > - break;\n> > + auto it = buffers_.find(bufferId);\n> > + if (it == buffers_.end()) {\n> > + LOG(IPAIPU3, Error) << \"Could not find param buffer!\";\n> > + return;\n> > }\n> > - case EventFillParams: {\n> > - auto it = buffers_.find(event.bufferId);\n> > - if (it == buffers_.end()) {\n> > - LOG(IPAIPU3, Error) << \"Could not find param buffer!\";\n> > - return;\n> > - }\n> > \n> > - Span mem = it->second.planes()[0];\n> > - ipu3_uapi_params *params =\n> > - reinterpret_cast(mem.data());\n> > + Span mem = it->second.planes()[0];\n> > + ipu3_uapi_params *params =\n> > + reinterpret_cast(mem.data());\n> > \n> > - fillParams(event.frame, params);\n> > - break;\n> > - }\n> > - case EventStatReady: {\n> > - auto it = buffers_.find(event.bufferId);\n> > - if (it == buffers_.end()) {\n> > - LOG(IPAIPU3, Error) << \"Could not find stats buffer!\";\n> > - return;\n> > - }\n> > + fillParams(frame, params);\n> > +}\n> > \n> > - Span mem = it->second.planes()[0];\n> > - const ipu3_uapi_stats_3a *stats =\n> > - reinterpret_cast(mem.data());\n> > +/**\n> > + * \\brief Process statistics after ISP completion\n> > + * \\param[in] frame The frame number\n> > + * \\param[in] frameTimestamp Timestamp of the frame\n> > + * \\param[in] bufferId ID of the statistics buffer\n> > + * \\param[in] sensorControls Sensor controls\n> > + */\n> > +void IPAIPU3::processStatsBuffer(const uint32_t frame, const int64_t frameTimestamp,\n> > + const uint32_t bufferId, const ControlList &sensorControls)\n> > +{\n> > + auto it = buffers_.find(bufferId);\n> > + if (it == buffers_.end()) {\n> > + LOG(IPAIPU3, Error) << \"Could not find stats buffer!\";\n> > + return;\n> > + }\n> > \n> > - int32_t exposure = event.sensorControls.get(V4L2_CID_EXPOSURE).get();\n> > - int32_t gain = event.sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get();\n> > + Span mem = it->second.planes()[0];\n> > + const ipu3_uapi_stats_3a *stats =\n> > + reinterpret_cast(mem.data());\n> > \n> > - context_.frameContext.sensor.exposure = exposure;\n> > - context_.frameContext.sensor.gain = camHelper_->gain(gain);\n> > + context_.frameContext.sensor.exposure = sensorControls.get(V4L2_CID_EXPOSURE).get();\n> > + context_.frameContext.sensor.gain = camHelper_->gain(sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get());\n> > \n> > - parseStatistics(event.frame, event.frameTimestamp, stats);\n> > - break;\n> > - }\n> > - default:\n> > - LOG(IPAIPU3, Error) << \"Unknown event \" << event.op;\n> > - break;\n> > - }\n> > + parseStatistics(frame, frameTimestamp, stats);\n> > }\n> > \n> > /**\n> > - * \\brief Process a control list for a request from the application\n> > + * \\brief Queue a request and process the control list from the application\n> > * \\param[in] frame The number of the frame which will be processed next\n> > * \\param[in] controls The controls for the \\a frame\n> > *\n> > * Parse the request to handle any IPA-managed controls that were set from the\n> > * application such as manual sensor settings.\n> > */\n> > -void IPAIPU3::processControls([[maybe_unused]] unsigned int frame,\n> > - [[maybe_unused]] const ControlList &controls)\n> > +void IPAIPU3::queueRequest(const uint32_t frame,\n> > + [[maybe_unused]] const ControlList &controls)\n> > {\n> > /* \\todo Start processing for 'frame' based on 'controls'. */\n> > }\n> > @@ -600,10 +593,7 @@ void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params)\n> > for (auto const &algo : algorithms_)\n> > algo->prepare(context_, params);\n> > \n> > - IPU3Action op;\n> > - op.op = ActionParamFilled;\n> > -\n> > - queueFrameAction.emit(frame, op);\n> > + paramsBufferReady.emit(frame);\n> > }\n> > \n> > /**\n> > @@ -647,11 +637,7 @@ void IPAIPU3::parseStatistics(unsigned int frame,\n> > * likely want to avoid putting platform specific metadata in.\n> > */\n> > \n> > - IPU3Action op;\n> > - op.op = ActionMetadataReady;\n> > - op.controls = ctrls;\n> > -\n> > - queueFrameAction.emit(frame, op);\n> > + metadataReady.emit(frame, ctrls);\n> > }\n> > \n> > /**\n> > @@ -663,23 +649,18 @@ void IPAIPU3::parseStatistics(unsigned int frame,\n> > */\n> > void IPAIPU3::setControls(unsigned int frame)\n> > {\n> > - IPU3Action op;\n> > - op.op = ActionSetSensorControls;\n> > -\n> > int32_t exposure = context_.frameContext.agc.exposure;\n> > int32_t gain = camHelper_->gainCode(context_.frameContext.agc.gain);\n> > \n> > ControlList ctrls(sensorCtrls_);\n> > ctrls.set(V4L2_CID_EXPOSURE, exposure);\n> > ctrls.set(V4L2_CID_ANALOGUE_GAIN, gain);\n> > - op.sensorControls = ctrls;\n> > \n> > ControlList lensCtrls(lensCtrls_);\n> > lensCtrls.set(V4L2_CID_FOCUS_ABSOLUTE,\n> > static_cast(context_.frameContext.af.focus));\n> > - op.lensControls = lensCtrls;\n> > \n> > - queueFrameAction.emit(frame, op);\n> > + setSensorControls.emit(frame, ctrls, lensCtrls);\n> > }\n> > \n> > } /* namespace ipa::ipu3 */\n> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > index 60e01917..59d7e9c0 100644\n> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > @@ -86,8 +86,10 @@ public:\n> > ControlInfoMap ipaControls_;\n> > \n> > private:\n> > - void queueFrameAction(unsigned int id,\n> > - const ipa::ipu3::IPU3Action &action);\n> > + void metadataReady(unsigned int id, const ControlList &metadata);\n> > + void paramsFilled(unsigned int id);\n> \n> This is confusing. ActionParamFilled becomes paramsBufferReady in the\n> commit message. Is this function used?\n> \n> > + void setSensorControls(unsigned int id, const ControlList &sensorControls,\n> > + const ControlList &lensControls);\n> > };\n> > \n> > class IPU3CameraConfiguration : public CameraConfiguration\n> > @@ -871,11 +873,7 @@ void IPU3CameraData::queuePendingRequests()\n> > \n> > info->rawBuffer = rawBuffer;\n> > \n> > - ipa::ipu3::IPU3Event ev;\n> > - ev.op = ipa::ipu3::EventProcessControls;\n> > - ev.frame = info->id;\n> > - ev.controls = request->controls();\n> > - ipa_->processEvent(ev);\n> > + ipa_->queueRequest(info->id, request->controls());\n> > \n> > pendingRequests_.pop();\n> > processingRequests_.push(request);\n> > @@ -1218,7 +1216,9 @@ int IPU3CameraData::loadIPA()\n> > if (!ipa_)\n> > return -ENOENT;\n> > \n> > - ipa_->queueFrameAction.connect(this, &IPU3CameraData::queueFrameAction);\n> > + ipa_->setSensorControls.connect(this, &IPU3CameraData::setSensorControls);\n> > + ipa_->paramsBufferReady.connect(this, &IPU3CameraData::paramsFilled);\n> \n> Oh - I see - the signals are connected to functions of the same name\n> within the IPU3CameraData.\n> \n> Is there a reason why \n> queueFrameAction => queueFrameAction,\n> setSensorControls => setSensorControls,\n> metadataReady => metadataReady,\n> but\n> paramsBufferReady => paramsFilled?\n> \n> \n> \n> > + ipa_->metadataReady.connect(this, &IPU3CameraData::metadataReady);\n> > \n> > /*\n> > * Pass the sensor info to the IPA to initialize controls.\n> > @@ -1253,69 +1253,58 @@ int IPU3CameraData::loadIPA()\n> > return 0;\n> > }\n> > \n> > -void IPU3CameraData::queueFrameAction(unsigned int id,\n> > - const ipa::ipu3::IPU3Action &action)\n> > +void IPU3CameraData::setSensorControls([[maybe_unused]] unsigned int id,\n> > + const ControlList &sensorControls,\n> > + const ControlList &lensControls)\n> > {\n> > - switch (action.op) {\n> > - case ipa::ipu3::ActionSetSensorControls: {\n> > - const ControlList &sensorControls = action.sensorControls;\n> > - delayedCtrls_->push(sensorControls);\n> > + delayedCtrls_->push(sensorControls);\n> > \n> > - CameraLens *focusLens = cio2_.sensor()->focusLens();\n> > - if (!focusLens)\n> > - break;\n> > -\n> > - const ControlList lensControls = action.lensControls;\n> > - if (!lensControls.contains(V4L2_CID_FOCUS_ABSOLUTE))\n> > - break;\n> > + CameraLens *focusLens = cio2_.sensor()->focusLens();\n> > + if (!focusLens)\n> > + return;\n> > \n> > - const ControlValue &focusValue =\n> > - lensControls.get(V4L2_CID_FOCUS_ABSOLUTE);\n> > + if (!lensControls.contains(V4L2_CID_FOCUS_ABSOLUTE))\n> > + return;\n> > \n> > - focusLens->setFocusPosition(focusValue.get());\n> > + const ControlValue &focusValue = lensControls.get(V4L2_CID_FOCUS_ABSOLUTE);\n> > \n> > - break;\n> > - }\n> > - case ipa::ipu3::ActionParamFilled: {\n> > - IPU3Frames::Info *info = frameInfos_.find(id);\n> > - if (!info)\n> > - break;\n> > -\n> > - /* Queue all buffers from the request aimed for the ImgU. */\n> > - for (auto it : info->request->buffers()) {\n> > - const Stream *stream = it.first;\n> > - FrameBuffer *outbuffer = it.second;\n> > + focusLens->setFocusPosition(focusValue.get());\n> > +}\n> > \n> > - if (stream == &outStream_)\n> > - imgu_->output_->queueBuffer(outbuffer);\n> > - else if (stream == &vfStream_)\n> > - imgu_->viewfinder_->queueBuffer(outbuffer);\n> > - }\n> > +void IPU3CameraData::paramsFilled(unsigned int id)\n> > +{\n> > + IPU3Frames::Info *info = frameInfos_.find(id);\n> > + if (!info)\n> > + return;\n> > \n> > - imgu_->param_->queueBuffer(info->paramBuffer);\n> > - imgu_->stat_->queueBuffer(info->statBuffer);\n> > - imgu_->input_->queueBuffer(info->rawBuffer);\n> > + /* Queue all buffers from the request aimed for the ImgU. */\n> > + for (auto it : info->request->buffers()) {\n> > + const Stream *stream = it.first;\n> > + FrameBuffer *outbuffer = it.second;\n> > \n> > - break;\n> > + if (stream == &outStream_)\n> > + imgu_->output_->queueBuffer(outbuffer);\n> > + else if (stream == &vfStream_)\n> > + imgu_->viewfinder_->queueBuffer(outbuffer);\n> > }\n> > - case ipa::ipu3::ActionMetadataReady: {\n> > - IPU3Frames::Info *info = frameInfos_.find(id);\n> > - if (!info)\n> > - break;\n> > \n> > - Request *request = info->request;\n> > - request->metadata().merge(action.controls);\n> > + imgu_->param_->queueBuffer(info->paramBuffer);\n> > + imgu_->stat_->queueBuffer(info->statBuffer);\n> > + imgu_->input_->queueBuffer(info->rawBuffer);\n> > +}\n> > \n> > - info->metadataProcessed = true;\n> > - if (frameInfos_.tryComplete(info))\n> > - pipe()->completeRequest(request);\n> > +void IPU3CameraData::metadataReady(unsigned int id, const ControlList &metadata)\n> > +{\n> > + IPU3Frames::Info *info = frameInfos_.find(id);\n> > + if (!info)\n> > + return;\n> > \n> > - break;\n> > - }\n> > - default:\n> > - LOG(IPU3, Error) << \"Unknown action \" << action.op;\n> > - break;\n> > - }\n> > + Request *request = info->request;\n> > + request->metadata().merge(metadata);\n> > +\n> > + info->metadataProcessed = true;\n> > + if (frameInfos_.tryComplete(info))\n> > + pipe()->completeRequest(request);\n> > }\n> > \n> > /* -----------------------------------------------------------------------------\n> > @@ -1390,11 +1379,7 @@ void IPU3CameraData::cio2BufferReady(FrameBuffer *buffer)\n> > if (request->findBuffer(&rawStream_))\n> > pipe()->completeBuffer(request, buffer);\n> > \n> > - ipa::ipu3::IPU3Event ev;\n> > - ev.op = ipa::ipu3::EventFillParams;\n> > - ev.frame = info->id;\n> > - ev.bufferId = info->paramBuffer->cookie();\n> > - ipa_->processEvent(ev);\n> > + ipa_->fillParamsBuffer(info->id, info->paramBuffer->cookie());\n> > }\n> > \n> > void IPU3CameraData::paramBufferReady(FrameBuffer *buffer)\n> > @@ -1438,13 +1423,8 @@ void IPU3CameraData::statBufferReady(FrameBuffer *buffer)\n> > return;\n> > }\n> > \n> > - ipa::ipu3::IPU3Event ev;\n> > - ev.op = ipa::ipu3::EventStatReady;\n> > - ev.frame = info->id;\n> > - ev.bufferId = info->statBuffer->cookie();\n> > - ev.frameTimestamp = request->metadata().get(controls::SensorTimestamp);\n> > - ev.sensorControls = info->effectiveSensorControls;\n> > - ipa_->processEvent(ev);\n> > + ipa_->processStatsBuffer(info->id, request->metadata().get(controls::SensorTimestamp),\n> > + info->statBuffer->cookie(), info->effectiveSensorControls);\n> > }\n> > \n> > /*\n> > -- \n> > 2.31.0\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 98B9AC0F1B\n\tfor ;\n\tFri, 8 Apr 2022 07:01:29 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E579765640;\n\tFri, 8 Apr 2022 09:01:28 +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 34AD161FBA\n\tfor ;\n\tFri, 8 Apr 2022 09:01:27 +0200 (CEST)","from pyrite.rasen.tech (softbank036240056250.bbtec.net\n\t[36.240.56.250])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 0F724499;\n\tFri, 8 Apr 2022 09:01:24 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1649401288;\n\tbh=wogDohXeY56I5VMl3VowuXDwdmaz86x5M/MieMVt0xE=;\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=T/Jx4JKfaYSmByTNgv56CPySXxANqR8UmAaKTvW7heTbrOsTeOxZJLPjoC6HEYMS6\n\tKfe+Rey40zdARXnZ1APOVPPUN7ku/R/uqW+IoGuwq7wZJhHIJTLcHriZC2BGXXxl9o\n\tq2YGqfKARnFn+LDUxAgNxJbiEHlquqFhW+YARkJZU0oy6t0jOvjkRqVTRxRhJptOiv\n\t3K7L9i3HtenWZ6IOI47seES2eBFVo1pANYdF7Ra+PFcrtDSZg1XszXf2DHQQJ0AXof\n\tLWmgEyEuM/OtKsdEs/maz/93daqahmttqb2iFKudP5NXuFnmkV1QFDE3Zl6d0813zG\n\tot/WahtlQQavw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1649401286;\n\tbh=wogDohXeY56I5VMl3VowuXDwdmaz86x5M/MieMVt0xE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=eqcpN70YlK7bzNyE2/mBUyzriBIcY6wheGyFQcsSa56XP4/L1eIak6HaFGCxOIpFL\n\ts9Wie2JeLr49Si6ILg+JEXQFgCQvFMyiPebo1rZ1pBJVzeU+4JAs87l0IS5zKpDfft\n\tsUHZYicUnw/ZN9Rpl+RL8+WUCnNIi7MDxMzOo3og="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"eqcpN70Y\"; dkim-atps=neutral","Date":"Fri, 8 Apr 2022 16:01:19 +0900","To":"Kieran Bingham ","Message-ID":"<20220408070119.GM3237525@pyrite.rasen.tech>","References":"<20220406141709.164794-1-umang.jain@ideasonboard.com>\n\t<20220406141709.164794-2-umang.jain@ideasonboard.com>\n\t<164936949446.22830.7307396101214349800@Monstersaurus>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<164936949446.22830.7307396101214349800@Monstersaurus>","Subject":"Re: [libcamera-devel] [PATCH v5 1/6] ipa: ipu3: Replace event-based\n\tops with dedicated functions","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":"Paul Elder via libcamera-devel ","Reply-To":"paul.elder@ideasonboard.com","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":22661,"web_url":"https://patchwork.libcamera.org/comment/22661/","msgid":"","date":"2022-04-08T07:04:03","subject":"Re: [libcamera-devel] [PATCH v5 1/6] ipa: ipu3: Replace event-based\n\tops with dedicated functions","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Kieran\n\nOn 4/8/22 03:41, Kieran Bingham wrote:\n> Quoting Umang Jain via libcamera-devel (2022-04-06 15:17:04)\n>> The IPAIPU3 interface currently uses event-type based structures in\n>> order to communicate with the pipeline-handler (and vice-versa).\n>> Replace the event based structures with dedicated functions associated\n>> to each operation.\n>>\n>> The translated naming scheme of operations to dedicated functions:\n> So I think these without () are signals? If so we should state that here?\n\n\nYes, it should clarify the commit message a bit here.\n\n>\n> The Actions are translated to signals:\n>> ActionSetSensorControls => setSensorControls\n>> ActionParamFilled => paramsBufferReady\n>> ActionMetadataReady => metadataReady\n> And Events are translated to dedicated functions:\n>\n>> EventProcessControls => queueRequest()\n>> EventStatReady => processStatsBuffer()\n>> EventFillParams => fillParamsBuffer()\n>>\n> Is there really a distinction between the use of signals and functions\n> in either direction? or is it ultimately just signals both ways? (Or is\n> it signals connected to functions of the same name? in both ways?)\n\n\nNot really. Dedicated functions are called by the PH to the IPA.\n\nThe IPA runs the functions (asynchronously) as per the calls invoked by \nthe PH and completes them by emitting the Signals.\n\nThis is how the `[async]` is introduced in the mojom interface I believe.\n\n\n>\n> Anyway - that bit doesn't really matter it's just the commit message.\n>\n> It's so hard to parse this patch. We really need to make sure something\n> like this is split from the start next time.\n\n\nApologies for that, I guess I didn't put in much effort in devising a \nway to split the commits and make them compile / run individually. I \nfound it hard the last time I tried to do that.\n\n>\n> I have one question about paramsBufferReady below - but I can't see\n> anything else standing out so with that resolved or otherwise:\n>\n> Reviewed-by: Kieran Bingham \n>\n>> Signed-off-by: Umang Jain \n>> Reviewed-by: Laurent Pinchart \n>> ---\n>> include/libcamera/ipa/ipu3.mojom | 36 ++-----\n>> src/ipa/ipu3/ipu3-ipa-design-guide.rst | 27 +++---\n>> src/ipa/ipu3/ipu3.cpp | 129 +++++++++++--------------\n>> src/libcamera/pipeline/ipu3/ipu3.cpp | 122 ++++++++++-------------\n>> 4 files changed, 130 insertions(+), 184 deletions(-)\n>>\n>> diff --git a/include/libcamera/ipa/ipu3.mojom b/include/libcamera/ipa/ipu3.mojom\n>> index 18cdc744..d1b1c6b8 100644\n>> --- a/include/libcamera/ipa/ipu3.mojom\n>> +++ b/include/libcamera/ipa/ipu3.mojom\n>> @@ -8,32 +8,6 @@ module ipa.ipu3;\n>> \n>> import \"include/libcamera/ipa/core.mojom\";\n>> \n>> -enum IPU3Operations {\n>> - ActionSetSensorControls = 1,\n>> - ActionParamFilled = 2,\n>> - ActionMetadataReady = 3,\n>> - EventProcessControls = 4,\n>> - EventStatReady = 5,\n>> - EventFillParams = 6,\n>> -};\n>> -\n>> -struct IPU3Event {\n>> - IPU3Operations op;\n>> - uint32 frame;\n>> - int64 frameTimestamp;\n>> - uint32 bufferId;\n>> - libcamera.ControlList controls;\n>> - libcamera.ControlList sensorControls;\n>> - libcamera.ControlList lensControls;\n>> -};\n>> -\n>> -struct IPU3Action {\n>> - IPU3Operations op;\n>> - libcamera.ControlList controls;\n>> - libcamera.ControlList sensorControls;\n>> - libcamera.ControlList lensControls;\n>> -};\n>> -\n>> struct IPAConfigInfo {\n>> libcamera.IPACameraSensorInfo sensorInfo;\n>> libcamera.ControlInfoMap sensorControls;\n>> @@ -56,9 +30,15 @@ interface IPAIPU3Interface {\n>> mapBuffers(array buffers);\n>> unmapBuffers(array ids);\n>> \n>> - [async] processEvent(IPU3Event ev);\n>> + [async] queueRequest(uint32 frame, libcamera.ControlList controls);\n>> + [async] fillParamsBuffer(uint32 frame, uint32 bufferId);\n>> + [async] processStatsBuffer(uint32 frame, int64 frameTimestamp,\n>> + uint32 bufferId, libcamera.ControlList sensorControls);\n>> };\n>> \n>> interface IPAIPU3EventInterface {\n>> - queueFrameAction(uint32 frame, IPU3Action action);\n>> + setSensorControls(uint32 frame, libcamera.ControlList sensorControls,\n>> + libcamera.ControlList lensControls);\n>> + paramsBufferReady(uint32 frame);\n>> + metadataReady(uint32 frame, libcamera.ControlList metadata);\n>> };\n>> diff --git a/src/ipa/ipu3/ipu3-ipa-design-guide.rst b/src/ipa/ipu3/ipu3-ipa-design-guide.rst\n>> index 89c71108..e724fdda 100644\n>> --- a/src/ipa/ipu3/ipu3-ipa-design-guide.rst\n>> +++ b/src/ipa/ipu3/ipu3-ipa-design-guide.rst\n>> @@ -25,7 +25,8 @@ from applications, and managing events from the pipeline handler.\n>> └─┬───┬───┬──────┬────┬────┬────┬─┴────▼─┬──┘ 1: init()\n>> │ │ │ │ ▲ │ ▲ │ ▲ │ ▲ │ 2: configure()\n>> │1 │2 │3 │4│ │4│ │4│ │4│ │5 3: mapBuffers(), start()\n>> - ▼ ▼ ▼ ▼ │ ▼ │ ▼ │ ▼ │ ▼ 4: processEvent()\n>> + │ │ │ │ │ │ │ │ │ │ │ │ 4: (▼) queueRequest(), fillParamsBuffer(), processStatsBuffer()\n>> + ▼ ▼ ▼ ▼ │ ▼ │ ▼ │ ▼ │ ▼ (▲) setSensorControls, paramsBufferReady, metadataReady Signals\n> I like that this is more descriptive about the direction and operations\n> that occur in 4.\n>\n>> ┌──────────────────┴────┴────┴────┴─────────┐ 5: stop(), unmapBuffers()\n>> │ IPU3 IPA │\n>> │ ┌───────────────────────┐ │\n>> @@ -100,8 +101,9 @@ There are three main interactions with the algorithms for the IPU3 IPA\n>> to operate when running:\n>> \n>> - configure()\n>> -- processEvent(``EventFillParams``)\n>> -- processEvent(``EventStatReady``)\n>> +- queueRequest()\n>> +- fillParamsBuffer()\n>> +- processStatsBuffer()\n>> \n>> The configuration phase allows the pipeline-handler to inform the IPA of\n>> the current stream configurations, which is then passed into each\n>> @@ -114,8 +116,8 @@ Pre-frame preparation\n>> When configured, the IPA is notified by the pipeline handler of the\n>> Camera ``start()`` event, after which incoming requests will be queued\n>> for processing, requiring a parameter buffer (``ipu3_uapi_params``) to\n>> -be populated for the ImgU. This is given to the IPA through the\n>> -``EventFillParams`` event, and then passed directly to each algorithm\n>> +be populated for the ImgU. This is given to the IPA through\n>> +``fillParamsBuffer()``, and then passed directly to each algorithm\n>> through the ``prepare()`` call allowing the ISP configuration to be\n>> updated for the needs of each component that the algorithm is\n>> responsible for.\n>> @@ -125,7 +127,7 @@ structure that it modifies, and it should take care to ensure that any\n>> structure set by a use flag is fully initialised to suitable values.\n>> \n>> The parameter buffer is returned to the pipeline handler through the\n>> -``ActionParamFilled`` event, and from there queued to the ImgU along\n>> +``paramsBufferReady`` signal, and from there queued to the ImgU along\n>> with a raw frame captured with the CIO2.\n>> \n>> Post-frame completion\n>> @@ -133,8 +135,8 @@ Post-frame completion\n>> \n>> When the capture of an image is completed, and successfully processed\n>> through the ImgU, the generated statistics buffer\n>> -(``ipu3_uapi_stats_3a``) is given to the IPA through the\n>> -``EventStatReady`` event. This provides the IPA with an opportunity to\n>> +(``ipu3_uapi_stats_3a``) is given to the IPA through\n>> +``processStatsBuffer()``. This provides the IPA with an opportunity to\n>> examine the results of the ISP and run the calculations required by each\n>> algorithm on the new data. The algorithms may require context from the\n>> operations of other algorithms, for example, the AWB might choose to use\n>> @@ -145,11 +147,14 @@ before they are needed.\n>> The ordering of the algorithm processing is determined by their\n>> placement in the ``IPU3::algorithms_`` ordered list.\n>> \n>> +Finally, the IPA metadata for the completed frame is returned back via\n>> +the ``metadataReady`` signal.\n>> +\n>> Sensor Controls\n>> ~~~~~~~~~~~~~~~\n>> \n>> The AutoExposure and AutoGain (AGC) algorithm differs slightly from the\n>> others as it requires operating directly on the sensor, as opposed to\n>> -through the ImgU ISP. To support this, there is a dedicated action\n>> -`ActionSetSensorControls` to allow the IPA to request controls to be set\n>> -on the camera sensor through the pipeline handler.\n>> +through the ImgU ISP. To support this, there is a ``setSensorControls``\n>> +signal to allow the IPA to request controls to be set on the camera\n>> +sensor through the pipeline handler.\n>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n>> index 50b52d8b..9a59f80f 100644\n>> --- a/src/ipa/ipu3/ipu3.cpp\n>> +++ b/src/ipa/ipu3/ipu3.cpp\n>> @@ -65,8 +65,9 @@ namespace ipa::ipu3 {\n>> * The IPU3 Pipeline defines an IPU3-specific interface for communication\n>> * between the PipelineHandler and the IPA module.\n>> *\n>> - * We extend the IPAIPU3Interface to implement our algorithms and handle events\n>> - * from the IPU3 PipelineHandler to satisfy requests from the application.\n>> + * We extend the IPAIPU3Interface to implement our algorithms and handle\n>> + * calls from the IPU3 PipelineHandler to satisfy requests from the\n>> + * application.\n>> *\n>> * At initialisation time, a CameraSensorHelper is instantiated to support\n>> * camera-specific calculations, while the default controls are computed, and\n>> @@ -81,14 +82,14 @@ namespace ipa::ipu3 {\n>> * parameter buffer, and adapting the settings of the sensor attached to the\n>> * IPU3 CIO2 through sensor-specific V4L2 controls.\n>> *\n>> - * When the event \\a EventFillParams occurs we populate the ImgU parameter\n>> - * buffer with settings to configure the device in preparation for handling the\n>> - * frame queued in the Request.\n>> + * In fillParamsBuffer(), we populate the ImgU parameter buffer with\n>> + * settings to configure the device in preparation for handling the frame\n>> + * queued in the Request.\n>> *\n>> * When the frame has completed processing, the ImgU will generate a statistics\n>> - * buffer which is given to the IPA as part of the \\a EventStatReady event. At\n>> - * this event we run the algorithms to parse the statistics and cache any\n>> - * results for the next \\a EventFillParams event.\n>> + * buffer which is given to the IPA with processStatsBuffer(). In this we run the\n>> + * algorithms to parse the statistics and cache any results for the next\n>> + * fillParamsBuffer() call.\n>> *\n>> * The individual algorithms are split into modular components that are called\n>> * iteratively to allow them to process statistics from the ImgU in a defined\n>> @@ -143,14 +144,18 @@ public:\n>> \n>> void mapBuffers(const std::vector &buffers) override;\n>> void unmapBuffers(const std::vector &ids) override;\n>> - void processEvent(const IPU3Event &event) override;\n>> \n>> + void queueRequest(const uint32_t frame, const ControlList &controls) override;\n>> + void fillParamsBuffer(const uint32_t frame, const uint32_t bufferId) override;\n>> + void processStatsBuffer(const uint32_t frame, const int64_t frameTimestamp,\n>> + const uint32_t bufferId,\n>> + const ControlList &sensorControls) override;\n>> private:\n>> void updateControls(const IPACameraSensorInfo &sensorInfo,\n>> const ControlInfoMap &sensorControls,\n>> ControlInfoMap *ipaControls);\n>> void updateSessionConfiguration(const ControlInfoMap &sensorControls);\n>> - void processControls(unsigned int frame, const ControlList &controls);\n>> +\n>> void fillParams(unsigned int frame, ipu3_uapi_params *params);\n>> void parseStatistics(unsigned int frame,\n>> int64_t frameTimestamp,\n>> @@ -505,73 +510,61 @@ void IPAIPU3::unmapBuffers(const std::vector &ids)\n>> }\n>> \n>> /**\n>> - * \\brief Process an event generated by the pipeline handler\n>> - * \\param[in] event The event sent from pipeline handler\n>> - *\n>> - * The expected event handling over the lifetime of a Request has\n>> - * the following sequence:\n>> - *\n>> - * - EventProcessControls : Handle controls from a new Request\n>> - * - EventFillParams : Prepare the ISP to process the Request\n>> - * - EventStatReady : Process statistics after ISP completion\n>> + * \\brief Fill and return a buffer with ISP processing parameters for a frame\n>> + * \\param[in] frame The frame number\n>> + * \\param[in] bufferId ID of the parameter buffer to fill\n>> */\n>> -void IPAIPU3::processEvent(const IPU3Event &event)\n>> +void IPAIPU3::fillParamsBuffer(const uint32_t frame, const uint32_t bufferId)\n>> {\n>> - switch (event.op) {\n>> - case EventProcessControls: {\n>> - processControls(event.frame, event.controls);\n>> - break;\n>> + auto it = buffers_.find(bufferId);\n>> + if (it == buffers_.end()) {\n>> + LOG(IPAIPU3, Error) << \"Could not find param buffer!\";\n>> + return;\n>> }\n>> - case EventFillParams: {\n>> - auto it = buffers_.find(event.bufferId);\n>> - if (it == buffers_.end()) {\n>> - LOG(IPAIPU3, Error) << \"Could not find param buffer!\";\n>> - return;\n>> - }\n>> \n>> - Span mem = it->second.planes()[0];\n>> - ipu3_uapi_params *params =\n>> - reinterpret_cast(mem.data());\n>> + Span mem = it->second.planes()[0];\n>> + ipu3_uapi_params *params =\n>> + reinterpret_cast(mem.data());\n>> \n>> - fillParams(event.frame, params);\n>> - break;\n>> - }\n>> - case EventStatReady: {\n>> - auto it = buffers_.find(event.bufferId);\n>> - if (it == buffers_.end()) {\n>> - LOG(IPAIPU3, Error) << \"Could not find stats buffer!\";\n>> - return;\n>> - }\n>> + fillParams(frame, params);\n>> +}\n>> \n>> - Span mem = it->second.planes()[0];\n>> - const ipu3_uapi_stats_3a *stats =\n>> - reinterpret_cast(mem.data());\n>> +/**\n>> + * \\brief Process statistics after ISP completion\n>> + * \\param[in] frame The frame number\n>> + * \\param[in] frameTimestamp Timestamp of the frame\n>> + * \\param[in] bufferId ID of the statistics buffer\n>> + * \\param[in] sensorControls Sensor controls\n>> + */\n>> +void IPAIPU3::processStatsBuffer(const uint32_t frame, const int64_t frameTimestamp,\n>> + const uint32_t bufferId, const ControlList &sensorControls)\n>> +{\n>> + auto it = buffers_.find(bufferId);\n>> + if (it == buffers_.end()) {\n>> + LOG(IPAIPU3, Error) << \"Could not find stats buffer!\";\n>> + return;\n>> + }\n>> \n>> - int32_t exposure = event.sensorControls.get(V4L2_CID_EXPOSURE).get();\n>> - int32_t gain = event.sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get();\n>> + Span mem = it->second.planes()[0];\n>> + const ipu3_uapi_stats_3a *stats =\n>> + reinterpret_cast(mem.data());\n>> \n>> - context_.frameContext.sensor.exposure = exposure;\n>> - context_.frameContext.sensor.gain = camHelper_->gain(gain);\n>> + context_.frameContext.sensor.exposure = sensorControls.get(V4L2_CID_EXPOSURE).get();\n>> + context_.frameContext.sensor.gain = camHelper_->gain(sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get());\n>> \n>> - parseStatistics(event.frame, event.frameTimestamp, stats);\n>> - break;\n>> - }\n>> - default:\n>> - LOG(IPAIPU3, Error) << \"Unknown event \" << event.op;\n>> - break;\n>> - }\n>> + parseStatistics(frame, frameTimestamp, stats);\n>> }\n>> \n>> /**\n>> - * \\brief Process a control list for a request from the application\n>> + * \\brief Queue a request and process the control list from the application\n>> * \\param[in] frame The number of the frame which will be processed next\n>> * \\param[in] controls The controls for the \\a frame\n>> *\n>> * Parse the request to handle any IPA-managed controls that were set from the\n>> * application such as manual sensor settings.\n>> */\n>> -void IPAIPU3::processControls([[maybe_unused]] unsigned int frame,\n>> - [[maybe_unused]] const ControlList &controls)\n>> +void IPAIPU3::queueRequest(const uint32_t frame,\n>> + [[maybe_unused]] const ControlList &controls)\n>> {\n>> /* \\todo Start processing for 'frame' based on 'controls'. */\n>> }\n>> @@ -600,10 +593,7 @@ void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params)\n>> for (auto const &algo : algorithms_)\n>> algo->prepare(context_, params);\n>> \n>> - IPU3Action op;\n>> - op.op = ActionParamFilled;\n>> -\n>> - queueFrameAction.emit(frame, op);\n>> + paramsBufferReady.emit(frame);\n>> }\n>> \n>> /**\n>> @@ -647,11 +637,7 @@ void IPAIPU3::parseStatistics(unsigned int frame,\n>> * likely want to avoid putting platform specific metadata in.\n>> */\n>> \n>> - IPU3Action op;\n>> - op.op = ActionMetadataReady;\n>> - op.controls = ctrls;\n>> -\n>> - queueFrameAction.emit(frame, op);\n>> + metadataReady.emit(frame, ctrls);\n>> }\n>> \n>> /**\n>> @@ -663,23 +649,18 @@ void IPAIPU3::parseStatistics(unsigned int frame,\n>> */\n>> void IPAIPU3::setControls(unsigned int frame)\n>> {\n>> - IPU3Action op;\n>> - op.op = ActionSetSensorControls;\n>> -\n>> int32_t exposure = context_.frameContext.agc.exposure;\n>> int32_t gain = camHelper_->gainCode(context_.frameContext.agc.gain);\n>> \n>> ControlList ctrls(sensorCtrls_);\n>> ctrls.set(V4L2_CID_EXPOSURE, exposure);\n>> ctrls.set(V4L2_CID_ANALOGUE_GAIN, gain);\n>> - op.sensorControls = ctrls;\n>> \n>> ControlList lensCtrls(lensCtrls_);\n>> lensCtrls.set(V4L2_CID_FOCUS_ABSOLUTE,\n>> static_cast(context_.frameContext.af.focus));\n>> - op.lensControls = lensCtrls;\n>> \n>> - queueFrameAction.emit(frame, op);\n>> + setSensorControls.emit(frame, ctrls, lensCtrls);\n>> }\n>> \n>> } /* namespace ipa::ipu3 */\n>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n>> index 60e01917..59d7e9c0 100644\n>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n>> @@ -86,8 +86,10 @@ public:\n>> ControlInfoMap ipaControls_;\n>> \n>> private:\n>> - void queueFrameAction(unsigned int id,\n>> - const ipa::ipu3::IPU3Action &action);\n>> + void metadataReady(unsigned int id, const ControlList &metadata);\n>> + void paramsFilled(unsigned int id);\n> This is confusing. ActionParamFilled becomes paramsBufferReady in the\n> commit message. Is this function used?\n>\n>> + void setSensorControls(unsigned int id, const ControlList &sensorControls,\n>> + const ControlList &lensControls);\n>> };\n>> \n>> class IPU3CameraConfiguration : public CameraConfiguration\n>> @@ -871,11 +873,7 @@ void IPU3CameraData::queuePendingRequests()\n>> \n>> info->rawBuffer = rawBuffer;\n>> \n>> - ipa::ipu3::IPU3Event ev;\n>> - ev.op = ipa::ipu3::EventProcessControls;\n>> - ev.frame = info->id;\n>> - ev.controls = request->controls();\n>> - ipa_->processEvent(ev);\n>> + ipa_->queueRequest(info->id, request->controls());\n>> \n>> pendingRequests_.pop();\n>> processingRequests_.push(request);\n>> @@ -1218,7 +1216,9 @@ int IPU3CameraData::loadIPA()\n>> if (!ipa_)\n>> return -ENOENT;\n>> \n>> - ipa_->queueFrameAction.connect(this, &IPU3CameraData::queueFrameAction);\n>> + ipa_->setSensorControls.connect(this, &IPU3CameraData::setSensorControls);\n>> + ipa_->paramsBufferReady.connect(this, &IPU3CameraData::paramsFilled);\n> Oh - I see - the signals are connected to functions of the same name\n> within the IPU3CameraData.\n\n\nYou can use different names, I don't think there is a strict requirement \nto use same name per-say\n\n>\n> Is there a reason why\n> queueFrameAction => queueFrameAction,\n> setSensorControls => setSensorControls,\n> metadataReady => metadataReady,\n> but\n> paramsBufferReady => paramsFilled?\n\n\nOK - probably I should s/paramsFilled/paramsBufferReady\n\nI have now addressed this locally.\n\n\n>\n>\n>\n>> + ipa_->metadataReady.connect(this, &IPU3CameraData::metadataReady);\n>> \n>> /*\n>> * Pass the sensor info to the IPA to initialize controls.\n>> @@ -1253,69 +1253,58 @@ int IPU3CameraData::loadIPA()\n>> return 0;\n>> }\n>> \n>> -void IPU3CameraData::queueFrameAction(unsigned int id,\n>> - const ipa::ipu3::IPU3Action &action)\n>> +void IPU3CameraData::setSensorControls([[maybe_unused]] unsigned int id,\n>> + const ControlList &sensorControls,\n>> + const ControlList &lensControls)\n>> {\n>> - switch (action.op) {\n>> - case ipa::ipu3::ActionSetSensorControls: {\n>> - const ControlList &sensorControls = action.sensorControls;\n>> - delayedCtrls_->push(sensorControls);\n>> + delayedCtrls_->push(sensorControls);\n>> \n>> - CameraLens *focusLens = cio2_.sensor()->focusLens();\n>> - if (!focusLens)\n>> - break;\n>> -\n>> - const ControlList lensControls = action.lensControls;\n>> - if (!lensControls.contains(V4L2_CID_FOCUS_ABSOLUTE))\n>> - break;\n>> + CameraLens *focusLens = cio2_.sensor()->focusLens();\n>> + if (!focusLens)\n>> + return;\n>> \n>> - const ControlValue &focusValue =\n>> - lensControls.get(V4L2_CID_FOCUS_ABSOLUTE);\n>> + if (!lensControls.contains(V4L2_CID_FOCUS_ABSOLUTE))\n>> + return;\n>> \n>> - focusLens->setFocusPosition(focusValue.get());\n>> + const ControlValue &focusValue = lensControls.get(V4L2_CID_FOCUS_ABSOLUTE);\n>> \n>> - break;\n>> - }\n>> - case ipa::ipu3::ActionParamFilled: {\n>> - IPU3Frames::Info *info = frameInfos_.find(id);\n>> - if (!info)\n>> - break;\n>> -\n>> - /* Queue all buffers from the request aimed for the ImgU. */\n>> - for (auto it : info->request->buffers()) {\n>> - const Stream *stream = it.first;\n>> - FrameBuffer *outbuffer = it.second;\n>> + focusLens->setFocusPosition(focusValue.get());\n>> +}\n>> \n>> - if (stream == &outStream_)\n>> - imgu_->output_->queueBuffer(outbuffer);\n>> - else if (stream == &vfStream_)\n>> - imgu_->viewfinder_->queueBuffer(outbuffer);\n>> - }\n>> +void IPU3CameraData::paramsFilled(unsigned int id)\n>> +{\n>> + IPU3Frames::Info *info = frameInfos_.find(id);\n>> + if (!info)\n>> + return;\n>> \n>> - imgu_->param_->queueBuffer(info->paramBuffer);\n>> - imgu_->stat_->queueBuffer(info->statBuffer);\n>> - imgu_->input_->queueBuffer(info->rawBuffer);\n>> + /* Queue all buffers from the request aimed for the ImgU. */\n>> + for (auto it : info->request->buffers()) {\n>> + const Stream *stream = it.first;\n>> + FrameBuffer *outbuffer = it.second;\n>> \n>> - break;\n>> + if (stream == &outStream_)\n>> + imgu_->output_->queueBuffer(outbuffer);\n>> + else if (stream == &vfStream_)\n>> + imgu_->viewfinder_->queueBuffer(outbuffer);\n>> }\n>> - case ipa::ipu3::ActionMetadataReady: {\n>> - IPU3Frames::Info *info = frameInfos_.find(id);\n>> - if (!info)\n>> - break;\n>> \n>> - Request *request = info->request;\n>> - request->metadata().merge(action.controls);\n>> + imgu_->param_->queueBuffer(info->paramBuffer);\n>> + imgu_->stat_->queueBuffer(info->statBuffer);\n>> + imgu_->input_->queueBuffer(info->rawBuffer);\n>> +}\n>> \n>> - info->metadataProcessed = true;\n>> - if (frameInfos_.tryComplete(info))\n>> - pipe()->completeRequest(request);\n>> +void IPU3CameraData::metadataReady(unsigned int id, const ControlList &metadata)\n>> +{\n>> + IPU3Frames::Info *info = frameInfos_.find(id);\n>> + if (!info)\n>> + return;\n>> \n>> - break;\n>> - }\n>> - default:\n>> - LOG(IPU3, Error) << \"Unknown action \" << action.op;\n>> - break;\n>> - }\n>> + Request *request = info->request;\n>> + request->metadata().merge(metadata);\n>> +\n>> + info->metadataProcessed = true;\n>> + if (frameInfos_.tryComplete(info))\n>> + pipe()->completeRequest(request);\n>> }\n>> \n>> /* -----------------------------------------------------------------------------\n>> @@ -1390,11 +1379,7 @@ void IPU3CameraData::cio2BufferReady(FrameBuffer *buffer)\n>> if (request->findBuffer(&rawStream_))\n>> pipe()->completeBuffer(request, buffer);\n>> \n>> - ipa::ipu3::IPU3Event ev;\n>> - ev.op = ipa::ipu3::EventFillParams;\n>> - ev.frame = info->id;\n>> - ev.bufferId = info->paramBuffer->cookie();\n>> - ipa_->processEvent(ev);\n>> + ipa_->fillParamsBuffer(info->id, info->paramBuffer->cookie());\n>> }\n>> \n>> void IPU3CameraData::paramBufferReady(FrameBuffer *buffer)\n>> @@ -1438,13 +1423,8 @@ void IPU3CameraData::statBufferReady(FrameBuffer *buffer)\n>> return;\n>> }\n>> \n>> - ipa::ipu3::IPU3Event ev;\n>> - ev.op = ipa::ipu3::EventStatReady;\n>> - ev.frame = info->id;\n>> - ev.bufferId = info->statBuffer->cookie();\n>> - ev.frameTimestamp = request->metadata().get(controls::SensorTimestamp);\n>> - ev.sensorControls = info->effectiveSensorControls;\n>> - ipa_->processEvent(ev);\n>> + ipa_->processStatsBuffer(info->id, request->metadata().get(controls::SensorTimestamp),\n>> + info->statBuffer->cookie(), info->effectiveSensorControls);\n>> }\n>> \n>> /*\n>> -- \n>> 2.31.0\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 A3A37C0F1B\n\tfor ;\n\tFri, 8 Apr 2022 07:04:11 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4FDEC65642;\n\tFri, 8 Apr 2022 09:04:11 +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 9F74361FBA\n\tfor ;\n\tFri, 8 Apr 2022 09:04:09 +0200 (CEST)","from [192.168.1.110] (unknown [27.57.186.178])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 85C39499;\n\tFri, 8 Apr 2022 09:04:08 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1649401451;\n\tbh=T6TTLoZgQWBdLJgQs5Ce5aVy2SlpUWA1HUSiH5P7BAM=;\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:\n\tFrom;\n\tb=vfBtyNX6Ctawk8vgBtad515R5rgJUECP4ZVjMrkVrc65Cr/Tp4dUsxlJctIeA8jxi\n\tH+i6cLcWM7H16qe5Fr+CLPvTFZzf/tU9N0XOz+/R9KxllDOFBLvR6/NQ/DnwwajaeD\n\thtDmusoHFyhV6+V2SLmnwMscIife3QrkynkPLqSKKcZ2Ht/2Wag+Z7mS2EUfjP+uMB\n\tSXI2B+fkWVftr/ENtRbN1jy4ZU7U9Gsl33pwD5KniWGA+TomCTBRx5IuDWKDJ1Uk7N\n\tMFYeq/2+ZTyEg1PHF+S+4fY7BxPKqkFGtWMxk1F0hGOD+qKRyKQYzFcve2prghotHd\n\ti89owCdtcr4Zg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1649401449;\n\tbh=T6TTLoZgQWBdLJgQs5Ce5aVy2SlpUWA1HUSiH5P7BAM=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=XrCBnxCVv5K67aNk31Po2praEdTddDTwo/uI2sJpTMHdjq4dD5Q2lcaqPx+S+j08V\n\tURT/M2MJHVdfWZzGhMbdeYnBGOMd4Ej4NAiXc2DLccz3vmEjXrZt3NlGhDfQzQ06zD\n\tNNwdkMurlPG/Xori4uyQD42TQt+Q8YBkPsRyPZsQ="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"XrCBnxCV\"; dkim-atps=neutral","Message-ID":"","Date":"Fri, 8 Apr 2022 12:34:03 +0530","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101\n\tThunderbird/91.4.1","Content-Language":"en-US","To":"Kieran Bingham ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20220406141709.164794-1-umang.jain@ideasonboard.com>\n\t<20220406141709.164794-2-umang.jain@ideasonboard.com>\n\t<164936949446.22830.7307396101214349800@Monstersaurus>","In-Reply-To":"<164936949446.22830.7307396101214349800@Monstersaurus>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH v5 1/6] ipa: ipu3: Replace event-based\n\tops with dedicated functions","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":"Umang Jain via libcamera-devel ","Reply-To":"Umang Jain ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":22662,"web_url":"https://patchwork.libcamera.org/comment/22662/","msgid":"","date":"2022-04-08T07:06:52","subject":"Re: [libcamera-devel] [PATCH v5 1/6] ipa: ipu3: Replace event-based\n\tops with dedicated functions","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Paul,\n\nOn 4/8/22 12:25, paul.elder@ideasonboard.com wrote:\n> Hi Umang,\n>\n> On Wed, Apr 06, 2022 at 07:47:04PM +0530, Umang Jain via libcamera-devel wrote:\n>> The IPAIPU3 interface currently uses event-type based structures in\n>> order to communicate with the pipeline-handler (and vice-versa).\n>> Replace the event based structures with dedicated functions associated\n>> to each operation.\n>>\n>> The translated naming scheme of operations to dedicated functions:\n>> ActionSetSensorControls => setSensorControls\n>> ActionParamFilled => paramsBufferReady\n>> ActionMetadataReady => metadataReady\n>> EventProcessControls => queueRequest()\n> Do you need to mention a bit on why just this one seems symmetrical? I\n> know what's going on since you recently had a similar change, but for\n> others perhaps not?\n\n\nI can provide a sentence that it symmetrical to other IPA interfaces \nnow. But actually it was other way around that, other IPAs interface I \nchanged recently, were brought to symmetry with IPU3 - but they got \nmerged first!\n\nNo issue, I think a sentence regarding EventProcessControls => \nqueueRequest() and what it is supposed to do might be helpful. I'll see \nwhat is the best way to phrase it.\n\n>\n>> EventStatReady => processStatsBuffer()\n>> EventFillParams => fillParamsBuffer()\n>>\n>> Signed-off-by: Umang Jain \n>> Reviewed-by: Laurent Pinchart \n> Reviewed-by: Paul Elder \n>\n>> ---\n>> include/libcamera/ipa/ipu3.mojom | 36 ++-----\n>> src/ipa/ipu3/ipu3-ipa-design-guide.rst | 27 +++---\n>> src/ipa/ipu3/ipu3.cpp | 129 +++++++++++--------------\n>> src/libcamera/pipeline/ipu3/ipu3.cpp | 122 ++++++++++-------------\n>> 4 files changed, 130 insertions(+), 184 deletions(-)\n>>\n>> diff --git a/include/libcamera/ipa/ipu3.mojom b/include/libcamera/ipa/ipu3.mojom\n>> index 18cdc744..d1b1c6b8 100644\n>> --- a/include/libcamera/ipa/ipu3.mojom\n>> +++ b/include/libcamera/ipa/ipu3.mojom\n>> @@ -8,32 +8,6 @@ module ipa.ipu3;\n>> \n>> import \"include/libcamera/ipa/core.mojom\";\n>> \n>> -enum IPU3Operations {\n>> -\tActionSetSensorControls = 1,\n>> -\tActionParamFilled = 2,\n>> -\tActionMetadataReady = 3,\n>> -\tEventProcessControls = 4,\n>> -\tEventStatReady = 5,\n>> -\tEventFillParams = 6,\n>> -};\n>> -\n>> -struct IPU3Event {\n>> -\tIPU3Operations op;\n>> -\tuint32 frame;\n>> -\tint64 frameTimestamp;\n>> -\tuint32 bufferId;\n>> -\tlibcamera.ControlList controls;\n>> -\tlibcamera.ControlList sensorControls;\n>> -\tlibcamera.ControlList lensControls;\n>> -};\n>> -\n>> -struct IPU3Action {\n>> -\tIPU3Operations op;\n>> -\tlibcamera.ControlList controls;\n>> -\tlibcamera.ControlList sensorControls;\n>> -\tlibcamera.ControlList lensControls;\n>> -};\n>> -\n>> struct IPAConfigInfo {\n>> \tlibcamera.IPACameraSensorInfo sensorInfo;\n>> \tlibcamera.ControlInfoMap sensorControls;\n>> @@ -56,9 +30,15 @@ interface IPAIPU3Interface {\n>> \tmapBuffers(array buffers);\n>> \tunmapBuffers(array ids);\n>> \n>> -\t[async] processEvent(IPU3Event ev);\n>> +\t[async] queueRequest(uint32 frame, libcamera.ControlList controls);\n>> +\t[async] fillParamsBuffer(uint32 frame, uint32 bufferId);\n>> +\t[async] processStatsBuffer(uint32 frame, int64 frameTimestamp,\n>> +\t\t\t\t uint32 bufferId, libcamera.ControlList sensorControls);\n>> };\n>> \n>> interface IPAIPU3EventInterface {\n>> -\tqueueFrameAction(uint32 frame, IPU3Action action);\n>> +\tsetSensorControls(uint32 frame, libcamera.ControlList sensorControls,\n>> +\t\t\t libcamera.ControlList lensControls);\n>> +\tparamsBufferReady(uint32 frame);\n>> +\tmetadataReady(uint32 frame, libcamera.ControlList metadata);\n>> };\n>> diff --git a/src/ipa/ipu3/ipu3-ipa-design-guide.rst b/src/ipa/ipu3/ipu3-ipa-design-guide.rst\n>> index 89c71108..e724fdda 100644\n>> --- a/src/ipa/ipu3/ipu3-ipa-design-guide.rst\n>> +++ b/src/ipa/ipu3/ipu3-ipa-design-guide.rst\n>> @@ -25,7 +25,8 @@ from applications, and managing events from the pipeline handler.\n>> └─┬───┬───┬──────┬────┬────┬────┬─┴────▼─┬──┘ 1: init()\n>> │ │ │ │ ▲ │ ▲ │ ▲ │ ▲ │ 2: configure()\n>> │1 │2 │3 │4│ │4│ │4│ │4│ │5 3: mapBuffers(), start()\n>> - ▼ ▼ ▼ ▼ │ ▼ │ ▼ │ ▼ │ ▼ 4: processEvent()\n>> + │ │ │ │ │ │ │ │ │ │ │ │ 4: (▼) queueRequest(), fillParamsBuffer(), processStatsBuffer()\n>> + ▼ ▼ ▼ ▼ │ ▼ │ ▼ │ ▼ │ ▼ (▲) setSensorControls, paramsBufferReady, metadataReady Signals\n>> ┌──────────────────┴────┴────┴────┴─────────┐ 5: stop(), unmapBuffers()\n>> │ IPU3 IPA │\n>> │ ┌───────────────────────┐ │\n>> @@ -100,8 +101,9 @@ There are three main interactions with the algorithms for the IPU3 IPA\n>> to operate when running:\n>> \n>> - configure()\n>> -- processEvent(``EventFillParams``)\n>> -- processEvent(``EventStatReady``)\n>> +- queueRequest()\n>> +- fillParamsBuffer()\n>> +- processStatsBuffer()\n>> \n>> The configuration phase allows the pipeline-handler to inform the IPA of\n>> the current stream configurations, which is then passed into each\n>> @@ -114,8 +116,8 @@ Pre-frame preparation\n>> When configured, the IPA is notified by the pipeline handler of the\n>> Camera ``start()`` event, after which incoming requests will be queued\n>> for processing, requiring a parameter buffer (``ipu3_uapi_params``) to\n>> -be populated for the ImgU. This is given to the IPA through the\n>> -``EventFillParams`` event, and then passed directly to each algorithm\n>> +be populated for the ImgU. This is given to the IPA through\n>> +``fillParamsBuffer()``, and then passed directly to each algorithm\n>> through the ``prepare()`` call allowing the ISP configuration to be\n>> updated for the needs of each component that the algorithm is\n>> responsible for.\n>> @@ -125,7 +127,7 @@ structure that it modifies, and it should take care to ensure that any\n>> structure set by a use flag is fully initialised to suitable values.\n>> \n>> The parameter buffer is returned to the pipeline handler through the\n>> -``ActionParamFilled`` event, and from there queued to the ImgU along\n>> +``paramsBufferReady`` signal, and from there queued to the ImgU along\n>> with a raw frame captured with the CIO2.\n>> \n>> Post-frame completion\n>> @@ -133,8 +135,8 @@ Post-frame completion\n>> \n>> When the capture of an image is completed, and successfully processed\n>> through the ImgU, the generated statistics buffer\n>> -(``ipu3_uapi_stats_3a``) is given to the IPA through the\n>> -``EventStatReady`` event. This provides the IPA with an opportunity to\n>> +(``ipu3_uapi_stats_3a``) is given to the IPA through\n>> +``processStatsBuffer()``. This provides the IPA with an opportunity to\n>> examine the results of the ISP and run the calculations required by each\n>> algorithm on the new data. The algorithms may require context from the\n>> operations of other algorithms, for example, the AWB might choose to use\n>> @@ -145,11 +147,14 @@ before they are needed.\n>> The ordering of the algorithm processing is determined by their\n>> placement in the ``IPU3::algorithms_`` ordered list.\n>> \n>> +Finally, the IPA metadata for the completed frame is returned back via\n>> +the ``metadataReady`` signal.\n>> +\n>> Sensor Controls\n>> ~~~~~~~~~~~~~~~\n>> \n>> The AutoExposure and AutoGain (AGC) algorithm differs slightly from the\n>> others as it requires operating directly on the sensor, as opposed to\n>> -through the ImgU ISP. To support this, there is a dedicated action\n>> -`ActionSetSensorControls` to allow the IPA to request controls to be set\n>> -on the camera sensor through the pipeline handler.\n>> +through the ImgU ISP. To support this, there is a ``setSensorControls``\n>> +signal to allow the IPA to request controls to be set on the camera\n>> +sensor through the pipeline handler.\n>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n>> index 50b52d8b..9a59f80f 100644\n>> --- a/src/ipa/ipu3/ipu3.cpp\n>> +++ b/src/ipa/ipu3/ipu3.cpp\n>> @@ -65,8 +65,9 @@ namespace ipa::ipu3 {\n>> * The IPU3 Pipeline defines an IPU3-specific interface for communication\n>> * between the PipelineHandler and the IPA module.\n>> *\n>> - * We extend the IPAIPU3Interface to implement our algorithms and handle events\n>> - * from the IPU3 PipelineHandler to satisfy requests from the application.\n>> + * We extend the IPAIPU3Interface to implement our algorithms and handle\n>> + * calls from the IPU3 PipelineHandler to satisfy requests from the\n>> + * application.\n>> *\n>> * At initialisation time, a CameraSensorHelper is instantiated to support\n>> * camera-specific calculations, while the default controls are computed, and\n>> @@ -81,14 +82,14 @@ namespace ipa::ipu3 {\n>> * parameter buffer, and adapting the settings of the sensor attached to the\n>> * IPU3 CIO2 through sensor-specific V4L2 controls.\n>> *\n>> - * When the event \\a EventFillParams occurs we populate the ImgU parameter\n>> - * buffer with settings to configure the device in preparation for handling the\n>> - * frame queued in the Request.\n>> + * In fillParamsBuffer(), we populate the ImgU parameter buffer with\n>> + * settings to configure the device in preparation for handling the frame\n>> + * queued in the Request.\n>> *\n>> * When the frame has completed processing, the ImgU will generate a statistics\n>> - * buffer which is given to the IPA as part of the \\a EventStatReady event. At\n>> - * this event we run the algorithms to parse the statistics and cache any\n>> - * results for the next \\a EventFillParams event.\n>> + * buffer which is given to the IPA with processStatsBuffer(). In this we run the\n>> + * algorithms to parse the statistics and cache any results for the next\n>> + * fillParamsBuffer() call.\n>> *\n>> * The individual algorithms are split into modular components that are called\n>> * iteratively to allow them to process statistics from the ImgU in a defined\n>> @@ -143,14 +144,18 @@ public:\n>> \n>> \tvoid mapBuffers(const std::vector &buffers) override;\n>> \tvoid unmapBuffers(const std::vector &ids) override;\n>> -\tvoid processEvent(const IPU3Event &event) override;\n>> \n>> +\tvoid queueRequest(const uint32_t frame, const ControlList &controls) override;\n>> +\tvoid fillParamsBuffer(const uint32_t frame, const uint32_t bufferId) override;\n>> +\tvoid processStatsBuffer(const uint32_t frame, const int64_t frameTimestamp,\n>> +\t\t\t\tconst uint32_t bufferId,\n>> +\t\t\t\tconst ControlList &sensorControls) override;\n>> private:\n>> \tvoid updateControls(const IPACameraSensorInfo &sensorInfo,\n>> \t\t\t const ControlInfoMap &sensorControls,\n>> \t\t\t ControlInfoMap *ipaControls);\n>> \tvoid updateSessionConfiguration(const ControlInfoMap &sensorControls);\n>> -\tvoid processControls(unsigned int frame, const ControlList &controls);\n>> +\n>> \tvoid fillParams(unsigned int frame, ipu3_uapi_params *params);\n>> \tvoid parseStatistics(unsigned int frame,\n>> \t\t\t int64_t frameTimestamp,\n>> @@ -505,73 +510,61 @@ void IPAIPU3::unmapBuffers(const std::vector &ids)\n>> }\n>> \n>> /**\n>> - * \\brief Process an event generated by the pipeline handler\n>> - * \\param[in] event The event sent from pipeline handler\n>> - *\n>> - * The expected event handling over the lifetime of a Request has\n>> - * the following sequence:\n>> - *\n>> - * - EventProcessControls : Handle controls from a new Request\n>> - * - EventFillParams : Prepare the ISP to process the Request\n>> - * - EventStatReady : Process statistics after ISP completion\n>> + * \\brief Fill and return a buffer with ISP processing parameters for a frame\n>> + * \\param[in] frame The frame number\n>> + * \\param[in] bufferId ID of the parameter buffer to fill\n>> */\n>> -void IPAIPU3::processEvent(const IPU3Event &event)\n>> +void IPAIPU3::fillParamsBuffer(const uint32_t frame, const uint32_t bufferId)\n>> {\n>> -\tswitch (event.op) {\n>> -\tcase EventProcessControls: {\n>> -\t\tprocessControls(event.frame, event.controls);\n>> -\t\tbreak;\n>> +\tauto it = buffers_.find(bufferId);\n>> +\tif (it == buffers_.end()) {\n>> +\t\tLOG(IPAIPU3, Error) << \"Could not find param buffer!\";\n>> +\t\treturn;\n>> \t}\n>> -\tcase EventFillParams: {\n>> -\t\tauto it = buffers_.find(event.bufferId);\n>> -\t\tif (it == buffers_.end()) {\n>> -\t\t\tLOG(IPAIPU3, Error) << \"Could not find param buffer!\";\n>> -\t\t\treturn;\n>> -\t\t}\n>> \n>> -\t\tSpan mem = it->second.planes()[0];\n>> -\t\tipu3_uapi_params *params =\n>> -\t\t\treinterpret_cast(mem.data());\n>> +\tSpan mem = it->second.planes()[0];\n>> +\tipu3_uapi_params *params =\n>> +\t\treinterpret_cast(mem.data());\n>> \n>> -\t\tfillParams(event.frame, params);\n>> -\t\tbreak;\n>> -\t}\n>> -\tcase EventStatReady: {\n>> -\t\tauto it = buffers_.find(event.bufferId);\n>> -\t\tif (it == buffers_.end()) {\n>> -\t\t\tLOG(IPAIPU3, Error) << \"Could not find stats buffer!\";\n>> -\t\t\treturn;\n>> -\t\t}\n>> +\tfillParams(frame, params);\n>> +}\n>> \n>> -\t\tSpan mem = it->second.planes()[0];\n>> -\t\tconst ipu3_uapi_stats_3a *stats =\n>> -\t\t\treinterpret_cast(mem.data());\n>> +/**\n>> + * \\brief Process statistics after ISP completion\n>> + * \\param[in] frame The frame number\n>> + * \\param[in] frameTimestamp Timestamp of the frame\n>> + * \\param[in] bufferId ID of the statistics buffer\n>> + * \\param[in] sensorControls Sensor controls\n>> + */\n>> +void IPAIPU3::processStatsBuffer(const uint32_t frame, const int64_t frameTimestamp,\n>> +\t\t\t\t const uint32_t bufferId, const ControlList &sensorControls)\n>> +{\n>> +\tauto it = buffers_.find(bufferId);\n>> +\tif (it == buffers_.end()) {\n>> +\t\tLOG(IPAIPU3, Error) << \"Could not find stats buffer!\";\n>> +\t\treturn;\n>> +\t}\n>> \n>> -\t\tint32_t exposure = event.sensorControls.get(V4L2_CID_EXPOSURE).get();\n>> -\t\tint32_t gain = event.sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get();\n>> +\tSpan mem = it->second.planes()[0];\n>> +\tconst ipu3_uapi_stats_3a *stats =\n>> +\t\treinterpret_cast(mem.data());\n>> \n>> -\t\tcontext_.frameContext.sensor.exposure = exposure;\n>> -\t\tcontext_.frameContext.sensor.gain = camHelper_->gain(gain);\n>> +\tcontext_.frameContext.sensor.exposure = sensorControls.get(V4L2_CID_EXPOSURE).get();\n>> +\tcontext_.frameContext.sensor.gain = camHelper_->gain(sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get());\n>> \n>> -\t\tparseStatistics(event.frame, event.frameTimestamp, stats);\n>> -\t\tbreak;\n>> -\t}\n>> -\tdefault:\n>> -\t\tLOG(IPAIPU3, Error) << \"Unknown event \" << event.op;\n>> -\t\tbreak;\n>> -\t}\n>> +\tparseStatistics(frame, frameTimestamp, stats);\n>> }\n>> \n>> /**\n>> - * \\brief Process a control list for a request from the application\n>> + * \\brief Queue a request and process the control list from the application\n>> * \\param[in] frame The number of the frame which will be processed next\n>> * \\param[in] controls The controls for the \\a frame\n>> *\n>> * Parse the request to handle any IPA-managed controls that were set from the\n>> * application such as manual sensor settings.\n>> */\n>> -void IPAIPU3::processControls([[maybe_unused]] unsigned int frame,\n>> -\t\t\t [[maybe_unused]] const ControlList &controls)\n>> +void IPAIPU3::queueRequest(const uint32_t frame,\n>> +\t\t\t [[maybe_unused]] const ControlList &controls)\n>> {\n>> \t/* \\todo Start processing for 'frame' based on 'controls'. */\n>> }\n>> @@ -600,10 +593,7 @@ void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params)\n>> \tfor (auto const &algo : algorithms_)\n>> \t\talgo->prepare(context_, params);\n>> \n>> -\tIPU3Action op;\n>> -\top.op = ActionParamFilled;\n>> -\n>> -\tqueueFrameAction.emit(frame, op);\n>> +\tparamsBufferReady.emit(frame);\n>> }\n>> \n>> /**\n>> @@ -647,11 +637,7 @@ void IPAIPU3::parseStatistics(unsigned int frame,\n>> \t * likely want to avoid putting platform specific metadata in.\n>> \t */\n>> \n>> -\tIPU3Action op;\n>> -\top.op = ActionMetadataReady;\n>> -\top.controls = ctrls;\n>> -\n>> -\tqueueFrameAction.emit(frame, op);\n>> +\tmetadataReady.emit(frame, ctrls);\n>> }\n>> \n>> /**\n>> @@ -663,23 +649,18 @@ void IPAIPU3::parseStatistics(unsigned int frame,\n>> */\n>> void IPAIPU3::setControls(unsigned int frame)\n>> {\n>> -\tIPU3Action op;\n>> -\top.op = ActionSetSensorControls;\n>> -\n>> \tint32_t exposure = context_.frameContext.agc.exposure;\n>> \tint32_t gain = camHelper_->gainCode(context_.frameContext.agc.gain);\n>> \n>> \tControlList ctrls(sensorCtrls_);\n>> \tctrls.set(V4L2_CID_EXPOSURE, exposure);\n>> \tctrls.set(V4L2_CID_ANALOGUE_GAIN, gain);\n>> -\top.sensorControls = ctrls;\n>> \n>> \tControlList lensCtrls(lensCtrls_);\n>> \tlensCtrls.set(V4L2_CID_FOCUS_ABSOLUTE,\n>> \t\t static_cast(context_.frameContext.af.focus));\n>> -\top.lensControls = lensCtrls;\n>> \n>> -\tqueueFrameAction.emit(frame, op);\n>> +\tsetSensorControls.emit(frame, ctrls, lensCtrls);\n>> }\n>> \n>> } /* namespace ipa::ipu3 */\n>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n>> index 60e01917..59d7e9c0 100644\n>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n>> @@ -86,8 +86,10 @@ public:\n>> \tControlInfoMap ipaControls_;\n>> \n>> private:\n>> -\tvoid queueFrameAction(unsigned int id,\n>> -\t\t\t const ipa::ipu3::IPU3Action &action);\n>> +\tvoid metadataReady(unsigned int id, const ControlList &metadata);\n>> +\tvoid paramsFilled(unsigned int id);\n>> +\tvoid setSensorControls(unsigned int id, const ControlList &sensorControls,\n>> +\t\t\t const ControlList &lensControls);\n>> };\n>> \n>> class IPU3CameraConfiguration : public CameraConfiguration\n>> @@ -871,11 +873,7 @@ void IPU3CameraData::queuePendingRequests()\n>> \n>> \t\tinfo->rawBuffer = rawBuffer;\n>> \n>> -\t\tipa::ipu3::IPU3Event ev;\n>> -\t\tev.op = ipa::ipu3::EventProcessControls;\n>> -\t\tev.frame = info->id;\n>> -\t\tev.controls = request->controls();\n>> -\t\tipa_->processEvent(ev);\n>> +\t\tipa_->queueRequest(info->id, request->controls());\n>> \n>> \t\tpendingRequests_.pop();\n>> \t\tprocessingRequests_.push(request);\n>> @@ -1218,7 +1216,9 @@ int IPU3CameraData::loadIPA()\n>> \tif (!ipa_)\n>> \t\treturn -ENOENT;\n>> \n>> -\tipa_->queueFrameAction.connect(this, &IPU3CameraData::queueFrameAction);\n>> +\tipa_->setSensorControls.connect(this, &IPU3CameraData::setSensorControls);\n>> +\tipa_->paramsBufferReady.connect(this, &IPU3CameraData::paramsFilled);\n>> +\tipa_->metadataReady.connect(this, &IPU3CameraData::metadataReady);\n>> \n>> \t/*\n>> \t * Pass the sensor info to the IPA to initialize controls.\n>> @@ -1253,69 +1253,58 @@ int IPU3CameraData::loadIPA()\n>> \treturn 0;\n>> }\n>> \n>> -void IPU3CameraData::queueFrameAction(unsigned int id,\n>> -\t\t\t\t const ipa::ipu3::IPU3Action &action)\n>> +void IPU3CameraData::setSensorControls([[maybe_unused]] unsigned int id,\n>> +\t\t\t\t const ControlList &sensorControls,\n>> +\t\t\t\t const ControlList &lensControls)\n>> {\n>> -\tswitch (action.op) {\n>> -\tcase ipa::ipu3::ActionSetSensorControls: {\n>> -\t\tconst ControlList &sensorControls = action.sensorControls;\n>> -\t\tdelayedCtrls_->push(sensorControls);\n>> +\tdelayedCtrls_->push(sensorControls);\n>> \n>> -\t\tCameraLens *focusLens = cio2_.sensor()->focusLens();\n>> -\t\tif (!focusLens)\n>> -\t\t\tbreak;\n>> -\n>> -\t\tconst ControlList lensControls = action.lensControls;\n>> -\t\tif (!lensControls.contains(V4L2_CID_FOCUS_ABSOLUTE))\n>> -\t\t\tbreak;\n>> +\tCameraLens *focusLens = cio2_.sensor()->focusLens();\n>> +\tif (!focusLens)\n>> +\t\treturn;\n>> \n>> -\t\tconst ControlValue &focusValue =\n>> -\t\t\tlensControls.get(V4L2_CID_FOCUS_ABSOLUTE);\n>> +\tif (!lensControls.contains(V4L2_CID_FOCUS_ABSOLUTE))\n>> +\t\treturn;\n>> \n>> -\t\tfocusLens->setFocusPosition(focusValue.get());\n>> +\tconst ControlValue &focusValue = lensControls.get(V4L2_CID_FOCUS_ABSOLUTE);\n>> \n>> -\t\tbreak;\n>> -\t}\n>> -\tcase ipa::ipu3::ActionParamFilled: {\n>> -\t\tIPU3Frames::Info *info = frameInfos_.find(id);\n>> -\t\tif (!info)\n>> -\t\t\tbreak;\n>> -\n>> -\t\t/* Queue all buffers from the request aimed for the ImgU. */\n>> -\t\tfor (auto it : info->request->buffers()) {\n>> -\t\t\tconst Stream *stream = it.first;\n>> -\t\t\tFrameBuffer *outbuffer = it.second;\n>> +\tfocusLens->setFocusPosition(focusValue.get());\n>> +}\n>> \n>> -\t\t\tif (stream == &outStream_)\n>> -\t\t\t\timgu_->output_->queueBuffer(outbuffer);\n>> -\t\t\telse if (stream == &vfStream_)\n>> -\t\t\t\timgu_->viewfinder_->queueBuffer(outbuffer);\n>> -\t\t}\n>> +void IPU3CameraData::paramsFilled(unsigned int id)\n>> +{\n>> +\tIPU3Frames::Info *info = frameInfos_.find(id);\n>> +\tif (!info)\n>> +\t\treturn;\n>> \n>> -\t\timgu_->param_->queueBuffer(info->paramBuffer);\n>> -\t\timgu_->stat_->queueBuffer(info->statBuffer);\n>> -\t\timgu_->input_->queueBuffer(info->rawBuffer);\n>> +\t/* Queue all buffers from the request aimed for the ImgU. */\n>> +\tfor (auto it : info->request->buffers()) {\n>> +\t\tconst Stream *stream = it.first;\n>> +\t\tFrameBuffer *outbuffer = it.second;\n>> \n>> -\t\tbreak;\n>> +\t\tif (stream == &outStream_)\n>> +\t\t\timgu_->output_->queueBuffer(outbuffer);\n>> +\t\telse if (stream == &vfStream_)\n>> +\t\t\timgu_->viewfinder_->queueBuffer(outbuffer);\n>> \t}\n>> -\tcase ipa::ipu3::ActionMetadataReady: {\n>> -\t\tIPU3Frames::Info *info = frameInfos_.find(id);\n>> -\t\tif (!info)\n>> -\t\t\tbreak;\n>> \n>> -\t\tRequest *request = info->request;\n>> -\t\trequest->metadata().merge(action.controls);\n>> +\timgu_->param_->queueBuffer(info->paramBuffer);\n>> +\timgu_->stat_->queueBuffer(info->statBuffer);\n>> +\timgu_->input_->queueBuffer(info->rawBuffer);\n>> +}\n>> \n>> -\t\tinfo->metadataProcessed = true;\n>> -\t\tif (frameInfos_.tryComplete(info))\n>> -\t\t\tpipe()->completeRequest(request);\n>> +void IPU3CameraData::metadataReady(unsigned int id, const ControlList &metadata)\n>> +{\n>> +\tIPU3Frames::Info *info = frameInfos_.find(id);\n>> +\tif (!info)\n>> +\t\treturn;\n>> \n>> -\t\tbreak;\n>> -\t}\n>> -\tdefault:\n>> -\t\tLOG(IPU3, Error) << \"Unknown action \" << action.op;\n>> -\t\tbreak;\n>> -\t}\n>> +\tRequest *request = info->request;\n>> +\trequest->metadata().merge(metadata);\n>> +\n>> +\tinfo->metadataProcessed = true;\n>> +\tif (frameInfos_.tryComplete(info))\n>> +\t\tpipe()->completeRequest(request);\n>> }\n>> \n>> /* -----------------------------------------------------------------------------\n>> @@ -1390,11 +1379,7 @@ void IPU3CameraData::cio2BufferReady(FrameBuffer *buffer)\n>> \tif (request->findBuffer(&rawStream_))\n>> \t\tpipe()->completeBuffer(request, buffer);\n>> \n>> -\tipa::ipu3::IPU3Event ev;\n>> -\tev.op = ipa::ipu3::EventFillParams;\n>> -\tev.frame = info->id;\n>> -\tev.bufferId = info->paramBuffer->cookie();\n>> -\tipa_->processEvent(ev);\n>> +\tipa_->fillParamsBuffer(info->id, info->paramBuffer->cookie());\n>> }\n>> \n>> void IPU3CameraData::paramBufferReady(FrameBuffer *buffer)\n>> @@ -1438,13 +1423,8 @@ void IPU3CameraData::statBufferReady(FrameBuffer *buffer)\n>> \t\treturn;\n>> \t}\n>> \n>> -\tipa::ipu3::IPU3Event ev;\n>> -\tev.op = ipa::ipu3::EventStatReady;\n>> -\tev.frame = info->id;\n>> -\tev.bufferId = info->statBuffer->cookie();\n>> -\tev.frameTimestamp = request->metadata().get(controls::SensorTimestamp);\n>> -\tev.sensorControls = info->effectiveSensorControls;\n>> -\tipa_->processEvent(ev);\n>> +\tipa_->processStatsBuffer(info->id, request->metadata().get(controls::SensorTimestamp),\n>> +\t\t\t\t info->statBuffer->cookie(), info->effectiveSensorControls);\n>> }\n>> \n>> /*\n>> -- \n>> 2.31.0\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 F0F24C3256\n\tfor ;\n\tFri, 8 Apr 2022 07:07:02 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1C1DB65642;\n\tFri, 8 Apr 2022 09:07:02 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9D22C61FBA\n\tfor ;\n\tFri, 8 Apr 2022 09:07:00 +0200 (CEST)","from [192.168.1.110] (unknown [27.57.186.178])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 6F478499;\n\tFri, 8 Apr 2022 09:06:59 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1649401622;\n\tbh=RE8PrrsBCMVqpgnalecoUpM0lwUwtxvBRb7NamjcjUo=;\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=N1cesKWQc82r2VipEWgRD3op5viixIF3Ag8vYvAh3phBpLFMp34V/+6PY8QDEJpZD\n\tzY1y/cVWSYulvrzxDOuRwg0AZPVPDUXDOggsFndPDwia3TIXsHA357XbyryjXr4Fru\n\tcGBuZ2UxsKalAWCZJDnQaCgo1e4xWltyf8f/hfaDifkHyYJKbbl45yEM4jBW3H3VDB\n\tp+w0HaMJeZHIik8cOqImS2baHfAom4VgJBgGOtUNeXIWvSZn6g+eOMeh3sl0acCMlP\n\tVXfz1M+H0iHewDG+ETjQrl7kbViPppXlHRhzgii1s3mp+yIiSOTI4U2d9TaYTjGMAE\n\tJ8ExneEvMCzNg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1649401620;\n\tbh=RE8PrrsBCMVqpgnalecoUpM0lwUwtxvBRb7NamjcjUo=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=BGC6jIMYAE/PfqXG8TJ8VvHdDuJYt5piqzZaqNhAnj6/0pOJxREkQDQrFuFjkMh70\n\tpiAESLWGXoKzXDG+rK5Whb1WMqfhAjoPFqQcvCmPzC1jn7uoogjcgxAjUzod0r5W7H\n\tZwc6sFuhbtO1OFtgmP8+vQt5+/s1OQvuOD5ih/4o="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"BGC6jIMY\"; dkim-atps=neutral","Message-ID":"","Date":"Fri, 8 Apr 2022 12:36:52 +0530","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101\n\tThunderbird/91.4.1","Content-Language":"en-US","To":"paul.elder@ideasonboard.com","References":"<20220406141709.164794-1-umang.jain@ideasonboard.com>\n\t<20220406141709.164794-2-umang.jain@ideasonboard.com>\n\t<20220408065529.GL3237525@pyrite.rasen.tech>","In-Reply-To":"<20220408065529.GL3237525@pyrite.rasen.tech>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH v5 1/6] ipa: ipu3: Replace event-based\n\tops with dedicated functions","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":"Umang Jain via libcamera-devel ","Reply-To":"Umang Jain ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":22667,"web_url":"https://patchwork.libcamera.org/comment/22667/","msgid":"","date":"2022-04-08T07:27:08","subject":"Re: [libcamera-devel] [PATCH v5 1/6] ipa: ipu3: Replace event-based\n\tops with dedicated functions","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Him\n\nOn 4/8/22 12:36, Umang Jain via libcamera-devel wrote:\n> Hi Paul,\n>\n> On 4/8/22 12:25, paul.elder@ideasonboard.com wrote:\n>> Hi Umang,\n>>\n>> On Wed, Apr 06, 2022 at 07:47:04PM +0530, Umang Jain via \n>> libcamera-devel wrote:\n>>> The IPAIPU3 interface currently uses event-type based structures in\n>>> order to communicate with the pipeline-handler (and vice-versa).\n>>> Replace the event based structures with dedicated functions associated\n>>> to each operation.\n>>>\n>>> The translated naming scheme of operations to dedicated functions:\n>>>    ActionSetSensorControls => setSensorControls\n>>>    ActionParamFilled       => paramsBufferReady\n>>>    ActionMetadataReady     => metadataReady\n>>>    EventProcessControls    => queueRequest()\n>> Do you need to mention a bit on why just this one seems symmetrical? I\n>> know what's going on since you recently had a similar change, but for\n>> others perhaps not?\n>\n>\n> I can provide a sentence that it symmetrical to other IPA interfaces \n> now. But actually it was other way around that, other IPAs interface I \n> changed recently, were brought to symmetry with IPU3 - but they got \n> merged first!\n>\n> No issue, I think a sentence regarding EventProcessControls => \n> queueRequest() and what it is supposed to do might be helpful. I'll \n> see what is the best way to phrase it.\n\n\nI think I will go with this re-phrasing of the commit message:\n\n     ipa: ipu3: Replace event-based ops with dedicated functions\n\n     The IPAIPU3 interface currently uses event-type based structures in\n     order to communicate with the pipeline-handler (and vice-versa).\n     Replace the event based structures with dedicated functions associated\n     to each operation.\n\n     The translated naming scheme of actions to signals are:\n       ActionSetSensorControls => setSensorControls\n       ActionParamFilled       => paramsBufferReady\n       ActionMetadataReady     => metadataReady\n\n     The translated naming scheme of events to dedicated functions are:\n       EventProcessControls    => queueRequest()\n       EventStatReady          => processStatsBuffer()\n       EventFillParams         => fillParamsBuffer()\n\n     The dedicated functions are called from pipeline-handler to the IPA\n     using IPC. These functions run asynchronously and when completed,\n     the IPA emits the respective signals as stated above in the translated\n     naming scheme.\n\n     The EventProcessControls is translated to queueRequest() to bring\n     symmetry to the IPU3 interface with other IPA interfaces.\n\n>\n>>\n>>>    EventStatReady          => processStatsBuffer()\n>>>    EventFillParams         => fillParamsBuffer()\n>>>\n>>> Signed-off-by: Umang Jain \n>>> Reviewed-by: Laurent Pinchart \n>> Reviewed-by: Paul Elder \n>>\n>>> ---\n>>>   include/libcamera/ipa/ipu3.mojom       |  36 ++-----\n>>>   src/ipa/ipu3/ipu3-ipa-design-guide.rst |  27 +++---\n>>>   src/ipa/ipu3/ipu3.cpp                  | 129 \n>>> +++++++++++--------------\n>>>   src/libcamera/pipeline/ipu3/ipu3.cpp   | 122 ++++++++++-------------\n>>>   4 files changed, 130 insertions(+), 184 deletions(-)\n>>>\n>>> diff --git a/include/libcamera/ipa/ipu3.mojom \n>>> b/include/libcamera/ipa/ipu3.mojom\n>>> index 18cdc744..d1b1c6b8 100644\n>>> --- a/include/libcamera/ipa/ipu3.mojom\n>>> +++ b/include/libcamera/ipa/ipu3.mojom\n>>> @@ -8,32 +8,6 @@ module ipa.ipu3;\n>>>     import \"include/libcamera/ipa/core.mojom\";\n>>>   -enum IPU3Operations {\n>>> -    ActionSetSensorControls = 1,\n>>> -    ActionParamFilled = 2,\n>>> -    ActionMetadataReady = 3,\n>>> -    EventProcessControls = 4,\n>>> -    EventStatReady = 5,\n>>> -    EventFillParams = 6,\n>>> -};\n>>> -\n>>> -struct IPU3Event {\n>>> -    IPU3Operations op;\n>>> -    uint32 frame;\n>>> -    int64 frameTimestamp;\n>>> -    uint32 bufferId;\n>>> -    libcamera.ControlList controls;\n>>> -    libcamera.ControlList sensorControls;\n>>> -    libcamera.ControlList lensControls;\n>>> -};\n>>> -\n>>> -struct IPU3Action {\n>>> -    IPU3Operations op;\n>>> -    libcamera.ControlList controls;\n>>> -    libcamera.ControlList sensorControls;\n>>> -    libcamera.ControlList lensControls;\n>>> -};\n>>> -\n>>>   struct IPAConfigInfo {\n>>>       libcamera.IPACameraSensorInfo sensorInfo;\n>>>       libcamera.ControlInfoMap sensorControls;\n>>> @@ -56,9 +30,15 @@ interface IPAIPU3Interface {\n>>>       mapBuffers(array buffers);\n>>>       unmapBuffers(array ids);\n>>>   -    [async] processEvent(IPU3Event ev);\n>>> +    [async] queueRequest(uint32 frame, libcamera.ControlList \n>>> controls);\n>>> +    [async] fillParamsBuffer(uint32 frame, uint32 bufferId);\n>>> +    [async] processStatsBuffer(uint32 frame, int64 frameTimestamp,\n>>> +                   uint32 bufferId, libcamera.ControlList \n>>> sensorControls);\n>>>   };\n>>>     interface IPAIPU3EventInterface {\n>>> -    queueFrameAction(uint32 frame, IPU3Action action);\n>>> +    setSensorControls(uint32 frame, libcamera.ControlList \n>>> sensorControls,\n>>> +              libcamera.ControlList lensControls);\n>>> +    paramsBufferReady(uint32 frame);\n>>> +    metadataReady(uint32 frame, libcamera.ControlList metadata);\n>>>   };\n>>> diff --git a/src/ipa/ipu3/ipu3-ipa-design-guide.rst \n>>> b/src/ipa/ipu3/ipu3-ipa-design-guide.rst\n>>> index 89c71108..e724fdda 100644\n>>> --- a/src/ipa/ipu3/ipu3-ipa-design-guide.rst\n>>> +++ b/src/ipa/ipu3/ipu3-ipa-design-guide.rst\n>>> @@ -25,7 +25,8 @@ from applications, and managing events from the \n>>> pipeline handler.\n>>>         └─┬───┬───┬──────┬────┬────┬────┬─┴────▼─┬──┘    1: init()\n>>>           │   │   │      │ ▲  │ ▲  │ ▲  │ ▲      │       2: configure()\n>>>           │1  │2  │3     │4│  │4│  │4│  │4│      │5      3: \n>>> mapBuffers(), start()\n>>> -        ▼   ▼   ▼      ▼ │  ▼ │  ▼ │  ▼ │      ▼       4: \n>>> processEvent()\n>>> +        │   │   │      │ │  │ │  │ │  │ │      │       4: (▼) \n>>> queueRequest(), fillParamsBuffer(), processStatsBuffer()\n>>> +        ▼   ▼   ▼      ▼ │  ▼ │  ▼ │  ▼ │      ▼          (▲) \n>>> setSensorControls, paramsBufferReady, metadataReady Signals\n>>>         ┌──────────────────┴────┴────┴────┴─────────┐    5: stop(), \n>>> unmapBuffers()\n>>>         │ IPU3 IPA                                  │\n>>>         │                 ┌───────────────────────┐ │\n>>> @@ -100,8 +101,9 @@ There are three main interactions with the \n>>> algorithms for the IPU3 IPA\n>>>   to operate when running:\n>>>     -  configure()\n>>> --  processEvent(``EventFillParams``)\n>>> --  processEvent(``EventStatReady``)\n>>> +-  queueRequest()\n>>> +-  fillParamsBuffer()\n>>> +-  processStatsBuffer()\n>>>     The configuration phase allows the pipeline-handler to inform \n>>> the IPA of\n>>>   the current stream configurations, which is then passed into each\n>>> @@ -114,8 +116,8 @@ Pre-frame preparation\n>>>   When configured, the IPA is notified by the pipeline handler of the\n>>>   Camera ``start()`` event, after which incoming requests will be \n>>> queued\n>>>   for processing, requiring a parameter buffer \n>>> (``ipu3_uapi_params``) to\n>>> -be populated for the ImgU. This is given to the IPA through the\n>>> -``EventFillParams`` event, and then passed directly to each algorithm\n>>> +be populated for the ImgU. This is given to the IPA through\n>>> +``fillParamsBuffer()``, and then passed directly to each algorithm\n>>>   through the ``prepare()`` call allowing the ISP configuration to be\n>>>   updated for the needs of each component that the algorithm is\n>>>   responsible for.\n>>> @@ -125,7 +127,7 @@ structure that it modifies, and it should take \n>>> care to ensure that any\n>>>   structure set by a use flag is fully initialised to suitable values.\n>>>     The parameter buffer is returned to the pipeline handler through \n>>> the\n>>> -``ActionParamFilled`` event, and from there queued to the ImgU along\n>>> +``paramsBufferReady`` signal, and from there queued to the ImgU along\n>>>   with a raw frame captured with the CIO2.\n>>>     Post-frame completion\n>>> @@ -133,8 +135,8 @@ Post-frame completion\n>>>     When the capture of an image is completed, and successfully \n>>> processed\n>>>   through the ImgU, the generated statistics buffer\n>>> -(``ipu3_uapi_stats_3a``) is given to the IPA through the\n>>> -``EventStatReady`` event. This provides the IPA with an opportunity to\n>>> +(``ipu3_uapi_stats_3a``) is given to the IPA through\n>>> +``processStatsBuffer()``. This provides the IPA with an opportunity to\n>>>   examine the results of the ISP and run the calculations required \n>>> by each\n>>>   algorithm on the new data. The algorithms may require context from \n>>> the\n>>>   operations of other algorithms, for example, the AWB might choose \n>>> to use\n>>> @@ -145,11 +147,14 @@ before they are needed.\n>>>   The ordering of the algorithm processing is determined by their\n>>>   placement in the ``IPU3::algorithms_`` ordered list.\n>>>   +Finally, the IPA metadata for the completed frame is returned \n>>> back via\n>>> +the ``metadataReady`` signal.\n>>> +\n>>>   Sensor Controls\n>>>   ~~~~~~~~~~~~~~~\n>>>     The AutoExposure and AutoGain (AGC) algorithm differs slightly \n>>> from the\n>>>   others as it requires operating directly on the sensor, as opposed to\n>>> -through the ImgU ISP. To support this, there is a dedicated action\n>>> -`ActionSetSensorControls` to allow the IPA to request controls to \n>>> be set\n>>> -on the camera sensor through the pipeline handler.\n>>> +through the ImgU ISP. To support this, there is a \n>>> ``setSensorControls``\n>>> +signal to allow the IPA to request controls to be set on the camera\n>>> +sensor through the pipeline handler.\n>>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n>>> index 50b52d8b..9a59f80f 100644\n>>> --- a/src/ipa/ipu3/ipu3.cpp\n>>> +++ b/src/ipa/ipu3/ipu3.cpp\n>>> @@ -65,8 +65,9 @@ namespace ipa::ipu3 {\n>>>    * The IPU3 Pipeline defines an IPU3-specific interface for \n>>> communication\n>>>    * between the PipelineHandler and the IPA module.\n>>>    *\n>>> - * We extend the IPAIPU3Interface to implement our algorithms and \n>>> handle events\n>>> - * from the IPU3 PipelineHandler to satisfy requests from the \n>>> application.\n>>> + * We extend the IPAIPU3Interface to implement our algorithms and \n>>> handle\n>>> + * calls from the IPU3 PipelineHandler to satisfy requests from the\n>>> + * application.\n>>>    *\n>>>    * At initialisation time, a CameraSensorHelper is instantiated to \n>>> support\n>>>    * camera-specific calculations, while the default controls are \n>>> computed, and\n>>> @@ -81,14 +82,14 @@ namespace ipa::ipu3 {\n>>>    * parameter buffer, and adapting the settings of the sensor \n>>> attached to the\n>>>    * IPU3 CIO2 through sensor-specific V4L2 controls.\n>>>    *\n>>> - * When the event \\a EventFillParams occurs we populate the ImgU \n>>> parameter\n>>> - * buffer with settings to configure the device in preparation for \n>>> handling the\n>>> - * frame queued in the Request.\n>>> + * In fillParamsBuffer(), we populate the ImgU parameter buffer with\n>>> + * settings to configure the device in preparation for handling the \n>>> frame\n>>> + * queued in the Request.\n>>>    *\n>>>    * When the frame has completed processing, the ImgU will generate \n>>> a statistics\n>>> - * buffer which is given to the IPA as part of the \\a \n>>> EventStatReady event. At\n>>> - * this event we run the algorithms to parse the statistics and \n>>> cache any\n>>> - * results for the next \\a EventFillParams event.\n>>> + * buffer which is given to the IPA with processStatsBuffer(). In \n>>> this we run the\n>>> + * algorithms to parse the statistics and cache any results for the \n>>> next\n>>> + * fillParamsBuffer() call.\n>>>    *\n>>>    * The individual algorithms are split into modular components \n>>> that are called\n>>>    * iteratively to allow them to process statistics from the ImgU \n>>> in a defined\n>>> @@ -143,14 +144,18 @@ public:\n>>>         void mapBuffers(const std::vector &buffers) \n>>> override;\n>>>       void unmapBuffers(const std::vector &ids) override;\n>>> -    void processEvent(const IPU3Event &event) override;\n>>>   +    void queueRequest(const uint32_t frame, const ControlList \n>>> &controls) override;\n>>> +    void fillParamsBuffer(const uint32_t frame, const uint32_t \n>>> bufferId) override;\n>>> +    void processStatsBuffer(const uint32_t frame, const int64_t \n>>> frameTimestamp,\n>>> +                const uint32_t bufferId,\n>>> +                const ControlList &sensorControls) override;\n>>>   private:\n>>>       void updateControls(const IPACameraSensorInfo &sensorInfo,\n>>>                   const ControlInfoMap &sensorControls,\n>>>                   ControlInfoMap *ipaControls);\n>>>       void updateSessionConfiguration(const ControlInfoMap \n>>> &sensorControls);\n>>> -    void processControls(unsigned int frame, const ControlList \n>>> &controls);\n>>> +\n>>>       void fillParams(unsigned int frame, ipu3_uapi_params *params);\n>>>       void parseStatistics(unsigned int frame,\n>>>                    int64_t frameTimestamp,\n>>> @@ -505,73 +510,61 @@ void IPAIPU3::unmapBuffers(const \n>>> std::vector &ids)\n>>>   }\n>>>     /**\n>>> - * \\brief Process an event generated by the pipeline handler\n>>> - * \\param[in] event The event sent from pipeline handler\n>>> - *\n>>> - * The expected event handling over the lifetime of a Request has\n>>> - * the following sequence:\n>>> - *\n>>> - *   - EventProcessControls : Handle controls from a new Request\n>>> - *   - EventFillParams : Prepare the ISP to process the Request\n>>> - *   - EventStatReady : Process statistics after ISP completion\n>>> + * \\brief Fill and return a buffer with ISP processing parameters \n>>> for a frame\n>>> + * \\param[in] frame The frame number\n>>> + * \\param[in] bufferId ID of the parameter buffer to fill\n>>>    */\n>>> -void IPAIPU3::processEvent(const IPU3Event &event)\n>>> +void IPAIPU3::fillParamsBuffer(const uint32_t frame, const uint32_t \n>>> bufferId)\n>>>   {\n>>> -    switch (event.op) {\n>>> -    case EventProcessControls: {\n>>> -        processControls(event.frame, event.controls);\n>>> -        break;\n>>> +    auto it = buffers_.find(bufferId);\n>>> +    if (it == buffers_.end()) {\n>>> +        LOG(IPAIPU3, Error) << \"Could not find param buffer!\";\n>>> +        return;\n>>>       }\n>>> -    case EventFillParams: {\n>>> -        auto it = buffers_.find(event.bufferId);\n>>> -        if (it == buffers_.end()) {\n>>> -            LOG(IPAIPU3, Error) << \"Could not find param buffer!\";\n>>> -            return;\n>>> -        }\n>>>   -        Span mem = it->second.planes()[0];\n>>> -        ipu3_uapi_params *params =\n>>> -            reinterpret_cast(mem.data());\n>>> +    Span mem = it->second.planes()[0];\n>>> +    ipu3_uapi_params *params =\n>>> +        reinterpret_cast(mem.data());\n>>>   -        fillParams(event.frame, params);\n>>> -        break;\n>>> -    }\n>>> -    case EventStatReady: {\n>>> -        auto it = buffers_.find(event.bufferId);\n>>> -        if (it == buffers_.end()) {\n>>> -            LOG(IPAIPU3, Error) << \"Could not find stats buffer!\";\n>>> -            return;\n>>> -        }\n>>> +    fillParams(frame, params);\n>>> +}\n>>>   -        Span mem = it->second.planes()[0];\n>>> -        const ipu3_uapi_stats_3a *stats =\n>>> -            reinterpret_cast(mem.data());\n>>> +/**\n>>> + * \\brief Process statistics after ISP completion\n>>> + * \\param[in] frame The frame number\n>>> + * \\param[in] frameTimestamp Timestamp of the frame\n>>> + * \\param[in] bufferId ID of the statistics buffer\n>>> + * \\param[in] sensorControls Sensor controls\n>>> + */\n>>> +void IPAIPU3::processStatsBuffer(const uint32_t frame, const \n>>> int64_t frameTimestamp,\n>>> +                 const uint32_t bufferId, const ControlList \n>>> &sensorControls)\n>>> +{\n>>> +    auto it = buffers_.find(bufferId);\n>>> +    if (it == buffers_.end()) {\n>>> +        LOG(IPAIPU3, Error) << \"Could not find stats buffer!\";\n>>> +        return;\n>>> +    }\n>>>   -        int32_t exposure = \n>>> event.sensorControls.get(V4L2_CID_EXPOSURE).get();\n>>> -        int32_t gain = \n>>> event.sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get();\n>>> +    Span mem = it->second.planes()[0];\n>>> +    const ipu3_uapi_stats_3a *stats =\n>>> +        reinterpret_cast(mem.data());\n>>>   -        context_.frameContext.sensor.exposure = exposure;\n>>> -        context_.frameContext.sensor.gain = camHelper_->gain(gain);\n>>> +    context_.frameContext.sensor.exposure = \n>>> sensorControls.get(V4L2_CID_EXPOSURE).get();\n>>> +    context_.frameContext.sensor.gain = \n>>> camHelper_->gain(sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get());\n>>>   -        parseStatistics(event.frame, event.frameTimestamp, stats);\n>>> -        break;\n>>> -    }\n>>> -    default:\n>>> -        LOG(IPAIPU3, Error) << \"Unknown event \" << event.op;\n>>> -        break;\n>>> -    }\n>>> +    parseStatistics(frame, frameTimestamp, stats);\n>>>   }\n>>>     /**\n>>> - * \\brief Process a control list for a request from the application\n>>> + * \\brief Queue a request and process the control list from the \n>>> application\n>>>    * \\param[in] frame The number of the frame which will be \n>>> processed next\n>>>    * \\param[in] controls The controls for the \\a frame\n>>>    *\n>>>    * Parse the request to handle any IPA-managed controls that were \n>>> set from the\n>>>    * application such as manual sensor settings.\n>>>    */\n>>> -void IPAIPU3::processControls([[maybe_unused]] unsigned int frame,\n>>> -                  [[maybe_unused]] const ControlList &controls)\n>>> +void IPAIPU3::queueRequest(const uint32_t frame,\n>>> +               [[maybe_unused]] const ControlList &controls)\n>>>   {\n>>>       /* \\todo Start processing for 'frame' based on 'controls'. */\n>>>   }\n>>> @@ -600,10 +593,7 @@ void IPAIPU3::fillParams(unsigned int frame, \n>>> ipu3_uapi_params *params)\n>>>       for (auto const &algo : algorithms_)\n>>>           algo->prepare(context_, params);\n>>>   -    IPU3Action op;\n>>> -    op.op = ActionParamFilled;\n>>> -\n>>> -    queueFrameAction.emit(frame, op);\n>>> +    paramsBufferReady.emit(frame);\n>>>   }\n>>>     /**\n>>> @@ -647,11 +637,7 @@ void IPAIPU3::parseStatistics(unsigned int frame,\n>>>        * likely want to avoid putting platform specific metadata in.\n>>>        */\n>>>   -    IPU3Action op;\n>>> -    op.op = ActionMetadataReady;\n>>> -    op.controls = ctrls;\n>>> -\n>>> -    queueFrameAction.emit(frame, op);\n>>> +    metadataReady.emit(frame, ctrls);\n>>>   }\n>>>     /**\n>>> @@ -663,23 +649,18 @@ void IPAIPU3::parseStatistics(unsigned int frame,\n>>>    */\n>>>   void IPAIPU3::setControls(unsigned int frame)\n>>>   {\n>>> -    IPU3Action op;\n>>> -    op.op = ActionSetSensorControls;\n>>> -\n>>>       int32_t exposure = context_.frameContext.agc.exposure;\n>>>       int32_t gain = \n>>> camHelper_->gainCode(context_.frameContext.agc.gain);\n>>>         ControlList ctrls(sensorCtrls_);\n>>>       ctrls.set(V4L2_CID_EXPOSURE, exposure);\n>>>       ctrls.set(V4L2_CID_ANALOGUE_GAIN, gain);\n>>> -    op.sensorControls = ctrls;\n>>>         ControlList lensCtrls(lensCtrls_);\n>>>       lensCtrls.set(V4L2_CID_FOCUS_ABSOLUTE,\n>>> static_cast(context_.frameContext.af.focus));\n>>> -    op.lensControls = lensCtrls;\n>>>   -    queueFrameAction.emit(frame, op);\n>>> +    setSensorControls.emit(frame, ctrls, lensCtrls);\n>>>   }\n>>>     } /* namespace ipa::ipu3 */\n>>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp \n>>> b/src/libcamera/pipeline/ipu3/ipu3.cpp\n>>> index 60e01917..59d7e9c0 100644\n>>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n>>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n>>> @@ -86,8 +86,10 @@ public:\n>>>       ControlInfoMap ipaControls_;\n>>>     private:\n>>> -    void queueFrameAction(unsigned int id,\n>>> -                  const ipa::ipu3::IPU3Action &action);\n>>> +    void metadataReady(unsigned int id, const ControlList &metadata);\n>>> +    void paramsFilled(unsigned int id);\n>>> +    void setSensorControls(unsigned int id, const ControlList \n>>> &sensorControls,\n>>> +                   const ControlList &lensControls);\n>>>   };\n>>>     class IPU3CameraConfiguration : public CameraConfiguration\n>>> @@ -871,11 +873,7 @@ void IPU3CameraData::queuePendingRequests()\n>>>             info->rawBuffer = rawBuffer;\n>>>   -        ipa::ipu3::IPU3Event ev;\n>>> -        ev.op = ipa::ipu3::EventProcessControls;\n>>> -        ev.frame = info->id;\n>>> -        ev.controls = request->controls();\n>>> -        ipa_->processEvent(ev);\n>>> +        ipa_->queueRequest(info->id, request->controls());\n>>>             pendingRequests_.pop();\n>>>           processingRequests_.push(request);\n>>> @@ -1218,7 +1216,9 @@ int IPU3CameraData::loadIPA()\n>>>       if (!ipa_)\n>>>           return -ENOENT;\n>>>   -    ipa_->queueFrameAction.connect(this, \n>>> &IPU3CameraData::queueFrameAction);\n>>> +    ipa_->setSensorControls.connect(this, \n>>> &IPU3CameraData::setSensorControls);\n>>> +    ipa_->paramsBufferReady.connect(this, \n>>> &IPU3CameraData::paramsFilled);\n>>> +    ipa_->metadataReady.connect(this, &IPU3CameraData::metadataReady);\n>>>         /*\n>>>        * Pass the sensor info to the IPA to initialize controls.\n>>> @@ -1253,69 +1253,58 @@ int IPU3CameraData::loadIPA()\n>>>       return 0;\n>>>   }\n>>>   -void IPU3CameraData::queueFrameAction(unsigned int id,\n>>> -                      const ipa::ipu3::IPU3Action &action)\n>>> +void IPU3CameraData::setSensorControls([[maybe_unused]] unsigned \n>>> int id,\n>>> +                       const ControlList &sensorControls,\n>>> +                       const ControlList &lensControls)\n>>>   {\n>>> -    switch (action.op) {\n>>> -    case ipa::ipu3::ActionSetSensorControls: {\n>>> -        const ControlList &sensorControls = action.sensorControls;\n>>> -        delayedCtrls_->push(sensorControls);\n>>> +    delayedCtrls_->push(sensorControls);\n>>>   -        CameraLens *focusLens = cio2_.sensor()->focusLens();\n>>> -        if (!focusLens)\n>>> -            break;\n>>> -\n>>> -        const ControlList lensControls = action.lensControls;\n>>> -        if (!lensControls.contains(V4L2_CID_FOCUS_ABSOLUTE))\n>>> -            break;\n>>> +    CameraLens *focusLens = cio2_.sensor()->focusLens();\n>>> +    if (!focusLens)\n>>> +        return;\n>>>   -        const ControlValue &focusValue =\n>>> -            lensControls.get(V4L2_CID_FOCUS_ABSOLUTE);\n>>> +    if (!lensControls.contains(V4L2_CID_FOCUS_ABSOLUTE))\n>>> +        return;\n>>>   - focusLens->setFocusPosition(focusValue.get());\n>>> +    const ControlValue &focusValue = \n>>> lensControls.get(V4L2_CID_FOCUS_ABSOLUTE);\n>>>   -        break;\n>>> -    }\n>>> -    case ipa::ipu3::ActionParamFilled: {\n>>> -        IPU3Frames::Info *info = frameInfos_.find(id);\n>>> -        if (!info)\n>>> -            break;\n>>> -\n>>> -        /* Queue all buffers from the request aimed for the ImgU. */\n>>> -        for (auto it : info->request->buffers()) {\n>>> -            const Stream *stream = it.first;\n>>> -            FrameBuffer *outbuffer = it.second;\n>>> + focusLens->setFocusPosition(focusValue.get());\n>>> +}\n>>>   -            if (stream == &outStream_)\n>>> -                imgu_->output_->queueBuffer(outbuffer);\n>>> -            else if (stream == &vfStream_)\n>>> - imgu_->viewfinder_->queueBuffer(outbuffer);\n>>> -        }\n>>> +void IPU3CameraData::paramsFilled(unsigned int id)\n>>> +{\n>>> +    IPU3Frames::Info *info = frameInfos_.find(id);\n>>> +    if (!info)\n>>> +        return;\n>>>   - imgu_->param_->queueBuffer(info->paramBuffer);\n>>> -        imgu_->stat_->queueBuffer(info->statBuffer);\n>>> -        imgu_->input_->queueBuffer(info->rawBuffer);\n>>> +    /* Queue all buffers from the request aimed for the ImgU. */\n>>> +    for (auto it : info->request->buffers()) {\n>>> +        const Stream *stream = it.first;\n>>> +        FrameBuffer *outbuffer = it.second;\n>>>   -        break;\n>>> +        if (stream == &outStream_)\n>>> +            imgu_->output_->queueBuffer(outbuffer);\n>>> +        else if (stream == &vfStream_)\n>>> +            imgu_->viewfinder_->queueBuffer(outbuffer);\n>>>       }\n>>> -    case ipa::ipu3::ActionMetadataReady: {\n>>> -        IPU3Frames::Info *info = frameInfos_.find(id);\n>>> -        if (!info)\n>>> -            break;\n>>>   -        Request *request = info->request;\n>>> -        request->metadata().merge(action.controls);\n>>> +    imgu_->param_->queueBuffer(info->paramBuffer);\n>>> +    imgu_->stat_->queueBuffer(info->statBuffer);\n>>> +    imgu_->input_->queueBuffer(info->rawBuffer);\n>>> +}\n>>>   -        info->metadataProcessed = true;\n>>> -        if (frameInfos_.tryComplete(info))\n>>> -            pipe()->completeRequest(request);\n>>> +void IPU3CameraData::metadataReady(unsigned int id, const \n>>> ControlList &metadata)\n>>> +{\n>>> +    IPU3Frames::Info *info = frameInfos_.find(id);\n>>> +    if (!info)\n>>> +        return;\n>>>   -        break;\n>>> -    }\n>>> -    default:\n>>> -        LOG(IPU3, Error) << \"Unknown action \" << action.op;\n>>> -        break;\n>>> -    }\n>>> +    Request *request = info->request;\n>>> +    request->metadata().merge(metadata);\n>>> +\n>>> +    info->metadataProcessed = true;\n>>> +    if (frameInfos_.tryComplete(info))\n>>> +        pipe()->completeRequest(request);\n>>>   }\n>>>     /* \n>>> -----------------------------------------------------------------------------\n>>> @@ -1390,11 +1379,7 @@ void \n>>> IPU3CameraData::cio2BufferReady(FrameBuffer *buffer)\n>>>       if (request->findBuffer(&rawStream_))\n>>>           pipe()->completeBuffer(request, buffer);\n>>>   -    ipa::ipu3::IPU3Event ev;\n>>> -    ev.op = ipa::ipu3::EventFillParams;\n>>> -    ev.frame = info->id;\n>>> -    ev.bufferId = info->paramBuffer->cookie();\n>>> -    ipa_->processEvent(ev);\n>>> +    ipa_->fillParamsBuffer(info->id, info->paramBuffer->cookie());\n>>>   }\n>>>     void IPU3CameraData::paramBufferReady(FrameBuffer *buffer)\n>>> @@ -1438,13 +1423,8 @@ void \n>>> IPU3CameraData::statBufferReady(FrameBuffer *buffer)\n>>>           return;\n>>>       }\n>>>   -    ipa::ipu3::IPU3Event ev;\n>>> -    ev.op = ipa::ipu3::EventStatReady;\n>>> -    ev.frame = info->id;\n>>> -    ev.bufferId = info->statBuffer->cookie();\n>>> -    ev.frameTimestamp = \n>>> request->metadata().get(controls::SensorTimestamp);\n>>> -    ev.sensorControls = info->effectiveSensorControls;\n>>> -    ipa_->processEvent(ev);\n>>> +    ipa_->processStatsBuffer(info->id, \n>>> request->metadata().get(controls::SensorTimestamp),\n>>> +                 info->statBuffer->cookie(), \n>>> info->effectiveSensorControls);\n>>>   }\n>>>     /*\n>>> -- \n>>> 2.31.0\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 2F66CC3256\n\tfor ;\n\tFri, 8 Apr 2022 07:27:15 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 529EC65644;\n\tFri, 8 Apr 2022 09:27:14 +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 8B49A61FBA\n\tfor ;\n\tFri, 8 Apr 2022 09:27:13 +0200 (CEST)","from [192.168.1.110] (unknown [27.57.186.178])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 2C105499;\n\tFri, 8 Apr 2022 09:27:11 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1649402834;\n\tbh=HeFOVFUt/h75EBS9qD5nisuf91LWCxBumzwKvZrgDkk=;\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=0NThMPOgDk/DQ/Q2XSc9PRskHF+5Qt7oILLw0uIT8aLcUcoBtUqFfRXNv2gQ/gNCA\n\to9iuA1OYqT+qX6g8OFcm9xXoyKyluqCVdAkWU4VbuxAzsNTImU02FkLOSTYR5A2+Vb\n\tKE5bfEEN+gbLtDUdbqyLOoq2/KvBMNdxxGbVL2nW3hb4lhSbgk3BgZBrgLotPyiSlM\n\tj8f8Ywx4zQrS6gdf0JqtDitRG3wjBNlxTn8QSCQzWEg/0eUFYdmfaOH7lcwhAHdyGG\n\teSsKs82wgZB/OhHTwGP0HQoOV/BE9UAr/qHtpblTblypSDKnl6yZoscHOLoXXL/Sqs\n\tKPu5UvOq7NFrw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1649402833;\n\tbh=HeFOVFUt/h75EBS9qD5nisuf91LWCxBumzwKvZrgDkk=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=D1tJcqhHnKN19OPe4t7dAfz4xFVnzyClmPBGKU91lE9kiYa8eByQmEiM0r9mdqG7i\n\tlZ6AOvrps24I3T3O+i2skh8KSZL4tYzj80VqS0xxSmsgroQSNAWmBw3r67zYBeVUGq\n\tTnE8nXXQBtGJBIoXQkMlMnTEGIkkt7npR7/NVbSA="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"D1tJcqhH\"; dkim-atps=neutral","Message-ID":"","Date":"Fri, 8 Apr 2022 12:57:08 +0530","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101\n\tThunderbird/91.4.1","Content-Language":"en-US","To":"paul.elder@ideasonboard.com","References":"<20220406141709.164794-1-umang.jain@ideasonboard.com>\n\t<20220406141709.164794-2-umang.jain@ideasonboard.com>\n\t<20220408065529.GL3237525@pyrite.rasen.tech>\n\t","In-Reply-To":"","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH v5 1/6] ipa: ipu3: Replace event-based\n\tops with dedicated functions","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":"Umang Jain via libcamera-devel ","Reply-To":"Umang Jain ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":22669,"web_url":"https://patchwork.libcamera.org/comment/22669/","msgid":"<20220408073513.GS3237525@pyrite.rasen.tech>","date":"2022-04-08T07:35:13","subject":"Re: [libcamera-devel] [PATCH v5 1/6] ipa: ipu3: Replace event-based\n\tops with dedicated functions","submitter":{"id":97,"url":"https://patchwork.libcamera.org/api/people/97/","name":"Nicolas Dufresne via libcamera-devel","email":"libcamera-devel@lists.libcamera.org"},"content":"Hi Umang,\n\nOn Fri, Apr 08, 2022 at 12:57:08PM +0530, Umang Jain wrote:\n> Him\n> \n> On 4/8/22 12:36, Umang Jain via libcamera-devel wrote:\n> > Hi Paul,\n> > \n> > On 4/8/22 12:25, paul.elder@ideasonboard.com wrote:\n> > > Hi Umang,\n> > > \n> > > On Wed, Apr 06, 2022 at 07:47:04PM +0530, Umang Jain via\n> > > libcamera-devel wrote:\n> > > > The IPAIPU3 interface currently uses event-type based structures in\n> > > > order to communicate with the pipeline-handler (and vice-versa).\n> > > > Replace the event based structures with dedicated functions associated\n> > > > to each operation.\n> > > > \n> > > > The translated naming scheme of operations to dedicated functions:\n> > > >    ActionSetSensorControls => setSensorControls\n> > > >    ActionParamFilled       => paramsBufferReady\n> > > >    ActionMetadataReady     => metadataReady\n> > > >    EventProcessControls    => queueRequest()\n> > > Do you need to mention a bit on why just this one seems symmetrical? I\n> > > know what's going on since you recently had a similar change, but for\n> > > others perhaps not?\n> > \n> > \n> > I can provide a sentence that it symmetrical to other IPA interfaces\n> > now. But actually it was other way around that, other IPAs interface I\n> > changed recently, were brought to symmetry with IPU3 - but they got\n> > merged first!\n> > \n> > No issue, I think a sentence regarding EventProcessControls =>\n> > queueRequest() and what it is supposed to do might be helpful. I'll see\n> > what is the best way to phrase it.\n> \n> \n> I think I will go with this re-phrasing of the commit message:\n> \n>     ipa: ipu3: Replace event-based ops with dedicated functions\n> \n>     The IPAIPU3 interface currently uses event-type based structures in\n>     order to communicate with the pipeline-handler (and vice-versa).\n>     Replace the event based structures with dedicated functions associated\n>     to each operation.\n> \n>     The translated naming scheme of actions to signals are:\n>       ActionSetSensorControls => setSensorControls\n>       ActionParamFilled       => paramsBufferReady\n>       ActionMetadataReady     => metadataReady\n> \n>     The translated naming scheme of events to dedicated functions are:\n>       EventProcessControls    => queueRequest()\n>       EventStatReady          => processStatsBuffer()\n>       EventFillParams         => fillParamsBuffer()\n> \n>     The dedicated functions are called from pipeline-handler to the IPA\n>     using IPC. These functions run asynchronously and when completed,\n>     the IPA emits the respective signals as stated above in the translated\n>     naming scheme.\n> \n>     The EventProcessControls is translated to queueRequest() to bring\n>     symmetry to the IPU3 interface with other IPA interfaces.\n\nLooks good!\n\n\nPaul\n\n> \n> > \n> > > \n> > > >    EventStatReady          => processStatsBuffer()\n> > > >    EventFillParams         => fillParamsBuffer()\n> > > > \n> > > > Signed-off-by: Umang Jain \n> > > > Reviewed-by: Laurent Pinchart \n> > > Reviewed-by: Paul Elder \n> > > \n> > > > ---\n> > > >   include/libcamera/ipa/ipu3.mojom       |  36 ++-----\n> > > >   src/ipa/ipu3/ipu3-ipa-design-guide.rst |  27 +++---\n> > > >   src/ipa/ipu3/ipu3.cpp                  | 129\n> > > > +++++++++++--------------\n> > > >   src/libcamera/pipeline/ipu3/ipu3.cpp   | 122 ++++++++++-------------\n> > > >   4 files changed, 130 insertions(+), 184 deletions(-)\n> > > > \n> > > > diff --git a/include/libcamera/ipa/ipu3.mojom\n> > > > b/include/libcamera/ipa/ipu3.mojom\n> > > > index 18cdc744..d1b1c6b8 100644\n> > > > --- a/include/libcamera/ipa/ipu3.mojom\n> > > > +++ b/include/libcamera/ipa/ipu3.mojom\n> > > > @@ -8,32 +8,6 @@ module ipa.ipu3;\n> > > >     import \"include/libcamera/ipa/core.mojom\";\n> > > >   -enum IPU3Operations {\n> > > > -    ActionSetSensorControls = 1,\n> > > > -    ActionParamFilled = 2,\n> > > > -    ActionMetadataReady = 3,\n> > > > -    EventProcessControls = 4,\n> > > > -    EventStatReady = 5,\n> > > > -    EventFillParams = 6,\n> > > > -};\n> > > > -\n> > > > -struct IPU3Event {\n> > > > -    IPU3Operations op;\n> > > > -    uint32 frame;\n> > > > -    int64 frameTimestamp;\n> > > > -    uint32 bufferId;\n> > > > -    libcamera.ControlList controls;\n> > > > -    libcamera.ControlList sensorControls;\n> > > > -    libcamera.ControlList lensControls;\n> > > > -};\n> > > > -\n> > > > -struct IPU3Action {\n> > > > -    IPU3Operations op;\n> > > > -    libcamera.ControlList controls;\n> > > > -    libcamera.ControlList sensorControls;\n> > > > -    libcamera.ControlList lensControls;\n> > > > -};\n> > > > -\n> > > >   struct IPAConfigInfo {\n> > > >       libcamera.IPACameraSensorInfo sensorInfo;\n> > > >       libcamera.ControlInfoMap sensorControls;\n> > > > @@ -56,9 +30,15 @@ interface IPAIPU3Interface {\n> > > >       mapBuffers(array buffers);\n> > > >       unmapBuffers(array ids);\n> > > >   -    [async] processEvent(IPU3Event ev);\n> > > > +    [async] queueRequest(uint32 frame, libcamera.ControlList\n> > > > controls);\n> > > > +    [async] fillParamsBuffer(uint32 frame, uint32 bufferId);\n> > > > +    [async] processStatsBuffer(uint32 frame, int64 frameTimestamp,\n> > > > +                   uint32 bufferId, libcamera.ControlList\n> > > > sensorControls);\n> > > >   };\n> > > >     interface IPAIPU3EventInterface {\n> > > > -    queueFrameAction(uint32 frame, IPU3Action action);\n> > > > +    setSensorControls(uint32 frame, libcamera.ControlList\n> > > > sensorControls,\n> > > > +              libcamera.ControlList lensControls);\n> > > > +    paramsBufferReady(uint32 frame);\n> > > > +    metadataReady(uint32 frame, libcamera.ControlList metadata);\n> > > >   };\n> > > > diff --git a/src/ipa/ipu3/ipu3-ipa-design-guide.rst\n> > > > b/src/ipa/ipu3/ipu3-ipa-design-guide.rst\n> > > > index 89c71108..e724fdda 100644\n> > > > --- a/src/ipa/ipu3/ipu3-ipa-design-guide.rst\n> > > > +++ b/src/ipa/ipu3/ipu3-ipa-design-guide.rst\n> > > > @@ -25,7 +25,8 @@ from applications, and managing events from\n> > > > the pipeline handler.\n> > > >         └─┬───┬───┬──────┬────┬────┬────┬─┴────▼─┬──┘    1: init()\n> > > >           │   │   │      │ ▲  │ ▲  │ ▲  │ ▲      │       2: configure()\n> > > >           │1  │2  │3     │4│  │4│  │4│  │4│      │5      3:\n> > > > mapBuffers(), start()\n> > > > -        ▼   ▼   ▼      ▼ │  ▼ │  ▼ │  ▼ │      ▼       4:\n> > > > processEvent()\n> > > > +        │   │   │      │ │  │ │  │ │  │ │      │       4: (▼)\n> > > > queueRequest(), fillParamsBuffer(), processStatsBuffer()\n> > > > +        ▼   ▼   ▼      ▼ │  ▼ │  ▼ │  ▼ │      ▼          (▲)\n> > > > setSensorControls, paramsBufferReady, metadataReady Signals\n> > > >         ┌──────────────────┴────┴────┴────┴─────────┐    5:\n> > > > stop(), unmapBuffers()\n> > > >         │ IPU3 IPA                                  │\n> > > >         │                 ┌───────────────────────┐ │\n> > > > @@ -100,8 +101,9 @@ There are three main interactions with the\n> > > > algorithms for the IPU3 IPA\n> > > >   to operate when running:\n> > > >     -  configure()\n> > > > --  processEvent(``EventFillParams``)\n> > > > --  processEvent(``EventStatReady``)\n> > > > +-  queueRequest()\n> > > > +-  fillParamsBuffer()\n> > > > +-  processStatsBuffer()\n> > > >     The configuration phase allows the pipeline-handler to\n> > > > inform the IPA of\n> > > >   the current stream configurations, which is then passed into each\n> > > > @@ -114,8 +116,8 @@ Pre-frame preparation\n> > > >   When configured, the IPA is notified by the pipeline handler of the\n> > > >   Camera ``start()`` event, after which incoming requests will\n> > > > be queued\n> > > >   for processing, requiring a parameter buffer\n> > > > (``ipu3_uapi_params``) to\n> > > > -be populated for the ImgU. This is given to the IPA through the\n> > > > -``EventFillParams`` event, and then passed directly to each algorithm\n> > > > +be populated for the ImgU. This is given to the IPA through\n> > > > +``fillParamsBuffer()``, and then passed directly to each algorithm\n> > > >   through the ``prepare()`` call allowing the ISP configuration to be\n> > > >   updated for the needs of each component that the algorithm is\n> > > >   responsible for.\n> > > > @@ -125,7 +127,7 @@ structure that it modifies, and it should\n> > > > take care to ensure that any\n> > > >   structure set by a use flag is fully initialised to suitable values.\n> > > >     The parameter buffer is returned to the pipeline handler\n> > > > through the\n> > > > -``ActionParamFilled`` event, and from there queued to the ImgU along\n> > > > +``paramsBufferReady`` signal, and from there queued to the ImgU along\n> > > >   with a raw frame captured with the CIO2.\n> > > >     Post-frame completion\n> > > > @@ -133,8 +135,8 @@ Post-frame completion\n> > > >     When the capture of an image is completed, and successfully\n> > > > processed\n> > > >   through the ImgU, the generated statistics buffer\n> > > > -(``ipu3_uapi_stats_3a``) is given to the IPA through the\n> > > > -``EventStatReady`` event. This provides the IPA with an opportunity to\n> > > > +(``ipu3_uapi_stats_3a``) is given to the IPA through\n> > > > +``processStatsBuffer()``. This provides the IPA with an opportunity to\n> > > >   examine the results of the ISP and run the calculations\n> > > > required by each\n> > > >   algorithm on the new data. The algorithms may require context\n> > > > from the\n> > > >   operations of other algorithms, for example, the AWB might\n> > > > choose to use\n> > > > @@ -145,11 +147,14 @@ before they are needed.\n> > > >   The ordering of the algorithm processing is determined by their\n> > > >   placement in the ``IPU3::algorithms_`` ordered list.\n> > > >   +Finally, the IPA metadata for the completed frame is returned\n> > > > back via\n> > > > +the ``metadataReady`` signal.\n> > > > +\n> > > >   Sensor Controls\n> > > >   ~~~~~~~~~~~~~~~\n> > > >     The AutoExposure and AutoGain (AGC) algorithm differs\n> > > > slightly from the\n> > > >   others as it requires operating directly on the sensor, as opposed to\n> > > > -through the ImgU ISP. To support this, there is a dedicated action\n> > > > -`ActionSetSensorControls` to allow the IPA to request controls\n> > > > to be set\n> > > > -on the camera sensor through the pipeline handler.\n> > > > +through the ImgU ISP. To support this, there is a\n> > > > ``setSensorControls``\n> > > > +signal to allow the IPA to request controls to be set on the camera\n> > > > +sensor through the pipeline handler.\n> > > > diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> > > > index 50b52d8b..9a59f80f 100644\n> > > > --- a/src/ipa/ipu3/ipu3.cpp\n> > > > +++ b/src/ipa/ipu3/ipu3.cpp\n> > > > @@ -65,8 +65,9 @@ namespace ipa::ipu3 {\n> > > >    * The IPU3 Pipeline defines an IPU3-specific interface for\n> > > > communication\n> > > >    * between the PipelineHandler and the IPA module.\n> > > >    *\n> > > > - * We extend the IPAIPU3Interface to implement our algorithms\n> > > > and handle events\n> > > > - * from the IPU3 PipelineHandler to satisfy requests from the\n> > > > application.\n> > > > + * We extend the IPAIPU3Interface to implement our algorithms\n> > > > and handle\n> > > > + * calls from the IPU3 PipelineHandler to satisfy requests from the\n> > > > + * application.\n> > > >    *\n> > > >    * At initialisation time, a CameraSensorHelper is\n> > > > instantiated to support\n> > > >    * camera-specific calculations, while the default controls\n> > > > are computed, and\n> > > > @@ -81,14 +82,14 @@ namespace ipa::ipu3 {\n> > > >    * parameter buffer, and adapting the settings of the sensor\n> > > > attached to the\n> > > >    * IPU3 CIO2 through sensor-specific V4L2 controls.\n> > > >    *\n> > > > - * When the event \\a EventFillParams occurs we populate the\n> > > > ImgU parameter\n> > > > - * buffer with settings to configure the device in preparation\n> > > > for handling the\n> > > > - * frame queued in the Request.\n> > > > + * In fillParamsBuffer(), we populate the ImgU parameter buffer with\n> > > > + * settings to configure the device in preparation for handling\n> > > > the frame\n> > > > + * queued in the Request.\n> > > >    *\n> > > >    * When the frame has completed processing, the ImgU will\n> > > > generate a statistics\n> > > > - * buffer which is given to the IPA as part of the \\a\n> > > > EventStatReady event. At\n> > > > - * this event we run the algorithms to parse the statistics and\n> > > > cache any\n> > > > - * results for the next \\a EventFillParams event.\n> > > > + * buffer which is given to the IPA with processStatsBuffer().\n> > > > In this we run the\n> > > > + * algorithms to parse the statistics and cache any results for\n> > > > the next\n> > > > + * fillParamsBuffer() call.\n> > > >    *\n> > > >    * The individual algorithms are split into modular components\n> > > > that are called\n> > > >    * iteratively to allow them to process statistics from the\n> > > > ImgU in a defined\n> > > > @@ -143,14 +144,18 @@ public:\n> > > >         void mapBuffers(const std::vector &buffers)\n> > > > override;\n> > > >       void unmapBuffers(const std::vector &ids) override;\n> > > > -    void processEvent(const IPU3Event &event) override;\n> > > >   +    void queueRequest(const uint32_t frame, const ControlList\n> > > > &controls) override;\n> > > > +    void fillParamsBuffer(const uint32_t frame, const uint32_t\n> > > > bufferId) override;\n> > > > +    void processStatsBuffer(const uint32_t frame, const int64_t\n> > > > frameTimestamp,\n> > > > +                const uint32_t bufferId,\n> > > > +                const ControlList &sensorControls) override;\n> > > >   private:\n> > > >       void updateControls(const IPACameraSensorInfo &sensorInfo,\n> > > >                   const ControlInfoMap &sensorControls,\n> > > >                   ControlInfoMap *ipaControls);\n> > > >       void updateSessionConfiguration(const ControlInfoMap\n> > > > &sensorControls);\n> > > > -    void processControls(unsigned int frame, const ControlList\n> > > > &controls);\n> > > > +\n> > > >       void fillParams(unsigned int frame, ipu3_uapi_params *params);\n> > > >       void parseStatistics(unsigned int frame,\n> > > >                    int64_t frameTimestamp,\n> > > > @@ -505,73 +510,61 @@ void IPAIPU3::unmapBuffers(const\n> > > > std::vector &ids)\n> > > >   }\n> > > >     /**\n> > > > - * \\brief Process an event generated by the pipeline handler\n> > > > - * \\param[in] event The event sent from pipeline handler\n> > > > - *\n> > > > - * The expected event handling over the lifetime of a Request has\n> > > > - * the following sequence:\n> > > > - *\n> > > > - *   - EventProcessControls : Handle controls from a new Request\n> > > > - *   - EventFillParams : Prepare the ISP to process the Request\n> > > > - *   - EventStatReady : Process statistics after ISP completion\n> > > > + * \\brief Fill and return a buffer with ISP processing\n> > > > parameters for a frame\n> > > > + * \\param[in] frame The frame number\n> > > > + * \\param[in] bufferId ID of the parameter buffer to fill\n> > > >    */\n> > > > -void IPAIPU3::processEvent(const IPU3Event &event)\n> > > > +void IPAIPU3::fillParamsBuffer(const uint32_t frame, const\n> > > > uint32_t bufferId)\n> > > >   {\n> > > > -    switch (event.op) {\n> > > > -    case EventProcessControls: {\n> > > > -        processControls(event.frame, event.controls);\n> > > > -        break;\n> > > > +    auto it = buffers_.find(bufferId);\n> > > > +    if (it == buffers_.end()) {\n> > > > +        LOG(IPAIPU3, Error) << \"Could not find param buffer!\";\n> > > > +        return;\n> > > >       }\n> > > > -    case EventFillParams: {\n> > > > -        auto it = buffers_.find(event.bufferId);\n> > > > -        if (it == buffers_.end()) {\n> > > > -            LOG(IPAIPU3, Error) << \"Could not find param buffer!\";\n> > > > -            return;\n> > > > -        }\n> > > >   -        Span mem = it->second.planes()[0];\n> > > > -        ipu3_uapi_params *params =\n> > > > -            reinterpret_cast(mem.data());\n> > > > +    Span mem = it->second.planes()[0];\n> > > > +    ipu3_uapi_params *params =\n> > > > +        reinterpret_cast(mem.data());\n> > > >   -        fillParams(event.frame, params);\n> > > > -        break;\n> > > > -    }\n> > > > -    case EventStatReady: {\n> > > > -        auto it = buffers_.find(event.bufferId);\n> > > > -        if (it == buffers_.end()) {\n> > > > -            LOG(IPAIPU3, Error) << \"Could not find stats buffer!\";\n> > > > -            return;\n> > > > -        }\n> > > > +    fillParams(frame, params);\n> > > > +}\n> > > >   -        Span mem = it->second.planes()[0];\n> > > > -        const ipu3_uapi_stats_3a *stats =\n> > > > -            reinterpret_cast(mem.data());\n> > > > +/**\n> > > > + * \\brief Process statistics after ISP completion\n> > > > + * \\param[in] frame The frame number\n> > > > + * \\param[in] frameTimestamp Timestamp of the frame\n> > > > + * \\param[in] bufferId ID of the statistics buffer\n> > > > + * \\param[in] sensorControls Sensor controls\n> > > > + */\n> > > > +void IPAIPU3::processStatsBuffer(const uint32_t frame, const\n> > > > int64_t frameTimestamp,\n> > > > +                 const uint32_t bufferId, const ControlList\n> > > > &sensorControls)\n> > > > +{\n> > > > +    auto it = buffers_.find(bufferId);\n> > > > +    if (it == buffers_.end()) {\n> > > > +        LOG(IPAIPU3, Error) << \"Could not find stats buffer!\";\n> > > > +        return;\n> > > > +    }\n> > > >   -        int32_t exposure =\n> > > > event.sensorControls.get(V4L2_CID_EXPOSURE).get();\n> > > > -        int32_t gain =\n> > > > event.sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get();\n> > > > +    Span mem = it->second.planes()[0];\n> > > > +    const ipu3_uapi_stats_3a *stats =\n> > > > +        reinterpret_cast(mem.data());\n> > > >   -        context_.frameContext.sensor.exposure = exposure;\n> > > > -        context_.frameContext.sensor.gain = camHelper_->gain(gain);\n> > > > +    context_.frameContext.sensor.exposure =\n> > > > sensorControls.get(V4L2_CID_EXPOSURE).get();\n> > > > +    context_.frameContext.sensor.gain = camHelper_->gain(sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get());\n> > > >   -        parseStatistics(event.frame, event.frameTimestamp, stats);\n> > > > -        break;\n> > > > -    }\n> > > > -    default:\n> > > > -        LOG(IPAIPU3, Error) << \"Unknown event \" << event.op;\n> > > > -        break;\n> > > > -    }\n> > > > +    parseStatistics(frame, frameTimestamp, stats);\n> > > >   }\n> > > >     /**\n> > > > - * \\brief Process a control list for a request from the application\n> > > > + * \\brief Queue a request and process the control list from the\n> > > > application\n> > > >    * \\param[in] frame The number of the frame which will be\n> > > > processed next\n> > > >    * \\param[in] controls The controls for the \\a frame\n> > > >    *\n> > > >    * Parse the request to handle any IPA-managed controls that\n> > > > were set from the\n> > > >    * application such as manual sensor settings.\n> > > >    */\n> > > > -void IPAIPU3::processControls([[maybe_unused]] unsigned int frame,\n> > > > -                  [[maybe_unused]] const ControlList &controls)\n> > > > +void IPAIPU3::queueRequest(const uint32_t frame,\n> > > > +               [[maybe_unused]] const ControlList &controls)\n> > > >   {\n> > > >       /* \\todo Start processing for 'frame' based on 'controls'. */\n> > > >   }\n> > > > @@ -600,10 +593,7 @@ void IPAIPU3::fillParams(unsigned int\n> > > > frame, ipu3_uapi_params *params)\n> > > >       for (auto const &algo : algorithms_)\n> > > >           algo->prepare(context_, params);\n> > > >   -    IPU3Action op;\n> > > > -    op.op = ActionParamFilled;\n> > > > -\n> > > > -    queueFrameAction.emit(frame, op);\n> > > > +    paramsBufferReady.emit(frame);\n> > > >   }\n> > > >     /**\n> > > > @@ -647,11 +637,7 @@ void IPAIPU3::parseStatistics(unsigned int frame,\n> > > >        * likely want to avoid putting platform specific metadata in.\n> > > >        */\n> > > >   -    IPU3Action op;\n> > > > -    op.op = ActionMetadataReady;\n> > > > -    op.controls = ctrls;\n> > > > -\n> > > > -    queueFrameAction.emit(frame, op);\n> > > > +    metadataReady.emit(frame, ctrls);\n> > > >   }\n> > > >     /**\n> > > > @@ -663,23 +649,18 @@ void IPAIPU3::parseStatistics(unsigned int frame,\n> > > >    */\n> > > >   void IPAIPU3::setControls(unsigned int frame)\n> > > >   {\n> > > > -    IPU3Action op;\n> > > > -    op.op = ActionSetSensorControls;\n> > > > -\n> > > >       int32_t exposure = context_.frameContext.agc.exposure;\n> > > >       int32_t gain =\n> > > > camHelper_->gainCode(context_.frameContext.agc.gain);\n> > > >         ControlList ctrls(sensorCtrls_);\n> > > >       ctrls.set(V4L2_CID_EXPOSURE, exposure);\n> > > >       ctrls.set(V4L2_CID_ANALOGUE_GAIN, gain);\n> > > > -    op.sensorControls = ctrls;\n> > > >         ControlList lensCtrls(lensCtrls_);\n> > > >       lensCtrls.set(V4L2_CID_FOCUS_ABSOLUTE,\n> > > > static_cast(context_.frameContext.af.focus));\n> > > > -    op.lensControls = lensCtrls;\n> > > >   -    queueFrameAction.emit(frame, op);\n> > > > +    setSensorControls.emit(frame, ctrls, lensCtrls);\n> > > >   }\n> > > >     } /* namespace ipa::ipu3 */\n> > > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > > b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > > index 60e01917..59d7e9c0 100644\n> > > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > > @@ -86,8 +86,10 @@ public:\n> > > >       ControlInfoMap ipaControls_;\n> > > >     private:\n> > > > -    void queueFrameAction(unsigned int id,\n> > > > -                  const ipa::ipu3::IPU3Action &action);\n> > > > +    void metadataReady(unsigned int id, const ControlList &metadata);\n> > > > +    void paramsFilled(unsigned int id);\n> > > > +    void setSensorControls(unsigned int id, const ControlList\n> > > > &sensorControls,\n> > > > +                   const ControlList &lensControls);\n> > > >   };\n> > > >     class IPU3CameraConfiguration : public CameraConfiguration\n> > > > @@ -871,11 +873,7 @@ void IPU3CameraData::queuePendingRequests()\n> > > >             info->rawBuffer = rawBuffer;\n> > > >   -        ipa::ipu3::IPU3Event ev;\n> > > > -        ev.op = ipa::ipu3::EventProcessControls;\n> > > > -        ev.frame = info->id;\n> > > > -        ev.controls = request->controls();\n> > > > -        ipa_->processEvent(ev);\n> > > > +        ipa_->queueRequest(info->id, request->controls());\n> > > >             pendingRequests_.pop();\n> > > >           processingRequests_.push(request);\n> > > > @@ -1218,7 +1216,9 @@ int IPU3CameraData::loadIPA()\n> > > >       if (!ipa_)\n> > > >           return -ENOENT;\n> > > >   -    ipa_->queueFrameAction.connect(this,\n> > > > &IPU3CameraData::queueFrameAction);\n> > > > +    ipa_->setSensorControls.connect(this,\n> > > > &IPU3CameraData::setSensorControls);\n> > > > +    ipa_->paramsBufferReady.connect(this,\n> > > > &IPU3CameraData::paramsFilled);\n> > > > +    ipa_->metadataReady.connect(this, &IPU3CameraData::metadataReady);\n> > > >         /*\n> > > >        * Pass the sensor info to the IPA to initialize controls.\n> > > > @@ -1253,69 +1253,58 @@ int IPU3CameraData::loadIPA()\n> > > >       return 0;\n> > > >   }\n> > > >   -void IPU3CameraData::queueFrameAction(unsigned int id,\n> > > > -                      const ipa::ipu3::IPU3Action &action)\n> > > > +void IPU3CameraData::setSensorControls([[maybe_unused]]\n> > > > unsigned int id,\n> > > > +                       const ControlList &sensorControls,\n> > > > +                       const ControlList &lensControls)\n> > > >   {\n> > > > -    switch (action.op) {\n> > > > -    case ipa::ipu3::ActionSetSensorControls: {\n> > > > -        const ControlList &sensorControls = action.sensorControls;\n> > > > -        delayedCtrls_->push(sensorControls);\n> > > > +    delayedCtrls_->push(sensorControls);\n> > > >   -        CameraLens *focusLens = cio2_.sensor()->focusLens();\n> > > > -        if (!focusLens)\n> > > > -            break;\n> > > > -\n> > > > -        const ControlList lensControls = action.lensControls;\n> > > > -        if (!lensControls.contains(V4L2_CID_FOCUS_ABSOLUTE))\n> > > > -            break;\n> > > > +    CameraLens *focusLens = cio2_.sensor()->focusLens();\n> > > > +    if (!focusLens)\n> > > > +        return;\n> > > >   -        const ControlValue &focusValue =\n> > > > -            lensControls.get(V4L2_CID_FOCUS_ABSOLUTE);\n> > > > +    if (!lensControls.contains(V4L2_CID_FOCUS_ABSOLUTE))\n> > > > +        return;\n> > > >   - focusLens->setFocusPosition(focusValue.get());\n> > > > +    const ControlValue &focusValue =\n> > > > lensControls.get(V4L2_CID_FOCUS_ABSOLUTE);\n> > > >   -        break;\n> > > > -    }\n> > > > -    case ipa::ipu3::ActionParamFilled: {\n> > > > -        IPU3Frames::Info *info = frameInfos_.find(id);\n> > > > -        if (!info)\n> > > > -            break;\n> > > > -\n> > > > -        /* Queue all buffers from the request aimed for the ImgU. */\n> > > > -        for (auto it : info->request->buffers()) {\n> > > > -            const Stream *stream = it.first;\n> > > > -            FrameBuffer *outbuffer = it.second;\n> > > > + focusLens->setFocusPosition(focusValue.get());\n> > > > +}\n> > > >   -            if (stream == &outStream_)\n> > > > -                imgu_->output_->queueBuffer(outbuffer);\n> > > > -            else if (stream == &vfStream_)\n> > > > - imgu_->viewfinder_->queueBuffer(outbuffer);\n> > > > -        }\n> > > > +void IPU3CameraData::paramsFilled(unsigned int id)\n> > > > +{\n> > > > +    IPU3Frames::Info *info = frameInfos_.find(id);\n> > > > +    if (!info)\n> > > > +        return;\n> > > >   - imgu_->param_->queueBuffer(info->paramBuffer);\n> > > > -        imgu_->stat_->queueBuffer(info->statBuffer);\n> > > > -        imgu_->input_->queueBuffer(info->rawBuffer);\n> > > > +    /* Queue all buffers from the request aimed for the ImgU. */\n> > > > +    for (auto it : info->request->buffers()) {\n> > > > +        const Stream *stream = it.first;\n> > > > +        FrameBuffer *outbuffer = it.second;\n> > > >   -        break;\n> > > > +        if (stream == &outStream_)\n> > > > +            imgu_->output_->queueBuffer(outbuffer);\n> > > > +        else if (stream == &vfStream_)\n> > > > +            imgu_->viewfinder_->queueBuffer(outbuffer);\n> > > >       }\n> > > > -    case ipa::ipu3::ActionMetadataReady: {\n> > > > -        IPU3Frames::Info *info = frameInfos_.find(id);\n> > > > -        if (!info)\n> > > > -            break;\n> > > >   -        Request *request = info->request;\n> > > > -        request->metadata().merge(action.controls);\n> > > > +    imgu_->param_->queueBuffer(info->paramBuffer);\n> > > > +    imgu_->stat_->queueBuffer(info->statBuffer);\n> > > > +    imgu_->input_->queueBuffer(info->rawBuffer);\n> > > > +}\n> > > >   -        info->metadataProcessed = true;\n> > > > -        if (frameInfos_.tryComplete(info))\n> > > > -            pipe()->completeRequest(request);\n> > > > +void IPU3CameraData::metadataReady(unsigned int id, const\n> > > > ControlList &metadata)\n> > > > +{\n> > > > +    IPU3Frames::Info *info = frameInfos_.find(id);\n> > > > +    if (!info)\n> > > > +        return;\n> > > >   -        break;\n> > > > -    }\n> > > > -    default:\n> > > > -        LOG(IPU3, Error) << \"Unknown action \" << action.op;\n> > > > -        break;\n> > > > -    }\n> > > > +    Request *request = info->request;\n> > > > +    request->metadata().merge(metadata);\n> > > > +\n> > > > +    info->metadataProcessed = true;\n> > > > +    if (frameInfos_.tryComplete(info))\n> > > > +        pipe()->completeRequest(request);\n> > > >   }\n> > > >     /* -----------------------------------------------------------------------------\n> > > > @@ -1390,11 +1379,7 @@ void\n> > > > IPU3CameraData::cio2BufferReady(FrameBuffer *buffer)\n> > > >       if (request->findBuffer(&rawStream_))\n> > > >           pipe()->completeBuffer(request, buffer);\n> > > >   -    ipa::ipu3::IPU3Event ev;\n> > > > -    ev.op = ipa::ipu3::EventFillParams;\n> > > > -    ev.frame = info->id;\n> > > > -    ev.bufferId = info->paramBuffer->cookie();\n> > > > -    ipa_->processEvent(ev);\n> > > > +    ipa_->fillParamsBuffer(info->id, info->paramBuffer->cookie());\n> > > >   }\n> > > >     void IPU3CameraData::paramBufferReady(FrameBuffer *buffer)\n> > > > @@ -1438,13 +1423,8 @@ void\n> > > > IPU3CameraData::statBufferReady(FrameBuffer *buffer)\n> > > >           return;\n> > > >       }\n> > > >   -    ipa::ipu3::IPU3Event ev;\n> > > > -    ev.op = ipa::ipu3::EventStatReady;\n> > > > -    ev.frame = info->id;\n> > > > -    ev.bufferId = info->statBuffer->cookie();\n> > > > -    ev.frameTimestamp =\n> > > > request->metadata().get(controls::SensorTimestamp);\n> > > > -    ev.sensorControls = info->effectiveSensorControls;\n> > > > -    ipa_->processEvent(ev);\n> > > > +    ipa_->processStatsBuffer(info->id,\n> > > > request->metadata().get(controls::SensorTimestamp),\n> > > > +                 info->statBuffer->cookie(),\n> > > > info->effectiveSensorControls);\n> > > >   }\n> > > >     /*\n> > > > -- \n> > > > 2.31.0\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 56281C0F1B\n\tfor ;\n\tFri, 8 Apr 2022 07:35:22 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0DC2E65645;\n\tFri, 8 Apr 2022 09:35:22 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9DAC661FBA\n\tfor ;\n\tFri, 8 Apr 2022 09:35:20 +0200 (CEST)","from pyrite.rasen.tech (softbank036240056250.bbtec.net\n\t[36.240.56.250])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id EDC6B499;\n\tFri, 8 Apr 2022 09:35:18 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1649403322;\n\tbh=HWG5xeMUhB5dTpzjkeupa45keL8JMulz6wQkax3yKaw=;\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=rh4CqMy+/mX2seZl7UvZlaDj6jUIIbyFJHgycV2URpZWVGRHk4aWbf8fokWfl221r\n\ta48y3bUFDUOaBVxmjj0xNizV9sHdBMm/50b7TvWkKZKNaQQmQgDs9o+zcQwdOZNaEd\n\tZNaFH1rk98W7OS4Iof5j8nyilDhzwlxeqpYY4xX0DFhuQwH4wj2pHwq8zgIuQxlGbe\n\tTTEUOMq77TIvNW2GaP2vJv4X1/OoTq4+n2k12VFry2NIaU8/WaEAMRWkGkSqEbLpLW\n\tTLuoSgCcNiifh2/5CCLrwu1GCUaPddHGww3D+YLEIAfDX5QXnk0fWecJCZo0rLtScH\n\tnnFR93HBALSFQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1649403320;\n\tbh=HWG5xeMUhB5dTpzjkeupa45keL8JMulz6wQkax3yKaw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=jLfD5qrIU+4DLPhrtYWs6mMA34GdgYWUHmyZHi67qaZf9pSaUZs4I5yTuFFfr7LW6\n\t41UZkoo+jtyKZ8F0ctG6IbX9YgTf2yjwaSYPzQzo1TgYFw5wDcCeNh8lfuQErHSXQg\n\t22kD86mwWRtrWkqoCKnENI3MOg/963aOyH+ZPw9A="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"jLfD5qrI\"; dkim-atps=neutral","Date":"Fri, 8 Apr 2022 16:35:13 +0900","To":"Umang Jain ","Message-ID":"<20220408073513.GS3237525@pyrite.rasen.tech>","References":"<20220406141709.164794-1-umang.jain@ideasonboard.com>\n\t<20220406141709.164794-2-umang.jain@ideasonboard.com>\n\t<20220408065529.GL3237525@pyrite.rasen.tech>\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v5 1/6] ipa: ipu3: Replace event-based\n\tops with dedicated functions","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":"Paul Elder via libcamera-devel ","Reply-To":"paul.elder@ideasonboard.com","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]