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

Groups > linux.kernel > #1504958 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH v5 0/4] drm: add explicit fencing Gustavo Padovan <gustavo@padovan.org> - 2016-10-20 17:00 +0200
    [PATCH v5 4/4] drm/fence: add out-fences support Gustavo Padovan <gustavo@padovan.org> - 2016-10-20 17:00 +0200
      Re: [PATCH v5 4/4] drm/fence: add out-fences support Ville Syrjälä <ville.syrjala@linux.intel.com> - 2016-10-20 17:40 +0200
        Re: [PATCH v5 4/4] drm/fence: add out-fences support Gustavo Padovan <gustavo.padovan@collabora.com> - 2016-10-20 18:00 +0200
          Re: [PATCH v5 4/4] drm/fence: add out-fences support Ville Syrjälä <ville.syrjala@linux.intel.com> - 2016-10-20 18:40 +0200
            Re: [PATCH v5 4/4] drm/fence: add out-fences support Ville Syrjälä <ville.syrjala@linux.intel.com> - 2016-10-20 21:20 +0200
              Re: [PATCH v5 4/4] drm/fence: add out-fences support Daniel Vetter <daniel@ffwll.ch> - 2016-10-21 15:00 +0200
                Re: [PATCH v5 4/4] drm/fence: add out-fences support Gustavo Padovan <gustavo@padovan.org> - 2016-10-21 15:10 +0200
      Re: [PATCH v5 4/4] drm/fence: add out-fences support Brian Starkey <brian.starkey@arm.com> - 2016-10-20 19:50 +0200
        Re: [PATCH v5 4/4] drm/fence: add out-fences support Gustavo Padovan <gustavo@padovan.org> - 2016-10-20 22:40 +0200
          Re: [PATCH v5 4/4] drm/fence: add out-fences support Brian Starkey <brian.starkey@arm.com> - 2016-10-21 13:00 +0200
            Re: [PATCH v5 4/4] drm/fence: add out-fences support Daniel Vetter <daniel@ffwll.ch> - 2016-10-21 15:10 +0200
      Re: [PATCH v5 4/4] drm/fence: add out-fences support Brian Starkey <brian.starkey@arm.com> - 2016-10-21 17:50 +0200
    Re: [PATCH v5 0/4] drm: add explicit fencing Daniel Vetter <daniel@ffwll.ch> - 2016-10-21 15:10 +0200

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

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

Hi,

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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


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

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


Userspace
---------

Fences support on drm_hwcomposer is currently a working in progress.

Regards,

        Gustavo

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

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

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

-- 
2.5.5

[toc] | [next] | [standalone]

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

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

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

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

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

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

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

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

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

v4: Create OUT_FENCE_PTR properties and remove old approach.

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

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

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

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

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

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

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

-- 
Ville Syrjälä
Intel OTC

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

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

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

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

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

Gustavo

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

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

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

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

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

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

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

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

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

-- 
Ville Syrjälä
Intel OTC

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

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

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

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

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

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

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

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

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

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

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

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

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


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

-- 
Ville Syrjälä
Intel OTC

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

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

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

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

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

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

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

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

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

Gustavo

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

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

Hi Gustavo,

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

Couple more comments below.

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

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

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

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

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

Thanks,
Brian

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

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

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

Hi Brian,

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

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

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

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

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

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

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

Gustavo

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

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

Hi Gustavo,

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

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

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

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

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

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

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

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

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

Thanks,
Brian

>Gustavo
>

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

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

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

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

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

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

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

Hi,

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

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

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

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

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

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

Cheers,
Brian

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

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

#1505911

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

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

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

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

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web