[{"id":13790,"web_url":"https://patchwork.libcamera.org/comment/13790/","msgid":"<20201118165224.gih7hhuo722rjrca@uno.localdomain>","date":"2020-11-18T16:52:24","subject":"Re: [libcamera-devel] [PATCH 09/11] libcamera: ipu3: Add helper for\n\tparameter and statistic buffers","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Niklas,\n\nOn Thu, Nov 05, 2020 at 01:15:44AM +0100, Niklas Söderlund wrote:\n> Add a helper class to aid in associating a parameter and statistic\n> buffer with each request queued to the pipeline. The helper helps with\n> tracking the state of the extra buffers and in completing the request\n> once all extra processing is done.\n>\n> This change only adds the helper more work is needed to integrate it\n> with the pipeline and an IPA.\n>\n> Signed-off-by: Niklas Söderlund \n> ---\n> src/libcamera/pipeline/ipu3/frames.cpp | 164 ++++++++++++++++++++++++\n> src/libcamera/pipeline/ipu3/frames.h | 68 ++++++++++\n> src/libcamera/pipeline/ipu3/meson.build | 1 +\n> 3 files changed, 233 insertions(+)\n> create mode 100644 src/libcamera/pipeline/ipu3/frames.cpp\n> create 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> new file mode 100644\n> index 0000000000000000..824844e345e13679\n> --- /dev/null\n> +++ b/src/libcamera/pipeline/ipu3/frames.cpp\n> @@ -0,0 +1,164 @@\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 \n> +#include \n> +\n> +#include \n> +\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(PipelineHandler *pipe, IPAProxy *ipa)\n> +\t: pipe_(pipe), ipa_(ipa), nextId_(0)\n> +{\n> +}\n> +\n> +void IPU3Frames::mapBuffers(const std::vector> ¶mBuffers,\n> +\t\t\t const std::vector> &statBuffers)\n> +{\n> +\tunsigned int ipaBufferId = 1;\n> +\n> +\tfor (const std::unique_ptr &buffer : paramBuffers) {\n> +\t\tbuffer->setCookie(ipaBufferId++);\n> +\t\tipaBuffers_.push_back({ .id = buffer->cookie(),\n> +\t\t\t\t\t.planes = buffer->planes() });\n> +\t\tavailableParamBuffers_.push(buffer.get());\n> +\t}\n> +\n> +\tfor (const std::unique_ptr &buffer : statBuffers) {\n> +\t\tbuffer->setCookie(ipaBufferId++);\n> +\t\tipaBuffers_.push_back({ .id = buffer->cookie(),\n> +\t\t\t\t\t.planes = buffer->planes() });\n> +\t\tavailableStatBuffers_.push(buffer.get());\n> +\t}\n> +\n> +\tipa_->mapBuffers(ipaBuffers_);\n> +\n> +\tnextId_ = 0;\n> +\tframeInfo_.clear();\n> +}\n> +\n> +void IPU3Frames::unmapBuffers()\n> +{\n> +\tavailableParamBuffers_ = {};\n> +\tavailableStatBuffers_ = {};\n> +\n> +\tstd::vector ids;\n> +\tfor (IPABuffer &ipabuf : ipaBuffers_)\n> +\t\tids.push_back(ipabuf.id);\n> +\n> +\tipa_->unmapBuffers(ids);\n> +\tipaBuffers_.clear();\n> +}\n\nI would do these two operations in the pipeline. Having the helper\ncalling the IPA is not nice imho and this really does too many\nthings, set cookies in buffers, map buffers in the IPA, fill the\navailable buffers in the helper, reset the id counter...\n\nI would make this an:\n IPU3Frames::initialize() or reset()\n\nthat initializes the available buffers and reset counters and leave\nthe IPA buffer mapping and cookies initialization to the pipeline handler.\n\n> +\n> +IPU3Frames::Info *IPU3Frames::create(Request *request)\n> +{\n> +\tunsigned int id = nextId_++;\n> +\n> +\tif (availableParamBuffers_.empty()) {\n> +\t\tLOG(IPU3, Error) << \"Parameters buffer underrun\";\n> +\t\treturn nullptr;\n> +\t}\n> +\tFrameBuffer *paramBuffer = availableParamBuffers_.front();\n> +\n> +\tif (availableStatBuffers_.empty()) {\n> +\t\tLOG(IPU3, Error) << \"Statisitc buffer underrun\";\n> +\t\treturn nullptr;\n> +\t}\n> +\tFrameBuffer *statBuffer = availableStatBuffers_.front();\n> +\n> +\tavailableParamBuffers_.pop();\n> +\tavailableStatBuffers_.pop();\n> +\n> +\tstd::unique_ptr info = std::make_unique();\n> +\n> +\tinfo->id = id;\n> +\tinfo->request = request;\n> +\tinfo->rawBuffer = nullptr;\n> +\tinfo->paramBuffer = paramBuffer;\n> +\tinfo->statBuffer = statBuffer;\n> +\tinfo->paramFilled = false;\n> +\tinfo->paramDequeued = false;\n> +\tinfo->metadataProcessed = false;\n> +\n> +\tframeInfo_[id] = std::move(info);\n> +\n> +\treturn frameInfo_[id].get();\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\nSo, paramDequeued is set when rawOnly, and paramFilled when the IPA is\ndone, but never really checked for here. Shouldn't this be a single\nflag maybe 'paramDone' set in case the ipa is done or the request\ncontains a raw frame only ?\n\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> +\tpipe_->completeRequest(request);\n\npipe_ is only used here, and calling back into the pipeline handler is\nan implicit behaviour I would avoid.\n\nWhat about the pipeline handler doing:\n if (frameInfo_->tryComplete(id))\n completeRequest(frameInfo_->find(id)->request)\n?\n\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> +\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> +\treturn nullptr;\n> +}\n> +\n> +IPU3Frames::Info *IPU3Frames::find(Request *request)\n> +{\n> +\tfor (auto const &itInfo : frameInfo_) {\n> +\t\tInfo *info = itInfo.second.get();\n> +\n> +\t\tif (info->request == request)\n> +\t\t\treturn info;\n> +\t}\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> new file mode 100644\n> index 0000000000000000..5c072f8ddbc9660f\n> --- /dev/null\n> +++ b/src/libcamera/pipeline/ipu3/frames.h\n> @@ -0,0 +1,68 @@\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> +#ifndef __LIBCAMERA_PIPELINE_IPU3_FRAMES_H__\n> +#define __LIBCAMERA_PIPELINE_IPU3_FRAMES_H__\n> +\n> +#include \n> +#include \n> +#include \n> +#include \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\tbool paramFilled;\n> +\t\tbool paramDequeued;\n> +\t\tbool metadataProcessed;\n> +\t};\n> +\n> +\tIPU3Frames(PipelineHandler *pipe, IPAProxy *ipa);\n> +\n> +\tvoid mapBuffers(const std::vector> ¶mBuffers,\n> +\t\t\tconst std::vector> &statBuffers);\n> +\tvoid unmapBuffers();\n> +\n> +\tInfo *create(Request *request);\n> +\tbool tryComplete(IPU3Frames::Info *info);\n> +\n> +\tInfo *find(unsigned int id);\n> +\tInfo *find(FrameBuffer *buffer);\n> +\tInfo *find(Request *request);\n> +\n> +private:\n> +\tPipelineHandler *pipe_;\n> +\tIPAProxy *ipa_;\n> +\n> +\tstd::queue availableParamBuffers_;\n> +\tstd::queue availableStatBuffers_;\n> +\n> +\tstd::vector ipaBuffers_;\n> +\n> +\tunsigned int nextId_;\n> +\tstd::map> frameInfo_;\n> +};\n> +\n> +} /* namespace libcamera */\n> +\n> +#endif /* __LIBCAMERA_PIPELINE_IPU3_FRAMES_H__ */\n> diff --git a/src/libcamera/pipeline/ipu3/meson.build b/src/libcamera/pipeline/ipu3/meson.build\n> index d60e07ae6ccac2bc..a1b0b31ac5bcf864 100644\n> --- a/src/libcamera/pipeline/ipu3/meson.build\n> +++ b/src/libcamera/pipeline/ipu3/meson.build\n> @@ -2,6 +2,7 @@\n>\n> libcamera_sources += files([\n> 'cio2.cpp',\n> + 'frames.cpp',\n> 'imgu.cpp',\n> 'ipu3.cpp',\n> ])\n> --\n> 2.29.2\n>\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 49883BE176\n\tfor ;\n\tWed, 18 Nov 2020 16:52:24 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9F61F7E9B2;\n\tWed, 18 Nov 2020 17:52:23 +0100 (CET)","from relay1-d.mail.gandi.net (relay1-d.mail.gandi.net\n\t[217.70.183.193])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id AA379632AF\n\tfor ;\n\tWed, 18 Nov 2020 17:52:21 +0100 (CET)","from uno.localdomain (93-34-118-233.ip49.fastwebnet.it\n\t[93.34.118.233]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay1-d.mail.gandi.net (Postfix) with ESMTPSA id 3A6C5240004;\n\tWed, 18 Nov 2020 16:52:21 +0000 (UTC)"],"X-Originating-IP":"93.34.118.233","Date":"Wed, 18 Nov 2020 17:52:24 +0100","From":"Jacopo Mondi ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Message-ID":"<20201118165224.gih7hhuo722rjrca@uno.localdomain>","References":"<20201105001546.1690179-1-niklas.soderlund@ragnatech.se>\n\t<20201105001546.1690179-10-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201105001546.1690179-10-niklas.soderlund@ragnatech.se>","Subject":"Re: [libcamera-devel] [PATCH 09/11] libcamera: ipu3: Add helper for\n\tparameter and statistic buffers","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":14121,"web_url":"https://patchwork.libcamera.org/comment/14121/","msgid":"","date":"2020-12-08T03:07:35","subject":"Re: [libcamera-devel] [PATCH 09/11] libcamera: ipu3: Add helper for\n\tparameter and statistic buffers","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Niklas,\n\nThank you for the patch.\n\nOn Wed, Nov 18, 2020 at 05:52:24PM +0100, Jacopo Mondi wrote:\n> On Thu, Nov 05, 2020 at 01:15:44AM +0100, Niklas Söderlund wrote:\n> > Add a helper class to aid in associating a parameter and statistic\n> > buffer with each request queued to the pipeline. The helper helps with\n> > tracking the state of the extra buffers and in completing the request\n> > once all extra processing is done.\n> >\n> > This change only adds the helper more work is needed to integrate it\n> > with the pipeline and an IPA.\n> >\n> > Signed-off-by: Niklas Söderlund \n> > ---\n> > src/libcamera/pipeline/ipu3/frames.cpp | 164 ++++++++++++++++++++++++\n> > src/libcamera/pipeline/ipu3/frames.h | 68 ++++++++++\n> > src/libcamera/pipeline/ipu3/meson.build | 1 +\n> > 3 files changed, 233 insertions(+)\n> > create mode 100644 src/libcamera/pipeline/ipu3/frames.cpp\n> > create 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> > new file mode 100644\n> > index 0000000000000000..824844e345e13679\n> > --- /dev/null\n> > +++ b/src/libcamera/pipeline/ipu3/frames.cpp\n> > @@ -0,0 +1,164 @@\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 \n> > +#include \n> > +\n> > +#include \n> > +\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(PipelineHandler *pipe, IPAProxy *ipa)\n> > +\t: pipe_(pipe), ipa_(ipa), nextId_(0)\n> > +{\n> > +}\n> > +\n> > +void IPU3Frames::mapBuffers(const std::vector> ¶mBuffers,\n> > +\t\t\t const std::vector> &statBuffers)\n> > +{\n> > +\tunsigned int ipaBufferId = 1;\n> > +\n> > +\tfor (const std::unique_ptr &buffer : paramBuffers) {\n> > +\t\tbuffer->setCookie(ipaBufferId++);\n> > +\t\tipaBuffers_.push_back({ .id = buffer->cookie(),\n> > +\t\t\t\t\t.planes = buffer->planes() });\n> > +\t\tavailableParamBuffers_.push(buffer.get());\n> > +\t}\n> > +\n> > +\tfor (const std::unique_ptr &buffer : statBuffers) {\n> > +\t\tbuffer->setCookie(ipaBufferId++);\n> > +\t\tipaBuffers_.push_back({ .id = buffer->cookie(),\n> > +\t\t\t\t\t.planes = buffer->planes() });\n> > +\t\tavailableStatBuffers_.push(buffer.get());\n> > +\t}\n> > +\n> > +\tipa_->mapBuffers(ipaBuffers_);\n> > +\n> > +\tnextId_ = 0;\n> > +\tframeInfo_.clear();\n> > +}\n> > +\n> > +void IPU3Frames::unmapBuffers()\n> > +{\n> > +\tavailableParamBuffers_ = {};\n> > +\tavailableStatBuffers_ = {};\n> > +\n> > +\tstd::vector ids;\n> > +\tfor (IPABuffer &ipabuf : ipaBuffers_)\n> > +\t\tids.push_back(ipabuf.id);\n> > +\n> > +\tipa_->unmapBuffers(ids);\n> > +\tipaBuffers_.clear();\n> > +}\n> \n> I would do these two operations in the pipeline. Having the helper\n> calling the IPA is not nice imho and this really does too many\n> things, set cookies in buffers, map buffers in the IPA, fill the\n> available buffers in the helper, reset the id counter...\n> \n> I would make this an:\n> IPU3Frames::initialize() or reset()\n> \n> that initializes the available buffers and reset counters and leave\n> the IPA buffer mapping and cookies initialization to the pipeline handler.\n\nI like moving the IPA handling to the pipeline handler. I think helpers\nto make buffer mapping more transparent would be nice though, but I\nwouldn't handle it in this class.\n\n> > +\n> > +IPU3Frames::Info *IPU3Frames::create(Request *request)\n> > +{\n> > +\tunsigned int id = nextId_++;\n> > +\n> > +\tif (availableParamBuffers_.empty()) {\n> > +\t\tLOG(IPU3, Error) << \"Parameters buffer underrun\";\n> > +\t\treturn nullptr;\n> > +\t}\n> > +\tFrameBuffer *paramBuffer = availableParamBuffers_.front();\n> > +\n> > +\tif (availableStatBuffers_.empty()) {\n> > +\t\tLOG(IPU3, Error) << \"Statisitc buffer underrun\";\n> > +\t\treturn nullptr;\n> > +\t}\n> > +\tFrameBuffer *statBuffer = availableStatBuffers_.front();\n> > +\n> > +\tavailableParamBuffers_.pop();\n> > +\tavailableStatBuffers_.pop();\n> > +\n> > +\tstd::unique_ptr info = std::make_unique();\n> > +\n> > +\tinfo->id = id;\n> > +\tinfo->request = request;\n> > +\tinfo->rawBuffer = nullptr;\n> > +\tinfo->paramBuffer = paramBuffer;\n> > +\tinfo->statBuffer = statBuffer;\n> > +\tinfo->paramFilled = false;\n> > +\tinfo->paramDequeued = false;\n> > +\tinfo->metadataProcessed = false;\n> > +\n> > +\tframeInfo_[id] = std::move(info);\n> > +\n> > +\treturn frameInfo_[id].get();\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> So, paramDequeued is set when rawOnly, and paramFilled when the IPA is\n> done, but never really checked for here. Shouldn't this be a single\n> flag maybe 'paramDone' set in case the ipa is done or the request\n> contains a raw frame only ?\n> \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> > +\tpipe_->completeRequest(request);\n> \n> pipe_ is only used here, and calling back into the pipeline handler is\n> an implicit behaviour I would avoid.\n> \n> What about the pipeline handler doing:\n> if (frameInfo_->tryComplete(id))\n> completeRequest(frameInfo_->find(id)->request)\n> ?\n\nThat's a nice suggestion :-)\n\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> > +\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> > +\treturn nullptr;\n> > +}\n> > +\n> > +IPU3Frames::Info *IPU3Frames::find(Request *request)\n> > +{\n> > +\tfor (auto const &itInfo : frameInfo_) {\n> > +\t\tInfo *info = itInfo.second.get();\n> > +\n> > +\t\tif (info->request == request)\n> > +\t\t\treturn info;\n> > +\t}\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> > new file mode 100644\n> > index 0000000000000000..5c072f8ddbc9660f\n> > --- /dev/null\n> > +++ b/src/libcamera/pipeline/ipu3/frames.h\n> > @@ -0,0 +1,68 @@\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> > +#ifndef __LIBCAMERA_PIPELINE_IPU3_FRAMES_H__\n> > +#define __LIBCAMERA_PIPELINE_IPU3_FRAMES_H__\n> > +\n> > +#include \n> > +#include \n> > +#include \n> > +#include \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\tbool paramFilled;\n> > +\t\tbool paramDequeued;\n> > +\t\tbool metadataProcessed;\n> > +\t};\n> > +\n> > +\tIPU3Frames(PipelineHandler *pipe, IPAProxy *ipa);\n> > +\n> > +\tvoid mapBuffers(const std::vector> ¶mBuffers,\n> > +\t\t\tconst std::vector> &statBuffers);\n> > +\tvoid unmapBuffers();\n> > +\n> > +\tInfo *create(Request *request);\n> > +\tbool tryComplete(IPU3Frames::Info *info);\n> > +\n> > +\tInfo *find(unsigned int id);\n> > +\tInfo *find(FrameBuffer *buffer);\n> > +\tInfo *find(Request *request);\n\nThis looks to me like a workaround for the lack of pipeline handler\nprivate data associated with requests, am I wrong ? We don't need to\nsolve the problem in the Request class as a dependency for this series,\nbut should it be eventually handled there ?\n\nI share Jacopo's feeling that this class looks a bit like a big carpet\nunder which all the dust is swept. There's nothing wrong with that as\nsuch, as developing helpers usually takes the form of trying different\nimplementations and seeing what makes sense. Already addressing the\nissues mentioned above would however be nice.\n\n> > +\n> > +private:\n> > +\tPipelineHandler *pipe_;\n> > +\tIPAProxy *ipa_;\n> > +\n> > +\tstd::queue availableParamBuffers_;\n> > +\tstd::queue availableStatBuffers_;\n> > +\n> > +\tstd::vector ipaBuffers_;\n> > +\n> > +\tunsigned int nextId_;\n> > +\tstd::map> frameInfo_;\n> > +};\n> > +\n> > +} /* namespace libcamera */\n> > +\n> > +#endif /* __LIBCAMERA_PIPELINE_IPU3_FRAMES_H__ */\n> > diff --git a/src/libcamera/pipeline/ipu3/meson.build b/src/libcamera/pipeline/ipu3/meson.build\n> > index d60e07ae6ccac2bc..a1b0b31ac5bcf864 100644\n> > --- a/src/libcamera/pipeline/ipu3/meson.build\n> > +++ b/src/libcamera/pipeline/ipu3/meson.build\n> > @@ -2,6 +2,7 @@\n> >\n> > libcamera_sources += files([\n> > 'cio2.cpp',\n> > + 'frames.cpp',\n> > 'imgu.cpp',\n> > 'ipu3.cpp',\n> > ])","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id DB620BDB20\n\tfor ;\n\tTue, 8 Dec 2020 03:07:40 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5376A67E71;\n\tTue, 8 Dec 2020 04:07:40 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 212D967E6C\n\tfor ;\n\tTue, 8 Dec 2020 04:07:39 +0100 (CET)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 7C1FADD;\n\tTue, 8 Dec 2020 04:07:38 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"DC1UMAPS\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1607396858;\n\tbh=gNjbK6lqO2e8HBIfSW9T0ME9MjGS5hfKfY8bY/YSwRs=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=DC1UMAPSRP09qHScAkZBogva7xHr1KXKyK0+J05TOS0S+VkQYWVMorR8rhMJQwiEq\n\tKWkqsWzQHQOXwwNPJbU+qEX0DbB4fkNqYqAS+FzJaJzXqu6JcfdTOcPP15pU2iGijy\n\tfyZBoK4n8JJa5oP5gnaF7kQw/J0mWE4BgQCIqPoE=","Date":"Tue, 8 Dec 2020 05:07:35 +0200","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Message-ID":"","References":"<20201105001546.1690179-1-niklas.soderlund@ragnatech.se>\n\t<20201105001546.1690179-10-niklas.soderlund@ragnatech.se>\n\t<20201118165224.gih7hhuo722rjrca@uno.localdomain>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201118165224.gih7hhuo722rjrca@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH 09/11] libcamera: ipu3: Add helper for\n\tparameter and statistic buffers","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]