[{"id":22200,"web_url":"https://patchwork.libcamera.org/comment/22200/","msgid":"<164605715969.2091767.17066819612604019589@Monstersaurus>","date":"2022-02-28T14:05:59","subject":"Re: [libcamera-devel] [PATCH 1/2] ipa: ipu3: Replace event-based\n\toperations with dedicated functions","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Umang,\n\nThanks for tackling this. I think this has been something that was\ndiscussed for a long time.\n\nThis will also need a corresponding patch for the out of tree Intel IPU3\nIPA. Can you prepare that to post with any next version of this please?\n\n(Make sure the IPU3IPA patch is marked perhaps as [PATCH IPU3IPA x/y],\nbut I think it could be in the same series for review purposes, but not\napplied to libcamera of course)\n\nI don't think I can see anything beyond some minor comments and\nsuggestions for a patch on top below, but it would have been easier to\nreview if each event/op was a patch on it's own. That could have been\ndone and still been bisectable, but probably isn't worth splitting now.\n\n\nQuoting Umang Jain (2022-02-28 13:22:49)\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> 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 |  19 ++--\n>  src/ipa/ipu3/ipu3.cpp                  | 116 ++++++++++-------------\n>  src/libcamera/pipeline/ipu3/ipu3.cpp   | 123 +++++++++++--------------\n>  4 files changed, 123 insertions(+), 171 deletions(-)\n> \n> diff --git a/include/libcamera/ipa/ipu3.mojom b/include/libcamera/ipa/ipu3.mojom\n> index cc0d822f..12b8bb2f 100644\n> --- a/include/libcamera/ipa/ipu3.mojom\n> +++ b/include/libcamera/ipa/ipu3.mojom\n> @@ -8,32 +8,6 @@ module ipa.ipu3;\n>  \n>  import \"include/libcamera/ipa/core.mojom\";\n>  \n> -enum IPU3Operations {\n> -       ActionSetSensorControls = 1,\n> -       ActionParamFilled = 2,\n> -       ActionMetadataReady = 3,\n> -       EventProcessControls = 4,\n> -       EventStatReady = 5,\n> -       EventFillParams = 6,\n> -};\n> -\n> -struct IPU3Event {\n> -       IPU3Operations op;\n> -       uint32 frame;\n> -       int64 frameTimestamp;\n> -       uint32 bufferId;\n> -       libcamera.ControlList controls;\n> -       libcamera.ControlList sensorControls;\n> -       libcamera.ControlList lensControls;\n> -};\n> -\n> -struct IPU3Action {\n> -       IPU3Operations op;\n> -       libcamera.ControlList controls;\n> -       libcamera.ControlList sensorControls;\n> -       libcamera.ControlList lensControls;\n> -};\n> -\n>  struct IPAConfigInfo {\n>         libcamera.IPACameraSensorInfo sensorInfo;\n>         libcamera.ControlInfoMap sensorControls;\n> @@ -55,9 +29,15 @@ interface IPAIPU3Interface {\n>         mapBuffers(array<libcamera.IPABuffer> buffers);\n>         unmapBuffers(array<uint32> ids);\n>  \n> -       [async] processEvent(IPU3Event ev);\n> +       [async] fillParameterBuffer(uint32 frame, uint32 bufferId);\n> +       [async] processControls(uint32 frame, libcamera.ControlList controls);\n> +       [async] statsReady(uint32 frame, int64 frameTimestamp, uint32 bufferId,\n> +                          libcamera.ControlList sensorControls);\n>  };\n>  \n>  interface IPAIPU3EventInterface {\n> -       queueFrameAction(uint32 frame, IPU3Action action);\n> +       setSensorControls(uint32 frame, libcamera.ControlList sensorControls,\n> +                         libcamera.ControlList lensControls);\n> +       paramFilled(uint32 frame);\n> +       metadataReady(uint32 frame, libcamera.ControlList metadata);\n>  };\n> diff --git a/src/ipa/ipu3/ipu3-ipa-design-guide.rst b/src/ipa/ipu3/ipu3-ipa-design-guide.rst\n> index 89c71108..83f8634d 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(), fillParametersBuffer(), statsReady()\n> +        ▼   ▼   ▼      ▼ │  ▼ │  ▼ │  ▼ │      ▼          (▲) setSensorControls, paramFilled, 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> +-  fillParameterBuffer()\n> +-  processControls()\n> +-  statsReady()\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> -``EventFillParams`` event, and then passed directly to each algorithm\n> +``fillParameterBuffer()``, 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> +``paramFilled`` 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> +``statsReady()``. 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 frame completed is returned back via\n\ns/frame completed/completed frame/ ?\n\nalso missing 'the':\n\nreturned back via the ``metadataReady``\n                  ^^^\n\n> +``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> +``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 3d307708..2fab4ee0 100644\n> --- a/src/ipa/ipu3/ipu3.cpp\n> +++ b/src/ipa/ipu3/ipu3.cpp\n> @@ -142,14 +142,18 @@ public:\n>  \n>         void mapBuffers(const std::vector<IPABuffer> &buffers) override;\n>         void unmapBuffers(const std::vector<unsigned int> &ids) override;\n> -       void processEvent(const IPU3Event &event) override;\n> +\n> +       void fillParameterBuffer(const uint32_t frame, const uint32_t bufferId) override;\n> +       void processControls(const uint32_t frame, const ControlList &controls) override;\n> +       void statsReady(const uint32_t frame, const int64_t frameTimestamp,\n> +                       const uint32_t bufferId, const ControlList &sensorControls) override;\n>  \n>  private:\n>         void updateControls(const IPACameraSensorInfo &sensorInfo,\n>                             const ControlInfoMap &sensorControls,\n>                             ControlInfoMap *ipaControls);\n>         void updateSessionConfiguration(const ControlInfoMap &sensorControls);\n> -       void processControls(unsigned int frame, const ControlList &controls);\n> +\n>         void fillParams(unsigned int frame, ipu3_uapi_params *params);\n>         void parseStatistics(unsigned int frame,\n>                              int64_t frameTimestamp,\n> @@ -505,58 +509,49 @@ 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 Prepare the ISP to process the Request\n> + * \\param[in] frame The frame number\n> + * \\param[in] bufferId Buffer ID\n>   */\n> -void IPAIPU3::processEvent(const IPU3Event &event)\n> +void IPAIPU3::fillParameterBuffer(const uint32_t frame, const uint32_t bufferId)\n>  {\n> -       switch (event.op) {\n> -       case EventProcessControls: {\n> -               processControls(event.frame, event.controls);\n> -               break;\n> -       }\n> -       case EventFillParams: {\n> -               auto it = buffers_.find(event.bufferId);\n> -               if (it == buffers_.end()) {\n> -                       LOG(IPAIPU3, Error) << \"Could not find param buffer!\";\n> -                       return;\n> -               }\n> -\n> -               Span<uint8_t> mem = it->second.planes()[0];\n> -               ipu3_uapi_params *params =\n> -                       reinterpret_cast<ipu3_uapi_params *>(mem.data());\n> -\n> -               fillParams(event.frame, params);\n> -               break;\n> +       auto it = buffers_.find(bufferId);\n> +       if (it == buffers_.end()) {\n> +               LOG(IPAIPU3, Error) << \"Could not find param buffer!\";\n> +               return;\n>         }\n> -       case EventStatReady: {\n> -               auto it = buffers_.find(event.bufferId);\n> -               if (it == buffers_.end()) {\n> -                       LOG(IPAIPU3, Error) << \"Could not find stats buffer!\";\n> -                       return;\n> -               }\n> -\n> -               Span<uint8_t> mem = it->second.planes()[0];\n> -               const ipu3_uapi_stats_3a *stats =\n> -                       reinterpret_cast<ipu3_uapi_stats_3a *>(mem.data());\n> -\n> -               context_.frameContext.sensor.exposure = event.sensorControls.get(V4L2_CID_EXPOSURE).get<int32_t>();\n> -               context_.frameContext.sensor.gain = camHelper_->gain(event.sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>());\n> -\n> -               parseStatistics(event.frame, event.frameTimestamp, stats);\n> -               break;\n> -       }\n> -       default:\n> -               LOG(IPAIPU3, Error) << \"Unknown event \" << event.op;\n> -               break;\n> +\n> +       Span<uint8_t> mem = it->second.planes()[0];\n> +       ipu3_uapi_params *params =\n> +               reinterpret_cast<ipu3_uapi_params *>(mem.data());\n> +\n> +       fillParams(frame, params);\n> +}\n> +\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> + * \\param[in] sensorControls Sensor controls\n> + */\n> +void IPAIPU3::statsReady(const uint32_t frame, const int64_t frameTimestamp,\n> +                        const uint32_t bufferId, const ControlList &sensorControls)\n> +{\n> +       auto it = buffers_.find(bufferId);\n> +       if (it == buffers_.end()) {\n> +               LOG(IPAIPU3, Error) << \"Could not find stats buffer!\";\n> +               return;\n>         }\n> +\n> +       Span<uint8_t> mem = it->second.planes()[0];\n> +       const ipu3_uapi_stats_3a *stats =\n> +               reinterpret_cast<ipu3_uapi_stats_3a *>(mem.data());\n> +\n> +       context_.frameContext.sensor.exposure = sensorControls.get(V4L2_CID_EXPOSURE).get<int32_t>();\n> +       context_.frameContext.sensor.gain = camHelper_->gain(sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>());\n> +\n> +       parseStatistics(frame, frameTimestamp, stats);\n>  }\n>  \n>  /**\n> @@ -567,7 +562,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>                               [[maybe_unused]] const ControlList &controls)\n>  {\n>         /* \\todo Start processing for 'frame' based on 'controls'. */\n> @@ -597,10 +592,7 @@ void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params)\n>         for (auto const &algo : algorithms_)\n>                 algo->prepare(context_, params);\n>  \n> -       IPU3Action op;\n> -       op.op = ActionParamFilled;\n> -\n> -       queueFrameAction.emit(frame, op);\n> +       paramFilled.emit(frame);\n>  }\n>  \n>  /**\n> @@ -642,11 +634,7 @@ void IPAIPU3::parseStatistics(unsigned int frame,\n>          * likely want to avoid putting platform specific metadata in.\n>          */\n>  \n> -       IPU3Action op;\n> -       op.op = ActionMetadataReady;\n> -       op.controls = ctrls;\n> -\n> -       queueFrameAction.emit(frame, op);\n> +       metadataReady.emit(frame, ctrls);\n>  }\n>  \n>  /**\n> @@ -658,18 +646,16 @@ void IPAIPU3::parseStatistics(unsigned int frame,\n>   */\n>  void IPAIPU3::setControls(unsigned int frame)\n>  {\n> -       IPU3Action op;\n> -       op.op = ActionSetSensorControls;\n> -\n>         exposure_ = context_.frameContext.agc.exposure;\n>         gain_ = camHelper_->gainCode(context_.frameContext.agc.gain);\n\nThese look like they could/should be locals here in this function. Can\nyou check to see if they are used after this in other way?\n\n>  \n>         ControlList ctrls(ctrls_);\n>         ctrls.set(V4L2_CID_EXPOSURE, static_cast<int32_t>(exposure_));\n>         ctrls.set(V4L2_CID_ANALOGUE_GAIN, static_cast<int32_t>(gain_));\n\nAnd if they are locals, they can be int32_t, to remove/simplify this\ncasting.\n\nIt would be worthwhile doing this in a separate patch of course to avoid\nhiding the change in this larger code move. Perhaps on top after you've\ndone the functional moves.\n\n\n> -       op.sensorControls = ctrls;\n>  \n> -       queueFrameAction.emit(frame, op);\n> +       ControlList lensCtrls;\n\nA little redundant, but worth keeping for now as we know we have lens\ncontrols coming, so it can be handled then.\n\n> +\n> +       setSensorControls.emit(frame, ctrls, lensCtrls);\n>  }\n>  \n>  } /* namespace ipa::ipu3 */\n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index 6c5617cd..dd1780ef 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -86,8 +86,10 @@ public:\n>         ControlInfoMap ipaControls_;\n>  \n>  private:\n> -       void queueFrameAction(unsigned int id,\n> -                             const ipa::ipu3::IPU3Action &action);\n> +       void metadataReady(unsigned int id, const ControlList &metadata);\n> +       void paramFilled(unsigned int id);\n> +       void setSensorControls(unsigned int id, const ControlList &sensorControls,\n> +                              const ControlList &lensControls);\n>  };\n>  \n>  class IPU3CameraConfiguration : public CameraConfiguration\n> @@ -866,11 +868,7 @@ void IPU3CameraData::queuePendingRequests()\n>  \n>                 info->rawBuffer = rawBuffer;\n>  \n> -               ipa::ipu3::IPU3Event ev;\n> -               ev.op = ipa::ipu3::EventProcessControls;\n> -               ev.frame = info->id;\n> -               ev.controls = request->controls();\n> -               ipa_->processEvent(ev);\n> +               ipa_->processControls(info->id, request->controls());\n>  \n>                 pendingRequests_.pop();\n>                 processingRequests_.push(request);\n> @@ -1213,7 +1211,9 @@ int IPU3CameraData::loadIPA()\n>         if (!ipa_)\n>                 return -ENOENT;\n>  \n> -       ipa_->queueFrameAction.connect(this, &IPU3CameraData::queueFrameAction);\n> +       ipa_->setSensorControls.connect(this, &IPU3CameraData::setSensorControls);\n> +       ipa_->paramFilled.connect(this, &IPU3CameraData::paramFilled);\n> +       ipa_->metadataReady.connect(this, &IPU3CameraData::metadataReady);\n>  \n>         /*\n>          * Pass the sensor info to the IPA to initialize controls.\n> @@ -1248,69 +1248,59 @@ int IPU3CameraData::loadIPA()\n>         return 0;\n>  }\n>  \n> -void IPU3CameraData::queueFrameAction(unsigned int id,\n> -                                     const ipa::ipu3::IPU3Action &action)\n> +void IPU3CameraData::setSensorControls([[maybe_unused]] unsigned int id,\n> +                                      const ControlList &sensorControls,\n> +                                      const ControlList &lensControls)\n>  {\n> -       switch (action.op) {\n> -       case ipa::ipu3::ActionSetSensorControls: {\n> -               const ControlList &sensorControls = action.sensorControls;\n> -               delayedCtrls_->push(sensorControls);\n> +       delayedCtrls_->push(sensorControls);\n>  \n> -               CameraLens *focusLens = cio2_.sensor()->focusLens();\n> -               if (!focusLens)\n> -                       break;\n> -\n> -               const ControlList lensControls = action.lensControls;\n> -               if (!lensControls.contains(V4L2_CID_FOCUS_ABSOLUTE))\n> -                       break;\n> +       CameraLens *focusLens = cio2_.sensor()->focusLens();\n> +       if (!focusLens)\n> +               return;\n>  \n> -               const ControlValue &focusValue =\n> -                       lensControls.get(V4L2_CID_FOCUS_ABSOLUTE);\n> +       if (!lensControls.contains(V4L2_CID_FOCUS_ABSOLUTE))\n> +               return;\n>  \n> -               focusLens->setFocusPostion(focusValue.get<int32_t>());\n> +       const ControlValue &focusValue =\n> +               lensControls.get(V4L2_CID_FOCUS_ABSOLUTE);\n>  \n> -               break;\n> -       }\n> -       case ipa::ipu3::ActionParamFilled: {\n> -               IPU3Frames::Info *info = frameInfos_.find(id);\n> -               if (!info)\n> -                       break;\n> -\n> -               /* Queue all buffers from the request aimed for the ImgU. */\n> -               for (auto it : info->request->buffers()) {\n> -                       const Stream *stream = it.first;\n> -                       FrameBuffer *outbuffer = it.second;\n> +       focusLens->setFocusPostion(focusValue.get<int32_t>());\n> +}\n>  \n> -                       if (stream == &outStream_)\n> -                               imgu_->output_->queueBuffer(outbuffer);\n> -                       else if (stream == &vfStream_)\n> -                               imgu_->viewfinder_->queueBuffer(outbuffer);\n> -               }\n> +void IPU3CameraData::paramFilled(unsigned int id)\n> +{\n> +       IPU3Frames::Info *info = frameInfos_.find(id);\n> +       if (!info)\n> +               return;\n>  \n> -               imgu_->param_->queueBuffer(info->paramBuffer);\n> -               imgu_->stat_->queueBuffer(info->statBuffer);\n> -               imgu_->input_->queueBuffer(info->rawBuffer);\n> +       /* Queue all buffers from the request aimed for the ImgU. */\n> +       for (auto it : info->request->buffers()) {\n> +               const Stream *stream = it.first;\n> +               FrameBuffer *outbuffer = it.second;\n>  \n> -               break;\n> +               if (stream == &outStream_)\n> +                       imgu_->output_->queueBuffer(outbuffer);\n> +               else if (stream == &vfStream_)\n> +                       imgu_->viewfinder_->queueBuffer(outbuffer);\n>         }\n> -       case ipa::ipu3::ActionMetadataReady: {\n> -               IPU3Frames::Info *info = frameInfos_.find(id);\n> -               if (!info)\n> -                       break;\n>  \n> -               Request *request = info->request;\n> -               request->metadata().merge(action.controls);\n> +       imgu_->param_->queueBuffer(info->paramBuffer);\n> +       imgu_->stat_->queueBuffer(info->statBuffer);\n> +       imgu_->input_->queueBuffer(info->rawBuffer);\n> +}\n>  \n> -               info->metadataProcessed = true;\n> -               if (frameInfos_.tryComplete(info))\n> -                       pipe()->completeRequest(request);\n> +void IPU3CameraData::metadataReady(unsigned int id, const ControlList &metadata)\n> +{\n> +       IPU3Frames::Info *info = frameInfos_.find(id);\n> +       if (!info)\n> +               return;\n>  \n> -               break;\n> -       }\n> -       default:\n> -               LOG(IPU3, Error) << \"Unknown action \" << action.op;\n> -               break;\n> -       }\n> +       Request *request = info->request;\n> +       request->metadata().merge(metadata);\n> +\n> +       info->metadataProcessed = true;\n> +       if (frameInfos_.tryComplete(info))\n> +               pipe()->completeRequest(request);\n>  }\n>  \n>  /* -----------------------------------------------------------------------------\n> @@ -1385,11 +1375,7 @@ void IPU3CameraData::cio2BufferReady(FrameBuffer *buffer)\n>         if (request->findBuffer(&rawStream_))\n>                 pipe()->completeBuffer(request, buffer);\n>  \n> -       ipa::ipu3::IPU3Event ev;\n> -       ev.op = ipa::ipu3::EventFillParams;\n> -       ev.frame = info->id;\n> -       ev.bufferId = info->paramBuffer->cookie();\n> -       ipa_->processEvent(ev);\n> +       ipa_->fillParameterBuffer(info->id, info->paramBuffer->cookie());\n>  }\n>  \n>  void IPU3CameraData::paramBufferReady(FrameBuffer *buffer)\n> @@ -1433,13 +1419,8 @@ void IPU3CameraData::statBufferReady(FrameBuffer *buffer)\n>                 return;\n>         }\n>  \n> -       ipa::ipu3::IPU3Event ev;\n> -       ev.op = ipa::ipu3::EventStatReady;\n> -       ev.frame = info->id;\n> -       ev.bufferId = info->statBuffer->cookie();\n> -       ev.frameTimestamp = request->metadata().get(controls::SensorTimestamp);\n> -       ev.sensorControls = info->effectiveSensorControls;\n> -       ipa_->processEvent(ev);\n> +       ipa_->statsReady(info->id, request->metadata().get(controls::SensorTimestamp),\n> +                        info->statBuffer->cookie(), info->effectiveSensorControls);\n>  }\n>  \n>  /*\n> -- \n> 2.31.1\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 3E8A3BF415\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon, 28 Feb 2022 14:06:05 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 93A4D61166;\n\tMon, 28 Feb 2022 15:06:04 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D045461101\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 28 Feb 2022 15:06:02 +0100 (CET)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 5C55A478;\n\tMon, 28 Feb 2022 15:06:02 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"N+QfllWu\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1646057162;\n\tbh=H1aCtuBIxu/mxLhEB8jwLm6lAGn3WzdnLOVUyO4epqM=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=N+QfllWuJ6IIiG7RNeIZvYEsdbe3iy4HA1226podEu9BmEe7StV8ZnIn3q6DVANin\n\tQ7I2x0Fjdz2xCU5eIdUvhf4VWMIm2TWqVv8TLlmdNA6CjsGzsY0wfsV3dL5D84H6M0\n\tyyP3JLHZufBtg6CCZNmjw6+aPq4CUz655eDsaPnk=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20220228132250.116211-2-umang.jain@ideasonboard.com>","References":"<20220228132250.116211-1-umang.jain@ideasonboard.com>\n\t<20220228132250.116211-2-umang.jain@ideasonboard.com>","From":"Kieran Bingham <kieran.bingham@ideasonboard.com>","To":"Umang Jain <umang.jain@ideasonboard.com>,\n\tlibcamera-devel@lists.libcamera.org","Date":"Mon, 28 Feb 2022 14:05:59 +0000","Message-ID":"<164605715969.2091767.17066819612604019589@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH 1/2] ipa: ipu3: Replace event-based\n\toperations 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>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":22204,"web_url":"https://patchwork.libcamera.org/comment/22204/","msgid":"<82631d8d-f8f5-5ae0-603e-2a4e02d7582a@ideasonboard.com>","date":"2022-02-28T18:30:47","subject":"Re: [libcamera-devel] [PATCH 1/2] ipa: ipu3: Replace event-based\n\toperations with dedicated functions","submitter":{"id":75,"url":"https://patchwork.libcamera.org/api/people/75/","name":"Jean-Michel Hautbois","email":"jeanmichel.hautbois@ideasonboard.com"},"content":"Hi Umang,\n\nThanks for this patch, a big one :-).\n\nOn 28/02/2022 14:22, Umang Jain 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> 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 |  19 ++--\n>   src/ipa/ipu3/ipu3.cpp                  | 116 ++++++++++-------------\n>   src/libcamera/pipeline/ipu3/ipu3.cpp   | 123 +++++++++++--------------\n>   4 files changed, 123 insertions(+), 171 deletions(-)\n> \n> diff --git a/include/libcamera/ipa/ipu3.mojom b/include/libcamera/ipa/ipu3.mojom\n> index cc0d822f..12b8bb2f 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> @@ -55,9 +29,15 @@ interface IPAIPU3Interface {\n>   \tmapBuffers(array<libcamera.IPABuffer> buffers);\n>   \tunmapBuffers(array<uint32> ids);\n>   \n> -\t[async] processEvent(IPU3Event ev);\n> +\t[async] fillParameterBuffer(uint32 frame, uint32 bufferId);\n> +\t[async] processControls(uint32 frame, libcamera.ControlList controls);\n> +\t[async] statsReady(uint32 frame, int64 frameTimestamp, uint32 bufferId,\n> +\t\t\t   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> +\tparamFilled(uint32 frame);\n> +\tmetadataReady(uint32 frame, libcamera.ControlList metadata);\n>   };\n\nI am a bit confused about the namings, I would go (personnal choice :-)) \nfor:\nfillParamsBuffer\nparamsBufferFilled\n\nOn the other end, we have the stats.\ns/statsReady/processStatsBuffer ?\nAnd then, I can't say why it is named metadataReady...\ns/metadataReady/statsBufferReady ?\n\nThis is a very first thinking about it, I know that namings are always \ntricky, my proposal are certainly not the best ones :-).\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..83f8634d 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(), fillParametersBuffer(), statsReady()\n> +        ▼   ▼   ▼      ▼ │  ▼ │  ▼ │  ▼ │      ▼          (▲) setSensorControls, paramFilled, 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> +-  fillParameterBuffer()\n> +-  processControls()\n> +-  statsReady()\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> -``EventFillParams`` event, and then passed directly to each algorithm\n> +``fillParameterBuffer()``, 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> +``paramFilled`` 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> +``statsReady()``. 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 frame completed is returned back via\n> +``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> +``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 3d307708..2fab4ee0 100644\n> --- a/src/ipa/ipu3/ipu3.cpp\n> +++ b/src/ipa/ipu3/ipu3.cpp\n> @@ -142,14 +142,18 @@ 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 fillParameterBuffer(const uint32_t frame, const uint32_t bufferId) override;\n> +\tvoid processControls(const uint32_t frame, const ControlList &controls) override;\n> +\tvoid statsReady(const uint32_t frame, const int64_t frameTimestamp,\n> +\t\t\tconst uint32_t bufferId, const ControlList &sensorControls) 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,58 +509,49 @@ 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 Prepare the ISP to process the Request\n> + * \\param[in] frame The frame number\n> + * \\param[in] bufferId Buffer ID\n>    */\n> -void IPAIPU3::processEvent(const IPU3Event &event)\n> +void IPAIPU3::fillParameterBuffer(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> -\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> -\n> -\t\tfillParams(event.frame, params);\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 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> -\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> -\t\tcontext_.frameContext.sensor.exposure = event.sensorControls.get(V4L2_CID_EXPOSURE).get<int32_t>();\n> -\t\tcontext_.frameContext.sensor.gain = camHelper_->gain(event.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> +\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> +\tfillParams(frame, params);\n> +}\n> +\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> + * \\param[in] sensorControls Sensor controls\n> + */\n> +void IPAIPU3::statsReady(const uint32_t frame, const int64_t frameTimestamp,\n> +\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> +\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> +\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> +\tparseStatistics(frame, frameTimestamp, stats);\n>   }\n>   \n>   /**\n> @@ -567,7 +562,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> @@ -597,10 +592,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> +\tparamFilled.emit(frame);\n>   }\n>   \n>   /**\n> @@ -642,11 +634,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> @@ -658,18 +646,16 @@ void IPAIPU3::parseStatistics(unsigned int frame,\n>    */\n>   void IPAIPU3::setControls(unsigned int frame)\n>   {\n> -\tIPU3Action op;\n> -\top.op = ActionSetSensorControls;\n> -\n>   \texposure_ = context_.frameContext.agc.exposure;\n>   \tgain_ = camHelper_->gainCode(context_.frameContext.agc.gain);\n>   \n>   \tControlList ctrls(ctrls_);\n>   \tctrls.set(V4L2_CID_EXPOSURE, static_cast<int32_t>(exposure_));\n>   \tctrls.set(V4L2_CID_ANALOGUE_GAIN, static_cast<int32_t>(gain_));\n> -\top.sensorControls = ctrls;\n>   \n> -\tqueueFrameAction.emit(frame, op);\n> +\tControlList lensCtrls;\n> +\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 6c5617cd..dd1780ef 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> +\tvoid setSensorControls(unsigned int id, const ControlList &sensorControls,\n> +\t\t\t       const ControlList &lensControls);\n>   };\n>   \n>   class IPU3CameraConfiguration : public CameraConfiguration\n> @@ -866,11 +868,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> @@ -1213,7 +1211,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_->paramFilled.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> @@ -1248,69 +1248,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->setFocusPostion(focusValue.get<int32_t>());\n> +\tconst ControlValue &focusValue =\n> +\t\tlensControls.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->setFocusPostion(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::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> @@ -1385,11 +1375,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_->fillParameterBuffer(info->id, info->paramBuffer->cookie());\n>   }\n>   \n>   void IPU3CameraData::paramBufferReady(FrameBuffer *buffer)\n> @@ -1433,13 +1419,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_->statsReady(info->id, request->metadata().get(controls::SensorTimestamp),\n> +\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 CF26BBF415\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon, 28 Feb 2022 18:30:51 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E46FA61166;\n\tMon, 28 Feb 2022 19:30:50 +0100 (CET)","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 76B3261101\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 28 Feb 2022 19:30:49 +0100 (CET)","from [IPV6:2a01:e0a:169:7140:dfc9:4b66:632e:8126] (unknown\n\t[IPv6:2a01:e0a:169:7140:dfc9:4b66:632e:8126])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 09CD8478;\n\tMon, 28 Feb 2022 19:30:49 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"Bi+Ppzon\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1646073049;\n\tbh=IHvPx3eUCAQ6ctnvLjrVHTts2UtcjIxuCXWIHDRjE4k=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=Bi+PpzonA56mHk/eTghgB3zmSb4gI0hRa5hmsTcCiMvk44LiKhvcKZgoTK/rIuQN+\n\tk4A+hyusbGloXbMQsADNDx+WqqddmcfVXTWEcOAqDrcY9Q7+YVpWSlDX8pk2sIxUEh\n\tPk+DlaRpwtkQ4HPmfydKhGW31Kb662+iCtZrhqE0=","Message-ID":"<82631d8d-f8f5-5ae0-603e-2a4e02d7582a@ideasonboard.com>","Date":"Mon, 28 Feb 2022 19:30:47 +0100","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101\n\tThunderbird/91.5.0","Content-Language":"en-US","To":"Umang Jain <umang.jain@ideasonboard.com>,\n\tlibcamera-devel@lists.libcamera.org","References":"<20220228132250.116211-1-umang.jain@ideasonboard.com>\n\t<20220228132250.116211-2-umang.jain@ideasonboard.com>","From":"Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>","In-Reply-To":"<20220228132250.116211-2-umang.jain@ideasonboard.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH 1/2] ipa: ipu3: Replace event-based\n\toperations 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>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":22205,"web_url":"https://patchwork.libcamera.org/comment/22205/","msgid":"<1c1fb50e-d697-a90c-eb97-ab0be8216e3c@ideasonboard.com>","date":"2022-02-28T18:32:47","subject":"Re: [libcamera-devel] [PATCH 1/2] ipa: ipu3: Replace event-based\n\toperations with dedicated functions","submitter":{"id":75,"url":"https://patchwork.libcamera.org/api/people/75/","name":"Jean-Michel Hautbois","email":"jeanmichel.hautbois@ideasonboard.com"},"content":"On 28/02/2022 19:30, Jean-Michel Hautbois wrote:\n> Hi Umang,\n> \n> Thanks for this patch, a big one :-).\n> \n> On 28/02/2022 14:22, Umang Jain 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>> 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 |  19 ++--\n>>   src/ipa/ipu3/ipu3.cpp                  | 116 ++++++++++-------------\n>>   src/libcamera/pipeline/ipu3/ipu3.cpp   | 123 +++++++++++--------------\n>>   4 files changed, 123 insertions(+), 171 deletions(-)\n>>\n>> diff --git a/include/libcamera/ipa/ipu3.mojom \n>> b/include/libcamera/ipa/ipu3.mojom\n>> index cc0d822f..12b8bb2f 100644\n>> --- a/include/libcamera/ipa/ipu3.mojom\n>> +++ b/include/libcamera/ipa/ipu3.mojom\n>> @@ -8,32 +8,6 @@ module ipa.ipu3;\n>>   import \"include/libcamera/ipa/core.mojom\";\n>> -enum IPU3Operations {\n>> -    ActionSetSensorControls = 1,\n>> -    ActionParamFilled = 2,\n>> -    ActionMetadataReady = 3,\n>> -    EventProcessControls = 4,\n>> -    EventStatReady = 5,\n>> -    EventFillParams = 6,\n>> -};\n>> -\n>> -struct IPU3Event {\n>> -    IPU3Operations op;\n>> -    uint32 frame;\n>> -    int64 frameTimestamp;\n>> -    uint32 bufferId;\n>> -    libcamera.ControlList controls;\n>> -    libcamera.ControlList sensorControls;\n>> -    libcamera.ControlList lensControls;\n>> -};\n>> -\n>> -struct IPU3Action {\n>> -    IPU3Operations op;\n>> -    libcamera.ControlList controls;\n>> -    libcamera.ControlList sensorControls;\n>> -    libcamera.ControlList lensControls;\n>> -};\n>> -\n>>   struct IPAConfigInfo {\n>>       libcamera.IPACameraSensorInfo sensorInfo;\n>>       libcamera.ControlInfoMap sensorControls;\n>> @@ -55,9 +29,15 @@ interface IPAIPU3Interface {\n>>       mapBuffers(array<libcamera.IPABuffer> buffers);\n>>       unmapBuffers(array<uint32> ids);\n>> -    [async] processEvent(IPU3Event ev);\n>> +    [async] fillParameterBuffer(uint32 frame, uint32 bufferId);\n>> +    [async] processControls(uint32 frame, libcamera.ControlList \n>> controls);\n>> +    [async] statsReady(uint32 frame, int64 frameTimestamp, uint32 \n>> bufferId,\n>> +               libcamera.ControlList sensorControls);\n>>   };\n>>   interface IPAIPU3EventInterface {\n>> -    queueFrameAction(uint32 frame, IPU3Action action);\n>> +    setSensorControls(uint32 frame, libcamera.ControlList \n>> sensorControls,\n>> +              libcamera.ControlList lensControls);\n>> +    paramFilled(uint32 frame);\n>> +    metadataReady(uint32 frame, libcamera.ControlList metadata);\n>>   };\n> \n> I am a bit confused about the namings, I would go (personnal choice :-)) \n> for:\n> fillParamsBuffer\n> paramsBufferFilled\n> \n> On the other end, we have the stats.\n> s/statsReady/processStatsBuffer ?\n> And then, I can't say why it is named metadataReady...\n> s/metadataReady/statsBufferReady ?\n> \n> This is a very first thinking about it, I know that namings are always \n> tricky, my proposal are certainly not the best ones :-).\n> \n\nAnd as I was reading myself, I thought:\ns/fillParameterBuffer/processParamsBuffer\ns/paramFilled/paramsBufferReady\ns/statsReady/processStatsBuffer\ns/metadataReady/statsBufferReady\n\n>> diff --git a/src/ipa/ipu3/ipu3-ipa-design-guide.rst \n>> b/src/ipa/ipu3/ipu3-ipa-design-guide.rst\n>> index 89c71108..83f8634d 100644\n>> --- a/src/ipa/ipu3/ipu3-ipa-design-guide.rst\n>> +++ b/src/ipa/ipu3/ipu3-ipa-design-guide.rst\n>> @@ -25,7 +25,8 @@ from applications, and managing events from the \n>> pipeline handler.\n>>         └─┬───┬───┬──────┬────┬────┬────┬─┴────▼─┬──┘    1: init()\n>>           │   │   │      │ ▲  │ ▲  │ ▲  │ ▲      │       2: configure()\n>>           │1  │2  │3     │4│  │4│  │4│  │4│      │5      3: \n>> mapBuffers(), start()\n>> -        ▼   ▼   ▼      ▼ │  ▼ │  ▼ │  ▼ │      ▼       4: processEvent()\n>> +        │   │   │      │ │  │ │  │ │  │ │      │       4: (▼) \n>> processControls(), fillParametersBuffer(), statsReady()\n>> +        ▼   ▼   ▼      ▼ │  ▼ │  ▼ │  ▼ │      ▼          (▲) \n>> setSensorControls, paramFilled, metadataReady Signals\n>>         ┌──────────────────┴────┴────┴────┴─────────┐    5: stop(), \n>> unmapBuffers()\n>>         │ IPU3 IPA                                  │\n>>         │                 ┌───────────────────────┐ │\n>> @@ -100,8 +101,9 @@ There are three main interactions with the \n>> algorithms for the IPU3 IPA\n>>   to operate when running:\n>>   -  configure()\n>> --  processEvent(``EventFillParams``)\n>> --  processEvent(``EventStatReady``)\n>> +-  fillParameterBuffer()\n>> +-  processControls()\n>> +-  statsReady()\n>>   The configuration phase allows the pipeline-handler to inform the \n>> 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 \n>> 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>> +``fillParameterBuffer()``, and then passed directly to each algorithm\n>>   through the ``prepare()`` call allowing the ISP configuration to be\n>>   updated for the needs of each component that the algorithm is\n>>   responsible for.\n>> @@ -125,7 +127,7 @@ structure that it modifies, and it should take \n>> care to ensure that any\n>>   structure set by a use flag is fully initialised to suitable values.\n>>   The parameter buffer is returned to the pipeline handler through the\n>> -``ActionParamFilled`` event, and from there queued to the ImgU along\n>> +``paramFilled`` signal, and from there queued to the ImgU along\n>>   with a raw frame captured with the CIO2.\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>> +``statsReady()``. This provides the IPA with an opportunity to\n>>   examine the results of the ISP and run the calculations required by \n>> 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 \n>> use\n>> @@ -145,11 +147,14 @@ before they are needed.\n>>   The ordering of the algorithm processing is determined by their\n>>   placement in the ``IPU3::algorithms_`` ordered list.\n>> +Finally, the IPA metadata for the frame completed is returned back via\n>> +``metadataReady`` Signal.\n>> +\n>>   Sensor Controls\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>> +``setSensorControls`` signal to allow the IPA to request controls to \n>> 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 3d307708..2fab4ee0 100644\n>> --- a/src/ipa/ipu3/ipu3.cpp\n>> +++ b/src/ipa/ipu3/ipu3.cpp\n>> @@ -142,14 +142,18 @@ public:\n>>       void mapBuffers(const std::vector<IPABuffer> &buffers) override;\n>>       void unmapBuffers(const std::vector<unsigned int> &ids) override;\n>> -    void processEvent(const IPU3Event &event) override;\n>> +\n>> +    void fillParameterBuffer(const uint32_t frame, const uint32_t \n>> bufferId) override;\n>> +    void processControls(const uint32_t frame, const ControlList \n>> &controls) override;\n>> +    void statsReady(const uint32_t frame, const int64_t frameTimestamp,\n>> +            const uint32_t bufferId, const ControlList \n>> &sensorControls) override;\n>>   private:\n>>       void updateControls(const IPACameraSensorInfo &sensorInfo,\n>>                   const ControlInfoMap &sensorControls,\n>>                   ControlInfoMap *ipaControls);\n>>       void updateSessionConfiguration(const ControlInfoMap \n>> &sensorControls);\n>> -    void processControls(unsigned int frame, const ControlList \n>> &controls);\n>> +\n>>       void fillParams(unsigned int frame, ipu3_uapi_params *params);\n>>       void parseStatistics(unsigned int frame,\n>>                    int64_t frameTimestamp,\n>> @@ -505,58 +509,49 @@ void IPAIPU3::unmapBuffers(const \n>> std::vector<unsigned int> &ids)\n>>   }\n>>   /**\n>> - * \\brief Process an event generated by the pipeline handler\n>> - * \\param[in] event The event sent from pipeline handler\n>> - *\n>> - * The expected event handling over the lifetime of a Request has\n>> - * the following sequence:\n>> - *\n>> - *   - EventProcessControls : Handle controls from a new Request\n>> - *   - EventFillParams : Prepare the ISP to process the Request\n>> - *   - EventStatReady : Process statistics after ISP completion\n>> + * \\brief Prepare the ISP to process the Request\n>> + * \\param[in] frame The frame number\n>> + * \\param[in] bufferId Buffer ID\n>>    */\n>> -void IPAIPU3::processEvent(const IPU3Event &event)\n>> +void IPAIPU3::fillParameterBuffer(const uint32_t frame, const \n>> uint32_t bufferId)\n>>   {\n>> -    switch (event.op) {\n>> -    case EventProcessControls: {\n>> -        processControls(event.frame, event.controls);\n>> -        break;\n>> -    }\n>> -    case EventFillParams: {\n>> -        auto it = buffers_.find(event.bufferId);\n>> -        if (it == buffers_.end()) {\n>> -            LOG(IPAIPU3, Error) << \"Could not find param buffer!\";\n>> -            return;\n>> -        }\n>> -\n>> -        Span<uint8_t> mem = it->second.planes()[0];\n>> -        ipu3_uapi_params *params =\n>> -            reinterpret_cast<ipu3_uapi_params *>(mem.data());\n>> -\n>> -        fillParams(event.frame, params);\n>> -        break;\n>> +    auto it = buffers_.find(bufferId);\n>> +    if (it == buffers_.end()) {\n>> +        LOG(IPAIPU3, Error) << \"Could not find param buffer!\";\n>> +        return;\n>>       }\n>> -    case EventStatReady: {\n>> -        auto it = buffers_.find(event.bufferId);\n>> -        if (it == buffers_.end()) {\n>> -            LOG(IPAIPU3, Error) << \"Could not find stats buffer!\";\n>> -            return;\n>> -        }\n>> -\n>> -        Span<uint8_t> mem = it->second.planes()[0];\n>> -        const ipu3_uapi_stats_3a *stats =\n>> -            reinterpret_cast<ipu3_uapi_stats_3a *>(mem.data());\n>> -\n>> -        context_.frameContext.sensor.exposure = \n>> event.sensorControls.get(V4L2_CID_EXPOSURE).get<int32_t>();\n>> -        context_.frameContext.sensor.gain = \n>> camHelper_->gain(event.sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>()); \n>>\n>> -\n>> -        parseStatistics(event.frame, event.frameTimestamp, stats);\n>> -        break;\n>> -    }\n>> -    default:\n>> -        LOG(IPAIPU3, Error) << \"Unknown event \" << event.op;\n>> -        break;\n>> +\n>> +    Span<uint8_t> mem = it->second.planes()[0];\n>> +    ipu3_uapi_params *params =\n>> +        reinterpret_cast<ipu3_uapi_params *>(mem.data());\n>> +\n>> +    fillParams(frame, params);\n>> +}\n>> +\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>> + * \\param[in] sensorControls Sensor controls\n>> + */\n>> +void IPAIPU3::statsReady(const uint32_t frame, const int64_t \n>> frameTimestamp,\n>> +             const uint32_t bufferId, const ControlList &sensorControls)\n>> +{\n>> +    auto it = buffers_.find(bufferId);\n>> +    if (it == buffers_.end()) {\n>> +        LOG(IPAIPU3, Error) << \"Could not find stats buffer!\";\n>> +        return;\n>>       }\n>> +\n>> +    Span<uint8_t> mem = it->second.planes()[0];\n>> +    const ipu3_uapi_stats_3a *stats =\n>> +        reinterpret_cast<ipu3_uapi_stats_3a *>(mem.data());\n>> +\n>> +    context_.frameContext.sensor.exposure = \n>> sensorControls.get(V4L2_CID_EXPOSURE).get<int32_t>();\n>> +    context_.frameContext.sensor.gain = \n>> camHelper_->gain(sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>()); \n>>\n>> +\n>> +    parseStatistics(frame, frameTimestamp, stats);\n>>   }\n>>   /**\n>> @@ -567,7 +562,7 @@ void IPAIPU3::processEvent(const IPU3Event &event)\n>>    * Parse the request to handle any IPA-managed controls that were \n>> set from the\n>>    * application such as manual sensor settings.\n>>    */\n>> -void IPAIPU3::processControls([[maybe_unused]] unsigned int frame,\n>> +void IPAIPU3::processControls(const uint32_t frame,\n>>                     [[maybe_unused]] const ControlList &controls)\n>>   {\n>>       /* \\todo Start processing for 'frame' based on 'controls'. */\n>> @@ -597,10 +592,7 @@ void IPAIPU3::fillParams(unsigned int frame, \n>> ipu3_uapi_params *params)\n>>       for (auto const &algo : algorithms_)\n>>           algo->prepare(context_, params);\n>> -    IPU3Action op;\n>> -    op.op = ActionParamFilled;\n>> -\n>> -    queueFrameAction.emit(frame, op);\n>> +    paramFilled.emit(frame);\n>>   }\n>>   /**\n>> @@ -642,11 +634,7 @@ void IPAIPU3::parseStatistics(unsigned int frame,\n>>        * likely want to avoid putting platform specific metadata in.\n>>        */\n>> -    IPU3Action op;\n>> -    op.op = ActionMetadataReady;\n>> -    op.controls = ctrls;\n>> -\n>> -    queueFrameAction.emit(frame, op);\n>> +    metadataReady.emit(frame, ctrls);\n>>   }\n>>   /**\n>> @@ -658,18 +646,16 @@ void IPAIPU3::parseStatistics(unsigned int frame,\n>>    */\n>>   void IPAIPU3::setControls(unsigned int frame)\n>>   {\n>> -    IPU3Action op;\n>> -    op.op = ActionSetSensorControls;\n>> -\n>>       exposure_ = context_.frameContext.agc.exposure;\n>>       gain_ = camHelper_->gainCode(context_.frameContext.agc.gain);\n>>       ControlList ctrls(ctrls_);\n>>       ctrls.set(V4L2_CID_EXPOSURE, static_cast<int32_t>(exposure_));\n>>       ctrls.set(V4L2_CID_ANALOGUE_GAIN, static_cast<int32_t>(gain_));\n>> -    op.sensorControls = ctrls;\n>> -    queueFrameAction.emit(frame, op);\n>> +    ControlList lensCtrls;\n>> +\n>> +    setSensorControls.emit(frame, ctrls, lensCtrls);\n>>   }\n>>   } /* namespace ipa::ipu3 */\n>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp \n>> b/src/libcamera/pipeline/ipu3/ipu3.cpp\n>> index 6c5617cd..dd1780ef 100644\n>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n>> @@ -86,8 +86,10 @@ public:\n>>       ControlInfoMap ipaControls_;\n>>   private:\n>> -    void queueFrameAction(unsigned int id,\n>> -                  const ipa::ipu3::IPU3Action &action);\n>> +    void metadataReady(unsigned int id, const ControlList &metadata);\n>> +    void paramFilled(unsigned int id);\n>> +    void setSensorControls(unsigned int id, const ControlList \n>> &sensorControls,\n>> +                   const ControlList &lensControls);\n>>   };\n>>   class IPU3CameraConfiguration : public CameraConfiguration\n>> @@ -866,11 +868,7 @@ void IPU3CameraData::queuePendingRequests()\n>>           info->rawBuffer = rawBuffer;\n>> -        ipa::ipu3::IPU3Event ev;\n>> -        ev.op = ipa::ipu3::EventProcessControls;\n>> -        ev.frame = info->id;\n>> -        ev.controls = request->controls();\n>> -        ipa_->processEvent(ev);\n>> +        ipa_->processControls(info->id, request->controls());\n>>           pendingRequests_.pop();\n>>           processingRequests_.push(request);\n>> @@ -1213,7 +1211,9 @@ int IPU3CameraData::loadIPA()\n>>       if (!ipa_)\n>>           return -ENOENT;\n>> -    ipa_->queueFrameAction.connect(this, \n>> &IPU3CameraData::queueFrameAction);\n>> +    ipa_->setSensorControls.connect(this, \n>> &IPU3CameraData::setSensorControls);\n>> +    ipa_->paramFilled.connect(this, &IPU3CameraData::paramFilled);\n>> +    ipa_->metadataReady.connect(this, &IPU3CameraData::metadataReady);\n>>       /*\n>>        * Pass the sensor info to the IPA to initialize controls.\n>> @@ -1248,69 +1248,59 @@ int IPU3CameraData::loadIPA()\n>>       return 0;\n>>   }\n>> -void IPU3CameraData::queueFrameAction(unsigned int id,\n>> -                      const ipa::ipu3::IPU3Action &action)\n>> +void IPU3CameraData::setSensorControls([[maybe_unused]] unsigned int id,\n>> +                       const ControlList &sensorControls,\n>> +                       const ControlList &lensControls)\n>>   {\n>> -    switch (action.op) {\n>> -    case ipa::ipu3::ActionSetSensorControls: {\n>> -        const ControlList &sensorControls = action.sensorControls;\n>> -        delayedCtrls_->push(sensorControls);\n>> +    delayedCtrls_->push(sensorControls);\n>> -        CameraLens *focusLens = cio2_.sensor()->focusLens();\n>> -        if (!focusLens)\n>> -            break;\n>> -\n>> -        const ControlList lensControls = action.lensControls;\n>> -        if (!lensControls.contains(V4L2_CID_FOCUS_ABSOLUTE))\n>> -            break;\n>> +    CameraLens *focusLens = cio2_.sensor()->focusLens();\n>> +    if (!focusLens)\n>> +        return;\n>> -        const ControlValue &focusValue =\n>> -            lensControls.get(V4L2_CID_FOCUS_ABSOLUTE);\n>> +    if (!lensControls.contains(V4L2_CID_FOCUS_ABSOLUTE))\n>> +        return;\n>> -        focusLens->setFocusPostion(focusValue.get<int32_t>());\n>> +    const ControlValue &focusValue =\n>> +        lensControls.get(V4L2_CID_FOCUS_ABSOLUTE);\n>> -        break;\n>> -    }\n>> -    case ipa::ipu3::ActionParamFilled: {\n>> -        IPU3Frames::Info *info = frameInfos_.find(id);\n>> -        if (!info)\n>> -            break;\n>> -\n>> -        /* Queue all buffers from the request aimed for the ImgU. */\n>> -        for (auto it : info->request->buffers()) {\n>> -            const Stream *stream = it.first;\n>> -            FrameBuffer *outbuffer = it.second;\n>> +    focusLens->setFocusPostion(focusValue.get<int32_t>());\n>> +}\n>> -            if (stream == &outStream_)\n>> -                imgu_->output_->queueBuffer(outbuffer);\n>> -            else if (stream == &vfStream_)\n>> -                imgu_->viewfinder_->queueBuffer(outbuffer);\n>> -        }\n>> +void IPU3CameraData::paramFilled(unsigned int id)\n>> +{\n>> +    IPU3Frames::Info *info = frameInfos_.find(id);\n>> +    if (!info)\n>> +        return;\n>> -        imgu_->param_->queueBuffer(info->paramBuffer);\n>> -        imgu_->stat_->queueBuffer(info->statBuffer);\n>> -        imgu_->input_->queueBuffer(info->rawBuffer);\n>> +    /* Queue all buffers from the request aimed for the ImgU. */\n>> +    for (auto it : info->request->buffers()) {\n>> +        const Stream *stream = it.first;\n>> +        FrameBuffer *outbuffer = it.second;\n>> -        break;\n>> +        if (stream == &outStream_)\n>> +            imgu_->output_->queueBuffer(outbuffer);\n>> +        else if (stream == &vfStream_)\n>> +            imgu_->viewfinder_->queueBuffer(outbuffer);\n>>       }\n>> -    case ipa::ipu3::ActionMetadataReady: {\n>> -        IPU3Frames::Info *info = frameInfos_.find(id);\n>> -        if (!info)\n>> -            break;\n>> -        Request *request = info->request;\n>> -        request->metadata().merge(action.controls);\n>> +    imgu_->param_->queueBuffer(info->paramBuffer);\n>> +    imgu_->stat_->queueBuffer(info->statBuffer);\n>> +    imgu_->input_->queueBuffer(info->rawBuffer);\n>> +}\n>> -        info->metadataProcessed = true;\n>> -        if (frameInfos_.tryComplete(info))\n>> -            pipe()->completeRequest(request);\n>> +void IPU3CameraData::metadataReady(unsigned int id, const ControlList \n>> &metadata)\n>> +{\n>> +    IPU3Frames::Info *info = frameInfos_.find(id);\n>> +    if (!info)\n>> +        return;\n>> -        break;\n>> -    }\n>> -    default:\n>> -        LOG(IPU3, Error) << \"Unknown action \" << action.op;\n>> -        break;\n>> -    }\n>> +    Request *request = info->request;\n>> +    request->metadata().merge(metadata);\n>> +\n>> +    info->metadataProcessed = true;\n>> +    if (frameInfos_.tryComplete(info))\n>> +        pipe()->completeRequest(request);\n>>   }\n>>   /* \n>> ----------------------------------------------------------------------------- \n>>\n>> @@ -1385,11 +1375,7 @@ void \n>> IPU3CameraData::cio2BufferReady(FrameBuffer *buffer)\n>>       if (request->findBuffer(&rawStream_))\n>>           pipe()->completeBuffer(request, buffer);\n>> -    ipa::ipu3::IPU3Event ev;\n>> -    ev.op = ipa::ipu3::EventFillParams;\n>> -    ev.frame = info->id;\n>> -    ev.bufferId = info->paramBuffer->cookie();\n>> -    ipa_->processEvent(ev);\n>> +    ipa_->fillParameterBuffer(info->id, info->paramBuffer->cookie());\n>>   }\n>>   void IPU3CameraData::paramBufferReady(FrameBuffer *buffer)\n>> @@ -1433,13 +1419,8 @@ void \n>> IPU3CameraData::statBufferReady(FrameBuffer *buffer)\n>>           return;\n>>       }\n>> -    ipa::ipu3::IPU3Event ev;\n>> -    ev.op = ipa::ipu3::EventStatReady;\n>> -    ev.frame = info->id;\n>> -    ev.bufferId = info->statBuffer->cookie();\n>> -    ev.frameTimestamp = \n>> request->metadata().get(controls::SensorTimestamp);\n>> -    ev.sensorControls = info->effectiveSensorControls;\n>> -    ipa_->processEvent(ev);\n>> +    ipa_->statsReady(info->id, \n>> request->metadata().get(controls::SensorTimestamp),\n>> +             info->statBuffer->cookie(), info->effectiveSensorControls);\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 802ABBE08A\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon, 28 Feb 2022 18:32:52 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D582F6116E;\n\tMon, 28 Feb 2022 19:32:51 +0100 (CET)","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 79FCD61101\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 28 Feb 2022 19:32:50 +0100 (CET)","from [IPV6:2a01:e0a:169:7140:dfc9:4b66:632e:8126] (unknown\n\t[IPv6:2a01:e0a:169:7140:dfc9:4b66:632e:8126])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 1AABA478;\n\tMon, 28 Feb 2022 19:32:50 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"asZMp9H4\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1646073170;\n\tbh=a1Q+TGMvW8BN5pDAPOZmXgI0/l/SRuMw2hhk6E9kEqk=;\n\th=Date:Subject:From:To:References:In-Reply-To:From;\n\tb=asZMp9H4eRaJkjLfFNcFsafS//iuRK2yPfarFH/X6YiVVQc2YcPYdz2ju8NxsoFEK\n\t9nl9GpoJxgk1aB5C1VrYAsRkJVqnR6KZ+nO6i8qYIQPJkQzSPZq4u6m5vPS+NLFgLZ\n\thXct6I5oLFMSZ/pHZYtSWFu4D4cgEteIZC/8HfR0=","Message-ID":"<1c1fb50e-d697-a90c-eb97-ab0be8216e3c@ideasonboard.com>","Date":"Mon, 28 Feb 2022 19:32:47 +0100","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101\n\tThunderbird/91.5.0","Content-Language":"en-US","From":"Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>","To":"Umang Jain <umang.jain@ideasonboard.com>,\n\tlibcamera-devel@lists.libcamera.org","References":"<20220228132250.116211-1-umang.jain@ideasonboard.com>\n\t<20220228132250.116211-2-umang.jain@ideasonboard.com>\n\t<82631d8d-f8f5-5ae0-603e-2a4e02d7582a@ideasonboard.com>","In-Reply-To":"<82631d8d-f8f5-5ae0-603e-2a4e02d7582a@ideasonboard.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH 1/2] ipa: ipu3: Replace event-based\n\toperations 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>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]