[{"id":22585,"web_url":"https://patchwork.libcamera.org/comment/22585/","msgid":"<YkzIAzGiEHVVSgV7@pendragon.ideasonboard.com>","date":"2022-04-05T22:51:47","subject":"Re: [libcamera-devel] [PATCH v4 1/6] 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 Thu, Mar 31, 2022 at 10:00:53PM +0530, Umang Jain via libcamera-devel wrote:\n> The IPAIPU3 interface currently uses event-type based structures in\n> order to communicate with the pipeline-handler (and vice-versa).\n> Replace the event based structures with dedicated functions associated\n> to each operation.\n> \n> The translated naming scheme of operations to dedicated functions:\n>   ActionSetSensorControls => setSensorControls\n>   ActionParamFilled       => paramsBufferReady\n>   ActionMetadataReady     => metadataReady\n>   EventProcessControls    => queueRequest()\n>   EventStatReady          => processStatsBuffer()\n>   EventFillParams         => fillParamsBuffer()\n> \n> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>\n> ---\n>  include/libcamera/ipa/ipu3.mojom       |  36 ++-----\n>  src/ipa/ipu3/ipu3-ipa-design-guide.rst |  27 ++---\n>  src/ipa/ipu3/ipu3.cpp                  | 132 +++++++++++--------------\n>  src/libcamera/pipeline/ipu3/ipu3.cpp   | 122 ++++++++++-------------\n>  4 files changed, 132 insertions(+), 185 deletions(-)\n> \n> diff --git a/include/libcamera/ipa/ipu3.mojom b/include/libcamera/ipa/ipu3.mojom\n> index 18cdc744..b1e06e65 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<libcamera.IPABuffer> buffers);\n>  \tunmapBuffers(array<uint32> ids);\n>  \n> -\t[async] processEvent(IPU3Event ev);\n> +\t[async] fillParamsBuffer(uint32 frame, uint32 bufferId);\n> +\t[async] processStatsBuffer(uint32 frame, int64 frameTimestamp,\n> +\t\t\t\t   uint32 bufferId, libcamera.ControlList sensorControls);\n> +\t[async] queueRequest(uint32 frame, libcamera.ControlList controls);\n\nNitpicking, I would have put queueRequest() first, to match the logical\norder in which the operations will be called.\n\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..13682599 100644\n> --- a/src/ipa/ipu3/ipu3-ipa-design-guide.rst\n> +++ b/src/ipa/ipu3/ipu3-ipa-design-guide.rst\n> @@ -25,7 +25,8 @@ from applications, and managing events from the pipeline handler.\n>        └─┬───┬───┬──────┬────┬────┬────┬─┴────▼─┬──┘    1: init()\n>          │   │   │      │ ▲  │ ▲  │ ▲  │ ▲      │       2: configure()\n>          │1  │2  │3     │4│  │4│  │4│  │4│      │5      3: mapBuffers(), start()\n> -        ▼   ▼   ▼      ▼ │  ▼ │  ▼ │  ▼ │      ▼       4: processEvent()\n> +        │   │   │      │ │  │ │  │ │  │ │      │       4: (▼) queueRequest(), fillParamsBuffer(), processStatsBuffer()\n> +        ▼   ▼   ▼      ▼ │  ▼ │  ▼ │  ▼ │      ▼          (▲) setSensorControls, paramsBufferReady, metadataReady Signals\n>        ┌──────────────────┴────┴────┴────┴─────────┐    5: stop(), unmapBuffers()\n>        │ IPU3 IPA                                  │\n>        │                 ┌───────────────────────┐ │\n> @@ -100,8 +101,9 @@ There are three main interactions with the algorithms for the IPU3 IPA\n>  to operate when running:\n>  \n>  -  configure()\n> --  processEvent(``EventFillParams``)\n> --  processEvent(``EventStatReady``)\n> +-  fillParamsBuffer()\n> +-  queueRequest()\n> +-  processStatsBuffer()\n\nSame here, same order as in the mojom file. And same in the\nimplementation.\n\n>  \n>  The configuration phase allows the pipeline-handler to inform the IPA of\n>  the current stream configurations, which is then passed into each\n> @@ -114,8 +116,8 @@ Pre-frame preparation\n>  When configured, the IPA is notified by the pipeline handler of the\n>  Camera ``start()`` event, after which incoming requests will be queued\n>  for processing, requiring a parameter buffer (``ipu3_uapi_params``) to\n> -be populated for the ImgU. This is given to the IPA through the\n> -``EventFillParams`` event, and then passed directly to each algorithm\n> +be populated for the ImgU. This is given to the IPA through\n> +``fillParamsBuffer()``, and then passed directly to each algorithm\n>  through the ``prepare()`` call allowing the ISP configuration to be\n>  updated for the needs of each component that the algorithm is\n>  responsible for.\n> @@ -125,7 +127,7 @@ structure that it modifies, and it should take care to ensure that any\n>  structure set by a use flag is fully initialised to suitable values.\n>  \n>  The parameter buffer is returned to the pipeline handler through the\n> -``ActionParamFilled`` event, and from there queued to the ImgU along\n> +``paramsBufferReady`` signal, and from there queued to the ImgU along\n>  with a raw frame captured with the CIO2.\n>  \n>  Post-frame completion\n> @@ -133,8 +135,8 @@ Post-frame completion\n>  \n>  When the capture of an image is completed, and successfully processed\n>  through the ImgU, the generated statistics buffer\n> -(``ipu3_uapi_stats_3a``) is given to the IPA through the\n> -``EventStatReady`` event. This provides the IPA with an opportunity to\n> +(``ipu3_uapi_stats_3a``) is given to the IPA through\n> +``processStatsBuffer()``. This provides the IPA with an opportunity to\n>  examine the results of the ISP and run the calculations required by each\n>  algorithm on the new data. The algorithms may require context from the\n>  operations of other algorithms, for example, the AWB might choose to use\n> @@ -145,11 +147,14 @@ before they are needed.\n>  The ordering of the algorithm processing is determined by their\n>  placement in the ``IPU3::algorithms_`` ordered list.\n>  \n> +Finally, the IPA metadata for the completed frame is returned back via\n> +the ``metadataReady`` signal.\n> +\n>  Sensor Controls\n>  ~~~~~~~~~~~~~~~\n>  \n>  The AutoExposure and AutoGain (AGC) algorithm differs slightly from the\n>  others as it requires operating directly on the sensor, as opposed to\n> -through the ImgU ISP. To support this, there is a dedicated action\n> -`ActionSetSensorControls` to allow the IPA to request controls to be set\n> -on the camera sensor through the pipeline handler.\n> +through the ImgU ISP. To support this, there is a ``setSensorControls``\n> +signal to allow the IPA to request controls to be set on the camera\n> +sensor through the pipeline handler.\n> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> index 50b52d8b..7779ad58 100644\n> --- a/src/ipa/ipu3/ipu3.cpp\n> +++ b/src/ipa/ipu3/ipu3.cpp\n> @@ -65,8 +65,9 @@ namespace ipa::ipu3 {\n>   * The IPU3 Pipeline defines an IPU3-specific interface for communication\n>   * between the PipelineHandler and the IPA module.\n>   *\n> - * We extend the IPAIPU3Interface to implement our algorithms and handle events\n> - * from the IPU3 PipelineHandler to satisfy requests from the application.\n> + * We extend the IPAIPU3Interface to implement our algorithms and handle\n> + * calls from the IPU3 PipelineHandler to satisfy requests from the\n> + * application.\n>   *\n>   * At initialisation time, a CameraSensorHelper is instantiated to support\n>   * camera-specific calculations, while the default controls are computed, and\n> @@ -81,14 +82,14 @@ namespace ipa::ipu3 {\n>   * parameter buffer, and adapting the settings of the sensor attached to the\n>   * IPU3 CIO2 through sensor-specific V4L2 controls.\n>   *\n> - * When the event \\a EventFillParams occurs we populate the ImgU parameter\n> - * buffer with settings to configure the device in preparation for handling the\n> - * frame queued in the Request.\n> + * In fillParamsBuffer(), we populate the ImgU parameter buffer with\n> + * settings to configure the device in preparation for handling the frame\n> + * queued in the Request.\n>   *\n>   * When the frame has completed processing, the ImgU will generate a statistics\n> - * buffer which is given to the IPA as part of the \\a EventStatReady event. At\n> - * this event we run the algorithms to parse the statistics and cache any\n> - * results for the next \\a EventFillParams event.\n> + * buffer which is given to the IPA with processStatsBuffer(). In this we run the\n> + * algorithms to parse the statistics and cache any results for the next\n> + * fillParamsBuffer() function.\n\ns/function/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,19 @@ public:\n>  \n>  \tvoid mapBuffers(const std::vector<IPABuffer> &buffers) override;\n>  \tvoid unmapBuffers(const std::vector<unsigned int> &ids) override;\n> -\tvoid processEvent(const IPU3Event &event) override;\n> +\n> +\tvoid fillParamsBuffer(const uint32_t frame, const uint32_t bufferId) override;\n> +\tvoid processStatsBuffer(const uint32_t frame, const int64_t frameTimestamp,\n> +\t\t\t\tconst uint32_t bufferId,\n> +\t\t\t\tconst ControlList &sensorControls) override;\n> +\tvoid queueRequest(const uint32_t frame, const ControlList &controls) override;\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,73 +511,61 @@ void IPAIPU3::unmapBuffers(const std::vector<unsigned int> &ids)\n>  }\n>  \n>  /**\n> - * \\brief Process an event generated by the pipeline handler\n> - * \\param[in] event The event sent from pipeline handler\n> - *\n> - * The expected event handling over the lifetime of a Request has\n> - * the following sequence:\n> - *\n> - *   - EventProcessControls : Handle controls from a new Request\n> - *   - EventFillParams : Prepare the ISP to process the Request\n> - *   - EventStatReady : Process statistics after ISP completion\n> + * \\brief Fill and return a buffer with ISP processing parameters for a frame\n> + * \\param[in] frame The frame number\n> + * \\param[in] bufferId ID of the parameter buffer to fill\n>   */\n> -void IPAIPU3::processEvent(const IPU3Event &event)\n> +void IPAIPU3::fillParamsBuffer(const uint32_t frame, const uint32_t bufferId)\n>  {\n> -\tswitch (event.op) {\n> -\tcase EventProcessControls: {\n> -\t\tprocessControls(event.frame, event.controls);\n> -\t\tbreak;\n> +\tauto it = buffers_.find(bufferId);\n> +\tif (it == buffers_.end()) {\n> +\t\tLOG(IPAIPU3, Error) << \"Could not find param buffer!\";\n> +\t\treturn;\n>  \t}\n> -\tcase EventFillParams: {\n> -\t\tauto it = buffers_.find(event.bufferId);\n> -\t\tif (it == buffers_.end()) {\n> -\t\t\tLOG(IPAIPU3, Error) << \"Could not find param buffer!\";\n> -\t\t\treturn;\n> -\t\t}\n>  \n> -\t\tSpan<uint8_t> mem = it->second.planes()[0];\n> -\t\tipu3_uapi_params *params =\n> -\t\t\treinterpret_cast<ipu3_uapi_params *>(mem.data());\n> +\tSpan<uint8_t> mem = it->second.planes()[0];\n> +\tipu3_uapi_params *params =\n> +\t\treinterpret_cast<ipu3_uapi_params *>(mem.data());\n>  \n> -\t\tfillParams(event.frame, params);\n> -\t\tbreak;\n> -\t}\n> -\tcase EventStatReady: {\n> -\t\tauto it = buffers_.find(event.bufferId);\n> -\t\tif (it == buffers_.end()) {\n> -\t\t\tLOG(IPAIPU3, Error) << \"Could not find stats buffer!\";\n> -\t\t\treturn;\n> -\t\t}\n> +\tfillParams(frame, params);\n> +}\n>  \n> -\t\tSpan<uint8_t> mem = it->second.planes()[0];\n> -\t\tconst ipu3_uapi_stats_3a *stats =\n> -\t\t\treinterpret_cast<ipu3_uapi_stats_3a *>(mem.data());\n> +/**\n> + * \\brief Process statistics after ISP completion\n> + * \\param[in] frame The frame number\n> + * \\param[in] frameTimestamp Timestamp of the frame\n> + * \\param[in] bufferId ID of the statistics buffer\n> + * \\param[in] sensorControls Sensor controls\n> + */\n> +void IPAIPU3::processStatsBuffer(const uint32_t frame, const int64_t frameTimestamp,\n> +\t\t\t\t const uint32_t bufferId, const ControlList &sensorControls)\n> +{\n> +\tauto it = buffers_.find(bufferId);\n> +\tif (it == buffers_.end()) {\n> +\t\tLOG(IPAIPU3, Error) << \"Could not find stats buffer!\";\n> +\t\treturn;\n> +\t}\n>  \n> -\t\tint32_t exposure = event.sensorControls.get(V4L2_CID_EXPOSURE).get<int32_t>();\n> -\t\tint32_t gain = event.sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>();\n> +\tSpan<uint8_t> mem = it->second.planes()[0];\n> +\tconst ipu3_uapi_stats_3a *stats =\n> +\t\treinterpret_cast<ipu3_uapi_stats_3a *>(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<int32_t>();\n> +\tcontext_.frameContext.sensor.gain = camHelper_->gain(sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>());\n>  \n> -\t\tparseStatistics(event.frame, event.frameTimestamp, stats);\n> -\t\tbreak;\n> -\t}\n> -\tdefault:\n> -\t\tLOG(IPAIPU3, Error) << \"Unknown event \" << event.op;\n> -\t\tbreak;\n> -\t}\n> +\tparseStatistics(frame, frameTimestamp, stats);\n>  }\n>  \n>  /**\n> - * \\brief Process a control list for a request from the application\n> + * \\brief Queue a request and process the control list from the application\n>   * \\param[in] frame The number of the frame which will be processed next\n>   * \\param[in] controls The controls for the \\a frame\n>   *\n>   * Parse the request to handle any IPA-managed controls that were set from the\n>   * application such as manual sensor settings.\n>   */\n> -void IPAIPU3::processControls([[maybe_unused]] unsigned int frame,\n> -\t\t\t      [[maybe_unused]] const ControlList &controls)\n> +void IPAIPU3::queueRequest(const uint32_t frame,\n> +\t\t\t   [[maybe_unused]] const ControlList &controls)\n>  {\n>  \t/* \\todo Start processing for 'frame' based on 'controls'. */\n>  }\n> @@ -600,10 +594,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 +638,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 +650,18 @@ void IPAIPU3::parseStatistics(unsigned int frame,\n>   */\n>  void IPAIPU3::setControls(unsigned int frame)\n>  {\n> -\tIPU3Action op;\n> -\top.op = ActionSetSensorControls;\n> -\n>  \tint32_t exposure = context_.frameContext.agc.exposure;\n>  \tint32_t gain = camHelper_->gainCode(context_.frameContext.agc.gain);\n>  \n>  \tControlList ctrls(sensorCtrls_);\n>  \tctrls.set(V4L2_CID_EXPOSURE, exposure);\n>  \tctrls.set(V4L2_CID_ANALOGUE_GAIN, gain);\n> -\top.sensorControls = ctrls;\n>  \n> -\tControlList lensCtrls(lensCtrls_);\n> +\tControlList lensCtrls(sensorCtrls_);\n\nIs this right ?\n\nReviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n\n>  \tlensCtrls.set(V4L2_CID_FOCUS_ABSOLUTE,\n>  \t\t      static_cast<int32_t>(context_.frameContext.af.focus));\n> -\top.lensControls = lensCtrls;\n>  \n> -\tqueueFrameAction.emit(frame, op);\n> +\tsetSensorControls.emit(frame, ctrls, lensCtrls);\n>  }\n>  \n>  } /* namespace ipa::ipu3 */\n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index 60e01917..59d7e9c0 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -86,8 +86,10 @@ public:\n>  \tControlInfoMap ipaControls_;\n>  \n>  private:\n> -\tvoid queueFrameAction(unsigned int id,\n> -\t\t\t      const ipa::ipu3::IPU3Action &action);\n> +\tvoid metadataReady(unsigned int id, const ControlList &metadata);\n> +\tvoid paramsFilled(unsigned int id);\n> +\tvoid setSensorControls(unsigned int id, const ControlList &sensorControls,\n> +\t\t\t       const ControlList &lensControls);\n>  };\n>  \n>  class IPU3CameraConfiguration : public CameraConfiguration\n> @@ -871,11 +873,7 @@ void IPU3CameraData::queuePendingRequests()\n>  \n>  \t\tinfo->rawBuffer = rawBuffer;\n>  \n> -\t\tipa::ipu3::IPU3Event ev;\n> -\t\tev.op = ipa::ipu3::EventProcessControls;\n> -\t\tev.frame = info->id;\n> -\t\tev.controls = request->controls();\n> -\t\tipa_->processEvent(ev);\n> +\t\tipa_->queueRequest(info->id, request->controls());\n>  \n>  \t\tpendingRequests_.pop();\n>  \t\tprocessingRequests_.push(request);\n> @@ -1218,7 +1216,9 @@ int IPU3CameraData::loadIPA()\n>  \tif (!ipa_)\n>  \t\treturn -ENOENT;\n>  \n> -\tipa_->queueFrameAction.connect(this, &IPU3CameraData::queueFrameAction);\n> +\tipa_->setSensorControls.connect(this, &IPU3CameraData::setSensorControls);\n> +\tipa_->paramsBufferReady.connect(this, &IPU3CameraData::paramsFilled);\n> +\tipa_->metadataReady.connect(this, &IPU3CameraData::metadataReady);\n>  \n>  \t/*\n>  \t * Pass the sensor info to the IPA to initialize controls.\n> @@ -1253,69 +1253,58 @@ int IPU3CameraData::loadIPA()\n>  \treturn 0;\n>  }\n>  \n> -void IPU3CameraData::queueFrameAction(unsigned int id,\n> -\t\t\t\t      const ipa::ipu3::IPU3Action &action)\n> +void IPU3CameraData::setSensorControls([[maybe_unused]] unsigned int id,\n> +\t\t\t\t       const ControlList &sensorControls,\n> +\t\t\t\t       const ControlList &lensControls)\n>  {\n> -\tswitch (action.op) {\n> -\tcase ipa::ipu3::ActionSetSensorControls: {\n> -\t\tconst ControlList &sensorControls = action.sensorControls;\n> -\t\tdelayedCtrls_->push(sensorControls);\n> +\tdelayedCtrls_->push(sensorControls);\n>  \n> -\t\tCameraLens *focusLens = cio2_.sensor()->focusLens();\n> -\t\tif (!focusLens)\n> -\t\t\tbreak;\n> -\n> -\t\tconst ControlList lensControls = action.lensControls;\n> -\t\tif (!lensControls.contains(V4L2_CID_FOCUS_ABSOLUTE))\n> -\t\t\tbreak;\n> +\tCameraLens *focusLens = cio2_.sensor()->focusLens();\n> +\tif (!focusLens)\n> +\t\treturn;\n>  \n> -\t\tconst ControlValue &focusValue =\n> -\t\t\tlensControls.get(V4L2_CID_FOCUS_ABSOLUTE);\n> +\tif (!lensControls.contains(V4L2_CID_FOCUS_ABSOLUTE))\n> +\t\treturn;\n>  \n> -\t\tfocusLens->setFocusPosition(focusValue.get<int32_t>());\n> +\tconst ControlValue &focusValue = lensControls.get(V4L2_CID_FOCUS_ABSOLUTE);\n>  \n> -\t\tbreak;\n> -\t}\n> -\tcase ipa::ipu3::ActionParamFilled: {\n> -\t\tIPU3Frames::Info *info = frameInfos_.find(id);\n> -\t\tif (!info)\n> -\t\t\tbreak;\n> -\n> -\t\t/* Queue all buffers from the request aimed for the ImgU. */\n> -\t\tfor (auto it : info->request->buffers()) {\n> -\t\t\tconst Stream *stream = it.first;\n> -\t\t\tFrameBuffer *outbuffer = it.second;\n> +\tfocusLens->setFocusPosition(focusValue.get<int32_t>());\n> +}\n>  \n> -\t\t\tif (stream == &outStream_)\n> -\t\t\t\timgu_->output_->queueBuffer(outbuffer);\n> -\t\t\telse if (stream == &vfStream_)\n> -\t\t\t\timgu_->viewfinder_->queueBuffer(outbuffer);\n> -\t\t}\n> +void IPU3CameraData::paramsFilled(unsigned int id)\n> +{\n> +\tIPU3Frames::Info *info = frameInfos_.find(id);\n> +\tif (!info)\n> +\t\treturn;\n>  \n> -\t\timgu_->param_->queueBuffer(info->paramBuffer);\n> -\t\timgu_->stat_->queueBuffer(info->statBuffer);\n> -\t\timgu_->input_->queueBuffer(info->rawBuffer);\n> +\t/* Queue all buffers from the request aimed for the ImgU. */\n> +\tfor (auto it : info->request->buffers()) {\n> +\t\tconst Stream *stream = it.first;\n> +\t\tFrameBuffer *outbuffer = it.second;\n>  \n> -\t\tbreak;\n> +\t\tif (stream == &outStream_)\n> +\t\t\timgu_->output_->queueBuffer(outbuffer);\n> +\t\telse if (stream == &vfStream_)\n> +\t\t\timgu_->viewfinder_->queueBuffer(outbuffer);\n>  \t}\n> -\tcase ipa::ipu3::ActionMetadataReady: {\n> -\t\tIPU3Frames::Info *info = frameInfos_.find(id);\n> -\t\tif (!info)\n> -\t\t\tbreak;\n>  \n> -\t\tRequest *request = info->request;\n> -\t\trequest->metadata().merge(action.controls);\n> +\timgu_->param_->queueBuffer(info->paramBuffer);\n> +\timgu_->stat_->queueBuffer(info->statBuffer);\n> +\timgu_->input_->queueBuffer(info->rawBuffer);\n> +}\n>  \n> -\t\tinfo->metadataProcessed = true;\n> -\t\tif (frameInfos_.tryComplete(info))\n> -\t\t\tpipe()->completeRequest(request);\n> +void IPU3CameraData::metadataReady(unsigned int id, const ControlList &metadata)\n> +{\n> +\tIPU3Frames::Info *info = frameInfos_.find(id);\n> +\tif (!info)\n> +\t\treturn;\n>  \n> -\t\tbreak;\n> -\t}\n> -\tdefault:\n> -\t\tLOG(IPU3, Error) << \"Unknown action \" << action.op;\n> -\t\tbreak;\n> -\t}\n> +\tRequest *request = info->request;\n> +\trequest->metadata().merge(metadata);\n> +\n> +\tinfo->metadataProcessed = true;\n> +\tif (frameInfos_.tryComplete(info))\n> +\t\tpipe()->completeRequest(request);\n>  }\n>  \n>  /* -----------------------------------------------------------------------------\n> @@ -1390,11 +1379,7 @@ void IPU3CameraData::cio2BufferReady(FrameBuffer *buffer)\n>  \tif (request->findBuffer(&rawStream_))\n>  \t\tpipe()->completeBuffer(request, buffer);\n>  \n> -\tipa::ipu3::IPU3Event ev;\n> -\tev.op = ipa::ipu3::EventFillParams;\n> -\tev.frame = info->id;\n> -\tev.bufferId = info->paramBuffer->cookie();\n> -\tipa_->processEvent(ev);\n> +\tipa_->fillParamsBuffer(info->id, info->paramBuffer->cookie());\n>  }\n>  \n>  void IPU3CameraData::paramBufferReady(FrameBuffer *buffer)\n> @@ -1438,13 +1423,8 @@ void IPU3CameraData::statBufferReady(FrameBuffer *buffer)\n>  \t\treturn;\n>  \t}\n>  \n> -\tipa::ipu3::IPU3Event ev;\n> -\tev.op = ipa::ipu3::EventStatReady;\n> -\tev.frame = info->id;\n> -\tev.bufferId = info->statBuffer->cookie();\n> -\tev.frameTimestamp = request->metadata().get(controls::SensorTimestamp);\n> -\tev.sensorControls = info->effectiveSensorControls;\n> -\tipa_->processEvent(ev);\n> +\tipa_->processStatsBuffer(info->id, request->metadata().get(controls::SensorTimestamp),\n> +\t\t\t\t info->statBuffer->cookie(), info->effectiveSensorControls);\n>  }\n>  \n>  /*","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 E2D9BC3256\n\tfor <parsemail@patchwork.libcamera.org>;\n\tTue,  5 Apr 2022 22:51:52 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3E4F165642;\n\tWed,  6 Apr 2022 00:51:52 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 548E1633A5\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed,  6 Apr 2022 00:51:51 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(117.145-247-81.adsl-dyn.isp.belgacom.be [81.247.145.117])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id CE411482;\n\tWed,  6 Apr 2022 00:51:50 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1649199112;\n\tbh=8UYci/MbHeEU4KX9jpjcHlc5fFsL0rl3qmwzRtcGt00=;\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=gZOxTgMFkkwQ0/pgV+gpbs7pD+2elJPQ6y5I2rv6eVjiPTAMq9jakyNvgfCIbOoFo\n\t7HYpxUovdhMrGSvzXsbKUIle/8FhlrGw67AHdFXBXKuk4Cvgstmx5FGAa30hXqyEWP\n\tjLki2pv5LPJJSYVSjHEKmNUFmmKShDqUdnAjoP4mlCpfKbBoU7tIblexnw2B6lEk1k\n\tL5n9H+TdE3nYEmG1teJy/FJyBEv6qHvB7s0rGWhmRrt36Nz0hdkznZ2feefzoJw1pq\n\t0cD2KRbaQa7s9EgxH0BQnV1fifhiANnHFH+ycwKE5fczAHoYT2800ylkbFCtNQDpzH\n\t2rwbF4JiYYtBg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1649199111;\n\tbh=8UYci/MbHeEU4KX9jpjcHlc5fFsL0rl3qmwzRtcGt00=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=QHVZAsuZOlq8Iady1oDhwJJVKD3Fwu/lyibp2CVwAaGA2p5z32ov/6LKK91vxXwBl\n\tWvJajqZvU8kAYT+ZzE09Bz0/Bmy137YU17GJnKEVmTiVgDiR5VGjiX2OSOg7QLHbr2\n\tKr6A3XYjjfxBpiT1js/9UtvG3PrzjbgqMML1Ynvw="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"QHVZAsuZ\"; dkim-atps=neutral","Date":"Wed, 6 Apr 2022 01:51:47 +0300","To":"Umang Jain <umang.jain@ideasonboard.com>","Message-ID":"<YkzIAzGiEHVVSgV7@pendragon.ideasonboard.com>","References":"<20220331163058.171418-1-umang.jain@ideasonboard.com>\n\t<20220331163058.171418-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":"<20220331163058.171418-2-umang.jain@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v4 1/6] ipa: ipu3: Replace event-based\n\tops with dedicated functions","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","From":"Laurent Pinchart via libcamera-devel\n\t<libcamera-devel@lists.libcamera.org>","Reply-To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":22611,"web_url":"https://patchwork.libcamera.org/comment/22611/","msgid":"<dcba5efa-5663-9714-743f-6033f277cddc@ideasonboard.com>","date":"2022-04-06T09:33:59","subject":"Re: [libcamera-devel] [PATCH v4 1/6] ipa: ipu3: Replace event-based\n\tops with dedicated functions","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi,\n\nOn 4/6/22 04:21, Laurent Pinchart wrote:\n> Hi Umang,\n>\n> Thank you for the patch.\n>\n> On Thu, Mar 31, 2022 at 10:00:53PM +0530, Umang Jain via libcamera-devel wrote:\n>> The IPAIPU3 interface currently uses event-type based structures in\n>> order to communicate with the pipeline-handler (and vice-versa).\n>> Replace the event based structures with dedicated functions associated\n>> to each operation.\n>>\n>> The translated naming scheme of operations to dedicated functions:\n>>    ActionSetSensorControls => setSensorControls\n>>    ActionParamFilled       => paramsBufferReady\n>>    ActionMetadataReady     => metadataReady\n>>    EventProcessControls    => queueRequest()\n>>    EventStatReady          => processStatsBuffer()\n>>    EventFillParams         => fillParamsBuffer()\n>>\n>> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>\n>> ---\n>>   include/libcamera/ipa/ipu3.mojom       |  36 ++-----\n>>   src/ipa/ipu3/ipu3-ipa-design-guide.rst |  27 ++---\n>>   src/ipa/ipu3/ipu3.cpp                  | 132 +++++++++++--------------\n>>   src/libcamera/pipeline/ipu3/ipu3.cpp   | 122 ++++++++++-------------\n>>   4 files changed, 132 insertions(+), 185 deletions(-)\n>>\n>> diff --git a/include/libcamera/ipa/ipu3.mojom b/include/libcamera/ipa/ipu3.mojom\n>> index 18cdc744..b1e06e65 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<libcamera.IPABuffer> buffers);\n>>   \tunmapBuffers(array<uint32> ids);\n>>   \n>> -\t[async] processEvent(IPU3Event ev);\n>> +\t[async] fillParamsBuffer(uint32 frame, uint32 bufferId);\n>> +\t[async] processStatsBuffer(uint32 frame, int64 frameTimestamp,\n>> +\t\t\t\t   uint32 bufferId, libcamera.ControlList sensorControls);\n>> +\t[async] queueRequest(uint32 frame, libcamera.ControlList controls);\n> Nitpicking, I would have put queueRequest() first, to match the logical\n> order in which the operations will be called.\n\n\nOkay.\n\n>\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..13682599 100644\n>> --- a/src/ipa/ipu3/ipu3-ipa-design-guide.rst\n>> +++ b/src/ipa/ipu3/ipu3-ipa-design-guide.rst\n>> @@ -25,7 +25,8 @@ from applications, and managing events from the pipeline handler.\n>>         └─┬───┬───┬──────┬────┬────┬────┬─┴────▼─┬──┘    1: init()\n>>           │   │   │      │ ▲  │ ▲  │ ▲  │ ▲      │       2: configure()\n>>           │1  │2  │3     │4│  │4│  │4│  │4│      │5      3: mapBuffers(), start()\n>> -        ▼   ▼   ▼      ▼ │  ▼ │  ▼ │  ▼ │      ▼       4: processEvent()\n>> +        │   │   │      │ │  │ │  │ │  │ │      │       4: (▼) queueRequest(), fillParamsBuffer(), processStatsBuffer()\n>> +        ▼   ▼   ▼      ▼ │  ▼ │  ▼ │  ▼ │      ▼          (▲) setSensorControls, paramsBufferReady, metadataReady Signals\n>>         ┌──────────────────┴────┴────┴────┴─────────┐    5: stop(), unmapBuffers()\n>>         │ IPU3 IPA                                  │\n>>         │                 ┌───────────────────────┐ │\n>> @@ -100,8 +101,9 @@ There are three main interactions with the algorithms for the IPU3 IPA\n>>   to operate when running:\n>>   \n>>   -  configure()\n>> --  processEvent(``EventFillParams``)\n>> --  processEvent(``EventStatReady``)\n>> +-  fillParamsBuffer()\n>> +-  queueRequest()\n>> +-  processStatsBuffer()\n> Same here, same order as in the mojom file. And same in the\n> implementation.\n>\n>>   \n>>   The configuration phase allows the pipeline-handler to inform the IPA of\n>>   the current stream configurations, which is then passed into each\n>> @@ -114,8 +116,8 @@ Pre-frame preparation\n>>   When configured, the IPA is notified by the pipeline handler of the\n>>   Camera ``start()`` event, after which incoming requests will be queued\n>>   for processing, requiring a parameter buffer (``ipu3_uapi_params``) to\n>> -be populated for the ImgU. This is given to the IPA through the\n>> -``EventFillParams`` event, and then passed directly to each algorithm\n>> +be populated for the ImgU. This is given to the IPA through\n>> +``fillParamsBuffer()``, and then passed directly to each algorithm\n>>   through the ``prepare()`` call allowing the ISP configuration to be\n>>   updated for the needs of each component that the algorithm is\n>>   responsible for.\n>> @@ -125,7 +127,7 @@ structure that it modifies, and it should take care to ensure that any\n>>   structure set by a use flag is fully initialised to suitable values.\n>>   \n>>   The parameter buffer is returned to the pipeline handler through the\n>> -``ActionParamFilled`` event, and from there queued to the ImgU along\n>> +``paramsBufferReady`` signal, and from there queued to the ImgU along\n>>   with a raw frame captured with the CIO2.\n>>   \n>>   Post-frame completion\n>> @@ -133,8 +135,8 @@ Post-frame completion\n>>   \n>>   When the capture of an image is completed, and successfully processed\n>>   through the ImgU, the generated statistics buffer\n>> -(``ipu3_uapi_stats_3a``) is given to the IPA through the\n>> -``EventStatReady`` event. This provides the IPA with an opportunity to\n>> +(``ipu3_uapi_stats_3a``) is given to the IPA through\n>> +``processStatsBuffer()``. This provides the IPA with an opportunity to\n>>   examine the results of the ISP and run the calculations required by each\n>>   algorithm on the new data. The algorithms may require context from the\n>>   operations of other algorithms, for example, the AWB might choose to use\n>> @@ -145,11 +147,14 @@ before they are needed.\n>>   The ordering of the algorithm processing is determined by their\n>>   placement in the ``IPU3::algorithms_`` ordered list.\n>>   \n>> +Finally, the IPA metadata for the completed frame is returned back via\n>> +the ``metadataReady`` signal.\n>> +\n>>   Sensor Controls\n>>   ~~~~~~~~~~~~~~~\n>>   \n>>   The AutoExposure and AutoGain (AGC) algorithm differs slightly from the\n>>   others as it requires operating directly on the sensor, as opposed to\n>> -through the ImgU ISP. To support this, there is a dedicated action\n>> -`ActionSetSensorControls` to allow the IPA to request controls to be set\n>> -on the camera sensor through the pipeline handler.\n>> +through the ImgU ISP. To support this, there is a ``setSensorControls``\n>> +signal to allow the IPA to request controls to be set on the camera\n>> +sensor through the pipeline handler.\n>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n>> index 50b52d8b..7779ad58 100644\n>> --- a/src/ipa/ipu3/ipu3.cpp\n>> +++ b/src/ipa/ipu3/ipu3.cpp\n>> @@ -65,8 +65,9 @@ namespace ipa::ipu3 {\n>>    * The IPU3 Pipeline defines an IPU3-specific interface for communication\n>>    * between the PipelineHandler and the IPA module.\n>>    *\n>> - * We extend the IPAIPU3Interface to implement our algorithms and handle events\n>> - * from the IPU3 PipelineHandler to satisfy requests from the application.\n>> + * We extend the IPAIPU3Interface to implement our algorithms and handle\n>> + * calls from the IPU3 PipelineHandler to satisfy requests from the\n>> + * application.\n>>    *\n>>    * At initialisation time, a CameraSensorHelper is instantiated to support\n>>    * camera-specific calculations, while the default controls are computed, and\n>> @@ -81,14 +82,14 @@ namespace ipa::ipu3 {\n>>    * parameter buffer, and adapting the settings of the sensor attached to the\n>>    * IPU3 CIO2 through sensor-specific V4L2 controls.\n>>    *\n>> - * When the event \\a EventFillParams occurs we populate the ImgU parameter\n>> - * buffer with settings to configure the device in preparation for handling the\n>> - * frame queued in the Request.\n>> + * In fillParamsBuffer(), we populate the ImgU parameter buffer with\n>> + * settings to configure the device in preparation for handling the frame\n>> + * queued in the Request.\n>>    *\n>>    * When the frame has completed processing, the ImgU will generate a statistics\n>> - * buffer which is given to the IPA as part of the \\a EventStatReady event. At\n>> - * this event we run the algorithms to parse the statistics and cache any\n>> - * results for the next \\a EventFillParams event.\n>> + * buffer which is given to the IPA with processStatsBuffer(). In this we run the\n>> + * algorithms to parse the statistics and cache any results for the next\n>> + * fillParamsBuffer() function.\n> s/function/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,19 @@ public:\n>>   \n>>   \tvoid mapBuffers(const std::vector<IPABuffer> &buffers) override;\n>>   \tvoid unmapBuffers(const std::vector<unsigned int> &ids) override;\n>> -\tvoid processEvent(const IPU3Event &event) override;\n>> +\n>> +\tvoid fillParamsBuffer(const uint32_t frame, const uint32_t bufferId) override;\n>> +\tvoid processStatsBuffer(const uint32_t frame, const int64_t frameTimestamp,\n>> +\t\t\t\tconst uint32_t bufferId,\n>> +\t\t\t\tconst ControlList &sensorControls) override;\n>> +\tvoid queueRequest(const uint32_t frame, const ControlList &controls) override;\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,73 +511,61 @@ void IPAIPU3::unmapBuffers(const std::vector<unsigned int> &ids)\n>>   }\n>>   \n>>   /**\n>> - * \\brief Process an event generated by the pipeline handler\n>> - * \\param[in] event The event sent from pipeline handler\n>> - *\n>> - * The expected event handling over the lifetime of a Request has\n>> - * the following sequence:\n>> - *\n>> - *   - EventProcessControls : Handle controls from a new Request\n>> - *   - EventFillParams : Prepare the ISP to process the Request\n>> - *   - EventStatReady : Process statistics after ISP completion\n>> + * \\brief Fill and return a buffer with ISP processing parameters for a frame\n>> + * \\param[in] frame The frame number\n>> + * \\param[in] bufferId ID of the parameter buffer to fill\n>>    */\n>> -void IPAIPU3::processEvent(const IPU3Event &event)\n>> +void IPAIPU3::fillParamsBuffer(const uint32_t frame, const uint32_t bufferId)\n>>   {\n>> -\tswitch (event.op) {\n>> -\tcase EventProcessControls: {\n>> -\t\tprocessControls(event.frame, event.controls);\n>> -\t\tbreak;\n>> +\tauto it = buffers_.find(bufferId);\n>> +\tif (it == buffers_.end()) {\n>> +\t\tLOG(IPAIPU3, Error) << \"Could not find param buffer!\";\n>> +\t\treturn;\n>>   \t}\n>> -\tcase EventFillParams: {\n>> -\t\tauto it = buffers_.find(event.bufferId);\n>> -\t\tif (it == buffers_.end()) {\n>> -\t\t\tLOG(IPAIPU3, Error) << \"Could not find param buffer!\";\n>> -\t\t\treturn;\n>> -\t\t}\n>>   \n>> -\t\tSpan<uint8_t> mem = it->second.planes()[0];\n>> -\t\tipu3_uapi_params *params =\n>> -\t\t\treinterpret_cast<ipu3_uapi_params *>(mem.data());\n>> +\tSpan<uint8_t> mem = it->second.planes()[0];\n>> +\tipu3_uapi_params *params =\n>> +\t\treinterpret_cast<ipu3_uapi_params *>(mem.data());\n>>   \n>> -\t\tfillParams(event.frame, params);\n>> -\t\tbreak;\n>> -\t}\n>> -\tcase EventStatReady: {\n>> -\t\tauto it = buffers_.find(event.bufferId);\n>> -\t\tif (it == buffers_.end()) {\n>> -\t\t\tLOG(IPAIPU3, Error) << \"Could not find stats buffer!\";\n>> -\t\t\treturn;\n>> -\t\t}\n>> +\tfillParams(frame, params);\n>> +}\n>>   \n>> -\t\tSpan<uint8_t> mem = it->second.planes()[0];\n>> -\t\tconst ipu3_uapi_stats_3a *stats =\n>> -\t\t\treinterpret_cast<ipu3_uapi_stats_3a *>(mem.data());\n>> +/**\n>> + * \\brief Process statistics after ISP completion\n>> + * \\param[in] frame The frame number\n>> + * \\param[in] frameTimestamp Timestamp of the frame\n>> + * \\param[in] bufferId ID of the statistics buffer\n>> + * \\param[in] sensorControls Sensor controls\n>> + */\n>> +void IPAIPU3::processStatsBuffer(const uint32_t frame, const int64_t frameTimestamp,\n>> +\t\t\t\t const uint32_t bufferId, const ControlList &sensorControls)\n>> +{\n>> +\tauto it = buffers_.find(bufferId);\n>> +\tif (it == buffers_.end()) {\n>> +\t\tLOG(IPAIPU3, Error) << \"Could not find stats buffer!\";\n>> +\t\treturn;\n>> +\t}\n>>   \n>> -\t\tint32_t exposure = event.sensorControls.get(V4L2_CID_EXPOSURE).get<int32_t>();\n>> -\t\tint32_t gain = event.sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>();\n>> +\tSpan<uint8_t> mem = it->second.planes()[0];\n>> +\tconst ipu3_uapi_stats_3a *stats =\n>> +\t\treinterpret_cast<ipu3_uapi_stats_3a *>(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<int32_t>();\n>> +\tcontext_.frameContext.sensor.gain = camHelper_->gain(sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>());\n>>   \n>> -\t\tparseStatistics(event.frame, event.frameTimestamp, stats);\n>> -\t\tbreak;\n>> -\t}\n>> -\tdefault:\n>> -\t\tLOG(IPAIPU3, Error) << \"Unknown event \" << event.op;\n>> -\t\tbreak;\n>> -\t}\n>> +\tparseStatistics(frame, frameTimestamp, stats);\n>>   }\n>>   \n>>   /**\n>> - * \\brief Process a control list for a request from the application\n>> + * \\brief Queue a request and process the control list from the application\n>>    * \\param[in] frame The number of the frame which will be processed next\n>>    * \\param[in] controls The controls for the \\a frame\n>>    *\n>>    * Parse the request to handle any IPA-managed controls that were set from the\n>>    * application such as manual sensor settings.\n>>    */\n>> -void IPAIPU3::processControls([[maybe_unused]] unsigned int frame,\n>> -\t\t\t      [[maybe_unused]] const ControlList &controls)\n>> +void IPAIPU3::queueRequest(const uint32_t frame,\n>> +\t\t\t   [[maybe_unused]] const ControlList &controls)\n>>   {\n>>   \t/* \\todo Start processing for 'frame' based on 'controls'. */\n>>   }\n>> @@ -600,10 +594,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 +638,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 +650,18 @@ void IPAIPU3::parseStatistics(unsigned int frame,\n>>    */\n>>   void IPAIPU3::setControls(unsigned int frame)\n>>   {\n>> -\tIPU3Action op;\n>> -\top.op = ActionSetSensorControls;\n>> -\n>>   \tint32_t exposure = context_.frameContext.agc.exposure;\n>>   \tint32_t gain = camHelper_->gainCode(context_.frameContext.agc.gain);\n>>   \n>>   \tControlList ctrls(sensorCtrls_);\n>>   \tctrls.set(V4L2_CID_EXPOSURE, exposure);\n>>   \tctrls.set(V4L2_CID_ANALOGUE_GAIN, gain);\n>> -\top.sensorControls = ctrls;\n>>   \n>> -\tControlList lensCtrls(lensCtrls_);\n>> +\tControlList lensCtrls(sensorCtrls_);\n> Is this right ?\n\n\nerr, it's seems not! Thank for catching this.\n\n>\n> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n>\n>>   \tlensCtrls.set(V4L2_CID_FOCUS_ABSOLUTE,\n>>   \t\t      static_cast<int32_t>(context_.frameContext.af.focus));\n>> -\top.lensControls = lensCtrls;\n>>   \n>> -\tqueueFrameAction.emit(frame, op);\n>> +\tsetSensorControls.emit(frame, ctrls, lensCtrls);\n>>   }\n>>   \n>>   } /* namespace ipa::ipu3 */\n>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n>> index 60e01917..59d7e9c0 100644\n>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n>> @@ -86,8 +86,10 @@ public:\n>>   \tControlInfoMap ipaControls_;\n>>   \n>>   private:\n>> -\tvoid queueFrameAction(unsigned int id,\n>> -\t\t\t      const ipa::ipu3::IPU3Action &action);\n>> +\tvoid metadataReady(unsigned int id, const ControlList &metadata);\n>> +\tvoid paramsFilled(unsigned int id);\n>> +\tvoid setSensorControls(unsigned int id, const ControlList &sensorControls,\n>> +\t\t\t       const ControlList &lensControls);\n>>   };\n>>   \n>>   class IPU3CameraConfiguration : public CameraConfiguration\n>> @@ -871,11 +873,7 @@ void IPU3CameraData::queuePendingRequests()\n>>   \n>>   \t\tinfo->rawBuffer = rawBuffer;\n>>   \n>> -\t\tipa::ipu3::IPU3Event ev;\n>> -\t\tev.op = ipa::ipu3::EventProcessControls;\n>> -\t\tev.frame = info->id;\n>> -\t\tev.controls = request->controls();\n>> -\t\tipa_->processEvent(ev);\n>> +\t\tipa_->queueRequest(info->id, request->controls());\n>>   \n>>   \t\tpendingRequests_.pop();\n>>   \t\tprocessingRequests_.push(request);\n>> @@ -1218,7 +1216,9 @@ int IPU3CameraData::loadIPA()\n>>   \tif (!ipa_)\n>>   \t\treturn -ENOENT;\n>>   \n>> -\tipa_->queueFrameAction.connect(this, &IPU3CameraData::queueFrameAction);\n>> +\tipa_->setSensorControls.connect(this, &IPU3CameraData::setSensorControls);\n>> +\tipa_->paramsBufferReady.connect(this, &IPU3CameraData::paramsFilled);\n>> +\tipa_->metadataReady.connect(this, &IPU3CameraData::metadataReady);\n>>   \n>>   \t/*\n>>   \t * Pass the sensor info to the IPA to initialize controls.\n>> @@ -1253,69 +1253,58 @@ int IPU3CameraData::loadIPA()\n>>   \treturn 0;\n>>   }\n>>   \n>> -void IPU3CameraData::queueFrameAction(unsigned int id,\n>> -\t\t\t\t      const ipa::ipu3::IPU3Action &action)\n>> +void IPU3CameraData::setSensorControls([[maybe_unused]] unsigned int id,\n>> +\t\t\t\t       const ControlList &sensorControls,\n>> +\t\t\t\t       const ControlList &lensControls)\n>>   {\n>> -\tswitch (action.op) {\n>> -\tcase ipa::ipu3::ActionSetSensorControls: {\n>> -\t\tconst ControlList &sensorControls = action.sensorControls;\n>> -\t\tdelayedCtrls_->push(sensorControls);\n>> +\tdelayedCtrls_->push(sensorControls);\n>>   \n>> -\t\tCameraLens *focusLens = cio2_.sensor()->focusLens();\n>> -\t\tif (!focusLens)\n>> -\t\t\tbreak;\n>> -\n>> -\t\tconst ControlList lensControls = action.lensControls;\n>> -\t\tif (!lensControls.contains(V4L2_CID_FOCUS_ABSOLUTE))\n>> -\t\t\tbreak;\n>> +\tCameraLens *focusLens = cio2_.sensor()->focusLens();\n>> +\tif (!focusLens)\n>> +\t\treturn;\n>>   \n>> -\t\tconst ControlValue &focusValue =\n>> -\t\t\tlensControls.get(V4L2_CID_FOCUS_ABSOLUTE);\n>> +\tif (!lensControls.contains(V4L2_CID_FOCUS_ABSOLUTE))\n>> +\t\treturn;\n>>   \n>> -\t\tfocusLens->setFocusPosition(focusValue.get<int32_t>());\n>> +\tconst ControlValue &focusValue = lensControls.get(V4L2_CID_FOCUS_ABSOLUTE);\n>>   \n>> -\t\tbreak;\n>> -\t}\n>> -\tcase ipa::ipu3::ActionParamFilled: {\n>> -\t\tIPU3Frames::Info *info = frameInfos_.find(id);\n>> -\t\tif (!info)\n>> -\t\t\tbreak;\n>> -\n>> -\t\t/* Queue all buffers from the request aimed for the ImgU. */\n>> -\t\tfor (auto it : info->request->buffers()) {\n>> -\t\t\tconst Stream *stream = it.first;\n>> -\t\t\tFrameBuffer *outbuffer = it.second;\n>> +\tfocusLens->setFocusPosition(focusValue.get<int32_t>());\n>> +}\n>>   \n>> -\t\t\tif (stream == &outStream_)\n>> -\t\t\t\timgu_->output_->queueBuffer(outbuffer);\n>> -\t\t\telse if (stream == &vfStream_)\n>> -\t\t\t\timgu_->viewfinder_->queueBuffer(outbuffer);\n>> -\t\t}\n>> +void IPU3CameraData::paramsFilled(unsigned int id)\n>> +{\n>> +\tIPU3Frames::Info *info = frameInfos_.find(id);\n>> +\tif (!info)\n>> +\t\treturn;\n>>   \n>> -\t\timgu_->param_->queueBuffer(info->paramBuffer);\n>> -\t\timgu_->stat_->queueBuffer(info->statBuffer);\n>> -\t\timgu_->input_->queueBuffer(info->rawBuffer);\n>> +\t/* Queue all buffers from the request aimed for the ImgU. */\n>> +\tfor (auto it : info->request->buffers()) {\n>> +\t\tconst Stream *stream = it.first;\n>> +\t\tFrameBuffer *outbuffer = it.second;\n>>   \n>> -\t\tbreak;\n>> +\t\tif (stream == &outStream_)\n>> +\t\t\timgu_->output_->queueBuffer(outbuffer);\n>> +\t\telse if (stream == &vfStream_)\n>> +\t\t\timgu_->viewfinder_->queueBuffer(outbuffer);\n>>   \t}\n>> -\tcase ipa::ipu3::ActionMetadataReady: {\n>> -\t\tIPU3Frames::Info *info = frameInfos_.find(id);\n>> -\t\tif (!info)\n>> -\t\t\tbreak;\n>>   \n>> -\t\tRequest *request = info->request;\n>> -\t\trequest->metadata().merge(action.controls);\n>> +\timgu_->param_->queueBuffer(info->paramBuffer);\n>> +\timgu_->stat_->queueBuffer(info->statBuffer);\n>> +\timgu_->input_->queueBuffer(info->rawBuffer);\n>> +}\n>>   \n>> -\t\tinfo->metadataProcessed = true;\n>> -\t\tif (frameInfos_.tryComplete(info))\n>> -\t\t\tpipe()->completeRequest(request);\n>> +void IPU3CameraData::metadataReady(unsigned int id, const ControlList &metadata)\n>> +{\n>> +\tIPU3Frames::Info *info = frameInfos_.find(id);\n>> +\tif (!info)\n>> +\t\treturn;\n>>   \n>> -\t\tbreak;\n>> -\t}\n>> -\tdefault:\n>> -\t\tLOG(IPU3, Error) << \"Unknown action \" << action.op;\n>> -\t\tbreak;\n>> -\t}\n>> +\tRequest *request = info->request;\n>> +\trequest->metadata().merge(metadata);\n>> +\n>> +\tinfo->metadataProcessed = true;\n>> +\tif (frameInfos_.tryComplete(info))\n>> +\t\tpipe()->completeRequest(request);\n>>   }\n>>   \n>>   /* -----------------------------------------------------------------------------\n>> @@ -1390,11 +1379,7 @@ void IPU3CameraData::cio2BufferReady(FrameBuffer *buffer)\n>>   \tif (request->findBuffer(&rawStream_))\n>>   \t\tpipe()->completeBuffer(request, buffer);\n>>   \n>> -\tipa::ipu3::IPU3Event ev;\n>> -\tev.op = ipa::ipu3::EventFillParams;\n>> -\tev.frame = info->id;\n>> -\tev.bufferId = info->paramBuffer->cookie();\n>> -\tipa_->processEvent(ev);\n>> +\tipa_->fillParamsBuffer(info->id, info->paramBuffer->cookie());\n>>   }\n>>   \n>>   void IPU3CameraData::paramBufferReady(FrameBuffer *buffer)\n>> @@ -1438,13 +1423,8 @@ void IPU3CameraData::statBufferReady(FrameBuffer *buffer)\n>>   \t\treturn;\n>>   \t}\n>>   \n>> -\tipa::ipu3::IPU3Event ev;\n>> -\tev.op = ipa::ipu3::EventStatReady;\n>> -\tev.frame = info->id;\n>> -\tev.bufferId = info->statBuffer->cookie();\n>> -\tev.frameTimestamp = request->metadata().get(controls::SensorTimestamp);\n>> -\tev.sensorControls = info->effectiveSensorControls;\n>> -\tipa_->processEvent(ev);\n>> +\tipa_->processStatsBuffer(info->id, request->metadata().get(controls::SensorTimestamp),\n>> +\t\t\t\t info->statBuffer->cookie(), info->effectiveSensorControls);\n>>   }\n>>   \n>>   /*","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 7CB3CC0F1B\n\tfor <parsemail@patchwork.libcamera.org>;\n\tWed,  6 Apr 2022 09:34:10 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B61AD65642;\n\tWed,  6 Apr 2022 11:34:09 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2D911604B8\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed,  6 Apr 2022 11:34:08 +0200 (CEST)","from [192.168.1.110] (unknown [27.57.186.178])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id CAB51482;\n\tWed,  6 Apr 2022 11:34:06 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1649237649;\n\tbh=ZtY0/x9AZfC2IDLTIsF/ZbdepuamU8sj1HjQTRAp2AU=;\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=ulyjrbW+RvpFMxRhyRX18SVlgz/WeXGqmKmozxZyIsJRFvHUR7UWTv7603DXNxpsr\n\t6WqoWYUw6xzfqXoYJhPANd/WuYa+K0kXSjFhchwLhs/W4dvb2+pjGq8l+vv4+fqvMS\n\tJKKu3gVreYU3MfSTmmZU+wZJ/9ZD39x/pOosy/Aw+HW2syTr7yw6RBhYJxQQhAdpHO\n\t7xcviF2xWBjSUQNiOpTJJbTyZorNkToRMX+TtHtmd/qiCUaAy9QIFroLqIypl2Hv5G\n\tH0nf4REN1bbBAvX/Spt9CgzGmj3U059GrPC6Dy+SqoZ1nm8x6RIPzrBngPIBTsliP8\n\tFwzxvPbkHwvDA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1649237647;\n\tbh=ZtY0/x9AZfC2IDLTIsF/ZbdepuamU8sj1HjQTRAp2AU=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=n49Ou6Lcp2q9T318bX+Xh7xEBc1Z4w1rznTGirtZo6+7jbXfKy8E/VVGgI46DPyAt\n\tpBVl5HSvVYy+C6Ay7IkfimYz1ngGlAEI7IeN2sn9M4JrQzvqMgptap0SJavzYMc28U\n\tvZnxizf1me6V11p+ulD8jbQXGTqqSIjb5UfTg46Q="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"n49Ou6Lc\"; dkim-atps=neutral","Message-ID":"<dcba5efa-5663-9714-743f-6033f277cddc@ideasonboard.com>","Date":"Wed, 6 Apr 2022 15:03:59 +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 <laurent.pinchart@ideasonboard.com>","References":"<20220331163058.171418-1-umang.jain@ideasonboard.com>\n\t<20220331163058.171418-2-umang.jain@ideasonboard.com>\n\t<YkzIAzGiEHVVSgV7@pendragon.ideasonboard.com>","In-Reply-To":"<YkzIAzGiEHVVSgV7@pendragon.ideasonboard.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH v4 1/6] ipa: ipu3: Replace event-based\n\tops with dedicated functions","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","From":"Umang Jain via libcamera-devel <libcamera-devel@lists.libcamera.org>","Reply-To":"Umang Jain <umang.jain@ideasonboard.com>","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]