| Message ID | 1450189512-30360-7-git-send-email-mika.kuoppala@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Dec 15, 2015 at 04:25:12PM +0200, Mika Kuoppala wrote: > Imre mentioned that chv might also have capability to > track unclaimed mmio accesses. And it has, so take it > into use. VLV too. My old patch: http://lists.freedesktop.org/archives/intel-gfx/2013-May/027599.html > > Cc: Imre Deak <imre.deak@intel.com> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com> > --- > drivers/gpu/drm/i915/i915_reg.h | 4 ++++ > drivers/gpu/drm/i915/intel_uncore.c | 34 ++++++++++++++++++++++++++++++---- > 2 files changed, 34 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index 007ae83..24686ab 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -1711,6 +1711,10 @@ enum skl_disp_power_wells { > #define FPGA_DBG _MMIO(0x42300) > #define FPGA_DBG_RM_NOCLAIM (1<<31) > > +#define CLAIM_ER _MMIO(0x182028) > +#define CLAIM_ER_CLR (1<<31) Looks like you forgot the overflow bit. > +#define CLAIM_ER_CTR_MASK (0xffff) Needless parens. > + > #define DERRMR _MMIO(0x44050) > /* Note that HBLANK events are reserved on bdw+ */ > #define DERRMR_PIPEA_SCANLINE (1<<0) > diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c > index 02bad32..ea8fcd4 100644 > --- a/drivers/gpu/drm/i915/intel_uncore.c > +++ b/drivers/gpu/drm/i915/intel_uncore.c > @@ -335,13 +335,10 @@ static void intel_uncore_ellc_detect(struct drm_device *dev) > } > > static bool > -check_for_unclaimed_mmio(struct drm_i915_private *dev_priv) > +fpga_check_for_unclaimed_mmio(struct drm_i915_private *dev_priv) > { > u32 dbg; > > - if (!HAS_FPGA_DBG_UNCLAIMED(dev_priv)) > - return false; > - > dbg = __raw_i915_read32(dev_priv, FPGA_DBG); > if (likely(!(dbg & FPGA_DBG_RM_NOCLAIM))) > return false; > @@ -354,6 +351,35 @@ check_for_unclaimed_mmio(struct drm_i915_private *dev_priv) > return true; > } > > +static bool > +chv_check_for_unclaimed_mmio(struct drm_i915_private *dev_priv) > +{ > + u32 cer; > + > + cer = __raw_i915_read32(dev_priv, CLAIM_ER); > + if (likely(!(cer & CLAIM_ER_CTR_MASK))) > + return false; > + > + __raw_i915_write32(dev_priv, CLAIM_ER, CLAIM_ER_CLR); > + > + /* We want to clear it asap, so post */ > + __raw_i915_read32(dev_priv, CLAIM_ER); __raw_posting_read() But not sure why we'd want to do this read really. Seems like pointless overhead. The other thing is that this only detect unclaimed RM cycles, so display registers only. Might be nice to only do the checks when accessing display registers so that we don't add overhead to the GT stuff. I think the same holds for HSW+ actually. > + > + return true; > +} > + > +static bool > +check_for_unclaimed_mmio(struct drm_i915_private *dev_priv) > +{ > + if (HAS_FPGA_DBG_UNCLAIMED(dev_priv)) > + return fpga_check_for_unclaimed_mmio(dev_priv); > + > + if (IS_CHERRYVIEW(dev_priv)) > + return chv_check_for_unclaimed_mmio(dev_priv); > + > + return false; > +} > + > static void __intel_uncore_early_sanitize(struct drm_device *dev, > bool restore_forcewake) > { > -- > 2.5.0
On Tue, Dec 15, 2015 at 04:44:25PM +0200, Ville Syrjälä wrote: > On Tue, Dec 15, 2015 at 04:25:12PM +0200, Mika Kuoppala wrote: > > Imre mentioned that chv might also have capability to > > track unclaimed mmio accesses. And it has, so take it > > into use. > > VLV too. > > My old patch: > http://lists.freedesktop.org/archives/intel-gfx/2013-May/027599.html > > > > > Cc: Imre Deak <imre.deak@intel.com> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com> > > --- > > drivers/gpu/drm/i915/i915_reg.h | 4 ++++ > > drivers/gpu/drm/i915/intel_uncore.c | 34 ++++++++++++++++++++++++++++++---- > > 2 files changed, 34 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > > index 007ae83..24686ab 100644 > > --- a/drivers/gpu/drm/i915/i915_reg.h > > +++ b/drivers/gpu/drm/i915/i915_reg.h > > @@ -1711,6 +1711,10 @@ enum skl_disp_power_wells { > > #define FPGA_DBG _MMIO(0x42300) > > #define FPGA_DBG_RM_NOCLAIM (1<<31) > > > > +#define CLAIM_ER _MMIO(0x182028) > > +#define CLAIM_ER_CLR (1<<31) > > Looks like you forgot the overflow bit. > > > +#define CLAIM_ER_CTR_MASK (0xffff) > > Needless parens. > > > + > > #define DERRMR _MMIO(0x44050) > > /* Note that HBLANK events are reserved on bdw+ */ > > #define DERRMR_PIPEA_SCANLINE (1<<0) > > diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c > > index 02bad32..ea8fcd4 100644 > > --- a/drivers/gpu/drm/i915/intel_uncore.c > > +++ b/drivers/gpu/drm/i915/intel_uncore.c > > @@ -335,13 +335,10 @@ static void intel_uncore_ellc_detect(struct drm_device *dev) > > } > > > > static bool > > -check_for_unclaimed_mmio(struct drm_i915_private *dev_priv) > > +fpga_check_for_unclaimed_mmio(struct drm_i915_private *dev_priv) > > { > > u32 dbg; > > > > - if (!HAS_FPGA_DBG_UNCLAIMED(dev_priv)) > > - return false; > > - > > dbg = __raw_i915_read32(dev_priv, FPGA_DBG); > > if (likely(!(dbg & FPGA_DBG_RM_NOCLAIM))) > > return false; > > @@ -354,6 +351,35 @@ check_for_unclaimed_mmio(struct drm_i915_private *dev_priv) > > return true; > > } > > > > +static bool > > +chv_check_for_unclaimed_mmio(struct drm_i915_private *dev_priv) > > +{ > > + u32 cer; > > + > > + cer = __raw_i915_read32(dev_priv, CLAIM_ER); > > + if (likely(!(cer & CLAIM_ER_CTR_MASK))) > > + return false; > > + > > + __raw_i915_write32(dev_priv, CLAIM_ER, CLAIM_ER_CLR); > > + > > + /* We want to clear it asap, so post */ > > + __raw_i915_read32(dev_priv, CLAIM_ER); > > __raw_posting_read() > > But not sure why we'd want to do this read really. Seems like pointless > overhead. > > The other thing is that this only detect unclaimed RM cycles, so display > registers only. Might be nice to only do the checks when accessing > display registers so that we don't add overhead to the GT stuff. > I think the same holds for HSW+ actually. Oh, after reading some more it doesn't look like you're adding the checks to the register accesses for chv. So no overhead there. I think one natural place for some explicit unclaimer reg checks would be the disp2d power well enable/disable, since a disabled power well might be the cause for the error. IIRC another could be simply a bogus register offset. > > > + > > + return true; > > +} > > + > > +static bool > > +check_for_unclaimed_mmio(struct drm_i915_private *dev_priv) > > +{ > > + if (HAS_FPGA_DBG_UNCLAIMED(dev_priv)) > > + return fpga_check_for_unclaimed_mmio(dev_priv); > > + > > + if (IS_CHERRYVIEW(dev_priv)) > > + return chv_check_for_unclaimed_mmio(dev_priv); > > + > > + return false; > > +} > > + > > static void __intel_uncore_early_sanitize(struct drm_device *dev, > > bool restore_forcewake) > > { > > -- > > 2.5.0 > > -- > Ville Syrjälä > Intel OTC > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Ville Syrjälä <ville.syrjala@linux.intel.com> writes: > On Tue, Dec 15, 2015 at 04:25:12PM +0200, Mika Kuoppala wrote: >> Imre mentioned that chv might also have capability to >> track unclaimed mmio accesses. And it has, so take it >> into use. > > VLV too. > > My old patch: > http://lists.freedesktop.org/archives/intel-gfx/2013-May/027599.html > >> >> Cc: Imre Deak <imre.deak@intel.com> >> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> >> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com> >> --- >> drivers/gpu/drm/i915/i915_reg.h | 4 ++++ >> drivers/gpu/drm/i915/intel_uncore.c | 34 ++++++++++++++++++++++++++++++---- >> 2 files changed, 34 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h >> index 007ae83..24686ab 100644 >> --- a/drivers/gpu/drm/i915/i915_reg.h >> +++ b/drivers/gpu/drm/i915/i915_reg.h >> @@ -1711,6 +1711,10 @@ enum skl_disp_power_wells { >> #define FPGA_DBG _MMIO(0x42300) >> #define FPGA_DBG_RM_NOCLAIM (1<<31) >> >> +#define CLAIM_ER _MMIO(0x182028) >> +#define CLAIM_ER_CLR (1<<31) > > Looks like you forgot the overflow bit. > >> +#define CLAIM_ER_CTR_MASK (0xffff) > > Needless parens. > >> + >> #define DERRMR _MMIO(0x44050) >> /* Note that HBLANK events are reserved on bdw+ */ >> #define DERRMR_PIPEA_SCANLINE (1<<0) >> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c >> index 02bad32..ea8fcd4 100644 >> --- a/drivers/gpu/drm/i915/intel_uncore.c >> +++ b/drivers/gpu/drm/i915/intel_uncore.c >> @@ -335,13 +335,10 @@ static void intel_uncore_ellc_detect(struct drm_device *dev) >> } >> >> static bool >> -check_for_unclaimed_mmio(struct drm_i915_private *dev_priv) >> +fpga_check_for_unclaimed_mmio(struct drm_i915_private *dev_priv) >> { >> u32 dbg; >> >> - if (!HAS_FPGA_DBG_UNCLAIMED(dev_priv)) >> - return false; >> - >> dbg = __raw_i915_read32(dev_priv, FPGA_DBG); >> if (likely(!(dbg & FPGA_DBG_RM_NOCLAIM))) >> return false; >> @@ -354,6 +351,35 @@ check_for_unclaimed_mmio(struct drm_i915_private *dev_priv) >> return true; >> } >> >> +static bool >> +chv_check_for_unclaimed_mmio(struct drm_i915_private *dev_priv) >> +{ >> + u32 cer; >> + >> + cer = __raw_i915_read32(dev_priv, CLAIM_ER); >> + if (likely(!(cer & CLAIM_ER_CTR_MASK))) >> + return false; >> + >> + __raw_i915_write32(dev_priv, CLAIM_ER, CLAIM_ER_CLR); >> + >> + /* We want to clear it asap, so post */ >> + __raw_i915_read32(dev_priv, CLAIM_ER); > > __raw_posting_read() > > But not sure why we'd want to do this read really. Seems like pointless > overhead. > I saw the bit stick for a short time even after write. That lead me to think that perhaps the detection logic in hw rearms based on this bit. So by flushing it quickly we dont coalesce the potential next write into the previous detect. Also as this is now in the slow path (mmio_debug==0), the extra overhead should not be an issue. But pointless overhead is, pointless. My worries about coalescing the detect bit might be theoretical at best, so I dont mind removing the posting. -Mika > The other thing is that this only detect unclaimed RM cycles, so display > registers only. Might be nice to only do the checks when accessing > display registers so that we don't add overhead to the GT stuff. > I think the same holds for HSW+ actually. > >> + >> + return true; >> +} >> + >> +static bool >> +check_for_unclaimed_mmio(struct drm_i915_private *dev_priv) >> +{ >> + if (HAS_FPGA_DBG_UNCLAIMED(dev_priv)) >> + return fpga_check_for_unclaimed_mmio(dev_priv); >> + >> + if (IS_CHERRYVIEW(dev_priv)) >> + return chv_check_for_unclaimed_mmio(dev_priv); >> + >> + return false; >> +} >> + >> static void __intel_uncore_early_sanitize(struct drm_device *dev, >> bool restore_forcewake) >> { >> -- >> 2.5.0 > > -- > Ville Syrjälä > Intel OTC
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index 007ae83..24686ab 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -1711,6 +1711,10 @@ enum skl_disp_power_wells { #define FPGA_DBG _MMIO(0x42300) #define FPGA_DBG_RM_NOCLAIM (1<<31) +#define CLAIM_ER _MMIO(0x182028) +#define CLAIM_ER_CLR (1<<31) +#define CLAIM_ER_CTR_MASK (0xffff) + #define DERRMR _MMIO(0x44050) /* Note that HBLANK events are reserved on bdw+ */ #define DERRMR_PIPEA_SCANLINE (1<<0) diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c index 02bad32..ea8fcd4 100644 --- a/drivers/gpu/drm/i915/intel_uncore.c +++ b/drivers/gpu/drm/i915/intel_uncore.c @@ -335,13 +335,10 @@ static void intel_uncore_ellc_detect(struct drm_device *dev) } static bool -check_for_unclaimed_mmio(struct drm_i915_private *dev_priv) +fpga_check_for_unclaimed_mmio(struct drm_i915_private *dev_priv) { u32 dbg; - if (!HAS_FPGA_DBG_UNCLAIMED(dev_priv)) - return false; - dbg = __raw_i915_read32(dev_priv, FPGA_DBG); if (likely(!(dbg & FPGA_DBG_RM_NOCLAIM))) return false; @@ -354,6 +351,35 @@ check_for_unclaimed_mmio(struct drm_i915_private *dev_priv) return true; } +static bool +chv_check_for_unclaimed_mmio(struct drm_i915_private *dev_priv) +{ + u32 cer; + + cer = __raw_i915_read32(dev_priv, CLAIM_ER); + if (likely(!(cer & CLAIM_ER_CTR_MASK))) + return false; + + __raw_i915_write32(dev_priv, CLAIM_ER, CLAIM_ER_CLR); + + /* We want to clear it asap, so post */ + __raw_i915_read32(dev_priv, CLAIM_ER); + + return true; +} + +static bool +check_for_unclaimed_mmio(struct drm_i915_private *dev_priv) +{ + if (HAS_FPGA_DBG_UNCLAIMED(dev_priv)) + return fpga_check_for_unclaimed_mmio(dev_priv); + + if (IS_CHERRYVIEW(dev_priv)) + return chv_check_for_unclaimed_mmio(dev_priv); + + return false; +} + static void __intel_uncore_early_sanitize(struct drm_device *dev, bool restore_forcewake) {
Imre mentioned that chv might also have capability to track unclaimed mmio accesses. And it has, so take it into use. Cc: Imre Deak <imre.deak@intel.com> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com> --- drivers/gpu/drm/i915/i915_reg.h | 4 ++++ drivers/gpu/drm/i915/intel_uncore.c | 34 ++++++++++++++++++++++++++++++---- 2 files changed, 34 insertions(+), 4 deletions(-)