[PATCH] drm/i915: fix up ilk rc6 disabling confusion

[PATCH] drm/i915: fix up ilk rc6 disabling confusion

[Daniel Vetter]

While creating the new enable/disable_gt_powersave functions in

commit 8090c6b9daa04dda649ac0a2209601042abfb0a4
Author: Daniel Vetter <daniel.vetter@ffwll.ch>
Date:   Sun Jun 24 16:42:32 2012 +0200

    drm/i915: wrap up gt powersave enabling functions

I've botched up the handling of ironlake_disable_rc6. Fix this up by
calling it at the right place. Note though that ironlake_disable_rc6
does a bit more than just disabling rc6 - it also tears down all the
allocated context objects.

Hence we need to move intel_teardown_rc6 out and directly call it from
intel_modeset_cleanup.

Also properly mark ironlake_enable_rc6 as static and kill the un-used
declaration in i915_drv.h.

Cc: Ben Widawsky <ben@bwidawsk.net>
Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_drv.h      |    1 -
 drivers/gpu/drm/i915/intel_display.c |    2 ++
 drivers/gpu/drm/i915/intel_drv.h     |    1 +
 drivers/gpu/drm/i915/intel_pm.c      |   13 +++++++------
 4 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index a0c15ab..6c077e9 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1510,7 +1510,6 @@ extern bool intel_fbc_enabled(struct drm_device *dev);
 extern void intel_disable_fbc(struct drm_device *dev);
 extern bool ironlake_set_drps(struct drm_device *dev, u8 val);
 extern void ironlake_init_pch_refclk(struct drm_device *dev);
-extern void ironlake_enable_rc6(struct drm_device *dev);
 extern void gen6_set_rps(struct drm_device *dev, u8 val);
 extern void intel_detect_pch(struct drm_device *dev);
 extern int intel_trans_dp_port_sel(struct drm_crtc *crtc);
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 3fbc802..717b553 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -7270,6 +7270,8 @@ void intel_modeset_cleanup(struct drm_device *dev)
 
 	intel_disable_gt_powersave(dev);
 
+	ironlake_teardown_rc6(dev);
+
 	if (IS_VALLEYVIEW(dev))
 		vlv_init_dpio(dev);
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index cc1573b..db226c5 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -493,6 +493,7 @@ extern void intel_gpu_ips_teardown(void);
 
 extern void intel_enable_gt_powersave(struct drm_device *dev);
 extern void intel_disable_gt_powersave(struct drm_device *dev);
+extern void ironlake_teardown_rc6(struct drm_device *dev);
 
 extern void intel_ddi_dpms(struct drm_encoder *encoder, int mode);
 extern void intel_ddi_mode_set(struct drm_encoder *encoder,
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 99bc1f3..ec25eca 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -2547,7 +2547,7 @@ static void gen6_update_ring_freq(struct drm_device *dev)
 	}
 }
 
-static void ironlake_teardown_rc6(struct drm_device *dev)
+void ironlake_teardown_rc6(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
@@ -2564,7 +2564,7 @@ static void ironlake_teardown_rc6(struct drm_device *dev)
 	}
 }
 
-void ironlake_disable_rc6(struct drm_device *dev)
+static void ironlake_disable_rc6(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
@@ -2580,8 +2580,6 @@ void ironlake_disable_rc6(struct drm_device *dev)
 		I915_WRITE(RSTDBYCTL, I915_READ(RSTDBYCTL) & ~RCX_SW_EXIT);
 		POSTING_READ(RSTDBYCTL);
 	}
-
-	ironlake_teardown_rc6(dev);
 }
 
 static int ironlake_setup_rc6(struct drm_device *dev)
@@ -2603,7 +2601,7 @@ static int ironlake_setup_rc6(struct drm_device *dev)
 	return 0;
 }
 
-void ironlake_enable_rc6(struct drm_device *dev)
+static void ironlake_enable_rc6(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_ring_buffer *ring = &dev_priv->ring[RCS];
@@ -3225,8 +3223,11 @@ static void intel_init_emon(struct drm_device *dev)
 
 void intel_disable_gt_powersave(struct drm_device *dev)
 {
-	if (IS_IRONLAKE_M(dev))
+	if (IS_IRONLAKE_M(dev)) {
 		ironlake_disable_drps(dev);
+		ironlake_disable_rc6(dev);
+	}
+
 	if (INTEL_INFO(dev)->gen >= 6 && !IS_VALLEYVIEW(dev))
 		gen6_disable_rps(dev);
 }
-- 
1.7.7.6

Re: [PATCH] drm/i915: fix up ilk rc6 disabling confusion

[Eugeni Dodonov]

On 06/29/2012 06:32 PM, Daniel Vetter wrote:
> @@ -7270,6 +7270,8 @@ void intel_modeset_cleanup(struct drm_device *dev)
>  
>  	intel_disable_gt_powersave(dev);
>  
> +	ironlake_teardown_rc6(dev);
> +

Couldn't we pass a true/false parameter to the
intel_disable_gt_powersave for tearing down ironlake RC6 directly instead?

Something like
intel_disable_gt_powersave(struct drm_device *dev, bool teardown_context);

maybe?

Apart from that,
Reviewed-by: Eugeni Dodonov <eugeni.dodonov@intel.com>

Eugeni

Re: [PATCH] drm/i915: fix up ilk rc6 disabling confusion

[Daniel Vetter]

On Fri, Jun 29, 2012 at 08:36:14PM -0300, Eugeni Dodonov wrote:
> On 06/29/2012 06:32 PM, Daniel Vetter wrote:
> > @@ -7270,6 +7270,8 @@ void intel_modeset_cleanup(struct drm_device *dev)
> >  
> >  	intel_disable_gt_powersave(dev);
> >  
> > +	ironlake_teardown_rc6(dev);
> > +
> 
> Couldn't we pass a true/false parameter to the
> intel_disable_gt_powersave for tearing down ironlake RC6 directly instead?
> 
> Something like
> intel_disable_gt_powersave(struct drm_device *dev, bool teardown_context);
> 
> maybe?

Well, I want to separate setup/teardown code that we only run at driver
load and unload time from hw enable/disable code that we also need to run
around suspend/resume. So no, I explicitly don't want such convoluted
stuff any more. Similarly for code that checks at resume time whether
things are set up already, e.g. the ringbuffer stuff or contexts.
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH] drm/i915: fix up ilk rc6 disabling confusion

[Ben Widawsky]

On Fri, 29 Jun 2012 23:32:16 +0200
Daniel Vetter <daniel.vetter@ffwll.ch> wrote:

> While creating the new enable/disable_gt_powersave functions in
> 
> commit 8090c6b9daa04dda649ac0a2209601042abfb0a4
> Author: Daniel Vetter <daniel.vetter@ffwll.ch>
> Date:   Sun Jun 24 16:42:32 2012 +0200
> 
>     drm/i915: wrap up gt powersave enabling functions
> 
> I've botched up the handling of ironlake_disable_rc6. Fix this up by
> calling it at the right place. Note though that ironlake_disable_rc6
> does a bit more than just disabling rc6 - it also tears down all the
> allocated context objects.

I can only hope that one day you will appreciate the API I set up in
the context code to handle this.

> 
> Hence we need to move intel_teardown_rc6 out and directly call it from
> intel_modeset_cleanup.

I don't mind that you've done this, however could you explain/modify the
comment when we want to disable without destroying the backing context
object?

> 
> Also properly mark ironlake_enable_rc6 as static and kill the un-used
> declaration in i915_drv.h.
> 
> Cc: Ben Widawsky <ben@bwidawsk.net>
> Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Reviewed-by: Ben Widawsky <ben@bwidawsk.net>

> ---
>  drivers/gpu/drm/i915/i915_drv.h      |    1 -
>  drivers/gpu/drm/i915/intel_display.c |    2 ++
>  drivers/gpu/drm/i915/intel_drv.h     |    1 +
>  drivers/gpu/drm/i915/intel_pm.c      |   13 +++++++------
>  4 files changed, 10 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index a0c15ab..6c077e9 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1510,7 +1510,6 @@ extern bool intel_fbc_enabled(struct drm_device *dev);
>  extern void intel_disable_fbc(struct drm_device *dev);
>  extern bool ironlake_set_drps(struct drm_device *dev, u8 val);
>  extern void ironlake_init_pch_refclk(struct drm_device *dev);
> -extern void ironlake_enable_rc6(struct drm_device *dev);
>  extern void gen6_set_rps(struct drm_device *dev, u8 val);
>  extern void intel_detect_pch(struct drm_device *dev);
>  extern int intel_trans_dp_port_sel(struct drm_crtc *crtc);
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 3fbc802..717b553 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -7270,6 +7270,8 @@ void intel_modeset_cleanup(struct drm_device *dev)
>  
>  	intel_disable_gt_powersave(dev);
>  
> +	ironlake_teardown_rc6(dev);
> +
>  	if (IS_VALLEYVIEW(dev))
>  		vlv_init_dpio(dev);
>  
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index cc1573b..db226c5 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -493,6 +493,7 @@ extern void intel_gpu_ips_teardown(void);
>  
>  extern void intel_enable_gt_powersave(struct drm_device *dev);
>  extern void intel_disable_gt_powersave(struct drm_device *dev);
> +extern void ironlake_teardown_rc6(struct drm_device *dev);
>  
>  extern void intel_ddi_dpms(struct drm_encoder *encoder, int mode);
>  extern void intel_ddi_mode_set(struct drm_encoder *encoder,
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 99bc1f3..ec25eca 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -2547,7 +2547,7 @@ static void gen6_update_ring_freq(struct drm_device *dev)
>  	}
>  }
>  
> -static void ironlake_teardown_rc6(struct drm_device *dev)
> +void ironlake_teardown_rc6(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  
> @@ -2564,7 +2564,7 @@ static void ironlake_teardown_rc6(struct drm_device *dev)
>  	}
>  }
>  
> -void ironlake_disable_rc6(struct drm_device *dev)
> +static void ironlake_disable_rc6(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  
> @@ -2580,8 +2580,6 @@ void ironlake_disable_rc6(struct drm_device *dev)
>  		I915_WRITE(RSTDBYCTL, I915_READ(RSTDBYCTL) & ~RCX_SW_EXIT);
>  		POSTING_READ(RSTDBYCTL);
>  	}
> -
> -	ironlake_teardown_rc6(dev);
>  }
>  
>  static int ironlake_setup_rc6(struct drm_device *dev)
> @@ -2603,7 +2601,7 @@ static int ironlake_setup_rc6(struct drm_device *dev)
>  	return 0;
>  }
>  
> -void ironlake_enable_rc6(struct drm_device *dev)
> +static void ironlake_enable_rc6(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_ring_buffer *ring = &dev_priv->ring[RCS];
> @@ -3225,8 +3223,11 @@ static void intel_init_emon(struct drm_device *dev)
>  
>  void intel_disable_gt_powersave(struct drm_device *dev)
>  {
> -	if (IS_IRONLAKE_M(dev))
> +	if (IS_IRONLAKE_M(dev)) {
>  		ironlake_disable_drps(dev);
> +		ironlake_disable_rc6(dev);
> +	}
> +
>  	if (INTEL_INFO(dev)->gen >= 6 && !IS_VALLEYVIEW(dev))
>  		gen6_disable_rps(dev);
>  }


-- 
Ben Widawsky, Intel Open Source Technology Center

Re: [PATCH] drm/i915: fix up ilk rc6 disabling confusion

[Daniel Vetter]

On Sun, Jul 1, 2012 at 5:04 AM, Ben Widawsky <ben@bwidawsk.net> wrote:
> On Fri, 29 Jun 2012 23:32:16 +0200
> Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
>
>> While creating the new enable/disable_gt_powersave functions in
>>
>> commit 8090c6b9daa04dda649ac0a2209601042abfb0a4
>> Author: Daniel Vetter <daniel.vetter@ffwll.ch>
>> Date:   Sun Jun 24 16:42:32 2012 +0200
>>
>>     drm/i915: wrap up gt powersave enabling functions
>>
>> I've botched up the handling of ironlake_disable_rc6. Fix this up by
>> calling it at the right place. Note though that ironlake_disable_rc6
>> does a bit more than just disabling rc6 - it also tears down all the
>> allocated context objects.
>
> I can only hope that one day you will appreciate the API I set up in
> the context code to handle this.

Ffs I'd like to merge this for the renderctx, my gripes have only been
with shoehorning the powerctx stuff into your ctx code. That one
simply doesn't make sense to me.

>> Hence we need to move intel_teardown_rc6 out and directly call it from
>> intel_modeset_cleanup.
>
> I don't mind that you've done this, however could you explain/modify the
> comment when we want to disable without destroying the backing context
> object?

Hm, I don't see the comment you're talking about ... Neither
ironlake_disable_rc6 nor ironlake_teardown_rc6 nor any of the callers
have one. Or am I totally missing your point?
-Daniel

>
>>
>> Also properly mark ironlake_enable_rc6 as static and kill the un-used
>> declaration in i915_drv.h.
>>
>> Cc: Ben Widawsky <ben@bwidawsk.net>
>> Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
>
>> ---
>>  drivers/gpu/drm/i915/i915_drv.h      |    1 -
>>  drivers/gpu/drm/i915/intel_display.c |    2 ++
>>  drivers/gpu/drm/i915/intel_drv.h     |    1 +
>>  drivers/gpu/drm/i915/intel_pm.c      |   13 +++++++------
>>  4 files changed, 10 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index a0c15ab..6c077e9 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -1510,7 +1510,6 @@ extern bool intel_fbc_enabled(struct drm_device *dev);
>>  extern void intel_disable_fbc(struct drm_device *dev);
>>  extern bool ironlake_set_drps(struct drm_device *dev, u8 val);
>>  extern void ironlake_init_pch_refclk(struct drm_device *dev);
>> -extern void ironlake_enable_rc6(struct drm_device *dev);
>>  extern void gen6_set_rps(struct drm_device *dev, u8 val);
>>  extern void intel_detect_pch(struct drm_device *dev);
>>  extern int intel_trans_dp_port_sel(struct drm_crtc *crtc);
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 3fbc802..717b553 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -7270,6 +7270,8 @@ void intel_modeset_cleanup(struct drm_device *dev)
>>
>>       intel_disable_gt_powersave(dev);
>>
>> +     ironlake_teardown_rc6(dev);
>> +
>>       if (IS_VALLEYVIEW(dev))
>>               vlv_init_dpio(dev);
>>
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index cc1573b..db226c5 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -493,6 +493,7 @@ extern void intel_gpu_ips_teardown(void);
>>
>>  extern void intel_enable_gt_powersave(struct drm_device *dev);
>>  extern void intel_disable_gt_powersave(struct drm_device *dev);
>> +extern void ironlake_teardown_rc6(struct drm_device *dev);
>>
>>  extern void intel_ddi_dpms(struct drm_encoder *encoder, int mode);
>>  extern void intel_ddi_mode_set(struct drm_encoder *encoder,
>> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
>> index 99bc1f3..ec25eca 100644
>> --- a/drivers/gpu/drm/i915/intel_pm.c
>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>> @@ -2547,7 +2547,7 @@ static void gen6_update_ring_freq(struct drm_device *dev)
>>       }
>>  }
>>
>> -static void ironlake_teardown_rc6(struct drm_device *dev)
>> +void ironlake_teardown_rc6(struct drm_device *dev)
>>  {
>>       struct drm_i915_private *dev_priv = dev->dev_private;
>>
>> @@ -2564,7 +2564,7 @@ static void ironlake_teardown_rc6(struct drm_device *dev)
>>       }
>>  }
>>
>> -void ironlake_disable_rc6(struct drm_device *dev)
>> +static void ironlake_disable_rc6(struct drm_device *dev)
>>  {
>>       struct drm_i915_private *dev_priv = dev->dev_private;
>>
>> @@ -2580,8 +2580,6 @@ void ironlake_disable_rc6(struct drm_device *dev)
>>               I915_WRITE(RSTDBYCTL, I915_READ(RSTDBYCTL) & ~RCX_SW_EXIT);
>>               POSTING_READ(RSTDBYCTL);
>>       }
>> -
>> -     ironlake_teardown_rc6(dev);
>>  }
>>
>>  static int ironlake_setup_rc6(struct drm_device *dev)
>> @@ -2603,7 +2601,7 @@ static int ironlake_setup_rc6(struct drm_device *dev)
>>       return 0;
>>  }
>>
>> -void ironlake_enable_rc6(struct drm_device *dev)
>> +static void ironlake_enable_rc6(struct drm_device *dev)
>>  {
>>       struct drm_i915_private *dev_priv = dev->dev_private;
>>       struct intel_ring_buffer *ring = &dev_priv->ring[RCS];
>> @@ -3225,8 +3223,11 @@ static void intel_init_emon(struct drm_device *dev)
>>
>>  void intel_disable_gt_powersave(struct drm_device *dev)
>>  {
>> -     if (IS_IRONLAKE_M(dev))
>> +     if (IS_IRONLAKE_M(dev)) {
>>               ironlake_disable_drps(dev);
>> +             ironlake_disable_rc6(dev);
>> +     }
>> +
>>       if (INTEL_INFO(dev)->gen >= 6 && !IS_VALLEYVIEW(dev))
>>               gen6_disable_rps(dev);
>>  }
>
>
> --
> Ben Widawsky, Intel Open Source Technology Center



-- 
Daniel Vetter
daniel.vetter@ffwll.ch - +41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/i915: fix up ilk rc6 disabling confusion

[Ben Widawsky]

On Sun, 1 Jul 2012 12:48:59 +0200
Daniel Vetter <daniel.vetter@ffwll.ch> wrote:

> On Sun, Jul 1, 2012 at 5:04 AM, Ben Widawsky <ben@bwidawsk.net> wrote:
> > On Fri, 29 Jun 2012 23:32:16 +0200
> > Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> >
> >> While creating the new enable/disable_gt_powersave functions in
> >>
> >> commit 8090c6b9daa04dda649ac0a2209601042abfb0a4
> >> Author: Daniel Vetter <daniel.vetter@ffwll.ch>
> >> Date:   Sun Jun 24 16:42:32 2012 +0200
> >>
> >>     drm/i915: wrap up gt powersave enabling functions
> >>
> >> I've botched up the handling of ironlake_disable_rc6. Fix this up by
> >> calling it at the right place. Note though that ironlake_disable_rc6
> >> does a bit more than just disabling rc6 - it also tears down all the
> >> allocated context objects.
> >
> > I can only hope that one day you will appreciate the API I set up in
> > the context code to handle this.
> 
> Ffs I'd like to merge this for the renderctx, my gripes have only been
> with shoehorning the powerctx stuff into your ctx code. That one
> simply doesn't make sense to me.

meh, it was a cheap shot that didn't merit a response ;-)

> 
> >> Hence we need to move intel_teardown_rc6 out and directly call it from
> >> intel_modeset_cleanup.
> >
> > I don't mind that you've done this, however could you explain/modify the
> > comment when we want to disable without destroying the backing context
> > object?
> 
> Hm, I don't see the comment you're talking about ... Neither
> ironlake_disable_rc6 nor ironlake_teardown_rc6 nor any of the callers
> have one. Or am I totally missing your point?
> -Daniel
> 

iirc in the past, disabling rc6 mean disable, and destroy the object.
Now it simply means disable. If that's the case, really just a comment
in the commit would have made me happy. If it's not the case, ignore.

> >
> >>
> >> Also properly mark ironlake_enable_rc6 as static and kill the un-used
> >> declaration in i915_drv.h.
> >>
> >> Cc: Ben Widawsky <ben@bwidawsk.net>
> >> Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Reviewed-by: Ben Widawsky <ben@bwidawsk.net>

Again,
Reviewed-by: Ben Widawsky <ben@bwidawsk.net>

> >
> >> ---
> >>  drivers/gpu/drm/i915/i915_drv.h      |    1 -
> >>  drivers/gpu/drm/i915/intel_display.c |    2 ++
> >>  drivers/gpu/drm/i915/intel_drv.h     |    1 +
> >>  drivers/gpu/drm/i915/intel_pm.c      |   13 +++++++------
> >>  4 files changed, 10 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> >> index a0c15ab..6c077e9 100644
> >> --- a/drivers/gpu/drm/i915/i915_drv.h
> >> +++ b/drivers/gpu/drm/i915/i915_drv.h
> >> @@ -1510,7 +1510,6 @@ extern bool intel_fbc_enabled(struct drm_device *dev);
> >>  extern void intel_disable_fbc(struct drm_device *dev);
> >>  extern bool ironlake_set_drps(struct drm_device *dev, u8 val);
> >>  extern void ironlake_init_pch_refclk(struct drm_device *dev);
> >> -extern void ironlake_enable_rc6(struct drm_device *dev);
> >>  extern void gen6_set_rps(struct drm_device *dev, u8 val);
> >>  extern void intel_detect_pch(struct drm_device *dev);
> >>  extern int intel_trans_dp_port_sel(struct drm_crtc *crtc);
> >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >> index 3fbc802..717b553 100644
> >> --- a/drivers/gpu/drm/i915/intel_display.c
> >> +++ b/drivers/gpu/drm/i915/intel_display.c
> >> @@ -7270,6 +7270,8 @@ void intel_modeset_cleanup(struct drm_device *dev)
> >>
> >>       intel_disable_gt_powersave(dev);
> >>
> >> +     ironlake_teardown_rc6(dev);
> >> +
> >>       if (IS_VALLEYVIEW(dev))
> >>               vlv_init_dpio(dev);
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> >> index cc1573b..db226c5 100644
> >> --- a/drivers/gpu/drm/i915/intel_drv.h
> >> +++ b/drivers/gpu/drm/i915/intel_drv.h
> >> @@ -493,6 +493,7 @@ extern void intel_gpu_ips_teardown(void);
> >>
> >>  extern void intel_enable_gt_powersave(struct drm_device *dev);
> >>  extern void intel_disable_gt_powersave(struct drm_device *dev);
> >> +extern void ironlake_teardown_rc6(struct drm_device *dev);
> >>
> >>  extern void intel_ddi_dpms(struct drm_encoder *encoder, int mode);
> >>  extern void intel_ddi_mode_set(struct drm_encoder *encoder,
> >> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> >> index 99bc1f3..ec25eca 100644
> >> --- a/drivers/gpu/drm/i915/intel_pm.c
> >> +++ b/drivers/gpu/drm/i915/intel_pm.c
> >> @@ -2547,7 +2547,7 @@ static void gen6_update_ring_freq(struct drm_device *dev)
> >>       }
> >>  }
> >>
> >> -static void ironlake_teardown_rc6(struct drm_device *dev)
> >> +void ironlake_teardown_rc6(struct drm_device *dev)
> >>  {
> >>       struct drm_i915_private *dev_priv = dev->dev_private;
> >>
> >> @@ -2564,7 +2564,7 @@ static void ironlake_teardown_rc6(struct drm_device *dev)
> >>       }
> >>  }
> >>
> >> -void ironlake_disable_rc6(struct drm_device *dev)
> >> +static void ironlake_disable_rc6(struct drm_device *dev)
> >>  {
> >>       struct drm_i915_private *dev_priv = dev->dev_private;
> >>
> >> @@ -2580,8 +2580,6 @@ void ironlake_disable_rc6(struct drm_device *dev)
> >>               I915_WRITE(RSTDBYCTL, I915_READ(RSTDBYCTL) & ~RCX_SW_EXIT);
> >>               POSTING_READ(RSTDBYCTL);
> >>       }
> >> -
> >> -     ironlake_teardown_rc6(dev);
> >>  }
> >>
> >>  static int ironlake_setup_rc6(struct drm_device *dev)
> >> @@ -2603,7 +2601,7 @@ static int ironlake_setup_rc6(struct drm_device *dev)
> >>       return 0;
> >>  }
> >>
> >> -void ironlake_enable_rc6(struct drm_device *dev)
> >> +static void ironlake_enable_rc6(struct drm_device *dev)
> >>  {
> >>       struct drm_i915_private *dev_priv = dev->dev_private;
> >>       struct intel_ring_buffer *ring = &dev_priv->ring[RCS];
> >> @@ -3225,8 +3223,11 @@ static void intel_init_emon(struct drm_device *dev)
> >>
> >>  void intel_disable_gt_powersave(struct drm_device *dev)
> >>  {
> >> -     if (IS_IRONLAKE_M(dev))
> >> +     if (IS_IRONLAKE_M(dev)) {
> >>               ironlake_disable_drps(dev);
> >> +             ironlake_disable_rc6(dev);
> >> +     }
> >> +
> >>       if (INTEL_INFO(dev)->gen >= 6 && !IS_VALLEYVIEW(dev))
> >>               gen6_disable_rps(dev);
> >>  }
> >
> >
> > --
> > Ben Widawsky, Intel Open Source Technology Center
> 
> 
> 



-- 
Ben Widawsky, Intel Open Source Technology Center

Re: [PATCH] drm/i915: fix up ilk rc6 disabling confusion

[Daniel Vetter]

On Mon, Jul 02, 2012 at 08:44:00AM -0700, Ben Widawsky wrote:
> On Sun, 1 Jul 2012 12:48:59 +0200
> Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> > Hm, I don't see the comment you're talking about ... Neither
> > ironlake_disable_rc6 nor ironlake_teardown_rc6 nor any of the callers
> > have one. Or am I totally missing your point?
> > -Daniel
> > 
> 
> iirc in the past, disabling rc6 mean disable, and destroy the object.
> Now it simply means disable. If that's the case, really just a comment
> in the commit would have made me happy. If it's not the case, ignore.

I think in the past disabl_rc6 meant disable&destroy on ilk, but disable
but not destroy on snb. I'll add a comment to the commit message to
mention that.
> 
> > >
> > >>
> > >> Also properly mark ironlake_enable_rc6 as static and kill the un-used
> > >> declaration in i915_drv.h.
> > >>
> > >> Cc: Ben Widawsky <ben@bwidawsk.net>
> > >> Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
> 
> Again,
> Reviewed-by: Ben Widawsky <ben@bwidawsk.net>

Thanks, Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH] drm/i915: fix up ilk rc6 disabling confusion

[Daniel Vetter]

On Sat, Jun 30, 2012 at 08:04:44PM -0700, Ben Widawsky wrote:
> On Fri, 29 Jun 2012 23:32:16 +0200
> Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> 
> > While creating the new enable/disable_gt_powersave functions in
> > 
> > commit 8090c6b9daa04dda649ac0a2209601042abfb0a4
> > Author: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Date:   Sun Jun 24 16:42:32 2012 +0200
> > 
> >     drm/i915: wrap up gt powersave enabling functions
> > 
> > I've botched up the handling of ironlake_disable_rc6. Fix this up by
> > calling it at the right place. Note though that ironlake_disable_rc6
> > does a bit more than just disabling rc6 - it also tears down all the
> > allocated context objects.
> 
> I can only hope that one day you will appreciate the API I set up in
> the context code to handle this.
> 
> > 
> > Hence we need to move intel_teardown_rc6 out and directly call it from
> > intel_modeset_cleanup.
> 
> I don't mind that you've done this, however could you explain/modify the
> comment when we want to disable without destroying the backing context
> object?
> 
> > 
> > Also properly mark ironlake_enable_rc6 as static and kill the un-used
> > declaration in i915_drv.h.
> > 
> > Cc: Ben Widawsky <ben@bwidawsk.net>
> > Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
Queued for -next (with a little comment added as discussed), thanks for
the review.
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48