[{"id":22497,"web_url":"https://patchwork.libcamera.org/comment/22497/","msgid":"","date":"2022-03-28T22:25:31","subject":"Re: [libcamera-devel] [PATCH v3 1/1] ipa: ipu3: Replace event-based\n\tops with dedicated functions","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Umang,\n\nThank you for the patch.\n\nOn Mon, Mar 28, 2022 at 11:06:11PM +0530, Umang Jain via libcamera-devel wrote:\n> From: Umang Jain via libcamera-devel \n\nThat from line looks suspicious.\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 operations to dedicated functions:\n> ActionSetSensorControls => setSensorControls\n> ActionParamFilled => paramsBufferReady\n> ActionMetadataReady => metadataReady\n> EventProcessControls => processControls()\n\nWhat would you think about renaming this one to queueRequest(), to match\nthe operation used by the rkisp1 IPA ?\n\n> EventStatReady => processStatsBuffer()\n> EventFillParams => processParamsBuffer()\n\nThis function doesn't process the buffer, but fills it. How about\nfillParamsBuffer() ?\n\n> Signed-off-by: Umang Jain \n> ---\n> include/libcamera/ipa/ipu3.mojom | 36 ++-----\n> src/ipa/ipu3/ipu3-ipa-design-guide.rst | 19 ++--\n> src/ipa/ipu3/ipu3.cpp | 132 +++++++++++--------------\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 123 ++++++++++-------------\n> 4 files changed, 129 insertions(+), 181 deletions(-)\n> \n> diff --git a/include/libcamera/ipa/ipu3.mojom b/include/libcamera/ipa/ipu3.mojom\n> index 18cdc744..0bd5d641 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] processControls(uint32 frame, libcamera.ControlList controls);\n> +\t[async] processParamsBuffer(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..ed4b5ab7 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: (▼) processControls(), processParamsBuffer(), 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> +- processParamsBuffer()\n> +- processControls()\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> @@ -115,7 +117,7 @@ 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\ns/ the$//\n\n(or add \"function\" or something similar after ``processParamsBuffer()``)\n\n> -``EventFillParams`` event, and then passed directly to each algorithm\n> +``processParamsBuffer()``, 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> @@ -134,7 +136,7 @@ Post-frame completion\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> +``processStatsBuffer()``. This provides the IPA with an opportunity to\n\nSame here/\n\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\ns/ action$//\n\n(you may want to drop \"dedicated\" too)\n\n> -`ActionSetSensorControls` to allow the IPA to request controls to be set\n> +``setSensorControls`` signal to allow the IPA to request controls to be set\n> on the camera sensor through the pipeline handler.\n> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> index 50b52d8b..2c99055d 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> + * invocations from the IPU3 PipelineHandler to satisfy requests from the\n\ns/invocations/calls/\n\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 \\a processParamsBuffer(), we populate the ImgU parameter buffer with\n\nNo need for \\a. It's used by doxygen to change the rendering of the next\nword, and we use it for function parameters only. Doxygen will detect\nfunction names thanks to the () and generate links.\n\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 \\a processStatsBuffer(). In this we run the\n\nSame, replace \"\\a\" with \"with\".\n\n> + * algorithms to parse the statistics and cache any results for the next\n> + * \\a processParamsBuffer() invocation.\n\ns/invocation/call/\n\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 processParamsBuffer(const uint32_t frame, const uint32_t bufferId) override;\n> +\tvoid processControls(const uint32_t frame, const ControlList &controls) override;\n> +\tvoid processStatsBuffer(const uint32_t frame, const int64_t frameTimestamp,\n> +\t\t\t\tconst uint32_t bufferId, const ControlList &sensorControls) override;\n\nLine break after bufferId ?\n\n> \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,61 +510,49 @@ 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 Prepare the ISP to process the Request\n\nMaybe we could improve this to be a bit more detailed ?\n\n * \\brief Fill and return a buffer with ISP processing parameters for a frame\n\n> + * \\param[in] frame The frame number\n> + * \\param[in] bufferId Buffer ID\n\nID of the parameter buffer to fill\n\n> */\n> -void IPAIPU3::processEvent(const IPU3Event &event)\n> +void IPAIPU3::processParamsBuffer(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\nHow about inlinking the fillParams() function here ? Now that there's no\nneed for a switch/case on the operation ID, there's little reason to\nsplit the operation in two functions. Same for the other operation\nhandlers.\n\nThis can be done on top of this patch, to keep it reviewable.\n\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 Buffer ID\n\nID of the statistics buffer\n\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> @@ -570,7 +563,7 @@ void IPAIPU3::processEvent(const IPU3Event &event)\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> +void IPAIPU3::processControls(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> @@ -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,19 @@ 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> +\tControlList lensCtrls(sensorCtrls_);\n> +\t/*\n> +\t * \\todo lensCtrls.set(V4L2_CID_FOCUS_ABSOLUTE, ));\n> +\t */\n\nDoesn't this cause a regression ?\n\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..75b070bf 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 paramFilled(unsigned int id);\n\ns/param/params/\n\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_->processControls(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::paramFilled);\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,59 @@ 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 =\n> +\t\tlensControls.get(V4L2_CID_FOCUS_ABSOLUTE);\n\nYou could make this a single line.\n\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::paramFilled(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 +1380,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_->processParamsBuffer(info->id, info->paramBuffer->cookie());\n> }\n> \n> void IPU3CameraData::paramBufferReady(FrameBuffer *buffer)\n> @@ -1438,13 +1424,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> /*","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 1A28EC3256\n\tfor ;\n\tMon, 28 Mar 2022 22:25:38 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5208065633;\n\tTue, 29 Mar 2022 00:25:37 +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 C9ACE600AB\n\tfor ;\n\tTue, 29 Mar 2022 00:25:34 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 1044F2F7;\n\tTue, 29 Mar 2022 00:25:33 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1648506337;\n\tbh=Alq+sGlQsoIfNp/jkKK/fFPeSWfISNCNciqVjh5W+cU=;\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=CwU3HhXZkcT3qeYvNKL1Bz1DKDYrhjz/vELQ9EaO4mKFTIv2MlVWH+R8Ut8qqs8Hg\n\tdFlYeoW4KDx7yfzWV1SnQ1re7F8a9lFSm3B177Dbs3me/ThgNqIE83J01QywqW1hQa\n\tEaRMOBYTLijqxclDedHOFO3Q2QwXpfpwRIzkKExKyYqx5V2vu4hZxQWyPttP+WPNVn\n\tRtupeSEHKnYMIrts0LZ1G/xVM1xepc5lis/IHS99U3FBNdUG5st11DUcVBJdk+PBjH\n\tx/1EBiFAoL0K4JLQOJvvXz4N1qsqkPOqOAis2qsnxyGEJzX0ShyS84gzbXU/gnrRkz\n\t3V1iEgBIkGcLA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1648506334;\n\tbh=Alq+sGlQsoIfNp/jkKK/fFPeSWfISNCNciqVjh5W+cU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=AO8Qm9bBExqltGoagnvFviuLSj89sMuN+TG6IUDRrRYl4kY49kulkEa8W2OOUFVJF\n\tzLTqKoyqwcYbGTL6K+RYZhe1x4eXmmicg25CPtOFH/mZD8tqHhrvXTondvZe73Wbg4\n\tn2vYdTHL33semwuqHzQao4lxWl2P4KvvII3CYNZQ="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"AO8Qm9bB\"; dkim-atps=neutral","Date":"Tue, 29 Mar 2022 01:25:31 +0300","To":"Umang Jain ","Message-ID":"","References":"<20220328173612.138332-1-umang.jain@ideasonboard.com>\n\t<20220328173612.138332-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":"<20220328173612.138332-2-umang.jain@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v3 1/1] 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":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":22515,"web_url":"https://patchwork.libcamera.org/comment/22515/","msgid":"<3e44b4b7-c122-af07-b33c-f075e580e946@ideasonboard.com>","date":"2022-03-29T19:25:29","subject":"Re: [libcamera-devel] [PATCH v3 1/1] 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 Laurent,\n\nOn 3/29/22 03:55, Laurent Pinchart wrote:\n> Hi Umang,\n>\n> Thank you for the patch.\n>\n> On Mon, Mar 28, 2022 at 11:06:11PM +0530, Umang Jain via libcamera-devel wrote:\n>> From: Umang Jain via libcamera-devel \n> That from line looks suspicious.\n\n\nYeah, I noticed only after sending in the patch :-/\n\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 operations to dedicated functions:\n>> ActionSetSensorControls => setSensorControls\n>> ActionParamFilled => paramsBufferReady\n>> ActionMetadataReady => metadataReady\n>> EventProcessControls => processControls()\n> What would you think about renaming this one to queueRequest(), to match\n> the operation used by the rkisp1 IPA ?\n\n\nI don't find the correct matching. rkisp1 queueRequest() is more of \nActionParamFilled rather than of processing Controls. It's basically \ndoing processing of incoming coming and filling of param buffer in one \nfunction.\n\nI probably we should probably separate the rkisp1 queueRequest() to \nmatch more of ipu3, rather the way around.\n\nThe IPAIPU3::processControls() seems more intuitive to me rather than \nIPAIPU3::queueRequest(...controls..). Since the queue-ing of Request \npart is done by the pipeline-handler and in process of doing so, it \nshall call ipa_->processControls(..controls..) to instruct the IPA to \naddress the incoming controls. Does it make sense?\n\n>\n>> EventStatReady => processStatsBuffer()\n>> EventFillParams => processParamsBuffer()\n> This function doesn't process the buffer, but fills it. How about\n> fillParamsBuffer() ?\n\n\nSounds good\n\n>\n>> Signed-off-by: Umang Jain \n>> ---\n>> include/libcamera/ipa/ipu3.mojom | 36 ++-----\n>> src/ipa/ipu3/ipu3-ipa-design-guide.rst | 19 ++--\n>> src/ipa/ipu3/ipu3.cpp | 132 +++++++++++--------------\n>> src/libcamera/pipeline/ipu3/ipu3.cpp | 123 ++++++++++-------------\n>> 4 files changed, 129 insertions(+), 181 deletions(-)\n>>\n>> diff --git a/include/libcamera/ipa/ipu3.mojom b/include/libcamera/ipa/ipu3.mojom\n>> index 18cdc744..0bd5d641 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] processControls(uint32 frame, libcamera.ControlList controls);\n>> +\t[async] processParamsBuffer(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..ed4b5ab7 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: (▼) processControls(), processParamsBuffer(), 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>> +- processParamsBuffer()\n>> +- processControls()\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>> @@ -115,7 +117,7 @@ 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> s/ the$//\n>\n> (or add \"function\" or something similar after ``processParamsBuffer()``)\n>\n>> -``EventFillParams`` event, and then passed directly to each algorithm\n>> +``processParamsBuffer()``, 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>> @@ -134,7 +136,7 @@ Post-frame completion\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>> +``processStatsBuffer()``. This provides the IPA with an opportunity to\n> Same here/\n>\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> s/ action$//\n>\n> (you may want to drop \"dedicated\" too)\n>\n>> -`ActionSetSensorControls` to allow the IPA to request controls to be set\n>> +``setSensorControls`` signal to allow the IPA to request controls to be set\n>> on the camera sensor through the pipeline handler.\n>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n>> index 50b52d8b..2c99055d 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>> + * invocations from the IPU3 PipelineHandler to satisfy requests from the\n> s/invocations/calls/\n>\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 \\a processParamsBuffer(), we populate the ImgU parameter buffer with\n> No need for \\a. It's used by doxygen to change the rendering of the next\n> word, and we use it for function parameters only. Doxygen will detect\n> function names thanks to the () and generate links.\n>\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 \\a processStatsBuffer(). In this we run the\n> Same, replace \"\\a\" with \"with\".\n>\n>> + * algorithms to parse the statistics and cache any results for the next\n>> + * \\a processParamsBuffer() invocation.\n> s/invocation/call/\n>\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 processParamsBuffer(const uint32_t frame, const uint32_t bufferId) override;\n>> +\tvoid processControls(const uint32_t frame, const ControlList &controls) override;\n>> +\tvoid processStatsBuffer(const uint32_t frame, const int64_t frameTimestamp,\n>> +\t\t\t\tconst uint32_t bufferId, const ControlList &sensorControls) override;\n> Line break after bufferId ?\n>\n>> \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,61 +510,49 @@ 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 Prepare the ISP to process the Request\n> Maybe we could improve this to be a bit more detailed ?\n>\n> * \\brief Fill and return a buffer with ISP processing parameters for a frame\n>\n>> + * \\param[in] frame The frame number\n>> + * \\param[in] bufferId Buffer ID\n> ID of the parameter buffer to fill\n>\n>> */\n>> -void IPAIPU3::processEvent(const IPU3Event &event)\n>> +void IPAIPU3::processParamsBuffer(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> How about inlinking the fillParams() function here ? Now that there's no\n> need for a switch/case on the operation ID, there's little reason to\n> split the operation in two functions. Same for the other operation\n> handlers.\n>\n> This can be done on top of this patch, to keep it reviewable.\n>\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 Buffer ID\n> ID of the statistics buffer\n>\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>> @@ -570,7 +563,7 @@ void IPAIPU3::processEvent(const IPU3Event &event)\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>> +void IPAIPU3::processControls(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>> @@ -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,19 @@ 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>> +\tControlList lensCtrls(sensorCtrls_);\n>> +\t/*\n>> +\t * \\todo lensCtrls.set(V4L2_CID_FOCUS_ABSOLUTE, ));\n>> +\t */\n> Doesn't this cause a regression ?\n\n\nops, yeah I think I ended up making a rebase fiasco. I'll address it.\n\n>\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..75b070bf 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 paramFilled(unsigned int id);\n> s/param/params/\n>\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_->processControls(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::paramFilled);\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,59 @@ 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 =\n>> +\t\tlensControls.get(V4L2_CID_FOCUS_ABSOLUTE);\n> You could make this a single line.\n>\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::paramFilled(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 +1380,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_->processParamsBuffer(info->id, info->paramBuffer->cookie());\n>> }\n>> \n>> void IPU3CameraData::paramBufferReady(FrameBuffer *buffer)\n>> @@ -1438,13 +1424,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>> /*","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 DBF24C0F1B\n\tfor ;\n\tTue, 29 Mar 2022 19:25:39 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3056B65634;\n\tTue, 29 Mar 2022 21:25: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 C2DE1604BD\n\tfor ;\n\tTue, 29 Mar 2022 21:25:36 +0200 (CEST)","from [IPV6:2401:4900:1f3e:6e7e:1ff:a5c2:a86e:ae1b] (unknown\n\t[IPv6:2401:4900:1f3e:6e7e:1ff:a5c2:a86e:ae1b])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 689A92F7;\n\tTue, 29 Mar 2022 21:25:35 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1648581939;\n\tbh=n74tlKVtKFgsqywBTzJ7dItKEdIUPNod0cH/WXzaT7k=;\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=CpSN8G3H3lMOSv69uVsRq9boRoIa6v8dwZcJO39juGV43AxoU8MUIg8JJcrlvE1er\n\tapVowf/1kB24x1yUpwNKeEhD4+ExUC4ihNR+QrvpeHH9T6b4bPxZnpLPI51VovzNSf\n\t2hy0OuawzN8ImVn9X4mRcBNhEgB33zG/bqNgBiK2irMFLDRjj8Dl0m3WTwKpuUr82D\n\tUFi086bPgfJ3sMT+pzVY+ZCoyflai7zsYXWNX3oZ1N2znhkzxfAlgxqPr1bPUjKQKt\n\tt15nyL7UUZEMS7eJ8AJIo33Gmmgjky1vYY2SL5N22D2XcNoHuLWZbtcYW/aPv0gZ4p\n\tPRotwqCljDfLw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1648581936;\n\tbh=n74tlKVtKFgsqywBTzJ7dItKEdIUPNod0cH/WXzaT7k=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=cD9n9pypwfNRReejXb66tjcT0YXF62lUTnuru88u8MJzYOdwLBZonFI3rbJpQdnSu\n\tm1GHdgCao/vaMFLu+bKASAhXooZBBt3K0oKfRewa5UkSf36oZXkHfbg8wqS4HcH0Ii\n\tDEonjNVBFtfuBmt1Y7AxUobKHMEvVtXBg0l1WMkk="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"cD9n9pyp\"; dkim-atps=neutral","Message-ID":"<3e44b4b7-c122-af07-b33c-f075e580e946@ideasonboard.com>","Date":"Wed, 30 Mar 2022 00:55:29 +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":"Laurent Pinchart ","References":"<20220328173612.138332-1-umang.jain@ideasonboard.com>\n\t<20220328173612.138332-2-umang.jain@ideasonboard.com>\n\t","In-Reply-To":"","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH v3 1/1] 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":22516,"web_url":"https://patchwork.libcamera.org/comment/22516/","msgid":"","date":"2022-03-29T20:04:26","subject":"Re: [libcamera-devel] [PATCH v3 1/1] ipa: ipu3: Replace event-based\n\tops with dedicated functions","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Umang,\n\nOn Wed, Mar 30, 2022 at 12:55:29AM +0530, Umang Jain wrote:\n> On 3/29/22 03:55, Laurent Pinchart wrote:\n> > On Mon, Mar 28, 2022 at 11:06:11PM +0530, Umang Jain via libcamera-devel wrote:\n> >> From: Umang Jain via libcamera-devel \n> > That from line looks suspicious.\n> \n> Yeah, I noticed only after sending in the patch :-/\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 operations to dedicated functions:\n> >> ActionSetSensorControls => setSensorControls\n> >> ActionParamFilled => paramsBufferReady\n> >> ActionMetadataReady => metadataReady\n> >> EventProcessControls => processControls()\n> >\n> > What would you think about renaming this one to queueRequest(), to match\n> > the operation used by the rkisp1 IPA ?\n> \n> I don't find the correct matching. rkisp1 queueRequest() is more of \n> ActionParamFilled rather than of processing Controls. It's basically \n> doing processing of incoming coming and filling of param buffer in one \n> function.\n> \n> I probably we should probably separate the rkisp1 queueRequest() to \n> match more of ipu3, rather the way around.\n> \n> The IPAIPU3::processControls() seems more intuitive to me rather than \n> IPAIPU3::queueRequest(...controls..). Since the queue-ing of Request \n> part is done by the pipeline-handler and in process of doing so, it \n> shall call ipa_->processControls(..controls..) to instruct the IPA to \n> address the incoming controls. Does it make sense?\n\nI think that what bothers me here is the word \"process\". When we will\nhave proper per-frame controls in the IPU3 and RkISP1 IPAs, there will\nbe a queue of frame contexts. The processControls() function, called\nwhen the request is queued, will give the controls present in the\nrequest to the IPA. Some of them may be processed immediately, but I'd\nexpect the general idea will be to store those controls in the\nappropriate frame context, for later processing. For instance, controls\nthat affect the ISP only may only be processing in the\nfillParamsBuffer() call. That's why I proposed queueRequest(), as the\nfunction will put the parameters for the request in a queue. We could\ncall it queueControls(), but \"controls\" alone is a bit ambiguous I\nthink.\n\nRegarding the RkISP1, I would indeed like to split the exising function\nthat passes the request controls to the IPA in two. We currently return\nthe ISP parameters at that time, which may be very early if the queue of\nrequests is large. If we wait a bit before asking the IPA for ISP\nparameters, algorithms could be more reactive. However, we can't wait\ntoo long, as the RkISP1 is an inline ISP, we don't have the luxury to\nwait for ISP parameters like with the IPU3.\n\n> >> EventStatReady => processStatsBuffer()\n> >> EventFillParams => processParamsBuffer()\n> >\n> > This function doesn't process the buffer, but fills it. How about\n> > fillParamsBuffer() ?\n> \n> Sounds good\n> \n> >> Signed-off-by: Umang Jain \n> >> ---\n> >> include/libcamera/ipa/ipu3.mojom | 36 ++-----\n> >> src/ipa/ipu3/ipu3-ipa-design-guide.rst | 19 ++--\n> >> src/ipa/ipu3/ipu3.cpp | 132 +++++++++++--------------\n> >> src/libcamera/pipeline/ipu3/ipu3.cpp | 123 ++++++++++-------------\n> >> 4 files changed, 129 insertions(+), 181 deletions(-)\n> >>\n> >> diff --git a/include/libcamera/ipa/ipu3.mojom b/include/libcamera/ipa/ipu3.mojom\n> >> index 18cdc744..0bd5d641 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] processControls(uint32 frame, libcamera.ControlList controls);\n> >> +\t[async] processParamsBuffer(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..ed4b5ab7 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: (▼) processControls(), processParamsBuffer(), 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> >> +- processParamsBuffer()\n> >> +- processControls()\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> >> @@ -115,7 +117,7 @@ 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> >\n> > s/ the$//\n> >\n> > (or add \"function\" or something similar after ``processParamsBuffer()``)\n> >\n> >> -``EventFillParams`` event, and then passed directly to each algorithm\n> >> +``processParamsBuffer()``, 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> >> @@ -134,7 +136,7 @@ Post-frame completion\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> >> +``processStatsBuffer()``. This provides the IPA with an opportunity to\n> >\n> > Same here/\n> >\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> >\n> > s/ action$//\n> >\n> > (you may want to drop \"dedicated\" too)\n> >\n> >> -`ActionSetSensorControls` to allow the IPA to request controls to be set\n> >> +``setSensorControls`` signal to allow the IPA to request controls to be set\n> >> on the camera sensor through the pipeline handler.\n> >> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> >> index 50b52d8b..2c99055d 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> >> + * invocations from the IPU3 PipelineHandler to satisfy requests from the\n> >\n> > s/invocations/calls/\n> >\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 \\a processParamsBuffer(), we populate the ImgU parameter buffer with\n> >\n> > No need for \\a. It's used by doxygen to change the rendering of the next\n> > word, and we use it for function parameters only. Doxygen will detect\n> > function names thanks to the () and generate links.\n> >\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 \\a processStatsBuffer(). In this we run the\n> >\n> > Same, replace \"\\a\" with \"with\".\n> >\n> >> + * algorithms to parse the statistics and cache any results for the next\n> >> + * \\a processParamsBuffer() invocation.\n> >\n> > s/invocation/call/\n> >\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 processParamsBuffer(const uint32_t frame, const uint32_t bufferId) override;\n> >> +\tvoid processControls(const uint32_t frame, const ControlList &controls) override;\n> >> +\tvoid processStatsBuffer(const uint32_t frame, const int64_t frameTimestamp,\n> >> +\t\t\t\tconst uint32_t bufferId, const ControlList &sensorControls) override;\n> >\n> > Line break after bufferId ?\n> >\n> >> \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,61 +510,49 @@ 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 Prepare the ISP to process the Request\n> >\n> > Maybe we could improve this to be a bit more detailed ?\n> >\n> > * \\brief Fill and return a buffer with ISP processing parameters for a frame\n> >\n> >> + * \\param[in] frame The frame number\n> >> + * \\param[in] bufferId Buffer ID\n> >\n> > ID of the parameter buffer to fill\n> >\n> >> */\n> >> -void IPAIPU3::processEvent(const IPU3Event &event)\n> >> +void IPAIPU3::processParamsBuffer(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> > How about inlinking the fillParams() function here ? Now that there's no\n> > need for a switch/case on the operation ID, there's little reason to\n> > split the operation in two functions. Same for the other operation\n> > handlers.\n> >\n> > This can be done on top of this patch, to keep it reviewable.\n> >\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 Buffer ID\n> >\n> > ID of the statistics buffer\n> >\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> >> @@ -570,7 +563,7 @@ void IPAIPU3::processEvent(const IPU3Event &event)\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> >> +void IPAIPU3::processControls(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> >> @@ -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,19 @@ 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> >> +\tControlList lensCtrls(sensorCtrls_);\n> >> +\t/*\n> >> +\t * \\todo lensCtrls.set(V4L2_CID_FOCUS_ABSOLUTE, ));\n> >> +\t */\n> >\n> > Doesn't this cause a regression ?\n> \n> ops, yeah I think I ended up making a rebase fiasco. I'll address it.\n> \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..75b070bf 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 paramFilled(unsigned int id);\n> >\n> > s/param/params/\n> >\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_->processControls(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::paramFilled);\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,59 @@ 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 =\n> >> +\t\tlensControls.get(V4L2_CID_FOCUS_ABSOLUTE);\n> >\n> > You could make this a single line.\n> >\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::paramFilled(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 +1380,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_->processParamsBuffer(info->id, info->paramBuffer->cookie());\n> >> }\n> >> \n> >> void IPU3CameraData::paramBufferReady(FrameBuffer *buffer)\n> >> @@ -1438,13 +1424,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> >> /*","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 E01F1C3256\n\tfor ;\n\tTue, 29 Mar 2022 20:04:31 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6461065634;\n\tTue, 29 Mar 2022 22:04:31 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 67360604BD\n\tfor ;\n\tTue, 29 Mar 2022 22:04:30 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id A8AF52F7;\n\tTue, 29 Mar 2022 22:04:29 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1648584271;\n\tbh=TvB5y+PCn8+kaDLA+JFM3zbwjaVljrg3fLS6qC7A+qg=;\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=fCtxjLoIKI71aV5YyEKrNQoANW8sg7JIIMlRlL9hCh1q+cp3+zyxE55JIZ6eOiD7X\n\tUfTcorHz59wb6y479e3kwFgjwikKxEjS67o1QvRON4CNc+frfj/Y0cxv2kyCCTfJ5Z\n\tBI9T/iwPE26+j5k1KXhTFL8WntId8Os1nt/3cEE+MkOr/o0B+qYkEi30fQ3CQbE/RF\n\t4XzzVOAiIGXw1SjRi0obZc8AaLkTm48xAQmIAt82dWTYxlXLO6uJDmyKzIGnGsSMIu\n\tSIlEnA3ALLFn6pXdbySfs2Q0s4od0kKxvHL4JPMkmg0pdzNygbFZnPhFiQgqlY2Gar\n\tRPHO5j7X1IZWw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1648584270;\n\tbh=TvB5y+PCn8+kaDLA+JFM3zbwjaVljrg3fLS6qC7A+qg=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Mju/1oGScyDOPkAlyT7SOLiBlG2LH3FzQxy1SOeh725/9ym3zpJEIsv7fmqLVKe83\n\tAUGiFu99qfe9Pm4lJ08tHJjuzTdqeT2nFOpmfowJb1LAnOs6/CetOuphgEM/97b58r\n\tTqRg0Li5xzaxsAN2Du1X0dyhQ1NORw286m44KE4U="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"Mju/1oGS\"; dkim-atps=neutral","Date":"Tue, 29 Mar 2022 23:04:26 +0300","To":"Umang Jain ","Message-ID":"","References":"<20220328173612.138332-1-umang.jain@ideasonboard.com>\n\t<20220328173612.138332-2-umang.jain@ideasonboard.com>\n\t\n\t<3e44b4b7-c122-af07-b33c-f075e580e946@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<3e44b4b7-c122-af07-b33c-f075e580e946@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v3 1/1] 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":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":22520,"web_url":"https://patchwork.libcamera.org/comment/22520/","msgid":"<32dc5be2-b43e-c8ff-591a-e56da4d59865@ideasonboard.com>","date":"2022-03-30T05:30:55","subject":"Re: [libcamera-devel] [PATCH v3 1/1] 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 Laurent,\n\nOn 3/30/22 01:34, Laurent Pinchart wrote:\n> Hi Umang,\n>\n> On Wed, Mar 30, 2022 at 12:55:29AM +0530, Umang Jain wrote:\n>> On 3/29/22 03:55, Laurent Pinchart wrote:\n>>> On Mon, Mar 28, 2022 at 11:06:11PM +0530, Umang Jain via libcamera-devel wrote:\n>>>> From: Umang Jain via libcamera-devel \n>>> That from line looks suspicious.\n>> Yeah, I noticed only after sending in the patch :-/\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 operations to dedicated functions:\n>>>> ActionSetSensorControls => setSensorControls\n>>>> ActionParamFilled => paramsBufferReady\n>>>> ActionMetadataReady => metadataReady\n>>>> EventProcessControls => processControls()\n>>> What would you think about renaming this one to queueRequest(), to match\n>>> the operation used by the rkisp1 IPA ?\n>> I don't find the correct matching. rkisp1 queueRequest() is more of\n>> ActionParamFilled rather than of processing Controls. It's basically\n>> doing processing of incoming coming and filling of param buffer in one\n>> function.\n>>\n>> I probably we should probably separate the rkisp1 queueRequest() to\n>> match more of ipu3, rather the way around.\n>>\n>> The IPAIPU3::processControls() seems more intuitive to me rather than\n>> IPAIPU3::queueRequest(...controls..). Since the queue-ing of Request\n>> part is done by the pipeline-handler and in process of doing so, it\n>> shall call ipa_->processControls(..controls..) to instruct the IPA to\n>> address the incoming controls. Does it make sense?\n> I think that what bothers me here is the word \"process\". When we will\n> have proper per-frame controls in the IPU3 and RkISP1 IPAs, there will\n> be a queue of frame contexts. The processControls() function, called\n> when the request is queued, will give the controls present in the\n> request to the IPA. Some of them may be processed immediately, but I'd\n> expect the general idea will be to store those controls in the\n> appropriate frame context, for later processing. For instance, controls\n> that affect the ISP only may only be processing in the\n> fillParamsBuffer() call. That's why I proposed queueRequest(), as the\n\n\nFair enough.\n\n> function will put the parameters for the request in a queue. We could\n> call it queueControls(), but \"controls\" alone is a bit ambiguous I\n> think.\n\n\nYes, queueControls() doesn't sound right.\n\nMaybe IPAIPU3::handleControls() or handleRequestControls() instead of \nprocessControls()  ?\n\n>\n> Regarding the RkISP1, I would indeed like to split the exising function\n> that passes the request controls to the IPA in two. We currently return\n\n\nYes, I was irked by this while changing the interface for RkISP1 IPA. \nHowever, I deliberately didn't introduce the change since I wanted to \naddress the interface migration first. I can take a look at it now that \nit has landed.\n\n> the ISP parameters at that time, which may be very early if the queue of\n> requests is large. If we wait a bit before asking the IPA for ISP\n> parameters, algorithms could be more reactive. However, we can't wait\n> too long, as the RkISP1 is an inline ISP, we don't have the luxury to\n> wait for ISP parameters like with the IPU3.\n>\n>>>> EventStatReady => processStatsBuffer()\n>>>> EventFillParams => processParamsBuffer()\n>>> This function doesn't process the buffer, but fills it. How about\n>>> fillParamsBuffer() ?\n>> Sounds good\n>>\n>>>> Signed-off-by: Umang Jain \n>>>> ---\n>>>> include/libcamera/ipa/ipu3.mojom | 36 ++-----\n>>>> src/ipa/ipu3/ipu3-ipa-design-guide.rst | 19 ++--\n>>>> src/ipa/ipu3/ipu3.cpp | 132 +++++++++++--------------\n>>>> src/libcamera/pipeline/ipu3/ipu3.cpp | 123 ++++++++++-------------\n>>>> 4 files changed, 129 insertions(+), 181 deletions(-)\n>>>>\n>>>> diff --git a/include/libcamera/ipa/ipu3.mojom b/include/libcamera/ipa/ipu3.mojom\n>>>> index 18cdc744..0bd5d641 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] processControls(uint32 frame, libcamera.ControlList controls);\n>>>> +\t[async] processParamsBuffer(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..ed4b5ab7 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: (▼) processControls(), processParamsBuffer(), 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>>>> +- processParamsBuffer()\n>>>> +- processControls()\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>>>> @@ -115,7 +117,7 @@ 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>>> s/ the$//\n>>>\n>>> (or add \"function\" or something similar after ``processParamsBuffer()``)\n>>>\n>>>> -``EventFillParams`` event, and then passed directly to each algorithm\n>>>> +``processParamsBuffer()``, 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>>>> @@ -134,7 +136,7 @@ Post-frame completion\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>>>> +``processStatsBuffer()``. This provides the IPA with an opportunity to\n>>> Same here/\n>>>\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>>> s/ action$//\n>>>\n>>> (you may want to drop \"dedicated\" too)\n>>>\n>>>> -`ActionSetSensorControls` to allow the IPA to request controls to be set\n>>>> +``setSensorControls`` signal to allow the IPA to request controls to be set\n>>>> on the camera sensor through the pipeline handler.\n>>>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n>>>> index 50b52d8b..2c99055d 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>>>> + * invocations from the IPU3 PipelineHandler to satisfy requests from the\n>>> s/invocations/calls/\n>>>\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 \\a processParamsBuffer(), we populate the ImgU parameter buffer with\n>>> No need for \\a. It's used by doxygen to change the rendering of the next\n>>> word, and we use it for function parameters only. Doxygen will detect\n>>> function names thanks to the () and generate links.\n>>>\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 \\a processStatsBuffer(). In this we run the\n>>> Same, replace \"\\a\" with \"with\".\n>>>\n>>>> + * algorithms to parse the statistics and cache any results for the next\n>>>> + * \\a processParamsBuffer() invocation.\n>>> s/invocation/call/\n>>>\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 processParamsBuffer(const uint32_t frame, const uint32_t bufferId) override;\n>>>> +\tvoid processControls(const uint32_t frame, const ControlList &controls) override;\n>>>> +\tvoid processStatsBuffer(const uint32_t frame, const int64_t frameTimestamp,\n>>>> +\t\t\t\tconst uint32_t bufferId, const ControlList &sensorControls) override;\n>>> Line break after bufferId ?\n>>>\n>>>> \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,61 +510,49 @@ 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 Prepare the ISP to process the Request\n>>> Maybe we could improve this to be a bit more detailed ?\n>>>\n>>> * \\brief Fill and return a buffer with ISP processing parameters for a frame\n>>>\n>>>> + * \\param[in] frame The frame number\n>>>> + * \\param[in] bufferId Buffer ID\n>>> ID of the parameter buffer to fill\n>>>\n>>>> */\n>>>> -void IPAIPU3::processEvent(const IPU3Event &event)\n>>>> +void IPAIPU3::processParamsBuffer(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>>> How about inlinking the fillParams() function here ? Now that there's no\n>>> need for a switch/case on the operation ID, there's little reason to\n>>> split the operation in two functions. Same for the other operation\n>>> handlers.\n>>>\n>>> This can be done on top of this patch, to keep it reviewable.\n>>>\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 Buffer ID\n>>> ID of the statistics buffer\n>>>\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>>>> @@ -570,7 +563,7 @@ void IPAIPU3::processEvent(const IPU3Event &event)\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>>>> +void IPAIPU3::processControls(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>>>> @@ -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,19 @@ 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>>>> +\tControlList lensCtrls(sensorCtrls_);\n>>>> +\t/*\n>>>> +\t * \\todo lensCtrls.set(V4L2_CID_FOCUS_ABSOLUTE, ));\n>>>> +\t */\n>>> Doesn't this cause a regression ?\n>> ops, yeah I think I ended up making a rebase fiasco. I'll address it.\n>>\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..75b070bf 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 paramFilled(unsigned int id);\n>>> s/param/params/\n>>>\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_->processControls(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::paramFilled);\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,59 @@ 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 =\n>>>> +\t\tlensControls.get(V4L2_CID_FOCUS_ABSOLUTE);\n>>> You could make this a single line.\n>>>\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::paramFilled(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 +1380,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_->processParamsBuffer(info->id, info->paramBuffer->cookie());\n>>>> }\n>>>> \n>>>> void IPU3CameraData::paramBufferReady(FrameBuffer *buffer)\n>>>> @@ -1438,13 +1424,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>>>> /*","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 9814AC3256\n\tfor ;\n\tWed, 30 Mar 2022 05:31:06 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0042165636;\n\tWed, 30 Mar 2022 07:31:05 +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 424E3604BB\n\tfor ;\n\tWed, 30 Mar 2022 07:31:05 +0200 (CEST)","from [192.168.1.105] (unknown [103.74.73.208])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 472092F7;\n\tWed, 30 Mar 2022 07:31:02 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1648618266;\n\tbh=W/CNAW+Bf0zwPgJVB3gBPr0ub0vM/+z2tKkr59fQKfM=;\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=tY76/RIA+6sGh6IigsAk8k1jx6GBFwyiaoqzxGCdGwYbZ53RfK/g+xVJK7byWNspU\n\tdsrZ5nznoj+d5AWURY91VORO9ESo+AsJxUmAmmmywowV3k1EkvMQUxzZ9VJtKJbHqk\n\tZtPRTYqG6vpgtLv4zUAbrr6rupgZdK0uFMhjAGCc/8B8mZRTFkhniNhPtrWA4gCqYw\n\tf9qHBYU4GMCnBdYEl7zOFH9DX6MrBSDOCYsl1SMbzNmlyblnj0UYCA8BwqkhQdrE69\n\tyv5gWwv/sj8VlIbQsQRyg8Rdwt19NItQhUw8OHksBPxQAQQPAconWuWggkyP8OZVaE\n\tbtL0tNSrU7NVg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1648618264;\n\tbh=W/CNAW+Bf0zwPgJVB3gBPr0ub0vM/+z2tKkr59fQKfM=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=Pz9RPwo701XvI4y60P+QOkroWJj8HitXNWKFDnJAH6ElhdVyEkcS4DzAO+//ntqf1\n\tPbtBZfY8e0mvnrp0IRRBSvOHr1Am+HDOrlzZ2PuMJrz/N/seQ8e86ZQlxqE4QIe9JR\n\tFhvEPth3ydzVUkK6JECXP0Xb20nOIldhj74Xd71k="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"Pz9RPwo7\"; dkim-atps=neutral","Message-ID":"<32dc5be2-b43e-c8ff-591a-e56da4d59865@ideasonboard.com>","Date":"Wed, 30 Mar 2022 11:00:55 +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":"Laurent Pinchart ","References":"<20220328173612.138332-1-umang.jain@ideasonboard.com>\n\t<20220328173612.138332-2-umang.jain@ideasonboard.com>\n\t\n\t<3e44b4b7-c122-af07-b33c-f075e580e946@ideasonboard.com>\n\t","In-Reply-To":"","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH v3 1/1] 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\" "}}]