| Message ID | 1491493182-31540-1-git-send-email-mika.kuoppala@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Apr 06, 2017 at 06:39:42PM +0300, Mika Kuoppala wrote: > Remove the per-mmio checking of the FIFO debug register into the common > conditional mmio debug handling. Based on patch from Chris Wilson. > > v2: postpone warn on fifodbg for unclaimed reg debugs > > Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com> > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> > --- > drivers/gpu/drm/i915/intel_uncore.c | 76 +++++++++++++++---------------------- > 1 file changed, 30 insertions(+), 46 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c > index 6d1ea26..7a8eb2e 100644 > --- a/drivers/gpu/drm/i915/intel_uncore.c > +++ b/drivers/gpu/drm/i915/intel_uncore.c > @@ -172,22 +172,6 @@ static void fw_domains_get_with_thread_status(struct drm_i915_private *dev_priv, > __gen6_gt_wait_for_thread_c0(dev_priv); > } > > -static void gen6_gt_check_fifodbg(struct drm_i915_private *dev_priv) > -{ > - u32 gtfifodbg; > - > - gtfifodbg = __raw_i915_read32(dev_priv, GTFIFODBG); > - if (WARN(gtfifodbg, "GT wake FIFO error 0x%x\n", gtfifodbg)) > - __raw_i915_write32(dev_priv, GTFIFODBG, gtfifodbg); > -} > - > -static void fw_domains_put_with_fifo(struct drm_i915_private *dev_priv, > - enum forcewake_domains fw_domains) > -{ > - fw_domains_put(dev_priv, fw_domains); > - gen6_gt_check_fifodbg(dev_priv); > -} > - > static inline u32 fifo_free_entries(struct drm_i915_private *dev_priv) > { > u32 count = __raw_i915_read32(dev_priv, GTFIFOCTL); > @@ -384,15 +368,35 @@ vlv_check_for_unclaimed_mmio(struct drm_i915_private *dev_priv) > } > > static bool > +gen6_check_for_fifo_debug(struct drm_i915_private *dev_priv) > +{ > + u32 fifodbg; > + > + fifodbg = __raw_i915_read32(dev_priv, GTFIFODBG); > + > + if (unlikely(fifodbg)) { > + DRM_DEBUG_DRIVER("GTFIFODBG = 0x08%x\n", fifodbg); > + __raw_i915_write32(dev_priv, GTFIFODBG, fifodbg); > + } > + > + return fifodbg; > +} > + > +static bool > check_for_unclaimed_mmio(struct drm_i915_private *dev_priv) > { > + bool ret = false; > + > if (HAS_FPGA_DBG_UNCLAIMED(dev_priv)) > - return fpga_check_for_unclaimed_mmio(dev_priv); > + ret |= fpga_check_for_unclaimed_mmio(dev_priv); > > if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) > - return vlv_check_for_unclaimed_mmio(dev_priv); > + ret |= vlv_check_for_unclaimed_mmio(dev_priv); > > - return false; > + if (IS_GEN6(dev_priv) || IS_GEN7(dev_priv)) > + ret |= gen6_check_for_fifo_debug(dev_priv); I'd prefer to keep unclaim vs. wake FIFO separate because the unclaimned stuff is only for display registers. In my plan to split the uncore lock into gt and display locks the unclaimed reg stuff would end up being protected by the display lock rather than the gt lock. > + > + return ret; > } > > static void __intel_uncore_early_sanitize(struct drm_i915_private *dev_priv, > @@ -404,11 +408,6 @@ static void __intel_uncore_early_sanitize(struct drm_i915_private *dev_priv, > if (check_for_unclaimed_mmio(dev_priv)) > DRM_DEBUG("unclaimed mmio detected on uncore init, clearing\n"); > > - /* clear out old GT FIFO errors */ > - if (IS_GEN6(dev_priv) || IS_GEN7(dev_priv)) > - __raw_i915_write32(dev_priv, GTFIFODBG, > - __raw_i915_read32(dev_priv, GTFIFODBG)); > - > /* WaDisableShadowRegForCpd:chv */ > if (IS_CHERRYVIEW(dev_priv)) { > __raw_i915_write32(dev_priv, GTFIFOCTL, > @@ -1047,15 +1046,10 @@ __gen2_write(32) > #define __gen6_write(x) \ > static void \ > gen6_write##x(struct drm_i915_private *dev_priv, i915_reg_t reg, u##x val, bool trace) { \ > - u32 __fifo_ret = 0; \ > GEN6_WRITE_HEADER; \ > - if (NEEDS_FORCE_WAKE(offset)) { \ > - __fifo_ret = __gen6_gt_wait_for_fifo(dev_priv); \ > - } \ > + if (NEEDS_FORCE_WAKE(offset)) \ > + __gen6_gt_wait_for_fifo(dev_priv); \ > __raw_i915_write##x(dev_priv, reg, val); \ > - if (unlikely(__fifo_ret)) { \ > - gen6_gt_check_fifodbg(dev_priv); \ > - } \ > GEN6_WRITE_FOOTER; \ > } > > @@ -1190,11 +1184,7 @@ static void intel_uncore_fw_domains_init(struct drm_i915_private *dev_priv) > FORCEWAKE_MEDIA_GEN9, FORCEWAKE_ACK_MEDIA_GEN9); > } else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) { > dev_priv->uncore.funcs.force_wake_get = fw_domains_get; > - if (!IS_CHERRYVIEW(dev_priv)) > - dev_priv->uncore.funcs.force_wake_put = > - fw_domains_put_with_fifo; > - else > - dev_priv->uncore.funcs.force_wake_put = fw_domains_put; > + dev_priv->uncore.funcs.force_wake_put = fw_domains_put; > fw_domain_init(dev_priv, FW_DOMAIN_ID_RENDER, > FORCEWAKE_VLV, FORCEWAKE_ACK_VLV); > fw_domain_init(dev_priv, FW_DOMAIN_ID_MEDIA, > @@ -1202,11 +1192,7 @@ static void intel_uncore_fw_domains_init(struct drm_i915_private *dev_priv) > } else if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) { > dev_priv->uncore.funcs.force_wake_get = > fw_domains_get_with_thread_status; > - if (IS_HASWELL(dev_priv)) > - dev_priv->uncore.funcs.force_wake_put = > - fw_domains_put_with_fifo; > - else > - dev_priv->uncore.funcs.force_wake_put = fw_domains_put; > + dev_priv->uncore.funcs.force_wake_put = fw_domains_put; > fw_domain_init(dev_priv, FW_DOMAIN_ID_RENDER, > FORCEWAKE_MT, FORCEWAKE_ACK_HSW); > } else if (IS_IVYBRIDGE(dev_priv)) { > @@ -1223,8 +1209,7 @@ static void intel_uncore_fw_domains_init(struct drm_i915_private *dev_priv) > */ > dev_priv->uncore.funcs.force_wake_get = > fw_domains_get_with_thread_status; > - dev_priv->uncore.funcs.force_wake_put = > - fw_domains_put_with_fifo; > + dev_priv->uncore.funcs.force_wake_put = fw_domains_put; > > /* We need to init first for ECOBUS access and then > * determine later if we want to reinit, in case of MT access is > @@ -1242,7 +1227,7 @@ static void intel_uncore_fw_domains_init(struct drm_i915_private *dev_priv) > spin_lock_irq(&dev_priv->uncore.lock); > fw_domains_get_with_thread_status(dev_priv, FORCEWAKE_RENDER); > ecobus = __raw_i915_read32(dev_priv, ECOBUS); > - fw_domains_put_with_fifo(dev_priv, FORCEWAKE_RENDER); > + fw_domains_put(dev_priv, FORCEWAKE_RENDER); > spin_unlock_irq(&dev_priv->uncore.lock); > > if (!(ecobus & FORCEWAKE_MT_ENABLE)) { > @@ -1254,8 +1239,7 @@ static void intel_uncore_fw_domains_init(struct drm_i915_private *dev_priv) > } else if (IS_GEN6(dev_priv)) { > dev_priv->uncore.funcs.force_wake_get = > fw_domains_get_with_thread_status; > - dev_priv->uncore.funcs.force_wake_put = > - fw_domains_put_with_fifo; > + dev_priv->uncore.funcs.force_wake_put = fw_domains_put; > fw_domain_init(dev_priv, FW_DOMAIN_ID_RENDER, > FORCEWAKE, FORCEWAKE_ACK); > } > -- > 2.7.4 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Thu, Apr 06, 2017 at 06:46:29PM +0300, Ville Syrjälä wrote: > On Thu, Apr 06, 2017 at 06:39:42PM +0300, Mika Kuoppala wrote: > > +static bool > > check_for_unclaimed_mmio(struct drm_i915_private *dev_priv) > > { > > + bool ret = false; > > + > > if (HAS_FPGA_DBG_UNCLAIMED(dev_priv)) > > - return fpga_check_for_unclaimed_mmio(dev_priv); > > + ret |= fpga_check_for_unclaimed_mmio(dev_priv); > > > > if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) > > - return vlv_check_for_unclaimed_mmio(dev_priv); > > + ret |= vlv_check_for_unclaimed_mmio(dev_priv); > > > > - return false; > > + if (IS_GEN6(dev_priv) || IS_GEN7(dev_priv)) > > + ret |= gen6_check_for_fifo_debug(dev_priv); > > I'd prefer to keep unclaim vs. wake FIFO separate because the > unclaimned stuff is only for display registers. In my plan to > split the uncore lock into gt and display locks the unclaimed > reg stuff would end up being protected by the display lock rather > than the gt lock. I don't think it is much of a hindrance, right? We just split it out when splitting dpy vs gt. Mika was digging through GTFIFODBG and thought it had some bits for a sideband underflow... Random thought, would i915->mmio.writeX[reg < 0x40000](i915, reg, val) or just push all the decisions to the backend? I hope gcc would be able to automatically store the function pointer i915->mmio.writeX[reg < 0x40000] -Chris
On Thu, Apr 06, 2017 at 05:05:10PM +0100, Chris Wilson wrote: > On Thu, Apr 06, 2017 at 06:46:29PM +0300, Ville Syrjälä wrote: > > On Thu, Apr 06, 2017 at 06:39:42PM +0300, Mika Kuoppala wrote: > > > +static bool > > > check_for_unclaimed_mmio(struct drm_i915_private *dev_priv) > > > { > > > + bool ret = false; > > > + > > > if (HAS_FPGA_DBG_UNCLAIMED(dev_priv)) > > > - return fpga_check_for_unclaimed_mmio(dev_priv); > > > + ret |= fpga_check_for_unclaimed_mmio(dev_priv); > > > > > > if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) > > > - return vlv_check_for_unclaimed_mmio(dev_priv); > > > + ret |= vlv_check_for_unclaimed_mmio(dev_priv); > > > > > > - return false; > > > + if (IS_GEN6(dev_priv) || IS_GEN7(dev_priv)) > > > + ret |= gen6_check_for_fifo_debug(dev_priv); > > > > I'd prefer to keep unclaim vs. wake FIFO separate because the > > unclaimned stuff is only for display registers. In my plan to > > split the uncore lock into gt and display locks the unclaimed > > reg stuff would end up being protected by the display lock rather > > than the gt lock. > > I don't think it is much of a hindrance, right? We just split it out when > splitting dpy vs gt. I suppose. Although if we want to do the FIFO checks for non-GT stuff as well, then I guess we'd have to risk hitting the FIFO register from both the display and gt code paths. Not sure if that's safe or not. > Mika was digging through GTFIFODBG and thought it > had some bits for a sideband underflow... > > Random thought, would i915->mmio.writeX[reg < 0x40000](i915, reg, val) > or just push all the decisions to the backend? I hope gcc would be able > to automatically store the function pointer i915->mmio.writeX[reg < 0x40000] I haven't done any real performance analysis on this stuff TBH, apart from looking at the duration of the atomic commits. I guess separate function pointers might be nice for code readability at least.
diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c index 6d1ea26..7a8eb2e 100644 --- a/drivers/gpu/drm/i915/intel_uncore.c +++ b/drivers/gpu/drm/i915/intel_uncore.c @@ -172,22 +172,6 @@ static void fw_domains_get_with_thread_status(struct drm_i915_private *dev_priv, __gen6_gt_wait_for_thread_c0(dev_priv); } -static void gen6_gt_check_fifodbg(struct drm_i915_private *dev_priv) -{ - u32 gtfifodbg; - - gtfifodbg = __raw_i915_read32(dev_priv, GTFIFODBG); - if (WARN(gtfifodbg, "GT wake FIFO error 0x%x\n", gtfifodbg)) - __raw_i915_write32(dev_priv, GTFIFODBG, gtfifodbg); -} - -static void fw_domains_put_with_fifo(struct drm_i915_private *dev_priv, - enum forcewake_domains fw_domains) -{ - fw_domains_put(dev_priv, fw_domains); - gen6_gt_check_fifodbg(dev_priv); -} - static inline u32 fifo_free_entries(struct drm_i915_private *dev_priv) { u32 count = __raw_i915_read32(dev_priv, GTFIFOCTL); @@ -384,15 +368,35 @@ vlv_check_for_unclaimed_mmio(struct drm_i915_private *dev_priv) } static bool +gen6_check_for_fifo_debug(struct drm_i915_private *dev_priv) +{ + u32 fifodbg; + + fifodbg = __raw_i915_read32(dev_priv, GTFIFODBG); + + if (unlikely(fifodbg)) { + DRM_DEBUG_DRIVER("GTFIFODBG = 0x08%x\n", fifodbg); + __raw_i915_write32(dev_priv, GTFIFODBG, fifodbg); + } + + return fifodbg; +} + +static bool check_for_unclaimed_mmio(struct drm_i915_private *dev_priv) { + bool ret = false; + if (HAS_FPGA_DBG_UNCLAIMED(dev_priv)) - return fpga_check_for_unclaimed_mmio(dev_priv); + ret |= fpga_check_for_unclaimed_mmio(dev_priv); if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) - return vlv_check_for_unclaimed_mmio(dev_priv); + ret |= vlv_check_for_unclaimed_mmio(dev_priv); - return false; + if (IS_GEN6(dev_priv) || IS_GEN7(dev_priv)) + ret |= gen6_check_for_fifo_debug(dev_priv); + + return ret; } static void __intel_uncore_early_sanitize(struct drm_i915_private *dev_priv, @@ -404,11 +408,6 @@ static void __intel_uncore_early_sanitize(struct drm_i915_private *dev_priv, if (check_for_unclaimed_mmio(dev_priv)) DRM_DEBUG("unclaimed mmio detected on uncore init, clearing\n"); - /* clear out old GT FIFO errors */ - if (IS_GEN6(dev_priv) || IS_GEN7(dev_priv)) - __raw_i915_write32(dev_priv, GTFIFODBG, - __raw_i915_read32(dev_priv, GTFIFODBG)); - /* WaDisableShadowRegForCpd:chv */ if (IS_CHERRYVIEW(dev_priv)) { __raw_i915_write32(dev_priv, GTFIFOCTL, @@ -1047,15 +1046,10 @@ __gen2_write(32) #define __gen6_write(x) \ static void \ gen6_write##x(struct drm_i915_private *dev_priv, i915_reg_t reg, u##x val, bool trace) { \ - u32 __fifo_ret = 0; \ GEN6_WRITE_HEADER; \ - if (NEEDS_FORCE_WAKE(offset)) { \ - __fifo_ret = __gen6_gt_wait_for_fifo(dev_priv); \ - } \ + if (NEEDS_FORCE_WAKE(offset)) \ + __gen6_gt_wait_for_fifo(dev_priv); \ __raw_i915_write##x(dev_priv, reg, val); \ - if (unlikely(__fifo_ret)) { \ - gen6_gt_check_fifodbg(dev_priv); \ - } \ GEN6_WRITE_FOOTER; \ } @@ -1190,11 +1184,7 @@ static void intel_uncore_fw_domains_init(struct drm_i915_private *dev_priv) FORCEWAKE_MEDIA_GEN9, FORCEWAKE_ACK_MEDIA_GEN9); } else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) { dev_priv->uncore.funcs.force_wake_get = fw_domains_get; - if (!IS_CHERRYVIEW(dev_priv)) - dev_priv->uncore.funcs.force_wake_put = - fw_domains_put_with_fifo; - else - dev_priv->uncore.funcs.force_wake_put = fw_domains_put; + dev_priv->uncore.funcs.force_wake_put = fw_domains_put; fw_domain_init(dev_priv, FW_DOMAIN_ID_RENDER, FORCEWAKE_VLV, FORCEWAKE_ACK_VLV); fw_domain_init(dev_priv, FW_DOMAIN_ID_MEDIA, @@ -1202,11 +1192,7 @@ static void intel_uncore_fw_domains_init(struct drm_i915_private *dev_priv) } else if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) { dev_priv->uncore.funcs.force_wake_get = fw_domains_get_with_thread_status; - if (IS_HASWELL(dev_priv)) - dev_priv->uncore.funcs.force_wake_put = - fw_domains_put_with_fifo; - else - dev_priv->uncore.funcs.force_wake_put = fw_domains_put; + dev_priv->uncore.funcs.force_wake_put = fw_domains_put; fw_domain_init(dev_priv, FW_DOMAIN_ID_RENDER, FORCEWAKE_MT, FORCEWAKE_ACK_HSW); } else if (IS_IVYBRIDGE(dev_priv)) { @@ -1223,8 +1209,7 @@ static void intel_uncore_fw_domains_init(struct drm_i915_private *dev_priv) */ dev_priv->uncore.funcs.force_wake_get = fw_domains_get_with_thread_status; - dev_priv->uncore.funcs.force_wake_put = - fw_domains_put_with_fifo; + dev_priv->uncore.funcs.force_wake_put = fw_domains_put; /* We need to init first for ECOBUS access and then * determine later if we want to reinit, in case of MT access is @@ -1242,7 +1227,7 @@ static void intel_uncore_fw_domains_init(struct drm_i915_private *dev_priv) spin_lock_irq(&dev_priv->uncore.lock); fw_domains_get_with_thread_status(dev_priv, FORCEWAKE_RENDER); ecobus = __raw_i915_read32(dev_priv, ECOBUS); - fw_domains_put_with_fifo(dev_priv, FORCEWAKE_RENDER); + fw_domains_put(dev_priv, FORCEWAKE_RENDER); spin_unlock_irq(&dev_priv->uncore.lock); if (!(ecobus & FORCEWAKE_MT_ENABLE)) { @@ -1254,8 +1239,7 @@ static void intel_uncore_fw_domains_init(struct drm_i915_private *dev_priv) } else if (IS_GEN6(dev_priv)) { dev_priv->uncore.funcs.force_wake_get = fw_domains_get_with_thread_status; - dev_priv->uncore.funcs.force_wake_put = - fw_domains_put_with_fifo; + dev_priv->uncore.funcs.force_wake_put = fw_domains_put; fw_domain_init(dev_priv, FW_DOMAIN_ID_RENDER, FORCEWAKE, FORCEWAKE_ACK); }