Re: [PATCH] drm/i915: Drop WA to fix Voltage not getting dropped to Vmin when Gfx is power gated.

[Deepak S <deepak.s@linux.intel.com>]

On Friday 13 June 2014 05:27 PM, Ville Syrjälä wrote:
> On Fri, Jun 13, 2014 at 02:33:44PM +0300, Ville Syrjälä wrote:
>> On Fri, Jun 13, 2014 at 03:46:14PM +0530, deepak.s@linux.intel.com wrote:
>>> From: Deepak S <deepak.s@linux.intel.com>
>>>
>>> Workaround fixed in BYT. Forcing Gfx clk up not needed, and Requesting the
>>> min freq should bring bring the voltage Vnn.
>>>
>>> Signed-off-by: Deepak S <deepak.s@linux.intel.com>
>>> ---
>>>   drivers/gpu/drm/i915/intel_pm.c | 40 +---------------------------------------
>>>   1 file changed, 1 insertion(+), 39 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
>>> index 0b088fe..9aee28b 100644
>>> --- a/drivers/gpu/drm/i915/intel_pm.c
>>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>>> @@ -3198,44 +3198,6 @@ void gen6_set_rps(struct drm_device *dev, u8 val)
>>>   	trace_intel_gpu_freq_change(val * 50);
>>>   }
>>>   
>>> -/* vlv_set_rps_idle: Set the frequency to Rpn if Gfx clocks are down
>>> - *
>>> - * * If Gfx is Idle, then
>>> - * 1. Mask Turbo interrupts
>>> - * 2. Bring up Gfx clock
>>> - * 3. Change the freq to Rpn and wait till P-Unit updates freq
>>> - * 4. Clear the Force GFX CLK ON bit so that Gfx can down
>>> - * 5. Unmask Turbo interrupts
>>> -*/
>>> -static void vlv_set_rps_idle(struct drm_i915_private *dev_priv)
>>> -{
>>> -	/*
>>> -	 * When we are idle.  Drop to min voltage state.
>>> -	 */
>>> -
>>> -	if (dev_priv->rps.cur_freq <= dev_priv->rps.min_freq_softlimit)
>>> -		return;
>>> -
>>> -	/* Mask turbo interrupt so that they will not come in between */
>>> -	I915_WRITE(GEN6_PMINTRMSK, 0xffffffff);
>>> -
>>> -	vlv_force_gfx_clock(dev_priv, true);
>>> -
>>> -	dev_priv->rps.cur_freq = dev_priv->rps.min_freq_softlimit;
>>> -
>>> -	vlv_punit_write(dev_priv, PUNIT_REG_GPU_FREQ_REQ,
>>> -					dev_priv->rps.min_freq_softlimit);
>>> -
>>> -	if (wait_for(((vlv_punit_read(dev_priv, PUNIT_REG_GPU_FREQ_STS))
>>> -				& GENFREQSTATUS) == 0, 5))
>>> -		DRM_ERROR("timed out waiting for Punit\n");
>>> -
>>> -	vlv_force_gfx_clock(dev_priv, false);
>>> -
>>> -	I915_WRITE(GEN6_PMINTRMSK,
>>> -		   gen6_rps_pm_mask(dev_priv, dev_priv->rps.cur_freq));
>>> -}
>>> -
>>>   void gen6_rps_idle(struct drm_i915_private *dev_priv)
>>>   {
>>>   	struct drm_device *dev = dev_priv->dev;
>>> @@ -3243,7 +3205,7 @@ void gen6_rps_idle(struct drm_i915_private *dev_priv)
>>>   	mutex_lock(&dev_priv->rps.hw_lock);
>>>   	if (dev_priv->rps.enabled) {
>>>   		if (IS_VALLEYVIEW(dev))
>>> -			vlv_set_rps_idle(dev_priv);
>>> +			valleyview_set_rps(dev_priv->dev, dev_priv->rps.min_freq_softlimit);
>> This should take care of https://bugs.freedesktop.org/show_bug.cgi?id=75244
>>
>> I don't know when the hardware got fixed so I'm hesitant to r-b it, but
>> at least my C0 works fine without this stuff, so:
>> Acked-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>
>> However to avoid future mishaps I think we should have some kind of a
>> comment before the valleyview_set_rps() call to let the reader know that
>> we really need this on VLV to drop the voltage.

hmm, Yes we might need this for other stepping. I will add a comment

Thanks for the review

>> Also it now occurs to me that we might be leaving the GPU frequency (and
>> thus Vnn) high during a system suspend. I think we need an explicit
>> rps_idle() call in the suspend path somewhere. Runtime suspend should be
>> fine already since it depends on intel_mark_idle() getting called before
>> the last rpm reference is dropped.
> Maybe this is all we need?
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 3768199..fabd852 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -4530,7 +4530,7 @@ i915_gem_suspend(struct drm_device *dev)
>   
>          del_timer_sync(&dev_priv->gpu_error.hangcheck_timer);
>          cancel_delayed_work_sync(&dev_priv->mm.retire_work);
> -       cancel_delayed_work_sync(&dev_priv->mm.idle_work);
> +       flush_delayed_work(&dev_priv->mm.idle_work);
>   
>          return 0;

Yes, while suspending we need move GPU to min_freq. flush_delayed_work should be fine. Let me create a patch for this.