[{"id":1285,"web_url":"https://patchwork.libcamera.org/comment/1285/","msgid":"<20190405154456.GB4636@pendragon.ideasonboard.com>","date":"2019-04-05T15:44:56","subject":"Re: [libcamera-devel] [PATCH v3 3/8] libcamera: camera:\n\tallocateBuffers: Pass the stream set","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 Wed, Apr 03, 2019 at 05:07:30PM +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 per each\n\ns/per/for/ or s/per each/per/\n\n> active stream, leaving no space for stream-independent configuration.\n> \n> Change this by providing to the pipeline handler the full set of active\n\ns/handler/handlers/\n\n> stream, and ask them to loop over them to perform per-streams\n\ns/stream/streams/\n\n> allocations after eventual stream-independent operations.\n\nAfter or before, it doesn't matter too much, I'd drop that part of the\nsentence.\n\n> The same rational applies to PipelineHandler::freeBuffers() and eventual\n\ns/rational/rationale/\ns/eventual/optional/ (or \"possible\")\n\nSeems that eventual has the same meaning in Italian than in French, but\nit's not the case in English :-)\n\n> stream-independent clean up operations.\n> \n> While at it, change the return type of freeBuffers() to void as it was\n> not checked by the Camera class method calling it.\n\nHow about checking it instead ? I think it's useful to propagate errors\nup. Even if there's not too much an application can do to recover, it\ncould for instance warn the user that an error occurred.\n\n> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>\n> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>\n> ---\n>  src/libcamera/camera.cpp                 | 15 +++++++--------\n>  src/libcamera/include/pipeline_handler.h |  6 ++++--\n>  src/libcamera/pipeline/ipu3/ipu3.cpp     | 15 +++++++++------\n>  src/libcamera/pipeline/uvcvideo.cpp      | 15 ++++++++++-----\n>  src/libcamera/pipeline/vimc.cpp          | 15 ++++++++++-----\n>  src/libcamera/pipeline_handler.cpp       |  8 ++++----\n>  6 files changed, 44 insertions(+), 30 deletions(-)\n> \n> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> index 8ee9cc086616..99eb0e6876f0 100644\n> --- a/src/libcamera/camera.cpp\n> +++ b/src/libcamera/camera.cpp\n> @@ -467,13 +467,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\nDo pipeline handlers expect freeBuffers() to be called if\npipe_->allocatedBuffers() failed ? This should at least be documented.\n\n> +\t\treturn ret;\n>  \t}\n>  \n>  \tstate_ = CameraPrepared;\n> @@ -503,9 +501,10 @@ 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> +\tpipe_->freeBuffers(this, activeStreams_);\n> +\n>  \tstate_ = CameraConfigured;\n>  \n>  \treturn 0;\n> diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h\n> index acb376e07030..920b57609470 100644\n> --- a/src/libcamera/include/pipeline_handler.h\n> +++ b/src/libcamera/include/pipeline_handler.h\n> @@ -57,8 +57,10 @@ public:\n>  \tvirtual int configureStreams(Camera *camera,\n>  \t\t\t\t     std::map<Stream *, StreamConfiguration> &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<Stream *> &streams) = 0;\n> +\tvirtual void freeBuffers(Camera *camera,\n> +\t\t\t\t const std::set<Stream *> &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 caf1051c58ab..a12e3f9a496d 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     std::map<Stream *, StreamConfiguration> &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<Stream *> &streams) override;\n> +\tvoid freeBuffers(Camera *camera,\n> +\t\t\t const std::set<Stream *> &streams) override;\n>  \n>  \tint start(Camera *camera) override;\n>  \tvoid stop(Camera *camera) override;\n> @@ -426,9 +428,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<Stream *> &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> @@ -467,14 +471,13 @@ int PipelineHandlerIPU3::allocateBuffers(Camera *camera, Stream *stream)\n>  \treturn 0;\n>  }\n>  \n> -int PipelineHandlerIPU3::freeBuffers(Camera *camera, Stream *stream)\n> +void PipelineHandlerIPU3::freeBuffers(Camera *camera,\n> +\t\t\t\t      const std::set<Stream *> &streams)\n>  {\n>  \tIPU3CameraData *data = cameraData(camera);\n>  \n>  \tdata->cio2_.freeBuffers();\n>  \tdata->imgu_->freeBuffers();\n> -\n> -\treturn 0;\n>  }\n>  \n>  int PipelineHandlerIPU3::start(Camera *camera)\n> diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp\n> index cc3e0cd9afab..128f0c49dba3 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     std::map<Stream *, StreamConfiguration> &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<Stream *> &streams) override;\n> +\tvoid freeBuffers(Camera *camera,\n> +\t\t\t const std::set<Stream *> &streams) override;\n>  \n>  \tint start(Camera *camera) override;\n>  \tvoid stop(Camera *camera) override;\n> @@ -129,9 +131,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<Stream *> &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> @@ -139,10 +143,11 @@ 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> +void PipelineHandlerUVC::freeBuffers(Camera *camera,\n> +\t\t\t\t     const std::set<Stream *> &streams)\n>  {\n>  \tUVCCameraData *data = cameraData(camera);\n> -\treturn data->video_->releaseBuffers();\n> +\tdata->video_->releaseBuffers();\n>  }\n>  \n>  int PipelineHandlerUVC::start(Camera *camera)\n> diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp\n> index 2e8c26fb7c0b..6735940799d8 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     std::map<Stream *, StreamConfiguration> &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<Stream *> &streams) override;\n> +\tvoid freeBuffers(Camera *camera,\n> +\t\t\t const std::set<Stream *> &streams) override;\n>  \n>  \tint start(Camera *camera) override;\n>  \tvoid stop(Camera *camera) override;\n> @@ -129,9 +131,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<Stream *> &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> @@ -139,10 +143,11 @@ 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> +void PipelineHandlerVimc::freeBuffers(Camera *camera,\n> +\t\t\t\t      const std::set<Stream *> &streams)\n>  {\n>  \tVimcCameraData *data = cameraData(camera);\n> -\treturn data->video_->releaseBuffers();\n> +\tdata->video_->releaseBuffers();\n>  }\n>  \n>  int PipelineHandlerVimc::start(Camera *camera)\n> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> index 1a858f2638ce..9a8a4fde57e6 100644\n> --- a/src/libcamera/pipeline_handler.cpp\n> +++ b/src/libcamera/pipeline_handler.cpp\n> @@ -193,10 +193,10 @@ 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 group of streams to allocate buffers for\n\ns/group/set/ ?\n\n>   *\n>   * This method allocates buffers internally in the pipeline handler and\n> - * associates them with the stream's buffer pool.\n> + * associates them with each stream's buffer pool.\n\n\"This method allocates buffers internally in the pipeline handler for\neach stream in the \\a streams set, and associates them with the stream's\nbuffer pool.\" ?\n\n>   *\n>   * The intended caller of this method is the Camera class.\n>   *\n> @@ -207,9 +207,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 group of streams to free buffers from\n\ns/group/set/\n\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":"<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 AC50660DB3\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri,  5 Apr 2019 17:45:07 +0200 (CEST)","from pendragon.ideasonboard.com (unknown [109.140.214.47])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id CC1C7E2;\n\tFri,  5 Apr 2019 17:45:06 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1554479107;\n\tbh=bA9ONkHZDGgHWlK8Y69ZF6buGTokaew1wDmGypwZrq8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=JN+2aeaFC2RGh2kS1fYyF/Y3lhNJdzq1nKuuYV1QnJC/eTV7pGsTf0jDat810WhyE\n\t+XNrH3IxA9Y1bW0eH8g2+QEOXfJxyBMYOPNLoOibahIwMiEMQXlpAs6xo2MvG+fO4o\n\twhZMIRXLd3d0IEW+ZUZoLzZZxiPHiW75gsZhbh5I=","Date":"Fri, 5 Apr 2019 18:44:56 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Jacopo Mondi <jacopo@jmondi.org>","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190405154456.GB4636@pendragon.ideasonboard.com>","References":"<20190403150735.27580-1-jacopo@jmondi.org>\n\t<20190403150735.27580-4-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20190403150735.27580-4-jacopo@jmondi.org>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v3 3/8] libcamera: camera:\n\tallocateBuffers: Pass the stream set","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","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":"Fri, 05 Apr 2019 15:45:07 -0000"}}]