On Tue, Dec 15, 2015 at 04:25:09PM +0200, Mika Kuoppala wrote:
> We have done unclaimed register access check in normal
> (mmio_debug=0) mode once per write. This adds probability
> of finding the exact sequence where we did the bad access, but
> also adds burden to each write.
>
> As we have mmio_debug available for more fine grained analysis,
> give up accuracy of detecting correct spot at the first occurrence
> by doing the one shot detection and arming of mmio_debug in hangcheck
> and in modeset. This removes the write path performance burden.
>
> Note that in hangcheck when we arm the fine grained debug
> facilities, there is no context even if the unclaimed access
> is already set and thus notifying in this point has no added value.
> In contrary, in display path, we add a debug message if the bit
> was set on arming.
>
> Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> Cc: Paulo Zanoni <przanoni@xxxxxxxxx>
> Signed-off-by: Mika Kuoppala <mika.kuoppala@xxxxxxxxx>
> ---
> drivers/gpu/drm/i915/i915_drv.h | 3 +++
> drivers/gpu/drm/i915/i915_irq.c | 10 ++++-----
> drivers/gpu/drm/i915/intel_display.c | 3 +++
> drivers/gpu/drm/i915/intel_uncore.c | 40 +++++++++++++++++++-----------------
> 4 files changed, 32 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 82c43b6..625aae9 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -723,6 +723,8 @@ struct intel_uncore {
> i915_reg_t reg_post;
> u32 val_reset;
> } fw_domain[FW_DOMAIN_ID_COUNT];
> +
> + int unclaimed_mmio_check;
> };
>
> /* Iterate over initialised fw domains */
> @@ -2732,6 +2734,7 @@ extern void intel_uncore_early_sanitize(struct drm_device *dev,
> bool restore_forcewake);
> extern void intel_uncore_init(struct drm_device *dev);
> extern bool intel_uncore_unclaimed_mmio(struct drm_i915_private *dev_priv);
> +extern bool intel_uncore_arm_unclaimed_mmio_detection(struct drm_i915_private *dev_priv);
> extern void intel_uncore_fini(struct drm_device *dev);
> extern void intel_uncore_forcewake_reset(struct drm_device *dev, bool restore);
> const char *intel_uncore_forcewake_domain_to_str(const enum forcewake_domain_id id);
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index a20dc64..725a340 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -2165,11 +2165,6 @@ static irqreturn_t ironlake_irq_handler(int irq, void *arg)
> if (!intel_irqs_enabled(dev_priv))
> return IRQ_NONE;
>
> - /* We get interrupts on unclaimed registers, so check for this before we
> - * do any I915_{READ,WRITE}. */
> - if (intel_uncore_unclaimed_mmio(dev_priv))
> - DRM_ERROR("Unclaimed register before interrupt\n");
> -
> /* disable master interrupt before clearing iir */
> de_ier = I915_READ(DEIER);
> I915_WRITE(DEIER, de_ier & ~DE_MASTER_IRQ_CONTROL);
> @@ -2990,6 +2985,11 @@ static void i915_hangcheck_elapsed(struct work_struct *work)
> if (!i915.enable_hangcheck)
> return;
>
> + /* Periodic arming of mmio_debug if hw detects mmio
> + * access to out of range or to an unpowered block
> + */
> + intel_uncore_arm_unclaimed_mmio_detection(dev_priv);
It's a bit weird to have this here since it'll only detect display
register accesses AFAIK. But I guess it's cheaper that adding a
dedicated timer or anything for it.
> +
> for_each_ring(ring, dev_priv, i) {
> u64 acthd;
> u32 seqno;
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 471f120..3a9229b 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -13545,6 +13545,9 @@ static int intel_atomic_commit(struct drm_device *dev,
>
> drm_atomic_state_free(state);
>
> + if (intel_uncore_arm_unclaimed_mmio_detection(dev_priv))
> + DRM_DEBUG_DRIVER("%s return with unclaimed access\n", __func__);
> +
> return 0;
> }
>
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> index 34b60cb..911f189 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -629,22 +629,6 @@ hsw_unclaimed_reg_debug(struct drm_i915_private *dev_priv,
> }
> }
>
> -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 (check_for_unclaimed_mmio(dev_priv)) {
> - DRM_DEBUG("Unclaimed register detected, "
> - "enabling oneshot unclaimed register reporting. "
> - "Please use i915.mmio_debug=N for more information.\n");
> - i915.mmio_debug = mmio_debug_once--;
> - }
> -}
> -
> #define GEN2_READ_HEADER(x) \
> u##x val = 0; \
> assert_device_not_suspended(dev_priv);
> @@ -924,7 +908,6 @@ hsw_write##x(struct drm_i915_private *dev_priv, i915_reg_t reg, u##x val, bool t
> gen6_gt_check_fifodbg(dev_priv); \
> } \
> hsw_unclaimed_reg_debug(dev_priv, reg, false, false); \
> - hsw_unclaimed_reg_detect(dev_priv); \
> GEN6_WRITE_FOOTER; \
> }
>
> @@ -959,7 +942,6 @@ gen8_write##x(struct drm_i915_private *dev_priv, i915_reg_t reg, u##x val, bool
> __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; \
> }
>
> @@ -1029,7 +1011,6 @@ gen9_write##x(struct drm_i915_private *dev_priv, i915_reg_t reg, u##x val, \
> __force_wake_get(dev_priv, fw_engine); \
> __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; \
> }
>
> @@ -1249,6 +1230,8 @@ void intel_uncore_init(struct drm_device *dev)
> intel_uncore_fw_domains_init(dev);
> __intel_uncore_early_sanitize(dev, false);
>
> + dev_priv->uncore.unclaimed_mmio_check = 1;
> +
> switch (INTEL_INFO(dev)->gen) {
> default:
> case 9:
> @@ -1610,3 +1593,22 @@ bool intel_uncore_unclaimed_mmio(struct drm_i915_private *dev_priv)
> {
> return check_for_unclaimed_mmio(dev_priv);
> }
> +
> +bool
> +intel_uncore_arm_unclaimed_mmio_detection(struct drm_i915_private *dev_priv)
> +{
> + if (unlikely(i915.mmio_debug ||
> + dev_priv->uncore.unclaimed_mmio_check <= 0))
> + return false;
> +
> + if (unlikely(intel_uncore_unclaimed_mmio(dev_priv))) {
> + DRM_DEBUG("Unclaimed register detected, "
> + "enabling oneshot unclaimed register reporting. "
> + "Please use i915.mmio_debug=N for more information.\n");
> + i915.mmio_debug++;
> + dev_priv->uncore.unclaimed_mmio_check--;
> + return true;
> + }
> +
> + return false;
> +}
> --
> 2.5.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
