[PATCH v6 0/6] drm: add explict fencing

[PATCH v6 0/6] drm: add explict fencing

[Gustavo Padovan]

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

This is yet another version of the DRM fences patches. Please refer
to the cover letter[1] in previous version to check the fences details.

For v6 we create drm_atomic_set_fence_for_plane() that tries to abstract
from drivers if we are using implicit or explicit fencing. There is lot of
improvements from the last version. Details of what changed can be found
on commit message of each patch.

Robert Foss managed to port Android's drm_hwcomposer to the new HWC2 API and
added support to fences. Current patches can be seen here:

https://git.collabora.com/cgit/user/robertfoss/drm_hwcomposer.git/log/?h=hwc2_fence_v1

He managed to run AOSP on top of padovan/fences kernel branch with full fence
support on qemu/virgl. Next we will be looking to msm db410c.

As for igt we have been progressing on adding sw_sync and drm fences tests.
I'll be improving the tests while waiting for review on this series. 

https://git.collabora.com/cgit/user/padovan/intel-gpu-tools.git/log/

Please review!

Gustavo

[1] https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1253822.html
---

Gustavo Padovan (6):
  drm/atomic: add drm_atomic_set_fence_for_plane()
  drm/imx: use drm_atomic_set_fence_for_plane() to set the fence
  drm/msm: use drm_atomic_set_fence_for_plane() to set the fence
  drm/fence: add in-fences support
  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        | 244 +++++++++++++++++++++++++++++++-----
 drivers/gpu/drm/drm_atomic_helper.c |   5 +-
 drivers/gpu/drm/drm_crtc.c          |  45 +++++++
 drivers/gpu/drm/drm_plane.c         |   1 +
 drivers/gpu/drm/imx/imx-drm-core.c  |   6 +-
 drivers/gpu/drm/msm/msm_atomic.c    |   3 +-
 include/drm/drm_atomic.h            |   3 +
 include/drm/drm_crtc.h              |  61 +++++++++
 9 files changed, 335 insertions(+), 34 deletions(-)

-- 
2.5.5

[PATCH v6 1/6] drm/atomic: add drm_atomic_set_fence_for_plane()

[Gustavo Padovan]

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

This new function should be used by drivers when setting a implicit
fence for the plane. It abstracts the fact that the user might have
chosen explicit fencing instead.

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
---
 drivers/gpu/drm/drm_atomic.c | 30 ++++++++++++++++++++++++++++++
 include/drm/drm_atomic.h     |  2 ++
 include/drm/drm_plane.h      |  2 +-
 3 files changed, 33 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index c32fb3c..5e73954 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -1147,6 +1147,36 @@ drm_atomic_set_fb_for_plane(struct drm_plane_state *plane_state,
 EXPORT_SYMBOL(drm_atomic_set_fb_for_plane);
 
 /**
+ * drm_atomic_set_fence_for_plane - set fence for plane
+ * @plane_state: atomic state object for the plane
+ * @fence: dma_fence to use for the plane
+ *
+ * Helper to setup the plane_state fence in case it is not set yet.
+ * By using this drivers doesn't need to worry if the user choose
+ * implicit or explicit fencing.
+ *
+ * This function will not set the fence to the state if it was set
+ * via explicit fencing interfaces on the atomic ioctl. It will
+ * all drope the reference to the fence as we not storing it
+ * anywhere.
+ *
+ * Otherwise, if plane_state->fence is not set this function we
+ * just set it with the received implict fence.
+ */
+void
+drm_atomic_set_fence_for_plane(struct drm_plane_state *plane_state,
+			       struct dma_fence *fence)
+{
+	if (plane_state->fence) {
+		dma_fence_put(fence);
+		return;
+	}
+
+	plane_state->fence = fence;
+}
+EXPORT_SYMBOL(drm_atomic_set_fence_for_plane);
+
+/**
  * drm_atomic_set_crtc_for_connector - set crtc for connector
  * @conn_state: atomic state object for the connector
  * @crtc: crtc to use for the connector
diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
index fc8af53..2d1e9c9 100644
--- a/include/drm/drm_atomic.h
+++ b/include/drm/drm_atomic.h
@@ -345,6 +345,8 @@ drm_atomic_set_crtc_for_plane(struct drm_plane_state *plane_state,
 			      struct drm_crtc *crtc);
 void drm_atomic_set_fb_for_plane(struct drm_plane_state *plane_state,
 				 struct drm_framebuffer *fb);
+void drm_atomic_set_fence_for_plane(struct drm_plane_state *plane_state,
+				    struct dma_fence *fence);
 int __must_check
 drm_atomic_set_crtc_for_connector(struct drm_connector_state *conn_state,
 				  struct drm_crtc *crtc);
diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
index c5e8a0d..68f6d22 100644
--- a/include/drm/drm_plane.h
+++ b/include/drm/drm_plane.h
@@ -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 dma_fence *fence;
+	struct dma_fence *fence; /* do not write directly, use drm_atomic_set_fence_for_plane() */
 
 	/* Signed dest location allows it to be partially off screen */
 	int32_t crtc_x, crtc_y;
-- 
2.5.5

[PATCH v6 2/6] drm/imx: use drm_atomic_set_fence_for_plane() to set the fence

[Gustavo Padovan]

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

drm_atomic_set_fence_for_plane() is smart and won't overwrite
plane_state->fence if the user already set an explicit fence there.

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

diff --git a/drivers/gpu/drm/imx/imx-drm-core.c b/drivers/gpu/drm/imx/imx-drm-core.c
index 98df09c..07fe955 100644
--- a/drivers/gpu/drm/imx/imx-drm-core.c
+++ b/drivers/gpu/drm/imx/imx-drm-core.c
@@ -158,6 +158,7 @@ static int imx_drm_atomic_commit(struct drm_device *dev,
 	struct drm_plane_state *plane_state;
 	struct drm_plane *plane;
 	struct dma_buf *dma_buf;
+	struct dma_fence *fence;
 	int i;
 
 	/*
@@ -170,8 +171,9 @@ static int imx_drm_atomic_commit(struct drm_device *dev,
 							 0)->base.dma_buf;
 			if (!dma_buf)
 				continue;
-			plane_state->fence =
-				reservation_object_get_excl_rcu(dma_buf->resv);
+			fence = reservation_object_get_excl_rcu(dma_buf->resv);
+
+			drm_atomic_set_fence_for_plane(plane_state, fence);
 		}
 	}
 
-- 
2.5.5

[PATCH v6 3/6] drm/msm: use drm_atomic_set_fence_for_plane() to set the fence

[Gustavo Padovan]

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

drm_atomic_set_fence_for_plane() is smart and won't overwrite
plane_state->fence if the user already set an explicit fence there.

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

diff --git a/drivers/gpu/drm/msm/msm_atomic.c b/drivers/gpu/drm/msm/msm_atomic.c
index db193f8..4e21e1d 100644
--- a/drivers/gpu/drm/msm/msm_atomic.c
+++ b/drivers/gpu/drm/msm/msm_atomic.c
@@ -217,8 +217,9 @@ int msm_atomic_commit(struct drm_device *dev,
 		if ((plane->state->fb != plane_state->fb) && plane_state->fb) {
 			struct drm_gem_object *obj = msm_framebuffer_bo(plane_state->fb, 0);
 			struct msm_gem_object *msm_obj = to_msm_bo(obj);
+			struct dma_fence *fence = reservation_object_get_excl_rcu(msm_obj->resv);
 
-			plane_state->fence = reservation_object_get_excl_rcu(msm_obj->resv);
+			drm_atomic_set_fence_for_plane(plane_state, fence);
 		}
 	}
 
-- 
2.5.5

[PATCH v6 4/6] 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

v6: Comments by Daniel Vetter:
	- rename plane_state->in_fence back to "fence"

     - rebase after fence -> dma_fence rename

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
---
 drivers/gpu/drm/Kconfig             |  1 +
 drivers/gpu/drm/drm_atomic.c        | 15 +++++++++++++++
 drivers/gpu/drm/drm_atomic_helper.c |  5 +++--
 drivers/gpu/drm/drm_crtc.c          |  6 ++++++
 drivers/gpu/drm/drm_plane.c         |  1 +
 include/drm/drm_crtc.h              |  5 +++++
 6 files changed, 31 insertions(+), 2 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 5e73954..28d9366 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->fence)
+			dma_fence_put(state->fence);
+
+		state->fence = sync_file_get_fence(val);
+		if (!state->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);
@@ -747,6 +759,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) {
@@ -1752,6 +1766,7 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
 		drm_mode_object_unreference(obj);
 	}
 
+
 	if (arg->flags & DRM_MODE_PAGE_FLIP_EVENT) {
 		for_each_crtc_in_state(state, crtc, crtc_state, i) {
 			struct drm_pending_vblank_event *e;
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 75ad01d..c34da9e 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -1034,8 +1034,6 @@ int drm_atomic_helper_wait_for_fences(struct drm_device *dev,
 		if (!plane_state->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
@@ -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->fence)
+		dma_fence_put(state->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 13441e2..7878bfd 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 fa1aa21..719b6a8 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -1201,6 +1201,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.
 	 */
-- 
2.5.5

[PATCH v6 5/6] 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

v4: Comments by Brian Starkey
	- Use even more meaninful name for the crtc timeline
	- add doc for timeline_name
    Comment by Daniel Vetter
	- use in-line style for comments

    - rebase after fence -> dma_fence rename

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

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 7878bfd..e2a06c8 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 dma_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 dma_fence *fence)
+{
+	struct drm_crtc *crtc = fence_to_crtc(fence);
+
+	return crtc->timeline_name;
+}
+
+static bool drm_crtc_fence_enable_signaling(struct dma_fence *fence)
+{
+	return true;
+}
+
+const struct dma_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 = dma_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 = dma_fence_context_alloc(1);
+	spin_lock_init(&crtc->fence_lock);
+	snprintf(crtc->timeline_name, sizeof(crtc->timeline_name),
+		 "CRTC:%d-%s", crtc->base.id, crtc->name);
+
 	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 719b6a8..278dbdd 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/dma-fence.h>
 #include <uapi/drm/drm_mode.h>
 #include <uapi/drm/drm_fourcc.h>
 #include <drm/drm_modeset_lock.h>
@@ -726,8 +728,45 @@ struct drm_crtc {
 	 */
 	struct drm_crtc_crc crc;
 #endif
+
+	/**
+	 * @fence_context:
+	 *
+	 * timeline context used for fence operations.
+	 */
+	unsigned int fence_context;
+
+	/**
+	 * @fence_lock:
+	 *
+	 * spinlock to protect the fences in the fence_context.
+	 */
+
+	spinlock_t fence_lock;
+	/**
+	 * @fence_seqno:
+	 *
+	 * Seqno variable used as monotonic counter for the fences
+	 * created on the CRTC's timeline.
+	 */
+	unsigned long fence_seqno;
+
+	/**
+	 * @timeline_name:
+	 *
+	 * The name of the CRTC's fence timeline.
+	 */
+	char timeline_name[32];
 };
 
+extern const struct dma_fence_ops drm_crtc_fence_ops;
+
+static inline struct drm_crtc *fence_to_crtc(struct dma_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 v6 6/6] 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.

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.

v5: Comments by Brian Starkey:
	- Remove extra fence_get() in atomic_ioctl()
	- Check ret before iterating on the crtc_state
	- check ret before fd_install
	- set fence_state to NULL at the beginning
	- check fence_state->out_fence_ptr before put_user()
	- change order of fput() and put_unused_fd() on failure

     - Add access_ok() check to the out_fence_ptr received
     - Rebase after fence -> dma_fence rename
     - Store out_fence_ptr in the drm_atomic_state
     - Split crtc_setup_out_fence()
     - return -1 as out_fence with TEST_ONLY flag

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
---
 drivers/gpu/drm/drm_atomic.c | 199 ++++++++++++++++++++++++++++++++++++-------
 drivers/gpu/drm/drm_crtc.c   |   8 ++
 include/drm/drm_atomic.h     |   1 +
 include/drm/drm_crtc.h       |  17 ++++
 4 files changed, 196 insertions(+), 29 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 28d9366..9b70a27 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -290,6 +290,46 @@ drm_atomic_get_crtc_state(struct drm_atomic_state *state,
 EXPORT_SYMBOL(drm_atomic_get_crtc_state);
 
 /**
+ * drm_atomic_set_out_fence_for_crtc - set CRTC out fence pointer
+ * @state: global atomic state object
+ * @crtc: crtc to set the out fence pointer
+ * @fence_ptr: the userspace pointer to user
+ *
+ * This function stores the out fence pointer in the atomic state.
+ */
+static void
+drm_atomic_set_out_fence_for_crtc(struct drm_atomic_state *state,
+				  struct drm_crtc *crtc, u64 __user *fence_ptr)
+{
+	state->crtcs[drm_crtc_index(crtc)].out_fence_ptr = fence_ptr;
+}
+
+/**
+ * drm_atomic_get_out_fence_for_crtc - get CRTC out fence pointer
+ * @state: global atomic state object
+ * @crtc: crtc to set the out fence pointer
+ *
+ * Get the user pointer that should be used for to store the out fence fd.
+ * This function should be called only once per atomic state as it clears
+ * the out_fence_ptr store there to prevent drivers to access them.
+ *
+ * Returns:
+ *
+ * The out fence user pointer.
+ */
+static u64 __user *
+drm_atomic_get_out_fence_for_crtc(struct drm_atomic_state *state,
+			     struct drm_crtc *crtc)
+{
+	u64 __user *fence_ptr;
+
+	fence_ptr = state->crtcs[drm_crtc_index(crtc)].out_fence_ptr;
+	state->crtcs[drm_crtc_index(crtc)].out_fence_ptr = NULL;
+
+	return fence_ptr;
+}
+
+/**
  * drm_atomic_set_mode_for_crtc - set mode for CRTC
  * @state: the CRTC whose incoming state to update
  * @mode: kernel-internal mode to use for the CRTC, or NULL to disable
@@ -490,6 +530,12 @@ 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) {
+		uint64_t __user *fence_ptr = u64_to_user_ptr(val);
+		if (!access_ok(VERIFY_WRITE, fence_ptr, sizeof(*fence_ptr)))
+			return -EFAULT;
+
+		drm_atomic_set_out_fence_for_crtc(state->state, crtc, fence_ptr);
 	} else if (crtc->funcs->atomic_set_property)
 		return crtc->funcs->atomic_set_property(crtc, state, property, val);
 	else
@@ -532,6 +578,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
@@ -1506,11 +1554,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 dma_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)
@@ -1520,17 +1566,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;
 }
 
@@ -1635,6 +1670,38 @@ void drm_atomic_clean_old_fb(struct drm_device *dev,
 }
 EXPORT_SYMBOL(drm_atomic_clean_old_fb);
 
+struct dma_fence *get_crtc_fence(struct drm_crtc *crtc,
+				 struct drm_crtc_state *crtc_state)
+{
+	struct dma_fence *fence;
+
+	fence = kzalloc(sizeof(*fence), GFP_KERNEL);
+	if (!fence)
+		return NULL;
+
+	dma_fence_init(fence, &drm_crtc_fence_ops, &crtc->fence_lock,
+		       crtc->fence_context, ++crtc->fence_seqno);
+
+	return fence;
+}
+
+static int setup_out_fence(struct drm_out_fence_state *fence_state,
+			   struct dma_fence *fence)
+{
+	fence_state->fd = get_unused_fd_flags(O_CLOEXEC);
+	if (fence_state->fd < 0)
+		return fence_state->fd;
+
+	if (put_user(fence_state->fd, fence_state->out_fence_ptr))
+		return -EFAULT;
+
+	fence_state->sync_file = sync_file_create(fence);
+	if(!fence_state->sync_file)
+		return -ENOMEM;
+
+	return 0;
+}
+
 int drm_mode_atomic_ioctl(struct drm_device *dev,
 			  void *data, struct drm_file *file_priv)
 {
@@ -1649,9 +1716,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 = NULL;
 	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))
@@ -1766,13 +1834,33 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
 		drm_mode_object_unreference(obj);
 	}
 
+	fence_state = kcalloc(dev->mode_config.num_crtc, sizeof(*fence_state),
+			      GFP_KERNEL);
+	if (!fence_state) {
+		ret = -ENOMEM;
+		goto out;
+	}
 
-	if (arg->flags & DRM_MODE_PAGE_FLIP_EVENT) {
-		for_each_crtc_in_state(state, crtc, crtc_state, i) {
+	for_each_crtc_in_state(state, crtc, crtc_state, i) {
+
+		fence_state->out_fence_ptr =
+			drm_atomic_get_out_fence_for_crtc(crtc_state->state, crtc);
+
+		if (fence_state->out_fence_ptr &&
+		    (arg->flags & DRM_MODE_ATOMIC_TEST_ONLY)) {
+			if (put_user(-1, fence_state->out_fence_ptr)) {
+				ret = -EFAULT;
+				goto out;
+			}
+
+			continue;
+		}
+
+		if (arg->flags & DRM_MODE_PAGE_FLIP_EVENT ||
+		    fence_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;
@@ -1780,6 +1868,39 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
 
 			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 (fence_state->out_fence_ptr) {
+			struct dma_fence *fence;
+
+			fence = get_crtc_fence(crtc, crtc_state);
+			if (!fence) {
+				ret = -ENOMEM;
+				goto out;
+			}
+
+			ret = setup_out_fence(&fence_state[fence_idx++], fence);
+			if (ret) {
+				dma_fence_put(fence);
+				goto out;
+			}
+
+			crtc_state->event->base.fence = fence;
+		}
 	}
 
 	if (arg->flags & DRM_MODE_ATOMIC_TEST_ONLY) {
@@ -1794,24 +1915,44 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
 		ret = drm_atomic_commit(state);
 	}
 
+	if (ret)
+		goto out;
+
+	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) {
-		/*
-		 * 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) {
 		for_each_crtc_in_state(state, crtc, crtc_state, i) {
-			if (!crtc_state->event)
-				continue;
+			/*
+			 * 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 (crtc_state->event)
+				drm_event_cancel_free(dev,
+						      &crtc_state->event->base);
+		}
 
-			drm_event_cancel_free(dev, &crtc_state->event->base);
+		if (fence_state) {
+			for (i = 0; i < fence_idx; i++) {
+				if (fence_state[i].sync_file)
+					fput(fence_state[i].sync_file->file);
+				if (fence_state[i].fd >= 0)
+					put_unused_fd(fence_state[i].fd);
+
+				/* If this fails log error to the user */
+				if (fence_state[i].out_fence_ptr &&
+				    put_user(-1, fence_state[i].out_fence_ptr))
+					DRM_INFO("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 e2a06c8..a18d81b 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_atomic.h b/include/drm/drm_atomic.h
index 2d1e9c9..ebde60e 100644
--- a/include/drm/drm_atomic.h
+++ b/include/drm/drm_atomic.h
@@ -144,6 +144,7 @@ struct __drm_crtcs_state {
 	struct drm_crtc *ptr;
 	struct drm_crtc_state *state;
 	struct drm_crtc_commit *commit;
+	u64 __user *out_fence_ptr;
 };
 
 struct __drm_connnectors_state {
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 278dbdd..2539394 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -768,6 +768,18 @@ static inline struct drm_crtc *fence_to_crtc(struct dma_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
@@ -1245,6 +1257,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 v6 1/6] drm/atomic: add drm_atomic_set_fence_for_plane()

[Daniel Vetter]

On Thu, Oct 27, 2016 at 05:37:06PM -0200, Gustavo Padovan wrote:
> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> 
> This new function should be used by drivers when setting a implicit
> fence for the plane. It abstracts the fact that the user might have
> chosen explicit fencing instead.
> 
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>

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

> ---
>  drivers/gpu/drm/drm_atomic.c | 30 ++++++++++++++++++++++++++++++
>  include/drm/drm_atomic.h     |  2 ++
>  include/drm/drm_plane.h      |  2 +-
>  3 files changed, 33 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index c32fb3c..5e73954 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -1147,6 +1147,36 @@ drm_atomic_set_fb_for_plane(struct drm_plane_state *plane_state,
>  EXPORT_SYMBOL(drm_atomic_set_fb_for_plane);
>  
>  /**
> + * drm_atomic_set_fence_for_plane - set fence for plane
> + * @plane_state: atomic state object for the plane
> + * @fence: dma_fence to use for the plane
> + *
> + * Helper to setup the plane_state fence in case it is not set yet.
> + * By using this drivers doesn't need to worry if the user choose
> + * implicit or explicit fencing.
> + *
> + * This function will not set the fence to the state if it was set
> + * via explicit fencing interfaces on the atomic ioctl. It will
> + * all drope the reference to the fence as we not storing it
> + * anywhere.
> + *
> + * Otherwise, if plane_state->fence is not set this function we
> + * just set it with the received implict fence.
> + */
> +void
> +drm_atomic_set_fence_for_plane(struct drm_plane_state *plane_state,
> +			       struct dma_fence *fence)
> +{
> +	if (plane_state->fence) {
> +		dma_fence_put(fence);
> +		return;
> +	}
> +
> +	plane_state->fence = fence;
> +}
> +EXPORT_SYMBOL(drm_atomic_set_fence_for_plane);
> +
> +/**
>   * drm_atomic_set_crtc_for_connector - set crtc for connector
>   * @conn_state: atomic state object for the connector
>   * @crtc: crtc to use for the connector
> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
> index fc8af53..2d1e9c9 100644
> --- a/include/drm/drm_atomic.h
> +++ b/include/drm/drm_atomic.h
> @@ -345,6 +345,8 @@ drm_atomic_set_crtc_for_plane(struct drm_plane_state *plane_state,
>  			      struct drm_crtc *crtc);
>  void drm_atomic_set_fb_for_plane(struct drm_plane_state *plane_state,
>  				 struct drm_framebuffer *fb);
> +void drm_atomic_set_fence_for_plane(struct drm_plane_state *plane_state,
> +				    struct dma_fence *fence);
>  int __must_check
>  drm_atomic_set_crtc_for_connector(struct drm_connector_state *conn_state,
>  				  struct drm_crtc *crtc);
> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
> index c5e8a0d..68f6d22 100644
> --- a/include/drm/drm_plane.h
> +++ b/include/drm/drm_plane.h
> @@ -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 dma_fence *fence;
> +	struct dma_fence *fence; /* do not write directly, use drm_atomic_set_fence_for_plane() */
>  
>  	/* 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 v6 1/6] drm/atomic: add drm_atomic_set_fence_for_plane()

[Daniel Vetter]

On Fri, Oct 28, 2016 at 09:30:26AM +0200, Daniel Vetter wrote:
> On Thu, Oct 27, 2016 at 05:37:06PM -0200, Gustavo Padovan wrote:
> > From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> > 
> > This new function should be used by drivers when setting a implicit
> > fence for the plane. It abstracts the fact that the user might have
> > chosen explicit fencing instead.
> > 
> > Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> 
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> > ---
> >  drivers/gpu/drm/drm_atomic.c | 30 ++++++++++++++++++++++++++++++
> >  include/drm/drm_atomic.h     |  2 ++
> >  include/drm/drm_plane.h      |  2 +-
> >  3 files changed, 33 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> > index c32fb3c..5e73954 100644
> > --- a/drivers/gpu/drm/drm_atomic.c
> > +++ b/drivers/gpu/drm/drm_atomic.c
> > @@ -1147,6 +1147,36 @@ drm_atomic_set_fb_for_plane(struct drm_plane_state *plane_state,
> >  EXPORT_SYMBOL(drm_atomic_set_fb_for_plane);
> >  
> >  /**
> > + * drm_atomic_set_fence_for_plane - set fence for plane
> > + * @plane_state: atomic state object for the plane
> > + * @fence: dma_fence to use for the plane
> > + *
> > + * Helper to setup the plane_state fence in case it is not set yet.
> > + * By using this drivers doesn't need to worry if the user choose
> > + * implicit or explicit fencing.
> > + *
> > + * This function will not set the fence to the state if it was set
> > + * via explicit fencing interfaces on the atomic ioctl. It will
> > + * all drope the reference to the fence as we not storing it
> > + * anywhere.
> > + *
> > + * Otherwise, if plane_state->fence is not set this function we
> > + * just set it with the received implict fence.
> > + */
> > +void
> > +drm_atomic_set_fence_for_plane(struct drm_plane_state *plane_state,
> > +			       struct dma_fence *fence)
> > +{
> > +	if (plane_state->fence) {
> > +		dma_fence_put(fence);
> > +		return;
> > +	}
> > +
> > +	plane_state->fence = fence;
> > +}
> > +EXPORT_SYMBOL(drm_atomic_set_fence_for_plane);
> > +
> > +/**
> >   * drm_atomic_set_crtc_for_connector - set crtc for connector
> >   * @conn_state: atomic state object for the connector
> >   * @crtc: crtc to use for the connector
> > diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
> > index fc8af53..2d1e9c9 100644
> > --- a/include/drm/drm_atomic.h
> > +++ b/include/drm/drm_atomic.h
> > @@ -345,6 +345,8 @@ drm_atomic_set_crtc_for_plane(struct drm_plane_state *plane_state,
> >  			      struct drm_crtc *crtc);
> >  void drm_atomic_set_fb_for_plane(struct drm_plane_state *plane_state,
> >  				 struct drm_framebuffer *fb);
> > +void drm_atomic_set_fence_for_plane(struct drm_plane_state *plane_state,
> > +				    struct dma_fence *fence);
> >  int __must_check
> >  drm_atomic_set_crtc_for_connector(struct drm_connector_state *conn_state,
> >  				  struct drm_crtc *crtc);
> > diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
> > index c5e8a0d..68f6d22 100644
> > --- a/include/drm/drm_plane.h
> > +++ b/include/drm/drm_plane.h
> > @@ -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 dma_fence *fence;
> > +	struct dma_fence *fence; /* do not write directly, use drm_atomic_set_fence_for_plane() */

Ok, small bikeshed maybe: When you feel the need to add more comments
in-line in a struct, then please switch to the in-line kernel-doc member
documentation style, and merge the existing kerneldoc together with these
additional comments. Would be great to do that with all the others here
too. Follow-up patch to address this would be great.
-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

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

Re: [PATCH v6 2/6] drm/imx: use drm_atomic_set_fence_for_plane() to set the fence

[Daniel Vetter]

On Thu, Oct 27, 2016 at 05:37:07PM -0200, Gustavo Padovan wrote:
> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> 
> drm_atomic_set_fence_for_plane() is smart and won't overwrite
> plane_state->fence if the user already set an explicit fence there.
> 
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>

Process nit: Please make sure you Cc: driver maintainers for driver
patches. Best done by putting the Cc: into the patch, then you never
forget ;-)

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/imx/imx-drm-core.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/imx/imx-drm-core.c b/drivers/gpu/drm/imx/imx-drm-core.c
> index 98df09c..07fe955 100644
> --- a/drivers/gpu/drm/imx/imx-drm-core.c
> +++ b/drivers/gpu/drm/imx/imx-drm-core.c
> @@ -158,6 +158,7 @@ static int imx_drm_atomic_commit(struct drm_device *dev,
>  	struct drm_plane_state *plane_state;
>  	struct drm_plane *plane;
>  	struct dma_buf *dma_buf;
> +	struct dma_fence *fence;
>  	int i;
>  
>  	/*
> @@ -170,8 +171,9 @@ static int imx_drm_atomic_commit(struct drm_device *dev,
>  							 0)->base.dma_buf;
>  			if (!dma_buf)
>  				continue;
> -			plane_state->fence =
> -				reservation_object_get_excl_rcu(dma_buf->resv);
> +			fence = reservation_object_get_excl_rcu(dma_buf->resv);
> +
> +			drm_atomic_set_fence_for_plane(plane_state, fence);
>  		}
>  	}
>  
> -- 
> 2.5.5
> 

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

Re: [PATCH v6 3/6] drm/msm: use drm_atomic_set_fence_for_plane() to set the fence

[Daniel Vetter]

On Thu, Oct 27, 2016 at 05:37:08PM -0200, Gustavo Padovan wrote:
> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> 
> drm_atomic_set_fence_for_plane() is smart and won't overwrite
> plane_state->fence if the user already set an explicit fence there.
> 
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>

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

> ---
>  drivers/gpu/drm/msm/msm_atomic.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/msm/msm_atomic.c b/drivers/gpu/drm/msm/msm_atomic.c
> index db193f8..4e21e1d 100644
> --- a/drivers/gpu/drm/msm/msm_atomic.c
> +++ b/drivers/gpu/drm/msm/msm_atomic.c
> @@ -217,8 +217,9 @@ int msm_atomic_commit(struct drm_device *dev,
>  		if ((plane->state->fb != plane_state->fb) && plane_state->fb) {
>  			struct drm_gem_object *obj = msm_framebuffer_bo(plane_state->fb, 0);
>  			struct msm_gem_object *msm_obj = to_msm_bo(obj);
> +			struct dma_fence *fence = reservation_object_get_excl_rcu(msm_obj->resv);
>  
> -			plane_state->fence = reservation_object_get_excl_rcu(msm_obj->resv);
> +			drm_atomic_set_fence_for_plane(plane_state, fence);
>  		}
>  	}
>  
> -- 
> 2.5.5
> 

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

Re: [PATCH v6 4/6] drm/fence: add in-fences support

[Daniel Vetter]

On Thu, Oct 27, 2016 at 05:37:09PM -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
> 
> v6: Comments by Daniel Vetter:
> 	- rename plane_state->in_fence back to "fence"
> 
>      - rebase after fence -> dma_fence rename
> 
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>

Looks good, but I'll wait with full review with all the fence patches
until the igts show up. It's much easier to check for gabs in input
validation code if you also have the testcases at hand. A few comments
below.
-Daniel

> ---
>  drivers/gpu/drm/Kconfig             |  1 +
>  drivers/gpu/drm/drm_atomic.c        | 15 +++++++++++++++
>  drivers/gpu/drm/drm_atomic_helper.c |  5 +++--
>  drivers/gpu/drm/drm_crtc.c          |  6 ++++++
>  drivers/gpu/drm/drm_plane.c         |  1 +
>  include/drm/drm_crtc.h              |  5 +++++
>  6 files changed, 31 insertions(+), 2 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 5e73954..28d9366 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->fence)
> +			dma_fence_put(state->fence);
> +
> +		state->fence = sync_file_get_fence(val);
> +		if (!state->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);
> @@ -747,6 +759,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) {
> @@ -1752,6 +1766,7 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
>  		drm_mode_object_unreference(obj);
>  	}
>  
> +

Spurios whitespace.

>  	if (arg->flags & DRM_MODE_PAGE_FLIP_EVENT) {
>  		for_each_crtc_in_state(state, crtc, crtc_state, i) {
>  			struct drm_pending_vblank_event *e;
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 75ad01d..c34da9e 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -1034,8 +1034,6 @@ int drm_atomic_helper_wait_for_fences(struct drm_device *dev,
>  		if (!plane_state->fence)
>  			continue;
>  
> -		WARN_ON(!plane_state->fb);
> -

Why this? We don't allow a plane to be enabled without an fb, and adding a
fence to a plane which is disabled sounds likea  bug. We probably should
have a bit of code in the core atomic check code to make sure userspace
never asks for a fence when the plane is off.

>  		/*
>  		 * If waiting for fences pre-swap (ie: nonblock), userspace can
>  		 * still interrupt the operation. Instead of blocking until the
> @@ -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->fence)
> +		dma_fence_put(state->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 13441e2..7878bfd 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 fa1aa21..719b6a8 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -1201,6 +1201,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.
>  	 */
> -- 
> 2.5.5
> 

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

Re: [PATCH v6 5/6] drm/fence: add fence timeline to drm_crtc

[Daniel Vetter]

On Thu, Oct 27, 2016 at 05:37:10PM -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
> 
> v4: Comments by Brian Starkey
> 	- Use even more meaninful name for the crtc timeline
> 	- add doc for timeline_name
>     Comment by Daniel Vetter
> 	- use in-line style for comments
> 
>     - rebase after fence -> dma_fence rename
> 
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> ---
>  drivers/gpu/drm/drm_crtc.c | 31 +++++++++++++++++++++++++++++++
>  include/drm/drm_crtc.h     | 39 +++++++++++++++++++++++++++++++++++++++
>  2 files changed, 70 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 7878bfd..e2a06c8 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 dma_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 dma_fence *fence)
> +{
> +	struct drm_crtc *crtc = fence_to_crtc(fence);
> +
> +	return crtc->timeline_name;
> +}
> +
> +static bool drm_crtc_fence_enable_signaling(struct dma_fence *fence)
> +{
> +	return true;
> +}
> +
> +const struct dma_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 = dma_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 = dma_fence_context_alloc(1);
> +	spin_lock_init(&crtc->fence_lock);
> +	snprintf(crtc->timeline_name, sizeof(crtc->timeline_name),
> +		 "CRTC:%d-%s", crtc->base.id, crtc->name);
> +
>  	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 719b6a8..278dbdd 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/dma-fence.h>
>  #include <uapi/drm/drm_mode.h>
>  #include <uapi/drm/drm_fourcc.h>
>  #include <drm/drm_modeset_lock.h>
> @@ -726,8 +728,45 @@ struct drm_crtc {
>  	 */
>  	struct drm_crtc_crc crc;
>  #endif
> +
> +	/**
> +	 * @fence_context:
> +	 *
> +	 * timeline context used for fence operations.
> +	 */
> +	unsigned int fence_context;
> +
> +	/**
> +	 * @fence_lock:
> +	 *
> +	 * spinlock to protect the fences in the fence_context.
> +	 */
> +
> +	spinlock_t fence_lock;
> +	/**
> +	 * @fence_seqno:
> +	 *
> +	 * Seqno variable used as monotonic counter for the fences
> +	 * created on the CRTC's timeline.
> +	 */
> +	unsigned long fence_seqno;
> +
> +	/**
> +	 * @timeline_name:
> +	 *
> +	 * The name of the CRTC's fence timeline.
> +	 */
> +	char timeline_name[32];
>  };
>  
> +extern const struct dma_fence_ops drm_crtc_fence_ops;
> +

Kerneldoc please. With that addressed:

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

> +static inline struct drm_crtc *fence_to_crtc(struct dma_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 v6 6/6] drm/fence: add out-fences support

[Daniel Vetter]

On Thu, Oct 27, 2016 at 05:37:11PM -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.
> 
> 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.
> 
> v5: Comments by Brian Starkey:
> 	- Remove extra fence_get() in atomic_ioctl()
> 	- Check ret before iterating on the crtc_state
> 	- check ret before fd_install
> 	- set fence_state to NULL at the beginning
> 	- check fence_state->out_fence_ptr before put_user()
> 	- change order of fput() and put_unused_fd() on failure
> 
>      - Add access_ok() check to the out_fence_ptr received
>      - Rebase after fence -> dma_fence rename
>      - Store out_fence_ptr in the drm_atomic_state
>      - Split crtc_setup_out_fence()
>      - return -1 as out_fence with TEST_ONLY flag
> 
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>

I think this looks good. Again I'll wait with full in-depth review of all
the input validation until the igt shows up, but I think we're mostly
covereed. A few smaller comments for polish below.
-Daniel

> ---
>  drivers/gpu/drm/drm_atomic.c | 199 ++++++++++++++++++++++++++++++++++++-------
>  drivers/gpu/drm/drm_crtc.c   |   8 ++
>  include/drm/drm_atomic.h     |   1 +
>  include/drm/drm_crtc.h       |  17 ++++
>  4 files changed, 196 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 28d9366..9b70a27 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -290,6 +290,46 @@ drm_atomic_get_crtc_state(struct drm_atomic_state *state,
>  EXPORT_SYMBOL(drm_atomic_get_crtc_state);
>  
>  /**
> + * drm_atomic_set_out_fence_for_crtc - set CRTC out fence pointer
> + * @state: global atomic state object
> + * @crtc: crtc to set the out fence pointer
> + * @fence_ptr: the userspace pointer to user
> + *
> + * This function stores the out fence pointer in the atomic state.
> + */
> +static void
> +drm_atomic_set_out_fence_for_crtc(struct drm_atomic_state *state,
> +				  struct drm_crtc *crtc, u64 __user *fence_ptr)
> +{
> +	state->crtcs[drm_crtc_index(crtc)].out_fence_ptr = fence_ptr;
> +}
> +
> +/**
> + * drm_atomic_get_out_fence_for_crtc - get CRTC out fence pointer
> + * @state: global atomic state object
> + * @crtc: crtc to set the out fence pointer
> + *
> + * Get the user pointer that should be used for to store the out fence fd.
> + * This function should be called only once per atomic state as it clears
> + * the out_fence_ptr store there to prevent drivers to access them.
> + *
> + * Returns:
> + *
> + * The out fence user pointer.
> + */

We don't document non-driver-visble functions and structs, and especially
static functions should be self explanatory. I feel bad for your effort in
typing kernel-doc here :(

> +static u64 __user *
> +drm_atomic_get_out_fence_for_crtc(struct drm_atomic_state *state,
> +			     struct drm_crtc *crtc)
> +{
> +	u64 __user *fence_ptr;
> +
> +	fence_ptr = state->crtcs[drm_crtc_index(crtc)].out_fence_ptr;
> +	state->crtcs[drm_crtc_index(crtc)].out_fence_ptr = NULL;
> +
> +	return fence_ptr;
> +}
> +
> +/**
>   * drm_atomic_set_mode_for_crtc - set mode for CRTC
>   * @state: the CRTC whose incoming state to update
>   * @mode: kernel-internal mode to use for the CRTC, or NULL to disable
> @@ -490,6 +530,12 @@ 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) {
> +		uint64_t __user *fence_ptr = u64_to_user_ptr(val);
> +		if (!access_ok(VERIFY_WRITE, fence_ptr, sizeof(*fence_ptr)))
> +			return -EFAULT;
> +
> +		drm_atomic_set_out_fence_for_crtc(state->state, crtc, fence_ptr);
>  	} else if (crtc->funcs->atomic_set_property)
>  		return crtc->funcs->atomic_set_property(crtc, state, property, val);
>  	else
> @@ -532,6 +578,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
> @@ -1506,11 +1554,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 dma_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)
> @@ -1520,17 +1566,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;
>  }
>  
> @@ -1635,6 +1670,38 @@ void drm_atomic_clean_old_fb(struct drm_device *dev,
>  }
>  EXPORT_SYMBOL(drm_atomic_clean_old_fb);
>  
> +struct dma_fence *get_crtc_fence(struct drm_crtc *crtc,
> +				 struct drm_crtc_state *crtc_state)

static?

> +{
> +	struct dma_fence *fence;
> +
> +	fence = kzalloc(sizeof(*fence), GFP_KERNEL);
> +	if (!fence)
> +		return NULL;
> +
> +	dma_fence_init(fence, &drm_crtc_fence_ops, &crtc->fence_lock,
> +		       crtc->fence_context, ++crtc->fence_seqno);
> +
> +	return fence;
> +}
> +
> +static int setup_out_fence(struct drm_out_fence_state *fence_state,
> +			   struct dma_fence *fence)
> +{
> +	fence_state->fd = get_unused_fd_flags(O_CLOEXEC);
> +	if (fence_state->fd < 0)
> +		return fence_state->fd;
> +
> +	if (put_user(fence_state->fd, fence_state->out_fence_ptr))
> +		return -EFAULT;
> +
> +	fence_state->sync_file = sync_file_create(fence);
> +	if(!fence_state->sync_file)
> +		return -ENOMEM;
> +
> +	return 0;
> +}
> +
>  int drm_mode_atomic_ioctl(struct drm_device *dev,
>  			  void *data, struct drm_file *file_priv)
>  {
> @@ -1649,9 +1716,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 = NULL;
>  	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))
> @@ -1766,13 +1834,33 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
>  		drm_mode_object_unreference(obj);
>  	}
>  
> +	fence_state = kcalloc(dev->mode_config.num_crtc, sizeof(*fence_state),
> +			      GFP_KERNEL);
> +	if (!fence_state) {
> +		ret = -ENOMEM;
> +		goto out;
> +	}
>  
> -	if (arg->flags & DRM_MODE_PAGE_FLIP_EVENT) {
> -		for_each_crtc_in_state(state, crtc, crtc_state, i) {
> +	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> +
> +		fence_state->out_fence_ptr =
> +			drm_atomic_get_out_fence_for_crtc(crtc_state->state, crtc);
> +
> +		if (fence_state->out_fence_ptr &&
> +		    (arg->flags & DRM_MODE_ATOMIC_TEST_ONLY)) {
> +			if (put_user(-1, fence_state->out_fence_ptr)) {
> +				ret = -EFAULT;
> +				goto out;
> +			}
> +
> +			continue;
> +		}
> +
> +		if (arg->flags & DRM_MODE_PAGE_FLIP_EVENT ||
> +		    fence_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;
> @@ -1780,6 +1868,39 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
>  
>  			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 (fence_state->out_fence_ptr) {
> +			struct dma_fence *fence;
> +
> +			fence = get_crtc_fence(crtc, crtc_state);
> +			if (!fence) {
> +				ret = -ENOMEM;
> +				goto out;
> +			}
> +
> +			ret = setup_out_fence(&fence_state[fence_idx++], fence);
> +			if (ret) {
> +				dma_fence_put(fence);
> +				goto out;
> +			}
> +
> +			crtc_state->event->base.fence = fence;
> +		}

I think the entire above loop body should be extracted into a helper
function, like prepare_crtc_signalling or similar. drm_mode_atomic_ioctl
is huge already as-is ;-)

>  	}
>  
>  	if (arg->flags & DRM_MODE_ATOMIC_TEST_ONLY) {
> @@ -1794,24 +1915,44 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
>  		ret = drm_atomic_commit(state);
>  	}
>  
> +	if (ret)
> +		goto out;
> +
> +	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) {
> -		/*
> -		 * 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) {
>  		for_each_crtc_in_state(state, crtc, crtc_state, i) {
> -			if (!crtc_state->event)
> -				continue;
> +			/*
> +			 * 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 (crtc_state->event)
> +				drm_event_cancel_free(dev,
> +						      &crtc_state->event->base);
> +		}
>  
> -			drm_event_cancel_free(dev, &crtc_state->event->base);
> +		if (fence_state) {
> +			for (i = 0; i < fence_idx; i++) {
> +				if (fence_state[i].sync_file)
> +					fput(fence_state[i].sync_file->file);
> +				if (fence_state[i].fd >= 0)
> +					put_unused_fd(fence_state[i].fd);
> +
> +				/* If this fails log error to the user */
> +				if (fence_state[i].out_fence_ptr &&
> +				    put_user(-1, fence_state[i].out_fence_ptr))
> +					DRM_INFO("Couldn't clear out_fence_ptr\n");
> +			}
>  		}

Same here, for symmetry maybe a unprepare_crtc_signalling or so, to make
it clear that this undoes what the first one set up.

>  	}
>  
> +	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 e2a06c8..a18d81b 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_atomic.h b/include/drm/drm_atomic.h
> index 2d1e9c9..ebde60e 100644
> --- a/include/drm/drm_atomic.h
> +++ b/include/drm/drm_atomic.h
> @@ -144,6 +144,7 @@ struct __drm_crtcs_state {
>  	struct drm_crtc *ptr;
>  	struct drm_crtc_state *state;
>  	struct drm_crtc_commit *commit;
> +	u64 __user *out_fence_ptr;
>  };
>  
>  struct __drm_connnectors_state {
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 278dbdd..2539394 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -768,6 +768,18 @@ static inline struct drm_crtc *fence_to_crtc(struct dma_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;
> +};

You only use this in drm_atomic.c, no need to have it in the header and
document it. Please move it into drm_atomic.c, right befoer it's used.

> +
> +/*
>   * 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
> @@ -1245,6 +1257,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
> 

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

Re: [PATCH v6 5/6] drm/fence: add fence timeline to drm_crtc

[Brian Starkey]

On Fri, Oct 28, 2016 at 09:42:12AM +0200, Daniel Vetter wrote:
>On Thu, Oct 27, 2016 at 05:37:10PM -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
>>
>> v4: Comments by Brian Starkey
>> 	- Use even more meaninful name for the crtc timeline
>> 	- add doc for timeline_name
>>     Comment by Daniel Vetter
>> 	- use in-line style for comments
>>
>>     - rebase after fence -> dma_fence rename
>>
>> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
>> ---
>>  drivers/gpu/drm/drm_crtc.c | 31 +++++++++++++++++++++++++++++++
>>  include/drm/drm_crtc.h     | 39 +++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 70 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
>> index 7878bfd..e2a06c8 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 dma_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 dma_fence *fence)
>> +{
>> +	struct drm_crtc *crtc = fence_to_crtc(fence);
>> +
>> +	return crtc->timeline_name;
>> +}
>> +
>> +static bool drm_crtc_fence_enable_signaling(struct dma_fence *fence)
>> +{
>> +	return true;
>> +}
>> +
>> +const struct dma_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 = dma_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 = dma_fence_context_alloc(1);
>> +	spin_lock_init(&crtc->fence_lock);
>> +	snprintf(crtc->timeline_name, sizeof(crtc->timeline_name),
>> +		 "CRTC:%d-%s", crtc->base.id, crtc->name);
>> +
>>  	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 719b6a8..278dbdd 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/dma-fence.h>
>>  #include <uapi/drm/drm_mode.h>
>>  #include <uapi/drm/drm_fourcc.h>
>>  #include <drm/drm_modeset_lock.h>
>> @@ -726,8 +728,45 @@ struct drm_crtc {
>>  	 */
>>  	struct drm_crtc_crc crc;
>>  #endif
>> +
>> +	/**
>> +	 * @fence_context:
>> +	 *
>> +	 * timeline context used for fence operations.
>> +	 */
>> +	unsigned int fence_context;
>> +
>> +	/**
>> +	 * @fence_lock:
>> +	 *
>> +	 * spinlock to protect the fences in the fence_context.
>> +	 */
>> +
>> +	spinlock_t fence_lock;
>> +	/**
>> +	 * @fence_seqno:
>> +	 *
>> +	 * Seqno variable used as monotonic counter for the fences
>> +	 * created on the CRTC's timeline.
>> +	 */
>> +	unsigned long fence_seqno;
>> +
>> +	/**
>> +	 * @timeline_name:
>> +	 *
>> +	 * The name of the CRTC's fence timeline.
>> +	 */
>> +	char timeline_name[32];
>>  };
>>
>> +extern const struct dma_fence_ops drm_crtc_fence_ops;
>> +
>
>Kerneldoc please. With that addressed:
>
>Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>

For my connector fences I exported a function to get the fence, then
you can keep the ops and fence_to_crtc private to drm_crtc.c. In that
case I think you can drop the BUG_ON.

Either way, lgtm.

Reviewed-by: Brian Starkey <brian.starkey@arm.com>

>> +static inline struct drm_crtc *fence_to_crtc(struct dma_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 v6 6/6] drm/fence: add out-fences support

[Brian Starkey]

Hi Gustavo,

On Thu, Oct 27, 2016 at 05:37:11PM -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.
>
>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.
>
>v5: Comments by Brian Starkey:
>	- Remove extra fence_get() in atomic_ioctl()
>	- Check ret before iterating on the crtc_state
>	- check ret before fd_install
>	- set fence_state to NULL at the beginning
>	- check fence_state->out_fence_ptr before put_user()
>	- change order of fput() and put_unused_fd() on failure
>
>     - Add access_ok() check to the out_fence_ptr received
>     - Rebase after fence -> dma_fence rename
>     - Store out_fence_ptr in the drm_atomic_state
>     - Split crtc_setup_out_fence()
>     - return -1 as out_fence with TEST_ONLY flag
>
>Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
>---
> drivers/gpu/drm/drm_atomic.c | 199 ++++++++++++++++++++++++++++++++++++-------
> drivers/gpu/drm/drm_crtc.c   |   8 ++
> include/drm/drm_atomic.h     |   1 +
> include/drm/drm_crtc.h       |  17 ++++
> 4 files changed, 196 insertions(+), 29 deletions(-)
>
>diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
>index 28d9366..9b70a27 100644
>--- a/drivers/gpu/drm/drm_atomic.c
>+++ b/drivers/gpu/drm/drm_atomic.c
>@@ -290,6 +290,46 @@ drm_atomic_get_crtc_state(struct drm_atomic_state *state,
> EXPORT_SYMBOL(drm_atomic_get_crtc_state);
>
> /**
>+ * drm_atomic_set_out_fence_for_crtc - set CRTC out fence pointer
>+ * @state: global atomic state object
>+ * @crtc: crtc to set the out fence pointer
>+ * @fence_ptr: the userspace pointer to user
>+ *
>+ * This function stores the out fence pointer in the atomic state.
>+ */
>+static void
>+drm_atomic_set_out_fence_for_crtc(struct drm_atomic_state *state,
>+				  struct drm_crtc *crtc, u64 __user *fence_ptr)

bikeshed: I'd rather these were called set/get_out_fence_ptr_for_crtc,
as they don't involve struct drm_fence at all.

>+{
>+	state->crtcs[drm_crtc_index(crtc)].out_fence_ptr = fence_ptr;
>+}
>+
>+/**
>+ * drm_atomic_get_out_fence_for_crtc - get CRTC out fence pointer
>+ * @state: global atomic state object
>+ * @crtc: crtc to set the out fence pointer
>+ *
>+ * Get the user pointer that should be used for to store the out fence fd.
>+ * This function should be called only once per atomic state as it clears
>+ * the out_fence_ptr store there to prevent drivers to access them.
>+ *
>+ * Returns:
>+ *
>+ * The out fence user pointer.
>+ */
>+static u64 __user *
>+drm_atomic_get_out_fence_for_crtc(struct drm_atomic_state *state,
>+			     struct drm_crtc *crtc)
>+{
>+	u64 __user *fence_ptr;
>+
>+	fence_ptr = state->crtcs[drm_crtc_index(crtc)].out_fence_ptr;
>+	state->crtcs[drm_crtc_index(crtc)].out_fence_ptr = NULL;
>+
>+	return fence_ptr;
>+}
>+
>+/**
>  * drm_atomic_set_mode_for_crtc - set mode for CRTC
>  * @state: the CRTC whose incoming state to update
>  * @mode: kernel-internal mode to use for the CRTC, or NULL to disable
>@@ -490,6 +530,12 @@ 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) {
>+		uint64_t __user *fence_ptr = u64_to_user_ptr(val);
>+		if (!access_ok(VERIFY_WRITE, fence_ptr, sizeof(*fence_ptr)))
>+			return -EFAULT;
>+
>+		drm_atomic_set_out_fence_for_crtc(state->state, crtc, fence_ptr);
> 	} else if (crtc->funcs->atomic_set_property)
> 		return crtc->funcs->atomic_set_property(crtc, state, property, val);
> 	else
>@@ -532,6 +578,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
>@@ -1506,11 +1554,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 dma_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)
>@@ -1520,17 +1566,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;
> }
>
>@@ -1635,6 +1670,38 @@ void drm_atomic_clean_old_fb(struct drm_device *dev,
> }
> EXPORT_SYMBOL(drm_atomic_clean_old_fb);
>
>+struct dma_fence *get_crtc_fence(struct drm_crtc *crtc,
>+				 struct drm_crtc_state *crtc_state)
>+{
>+	struct dma_fence *fence;
>+
>+	fence = kzalloc(sizeof(*fence), GFP_KERNEL);
>+	if (!fence)
>+		return NULL;
>+
>+	dma_fence_init(fence, &drm_crtc_fence_ops, &crtc->fence_lock,
>+		       crtc->fence_context, ++crtc->fence_seqno);
>+
>+	return fence;
>+}
>+
>+static int setup_out_fence(struct drm_out_fence_state *fence_state,
>+			   struct dma_fence *fence)
>+{
>+	fence_state->fd = get_unused_fd_flags(O_CLOEXEC);
>+	if (fence_state->fd < 0)
>+		return fence_state->fd;
>+
>+	if (put_user(fence_state->fd, fence_state->out_fence_ptr))
>+		return -EFAULT;
>+
>+	fence_state->sync_file = sync_file_create(fence);
>+	if(!fence_state->sync_file)
>+		return -ENOMEM;
>+
>+	return 0;
>+}
>+
> int drm_mode_atomic_ioctl(struct drm_device *dev,
> 			  void *data, struct drm_file *file_priv)
> {
>@@ -1649,9 +1716,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 = NULL;
> 	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))
>@@ -1766,13 +1834,33 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
> 		drm_mode_object_unreference(obj);
> 	}
>
>+	fence_state = kcalloc(dev->mode_config.num_crtc, sizeof(*fence_state),
>+			      GFP_KERNEL);
>+	if (!fence_state) {
>+		ret = -ENOMEM;
>+		goto out;
>+	}
>
>-	if (arg->flags & DRM_MODE_PAGE_FLIP_EVENT) {
>-		for_each_crtc_in_state(state, crtc, crtc_state, i) {
>+	for_each_crtc_in_state(state, crtc, crtc_state, i) {
>+
>+		fence_state->out_fence_ptr =
>+			drm_atomic_get_out_fence_for_crtc(crtc_state->state, crtc);

I think you're missing some subscripting on fence_state in this loop.
Here you set fence_state->out_fence_ptr directly, whereas I guess it
should be fence_state[fence_idx].out_fence_ptr;

>+
>+		if (fence_state->out_fence_ptr &&
>+		    (arg->flags & DRM_MODE_ATOMIC_TEST_ONLY)) {
>+			if (put_user(-1, fence_state->out_fence_ptr)) {
>+				ret = -EFAULT;
>+				goto out;
>+			}
>+
>+			continue;
>+		}
>+
>+		if (arg->flags & DRM_MODE_PAGE_FLIP_EVENT ||
>+		    fence_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;
>@@ -1780,6 +1868,39 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
>
> 			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 (fence_state->out_fence_ptr) {
>+			struct dma_fence *fence;
>+
>+			fence = get_crtc_fence(crtc, crtc_state);
>+			if (!fence) {
>+				ret = -ENOMEM;
>+				goto out;
>+			}
>+
>+			ret = setup_out_fence(&fence_state[fence_idx++], fence);

Related to above, I think if you're going to manipulate the
fence_state array outside of setup_out_fence you should drop the
inline increment here, as it will be easy for a refactor to miss it
and add bugs.

>+			if (ret) {
>+				dma_fence_put(fence);
>+				goto out;
>+			}
>+
>+			crtc_state->event->base.fence = fence;
>+		}
> 	}
>
> 	if (arg->flags & DRM_MODE_ATOMIC_TEST_ONLY) {
>@@ -1794,24 +1915,44 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
> 		ret = drm_atomic_commit(state);
> 	}
>
>+	if (ret)
>+		goto out;
>+
>+	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) {
>-		/*
>-		 * 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) {
> 		for_each_crtc_in_state(state, crtc, crtc_state, i) {
>-			if (!crtc_state->event)
>-				continue;
>+			/*
>+			 * 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 (crtc_state->event)
>+				drm_event_cancel_free(dev,
>+						      &crtc_state->event->base);
>+		}
>
>-			drm_event_cancel_free(dev, &crtc_state->event->base);
>+		if (fence_state) {
>+			for (i = 0; i < fence_idx; i++) {
>+				if (fence_state[i].sync_file)
>+					fput(fence_state[i].sync_file->file);
>+				if (fence_state[i].fd >= 0)
>+					put_unused_fd(fence_state[i].fd);
>+
>+				/* If this fails log error to the user */
>+				if (fence_state[i].out_fence_ptr &&
>+				    put_user(-1, fence_state[i].out_fence_ptr))
>+					DRM_INFO("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 e2a06c8..a18d81b 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_atomic.h b/include/drm/drm_atomic.h
>index 2d1e9c9..ebde60e 100644
>--- a/include/drm/drm_atomic.h
>+++ b/include/drm/drm_atomic.h
>@@ -144,6 +144,7 @@ struct __drm_crtcs_state {
> 	struct drm_crtc *ptr;
> 	struct drm_crtc_state *state;
> 	struct drm_crtc_commit *commit;
>+	u64 __user *out_fence_ptr;
> };
>
> struct __drm_connnectors_state {
>diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
>index 278dbdd..2539394 100644
>--- a/include/drm/drm_crtc.h
>+++ b/include/drm/drm_crtc.h
>@@ -768,6 +768,18 @@ static inline struct drm_crtc *fence_to_crtc(struct dma_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
>@@ -1245,6 +1257,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.
>+	 */

Is it worth noting that userspace should provide a pointer to a u64
and not a plain int? Either here or in some more detailed kernel-doc
for the OUT_FENCE_PTR property.

Cheers,
Brian

>+	struct drm_property *prop_out_fence_ptr;
>+	/**
> 	 * @prop_crtc_id: Default atomic plane property to specify the
> 	 * &drm_crtc.
> 	 */
>-- 
>2.5.5
>

Re: [PATCH v6 4/6] drm/fence: add in-fences support

[Gustavo Padovan]

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

> On Thu, Oct 27, 2016 at 05:37:09PM -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
> > 
> > v6: Comments by Daniel Vetter:
> > 	- rename plane_state->in_fence back to "fence"
> > 
> >      - rebase after fence -> dma_fence rename
> > 
> > Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> 
> Looks good, but I'll wait with full review with all the fence patches
> until the igts show up. It's much easier to check for gabs in input
> validation code if you also have the testcases at hand. A few comments
> below.
> -Daniel
> 
> > ---
> >  drivers/gpu/drm/Kconfig             |  1 +
> >  drivers/gpu/drm/drm_atomic.c        | 15 +++++++++++++++
> >  drivers/gpu/drm/drm_atomic_helper.c |  5 +++--
> >  drivers/gpu/drm/drm_crtc.c          |  6 ++++++
> >  drivers/gpu/drm/drm_plane.c         |  1 +
> >  include/drm/drm_crtc.h              |  5 +++++
> >  6 files changed, 31 insertions(+), 2 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 5e73954..28d9366 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->fence)
> > +			dma_fence_put(state->fence);
> > +
> > +		state->fence = sync_file_get_fence(val);
> > +		if (!state->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);
> > @@ -747,6 +759,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) {
> > @@ -1752,6 +1766,7 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
> >  		drm_mode_object_unreference(obj);
> >  	}
> >  
> > +
> 
> Spurios whitespace.
> 
> >  	if (arg->flags & DRM_MODE_PAGE_FLIP_EVENT) {
> >  		for_each_crtc_in_state(state, crtc, crtc_state, i) {
> >  			struct drm_pending_vblank_event *e;
> > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> > index 75ad01d..c34da9e 100644
> > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > @@ -1034,8 +1034,6 @@ int drm_atomic_helper_wait_for_fences(struct drm_device *dev,
> >  		if (!plane_state->fence)
> >  			continue;
> >  
> > -		WARN_ON(!plane_state->fb);
> > -
> 
> Why this? We don't allow a plane to be enabled without an fb, and adding a
> fence to a plane which is disabled sounds likea  bug. We probably should
> have a bit of code in the core atomic check code to make sure userspace
> never asks for a fence when the plane is off.

This was Maarten's suggestion that we should be able to sent a fence to
determine when then plane should be disabled. Not sure which usecase he
had in mind.

Gustavo