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 07:24 PM, Daniel Vetter wrote:
> On Fri, Jun 13, 2014 at 05:56:41PM +0530, Deepak S wrote:
>> 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
> Please don't put the stepping info in the comment or commit message
> though, that freaks out people ;-) Usually we go with "pre-production" or
> "early revisions" or something non-specific.

Ok Sure :)

>> 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.
> Since this is gt powersave related can you please also check whether we
> shouldn't move this to the intel_suspend_gt_powersave function Jesse
> recently added to -nightly?
>
> Thanks, Daniel