[Intel-gfx] [PATCH] drm/i915: Don't deref pipe->cpu_transcoder in the hangcheck code
Jani Nikula
jani.nikula at intel.com
Thu Aug 8 15:40:51 CEST 2013
On Thu, 08 Aug 2013, Daniel Vetter <daniel.vetter at ffwll.ch> wrote:
> From: Chris Wilson <chris at 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 at 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 at intel.com>
> Cc: Damien Lespiau <damien.lespiau at intel.com>
> Cc: Jani Nikula <jani.nikula at intel.com>
s/Cc/Reviewed-by/
Cheers,
Jani.
> Cc: Chris Wilson <chris at chris-wilson.co.uk>
> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=60021
> Cc: stable at vger.kernel.org
> Signed-off-by: Daniel Vetter <daniel.vetter at 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
More information about the Intel-gfx
mailing list