[{"id":1148,"web_url":"https://patchwork.libcamera.org/comment/1148/","msgid":"<20190327094138.GB23466@bigcity.dyn.berto.se>","date":"2019-03-27T09:41:38","subject":"Re: [libcamera-devel] [PATCH] libcamera: camera: allocateBuffers:\n\tPass the stream set","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Jacopo,\n\nThanks for your patch.\n\nOn 2019-03-26 17:50:11 +0100, 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> 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> stream, and ask them to loop over them to perform per-streams\n> allocations after eventual stream-independent operations.\n> \n> The same rational applies to PipelineHandler::freeBuffers() and eventual\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> \n> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>\n> ---\n> As noted with the development of multi stream support for the IPU3 platform,\n> pipeline handlers might need room to perform pre and post stream buffer memory\n> allocation operations.\n> \n> I initally implemented pre and post hooks, but as Laurent suggested, changing\n> the interface between Camera and Pipeline Handlers is a much nicer solution.\n> ---\n>  src/libcamera/camera.cpp                 | 15 +++++++--------\n>  src/libcamera/include/pipeline_handler.h |  6 ++++--\n>  src/libcamera/pipeline/ipu3/ipu3.cpp     | 19 ++++++++++---------\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, 45 insertions(+), 33 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> +\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 55489c31df2d..c13252b6f77e 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -36,8 +36,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> @@ -188,9 +190,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>  \tconst StreamConfiguration &cfg = stream->configuration();\n> \n>  \tif (!cfg.bufferCount)\n> @@ -205,17 +209,14 @@ 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>  \tint ret = data->cio2_->releaseBuffers();\n> -\tif (ret) {\n> +\tif (ret)\n>  \t\tLOG(IPU3, Error) << \"Failed to release memory\";\n> -\t\treturn ret;\n> -\t}\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..d14d463cf610 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 active streams to allocate buffers for\n\nI would s/active// here and bellow, the pipeline handler cares not that \nthe camera calls this interface using the active streams. The parameter \nis an array of streams to allocate/free buffers from.\n\nWith this fixed,\n\nReviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>\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>   * 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 active 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.\n> --\n> 2.21.0\n> \n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"<niklas.soderlund@ragnatech.se>","Received":["from mail-lf1-x129.google.com (mail-lf1-x129.google.com\n\t[IPv6:2a00:1450:4864:20::129])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 718BA610B3\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 27 Mar 2019 10:41:40 +0100 (CET)","by mail-lf1-x129.google.com with SMTP id 10so10839953lfr.8\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 27 Mar 2019 02:41:40 -0700 (PDT)","from localhost (89-233-230-99.cust.bredband2.com. [89.233.230.99])\n\tby smtp.gmail.com with ESMTPSA id\n\tt29sm2072440ljd.82.2019.03.27.02.41.38\n\t(version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256);\n\tWed, 27 Mar 2019 02:41:38 -0700 (PDT)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to\n\t:user-agent; bh=gsg5PvdI4MT/Y6pk+8Ea+mlKrHuFancODV3BgqJ4dqU=;\n\tb=OGKeE+QzelSIv8PHxTJkMLBKjFgNdOQQAvTe/JezFG9faisINNppnQ4sqWp9APcdWk\n\tMr3ZKVaVfAQ6SunBSSHGjQ3xBv/Er68DH5GIqXiIK4lzuGPdGVxzCuhqdVSSexO7GIWj\n\ti7O1s1VC1sjg30DmVIgP+elpTfO+Fpv8a2OfynogYOdYYcx9rIQgiCWR3bbFyKNwYT5l\n\tMh1ebsYwc9dwOBxYbMKrLfmi1jTTz54Ukg7SPrEp59VDT7yTyliRy0VlelN1CEaApszH\n\t3pdJzPndQV0NRAqPzO3sJ0qNSP3i1xsHMcf+6XPzRpKbLKJQwbZ1xQxP7QFuBAfjKJV5\n\tUHZQ==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to:user-agent;\n\tbh=gsg5PvdI4MT/Y6pk+8Ea+mlKrHuFancODV3BgqJ4dqU=;\n\tb=gisIfmfQ0LYRd1KfPG00p255Pq5PqCD/tlzckYzCi/r1x23rq1YskECgdQVdUM762s\n\tkCYo82b8IQ9wi69M/4hJIQbS+P7IQuGDDjIa7LIjqHs96p7h5Fe/buVCR0yCOw57JSWU\n\tZkYRR4G9gRQ22OOLtsnlMkUQeeunBwl11L7EH7vyZexrWfPE0ZHbtfrWMDL9PNeoZLSL\n\tusFN++0PPnPVABPuxNa2VWBCb5QmgcJM/KwCU2G8OUaP9EGGGEJNeSTKHjsDAUXshwIf\n\t1RYrwDbjTdn5sxJEvi55/WhZxvDINIIWb9qjTaR8tvRR/Cb19al2IScqk6NXa5aNO7Nk\n\tJQVg==","X-Gm-Message-State":"APjAAAUSe0dcWQwJ2JEGahquVyusJuQBGyoJNQFv2u7eMr5h5h/85BXk\n\tLCg2w6KnN6/akGJ3zUhxbtrxvw==","X-Google-Smtp-Source":"APXvYqwvLlh7zWOPUeZsYWP5UT04OktursowVN2aozLRGVUbDX+XzMOTaSKmv81h3SsK0G7IJOHlfA==","X-Received":"by 2002:a19:d144:: with SMTP id\n\ti65mr16327270lfg.52.1553679699497; \n\tWed, 27 Mar 2019 02:41:39 -0700 (PDT)","Date":"Wed, 27 Mar 2019 10:41:38 +0100","From":"Niklas =?iso-8859-1?q?S=F6derlund?= <niklas.soderlund@ragnatech.se>","To":"Jacopo Mondi <jacopo@jmondi.org>","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190327094138.GB23466@bigcity.dyn.berto.se>","References":"<20190326165011.10817-1-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20190326165011.10817-1-jacopo@jmondi.org>","User-Agent":"Mutt/1.11.3 (2019-02-01)","Subject":"Re: [libcamera-devel] [PATCH] libcamera: camera: allocateBuffers:\n\tPass 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":"Wed, 27 Mar 2019 09:41:40 -0000"}}]