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

[PATCH 1/3] 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: Keith Packard <keithp@keithp.com>
Reported-by: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
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..1d66c24 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;
@@ -1159,6 +1126,40 @@ 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)
+				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;
+	}
+
 	trace_i915_gem_ring_dispatch(ring, seqno);
 
 	exec_start = batch_obj->gtt_offset + args->batch_start_offset;
-- 
1.7.7

[PATCH 2/3] 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+;

Cc: Keith Packard <keithp@keithp.com>
Reported-by: Kenneth Graunke <kenneth@whitecape.org>
Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 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 1d66c24..1589a19 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)) {
@@ -1127,6 +1128,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:
@@ -1140,6 +1142,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;
@@ -1147,8 +1153,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

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

[Eric Anholt]

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

On Sat, 22 Oct 2011 19:41:24 -0700, 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+;
> 
> Cc: Keith Packard <keithp@keithp.com>
> Reported-by: Kenneth Graunke <kenneth@whitecape.org>
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Reviewed-by: Eric Anholt <eric@anholt.net>

however, it doesn't appear to help Ivybridge IRQ troubles.

[-- 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 2/3] drm/i915: Force sync command ordering (Gen6+)

[Jesse Barnes]

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

On Wed, 07 Dec 2011 10:35:45 -0800
Eric Anholt <eric@anholt.net> wrote:

> On Sat, 22 Oct 2011 19:41:24 -0700, 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+;
> > 
> > Cc: Keith Packard <keithp@keithp.com>
> > Reported-by: Kenneth Graunke <kenneth@whitecape.org>
> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> Reviewed-by: Eric Anholt <eric@anholt.net>
> 
> however, it doesn't appear to help Ivybridge IRQ troubles.

You could try something like the below to force the use of PIPE_NOTIFY
instead.  Only lightly tested on IVB when we had lots of other bugs, so
I'm not sure if it works at all.

-- 
Jesse Barnes, Intel Open Source Technology Center

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 60e4b9e..ce045a8 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -62,7 +62,7 @@ module_param_named(semaphores, i915_semaphores, int, 0600);
 MODULE_PARM_DESC(semaphores,
 		"Use semaphores for inter-ring sync (default: false)");
 
-unsigned int i915_enable_rc6 __read_mostly = 1;
+unsigned int i915_enable_rc6 __read_mostly = 0;
 module_param_named(i915_enable_rc6, i915_enable_rc6, int, 0600);
 MODULE_PARM_DESC(i915_enable_rc6,
 		"Enable power-saving render C-state 6 (default: true)");
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 02f96fd..4ab2e90 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -568,7 +568,7 @@ static irqreturn_t ironlake_irq_handler(DRM_IRQ_ARGS)
 	atomic_inc(&dev_priv->irq_received);
 
 	if (IS_GEN6(dev))
-		bsd_usr_interrupt = GT_GEN6_BSD_USER_INTERRUPT;
+		bsd_usr_interrupt = GT_GEN6_BSD_USER_INTERRUPT | GT_PIPE_NOTIFY;
 
 	/* disable master interrupt before clearing iir  */
 	de_ier = I915_READ(DEIER);
@@ -602,7 +602,7 @@ static irqreturn_t ironlake_irq_handler(DRM_IRQ_ARGS)
 		notify_ring(dev, &dev_priv->ring[RCS]);
 	if (gt_iir & bsd_usr_interrupt)
 		notify_ring(dev, &dev_priv->ring[VCS]);
-	if (gt_iir & GT_BLT_USER_INTERRUPT)
+	if (gt_iir & (GT_BLT_USER_INTERRUPT | GT_PIPE_NOTIFY))
 		notify_ring(dev, &dev_priv->ring[BCS]);
 
 	if (de_iir & DE_GSE)
@@ -1810,7 +1810,8 @@ static int ironlake_irq_postinstall(struct drm_device *dev)
 		render_irqs =
 			GT_USER_INTERRUPT |
 			GT_GEN6_BSD_USER_INTERRUPT |
-			GT_BLT_USER_INTERRUPT;
+			GT_BLT_USER_INTERRUPT |
+			GT_PIPE_NOTIFY;
 	else
 		render_irqs =
 			GT_USER_INTERRUPT |
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 47b9b27..0a67334 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -292,8 +292,7 @@ static int init_render_ring(struct intel_ring_buffer *ring)
 		I915_WRITE(MI_MODE, mode);
 	}
 
-	if (INTEL_INFO(dev)->gen >= 6) {
-	} else if (IS_GEN5(dev)) {
+	if (INTEL_INFO(dev)->gen >= 5) {
 		ret = init_pipe_control(ring);
 		if (ret)
 			return ret;
@@ -411,10 +410,13 @@ pc_render_add_request(struct intel_ring_buffer *ring,
 	 * incoherence by flushing the 6 PIPE_NOTIFY buffers out to
 	 * memory before requesting an interrupt.
 	 */
-	ret = intel_ring_begin(ring, 32);
+	ret = intel_ring_begin(ring, 38);
 	if (ret)
 		return ret;
 
+	update_semaphore(ring, 0, seqno);
+	update_semaphore(ring, 1, seqno);
+
 	intel_ring_emit(ring, GFX_OP_PIPE_CONTROL | PIPE_CONTROL_QW_WRITE |
 			PIPE_CONTROL_WC_FLUSH | PIPE_CONTROL_TC_FLUSH);
 	intel_ring_emit(ring, pc->gtt_offset | PIPE_CONTROL_GLOBAL_GTT);
@@ -1289,12 +1291,10 @@ int intel_init_render_ring_buffer(struct drm_device *dev)
 	struct intel_ring_buffer *ring = &dev_priv->ring[RCS];
 
 	*ring = render_ring;
-	if (INTEL_INFO(dev)->gen >= 6) {
-		ring->add_request = gen6_add_request;
+	if (INTEL_INFO(dev)->gen >= 5) {
+		ring->add_request = pc_render_add_request;
 		ring->irq_get = gen6_render_ring_get_irq;
 		ring->irq_put = gen6_render_ring_put_irq;
-	} else if (IS_GEN5(dev)) {
-		ring->add_request = pc_render_add_request;
 		ring->get_seqno = pc_render_get_seqno;
 	}
 


[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 836 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 2/3] drm/i915: Force sync command ordering (Gen6+)

[Jesse Barnes]

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

On Wed, 7 Dec 2011 10:38:41 -0800
Jesse Barnes <jbarnes@virtuousgeek.org> wrote:

> On Wed, 07 Dec 2011 10:35:45 -0800
> Eric Anholt <eric@anholt.net> wrote:
> 
> > On Sat, 22 Oct 2011 19:41:24 -0700, 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+;
> > > 
> > > Cc: Keith Packard <keithp@keithp.com>
> > > Reported-by: Kenneth Graunke <kenneth@whitecape.org>
> > > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> > > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > 
> > Reviewed-by: Eric Anholt <eric@anholt.net>
> > 
> > however, it doesn't appear to help Ivybridge IRQ troubles.
> 
> You could try something like the below to force the use of PIPE_NOTIFY
> instead.  Only lightly tested on IVB when we had lots of other bugs, so
> I'm not sure if it works at all.

Though if it's the blit ring hanging, you'd have to try using a
flush_dw notify (if such a thing exists) instead...  I don't think the
BLT ring gets much exercise outside Linux so there could well be some
bugs.

-- 
Jesse Barnes, Intel Open Source Technology Center

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 836 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 2/3] drm/i915: Force sync command ordering (Gen6+)

[Eric Anholt]

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

On Wed, 7 Dec 2011 11:58:05 -0800, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> On Wed, 7 Dec 2011 10:38:41 -0800
> Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> 
> > On Wed, 07 Dec 2011 10:35:45 -0800
> > Eric Anholt <eric@anholt.net> wrote:
> > 
> > > On Sat, 22 Oct 2011 19:41:24 -0700, 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+;
> > > > 
> > > > Cc: Keith Packard <keithp@keithp.com>
> > > > Reported-by: Kenneth Graunke <kenneth@whitecape.org>
> > > > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> > > > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > 
> > > Reviewed-by: Eric Anholt <eric@anholt.net>
> > > 
> > > however, it doesn't appear to help Ivybridge IRQ troubles.
> > 
> > You could try something like the below to force the use of PIPE_NOTIFY
> > instead.  Only lightly tested on IVB when we had lots of other bugs, so
> > I'm not sure if it works at all.
> 
> Though if it's the blit ring hanging, you'd have to try using a
> flush_dw notify (if such a thing exists) instead...

Yeah, MI_FLUSH_DW as opposed to MI_STORE_DW + MI_USER_INTERRUPT.

[-- 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 2/3] drm/i915: Force sync command ordering (Gen6+)

[Jesse Barnes]

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

On Wed, 07 Dec 2011 12:54:07 -0800
Eric Anholt <eric@anholt.net> wrote:

> On Wed, 7 Dec 2011 11:58:05 -0800, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> > On Wed, 7 Dec 2011 10:38:41 -0800
> > Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> > 
> > > On Wed, 07 Dec 2011 10:35:45 -0800
> > > Eric Anholt <eric@anholt.net> wrote:
> > > 
> > > > On Sat, 22 Oct 2011 19:41:24 -0700, 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+;
> > > > > 
> > > > > Cc: Keith Packard <keithp@keithp.com>
> > > > > Reported-by: Kenneth Graunke <kenneth@whitecape.org>
> > > > > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> > > > > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > > > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > > 
> > > > Reviewed-by: Eric Anholt <eric@anholt.net>
> > > > 
> > > > however, it doesn't appear to help Ivybridge IRQ troubles.
> > > 
> > > You could try something like the below to force the use of PIPE_NOTIFY
> > > instead.  Only lightly tested on IVB when we had lots of other bugs, so
> > > I'm not sure if it works at all.
> > 
> > Though if it's the blit ring hanging, you'd have to try using a
> > flush_dw notify (if such a thing exists) instead...
> 
> Yeah, MI_FLUSH_DW as opposed to MI_STORE_DW + MI_USER_INTERRUPT.

Looks like there is a notify option, bit 8 of MI_FLUSH_DW.  It's a long
shot, but does anyone want to give it a try?

-- 
Jesse Barnes, Intel Open Source Technology Center

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 836 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 2/3] drm/i915: Force sync command ordering (Gen6+)

[Eric Anholt]

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

On Wed, 7 Dec 2011 13:03:29 -0800, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> On Wed, 07 Dec 2011 12:54:07 -0800
> Eric Anholt <eric@anholt.net> wrote:
> 
> > On Wed, 7 Dec 2011 11:58:05 -0800, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> > > On Wed, 7 Dec 2011 10:38:41 -0800
> > > Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> > > 
> > > > On Wed, 07 Dec 2011 10:35:45 -0800
> > > > Eric Anholt <eric@anholt.net> wrote:
> > > > 
> > > > > On Sat, 22 Oct 2011 19:41:24 -0700, 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+;
> > > > > > 
> > > > > > Cc: Keith Packard <keithp@keithp.com>
> > > > > > Reported-by: Kenneth Graunke <kenneth@whitecape.org>
> > > > > > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> > > > > > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > > > > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > > > 
> > > > > Reviewed-by: Eric Anholt <eric@anholt.net>
> > > > > 
> > > > > however, it doesn't appear to help Ivybridge IRQ troubles.
> > > > 
> > > > You could try something like the below to force the use of PIPE_NOTIFY
> > > > instead.  Only lightly tested on IVB when we had lots of other bugs, so
> > > > I'm not sure if it works at all.
> > > 
> > > Though if it's the blit ring hanging, you'd have to try using a
> > > flush_dw notify (if such a thing exists) instead...
> > 
> > Yeah, MI_FLUSH_DW as opposed to MI_STORE_DW + MI_USER_INTERRUPT.
> 
> Looks like there is a notify option, bit 8 of MI_FLUSH_DW.  It's a long
> shot, but does anyone want to give it a try?

Since MI_FLUSH_DW exists on gen6, and keithp says we still have
outstanding issues with missed blit IRQs there, I started trying it
today.  Two kernel branches posted at
git://people.freedesktop.org/~anholt/linux/

flush-dw-notify: This is the initial attempt I did with MI_FLUSH_DW with
internal notify.  Quickly produced missed blit IRQs.  I thought this was
because the notify was in parallel with the post-sync op, not synced to
be after.  So I reverted part of the patch and produced...

flush-dw: This branch still uses MI_USER_INTERRUPT, with a commit message
explaining my theory as to why it works while the previous attempt
didn't.  But if my theory is correct, then the head commit of that
(removing the HWSTAM update of HWS at interrupt time) should be safe,
while that commit pretty quickly produced missed interrupts.

So, while I've got code at flush-dw~1 that appears to work equivalently
to master, my mental model of how the hardware works is clearly still
not correct.

On gen7, flush-dw~1 still misses IRQs.  flush-dw-notify still misses
irqs as well, but it may make them less frequent.  It would take more
testing to quantify.


[-- 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 2/3] drm/i915: Force sync command ordering (Gen6+)

[Jesse Barnes]

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

On Thu, 08 Dec 2011 18:35:24 -0800
Eric Anholt <eric@anholt.net> wrote:
> Since MI_FLUSH_DW exists on gen6, and keithp says we still have
> outstanding issues with missed blit IRQs there, I started trying it
> today.  Two kernel branches posted at
> git://people.freedesktop.org/~anholt/linux/
> 
> flush-dw-notify: This is the initial attempt I did with MI_FLUSH_DW with
> internal notify.  Quickly produced missed blit IRQs.  I thought this was
> because the notify was in parallel with the post-sync op, not synced to
> be after.  So I reverted part of the patch and produced...

Bummer, that one looks like it ought to work.

On current drm-intel-next, this patch seems to be preventing missed
IRQs on IVB at least.  Anyone else wanna give it a try and confirm?
I've only tested with Eric's blit-and-wait.c test so far.

-- 
Jesse Barnes, Intel Open Source Technology Center

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index b40004b..cb821a0 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -482,78 +482,83 @@ static irqreturn_t ivybridge_irq_handler(DRM_IRQ_ARGS)
 	I915_WRITE(DEIER, de_ier & ~DE_MASTER_IRQ_CONTROL);
 	POSTING_READ(DEIER);
 
-	de_iir = I915_READ(DEIIR);
-	gt_iir = I915_READ(GTIIR);
-	pch_iir = I915_READ(SDEIIR);
-	pm_iir = I915_READ(GEN6_PMIIR);
+	/*
+	 * Try to mitigate dropped IRQs by handling as many as possible
+	 * each time we get a physical interrupt.
+	 */
+	while (1) {
+		de_iir = I915_READ(DEIIR);
+		gt_iir = I915_READ(GTIIR);
+		pch_iir = I915_READ(SDEIIR);
+		pm_iir = I915_READ(GEN6_PMIIR);
+
+		if (de_iir == 0 && gt_iir == 0 && pch_iir == 0 && pm_iir == 0) {
+			I915_WRITE(DEIER, de_ier);
+			POSTING_READ(DEIER);
+			return ret;
+		}
 
-	if (de_iir == 0 && gt_iir == 0 && pch_iir == 0 && pm_iir == 0)
-		goto done;
+		ret = IRQ_HANDLED;
 
-	ret = IRQ_HANDLED;
+		if (dev->primary->master) {
+			master_priv = dev->primary->master->driver_priv;
+			if (master_priv->sarea_priv)
+				master_priv->sarea_priv->last_dispatch =
+					READ_BREADCRUMB(dev_priv);
+		}
 
-	if (dev->primary->master) {
-		master_priv = dev->primary->master->driver_priv;
-		if (master_priv->sarea_priv)
-			master_priv->sarea_priv->last_dispatch =
-				READ_BREADCRUMB(dev_priv);
-	}
+		if (gt_iir & (GT_USER_INTERRUPT | GT_PIPE_NOTIFY))
+			notify_ring(dev, &dev_priv->ring[RCS]);
+		if (gt_iir & GT_GEN6_BSD_USER_INTERRUPT)
+			notify_ring(dev, &dev_priv->ring[VCS]);
+		if (gt_iir & GT_BLT_USER_INTERRUPT)
+			notify_ring(dev, &dev_priv->ring[BCS]);
 
-	if (gt_iir & (GT_USER_INTERRUPT | GT_PIPE_NOTIFY))
-		notify_ring(dev, &dev_priv->ring[RCS]);
-	if (gt_iir & GT_GEN6_BSD_USER_INTERRUPT)
-		notify_ring(dev, &dev_priv->ring[VCS]);
-	if (gt_iir & GT_BLT_USER_INTERRUPT)
-		notify_ring(dev, &dev_priv->ring[BCS]);
+		if (de_iir & DE_GSE_IVB)
+			intel_opregion_gse_intr(dev);
 
-	if (de_iir & DE_GSE_IVB)
-		intel_opregion_gse_intr(dev);
+		if (de_iir & DE_PLANEA_FLIP_DONE_IVB) {
+			intel_prepare_page_flip(dev, 0);
+			intel_finish_page_flip_plane(dev, 0);
+		}
 
-	if (de_iir & DE_PLANEA_FLIP_DONE_IVB) {
-		intel_prepare_page_flip(dev, 0);
-		intel_finish_page_flip_plane(dev, 0);
-	}
+		if (de_iir & DE_PLANEB_FLIP_DONE_IVB) {
+			intel_prepare_page_flip(dev, 1);
+			intel_finish_page_flip_plane(dev, 1);
+		}
 
-	if (de_iir & DE_PLANEB_FLIP_DONE_IVB) {
-		intel_prepare_page_flip(dev, 1);
-		intel_finish_page_flip_plane(dev, 1);
-	}
+		if (de_iir & DE_PIPEA_VBLANK_IVB)
+			drm_handle_vblank(dev, 0);
 
-	if (de_iir & DE_PIPEA_VBLANK_IVB)
-		drm_handle_vblank(dev, 0);
+		if (de_iir & DE_PIPEB_VBLANK_IVB)
+			drm_handle_vblank(dev, 1);
 
-	if (de_iir & DE_PIPEB_VBLANK_IVB)
-		drm_handle_vblank(dev, 1);
+		/* check event from PCH */
+		if (de_iir & DE_PCH_EVENT_IVB) {
+			if (pch_iir & SDE_HOTPLUG_MASK_CPT)
+				queue_work(dev_priv->wq,
+					   &dev_priv->hotplug_work);
+			pch_irq_handler(dev);
+		}
 
-	/* check event from PCH */
-	if (de_iir & DE_PCH_EVENT_IVB) {
-		if (pch_iir & SDE_HOTPLUG_MASK_CPT)
-			queue_work(dev_priv->wq, &dev_priv->hotplug_work);
-		pch_irq_handler(dev);
-	}
+		if (pm_iir & GEN6_PM_DEFERRED_EVENTS) {
+			unsigned long flags;
+			spin_lock_irqsave(&dev_priv->rps_lock, flags);
+			WARN(dev_priv->pm_iir & pm_iir,
+			     "Missed a PM interrupt\n");
+			dev_priv->pm_iir |= pm_iir;
+			I915_WRITE(GEN6_PMIMR, dev_priv->pm_iir);
+			POSTING_READ(GEN6_PMIMR);
+			spin_unlock_irqrestore(&dev_priv->rps_lock, flags);
+			queue_work(dev_priv->wq, &dev_priv->rps_work);
+		}
 
-	if (pm_iir & GEN6_PM_DEFERRED_EVENTS) {
-		unsigned long flags;
-		spin_lock_irqsave(&dev_priv->rps_lock, flags);
-		WARN(dev_priv->pm_iir & pm_iir, "Missed a PM interrupt\n");
-		dev_priv->pm_iir |= pm_iir;
-		I915_WRITE(GEN6_PMIMR, dev_priv->pm_iir);
-		POSTING_READ(GEN6_PMIMR);
-		spin_unlock_irqrestore(&dev_priv->rps_lock, flags);
-		queue_work(dev_priv->wq, &dev_priv->rps_work);
+		/* should clear PCH hotplug event before clear CPU irq */
+		I915_WRITE(SDEIIR, pch_iir);
+		I915_WRITE(GTIIR, gt_iir);
+		I915_WRITE(DEIIR, de_iir);
+		I915_WRITE(GEN6_PMIIR, pm_iir);
 	}
-
-	/* should clear PCH hotplug event before clear CPU irq */
-	I915_WRITE(SDEIIR, pch_iir);
-	I915_WRITE(GTIIR, gt_iir);
-	I915_WRITE(DEIIR, de_iir);
-	I915_WRITE(GEN6_PMIIR, pm_iir);
-
-done:
-	I915_WRITE(DEIER, de_ier);
-	POSTING_READ(DEIER);
-
-	return ret;
 }
 
 static irqreturn_t ironlake_irq_handler(DRM_IRQ_ARGS)
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index ca70e2f..a9bdcd6 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -1301,9 +1301,11 @@ gen6_bsd_ring_get_irq(struct intel_ring_buffer *ring)
 static void
 gen6_bsd_ring_put_irq(struct intel_ring_buffer *ring)
 {
+#if 0
 	return gen6_ring_put_irq(ring,
 				 GT_GEN6_BSD_USER_INTERRUPT,
 				 GEN6_BSD_USER_INTERRUPT);
+#endif
 }
 
 /* ring buffer for Video Codec for Gen6+ */


[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 836 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 2/3] drm/i915: Force sync command ordering (Gen6+)

[Eugeni Dodonov]

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

On Wed, Dec 14, 2011 at 19:33, Jesse Barnes <jbarnes@virtuousgeek.org>wrote:

> On Thu, 08 Dec 2011 18:35:24 -0800
> Eric Anholt <eric@anholt.net> wrote:
> > Since MI_FLUSH_DW exists on gen6, and keithp says we still have
> > outstanding issues with missed blit IRQs there, I started trying it
> > today.  Two kernel branches posted at
> > git://people.freedesktop.org/~anholt/linux/<http://people.freedesktop.org/%7Eanholt/linux/>
> >
> > flush-dw-notify: This is the initial attempt I did with MI_FLUSH_DW with
> > internal notify.  Quickly produced missed blit IRQs.  I thought this was
> > because the notify was in parallel with the post-sync op, not synced to
> > be after.  So I reverted part of the patch and produced...
>
> Bummer, that one looks like it ought to work.
>
> On current drm-intel-next, this patch seems to be preventing missed
> IRQs on IVB at least.  Anyone else wanna give it a try and confirm?
> I've only tested with Eric's blit-and-wait.c test so far.
>

I am still hitting missed IRQs with gem_dummy_reloc_loop even with this one
on IVB mobile :(.

-- 
Eugeni Dodonov
<http://eugeni.dodonov.net/>

[-- Attachment #1.2: Type: text/html, Size: 1609 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

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

[Ben Widawsky]

Simple refactor.

Cc: Keith Packard <keithp@keithp.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
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 1589a19..a5c856b 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)) {
@@ -1128,42 +1171,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;
-	}
 
 	trace_i915_gem_ring_dispatch(ring, seqno);
 
-- 
1.7.7

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

[Ben Widawsky]

Keith, I believe this series belongs in -next. The first two could
actually go in fixes.

Ben

On Sat, 22 Oct 2011 19:41:23 -0700
Ben Widawsky <ben@bwidawsk.net> 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: Keith Packard <keithp@keithp.com>
> Reported-by: Chris Wilson <chris@chris-wilson.co.uk>
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 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..1d66c24 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;
> @@ -1159,6 +1126,40 @@ 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)
> +				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;
> +	}
> +
>  	trace_i915_gem_ring_dispatch(ring, seqno);
>  
>  	exec_start = batch_obj->gtt_offset + args->batch_start_offset;

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

[Keith Packard]

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

On Sat, 22 Oct 2011 19:41:23 -0700, Ben Widawsky <ben@bwidawsk.net> wrote:

> +		    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;

You need to clean up before returning; you've allocated cliprects and an
exec buffer and also locked the mutex.

-- 
keith.packard@intel.com

[-- Attachment #1.2: Type: application/pgp-signature, Size: 827 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 1/3] drm/i915: relative_constants_mode race fix

[Ben Widawsky]

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

On Wed, Nov 23, 2011 at 01:34:06PM -0800, Keith Packard wrote:
> On Sat, 22 Oct 2011 19:41:23 -0700, Ben Widawsky <ben@bwidawsk.net> wrote:
> 
> > +		    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;
> 
> You need to clean up before returning; you've allocated cliprects and an
> exec buffer and also locked the mutex.

The hits just keep on coming for me it seems. I had this fixed locally
and I'm not even sure how I ended up propagating this patch. It all
broke when I rebased the original patch series to split this fix out.

Even more unfortunate is this repo with the changes has long since been
corrupted to destruction. So I shall fix this for the forced throttling
series, and you can take it from there if you like it.

Thanks for catching this. Shame on me...
Ben

[-- Attachment #1.2: Type: application/pgp-signature, Size: 490 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