[Intel-gfx] [PATCH 10/15] drm/i915: check the power well when capturing error state

[Ben Widawsky]

On Wed, Mar 06, 2013 at 08:03:17PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni at intel.com>
> 
> This solves many "unclaimed register" messages when the power well is
> down and we get a GPU hang.
> 
> Also print the power well register and each pipe's CPU transcoder on
> the error state to allow proper interpratation of the registers. And
> kzalloc the error state structure since we may not read some of the
> registers (to avoid the "unclaimed register" messages).

Just a thought, but trying to bring the power well up and reading the
registers could also be interesting.

> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni at intel.com>

> ---
>  drivers/gpu/drm/i915/intel_display.c |   42 ++++++++++++++++++++++++++--------
>  1 file changed, 32 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index b319cd3..835bec2 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -9290,6 +9290,9 @@ int intel_modeset_vga_set_state(struct drm_device *dev, bool state)
>  #include <linux/seq_file.h>
>  
>  struct intel_display_error_state {
> +
> +	u32 power_well_driver;
> +
>  	struct intel_cursor_error_state {
>  		u32 control;
>  		u32 position;
> @@ -9298,6 +9301,7 @@ struct intel_display_error_state {
>  	} cursor[I915_MAX_PIPES];
>  
>  	struct intel_pipe_error_state {
> +		enum transcoder cpu_transcoder;
>  		u32 conf;
>  		u32 source;
>  
Putting this in the error state looks like a nice touch to me.

> @@ -9324,15 +9328,35 @@ 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;
> +	enum transcoder transcoder;
> +	bool power_well_is_down;
>  	int i;
>  
> -	error = kmalloc(sizeof(*error), GFP_ATOMIC);
> +	error = kzalloc(sizeof(*error), GFP_ATOMIC);
>  	if (error == NULL)
>  		return NULL;
>  
> +	power_well_is_down = intel_power_well_is_down(dev);
> +
> +	if (IS_HASWELL(dev))
> +		error->power_well_driver = I915_READ(HSW_PWR_WELL_DRIVER);

This is still not a stunning reason to have to pass in dev to
intel_power_well_is_down() since you have an IS_HASWELL check anyway
(from my original bikeshed so many patches ago).

> +
>  	for_each_pipe(i) {
> -		cpu_transcoder = intel_pipe_to_cpu_transcoder(dev_priv, i);
> +		transcoder = intel_pipe_to_cpu_transcoder(dev_priv, i);
> +		error->pipe[i].cpu_transcoder = transcoder;
> +
> +		if (transcoder == TRANSCODER_EDP || !power_well_is_down) {
> +			error->pipe[i].conf = I915_READ(PIPECONF(transcoder));
> +			error->pipe[i].htotal = I915_READ(HTOTAL(transcoder));
> +			error->pipe[i].hblank = I915_READ(HBLANK(transcoder));
> +			error->pipe[i].hsync = I915_READ(HSYNC(transcoder));
> +			error->pipe[i].vtotal = I915_READ(VTOTAL(transcoder));
> +			error->pipe[i].vblank = I915_READ(VBLANK(transcoder));
> +			error->pipe[i].vsync = I915_READ(VSYNC(transcoder));
> +		}
> +
> +		if (power_well_is_down)
> +			continue;
>  
>  		if (INTEL_INFO(dev)->gen <= 6) {
>  			error->cursor[i].control = I915_READ(CURCNTR(i));
> @@ -9355,14 +9379,7 @@ 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].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));
>  	}
>  
>  	return error;
> @@ -9377,8 +9394,13 @@ intel_display_print_error_state(struct seq_file *m,
>  	int i;
>  
>  	seq_printf(m, "Num Pipes: %d\n", dev_priv->num_pipe);
> +	if (IS_HASWELL(dev))
> +		seq_printf(m, "PWR_WELL_CTL2: %08x\n",
> +			   error->power_well_driver);

I wouldn't bother with the IS_HASWELL check since it's kind of nice to
have error register dumps which are the same length.

>  	for_each_pipe(i) {
>  		seq_printf(m, "Pipe [%d]:\n", i);
> +		seq_printf(m, "  CPU transcoder: %c\n",
> +			   transcoder_name(error->pipe[i].cpu_transcoder));
>  		seq_printf(m, "  CONF: %08x\n", error->pipe[i].conf);
>  		seq_printf(m, "  SRC: %08x\n", error->pipe[i].source);
>  		seq_printf(m, "  HTOTAL: %08x\n", error->pipe[i].htotal);

-- 
Ben Widawsky, Intel Open Source Technology Center