[libcamera-devel,v3,6/8] cam: Add KMS sink class

13200 diff mbox series

Add a KMSSink class to display framebuffers through the DRM/KMS API.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
Changes since v2:

- Simplify connector selection logic, and fix it when a connector name
  is specified
- Fail configure() if no connector is selected
- Update copyright years

Changes since v1:

- Use formats:: namespace

fixup! cam: Add KMS sink class
---
 src/cam/kms_sink.cpp | 306 +++++++++++++++++++++++++++++++++++++++++++
 src/cam/kms_sink.h   |  76 +++++++++++
 src/cam/meson.build  |   1 +
 3 files changed, 383 insertions(+)
 create mode 100644 src/cam/kms_sink.cpp
 create mode 100644 src/cam/kms_sink.h

Hi Laurent,

On Wed, Aug 04, 2021 at 03:43:12PM +0300, Laurent Pinchart wrote:
> Add a KMSSink class to display framebuffers through the DRM/KMS API.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Looks good.

Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>

> ---
> Changes since v2:
> 
> - Simplify connector selection logic, and fix it when a connector name
>   is specified
> - Fail configure() if no connector is selected
> - Update copyright years
> 
> Changes since v1:
> 
> - Use formats:: namespace
> 
> fixup! cam: Add KMS sink class
> ---
>  src/cam/kms_sink.cpp | 306 +++++++++++++++++++++++++++++++++++++++++++
>  src/cam/kms_sink.h   |  76 +++++++++++
>  src/cam/meson.build  |   1 +
>  3 files changed, 383 insertions(+)
>  create mode 100644 src/cam/kms_sink.cpp
>  create mode 100644 src/cam/kms_sink.h
> 
> diff --git a/src/cam/kms_sink.cpp b/src/cam/kms_sink.cpp
> new file mode 100644
> index 000000000000..f708b613f226
> --- /dev/null
> +++ b/src/cam/kms_sink.cpp
> @@ -0,0 +1,306 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Copyright (C) 2021, Ideas on Board Oy
> + *
> + * kms_sink.cpp - KMS Sink
> + */
> +
> +#include "kms_sink.h"
> +
> +#include <algorithm>
> +#include <assert.h>
> +#include <iostream>
> +#include <memory>
> +#include <string.h>
> +
> +#include <libcamera/camera.h>
> +#include <libcamera/formats.h>
> +#include <libcamera/framebuffer.h>
> +#include <libcamera/stream.h>
> +
> +#include "drm.h"
> +
> +KMSSink::KMSSink(const std::string &connectorName)
> +	: connector_(nullptr), crtc_(nullptr), plane_(nullptr), mode_(nullptr)
> +{
> +	int ret = dev_.init();
> +	if (ret < 0)
> +		return;
> +
> +	/*
> +	 * Find the requested connector. If no specific connector is requested,
> +	 * pick the first connected connector or, if no connector is connected,
> +	 * the first connector with unknown status.
> +	 */
> +	for (const DRM::Connector &conn : dev_.connectors()) {
> +		if (!connectorName.empty()) {
> +			if (conn.name() != connectorName)
> +				continue;
> +
> +			connector_ = &conn;
> +			break;
> +		}
> +
> +		if (conn.status() == DRM::Connector::Connected) {
> +			connector_ = &conn;
> +			break;
> +		}
> +
> +		if (!connector_ && conn.status() == DRM::Connector::Unknown)
> +			connector_ = &conn;
> +	}
> +
> +	if (!connector_) {
> +		if (!connectorName.empty())
> +			std::cerr
> +				<< "Connector " << connectorName << " not found"
> +				<< std::endl;
> +		else
> +			std::cerr << "No connected connector found" << std::endl;
> +		return;
> +	}
> +
> +	dev_.requestComplete.connect(this, &KMSSink::requestComplete);
> +}
> +
> +void KMSSink::mapBuffer(libcamera::FrameBuffer *buffer)
> +{
> +	std::unique_ptr<DRM::FrameBuffer> drmBuffer =
> +		dev_.createFrameBuffer(*buffer, format_, size_, stride_);
> +	if (!drmBuffer)
> +		return;
> +
> +	buffers_.emplace(std::piecewise_construct,
> +			 std::forward_as_tuple(buffer),
> +			 std::forward_as_tuple(std::move(drmBuffer)));
> +}
> +
> +int KMSSink::configure(const libcamera::CameraConfiguration &config)
> +{
> +	if (!connector_)
> +		return -EINVAL;
> +
> +	crtc_ = nullptr;
> +	plane_ = nullptr;
> +	mode_ = nullptr;
> +
> +	const libcamera::StreamConfiguration &cfg = config.at(0);
> +	int ret = configurePipeline(cfg.pixelFormat);
> +	if (ret < 0)
> +		return ret;
> +
> +	const std::vector<DRM::Mode> &modes = connector_->modes();
> +	const auto iter = std::find_if(modes.begin(), modes.end(),
> +				       [&](const DRM::Mode &mode) {
> +					       return mode.hdisplay == cfg.size.width &&
> +						      mode.vdisplay == cfg.size.height;
> +				       });
> +	if (iter == modes.end()) {
> +		std::cerr
> +			<< "No mode matching " << cfg.size.toString()
> +			<< std::endl;
> +		return -EINVAL;
> +	}
> +
> +	mode_ = &*iter;
> +	size_ = cfg.size;
> +	stride_ = cfg.stride;
> +
> +	return 0;
> +}
> +
> +int KMSSink::configurePipeline(const libcamera::PixelFormat &format)
> +{
> +	/*
> +	 * If the requested format has an alpha channel, also consider the X
> +	 * variant.
> +	 */
> +	libcamera::PixelFormat xFormat;
> +
> +	switch (format) {
> +	case libcamera::formats::ABGR8888:
> +		xFormat = libcamera::formats::XBGR8888;
> +		break;
> +	case libcamera::formats::ARGB8888:
> +		xFormat = libcamera::formats::XRGB8888;
> +		break;
> +	case libcamera::formats::BGRA8888:
> +		xFormat = libcamera::formats::BGRX8888;
> +		break;
> +	case libcamera::formats::RGBA8888:
> +		xFormat = libcamera::formats::RGBX8888;
> +		break;
> +	}
> +
> +	/*
> +	 * Find a CRTC and plane suitable for the request format and the
> +	 * connector at the end of the pipeline. Restrict the search to primary
> +	 * planes for now.
> +	 */
> +	for (const DRM::Encoder *encoder : connector_->encoders()) {
> +		for (const DRM::Crtc *crtc : encoder->possibleCrtcs()) {
> +			for (const DRM::Plane *plane : crtc->planes()) {
> +				if (plane->type() != DRM::Plane::TypePrimary)
> +					continue;
> +
> +				if (plane->supportsFormat(format)) {
> +					crtc_ = crtc;
> +					plane_ = plane;
> +					format_ = format;
> +					return 0;
> +				}
> +
> +				if (plane->supportsFormat(xFormat)) {
> +					crtc_ = crtc;
> +					plane_ = plane;
> +					format_ = xFormat;
> +					return 0;
> +				}
> +			}
> +		}
> +	}
> +
> +	std::cerr
> +		<< "Unable to find display pipeline for format "
> +		<< format.toString() << std::endl;
> +	return -EPIPE;
> +}
> +
> +int KMSSink::start()
> +{
> +	std::unique_ptr<DRM::AtomicRequest> request;
> +
> +	int ret = FrameSink::start();
> +	if (ret < 0)
> +		return ret;
> +
> +	/* Disable all CRTCs and planes to start from a known valid state. */
> +	request = std::make_unique<DRM::AtomicRequest>(&dev_);
> +
> +	for (const DRM::Crtc &crtc : dev_.crtcs())
> +		request->addProperty(&crtc, "ACTIVE", 0);
> +
> +	for (const DRM::Plane &plane : dev_.planes()) {
> +		request->addProperty(&plane, "CRTC_ID", 0);
> +		request->addProperty(&plane, "FB_ID", 0);
> +	}
> +
> +	ret = request->commit(DRM::AtomicRequest::FlagAllowModeset);
> +	if (ret < 0) {
> +		std::cerr
> +			<< "Failed to disable CRTCs and planes: "
> +			<< strerror(-ret) << std::endl;
> +		return ret;
> +	}
> +
> +	/* Enable the display pipeline with no plane to start with. */
> +	request = std::make_unique<DRM::AtomicRequest>(&dev_);
> +
> +	request->addProperty(connector_, "CRTC_ID", crtc_->id());
> +	request->addProperty(crtc_, "ACTIVE", 1);
> +	request->addProperty(crtc_, "MODE_ID", mode_->toBlob(&dev_));
> +
> +	ret = request->commit(DRM::AtomicRequest::FlagAllowModeset);
> +	if (ret < 0) {
> +		std::cerr
> +			<< "Failed to enable display pipeline: "
> +			<< strerror(-ret) << std::endl;
> +		return ret;
> +	}
> +
> +	planeInitialized_ = false;
> +
> +	return 0;
> +}
> +
> +int KMSSink::stop()
> +{
> +	/* Display pipeline. */
> +	DRM::AtomicRequest request(&dev_);
> +
> +	request.addProperty(connector_, "CRTC_ID", 0);
> +	request.addProperty(crtc_, "ACTIVE", 0);
> +	request.addProperty(crtc_, "MODE_ID", 0);
> +	request.addProperty(plane_, "CRTC_ID", 0);
> +	request.addProperty(plane_, "FB_ID", 0);
> +
> +	int ret = request.commit(DRM::AtomicRequest::FlagAllowModeset);
> +	if (ret < 0) {
> +		std::cerr
> +			<< "Failed to stop display pipeline: "
> +			<< strerror(-ret) << std::endl;
> +		return ret;
> +	}
> +
> +	/* Free all buffers. */
> +	pending_.reset();
> +	queued_.reset();
> +	active_.reset();
> +	buffers_.clear();
> +
> +	return FrameSink::stop();
> +}
> +
> +bool KMSSink::processRequest(libcamera::Request *camRequest)
> +{
> +	if (pending_)
> +		return true;
> +
> +	libcamera::FrameBuffer *buffer = camRequest->buffers().begin()->second;
> +	auto iter = buffers_.find(buffer);
> +	if (iter == buffers_.end())
> +		return true;
> +
> +	DRM::FrameBuffer *drmBuffer = iter->second.get();
> +
> +	DRM::AtomicRequest *drmRequest = new DRM::AtomicRequest(&dev_);
> +	drmRequest->addProperty(plane_, "FB_ID", drmBuffer->id());
> +
> +	if (!planeInitialized_) {
> +		drmRequest->addProperty(plane_, "CRTC_ID", crtc_->id());
> +		drmRequest->addProperty(plane_, "SRC_X", 0 << 16);
> +		drmRequest->addProperty(plane_, "SRC_Y", 0 << 16);
> +		drmRequest->addProperty(plane_, "SRC_W", mode_->hdisplay << 16);
> +		drmRequest->addProperty(plane_, "SRC_H", mode_->vdisplay << 16);
> +		drmRequest->addProperty(plane_, "CRTC_X", 0);
> +		drmRequest->addProperty(plane_, "CRTC_Y", 0);
> +		drmRequest->addProperty(plane_, "CRTC_W", mode_->hdisplay);
> +		drmRequest->addProperty(plane_, "CRTC_H", mode_->vdisplay);
> +		planeInitialized_ = true;
> +	}
> +
> +	pending_ = std::make_unique<Request>(drmRequest, camRequest);
> +
> +	std::lock_guard<std::mutex> lock(lock_);
> +
> +	if (!queued_) {
> +		int ret = drmRequest->commit(DRM::AtomicRequest::FlagAsync);
> +		if (ret < 0)
> +			std::cerr
> +				<< "Failed to commit atomic request: "
> +				<< strerror(-ret) << std::endl;
> +		queued_ = std::move(pending_);
> +	}
> +
> +	return false;
> +}
> +
> +void KMSSink::requestComplete(DRM::AtomicRequest *request)
> +{
> +	std::lock_guard<std::mutex> lock(lock_);
> +
> +	assert(queued_ && queued_->drmRequest_.get() == request);
> +
> +	/* Complete the active request, if any. */
> +	if (active_)
> +		requestProcessed.emit(active_->camRequest_);
> +
> +	/* The queued request becomes active. */
> +	active_ = std::move(queued_);
> +
> +	/* Queue the pending request, if any. */
> +	if (pending_) {
> +		pending_->drmRequest_->commit(DRM::AtomicRequest::FlagAsync);
> +		queued_ = std::move(pending_);
> +	}
> +}
> diff --git a/src/cam/kms_sink.h b/src/cam/kms_sink.h
> new file mode 100644
> index 000000000000..2895e00f84a1
> --- /dev/null
> +++ b/src/cam/kms_sink.h
> @@ -0,0 +1,76 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Copyright (C) 2021, Ideas on Board Oy
> + *
> + * kms_sink.h - KMS Sink
> + */
> +#ifndef __CAM_KMS_SINK_H__
> +#define __CAM_KMS_SINK_H__
> +
> +#include <list>
> +#include <memory>
> +#include <mutex>
> +#include <string>
> +#include <utility>
> +
> +#include <libcamera/base/signal.h>
> +
> +#include <libcamera/geometry.h>
> +#include <libcamera/pixel_format.h>
> +
> +#include "drm.h"
> +#include "frame_sink.h"
> +
> +class KMSSink : public FrameSink
> +{
> +public:
> +	KMSSink(const std::string &connectorName);
> +
> +	bool isValid() const { return connector_ != nullptr; }
> +
> +	void mapBuffer(libcamera::FrameBuffer *buffer) override;
> +
> +	int configure(const libcamera::CameraConfiguration &config) override;
> +	int start() override;
> +	int stop() override;
> +
> +	bool processRequest(libcamera::Request *request) override;
> +
> +private:
> +	class Request
> +	{
> +	public:
> +		Request(DRM::AtomicRequest *drmRequest, libcamera::Request *camRequest)
> +			: drmRequest_(drmRequest), camRequest_(camRequest)
> +		{
> +		}
> +
> +		std::unique_ptr<DRM::AtomicRequest> drmRequest_;
> +		libcamera::Request *camRequest_;
> +	};
> +
> +	int configurePipeline(const libcamera::PixelFormat &format);
> +	void requestComplete(DRM::AtomicRequest *request);
> +
> +	DRM::Device dev_;
> +
> +	const DRM::Connector *connector_;
> +	const DRM::Crtc *crtc_;
> +	const DRM::Plane *plane_;
> +	const DRM::Mode *mode_;
> +
> +	libcamera::PixelFormat format_;
> +	libcamera::Size size_;
> +	unsigned int stride_;
> +
> +	bool planeInitialized_;
> +
> +	std::map<libcamera::FrameBuffer *, std::unique_ptr<DRM::FrameBuffer>> buffers_;
> +
> +	std::mutex lock_;
> +	std::unique_ptr<Request> pending_;
> +	std::unique_ptr<Request> queued_;
> +	std::unique_ptr<Request> active_;
> +};
> +
> +#endif /* __CAM_KMS_SINK_H__ */
> diff --git a/src/cam/meson.build b/src/cam/meson.build
> index b47add55b0cb..ea36aaa5c514 100644
> --- a/src/cam/meson.build
> +++ b/src/cam/meson.build
> @@ -27,6 +27,7 @@ if libdrm.found()
>  cam_cpp_args += [ '-DHAVE_KMS' ]
>  cam_sources += files([
>      'drm.cpp',
> +    'kms_sink.cpp'
>  ])
>  endif
>  
> -- 
> Regards,
> 
> Laurent Pinchart
>

Hi Laurent,

Thank you for the patch

On 8/4/21 6:13 PM, Laurent Pinchart wrote:
> Add a KMSSink class to display framebuffers through the DRM/KMS API.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> Changes since v2:
>
> - Simplify connector selection logic, and fix it when a connector name
>    is specified
> - Fail configure() if no connector is selected
> - Update copyright years
>
> Changes since v1:
>
> - Use formats:: namespace
>
> fixup! cam: Add KMS sink class
> ---
>   src/cam/kms_sink.cpp | 306 +++++++++++++++++++++++++++++++++++++++++++
>   src/cam/kms_sink.h   |  76 +++++++++++
>   src/cam/meson.build  |   1 +
>   3 files changed, 383 insertions(+)
>   create mode 100644 src/cam/kms_sink.cpp
>   create mode 100644 src/cam/kms_sink.h
>
> diff --git a/src/cam/kms_sink.cpp b/src/cam/kms_sink.cpp
> new file mode 100644
> index 000000000000..f708b613f226
> --- /dev/null
> +++ b/src/cam/kms_sink.cpp
> @@ -0,0 +1,306 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Copyright (C) 2021, Ideas on Board Oy
> + *
> + * kms_sink.cpp - KMS Sink
> + */
> +
> +#include "kms_sink.h"
> +
> +#include <algorithm>
> +#include <assert.h>
> +#include <iostream>
> +#include <memory>
> +#include <string.h>
> +
> +#include <libcamera/camera.h>
> +#include <libcamera/formats.h>
> +#include <libcamera/framebuffer.h>
> +#include <libcamera/stream.h>
> +
> +#include "drm.h"
> +
> +KMSSink::KMSSink(const std::string &connectorName)
> +	: connector_(nullptr), crtc_(nullptr), plane_(nullptr), mode_(nullptr)
> +{
> +	int ret = dev_.init();
> +	if (ret < 0)
> +		return;
> +
> +	/*
> +	 * Find the requested connector. If no specific connector is requested,
> +	 * pick the first connected connector or, if no connector is connected,
> +	 * the first connector with unknown status.
> +	 */
> +	for (const DRM::Connector &conn : dev_.connectors()) {
> +		if (!connectorName.empty()) {
> +			if (conn.name() != connectorName)
> +				continue;
> +
> +			connector_ = &conn;
> +			break;
> +		}
> +
> +		if (conn.status() == DRM::Connector::Connected) {
> +			connector_ = &conn;
> +			break;
> +		}
> +
> +		if (!connector_ && conn.status() == DRM::Connector::Unknown)
> +			connector_ = &conn;
> +	}
> +
> +	if (!connector_) {
> +		if (!connectorName.empty())
> +			std::cerr
> +				<< "Connector " << connectorName << " not found"
> +				<< std::endl;
> +		else
> +			std::cerr << "No connected connector found" << std::endl;
> +		return;
> +	}
> +
> +	dev_.requestComplete.connect(this, &KMSSink::requestComplete);
> +}
> +
> +void KMSSink::mapBuffer(libcamera::FrameBuffer *buffer)
> +{
> +	std::unique_ptr<DRM::FrameBuffer> drmBuffer =
> +		dev_.createFrameBuffer(*buffer, format_, size_, stride_);
> +	if (!drmBuffer)
> +		return;
> +
> +	buffers_.emplace(std::piecewise_construct,
> +			 std::forward_as_tuple(buffer),
> +			 std::forward_as_tuple(std::move(drmBuffer)));
> +}
> +
> +int KMSSink::configure(const libcamera::CameraConfiguration &config)
> +{
> +	if (!connector_)
> +		return -EINVAL;
I assumed there isValid() for a reason, an alternative would be use to 
that. But this is fine as well probably.
> +
> +	crtc_ = nullptr;
> +	plane_ = nullptr;
> +	mode_ = nullptr;
> +
> +	const libcamera::StreamConfiguration &cfg = config.at(0);
> +	int ret = configurePipeline(cfg.pixelFormat);

Should we be finding a compatiable connector's mode (as done below) 
before configurePipeline() ? Does not seem logical to error out on 
incompatiable mode after we have configured the pipeline with possible 
crtc  and encoder.

(But I maybe wrong here, there might be some implicit operational 
sequence between these two, that I am missing)

> +	if (ret < 0)
> +		return ret;
> +
> +	const std::vector<DRM::Mode> &modes = connector_->modes();
> +	const auto iter = std::find_if(modes.begin(), modes.end(),
> +				       [&](const DRM::Mode &mode) {
> +					       return mode.hdisplay == cfg.size.width &&
> +						      mode.vdisplay == cfg.size.height;
> +				       });
> +	if (iter == modes.end()) {
> +		std::cerr
> +			<< "No mode matching " << cfg.size.toString()
> +			<< std::endl;
> +		return -EINVAL;
> +	}
> +
> +	mode_ = &*iter;
> +	size_ = cfg.size;
> +	stride_ = cfg.stride;
> +
> +	return 0;
> +}
> +
> +int KMSSink::configurePipeline(const libcamera::PixelFormat &format)
> +{
> +	/*
> +	 * If the requested format has an alpha channel, also consider the X
> +	 * variant.
> +	 */
> +	libcamera::PixelFormat xFormat;
> +
> +	switch (format) {
> +	case libcamera::formats::ABGR8888:
> +		xFormat = libcamera::formats::XBGR8888;
> +		break;
> +	case libcamera::formats::ARGB8888:
> +		xFormat = libcamera::formats::XRGB8888;
> +		break;
> +	case libcamera::formats::BGRA8888:
> +		xFormat = libcamera::formats::BGRX8888;
> +		break;
> +	case libcamera::formats::RGBA8888:
> +		xFormat = libcamera::formats::RGBX8888;
> +		break;
> +	}
> +
> +	/*
> +	 * Find a CRTC and plane suitable for the request format and the
> +	 * connector at the end of the pipeline. Restrict the search to primary
> +	 * planes for now.
> +	 */
> +	for (const DRM::Encoder *encoder : connector_->encoders()) {
> +		for (const DRM::Crtc *crtc : encoder->possibleCrtcs()) {
> +			for (const DRM::Plane *plane : crtc->planes()) {
> +				if (plane->type() != DRM::Plane::TypePrimary)
> +					continue;
> +
> +				if (plane->supportsFormat(format)) {
> +					crtc_ = crtc;
> +					plane_ = plane;
> +					format_ = format;
> +					return 0;
> +				}
> +
> +				if (plane->supportsFormat(xFormat)) {
> +					crtc_ = crtc;
> +					plane_ = plane;
> +					format_ = xFormat;
> +					return 0;
> +				}
> +			}
> +		}
> +	}
> +
> +	std::cerr
> +		<< "Unable to find display pipeline for format "
> +		<< format.toString() << std::endl;
> +	return -EPIPE;
> +}
> +
> +int KMSSink::start()
> +{
> +	std::unique_ptr<DRM::AtomicRequest> request;
> +
> +	int ret = FrameSink::start();
> +	if (ret < 0)
> +		return ret;
> +
> +	/* Disable all CRTCs and planes to start from a known valid state. */
> +	request = std::make_unique<DRM::AtomicRequest>(&dev_);
> +
> +	for (const DRM::Crtc &crtc : dev_.crtcs())
> +		request->addProperty(&crtc, "ACTIVE", 0);
> +
> +	for (const DRM::Plane &plane : dev_.planes()) {
> +		request->addProperty(&plane, "CRTC_ID", 0);
> +		request->addProperty(&plane, "FB_ID", 0);
> +	}
> +
> +	ret = request->commit(DRM::AtomicRequest::FlagAllowModeset);
> +	if (ret < 0) {
> +		std::cerr
> +			<< "Failed to disable CRTCs and planes: "
> +			<< strerror(-ret) << std::endl;
> +		return ret;
> +	}
> +
> +	/* Enable the display pipeline with no plane to start with. */
> +	request = std::make_unique<DRM::AtomicRequest>(&dev_);
> +
> +	request->addProperty(connector_, "CRTC_ID", crtc_->id());
> +	request->addProperty(crtc_, "ACTIVE", 1);
> +	request->addProperty(crtc_, "MODE_ID", mode_->toBlob(&dev_));
> +
> +	ret = request->commit(DRM::AtomicRequest::FlagAllowModeset);
> +	if (ret < 0) {
> +		std::cerr
> +			<< "Failed to enable display pipeline: "
> +			<< strerror(-ret) << std::endl;
> +		return ret;
> +	}
> +
> +	planeInitialized_ = false;
> +
> +	return 0;
> +}
> +
> +int KMSSink::stop()
> +{
> +	/* Display pipeline. */
> +	DRM::AtomicRequest request(&dev_);
> +
> +	request.addProperty(connector_, "CRTC_ID", 0);
> +	request.addProperty(crtc_, "ACTIVE", 0);
> +	request.addProperty(crtc_, "MODE_ID", 0);
> +	request.addProperty(plane_, "CRTC_ID", 0);
> +	request.addProperty(plane_, "FB_ID", 0);
> +
> +	int ret = request.commit(DRM::AtomicRequest::FlagAllowModeset);
> +	if (ret < 0) {
> +		std::cerr
> +			<< "Failed to stop display pipeline: "
> +			<< strerror(-ret) << std::endl;
> +		return ret;
> +	}
> +
> +	/* Free all buffers. */
> +	pending_.reset();
> +	queued_.reset();
> +	active_.reset();
> +	buffers_.clear();
> +
> +	return FrameSink::stop();
> +}
> +
> +bool KMSSink::processRequest(libcamera::Request *camRequest)
> +{
> +	if (pending_)
> +		return true;
> +
> +	libcamera::FrameBuffer *buffer = camRequest->buffers().begin()->second;
> +	auto iter = buffers_.find(buffer);
> +	if (iter == buffers_.end())
> +		return true;
> +
> +	DRM::FrameBuffer *drmBuffer = iter->second.get();
> +
> +	DRM::AtomicRequest *drmRequest = new DRM::AtomicRequest(&dev_);
> +	drmRequest->addProperty(plane_, "FB_ID", drmBuffer->id());
> +
> +	if (!planeInitialized_) {
> +		drmRequest->addProperty(plane_, "CRTC_ID", crtc_->id());
> +		drmRequest->addProperty(plane_, "SRC_X", 0 << 16);
> +		drmRequest->addProperty(plane_, "SRC_Y", 0 << 16);
> +		drmRequest->addProperty(plane_, "SRC_W", mode_->hdisplay << 16);
> +		drmRequest->addProperty(plane_, "SRC_H", mode_->vdisplay << 16);
> +		drmRequest->addProperty(plane_, "CRTC_X", 0);
> +		drmRequest->addProperty(plane_, "CRTC_Y", 0);
> +		drmRequest->addProperty(plane_, "CRTC_W", mode_->hdisplay);
> +		drmRequest->addProperty(plane_, "CRTC_H", mode_->vdisplay);
> +		planeInitialized_ = true;
> +	}
> +
> +	pending_ = std::make_unique<Request>(drmRequest, camRequest);

This is neat! :)

Looks good to me with my limited understanding I gained this morning on 
the subject:

Reviewed-by: Umang Jain <umang.jain@ideasonboard.com>

> +
> +	std::lock_guard<std::mutex> lock(lock_);
> +
> +	if (!queued_) {
> +		int ret = drmRequest->commit(DRM::AtomicRequest::FlagAsync);
> +		if (ret < 0)
> +			std::cerr
> +				<< "Failed to commit atomic request: "
> +				<< strerror(-ret) << std::endl;
> +		queued_ = std::move(pending_);
> +	}
> +
> +	return false;
> +}
> +
> +void KMSSink::requestComplete(DRM::AtomicRequest *request)
> +{
> +	std::lock_guard<std::mutex> lock(lock_);
> +
> +	assert(queued_ && queued_->drmRequest_.get() == request);
> +
> +	/* Complete the active request, if any. */
> +	if (active_)
> +		requestProcessed.emit(active_->camRequest_);
> +
> +	/* The queued request becomes active. */
> +	active_ = std::move(queued_);
> +
> +	/* Queue the pending request, if any. */
> +	if (pending_) {
> +		pending_->drmRequest_->commit(DRM::AtomicRequest::FlagAsync);
> +		queued_ = std::move(pending_);
> +	}
> +}
> diff --git a/src/cam/kms_sink.h b/src/cam/kms_sink.h
> new file mode 100644
> index 000000000000..2895e00f84a1
> --- /dev/null
> +++ b/src/cam/kms_sink.h
> @@ -0,0 +1,76 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Copyright (C) 2021, Ideas on Board Oy
> + *
> + * kms_sink.h - KMS Sink
> + */
> +#ifndef __CAM_KMS_SINK_H__
> +#define __CAM_KMS_SINK_H__
> +
> +#include <list>
> +#include <memory>
> +#include <mutex>
> +#include <string>
> +#include <utility>
> +
> +#include <libcamera/base/signal.h>
> +
> +#include <libcamera/geometry.h>
> +#include <libcamera/pixel_format.h>
> +
> +#include "drm.h"
> +#include "frame_sink.h"
> +
> +class KMSSink : public FrameSink
> +{
> +public:
> +	KMSSink(const std::string &connectorName);
> +
> +	bool isValid() const { return connector_ != nullptr; }
> +
> +	void mapBuffer(libcamera::FrameBuffer *buffer) override;
> +
> +	int configure(const libcamera::CameraConfiguration &config) override;
> +	int start() override;
> +	int stop() override;
> +
> +	bool processRequest(libcamera::Request *request) override;
> +
> +private:
> +	class Request
> +	{
> +	public:
> +		Request(DRM::AtomicRequest *drmRequest, libcamera::Request *camRequest)
> +			: drmRequest_(drmRequest), camRequest_(camRequest)
> +		{
> +		}
> +
> +		std::unique_ptr<DRM::AtomicRequest> drmRequest_;
> +		libcamera::Request *camRequest_;
> +	};
> +
> +	int configurePipeline(const libcamera::PixelFormat &format);
> +	void requestComplete(DRM::AtomicRequest *request);
> +
> +	DRM::Device dev_;
> +
> +	const DRM::Connector *connector_;
> +	const DRM::Crtc *crtc_;
> +	const DRM::Plane *plane_;
> +	const DRM::Mode *mode_;
> +
> +	libcamera::PixelFormat format_;
> +	libcamera::Size size_;
> +	unsigned int stride_;
> +
> +	bool planeInitialized_;
> +
> +	std::map<libcamera::FrameBuffer *, std::unique_ptr<DRM::FrameBuffer>> buffers_;
> +
> +	std::mutex lock_;
> +	std::unique_ptr<Request> pending_;
> +	std::unique_ptr<Request> queued_;
> +	std::unique_ptr<Request> active_;
> +};
> +
> +#endif /* __CAM_KMS_SINK_H__ */
> diff --git a/src/cam/meson.build b/src/cam/meson.build
> index b47add55b0cb..ea36aaa5c514 100644
> --- a/src/cam/meson.build
> +++ b/src/cam/meson.build
> @@ -27,6 +27,7 @@ if libdrm.found()
>   cam_cpp_args += [ '-DHAVE_KMS' ]
>   cam_sources += files([
>       'drm.cpp',
> +    'kms_sink.cpp'
>   ])
>   endif
>

Hi Umang,

On Thu, Aug 05, 2021 at 12:34:06PM +0530, Umang Jain wrote:
> On 8/4/21 6:13 PM, Laurent Pinchart wrote:
> > Add a KMSSink class to display framebuffers through the DRM/KMS API.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> > Changes since v2:
> >
> > - Simplify connector selection logic, and fix it when a connector name
> >    is specified
> > - Fail configure() if no connector is selected
> > - Update copyright years
> >
> > Changes since v1:
> >
> > - Use formats:: namespace
> >
> > fixup! cam: Add KMS sink class
> > ---
> >   src/cam/kms_sink.cpp | 306 +++++++++++++++++++++++++++++++++++++++++++
> >   src/cam/kms_sink.h   |  76 +++++++++++
> >   src/cam/meson.build  |   1 +
> >   3 files changed, 383 insertions(+)
> >   create mode 100644 src/cam/kms_sink.cpp
> >   create mode 100644 src/cam/kms_sink.h
> >
> > diff --git a/src/cam/kms_sink.cpp b/src/cam/kms_sink.cpp
> > new file mode 100644
> > index 000000000000..f708b613f226
> > --- /dev/null
> > +++ b/src/cam/kms_sink.cpp
> > @@ -0,0 +1,306 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > +/*
> > + * Copyright (C) 2021, Ideas on Board Oy
> > + *
> > + * kms_sink.cpp - KMS Sink
> > + */
> > +
> > +#include "kms_sink.h"
> > +
> > +#include <algorithm>
> > +#include <assert.h>
> > +#include <iostream>
> > +#include <memory>
> > +#include <string.h>
> > +
> > +#include <libcamera/camera.h>
> > +#include <libcamera/formats.h>
> > +#include <libcamera/framebuffer.h>
> > +#include <libcamera/stream.h>
> > +
> > +#include "drm.h"
> > +
> > +KMSSink::KMSSink(const std::string &connectorName)
> > +	: connector_(nullptr), crtc_(nullptr), plane_(nullptr), mode_(nullptr)
> > +{
> > +	int ret = dev_.init();
> > +	if (ret < 0)
> > +		return;
> > +
> > +	/*
> > +	 * Find the requested connector. If no specific connector is requested,
> > +	 * pick the first connected connector or, if no connector is connected,
> > +	 * the first connector with unknown status.
> > +	 */
> > +	for (const DRM::Connector &conn : dev_.connectors()) {
> > +		if (!connectorName.empty()) {
> > +			if (conn.name() != connectorName)
> > +				continue;
> > +
> > +			connector_ = &conn;
> > +			break;
> > +		}
> > +
> > +		if (conn.status() == DRM::Connector::Connected) {
> > +			connector_ = &conn;
> > +			break;
> > +		}
> > +
> > +		if (!connector_ && conn.status() == DRM::Connector::Unknown)
> > +			connector_ = &conn;
> > +	}
> > +
> > +	if (!connector_) {
> > +		if (!connectorName.empty())
> > +			std::cerr
> > +				<< "Connector " << connectorName << " not found"
> > +				<< std::endl;
> > +		else
> > +			std::cerr << "No connected connector found" << std::endl;
> > +		return;
> > +	}
> > +
> > +	dev_.requestComplete.connect(this, &KMSSink::requestComplete);
> > +}
> > +
> > +void KMSSink::mapBuffer(libcamera::FrameBuffer *buffer)
> > +{
> > +	std::unique_ptr<DRM::FrameBuffer> drmBuffer =
> > +		dev_.createFrameBuffer(*buffer, format_, size_, stride_);
> > +	if (!drmBuffer)
> > +		return;
> > +
> > +	buffers_.emplace(std::piecewise_construct,
> > +			 std::forward_as_tuple(buffer),
> > +			 std::forward_as_tuple(std::move(drmBuffer)));
> > +}
> > +
> > +int KMSSink::configure(const libcamera::CameraConfiguration &config)
> > +{
> > +	if (!connector_)
> > +		return -EINVAL;
>
> I assumed there isValid() for a reason, an alternative would be use to 
> that. But this is fine as well probably.

That function is actually unused, so I'll drop it.

> > +
> > +	crtc_ = nullptr;
> > +	plane_ = nullptr;
> > +	mode_ = nullptr;
> > +
> > +	const libcamera::StreamConfiguration &cfg = config.at(0);
> > +	int ret = configurePipeline(cfg.pixelFormat);
> 
> Should we be finding a compatiable connector's mode (as done below) 
> before configurePipeline() ? Does not seem logical to error out on 
> incompatiable mode after we have configured the pipeline with possible 
> crtc  and encoder.
> 
> (But I maybe wrong here, there might be some implicit operational 
> sequence between these two, that I am missing)

Sounds reasonable, I'll swap the two.

> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	const std::vector<DRM::Mode> &modes = connector_->modes();
> > +	const auto iter = std::find_if(modes.begin(), modes.end(),
> > +				       [&](const DRM::Mode &mode) {
> > +					       return mode.hdisplay == cfg.size.width &&
> > +						      mode.vdisplay == cfg.size.height;
> > +				       });
> > +	if (iter == modes.end()) {
> > +		std::cerr
> > +			<< "No mode matching " << cfg.size.toString()
> > +			<< std::endl;
> > +		return -EINVAL;
> > +	}
> > +
> > +	mode_ = &*iter;
> > +	size_ = cfg.size;
> > +	stride_ = cfg.stride;
> > +
> > +	return 0;
> > +}
> > +
> > +int KMSSink::configurePipeline(const libcamera::PixelFormat &format)
> > +{
> > +	/*
> > +	 * If the requested format has an alpha channel, also consider the X
> > +	 * variant.
> > +	 */
> > +	libcamera::PixelFormat xFormat;
> > +
> > +	switch (format) {
> > +	case libcamera::formats::ABGR8888:
> > +		xFormat = libcamera::formats::XBGR8888;
> > +		break;
> > +	case libcamera::formats::ARGB8888:
> > +		xFormat = libcamera::formats::XRGB8888;
> > +		break;
> > +	case libcamera::formats::BGRA8888:
> > +		xFormat = libcamera::formats::BGRX8888;
> > +		break;
> > +	case libcamera::formats::RGBA8888:
> > +		xFormat = libcamera::formats::RGBX8888;
> > +		break;
> > +	}
> > +
> > +	/*
> > +	 * Find a CRTC and plane suitable for the request format and the
> > +	 * connector at the end of the pipeline. Restrict the search to primary
> > +	 * planes for now.
> > +	 */
> > +	for (const DRM::Encoder *encoder : connector_->encoders()) {
> > +		for (const DRM::Crtc *crtc : encoder->possibleCrtcs()) {
> > +			for (const DRM::Plane *plane : crtc->planes()) {
> > +				if (plane->type() != DRM::Plane::TypePrimary)
> > +					continue;
> > +
> > +				if (plane->supportsFormat(format)) {
> > +					crtc_ = crtc;
> > +					plane_ = plane;
> > +					format_ = format;
> > +					return 0;
> > +				}
> > +
> > +				if (plane->supportsFormat(xFormat)) {
> > +					crtc_ = crtc;
> > +					plane_ = plane;
> > +					format_ = xFormat;
> > +					return 0;
> > +				}
> > +			}
> > +		}
> > +	}
> > +
> > +	std::cerr
> > +		<< "Unable to find display pipeline for format "
> > +		<< format.toString() << std::endl;
> > +	return -EPIPE;
> > +}
> > +
> > +int KMSSink::start()
> > +{
> > +	std::unique_ptr<DRM::AtomicRequest> request;
> > +
> > +	int ret = FrameSink::start();
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	/* Disable all CRTCs and planes to start from a known valid state. */
> > +	request = std::make_unique<DRM::AtomicRequest>(&dev_);
> > +
> > +	for (const DRM::Crtc &crtc : dev_.crtcs())
> > +		request->addProperty(&crtc, "ACTIVE", 0);
> > +
> > +	for (const DRM::Plane &plane : dev_.planes()) {
> > +		request->addProperty(&plane, "CRTC_ID", 0);
> > +		request->addProperty(&plane, "FB_ID", 0);
> > +	}
> > +
> > +	ret = request->commit(DRM::AtomicRequest::FlagAllowModeset);
> > +	if (ret < 0) {
> > +		std::cerr
> > +			<< "Failed to disable CRTCs and planes: "
> > +			<< strerror(-ret) << std::endl;
> > +		return ret;
> > +	}
> > +
> > +	/* Enable the display pipeline with no plane to start with. */
> > +	request = std::make_unique<DRM::AtomicRequest>(&dev_);
> > +
> > +	request->addProperty(connector_, "CRTC_ID", crtc_->id());
> > +	request->addProperty(crtc_, "ACTIVE", 1);
> > +	request->addProperty(crtc_, "MODE_ID", mode_->toBlob(&dev_));
> > +
> > +	ret = request->commit(DRM::AtomicRequest::FlagAllowModeset);
> > +	if (ret < 0) {
> > +		std::cerr
> > +			<< "Failed to enable display pipeline: "
> > +			<< strerror(-ret) << std::endl;
> > +		return ret;
> > +	}
> > +
> > +	planeInitialized_ = false;
> > +
> > +	return 0;
> > +}
> > +
> > +int KMSSink::stop()
> > +{
> > +	/* Display pipeline. */
> > +	DRM::AtomicRequest request(&dev_);
> > +
> > +	request.addProperty(connector_, "CRTC_ID", 0);
> > +	request.addProperty(crtc_, "ACTIVE", 0);
> > +	request.addProperty(crtc_, "MODE_ID", 0);
> > +	request.addProperty(plane_, "CRTC_ID", 0);
> > +	request.addProperty(plane_, "FB_ID", 0);
> > +
> > +	int ret = request.commit(DRM::AtomicRequest::FlagAllowModeset);
> > +	if (ret < 0) {
> > +		std::cerr
> > +			<< "Failed to stop display pipeline: "
> > +			<< strerror(-ret) << std::endl;
> > +		return ret;
> > +	}
> > +
> > +	/* Free all buffers. */
> > +	pending_.reset();
> > +	queued_.reset();
> > +	active_.reset();
> > +	buffers_.clear();
> > +
> > +	return FrameSink::stop();
> > +}
> > +
> > +bool KMSSink::processRequest(libcamera::Request *camRequest)
> > +{
> > +	if (pending_)
> > +		return true;
> > +
> > +	libcamera::FrameBuffer *buffer = camRequest->buffers().begin()->second;
> > +	auto iter = buffers_.find(buffer);
> > +	if (iter == buffers_.end())
> > +		return true;
> > +
> > +	DRM::FrameBuffer *drmBuffer = iter->second.get();
> > +
> > +	DRM::AtomicRequest *drmRequest = new DRM::AtomicRequest(&dev_);
> > +	drmRequest->addProperty(plane_, "FB_ID", drmBuffer->id());
> > +
> > +	if (!planeInitialized_) {
> > +		drmRequest->addProperty(plane_, "CRTC_ID", crtc_->id());
> > +		drmRequest->addProperty(plane_, "SRC_X", 0 << 16);
> > +		drmRequest->addProperty(plane_, "SRC_Y", 0 << 16);
> > +		drmRequest->addProperty(plane_, "SRC_W", mode_->hdisplay << 16);
> > +		drmRequest->addProperty(plane_, "SRC_H", mode_->vdisplay << 16);
> > +		drmRequest->addProperty(plane_, "CRTC_X", 0);
> > +		drmRequest->addProperty(plane_, "CRTC_Y", 0);
> > +		drmRequest->addProperty(plane_, "CRTC_W", mode_->hdisplay);
> > +		drmRequest->addProperty(plane_, "CRTC_H", mode_->vdisplay);
> > +		planeInitialized_ = true;
> > +	}
> > +
> > +	pending_ = std::make_unique<Request>(drmRequest, camRequest);
> 
> This is neat! :)

Unique pointers are really nice, yes.

> Looks good to me with my limited understanding I gained this morning on 
> the subject:
> 
> Reviewed-by: Umang Jain <umang.jain@ideasonboard.com>
> 
> > +
> > +	std::lock_guard<std::mutex> lock(lock_);
> > +
> > +	if (!queued_) {
> > +		int ret = drmRequest->commit(DRM::AtomicRequest::FlagAsync);
> > +		if (ret < 0)
> > +			std::cerr
> > +				<< "Failed to commit atomic request: "
> > +				<< strerror(-ret) << std::endl;
> > +		queued_ = std::move(pending_);
> > +	}
> > +
> > +	return false;
> > +}
> > +
> > +void KMSSink::requestComplete(DRM::AtomicRequest *request)
> > +{
> > +	std::lock_guard<std::mutex> lock(lock_);
> > +
> > +	assert(queued_ && queued_->drmRequest_.get() == request);
> > +
> > +	/* Complete the active request, if any. */
> > +	if (active_)
> > +		requestProcessed.emit(active_->camRequest_);
> > +
> > +	/* The queued request becomes active. */
> > +	active_ = std::move(queued_);
> > +
> > +	/* Queue the pending request, if any. */
> > +	if (pending_) {
> > +		pending_->drmRequest_->commit(DRM::AtomicRequest::FlagAsync);
> > +		queued_ = std::move(pending_);
> > +	}
> > +}
> > diff --git a/src/cam/kms_sink.h b/src/cam/kms_sink.h
> > new file mode 100644
> > index 000000000000..2895e00f84a1
> > --- /dev/null
> > +++ b/src/cam/kms_sink.h
> > @@ -0,0 +1,76 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > +/*
> > + * Copyright (C) 2021, Ideas on Board Oy
> > + *
> > + * kms_sink.h - KMS Sink
> > + */
> > +#ifndef __CAM_KMS_SINK_H__
> > +#define __CAM_KMS_SINK_H__
> > +
> > +#include <list>
> > +#include <memory>
> > +#include <mutex>
> > +#include <string>
> > +#include <utility>
> > +
> > +#include <libcamera/base/signal.h>
> > +
> > +#include <libcamera/geometry.h>
> > +#include <libcamera/pixel_format.h>
> > +
> > +#include "drm.h"
> > +#include "frame_sink.h"
> > +
> > +class KMSSink : public FrameSink
> > +{
> > +public:
> > +	KMSSink(const std::string &connectorName);
> > +
> > +	bool isValid() const { return connector_ != nullptr; }
> > +
> > +	void mapBuffer(libcamera::FrameBuffer *buffer) override;
> > +
> > +	int configure(const libcamera::CameraConfiguration &config) override;
> > +	int start() override;
> > +	int stop() override;
> > +
> > +	bool processRequest(libcamera::Request *request) override;
> > +
> > +private:
> > +	class Request
> > +	{
> > +	public:
> > +		Request(DRM::AtomicRequest *drmRequest, libcamera::Request *camRequest)
> > +			: drmRequest_(drmRequest), camRequest_(camRequest)
> > +		{
> > +		}
> > +
> > +		std::unique_ptr<DRM::AtomicRequest> drmRequest_;
> > +		libcamera::Request *camRequest_;
> > +	};
> > +
> > +	int configurePipeline(const libcamera::PixelFormat &format);
> > +	void requestComplete(DRM::AtomicRequest *request);
> > +
> > +	DRM::Device dev_;
> > +
> > +	const DRM::Connector *connector_;
> > +	const DRM::Crtc *crtc_;
> > +	const DRM::Plane *plane_;
> > +	const DRM::Mode *mode_;
> > +
> > +	libcamera::PixelFormat format_;
> > +	libcamera::Size size_;
> > +	unsigned int stride_;
> > +
> > +	bool planeInitialized_;
> > +
> > +	std::map<libcamera::FrameBuffer *, std::unique_ptr<DRM::FrameBuffer>> buffers_;
> > +
> > +	std::mutex lock_;
> > +	std::unique_ptr<Request> pending_;
> > +	std::unique_ptr<Request> queued_;
> > +	std::unique_ptr<Request> active_;
> > +};
> > +
> > +#endif /* __CAM_KMS_SINK_H__ */
> > diff --git a/src/cam/meson.build b/src/cam/meson.build
> > index b47add55b0cb..ea36aaa5c514 100644
> > --- a/src/cam/meson.build
> > +++ b/src/cam/meson.build
> > @@ -27,6 +27,7 @@ if libdrm.found()
> >   cam_cpp_args += [ '-DHAVE_KMS' ]
> >   cam_sources += files([
> >       'drm.cpp',
> > +    'kms_sink.cpp'
> >   ])
> >   endif
> >

Hi Laurent,

On 04/08/2021 13:43, Laurent Pinchart wrote:
> Add a KMSSink class to display framebuffers through the DRM/KMS API.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> Changes since v2:
> 
> - Simplify connector selection logic, and fix it when a connector name
>   is specified
> - Fail configure() if no connector is selected
> - Update copyright years
> 
> Changes since v1:
> 
> - Use formats:: namespace
> 
> fixup! cam: Add KMS sink class

Ok ;-)


> ---
>  src/cam/kms_sink.cpp | 306 +++++++++++++++++++++++++++++++++++++++++++
>  src/cam/kms_sink.h   |  76 +++++++++++
>  src/cam/meson.build  |   1 +
>  3 files changed, 383 insertions(+)
>  create mode 100644 src/cam/kms_sink.cpp
>  create mode 100644 src/cam/kms_sink.h
> 
> diff --git a/src/cam/kms_sink.cpp b/src/cam/kms_sink.cpp
> new file mode 100644
> index 000000000000..f708b613f226
> --- /dev/null
> +++ b/src/cam/kms_sink.cpp
> @@ -0,0 +1,306 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Copyright (C) 2021, Ideas on Board Oy
> + *
> + * kms_sink.cpp - KMS Sink
> + */
> +
> +#include "kms_sink.h"
> +
> +#include <algorithm>
> +#include <assert.h>
> +#include <iostream>
> +#include <memory>
> +#include <string.h>
> +
> +#include <libcamera/camera.h>
> +#include <libcamera/formats.h>
> +#include <libcamera/framebuffer.h>
> +#include <libcamera/stream.h>
> +
> +#include "drm.h"
> +
> +KMSSink::KMSSink(const std::string &connectorName)
> +	: connector_(nullptr), crtc_(nullptr), plane_(nullptr), mode_(nullptr)
> +{
> +	int ret = dev_.init();
> +	if (ret < 0)
> +		return;
> +
> +	/*
> +	 * Find the requested connector. If no specific connector is requested,
> +	 * pick the first connected connector or, if no connector is connected,
> +	 * the first connector with unknown status.
> +	 */
> +	for (const DRM::Connector &conn : dev_.connectors()) {
> +		if (!connectorName.empty()) {
> +			if (conn.name() != connectorName)
> +				continue;
> +
> +			connector_ = &conn;
> +			break;
> +		}
> +
> +		if (conn.status() == DRM::Connector::Connected) {
> +			connector_ = &conn;
> +			break;
> +		}
> +
> +		if (!connector_ && conn.status() == DRM::Connector::Unknown)
> +			connector_ = &conn;
> +	}
> +
> +	if (!connector_) {
> +		if (!connectorName.empty())
> +			std::cerr
> +				<< "Connector " << connectorName << " not found"
> +				<< std::endl;
> +		else
> +			std::cerr << "No connected connector found" << std::endl;
> +		return;
> +	}
> +
> +	dev_.requestComplete.connect(this, &KMSSink::requestComplete);
> +}
> +
> +void KMSSink::mapBuffer(libcamera::FrameBuffer *buffer)
> +{
> +	std::unique_ptr<DRM::FrameBuffer> drmBuffer =
> +		dev_.createFrameBuffer(*buffer, format_, size_, stride_);
> +	if (!drmBuffer)
> +		return;
> +
> +	buffers_.emplace(std::piecewise_construct,
> +			 std::forward_as_tuple(buffer),
> +			 std::forward_as_tuple(std::move(drmBuffer)));
> +}
> +
> +int KMSSink::configure(const libcamera::CameraConfiguration &config)
> +{
> +	if (!connector_)
> +		return -EINVAL;
> +
> +	crtc_ = nullptr;
> +	plane_ = nullptr;
> +	mode_ = nullptr;
> +
> +	const libcamera::StreamConfiguration &cfg = config.at(0);
> +	int ret = configurePipeline(cfg.pixelFormat);
> +	if (ret < 0)
> +		return ret;
> +
> +	const std::vector<DRM::Mode> &modes = connector_->modes();
> +	const auto iter = std::find_if(modes.begin(), modes.end(),
> +				       [&](const DRM::Mode &mode) {
> +					       return mode.hdisplay == cfg.size.width &&
> +						      mode.vdisplay == cfg.size.height;
> +				       });
> +	if (iter == modes.end()) {
> +		std::cerr
> +			<< "No mode matching " << cfg.size.toString()
> +			<< std::endl;
> +		return -EINVAL;
> +	}
> +
> +	mode_ = &*iter;
> +	size_ = cfg.size;
> +	stride_ = cfg.stride;
> +
> +	return 0;
> +}
> +
> +int KMSSink::configurePipeline(const libcamera::PixelFormat &format)
> +{
> +	/*
> +	 * If the requested format has an alpha channel, also consider the X
> +	 * variant.
> +	 */
> +	libcamera::PixelFormat xFormat;
> +
> +	switch (format) {
> +	case libcamera::formats::ABGR8888:
> +		xFormat = libcamera::formats::XBGR8888;
> +		break;
> +	case libcamera::formats::ARGB8888:
> +		xFormat = libcamera::formats::XRGB8888;
> +		break;
> +	case libcamera::formats::BGRA8888:
> +		xFormat = libcamera::formats::BGRX8888;
> +		break;
> +	case libcamera::formats::RGBA8888:
> +		xFormat = libcamera::formats::RGBX8888;
> +		break;
> +	}
> +
> +	/*
> +	 * Find a CRTC and plane suitable for the request format and the
> +	 * connector at the end of the pipeline. Restrict the search to primary
> +	 * planes for now.
> +	 */
> +	for (const DRM::Encoder *encoder : connector_->encoders()) {
> +		for (const DRM::Crtc *crtc : encoder->possibleCrtcs()) {
> +			for (const DRM::Plane *plane : crtc->planes()) {
> +				if (plane->type() != DRM::Plane::TypePrimary)
> +					continue;
> +
> +				if (plane->supportsFormat(format)) {
> +					crtc_ = crtc;
> +					plane_ = plane;
> +					format_ = format;
> +					return 0;
> +				}
> +
> +				if (plane->supportsFormat(xFormat)) {
> +					crtc_ = crtc;
> +					plane_ = plane;
> +					format_ = xFormat;
> +					return 0;
> +				}
> +			}
> +		}
> +	}
> +
> +	std::cerr
> +		<< "Unable to find display pipeline for format "
> +		<< format.toString() << std::endl;

possibly a blank line here but meh.


> +	return -EPIPE;
> +}
> +
> +int KMSSink::start()
> +{
> +	std::unique_ptr<DRM::AtomicRequest> request;
> +
> +	int ret = FrameSink::start();
> +	if (ret < 0)
> +		return ret;
> +
> +	/* Disable all CRTCs and planes to start from a known valid state. */
> +	request = std::make_unique<DRM::AtomicRequest>(&dev_);
> +
> +	for (const DRM::Crtc &crtc : dev_.crtcs())
> +		request->addProperty(&crtc, "ACTIVE", 0);
> +
> +	for (const DRM::Plane &plane : dev_.planes()) {
> +		request->addProperty(&plane, "CRTC_ID", 0);
> +		request->addProperty(&plane, "FB_ID", 0);
> +	}
> +
> +	ret = request->commit(DRM::AtomicRequest::FlagAllowModeset);
> +	if (ret < 0) {
> +		std::cerr
> +			<< "Failed to disable CRTCs and planes: "
> +			<< strerror(-ret) << std::endl;
> +		return ret;
> +	}
> +
> +	/* Enable the display pipeline with no plane to start with. */
> +	request = std::make_unique<DRM::AtomicRequest>(&dev_);
> +
> +	request->addProperty(connector_, "CRTC_ID", crtc_->id());
> +	request->addProperty(crtc_, "ACTIVE", 1);
> +	request->addProperty(crtc_, "MODE_ID", mode_->toBlob(&dev_));
> +
> +	ret = request->commit(DRM::AtomicRequest::FlagAllowModeset);
> +	if (ret < 0) {
> +		std::cerr
> +			<< "Failed to enable display pipeline: "
> +			<< strerror(-ret) << std::endl;
> +		return ret;
> +	}
> +
> +	planeInitialized_ = false;
> +
> +	return 0;
> +}
> +
> +int KMSSink::stop()
> +{
> +	/* Display pipeline. */
> +	DRM::AtomicRequest request(&dev_);
> +
> +	request.addProperty(connector_, "CRTC_ID", 0);
> +	request.addProperty(crtc_, "ACTIVE", 0);
> +	request.addProperty(crtc_, "MODE_ID", 0);
> +	request.addProperty(plane_, "CRTC_ID", 0);
> +	request.addProperty(plane_, "FB_ID", 0);
> +
> +	int ret = request.commit(DRM::AtomicRequest::FlagAllowModeset);
> +	if (ret < 0) {
> +		std::cerr
> +			<< "Failed to stop display pipeline: "
> +			<< strerror(-ret) << std::endl;
> +		return ret;
> +	}
> +
> +	/* Free all buffers. */
> +	pending_.reset();
> +	queued_.reset();
> +	active_.reset();
> +	buffers_.clear();
> +
> +	return FrameSink::stop();
> +}
> +
> +bool KMSSink::processRequest(libcamera::Request *camRequest)
> +{
> +	if (pending_)
> +		return true;

This feels like it needs a comment.

If there is a pending - are we skipping this request because we're
essentially overflowing / receiving more completed requests than we can
display?




Reading that I'm resisting suggesting that processRequest could return
an 'enum state' instead of bool.

	if (pending_)
		return FrameSink::RequestHandled;

But that's probably overkill, so maybe I won't suggest it ;-)


> +
> +	libcamera::FrameBuffer *buffer = camRequest->buffers().begin()->second;
> +	auto iter = buffers_.find(buffer);
> +	if (iter == buffers_.end())
> +		return true;
> +
> +	DRM::FrameBuffer *drmBuffer = iter->second.get();
> +
> +	DRM::AtomicRequest *drmRequest = new DRM::AtomicRequest(&dev_);
> +	drmRequest->addProperty(plane_, "FB_ID", drmBuffer->id());
> +
> +	if (!planeInitialized_) {
> +		drmRequest->addProperty(plane_, "CRTC_ID", crtc_->id());
> +		drmRequest->addProperty(plane_, "SRC_X", 0 << 16);
> +		drmRequest->addProperty(plane_, "SRC_Y", 0 << 16);
> +		drmRequest->addProperty(plane_, "SRC_W", mode_->hdisplay << 16);
> +		drmRequest->addProperty(plane_, "SRC_H", mode_->vdisplay << 16);
> +		drmRequest->addProperty(plane_, "CRTC_X", 0);
> +		drmRequest->addProperty(plane_, "CRTC_Y", 0);
> +		drmRequest->addProperty(plane_, "CRTC_W", mode_->hdisplay);
> +		drmRequest->addProperty(plane_, "CRTC_H", mode_->vdisplay);
> +		planeInitialized_ = true;
> +	}
> +
> +	pending_ = std::make_unique<Request>(drmRequest, camRequest);

I wonder if we should optimise this so we don't have to malloc a new
object each time - but that could be ignored here.

If we had a means to 'set' the drmRequest and camRequest, we could have
a fourth st::unique_pointer<Request> to move the latest completed
request to - and re-use it - but we're not worried about


> +
> +	std::lock_guard<std::mutex> lock(lock_);
> +
> +	if (!queued_) {
> +		int ret = drmRequest->commit(DRM::AtomicRequest::FlagAsync);
> +		if (ret < 0)
> +			std::cerr
> +				<< "Failed to commit atomic request: "
> +				<< strerror(-ret) << std::endl;
> +		queued_ = std::move(pending_);

Is this right? If we failed to queue it - should we mark it as queued?
Or is this just to manage the lifecycle of the allocation...

If it's intentional - it deserves a comment to express that I think.

I suspect it is required to release pending_ so that we can let the next
request in regardless...



> +	}
> +
> +	return false;
> +}
> +
> +void KMSSink::requestComplete(DRM::AtomicRequest *request)
> +{
> +	std::lock_guard<std::mutex> lock(lock_);
> +
> +	assert(queued_ && queued_->drmRequest_.get() == request);
> +
> +	/* Complete the active request, if any. */
> +	if (active_)
> +		requestProcessed.emit(active_->camRequest_);
> +
> +	/* The queued request becomes active. */
> +	active_ = std::move(queued_);

Is the request guaranteed to have been queued to the hardware at this
point? Is there any error handling - such as some state to track or
report in the AtomicRequest?

Either way, even if that failed, the active_ would be released back to
the requestProcessed signal on the next iteration anyway... so the
'pipeline' here will be fine as far as I can see.


Looks like my comments are only about adding comments so ...

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>




> +
> +	/* Queue the pending request, if any. */
> +	if (pending_) {
> +		pending_->drmRequest_->commit(DRM::AtomicRequest::FlagAsync);
> +		queued_ = std::move(pending_);
> +	}
> +}
> diff --git a/src/cam/kms_sink.h b/src/cam/kms_sink.h
> new file mode 100644
> index 000000000000..2895e00f84a1
> --- /dev/null
> +++ b/src/cam/kms_sink.h
> @@ -0,0 +1,76 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Copyright (C) 2021, Ideas on Board Oy
> + *
> + * kms_sink.h - KMS Sink
> + */
> +#ifndef __CAM_KMS_SINK_H__
> +#define __CAM_KMS_SINK_H__
> +
> +#include <list>
> +#include <memory>
> +#include <mutex>
> +#include <string>
> +#include <utility>
> +
> +#include <libcamera/base/signal.h>
> +
> +#include <libcamera/geometry.h>
> +#include <libcamera/pixel_format.h>
> +
> +#include "drm.h"
> +#include "frame_sink.h"
> +
> +class KMSSink : public FrameSink
> +{
> +public:
> +	KMSSink(const std::string &connectorName);
> +
> +	bool isValid() const { return connector_ != nullptr; }
> +
> +	void mapBuffer(libcamera::FrameBuffer *buffer) override;
> +
> +	int configure(const libcamera::CameraConfiguration &config) override;
> +	int start() override;
> +	int stop() override;
> +
> +	bool processRequest(libcamera::Request *request) override;
> +
> +private:
> +	class Request
> +	{
> +	public:
> +		Request(DRM::AtomicRequest *drmRequest, libcamera::Request *camRequest)
> +			: drmRequest_(drmRequest), camRequest_(camRequest)
> +		{
> +		}
> +
> +		std::unique_ptr<DRM::AtomicRequest> drmRequest_;
> +		libcamera::Request *camRequest_;
> +	};
> +
> +	int configurePipeline(const libcamera::PixelFormat &format);
> +	void requestComplete(DRM::AtomicRequest *request);
> +
> +	DRM::Device dev_;
> +
> +	const DRM::Connector *connector_;
> +	const DRM::Crtc *crtc_;
> +	const DRM::Plane *plane_;
> +	const DRM::Mode *mode_;
> +
> +	libcamera::PixelFormat format_;
> +	libcamera::Size size_;
> +	unsigned int stride_;
> +
> +	bool planeInitialized_;
> +
> +	std::map<libcamera::FrameBuffer *, std::unique_ptr<DRM::FrameBuffer>> buffers_;
> +
> +	std::mutex lock_;
> +	std::unique_ptr<Request> pending_;
> +	std::unique_ptr<Request> queued_;
> +	std::unique_ptr<Request> active_;> +};
> +
> +#endif /* __CAM_KMS_SINK_H__ */
> diff --git a/src/cam/meson.build b/src/cam/meson.build
> index b47add55b0cb..ea36aaa5c514 100644
> --- a/src/cam/meson.build
> +++ b/src/cam/meson.build
> @@ -27,6 +27,7 @@ if libdrm.found()
>  cam_cpp_args += [ '-DHAVE_KMS' ]
>  cam_sources += files([
>      'drm.cpp',
> +    'kms_sink.cpp'
>  ])
>  endif
>  
>

Hi Kieran,

On Thu, Aug 05, 2021 at 01:32:35PM +0100, Kieran Bingham wrote:
> On 04/08/2021 13:43, Laurent Pinchart wrote:
> > Add a KMSSink class to display framebuffers through the DRM/KMS API.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> > Changes since v2:
> > 
> > - Simplify connector selection logic, and fix it when a connector name
> >   is specified
> > - Fail configure() if no connector is selected
> > - Update copyright years
> > 
> > Changes since v1:
> > 
> > - Use formats:: namespace
> > 
> > fixup! cam: Add KMS sink class
> 
> Ok ;-)

Oops, caught red-handed using git commit --fixup :-)

> > ---
> >  src/cam/kms_sink.cpp | 306 +++++++++++++++++++++++++++++++++++++++++++
> >  src/cam/kms_sink.h   |  76 +++++++++++
> >  src/cam/meson.build  |   1 +
> >  3 files changed, 383 insertions(+)
> >  create mode 100644 src/cam/kms_sink.cpp
> >  create mode 100644 src/cam/kms_sink.h
> > 
> > diff --git a/src/cam/kms_sink.cpp b/src/cam/kms_sink.cpp
> > new file mode 100644
> > index 000000000000..f708b613f226
> > --- /dev/null
> > +++ b/src/cam/kms_sink.cpp
> > @@ -0,0 +1,306 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > +/*
> > + * Copyright (C) 2021, Ideas on Board Oy
> > + *
> > + * kms_sink.cpp - KMS Sink
> > + */
> > +
> > +#include "kms_sink.h"
> > +
> > +#include <algorithm>
> > +#include <assert.h>
> > +#include <iostream>
> > +#include <memory>
> > +#include <string.h>
> > +
> > +#include <libcamera/camera.h>
> > +#include <libcamera/formats.h>
> > +#include <libcamera/framebuffer.h>
> > +#include <libcamera/stream.h>
> > +
> > +#include "drm.h"
> > +
> > +KMSSink::KMSSink(const std::string &connectorName)
> > +	: connector_(nullptr), crtc_(nullptr), plane_(nullptr), mode_(nullptr)
> > +{
> > +	int ret = dev_.init();
> > +	if (ret < 0)
> > +		return;
> > +
> > +	/*
> > +	 * Find the requested connector. If no specific connector is requested,
> > +	 * pick the first connected connector or, if no connector is connected,
> > +	 * the first connector with unknown status.
> > +	 */
> > +	for (const DRM::Connector &conn : dev_.connectors()) {
> > +		if (!connectorName.empty()) {
> > +			if (conn.name() != connectorName)
> > +				continue;
> > +
> > +			connector_ = &conn;
> > +			break;
> > +		}
> > +
> > +		if (conn.status() == DRM::Connector::Connected) {
> > +			connector_ = &conn;
> > +			break;
> > +		}
> > +
> > +		if (!connector_ && conn.status() == DRM::Connector::Unknown)
> > +			connector_ = &conn;
> > +	}
> > +
> > +	if (!connector_) {
> > +		if (!connectorName.empty())
> > +			std::cerr
> > +				<< "Connector " << connectorName << " not found"
> > +				<< std::endl;
> > +		else
> > +			std::cerr << "No connected connector found" << std::endl;
> > +		return;
> > +	}
> > +
> > +	dev_.requestComplete.connect(this, &KMSSink::requestComplete);
> > +}
> > +
> > +void KMSSink::mapBuffer(libcamera::FrameBuffer *buffer)
> > +{
> > +	std::unique_ptr<DRM::FrameBuffer> drmBuffer =
> > +		dev_.createFrameBuffer(*buffer, format_, size_, stride_);
> > +	if (!drmBuffer)
> > +		return;
> > +
> > +	buffers_.emplace(std::piecewise_construct,
> > +			 std::forward_as_tuple(buffer),
> > +			 std::forward_as_tuple(std::move(drmBuffer)));
> > +}
> > +
> > +int KMSSink::configure(const libcamera::CameraConfiguration &config)
> > +{
> > +	if (!connector_)
> > +		return -EINVAL;
> > +
> > +	crtc_ = nullptr;
> > +	plane_ = nullptr;
> > +	mode_ = nullptr;
> > +
> > +	const libcamera::StreamConfiguration &cfg = config.at(0);
> > +	int ret = configurePipeline(cfg.pixelFormat);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	const std::vector<DRM::Mode> &modes = connector_->modes();
> > +	const auto iter = std::find_if(modes.begin(), modes.end(),
> > +				       [&](const DRM::Mode &mode) {
> > +					       return mode.hdisplay == cfg.size.width &&
> > +						      mode.vdisplay == cfg.size.height;
> > +				       });
> > +	if (iter == modes.end()) {
> > +		std::cerr
> > +			<< "No mode matching " << cfg.size.toString()
> > +			<< std::endl;
> > +		return -EINVAL;
> > +	}
> > +
> > +	mode_ = &*iter;
> > +	size_ = cfg.size;
> > +	stride_ = cfg.stride;
> > +
> > +	return 0;
> > +}
> > +
> > +int KMSSink::configurePipeline(const libcamera::PixelFormat &format)
> > +{
> > +	/*
> > +	 * If the requested format has an alpha channel, also consider the X
> > +	 * variant.
> > +	 */
> > +	libcamera::PixelFormat xFormat;
> > +
> > +	switch (format) {
> > +	case libcamera::formats::ABGR8888:
> > +		xFormat = libcamera::formats::XBGR8888;
> > +		break;
> > +	case libcamera::formats::ARGB8888:
> > +		xFormat = libcamera::formats::XRGB8888;
> > +		break;
> > +	case libcamera::formats::BGRA8888:
> > +		xFormat = libcamera::formats::BGRX8888;
> > +		break;
> > +	case libcamera::formats::RGBA8888:
> > +		xFormat = libcamera::formats::RGBX8888;
> > +		break;
> > +	}
> > +
> > +	/*
> > +	 * Find a CRTC and plane suitable for the request format and the
> > +	 * connector at the end of the pipeline. Restrict the search to primary
> > +	 * planes for now.
> > +	 */
> > +	for (const DRM::Encoder *encoder : connector_->encoders()) {
> > +		for (const DRM::Crtc *crtc : encoder->possibleCrtcs()) {
> > +			for (const DRM::Plane *plane : crtc->planes()) {
> > +				if (plane->type() != DRM::Plane::TypePrimary)
> > +					continue;
> > +
> > +				if (plane->supportsFormat(format)) {
> > +					crtc_ = crtc;
> > +					plane_ = plane;
> > +					format_ = format;
> > +					return 0;
> > +				}
> > +
> > +				if (plane->supportsFormat(xFormat)) {
> > +					crtc_ = crtc;
> > +					plane_ = plane;
> > +					format_ = xFormat;
> > +					return 0;
> > +				}
> > +			}
> > +		}
> > +	}
> > +
> > +	std::cerr
> > +		<< "Unable to find display pipeline for format "
> > +		<< format.toString() << std::endl;
> 
> possibly a blank line here but meh.

If that's all it takes to make you happy, sure :-)

> > +	return -EPIPE;
> > +}
> > +
> > +int KMSSink::start()
> > +{
> > +	std::unique_ptr<DRM::AtomicRequest> request;
> > +
> > +	int ret = FrameSink::start();
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	/* Disable all CRTCs and planes to start from a known valid state. */
> > +	request = std::make_unique<DRM::AtomicRequest>(&dev_);
> > +
> > +	for (const DRM::Crtc &crtc : dev_.crtcs())
> > +		request->addProperty(&crtc, "ACTIVE", 0);
> > +
> > +	for (const DRM::Plane &plane : dev_.planes()) {
> > +		request->addProperty(&plane, "CRTC_ID", 0);
> > +		request->addProperty(&plane, "FB_ID", 0);
> > +	}
> > +
> > +	ret = request->commit(DRM::AtomicRequest::FlagAllowModeset);
> > +	if (ret < 0) {
> > +		std::cerr
> > +			<< "Failed to disable CRTCs and planes: "
> > +			<< strerror(-ret) << std::endl;
> > +		return ret;
> > +	}
> > +
> > +	/* Enable the display pipeline with no plane to start with. */
> > +	request = std::make_unique<DRM::AtomicRequest>(&dev_);
> > +
> > +	request->addProperty(connector_, "CRTC_ID", crtc_->id());
> > +	request->addProperty(crtc_, "ACTIVE", 1);
> > +	request->addProperty(crtc_, "MODE_ID", mode_->toBlob(&dev_));
> > +
> > +	ret = request->commit(DRM::AtomicRequest::FlagAllowModeset);
> > +	if (ret < 0) {
> > +		std::cerr
> > +			<< "Failed to enable display pipeline: "
> > +			<< strerror(-ret) << std::endl;
> > +		return ret;
> > +	}
> > +
> > +	planeInitialized_ = false;
> > +
> > +	return 0;
> > +}
> > +
> > +int KMSSink::stop()
> > +{
> > +	/* Display pipeline. */
> > +	DRM::AtomicRequest request(&dev_);
> > +
> > +	request.addProperty(connector_, "CRTC_ID", 0);
> > +	request.addProperty(crtc_, "ACTIVE", 0);
> > +	request.addProperty(crtc_, "MODE_ID", 0);
> > +	request.addProperty(plane_, "CRTC_ID", 0);
> > +	request.addProperty(plane_, "FB_ID", 0);
> > +
> > +	int ret = request.commit(DRM::AtomicRequest::FlagAllowModeset);
> > +	if (ret < 0) {
> > +		std::cerr
> > +			<< "Failed to stop display pipeline: "
> > +			<< strerror(-ret) << std::endl;
> > +		return ret;
> > +	}
> > +
> > +	/* Free all buffers. */
> > +	pending_.reset();
> > +	queued_.reset();
> > +	active_.reset();
> > +	buffers_.clear();
> > +
> > +	return FrameSink::stop();
> > +}
> > +
> > +bool KMSSink::processRequest(libcamera::Request *camRequest)
> > +{
> > +	if (pending_)
> > +		return true;
> 
> This feels like it needs a comment.
> 
> If there is a pending - are we skipping this request because we're
> essentially overflowing / receiving more completed requests than we can
> display?

Correct. I'll add a comment.

        /*
         * Perform a very crude rate adaptation by simply dropping the request
         * if the display queue is full.
         */

> Reading that I'm resisting suggesting that processRequest could return
> an 'enum state' instead of bool.
> 
> 	if (pending_)
> 		return FrameSink::RequestHandled;
> 
> But that's probably overkill, so maybe I won't suggest it ;-)

:-) I think we'll rework this anyway when adding the option to use
multiple sinks.

> > +
> > +	libcamera::FrameBuffer *buffer = camRequest->buffers().begin()->second;
> > +	auto iter = buffers_.find(buffer);
> > +	if (iter == buffers_.end())
> > +		return true;
> > +
> > +	DRM::FrameBuffer *drmBuffer = iter->second.get();
> > +
> > +	DRM::AtomicRequest *drmRequest = new DRM::AtomicRequest(&dev_);
> > +	drmRequest->addProperty(plane_, "FB_ID", drmBuffer->id());
> > +
> > +	if (!planeInitialized_) {
> > +		drmRequest->addProperty(plane_, "CRTC_ID", crtc_->id());
> > +		drmRequest->addProperty(plane_, "SRC_X", 0 << 16);
> > +		drmRequest->addProperty(plane_, "SRC_Y", 0 << 16);
> > +		drmRequest->addProperty(plane_, "SRC_W", mode_->hdisplay << 16);
> > +		drmRequest->addProperty(plane_, "SRC_H", mode_->vdisplay << 16);
> > +		drmRequest->addProperty(plane_, "CRTC_X", 0);
> > +		drmRequest->addProperty(plane_, "CRTC_Y", 0);
> > +		drmRequest->addProperty(plane_, "CRTC_W", mode_->hdisplay);
> > +		drmRequest->addProperty(plane_, "CRTC_H", mode_->vdisplay);
> > +		planeInitialized_ = true;
> > +	}
> > +
> > +	pending_ = std::make_unique<Request>(drmRequest, camRequest);
> 
> I wonder if we should optimise this so we don't have to malloc a new
> object each time - but that could be ignored here.
> 
> If we had a means to 'set' the drmRequest and camRequest, we could have
> a fourth st::unique_pointer<Request> to move the latest completed
> request to - and re-use it - but we're not worried about

We would also have to reuse the DRM::AtomicRequest. If we wanted to go
for zero dynamic allocations, it would be quite interesting, even adding
a property to the request will allocate memory internally in libdrm.

> > +
> > +	std::lock_guard<std::mutex> lock(lock_);
> > +
> > +	if (!queued_) {
> > +		int ret = drmRequest->commit(DRM::AtomicRequest::FlagAsync);
> > +		if (ret < 0)
> > +			std::cerr
> > +				<< "Failed to commit atomic request: "
> > +				<< strerror(-ret) << std::endl;
> > +		queued_ = std::move(pending_);
> 
> Is this right? If we failed to queue it - should we mark it as queued?
> Or is this just to manage the lifecycle of the allocation...
> 
> If it's intentional - it deserves a comment to express that I think.
> 
> I suspect it is required to release pending_ so that we can let the next
> request in regardless...

It's intentional in the sense that I have intentionally not implemented
error recovery :-)  Errors should really not happen here, they would
likely be fatal. I'll add a coment.

> > +	}
> > +
> > +	return false;
> > +}
> > +
> > +void KMSSink::requestComplete(DRM::AtomicRequest *request)
> > +{
> > +	std::lock_guard<std::mutex> lock(lock_);
> > +
> > +	assert(queued_ && queued_->drmRequest_.get() == request);
> > +
> > +	/* Complete the active request, if any. */
> > +	if (active_)
> > +		requestProcessed.emit(active_->camRequest_);
> > +
> > +	/* The queued request becomes active. */
> > +	active_ = std::move(queued_);
> 
> Is the request guaranteed to have been queued to the hardware at this
> point? Is there any error handling - such as some state to track or
> report in the AtomicRequest?

There's no error reporting mechanism I'm aware of, atomic commits are
not supposed to fail once they're queued.

> Either way, even if that failed, the active_ would be released back to
> the requestProcessed signal on the next iteration anyway... so the
> 'pipeline' here will be fine as far as I can see.
> 
> 
> Looks like my comments are only about adding comments so ...
> 
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> > +
> > +	/* Queue the pending request, if any. */
> > +	if (pending_) {
> > +		pending_->drmRequest_->commit(DRM::AtomicRequest::FlagAsync);
> > +		queued_ = std::move(pending_);
> > +	}
> > +}
> > diff --git a/src/cam/kms_sink.h b/src/cam/kms_sink.h
> > new file mode 100644
> > index 000000000000..2895e00f84a1
> > --- /dev/null
> > +++ b/src/cam/kms_sink.h
> > @@ -0,0 +1,76 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > +/*
> > + * Copyright (C) 2021, Ideas on Board Oy
> > + *
> > + * kms_sink.h - KMS Sink
> > + */
> > +#ifndef __CAM_KMS_SINK_H__
> > +#define __CAM_KMS_SINK_H__
> > +
> > +#include <list>
> > +#include <memory>
> > +#include <mutex>
> > +#include <string>
> > +#include <utility>
> > +
> > +#include <libcamera/base/signal.h>
> > +
> > +#include <libcamera/geometry.h>
> > +#include <libcamera/pixel_format.h>
> > +
> > +#include "drm.h"
> > +#include "frame_sink.h"
> > +
> > +class KMSSink : public FrameSink
> > +{
> > +public:
> > +	KMSSink(const std::string &connectorName);
> > +
> > +	bool isValid() const { return connector_ != nullptr; }
> > +
> > +	void mapBuffer(libcamera::FrameBuffer *buffer) override;
> > +
> > +	int configure(const libcamera::CameraConfiguration &config) override;
> > +	int start() override;
> > +	int stop() override;
> > +
> > +	bool processRequest(libcamera::Request *request) override;
> > +
> > +private:
> > +	class Request
> > +	{
> > +	public:
> > +		Request(DRM::AtomicRequest *drmRequest, libcamera::Request *camRequest)
> > +			: drmRequest_(drmRequest), camRequest_(camRequest)
> > +		{
> > +		}
> > +
> > +		std::unique_ptr<DRM::AtomicRequest> drmRequest_;
> > +		libcamera::Request *camRequest_;
> > +	};
> > +
> > +	int configurePipeline(const libcamera::PixelFormat &format);
> > +	void requestComplete(DRM::AtomicRequest *request);
> > +
> > +	DRM::Device dev_;
> > +
> > +	const DRM::Connector *connector_;
> > +	const DRM::Crtc *crtc_;
> > +	const DRM::Plane *plane_;
> > +	const DRM::Mode *mode_;
> > +
> > +	libcamera::PixelFormat format_;
> > +	libcamera::Size size_;
> > +	unsigned int stride_;
> > +
> > +	bool planeInitialized_;
> > +
> > +	std::map<libcamera::FrameBuffer *, std::unique_ptr<DRM::FrameBuffer>> buffers_;
> > +
> > +	std::mutex lock_;
> > +	std::unique_ptr<Request> pending_;
> > +	std::unique_ptr<Request> queued_;
> > +	std::unique_ptr<Request> active_;> +};
> > +
> > +#endif /* __CAM_KMS_SINK_H__ */
> > diff --git a/src/cam/meson.build b/src/cam/meson.build
> > index b47add55b0cb..ea36aaa5c514 100644
> > --- a/src/cam/meson.build
> > +++ b/src/cam/meson.build
> > @@ -27,6 +27,7 @@ if libdrm.found()
> >  cam_cpp_args += [ '-DHAVE_KMS' ]
> >  cam_sources += files([
> >      'drm.cpp',
> > +    'kms_sink.cpp'
> >  ])
> >  endif
> >