[{"id":18560,"web_url":"https://patchwork.libcamera.org/comment/18560/","msgid":"<20210805040216.GM2167@pyrite.rasen.tech>","date":"2021-08-05T04:02:16","subject":"Re: [libcamera-devel] [PATCH v3 6/8] cam: Add KMS sink class","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Hi Laurent,\n\nOn Wed, Aug 04, 2021 at 03:43:12PM +0300, Laurent Pinchart wrote:\n> Add a KMSSink class to display framebuffers through the DRM/KMS API.\n> \n> Signed-off-by: Laurent Pinchart \n\nLooks good.\n\nReviewed-by: Paul Elder \n\n> ---\n> Changes since v2:\n> \n> - Simplify connector selection logic, and fix it when a connector name\n> is specified\n> - Fail configure() if no connector is selected\n> - Update copyright years\n> \n> Changes since v1:\n> \n> - Use formats:: namespace\n> \n> fixup! cam: Add KMS sink class\n> ---\n> src/cam/kms_sink.cpp | 306 +++++++++++++++++++++++++++++++++++++++++++\n> src/cam/kms_sink.h | 76 +++++++++++\n> src/cam/meson.build | 1 +\n> 3 files changed, 383 insertions(+)\n> create mode 100644 src/cam/kms_sink.cpp\n> create mode 100644 src/cam/kms_sink.h\n> \n> diff --git a/src/cam/kms_sink.cpp b/src/cam/kms_sink.cpp\n> new file mode 100644\n> index 000000000000..f708b613f226\n> --- /dev/null\n> +++ b/src/cam/kms_sink.cpp\n> @@ -0,0 +1,306 @@\n> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> +/*\n> + * Copyright (C) 2021, Ideas on Board Oy\n> + *\n> + * kms_sink.cpp - KMS Sink\n> + */\n> +\n> +#include \"kms_sink.h\"\n> +\n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +\n> +#include \n> +#include \n> +#include \n> +#include \n> +\n> +#include \"drm.h\"\n> +\n> +KMSSink::KMSSink(const std::string &connectorName)\n> +\t: connector_(nullptr), crtc_(nullptr), plane_(nullptr), mode_(nullptr)\n> +{\n> +\tint ret = dev_.init();\n> +\tif (ret < 0)\n> +\t\treturn;\n> +\n> +\t/*\n> +\t * Find the requested connector. If no specific connector is requested,\n> +\t * pick the first connected connector or, if no connector is connected,\n> +\t * the first connector with unknown status.\n> +\t */\n> +\tfor (const DRM::Connector &conn : dev_.connectors()) {\n> +\t\tif (!connectorName.empty()) {\n> +\t\t\tif (conn.name() != connectorName)\n> +\t\t\t\tcontinue;\n> +\n> +\t\t\tconnector_ = &conn;\n> +\t\t\tbreak;\n> +\t\t}\n> +\n> +\t\tif (conn.status() == DRM::Connector::Connected) {\n> +\t\t\tconnector_ = &conn;\n> +\t\t\tbreak;\n> +\t\t}\n> +\n> +\t\tif (!connector_ && conn.status() == DRM::Connector::Unknown)\n> +\t\t\tconnector_ = &conn;\n> +\t}\n> +\n> +\tif (!connector_) {\n> +\t\tif (!connectorName.empty())\n> +\t\t\tstd::cerr\n> +\t\t\t\t<< \"Connector \" << connectorName << \" not found\"\n> +\t\t\t\t<< std::endl;\n> +\t\telse\n> +\t\t\tstd::cerr << \"No connected connector found\" << std::endl;\n> +\t\treturn;\n> +\t}\n> +\n> +\tdev_.requestComplete.connect(this, &KMSSink::requestComplete);\n> +}\n> +\n> +void KMSSink::mapBuffer(libcamera::FrameBuffer *buffer)\n> +{\n> +\tstd::unique_ptr drmBuffer =\n> +\t\tdev_.createFrameBuffer(*buffer, format_, size_, stride_);\n> +\tif (!drmBuffer)\n> +\t\treturn;\n> +\n> +\tbuffers_.emplace(std::piecewise_construct,\n> +\t\t\t std::forward_as_tuple(buffer),\n> +\t\t\t std::forward_as_tuple(std::move(drmBuffer)));\n> +}\n> +\n> +int KMSSink::configure(const libcamera::CameraConfiguration &config)\n> +{\n> +\tif (!connector_)\n> +\t\treturn -EINVAL;\n> +\n> +\tcrtc_ = nullptr;\n> +\tplane_ = nullptr;\n> +\tmode_ = nullptr;\n> +\n> +\tconst libcamera::StreamConfiguration &cfg = config.at(0);\n> +\tint ret = configurePipeline(cfg.pixelFormat);\n> +\tif (ret < 0)\n> +\t\treturn ret;\n> +\n> +\tconst std::vector &modes = connector_->modes();\n> +\tconst auto iter = std::find_if(modes.begin(), modes.end(),\n> +\t\t\t\t [&](const DRM::Mode &mode) {\n> +\t\t\t\t\t return mode.hdisplay == cfg.size.width &&\n> +\t\t\t\t\t\t mode.vdisplay == cfg.size.height;\n> +\t\t\t\t });\n> +\tif (iter == modes.end()) {\n> +\t\tstd::cerr\n> +\t\t\t<< \"No mode matching \" << cfg.size.toString()\n> +\t\t\t<< std::endl;\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\tmode_ = &*iter;\n> +\tsize_ = cfg.size;\n> +\tstride_ = cfg.stride;\n> +\n> +\treturn 0;\n> +}\n> +\n> +int KMSSink::configurePipeline(const libcamera::PixelFormat &format)\n> +{\n> +\t/*\n> +\t * If the requested format has an alpha channel, also consider the X\n> +\t * variant.\n> +\t */\n> +\tlibcamera::PixelFormat xFormat;\n> +\n> +\tswitch (format) {\n> +\tcase libcamera::formats::ABGR8888:\n> +\t\txFormat = libcamera::formats::XBGR8888;\n> +\t\tbreak;\n> +\tcase libcamera::formats::ARGB8888:\n> +\t\txFormat = libcamera::formats::XRGB8888;\n> +\t\tbreak;\n> +\tcase libcamera::formats::BGRA8888:\n> +\t\txFormat = libcamera::formats::BGRX8888;\n> +\t\tbreak;\n> +\tcase libcamera::formats::RGBA8888:\n> +\t\txFormat = libcamera::formats::RGBX8888;\n> +\t\tbreak;\n> +\t}\n> +\n> +\t/*\n> +\t * Find a CRTC and plane suitable for the request format and the\n> +\t * connector at the end of the pipeline. Restrict the search to primary\n> +\t * planes for now.\n> +\t */\n> +\tfor (const DRM::Encoder *encoder : connector_->encoders()) {\n> +\t\tfor (const DRM::Crtc *crtc : encoder->possibleCrtcs()) {\n> +\t\t\tfor (const DRM::Plane *plane : crtc->planes()) {\n> +\t\t\t\tif (plane->type() != DRM::Plane::TypePrimary)\n> +\t\t\t\t\tcontinue;\n> +\n> +\t\t\t\tif (plane->supportsFormat(format)) {\n> +\t\t\t\t\tcrtc_ = crtc;\n> +\t\t\t\t\tplane_ = plane;\n> +\t\t\t\t\tformat_ = format;\n> +\t\t\t\t\treturn 0;\n> +\t\t\t\t}\n> +\n> +\t\t\t\tif (plane->supportsFormat(xFormat)) {\n> +\t\t\t\t\tcrtc_ = crtc;\n> +\t\t\t\t\tplane_ = plane;\n> +\t\t\t\t\tformat_ = xFormat;\n> +\t\t\t\t\treturn 0;\n> +\t\t\t\t}\n> +\t\t\t}\n> +\t\t}\n> +\t}\n> +\n> +\tstd::cerr\n> +\t\t<< \"Unable to find display pipeline for format \"\n> +\t\t<< format.toString() << std::endl;\n> +\treturn -EPIPE;\n> +}\n> +\n> +int KMSSink::start()\n> +{\n> +\tstd::unique_ptr request;\n> +\n> +\tint ret = FrameSink::start();\n> +\tif (ret < 0)\n> +\t\treturn ret;\n> +\n> +\t/* Disable all CRTCs and planes to start from a known valid state. */\n> +\trequest = std::make_unique(&dev_);\n> +\n> +\tfor (const DRM::Crtc &crtc : dev_.crtcs())\n> +\t\trequest->addProperty(&crtc, \"ACTIVE\", 0);\n> +\n> +\tfor (const DRM::Plane &plane : dev_.planes()) {\n> +\t\trequest->addProperty(&plane, \"CRTC_ID\", 0);\n> +\t\trequest->addProperty(&plane, \"FB_ID\", 0);\n> +\t}\n> +\n> +\tret = request->commit(DRM::AtomicRequest::FlagAllowModeset);\n> +\tif (ret < 0) {\n> +\t\tstd::cerr\n> +\t\t\t<< \"Failed to disable CRTCs and planes: \"\n> +\t\t\t<< strerror(-ret) << std::endl;\n> +\t\treturn ret;\n> +\t}\n> +\n> +\t/* Enable the display pipeline with no plane to start with. */\n> +\trequest = std::make_unique(&dev_);\n> +\n> +\trequest->addProperty(connector_, \"CRTC_ID\", crtc_->id());\n> +\trequest->addProperty(crtc_, \"ACTIVE\", 1);\n> +\trequest->addProperty(crtc_, \"MODE_ID\", mode_->toBlob(&dev_));\n> +\n> +\tret = request->commit(DRM::AtomicRequest::FlagAllowModeset);\n> +\tif (ret < 0) {\n> +\t\tstd::cerr\n> +\t\t\t<< \"Failed to enable display pipeline: \"\n> +\t\t\t<< strerror(-ret) << std::endl;\n> +\t\treturn ret;\n> +\t}\n> +\n> +\tplaneInitialized_ = false;\n> +\n> +\treturn 0;\n> +}\n> +\n> +int KMSSink::stop()\n> +{\n> +\t/* Display pipeline. */\n> +\tDRM::AtomicRequest request(&dev_);\n> +\n> +\trequest.addProperty(connector_, \"CRTC_ID\", 0);\n> +\trequest.addProperty(crtc_, \"ACTIVE\", 0);\n> +\trequest.addProperty(crtc_, \"MODE_ID\", 0);\n> +\trequest.addProperty(plane_, \"CRTC_ID\", 0);\n> +\trequest.addProperty(plane_, \"FB_ID\", 0);\n> +\n> +\tint ret = request.commit(DRM::AtomicRequest::FlagAllowModeset);\n> +\tif (ret < 0) {\n> +\t\tstd::cerr\n> +\t\t\t<< \"Failed to stop display pipeline: \"\n> +\t\t\t<< strerror(-ret) << std::endl;\n> +\t\treturn ret;\n> +\t}\n> +\n> +\t/* Free all buffers. */\n> +\tpending_.reset();\n> +\tqueued_.reset();\n> +\tactive_.reset();\n> +\tbuffers_.clear();\n> +\n> +\treturn FrameSink::stop();\n> +}\n> +\n> +bool KMSSink::processRequest(libcamera::Request *camRequest)\n> +{\n> +\tif (pending_)\n> +\t\treturn true;\n> +\n> +\tlibcamera::FrameBuffer *buffer = camRequest->buffers().begin()->second;\n> +\tauto iter = buffers_.find(buffer);\n> +\tif (iter == buffers_.end())\n> +\t\treturn true;\n> +\n> +\tDRM::FrameBuffer *drmBuffer = iter->second.get();\n> +\n> +\tDRM::AtomicRequest *drmRequest = new DRM::AtomicRequest(&dev_);\n> +\tdrmRequest->addProperty(plane_, \"FB_ID\", drmBuffer->id());\n> +\n> +\tif (!planeInitialized_) {\n> +\t\tdrmRequest->addProperty(plane_, \"CRTC_ID\", crtc_->id());\n> +\t\tdrmRequest->addProperty(plane_, \"SRC_X\", 0 << 16);\n> +\t\tdrmRequest->addProperty(plane_, \"SRC_Y\", 0 << 16);\n> +\t\tdrmRequest->addProperty(plane_, \"SRC_W\", mode_->hdisplay << 16);\n> +\t\tdrmRequest->addProperty(plane_, \"SRC_H\", mode_->vdisplay << 16);\n> +\t\tdrmRequest->addProperty(plane_, \"CRTC_X\", 0);\n> +\t\tdrmRequest->addProperty(plane_, \"CRTC_Y\", 0);\n> +\t\tdrmRequest->addProperty(plane_, \"CRTC_W\", mode_->hdisplay);\n> +\t\tdrmRequest->addProperty(plane_, \"CRTC_H\", mode_->vdisplay);\n> +\t\tplaneInitialized_ = true;\n> +\t}\n> +\n> +\tpending_ = std::make_unique(drmRequest, camRequest);\n> +\n> +\tstd::lock_guard lock(lock_);\n> +\n> +\tif (!queued_) {\n> +\t\tint ret = drmRequest->commit(DRM::AtomicRequest::FlagAsync);\n> +\t\tif (ret < 0)\n> +\t\t\tstd::cerr\n> +\t\t\t\t<< \"Failed to commit atomic request: \"\n> +\t\t\t\t<< strerror(-ret) << std::endl;\n> +\t\tqueued_ = std::move(pending_);\n> +\t}\n> +\n> +\treturn false;\n> +}\n> +\n> +void KMSSink::requestComplete(DRM::AtomicRequest *request)\n> +{\n> +\tstd::lock_guard lock(lock_);\n> +\n> +\tassert(queued_ && queued_->drmRequest_.get() == request);\n> +\n> +\t/* Complete the active request, if any. */\n> +\tif (active_)\n> +\t\trequestProcessed.emit(active_->camRequest_);\n> +\n> +\t/* The queued request becomes active. */\n> +\tactive_ = std::move(queued_);\n> +\n> +\t/* Queue the pending request, if any. */\n> +\tif (pending_) {\n> +\t\tpending_->drmRequest_->commit(DRM::AtomicRequest::FlagAsync);\n> +\t\tqueued_ = std::move(pending_);\n> +\t}\n> +}\n> diff --git a/src/cam/kms_sink.h b/src/cam/kms_sink.h\n> new file mode 100644\n> index 000000000000..2895e00f84a1\n> --- /dev/null\n> +++ b/src/cam/kms_sink.h\n> @@ -0,0 +1,76 @@\n> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> +/*\n> + * Copyright (C) 2021, Ideas on Board Oy\n> + *\n> + * kms_sink.h - KMS Sink\n> + */\n> +#ifndef __CAM_KMS_SINK_H__\n> +#define __CAM_KMS_SINK_H__\n> +\n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +\n> +#include \n> +\n> +#include \n> +#include \n> +\n> +#include \"drm.h\"\n> +#include \"frame_sink.h\"\n> +\n> +class KMSSink : public FrameSink\n> +{\n> +public:\n> +\tKMSSink(const std::string &connectorName);\n> +\n> +\tbool isValid() const { return connector_ != nullptr; }\n> +\n> +\tvoid mapBuffer(libcamera::FrameBuffer *buffer) override;\n> +\n> +\tint configure(const libcamera::CameraConfiguration &config) override;\n> +\tint start() override;\n> +\tint stop() override;\n> +\n> +\tbool processRequest(libcamera::Request *request) override;\n> +\n> +private:\n> +\tclass Request\n> +\t{\n> +\tpublic:\n> +\t\tRequest(DRM::AtomicRequest *drmRequest, libcamera::Request *camRequest)\n> +\t\t\t: drmRequest_(drmRequest), camRequest_(camRequest)\n> +\t\t{\n> +\t\t}\n> +\n> +\t\tstd::unique_ptr drmRequest_;\n> +\t\tlibcamera::Request *camRequest_;\n> +\t};\n> +\n> +\tint configurePipeline(const libcamera::PixelFormat &format);\n> +\tvoid requestComplete(DRM::AtomicRequest *request);\n> +\n> +\tDRM::Device dev_;\n> +\n> +\tconst DRM::Connector *connector_;\n> +\tconst DRM::Crtc *crtc_;\n> +\tconst DRM::Plane *plane_;\n> +\tconst DRM::Mode *mode_;\n> +\n> +\tlibcamera::PixelFormat format_;\n> +\tlibcamera::Size size_;\n> +\tunsigned int stride_;\n> +\n> +\tbool planeInitialized_;\n> +\n> +\tstd::map> buffers_;\n> +\n> +\tstd::mutex lock_;\n> +\tstd::unique_ptr pending_;\n> +\tstd::unique_ptr queued_;\n> +\tstd::unique_ptr active_;\n> +};\n> +\n> +#endif /* __CAM_KMS_SINK_H__ */\n> diff --git a/src/cam/meson.build b/src/cam/meson.build\n> index b47add55b0cb..ea36aaa5c514 100644\n> --- a/src/cam/meson.build\n> +++ b/src/cam/meson.build\n> @@ -27,6 +27,7 @@ if libdrm.found()\n> cam_cpp_args += [ '-DHAVE_KMS' ]\n> cam_sources += files([\n> 'drm.cpp',\n> + 'kms_sink.cpp'\n> ])\n> endif\n> \n> -- \n> Regards,\n> \n> Laurent Pinchart\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 5CE13C3232\n\tfor ;\n\tThu, 5 Aug 2021 04:02:28 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9E82B68811;\n\tThu, 5 Aug 2021 06:02:26 +0200 (CEST)","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 C1AAB60267\n\tfor ;\n\tThu, 5 Aug 2021 06:02:24 +0200 (CEST)","from pyrite.rasen.tech (unknown\n\t[IPv6:2400:4051:61:600:2c71:1b79:d06d:5032])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 320CC4A3;\n\tThu, 5 Aug 2021 06:02:22 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"m/kFsQbI\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1628136144;\n\tbh=yrTncMzbvFqhdcLEZ6ldQ3A+0Ru6QkZMdUGIAGSnESk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=m/kFsQbIbRfQf8eR/+p/nYQp0hKEqEjN2nla4WonrLvNeLOn+wWfEq7HEvV9qfQ/A\n\tbwo6jnZLm//BfVNXVNd0oO66qJp6B1OT+iag3XiOebgYC9MKY+p4T2IUV5WN3Ny9Ni\n\tKDu7zdHylzRTjls5OYz66qmm4U/1MNz1f5g8lNXM=","Date":"Thu, 5 Aug 2021 13:02:16 +0900","From":"paul.elder@ideasonboard.com","To":"Laurent Pinchart ","Message-ID":"<20210805040216.GM2167@pyrite.rasen.tech>","References":"<20210804124314.8044-1-laurent.pinchart@ideasonboard.com>\n\t<20210804124314.8044-7-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20210804124314.8044-7-laurent.pinchart@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v3 6/8] cam: Add KMS sink class","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18562,"web_url":"https://patchwork.libcamera.org/comment/18562/","msgid":"","date":"2021-08-05T07:04:06","subject":"Re: [libcamera-devel] [PATCH v3 6/8] cam: Add KMS sink class","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Laurent,\n\nThank you for the patch\n\nOn 8/4/21 6:13 PM, Laurent Pinchart wrote:\n> Add a KMSSink class to display framebuffers through the DRM/KMS API.\n>\n> Signed-off-by: Laurent Pinchart \n> ---\n> Changes since v2:\n>\n> - Simplify connector selection logic, and fix it when a connector name\n> is specified\n> - Fail configure() if no connector is selected\n> - Update copyright years\n>\n> Changes since v1:\n>\n> - Use formats:: namespace\n>\n> fixup! cam: Add KMS sink class\n> ---\n> src/cam/kms_sink.cpp | 306 +++++++++++++++++++++++++++++++++++++++++++\n> src/cam/kms_sink.h | 76 +++++++++++\n> src/cam/meson.build | 1 +\n> 3 files changed, 383 insertions(+)\n> create mode 100644 src/cam/kms_sink.cpp\n> create mode 100644 src/cam/kms_sink.h\n>\n> diff --git a/src/cam/kms_sink.cpp b/src/cam/kms_sink.cpp\n> new file mode 100644\n> index 000000000000..f708b613f226\n> --- /dev/null\n> +++ b/src/cam/kms_sink.cpp\n> @@ -0,0 +1,306 @@\n> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> +/*\n> + * Copyright (C) 2021, Ideas on Board Oy\n> + *\n> + * kms_sink.cpp - KMS Sink\n> + */\n> +\n> +#include \"kms_sink.h\"\n> +\n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +\n> +#include \n> +#include \n> +#include \n> +#include \n> +\n> +#include \"drm.h\"\n> +\n> +KMSSink::KMSSink(const std::string &connectorName)\n> +\t: connector_(nullptr), crtc_(nullptr), plane_(nullptr), mode_(nullptr)\n> +{\n> +\tint ret = dev_.init();\n> +\tif (ret < 0)\n> +\t\treturn;\n> +\n> +\t/*\n> +\t * Find the requested connector. If no specific connector is requested,\n> +\t * pick the first connected connector or, if no connector is connected,\n> +\t * the first connector with unknown status.\n> +\t */\n> +\tfor (const DRM::Connector &conn : dev_.connectors()) {\n> +\t\tif (!connectorName.empty()) {\n> +\t\t\tif (conn.name() != connectorName)\n> +\t\t\t\tcontinue;\n> +\n> +\t\t\tconnector_ = &conn;\n> +\t\t\tbreak;\n> +\t\t}\n> +\n> +\t\tif (conn.status() == DRM::Connector::Connected) {\n> +\t\t\tconnector_ = &conn;\n> +\t\t\tbreak;\n> +\t\t}\n> +\n> +\t\tif (!connector_ && conn.status() == DRM::Connector::Unknown)\n> +\t\t\tconnector_ = &conn;\n> +\t}\n> +\n> +\tif (!connector_) {\n> +\t\tif (!connectorName.empty())\n> +\t\t\tstd::cerr\n> +\t\t\t\t<< \"Connector \" << connectorName << \" not found\"\n> +\t\t\t\t<< std::endl;\n> +\t\telse\n> +\t\t\tstd::cerr << \"No connected connector found\" << std::endl;\n> +\t\treturn;\n> +\t}\n> +\n> +\tdev_.requestComplete.connect(this, &KMSSink::requestComplete);\n> +}\n> +\n> +void KMSSink::mapBuffer(libcamera::FrameBuffer *buffer)\n> +{\n> +\tstd::unique_ptr drmBuffer =\n> +\t\tdev_.createFrameBuffer(*buffer, format_, size_, stride_);\n> +\tif (!drmBuffer)\n> +\t\treturn;\n> +\n> +\tbuffers_.emplace(std::piecewise_construct,\n> +\t\t\t std::forward_as_tuple(buffer),\n> +\t\t\t std::forward_as_tuple(std::move(drmBuffer)));\n> +}\n> +\n> +int KMSSink::configure(const libcamera::CameraConfiguration &config)\n> +{\n> +\tif (!connector_)\n> +\t\treturn -EINVAL;\nI assumed there isValid() for a reason, an alternative would be use to \nthat. But this is fine as well probably.\n> +\n> +\tcrtc_ = nullptr;\n> +\tplane_ = nullptr;\n> +\tmode_ = nullptr;\n> +\n> +\tconst libcamera::StreamConfiguration &cfg = config.at(0);\n> +\tint ret = configurePipeline(cfg.pixelFormat);\n\nShould we be finding a compatiable connector's mode (as done below) \nbefore configurePipeline() ? Does not seem logical to error out on \nincompatiable mode after we have configured the pipeline with possible \ncrtc  and encoder.\n\n(But I maybe wrong here, there might be some implicit operational \nsequence between these two, that I am missing)\n\n> +\tif (ret < 0)\n> +\t\treturn ret;\n> +\n> +\tconst std::vector &modes = connector_->modes();\n> +\tconst auto iter = std::find_if(modes.begin(), modes.end(),\n> +\t\t\t\t [&](const DRM::Mode &mode) {\n> +\t\t\t\t\t return mode.hdisplay == cfg.size.width &&\n> +\t\t\t\t\t\t mode.vdisplay == cfg.size.height;\n> +\t\t\t\t });\n> +\tif (iter == modes.end()) {\n> +\t\tstd::cerr\n> +\t\t\t<< \"No mode matching \" << cfg.size.toString()\n> +\t\t\t<< std::endl;\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\tmode_ = &*iter;\n> +\tsize_ = cfg.size;\n> +\tstride_ = cfg.stride;\n> +\n> +\treturn 0;\n> +}\n> +\n> +int KMSSink::configurePipeline(const libcamera::PixelFormat &format)\n> +{\n> +\t/*\n> +\t * If the requested format has an alpha channel, also consider the X\n> +\t * variant.\n> +\t */\n> +\tlibcamera::PixelFormat xFormat;\n> +\n> +\tswitch (format) {\n> +\tcase libcamera::formats::ABGR8888:\n> +\t\txFormat = libcamera::formats::XBGR8888;\n> +\t\tbreak;\n> +\tcase libcamera::formats::ARGB8888:\n> +\t\txFormat = libcamera::formats::XRGB8888;\n> +\t\tbreak;\n> +\tcase libcamera::formats::BGRA8888:\n> +\t\txFormat = libcamera::formats::BGRX8888;\n> +\t\tbreak;\n> +\tcase libcamera::formats::RGBA8888:\n> +\t\txFormat = libcamera::formats::RGBX8888;\n> +\t\tbreak;\n> +\t}\n> +\n> +\t/*\n> +\t * Find a CRTC and plane suitable for the request format and the\n> +\t * connector at the end of the pipeline. Restrict the search to primary\n> +\t * planes for now.\n> +\t */\n> +\tfor (const DRM::Encoder *encoder : connector_->encoders()) {\n> +\t\tfor (const DRM::Crtc *crtc : encoder->possibleCrtcs()) {\n> +\t\t\tfor (const DRM::Plane *plane : crtc->planes()) {\n> +\t\t\t\tif (plane->type() != DRM::Plane::TypePrimary)\n> +\t\t\t\t\tcontinue;\n> +\n> +\t\t\t\tif (plane->supportsFormat(format)) {\n> +\t\t\t\t\tcrtc_ = crtc;\n> +\t\t\t\t\tplane_ = plane;\n> +\t\t\t\t\tformat_ = format;\n> +\t\t\t\t\treturn 0;\n> +\t\t\t\t}\n> +\n> +\t\t\t\tif (plane->supportsFormat(xFormat)) {\n> +\t\t\t\t\tcrtc_ = crtc;\n> +\t\t\t\t\tplane_ = plane;\n> +\t\t\t\t\tformat_ = xFormat;\n> +\t\t\t\t\treturn 0;\n> +\t\t\t\t}\n> +\t\t\t}\n> +\t\t}\n> +\t}\n> +\n> +\tstd::cerr\n> +\t\t<< \"Unable to find display pipeline for format \"\n> +\t\t<< format.toString() << std::endl;\n> +\treturn -EPIPE;\n> +}\n> +\n> +int KMSSink::start()\n> +{\n> +\tstd::unique_ptr request;\n> +\n> +\tint ret = FrameSink::start();\n> +\tif (ret < 0)\n> +\t\treturn ret;\n> +\n> +\t/* Disable all CRTCs and planes to start from a known valid state. */\n> +\trequest = std::make_unique(&dev_);\n> +\n> +\tfor (const DRM::Crtc &crtc : dev_.crtcs())\n> +\t\trequest->addProperty(&crtc, \"ACTIVE\", 0);\n> +\n> +\tfor (const DRM::Plane &plane : dev_.planes()) {\n> +\t\trequest->addProperty(&plane, \"CRTC_ID\", 0);\n> +\t\trequest->addProperty(&plane, \"FB_ID\", 0);\n> +\t}\n> +\n> +\tret = request->commit(DRM::AtomicRequest::FlagAllowModeset);\n> +\tif (ret < 0) {\n> +\t\tstd::cerr\n> +\t\t\t<< \"Failed to disable CRTCs and planes: \"\n> +\t\t\t<< strerror(-ret) << std::endl;\n> +\t\treturn ret;\n> +\t}\n> +\n> +\t/* Enable the display pipeline with no plane to start with. */\n> +\trequest = std::make_unique(&dev_);\n> +\n> +\trequest->addProperty(connector_, \"CRTC_ID\", crtc_->id());\n> +\trequest->addProperty(crtc_, \"ACTIVE\", 1);\n> +\trequest->addProperty(crtc_, \"MODE_ID\", mode_->toBlob(&dev_));\n> +\n> +\tret = request->commit(DRM::AtomicRequest::FlagAllowModeset);\n> +\tif (ret < 0) {\n> +\t\tstd::cerr\n> +\t\t\t<< \"Failed to enable display pipeline: \"\n> +\t\t\t<< strerror(-ret) << std::endl;\n> +\t\treturn ret;\n> +\t}\n> +\n> +\tplaneInitialized_ = false;\n> +\n> +\treturn 0;\n> +}\n> +\n> +int KMSSink::stop()\n> +{\n> +\t/* Display pipeline. */\n> +\tDRM::AtomicRequest request(&dev_);\n> +\n> +\trequest.addProperty(connector_, \"CRTC_ID\", 0);\n> +\trequest.addProperty(crtc_, \"ACTIVE\", 0);\n> +\trequest.addProperty(crtc_, \"MODE_ID\", 0);\n> +\trequest.addProperty(plane_, \"CRTC_ID\", 0);\n> +\trequest.addProperty(plane_, \"FB_ID\", 0);\n> +\n> +\tint ret = request.commit(DRM::AtomicRequest::FlagAllowModeset);\n> +\tif (ret < 0) {\n> +\t\tstd::cerr\n> +\t\t\t<< \"Failed to stop display pipeline: \"\n> +\t\t\t<< strerror(-ret) << std::endl;\n> +\t\treturn ret;\n> +\t}\n> +\n> +\t/* Free all buffers. */\n> +\tpending_.reset();\n> +\tqueued_.reset();\n> +\tactive_.reset();\n> +\tbuffers_.clear();\n> +\n> +\treturn FrameSink::stop();\n> +}\n> +\n> +bool KMSSink::processRequest(libcamera::Request *camRequest)\n> +{\n> +\tif (pending_)\n> +\t\treturn true;\n> +\n> +\tlibcamera::FrameBuffer *buffer = camRequest->buffers().begin()->second;\n> +\tauto iter = buffers_.find(buffer);\n> +\tif (iter == buffers_.end())\n> +\t\treturn true;\n> +\n> +\tDRM::FrameBuffer *drmBuffer = iter->second.get();\n> +\n> +\tDRM::AtomicRequest *drmRequest = new DRM::AtomicRequest(&dev_);\n> +\tdrmRequest->addProperty(plane_, \"FB_ID\", drmBuffer->id());\n> +\n> +\tif (!planeInitialized_) {\n> +\t\tdrmRequest->addProperty(plane_, \"CRTC_ID\", crtc_->id());\n> +\t\tdrmRequest->addProperty(plane_, \"SRC_X\", 0 << 16);\n> +\t\tdrmRequest->addProperty(plane_, \"SRC_Y\", 0 << 16);\n> +\t\tdrmRequest->addProperty(plane_, \"SRC_W\", mode_->hdisplay << 16);\n> +\t\tdrmRequest->addProperty(plane_, \"SRC_H\", mode_->vdisplay << 16);\n> +\t\tdrmRequest->addProperty(plane_, \"CRTC_X\", 0);\n> +\t\tdrmRequest->addProperty(plane_, \"CRTC_Y\", 0);\n> +\t\tdrmRequest->addProperty(plane_, \"CRTC_W\", mode_->hdisplay);\n> +\t\tdrmRequest->addProperty(plane_, \"CRTC_H\", mode_->vdisplay);\n> +\t\tplaneInitialized_ = true;\n> +\t}\n> +\n> +\tpending_ = std::make_unique(drmRequest, camRequest);\n\nThis is neat! :)\n\nLooks good to me with my limited understanding I gained this morning on \nthe subject:\n\nReviewed-by: Umang Jain \n\n> +\n> +\tstd::lock_guard lock(lock_);\n> +\n> +\tif (!queued_) {\n> +\t\tint ret = drmRequest->commit(DRM::AtomicRequest::FlagAsync);\n> +\t\tif (ret < 0)\n> +\t\t\tstd::cerr\n> +\t\t\t\t<< \"Failed to commit atomic request: \"\n> +\t\t\t\t<< strerror(-ret) << std::endl;\n> +\t\tqueued_ = std::move(pending_);\n> +\t}\n> +\n> +\treturn false;\n> +}\n> +\n> +void KMSSink::requestComplete(DRM::AtomicRequest *request)\n> +{\n> +\tstd::lock_guard lock(lock_);\n> +\n> +\tassert(queued_ && queued_->drmRequest_.get() == request);\n> +\n> +\t/* Complete the active request, if any. */\n> +\tif (active_)\n> +\t\trequestProcessed.emit(active_->camRequest_);\n> +\n> +\t/* The queued request becomes active. */\n> +\tactive_ = std::move(queued_);\n> +\n> +\t/* Queue the pending request, if any. */\n> +\tif (pending_) {\n> +\t\tpending_->drmRequest_->commit(DRM::AtomicRequest::FlagAsync);\n> +\t\tqueued_ = std::move(pending_);\n> +\t}\n> +}\n> diff --git a/src/cam/kms_sink.h b/src/cam/kms_sink.h\n> new file mode 100644\n> index 000000000000..2895e00f84a1\n> --- /dev/null\n> +++ b/src/cam/kms_sink.h\n> @@ -0,0 +1,76 @@\n> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> +/*\n> + * Copyright (C) 2021, Ideas on Board Oy\n> + *\n> + * kms_sink.h - KMS Sink\n> + */\n> +#ifndef __CAM_KMS_SINK_H__\n> +#define __CAM_KMS_SINK_H__\n> +\n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +\n> +#include \n> +\n> +#include \n> +#include \n> +\n> +#include \"drm.h\"\n> +#include \"frame_sink.h\"\n> +\n> +class KMSSink : public FrameSink\n> +{\n> +public:\n> +\tKMSSink(const std::string &connectorName);\n> +\n> +\tbool isValid() const { return connector_ != nullptr; }\n> +\n> +\tvoid mapBuffer(libcamera::FrameBuffer *buffer) override;\n> +\n> +\tint configure(const libcamera::CameraConfiguration &config) override;\n> +\tint start() override;\n> +\tint stop() override;\n> +\n> +\tbool processRequest(libcamera::Request *request) override;\n> +\n> +private:\n> +\tclass Request\n> +\t{\n> +\tpublic:\n> +\t\tRequest(DRM::AtomicRequest *drmRequest, libcamera::Request *camRequest)\n> +\t\t\t: drmRequest_(drmRequest), camRequest_(camRequest)\n> +\t\t{\n> +\t\t}\n> +\n> +\t\tstd::unique_ptr drmRequest_;\n> +\t\tlibcamera::Request *camRequest_;\n> +\t};\n> +\n> +\tint configurePipeline(const libcamera::PixelFormat &format);\n> +\tvoid requestComplete(DRM::AtomicRequest *request);\n> +\n> +\tDRM::Device dev_;\n> +\n> +\tconst DRM::Connector *connector_;\n> +\tconst DRM::Crtc *crtc_;\n> +\tconst DRM::Plane *plane_;\n> +\tconst DRM::Mode *mode_;\n> +\n> +\tlibcamera::PixelFormat format_;\n> +\tlibcamera::Size size_;\n> +\tunsigned int stride_;\n> +\n> +\tbool planeInitialized_;\n> +\n> +\tstd::map> buffers_;\n> +\n> +\tstd::mutex lock_;\n> +\tstd::unique_ptr pending_;\n> +\tstd::unique_ptr queued_;\n> +\tstd::unique_ptr active_;\n> +};\n> +\n> +#endif /* __CAM_KMS_SINK_H__ */\n> diff --git a/src/cam/meson.build b/src/cam/meson.build\n> index b47add55b0cb..ea36aaa5c514 100644\n> --- a/src/cam/meson.build\n> +++ b/src/cam/meson.build\n> @@ -27,6 +27,7 @@ if libdrm.found()\n> cam_cpp_args += [ '-DHAVE_KMS' ]\n> cam_sources += files([\n> 'drm.cpp',\n> + 'kms_sink.cpp'\n> ])\n> endif\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id EDE63C3232\n\tfor ;\n\tThu, 5 Aug 2021 07:04:13 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 69C7468811;\n\tThu, 5 Aug 2021 09:04:13 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A9096687CB\n\tfor ;\n\tThu, 5 Aug 2021 09:04:11 +0200 (CEST)","from [192.168.1.104] (unknown [103.251.226.40])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 88A7151D;\n\tThu, 5 Aug 2021 09:04:10 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"WAwv55/4\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1628147051;\n\tbh=vTymC6ABpKToljiVpYsaBq1a3mTqMmIN9OkKXjdevmY=;\n\th=Subject:To:References:From:Date:In-Reply-To:From;\n\tb=WAwv55/4rw/u+6o4jaAPQXGqYzpHQFceBdvQY02aaXsUY9uoeqKGRhcpt+rTQiIPd\n\tIzVVpYYLCLETkd1FFXpHR2NXmq8ht5+S+Lqk/2fWbtOWA+E8E/XNta/CCzIjyYUqTE\n\tfCHwL6U0ibbDnVCtE7NAjeH51nH9244ESGA/zFKE=","To":"Laurent Pinchart ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20210804124314.8044-1-laurent.pinchart@ideasonboard.com>\n\t<20210804124314.8044-7-laurent.pinchart@ideasonboard.com>","From":"Umang Jain ","Message-ID":"","Date":"Thu, 5 Aug 2021 12:34:06 +0530","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.10.2","MIME-Version":"1.0","In-Reply-To":"<20210804124314.8044-7-laurent.pinchart@ideasonboard.com>","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Transfer-Encoding":"8bit","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH v3 6/8] cam: Add KMS sink class","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18567,"web_url":"https://patchwork.libcamera.org/comment/18567/","msgid":"","date":"2021-08-05T11:13:58","subject":"Re: [libcamera-devel] [PATCH v3 6/8] cam: Add KMS sink class","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Umang,\n\nOn Thu, Aug 05, 2021 at 12:34:06PM +0530, Umang Jain wrote:\n> On 8/4/21 6:13 PM, Laurent Pinchart wrote:\n> > Add a KMSSink class to display framebuffers through the DRM/KMS API.\n> >\n> > Signed-off-by: Laurent Pinchart \n> > ---\n> > Changes since v2:\n> >\n> > - Simplify connector selection logic, and fix it when a connector name\n> > is specified\n> > - Fail configure() if no connector is selected\n> > - Update copyright years\n> >\n> > Changes since v1:\n> >\n> > - Use formats:: namespace\n> >\n> > fixup! cam: Add KMS sink class\n> > ---\n> > src/cam/kms_sink.cpp | 306 +++++++++++++++++++++++++++++++++++++++++++\n> > src/cam/kms_sink.h | 76 +++++++++++\n> > src/cam/meson.build | 1 +\n> > 3 files changed, 383 insertions(+)\n> > create mode 100644 src/cam/kms_sink.cpp\n> > create mode 100644 src/cam/kms_sink.h\n> >\n> > diff --git a/src/cam/kms_sink.cpp b/src/cam/kms_sink.cpp\n> > new file mode 100644\n> > index 000000000000..f708b613f226\n> > --- /dev/null\n> > +++ b/src/cam/kms_sink.cpp\n> > @@ -0,0 +1,306 @@\n> > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > +/*\n> > + * Copyright (C) 2021, Ideas on Board Oy\n> > + *\n> > + * kms_sink.cpp - KMS Sink\n> > + */\n> > +\n> > +#include \"kms_sink.h\"\n> > +\n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +\n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +\n> > +#include \"drm.h\"\n> > +\n> > +KMSSink::KMSSink(const std::string &connectorName)\n> > +\t: connector_(nullptr), crtc_(nullptr), plane_(nullptr), mode_(nullptr)\n> > +{\n> > +\tint ret = dev_.init();\n> > +\tif (ret < 0)\n> > +\t\treturn;\n> > +\n> > +\t/*\n> > +\t * Find the requested connector. If no specific connector is requested,\n> > +\t * pick the first connected connector or, if no connector is connected,\n> > +\t * the first connector with unknown status.\n> > +\t */\n> > +\tfor (const DRM::Connector &conn : dev_.connectors()) {\n> > +\t\tif (!connectorName.empty()) {\n> > +\t\t\tif (conn.name() != connectorName)\n> > +\t\t\t\tcontinue;\n> > +\n> > +\t\t\tconnector_ = &conn;\n> > +\t\t\tbreak;\n> > +\t\t}\n> > +\n> > +\t\tif (conn.status() == DRM::Connector::Connected) {\n> > +\t\t\tconnector_ = &conn;\n> > +\t\t\tbreak;\n> > +\t\t}\n> > +\n> > +\t\tif (!connector_ && conn.status() == DRM::Connector::Unknown)\n> > +\t\t\tconnector_ = &conn;\n> > +\t}\n> > +\n> > +\tif (!connector_) {\n> > +\t\tif (!connectorName.empty())\n> > +\t\t\tstd::cerr\n> > +\t\t\t\t<< \"Connector \" << connectorName << \" not found\"\n> > +\t\t\t\t<< std::endl;\n> > +\t\telse\n> > +\t\t\tstd::cerr << \"No connected connector found\" << std::endl;\n> > +\t\treturn;\n> > +\t}\n> > +\n> > +\tdev_.requestComplete.connect(this, &KMSSink::requestComplete);\n> > +}\n> > +\n> > +void KMSSink::mapBuffer(libcamera::FrameBuffer *buffer)\n> > +{\n> > +\tstd::unique_ptr drmBuffer =\n> > +\t\tdev_.createFrameBuffer(*buffer, format_, size_, stride_);\n> > +\tif (!drmBuffer)\n> > +\t\treturn;\n> > +\n> > +\tbuffers_.emplace(std::piecewise_construct,\n> > +\t\t\t std::forward_as_tuple(buffer),\n> > +\t\t\t std::forward_as_tuple(std::move(drmBuffer)));\n> > +}\n> > +\n> > +int KMSSink::configure(const libcamera::CameraConfiguration &config)\n> > +{\n> > +\tif (!connector_)\n> > +\t\treturn -EINVAL;\n>\n> I assumed there isValid() for a reason, an alternative would be use to \n> that. But this is fine as well probably.\n\nThat function is actually unused, so I'll drop it.\n\n> > +\n> > +\tcrtc_ = nullptr;\n> > +\tplane_ = nullptr;\n> > +\tmode_ = nullptr;\n> > +\n> > +\tconst libcamera::StreamConfiguration &cfg = config.at(0);\n> > +\tint ret = configurePipeline(cfg.pixelFormat);\n> \n> Should we be finding a compatiable connector's mode (as done below) \n> before configurePipeline() ? Does not seem logical to error out on \n> incompatiable mode after we have configured the pipeline with possible \n> crtc  and encoder.\n> \n> (But I maybe wrong here, there might be some implicit operational \n> sequence between these two, that I am missing)\n\nSounds reasonable, I'll swap the two.\n\n> > +\tif (ret < 0)\n> > +\t\treturn ret;\n> > +\n> > +\tconst std::vector &modes = connector_->modes();\n> > +\tconst auto iter = std::find_if(modes.begin(), modes.end(),\n> > +\t\t\t\t [&](const DRM::Mode &mode) {\n> > +\t\t\t\t\t return mode.hdisplay == cfg.size.width &&\n> > +\t\t\t\t\t\t mode.vdisplay == cfg.size.height;\n> > +\t\t\t\t });\n> > +\tif (iter == modes.end()) {\n> > +\t\tstd::cerr\n> > +\t\t\t<< \"No mode matching \" << cfg.size.toString()\n> > +\t\t\t<< std::endl;\n> > +\t\treturn -EINVAL;\n> > +\t}\n> > +\n> > +\tmode_ = &*iter;\n> > +\tsize_ = cfg.size;\n> > +\tstride_ = cfg.stride;\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > +int KMSSink::configurePipeline(const libcamera::PixelFormat &format)\n> > +{\n> > +\t/*\n> > +\t * If the requested format has an alpha channel, also consider the X\n> > +\t * variant.\n> > +\t */\n> > +\tlibcamera::PixelFormat xFormat;\n> > +\n> > +\tswitch (format) {\n> > +\tcase libcamera::formats::ABGR8888:\n> > +\t\txFormat = libcamera::formats::XBGR8888;\n> > +\t\tbreak;\n> > +\tcase libcamera::formats::ARGB8888:\n> > +\t\txFormat = libcamera::formats::XRGB8888;\n> > +\t\tbreak;\n> > +\tcase libcamera::formats::BGRA8888:\n> > +\t\txFormat = libcamera::formats::BGRX8888;\n> > +\t\tbreak;\n> > +\tcase libcamera::formats::RGBA8888:\n> > +\t\txFormat = libcamera::formats::RGBX8888;\n> > +\t\tbreak;\n> > +\t}\n> > +\n> > +\t/*\n> > +\t * Find a CRTC and plane suitable for the request format and the\n> > +\t * connector at the end of the pipeline. Restrict the search to primary\n> > +\t * planes for now.\n> > +\t */\n> > +\tfor (const DRM::Encoder *encoder : connector_->encoders()) {\n> > +\t\tfor (const DRM::Crtc *crtc : encoder->possibleCrtcs()) {\n> > +\t\t\tfor (const DRM::Plane *plane : crtc->planes()) {\n> > +\t\t\t\tif (plane->type() != DRM::Plane::TypePrimary)\n> > +\t\t\t\t\tcontinue;\n> > +\n> > +\t\t\t\tif (plane->supportsFormat(format)) {\n> > +\t\t\t\t\tcrtc_ = crtc;\n> > +\t\t\t\t\tplane_ = plane;\n> > +\t\t\t\t\tformat_ = format;\n> > +\t\t\t\t\treturn 0;\n> > +\t\t\t\t}\n> > +\n> > +\t\t\t\tif (plane->supportsFormat(xFormat)) {\n> > +\t\t\t\t\tcrtc_ = crtc;\n> > +\t\t\t\t\tplane_ = plane;\n> > +\t\t\t\t\tformat_ = xFormat;\n> > +\t\t\t\t\treturn 0;\n> > +\t\t\t\t}\n> > +\t\t\t}\n> > +\t\t}\n> > +\t}\n> > +\n> > +\tstd::cerr\n> > +\t\t<< \"Unable to find display pipeline for format \"\n> > +\t\t<< format.toString() << std::endl;\n> > +\treturn -EPIPE;\n> > +}\n> > +\n> > +int KMSSink::start()\n> > +{\n> > +\tstd::unique_ptr request;\n> > +\n> > +\tint ret = FrameSink::start();\n> > +\tif (ret < 0)\n> > +\t\treturn ret;\n> > +\n> > +\t/* Disable all CRTCs and planes to start from a known valid state. */\n> > +\trequest = std::make_unique(&dev_);\n> > +\n> > +\tfor (const DRM::Crtc &crtc : dev_.crtcs())\n> > +\t\trequest->addProperty(&crtc, \"ACTIVE\", 0);\n> > +\n> > +\tfor (const DRM::Plane &plane : dev_.planes()) {\n> > +\t\trequest->addProperty(&plane, \"CRTC_ID\", 0);\n> > +\t\trequest->addProperty(&plane, \"FB_ID\", 0);\n> > +\t}\n> > +\n> > +\tret = request->commit(DRM::AtomicRequest::FlagAllowModeset);\n> > +\tif (ret < 0) {\n> > +\t\tstd::cerr\n> > +\t\t\t<< \"Failed to disable CRTCs and planes: \"\n> > +\t\t\t<< strerror(-ret) << std::endl;\n> > +\t\treturn ret;\n> > +\t}\n> > +\n> > +\t/* Enable the display pipeline with no plane to start with. */\n> > +\trequest = std::make_unique(&dev_);\n> > +\n> > +\trequest->addProperty(connector_, \"CRTC_ID\", crtc_->id());\n> > +\trequest->addProperty(crtc_, \"ACTIVE\", 1);\n> > +\trequest->addProperty(crtc_, \"MODE_ID\", mode_->toBlob(&dev_));\n> > +\n> > +\tret = request->commit(DRM::AtomicRequest::FlagAllowModeset);\n> > +\tif (ret < 0) {\n> > +\t\tstd::cerr\n> > +\t\t\t<< \"Failed to enable display pipeline: \"\n> > +\t\t\t<< strerror(-ret) << std::endl;\n> > +\t\treturn ret;\n> > +\t}\n> > +\n> > +\tplaneInitialized_ = false;\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > +int KMSSink::stop()\n> > +{\n> > +\t/* Display pipeline. */\n> > +\tDRM::AtomicRequest request(&dev_);\n> > +\n> > +\trequest.addProperty(connector_, \"CRTC_ID\", 0);\n> > +\trequest.addProperty(crtc_, \"ACTIVE\", 0);\n> > +\trequest.addProperty(crtc_, \"MODE_ID\", 0);\n> > +\trequest.addProperty(plane_, \"CRTC_ID\", 0);\n> > +\trequest.addProperty(plane_, \"FB_ID\", 0);\n> > +\n> > +\tint ret = request.commit(DRM::AtomicRequest::FlagAllowModeset);\n> > +\tif (ret < 0) {\n> > +\t\tstd::cerr\n> > +\t\t\t<< \"Failed to stop display pipeline: \"\n> > +\t\t\t<< strerror(-ret) << std::endl;\n> > +\t\treturn ret;\n> > +\t}\n> > +\n> > +\t/* Free all buffers. */\n> > +\tpending_.reset();\n> > +\tqueued_.reset();\n> > +\tactive_.reset();\n> > +\tbuffers_.clear();\n> > +\n> > +\treturn FrameSink::stop();\n> > +}\n> > +\n> > +bool KMSSink::processRequest(libcamera::Request *camRequest)\n> > +{\n> > +\tif (pending_)\n> > +\t\treturn true;\n> > +\n> > +\tlibcamera::FrameBuffer *buffer = camRequest->buffers().begin()->second;\n> > +\tauto iter = buffers_.find(buffer);\n> > +\tif (iter == buffers_.end())\n> > +\t\treturn true;\n> > +\n> > +\tDRM::FrameBuffer *drmBuffer = iter->second.get();\n> > +\n> > +\tDRM::AtomicRequest *drmRequest = new DRM::AtomicRequest(&dev_);\n> > +\tdrmRequest->addProperty(plane_, \"FB_ID\", drmBuffer->id());\n> > +\n> > +\tif (!planeInitialized_) {\n> > +\t\tdrmRequest->addProperty(plane_, \"CRTC_ID\", crtc_->id());\n> > +\t\tdrmRequest->addProperty(plane_, \"SRC_X\", 0 << 16);\n> > +\t\tdrmRequest->addProperty(plane_, \"SRC_Y\", 0 << 16);\n> > +\t\tdrmRequest->addProperty(plane_, \"SRC_W\", mode_->hdisplay << 16);\n> > +\t\tdrmRequest->addProperty(plane_, \"SRC_H\", mode_->vdisplay << 16);\n> > +\t\tdrmRequest->addProperty(plane_, \"CRTC_X\", 0);\n> > +\t\tdrmRequest->addProperty(plane_, \"CRTC_Y\", 0);\n> > +\t\tdrmRequest->addProperty(plane_, \"CRTC_W\", mode_->hdisplay);\n> > +\t\tdrmRequest->addProperty(plane_, \"CRTC_H\", mode_->vdisplay);\n> > +\t\tplaneInitialized_ = true;\n> > +\t}\n> > +\n> > +\tpending_ = std::make_unique(drmRequest, camRequest);\n> \n> This is neat! :)\n\nUnique pointers are really nice, yes.\n\n> Looks good to me with my limited understanding I gained this morning on \n> the subject:\n> \n> Reviewed-by: Umang Jain \n> \n> > +\n> > +\tstd::lock_guard lock(lock_);\n> > +\n> > +\tif (!queued_) {\n> > +\t\tint ret = drmRequest->commit(DRM::AtomicRequest::FlagAsync);\n> > +\t\tif (ret < 0)\n> > +\t\t\tstd::cerr\n> > +\t\t\t\t<< \"Failed to commit atomic request: \"\n> > +\t\t\t\t<< strerror(-ret) << std::endl;\n> > +\t\tqueued_ = std::move(pending_);\n> > +\t}\n> > +\n> > +\treturn false;\n> > +}\n> > +\n> > +void KMSSink::requestComplete(DRM::AtomicRequest *request)\n> > +{\n> > +\tstd::lock_guard lock(lock_);\n> > +\n> > +\tassert(queued_ && queued_->drmRequest_.get() == request);\n> > +\n> > +\t/* Complete the active request, if any. */\n> > +\tif (active_)\n> > +\t\trequestProcessed.emit(active_->camRequest_);\n> > +\n> > +\t/* The queued request becomes active. */\n> > +\tactive_ = std::move(queued_);\n> > +\n> > +\t/* Queue the pending request, if any. */\n> > +\tif (pending_) {\n> > +\t\tpending_->drmRequest_->commit(DRM::AtomicRequest::FlagAsync);\n> > +\t\tqueued_ = std::move(pending_);\n> > +\t}\n> > +}\n> > diff --git a/src/cam/kms_sink.h b/src/cam/kms_sink.h\n> > new file mode 100644\n> > index 000000000000..2895e00f84a1\n> > --- /dev/null\n> > +++ b/src/cam/kms_sink.h\n> > @@ -0,0 +1,76 @@\n> > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > +/*\n> > + * Copyright (C) 2021, Ideas on Board Oy\n> > + *\n> > + * kms_sink.h - KMS Sink\n> > + */\n> > +#ifndef __CAM_KMS_SINK_H__\n> > +#define __CAM_KMS_SINK_H__\n> > +\n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +\n> > +#include \n> > +\n> > +#include \n> > +#include \n> > +\n> > +#include \"drm.h\"\n> > +#include \"frame_sink.h\"\n> > +\n> > +class KMSSink : public FrameSink\n> > +{\n> > +public:\n> > +\tKMSSink(const std::string &connectorName);\n> > +\n> > +\tbool isValid() const { return connector_ != nullptr; }\n> > +\n> > +\tvoid mapBuffer(libcamera::FrameBuffer *buffer) override;\n> > +\n> > +\tint configure(const libcamera::CameraConfiguration &config) override;\n> > +\tint start() override;\n> > +\tint stop() override;\n> > +\n> > +\tbool processRequest(libcamera::Request *request) override;\n> > +\n> > +private:\n> > +\tclass Request\n> > +\t{\n> > +\tpublic:\n> > +\t\tRequest(DRM::AtomicRequest *drmRequest, libcamera::Request *camRequest)\n> > +\t\t\t: drmRequest_(drmRequest), camRequest_(camRequest)\n> > +\t\t{\n> > +\t\t}\n> > +\n> > +\t\tstd::unique_ptr drmRequest_;\n> > +\t\tlibcamera::Request *camRequest_;\n> > +\t};\n> > +\n> > +\tint configurePipeline(const libcamera::PixelFormat &format);\n> > +\tvoid requestComplete(DRM::AtomicRequest *request);\n> > +\n> > +\tDRM::Device dev_;\n> > +\n> > +\tconst DRM::Connector *connector_;\n> > +\tconst DRM::Crtc *crtc_;\n> > +\tconst DRM::Plane *plane_;\n> > +\tconst DRM::Mode *mode_;\n> > +\n> > +\tlibcamera::PixelFormat format_;\n> > +\tlibcamera::Size size_;\n> > +\tunsigned int stride_;\n> > +\n> > +\tbool planeInitialized_;\n> > +\n> > +\tstd::map> buffers_;\n> > +\n> > +\tstd::mutex lock_;\n> > +\tstd::unique_ptr pending_;\n> > +\tstd::unique_ptr queued_;\n> > +\tstd::unique_ptr active_;\n> > +};\n> > +\n> > +#endif /* __CAM_KMS_SINK_H__ */\n> > diff --git a/src/cam/meson.build b/src/cam/meson.build\n> > index b47add55b0cb..ea36aaa5c514 100644\n> > --- a/src/cam/meson.build\n> > +++ b/src/cam/meson.build\n> > @@ -27,6 +27,7 @@ if libdrm.found()\n> > cam_cpp_args += [ '-DHAVE_KMS' ]\n> > cam_sources += files([\n> > 'drm.cpp',\n> > + 'kms_sink.cpp'\n> > ])\n> > endif\n> >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id C19A2C3235\n\tfor ;\n\tThu, 5 Aug 2021 11:14:12 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 387BF68811;\n\tThu, 5 Aug 2021 13:14:12 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 608DD6026D\n\tfor ;\n\tThu, 5 Aug 2021 13:14:11 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id D2D11E04;\n\tThu, 5 Aug 2021 13:14:10 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"cmlRfNGP\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1628162051;\n\tbh=3mdWTZ5h10omxCCDWqXvGkHrzUAhdk2AtlGG+ZkSisE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=cmlRfNGPG7TM/7wouVwAIQRouQKG0Nsu674ZVfFP1zxQY5EtcZg0RR8oK1Bbf+BTi\n\tx3IBl9Ek1OCP4eqBBe5WWpZI1JQbOBecwoJRrsPKemNce46XePWkPDLGx7xtkBzFVC\n\tergEoAjbP8eIwi8w7ZbTHI7hEs7unV7M3ZbvF9NI=","Date":"Thu, 5 Aug 2021 14:13:58 +0300","From":"Laurent Pinchart ","To":"Umang Jain ","Message-ID":"","References":"<20210804124314.8044-1-laurent.pinchart@ideasonboard.com>\n\t<20210804124314.8044-7-laurent.pinchart@ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v3 6/8] cam: Add KMS sink class","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18570,"web_url":"https://patchwork.libcamera.org/comment/18570/","msgid":"<3396c55d-bf29-7b8a-5e34-07bdc18a064f@ideasonboard.com>","date":"2021-08-05T12:32:35","subject":"Re: [libcamera-devel] [PATCH v3 6/8] cam: Add KMS sink class","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Laurent,\n\nOn 04/08/2021 13:43, Laurent Pinchart wrote:\n> Add a KMSSink class to display framebuffers through the DRM/KMS API.\n> \n> Signed-off-by: Laurent Pinchart \n> ---\n> Changes since v2:\n> \n> - Simplify connector selection logic, and fix it when a connector name\n> is specified\n> - Fail configure() if no connector is selected\n> - Update copyright years\n> \n> Changes since v1:\n> \n> - Use formats:: namespace\n> \n> fixup! cam: Add KMS sink class\n\nOk ;-)\n\n\n> ---\n> src/cam/kms_sink.cpp | 306 +++++++++++++++++++++++++++++++++++++++++++\n> src/cam/kms_sink.h | 76 +++++++++++\n> src/cam/meson.build | 1 +\n> 3 files changed, 383 insertions(+)\n> create mode 100644 src/cam/kms_sink.cpp\n> create mode 100644 src/cam/kms_sink.h\n> \n> diff --git a/src/cam/kms_sink.cpp b/src/cam/kms_sink.cpp\n> new file mode 100644\n> index 000000000000..f708b613f226\n> --- /dev/null\n> +++ b/src/cam/kms_sink.cpp\n> @@ -0,0 +1,306 @@\n> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> +/*\n> + * Copyright (C) 2021, Ideas on Board Oy\n> + *\n> + * kms_sink.cpp - KMS Sink\n> + */\n> +\n> +#include \"kms_sink.h\"\n> +\n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +\n> +#include \n> +#include \n> +#include \n> +#include \n> +\n> +#include \"drm.h\"\n> +\n> +KMSSink::KMSSink(const std::string &connectorName)\n> +\t: connector_(nullptr), crtc_(nullptr), plane_(nullptr), mode_(nullptr)\n> +{\n> +\tint ret = dev_.init();\n> +\tif (ret < 0)\n> +\t\treturn;\n> +\n> +\t/*\n> +\t * Find the requested connector. If no specific connector is requested,\n> +\t * pick the first connected connector or, if no connector is connected,\n> +\t * the first connector with unknown status.\n> +\t */\n> +\tfor (const DRM::Connector &conn : dev_.connectors()) {\n> +\t\tif (!connectorName.empty()) {\n> +\t\t\tif (conn.name() != connectorName)\n> +\t\t\t\tcontinue;\n> +\n> +\t\t\tconnector_ = &conn;\n> +\t\t\tbreak;\n> +\t\t}\n> +\n> +\t\tif (conn.status() == DRM::Connector::Connected) {\n> +\t\t\tconnector_ = &conn;\n> +\t\t\tbreak;\n> +\t\t}\n> +\n> +\t\tif (!connector_ && conn.status() == DRM::Connector::Unknown)\n> +\t\t\tconnector_ = &conn;\n> +\t}\n> +\n> +\tif (!connector_) {\n> +\t\tif (!connectorName.empty())\n> +\t\t\tstd::cerr\n> +\t\t\t\t<< \"Connector \" << connectorName << \" not found\"\n> +\t\t\t\t<< std::endl;\n> +\t\telse\n> +\t\t\tstd::cerr << \"No connected connector found\" << std::endl;\n> +\t\treturn;\n> +\t}\n> +\n> +\tdev_.requestComplete.connect(this, &KMSSink::requestComplete);\n> +}\n> +\n> +void KMSSink::mapBuffer(libcamera::FrameBuffer *buffer)\n> +{\n> +\tstd::unique_ptr drmBuffer =\n> +\t\tdev_.createFrameBuffer(*buffer, format_, size_, stride_);\n> +\tif (!drmBuffer)\n> +\t\treturn;\n> +\n> +\tbuffers_.emplace(std::piecewise_construct,\n> +\t\t\t std::forward_as_tuple(buffer),\n> +\t\t\t std::forward_as_tuple(std::move(drmBuffer)));\n> +}\n> +\n> +int KMSSink::configure(const libcamera::CameraConfiguration &config)\n> +{\n> +\tif (!connector_)\n> +\t\treturn -EINVAL;\n> +\n> +\tcrtc_ = nullptr;\n> +\tplane_ = nullptr;\n> +\tmode_ = nullptr;\n> +\n> +\tconst libcamera::StreamConfiguration &cfg = config.at(0);\n> +\tint ret = configurePipeline(cfg.pixelFormat);\n> +\tif (ret < 0)\n> +\t\treturn ret;\n> +\n> +\tconst std::vector &modes = connector_->modes();\n> +\tconst auto iter = std::find_if(modes.begin(), modes.end(),\n> +\t\t\t\t [&](const DRM::Mode &mode) {\n> +\t\t\t\t\t return mode.hdisplay == cfg.size.width &&\n> +\t\t\t\t\t\t mode.vdisplay == cfg.size.height;\n> +\t\t\t\t });\n> +\tif (iter == modes.end()) {\n> +\t\tstd::cerr\n> +\t\t\t<< \"No mode matching \" << cfg.size.toString()\n> +\t\t\t<< std::endl;\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\tmode_ = &*iter;\n> +\tsize_ = cfg.size;\n> +\tstride_ = cfg.stride;\n> +\n> +\treturn 0;\n> +}\n> +\n> +int KMSSink::configurePipeline(const libcamera::PixelFormat &format)\n> +{\n> +\t/*\n> +\t * If the requested format has an alpha channel, also consider the X\n> +\t * variant.\n> +\t */\n> +\tlibcamera::PixelFormat xFormat;\n> +\n> +\tswitch (format) {\n> +\tcase libcamera::formats::ABGR8888:\n> +\t\txFormat = libcamera::formats::XBGR8888;\n> +\t\tbreak;\n> +\tcase libcamera::formats::ARGB8888:\n> +\t\txFormat = libcamera::formats::XRGB8888;\n> +\t\tbreak;\n> +\tcase libcamera::formats::BGRA8888:\n> +\t\txFormat = libcamera::formats::BGRX8888;\n> +\t\tbreak;\n> +\tcase libcamera::formats::RGBA8888:\n> +\t\txFormat = libcamera::formats::RGBX8888;\n> +\t\tbreak;\n> +\t}\n> +\n> +\t/*\n> +\t * Find a CRTC and plane suitable for the request format and the\n> +\t * connector at the end of the pipeline. Restrict the search to primary\n> +\t * planes for now.\n> +\t */\n> +\tfor (const DRM::Encoder *encoder : connector_->encoders()) {\n> +\t\tfor (const DRM::Crtc *crtc : encoder->possibleCrtcs()) {\n> +\t\t\tfor (const DRM::Plane *plane : crtc->planes()) {\n> +\t\t\t\tif (plane->type() != DRM::Plane::TypePrimary)\n> +\t\t\t\t\tcontinue;\n> +\n> +\t\t\t\tif (plane->supportsFormat(format)) {\n> +\t\t\t\t\tcrtc_ = crtc;\n> +\t\t\t\t\tplane_ = plane;\n> +\t\t\t\t\tformat_ = format;\n> +\t\t\t\t\treturn 0;\n> +\t\t\t\t}\n> +\n> +\t\t\t\tif (plane->supportsFormat(xFormat)) {\n> +\t\t\t\t\tcrtc_ = crtc;\n> +\t\t\t\t\tplane_ = plane;\n> +\t\t\t\t\tformat_ = xFormat;\n> +\t\t\t\t\treturn 0;\n> +\t\t\t\t}\n> +\t\t\t}\n> +\t\t}\n> +\t}\n> +\n> +\tstd::cerr\n> +\t\t<< \"Unable to find display pipeline for format \"\n> +\t\t<< format.toString() << std::endl;\n\npossibly a blank line here but meh.\n\n\n> +\treturn -EPIPE;\n> +}\n> +\n> +int KMSSink::start()\n> +{\n> +\tstd::unique_ptr request;\n> +\n> +\tint ret = FrameSink::start();\n> +\tif (ret < 0)\n> +\t\treturn ret;\n> +\n> +\t/* Disable all CRTCs and planes to start from a known valid state. */\n> +\trequest = std::make_unique(&dev_);\n> +\n> +\tfor (const DRM::Crtc &crtc : dev_.crtcs())\n> +\t\trequest->addProperty(&crtc, \"ACTIVE\", 0);\n> +\n> +\tfor (const DRM::Plane &plane : dev_.planes()) {\n> +\t\trequest->addProperty(&plane, \"CRTC_ID\", 0);\n> +\t\trequest->addProperty(&plane, \"FB_ID\", 0);\n> +\t}\n> +\n> +\tret = request->commit(DRM::AtomicRequest::FlagAllowModeset);\n> +\tif (ret < 0) {\n> +\t\tstd::cerr\n> +\t\t\t<< \"Failed to disable CRTCs and planes: \"\n> +\t\t\t<< strerror(-ret) << std::endl;\n> +\t\treturn ret;\n> +\t}\n> +\n> +\t/* Enable the display pipeline with no plane to start with. */\n> +\trequest = std::make_unique(&dev_);\n> +\n> +\trequest->addProperty(connector_, \"CRTC_ID\", crtc_->id());\n> +\trequest->addProperty(crtc_, \"ACTIVE\", 1);\n> +\trequest->addProperty(crtc_, \"MODE_ID\", mode_->toBlob(&dev_));\n> +\n> +\tret = request->commit(DRM::AtomicRequest::FlagAllowModeset);\n> +\tif (ret < 0) {\n> +\t\tstd::cerr\n> +\t\t\t<< \"Failed to enable display pipeline: \"\n> +\t\t\t<< strerror(-ret) << std::endl;\n> +\t\treturn ret;\n> +\t}\n> +\n> +\tplaneInitialized_ = false;\n> +\n> +\treturn 0;\n> +}\n> +\n> +int KMSSink::stop()\n> +{\n> +\t/* Display pipeline. */\n> +\tDRM::AtomicRequest request(&dev_);\n> +\n> +\trequest.addProperty(connector_, \"CRTC_ID\", 0);\n> +\trequest.addProperty(crtc_, \"ACTIVE\", 0);\n> +\trequest.addProperty(crtc_, \"MODE_ID\", 0);\n> +\trequest.addProperty(plane_, \"CRTC_ID\", 0);\n> +\trequest.addProperty(plane_, \"FB_ID\", 0);\n> +\n> +\tint ret = request.commit(DRM::AtomicRequest::FlagAllowModeset);\n> +\tif (ret < 0) {\n> +\t\tstd::cerr\n> +\t\t\t<< \"Failed to stop display pipeline: \"\n> +\t\t\t<< strerror(-ret) << std::endl;\n> +\t\treturn ret;\n> +\t}\n> +\n> +\t/* Free all buffers. */\n> +\tpending_.reset();\n> +\tqueued_.reset();\n> +\tactive_.reset();\n> +\tbuffers_.clear();\n> +\n> +\treturn FrameSink::stop();\n> +}\n> +\n> +bool KMSSink::processRequest(libcamera::Request *camRequest)\n> +{\n> +\tif (pending_)\n> +\t\treturn true;\n\nThis feels like it needs a comment.\n\nIf there is a pending - are we skipping this request because we're\nessentially overflowing / receiving more completed requests than we can\ndisplay?\n\n\n\n\nReading that I'm resisting suggesting that processRequest could return\nan 'enum state' instead of bool.\n\n\tif (pending_)\n\t\treturn FrameSink::RequestHandled;\n\nBut that's probably overkill, so maybe I won't suggest it ;-)\n\n\n> +\n> +\tlibcamera::FrameBuffer *buffer = camRequest->buffers().begin()->second;\n> +\tauto iter = buffers_.find(buffer);\n> +\tif (iter == buffers_.end())\n> +\t\treturn true;\n> +\n> +\tDRM::FrameBuffer *drmBuffer = iter->second.get();\n> +\n> +\tDRM::AtomicRequest *drmRequest = new DRM::AtomicRequest(&dev_);\n> +\tdrmRequest->addProperty(plane_, \"FB_ID\", drmBuffer->id());\n> +\n> +\tif (!planeInitialized_) {\n> +\t\tdrmRequest->addProperty(plane_, \"CRTC_ID\", crtc_->id());\n> +\t\tdrmRequest->addProperty(plane_, \"SRC_X\", 0 << 16);\n> +\t\tdrmRequest->addProperty(plane_, \"SRC_Y\", 0 << 16);\n> +\t\tdrmRequest->addProperty(plane_, \"SRC_W\", mode_->hdisplay << 16);\n> +\t\tdrmRequest->addProperty(plane_, \"SRC_H\", mode_->vdisplay << 16);\n> +\t\tdrmRequest->addProperty(plane_, \"CRTC_X\", 0);\n> +\t\tdrmRequest->addProperty(plane_, \"CRTC_Y\", 0);\n> +\t\tdrmRequest->addProperty(plane_, \"CRTC_W\", mode_->hdisplay);\n> +\t\tdrmRequest->addProperty(plane_, \"CRTC_H\", mode_->vdisplay);\n> +\t\tplaneInitialized_ = true;\n> +\t}\n> +\n> +\tpending_ = std::make_unique(drmRequest, camRequest);\n\nI wonder if we should optimise this so we don't have to malloc a new\nobject each time - but that could be ignored here.\n\nIf we had a means to 'set' the drmRequest and camRequest, we could have\na fourth st::unique_pointer to move the latest completed\nrequest to - and re-use it - but we're not worried about\n\n\n> +\n> +\tstd::lock_guard lock(lock_);\n> +\n> +\tif (!queued_) {\n> +\t\tint ret = drmRequest->commit(DRM::AtomicRequest::FlagAsync);\n> +\t\tif (ret < 0)\n> +\t\t\tstd::cerr\n> +\t\t\t\t<< \"Failed to commit atomic request: \"\n> +\t\t\t\t<< strerror(-ret) << std::endl;\n> +\t\tqueued_ = std::move(pending_);\n\nIs this right? If we failed to queue it - should we mark it as queued?\nOr is this just to manage the lifecycle of the allocation...\n\nIf it's intentional - it deserves a comment to express that I think.\n\nI suspect it is required to release pending_ so that we can let the next\nrequest in regardless...\n\n\n\n> +\t}\n> +\n> +\treturn false;\n> +}\n> +\n> +void KMSSink::requestComplete(DRM::AtomicRequest *request)\n> +{\n> +\tstd::lock_guard lock(lock_);\n> +\n> +\tassert(queued_ && queued_->drmRequest_.get() == request);\n> +\n> +\t/* Complete the active request, if any. */\n> +\tif (active_)\n> +\t\trequestProcessed.emit(active_->camRequest_);\n> +\n> +\t/* The queued request becomes active. */\n> +\tactive_ = std::move(queued_);\n\nIs the request guaranteed to have been queued to the hardware at this\npoint? Is there any error handling - such as some state to track or\nreport in the AtomicRequest?\n\nEither way, even if that failed, the active_ would be released back to\nthe requestProcessed signal on the next iteration anyway... so the\n'pipeline' here will be fine as far as I can see.\n\n\nLooks like my comments are only about adding comments so ...\n\nReviewed-by: Kieran Bingham \n\n\n\n\n> +\n> +\t/* Queue the pending request, if any. */\n> +\tif (pending_) {\n> +\t\tpending_->drmRequest_->commit(DRM::AtomicRequest::FlagAsync);\n> +\t\tqueued_ = std::move(pending_);\n> +\t}\n> +}\n> diff --git a/src/cam/kms_sink.h b/src/cam/kms_sink.h\n> new file mode 100644\n> index 000000000000..2895e00f84a1\n> --- /dev/null\n> +++ b/src/cam/kms_sink.h\n> @@ -0,0 +1,76 @@\n> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> +/*\n> + * Copyright (C) 2021, Ideas on Board Oy\n> + *\n> + * kms_sink.h - KMS Sink\n> + */\n> +#ifndef __CAM_KMS_SINK_H__\n> +#define __CAM_KMS_SINK_H__\n> +\n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +\n> +#include \n> +\n> +#include \n> +#include \n> +\n> +#include \"drm.h\"\n> +#include \"frame_sink.h\"\n> +\n> +class KMSSink : public FrameSink\n> +{\n> +public:\n> +\tKMSSink(const std::string &connectorName);\n> +\n> +\tbool isValid() const { return connector_ != nullptr; }\n> +\n> +\tvoid mapBuffer(libcamera::FrameBuffer *buffer) override;\n> +\n> +\tint configure(const libcamera::CameraConfiguration &config) override;\n> +\tint start() override;\n> +\tint stop() override;\n> +\n> +\tbool processRequest(libcamera::Request *request) override;\n> +\n> +private:\n> +\tclass Request\n> +\t{\n> +\tpublic:\n> +\t\tRequest(DRM::AtomicRequest *drmRequest, libcamera::Request *camRequest)\n> +\t\t\t: drmRequest_(drmRequest), camRequest_(camRequest)\n> +\t\t{\n> +\t\t}\n> +\n> +\t\tstd::unique_ptr drmRequest_;\n> +\t\tlibcamera::Request *camRequest_;\n> +\t};\n> +\n> +\tint configurePipeline(const libcamera::PixelFormat &format);\n> +\tvoid requestComplete(DRM::AtomicRequest *request);\n> +\n> +\tDRM::Device dev_;\n> +\n> +\tconst DRM::Connector *connector_;\n> +\tconst DRM::Crtc *crtc_;\n> +\tconst DRM::Plane *plane_;\n> +\tconst DRM::Mode *mode_;\n> +\n> +\tlibcamera::PixelFormat format_;\n> +\tlibcamera::Size size_;\n> +\tunsigned int stride_;\n> +\n> +\tbool planeInitialized_;\n> +\n> +\tstd::map> buffers_;\n> +\n> +\tstd::mutex lock_;\n> +\tstd::unique_ptr pending_;\n> +\tstd::unique_ptr queued_;\n> +\tstd::unique_ptr active_;> +};\n> +\n> +#endif /* __CAM_KMS_SINK_H__ */\n> diff --git a/src/cam/meson.build b/src/cam/meson.build\n> index b47add55b0cb..ea36aaa5c514 100644\n> --- a/src/cam/meson.build\n> +++ b/src/cam/meson.build\n> @@ -27,6 +27,7 @@ if libdrm.found()\n> cam_cpp_args += [ '-DHAVE_KMS' ]\n> cam_sources += files([\n> 'drm.cpp',\n> + 'kms_sink.cpp'\n> ])\n> endif\n> \n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 44A51C3237\n\tfor ;\n\tThu, 5 Aug 2021 12:32:41 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 93CAD68811;\n\tThu, 5 Aug 2021 14:32:40 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0BD426026D\n\tfor ;\n\tThu, 5 Aug 2021 14:32:39 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 743BC51D;\n\tThu, 5 Aug 2021 14:32:38 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"dI+99TNs\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1628166758;\n\tbh=OJ/x+Ns770QwNMppKjZPil7Q5rAFmSE0b37ofxTbnAA=;\n\th=To:References:From:Subject:Date:In-Reply-To:From;\n\tb=dI+99TNs4fDvtDmAevWWBcgpk32vTrDx6pypoWkc4y7v+0NUIt5XIEYC4UO+oko+S\n\tGLYfJLNmuIWz8JrFeACuDRAh/kHwITEBEGKppLOjAr20+iFPssaID3cBzuitIP2S3h\n\t/j2UbF4IuDFaP1ZfaoQK8HKPB1P+8MKBTYwMYIeo=","To":"Laurent Pinchart ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20210804124314.8044-1-laurent.pinchart@ideasonboard.com>\n\t<20210804124314.8044-7-laurent.pinchart@ideasonboard.com>","From":"Kieran Bingham ","Message-ID":"<3396c55d-bf29-7b8a-5e34-07bdc18a064f@ideasonboard.com>","Date":"Thu, 5 Aug 2021 13:32:35 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.11.0","MIME-Version":"1.0","In-Reply-To":"<20210804124314.8044-7-laurent.pinchart@ideasonboard.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH v3 6/8] cam: Add KMS sink class","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18574,"web_url":"https://patchwork.libcamera.org/comment/18574/","msgid":"","date":"2021-08-05T13:06:38","subject":"Re: [libcamera-devel] [PATCH v3 6/8] cam: Add KMS sink class","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nOn Thu, Aug 05, 2021 at 01:32:35PM +0100, Kieran Bingham wrote:\n> On 04/08/2021 13:43, Laurent Pinchart wrote:\n> > Add a KMSSink class to display framebuffers through the DRM/KMS API.\n> > \n> > Signed-off-by: Laurent Pinchart \n> > ---\n> > Changes since v2:\n> > \n> > - Simplify connector selection logic, and fix it when a connector name\n> > is specified\n> > - Fail configure() if no connector is selected\n> > - Update copyright years\n> > \n> > Changes since v1:\n> > \n> > - Use formats:: namespace\n> > \n> > fixup! cam: Add KMS sink class\n> \n> Ok ;-)\n\nOops, caught red-handed using git commit --fixup :-)\n\n> > ---\n> > src/cam/kms_sink.cpp | 306 +++++++++++++++++++++++++++++++++++++++++++\n> > src/cam/kms_sink.h | 76 +++++++++++\n> > src/cam/meson.build | 1 +\n> > 3 files changed, 383 insertions(+)\n> > create mode 100644 src/cam/kms_sink.cpp\n> > create mode 100644 src/cam/kms_sink.h\n> > \n> > diff --git a/src/cam/kms_sink.cpp b/src/cam/kms_sink.cpp\n> > new file mode 100644\n> > index 000000000000..f708b613f226\n> > --- /dev/null\n> > +++ b/src/cam/kms_sink.cpp\n> > @@ -0,0 +1,306 @@\n> > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > +/*\n> > + * Copyright (C) 2021, Ideas on Board Oy\n> > + *\n> > + * kms_sink.cpp - KMS Sink\n> > + */\n> > +\n> > +#include \"kms_sink.h\"\n> > +\n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +\n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +\n> > +#include \"drm.h\"\n> > +\n> > +KMSSink::KMSSink(const std::string &connectorName)\n> > +\t: connector_(nullptr), crtc_(nullptr), plane_(nullptr), mode_(nullptr)\n> > +{\n> > +\tint ret = dev_.init();\n> > +\tif (ret < 0)\n> > +\t\treturn;\n> > +\n> > +\t/*\n> > +\t * Find the requested connector. If no specific connector is requested,\n> > +\t * pick the first connected connector or, if no connector is connected,\n> > +\t * the first connector with unknown status.\n> > +\t */\n> > +\tfor (const DRM::Connector &conn : dev_.connectors()) {\n> > +\t\tif (!connectorName.empty()) {\n> > +\t\t\tif (conn.name() != connectorName)\n> > +\t\t\t\tcontinue;\n> > +\n> > +\t\t\tconnector_ = &conn;\n> > +\t\t\tbreak;\n> > +\t\t}\n> > +\n> > +\t\tif (conn.status() == DRM::Connector::Connected) {\n> > +\t\t\tconnector_ = &conn;\n> > +\t\t\tbreak;\n> > +\t\t}\n> > +\n> > +\t\tif (!connector_ && conn.status() == DRM::Connector::Unknown)\n> > +\t\t\tconnector_ = &conn;\n> > +\t}\n> > +\n> > +\tif (!connector_) {\n> > +\t\tif (!connectorName.empty())\n> > +\t\t\tstd::cerr\n> > +\t\t\t\t<< \"Connector \" << connectorName << \" not found\"\n> > +\t\t\t\t<< std::endl;\n> > +\t\telse\n> > +\t\t\tstd::cerr << \"No connected connector found\" << std::endl;\n> > +\t\treturn;\n> > +\t}\n> > +\n> > +\tdev_.requestComplete.connect(this, &KMSSink::requestComplete);\n> > +}\n> > +\n> > +void KMSSink::mapBuffer(libcamera::FrameBuffer *buffer)\n> > +{\n> > +\tstd::unique_ptr drmBuffer =\n> > +\t\tdev_.createFrameBuffer(*buffer, format_, size_, stride_);\n> > +\tif (!drmBuffer)\n> > +\t\treturn;\n> > +\n> > +\tbuffers_.emplace(std::piecewise_construct,\n> > +\t\t\t std::forward_as_tuple(buffer),\n> > +\t\t\t std::forward_as_tuple(std::move(drmBuffer)));\n> > +}\n> > +\n> > +int KMSSink::configure(const libcamera::CameraConfiguration &config)\n> > +{\n> > +\tif (!connector_)\n> > +\t\treturn -EINVAL;\n> > +\n> > +\tcrtc_ = nullptr;\n> > +\tplane_ = nullptr;\n> > +\tmode_ = nullptr;\n> > +\n> > +\tconst libcamera::StreamConfiguration &cfg = config.at(0);\n> > +\tint ret = configurePipeline(cfg.pixelFormat);\n> > +\tif (ret < 0)\n> > +\t\treturn ret;\n> > +\n> > +\tconst std::vector &modes = connector_->modes();\n> > +\tconst auto iter = std::find_if(modes.begin(), modes.end(),\n> > +\t\t\t\t [&](const DRM::Mode &mode) {\n> > +\t\t\t\t\t return mode.hdisplay == cfg.size.width &&\n> > +\t\t\t\t\t\t mode.vdisplay == cfg.size.height;\n> > +\t\t\t\t });\n> > +\tif (iter == modes.end()) {\n> > +\t\tstd::cerr\n> > +\t\t\t<< \"No mode matching \" << cfg.size.toString()\n> > +\t\t\t<< std::endl;\n> > +\t\treturn -EINVAL;\n> > +\t}\n> > +\n> > +\tmode_ = &*iter;\n> > +\tsize_ = cfg.size;\n> > +\tstride_ = cfg.stride;\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > +int KMSSink::configurePipeline(const libcamera::PixelFormat &format)\n> > +{\n> > +\t/*\n> > +\t * If the requested format has an alpha channel, also consider the X\n> > +\t * variant.\n> > +\t */\n> > +\tlibcamera::PixelFormat xFormat;\n> > +\n> > +\tswitch (format) {\n> > +\tcase libcamera::formats::ABGR8888:\n> > +\t\txFormat = libcamera::formats::XBGR8888;\n> > +\t\tbreak;\n> > +\tcase libcamera::formats::ARGB8888:\n> > +\t\txFormat = libcamera::formats::XRGB8888;\n> > +\t\tbreak;\n> > +\tcase libcamera::formats::BGRA8888:\n> > +\t\txFormat = libcamera::formats::BGRX8888;\n> > +\t\tbreak;\n> > +\tcase libcamera::formats::RGBA8888:\n> > +\t\txFormat = libcamera::formats::RGBX8888;\n> > +\t\tbreak;\n> > +\t}\n> > +\n> > +\t/*\n> > +\t * Find a CRTC and plane suitable for the request format and the\n> > +\t * connector at the end of the pipeline. Restrict the search to primary\n> > +\t * planes for now.\n> > +\t */\n> > +\tfor (const DRM::Encoder *encoder : connector_->encoders()) {\n> > +\t\tfor (const DRM::Crtc *crtc : encoder->possibleCrtcs()) {\n> > +\t\t\tfor (const DRM::Plane *plane : crtc->planes()) {\n> > +\t\t\t\tif (plane->type() != DRM::Plane::TypePrimary)\n> > +\t\t\t\t\tcontinue;\n> > +\n> > +\t\t\t\tif (plane->supportsFormat(format)) {\n> > +\t\t\t\t\tcrtc_ = crtc;\n> > +\t\t\t\t\tplane_ = plane;\n> > +\t\t\t\t\tformat_ = format;\n> > +\t\t\t\t\treturn 0;\n> > +\t\t\t\t}\n> > +\n> > +\t\t\t\tif (plane->supportsFormat(xFormat)) {\n> > +\t\t\t\t\tcrtc_ = crtc;\n> > +\t\t\t\t\tplane_ = plane;\n> > +\t\t\t\t\tformat_ = xFormat;\n> > +\t\t\t\t\treturn 0;\n> > +\t\t\t\t}\n> > +\t\t\t}\n> > +\t\t}\n> > +\t}\n> > +\n> > +\tstd::cerr\n> > +\t\t<< \"Unable to find display pipeline for format \"\n> > +\t\t<< format.toString() << std::endl;\n> \n> possibly a blank line here but meh.\n\nIf that's all it takes to make you happy, sure :-)\n\n> > +\treturn -EPIPE;\n> > +}\n> > +\n> > +int KMSSink::start()\n> > +{\n> > +\tstd::unique_ptr request;\n> > +\n> > +\tint ret = FrameSink::start();\n> > +\tif (ret < 0)\n> > +\t\treturn ret;\n> > +\n> > +\t/* Disable all CRTCs and planes to start from a known valid state. */\n> > +\trequest = std::make_unique(&dev_);\n> > +\n> > +\tfor (const DRM::Crtc &crtc : dev_.crtcs())\n> > +\t\trequest->addProperty(&crtc, \"ACTIVE\", 0);\n> > +\n> > +\tfor (const DRM::Plane &plane : dev_.planes()) {\n> > +\t\trequest->addProperty(&plane, \"CRTC_ID\", 0);\n> > +\t\trequest->addProperty(&plane, \"FB_ID\", 0);\n> > +\t}\n> > +\n> > +\tret = request->commit(DRM::AtomicRequest::FlagAllowModeset);\n> > +\tif (ret < 0) {\n> > +\t\tstd::cerr\n> > +\t\t\t<< \"Failed to disable CRTCs and planes: \"\n> > +\t\t\t<< strerror(-ret) << std::endl;\n> > +\t\treturn ret;\n> > +\t}\n> > +\n> > +\t/* Enable the display pipeline with no plane to start with. */\n> > +\trequest = std::make_unique(&dev_);\n> > +\n> > +\trequest->addProperty(connector_, \"CRTC_ID\", crtc_->id());\n> > +\trequest->addProperty(crtc_, \"ACTIVE\", 1);\n> > +\trequest->addProperty(crtc_, \"MODE_ID\", mode_->toBlob(&dev_));\n> > +\n> > +\tret = request->commit(DRM::AtomicRequest::FlagAllowModeset);\n> > +\tif (ret < 0) {\n> > +\t\tstd::cerr\n> > +\t\t\t<< \"Failed to enable display pipeline: \"\n> > +\t\t\t<< strerror(-ret) << std::endl;\n> > +\t\treturn ret;\n> > +\t}\n> > +\n> > +\tplaneInitialized_ = false;\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > +int KMSSink::stop()\n> > +{\n> > +\t/* Display pipeline. */\n> > +\tDRM::AtomicRequest request(&dev_);\n> > +\n> > +\trequest.addProperty(connector_, \"CRTC_ID\", 0);\n> > +\trequest.addProperty(crtc_, \"ACTIVE\", 0);\n> > +\trequest.addProperty(crtc_, \"MODE_ID\", 0);\n> > +\trequest.addProperty(plane_, \"CRTC_ID\", 0);\n> > +\trequest.addProperty(plane_, \"FB_ID\", 0);\n> > +\n> > +\tint ret = request.commit(DRM::AtomicRequest::FlagAllowModeset);\n> > +\tif (ret < 0) {\n> > +\t\tstd::cerr\n> > +\t\t\t<< \"Failed to stop display pipeline: \"\n> > +\t\t\t<< strerror(-ret) << std::endl;\n> > +\t\treturn ret;\n> > +\t}\n> > +\n> > +\t/* Free all buffers. */\n> > +\tpending_.reset();\n> > +\tqueued_.reset();\n> > +\tactive_.reset();\n> > +\tbuffers_.clear();\n> > +\n> > +\treturn FrameSink::stop();\n> > +}\n> > +\n> > +bool KMSSink::processRequest(libcamera::Request *camRequest)\n> > +{\n> > +\tif (pending_)\n> > +\t\treturn true;\n> \n> This feels like it needs a comment.\n> \n> If there is a pending - are we skipping this request because we're\n> essentially overflowing / receiving more completed requests than we can\n> display?\n\nCorrect. I'll add a comment.\n\n /*\n * Perform a very crude rate adaptation by simply dropping the request\n * if the display queue is full.\n */\n\n> Reading that I'm resisting suggesting that processRequest could return\n> an 'enum state' instead of bool.\n> \n> \tif (pending_)\n> \t\treturn FrameSink::RequestHandled;\n> \n> But that's probably overkill, so maybe I won't suggest it ;-)\n\n:-) I think we'll rework this anyway when adding the option to use\nmultiple sinks.\n\n> > +\n> > +\tlibcamera::FrameBuffer *buffer = camRequest->buffers().begin()->second;\n> > +\tauto iter = buffers_.find(buffer);\n> > +\tif (iter == buffers_.end())\n> > +\t\treturn true;\n> > +\n> > +\tDRM::FrameBuffer *drmBuffer = iter->second.get();\n> > +\n> > +\tDRM::AtomicRequest *drmRequest = new DRM::AtomicRequest(&dev_);\n> > +\tdrmRequest->addProperty(plane_, \"FB_ID\", drmBuffer->id());\n> > +\n> > +\tif (!planeInitialized_) {\n> > +\t\tdrmRequest->addProperty(plane_, \"CRTC_ID\", crtc_->id());\n> > +\t\tdrmRequest->addProperty(plane_, \"SRC_X\", 0 << 16);\n> > +\t\tdrmRequest->addProperty(plane_, \"SRC_Y\", 0 << 16);\n> > +\t\tdrmRequest->addProperty(plane_, \"SRC_W\", mode_->hdisplay << 16);\n> > +\t\tdrmRequest->addProperty(plane_, \"SRC_H\", mode_->vdisplay << 16);\n> > +\t\tdrmRequest->addProperty(plane_, \"CRTC_X\", 0);\n> > +\t\tdrmRequest->addProperty(plane_, \"CRTC_Y\", 0);\n> > +\t\tdrmRequest->addProperty(plane_, \"CRTC_W\", mode_->hdisplay);\n> > +\t\tdrmRequest->addProperty(plane_, \"CRTC_H\", mode_->vdisplay);\n> > +\t\tplaneInitialized_ = true;\n> > +\t}\n> > +\n> > +\tpending_ = std::make_unique(drmRequest, camRequest);\n> \n> I wonder if we should optimise this so we don't have to malloc a new\n> object each time - but that could be ignored here.\n> \n> If we had a means to 'set' the drmRequest and camRequest, we could have\n> a fourth st::unique_pointer to move the latest completed\n> request to - and re-use it - but we're not worried about\n\nWe would also have to reuse the DRM::AtomicRequest. If we wanted to go\nfor zero dynamic allocations, it would be quite interesting, even adding\na property to the request will allocate memory internally in libdrm.\n\n> > +\n> > +\tstd::lock_guard lock(lock_);\n> > +\n> > +\tif (!queued_) {\n> > +\t\tint ret = drmRequest->commit(DRM::AtomicRequest::FlagAsync);\n> > +\t\tif (ret < 0)\n> > +\t\t\tstd::cerr\n> > +\t\t\t\t<< \"Failed to commit atomic request: \"\n> > +\t\t\t\t<< strerror(-ret) << std::endl;\n> > +\t\tqueued_ = std::move(pending_);\n> \n> Is this right? If we failed to queue it - should we mark it as queued?\n> Or is this just to manage the lifecycle of the allocation...\n> \n> If it's intentional - it deserves a comment to express that I think.\n> \n> I suspect it is required to release pending_ so that we can let the next\n> request in regardless...\n\nIt's intentional in the sense that I have intentionally not implemented\nerror recovery :-) Errors should really not happen here, they would\nlikely be fatal. I'll add a coment.\n\n> > +\t}\n> > +\n> > +\treturn false;\n> > +}\n> > +\n> > +void KMSSink::requestComplete(DRM::AtomicRequest *request)\n> > +{\n> > +\tstd::lock_guard lock(lock_);\n> > +\n> > +\tassert(queued_ && queued_->drmRequest_.get() == request);\n> > +\n> > +\t/* Complete the active request, if any. */\n> > +\tif (active_)\n> > +\t\trequestProcessed.emit(active_->camRequest_);\n> > +\n> > +\t/* The queued request becomes active. */\n> > +\tactive_ = std::move(queued_);\n> \n> Is the request guaranteed to have been queued to the hardware at this\n> point? Is there any error handling - such as some state to track or\n> report in the AtomicRequest?\n\nThere's no error reporting mechanism I'm aware of, atomic commits are\nnot supposed to fail once they're queued.\n\n> Either way, even if that failed, the active_ would be released back to\n> the requestProcessed signal on the next iteration anyway... so the\n> 'pipeline' here will be fine as far as I can see.\n> \n> \n> Looks like my comments are only about adding comments so ...\n> \n> Reviewed-by: Kieran Bingham \n> \n> > +\n> > +\t/* Queue the pending request, if any. */\n> > +\tif (pending_) {\n> > +\t\tpending_->drmRequest_->commit(DRM::AtomicRequest::FlagAsync);\n> > +\t\tqueued_ = std::move(pending_);\n> > +\t}\n> > +}\n> > diff --git a/src/cam/kms_sink.h b/src/cam/kms_sink.h\n> > new file mode 100644\n> > index 000000000000..2895e00f84a1\n> > --- /dev/null\n> > +++ b/src/cam/kms_sink.h\n> > @@ -0,0 +1,76 @@\n> > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > +/*\n> > + * Copyright (C) 2021, Ideas on Board Oy\n> > + *\n> > + * kms_sink.h - KMS Sink\n> > + */\n> > +#ifndef __CAM_KMS_SINK_H__\n> > +#define __CAM_KMS_SINK_H__\n> > +\n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +\n> > +#include \n> > +\n> > +#include \n> > +#include \n> > +\n> > +#include \"drm.h\"\n> > +#include \"frame_sink.h\"\n> > +\n> > +class KMSSink : public FrameSink\n> > +{\n> > +public:\n> > +\tKMSSink(const std::string &connectorName);\n> > +\n> > +\tbool isValid() const { return connector_ != nullptr; }\n> > +\n> > +\tvoid mapBuffer(libcamera::FrameBuffer *buffer) override;\n> > +\n> > +\tint configure(const libcamera::CameraConfiguration &config) override;\n> > +\tint start() override;\n> > +\tint stop() override;\n> > +\n> > +\tbool processRequest(libcamera::Request *request) override;\n> > +\n> > +private:\n> > +\tclass Request\n> > +\t{\n> > +\tpublic:\n> > +\t\tRequest(DRM::AtomicRequest *drmRequest, libcamera::Request *camRequest)\n> > +\t\t\t: drmRequest_(drmRequest), camRequest_(camRequest)\n> > +\t\t{\n> > +\t\t}\n> > +\n> > +\t\tstd::unique_ptr drmRequest_;\n> > +\t\tlibcamera::Request *camRequest_;\n> > +\t};\n> > +\n> > +\tint configurePipeline(const libcamera::PixelFormat &format);\n> > +\tvoid requestComplete(DRM::AtomicRequest *request);\n> > +\n> > +\tDRM::Device dev_;\n> > +\n> > +\tconst DRM::Connector *connector_;\n> > +\tconst DRM::Crtc *crtc_;\n> > +\tconst DRM::Plane *plane_;\n> > +\tconst DRM::Mode *mode_;\n> > +\n> > +\tlibcamera::PixelFormat format_;\n> > +\tlibcamera::Size size_;\n> > +\tunsigned int stride_;\n> > +\n> > +\tbool planeInitialized_;\n> > +\n> > +\tstd::map> buffers_;\n> > +\n> > +\tstd::mutex lock_;\n> > +\tstd::unique_ptr pending_;\n> > +\tstd::unique_ptr queued_;\n> > +\tstd::unique_ptr active_;> +};\n> > +\n> > +#endif /* __CAM_KMS_SINK_H__ */\n> > diff --git a/src/cam/meson.build b/src/cam/meson.build\n> > index b47add55b0cb..ea36aaa5c514 100644\n> > --- a/src/cam/meson.build\n> > +++ b/src/cam/meson.build\n> > @@ -27,6 +27,7 @@ if libdrm.found()\n> > cam_cpp_args += [ '-DHAVE_KMS' ]\n> > cam_sources += files([\n> > 'drm.cpp',\n> > + 'kms_sink.cpp'\n> > ])\n> > endif\n> >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id B77E7C3235\n\tfor ;\n\tThu, 5 Aug 2021 13:06:53 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B8DEA68811;\n\tThu, 5 Aug 2021 15:06:52 +0200 (CEST)","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 5CAC06026D\n\tfor ;\n\tThu, 5 Aug 2021 15:06:51 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id C6F6951D;\n\tThu, 5 Aug 2021 15:06:50 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"ol6jDMb/\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1628168811;\n\tbh=4X/LK3OAtE16Cz7QXSNgX1ch0oRgs+1b19S7X2prTkI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=ol6jDMb/vR9wvpOy+wWcO2x3Vv2jkbAsIAr2Kf/uPRJ1+oXinYgS1rqmfMeFcz32w\n\tXoLWg8pEYuetaUyR5KsuoyTgbeKl8fHSh97t082QWmy8pVx87Ka5zkVNKyDxdACXzt\n\tEc6czo21urLly7xb8lAb33RJWOkShnh+yUWAKP4w=","Date":"Thu, 5 Aug 2021 16:06:38 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Message-ID":"","References":"<20210804124314.8044-1-laurent.pinchart@ideasonboard.com>\n\t<20210804124314.8044-7-laurent.pinchart@ideasonboard.com>\n\t<3396c55d-bf29-7b8a-5e34-07bdc18a064f@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<3396c55d-bf29-7b8a-5e34-07bdc18a064f@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v3 6/8] cam: Add KMS sink class","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]