[Intel-gfx] [PATCH 7/7] drm/i915/chv: Add non claimed mmio checking

[Mika Kuoppala]

Ville Syrjälä <ville.syrjala at linux.intel.com> writes:

> On Tue, Dec 15, 2015 at 04:25:12PM +0200, Mika Kuoppala wrote:
>> Imre mentioned that chv might also have capability to
>> track unclaimed mmio accesses. And it has, so take it
>> into use.
>
> VLV too.
>
> My old patch:
> http://lists.freedesktop.org/archives/intel-gfx/2013-May/027599.html
>
>> 
>> Cc: Imre Deak <imre.deak at intel.com>
>> Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
>> Signed-off-by: Mika Kuoppala <mika.kuoppala at intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_reg.h     |  4 ++++
>>  drivers/gpu/drm/i915/intel_uncore.c | 34 ++++++++++++++++++++++++++++++----
>>  2 files changed, 34 insertions(+), 4 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>> index 007ae83..24686ab 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -1711,6 +1711,10 @@ enum skl_disp_power_wells {
>>  #define FPGA_DBG		_MMIO(0x42300)
>>  #define   FPGA_DBG_RM_NOCLAIM	(1<<31)
>>  
>> +#define CLAIM_ER		_MMIO(0x182028)
>> +#define   CLAIM_ER_CLR		(1<<31)
>
> Looks like you forgot the overflow bit.
>
>> +#define   CLAIM_ER_CTR_MASK	(0xffff)
>
> Needless parens.
>
>> +
>>  #define DERRMR		_MMIO(0x44050)
>>  /* Note that HBLANK events are reserved on bdw+ */
>>  #define   DERRMR_PIPEA_SCANLINE		(1<<0)
>> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
>> index 02bad32..ea8fcd4 100644
>> --- a/drivers/gpu/drm/i915/intel_uncore.c
>> +++ b/drivers/gpu/drm/i915/intel_uncore.c
>> @@ -335,13 +335,10 @@ static void intel_uncore_ellc_detect(struct drm_device *dev)
>>  }
>>  
>>  static bool
>> -check_for_unclaimed_mmio(struct drm_i915_private *dev_priv)
>> +fpga_check_for_unclaimed_mmio(struct drm_i915_private *dev_priv)
>>  {
>>  	u32 dbg;
>>  
>> -	if (!HAS_FPGA_DBG_UNCLAIMED(dev_priv))
>> -		return false;
>> -
>>  	dbg = __raw_i915_read32(dev_priv, FPGA_DBG);
>>  	if (likely(!(dbg & FPGA_DBG_RM_NOCLAIM)))
>>  		return false;
>> @@ -354,6 +351,35 @@ check_for_unclaimed_mmio(struct drm_i915_private *dev_priv)
>>  	return true;
>>  }
>>  
>> +static bool
>> +chv_check_for_unclaimed_mmio(struct drm_i915_private *dev_priv)
>> +{
>> +	u32 cer;
>> +
>> +	cer = __raw_i915_read32(dev_priv, CLAIM_ER);
>> +	if (likely(!(cer & CLAIM_ER_CTR_MASK)))
>> +		return false;
>> +
>> +	__raw_i915_write32(dev_priv, CLAIM_ER, CLAIM_ER_CLR);
>> +
>> +	/* We want to clear it asap, so post */
>> +	__raw_i915_read32(dev_priv, CLAIM_ER);
>
> __raw_posting_read()
>
> But not sure why we'd want to do this read really. Seems like pointless
> overhead.
>

I saw the bit stick for a short time even after write. That lead
me to think that perhaps the detection logic in hw rearms
based on this bit. So by flushing it quickly we dont coalesce
the potential next write into the previous detect.

Also as this is now in the slow path (mmio_debug==0),
the extra overhead should not be an issue.

But pointless overhead is, pointless. My worries about coalescing
the detect bit might be theoretical at best, so I dont mind removing
the posting.

-Mika

> The other thing is that this only detect unclaimed RM cycles, so display
> registers only. Might be nice to only do the checks when accessing
> display registers so that we don't add overhead to the GT stuff.
> I think the same holds for HSW+ actually.
>
>> +
>> +	return true;
>> +}
>> +
>> +static bool
>> +check_for_unclaimed_mmio(struct drm_i915_private *dev_priv)
>> +{
>> +	if (HAS_FPGA_DBG_UNCLAIMED(dev_priv))
>> +		return fpga_check_for_unclaimed_mmio(dev_priv);
>> +
>> +	if (IS_CHERRYVIEW(dev_priv))
>> +		return chv_check_for_unclaimed_mmio(dev_priv);
>> +
>> +	return false;
>> +}
>> +
>>  static void __intel_uncore_early_sanitize(struct drm_device *dev,
>>  					  bool restore_forcewake)
>>  {
>> -- 
>> 2.5.0
>
> -- 
> Ville Syrjälä
> Intel OTC