[PATCH 1/7] [v2] drm/i915: Create a USES_PPGTT macro

[PATCH 1/7] [v2] drm/i915: Create a USES_PPGTT macro

[Ben Widawsky]

There are cases where we want to know if there is a full, or aliased
PPGTT. Currently, in fact the only distinction we ever need to make is
when we're using full PPGTT.

This patch is simply to promote readability and clarify for the
confusing existing usage where "aliasing" meant aliasing and full.

v2: Remove USES_ALIASING_PPGTT since there are currently no cases where
we need to check if we're using aliasing, but not full PPGTT. (Daniel)

Cc: Daniel Vetter <daniel@ffwll.ch>
Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_drv.h         | 5 +++--
 drivers/gpu/drm/i915/i915_gem_context.c | 6 +++---
 2 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 4a86d56..d08064e 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1839,8 +1839,9 @@ struct drm_i915_file_private {
 
 #define HAS_HW_CONTEXTS(dev)	(INTEL_INFO(dev)->gen >= 6)
 #define HAS_ALIASING_PPGTT(dev)	(INTEL_INFO(dev)->gen >= 6 && !IS_VALLEYVIEW(dev))
-#define HAS_PPGTT(dev)		(INTEL_INFO(dev)->gen >= 7 && !IS_VALLEYVIEW(dev) && !IS_BROADWELL(dev))
-#define USES_ALIASING_PPGTT(dev) intel_enable_ppgtt(dev, false)
+#define HAS_PPGTT(dev)		(INTEL_INFO(dev)->gen >= 7 && !IS_VALLEYVIEW(dev) \
+				 && !IS_BROADWELL(dev))
+#define USES_PPGTT(dev)		intel_enable_ppgtt(dev, false)
 #define USES_FULL_PPGTT(dev)	intel_enable_ppgtt(dev, true)
 
 #define HAS_OVERLAY(dev)		(INTEL_INFO(dev)->has_overlay)
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index 1c94082..5d4183f 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -142,7 +142,7 @@ void i915_gem_context_free(struct kref *ctx_ref)
 	struct i915_hw_ppgtt *ppgtt = NULL;
 
 	/* We refcount even the aliasing PPGTT to keep the code symmetric */
-	if (USES_ALIASING_PPGTT(ctx->obj->base.dev))
+	if (USES_PPGTT(ctx->obj->base.dev))
 		ppgtt = ctx_to_ppgtt(ctx);
 
 	/* XXX: Free up the object before tearing down the address space, in
@@ -291,7 +291,7 @@ i915_gem_create_context(struct drm_device *dev,
 
 			dev_priv->mm.aliasing_ppgtt = ppgtt;
 		}
-	} else if (USES_ALIASING_PPGTT(dev)) {
+	} else if (USES_PPGTT(dev)) {
 		/* For platforms which only have aliasing PPGTT, we fake the
 		 * address space and refcounting. */
 		ctx->vm = &dev_priv->mm.aliasing_ppgtt->base;
@@ -373,7 +373,7 @@ int i915_gem_context_init(struct drm_device *dev)
 	}
 
 	dev_priv->ring[RCS].default_context =
-		i915_gem_create_context(dev, NULL, USES_ALIASING_PPGTT(dev));
+		i915_gem_create_context(dev, NULL, USES_PPGTT(dev));
 
 	if (IS_ERR_OR_NULL(dev_priv->ring[RCS].default_context)) {
 		DRM_DEBUG_DRIVER("Disabling HW Contexts; create failed %ld\n",
-- 
1.8.5.3

[PATCH 2/7] drm/i915: Extract register state error capture

[Ben Widawsky]

The code has become quite hairy. By relocating all the generic registers
it will become more obvious where future ones should go. There is still
admittedly a bit of confusion left for things like per ring registers.

A subsequent patch will clean this function up.

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_gpu_error.c | 77 +++++++++++++++++++----------------
 1 file changed, 43 insertions(+), 34 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
index 21cf0cf..67c82e5 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.c
+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
@@ -1012,43 +1012,13 @@ static void i915_gem_capture_buffers(struct drm_i915_private *dev_priv,
 	}
 }
 
-/**
- * i915_capture_error_state - capture an error record for later analysis
- * @dev: drm device
- *
- * Should be called when an error is detected (either a hang or an error
- * interrupt) to capture error state from the time of the error.  Fills
- * out a structure which becomes available in debugfs for user level tools
- * to pick up.
- */
-void i915_capture_error_state(struct drm_device *dev)
+/* Capture all registers which don't fit into another category. */
+static void i915_capture_reg_state(struct drm_i915_private *dev_priv,
+				   struct drm_i915_error_state *error)
 {
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct drm_i915_error_state *error;
-	unsigned long flags;
+	struct drm_device *dev = dev_priv->dev;
 	int pipe;
 
-	spin_lock_irqsave(&dev_priv->gpu_error.lock, flags);
-	error = dev_priv->gpu_error.first_error;
-	spin_unlock_irqrestore(&dev_priv->gpu_error.lock, flags);
-	if (error)
-		return;
-
-	/* Account for pipe specific data like PIPE*STAT */
-	error = kzalloc(sizeof(*error), GFP_ATOMIC);
-	if (!error) {
-		DRM_DEBUG_DRIVER("out of memory, not capturing error state\n");
-		return;
-	}
-
-	DRM_INFO("GPU crash dump saved to /sys/class/drm/card%d/error\n",
-		 dev->primary->index);
-	DRM_INFO("GPU hangs can indicate a bug anywhere in the entire gfx stack, including userspace.\n");
-	DRM_INFO("Please file a _new_ bug report on bugs.freedesktop.org against DRI -> DRM/Intel\n");
-	DRM_INFO("drm/i915 developers can then reassign to the right component if it's not a kernel issue.\n");
-	DRM_INFO("The gpu crash dump is required to analyze gpu hangs, so please always attach it.\n");
-
-	kref_init(&error->ref);
 	error->eir = I915_READ(EIR);
 	error->pgtbl_er = I915_READ(PGTBL_ER);
 	if (HAS_HW_CONTEXTS(dev))
@@ -1086,7 +1056,46 @@ void i915_capture_error_state(struct drm_device *dev)
 		error->err_int = I915_READ(GEN7_ERR_INT);
 
 	i915_get_extra_instdone(dev, error->extra_instdone);
+}
+
+/**
+ * i915_capture_error_state - capture an error record for later analysis
+ * @dev: drm device
+ *
+ * Should be called when an error is detected (either a hang or an error
+ * interrupt) to capture error state from the time of the error.  Fills
+ * out a structure which becomes available in debugfs for user level tools
+ * to pick up.
+ */
+void i915_capture_error_state(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct drm_i915_error_state *error;
+	unsigned long flags;
+
+	spin_lock_irqsave(&dev_priv->gpu_error.lock, flags);
+	error = dev_priv->gpu_error.first_error;
+	spin_unlock_irqrestore(&dev_priv->gpu_error.lock, flags);
+	if (error)
+		return;
+
+	/* Account for pipe specific data like PIPE*STAT */
+	error = kzalloc(sizeof(*error), GFP_ATOMIC);
+	if (!error) {
+		DRM_DEBUG_DRIVER("out of memory, not capturing error state\n");
+		return;
+	}
+
+	DRM_INFO("GPU crash dump saved to /sys/class/drm/card%d/error\n",
+		 dev->primary->index);
+	DRM_INFO("GPU hangs can indicate a bug anywhere in the entire gfx stack, including userspace.\n");
+	DRM_INFO("Please file a _new_ bug report on bugs.freedesktop.org against DRI -> DRM/Intel\n");
+	DRM_INFO("drm/i915 developers can then reassign to the right component if it's not a kernel issue.\n");
+	DRM_INFO("The gpu crash dump is required to analyze gpu hangs, so please always attach it.\n");
+
+	kref_init(&error->ref);
 
+	i915_capture_reg_state(dev_priv, error);
 	i915_gem_capture_buffers(dev_priv, error);
 	i915_gem_record_fences(dev, error);
 	i915_gem_record_rings(dev, error);
-- 
1.8.5.3

Re: [PATCH 2/7] drm/i915: Extract register state error capture

[Chris Wilson]

On Mon, Jan 27, 2014 at 11:07:01PM -0800, Ben Widawsky wrote:
> The code has become quite hairy. By relocating all the generic registers
> it will become more obvious where future ones should go. There is still
> admittedly a bit of confusion left for things like per ring registers.
> 
> A subsequent patch will clean this function up.
> 
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH 3/7] drm/i915: Logically reorder error register capture

[Ben Widawsky]

Create logical sections in an attempt to clean up, and continue to keep
future additions clean.

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_gpu_error.c | 60 +++++++++++++++++++++--------------
 1 file changed, 36 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
index 67c82e5..7ded9c2 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.c
+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
@@ -1019,41 +1019,53 @@ static void i915_capture_reg_state(struct drm_i915_private *dev_priv,
 	struct drm_device *dev = dev_priv->dev;
 	int pipe;
 
-	error->eir = I915_READ(EIR);
-	error->pgtbl_er = I915_READ(PGTBL_ER);
-	if (HAS_HW_CONTEXTS(dev))
-		error->ccid = I915_READ(CCID);
-
-	if (HAS_PCH_SPLIT(dev))
-		error->ier = I915_READ(DEIER) | I915_READ(GTIER);
-	else if (IS_VALLEYVIEW(dev))
+	/* General organization
+	 * 1. GEN specific registers
+	 * 2. >= GEN specific registers
+	 * 3. Feature specific registers.
+	 * 4. Everything else
+	 * Please try to follow the order.
+	 *
+	 * 1: */
+	if (IS_VALLEYVIEW(dev)) {
 		error->ier = I915_READ(GTIER) | I915_READ(VLV_IER);
-	else if (IS_GEN2(dev))
-		error->ier = I915_READ16(IER);
-	else
-		error->ier = I915_READ(IER);
+		error->forcewake = I915_READ(FORCEWAKE_VLV);
+	}
 
-	if (INTEL_INFO(dev)->gen >= 6)
-		error->derrmr = I915_READ(DERRMR);
+	if (IS_GEN7(dev))
+		error->err_int = I915_READ(GEN7_ERR_INT);
 
-	if (IS_VALLEYVIEW(dev))
-		error->forcewake = I915_READ(FORCEWAKE_VLV);
-	else if (INTEL_INFO(dev)->gen >= 7)
-		error->forcewake = I915_READ(FORCEWAKE_MT);
-	else if (INTEL_INFO(dev)->gen == 6)
+	if (IS_GEN6(dev))
 		error->forcewake = I915_READ(FORCEWAKE);
 
-	if (!HAS_PCH_SPLIT(dev))
-		for_each_pipe(pipe)
-			error->pipestat[pipe] = I915_READ(PIPESTAT(pipe));
+	if (IS_GEN2(dev))
+		error->ier = I915_READ16(IER);
+
+	/* 2: */
+	if (INTEL_INFO(dev)->gen >= 7)
+		error->forcewake = I915_READ(FORCEWAKE_MT);
 
 	if (INTEL_INFO(dev)->gen >= 6) {
+		error->derrmr = I915_READ(DERRMR);
 		error->error = I915_READ(ERROR_GEN6);
 		error->done_reg = I915_READ(DONE_REG);
 	}
 
-	if (INTEL_INFO(dev)->gen == 7)
-		error->err_int = I915_READ(GEN7_ERR_INT);
+	/* 3: */
+	if (HAS_HW_CONTEXTS(dev))
+		error->ccid = I915_READ(CCID);
+
+	if (HAS_PCH_SPLIT(dev))
+		error->ier = I915_READ(DEIER) | I915_READ(GTIER);
+	else {
+		error->ier = I915_READ(IER);
+		for_each_pipe(pipe)
+			error->pipestat[pipe] = I915_READ(PIPESTAT(pipe));
+	}
+
+	/* 4: */
+	error->eir = I915_READ(EIR);
+	error->pgtbl_er = I915_READ(PGTBL_ER);
 
 	i915_get_extra_instdone(dev, error->extra_instdone);
 }
-- 
1.8.5.3

Re: [PATCH 3/7] drm/i915: Logically reorder error register capture

[Chris Wilson]

On Mon, Jan 27, 2014 at 11:07:02PM -0800, Ben Widawsky wrote:
> Create logical sections in an attempt to clean up, and continue to keep
> future additions clean.
> 
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> ---
>  drivers/gpu/drm/i915/i915_gpu_error.c | 60 +++++++++++++++++++++--------------
>  1 file changed, 36 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
> index 67c82e5..7ded9c2 100644
> --- a/drivers/gpu/drm/i915/i915_gpu_error.c
> +++ b/drivers/gpu/drm/i915/i915_gpu_error.c
> @@ -1019,41 +1019,53 @@ static void i915_capture_reg_state(struct drm_i915_private *dev_priv,
>  	struct drm_device *dev = dev_priv->dev;
>  	int pipe;
>  
> -	error->eir = I915_READ(EIR);
> -	error->pgtbl_er = I915_READ(PGTBL_ER);
> -	if (HAS_HW_CONTEXTS(dev))
> -		error->ccid = I915_READ(CCID);
> -
> -	if (HAS_PCH_SPLIT(dev))
> -		error->ier = I915_READ(DEIER) | I915_READ(GTIER);
> -	else if (IS_VALLEYVIEW(dev))
> +	/* General organization
> +	 * 1. GEN specific registers
1. Registers specific to a single generation
2. Registers specific to multiple generations

Does that come out neater than chronological ordering?
Yes, it probably does thanks to IS_VALLEYVIEW etc.

> +	 * 2. >= GEN specific registers
> +	 * 3. Feature specific registers.
> +	 * 4. Everything else
> +	 * Please try to follow the order.
*/

I would split these into two (and I would repeat the section headings as
well as the section numbers).
> +	 *

/*
> +	 * 1: */

Otherwise, it lgtm.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH 4/7] drm/i915: Reorder struct members

[Ben Widawsky]

This helps make an upcoming patch a bit more reviewable

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_drv.h | 43 ++++++++++++++++++++++++-----------------
 1 file changed, 25 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index d08064e..bb53de5 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -295,14 +295,26 @@ struct intel_display_error_state;
 
 struct drm_i915_error_state {
 	struct kref ref;
+	struct timeval time;
+
+	/* Generic register state */
 	u32 eir;
 	u32 pgtbl_er;
 	u32 ier;
 	u32 ccid;
 	u32 derrmr;
 	u32 forcewake;
-	bool waiting[I915_NUM_RINGS];
+	u32 error; /* gen6+ */
+	u32 err_int; /* gen7 */
+	u32 done_reg;
+	u32 extra_instdone[I915_NUM_INSTDONE_REG];
 	u32 pipestat[I915_MAX_PIPES];
+	u64 fence[I915_MAX_NUM_FENCES];
+	struct intel_overlay_error_state *overlay;
+	struct intel_display_error_state *display;
+
+	/* Per ring register state
+	 * TODO: Move these to per ring */
 	u32 tail[I915_NUM_RINGS];
 	u32 head[I915_NUM_RINGS];
 	u32 ctl[I915_NUM_RINGS];
@@ -311,25 +323,25 @@ struct drm_i915_error_state {
 	u32 ipehr[I915_NUM_RINGS];
 	u32 instdone[I915_NUM_RINGS];
 	u32 acthd[I915_NUM_RINGS];
-	u32 semaphore_mboxes[I915_NUM_RINGS][I915_NUM_RINGS - 1];
-	u32 semaphore_seqno[I915_NUM_RINGS][I915_NUM_RINGS - 1];
-	u32 rc_psmi[I915_NUM_RINGS]; /* sleep state */
-	/* our own tracking of ring head and tail */
-	u32 cpu_ring_head[I915_NUM_RINGS];
-	u32 cpu_ring_tail[I915_NUM_RINGS];
-	u32 error; /* gen6+ */
-	u32 err_int; /* gen7 */
 	u32 bbstate[I915_NUM_RINGS];
 	u32 instpm[I915_NUM_RINGS];
 	u32 instps[I915_NUM_RINGS];
-	u32 extra_instdone[I915_NUM_INSTDONE_REG];
 	u32 seqno[I915_NUM_RINGS];
 	u64 bbaddr[I915_NUM_RINGS];
 	u32 fault_reg[I915_NUM_RINGS];
-	u32 done_reg;
 	u32 faddr[I915_NUM_RINGS];
-	u64 fence[I915_MAX_NUM_FENCES];
-	struct timeval time;
+	u32 rc_psmi[I915_NUM_RINGS]; /* sleep state */
+	u32 semaphore_mboxes[I915_NUM_RINGS][I915_NUM_RINGS - 1];
+
+	/* Software tracked state */
+	bool waiting[I915_NUM_RINGS];
+	int hangcheck_score[I915_NUM_RINGS];
+	enum intel_ring_hangcheck_action hangcheck_action[I915_NUM_RINGS];
+
+	/* our own tracking of ring head and tail */
+	u32 cpu_ring_head[I915_NUM_RINGS];
+	u32 cpu_ring_tail[I915_NUM_RINGS];
+	u32 semaphore_seqno[I915_NUM_RINGS][I915_NUM_RINGS - 1];
 
 	struct drm_i915_error_ring {
 		bool valid;
@@ -363,11 +375,6 @@ struct drm_i915_error_state {
 	} **active_bo, **pinned_bo;
 	u32 *active_bo_count, *pinned_bo_count;
 	u32 vm_count;
-
-	struct intel_overlay_error_state *overlay;
-	struct intel_display_error_state *display;
-	int hangcheck_score[I915_NUM_RINGS];
-	enum intel_ring_hangcheck_action hangcheck_action[I915_NUM_RINGS];
 };
 
 struct intel_connector;
-- 
1.8.5.3

Re: [PATCH 4/7] drm/i915: Reorder struct members

[Chris Wilson]

On Mon, Jan 27, 2014 at 11:07:03PM -0800, Ben Widawsky wrote:
> This helps make an upcoming patch a bit more reviewable
> 
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>

Slightly less choatic. By itself this patch is just churn, but I think
it just about justifies itself in the next patch.

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH 5/7] drm/i915: Move per ring error state to ring_error

[Ben Widawsky]

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_drv.h       |  62 +++++++--------
 drivers/gpu/drm/i915/i915_gpu_error.c | 137 +++++++++++++++++-----------------
 2 files changed, 99 insertions(+), 100 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index bb53de5..defdb00 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -313,49 +313,49 @@ struct drm_i915_error_state {
 	struct intel_overlay_error_state *overlay;
 	struct intel_display_error_state *display;
 
-	/* Per ring register state
-	 * TODO: Move these to per ring */
-	u32 tail[I915_NUM_RINGS];
-	u32 head[I915_NUM_RINGS];
-	u32 ctl[I915_NUM_RINGS];
-	u32 hws[I915_NUM_RINGS];
-	u32 ipeir[I915_NUM_RINGS];
-	u32 ipehr[I915_NUM_RINGS];
-	u32 instdone[I915_NUM_RINGS];
-	u32 acthd[I915_NUM_RINGS];
-	u32 bbstate[I915_NUM_RINGS];
-	u32 instpm[I915_NUM_RINGS];
-	u32 instps[I915_NUM_RINGS];
-	u32 seqno[I915_NUM_RINGS];
-	u64 bbaddr[I915_NUM_RINGS];
-	u32 fault_reg[I915_NUM_RINGS];
-	u32 faddr[I915_NUM_RINGS];
-	u32 rc_psmi[I915_NUM_RINGS]; /* sleep state */
-	u32 semaphore_mboxes[I915_NUM_RINGS][I915_NUM_RINGS - 1];
-
-	/* Software tracked state */
-	bool waiting[I915_NUM_RINGS];
-	int hangcheck_score[I915_NUM_RINGS];
-	enum intel_ring_hangcheck_action hangcheck_action[I915_NUM_RINGS];
-
-	/* our own tracking of ring head and tail */
-	u32 cpu_ring_head[I915_NUM_RINGS];
-	u32 cpu_ring_tail[I915_NUM_RINGS];
-	u32 semaphore_seqno[I915_NUM_RINGS][I915_NUM_RINGS - 1];
-
 	struct drm_i915_error_ring {
 		bool valid;
+		/* Software tracked state */
+		bool waiting;
+		int hangcheck_score;
+		enum intel_ring_hangcheck_action hangcheck_action;
+
+		/* Register state */
+		u32 tail;
+		u32 head;
+		u32 ctl;
+		u32 ipeir;
+		u32 ipehr;
+		u32 instdone;
+		u32 acthd;
+		u32 bbstate;
+		u32 instpm;
+		u32 instps;
+		u32 seqno;
+		u64 bbaddr;
+		u32 fault_reg;
+		u32 faddr;
+		u32 rc_psmi; /* sleep state */
+		u32 semaphore_mboxes[I915_NUM_RINGS - 1];
+
+		/* our own tracking of ring head and tail */
+		u32 cpu_ring_head;
+		u32 cpu_ring_tail;
+
+		u32 semaphore_seqno[I915_NUM_RINGS - 1];
+
 		struct drm_i915_error_object {
 			int page_count;
 			u32 gtt_offset;
 			u32 *pages[0];
 		} *ringbuffer, *batchbuffer, *ctx, *hws;
+
+		int num_requests;
 		struct drm_i915_error_request {
 			long jiffies;
 			u32 seqno;
 			u32 tail;
 		} *requests;
-		int num_requests;
 	} ring[I915_NUM_RINGS];
 
 	struct drm_i915_error_buffer {
diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
index 7ded9c2..3f35896 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.c
+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
@@ -235,51 +235,48 @@ static const char *hangcheck_action_to_str(enum intel_ring_hangcheck_action a)
 
 static void i915_ring_error_state(struct drm_i915_error_state_buf *m,
 				  struct drm_device *dev,
-				  struct drm_i915_error_state *error,
-				  unsigned ring)
+				  struct drm_i915_error_ring *ring)
 {
-	BUG_ON(ring >= I915_NUM_RINGS); /* shut up confused gcc */
-	if (!error->ring[ring].valid)
+	if (!ring->valid)
 		return;
 
-	err_printf(m, "%s command stream:\n", ring_str(ring));
-	err_printf(m, "  HEAD: 0x%08x\n", error->head[ring]);
-	err_printf(m, "  TAIL: 0x%08x\n", error->tail[ring]);
-	err_printf(m, "  CTL: 0x%08x\n", error->ctl[ring]);
-	err_printf(m, "  HWS: 0x%08x\n", error->hws[ring]);
-	err_printf(m, "  ACTHD: 0x%08x\n", error->acthd[ring]);
-	err_printf(m, "  IPEIR: 0x%08x\n", error->ipeir[ring]);
-	err_printf(m, "  IPEHR: 0x%08x\n", error->ipehr[ring]);
-	err_printf(m, "  INSTDONE: 0x%08x\n", error->instdone[ring]);
+	err_printf(m, "  HEAD: 0x%08x\n", ring->head);
+	err_printf(m, "  TAIL: 0x%08x\n", ring->tail);
+	err_printf(m, "  CTL: 0x%08x\n", ring->ctl);
+	err_printf(m, "  HWS: 0x%08x\n", ring->hws);
+	err_printf(m, "  ACTHD: 0x%08x\n", ring->acthd);
+	err_printf(m, "  IPEIR: 0x%08x\n", ring->ipeir);
+	err_printf(m, "  IPEHR: 0x%08x\n", ring->ipehr);
+	err_printf(m, "  INSTDONE: 0x%08x\n", ring->instdone);
 	if (INTEL_INFO(dev)->gen >= 4) {
-		err_printf(m, "  BBADDR: 0x%08llx\n", error->bbaddr[ring]);
-		err_printf(m, "  BB_STATE: 0x%08x\n", error->bbstate[ring]);
-		err_printf(m, "  INSTPS: 0x%08x\n", error->instps[ring]);
+		err_printf(m, "  BBADDR: 0x%08llx\n", ring->bbaddr);
+		err_printf(m, "  BB_STATE: 0x%08x\n", ring->bbstate);
+		err_printf(m, "  INSTPS: 0x%08x\n", ring->instps);
 	}
-	err_printf(m, "  INSTPM: 0x%08x\n", error->instpm[ring]);
-	err_printf(m, "  FADDR: 0x%08x\n", error->faddr[ring]);
+	err_printf(m, "  INSTPM: 0x%08x\n", ring->instpm);
+	err_printf(m, "  FADDR: 0x%08x\n", ring->faddr);
 	if (INTEL_INFO(dev)->gen >= 6) {
-		err_printf(m, "  RC PSMI: 0x%08x\n", error->rc_psmi[ring]);
-		err_printf(m, "  FAULT_REG: 0x%08x\n", error->fault_reg[ring]);
+		err_printf(m, "  RC PSMI: 0x%08x\n", ring->rc_psmi);
+		err_printf(m, "  FAULT_REG: 0x%08x\n", ring->fault_reg);
 		err_printf(m, "  SYNC_0: 0x%08x [last synced 0x%08x]\n",
-			   error->semaphore_mboxes[ring][0],
-			   error->semaphore_seqno[ring][0]);
+			   ring->semaphore_mboxes[0],
+			   ring->semaphore_seqno[0]);
 		err_printf(m, "  SYNC_1: 0x%08x [last synced 0x%08x]\n",
-			   error->semaphore_mboxes[ring][1],
-			   error->semaphore_seqno[ring][1]);
+			   ring->semaphore_mboxes[1],
+			   ring->semaphore_seqno[1]);
 		if (HAS_VEBOX(dev)) {
 			err_printf(m, "  SYNC_2: 0x%08x [last synced 0x%08x]\n",
-				   error->semaphore_mboxes[ring][2],
-				   error->semaphore_seqno[ring][2]);
+				   ring->semaphore_mboxes[2],
+				   ring->semaphore_seqno[2]);
 		}
 	}
-	err_printf(m, "  seqno: 0x%08x\n", error->seqno[ring]);
-	err_printf(m, "  waiting: %s\n", yesno(error->waiting[ring]));
-	err_printf(m, "  ring->head: 0x%08x\n", error->cpu_ring_head[ring]);
-	err_printf(m, "  ring->tail: 0x%08x\n", error->cpu_ring_tail[ring]);
+	err_printf(m, "  seqno: 0x%08x\n", ring->seqno);
+	err_printf(m, "  waiting: %s\n", yesno(ring->waiting));
+	err_printf(m, "  ring->head: 0x%08x\n", ring->cpu_ring_head);
+	err_printf(m, "  ring->tail: 0x%08x\n", ring->cpu_ring_tail);
 	err_printf(m, "  hangcheck: %s [%d]\n",
-		   hangcheck_action_to_str(error->hangcheck_action[ring]),
-		   error->hangcheck_score[ring]);
+		   hangcheck_action_to_str(ring->hangcheck_action),
+		   ring->hangcheck_score);
 }
 
 void i915_error_printf(struct drm_i915_error_state_buf *e, const char *f, ...)
@@ -331,8 +328,10 @@ int i915_error_state_to_str(struct drm_i915_error_state_buf *m,
 	if (INTEL_INFO(dev)->gen == 7)
 		err_printf(m, "ERR_INT: 0x%08x\n", error->err_int);
 
-	for (i = 0; i < ARRAY_SIZE(error->ring); i++)
-		i915_ring_error_state(m, dev, error, i);
+	for (i = 0; i < ARRAY_SIZE(error->ring); i++) {
+		err_printf(m, "%s command stream:\n", ring_str(i));
+		i915_ring_error_state(m, dev, &error->ring[i]);
+	}
 
 	for (i = 0; i < error->vm_count; i++) {
 		err_printf(m, "vm[%d]\n", i);
@@ -767,52 +766,52 @@ i915_error_first_batchbuffer(struct drm_i915_private *dev_priv,
 }
 
 static void i915_record_ring_state(struct drm_device *dev,
-				   struct drm_i915_error_state *error,
-				   struct intel_ring_buffer *ring)
+				   struct intel_ring_buffer *ring,
+				   struct drm_i915_error_ring *ering)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
 	if (INTEL_INFO(dev)->gen >= 6) {
-		error->rc_psmi[ring->id] = I915_READ(ring->mmio_base + 0x50);
-		error->fault_reg[ring->id] = I915_READ(RING_FAULT_REG(ring));
-		error->semaphore_mboxes[ring->id][0]
+		ering->rc_psmi = I915_READ(ring->mmio_base + 0x50);
+		ering->fault_reg = I915_READ(RING_FAULT_REG(ring));
+		ering->semaphore_mboxes[0]
 			= I915_READ(RING_SYNC_0(ring->mmio_base));
-		error->semaphore_mboxes[ring->id][1]
+		ering->semaphore_mboxes[1]
 			= I915_READ(RING_SYNC_1(ring->mmio_base));
-		error->semaphore_seqno[ring->id][0] = ring->sync_seqno[0];
-		error->semaphore_seqno[ring->id][1] = ring->sync_seqno[1];
+		ering->semaphore_seqno[0] = ring->sync_seqno[0];
+		ering->semaphore_seqno[1] = ring->sync_seqno[1];
 	}
 
 	if (HAS_VEBOX(dev)) {
-		error->semaphore_mboxes[ring->id][2] =
+		ering->semaphore_mboxes[2] =
 			I915_READ(RING_SYNC_2(ring->mmio_base));
-		error->semaphore_seqno[ring->id][2] = ring->sync_seqno[2];
+		ering->semaphore_seqno[2] = ring->sync_seqno[2];
 	}
 
 	if (INTEL_INFO(dev)->gen >= 4) {
-		error->faddr[ring->id] = I915_READ(RING_DMA_FADD(ring->mmio_base));
-		error->ipeir[ring->id] = I915_READ(RING_IPEIR(ring->mmio_base));
-		error->ipehr[ring->id] = I915_READ(RING_IPEHR(ring->mmio_base));
-		error->instdone[ring->id] = I915_READ(RING_INSTDONE(ring->mmio_base));
-		error->instps[ring->id] = I915_READ(RING_INSTPS(ring->mmio_base));
-		error->bbaddr[ring->id] = I915_READ(RING_BBADDR(ring->mmio_base));
+		ering->faddr = I915_READ(RING_DMA_FADD(ring->mmio_base));
+		ering->ipeir = I915_READ(RING_IPEIR(ring->mmio_base));
+		ering->ipehr = I915_READ(RING_IPEHR(ring->mmio_base));
+		ering->instdone = I915_READ(RING_INSTDONE(ring->mmio_base));
+		ering->instps = I915_READ(RING_INSTPS(ring->mmio_base));
+		ering->bbaddr = I915_READ(RING_BBADDR(ring->mmio_base));
 		if (INTEL_INFO(dev)->gen >= 8)
-			error->bbaddr[ring->id] |= (u64) I915_READ(RING_BBADDR_UDW(ring->mmio_base)) << 32;
-		error->bbstate[ring->id] = I915_READ(RING_BBSTATE(ring->mmio_base));
+			ering->bbaddr |= (u64) I915_READ(RING_BBADDR_UDW(ring->mmio_base)) << 32;
+		ering->bbstate = I915_READ(RING_BBSTATE(ring->mmio_base));
 	} else {
-		error->faddr[ring->id] = I915_READ(DMA_FADD_I8XX);
-		error->ipeir[ring->id] = I915_READ(IPEIR);
-		error->ipehr[ring->id] = I915_READ(IPEHR);
-		error->instdone[ring->id] = I915_READ(INSTDONE);
+		ering->faddr = I915_READ(DMA_FADD_I8XX);
+		ering->ipeir = I915_READ(IPEIR);
+		ering->ipehr = I915_READ(IPEHR);
+		ering->instdone = I915_READ(INSTDONE);
 	}
 
-	error->waiting[ring->id] = waitqueue_active(&ring->irq_queue);
-	error->instpm[ring->id] = I915_READ(RING_INSTPM(ring->mmio_base));
-	error->seqno[ring->id] = ring->get_seqno(ring, false);
-	error->acthd[ring->id] = intel_ring_get_active_head(ring);
-	error->head[ring->id] = I915_READ_HEAD(ring);
-	error->tail[ring->id] = I915_READ_TAIL(ring);
-	error->ctl[ring->id] = I915_READ_CTL(ring);
+	ering->waiting = waitqueue_active(&ring->irq_queue);
+	ering->instpm = I915_READ(RING_INSTPM(ring->mmio_base));
+	ering->seqno = ring->get_seqno(ring, false);
+	ering->acthd = intel_ring_get_active_head(ring);
+	ering->head = I915_READ_HEAD(ring);
+	ering->tail = I915_READ_TAIL(ring);
+	ering->ctl = I915_READ_CTL(ring);
 
 	if (I915_NEED_GFX_HWS(dev)) {
 		int mmio;
@@ -840,14 +839,14 @@ static void i915_record_ring_state(struct drm_device *dev,
 			mmio = RING_HWS_PGA(ring->mmio_base);
 		}
 
-		error->hws[ring->id] = I915_READ(mmio);
+		ering->hws = I915_READ(mmio);
 	}
 
-	error->cpu_ring_head[ring->id] = ring->head;
-	error->cpu_ring_tail[ring->id] = ring->tail;
+	ering->cpu_ring_head = ring->head;
+	ering->cpu_ring_tail = ring->tail;
 
-	error->hangcheck_score[ring->id] = ring->hangcheck.score;
-	error->hangcheck_action[ring->id] = ring->hangcheck.action;
+	ering->hangcheck_score = ring->hangcheck.score;
+	ering->hangcheck_action = ring->hangcheck.action;
 }
 
 
@@ -888,7 +887,7 @@ static void i915_gem_record_rings(struct drm_device *dev,
 
 		error->ring[i].valid = true;
 
-		i915_record_ring_state(dev, error, ring);
+		i915_record_ring_state(dev, ring, &error->ring[i]);
 
 		error->ring[i].batchbuffer =
 			i915_error_first_batchbuffer(dev_priv, ring);
-- 
1.8.5.3

Re: [PATCH 5/7] drm/i915: Move per ring error state to ring_error

[Chris Wilson]

On Mon, Jan 27, 2014 at 11:07:04PM -0800, Ben Widawsky wrote:
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> ---
>  drivers/gpu/drm/i915/i915_drv.h       |  62 +++++++--------
>  drivers/gpu/drm/i915/i915_gpu_error.c | 137 +++++++++++++++++-----------------
>  2 files changed, 99 insertions(+), 100 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index bb53de5..defdb00 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -313,49 +313,49 @@ struct drm_i915_error_state {
>  	struct intel_overlay_error_state *overlay;
>  	struct intel_display_error_state *display;
>  
> -	/* Per ring register state
> -	 * TODO: Move these to per ring */
> -	u32 tail[I915_NUM_RINGS];
> -	u32 head[I915_NUM_RINGS];
> -	u32 ctl[I915_NUM_RINGS];
> -	u32 hws[I915_NUM_RINGS];
> -	u32 ipeir[I915_NUM_RINGS];
> -	u32 ipehr[I915_NUM_RINGS];
> -	u32 instdone[I915_NUM_RINGS];
> -	u32 acthd[I915_NUM_RINGS];
> -	u32 bbstate[I915_NUM_RINGS];
> -	u32 instpm[I915_NUM_RINGS];
> -	u32 instps[I915_NUM_RINGS];
> -	u32 seqno[I915_NUM_RINGS];
> -	u64 bbaddr[I915_NUM_RINGS];
> -	u32 fault_reg[I915_NUM_RINGS];
> -	u32 faddr[I915_NUM_RINGS];
> -	u32 rc_psmi[I915_NUM_RINGS]; /* sleep state */
> -	u32 semaphore_mboxes[I915_NUM_RINGS][I915_NUM_RINGS - 1];
> -
> -	/* Software tracked state */
> -	bool waiting[I915_NUM_RINGS];
> -	int hangcheck_score[I915_NUM_RINGS];
> -	enum intel_ring_hangcheck_action hangcheck_action[I915_NUM_RINGS];
> -
> -	/* our own tracking of ring head and tail */
> -	u32 cpu_ring_head[I915_NUM_RINGS];
> -	u32 cpu_ring_tail[I915_NUM_RINGS];
> -	u32 semaphore_seqno[I915_NUM_RINGS][I915_NUM_RINGS - 1];
> -
>  	struct drm_i915_error_ring {
>  		bool valid;
> +		/* Software tracked state */
> +		bool waiting;

Looks like we have some bools to coallesce! :)

> +		int hangcheck_score;
> +		enum intel_ring_hangcheck_action hangcheck_action;
> +
> +		/* Register state */
> +		u32 tail;
> +		u32 head;
> +		u32 ctl;
> +		u32 ipeir;
> +		u32 ipehr;
> +		u32 instdone;
> +		u32 acthd;
> +		u32 bbstate;
> +		u32 instpm;
> +		u32 instps;
> +		u32 seqno;
> +		u64 bbaddr;
> +		u32 fault_reg;
> +		u32 faddr;
> +		u32 rc_psmi; /* sleep state */
> +		u32 semaphore_mboxes[I915_NUM_RINGS - 1];
> +
> +		/* our own tracking of ring head and tail */
> +		u32 cpu_ring_head;
> +		u32 cpu_ring_tail;
> +
> +		u32 semaphore_seqno[I915_NUM_RINGS - 1];
> +
>  		struct drm_i915_error_object {
>  			int page_count;
>  			u32 gtt_offset;
>  			u32 *pages[0];
>  		} *ringbuffer, *batchbuffer, *ctx, *hws;
> +
> +		int num_requests;

This feels a little odd - we split up a set of pointers with a int+hole.

>  		struct drm_i915_error_request {
>  			long jiffies;
>  			u32 seqno;
>  			u32 tail;
>  		} *requests;
> -		int num_requests;
>  	} ring[I915_NUM_RINGS];
>  
>  	struct drm_i915_error_buffer {

The code cleanup is well worth it,
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH 6/7] drm/i915: Add some more registers to error state

[Ben Widawsky]

Chris:
Do we also want to capture?
  GAC_ECO_BITS /* gen6,7 */
  GAM_ECOCHK /* gen6,7 */
  GAB_CTL /* gen6 */
  GFX_MODE /* gen6 */

Requested-by: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_drv.h       |  4 ++++
 drivers/gpu/drm/i915/i915_gpu_error.c | 16 +++++++++++++---
 2 files changed, 17 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index defdb00..a144bc3 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -307,6 +307,10 @@ struct drm_i915_error_state {
 	u32 error; /* gen6+ */
 	u32 err_int; /* gen7 */
 	u32 done_reg;
+	u32 gac_eco;
+	u32 gam_ecochk;
+	u32 gab_ctl;
+	u32 gfx_mode;
 	u32 extra_instdone[I915_NUM_INSTDONE_REG];
 	u32 pipestat[I915_MAX_PIPES];
 	u64 fence[I915_MAX_NUM_FENCES];
diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
index 3f35896..2ef0566 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.c
+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
@@ -1021,8 +1021,9 @@ static void i915_capture_reg_state(struct drm_i915_private *dev_priv,
 	/* General organization
 	 * 1. GEN specific registers
 	 * 2. >= GEN specific registers
-	 * 3. Feature specific registers.
-	 * 4. Everything else
+	 * 3. GEN ranges
+	 * 4. Feature specific registers.
+	 * 5. Everything else
 	 * Please try to follow the order.
 	 *
 	 * 1: */
@@ -1034,8 +1035,11 @@ static void i915_capture_reg_state(struct drm_i915_private *dev_priv,
 	if (IS_GEN7(dev))
 		error->err_int = I915_READ(GEN7_ERR_INT);
 
-	if (IS_GEN6(dev))
+	if (IS_GEN6(dev)) {
 		error->forcewake = I915_READ(FORCEWAKE);
+		error->gab_ctl = I915_READ(GAB_CTL);
+		error->gfx_mode = I915_READ(GFX_MODE);
+	}
 
 	if (IS_GEN2(dev))
 		error->ier = I915_READ16(IER);
@@ -1051,6 +1055,12 @@ static void i915_capture_reg_state(struct drm_i915_private *dev_priv,
 	}
 
 	/* 3: */
+	if (IS_GEN6(dev) || IS_GEN7(dev)) {
+		error->gam_ecochk = I915_READ(GAM_ECOCHK);
+		error->gac_eco = I915_READ(GAC_ECO_BITS);
+	}
+
+	/* 4: */
 	if (HAS_HW_CONTEXTS(dev))
 		error->ccid = I915_READ(CCID);
 
-- 
1.8.5.3

Re: [PATCH 6/7] drm/i915: Add some more registers to error state

[Chris Wilson]

On Mon, Jan 27, 2014 at 11:07:05PM -0800, Ben Widawsky wrote:
> Chris:
> Do we also want to capture?
>   GAC_ECO_BITS /* gen6,7 */
>   GAM_ECOCHK /* gen6,7 */
>   GAB_CTL /* gen6 */
>   GFX_MODE /* gen6 */
> 
> Requested-by: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH 7/7] [v2] drm/i915: Capture PPGTT info on error capture

[Ben Widawsky]

v2: Rebased upon cleaned up error state

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_drv.h       |  9 ++++++++
 drivers/gpu/drm/i915/i915_gpu_error.c | 39 +++++++++++++++++++++++++++++++++++
 2 files changed, 48 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index a144bc3..1712ecc 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -360,6 +360,14 @@ struct drm_i915_error_state {
 			u32 seqno;
 			u32 tail;
 		} *requests;
+
+		struct {
+			u32 gfx_mode;
+			union {
+				u64 pdp[4];
+				u32 pp_dir_base;
+			};
+		} vm_info;
 	} ring[I915_NUM_RINGS];
 
 	struct drm_i915_error_buffer {
@@ -377,6 +385,7 @@ struct drm_i915_error_state {
 		s32 ring:4;
 		u32 cache_level:3;
 	} **active_bo, **pinned_bo;
+
 	u32 *active_bo_count, *pinned_bo_count;
 	u32 vm_count;
 };
diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
index 2ef0566..7fea86d 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.c
+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
@@ -277,6 +277,21 @@ static void i915_ring_error_state(struct drm_i915_error_state_buf *m,
 	err_printf(m, "  hangcheck: %s [%d]\n",
 		   hangcheck_action_to_str(ring->hangcheck_action),
 		   ring->hangcheck_score);
+
+	if (USES_PPGTT(dev)) {
+		err_printf(m, "  GFX_MODE: 0x%08x\n", ring->vm_info.gfx_mode);
+
+		if (INTEL_INFO(dev)->gen >= 8) {
+			int i;
+			for (i = 0; i < 4; i++)
+				err_printf(m, "  PDP%d: 0x%016llx\n",
+					   i, ring->vm_info.pdp[i]);
+		} else {
+			err_printf(m, "  PP_DIR_BASE: 0x%08x\n",
+				   ring->vm_info.pp_dir_base);
+		}
+	}
+
 }
 
 void i915_error_printf(struct drm_i915_error_state_buf *e, const char *f, ...)
@@ -847,6 +862,30 @@ static void i915_record_ring_state(struct drm_device *dev,
 
 	ering->hangcheck_score = ring->hangcheck.score;
 	ering->hangcheck_action = ring->hangcheck.action;
+
+	if (USES_PPGTT(dev)) {
+		int i;
+
+		ering->vm_info.gfx_mode = I915_READ(RING_MODE_GEN7(ring));
+
+		switch (INTEL_INFO(dev)->gen) {
+		case 8:
+			for (i = 0; i < 4; i++) {
+				ering->vm_info.pdp[i] =
+					I915_READ(GEN8_RING_PDP_UDW(ring, i));
+				ering->vm_info.pdp[i] <<= 32;
+				ering->vm_info.pdp[i] |=
+					I915_READ(GEN8_RING_PDP_LDW(ring, i));
+			}
+			break;
+		case 7:
+			ering->vm_info.pp_dir_base = RING_PP_DIR_BASE(ring);
+			break;
+		case 6:
+			ering->vm_info.pp_dir_base = RING_PP_DIR_BASE_READ(ring);
+			break;
+		}
+	}
 }
 
 
-- 
1.8.5.3

Re: [PATCH 7/7] [v2] drm/i915: Capture PPGTT info on error capture

[Chris Wilson]

On Mon, Jan 27, 2014 at 11:07:06PM -0800, Ben Widawsky wrote:
> v2: Rebased upon cleaned up error state
> 
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> ---
>  drivers/gpu/drm/i915/i915_drv.h       |  9 ++++++++
>  drivers/gpu/drm/i915/i915_gpu_error.c | 39 +++++++++++++++++++++++++++++++++++
>  2 files changed, 48 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index a144bc3..1712ecc 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -360,6 +360,14 @@ struct drm_i915_error_state {
>  			u32 seqno;
>  			u32 tail;
>  		} *requests;
> +
> +		struct {
> +			u32 gfx_mode;
> +			union {
> +				u64 pdp[4];
> +				u32 pp_dir_base;
> +			};
> +		} vm_info;
>  	} ring[I915_NUM_RINGS];
>  
>  	struct drm_i915_error_buffer {
> @@ -377,6 +385,7 @@ struct drm_i915_error_state {
>  		s32 ring:4;
>  		u32 cache_level:3;
>  	} **active_bo, **pinned_bo;
> +
>  	u32 *active_bo_count, *pinned_bo_count;
>  	u32 vm_count;
>  };
> diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
> index 2ef0566..7fea86d 100644
> --- a/drivers/gpu/drm/i915/i915_gpu_error.c
> +++ b/drivers/gpu/drm/i915/i915_gpu_error.c
> @@ -277,6 +277,21 @@ static void i915_ring_error_state(struct drm_i915_error_state_buf *m,
>  	err_printf(m, "  hangcheck: %s [%d]\n",
>  		   hangcheck_action_to_str(ring->hangcheck_action),
>  		   ring->hangcheck_score);
> +
> +	if (USES_PPGTT(dev)) {
> +		err_printf(m, "  GFX_MODE: 0x%08x\n", ring->vm_info.gfx_mode);
> +
> +		if (INTEL_INFO(dev)->gen >= 8) {
> +			int i;
> +			for (i = 0; i < 4; i++)
> +				err_printf(m, "  PDP%d: 0x%016llx\n",
> +					   i, ring->vm_info.pdp[i]);
> +		} else {
> +			err_printf(m, "  PP_DIR_BASE: 0x%08x\n",
> +				   ring->vm_info.pp_dir_base);
> +		}
> +	}
> +
Extra whitespace.

I think the order here is wrong - I want to keep the "hangcheck:
idle/wait/hung" as the last item in each ring block. It's one of the
values I like to cross-check and so having it in a consistent position
helps. Or we can put it first, perhaps inline with the ring name. Not
only thank, we have all the other registers first before the software
state.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 1/7] [v2] drm/i915: Create a USES_PPGTT macro

[Daniel Vetter]

On Mon, Jan 27, 2014 at 11:07:00PM -0800, Ben Widawsky wrote:
> There are cases where we want to know if there is a full, or aliased
> PPGTT. Currently, in fact the only distinction we ever need to make is
> when we're using full PPGTT.
> 
> This patch is simply to promote readability and clarify for the
> confusing existing usage where "aliasing" meant aliasing and full.
> 
> v2: Remove USES_ALIASING_PPGTT since there are currently no cases where
> we need to check if we're using aliasing, but not full PPGTT. (Daniel)
> 
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>

Queued for -next, thanks for the patch.
-Daniel

> ---
>  drivers/gpu/drm/i915/i915_drv.h         | 5 +++--
>  drivers/gpu/drm/i915/i915_gem_context.c | 6 +++---
>  2 files changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 4a86d56..d08064e 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1839,8 +1839,9 @@ struct drm_i915_file_private {
>  
>  #define HAS_HW_CONTEXTS(dev)	(INTEL_INFO(dev)->gen >= 6)
>  #define HAS_ALIASING_PPGTT(dev)	(INTEL_INFO(dev)->gen >= 6 && !IS_VALLEYVIEW(dev))
> -#define HAS_PPGTT(dev)		(INTEL_INFO(dev)->gen >= 7 && !IS_VALLEYVIEW(dev) && !IS_BROADWELL(dev))
> -#define USES_ALIASING_PPGTT(dev) intel_enable_ppgtt(dev, false)
> +#define HAS_PPGTT(dev)		(INTEL_INFO(dev)->gen >= 7 && !IS_VALLEYVIEW(dev) \
> +				 && !IS_BROADWELL(dev))
> +#define USES_PPGTT(dev)		intel_enable_ppgtt(dev, false)
>  #define USES_FULL_PPGTT(dev)	intel_enable_ppgtt(dev, true)
>  
>  #define HAS_OVERLAY(dev)		(INTEL_INFO(dev)->has_overlay)
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index 1c94082..5d4183f 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -142,7 +142,7 @@ void i915_gem_context_free(struct kref *ctx_ref)
>  	struct i915_hw_ppgtt *ppgtt = NULL;
>  
>  	/* We refcount even the aliasing PPGTT to keep the code symmetric */
> -	if (USES_ALIASING_PPGTT(ctx->obj->base.dev))
> +	if (USES_PPGTT(ctx->obj->base.dev))
>  		ppgtt = ctx_to_ppgtt(ctx);
>  
>  	/* XXX: Free up the object before tearing down the address space, in
> @@ -291,7 +291,7 @@ i915_gem_create_context(struct drm_device *dev,
>  
>  			dev_priv->mm.aliasing_ppgtt = ppgtt;
>  		}
> -	} else if (USES_ALIASING_PPGTT(dev)) {
> +	} else if (USES_PPGTT(dev)) {
>  		/* For platforms which only have aliasing PPGTT, we fake the
>  		 * address space and refcounting. */
>  		ctx->vm = &dev_priv->mm.aliasing_ppgtt->base;
> @@ -373,7 +373,7 @@ int i915_gem_context_init(struct drm_device *dev)
>  	}
>  
>  	dev_priv->ring[RCS].default_context =
> -		i915_gem_create_context(dev, NULL, USES_ALIASING_PPGTT(dev));
> +		i915_gem_create_context(dev, NULL, USES_PPGTT(dev));
>  
>  	if (IS_ERR_OR_NULL(dev_priv->ring[RCS].default_context)) {
>  		DRM_DEBUG_DRIVER("Disabling HW Contexts; create failed %ld\n",
> -- 
> 1.8.5.3
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch