[PATCH] drm/i915: Read the hardware state for the transcoder link upon error

[PATCH] drm/i915: Read the hardware state for the transcoder link upon error

[Chris Wilson]

Do not trust our bookkeeping when reporting errors, and instead dump the
register contents. In particular, this solves one particular issue when
an error is reported before we finish setting up the outputs and have a
complete mapping (i.e. during initialisation we set garbage state). If
an error occurs at that early stage, it is vital that we get an accurate
report of the hardware state and not conflated with our own inaccurate
opinions.

This fixes a panic for a large number of pre-Haswell machines that
currently trigger an error during KMS takeover.

Reported-by: Dustin King <daking@rescomp.stanford.edu>
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=60021
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: stable@vger.kernel.org
---
 drivers/gpu/drm/i915/intel_display.c | 37 ++++++++++++++++++++++++++++++++++--
 1 file changed, 35 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 5988bda..7ce4588 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -10273,12 +10273,43 @@ struct intel_display_error_state {
 	} plane[I915_MAX_PIPES];
 };
 
+static enum transcoder
+read_cpu_transcoder(struct drm_i915_private *dev_priv, int pipe)
+{
+	if (HAS_DDI(dev_priv->dev)) {
+		int edp_pipe;
+
+		switch (I915_READ(TRANS_DDI_FUNC_CTL(TRANSCODER_EDP)) & (TRANS_DDI_FUNC_ENABLE | TRANS_DDI_EDP_INPUT_MASK)) {
+		default:
+			edp_pipe = -1;
+			break;
+
+		case TRANS_DDI_EDP_INPUT_A_ONOFF | TRANS_DDI_FUNC_ENABLE:
+		case TRANS_DDI_EDP_INPUT_A_ON | TRANS_DDI_FUNC_ENABLE:
+			edp_pipe = PIPE_A;
+			break;
+
+		case TRANS_DDI_EDP_INPUT_B_ONOFF | TRANS_DDI_FUNC_ENABLE:
+			edp_pipe = PIPE_B;
+			break;
+
+		case TRANS_DDI_EDP_INPUT_C_ONOFF | TRANS_DDI_FUNC_ENABLE:
+			edp_pipe = PIPE_C;
+			break;
+		}
+
+		if (edp_pipe == pipe)
+			pipe = TRANSCODER_EDP;
+	}
+
+	return pipe;
+}
+
 struct intel_display_error_state *
 intel_display_capture_error_state(struct drm_device *dev)
 {
 	drm_i915_private_t *dev_priv = dev->dev_private;
 	struct intel_display_error_state *error;
-	enum transcoder cpu_transcoder;
 	int i;
 
 	error = kmalloc(sizeof(*error), GFP_ATOMIC);
@@ -10289,7 +10320,9 @@ intel_display_capture_error_state(struct drm_device *dev)
 		error->power_well_driver = I915_READ(HSW_PWR_WELL_DRIVER);
 
 	for_each_pipe(i) {
-		cpu_transcoder = intel_pipe_to_cpu_transcoder(dev_priv, i);
+		enum transcoder cpu_transcoder;
+
+		cpu_transcoder = read_cpu_transcoder(dev_priv, i);
 		error->pipe[i].cpu_transcoder = cpu_transcoder;
 
 		if (INTEL_INFO(dev)->gen <= 6 || IS_VALLEYVIEW(dev)) {
-- 
1.8.3.1

Re: [Intel-gfx] [PATCH] drm/i915: Read the hardware state for the transcoder link upon error

[Daniel Vetter]

On Fri, Jun 21, 2013 at 03:40:04PM +0100, Chris Wilson wrote:
> Do not trust our bookkeeping when reporting errors, and instead dump the
> register contents. In particular, this solves one particular issue when
> an error is reported before we finish setting up the outputs and have a
> complete mapping (i.e. during initialisation we set garbage state). If
> an error occurs at that early stage, it is vital that we get an accurate
> report of the hardware state and not conflated with our own inaccurate
> opinions.
> 
> This fixes a panic for a large number of pre-Haswell machines that
> currently trigger an error during KMS takeover.
> 
> Reported-by: Dustin King <daking@rescomp.stanford.edu>
> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=60021
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: stable@vger.kernel.org

Shouldn't we just dump all transcoder registers on Haswell instead of
potentially fragile dances trying to reconstruct state?
-Daniel

> ---
>  drivers/gpu/drm/i915/intel_display.c | 37 ++++++++++++++++++++++++++++++++++--
>  1 file changed, 35 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 5988bda..7ce4588 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -10273,12 +10273,43 @@ struct intel_display_error_state {
>  	} plane[I915_MAX_PIPES];
>  };
>  
> +static enum transcoder
> +read_cpu_transcoder(struct drm_i915_private *dev_priv, int pipe)
> +{
> +	if (HAS_DDI(dev_priv->dev)) {
> +		int edp_pipe;
> +
> +		switch (I915_READ(TRANS_DDI_FUNC_CTL(TRANSCODER_EDP)) & (TRANS_DDI_FUNC_ENABLE | TRANS_DDI_EDP_INPUT_MASK)) {
> +		default:
> +			edp_pipe = -1;
> +			break;
> +
> +		case TRANS_DDI_EDP_INPUT_A_ONOFF | TRANS_DDI_FUNC_ENABLE:
> +		case TRANS_DDI_EDP_INPUT_A_ON | TRANS_DDI_FUNC_ENABLE:
> +			edp_pipe = PIPE_A;
> +			break;
> +
> +		case TRANS_DDI_EDP_INPUT_B_ONOFF | TRANS_DDI_FUNC_ENABLE:
> +			edp_pipe = PIPE_B;
> +			break;
> +
> +		case TRANS_DDI_EDP_INPUT_C_ONOFF | TRANS_DDI_FUNC_ENABLE:
> +			edp_pipe = PIPE_C;
> +			break;
> +		}
> +
> +		if (edp_pipe == pipe)
> +			pipe = TRANSCODER_EDP;
> +	}
> +
> +	return pipe;
> +}
> +
>  struct intel_display_error_state *
>  intel_display_capture_error_state(struct drm_device *dev)
>  {
>  	drm_i915_private_t *dev_priv = dev->dev_private;
>  	struct intel_display_error_state *error;
> -	enum transcoder cpu_transcoder;
>  	int i;
>  
>  	error = kmalloc(sizeof(*error), GFP_ATOMIC);
> @@ -10289,7 +10320,9 @@ intel_display_capture_error_state(struct drm_device *dev)
>  		error->power_well_driver = I915_READ(HSW_PWR_WELL_DRIVER);
>  
>  	for_each_pipe(i) {
> -		cpu_transcoder = intel_pipe_to_cpu_transcoder(dev_priv, i);
> +		enum transcoder cpu_transcoder;
> +
> +		cpu_transcoder = read_cpu_transcoder(dev_priv, i);
>  		error->pipe[i].cpu_transcoder = cpu_transcoder;
>  
>  		if (INTEL_INFO(dev)->gen <= 6 || IS_VALLEYVIEW(dev)) {
> -- 
> 1.8.3.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [Intel-gfx] [PATCH] drm/i915: Read the hardware state for the transcoder link upon error

[Chris Wilson]

On Mon, Jun 24, 2013 at 09:59:33AM +0200, Daniel Vetter wrote:
> On Fri, Jun 21, 2013 at 03:40:04PM +0100, Chris Wilson wrote:
> > Do not trust our bookkeeping when reporting errors, and instead dump the
> > register contents. In particular, this solves one particular issue when
> > an error is reported before we finish setting up the outputs and have a
> > complete mapping (i.e. during initialisation we set garbage state). If
> > an error occurs at that early stage, it is vital that we get an accurate
> > report of the hardware state and not conflated with our own inaccurate
> > opinions.
> > 
> > This fixes a panic for a large number of pre-Haswell machines that
> > currently trigger an error during KMS takeover.
> > 
> > Reported-by: Dustin King <daking@rescomp.stanford.edu>
> > Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=60021
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: stable@vger.kernel.org
> 
> Shouldn't we just dump all transcoder registers on Haswell instead of
> potentially fragile dances trying to reconstruct state?

You still need to reconstruct the pipe->transcoder link as they are
separate banks of registers. This patch is just a smaller part of the
patch to dump everything.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH] drm/i915: Dump all transcoder registers on error

[Chris Wilson]

With Haswell the transcoders are a separate bank of registers from the
pipes (4 transcoders, 3 pipes). In event of an error, we want to be sure
we have a complete and accurate picture of the machine state, so record
all the transcoders in addition to all the active pipes.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_display.c | 93 +++++++++++++++++++++++++-----------
 1 file changed, 65 insertions(+), 28 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 7ce4588..fbc2daed 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -10242,6 +10242,9 @@ struct intel_display_error_state {
 
 	u32 power_well_driver;
 
+	int num_pipes;
+	int num_transcoders;
+
 	struct intel_cursor_error_state {
 		u32 control;
 		u32 position;
@@ -10251,15 +10254,7 @@ struct intel_display_error_state {
 
 	struct intel_pipe_error_state {
 		enum transcoder cpu_transcoder;
-		u32 conf;
 		u32 source;
-
-		u32 htotal;
-		u32 hblank;
-		u32 hsync;
-		u32 vtotal;
-		u32 vblank;
-		u32 vsync;
 	} pipe[I915_MAX_PIPES];
 
 	struct intel_plane_error_state {
@@ -10271,6 +10266,19 @@ struct intel_display_error_state {
 		u32 surface;
 		u32 tile_offset;
 	} plane[I915_MAX_PIPES];
+
+	struct intel_transcoder_error_state {
+		enum transcoder cpu_transcoder;
+
+		u32 conf;
+
+		u32 htotal;
+		u32 hblank;
+		u32 hsync;
+		u32 vtotal;
+		u32 vblank;
+		u32 vsync;
+	} transcoder[4];
 };
 
 static enum transcoder
@@ -10310,6 +10318,13 @@ intel_display_capture_error_state(struct drm_device *dev)
 {
 	drm_i915_private_t *dev_priv = dev->dev_private;
 	struct intel_display_error_state *error;
+	int transcoders[] = {
+		TRANSCODER_A,
+		TRANSCODER_B,
+		TRANSCODER_C,
+		TRANSCODER_EDP,
+		-1
+	};
 	int i;
 
 	error = kmalloc(sizeof(*error), GFP_ATOMIC);
@@ -10319,12 +10334,15 @@ intel_display_capture_error_state(struct drm_device *dev)
 	if (HAS_POWER_WELL(dev))
 		error->power_well_driver = I915_READ(HSW_PWR_WELL_DRIVER);
 
-	for_each_pipe(i) {
-		enum transcoder cpu_transcoder;
-
-		cpu_transcoder = read_cpu_transcoder(dev_priv, i);
-		error->pipe[i].cpu_transcoder = cpu_transcoder;
+	if (!HAS_DDI(dev_priv->dev))
+		transcoders[3] = -1; /* no EDP transcoder */
+	if (dev_priv->info->num_pipes < 3)
+		transcoders[2] = -1;
+	if (dev_priv->info->num_pipes < 2)
+		transcoders[1] = -1;
 
+	error->num_pipes = 0;
+	for_each_pipe(i) {
 		if (INTEL_INFO(dev)->gen <= 6 || IS_VALLEYVIEW(dev)) {
 			error->cursor[i].control = I915_READ(CURCNTR(i));
 			error->cursor[i].position = I915_READ(CURPOS(i));
@@ -10348,14 +10366,28 @@ intel_display_capture_error_state(struct drm_device *dev)
 			error->plane[i].tile_offset = I915_READ(DSPTILEOFF(i));
 		}
 
-		error->pipe[i].conf = I915_READ(PIPECONF(cpu_transcoder));
+		error->pipe[i].cpu_transcoder =
+			read_cpu_transcoder(dev_priv, i);
 		error->pipe[i].source = I915_READ(PIPESRC(i));
-		error->pipe[i].htotal = I915_READ(HTOTAL(cpu_transcoder));
-		error->pipe[i].hblank = I915_READ(HBLANK(cpu_transcoder));
-		error->pipe[i].hsync = I915_READ(HSYNC(cpu_transcoder));
-		error->pipe[i].vtotal = I915_READ(VTOTAL(cpu_transcoder));
-		error->pipe[i].vblank = I915_READ(VBLANK(cpu_transcoder));
-		error->pipe[i].vsync = I915_READ(VSYNC(cpu_transcoder));
+
+		error->num_pipes++;
+	}
+
+	error->num_transcoders = 0;
+	for (i = 0; transcoders[i] != -1; i++) {
+		enum transcoder cpu_transcoder = transcoders[i];
+
+		error->transcoder[i].cpu_transcoder = cpu_transcoder;
+
+		error->transcoder[i].conf = I915_READ(PIPECONF(cpu_transcoder));
+		error->transcoder[i].htotal = I915_READ(HTOTAL(cpu_transcoder));
+		error->transcoder[i].hblank = I915_READ(HBLANK(cpu_transcoder));
+		error->transcoder[i].hsync = I915_READ(HSYNC(cpu_transcoder));
+		error->transcoder[i].vtotal = I915_READ(VTOTAL(cpu_transcoder));
+		error->transcoder[i].vblank = I915_READ(VBLANK(cpu_transcoder));
+		error->transcoder[i].vsync = I915_READ(VSYNC(cpu_transcoder));
+
+		error->num_transcoders++;
 	}
 
 	/* In the code above we read the registers without checking if the power
@@ -10381,18 +10413,11 @@ intel_display_print_error_state(struct drm_i915_error_state_buf *m,
 	if (HAS_POWER_WELL(dev))
 		err_printf(m, "PWR_WELL_CTL2: %08x\n",
 			   error->power_well_driver);
-	for_each_pipe(i) {
+	for (i = 0; i < error->num_pipes; i++) {
 		err_printf(m, "Pipe [%d]:\n", i);
 		err_printf(m, "  CPU transcoder: %c\n",
 			   transcoder_name(error->pipe[i].cpu_transcoder));
-		err_printf(m, "  CONF: %08x\n", error->pipe[i].conf);
 		err_printf(m, "  SRC: %08x\n", error->pipe[i].source);
-		err_printf(m, "  HTOTAL: %08x\n", error->pipe[i].htotal);
-		err_printf(m, "  HBLANK: %08x\n", error->pipe[i].hblank);
-		err_printf(m, "  HSYNC: %08x\n", error->pipe[i].hsync);
-		err_printf(m, "  VTOTAL: %08x\n", error->pipe[i].vtotal);
-		err_printf(m, "  VBLANK: %08x\n", error->pipe[i].vblank);
-		err_printf(m, "  VSYNC: %08x\n", error->pipe[i].vsync);
 
 		err_printf(m, "Plane [%d]:\n", i);
 		err_printf(m, "  CNTR: %08x\n", error->plane[i].control);
@@ -10413,5 +10438,17 @@ intel_display_print_error_state(struct drm_i915_error_state_buf *m,
 		err_printf(m, "  POS: %08x\n", error->cursor[i].position);
 		err_printf(m, "  BASE: %08x\n", error->cursor[i].base);
 	}
+
+	for (i = 0; i < error->num_transcoders; i++) {
+		err_printf(m, "  CPU transcoder: %c\n",
+			   transcoder_name(error->transcoder[i].cpu_transcoder));
+		err_printf(m, "  CONF: %08x\n", error->transcoder[i].conf);
+		err_printf(m, "  HTOTAL: %08x\n", error->transcoder[i].htotal);
+		err_printf(m, "  HBLANK: %08x\n", error->transcoder[i].hblank);
+		err_printf(m, "  HSYNC: %08x\n", error->transcoder[i].hsync);
+		err_printf(m, "  VTOTAL: %08x\n", error->transcoder[i].vtotal);
+		err_printf(m, "  VBLANK: %08x\n", error->transcoder[i].vblank);
+		err_printf(m, "  VSYNC: %08x\n", error->transcoder[i].vsync);
+	}
 }
 #endif
-- 
1.8.3.1

Re: [PATCH] drm/i915: Dump all transcoder registers on error

[Daniel Vetter]

On Tue, Jun 25, 2013 at 08:15:22AM +0100, Chris Wilson wrote:
> With Haswell the transcoders are a separate bank of registers from the
> pipes (4 transcoders, 3 pipes). In event of an error, we want to be sure
> we have a complete and accurate picture of the machine state, so record
> all the transcoders in addition to all the active pipes.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

I think we should squash this together with the previous patch and move
the cpu_transcoder->pipe readout logic into intel_error_decode, similarly
to how we already augment the various ring register state with useful
context information.

I just generally prefer our error state capture code to be as dumb as
possible, with the least amount of trust for our hw/sw state that we can
get away with.
-Daniel

> ---
>  drivers/gpu/drm/i915/intel_display.c | 93 +++++++++++++++++++++++++-----------
>  1 file changed, 65 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 7ce4588..fbc2daed 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -10242,6 +10242,9 @@ struct intel_display_error_state {
>  
>  	u32 power_well_driver;
>  
> +	int num_pipes;
> +	int num_transcoders;
> +
>  	struct intel_cursor_error_state {
>  		u32 control;
>  		u32 position;
> @@ -10251,15 +10254,7 @@ struct intel_display_error_state {
>  
>  	struct intel_pipe_error_state {
>  		enum transcoder cpu_transcoder;
> -		u32 conf;
>  		u32 source;
> -
> -		u32 htotal;
> -		u32 hblank;
> -		u32 hsync;
> -		u32 vtotal;
> -		u32 vblank;
> -		u32 vsync;
>  	} pipe[I915_MAX_PIPES];
>  
>  	struct intel_plane_error_state {
> @@ -10271,6 +10266,19 @@ struct intel_display_error_state {
>  		u32 surface;
>  		u32 tile_offset;
>  	} plane[I915_MAX_PIPES];
> +
> +	struct intel_transcoder_error_state {
> +		enum transcoder cpu_transcoder;
> +
> +		u32 conf;
> +
> +		u32 htotal;
> +		u32 hblank;
> +		u32 hsync;
> +		u32 vtotal;
> +		u32 vblank;
> +		u32 vsync;
> +	} transcoder[4];
>  };
>  
>  static enum transcoder
> @@ -10310,6 +10318,13 @@ intel_display_capture_error_state(struct drm_device *dev)
>  {
>  	drm_i915_private_t *dev_priv = dev->dev_private;
>  	struct intel_display_error_state *error;
> +	int transcoders[] = {
> +		TRANSCODER_A,
> +		TRANSCODER_B,
> +		TRANSCODER_C,
> +		TRANSCODER_EDP,
> +		-1
> +	};
>  	int i;
>  
>  	error = kmalloc(sizeof(*error), GFP_ATOMIC);
> @@ -10319,12 +10334,15 @@ intel_display_capture_error_state(struct drm_device *dev)
>  	if (HAS_POWER_WELL(dev))
>  		error->power_well_driver = I915_READ(HSW_PWR_WELL_DRIVER);
>  
> -	for_each_pipe(i) {
> -		enum transcoder cpu_transcoder;
> -
> -		cpu_transcoder = read_cpu_transcoder(dev_priv, i);
> -		error->pipe[i].cpu_transcoder = cpu_transcoder;
> +	if (!HAS_DDI(dev_priv->dev))
> +		transcoders[3] = -1; /* no EDP transcoder */
> +	if (dev_priv->info->num_pipes < 3)
> +		transcoders[2] = -1;
> +	if (dev_priv->info->num_pipes < 2)
> +		transcoders[1] = -1;
>  
> +	error->num_pipes = 0;
> +	for_each_pipe(i) {
>  		if (INTEL_INFO(dev)->gen <= 6 || IS_VALLEYVIEW(dev)) {
>  			error->cursor[i].control = I915_READ(CURCNTR(i));
>  			error->cursor[i].position = I915_READ(CURPOS(i));
> @@ -10348,14 +10366,28 @@ intel_display_capture_error_state(struct drm_device *dev)
>  			error->plane[i].tile_offset = I915_READ(DSPTILEOFF(i));
>  		}
>  
> -		error->pipe[i].conf = I915_READ(PIPECONF(cpu_transcoder));
> +		error->pipe[i].cpu_transcoder =
> +			read_cpu_transcoder(dev_priv, i);
>  		error->pipe[i].source = I915_READ(PIPESRC(i));
> -		error->pipe[i].htotal = I915_READ(HTOTAL(cpu_transcoder));
> -		error->pipe[i].hblank = I915_READ(HBLANK(cpu_transcoder));
> -		error->pipe[i].hsync = I915_READ(HSYNC(cpu_transcoder));
> -		error->pipe[i].vtotal = I915_READ(VTOTAL(cpu_transcoder));
> -		error->pipe[i].vblank = I915_READ(VBLANK(cpu_transcoder));
> -		error->pipe[i].vsync = I915_READ(VSYNC(cpu_transcoder));
> +
> +		error->num_pipes++;
> +	}
> +
> +	error->num_transcoders = 0;
> +	for (i = 0; transcoders[i] != -1; i++) {
> +		enum transcoder cpu_transcoder = transcoders[i];
> +
> +		error->transcoder[i].cpu_transcoder = cpu_transcoder;
> +
> +		error->transcoder[i].conf = I915_READ(PIPECONF(cpu_transcoder));
> +		error->transcoder[i].htotal = I915_READ(HTOTAL(cpu_transcoder));
> +		error->transcoder[i].hblank = I915_READ(HBLANK(cpu_transcoder));
> +		error->transcoder[i].hsync = I915_READ(HSYNC(cpu_transcoder));
> +		error->transcoder[i].vtotal = I915_READ(VTOTAL(cpu_transcoder));
> +		error->transcoder[i].vblank = I915_READ(VBLANK(cpu_transcoder));
> +		error->transcoder[i].vsync = I915_READ(VSYNC(cpu_transcoder));
> +
> +		error->num_transcoders++;
>  	}
>  
>  	/* In the code above we read the registers without checking if the power
> @@ -10381,18 +10413,11 @@ intel_display_print_error_state(struct drm_i915_error_state_buf *m,
>  	if (HAS_POWER_WELL(dev))
>  		err_printf(m, "PWR_WELL_CTL2: %08x\n",
>  			   error->power_well_driver);
> -	for_each_pipe(i) {
> +	for (i = 0; i < error->num_pipes; i++) {
>  		err_printf(m, "Pipe [%d]:\n", i);
>  		err_printf(m, "  CPU transcoder: %c\n",
>  			   transcoder_name(error->pipe[i].cpu_transcoder));
> -		err_printf(m, "  CONF: %08x\n", error->pipe[i].conf);
>  		err_printf(m, "  SRC: %08x\n", error->pipe[i].source);
> -		err_printf(m, "  HTOTAL: %08x\n", error->pipe[i].htotal);
> -		err_printf(m, "  HBLANK: %08x\n", error->pipe[i].hblank);
> -		err_printf(m, "  HSYNC: %08x\n", error->pipe[i].hsync);
> -		err_printf(m, "  VTOTAL: %08x\n", error->pipe[i].vtotal);
> -		err_printf(m, "  VBLANK: %08x\n", error->pipe[i].vblank);
> -		err_printf(m, "  VSYNC: %08x\n", error->pipe[i].vsync);
>  
>  		err_printf(m, "Plane [%d]:\n", i);
>  		err_printf(m, "  CNTR: %08x\n", error->plane[i].control);
> @@ -10413,5 +10438,17 @@ intel_display_print_error_state(struct drm_i915_error_state_buf *m,
>  		err_printf(m, "  POS: %08x\n", error->cursor[i].position);
>  		err_printf(m, "  BASE: %08x\n", error->cursor[i].base);
>  	}
> +
> +	for (i = 0; i < error->num_transcoders; i++) {
> +		err_printf(m, "  CPU transcoder: %c\n",
> +			   transcoder_name(error->transcoder[i].cpu_transcoder));
> +		err_printf(m, "  CONF: %08x\n", error->transcoder[i].conf);
> +		err_printf(m, "  HTOTAL: %08x\n", error->transcoder[i].htotal);
> +		err_printf(m, "  HBLANK: %08x\n", error->transcoder[i].hblank);
> +		err_printf(m, "  HSYNC: %08x\n", error->transcoder[i].hsync);
> +		err_printf(m, "  VTOTAL: %08x\n", error->transcoder[i].vtotal);
> +		err_printf(m, "  VBLANK: %08x\n", error->transcoder[i].vblank);
> +		err_printf(m, "  VSYNC: %08x\n", error->transcoder[i].vsync);
> +	}
>  }
>  #endif
> -- 
> 1.8.3.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH] drm/i915: Dump all transcoder registers on error

[Daniel Vetter]

On Tue, Jun 25, 2013 at 02:43:57PM +0200, Daniel Vetter wrote:
> On Tue, Jun 25, 2013 at 08:15:22AM +0100, Chris Wilson wrote:
> > With Haswell the transcoders are a separate bank of registers from the
> > pipes (4 transcoders, 3 pipes). In event of an error, we want to be sure
> > we have a complete and accurate picture of the machine state, so record
> > all the transcoders in addition to all the active pipes.
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> 
> I think we should squash this together with the previous patch and move
> the cpu_transcoder->pipe readout logic into intel_error_decode, similarly
> to how we already augment the various ring register state with useful
> context information.
> 
> I just generally prefer our error state capture code to be as dumb as
> possible, with the least amount of trust for our hw/sw state that we can
> get away with.

I've gone ahead and done this and pushed the frobbed patch to dinq.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch