diff mbox

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

Message ID 1390892826-26973-3-git-send-email-benjamin.widawsky@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Ben Widawsky Jan. 28, 2014, 7:07 a.m. UTC 
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(-)

Comments

Chris Wilson Jan. 28, 2014, 11:37 a.m. UTC | #1 
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
diff mbox

Patch

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);
 }