2015-03-27 8:01 GMT-03:00 Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>:
> Delay the expensive read on the FPGA_DBG register from once per mmio to
> once per forcewake section when we are doing the general wellbeing
> check rather than the targetted error detection. This almost reduces
> the overhead of the debug facility (for example when submitting execlists)
> to zero whilst keeping the debug checks around.
>
> v2: Enable one-shot mmio debugging from the interrupt check as well as a
> safeguard to catch invalid display writes from outside the powerwell.
>
> Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> Cc: Daniel Vetter <daniel.vetter@xxxxxxxx>
> Cc: Mika Kuoppala <mika.kuoppala@xxxxxxxxx>
> Cc: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx>
> ---
> drivers/gpu/drm/i915/intel_uncore.c | 56 ++++++++++++++++++++-----------------
> 1 file changed, 30 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> index ab5cc94588e1..0e32bbbcada8 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -149,6 +149,30 @@ fw_domains_put(struct drm_i915_private *dev_priv, enum forcewake_domains fw_doma
> }
>
> static void
> +hsw_unclaimed_reg_detect(struct drm_i915_private *dev_priv)
> +{
> + static bool mmio_debug_once = true;
> +
> + if (i915.mmio_debug || !mmio_debug_once)
> + return;
> +
> + if (__raw_i915_read32(dev_priv, FPGA_DBG) & FPGA_DBG_RM_NOCLAIM) {
> + DRM_DEBUG("Unclaimed register detected, "
> + "enabling oneshot unclaimed register reporting. "
> + "Please use i915.mmio_debug=N for more information.\n");
> + __raw_i915_write32(dev_priv, FPGA_DBG, FPGA_DBG_RM_NOCLAIM);
> + i915.mmio_debug = mmio_debug_once--;
> + }
> +}
> +
> +static void
> +fw_domains_put_debug(struct drm_i915_private *dev_priv, enum forcewake_domains fw_domains)
> +{
> + hsw_unclaimed_reg_detect(dev_priv);
> + fw_domains_put(dev_priv, fw_domains);
> +}
This means we won't check during the forcewake puts that are on the
register read/write macros. Is this intentional? I tried checking the
FW code calls, and it seems to me that we're not really going to run
hsw_unclaimed_reg_detect very frequently anymore. I wonder if there's
the risk of staying a long time without running it. But maybe I'm just
wrong.
> +
> +static void
> fw_domains_posting_read(struct drm_i915_private *dev_priv)
> {
> struct intel_uncore_forcewake_domain *d;
> @@ -561,23 +585,6 @@ hsw_unclaimed_reg_debug(struct drm_i915_private *dev_priv, u32 reg, bool read,
> }
> }
>
> -static void
> -hsw_unclaimed_reg_detect(struct drm_i915_private *dev_priv)
> -{
> - static bool mmio_debug_once = true;
> -
> - if (i915.mmio_debug || !mmio_debug_once)
> - return;
> -
> - if (__raw_i915_read32(dev_priv, FPGA_DBG) & FPGA_DBG_RM_NOCLAIM) {
> - DRM_DEBUG("Unclaimed register detected, "
> - "enabling oneshot unclaimed register reporting. "
> - "Please use i915.mmio_debug=N for more information.\n");
> - __raw_i915_write32(dev_priv, FPGA_DBG, FPGA_DBG_RM_NOCLAIM);
> - i915.mmio_debug = mmio_debug_once--;
> - }
> -}
> -
> #define GEN2_READ_HEADER(x) \
> u##x val = 0; \
> assert_device_not_suspended(dev_priv);
> @@ -829,7 +836,6 @@ hsw_write##x(struct drm_i915_private *dev_priv, off_t reg, u##x val, bool trace)
> gen6_gt_check_fifodbg(dev_priv); \
> } \
> hsw_unclaimed_reg_debug(dev_priv, reg, false, false); \
> - hsw_unclaimed_reg_detect(dev_priv); \
> GEN6_WRITE_FOOTER; \
> }
>
> @@ -871,7 +877,6 @@ gen8_write##x(struct drm_i915_private *dev_priv, off_t reg, u##x val, bool trace
> __force_wake_get(dev_priv, FORCEWAKE_RENDER); \
> __raw_i915_write##x(dev_priv, reg, val); \
> hsw_unclaimed_reg_debug(dev_priv, reg, false, false); \
> - hsw_unclaimed_reg_detect(dev_priv); \
> GEN6_WRITE_FOOTER; \
> }
>
> @@ -1120,6 +1125,10 @@ static void intel_uncore_fw_domains_init(struct drm_device *dev)
> FORCEWAKE, FORCEWAKE_ACK);
> }
>
> + if (HAS_FPGA_DBG_UNCLAIMED(dev) &&
> + dev_priv->uncore.funcs.force_wake_put == fw_domains_put)
My fear here is that simple changes to the FW code by a future
programmer could unintentionally kill the unclaimed register detection
feature, and we probably wouldn't notice for a looong time. Why not
just omit this fw_domains_put check, since it is true for all
platforms where HAS_FPG_DBG_UNCLAIMED is also true? The side effect of
calling fw_domains_put() when we shouldn't is probably more noticeable
than having unclaimed register detection gone.
> + dev_priv->uncore.funcs.force_wake_put = fw_domains_put_debug;
> +
> /* All future platforms are expected to require complex power gating */
> WARN_ON(dev_priv->uncore.fw_domains == 0);
> }
> @@ -1411,11 +1420,6 @@ int intel_gpu_reset(struct drm_device *dev)
>
> void intel_uncore_check_errors(struct drm_device *dev)
> {
> - struct drm_i915_private *dev_priv = dev->dev_private;
> -
> - if (HAS_FPGA_DBG_UNCLAIMED(dev) &&
> - (__raw_i915_read32(dev_priv, FPGA_DBG) & FPGA_DBG_RM_NOCLAIM)) {
> - DRM_ERROR("Unclaimed register before interrupt\n");
> - __raw_i915_write32(dev_priv, FPGA_DBG, FPGA_DBG_RM_NOCLAIM);
> - }
> + if (HAS_FPGA_DBG_UNCLAIMED(dev))
> + hsw_unclaimed_reg_detect(to_i915(dev));
This means we won't check for unclaimed registers during interrupts if
i915.mmio_debug is being used, which is probably not what we want.
One of the things that worries me a little is that now we'll be
running a mostly-display-related feature only when certain pieces of
non-display-related code run. Instead of doing the checks at the
forcewake puts, maybe we could tie the frequency of the checks to
something in the display code, or just do the check every X seconds. I
don't really know what would be ideal here, I'm just throwing the
ideas. I'm also not blocking this patch, just pointing things that
could maybe be improved.
Since it's impacting performance, perhaps we could even completely
kill unclaimed register detection from the normal use case, hiding it
behind i915.mmio_debug=1 (and maybe a kconfig option)? We would then
instruct QA and developers to always have the option enabled. Just
like QA needs to have lockdep enabled, we could ask it to have
mmio_debug enabled all the time too.
> }
> --
> 2.1.4
>
> _______________________________________________
> 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
