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

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

[deepak.s]

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);
 		else
 			gen6_set_rps(dev_priv->dev, dev_priv->rps.min_freq_softlimit);
 		dev_priv->rps.last_adj = 0;
-- 
1.9.1

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

[Ville Syrjälä]

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.

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.

>  		else
>  			gen6_set_rps(dev_priv->dev, dev_priv->rps.min_freq_softlimit);
>  
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC

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

[Ville Syrjälä]

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.
> 
> 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;

-- 
Ville Syrjälä
Intel OTC

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

[Deepak S]

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.

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

[Daniel Vetter]

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.
> 
> 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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

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

[Deepak S]

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

Re: [PATCH v2] drm/i915: Drop WA to fix Voltage not getting dropped to Vmin when Gfx is power gated for latest VLV revision

[Jani Nikula]

On Sat, 28 Jun 2014, deepak.s@linux.intel.com wrote:
> From: Deepak S <deepak.s@linux.intel.com>
>
> Workaround fixed in Latest VLV revision. Forcing Gfx clk up not needed, and Requesting the
> min freq should bring bring the voltage Vnn.
>
> v2: Drop WA for Latest VLV revision (Ville)

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=75244

> Signed-off-by: Deepak S <deepak.s@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index a90fdbd..6b6cfd4 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3212,6 +3212,14 @@ void gen6_set_rps(struct drm_device *dev, u8 val)
>  */
>  static void vlv_set_rps_idle(struct drm_i915_private *dev_priv)
>  {
> +	struct drm_device *dev = dev_priv->dev;
> +
> +	/* Latest VLV doesn't need Vnn WA*/
> +	if (dev->pdev->revision >= 0xd) {
> +		valleyview_set_rps(dev_priv->dev, dev_priv->rps.min_freq_softlimit);
> +		return;
> +	}
> +
>  	/*
>  	 * When we are idle.  Drop to min voltage state.
>  	 */
> -- 
> 1.9.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center

Re: [PATCH v2] drm/i915: Drop WA to fix Voltage not getting dropped to Vmin when Gfx is power gated for latest VLV revision

[Ville Syrjälä]

On Sat, Jun 28, 2014 at 11:26:11AM +0530, deepak.s@linux.intel.com wrote:
> From: Deepak S <deepak.s@linux.intel.com>
> 
> Workaround fixed in Latest VLV revision. Forcing Gfx clk up not needed, and Requesting the
> min freq should bring bring the voltage Vnn.
> 
> v2: Drop WA for Latest VLV revision (Ville)
> 
> Signed-off-by: Deepak S <deepak.s@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index a90fdbd..6b6cfd4 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3212,6 +3212,14 @@ void gen6_set_rps(struct drm_device *dev, u8 val)
>  */
>  static void vlv_set_rps_idle(struct drm_i915_private *dev_priv)
>  {
> +	struct drm_device *dev = dev_priv->dev;
> +
> +	/* Latest VLV doesn't need Vnn WA*/

Maybe this should say "Latest VLV doesn't need to force the gfx clock"
or something like that. We are still doing this to reduce Vnn after all.

Apart from that this matches my observations so:
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> +	if (dev->pdev->revision >= 0xd) {
> +		valleyview_set_rps(dev_priv->dev, dev_priv->rps.min_freq_softlimit);
> +		return;
> +	}
> +
>  	/*
>  	 * When we are idle.  Drop to min voltage state.
>  	 */
> -- 
> 1.9.1

-- 
Ville Syrjälä
Intel OTC

[PATCH v2] drm/i915: Drop WA to fix Voltage not getting dropped to Vmin when Gfx is power gated for latest VLV revision

[deepak.s]

From: Deepak S <deepak.s@linux.intel.com>

Workaround fixed in Latest VLV revision. Forcing Gfx clk up not needed, and Requesting the
min freq should bring bring the voltage Vnn.

v2: Drop WA for Latest VLV revision (Ville)

Signed-off-by: Deepak S <deepak.s@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index a90fdbd..6b6cfd4 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3212,6 +3212,14 @@ void gen6_set_rps(struct drm_device *dev, u8 val)
 */
 static void vlv_set_rps_idle(struct drm_i915_private *dev_priv)
 {
+	struct drm_device *dev = dev_priv->dev;
+
+	/* Latest VLV doesn't need Vnn WA*/
+	if (dev->pdev->revision >= 0xd) {
+		valleyview_set_rps(dev_priv->dev, dev_priv->rps.min_freq_softlimit);
+		return;
+	}
+
 	/*
 	 * When we are idle.  Drop to min voltage state.
 	 */
-- 
1.9.1

Re: [PATCH v2] drm/i915: Drop WA to fix Voltage not getting dropped to Vmin when Gfx is power gated for latest VLV revision

[Jani Nikula]

On Fri, 27 Jun 2014, Jani Nikula <jani.nikula@linux.intel.com> wrote:
> On Sat, 28 Jun 2014, deepak.s@linux.intel.com wrote:
>> From: Deepak S <deepak.s@linux.intel.com>
>>
>> Workaround fixed in Latest VLV revision. Forcing Gfx clk up not needed, and Requesting the
>> min freq should bring bring the voltage Vnn.
>>
>> v2: Drop WA for Latest VLV revision (Ville)
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=75244

Per comments in the bug, scratch that.

Deepak, I presume this patch is needed nonetheless?

BR,
Jani.



>
>> Signed-off-by: Deepak S <deepak.s@linux.intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_pm.c | 8 ++++++++
>>  1 file changed, 8 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
>> index a90fdbd..6b6cfd4 100644
>> --- a/drivers/gpu/drm/i915/intel_pm.c
>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>> @@ -3212,6 +3212,14 @@ void gen6_set_rps(struct drm_device *dev, u8 val)
>>  */
>>  static void vlv_set_rps_idle(struct drm_i915_private *dev_priv)
>>  {
>> +	struct drm_device *dev = dev_priv->dev;
>> +
>> +	/* Latest VLV doesn't need Vnn WA*/
>> +	if (dev->pdev->revision >= 0xd) {
>> +		valleyview_set_rps(dev_priv->dev, dev_priv->rps.min_freq_softlimit);
>> +		return;
>> +	}
>> +
>>  	/*
>>  	 * When we are idle.  Drop to min voltage state.
>>  	 */
>> -- 
>> 1.9.1
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> -- 
> Jani Nikula, Intel Open Source Technology Center
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center

Re: [PATCH v2] drm/i915: Drop WA to fix Voltage not getting dropped to Vmin when Gfx is power gated for latest VLV revision

[Jani Nikula]

On Fri, 27 Jun 2014, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Sat, Jun 28, 2014 at 11:26:11AM +0530, deepak.s@linux.intel.com wrote:
>> From: Deepak S <deepak.s@linux.intel.com>
>> 
>> Workaround fixed in Latest VLV revision. Forcing Gfx clk up not needed, and Requesting the
>> min freq should bring bring the voltage Vnn.
>> 
>> v2: Drop WA for Latest VLV revision (Ville)
>> 
>> Signed-off-by: Deepak S <deepak.s@linux.intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_pm.c | 8 ++++++++
>>  1 file changed, 8 insertions(+)
>> 
>> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
>> index a90fdbd..6b6cfd4 100644
>> --- a/drivers/gpu/drm/i915/intel_pm.c
>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>> @@ -3212,6 +3212,14 @@ void gen6_set_rps(struct drm_device *dev, u8 val)
>>  */
>>  static void vlv_set_rps_idle(struct drm_i915_private *dev_priv)
>>  {
>> +	struct drm_device *dev = dev_priv->dev;
>> +
>> +	/* Latest VLV doesn't need Vnn WA*/
>
> Maybe this should say "Latest VLV doesn't need to force the gfx clock"
> or something like that. We are still doing this to reduce Vnn after all.
>
> Apart from that this matches my observations so:
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Pushed to -fixes with the comment changed and commit message massaged a
bit. Thanks for the patch and review.

BR,
Jani.


>
>> +	if (dev->pdev->revision >= 0xd) {
>> +		valleyview_set_rps(dev_priv->dev, dev_priv->rps.min_freq_softlimit);
>> +		return;
>> +	}
>> +
>>  	/*
>>  	 * When we are idle.  Drop to min voltage state.
>>  	 */
>> -- 
>> 1.9.1
>
> -- 
> Ville Syrjälä
> Intel OTC
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx