[{"id":2600,"web_url":"https://patchwork.libcamera.org/comment/2600/","msgid":"<20190904180518.GD5433@pendragon.ideasonboard.com>","date":"2019-09-04T18:05:18","subject":"Re: [libcamera-devel] [PATCH v2 04/14] libcamera: pipeline: Add\n\tmethod to prepare internal 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 Fri, Aug 30, 2019 at 01:26:43AM +0200, Niklas Söderlund wrote:\n> Buffers internal to a pipeline handler (buffers cycled between a CSI-2\n> pipeline to a ISP pipeline, parameters and statistics buffers) needs to\n> be prepared before they can be properly used inside a pipeline handler.\n> \n> At this point the preparation consists of mapping the Buffer objects\n> memory and associating it with a request. Instead of adding this helper\n> on the Buffer object itself add it to the pipeline handler base class to\n> prevent it from being exposed to applications.\n> \n> Signed-off-by: Niklas Söderlund \n> ---\n> src/libcamera/include/pipeline_handler.h | 2 ++\n> src/libcamera/pipeline_handler.cpp | 18 ++++++++++++++++++\n> 2 files changed, 20 insertions(+)\n> \n> diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h\n> index ffc7adb802215313..91d40ef40a465c4e 100644\n> --- a/src/libcamera/include/pipeline_handler.h\n> +++ b/src/libcamera/include/pipeline_handler.h\n> @@ -98,6 +98,8 @@ protected:\n> \n> \tCameraData *cameraData(const Camera *camera);\n> \n> +\tvoid prepareInternalBuffer(Buffer *buffer, Request *request,\n> +\t\t\t\t BufferMemory *mem);\n> \tCameraManager *manager_;\n> \n> private:\n> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> index 558b4b254d111e31..846272485c7d2fc0 100644\n> --- a/src/libcamera/pipeline_handler.cpp\n> +++ b/src/libcamera/pipeline_handler.cpp\n> @@ -485,6 +485,24 @@ void PipelineHandler::hotplugMediaDevice(MediaDevice *media)\n> \tmedia->disconnected.connect(this, &PipelineHandler::mediaDeviceDisconnected);\n> }\n> \n> +/**\n> + * \\brief Prepare buffer for internal usage by a pipeline handler\n> + * \\param[in,out] buffer The buffer to prepare\n> + * \\param[in] request The request to associate the \\a buffer with\n> + * \\param[in] mem The memory to associate the \\a buffer with\n> + *\n> + * Pipeline handlers creating internal buffers to facilitate data flow in the\n> + * pipeline need to prepare the buffers by setting up the buffer object state.\n> + * This function help pipeline handler implementations to perform this\n> + * preparation.\n> + */\n\nI'm afraid I don't like this much. First of all, reading the\ndocumentation, I have no idea how/when this is supposed to be used, so\nwe need to improve that. Then, looking at the rest of the series, I see\nthis method only used for the rkisp1 stats and params buffers. We have\nother internal buffers, such as between the CIO2 and ImgU, or ImgU\nparams or stats buffers. We should use this method consistently, and I\nwould like to see pipeline handlers updated to use it as part of this\npatch, to show how it is supposed to be used (although if the\ndocumentation was clearer maybe I wouldn't feel this need).\n\nFinally, as mentioned in a review of v1, this belongs to the Buffer\nclass. I understand you don't want to expose this to applications, but\nthe method really doesn't belong to the PipelineHandler class. At the\nvery least it should be a global helper method. I think this however\ncalls for a rework of the buffer API.\n\n> +void PipelineHandler::prepareInternalBuffer(Buffer *buffer, Request *request,\n> +\t\t\t\t\t BufferMemory *mem)\n> +{\n> +\tbuffer->request_ = request;\n> +\tbuffer->mem_ = mem;\n> +}\n> +\n> /**\n> * \\brief Slot for the MediaDevice disconnected signal\n> */","headers":{"Return-Path":"","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 CF71960BB2\n\tfor ;\n\tWed, 4 Sep 2019 20:05:24 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(dfj612yhrgyx302h3jwwy-3.rev.dnainternet.fi\n\t[IPv6:2001:14ba:21f5:5b00:ce28:277f:58d7:3ca4])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 4E989440;\n\tWed, 4 Sep 2019 20:05:24 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1567620324;\n\tbh=JTOqIJKYQqDmW6mPfZTcBp+IGnXD+ZFeuoP2owRDmy0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=byPV5mAzupJFcdAVl+qwrAgrjLxuDqgKHlYmbZfWSazszvccX9qx/pSZuI3bUXCRH\n\ttj4+mfAq9ul4pa2OkyES5G01pJDTXZWw9WHhy8kRg0KS6m4z2wjMbMBVCxEnIK1kwO\n\t+zsXtBxUA4VMrSSiqTW3GPfZKwqcs3+sJ4poxttA=","Date":"Wed, 4 Sep 2019 21:05:18 +0300","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190904180518.GD5433@pendragon.ideasonboard.com>","References":"<20190829232653.13214-1-niklas.soderlund@ragnatech.se>\n\t<20190829232653.13214-5-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":"<20190829232653.13214-5-niklas.soderlund@ragnatech.se>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v2 04/14] libcamera: pipeline: Add\n\tmethod to prepare internal buffers","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Wed, 04 Sep 2019 18:05:25 -0000"}}]