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

From: Chris Wilson <chris@chris-wilson.co.uk>
To: Ben Widawsky <benjamin.widawsky@intel.com>
Cc: Intel GFX <intel-gfx@lists.freedesktop.org>,
	Ben Widawsky <ben@bwidawsk.net>
Subject: Re: [PATCH 3/7] drm/i915: Logically reorder error register capture
Date: Tue, 28 Jan 2014 11:37:57 +0000	[thread overview]
Message-ID: <20140128113757.GJ2508@nuc-i3427.alporthouse.com> (raw)
In-Reply-To: <1390892826-26973-3-git-send-email-benjamin.widawsky@intel.com>

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