| Message ID | 1406560776-29622-2-git-send-email-rodrigo.vivi@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
2014-07-28 12:19 GMT-03:00 Rodrigo Vivi <rodrigo.vivi@intel.com>: > 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@intel.com> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > --- > 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. > 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. I also just noticed that this function contains the "/* 4: Everything else */" comment twice. You could remove one :) > + 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@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Wed, Jul 30, 2014 at 11:09 AM, Paulo Zanoni <przanoni@gmail.com> wrote: > 2014-07-28 12:19 GMT-03:00 Rodrigo Vivi <rodrigo.vivi@intel.com>: > > 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@intel.com> > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > --- > > 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... > > > > 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@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > -- > Paulo Zanoni > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx >
2014-07-30 15:20 GMT-03:00 Rodrigo Vivi <rodrigo.vivi@gmail.com>: > > > > On Wed, Jul 30, 2014 at 11:09 AM, Paulo Zanoni <przanoni@gmail.com> wrote: >> >> 2014-07-28 12:19 GMT-03:00 Rodrigo Vivi <rodrigo.vivi@intel.com>: >> > 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@intel.com> >> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> >> > --- >> > 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@lists.freedesktop.org >> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx >> >> >> >> -- >> Paulo Zanoni >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > > -- > Rodrigo Vivi > Blog: http://blog.vivi.eng.br >
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", 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 (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)) {
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@intel.com> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> --- 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(-)