[{"id":28919,"web_url":"https://patchwork.libcamera.org/comment/28919/","msgid":"<7964f2a1-de1b-40fd-9675-91806d2a4729@ideasonboard.com>","date":"2024-03-11T16:04:49","subject":"Re: [PATCH v2 3/4] libcamera: ipu3: Replace IPU3FrameInfo","submitter":{"id":156,"url":"https://patchwork.libcamera.org/api/people/156/","name":"Dan Scally","email":"dan.scally@ideasonboard.com"},"content":"Hi Jacopo\n\nOn 11/03/2024 12:32, Jacopo Mondi wrote:\n> Now that the pipeline handler can create a private derived class of\n> Request::Private, use it to store the pipeline specific data.\n>\n> In the case of IPU3 we associate statistics and paramters buffer with a\n> Request and a raw buffer which gets dequeued from the CIO2 and input\n> to the ImgU input.\n>\n> This replaces the functionalities of IPU3FrameInfo which can now be\n> removed.\n>\n> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>\n\n\nLooks good now, and I tested this version too:\n\n\nReviewed-by: Daniel Scally <dan.scally@ideasonboard.com>\n\nTested-by: Daniel Scally <dan.scally@ideasonboard.com>\n\n> ---\n>   src/libcamera/pipeline/ipu3/frames.cpp  | 143 ----------------\n>   src/libcamera/pipeline/ipu3/frames.h    |  67 --------\n>   src/libcamera/pipeline/ipu3/ipu3.cpp    | 212 +++++++++++++++---------\n>   src/libcamera/pipeline/ipu3/meson.build |   1 -\n>   4 files changed, 137 insertions(+), 286 deletions(-)\n>   delete mode 100644 src/libcamera/pipeline/ipu3/frames.cpp\n>   delete mode 100644 src/libcamera/pipeline/ipu3/frames.h\n>\n> diff --git a/src/libcamera/pipeline/ipu3/frames.cpp b/src/libcamera/pipeline/ipu3/frames.cpp\n> deleted file mode 100644\n> index a4c3477cd9ef..000000000000\n> --- a/src/libcamera/pipeline/ipu3/frames.cpp\n> +++ /dev/null\n> @@ -1,143 +0,0 @@\n> -/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> -/*\n> - * Copyright (C) 2020, Google Inc.\n> - *\n> - * frames.cpp - Intel IPU3 Frames helper\n> - */\n> -\n> -#include \"frames.h\"\n> -\n> -#include <libcamera/framebuffer.h>\n> -#include <libcamera/request.h>\n> -\n> -#include \"libcamera/internal/framebuffer.h\"\n> -#include \"libcamera/internal/pipeline_handler.h\"\n> -#include \"libcamera/internal/v4l2_videodevice.h\"\n> -\n> -namespace libcamera {\n> -\n> -LOG_DECLARE_CATEGORY(IPU3)\n> -\n> -IPU3Frames::IPU3Frames()\n> -{\n> -}\n> -\n> -void IPU3Frames::init(const std::vector<std::unique_ptr<FrameBuffer>> &paramBuffers,\n> -\t\t      const std::vector<std::unique_ptr<FrameBuffer>> &statBuffers)\n> -{\n> -\tfor (const std::unique_ptr<FrameBuffer> &buffer : paramBuffers)\n> -\t\tavailableParamBuffers_.push(buffer.get());\n> -\n> -\tfor (const std::unique_ptr<FrameBuffer> &buffer : statBuffers)\n> -\t\tavailableStatBuffers_.push(buffer.get());\n> -\n> -\tframeInfo_.clear();\n> -}\n> -\n> -void IPU3Frames::clear()\n> -{\n> -\tavailableParamBuffers_ = {};\n> -\tavailableStatBuffers_ = {};\n> -}\n> -\n> -IPU3Frames::Info *IPU3Frames::create(Request *request)\n> -{\n> -\tunsigned int id = request->sequence();\n> -\n> -\tif (availableParamBuffers_.empty()) {\n> -\t\tLOG(IPU3, Debug) << \"Parameters buffer underrun\";\n> -\t\treturn nullptr;\n> -\t}\n> -\n> -\tif (availableStatBuffers_.empty()) {\n> -\t\tLOG(IPU3, Debug) << \"Statistics buffer underrun\";\n> -\t\treturn nullptr;\n> -\t}\n> -\n> -\tFrameBuffer *paramBuffer = availableParamBuffers_.front();\n> -\tFrameBuffer *statBuffer = availableStatBuffers_.front();\n> -\n> -\tparamBuffer->_d()->setRequest(request);\n> -\tstatBuffer->_d()->setRequest(request);\n> -\n> -\tavailableParamBuffers_.pop();\n> -\tavailableStatBuffers_.pop();\n> -\n> -\t/* \\todo Remove the dynamic allocation of Info */\n> -\tstd::unique_ptr<Info> info = std::make_unique<Info>();\n> -\n> -\tinfo->id = id;\n> -\tinfo->request = request;\n> -\tinfo->rawBuffer = nullptr;\n> -\tinfo->paramBuffer = paramBuffer;\n> -\tinfo->statBuffer = statBuffer;\n> -\tinfo->paramDequeued = false;\n> -\tinfo->metadataProcessed = false;\n> -\n> -\tframeInfo_[id] = std::move(info);\n> -\n> -\treturn frameInfo_[id].get();\n> -}\n> -\n> -void IPU3Frames::remove(IPU3Frames::Info *info)\n> -{\n> -\t/* Return params and stat buffer for reuse. */\n> -\tavailableParamBuffers_.push(info->paramBuffer);\n> -\tavailableStatBuffers_.push(info->statBuffer);\n> -\n> -\t/* Delete the extended frame information. */\n> -\tframeInfo_.erase(info->id);\n> -}\n> -\n> -bool IPU3Frames::tryComplete(IPU3Frames::Info *info)\n> -{\n> -\tRequest *request = info->request;\n> -\n> -\tif (request->hasPendingBuffers())\n> -\t\treturn false;\n> -\n> -\tif (!info->metadataProcessed)\n> -\t\treturn false;\n> -\n> -\tif (!info->paramDequeued)\n> -\t\treturn false;\n> -\n> -\tremove(info);\n> -\n> -\tbufferAvailable.emit();\n> -\n> -\treturn true;\n> -}\n> -\n> -IPU3Frames::Info *IPU3Frames::find(unsigned int id)\n> -{\n> -\tconst auto &itInfo = frameInfo_.find(id);\n> -\n> -\tif (itInfo != frameInfo_.end())\n> -\t\treturn itInfo->second.get();\n> -\n> -\tLOG(IPU3, Fatal) << \"Can't find tracking information for frame \" << id;\n> -\n> -\treturn nullptr;\n> -}\n> -\n> -IPU3Frames::Info *IPU3Frames::find(FrameBuffer *buffer)\n> -{\n> -\tfor (auto const &itInfo : frameInfo_) {\n> -\t\tInfo *info = itInfo.second.get();\n> -\n> -\t\tfor (auto const itBuffers : info->request->buffers())\n> -\t\t\tif (itBuffers.second == buffer)\n> -\t\t\t\treturn info;\n> -\n> -\t\tif (info->rawBuffer == buffer || info->paramBuffer == buffer ||\n> -\t\t    info->statBuffer == buffer)\n> -\t\t\treturn info;\n> -\t}\n> -\n> -\tLOG(IPU3, Fatal) << \"Can't find tracking information from buffer\";\n> -\n> -\treturn nullptr;\n> -}\n> -\n> -} /* namespace libcamera */\n> diff --git a/src/libcamera/pipeline/ipu3/frames.h b/src/libcamera/pipeline/ipu3/frames.h\n> deleted file mode 100644\n> index 6e3cb915c7b8..000000000000\n> --- a/src/libcamera/pipeline/ipu3/frames.h\n> +++ /dev/null\n> @@ -1,67 +0,0 @@\n> -/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> -/*\n> - * Copyright (C) 2020, Google Inc.\n> - *\n> - * frames.h - Intel IPU3 Frames helper\n> - */\n> -\n> -#pragma once\n> -\n> -#include <map>\n> -#include <memory>\n> -#include <queue>\n> -#include <vector>\n> -\n> -#include <libcamera/base/signal.h>\n> -\n> -#include <libcamera/controls.h>\n> -\n> -namespace libcamera {\n> -\n> -class FrameBuffer;\n> -class IPAProxy;\n> -class PipelineHandler;\n> -class Request;\n> -class V4L2VideoDevice;\n> -struct IPABuffer;\n> -\n> -class IPU3Frames\n> -{\n> -public:\n> -\tstruct Info {\n> -\t\tunsigned int id;\n> -\t\tRequest *request;\n> -\n> -\t\tFrameBuffer *rawBuffer;\n> -\t\tFrameBuffer *paramBuffer;\n> -\t\tFrameBuffer *statBuffer;\n> -\n> -\t\tControlList effectiveSensorControls;\n> -\n> -\t\tbool paramDequeued;\n> -\t\tbool metadataProcessed;\n> -\t};\n> -\n> -\tIPU3Frames();\n> -\n> -\tvoid init(const std::vector<std::unique_ptr<FrameBuffer>> &paramBuffers,\n> -\t\t  const std::vector<std::unique_ptr<FrameBuffer>> &statBuffers);\n> -\tvoid clear();\n> -\n> -\tInfo *create(Request *request);\n> -\tvoid remove(Info *info);\n> -\tbool tryComplete(Info *info);\n> -\n> -\tInfo *find(unsigned int id);\n> -\tInfo *find(FrameBuffer *buffer);\n> -\n> -\tSignal<> bufferAvailable;\n> -\n> -private:\n> -\tstd::queue<FrameBuffer *> availableParamBuffers_;\n> -\tstd::queue<FrameBuffer *> availableStatBuffers_;\n> -\n> -\tstd::map<unsigned int, std::unique_ptr<Info>> frameInfo_;\n> -};\n> -\n> -} /* namespace libcamera */\n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index fa4bd0bb73e2..0c9d3167d2e6 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -34,9 +34,9 @@\n>   #include \"libcamera/internal/ipa_manager.h\"\n>   #include \"libcamera/internal/media_device.h\"\n>   #include \"libcamera/internal/pipeline_handler.h\"\n> +#include \"libcamera/internal/request.h\"\n>   \n>   #include \"cio2.h\"\n> -#include \"frames.h\"\n>   #include \"imgu.h\"\n>   \n>   namespace libcamera {\n> @@ -47,6 +47,24 @@ static const ControlInfoMap::Map IPU3Controls = {\n>   \t{ &controls::draft::PipelineDepth, ControlInfo(2, 3) },\n>   };\n>   \n> +class IPU3Request : public Request::Private\n> +{\n> +public:\n> +\tIPU3Request(Camera *camera)\n> +\t\t: Request::Private(camera)\n> +\t{\n> +\t}\n> +\n> +\tFrameBuffer *rawBuffer;\n> +\tFrameBuffer *paramBuffer;\n> +\tFrameBuffer *statBuffer;\n> +\n> +\tControlList effectiveSensorControls;\n> +\n> +\tbool paramDequeued;\n> +\tbool metadataProcessed;\n> +};\n> +\n>   class IPU3CameraData : public Camera::Private\n>   {\n>   public:\n> @@ -57,6 +75,7 @@ public:\n>   \n>   \tint loadIPA();\n>   \n> +\tvoid tryCompleteRequest(IPU3Request *request);\n>   \tvoid imguOutputBufferReady(FrameBuffer *buffer);\n>   \tvoid cio2BufferReady(FrameBuffer *buffer);\n>   \tvoid paramBufferReady(FrameBuffer *buffer);\n> @@ -75,7 +94,6 @@ public:\n>   \tRectangle cropRegion_;\n>   \n>   \tstd::unique_ptr<DelayedControls> delayedCtrls_;\n> -\tIPU3Frames frameInfos_;\n>   \n>   \tstd::unique_ptr<ipa::ipu3::IPAProxyIPU3> ipa_;\n>   \n> @@ -86,7 +104,17 @@ public:\n>   \n>   \tControlInfoMap ipaControls_;\n>   \n> +\tstd::map<unsigned int, IPU3Request *> requestMap_;\n> +\n> +\tstd::queue<FrameBuffer *> availableParamBuffers_;\n> +\tstd::queue<FrameBuffer *> availableStatBuffers_;\n> +\n>   private:\n> +\tIPU3Request *cameraRequest(Request *request)\n> +\t{\n> +\t\treturn static_cast<IPU3Request *>(request->_d());\n> +\t}\n> +\n>   \tvoid metadataReady(unsigned int id, const ControlList &metadata);\n>   \tvoid paramsBufferReady(unsigned int id);\n>   \tvoid setSensorControls(unsigned int id, const ControlList &sensorControls,\n> @@ -144,6 +172,8 @@ public:\n>   \tint start(Camera *camera, const ControlList *controls) override;\n>   \tvoid stopDevice(Camera *camera) override;\n>   \n> +\tstd::unique_ptr<Request> createRequestDevice(Camera *camera,\n> +\t\t\t\t\t\t     uint64_t cookie) override;\n>   \tint queueRequestDevice(Camera *camera, Request *request) override;\n>   \n>   \tbool match(DeviceEnumerator *enumerator) override;\n> @@ -680,19 +710,17 @@ int PipelineHandlerIPU3::allocateBuffers(Camera *camera)\n>   \tfor (const std::unique_ptr<FrameBuffer> &buffer : imgu->paramBuffers_) {\n>   \t\tbuffer->setCookie(ipaBufferId++);\n>   \t\tipaBuffers_.emplace_back(buffer->cookie(), buffer->planes());\n> +\t\tdata->availableParamBuffers_.push(buffer.get());\n>   \t}\n>   \n>   \tfor (const std::unique_ptr<FrameBuffer> &buffer : imgu->statBuffers_) {\n>   \t\tbuffer->setCookie(ipaBufferId++);\n>   \t\tipaBuffers_.emplace_back(buffer->cookie(), buffer->planes());\n> +\t\tdata->availableStatBuffers_.push(buffer.get());\n>   \t}\n>   \n>   \tdata->ipa_->mapBuffers(ipaBuffers_);\n>   \n> -\tdata->frameInfos_.init(imgu->paramBuffers_, imgu->statBuffers_);\n> -\tdata->frameInfos_.bufferAvailable.connect(\n> -\t\tdata, &IPU3CameraData::queuePendingRequests);\n> -\n>   \treturn 0;\n>   }\n>   \n> @@ -700,8 +728,6 @@ int PipelineHandlerIPU3::freeBuffers(Camera *camera)\n>   {\n>   \tIPU3CameraData *data = cameraData(camera);\n>   \n> -\tdata->frameInfos_.clear();\n> -\n>   \tstd::vector<unsigned int> ids;\n>   \tfor (IPABuffer &ipabuf : ipaBuffers_)\n>   \t\tids.push_back(ipabuf.id);\n> @@ -777,6 +803,7 @@ void PipelineHandlerIPU3::stopDevice(Camera *camera)\n>   \t\tLOG(IPU3, Warning) << \"Failed to stop camera \" << camera->id();\n>   \n>   \tfreeBuffers(camera);\n> +\tdata->requestMap_.clear();\n>   }\n>   \n>   void IPU3CameraData::cancelPendingRequests()\n> @@ -801,37 +828,76 @@ void IPU3CameraData::queuePendingRequests()\n>   {\n>   \twhile (!pendingRequests_.empty()) {\n>   \t\tRequest *request = pendingRequests_.front();\n> +\t\tIPU3Request *ipu3Request = cameraRequest(request);\n>   \n> -\t\tIPU3Frames::Info *info = frameInfos_.create(request);\n> -\t\tif (!info)\n> -\t\t\tbreak;\n> +\t\trequestMap_[request->sequence()] = ipu3Request;\n>   \n>   \t\t/*\n>   \t\t * Queue a buffer on the CIO2, using the raw stream buffer\n>   \t\t * provided in the request, if any, or a CIO2 internal buffer\n>   \t\t * otherwise.\n> +\t\t *\n> +\t\t * No need to set a buffer->setRequest() as\n> +\t\t * a) If the buffer is provided by the application that's\n> +\t\t *    already been done\n> +\t\t * b) If the buffer comes from the CIO2 internal pool,\n> +\t\t *    CIO2Device::queueBuffer() does that for us\n>   \t\t */\n>   \t\tFrameBuffer *reqRawBuffer = request->findBuffer(&rawStream_);\n>   \t\tFrameBuffer *rawBuffer = cio2_.queueBuffer(request, reqRawBuffer);\n> +\n>   \t\t/*\n>   \t\t * \\todo If queueBuffer fails in queuing a buffer to the device,\n>   \t\t * report the request as error by cancelling the request and\n>   \t\t * calling PipelineHandler::completeRequest().\n>   \t\t */\n> -\t\tif (!rawBuffer) {\n> -\t\t\tframeInfos_.remove(info);\n> +\t\tif (!rawBuffer)\n> +\t\t\tbreak;\n> +\n> +\t\t/*\n> +\t\t * Store the raw buffer queued to the CIO2 to queue it to the\n> +\t\t * Imgu input when the IPA has prepared the parameters buffer.\n> +\t\t */\n> +\t\tipu3Request->rawBuffer = rawBuffer;\n> +\n> +\t\t/*\n> +\t\t * Prepare the stats and parameters buffer and associate them\n> +\t\t * with the currently queued request.\n> +\t\t */\n> +\t\tif (availableParamBuffers_.empty()) {\n> +\t\t\tLOG(IPU3, Warning) << \"Parameters buffer underrun\";\n>   \t\t\tbreak;\n>   \t\t}\n>   \n> -\t\tinfo->rawBuffer = rawBuffer;\n> +\t\tif (availableStatBuffers_.empty()) {\n> +\t\t\tLOG(IPU3, Warning) << \"Statistics buffer underrun\";\n> +\t\t\tbreak;\n> +\t\t}\n>   \n> -\t\tipa_->queueRequest(info->id, request->controls());\n> +\t\tipu3Request->paramBuffer = availableParamBuffers_.front();\n> +\t\tipu3Request->paramDequeued = false;\n> +\t\tipu3Request->paramBuffer->_d()->setRequest(request);\n> +\t\tavailableParamBuffers_.pop();\n> +\n> +\t\tipu3Request->statBuffer = availableStatBuffers_.front();\n> +\t\tipu3Request->metadataProcessed = false;\n> +\t\tipu3Request->statBuffer->_d()->setRequest(request);\n> +\t\tavailableStatBuffers_.pop();\n> +\n> +\t\tipa_->queueRequest(request->sequence(), request->controls());\n>   \n>   \t\tpendingRequests_.pop();\n>   \t\tprocessingRequests_.push(request);\n>   \t}\n>   }\n>   \n> +std::unique_ptr<Request> PipelineHandlerIPU3::createRequestDevice(Camera *camera,\n> +\t\t\t\t\t\t\t\t  uint64_t cookie)\n> +{\n> +\tauto request = std::make_unique<IPU3Request>(camera);\n> +\treturn Request::create(std::move(request), cookie);\n> +}\n> +\n>   int PipelineHandlerIPU3::queueRequestDevice(Camera *camera, Request *request)\n>   {\n>   \tIPU3CameraData *data = cameraData(camera);\n> @@ -1200,6 +1266,26 @@ int IPU3CameraData::loadIPA()\n>   \treturn 0;\n>   }\n>   \n> +void IPU3CameraData::tryCompleteRequest(IPU3Request *request)\n> +{\n> +\tif (request->hasPendingBuffers())\n> +\t\treturn;\n> +\n> +\tif (!request->metadataProcessed)\n> +\t\treturn;\n> +\n> +\tif (!request->paramDequeued)\n> +\t\treturn;\n> +\n> +\tavailableParamBuffers_.push(request->paramBuffer);\n> +\tavailableStatBuffers_.push(request->statBuffer);\n> +\n> +\tpipe()->completeRequest(request->_o<Request>());\n> +\n> +\t/* Try queue another request now that this one has completed. */\n> +\tqueuePendingRequests();\n> +}\n> +\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> @@ -1220,12 +1306,10 @@ void IPU3CameraData::setSensorControls([[maybe_unused]] unsigned int id,\n>   \n>   void IPU3CameraData::paramsBufferReady(unsigned int id)\n>   {\n> -\tIPU3Frames::Info *info = frameInfos_.find(id);\n> -\tif (!info)\n> -\t\treturn;\n> +\tIPU3Request *request = requestMap_[id];\n>   \n>   \t/* Queue all buffers from the request aimed for the ImgU. */\n> -\tfor (auto it : info->request->buffers()) {\n> +\tfor (auto it : request->_o<Request>()->buffers()) {\n>   \t\tconst Stream *stream = it.first;\n>   \t\tFrameBuffer *outbuffer = it.second;\n>   \n> @@ -1235,25 +1319,21 @@ void IPU3CameraData::paramsBufferReady(unsigned int id)\n>   \t\t\timgu_->viewfinder_->queueBuffer(outbuffer);\n>   \t}\n>   \n> -\tinfo->paramBuffer->_d()->metadata().planes()[0].bytesused =\n> +\trequest->paramBuffer->_d()->metadata().planes()[0].bytesused =\n>   \t\tsizeof(struct ipu3_uapi_params);\n> -\timgu_->param_->queueBuffer(info->paramBuffer);\n> -\timgu_->stat_->queueBuffer(info->statBuffer);\n> -\timgu_->input_->queueBuffer(info->rawBuffer);\n> +\timgu_->param_->queueBuffer(request->paramBuffer);\n> +\timgu_->stat_->queueBuffer(request->statBuffer);\n> +\timgu_->input_->queueBuffer(request->rawBuffer);\n>   }\n>   \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> +\tIPU3Request *request = requestMap_[id];\n>   \n> -\tRequest *request = info->request;\n> -\trequest->metadata().merge(metadata);\n> +\trequest->_o<Request>()->metadata().merge(metadata);\n>   \n> -\tinfo->metadataProcessed = true;\n> -\tif (frameInfos_.tryComplete(info))\n> -\t\tpipe()->completeRequest(request);\n> +\trequest->metadataProcessed = true;\n> +\ttryCompleteRequest(request);\n>   }\n>   \n>   /* -----------------------------------------------------------------------------\n> @@ -1268,11 +1348,7 @@ void IPU3CameraData::metadataReady(unsigned int id, const ControlList &metadata)\n>    */\n>   void IPU3CameraData::imguOutputBufferReady(FrameBuffer *buffer)\n>   {\n> -\tIPU3Frames::Info *info = frameInfos_.find(buffer);\n> -\tif (!info)\n> -\t\treturn;\n> -\n> -\tRequest *request = info->request;\n> +\tRequest *request = buffer->request();\n>   \n>   \tpipe()->completeBuffer(request, buffer);\n>   \n> @@ -1283,8 +1359,7 @@ void IPU3CameraData::imguOutputBufferReady(FrameBuffer *buffer)\n>   \t\tcropRegion_ = *scalerCrop;\n>   \trequest->metadata().set(controls::ScalerCrop, cropRegion_);\n>   \n> -\tif (frameInfos_.tryComplete(info))\n> -\t\tpipe()->completeRequest(request);\n> +\ttryCompleteRequest(cameraRequest(request));\n>   }\n>   \n>   /**\n> @@ -1296,22 +1371,20 @@ void IPU3CameraData::imguOutputBufferReady(FrameBuffer *buffer)\n>    */\n>   void IPU3CameraData::cio2BufferReady(FrameBuffer *buffer)\n>   {\n> -\tIPU3Frames::Info *info = frameInfos_.find(buffer);\n> -\tif (!info)\n> -\t\treturn;\n> -\n> -\tRequest *request = info->request;\n> +\tIPU3Request *request = cameraRequest(buffer->request());\n>   \n>   \t/* If the buffer is cancelled force a complete of the whole request. */\n>   \tif (buffer->metadata().status == FrameMetadata::FrameCancelled) {\n> -\t\tfor (auto it : request->buffers()) {\n> +\t\tfor (auto it : request->_o<Request>()->buffers()) {\n>   \t\t\tFrameBuffer *b = it.second;\n>   \t\t\tb->_d()->cancel();\n> -\t\t\tpipe()->completeBuffer(request, b);\n> +\t\t\tpipe()->completeBuffer(request->_o<Request>(), b);\n>   \t\t}\n>   \n> -\t\tframeInfos_.remove(info);\n> -\t\tpipe()->completeRequest(request);\n> +\t\tavailableParamBuffers_.push(request->paramBuffer);\n> +\t\tavailableStatBuffers_.push(request->statBuffer);\n> +\n> +\t\tpipe()->completeRequest(request->_o<Request>());\n>   \t\treturn;\n>   \t}\n>   \n> @@ -1321,24 +1394,23 @@ void IPU3CameraData::cio2BufferReady(FrameBuffer *buffer)\n>   \t * \\todo The sensor timestamp should be better estimated by connecting\n>   \t * to the V4L2Device::frameStart signal.\n>   \t */\n> -\trequest->metadata().set(controls::SensorTimestamp,\n> -\t\t\t\tbuffer->metadata().timestamp);\n> +\trequest->_o<Request>()->metadata().set(controls::SensorTimestamp,\n> +\t\t\t\t\t       buffer->metadata().timestamp);\n>   \n> -\tinfo->effectiveSensorControls = delayedCtrls_->get(buffer->metadata().sequence);\n> +\trequest->effectiveSensorControls =\n> +\t\tdelayedCtrls_->get(buffer->metadata().sequence);\n>   \n> -\tif (request->findBuffer(&rawStream_))\n> -\t\tpipe()->completeBuffer(request, buffer);\n> +\tif (request->_o<Request>()->findBuffer(&rawStream_))\n> +\t\tpipe()->completeBuffer(request->_o<Request>(), buffer);\n>   \n> -\tipa_->fillParamsBuffer(info->id, info->paramBuffer->cookie());\n> +\tipa_->fillParamsBuffer(request->_o<Request>()->sequence(),\n> +\t\t\t       request->paramBuffer->cookie());\n>   }\n>   \n>   void IPU3CameraData::paramBufferReady(FrameBuffer *buffer)\n>   {\n> -\tIPU3Frames::Info *info = frameInfos_.find(buffer);\n> -\tif (!info)\n> -\t\treturn;\n> -\n> -\tinfo->paramDequeued = true;\n> +\tIPU3Request *request = cameraRequest(buffer->request());\n> +\trequest->paramDequeued = true;\n>   \n>   \t/*\n>   \t * tryComplete() will delete info if it completes the IPU3Frame.\n> @@ -1346,35 +1418,25 @@ void IPU3CameraData::paramBufferReady(FrameBuffer *buffer)\n>   \t *\n>   \t * \\todo Improve the FrameInfo API to avoid this type of issue\n>   \t */\n> -\tRequest *request = info->request;\n>   \n> -\tif (frameInfos_.tryComplete(info))\n> -\t\tpipe()->completeRequest(request);\n> +\ttryCompleteRequest(request);\n>   }\n>   \n>   void IPU3CameraData::statBufferReady(FrameBuffer *buffer)\n>   {\n> -\tIPU3Frames::Info *info = frameInfos_.find(buffer);\n> -\tif (!info)\n> -\t\treturn;\n> -\n> -\tRequest *request = info->request;\n> +\tIPU3Request *request = cameraRequest(buffer->request());\n>   \n>   \tif (buffer->metadata().status == FrameMetadata::FrameCancelled) {\n> -\t\tinfo->metadataProcessed = true;\n> +\t\trequest->metadataProcessed = true;\n>   \n> -\t\t/*\n> -\t\t * tryComplete() will delete info if it completes the IPU3Frame.\n> -\t\t * In that event, we must have obtained the Request before hand.\n> -\t\t */\n> -\t\tif (frameInfos_.tryComplete(info))\n> -\t\t\tpipe()->completeRequest(request);\n> +\t\ttryCompleteRequest(request);\n>   \n>   \t\treturn;\n>   \t}\n>   \n> -\tipa_->processStatsBuffer(info->id, request->metadata().get(controls::SensorTimestamp).value_or(0),\n> -\t\t\t\t info->statBuffer->cookie(), info->effectiveSensorControls);\n> +\tipa_->processStatsBuffer(request->_o<Request>()->sequence(),\n> +\t\t\t\t request->_o<Request>()->metadata().get(controls::SensorTimestamp).value_or(0),\n> +\t\t\t\t request->statBuffer->cookie(), request->effectiveSensorControls);\n>   }\n>   \n>   /*\n> diff --git a/src/libcamera/pipeline/ipu3/meson.build b/src/libcamera/pipeline/ipu3/meson.build\n> index a1b0b31ac5bc..d60e07ae6cca 100644\n> --- a/src/libcamera/pipeline/ipu3/meson.build\n> +++ b/src/libcamera/pipeline/ipu3/meson.build\n> @@ -2,7 +2,6 @@\n>   \n>   libcamera_sources += files([\n>       'cio2.cpp',\n> -    'frames.cpp',\n>       'imgu.cpp',\n>       'ipu3.cpp',\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 7F95EBD160\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon, 11 Mar 2024 16:04:54 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E538562C80;\n\tMon, 11 Mar 2024 17:04:53 +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 96DD461C7C\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 11 Mar 2024 17:04:52 +0100 (CET)","from [192.168.0.43]\n\t(cpc141996-chfd3-2-0-cust928.12-3.cable.virginm.net [86.13.91.161])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 3BCC06BE\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 11 Mar 2024 17:04:31 +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=\"ac4H3cU2\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1710173071;\n\tbh=4T3F2ZWTOznCZ9uJe0UcJRfMe4Z9+soy8m27/uGGYBg=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=ac4H3cU2NLGcKXSPUYrTk6nLPjROBjj4rxRTrWy9w720TsEbyD6Bl2BYKwRxBxlJj\n\t406HUzdtuLZ9NOA14+77uJW75jYnkPmp1FGt4vGkPqso8iE5oYLuyYD4NNUIWSeYLY\n\too7t9FbAxcw7GSCR1QmBLccudTNpkBEFuRC0mXOE=","Message-ID":"<7964f2a1-de1b-40fd-9675-91806d2a4729@ideasonboard.com>","Date":"Mon, 11 Mar 2024 16:04:49 +0000","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v2 3/4] libcamera: ipu3: Replace IPU3FrameInfo","Content-Language":"en-US","To":"libcamera-devel@lists.libcamera.org","References":"<20240311123234.32925-1-jacopo.mondi@ideasonboard.com>\n\t<20240311123234.32925-4-jacopo.mondi@ideasonboard.com>","From":"Dan Scally <dan.scally@ideasonboard.com>","Autocrypt":"addr=dan.scally@ideasonboard.com; keydata=\n\txsFNBGLydlEBEADa5O2s0AbUguprfvXOQun/0a8y2Vk6BqkQALgeD6KnXSWwaoCULp18etYW\n\tB31bfgrdphXQ5kUQibB0ADK8DERB4wrzrUb5CMxLBFE7mQty+v5NsP0OFNK9XTaAOcmD+Ove\n\teIjYvqurAaro91jrRVrS1gBRxIFqyPgNvwwL+alMZhn3/2jU2uvBmuRrgnc/e9cHKiuT3Dtq\n\tMHGPKL2m+plk+7tjMoQFfexoQ1JKugHAjxAhJfrkXh6uS6rc01bYCyo7ybzg53m1HLFJdNGX\n\tsUKR+dQpBs3SY4s66tc1sREJqdYyTsSZf80HjIeJjU/hRunRo4NjRIJwhvnK1GyjOvvuCKVU\n\tRWpY8dNjNu5OeAfdrlvFJOxIE9M8JuYCQTMULqd1NuzbpFMjc9524U3Cngs589T7qUMPb1H1\n\tNTA81LmtJ6Y+IV5/kiTUANflpzBwhu18Ok7kGyCq2a2jsOcVmk8gZNs04gyjuj8JziYwwLbf\n\tvzABwpFVcS8aR+nHIZV1HtOzyw8CsL8OySc3K9y+Y0NRpziMRvutrppzgyMb9V+N31mK9Mxl\n\t1YkgaTl4ciNWpdfUe0yxH03OCuHi3922qhPLF4XX5LN+NaVw5Xz2o3eeWklXdouxwV7QlN33\n\tu4+u2FWzKxDqO6WLQGjxPE0mVB4Gh5Pa1Vb0ct9Ctg0qElvtGQARAQABzShEYW4gU2NhbGx5\n\tIDxkYW4uc2NhbGx5QGlkZWFzb25ib2FyZC5jb20+wsGNBBMBCAA3FiEEsdtt8OWP7+8SNfQe\n\tkiQuh/L+GMQFAmLydlIFCQWjmoACGwMECwkIBwUVCAkKCwUWAgMBAAAKCRCSJC6H8v4YxDI2\n\tEAC2Gz0iyaXJkPInyshrREEWbo0CA6v5KKf3I/HlMPqkZ48bmGoYm4mEQGFWZJAT3K4ir8bg\n\tcEfs9V54gpbrZvdwS4abXbUK4WjKwEs8HK3XJv1WXUN2bsz5oEJWZUImh9gD3naiLLI9QMMm\n\tw/aZkT+NbN5/2KvChRWhdcha7+2Te4foOY66nIM+pw2FZM6zIkInLLUik2zXOhaZtqdeJZQi\n\tHSPU9xu7TRYN4cvdZAnSpG7gQqmLm5/uGZN1/sB3kHTustQtSXKMaIcD/DMNI3JN/t+RJVS7\n\tc0Jh/ThzTmhHyhxx3DRnDIy7kwMI4CFvmhkVC2uNs9kWsj1DuX5kt8513mvfw2OcX9UnNKmZ\n\tnhNCuF6DxVrL8wjOPuIpiEj3V+K7DFF1Cxw1/yrLs8dYdYh8T8vCY2CHBMsqpESROnTazboh\n\tAiQ2xMN1cyXtX11Qwqm5U3sykpLbx2BcmUUUEAKNsM//Zn81QXKG8vOx0ZdMfnzsCaCzt8f6\n\t9dcDBBI3tJ0BI9ByiocqUoL6759LM8qm18x3FYlxvuOs4wSGPfRVaA4yh0pgI+ModVC2Pu3y\n\tejE/IxeatGqJHh6Y+iJzskdi27uFkRixl7YJZvPJAbEn7kzSi98u/5ReEA8Qhc8KO/B7wprj\n\txjNMZNYd0Eth8+WkixHYj752NT5qshKJXcyUU87BTQRi8nZSARAAx0BJayh1Fhwbf4zoY56x\n\txHEpT6DwdTAYAetd3yiKClLVJadYxOpuqyWa1bdfQWPb+h4MeXbWw/53PBgn7gI2EA7ebIRC\n\tPJJhAIkeym7hHZoxqDQTGDJjxFEL11qF+U3rhWiL2Zt0Pl+zFq0eWYYVNiXjsIS4FI2+4m16\n\ttPbDWZFJnSZ828VGtRDQdhXfx3zyVX21lVx1bX4/OZvIET7sVUufkE4hrbqrrufre7wsjD1t\n\t8MQKSapVrr1RltpzPpScdoxknOSBRwOvpp57pJJe5A0L7+WxJ+vQoQXj0j+5tmIWOAV1qBQp\n\thyoyUk9JpPfntk2EKnZHWaApFp5TcL6c5LhUvV7F6XwOjGPuGlZQCWXee9dr7zym8iR3irWT\n\t+49bIh5PMlqSLXJDYbuyFQHFxoiNdVvvf7etvGfqFYVMPVjipqfEQ38ST2nkzx+KBICz7uwj\n\tJwLBdTXzGFKHQNckGMl7F5QdO/35An/QcxBnHVMXqaSd12tkJmoRVWduwuuoFfkTY5mUV3uX\n\txGj3iVCK4V+ezOYA7c2YolfRCNMTza6vcK/P4tDjjsyBBZrCCzhBvd4VVsnnlZhVaIxoky4K\n\taL+AP+zcQrUZmXmgZjXOLryGnsaeoVrIFyrU6ly90s1y3KLoPsDaTBMtnOdwxPmo1xisH8oL\n\ta/VRgpFBfojLPxMAEQEAAcLBfAQYAQgAJhYhBLHbbfDlj+/vEjX0HpIkLofy/hjEBQJi8nZT\n\tBQkFo5qAAhsMAAoJEJIkLofy/hjEXPcQAMIPNqiWiz/HKu9W4QIf1OMUpKn3YkVIj3p3gvfM\n\tRes4fGX94Ji599uLNrPoxKyaytC4R6BTxVriTJjWK8mbo9jZIRM4vkwkZZ2bu98EweSucxbp\n\tvjESsvMXGgxniqV/RQ/3T7LABYRoIUutARYq58p5HwSP0frF0fdFHYdTa2g7MYZl1ur2JzOC\n\tFHRpGadlNzKDE3fEdoMobxHB3Lm6FDml5GyBAA8+dQYVI0oDwJ3gpZPZ0J5Vx9RbqXe8RDuR\n\tdu90hvCJkq7/tzSQ0GeD3BwXb9/R/A4dVXhaDd91Q1qQXidI+2jwhx8iqiYxbT+DoAUkQRQy\n\txBtoCM1CxH7u45URUgD//fxYr3D4B1SlonA6vdaEdHZOGwECnDpTxecENMbz/Bx7qfrmd901\n\tD+N9SjIwrbVhhSyUXYnSUb8F+9g2RDY42Sk7GcYxIeON4VzKqWM7hpkXZ47pkK0YodO+dRKM\n\tyMcoUWrTK0Uz6UzUGKoJVbxmSW/EJLEGoI5p3NWxWtScEVv8mO49gqQdrRIOheZycDmHnItt\n\t9Qjv00uFhEwv2YfiyGk6iGF2W40s2pH2t6oeuGgmiZ7g6d0MEK8Ql/4zPItvr1c1rpwpXUC1\n\tu1kQWgtnNjFHX3KiYdqjcZeRBiry1X0zY+4Y24wUU0KsEewJwjhmCKAsju1RpdlPg2kC","In-Reply-To":"<20240311123234.32925-4-jacopo.mondi@ideasonboard.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","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":28928,"web_url":"https://patchwork.libcamera.org/comment/28928/","msgid":"<171024284783.252503.16918205638879056926@ping.linuxembedded.co.uk>","date":"2024-03-12T11:27:27","subject":"Re: [PATCH v2 3/4] libcamera: ipu3: Replace IPU3FrameInfo","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Jacopo Mondi (2024-03-11 12:32:31)\n> Now that the pipeline handler can create a private derived class of\n> Request::Private, use it to store the pipeline specific data.\n> \n> In the case of IPU3 we associate statistics and paramters buffer with a\n> Request and a raw buffer which gets dequeued from the CIO2 and input\n> to the ImgU input.\n> \n> This replaces the functionalities of IPU3FrameInfo which can now be\n> removed.\n\nThe only comments I have here are syntactic sugar that could make using\nthe pipeline specific request easier but maybe that might just obfuscate\nthings anyway - so I still think this is a good way forwards.\n\n\nReviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n\n> \n> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>\n> ---\n>  src/libcamera/pipeline/ipu3/frames.cpp  | 143 ----------------\n>  src/libcamera/pipeline/ipu3/frames.h    |  67 --------\n>  src/libcamera/pipeline/ipu3/ipu3.cpp    | 212 +++++++++++++++---------\n>  src/libcamera/pipeline/ipu3/meson.build |   1 -\n>  4 files changed, 137 insertions(+), 286 deletions(-)\n>  delete mode 100644 src/libcamera/pipeline/ipu3/frames.cpp\n>  delete mode 100644 src/libcamera/pipeline/ipu3/frames.h\n> \n> diff --git a/src/libcamera/pipeline/ipu3/frames.cpp b/src/libcamera/pipeline/ipu3/frames.cpp\n> deleted file mode 100644\n> index a4c3477cd9ef..000000000000\n> --- a/src/libcamera/pipeline/ipu3/frames.cpp\n> +++ /dev/null\n> @@ -1,143 +0,0 @@\n> -/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> -/*\n> - * Copyright (C) 2020, Google Inc.\n> - *\n> - * frames.cpp - Intel IPU3 Frames helper\n> - */\n> -\n> -#include \"frames.h\"\n> -\n> -#include <libcamera/framebuffer.h>\n> -#include <libcamera/request.h>\n> -\n> -#include \"libcamera/internal/framebuffer.h\"\n> -#include \"libcamera/internal/pipeline_handler.h\"\n> -#include \"libcamera/internal/v4l2_videodevice.h\"\n> -\n> -namespace libcamera {\n> -\n> -LOG_DECLARE_CATEGORY(IPU3)\n> -\n> -IPU3Frames::IPU3Frames()\n> -{\n> -}\n> -\n> -void IPU3Frames::init(const std::vector<std::unique_ptr<FrameBuffer>> &paramBuffers,\n> -                     const std::vector<std::unique_ptr<FrameBuffer>> &statBuffers)\n> -{\n> -       for (const std::unique_ptr<FrameBuffer> &buffer : paramBuffers)\n> -               availableParamBuffers_.push(buffer.get());\n> -\n> -       for (const std::unique_ptr<FrameBuffer> &buffer : statBuffers)\n> -               availableStatBuffers_.push(buffer.get());\n> -\n> -       frameInfo_.clear();\n> -}\n> -\n> -void IPU3Frames::clear()\n> -{\n> -       availableParamBuffers_ = {};\n> -       availableStatBuffers_ = {};\n> -}\n> -\n> -IPU3Frames::Info *IPU3Frames::create(Request *request)\n> -{\n> -       unsigned int id = request->sequence();\n> -\n> -       if (availableParamBuffers_.empty()) {\n> -               LOG(IPU3, Debug) << \"Parameters buffer underrun\";\n> -               return nullptr;\n> -       }\n> -\n> -       if (availableStatBuffers_.empty()) {\n> -               LOG(IPU3, Debug) << \"Statistics buffer underrun\";\n> -               return nullptr;\n> -       }\n> -\n> -       FrameBuffer *paramBuffer = availableParamBuffers_.front();\n> -       FrameBuffer *statBuffer = availableStatBuffers_.front();\n> -\n> -       paramBuffer->_d()->setRequest(request);\n> -       statBuffer->_d()->setRequest(request);\n> -\n> -       availableParamBuffers_.pop();\n> -       availableStatBuffers_.pop();\n> -\n> -       /* \\todo Remove the dynamic allocation of Info */\n> -       std::unique_ptr<Info> info = std::make_unique<Info>();\n> -\n> -       info->id = id;\n> -       info->request = request;\n> -       info->rawBuffer = nullptr;\n> -       info->paramBuffer = paramBuffer;\n> -       info->statBuffer = statBuffer;\n> -       info->paramDequeued = false;\n> -       info->metadataProcessed = false;\n> -\n> -       frameInfo_[id] = std::move(info);\n> -\n> -       return frameInfo_[id].get();\n> -}\n> -\n> -void IPU3Frames::remove(IPU3Frames::Info *info)\n> -{\n> -       /* Return params and stat buffer for reuse. */\n> -       availableParamBuffers_.push(info->paramBuffer);\n> -       availableStatBuffers_.push(info->statBuffer);\n> -\n> -       /* Delete the extended frame information. */\n> -       frameInfo_.erase(info->id);\n> -}\n> -\n> -bool IPU3Frames::tryComplete(IPU3Frames::Info *info)\n> -{\n> -       Request *request = info->request;\n> -\n> -       if (request->hasPendingBuffers())\n> -               return false;\n> -\n> -       if (!info->metadataProcessed)\n> -               return false;\n> -\n> -       if (!info->paramDequeued)\n> -               return false;\n> -\n> -       remove(info);\n> -\n> -       bufferAvailable.emit();\n> -\n> -       return true;\n> -}\n> -\n> -IPU3Frames::Info *IPU3Frames::find(unsigned int id)\n> -{\n> -       const auto &itInfo = frameInfo_.find(id);\n> -\n> -       if (itInfo != frameInfo_.end())\n> -               return itInfo->second.get();\n> -\n> -       LOG(IPU3, Fatal) << \"Can't find tracking information for frame \" << id;\n> -\n> -       return nullptr;\n> -}\n> -\n> -IPU3Frames::Info *IPU3Frames::find(FrameBuffer *buffer)\n> -{\n> -       for (auto const &itInfo : frameInfo_) {\n> -               Info *info = itInfo.second.get();\n> -\n> -               for (auto const itBuffers : info->request->buffers())\n> -                       if (itBuffers.second == buffer)\n> -                               return info;\n> -\n> -               if (info->rawBuffer == buffer || info->paramBuffer == buffer ||\n> -                   info->statBuffer == buffer)\n> -                       return info;\n> -       }\n> -\n> -       LOG(IPU3, Fatal) << \"Can't find tracking information from buffer\";\n> -\n> -       return nullptr;\n> -}\n> -\n> -} /* namespace libcamera */\n> diff --git a/src/libcamera/pipeline/ipu3/frames.h b/src/libcamera/pipeline/ipu3/frames.h\n> deleted file mode 100644\n> index 6e3cb915c7b8..000000000000\n> --- a/src/libcamera/pipeline/ipu3/frames.h\n> +++ /dev/null\n> @@ -1,67 +0,0 @@\n> -/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> -/*\n> - * Copyright (C) 2020, Google Inc.\n> - *\n> - * frames.h - Intel IPU3 Frames helper\n> - */\n> -\n> -#pragma once\n> -\n> -#include <map>\n> -#include <memory>\n> -#include <queue>\n> -#include <vector>\n> -\n> -#include <libcamera/base/signal.h>\n> -\n> -#include <libcamera/controls.h>\n> -\n> -namespace libcamera {\n> -\n> -class FrameBuffer;\n> -class IPAProxy;\n> -class PipelineHandler;\n> -class Request;\n> -class V4L2VideoDevice;\n> -struct IPABuffer;\n> -\n> -class IPU3Frames\n> -{\n> -public:\n> -       struct Info {\n> -               unsigned int id;\n> -               Request *request;\n> -\n> -               FrameBuffer *rawBuffer;\n> -               FrameBuffer *paramBuffer;\n> -               FrameBuffer *statBuffer;\n> -\n> -               ControlList effectiveSensorControls;\n> -\n> -               bool paramDequeued;\n> -               bool metadataProcessed;\n> -       };\n> -\n> -       IPU3Frames();\n> -\n> -       void init(const std::vector<std::unique_ptr<FrameBuffer>> &paramBuffers,\n> -                 const std::vector<std::unique_ptr<FrameBuffer>> &statBuffers);\n> -       void clear();\n> -\n> -       Info *create(Request *request);\n> -       void remove(Info *info);\n> -       bool tryComplete(Info *info);\n> -\n> -       Info *find(unsigned int id);\n> -       Info *find(FrameBuffer *buffer);\n> -\n> -       Signal<> bufferAvailable;\n> -\n> -private:\n> -       std::queue<FrameBuffer *> availableParamBuffers_;\n> -       std::queue<FrameBuffer *> availableStatBuffers_;\n> -\n> -       std::map<unsigned int, std::unique_ptr<Info>> frameInfo_;\n> -};\n> -\n> -} /* namespace libcamera */\n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index fa4bd0bb73e2..0c9d3167d2e6 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -34,9 +34,9 @@\n>  #include \"libcamera/internal/ipa_manager.h\"\n>  #include \"libcamera/internal/media_device.h\"\n>  #include \"libcamera/internal/pipeline_handler.h\"\n> +#include \"libcamera/internal/request.h\"\n>  \n>  #include \"cio2.h\"\n> -#include \"frames.h\"\n>  #include \"imgu.h\"\n>  \n>  namespace libcamera {\n> @@ -47,6 +47,24 @@ static const ControlInfoMap::Map IPU3Controls = {\n>         { &controls::draft::PipelineDepth, ControlInfo(2, 3) },\n>  };\n>  \n> +class IPU3Request : public Request::Private\n> +{\n> +public:\n> +       IPU3Request(Camera *camera)\n> +               : Request::Private(camera)\n> +       {\n> +       }\n> +\n> +       FrameBuffer *rawBuffer;\n> +       FrameBuffer *paramBuffer;\n> +       FrameBuffer *statBuffer;\n> +\n> +       ControlList effectiveSensorControls;\n> +\n> +       bool paramDequeued;\n> +       bool metadataProcessed;\n> +};\n> +\n>  class IPU3CameraData : public Camera::Private\n>  {\n>  public:\n> @@ -57,6 +75,7 @@ public:\n>  \n>         int loadIPA();\n>  \n> +       void tryCompleteRequest(IPU3Request *request);\n>         void imguOutputBufferReady(FrameBuffer *buffer);\n>         void cio2BufferReady(FrameBuffer *buffer);\n>         void paramBufferReady(FrameBuffer *buffer);\n> @@ -75,7 +94,6 @@ public:\n>         Rectangle cropRegion_;\n>  \n>         std::unique_ptr<DelayedControls> delayedCtrls_;\n> -       IPU3Frames frameInfos_;\n>  \n>         std::unique_ptr<ipa::ipu3::IPAProxyIPU3> ipa_;\n>  \n> @@ -86,7 +104,17 @@ public:\n>  \n>         ControlInfoMap ipaControls_;\n>  \n> +       std::map<unsigned int, IPU3Request *> requestMap_;\n> +\n> +       std::queue<FrameBuffer *> availableParamBuffers_;\n> +       std::queue<FrameBuffer *> availableStatBuffers_;\n> +\n>  private:\n> +       IPU3Request *cameraRequest(Request *request)\n> +       {\n> +               return static_cast<IPU3Request *>(request->_d());\n> +       }\n> +\n>         void metadataReady(unsigned int id, const ControlList &metadata);\n>         void paramsBufferReady(unsigned int id);\n>         void setSensorControls(unsigned int id, const ControlList &sensorControls,\n> @@ -144,6 +172,8 @@ public:\n>         int start(Camera *camera, const ControlList *controls) override;\n>         void stopDevice(Camera *camera) override;\n>  \n> +       std::unique_ptr<Request> createRequestDevice(Camera *camera,\n> +                                                    uint64_t cookie) override;\n>         int queueRequestDevice(Camera *camera, Request *request) override;\n>  \n>         bool match(DeviceEnumerator *enumerator) override;\n> @@ -680,19 +710,17 @@ int PipelineHandlerIPU3::allocateBuffers(Camera *camera)\n>         for (const std::unique_ptr<FrameBuffer> &buffer : imgu->paramBuffers_) {\n>                 buffer->setCookie(ipaBufferId++);\n>                 ipaBuffers_.emplace_back(buffer->cookie(), buffer->planes());\n> +               data->availableParamBuffers_.push(buffer.get());\n>         }\n>  \n>         for (const std::unique_ptr<FrameBuffer> &buffer : imgu->statBuffers_) {\n>                 buffer->setCookie(ipaBufferId++);\n>                 ipaBuffers_.emplace_back(buffer->cookie(), buffer->planes());\n> +               data->availableStatBuffers_.push(buffer.get());\n>         }\n>  \n>         data->ipa_->mapBuffers(ipaBuffers_);\n>  \n> -       data->frameInfos_.init(imgu->paramBuffers_, imgu->statBuffers_);\n> -       data->frameInfos_.bufferAvailable.connect(\n> -               data, &IPU3CameraData::queuePendingRequests);\n> -\n>         return 0;\n>  }\n>  \n> @@ -700,8 +728,6 @@ int PipelineHandlerIPU3::freeBuffers(Camera *camera)\n>  {\n>         IPU3CameraData *data = cameraData(camera);\n>  \n> -       data->frameInfos_.clear();\n> -\n>         std::vector<unsigned int> ids;\n>         for (IPABuffer &ipabuf : ipaBuffers_)\n>                 ids.push_back(ipabuf.id);\n> @@ -777,6 +803,7 @@ void PipelineHandlerIPU3::stopDevice(Camera *camera)\n>                 LOG(IPU3, Warning) << \"Failed to stop camera \" << camera->id();\n>  \n>         freeBuffers(camera);\n> +       data->requestMap_.clear();\n>  }\n>  \n>  void IPU3CameraData::cancelPendingRequests()\n> @@ -801,37 +828,76 @@ void IPU3CameraData::queuePendingRequests()\n>  {\n>         while (!pendingRequests_.empty()) {\n>                 Request *request = pendingRequests_.front();\n> +               IPU3Request *ipu3Request = cameraRequest(request);\n>  \n> -               IPU3Frames::Info *info = frameInfos_.create(request);\n> -               if (!info)\n> -                       break;\n> +               requestMap_[request->sequence()] = ipu3Request;\n>  \n>                 /*\n>                  * Queue a buffer on the CIO2, using the raw stream buffer\n>                  * provided in the request, if any, or a CIO2 internal buffer\n>                  * otherwise.\n> +                *\n> +                * No need to set a buffer->setRequest() as\n> +                * a) If the buffer is provided by the application that's\n> +                *    already been done\n> +                * b) If the buffer comes from the CIO2 internal pool,\n> +                *    CIO2Device::queueBuffer() does that for us\n>                  */\n>                 FrameBuffer *reqRawBuffer = request->findBuffer(&rawStream_);\n>                 FrameBuffer *rawBuffer = cio2_.queueBuffer(request, reqRawBuffer);\n> +\n>                 /*\n>                  * \\todo If queueBuffer fails in queuing a buffer to the device,\n>                  * report the request as error by cancelling the request and\n>                  * calling PipelineHandler::completeRequest().\n>                  */\n> -               if (!rawBuffer) {\n> -                       frameInfos_.remove(info);\n> +               if (!rawBuffer)\n> +                       break;\n> +\n> +               /*\n> +                * Store the raw buffer queued to the CIO2 to queue it to the\n> +                * Imgu input when the IPA has prepared the parameters buffer.\n> +                */\n> +               ipu3Request->rawBuffer = rawBuffer;\n> +\n> +               /*\n> +                * Prepare the stats and parameters buffer and associate them\n> +                * with the currently queued request.\n> +                */\n> +               if (availableParamBuffers_.empty()) {\n> +                       LOG(IPU3, Warning) << \"Parameters buffer underrun\";\n>                         break;\n>                 }\n>  \n> -               info->rawBuffer = rawBuffer;\n> +               if (availableStatBuffers_.empty()) {\n> +                       LOG(IPU3, Warning) << \"Statistics buffer underrun\";\n> +                       break;\n> +               }\n>  \n> -               ipa_->queueRequest(info->id, request->controls());\n> +               ipu3Request->paramBuffer = availableParamBuffers_.front();\n> +               ipu3Request->paramDequeued = false;\n> +               ipu3Request->paramBuffer->_d()->setRequest(request);\n> +               availableParamBuffers_.pop();\n> +\n> +               ipu3Request->statBuffer = availableStatBuffers_.front();\n> +               ipu3Request->metadataProcessed = false;\n> +               ipu3Request->statBuffer->_d()->setRequest(request);\n> +               availableStatBuffers_.pop();\n> +\n> +               ipa_->queueRequest(request->sequence(), request->controls());\n>  \n>                 pendingRequests_.pop();\n>                 processingRequests_.push(request);\n>         }\n>  }\n>  \n> +std::unique_ptr<Request> PipelineHandlerIPU3::createRequestDevice(Camera *camera,\n> +                                                                 uint64_t cookie)\n> +{\n> +       auto request = std::make_unique<IPU3Request>(camera);\n> +       return Request::create(std::move(request), cookie);\n> +}\n> +\n>  int PipelineHandlerIPU3::queueRequestDevice(Camera *camera, Request *request)\n>  {\n>         IPU3CameraData *data = cameraData(camera);\n> @@ -1200,6 +1266,26 @@ int IPU3CameraData::loadIPA()\n>         return 0;\n>  }\n>  \n> +void IPU3CameraData::tryCompleteRequest(IPU3Request *request)\n> +{\n> +       if (request->hasPendingBuffers())\n> +               return;\n> +\n> +       if (!request->metadataProcessed)\n> +               return;\n> +\n> +       if (!request->paramDequeued)\n> +               return;\n> +\n> +       availableParamBuffers_.push(request->paramBuffer);\n> +       availableStatBuffers_.push(request->statBuffer);\n> +\n> +       pipe()->completeRequest(request->_o<Request>());\n> +\n> +       /* Try queue another request now that this one has completed. */\n> +       queuePendingRequests();\n> +}\n> +\n>  void IPU3CameraData::setSensorControls([[maybe_unused]] unsigned int id,\n>                                        const ControlList &sensorControls,\n>                                        const ControlList &lensControls)\n> @@ -1220,12 +1306,10 @@ void IPU3CameraData::setSensorControls([[maybe_unused]] unsigned int id,\n>  \n>  void IPU3CameraData::paramsBufferReady(unsigned int id)\n>  {\n> -       IPU3Frames::Info *info = frameInfos_.find(id);\n> -       if (!info)\n> -               return;\n> +       IPU3Request *request = requestMap_[id];\n>  \n>         /* Queue all buffers from the request aimed for the ImgU. */\n> -       for (auto it : info->request->buffers()) {\n> +       for (auto it : request->_o<Request>()->buffers()) {\n>                 const Stream *stream = it.first;\n>                 FrameBuffer *outbuffer = it.second;\n>  \n> @@ -1235,25 +1319,21 @@ void IPU3CameraData::paramsBufferReady(unsigned int id)\n>                         imgu_->viewfinder_->queueBuffer(outbuffer);\n>         }\n>  \n> -       info->paramBuffer->_d()->metadata().planes()[0].bytesused =\n> +       request->paramBuffer->_d()->metadata().planes()[0].bytesused =\n>                 sizeof(struct ipu3_uapi_params);\n> -       imgu_->param_->queueBuffer(info->paramBuffer);\n> -       imgu_->stat_->queueBuffer(info->statBuffer);\n> -       imgu_->input_->queueBuffer(info->rawBuffer);\n> +       imgu_->param_->queueBuffer(request->paramBuffer);\n> +       imgu_->stat_->queueBuffer(request->statBuffer);\n> +       imgu_->input_->queueBuffer(request->rawBuffer);\n>  }\n>  \n>  void IPU3CameraData::metadataReady(unsigned int id, const ControlList &metadata)\n>  {\n> -       IPU3Frames::Info *info = frameInfos_.find(id);\n> -       if (!info)\n> -               return;\n> +       IPU3Request *request = requestMap_[id];\n>  \n> -       Request *request = info->request;\n> -       request->metadata().merge(metadata);\n> +       request->_o<Request>()->metadata().merge(metadata);\n>  \n> -       info->metadataProcessed = true;\n> -       if (frameInfos_.tryComplete(info))\n> -               pipe()->completeRequest(request);\n> +       request->metadataProcessed = true;\n> +       tryCompleteRequest(request);\n>  }\n>  \n>  /* -----------------------------------------------------------------------------\n> @@ -1268,11 +1348,7 @@ void IPU3CameraData::metadataReady(unsigned int id, const ControlList &metadata)\n>   */\n>  void IPU3CameraData::imguOutputBufferReady(FrameBuffer *buffer)\n>  {\n> -       IPU3Frames::Info *info = frameInfos_.find(buffer);\n> -       if (!info)\n> -               return;\n> -\n> -       Request *request = info->request;\n> +       Request *request = buffer->request();\n>  \n>         pipe()->completeBuffer(request, buffer);\n>  \n> @@ -1283,8 +1359,7 @@ void IPU3CameraData::imguOutputBufferReady(FrameBuffer *buffer)\n>                 cropRegion_ = *scalerCrop;\n>         request->metadata().set(controls::ScalerCrop, cropRegion_);\n>  \n> -       if (frameInfos_.tryComplete(info))\n> -               pipe()->completeRequest(request);\n> +       tryCompleteRequest(cameraRequest(request));\n>  }\n>  \n>  /**\n> @@ -1296,22 +1371,20 @@ void IPU3CameraData::imguOutputBufferReady(FrameBuffer *buffer)\n>   */\n>  void IPU3CameraData::cio2BufferReady(FrameBuffer *buffer)\n>  {\n> -       IPU3Frames::Info *info = frameInfos_.find(buffer);\n> -       if (!info)\n> -               return;\n> -\n> -       Request *request = info->request;\n> +       IPU3Request *request = cameraRequest(buffer->request());\n>  \n>         /* If the buffer is cancelled force a complete of the whole request. */\n>         if (buffer->metadata().status == FrameMetadata::FrameCancelled) {\n> -               for (auto it : request->buffers()) {\n> +               for (auto it : request->_o<Request>()->buffers()) {\n>                         FrameBuffer *b = it.second;\n>                         b->_d()->cancel();\n> -                       pipe()->completeBuffer(request, b);\n> +                       pipe()->completeBuffer(request->_o<Request>(), b);\n>                 }\n>  \n> -               frameInfos_.remove(info);\n> -               pipe()->completeRequest(request);\n> +               availableParamBuffers_.push(request->paramBuffer);\n> +               availableStatBuffers_.push(request->statBuffer);\n> +\n> +               pipe()->completeRequest(request->_o<Request>());\n>                 return;\n>         }\n>  \n> @@ -1321,24 +1394,23 @@ void IPU3CameraData::cio2BufferReady(FrameBuffer *buffer)\n>          * \\todo The sensor timestamp should be better estimated by connecting\n>          * to the V4L2Device::frameStart signal.\n>          */\n> -       request->metadata().set(controls::SensorTimestamp,\n> -                               buffer->metadata().timestamp);\n> +       request->_o<Request>()->metadata().set(controls::SensorTimestamp,\n\nI still think syntatic sugar could be improved here, but that can easily\nbe for later.\n\n> +                                              buffer->metadata().timestamp);\n>  \n> -       info->effectiveSensorControls = delayedCtrls_->get(buffer->metadata().sequence);\n> +       request->effectiveSensorControls =\n> +               delayedCtrls_->get(buffer->metadata().sequence);\n>  \n> -       if (request->findBuffer(&rawStream_))\n> -               pipe()->completeBuffer(request, buffer);\n> +       if (request->_o<Request>()->findBuffer(&rawStream_))\n> +               pipe()->completeBuffer(request->_o<Request>(), buffer);\n>  \n> -       ipa_->fillParamsBuffer(info->id, info->paramBuffer->cookie());\n> +       ipa_->fillParamsBuffer(request->_o<Request>()->sequence(),\n> +                              request->paramBuffer->cookie());\n>  }\n>  \n>  void IPU3CameraData::paramBufferReady(FrameBuffer *buffer)\n>  {\n> -       IPU3Frames::Info *info = frameInfos_.find(buffer);\n> -       if (!info)\n> -               return;\n> -\n> -       info->paramDequeued = true;\n> +       IPU3Request *request = cameraRequest(buffer->request());\n> +       request->paramDequeued = true;\n>  \n>         /*\n>          * tryComplete() will delete info if it completes the IPU3Frame.\n> @@ -1346,35 +1418,25 @@ void IPU3CameraData::paramBufferReady(FrameBuffer *buffer)\n>          *\n>          * \\todo Improve the FrameInfo API to avoid this type of issue\n>          */\n> -       Request *request = info->request;\n>  \n> -       if (frameInfos_.tryComplete(info))\n> -               pipe()->completeRequest(request);\n> +       tryCompleteRequest(request);\n>  }\n>  \n>  void IPU3CameraData::statBufferReady(FrameBuffer *buffer)\n>  {\n> -       IPU3Frames::Info *info = frameInfos_.find(buffer);\n> -       if (!info)\n> -               return;\n> -\n> -       Request *request = info->request;\n> +       IPU3Request *request = cameraRequest(buffer->request());\n>  \n>         if (buffer->metadata().status == FrameMetadata::FrameCancelled) {\n> -               info->metadataProcessed = true;\n> +               request->metadataProcessed = true;\n>  \n> -               /*\n> -                * tryComplete() will delete info if it completes the IPU3Frame.\n> -                * In that event, we must have obtained the Request before hand.\n> -                */\n> -               if (frameInfos_.tryComplete(info))\n> -                       pipe()->completeRequest(request);\n> +               tryCompleteRequest(request);\n>  \n>                 return;\n>         }\n>  \n> -       ipa_->processStatsBuffer(info->id, request->metadata().get(controls::SensorTimestamp).value_or(0),\n> -                                info->statBuffer->cookie(), info->effectiveSensorControls);\n> +       ipa_->processStatsBuffer(request->_o<Request>()->sequence(),\n> +                                request->_o<Request>()->metadata().get(controls::SensorTimestamp).value_or(0),\n> +                                request->statBuffer->cookie(), request->effectiveSensorControls);\n>  }\n>  \n>  /*\n> diff --git a/src/libcamera/pipeline/ipu3/meson.build b/src/libcamera/pipeline/ipu3/meson.build\n> index a1b0b31ac5bc..d60e07ae6cca 100644\n> --- a/src/libcamera/pipeline/ipu3/meson.build\n> +++ b/src/libcamera/pipeline/ipu3/meson.build\n> @@ -2,7 +2,6 @@\n>  \n>  libcamera_sources += files([\n>      'cio2.cpp',\n> -    'frames.cpp',\n>      'imgu.cpp',\n>      'ipu3.cpp',\n>  ])\n> -- \n> 2.43.2\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 2BB29BD1F1\n\tfor <parsemail@patchwork.libcamera.org>;\n\tTue, 12 Mar 2024 11:27:33 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4A39262C8B;\n\tTue, 12 Mar 2024 12:27:32 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6CB9D62973\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue, 12 Mar 2024 12:27:30 +0100 (CET)","from pendragon.ideasonboard.com\n\t(aztw-30-b2-v4wan-166917-cust845.vm26.cable.virginm.net\n\t[82.37.23.78])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 85E2DE4;\n\tTue, 12 Mar 2024 12:27:08 +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=\"Vii6MxoT\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1710242828;\n\tbh=J596Z9LCTmW0OC0cazE5/zbtpIjmud/57eW6rIZwVpw=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=Vii6MxoTkCFCP90ntDOuUVxqmWNYFGn24UkT1mEegmCQoVI6zOgYFxLSyIxf2OZkM\n\tlPOGJrkaxn2z1uqC1m2Srdijcufwj+wmVi5Hax6GitB6NtipwXjZopVwIFCfxBeKgF\n\tQdy7V3gOensEbKh9kRWQhW9ytqL7jBaBOY2WK1NI=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20240311123234.32925-4-jacopo.mondi@ideasonboard.com>","References":"<20240311123234.32925-1-jacopo.mondi@ideasonboard.com>\n\t<20240311123234.32925-4-jacopo.mondi@ideasonboard.com>","Subject":"Re: [PATCH v2 3/4] libcamera: ipu3: Replace IPU3FrameInfo","From":"Kieran Bingham <kieran.bingham@ideasonboard.com>","To":"Jacopo Mondi <jacopo.mondi@ideasonboard.com>,\n\tlibcamera-devel@lists.libcamera.org","Date":"Tue, 12 Mar 2024 11:27:27 +0000","Message-ID":"<171024284783.252503.16918205638879056926@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","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>","Cc":"Jacopo Mondi <jacopo.mondi@ideasonboard.com>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]