2014-07-30 15:20 GMT-03:00 Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>: > > > > On Wed, Jul 30, 2014 at 11:09 AM, Paulo Zanoni <przanoni@xxxxxxxxx> wrote: >> >> 2014-07-28 12:19 GMT-03:00 Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>: >> > BDW has many other Display Engine interrupts and GT interrupts >> > registers. >> > Collecting it properly on gpu_error_state. >> > >> > On debugfs all was properly listed already but besides we were also >> > listing old >> > DEIER and GTIER that doesn't exist on BDW anymore. This was causing >> > unclaimed register messages: >> > >> > https://bugs.freedesktop.org/show_bug.cgi?id=81701 >> > >> > Cc: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx> >> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx> >> > --- >> > drivers/gpu/drm/i915/i915_debugfs.c | 2 +- >> > drivers/gpu/drm/i915/i915_drv.h | 4 +++- >> > drivers/gpu/drm/i915/i915_gpu_error.c | 23 ++++++++++++++++++++--- >> > 3 files changed, 24 insertions(+), 5 deletions(-) >> > >> > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c >> > b/drivers/gpu/drm/i915/i915_debugfs.c >> > index 9e737b7..679cda6 100644 >> > --- a/drivers/gpu/drm/i915/i915_debugfs.c >> > +++ b/drivers/gpu/drm/i915/i915_debugfs.c >> > @@ -783,7 +783,7 @@ static int i915_interrupt_info(struct seq_file *m, >> > void *data) >> > seq_printf(m, "Pipe %c stat: %08x\n", >> > pipe_name(pipe), >> > I915_READ(PIPESTAT(pipe))); >> > - } else { >> > + } else if (!IS_BROADWELL(dev)) { >> > seq_printf(m, "North Display Interrupt enable: >> > %08x\n", >> > I915_READ(DEIER)); >> > seq_printf(m, "North Display Interrupt identity: >> > %08x\n", >> >> This chunk is not needed since we already have a check for "gen >= 8" >> at the top. > > > oh, for a moment I thought it was messy as the other code and it was writing > twice... > mainly when you mentioned that you could reproduce the unclaimed when > reading debugfs... > but probably was related to the powerwell you mentioned below... Yes, at i915_interrupt_info we also need to call intel_display_power_enabled(). I also won't complain if you fix i915_frequency_info on BDW :) > >> >> >> >> > diff --git a/drivers/gpu/drm/i915/i915_drv.h >> > b/drivers/gpu/drm/i915/i915_drv.h >> > index ccb97f1..ee28cd7 100644 >> > --- a/drivers/gpu/drm/i915/i915_drv.h >> > +++ b/drivers/gpu/drm/i915/i915_drv.h >> > @@ -314,7 +314,9 @@ struct drm_i915_error_state { >> > u32 eir; >> > u32 pgtbl_er; >> > u32 ier; >> > - u32 gtier; >> > + u32 gtier[4]; >> > + u32 deier[3]; >> > + u32 de_misc_ier; >> > u32 ccid; >> > u32 derrmr; >> > u32 forcewake; >> > diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c >> > b/drivers/gpu/drm/i915/i915_gpu_error.c >> > index 372fea3..f865d1d 100644 >> > --- a/drivers/gpu/drm/i915/i915_gpu_error.c >> > +++ b/drivers/gpu/drm/i915/i915_gpu_error.c >> > @@ -358,9 +358,19 @@ int i915_error_state_to_str(struct >> > drm_i915_error_state_buf *m, >> > err_printf(m, "Suspend count: %u\n", error->suspend_count); >> > err_printf(m, "PCI ID: 0x%04x\n", dev->pdev->device); >> > err_printf(m, "EIR: 0x%08x\n", error->eir); >> > - err_printf(m, "IER: 0x%08x\n", error->ier); >> > + if (IS_BROADWELL(dev)) { >> > + for_each_pipe(i) >> > + err_printf(m, "DEIER pipe %c: 0x%08x\n", >> > pipe_name(i), >> > + error->deier[i]); >> > + for (i = 0; i < 4; i++) >> > + err_printf(m, "GTIER gt %d: 0x%08x\n", i, >> > + error->gtier[i]); >> > + err_printf(m, "DE_MISC_IER: 0x%08x\n", >> > error->de_misc_ier); >> > + } else { >> > + err_printf(m, "IER: 0x%08x\n", error->ier); >> > + } >> > if (IS_HASWELL(dev)) >> > - err_printf(m, "GTIER: 0x%08x\n", error->gtier); >> > + err_printf(m, "GTIER: 0x%08x\n", error->gtier[0]); >> > err_printf(m, "PGTBL_ER: 0x%08x\n", error->pgtbl_er); >> > err_printf(m, "FORCEWAKE: 0x%08x\n", error->forcewake); >> > err_printf(m, "DERRMR: 0x%08x\n", error->derrmr); >> > @@ -1093,6 +1103,7 @@ static void i915_capture_reg_state(struct >> > drm_i915_private *dev_priv, >> > struct drm_i915_error_state *error) >> > { >> > struct drm_device *dev = dev_priv->dev; >> > + int i; >> > >> > /* General organization >> > * 1. Registers specific to a single generation >> > @@ -1139,7 +1150,13 @@ static void i915_capture_reg_state(struct >> > drm_i915_private *dev_priv, >> > >> > if (IS_HASWELL(dev)) { >> > error->ier = I915_READ(DEIER); >> > - error->gtier = I915_READ(GTIER); >> > + error->gtier[0] = I915_READ(GTIER); >> > + } else if (IS_BROADWELL(dev)) { >> > + for_each_pipe(i) >> > + error->deier[i] = >> > I915_READ(GEN8_DE_PIPE_IER(i)); >> >> for_each_pipe(i) >> if (intel_display_power_enabled(dev_priv, POWER_DOMAIN_PIPE(i))) >> error->deier[i] = I915_READ(GEN8_DE_PIPE_IER(i)); >> >> If the pipe's power well is disabled, the IER register is powered off >> and we can get an unclaimed register errors too. > > > thanks! I'll add it... > Also also need to be added to debugfs interface. > >> >> >> I also just noticed that this function contains the "/* 4: Everything >> else */" comment twice. You could remove one :) > > > indeed. > > >> >> >> >> > + for (i = 0; i < 4; i++) >> > + error->gtier[3] = I915_READ(GEN8_GT_IER(i)); >> > + error->de_misc_ier = I915_READ(GEN8_DE_MISC_IER); >> > } else if (HAS_PCH_SPLIT(dev)) { >> > error->ier = I915_READ(DEIER) | I915_READ(GTIER); >> > } else if (IS_GEN2(dev)) { >> > -- >> > 1.9.3 >> > >> > _______________________________________________ >> > Intel-gfx mailing list >> > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx >> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx >> >> >> >> -- >> Paulo Zanoni >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > > -- > Rodrigo Vivi > Blog: http://blog.vivi.eng.br > -- Paulo Zanoni _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
