Attachment 29294 Details for Bug 22336 – Avoid wrapping mid-instruction.

[patch] Avoid wrapping mid-instruction.

0001-drm-i915-Pad-ringbuffer-with-NOOPs-before-wrapping.patch (text/plain), 6.46 KB, created by Chris Wilson on 2009-09-07 02:06:39 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Chris Wilson

Created: 2009-09-07 02:06:39 UTC

Size: 6.46 KB

patch

obsolete

>From 6a3fca3e2d168be5076c0d2fc94d0f038ebdb42c Mon Sep 17 00:00:00 2001
>From: Chris Wilson <chris@chris-wilson.co.uk>
>Date: Sat, 5 Sep 2009 13:46:58 +0100
>Subject: [PATCH] drm/i915: Pad ringbuffer with NOOPs before wrapping
>MIME-Version: 1.0
>Content-Type: text/plain; charset=utf-8
>Content-Transfer-Encoding: 8bit
>
>According to the docs, the ringbuffer is not allowed to wrap in the middle
>of an instruction.
>
>G45 PRM, Vol 1b, p101:
>  While the âfree spaceâ wrap may allow commands to be wrapped around the
>  end of the Ring Buffer, the wrap should only occur between commands.
>  Padding (with NOP) may be required to follow this restriction.
>
>Do as commanded.
>
>[Having seen bug reports where there is evidence of split commands, but
>apparently the GPU has continued on merrily before a bizarre and untimely
>death, this may or may not fix a few random hangs.]
>
>Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>CC: Eric Anholt <eric@anholt.net>
>---
> drivers/gpu/drm/i915/i915_debugfs.c |    4 +--
> drivers/gpu/drm/i915/i915_dma.c     |   29 ++++++++++++++++++++++-
> drivers/gpu/drm/i915/i915_drv.h     |   43 +++++++++++++++++------------------
> drivers/gpu/drm/i915/i915_gem.c     |    1 -
> 4 files changed, 50 insertions(+), 27 deletions(-)
>
>diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
>index 9acc1da..4d293c1 100644
>--- a/drivers/gpu/drm/i915/i915_debugfs.c
>+++ b/drivers/gpu/drm/i915/i915_debugfs.c
>@@ -346,15 +346,13 @@ static int i915_ringbuffer_info(struct seq_file *m, void *data)
> 	struct drm_info_node *node = (struct drm_info_node *) m->private;
> 	struct drm_device *dev = node->minor->dev;
> 	drm_i915_private_t *dev_priv = dev->dev_private;
>-	unsigned int head, tail, mask;
>+	unsigned int head, tail;
> 
> 	head = I915_READ(PRB0_HEAD) & HEAD_ADDR;
> 	tail = I915_READ(PRB0_TAIL) & TAIL_ADDR;
>-	mask = dev_priv->ring.tail_mask;
> 
> 	seq_printf(m, "RingHead :  %08x\n", head);
> 	seq_printf(m, "RingTail :  %08x\n", tail);
>-	seq_printf(m, "RingMask :  %08x\n", mask);
> 	seq_printf(m, "RingSize :  %08lx\n", dev_priv->ring.Size);
> 	seq_printf(m, "Acthd :     %08x\n", I915_READ(IS_I965G(dev) ? ACTHD_I965 : ACTHD));
> 
>diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
>index 50d1f78..f135bdc 100644
>--- a/drivers/gpu/drm/i915/i915_dma.c
>+++ b/drivers/gpu/drm/i915/i915_dma.c
>@@ -80,6 +80,34 @@ int i915_wait_ring(struct drm_device * dev, int n, const char *caller)
> 	return -EBUSY;
> }
> 
>+/* As a ringbuffer is only allowed to wrap between instructions, fill
>+ * the tail with NOOPs.
>+ */
>+int i915_wrap_ring(struct drm_device *dev)
>+{
>+	drm_i915_private_t *dev_priv = dev->dev_private;
>+	volatile unsigned int *virt;
>+	int rem;
>+
>+	rem = dev_priv->ring.Size - dev_priv->ring.tail;
>+	if (dev_priv->ring.space < rem) {
>+		int ret = i915_wait_ring(dev, rem, __func__);
>+		if (ret)
>+			return ret;
>+	}
>+	dev_priv->ring.space -= rem;
>+
>+	virt = (unsigned int *)
>+		(dev_priv->ring.virtual_start + dev_priv->ring.tail);
>+	rem /= 4;
>+	while (rem--)
>+		*virt++ = MI_NOOP;
>+
>+	dev_priv->ring.tail = 0;
>+
>+	return 0;
>+}
>+
> /**
>  * Sets up the hardware status page for devices that need a physical address
>  * in the register.
>@@ -200,7 +228,6 @@ static int i915_initialize(struct drm_device * dev, drm_i915_init_t * init)
> 		}
> 
> 		dev_priv->ring.Size = init->ring_size;
>-		dev_priv->ring.tail_mask = dev_priv->ring.Size - 1;
> 
> 		dev_priv->ring.map.offset = init->ring_start;
> 		dev_priv->ring.map.size = init->ring_size;
>diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>index 76914ae..2d5bce6 100644
>--- a/drivers/gpu/drm/i915/i915_drv.h
>+++ b/drivers/gpu/drm/i915/i915_drv.h
>@@ -85,7 +85,6 @@ struct drm_i915_gem_phys_object {
> };
> 
> typedef struct _drm_i915_ring_buffer {
>-	int tail_mask;
> 	unsigned long Size;
> 	u8 *virtual_start;
> 	int head;
>@@ -790,33 +789,32 @@ extern void intel_modeset_cleanup(struct drm_device *dev);
> 
> #define I915_VERBOSE 0
> 
>-#define RING_LOCALS	unsigned int outring, ringmask, outcount; \
>-                        volatile char *virt;
>-
>-#define BEGIN_LP_RING(n) do {				\
>-	if (I915_VERBOSE)				\
>-		DRM_DEBUG("BEGIN_LP_RING(%d)\n", (n));	\
>-	if (dev_priv->ring.space < (n)*4)		\
>-		i915_wait_ring(dev, (n)*4, __func__);		\
>-	outcount = 0;					\
>-	outring = dev_priv->ring.tail;			\
>-	ringmask = dev_priv->ring.tail_mask;		\
>-	virt = dev_priv->ring.virtual_start;		\
>+#define RING_LOCALS	volatile unsigned int *ring_virt__;
>+
>+#define BEGIN_LP_RING(n) do {						\
>+	int bytes__ = 4*(n);						\
>+	if (I915_VERBOSE) DRM_DEBUG("BEGIN_LP_RING(%d)\n", (n));	\
>+	/* a wrap must occur between instructions so pad beforehand */	\
>+	if (unlikely (dev_priv->ring.tail + bytes__ > dev_priv->ring.Size)) \
>+		i915_wrap_ring(dev);					\
>+	if (unlikely (dev_priv->ring.space < bytes__))			\
>+		i915_wait_ring(dev, bytes__, __func__);			\
>+	ring_virt__ = (unsigned int *)					\
>+	        (dev_priv->ring.virtual_start + dev_priv->ring.tail);	\
>+	dev_priv->ring.tail += bytes__;					\
>+	dev_priv->ring.tail &= dev_priv->ring.Size - 1;			\
>+	dev_priv->ring.space -= bytes__;				\
> } while (0)
> 
>-#define OUT_RING(n) do {					\
>+#define OUT_RING(n) do {						\
> 	if (I915_VERBOSE) DRM_DEBUG("   OUT_RING %x\n", (int)(n));	\
>-	*(volatile unsigned int *)(virt + outring) = (n);	\
>-        outcount++;						\
>-	outring += 4;						\
>-	outring &= ringmask;					\
>+	*ring_virt__++ = (n);						\
> } while (0)
> 
> #define ADVANCE_LP_RING() do {						\
>-	if (I915_VERBOSE) DRM_DEBUG("ADVANCE_LP_RING %x\n", outring);	\
>-	dev_priv->ring.tail = outring;					\
>-	dev_priv->ring.space -= outcount * 4;				\
>-	I915_WRITE(PRB0_TAIL, outring);			\
>+	if (I915_VERBOSE)						\
>+		DRM_DEBUG("ADVANCE_LP_RING %x\n", dev_priv->ring.tail);	\
>+	I915_WRITE(PRB0_TAIL, dev_priv->ring.tail);			\
> } while(0)
> 
> /**
>@@ -839,6 +837,7 @@ extern void intel_modeset_cleanup(struct drm_device *dev);
> #define I915_GEM_HWS_INDEX		0x20
> #define I915_BREADCRUMB_INDEX		0x21
> 
>+extern int i915_wrap_ring(struct drm_device * dev);
> extern int i915_wait_ring(struct drm_device * dev, int n, const char *caller);
> 
> #define IS_I830(dev) ((dev)->pci_device == 0x3577)
>diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
>index bb816ec..f52dab0 100644
>--- a/drivers/gpu/drm/i915/i915_gem.c
>+++ b/drivers/gpu/drm/i915/i915_gem.c
>@@ -4094,7 +4094,6 @@ i915_gem_init_ringbuffer(struct drm_device *dev)
> 
> 	/* Set up the kernel mapping for the ring. */
> 	ring->Size = obj->size;
>-	ring->tail_mask = obj->size - 1;
> 
> 	ring->map.offset = dev->agp->base + obj_priv->gtt_offset;
> 	ring->map.size = obj->size;
>-- 
>1.6.3.3
>

Actions: View | Splinter Review

Attachments on bug 22336: 26894 | 26895 | 26932 | 27401 | 28262 | 29294

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.