[{"id":3382,"web_url":"https://patchwork.libcamera.org/comment/3382/","msgid":"<20200108014539.GX4871@pendragon.ideasonboard.com>","date":"2020-01-08T01:45:39","subject":"Re: [libcamera-devel] [PATCH v2 16/25] libcamera: pipeline: rkisp1:\n\tSwitch to FrameBuffer interface for stat and param","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 Mon, Dec 30, 2019 at 01:05:01PM +0100, Niklas Söderlund wrote:\n> The V4L2VideoDevice class can now operate using a FrameBuffer interface,\n> switch the RkISP1 statistics and parameters buffer to use it. We can not\n> yet convert the application facing buffers yet.\n\nDouble yet, s/yet convert/convert/\ns/application facing/application-facing/\n\n> \n> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>\n> ---\n>  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 122 ++++++++++++-----------\n>  1 file changed, 66 insertions(+), 56 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index 9cd0ab3ad88b35cc..ea581aaaa85088cd 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -32,9 +32,6 @@\n>  #include \"v4l2_subdevice.h\"\n>  #include \"v4l2_videodevice.h\"\n>  \n> -#define RKISP1_PARAM_BASE 0x100\n> -#define RKISP1_STAT_BASE 0x200\n> -\n\nThis is nice.\n\n>  namespace libcamera {\n>  \n>  LOG_DEFINE_CATEGORY(RkISP1)\n> @@ -52,8 +49,8 @@ struct RkISP1FrameInfo {\n>  \tunsigned int frame;\n>  \tRequest *request;\n>  \n> -\tBuffer *paramBuffer;\n> -\tBuffer *statBuffer;\n> +\tFrameBuffer *paramBuffer;\n> +\tFrameBuffer *statBuffer;\n>  \tBuffer *videoBuffer;\n>  \n>  \tbool paramFilled;\n> @@ -71,6 +68,7 @@ public:\n>  \n>  \tRkISP1FrameInfo *find(unsigned int frame);\n>  \tRkISP1FrameInfo *find(Buffer *buffer);\n> +\tRkISP1FrameInfo *find(FrameBuffer *buffer);\n>  \tRkISP1FrameInfo *find(Request *request);\n>  \n>  private:\n> @@ -152,6 +150,7 @@ public:\n>  \n>  \tconst V4L2SubdeviceFormat &sensorFormat() { return sensorFormat_; }\n>  \n> +\n\nNot needed.\n\n>  private:\n>  \tstatic constexpr unsigned int RKISP1_BUFFER_COUNT = 4;\n>  \n> @@ -203,8 +202,8 @@ private:\n>  \tint createCamera(MediaEntity *sensor);\n>  \tvoid tryCompleteRequest(Request *request);\n>  \tvoid bufferReady(Buffer *buffer);\n> -\tvoid paramReady(Buffer *buffer);\n> -\tvoid statReady(Buffer *buffer);\n> +\tvoid paramReady(FrameBuffer *buffer);\n> +\tvoid statReady(FrameBuffer *buffer);\n>  \n>  \tMediaDevice *media_;\n>  \tV4L2Subdevice *dphy_;\n> @@ -213,11 +212,10 @@ private:\n>  \tV4L2VideoDevice *param_;\n>  \tV4L2VideoDevice *stat_;\n>  \n> -\tBufferPool paramPool_;\n> -\tBufferPool statPool_;\n> -\n> -\tstd::queue<Buffer *> paramBuffers_;\n> -\tstd::queue<Buffer *> statBuffers_;\n> +\tstd::vector<std::unique_ptr<FrameBuffer>> paramBuffersMemory;\n> +\tstd::vector<std::unique_ptr<FrameBuffer>> statBuffersMemory;\n\nMissing _ in the data member names.\n\nHow about naming these paramBuffers_ and statBuffers_, and ...\n\n> +\tstd::queue<FrameBuffer *> paramBuffers_;\n> +\tstd::queue<FrameBuffer *> statBuffers_;\n\n... renaming those two availableParamBuffers_ and availableStatBuffers_\n? (or s/available/free/)\n\n>  \n>  \tCamera *activeCamera_;\n>  };\n> @@ -233,13 +231,13 @@ RkISP1FrameInfo *RkISP1Frames::create(unsigned int frame, Request *request, Stre\n>  \t\tLOG(RkISP1, Error) << \"Parameters buffer underrun\";\n>  \t\treturn nullptr;\n>  \t}\n> -\tBuffer *paramBuffer = pipe_->paramBuffers_.front();\n> +\tFrameBuffer *paramBuffer = pipe_->paramBuffers_.front();\n>  \n>  \tif (pipe_->statBuffers_.empty()) {\n>  \t\tLOG(RkISP1, Error) << \"Statisitc buffer underrun\";\n>  \t\treturn nullptr;\n>  \t}\n> -\tBuffer *statBuffer = pipe_->statBuffers_.front();\n> +\tFrameBuffer *statBuffer = pipe_->statBuffers_.front();\n>  \n>  \tBuffer *videoBuffer = request->findBuffer(stream);\n>  \tif (!videoBuffer) {\n> @@ -299,9 +297,21 @@ RkISP1FrameInfo *RkISP1Frames::find(Buffer *buffer)\n>  \tfor (auto &itInfo : frameInfo_) {\n>  \t\tRkISP1FrameInfo *info = itInfo.second;\n>  \n> +\t\tif (info->videoBuffer == buffer)\n> +\t\t\treturn info;\n> +\t}\n> +\n> +\tLOG(RkISP1, Error) << \"Can't locate info from buffer\";\n> +\treturn nullptr;\n> +}\n> +\n> +RkISP1FrameInfo *RkISP1Frames::find(FrameBuffer *buffer)\n> +{\n> +\tfor (auto &itInfo : frameInfo_) {\n> +\t\tRkISP1FrameInfo *info = itInfo.second;\n> +\n>  \t\tif (info->paramBuffer == buffer ||\n> -\t\t    info->statBuffer == buffer ||\n> -\t\t    info->videoBuffer == buffer)\n> +\t\t    info->statBuffer == buffer)\n>  \t\t\treturn info;\n>  \t}\n>  \n> @@ -660,6 +670,7 @@ int PipelineHandlerRkISP1::allocateBuffers(Camera *camera,\n>  \t\t\t\t\t   const std::set<Stream *> &streams)\n>  {\n>  \tRkISP1CameraData *data = cameraData(camera);\n> +\tunsigned int count = 1;\n\nMy OCD would have moved this after the next line.\n\n>  \tStream *stream = *streams.begin();\n>  \tint ret;\n>  \n> @@ -671,43 +682,41 @@ int PipelineHandlerRkISP1::allocateBuffers(Camera *camera,\n>  \tif (ret)\n>  \t\treturn ret;\n>  \n> -\tparamPool_.createBuffers(stream->configuration().bufferCount + 1);\n\nWhy did we have a + 1 ?\n\n> -\tret = param_->exportBuffers(&paramPool_);\n> -\tif (ret) {\n> -\t\tvideo_->releaseBuffers();\n> -\t\treturn ret;\n> -\t}\n> +\tunsigned int maxBuffers = 0;\n> +\tfor (const Stream *s : camera->streams())\n> +\t\tmaxBuffers = std::max(maxBuffers, s->configuration().bufferCount);\n>  \n> -\tstatPool_.createBuffers(stream->configuration().bufferCount + 1);\n> -\tret = stat_->exportBuffers(&statPool_);\n> -\tif (ret) {\n> -\t\tparam_->releaseBuffers();\n> -\t\tvideo_->releaseBuffers();\n> -\t\treturn ret;\n> -\t}\n> +\tret = param_->exportBuffers(maxBuffers, &paramBuffersMemory);\n> +\tif (ret < 0)\n> +\t\tgoto err;\n>  \n> -\tfor (unsigned int i = 0; i < stream->configuration().bufferCount + 1; i++) {\n> -\t\tFrameBuffer::Plane plane;\n> -\t\tplane.fd = FileDescriptor(paramPool_.buffers()[i].planes()[0].fd.fd());\n> -\t\tplane.length = paramPool_.buffers()[i].planes()[0].length;\n> +\tret = stat_->exportBuffers(maxBuffers, &statBuffersMemory);\n> +\tif (ret < 0)\n> +\t\tgoto err;\n>  \n> -\t\tdata->ipaBuffers_.push_back({ .id = RKISP1_PARAM_BASE | i,\n> -\t\t\t\t\t      .planes = { plane } });\n> -\t\tparamBuffers_.push(new Buffer(i));\n> +\tfor (std::unique_ptr<FrameBuffer> &buffer : paramBuffersMemory) {\n> +\t\tbuffer->setCookie(count++);\n> +\t\tdata->ipaBuffers_.push_back({ .id = buffer->cookie(),\n> +\t\t\t\t\t      .planes = buffer->planes() });\n\nThis duplicates the FileDescriptor instances :-S We really need to sort\nthis out, possibly on top of this series, but sooner than later.\n\n> +\t\tparamBuffers_.push(buffer.get());\n>  \t}\n>  \n> -\tfor (unsigned int i = 0; i < stream->configuration().bufferCount + 1; i++) {\n> -\t\tFrameBuffer::Plane plane;\n> -\t\tplane.fd = FileDescriptor(statPool_.buffers()[i].planes()[0].fd.fd());\n> -\t\tplane.length = statPool_.buffers()[i].planes()[0].length;\n> -\n> -\t\tdata->ipaBuffers_.push_back({ .id = RKISP1_STAT_BASE | i,\n> -\t\t\t\t\t      .planes = { plane } });\n> -\t\tstatBuffers_.push(new Buffer(i));\n> +\tfor (std::unique_ptr<FrameBuffer> &buffer : statBuffersMemory) {\n> +\t\tbuffer->setCookie(count++);\n> +\t\tdata->ipaBuffers_.push_back({ .id = buffer->cookie(),\n> +\t\t\t\t\t      .planes = buffer->planes() });\n> +\t\tstatBuffers_.push(buffer.get());\n>  \t}\n>  \n>  \tdata->ipa_->mapBuffers(data->ipaBuffers_);\n>  \n> +\treturn 0;\n> +\n> +err:\n\ns/err/error/ ?\n\n> +\tparamBuffersMemory.clear();\n> +\tstatBuffersMemory.clear();\n> +\tvideo_->releaseBuffers();\n> +\n>  \treturn ret;\n>  }\n>  \n> @@ -716,15 +725,14 @@ int PipelineHandlerRkISP1::freeBuffers(Camera *camera,\n>  {\n>  \tRkISP1CameraData *data = cameraData(camera);\n>  \n> -\twhile (!statBuffers_.empty()) {\n> -\t\tdelete statBuffers_.front();\n> +\twhile (!statBuffers_.empty())\n>  \t\tstatBuffers_.pop();\n\n\tstatBuffers_.clear();\n\n> -\t}\n>  \n> -\twhile (!paramBuffers_.empty()) {\n> -\t\tdelete paramBuffers_.front();\n> +\twhile (!paramBuffers_.empty())\n>  \t\tparamBuffers_.pop();\n\n\tparamBuffers_.clear();\n\n> -\t}\n> +\n> +\tparamBuffersMemory.clear();\n> +\tstatBuffersMemory.clear();\n>  \n>  \tstd::vector<unsigned int> ids;\n>  \tfor (IPABuffer &ipabuf : data->ipaBuffers_)\n> @@ -829,9 +837,11 @@ int PipelineHandlerRkISP1::queueRequestDevice(Camera *camera,\n>  \tif (!info)\n>  \t\treturn -ENOENT;\n>  \n> +\tunsigned int paramid = info->paramBuffer->cookie();\n\ns/paramid/paramId/\n\n> +\n>  \tIPAOperationData op;\n>  \top.operation = RKISP1_IPA_EVENT_QUEUE_REQUEST;\n> -\top.data = { data->frame_, RKISP1_PARAM_BASE | info->paramBuffer->index() };\n> +\top.data = { data->frame_, paramid };\n\nOr better,\n\n\top.data = { data->frame_, info->paramBuffer->cookie() };\n\n>  \top.controls = { request->controls() };\n>  \tdata->ipa_->processEvent(op);\n>  \n> @@ -946,8 +956,8 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)\n>  \t\treturn false;\n>  \n>  \tvideo_->bufferReady.connect(this, &PipelineHandlerRkISP1::bufferReady);\n> -\tstat_->bufferReady.connect(this, &PipelineHandlerRkISP1::statReady);\n> -\tparam_->bufferReady.connect(this, &PipelineHandlerRkISP1::paramReady);\n> +\tstat_->frameBufferReady.connect(this, &PipelineHandlerRkISP1::statReady);\n> +\tparam_->frameBufferReady.connect(this, &PipelineHandlerRkISP1::paramReady);\n>  \n>  \t/* Configure default links. */\n>  \tif (initLinks() < 0) {\n> @@ -1009,7 +1019,7 @@ void PipelineHandlerRkISP1::bufferReady(Buffer *buffer)\n>  \ttryCompleteRequest(request);\n>  }\n>  \n> -void PipelineHandlerRkISP1::paramReady(Buffer *buffer)\n> +void PipelineHandlerRkISP1::paramReady(FrameBuffer *buffer)\n>  {\n>  \tASSERT(activeCamera_);\n>  \tRkISP1CameraData *data = cameraData(activeCamera_);\n> @@ -1020,7 +1030,7 @@ void PipelineHandlerRkISP1::paramReady(Buffer *buffer)\n>  \ttryCompleteRequest(info->request);\n>  }\n>  \n> -void PipelineHandlerRkISP1::statReady(Buffer *buffer)\n> +void PipelineHandlerRkISP1::statReady(FrameBuffer *buffer)\n>  {\n>  \tASSERT(activeCamera_);\n>  \tRkISP1CameraData *data = cameraData(activeCamera_);\n> @@ -1030,7 +1040,7 @@ void PipelineHandlerRkISP1::statReady(Buffer *buffer)\n>  \t\treturn;\n>  \n>  \tunsigned int frame = info->frame;\n> -\tunsigned int statid = RKISP1_STAT_BASE | info->statBuffer->index();\n> +\tunsigned int statid = info->statBuffer->cookie();\n\nWhile at it, s/statid/statId/ or the better option proposed above ?\n\nReviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n\n>  \n>  \tIPAOperationData op;\n>  \top.operation = RKISP1_IPA_EVENT_SIGNAL_STAT_BUFFER;","headers":{"Return-Path":"<laurent.pinchart@ideasonboard.com>","Received":["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 285CF60612\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed,  8 Jan 2020 02:45:53 +0100 (CET)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 03AD230F;\n\tWed,  8 Jan 2020 02:45:51 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1578447952;\n\tbh=Bz6YTZ4wP49l4fi6+o4mRLqlYzDX/YoMHP61Ll0Gwlc=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=n7A5GUKnPP+auj/5hNVgl/FwjePALSEIqJ9Oz+qgo+hdxunYliFLDkUBRbxCsuaHe\n\tfI6OWriYZniymeZi0QHs3zfDwepxDhgvZnr1ww7cR/zy3h0Dy0mj6LnceYdgMRqh55\n\t4T5P+3GGyjhqlm1H0Dsma3Nb2idrQaat6rdFirrw=","Date":"Wed, 8 Jan 2020 03:45:39 +0200","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Niklas =?utf-8?q?S=C3=B6derlund?= <niklas.soderlund@ragnatech.se>","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200108014539.GX4871@pendragon.ideasonboard.com>","References":"<20191230120510.938333-1-niklas.soderlund@ragnatech.se>\n\t<20191230120510.938333-17-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20191230120510.938333-17-niklas.soderlund@ragnatech.se>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v2 16/25] libcamera: pipeline: rkisp1:\n\tSwitch to FrameBuffer interface for stat and param","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>","X-List-Received-Date":"Wed, 08 Jan 2020 01:45:53 -0000"}}]