[{"id":3255,"web_url":"https://patchwork.libcamera.org/comment/3255/","msgid":"<20191215092842.GB4889@pendragon.ideasonboard.com>","date":"2019-12-15T09:28:42","subject":"Re: [libcamera-devel] [PATCH 29/30] libcamera: pipeline: Remove\n\texplicit buffer handling","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 Wed, Nov 27, 2019 at 12:36:19AM +0100, Niklas Söderlund wrote:\n> With FrameBuffer interface in place there is no need for the Camera to\n\ns/FrameBuffer/the FrameBuffer/\n\n> call into the specific pipelines allocation and freeing of buffers as it\n> no longer needs to be synced with buffer allocation by the application.\n\ns/synced/synchronized/\n\n> Remove the function prototypes in the pipeline handler base class and\n> fold the functionality in the pipelines start() and stop() functions\n> where needed. A follow up patch will remove the now no-op\n> Camera::allocateBuffers() and Camera::freeBuffers().\n> \n> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>\n> ---\n>  src/libcamera/camera.cpp                 |  8 +------\n>  src/libcamera/include/pipeline_handler.h |  5 ----\n>  src/libcamera/pipeline/ipu3/ipu3.cpp     | 21 +++++++++--------\n>  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 21 ++++++++++-------\n>  src/libcamera/pipeline/uvcvideo.cpp      | 17 --------------\n>  src/libcamera/pipeline/vimc.cpp          | 17 --------------\n>  src/libcamera/pipeline_handler.cpp       | 29 ------------------------\n>  7 files changed, 26 insertions(+), 92 deletions(-)\n> \n> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> index 05fdcaab8f918afa..12878a1c2e6076d3 100644\n> --- a/src/libcamera/camera.cpp\n> +++ b/src/libcamera/camera.cpp\n> @@ -709,12 +709,6 @@ int Camera::allocateBuffers()\n>  \t\treturn -EINVAL;\n>  \t}\n>  \n> -\tint ret = pipe_->allocateBuffers(this, activeStreams_);\n> -\tif (ret) {\n> -\t\tLOG(Camera, Error) << \"Failed to allocate buffers\";\n> -\t\treturn ret;\n> -\t}\n> -\n>  \tstate_ = CameraPrepared;\n>  \n>  \treturn 0;\n> @@ -735,7 +729,7 @@ int Camera::freeBuffers()\n>  \n>  \tstate_ = CameraConfigured;\n>  \n> -\treturn pipe_->freeBuffers(this, activeStreams_);\n> +\treturn 0;\n>  }\n>  \n>  /**\n> diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h\n> index e4588f10bac0228c..9e5dca894aff45ae 100644\n> --- a/src/libcamera/include/pipeline_handler.h\n> +++ b/src/libcamera/include/pipeline_handler.h\n> @@ -69,11 +69,6 @@ public:\n>  \t\tconst StreamRoles &roles) = 0;\n>  \tvirtual int configure(Camera *camera, CameraConfiguration *config) = 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> -\t\t\t\tconst std::set<Stream *> &streams) = 0;\n> -\n>  \tvirtual int start(Camera *camera) = 0;\n>  \tvirtual void stop(Camera *camera) = 0;\n>  \n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index 8d9420aea3eb0b21..6d6ecb511777ec49 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -215,10 +215,8 @@ public:\n>  \t\tconst StreamRoles &roles) override;\n>  \tint configure(Camera *camera, CameraConfiguration *config) override;\n>  \n> -\tint allocateBuffers(Camera *camera,\n> -\t\t\t    const std::set<Stream *> &streams) override;\n> -\tint freeBuffers(Camera *camera,\n> -\t\t\tconst std::set<Stream *> &streams) override;\n> +\tint allocateBuffers(Camera *camera);\n> +\tint freeBuffers(Camera *camera);\n\nShouldn't those methods be made private ?\n\n>  \n>  \tint start(Camera *camera) override;\n>  \tvoid stop(Camera *camera) override;\n> @@ -629,8 +627,7 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)\n>   * In order to be able to start the 'viewfinder' and 'stat' nodes, we need\n>   * memory to be reserved.\n>   */\n> -int PipelineHandlerIPU3::allocateBuffers(Camera *camera,\n> -\t\t\t\t\t const std::set<Stream *> &streams)\n> +int PipelineHandlerIPU3::allocateBuffers(Camera *camera)\n>  {\n>  \tIPU3CameraData *data = cameraData(camera);\n>  \tunsigned int bufferCount;\n> @@ -670,13 +667,12 @@ int PipelineHandlerIPU3::allocateBuffers(Camera *camera,\n>  \n>  \treturn 0;\n>  error:\n> -\tfreeBuffers(camera, streams);\n> +\tfreeBuffers(camera);\n>  \n>  \treturn ret;\n>  }\n>  \n> -int PipelineHandlerIPU3::freeBuffers(Camera *camera,\n> -\t\t\t\t     const std::set<Stream *> &streams)\n> +int PipelineHandlerIPU3::freeBuffers(Camera *camera)\n>  {\n>  \tIPU3CameraData *data = cameraData(camera);\n>  \n> @@ -693,6 +689,10 @@ int PipelineHandlerIPU3::start(Camera *camera)\n>  \tImgUDevice *imgu = data->imgu_;\n>  \tint ret;\n>  \n> +\tret = allocateBuffers(camera);\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n>  \t/*\n>  \t * Start the ImgU video devices, buffers will be queued to the\n>  \t * ImgU output and viewfinder when requests will be queued.\n> @@ -711,6 +711,7 @@ int PipelineHandlerIPU3::start(Camera *camera)\n>  \treturn 0;\n>  \n>  error:\n> +\tfreeBuffers(camera);\n>  \tLOG(IPU3, Error) << \"Failed to start camera \" << camera->name();\n>  \n>  \treturn ret;\n> @@ -726,6 +727,8 @@ void PipelineHandlerIPU3::stop(Camera *camera)\n>  \tif (ret)\n>  \t\tLOG(IPU3, Warning) << \"Failed to stop camera \"\n>  \t\t\t\t   << camera->name();\n> +\n> +\tfreeBuffers(camera);\n>  }\n>  \n>  int PipelineHandlerIPU3::queueRequestHardware(Camera *camera, Request *request)\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index 96e863f0208fa748..a56c7022f57ce771 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -174,10 +174,8 @@ public:\n>  \t\tconst StreamRoles &roles) override;\n>  \tint configure(Camera *camera, CameraConfiguration *config) override;\n>  \n> -\tint allocateBuffers(Camera *camera,\n> -\t\tconst std::set<Stream *> &streams) override;\n> -\tint freeBuffers(Camera *camera,\n> -\t\tconst std::set<Stream *> &streams) override;\n> +\tint allocateBuffers(Camera *camera);\n> +\tint freeBuffers(Camera *camera);\n\nSame here, shouldn't these methods be made private ?\n\nWith this fixed,\n\nReviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n\n>  \n>  \tint start(Camera *camera) override;\n>  \tvoid stop(Camera *camera) override;\n> @@ -651,8 +649,7 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)\n>  \treturn 0;\n>  }\n>  \n> -int PipelineHandlerRkISP1::allocateBuffers(Camera *camera,\n> -\t\t\t\t\t   const std::set<Stream *> &streams)\n> +int PipelineHandlerRkISP1::allocateBuffers(Camera *camera)\n>  {\n>  \tRkISP1CameraData *data = cameraData(camera);\n>  \tstd::vector<FrameBuffer *> paramBuffers, statBuffers;\n> @@ -695,8 +692,7 @@ err:\n>  \treturn ret;\n>  }\n>  \n> -int PipelineHandlerRkISP1::freeBuffers(Camera *camera,\n> -\t\t\t\t       const std::set<Stream *> &streams)\n> +int PipelineHandlerRkISP1::freeBuffers(Camera *camera)\n>  {\n>  \tRkISP1CameraData *data = cameraData(camera);\n>  \n> @@ -731,10 +727,15 @@ int PipelineHandlerRkISP1::start(Camera *camera)\n>  \tRkISP1CameraData *data = cameraData(camera);\n>  \tint ret;\n>  \n> +\tret = allocateBuffers(camera);\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n>  \tdata->frame_ = 0;\n>  \n>  \tret = param_->streamOn();\n>  \tif (ret) {\n> +\t\tfreeBuffers(camera);\n>  \t\tLOG(RkISP1, Error)\n>  \t\t\t<< \"Failed to start parameters \" << camera->name();\n>  \t\treturn ret;\n> @@ -743,6 +744,7 @@ int PipelineHandlerRkISP1::start(Camera *camera)\n>  \tret = stat_->streamOn();\n>  \tif (ret) {\n>  \t\tparam_->streamOff();\n> +\t\tfreeBuffers(camera);\n>  \t\tLOG(RkISP1, Error)\n>  \t\t\t<< \"Failed to start statistics \" << camera->name();\n>  \t\treturn ret;\n> @@ -752,6 +754,7 @@ int PipelineHandlerRkISP1::start(Camera *camera)\n>  \tif (ret) {\n>  \t\tparam_->streamOff();\n>  \t\tstat_->streamOff();\n> +\t\tfreeBuffers(camera);\n>  \n>  \t\tLOG(RkISP1, Error)\n>  \t\t\t<< \"Failed to start camera \" << camera->name();\n> @@ -796,6 +799,8 @@ void PipelineHandlerRkISP1::stop(Camera *camera)\n>  \n>  \tdata->timeline_.reset();\n>  \n> +\tfreeBuffers(camera);\n> +\n>  \tactiveCamera_ = nullptr;\n>  }\n>  \n> diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp\n> index c275b9f0f75429bd..2be7864ef59e932b 100644\n> --- a/src/libcamera/pipeline/uvcvideo.cpp\n> +++ b/src/libcamera/pipeline/uvcvideo.cpp\n> @@ -65,11 +65,6 @@ public:\n>  \t\tconst StreamRoles &roles) override;\n>  \tint configure(Camera *camera, CameraConfiguration *config) override;\n>  \n> -\tint allocateBuffers(Camera *camera,\n> -\t\t\t    const std::set<Stream *> &streams) override;\n> -\tint freeBuffers(Camera *camera,\n> -\t\t\tconst std::set<Stream *> &streams) override;\n> -\n>  \tint start(Camera *camera) override;\n>  \tvoid stop(Camera *camera) override;\n>  \n> @@ -193,18 +188,6 @@ int PipelineHandlerUVC::configure(Camera *camera, CameraConfiguration *config)\n>  \treturn 0;\n>  }\n>  \n> -int PipelineHandlerUVC::allocateBuffers(Camera *camera,\n> -\t\t\t\t\tconst std::set<Stream *> &streams)\n> -{\n> -\treturn 0;\n> -}\n> -\n> -int PipelineHandlerUVC::freeBuffers(Camera *camera,\n> -\t\t\t\t    const std::set<Stream *> &streams)\n> -{\n> -\treturn 0;\n> -}\n> -\n>  int PipelineHandlerUVC::start(Camera *camera)\n>  {\n>  \tUVCCameraData *data = cameraData(camera);\n> diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp\n> index 64793788c752c791..cacf8bfe3ff9cc45 100644\n> --- a/src/libcamera/pipeline/vimc.cpp\n> +++ b/src/libcamera/pipeline/vimc.cpp\n> @@ -84,11 +84,6 @@ public:\n>  \t\tconst StreamRoles &roles) override;\n>  \tint configure(Camera *camera, CameraConfiguration *config) override;\n>  \n> -\tint allocateBuffers(Camera *camera,\n> -\t\t\t    const std::set<Stream *> &streams) override;\n> -\tint freeBuffers(Camera *camera,\n> -\t\t\tconst std::set<Stream *> &streams) override;\n> -\n>  \tint start(Camera *camera) override;\n>  \tvoid stop(Camera *camera) override;\n>  \n> @@ -261,18 +256,6 @@ int PipelineHandlerVimc::configure(Camera *camera, CameraConfiguration *config)\n>  \treturn 0;\n>  }\n>  \n> -int PipelineHandlerVimc::allocateBuffers(Camera *camera,\n> -\t\t\t\t\t const std::set<Stream *> &streams)\n> -{\n> -\treturn 0;\n> -}\n> -\n> -int PipelineHandlerVimc::freeBuffers(Camera *camera,\n> -\t\t\t\t     const std::set<Stream *> &streams)\n> -{\n> -\treturn 0;\n> -}\n> -\n>  int PipelineHandlerVimc::start(Camera *camera)\n>  {\n>  \tVimcCameraData *data = cameraData(camera);\n> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> index b107e23258dee692..67758f3f1ebc39e5 100644\n> --- a/src/libcamera/pipeline_handler.cpp\n> +++ b/src/libcamera/pipeline_handler.cpp\n> @@ -287,35 +287,6 @@ const ControlInfoMap &PipelineHandler::controls(Camera *camera)\n>   * \\return 0 on success or a negative error code otherwise\n>   */\n>  \n> -/**\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] streams The set of streams to allocate buffers for\n> - *\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> - * \\return 0 on success or a negative error code otherwise\n> - */\n> -\n> -/**\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] streams The set of streams to free buffers from\n> - *\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.\n> - *\n> - * \\return 0 on success or a negative error code otherwise\n> - */\n> -\n>  /**\n>   * \\fn PipelineHandler::start()\n>   * \\brief Start capturing from a group of streams","headers":{"Return-Path":"<laurent.pinchart@ideasonboard.com>","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id CB748601E4\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tSun, 15 Dec 2019 10:28:51 +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 4FF7F2D1;\n\tSun, 15 Dec 2019 10:28:51 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1576402131;\n\tbh=Tuo4swWWBuaBr3lHnu8lGzGO/MBuAaYrxM+W5daL50s=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Jw4BXHMZsG3eyy+ab/mF7S34il/48AxJ3sPWpgmvig3f6021jcgXlHK2u6bvtQECL\n\tdQy8Kjp5yvTZjv4a+jyrIbUZ9bzywrspLUIsdKgpoVNU/WedYi7SXvhDEzwAE099qm\n\tnyP94hQtR3GfpxdkXxAfmNYLHbASQ/msH1sreAPw=","Date":"Sun, 15 Dec 2019 11:28:42 +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":"<20191215092842.GB4889@pendragon.ideasonboard.com>","References":"<20191126233620.1695316-1-niklas.soderlund@ragnatech.se>\n\t<20191126233620.1695316-30-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":"<20191126233620.1695316-30-niklas.soderlund@ragnatech.se>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 29/30] libcamera: pipeline: Remove\n\texplicit buffer handling","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, 15 Dec 2019 09:28:52 -0000"}},{"id":3258,"web_url":"https://patchwork.libcamera.org/comment/3258/","msgid":"<20191215140103.GA14495@pendragon.ideasonboard.com>","date":"2019-12-15T14:01:03","subject":"Re: [libcamera-devel] [PATCH 29/30] libcamera: pipeline: Remove\n\texplicit buffer handling","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Niklas,\n\nOne additional comment.\n\nOn Sun, Dec 15, 2019 at 11:28:42AM +0200, Laurent Pinchart wrote:\n> On Wed, Nov 27, 2019 at 12:36:19AM +0100, Niklas Söderlund wrote:\n> > With FrameBuffer interface in place there is no need for the Camera to\n> \n> s/FrameBuffer/the FrameBuffer/\n> \n> > call into the specific pipelines allocation and freeing of buffers as it\n> > no longer needs to be synced with buffer allocation by the application.\n> \n> s/synced/synchronized/\n> \n> > Remove the function prototypes in the pipeline handler base class and\n> > fold the functionality in the pipelines start() and stop() functions\n> > where needed. A follow up patch will remove the now no-op\n> > Camera::allocateBuffers() and Camera::freeBuffers().\n> > \n> > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>\n> > ---\n> >  src/libcamera/camera.cpp                 |  8 +------\n> >  src/libcamera/include/pipeline_handler.h |  5 ----\n> >  src/libcamera/pipeline/ipu3/ipu3.cpp     | 21 +++++++++--------\n> >  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 21 ++++++++++-------\n> >  src/libcamera/pipeline/uvcvideo.cpp      | 17 --------------\n> >  src/libcamera/pipeline/vimc.cpp          | 17 --------------\n> >  src/libcamera/pipeline_handler.cpp       | 29 ------------------------\n> >  7 files changed, 26 insertions(+), 92 deletions(-)\n> > \n> > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> > index 05fdcaab8f918afa..12878a1c2e6076d3 100644\n> > --- a/src/libcamera/camera.cpp\n> > +++ b/src/libcamera/camera.cpp\n> > @@ -709,12 +709,6 @@ int Camera::allocateBuffers()\n> >  \t\treturn -EINVAL;\n> >  \t}\n> >  \n> > -\tint ret = pipe_->allocateBuffers(this, activeStreams_);\n> > -\tif (ret) {\n> > -\t\tLOG(Camera, Error) << \"Failed to allocate buffers\";\n> > -\t\treturn ret;\n> > -\t}\n> > -\n> >  \tstate_ = CameraPrepared;\n> >  \n> >  \treturn 0;\n> > @@ -735,7 +729,7 @@ int Camera::freeBuffers()\n> >  \n> >  \tstate_ = CameraConfigured;\n> >  \n> > -\treturn pipe_->freeBuffers(this, activeStreams_);\n> > +\treturn 0;\n> >  }\n> >  \n> >  /**\n> > diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h\n> > index e4588f10bac0228c..9e5dca894aff45ae 100644\n> > --- a/src/libcamera/include/pipeline_handler.h\n> > +++ b/src/libcamera/include/pipeline_handler.h\n> > @@ -69,11 +69,6 @@ public:\n> >  \t\tconst StreamRoles &roles) = 0;\n> >  \tvirtual int configure(Camera *camera, CameraConfiguration *config) = 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> > -\t\t\t\tconst std::set<Stream *> &streams) = 0;\n> > -\n> >  \tvirtual int start(Camera *camera) = 0;\n> >  \tvirtual void stop(Camera *camera) = 0;\n> >  \n> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > index 8d9420aea3eb0b21..6d6ecb511777ec49 100644\n> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > @@ -215,10 +215,8 @@ public:\n> >  \t\tconst StreamRoles &roles) override;\n> >  \tint configure(Camera *camera, CameraConfiguration *config) override;\n> >  \n> > -\tint allocateBuffers(Camera *camera,\n> > -\t\t\t    const std::set<Stream *> &streams) override;\n> > -\tint freeBuffers(Camera *camera,\n> > -\t\t\tconst std::set<Stream *> &streams) override;\n> > +\tint allocateBuffers(Camera *camera);\n> > +\tint freeBuffers(Camera *camera);\n> \n> Shouldn't those methods be made private ?\n> \n> >  \n> >  \tint start(Camera *camera) override;\n> >  \tvoid stop(Camera *camera) override;\n> > @@ -629,8 +627,7 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)\n> >   * In order to be able to start the 'viewfinder' and 'stat' nodes, we need\n> >   * memory to be reserved.\n> >   */\n> > -int PipelineHandlerIPU3::allocateBuffers(Camera *camera,\n> > -\t\t\t\t\t const std::set<Stream *> &streams)\n> > +int PipelineHandlerIPU3::allocateBuffers(Camera *camera)\n> >  {\n> >  \tIPU3CameraData *data = cameraData(camera);\n> >  \tunsigned int bufferCount;\n> > @@ -670,13 +667,12 @@ int PipelineHandlerIPU3::allocateBuffers(Camera *camera,\n> >  \n> >  \treturn 0;\n> >  error:\n> > -\tfreeBuffers(camera, streams);\n> > +\tfreeBuffers(camera);\n> >  \n> >  \treturn ret;\n> >  }\n> >  \n> > -int PipelineHandlerIPU3::freeBuffers(Camera *camera,\n> > -\t\t\t\t     const std::set<Stream *> &streams)\n> > +int PipelineHandlerIPU3::freeBuffers(Camera *camera)\n> >  {\n> >  \tIPU3CameraData *data = cameraData(camera);\n> >  \n> > @@ -693,6 +689,10 @@ int PipelineHandlerIPU3::start(Camera *camera)\n> >  \tImgUDevice *imgu = data->imgu_;\n> >  \tint ret;\n> >  \n\nI think we need a comment here to explain why buffers need to be\nallocated here, and what buffers are being allocated. This code will be\nused as a reference implementation, so it should have detailed comments.\nSame for the RkISP1 pipeline handler.\n\n> > +\tret = allocateBuffers(camera);\n> > +\tif (ret)\n> > +\t\treturn ret;\n> > +\n> >  \t/*\n> >  \t * Start the ImgU video devices, buffers will be queued to the\n> >  \t * ImgU output and viewfinder when requests will be queued.\n> > @@ -711,6 +711,7 @@ int PipelineHandlerIPU3::start(Camera *camera)\n> >  \treturn 0;\n> >  \n> >  error:\n> > +\tfreeBuffers(camera);\n> >  \tLOG(IPU3, Error) << \"Failed to start camera \" << camera->name();\n> >  \n> >  \treturn ret;\n> > @@ -726,6 +727,8 @@ void PipelineHandlerIPU3::stop(Camera *camera)\n> >  \tif (ret)\n> >  \t\tLOG(IPU3, Warning) << \"Failed to stop camera \"\n> >  \t\t\t\t   << camera->name();\n> > +\n> > +\tfreeBuffers(camera);\n> >  }\n> >  \n> >  int PipelineHandlerIPU3::queueRequestHardware(Camera *camera, Request *request)\n> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > index 96e863f0208fa748..a56c7022f57ce771 100644\n> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > @@ -174,10 +174,8 @@ public:\n> >  \t\tconst StreamRoles &roles) override;\n> >  \tint configure(Camera *camera, CameraConfiguration *config) override;\n> >  \n> > -\tint allocateBuffers(Camera *camera,\n> > -\t\tconst std::set<Stream *> &streams) override;\n> > -\tint freeBuffers(Camera *camera,\n> > -\t\tconst std::set<Stream *> &streams) override;\n> > +\tint allocateBuffers(Camera *camera);\n> > +\tint freeBuffers(Camera *camera);\n> \n> Same here, shouldn't these methods be made private ?\n> \n> With this fixed,\n> \n> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n> \n> >  \n> >  \tint start(Camera *camera) override;\n> >  \tvoid stop(Camera *camera) override;\n> > @@ -651,8 +649,7 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)\n> >  \treturn 0;\n> >  }\n> >  \n> > -int PipelineHandlerRkISP1::allocateBuffers(Camera *camera,\n> > -\t\t\t\t\t   const std::set<Stream *> &streams)\n> > +int PipelineHandlerRkISP1::allocateBuffers(Camera *camera)\n> >  {\n> >  \tRkISP1CameraData *data = cameraData(camera);\n> >  \tstd::vector<FrameBuffer *> paramBuffers, statBuffers;\n> > @@ -695,8 +692,7 @@ err:\n> >  \treturn ret;\n> >  }\n> >  \n> > -int PipelineHandlerRkISP1::freeBuffers(Camera *camera,\n> > -\t\t\t\t       const std::set<Stream *> &streams)\n> > +int PipelineHandlerRkISP1::freeBuffers(Camera *camera)\n> >  {\n> >  \tRkISP1CameraData *data = cameraData(camera);\n> >  \n> > @@ -731,10 +727,15 @@ int PipelineHandlerRkISP1::start(Camera *camera)\n> >  \tRkISP1CameraData *data = cameraData(camera);\n> >  \tint ret;\n> >  \n> > +\tret = allocateBuffers(camera);\n> > +\tif (ret)\n> > +\t\treturn ret;\n> > +\n> >  \tdata->frame_ = 0;\n> >  \n> >  \tret = param_->streamOn();\n> >  \tif (ret) {\n> > +\t\tfreeBuffers(camera);\n> >  \t\tLOG(RkISP1, Error)\n> >  \t\t\t<< \"Failed to start parameters \" << camera->name();\n> >  \t\treturn ret;\n> > @@ -743,6 +744,7 @@ int PipelineHandlerRkISP1::start(Camera *camera)\n> >  \tret = stat_->streamOn();\n> >  \tif (ret) {\n> >  \t\tparam_->streamOff();\n> > +\t\tfreeBuffers(camera);\n> >  \t\tLOG(RkISP1, Error)\n> >  \t\t\t<< \"Failed to start statistics \" << camera->name();\n> >  \t\treturn ret;\n> > @@ -752,6 +754,7 @@ int PipelineHandlerRkISP1::start(Camera *camera)\n> >  \tif (ret) {\n> >  \t\tparam_->streamOff();\n> >  \t\tstat_->streamOff();\n> > +\t\tfreeBuffers(camera);\n> >  \n> >  \t\tLOG(RkISP1, Error)\n> >  \t\t\t<< \"Failed to start camera \" << camera->name();\n> > @@ -796,6 +799,8 @@ void PipelineHandlerRkISP1::stop(Camera *camera)\n> >  \n> >  \tdata->timeline_.reset();\n> >  \n> > +\tfreeBuffers(camera);\n> > +\n> >  \tactiveCamera_ = nullptr;\n> >  }\n> >  \n> > diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp\n> > index c275b9f0f75429bd..2be7864ef59e932b 100644\n> > --- a/src/libcamera/pipeline/uvcvideo.cpp\n> > +++ b/src/libcamera/pipeline/uvcvideo.cpp\n> > @@ -65,11 +65,6 @@ public:\n> >  \t\tconst StreamRoles &roles) override;\n> >  \tint configure(Camera *camera, CameraConfiguration *config) override;\n> >  \n> > -\tint allocateBuffers(Camera *camera,\n> > -\t\t\t    const std::set<Stream *> &streams) override;\n> > -\tint freeBuffers(Camera *camera,\n> > -\t\t\tconst std::set<Stream *> &streams) override;\n> > -\n> >  \tint start(Camera *camera) override;\n> >  \tvoid stop(Camera *camera) override;\n> >  \n> > @@ -193,18 +188,6 @@ int PipelineHandlerUVC::configure(Camera *camera, CameraConfiguration *config)\n> >  \treturn 0;\n> >  }\n> >  \n> > -int PipelineHandlerUVC::allocateBuffers(Camera *camera,\n> > -\t\t\t\t\tconst std::set<Stream *> &streams)\n> > -{\n> > -\treturn 0;\n> > -}\n> > -\n> > -int PipelineHandlerUVC::freeBuffers(Camera *camera,\n> > -\t\t\t\t    const std::set<Stream *> &streams)\n> > -{\n> > -\treturn 0;\n> > -}\n> > -\n> >  int PipelineHandlerUVC::start(Camera *camera)\n> >  {\n> >  \tUVCCameraData *data = cameraData(camera);\n> > diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp\n> > index 64793788c752c791..cacf8bfe3ff9cc45 100644\n> > --- a/src/libcamera/pipeline/vimc.cpp\n> > +++ b/src/libcamera/pipeline/vimc.cpp\n> > @@ -84,11 +84,6 @@ public:\n> >  \t\tconst StreamRoles &roles) override;\n> >  \tint configure(Camera *camera, CameraConfiguration *config) override;\n> >  \n> > -\tint allocateBuffers(Camera *camera,\n> > -\t\t\t    const std::set<Stream *> &streams) override;\n> > -\tint freeBuffers(Camera *camera,\n> > -\t\t\tconst std::set<Stream *> &streams) override;\n> > -\n> >  \tint start(Camera *camera) override;\n> >  \tvoid stop(Camera *camera) override;\n> >  \n> > @@ -261,18 +256,6 @@ int PipelineHandlerVimc::configure(Camera *camera, CameraConfiguration *config)\n> >  \treturn 0;\n> >  }\n> >  \n> > -int PipelineHandlerVimc::allocateBuffers(Camera *camera,\n> > -\t\t\t\t\t const std::set<Stream *> &streams)\n> > -{\n> > -\treturn 0;\n> > -}\n> > -\n> > -int PipelineHandlerVimc::freeBuffers(Camera *camera,\n> > -\t\t\t\t     const std::set<Stream *> &streams)\n> > -{\n> > -\treturn 0;\n> > -}\n> > -\n> >  int PipelineHandlerVimc::start(Camera *camera)\n> >  {\n> >  \tVimcCameraData *data = cameraData(camera);\n> > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> > index b107e23258dee692..67758f3f1ebc39e5 100644\n> > --- a/src/libcamera/pipeline_handler.cpp\n> > +++ b/src/libcamera/pipeline_handler.cpp\n> > @@ -287,35 +287,6 @@ const ControlInfoMap &PipelineHandler::controls(Camera *camera)\n> >   * \\return 0 on success or a negative error code otherwise\n> >   */\n> >  \n> > -/**\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] streams The set of streams to allocate buffers for\n> > - *\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> > - * \\return 0 on success or a negative error code otherwise\n> > - */\n> > -\n> > -/**\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] streams The set of streams to free buffers from\n> > - *\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.\n> > - *\n> > - * \\return 0 on success or a negative error code otherwise\n> > - */\n> > -\n> >  /**\n> >   * \\fn PipelineHandler::start()\n> >   * \\brief Start capturing from a group of streams","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 C960060100\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tSun, 15 Dec 2019 15:01:13 +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 32AD6DD;\n\tSun, 15 Dec 2019 15:01:13 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1576418473;\n\tbh=G3IhUbC4KMUelNvFWNYAKa632KcaEJTnsXTQiTh46Ck=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=mFiAnOyjEzrdTC/39Qo1AcN9XY4vAr59L9tTNktbIDWfQFl/i4p9K0fOQgibg4f4d\n\tUmqiF1T/sUd0VQDT3IjPy6sLEhCkj43fwjjJLlAzZjoa0wtCUfQ6cS1216zJ9l+2bk\n\tAixJMzWQG56HUhmMCaOmOMqyUyK//5rEzybK2sRU=","Date":"Sun, 15 Dec 2019 16:01:03 +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":"<20191215140103.GA14495@pendragon.ideasonboard.com>","References":"<20191126233620.1695316-1-niklas.soderlund@ragnatech.se>\n\t<20191126233620.1695316-30-niklas.soderlund@ragnatech.se>\n\t<20191215092842.GB4889@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20191215092842.GB4889@pendragon.ideasonboard.com>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 29/30] libcamera: pipeline: Remove\n\texplicit buffer handling","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, 15 Dec 2019 14:01:14 -0000"}}]