[PATCH] drm/i915: Check that the plane points to the pipe's framebuffer before enabling

[PATCH] drm/i915: Check that the plane points to the pipe's framebuffer before enabling

[Chris Wilson]

Knut Petersen reported a GPU hang when he left x11perf running
overnight. The error state quite clearly indicates that plane A was
enabled without being fully setup:

PGTBL_ER: 0x00000010
    Display A: Invalid GTT PTE
Plane [0]:
  CNTR: c1000000
  STRIDE: 00000c80
  SIZE: 03ff04ff
  POS: 00000000
  ADDR: 00000000

[GTT offset on his system being pinned for the ringbuffer.]

This is a simple debugging patch to assert that this cannot be so!

References: https://bugs.freedesktop.org/show_bug.cgi?id=36246
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/gpu/drm/i915/intel_display.c |   25 +++++++++++++++++++++++++
 1 files changed, 25 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 7734d1e..82c22a3 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -1274,6 +1274,30 @@ static void intel_disable_pipe(struct drm_i915_private *dev_priv,
 	intel_wait_for_pipe_off(dev_priv->dev, pipe);
 }
 
+/* Check that the DSPADDR points to the right framebufffer for the pipe. */
+static void assert_fb_bound_to_plane(struct drm_i915_private *dev_priv,
+				     enum pipe pipe, enum plane *plane)
+{
+	struct drm_crtc *crtc;
+	struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb);
+	u32 val, base, size;
+
+	crtc = intel_get_crtc_for_pipe(dev, pipe);
+	if (WARN(crtc->fb == NULL,
+		 "no framebuffer attached to pipe %c\n",
+		 pipe_name(pipe)))
+		return;
+
+	intel_fb = to_intel_framebuffer(crtc->fb);
+	base = intel_fb->obj->gtt_offset;
+	size = intel_fb->obj->base.size;
+
+	val = I915_READ(DSPADDR(plane));
+	WARN(val < offset || val >= base + size,
+	     "mismatching framebuffer for plane %c attached to pipe %c\n",
+	     plane_name(plane), pipe_name(pipe));
+}
+
 /**
  * intel_enable_plane - enable a display plane on a given pipe
  * @dev_priv: i915 private structure
@@ -1290,6 +1314,7 @@ static void intel_enable_plane(struct drm_i915_private *dev_priv,
 
 	/* If the pipe isn't enabled, we can't pump pixels and may hang */
 	assert_pipe_enabled(dev_priv, pipe);
+	assert_fb_bound_to_plane(dev_priv, pipe, plane);
 
 	reg = DSPCNTR(plane);
 	val = I915_READ(reg);
-- 
1.7.4.1

[PATCH] drm/i915: Check that the plane points to the pipe's framebuffer before enabling

[Chris Wilson]

Knut Petersen reported a GPU hang when he left x11perf running
overnight. The error state quite clearly indicates that plane A was
enabled without being fully setup:

PGTBL_ER: 0x00000010
    Display A: Invalid GTT PTE
Plane [0]:
  CNTR: c1000000
  STRIDE: 00000c80
  SIZE: 03ff04ff
  POS: 00000000
  ADDR: 00000000

[That GTT offset on his system being pinned for the ringbuffer.]

This is a simple debugging patch to assert that this cannot be so!

References: https://bugs.freedesktop.org/show_bug.cgi?id=36246
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
---

v2: Remember that gen4 splits the base + offset between different regs...

---
 drivers/gpu/drm/i915/intel_display.c |   29 +++++++++++++++++++++++++++++
 1 files changed, 29 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 7734d1e..ab80046 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -1274,6 +1274,34 @@ static void intel_disable_pipe(struct drm_i915_private *dev_priv,
 	intel_wait_for_pipe_off(dev_priv->dev, pipe);
 }
 
+/* Check that the DSPADDR points to the right framebufffer for the pipe. */
+static void assert_fb_bound_to_plane(struct drm_i915_private *dev_priv,
+				     enum pipe pipe, enum plane plane)
+{
+	struct drm_crtc *crtc;
+	struct intel_framebuffer *intel_fb;
+	u32 val, base, size;
+
+	crtc = intel_get_crtc_for_pipe(dev_priv->dev, pipe);
+	if (WARN(crtc->fb == NULL,
+		 "no framebuffer attached to pipe %c\n",
+		 pipe_name(pipe)))
+		return;
+
+	intel_fb = to_intel_framebuffer(crtc->fb);
+	base = intel_fb->obj->gtt_offset;
+	size = intel_fb->obj->base.size;
+
+	if (dev_priv->info->gen >= 4)
+		val = I915_READ(DSPSURF(plane));
+	else
+		val = I915_READ(DSPADDR(plane));
+	WARN(val < base || val >= base + size,
+	     "mismatching framebuffer for plane %c attached to pipe %c, expected %x-%x found %x\n",
+	     plane_name(plane), pipe_name(pipe),
+	     base, base + size, val);
+}
+
 /**
  * intel_enable_plane - enable a display plane on a given pipe
  * @dev_priv: i915 private structure
@@ -1290,6 +1318,7 @@ static void intel_enable_plane(struct drm_i915_private *dev_priv,
 
 	/* If the pipe isn't enabled, we can't pump pixels and may hang */
 	assert_pipe_enabled(dev_priv, pipe);
+	assert_fb_bound_to_plane(dev_priv, pipe, plane);
 
 	reg = DSPCNTR(plane);
 	val = I915_READ(reg);
-- 
1.7.4.4

Re: [PATCH] drm/i915: Check that the plane points to the pipe's framebuffer before enabling

[Jesse Barnes]

On Sun, 17 Apr 2011 10:32:41 +0100
Chris Wilson <chris@chris-wilson.co.uk> wrote:

> Knut Petersen reported a GPU hang when he left x11perf running
> overnight. The error state quite clearly indicates that plane A was
> enabled without being fully setup:
> 
> PGTBL_ER: 0x00000010
>     Display A: Invalid GTT PTE
> Plane [0]:
>   CNTR: c1000000
>   STRIDE: 00000c80
>   SIZE: 03ff04ff
>   POS: 00000000
>   ADDR: 00000000
> 
> [That GTT offset on his system being pinned for the ringbuffer.]
> 
> This is a simple debugging patch to assert that this cannot be so!
> 

I like it (though now the comment talks about DSPADDR and leaves
DSPSURF out in the cold).

Sounds like our dpms code may be causing trouble for this x11perf run
perhaps?  What else would cause us to change base addrs during a bench
run like that?

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH] drm/i915: Check that the plane points to the pipes framebuffer before enabling

[Chris Wilson]

On Mon, 18 Apr 2011 08:54:44 -0700, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> I like it (though now the comment talks about DSPADDR and leaves
> DSPSURF out in the cold).

D'oh. That'll teach me to try and write a comment to explain a function
first!

> Sounds like our dpms code may be causing trouble for this x11perf run
> perhaps?  What else would cause us to change base addrs during a bench
> run like that?

Right. The early observation is that we don't assign a framebuffer to
the CRTC borrowed for VGA/TV load detection. And I can postulate that we
contrived to access a PTE during a hotplug poll whilst rewriting the GTT
(or something similar to the object following the ringbuffer).

I think we can just attach the fbcon for the purposes of load detection.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre