Re: [PATCH v2] drm/i915: Bring GPU Freq to min while suspending.

Re: [PATCH v2] drm/i915: Bring GPU Freq to min while suspending.

[Daniel Vetter]

On Wed, Jun 18, 2014 at 05:30:53AM +0530, deepak.s@linux.intel.com wrote:
> From: Deepak S <deepak.s@linux.intel.com>
> 
> We might be leaving the GPU Frequency (and thus vnn) high during the suspend.
> Flush the delayed work queue should take care of this.
> 
> v2: Fixed typo in commit message (Deepak)
> 
> Signed-off-by: Deepak S <deepak.s@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 7f643db..8d5ae82 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -4541,7 +4541,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);

Shouldn't we do that in suspend_gt_powersave instead? Also if we cancel
the retire work the idle work won't necessarily get armed and we might
miss the window. Just forcing the gt to the lowest freq in
suspend_gt_powersave should be more reliable.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH v2] drm/i915: Bring GPU Freq to min while suspending.

[deepak.s]

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

We might be leaving the GPU Frequency (and thus vnn) high during the suspend.
Flush the delayed work queue should take care of this.

v2: Fixed typo in commit message (Deepak)

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

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 7f643db..8d5ae82 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4541,7 +4541,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;
 
-- 
1.9.1

Re: [PATCH v2] drm/i915: Bring GPU Freq to min while suspending.

[Daniel Vetter]

On Thu, Jun 19, 2014 at 08:53:24AM +0530, Deepak S wrote:
> 
> On Wednesday 18 June 2014 03:47 AM, Daniel Vetter wrote:
> >On Wed, Jun 18, 2014 at 05:30:53AM +0530, deepak.s@linux.intel.com wrote:
> >>From: Deepak S <deepak.s@linux.intel.com>
> >>
> >>We might be leaving the GPU Frequency (and thus vnn) high during the suspend.
> >>Flush the delayed work queue should take care of this.
> >>
> >>v2: Fixed typo in commit message (Deepak)
> >>
> >>Signed-off-by: Deepak S <deepak.s@linux.intel.com>
> >>---
> >>  drivers/gpu/drm/i915/i915_gem.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >>diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> >>index 7f643db..8d5ae82 100644
> >>--- a/drivers/gpu/drm/i915/i915_gem.c
> >>+++ b/drivers/gpu/drm/i915/i915_gem.c
> >>@@ -4541,7 +4541,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);
> >Shouldn't we do that in suspend_gt_powersave instead? Also if we cancel
> >the retire work the idle work won't necessarily get armed and we might
> >miss the window. Just forcing the gt to the lowest freq in
> >suspend_gt_powersave should be more reliable.
> >-Daniel
> 
> Since we a calling suspend_gt_powersave after i915_gem_suspend, i added the flush in suspend.

I'm confused: So you're doing the change I've suggested or it doesn't
work? Please unconfuse me ;-)

> Yes i agree forcing the gt freq us more reliable

So new patch or not?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH v2] drm/i915: Bring GPU Freq to min while suspending.

[Deepak S]

On Wednesday 18 June 2014 03:47 AM, Daniel Vetter wrote:
> On Wed, Jun 18, 2014 at 05:30:53AM +0530, deepak.s@linux.intel.com wrote:
>> From: Deepak S <deepak.s@linux.intel.com>
>>
>> We might be leaving the GPU Frequency (and thus vnn) high during the suspend.
>> Flush the delayed work queue should take care of this.
>>
>> v2: Fixed typo in commit message (Deepak)
>>
>> Signed-off-by: Deepak S <deepak.s@linux.intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_gem.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
>> index 7f643db..8d5ae82 100644
>> --- a/drivers/gpu/drm/i915/i915_gem.c
>> +++ b/drivers/gpu/drm/i915/i915_gem.c
>> @@ -4541,7 +4541,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);
> Shouldn't we do that in suspend_gt_powersave instead? Also if we cancel
> the retire work the idle work won't necessarily get armed and we might
> miss the window. Just forcing the gt to the lowest freq in
> suspend_gt_powersave should be more reliable.
> -Daniel

Since we a calling suspend_gt_powersave after i915_gem_suspend, i added the flush in suspend.

Yes i agree forcing the gt freq us more reliable

Re: [PATCH v3] drm/i915: Force GPU Freq to lowest while suspending.

[Daniel Vetter]

On Fri, Jun 20, 2014 at 1:59 PM,  <deepak.s@linux.intel.com> wrote:
>
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 2043c4b..6bbb90b 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -4881,6 +4881,9 @@ void intel_suspend_gt_powersave(struct drm_device *dev)
>         /* Interrupts should be disabled already to avoid re-arming. */
>         WARN_ON(dev->irq_enabled);
>
> +       /* Force GPU to min freq during suspend */
> +       gen6_rps_idle(dev_priv);
> +

Shouldn't this be _after_ we've cancelled the rps works? Otherwise the
work item might sneak in and undo the idling between the idle and work
cancelling.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH v3] drm/i915: Force GPU Freq to lowest while suspending.

[deepak.s]

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

We might be leaving the GPU Frequency (and thus vnn) high during the suspend.
Force gt to move to lowest freq while suspending.

v2: Fixed typo in commit message (Deepak)

v3: Force gt to lowest freq in suspend_gt_powersave (Daniel)

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

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 2043c4b..6bbb90b 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4881,6 +4881,9 @@ void intel_suspend_gt_powersave(struct drm_device *dev)
 	/* Interrupts should be disabled already to avoid re-arming. */
 	WARN_ON(dev->irq_enabled);
 
+	/* Force GPU to min freq during suspend */
+	gen6_rps_idle(dev_priv);
+
 	flush_delayed_work(&dev_priv->rps.delayed_resume_work);
 
 	cancel_work_sync(&dev_priv->rps.work);
-- 
1.9.1

Re: [PATCH v3] drm/i915: Force GPU Freq to lowest while suspending.

[Deepak S]

On Thursday 19 June 2014 06:04 PM, Daniel Vetter wrote:
> On Fri, Jun 20, 2014 at 1:59 PM,  <deepak.s@linux.intel.com> wrote:
>> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
>> index 2043c4b..6bbb90b 100644
>> --- a/drivers/gpu/drm/i915/intel_pm.c
>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>> @@ -4881,6 +4881,9 @@ void intel_suspend_gt_powersave(struct drm_device *dev)
>>          /* Interrupts should be disabled already to avoid re-arming. */
>>          WARN_ON(dev->irq_enabled);
>>
>> +       /* Force GPU to min freq during suspend */
>> +       gen6_rps_idle(dev_priv);
>> +
> Shouldn't this be _after_ we've cancelled the rps works? Otherwise the
> work item might sneak in and undo the idling between the idle and work
> cancelling.
> -Daniel

My mistake sending updated patch, -Deepak

[PATCH v4] drm/i915: Force GPU Freq to lowest while suspending.

[deepak.s]

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

We might be leaving the GPU Frequency (and thus vnn) high during the suspend.
Force gt to move to lowest freq while suspending.

v2: Fixed typo in commit message (Deepak)

v3: Force gt to lowest freq in suspend_gt_powersave (Daniel)

v4: Add GPU min freq set _after_ we've cancelled the rps works (Daniel)

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

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 2043c4b..0543407 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4884,6 +4884,9 @@ void intel_suspend_gt_powersave(struct drm_device *dev)
 	flush_delayed_work(&dev_priv->rps.delayed_resume_work);
 
 	cancel_work_sync(&dev_priv->rps.work);
+
+	/* Force GPU to min freq during suspend */
+	gen6_rps_idle(dev_priv);
 }
 
 void intel_disable_gt_powersave(struct drm_device *dev)
-- 
1.9.1

Re: [PATCH v4] drm/i915: Force GPU Freq to lowest while suspending.

[Jesse Barnes]

On Tue, 15 Jul 2014 13:05:48 +0530
Deepak S <deepak.s@linux.intel.com> wrote:

> 
> On Friday 20 June 2014 08:03 PM, deepak.s@linux.intel.com wrote:
> > From: Deepak S <deepak.s@linux.intel.com>
> >
> > We might be leaving the GPU Frequency (and thus vnn) high during the suspend.
> > Force gt to move to lowest freq while suspending.
> >
> > v2: Fixed typo in commit message (Deepak)
> >
> > v3: Force gt to lowest freq in suspend_gt_powersave (Daniel)
> >
> > v4: Add GPU min freq set _after_ we've cancelled the rps works (Daniel)
> >
> > Signed-off-by: Deepak S <deepak.s@linux.intel.com>
> > ---
> >   drivers/gpu/drm/i915/intel_pm.c | 3 +++
> >   1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index 2043c4b..0543407 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -4884,6 +4884,9 @@ void intel_suspend_gt_powersave(struct drm_device *dev)
> >   	flush_delayed_work(&dev_priv->rps.delayed_resume_work);
> >   
> >   	cancel_work_sync(&dev_priv->rps.work);
> > +
> > +	/* Force GPU to min freq during suspend */
> > +	gen6_rps_idle(dev_priv);
> >   }
> >   
> >   void intel_disable_gt_powersave(struct drm_device *dev)
> 
> Hi Jesse,
> 
> Please review the patch

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH v4] drm/i915: Force GPU Freq to lowest while suspending.

[Deepak S]

On Friday 20 June 2014 08:03 PM, deepak.s@linux.intel.com wrote:
> From: Deepak S <deepak.s@linux.intel.com>
>
> We might be leaving the GPU Frequency (and thus vnn) high during the suspend.
> Force gt to move to lowest freq while suspending.
>
> v2: Fixed typo in commit message (Deepak)
>
> v3: Force gt to lowest freq in suspend_gt_powersave (Daniel)
>
> v4: Add GPU min freq set _after_ we've cancelled the rps works (Daniel)
>
> Signed-off-by: Deepak S <deepak.s@linux.intel.com>
> ---
>   drivers/gpu/drm/i915/intel_pm.c | 3 +++
>   1 file changed, 3 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 2043c4b..0543407 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -4884,6 +4884,9 @@ void intel_suspend_gt_powersave(struct drm_device *dev)
>   	flush_delayed_work(&dev_priv->rps.delayed_resume_work);
>   
>   	cancel_work_sync(&dev_priv->rps.work);
> +
> +	/* Force GPU to min freq during suspend */
> +	gen6_rps_idle(dev_priv);
>   }
>   
>   void intel_disable_gt_powersave(struct drm_device *dev)

Hi Jesse,

Please review the patch

Thanks
Deepak

Re: [PATCH v4] drm/i915: Force GPU Freq to lowest while suspending.

[Deepak S]

On Monday 14 July 2014 08:47 PM, Jesse Barnes wrote:
> On Tue, 15 Jul 2014 13:05:48 +0530
> Deepak S <deepak.s@linux.intel.com> wrote:
>
>> On Friday 20 June 2014 08:03 PM, deepak.s@linux.intel.com wrote:
>>> From: Deepak S <deepak.s@linux.intel.com>
>>>
>>> We might be leaving the GPU Frequency (and thus vnn) high during the suspend.
>>> Force gt to move to lowest freq while suspending.
>>>
>>> v2: Fixed typo in commit message (Deepak)
>>>
>>> v3: Force gt to lowest freq in suspend_gt_powersave (Daniel)
>>>
>>> v4: Add GPU min freq set _after_ we've cancelled the rps works (Daniel)
>>>
>>> Signed-off-by: Deepak S <deepak.s@linux.intel.com>
>>> ---
>>>    drivers/gpu/drm/i915/intel_pm.c | 3 +++
>>>    1 file changed, 3 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
>>> index 2043c4b..0543407 100644
>>> --- a/drivers/gpu/drm/i915/intel_pm.c
>>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>>> @@ -4884,6 +4884,9 @@ void intel_suspend_gt_powersave(struct drm_device *dev)
>>>    	flush_delayed_work(&dev_priv->rps.delayed_resume_work);
>>>    
>>>    	cancel_work_sync(&dev_priv->rps.work);
>>> +
>>> +	/* Force GPU to min freq during suspend */
>>> +	gen6_rps_idle(dev_priv);
>>>    }
>>>    
>>>    void intel_disable_gt_powersave(struct drm_device *dev)
>> Hi Jesse,
>>
>> Please review the patch
> Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
>

Thanks for reviewing