[RFC PATCH v3 0/6] Introduce writeback connectors

[RFC PATCH v3 0/6] Introduce writeback connectors

[Brian Starkey]

Hi,

This is v3 of my series introducing a new connector type:
 DRM_MODE_CONNECTOR_WRITEBACK
See v1 and v2 here: [1] [2]

Writeback connectors are used to expose the memory writeback engines
found in some display controllers, which can write a CRTC's
composition result to a memory buffer.
This is useful e.g. for testing, screen-recording, screenshots,
wireless display, display cloning, memory-to-memory composition.

Writeback connectors are given a WRITEBACK_FB_ID property (which acts
slightly differently to FB_ID, so gets a new name), as well as
a PIXEL_FORMATS blob to list the supported writeback formats, and
OUT_FENCE_PTR to be used for out-fences.

The changes since v2 are in the commit messages of each commit.

The main differences are:
 - Subclass drm_connector as drm_writeback_connector
 - Slight relaxation of core checks, to allow
   (connector->crtc && !connector->fb)
 - Dropped PIXEL_FORMATS_SIZE, which was redundant
 - Reworked the event interface, drivers don't need to deal with the
   fence directly
 - Re-ordered the commits to introduce writeback out-fences up-front.

I've kept RFC on this series because the event reporting (introduction
of drm_writeback_job) is probably up for debate.

v4 will be accompanied by igt tests.

As always, I look forward to any comments.

Thanks,
Brian

[1] http://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1247574.html
[2] http://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1258017.html

---

Brian Starkey (5):
  drm: Add writeback connector type
  drm: writeback: Add out-fences for writeback connectors
  drm: mali-dp: Rename malidp_input_format
  drm: mali-dp: Add RGB writeback formats for DP550/DP650
  drm: mali-dp: Add writeback connector

Liviu Dudau (1):
  drm: mali-dp: Add support for writeback on DP550/DP650

 Documentation/gpu/drm-kms.rst       |    9 +
 drivers/gpu/drm/Makefile            |    2 +-
 drivers/gpu/drm/arm/Makefile        |    1 +
 drivers/gpu/drm/arm/malidp_crtc.c   |   21 +++
 drivers/gpu/drm/arm/malidp_drv.c    |   25 ++-
 drivers/gpu/drm/arm/malidp_drv.h    |    3 +
 drivers/gpu/drm/arm/malidp_hw.c     |  111 ++++++++----
 drivers/gpu/drm/arm/malidp_hw.h     |   27 ++-
 drivers/gpu/drm/arm/malidp_mw.c     |  278 ++++++++++++++++++++++++++++++
 drivers/gpu/drm/arm/malidp_mw.h     |   18 ++
 drivers/gpu/drm/arm/malidp_planes.c |    8 +-
 drivers/gpu/drm/arm/malidp_regs.h   |   15 ++
 drivers/gpu/drm/drm_atomic.c        |  226 +++++++++++++++++++++++-
 drivers/gpu/drm/drm_atomic_helper.c |    6 +
 drivers/gpu/drm/drm_connector.c     |    4 +-
 drivers/gpu/drm/drm_writeback.c     |  325 +++++++++++++++++++++++++++++++++++
 include/drm/drm_atomic.h            |   11 ++
 include/drm/drm_connector.h         |   13 ++
 include/drm/drm_mode_config.h       |   14 ++
 include/drm/drm_writeback.h         |  115 +++++++++++++
 include/uapi/drm/drm_mode.h         |    1 +
 21 files changed, 1181 insertions(+), 52 deletions(-)
 create mode 100644 drivers/gpu/drm/arm/malidp_mw.c
 create mode 100644 drivers/gpu/drm/arm/malidp_mw.h
 create mode 100644 drivers/gpu/drm/drm_writeback.c
 create mode 100644 include/drm/drm_writeback.h

-- 
1.7.9.5

[PATCH 1/6] drm: Add writeback connector type

[Brian Starkey]

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 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

Signed-off-by: Brian Starkey <brian.starkey@arm.com>
---
 Documentation/gpu/drm-kms.rst       |    9 ++
 drivers/gpu/drm/Makefile            |    2 +-
 drivers/gpu/drm/drm_atomic.c        |  130 ++++++++++++++++++++
 drivers/gpu/drm/drm_atomic_helper.c |    6 +
 drivers/gpu/drm/drm_connector.c     |    4 +-
 drivers/gpu/drm/drm_writeback.c     |  230 +++++++++++++++++++++++++++++++++++
 include/drm/drm_atomic.h            |    3 +
 include/drm/drm_connector.h         |   13 ++
 include/drm/drm_mode_config.h       |   14 +++
 include/drm/drm_writeback.h         |   78 ++++++++++++
 include/uapi/drm/drm_mode.h         |    1 +
 11 files changed, 488 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 568f3c2..3a4f35b 100644
--- a/Documentation/gpu/drm-kms.rst
+++ b/Documentation/gpu/drm-kms.rst
@@ -122,6 +122,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 883f3e7..3209aa4 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -16,7 +16,7 @@ drm-y       :=	drm_auth.o drm_bufs.o drm_cache.o \
 		drm_framebuffer.o drm_connector.o drm_blend.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_dumb_buffers.o drm_mode_config.o drm_writeback.o
 
 drm-$(CONFIG_COMPAT) += drm_ioc32.o
 drm-$(CONFIG_DRM_GEM_CMA_HELPER) += drm_gem_cma_helper.o
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index b476ec5..343e2b7 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -31,6 +31,7 @@
 #include <drm/drm_mode.h>
 #include <drm/drm_plane_helper.h>
 #include <drm/drm_print.h>
+#include <drm/drm_writeback.h>
 #include <linux/sync_file.h>
 
 #include "drm_crtc_internal.h"
@@ -659,6 +660,46 @@ static void drm_atomic_crtc_print_state(struct drm_printer *p,
 }
 
 /**
+ * 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
  * @plane: plane to get state object for
@@ -1087,6 +1128,12 @@ int drm_atomic_connector_set_property(struct drm_connector *connector,
 		 * now?) atomic writes to DPMS property:
 		 */
 		return -EINVAL;
+	} else if (property == config->writeback_fb_id_property) {
+		struct drm_framebuffer *fb = drm_framebuffer_lookup(dev, 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);
@@ -1135,6 +1182,9 @@ static void drm_atomic_connector_print_state(struct drm_printer *p,
 		*val = (state->crtc) ? state->crtc->base.id : 0;
 	} else if (property == config->dpms_property) {
 		*val = connector->dpms;
+	} 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);
@@ -1347,6 +1397,75 @@ int drm_atomic_get_property(struct drm_mode_object *obj,
 }
 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
@@ -1501,6 +1620,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);
@@ -1523,6 +1644,15 @@ int drm_atomic_check_only(struct drm_atomic_state *state)
 		}
 	}
 
+	for_each_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 0b16587..75812b9 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_internal.h"
@@ -3192,6 +3193,9 @@ void drm_atomic_helper_connector_reset(struct drm_connector *connector)
 	memcpy(state, connector->state, sizeof(*state));
 	if (state->crtc)
 		drm_connector_reference(connector);
+
+	/* Don't copy over a writeback job, they are used only once */
+	state->writeback_job = NULL;
 }
 EXPORT_SYMBOL(__drm_atomic_helper_connector_duplicate_state);
 
@@ -3319,6 +3323,8 @@ struct drm_atomic_state *
 	 */
 	if (state->crtc)
 		drm_connector_unreference(state->connector);
+	if (state->writeback_job)
+		drm_writeback_cleanup_job(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 b5c6a8e..6bbd93f 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -86,6 +86,7 @@ struct drm_conn_prop_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)
@@ -235,7 +236,8 @@ int drm_connector_init(struct drm_device *dev,
 	list_add_tail(&connector->head, &config->connector_list);
 	config->num_connector++;
 
-	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 0000000..75a1dbf
--- /dev/null
+++ b/drivers/gpu/drm/drm_writeback.c
@@ -0,0 +1,230 @@
+/*
+ * (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_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.
+ *
+ *  "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 bool 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 false;
+		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_IMMUTABLE,
+					   "PIXEL_FORMATS", 0);
+		if (!prop)
+			return false;
+		dev->mode_config.writeback_pixel_formats_property = prop;
+	}
+
+	return true;
+}
+
+/**
+ * drm_writeback_connector_init - Initialize a writeback connector and its properties
+ * @dev: DRM device
+ * @wb_connector: Writeback connector to initialize
+ * @funcs: Connector funcs vtable
+ * @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.
+ *
+ * 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 *funcs,
+				 u32 *formats, int n_formats)
+{
+	int ret;
+	struct drm_property_blob *blob;
+	struct drm_connector *connector = &wb_connector->base;
+	struct drm_mode_config *config = &dev->mode_config;
+
+	if (!create_writeback_properties(dev))
+		return -EINVAL;
+
+	blob = drm_property_create_blob(dev, n_formats * sizeof(*formats),
+					formats);
+	if (IS_ERR(blob))
+		return PTR_ERR(blob);
+
+	ret = drm_connector_init(dev, connector, funcs,
+				 DRM_MODE_CONNECTOR_WRITEBACK);
+	if (ret)
+		goto 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;
+
+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)
+		return;
+
+	if (job->fb)
+		drm_framebuffer_unreference(job->fb);
+	kfree(job);
+}
+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 c0eaec7..476561d 100644
--- a/include/drm/drm_atomic.h
+++ b/include/drm/drm_atomic.h
@@ -351,6 +351,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 34f9741..dc4910d6 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -214,6 +214,19 @@ struct drm_connector_state {
 	struct drm_encoder *best_encoder;
 
 	struct drm_atomic_state *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.
+	 *
+	 * 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 bf9991b2..3d3d07f 100644
--- a/include/drm/drm_mode_config.h
+++ b/include/drm/drm_mode_config.h
@@ -634,6 +634,20 @@ struct drm_mode_config {
 	 */
 	struct drm_property *suggested_y_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_writeback.h b/include/drm/drm_writeback.h
new file mode 100644
index 0000000..6b2ac45
--- /dev/null
+++ b/include/drm/drm_writeback.h
@@ -0,0 +1,78 @@
+/*
+ * (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 <linux/workqueue.h>
+
+struct drm_writeback_connector {
+	struct drm_connector base;
+
+	/**
+	 * @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 *funcs,
+				 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 ebf622f..ae5e4f7 100644
--- a/include/uapi/drm/drm_mode.h
+++ b/include/uapi/drm/drm_mode.h
@@ -263,6 +263,7 @@ struct drm_mode_get_encoder {
 #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 {
 
-- 
1.7.9.5

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

[Brian Starkey]

Add the 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

Signed-off-by: Brian Starkey <brian.starkey@arm.com>
---
 drivers/gpu/drm/drm_atomic.c    |   96 +++++++++++++++++++++++++++++++++----
 drivers/gpu/drm/drm_writeback.c |   99 ++++++++++++++++++++++++++++++++++++++-
 include/drm/drm_atomic.h        |    8 ++++
 include/drm/drm_connector.h     |    8 ++--
 include/drm/drm_writeback.h     |   39 ++++++++++++++-
 5 files changed, 234 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 343e2b7..0e3c900 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -308,6 +308,32 @@ static s64 __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,
+					s64 __user *fence_ptr)
+{
+	unsigned int index = drm_connector_index(connector);
+
+	if (fence_ptr && put_user(-1, fence_ptr))
+		return -EFAULT;
+
+	state->connectors[index].out_fence_ptr = fence_ptr;
+
+	return 0;
+}
+
+static s64 __user *get_out_fence_for_connector(struct drm_atomic_state *state,
+					       struct drm_connector *connector)
+{
+	unsigned int index = drm_connector_index(connector);
+	s64 __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
@@ -696,6 +722,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;
 }
 
@@ -1134,6 +1166,11 @@ int drm_atomic_connector_set_property(struct drm_connector *connector,
 		if (fb)
 			drm_framebuffer_unreference(fb);
 		return ret;
+	} else if (property == config->prop_out_fence_ptr) {
+		s64 __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);
@@ -1185,6 +1222,8 @@ static void drm_atomic_connector_print_state(struct drm_printer *p,
 	} else if (property == config->writeback_fb_id_property) {
 		/* Writeback framebuffer is one-shot, write and forget */
 		*val = 0;
+	} else if (property == config->prop_out_fence_ptr) {
+		*val = 0;
 	} else if (connector->funcs->atomic_get_property) {
 		return connector->funcs->atomic_get_property(connector,
 				state, property, val);
@@ -1976,7 +2015,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,
@@ -1985,6 +2024,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, ret;
 
 	if (arg->flags & DRM_MODE_ATOMIC_TEST_ONLY)
@@ -2048,14 +2089,51 @@ static int prepare_crtc_signaling(struct drm_device *dev,
 		}
 	}
 
+	for_each_connector_in_state(state, conn, conn_state, i) {
+		struct drm_writeback_job *job;
+		struct drm_out_fence_state *f;
+		struct dma_fence *fence;
+		s64 __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;
+	}
+
 	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;
@@ -2228,8 +2306,8 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
 		drm_mode_object_unreference(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;
 
@@ -2251,7 +2329,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 75a1dbf..3637b9a 100644
--- a/drivers/gpu/drm/drm_writeback.c
+++ b/drivers/gpu/drm/drm_writeback.c
@@ -12,6 +12,7 @@
 #include <drm/drm_property.h>
 #include <drm/drm_writeback.h>
 #include <drm/drmP.h>
+#include <linux/dma-fence.h>
 
 /**
  * DOC: overview
@@ -29,6 +30,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
+ * 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:
  *
@@ -45,8 +56,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.
+ *
+ *  "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 64-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 bool create_writeback_properties(struct drm_device *dev)
 {
 	struct drm_property *prop;
@@ -115,6 +172,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->prop_out_fence_ptr, 0);
+
 	drm_object_attach_property(&connector->base,
 				   config->writeback_fb_id_property, 0);
 
@@ -173,6 +239,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);
 	kfree(job);
 }
 EXPORT_SYMBOL(drm_writeback_cleanup_job);
@@ -195,6 +262,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
@@ -208,7 +276,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;
@@ -217,8 +286,13 @@ static void cleanup_work(struct work_struct *work)
 	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) {
+			job->out_fence->status = status;
+			dma_fence_signal(job->out_fence);
+		}
+	}
 	spin_unlock_irqrestore(&wb_connector->job_lock, flags);
 
 	if (WARN_ON(!job))
@@ -228,3 +302,24 @@ static void cleanup_work(struct work_struct *work)
 	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 476561d..7c4ad0b 100644
--- a/include/drm/drm_atomic.h
+++ b/include/drm/drm_atomic.h
@@ -150,6 +150,14 @@ struct __drm_crtcs_state {
 struct __drm_connnectors_state {
 	struct drm_connector *ptr;
 	struct drm_connector_state *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.
+	 */
+	s64 __user *out_fence_ptr;
 };
 
 /**
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index dc4910d6..a049cec 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -218,10 +218,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_writeback.h b/include/drm/drm_writeback.h
index 6b2ac45..989cedd 100644
--- a/include/drm/drm_writeback.h
+++ b/include/drm/drm_writeback.h
@@ -38,6 +38,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 {
@@ -61,6 +87,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,
@@ -74,5 +107,9 @@ void drm_writeback_queue_job(struct drm_writeback_connector *wb_connector,
 void drm_writeback_cleanup_job(struct drm_writeback_job *job);
 
 void
-drm_writeback_signal_completion(struct drm_writeback_connector *wb_connector);
+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
-- 
1.7.9.5

[PATCH 3/6] drm: mali-dp: Rename malidp_input_format

[Brian Starkey]

We're going to use the same format list for output formats, so rename
everything related to input formats to avoid confusion.

Signed-off-by: Brian Starkey <brian.starkey@arm.com>
Reviewed-by: Liviu Dudau <Liviu.Dudau@arm.com>
---
 drivers/gpu/drm/arm/malidp_hw.c     |   24 ++++++++++++------------
 drivers/gpu/drm/arm/malidp_hw.h     |    8 ++++----
 drivers/gpu/drm/arm/malidp_planes.c |    8 ++++----
 3 files changed, 20 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/arm/malidp_hw.c b/drivers/gpu/drm/arm/malidp_hw.c
index 4bdf531..9ec6d69 100644
--- a/drivers/gpu/drm/arm/malidp_hw.c
+++ b/drivers/gpu/drm/arm/malidp_hw.c
@@ -21,7 +21,7 @@
 #include "malidp_drv.h"
 #include "malidp_hw.h"
 
-static const struct malidp_input_format malidp500_de_formats[] = {
+static const struct malidp_format_id malidp500_de_formats[] = {
 	/*    fourcc,   layers supporting the format,     internal id  */
 	{ DRM_FORMAT_ARGB2101010, DE_VIDEO1 | DE_GRAPHICS1 | DE_GRAPHICS2,  0 },
 	{ DRM_FORMAT_ABGR2101010, DE_VIDEO1 | DE_GRAPHICS1 | DE_GRAPHICS2,  1 },
@@ -69,7 +69,7 @@
 	{ DRM_FORMAT_NV12, DE_VIDEO1 | DE_VIDEO2, MALIDP_ID(5, 6) },	\
 	{ DRM_FORMAT_YUV420, DE_VIDEO1 | DE_VIDEO2, MALIDP_ID(5, 7) }
 
-static const struct malidp_input_format malidp550_de_formats[] = {
+static const struct malidp_format_id malidp550_de_formats[] = {
 	MALIDP_COMMON_FORMATS,
 };
 
@@ -436,8 +436,8 @@ static int malidp650_query_hw(struct malidp_hw_device *hwdev)
 				.irq_mask = MALIDP500_DE_IRQ_CONF_VALID,
 				.vsync_irq = MALIDP500_DE_IRQ_CONF_VALID,
 			},
-			.input_formats = malidp500_de_formats,
-			.n_input_formats = ARRAY_SIZE(malidp500_de_formats),
+			.pixel_formats = malidp500_de_formats,
+			.n_pixel_formats = ARRAY_SIZE(malidp500_de_formats),
 			.bus_align_bytes = 8,
 		},
 		.query_hw = malidp500_query_hw,
@@ -469,8 +469,8 @@ static int malidp650_query_hw(struct malidp_hw_device *hwdev)
 				.irq_mask = MALIDP550_DC_IRQ_CONF_VALID,
 				.vsync_irq = MALIDP550_DC_IRQ_CONF_VALID,
 			},
-			.input_formats = malidp550_de_formats,
-			.n_input_formats = ARRAY_SIZE(malidp550_de_formats),
+			.pixel_formats = malidp550_de_formats,
+			.n_pixel_formats = ARRAY_SIZE(malidp550_de_formats),
 			.bus_align_bytes = 8,
 		},
 		.query_hw = malidp550_query_hw,
@@ -503,8 +503,8 @@ static int malidp650_query_hw(struct malidp_hw_device *hwdev)
 				.irq_mask = MALIDP550_DC_IRQ_CONF_VALID,
 				.vsync_irq = MALIDP550_DC_IRQ_CONF_VALID,
 			},
-			.input_formats = malidp550_de_formats,
-			.n_input_formats = ARRAY_SIZE(malidp550_de_formats),
+			.pixel_formats = malidp550_de_formats,
+			.n_pixel_formats = ARRAY_SIZE(malidp550_de_formats),
 			.bus_align_bytes = 16,
 		},
 		.query_hw = malidp650_query_hw,
@@ -522,10 +522,10 @@ u8 malidp_hw_get_format_id(const struct malidp_hw_regmap *map,
 {
 	unsigned int i;
 
-	for (i = 0; i < map->n_input_formats; i++) {
-		if (((map->input_formats[i].layer & layer_id) == layer_id) &&
-		    (map->input_formats[i].format == format))
-			return map->input_formats[i].id;
+	for (i = 0; i < map->n_pixel_formats; i++) {
+		if (((map->pixel_formats[i].layer & layer_id) == layer_id) &&
+		    (map->pixel_formats[i].format == format))
+			return map->pixel_formats[i].id;
 	}
 
 	return MALIDP_INVALID_FORMAT_ID;
diff --git a/drivers/gpu/drm/arm/malidp_hw.h b/drivers/gpu/drm/arm/malidp_hw.h
index 087e1202..4f8c884 100644
--- a/drivers/gpu/drm/arm/malidp_hw.h
+++ b/drivers/gpu/drm/arm/malidp_hw.h
@@ -35,7 +35,7 @@ enum {
 	DE_SMART = BIT(4),
 };
 
-struct malidp_input_format {
+struct malidp_format_id {
 	u32 format;		/* DRM fourcc */
 	u8 layer;		/* bitmask of layers supporting it */
 	u8 id;			/* used internally */
@@ -85,9 +85,9 @@ struct malidp_hw_regmap {
 	const struct malidp_irq_map se_irq_map;
 	const struct malidp_irq_map dc_irq_map;
 
-	/* list of supported input formats for each layer */
-	const struct malidp_input_format *input_formats;
-	const u8 n_input_formats;
+	/* list of supported pixel formats for each layer */
+	const struct malidp_format_id *pixel_formats;
+	const u8 n_pixel_formats;
 
 	/* pitch alignment requirement in bytes */
 	const u8 bus_align_bytes;
diff --git a/drivers/gpu/drm/arm/malidp_planes.c b/drivers/gpu/drm/arm/malidp_planes.c
index 63eec8f..e3cfd3a 100644
--- a/drivers/gpu/drm/arm/malidp_planes.c
+++ b/drivers/gpu/drm/arm/malidp_planes.c
@@ -258,7 +258,7 @@ int malidp_de_planes_init(struct drm_device *drm)
 	u32 *formats;
 	int ret, i, j, n;
 
-	formats = kcalloc(map->n_input_formats, sizeof(*formats), GFP_KERNEL);
+	formats = kcalloc(map->n_pixel_formats, sizeof(*formats), GFP_KERNEL);
 	if (!formats) {
 		ret = -ENOMEM;
 		goto cleanup;
@@ -274,9 +274,9 @@ int malidp_de_planes_init(struct drm_device *drm)
 		}
 
 		/* build the list of DRM supported formats based on the map */
-		for (n = 0, j = 0;  j < map->n_input_formats; j++) {
-			if ((map->input_formats[j].layer & id) == id)
-				formats[n++] = map->input_formats[j].format;
+		for (n = 0, j = 0;  j < map->n_pixel_formats; j++) {
+			if ((map->pixel_formats[j].layer & id) == id)
+				formats[n++] = map->pixel_formats[j].format;
 		}
 
 		plane_type = (i == 0) ? DRM_PLANE_TYPE_PRIMARY :
-- 
1.7.9.5

[PATCH 4/6] drm: mali-dp: Add support for writeback on DP550/DP650

[Brian Starkey]

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

Mali-DP display processors are able to write the composition result to a
memory buffer via the SE.

Add entry points in the HAL for enabling/disabling this feature, and
implement support for it on DP650 and DP550. DP500 acts differently and
so is omitted from this change.

Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>
Signed-off-by: Brian Starkey <brian.starkey@arm.com>
---
 drivers/gpu/drm/arm/malidp_hw.c   |   52 +++++++++++++++++++++++++++++++++++--
 drivers/gpu/drm/arm/malidp_hw.h   |   18 +++++++++++++
 drivers/gpu/drm/arm/malidp_regs.h |   15 +++++++++++
 3 files changed, 83 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/arm/malidp_hw.c b/drivers/gpu/drm/arm/malidp_hw.c
index 9ec6d69..c696e67 100644
--- a/drivers/gpu/drm/arm/malidp_hw.c
+++ b/drivers/gpu/drm/arm/malidp_hw.c
@@ -384,6 +384,48 @@ static int malidp550_rotmem_required(struct malidp_hw_device *hwdev, u16 w, u16
 	return w * bytes_per_col;
 }
 
+static int malidp550_enable_memwrite(struct malidp_hw_device *hwdev,
+				     dma_addr_t *addrs, s32 *pitches,
+				     int num_planes, u16 w, u16 h, u32 fmt_id)
+{
+	u32 base = MALIDP550_SE_MEMWRITE_BASE;
+	u32 de_base = malidp_get_block_base(hwdev, MALIDP_DE_BLOCK);
+
+	/* enable the scaling engine block */
+	malidp_hw_setbits(hwdev, MALIDP_SCALE_ENGINE_EN, de_base + MALIDP_DE_DISPLAY_FUNC);
+
+	malidp_hw_write(hwdev, fmt_id, base + MALIDP_MW_FORMAT);
+	switch (num_planes) {
+	case 2:
+		malidp_hw_write(hwdev, lower_32_bits(addrs[1]), base + MALIDP_MW_P2_PTR_LOW);
+		malidp_hw_write(hwdev, upper_32_bits(addrs[1]), base + MALIDP_MW_P2_PTR_HIGH);
+		malidp_hw_write(hwdev, pitches[1], base + MALIDP_MW_P2_STRIDE);
+		/* fall through */
+	case 1:
+		malidp_hw_write(hwdev, lower_32_bits(addrs[0]), base + MALIDP_MW_P1_PTR_LOW);
+		malidp_hw_write(hwdev, upper_32_bits(addrs[0]), base + MALIDP_MW_P1_PTR_HIGH);
+		malidp_hw_write(hwdev, pitches[0], base + MALIDP_MW_P1_STRIDE);
+		break;
+	default:
+		WARN(1, "Invalid number of planes");
+	}
+
+	malidp_hw_write(hwdev, MALIDP_DE_H_ACTIVE(w) | MALIDP_DE_V_ACTIVE(h),
+			MALIDP550_SE_MEMWRITE_OUT_SIZE);
+	malidp_hw_setbits(hwdev, MALIDP550_SE_MEMWRITE_ONESHOT | MALIDP_SE_MEMWRITE_EN,
+			  MALIDP550_SE_CONTROL);
+
+	return 0;
+}
+
+static void malidp550_disable_memwrite(struct malidp_hw_device *hwdev)
+{
+	u32 base = malidp_get_block_base(hwdev, MALIDP_DE_BLOCK);
+	malidp_hw_clearbits(hwdev, MALIDP550_SE_MEMWRITE_ONESHOT | MALIDP_SE_MEMWRITE_EN,
+			    MALIDP550_SE_CONTROL);
+	malidp_hw_clearbits(hwdev, MALIDP_SCALE_ENGINE_EN, base + MALIDP_DE_DISPLAY_FUNC);
+}
+
 static int malidp650_query_hw(struct malidp_hw_device *hwdev)
 {
 	u32 conf = malidp_hw_read(hwdev, MALIDP550_CONFIG_ID);
@@ -466,7 +508,8 @@ static int malidp650_query_hw(struct malidp_hw_device *hwdev)
 					    MALIDP550_SE_IRQ_AXI_ERR,
 			},
 			.dc_irq_map = {
-				.irq_mask = MALIDP550_DC_IRQ_CONF_VALID,
+				.irq_mask = MALIDP550_DC_IRQ_CONF_VALID |
+					    MALIDP550_DC_IRQ_SE,
 				.vsync_irq = MALIDP550_DC_IRQ_CONF_VALID,
 			},
 			.pixel_formats = malidp550_de_formats,
@@ -480,6 +523,8 @@ static int malidp650_query_hw(struct malidp_hw_device *hwdev)
 		.set_config_valid = malidp550_set_config_valid,
 		.modeset = malidp550_modeset,
 		.rotmem_required = malidp550_rotmem_required,
+		.enable_memwrite = malidp550_enable_memwrite,
+		.disable_memwrite = malidp550_disable_memwrite,
 	},
 	[MALIDP_650] = {
 		.map = {
@@ -500,7 +545,8 @@ static int malidp650_query_hw(struct malidp_hw_device *hwdev)
 					    MALIDP550_SE_IRQ_AXI_ERR,
 			},
 			.dc_irq_map = {
-				.irq_mask = MALIDP550_DC_IRQ_CONF_VALID,
+				.irq_mask = MALIDP550_DC_IRQ_CONF_VALID |
+					    MALIDP550_DC_IRQ_SE,
 				.vsync_irq = MALIDP550_DC_IRQ_CONF_VALID,
 			},
 			.pixel_formats = malidp550_de_formats,
@@ -514,6 +560,8 @@ static int malidp650_query_hw(struct malidp_hw_device *hwdev)
 		.set_config_valid = malidp550_set_config_valid,
 		.modeset = malidp550_modeset,
 		.rotmem_required = malidp550_rotmem_required,
+		.enable_memwrite = malidp550_enable_memwrite,
+		.disable_memwrite = malidp550_disable_memwrite,
 	},
 };
 
diff --git a/drivers/gpu/drm/arm/malidp_hw.h b/drivers/gpu/drm/arm/malidp_hw.h
index 4f8c884..091171d 100644
--- a/drivers/gpu/drm/arm/malidp_hw.h
+++ b/drivers/gpu/drm/arm/malidp_hw.h
@@ -146,6 +146,24 @@ struct malidp_hw_device {
 	 */
 	int (*rotmem_required)(struct malidp_hw_device *hwdev, u16 w, u16 h, u32 fmt);
 
+	/**
+	 * Enable writing to memory the content of the next frame
+	 * @param hwdev - malidp_hw_device structure containing the HW description
+	 * @param addrs - array of addresses for each plane
+	 * @param pitches - array of pitches for each plane
+	 * @param num_planes - number of planes to be written
+	 * @param w - width of the output frame
+	 * @param h - height of the output frame
+	 * @param fmt_id - internal format ID of output buffer
+	 */
+	int (*enable_memwrite)(struct malidp_hw_device *hwdev, dma_addr_t *addrs,
+			       s32 *pitches, int num_planes, u16 w, u16 h, u32 fmt_id);
+
+	/*
+	 * Disable the writing to memory of the next frame's content.
+	 */
+	void (*disable_memwrite)(struct malidp_hw_device *hwdev);
+
 	u8 features;
 
 	u8 min_line_size;
diff --git a/drivers/gpu/drm/arm/malidp_regs.h b/drivers/gpu/drm/arm/malidp_regs.h
index 73fecb3..cab086c 100644
--- a/drivers/gpu/drm/arm/malidp_regs.h
+++ b/drivers/gpu/drm/arm/malidp_regs.h
@@ -64,6 +64,8 @@
 /* bit masks that are common between products */
 #define   MALIDP_CFG_VALID		(1 << 0)
 #define   MALIDP_DISP_FUNC_ILACED	(1 << 8)
+#define   MALIDP_SCALE_ENGINE_EN	(1 << 16)
+#define   MALIDP_SE_MEMWRITE_EN		(2 << 5)
 
 /* register offsets for IRQ management */
 #define MALIDP_REG_STATUS		0x00000
@@ -92,6 +94,15 @@
 #define MALIDP_DE_H_ACTIVE(x)		(((x) & 0x1fff) << 0)
 #define MALIDP_DE_V_ACTIVE(x)		(((x) & 0x1fff) << 16)
 
+/* register offsets relative to MALIDP5x0_SE_MEMWRITE_BASE */
+#define MALIDP_MW_FORMAT		0x00000
+#define MALIDP_MW_P1_STRIDE		0x00004
+#define MALIDP_MW_P2_STRIDE		0x00008
+#define MALIDP_MW_P1_PTR_LOW		0x0000c
+#define MALIDP_MW_P1_PTR_HIGH		0x00010
+#define MALIDP_MW_P2_PTR_LOW		0x0002c
+#define MALIDP_MW_P2_PTR_HIGH		0x00030
+
 /* register offsets and bits specific to DP500 */
 #define MALIDP500_DC_BASE		0x00000
 #define MALIDP500_DC_CONTROL		0x0000c
@@ -149,6 +160,10 @@
 #define MALIDP550_DE_LS_PTR_BASE	0x0042c
 #define MALIDP550_DE_PERF_BASE		0x00500
 #define MALIDP550_SE_BASE		0x08000
+#define MALIDP550_SE_CONTROL		0x08010
+#define   MALIDP550_SE_MEMWRITE_ONESHOT	(1 << 7)
+#define MALIDP550_SE_MEMWRITE_OUT_SIZE	0x08030
+#define MALIDP550_SE_MEMWRITE_BASE	0x08100
 #define MALIDP550_DC_BASE		0x0c000
 #define MALIDP550_DC_CONTROL		0x0c010
 #define   MALIDP550_DC_CONFIG_REQ	(1 << 16)
-- 
1.7.9.5

[PATCH 5/6] drm: mali-dp: Add RGB writeback formats for DP550/DP650

[Brian Starkey]

Add a layer bit for the SE memory-write, and add it to the pixel format
matrix for DP550/DP650.

Signed-off-by: Brian Starkey <brian.starkey@arm.com>
---
 drivers/gpu/drm/arm/malidp_hw.c |   28 ++++++++++++++--------------
 drivers/gpu/drm/arm/malidp_hw.h |    1 +
 2 files changed, 15 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/arm/malidp_hw.c b/drivers/gpu/drm/arm/malidp_hw.c
index c696e67..be17631 100644
--- a/drivers/gpu/drm/arm/malidp_hw.c
+++ b/drivers/gpu/drm/arm/malidp_hw.c
@@ -46,20 +46,20 @@
 
 #define MALIDP_COMMON_FORMATS \
 	/*    fourcc,   layers supporting the format,      internal id   */ \
-	{ DRM_FORMAT_ARGB2101010, DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2, MALIDP_ID(0, 0) }, \
-	{ DRM_FORMAT_ABGR2101010, DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2, MALIDP_ID(0, 1) }, \
-	{ DRM_FORMAT_RGBA1010102, DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2, MALIDP_ID(0, 2) }, \
-	{ DRM_FORMAT_BGRA1010102, DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2, MALIDP_ID(0, 3) }, \
-	{ DRM_FORMAT_ARGB8888, DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2 | DE_SMART, MALIDP_ID(1, 0) }, \
-	{ DRM_FORMAT_ABGR8888, DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2 | DE_SMART, MALIDP_ID(1, 1) }, \
-	{ DRM_FORMAT_RGBA8888, DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2 | DE_SMART, MALIDP_ID(1, 2) }, \
-	{ DRM_FORMAT_BGRA8888, DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2 | DE_SMART, MALIDP_ID(1, 3) }, \
-	{ DRM_FORMAT_XRGB8888, DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2 | DE_SMART, MALIDP_ID(2, 0) }, \
-	{ DRM_FORMAT_XBGR8888, DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2 | DE_SMART, MALIDP_ID(2, 1) }, \
-	{ DRM_FORMAT_RGBX8888, DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2 | DE_SMART, MALIDP_ID(2, 2) }, \
-	{ DRM_FORMAT_BGRX8888, DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2 | DE_SMART, MALIDP_ID(2, 3) }, \
-	{ DRM_FORMAT_RGB888, DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2, MALIDP_ID(3, 0) }, \
-	{ DRM_FORMAT_BGR888, DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2, MALIDP_ID(3, 1) }, \
+	{ DRM_FORMAT_ARGB2101010, DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2 | SE_MEMWRITE, MALIDP_ID(0, 0) }, \
+	{ DRM_FORMAT_ABGR2101010, DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2 | SE_MEMWRITE, MALIDP_ID(0, 1) }, \
+	{ DRM_FORMAT_RGBA1010102, DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2 | SE_MEMWRITE, MALIDP_ID(0, 2) }, \
+	{ DRM_FORMAT_BGRA1010102, DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2 | SE_MEMWRITE, MALIDP_ID(0, 3) }, \
+	{ DRM_FORMAT_ARGB8888, DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2 | DE_SMART | SE_MEMWRITE, MALIDP_ID(1, 0) }, \
+	{ DRM_FORMAT_ABGR8888, DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2 | DE_SMART | SE_MEMWRITE, MALIDP_ID(1, 1) }, \
+	{ DRM_FORMAT_RGBA8888, DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2 | DE_SMART | SE_MEMWRITE, MALIDP_ID(1, 2) }, \
+	{ DRM_FORMAT_BGRA8888, DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2 | DE_SMART | SE_MEMWRITE, MALIDP_ID(1, 3) }, \
+	{ DRM_FORMAT_XRGB8888, DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2 | DE_SMART | SE_MEMWRITE, MALIDP_ID(2, 0) }, \
+	{ DRM_FORMAT_XBGR8888, DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2 | DE_SMART | SE_MEMWRITE, MALIDP_ID(2, 1) }, \
+	{ DRM_FORMAT_RGBX8888, DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2 | DE_SMART | SE_MEMWRITE, MALIDP_ID(2, 2) }, \
+	{ DRM_FORMAT_BGRX8888, DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2 | DE_SMART | SE_MEMWRITE, MALIDP_ID(2, 3) }, \
+	{ DRM_FORMAT_RGB888, DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2 | SE_MEMWRITE, MALIDP_ID(3, 0) }, \
+	{ DRM_FORMAT_BGR888, DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2 | SE_MEMWRITE, MALIDP_ID(3, 1) }, \
 	{ DRM_FORMAT_RGBA5551, DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2, MALIDP_ID(4, 0) }, \
 	{ DRM_FORMAT_ABGR1555, DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2, MALIDP_ID(4, 1) }, \
 	{ DRM_FORMAT_RGB565, DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2, MALIDP_ID(4, 2) }, \
diff --git a/drivers/gpu/drm/arm/malidp_hw.h b/drivers/gpu/drm/arm/malidp_hw.h
index 091171d..8056efa 100644
--- a/drivers/gpu/drm/arm/malidp_hw.h
+++ b/drivers/gpu/drm/arm/malidp_hw.h
@@ -33,6 +33,7 @@ enum {
 	DE_GRAPHICS2 = BIT(2), /* used only in DP500 */
 	DE_VIDEO2 = BIT(3),
 	DE_SMART = BIT(4),
+	SE_MEMWRITE = BIT(5),
 };
 
 struct malidp_format_id {
-- 
1.7.9.5

[PATCH 6/6] drm: mali-dp: Add writeback connector

[Brian Starkey]

Mali-DP has a memory writeback engine which can be used to write the
composition result to a memory buffer. Expose this functionality as a
DRM writeback connector on supported hardware.

Changes since v1:
 Daniel Vetter:
 - Don't require a modeset when writeback routing changes
 - Make writeback connector always disconnected

Changes since v2:
 - Rebase onto new drm_writeback_connector
 - Add reset callback, allocating subclassed state
 Daniel Vetter:
 - Squash out-fence support into this commit
 Gustavo Padovan:
 - Don't signal fence directly from driver (and drop malidp_mw_job)

Signed-off-by: Brian Starkey <brian.starkey@arm.com>
---
 drivers/gpu/drm/arm/Makefile      |    1 +
 drivers/gpu/drm/arm/malidp_crtc.c |   21 +++
 drivers/gpu/drm/arm/malidp_drv.c  |   25 +++-
 drivers/gpu/drm/arm/malidp_drv.h  |    3 +
 drivers/gpu/drm/arm/malidp_hw.c   |    7 +-
 drivers/gpu/drm/arm/malidp_mw.c   |  278 +++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/arm/malidp_mw.h   |   18 +++
 7 files changed, 348 insertions(+), 5 deletions(-)
 create mode 100644 drivers/gpu/drm/arm/malidp_mw.c
 create mode 100644 drivers/gpu/drm/arm/malidp_mw.h

diff --git a/drivers/gpu/drm/arm/Makefile b/drivers/gpu/drm/arm/Makefile
index bb8b158..3bf31d1 100644
--- a/drivers/gpu/drm/arm/Makefile
+++ b/drivers/gpu/drm/arm/Makefile
@@ -1,4 +1,5 @@
 hdlcd-y := hdlcd_drv.o hdlcd_crtc.o
 obj-$(CONFIG_DRM_HDLCD)	+= hdlcd.o
 mali-dp-y := malidp_drv.o malidp_hw.o malidp_planes.o malidp_crtc.o
+mali-dp-y += malidp_mw.o
 obj-$(CONFIG_DRM_MALI_DISPLAY)	+= mali-dp.o
diff --git a/drivers/gpu/drm/arm/malidp_crtc.c b/drivers/gpu/drm/arm/malidp_crtc.c
index 08e6a71..5413a5a 100644
--- a/drivers/gpu/drm/arm/malidp_crtc.c
+++ b/drivers/gpu/drm/arm/malidp_crtc.c
@@ -68,6 +68,18 @@ static void malidp_crtc_enable(struct drm_crtc *crtc)
 	clk_set_rate(hwdev->pxlclk, crtc->state->adjusted_mode.crtc_clock * 1000);
 
 	hwdev->modeset(hwdev, &vm);
+	/*
+	 * We should always disable the memory write when leaving config mode,
+	 * otherwise the hardware will start writing right away - possibly with
+	 * a stale config, and definitely before we've had a chance to configure
+	 * the planes.
+	 * If the memory write needs to be enabled, that will get taken care
+	 * of later during the atomic commit
+	 */
+	if (hwdev->disable_memwrite) {
+		DRM_DEV_DEBUG_DRIVER(crtc->dev->dev, "Disable memwrite\n");
+		hwdev->disable_memwrite(hwdev);
+	}
 	hwdev->leave_config_mode(hwdev);
 	drm_crtc_vblank_on(crtc);
 }
@@ -157,6 +169,15 @@ static int malidp_crtc_atomic_check(struct drm_crtc *crtc,
 		}
 	}
 
+	/* If only the writeback routing has changed, we don't need a modeset */
+	if (state->connectors_changed) {
+		u32 old_mask = crtc->state->connector_mask;
+		u32 new_mask = state->connector_mask;
+		if ((old_mask ^ new_mask) ==
+		    (1 << drm_connector_index(&malidp->mw_connector.base)))
+			state->connectors_changed = false;
+	}
+
 	return 0;
 }
 
diff --git a/drivers/gpu/drm/arm/malidp_drv.c b/drivers/gpu/drm/arm/malidp_drv.c
index 32f746e..2d0465b 100644
--- a/drivers/gpu/drm/arm/malidp_drv.c
+++ b/drivers/gpu/drm/arm/malidp_drv.c
@@ -28,6 +28,7 @@
 #include <drm/drm_of.h>
 
 #include "malidp_drv.h"
+#include "malidp_mw.h"
 #include "malidp_regs.h"
 #include "malidp_hw.h"
 
@@ -92,6 +93,14 @@ static void malidp_atomic_commit_tail(struct drm_atomic_state *state)
 
 	drm_atomic_helper_commit_modeset_disables(drm, state);
 	drm_atomic_helper_commit_modeset_enables(drm, state);
+
+	/*
+	 * The order here is important. We must configure memory-write after
+	 * the CRTC is already enabled, so that its configuration update is
+	 * gated on the next CVAL.
+	 */
+	malidp_mw_atomic_commit(drm, state);
+
 	drm_atomic_helper_commit_planes(drm, state, 0);
 
 	malidp_atomic_commit_hw_done(state);
@@ -147,12 +156,20 @@ static int malidp_init(struct drm_device *drm)
 	drm->mode_config.helper_private = &malidp_mode_config_helpers;
 
 	ret = malidp_crtc_init(drm);
-	if (ret) {
-		drm_mode_config_cleanup(drm);
-		return ret;
-	}
+	if (ret)
+		goto crtc_fail;
+
+	ret = malidp_mw_connector_init(drm);
+	if (ret)
+		goto mw_fail;
 
 	return 0;
+
+mw_fail:
+	malidp_de_planes_destroy(drm);
+crtc_fail:
+	drm_mode_config_cleanup(drm);
+	return ret;
 }
 
 static void malidp_fini(struct drm_device *drm)
diff --git a/drivers/gpu/drm/arm/malidp_drv.h b/drivers/gpu/drm/arm/malidp_drv.h
index 9fc8a2e..8abfa8a 100644
--- a/drivers/gpu/drm/arm/malidp_drv.h
+++ b/drivers/gpu/drm/arm/malidp_drv.h
@@ -13,6 +13,7 @@
 #ifndef __MALIDP_DRV_H__
 #define __MALIDP_DRV_H__
 
+#include <drm/drm_writeback.h>
 #include <linux/mutex.h>
 #include <linux/wait.h>
 #include "malidp_hw.h"
@@ -22,6 +23,8 @@ struct malidp_drm {
 	struct drm_fbdev_cma *fbdev;
 	struct list_head event_list;
 	struct drm_crtc crtc;
+	struct drm_encoder mw_encoder;
+	struct drm_writeback_connector mw_connector;
 	wait_queue_head_t wq;
 	atomic_t config_valid;
 };
diff --git a/drivers/gpu/drm/arm/malidp_hw.c b/drivers/gpu/drm/arm/malidp_hw.c
index be17631..1fee5ee 100644
--- a/drivers/gpu/drm/arm/malidp_hw.c
+++ b/drivers/gpu/drm/arm/malidp_hw.c
@@ -20,6 +20,7 @@
 
 #include "malidp_drv.h"
 #include "malidp_hw.h"
+#include "malidp_mw.h"
 
 static const struct malidp_format_id malidp500_de_formats[] = {
 	/*    fourcc,   layers supporting the format,     internal id  */
@@ -543,6 +544,7 @@ static int malidp650_query_hw(struct malidp_hw_device *hwdev)
 			.se_irq_map = {
 				.irq_mask = MALIDP550_SE_IRQ_EOW |
 					    MALIDP550_SE_IRQ_AXI_ERR,
+				.vsync_irq = MALIDP550_SE_IRQ_EOW,
 			},
 			.dc_irq_map = {
 				.irq_mask = MALIDP550_DC_IRQ_CONF_VALID |
@@ -684,6 +686,7 @@ static irqreturn_t malidp_se_irq(int irq, void *arg)
 	struct drm_device *drm = arg;
 	struct malidp_drm *malidp = drm->dev_private;
 	struct malidp_hw_device *hwdev = malidp->dev;
+	const struct malidp_irq_map *se = &hwdev->map.se_irq_map;
 	u32 status, mask;
 
 	status = malidp_hw_read(hwdev, hwdev->map.se_base + MALIDP_REG_STATUS);
@@ -693,7 +696,9 @@ static irqreturn_t malidp_se_irq(int irq, void *arg)
 	mask = malidp_hw_read(hwdev, hwdev->map.se_base + MALIDP_REG_MASKIRQ);
 	status = malidp_hw_read(hwdev, hwdev->map.se_base + MALIDP_REG_STATUS);
 	status &= mask;
-	/* ToDo: status decoding and firing up of VSYNC and page flip events */
+
+	if (status & se->vsync_irq)
+		drm_writeback_signal_completion(&malidp->mw_connector, 0);
 
 	malidp_hw_clear_irq(hwdev, MALIDP_SE_BLOCK, status);
 
diff --git a/drivers/gpu/drm/arm/malidp_mw.c b/drivers/gpu/drm/arm/malidp_mw.c
new file mode 100644
index 0000000..40f1137
--- /dev/null
+++ b/drivers/gpu/drm/arm/malidp_mw.c
@@ -0,0 +1,278 @@
+/*
+ * (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.
+ *
+ * ARM Mali DP Writeback connector implementation
+ */
+#include <drm/drm_atomic.h>
+#include <drm/drm_atomic_helper.h>
+#include <drm/drm_crtc.h>
+#include <drm/drm_crtc_helper.h>
+#include <drm/drm_fb_cma_helper.h>
+#include <drm/drm_gem_cma_helper.h>
+#include <drm/drmP.h>
+#include <drm/drm_writeback.h>
+
+#include "malidp_drv.h"
+#include "malidp_hw.h"
+#include "malidp_mw.h"
+
+#define to_mw_state(_state) (struct malidp_mw_connector_state *)(_state)
+
+struct malidp_mw_connector_state {
+	struct drm_connector_state base;
+	dma_addr_t addrs[2];
+	s32 pitches[2];
+	u8 format;
+	u8 n_planes;
+};
+
+static int malidp_mw_connector_get_modes(struct drm_connector *connector)
+{
+	struct drm_device *dev = connector->dev;
+
+	return drm_add_modes_noedid(connector, dev->mode_config.max_width,
+				    dev->mode_config.max_height);
+}
+
+static enum drm_mode_status
+malidp_mw_connector_mode_valid(struct drm_connector *connector,
+			       struct drm_display_mode *mode)
+{
+	struct drm_device *dev = connector->dev;
+	struct drm_mode_config *mode_config = &dev->mode_config;
+	int w = mode->hdisplay, h = mode->vdisplay;
+
+	if ((w < mode_config->min_width) || (w > mode_config->max_width))
+		return MODE_BAD_HVALUE;
+
+	if ((h < mode_config->min_height) || (h > mode_config->max_height))
+		return MODE_BAD_VVALUE;
+
+	return MODE_OK;
+}
+
+const struct drm_connector_helper_funcs malidp_mw_connector_helper_funcs = {
+	.get_modes = malidp_mw_connector_get_modes,
+	.mode_valid = malidp_mw_connector_mode_valid,
+};
+
+static void malidp_mw_connector_reset(struct drm_connector *connector)
+{
+	struct malidp_mw_connector_state *mw_state =
+		kzalloc(sizeof(*mw_state), GFP_KERNEL);
+
+	if (connector->state)
+		__drm_atomic_helper_connector_destroy_state(connector->state);
+
+	kfree(connector->state);
+	__drm_atomic_helper_connector_reset(connector, &mw_state->base);
+}
+
+static enum drm_connector_status
+malidp_mw_connector_detect(struct drm_connector *connector, bool force)
+{
+	return connector_status_disconnected;
+}
+
+static void malidp_mw_connector_destroy(struct drm_connector *connector)
+{
+	drm_connector_cleanup(connector);
+}
+
+static struct drm_connector_state *
+malidp_mw_connector_duplicate_state(struct drm_connector *connector)
+{
+	struct malidp_mw_connector_state *mw_state;
+
+	if (WARN_ON(!connector->state))
+		return NULL;
+
+	mw_state = kzalloc(sizeof(*mw_state), GFP_KERNEL);
+	if (!mw_state)
+		return NULL;
+
+	/* No need to preserve any of our driver-local data */
+	__drm_atomic_helper_connector_duplicate_state(connector, &mw_state->base);
+
+	return &mw_state->base;
+}
+
+static const struct drm_connector_funcs malidp_mw_connector_funcs = {
+	.dpms = drm_atomic_helper_connector_dpms,
+	.reset = malidp_mw_connector_reset,
+	.detect = malidp_mw_connector_detect,
+	.fill_modes = drm_helper_probe_single_connector_modes,
+	.destroy = malidp_mw_connector_destroy,
+	.atomic_duplicate_state = malidp_mw_connector_duplicate_state,
+	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
+};
+
+static int
+malidp_mw_encoder_atomic_check(struct drm_encoder *encoder,
+			       struct drm_crtc_state *crtc_state,
+			       struct drm_connector_state *conn_state)
+{
+	struct malidp_mw_connector_state *mw_state = to_mw_state(conn_state);
+	struct malidp_drm *malidp = encoder->dev->dev_private;
+	struct drm_framebuffer *fb;
+	int i, n_planes;
+
+	if (!conn_state->writeback_job || !conn_state->writeback_job->fb)
+		return 0;
+
+	fb = conn_state->writeback_job->fb;
+	if ((fb->width != crtc_state->mode.hdisplay) ||
+	    (fb->height != crtc_state->mode.vdisplay)) {
+		DRM_DEBUG_KMS("Invalid framebuffer size %ux%u\n",
+				fb->width, fb->height);
+		return -EINVAL;
+	}
+
+	mw_state->format =
+		malidp_hw_get_format_id(&malidp->dev->map, SE_MEMWRITE,
+					fb->pixel_format);
+	if (mw_state->format == MALIDP_INVALID_FORMAT_ID) {
+		struct drm_format_name_buf format_name;
+
+		DRM_DEBUG_KMS("Invalid pixel format %s\n",
+			      drm_get_format_name(fb->pixel_format, &format_name));
+		return -EINVAL;
+	}
+
+	n_planes = drm_format_num_planes(fb->pixel_format);
+	for (i = 0; i < n_planes; i++) {
+		struct drm_gem_cma_object *obj = drm_fb_cma_get_gem_obj(fb, i);
+		if (!malidp_hw_pitch_valid(malidp->dev, fb->pitches[i])) {
+			DRM_DEBUG_KMS("Invalid pitch %u for plane %d\n",
+				      fb->pitches[i], i);
+			return -EINVAL;
+		}
+		mw_state->pitches[i] = fb->pitches[i];
+		mw_state->addrs[i] = obj->paddr + fb->offsets[i];
+	}
+	mw_state->n_planes = n_planes;
+
+	return 0;
+}
+
+static const struct drm_encoder_helper_funcs malidp_mw_encoder_helper_funcs = {
+	.atomic_check = malidp_mw_encoder_atomic_check,
+};
+
+static void malidp_mw_encoder_destroy(struct drm_encoder *encoder)
+{
+	drm_encoder_cleanup(encoder);
+}
+
+static const struct drm_encoder_funcs malidp_mw_encoder_funcs = {
+	.destroy = malidp_mw_encoder_destroy,
+};
+
+static u32 *get_writeback_formats(struct malidp_drm *malidp, int *n_formats)
+{
+	const struct malidp_hw_regmap *map = &malidp->dev->map;
+	u32 *formats;
+	int n, i;
+
+	formats = kcalloc(map->n_pixel_formats, sizeof(*formats),
+			  GFP_KERNEL);
+	if (!formats)
+		return NULL;
+
+	for (n = 0, i = 0;  i < map->n_pixel_formats; i++) {
+		if (map->pixel_formats[i].layer & SE_MEMWRITE)
+			formats[n++] = map->pixel_formats[i].format;
+	}
+
+	*n_formats = n;
+	return formats;
+}
+
+int malidp_mw_connector_init(struct drm_device *drm)
+{
+	struct malidp_drm *malidp = drm->dev_private;
+	u32 *formats;
+	int ret, n_formats;
+
+	if (!malidp->dev->enable_memwrite)
+		return 0;
+
+	drm_encoder_helper_add(&malidp->mw_encoder, &malidp_mw_encoder_helper_funcs);
+	malidp->mw_encoder.possible_crtcs = 1 << drm_crtc_index(&malidp->crtc);
+	ret = drm_encoder_init(drm, &malidp->mw_encoder, &malidp_mw_encoder_funcs,
+			       DRM_MODE_ENCODER_VIRTUAL, NULL);
+	if (ret)
+		return ret;
+
+	drm_connector_helper_add(&malidp->mw_connector.base,
+				 &malidp_mw_connector_helper_funcs);
+	malidp->mw_connector.base.interlace_allowed = 0;
+
+	formats = get_writeback_formats(malidp, &n_formats);
+	if (!formats) {
+		ret = -ENOMEM;
+		goto err_encoder;
+	}
+
+	ret = drm_writeback_connector_init(drm, &malidp->mw_connector,
+					   &malidp_mw_connector_funcs,
+					   formats, n_formats);
+	kfree(formats);
+	if (ret)
+		goto err_encoder;
+
+	ret = drm_mode_connector_attach_encoder(&malidp->mw_connector.base,
+						&malidp->mw_encoder);
+	if (ret)
+		goto err_connector;
+
+	return 0;
+
+err_connector:
+	drm_connector_cleanup(&malidp->mw_connector.base);
+err_encoder:
+	drm_encoder_cleanup(&malidp->mw_encoder);
+	return ret;
+}
+
+void malidp_mw_atomic_commit(struct drm_device *drm,
+			     struct drm_atomic_state *old_state)
+{
+	struct malidp_drm *malidp = drm->dev_private;
+	struct drm_writeback_connector *mw_conn = &malidp->mw_connector;
+	struct drm_connector_state *conn_state = mw_conn->base.state;
+	struct malidp_hw_device *hwdev = malidp->dev;
+	struct malidp_mw_connector_state *mw_state;
+
+	if (!conn_state)
+		return;
+
+	mw_state = to_mw_state(conn_state);
+
+	if (conn_state->writeback_job && conn_state->writeback_job->fb) {
+		struct drm_framebuffer *fb = conn_state->writeback_job->fb;
+
+		DRM_DEV_DEBUG_DRIVER(drm->dev,
+				     "Enable memwrite %ux%u:%d %pad fmt: %u\n",
+				     fb->width, fb->height,
+				     mw_state->pitches[0],
+				     &mw_state->addrs[0],
+				     mw_state->format);
+
+		drm_writeback_queue_job(mw_conn, conn_state->writeback_job);
+		conn_state->writeback_job = NULL;
+
+		hwdev->enable_memwrite(hwdev, mw_state->addrs,
+				       mw_state->pitches, mw_state->n_planes,
+				       fb->width, fb->height, mw_state->format);
+	} else {
+		DRM_DEV_DEBUG_DRIVER(drm->dev, "Disable memwrite\n");
+		hwdev->disable_memwrite(hwdev);
+	}
+}
diff --git a/drivers/gpu/drm/arm/malidp_mw.h b/drivers/gpu/drm/arm/malidp_mw.h
new file mode 100644
index 0000000..93477ea
--- /dev/null
+++ b/drivers/gpu/drm/arm/malidp_mw.h
@@ -0,0 +1,18 @@
+/*
+ * (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 __MALIDP_MW_H__
+#define __MALIDP_MW_H__
+
+int malidp_mw_connector_init(struct drm_device *drm);
+void malidp_mw_atomic_commit(struct drm_device *drm,
+			     struct drm_atomic_state *old_state);
+#endif
-- 
1.7.9.5

Re: [RFC PATCH v3 0/6] Introduce writeback connectors

[Boris Brezillon]

Hi Brian,

On Fri, 25 Nov 2016 16:48:58 +0000
Brian Starkey <brian.starkey@arm.com> wrote:

> Hi,
> 
> This is v3 of my series introducing a new connector type:
>  DRM_MODE_CONNECTOR_WRITEBACK
> See v1 and v2 here: [1] [2]
> 
> Writeback connectors are used to expose the memory writeback engines
> found in some display controllers, which can write a CRTC's
> composition result to a memory buffer.
> This is useful e.g. for testing, screen-recording, screenshots,
> wireless display, display cloning, memory-to-memory composition.
> 
> Writeback connectors are given a WRITEBACK_FB_ID property (which acts
> slightly differently to FB_ID, so gets a new name), as well as
> a PIXEL_FORMATS blob to list the supported writeback formats, and
> OUT_FENCE_PTR to be used for out-fences.
> 
> The changes since v2 are in the commit messages of each commit.
> 
> The main differences are:
>  - Subclass drm_connector as drm_writeback_connector
>  - Slight relaxation of core checks, to allow
>    (connector->crtc && !connector->fb)
>  - Dropped PIXEL_FORMATS_SIZE, which was redundant
>  - Reworked the event interface, drivers don't need to deal with the
>    fence directly
>  - Re-ordered the commits to introduce writeback out-fences up-front.
> 
> I've kept RFC on this series because the event reporting (introduction
> of drm_writeback_job) is probably up for debate.
> 
> v4 will be accompanied by igt tests.

I plan to add writeback support to the VC4 driver and wanted to know if
anything has changed since this v3 (IOW, do you have a v4 + igt tests
ready)?

> 
> As always, I look forward to any comments.

I'll try to review these patches. Keep in mind that I didn't follow the
initial discussions and might suggest things or ask questions that have
already been answered in previous versions of this series or on IRC.

Regards,

Boris

Re: [PATCH 6/6] drm: mali-dp: Add writeback connector

[Boris Brezillon]

On Fri, 25 Nov 2016 16:49:04 +0000
Brian Starkey <brian.starkey@arm.com> wrote:

> +static int
> +malidp_mw_encoder_atomic_check(struct drm_encoder *encoder,
> +			       struct drm_crtc_state *crtc_state,
> +			       struct drm_connector_state *conn_state)
> +{
> +	struct malidp_mw_connector_state *mw_state = to_mw_state(conn_state);
> +	struct malidp_drm *malidp = encoder->dev->dev_private;
> +	struct drm_framebuffer *fb;
> +	int i, n_planes;
> +
> +	if (!conn_state->writeback_job || !conn_state->writeback_job->fb)
> +		return 0;
> +
> +	fb = conn_state->writeback_job->fb;
> +	if ((fb->width != crtc_state->mode.hdisplay) ||
> +	    (fb->height != crtc_state->mode.vdisplay)) {
> +		DRM_DEBUG_KMS("Invalid framebuffer size %ux%u\n",
> +				fb->width, fb->height);
> +		return -EINVAL;
> +	}

These checks look pretty generic to me. Shouldn't we have a default
helper doing that?

> +
> +	mw_state->format =
> +		malidp_hw_get_format_id(&malidp->dev->map, SE_MEMWRITE,
> +					fb->pixel_format);
> +	if (mw_state->format == MALIDP_INVALID_FORMAT_ID) {

Same goes here. By adding a format_types table similar to what is
exposed in drm_plane [1], we could do this check in the core. The only
thing left to the driver is the 4CC -> driver-specific-id conversion.

> +		struct drm_format_name_buf format_name;
> +
> +		DRM_DEBUG_KMS("Invalid pixel format %s\n",
> +			      drm_get_format_name(fb->pixel_format, &format_name));
> +		return -EINVAL;
> +	}
> +
> +	n_planes = drm_format_num_planes(fb->pixel_format);
> +	for (i = 0; i < n_planes; i++) {
> +		struct drm_gem_cma_object *obj = drm_fb_cma_get_gem_obj(fb, i);
> +		if (!malidp_hw_pitch_valid(malidp->dev, fb->pitches[i])) {
> +			DRM_DEBUG_KMS("Invalid pitch %u for plane %d\n",
> +				      fb->pitches[i], i);
> +			return -EINVAL;
> +		}
> +		mw_state->pitches[i] = fb->pitches[i];
> +		mw_state->addrs[i] = obj->paddr + fb->offsets[i];
> +	}
> +	mw_state->n_planes = n_planes;
> +
> +	return 0;
> +}


[1]http://lxr.free-electrons.com/source/include/drm/drm_plane.h#L482

Re: [PATCH 1/6] drm: Add writeback connector type

[Boris Brezillon]

On Fri, 25 Nov 2016 16:48:59 +0000
Brian Starkey <brian.starkey@arm.com> wrote:


>  
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index b5c6a8e..6bbd93f 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -86,6 +86,7 @@ struct drm_conn_prop_enum_list {
>  	{ DRM_MODE_CONNECTOR_VIRTUAL, "Virtual" },
>  	{ DRM_MODE_CONNECTOR_DSI, "DSI" },
>  	{ DRM_MODE_CONNECTOR_DPI, "DPI" },
> +	{ DRM_MODE_CONNECTOR_WRITEBACK, "Writeback" },

Is there a reason we have a Writeback connector, but keep using a
Virtual encoder to connect it to the CRTC? Wouldn't it make more sense
to also add a Writeback encoder?

>  };
>  
>  void drm_connector_ida_init(void)
> @@ -235,7 +236,8 @@ int drm_connector_init(struct drm_device *dev,
>  	list_add_tail(&connector->head, &config->connector_list);
>  	config->num_connector++;
>  
> -	if (connector_type != DRM_MODE_CONNECTOR_VIRTUAL)
> +	if ((connector_type != DRM_MODE_CONNECTOR_VIRTUAL) &&
> +	    (connector_type != DRM_MODE_CONNECTOR_WRITEBACK))

Nitpick: you don't need the extra parenthesis:

	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/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index 34f9741..dc4910d6 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -214,6 +214,19 @@ struct drm_connector_state {
>  	struct drm_encoder *best_encoder;
>  
>  	struct drm_atomic_state *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.
> +	 *
> +	 * See also: drm_writeback_queue_job() and
> +	 * drm_writeback_signal_completion()
> +	 */
> +	struct drm_writeback_job *writeback_job;

Maybe I'm wrong, but is feels weird to have the writeback_job field
directly embedded in drm_connector_state, while drm_writeback_connector
inherits from drm_connector.

IMO, either you decide to directly put the drm_writeback_connector's
job_xxx fields in drm_connector and keep the drm_connector_state as is,
or you create a drm_writeback_connector_state which inherits from
drm_connector_state and embeds the writeback_job field.

Anyway, wait for Daniel's feedback before doing this change.

>  };
>  
>  /**
> diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
> index bf9991b2..3d3d07f 100644
> --- a/include/drm/drm_mode_config.h
> +++ b/include/drm/drm_mode_config.h
> @@ -634,6 +634,20 @@ struct drm_mode_config {
>  	 */
>  	struct drm_property *suggested_y_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_writeback.h b/include/drm/drm_writeback.h
> new file mode 100644
> index 0000000..6b2ac45
> --- /dev/null
> +++ b/include/drm/drm_writeback.h
> @@ -0,0 +1,78 @@
> +/*
> + * (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 <linux/workqueue.h>
> +
> +struct drm_writeback_connector {
> +	struct drm_connector base;

AFAIU, a writeback connector will always require an 'dummy' encoder to
make the DRM framework happy (AFAIK, a connector is always connected to
a CRTC through an encoder).

Wouldn't it make more sense to have a drm_encoder object embedded in
drm_writeback_connector so that people don't have to declare an extra
structure containing both the drm_writeback_connector connector and a
drm_encoder? Is there a good reason to keep them separate?

> +
> +	/**
> +	 * @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;
> +};

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

[Boris Brezillon]

On Fri, 25 Nov 2016 16:49:00 +0000
Brian Starkey <brian.starkey@arm.com> wrote:

> Add the 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 *

Don't know what happened, but I still see s32 __user * types in this
patch (I had to patch it to make in work on top of 4.11-rc1).

Re: [RFC PATCH v3 0/6] Introduce writeback connectors

[Brian Starkey]

Hi Boris,

On Fri, Apr 14, 2017 at 11:35:17AM +0200, Boris Brezillon wrote:
>Hi Brian,
>
>On Fri, 25 Nov 2016 16:48:58 +0000
>Brian Starkey <brian.starkey@arm.com> wrote:
>
>> Hi,
>>
>> This is v3 of my series introducing a new connector type:
>>  DRM_MODE_CONNECTOR_WRITEBACK
>> See v1 and v2 here: [1] [2]
>>
>> Writeback connectors are used to expose the memory writeback engines
>> found in some display controllers, which can write a CRTC's
>> composition result to a memory buffer.
>> This is useful e.g. for testing, screen-recording, screenshots,
>> wireless display, display cloning, memory-to-memory composition.
>>
>> Writeback connectors are given a WRITEBACK_FB_ID property (which acts
>> slightly differently to FB_ID, so gets a new name), as well as
>> a PIXEL_FORMATS blob to list the supported writeback formats, and
>> OUT_FENCE_PTR to be used for out-fences.
>>
>> The changes since v2 are in the commit messages of each commit.
>>
>> The main differences are:
>>  - Subclass drm_connector as drm_writeback_connector
>>  - Slight relaxation of core checks, to allow
>>    (connector->crtc && !connector->fb)
>>  - Dropped PIXEL_FORMATS_SIZE, which was redundant
>>  - Reworked the event interface, drivers don't need to deal with the
>>    fence directly
>>  - Re-ordered the commits to introduce writeback out-fences up-front.
>>
>> I've kept RFC on this series because the event reporting (introduction
>> of drm_writeback_job) is probably up for debate.
>>
>> v4 will be accompanied by igt tests.
>
>I plan to add writeback support to the VC4 driver and wanted to know if
>anything has changed since this v3 (IOW, do you have a v4 + igt tests
>ready)?
>

Oh that's good to hear. I've got a v4 (just rebased for the most
part), but was holding off sending it until having some "proper"
userspace to support it. Unfortunately in the meantime we've had some
team changes which mean I'm not really able to work on it to move
things forward - Liviu might be able to pick this up.

I'll collect together what I have and send it out anyway. It includes
some functional tests in igt, but I'm not sure if that meets the "new
uapi needs userspace" bar.

>>
>> As always, I look forward to any comments.
>
>I'll try to review these patches. Keep in mind that I didn't follow the
>initial discussions and might suggest things or ask questions that have
>already been answered in previous versions of this series or on IRC.

Thanks,

-Brian

>
>Regards,
>
>Boris

Re: [PATCH 1/6] drm: Add writeback connector type

[Brian Starkey]

Hi Boris,

On Fri, Apr 14, 2017 at 12:08:23PM +0200, Boris Brezillon wrote:
>On Fri, 25 Nov 2016 16:48:59 +0000
>Brian Starkey <brian.starkey@arm.com> wrote:
>
>
>>
>> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
>> index b5c6a8e..6bbd93f 100644
>> --- a/drivers/gpu/drm/drm_connector.c
>> +++ b/drivers/gpu/drm/drm_connector.c
>> @@ -86,6 +86,7 @@ struct drm_conn_prop_enum_list {
>>  	{ DRM_MODE_CONNECTOR_VIRTUAL, "Virtual" },
>>  	{ DRM_MODE_CONNECTOR_DSI, "DSI" },
>>  	{ DRM_MODE_CONNECTOR_DPI, "DPI" },
>> +	{ DRM_MODE_CONNECTOR_WRITEBACK, "Writeback" },
>
>Is there a reason we have a Writeback connector, but keep using a
>Virtual encoder to connect it to the CRTC? Wouldn't it make more sense
>to also add a Writeback encoder?
>

Only that a writeback connector is functionally and conceptually quite
different from the existing connector types, whereas the "encoder"
(which realistically only exists because the framework forces it to)
acts pretty much like any other.

>>  };
>>
>>  void drm_connector_ida_init(void)
>> @@ -235,7 +236,8 @@ int drm_connector_init(struct drm_device *dev,
>>  	list_add_tail(&connector->head, &config->connector_list);
>>  	config->num_connector++;
>>
>> -	if (connector_type != DRM_MODE_CONNECTOR_VIRTUAL)
>> +	if ((connector_type != DRM_MODE_CONNECTOR_VIRTUAL) &&
>> +	    (connector_type != DRM_MODE_CONNECTOR_WRITEBACK))
>
>Nitpick: you don't need the extra parenthesis:
>
>	if (connector_type != DRM_MODE_CONNECTOR_VIRTUAL &&
>	    connector_type != DRM_MODE_CONNECTOR_WRITEBACK)
>

Yeah fair enough, I can drop them.

>>  		drm_object_attach_property(&connector->base,
>>  					      config->edid_property,
>>  					      0);
>
>
>
>> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
>> index 34f9741..dc4910d6 100644
>> --- a/include/drm/drm_connector.h
>> +++ b/include/drm/drm_connector.h
>> @@ -214,6 +214,19 @@ struct drm_connector_state {
>>  	struct drm_encoder *best_encoder;
>>
>>  	struct drm_atomic_state *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.
>> +	 *
>> +	 * See also: drm_writeback_queue_job() and
>> +	 * drm_writeback_signal_completion()
>> +	 */
>> +	struct drm_writeback_job *writeback_job;
>
>Maybe I'm wrong, but is feels weird to have the writeback_job field
>directly embedded in drm_connector_state, while drm_writeback_connector
>inherits from drm_connector.
>
>IMO, either you decide to directly put the drm_writeback_connector's
>job_xxx fields in drm_connector and keep the drm_connector_state as is,
>or you create a drm_writeback_connector_state which inherits from
>drm_connector_state and embeds the writeback_job field.

I did spend a decent amount of time looking at tracking the writeback
state along with the normal connector state. I couldn't come up with
anything I liked.

As the comment mentions, one of the problems is that you have to make
sure the relevant parts of the connector_state stay around until the
writeback is finished. That means you've got to block before
"swap_state()" until the previous writeback is done, and that
effectively limits your frame rate to refresh/2.

The Mali-DP HW doesn't have that limitation - we can queue up a new
commit while the current writeback is ongoing. For that reason I
didn't want to impose such a limitation in the framework.

In v1 I allowed that by making the Mali-DP driver hold its own
references to the relevant bits of the state for as long as it needed
them. In v3 I moved most of that code back to the core (in part
because Gustavo didn't like me signalling the DRM-"owned" fence from
my driver code directly). I think the new approach of "queue_job()"
and "signal_job()" reduces the amount of tricky code in drivers, and
is generally more clear (also familiar, when compared to vsync
events).

I'm certain there's other ways to do it (refcount atomic states?), but
it seemed like a biggish overhaul to achieve what would basically be
the same thing.

I was expecting each driver supporting writeback to have its own
different requirements around writeback lifetime/duration. For example
I think VC4 specifically came up, in that its writeback could take
several frames, whereas on Mali-DP we either finish within the frame
or we fail.

Letting the driver manage its writeback_job lifetime seemed like a
reasonable way to handle all that, with the documentation stating the
only behaviour which is guaranteed to work on all drivers:

   *     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.



... so all of that is why the _job fields don't live in a *_state
structure directly, and instead have to live in the separately-managed
structure pointed to by ->writeback_job.

Now, I did look at creating drm_writeback_connector_state, but as it
would only be holding the job pointer (see above) it didn't seem worth
scattering around the

    if (conn_state->connector->connector_type ==
        DRM_MODE_CONNECTOR_WRITEBACK)

checks everywhere before up-casting - {clear,reset,duplicate}_state(),
prepare_signalling(), complete_signalling(), etc. It just touched a
lot of code for the sake of an extra pointer field in each connector
state.

I can easily revisit that part if you like.

>
>Anyway, wait for Daniel's feedback before doing this change.
>

Am I expecting some more feedback from Daniel?

>>  };
>>
>>  /**
>> diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
>> index bf9991b2..3d3d07f 100644
>> --- a/include/drm/drm_mode_config.h
>> +++ b/include/drm/drm_mode_config.h
>> @@ -634,6 +634,20 @@ struct drm_mode_config {
>>  	 */
>>  	struct drm_property *suggested_y_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_writeback.h b/include/drm/drm_writeback.h
>> new file mode 100644
>> index 0000000..6b2ac45
>> --- /dev/null
>> +++ b/include/drm/drm_writeback.h
>> @@ -0,0 +1,78 @@
>> +/*
>> + * (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 <linux/workqueue.h>
>> +
>> +struct drm_writeback_connector {
>> +	struct drm_connector base;
>
>AFAIU, a writeback connector will always require an 'dummy' encoder to
>make the DRM framework happy (AFAIK, a connector is always connected to
>a CRTC through an encoder).
>
>Wouldn't it make more sense to have a drm_encoder object embedded in
>drm_writeback_connector so that people don't have to declare an extra
>structure containing both the drm_writeback_connector connector and a
>drm_encoder? Is there a good reason to keep them separate?
>

Yeah that's not a bad idea. The encoder funcs could be passed in to
drm_writeback_connector_init() (in which case adding a writeback
encoder type would also make sense).

Thanks for the review,

-Brian

>> +
>> +	/**
>> +	 * @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;
>> +};

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

[Brian Starkey]

On Fri, Apr 14, 2017 at 12:11:14PM +0200, Boris Brezillon wrote:
>On Fri, 25 Nov 2016 16:49:00 +0000
>Brian Starkey <brian.starkey@arm.com> wrote:
>
>> Add the 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 *
>
>Don't know what happened, but I still see s32 __user * types in this
>patch (I had to patch it to make in work on top of 4.11-rc1).

Yeah this really confused me too when rebasing. Given that this patch
predates Gustavo's change to s32 I can only assume I typo'd and meant
s64 in this commit message.

-Brian

Re: [PATCH 6/6] drm: mali-dp: Add writeback connector

[Brian Starkey]

On Fri, Apr 14, 2017 at 11:47:00AM +0200, Boris Brezillon wrote:
>On Fri, 25 Nov 2016 16:49:04 +0000
>Brian Starkey <brian.starkey@arm.com> wrote:
>
>> +static int
>> +malidp_mw_encoder_atomic_check(struct drm_encoder *encoder,
>> +			       struct drm_crtc_state *crtc_state,
>> +			       struct drm_connector_state *conn_state)
>> +{
>> +	struct malidp_mw_connector_state *mw_state = to_mw_state(conn_state);
>> +	struct malidp_drm *malidp = encoder->dev->dev_private;
>> +	struct drm_framebuffer *fb;
>> +	int i, n_planes;
>> +
>> +	if (!conn_state->writeback_job || !conn_state->writeback_job->fb)
>> +		return 0;
>> +
>> +	fb = conn_state->writeback_job->fb;
>> +	if ((fb->width != crtc_state->mode.hdisplay) ||
>> +	    (fb->height != crtc_state->mode.vdisplay)) {
>> +		DRM_DEBUG_KMS("Invalid framebuffer size %ux%u\n",
>> +				fb->width, fb->height);
>> +		return -EINVAL;
>> +	}
>
>These checks look pretty generic to me. Shouldn't we have a default
>helper doing that?
>

Yeah makes sense. These should be common to everyone until
cropping/scaling support is added.

>> +
>> +	mw_state->format =
>> +		malidp_hw_get_format_id(&malidp->dev->map, SE_MEMWRITE,
>> +					fb->pixel_format);
>> +	if (mw_state->format == MALIDP_INVALID_FORMAT_ID) {
>
>Same goes here. By adding a format_types table similar to what is
>exposed in drm_plane [1], we could do this check in the core. The only
>thing left to the driver is the 4CC -> driver-specific-id conversion.
>

Yeah could do, but given our driver requires us to run through the
whole table to get the HW ID anyway it seemed like totally wasted
effort to do the same thing in the core.

It's probably a negligible overhead, but it's also unnecessary for
100% of the current writeback implementations ;-)

If a different driver is implemented such that the HW ID lookup isn't
an exhaustive list search then we could add a helper for them to use
which checks the blob.

Cheers,
-Brian

>> +		struct drm_format_name_buf format_name;
>> +
>> +		DRM_DEBUG_KMS("Invalid pixel format %s\n",
>> +			      drm_get_format_name(fb->pixel_format, &format_name));
>> +		return -EINVAL;
>> +	}
>> +
>> +	n_planes = drm_format_num_planes(fb->pixel_format);
>> +	for (i = 0; i < n_planes; i++) {
>> +		struct drm_gem_cma_object *obj = drm_fb_cma_get_gem_obj(fb, i);
>> +		if (!malidp_hw_pitch_valid(malidp->dev, fb->pitches[i])) {
>> +			DRM_DEBUG_KMS("Invalid pitch %u for plane %d\n",
>> +				      fb->pitches[i], i);
>> +			return -EINVAL;
>> +		}
>> +		mw_state->pitches[i] = fb->pitches[i];
>> +		mw_state->addrs[i] = obj->paddr + fb->offsets[i];
>> +	}
>> +	mw_state->n_planes = n_planes;
>> +
>> +	return 0;
>> +}
>
>
>[1]http://lxr.free-electrons.com/source/include/drm/drm_plane.h#L482

Re: [PATCH 1/6] drm: Add writeback connector type

[Boris Brezillon]

Hi Brian,

On Tue, 18 Apr 2017 18:34:43 +0100
Brian Starkey <brian.starkey@arm.com> wrote:

> >> @@ -214,6 +214,19 @@ struct drm_connector_state {
> >>  	struct drm_encoder *best_encoder;
> >>
> >>  	struct drm_atomic_state *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.
> >> +	 *
> >> +	 * See also: drm_writeback_queue_job() and
> >> +	 * drm_writeback_signal_completion()
> >> +	 */
> >> +	struct drm_writeback_job *writeback_job;  
> >
> >Maybe I'm wrong, but is feels weird to have the writeback_job field
> >directly embedded in drm_connector_state, while drm_writeback_connector
> >inherits from drm_connector.
> >
> >IMO, either you decide to directly put the drm_writeback_connector's
> >job_xxx fields in drm_connector and keep the drm_connector_state as is,
> >or you create a drm_writeback_connector_state which inherits from
> >drm_connector_state and embeds the writeback_job field.  
> 
> I did spend a decent amount of time looking at tracking the writeback
> state along with the normal connector state. I couldn't come up with
> anything I liked.
> 
> As the comment mentions, one of the problems is that you have to make
> sure the relevant parts of the connector_state stay around until the
> writeback is finished. That means you've got to block before
> "swap_state()" until the previous writeback is done, and that
> effectively limits your frame rate to refresh/2.
> 
> The Mali-DP HW doesn't have that limitation - we can queue up a new
> commit while the current writeback is ongoing. For that reason I
> didn't want to impose such a limitation in the framework.
> 
> In v1 I allowed that by making the Mali-DP driver hold its own
> references to the relevant bits of the state for as long as it needed
> them. In v3 I moved most of that code back to the core (in part
> because Gustavo didn't like me signalling the DRM-"owned" fence from
> my driver code directly). I think the new approach of "queue_job()"
> and "signal_job()" reduces the amount of tricky code in drivers, and
> is generally more clear (also familiar, when compared to vsync
> events).
> 
> I'm certain there's other ways to do it (refcount atomic states?), but
> it seemed like a biggish overhaul to achieve what would basically be
> the same thing.
> 
> I was expecting each driver supporting writeback to have its own
> different requirements around writeback lifetime/duration. For example
> I think VC4 specifically came up, in that its writeback could take
> several frames, whereas on Mali-DP we either finish within the frame
> or we fail.
> 
> Letting the driver manage its writeback_job lifetime seemed like a
> reasonable way to handle all that, with the documentation stating the
> only behaviour which is guaranteed to work on all drivers:
> 
>    *     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.
> 
> 
> 
> ... so all of that is why the _job fields don't live in a *_state
> structure directly, and instead have to live in the separately-managed
> structure pointed to by ->writeback_job.
> 
> Now, I did look at creating drm_writeback_connector_state, but as it
> would only be holding the job pointer (see above) it didn't seem worth
> scattering around the
> 
>     if (conn_state->connector->connector_type ==
>         DRM_MODE_CONNECTOR_WRITEBACK)
> 
> checks everywhere before up-casting - {clear,reset,duplicate}_state(),
> prepare_signalling(), complete_signalling(), etc. It just touched a
> lot of code for the sake of an extra pointer field in each connector
> state.
> 
> I can easily revisit that part if you like.

I think there's a misunderstanding. I was just suggesting to be
consistent in the inheritance vs 'one object to handle everything'
approach.

I'm perfectly fine with embedding the writeback_job pointer directly in
drm_connector_state, but then it would IMO make more sense to do the
same for the drm_connector object (embed drm_writeback_connector fields
into drm_connector instead of making drm_writeback_connector inherit
from drm_connector).

Anyway, that's just a detail.

> 
> >
> >Anyway, wait for Daniel's feedback before doing this change.
> >  
> 
> Am I expecting some more feedback from Daniel?

No, I was just saying that before doing the changes I was suggesting,
you should wait for Daniel's opinion, because I might be wrong.

> 
> >>  };
> >>
> >>  /**
> >> diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
> >> index bf9991b2..3d3d07f 100644
> >> --- a/include/drm/drm_mode_config.h
> >> +++ b/include/drm/drm_mode_config.h
> >> @@ -634,6 +634,20 @@ struct drm_mode_config {
> >>  	 */
> >>  	struct drm_property *suggested_y_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_writeback.h b/include/drm/drm_writeback.h
> >> new file mode 100644
> >> index 0000000..6b2ac45
> >> --- /dev/null
> >> +++ b/include/drm/drm_writeback.h
> >> @@ -0,0 +1,78 @@
> >> +/*
> >> + * (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 <linux/workqueue.h>
> >> +
> >> +struct drm_writeback_connector {
> >> +	struct drm_connector base;  
> >
> >AFAIU, a writeback connector will always require an 'dummy' encoder to
> >make the DRM framework happy (AFAIK, a connector is always connected to
> >a CRTC through an encoder).
> >
> >Wouldn't it make more sense to have a drm_encoder object embedded in
> >drm_writeback_connector so that people don't have to declare an extra
> >structure containing both the drm_writeback_connector connector and a
> >drm_encoder? Is there a good reason to keep them separate?
> >  
> 
> Yeah that's not a bad idea. The encoder funcs could be passed in to
> drm_writeback_connector_init() (in which case adding a writeback
> encoder type would also make sense).

Well, the more I look at it the more I find it weird to represent this
writeback feature as a connector. To me, it seems to be closer to a
plane object (DRM_PLANE_TYPE_WRITEBACK?) than a real connector.
Actually, the Atmel HLCDC IP use the same register interface to expose
the overlay planes and writeback features.
Of course, representing it as a plane requires patching the core to
allow enabling a CRTC that has no active connectors connected to it if
at least one writeback plane is enabled and connected to the CRTC, but
it should be doable.
By doing that, we would also get rid of these fake connector/encoder
objects as well as the fake modes we are returning in
connector->get_modes().

As far as I can tell, the VC4 and Atmel HLCDC IP should fit pretty well
in this model, not sure about the mali-dp though.

Brian, did you consider this approach, and if you did, can you detail
why you decided to expose this feature as a connector?

Daniel (or anyone else), please step in if you think this is a stupid
idea :-).

Re: [PATCH 1/6] drm: Add writeback connector type

[Brian Starkey]

On Tue, Apr 18, 2017 at 09:57:17PM +0200, Boris Brezillon wrote:
>Hi Brian,
>
>On Tue, 18 Apr 2017 18:34:43 +0100
>Brian Starkey <brian.starkey@arm.com> wrote:
>
>> >> @@ -214,6 +214,19 @@ struct drm_connector_state {
>> >>  	struct drm_encoder *best_encoder;
>> >>
>> >>  	struct drm_atomic_state *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.
>> >> +	 *
>> >> +	 * See also: drm_writeback_queue_job() and
>> >> +	 * drm_writeback_signal_completion()
>> >> +	 */
>> >> +	struct drm_writeback_job *writeback_job;
>> >
>> >Maybe I'm wrong, but is feels weird to have the writeback_job field
>> >directly embedded in drm_connector_state, while drm_writeback_connector
>> >inherits from drm_connector.
>> >
>> >IMO, either you decide to directly put the drm_writeback_connector's
>> >job_xxx fields in drm_connector and keep the drm_connector_state as is,
>> >or you create a drm_writeback_connector_state which inherits from
>> >drm_connector_state and embeds the writeback_job field.
>>
>> I did spend a decent amount of time looking at tracking the writeback
>> state along with the normal connector state. I couldn't come up with
>> anything I liked.
>>
>> As the comment mentions, one of the problems is that you have to make
>> sure the relevant parts of the connector_state stay around until the
>> writeback is finished. That means you've got to block before
>> "swap_state()" until the previous writeback is done, and that
>> effectively limits your frame rate to refresh/2.
>>
>> The Mali-DP HW doesn't have that limitation - we can queue up a new
>> commit while the current writeback is ongoing. For that reason I
>> didn't want to impose such a limitation in the framework.
>>
>> In v1 I allowed that by making the Mali-DP driver hold its own
>> references to the relevant bits of the state for as long as it needed
>> them. In v3 I moved most of that code back to the core (in part
>> because Gustavo didn't like me signalling the DRM-"owned" fence from
>> my driver code directly). I think the new approach of "queue_job()"
>> and "signal_job()" reduces the amount of tricky code in drivers, and
>> is generally more clear (also familiar, when compared to vsync
>> events).
>>
>> I'm certain there's other ways to do it (refcount atomic states?), but
>> it seemed like a biggish overhaul to achieve what would basically be
>> the same thing.
>>
>> I was expecting each driver supporting writeback to have its own
>> different requirements around writeback lifetime/duration. For example
>> I think VC4 specifically came up, in that its writeback could take
>> several frames, whereas on Mali-DP we either finish within the frame
>> or we fail.
>>
>> Letting the driver manage its writeback_job lifetime seemed like a
>> reasonable way to handle all that, with the documentation stating the
>> only behaviour which is guaranteed to work on all drivers:
>>
>>    *     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.
>>
>>
>>
>> ... so all of that is why the _job fields don't live in a *_state
>> structure directly, and instead have to live in the separately-managed
>> structure pointed to by ->writeback_job.
>>
>> Now, I did look at creating drm_writeback_connector_state, but as it
>> would only be holding the job pointer (see above) it didn't seem worth
>> scattering around the
>>
>>     if (conn_state->connector->connector_type ==
>>         DRM_MODE_CONNECTOR_WRITEBACK)
>>
>> checks everywhere before up-casting - {clear,reset,duplicate}_state(),
>> prepare_signalling(), complete_signalling(), etc. It just touched a
>> lot of code for the sake of an extra pointer field in each connector
>> state.
>>
>> I can easily revisit that part if you like.
>
>I think there's a misunderstanding. I was just suggesting to be
>consistent in the inheritance vs 'one object to handle everything'
>approach.

doh.. right yeah I misread. Sorry for the tangent. :-)

>
>I'm perfectly fine with embedding the writeback_job pointer directly in
>drm_connector_state, but then it would IMO make more sense to do the
>same for the drm_connector object (embed drm_writeback_connector fields
>into drm_connector instead of making drm_writeback_connector inherit
>from drm_connector).
>

I agree that it's inconsistent. I guess I did it out of pragmatism -
there's quite a lot of new fields in drm_writeback_connector, and the
code needed to support it was comparatively small. On the other hand
there's only one additional field in drm_connector_state and the code
required to subclass it looked comparatively large.

>Anyway, that's just a detail.
>
>>
>> >
>> >Anyway, wait for Daniel's feedback before doing this change.
>> >
>>
>> Am I expecting some more feedback from Daniel?
>
>No, I was just saying that before doing the changes I was suggesting,
>you should wait for Daniel's opinion, because I might be wrong.
>
>>
>> >>  };
>> >>
>> >>  /**
>> >> diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
>> >> index bf9991b2..3d3d07f 100644
>> >> --- a/include/drm/drm_mode_config.h
>> >> +++ b/include/drm/drm_mode_config.h
>> >> @@ -634,6 +634,20 @@ struct drm_mode_config {
>> >>  	 */
>> >>  	struct drm_property *suggested_y_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_writeback.h b/include/drm/drm_writeback.h
>> >> new file mode 100644
>> >> index 0000000..6b2ac45
>> >> --- /dev/null
>> >> +++ b/include/drm/drm_writeback.h
>> >> @@ -0,0 +1,78 @@
>> >> +/*
>> >> + * (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 <linux/workqueue.h>
>> >> +
>> >> +struct drm_writeback_connector {
>> >> +	struct drm_connector base;
>> >
>> >AFAIU, a writeback connector will always require an 'dummy' encoder to
>> >make the DRM framework happy (AFAIK, a connector is always connected to
>> >a CRTC through an encoder).
>> >
>> >Wouldn't it make more sense to have a drm_encoder object embedded in
>> >drm_writeback_connector so that people don't have to declare an extra
>> >structure containing both the drm_writeback_connector connector and a
>> >drm_encoder? Is there a good reason to keep them separate?
>> >
>>
>> Yeah that's not a bad idea. The encoder funcs could be passed in to
>> drm_writeback_connector_init() (in which case adding a writeback
>> encoder type would also make sense).
>
>Well, the more I look at it the more I find it weird to represent this
>writeback feature as a connector. To me, it seems to be closer to a
>plane object (DRM_PLANE_TYPE_WRITEBACK?) than a real connector.
>Actually, the Atmel HLCDC IP use the same register interface to expose
>the overlay planes and writeback features.
>Of course, representing it as a plane requires patching the core to
>allow enabling a CRTC that has no active connectors connected to it if
>at least one writeback plane is enabled and connected to the CRTC, but
>it should be doable.

Could you expand a bit on how you think planes fit better? It is
something we've previously talked about internally, but so far I'm not
convinced :-)

>By doing that, we would also get rid of these fake connector/encoder
>objects as well as the fake modes we are returning in
>connector->get_modes().

What makes the connector/encoder fake? They represent a real piece of
hardware just the same as a drm_plane would.

I don't mind dropping the mode list and letting userspace just make
up whatever timing it wants - it'd need to do the same if writeback
was a plane - but in some respects giving it a list of modes the same
way we normally do seems nicer for userspace.

>
>As far as I can tell, the VC4 and Atmel HLCDC IP should fit pretty well
>in this model, not sure about the mali-dp though.
>
>Brian, did you consider this approach, and if you did, can you detail
>why you decided to expose this feature as a connector?
>
>Daniel (or anyone else), please step in if you think this is a stupid
>idea :-).

Ville originally suggested using a connector, which Eric followed up
by saying that's what he was thinking of for VC4 writeback too[1].
That was my initial reason for focussing on a connector-based
approach.

I prefer connector over plane conceptually because it keeps with the
established data flow: planes are sources, connectors are sinks.

In some respects the plane _object_ looks like it would fit - it has a
pixel format list and an FB_ID. But everything else would be acting
the exact opposite to a normal plane, and I think there's a bunch of
baked-in assumptions in the kernel and userspace around CRTCs always
having at least one connector.

On the other hand, a writeback connector gains a few extra properties
over a normal connector, but most stuff stays the same. The pipeline
setup looks the same as normal to userspace, you don't need a CRTC to
be active with no connectors, output cloning is the same etc.

Thanks,
-Brian

[1] https://lists.freedesktop.org/archives/dri-devel/2016-July/113329.html

Re: [PATCH 1/6] drm: Add writeback connector type

[Boris Brezillon]

On Wed, 19 Apr 2017 10:51:23 +0100
Brian Starkey <brian.starkey@arm.com> wrote:

> On Tue, Apr 18, 2017 at 09:57:17PM +0200, Boris Brezillon wrote:
> >Hi Brian,
> >
> >On Tue, 18 Apr 2017 18:34:43 +0100
> >Brian Starkey <brian.starkey@arm.com> wrote:
> >  
> >> >> @@ -214,6 +214,19 @@ struct drm_connector_state {
> >> >>  	struct drm_encoder *best_encoder;
> >> >>
> >> >>  	struct drm_atomic_state *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.
> >> >> +	 *
> >> >> +	 * See also: drm_writeback_queue_job() and
> >> >> +	 * drm_writeback_signal_completion()
> >> >> +	 */
> >> >> +	struct drm_writeback_job *writeback_job;  
> >> >
> >> >Maybe I'm wrong, but is feels weird to have the writeback_job field
> >> >directly embedded in drm_connector_state, while drm_writeback_connector
> >> >inherits from drm_connector.
> >> >
> >> >IMO, either you decide to directly put the drm_writeback_connector's
> >> >job_xxx fields in drm_connector and keep the drm_connector_state as is,
> >> >or you create a drm_writeback_connector_state which inherits from
> >> >drm_connector_state and embeds the writeback_job field.  
> >>
> >> I did spend a decent amount of time looking at tracking the writeback
> >> state along with the normal connector state. I couldn't come up with
> >> anything I liked.
> >>
> >> As the comment mentions, one of the problems is that you have to make
> >> sure the relevant parts of the connector_state stay around until the
> >> writeback is finished. That means you've got to block before
> >> "swap_state()" until the previous writeback is done, and that
> >> effectively limits your frame rate to refresh/2.
> >>
> >> The Mali-DP HW doesn't have that limitation - we can queue up a new
> >> commit while the current writeback is ongoing. For that reason I
> >> didn't want to impose such a limitation in the framework.
> >>
> >> In v1 I allowed that by making the Mali-DP driver hold its own
> >> references to the relevant bits of the state for as long as it needed
> >> them. In v3 I moved most of that code back to the core (in part
> >> because Gustavo didn't like me signalling the DRM-"owned" fence from
> >> my driver code directly). I think the new approach of "queue_job()"
> >> and "signal_job()" reduces the amount of tricky code in drivers, and
> >> is generally more clear (also familiar, when compared to vsync
> >> events).
> >>
> >> I'm certain there's other ways to do it (refcount atomic states?), but
> >> it seemed like a biggish overhaul to achieve what would basically be
> >> the same thing.
> >>
> >> I was expecting each driver supporting writeback to have its own
> >> different requirements around writeback lifetime/duration. For example
> >> I think VC4 specifically came up, in that its writeback could take
> >> several frames, whereas on Mali-DP we either finish within the frame
> >> or we fail.
> >>
> >> Letting the driver manage its writeback_job lifetime seemed like a
> >> reasonable way to handle all that, with the documentation stating the
> >> only behaviour which is guaranteed to work on all drivers:
> >>
> >>    *     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.
> >>
> >>
> >>
> >> ... so all of that is why the _job fields don't live in a *_state
> >> structure directly, and instead have to live in the separately-managed
> >> structure pointed to by ->writeback_job.
> >>
> >> Now, I did look at creating drm_writeback_connector_state, but as it
> >> would only be holding the job pointer (see above) it didn't seem worth
> >> scattering around the
> >>
> >>     if (conn_state->connector->connector_type ==
> >>         DRM_MODE_CONNECTOR_WRITEBACK)
> >>
> >> checks everywhere before up-casting - {clear,reset,duplicate}_state(),
> >> prepare_signalling(), complete_signalling(), etc. It just touched a
> >> lot of code for the sake of an extra pointer field in each connector
> >> state.
> >>
> >> I can easily revisit that part if you like.  
> >
> >I think there's a misunderstanding. I was just suggesting to be
> >consistent in the inheritance vs 'one object to handle everything'
> >approach.  
> 
> doh.. right yeah I misread. Sorry for the tangent. :-)
> 
> >
> >I'm perfectly fine with embedding the writeback_job pointer directly in
> >drm_connector_state, but then it would IMO make more sense to do the
> >same for the drm_connector object (embed drm_writeback_connector fields
> >into drm_connector instead of making drm_writeback_connector inherit
> >from drm_connector).
> >  
> 
> I agree that it's inconsistent. I guess I did it out of pragmatism -
> there's quite a lot of new fields in drm_writeback_connector, and the
> code needed to support it was comparatively small. On the other hand
> there's only one additional field in drm_connector_state and the code
> required to subclass it looked comparatively large.
> 
> >Anyway, that's just a detail.
> >  
> >>  
> >> >
> >> >Anyway, wait for Daniel's feedback before doing this change.
> >> >  
> >>
> >> Am I expecting some more feedback from Daniel?  
> >
> >No, I was just saying that before doing the changes I was suggesting,
> >you should wait for Daniel's opinion, because I might be wrong.
> >  
> >>  
> >> >>  };
> >> >>
> >> >>  /**
> >> >> diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
> >> >> index bf9991b2..3d3d07f 100644
> >> >> --- a/include/drm/drm_mode_config.h
> >> >> +++ b/include/drm/drm_mode_config.h
> >> >> @@ -634,6 +634,20 @@ struct drm_mode_config {
> >> >>  	 */
> >> >>  	struct drm_property *suggested_y_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_writeback.h b/include/drm/drm_writeback.h
> >> >> new file mode 100644
> >> >> index 0000000..6b2ac45
> >> >> --- /dev/null
> >> >> +++ b/include/drm/drm_writeback.h
> >> >> @@ -0,0 +1,78 @@
> >> >> +/*
> >> >> + * (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 <linux/workqueue.h>
> >> >> +
> >> >> +struct drm_writeback_connector {
> >> >> +	struct drm_connector base;  
> >> >
> >> >AFAIU, a writeback connector will always require an 'dummy' encoder to
> >> >make the DRM framework happy (AFAIK, a connector is always connected to
> >> >a CRTC through an encoder).
> >> >
> >> >Wouldn't it make more sense to have a drm_encoder object embedded in
> >> >drm_writeback_connector so that people don't have to declare an extra
> >> >structure containing both the drm_writeback_connector connector and a
> >> >drm_encoder? Is there a good reason to keep them separate?
> >> >  
> >>
> >> Yeah that's not a bad idea. The encoder funcs could be passed in to
> >> drm_writeback_connector_init() (in which case adding a writeback
> >> encoder type would also make sense).  
> >
> >Well, the more I look at it the more I find it weird to represent this
> >writeback feature as a connector. To me, it seems to be closer to a
> >plane object (DRM_PLANE_TYPE_WRITEBACK?) than a real connector.
> >Actually, the Atmel HLCDC IP use the same register interface to expose
> >the overlay planes and writeback features.
> >Of course, representing it as a plane requires patching the core to
> >allow enabling a CRTC that has no active connectors connected to it if
> >at least one writeback plane is enabled and connected to the CRTC, but
> >it should be doable.  
> 
> Could you expand a bit on how you think planes fit better?

Just had the impression that the writeback feature was conceptually
closer to a plane object (which is attached buffers and expose ways to
specify which portion of the buffer should be used, provides way to
atomically switch 2 buffers, ...).

> It is
> something we've previously talked about internally, but so far I'm not
> convinced :-)

Okay, as I said, I don't know all the history, hence my questions ;-).

> 
> >By doing that, we would also get rid of these fake connector/encoder
> >objects as well as the fake modes we are returning in
> >connector->get_modes().  
> 
> What makes the connector/encoder fake? They represent a real piece of
> hardware just the same as a drm_plane would.

Well, that's probably subject to interpretation, but I don't consider
these writeback encoders/connectors as real encoders/connectors. They
are definitely real HW blocks, but not what we usually consider as an
encoder/connector.

> 
> I don't mind dropping the mode list and letting userspace just make
> up whatever timing it wants - it'd need to do the same if writeback
> was a plane - but in some respects giving it a list of modes the same
> way we normally do seems nicer for userspace.
> 
> >
> >As far as I can tell, the VC4 and Atmel HLCDC IP should fit pretty well
> >in this model, not sure about the mali-dp though.
> >
> >Brian, did you consider this approach, and if you did, can you detail
> >why you decided to expose this feature as a connector?
> >
> >Daniel (or anyone else), please step in if you think this is a stupid
> >idea :-).  
> 
> Ville originally suggested using a connector, which Eric followed up
> by saying that's what he was thinking of for VC4 writeback too[1].

Thanks for the pointer.

> That was my initial reason for focussing on a connector-based
> approach.
> 
> I prefer connector over plane conceptually because it keeps with the
> established data flow: planes are sources, connectors are sinks.

Okay, it's a valid point.

> 
> In some respects the plane _object_ looks like it would fit - it has a
> pixel format list and an FB_ID. But everything else would be acting
> the exact opposite to a normal plane, and I think there's a bunch of
> baked-in assumptions in the kernel and userspace around CRTCs always
> having at least one connector.

Yep, but writeback connectors are already different enough to not be
considered as regular connectors, so userspace programs will have to
handle them differently anyway (pass a framebuffer and pixel format to
it before adding them to the display pipeline).

Anyway, I see this approach has already been suggested in [1], and you
all agreed that the writeback feature should be exposed as a connector,
so I'll just stop here :-).

Thanks for taking the time to explain the rationale behind this
decision.

> 
> On the other hand, a writeback connector gains a few extra properties
> over a normal connector, but most stuff stays the same. The pipeline
> setup looks the same as normal to userspace, you don't need a CRTC to
> be active with no connectors, output cloning is the same etc.
> 
> Thanks,
> -Brian
> 
> [1] https://lists.freedesktop.org/archives/dri-devel/2016-July/113329.html
> 

Re: [PATCH 1/6] drm: Add writeback connector type

[Brian Starkey]

On Wed, Apr 19, 2017 at 01:34:34PM +0200, Boris Brezillon wrote:
>On Wed, 19 Apr 2017 10:51:23 +0100
>Brian Starkey <brian.starkey@arm.com> wrote:

[snip]

>> Could you expand a bit on how you think planes fit better?
>
>Just had the impression that the writeback feature was conceptually
>closer to a plane object (which is attached buffers and expose ways to
>specify which portion of the buffer should be used, provides way to
>atomically switch 2 buffers, ...).

Yeah sort-of, except that SRC_X/Y/W/H doesn't mean the same for an
"output" plane as an "input" plane (and CRTC_X/Y/W/H similarly,
probably other properties too).

In atomic land, the swapping of buffers is really just the swapping of
object IDs via properties - I don't think planes actually have
anything special in terms of buffer handling, except for all the
legacy state handling cruft.

>
>> It is
>> something we've previously talked about internally, but so far I'm not
>> convinced :-)
>
>Okay, as I said, I don't know all the history, hence my questions ;-).
>

I think that history was here in our office rather than on the list
anyway.

>>
>> >By doing that, we would also get rid of these fake connector/encoder
>> >objects as well as the fake modes we are returning in
>> >connector->get_modes().
>>
>> What makes the connector/encoder fake? They represent a real piece of
>> hardware just the same as a drm_plane would.
>
>Well, that's probably subject to interpretation, but I don't consider
>these writeback encoders/connectors as real encoders/connectors. They
>are definitely real HW blocks, but not what we usually consider as an
>encoder/connector.
>

This is true

>>
>> I don't mind dropping the mode list and letting userspace just make
>> up whatever timing it wants - it'd need to do the same if writeback
>> was a plane - but in some respects giving it a list of modes the same
>> way we normally do seems nicer for userspace.
>>
>> >
>> >As far as I can tell, the VC4 and Atmel HLCDC IP should fit pretty well
>> >in this model, not sure about the mali-dp though.
>> >
>> >Brian, did you consider this approach, and if you did, can you detail
>> >why you decided to expose this feature as a connector?
>> >
>> >Daniel (or anyone else), please step in if you think this is a stupid
>> >idea :-).
>>
>> Ville originally suggested using a connector, which Eric followed up
>> by saying that's what he was thinking of for VC4 writeback too[1].
>
>Thanks for the pointer.
>
>> That was my initial reason for focussing on a connector-based
>> approach.
>>
>> I prefer connector over plane conceptually because it keeps with the
>> established data flow: planes are sources, connectors are sinks.
>
>Okay, it's a valid point.
>
>>
>> In some respects the plane _object_ looks like it would fit - it has a
>> pixel format list and an FB_ID. But everything else would be acting
>> the exact opposite to a normal plane, and I think there's a bunch of
>> baked-in assumptions in the kernel and userspace around CRTCs always
>> having at least one connector.
>
>Yep, but writeback connectors are already different enough to not be
>considered as regular connectors, so userspace programs will have to
>handle them differently anyway (pass a framebuffer and pixel format to
>it before adding them to the display pipeline).
>
>Anyway, I see this approach has already been suggested in [1], and you
>all agreed that the writeback feature should be exposed as a connector,
>so I'll just stop here :-).
>
>Thanks for taking the time to explain the rationale behind this
>decision.
>

No problem, now is the right time to be discussing the decision before
we merge something wrong.

Are you happy enough with the connector approach then? Any concerns
with going ahead with it?

Cheers,
-Brian

Re: [PATCH 1/6] drm: Add writeback connector type

[Boris Brezillon]

On Fri, 25 Nov 2016 16:48:59 +0000
Brian Starkey <brian.starkey@arm.com> wrote:

> +/**
> + * drm_writeback_connector_init - Initialize a writeback connector and its properties
> + * @dev: DRM device
> + * @wb_connector: Writeback connector to initialize
> + * @funcs: Connector funcs vtable
> + * @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.
> + *
> + * 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 *funcs,
> +				 u32 *formats, int n_formats)

This should probably be 'const u32 *formats', since developers are
likely to define a this array with a 'static const' specifier in their
driver.

Re: [PATCH 1/6] drm: Add writeback connector type

[Liviu Dudau]

On Fri, May 05, 2017 at 10:22:19AM +0200, Boris Brezillon wrote:
> On Fri, 25 Nov 2016 16:48:59 +0000
> Brian Starkey <brian.starkey@arm.com> wrote:
> 
> > +/**
> > + * drm_writeback_connector_init - Initialize a writeback connector and its properties
> > + * @dev: DRM device
> > + * @wb_connector: Writeback connector to initialize
> > + * @funcs: Connector funcs vtable
> > + * @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.
> > + *
> > + * 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 *funcs,
> > +				 u32 *formats, int n_formats)
> 
> This should probably be 'const u32 *formats', since developers are
> likely to define a this array with a 'static const' specifier in their
> driver.

Fixed in the v4.

Thanks,
Liviu

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