Re: [PATCH 4/7] drm/i915: Do one shot unclaimed mmio detection less frequently

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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@chris-wilson.co.uk>
> Cc: Paulo Zanoni <przanoni@gmail.com>
> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
> ---
>  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@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx