Re: [PATCH 2/4] drm/i915/gen9: Verify and enforce dc6 state writes

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

Imre Deak <imre.deak@intel.com> writes:

> On to, 2016-02-18 at 17:21 +0200, Mika Kuoppala wrote:
>> It has been observed that sometimes disabling the dc6 fails
>> and dc6 state pops back up, brief moment after disabling. This
>> has to be dmc save/restore timing issue or other bug in the
>> way dc states are handled.
>> 
>> Try to work around this issue as we don't have firmware fix
>> yet available. Verify that the value we wrote for the dmc sticks,
>> and also enforce it by rewriting it, if it didn't.
>> 
>> Testcase: kms_flip/basic-flip-vs-dpms
>> References: https://bugs.freedesktop.org/show_bug.cgi?id=93768
>> Cc: Patrik Jakobsson <patrik.jakobsson@linux.intel.com>
>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
>> Cc: Imre Deak <imre.deak@intel.com>
>> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_runtime_pm.c | 40
>> +++++++++++++++++++++++++++++++--
>>  1 file changed, 38 insertions(+), 2 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c
>> b/drivers/gpu/drm/i915/intel_runtime_pm.c
>> index 8b9290fdb3b2..cb91540cfbad 100644
>> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
>> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
>> @@ -470,6 +470,42 @@ static void
>> gen9_set_dc_state_debugmask_memory_up(
>>  	}
>>  }
>>  
>> +static void gen9_write_dc_state(struct drm_i915_private *dev_priv,
>> +				u32 state)
>> +{
>> +	int rewrites = 0;
>> +	int rereads = 0;
>> +	u32 v;
>> +
>> +	I915_WRITE(DC_STATE_EN, state);
>> +
>> +	/* It has been observed that disabling the dc6 state
>> sometimes
>> +	 * doesn't stick and dmc keeps returning old value. Make
>> sure
>> +	 * the write really sticks enough times and also force
>> rewrite until
>> +	 * we are confident that state is exactly what we want.
>> +	 */
>> +	do  {
>> +		v = I915_READ(DC_STATE_EN);
>> +
>> +		if (v != state) {
>> +			I915_WRITE(DC_STATE_EN, state);
>> +			rewrites++;
>
> Could be rereads = 0; for extra paranoia. Either way:

Oh yes, extra paranoia in here is warranted. I will
add that.

> Reviewed-by: Imre Deak <imre.deak@intel.com>

Thanks,
-Mika

>
>> +		} else if (rereads++ > 5) {
>> +			break;
>> +		}
>> +
>> +	} while (rewrites < 100);
>> +
>> +	if (v != state)
>> +		DRM_ERROR("Writing dc state to 0x%x failed, now
>> 0x%x\n",
>> +			  state, v);
>> +
>> +	/* Most of the times we need one retry, avoid spam */
>> +	if (rewrites > 1)
>> +		DRM_DEBUG_KMS("Rewrote dc state to 0x%x %d times\n",
>> +			      state, rewrites);
>> +}
>> +
>>  static void gen9_set_dc_state(struct drm_i915_private *dev_priv,
>> uint32_t state)
>>  {
>>  	uint32_t val;
>> @@ -502,8 +538,8 @@ static void gen9_set_dc_state(struct
>> drm_i915_private *dev_priv, uint32_t state)
>>  
>>  	val &= ~mask;
>>  	val |= state;
>> -	I915_WRITE(DC_STATE_EN, val);
>> -	POSTING_READ(DC_STATE_EN);
>> +
>> +	gen9_write_dc_state(dev_priv, val);
>>  
>>  	dev_priv->csr.dc_state = val & mask;
>>  }
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx