[PATCH] drm/i915: Avoid reading through unaccessible PTEs during an error event

[PATCH] drm/i915: Avoid reading through unaccessible PTEs during an error event

[Chris Wilson]

We need to sanity check that the buffer is actually bound into the
mappable range of the GTT prior to reading it back through the GTT with
the CPU. Fortuitously, the only buffers we have been interested in so
far are constrained to be in the mappable region in order to handle
potential relocations. However, this can be relaxed in future and given
that the purpose is to read back following an error we should be extra
careful and not assume everything is safe.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_irq.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 26ac3bf..9b38226 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -708,7 +708,7 @@ i915_error_object_create(struct drm_i915_private *dev_priv,
 	int page, page_count;
 	u32 reloc_offset;
 
-	if (src == NULL || src->pages == NULL)
+	if (src == NULL || src->pages == NULL || !src->map_and_fenceable)
 		return NULL;
 
 	page_count = src->base.size / PAGE_SIZE;
-- 
1.7.7.3

Re: [PATCH] drm/i915: Avoid reading through unaccessible PTEs during an error event

[Daniel Vetter]

On Tue, Nov 29, 2011 at 09:35:10AM +0000, Chris Wilson wrote:
> We need to sanity check that the buffer is actually bound into the
> mappable range of the GTT prior to reading it back through the GTT with
> the CPU. Fortuitously, the only buffers we have been interested in so
> far are constrained to be in the mappable region in order to handle
> potential relocations. However, this can be relaxed in future and given
> that the purpose is to read back following an error we should be extra
> careful and not assume everything is safe.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

For the lazy-gtt binding we need to also check whether the ptes are
correct (they should because we pin buffers with relocations as mappable).
I'll add that additional paranoia to my series.
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

[PATCH] drm/i915: Handle unmappable buffers during error state capture

[Chris Wilson]

As the buffer is not necessarily accessible through the GTT at the time
of a GPU hang, and capturing some of its contents is far more valuable
than skipping it, provide a clflushed fallback read path. We still
prefer to read through the GTT as that is more consistent with the GPU
access of the same buffer. So example it will demonstrate any errorneous
tiling or swizzling of the command buffer as seen by the GPU.

This becomes necessary with use of CPU relocations and lazy GTT binding,
but could potentially happen anyway as a result of a pathological error.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
---

We continued discussing how best to handle this in the light of the
desirability of unmappable command buffers and decided that a CPU
fallback path was necessary to maintain the utility of the
i915_error_state and a prerequisite for LLC relocations.
-Chris

---
 drivers/gpu/drm/i915/i915_irq.c |   28 +++++++++++++++++++++++-----
 1 files changed, 23 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index b40004b..08877a7 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -720,7 +720,6 @@ i915_error_object_create(struct drm_i915_private *dev_priv,
 	reloc_offset = src->gtt_offset;
 	for (page = 0; page < page_count; page++) {
 		unsigned long flags;
-		void __iomem *s;
 		void *d;
 
 		d = kmalloc(PAGE_SIZE, GFP_ATOMIC);
@@ -728,10 +727,29 @@ i915_error_object_create(struct drm_i915_private *dev_priv,
 			goto unwind;
 
 		local_irq_save(flags);
-		s = io_mapping_map_atomic_wc(dev_priv->mm.gtt_mapping,
-					     reloc_offset);
-		memcpy_fromio(d, s, PAGE_SIZE);
-		io_mapping_unmap_atomic(s);
+		if (reloc_offset < dev_priv->mm.gtt_mappable_end) {
+			void __iomem *s;
+
+			/* Simply ignore tiling or any overlapping fence.
+			 * It's part of the error state, and this hopefully
+			 * captures what the GPU read.
+			 */
+
+			s = io_mapping_map_atomic_wc(dev_priv->mm.gtt_mapping,
+						     reloc_offset);
+			memcpy_fromio(d, s, PAGE_SIZE);
+			io_mapping_unmap_atomic(s);
+		} else {
+			void *s;
+
+			drm_clflush_pages(&src->pages[page], 1);
+
+			s = kmap_atomic(src->pages[page]);
+			memcpy(d, s, PAGE_SIZE);
+			kunmap_atomic(s);
+
+			drm_clflush_pages(&src->pages[page], 1);
+		}
 		local_irq_restore(flags);
 
 		dst->pages[page] = d;
-- 
1.7.7.3

Re: [PATCH] drm/i915: Handle unmappable buffers during error state capture

[Daniel Vetter]

On Tue, Nov 29, 2011 at 07:02:06PM +0000, Chris Wilson wrote:
> As the buffer is not necessarily accessible through the GTT at the time
> of a GPU hang, and capturing some of its contents is far more valuable
> than skipping it, provide a clflushed fallback read path. We still
> prefer to read through the GTT as that is more consistent with the GPU
> access of the same buffer. So example it will demonstrate any errorneous
> tiling or swizzling of the command buffer as seen by the GPU.
> 
> This becomes necessary with use of CPU relocations and lazy GTT binding,
> but could potentially happen anyway as a result of a pathological error.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH] drm/i915: Handle unmappable buffers during error state capture

[Eugeni Dodonov]

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

On Tue, Nov 29, 2011 at 17:02, Chris Wilson <chris@chris-wilson.co.uk>wrote:

> As the buffer is not necessarily accessible through the GTT at the time
> of a GPU hang, and capturing some of its contents is far more valuable
> than skipping it, provide a clflushed fallback read path. We still
> prefer to read through the GTT as that is more consistent with the GPU
> access of the same buffer. So example it will demonstrate any errorneous
> tiling or swizzling of the command buffer as seen by the GPU.
>
> This becomes necessary with use of CPU relocations and lazy GTT binding,
> but could potentially happen anyway as a result of a pathological error.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>

Reviewed-by: Eugeni Dodonov <eugeni.dodonov@intel.com>

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

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