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
