[PATCH v5 0/4] drm: add explicit fencing

[PATCH v5 0/4] drm: add explicit fencing

[Gustavo Padovan]

From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>

Hi,

Currently the Linux Kernel only have an implicit fencing mechanism, through 
reservation objects, in which the fences are attached directly to buffers
operations and userspace is unaware of what is happening. On the other hand
explicit fencing exposes fences to the userspace to handle fencing between
producer/consumer explicitely.

To support explicit fencing in the mainline kernel we de-staged the the needed
parts of the Android Sync Framework[1] to be able to send and received fences
to/from userspace. It has the concept of sync_file that exposes the driver's
fences to userspace as file descriptors.

With explicit fencing we have a global mechanism that optimizes the flow of
buffers between consumers and producers, avoiding a lot of waiting and some
potential desktop freeze. So instead of waiting for a buffer to be processed by
the GPU before sending it to DRM in an Atomic IOCTL we can get a sync_file fd
from the GPU driver at the moment we submit the buffer processing. The
compositor then passes these fds to DRM in an Atomic IOCTL, that will
not be displayed until the fences signal, i.e, the GPU finished processing the
buffer and it is ready to display. In DRM the fences we wait on before
displaying a buffer are called in-fences and they are a per-plane deal.

Vice-versa, we have out-fences to sychronize the return of buffers to GPU
(producer) to be processed again. When DRM receives an atomic request with a
the OUT_FENCE_PTR property it create a fence attach it to a per-crtc
sync_file. It then returns the sync_file fds to userspace. With the fences
available userspace can forward these fences to the GPU, where it will wait the
fence to signal before starting to process on buffer again. Out-fences are
per-crtc.

While these are out-fences in DRM (the consumer) they become in-fences once
they get to the GPU (the producer).

DRM explicit fences are opt-in, as the default will still be implicit fencing.

In-fences
---------

In the first discussions on #dri-devel on IRC we decided to hide the Sync
Framework from DRM drivers to reduce complexity, so as soon we get the fd
via IN_FENCE_FD plane property we convert the sync_file fd to a struct fence.
However a sync_file might contain more than one fence, so we created the
fence_array concept. struct fence_array is a subclass of struct
fence and stores a group of fences that needs to be waited together.

Then we just use the already in place fence waiting support to wait on those
fences. Once the producer calls fence_signal() for all fences on wait we can
proceed with the atomic commit and display the framebuffers.

Out-fences
----------

Passing a pointer to OUT_FENCE_PTR property in an atomic request enables
out-fences. The kernel then creates a fence, attach it to a sync_file and
install this file on a unused fd for each crtc. The kernel writes the fd in
the memory pointed by the out_fence_ptr provided. In case of error it writes -1.

DRM core use the already in place drm_event infrastructure to help signal
fences, we've added a fence pointer to struct drm_pending_event. We signal
signal fences when all the buffer related to that CRTC are *on* the screen.

Kernel tree
-----------

For those who want all patches on this RFC are in my tree:

https://git.kernel.org/cgit/linux/kernel/git/padovan/linux.git/log/?h=fences


v5 changes
----------

The changes from  v5 to v4 are in the patches description.


Userspace
---------

Fences support on drm_hwcomposer is currently a working in progress.

Regards,

        Gustavo

[1] https://source.android.com/devices/graphics/implement.html#vsync

---
Gustavo Padovan (4):
  drm/fence: add in-fences support
  drm/fence: release fence reference when canceling event
  drm/fence: add fence timeline to drm_crtc
  drm/fence: add out-fences support

 drivers/gpu/drm/Kconfig             |   1 +
 drivers/gpu/drm/drm_atomic.c        | 138 +++++++++++++++++++++++++++++-------
 drivers/gpu/drm/drm_atomic_helper.c |  13 ++--
 drivers/gpu/drm/drm_crtc.c          |  45 ++++++++++++
 drivers/gpu/drm/drm_fops.c          |   4 ++
 drivers/gpu/drm/drm_plane.c         |   1 +
 include/drm/drm_crtc.h              |  43 +++++++++++
 include/drm/drm_plane.h             |   4 +-
 8 files changed, 214 insertions(+), 35 deletions(-)

-- 
2.5.5

[PATCH v5 1/4] drm/fence: add in-fences support

[Gustavo Padovan]

From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>

There is now a new property called FENCE_FD attached to every plane
state that receives the sync_file fd from userspace via the atomic commit
IOCTL.

The fd is then translated to a fence (that may be a fence_collection
subclass or just a normal fence) and then used by DRM to fence_wait() for
all fences in the sync_file to signal. So it only commits when all
framebuffers are ready to scanout.

v2: Comments by Daniel Vetter:
	- remove set state->fence = NULL in destroy phase
	- accept fence -1 as valid and just return 0
	- do not call fence_get() - sync_file_fences_get() already calls it
	- fence_put() if state->fence is already set, in case userspace
	set the property more than once.

v3: WARN_ON if fence is set but state has no FB

v4: Comment from Maarten Lankhorst
	- allow set fence with no related fb

v5: rename FENCE_FD to IN_FENCE_FD

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
---
 drivers/gpu/drm/Kconfig             |  1 +
 drivers/gpu/drm/drm_atomic.c        | 14 ++++++++++++++
 drivers/gpu/drm/drm_atomic_helper.c | 13 +++++++------
 drivers/gpu/drm/drm_crtc.c          |  6 ++++++
 drivers/gpu/drm/drm_plane.c         |  1 +
 include/drm/drm_crtc.h              |  5 +++++
 include/drm/drm_plane.h             |  4 ++--
 7 files changed, 36 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index 483059a..43cb33d 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -12,6 +12,7 @@ menuconfig DRM
 	select I2C
 	select I2C_ALGOBIT
 	select DMA_SHARED_BUFFER
+	select SYNC_FILE
 	help
 	  Kernel-level support for the Direct Rendering Infrastructure (DRI)
 	  introduced in XFree86 4.0. If you say Y here, you need to select
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 5dd7054..b3284b2 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -30,6 +30,7 @@
 #include <drm/drm_atomic.h>
 #include <drm/drm_mode.h>
 #include <drm/drm_plane_helper.h>
+#include <linux/sync_file.h>
 
 #include "drm_crtc_internal.h"
 
@@ -686,6 +687,17 @@ int drm_atomic_plane_set_property(struct drm_plane *plane,
 		drm_atomic_set_fb_for_plane(state, fb);
 		if (fb)
 			drm_framebuffer_unreference(fb);
+	} else if (property == config->prop_in_fence_fd) {
+		if (U642I64(val) == -1)
+			return 0;
+
+		if (state->in_fence)
+			fence_put(state->in_fence);
+
+		state->in_fence = sync_file_get_fence(val);
+		if (!state->in_fence)
+			return -EINVAL;
+
 	} else if (property == config->prop_crtc_id) {
 		struct drm_crtc *crtc = drm_crtc_find(dev, val);
 		return drm_atomic_set_crtc_for_plane(state, crtc);
@@ -745,6 +757,8 @@ drm_atomic_plane_get_property(struct drm_plane *plane,
 
 	if (property == config->prop_fb_id) {
 		*val = (state->fb) ? state->fb->base.id : 0;
+	} else if (property == config->prop_in_fence_fd) {
+		*val = -1;
 	} else if (property == config->prop_crtc_id) {
 		*val = (state->crtc) ? state->crtc->base.id : 0;
 	} else if (property == config->prop_crtc_x) {
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 07b432f..73464b1 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -1031,22 +1031,20 @@ int drm_atomic_helper_wait_for_fences(struct drm_device *dev,
 		if (!pre_swap)
 			plane_state = plane->state;
 
-		if (!plane_state->fence)
+		if (!plane_state->in_fence)
 			continue;
 
-		WARN_ON(!plane_state->fb);
-
 		/*
 		 * If waiting for fences pre-swap (ie: nonblock), userspace can
 		 * still interrupt the operation. Instead of blocking until the
 		 * timer expires, make the wait interruptible.
 		 */
-		ret = fence_wait(plane_state->fence, pre_swap);
+		ret = fence_wait(plane_state->in_fence, pre_swap);
 		if (ret)
 			return ret;
 
-		fence_put(plane_state->fence);
-		plane_state->fence = NULL;
+		fence_put(plane_state->in_fence);
+		plane_state->in_fence = NULL;
 	}
 
 	return 0;
@@ -3114,6 +3112,9 @@ void __drm_atomic_helper_plane_destroy_state(struct drm_plane_state *state)
 {
 	if (state->fb)
 		drm_framebuffer_unreference(state->fb);
+
+	if (state->in_fence)
+		fence_put(state->in_fence);
 }
 EXPORT_SYMBOL(__drm_atomic_helper_plane_destroy_state);
 
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 60403bf..fcb6453 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -397,6 +397,12 @@ static int drm_mode_create_standard_properties(struct drm_device *dev)
 		return -ENOMEM;
 	dev->mode_config.prop_fb_id = prop;
 
+	prop = drm_property_create_signed_range(dev, DRM_MODE_PROP_ATOMIC,
+			"IN_FENCE_FD", -1, INT_MAX);
+	if (!prop)
+		return -ENOMEM;
+	dev->mode_config.prop_in_fence_fd = prop;
+
 	prop = drm_property_create_object(dev, DRM_MODE_PROP_ATOMIC,
 			"CRTC_ID", DRM_MODE_OBJECT_CRTC);
 	if (!prop)
diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
index 249c0ae..3957ef8 100644
--- a/drivers/gpu/drm/drm_plane.c
+++ b/drivers/gpu/drm/drm_plane.c
@@ -137,6 +137,7 @@ int drm_universal_plane_init(struct drm_device *dev, struct drm_plane *plane,
 
 	if (drm_core_check_feature(dev, DRIVER_ATOMIC)) {
 		drm_object_attach_property(&plane->base, config->prop_fb_id, 0);
+		drm_object_attach_property(&plane->base, config->prop_in_fence_fd, -1);
 		drm_object_attach_property(&plane->base, config->prop_crtc_id, 0);
 		drm_object_attach_property(&plane->base, config->prop_crtc_x, 0);
 		drm_object_attach_property(&plane->base, config->prop_crtc_y, 0);
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 284c1b3..279132e 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -1206,6 +1206,11 @@ struct drm_mode_config {
 	 */
 	struct drm_property *prop_fb_id;
 	/**
+	 * @prop_in_fence_fd: Sync File fd representing the incoming fences
+	 * for a Plane.
+	 */
+	struct drm_property *prop_in_fence_fd;
+	/**
 	 * @prop_crtc_id: Default atomic plane property to specify the
 	 * &drm_crtc.
 	 */
diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
index 0235390..3cb9bf1 100644
--- a/include/drm/drm_plane.h
+++ b/include/drm/drm_plane.h
@@ -34,7 +34,7 @@ struct drm_crtc;
  * @plane: backpointer to the plane
  * @crtc: currently bound CRTC, NULL if disabled
  * @fb: currently bound framebuffer
- * @fence: optional fence to wait for before scanning out @fb
+ * @in_fence: optional fence to wait for before scanning out @fb
  * @crtc_x: left position of visible portion of plane on crtc
  * @crtc_y: upper position of visible portion of plane on crtc
  * @crtc_w: width of visible portion of plane on crtc
@@ -59,7 +59,7 @@ struct drm_plane_state {
 
 	struct drm_crtc *crtc;   /* do not write directly, use drm_atomic_set_crtc_for_plane() */
 	struct drm_framebuffer *fb;  /* do not write directly, use drm_atomic_set_fb_for_plane() */
-	struct fence *fence;
+	struct fence *in_fence;
 
 	/* Signed dest location allows it to be partially off screen */
 	int32_t crtc_x, crtc_y;
-- 
2.5.5

[PATCH v5 2/4] drm/fence: release fence reference when canceling event

[Gustavo Padovan]

From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>

If the event gets canceled we also need to put away the fence
reference it holds.

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
---
 drivers/gpu/drm/drm_fops.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c
index e84faec..8bed5f4 100644
--- a/drivers/gpu/drm/drm_fops.c
+++ b/drivers/gpu/drm/drm_fops.c
@@ -663,6 +663,10 @@ void drm_event_cancel_free(struct drm_device *dev,
 		list_del(&p->pending_link);
 	}
 	spin_unlock_irqrestore(&dev->event_lock, flags);
+
+	if (p->fence)
+		fence_put(p->fence);
+
 	kfree(p);
 }
 EXPORT_SYMBOL(drm_event_cancel_free);
-- 
2.5.5

[PATCH v5 3/4] drm/fence: add fence timeline to drm_crtc

[Gustavo Padovan]

From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>

Create one timeline context for each CRTC to be able to handle out-fences
and signal them. It adds a few members to struct drm_crtc: fence_context,
where we store the context we get from fence_context_alloc(), the
fence seqno and the fence lock, that we pass in fence_init() to be
used by the fence.

v2: Comment by Daniel Stone:
	- add BUG_ON() to fence_to_crtc() macro

v3: Comment by Ville Syrjälä
	- Use more meaningful name as crtc timeline name

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
---
 drivers/gpu/drm/drm_crtc.c | 31 +++++++++++++++++++++++++++++++
 include/drm/drm_crtc.h     | 19 +++++++++++++++++++
 2 files changed, 50 insertions(+)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index fcb6453..b99090f 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -165,6 +165,32 @@ static void drm_crtc_crc_fini(struct drm_crtc *crtc)
 #endif
 }
 
+static const char *drm_crtc_fence_get_driver_name(struct fence *fence)
+{
+	struct drm_crtc *crtc = fence_to_crtc(fence);
+
+	return crtc->dev->driver->name;
+}
+
+static const char *drm_crtc_fence_get_timeline_name(struct fence *fence)
+{
+	struct drm_crtc *crtc = fence_to_crtc(fence);
+
+	return crtc->timeline_name;
+}
+
+static bool drm_crtc_fence_enable_signaling(struct fence *fence)
+{
+	return true;
+}
+
+const struct fence_ops drm_crtc_fence_ops = {
+	.get_driver_name = drm_crtc_fence_get_driver_name,
+	.get_timeline_name = drm_crtc_fence_get_timeline_name,
+	.enable_signaling = drm_crtc_fence_enable_signaling,
+	.wait = fence_default_wait,
+};
+
 /**
  * drm_crtc_init_with_planes - Initialise a new CRTC object with
  *    specified primary and cursor planes.
@@ -222,6 +248,11 @@ int drm_crtc_init_with_planes(struct drm_device *dev, struct drm_crtc *crtc,
 		return -ENOMEM;
 	}
 
+	crtc->fence_context = fence_context_alloc(1);
+	spin_lock_init(&crtc->fence_lock);
+	snprintf(crtc->timeline_name, sizeof(crtc->timeline_name),
+		 "drm_crtc-%d", crtc->base.id);
+
 	crtc->base.properties = &crtc->properties;
 
 	list_add_tail(&crtc->head, &config->crtc_list);
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 279132e..657a33a 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -32,6 +32,8 @@
 #include <linux/fb.h>
 #include <linux/hdmi.h>
 #include <linux/media-bus-format.h>
+#include <linux/srcu.h>
+#include <linux/fence.h>
 #include <uapi/drm/drm_mode.h>
 #include <uapi/drm/drm_fourcc.h>
 #include <drm/drm_modeset_lock.h>
@@ -618,6 +620,9 @@ struct drm_crtc_funcs {
  * @gamma_store: gamma ramp values
  * @helper_private: mid-layer private data
  * @properties: property tracking for this CRTC
+ * @fence_context: context for fence signalling
+ * @fence_lock: fence lock for the fence context
+ * @fence_seqno: seqno variable to create fences
  *
  * Each CRTC may have one or more connectors associated with it.  This structure
  * allows the CRTC to be controlled.
@@ -726,8 +731,22 @@ struct drm_crtc {
 	 */
 	struct drm_crtc_crc crc;
 #endif
+
+	/* fence timelines info for DRM out-fences */
+	unsigned int fence_context;
+	spinlock_t fence_lock;
+	unsigned long fence_seqno;
+	char timeline_name[32];
 };
 
+extern const struct fence_ops drm_crtc_fence_ops;
+
+static inline struct drm_crtc *fence_to_crtc(struct fence *fence)
+{
+	BUG_ON(fence->ops != &drm_crtc_fence_ops);
+	return container_of(fence->lock, struct drm_crtc, fence_lock);
+}
+
 /**
  * struct drm_mode_set - new values for a CRTC config change
  * @fb: framebuffer to use for new config
-- 
2.5.5

[PATCH v5 4/4] drm/fence: add out-fences support

[Gustavo Padovan]

From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>

Support DRM out-fences by creating a sync_file with a fence for each CRTC
that sets the OUT_FENCE_PTR property.

We use the out_fence pointer received in the OUT_FENCE_PTR prop to send
the sync_file fd back to userspace.

The sync_file and fd are allocated/created before commit, but the
fd_install operation only happens after we know that commit succeed.

In case of error userspace should receive a fd number of -1.

v2: Comment by Rob Clark:
	- Squash commit that adds DRM_MODE_ATOMIC_OUT_FENCE flag here.

    Comment by Daniel Vetter:
	- Add clean up code for out_fences

v3: Comments by Daniel Vetter:
	- create DRM_MODE_ATOMIC_EVENT_MASK
	- userspace should fill out_fences_ptr with the crtc_ids for which
	it wants fences back.

v4: Create OUT_FENCE_PTR properties and remove old approach.

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
---
 drivers/gpu/drm/drm_atomic.c | 116 ++++++++++++++++++++++++++++++++++---------
 drivers/gpu/drm/drm_crtc.c   |   8 +++
 include/drm/drm_crtc.h       |  19 +++++++
 3 files changed, 119 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index b3284b2..07775fc 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -490,6 +490,8 @@ int drm_atomic_crtc_set_property(struct drm_crtc *crtc,
 					&replaced);
 		state->color_mgmt_changed |= replaced;
 		return ret;
+	} else if (property == config->prop_out_fence_ptr) {
+		state->out_fence_ptr = u64_to_user_ptr(val);
 	} else if (crtc->funcs->atomic_set_property)
 		return crtc->funcs->atomic_set_property(crtc, state, property, val);
 	else
@@ -532,6 +534,8 @@ drm_atomic_crtc_get_property(struct drm_crtc *crtc,
 		*val = (state->ctm) ? state->ctm->base.id : 0;
 	else if (property == config->gamma_lut_property)
 		*val = (state->gamma_lut) ? state->gamma_lut->base.id : 0;
+	else if (property == config->prop_out_fence_ptr)
+		*val = 0;
 	else if (crtc->funcs->atomic_get_property)
 		return crtc->funcs->atomic_get_property(crtc, state, property, val);
 	else
@@ -1474,11 +1478,9 @@ EXPORT_SYMBOL(drm_atomic_nonblocking_commit);
  */
 
 static struct drm_pending_vblank_event *create_vblank_event(
-		struct drm_device *dev, struct drm_file *file_priv,
-		struct fence *fence, uint64_t user_data)
+		struct drm_device *dev, uint64_t user_data)
 {
 	struct drm_pending_vblank_event *e = NULL;
-	int ret;
 
 	e = kzalloc(sizeof *e, GFP_KERNEL);
 	if (!e)
@@ -1488,17 +1490,6 @@ static struct drm_pending_vblank_event *create_vblank_event(
 	e->event.base.length = sizeof(e->event);
 	e->event.user_data = user_data;
 
-	if (file_priv) {
-		ret = drm_event_reserve_init(dev, file_priv, &e->base,
-					     &e->event.base);
-		if (ret) {
-			kfree(e);
-			return NULL;
-		}
-	}
-
-	e->base.fence = fence;
-
 	return e;
 }
 
@@ -1603,6 +1594,40 @@ void drm_atomic_clean_old_fb(struct drm_device *dev,
 }
 EXPORT_SYMBOL(drm_atomic_clean_old_fb);
 
+static int crtc_setup_out_fence(struct drm_crtc *crtc,
+				struct drm_crtc_state *crtc_state,
+				struct drm_out_fence_state *fence_state)
+{
+	struct fence *fence;
+
+	fence_state->fd = get_unused_fd_flags(O_CLOEXEC);
+	if (fence_state->fd < 0) {
+		return fence_state->fd;
+	}
+
+	fence_state->out_fence_ptr = crtc_state->out_fence_ptr;
+	crtc_state->out_fence_ptr = NULL;
+
+	if (put_user(fence_state->fd, fence_state->out_fence_ptr))
+		return -EFAULT;
+
+	fence = kzalloc(sizeof(*fence), GFP_KERNEL);
+	if (!fence)
+		return -ENOMEM;
+
+	fence_init(fence, &drm_crtc_fence_ops, &crtc->fence_lock,
+		   crtc->fence_context, ++crtc->fence_seqno);
+
+	fence_state->sync_file = sync_file_create(fence);
+	if(!fence_state->sync_file) {
+		fence_put(fence);
+		return -ENOMEM;
+	}
+
+	crtc_state->event->base.fence = fence_get(fence);
+	return 0;
+}
+
 int drm_mode_atomic_ioctl(struct drm_device *dev,
 			  void *data, struct drm_file *file_priv)
 {
@@ -1617,9 +1642,10 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
 	struct drm_plane *plane;
 	struct drm_crtc *crtc;
 	struct drm_crtc_state *crtc_state;
+	struct drm_out_fence_state *fence_state;
 	unsigned plane_mask;
 	int ret = 0;
-	unsigned int i, j;
+	unsigned int i, j, fence_idx = 0;
 
 	/* disallow for drivers not supporting atomic: */
 	if (!drm_core_check_feature(dev, DRIVER_ATOMIC))
@@ -1734,12 +1760,19 @@ retry:
 		drm_mode_object_unreference(obj);
 	}
 
-	if (arg->flags & DRM_MODE_PAGE_FLIP_EVENT) {
-		for_each_crtc_in_state(state, crtc, crtc_state, i) {
+	fence_state = kcalloc(dev->mode_config.num_crtc, sizeof(*fence_state),
+			      GFP_KERNEL);
+	if (!fence_state) {
+		ret = -ENOMEM;
+		goto out;
+	}
+
+	for_each_crtc_in_state(state, crtc, crtc_state, i) {
+		if (arg->flags & DRM_MODE_PAGE_FLIP_EVENT ||
+		    crtc_state->out_fence_ptr) {
 			struct drm_pending_vblank_event *e;
 
-			e = create_vblank_event(dev, file_priv, NULL,
-						arg->user_data);
+			e = create_vblank_event(dev, arg->user_data);
 			if (!e) {
 				ret = -ENOMEM;
 				goto out;
@@ -1747,6 +1780,28 @@ retry:
 
 			crtc_state->event = e;
 		}
+
+		if (arg->flags & DRM_MODE_PAGE_FLIP_EVENT) {
+			struct drm_pending_vblank_event *e = crtc_state->event;
+
+			if (!file_priv)
+				continue;
+
+			ret = drm_event_reserve_init(dev, file_priv, &e->base,
+						     &e->event.base);
+			if (ret) {
+				kfree(e);
+				crtc_state->event = NULL;
+				goto out;
+			}
+		}
+
+		if (crtc_state->out_fence_ptr) {
+			ret = crtc_setup_out_fence(crtc, crtc_state,
+						   &fence_state[fence_idx++]);
+			if (ret)
+				goto out;
+		}
 	}
 
 	if (arg->flags & DRM_MODE_ATOMIC_TEST_ONLY) {
@@ -1761,24 +1816,37 @@ retry:
 		ret = drm_atomic_commit(state);
 	}
 
+	for (i = 0; i < fence_idx; i++)
+		fd_install(fence_state[i].fd, fence_state[i].sync_file->file);
+
 out:
 	drm_atomic_clean_old_fb(dev, plane_mask, ret);
 
-	if (ret && arg->flags & DRM_MODE_PAGE_FLIP_EVENT) {
+	for_each_crtc_in_state(state, crtc, crtc_state, i) {
 		/*
 		 * TEST_ONLY and PAGE_FLIP_EVENT are mutually exclusive,
 		 * if they weren't, this code should be called on success
 		 * for TEST_ONLY too.
 		 */
+		if (ret && crtc_state->event)
+			drm_event_cancel_free(dev, &crtc_state->event->base);
+	}
 
-		for_each_crtc_in_state(state, crtc, crtc_state, i) {
-			if (!crtc_state->event)
-				continue;
+	if (ret && fence_state) {
+		for (i = 0; i < fence_idx; i++) {
+			if (fence_state[i].fd >= 0)
+				put_unused_fd(fence_state[i].fd);
+			if (fence_state[i].sync_file)
+				fput(fence_state[i].sync_file->file);
 
-			drm_event_cancel_free(dev, &crtc_state->event->base);
+			/* If this fails, there's not much we can do about it */
+			if (put_user(-1, fence_state->out_fence_ptr))
+				DRM_ERROR("Couldn't clear out_fence_ptr\n");
 		}
 	}
 
+	kfree(fence_state);
+
 	if (ret == -EDEADLK) {
 		drm_atomic_state_clear(state);
 		drm_modeset_backoff(&ctx);
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index b99090f..40bce97 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -274,6 +274,8 @@ int drm_crtc_init_with_planes(struct drm_device *dev, struct drm_crtc *crtc,
 	if (drm_core_check_feature(dev, DRIVER_ATOMIC)) {
 		drm_object_attach_property(&crtc->base, config->prop_active, 0);
 		drm_object_attach_property(&crtc->base, config->prop_mode_id, 0);
+		drm_object_attach_property(&crtc->base,
+					   config->prop_out_fence_ptr, 0);
 	}
 
 	return 0;
@@ -434,6 +436,12 @@ static int drm_mode_create_standard_properties(struct drm_device *dev)
 		return -ENOMEM;
 	dev->mode_config.prop_in_fence_fd = prop;
 
+	prop = drm_property_create_range(dev, DRM_MODE_PROP_ATOMIC,
+			"OUT_FENCE_PTR", 0, U64_MAX);
+	if (!prop)
+		return -ENOMEM;
+	dev->mode_config.prop_out_fence_ptr = prop;
+
 	prop = drm_property_create_object(dev, DRM_MODE_PROP_ATOMIC,
 			"CRTC_ID", DRM_MODE_OBJECT_CRTC);
 	if (!prop)
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 657a33a..b898604 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -165,6 +165,8 @@ struct drm_crtc_state {
 	struct drm_property_blob *ctm;
 	struct drm_property_blob *gamma_lut;
 
+	u64 __user *out_fence_ptr;
+
 	/**
 	 * @event:
 	 *
@@ -748,6 +750,18 @@ static inline struct drm_crtc *fence_to_crtc(struct fence *fence)
 }
 
 /**
+ * struct drm_out_fence_state - store out_fence data for install and clean up
+ * @out_fence_ptr: user pointer to store the fence fd in.
+ * @sync_file: sync_file related to the out_fence
+ * @fd: file descriptor to installed on the sync_file.
+ */
+struct drm_out_fence_state {
+	u64 __user *out_fence_ptr;
+	struct sync_file *sync_file;
+	int fd;
+};
+
+/*
  * struct drm_mode_set - new values for a CRTC config change
  * @fb: framebuffer to use for new config
  * @crtc: CRTC whose configuration we're about to change
@@ -1230,6 +1244,11 @@ struct drm_mode_config {
 	 */
 	struct drm_property *prop_in_fence_fd;
 	/**
+	 * @prop_out_fence_ptr: Sync File fd pointer representing the
+	 * outgoing fences for a CRTC.
+	 */
+	struct drm_property *prop_out_fence_ptr;
+	/**
 	 * @prop_crtc_id: Default atomic plane property to specify the
 	 * &drm_crtc.
 	 */
-- 
2.5.5

Re: [PATCH v5 4/4] drm/fence: add out-fences support

[Ville Syrjälä]

On Thu, Oct 20, 2016 at 12:50:05PM -0200, Gustavo Padovan wrote:
> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> 
> Support DRM out-fences by creating a sync_file with a fence for each CRTC
> that sets the OUT_FENCE_PTR property.

I still maintain the out fence should also be per fb (well, per plane
since we can't add it to fbs). Otherwise it's not going to be at all
useful if you do fps>vrefresh and don't include the same set of planes
in every atomic ioctl, eg. if you only ever include ones that are
somehow dirty.

> 
> We use the out_fence pointer received in the OUT_FENCE_PTR prop to send
> the sync_file fd back to userspace.
> 
> The sync_file and fd are allocated/created before commit, but the
> fd_install operation only happens after we know that commit succeed.
> 
> In case of error userspace should receive a fd number of -1.
> 
> v2: Comment by Rob Clark:
> 	- Squash commit that adds DRM_MODE_ATOMIC_OUT_FENCE flag here.
> 
>     Comment by Daniel Vetter:
> 	- Add clean up code for out_fences
> 
> v3: Comments by Daniel Vetter:
> 	- create DRM_MODE_ATOMIC_EVENT_MASK
> 	- userspace should fill out_fences_ptr with the crtc_ids for which
> 	it wants fences back.
> 
> v4: Create OUT_FENCE_PTR properties and remove old approach.
> 
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> ---
>  drivers/gpu/drm/drm_atomic.c | 116 ++++++++++++++++++++++++++++++++++---------
>  drivers/gpu/drm/drm_crtc.c   |   8 +++
>  include/drm/drm_crtc.h       |  19 +++++++
>  3 files changed, 119 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index b3284b2..07775fc 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -490,6 +490,8 @@ int drm_atomic_crtc_set_property(struct drm_crtc *crtc,
>  					&replaced);
>  		state->color_mgmt_changed |= replaced;
>  		return ret;
> +	} else if (property == config->prop_out_fence_ptr) {
> +		state->out_fence_ptr = u64_to_user_ptr(val);
>  	} else if (crtc->funcs->atomic_set_property)
>  		return crtc->funcs->atomic_set_property(crtc, state, property, val);
>  	else
> @@ -532,6 +534,8 @@ drm_atomic_crtc_get_property(struct drm_crtc *crtc,
>  		*val = (state->ctm) ? state->ctm->base.id : 0;
>  	else if (property == config->gamma_lut_property)
>  		*val = (state->gamma_lut) ? state->gamma_lut->base.id : 0;
> +	else if (property == config->prop_out_fence_ptr)
> +		*val = 0;
>  	else if (crtc->funcs->atomic_get_property)
>  		return crtc->funcs->atomic_get_property(crtc, state, property, val);
>  	else
> @@ -1474,11 +1478,9 @@ EXPORT_SYMBOL(drm_atomic_nonblocking_commit);
>   */
>  
>  static struct drm_pending_vblank_event *create_vblank_event(
> -		struct drm_device *dev, struct drm_file *file_priv,
> -		struct fence *fence, uint64_t user_data)
> +		struct drm_device *dev, uint64_t user_data)
>  {
>  	struct drm_pending_vblank_event *e = NULL;
> -	int ret;
>  
>  	e = kzalloc(sizeof *e, GFP_KERNEL);
>  	if (!e)
> @@ -1488,17 +1490,6 @@ static struct drm_pending_vblank_event *create_vblank_event(
>  	e->event.base.length = sizeof(e->event);
>  	e->event.user_data = user_data;
>  
> -	if (file_priv) {
> -		ret = drm_event_reserve_init(dev, file_priv, &e->base,
> -					     &e->event.base);
> -		if (ret) {
> -			kfree(e);
> -			return NULL;
> -		}
> -	}
> -
> -	e->base.fence = fence;
> -
>  	return e;
>  }
>  
> @@ -1603,6 +1594,40 @@ void drm_atomic_clean_old_fb(struct drm_device *dev,
>  }
>  EXPORT_SYMBOL(drm_atomic_clean_old_fb);
>  
> +static int crtc_setup_out_fence(struct drm_crtc *crtc,
> +				struct drm_crtc_state *crtc_state,
> +				struct drm_out_fence_state *fence_state)
> +{
> +	struct fence *fence;
> +
> +	fence_state->fd = get_unused_fd_flags(O_CLOEXEC);
> +	if (fence_state->fd < 0) {
> +		return fence_state->fd;
> +	}
> +
> +	fence_state->out_fence_ptr = crtc_state->out_fence_ptr;
> +	crtc_state->out_fence_ptr = NULL;
> +
> +	if (put_user(fence_state->fd, fence_state->out_fence_ptr))
> +		return -EFAULT;
> +
> +	fence = kzalloc(sizeof(*fence), GFP_KERNEL);
> +	if (!fence)
> +		return -ENOMEM;
> +
> +	fence_init(fence, &drm_crtc_fence_ops, &crtc->fence_lock,
> +		   crtc->fence_context, ++crtc->fence_seqno);
> +
> +	fence_state->sync_file = sync_file_create(fence);
> +	if(!fence_state->sync_file) {
> +		fence_put(fence);
> +		return -ENOMEM;
> +	}
> +
> +	crtc_state->event->base.fence = fence_get(fence);
> +	return 0;
> +}
> +
>  int drm_mode_atomic_ioctl(struct drm_device *dev,
>  			  void *data, struct drm_file *file_priv)
>  {
> @@ -1617,9 +1642,10 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
>  	struct drm_plane *plane;
>  	struct drm_crtc *crtc;
>  	struct drm_crtc_state *crtc_state;
> +	struct drm_out_fence_state *fence_state;
>  	unsigned plane_mask;
>  	int ret = 0;
> -	unsigned int i, j;
> +	unsigned int i, j, fence_idx = 0;
>  
>  	/* disallow for drivers not supporting atomic: */
>  	if (!drm_core_check_feature(dev, DRIVER_ATOMIC))
> @@ -1734,12 +1760,19 @@ retry:
>  		drm_mode_object_unreference(obj);
>  	}
>  
> -	if (arg->flags & DRM_MODE_PAGE_FLIP_EVENT) {
> -		for_each_crtc_in_state(state, crtc, crtc_state, i) {
> +	fence_state = kcalloc(dev->mode_config.num_crtc, sizeof(*fence_state),
> +			      GFP_KERNEL);
> +	if (!fence_state) {
> +		ret = -ENOMEM;
> +		goto out;
> +	}
> +
> +	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> +		if (arg->flags & DRM_MODE_PAGE_FLIP_EVENT ||
> +		    crtc_state->out_fence_ptr) {
>  			struct drm_pending_vblank_event *e;
>  
> -			e = create_vblank_event(dev, file_priv, NULL,
> -						arg->user_data);
> +			e = create_vblank_event(dev, arg->user_data);
>  			if (!e) {
>  				ret = -ENOMEM;
>  				goto out;
> @@ -1747,6 +1780,28 @@ retry:
>  
>  			crtc_state->event = e;
>  		}
> +
> +		if (arg->flags & DRM_MODE_PAGE_FLIP_EVENT) {
> +			struct drm_pending_vblank_event *e = crtc_state->event;
> +
> +			if (!file_priv)
> +				continue;
> +
> +			ret = drm_event_reserve_init(dev, file_priv, &e->base,
> +						     &e->event.base);
> +			if (ret) {
> +				kfree(e);
> +				crtc_state->event = NULL;
> +				goto out;
> +			}
> +		}
> +
> +		if (crtc_state->out_fence_ptr) {
> +			ret = crtc_setup_out_fence(crtc, crtc_state,
> +						   &fence_state[fence_idx++]);
> +			if (ret)
> +				goto out;
> +		}
>  	}
>  
>  	if (arg->flags & DRM_MODE_ATOMIC_TEST_ONLY) {
> @@ -1761,24 +1816,37 @@ retry:
>  		ret = drm_atomic_commit(state);
>  	}
>  
> +	for (i = 0; i < fence_idx; i++)
> +		fd_install(fence_state[i].fd, fence_state[i].sync_file->file);
> +
>  out:
>  	drm_atomic_clean_old_fb(dev, plane_mask, ret);
>  
> -	if (ret && arg->flags & DRM_MODE_PAGE_FLIP_EVENT) {
> +	for_each_crtc_in_state(state, crtc, crtc_state, i) {
>  		/*
>  		 * TEST_ONLY and PAGE_FLIP_EVENT are mutually exclusive,
>  		 * if they weren't, this code should be called on success
>  		 * for TEST_ONLY too.
>  		 */
> +		if (ret && crtc_state->event)
> +			drm_event_cancel_free(dev, &crtc_state->event->base);
> +	}
>  
> -		for_each_crtc_in_state(state, crtc, crtc_state, i) {
> -			if (!crtc_state->event)
> -				continue;
> +	if (ret && fence_state) {
> +		for (i = 0; i < fence_idx; i++) {
> +			if (fence_state[i].fd >= 0)
> +				put_unused_fd(fence_state[i].fd);
> +			if (fence_state[i].sync_file)
> +				fput(fence_state[i].sync_file->file);
>  
> -			drm_event_cancel_free(dev, &crtc_state->event->base);
> +			/* If this fails, there's not much we can do about it */
> +			if (put_user(-1, fence_state->out_fence_ptr))
> +				DRM_ERROR("Couldn't clear out_fence_ptr\n");
>  		}
>  	}
>  
> +	kfree(fence_state);
> +
>  	if (ret == -EDEADLK) {
>  		drm_atomic_state_clear(state);
>  		drm_modeset_backoff(&ctx);
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index b99090f..40bce97 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -274,6 +274,8 @@ int drm_crtc_init_with_planes(struct drm_device *dev, struct drm_crtc *crtc,
>  	if (drm_core_check_feature(dev, DRIVER_ATOMIC)) {
>  		drm_object_attach_property(&crtc->base, config->prop_active, 0);
>  		drm_object_attach_property(&crtc->base, config->prop_mode_id, 0);
> +		drm_object_attach_property(&crtc->base,
> +					   config->prop_out_fence_ptr, 0);
>  	}
>  
>  	return 0;
> @@ -434,6 +436,12 @@ static int drm_mode_create_standard_properties(struct drm_device *dev)
>  		return -ENOMEM;
>  	dev->mode_config.prop_in_fence_fd = prop;
>  
> +	prop = drm_property_create_range(dev, DRM_MODE_PROP_ATOMIC,
> +			"OUT_FENCE_PTR", 0, U64_MAX);
> +	if (!prop)
> +		return -ENOMEM;
> +	dev->mode_config.prop_out_fence_ptr = prop;
> +
>  	prop = drm_property_create_object(dev, DRM_MODE_PROP_ATOMIC,
>  			"CRTC_ID", DRM_MODE_OBJECT_CRTC);
>  	if (!prop)
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 657a33a..b898604 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -165,6 +165,8 @@ struct drm_crtc_state {
>  	struct drm_property_blob *ctm;
>  	struct drm_property_blob *gamma_lut;
>  
> +	u64 __user *out_fence_ptr;
> +
>  	/**
>  	 * @event:
>  	 *
> @@ -748,6 +750,18 @@ static inline struct drm_crtc *fence_to_crtc(struct fence *fence)
>  }
>  
>  /**
> + * struct drm_out_fence_state - store out_fence data for install and clean up
> + * @out_fence_ptr: user pointer to store the fence fd in.
> + * @sync_file: sync_file related to the out_fence
> + * @fd: file descriptor to installed on the sync_file.
> + */
> +struct drm_out_fence_state {
> +	u64 __user *out_fence_ptr;
> +	struct sync_file *sync_file;
> +	int fd;
> +};
> +
> +/*
>   * struct drm_mode_set - new values for a CRTC config change
>   * @fb: framebuffer to use for new config
>   * @crtc: CRTC whose configuration we're about to change
> @@ -1230,6 +1244,11 @@ struct drm_mode_config {
>  	 */
>  	struct drm_property *prop_in_fence_fd;
>  	/**
> +	 * @prop_out_fence_ptr: Sync File fd pointer representing the
> +	 * outgoing fences for a CRTC.
> +	 */
> +	struct drm_property *prop_out_fence_ptr;
> +	/**
>  	 * @prop_crtc_id: Default atomic plane property to specify the
>  	 * &drm_crtc.
>  	 */
> -- 
> 2.5.5
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH v5 4/4] drm/fence: add out-fences support

[Gustavo Padovan]

2016-10-20 Ville Syrjälä <ville.syrjala@linux.intel.com>:

> On Thu, Oct 20, 2016 at 12:50:05PM -0200, Gustavo Padovan wrote:
> > From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> > 
> > Support DRM out-fences by creating a sync_file with a fence for each CRTC
> > that sets the OUT_FENCE_PTR property.
> 
> I still maintain the out fence should also be per fb (well, per plane
> since we can't add it to fbs). Otherwise it's not going to be at all
> useful if you do fps>vrefresh and don't include the same set of planes
> in every atomic ioctl, eg. if you only ever include ones that are
> somehow dirty.

How would the kernel signal these dirty planes then? Right now we signal
at the vblank.

Gustavo

Re: [PATCH v5 4/4] drm/fence: add out-fences support

[Ville Syrjälä]

On Thu, Oct 20, 2016 at 01:55:38PM -0200, Gustavo Padovan wrote:
> 2016-10-20 Ville Syrjälä <ville.syrjala@linux.intel.com>:
> 
> > On Thu, Oct 20, 2016 at 12:50:05PM -0200, Gustavo Padovan wrote:
> > > From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> > > 
> > > Support DRM out-fences by creating a sync_file with a fence for each CRTC
> > > that sets the OUT_FENCE_PTR property.
> > 
> > I still maintain the out fence should also be per fb (well, per plane
> > since we can't add it to fbs). Otherwise it's not going to be at all
> > useful if you do fps>vrefresh and don't include the same set of planes
> > in every atomic ioctl, eg. if you only ever include ones that are
> > somehow dirty.
> 
> How would the kernel signal these dirty planes then? Right now we signal
> at the vblank.

So if I think about it in terms of the previous fbs something like this
comes to mind:

 starting point:
 plane a, fb 0
 plane b, fb 1

 ioctl:
 plane a, fb 2, fence A
 plane b, fb 3, fence B

 ioctl:
 plane a, fb 4, fence C
 -> fb 2 can be reused, so fence C should signal immediately?

 vblank:
 -> fb 0,1 can be reused, so fence A,B signal?

It feels a bit wonky since the fence is effectively associated with the
previous fb after the previous fb was already submitted to the kernel.
One might assume fence A to be the one signalled early, but that would
give the impression that fb 0 would be free for reuse when it's not.

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH v5 3/4] drm/fence: add fence timeline to drm_crtc

[Brian Starkey]

Hi Gustavo,

On Thu, Oct 20, 2016 at 12:50:04PM -0200, Gustavo Padovan wrote:
>From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
>
>Create one timeline context for each CRTC to be able to handle out-fences
>and signal them. It adds a few members to struct drm_crtc: fence_context,
>where we store the context we get from fence_context_alloc(), the
>fence seqno and the fence lock, that we pass in fence_init() to be
>used by the fence.
>
>v2: Comment by Daniel Stone:
>	- add BUG_ON() to fence_to_crtc() macro
>
>v3: Comment by Ville Syrjälä
>	- Use more meaningful name as crtc timeline name
>
>Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
>---
> drivers/gpu/drm/drm_crtc.c | 31 +++++++++++++++++++++++++++++++
> include/drm/drm_crtc.h     | 19 +++++++++++++++++++
> 2 files changed, 50 insertions(+)
>
>diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
>index fcb6453..b99090f 100644
>--- a/drivers/gpu/drm/drm_crtc.c
>+++ b/drivers/gpu/drm/drm_crtc.c
>@@ -165,6 +165,32 @@ static void drm_crtc_crc_fini(struct drm_crtc *crtc)
> #endif
> }
>
>+static const char *drm_crtc_fence_get_driver_name(struct fence *fence)
>+{
>+	struct drm_crtc *crtc = fence_to_crtc(fence);
>+
>+	return crtc->dev->driver->name;
>+}
>+
>+static const char *drm_crtc_fence_get_timeline_name(struct fence *fence)
>+{
>+	struct drm_crtc *crtc = fence_to_crtc(fence);
>+
>+	return crtc->timeline_name;
>+}
>+
>+static bool drm_crtc_fence_enable_signaling(struct fence *fence)
>+{
>+	return true;
>+}
>+
>+const struct fence_ops drm_crtc_fence_ops = {
>+	.get_driver_name = drm_crtc_fence_get_driver_name,
>+	.get_timeline_name = drm_crtc_fence_get_timeline_name,
>+	.enable_signaling = drm_crtc_fence_enable_signaling,
>+	.wait = fence_default_wait,
>+};
>+
> /**
>  * drm_crtc_init_with_planes - Initialise a new CRTC object with
>  *    specified primary and cursor planes.
>@@ -222,6 +248,11 @@ int drm_crtc_init_with_planes(struct drm_device *dev, struct drm_crtc *crtc,
> 		return -ENOMEM;
> 	}
>
>+	crtc->fence_context = fence_context_alloc(1);
>+	spin_lock_init(&crtc->fence_lock);
>+	snprintf(crtc->timeline_name, sizeof(crtc->timeline_name),
>+		 "drm_crtc-%d", crtc->base.id);

I wondered about "[CRTC:id:name]" to be consistent with the DRM debug
prints.

>+
> 	crtc->base.properties = &crtc->properties;
>
> 	list_add_tail(&crtc->head, &config->crtc_list);
>diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
>index 279132e..657a33a 100644
>--- a/include/drm/drm_crtc.h
>+++ b/include/drm/drm_crtc.h
>@@ -32,6 +32,8 @@
> #include <linux/fb.h>
> #include <linux/hdmi.h>
> #include <linux/media-bus-format.h>
>+#include <linux/srcu.h>
>+#include <linux/fence.h>
> #include <uapi/drm/drm_mode.h>
> #include <uapi/drm/drm_fourcc.h>
> #include <drm/drm_modeset_lock.h>
>@@ -618,6 +620,9 @@ struct drm_crtc_funcs {
>  * @gamma_store: gamma ramp values
>  * @helper_private: mid-layer private data
>  * @properties: property tracking for this CRTC
>+ * @fence_context: context for fence signalling
>+ * @fence_lock: fence lock for the fence context
>+ * @fence_seqno: seqno variable to create fences

@timeline_name ?

Cheers,
Brian

>  *
>  * Each CRTC may have one or more connectors associated with it.  This structure
>  * allows the CRTC to be controlled.
>@@ -726,8 +731,22 @@ struct drm_crtc {
> 	 */
> 	struct drm_crtc_crc crc;
> #endif
>+
>+	/* fence timelines info for DRM out-fences */
>+	unsigned int fence_context;
>+	spinlock_t fence_lock;
>+	unsigned long fence_seqno;
>+	char timeline_name[32];
> };
>
>+extern const struct fence_ops drm_crtc_fence_ops;
>+
>+static inline struct drm_crtc *fence_to_crtc(struct fence *fence)
>+{
>+	BUG_ON(fence->ops != &drm_crtc_fence_ops);
>+	return container_of(fence->lock, struct drm_crtc, fence_lock);
>+}
>+
> /**
>  * struct drm_mode_set - new values for a CRTC config change
>  * @fb: framebuffer to use for new config
>-- 
>2.5.5
>
>_______________________________________________
>dri-devel mailing list
>dri-devel@lists.freedesktop.org
>https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v5 4/4] drm/fence: add out-fences support

[Brian Starkey]

Hi Gustavo,

I notice your branch has the sync_file refcount change in, but this
doesn't seem to take account for that. Will you be dropping that
change to match the semantics of fence_array?

Couple more comments below.

On Thu, Oct 20, 2016 at 12:50:05PM -0200, Gustavo Padovan wrote:
>From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
>
>Support DRM out-fences by creating a sync_file with a fence for each CRTC
>that sets the OUT_FENCE_PTR property.
>
>We use the out_fence pointer received in the OUT_FENCE_PTR prop to send
>the sync_file fd back to userspace.
>
>The sync_file and fd are allocated/created before commit, but the
>fd_install operation only happens after we know that commit succeed.
>
>In case of error userspace should receive a fd number of -1.
>
>v2: Comment by Rob Clark:
>	- Squash commit that adds DRM_MODE_ATOMIC_OUT_FENCE flag here.
>
>    Comment by Daniel Vetter:
>	- Add clean up code for out_fences
>
>v3: Comments by Daniel Vetter:
>	- create DRM_MODE_ATOMIC_EVENT_MASK
>	- userspace should fill out_fences_ptr with the crtc_ids for which
>	it wants fences back.
>
>v4: Create OUT_FENCE_PTR properties and remove old approach.
>
>Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
>---
> drivers/gpu/drm/drm_atomic.c | 116 ++++++++++++++++++++++++++++++++++---------
> drivers/gpu/drm/drm_crtc.c   |   8 +++
> include/drm/drm_crtc.h       |  19 +++++++
> 3 files changed, 119 insertions(+), 24 deletions(-)
>
>diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
>index b3284b2..07775fc 100644
>--- a/drivers/gpu/drm/drm_atomic.c
>+++ b/drivers/gpu/drm/drm_atomic.c
>@@ -490,6 +490,8 @@ int drm_atomic_crtc_set_property(struct drm_crtc *crtc,
> 					&replaced);
> 		state->color_mgmt_changed |= replaced;
> 		return ret;
>+	} else if (property == config->prop_out_fence_ptr) {
>+		state->out_fence_ptr = u64_to_user_ptr(val);
> 	} else if (crtc->funcs->atomic_set_property)
> 		return crtc->funcs->atomic_set_property(crtc, state, property, val);
> 	else
>@@ -532,6 +534,8 @@ drm_atomic_crtc_get_property(struct drm_crtc *crtc,
> 		*val = (state->ctm) ? state->ctm->base.id : 0;
> 	else if (property == config->gamma_lut_property)
> 		*val = (state->gamma_lut) ? state->gamma_lut->base.id : 0;
>+	else if (property == config->prop_out_fence_ptr)
>+		*val = 0;
> 	else if (crtc->funcs->atomic_get_property)
> 		return crtc->funcs->atomic_get_property(crtc, state, property, val);
> 	else
>@@ -1474,11 +1478,9 @@ EXPORT_SYMBOL(drm_atomic_nonblocking_commit);
>  */
>
> static struct drm_pending_vblank_event *create_vblank_event(
>-		struct drm_device *dev, struct drm_file *file_priv,
>-		struct fence *fence, uint64_t user_data)
>+		struct drm_device *dev, uint64_t user_data)
> {
> 	struct drm_pending_vblank_event *e = NULL;
>-	int ret;
>
> 	e = kzalloc(sizeof *e, GFP_KERNEL);
> 	if (!e)
>@@ -1488,17 +1490,6 @@ static struct drm_pending_vblank_event *create_vblank_event(
> 	e->event.base.length = sizeof(e->event);
> 	e->event.user_data = user_data;
>
>-	if (file_priv) {
>-		ret = drm_event_reserve_init(dev, file_priv, &e->base,
>-					     &e->event.base);
>-		if (ret) {
>-			kfree(e);
>-			return NULL;
>-		}
>-	}
>-
>-	e->base.fence = fence;
>-
> 	return e;
> }
>
>@@ -1603,6 +1594,40 @@ void drm_atomic_clean_old_fb(struct drm_device *dev,
> }
> EXPORT_SYMBOL(drm_atomic_clean_old_fb);
>
>+static int crtc_setup_out_fence(struct drm_crtc *crtc,
>+				struct drm_crtc_state *crtc_state,
>+				struct drm_out_fence_state *fence_state)
>+{
>+	struct fence *fence;
>+
>+	fence_state->fd = get_unused_fd_flags(O_CLOEXEC);
>+	if (fence_state->fd < 0) {
>+		return fence_state->fd;
>+	}
>+
>+	fence_state->out_fence_ptr = crtc_state->out_fence_ptr;
>+	crtc_state->out_fence_ptr = NULL;
>+
>+	if (put_user(fence_state->fd, fence_state->out_fence_ptr))
>+		return -EFAULT;
>+
>+	fence = kzalloc(sizeof(*fence), GFP_KERNEL);
>+	if (!fence)
>+		return -ENOMEM;
>+
>+	fence_init(fence, &drm_crtc_fence_ops, &crtc->fence_lock,
>+		   crtc->fence_context, ++crtc->fence_seqno);
>+
>+	fence_state->sync_file = sync_file_create(fence);
>+	if(!fence_state->sync_file) {
>+		fence_put(fence);
>+		return -ENOMEM;
>+	}
>+
>+	crtc_state->event->base.fence = fence_get(fence);
>+	return 0;
>+}
>+
> int drm_mode_atomic_ioctl(struct drm_device *dev,
> 			  void *data, struct drm_file *file_priv)
> {
>@@ -1617,9 +1642,10 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
> 	struct drm_plane *plane;
> 	struct drm_crtc *crtc;
> 	struct drm_crtc_state *crtc_state;
>+	struct drm_out_fence_state *fence_state;
> 	unsigned plane_mask;
> 	int ret = 0;
>-	unsigned int i, j;
>+	unsigned int i, j, fence_idx = 0;
>
> 	/* disallow for drivers not supporting atomic: */
> 	if (!drm_core_check_feature(dev, DRIVER_ATOMIC))
>@@ -1734,12 +1760,19 @@ retry:
> 		drm_mode_object_unreference(obj);
> 	}
>
>-	if (arg->flags & DRM_MODE_PAGE_FLIP_EVENT) {
>-		for_each_crtc_in_state(state, crtc, crtc_state, i) {
>+	fence_state = kcalloc(dev->mode_config.num_crtc, sizeof(*fence_state),
>+			      GFP_KERNEL);
>+	if (!fence_state) {
>+		ret = -ENOMEM;
>+		goto out;
>+	}
>+
>+	for_each_crtc_in_state(state, crtc, crtc_state, i) {
>+		if (arg->flags & DRM_MODE_PAGE_FLIP_EVENT ||
>+		    crtc_state->out_fence_ptr) {
> 			struct drm_pending_vblank_event *e;
>
>-			e = create_vblank_event(dev, file_priv, NULL,
>-						arg->user_data);
>+			e = create_vblank_event(dev, arg->user_data);
> 			if (!e) {
> 				ret = -ENOMEM;
> 				goto out;
>@@ -1747,6 +1780,28 @@ retry:
>
> 			crtc_state->event = e;
> 		}
>+
>+		if (arg->flags & DRM_MODE_PAGE_FLIP_EVENT) {
>+			struct drm_pending_vblank_event *e = crtc_state->event;
>+
>+			if (!file_priv)
>+				continue;
>+
>+			ret = drm_event_reserve_init(dev, file_priv, &e->base,
>+						     &e->event.base);
>+			if (ret) {
>+				kfree(e);
>+				crtc_state->event = NULL;
>+				goto out;
>+			}
>+		}
>+
>+		if (crtc_state->out_fence_ptr) {
>+			ret = crtc_setup_out_fence(crtc, crtc_state,
>+						   &fence_state[fence_idx++]);
>+			if (ret)
>+				goto out;
>+		}
> 	}
>
> 	if (arg->flags & DRM_MODE_ATOMIC_TEST_ONLY) {
>@@ -1761,24 +1816,37 @@ retry:
> 		ret = drm_atomic_commit(state);
> 	}
>
>+	for (i = 0; i < fence_idx; i++)
>+		fd_install(fence_state[i].fd, fence_state[i].sync_file->file);
>+
> out:
> 	drm_atomic_clean_old_fb(dev, plane_mask, ret);
>
>-	if (ret && arg->flags & DRM_MODE_PAGE_FLIP_EVENT) {
>+	for_each_crtc_in_state(state, crtc, crtc_state, i) {

I think a check on ret is still needed before you iterate the CRTCs.
If the commit is successful then state has already been freed by this
point, and I get a splat.

> 		/*
> 		 * TEST_ONLY and PAGE_FLIP_EVENT are mutually exclusive,
> 		 * if they weren't, this code should be called on success
> 		 * for TEST_ONLY too.
> 		 */
>+		if (ret && crtc_state->event)
>+			drm_event_cancel_free(dev, &crtc_state->event->base);
>+	}
>
>-		for_each_crtc_in_state(state, crtc, crtc_state, i) {
>-			if (!crtc_state->event)
>-				continue;
>+	if (ret && fence_state) {
>+		for (i = 0; i < fence_idx; i++) {
>+			if (fence_state[i].fd >= 0)
>+				put_unused_fd(fence_state[i].fd);
>+			if (fence_state[i].sync_file)
>+				fput(fence_state[i].sync_file->file);
>
>-			drm_event_cancel_free(dev, &crtc_state->event->base);
>+			/* If this fails, there's not much we can do about it */
>+			if (put_user(-1, fence_state->out_fence_ptr))
>+				DRM_ERROR("Couldn't clear out_fence_ptr\n");
> 		}
> 	}
>
>+	kfree(fence_state);
>+
> 	if (ret == -EDEADLK) {
> 		drm_atomic_state_clear(state);
> 		drm_modeset_backoff(&ctx);
>diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
>index b99090f..40bce97 100644
>--- a/drivers/gpu/drm/drm_crtc.c
>+++ b/drivers/gpu/drm/drm_crtc.c
>@@ -274,6 +274,8 @@ int drm_crtc_init_with_planes(struct drm_device *dev, struct drm_crtc *crtc,
> 	if (drm_core_check_feature(dev, DRIVER_ATOMIC)) {
> 		drm_object_attach_property(&crtc->base, config->prop_active, 0);
> 		drm_object_attach_property(&crtc->base, config->prop_mode_id, 0);
>+		drm_object_attach_property(&crtc->base,
>+					   config->prop_out_fence_ptr, 0);
> 	}
>
> 	return 0;
>@@ -434,6 +436,12 @@ static int drm_mode_create_standard_properties(struct drm_device *dev)
> 		return -ENOMEM;
> 	dev->mode_config.prop_in_fence_fd = prop;
>
>+	prop = drm_property_create_range(dev, DRM_MODE_PROP_ATOMIC,
>+			"OUT_FENCE_PTR", 0, U64_MAX);
>+	if (!prop)
>+		return -ENOMEM;
>+	dev->mode_config.prop_out_fence_ptr = prop;
>+
> 	prop = drm_property_create_object(dev, DRM_MODE_PROP_ATOMIC,
> 			"CRTC_ID", DRM_MODE_OBJECT_CRTC);
> 	if (!prop)
>diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
>index 657a33a..b898604 100644
>--- a/include/drm/drm_crtc.h
>+++ b/include/drm/drm_crtc.h
>@@ -165,6 +165,8 @@ struct drm_crtc_state {
> 	struct drm_property_blob *ctm;
> 	struct drm_property_blob *gamma_lut;
>
>+	u64 __user *out_fence_ptr;
>+

I'm somewhat not convinced about stashing a __user pointer in the
CRTC atomic state. I know it gets cleared before the driver sees it,
but if anything I think that hints that this isn't the right place to
store it.

I've been trying to think of other ways to get from a property to
something drm_mode_atomic_ioctl can use - maybe we can store some
stuff in drm_atomic_state which only lives for the length of the ioctl
call and put this pointer in there.

Thanks,
Brian

> 	/**
> 	 * @event:
> 	 *
>@@ -748,6 +750,18 @@ static inline struct drm_crtc *fence_to_crtc(struct fence *fence)
> }
>
> /**
>+ * struct drm_out_fence_state - store out_fence data for install and clean up
>+ * @out_fence_ptr: user pointer to store the fence fd in.
>+ * @sync_file: sync_file related to the out_fence
>+ * @fd: file descriptor to installed on the sync_file.
>+ */
>+struct drm_out_fence_state {
>+	u64 __user *out_fence_ptr;
>+	struct sync_file *sync_file;
>+	int fd;
>+};
>+
>+/*
>  * struct drm_mode_set - new values for a CRTC config change
>  * @fb: framebuffer to use for new config
>  * @crtc: CRTC whose configuration we're about to change
>@@ -1230,6 +1244,11 @@ struct drm_mode_config {
> 	 */
> 	struct drm_property *prop_in_fence_fd;
> 	/**
>+	 * @prop_out_fence_ptr: Sync File fd pointer representing the
>+	 * outgoing fences for a CRTC.
>+	 */
>+	struct drm_property *prop_out_fence_ptr;
>+	/**
> 	 * @prop_crtc_id: Default atomic plane property to specify the
> 	 * &drm_crtc.
> 	 */
>-- 
>2.5.5
>
>_______________________________________________
>dri-devel mailing list
>dri-devel@lists.freedesktop.org
>https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v5 4/4] drm/fence: add out-fences support

[Brian Starkey]

Hi Gustavo,

I notice your branch has the sync_file refcount change in, but this
doesn't seem to take account for that. Will you be dropping that
change to match the semantics of fence_array?

Couple more comments below.

On Thu, Oct 20, 2016 at 12:50:05PM -0200, Gustavo Padovan wrote:
>From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
>
>Support DRM out-fences by creating a sync_file with a fence for each CRTC
>that sets the OUT_FENCE_PTR property.
>
>We use the out_fence pointer received in the OUT_FENCE_PTR prop to send
>the sync_file fd back to userspace.
>
>The sync_file and fd are allocated/created before commit, but the
>fd_install operation only happens after we know that commit succeed.
>
>In case of error userspace should receive a fd number of -1.
>
>v2: Comment by Rob Clark:
>	- Squash commit that adds DRM_MODE_ATOMIC_OUT_FENCE flag here.
>
>    Comment by Daniel Vetter:
>	- Add clean up code for out_fences
>
>v3: Comments by Daniel Vetter:
>	- create DRM_MODE_ATOMIC_EVENT_MASK
>	- userspace should fill out_fences_ptr with the crtc_ids for which
>	it wants fences back.
>
>v4: Create OUT_FENCE_PTR properties and remove old approach.
>
>Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
>---
> drivers/gpu/drm/drm_atomic.c | 116 ++++++++++++++++++++++++++++++++++---------
> drivers/gpu/drm/drm_crtc.c   |   8 +++
> include/drm/drm_crtc.h       |  19 +++++++
> 3 files changed, 119 insertions(+), 24 deletions(-)
>
>diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
>index b3284b2..07775fc 100644
>--- a/drivers/gpu/drm/drm_atomic.c
>+++ b/drivers/gpu/drm/drm_atomic.c
>@@ -490,6 +490,8 @@ int drm_atomic_crtc_set_property(struct drm_crtc *crtc,
> 					&replaced);
> 		state->color_mgmt_changed |= replaced;
> 		return ret;
>+	} else if (property == config->prop_out_fence_ptr) {
>+		state->out_fence_ptr = u64_to_user_ptr(val);
> 	} else if (crtc->funcs->atomic_set_property)
> 		return crtc->funcs->atomic_set_property(crtc, state, property, val);
> 	else
>@@ -532,6 +534,8 @@ drm_atomic_crtc_get_property(struct drm_crtc *crtc,
> 		*val = (state->ctm) ? state->ctm->base.id : 0;
> 	else if (property == config->gamma_lut_property)
> 		*val = (state->gamma_lut) ? state->gamma_lut->base.id : 0;
>+	else if (property == config->prop_out_fence_ptr)
>+		*val = 0;
> 	else if (crtc->funcs->atomic_get_property)
> 		return crtc->funcs->atomic_get_property(crtc, state, property, val);
> 	else
>@@ -1474,11 +1478,9 @@ EXPORT_SYMBOL(drm_atomic_nonblocking_commit);
>  */
>
> static struct drm_pending_vblank_event *create_vblank_event(
>-		struct drm_device *dev, struct drm_file *file_priv,
>-		struct fence *fence, uint64_t user_data)
>+		struct drm_device *dev, uint64_t user_data)
> {
> 	struct drm_pending_vblank_event *e = NULL;
>-	int ret;
>
> 	e = kzalloc(sizeof *e, GFP_KERNEL);
> 	if (!e)
>@@ -1488,17 +1490,6 @@ static struct drm_pending_vblank_event *create_vblank_event(
> 	e->event.base.length = sizeof(e->event);
> 	e->event.user_data = user_data;
>
>-	if (file_priv) {
>-		ret = drm_event_reserve_init(dev, file_priv, &e->base,
>-					     &e->event.base);
>-		if (ret) {
>-			kfree(e);
>-			return NULL;
>-		}
>-	}
>-
>-	e->base.fence = fence;
>-
> 	return e;
> }
>
>@@ -1603,6 +1594,40 @@ void drm_atomic_clean_old_fb(struct drm_device *dev,
> }
> EXPORT_SYMBOL(drm_atomic_clean_old_fb);
>
>+static int crtc_setup_out_fence(struct drm_crtc *crtc,
>+				struct drm_crtc_state *crtc_state,
>+				struct drm_out_fence_state *fence_state)
>+{
>+	struct fence *fence;
>+
>+	fence_state->fd = get_unused_fd_flags(O_CLOEXEC);
>+	if (fence_state->fd < 0) {
>+		return fence_state->fd;
>+	}
>+
>+	fence_state->out_fence_ptr = crtc_state->out_fence_ptr;
>+	crtc_state->out_fence_ptr = NULL;
>+
>+	if (put_user(fence_state->fd, fence_state->out_fence_ptr))
>+		return -EFAULT;
>+
>+	fence = kzalloc(sizeof(*fence), GFP_KERNEL);
>+	if (!fence)
>+		return -ENOMEM;
>+
>+	fence_init(fence, &drm_crtc_fence_ops, &crtc->fence_lock,
>+		   crtc->fence_context, ++crtc->fence_seqno);
>+
>+	fence_state->sync_file = sync_file_create(fence);
>+	if(!fence_state->sync_file) {
>+		fence_put(fence);
>+		return -ENOMEM;
>+	}
>+
>+	crtc_state->event->base.fence = fence_get(fence);
>+	return 0;
>+}
>+
> int drm_mode_atomic_ioctl(struct drm_device *dev,
> 			  void *data, struct drm_file *file_priv)
> {
>@@ -1617,9 +1642,10 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
> 	struct drm_plane *plane;
> 	struct drm_crtc *crtc;
> 	struct drm_crtc_state *crtc_state;
>+	struct drm_out_fence_state *fence_state;
> 	unsigned plane_mask;
> 	int ret = 0;
>-	unsigned int i, j;
>+	unsigned int i, j, fence_idx = 0;
>
> 	/* disallow for drivers not supporting atomic: */
> 	if (!drm_core_check_feature(dev, DRIVER_ATOMIC))
>@@ -1734,12 +1760,19 @@ retry:
> 		drm_mode_object_unreference(obj);
> 	}
>
>-	if (arg->flags & DRM_MODE_PAGE_FLIP_EVENT) {
>-		for_each_crtc_in_state(state, crtc, crtc_state, i) {
>+	fence_state = kcalloc(dev->mode_config.num_crtc, sizeof(*fence_state),
>+			      GFP_KERNEL);
>+	if (!fence_state) {
>+		ret = -ENOMEM;
>+		goto out;
>+	}
>+
>+	for_each_crtc_in_state(state, crtc, crtc_state, i) {
>+		if (arg->flags & DRM_MODE_PAGE_FLIP_EVENT ||
>+		    crtc_state->out_fence_ptr) {
> 			struct drm_pending_vblank_event *e;
>
>-			e = create_vblank_event(dev, file_priv, NULL,
>-						arg->user_data);
>+			e = create_vblank_event(dev, arg->user_data);
> 			if (!e) {
> 				ret = -ENOMEM;
> 				goto out;
>@@ -1747,6 +1780,28 @@ retry:
>
> 			crtc_state->event = e;
> 		}
>+
>+		if (arg->flags & DRM_MODE_PAGE_FLIP_EVENT) {
>+			struct drm_pending_vblank_event *e = crtc_state->event;
>+
>+			if (!file_priv)
>+				continue;
>+
>+			ret = drm_event_reserve_init(dev, file_priv, &e->base,
>+						     &e->event.base);
>+			if (ret) {
>+				kfree(e);
>+				crtc_state->event = NULL;
>+				goto out;
>+			}
>+		}
>+
>+		if (crtc_state->out_fence_ptr) {
>+			ret = crtc_setup_out_fence(crtc, crtc_state,
>+						   &fence_state[fence_idx++]);
>+			if (ret)
>+				goto out;
>+		}
> 	}
>
> 	if (arg->flags & DRM_MODE_ATOMIC_TEST_ONLY) {
>@@ -1761,24 +1816,37 @@ retry:
> 		ret = drm_atomic_commit(state);
> 	}
>
>+	for (i = 0; i < fence_idx; i++)
>+		fd_install(fence_state[i].fd, fence_state[i].sync_file->file);
>+
> out:
> 	drm_atomic_clean_old_fb(dev, plane_mask, ret);
>
>-	if (ret && arg->flags & DRM_MODE_PAGE_FLIP_EVENT) {
>+	for_each_crtc_in_state(state, crtc, crtc_state, i) {

I think a check on ret is still needed before you iterate the CRTCs.
If the commit is successful then state has already been freed by this
point, and I get a splat.

> 		/*
> 		 * TEST_ONLY and PAGE_FLIP_EVENT are mutually exclusive,
> 		 * if they weren't, this code should be called on success
> 		 * for TEST_ONLY too.
> 		 */
>+		if (ret && crtc_state->event)
>+			drm_event_cancel_free(dev, &crtc_state->event->base);
>+	}
>
>-		for_each_crtc_in_state(state, crtc, crtc_state, i) {
>-			if (!crtc_state->event)
>-				continue;
>+	if (ret && fence_state) {
>+		for (i = 0; i < fence_idx; i++) {
>+			if (fence_state[i].fd >= 0)
>+				put_unused_fd(fence_state[i].fd);
>+			if (fence_state[i].sync_file)
>+				fput(fence_state[i].sync_file->file);
>
>-			drm_event_cancel_free(dev, &crtc_state->event->base);
>+			/* If this fails, there's not much we can do about it */
>+			if (put_user(-1, fence_state->out_fence_ptr))
>+				DRM_ERROR("Couldn't clear out_fence_ptr\n");
> 		}
> 	}
>
>+	kfree(fence_state);
>+
> 	if (ret == -EDEADLK) {
> 		drm_atomic_state_clear(state);
> 		drm_modeset_backoff(&ctx);
>diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
>index b99090f..40bce97 100644
>--- a/drivers/gpu/drm/drm_crtc.c
>+++ b/drivers/gpu/drm/drm_crtc.c
>@@ -274,6 +274,8 @@ int drm_crtc_init_with_planes(struct drm_device *dev, struct drm_crtc *crtc,
> 	if (drm_core_check_feature(dev, DRIVER_ATOMIC)) {
> 		drm_object_attach_property(&crtc->base, config->prop_active, 0);
> 		drm_object_attach_property(&crtc->base, config->prop_mode_id, 0);
>+		drm_object_attach_property(&crtc->base,
>+					   config->prop_out_fence_ptr, 0);
> 	}
>
> 	return 0;
>@@ -434,6 +436,12 @@ static int drm_mode_create_standard_properties(struct drm_device *dev)
> 		return -ENOMEM;
> 	dev->mode_config.prop_in_fence_fd = prop;
>
>+	prop = drm_property_create_range(dev, DRM_MODE_PROP_ATOMIC,
>+			"OUT_FENCE_PTR", 0, U64_MAX);
>+	if (!prop)
>+		return -ENOMEM;
>+	dev->mode_config.prop_out_fence_ptr = prop;
>+
> 	prop = drm_property_create_object(dev, DRM_MODE_PROP_ATOMIC,
> 			"CRTC_ID", DRM_MODE_OBJECT_CRTC);
> 	if (!prop)
>diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
>index 657a33a..b898604 100644
>--- a/include/drm/drm_crtc.h
>+++ b/include/drm/drm_crtc.h
>@@ -165,6 +165,8 @@ struct drm_crtc_state {
> 	struct drm_property_blob *ctm;
> 	struct drm_property_blob *gamma_lut;
>
>+	u64 __user *out_fence_ptr;
>+

I'm somewhat not convinced about stashing a __user pointer in the
CRTC atomic state. I know it gets cleared before the driver sees it,
but if anything I think that hints that this isn't the right place to
store it.

I've been trying to think of other ways to get from a property to
something drm_mode_atomic_ioctl can use - maybe we can store some
stuff in drm_atomic_state which only lives for the length of the ioctl
call and put this pointer in there.

Thanks,
Brian

> 	/**
> 	 * @event:
> 	 *
>@@ -748,6 +750,18 @@ static inline struct drm_crtc *fence_to_crtc(struct fence *fence)
> }
>
> /**
>+ * struct drm_out_fence_state - store out_fence data for install and clean up
>+ * @out_fence_ptr: user pointer to store the fence fd in.
>+ * @sync_file: sync_file related to the out_fence
>+ * @fd: file descriptor to installed on the sync_file.
>+ */
>+struct drm_out_fence_state {
>+	u64 __user *out_fence_ptr;
>+	struct sync_file *sync_file;
>+	int fd;
>+};
>+
>+/*
>  * struct drm_mode_set - new values for a CRTC config change
>  * @fb: framebuffer to use for new config
>  * @crtc: CRTC whose configuration we're about to change
>@@ -1230,6 +1244,11 @@ struct drm_mode_config {
> 	 */
> 	struct drm_property *prop_in_fence_fd;
> 	/**
>+	 * @prop_out_fence_ptr: Sync File fd pointer representing the
>+	 * outgoing fences for a CRTC.
>+	 */
>+	struct drm_property *prop_out_fence_ptr;
>+	/**
> 	 * @prop_crtc_id: Default atomic plane property to specify the
> 	 * &drm_crtc.
> 	 */
>-- 
>2.5.5
>
>_______________________________________________
>dri-devel mailing list
>dri-devel@lists.freedesktop.org
>https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v5 4/4] drm/fence: add out-fences support

[Ville Syrjälä]

On Thu, Oct 20, 2016 at 07:34:44PM +0300, Ville Syrjälä wrote:
> On Thu, Oct 20, 2016 at 01:55:38PM -0200, Gustavo Padovan wrote:
> > 2016-10-20 Ville Syrjälä <ville.syrjala@linux.intel.com>:
> > 
> > > On Thu, Oct 20, 2016 at 12:50:05PM -0200, Gustavo Padovan wrote:
> > > > From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> > > > 
> > > > Support DRM out-fences by creating a sync_file with a fence for each CRTC
> > > > that sets the OUT_FENCE_PTR property.
> > > 
> > > I still maintain the out fence should also be per fb (well, per plane
> > > since we can't add it to fbs). Otherwise it's not going to be at all
> > > useful if you do fps>vrefresh and don't include the same set of planes
> > > in every atomic ioctl, eg. if you only ever include ones that are
> > > somehow dirty.
> > 
> > How would the kernel signal these dirty planes then? Right now we signal
> > at the vblank.
> 
> So if I think about it in terms of the previous fbs something like this
> comes to mind:
> 
>  starting point:
>  plane a, fb 0
>  plane b, fb 1
> 
>  ioctl:
>  plane a, fb 2, fence A
>  plane b, fb 3, fence B
> 
>  ioctl:
>  plane a, fb 4, fence C
>  -> fb 2 can be reused, so fence C should signal immediately?
> 
>  vblank:
>  -> fb 0,1 can be reused, so fence A,B signal?
> 
> It feels a bit wonky since the fence is effectively associated with the
> previous fb after the previous fb was already submitted to the kernel.
> One might assume fence A to be the one signalled early, but that would
> give the impression that fb 0 would be free for reuse when it's not.

OK, so after hashing this out on irc with Gustavo and Brian, we came
to the conclusion that the per-crtc out fence should be sufficient
after all.

The way it can work is that the first ioctl will return the fence,
doesn't really matter how many of the planes are involved in this
ioctl. Subsequent ioctls that manage to get in before the next
vblank will get back a -1 as the fence, even if the set if planes
involved is not the same as in the first ioctl. From the -1
userspace can tell what happened, and can then consult its own
records to see which still pending flips were overwritten, and
thus knows which buffers can be reused immediately.

If userspace plans on sending out the now free buffers to someone
else accompanied by a fence, it can just create an already signalled
fence to send instead of sending the fence it got back from the
atomic ioctl since that one is still associated with the original
scanout buffers.

The fence returned by the atomic ioctl will only signal after the
vblank, at which point userspace will obviously know that the
orginal scanout buffers are now also ready for reuse, and that
the last buffer submitted for each plane is now being actively
scanned out. So if userspace wants to send out some of the
original buffers to someone else, it can send along the fence
returned from the atomic ioctl.

So requires a bit more work from userspace perhaps. But obviously
it only has to do this if it plans on submitting multiple operations
per frame.

So a slightly expanded version of my previous example might look like:
starting point:
 plane a, fb 0
 plane b, fb 1

ioctl:
 crtc: fence A
 plane a, fb 2
 plane b, fb 3

ioctl:
 crtc: fence -1
 plane a, fb 4
 -> the previously submitted fb for plane a (fb 2) can be reused

ioctl:
 crtc: fence -1
 plane a, fb 5
 -> the previously submitted fb for plane a (fb 4) can be reused

vblank:
 -> fence A signals, fb 0,1 can be reused
    fb 3 and 5 being scanned out now


We also had a quick side track w.r.t. variable refresh rate displays,
and I think the conclusion was that there the vblank may just happen
sooner or later. Nothing else should change. What's unclear is how
the refresh rate would be controlled. The two obvious options are
explicit control, and automagic based on the submit rate. But that's
a separate topic entirely.

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH v5 3/4] drm/fence: add fence timeline to drm_crtc

[Gustavo Padovan]

2016-10-20 Brian Starkey <brian.starkey@arm.com>:

> Hi Gustavo,
> 
> On Thu, Oct 20, 2016 at 12:50:04PM -0200, Gustavo Padovan wrote:
> > From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> > 
> > Create one timeline context for each CRTC to be able to handle out-fences
> > and signal them. It adds a few members to struct drm_crtc: fence_context,
> > where we store the context we get from fence_context_alloc(), the
> > fence seqno and the fence lock, that we pass in fence_init() to be
> > used by the fence.
> > 
> > v2: Comment by Daniel Stone:
> > 	- add BUG_ON() to fence_to_crtc() macro
> > 
> > v3: Comment by Ville Syrjälä
> > 	- Use more meaningful name as crtc timeline name
> > 
> > Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> > ---
> > drivers/gpu/drm/drm_crtc.c | 31 +++++++++++++++++++++++++++++++
> > include/drm/drm_crtc.h     | 19 +++++++++++++++++++
> > 2 files changed, 50 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> > index fcb6453..b99090f 100644
> > --- a/drivers/gpu/drm/drm_crtc.c
> > +++ b/drivers/gpu/drm/drm_crtc.c
> > @@ -165,6 +165,32 @@ static void drm_crtc_crc_fini(struct drm_crtc *crtc)
> > #endif
> > }
> > 
> > +static const char *drm_crtc_fence_get_driver_name(struct fence *fence)
> > +{
> > +	struct drm_crtc *crtc = fence_to_crtc(fence);
> > +
> > +	return crtc->dev->driver->name;
> > +}
> > +
> > +static const char *drm_crtc_fence_get_timeline_name(struct fence *fence)
> > +{
> > +	struct drm_crtc *crtc = fence_to_crtc(fence);
> > +
> > +	return crtc->timeline_name;
> > +}
> > +
> > +static bool drm_crtc_fence_enable_signaling(struct fence *fence)
> > +{
> > +	return true;
> > +}
> > +
> > +const struct fence_ops drm_crtc_fence_ops = {
> > +	.get_driver_name = drm_crtc_fence_get_driver_name,
> > +	.get_timeline_name = drm_crtc_fence_get_timeline_name,
> > +	.enable_signaling = drm_crtc_fence_enable_signaling,
> > +	.wait = fence_default_wait,
> > +};
> > +
> > /**
> >  * drm_crtc_init_with_planes - Initialise a new CRTC object with
> >  *    specified primary and cursor planes.
> > @@ -222,6 +248,11 @@ int drm_crtc_init_with_planes(struct drm_device *dev, struct drm_crtc *crtc,
> > 		return -ENOMEM;
> > 	}
> > 
> > +	crtc->fence_context = fence_context_alloc(1);
> > +	spin_lock_init(&crtc->fence_lock);
> > +	snprintf(crtc->timeline_name, sizeof(crtc->timeline_name),
> > +		 "drm_crtc-%d", crtc->base.id);
> 
> I wondered about "[CRTC:id:name]" to be consistent with the DRM debug
> prints.

Yeah, sounds good to me.

> 
> > +
> > 	crtc->base.properties = &crtc->properties;
> > 
> > 	list_add_tail(&crtc->head, &config->crtc_list);
> > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> > index 279132e..657a33a 100644
> > --- a/include/drm/drm_crtc.h
> > +++ b/include/drm/drm_crtc.h
> > @@ -32,6 +32,8 @@
> > #include <linux/fb.h>
> > #include <linux/hdmi.h>
> > #include <linux/media-bus-format.h>
> > +#include <linux/srcu.h>
> > +#include <linux/fence.h>
> > #include <uapi/drm/drm_mode.h>
> > #include <uapi/drm/drm_fourcc.h>
> > #include <drm/drm_modeset_lock.h>
> > @@ -618,6 +620,9 @@ struct drm_crtc_funcs {
> >  * @gamma_store: gamma ramp values
> >  * @helper_private: mid-layer private data
> >  * @properties: property tracking for this CRTC
> > + * @fence_context: context for fence signalling
> > + * @fence_lock: fence lock for the fence context
> > + * @fence_seqno: seqno variable to create fences
> 
> @timeline_name ?

Sure.

Gustavo

Re: [PATCH v5 4/4] drm/fence: add out-fences support

[Gustavo Padovan]

Hi Brian,

2016-10-20 Brian Starkey <brian.starkey@arm.com>:

> Hi Gustavo,
> 
> I notice your branch has the sync_file refcount change in, but this
> doesn't seem to take account for that. Will you be dropping that
> change to match the semantics of fence_array?

I will drop the fence_get() in the out-fence patch because we already
hold the ref when we create the fence. The sync_file refcount patch will
remain.

> 
> Couple more comments below.
> 
> On Thu, Oct 20, 2016 at 12:50:05PM -0200, Gustavo Padovan wrote:
> > From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> > 
> > Support DRM out-fences by creating a sync_file with a fence for each CRTC
> > that sets the OUT_FENCE_PTR property.
> > 
> > We use the out_fence pointer received in the OUT_FENCE_PTR prop to send
> > the sync_file fd back to userspace.
> > 
> > The sync_file and fd are allocated/created before commit, but the
> > fd_install operation only happens after we know that commit succeed.
> > 
> > In case of error userspace should receive a fd number of -1.
> > 
> > v2: Comment by Rob Clark:
> > 	- Squash commit that adds DRM_MODE_ATOMIC_OUT_FENCE flag here.
> > 
> >    Comment by Daniel Vetter:
> > 	- Add clean up code for out_fences
> > 
> > v3: Comments by Daniel Vetter:
> > 	- create DRM_MODE_ATOMIC_EVENT_MASK
> > 	- userspace should fill out_fences_ptr with the crtc_ids for which
> > 	it wants fences back.
> > 
> > v4: Create OUT_FENCE_PTR properties and remove old approach.
> > 
> > Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> > ---
> > drivers/gpu/drm/drm_atomic.c | 116 ++++++++++++++++++++++++++++++++++---------
> > drivers/gpu/drm/drm_crtc.c   |   8 +++
> > include/drm/drm_crtc.h       |  19 +++++++
> > 3 files changed, 119 insertions(+), 24 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> > index b3284b2..07775fc 100644
> > --- a/drivers/gpu/drm/drm_atomic.c
> > +++ b/drivers/gpu/drm/drm_atomic.c
> > @@ -490,6 +490,8 @@ int drm_atomic_crtc_set_property(struct drm_crtc *crtc,
> > 					&replaced);
> > 		state->color_mgmt_changed |= replaced;
> > 		return ret;
> > +	} else if (property == config->prop_out_fence_ptr) {
> > +		state->out_fence_ptr = u64_to_user_ptr(val);
> > 	} else if (crtc->funcs->atomic_set_property)
> > 		return crtc->funcs->atomic_set_property(crtc, state, property, val);
> > 	else
> > @@ -532,6 +534,8 @@ drm_atomic_crtc_get_property(struct drm_crtc *crtc,
> > 		*val = (state->ctm) ? state->ctm->base.id : 0;
> > 	else if (property == config->gamma_lut_property)
> > 		*val = (state->gamma_lut) ? state->gamma_lut->base.id : 0;
> > +	else if (property == config->prop_out_fence_ptr)
> > +		*val = 0;
> > 	else if (crtc->funcs->atomic_get_property)
> > 		return crtc->funcs->atomic_get_property(crtc, state, property, val);
> > 	else
> > @@ -1474,11 +1478,9 @@ EXPORT_SYMBOL(drm_atomic_nonblocking_commit);
> >  */
> > 
> > static struct drm_pending_vblank_event *create_vblank_event(
> > -		struct drm_device *dev, struct drm_file *file_priv,
> > -		struct fence *fence, uint64_t user_data)
> > +		struct drm_device *dev, uint64_t user_data)
> > {
> > 	struct drm_pending_vblank_event *e = NULL;
> > -	int ret;
> > 
> > 	e = kzalloc(sizeof *e, GFP_KERNEL);
> > 	if (!e)
> > @@ -1488,17 +1490,6 @@ static struct drm_pending_vblank_event *create_vblank_event(
> > 	e->event.base.length = sizeof(e->event);
> > 	e->event.user_data = user_data;
> > 
> > -	if (file_priv) {
> > -		ret = drm_event_reserve_init(dev, file_priv, &e->base,
> > -					     &e->event.base);
> > -		if (ret) {
> > -			kfree(e);
> > -			return NULL;
> > -		}
> > -	}
> > -
> > -	e->base.fence = fence;
> > -
> > 	return e;
> > }
> > 
> > @@ -1603,6 +1594,40 @@ void drm_atomic_clean_old_fb(struct drm_device *dev,
> > }
> > EXPORT_SYMBOL(drm_atomic_clean_old_fb);
> > 
> > +static int crtc_setup_out_fence(struct drm_crtc *crtc,
> > +				struct drm_crtc_state *crtc_state,
> > +				struct drm_out_fence_state *fence_state)
> > +{
> > +	struct fence *fence;
> > +
> > +	fence_state->fd = get_unused_fd_flags(O_CLOEXEC);
> > +	if (fence_state->fd < 0) {
> > +		return fence_state->fd;
> > +	}
> > +
> > +	fence_state->out_fence_ptr = crtc_state->out_fence_ptr;
> > +	crtc_state->out_fence_ptr = NULL;
> > +
> > +	if (put_user(fence_state->fd, fence_state->out_fence_ptr))
> > +		return -EFAULT;
> > +
> > +	fence = kzalloc(sizeof(*fence), GFP_KERNEL);
> > +	if (!fence)
> > +		return -ENOMEM;
> > +
> > +	fence_init(fence, &drm_crtc_fence_ops, &crtc->fence_lock,
> > +		   crtc->fence_context, ++crtc->fence_seqno);
> > +
> > +	fence_state->sync_file = sync_file_create(fence);
> > +	if(!fence_state->sync_file) {
> > +		fence_put(fence);
> > +		return -ENOMEM;
> > +	}
> > +
> > +	crtc_state->event->base.fence = fence_get(fence);
> > +	return 0;
> > +}
> > +
> > int drm_mode_atomic_ioctl(struct drm_device *dev,
> > 			  void *data, struct drm_file *file_priv)
> > {
> > @@ -1617,9 +1642,10 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
> > 	struct drm_plane *plane;
> > 	struct drm_crtc *crtc;
> > 	struct drm_crtc_state *crtc_state;
> > +	struct drm_out_fence_state *fence_state;
> > 	unsigned plane_mask;
> > 	int ret = 0;
> > -	unsigned int i, j;
> > +	unsigned int i, j, fence_idx = 0;
> > 
> > 	/* disallow for drivers not supporting atomic: */
> > 	if (!drm_core_check_feature(dev, DRIVER_ATOMIC))
> > @@ -1734,12 +1760,19 @@ retry:
> > 		drm_mode_object_unreference(obj);
> > 	}
> > 
> > -	if (arg->flags & DRM_MODE_PAGE_FLIP_EVENT) {
> > -		for_each_crtc_in_state(state, crtc, crtc_state, i) {
> > +	fence_state = kcalloc(dev->mode_config.num_crtc, sizeof(*fence_state),
> > +			      GFP_KERNEL);
> > +	if (!fence_state) {
> > +		ret = -ENOMEM;
> > +		goto out;
> > +	}
> > +
> > +	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> > +		if (arg->flags & DRM_MODE_PAGE_FLIP_EVENT ||
> > +		    crtc_state->out_fence_ptr) {
> > 			struct drm_pending_vblank_event *e;
> > 
> > -			e = create_vblank_event(dev, file_priv, NULL,
> > -						arg->user_data);
> > +			e = create_vblank_event(dev, arg->user_data);
> > 			if (!e) {
> > 				ret = -ENOMEM;
> > 				goto out;
> > @@ -1747,6 +1780,28 @@ retry:
> > 
> > 			crtc_state->event = e;
> > 		}
> > +
> > +		if (arg->flags & DRM_MODE_PAGE_FLIP_EVENT) {
> > +			struct drm_pending_vblank_event *e = crtc_state->event;
> > +
> > +			if (!file_priv)
> > +				continue;
> > +
> > +			ret = drm_event_reserve_init(dev, file_priv, &e->base,
> > +						     &e->event.base);
> > +			if (ret) {
> > +				kfree(e);
> > +				crtc_state->event = NULL;
> > +				goto out;
> > +			}
> > +		}
> > +
> > +		if (crtc_state->out_fence_ptr) {
> > +			ret = crtc_setup_out_fence(crtc, crtc_state,
> > +						   &fence_state[fence_idx++]);
> > +			if (ret)
> > +				goto out;
> > +		}
> > 	}
> > 
> > 	if (arg->flags & DRM_MODE_ATOMIC_TEST_ONLY) {
> > @@ -1761,24 +1816,37 @@ retry:
> > 		ret = drm_atomic_commit(state);
> > 	}
> > 
> > +	for (i = 0; i < fence_idx; i++)
> > +		fd_install(fence_state[i].fd, fence_state[i].sync_file->file);
> > +
> > out:
> > 	drm_atomic_clean_old_fb(dev, plane_mask, ret);
> > 
> > -	if (ret && arg->flags & DRM_MODE_PAGE_FLIP_EVENT) {
> > +	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> 
> I think a check on ret is still needed before you iterate the CRTCs.
> If the commit is successful then state has already been freed by this
> point, and I get a splat.

Yes, I believe I only tested with non-blocking requests.

> 
> > 		/*
> > 		 * TEST_ONLY and PAGE_FLIP_EVENT are mutually exclusive,
> > 		 * if they weren't, this code should be called on success
> > 		 * for TEST_ONLY too.
> > 		 */
> > +		if (ret && crtc_state->event)
> > +			drm_event_cancel_free(dev, &crtc_state->event->base);
> > +	}
> > 
> > -		for_each_crtc_in_state(state, crtc, crtc_state, i) {
> > -			if (!crtc_state->event)
> > -				continue;
> > +	if (ret && fence_state) {
> > +		for (i = 0; i < fence_idx; i++) {
> > +			if (fence_state[i].fd >= 0)
> > +				put_unused_fd(fence_state[i].fd);
> > +			if (fence_state[i].sync_file)
> > +				fput(fence_state[i].sync_file->file);
> > 
> > -			drm_event_cancel_free(dev, &crtc_state->event->base);
> > +			/* If this fails, there's not much we can do about it */
> > +			if (put_user(-1, fence_state->out_fence_ptr))
> > +				DRM_ERROR("Couldn't clear out_fence_ptr\n");
> > 		}
> > 	}
> > 
> > +	kfree(fence_state);
> > +
> > 	if (ret == -EDEADLK) {
> > 		drm_atomic_state_clear(state);
> > 		drm_modeset_backoff(&ctx);
> > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> > index b99090f..40bce97 100644
> > --- a/drivers/gpu/drm/drm_crtc.c
> > +++ b/drivers/gpu/drm/drm_crtc.c
> > @@ -274,6 +274,8 @@ int drm_crtc_init_with_planes(struct drm_device *dev, struct drm_crtc *crtc,
> > 	if (drm_core_check_feature(dev, DRIVER_ATOMIC)) {
> > 		drm_object_attach_property(&crtc->base, config->prop_active, 0);
> > 		drm_object_attach_property(&crtc->base, config->prop_mode_id, 0);
> > +		drm_object_attach_property(&crtc->base,
> > +					   config->prop_out_fence_ptr, 0);
> > 	}
> > 
> > 	return 0;
> > @@ -434,6 +436,12 @@ static int drm_mode_create_standard_properties(struct drm_device *dev)
> > 		return -ENOMEM;
> > 	dev->mode_config.prop_in_fence_fd = prop;
> > 
> > +	prop = drm_property_create_range(dev, DRM_MODE_PROP_ATOMIC,
> > +			"OUT_FENCE_PTR", 0, U64_MAX);
> > +	if (!prop)
> > +		return -ENOMEM;
> > +	dev->mode_config.prop_out_fence_ptr = prop;
> > +
> > 	prop = drm_property_create_object(dev, DRM_MODE_PROP_ATOMIC,
> > 			"CRTC_ID", DRM_MODE_OBJECT_CRTC);
> > 	if (!prop)
> > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> > index 657a33a..b898604 100644
> > --- a/include/drm/drm_crtc.h
> > +++ b/include/drm/drm_crtc.h
> > @@ -165,6 +165,8 @@ struct drm_crtc_state {
> > 	struct drm_property_blob *ctm;
> > 	struct drm_property_blob *gamma_lut;
> > 
> > +	u64 __user *out_fence_ptr;
> > +
> 
> I'm somewhat not convinced about stashing a __user pointer in the
> CRTC atomic state. I know it gets cleared before the driver sees it,
> but if anything I think that hints that this isn't the right place to
> store it.
> 
> I've been trying to think of other ways to get from a property to
> something drm_mode_atomic_ioctl can use - maybe we can store some
> stuff in drm_atomic_state which only lives for the length of the ioctl
> call and put this pointer in there.

The drm_atomic_state is still visible by the drivers. Between there and
crtc_state, I would keep in the crtc_state for now.

Gustavo

Re: [PATCH v5 4/4] drm/fence: add out-fences support

[Brian Starkey]

Hi Gustavo,

On Thu, Oct 20, 2016 at 06:30:17PM -0200, Gustavo Padovan wrote:
>Hi Brian,
>
>2016-10-20 Brian Starkey <brian.starkey@arm.com>:
>
>> Hi Gustavo,
>>
>> I notice your branch has the sync_file refcount change in, but this
>> doesn't seem to take account for that. Will you be dropping that
>> change to match the semantics of fence_array?
>
>I will drop the fence_get() in the out-fence patch because we already
>hold the ref when we create the fence. The sync_file refcount patch will
>remain.
>

OK, I'll work on that basis, thanks.

>>
>> Couple more comments below.
>>
>> On Thu, Oct 20, 2016 at 12:50:05PM -0200, Gustavo Padovan wrote:
>> > From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
>> >
>> > Support DRM out-fences by creating a sync_file with a fence for each CRTC
>> > that sets the OUT_FENCE_PTR property.
>> >
>> > We use the out_fence pointer received in the OUT_FENCE_PTR prop to send
>> > the sync_file fd back to userspace.
>> >
>> > The sync_file and fd are allocated/created before commit, but the
>> > fd_install operation only happens after we know that commit succeed.
>> >
>> > In case of error userspace should receive a fd number of -1.
>> >
>> > v2: Comment by Rob Clark:
>> > 	- Squash commit that adds DRM_MODE_ATOMIC_OUT_FENCE flag here.
>> >
>> >    Comment by Daniel Vetter:
>> > 	- Add clean up code for out_fences
>> >
>> > v3: Comments by Daniel Vetter:
>> > 	- create DRM_MODE_ATOMIC_EVENT_MASK
>> > 	- userspace should fill out_fences_ptr with the crtc_ids for which
>> > 	it wants fences back.
>> >
>> > v4: Create OUT_FENCE_PTR properties and remove old approach.
>> >
>> > Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
>> > ---
>> > drivers/gpu/drm/drm_atomic.c | 116 ++++++++++++++++++++++++++++++++++---------
>> > drivers/gpu/drm/drm_crtc.c   |   8 +++
>> > include/drm/drm_crtc.h       |  19 +++++++
>> > 3 files changed, 119 insertions(+), 24 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
>> > index b3284b2..07775fc 100644
>> > --- a/drivers/gpu/drm/drm_atomic.c
>> > +++ b/drivers/gpu/drm/drm_atomic.c
>> > @@ -490,6 +490,8 @@ int drm_atomic_crtc_set_property(struct drm_crtc *crtc,
>> > 					&replaced);
>> > 		state->color_mgmt_changed |= replaced;
>> > 		return ret;
>> > +	} else if (property == config->prop_out_fence_ptr) {
>> > +		state->out_fence_ptr = u64_to_user_ptr(val);
>> > 	} else if (crtc->funcs->atomic_set_property)
>> > 		return crtc->funcs->atomic_set_property(crtc, state, property, val);
>> > 	else
>> > @@ -532,6 +534,8 @@ drm_atomic_crtc_get_property(struct drm_crtc *crtc,
>> > 		*val = (state->ctm) ? state->ctm->base.id : 0;
>> > 	else if (property == config->gamma_lut_property)
>> > 		*val = (state->gamma_lut) ? state->gamma_lut->base.id : 0;
>> > +	else if (property == config->prop_out_fence_ptr)
>> > +		*val = 0;
>> > 	else if (crtc->funcs->atomic_get_property)
>> > 		return crtc->funcs->atomic_get_property(crtc, state, property, val);
>> > 	else
>> > @@ -1474,11 +1478,9 @@ EXPORT_SYMBOL(drm_atomic_nonblocking_commit);
>> >  */
>> >
>> > static struct drm_pending_vblank_event *create_vblank_event(
>> > -		struct drm_device *dev, struct drm_file *file_priv,
>> > -		struct fence *fence, uint64_t user_data)
>> > +		struct drm_device *dev, uint64_t user_data)
>> > {
>> > 	struct drm_pending_vblank_event *e = NULL;
>> > -	int ret;
>> >
>> > 	e = kzalloc(sizeof *e, GFP_KERNEL);
>> > 	if (!e)
>> > @@ -1488,17 +1490,6 @@ static struct drm_pending_vblank_event *create_vblank_event(
>> > 	e->event.base.length = sizeof(e->event);
>> > 	e->event.user_data = user_data;
>> >
>> > -	if (file_priv) {
>> > -		ret = drm_event_reserve_init(dev, file_priv, &e->base,
>> > -					     &e->event.base);
>> > -		if (ret) {
>> > -			kfree(e);
>> > -			return NULL;
>> > -		}
>> > -	}
>> > -
>> > -	e->base.fence = fence;
>> > -
>> > 	return e;
>> > }
>> >
>> > @@ -1603,6 +1594,40 @@ void drm_atomic_clean_old_fb(struct drm_device *dev,
>> > }
>> > EXPORT_SYMBOL(drm_atomic_clean_old_fb);
>> >
>> > +static int crtc_setup_out_fence(struct drm_crtc *crtc,
>> > +				struct drm_crtc_state *crtc_state,
>> > +				struct drm_out_fence_state *fence_state)
>> > +{
>> > +	struct fence *fence;
>> > +
>> > +	fence_state->fd = get_unused_fd_flags(O_CLOEXEC);
>> > +	if (fence_state->fd < 0) {
>> > +		return fence_state->fd;
>> > +	}
>> > +
>> > +	fence_state->out_fence_ptr = crtc_state->out_fence_ptr;
>> > +	crtc_state->out_fence_ptr = NULL;
>> > +
>> > +	if (put_user(fence_state->fd, fence_state->out_fence_ptr))
>> > +		return -EFAULT;
>> > +
>> > +	fence = kzalloc(sizeof(*fence), GFP_KERNEL);
>> > +	if (!fence)
>> > +		return -ENOMEM;
>> > +
>> > +	fence_init(fence, &drm_crtc_fence_ops, &crtc->fence_lock,
>> > +		   crtc->fence_context, ++crtc->fence_seqno);
>> > +
>> > +	fence_state->sync_file = sync_file_create(fence);
>> > +	if(!fence_state->sync_file) {
>> > +		fence_put(fence);
>> > +		return -ENOMEM;
>> > +	}
>> > +
>> > +	crtc_state->event->base.fence = fence_get(fence);
>> > +	return 0;
>> > +}
>> > +
>> > int drm_mode_atomic_ioctl(struct drm_device *dev,
>> > 			  void *data, struct drm_file *file_priv)
>> > {
>> > @@ -1617,9 +1642,10 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
>> > 	struct drm_plane *plane;
>> > 	struct drm_crtc *crtc;
>> > 	struct drm_crtc_state *crtc_state;
>> > +	struct drm_out_fence_state *fence_state;

I just hit another crash - looks like fence_state needs to be
initialised to NULL here, otherwise if property lookup/set fails we
kfree() a bad pointer.

>> > 	unsigned plane_mask;
>> > 	int ret = 0;
>> > -	unsigned int i, j;
>> > +	unsigned int i, j, fence_idx = 0;
>> >
>> > 	/* disallow for drivers not supporting atomic: */
>> > 	if (!drm_core_check_feature(dev, DRIVER_ATOMIC))
>> > @@ -1734,12 +1760,19 @@ retry:
>> > 		drm_mode_object_unreference(obj);
>> > 	}
>> >
>> > -	if (arg->flags & DRM_MODE_PAGE_FLIP_EVENT) {
>> > -		for_each_crtc_in_state(state, crtc, crtc_state, i) {
>> > +	fence_state = kcalloc(dev->mode_config.num_crtc, sizeof(*fence_state),
>> > +			      GFP_KERNEL);
>> > +	if (!fence_state) {
>> > +		ret = -ENOMEM;
>> > +		goto out;
>> > +	}
>> > +
>> > +	for_each_crtc_in_state(state, crtc, crtc_state, i) {
>> > +		if (arg->flags & DRM_MODE_PAGE_FLIP_EVENT ||
>> > +		    crtc_state->out_fence_ptr) {
>> > 			struct drm_pending_vblank_event *e;
>> >
>> > -			e = create_vblank_event(dev, file_priv, NULL,
>> > -						arg->user_data);
>> > +			e = create_vblank_event(dev, arg->user_data);
>> > 			if (!e) {
>> > 				ret = -ENOMEM;
>> > 				goto out;
>> > @@ -1747,6 +1780,28 @@ retry:
>> >
>> > 			crtc_state->event = e;
>> > 		}
>> > +
>> > +		if (arg->flags & DRM_MODE_PAGE_FLIP_EVENT) {
>> > +			struct drm_pending_vblank_event *e = crtc_state->event;
>> > +
>> > +			if (!file_priv)
>> > +				continue;
>> > +
>> > +			ret = drm_event_reserve_init(dev, file_priv, &e->base,
>> > +						     &e->event.base);
>> > +			if (ret) {
>> > +				kfree(e);
>> > +				crtc_state->event = NULL;
>> > +				goto out;
>> > +			}
>> > +		}
>> > +
>> > +		if (crtc_state->out_fence_ptr) {
>> > +			ret = crtc_setup_out_fence(crtc, crtc_state,
>> > +						   &fence_state[fence_idx++]);
>> > +			if (ret)
>> > +				goto out;
>> > +		}
>> > 	}
>> >
>> > 	if (arg->flags & DRM_MODE_ATOMIC_TEST_ONLY) {
>> > @@ -1761,24 +1816,37 @@ retry:
>> > 		ret = drm_atomic_commit(state);
>> > 	}
>> >
>> > +	for (i = 0; i < fence_idx; i++)
>> > +		fd_install(fence_state[i].fd, fence_state[i].sync_file->file);
>> > +
>> > out:
>> > 	drm_atomic_clean_old_fb(dev, plane_mask, ret);
>> >
>> > -	if (ret && arg->flags & DRM_MODE_PAGE_FLIP_EVENT) {
>> > +	for_each_crtc_in_state(state, crtc, crtc_state, i) {
>>
>> I think a check on ret is still needed before you iterate the CRTCs.
>> If the commit is successful then state has already been freed by this
>> point, and I get a splat.
>
>Yes, I believe I only tested with non-blocking requests.
>

Right, I suppose you should get here before the free in that case.

>>
>> > 		/*
>> > 		 * TEST_ONLY and PAGE_FLIP_EVENT are mutually exclusive,
>> > 		 * if they weren't, this code should be called on success
>> > 		 * for TEST_ONLY too.
>> > 		 */
>> > +		if (ret && crtc_state->event)
>> > +			drm_event_cancel_free(dev, &crtc_state->event->base);
>> > +	}
>> >
>> > -		for_each_crtc_in_state(state, crtc, crtc_state, i) {
>> > -			if (!crtc_state->event)
>> > -				continue;
>> > +	if (ret && fence_state) {
>> > +		for (i = 0; i < fence_idx; i++) {
>> > +			if (fence_state[i].fd >= 0)
>> > +				put_unused_fd(fence_state[i].fd);
>> > +			if (fence_state[i].sync_file)
>> > +				fput(fence_state[i].sync_file->file);
>> >
>> > -			drm_event_cancel_free(dev, &crtc_state->event->base);
>> > +			/* If this fails, there's not much we can do about it */
>> > +			if (put_user(-1, fence_state->out_fence_ptr))
>> > +				DRM_ERROR("Couldn't clear out_fence_ptr\n");
>> > 		}
>> > 	}
>> >
>> > +	kfree(fence_state);
>> > +
>> > 	if (ret == -EDEADLK) {
>> > 		drm_atomic_state_clear(state);
>> > 		drm_modeset_backoff(&ctx);
>> > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
>> > index b99090f..40bce97 100644
>> > --- a/drivers/gpu/drm/drm_crtc.c
>> > +++ b/drivers/gpu/drm/drm_crtc.c
>> > @@ -274,6 +274,8 @@ int drm_crtc_init_with_planes(struct drm_device *dev, struct drm_crtc *crtc,
>> > 	if (drm_core_check_feature(dev, DRIVER_ATOMIC)) {
>> > 		drm_object_attach_property(&crtc->base, config->prop_active, 0);
>> > 		drm_object_attach_property(&crtc->base, config->prop_mode_id, 0);
>> > +		drm_object_attach_property(&crtc->base,
>> > +					   config->prop_out_fence_ptr, 0);
>> > 	}
>> >
>> > 	return 0;
>> > @@ -434,6 +436,12 @@ static int drm_mode_create_standard_properties(struct drm_device *dev)
>> > 		return -ENOMEM;
>> > 	dev->mode_config.prop_in_fence_fd = prop;
>> >
>> > +	prop = drm_property_create_range(dev, DRM_MODE_PROP_ATOMIC,
>> > +			"OUT_FENCE_PTR", 0, U64_MAX);
>> > +	if (!prop)
>> > +		return -ENOMEM;
>> > +	dev->mode_config.prop_out_fence_ptr = prop;
>> > +
>> > 	prop = drm_property_create_object(dev, DRM_MODE_PROP_ATOMIC,
>> > 			"CRTC_ID", DRM_MODE_OBJECT_CRTC);
>> > 	if (!prop)
>> > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
>> > index 657a33a..b898604 100644
>> > --- a/include/drm/drm_crtc.h
>> > +++ b/include/drm/drm_crtc.h
>> > @@ -165,6 +165,8 @@ struct drm_crtc_state {
>> > 	struct drm_property_blob *ctm;
>> > 	struct drm_property_blob *gamma_lut;
>> >
>> > +	u64 __user *out_fence_ptr;
>> > +
>>
>> I'm somewhat not convinced about stashing a __user pointer in the
>> CRTC atomic state. I know it gets cleared before the driver sees it,
>> but if anything I think that hints that this isn't the right place to
>> store it.
>>
>> I've been trying to think of other ways to get from a property to
>> something drm_mode_atomic_ioctl can use - maybe we can store some
>> stuff in drm_atomic_state which only lives for the length of the ioctl
>> call and put this pointer in there.
>
>The drm_atomic_state is still visible by the drivers. Between there and
>crtc_state, I would keep in the crtc_state for now.
>

Mm, yeah I suppose they could get to it if they went looking for it
in ->atomic_set_property or something, but I was thinking of freeing
it before the drm_atomic_commit.

Anyway, this way is certainly simpler, and setting it to NULL should
be enough to limit the damage a driver can do :-)

Thanks,
Brian

>Gustavo
>

Re: [PATCH v5 1/4] drm/fence: add in-fences support

[Daniel Vetter]

On Thu, Oct 20, 2016 at 12:50:02PM -0200, Gustavo Padovan wrote:
> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> 
> There is now a new property called FENCE_FD attached to every plane
> state that receives the sync_file fd from userspace via the atomic commit
> IOCTL.
> 
> The fd is then translated to a fence (that may be a fence_collection
> subclass or just a normal fence) and then used by DRM to fence_wait() for
> all fences in the sync_file to signal. So it only commits when all
> framebuffers are ready to scanout.
> 
> v2: Comments by Daniel Vetter:
> 	- remove set state->fence = NULL in destroy phase
> 	- accept fence -1 as valid and just return 0
> 	- do not call fence_get() - sync_file_fences_get() already calls it
> 	- fence_put() if state->fence is already set, in case userspace
> 	set the property more than once.
> 
> v3: WARN_ON if fence is set but state has no FB
> 
> v4: Comment from Maarten Lankhorst
> 	- allow set fence with no related fb
> 
> v5: rename FENCE_FD to IN_FENCE_FD
> 
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>

Since you rename plane_state->fence to in_fence (why exactly?) this breaks
compilation for imx and msm. Also there's the question of which fence
should win, if there's both explicit and implicit fences. I think the best
way to solve that is by adding a helper to set the implicit fence, which
drivers are supposed to call. That helper only sets the fence if there's
not yet an explicit fence attached. New sequence would be:

- patch1: roll out that helper (e.g. drm_atomic_plane_state_set_fence or
  whatever) for all drivers using it.
- patch2 (this one here): add explicit fences and change the helper to
  cooporate correctly.

> ---
>  drivers/gpu/drm/Kconfig             |  1 +
>  drivers/gpu/drm/drm_atomic.c        | 14 ++++++++++++++
>  drivers/gpu/drm/drm_atomic_helper.c | 13 +++++++------
>  drivers/gpu/drm/drm_crtc.c          |  6 ++++++
>  drivers/gpu/drm/drm_plane.c         |  1 +
>  include/drm/drm_crtc.h              |  5 +++++
>  include/drm/drm_plane.h             |  4 ++--
>  7 files changed, 36 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> index 483059a..43cb33d 100644
> --- a/drivers/gpu/drm/Kconfig
> +++ b/drivers/gpu/drm/Kconfig
> @@ -12,6 +12,7 @@ menuconfig DRM
>  	select I2C
>  	select I2C_ALGOBIT
>  	select DMA_SHARED_BUFFER
> +	select SYNC_FILE
>  	help
>  	  Kernel-level support for the Direct Rendering Infrastructure (DRI)
>  	  introduced in XFree86 4.0. If you say Y here, you need to select
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 5dd7054..b3284b2 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -30,6 +30,7 @@
>  #include <drm/drm_atomic.h>
>  #include <drm/drm_mode.h>
>  #include <drm/drm_plane_helper.h>
> +#include <linux/sync_file.h>
>  
>  #include "drm_crtc_internal.h"
>  
> @@ -686,6 +687,17 @@ int drm_atomic_plane_set_property(struct drm_plane *plane,
>  		drm_atomic_set_fb_for_plane(state, fb);
>  		if (fb)
>  			drm_framebuffer_unreference(fb);
> +	} else if (property == config->prop_in_fence_fd) {
> +		if (U642I64(val) == -1)
> +			return 0;
> +
> +		if (state->in_fence)
> +			fence_put(state->in_fence);
> +
> +		state->in_fence = sync_file_get_fence(val);
> +		if (!state->in_fence)
> +			return -EINVAL;
> +
>  	} else if (property == config->prop_crtc_id) {
>  		struct drm_crtc *crtc = drm_crtc_find(dev, val);
>  		return drm_atomic_set_crtc_for_plane(state, crtc);
> @@ -745,6 +757,8 @@ drm_atomic_plane_get_property(struct drm_plane *plane,
>  
>  	if (property == config->prop_fb_id) {
>  		*val = (state->fb) ? state->fb->base.id : 0;
> +	} else if (property == config->prop_in_fence_fd) {
> +		*val = -1;
>  	} else if (property == config->prop_crtc_id) {
>  		*val = (state->crtc) ? state->crtc->base.id : 0;
>  	} else if (property == config->prop_crtc_x) {
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 07b432f..73464b1 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -1031,22 +1031,20 @@ int drm_atomic_helper_wait_for_fences(struct drm_device *dev,
>  		if (!pre_swap)
>  			plane_state = plane->state;
>  
> -		if (!plane_state->fence)
> +		if (!plane_state->in_fence)
>  			continue;
>  
> -		WARN_ON(!plane_state->fb);
> -
>  		/*
>  		 * If waiting for fences pre-swap (ie: nonblock), userspace can
>  		 * still interrupt the operation. Instead of blocking until the
>  		 * timer expires, make the wait interruptible.
>  		 */
> -		ret = fence_wait(plane_state->fence, pre_swap);
> +		ret = fence_wait(plane_state->in_fence, pre_swap);
>  		if (ret)
>  			return ret;
>  
> -		fence_put(plane_state->fence);
> -		plane_state->fence = NULL;
> +		fence_put(plane_state->in_fence);
> +		plane_state->in_fence = NULL;
>  	}
>  
>  	return 0;
> @@ -3114,6 +3112,9 @@ void __drm_atomic_helper_plane_destroy_state(struct drm_plane_state *state)
>  {
>  	if (state->fb)
>  		drm_framebuffer_unreference(state->fb);
> +
> +	if (state->in_fence)
> +		fence_put(state->in_fence);
>  }
>  EXPORT_SYMBOL(__drm_atomic_helper_plane_destroy_state);
>  
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 60403bf..fcb6453 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -397,6 +397,12 @@ static int drm_mode_create_standard_properties(struct drm_device *dev)
>  		return -ENOMEM;
>  	dev->mode_config.prop_fb_id = prop;
>  
> +	prop = drm_property_create_signed_range(dev, DRM_MODE_PROP_ATOMIC,
> +			"IN_FENCE_FD", -1, INT_MAX);
> +	if (!prop)
> +		return -ENOMEM;
> +	dev->mode_config.prop_in_fence_fd = prop;
> +
>  	prop = drm_property_create_object(dev, DRM_MODE_PROP_ATOMIC,
>  			"CRTC_ID", DRM_MODE_OBJECT_CRTC);
>  	if (!prop)
> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> index 249c0ae..3957ef8 100644
> --- a/drivers/gpu/drm/drm_plane.c
> +++ b/drivers/gpu/drm/drm_plane.c
> @@ -137,6 +137,7 @@ int drm_universal_plane_init(struct drm_device *dev, struct drm_plane *plane,
>  
>  	if (drm_core_check_feature(dev, DRIVER_ATOMIC)) {
>  		drm_object_attach_property(&plane->base, config->prop_fb_id, 0);
> +		drm_object_attach_property(&plane->base, config->prop_in_fence_fd, -1);
>  		drm_object_attach_property(&plane->base, config->prop_crtc_id, 0);
>  		drm_object_attach_property(&plane->base, config->prop_crtc_x, 0);
>  		drm_object_attach_property(&plane->base, config->prop_crtc_y, 0);
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 284c1b3..279132e 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -1206,6 +1206,11 @@ struct drm_mode_config {
>  	 */
>  	struct drm_property *prop_fb_id;
>  	/**
> +	 * @prop_in_fence_fd: Sync File fd representing the incoming fences
> +	 * for a Plane.
> +	 */
> +	struct drm_property *prop_in_fence_fd;
> +	/**
>  	 * @prop_crtc_id: Default atomic plane property to specify the
>  	 * &drm_crtc.
>  	 */
> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
> index 0235390..3cb9bf1 100644
> --- a/include/drm/drm_plane.h
> +++ b/include/drm/drm_plane.h
> @@ -34,7 +34,7 @@ struct drm_crtc;
>   * @plane: backpointer to the plane
>   * @crtc: currently bound CRTC, NULL if disabled
>   * @fb: currently bound framebuffer
> - * @fence: optional fence to wait for before scanning out @fb
> + * @in_fence: optional fence to wait for before scanning out @fb
>   * @crtc_x: left position of visible portion of plane on crtc
>   * @crtc_y: upper position of visible portion of plane on crtc
>   * @crtc_w: width of visible portion of plane on crtc
> @@ -59,7 +59,7 @@ struct drm_plane_state {
>  
>  	struct drm_crtc *crtc;   /* do not write directly, use drm_atomic_set_crtc_for_plane() */
>  	struct drm_framebuffer *fb;  /* do not write directly, use drm_atomic_set_fb_for_plane() */
> -	struct fence *fence;
> +	struct fence *in_fence;

I think it'd be good to move the kerneldoc into an in-line comment here
and expand a bit upon the expected semantics. This is a pretty important
part of the atomic uabi!

Otherwise looks all good.
-Daniel

>  
>  	/* Signed dest location allows it to be partially off screen */
>  	int32_t crtc_x, crtc_y;
> -- 
> 2.5.5
> 

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

Re: [PATCH v5 1/4] drm/fence: add in-fences support

[Daniel Vetter]

On Thu, Oct 20, 2016 at 12:50:02PM -0200, Gustavo Padovan wrote:
> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> 
> There is now a new property called FENCE_FD attached to every plane
> state that receives the sync_file fd from userspace via the atomic commit
> IOCTL.
> 
> The fd is then translated to a fence (that may be a fence_collection
> subclass or just a normal fence) and then used by DRM to fence_wait() for
> all fences in the sync_file to signal. So it only commits when all
> framebuffers are ready to scanout.
> 
> v2: Comments by Daniel Vetter:
> 	- remove set state->fence = NULL in destroy phase
> 	- accept fence -1 as valid and just return 0
> 	- do not call fence_get() - sync_file_fences_get() already calls it
> 	- fence_put() if state->fence is already set, in case userspace
> 	set the property more than once.
> 
> v3: WARN_ON if fence is set but state has no FB
> 
> v4: Comment from Maarten Lankhorst
> 	- allow set fence with no related fb
> 
> v5: rename FENCE_FD to IN_FENCE_FD
> 
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>

Since you rename plane_state->fence to in_fence (why exactly?) this breaks
compilation for imx and msm. Also there's the question of which fence
should win, if there's both explicit and implicit fences. I think the best
way to solve that is by adding a helper to set the implicit fence, which
drivers are supposed to call. That helper only sets the fence if there's
not yet an explicit fence attached. New sequence would be:

- patch1: roll out that helper (e.g. drm_atomic_plane_state_set_fence or
  whatever) for all drivers using it.
- patch2 (this one here): add explicit fences and change the helper to
  cooporate correctly.

> ---
>  drivers/gpu/drm/Kconfig             |  1 +
>  drivers/gpu/drm/drm_atomic.c        | 14 ++++++++++++++
>  drivers/gpu/drm/drm_atomic_helper.c | 13 +++++++------
>  drivers/gpu/drm/drm_crtc.c          |  6 ++++++
>  drivers/gpu/drm/drm_plane.c         |  1 +
>  include/drm/drm_crtc.h              |  5 +++++
>  include/drm/drm_plane.h             |  4 ++--
>  7 files changed, 36 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> index 483059a..43cb33d 100644
> --- a/drivers/gpu/drm/Kconfig
> +++ b/drivers/gpu/drm/Kconfig
> @@ -12,6 +12,7 @@ menuconfig DRM
>  	select I2C
>  	select I2C_ALGOBIT
>  	select DMA_SHARED_BUFFER
> +	select SYNC_FILE
>  	help
>  	  Kernel-level support for the Direct Rendering Infrastructure (DRI)
>  	  introduced in XFree86 4.0. If you say Y here, you need to select
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 5dd7054..b3284b2 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -30,6 +30,7 @@
>  #include <drm/drm_atomic.h>
>  #include <drm/drm_mode.h>
>  #include <drm/drm_plane_helper.h>
> +#include <linux/sync_file.h>
>  
>  #include "drm_crtc_internal.h"
>  
> @@ -686,6 +687,17 @@ int drm_atomic_plane_set_property(struct drm_plane *plane,
>  		drm_atomic_set_fb_for_plane(state, fb);
>  		if (fb)
>  			drm_framebuffer_unreference(fb);
> +	} else if (property == config->prop_in_fence_fd) {
> +		if (U642I64(val) == -1)
> +			return 0;
> +
> +		if (state->in_fence)
> +			fence_put(state->in_fence);
> +
> +		state->in_fence = sync_file_get_fence(val);
> +		if (!state->in_fence)
> +			return -EINVAL;
> +
>  	} else if (property == config->prop_crtc_id) {
>  		struct drm_crtc *crtc = drm_crtc_find(dev, val);
>  		return drm_atomic_set_crtc_for_plane(state, crtc);
> @@ -745,6 +757,8 @@ drm_atomic_plane_get_property(struct drm_plane *plane,
>  
>  	if (property == config->prop_fb_id) {
>  		*val = (state->fb) ? state->fb->base.id : 0;
> +	} else if (property == config->prop_in_fence_fd) {
> +		*val = -1;
>  	} else if (property == config->prop_crtc_id) {
>  		*val = (state->crtc) ? state->crtc->base.id : 0;
>  	} else if (property == config->prop_crtc_x) {
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 07b432f..73464b1 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -1031,22 +1031,20 @@ int drm_atomic_helper_wait_for_fences(struct drm_device *dev,
>  		if (!pre_swap)
>  			plane_state = plane->state;
>  
> -		if (!plane_state->fence)
> +		if (!plane_state->in_fence)
>  			continue;
>  
> -		WARN_ON(!plane_state->fb);
> -
>  		/*
>  		 * If waiting for fences pre-swap (ie: nonblock), userspace can
>  		 * still interrupt the operation. Instead of blocking until the
>  		 * timer expires, make the wait interruptible.
>  		 */
> -		ret = fence_wait(plane_state->fence, pre_swap);
> +		ret = fence_wait(plane_state->in_fence, pre_swap);
>  		if (ret)
>  			return ret;
>  
> -		fence_put(plane_state->fence);
> -		plane_state->fence = NULL;
> +		fence_put(plane_state->in_fence);
> +		plane_state->in_fence = NULL;
>  	}
>  
>  	return 0;
> @@ -3114,6 +3112,9 @@ void __drm_atomic_helper_plane_destroy_state(struct drm_plane_state *state)
>  {
>  	if (state->fb)
>  		drm_framebuffer_unreference(state->fb);
> +
> +	if (state->in_fence)
> +		fence_put(state->in_fence);
>  }
>  EXPORT_SYMBOL(__drm_atomic_helper_plane_destroy_state);
>  
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 60403bf..fcb6453 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -397,6 +397,12 @@ static int drm_mode_create_standard_properties(struct drm_device *dev)
>  		return -ENOMEM;
>  	dev->mode_config.prop_fb_id = prop;
>  
> +	prop = drm_property_create_signed_range(dev, DRM_MODE_PROP_ATOMIC,
> +			"IN_FENCE_FD", -1, INT_MAX);
> +	if (!prop)
> +		return -ENOMEM;
> +	dev->mode_config.prop_in_fence_fd = prop;
> +
>  	prop = drm_property_create_object(dev, DRM_MODE_PROP_ATOMIC,
>  			"CRTC_ID", DRM_MODE_OBJECT_CRTC);
>  	if (!prop)
> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> index 249c0ae..3957ef8 100644
> --- a/drivers/gpu/drm/drm_plane.c
> +++ b/drivers/gpu/drm/drm_plane.c
> @@ -137,6 +137,7 @@ int drm_universal_plane_init(struct drm_device *dev, struct drm_plane *plane,
>  
>  	if (drm_core_check_feature(dev, DRIVER_ATOMIC)) {
>  		drm_object_attach_property(&plane->base, config->prop_fb_id, 0);
> +		drm_object_attach_property(&plane->base, config->prop_in_fence_fd, -1);
>  		drm_object_attach_property(&plane->base, config->prop_crtc_id, 0);
>  		drm_object_attach_property(&plane->base, config->prop_crtc_x, 0);
>  		drm_object_attach_property(&plane->base, config->prop_crtc_y, 0);
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 284c1b3..279132e 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -1206,6 +1206,11 @@ struct drm_mode_config {
>  	 */
>  	struct drm_property *prop_fb_id;
>  	/**
> +	 * @prop_in_fence_fd: Sync File fd representing the incoming fences
> +	 * for a Plane.
> +	 */
> +	struct drm_property *prop_in_fence_fd;
> +	/**
>  	 * @prop_crtc_id: Default atomic plane property to specify the
>  	 * &drm_crtc.
>  	 */
> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
> index 0235390..3cb9bf1 100644
> --- a/include/drm/drm_plane.h
> +++ b/include/drm/drm_plane.h
> @@ -34,7 +34,7 @@ struct drm_crtc;
>   * @plane: backpointer to the plane
>   * @crtc: currently bound CRTC, NULL if disabled
>   * @fb: currently bound framebuffer
> - * @fence: optional fence to wait for before scanning out @fb
> + * @in_fence: optional fence to wait for before scanning out @fb
>   * @crtc_x: left position of visible portion of plane on crtc
>   * @crtc_y: upper position of visible portion of plane on crtc
>   * @crtc_w: width of visible portion of plane on crtc
> @@ -59,7 +59,7 @@ struct drm_plane_state {
>  
>  	struct drm_crtc *crtc;   /* do not write directly, use drm_atomic_set_crtc_for_plane() */
>  	struct drm_framebuffer *fb;  /* do not write directly, use drm_atomic_set_fb_for_plane() */
> -	struct fence *fence;
> +	struct fence *in_fence;

I think it'd be good to move the kerneldoc into an in-line comment here
and expand a bit upon the expected semantics. This is a pretty important
part of the atomic uabi!

Otherwise looks all good.
-Daniel

>  
>  	/* Signed dest location allows it to be partially off screen */
>  	int32_t crtc_x, crtc_y;
> -- 
> 2.5.5
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v5 2/4] drm/fence: release fence reference when canceling event

[Daniel Vetter]

On Thu, Oct 20, 2016 at 12:50:03PM -0200, Gustavo Padovan wrote:
> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> 
> If the event gets canceled we also need to put away the fence
> reference it holds.
> 
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

I've broken my local dim scripts right now, so can't apply ;-)
-Daniel

> ---
>  drivers/gpu/drm/drm_fops.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c
> index e84faec..8bed5f4 100644
> --- a/drivers/gpu/drm/drm_fops.c
> +++ b/drivers/gpu/drm/drm_fops.c
> @@ -663,6 +663,10 @@ void drm_event_cancel_free(struct drm_device *dev,
>  		list_del(&p->pending_link);
>  	}
>  	spin_unlock_irqrestore(&dev->event_lock, flags);
> +
> +	if (p->fence)
> +		fence_put(p->fence);
> +
>  	kfree(p);
>  }
>  EXPORT_SYMBOL(drm_event_cancel_free);
> -- 
> 2.5.5
> 

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

Re: [PATCH v5 2/4] drm/fence: release fence reference when canceling event

[Daniel Vetter]

On Thu, Oct 20, 2016 at 12:50:03PM -0200, Gustavo Padovan wrote:
> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> 
> If the event gets canceled we also need to put away the fence
> reference it holds.
> 
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

I've broken my local dim scripts right now, so can't apply ;-)
-Daniel

> ---
>  drivers/gpu/drm/drm_fops.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c
> index e84faec..8bed5f4 100644
> --- a/drivers/gpu/drm/drm_fops.c
> +++ b/drivers/gpu/drm/drm_fops.c
> @@ -663,6 +663,10 @@ void drm_event_cancel_free(struct drm_device *dev,
>  		list_del(&p->pending_link);
>  	}
>  	spin_unlock_irqrestore(&dev->event_lock, flags);
> +
> +	if (p->fence)
> +		fence_put(p->fence);
> +
>  	kfree(p);
>  }
>  EXPORT_SYMBOL(drm_event_cancel_free);
> -- 
> 2.5.5
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v5 3/4] drm/fence: add fence timeline to drm_crtc

[Daniel Vetter]

On Thu, Oct 20, 2016 at 12:50:04PM -0200, Gustavo Padovan wrote:
> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> 
> Create one timeline context for each CRTC to be able to handle out-fences
> and signal them. It adds a few members to struct drm_crtc: fence_context,
> where we store the context we get from fence_context_alloc(), the
> fence seqno and the fence lock, that we pass in fence_init() to be
> used by the fence.
> 
> v2: Comment by Daniel Stone:
> 	- add BUG_ON() to fence_to_crtc() macro
> 
> v3: Comment by Ville Syrjälä
> 	- Use more meaningful name as crtc timeline name
> 
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> ---
>  drivers/gpu/drm/drm_crtc.c | 31 +++++++++++++++++++++++++++++++
>  include/drm/drm_crtc.h     | 19 +++++++++++++++++++
>  2 files changed, 50 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index fcb6453..b99090f 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -165,6 +165,32 @@ static void drm_crtc_crc_fini(struct drm_crtc *crtc)
>  #endif
>  }
>  
> +static const char *drm_crtc_fence_get_driver_name(struct fence *fence)
> +{
> +	struct drm_crtc *crtc = fence_to_crtc(fence);
> +
> +	return crtc->dev->driver->name;
> +}
> +
> +static const char *drm_crtc_fence_get_timeline_name(struct fence *fence)
> +{
> +	struct drm_crtc *crtc = fence_to_crtc(fence);
> +
> +	return crtc->timeline_name;
> +}
> +
> +static bool drm_crtc_fence_enable_signaling(struct fence *fence)
> +{
> +	return true;
> +}
> +
> +const struct fence_ops drm_crtc_fence_ops = {
> +	.get_driver_name = drm_crtc_fence_get_driver_name,
> +	.get_timeline_name = drm_crtc_fence_get_timeline_name,
> +	.enable_signaling = drm_crtc_fence_enable_signaling,
> +	.wait = fence_default_wait,
> +};
> +
>  /**
>   * drm_crtc_init_with_planes - Initialise a new CRTC object with
>   *    specified primary and cursor planes.
> @@ -222,6 +248,11 @@ int drm_crtc_init_with_planes(struct drm_device *dev, struct drm_crtc *crtc,
>  		return -ENOMEM;
>  	}
>  
> +	crtc->fence_context = fence_context_alloc(1);
> +	spin_lock_init(&crtc->fence_lock);
> +	snprintf(crtc->timeline_name, sizeof(crtc->timeline_name),
> +		 "drm_crtc-%d", crtc->base.id);
> +
>  	crtc->base.properties = &crtc->properties;
>  
>  	list_add_tail(&crtc->head, &config->crtc_list);
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 279132e..657a33a 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -32,6 +32,8 @@
>  #include <linux/fb.h>
>  #include <linux/hdmi.h>
>  #include <linux/media-bus-format.h>
> +#include <linux/srcu.h>
> +#include <linux/fence.h>
>  #include <uapi/drm/drm_mode.h>
>  #include <uapi/drm/drm_fourcc.h>
>  #include <drm/drm_modeset_lock.h>
> @@ -618,6 +620,9 @@ struct drm_crtc_funcs {
>   * @gamma_store: gamma ramp values
>   * @helper_private: mid-layer private data
>   * @properties: property tracking for this CRTC
> + * @fence_context: context for fence signalling
> + * @fence_lock: fence lock for the fence context
> + * @fence_seqno: seqno variable to create fences

For new stuff I much prefer in-line kerneldoc with structures, keeps the
comments much closer to the code ...
>   *
>   * Each CRTC may have one or more connectors associated with it.  This structure
>   * allows the CRTC to be controlled.
> @@ -726,8 +731,22 @@ struct drm_crtc {
>  	 */
>  	struct drm_crtc_crc crc;
>  #endif
> +
> +	/* fence timelines info for DRM out-fences */

... and avoids duplicated comments like this one here.

Otherwise lgtm.
-Daniel

> +	unsigned int fence_context;
> +	spinlock_t fence_lock;
> +	unsigned long fence_seqno;
> +	char timeline_name[32];
>  };
>  
> +extern const struct fence_ops drm_crtc_fence_ops;
> +
> +static inline struct drm_crtc *fence_to_crtc(struct fence *fence)
> +{
> +	BUG_ON(fence->ops != &drm_crtc_fence_ops);
> +	return container_of(fence->lock, struct drm_crtc, fence_lock);
> +}
> +
>  /**
>   * struct drm_mode_set - new values for a CRTC config change
>   * @fb: framebuffer to use for new config
> -- 
> 2.5.5
> 

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

Re: [PATCH v5 3/4] drm/fence: add fence timeline to drm_crtc

[Daniel Vetter]

On Thu, Oct 20, 2016 at 12:50:04PM -0200, Gustavo Padovan wrote:
> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> 
> Create one timeline context for each CRTC to be able to handle out-fences
> and signal them. It adds a few members to struct drm_crtc: fence_context,
> where we store the context we get from fence_context_alloc(), the
> fence seqno and the fence lock, that we pass in fence_init() to be
> used by the fence.
> 
> v2: Comment by Daniel Stone:
> 	- add BUG_ON() to fence_to_crtc() macro
> 
> v3: Comment by Ville Syrjälä
> 	- Use more meaningful name as crtc timeline name
> 
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> ---
>  drivers/gpu/drm/drm_crtc.c | 31 +++++++++++++++++++++++++++++++
>  include/drm/drm_crtc.h     | 19 +++++++++++++++++++
>  2 files changed, 50 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index fcb6453..b99090f 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -165,6 +165,32 @@ static void drm_crtc_crc_fini(struct drm_crtc *crtc)
>  #endif
>  }
>  
> +static const char *drm_crtc_fence_get_driver_name(struct fence *fence)
> +{
> +	struct drm_crtc *crtc = fence_to_crtc(fence);
> +
> +	return crtc->dev->driver->name;
> +}
> +
> +static const char *drm_crtc_fence_get_timeline_name(struct fence *fence)
> +{
> +	struct drm_crtc *crtc = fence_to_crtc(fence);
> +
> +	return crtc->timeline_name;
> +}
> +
> +static bool drm_crtc_fence_enable_signaling(struct fence *fence)
> +{
> +	return true;
> +}
> +
> +const struct fence_ops drm_crtc_fence_ops = {
> +	.get_driver_name = drm_crtc_fence_get_driver_name,
> +	.get_timeline_name = drm_crtc_fence_get_timeline_name,
> +	.enable_signaling = drm_crtc_fence_enable_signaling,
> +	.wait = fence_default_wait,
> +};
> +
>  /**
>   * drm_crtc_init_with_planes - Initialise a new CRTC object with
>   *    specified primary and cursor planes.
> @@ -222,6 +248,11 @@ int drm_crtc_init_with_planes(struct drm_device *dev, struct drm_crtc *crtc,
>  		return -ENOMEM;
>  	}
>  
> +	crtc->fence_context = fence_context_alloc(1);
> +	spin_lock_init(&crtc->fence_lock);
> +	snprintf(crtc->timeline_name, sizeof(crtc->timeline_name),
> +		 "drm_crtc-%d", crtc->base.id);
> +
>  	crtc->base.properties = &crtc->properties;
>  
>  	list_add_tail(&crtc->head, &config->crtc_list);
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 279132e..657a33a 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -32,6 +32,8 @@
>  #include <linux/fb.h>
>  #include <linux/hdmi.h>
>  #include <linux/media-bus-format.h>
> +#include <linux/srcu.h>
> +#include <linux/fence.h>
>  #include <uapi/drm/drm_mode.h>
>  #include <uapi/drm/drm_fourcc.h>
>  #include <drm/drm_modeset_lock.h>
> @@ -618,6 +620,9 @@ struct drm_crtc_funcs {
>   * @gamma_store: gamma ramp values
>   * @helper_private: mid-layer private data
>   * @properties: property tracking for this CRTC
> + * @fence_context: context for fence signalling
> + * @fence_lock: fence lock for the fence context
> + * @fence_seqno: seqno variable to create fences

For new stuff I much prefer in-line kerneldoc with structures, keeps the
comments much closer to the code ...
>   *
>   * Each CRTC may have one or more connectors associated with it.  This structure
>   * allows the CRTC to be controlled.
> @@ -726,8 +731,22 @@ struct drm_crtc {
>  	 */
>  	struct drm_crtc_crc crc;
>  #endif
> +
> +	/* fence timelines info for DRM out-fences */

... and avoids duplicated comments like this one here.

Otherwise lgtm.
-Daniel

> +	unsigned int fence_context;
> +	spinlock_t fence_lock;
> +	unsigned long fence_seqno;
> +	char timeline_name[32];
>  };
>  
> +extern const struct fence_ops drm_crtc_fence_ops;
> +
> +static inline struct drm_crtc *fence_to_crtc(struct fence *fence)
> +{
> +	BUG_ON(fence->ops != &drm_crtc_fence_ops);
> +	return container_of(fence->lock, struct drm_crtc, fence_lock);
> +}
> +
>  /**
>   * struct drm_mode_set - new values for a CRTC config change
>   * @fb: framebuffer to use for new config
> -- 
> 2.5.5
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v5 4/4] drm/fence: add out-fences support

[Daniel Vetter]

On Thu, Oct 20, 2016 at 10:15:20PM +0300, Ville Syrjälä wrote:
> On Thu, Oct 20, 2016 at 07:34:44PM +0300, Ville Syrjälä wrote:
> > On Thu, Oct 20, 2016 at 01:55:38PM -0200, Gustavo Padovan wrote:
> > > 2016-10-20 Ville Syrjälä <ville.syrjala@linux.intel.com>:
> > > 
> > > > On Thu, Oct 20, 2016 at 12:50:05PM -0200, Gustavo Padovan wrote:
> > > > > From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> > > > > 
> > > > > Support DRM out-fences by creating a sync_file with a fence for each CRTC
> > > > > that sets the OUT_FENCE_PTR property.
> > > > 
> > > > I still maintain the out fence should also be per fb (well, per plane
> > > > since we can't add it to fbs). Otherwise it's not going to be at all
> > > > useful if you do fps>vrefresh and don't include the same set of planes
> > > > in every atomic ioctl, eg. if you only ever include ones that are
> > > > somehow dirty.
> > > 
> > > How would the kernel signal these dirty planes then? Right now we signal
> > > at the vblank.
> > 
> > So if I think about it in terms of the previous fbs something like this
> > comes to mind:
> > 
> >  starting point:
> >  plane a, fb 0
> >  plane b, fb 1
> > 
> >  ioctl:
> >  plane a, fb 2, fence A
> >  plane b, fb 3, fence B
> > 
> >  ioctl:
> >  plane a, fb 4, fence C
> >  -> fb 2 can be reused, so fence C should signal immediately?
> > 
> >  vblank:
> >  -> fb 0,1 can be reused, so fence A,B signal?
> > 
> > It feels a bit wonky since the fence is effectively associated with the
> > previous fb after the previous fb was already submitted to the kernel.
> > One might assume fence A to be the one signalled early, but that would
> > give the impression that fb 0 would be free for reuse when it's not.
> 
> OK, so after hashing this out on irc with Gustavo and Brian, we came
> to the conclusion that the per-crtc out fence should be sufficient
> after all.
> 
> The way it can work is that the first ioctl will return the fence,
> doesn't really matter how many of the planes are involved in this
> ioctl. Subsequent ioctls that manage to get in before the next
> vblank will get back a -1 as the fence, even if the set if planes
> involved is not the same as in the first ioctl. From the -1
> userspace can tell what happened, and can then consult its own
> records to see which still pending flips were overwritten, and
> thus knows which buffers can be reused immediately.
> 
> If userspace plans on sending out the now free buffers to someone
> else accompanied by a fence, it can just create an already signalled
> fence to send instead of sending the fence it got back from the
> atomic ioctl since that one is still associated with the original
> scanout buffers.
> 
> The fence returned by the atomic ioctl will only signal after the
> vblank, at which point userspace will obviously know that the
> orginal scanout buffers are now also ready for reuse, and that
> the last buffer submitted for each plane is now being actively
> scanned out. So if userspace wants to send out some of the
> original buffers to someone else, it can send along the fence
> returned from the atomic ioctl.
> 
> So requires a bit more work from userspace perhaps. But obviously
> it only has to do this if it plans on submitting multiple operations
> per frame.
> 
> So a slightly expanded version of my previous example might look like:
> starting point:
>  plane a, fb 0
>  plane b, fb 1
> 
> ioctl:
>  crtc: fence A
>  plane a, fb 2
>  plane b, fb 3
> 
> ioctl:
>  crtc: fence -1
>  plane a, fb 4
>  -> the previously submitted fb for plane a (fb 2) can be reused
> 
> ioctl:
>  crtc: fence -1
>  plane a, fb 5
>  -> the previously submitted fb for plane a (fb 4) can be reused
> 
> vblank:
>  -> fence A signals, fb 0,1 can be reused
>     fb 3 and 5 being scanned out now
> 
> 
> We also had a quick side track w.r.t. variable refresh rate displays,
> and I think the conclusion was that there the vblank may just happen
> sooner or later. Nothing else should change. What's unclear is how
> the refresh rate would be controlled. The two obvious options are
> explicit control, and automagic based on the submit rate. But that's
> a separate topic entirely.

Thanks a lot for doing this discussion and the detailed write-up. Imo we
should bake this into the kerneldoc, as uabi documentation examples.
Gustavo, can you pls include this? I'd put it into the kerneldoc for
@out_fence, with inline struct comments we can be rather excessive in
their lenght ;-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH v5 4/4] drm/fence: add out-fences support

[Daniel Vetter]

On Thu, Oct 20, 2016 at 10:15:20PM +0300, Ville Syrjälä wrote:
> On Thu, Oct 20, 2016 at 07:34:44PM +0300, Ville Syrjälä wrote:
> > On Thu, Oct 20, 2016 at 01:55:38PM -0200, Gustavo Padovan wrote:
> > > 2016-10-20 Ville Syrjälä <ville.syrjala@linux.intel.com>:
> > > 
> > > > On Thu, Oct 20, 2016 at 12:50:05PM -0200, Gustavo Padovan wrote:
> > > > > From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> > > > > 
> > > > > Support DRM out-fences by creating a sync_file with a fence for each CRTC
> > > > > that sets the OUT_FENCE_PTR property.
> > > > 
> > > > I still maintain the out fence should also be per fb (well, per plane
> > > > since we can't add it to fbs). Otherwise it's not going to be at all
> > > > useful if you do fps>vrefresh and don't include the same set of planes
> > > > in every atomic ioctl, eg. if you only ever include ones that are
> > > > somehow dirty.
> > > 
> > > How would the kernel signal these dirty planes then? Right now we signal
> > > at the vblank.
> > 
> > So if I think about it in terms of the previous fbs something like this
> > comes to mind:
> > 
> >  starting point:
> >  plane a, fb 0
> >  plane b, fb 1
> > 
> >  ioctl:
> >  plane a, fb 2, fence A
> >  plane b, fb 3, fence B
> > 
> >  ioctl:
> >  plane a, fb 4, fence C
> >  -> fb 2 can be reused, so fence C should signal immediately?
> > 
> >  vblank:
> >  -> fb 0,1 can be reused, so fence A,B signal?
> > 
> > It feels a bit wonky since the fence is effectively associated with the
> > previous fb after the previous fb was already submitted to the kernel.
> > One might assume fence A to be the one signalled early, but that would
> > give the impression that fb 0 would be free for reuse when it's not.
> 
> OK, so after hashing this out on irc with Gustavo and Brian, we came
> to the conclusion that the per-crtc out fence should be sufficient
> after all.
> 
> The way it can work is that the first ioctl will return the fence,
> doesn't really matter how many of the planes are involved in this
> ioctl. Subsequent ioctls that manage to get in before the next
> vblank will get back a -1 as the fence, even if the set if planes
> involved is not the same as in the first ioctl. From the -1
> userspace can tell what happened, and can then consult its own
> records to see which still pending flips were overwritten, and
> thus knows which buffers can be reused immediately.
> 
> If userspace plans on sending out the now free buffers to someone
> else accompanied by a fence, it can just create an already signalled
> fence to send instead of sending the fence it got back from the
> atomic ioctl since that one is still associated with the original
> scanout buffers.
> 
> The fence returned by the atomic ioctl will only signal after the
> vblank, at which point userspace will obviously know that the
> orginal scanout buffers are now also ready for reuse, and that
> the last buffer submitted for each plane is now being actively
> scanned out. So if userspace wants to send out some of the
> original buffers to someone else, it can send along the fence
> returned from the atomic ioctl.
> 
> So requires a bit more work from userspace perhaps. But obviously
> it only has to do this if it plans on submitting multiple operations
> per frame.
> 
> So a slightly expanded version of my previous example might look like:
> starting point:
>  plane a, fb 0
>  plane b, fb 1
> 
> ioctl:
>  crtc: fence A
>  plane a, fb 2
>  plane b, fb 3
> 
> ioctl:
>  crtc: fence -1
>  plane a, fb 4
>  -> the previously submitted fb for plane a (fb 2) can be reused
> 
> ioctl:
>  crtc: fence -1
>  plane a, fb 5
>  -> the previously submitted fb for plane a (fb 4) can be reused
> 
> vblank:
>  -> fence A signals, fb 0,1 can be reused
>     fb 3 and 5 being scanned out now
> 
> 
> We also had a quick side track w.r.t. variable refresh rate displays,
> and I think the conclusion was that there the vblank may just happen
> sooner or later. Nothing else should change. What's unclear is how
> the refresh rate would be controlled. The two obvious options are
> explicit control, and automagic based on the submit rate. But that's
> a separate topic entirely.

Thanks a lot for doing this discussion and the detailed write-up. Imo we
should bake this into the kerneldoc, as uabi documentation examples.
Gustavo, can you pls include this? I'd put it into the kerneldoc for
@out_fence, with inline struct comments we can be rather excessive in
their lenght ;-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v5 4/4] drm/fence: add out-fences support

[Daniel Vetter]

On Fri, Oct 21, 2016 at 11:55:52AM +0100, Brian Starkey wrote:
> On Thu, Oct 20, 2016 at 06:30:17PM -0200, Gustavo Padovan wrote:
> > 2016-10-20 Brian Starkey <brian.starkey@arm.com>:
> > > > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> > > > index 657a33a..b898604 100644
> > > > --- a/include/drm/drm_crtc.h
> > > > +++ b/include/drm/drm_crtc.h
> > > > @@ -165,6 +165,8 @@ struct drm_crtc_state {
> > > > 	struct drm_property_blob *ctm;
> > > > 	struct drm_property_blob *gamma_lut;
> > > >
> > > > +	u64 __user *out_fence_ptr;
> > > > +
> > > 
> > > I'm somewhat not convinced about stashing a __user pointer in the
> > > CRTC atomic state. I know it gets cleared before the driver sees it,
> > > but if anything I think that hints that this isn't the right place to
> > > store it.
> > > 
> > > I've been trying to think of other ways to get from a property to
> > > something drm_mode_atomic_ioctl can use - maybe we can store some
> > > stuff in drm_atomic_state which only lives for the length of the ioctl
> > > call and put this pointer in there.
> > 
> > The drm_atomic_state is still visible by the drivers. Between there and
> > crtc_state, I would keep in the crtc_state for now.
> > 
> 
> Mm, yeah I suppose they could get to it if they went looking for it
> in ->atomic_set_property or something, but I was thinking of freeing
> it before the drm_atomic_commit.
> 
> Anyway, this way is certainly simpler, and setting it to NULL should
> be enough to limit the damage a driver can do :-)

+1 on moving this out of drm_crtc_state. drm_atomic_state already has
per-crtc structs, so easy to extend them with this. And yes drivers can
still see it, but mostly they're not supposed to touch drm_atomic_state
internals - the book-keeping is all done by the core.

The per-object state structs otoh are meant to be massively mangled by
drivers.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH v5 4/4] drm/fence: add out-fences support

[Daniel Vetter]

On Fri, Oct 21, 2016 at 11:55:52AM +0100, Brian Starkey wrote:
> On Thu, Oct 20, 2016 at 06:30:17PM -0200, Gustavo Padovan wrote:
> > 2016-10-20 Brian Starkey <brian.starkey@arm.com>:
> > > > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> > > > index 657a33a..b898604 100644
> > > > --- a/include/drm/drm_crtc.h
> > > > +++ b/include/drm/drm_crtc.h
> > > > @@ -165,6 +165,8 @@ struct drm_crtc_state {
> > > > 	struct drm_property_blob *ctm;
> > > > 	struct drm_property_blob *gamma_lut;
> > > >
> > > > +	u64 __user *out_fence_ptr;
> > > > +
> > > 
> > > I'm somewhat not convinced about stashing a __user pointer in the
> > > CRTC atomic state. I know it gets cleared before the driver sees it,
> > > but if anything I think that hints that this isn't the right place to
> > > store it.
> > > 
> > > I've been trying to think of other ways to get from a property to
> > > something drm_mode_atomic_ioctl can use - maybe we can store some
> > > stuff in drm_atomic_state which only lives for the length of the ioctl
> > > call and put this pointer in there.
> > 
> > The drm_atomic_state is still visible by the drivers. Between there and
> > crtc_state, I would keep in the crtc_state for now.
> > 
> 
> Mm, yeah I suppose they could get to it if they went looking for it
> in ->atomic_set_property or something, but I was thinking of freeing
> it before the drm_atomic_commit.
> 
> Anyway, this way is certainly simpler, and setting it to NULL should
> be enough to limit the damage a driver can do :-)

+1 on moving this out of drm_crtc_state. drm_atomic_state already has
per-crtc structs, so easy to extend them with this. And yes drivers can
still see it, but mostly they're not supposed to touch drm_atomic_state
internals - the book-keeping is all done by the core.

The per-object state structs otoh are meant to be massively mangled by
drivers.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v5 0/4] drm: add explicit fencing

[Daniel Vetter]

On Thu, Oct 20, 2016 at 12:50:01PM -0200, Gustavo Padovan wrote:
> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> 
> Hi,
> 
> Currently the Linux Kernel only have an implicit fencing mechanism, through 
> reservation objects, in which the fences are attached directly to buffers
> operations and userspace is unaware of what is happening. On the other hand
> explicit fencing exposes fences to the userspace to handle fencing between
> producer/consumer explicitely.
> 
> To support explicit fencing in the mainline kernel we de-staged the the needed
> parts of the Android Sync Framework[1] to be able to send and received fences
> to/from userspace. It has the concept of sync_file that exposes the driver's
> fences to userspace as file descriptors.
> 
> With explicit fencing we have a global mechanism that optimizes the flow of
> buffers between consumers and producers, avoiding a lot of waiting and some
> potential desktop freeze. So instead of waiting for a buffer to be processed by
> the GPU before sending it to DRM in an Atomic IOCTL we can get a sync_file fd
> from the GPU driver at the moment we submit the buffer processing. The
> compositor then passes these fds to DRM in an Atomic IOCTL, that will
> not be displayed until the fences signal, i.e, the GPU finished processing the
> buffer and it is ready to display. In DRM the fences we wait on before
> displaying a buffer are called in-fences and they are a per-plane deal.
> 
> Vice-versa, we have out-fences to sychronize the return of buffers to GPU
> (producer) to be processed again. When DRM receives an atomic request with a
> the OUT_FENCE_PTR property it create a fence attach it to a per-crtc
> sync_file. It then returns the sync_file fds to userspace. With the fences
> available userspace can forward these fences to the GPU, where it will wait the
> fence to signal before starting to process on buffer again. Out-fences are
> per-crtc.
> 
> While these are out-fences in DRM (the consumer) they become in-fences once
> they get to the GPU (the producer).
> 
> DRM explicit fences are opt-in, as the default will still be implicit fencing.
> 
> In-fences
> ---------
> 
> In the first discussions on #dri-devel on IRC we decided to hide the Sync
> Framework from DRM drivers to reduce complexity, so as soon we get the fd
> via IN_FENCE_FD plane property we convert the sync_file fd to a struct fence.
> However a sync_file might contain more than one fence, so we created the
> fence_array concept. struct fence_array is a subclass of struct
> fence and stores a group of fences that needs to be waited together.
> 
> Then we just use the already in place fence waiting support to wait on those
> fences. Once the producer calls fence_signal() for all fences on wait we can
> proceed with the atomic commit and display the framebuffers.
> 
> Out-fences
> ----------
> 
> Passing a pointer to OUT_FENCE_PTR property in an atomic request enables
> out-fences. The kernel then creates a fence, attach it to a sync_file and
> install this file on a unused fd for each crtc. The kernel writes the fd in
> the memory pointed by the out_fence_ptr provided. In case of error it writes -1.
> 
> DRM core use the already in place drm_event infrastructure to help signal
> fences, we've added a fence pointer to struct drm_pending_event. We signal
> signal fences when all the buffer related to that CRTC are *on* the screen.
> 
> Kernel tree
> -----------
> 
> For those who want all patches on this RFC are in my tree:
> 
> https://git.kernel.org/cgit/linux/kernel/git/padovan/linux.git/log/?h=fences
> 
> 
> v5 changes
> ----------
> 
> The changes from  v5 to v4 are in the patches description.
> 
> 
> Userspace
> ---------
> 
> Fences support on drm_hwcomposer is currently a working in progress.

Where are the igts? There's some fairly tricky error recovery and handling
code in there, I think we should have testcases for all of them. E.g.
out_fence property set, but then some invalid property later on to
exercise the error handling paths. Another one would be an atomic update
(maybe of a primary plane) which should work, except the fb is way too
small. That's checked by core code, but only at ->atomic_check phase, and
so could be used to exercise the even later error code.

Other things are mixing up in_fences, out_fences and events in different
ways, and making sure it all works out. And then maybe also mix in
nonblocking commit vs. blocking commit.

If you need something to generate fences: vgem has them. Chris Wilson can
point you at the code he's done in igt for testing the implicit fence
support in i915.

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

Re: [PATCH v5 0/4] drm: add explicit fencing

[Daniel Vetter]

On Thu, Oct 20, 2016 at 12:50:01PM -0200, Gustavo Padovan wrote:
> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> 
> Hi,
> 
> Currently the Linux Kernel only have an implicit fencing mechanism, through 
> reservation objects, in which the fences are attached directly to buffers
> operations and userspace is unaware of what is happening. On the other hand
> explicit fencing exposes fences to the userspace to handle fencing between
> producer/consumer explicitely.
> 
> To support explicit fencing in the mainline kernel we de-staged the the needed
> parts of the Android Sync Framework[1] to be able to send and received fences
> to/from userspace. It has the concept of sync_file that exposes the driver's
> fences to userspace as file descriptors.
> 
> With explicit fencing we have a global mechanism that optimizes the flow of
> buffers between consumers and producers, avoiding a lot of waiting and some
> potential desktop freeze. So instead of waiting for a buffer to be processed by
> the GPU before sending it to DRM in an Atomic IOCTL we can get a sync_file fd
> from the GPU driver at the moment we submit the buffer processing. The
> compositor then passes these fds to DRM in an Atomic IOCTL, that will
> not be displayed until the fences signal, i.e, the GPU finished processing the
> buffer and it is ready to display. In DRM the fences we wait on before
> displaying a buffer are called in-fences and they are a per-plane deal.
> 
> Vice-versa, we have out-fences to sychronize the return of buffers to GPU
> (producer) to be processed again. When DRM receives an atomic request with a
> the OUT_FENCE_PTR property it create a fence attach it to a per-crtc
> sync_file. It then returns the sync_file fds to userspace. With the fences
> available userspace can forward these fences to the GPU, where it will wait the
> fence to signal before starting to process on buffer again. Out-fences are
> per-crtc.
> 
> While these are out-fences in DRM (the consumer) they become in-fences once
> they get to the GPU (the producer).
> 
> DRM explicit fences are opt-in, as the default will still be implicit fencing.
> 
> In-fences
> ---------
> 
> In the first discussions on #dri-devel on IRC we decided to hide the Sync
> Framework from DRM drivers to reduce complexity, so as soon we get the fd
> via IN_FENCE_FD plane property we convert the sync_file fd to a struct fence.
> However a sync_file might contain more than one fence, so we created the
> fence_array concept. struct fence_array is a subclass of struct
> fence and stores a group of fences that needs to be waited together.
> 
> Then we just use the already in place fence waiting support to wait on those
> fences. Once the producer calls fence_signal() for all fences on wait we can
> proceed with the atomic commit and display the framebuffers.
> 
> Out-fences
> ----------
> 
> Passing a pointer to OUT_FENCE_PTR property in an atomic request enables
> out-fences. The kernel then creates a fence, attach it to a sync_file and
> install this file on a unused fd for each crtc. The kernel writes the fd in
> the memory pointed by the out_fence_ptr provided. In case of error it writes -1.
> 
> DRM core use the already in place drm_event infrastructure to help signal
> fences, we've added a fence pointer to struct drm_pending_event. We signal
> signal fences when all the buffer related to that CRTC are *on* the screen.
> 
> Kernel tree
> -----------
> 
> For those who want all patches on this RFC are in my tree:
> 
> https://git.kernel.org/cgit/linux/kernel/git/padovan/linux.git/log/?h=fences
> 
> 
> v5 changes
> ----------
> 
> The changes from  v5 to v4 are in the patches description.
> 
> 
> Userspace
> ---------
> 
> Fences support on drm_hwcomposer is currently a working in progress.

Where are the igts? There's some fairly tricky error recovery and handling
code in there, I think we should have testcases for all of them. E.g.
out_fence property set, but then some invalid property later on to
exercise the error handling paths. Another one would be an atomic update
(maybe of a primary plane) which should work, except the fb is way too
small. That's checked by core code, but only at ->atomic_check phase, and
so could be used to exercise the even later error code.

Other things are mixing up in_fences, out_fences and events in different
ways, and making sure it all works out. And then maybe also mix in
nonblocking commit vs. blocking commit.

If you need something to generate fences: vgem has them. Chris Wilson can
point you at the code he's done in igt for testing the implicit fence
support in i915.

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v5 4/4] drm/fence: add out-fences support

[Gustavo Padovan]

2016-10-21 Daniel Vetter <daniel@ffwll.ch>:

> On Thu, Oct 20, 2016 at 10:15:20PM +0300, Ville Syrjälä wrote:
> > On Thu, Oct 20, 2016 at 07:34:44PM +0300, Ville Syrjälä wrote:
> > > On Thu, Oct 20, 2016 at 01:55:38PM -0200, Gustavo Padovan wrote:
> > > > 2016-10-20 Ville Syrjälä <ville.syrjala@linux.intel.com>:
> > > > 
> > > > > On Thu, Oct 20, 2016 at 12:50:05PM -0200, Gustavo Padovan wrote:
> > > > > > From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> > > > > > 
> > > > > > Support DRM out-fences by creating a sync_file with a fence for each CRTC
> > > > > > that sets the OUT_FENCE_PTR property.
> > > > > 
> > > > > I still maintain the out fence should also be per fb (well, per plane
> > > > > since we can't add it to fbs). Otherwise it's not going to be at all
> > > > > useful if you do fps>vrefresh and don't include the same set of planes
> > > > > in every atomic ioctl, eg. if you only ever include ones that are
> > > > > somehow dirty.
> > > > 
> > > > How would the kernel signal these dirty planes then? Right now we signal
> > > > at the vblank.
> > > 
> > > So if I think about it in terms of the previous fbs something like this
> > > comes to mind:
> > > 
> > >  starting point:
> > >  plane a, fb 0
> > >  plane b, fb 1
> > > 
> > >  ioctl:
> > >  plane a, fb 2, fence A
> > >  plane b, fb 3, fence B
> > > 
> > >  ioctl:
> > >  plane a, fb 4, fence C
> > >  -> fb 2 can be reused, so fence C should signal immediately?
> > > 
> > >  vblank:
> > >  -> fb 0,1 can be reused, so fence A,B signal?
> > > 
> > > It feels a bit wonky since the fence is effectively associated with the
> > > previous fb after the previous fb was already submitted to the kernel.
> > > One might assume fence A to be the one signalled early, but that would
> > > give the impression that fb 0 would be free for reuse when it's not.
> > 
> > OK, so after hashing this out on irc with Gustavo and Brian, we came
> > to the conclusion that the per-crtc out fence should be sufficient
> > after all.
> > 
> > The way it can work is that the first ioctl will return the fence,
> > doesn't really matter how many of the planes are involved in this
> > ioctl. Subsequent ioctls that manage to get in before the next
> > vblank will get back a -1 as the fence, even if the set if planes
> > involved is not the same as in the first ioctl. From the -1
> > userspace can tell what happened, and can then consult its own
> > records to see which still pending flips were overwritten, and
> > thus knows which buffers can be reused immediately.
> > 
> > If userspace plans on sending out the now free buffers to someone
> > else accompanied by a fence, it can just create an already signalled
> > fence to send instead of sending the fence it got back from the
> > atomic ioctl since that one is still associated with the original
> > scanout buffers.
> > 
> > The fence returned by the atomic ioctl will only signal after the
> > vblank, at which point userspace will obviously know that the
> > orginal scanout buffers are now also ready for reuse, and that
> > the last buffer submitted for each plane is now being actively
> > scanned out. So if userspace wants to send out some of the
> > original buffers to someone else, it can send along the fence
> > returned from the atomic ioctl.
> > 
> > So requires a bit more work from userspace perhaps. But obviously
> > it only has to do this if it plans on submitting multiple operations
> > per frame.
> > 
> > So a slightly expanded version of my previous example might look like:
> > starting point:
> >  plane a, fb 0
> >  plane b, fb 1
> > 
> > ioctl:
> >  crtc: fence A
> >  plane a, fb 2
> >  plane b, fb 3
> > 
> > ioctl:
> >  crtc: fence -1
> >  plane a, fb 4
> >  -> the previously submitted fb for plane a (fb 2) can be reused
> > 
> > ioctl:
> >  crtc: fence -1
> >  plane a, fb 5
> >  -> the previously submitted fb for plane a (fb 4) can be reused
> > 
> > vblank:
> >  -> fence A signals, fb 0,1 can be reused
> >     fb 3 and 5 being scanned out now
> > 
> > 
> > We also had a quick side track w.r.t. variable refresh rate displays,
> > and I think the conclusion was that there the vblank may just happen
> > sooner or later. Nothing else should change. What's unclear is how
> > the refresh rate would be controlled. The two obvious options are
> > explicit control, and automagic based on the submit rate. But that's
> > a separate topic entirely.
> 
> Thanks a lot for doing this discussion and the detailed write-up. Imo we
> should bake this into the kerneldoc, as uabi documentation examples.
> Gustavo, can you pls include this? I'd put it into the kerneldoc for
> @out_fence, with inline struct comments we can be rather excessive in
> their lenght ;-)

Sure, I'll work on kernel doc for this.

Gustavo

Re: [PATCH v5 4/4] drm/fence: add out-fences support

[Gustavo Padovan]

2016-10-21 Daniel Vetter <daniel@ffwll.ch>:

> On Thu, Oct 20, 2016 at 10:15:20PM +0300, Ville Syrjälä wrote:
> > On Thu, Oct 20, 2016 at 07:34:44PM +0300, Ville Syrjälä wrote:
> > > On Thu, Oct 20, 2016 at 01:55:38PM -0200, Gustavo Padovan wrote:
> > > > 2016-10-20 Ville Syrjälä <ville.syrjala@linux.intel.com>:
> > > > 
> > > > > On Thu, Oct 20, 2016 at 12:50:05PM -0200, Gustavo Padovan wrote:
> > > > > > From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> > > > > > 
> > > > > > Support DRM out-fences by creating a sync_file with a fence for each CRTC
> > > > > > that sets the OUT_FENCE_PTR property.
> > > > > 
> > > > > I still maintain the out fence should also be per fb (well, per plane
> > > > > since we can't add it to fbs). Otherwise it's not going to be at all
> > > > > useful if you do fps>vrefresh and don't include the same set of planes
> > > > > in every atomic ioctl, eg. if you only ever include ones that are
> > > > > somehow dirty.
> > > > 
> > > > How would the kernel signal these dirty planes then? Right now we signal
> > > > at the vblank.
> > > 
> > > So if I think about it in terms of the previous fbs something like this
> > > comes to mind:
> > > 
> > >  starting point:
> > >  plane a, fb 0
> > >  plane b, fb 1
> > > 
> > >  ioctl:
> > >  plane a, fb 2, fence A
> > >  plane b, fb 3, fence B
> > > 
> > >  ioctl:
> > >  plane a, fb 4, fence C
> > >  -> fb 2 can be reused, so fence C should signal immediately?
> > > 
> > >  vblank:
> > >  -> fb 0,1 can be reused, so fence A,B signal?
> > > 
> > > It feels a bit wonky since the fence is effectively associated with the
> > > previous fb after the previous fb was already submitted to the kernel.
> > > One might assume fence A to be the one signalled early, but that would
> > > give the impression that fb 0 would be free for reuse when it's not.
> > 
> > OK, so after hashing this out on irc with Gustavo and Brian, we came
> > to the conclusion that the per-crtc out fence should be sufficient
> > after all.
> > 
> > The way it can work is that the first ioctl will return the fence,
> > doesn't really matter how many of the planes are involved in this
> > ioctl. Subsequent ioctls that manage to get in before the next
> > vblank will get back a -1 as the fence, even if the set if planes
> > involved is not the same as in the first ioctl. From the -1
> > userspace can tell what happened, and can then consult its own
> > records to see which still pending flips were overwritten, and
> > thus knows which buffers can be reused immediately.
> > 
> > If userspace plans on sending out the now free buffers to someone
> > else accompanied by a fence, it can just create an already signalled
> > fence to send instead of sending the fence it got back from the
> > atomic ioctl since that one is still associated with the original
> > scanout buffers.
> > 
> > The fence returned by the atomic ioctl will only signal after the
> > vblank, at which point userspace will obviously know that the
> > orginal scanout buffers are now also ready for reuse, and that
> > the last buffer submitted for each plane is now being actively
> > scanned out. So if userspace wants to send out some of the
> > original buffers to someone else, it can send along the fence
> > returned from the atomic ioctl.
> > 
> > So requires a bit more work from userspace perhaps. But obviously
> > it only has to do this if it plans on submitting multiple operations
> > per frame.
> > 
> > So a slightly expanded version of my previous example might look like:
> > starting point:
> >  plane a, fb 0
> >  plane b, fb 1
> > 
> > ioctl:
> >  crtc: fence A
> >  plane a, fb 2
> >  plane b, fb 3
> > 
> > ioctl:
> >  crtc: fence -1
> >  plane a, fb 4
> >  -> the previously submitted fb for plane a (fb 2) can be reused
> > 
> > ioctl:
> >  crtc: fence -1
> >  plane a, fb 5
> >  -> the previously submitted fb for plane a (fb 4) can be reused
> > 
> > vblank:
> >  -> fence A signals, fb 0,1 can be reused
> >     fb 3 and 5 being scanned out now
> > 
> > 
> > We also had a quick side track w.r.t. variable refresh rate displays,
> > and I think the conclusion was that there the vblank may just happen
> > sooner or later. Nothing else should change. What's unclear is how
> > the refresh rate would be controlled. The two obvious options are
> > explicit control, and automagic based on the submit rate. But that's
> > a separate topic entirely.
> 
> Thanks a lot for doing this discussion and the detailed write-up. Imo we
> should bake this into the kerneldoc, as uabi documentation examples.
> Gustavo, can you pls include this? I'd put it into the kerneldoc for
> @out_fence, with inline struct comments we can be rather excessive in
> their lenght ;-)

Sure, I'll work on kernel doc for this.

Gustavo
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v5 4/4] drm/fence: add out-fences support

[Brian Starkey]

Hi,

Sorry, I hit another couple of bugs that originated from my hastebin
patch - see below.

On Thu, Oct 20, 2016 at 12:50:05PM -0200, Gustavo Padovan wrote:
>From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
>
>Support DRM out-fences by creating a sync_file with a fence for each CRTC
>that sets the OUT_FENCE_PTR property.
>
>We use the out_fence pointer received in the OUT_FENCE_PTR prop to send
>the sync_file fd back to userspace.
>
>The sync_file and fd are allocated/created before commit, but the
>fd_install operation only happens after we know that commit succeed.
>
>In case of error userspace should receive a fd number of -1.
>
>v2: Comment by Rob Clark:
>	- Squash commit that adds DRM_MODE_ATOMIC_OUT_FENCE flag here.
>
>    Comment by Daniel Vetter:
>	- Add clean up code for out_fences
>
>v3: Comments by Daniel Vetter:
>	- create DRM_MODE_ATOMIC_EVENT_MASK
>	- userspace should fill out_fences_ptr with the crtc_ids for which
>	it wants fences back.
>
>v4: Create OUT_FENCE_PTR properties and remove old approach.
>
>Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
>---
> drivers/gpu/drm/drm_atomic.c | 116 ++++++++++++++++++++++++++++++++++---------
> drivers/gpu/drm/drm_crtc.c   |   8 +++
> include/drm/drm_crtc.h       |  19 +++++++
> 3 files changed, 119 insertions(+), 24 deletions(-)
>
>diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
>index b3284b2..07775fc 100644
>--- a/drivers/gpu/drm/drm_atomic.c
>+++ b/drivers/gpu/drm/drm_atomic.c
>@@ -490,6 +490,8 @@ int drm_atomic_crtc_set_property(struct drm_crtc *crtc,
> 					&replaced);
> 		state->color_mgmt_changed |= replaced;
> 		return ret;
>+	} else if (property == config->prop_out_fence_ptr) {
>+		state->out_fence_ptr = u64_to_user_ptr(val);
> 	} else if (crtc->funcs->atomic_set_property)
> 		return crtc->funcs->atomic_set_property(crtc, state, property, val);
> 	else
>@@ -532,6 +534,8 @@ drm_atomic_crtc_get_property(struct drm_crtc *crtc,
> 		*val = (state->ctm) ? state->ctm->base.id : 0;
> 	else if (property == config->gamma_lut_property)
> 		*val = (state->gamma_lut) ? state->gamma_lut->base.id : 0;
>+	else if (property == config->prop_out_fence_ptr)
>+		*val = 0;
> 	else if (crtc->funcs->atomic_get_property)
> 		return crtc->funcs->atomic_get_property(crtc, state, property, val);
> 	else
>@@ -1474,11 +1478,9 @@ EXPORT_SYMBOL(drm_atomic_nonblocking_commit);
>  */
>
> static struct drm_pending_vblank_event *create_vblank_event(
>-		struct drm_device *dev, struct drm_file *file_priv,
>-		struct fence *fence, uint64_t user_data)
>+		struct drm_device *dev, uint64_t user_data)
> {
> 	struct drm_pending_vblank_event *e = NULL;
>-	int ret;
>
> 	e = kzalloc(sizeof *e, GFP_KERNEL);
> 	if (!e)
>@@ -1488,17 +1490,6 @@ static struct drm_pending_vblank_event *create_vblank_event(
> 	e->event.base.length = sizeof(e->event);
> 	e->event.user_data = user_data;
>
>-	if (file_priv) {
>-		ret = drm_event_reserve_init(dev, file_priv, &e->base,
>-					     &e->event.base);
>-		if (ret) {
>-			kfree(e);
>-			return NULL;
>-		}
>-	}
>-
>-	e->base.fence = fence;
>-
> 	return e;
> }
>
>@@ -1603,6 +1594,40 @@ void drm_atomic_clean_old_fb(struct drm_device *dev,
> }
> EXPORT_SYMBOL(drm_atomic_clean_old_fb);
>
>+static int crtc_setup_out_fence(struct drm_crtc *crtc,
>+				struct drm_crtc_state *crtc_state,
>+				struct drm_out_fence_state *fence_state)
>+{
>+	struct fence *fence;
>+
>+	fence_state->fd = get_unused_fd_flags(O_CLOEXEC);
>+	if (fence_state->fd < 0) {
>+		return fence_state->fd;
>+	}
>+
>+	fence_state->out_fence_ptr = crtc_state->out_fence_ptr;
>+	crtc_state->out_fence_ptr = NULL;
>+
>+	if (put_user(fence_state->fd, fence_state->out_fence_ptr))
>+		return -EFAULT;
>+
>+	fence = kzalloc(sizeof(*fence), GFP_KERNEL);
>+	if (!fence)
>+		return -ENOMEM;
>+
>+	fence_init(fence, &drm_crtc_fence_ops, &crtc->fence_lock,
>+		   crtc->fence_context, ++crtc->fence_seqno);
>+
>+	fence_state->sync_file = sync_file_create(fence);
>+	if(!fence_state->sync_file) {
>+		fence_put(fence);
>+		return -ENOMEM;
>+	}
>+
>+	crtc_state->event->base.fence = fence_get(fence);
>+	return 0;
>+}
>+
> int drm_mode_atomic_ioctl(struct drm_device *dev,
> 			  void *data, struct drm_file *file_priv)
> {
>@@ -1617,9 +1642,10 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
> 	struct drm_plane *plane;
> 	struct drm_crtc *crtc;
> 	struct drm_crtc_state *crtc_state;
>+	struct drm_out_fence_state *fence_state;
> 	unsigned plane_mask;
> 	int ret = 0;
>-	unsigned int i, j;
>+	unsigned int i, j, fence_idx = 0;
>
> 	/* disallow for drivers not supporting atomic: */
> 	if (!drm_core_check_feature(dev, DRIVER_ATOMIC))
>@@ -1734,12 +1760,19 @@ retry:
> 		drm_mode_object_unreference(obj);
> 	}
>
>-	if (arg->flags & DRM_MODE_PAGE_FLIP_EVENT) {
>-		for_each_crtc_in_state(state, crtc, crtc_state, i) {
>+	fence_state = kcalloc(dev->mode_config.num_crtc, sizeof(*fence_state),
>+			      GFP_KERNEL);
>+	if (!fence_state) {
>+		ret = -ENOMEM;
>+		goto out;
>+	}
>+
>+	for_each_crtc_in_state(state, crtc, crtc_state, i) {
>+		if (arg->flags & DRM_MODE_PAGE_FLIP_EVENT ||
>+		    crtc_state->out_fence_ptr) {
> 			struct drm_pending_vblank_event *e;
>
>-			e = create_vblank_event(dev, file_priv, NULL,
>-						arg->user_data);
>+			e = create_vblank_event(dev, arg->user_data);
> 			if (!e) {
> 				ret = -ENOMEM;
> 				goto out;
>@@ -1747,6 +1780,28 @@ retry:
>
> 			crtc_state->event = e;
> 		}
>+
>+		if (arg->flags & DRM_MODE_PAGE_FLIP_EVENT) {
>+			struct drm_pending_vblank_event *e = crtc_state->event;
>+
>+			if (!file_priv)
>+				continue;
>+
>+			ret = drm_event_reserve_init(dev, file_priv, &e->base,
>+						     &e->event.base);
>+			if (ret) {
>+				kfree(e);
>+				crtc_state->event = NULL;
>+				goto out;
>+			}
>+		}
>+
>+		if (crtc_state->out_fence_ptr) {
>+			ret = crtc_setup_out_fence(crtc, crtc_state,
>+						   &fence_state[fence_idx++]);
>+			if (ret)
>+				goto out;
>+		}
> 	}
>
> 	if (arg->flags & DRM_MODE_ATOMIC_TEST_ONLY) {
>@@ -1761,24 +1816,37 @@ retry:
> 		ret = drm_atomic_commit(state);
> 	}
>
>+	for (i = 0; i < fence_idx; i++)
>+		fd_install(fence_state[i].fd, fence_state[i].sync_file->file);

This loop should be conditional on ret. If drm_atomic_commit fails it
doesn't jump over this, it falls into it.

>+
> out:
> 	drm_atomic_clean_old_fb(dev, plane_mask, ret);
>
>-	if (ret && arg->flags & DRM_MODE_PAGE_FLIP_EVENT) {
>+	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> 		/*
> 		 * TEST_ONLY and PAGE_FLIP_EVENT are mutually exclusive,
> 		 * if they weren't, this code should be called on success
> 		 * for TEST_ONLY too.
> 		 */
>+		if (ret && crtc_state->event)
>+			drm_event_cancel_free(dev, &crtc_state->event->base);
>+	}
>
>-		for_each_crtc_in_state(state, crtc, crtc_state, i) {
>-			if (!crtc_state->event)
>-				continue;
>+	if (ret && fence_state) {
>+		for (i = 0; i < fence_idx; i++) {
>+			if (fence_state[i].fd >= 0)
>+				put_unused_fd(fence_state[i].fd);
>+			if (fence_state[i].sync_file)
>+				fput(fence_state[i].sync_file->file);
>
>-			drm_event_cancel_free(dev, &crtc_state->event->base);
>+			/* If this fails, there's not much we can do about it */
>+			if (put_user(-1, fence_state->out_fence_ptr))
>+				DRM_ERROR("Couldn't clear out_fence_ptr\n");

This should be conditional on fence_state->out_fence_ptr, otherwise
it's needlessly noisy for an incompletely filled-in fence_state.

I also wonder if the fput should be first, for symmetry.

Cheers,
Brian

> 		}
> 	}
>
>+	kfree(fence_state);
>+
> 	if (ret == -EDEADLK) {
> 		drm_atomic_state_clear(state);
> 		drm_modeset_backoff(&ctx);
>diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
>index b99090f..40bce97 100644
>--- a/drivers/gpu/drm/drm_crtc.c
>+++ b/drivers/gpu/drm/drm_crtc.c
>@@ -274,6 +274,8 @@ int drm_crtc_init_with_planes(struct drm_device *dev, struct drm_crtc *crtc,
> 	if (drm_core_check_feature(dev, DRIVER_ATOMIC)) {
> 		drm_object_attach_property(&crtc->base, config->prop_active, 0);
> 		drm_object_attach_property(&crtc->base, config->prop_mode_id, 0);
>+		drm_object_attach_property(&crtc->base,
>+					   config->prop_out_fence_ptr, 0);
> 	}
>
> 	return 0;
>@@ -434,6 +436,12 @@ static int drm_mode_create_standard_properties(struct drm_device *dev)
> 		return -ENOMEM;
> 	dev->mode_config.prop_in_fence_fd = prop;
>
>+	prop = drm_property_create_range(dev, DRM_MODE_PROP_ATOMIC,
>+			"OUT_FENCE_PTR", 0, U64_MAX);
>+	if (!prop)
>+		return -ENOMEM;
>+	dev->mode_config.prop_out_fence_ptr = prop;
>+
> 	prop = drm_property_create_object(dev, DRM_MODE_PROP_ATOMIC,
> 			"CRTC_ID", DRM_MODE_OBJECT_CRTC);
> 	if (!prop)
>diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
>index 657a33a..b898604 100644
>--- a/include/drm/drm_crtc.h
>+++ b/include/drm/drm_crtc.h
>@@ -165,6 +165,8 @@ struct drm_crtc_state {
> 	struct drm_property_blob *ctm;
> 	struct drm_property_blob *gamma_lut;
>
>+	u64 __user *out_fence_ptr;
>+
> 	/**
> 	 * @event:
> 	 *
>@@ -748,6 +750,18 @@ static inline struct drm_crtc *fence_to_crtc(struct fence *fence)
> }
>
> /**
>+ * struct drm_out_fence_state - store out_fence data for install and clean up
>+ * @out_fence_ptr: user pointer to store the fence fd in.
>+ * @sync_file: sync_file related to the out_fence
>+ * @fd: file descriptor to installed on the sync_file.
>+ */
>+struct drm_out_fence_state {
>+	u64 __user *out_fence_ptr;
>+	struct sync_file *sync_file;
>+	int fd;
>+};
>+
>+/*
>  * struct drm_mode_set - new values for a CRTC config change
>  * @fb: framebuffer to use for new config
>  * @crtc: CRTC whose configuration we're about to change
>@@ -1230,6 +1244,11 @@ struct drm_mode_config {
> 	 */
> 	struct drm_property *prop_in_fence_fd;
> 	/**
>+	 * @prop_out_fence_ptr: Sync File fd pointer representing the
>+	 * outgoing fences for a CRTC.
>+	 */
>+	struct drm_property *prop_out_fence_ptr;
>+	/**
> 	 * @prop_crtc_id: Default atomic plane property to specify the
> 	 * &drm_crtc.
> 	 */
>-- 
>2.5.5
>
>_______________________________________________
>dri-devel mailing list
>dri-devel@lists.freedesktop.org
>https://lists.freedesktop.org/mailman/listinfo/dri-devel