[{"id":3386,"web_url":"https://patchwork.libcamera.org/comment/3386/","msgid":"<20200108030010.GZ4871@pendragon.ideasonboard.com>","date":"2020-01-08T03:00:10","subject":"Re: [libcamera-devel] [PATCH v2 18/25] libcamera: pipelines: 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\nIn the subject line, s/pipelines/pipeline/\n\nOn Mon, Dec 30, 2019 at 01:05:03PM +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> ---\n>  src/libcamera/include/pipeline_handler.h |  7 +++\n>  src/libcamera/pipeline/ipu3/ipu3.cpp     | 34 +++++++++++++\n>  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 25 ++++++++++\n>  src/libcamera/pipeline/uvcvideo.cpp      | 31 ++++++++++++\n>  src/libcamera/pipeline/vimc.cpp          | 31 ++++++++++++\n>  src/libcamera/pipeline_handler.cpp       | 62 ++++++++++++++++++++++++\n>  6 files changed, 190 insertions(+)\n> \n> diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h\n> index f3622631022d87b9..520d3fccaaa130cc 100644\n> --- a/src/libcamera/include/pipeline_handler.h\n> +++ b/src/libcamera/include/pipeline_handler.h\n> @@ -69,6 +69,13 @@ public:\n>  \t\tconst StreamRoles &roles) = 0;\n>  \tvirtual int configure(Camera *camera, CameraConfiguration *config) = 0;\n>  \n> +\tvirtual int allocateFrameBuffers(Camera *camera, Stream *stream,\n> +\t\t\t\t\t unsigned int count,\n> +\t\t\t\t\t std::vector<std::unique_ptr<FrameBuffer>> *buffers) = 0;\n> +\tvirtual int importFrameBuffers(Camera *camera, Stream *stream,\n> +\t\t\t\t       unsigned int count) = 0;\n\nallocate vs. import sounds a bit strange. Should we s/allocate/export/ ?\n\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 030ba2b6a0df2e0b..f9bddcc88523301f 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -210,6 +210,13 @@ public:\n>  \t\tconst StreamRoles &roles) override;\n>  \tint configure(Camera *camera, CameraConfiguration *config) override;\n>  \n> +\tint allocateFrameBuffers(Camera *camera, Stream *stream,\n> +\t\t\t\t unsigned int count,\n> +\t\t\t\t std::vector<std::unique_ptr<FrameBuffer>> *buffers) override;\n> +\tint importFrameBuffers(Camera *camera, Stream *stream,\n> +\t\t\t       unsigned int count) 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 +623,33 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)\n>  \treturn 0;\n>  }\n>  \n> +int PipelineHandlerIPU3::allocateFrameBuffers(Camera *camera, Stream *stream,\n> +\t\t\t\t\t      unsigned int count,\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> +\n> +\treturn video->exportBuffers(count, buffers);\n> +}\n> +\n> +int PipelineHandlerIPU3::importFrameBuffers(Camera *camera, Stream *stream,\n> +\t\t\t\t\t    unsigned int count)\n> +{\n> +\tIPU3Stream *ipu3stream = static_cast<IPU3Stream *>(stream);\n> +\tV4L2VideoDevice *video = ipu3stream->device_->dev;\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 ea581aaaa85088cd..e77925f6f9deff08 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -175,6 +175,13 @@ public:\n>  \t\tconst StreamRoles &roles) override;\n>  \tint configure(Camera *camera, CameraConfiguration *config) override;\n>  \n> +\tint allocateFrameBuffers(Camera *camera, Stream *stream,\n> +\t\t\t\t unsigned int count,\n> +\t\t\t\t std::vector<std::unique_ptr<FrameBuffer>> *buffers) override;\n> +\tint importFrameBuffers(Camera *camera, Stream *stream,\n> +\t\t\t       unsigned int count) 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> @@ -666,6 +673,24 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)\n>  \treturn 0;\n>  }\n>  \n> +int PipelineHandlerRkISP1::allocateFrameBuffers(Camera *camera, Stream *stream,\n> +\t\t\t\t\t\tunsigned int count,\n> +\t\t\t\t\t\tstd::vector<std::unique_ptr<FrameBuffer>> *buffers)\n> +{\n> +\treturn video_->exportBuffers(count, buffers);\n> +}\n> +\n> +int PipelineHandlerRkISP1::importFrameBuffers(Camera *camera, Stream *stream,\n> +\t\t\t\t\t      unsigned int count)\n> +{\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 3043366bee38bcfc..655c5e6d96a696d6 100644\n> --- a/src/libcamera/pipeline/uvcvideo.cpp\n> +++ b/src/libcamera/pipeline/uvcvideo.cpp\n> @@ -64,6 +64,13 @@ public:\n>  \t\tconst StreamRoles &roles) override;\n>  \tint configure(Camera *camera, CameraConfiguration *config) override;\n>  \n> +\tint allocateFrameBuffers(Camera *camera, Stream *stream,\n> +\t\t\t\t unsigned int count,\n> +\t\t\t\t std::vector<std::unique_ptr<FrameBuffer>> *buffers) override;\n> +\tint importFrameBuffers(Camera *camera, Stream *stream,\n> +\t\t\t       unsigned int count) 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> @@ -192,6 +199,30 @@ int PipelineHandlerUVC::configure(Camera *camera, CameraConfiguration *config)\n>  \treturn 0;\n>  }\n>  \n> +int PipelineHandlerUVC::allocateFrameBuffers(Camera *camera, Stream *stream,\n> +\t\t\t\t\t     unsigned int count,\n> +\t\t\t\t\t     std::vector<std::unique_ptr<FrameBuffer>> *buffers)\n> +{\n> +\tUVCCameraData *data = cameraData(camera);\n> +\n> +\treturn data->video_->exportBuffers(count, buffers);\n> +}\n> +\n> +int PipelineHandlerUVC::importFrameBuffers(Camera *camera, Stream *stream,\n> +\t\t\t\t\t   unsigned int count)\n> +{\n> +\tUVCCameraData *data = cameraData(camera);\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..02235ffb0515982f 100644\n> --- a/src/libcamera/pipeline/vimc.cpp\n> +++ b/src/libcamera/pipeline/vimc.cpp\n> @@ -82,6 +82,13 @@ public:\n>  \t\tconst StreamRoles &roles) override;\n>  \tint configure(Camera *camera, CameraConfiguration *config) override;\n>  \n> +\tint allocateFrameBuffers(Camera *camera, Stream *stream,\n> +\t\t\t\t unsigned int count,\n> +\t\t\t\t std::vector<std::unique_ptr<FrameBuffer>> *buffers) override;\n> +\tint importFrameBuffers(Camera *camera, Stream *stream,\n> +\t\t\t       unsigned int count) 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 +266,30 @@ int PipelineHandlerVimc::configure(Camera *camera, CameraConfiguration *config)\n>  \treturn 0;\n>  }\n>  \n> +int PipelineHandlerVimc::allocateFrameBuffers(Camera *camera, Stream *stream,\n> +\t\t\t\t\t      unsigned int count,\n> +\t\t\t\t\t      std::vector<std::unique_ptr<FrameBuffer>> *buffers)\n> +{\n> +\tVimcCameraData *data = cameraData(camera);\n> +\n> +\treturn data->video_->exportBuffers(count, buffers);\n> +}\n> +\n> +int PipelineHandlerVimc::importFrameBuffers(Camera *camera, Stream *stream,\n> +\t\t\t\t\t    unsigned int count)\n> +{\n> +\tVimcCameraData *data = cameraData(camera);\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 5badf31ce793edf8..fb69ca8e97216e6c 100644\n> --- a/src/libcamera/pipeline_handler.cpp\n> +++ b/src/libcamera/pipeline_handler.cpp\n> @@ -287,6 +287,68 @@ const ControlInfoMap &PipelineHandler::controls(Camera *camera)\n>   * \\return 0 on success or a negative error code otherwise\n>   */\n>  \n> +/**\n> + * \\fn PipelineHandler::allocateFrameBuffers()\n> + * \\brief Allocate buffers for \\a stream\n> + * \\param[in] camera The camera to allocate buffers on\n\nI think \"The camera\" would be enough. Same for the methods below.\n\n> + * \\param[in] stream The stream to allocate buffers for\n> + * \\param[in] count Number of buffers to allocate\n> + * \\param[out] buffers Array of buffers successfully allocated\n> + *\n> + * Allocate buffers for \\a stream using the resources associated with the stream\n> + * inside the pipelinehandler.\n\ns/pipelinehandler/pipeline handler/\n\n> + *\n> + * The method could be called multiple times to operate on different streams.\n> + * It is not allowed to call allocateFrameBuffers() and importFrameBuffers()\n> + * on the same \\a stream as they are mutually exclusive operations. The\n> + * pipelinehandler must guarantee that all streams exposed as part of the Camera\n> + * can be used simultaneously with all combinations of the two.\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\nThis tells the pipeline handler authors what they need to return.\n\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\nThis tells the pipeline handler authors when they shall expect the\nmethod to be called.\n\n *\n * allocateFrameBuffers() 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 * allocateFrameBuffers() and importFrameBuffers() for the streams contained in\n * any camera configuration.\n\n> + *\n> + * This is a helper which may be used by libcamera helper classes to allocate\n> + * buffers from the stream itself. The allocated buffers may then be treated\n> + * in the same way as if they where externally allocated.\n\nI would drop this paragraph.\n\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 to prepare for import on\n> + * \\param[in] stream The stream to prepare for import\n> + * \\param[in] count Number of buffers to import\n> + *\n> + * Prepare the \\a stream to use \\a count number of buffers allocated outside\n> + * the pipelinehandler.\n> + *\n> + * The method could be called multiple times to operate on different streams.\n> + * It is not allowed to call allocateFrameBuffers() and importFrameBuffers()\n> + * on the same \\a stream as they are mutually exclusive operations. The\n> + * pipelinehandler must guarantee that all streams exposed as part of the Camera\n> + * can be used simultaneously with all combinations of the two.\n> + *\n\n * This method prepares the pipeline handler to use \\a count buffers provided by\n * the 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 allocateFrameBuffers(), 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 * allocateFrameBuffers() and importFrameBuffers() for the streams contained in\n * any camera configuration.\n\nI'm still not 100% sure this API is the best, I wonder if we shouldn't\nhave an optional allocateFrameBuffers() and a prepare() that is called\nunconditionally for all streams, with a parameter telling whether\nallocateFrameBuffers() has been called. This could be done on top of\nthis series, we need to evaluate the impact on pipeline handlers, and\nit's likely easier to do so on top.\n\n> + * This is a helper which may be used by libcamera helper classes to import\n> + * buffers to the \\a stream from external sources.\n\nI would drop this paragraph.\n\n> + * The only intended caller is the FrameBufferAllocator helper.\n\nThat's not true, the only intended caller is the Camera::start() method.\n\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 stram\n\ns/stram/stream/\n\n> + * \\param[in] camera The camera to free buffers on\n> + * \\param[in] stream The stream to free buffers for\n> + *\n> + * This is a helper that frees buffers and resources allocated using\n> + * allocateFrameBuffers() or importFrameBuffers().\n\n * This method shall free all buffers and all other resources allocated for the\n * \\a stream by allocateFrameBuffers() 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> + *\n> + * The only intended caller is the FrameBufferAllocator helper.\n\nThis isn't correct, the method is called by Camera::stop() too.\n\nReviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n\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 33E376045D\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed,  8 Jan 2020 04:00:22 +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 9C57152F;\n\tWed,  8 Jan 2020 04:00:21 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1578452421;\n\tbh=ypAKHCVWeLA4jf2vhn8OaFhEjxbXRjWa5LxfhUrp9No=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=tXmWRDWxLoqdvog/zrrgSDR9PawGc/FTaoONRkFY/d6UwVfJo6Z8abaoA9blHdUNu\n\t0roNP00wz8+Pz6IUBV+o9suoHtw1DsKRD/eovwOQYwa97MjEHOVAmNQ5OSlQ7be7WT\n\tHIab2Wqe5f/uLRZ5PvwAMNiMqweYoeYmvjz9IA/U=","Date":"Wed, 8 Jan 2020 05:00:10 +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":"<20200108030010.GZ4871@pendragon.ideasonboard.com>","References":"<20191230120510.938333-1-niklas.soderlund@ragnatech.se>\n\t<20191230120510.938333-19-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-19-niklas.soderlund@ragnatech.se>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v2 18/25] libcamera: pipelines: 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":"Wed, 08 Jan 2020 03:00:22 -0000"}}]