| Message ID | 1406560776-29622-1-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>: > GTIER and DEIER doesn't have same interface on HSW so this "or" operation > makes the information provided useless. > > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > --- > drivers/gpu/drm/i915/i915_drv.h | 1 + > drivers/gpu/drm/i915/i915_gpu_error.c | 16 ++++++++++------ > 2 files changed, 11 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index ef38c3b..ccb97f1 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -314,6 +314,7 @@ struct drm_i915_error_state { > u32 eir; > u32 pgtbl_er; > u32 ier; > + u32 gtier; > 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 0b3f694..372fea3 100644 > --- a/drivers/gpu/drm/i915/i915_gpu_error.c > +++ b/drivers/gpu/drm/i915/i915_gpu_error.c > @@ -359,6 +359,8 @@ int i915_error_state_to_str(struct drm_i915_error_state_buf *m, > 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_HASWELL(dev)) > + err_printf(m, "GTIER: 0x%08x\n", error->gtier); > 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); > @@ -1135,13 +1137,15 @@ static void i915_capture_reg_state(struct drm_i915_private *dev_priv, > if (HAS_HW_CONTEXTS(dev)) > error->ccid = I915_READ(CCID); > > - if (HAS_PCH_SPLIT(dev)) > + if (IS_HASWELL(dev)) { > + error->ier = I915_READ(DEIER); > + error->gtier = I915_READ(GTIER); > + } else if (HAS_PCH_SPLIT(dev)) { > error->ier = I915_READ(DEIER) | I915_READ(GTIER); You did a change for HSW only, but we have these bits since Gen5. Why don't you do this change for the whole HAS_PCH_SPLIT chunk instead of adding a HSW-specific piece? I am not a huge user of these error state files, I can't really think why we would want to "or" the IER bits, so your patch looks correct to me. > - else { > - if (IS_GEN2(dev)) > - error->ier = I915_READ16(IER); > - else > - error->ier = I915_READ(IER); > + } else if (IS_GEN2(dev)) { > + error->ier = I915_READ16(IER); > + } else { > + error->ier = I915_READ(IER); While reviewing your patch I also noticed that at the top of this function we set error->ier for VLV, but then at this point we just overwrite what was previously set. You could write another patch to fix VLV too :) > } > > /* 4: Everything else */ > -- > 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 10:53 AM, Paulo Zanoni <przanoni@gmail.com> wrote: > 2014-07-28 12:19 GMT-03:00 Rodrigo Vivi <rodrigo.vivi@intel.com>: > > GTIER and DEIER doesn't have same interface on HSW so this "or" operation > > makes the information provided useless. > > > > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > --- > > drivers/gpu/drm/i915/i915_drv.h | 1 + > > drivers/gpu/drm/i915/i915_gpu_error.c | 16 ++++++++++------ > > 2 files changed, 11 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h > b/drivers/gpu/drm/i915/i915_drv.h > > index ef38c3b..ccb97f1 100644 > > --- a/drivers/gpu/drm/i915/i915_drv.h > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > @@ -314,6 +314,7 @@ struct drm_i915_error_state { > > u32 eir; > > u32 pgtbl_er; > > u32 ier; > > + u32 gtier; > > 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 0b3f694..372fea3 100644 > > --- a/drivers/gpu/drm/i915/i915_gpu_error.c > > +++ b/drivers/gpu/drm/i915/i915_gpu_error.c > > @@ -359,6 +359,8 @@ int i915_error_state_to_str(struct > drm_i915_error_state_buf *m, > > 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_HASWELL(dev)) > > + err_printf(m, "GTIER: 0x%08x\n", error->gtier); > > 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); > > @@ -1135,13 +1137,15 @@ static void i915_capture_reg_state(struct > drm_i915_private *dev_priv, > > if (HAS_HW_CONTEXTS(dev)) > > error->ccid = I915_READ(CCID); > > > > - if (HAS_PCH_SPLIT(dev)) > > + if (IS_HASWELL(dev)) { > > + error->ier = I915_READ(DEIER); > > + error->gtier = I915_READ(GTIER); > > + } else if (HAS_PCH_SPLIT(dev)) { > > error->ier = I915_READ(DEIER) | I915_READ(GTIER); > > You did a change for HSW only, but we have these bits since Gen5. Why > don't you do this change for the whole HAS_PCH_SPLIT chunk instead of > adding a HSW-specific piece? > > I am not a huge user of these error state files, I can't really think > why we would want to "or" the IER bits, so your patch looks correct to > me. > I believe before HSW they had the same interface both DEIR and GTIER, but since I'm splitting it for HSW you are right we can split everywhere else with or without common interface. > > > > - else { > > - if (IS_GEN2(dev)) > > - error->ier = I915_READ16(IER); > > - else > > - error->ier = I915_READ(IER); > > + } else if (IS_GEN2(dev)) { > > + error->ier = I915_READ16(IER); > > + } else { > > + error->ier = I915_READ(IER); > > While reviewing your patch I also noticed that at the top of this > function we set error->ier for VLV, but then at this point we just > overwrite what was previously set. You could write another patch to > fix VLV too :) > Yeah, it is messy... I'll also split for VLV and organize a bit to avoid overwriting... > > > > } > > > > /* 4: Everything else */ > > -- > > 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 >
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index ef38c3b..ccb97f1 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -314,6 +314,7 @@ struct drm_i915_error_state { u32 eir; u32 pgtbl_er; u32 ier; + u32 gtier; 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 0b3f694..372fea3 100644 --- a/drivers/gpu/drm/i915/i915_gpu_error.c +++ b/drivers/gpu/drm/i915/i915_gpu_error.c @@ -359,6 +359,8 @@ int i915_error_state_to_str(struct drm_i915_error_state_buf *m, 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_HASWELL(dev)) + err_printf(m, "GTIER: 0x%08x\n", error->gtier); 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); @@ -1135,13 +1137,15 @@ static void i915_capture_reg_state(struct drm_i915_private *dev_priv, if (HAS_HW_CONTEXTS(dev)) error->ccid = I915_READ(CCID); - if (HAS_PCH_SPLIT(dev)) + if (IS_HASWELL(dev)) { + error->ier = I915_READ(DEIER); + error->gtier = I915_READ(GTIER); + } else if (HAS_PCH_SPLIT(dev)) { error->ier = I915_READ(DEIER) | I915_READ(GTIER); - else { - if (IS_GEN2(dev)) - error->ier = I915_READ16(IER); - else - error->ier = I915_READ(IER); + } else if (IS_GEN2(dev)) { + error->ier = I915_READ16(IER); + } else { + error->ier = I915_READ(IER); } /* 4: Everything else */
GTIER and DEIER doesn't have same interface on HSW so this "or" operation makes the information provided useless. Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> --- drivers/gpu/drm/i915/i915_drv.h | 1 + drivers/gpu/drm/i915/i915_gpu_error.c | 16 ++++++++++------ 2 files changed, 11 insertions(+), 6 deletions(-)