[PATCH 1/6] drm/i915: Replace hardcoded cacheline size with macro

[PATCH 1/6] drm/i915: Replace hardcoded cacheline size with macro

[Chris Wilson]

For readibility and guess at the meaning behind the constants.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_ringbuffer.c | 29 ++++++++++++++++-------------
 1 file changed, 16 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 785f246d28a8..475391ce671a 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -33,6 +33,8 @@
 #include "i915_trace.h"
 #include "intel_drv.h"
 
+#define CACHELINE_BYTES 64
+
 static inline int ring_space(struct intel_ring_buffer *ring)
 {
 	int space = (ring->head & HEAD_ADDR) - (ring->tail + I915_RING_FREE_SPACE);
@@ -175,7 +177,7 @@ gen4_render_ring_flush(struct intel_ring_buffer *ring,
 static int
 intel_emit_post_sync_nonzero_flush(struct intel_ring_buffer *ring)
 {
-	u32 scratch_addr = ring->scratch.gtt_offset + 128;
+	u32 scratch_addr = ring->scratch.gtt_offset + 2 * CACHELINE_BYTES;
 	int ret;
 
 
@@ -212,7 +214,7 @@ gen6_render_ring_flush(struct intel_ring_buffer *ring,
                          u32 invalidate_domains, u32 flush_domains)
 {
 	u32 flags = 0;
-	u32 scratch_addr = ring->scratch.gtt_offset + 128;
+	u32 scratch_addr = ring->scratch.gtt_offset + 2 * CACHELINE_BYTES;
 	int ret;
 
 	/* Force SNB workarounds for PIPE_CONTROL flushes */
@@ -306,7 +308,7 @@ gen7_render_ring_flush(struct intel_ring_buffer *ring,
 		       u32 invalidate_domains, u32 flush_domains)
 {
 	u32 flags = 0;
-	u32 scratch_addr = ring->scratch.gtt_offset + 128;
+	u32 scratch_addr = ring->scratch.gtt_offset + 2 * CACHELINE_BYTES;
 	int ret;
 
 	/*
@@ -367,7 +369,7 @@ gen8_render_ring_flush(struct intel_ring_buffer *ring,
 		       u32 invalidate_domains, u32 flush_domains)
 {
 	u32 flags = 0;
-	u32 scratch_addr = ring->scratch.gtt_offset + 128;
+	u32 scratch_addr = ring->scratch.gtt_offset + 2 * CACHELINE_BYTES;
 	int ret;
 
 	flags |= PIPE_CONTROL_CS_STALL;
@@ -772,7 +774,7 @@ do {									\
 static int
 pc_render_add_request(struct intel_ring_buffer *ring)
 {
-	u32 scratch_addr = ring->scratch.gtt_offset + 128;
+	u32 scratch_addr = ring->scratch.gtt_offset + 2 * CACHELINE_BYTES;
 	int ret;
 
 	/* For Ironlake, MI_USER_INTERRUPT was deprecated and apparently
@@ -794,15 +796,15 @@ pc_render_add_request(struct intel_ring_buffer *ring)
 	intel_ring_emit(ring, ring->outstanding_lazy_seqno);
 	intel_ring_emit(ring, 0);
 	PIPE_CONTROL_FLUSH(ring, scratch_addr);
-	scratch_addr += 128; /* write to separate cachelines */
+	scratch_addr += 2 * CACHELINE_BYTES; /* write to separate cachelines */
 	PIPE_CONTROL_FLUSH(ring, scratch_addr);
-	scratch_addr += 128;
+	scratch_addr += 2 * CACHELINE_BYTES;
 	PIPE_CONTROL_FLUSH(ring, scratch_addr);
-	scratch_addr += 128;
+	scratch_addr += 2 * CACHELINE_BYTES;
 	PIPE_CONTROL_FLUSH(ring, scratch_addr);
-	scratch_addr += 128;
+	scratch_addr += 2 * CACHELINE_BYTES;
 	PIPE_CONTROL_FLUSH(ring, scratch_addr);
-	scratch_addr += 128;
+	scratch_addr += 2 * CACHELINE_BYTES;
 	PIPE_CONTROL_FLUSH(ring, scratch_addr);
 
 	intel_ring_emit(ring, GFX_OP_PIPE_CONTROL(4) | PIPE_CONTROL_QW_WRITE |
@@ -1411,7 +1413,7 @@ static int intel_init_ring_buffer(struct drm_device *dev,
 	 */
 	ring->effective_size = ring->size;
 	if (IS_I830(ring->dev) || IS_845G(ring->dev))
-		ring->effective_size -= 128;
+		ring->effective_size -= 2 * CACHELINE_BYTES;
 
 	i915_cmd_parser_init_ring(ring);
 
@@ -1672,12 +1674,13 @@ int intel_ring_begin(struct intel_ring_buffer *ring,
 /* Align the ring tail to a cacheline boundary */
 int intel_ring_cacheline_align(struct intel_ring_buffer *ring)
 {
-	int num_dwords = (64 - (ring->tail & 63)) / sizeof(uint32_t);
+	int num_dwords = (ring->tail & (CACHELINE_BYTES - 1)) / sizeof(uint32_t);
 	int ret;
 
 	if (num_dwords == 0)
 		return 0;
 
+	num_dwords = CACHELINE_BYTES / sizeof(uint32_t) - num_dwords;
 	ret = intel_ring_begin(ring, num_dwords);
 	if (ret)
 		return ret;
@@ -2034,7 +2037,7 @@ int intel_render_ring_init_dri(struct drm_device *dev, u64 start, u32 size)
 	ring->size = size;
 	ring->effective_size = ring->size;
 	if (IS_I830(ring->dev) || IS_845G(ring->dev))
-		ring->effective_size -= 128;
+		ring->effective_size -= 2 * CACHELINE_BYTES;
 
 	ring->virtual_start = ioremap_wc(start, size);
 	if (ring->virtual_start == NULL) {
-- 
1.9.1

[PATCH 2/6] drm/i915: Move all ring resets before setting the HWS page

[Chris Wilson]

In commit a51435a3137ad8ae75c288c39bd2d8b2696bae8f
Author: Naresh Kumar Kachhi <naresh.kumar.kachhi@intel.com>
Date:   Wed Mar 12 16:39:40 2014 +0530

    drm/i915: disable rings before HW status page setup

we reordered stopping the rings to do so before we set the HWS register.
However, there is an extra workaround for g45 to reset the rings twice,
and for consistency we should apply that workaround before setting the
HWS to be sure that the rings are truly stopped.

Cc: Naresh Kumar Kachhi <naresh.kumar.kachhi@intel.com>
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_reg.h         |  1 +
 drivers/gpu/drm/i915/intel_ringbuffer.c | 54 +++++++++++++++++++++------------
 drivers/gpu/drm/i915/intel_ringbuffer.h |  1 +
 3 files changed, 36 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 393f93ecd41a..6dd35f849781 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -915,6 +915,7 @@ enum punit_power_well {
 # define MI_FLUSH_ENABLE				(1 << 12)
 # define ASYNC_FLIP_PERF_DISABLE			(1 << 14)
 # define MODE_IDLE					(1 << 9)
+# define STOP_RING					(1 << 8)
 
 #define GEN6_GT_MODE	0x20d0
 #define GEN7_GT_MODE	0x7008
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 475391ce671a..98ba7d594718 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -439,32 +439,41 @@ static void ring_setup_phys_status_page(struct intel_ring_buffer *ring)
 	I915_WRITE(HWS_PGA, addr);
 }
 
-static int init_ring_common(struct intel_ring_buffer *ring)
+static bool stop_ring(struct intel_ring_buffer *ring)
 {
-	struct drm_device *dev = ring->dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct drm_i915_gem_object *obj = ring->obj;
-	int ret = 0;
-	u32 head;
+	struct drm_i915_private *dev_priv = to_i915(ring->dev);
 
-	gen6_gt_force_wake_get(dev_priv, FORCEWAKE_ALL);
+	if (!IS_GEN2(ring->dev)) {
+		I915_WRITE_MODE(ring, _MASKED_BIT_ENABLE(STOP_RING));
+		if (wait_for_atomic((I915_READ_MODE(ring) & MODE_IDLE) != 0, 1000)) {
+			DRM_ERROR("%s :timed out trying to stop ring\n", ring->name);
+			return false;
+		}
+	}
 
-	/* Stop the ring if it's running. */
 	I915_WRITE_CTL(ring, 0);
 	I915_WRITE_HEAD(ring, 0);
 	ring->write_tail(ring, 0);
-	if (wait_for_atomic((I915_READ_MODE(ring) & MODE_IDLE) != 0, 1000))
-		DRM_ERROR("%s :timed out trying to stop ring\n", ring->name);
 
-	if (I915_NEED_GFX_HWS(dev))
-		intel_ring_setup_status_page(ring);
-	else
-		ring_setup_phys_status_page(ring);
+	if (!IS_GEN2(ring->dev)) {
+		(void)I915_READ_CTL(ring);
+		I915_WRITE_MODE(ring, _MASKED_BIT_DISABLE(STOP_RING));
+	}
 
-	head = I915_READ_HEAD(ring) & HEAD_ADDR;
+	return (I915_READ_HEAD(ring) & HEAD_ADDR) == 0;
+}
 
-	/* G45 ring initialization fails to reset head to zero */
-	if (head != 0) {
+static int init_ring_common(struct intel_ring_buffer *ring)
+{
+	struct drm_device *dev = ring->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct drm_i915_gem_object *obj = ring->obj;
+	int ret = 0;
+
+	gen6_gt_force_wake_get(dev_priv, FORCEWAKE_ALL);
+
+	if (!stop_ring(ring)) {
+		/* G45 ring initialization often fails to reset head to zero */
 		DRM_DEBUG_KMS("%s head not reset to zero "
 			      "ctl %08x head %08x tail %08x start %08x\n",
 			      ring->name,
@@ -473,9 +482,7 @@ static int init_ring_common(struct intel_ring_buffer *ring)
 			      I915_READ_TAIL(ring),
 			      I915_READ_START(ring));
 
-		I915_WRITE_HEAD(ring, 0);
-
-		if (I915_READ_HEAD(ring) & HEAD_ADDR) {
+		if (!stop_ring(ring)) {
 			DRM_ERROR("failed to set %s head to zero "
 				  "ctl %08x head %08x tail %08x start %08x\n",
 				  ring->name,
@@ -483,9 +490,16 @@ static int init_ring_common(struct intel_ring_buffer *ring)
 				  I915_READ_HEAD(ring),
 				  I915_READ_TAIL(ring),
 				  I915_READ_START(ring));
+			ret = -EIO;
+			goto out;
 		}
 	}
 
+	if (I915_NEED_GFX_HWS(dev))
+		intel_ring_setup_status_page(ring);
+	else
+		ring_setup_phys_status_page(ring);
+
 	/* Initialize the ring. This must happen _after_ we've cleared the ring
 	 * registers with the above sequence (the readback of the HEAD registers
 	 * also enforces ordering), otherwise the hw might lose the new ring
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index 270a6a973438..2b91c4b4d34b 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -34,6 +34,7 @@ struct  intel_hw_status_page {
 #define I915_WRITE_IMR(ring, val) I915_WRITE(RING_IMR((ring)->mmio_base), val)
 
 #define I915_READ_MODE(ring) I915_READ(RING_MI_MODE((ring)->mmio_base))
+#define I915_WRITE_MODE(ring, val) I915_WRITE(RING_MI_MODE((ring)->mmio_base), val)
 
 enum intel_ring_hangcheck_action {
 	HANGCHECK_IDLE = 0,
-- 
1.9.1

[PATCH 3/6] drm/i915: Preserve ring buffers objects across resume

[Chris Wilson]

Tearing down the ring buffers across resume is overkill, risks
unnecessary failure and increases fragmentation.

After failure, since the device is still active we may end up trying to
write into the dangling iomapping and trigger an oops.

Reported-by: Jae-hyeon Park <jhyeon@gmail.com>
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=72351
References: https://bugs.freedesktop.org/show_bug.cgi?id=76554
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_gem.c         |  20 +++-
 drivers/gpu/drm/i915/intel_ringbuffer.c | 168 +++++++++++++++++---------------
 drivers/gpu/drm/i915/intel_ringbuffer.h |   1 +
 3 files changed, 105 insertions(+), 84 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index c70121dbce78..0d5f8fb15677 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2383,6 +2383,11 @@ static void i915_gem_reset_ring_cleanup(struct drm_i915_private *dev_priv,
 
 		i915_gem_free_request(request);
 	}
+
+	/* These may not have been flush before the reset, do so now */
+	kfree(ring->preallocated_lazy_request);
+	ring->preallocated_lazy_request = NULL;
+	ring->outstanding_lazy_seqno = 0;
 }
 
 void i915_gem_restore_fences(struct drm_device *dev)
@@ -2423,8 +2428,6 @@ void i915_gem_reset(struct drm_device *dev)
 	for_each_ring(ring, dev_priv, i)
 		i915_gem_reset_ring_cleanup(dev_priv, ring);
 
-	i915_gem_cleanup_ringbuffer(dev);
-
 	i915_gem_context_reset(dev);
 
 	i915_gem_restore_fences(dev);
@@ -4231,6 +4234,17 @@ void i915_gem_vma_destroy(struct i915_vma *vma)
 	kfree(vma);
 }
 
+static void
+i915_gem_stop_ringbuffers(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_ring_buffer *ring;
+	int i;
+
+	for_each_ring(ring, dev_priv, i)
+		intel_cleanup_ring_buffer(ring);
+}
+
 int
 i915_gem_suspend(struct drm_device *dev)
 {
@@ -4252,7 +4266,7 @@ i915_gem_suspend(struct drm_device *dev)
 		i915_gem_evict_everything(dev);
 
 	i915_kernel_lost_context(dev);
-	i915_gem_cleanup_ringbuffer(dev);
+	i915_gem_stop_ringbuffers(dev);
 
 	/* Hack!  Don't let anybody do execbuf while we don't control the chip.
 	 * We need to replace this with a semaphore, or something.
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 98ba7d594718..9bd32a34552f 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -1303,45 +1303,39 @@ static void cleanup_status_page(struct intel_ring_buffer *ring)
 
 static int init_status_page(struct intel_ring_buffer *ring)
 {
-	struct drm_device *dev = ring->dev;
 	struct drm_i915_gem_object *obj;
-	int ret;
 
-	obj = i915_gem_alloc_object(dev, 4096);
-	if (obj == NULL) {
-		DRM_ERROR("Failed to allocate status page\n");
-		ret = -ENOMEM;
-		goto err;
-	}
+	if ((obj = ring->status_page.obj) == NULL) {
+		int ret;
 
-	ret = i915_gem_object_set_cache_level(obj, I915_CACHE_LLC);
-	if (ret)
-		goto err_unref;
+		obj = i915_gem_alloc_object(ring->dev, 4096);
+		if (obj == NULL) {
+			DRM_ERROR("Failed to allocate status page\n");
+			return -ENOMEM;
+		}
 
-	ret = i915_gem_obj_ggtt_pin(obj, 4096, 0);
-	if (ret)
-		goto err_unref;
+		ret = i915_gem_object_set_cache_level(obj, I915_CACHE_LLC);
+		if (ret)
+			goto err_unref;
+
+		ret = i915_gem_obj_ggtt_pin(obj, 4096, 0);
+		if (ret) {
+err_unref:
+			drm_gem_object_unreference(&obj->base);
+			return ret;
+		}
+
+		ring->status_page.obj = obj;
+	}
 
 	ring->status_page.gfx_addr = i915_gem_obj_ggtt_offset(obj);
 	ring->status_page.page_addr = kmap(sg_page(obj->pages->sgl));
-	if (ring->status_page.page_addr == NULL) {
-		ret = -ENOMEM;
-		goto err_unpin;
-	}
-	ring->status_page.obj = obj;
 	memset(ring->status_page.page_addr, 0, PAGE_SIZE);
 
 	DRM_DEBUG_DRIVER("%s hws offset: 0x%08x\n",
 			ring->name, ring->status_page.gfx_addr);
 
 	return 0;
-
-err_unpin:
-	i915_gem_object_ggtt_unpin(obj);
-err_unref:
-	drm_gem_object_unreference(&obj->base);
-err:
-	return ret;
 }
 
 static int init_phys_status_page(struct intel_ring_buffer *ring)
@@ -1361,44 +1355,23 @@ static int init_phys_status_page(struct intel_ring_buffer *ring)
 	return 0;
 }
 
-static int intel_init_ring_buffer(struct drm_device *dev,
-				  struct intel_ring_buffer *ring)
+static int allocate_ring_buffer(struct intel_ring_buffer *ring)
 {
+	struct drm_device *dev = ring->dev;
+	struct drm_i915_private *dev_priv = to_i915(dev);
 	struct drm_i915_gem_object *obj;
-	struct drm_i915_private *dev_priv = dev->dev_private;
 	int ret;
 
-	ring->dev = dev;
-	INIT_LIST_HEAD(&ring->active_list);
-	INIT_LIST_HEAD(&ring->request_list);
-	ring->size = 32 * PAGE_SIZE;
-	memset(ring->sync_seqno, 0, sizeof(ring->sync_seqno));
-
-	init_waitqueue_head(&ring->irq_queue);
-
-	if (I915_NEED_GFX_HWS(dev)) {
-		ret = init_status_page(ring);
-		if (ret)
-			return ret;
-	} else {
-		BUG_ON(ring->id != RCS);
-		ret = init_phys_status_page(ring);
-		if (ret)
-			return ret;
-	}
+	if (ring->obj)
+		return 0;
 
 	obj = NULL;
 	if (!HAS_LLC(dev))
 		obj = i915_gem_object_create_stolen(dev, ring->size);
 	if (obj == NULL)
 		obj = i915_gem_alloc_object(dev, ring->size);
-	if (obj == NULL) {
-		DRM_ERROR("Failed to allocate ringbuffer\n");
-		ret = -ENOMEM;
-		goto err_hws;
-	}
-
-	ring->obj = obj;
+	if (obj == NULL)
+		return -ENOMEM;
 
 	ret = i915_gem_obj_ggtt_pin(obj, PAGE_SIZE, PIN_MAPPABLE);
 	if (ret)
@@ -1412,55 +1385,72 @@ static int intel_init_ring_buffer(struct drm_device *dev,
 		ioremap_wc(dev_priv->gtt.mappable_base + i915_gem_obj_ggtt_offset(obj),
 			   ring->size);
 	if (ring->virtual_start == NULL) {
-		DRM_ERROR("Failed to map ringbuffer.\n");
 		ret = -EINVAL;
 		goto err_unpin;
 	}
 
-	ret = ring->init(ring);
-	if (ret)
-		goto err_unmap;
+	ring->obj = obj;
+	return 0;
+
+err_unpin:
+	i915_gem_object_ggtt_unpin(obj);
+err_unref:
+	drm_gem_object_unreference(&obj->base);
+	return ret;
+}
+
+static int intel_init_ring_buffer(struct drm_device *dev,
+				  struct intel_ring_buffer *ring)
+{
+	int ret;
+
+	ring->dev = dev;
+	INIT_LIST_HEAD(&ring->active_list);
+	INIT_LIST_HEAD(&ring->request_list);
+	ring->size = 32 * PAGE_SIZE;
+	memset(ring->sync_seqno, 0, sizeof(ring->sync_seqno));
+
+	init_waitqueue_head(&ring->irq_queue);
+
+	if (I915_NEED_GFX_HWS(dev)) {
+		ret = init_status_page(ring);
+		if (ret)
+			return ret;
+	} else {
+		BUG_ON(ring->id != RCS);
+		ret = init_phys_status_page(ring);
+		if (ret)
+			return ret;
+	}
+
+	ret = allocate_ring_buffer(ring);
+	if (ret) {
+		DRM_ERROR("Failed to allocate ringbuffer %s: %d\n", ring->name, ret);
+		return ret;
+	}
 
 	/* Workaround an erratum on the i830 which causes a hang if
 	 * the TAIL pointer points to within the last 2 cachelines
 	 * of the buffer.
 	 */
 	ring->effective_size = ring->size;
-	if (IS_I830(ring->dev) || IS_845G(ring->dev))
+	if (IS_I830(dev) || IS_845G(dev))
 		ring->effective_size -= 2 * CACHELINE_BYTES;
 
 	i915_cmd_parser_init_ring(ring);
 
-	return 0;
-
-err_unmap:
-	iounmap(ring->virtual_start);
-err_unpin:
-	i915_gem_object_ggtt_unpin(obj);
-err_unref:
-	drm_gem_object_unreference(&obj->base);
-	ring->obj = NULL;
-err_hws:
-	cleanup_status_page(ring);
-	return ret;
+	return ring->init(ring);
 }
 
 void intel_cleanup_ring_buffer(struct intel_ring_buffer *ring)
 {
-	struct drm_i915_private *dev_priv;
-	int ret;
+	struct drm_i915_private *dev_priv = to_i915(ring->dev);
 
 	if (ring->obj == NULL)
 		return;
 
-	/* Disable the ring buffer. The ring must be idle at this point */
-	dev_priv = ring->dev->dev_private;
-	ret = intel_ring_idle(ring);
-	if (ret && !i915_reset_in_progress(&dev_priv->gpu_error))
-		DRM_ERROR("failed to quiesce %s whilst cleaning up: %d\n",
-			  ring->name, ret);
-
-	I915_WRITE_CTL(ring, 0);
+	intel_stop_ring_buffer(ring);
+	WARN_ON((I915_READ_MODE(ring) & MODE_IDLE) == 0);
 
 	iounmap(ring->virtual_start);
 
@@ -2250,3 +2240,19 @@ intel_ring_invalidate_all_caches(struct intel_ring_buffer *ring)
 	ring->gpu_caches_dirty = false;
 	return 0;
 }
+
+void
+intel_stop_ring_buffer(struct intel_ring_buffer *ring)
+{
+	int ret;
+
+	if (ring->obj == NULL)
+		return;
+
+	ret = intel_ring_idle(ring);
+	if (ret && !i915_reset_in_progress(&to_i915(ring->dev)->gpu_error))
+		DRM_ERROR("failed to quiesce %s whilst cleaning up: %d\n",
+			  ring->name, ret);
+
+	stop_ring(ring);
+}
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index 2b91c4b4d34b..06c6407476cf 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -267,6 +267,7 @@ intel_write_status_page(struct intel_ring_buffer *ring,
 #define I915_GEM_HWS_SCRATCH_INDEX	0x30
 #define I915_GEM_HWS_SCRATCH_ADDR (I915_GEM_HWS_SCRATCH_INDEX << MI_STORE_DWORD_INDEX_SHIFT)
 
+void intel_stop_ring_buffer(struct intel_ring_buffer *ring);
 void intel_cleanup_ring_buffer(struct intel_ring_buffer *ring);
 
 int __must_check intel_ring_begin(struct intel_ring_buffer *ring, int n);
-- 
1.9.1

[PATCH 4/6] drm/i915: Allow the module to load even if we fail to setup rings

[Chris Wilson]

Even without enabling the ringbuffers to allow command execution, we can
still control the display engines to enable modesetting. So make the
ringbuffer initialization failure soft, and mark the GPU as wedged
instead.

v2: Only treat an EIO from ring initialisation as a soft failure, and
abort module load for any other failure, such as allocation failures.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_gem.c | 24 +++++++++++-------------
 1 file changed, 11 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 0d5f8fb15677..850ec98239f4 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4450,15 +4450,11 @@ i915_gem_init_hw(struct drm_device *dev)
 	 * the do_switch), but before enabling PPGTT. So don't move this.
 	 */
 	ret = i915_gem_context_enable(dev_priv);
-	if (ret) {
+	if (ret && ret != -EIO) {
 		DRM_ERROR("Context enable failed %d\n", ret);
-		goto err_out;
+		i915_gem_cleanup_ringbuffer(dev);
 	}
 
-	return 0;
-
-err_out:
-	i915_gem_cleanup_ringbuffer(dev);
 	return ret;
 }
 
@@ -4485,18 +4481,20 @@ int i915_gem_init(struct drm_device *dev)
 	}
 
 	ret = i915_gem_init_hw(dev);
-	mutex_unlock(&dev->struct_mutex);
-	if (ret) {
-		WARN_ON(dev_priv->mm.aliasing_ppgtt);
-		i915_gem_context_fini(dev);
-		drm_mm_takedown(&dev_priv->gtt.base.mm);
-		return ret;
+	if (ret == -EIO) {
+		/* Allow ring initialisation to fail by marking the GPU as
+		 * wedged. But we only want to do this where the GPU is angry,
+		 * for all other failure, such as an allocation failure, bail.
+		 */
+		atomic_set_mask(I915_WEDGED, &dev_priv->gpu_error.reset_counter);
+		ret = 0;
 	}
+	mutex_unlock(&dev->struct_mutex);
 
 	/* Allow hardware batchbuffers unless told otherwise, but not for KMS. */
 	if (!drm_core_check_feature(dev, DRIVER_MODESET))
 		dev_priv->dri1.allow_batchbuffer = 1;
-	return 0;
+	return ret;
 }
 
 void
-- 
1.9.1

[PATCH 5/6] drm/i915: Mark device as wedged if we fail to resume

[Chris Wilson]

During module load, if we fail to initialise the rings, we abort the
load reporting EIO. However during resume, even though we report EIO as
we fail to reinitialize the ringbuffers, the resume continues and the
device is restored - albeit in a non-functional state. As we cannot
execute any commands on the GPU, it is effectively wedged, mark it so.

As we now preserve the ringbuffers across resume, this should prevent
UXA from falling into the trap of repeatedly sending invalid
batchbuffers and dropping all further rendering into /dev/null.

Reported-and-tested-by: Jiri Kosina <jkosina@suse.cz>
References: https://bugs.freedesktop.org/show_bug.cgi?id=76554
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_drv.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index a01faea0401c..032a74b0b8e9 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -540,7 +540,6 @@ static void intel_resume_hotplug(struct drm_device *dev)
 static int __i915_drm_thaw(struct drm_device *dev, bool restore_gtt_mappings)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	int error = 0;
 
 	intel_uncore_early_sanitize(dev);
 
@@ -564,8 +563,10 @@ static int __i915_drm_thaw(struct drm_device *dev, bool restore_gtt_mappings)
 		drm_mode_config_reset(dev);
 
 		mutex_lock(&dev->struct_mutex);
-
-		error = i915_gem_init_hw(dev);
+		if (i915_gem_init_hw(dev)) {
+			DRM_ERROR("failed to re-initialize GPU, declaring wedged!\n");
+			atomic_set_mask(I915_WEDGED, &dev_priv->gpu_error.reset_counter);
+		}
 		mutex_unlock(&dev->struct_mutex);
 
 		/* We need working interrupts for modeset enabling ... */
@@ -608,7 +609,7 @@ static int __i915_drm_thaw(struct drm_device *dev, bool restore_gtt_mappings)
 	mutex_unlock(&dev_priv->modeset_restore_lock);
 
 	intel_runtime_pm_put(dev_priv);
-	return error;
+	return 0;
 }
 
 static int i915_drm_thaw(struct drm_device *dev)
-- 
1.9.1

[PATCH 6/6] drm/i915: Include a little more information about why ring init fails

[Chris Wilson]

If we include the expected values for the failing ring register checks,
it makes it marginally easier to see which is the culprit.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_ringbuffer.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 9bd32a34552f..849ffc79ecd7 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -514,12 +514,11 @@ static int init_ring_common(struct intel_ring_buffer *ring)
 		     I915_READ_START(ring) == i915_gem_obj_ggtt_offset(obj) &&
 		     (I915_READ_HEAD(ring) & HEAD_ADDR) == 0, 50)) {
 		DRM_ERROR("%s initialization failed "
-				"ctl %08x head %08x tail %08x start %08x\n",
-				ring->name,
-				I915_READ_CTL(ring),
-				I915_READ_HEAD(ring),
-				I915_READ_TAIL(ring),
-				I915_READ_START(ring));
+			  "ctl %08x (valid? %d) head %08x tail %08x start %08x [expected %08x]\n",
+			  ring->name,
+			  I915_READ_CTL(ring), I915_READ_CTL(ring) & RING_VALID,
+			  I915_READ_HEAD(ring), I915_READ_TAIL(ring),
+			  I915_READ_START(ring), i915_gem_obj_ggtt_offset(obj));
 		ret = -EIO;
 		goto out;
 	}
-- 
1.9.1

Re: [PATCH 1/6] drm/i915: Replace hardcoded cacheline size with macro

[Jesse Barnes]

On Wed,  2 Apr 2014 16:36:06 +0100
Chris Wilson <chris@chris-wilson.co.uk> wrote:

> For readibility and guess at the meaning behind the constants.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/intel_ringbuffer.c | 29 ++++++++++++++++-------------
>  1 file changed, 16 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index 785f246d28a8..475391ce671a 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -33,6 +33,8 @@
>  #include "i915_trace.h"
>  #include "intel_drv.h"
>  
> +#define CACHELINE_BYTES 64
> +

Are you sure it's 64 on every gen?  It changes on the CPU side from
time to time... I thought it might have changed over time on the GPU
too but I haven't checked the specs.

Either way a doc ref would be nice here.

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 2/6] drm/i915: Move all ring resets before setting the HWS page

[Jesse Barnes]

On Wed,  2 Apr 2014 16:36:07 +0100
Chris Wilson <chris@chris-wilson.co.uk> wrote:

> In commit a51435a3137ad8ae75c288c39bd2d8b2696bae8f
> Author: Naresh Kumar Kachhi <naresh.kumar.kachhi@intel.com>
> Date:   Wed Mar 12 16:39:40 2014 +0530
> 
>     drm/i915: disable rings before HW status page setup
> 
> we reordered stopping the rings to do so before we set the HWS register.
> However, there is an extra workaround for g45 to reset the rings twice,
> and for consistency we should apply that workaround before setting the
> HWS to be sure that the rings are truly stopped.
> 
> Cc: Naresh Kumar Kachhi <naresh.kumar.kachhi@intel.com>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/i915_reg.h         |  1 +
>  drivers/gpu/drm/i915/intel_ringbuffer.c | 54 +++++++++++++++++++++------------
>  drivers/gpu/drm/i915/intel_ringbuffer.h |  1 +
>  3 files changed, 36 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 393f93ecd41a..6dd35f849781 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -915,6 +915,7 @@ enum punit_power_well {
>  # define MI_FLUSH_ENABLE				(1 << 12)
>  # define ASYNC_FLIP_PERF_DISABLE			(1 << 14)
>  # define MODE_IDLE					(1 << 9)
> +# define STOP_RING					(1 << 8)
>  
>  #define GEN6_GT_MODE	0x20d0
>  #define GEN7_GT_MODE	0x7008
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index 475391ce671a..98ba7d594718 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -439,32 +439,41 @@ static void ring_setup_phys_status_page(struct intel_ring_buffer *ring)
>  	I915_WRITE(HWS_PGA, addr);
>  }
>  
> -static int init_ring_common(struct intel_ring_buffer *ring)
> +static bool stop_ring(struct intel_ring_buffer *ring)
>  {
> -	struct drm_device *dev = ring->dev;
> -	struct drm_i915_private *dev_priv = dev->dev_private;
> -	struct drm_i915_gem_object *obj = ring->obj;
> -	int ret = 0;
> -	u32 head;
> +	struct drm_i915_private *dev_priv = to_i915(ring->dev);
>  
> -	gen6_gt_force_wake_get(dev_priv, FORCEWAKE_ALL);
> +	if (!IS_GEN2(ring->dev)) {
> +		I915_WRITE_MODE(ring, _MASKED_BIT_ENABLE(STOP_RING));
> +		if (wait_for_atomic((I915_READ_MODE(ring) & MODE_IDLE) != 0, 1000)) {
> +			DRM_ERROR("%s :timed out trying to stop ring\n", ring->name);
> +			return false;
> +		}
> +	}
>  
> -	/* Stop the ring if it's running. */
>  	I915_WRITE_CTL(ring, 0);
>  	I915_WRITE_HEAD(ring, 0);
>  	ring->write_tail(ring, 0);
> -	if (wait_for_atomic((I915_READ_MODE(ring) & MODE_IDLE) != 0, 1000))
> -		DRM_ERROR("%s :timed out trying to stop ring\n", ring->name);
>  
> -	if (I915_NEED_GFX_HWS(dev))
> -		intel_ring_setup_status_page(ring);
> -	else
> -		ring_setup_phys_status_page(ring);
> +	if (!IS_GEN2(ring->dev)) {
> +		(void)I915_READ_CTL(ring);
> +		I915_WRITE_MODE(ring, _MASKED_BIT_DISABLE(STOP_RING));
> +	}
>  
> -	head = I915_READ_HEAD(ring) & HEAD_ADDR;
> +	return (I915_READ_HEAD(ring) & HEAD_ADDR) == 0;
> +}
>  
> -	/* G45 ring initialization fails to reset head to zero */
> -	if (head != 0) {
> +static int init_ring_common(struct intel_ring_buffer *ring)
> +{
> +	struct drm_device *dev = ring->dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct drm_i915_gem_object *obj = ring->obj;
> +	int ret = 0;
> +
> +	gen6_gt_force_wake_get(dev_priv, FORCEWAKE_ALL);
> +
> +	if (!stop_ring(ring)) {
> +		/* G45 ring initialization often fails to reset head to zero */
>  		DRM_DEBUG_KMS("%s head not reset to zero "
>  			      "ctl %08x head %08x tail %08x start %08x\n",
>  			      ring->name,
> @@ -473,9 +482,7 @@ static int init_ring_common(struct intel_ring_buffer *ring)
>  			      I915_READ_TAIL(ring),
>  			      I915_READ_START(ring));
>  
> -		I915_WRITE_HEAD(ring, 0);
> -
> -		if (I915_READ_HEAD(ring) & HEAD_ADDR) {
> +		if (!stop_ring(ring)) {
>  			DRM_ERROR("failed to set %s head to zero "
>  				  "ctl %08x head %08x tail %08x start %08x\n",
>  				  ring->name,
> @@ -483,9 +490,16 @@ static int init_ring_common(struct intel_ring_buffer *ring)
>  				  I915_READ_HEAD(ring),
>  				  I915_READ_TAIL(ring),
>  				  I915_READ_START(ring));
> +			ret = -EIO;
> +			goto out;
>  		}
>  	}
>  
> +	if (I915_NEED_GFX_HWS(dev))
> +		intel_ring_setup_status_page(ring);
> +	else
> +		ring_setup_phys_status_page(ring);
> +
>  	/* Initialize the ring. This must happen _after_ we've cleared the ring
>  	 * registers with the above sequence (the readback of the HEAD registers
>  	 * also enforces ordering), otherwise the hw might lose the new ring
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> index 270a6a973438..2b91c4b4d34b 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -34,6 +34,7 @@ struct  intel_hw_status_page {
>  #define I915_WRITE_IMR(ring, val) I915_WRITE(RING_IMR((ring)->mmio_base), val)
>  
>  #define I915_READ_MODE(ring) I915_READ(RING_MI_MODE((ring)->mmio_base))
> +#define I915_WRITE_MODE(ring, val) I915_WRITE(RING_MI_MODE((ring)->mmio_base), val)
>  
>  enum intel_ring_hangcheck_action {
>  	HANGCHECK_IDLE = 0,

Bad Chris, mixing a nice refactor and a nice fix in the same patch.
I'll still give you a cookie though.

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 3/6] drm/i915: Preserve ring buffers objects across resume

[Jesse Barnes]

On Wed,  2 Apr 2014 16:36:08 +0100
Chris Wilson <chris@chris-wilson.co.uk> wrote:

> +static void
> +i915_gem_stop_ringbuffers(struct drm_device *dev)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct intel_ring_buffer *ring;
> +	int i;
> +
> +	for_each_ring(ring, dev_priv, i)
> +		intel_cleanup_ring_buffer(ring);
> +}
> +

You've improved the init/alloc ringbuffer naming, but this one
confuses me.  stop_ringbuffers to me says we're just idling them, but
cleanup_ring_buffer actually does the unmap and free right?

If so, it looks like this will still tear things down on suspend?

Maybe it's all the refactoring making me miss it. :)

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 4/6] drm/i915: Allow the module to load even if we fail to setup rings

[Jesse Barnes]

On Wed,  2 Apr 2014 16:36:09 +0100
Chris Wilson <chris@chris-wilson.co.uk> wrote:

> Even without enabling the ringbuffers to allow command execution, we can
> still control the display engines to enable modesetting. So make the
> ringbuffer initialization failure soft, and mark the GPU as wedged
> instead.
> 
> v2: Only treat an EIO from ring initialisation as a soft failure, and
> abort module load for any other failure, such as allocation failures.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/i915_gem.c | 24 +++++++++++-------------
>  1 file changed, 11 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 0d5f8fb15677..850ec98239f4 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -4450,15 +4450,11 @@ i915_gem_init_hw(struct drm_device *dev)
>  	 * the do_switch), but before enabling PPGTT. So don't move this.
>  	 */
>  	ret = i915_gem_context_enable(dev_priv);
> -	if (ret) {
> +	if (ret && ret != -EIO) {
>  		DRM_ERROR("Context enable failed %d\n", ret);
> -		goto err_out;
> +		i915_gem_cleanup_ringbuffer(dev);
>  	}
>  
> -	return 0;
> -
> -err_out:
> -	i915_gem_cleanup_ringbuffer(dev);
>  	return ret;
>  }
>  
> @@ -4485,18 +4481,20 @@ int i915_gem_init(struct drm_device *dev)
>  	}
>  
>  	ret = i915_gem_init_hw(dev);
> -	mutex_unlock(&dev->struct_mutex);
> -	if (ret) {
> -		WARN_ON(dev_priv->mm.aliasing_ppgtt);
> -		i915_gem_context_fini(dev);
> -		drm_mm_takedown(&dev_priv->gtt.base.mm);
> -		return ret;
> +	if (ret == -EIO) {
> +		/* Allow ring initialisation to fail by marking the GPU as
> +		 * wedged. But we only want to do this where the GPU is angry,
> +		 * for all other failure, such as an allocation failure, bail.
> +		 */
> +		atomic_set_mask(I915_WEDGED, &dev_priv->gpu_error.reset_counter);
> +		ret = 0;
>  	}
> +	mutex_unlock(&dev->struct_mutex);
>  
>  	/* Allow hardware batchbuffers unless told otherwise, but not for KMS. */
>  	if (!drm_core_check_feature(dev, DRIVER_MODESET))
>  		dev_priv->dri1.allow_batchbuffer = 1;
> -	return 0;
> +	return ret;
>  }
>  
>  void

Will we still have some loud spewing into the log in this case?  If so,
then
Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 4/6] drm/i915: Allow the module to load even if we fail to setup rings

[Chris Wilson]

On Wed, Apr 02, 2014 at 03:11:40PM -0700, Jesse Barnes wrote:
> On Wed,  2 Apr 2014 16:36:09 +0100
> Chris Wilson <chris@chris-wilson.co.uk> wrote:
> 
> > Even without enabling the ringbuffers to allow command execution, we can
> > still control the display engines to enable modesetting. So make the
> > ringbuffer initialization failure soft, and mark the GPU as wedged
> > instead.
> > 
> > v2: Only treat an EIO from ring initialisation as a soft failure, and
> > abort module load for any other failure, such as allocation failures.

> Will we still have some loud spewing into the log in this case?  If so,

We have the usual *ERROR*, but we lack an indication that the GPU is now
wedged.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 3/6] drm/i915: Preserve ring buffers objects across resume

[Chris Wilson]

On Wed, Apr 02, 2014 at 03:10:13PM -0700, Jesse Barnes wrote:
> On Wed,  2 Apr 2014 16:36:08 +0100
> Chris Wilson <chris@chris-wilson.co.uk> wrote:
> 
> > +static void
> > +i915_gem_stop_ringbuffers(struct drm_device *dev)
> > +{
> > +	struct drm_i915_private *dev_priv = dev->dev_private;
> > +	struct intel_ring_buffer *ring;
> > +	int i;
> > +
> > +	for_each_ring(ring, dev_priv, i)
> > +		intel_cleanup_ring_buffer(ring);
> > +}
> > +
> 
> You've improved the init/alloc ringbuffer naming, but this one
> confuses me.  stop_ringbuffers to me says we're just idling them, but
> cleanup_ring_buffer actually does the unmap and free right?
> 
> If so, it looks like this will still tear things down on suspend?
> 
> Maybe it's all the refactoring making me miss it. :)

No, something went wrong in the rebase... stop_ringbuffers(dev) is meant to
call foreach(ring) stop_ring_buffer(ring).
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 1/6] drm/i915: Replace hardcoded cacheline size with macro

[Chris Wilson]

On Wed, Apr 02, 2014 at 02:57:11PM -0700, Jesse Barnes wrote:
> On Wed,  2 Apr 2014 16:36:06 +0100
> Chris Wilson <chris@chris-wilson.co.uk> wrote:
> 
> > For readibility and guess at the meaning behind the constants.
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > ---
> >  drivers/gpu/drm/i915/intel_ringbuffer.c | 29 ++++++++++++++++-------------
> >  1 file changed, 16 insertions(+), 13 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> > index 785f246d28a8..475391ce671a 100644
> > --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> > @@ -33,6 +33,8 @@
> >  #include "i915_trace.h"
> >  #include "intel_drv.h"
> >  
> > +#define CACHELINE_BYTES 64
> > +
> 
> Are you sure it's 64 on every gen?  It changes on the CPU side from
> time to time... I thought it might have changed over time on the GPU
> too but I haven't checked the specs.

The cacheline is 32bytes on gen2, 64 elsewhere. We've made a blanket
assumption of 64 and then some random factors on top. (Some cachelines
may be as large as 1024bytes elsewhere in the chip.) I'm not sure where
some of the values used in the code where plucked from, Jesse?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 1/6] drm/i915: Replace hardcoded cacheline size with macro

[Daniel Vetter]

On Thu, Apr 03, 2014 at 07:45:40AM +0100, Chris Wilson wrote:
> On Wed, Apr 02, 2014 at 02:57:11PM -0700, Jesse Barnes wrote:
> > On Wed,  2 Apr 2014 16:36:06 +0100
> > Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > 
> > > For readibility and guess at the meaning behind the constants.
> > > 
> > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > ---
> > >  drivers/gpu/drm/i915/intel_ringbuffer.c | 29 ++++++++++++++++-------------
> > >  1 file changed, 16 insertions(+), 13 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> > > index 785f246d28a8..475391ce671a 100644
> > > --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> > > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> > > @@ -33,6 +33,8 @@
> > >  #include "i915_trace.h"
> > >  #include "intel_drv.h"
> > >  
> > > +#define CACHELINE_BYTES 64
> > > +
> > 
> > Are you sure it's 64 on every gen?  It changes on the CPU side from
> > time to time... I thought it might have changed over time on the GPU
> > too but I haven't checked the specs.
> 
> The cacheline is 32bytes on gen2, 64 elsewhere. We've made a blanket
> assumption of 64 and then some random factors on top. (Some cachelines
> may be as large as 1024bytes elsewhere in the chip.) I'm not sure where
> some of the values used in the code where plucked from, Jesse?

Maybe a quick comment that this is the maximum and that gen2 has only
32bytes but aligning more isn't harmful?

Perhaps also mention that for actual cacheline flushing we _must_ use the
cl size of the cpu for otherwise we don't flush poperly. If your define
has some comment/warning to that effect I'm ok with this generalization.
And it nicely makes some of the additional lore (128bytes ?!) stick out
more.

btw the 1k thing at least on i865G is iirc just the writeout fifo between
the cpu and the gmch to paper over FSB latencies (or whatever irked hw
designers).
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 2/6] drm/i915: Move all ring resets before setting the HWS page

[Daniel Vetter]

On Wed, Apr 02, 2014 at 02:58:54PM -0700, Jesse Barnes wrote:
> On Wed,  2 Apr 2014 16:36:07 +0100
> Chris Wilson <chris@chris-wilson.co.uk> wrote:
> 
> > In commit a51435a3137ad8ae75c288c39bd2d8b2696bae8f
> > Author: Naresh Kumar Kachhi <naresh.kumar.kachhi@intel.com>
> > Date:   Wed Mar 12 16:39:40 2014 +0530
> > 
> >     drm/i915: disable rings before HW status page setup
> > 
> > we reordered stopping the rings to do so before we set the HWS register.
> > However, there is an extra workaround for g45 to reset the rings twice,
> > and for consistency we should apply that workaround before setting the
> > HWS to be sure that the rings are truly stopped.
> > 
> > Cc: Naresh Kumar Kachhi <naresh.kumar.kachhi@intel.com>
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > ---
> >  drivers/gpu/drm/i915/i915_reg.h         |  1 +
> >  drivers/gpu/drm/i915/intel_ringbuffer.c | 54 +++++++++++++++++++++------------
> >  drivers/gpu/drm/i915/intel_ringbuffer.h |  1 +
> >  3 files changed, 36 insertions(+), 20 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index 393f93ecd41a..6dd35f849781 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -915,6 +915,7 @@ enum punit_power_well {
> >  # define MI_FLUSH_ENABLE				(1 << 12)
> >  # define ASYNC_FLIP_PERF_DISABLE			(1 << 14)
> >  # define MODE_IDLE					(1 << 9)
> > +# define STOP_RING					(1 << 8)
> >  
> >  #define GEN6_GT_MODE	0x20d0
> >  #define GEN7_GT_MODE	0x7008
> > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> > index 475391ce671a..98ba7d594718 100644
> > --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> > @@ -439,32 +439,41 @@ static void ring_setup_phys_status_page(struct intel_ring_buffer *ring)
> >  	I915_WRITE(HWS_PGA, addr);
> >  }
> >  
> > -static int init_ring_common(struct intel_ring_buffer *ring)
> > +static bool stop_ring(struct intel_ring_buffer *ring)
> >  {
> > -	struct drm_device *dev = ring->dev;
> > -	struct drm_i915_private *dev_priv = dev->dev_private;
> > -	struct drm_i915_gem_object *obj = ring->obj;
> > -	int ret = 0;
> > -	u32 head;
> > +	struct drm_i915_private *dev_priv = to_i915(ring->dev);
> >  
> > -	gen6_gt_force_wake_get(dev_priv, FORCEWAKE_ALL);
> > +	if (!IS_GEN2(ring->dev)) {
> > +		I915_WRITE_MODE(ring, _MASKED_BIT_ENABLE(STOP_RING));
> > +		if (wait_for_atomic((I915_READ_MODE(ring) & MODE_IDLE) != 0, 1000)) {
> > +			DRM_ERROR("%s :timed out trying to stop ring\n", ring->name);
> > +			return false;
> > +		}
> > +	}
> >  
> > -	/* Stop the ring if it's running. */
> >  	I915_WRITE_CTL(ring, 0);
> >  	I915_WRITE_HEAD(ring, 0);
> >  	ring->write_tail(ring, 0);
> > -	if (wait_for_atomic((I915_READ_MODE(ring) & MODE_IDLE) != 0, 1000))
> > -		DRM_ERROR("%s :timed out trying to stop ring\n", ring->name);
> >  
> > -	if (I915_NEED_GFX_HWS(dev))
> > -		intel_ring_setup_status_page(ring);
> > -	else
> > -		ring_setup_phys_status_page(ring);
> > +	if (!IS_GEN2(ring->dev)) {
> > +		(void)I915_READ_CTL(ring);
> > +		I915_WRITE_MODE(ring, _MASKED_BIT_DISABLE(STOP_RING));
> > +	}
> >  
> > -	head = I915_READ_HEAD(ring) & HEAD_ADDR;
> > +	return (I915_READ_HEAD(ring) & HEAD_ADDR) == 0;
> > +}
> >  
> > -	/* G45 ring initialization fails to reset head to zero */
> > -	if (head != 0) {
> > +static int init_ring_common(struct intel_ring_buffer *ring)
> > +{
> > +	struct drm_device *dev = ring->dev;
> > +	struct drm_i915_private *dev_priv = dev->dev_private;
> > +	struct drm_i915_gem_object *obj = ring->obj;
> > +	int ret = 0;
> > +
> > +	gen6_gt_force_wake_get(dev_priv, FORCEWAKE_ALL);
> > +
> > +	if (!stop_ring(ring)) {
> > +		/* G45 ring initialization often fails to reset head to zero */
> >  		DRM_DEBUG_KMS("%s head not reset to zero "
> >  			      "ctl %08x head %08x tail %08x start %08x\n",
> >  			      ring->name,
> > @@ -473,9 +482,7 @@ static int init_ring_common(struct intel_ring_buffer *ring)
> >  			      I915_READ_TAIL(ring),
> >  			      I915_READ_START(ring));
> >  
> > -		I915_WRITE_HEAD(ring, 0);
> > -
> > -		if (I915_READ_HEAD(ring) & HEAD_ADDR) {
> > +		if (!stop_ring(ring)) {
> >  			DRM_ERROR("failed to set %s head to zero "
> >  				  "ctl %08x head %08x tail %08x start %08x\n",
> >  				  ring->name,
> > @@ -483,9 +490,16 @@ static int init_ring_common(struct intel_ring_buffer *ring)
> >  				  I915_READ_HEAD(ring),
> >  				  I915_READ_TAIL(ring),
> >  				  I915_READ_START(ring));
> > +			ret = -EIO;
> > +			goto out;
> >  		}
> >  	}
> >  
> > +	if (I915_NEED_GFX_HWS(dev))
> > +		intel_ring_setup_status_page(ring);
> > +	else
> > +		ring_setup_phys_status_page(ring);
> > +
> >  	/* Initialize the ring. This must happen _after_ we've cleared the ring
> >  	 * registers with the above sequence (the readback of the HEAD registers
> >  	 * also enforces ordering), otherwise the hw might lose the new ring
> > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> > index 270a6a973438..2b91c4b4d34b 100644
> > --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> > @@ -34,6 +34,7 @@ struct  intel_hw_status_page {
> >  #define I915_WRITE_IMR(ring, val) I915_WRITE(RING_IMR((ring)->mmio_base), val)
> >  
> >  #define I915_READ_MODE(ring) I915_READ(RING_MI_MODE((ring)->mmio_base))
> > +#define I915_WRITE_MODE(ring, val) I915_WRITE(RING_MI_MODE((ring)->mmio_base), val)
> >  
> >  enum intel_ring_hangcheck_action {
> >  	HANGCHECK_IDLE = 0,
> 
> Bad Chris, mixing a nice refactor and a nice fix in the same patch.
> I'll still give you a cookie though.
> 
> Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

Queued for -next, thanks for the patch. I'll let the other settle a bit
more.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 1/6] drm/i915: Replace hardcoded cacheline size with macro

[Chris Wilson]

On Thu, Apr 03, 2014 at 05:16:16PM +0200, Daniel Vetter wrote:
> btw the 1k thing at least on i865G is iirc just the writeout fifo between
> the cpu and the gmch to paper over FSB latencies (or whatever irked hw
> designers).

Isn't there a 1024 byte supercacheline for msaa as well? At least that
sticks out in my mind from the discussions on when not to use eDRAM etc.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 1/6] drm/i915: Replace hardcoded cacheline size with macro

[Daniel Vetter]

On Thu, Apr 03, 2014 at 04:23:05PM +0100, Chris Wilson wrote:
> On Thu, Apr 03, 2014 at 05:16:16PM +0200, Daniel Vetter wrote:
> > btw the 1k thing at least on i865G is iirc just the writeout fifo between
> > the cpu and the gmch to paper over FSB latencies (or whatever irked hw
> > designers).
> 
> Isn't there a 1024 byte supercacheline for msaa as well? At least that
> sticks out in my mind from the discussions on when not to use eDRAM etc.

I've thought that was just a recommendation to not use eDRAM with
compressed msaa buffers since for those you only use the pixels on the
edges of primitives. And since one msaa pixel is less than the 1024bytes
the supercacheline is actual edram utilization for common workloads would
be horrible.

Or at least that's how I've interpreted the graphs and diagrams in the
docs. Afaik the coherency mocs bits for edram are still available on a 64
byte boundary, it's just the address tag which is much larger.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch