[PATCH v8 0/3] drm: Introduce writeback connectors

[PATCH v8 0/3] drm: Introduce writeback connectors

[Liviu Dudau]

Hi,

This is v8 of the writeback connector series. v7 got flagged by the
kbuild bot as not being correctly bisectable, so I went and fix that.

For anyone that wants a refresh on what changed in v6, the series
can be found here [2]. The only change in v7 is that the userspace
capabilities patch does not depend on the cleanup patch that got NACKed.

Best regards,
Liviu


[1] https://gitlab.freedesktop.org/drm-hwcomposer/drm-hwcomposer/merge_requests/3
[2] https://lists.freedesktop.org/archives/dri-devel/2018-February/167703.html

Brian Starkey (2):
  drm: Add writeback connector type
  drm: writeback: Add out-fences for writeback connectors

Liviu Dudau (1):
  drm: writeback: Add client capability for exposing writeback
    connectors

 Documentation/gpu/drm-kms.rst            |   9 +
 drivers/gpu/drm/Makefile                 |   2 +-
 drivers/gpu/drm/drm_atomic.c             | 227 +++++++++++++-
 drivers/gpu/drm/drm_atomic_helper.c      |  30 ++
 drivers/gpu/drm/drm_connector.c          |   4 +-
 drivers/gpu/drm/drm_ioctl.c              |   7 +
 drivers/gpu/drm/drm_mode_config.c        |   5 +
 drivers/gpu/drm/drm_writeback.c          | 360 +++++++++++++++++++++++
 include/drm/drm_atomic.h                 |  11 +
 include/drm/drm_connector.h              |  13 +
 include/drm/drm_file.h                   |   7 +
 include/drm/drm_mode_config.h            |  23 ++
 include/drm/drm_modeset_helper_vtables.h |  11 +
 include/drm/drm_writeback.h              | 129 ++++++++
 include/uapi/drm/drm.h                   |   9 +
 include/uapi/drm/drm_mode.h              |   1 +
 16 files changed, 837 insertions(+), 11 deletions(-)
 create mode 100644 drivers/gpu/drm/drm_writeback.c
 create mode 100644 include/drm/drm_writeback.h

-- 
2.17.0

[PATCH v8 1/3] drm: Add writeback connector type

[Liviu Dudau]

From: Brian Starkey <brian.starkey@arm.com>

Writeback connectors represent writeback engines which can write the
CRTC output to a memory framebuffer. Add a writeback connector type and
related support functions.

Drivers should initialize a writeback connector with
drm_writeback_connector_init() which takes care of setting up all the
writeback-specific details on top of the normal functionality of
drm_connector_init().

Writeback connectors have a WRITEBACK_FB_ID property, used to set the
output framebuffer, and a WRITEBACK_PIXEL_FORMATS blob used to expose the
supported writeback formats to userspace.

When a framebuffer is attached to a writeback connector with the
WRITEBACK_FB_ID property, it is used only once (for the commit in which
it was included), and userspace can never read back the value of
WRITEBACK_FB_ID. WRITEBACK_FB_ID can only be set if the connector is
attached to a CRTC.

Changes since v1:
 - Added drm_writeback.c + documentation
 - Added helper to initialize writeback connector in one go
 - Added core checks
 - Squashed into a single commit
 - Dropped the client cap
 - Writeback framebuffers are no longer persistent

Changes since v2:
 Daniel Vetter:
 - Subclass drm_connector to drm_writeback_connector
 - Relax check to allow CRTC to be set without an FB
 - Add some writeback_ prefixes
 - Drop PIXEL_FORMATS_SIZE property, as it was unnecessary
 Gustavo Padovan:
 - Add drm_writeback_job to handle writeback signalling centrally

Changes since v3:
 - Rebased
 - Rename PIXEL_FORMATS -> WRITEBACK_PIXEL_FORMATS

Chances since v4:
 - Embed a drm_encoder inside the drm_writeback_connector to
   reduce the amount of boilerplate code required from the drivers
   that are using it.

Changes since v5:
 - Added Rob Clark's atomic_commit() vfunc to connector helper
   funcs, so that writeback jobs are committed from atomic helpers
 - Updated create_writeback_properties() signature to return an
   error code rather than a boolean false for failure.
 - Free writeback job with the connector state rather than when
   doing the cleanup_work()

Changes since v7:
 - fix extraneous use of out_fence that is only introduced in a
   subsequent patch.

Signed-off-by: Brian Starkey <brian.starkey@arm.com>
[rebased and fixed conflicts]
Signed-off-by: Mihail Atanassov <mihail.atanassov@arm.com>
[rebased and added atomic_commit() vfunc for writeback jobs]
Signed-off-by: Rob Clark <robdclark@gmail.com>
Signed-off-by: Liviu Dudau <liviu.dudau@arm.com>
---
 Documentation/gpu/drm-kms.rst            |   9 +
 drivers/gpu/drm/Makefile                 |   2 +-
 drivers/gpu/drm/drm_atomic.c             | 128 ++++++++++++
 drivers/gpu/drm/drm_atomic_helper.c      |  30 +++
 drivers/gpu/drm/drm_connector.c          |   4 +-
 drivers/gpu/drm/drm_writeback.c          | 255 +++++++++++++++++++++++
 include/drm/drm_atomic.h                 |   3 +
 include/drm/drm_connector.h              |  13 ++
 include/drm/drm_mode_config.h            |  15 ++
 include/drm/drm_modeset_helper_vtables.h |  11 +
 include/drm/drm_writeback.h              |  88 ++++++++
 include/uapi/drm/drm_mode.h              |   1 +
 12 files changed, 557 insertions(+), 2 deletions(-)
 create mode 100644 drivers/gpu/drm/drm_writeback.c
 create mode 100644 include/drm/drm_writeback.h

diff --git a/Documentation/gpu/drm-kms.rst b/Documentation/gpu/drm-kms.rst
index 1dffd1ac4cd44..809d403087f95 100644
--- a/Documentation/gpu/drm-kms.rst
+++ b/Documentation/gpu/drm-kms.rst
@@ -373,6 +373,15 @@ Connector Functions Reference
 .. kernel-doc:: drivers/gpu/drm/drm_connector.c
    :export:
 
+Writeback Connectors
+--------------------
+
+.. kernel-doc:: drivers/gpu/drm/drm_writeback.c
+  :doc: overview
+
+.. kernel-doc:: drivers/gpu/drm/drm_writeback.c
+  :export:
+
 Encoder Abstraction
 ===================
 
diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index 50093ff4479b4..3d708959b224c 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -18,7 +18,7 @@ drm-y       :=	drm_auth.o drm_bufs.o drm_cache.o \
 		drm_encoder.o drm_mode_object.o drm_property.o \
 		drm_plane.o drm_color_mgmt.o drm_print.o \
 		drm_dumb_buffers.o drm_mode_config.o drm_vblank.o \
-		drm_syncobj.o drm_lease.o
+		drm_syncobj.o drm_lease.o drm_writeback.o
 
 drm-$(CONFIG_DRM_LIB_RANDOM) += lib/drm_random.o
 drm-$(CONFIG_DRM_VM) += drm_vm.o
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 7d25c42f22dbc..3f1e4b894803b 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -30,6 +30,7 @@
 #include <drm/drm_atomic.h>
 #include <drm/drm_mode.h>
 #include <drm/drm_print.h>
+#include <drm/drm_writeback.h>
 #include <linux/sync_file.h>
 
 #include "drm_crtc_internal.h"
@@ -668,6 +669,45 @@ static void drm_atomic_crtc_print_state(struct drm_printer *p,
 		crtc->funcs->atomic_print_state(p, state);
 }
 
+/**
+ * drm_atomic_connector_check - check connector state
+ * @connector: connector to check
+ * @state: connector state to check
+ *
+ * Provides core sanity checks for connector state.
+ *
+ * RETURNS:
+ * Zero on success, error code on failure
+ */
+static int drm_atomic_connector_check(struct drm_connector *connector,
+		struct drm_connector_state *state)
+{
+	struct drm_crtc_state *crtc_state;
+	struct drm_writeback_job *writeback_job = state->writeback_job;
+
+	if ((connector->connector_type != DRM_MODE_CONNECTOR_WRITEBACK) || !writeback_job)
+		return 0;
+
+	if (writeback_job->fb && !state->crtc) {
+		DRM_DEBUG_ATOMIC("[CONNECTOR:%d:%s] framebuffer without CRTC\n",
+				 connector->base.id, connector->name);
+		return -EINVAL;
+	}
+
+	if (state->crtc)
+		crtc_state = drm_atomic_get_existing_crtc_state(state->state,
+								state->crtc);
+
+	if (writeback_job->fb && !crtc_state->active) {
+		DRM_DEBUG_ATOMIC("[CONNECTOR:%d:%s] has framebuffer, but [CRTC:%d] is off\n",
+				 connector->base.id, connector->name,
+				 state->crtc->base.id);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 /**
  * drm_atomic_get_plane_state - get plane state
  * @state: global atomic state object
@@ -1274,6 +1314,12 @@ static int drm_atomic_connector_set_property(struct drm_connector *connector,
 			return -EINVAL;
 		}
 		state->content_protection = val;
+	} else if (property == config->writeback_fb_id_property) {
+		struct drm_framebuffer *fb = drm_framebuffer_lookup(dev, NULL, val);
+		int ret = drm_atomic_set_writeback_fb_for_connector(state, fb);
+		if (fb)
+			drm_framebuffer_unreference(fb);
+		return ret;
 	} else if (connector->funcs->atomic_set_property) {
 		return connector->funcs->atomic_set_property(connector,
 				state, property, val);
@@ -1355,6 +1401,9 @@ drm_atomic_connector_get_property(struct drm_connector *connector,
 		*val = state->scaling_mode;
 	} else if (property == connector->content_protection_property) {
 		*val = state->content_protection;
+	} else if (property == config->writeback_fb_id_property) {
+		/* Writeback framebuffer is one-shot, write and forget */
+		*val = 0;
 	} else if (connector->funcs->atomic_get_property) {
 		return connector->funcs->atomic_get_property(connector,
 				state, property, val);
@@ -1562,6 +1611,74 @@ drm_atomic_set_crtc_for_connector(struct drm_connector_state *conn_state,
 }
 EXPORT_SYMBOL(drm_atomic_set_crtc_for_connector);
 
+/*
+ * drm_atomic_get_writeback_job - return or allocate a writeback job
+ * @conn_state: Connector state to get the job for
+ *
+ * Writeback jobs have a different lifetime to the atomic state they are
+ * associated with. This convenience function takes care of allocating a job
+ * if there isn't yet one associated with the connector state, otherwise
+ * it just returns the existing job.
+ *
+ * Returns: The writeback job for the given connector state
+ */
+static struct drm_writeback_job *
+drm_atomic_get_writeback_job(struct drm_connector_state *conn_state)
+{
+	WARN_ON(conn_state->connector->connector_type != DRM_MODE_CONNECTOR_WRITEBACK);
+
+	if (!conn_state->writeback_job)
+		conn_state->writeback_job =
+			kzalloc(sizeof(*conn_state->writeback_job), GFP_KERNEL);
+
+	return conn_state->writeback_job;
+}
+
+/**
+ * drm_atomic_set_writeback_fb_for_connector - set writeback framebuffer
+ * @conn_state: atomic state object for the connector
+ * @fb: fb to use for the connector
+ *
+ * This is used to set the framebuffer for a writeback connector, which outputs
+ * to a buffer instead of an actual physical connector.
+ * Changing the assigned framebuffer requires us to grab a reference to the new
+ * fb and drop the reference to the old fb, if there is one. This function
+ * takes care of all these details besides updating the pointer in the
+ * state object itself.
+ *
+ * Note: The only way conn_state can already have an fb set is if the commit
+ * sets the property more than once.
+ *
+ * See also: drm_writeback_connector_init()
+ *
+ * Returns: 0 on success
+ */
+int drm_atomic_set_writeback_fb_for_connector(
+		struct drm_connector_state *conn_state,
+		struct drm_framebuffer *fb)
+{
+	struct drm_writeback_job *job =
+		drm_atomic_get_writeback_job(conn_state);
+	if (!job)
+		return -ENOMEM;
+
+	if (job->fb)
+		drm_framebuffer_unreference(job->fb);
+	if (fb)
+		drm_framebuffer_reference(fb);
+	job->fb = fb;
+
+	if (fb)
+		DRM_DEBUG_ATOMIC("Set [FB:%d] for connector state %p\n",
+				 fb->base.id, conn_state);
+	else
+		DRM_DEBUG_ATOMIC("Set [NOFB] for connector state %p\n",
+				 conn_state);
+
+	return 0;
+}
+EXPORT_SYMBOL(drm_atomic_set_writeback_fb_for_connector);
+
 /**
  * drm_atomic_add_affected_connectors - add connectors for crtc
  * @state: atomic state
@@ -1680,6 +1797,8 @@ int drm_atomic_check_only(struct drm_atomic_state *state)
 	struct drm_plane_state *plane_state;
 	struct drm_crtc *crtc;
 	struct drm_crtc_state *crtc_state;
+	struct drm_connector *conn;
+	struct drm_connector_state *conn_state;
 	int i, ret = 0;
 
 	DRM_DEBUG_ATOMIC("checking %p\n", state);
@@ -1702,6 +1821,15 @@ int drm_atomic_check_only(struct drm_atomic_state *state)
 		}
 	}
 
+	for_each_new_connector_in_state(state, conn, conn_state, i) {
+		ret = drm_atomic_connector_check(conn, conn_state);
+		if (ret) {
+			DRM_DEBUG_ATOMIC("[CONNECTOR:%d:%s] atomic core check failed\n",
+					 conn->base.id, conn->name);
+			return ret;
+		}
+	}
+
 	if (config->funcs->atomic_check)
 		ret = config->funcs->atomic_check(state->dev, state);
 
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index c35654591c124..276f247b2733b 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -30,6 +30,7 @@
 #include <drm/drm_plane_helper.h>
 #include <drm/drm_crtc_helper.h>
 #include <drm/drm_atomic_helper.h>
+#include <drm/drm_writeback.h>
 #include <linux/dma-fence.h>
 
 #include "drm_crtc_helper_internal.h"
@@ -1161,6 +1162,25 @@ void drm_atomic_helper_commit_modeset_disables(struct drm_device *dev,
 }
 EXPORT_SYMBOL(drm_atomic_helper_commit_modeset_disables);
 
+static void drm_atomic_helper_commit_writebacks(struct drm_device *dev,
+						struct drm_atomic_state *old_state)
+{
+	struct drm_connector *connector;
+	struct drm_connector_state *new_conn_state;
+	int i;
+
+	for_each_new_connector_in_state(old_state, connector, new_conn_state, i) {
+		const struct drm_connector_helper_funcs *funcs;
+
+		funcs = connector->helper_private;
+
+		if (new_conn_state->writeback_job && new_conn_state->writeback_job->fb) {
+			WARN_ON(connector->connector_type != DRM_MODE_CONNECTOR_WRITEBACK);
+			funcs->atomic_commit(connector, new_conn_state->writeback_job);
+		}
+	}
+}
+
 /**
  * drm_atomic_helper_commit_modeset_enables - modeset commit to enable outputs
  * @dev: DRM device
@@ -1240,6 +1260,8 @@ void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev,
 
 		drm_bridge_enable(encoder->bridge);
 	}
+
+	drm_atomic_helper_commit_writebacks(dev, old_state);
 }
 EXPORT_SYMBOL(drm_atomic_helper_commit_modeset_enables);
 
@@ -3629,6 +3651,9 @@ __drm_atomic_helper_connector_duplicate_state(struct drm_connector *connector,
 	if (state->crtc)
 		drm_connector_get(connector);
 	state->commit = NULL;
+
+	/* Don't copy over a writeback job, they are used only once */
+	state->writeback_job = NULL;
 }
 EXPORT_SYMBOL(__drm_atomic_helper_connector_duplicate_state);
 
@@ -3758,6 +3783,11 @@ __drm_atomic_helper_connector_destroy_state(struct drm_connector_state *state)
 
 	if (state->commit)
 		drm_crtc_commit_put(state->commit);
+
+	if (state->writeback_job) {
+		drm_writeback_cleanup_job(state->writeback_job);
+		kfree(state->writeback_job);
+	}
 }
 EXPORT_SYMBOL(__drm_atomic_helper_connector_destroy_state);
 
diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index b3cde897cd802..929c547ebb431 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -87,6 +87,7 @@ static struct drm_conn_prop_enum_list drm_connector_enum_list[] = {
 	{ DRM_MODE_CONNECTOR_VIRTUAL, "Virtual" },
 	{ DRM_MODE_CONNECTOR_DSI, "DSI" },
 	{ DRM_MODE_CONNECTOR_DPI, "DPI" },
+	{ DRM_MODE_CONNECTOR_WRITEBACK, "Writeback" },
 };
 
 void drm_connector_ida_init(void)
@@ -249,7 +250,8 @@ int drm_connector_init(struct drm_device *dev,
 	config->num_connector++;
 	spin_unlock_irq(&config->connector_list_lock);
 
-	if (connector_type != DRM_MODE_CONNECTOR_VIRTUAL)
+	if (connector_type != DRM_MODE_CONNECTOR_VIRTUAL &&
+	    connector_type != DRM_MODE_CONNECTOR_WRITEBACK)
 		drm_object_attach_property(&connector->base,
 					      config->edid_property,
 					      0);
diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c
new file mode 100644
index 0000000000000..1141325b004ab
--- /dev/null
+++ b/drivers/gpu/drm/drm_writeback.c
@@ -0,0 +1,255 @@
+/*
+ * (C) COPYRIGHT 2016 ARM Limited. All rights reserved.
+ * Author: Brian Starkey <brian.starkey@arm.com>
+ *
+ * This program is free software and is provided to you under the terms of the
+ * GNU General Public License version 2 as published by the Free Software
+ * Foundation, and any use by you of this program is subject to the terms
+ * of such GNU licence.
+ */
+
+#include <drm/drm_crtc.h>
+#include <drm/drm_modeset_helper_vtables.h>
+#include <drm/drm_property.h>
+#include <drm/drm_writeback.h>
+#include <drm/drmP.h>
+
+/**
+ * DOC: overview
+ *
+ * Writeback connectors are used to expose hardware which can write the output
+ * from a CRTC to a memory buffer. They are used and act similarly to other
+ * types of connectors, with some important differences:
+ *  - Writeback connectors don't provide a way to output visually to the user.
+ *  - Writeback connectors should always report as "disconnected" (so that
+ *    clients which don't understand them will ignore them).
+ *  - Writeback connectors don't have EDID.
+ *
+ * A framebuffer may only be attached to a writeback connector when the
+ * connector is attached to a CRTC. The WRITEBACK_FB_ID property which sets the
+ * framebuffer applies only to a single commit (see below). A framebuffer may
+ * not be attached while the CRTC is off.
+ *
+ * Writeback connectors have some additional properties, which userspace
+ * can use to query and control them:
+ *
+ *  "WRITEBACK_FB_ID":
+ *	Write-only object property storing a DRM_MODE_OBJECT_FB: it stores the
+ *	framebuffer to be written by the writeback connector. This property is
+ *	similar to the FB_ID property on planes, but will always read as zero
+ *	and is not preserved across commits.
+ *	Userspace must set this property to an output buffer every time it
+ *	wishes the buffer to get filled.
+ *
+ *  "WRITEBACK_PIXEL_FORMATS":
+ *	Immutable blob property to store the supported pixel formats table. The
+ *	data is an array of u32 DRM_FORMAT_* fourcc values.
+ *	Userspace can use this blob to find out what pixel formats are supported
+ *	by the connector's writeback engine.
+ */
+
+static int create_writeback_properties(struct drm_device *dev)
+{
+	struct drm_property *prop;
+
+	if (!dev->mode_config.writeback_fb_id_property) {
+		prop = drm_property_create_object(dev, DRM_MODE_PROP_ATOMIC,
+						  "WRITEBACK_FB_ID",
+						  DRM_MODE_OBJECT_FB);
+		if (!prop)
+			return -ENOMEM;
+		dev->mode_config.writeback_fb_id_property = prop;
+	}
+
+	if (!dev->mode_config.writeback_pixel_formats_property) {
+		prop = drm_property_create(dev, DRM_MODE_PROP_BLOB |
+					   DRM_MODE_PROP_ATOMIC |
+					   DRM_MODE_PROP_IMMUTABLE,
+					   "WRITEBACK_PIXEL_FORMATS", 0);
+		if (!prop)
+			return -ENOMEM;
+		dev->mode_config.writeback_pixel_formats_property = prop;
+	}
+
+	return 0;
+}
+
+static const struct drm_encoder_funcs drm_writeback_encoder_funcs = {
+	.destroy = drm_encoder_cleanup,
+};
+
+/**
+ * drm_writeback_connector_init - Initialize a writeback connector and its properties
+ * @dev: DRM device
+ * @wb_connector: Writeback connector to initialize
+ * @con_funcs: Connector funcs vtable
+ * @enc_helper_funcs: Encoder helper funcs vtable to be used by the internal encoder
+ * @formats: Array of supported pixel formats for the writeback engine
+ * @n_formats: Length of the formats array
+ *
+ * This function creates the writeback-connector-specific properties if they
+ * have not been already created, initializes the connector as
+ * type DRM_MODE_CONNECTOR_WRITEBACK, and correctly initializes the property
+ * values. It will also create an internal encoder associated with the
+ * drm_writeback_connector and set it to use the @enc_helper_funcs vtable for
+ * the encoder helper.
+ *
+ * Drivers should always use this function instead of drm_connector_init() to
+ * set up writeback connectors.
+ *
+ * Returns: 0 on success, or a negative error code
+ */
+int drm_writeback_connector_init(struct drm_device *dev,
+				 struct drm_writeback_connector *wb_connector,
+				 const struct drm_connector_funcs *con_funcs,
+				 const struct drm_encoder_helper_funcs *enc_helper_funcs,
+				 const u32 *formats, int n_formats)
+{
+	struct drm_property_blob *blob;
+	struct drm_connector *connector = &wb_connector->base;
+	struct drm_mode_config *config = &dev->mode_config;
+	int ret = create_writeback_properties(dev);
+
+	if (ret != 0)
+		return ret;
+
+	blob = drm_property_create_blob(dev, n_formats * sizeof(*formats),
+					formats);
+	if (IS_ERR(blob))
+		return PTR_ERR(blob);
+
+	drm_encoder_helper_add(&wb_connector->encoder, enc_helper_funcs);
+	ret = drm_encoder_init(dev, &wb_connector->encoder,
+			       &drm_writeback_encoder_funcs,
+			       DRM_MODE_ENCODER_VIRTUAL, NULL);
+	if (ret)
+		goto fail;
+
+	connector->interlace_allowed = 0;
+
+	ret = drm_connector_init(dev, connector, con_funcs,
+				 DRM_MODE_CONNECTOR_WRITEBACK);
+	if (ret)
+		goto connector_fail;
+
+	ret = drm_mode_connector_attach_encoder(connector,
+						&wb_connector->encoder);
+	if (ret)
+		goto attach_fail;
+
+	INIT_LIST_HEAD(&wb_connector->job_queue);
+	spin_lock_init(&wb_connector->job_lock);
+
+	drm_object_attach_property(&connector->base,
+				   config->writeback_fb_id_property, 0);
+
+	drm_object_attach_property(&connector->base,
+				   config->writeback_pixel_formats_property,
+				   blob->base.id);
+	wb_connector->pixel_formats_blob_ptr = blob;
+
+	return 0;
+
+attach_fail:
+	drm_connector_cleanup(connector);
+connector_fail:
+	drm_encoder_cleanup(&wb_connector->encoder);
+fail:
+	drm_property_unreference_blob(blob);
+	return ret;
+}
+EXPORT_SYMBOL(drm_writeback_connector_init);
+
+/**
+ * drm_writeback_queue_job - Queue a writeback job for later signalling
+ * @wb_connector: The writeback connector to queue a job on
+ * @job: The job to queue
+ *
+ * This function adds a job to the job_queue for a writeback connector. It
+ * should be considered to take ownership of the writeback job, and so any other
+ * references to the job must be cleared after calling this function.
+ *
+ * Drivers must ensure that for a given writeback connector, jobs are queued in
+ * exactly the same order as they will be completed by the hardware (and
+ * signaled via drm_writeback_signal_completion).
+ *
+ * For every call to drm_writeback_queue_job() there must be exactly one call to
+ * drm_writeback_signal_completion()
+ *
+ * See also: drm_writeback_signal_completion()
+ */
+void drm_writeback_queue_job(struct drm_writeback_connector *wb_connector,
+			     struct drm_writeback_job *job)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&wb_connector->job_lock, flags);
+	list_add_tail(&job->list_entry, &wb_connector->job_queue);
+	spin_unlock_irqrestore(&wb_connector->job_lock, flags);
+}
+EXPORT_SYMBOL(drm_writeback_queue_job);
+
+/**
+ * drm_writeback_cleanup_job - Cleanup and free a writeback job
+ * @job: The writeback job to free
+ *
+ * Drops any references held by the writeback job, and frees the structure.
+ */
+void drm_writeback_cleanup_job(struct drm_writeback_job *job)
+{
+	if (job->fb)
+		drm_framebuffer_unreference(job->fb);
+}
+EXPORT_SYMBOL(drm_writeback_cleanup_job);
+
+/*
+ * @cleanup_work: deferred cleanup of a writeback job
+ *
+ * The job cannot be cleaned up directly in drm_writeback_signal_completion,
+ * because it may be called in interrupt context. Dropping the framebuffer
+ * reference can sleep, and so the cleanup is deferred to a workqueue.
+ */
+static void cleanup_work(struct work_struct *work)
+{
+	struct drm_writeback_job *job = container_of(work,
+						     struct drm_writeback_job,
+						     cleanup_work);
+	drm_writeback_cleanup_job(job);
+}
+
+/**
+ * drm_writeback_signal_completion - Signal the completion of a writeback job
+ * @wb_connector: The writeback connector whose job is complete
+ *
+ * Drivers should call this to signal the completion of a previously queued
+ * writeback job. It should be called as soon as possible after the hardware
+ * has finished writing, and may be called from interrupt context.
+ * It is the driver's responsibility to ensure that for a given connector, the
+ * hardware completes writeback jobs in the same order as they are queued.
+ *
+ * Unless the driver is holding its own reference to the framebuffer, it must
+ * not be accessed after calling this function.
+ *
+ * See also: drm_writeback_queue_job()
+ */
+void
+drm_writeback_signal_completion(struct drm_writeback_connector *wb_connector)
+{
+	unsigned long flags;
+	struct drm_writeback_job *job;
+
+	spin_lock_irqsave(&wb_connector->job_lock, flags);
+	job = list_first_entry_or_null(&wb_connector->job_queue,
+				       struct drm_writeback_job,
+				       list_entry);
+	if (job)
+		list_del(&job->list_entry);
+	spin_unlock_irqrestore(&wb_connector->job_lock, flags);
+
+	if (WARN_ON(!job))
+		return;
+
+	INIT_WORK(&job->cleanup_work, cleanup_work);
+	queue_work(system_long_wq, &job->cleanup_work);
+}
+EXPORT_SYMBOL(drm_writeback_signal_completion);
diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
index a57a8aa90ffb7..a5e904105db3a 100644
--- a/include/drm/drm_atomic.h
+++ b/include/drm/drm_atomic.h
@@ -594,6 +594,9 @@ void drm_atomic_set_fence_for_plane(struct drm_plane_state *plane_state,
 int __must_check
 drm_atomic_set_crtc_for_connector(struct drm_connector_state *conn_state,
 				  struct drm_crtc *crtc);
+int drm_atomic_set_writeback_fb_for_connector(
+		struct drm_connector_state *conn_state,
+		struct drm_framebuffer *fb);
 int __must_check
 drm_atomic_add_affected_connectors(struct drm_atomic_state *state,
 				   struct drm_crtc *crtc);
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index 675cc3f8cf852..60c0ffb947d5e 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -429,6 +429,19 @@ struct drm_connector_state {
 	 * protection. This is most commonly used for HDCP.
 	 */
 	unsigned int content_protection;
+
+	/**
+	 * @writeback_job: Writeback job for writeback connectors
+	 *
+	 * Holds the framebuffer for a writeback connector. As the writeback
+	 * completion may be asynchronous to the normal commit cycle, the
+	 * writeback job lifetime is managed separately from the normal atomic
+	 * state by this object.
+	 *
+	 * See also: drm_writeback_queue_job() and
+	 * drm_writeback_signal_completion()
+	 */
+	struct drm_writeback_job *writeback_job;
 };
 
 /**
diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
index 7569f22ffef6b..de8881324f734 100644
--- a/include/drm/drm_mode_config.h
+++ b/include/drm/drm_mode_config.h
@@ -779,6 +779,21 @@ struct drm_mode_config {
 	 */
 	struct drm_property *panel_orientation_property;
 
+	/**
+	 * @writeback_fb_id_property: Property for writeback connectors, storing
+	 * the ID of the output framebuffer.
+	 * See also: drm_writeback_connector_init()
+	 */
+	struct drm_property *writeback_fb_id_property;
+
+	/**
+	 * @writeback_pixel_formats_property: Property for writeback connectors,
+	 * storing an array of the supported pixel formats for the writeback
+	 * engine (read-only).
+	 * See also: drm_writeback_connector_init()
+	 */
+	struct drm_property *writeback_pixel_formats_property;
+
 	/* dumb ioctl parameters */
 	uint32_t preferred_depth, prefer_shadow;
 
diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h
index 3e76ca805b0f4..636deaf52325a 100644
--- a/include/drm/drm_modeset_helper_vtables.h
+++ b/include/drm/drm_modeset_helper_vtables.h
@@ -974,6 +974,17 @@ struct drm_connector_helper_funcs {
 	 */
 	int (*atomic_check)(struct drm_connector *connector,
 			    struct drm_connector_state *state);
+
+	/**
+	 * @atomic_commit:
+	 *
+	 * This hook is to be used by drivers implementing writeback connectors
+	 * that need a point when to commit the writeback job to the hardware.
+	 *
+	 * This callback is used by the atomic modeset helpers.
+	 */
+	void (*atomic_commit)(struct drm_connector *connector,
+			      struct drm_writeback_job *writeback_job);
 };
 
 /**
diff --git a/include/drm/drm_writeback.h b/include/drm/drm_writeback.h
new file mode 100644
index 0000000000000..cf3a28676006a
--- /dev/null
+++ b/include/drm/drm_writeback.h
@@ -0,0 +1,88 @@
+/*
+ * (C) COPYRIGHT 2016 ARM Limited. All rights reserved.
+ * Author: Brian Starkey <brian.starkey@arm.com>
+ *
+ * This program is free software and is provided to you under the terms of the
+ * GNU General Public License version 2 as published by the Free Software
+ * Foundation, and any use by you of this program is subject to the terms
+ * of such GNU licence.
+ */
+
+#ifndef __DRM_WRITEBACK_H__
+#define __DRM_WRITEBACK_H__
+#include <drm/drm_connector.h>
+#include <drm/drm_encoder.h>
+#include <linux/workqueue.h>
+
+struct drm_writeback_connector {
+	struct drm_connector base;
+
+	/**
+	 * @encoder: Internal encoder used by the connector to fulfill
+	 * the DRM framework requirements. The users of the
+	 * @drm_writeback_connector control the behaviour of the @encoder
+	 * by passing the @enc_funcs parameter to drm_writeback_connector_init()
+	 * function.
+	 */
+	struct drm_encoder encoder;
+
+	/**
+	 * @pixel_formats_blob_ptr:
+	 *
+	 * DRM blob property data for the pixel formats list on writeback
+	 * connectors
+	 * See also drm_writeback_connector_init()
+	 */
+	struct drm_property_blob *pixel_formats_blob_ptr;
+
+	/** @job_lock: Protects job_queue */
+	spinlock_t job_lock;
+
+	/**
+	 * @job_queue:
+	 *
+	 * Holds a list of a connector's writeback jobs; the last item is the
+	 * most recent. The first item may be either waiting for the hardware
+	 * to begin writing, or currently being written.
+	 *
+	 * See also: drm_writeback_queue_job() and
+	 * drm_writeback_signal_completion()
+	 */
+	struct list_head job_queue;
+};
+
+struct drm_writeback_job {
+	/**
+	 * @cleanup_work:
+	 *
+	 * Used to allow drm_writeback_signal_completion to defer dropping the
+	 * framebuffer reference to a workqueue.
+	 */
+	struct work_struct cleanup_work;
+	/**
+	 * @list_entry:
+	 *
+	 * List item for the connector's @job_queue
+	 */
+	struct list_head list_entry;
+	/**
+	 * @fb:
+	 *
+	 * Framebuffer to be written to by the writeback connector. Do not set
+	 * directly, use drm_atomic_set_writeback_fb_for_connector()
+	 */
+	struct drm_framebuffer *fb;
+};
+
+int drm_writeback_connector_init(struct drm_device *dev,
+				 struct drm_writeback_connector *wb_connector,
+				 const struct drm_connector_funcs *con_funcs,
+				 const struct drm_encoder_helper_funcs *enc_helper_funcs,
+				 const u32 *formats, int n_formats);
+
+void drm_writeback_queue_job(struct drm_writeback_connector *wb_connector,
+			     struct drm_writeback_job *job);
+
+void drm_writeback_cleanup_job(struct drm_writeback_job *job);
+void drm_writeback_signal_completion(struct drm_writeback_connector *wb_connector);
+#endif
diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
index 50bcf4214ff97..704d67ab1235e 100644
--- a/include/uapi/drm/drm_mode.h
+++ b/include/uapi/drm/drm_mode.h
@@ -338,6 +338,7 @@ enum drm_mode_subconnector {
 #define DRM_MODE_CONNECTOR_VIRTUAL      15
 #define DRM_MODE_CONNECTOR_DSI		16
 #define DRM_MODE_CONNECTOR_DPI		17
+#define DRM_MODE_CONNECTOR_WRITEBACK	18
 
 struct drm_mode_get_connector {
 
-- 
2.17.0

[PATCH v8 2/3] drm: writeback: Add out-fences for writeback connectors

[Liviu Dudau]

From: Brian Starkey <brian.starkey@arm.com>

Add the WRITEBACK_OUT_FENCE_PTR property to writeback connectors, to
enable userspace to get a fence which will signal once the writeback is
complete. It is not allowed to request an out-fence without a
framebuffer attached to the connector.

A timeline is added to drm_writeback_connector for use by the writeback
out-fences.

In the case of a commit failure or DRM_MODE_ATOMIC_TEST_ONLY, the fence
is set to -1.

Changes from v2:
 - Rebase onto Gustavo Padovan's v9 explicit sync series
 - Change out_fence_ptr type to s32 __user *
 - Set *out_fence_ptr to -1 in drm_atomic_connector_set_property
 - Store fence in drm_writeback_job
 Gustavo Padovan:
 - Move out_fence_ptr out of connector_state
 - Signal fence from drm_writeback_signal_completion instead of
   in driver directly

Changes from v3:
 - Rebase onto commit 7e9081c5aac7 ("drm/fence: fix memory overwrite
   when setting out_fence fd") (change out_fence_ptr to s32 __user *,
   for real this time.)
 - Update documentation around WRITEBACK_OUT_FENCE_PTR

Signed-off-by: Brian Starkey <brian.starkey@arm.com>
[rebased and fixed conflicts]
Signed-off-by: Mihail Atanassov <mihail.atanassov@arm.com>
Signed-off-by: Liviu Dudau <liviu.dudau@arm.com>
---
 drivers/gpu/drm/drm_atomic.c    |  99 ++++++++++++++++++++++++++---
 drivers/gpu/drm/drm_writeback.c | 109 +++++++++++++++++++++++++++++++-
 include/drm/drm_atomic.h        |   8 +++
 include/drm/drm_connector.h     |   8 +--
 include/drm/drm_mode_config.h   |   8 +++
 include/drm/drm_writeback.h     |  43 ++++++++++++-
 6 files changed, 259 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 3f1e4b894803b..cc2f86c68b293 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -318,6 +318,35 @@ static s32 __user *get_out_fence_for_crtc(struct drm_atomic_state *state,
 	return fence_ptr;
 }
 
+static int set_out_fence_for_connector(struct drm_atomic_state *state,
+					struct drm_connector *connector,
+					s32 __user *fence_ptr)
+{
+	unsigned int index = drm_connector_index(connector);
+
+	if (!fence_ptr)
+		return 0;
+
+	if (put_user(-1, fence_ptr))
+		return -EFAULT;
+
+	state->connectors[index].out_fence_ptr = fence_ptr;
+
+	return 0;
+}
+
+static s32 __user *get_out_fence_for_connector(struct drm_atomic_state *state,
+					       struct drm_connector *connector)
+{
+	unsigned int index = drm_connector_index(connector);
+	s32 __user *fence_ptr;
+
+	fence_ptr = state->connectors[index].out_fence_ptr;
+	state->connectors[index].out_fence_ptr = NULL;
+
+	return fence_ptr;
+}
+
 /**
  * drm_atomic_set_mode_for_crtc - set mode for CRTC
  * @state: the CRTC whose incoming state to update
@@ -705,6 +734,12 @@ static int drm_atomic_connector_check(struct drm_connector *connector,
 		return -EINVAL;
 	}
 
+	if (writeback_job->out_fence && !writeback_job->fb) {
+		DRM_DEBUG_ATOMIC("[CONNECTOR:%d:%s] requesting out-fence without framebuffer\n",
+				 connector->base.id, connector->name);
+		return -EINVAL;
+	}
+
 	return 0;
 }
 
@@ -1320,6 +1355,11 @@ static int drm_atomic_connector_set_property(struct drm_connector *connector,
 		if (fb)
 			drm_framebuffer_unreference(fb);
 		return ret;
+	} else if (property == config->writeback_out_fence_ptr_property) {
+		s32 __user *fence_ptr = u64_to_user_ptr(val);
+
+		return set_out_fence_for_connector(state->state, connector,
+						   fence_ptr);
 	} else if (connector->funcs->atomic_set_property) {
 		return connector->funcs->atomic_set_property(connector,
 				state, property, val);
@@ -1404,6 +1444,8 @@ drm_atomic_connector_get_property(struct drm_connector *connector,
 	} else if (property == config->writeback_fb_id_property) {
 		/* Writeback framebuffer is one-shot, write and forget */
 		*val = 0;
+	} else if (property == config->writeback_out_fence_ptr_property) {
+		*val = 0;
 	} else if (connector->funcs->atomic_get_property) {
 		return connector->funcs->atomic_get_property(connector,
 				state, property, val);
@@ -2263,7 +2305,7 @@ static int setup_out_fence(struct drm_out_fence_state *fence_state,
 	return 0;
 }
 
-static int prepare_crtc_signaling(struct drm_device *dev,
+static int prepare_signaling(struct drm_device *dev,
 				  struct drm_atomic_state *state,
 				  struct drm_mode_atomic *arg,
 				  struct drm_file *file_priv,
@@ -2272,6 +2314,8 @@ static int prepare_crtc_signaling(struct drm_device *dev,
 {
 	struct drm_crtc *crtc;
 	struct drm_crtc_state *crtc_state;
+	struct drm_connector *conn;
+	struct drm_connector_state *conn_state;
 	int i, c = 0, ret;
 
 	if (arg->flags & DRM_MODE_ATOMIC_TEST_ONLY)
@@ -2337,6 +2381,43 @@ static int prepare_crtc_signaling(struct drm_device *dev,
 		c++;
 	}
 
+	for_each_new_connector_in_state(state, conn, conn_state, i) {
+		struct drm_writeback_job *job;
+		struct drm_out_fence_state *f;
+		struct dma_fence *fence;
+		s32 __user *fence_ptr;
+
+		fence_ptr = get_out_fence_for_connector(state, conn);
+		if (!fence_ptr)
+			continue;
+
+		job = drm_atomic_get_writeback_job(conn_state);
+		if (!job)
+			return -ENOMEM;
+
+		f = krealloc(*fence_state, sizeof(**fence_state) *
+			     (*num_fences + 1), GFP_KERNEL);
+		if (!f)
+			return -ENOMEM;
+
+		memset(&f[*num_fences], 0, sizeof(*f));
+
+		f[*num_fences].out_fence_ptr = fence_ptr;
+		*fence_state = f;
+
+		fence = drm_writeback_get_out_fence((struct drm_writeback_connector *)conn);
+		if (!fence)
+			return -ENOMEM;
+
+		ret = setup_out_fence(&f[(*num_fences)++], fence);
+		if (ret) {
+			dma_fence_put(fence);
+			return ret;
+		}
+
+		job->out_fence = fence;
+	}
+
 	/*
 	 * Having this flag means user mode pends on event which will never
 	 * reach due to lack of at least one CRTC for signaling
@@ -2347,11 +2428,11 @@ static int prepare_crtc_signaling(struct drm_device *dev,
 	return 0;
 }
 
-static void complete_crtc_signaling(struct drm_device *dev,
-				    struct drm_atomic_state *state,
-				    struct drm_out_fence_state *fence_state,
-				    unsigned int num_fences,
-				    bool install_fds)
+static void complete_signaling(struct drm_device *dev,
+			       struct drm_atomic_state *state,
+			       struct drm_out_fence_state *fence_state,
+			       unsigned int num_fences,
+			       bool install_fds)
 {
 	struct drm_crtc *crtc;
 	struct drm_crtc_state *crtc_state;
@@ -2530,8 +2611,8 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
 		drm_mode_object_put(obj);
 	}
 
-	ret = prepare_crtc_signaling(dev, state, arg, file_priv, &fence_state,
-				     &num_fences);
+	ret = prepare_signaling(dev, state, arg, file_priv, &fence_state,
+				&num_fences);
 	if (ret)
 		goto out;
 
@@ -2549,7 +2630,7 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
 out:
 	drm_atomic_clean_old_fb(dev, plane_mask, ret);
 
-	complete_crtc_signaling(dev, state, fence_state, num_fences, !ret);
+	complete_signaling(dev, state, fence_state, num_fences, !ret);
 
 	if (ret == -EDEADLK) {
 		drm_atomic_state_clear(state);
diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c
index 1141325b004ab..46a93a883344f 100644
--- a/drivers/gpu/drm/drm_writeback.c
+++ b/drivers/gpu/drm/drm_writeback.c
@@ -13,6 +13,7 @@
 #include <drm/drm_property.h>
 #include <drm/drm_writeback.h>
 #include <drm/drmP.h>
+#include <linux/dma-fence.h>
 
 /**
  * DOC: overview
@@ -30,6 +31,16 @@
  * framebuffer applies only to a single commit (see below). A framebuffer may
  * not be attached while the CRTC is off.
  *
+ * Unlike with planes, when a writeback framebuffer is removed by userspace DRM
+ * makes no attempt to remove it from active use by the connector. This is
+ * because no method is provided to abort a writeback operation, and in any
+ * case making a new commit whilst a writeback is ongoing is undefined (see
+ * WRITEBACK_OUT_FENCE_PTR below). As soon as the current writeback is finished,
+ * the framebuffer will automatically no longer be in active use. As it will
+ * also have already been removed from the framebuffer list, there will be no
+ * way for any userspace application to retrieve a reference to it in the
+ * intervening period.
+ *
  * Writeback connectors have some additional properties, which userspace
  * can use to query and control them:
  *
@@ -46,8 +57,54 @@
  *	data is an array of u32 DRM_FORMAT_* fourcc values.
  *	Userspace can use this blob to find out what pixel formats are supported
  *	by the connector's writeback engine.
+ *
+ *  "WRITEBACK_OUT_FENCE_PTR":
+ *	Userspace can use this property to provide a pointer for the kernel to
+ *	fill with a sync_file file descriptor, which will signal once the
+ *	writeback is finished. The value should be the address of a 32-bit
+ *	signed integer, cast to a u64.
+ *	Userspace should wait for this fence to signal before making another
+ *	commit affecting any of the same CRTCs, Planes or Connectors.
+ *	**Failure to do so will result in undefined behaviour.**
+ *	For this reason it is strongly recommended that all userspace
+ *	applications making use of writeback connectors *always* retrieve an
+ *	out-fence for the commit and use it appropriately.
+ *	From userspace, this property will always read as zero.
  */
 
+#define fence_to_wb_connector(x) container_of(x->lock, \
+					      struct drm_writeback_connector, \
+					      fence_lock)
+
+static const char *drm_writeback_fence_get_driver_name(struct dma_fence *fence)
+{
+	struct drm_writeback_connector *wb_connector =
+		fence_to_wb_connector(fence);
+
+	return wb_connector->base.dev->driver->name;
+}
+
+static const char *
+drm_writeback_fence_get_timeline_name(struct dma_fence *fence)
+{
+	struct drm_writeback_connector *wb_connector =
+		fence_to_wb_connector(fence);
+
+	return wb_connector->timeline_name;
+}
+
+static bool drm_writeback_fence_enable_signaling(struct dma_fence *fence)
+{
+	return true;
+}
+
+static const struct dma_fence_ops drm_writeback_fence_ops = {
+	.get_driver_name = drm_writeback_fence_get_driver_name,
+	.get_timeline_name = drm_writeback_fence_get_timeline_name,
+	.enable_signaling = drm_writeback_fence_enable_signaling,
+	.wait = dma_fence_default_wait,
+};
+
 static int create_writeback_properties(struct drm_device *dev)
 {
 	struct drm_property *prop;
@@ -71,6 +128,15 @@ static int create_writeback_properties(struct drm_device *dev)
 		dev->mode_config.writeback_pixel_formats_property = prop;
 	}
 
+	if (!dev->mode_config.writeback_out_fence_ptr_property) {
+		prop = drm_property_create_range(dev, DRM_MODE_PROP_ATOMIC,
+						 "WRITEBACK_OUT_FENCE_PTR", 0,
+						 U64_MAX);
+		if (!prop)
+			return -ENOMEM;
+		dev->mode_config.writeback_out_fence_ptr_property = prop;
+	}
+
 	return 0;
 }
 
@@ -140,6 +206,15 @@ int drm_writeback_connector_init(struct drm_device *dev,
 	INIT_LIST_HEAD(&wb_connector->job_queue);
 	spin_lock_init(&wb_connector->job_lock);
 
+	wb_connector->fence_context = dma_fence_context_alloc(1);
+	spin_lock_init(&wb_connector->fence_lock);
+	snprintf(wb_connector->timeline_name,
+		 sizeof(wb_connector->timeline_name),
+		 "CONNECTOR:%d-%s", connector->base.id, connector->name);
+
+	drm_object_attach_property(&connector->base,
+				   config->writeback_out_fence_ptr_property, 0);
+
 	drm_object_attach_property(&connector->base,
 				   config->writeback_fb_id_property, 0);
 
@@ -199,6 +274,7 @@ void drm_writeback_cleanup_job(struct drm_writeback_job *job)
 {
 	if (job->fb)
 		drm_framebuffer_unreference(job->fb);
+	dma_fence_put(job->out_fence);
 }
 EXPORT_SYMBOL(drm_writeback_cleanup_job);
 
@@ -220,6 +296,7 @@ static void cleanup_work(struct work_struct *work)
 /**
  * drm_writeback_signal_completion - Signal the completion of a writeback job
  * @wb_connector: The writeback connector whose job is complete
+ * @status: Status code to set in the writeback out_fence (0 for success)
  *
  * Drivers should call this to signal the completion of a previously queued
  * writeback job. It should be called as soon as possible after the hardware
@@ -233,7 +310,8 @@ static void cleanup_work(struct work_struct *work)
  * See also: drm_writeback_queue_job()
  */
 void
-drm_writeback_signal_completion(struct drm_writeback_connector *wb_connector)
+drm_writeback_signal_completion(struct drm_writeback_connector *wb_connector,
+				int status)
 {
 	unsigned long flags;
 	struct drm_writeback_job *job;
@@ -242,8 +320,14 @@ drm_writeback_signal_completion(struct drm_writeback_connector *wb_connector)
 	job = list_first_entry_or_null(&wb_connector->job_queue,
 				       struct drm_writeback_job,
 				       list_entry);
-	if (job)
+	if (job) {
 		list_del(&job->list_entry);
+		if (job->out_fence) {
+			if (status)
+				dma_fence_set_error(job->out_fence, status);
+			dma_fence_signal(job->out_fence);
+		}
+	}
 	spin_unlock_irqrestore(&wb_connector->job_lock, flags);
 
 	if (WARN_ON(!job))
@@ -253,3 +337,24 @@ drm_writeback_signal_completion(struct drm_writeback_connector *wb_connector)
 	queue_work(system_long_wq, &job->cleanup_work);
 }
 EXPORT_SYMBOL(drm_writeback_signal_completion);
+
+struct dma_fence *
+drm_writeback_get_out_fence(struct drm_writeback_connector *wb_connector)
+{
+	struct dma_fence *fence;
+
+	if (WARN_ON(wb_connector->base.connector_type !=
+		    DRM_MODE_CONNECTOR_WRITEBACK))
+		return NULL;
+
+	fence = kzalloc(sizeof(*fence), GFP_KERNEL);
+	if (!fence)
+		return NULL;
+
+	dma_fence_init(fence, &drm_writeback_fence_ops,
+		       &wb_connector->fence_lock, wb_connector->fence_context,
+		       ++wb_connector->fence_seqno);
+
+	return fence;
+}
+EXPORT_SYMBOL(drm_writeback_get_out_fence);
diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
index a5e904105db3a..7c77a1916182a 100644
--- a/include/drm/drm_atomic.h
+++ b/include/drm/drm_atomic.h
@@ -160,6 +160,14 @@ struct __drm_crtcs_state {
 struct __drm_connnectors_state {
 	struct drm_connector *ptr;
 	struct drm_connector_state *state, *old_state, *new_state;
+	/**
+	 * @out_fence_ptr:
+	 *
+	 * User-provided pointer which the kernel uses to return a sync_file
+	 * file descriptor. Used by writeback connectors to signal completion of
+	 * the writeback.
+	 */
+	s32 __user *out_fence_ptr;
 };
 
 struct drm_private_obj;
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index 60c0ffb947d5e..b0d2ed13d625b 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -433,10 +433,10 @@ struct drm_connector_state {
 	/**
 	 * @writeback_job: Writeback job for writeback connectors
 	 *
-	 * Holds the framebuffer for a writeback connector. As the writeback
-	 * completion may be asynchronous to the normal commit cycle, the
-	 * writeback job lifetime is managed separately from the normal atomic
-	 * state by this object.
+	 * Holds the framebuffer and out-fence for a writeback connector. As
+	 * the writeback completion may be asynchronous to the normal commit
+	 * cycle, the writeback job lifetime is managed separately from the
+	 * normal atomic state by this object.
 	 *
 	 * See also: drm_writeback_queue_job() and
 	 * drm_writeback_signal_completion()
diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
index de8881324f734..74fe26aaf5444 100644
--- a/include/drm/drm_mode_config.h
+++ b/include/drm/drm_mode_config.h
@@ -793,6 +793,14 @@ struct drm_mode_config {
 	 * See also: drm_writeback_connector_init()
 	 */
 	struct drm_property *writeback_pixel_formats_property;
+	/**
+	 * @writeback_out_fence_ptr_property: Property for writeback connectors,
+	 * fd pointer representing the outgoing fences for a writeback
+	 * connector. Userspace should provide a pointer to a value of type s32,
+	 * and then cast that pointer to u64.
+	 * See also: drm_writeback_connector_init()
+	 */
+	struct drm_property *writeback_out_fence_ptr_property;
 
 	/* dumb ioctl parameters */
 	uint32_t preferred_depth, prefer_shadow;
diff --git a/include/drm/drm_writeback.h b/include/drm/drm_writeback.h
index cf3a28676006a..6a7462c1821ad 100644
--- a/include/drm/drm_writeback.h
+++ b/include/drm/drm_writeback.h
@@ -49,6 +49,32 @@ struct drm_writeback_connector {
 	 * drm_writeback_signal_completion()
 	 */
 	struct list_head job_queue;
+
+	/**
+	 * @fence_context:
+	 *
+	 * timeline context used for fence operations.
+	 */
+	unsigned int fence_context;
+	/**
+	 * @fence_lock:
+	 *
+	 * spinlock to protect the fences in the fence_context.
+	 */
+	spinlock_t fence_lock;
+	/**
+	 * @fence_seqno:
+	 *
+	 * Seqno variable used as monotonic counter for the fences
+	 * created on the connector's timeline.
+	 */
+	unsigned long fence_seqno;
+	/**
+	 * @timeline_name:
+	 *
+	 * The name of the connector's fence timeline.
+	 */
+	char timeline_name[32];
 };
 
 struct drm_writeback_job {
@@ -59,12 +85,14 @@ struct drm_writeback_job {
 	 * framebuffer reference to a workqueue.
 	 */
 	struct work_struct cleanup_work;
+
 	/**
 	 * @list_entry:
 	 *
 	 * List item for the connector's @job_queue
 	 */
 	struct list_head list_entry;
+
 	/**
 	 * @fb:
 	 *
@@ -72,6 +100,13 @@ struct drm_writeback_job {
 	 * directly, use drm_atomic_set_writeback_fb_for_connector()
 	 */
 	struct drm_framebuffer *fb;
+
+	/**
+	 * @out_fence:
+	 *
+	 * Fence which will signal once the writeback has completed
+	 */
+	struct dma_fence *out_fence;
 };
 
 int drm_writeback_connector_init(struct drm_device *dev,
@@ -84,5 +119,11 @@ void drm_writeback_queue_job(struct drm_writeback_connector *wb_connector,
 			     struct drm_writeback_job *job);
 
 void drm_writeback_cleanup_job(struct drm_writeback_job *job);
-void drm_writeback_signal_completion(struct drm_writeback_connector *wb_connector);
+
+void
+drm_writeback_signal_completion(struct drm_writeback_connector *wb_connector,
+				int status);
+
+struct dma_fence *
+drm_writeback_get_out_fence(struct drm_writeback_connector *wb_connector);
 #endif
-- 
2.17.0

Re: [PATCH v8 2/3] drm: writeback: Add out-fences for writeback connectors

[Eric Anholt]

[-- Attachment #1: Type: text/plain, Size: 2135 bytes --]

Liviu Dudau <Liviu.Dudau@arm.com> writes:

> From: Brian Starkey <brian.starkey@arm.com>
>
> Add the WRITEBACK_OUT_FENCE_PTR property to writeback connectors, to
> enable userspace to get a fence which will signal once the writeback is
> complete. It is not allowed to request an out-fence without a
> framebuffer attached to the connector.
>
> A timeline is added to drm_writeback_connector for use by the writeback
> out-fences.


> diff --git a/include/drm/drm_writeback.h b/include/drm/drm_writeback.h
> index cf3a28676006a..6a7462c1821ad 100644
> --- a/include/drm/drm_writeback.h
> +++ b/include/drm/drm_writeback.h
> @@ -49,6 +49,32 @@ struct drm_writeback_connector {
>  	 * drm_writeback_signal_completion()
>  	 */
>  	struct list_head job_queue;
> +
> +	/**
> +	 * @fence_context:
> +	 *
> +	 * timeline context used for fence operations.
> +	 */
> +	unsigned int fence_context;
> +	/**
> +	 * @fence_lock:
> +	 *
> +	 * spinlock to protect the fences in the fence_context.
> +	 */
> +	spinlock_t fence_lock;
> +	/**
> +	 * @fence_seqno:
> +	 *
> +	 * Seqno variable used as monotonic counter for the fences
> +	 * created on the connector's timeline.
> +	 */
> +	unsigned long fence_seqno;
> +	/**
> +	 * @timeline_name:
> +	 *
> +	 * The name of the connector's fence timeline.
> +	 */
> +	char timeline_name[32];
>  };
>  
>  struct drm_writeback_job {
> @@ -59,12 +85,14 @@ struct drm_writeback_job {
>  	 * framebuffer reference to a workqueue.
>  	 */
>  	struct work_struct cleanup_work;
> +
>  	/**
>  	 * @list_entry:
>  	 *
>  	 * List item for the connector's @job_queue
>  	 */
>  	struct list_head list_entry;
> +
>  	/**
>  	 * @fb:
>  	 *

Move this hunk into patch 1?

Other than that, the series is:

Reviewed-by: Eric Anholt <eric@anholt.net>

It's pretty clean and makes sense to me.  I only had some questions
about the job_queue, which seems superfluous if we aren't supporting
firing off a new writeback while an old one is outstanding (and maybe we
should throw an error in that case).  Still, I think this is ready to land.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH v8 2/3] drm: writeback: Add out-fences for writeback connectors

[Liviu Dudau]

On Mon, May 21, 2018 at 12:02:25PM -0700, Eric Anholt wrote:
> Liviu Dudau <Liviu.Dudau@arm.com> writes:
> 
> > From: Brian Starkey <brian.starkey@arm.com>
> >
> > Add the WRITEBACK_OUT_FENCE_PTR property to writeback connectors, to
> > enable userspace to get a fence which will signal once the writeback is
> > complete. It is not allowed to request an out-fence without a
> > framebuffer attached to the connector.
> >
> > A timeline is added to drm_writeback_connector for use by the writeback
> > out-fences.
> 
> 
> > diff --git a/include/drm/drm_writeback.h b/include/drm/drm_writeback.h
> > index cf3a28676006a..6a7462c1821ad 100644
> > --- a/include/drm/drm_writeback.h
> > +++ b/include/drm/drm_writeback.h
> > @@ -49,6 +49,32 @@ struct drm_writeback_connector {
> >  	 * drm_writeback_signal_completion()
> >  	 */
> >  	struct list_head job_queue;
> > +
> > +	/**
> > +	 * @fence_context:
> > +	 *
> > +	 * timeline context used for fence operations.
> > +	 */
> > +	unsigned int fence_context;
> > +	/**
> > +	 * @fence_lock:
> > +	 *
> > +	 * spinlock to protect the fences in the fence_context.
> > +	 */
> > +	spinlock_t fence_lock;
> > +	/**
> > +	 * @fence_seqno:
> > +	 *
> > +	 * Seqno variable used as monotonic counter for the fences
> > +	 * created on the connector's timeline.
> > +	 */
> > +	unsigned long fence_seqno;
> > +	/**
> > +	 * @timeline_name:
> > +	 *
> > +	 * The name of the connector's fence timeline.
> > +	 */
> > +	char timeline_name[32];
> >  };
> >  
> >  struct drm_writeback_job {
> > @@ -59,12 +85,14 @@ struct drm_writeback_job {
> >  	 * framebuffer reference to a workqueue.
> >  	 */
> >  	struct work_struct cleanup_work;
> > +
> >  	/**
> >  	 * @list_entry:
> >  	 *
> >  	 * List item for the connector's @job_queue
> >  	 */
> >  	struct list_head list_entry;
> > +
> >  	/**
> >  	 * @fb:
> >  	 *
> 
> Move this hunk into patch 1?

I can, however this is the only change I will be making. Is it worth
respinning a new revision for it?

> 
> Other than that, the series is:
> 
> Reviewed-by: Eric Anholt <eric@anholt.net>

Many thanks for that!

> 
> It's pretty clean and makes sense to me.  I only had some questions
> about the job_queue, which seems superfluous if we aren't supporting
> firing off a new writeback while an old one is outstanding (and maybe we
> should throw an error in that case).  Still, I think this is ready to land.

I know Sean and Daniel are on holiday. Are you OK to pull this into
drm-misc? Should I ask Gustavo to do it?

Best regards,
Liviu

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯

[PATCH v8 3/3] drm: writeback: Add client capability for exposing writeback connectors

[Liviu Dudau]

Due to the fact that writeback connectors behave in a special way
in DRM (they always report being disconnected) we might confuse some
userspace. Add a client capability for writeback connectors that will
filter them out for clients that don't understand the capability.

Re-requested-by: Sean Paul <seanpaul@chromium.org>
Cc: Brian Starkey <brian.starkey@arm.com>
Signed-off-by: Liviu Dudau <liviu.dudau@arm.com>
---
 drivers/gpu/drm/drm_ioctl.c       | 7 +++++++
 drivers/gpu/drm/drm_mode_config.c | 5 +++++
 include/drm/drm_file.h            | 7 +++++++
 include/uapi/drm/drm.h            | 9 +++++++++
 4 files changed, 28 insertions(+)

diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
index af782911c505d..59951ff3e3630 100644
--- a/drivers/gpu/drm/drm_ioctl.c
+++ b/drivers/gpu/drm/drm_ioctl.c
@@ -325,6 +325,13 @@ drm_setclientcap(struct drm_device *dev, void *data, struct drm_file *file_priv)
 		file_priv->atomic = req->value;
 		file_priv->universal_planes = req->value;
 		break;
+	case DRM_CLIENT_CAP_WRITEBACK_CONNECTORS:
+		if (!file_priv->atomic || !drm_core_check_feature(dev, DRIVER_ATOMIC))
+			return -EINVAL;
+		if (req->value > 1)
+			return -EINVAL;
+		file_priv->writeback_connectors = req->value;
+		break;
 	default:
 		return -EINVAL;
 	}
diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c
index e5c653357024d..21e353bd3948e 100644
--- a/drivers/gpu/drm/drm_mode_config.c
+++ b/drivers/gpu/drm/drm_mode_config.c
@@ -145,6 +145,11 @@ int drm_mode_getresources(struct drm_device *dev, void *data,
 	count = 0;
 	connector_id = u64_to_user_ptr(card_res->connector_id_ptr);
 	drm_for_each_connector_iter(connector, &conn_iter) {
+		/* only expose writeback connectors if userspace understands them */
+		if (!file_priv->writeback_connectors &&
+		    (connector->connector_type == DRM_MODE_CONNECTOR_WRITEBACK))
+			continue;
+
 		if (drm_lease_held(file_priv, connector->base.id)) {
 			if (count < card_res->count_connectors &&
 			    put_user(connector->base.id, connector_id + count)) {
diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h
index 5176c3797680c..2a09b3c8965c6 100644
--- a/include/drm/drm_file.h
+++ b/include/drm/drm_file.h
@@ -181,6 +181,13 @@ struct drm_file {
 	/** @atomic: True if client understands atomic properties. */
 	unsigned atomic:1;
 
+	/**
+	 * @writeback_connectors:
+	 *
+	 * True if client understands writeback connectors
+	 */
+	unsigned writeback_connectors:1;
+
 	/**
 	 * @is_master:
 	 *
diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
index 6fdff5945c8a0..59f27ea928b42 100644
--- a/include/uapi/drm/drm.h
+++ b/include/uapi/drm/drm.h
@@ -680,6 +680,15 @@ struct drm_get_cap {
  */
 #define DRM_CLIENT_CAP_ATOMIC	3
 
+/**
+ * DRM_CLIENT_CAP_WRITEBACK_CONNECTORS
+ *
+ * If set to 1, the DRM core will expose special connectors to be used for
+ * writing back to memory the scene setup in the commit. Depends on client
+ * also supporting DRM_CLIENT_CAP_ATOMIC
+ */
+#define DRM_CLIENT_CAP_WRITEBACK_CONNECTORS	4
+
 /** DRM_IOCTL_SET_CLIENT_CAP ioctl argument type */
 struct drm_set_client_cap {
 	__u64 capability;
-- 
2.17.0

Re: [PATCH v8 3/3] drm: writeback: Add client capability for exposing writeback connectors

[Maarten Lankhorst]

Op 18-05-18 om 17:17 schreef Liviu Dudau:
> Due to the fact that writeback connectors behave in a special way
> in DRM (they always report being disconnected) we might confuse some
> userspace. Add a client capability for writeback connectors that will
> filter them out for clients that don't understand the capability.
>
> Re-requested-by: Sean Paul <seanpaul@chromium.org>
> Cc: Brian Starkey <brian.starkey@arm.com>
> Signed-off-by: Liviu Dudau <liviu.dudau@arm.com>
> ---
>  drivers/gpu/drm/drm_ioctl.c       | 7 +++++++
>  drivers/gpu/drm/drm_mode_config.c | 5 +++++
>  include/drm/drm_file.h            | 7 +++++++
>  include/uapi/drm/drm.h            | 9 +++++++++
>  4 files changed, 28 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
> index af782911c505d..59951ff3e3630 100644
> --- a/drivers/gpu/drm/drm_ioctl.c
> +++ b/drivers/gpu/drm/drm_ioctl.c
> @@ -325,6 +325,13 @@ drm_setclientcap(struct drm_device *dev, void *data, struct drm_file *file_priv)
>  		file_priv->atomic = req->value;
>  		file_priv->universal_planes = req->value;
>  		break;
> +	case DRM_CLIENT_CAP_WRITEBACK_CONNECTORS:
> +		if (!file_priv->atomic || !drm_core_check_feature(dev, DRIVER_ATOMIC))
> +			return -EINVAL;
Wondering how you can set the atomic cap without DRIVER_ATOMIC. :)

That part could be dropped I think. We should probably WARN when trying to create a writeback connector without the DRIVER_ATOMIC cap set.
> +		if (req->value > 1)
> +			return -EINVAL;
> +		file_priv->writeback_connectors = req->value;
> +		break;
>  	default:
>  		return -EINVAL;
>  	}
> diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c
> index e5c653357024d..21e353bd3948e 100644
> --- a/drivers/gpu/drm/drm_mode_config.c
> +++ b/drivers/gpu/drm/drm_mode_config.c
> @@ -145,6 +145,11 @@ int drm_mode_getresources(struct drm_device *dev, void *data,
>  	count = 0;
>  	connector_id = u64_to_user_ptr(card_res->connector_id_ptr);
>  	drm_for_each_connector_iter(connector, &conn_iter) {
> +		/* only expose writeback connectors if userspace understands them */
> +		if (!file_priv->writeback_connectors &&
> +		    (connector->connector_type == DRM_MODE_CONNECTOR_WRITEBACK))
> +			continue;
> +
>  		if (drm_lease_held(file_priv, connector->base.id)) {
>  			if (count < card_res->count_connectors &&
>  			    put_user(connector->base.id, connector_id + count)) {
> diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h
> index 5176c3797680c..2a09b3c8965c6 100644
> --- a/include/drm/drm_file.h
> +++ b/include/drm/drm_file.h
> @@ -181,6 +181,13 @@ struct drm_file {
>  	/** @atomic: True if client understands atomic properties. */
>  	unsigned atomic:1;
>  
> +	/**
> +	 * @writeback_connectors:
> +	 *
> +	 * True if client understands writeback connectors
> +	 */
> +	unsigned writeback_connectors:1;
> +
>  	/**
>  	 * @is_master:
>  	 *
> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
> index 6fdff5945c8a0..59f27ea928b42 100644
> --- a/include/uapi/drm/drm.h
> +++ b/include/uapi/drm/drm.h
> @@ -680,6 +680,15 @@ struct drm_get_cap {
>   */
>  #define DRM_CLIENT_CAP_ATOMIC	3
>  
> +/**
> + * DRM_CLIENT_CAP_WRITEBACK_CONNECTORS
> + *
> + * If set to 1, the DRM core will expose special connectors to be used for
> + * writing back to memory the scene setup in the commit. Depends on client
> + * also supporting DRM_CLIENT_CAP_ATOMIC
> + */
> +#define DRM_CLIENT_CAP_WRITEBACK_CONNECTORS	4
> +
>  /** DRM_IOCTL_SET_CLIENT_CAP ioctl argument type */
>  struct drm_set_client_cap {
>  	__u64 capability;

~Maarten

Re: [PATCH v8 3/3] drm: writeback: Add client capability for exposing writeback connectors

[Liviu Dudau]

On Wed, May 23, 2018 at 11:34:32AM +0200, Maarten Lankhorst wrote:
> Op 18-05-18 om 17:17 schreef Liviu Dudau:
> > Due to the fact that writeback connectors behave in a special way
> > in DRM (they always report being disconnected) we might confuse some
> > userspace. Add a client capability for writeback connectors that will
> > filter them out for clients that don't understand the capability.
> >
> > Re-requested-by: Sean Paul <seanpaul@chromium.org>
> > Cc: Brian Starkey <brian.starkey@arm.com>
> > Signed-off-by: Liviu Dudau <liviu.dudau@arm.com>
> > ---
> >  drivers/gpu/drm/drm_ioctl.c       | 7 +++++++
> >  drivers/gpu/drm/drm_mode_config.c | 5 +++++
> >  include/drm/drm_file.h            | 7 +++++++
> >  include/uapi/drm/drm.h            | 9 +++++++++
> >  4 files changed, 28 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
> > index af782911c505d..59951ff3e3630 100644
> > --- a/drivers/gpu/drm/drm_ioctl.c
> > +++ b/drivers/gpu/drm/drm_ioctl.c
> > @@ -325,6 +325,13 @@ drm_setclientcap(struct drm_device *dev, void *data, struct drm_file *file_priv)
> >  		file_priv->atomic = req->value;
> >  		file_priv->universal_planes = req->value;
> >  		break;
> > +	case DRM_CLIENT_CAP_WRITEBACK_CONNECTORS:
> > +		if (!file_priv->atomic || !drm_core_check_feature(dev, DRIVER_ATOMIC))
> > +			return -EINVAL;
> Wondering how you can set the atomic cap without DRIVER_ATOMIC. :)

Caps can only be set one at a time. I was trying to cater for the cases
where userspace can set the WRITEBACK_CONNECTORS client cap before or
after setting the atomic cap.

> 
> That part could be dropped I think. We should probably WARN when trying to create a writeback connector without the DRIVER_ATOMIC cap set.

I could still keep the check for file_priv->atomic being set when
accepting the DRM_CLIENT_CAP_WRITEBACK_CONNECTORS and would not need a
warn.

Best regards,
Liviu

> > +		if (req->value > 1)
> > +			return -EINVAL;
> > +		file_priv->writeback_connectors = req->value;
> > +		break;
> >  	default:
> >  		return -EINVAL;
> >  	}
> > diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c
> > index e5c653357024d..21e353bd3948e 100644
> > --- a/drivers/gpu/drm/drm_mode_config.c
> > +++ b/drivers/gpu/drm/drm_mode_config.c
> > @@ -145,6 +145,11 @@ int drm_mode_getresources(struct drm_device *dev, void *data,
> >  	count = 0;
> >  	connector_id = u64_to_user_ptr(card_res->connector_id_ptr);
> >  	drm_for_each_connector_iter(connector, &conn_iter) {
> > +		/* only expose writeback connectors if userspace understands them */
> > +		if (!file_priv->writeback_connectors &&
> > +		    (connector->connector_type == DRM_MODE_CONNECTOR_WRITEBACK))
> > +			continue;
> > +
> >  		if (drm_lease_held(file_priv, connector->base.id)) {
> >  			if (count < card_res->count_connectors &&
> >  			    put_user(connector->base.id, connector_id + count)) {
> > diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h
> > index 5176c3797680c..2a09b3c8965c6 100644
> > --- a/include/drm/drm_file.h
> > +++ b/include/drm/drm_file.h
> > @@ -181,6 +181,13 @@ struct drm_file {
> >  	/** @atomic: True if client understands atomic properties. */
> >  	unsigned atomic:1;
> >  
> > +	/**
> > +	 * @writeback_connectors:
> > +	 *
> > +	 * True if client understands writeback connectors
> > +	 */
> > +	unsigned writeback_connectors:1;
> > +
> >  	/**
> >  	 * @is_master:
> >  	 *
> > diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
> > index 6fdff5945c8a0..59f27ea928b42 100644
> > --- a/include/uapi/drm/drm.h
> > +++ b/include/uapi/drm/drm.h
> > @@ -680,6 +680,15 @@ struct drm_get_cap {
> >   */
> >  #define DRM_CLIENT_CAP_ATOMIC	3
> >  
> > +/**
> > + * DRM_CLIENT_CAP_WRITEBACK_CONNECTORS
> > + *
> > + * If set to 1, the DRM core will expose special connectors to be used for
> > + * writing back to memory the scene setup in the commit. Depends on client
> > + * also supporting DRM_CLIENT_CAP_ATOMIC
> > + */
> > +#define DRM_CLIENT_CAP_WRITEBACK_CONNECTORS	4
> > +
> >  /** DRM_IOCTL_SET_CLIENT_CAP ioctl argument type */
> >  struct drm_set_client_cap {
> >  	__u64 capability;
> 
> ~Maarten
> 

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯

Re: [PATCH v8 3/3] drm: writeback: Add client capability for exposing writeback connectors

[Daniel Vetter]

On Wed, May 23, 2018 at 01:27:17PM +0100, Liviu Dudau wrote:
> On Wed, May 23, 2018 at 11:34:32AM +0200, Maarten Lankhorst wrote:
> > Op 18-05-18 om 17:17 schreef Liviu Dudau:
> > > Due to the fact that writeback connectors behave in a special way
> > > in DRM (they always report being disconnected) we might confuse some
> > > userspace. Add a client capability for writeback connectors that will
> > > filter them out for clients that don't understand the capability.
> > >
> > > Re-requested-by: Sean Paul <seanpaul@chromium.org>
> > > Cc: Brian Starkey <brian.starkey@arm.com>
> > > Signed-off-by: Liviu Dudau <liviu.dudau@arm.com>
> > > ---
> > >  drivers/gpu/drm/drm_ioctl.c       | 7 +++++++
> > >  drivers/gpu/drm/drm_mode_config.c | 5 +++++
> > >  include/drm/drm_file.h            | 7 +++++++
> > >  include/uapi/drm/drm.h            | 9 +++++++++
> > >  4 files changed, 28 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
> > > index af782911c505d..59951ff3e3630 100644
> > > --- a/drivers/gpu/drm/drm_ioctl.c
> > > +++ b/drivers/gpu/drm/drm_ioctl.c
> > > @@ -325,6 +325,13 @@ drm_setclientcap(struct drm_device *dev, void *data, struct drm_file *file_priv)
> > >  		file_priv->atomic = req->value;
> > >  		file_priv->universal_planes = req->value;
> > >  		break;
> > > +	case DRM_CLIENT_CAP_WRITEBACK_CONNECTORS:
> > > +		if (!file_priv->atomic || !drm_core_check_feature(dev, DRIVER_ATOMIC))
> > > +			return -EINVAL;
> > Wondering how you can set the atomic cap without DRIVER_ATOMIC. :)
> 
> Caps can only be set one at a time. I was trying to cater for the cases
> where userspace can set the WRITEBACK_CONNECTORS client cap before or
> after setting the atomic cap.

The way we usually solve this is that if there's a dependency in caps, we
set them all. E.g. atomic above also sets universal planes. I recommend we
do the same for writeback.

> > That part could be dropped I think. We should probably WARN when trying to create a writeback connector without the DRIVER_ATOMIC cap set.
> 
> I could still keep the check for file_priv->atomic being set when
> accepting the DRM_CLIENT_CAP_WRITEBACK_CONNECTORS and would not need a
> warn.

Or this.
-Daniel

> 
> Best regards,
> Liviu
> 
> > > +		if (req->value > 1)
> > > +			return -EINVAL;
> > > +		file_priv->writeback_connectors = req->value;
> > > +		break;
> > >  	default:
> > >  		return -EINVAL;
> > >  	}
> > > diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c
> > > index e5c653357024d..21e353bd3948e 100644
> > > --- a/drivers/gpu/drm/drm_mode_config.c
> > > +++ b/drivers/gpu/drm/drm_mode_config.c
> > > @@ -145,6 +145,11 @@ int drm_mode_getresources(struct drm_device *dev, void *data,
> > >  	count = 0;
> > >  	connector_id = u64_to_user_ptr(card_res->connector_id_ptr);
> > >  	drm_for_each_connector_iter(connector, &conn_iter) {
> > > +		/* only expose writeback connectors if userspace understands them */
> > > +		if (!file_priv->writeback_connectors &&
> > > +		    (connector->connector_type == DRM_MODE_CONNECTOR_WRITEBACK))
> > > +			continue;
> > > +
> > >  		if (drm_lease_held(file_priv, connector->base.id)) {
> > >  			if (count < card_res->count_connectors &&
> > >  			    put_user(connector->base.id, connector_id + count)) {
> > > diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h
> > > index 5176c3797680c..2a09b3c8965c6 100644
> > > --- a/include/drm/drm_file.h
> > > +++ b/include/drm/drm_file.h
> > > @@ -181,6 +181,13 @@ struct drm_file {
> > >  	/** @atomic: True if client understands atomic properties. */
> > >  	unsigned atomic:1;
> > >  
> > > +	/**
> > > +	 * @writeback_connectors:
> > > +	 *
> > > +	 * True if client understands writeback connectors
> > > +	 */
> > > +	unsigned writeback_connectors:1;
> > > +
> > >  	/**
> > >  	 * @is_master:
> > >  	 *
> > > diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
> > > index 6fdff5945c8a0..59f27ea928b42 100644
> > > --- a/include/uapi/drm/drm.h
> > > +++ b/include/uapi/drm/drm.h
> > > @@ -680,6 +680,15 @@ struct drm_get_cap {
> > >   */
> > >  #define DRM_CLIENT_CAP_ATOMIC	3
> > >  
> > > +/**
> > > + * DRM_CLIENT_CAP_WRITEBACK_CONNECTORS
> > > + *
> > > + * If set to 1, the DRM core will expose special connectors to be used for
> > > + * writing back to memory the scene setup in the commit. Depends on client
> > > + * also supporting DRM_CLIENT_CAP_ATOMIC
> > > + */
> > > +#define DRM_CLIENT_CAP_WRITEBACK_CONNECTORS	4
> > > +
> > >  /** DRM_IOCTL_SET_CLIENT_CAP ioctl argument type */
> > >  struct drm_set_client_cap {
> > >  	__u64 capability;
> > 
> > ~Maarten
> > 
> 
> -- 
> ====================
> | I would like to |
> | fix the world,  |
> | but they're not |
> | giving me the   |
>  \ source code!  /
>   ---------------
>     ¯\_(ツ)_/¯
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch