Re: [PATCH 4/4] drm/i915: Reduce frequency of unspecific HSW reg debugging

[Mika Kuoppala <mika.kuoppala@linux.intel.com>]

Daniel Vetter <daniel@ffwll.ch> writes:

> On Thu, Sep 03, 2015 at 04:51:45PM -0300, Paulo Zanoni wrote:
>> From: Chris Wilson <chris@chris-wilson.co.uk>
>> 
>> 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.
>> v3 (from Paulo): rebase since gen9 addition and intel_uncore_check_errors
>>     removal
>> 
>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>> Cc: Mika Kuoppala <mika.kuoppala@intel.com>
>> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>
> I'm unclear how this interacts (or how it sould interact) with patch 2:
> Forcwake is mostly for GT registers, but patch 2 also tries to optimize
> forcwake for GT registers. Do we really need both?

Assuming the hardware detects access to unpowered domains and
to unregistered ranges by setting this bit, I would say that patch 2
is not needed. One could argue that patch 2 is somewhat harmful as
current register access pattern affects the detection.

Also the commit message in patch 2 is not valid wrt the code.

With skl, the debug bit seems to decay with time, instead of being
sticky. So in there we could argue that in patch 4/4, the reading
should be done before (and after) the forcewake scope.

Paulo, have you tried if this bit detects access to unpowered
domain with hsw/bdw?

-Mika

> -Daniel
>
>> ---
>>  drivers/gpu/drm/i915/intel_uncore.c | 52 +++++++++++++++++++++----------------
>>  1 file changed, 29 insertions(+), 23 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
>> index 8844c314..1fe63fc 100644
>> --- a/drivers/gpu/drm/i915/intel_uncore.c
>> +++ b/drivers/gpu/drm/i915/intel_uncore.c
>> @@ -148,6 +148,31 @@ 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_ERROR("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);
>> +}
>> +
>> +static void
>>  fw_domains_posting_read(struct drm_i915_private *dev_priv)
>>  {
>>  	struct intel_uncore_forcewake_domain *d;
>> @@ -627,26 +652,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, u32 reg)
>> -{
>> -	static bool mmio_debug_once = true;
>> -
>> -	if (!UNCLAIMED_CHECK_RANGE(reg))
>> -		return;
>> -
>> -	if (i915.mmio_debug || !mmio_debug_once)
>> -		return;
>> -
>> -	if (__raw_i915_read32(dev_priv, FPGA_DBG) & FPGA_DBG_RM_NOCLAIM) {
>> -		DRM_ERROR("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);
>> @@ -900,7 +905,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, reg); \
>>  	GEN6_WRITE_FOOTER; \
>>  }
>>  
>> @@ -942,7 +946,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, reg); \
>>  	GEN6_WRITE_FOOTER; \
>>  }
>>  
>> @@ -1008,7 +1011,6 @@ gen9_write##x(struct drm_i915_private *dev_priv, off_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, reg); \
>>  	GEN6_WRITE_FOOTER; \
>>  }
>>  
>> @@ -1194,6 +1196,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)
>> +		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);
>>  }
>> -- 
>> 2.5.0
>> 
>
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx