[{"id":3429,"web_url":"https://patchwork.libcamera.org/comment/3429/","msgid":"<20200112134222.GE25899@pendragon.ideasonboard.com>","date":"2020-01-12T13:42:22","subject":"Re: [libcamera-devel] [PATCH v4 24/32] libcamera: pipeline: Add\n\tFrameBuffer handlers","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 Sun, Jan 12, 2020 at 02:02:04AM +0100, Niklas Söderlund wrote:\n> Extend the pipeline handlers to support the FrameBuffer API with three\n> new methods to handle allocation, importing and freeing of buffers. The\n> new methods will replace allocateBuffers() and freeBuffers().\n> \n> The FrameBuffer API will use the methods on a stream level and either\n> allocate or import buffers for each active stream controlled from the\n> Camera class and an upcoming FrameBufferAllocator helper. With this new\n> API the implementation in pipeline handlers can be made simpler as all\n> streams don't need to be handled in allocateBuffers().\n> \n> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>\n> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n> ---\n> * Changes since v3\n> - Add documentation paragraph for PipelineHandler::exportFrameBuffers()\n>   which was lost from review of v2.\n\nI don't see the paragraph being added below...\n\n> * Changes since v2\n> - In the subject line, s/pipelines/pipeline/\n> - Rename allocateFrameBuffers() to exportFrameBuffers()\n> - Rewrite of documentation, thanks Laurent!\n> - Remove count argument from exportFrameBuffers() and\n>   importFrameBuffers() and instead get the buffer count from the stream\n>   configuration.\n> ---\n>  src/libcamera/include/pipeline_handler.h |  5 ++\n>  src/libcamera/pipeline/ipu3/ipu3.cpp     | 32 +++++++++++\n>  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 23 ++++++++\n>  src/libcamera/pipeline/uvcvideo.cpp      | 29 ++++++++++\n>  src/libcamera/pipeline/vimc.cpp          | 29 ++++++++++\n>  src/libcamera/pipeline_handler.cpp       | 67 ++++++++++++++++++++++++\n>  6 files changed, 185 insertions(+)\n> \n> diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h\n> index 067baef56278b577..560be3bad8d59b20 100644\n> --- a/src/libcamera/include/pipeline_handler.h\n> +++ b/src/libcamera/include/pipeline_handler.h\n> @@ -70,6 +70,11 @@ public:\n>  \t\tconst StreamRoles &roles) = 0;\n>  \tvirtual int configure(Camera *camera, CameraConfiguration *config) = 0;\n>  \n> +\tvirtual int exportFrameBuffers(Camera *camera, Stream *stream,\n> +\t\t\t\t       std::vector<std::unique_ptr<FrameBuffer>> *buffers) = 0;\n> +\tvirtual int importFrameBuffers(Camera *camera, Stream *stream) = 0;\n> +\tvirtual void freeFrameBuffers(Camera *camera, Stream *stream) = 0;\n> +\n>  \tvirtual int allocateBuffers(Camera *camera,\n>  \t\t\t\t    const std::set<Stream *> &streams) = 0;\n>  \tvirtual int freeBuffers(Camera *camera,\n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index 7c4539d488bd2d26..b73f0f0725ee2613 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -210,6 +210,11 @@ public:\n>  \t\tconst StreamRoles &roles) override;\n>  \tint configure(Camera *camera, CameraConfiguration *config) override;\n>  \n> +\tint exportFrameBuffers(Camera *camera, Stream *stream,\n> +\t\t\t       std::vector<std::unique_ptr<FrameBuffer>> *buffers) override;\n> +\tint importFrameBuffers(Camera *camera, Stream *stream) override;\n> +\tvoid freeFrameBuffers(Camera *camera, Stream *stream) override;\n> +\n>  \tint allocateBuffers(Camera *camera,\n>  \t\t\t    const std::set<Stream *> &streams) override;\n>  \tint freeBuffers(Camera *camera,\n> @@ -616,6 +621,33 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)\n>  \treturn 0;\n>  }\n>  \n> +int PipelineHandlerIPU3::exportFrameBuffers(Camera *camera, Stream *stream,\n> +\t\t\t\t\t    std::vector<std::unique_ptr<FrameBuffer>> *buffers)\n> +{\n> +\tIPU3Stream *ipu3stream = static_cast<IPU3Stream *>(stream);\n> +\tV4L2VideoDevice *video = ipu3stream->device_->dev;\n> +\tunsigned int count = stream->configuration().bufferCount;\n> +\n> +\treturn video->exportBuffers(count, buffers);\n> +}\n> +\n> +int PipelineHandlerIPU3::importFrameBuffers(Camera *camera, Stream *stream)\n> +{\n> +\tIPU3Stream *ipu3stream = static_cast<IPU3Stream *>(stream);\n> +\tV4L2VideoDevice *video = ipu3stream->device_->dev;\n> +\tunsigned int count = stream->configuration().bufferCount;\n> +\n> +\treturn video->importBuffers(count);\n> +}\n> +\n> +void PipelineHandlerIPU3::freeFrameBuffers(Camera *camera, Stream *stream)\n> +{\n> +\tIPU3Stream *ipu3stream = static_cast<IPU3Stream *>(stream);\n> +\tV4L2VideoDevice *video = ipu3stream->device_->dev;\n> +\n> +\tvideo->releaseBuffers();\n> +}\n> +\n>  /**\n>   * \\todo Clarify if 'viewfinder' and 'stat' nodes have to be set up and\n>   * started even if not in use. As of now, if not properly configured and\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index dbc5df801f30e76b..ba8f93e8584c97a9 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -174,6 +174,11 @@ public:\n>  \t\tconst StreamRoles &roles) override;\n>  \tint configure(Camera *camera, CameraConfiguration *config) override;\n>  \n> +\tint exportFrameBuffers(Camera *camera, Stream *stream,\n> +\t\t\t       std::vector<std::unique_ptr<FrameBuffer>> *buffers) override;\n> +\tint importFrameBuffers(Camera *camera, Stream *stream) override;\n> +\tvoid freeFrameBuffers(Camera *camera, Stream *stream) override;\n> +\n>  \tint allocateBuffers(Camera *camera,\n>  \t\tconst std::set<Stream *> &streams) override;\n>  \tint freeBuffers(Camera *camera,\n> @@ -665,6 +670,24 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)\n>  \treturn 0;\n>  }\n>  \n> +int PipelineHandlerRkISP1::exportFrameBuffers(Camera *camera, Stream *stream,\n> +\t\t\t\t\t      std::vector<std::unique_ptr<FrameBuffer>> *buffers)\n> +{\n> +\tunsigned int count = stream->configuration().bufferCount;\n> +\treturn video_->exportBuffers(count, buffers);\n> +}\n> +\n> +int PipelineHandlerRkISP1::importFrameBuffers(Camera *camera, Stream *stream)\n> +{\n> +\tunsigned int count = stream->configuration().bufferCount;\n> +\treturn video_->importBuffers(count);\n> +}\n> +\n> +void PipelineHandlerRkISP1::freeFrameBuffers(Camera *camera, Stream *stream)\n> +{\n> +\tvideo_->releaseBuffers();\n> +}\n> +\n>  int PipelineHandlerRkISP1::allocateBuffers(Camera *camera,\n>  \t\t\t\t\t   const std::set<Stream *> &streams)\n>  {\n> diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp\n> index ce38445d2a48ca82..b72841edee572f99 100644\n> --- a/src/libcamera/pipeline/uvcvideo.cpp\n> +++ b/src/libcamera/pipeline/uvcvideo.cpp\n> @@ -65,6 +65,11 @@ public:\n>  \t\tconst StreamRoles &roles) override;\n>  \tint configure(Camera *camera, CameraConfiguration *config) override;\n>  \n> +\tint exportFrameBuffers(Camera *camera, Stream *stream,\n> +\t\t\t       std::vector<std::unique_ptr<FrameBuffer>> *buffers) override;\n> +\tint importFrameBuffers(Camera *camera, Stream *stream) override;\n> +\tvoid freeFrameBuffers(Camera *camera, Stream *stream) override;\n> +\n>  \tint allocateBuffers(Camera *camera,\n>  \t\t\t    const std::set<Stream *> &streams) override;\n>  \tint freeBuffers(Camera *camera,\n> @@ -193,6 +198,30 @@ int PipelineHandlerUVC::configure(Camera *camera, CameraConfiguration *config)\n>  \treturn 0;\n>  }\n>  \n> +int PipelineHandlerUVC::exportFrameBuffers(Camera *camera, Stream *stream,\n> +\t\t\t\t\t   std::vector<std::unique_ptr<FrameBuffer>> *buffers)\n> +{\n> +\tUVCCameraData *data = cameraData(camera);\n> +\tunsigned int count = stream->configuration().bufferCount;\n> +\n> +\treturn data->video_->exportBuffers(count, buffers);\n> +}\n> +\n> +int PipelineHandlerUVC::importFrameBuffers(Camera *camera, Stream *stream)\n> +{\n> +\tUVCCameraData *data = cameraData(camera);\n> +\tunsigned int count = stream->configuration().bufferCount;\n> +\n> +\treturn data->video_->importBuffers(count);\n> +}\n> +\n> +void PipelineHandlerUVC::freeFrameBuffers(Camera *camera, Stream *stream)\n> +{\n> +\tUVCCameraData *data = cameraData(camera);\n> +\n> +\tdata->video_->releaseBuffers();\n> +}\n> +\n>  int PipelineHandlerUVC::allocateBuffers(Camera *camera,\n>  \t\t\t\t\tconst std::set<Stream *> &streams)\n>  {\n> diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp\n> index 292900bcf8d1b359..3fb5cacde0dab5b6 100644\n> --- a/src/libcamera/pipeline/vimc.cpp\n> +++ b/src/libcamera/pipeline/vimc.cpp\n> @@ -82,6 +82,11 @@ public:\n>  \t\tconst StreamRoles &roles) override;\n>  \tint configure(Camera *camera, CameraConfiguration *config) override;\n>  \n> +\tint exportFrameBuffers(Camera *camera, Stream *stream,\n> +\t\t\t       std::vector<std::unique_ptr<FrameBuffer>> *buffers) override;\n> +\tint importFrameBuffers(Camera *camera, Stream *stream) override;\n> +\tvoid freeFrameBuffers(Camera *camera, Stream *stream) override;\n> +\n>  \tint allocateBuffers(Camera *camera,\n>  \t\t\t    const std::set<Stream *> &streams) override;\n>  \tint freeBuffers(Camera *camera,\n> @@ -259,6 +264,30 @@ int PipelineHandlerVimc::configure(Camera *camera, CameraConfiguration *config)\n>  \treturn 0;\n>  }\n>  \n> +int PipelineHandlerVimc::exportFrameBuffers(Camera *camera, Stream *stream,\n> +\t\t\t\t\t    std::vector<std::unique_ptr<FrameBuffer>> *buffers)\n> +{\n> +\tVimcCameraData *data = cameraData(camera);\n> +\tunsigned int count = stream->configuration().bufferCount;\n> +\n> +\treturn data->video_->exportBuffers(count, buffers);\n> +}\n> +\n> +int PipelineHandlerVimc::importFrameBuffers(Camera *camera, Stream *stream)\n> +{\n> +\tVimcCameraData *data = cameraData(camera);\n> +\tunsigned int count = stream->configuration().bufferCount;\n> +\n> +\treturn data->video_->importBuffers(count);\n> +}\n> +\n> +void PipelineHandlerVimc::freeFrameBuffers(Camera *camera, Stream *stream)\n> +{\n> +\tVimcCameraData *data = cameraData(camera);\n> +\n> +\tdata->video_->releaseBuffers();\n> +}\n> +\n>  int PipelineHandlerVimc::allocateBuffers(Camera *camera,\n>  \t\t\t\t\t const std::set<Stream *> &streams)\n>  {\n> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> index 698dd52560797d7c..572f751b5217eb5f 100644\n> --- a/src/libcamera/pipeline_handler.cpp\n> +++ b/src/libcamera/pipeline_handler.cpp\n> @@ -289,6 +289,73 @@ const ControlInfoMap &PipelineHandler::controls(Camera *camera)\n>   * \\return 0 on success or a negative error code otherwise\n>   */\n>  \n> +/**\n> + * \\fn PipelineHandler::exportFrameBuffers()\n> + * \\brief Allocate buffers for \\a stream\n> + * \\param[in] camera The camera\n> + * \\param[in] stream The stream to allocate buffers for\n> + * \\param[out] buffers Array of buffers successfully allocated\n> + *\n> + * This method allocates buffers for the \\a stream from the devices associated\n> + * with the stream in the corresponding pipeline handler. Those buffers shall be\n> + * suitable to be added to a Request for the stream, and shall be mappable to\n> + * the CPU through their associated dmabufs with mmap().\n> + *\n> + * exportFrameBuffers() shall also allocate all other resources required by\n> + * the pipeline handler for the stream to prepare for starting the Camera. This\n> + * responsibility is shared with importFrameBuffers(), and one and only one of\n> + * those two methods shall be called for each stream until the buffers are\n> + * freed. The pipeline handler shall support all combinations of\n> + * exportFrameBuffers() and importFrameBuffers() for the streams contained in\n> + * any camera configuration.\n> + *\n> + * The only intended caller is the FrameBufferAllocator helper.\n> + *\n> + * \\return 0 on success or a negative error code otherwise\n> + */\n> +\n> +/**\n> + * \\fn PipelineHandler::importFrameBuffers()\n> + * \\brief Prepare \\a stream to use external buffers\n> + * \\param[in] camera The camera\n> + * \\param[in] stream The stream to prepare for import\n> + *\n> + * This method prepares the pipeline handler to use buffers provided by the\n> + * application for the \\a stream.\n> + *\n> + * The method may only be called after the Camera has been configured and before\n> + * it gets started, or after it gets stopped. It shall be called only for\n> + * streams that are part of the active camera configuration, and at most once\n> + * per stream until buffers for the stream are freed with freeFrameBuffers().\n> + *\n> + * importFrameBuffers() shall also allocate all other resources required by the\n> + * pipeline handler for the stream to prepare for starting the Camera. This\n> + * responsibility is shared with exportFrameBuffers(), and one and only one of\n> + * those two methods shall be called for each stream until the buffers are\n> + * freed. The pipeline handler shall support all combinations of\n> + * exportFrameBuffers() and importFrameBuffers() for the streams contained in\n> + * any camera configuration.\n> + *\n> + * The only intended caller is Camera::start().\n> + *\n> + * \\return 0 on success or a negative error code otherwise\n> + */\n> +\n> +/**\n> + * \\fn PipelineHandler::freeFrameBuffers()\n> + * \\brief Free buffers allocated from the stream\n> + * \\param[in] camera The camera\n> + * \\param[in] stream The stream to free buffers for\n> + *\n> + * This method shall free all buffers and all other resources allocated for the\n> + * \\a stream by exportFrameBuffers() or importFrameBuffers(). It shall be\n> + * called only after a successful call to either of these two methods, and only\n> + * once per stream.\n> + *\n> + * The only intended callers are Camera::stop() and the FrameBufferAllocator\n> + * helper.\n> + */\n> +\n>  /**\n>   * \\fn PipelineHandler::allocateBuffers()\n>   * \\brief Allocate buffers for a stream","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 DF0826045E\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tSun, 12 Jan 2020 14:42:41 +0100 (CET)","from pendragon.ideasonboard.com (85-76-9-232-nat.elisa-mobile.fi\n\t[85.76.9.232])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id D3D8E30F;\n\tSun, 12 Jan 2020 14:42:39 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1578836561;\n\tbh=v7MErv8f3fhVPAR13BXMRcbCJLmbdng0aroAQHnOG9o=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=MM1vkOyWQuYQpXH/JteJSAPufz8HoP935UBTvWn5az/X2NQK4cSZkEsE9qdHalnH2\n\tZvQp2NhIU9yzs0iTntwLEwISDbevn2PZwBauQ8mZutGDtD44LtigIlhwiQvsEaHCCM\n\tX1a4HTJ/8sp/TqQJwN0Qjc4y66aN8kyIuWpB+mJE=","Date":"Sun, 12 Jan 2020 15:42:22 +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":"<20200112134222.GE25899@pendragon.ideasonboard.com>","References":"<20200112010212.2609025-1-niklas.soderlund@ragnatech.se>\n\t<20200112010212.2609025-25-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":"<20200112010212.2609025-25-niklas.soderlund@ragnatech.se>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v4 24/32] libcamera: pipeline: Add\n\tFrameBuffer handlers","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":"Sun, 12 Jan 2020 13:42:42 -0000"}}]