Re: [PATCH] drm/i915: Don't deref pipe->cpu_transcoder in the hangcheck code

[Jani Nikula <jani.nikula@intel.com>]

On Thu, 08 Aug 2013, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> From: Chris Wilson <chris@chris-wilson.co.uk>
>
> If we get an error event really early in the driver setup sequence,
> which gen3 is especially prone to with various display GTT faults we
> Oops. So try to avoid this.
>
> Additionally 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.
>
> This regression has been introduced in
>
> commit 702e7a56af3780d8b3a717f698209bef44187bb0
> Author: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Date:   Tue Oct 23 18:29:59 2012 -0200
>
>     drm/i915: convert PIPECONF to use transcoder instead of pipe
>
> Based on the patch "drm/i915: Dump all transcoder registers on error"
> from Chris Wilson:
>
> v2: Rebase so that we don't try to be clever and try to figure out the
> cpu transcoder from hw state. That exercise should be done when we
> analyze the error state offline.
>
> The actual bugfix is to not call intel_pipe_to_cpu_transcoder in the
> error state capture code in case the pipes aren't fully set up yet.
>
> v3: Simplifiy the err->num_transcoders computation a bit. While at it
> make the error capture stuff save on systems without a display block.
>
> v4: Fix fail, spotted by Jani.
>
> v5: Completely new commit message, cc: stable.
>
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Cc: Damien Lespiau <damien.lespiau@intel.com>
> Cc: Jani Nikula <jani.nikula@intel.com>

s/Cc/Reviewed-by/

Cheers,
Jani.

> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=60021
> Cc: stable@vger.kernel.org
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 86 ++++++++++++++++++++++++------------
>  1 file changed, 57 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 4127ad2..0b11405 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -10380,6 +10380,8 @@ struct intel_display_error_state {
>  
>  	u32 power_well_driver;
>  
> +	int num_transcoders;
> +
>  	struct intel_cursor_error_state {
>  		u32 control;
>  		u32 position;
> @@ -10388,16 +10390,7 @@ struct intel_display_error_state {
>  	} cursor[I915_MAX_PIPES];
>  
>  	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 {
> @@ -10409,6 +10402,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];
>  };
>  
>  struct intel_display_error_state *
> @@ -10416,9 +10422,17 @@ 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 transcoders[] = {
> +		TRANSCODER_A,
> +		TRANSCODER_B,
> +		TRANSCODER_C,
> +		TRANSCODER_EDP,
> +	};
>  	int i;
>  
> +	if (INTEL_INFO(dev)->num_pipes == 0)
> +		return NULL;
> +
>  	error = kmalloc(sizeof(*error), GFP_ATOMIC);
>  	if (error == NULL)
>  		return NULL;
> @@ -10427,9 +10441,6 @@ 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);
> -		error->pipe[i].cpu_transcoder = cpu_transcoder;
> -
>  		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));
> @@ -10453,14 +10464,25 @@ 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));
> +	}
> +
> +	error->num_transcoders = INTEL_INFO(dev)->num_pipes;
> +	if (HAS_DDI(dev_priv->dev))
> +		error->num_transcoders++; /* Account for eDP. */
> +
> +	for (i = 0; i < error->num_transcoders; 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));
>  	}
>  
>  	/* In the code above we read the registers without checking if the power
> @@ -10481,22 +10503,16 @@ intel_display_print_error_state(struct drm_i915_error_state_buf *m,
>  {
>  	int i;
>  
> +	if (!error)
> +		return;
> +
>  	err_printf(m, "Num Pipes: %d\n", INTEL_INFO(dev)->num_pipes);
>  	if (HAS_POWER_WELL(dev))
>  		err_printf(m, "PWR_WELL_CTL2: %08x\n",
>  			   error->power_well_driver);
>  	for_each_pipe(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);
> @@ -10517,4 +10533,16 @@ 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);
> +	}
>  }
> -- 
> 1.8.3.2
>

-- 
Jani Nikula, Intel Open Source Technology Center