Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1363609 > unrolled thread

[RFC 0/6] drm/fences: add in-fences to DRM

Back to article view | Back to linux.kernel

Contents

  [RFC 0/6] drm/fences: add in-fences to DRM Gustavo Padovan <gustavo@padovan.org> - 2016-03-23 19:50 +0100
    [RFC 6/6] drm/fence: support fence_collection on atomic commit Gustavo Padovan <gustavo@padovan.org> - 2016-03-23 19:50 +0100
    [RFC 5/6] dma-buf/fence: add fence_collection_wait() Gustavo Padovan <gustavo@padovan.org> - 2016-03-23 19:50 +0100
    [RFC 4/6] dma-buf/fence: add fence_collection_put() Gustavo Padovan <gustavo@padovan.org> - 2016-03-23 19:50 +0100
    [RFC 3/6] dma-buf/sync_file: add sync_file_fences_get() Gustavo Padovan <gustavo@padovan.org> - 2016-03-23 19:50 +0100
    [RFC 1/6] drm/fence: add FENCE_FD property to planes Gustavo Padovan <gustavo@padovan.org> - 2016-03-23 19:50 +0100
    Re: [RFC 0/6] drm/fences: add in-fences to DRM Maarten Lankhorst <maarten.lankhorst@linux.intel.com> - 2016-03-24 08:30 +0100
      Re: [RFC 0/6] drm/fences: add in-fences to DRM Gustavo Padovan <gustavo@padovan.org> - 2016-03-24 15:40 +0100
    Re: [RFC 0/6] drm/fences: add in-fences to DRM Inki Dae <inki.dae@samsung.com> - 2016-03-24 09:20 +0100
      Re: [RFC 0/6] drm/fences: add in-fences to DRM Gustavo Padovan <gustavo@padovan.org> - 2016-03-24 15:50 +0100
        Re: [RFC 0/6] drm/fences: add in-fences to DRM Inki Dae <inki.dae@samsung.com> - 2016-03-25 00:10 +0100
      Re: [RFC 0/6] drm/fences: add in-fences to DRM Rob Clark <robdclark@gmail.com> - 2016-03-24 16:50 +0100
        Re: [RFC 0/6] drm/fences: add in-fences to DRM Inki Dae <inki.dae@samsung.com> - 2016-03-25 00:50 +0100
          Re: [RFC 0/6] drm/fences: add in-fences to DRM Rob Clark <robdclark@gmail.com> - 2016-03-25 13:00 +0100
            Re: [RFC 0/6] drm/fences: add in-fences to DRM Daniel Stone <daniel@fooishbar.org> - 2016-03-25 13:20 +0100
              Re: [RFC 0/6] drm/fences: add in-fences to DRM Inki Dae <inki.dae@samsung.com> - 2016-03-28 03:30 +0200
                Re: [RFC 0/6] drm/fences: add in-fences to DRM Daniel Stone <daniel@fooishbar.org> - 2016-03-28 15:30 +0200
                  Re: [RFC 0/6] drm/fences: add in-fences to DRM Inki Dae <inki.dae@samsung.com> - 2016-03-29 04:20 +0200
                    Re: [RFC 0/6] drm/fences: add in-fences to DRM Rob Clark <robdclark@gmail.com> - 2016-03-29 15:30 +0200
                      Re: [RFC 0/6] drm/fences: add in-fences to DRM Inki Dae <inki.dae@samsung.com> - 2016-03-31 09:50 +0200
                        Re: [RFC 0/6] drm/fences: add in-fences to DRM Daniel Stone <daniel@fooishbar.org> - 2016-03-31 11:40 +0200
                          Re: [RFC 0/6] drm/fences: add in-fences to DRM Inki Dae <inki.dae@samsung.com> - 2016-03-31 12:10 +0200
                            Re: [RFC 0/6] drm/fences: add in-fences to DRM Daniel Stone <daniel@fooishbar.org> - 2016-03-31 13:00 +0200
                              Re: [RFC 0/6] drm/fences: add in-fences to DRM Inki Dae <daeinki@gmail.com> - 2016-03-31 13:30 +0200
                                Re: [RFC 0/6] drm/fences: add in-fences to DRM Daniel Stone <daniel@fooishbar.org> - 2016-03-31 13:50 +0200
                                Re: [RFC 0/6] drm/fences: add in-fences to DRM Rob Clark <robdclark@gmail.com> - 2016-03-31 16:20 +0200
                          Re: [RFC 0/6] drm/fences: add in-fences to DRM Daniel Vetter <daniel@ffwll.ch> - 2016-03-31 12:10 +0200
                            Re: [RFC 0/6] drm/fences: add in-fences to DRM Inki Dae <daeinki@gmail.com> - 2016-03-31 13:50 +0200

Page 1 of 2  [1] 2  Next page →

#1363609 — [RFC 0/6] drm/fences: add in-fences to DRM

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

Hi,

This is a first proposal to discuss the addition of in-fences support
to DRM. It adds a new struct to fence.c to abstract the use of sync_file
in DRM drivers. The new struct fence_collection contains a array with all
fences that a atomic commit needs to wait on

/**
 * struct fence_collection - aggregate fences together
 * @num_fences: number of fence in the collection.
 * @user_data: user data.
 * @func: user callback to put user data.
 * @fences: array of @num_fences fences.
 */
struct fence_collection {
       int num_fences;
       void *user_data;
       collection_put_func_t func;
       struct fence *fences[];
};


The fence_collection is allocated and filled by sync_file_fences_get() and
atomic_commit helpers can use fence_collection_wait() to wait the fences to
signal.

These patches depends on the sync ABI rework:

https://www.spinics.net/lists/dri-devel/msg102795.html

and the patch to de-stage the sync framework:

https://www.spinics.net/lists/dri-devel/msg102799.html


I also hacked together some sync support into modetest for testing:

https://git.collabora.com/cgit/user/padovan/libdrm.git/log/?h=atomic


	Gustavo


Gustavo Padovan (6):
  drm/fence: add FENCE_FD property to planes
  dma-buf/fence: add struct fence_collection
  dma-buf/sync_file: add sync_file_fences_get()
  dma-buf/fence: add fence_collection_put()
  dma-buf/fence: add fence_collection_wait()
  drm/fence: support fence_collection on atomic commit

 drivers/dma-buf/fence.c             | 33 +++++++++++++++++++++++++++++++++
 drivers/dma-buf/sync_file.c         | 36 ++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/drm_atomic.c        | 13 +++++++++++++
 drivers/gpu/drm/drm_atomic_helper.c | 10 ++++++----
 drivers/gpu/drm/drm_crtc.c          |  7 +++++++
 include/drm/drm_crtc.h              |  5 ++++-
 include/linux/fence.h               | 19 +++++++++++++++++++
 include/linux/sync_file.h           |  8 ++++++++
 8 files changed, 126 insertions(+), 5 deletions(-)

-- 
2.5.0

[toc] | [next] | [standalone]

#1363610 — [RFC 6/6] drm/fence: support fence_collection on atomic commit

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

Let atomic_commit() wait on a collection of fences before proceed with
the scanout.

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
---
 drivers/gpu/drm/drm_atomic.c        | 9 +++++++++
 drivers/gpu/drm/drm_atomic_helper.c | 9 +++++----
 include/drm/drm_crtc.h              | 2 +-
 3 files changed, 15 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 8bc364c..28a65d1 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -29,6 +29,7 @@
 #include <drm/drmP.h>
 #include <drm/drm_atomic.h>
 #include <drm/drm_plane_helper.h>
+#include <linux/sync_file.h>
 
 /**
  * drm_atomic_state_default_release -
@@ -795,6 +796,14 @@ static int drm_atomic_plane_check(struct drm_plane *plane,
 		return -EINVAL;
 	}
 
+#ifdef CONFIG_SYNC_FILE
+	if (state->fence_fd >= 0) {
+		state->fences = sync_file_fences_get(state->fence_fd);
+		if (!state->fences)
+			return -EINVAL;
+	}
+#endif
+
 	return 0;
 }
 
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 4f91f84..a6e34b6 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -977,14 +977,12 @@ static void wait_for_fences(struct drm_device *dev,
 	int i;
 
 	for_each_plane_in_state(state, plane, plane_state, i) {
-		if (!plane->state->fence)
+		if (!plane->state->fences)
 			continue;
 
 		WARN_ON(!plane->state->fb);
 
-		fence_wait(plane->state->fence, false);
-		fence_put(plane->state->fence);
-		plane->state->fence = NULL;
+		fence_collection_wait(plane->state->fences);
 	}
 }
 
@@ -2654,6 +2652,9 @@ void __drm_atomic_helper_plane_destroy_state(struct drm_plane *plane,
 {
 	if (state->fb)
 		drm_framebuffer_unreference(state->fb);
+
+	if (state->fences)
+		fence_collection_put(state->fences);
 }
 EXPORT_SYMBOL(__drm_atomic_helper_plane_destroy_state);
 
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index a8f6ec0..c221c28 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -1257,7 +1257,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_collection *fences;
 	int fence_fd;
 
 	/* Signed dest location allows it to be partially off screen */
-- 
2.5.0

[toc] | [prev] | [next] | [standalone]

#1363613 — [RFC 5/6] dma-buf/fence: add fence_collection_wait()

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

Iterate over the array of fences and wait for all of the to finish.

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
---
 drivers/dma-buf/fence.c | 16 ++++++++++++++++
 include/linux/fence.h   |  1 +
 2 files changed, 17 insertions(+)

diff --git a/drivers/dma-buf/fence.c b/drivers/dma-buf/fence.c
index a3fe3e7..31e554b 100644
--- a/drivers/dma-buf/fence.c
+++ b/drivers/dma-buf/fence.c
@@ -532,6 +532,22 @@ fence_init(struct fence *fence, const struct fence_ops *ops,
 EXPORT_SYMBOL(fence_init);
 
 /**
+ * fence_collection_wait - Wait for collection of fences to signal
+ * @collection:	[in]	the collection to wait on
+ *
+ * This functions simplifies the waiting process when one needs to
+ * wait for many fences at the same time.
+ */
+void fence_collection_wait(struct fence_collection *collection)
+{
+	int i;
+
+	for (i = 0 ; i < collection->num_fences ; i++)
+		fence_wait(collection->fences[i], false);
+}
+EXPORT_SYMBOL(fence_collection_wait);
+
+/**
  * fence_collection_put - put all the fences in a collection
  * @collection:	[in]	the collection to put fences
  *
diff --git a/include/linux/fence.h b/include/linux/fence.h
index 3f871b0..52f1aea 100644
--- a/include/linux/fence.h
+++ b/include/linux/fence.h
@@ -244,6 +244,7 @@ int fence_add_callback(struct fence *fence, struct fence_cb *cb,
 bool fence_remove_callback(struct fence *fence, struct fence_cb *cb);
 void fence_enable_sw_signaling(struct fence *fence);
 
+void fence_collection_wait(struct fence_collection *collection);
 void fence_collection_put(struct fence_collection *collection);
 
 /**
-- 
2.5.0

[toc] | [prev] | [next] | [standalone]

#1363614 — [RFC 4/6] dma-buf/fence: add fence_collection_put()

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

Put fence_collection data. For that calls fence_put() on all fences
and the user put callback.

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
---
 drivers/dma-buf/fence.c | 17 +++++++++++++++++
 include/linux/fence.h   |  2 ++
 2 files changed, 19 insertions(+)

diff --git a/drivers/dma-buf/fence.c b/drivers/dma-buf/fence.c
index 7b05dbe..a3fe3e7 100644
--- a/drivers/dma-buf/fence.c
+++ b/drivers/dma-buf/fence.c
@@ -530,3 +530,20 @@ fence_init(struct fence *fence, const struct fence_ops *ops,
 	trace_fence_init(fence);
 }
 EXPORT_SYMBOL(fence_init);
+
+/**
+ * fence_collection_put - put all the fences in a collection
+ * @collection:	[in]	the collection to put fences
+ *
+ * This functions unrefs all fences in the collection.
+ */
+void fence_collection_put(struct fence_collection *collection)
+{
+	int i;
+
+	for (i = 0 ; i < collection->num_fences ; i++)
+		fence_put(collection->fences[i]);
+
+	collection->func(collection->user_data);
+}
+EXPORT_SYMBOL(fence_collection_put);
diff --git a/include/linux/fence.h b/include/linux/fence.h
index 3d1151f..3f871b0 100644
--- a/include/linux/fence.h
+++ b/include/linux/fence.h
@@ -244,6 +244,8 @@ int fence_add_callback(struct fence *fence, struct fence_cb *cb,
 bool fence_remove_callback(struct fence *fence, struct fence_cb *cb);
 void fence_enable_sw_signaling(struct fence *fence);
 
+void fence_collection_put(struct fence_collection *collection);
+
 /**
  * fence_is_signaled_locked - Return an indication if the fence is signaled yet.
  * @fence:	[in]	the fence to check
-- 
2.5.0

[toc] | [prev] | [next] | [standalone]

#1363615 — [RFC 3/6] dma-buf/sync_file: add sync_file_fences_get()

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

Creates a function that given an sync file descriptor returns a
fence_collection containing all fences in the sync_file.

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
---
 drivers/dma-buf/sync_file.c | 36 ++++++++++++++++++++++++++++++++++++
 include/linux/sync_file.h   |  8 ++++++++
 2 files changed, 44 insertions(+)

diff --git a/drivers/dma-buf/sync_file.c b/drivers/dma-buf/sync_file.c
index b67876b..16a3ef7 100644
--- a/drivers/dma-buf/sync_file.c
+++ b/drivers/dma-buf/sync_file.c
@@ -122,6 +122,42 @@ void sync_file_install(struct sync_file *sync_file, int fd)
 }
 EXPORT_SYMBOL(sync_file_install);
 
+static void sync_file_fence_collection_put(void *data)
+{
+	struct sync_file *sync_file = data;
+
+	sync_file_put(sync_file);
+}
+
+struct fence_collection *sync_file_fences_get(int fd)
+{
+	struct fence_collection *collection;
+	struct sync_file *sync_file;
+	int i;
+
+	sync_file = sync_file_fdget(fd);
+	if (!sync_file)
+		return NULL;
+
+	collection = kzalloc(offsetof(struct fence_collection,
+				  fences[sync_file->num_fences]),
+			     GFP_KERNEL);
+	if (!collection)
+		return NULL;
+
+	collection->num_fences = sync_file->num_fences;
+	collection->user_data = sync_file;
+	collection->func = sync_file_fence_collection_put;
+
+	for (i = 0; i < sync_file->num_fences; ++i) {
+		collection->fences[i] = sync_file->cbs[i].fence;
+		fence_get(collection->fences[i]);
+	}
+
+	return collection;
+}
+EXPORT_SYMBOL(sync_file_fences_get);
+
 static void sync_file_add_pt(struct sync_file *sync_file, int *i,
 			     struct fence *fence)
 {
diff --git a/include/linux/sync_file.h b/include/linux/sync_file.h
index 7b7a89d..1b9386a 100644
--- a/include/linux/sync_file.h
+++ b/include/linux/sync_file.h
@@ -103,4 +103,12 @@ void sync_file_put(struct sync_file *sync_file);
  */
 void sync_file_install(struct sync_file *sync_file, int fd);
 
+/**
+ * sync_file_fences_get - get the fence collection related to the fd
+ * @fd:		file descriptor to look for a fence collection
+ *
+ * Ensures @fd references a valid sync_file, increments the refcount of the
+ * backing file. Returns the fence_collection or NULL in case of error.
+ */
+struct fence_collection *sync_file_fences_get(int fd);
 #endif /* _LINUX_SYNC_H */
-- 
2.5.0

[toc] | [prev] | [next] | [standalone]

#1363616 — [RFC 1/6] drm/fence: add FENCE_FD property to planes

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

FENCE_FD can now be set by the user during an atomic IOCTL, it
will be used by atomic_commit to wait until the sync_file is signalled,
i.e., the framebuffer is ready for scanout.

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
---
 drivers/gpu/drm/drm_atomic.c        | 4 ++++
 drivers/gpu/drm/drm_atomic_helper.c | 1 +
 drivers/gpu/drm/drm_crtc.c          | 7 +++++++
 include/drm/drm_crtc.h              | 3 +++
 4 files changed, 15 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 8fb469c..8bc364c 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -609,6 +609,8 @@ 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_fence_fd) {
+		state->fence_fd = val;
 	} 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);
@@ -666,6 +668,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_fence_fd) {
+		*val = state->fence_fd;
 	} 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 2b430b0..4f91f84 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -2594,6 +2594,7 @@ void drm_atomic_helper_plane_reset(struct drm_plane *plane)
 	if (plane->state) {
 		plane->state->plane = plane;
 		plane->state->rotation = BIT(DRM_ROTATE_0);
+		plane->state->fence_fd = -1;
 	}
 }
 EXPORT_SYMBOL(drm_atomic_helper_plane_reset);
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 65258ac..165f199 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -1291,6 +1291,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_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);
@@ -1546,6 +1547,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,
+			"FENCE_FD", -1, INT_MAX);
+	if (!prop)
+		return -ENOMEM;
+	dev->mode_config.prop_fence_fd = 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 8c7fb3d..a8f6ec0 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -1239,6 +1239,7 @@ struct drm_connector {
  * @crtc: currently bound CRTC, NULL if disabled
  * @fb: currently bound framebuffer
  * @fence: optional fence to wait for before scanning out @fb
+ * @fence_fd: fd representing the sync_fence
  * @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
@@ -1257,6 +1258,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;
+	int fence_fd;
 
 	/* Signed dest location allows it to be partially off screen */
 	int32_t crtc_x, crtc_y;
@@ -2098,6 +2100,7 @@ struct drm_mode_config {
 	struct drm_property *prop_crtc_w;
 	struct drm_property *prop_crtc_h;
 	struct drm_property *prop_fb_id;
+	struct drm_property *prop_fence_fd;
 	struct drm_property *prop_crtc_id;
 	struct drm_property *prop_active;
 	struct drm_property *prop_mode_id;
-- 
2.5.0

[toc] | [prev] | [next] | [standalone]

#1363963

Hey,

Op 23-03-16 om 19:47 schreef Gustavo Padovan:
> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
>
> Hi,
>
> This is a first proposal to discuss the addition of in-fences support
> to DRM. It adds a new struct to fence.c to abstract the use of sync_file
> in DRM drivers. The new struct fence_collection contains a array with all
> fences that a atomic commit needs to wait on
>
> /**
>  * struct fence_collection - aggregate fences together
>  * @num_fences: number of fence in the collection.
>  * @user_data: user data.
>  * @func: user callback to put user data.
>  * @fences: array of @num_fences fences.
>  */
> struct fence_collection {
>        int num_fences;
>        void *user_data;
>        collection_put_func_t func;
>        struct fence *fences[];
> };
>
>
> The fence_collection is allocated and filled by sync_file_fences_get() and
> atomic_commit helpers can use fence_collection_wait() to wait the fences to
> signal.
>
> These patches depends on the sync ABI rework:
>
> https://www.spinics.net/lists/dri-devel/msg102795.html
>
> and the patch to de-stage the sync framework:
>
> https://www.spinics.net/lists/dri-devel/msg102799.html
>
>
> I also hacked together some sync support into modetest for testing:
>
> https://git.collabora.com/cgit/user/padovan/libdrm.git/log/?h=atomic
>
Why did you choose to add fence_collection, rather than putting sync_file in state?

There used to be a sync_fence_wait function, which would mean you'd have everything you need.

~Maarten

[toc] | [prev] | [next] | [standalone]

#1364208

Hi Maarten,

2016-03-24 Maarten Lankhorst <maarten.lankhorst@linux.intel.com>:

> Hey,
> 
> Op 23-03-16 om 19:47 schreef Gustavo Padovan:
> > From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> >
> > Hi,
> >
> > This is a first proposal to discuss the addition of in-fences support
> > to DRM. It adds a new struct to fence.c to abstract the use of sync_file
> > in DRM drivers. The new struct fence_collection contains a array with all
> > fences that a atomic commit needs to wait on
> >
> > /**
> >  * struct fence_collection - aggregate fences together
> >  * @num_fences: number of fence in the collection.
> >  * @user_data: user data.
> >  * @func: user callback to put user data.
> >  * @fences: array of @num_fences fences.
> >  */
> > struct fence_collection {
> >        int num_fences;
> >        void *user_data;
> >        collection_put_func_t func;
> >        struct fence *fences[];
> > };
> >
> >
> > The fence_collection is allocated and filled by sync_file_fences_get() and
> > atomic_commit helpers can use fence_collection_wait() to wait the fences to
> > signal.
> >
> > These patches depends on the sync ABI rework:
> >
> > https://www.spinics.net/lists/dri-devel/msg102795.html
> >
> > and the patch to de-stage the sync framework:
> >
> > https://www.spinics.net/lists/dri-devel/msg102799.html
> >
> >
> > I also hacked together some sync support into modetest for testing:
> >
> > https://git.collabora.com/cgit/user/padovan/libdrm.git/log/?h=atomic
> >
> Why did you choose to add fence_collection, rather than putting sync_file in state?
> 
> There used to be a sync_fence_wait function, which would mean you'd have everything you need.

We discussed this on #dri-devel a few days ago. The idea behind this is
to abstract sync_file from any drm driver and let only drm core deal
with sync_file. 

In the next iteration even fence_collection will be gone, so the driver
we deal only with struct fence and the fence_collection will be a
subclass of fence.

	Gustavo

[toc] | [prev] | [next] | [standalone]

#1364004

Hi,

2016년 03월 24일 03:47에 Gustavo Padovan 이(가) 쓴 글:
> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> 
> Hi,
> 
> This is a first proposal to discuss the addition of in-fences support
> to DRM. It adds a new struct to fence.c to abstract the use of sync_file
> in DRM drivers. The new struct fence_collection contains a array with all
> fences that a atomic commit needs to wait on

As I mentioned already like below,
http://www.spinics.net/lists/dri-devel/msg103225.html

I don't see why Android specific thing is tried to propagate to Linux DRM. In Linux mainline, it has already implicit sync interfaces for DMA devices called dma fence which forces registering a fence obejct to DMABUF through a reservation obejct when a dmabuf object is created. However, Android sync driver creates a new file for a sync object and this would have different point of view.

Is there anyone who can explan why Android specific thing is tried to spread into Linux DRM? Was there any consensus to use Android sync driver - which uses explicit sync interfaces - as Linux standard?

Thanks,
Inki Dae

> 
> /**
>  * struct fence_collection - aggregate fences together
>  * @num_fences: number of fence in the collection.
>  * @user_data: user data.
>  * @func: user callback to put user data.
>  * @fences: array of @num_fences fences.
>  */
> struct fence_collection {
>        int num_fences;
>        void *user_data;
>        collection_put_func_t func;
>        struct fence *fences[];
> };
> 
> 
> The fence_collection is allocated and filled by sync_file_fences_get() and
> atomic_commit helpers can use fence_collection_wait() to wait the fences to
> signal.
> 
> These patches depends on the sync ABI rework:
> 
> https://www.spinics.net/lists/dri-devel/msg102795.html
> 
> and the patch to de-stage the sync framework:
> 
> https://www.spinics.net/lists/dri-devel/msg102799.html
> 
> 
> I also hacked together some sync support into modetest for testing:
> 
> https://git.collabora.com/cgit/user/padovan/libdrm.git/log/?h=atomic
> 
> 
> 	Gustavo
> 
> 
> Gustavo Padovan (6):
>   drm/fence: add FENCE_FD property to planes
>   dma-buf/fence: add struct fence_collection
>   dma-buf/sync_file: add sync_file_fences_get()
>   dma-buf/fence: add fence_collection_put()
>   dma-buf/fence: add fence_collection_wait()
>   drm/fence: support fence_collection on atomic commit
> 
>  drivers/dma-buf/fence.c             | 33 +++++++++++++++++++++++++++++++++
>  drivers/dma-buf/sync_file.c         | 36 ++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/drm_atomic.c        | 13 +++++++++++++
>  drivers/gpu/drm/drm_atomic_helper.c | 10 ++++++----
>  drivers/gpu/drm/drm_crtc.c          |  7 +++++++
>  include/drm/drm_crtc.h              |  5 ++++-
>  include/linux/fence.h               | 19 +++++++++++++++++++
>  include/linux/sync_file.h           |  8 ++++++++
>  8 files changed, 126 insertions(+), 5 deletions(-)
> 

[toc] | [prev] | [next] | [standalone]

#1364229

Hi Inki,

2016-03-24 Inki Dae <inki.dae@samsung.com>:

> Hi,
> 
> 2016년 03월 24일 03:47에 Gustavo Padovan 이(가) 쓴 글:
> > From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> >
> > Hi,
> >
> > This is a first proposal to discuss the addition of in-fences support
> > to DRM. It adds a new struct to fence.c to abstract the use of sync_file
> > in DRM drivers. The new struct fence_collection contains a array with all
> > fences that a atomic commit needs to wait on
> 
> As I mentioned already like below,
> http://www.spinics.net/lists/dri-devel/msg103225.html
> 
> I don't see why Android specific thing is tried to propagate to Linux DRM. In Linux mainline, it has already implicit sync interfaces for DMA devices called dma fence which forces registering a fence obejct to DMABUF through a reservation obejct when a dmabuf object is created. However, Android sync driver creates a new file for a sync object and this would have different point of view.
> 
> Is there anyone who can explan why Android specific thing is tried to spread into Linux DRM? Was there any consensus to use Android sync driver - which uses explicit sync interfaces - as Linux standard?

Because we want explicit fencing as the Linux standard in the future to
be able to do smart scheduling, e.g., send async jobs to the gpu and at
the same time send async atomic commits with sync_file fd attached so
they can wait the GPU to finish and we don't block in userspace anymore,
quite similar to what Android does.

This would still use dma-buf fences in the driver level, but it has a
lot more advantages than implicit fencing.

	Gustavo

[toc] | [prev] | [next] | [standalone]

#1364479

Hi Guestavo,

2016년 03월 24일 23:39에 Gustavo Padovan 이(가) 쓴 글:
> Hi Inki,
> 
> 2016-03-24 Inki Dae <inki.dae@samsung.com>:
> 
>> Hi,
>>
>> 2016년 03월 24일 03:47에 Gustavo Padovan 이(가) 쓴 글:
>>> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
>>>
>>> Hi,
>>>
>>> This is a first proposal to discuss the addition of in-fences support
>>> to DRM. It adds a new struct to fence.c to abstract the use of sync_file
>>> in DRM drivers. The new struct fence_collection contains a array with all
>>> fences that a atomic commit needs to wait on
>>
>> As I mentioned already like below,
>> http://www.spinics.net/lists/dri-devel/msg103225.html
>>
>> I don't see why Android specific thing is tried to propagate to Linux DRM. In Linux mainline, it has already implicit sync interfaces for DMA devices called dma fence which forces registering a fence obejct to DMABUF through a reservation obejct when a dmabuf object is created. However, Android sync driver creates a new file for a sync object and this would have different point of view.
>>
>> Is there anyone who can explan why Android specific thing is tried to spread into Linux DRM? Was there any consensus to use Android sync driver - which uses explicit sync interfaces - as Linux standard?
> 
> Because we want explicit fencing as the Linux standard in the future to
> be able to do smart scheduling, e.g., send async jobs to the gpu and at
> the same time send async atomic commits with sync_file fd attached so
> they can wait the GPU to finish and we don't block in userspace anymore,
> quite similar to what Android does.

GPU is also DMA device so I think the synchonization should be handled transparent to user-space.
And I know that Chromium guy already did similar thing with non-atomic commit only using implicit sync,
https://chromium.googlesource.com/chromiumos/third_party/kernel
branch name : chromeos-3.14

Of course, this approach uses a new helper framework placed in drm directory so I think if this framework can be moved into dma-buf directory after some cleanup and refactoring them if necessary.
Anyway, I'm not sure I understood the smart scheduling you mentioned but I think we could do what you try to do without the explicit fence.

> 
> This would still use dma-buf fences in the driver level, but it has a
> lot more advantages than implicit fencing.

You means things for rendering pipeline debugging and merging sync fences?

Thanks,
Inki Dae

> 
> 	Gustavo
> 
> 

[toc] | [prev] | [next] | [standalone]

#1364290

On Thu, Mar 24, 2016 at 4:18 AM, Inki Dae <inki.dae@samsung.com> wrote:
> Hi,
>
> 2016년 03월 24일 03:47에 Gustavo Padovan 이(가) 쓴 글:
>> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
>>
>> Hi,
>>
>> This is a first proposal to discuss the addition of in-fences support
>> to DRM. It adds a new struct to fence.c to abstract the use of sync_file
>> in DRM drivers. The new struct fence_collection contains a array with all
>> fences that a atomic commit needs to wait on
>
> As I mentioned already like below,
> http://www.spinics.net/lists/dri-devel/msg103225.html
>
> I don't see why Android specific thing is tried to propagate to Linux DRM. In Linux mainline, it has already implicit sync interfaces for DMA devices called dma fence which forces registering a fence obejct to DMABUF through a reservation obejct when a dmabuf object is created. However, Android sync driver creates a new file for a sync object and this would have different point of view.
>
> Is there anyone who can explan why Android specific thing is tried to spread into Linux DRM? Was there any consensus to use Android sync driver - which uses explicit sync interfaces - as Linux standard?
>

btw, there is already plane_state->fence .. which I don't think has
any users yet, but I start to use it in my patchset that converts
drm/msm to 'struct fence'

That said, we do need syncpt as the way to expose fences to userspace
for explicit synchronization, but I'm not entirely sure that the
various drivers ever need to see that (vs just struct fence), at least
on the kms side of things.

BR,
-R


> Thanks,
> Inki Dae
>
>>
>> /**
>>  * struct fence_collection - aggregate fences together
>>  * @num_fences: number of fence in the collection.
>>  * @user_data: user data.
>>  * @func: user callback to put user data.
>>  * @fences: array of @num_fences fences.
>>  */
>> struct fence_collection {
>>        int num_fences;
>>        void *user_data;
>>        collection_put_func_t func;
>>        struct fence *fences[];
>> };
>>
>>
>> The fence_collection is allocated and filled by sync_file_fences_get() and
>> atomic_commit helpers can use fence_collection_wait() to wait the fences to
>> signal.
>>
>> These patches depends on the sync ABI rework:
>>
>> https://www.spinics.net/lists/dri-devel/msg102795.html
>>
>> and the patch to de-stage the sync framework:
>>
>> https://www.spinics.net/lists/dri-devel/msg102799.html
>>
>>
>> I also hacked together some sync support into modetest for testing:
>>
>> https://git.collabora.com/cgit/user/padovan/libdrm.git/log/?h=atomic
>>
>>
>>       Gustavo
>>
>>
>> Gustavo Padovan (6):
>>   drm/fence: add FENCE_FD property to planes
>>   dma-buf/fence: add struct fence_collection
>>   dma-buf/sync_file: add sync_file_fences_get()
>>   dma-buf/fence: add fence_collection_put()
>>   dma-buf/fence: add fence_collection_wait()
>>   drm/fence: support fence_collection on atomic commit
>>
>>  drivers/dma-buf/fence.c             | 33 +++++++++++++++++++++++++++++++++
>>  drivers/dma-buf/sync_file.c         | 36 ++++++++++++++++++++++++++++++++++++
>>  drivers/gpu/drm/drm_atomic.c        | 13 +++++++++++++
>>  drivers/gpu/drm/drm_atomic_helper.c | 10 ++++++----
>>  drivers/gpu/drm/drm_crtc.c          |  7 +++++++
>>  include/drm/drm_crtc.h              |  5 ++++-
>>  include/linux/fence.h               | 19 +++++++++++++++++++
>>  include/linux/sync_file.h           |  8 ++++++++
>>  8 files changed, 126 insertions(+), 5 deletions(-)
>>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

[toc] | [prev] | [next] | [standalone]

#1364483

2016년 03월 25일 00:40에 Rob Clark 이(가) 쓴 글:
> On Thu, Mar 24, 2016 at 4:18 AM, Inki Dae <inki.dae@samsung.com> wrote:
>> Hi,
>>
>> 2016년 03월 24일 03:47에 Gustavo Padovan 이(가) 쓴 글:
>>> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
>>>
>>> Hi,
>>>
>>> This is a first proposal to discuss the addition of in-fences support
>>> to DRM. It adds a new struct to fence.c to abstract the use of sync_file
>>> in DRM drivers. The new struct fence_collection contains a array with all
>>> fences that a atomic commit needs to wait on
>>
>> As I mentioned already like below,
>> http://www.spinics.net/lists/dri-devel/msg103225.html
>>
>> I don't see why Android specific thing is tried to propagate to Linux DRM. In Linux mainline, it has already implicit sync interfaces for DMA devices called dma fence which forces registering a fence obejct to DMABUF through a reservation obejct when a dmabuf object is created. However, Android sync driver creates a new file for a sync object and this would have different point of view.
>>
>> Is there anyone who can explan why Android specific thing is tried to spread into Linux DRM? Was there any consensus to use Android sync driver - which uses explicit sync interfaces - as Linux standard?
>>
> 
> btw, there is already plane_state->fence .. which I don't think has
> any users yet, but I start to use it in my patchset that converts
> drm/msm to 'struct fence'

Yes, Exynos also started it.

> 
> That said, we do need syncpt as the way to expose fences to userspace
> for explicit synchronization, but I'm not entirely sure that the

It's definitely different case. This tries to add new user-space interfaces to expose fences to user-space. At least, implicit interfaces are embedded into drivers.
So I'd like to give you a question. Why exposing fences to user-space is required? To provide easy-to-debug solution to rendering pipleline? To provide merge fence feature? 

And if we need really to expose fences to user-space and there is really real user, then we have already good candidates, DMA-BUF-IOCTL-SYNC or maybe fcntl system call because we share already DMA buffer between CPU <-> DMA and DMA <-> DMA using DMABUF.
For DMA-BUF-IOCTL-SYNC, I think you remember that was what I tried long time ago because you was there. Several years ago, I tried to couple exposing the fences to user-space with cache operation although at that time, I really misleaded the fence machnism. My trying was also for the potential users.

Anyway, my opinion is that we could expose the fences hided by DMABUF to user-space using interfaces it exists already around us. And for this, below Chromium solution would also give us some helps,
https://chromium.googlesource.com/chromiumos/third_party/kernel/+/chromeos-3.18/drivers/gpu/drm/drm_sync_helper.c

And in /driver/dma-buf/, there are DMABUF-centric modules so looks strange sync_file module of Android is placed in that directory - Android sync driver doesn't use really DMABUF but creates new file for their sync fence instead. 
For implicit sync interfaces for DMA devices, we use DMABUF and for explict sync interfaces for user-space, we use sync_file not DMABUF? That doesn't make sense. 

I love really Android but I feel as if we try to give a seat available to Android somehow.

Thanks,
Inki Dae

> various drivers ever need to see that (vs just struct fence), at least
> on the kms side of things.
> 
> BR,
> -R
> 
> 
>> Thanks,
>> Inki Dae
>>
>>>
>>> /**
>>>  * struct fence_collection - aggregate fences together
>>>  * @num_fences: number of fence in the collection.
>>>  * @user_data: user data.
>>>  * @func: user callback to put user data.
>>>  * @fences: array of @num_fences fences.
>>>  */
>>> struct fence_collection {
>>>        int num_fences;
>>>        void *user_data;
>>>        collection_put_func_t func;
>>>        struct fence *fences[];
>>> };
>>>
>>>
>>> The fence_collection is allocated and filled by sync_file_fences_get() and
>>> atomic_commit helpers can use fence_collection_wait() to wait the fences to
>>> signal.
>>>
>>> These patches depends on the sync ABI rework:
>>>
>>> https://www.spinics.net/lists/dri-devel/msg102795.html
>>>
>>> and the patch to de-stage the sync framework:
>>>
>>> https://www.spinics.net/lists/dri-devel/msg102799.html
>>>
>>>
>>> I also hacked together some sync support into modetest for testing:
>>>
>>> https://git.collabora.com/cgit/user/padovan/libdrm.git/log/?h=atomic
>>>
>>>
>>>       Gustavo
>>>
>>>
>>> Gustavo Padovan (6):
>>>   drm/fence: add FENCE_FD property to planes
>>>   dma-buf/fence: add struct fence_collection
>>>   dma-buf/sync_file: add sync_file_fences_get()
>>>   dma-buf/fence: add fence_collection_put()
>>>   dma-buf/fence: add fence_collection_wait()
>>>   drm/fence: support fence_collection on atomic commit
>>>
>>>  drivers/dma-buf/fence.c             | 33 +++++++++++++++++++++++++++++++++
>>>  drivers/dma-buf/sync_file.c         | 36 ++++++++++++++++++++++++++++++++++++
>>>  drivers/gpu/drm/drm_atomic.c        | 13 +++++++++++++
>>>  drivers/gpu/drm/drm_atomic_helper.c | 10 ++++++----
>>>  drivers/gpu/drm/drm_crtc.c          |  7 +++++++
>>>  include/drm/drm_crtc.h              |  5 ++++-
>>>  include/linux/fence.h               | 19 +++++++++++++++++++
>>>  include/linux/sync_file.h           |  8 ++++++++
>>>  8 files changed, 126 insertions(+), 5 deletions(-)
>>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> 

[toc] | [prev] | [next] | [standalone]

#1364643

On Thu, Mar 24, 2016 at 7:49 PM, Inki Dae <inki.dae@samsung.com> wrote:
>
>
> 2016년 03월 25일 00:40에 Rob Clark 이(가) 쓴 글:
>> On Thu, Mar 24, 2016 at 4:18 AM, Inki Dae <inki.dae@samsung.com> wrote:
>>> Hi,
>>>
>>> 2016년 03월 24일 03:47에 Gustavo Padovan 이(가) 쓴 글:
>>>> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
>>>>
>>>> Hi,
>>>>
>>>> This is a first proposal to discuss the addition of in-fences support
>>>> to DRM. It adds a new struct to fence.c to abstract the use of sync_file
>>>> in DRM drivers. The new struct fence_collection contains a array with all
>>>> fences that a atomic commit needs to wait on
>>>
>>> As I mentioned already like below,
>>> http://www.spinics.net/lists/dri-devel/msg103225.html
>>>
>>> I don't see why Android specific thing is tried to propagate to Linux DRM. In Linux mainline, it has already implicit sync interfaces for DMA devices called dma fence which forces registering a fence obejct to DMABUF through a reservation obejct when a dmabuf object is created. However, Android sync driver creates a new file for a sync object and this would have different point of view.
>>>
>>> Is there anyone who can explan why Android specific thing is tried to spread into Linux DRM? Was there any consensus to use Android sync driver - which uses explicit sync interfaces - as Linux standard?
>>>
>>
>> btw, there is already plane_state->fence .. which I don't think has
>> any users yet, but I start to use it in my patchset that converts
>> drm/msm to 'struct fence'
>
> Yes, Exynos also started it.
>
>>
>> That said, we do need syncpt as the way to expose fences to userspace
>> for explicit synchronization, but I'm not entirely sure that the
>
> It's definitely different case. This tries to add new user-space interfaces to expose fences to user-space. At least, implicit interfaces are embedded into drivers.
> So I'd like to give you a question. Why exposing fences to user-space is required? To provide easy-to-debug solution to rendering pipleline? To provide merge fence feature?
>

Well, implicit sync and explicit sync are two different cases.
Implicit sync ofc remains the default, but userspace could opt-in to
explicit sync instead.  For example, on the gpu side of things,
depending on flags userspace passes in to the submit ioctl we would
either attach the fence to all the written buffers (implicit) or
return it as a fence fd to userspace (explicit), which userspace could
then pass in to atomic ioctl to synchronize pageflip.

And visa-versa, we can pass the pageflip (atomic) completion fence
back in to gpu so it doesn't start rendering the next frame until the
buffer is off screen.

fwiw, currently android is the first user of explicit sync (although I
expect wayland/weston to follow suit).  A couple linaro folks have
android running with an upstream kernel + mesa + atomic/kms hwc on a
couple devices (nexus7 and db410c with freedreno, and qemu with
virgl).  But there are some limitations due to missing the
EGL_ANDROID_native_fence_sync extension in mesa.  I plan to implement
that, but I ofc need the fence fd stuff in order to do so ;-)

> And if we need really to expose fences to user-space and there is really real user, then we have already good candidates, DMA-BUF-IOCTL-SYNC or maybe fcntl system call because we share already DMA buffer between CPU <-> DMA and DMA <-> DMA using DMABUF.
> For DMA-BUF-IOCTL-SYNC, I think you remember that was what I tried long time ago because you was there. Several years ago, I tried to couple exposing the fences to user-space with cache operation although at that time, I really misleaded the fence machnism. My trying was also for the potential users.

Note that this is not (just) about sw sync, but also sync between
multiple hw devices.

BR,
-R

> Anyway, my opinion is that we could expose the fences hided by DMABUF to user-space using interfaces it exists already around us. And for this, below Chromium solution would also give us some helps,
> https://chromium.googlesource.com/chromiumos/third_party/kernel/+/chromeos-3.18/drivers/gpu/drm/drm_sync_helper.c
>
> And in /driver/dma-buf/, there are DMABUF-centric modules so looks strange sync_file module of Android is placed in that directory - Android sync driver doesn't use really DMABUF but creates new file for their sync fence instead.
> For implicit sync interfaces for DMA devices, we use DMABUF and for explict sync interfaces for user-space, we use sync_file not DMABUF? That doesn't make sense.
>
> I love really Android but I feel as if we try to give a seat available to Android somehow.
>
> Thanks,
> Inki Dae
>
>> various drivers ever need to see that (vs just struct fence), at least
>> on the kms side of things.
>>
>> BR,
>> -R
>>
>>
>>> Thanks,
>>> Inki Dae
>>>
>>>>
>>>> /**
>>>>  * struct fence_collection - aggregate fences together
>>>>  * @num_fences: number of fence in the collection.
>>>>  * @user_data: user data.
>>>>  * @func: user callback to put user data.
>>>>  * @fences: array of @num_fences fences.
>>>>  */
>>>> struct fence_collection {
>>>>        int num_fences;
>>>>        void *user_data;
>>>>        collection_put_func_t func;
>>>>        struct fence *fences[];
>>>> };
>>>>
>>>>
>>>> The fence_collection is allocated and filled by sync_file_fences_get() and
>>>> atomic_commit helpers can use fence_collection_wait() to wait the fences to
>>>> signal.
>>>>
>>>> These patches depends on the sync ABI rework:
>>>>
>>>> https://www.spinics.net/lists/dri-devel/msg102795.html
>>>>
>>>> and the patch to de-stage the sync framework:
>>>>
>>>> https://www.spinics.net/lists/dri-devel/msg102799.html
>>>>
>>>>
>>>> I also hacked together some sync support into modetest for testing:
>>>>
>>>> https://git.collabora.com/cgit/user/padovan/libdrm.git/log/?h=atomic
>>>>
>>>>
>>>>       Gustavo
>>>>
>>>>
>>>> Gustavo Padovan (6):
>>>>   drm/fence: add FENCE_FD property to planes
>>>>   dma-buf/fence: add struct fence_collection
>>>>   dma-buf/sync_file: add sync_file_fences_get()
>>>>   dma-buf/fence: add fence_collection_put()
>>>>   dma-buf/fence: add fence_collection_wait()
>>>>   drm/fence: support fence_collection on atomic commit
>>>>
>>>>  drivers/dma-buf/fence.c             | 33 +++++++++++++++++++++++++++++++++
>>>>  drivers/dma-buf/sync_file.c         | 36 ++++++++++++++++++++++++++++++++++++
>>>>  drivers/gpu/drm/drm_atomic.c        | 13 +++++++++++++
>>>>  drivers/gpu/drm/drm_atomic_helper.c | 10 ++++++----
>>>>  drivers/gpu/drm/drm_crtc.c          |  7 +++++++
>>>>  include/drm/drm_crtc.h              |  5 ++++-
>>>>  include/linux/fence.h               | 19 +++++++++++++++++++
>>>>  include/linux/sync_file.h           |  8 ++++++++
>>>>  8 files changed, 126 insertions(+), 5 deletions(-)
>>>>
>>> _______________________________________________
>>> dri-devel mailing list
>>> dri-devel@lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>
>>

[toc] | [prev] | [next] | [standalone]

#1364647

Hi all,

On 25 March 2016 at 11:58, Rob Clark <robdclark@gmail.com> wrote:
> On Thu, Mar 24, 2016 at 7:49 PM, Inki Dae <inki.dae@samsung.com> wrote:
>> It's definitely different case. This tries to add new user-space interfaces to expose fences to user-space. At least, implicit interfaces are embedded into drivers.
>> So I'd like to give you a question. Why exposing fences to user-space is required? To provide easy-to-debug solution to rendering pipleline? To provide merge fence feature?
>
> Well, implicit sync and explicit sync are two different cases.
> Implicit sync ofc remains the default, but userspace could opt-in to
> explicit sync instead.  For example, on the gpu side of things,
> depending on flags userspace passes in to the submit ioctl we would
> either attach the fence to all the written buffers (implicit) or
> return it as a fence fd to userspace (explicit), which userspace could
> then pass in to atomic ioctl to synchronize pageflip.
>
> And visa-versa, we can pass the pageflip (atomic) completion fence
> back in to gpu so it doesn't start rendering the next frame until the
> buffer is off screen.
>
> fwiw, currently android is the first user of explicit sync (although I
> expect wayland/weston to follow suit).

Second, really. Vulkan avoids implicit sync entirely, and exposes
fence-like primitives throughout its whole API. These include being
able to pass prerequisite fences for display (what Gustavo is adding
here: something to block on before display), and also when the user
acquires a buffer as a render target, it is given another prerequisite
fence (the other side of what Gustavo is suggesting, i.e. the fence
triggers when the buffer is no longer displayed and becomes available
for rendering).

In order to implement this correctly, and avoid performance bubbles,
we need a primitive like this exposed through the KMS API, from both
sides. This is especially important when you take the case of
userspace suballocation, where userspace allocates larger blocks and
divides the allocation internally for different uses. Implicit sync
does not work at all for that case.

As stated before, there are other benefits, including much better
traceability. I would expect Wayland/Weston to also start pushing
support for this API relatively soon.

> A couple linaro folks have
> android running with an upstream kernel + mesa + atomic/kms hwc on a
> couple devices (nexus7 and db410c with freedreno, and qemu with
> virgl).  But there are some limitations due to missing the
> EGL_ANDROID_native_fence_sync extension in mesa.  I plan to implement
> that, but I ofc need the fence fd stuff in order to do so ;-)

Yes, having that would be a godsend for a lot of people.

>> And if we need really to expose fences to user-space and there is really real user, then we have already good candidates, DMA-BUF-IOCTL-SYNC or maybe fcntl system call because we share already DMA buffer between CPU <-> DMA and DMA <-> DMA using DMABUF.
>> For DMA-BUF-IOCTL-SYNC, I think you remember that was what I tried long time ago because you was there. Several years ago, I tried to couple exposing the fences to user-space with cache operation although at that time, I really misleaded the fence machnism. My trying was also for the potential users.
>
> Note that this is not (just) about sw sync, but also sync between
> multiple hw devices.

Sync isn't quite good enough, because it's a mandatory blocking point
for userspace. We want to push the explicit fences further down the
line, so userspace can parallelise its work.

Even if none of the above requirements held true, I don't think being
able to support Android is a bad thing. It's completely right to be
worried about pushing in Android work and APIs for the sake of it -
hence why we didn't take ADF! - but in this case it's definitely a
good thing. This is also the model that ChromeOS is moving towards, so
it becomes more important from that point of view as well.

Cheers,
Daniel

[toc] | [prev] | [next] | [standalone]

#1365258

Hi Rob and Daniel,

2016년 03월 25일 21:10에 Daniel Stone 이(가) 쓴 글:
> Hi all,
> 
> On 25 March 2016 at 11:58, Rob Clark <robdclark@gmail.com> wrote:
>> On Thu, Mar 24, 2016 at 7:49 PM, Inki Dae <inki.dae@samsung.com> wrote:
>>> It's definitely different case. This tries to add new user-space interfaces to expose fences to user-space. At least, implicit interfaces are embedded into drivers.
>>> So I'd like to give you a question. Why exposing fences to user-space is required? To provide easy-to-debug solution to rendering pipleline? To provide merge fence feature?
>>
>> Well, implicit sync and explicit sync are two different cases.
>> Implicit sync ofc remains the default, but userspace could opt-in to
>> explicit sync instead.  For example, on the gpu side of things,
>> depending on flags userspace passes in to the submit ioctl we would
>> either attach the fence to all the written buffers (implicit) or
>> return it as a fence fd to userspace (explicit), which userspace could
>> then pass in to atomic ioctl to synchronize pageflip.
>>
>> And visa-versa, we can pass the pageflip (atomic) completion fence
>> back in to gpu so it doesn't start rendering the next frame until the
>> buffer is off screen.
>>
>> fwiw, currently android is the first user of explicit sync (although I
>> expect wayland/weston to follow suit).
> 
> Second, really. Vulkan avoids implicit sync entirely, and exposes
> fence-like primitives throughout its whole API. These include being
> able to pass prerequisite fences for display (what Gustavo is adding
> here: something to block on before display), and also when the user
> acquires a buffer as a render target, it is given another prerequisite
> fence (the other side of what Gustavo is suggesting, i.e. the fence
> triggers when the buffer is no longer displayed and becomes available
> for rendering).
> 
> In order to implement this correctly, and avoid performance bubbles,
> we need a primitive like this exposed through the KMS API, from both
> sides. This is especially important when you take the case of
> userspace suballocation, where userspace allocates larger blocks and
> divides the allocation internally for different uses. Implicit sync
> does not work at all for that case.

Can you give me more details why implicit sync cannot take care of the case of userspace suballocation?
And is there any reason that fence fd shouldn't dependent of DMABUF - now fence fd is a new file, not DMABUF fd?

> 
> As stated before, there are other benefits, including much better
> traceability. I would expect Wayland/Weston to also start pushing
> support for this API relatively soon.
> 
>> A couple linaro folks have
>> android running with an upstream kernel + mesa + atomic/kms hwc on a
>> couple devices (nexus7 and db410c with freedreno, and qemu with
>> virgl).  But there are some limitations due to missing the
>> EGL_ANDROID_native_fence_sync extension in mesa.  I plan to implement
>> that, but I ofc need the fence fd stuff in order to do so ;-)
> 
> Yes, having that would be a godsend for a lot of people.
> 
>>> And if we need really to expose fences to user-space and there is really real user, then we have already good candidates, DMA-BUF-IOCTL-SYNC or maybe fcntl system call because we share already DMA buffer between CPU <-> DMA and DMA <-> DMA using DMABUF.
>>> For DMA-BUF-IOCTL-SYNC, I think you remember that was what I tried long time ago because you was there. Several years ago, I tried to couple exposing the fences to user-space with cache operation although at that time, I really misleaded the fence machnism. My trying was also for the potential users.
>>
>> Note that this is not (just) about sw sync, but also sync between
>> multiple hw devices.
> 
> Sync isn't quite good enough, because it's a mandatory blocking point
> for userspace. We want to push the explicit fences further down the
> line, so userspace can parallelise its work.
> 
> Even if none of the above requirements held true, I don't think being
> able to support Android is a bad thing. It's completely right to be
> worried about pushing in Android work and APIs for the sake of it -
> hence why we didn't take ADF! - but in this case it's definitely a

As least Google's ADF boosted up atomic KMS. :) 

> good thing. This is also the model that ChromeOS is moving towards, so
> it becomes more important from that point of view as well.

I think Gustavo should had explaned this path series enough to other people when posting them - ie, what relationship explict and implicit fences have, and why implicit fence - which is independent of DMABUF - is required, and what use cases there are in real users, and etc.


Thanks,
Inki Dae

> 
> Cheers,
> Daniel
> 
> 

[toc] | [prev] | [next] | [standalone]

#1365485

Hi Inki,

On 28 March 2016 at 02:26, Inki Dae <inki.dae@samsung.com> wrote:
> 2016년 03월 25일 21:10에 Daniel Stone 이(가) 쓴 글:
>> Second, really. Vulkan avoids implicit sync entirely, and exposes
>> fence-like primitives throughout its whole API. These include being
>> able to pass prerequisite fences for display (what Gustavo is adding
>> here: something to block on before display), and also when the user
>> acquires a buffer as a render target, it is given another prerequisite
>> fence (the other side of what Gustavo is suggesting, i.e. the fence
>> triggers when the buffer is no longer displayed and becomes available
>> for rendering).
>>
>> In order to implement this correctly, and avoid performance bubbles,
>> we need a primitive like this exposed through the KMS API, from both
>> sides. This is especially important when you take the case of
>> userspace suballocation, where userspace allocates larger blocks and
>> divides the allocation internally for different uses. Implicit sync
>> does not work at all for that case.
>
> Can you give me more details why implicit sync cannot take care of the case of userspace suballocation?

Implicit sync does not know about suballocation, so implicit will
operate for every range in the buffer, not just the one you want.

Say you have one kernel buffer, which userspace subdivides into four
independent buffers. It can perform operations on these buffers which
are completely independent of each other, and an explicit sync model
allows this independence to be kept. Implicit sync ties them together,
so that you cannot do any operations on buffer 1 until all operations
on buffer 2 have completed.

> And is there any reason that fence fd shouldn't dependent of DMABUF - now fence fd is a new file, not DMABUF fd?

Because dmabuf is for buffer sharing, and fences aren't buffers (they
will never export page ranges). Is there any particular benefit you
think you would get from doing this?

>> good thing. This is also the model that ChromeOS is moving towards, so
>> it becomes more important from that point of view as well.
>
> I think Gustavo should had explaned this path series enough to other people when posting them - ie, what relationship explict and implicit fences have, and why implicit fence - which is independent of DMABUF - is required, and what use cases there are in real users, and etc.

Fair enough, the summary could perhaps contain something like this.

Cheers,
Daniel

[toc] | [prev] | [next] | [standalone]

#1365802

Hi Daniel,

2016년 03월 28일 22:26에 Daniel Stone 이(가) 쓴 글:
> Hi Inki,
> 
> On 28 March 2016 at 02:26, Inki Dae <inki.dae@samsung.com> wrote:
>> 2016년 03월 25일 21:10에 Daniel Stone 이(가) 쓴 글:
>>> Second, really. Vulkan avoids implicit sync entirely, and exposes
>>> fence-like primitives throughout its whole API. These include being
>>> able to pass prerequisite fences for display (what Gustavo is adding
>>> here: something to block on before display), and also when the user
>>> acquires a buffer as a render target, it is given another prerequisite
>>> fence (the other side of what Gustavo is suggesting, i.e. the fence
>>> triggers when the buffer is no longer displayed and becomes available
>>> for rendering).
>>>
>>> In order to implement this correctly, and avoid performance bubbles,
>>> we need a primitive like this exposed through the KMS API, from both
>>> sides. This is especially important when you take the case of
>>> userspace suballocation, where userspace allocates larger blocks and
>>> divides the allocation internally for different uses. Implicit sync
>>> does not work at all for that case.
>>
>> Can you give me more details why implicit sync cannot take care of the case of userspace suballocation?
> 
> Implicit sync does not know about suballocation, so implicit will
> operate for every range in the buffer, not just the one you want.
> 
> Say you have one kernel buffer, which userspace subdivides into four
> independent buffers. It can perform operations on these buffers which
> are completely independent of each other, and an explicit sync model
> allows this independence to be kept. Implicit sync ties them together,
> so that you cannot do any operations on buffer 1 until all operations
> on buffer 2 have completed.
> 
>> And is there any reason that fence fd shouldn't dependent of DMABUF - now fence fd is a new file, not DMABUF fd?
> 
> Because dmabuf is for buffer sharing, and fences aren't buffers (they
> will never export page ranges). Is there any particular benefit you
> think you would get from doing this?

Just for consistency. As you know, implicit sync hangs DMA fence up on dmabuf object through reservation object so dmabuf independent explicit sync looked strange to me.
As you mentioned above, the suballocation would be why explicit sync should be indepenent of DMABUF.

In addition, I wonder how explicit and implicit fences could coexist together.
Rob said,
"Implicit sync ofc remains the default, but userspace could opt-in to explicit sync instead"

This would mean that if we use explicit sync for user-space then it coexists with implicit sync. However, these two sync fences can't see same DMA buffer because explicit fence has a different file object from implicit one.
So in this case, I think explicit fence would need to be hung up on the reservation object of dmabuf object somehow. Otherwise, although they coexist together, are these fences - explicit and implicit - used for differenct purpose separately? 

Thanks,
Inki Dae

> 
>>> good thing. This is also the model that ChromeOS is moving towards, so
>>> it becomes more important from that point of view as well.
>>
>> I think Gustavo should had explaned this path series enough to other people when posting them - ie, what relationship explict and implicit fences have, and why implicit fence - which is independent of DMABUF - is required, and what use cases there are in real users, and etc.
> 
> Fair enough, the summary could perhaps contain something like this.
> 
> Cheers,
> Daniel
> 
> 

[toc] | [prev] | [next] | [standalone]

#1366285

On Mon, Mar 28, 2016 at 10:18 PM, Inki Dae <inki.dae@samsung.com> wrote:
>
> In addition, I wonder how explicit and implicit fences could coexist together.
> Rob said,
> "Implicit sync ofc remains the default, but userspace could opt-in to explicit sync instead"
>
> This would mean that if we use explicit sync for user-space then it coexists with implicit sync. However, these two sync fences can't see same DMA buffer because explicit fence has a different file object from implicit one.
> So in this case, I think explicit fence would need to be hung up on the reservation object of dmabuf object somehow. Otherwise, although they coexist together, are these fences - explicit and implicit - used for differenct purpose separately?
>

I'm not entirely sure about coexistance at the same time.  It ofc
shouldn't be a problem for one kernel to support both kinds of
userspace (pure explicit and pure implicit).  And how this would work
on kms atomic ioctl (compositor/consumer) side seems clear enough..
ie. some sort of flag, which if set user provides an explicit fence
fd, and if not set we fall back to current behaviour (ie. get fences
from resv object).

On the gpu/producer side, I think what makes sense is to both attach
the fence to the resv objects and (optionally, specified by an submit
ioctl flag) return a fence fd.  The other option is to add a new ioctl
to convert an internal per-ring fence/seqno (that is already returned
by submit ioctl) to a fence fd.. but I think that would end up with
duplicate 'struct fence' objects on the kernel side (not sure if that
would cause issues somehow), and might be unneeded since with
EGL_ANDROID_native_fence_sync since we should know before glFlush() is
called whether we want an fd or not.  But main thing I'm pondering
here is how to sanely support the old way of userspace gl driver
internal synchronization with new userspace on old kernel, but also
conditionally support EGL_ANDROID_native_fence_sync if you have a new
enough kernel.

BR,
-R

[toc] | [prev] | [next] | [standalone]

#1367915

2016년 03월 29일 22:23에 Rob Clark 이(가) 쓴 글:
> On Mon, Mar 28, 2016 at 10:18 PM, Inki Dae <inki.dae@samsung.com> wrote:
>>
>> In addition, I wonder how explicit and implicit fences could coexist together.
>> Rob said,
>> "Implicit sync ofc remains the default, but userspace could opt-in to explicit sync instead"
>>
>> This would mean that if we use explicit sync for user-space then it coexists with implicit sync. However, these two sync fences can't see same DMA buffer because explicit fence has a different file object from implicit one.
>> So in this case, I think explicit fence would need to be hung up on the reservation object of dmabuf object somehow. Otherwise, although they coexist together, are these fences - explicit and implicit - used for differenct purpose separately?
>>
> 
> I'm not entirely sure about coexistance at the same time.  It ofc
> shouldn't be a problem for one kernel to support both kinds of
> userspace (pure explicit and pure implicit).  And how this would work
> on kms atomic ioctl (compositor/consumer) side seems clear enough..
> ie. some sort of flag, which if set user provides an explicit fence
> fd, and if not set we fall back to current behaviour (ie. get fences
> from resv object).

With this patch series, users can register explicit fence(s) to atomic kms(consumer side) through kms property interface for the explicit sync.

However, now several DRM drivers(also consumer) already have beeen using implicit fence. So while GPU(producer side) is accessing DMA buffer after registering its explicit fence to atomic kms, and if atomic commit is requested by user-space, then atomic helper framework will try to synchronize with the producer - waiting for the signal of GPU side(producer), and device specific page flip function will also try to do same thing.

As of now, it seems that this wouldn't be optional but mandatory if explicit fence support is added to the atomic helper framework. This would definitely be duplication and it seems not clear enough even if one of them is just skipped in runtime.

> 
> On the gpu/producer side, I think what makes sense is to both attach
> the fence to the resv objects and (optionally, specified by an submit
> ioctl flag) return a fence fd.  The other option is to add a new ioctl
> to convert an internal per-ring fence/seqno (that is already returned
> by submit ioctl) to a fence fd.. but I think that would end up with
> duplicate 'struct fence' objects on the kernel side (not sure if that

I think the problem is not that kernel just keeps duplicate fence objects separately but is that these fences can be performed separately for same purpose.

> would cause issues somehow), and might be unneeded since with
> EGL_ANDROID_native_fence_sync since we should know before glFlush() is
> called whether we want an fd or not.  But main thing I'm pondering

So I think this is not user-space issue. All users don't have to know whether DMA drivers support implicit fence or not so as soon as user uses explicit fence, the duplication would happen.

There may be something I missed so your comment would be helpful in understanding it.


Thanks,
Inki Dae

> here is how to sanely support the old way of userspace gl driver
> internal synchronization with new userspace on old kernel, but also
> conditionally support EGL_ANDROID_native_fence_sync if you have a new
> enough kernel.
> 
> BR,
> -R
> 
> 

[toc] | [prev] | [next] | [standalone]

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web