| Message ID | 1375967526-12655-1-git-send-email-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 >
On Thu, Aug 08, 2013 at 03:12:06PM +0200, Daniel Vetter 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> > 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> Lgtm. We may have to modify transcoders[] to be more dynamic in future, but for now this works. Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> -Chris
On Thu, Aug 08, 2013 at 02:44:23PM +0100, Chris Wilson wrote: > On Thu, Aug 08, 2013 at 03:12:06PM +0200, Daniel Vetter 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> > > 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> > > Lgtm. We may have to modify transcoders[] to be more dynamic in future, > but for now this works. > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> Picked up for -fixes, thanks for the review. -Daniel
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); + } }