[PATCH 0/5] execbuffer cleanups + fixes

[PATCH 0/5] execbuffer cleanups + fixes

[Ben Widawsky]

Some refactors, a bug fixes (found through refactoring), as well as
setting a bit because the docs says so. That bit is hanging Daniel's
machine, so we may want to hold off on the last patch for now.

This patch series is in preparation for some other work I'm doing in
this area of the code. I think this should be less risky, less
interesting, and require more nitpicking. That is why I am unleashing it
now.

Ben Widawsky (5):
  drm/i915: execbuffer simplification
  drm/i915: extract constant offset setting
  drm/i915: make eb structure do more
  drm/i915: relative_constants_mode race fix
  drm/i915: Force sync command ordering (Gen6+)

 drivers/gpu/drm/i915/i915_gem_execbuffer.c |  188 +++++++++++++++-------------
 drivers/gpu/drm/i915/i915_reg.h            |    1 +
 drivers/gpu/drm/i915/intel_ringbuffer.c    |    2 +
 3 files changed, 105 insertions(+), 86 deletions(-)

-- 
1.7.7

[PATCH 1/5] drm/i915: execbuffer simplification

[Ben Widawsky]

I find this to be easier to understand.

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_gem_execbuffer.c |    8 +++-----
 1 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 3693e83..cc69861 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -963,6 +963,7 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 	struct list_head objects;
 	struct eb_objects *eb;
 	struct drm_i915_gem_object *batch_obj;
+	struct drm_i915_gem_object *obj = NULL;
 	struct drm_clip_rect *cliprects = NULL;
 	struct intel_ring_buffer *ring;
 	u32 exec_start, exec_len;
@@ -1083,7 +1084,6 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 	/* Look up object handles */
 	INIT_LIST_HEAD(&objects);
 	for (i = 0; i < args->buffer_count; i++) {
-		struct drm_i915_gem_object *obj;
 
 		obj = to_intel_bo(drm_gem_object_lookup(dev, file,
 							exec[i].handle));
@@ -1108,10 +1108,8 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 		eb_add_object(eb, obj);
 	}
 
-	/* take note of the batch buffer before we might reorder the lists */
-	batch_obj = list_entry(objects.prev,
-			       struct drm_i915_gem_object,
-			       exec_list);
+	/* The last object is the batch */
+	batch_obj = obj;
 
 	/* Move the objects en-masse into the GTT, evicting if necessary. */
 	ret = i915_gem_execbuffer_reserve(ring, file, &objects);
-- 
1.7.7

[PATCH 2/5] drm/i915: extract constant offset setting

[Ben Widawsky]

Simple refactor.

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_gem_execbuffer.c |   78 +++++++++++++++++-----------
 1 files changed, 47 insertions(+), 31 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index cc69861..5f01227 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -954,6 +954,50 @@ i915_gem_execbuffer_retire_commands(struct drm_device *dev,
 }
 
 static int
+i915_gem_set_constant_offset(struct intel_ring_buffer *ring, int mode)
+{
+	struct drm_device *dev = ring->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	uint32_t mask = I915_EXEC_CONSTANTS_MASK;
+	int ret;
+
+	switch (mode) {
+	case I915_EXEC_CONSTANTS_REL_GENERAL:
+	case I915_EXEC_CONSTANTS_ABSOLUTE:
+	case I915_EXEC_CONSTANTS_REL_SURFACE:
+		if (ring == &dev_priv->ring[RCS] &&
+		    mode != dev_priv->relative_constants_mode) {
+			if (INTEL_INFO(dev)->gen < 4)
+				return -EINVAL;
+
+			if (INTEL_INFO(dev)->gen > 5 &&
+			    mode == I915_EXEC_CONSTANTS_REL_SURFACE)
+				return -EINVAL;
+
+			/* The HW changed the meaning on this bit on gen6 */
+			if (INTEL_INFO(dev)->gen >= 6)
+				mask &= ~I915_EXEC_CONSTANTS_REL_SURFACE;
+
+			ret = intel_ring_begin(ring, 4);
+			if (ret)
+				return ret;
+
+			intel_ring_emit(ring, MI_NOOP);
+			intel_ring_emit(ring, MI_LOAD_REGISTER_IMM(1));
+			intel_ring_emit(ring, INSTPM);
+			intel_ring_emit(ring, mask << 16 | mode);
+			intel_ring_advance(ring);
+
+			dev_priv->relative_constants_mode = mode;
+		}
+		return 0;
+	default:
+		DRM_ERROR("execbuf with unknown constants: %d\n", mode);
+		return -EINVAL;
+	}
+}
+
+static int
 i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 		       struct drm_file *file,
 		       struct drm_i915_gem_execbuffer2 *args,
@@ -1005,37 +1049,9 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 	}
 
 	mode = args->flags & I915_EXEC_CONSTANTS_MASK;
-	switch (mode) {
-	case I915_EXEC_CONSTANTS_REL_GENERAL:
-	case I915_EXEC_CONSTANTS_ABSOLUTE:
-	case I915_EXEC_CONSTANTS_REL_SURFACE:
-		if (ring == &dev_priv->ring[RCS] &&
-		    mode != dev_priv->relative_constants_mode) {
-			if (INTEL_INFO(dev)->gen < 4)
-				return -EINVAL;
-
-			if (INTEL_INFO(dev)->gen > 5 &&
-			    mode == I915_EXEC_CONSTANTS_REL_SURFACE)
-				return -EINVAL;
-
-			ret = intel_ring_begin(ring, 4);
-			if (ret)
-				return ret;
-
-			intel_ring_emit(ring, MI_NOOP);
-			intel_ring_emit(ring, MI_LOAD_REGISTER_IMM(1));
-			intel_ring_emit(ring, INSTPM);
-			intel_ring_emit(ring,
-					I915_EXEC_CONSTANTS_MASK << 16 | mode);
-			intel_ring_advance(ring);
-
-			dev_priv->relative_constants_mode = mode;
-		}
-		break;
-	default:
-		DRM_ERROR("execbuf with unknown constants: %d\n", mode);
-		return -EINVAL;
-	}
+	ret = i915_gem_set_constant_offset(ring, mode);
+	if (ret)
+		return ret;
 
 	if (args->buffer_count < 1) {
 		DRM_ERROR("execbuf with %d buffers\n", args->buffer_count);
-- 
1.7.7

[PATCH 3/5] drm/i915: make eb structure do more

[Ben Widawsky]

clean up some code using the existing eb structure.

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_gem_execbuffer.c |  108 ++++++++++++++--------------
 1 files changed, 54 insertions(+), 54 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 5f01227..20587a3 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -40,6 +40,16 @@ struct change_domains {
 	uint32_t flips;
 };
 
+struct eb_objects {
+	struct drm_i915_gem_object *batch_obj;
+	struct list_head objects;
+	int buffer_count;
+	int mode;
+	int and;
+	struct hlist_head buckets[0];
+	/* DO NOT PUT ANYTHING HERE */
+};
+
 /*
  * Set the next domain for the specified object. This
  * may not actually perform the necessary flushing/invaliding though,
@@ -207,11 +217,6 @@ i915_gem_object_set_to_gpu_domain(struct drm_i915_gem_object *obj,
 		cd->flush_rings |= ring->id;
 }
 
-struct eb_objects {
-	int and;
-	struct hlist_head buckets[0];
-};
-
 static struct eb_objects *
 eb_create(int size)
 {
@@ -225,6 +230,8 @@ eb_create(int size)
 	if (eb == NULL)
 		return eb;
 
+	INIT_LIST_HEAD(&eb->objects);
+
 	eb->and = count - 1;
 	return eb;
 }
@@ -232,12 +239,22 @@ eb_create(int size)
 static void
 eb_reset(struct eb_objects *eb)
 {
+	while (!list_empty(&eb->objects)) {
+		struct drm_i915_gem_object *obj;
+
+		obj = list_first_entry(&eb->objects,
+				       struct drm_i915_gem_object,
+				       exec_list);
+		list_del_init(&obj->exec_list);
+		drm_gem_object_unreference(&obj->base);
+	}
 	memset(eb->buckets, 0, (eb->and+1)*sizeof(struct hlist_head));
 }
 
 static void
 eb_add_object(struct eb_objects *eb, struct drm_i915_gem_object *obj)
 {
+	list_add_tail(&obj->exec_list, &eb->objects);
 	hlist_add_head(&obj->exec_node,
 		       &eb->buckets[obj->exec_handle & eb->and]);
 }
@@ -262,6 +279,16 @@ eb_get_object(struct eb_objects *eb, unsigned long handle)
 static void
 eb_destroy(struct eb_objects *eb)
 {
+	while (!list_empty(&eb->objects)) {
+		struct drm_i915_gem_object *obj;
+
+		obj = list_first_entry(&eb->objects,
+				       struct drm_i915_gem_object,
+				       exec_list);
+		list_del_init(&obj->exec_list);
+		drm_gem_object_unreference(&obj->base);
+	}
+
 	kfree(eb);
 }
 
@@ -436,8 +463,7 @@ i915_gem_execbuffer_relocate_object_slow(struct drm_i915_gem_object *obj,
 
 static int
 i915_gem_execbuffer_relocate(struct drm_device *dev,
-			     struct eb_objects *eb,
-			     struct list_head *objects)
+			     struct eb_objects *eb)
 {
 	struct drm_i915_gem_object *obj;
 	int ret = 0;
@@ -450,7 +476,7 @@ i915_gem_execbuffer_relocate(struct drm_device *dev,
 	 * lockdep complains vehemently.
 	 */
 	pagefault_disable();
-	list_for_each_entry(obj, objects, exec_list) {
+	list_for_each_entry(obj, &eb->objects, exec_list) {
 		ret = i915_gem_execbuffer_relocate_object(obj, eb);
 		if (ret)
 			break;
@@ -618,24 +644,18 @@ static int
 i915_gem_execbuffer_relocate_slow(struct drm_device *dev,
 				  struct drm_file *file,
 				  struct intel_ring_buffer *ring,
-				  struct list_head *objects,
 				  struct eb_objects *eb,
 				  struct drm_i915_gem_exec_object2 *exec,
 				  int count)
 {
 	struct drm_i915_gem_relocation_entry *reloc;
 	struct drm_i915_gem_object *obj;
+	struct list_head *objects = &eb->objects;
 	int *reloc_offset;
 	int i, total, ret;
 
 	/* We may process another execbuffer during the unlock... */
-	while (!list_empty(objects)) {
-		obj = list_first_entry(objects,
-				       struct drm_i915_gem_object,
-				       exec_list);
-		list_del_init(&obj->exec_list);
-		drm_gem_object_unreference(&obj->base);
-	}
+	eb_reset(eb);
 
 	mutex_unlock(&dev->struct_mutex);
 
@@ -676,7 +696,6 @@ i915_gem_execbuffer_relocate_slow(struct drm_device *dev,
 	}
 
 	/* reacquire the objects */
-	eb_reset(eb);
 	for (i = 0; i < count; i++) {
 		obj = to_intel_bo(drm_gem_object_lookup(dev, file,
 							exec[i].handle));
@@ -687,7 +706,6 @@ i915_gem_execbuffer_relocate_slow(struct drm_device *dev,
 			goto err;
 		}
 
-		list_add_tail(&obj->exec_list, objects);
 		obj->exec_handle = exec[i].handle;
 		obj->exec_entry = &exec[i];
 		eb_add_object(eb, obj);
@@ -1004,15 +1022,13 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 		       struct drm_i915_gem_exec_object2 *exec)
 {
 	drm_i915_private_t *dev_priv = dev->dev_private;
-	struct list_head objects;
-	struct eb_objects *eb;
-	struct drm_i915_gem_object *batch_obj;
+	struct eb_objects *eb = NULL;
 	struct drm_i915_gem_object *obj = NULL;
 	struct drm_clip_rect *cliprects = NULL;
 	struct intel_ring_buffer *ring;
 	u32 exec_start, exec_len;
 	u32 seqno;
-	int ret, mode, i;
+	int ret, i;
 
 	if (!i915_gem_check_execbuffer(args)) {
 		DRM_ERROR("execbuf with invalid offset/length\n");
@@ -1048,8 +1064,12 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 		return -EINVAL;
 	}
 
-	mode = args->flags & I915_EXEC_CONSTANTS_MASK;
-	ret = i915_gem_set_constant_offset(ring, mode);
+	eb = eb_create(args->buffer_count);
+	if (eb == NULL)
+		return -ENOMEM;
+
+	eb->mode = args->flags & I915_EXEC_CONSTANTS_MASK;
+	ret = i915_gem_set_constant_offset(ring, eb->mode);
 	if (ret)
 		return ret;
 
@@ -1090,15 +1110,6 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 		goto pre_mutex_err;
 	}
 
-	eb = eb_create(args->buffer_count);
-	if (eb == NULL) {
-		mutex_unlock(&dev->struct_mutex);
-		ret = -ENOMEM;
-		goto pre_mutex_err;
-	}
-
-	/* Look up object handles */
-	INIT_LIST_HEAD(&objects);
 	for (i = 0; i < args->buffer_count; i++) {
 
 		obj = to_intel_bo(drm_gem_object_lookup(dev, file,
@@ -1118,26 +1129,25 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 			goto err;
 		}
 
-		list_add_tail(&obj->exec_list, &objects);
 		obj->exec_handle = exec[i].handle;
 		obj->exec_entry = &exec[i];
 		eb_add_object(eb, obj);
 	}
 
-	/* The last object is the batch */
-	batch_obj = obj;
+	/* The last object is the batch object */
+	eb->batch_obj = obj;
 
 	/* Move the objects en-masse into the GTT, evicting if necessary. */
-	ret = i915_gem_execbuffer_reserve(ring, file, &objects);
+	ret = i915_gem_execbuffer_reserve(ring, file, &eb->objects);
 	if (ret)
 		goto err;
 
 	/* The objects are in their final locations, apply the relocations. */
-	ret = i915_gem_execbuffer_relocate(dev, eb, &objects);
+	ret = i915_gem_execbuffer_relocate(dev, eb);
 	if (ret) {
 		if (ret == -EFAULT) {
 			ret = i915_gem_execbuffer_relocate_slow(dev, file, ring,
-								&objects, eb,
+								eb,
 								exec,
 								args->buffer_count);
 			BUG_ON(!mutex_is_locked(&dev->struct_mutex));
@@ -1147,14 +1157,14 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 	}
 
 	/* Set the pending read domains for the batch buffer to COMMAND */
-	if (batch_obj->base.pending_write_domain) {
+	if (eb->batch_obj->base.pending_write_domain) {
 		DRM_ERROR("Attempting to use self-modifying batch buffer\n");
 		ret = -EINVAL;
 		goto err;
 	}
-	batch_obj->base.pending_read_domains |= I915_GEM_DOMAIN_COMMAND;
+	eb->batch_obj->base.pending_read_domains |= I915_GEM_DOMAIN_COMMAND;
 
-	ret = i915_gem_execbuffer_move_to_gpu(ring, &objects);
+	ret = i915_gem_execbuffer_move_to_gpu(ring, &eb->objects);
 	if (ret)
 		goto err;
 
@@ -1175,7 +1185,7 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 
 	trace_i915_gem_ring_dispatch(ring, seqno);
 
-	exec_start = batch_obj->gtt_offset + args->batch_start_offset;
+	exec_start = eb->batch_obj->gtt_offset + args->batch_start_offset;
 	exec_len = args->batch_len;
 	if (cliprects) {
 		for (i = 0; i < args->num_cliprects; i++) {
@@ -1195,21 +1205,11 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 			goto err;
 	}
 
-	i915_gem_execbuffer_move_to_active(&objects, ring, seqno);
+	i915_gem_execbuffer_move_to_active(&eb->objects, ring, seqno);
 	i915_gem_execbuffer_retire_commands(dev, file, ring);
 
 err:
 	eb_destroy(eb);
-	while (!list_empty(&objects)) {
-		struct drm_i915_gem_object *obj;
-
-		obj = list_first_entry(&objects,
-				       struct drm_i915_gem_object,
-				       exec_list);
-		list_del_init(&obj->exec_list);
-		drm_gem_object_unreference(&obj->base);
-	}
-
 	mutex_unlock(&dev->struct_mutex);
 
 pre_mutex_err:
-- 
1.7.7

[PATCH 4/5] drm/i915: relative_constants_mode race fix

[Ben Widawsky]

I think Chris was pointing out the following, hopefully I caught his
meaning right.

dev_priv keeps track of the current addressing mode that gets set at
execbuffer time. Unfortunately the existing code was doing this before
acquiring struct_mutex which leaves a race with another thread also
doing an execbuffer. If that wasn't bad enough, relocate_slow drops
struct_mutex which opens a much more likely error where another thread
comes in and modifies the state while relocate_slow is being slow.

The solution here is to just defer setting this state until we
absolutely need it, and more importantly we know we'll have struct_mutex
for the remainder of the code path.

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_gem_execbuffer.c |   10 ++++++----
 1 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 20587a3..d419c30 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -1068,10 +1068,6 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 	if (eb == NULL)
 		return -ENOMEM;
 
-	eb->mode = args->flags & I915_EXEC_CONSTANTS_MASK;
-	ret = i915_gem_set_constant_offset(ring, eb->mode);
-	if (ret)
-		return ret;
 
 	if (args->buffer_count < 1) {
 		DRM_ERROR("execbuf with %d buffers\n", args->buffer_count);
@@ -1187,6 +1183,12 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 
 	exec_start = eb->batch_obj->gtt_offset + args->batch_start_offset;
 	exec_len = args->batch_len;
+
+	eb->mode = args->flags & I915_EXEC_CONSTANTS_MASK;
+	ret = i915_gem_set_constant_offset(ring, eb->mode);
+	if (ret)
+		goto err;
+
 	if (cliprects) {
 		for (i = 0; i < args->num_cliprects; i++) {
 			ret = i915_emit_box(dev, &cliprects[i],
-- 
1.7.7

[PATCH 5/5] drm/i915: Force sync command ordering (Gen6+)

[Ben Widawsky]

The docs say this is required for Gen7, and since the bit was added for
Gen6, we are also setting it there pit pf paranoia. Particularly as
Chris points out, if PIPE_CONTROL counts as a 3d state packet.

This was found through doc inspection by Ken and applies to Gen6+;

It is currently hanging Daniel's maching.

Reported-by: Kenneth Graunke <kenneth@whitecape.org>
Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_reg.h         |    1 +
 drivers/gpu/drm/i915/intel_ringbuffer.c |    2 ++
 2 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 138eae1..51569f2 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -436,6 +436,7 @@
 #define   INSTPM_AGPBUSY_DIS (1<<11) /* gen3: when disabled, pending interrupts
 					will not assert AGPBUSY# and will only
 					be delivered when out of C3. */
+#define   INSTPM_FORCE_ORDERING				(1<<7) /* GEN6+ */
 #define ACTHD	        0x020c8
 #define FW_BLC		0x020d8
 #define FW_BLC2		0x020dc
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 0e99589..b1d312f 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -297,6 +297,8 @@ static int init_render_ring(struct intel_ring_buffer *ring)
 	}
 
 	if (INTEL_INFO(dev)->gen >= 6) {
+		I915_WRITE(INSTPM, INSTPM_FORCE_ORDERING << 16 |
+				   INSTPM_FORCE_ORDERING);
 	} else if (IS_GEN5(dev)) {
 		ret = init_pipe_control(ring);
 		if (ret)
-- 
1.7.7

Re: [PATCH 5/5] drm/i915: Force sync command ordering (Gen6+)

[Ben Widawsky]

On Oct 11, 2011, at 9:43 PM, Ben Widawsky <ben@bwidawsk.net> wrote:

> The docs say this is required for Gen7, and since the bit was added for
> Gen6, we are also setting it there pit pf paranoia. Particularly as
> Chris points out, if PIPE_CONTROL counts as a 3d state packet.
> 
> This was found through doc inspection by Ken and applies to Gen6+;
> 
> It is currently hanging Daniel's maching.
> 
> Reported-by: Kenneth Graunke <kenneth@whitecape.org>
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
> drivers/gpu/drm/i915/i915_reg.h         |    1 +
> drivers/gpu/drm/i915/intel_ringbuffer.c |    2 ++
> 2 files changed, 3 insertions(+), 0 deletions(-)
> 

It appears I dropped the second hunk by mistake.

NAK

Re: [PATCH 5/5] drm/i915: Force sync command ordering (Gen6+)

[Daniel Vetter]

On Tue, Oct 11, 2011 at 09:43:53PM -0700, Ben Widawsky wrote:
> The docs say this is required for Gen7, and since the bit was added for
> Gen6, we are also setting it there pit pf paranoia. Particularly as
> Chris points out, if PIPE_CONTROL counts as a 3d state packet.
> 
> This was found through doc inspection by Ken and applies to Gen6+;
> 
> It is currently hanging Daniel's maching.

No longer. The issue was utter fail in conflict resolution with the
PIPE_CONTROL patches on my side. I'm now checking whether this fixes any
of the ivb hangs I'm seeing.
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH 2/5] drm/i915: extract constant offset setting

[Ben Widawsky]

On Tue, 11 Oct 2011 21:43:50 -0700
Ben Widawsky <ben@bwidawsk.net> wrote:

> Simple refactor.
> 
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> ---
>  drivers/gpu/drm/i915/i915_gem_execbuffer.c |   78 +++++++++++++++++-----------
>  1 files changed, 47 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index cc69861..5f01227 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -954,6 +954,50 @@ i915_gem_execbuffer_retire_commands(struct drm_device *dev,
>  }
>  
>  static int
> +i915_gem_set_constant_offset(struct intel_ring_buffer *ring, int mode)
> +{
> +	struct drm_device *dev = ring->dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	uint32_t mask = I915_EXEC_CONSTANTS_MASK;
> +	int ret;
> +
> +	switch (mode) {
> +	case I915_EXEC_CONSTANTS_REL_GENERAL:
> +	case I915_EXEC_CONSTANTS_ABSOLUTE:
> +	case I915_EXEC_CONSTANTS_REL_SURFACE:
> +		if (ring == &dev_priv->ring[RCS] &&
> +		    mode != dev_priv->relative_constants_mode) {
> +			if (INTEL_INFO(dev)->gen < 4)
> +				return -EINVAL;
> +
> +			if (INTEL_INFO(dev)->gen > 5 &&
> +			    mode == I915_EXEC_CONSTANTS_REL_SURFACE)
> +				return -EINVAL;
> +
> +			/* The HW changed the meaning on this bit on gen6 */
> +			if (INTEL_INFO(dev)->gen >= 6)

NAK
This bit belongs in patch 5. I will fix this after I get more feedback
on the series. I've already nak'd patch 5.

Ben

Re: [PATCH 0/5] execbuffer cleanups + fixes

[Ben Widawsky]

On Tue, 11 Oct 2011 21:43:48 -0700
Ben Widawsky <ben@bwidawsk.net> wrote:

> Some refactors, a bug fixes (found through refactoring), as well as
> setting a bit because the docs says so. That bit is hanging Daniel's
> machine, so we may want to hold off on the last patch for now.
> 
> This patch series is in preparation for some other work I'm doing in
> this area of the code. I think this should be less risky, less
> interesting, and require more nitpicking. That is why I am unleashing it
> now.
> 
> Ben Widawsky (5):
>   drm/i915: execbuffer simplification
>   drm/i915: extract constant offset setting
>   drm/i915: make eb structure do more
>   drm/i915: relative_constants_mode race fix
>   drm/i915: Force sync command ordering (Gen6+)
> 
>  drivers/gpu/drm/i915/i915_gem_execbuffer.c |  188 +++++++++++++++-------------
>  drivers/gpu/drm/i915/i915_reg.h            |    1 +
>  drivers/gpu/drm/i915/intel_ringbuffer.c    |    2 +
>  3 files changed, 105 insertions(+), 86 deletions(-)
> 

As a whole, the series is fine, but I accidentally put a hunk in patch 2 that
belonged in patch 5. Since patch 2 comments specifically say it's just a
refactor (when it's actually a functional change) I think I must nak the
series.

Please continue to comment and give feedback which I can incorporate on the
respin.

Ben

[PATCH 0/4] pre-v2 series

[Ben Widawsky]

Here is the patch series with the fixes I mentioned. Still would like to
get some review, etc. before I officially repost the series.

I reordered it so that the actual bug fixes come first, and the
refactors come next. I would really like to get the refactors in though
because a) they helped find these bugs, b) I will need them.

I also squashed a patch in since that was easier, and not really super
necessary to separate.


Ben Widawsky (4):
  drm/i915: relative_constants_mode race fix
  drm/i915: Force sync command ordering (Gen6+)
  drm/i915: extract constant offset setting
  drm/i915: Use eb for more stuff

 drivers/gpu/drm/i915/i915_gem_execbuffer.c |  180 +++++++++++++++-------------
 drivers/gpu/drm/i915/i915_reg.h            |    1 +
 drivers/gpu/drm/i915/intel_ringbuffer.c    |    3 +
 3 files changed, 102 insertions(+), 82 deletions(-)

-- 
1.7.7

[PATCH 1/4] drm/i915: relative_constants_mode race fix

[Ben Widawsky]

After my refactoring, Chris noticed that we had a bug.

dev_priv keeps track of the current addressing mode that gets set at
execbuffer time. Unfortunately the existing code was doing this before
acquiring struct_mutex which leaves a race with another thread also
doing an execbuffer. If that wasn't bad enough, relocate_slow drops
struct_mutex which opens a much more likely error where another thread
comes in and modifies the state while relocate_slow is being slow.

The solution here is to just defer setting this state until we
absolutely need it, and we know we'll have struct_mutex for the
remainder of our code path.

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_gem_execbuffer.c |   67 ++++++++++++++--------------
 1 files changed, 34 insertions(+), 33 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 3693e83..0b343af 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -1003,39 +1003,6 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 		return -EINVAL;
 	}
 
-	mode = args->flags & I915_EXEC_CONSTANTS_MASK;
-	switch (mode) {
-	case I915_EXEC_CONSTANTS_REL_GENERAL:
-	case I915_EXEC_CONSTANTS_ABSOLUTE:
-	case I915_EXEC_CONSTANTS_REL_SURFACE:
-		if (ring == &dev_priv->ring[RCS] &&
-		    mode != dev_priv->relative_constants_mode) {
-			if (INTEL_INFO(dev)->gen < 4)
-				return -EINVAL;
-
-			if (INTEL_INFO(dev)->gen > 5 &&
-			    mode == I915_EXEC_CONSTANTS_REL_SURFACE)
-				return -EINVAL;
-
-			ret = intel_ring_begin(ring, 4);
-			if (ret)
-				return ret;
-
-			intel_ring_emit(ring, MI_NOOP);
-			intel_ring_emit(ring, MI_LOAD_REGISTER_IMM(1));
-			intel_ring_emit(ring, INSTPM);
-			intel_ring_emit(ring,
-					I915_EXEC_CONSTANTS_MASK << 16 | mode);
-			intel_ring_advance(ring);
-
-			dev_priv->relative_constants_mode = mode;
-		}
-		break;
-	default:
-		DRM_ERROR("execbuf with unknown constants: %d\n", mode);
-		return -EINVAL;
-	}
-
 	if (args->buffer_count < 1) {
 		DRM_ERROR("execbuf with %d buffers\n", args->buffer_count);
 		return -EINVAL;
@@ -1132,6 +1099,40 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 			goto err;
 	}
 
+	mode = args->flags & I915_EXEC_CONSTANTS_MASK;
+	switch (mode) {
+	case I915_EXEC_CONSTANTS_REL_GENERAL:
+	case I915_EXEC_CONSTANTS_ABSOLUTE:
+	case I915_EXEC_CONSTANTS_REL_SURFACE:
+		if (ring == &dev_priv->ring[RCS] &&
+		    mode != dev_priv->relative_constants_mode) {
+			if (INTEL_INFO(dev)->gen < 4)
+				return -EINVAL;
+
+			if (INTEL_INFO(dev)->gen > 5 &&
+			    mode == I915_EXEC_CONSTANTS_REL_SURFACE)
+				return -EINVAL;
+
+			ret = intel_ring_begin(ring, 4);
+			if (ret)
+				goto err;
+
+			intel_ring_emit(ring, MI_NOOP);
+			intel_ring_emit(ring, MI_LOAD_REGISTER_IMM(1));
+			intel_ring_emit(ring, INSTPM);
+			intel_ring_emit(ring,
+					I915_EXEC_CONSTANTS_MASK << 16 | mode);
+			intel_ring_advance(ring);
+
+			dev_priv->relative_constants_mode = mode;
+		}
+		break;
+	default:
+		DRM_ERROR("execbuf with unknown constants: %d\n", mode);
+		ret -EINVAL;
+		goto err;
+	}
+
 	/* Set the pending read domains for the batch buffer to COMMAND */
 	if (batch_obj->base.pending_write_domain) {
 		DRM_ERROR("Attempting to use self-modifying batch buffer\n");
-- 
1.7.7

[PATCH 2/4] drm/i915: Force sync command ordering (Gen6+)

[Ben Widawsky]

The docs say this is required for Gen7, and since the bit was added for
Gen6, we are also setting it there pit pf paranoia. Particularly as
Chris points out, if PIPE_CONTROL counts as a 3d state packet.

This was found through doc inspection by Ken and applies to Gen6+;

Reported-by: Kenneth Graunke <kenneth@whitecape.org>
Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_gem_execbuffer.c |    9 +++++++--
 drivers/gpu/drm/i915/i915_reg.h            |    1 +
 drivers/gpu/drm/i915/intel_ringbuffer.c    |    3 +++
 3 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 0b343af..1ee1872 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -967,6 +967,7 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 	struct intel_ring_buffer *ring;
 	u32 exec_start, exec_len;
 	u32 seqno;
+	u32 mask;
 	int ret, mode, i;
 
 	if (!i915_gem_check_execbuffer(args)) {
@@ -1100,6 +1101,7 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 	}
 
 	mode = args->flags & I915_EXEC_CONSTANTS_MASK;
+	mask = I915_EXEC_CONSTANTS_MASK;
 	switch (mode) {
 	case I915_EXEC_CONSTANTS_REL_GENERAL:
 	case I915_EXEC_CONSTANTS_ABSOLUTE:
@@ -1113,6 +1115,10 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 			    mode == I915_EXEC_CONSTANTS_REL_SURFACE)
 				return -EINVAL;
 
+			/* The HW changed the meaning on this bit on gen6 */
+			if (INTEL_INFO(dev)->gen >= 6)
+				mask &= ~I915_EXEC_CONSTANTS_REL_SURFACE;
+
 			ret = intel_ring_begin(ring, 4);
 			if (ret)
 				goto err;
@@ -1120,8 +1126,7 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 			intel_ring_emit(ring, MI_NOOP);
 			intel_ring_emit(ring, MI_LOAD_REGISTER_IMM(1));
 			intel_ring_emit(ring, INSTPM);
-			intel_ring_emit(ring,
-					I915_EXEC_CONSTANTS_MASK << 16 | mode);
+			intel_ring_emit(ring, mask << 16 | mode);
 			intel_ring_advance(ring);
 
 			dev_priv->relative_constants_mode = mode;
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 138eae1..51569f2 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -436,6 +436,7 @@
 #define   INSTPM_AGPBUSY_DIS (1<<11) /* gen3: when disabled, pending interrupts
 					will not assert AGPBUSY# and will only
 					be delivered when out of C3. */
+#define   INSTPM_FORCE_ORDERING				(1<<7) /* GEN6+ */
 #define ACTHD	        0x020c8
 #define FW_BLC		0x020d8
 #define FW_BLC2		0x020dc
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 0e99589..a3c0b13 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -297,6 +297,9 @@ static int init_render_ring(struct intel_ring_buffer *ring)
 	}
 
 	if (INTEL_INFO(dev)->gen >= 6) {
+		I915_WRITE(INSTPM,
+			   INSTPM_FORCE_ORDERING << 16 |
+			   INSTPM_FORCE_ORDERING);
 	} else if (IS_GEN5(dev)) {
 		ret = init_pipe_control(ring);
 		if (ret)
-- 
1.7.7

[PATCH 3/4] drm/i915: extract constant offset setting

[Ben Widawsky]

Simple refactor.

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_gem_execbuffer.c |   82 ++++++++++++++++------------
 1 files changed, 46 insertions(+), 36 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 1ee1872..182a2b9 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -954,6 +954,50 @@ i915_gem_execbuffer_retire_commands(struct drm_device *dev,
 }
 
 static int
+i915_gem_set_constant_offset(struct intel_ring_buffer *ring, int mode)
+{
+	struct drm_device *dev = ring->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	uint32_t mask = I915_EXEC_CONSTANTS_MASK;
+	int ret;
+
+	switch (mode) {
+	case I915_EXEC_CONSTANTS_REL_GENERAL:
+	case I915_EXEC_CONSTANTS_ABSOLUTE:
+	case I915_EXEC_CONSTANTS_REL_SURFACE:
+		if (ring == &dev_priv->ring[RCS] &&
+		    mode != dev_priv->relative_constants_mode) {
+			if (INTEL_INFO(dev)->gen < 4)
+				return -EINVAL;
+
+			if (INTEL_INFO(dev)->gen > 5 &&
+			    mode == I915_EXEC_CONSTANTS_REL_SURFACE)
+				return -EINVAL;
+
+			/* The HW changed the meaning on this bit on gen6 */
+			if (INTEL_INFO(dev)->gen >= 6)
+				mask &= ~I915_EXEC_CONSTANTS_REL_SURFACE;
+
+			ret = intel_ring_begin(ring, 4);
+			if (ret)
+				return ret;
+
+			intel_ring_emit(ring, MI_NOOP);
+			intel_ring_emit(ring, MI_LOAD_REGISTER_IMM(1));
+			intel_ring_emit(ring, INSTPM);
+			intel_ring_emit(ring, mask << 16 | mode);
+			intel_ring_advance(ring);
+
+			dev_priv->relative_constants_mode = mode;
+		}
+		return 0;
+	default:
+		DRM_ERROR("execbuf with unknown constants: %d\n", mode);
+		return -EINVAL;
+	}
+}
+
+static int
 i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 		       struct drm_file *file,
 		       struct drm_i915_gem_execbuffer2 *args,
@@ -967,7 +1011,6 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 	struct intel_ring_buffer *ring;
 	u32 exec_start, exec_len;
 	u32 seqno;
-	u32 mask;
 	int ret, mode, i;
 
 	if (!i915_gem_check_execbuffer(args)) {
@@ -1101,42 +1144,9 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 	}
 
 	mode = args->flags & I915_EXEC_CONSTANTS_MASK;
-	mask = I915_EXEC_CONSTANTS_MASK;
-	switch (mode) {
-	case I915_EXEC_CONSTANTS_REL_GENERAL:
-	case I915_EXEC_CONSTANTS_ABSOLUTE:
-	case I915_EXEC_CONSTANTS_REL_SURFACE:
-		if (ring == &dev_priv->ring[RCS] &&
-		    mode != dev_priv->relative_constants_mode) {
-			if (INTEL_INFO(dev)->gen < 4)
-				return -EINVAL;
-
-			if (INTEL_INFO(dev)->gen > 5 &&
-			    mode == I915_EXEC_CONSTANTS_REL_SURFACE)
-				return -EINVAL;
-
-			/* The HW changed the meaning on this bit on gen6 */
-			if (INTEL_INFO(dev)->gen >= 6)
-				mask &= ~I915_EXEC_CONSTANTS_REL_SURFACE;
-
-			ret = intel_ring_begin(ring, 4);
-			if (ret)
-				goto err;
-
-			intel_ring_emit(ring, MI_NOOP);
-			intel_ring_emit(ring, MI_LOAD_REGISTER_IMM(1));
-			intel_ring_emit(ring, INSTPM);
-			intel_ring_emit(ring, mask << 16 | mode);
-			intel_ring_advance(ring);
-
-			dev_priv->relative_constants_mode = mode;
-		}
-		break;
-	default:
-		DRM_ERROR("execbuf with unknown constants: %d\n", mode);
-		ret -EINVAL;
+	ret = i915_gem_set_constant_offset(ring, mode);
+	if (ret)
 		goto err;
-	}
 
 	/* Set the pending read domains for the batch buffer to COMMAND */
 	if (batch_obj->base.pending_write_domain) {
-- 
1.7.7

[PATCH 4/4] drm/i915: Use eb for more stuff

[Ben Widawsky]

This refactor is useful for some future work I'll be doing on the
execbuffer path. In addition to being a pretty easy prerequisite, it
also helped me track down the bug uncovered in the first patch.

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_gem_execbuffer.c |  102 ++++++++++++++--------------
 1 files changed, 51 insertions(+), 51 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 182a2b9..b3beaae 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -40,6 +40,16 @@ struct change_domains {
 	uint32_t flips;
 };
 
+struct eb_objects {
+	struct drm_i915_gem_object *batch_obj;
+	struct list_head objects;
+	int buffer_count;
+	int mode;
+	int and;
+	struct hlist_head buckets[0];
+	/* DO NOT PUT ANYTHING HERE */
+};
+
 /*
  * Set the next domain for the specified object. This
  * may not actually perform the necessary flushing/invaliding though,
@@ -207,11 +217,6 @@ i915_gem_object_set_to_gpu_domain(struct drm_i915_gem_object *obj,
 		cd->flush_rings |= ring->id;
 }
 
-struct eb_objects {
-	int and;
-	struct hlist_head buckets[0];
-};
-
 static struct eb_objects *
 eb_create(int size)
 {
@@ -225,6 +230,8 @@ eb_create(int size)
 	if (eb == NULL)
 		return eb;
 
+	INIT_LIST_HEAD(&eb->objects);
+
 	eb->and = count - 1;
 	return eb;
 }
@@ -232,12 +239,22 @@ eb_create(int size)
 static void
 eb_reset(struct eb_objects *eb)
 {
+	while (!list_empty(&eb->objects)) {
+		struct drm_i915_gem_object *obj;
+
+		obj = list_first_entry(&eb->objects,
+				       struct drm_i915_gem_object,
+				       exec_list);
+		list_del_init(&obj->exec_list);
+		drm_gem_object_unreference(&obj->base);
+	}
 	memset(eb->buckets, 0, (eb->and+1)*sizeof(struct hlist_head));
 }
 
 static void
 eb_add_object(struct eb_objects *eb, struct drm_i915_gem_object *obj)
 {
+	list_add_tail(&obj->exec_list, &eb->objects);
 	hlist_add_head(&obj->exec_node,
 		       &eb->buckets[obj->exec_handle & eb->and]);
 }
@@ -262,6 +279,16 @@ eb_get_object(struct eb_objects *eb, unsigned long handle)
 static void
 eb_destroy(struct eb_objects *eb)
 {
+	while (!list_empty(&eb->objects)) {
+		struct drm_i915_gem_object *obj;
+
+		obj = list_first_entry(&eb->objects,
+				       struct drm_i915_gem_object,
+				       exec_list);
+		list_del_init(&obj->exec_list);
+		drm_gem_object_unreference(&obj->base);
+	}
+
 	kfree(eb);
 }
 
@@ -436,8 +463,7 @@ i915_gem_execbuffer_relocate_object_slow(struct drm_i915_gem_object *obj,
 
 static int
 i915_gem_execbuffer_relocate(struct drm_device *dev,
-			     struct eb_objects *eb,
-			     struct list_head *objects)
+			     struct eb_objects *eb)
 {
 	struct drm_i915_gem_object *obj;
 	int ret = 0;
@@ -450,7 +476,7 @@ i915_gem_execbuffer_relocate(struct drm_device *dev,
 	 * lockdep complains vehemently.
 	 */
 	pagefault_disable();
-	list_for_each_entry(obj, objects, exec_list) {
+	list_for_each_entry(obj, &eb->objects, exec_list) {
 		ret = i915_gem_execbuffer_relocate_object(obj, eb);
 		if (ret)
 			break;
@@ -618,24 +644,18 @@ static int
 i915_gem_execbuffer_relocate_slow(struct drm_device *dev,
 				  struct drm_file *file,
 				  struct intel_ring_buffer *ring,
-				  struct list_head *objects,
 				  struct eb_objects *eb,
 				  struct drm_i915_gem_exec_object2 *exec,
 				  int count)
 {
 	struct drm_i915_gem_relocation_entry *reloc;
 	struct drm_i915_gem_object *obj;
+	struct list_head *objects = &eb->objects;
 	int *reloc_offset;
 	int i, total, ret;
 
 	/* We may process another execbuffer during the unlock... */
-	while (!list_empty(objects)) {
-		obj = list_first_entry(objects,
-				       struct drm_i915_gem_object,
-				       exec_list);
-		list_del_init(&obj->exec_list);
-		drm_gem_object_unreference(&obj->base);
-	}
+	eb_reset(eb);
 
 	mutex_unlock(&dev->struct_mutex);
 
@@ -676,7 +696,6 @@ i915_gem_execbuffer_relocate_slow(struct drm_device *dev,
 	}
 
 	/* reacquire the objects */
-	eb_reset(eb);
 	for (i = 0; i < count; i++) {
 		obj = to_intel_bo(drm_gem_object_lookup(dev, file,
 							exec[i].handle));
@@ -687,7 +706,6 @@ i915_gem_execbuffer_relocate_slow(struct drm_device *dev,
 			goto err;
 		}
 
-		list_add_tail(&obj->exec_list, objects);
 		obj->exec_handle = exec[i].handle;
 		obj->exec_entry = &exec[i];
 		eb_add_object(eb, obj);
@@ -1004,14 +1022,13 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 		       struct drm_i915_gem_exec_object2 *exec)
 {
 	drm_i915_private_t *dev_priv = dev->dev_private;
-	struct list_head objects;
-	struct eb_objects *eb;
-	struct drm_i915_gem_object *batch_obj;
+	struct eb_objects *eb = NULL;
+	struct drm_i915_gem_object *obj = NULL;
 	struct drm_clip_rect *cliprects = NULL;
 	struct intel_ring_buffer *ring;
 	u32 exec_start, exec_len;
 	u32 seqno;
-	int ret, mode, i;
+	int ret, i;
 
 	if (!i915_gem_check_execbuffer(args)) {
 		DRM_ERROR("execbuf with invalid offset/length\n");
@@ -1091,11 +1108,7 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 		goto pre_mutex_err;
 	}
 
-	/* Look up object handles */
-	INIT_LIST_HEAD(&objects);
 	for (i = 0; i < args->buffer_count; i++) {
-		struct drm_i915_gem_object *obj;
-
 		obj = to_intel_bo(drm_gem_object_lookup(dev, file,
 							exec[i].handle));
 		if (&obj->base == NULL) {
@@ -1113,28 +1126,25 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 			goto err;
 		}
 
-		list_add_tail(&obj->exec_list, &objects);
 		obj->exec_handle = exec[i].handle;
 		obj->exec_entry = &exec[i];
 		eb_add_object(eb, obj);
 	}
 
-	/* take note of the batch buffer before we might reorder the lists */
-	batch_obj = list_entry(objects.prev,
-			       struct drm_i915_gem_object,
-			       exec_list);
+	/* The last object is the batch object */
+	eb->batch_obj = obj;
 
 	/* Move the objects en-masse into the GTT, evicting if necessary. */
-	ret = i915_gem_execbuffer_reserve(ring, file, &objects);
+	ret = i915_gem_execbuffer_reserve(ring, file, &eb->objects);
 	if (ret)
 		goto err;
 
 	/* The objects are in their final locations, apply the relocations. */
-	ret = i915_gem_execbuffer_relocate(dev, eb, &objects);
+	ret = i915_gem_execbuffer_relocate(dev, eb);
 	if (ret) {
 		if (ret == -EFAULT) {
 			ret = i915_gem_execbuffer_relocate_slow(dev, file, ring,
-								&objects, eb,
+								eb,
 								exec,
 								args->buffer_count);
 			BUG_ON(!mutex_is_locked(&dev->struct_mutex));
@@ -1143,20 +1153,20 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 			goto err;
 	}
 
-	mode = args->flags & I915_EXEC_CONSTANTS_MASK;
-	ret = i915_gem_set_constant_offset(ring, mode);
+	eb->mode = args->flags & I915_EXEC_CONSTANTS_MASK;
+	ret = i915_gem_set_constant_offset(ring, eb->mode);
 	if (ret)
 		goto err;
 
 	/* Set the pending read domains for the batch buffer to COMMAND */
-	if (batch_obj->base.pending_write_domain) {
+	if (eb->batch_obj->base.pending_write_domain) {
 		DRM_ERROR("Attempting to use self-modifying batch buffer\n");
 		ret = -EINVAL;
 		goto err;
 	}
-	batch_obj->base.pending_read_domains |= I915_GEM_DOMAIN_COMMAND;
+	eb->batch_obj->base.pending_read_domains |= I915_GEM_DOMAIN_COMMAND;
 
-	ret = i915_gem_execbuffer_move_to_gpu(ring, &objects);
+	ret = i915_gem_execbuffer_move_to_gpu(ring, &eb->objects);
 	if (ret)
 		goto err;
 
@@ -1177,7 +1187,7 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 
 	trace_i915_gem_ring_dispatch(ring, seqno);
 
-	exec_start = batch_obj->gtt_offset + args->batch_start_offset;
+	exec_start = eb->batch_obj->gtt_offset + args->batch_start_offset;
 	exec_len = args->batch_len;
 	if (cliprects) {
 		for (i = 0; i < args->num_cliprects; i++) {
@@ -1197,21 +1207,11 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 			goto err;
 	}
 
-	i915_gem_execbuffer_move_to_active(&objects, ring, seqno);
+	i915_gem_execbuffer_move_to_active(&eb->objects, ring, seqno);
 	i915_gem_execbuffer_retire_commands(dev, file, ring);
 
 err:
 	eb_destroy(eb);
-	while (!list_empty(&objects)) {
-		struct drm_i915_gem_object *obj;
-
-		obj = list_first_entry(&objects,
-				       struct drm_i915_gem_object,
-				       exec_list);
-		list_del_init(&obj->exec_list);
-		drm_gem_object_unreference(&obj->base);
-	}
-
 	mutex_unlock(&dev->struct_mutex);
 
 pre_mutex_err:
-- 
1.7.7

Re: [PATCH 1/4] drm/i915: relative_constants_mode race fix

[Daniel Vetter]

On Wed, Oct 12, 2011 at 03:56:19PM -0700, Ben Widawsky wrote:
> After my refactoring, Chris noticed that we had a bug.
> 
> dev_priv keeps track of the current addressing mode that gets set at
> execbuffer time. Unfortunately the existing code was doing this before
> acquiring struct_mutex which leaves a race with another thread also
> doing an execbuffer. If that wasn't bad enough, relocate_slow drops
> struct_mutex which opens a much more likely error where another thread
> comes in and modifies the state while relocate_slow is being slow.
> 
> The solution here is to just defer setting this state until we
> absolutely need it, and we know we'll have struct_mutex for the
> remainder of our code path.
> 
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> ---
>  drivers/gpu/drm/i915/i915_gem_execbuffer.c |   67 ++++++++++++++--------------
>  1 files changed, 34 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index 3693e83..0b343af 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -1003,39 +1003,6 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
>  		return -EINVAL;
>  	}
>  
> -	mode = args->flags & I915_EXEC_CONSTANTS_MASK;
> -	switch (mode) {
> -	case I915_EXEC_CONSTANTS_REL_GENERAL:
> -	case I915_EXEC_CONSTANTS_ABSOLUTE:
> -	case I915_EXEC_CONSTANTS_REL_SURFACE:
> -		if (ring == &dev_priv->ring[RCS] &&
> -		    mode != dev_priv->relative_constants_mode) {
> -			if (INTEL_INFO(dev)->gen < 4)
> -				return -EINVAL;
> -
> -			if (INTEL_INFO(dev)->gen > 5 &&
> -			    mode == I915_EXEC_CONSTANTS_REL_SURFACE)
> -				return -EINVAL;
> -
> -			ret = intel_ring_begin(ring, 4);
> -			if (ret)
> -				return ret;
> -
> -			intel_ring_emit(ring, MI_NOOP);
> -			intel_ring_emit(ring, MI_LOAD_REGISTER_IMM(1));
> -			intel_ring_emit(ring, INSTPM);
> -			intel_ring_emit(ring,
> -					I915_EXEC_CONSTANTS_MASK << 16 | mode);
> -			intel_ring_advance(ring);
> -
> -			dev_priv->relative_constants_mode = mode;
> -		}
> -		break;
> -	default:
> -		DRM_ERROR("execbuf with unknown constants: %d\n", mode);
> -		return -EINVAL;
> -	}
> -
>  	if (args->buffer_count < 1) {
>  		DRM_ERROR("execbuf with %d buffers\n", args->buffer_count);
>  		return -EINVAL;
> @@ -1132,6 +1099,40 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
>  			goto err;
>  	}
>  
> +	mode = args->flags & I915_EXEC_CONSTANTS_MASK;
> +	switch (mode) {
> +	case I915_EXEC_CONSTANTS_REL_GENERAL:
> +	case I915_EXEC_CONSTANTS_ABSOLUTE:
> +	case I915_EXEC_CONSTANTS_REL_SURFACE:
> +		if (ring == &dev_priv->ring[RCS] &&
> +		    mode != dev_priv->relative_constants_mode) {
> +			if (INTEL_INFO(dev)->gen < 4)
> +				return -EINVAL;
> +
> +			if (INTEL_INFO(dev)->gen > 5 &&
> +			    mode == I915_EXEC_CONSTANTS_REL_SURFACE)
> +				return -EINVAL;
> +
> +			ret = intel_ring_begin(ring, 4);
> +			if (ret)
> +				goto err;
> +
> +			intel_ring_emit(ring, MI_NOOP);
> +			intel_ring_emit(ring, MI_LOAD_REGISTER_IMM(1));
> +			intel_ring_emit(ring, INSTPM);
> +			intel_ring_emit(ring,
> +					I915_EXEC_CONSTANTS_MASK << 16 | mode);
> +			intel_ring_advance(ring);
> +
> +			dev_priv->relative_constants_mode = mode;
> +		}
> +		break;
> +	default:
> +		DRM_ERROR("execbuf with unknown constants: %d\n", mode);
> +		ret -EINVAL;

This probably wont compile ;-) I'd also move this block even further down
after gem_execbuffer_move_to_gpu. It doesn't matter for correctness, but
now it's sitting in the middle of the relocation and cache domain
handling, which doesn't make sense.

> +		goto err;
> +	}
> +
>  	/* Set the pending read domains for the batch buffer to COMMAND */
>  	if (batch_obj->base.pending_write_domain) {
>  		DRM_ERROR("Attempting to use self-modifying batch buffer\n");
> -- 
> 1.7.7
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH 2/4] drm/i915: Force sync command ordering (Gen6+)

[Daniel Vetter]

On Wed, Oct 12, 2011 at 03:56:20PM -0700, Ben Widawsky wrote:
> The docs say this is required for Gen7, and since the bit was added for
> Gen6, we are also setting it there pit pf paranoia. Particularly as
> Chris points out, if PIPE_CONTROL counts as a 3d state packet.
> 
> This was found through doc inspection by Ken and applies to Gen6+;
> 
> Reported-by: Kenneth Graunke <kenneth@whitecape.org>
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

Now even I get it, thanks to the comment ;-)

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH 3/4] drm/i915: extract constant offset setting

[Daniel Vetter]

On Wed, Oct 12, 2011 at 03:56:21PM -0700, Ben Widawsky wrote:
> Simple refactor.
> 
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>

Imo you can fold this in with the fix. Usually it's good to separate fixes
from refactoring, but it this special case of just moving around code
blocks, it doesn't add anything.

> ---
>  drivers/gpu/drm/i915/i915_gem_execbuffer.c |   82 ++++++++++++++++------------
>  1 files changed, 46 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index 1ee1872..182a2b9 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -954,6 +954,50 @@ i915_gem_execbuffer_retire_commands(struct drm_device *dev,
>  }
>  
>  static int
> +i915_gem_set_constant_offset(struct intel_ring_buffer *ring, int mode)
> +{
> +	struct drm_device *dev = ring->dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	uint32_t mask = I915_EXEC_CONSTANTS_MASK;
> +	int ret;
> +
> +	switch (mode) {
> +	case I915_EXEC_CONSTANTS_REL_GENERAL:
> +	case I915_EXEC_CONSTANTS_ABSOLUTE:
> +	case I915_EXEC_CONSTANTS_REL_SURFACE:
> +		if (ring == &dev_priv->ring[RCS] &&
> +		    mode != dev_priv->relative_constants_mode) {
> +			if (INTEL_INFO(dev)->gen < 4)
> +				return -EINVAL;
> +
> +			if (INTEL_INFO(dev)->gen > 5 &&
> +			    mode == I915_EXEC_CONSTANTS_REL_SURFACE)
> +				return -EINVAL;
> +
> +			/* The HW changed the meaning on this bit on gen6 */
> +			if (INTEL_INFO(dev)->gen >= 6)
> +				mask &= ~I915_EXEC_CONSTANTS_REL_SURFACE;
> +
> +			ret = intel_ring_begin(ring, 4);
> +			if (ret)
> +				return ret;
> +
> +			intel_ring_emit(ring, MI_NOOP);
> +			intel_ring_emit(ring, MI_LOAD_REGISTER_IMM(1));
> +			intel_ring_emit(ring, INSTPM);
> +			intel_ring_emit(ring, mask << 16 | mode);
> +			intel_ring_advance(ring);
> +
> +			dev_priv->relative_constants_mode = mode;
> +		}
> +		return 0;
> +	default:
> +		DRM_ERROR("execbuf with unknown constants: %d\n", mode);
> +		return -EINVAL;
> +	}
> +}
> +
> +static int
>  i915_gem_do_execbuffer(struct drm_device *dev, void *data,
>  		       struct drm_file *file,
>  		       struct drm_i915_gem_execbuffer2 *args,
> @@ -967,7 +1011,6 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
>  	struct intel_ring_buffer *ring;
>  	u32 exec_start, exec_len;
>  	u32 seqno;
> -	u32 mask;
>  	int ret, mode, i;
>  
>  	if (!i915_gem_check_execbuffer(args)) {
> @@ -1101,42 +1144,9 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
>  	}
>  
>  	mode = args->flags & I915_EXEC_CONSTANTS_MASK;
> -	mask = I915_EXEC_CONSTANTS_MASK;
> -	switch (mode) {
> -	case I915_EXEC_CONSTANTS_REL_GENERAL:
> -	case I915_EXEC_CONSTANTS_ABSOLUTE:
> -	case I915_EXEC_CONSTANTS_REL_SURFACE:
> -		if (ring == &dev_priv->ring[RCS] &&
> -		    mode != dev_priv->relative_constants_mode) {
> -			if (INTEL_INFO(dev)->gen < 4)
> -				return -EINVAL;
> -
> -			if (INTEL_INFO(dev)->gen > 5 &&
> -			    mode == I915_EXEC_CONSTANTS_REL_SURFACE)
> -				return -EINVAL;
> -
> -			/* The HW changed the meaning on this bit on gen6 */
> -			if (INTEL_INFO(dev)->gen >= 6)
> -				mask &= ~I915_EXEC_CONSTANTS_REL_SURFACE;
> -
> -			ret = intel_ring_begin(ring, 4);
> -			if (ret)
> -				goto err;
> -
> -			intel_ring_emit(ring, MI_NOOP);
> -			intel_ring_emit(ring, MI_LOAD_REGISTER_IMM(1));
> -			intel_ring_emit(ring, INSTPM);
> -			intel_ring_emit(ring, mask << 16 | mode);
> -			intel_ring_advance(ring);
> -
> -			dev_priv->relative_constants_mode = mode;
> -		}
> -		break;
> -	default:
> -		DRM_ERROR("execbuf with unknown constants: %d\n", mode);
> -		ret -EINVAL;
> +	ret = i915_gem_set_constant_offset(ring, mode);
> +	if (ret)
>  		goto err;
> -	}
>  
>  	/* Set the pending read domains for the batch buffer to COMMAND */
>  	if (batch_obj->base.pending_write_domain) {
> -- 
> 1.7.7
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH 4/4] drm/i915: Use eb for more stuff

[Daniel Vetter]

On Wed, Oct 12, 2011 at 03:56:22PM -0700, Ben Widawsky wrote:
> This refactor is useful for some future work I'll be doing on the
> execbuffer path. In addition to being a pretty easy prerequisite, it
> also helped me track down the bug uncovered in the first patch.
> 
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> ---
>  drivers/gpu/drm/i915/i915_gem_execbuffer.c |  102 ++++++++++++++--------------
>  1 files changed, 51 insertions(+), 51 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index 182a2b9..b3beaae 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -40,6 +40,16 @@ struct change_domains {
>  	uint32_t flips;
>  };
>  
> +struct eb_objects {
> +	struct drm_i915_gem_object *batch_obj;
> +	struct list_head objects;
> +	int buffer_count;
> +	int mode;
> +	int and;

While you whack this code, can you do an s/and/end, I think that's just a
typo ...

> +	struct hlist_head buckets[0];
> +	/* DO NOT PUT ANYTHING HERE */

When you drop the array size, the compiler will enforce that for you (for
$compiler = gcc at least).

> +};
> +
>  /*
>   * Set the next domain for the specified object. This
>   * may not actually perform the necessary flushing/invaliding though,
> @@ -207,11 +217,6 @@ i915_gem_object_set_to_gpu_domain(struct drm_i915_gem_object *obj,
>  		cd->flush_rings |= ring->id;
>  }
>  
> -struct eb_objects {
> -	int and;
> -	struct hlist_head buckets[0];
> -};
> -
>  static struct eb_objects *
>  eb_create(int size)
>  {
> @@ -225,6 +230,8 @@ eb_create(int size)
>  	if (eb == NULL)
>  		return eb;
>  
> +	INIT_LIST_HEAD(&eb->objects);
> +
>  	eb->and = count - 1;
>  	return eb;
>  }
> @@ -232,12 +239,22 @@ eb_create(int size)
>  static void
>  eb_reset(struct eb_objects *eb)
>  {
> +	while (!list_empty(&eb->objects)) {

list_for_each_entry_safe

> +		struct drm_i915_gem_object *obj;
> +
> +		obj = list_first_entry(&eb->objects,
> +				       struct drm_i915_gem_object,
> +				       exec_list);
> +		list_del_init(&obj->exec_list);
> +		drm_gem_object_unreference(&obj->base);
> +	}
>  	memset(eb->buckets, 0, (eb->and+1)*sizeof(struct hlist_head));
>  }
>  
>  static void
>  eb_add_object(struct eb_objects *eb, struct drm_i915_gem_object *obj)
>  {
> +	list_add_tail(&obj->exec_list, &eb->objects);
>  	hlist_add_head(&obj->exec_node,
>  		       &eb->buckets[obj->exec_handle & eb->and]);
>  }
> @@ -262,6 +279,16 @@ eb_get_object(struct eb_objects *eb, unsigned long handle)
>  static void
>  eb_destroy(struct eb_objects *eb)
>  {
> +	while (!list_empty(&eb->objects)) {

dito

> +		struct drm_i915_gem_object *obj;
> +
> +		obj = list_first_entry(&eb->objects,
> +				       struct drm_i915_gem_object,
> +				       exec_list);
> +		list_del_init(&obj->exec_list);
> +		drm_gem_object_unreference(&obj->base);
> +	}
> +
>  	kfree(eb);
>  }
>  
> @@ -436,8 +463,7 @@ i915_gem_execbuffer_relocate_object_slow(struct drm_i915_gem_object *obj,
>  
>  static int
>  i915_gem_execbuffer_relocate(struct drm_device *dev,
> -			     struct eb_objects *eb,
> -			     struct list_head *objects)
> +			     struct eb_objects *eb)
>  {
>  	struct drm_i915_gem_object *obj;
>  	int ret = 0;
> @@ -450,7 +476,7 @@ i915_gem_execbuffer_relocate(struct drm_device *dev,
>  	 * lockdep complains vehemently.
>  	 */
>  	pagefault_disable();
> -	list_for_each_entry(obj, objects, exec_list) {
> +	list_for_each_entry(obj, &eb->objects, exec_list) {
>  		ret = i915_gem_execbuffer_relocate_object(obj, eb);
>  		if (ret)
>  			break;
> @@ -618,24 +644,18 @@ static int
>  i915_gem_execbuffer_relocate_slow(struct drm_device *dev,
>  				  struct drm_file *file,
>  				  struct intel_ring_buffer *ring,
> -				  struct list_head *objects,
>  				  struct eb_objects *eb,
>  				  struct drm_i915_gem_exec_object2 *exec,
>  				  int count)
>  {
>  	struct drm_i915_gem_relocation_entry *reloc;
>  	struct drm_i915_gem_object *obj;
> +	struct list_head *objects = &eb->objects;
>  	int *reloc_offset;
>  	int i, total, ret;
>  
>  	/* We may process another execbuffer during the unlock... */
> -	while (!list_empty(objects)) {
> -		obj = list_first_entry(objects,
> -				       struct drm_i915_gem_object,
> -				       exec_list);
> -		list_del_init(&obj->exec_list);
> -		drm_gem_object_unreference(&obj->base);
> -	}
> +	eb_reset(eb);
>  
>  	mutex_unlock(&dev->struct_mutex);
>  
> @@ -676,7 +696,6 @@ i915_gem_execbuffer_relocate_slow(struct drm_device *dev,
>  	}
>  
>  	/* reacquire the objects */
> -	eb_reset(eb);
>  	for (i = 0; i < count; i++) {
>  		obj = to_intel_bo(drm_gem_object_lookup(dev, file,
>  							exec[i].handle));
> @@ -687,7 +706,6 @@ i915_gem_execbuffer_relocate_slow(struct drm_device *dev,
>  			goto err;
>  		}
>  
> -		list_add_tail(&obj->exec_list, objects);
>  		obj->exec_handle = exec[i].handle;
>  		obj->exec_entry = &exec[i];
>  		eb_add_object(eb, obj);
> @@ -1004,14 +1022,13 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
>  		       struct drm_i915_gem_exec_object2 *exec)
>  {
>  	drm_i915_private_t *dev_priv = dev->dev_private;
> -	struct list_head objects;
> -	struct eb_objects *eb;
> -	struct drm_i915_gem_object *batch_obj;
> +	struct eb_objects *eb = NULL;
> +	struct drm_i915_gem_object *obj = NULL;
>  	struct drm_clip_rect *cliprects = NULL;
>  	struct intel_ring_buffer *ring;
>  	u32 exec_start, exec_len;
>  	u32 seqno;
> -	int ret, mode, i;
> +	int ret, i;
>  
>  	if (!i915_gem_check_execbuffer(args)) {
>  		DRM_ERROR("execbuf with invalid offset/length\n");
> @@ -1091,11 +1108,7 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
>  		goto pre_mutex_err;
>  	}
>  
> -	/* Look up object handles */
> -	INIT_LIST_HEAD(&objects);
>  	for (i = 0; i < args->buffer_count; i++) {
> -		struct drm_i915_gem_object *obj;
> -
>  		obj = to_intel_bo(drm_gem_object_lookup(dev, file,
>  							exec[i].handle));
>  		if (&obj->base == NULL) {
> @@ -1113,28 +1126,25 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
>  			goto err;
>  		}
>  
> -		list_add_tail(&obj->exec_list, &objects);
>  		obj->exec_handle = exec[i].handle;
>  		obj->exec_entry = &exec[i];
>  		eb_add_object(eb, obj);
>  	}
>  
> -	/* take note of the batch buffer before we might reorder the lists */
> -	batch_obj = list_entry(objects.prev,
> -			       struct drm_i915_gem_object,
> -			       exec_list);
> +	/* The last object is the batch object */
> +	eb->batch_obj = obj;
>  
>  	/* Move the objects en-masse into the GTT, evicting if necessary. */
> -	ret = i915_gem_execbuffer_reserve(ring, file, &objects);
> +	ret = i915_gem_execbuffer_reserve(ring, file, &eb->objects);
>  	if (ret)
>  		goto err;
>  
>  	/* The objects are in their final locations, apply the relocations. */
> -	ret = i915_gem_execbuffer_relocate(dev, eb, &objects);
> +	ret = i915_gem_execbuffer_relocate(dev, eb);
>  	if (ret) {
>  		if (ret == -EFAULT) {
>  			ret = i915_gem_execbuffer_relocate_slow(dev, file, ring,
> -								&objects, eb,
> +								eb,
>  								exec,
>  								args->buffer_count);

Looks like you've planned to replace args->buffer_count with
eb->buffer_count, but didn't see it through. Please do it, cause I like
it.

>  			BUG_ON(!mutex_is_locked(&dev->struct_mutex));
> @@ -1143,20 +1153,20 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
>  			goto err;
>  	}
>  
> -	mode = args->flags & I915_EXEC_CONSTANTS_MASK;
> -	ret = i915_gem_set_constant_offset(ring, mode);
> +	eb->mode = args->flags & I915_EXEC_CONSTANTS_MASK;
> +	ret = i915_gem_set_constant_offset(ring, eb->mode);

The eb->mode refactor here otoh looks a bit superflous. Is this needed for
a future patch of yours?

>  	if (ret)
>  		goto err;
>  
>  	/* Set the pending read domains for the batch buffer to COMMAND */
> -	if (batch_obj->base.pending_write_domain) {
> +	if (eb->batch_obj->base.pending_write_domain) {
>  		DRM_ERROR("Attempting to use self-modifying batch buffer\n");
>  		ret = -EINVAL;
>  		goto err;
>  	}
> -	batch_obj->base.pending_read_domains |= I915_GEM_DOMAIN_COMMAND;
> +	eb->batch_obj->base.pending_read_domains |= I915_GEM_DOMAIN_COMMAND;
>  
> -	ret = i915_gem_execbuffer_move_to_gpu(ring, &objects);
> +	ret = i915_gem_execbuffer_move_to_gpu(ring, &eb->objects);
>  	if (ret)
>  		goto err;
>  
> @@ -1177,7 +1187,7 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
>  
>  	trace_i915_gem_ring_dispatch(ring, seqno);
>  
> -	exec_start = batch_obj->gtt_offset + args->batch_start_offset;
> +	exec_start = eb->batch_obj->gtt_offset + args->batch_start_offset;

Dito for eb->batch_obj, only used in do_execbuffer, afaics. Again, if you
need this later on, maybe explain it in the commit msg?

>  	exec_len = args->batch_len;
>  	if (cliprects) {
>  		for (i = 0; i < args->num_cliprects; i++) {
> @@ -1197,21 +1207,11 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
>  			goto err;
>  	}
>  
> -	i915_gem_execbuffer_move_to_active(&objects, ring, seqno);
> +	i915_gem_execbuffer_move_to_active(&eb->objects, ring, seqno);
>  	i915_gem_execbuffer_retire_commands(dev, file, ring);
>  
>  err:
>  	eb_destroy(eb);
> -	while (!list_empty(&objects)) {
> -		struct drm_i915_gem_object *obj;
> -
> -		obj = list_first_entry(&objects,
> -				       struct drm_i915_gem_object,
> -				       exec_list);
> -		list_del_init(&obj->exec_list);
> -		drm_gem_object_unreference(&obj->base);
> -	}
> -
>  	mutex_unlock(&dev->struct_mutex);
>  
>  pre_mutex_err:
> -- 
> 1.7.7
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH 4/4] drm/i915: Use eb for more stuff

[Chris Wilson]

On Thu, 13 Oct 2011 10:58:25 +0200, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Wed, Oct 12, 2011 at 03:56:22PM -0700, Ben Widawsky wrote:
> > This refactor is useful for some future work I'll be doing on the
> > execbuffer path. In addition to being a pretty easy prerequisite, it
> > also helped me track down the bug uncovered in the first patch.
> > 
> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> > ---
> >  drivers/gpu/drm/i915/i915_gem_execbuffer.c |  102 ++++++++++++++--------------
> >  1 files changed, 51 insertions(+), 51 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > index 182a2b9..b3beaae 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > @@ -40,6 +40,16 @@ struct change_domains {
> >  	uint32_t flips;
> >  };
> >  
> > +struct eb_objects {
> > +	struct drm_i915_gem_object *batch_obj;
> > +	struct list_head objects;
> > +	int buffer_count;
> > +	int mode;
> > +	int and;
> 
> While you whack this code, can you do an s/and/end, I think that's just a
> typo ...

It was meant to be 'and' since it was the value of the mask I was anding
with. I'm sorry I appear to have spent too much time inside the Xserver,
end = and + 1.
> 
> > +	struct hlist_head buckets[0];
> > +	/* DO NOT PUT ANYTHING HERE */
> 
> When you drop the array size, the compiler will enforce that for you (for
> $compiler = gcc at least).

I wanted to say that surely the comment was obvious from the '[0]' ;-)

> 
> > +};
> > +
> >  /*
> >   * Set the next domain for the specified object. This
> >   * may not actually perform the necessary flushing/invaliding though,
> > @@ -207,11 +217,6 @@ i915_gem_object_set_to_gpu_domain(struct drm_i915_gem_object *obj,
> >  		cd->flush_rings |= ring->id;
> >  }
> >  
> > -struct eb_objects {
> > -	int and;
> > -	struct hlist_head buckets[0];
> > -};
> > -
> >  static struct eb_objects *
> >  eb_create(int size)
> >  {
> > @@ -225,6 +230,8 @@ eb_create(int size)
> >  	if (eb == NULL)
> >  		return eb;
> >  
> > +	INIT_LIST_HEAD(&eb->objects);
> > +
> >  	eb->and = count - 1;
> >  	return eb;
> >  }
> > @@ -232,12 +239,22 @@ eb_create(int size)
> >  static void
> >  eb_reset(struct eb_objects *eb)
> >  {
> > +	while (!list_empty(&eb->objects)) {
> 
> list_for_each_entry_safe

No, the while (!empty) is the more common and much simpler idiom to use
here when clearing a list.

> > -	mode = args->flags & I915_EXEC_CONSTANTS_MASK;
> > -	ret = i915_gem_set_constant_offset(ring, mode);
> > +	eb->mode = args->flags & I915_EXEC_CONSTANTS_MASK;
> > +	ret = i915_gem_set_constant_offset(ring, eb->mode);
> 
> The eb->mode refactor here otoh looks a bit superflous. Is this needed for
> a future patch of yours?

I'm waiting to see how this pans out as well... ;-)
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 4/4] drm/i915: Use eb for more stuff

[Daniel Vetter]

On Thu, Oct 13, 2011 at 01:00:02PM +0100, Chris Wilson wrote:
> On Thu, 13 Oct 2011 10:58:25 +0200, Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Wed, Oct 12, 2011 at 03:56:22PM -0700, Ben Widawsky wrote:
> > > This refactor is useful for some future work I'll be doing on the
> > > execbuffer path. In addition to being a pretty easy prerequisite, it
> > > also helped me track down the bug uncovered in the first patch.
> > > 
> > > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> > > ---
> > >  drivers/gpu/drm/i915/i915_gem_execbuffer.c |  102 ++++++++++++++--------------
> > >  1 files changed, 51 insertions(+), 51 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > > index 182a2b9..b3beaae 100644
> > > --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > > +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > > @@ -40,6 +40,16 @@ struct change_domains {
> > >  	uint32_t flips;
> > >  };
> > >  
> > > +struct eb_objects {
> > > +	struct drm_i915_gem_object *batch_obj;
> > > +	struct list_head objects;
> > > +	int buffer_count;
> > > +	int mode;
> > > +	int and;
> > 
> > While you whack this code, can you do an s/and/end, I think that's just a
> > typo ...
> 
> It was meant to be 'and' since it was the value of the mask I was anding
> with. I'm sorry I appear to have spent too much time inside the Xserver,
> end = and + 1.

Indeed, maybe s/and/hash_mask/?
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH 3/4] drm/i915: extract constant offset setting

[Ben Widawsky]

On Thu, Oct 13, 2011 at 10:46:29AM +0200, Daniel Vetter wrote:
> On Wed, Oct 12, 2011 at 03:56:21PM -0700, Ben Widawsky wrote:
> > Simple refactor.
> > 
> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> 
> Imo you can fold this in with the fix. Usually it's good to separate fixes
> from refactoring, but it this special case of just moving around code
> blocks, it doesn't add anything.
> 

I agree, and I had it like this locally, but Chris suggested that he
preferred the separation as it highlighted the fix.

I largely do not care, I just want an r-b. So you two can fight it out.

Ben

Re: [PATCH 1/4] drm/i915: relative_constants_mode race fix

[Ben Widawsky]

On Thu, Oct 13, 2011 at 10:35:23AM +0200, Daniel Vetter wrote:
> On Wed, Oct 12, 2011 at 03:56:19PM -0700, Ben Widawsky wrote:
> > After my refactoring, Chris noticed that we had a bug.
> > 
> > dev_priv keeps track of the current addressing mode that gets set at
> > execbuffer time. Unfortunately the existing code was doing this before
> > acquiring struct_mutex which leaves a race with another thread also
> > doing an execbuffer. If that wasn't bad enough, relocate_slow drops
> > struct_mutex which opens a much more likely error where another thread
> > comes in and modifies the state while relocate_slow is being slow.
> > 
> > The solution here is to just defer setting this state until we
> > absolutely need it, and we know we'll have struct_mutex for the
> > remainder of our code path.
> > 
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> > ---
> >  drivers/gpu/drm/i915/i915_gem_execbuffer.c |   67 ++++++++++++++--------------
> >  1 files changed, 34 insertions(+), 33 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > index 3693e83..0b343af 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > @@ -1003,39 +1003,6 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
> >  		return -EINVAL;
> >  	}
> >  
> > -	mode = args->flags & I915_EXEC_CONSTANTS_MASK;
> > -	switch (mode) {
> > -	case I915_EXEC_CONSTANTS_REL_GENERAL:
> > -	case I915_EXEC_CONSTANTS_ABSOLUTE:
> > -	case I915_EXEC_CONSTANTS_REL_SURFACE:
> > -		if (ring == &dev_priv->ring[RCS] &&
> > -		    mode != dev_priv->relative_constants_mode) {
> > -			if (INTEL_INFO(dev)->gen < 4)
> > -				return -EINVAL;
> > -
> > -			if (INTEL_INFO(dev)->gen > 5 &&
> > -			    mode == I915_EXEC_CONSTANTS_REL_SURFACE)
> > -				return -EINVAL;
> > -
> > -			ret = intel_ring_begin(ring, 4);
> > -			if (ret)
> > -				return ret;
> > -
> > -			intel_ring_emit(ring, MI_NOOP);
> > -			intel_ring_emit(ring, MI_LOAD_REGISTER_IMM(1));
> > -			intel_ring_emit(ring, INSTPM);
> > -			intel_ring_emit(ring,
> > -					I915_EXEC_CONSTANTS_MASK << 16 | mode);
> > -			intel_ring_advance(ring);
> > -
> > -			dev_priv->relative_constants_mode = mode;
> > -		}
> > -		break;
> > -	default:
> > -		DRM_ERROR("execbuf with unknown constants: %d\n", mode);
> > -		return -EINVAL;
> > -	}
> > -
> >  	if (args->buffer_count < 1) {
> >  		DRM_ERROR("execbuf with %d buffers\n", args->buffer_count);
> >  		return -EINVAL;
> > @@ -1132,6 +1099,40 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
> >  			goto err;
> >  	}
> >  
> > +	mode = args->flags & I915_EXEC_CONSTANTS_MASK;
> > +	switch (mode) {
> > +	case I915_EXEC_CONSTANTS_REL_GENERAL:
> > +	case I915_EXEC_CONSTANTS_ABSOLUTE:
> > +	case I915_EXEC_CONSTANTS_REL_SURFACE:
> > +		if (ring == &dev_priv->ring[RCS] &&
> > +		    mode != dev_priv->relative_constants_mode) {
> > +			if (INTEL_INFO(dev)->gen < 4)
> > +				return -EINVAL;
> > +
> > +			if (INTEL_INFO(dev)->gen > 5 &&
> > +			    mode == I915_EXEC_CONSTANTS_REL_SURFACE)
> > +				return -EINVAL;
> > +
> > +			ret = intel_ring_begin(ring, 4);
> > +			if (ret)
> > +				goto err;
> > +
> > +			intel_ring_emit(ring, MI_NOOP);
> > +			intel_ring_emit(ring, MI_LOAD_REGISTER_IMM(1));
> > +			intel_ring_emit(ring, INSTPM);
> > +			intel_ring_emit(ring,
> > +					I915_EXEC_CONSTANTS_MASK << 16 | mode);
> > +			intel_ring_advance(ring);
> > +
> > +			dev_priv->relative_constants_mode = mode;
> > +		}
> > +		break;
> > +	default:
> > +		DRM_ERROR("execbuf with unknown constants: %d\n", mode);
> > +		ret -EINVAL;
> 
> This probably wont compile ;-) I'd also move this block even further down
> after gem_execbuffer_move_to_gpu. It doesn't matter for correctness, but
> now it's sitting in the middle of the relocation and cache domain
> handling, which doesn't make sense.
> 

Crap, I must have missed the compile test. I did have this further down
originally and forgot why I moved it up here - I think it was something
like, I wanted to set the proper state before actually relocated any
objects in case we decide in the future that some addressing mode
requires relocation restrictions. (So may as well fail early if we don't
set the addressing mode).

I'll move it unless anyone else chimes in.

Ben

Re: [PATCH 4/4] drm/i915: Use eb for more stuff

[Ben Widawsky]

On Thu, Oct 13, 2011 at 01:00:02PM +0100, Chris Wilson wrote:
> On Thu, 13 Oct 2011 10:58:25 +0200, Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Wed, Oct 12, 2011 at 03:56:22PM -0700, Ben Widawsky wrote:
> > > This refactor is useful for some future work I'll be doing on the
> > > execbuffer path. In addition to being a pretty easy prerequisite, it
> > > also helped me track down the bug uncovered in the first patch.
> > > 
> > > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> > > ---
> > >  drivers/gpu/drm/i915/i915_gem_execbuffer.c |  102 ++++++++++++++--------------
> > >  1 files changed, 51 insertions(+), 51 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > > index 182a2b9..b3beaae 100644
> > > --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > > +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > > @@ -40,6 +40,16 @@ struct change_domains {
> > >  	uint32_t flips;
> > >  };
> > >  
> > > +struct eb_objects {
> > > +	struct drm_i915_gem_object *batch_obj;
> > > +	struct list_head objects;
> > > +	int buffer_count;
> > > +	int mode;
> > > +	int and;
> > 
> > While you whack this code, can you do an s/and/end, I think that's just a
> > typo ...
> 
> It was meant to be 'and' since it was the value of the mask I was anding
> with. I'm sorry I appear to have spent too much time inside the Xserver,
> end = and + 1.
> > 
> > > +	struct hlist_head buckets[0];
> > > +	/* DO NOT PUT ANYTHING HERE */
> > 
> > When you drop the array size, the compiler will enforce that for you (for
> > $compiler = gcc at least).
> 
> I wanted to say that surely the comment was obvious from the '[0]' ;-)
> 

Well not obvious to me. Though it hindsight it should have been obvious.
GCC 4.6.1 does not produce an error or warning if you do this.

> 
> > > -	mode = args->flags & I915_EXEC_CONSTANTS_MASK;
> > > -	ret = i915_gem_set_constant_offset(ring, mode);
> > > +	eb->mode = args->flags & I915_EXEC_CONSTANTS_MASK;
> > > +	ret = i915_gem_set_constant_offset(ring, eb->mode);
> > 
> > The eb->mode refactor here otoh looks a bit superflous. Is this needed for
> > a future patch of yours?
> 
> I'm waiting to see how this pans out as well... ;-)
> -Chris

It's needed for a future patch, I'm cool with call this superfluous for
now, and Keith can take it, or wait until it's needed. I can make it a
bit more uniform as Chris recommended in some other comment.

Ben