[{"id":4031,"web_url":"https://patchwork.libcamera.org/comment/4031/","msgid":"<20200316154459.GG2260535@oden.dyn.berto.se>","date":"2020-03-16T15:44:59","subject":"Re: [libcamera-devel] [PATCH 7/9] libcamera: pipeline_handler:\n\tDecouple buffer import and export","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Laurent,\n\nThanks for your work.\n\nOn 2020-03-15 01:57:26 +0200, Laurent Pinchart wrote:\n> Use the V4L2 buffer orphaning feature, exposed through\n> V4L2VideoDevice::exportBuffers(), to decouple buffer import and export.\n> The PipelineHandler::importFrameBuffers() function is now called for all\n> streams regardless of whether exportFrameBuffers() has been called or\n> not. This simplifies the Camera implementation slightly, and opens the\n> door to additional simplifications.\n> \n> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n\nReviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>\n\n> ---\n>  include/libcamera/camera.h               |  1 -\n>  src/libcamera/camera.cpp                 | 21 +-------------------\n>  src/libcamera/framebuffer_allocator.cpp  |  9 ---------\n>  src/libcamera/pipeline/ipu3/ipu3.cpp     |  2 +-\n>  src/libcamera/pipeline/rkisp1/rkisp1.cpp |  2 +-\n>  src/libcamera/pipeline/uvcvideo.cpp      |  2 +-\n>  src/libcamera/pipeline/vimc.cpp          |  2 +-\n>  src/libcamera/pipeline_handler.cpp       | 25 +++++-------------------\n>  8 files changed, 10 insertions(+), 54 deletions(-)\n> \n> diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h\n> index c453b95228a7..a5e2b49f0f25 100644\n> --- a/include/libcamera/camera.h\n> +++ b/include/libcamera/camera.h\n> @@ -113,7 +113,6 @@ private:\n>  \tfriend class FrameBufferAllocator;\n>  \tint exportFrameBuffers(Stream *stream,\n>  \t\t\t       std::vector<std::unique_ptr<FrameBuffer>> *buffers);\n> -\tint freeFrameBuffers(Stream *stream);\n>  \t/* \\todo Remove allocator_ from the exposed API */\n>  \tFrameBufferAllocator *allocator_;\n>  };\n> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> index 9c432adb1d97..3192dfb42d01 100644\n> --- a/src/libcamera/camera.cpp\n> +++ b/src/libcamera/camera.cpp\n> @@ -554,18 +554,6 @@ int Camera::exportFrameBuffers(Stream *stream,\n>  \t\t\t\t       buffers);\n>  }\n>  \n> -int Camera::freeFrameBuffers(Stream *stream)\n> -{\n> -\tint ret = p_->isAccessAllowed(Private::CameraConfigured, true);\n> -\tif (ret < 0)\n> -\t\treturn ret;\n> -\n> -\tp_->pipe_->invokeMethod(&PipelineHandler::freeFrameBuffers,\n> -\t\t\t\tConnectionTypeBlocking, this, stream);\n> -\n> -\treturn 0;\n> -}\n> -\n>  /**\n>   * \\brief Acquire the camera device for exclusive access\n>   *\n> @@ -928,9 +916,6 @@ int Camera::start()\n>  \tLOG(Camera, Debug) << \"Starting capture\";\n>  \n>  \tfor (Stream *stream : p_->activeStreams_) {\n> -\t\tif (allocator_ && !allocator_->buffers(stream).empty())\n> -\t\t\tcontinue;\n> -\n>  \t\tret = p_->pipe_->invokeMethod(&PipelineHandler::importFrameBuffers,\n>  \t\t\t\t\t      ConnectionTypeDirect, this, stream);\n>  \t\tif (ret < 0)\n> @@ -974,13 +959,9 @@ int Camera::stop()\n>  \tp_->pipe_->invokeMethod(&PipelineHandler::stop, ConnectionTypeBlocking,\n>  \t\t\t\tthis);\n>  \n> -\tfor (Stream *stream : p_->activeStreams_) {\n> -\t\tif (allocator_ && !allocator_->buffers(stream).empty())\n> -\t\t\tcontinue;\n> -\n> +\tfor (Stream *stream : p_->activeStreams_)\n>  \t\tp_->pipe_->invokeMethod(&PipelineHandler::freeFrameBuffers,\n>  \t\t\t\t\tConnectionTypeBlocking, this, stream);\n> -\t}\n>  \n>  \treturn 0;\n>  }\n> diff --git a/src/libcamera/framebuffer_allocator.cpp b/src/libcamera/framebuffer_allocator.cpp\n> index e79f4a8f65c8..6f7a2e90b08a 100644\n> --- a/src/libcamera/framebuffer_allocator.cpp\n> +++ b/src/libcamera/framebuffer_allocator.cpp\n> @@ -87,11 +87,6 @@ FrameBufferAllocator::FrameBufferAllocator(std::shared_ptr<Camera> camera)\n>  \n>  FrameBufferAllocator::~FrameBufferAllocator()\n>  {\n> -\tfor (auto &value : buffers_) {\n> -\t\tStream *stream = value.first;\n> -\t\tcamera_->freeFrameBuffers(stream);\n> -\t}\n> -\n>  \tbuffers_.clear();\n>  \n>  \tcamera_->allocator_ = nullptr;\n> @@ -148,10 +143,6 @@ int FrameBufferAllocator::free(Stream *stream)\n>  \tif (iter == buffers_.end())\n>  \t\treturn -EINVAL;\n>  \n> -\tint ret = camera_->freeFrameBuffers(stream);\n> -\tif (ret < 0)\n> -\t\treturn ret;\n> -\n>  \tstd::vector<std::unique_ptr<FrameBuffer>> &buffers = iter->second;\n>  \tbuffers.clear();\n>  \tbuffers_.erase(iter);\n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index 10a2698bad09..b6db8d567ea4 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -622,7 +622,7 @@ int PipelineHandlerIPU3::exportFrameBuffers(Camera *camera, Stream *stream,\n>  \tV4L2VideoDevice *video = ipu3stream->device_->dev;\n>  \tunsigned int count = stream->configuration().bufferCount;\n>  \n> -\treturn video->allocateBuffers(count, buffers);\n> +\treturn video->exportBuffers(count, buffers);\n>  }\n>  \n>  int PipelineHandlerIPU3::importFrameBuffers(Camera *camera, Stream *stream)\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index f6934324c5a3..1ad53fbde112 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -665,7 +665,7 @@ int PipelineHandlerRkISP1::exportFrameBuffers(Camera *camera, Stream *stream,\n>  \t\t\t\t\t      std::vector<std::unique_ptr<FrameBuffer>> *buffers)\n>  {\n>  \tunsigned int count = stream->configuration().bufferCount;\n> -\treturn video_->allocateBuffers(count, buffers);\n> +\treturn video_->exportBuffers(count, buffers);\n>  }\n>  \n>  int PipelineHandlerRkISP1::importFrameBuffers(Camera *camera, Stream *stream)\n> diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp\n> index d81627c224ea..29afb121aa46 100644\n> --- a/src/libcamera/pipeline/uvcvideo.cpp\n> +++ b/src/libcamera/pipeline/uvcvideo.cpp\n> @@ -199,7 +199,7 @@ int PipelineHandlerUVC::exportFrameBuffers(Camera *camera, Stream *stream,\n>  \tUVCCameraData *data = cameraData(camera);\n>  \tunsigned int count = stream->configuration().bufferCount;\n>  \n> -\treturn data->video_->allocateBuffers(count, buffers);\n> +\treturn data->video_->exportBuffers(count, buffers);\n>  }\n>  \n>  int PipelineHandlerUVC::importFrameBuffers(Camera *camera, Stream *stream)\n> diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp\n> index bb94ef7fd38d..5d3d12fef30b 100644\n> --- a/src/libcamera/pipeline/vimc.cpp\n> +++ b/src/libcamera/pipeline/vimc.cpp\n> @@ -265,7 +265,7 @@ int PipelineHandlerVimc::exportFrameBuffers(Camera *camera, Stream *stream,\n>  \tVimcCameraData *data = cameraData(camera);\n>  \tunsigned int count = stream->configuration().bufferCount;\n>  \n> -\treturn data->video_->allocateBuffers(count, buffers);\n> +\treturn data->video_->exportBuffers(count, buffers);\n>  }\n>  \n>  int PipelineHandlerVimc::importFrameBuffers(Camera *camera, Stream *stream)\n> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> index 8d623f5431fa..e5034c54e2fb 100644\n> --- a/src/libcamera/pipeline_handler.cpp\n> +++ b/src/libcamera/pipeline_handler.cpp\n> @@ -337,16 +337,7 @@ const ControlList &PipelineHandler::properties(Camera *camera)\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> - * exportFrameBuffers() 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> - * exportFrameBuffers() and importFrameBuffers() for the streams contained in\n> - * any camera configuration.\n> + * streams that are part of the active camera configuration.\n>   *\n>   * The only intended caller is Camera::exportFrameBuffers().\n>   *\n> @@ -371,12 +362,7 @@ const ControlList &PipelineHandler::properties(Camera *camera)\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 exportFrameBuffers(), 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> - * exportFrameBuffers() and importFrameBuffers() for the streams contained in\n> - * any camera configuration.\n> + * pipeline handler for the stream to prepare for starting the Camera.\n>   *\n>   * The only intended caller is Camera::start().\n>   *\n> @@ -391,10 +377,9 @@ const ControlList &PipelineHandler::properties(Camera *camera)\n>   * \\param[in] camera The camera\n>   * \\param[in] stream The stream to free buffers for\n>   *\n> - * This method shall free all buffers and all other resources allocated for the\n> - * \\a stream by exportFrameBuffers() 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> + * This method shall release all resources allocated for the \\a stream by\n> + * importFrameBuffers(). It shall be called only after a successful call that\n> + * method, and only once per stream.\n>   *\n>   * The only intended callers are Camera::stop() and Camera::freeFrameBuffers().\n>   *\n> -- \n> Regards,\n> \n> Laurent Pinchart\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-lj1-x241.google.com (mail-lj1-x241.google.com\n\t[IPv6:2a00:1450:4864:20::241])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B09F36041A\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 16 Mar 2020 16:45:01 +0100 (CET)","by mail-lj1-x241.google.com with SMTP id s13so19224724ljm.1\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 16 Mar 2020 08:45:01 -0700 (PDT)","from localhost (h-200-138.A463.priv.bahnhof.se. [176.10.200.138])\n\tby smtp.gmail.com with ESMTPSA id\n\ti18sm201650lfe.15.2020.03.16.08.45.00\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tMon, 16 Mar 2020 08:45:00 -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\tbh=CBjjLovs9LBYqIZ3eaoYPnfON1LF9cevMmVs74xPqtA=;\n\tb=HYzi2QCQoF4+ZRNHhaBdq4itz+r77iXNMci4cqUegJfddulWT6kRSweYX/LHybN8/6\n\tJ2FK6vkjXtMu2ZBfFV0Tgvzsu7GqUyviM0ILggfKvljK6+BoW8gSdx81I7KfPzitpxJt\n\tVsXa2D59/I1rX2x8laJyWvOXJKv8Kc6qjeqc0TDnwiOOWoesSSUPBqRaeXa6ab2hcaoE\n\tWYJvnDeTsv6W1AJ0rmhO1yukuzf5RgP7HHW9b/W6OV8I1QyEdNKSbX5KlqiBHkdzfnf7\n\tTeiInvbGm2JrDUEW1DxWFuJ04LRviIV7xfuqppodCrO8KjvVbYE8mvfDDhEwUQoLj+pg\n\tp0dg==","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;\n\tbh=CBjjLovs9LBYqIZ3eaoYPnfON1LF9cevMmVs74xPqtA=;\n\tb=Dqf5Ec5DWEklpiGT81quUnakPygf1MTxVuzJjCK4qqgNzoUccdqrONMGcON0HSCkFf\n\tYrd7lAw3WsholfmePPN5Y2ZsUkamcGjePJUK+CcHBBoPxAg4NuzRnLaKFHKd61O5DAWz\n\tvktz2doe7P/mNe0K53wvE7ETdr6UTv9hzdEshWkFjLxXWBfrNJ7g+pRSGHloOa6awzb0\n\tajJuznPT2RqNOImlICtPJG9Ib7Qlwr1yoLoYuDlRQuD2XBZXr8Swapl9gNqbSzgOX3mv\n\tr1chRBXAG4gEN5uj3s/m1gBxCl4wqtIqtzm7VDiYZqIV+UhdkjDvwoP/isai5PDfcE6M\n\tnzfA==","X-Gm-Message-State":"ANhLgQ3XJ8+xOct2QckhDqbIFqzIGk7xMiLh801M/MDq5tFAP5nWJuE2\n\tusoqE8fDfXQ9XAnGzzqRJWh/SdrSivI=","X-Google-Smtp-Source":"ADFU+vuyIHDKMrwwtGtMNP+t52oOCri9yOiGR0HjYpw90i5xLGURkRpEGTJ99I7yZHK1DTkWywwM0Q==","X-Received":"by 2002:a05:651c:1139:: with SMTP id\n\te25mr16755959ljo.261.1584373500912; \n\tMon, 16 Mar 2020 08:45:00 -0700 (PDT)","Date":"Mon, 16 Mar 2020 16:44:59 +0100","From":"Niklas =?iso-8859-1?q?S=F6derlund?= <niklas.soderlund@ragnatech.se>","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200316154459.GG2260535@oden.dyn.berto.se>","References":"<20200314235728.15495-1-laurent.pinchart@ideasonboard.com>\n\t<20200314235728.15495-8-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20200314235728.15495-8-laurent.pinchart@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 7/9] libcamera: pipeline_handler:\n\tDecouple buffer import and export","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":"Mon, 16 Mar 2020 15:45:01 -0000"}}]