[PATCH 0/4] prevent stalls due to tiling changes and bo reuse

[PATCH 0/4] prevent stalls due to tiling changes and bo reuse

[Daniel Vetter]

Hi all,

This patch-set prevents stalls due to bo reuse in libdrm. After having
tried to fix the set_tiling stall both in libdrm and the kernel, I think
fixing this in the kernel is the better option: This way gem nicely
abstracts away the execution semantics. The only place where the
asynchronous nature of the gpu leaks is in the busy ioctl, which userspace
should call to check whether a mmap of the bo would stall.

Performance gains are minimal, but strictly positive (neglecting a 1% noise
level).  cairo perf traces on my i945:

old: without-patches
new: with-patches
Speedups
========
 xlib-rgba       firefox-planet-gnome-0    62418.67 (64895.61 1.98%) -> 57833.66
 (61105.65 2.92%):  1.08x speedup

A few traces more have small speedups ranging 1%-5%.

I expect this to perform better when all the other tiling related problems
are fixed - unfortunately I can't benchmark this because fixing tiling needs
portions of my pipelined fencing work (the exact fence lru accounting),
which is not (yet) ported to current drm-next.

Please review and consider merging for -next. Patches apply to current
drm-next.

Thanks, Daniel

Daniel Vetter (4):
  drm/i915: don't allow tiling changes on pinned buffers
  drm/i915: introduce i915_gem_object_adjust_fencing
  drm/i915: adjust fence register asynchronously on tiling changes
  drm/i915: report all active objects as busy

 drivers/gpu/drm/i915/i915_drv.h        |    8 +++-
 drivers/gpu/drm/i915/i915_gem.c        |   88 ++++++++++++++++++++++++++------
 drivers/gpu/drm/i915/i915_gem_tiling.c |   29 +++-------
 drivers/gpu/drm/i915/intel_display.c   |   16 ++----
 4 files changed, 92 insertions(+), 49 deletions(-)

[PATCH 1/4] drm/i915: don't allow tiling changes on pinned buffers

[Daniel Vetter]

Makes no sense and complicates matters for pipelined tiling changes.
So don't allow it and return -EBUSY.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_gem_tiling.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_tiling.c b/drivers/gpu/drm/i915/i915_gem_tiling.c
index 449157f..00f264d 100644
--- a/drivers/gpu/drm/i915/i915_gem_tiling.c
+++ b/drivers/gpu/drm/i915/i915_gem_tiling.c
@@ -287,6 +287,9 @@ i915_gem_set_tiling(struct drm_device *dev, void *data,
 		return -EINVAL;
 	}
 
+	if (obj_priv->pin_count)
+		return -EBUSY;
+
 	if (args->tiling_mode == I915_TILING_NONE) {
 		args->swizzle_mode = I915_BIT_6_SWIZZLE_NONE;
 		args->stride = 0;
-- 
1.6.6.1

[PATCH 2/4] drm/i915: introduce i915_gem_object_adjust_fencing

[Daniel Vetter]

Pipelining tiling changes will complicate all places that need
a correct fence register to be set up. So hide this complexity
behind a small helper function.

i915_gem_object_get_fence loses all it's external callers by these
changes, so convert it into a static function

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_drv.h      |    2 +-
 drivers/gpu/drm/i915/i915_gem.c      |   52 ++++++++++++++++++++++++++++-----
 drivers/gpu/drm/i915/intel_display.c |   16 ++--------
 3 files changed, 49 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 242993b..7aec3ec 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -918,7 +918,7 @@ void i915_gem_release_mmap(struct drm_gem_object *obj);
 void i915_gem_lastclose(struct drm_device *dev);
 uint32_t i915_get_gem_seqno(struct drm_device *dev);
 bool i915_seqno_passed(uint32_t seq1, uint32_t seq2);
-int i915_gem_object_get_fence_reg(struct drm_gem_object *obj);
+int i915_gem_object_adjust_fencing(struct drm_gem_object *obj);
 int i915_gem_object_put_fence_reg(struct drm_gem_object *obj);
 void i915_gem_retire_requests(struct drm_device *dev);
 void i915_gem_retire_work_handler(struct work_struct *work);
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 47c46ed..8e73a12 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -974,6 +974,10 @@ i915_gem_pwrite_ioctl(struct drm_device *dev, void *data,
 		ret = i915_gem_phys_pwrite(dev, obj, args, file_priv);
 	else if (obj_priv->tiling_mode == I915_TILING_NONE &&
 		 dev->gtt_total != 0) {
+		ret = i915_gem_object_adjust_fencing(obj);
+		if (ret != 0)
+			return ret;
+
 		ret = i915_gem_gtt_pwrite_fast(dev, obj, args, file_priv);
 		if (ret == -EFAULT) {
 			ret = i915_gem_gtt_pwrite_slow(dev, obj, args,
@@ -1191,12 +1195,9 @@ int i915_gem_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
 			goto unlock;
 	}
 
-	/* Need a new fence register? */
-	if (obj_priv->tiling_mode != I915_TILING_NONE) {
-		ret = i915_gem_object_get_fence_reg(obj);
-		if (ret)
-			goto unlock;
-	}
+	ret = i915_gem_object_adjust_fencing(obj);
+	if (ret)
+		goto unlock;
 
 	pfn = ((dev->agp->base + obj_priv->gtt_offset) >> PAGE_SHIFT) +
 		page_offset;
@@ -2475,7 +2476,7 @@ static int i915_find_fence_reg(struct drm_device *dev)
  * It then sets up the reg based on the object's properties: address, pitch
  * and tiling format.
  */
-int
+static int
 i915_gem_object_get_fence_reg(struct drm_gem_object *obj)
 {
 	struct drm_device *dev = obj->dev;
@@ -2536,6 +2537,41 @@ i915_gem_object_get_fence_reg(struct drm_gem_object *obj)
 }
 
 /**
+ * i915_gem_object_adjust_fencing - ensure correct fencing for an object
+ * @obj: object to map through a fence reg
+ *
+ * Tiling changes may be delayed untill fenced access is actually needed.
+ * Batchbuffer submitted via execbuf2 may not need a fence, so delaying
+ * gtt remapping and fence register changes (which all stall the gpu) as
+ * long as possible is beneficial.
+ *
+ * This function ensures that all delayed operations have been carried out and
+ * the object can be correctly accessed via fences.
+ */
+int
+i915_gem_object_adjust_fencing(struct drm_gem_object *obj)
+{
+	struct drm_i915_gem_object *obj_priv = to_intel_bo(obj);
+	int ret;
+
+	if (!i915_gem_object_fence_offset_ok(obj, obj_priv->tiling_mode)) {
+		ret = i915_gem_object_unbind(obj);
+		if (ret != 0)
+			return ret;
+		ret = i915_gem_object_bind_to_gtt(obj, 0);
+		if (ret != 0)
+			return ret;
+	}
+
+	if (obj_priv->tiling_mode != I915_TILING_NONE) {
+		ret = i915_gem_object_get_fence_reg(obj);
+		if (ret != 0)
+			return ret;
+	}
+	return 0;
+}
+
+/**
  * i915_gem_clear_fence_reg - clear out fence register info
  * @obj: object to clear
  *
@@ -3309,7 +3345,7 @@ i915_gem_object_pin_and_relocate(struct drm_gem_object *obj,
 	 * properly handle blits to/from tiled surfaces.
 	 */
 	if (need_fence) {
-		ret = i915_gem_object_get_fence_reg(obj);
+		ret = i915_gem_object_adjust_fencing(obj);
 		if (ret != 0) {
 			if (ret != -EBUSY && ret != -ERESTARTSYS)
 				DRM_ERROR("Failure to install fence: %d\n",
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 3836f56..56b673d 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -1249,18 +1249,10 @@ intel_pin_and_fence_fb_obj(struct drm_device *dev, struct drm_gem_object *obj)
 	if (ret != 0)
 		return ret;
 
-	/* Install a fence for tiled scan-out. Pre-i965 always needs a
-	 * fence, whereas 965+ only requires a fence if using
-	 * framebuffer compression.  For simplicity, we always install
-	 * a fence as the cost is not that onerous.
-	 */
-	if (obj_priv->fence_reg == I915_FENCE_REG_NONE &&
-	    obj_priv->tiling_mode != I915_TILING_NONE) {
-		ret = i915_gem_object_get_fence_reg(obj);
-		if (ret != 0) {
-			i915_gem_object_unpin(obj);
-			return ret;
-		}
+	ret = i915_gem_object_adjust_fencing(obj);
+	if (ret != 0) {
+		i915_gem_object_unpin(obj);
+		return ret;
 	}
 
 	return 0;
-- 
1.6.6.1

[PATCH 3/4] drm/i915: adjust fence registers asynchronously on tiling changes

[Daniel Vetter]

This avoids stalling on the gpu. With the preparation from the
previous patch, this is really just a small change.

Thanks to Owain Ainsworth <zerooa@googlemail.com> for coming up
with the idea for this patch and hashing out the implementation
with me on irc.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_drv.h        |    6 ++++++
 drivers/gpu/drm/i915/i915_gem.c        |    9 +++++++++
 drivers/gpu/drm/i915/i915_gem_tiling.c |   26 ++++++--------------------
 3 files changed, 21 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 7aec3ec..174269c 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -698,6 +698,12 @@ struct drm_i915_gem_object {
 	 */
 	int fence_reg;
 
+	/**
+	 * Tiling changes can happen asynchronous to avoid gpu stalls.
+	 * This will be set if the current fencing may be invalid.
+	 */
+	int fence_invalid;
+
 	/** How many users have pinned this object in GTT space */
 	int pin_count;
 
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 8e73a12..8e18c9d 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2485,6 +2485,8 @@ i915_gem_object_get_fence_reg(struct drm_gem_object *obj)
 	struct drm_i915_fence_reg *reg = NULL;
 	int ret;
 
+	BUG_ON(obj_priv->fence_invalid);
+
 	/* Just update our place in the LRU if our fence is getting used. */
 	if (obj_priv->fence_reg != I915_FENCE_REG_NONE) {
 		list_move_tail(&obj_priv->fence_list, &dev_priv->mm.fence_list);
@@ -2554,6 +2556,13 @@ i915_gem_object_adjust_fencing(struct drm_gem_object *obj)
 	struct drm_i915_gem_object *obj_priv = to_intel_bo(obj);
 	int ret;
 
+	if (obj_priv->fence_invalid) {
+		ret = i915_gem_object_put_fence_reg(obj);
+		if (ret != 0)
+			return ret;
+	}
+	obj_priv->fence_invalid = 0;
+
 	if (!i915_gem_object_fence_offset_ok(obj, obj_priv->tiling_mode)) {
 		ret = i915_gem_object_unbind(obj);
 		if (ret != 0)
diff --git a/drivers/gpu/drm/i915/i915_gem_tiling.c b/drivers/gpu/drm/i915/i915_gem_tiling.c
index 00f264d..aadaaa6 100644
--- a/drivers/gpu/drm/i915/i915_gem_tiling.c
+++ b/drivers/gpu/drm/i915/i915_gem_tiling.c
@@ -322,30 +322,16 @@ i915_gem_set_tiling(struct drm_device *dev, void *data,
 	mutex_lock(&dev->struct_mutex);
 	if (args->tiling_mode != obj_priv->tiling_mode ||
 	    args->stride != obj_priv->stride) {
-		/* We need to rebind the object if its current allocation
-		 * no longer meets the alignment restrictions for its new
-		 * tiling mode. Otherwise we can just leave it alone, but
-		 * need to ensure that any fence register is cleared.
-		 */
-		if (!i915_gem_object_fence_offset_ok(obj, args->tiling_mode))
-			ret = i915_gem_object_unbind(obj);
-		else if (obj_priv->fence_reg != I915_FENCE_REG_NONE)
-			ret = i915_gem_object_put_fence_reg(obj);
-		else
-			i915_gem_release_mmap(obj);
-
-		if (ret != 0) {
-			WARN(ret != -ERESTARTSYS,
-			     "failed to reset object for tiling switch");
-			args->tiling_mode = obj_priv->tiling_mode;
-			args->stride = obj_priv->stride;
-			goto err;
-		}
+		/* Change tiling parameter asynchronously to avoid
+		 * gpu stalls. */
+		obj_priv->fence_invalid = 1;
+
+		i915_gem_release_mmap(obj);
 
 		obj_priv->tiling_mode = args->tiling_mode;
 		obj_priv->stride = args->stride;
 	}
-err:
+
 	drm_gem_object_unreference(obj);
 	mutex_unlock(&dev->struct_mutex);
 
-- 
1.6.6.1

[PATCH 4/4] drm/i915: report all active objects as busy

[Daniel Vetter]

Reporting objects that are not currently used by the gpu but are
dirty and not yet flushed creates havoc to libdrm's bo reuse:
The usually immediatly following gtt_map afterwards will stall
until the gpu has executed the writeback.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_gem.c |   27 +++++++++++++++++++--------
 1 files changed, 19 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 8e18c9d..7950169 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4395,14 +4395,25 @@ i915_gem_busy_ioctl(struct drm_device *dev, void *data,
 	i915_gem_retire_requests(dev);
 
 	obj_priv = to_intel_bo(obj);
-	/* Don't count being on the flushing list against the object being
-	 * done.  Otherwise, a buffer left on the flushing list but not getting
-	 * flushed (because nobody's flushing that domain) won't ever return
-	 * unbusy and get reused by libdrm's bo cache.  The other expected
-	 * consumer of this interface, OpenGL's occlusion queries, also specs
-	 * that the objects get unbusy "eventually" without any interference.
-	 */
-	args->busy = obj_priv->active && obj_priv->last_rendering_seqno != 0;
+
+	/* Count all active objects as busy, even if they are currently not used
+	 * by the gpu. Users of this interface expect objects to eventually
+	 * become non-busy without any further actions, therefore emit any
+	 * necessary flushes here. */
+	args->busy = obj_priv->active;
+
+	/* Unconditionally flush objects, even when the gpu still uses this
+	 * object. Userspace calling this function indicates that it wants to
+	 * use this buffer rather sooner than later, so issuing the required
+	 * flush earlier is beneficial. */
+	if (obj->write_domain) {
+		uint32_t seqno;
+
+		i915_gem_flush(dev, 0, obj->write_domain);
+		seqno = i915_add_request(dev, file_priv, obj->write_domain);
+		if (seqno == 0)
+			return -ENOMEM;
+	}
 
 	drm_gem_object_unreference(obj);
 	mutex_unlock(&dev->struct_mutex);
-- 
1.6.6.1

[PATCH] drm/i915: adjust fence registers asynchronously on tiling changes v2

[Daniel Vetter]

This avoids stalling on the gpu. With the preparation from the
previous patch, this is really just a small change.

Thanks to Owain Ainsworth <zerooa@googlemail.com> for coming up
with the idea for this patch and hashing out the implementation
with me on irc.

v2: Reset obj_priv->fence_invalid in put_fence. No functional
change because put_fence checks whether there is actually a
fence allocated, so the put_fence in adjust_fencing won't harm.
But IMHO the code makes slightly more sense this way around.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_drv.h        |    6 ++++++
 drivers/gpu/drm/i915/i915_gem.c        |   10 ++++++++++
 drivers/gpu/drm/i915/i915_gem_tiling.c |   26 ++++++--------------------
 3 files changed, 22 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 7aec3ec..174269c 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -698,6 +698,12 @@ struct drm_i915_gem_object {
 	 */
 	int fence_reg;
 
+	/**
+	 * Tiling changes can happen asynchronously to avoid gpu stalls.
+	 * This will be set if the current fencing may be invalid.
+	 */
+	int fence_invalid;
+
 	/** How many users have pinned this object in GTT space */
 	int pin_count;
 
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 8e73a12..023f4db 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2485,6 +2485,8 @@ i915_gem_object_get_fence_reg(struct drm_gem_object *obj)
 	struct drm_i915_fence_reg *reg = NULL;
 	int ret;
 
+	BUG_ON(obj_priv->fence_invalid);
+
 	/* Just update our place in the LRU if our fence is getting used. */
 	if (obj_priv->fence_reg != I915_FENCE_REG_NONE) {
 		list_move_tail(&obj_priv->fence_list, &dev_priv->mm.fence_list);
@@ -2554,6 +2556,12 @@ i915_gem_object_adjust_fencing(struct drm_gem_object *obj)
 	struct drm_i915_gem_object *obj_priv = to_intel_bo(obj);
 	int ret;
 
+	if (obj_priv->fence_invalid) {
+		ret = i915_gem_object_put_fence_reg(obj);
+		if (ret != 0)
+			return ret;
+	}
+
 	if (!i915_gem_object_fence_offset_ok(obj, obj_priv->tiling_mode)) {
 		ret = i915_gem_object_unbind(obj);
 		if (ret != 0)
@@ -2621,6 +2629,8 @@ i915_gem_object_put_fence_reg(struct drm_gem_object *obj)
 	struct drm_device *dev = obj->dev;
 	struct drm_i915_gem_object *obj_priv = to_intel_bo(obj);
 
+	obj_priv->fence_invalid = 0;
+
 	if (obj_priv->fence_reg == I915_FENCE_REG_NONE)
 		return 0;
 
diff --git a/drivers/gpu/drm/i915/i915_gem_tiling.c b/drivers/gpu/drm/i915/i915_gem_tiling.c
index 00f264d..aadaaa6 100644
--- a/drivers/gpu/drm/i915/i915_gem_tiling.c
+++ b/drivers/gpu/drm/i915/i915_gem_tiling.c
@@ -322,30 +322,16 @@ i915_gem_set_tiling(struct drm_device *dev, void *data,
 	mutex_lock(&dev->struct_mutex);
 	if (args->tiling_mode != obj_priv->tiling_mode ||
 	    args->stride != obj_priv->stride) {
-		/* We need to rebind the object if its current allocation
-		 * no longer meets the alignment restrictions for its new
-		 * tiling mode. Otherwise we can just leave it alone, but
-		 * need to ensure that any fence register is cleared.
-		 */
-		if (!i915_gem_object_fence_offset_ok(obj, args->tiling_mode))
-			ret = i915_gem_object_unbind(obj);
-		else if (obj_priv->fence_reg != I915_FENCE_REG_NONE)
-			ret = i915_gem_object_put_fence_reg(obj);
-		else
-			i915_gem_release_mmap(obj);
-
-		if (ret != 0) {
-			WARN(ret != -ERESTARTSYS,
-			     "failed to reset object for tiling switch");
-			args->tiling_mode = obj_priv->tiling_mode;
-			args->stride = obj_priv->stride;
-			goto err;
-		}
+		/* Change tiling parameter asynchronously to avoid
+		 * gpu stalls. */
+		obj_priv->fence_invalid = 1;
+
+		i915_gem_release_mmap(obj);
 
 		obj_priv->tiling_mode = args->tiling_mode;
 		obj_priv->stride = args->stride;
 	}
-err:
+
 	drm_gem_object_unreference(obj);
 	mutex_unlock(&dev->struct_mutex);
 
-- 
1.6.6.1

Re: [PATCH 4/4] drm/i915: report all active objects as busy

[Chris Wilson]

On Thu, 22 Apr 2010 22:12:52 +0200, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> Reporting objects that are not currently used by the gpu but are
> dirty and not yet flushed creates havoc to libdrm's bo reuse:
> The usually immediatly following gtt_map afterwards will stall
> until the gpu has executed the writeback.

I like this and I think Eric will approve. Just one comment inline.
 
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/i915_gem.c |   27 +++++++++++++++++++--------
>  1 files changed, 19 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 8e18c9d..7950169 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -4395,14 +4395,25 @@ i915_gem_busy_ioctl(struct drm_device *dev, void *data,
>  	i915_gem_retire_requests(dev);
>  
>  	obj_priv = to_intel_bo(obj);
> -	/* Don't count being on the flushing list against the object being
> -	 * done.  Otherwise, a buffer left on the flushing list but not getting
> -	 * flushed (because nobody's flushing that domain) won't ever return
> -	 * unbusy and get reused by libdrm's bo cache.  The other expected
> -	 * consumer of this interface, OpenGL's occlusion queries, also specs
> -	 * that the objects get unbusy "eventually" without any interference.
> -	 */
> -	args->busy = obj_priv->active && obj_priv->last_rendering_seqno != 0;
> +
> +	/* Count all active objects as busy, even if they are currently not used
> +	 * by the gpu. Users of this interface expect objects to eventually
> +	 * become non-busy without any further actions, therefore emit any
> +	 * necessary flushes here. */
> +	args->busy = obj_priv->active;
> +
> +	/* Unconditionally flush objects, even when the gpu still uses this
> +	 * object. Userspace calling this function indicates that it wants to
> +	 * use this buffer rather sooner than later, so issuing the required
> +	 * flush earlier is beneficial. */
> +	if (obj->write_domain) {
> +		uint32_t seqno;
> +
> +		i915_gem_flush(dev, 0, obj->write_domain);
> +		seqno = i915_add_request(dev, file_priv, obj->write_domain);
> +		if (seqno == 0)
> +			return -ENOMEM;

Return whilst holding the mutex and leaking a reference? Oh noes! Two wrongs
do not make a right. ;-)
-ickle

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 0/4] prevent stalls due to tiling changes and bo reuse

[Chris Wilson]

On Thu, 22 Apr 2010 22:12:48 +0200, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> Hi all,
> 
> This patch-set prevents stalls due to bo reuse in libdrm. After having
> tried to fix the set_tiling stall both in libdrm and the kernel, I think
> fixing this in the kernel is the better option: This way gem nicely
> abstracts away the execution semantics. The only place where the
> asynchronous nature of the gpu leaks is in the busy ioctl, which userspace
> should call to check whether a mmap of the bo would stall.
> 
> Performance gains are minimal, but strictly positive (neglecting a 1% noise
> level).  cairo perf traces on my i945:
> 
> old: without-patches
> new: with-patches
> Speedups
> ========
>  xlib-rgba       firefox-planet-gnome-0    62418.67 (64895.61 1.98%) -> 57833.66
>  (61105.65 2.92%):  1.08x speedup

That's a little better than what I see here, but there is just about some
improvements over the noise.

The patches themselves look worthwhile cleanups.
-ickle

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH] drm/i915: report all active objects as busy v2

[Daniel Vetter]

Reporting objects that are not currently used by the gpu but are
dirty and not yet flushed creates havoc to libdrm's bo reuse:
The usually immediatly following gtt_map afterwards will stall
until the gpu has executed the writeback.

v2: Fix refcount leak and missing unlock in the error path. Spotted
by Chris Wilson.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_gem.c |   30 +++++++++++++++++++++---------
 1 files changed, 21 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 023f4db..eedc456 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4379,6 +4379,7 @@ i915_gem_busy_ioctl(struct drm_device *dev, void *data,
 	struct drm_i915_gem_busy *args = data;
 	struct drm_gem_object *obj;
 	struct drm_i915_gem_object *obj_priv;
+	int ret = 0;
 
 	obj = drm_gem_object_lookup(dev, file_priv, args->handle);
 	if (obj == NULL) {
@@ -4396,18 +4397,29 @@ i915_gem_busy_ioctl(struct drm_device *dev, void *data,
 	i915_gem_retire_requests(dev);
 
 	obj_priv = to_intel_bo(obj);
-	/* Don't count being on the flushing list against the object being
-	 * done.  Otherwise, a buffer left on the flushing list but not getting
-	 * flushed (because nobody's flushing that domain) won't ever return
-	 * unbusy and get reused by libdrm's bo cache.  The other expected
-	 * consumer of this interface, OpenGL's occlusion queries, also specs
-	 * that the objects get unbusy "eventually" without any interference.
-	 */
-	args->busy = obj_priv->active && obj_priv->last_rendering_seqno != 0;
+
+	/* Count all active objects as busy, even if they are currently not used
+	 * by the gpu. Users of this interface expect objects to eventually
+	 * become non-busy without any further actions, therefore emit any
+	 * necessary flushes here. */
+	args->busy = obj_priv->active;
+
+	/* Unconditionally flush objects, even when the gpu still uses this
+	 * object. Userspace calling this function indicates that it wants to
+	 * use this buffer rather sooner than later, so issuing the required
+	 * flush earlier is beneficial. */
+	if (obj->write_domain) {
+		uint32_t seqno;
+
+		i915_gem_flush(dev, 0, obj->write_domain);
+		seqno = i915_add_request(dev, file_priv, obj->write_domain);
+		if (seqno == 0)
+			ret = -ENOMEM;
+	}
 
 	drm_gem_object_unreference(obj);
 	mutex_unlock(&dev->struct_mutex);
-	return 0;
+	return ret;
 }
 
 int
-- 
1.6.6.1

Re: [PATCH 1/4] drm/i915: don't allow tiling changes on pinned buffers

[Owain Ainsworth]

On Thu, Apr 22, 2010 at 10:12:49PM +0200, Daniel Vetter wrote:
> Makes no sense and complicates matters for pipelined tiling changes.
> So don't allow it and return -EBUSY.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/i915_gem_tiling.c |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_tiling.c b/drivers/gpu/drm/i915/i915_gem_tiling.c
> index 449157f..00f264d 100644
> --- a/drivers/gpu/drm/i915/i915_gem_tiling.c
> +++ b/drivers/gpu/drm/i915/i915_gem_tiling.c
> @@ -287,6 +287,9 @@ i915_gem_set_tiling(struct drm_device *dev, void *data,
>  		return -EINVAL;
>  	}
>  
> +	if (obj_priv->pin_count)
> +		return -EBUSY;
> +

you leak a reference here.

-0-
-- 
When all other means of communication fail, try words.

[PATCH] drm/i915: don't allow tiling changes on pinned buffers v2

[Daniel Vetter]

Makes no sense and complicates matters for pipelined tiling changes.
So don't allow it and return -EBUSY.

v2: Fix reference leak. Thanks to Owain Ainsworth for spotting this.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_gem_tiling.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_tiling.c b/drivers/gpu/drm/i915/i915_gem_tiling.c
index 449157f..4aed1f3 100644
--- a/drivers/gpu/drm/i915/i915_gem_tiling.c
+++ b/drivers/gpu/drm/i915/i915_gem_tiling.c
@@ -287,6 +287,11 @@ i915_gem_set_tiling(struct drm_device *dev, void *data,
 		return -EINVAL;
 	}
 
+	if (obj_priv->pin_count) {
+		drm_gem_object_unreference_unlocked(obj);
+		return -EBUSY;
+	}
+
 	if (args->tiling_mode == I915_TILING_NONE) {
 		args->swizzle_mode = I915_BIT_6_SWIZZLE_NONE;
 		args->stride = 0;
-- 
1.6.6.1

Re: [PATCH] drm/i915: report all active objects as busy v2

[Eric Anholt]

[-- Attachment #1.1: Type: text/plain, Size: 4070 bytes --]

On Fri, 23 Apr 2010 14:32:39 +0200, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> Reporting objects that are not currently used by the gpu but are
> dirty and not yet flushed creates havoc to libdrm's bo reuse:
> The usually immediatly following gtt_map afterwards will stall
> until the gpu has executed the writeback.
> 
> v2: Fix refcount leak and missing unlock in the error path. Spotted
> by Chris Wilson.

I've thought about writing this patch several times.  Here's my concern.
Say I drew to an FBO (render cache dirty) temporarily then freed it.  It
goes into the bo reuse list in userland, and is on the flushing list in
kernel.  Now I start doing a bunch of texturing -- allocating and
deleting objects, only flushing the texture cache.  When I go to
allocate, I see that that old FBO object is busy, and allocate new
instead of using cache.  When I free, I put my thing at the end of the
list.  Memory usage goes up and up and up.

Also, note that that ioctl says it's trying to implement OpenGL
semantics.  Please don't change its behavior and break those semantics.

It seems like the problem here is that userland doesn't know the
active/flushing distinction, and if it did it could manage its lists
almost in parallel to the kernel's list and reuse the right BOs at the
right times.  So maybe make a new busy ioctl that returns both active
and flushing booleans?  Then libdrm can be smart and OpenGL can be
happy.

(Actually, OpenGL would be happier if the thing it was asking for
results on got off the flushing list sooner please.  It's trying to get
the results of PIPE_CONTROL writes, and right now we're faking tracking
that using GEM_DOMAIN_INSTRUCTION as the write domain).

> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/i915_gem.c |   30 +++++++++++++++++++++---------
>  1 files changed, 21 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 023f4db..eedc456 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -4379,6 +4379,7 @@ i915_gem_busy_ioctl(struct drm_device *dev, void *data,
>  	struct drm_i915_gem_busy *args = data;
>  	struct drm_gem_object *obj;
>  	struct drm_i915_gem_object *obj_priv;
> +	int ret = 0;
>  
>  	obj = drm_gem_object_lookup(dev, file_priv, args->handle);
>  	if (obj == NULL) {
> @@ -4396,18 +4397,29 @@ i915_gem_busy_ioctl(struct drm_device *dev, void *data,
>  	i915_gem_retire_requests(dev);
>  
>  	obj_priv = to_intel_bo(obj);
> -	/* Don't count being on the flushing list against the object being
> -	 * done.  Otherwise, a buffer left on the flushing list but not getting
> -	 * flushed (because nobody's flushing that domain) won't ever return
> -	 * unbusy and get reused by libdrm's bo cache.  The other expected
> -	 * consumer of this interface, OpenGL's occlusion queries, also specs
> -	 * that the objects get unbusy "eventually" without any interference.
> -	 */
> -	args->busy = obj_priv->active && obj_priv->last_rendering_seqno != 0;
> +
> +	/* Count all active objects as busy, even if they are currently not used
> +	 * by the gpu. Users of this interface expect objects to eventually
> +	 * become non-busy without any further actions, therefore emit any
> +	 * necessary flushes here. */
> +	args->busy = obj_priv->active;
> +
> +	/* Unconditionally flush objects, even when the gpu still uses this
> +	 * object. Userspace calling this function indicates that it wants to
> +	 * use this buffer rather sooner than later, so issuing the required
> +	 * flush earlier is beneficial. */
> +	if (obj->write_domain) {
> +		uint32_t seqno;
> +
> +		i915_gem_flush(dev, 0, obj->write_domain);
> +		seqno = i915_add_request(dev, file_priv, obj->write_domain);
> +		if (seqno == 0)
> +			ret = -ENOMEM;
> +	}
>  
>  	drm_gem_object_unreference(obj);
>  	mutex_unlock(&dev->struct_mutex);
> -	return 0;
> +	return ret;
>  }
>  
>  int
> -- 
> 1.6.6.1
> 

[-- Attachment #1.2: Type: application/pgp-signature, Size: 197 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: report all active objects as busy v2

[Daniel Vetter]

On Sun, May 02, 2010 at 11:13:18AM -0700, Eric Anholt wrote:
> I've thought about writing this patch several times.  Here's my concern.
> Say I drew to an FBO (render cache dirty) temporarily then freed it.  It
> goes into the bo reuse list in userland, and is on the flushing list in
> kernel.  Now I start doing a bunch of texturing -- allocating and
> deleting objects, only flushing the texture cache.  When I go to
> allocate, I see that that old FBO object is busy, and allocate new
> instead of using cache.  When I free, I put my thing at the end of the
> list.  Memory usage goes up and up and up.

This should not happen because the first busy_ioctl will flush all pending
write domains. If the gpu eventually comes around to executing this flush,
a subsequent ioctl call will report this fbo object as non-busy, hence it
can be reused.

> Also, note that that ioctl says it's trying to implement OpenGL
> semantics.  Please don't change its behavior and break those semantics.

I've thought the semantics this ioctl should implement are as follows:
a) An object should be reported busy if a subsequent map of the same
object without any other operations in between will not have to wait for
the gpu. The old implementation fails this for not yet flushed objects.
b) An object should become non-busy eventually. In practical terms this
means the following loop terminates in finite time:

while (object_is_busy)
	;

[Minus gpu hangs, of course]. Both implementations follow this.

> It seems like the problem here is that userland doesn't know the
> active/flushing distinction, and if it did it could manage its lists
> almost in parallel to the kernel's list and reuse the right BOs at the
> right times.  So maybe make a new busy ioctl that returns both active
> and flushing booleans?  Then libdrm can be smart and OpenGL can be
> happy.

This would leak gem implementation details out to userspace. IMHO this is
a path paved with good intentions leading directly to hell. If this really
is a problem perhaps a ioctl to tell the kernel that "userspace needs the
contents of this buffer asap, do whatever is needed" is the cleaner
abstraction.

> (Actually, OpenGL would be happier if the thing it was asking for
> results on got off the flushing list sooner please.  It's trying to get
> the results of PIPE_CONTROL writes, and right now we're faking tracking
> that using GEM_DOMAIN_INSTRUCTION as the write domain).

I'll look into that use case.

On a related note: While forward porting and benchmarking my exact fence
lru for execbuf2 stuff from the pipelined fencing series I've noticed that
at least uxa accel on my i855 is very sensitive to how we emit gpu
flushes. Results easily vary by up to a factor of two on certain cairo
traces.

So please hold off on this one till it's clear what's going on and what
exactly userspace needs/wants.

Yours, Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH] drm/i915: adjust fence registers asynchronously on tiling changes v2

[Eric Anholt]

[-- Attachment #1.1: Type: text/plain, Size: 783 bytes --]

On Fri, 23 Apr 2010 00:28:29 +0200, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> This avoids stalling on the gpu. With the preparation from the
> previous patch, this is really just a small change.
> 
> Thanks to Owain Ainsworth <zerooa@googlemail.com> for coming up
> with the idea for this patch and hashing out the implementation
> with me on irc.
> 
> v2: Reset obj_priv->fence_invalid in put_fence. No functional
> change because put_fence checks whether there is actually a
> fence allocated, so the put_fence in adjust_fencing won't harm.
> But IMHO the code makes slightly more sense this way around.

Keith bisected down both of our laptops getting GPU hangs in short order
to this patch, so I've gone back and pulled it and a few others out of
the tree.

[-- Attachment #1.2: Type: application/pgp-signature, Size: 197 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx