[{"id":1410,"web_url":"https://patchwork.libcamera.org/comment/1410/","msgid":"<20190416221418.GH4822@pendragon.ideasonboard.com>","date":"2019-04-16T22:14:18","subject":"Re: [libcamera-devel] [PATCH v6 2/7] libcamera: camera: Pass the\n\tstream set to allocate/freeBuffers()","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nThank you for the patch.\n\nOn Tue, Apr 16, 2019 at 03:42:05PM +0200, Jacopo Mondi wrote:\n> Pipeline handlers might need to perform allocation of internal buffers,\n> setup operations, or simple sanity check before going into the\n> per-stream buffer allocation.\n> \n> As of now, PipelineHandler::allocateBuffers() is called once for each\n> active stream, leaving no space for stream-independent configuration.\n> \n> Change this by providing to the pipeline handlers the full set of active\n> streams, and ask them to loop over them to perform per-streams\n> memory allocations and freeing.\n> \n> Reviewed-by: Niklas Söderlund \n> Signed-off-by: Jacopo Mondi \n> ---\n> src/libcamera/camera.cpp | 15 ++++++---------\n> src/libcamera/include/pipeline_handler.h | 6 ++++--\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 13 +++++++++----\n> src/libcamera/pipeline/uvcvideo.cpp | 13 +++++++++----\n> src/libcamera/pipeline/vimc.cpp | 13 +++++++++----\n> src/libcamera/pipeline_handler.cpp | 11 ++++++-----\n> 6 files changed, 43 insertions(+), 28 deletions(-)\n> \n> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> index bdf14b31d8ee..21caa24b90b5 100644\n> --- a/src/libcamera/camera.cpp\n> +++ b/src/libcamera/camera.cpp\n> @@ -647,13 +647,11 @@ int Camera::allocateBuffers()\n> \t\treturn -EINVAL;\n> \t}\n> \n> -\tfor (Stream *stream : activeStreams_) {\n> -\t\tint ret = pipe_->allocateBuffers(this, stream);\n> -\t\tif (ret) {\n> -\t\t\tLOG(Camera, Error) << \"Failed to allocate buffers\";\n> -\t\t\tfreeBuffers();\n> -\t\t\treturn ret;\n> -\t\t}\n> +\tint ret = pipe_->allocateBuffers(this, activeStreams_);\n> +\tif (ret) {\n> +\t\tLOG(Camera, Error) << \"Failed to allocate buffers\";\n> +\t\tfreeBuffers();\n\nI would squash patch 3/7 with this patch. Apart from that,\n\nReviewed-by: Laurent Pinchart \n\n> +\t\treturn ret;\n> \t}\n> \n> \tstate_ = CameraPrepared;\n> @@ -683,12 +681,11 @@ int Camera::freeBuffers()\n> \t\t * by the V4L2 device that has allocated them.\n> \t\t */\n> \t\tstream->bufferPool().destroyBuffers();\n> -\t\tpipe_->freeBuffers(this, stream);\n> \t}\n> \n> \tstate_ = CameraConfigured;\n> \n> -\treturn 0;\n> +\treturn pipe_->freeBuffers(this, activeStreams_);\n> }\n> \n> /**\n> diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h\n> index 33b820e706cc..a0862ebf35df 100644\n> --- a/src/libcamera/include/pipeline_handler.h\n> +++ b/src/libcamera/include/pipeline_handler.h\n> @@ -58,8 +58,10 @@ public:\n> \tstreamConfiguration(Camera *camera, const std::vector &usages) = 0;\n> \tvirtual int configureStreams(Camera *camera, const CameraConfiguration &config) = 0;\n> \n> -\tvirtual int allocateBuffers(Camera *camera, Stream *stream) = 0;\n> -\tvirtual int freeBuffers(Camera *camera, Stream *stream) = 0;\n> +\tvirtual int allocateBuffers(Camera *camera,\n> +\t\t\t\t const std::set &streams) = 0;\n> +\tvirtual int freeBuffers(Camera *camera,\n> +\t\t\t\tconst std::set &streams) = 0;\n> \n> \tvirtual int start(Camera *camera) = 0;\n> \tvirtual void stop(Camera *camera);\n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index ca09da753b90..f96e8763bce9 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -145,8 +145,10 @@ public:\n> \tint configureStreams(Camera *camera,\n> \t\t\t const CameraConfiguration &config) override;\n> \n> -\tint allocateBuffers(Camera *camera, Stream *stream) override;\n> -\tint freeBuffers(Camera *camera, Stream *stream) override;\n> +\tint allocateBuffers(Camera *camera,\n> +\t\t\t const std::set &streams) override;\n> +\tint freeBuffers(Camera *camera,\n> +\t\t\tconst std::set &streams) override;\n> \n> \tint start(Camera *camera) override;\n> \tvoid stop(Camera *camera) override;\n> @@ -305,9 +307,11 @@ int PipelineHandlerIPU3::configureStreams(Camera *camera,\n> \treturn 0;\n> }\n> \n> -int PipelineHandlerIPU3::allocateBuffers(Camera *camera, Stream *stream)\n> +int PipelineHandlerIPU3::allocateBuffers(Camera *camera,\n> +\t\t\t\t\t const std::set &streams)\n> {\n> \tIPU3CameraData *data = cameraData(camera);\n> +\tStream *stream = *streams.begin();\n> \tCIO2Device *cio2 = &data->cio2_;\n> \tImgUDevice *imgu = data->imgu_;\n> \tint ret;\n> @@ -346,7 +350,8 @@ int PipelineHandlerIPU3::allocateBuffers(Camera *camera, Stream *stream)\n> \treturn 0;\n> }\n> \n> -int PipelineHandlerIPU3::freeBuffers(Camera *camera, Stream *stream)\n> +int PipelineHandlerIPU3::freeBuffers(Camera *camera,\n> +\t\t\t\t const std::set &streams)\n> {\n> \tIPU3CameraData *data = cameraData(camera);\n> \n> diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp\n> index cd472cfadd86..b8f634d88b46 100644\n> --- a/src/libcamera/pipeline/uvcvideo.cpp\n> +++ b/src/libcamera/pipeline/uvcvideo.cpp\n> @@ -32,8 +32,10 @@ public:\n> \tint configureStreams(Camera *camera,\n> \t\t\t const CameraConfiguration &config) override;\n> \n> -\tint allocateBuffers(Camera *camera, Stream *stream) override;\n> -\tint freeBuffers(Camera *camera, Stream *stream) override;\n> +\tint allocateBuffers(Camera *camera,\n> +\t\t\t const std::set &streams) override;\n> +\tint freeBuffers(Camera *camera,\n> +\t\t\tconst std::set &streams) override;\n> \n> \tint start(Camera *camera) override;\n> \tvoid stop(Camera *camera) override;\n> @@ -127,9 +129,11 @@ int PipelineHandlerUVC::configureStreams(Camera *camera,\n> \treturn 0;\n> }\n> \n> -int PipelineHandlerUVC::allocateBuffers(Camera *camera, Stream *stream)\n> +int PipelineHandlerUVC::allocateBuffers(Camera *camera,\n> +\t\t\t\t\tconst std::set &streams)\n> {\n> \tUVCCameraData *data = cameraData(camera);\n> +\tStream *stream = *streams.begin();\n> \tconst StreamConfiguration &cfg = stream->configuration();\n> \n> \tLOG(UVC, Debug) << \"Requesting \" << cfg.bufferCount << \" buffers\";\n> @@ -137,7 +141,8 @@ int PipelineHandlerUVC::allocateBuffers(Camera *camera, Stream *stream)\n> \treturn data->video_->exportBuffers(&stream->bufferPool());\n> }\n> \n> -int PipelineHandlerUVC::freeBuffers(Camera *camera, Stream *stream)\n> +int PipelineHandlerUVC::freeBuffers(Camera *camera,\n> +\t\t\t\t const std::set &streams)\n> {\n> \tUVCCameraData *data = cameraData(camera);\n> \treturn data->video_->releaseBuffers();\n> diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp\n> index c8bbe2a19847..22449e47bc2d 100644\n> --- a/src/libcamera/pipeline/vimc.cpp\n> +++ b/src/libcamera/pipeline/vimc.cpp\n> @@ -32,8 +32,10 @@ public:\n> \tint configureStreams(Camera *camera,\n> \t\t\t const CameraConfiguration &config) override;\n> \n> -\tint allocateBuffers(Camera *camera, Stream *stream) override;\n> -\tint freeBuffers(Camera *camera, Stream *stream) override;\n> +\tint allocateBuffers(Camera *camera,\n> +\t\t\t const std::set &streams) override;\n> +\tint freeBuffers(Camera *camera,\n> +\t\t\tconst std::set &streams) override;\n> \n> \tint start(Camera *camera) override;\n> \tvoid stop(Camera *camera) override;\n> @@ -127,9 +129,11 @@ int PipelineHandlerVimc::configureStreams(Camera *camera,\n> \treturn 0;\n> }\n> \n> -int PipelineHandlerVimc::allocateBuffers(Camera *camera, Stream *stream)\n> +int PipelineHandlerVimc::allocateBuffers(Camera *camera,\n> +\t\t\t\t\t const std::set &streams)\n> {\n> \tVimcCameraData *data = cameraData(camera);\n> +\tStream *stream = *streams.begin();\n> \tconst StreamConfiguration &cfg = stream->configuration();\n> \n> \tLOG(VIMC, Debug) << \"Requesting \" << cfg.bufferCount << \" buffers\";\n> @@ -137,7 +141,8 @@ int PipelineHandlerVimc::allocateBuffers(Camera *camera, Stream *stream)\n> \treturn data->video_->exportBuffers(&stream->bufferPool());\n> }\n> \n> -int PipelineHandlerVimc::freeBuffers(Camera *camera, Stream *stream)\n> +int PipelineHandlerVimc::freeBuffers(Camera *camera,\n> +\t\t\t\t const std::set &streams)\n> {\n> \tVimcCameraData *data = cameraData(camera);\n> \treturn data->video_->releaseBuffers();\n> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> index 43550c0e0210..911d08448e69 100644\n> --- a/src/libcamera/pipeline_handler.cpp\n> +++ b/src/libcamera/pipeline_handler.cpp\n> @@ -193,10 +193,11 @@ PipelineHandler::~PipelineHandler()\n> * \\fn PipelineHandler::allocateBuffers()\n> * \\brief Allocate buffers for a stream\n> * \\param[in] camera The camera the \\a stream belongs to\n> - * \\param[in] stream The stream to allocate buffers for\n> + * \\param[in] streams The set of streams to allocate buffers for\n> *\n> - * This method allocates buffers internally in the pipeline handler and\n> - * associates them with the stream's buffer pool.\n> + * This method allocates buffers internally in the pipeline handler for each\n> + * stream in the \\a streams buffer set, and associates them with the stream's\n> + * buffer pool.\n> *\n> * The intended caller of this method is the Camera class.\n> *\n> @@ -207,9 +208,9 @@ PipelineHandler::~PipelineHandler()\n> * \\fn PipelineHandler::freeBuffers()\n> * \\brief Free all buffers associated with a stream\n> * \\param[in] camera The camera the \\a stream belongs to\n> - * \\param[in] stream The stream to free buffers from\n> + * \\param[in] streams The set of streams to free buffers from\n> *\n> - * After a capture session has been stopped all buffers associated with the\n> + * After a capture session has been stopped all buffers associated with each\n> * stream shall be freed.\n> *\n> * The intended caller of this method is the Camera class.","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 660E860004\n\tfor ;\n\tWed, 17 Apr 2019 00:14:27 +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 C3555E2;\n\tWed, 17 Apr 2019 00:14:26 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1555452866;\n\tbh=nkSHRLvOIyRwJiwH22fuvWza71lKdVhmH2HZCNBGty0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Lbc/WeMT/bTJ8s/7+eGjBxXLcDkN2L7oq2ODVfyua0hoXGCipLaUHosuuQY7D0O4n\n\tkU6cpfK4cAofTKBG/+Q6qndBqwKgVZC3M1KBscDbR/ZkZ/AqHyU/EvPLJeElS+m4Vg\n\tBvk9s/pTVvDKwmkq6QCnsAD3L+XfvyPwwMWoNQq0=","Date":"Wed, 17 Apr 2019 01:14:18 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190416221418.GH4822@pendragon.ideasonboard.com>","References":"<20190416134210.21097-1-jacopo@jmondi.org>\n\t<20190416134210.21097-3-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20190416134210.21097-3-jacopo@jmondi.org>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v6 2/7] libcamera: camera: Pass the\n\tstream set to allocate/freeBuffers()","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":"Tue, 16 Apr 2019 22:14:27 -0000"}}]