[PATCH 0/7] Added missing changes for Turbo feature on SKL

[PATCH 0/7] Added missing changes for Turbo feature on SKL

[akash.goel]

From: Akash Goel <akash.goel@intel.com>

This patch series add missing changes, required for proper functioning of the
Turbo feature on SKL.

Akash Goel (7):
  drm/i915/skl: Added new macros
  drm/i915/skl: Updated the gen6_set_rps function
  drm/i915/skl: Restructured the gen6_set_rps_thresholds function
  drm/i915/skl: Updated the gen6_rps_limits function
  drm/i915/skl: Updated the gen9_enable_rps function
  drm/i915/skl: Updated the 'i915_frequency_info' debugs function
  drm/i915/skl: Enabling processing of Turbo interrupts

 drivers/gpu/drm/i915/i915_debugfs.c |  10 ++-
 drivers/gpu/drm/i915/i915_drv.h     |   1 +
 drivers/gpu/drm/i915/i915_irq.c     |   5 --
 drivers/gpu/drm/i915/i915_reg.h     |   9 +++
 drivers/gpu/drm/i915/intel_pm.c     | 132 ++++++++++++++++++++++--------------
 5 files changed, 99 insertions(+), 58 deletions(-)

-- 
1.9.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 1/7] drm/i915/skl: Added new macros

[akash.goel]

From: Akash Goel <akash.goel@intel.com>

For SKL, register definition for RPNSWREQ (A008), RPSTAT1(A01C)
have changed slightly. Also on SKL, frequency is specified in
units of 16.66 MHZ, compared to 50 MHZ for most of the earlier
platforms and the time values are expressed in units of 1.33 us,
compared to 1.28 us for earlier platforms.
Added new macros for the aforementioned changes.

Signed-off-by: Akash Goel <akash.goel@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h | 1 +
 drivers/gpu/drm/i915/i915_reg.h | 9 +++++++++
 2 files changed, 10 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index ca64b99..529b9b2 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2485,6 +2485,7 @@ struct drm_i915_cmd_table {
 #define NUM_L3_SLICES(dev) (IS_HSW_GT3(dev) ? 2 : HAS_L3_DPF(dev))
 
 #define GT_FREQUENCY_MULTIPLIER 50
+#define GEN9_FREQ_SCALER 3
 
 #include "i915_trace.h"
 
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index cd3430f9..c4a4c58 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -2389,6 +2389,12 @@ enum skl_disp_power_wells {
 #define GEN6_RP_STATE_LIMITS	(MCHBAR_MIRROR_BASE_SNB + 0x5994)
 #define GEN6_RP_STATE_CAP	(MCHBAR_MIRROR_BASE_SNB + 0x5998)
 
+#define FREQ_1_28_US(us)	(((us) * 100) >> 7)
+#define FREQ_1_33_US(us)	(((us) * 3)   >> 2)
+#define GT_FREQ_FROM_PERIOD(us, dev) (IS_GEN9(dev) ? \
+				FREQ_1_33_US(us) : \
+				FREQ_1_28_US(us))
+
 /*
  * Logical Context regs
  */
@@ -6023,6 +6029,7 @@ enum skl_disp_power_wells {
 #define   GEN6_TURBO_DISABLE			(1<<31)
 #define   GEN6_FREQUENCY(x)			((x)<<25)
 #define   HSW_FREQUENCY(x)			((x)<<24)
+#define   GEN9_FREQUENCY(x)			((x)<<23)
 #define   GEN6_OFFSET(x)			((x)<<19)
 #define   GEN6_AGGRESSIVE_TURBO			(0<<15)
 #define GEN6_RC_VIDEO_FREQ			0xA00C
@@ -6041,8 +6048,10 @@ enum skl_disp_power_wells {
 #define GEN6_RPSTAT1				0xA01C
 #define   GEN6_CAGF_SHIFT			8
 #define   HSW_CAGF_SHIFT			7
+#define   GEN9_CAGF_SHIFT			23
 #define   GEN6_CAGF_MASK			(0x7f << GEN6_CAGF_SHIFT)
 #define   HSW_CAGF_MASK				(0x7f << HSW_CAGF_SHIFT)
+#define   GEN9_CAGF_MASK			(0x1ff << GEN9_CAGF_SHIFT)
 #define GEN6_RP_CONTROL				0xA024
 #define   GEN6_RP_MEDIA_TURBO			(1<<11)
 #define   GEN6_RP_MEDIA_MODE_MASK		(3<<9)
-- 
1.9.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/7] drm/i915/skl: Added new macros

[Damien Lespiau]

On Fri, Feb 06, 2015 at 08:26:32PM +0530, akash.goel@intel.com wrote:
> From: Akash Goel <akash.goel@intel.com>
> 
> For SKL, register definition for RPNSWREQ (A008), RPSTAT1(A01C)
> have changed slightly. Also on SKL, frequency is specified in
> units of 16.66 MHZ, compared to 50 MHZ for most of the earlier
> platforms and the time values are expressed in units of 1.33 us,
> compared to 1.28 us for earlier platforms.
> Added new macros for the aforementioned changes.
> 
> Signed-off-by: Akash Goel <akash.goel@intel.com>

We try to put the relevant defines in the patch using them (for next
time :). Everything looks correct though:

Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>

-- 
Damien

> ---
>  drivers/gpu/drm/i915/i915_drv.h | 1 +
>  drivers/gpu/drm/i915/i915_reg.h | 9 +++++++++
>  2 files changed, 10 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index ca64b99..529b9b2 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2485,6 +2485,7 @@ struct drm_i915_cmd_table {
>  #define NUM_L3_SLICES(dev) (IS_HSW_GT3(dev) ? 2 : HAS_L3_DPF(dev))
>  
>  #define GT_FREQUENCY_MULTIPLIER 50
> +#define GEN9_FREQ_SCALER 3
>  
>  #include "i915_trace.h"
>  
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index cd3430f9..c4a4c58 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -2389,6 +2389,12 @@ enum skl_disp_power_wells {
>  #define GEN6_RP_STATE_LIMITS	(MCHBAR_MIRROR_BASE_SNB + 0x5994)
>  #define GEN6_RP_STATE_CAP	(MCHBAR_MIRROR_BASE_SNB + 0x5998)
>  
> +#define FREQ_1_28_US(us)	(((us) * 100) >> 7)
> +#define FREQ_1_33_US(us)	(((us) * 3)   >> 2)
> +#define GT_FREQ_FROM_PERIOD(us, dev) (IS_GEN9(dev) ? \
> +				FREQ_1_33_US(us) : \
> +				FREQ_1_28_US(us))
> +
>  /*
>   * Logical Context regs
>   */
> @@ -6023,6 +6029,7 @@ enum skl_disp_power_wells {
>  #define   GEN6_TURBO_DISABLE			(1<<31)
>  #define   GEN6_FREQUENCY(x)			((x)<<25)
>  #define   HSW_FREQUENCY(x)			((x)<<24)
> +#define   GEN9_FREQUENCY(x)			((x)<<23)
>  #define   GEN6_OFFSET(x)			((x)<<19)
>  #define   GEN6_AGGRESSIVE_TURBO			(0<<15)
>  #define GEN6_RC_VIDEO_FREQ			0xA00C
> @@ -6041,8 +6048,10 @@ enum skl_disp_power_wells {
>  #define GEN6_RPSTAT1				0xA01C
>  #define   GEN6_CAGF_SHIFT			8
>  #define   HSW_CAGF_SHIFT			7
> +#define   GEN9_CAGF_SHIFT			23
>  #define   GEN6_CAGF_MASK			(0x7f << GEN6_CAGF_SHIFT)
>  #define   HSW_CAGF_MASK				(0x7f << HSW_CAGF_SHIFT)
> +#define   GEN9_CAGF_MASK			(0x1ff << GEN9_CAGF_SHIFT)
>  #define GEN6_RP_CONTROL				0xA024
>  #define   GEN6_RP_MEDIA_TURBO			(1<<11)
>  #define   GEN6_RP_MEDIA_MODE_MASK		(3<<9)
> -- 
> 1.9.2
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/7] drm/i915/skl: Added new macros

[Damien Lespiau]

On Fri, Feb 06, 2015 at 08:26:32PM +0530, akash.goel@intel.com wrote:
> +#define FREQ_1_28_US(us)	(((us) * 100) >> 7)
> +#define FREQ_1_33_US(us)	(((us) * 3)   >> 2)
> +#define GT_FREQ_FROM_PERIOD(us, dev) (IS_GEN9(dev) ? \
> +				FREQ_1_33_US(us) : \
> +				FREQ_1_28_US(us))

I'm not sure why you call that GT_FREQ when it looks like a time for
evaluation intervals.

-- 
Damien
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/7] drm/i915/skl: Added new macros

[Goel, Akash]

Thanks for the review. Agree it's not an appropriate name. 
Please kindly suggest one.  
'GT_TIME_COUNTER_UNITS_FROM_PERIOD' ??

Best regards
Akash

-----Original Message-----
From: Lespiau, Damien 
Sent: Tuesday, February 17, 2015 8:10 PM
To: Goel, Akash
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 1/7] drm/i915/skl: Added new macros

On Fri, Feb 06, 2015 at 08:26:32PM +0530, akash.goel@intel.com wrote:
> +#define FREQ_1_28_US(us)	(((us) * 100) >> 7)
> +#define FREQ_1_33_US(us)	(((us) * 3)   >> 2)
> +#define GT_FREQ_FROM_PERIOD(us, dev) (IS_GEN9(dev) ? \
> +				FREQ_1_33_US(us) : \
> +				FREQ_1_28_US(us))

I'm not sure why you call that GT_FREQ when it looks like a time for evaluation intervals.

--
Damien
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/7] drm/i915/skl: Added new macros

[Damien Lespiau]

How about GT_INTERVAL_FROM_US()? GT_EVALUATION_COUNTER_FROM_US()?
something along these lines I guess.

-- 
Damien

On Tue, Feb 17, 2015 at 03:20:53PM +0000, Goel, Akash wrote:
> Thanks for the review. Agree it's not an appropriate name. 
> Please kindly suggest one.  
> 'GT_TIME_COUNTER_UNITS_FROM_PERIOD' ??
> 
> Best regards
> Akash
> 
> -----Original Message-----
> From: Lespiau, Damien 
> Sent: Tuesday, February 17, 2015 8:10 PM
> To: Goel, Akash
> Cc: intel-gfx@lists.freedesktop.org
> Subject: Re: [PATCH 1/7] drm/i915/skl: Added new macros
> 
> On Fri, Feb 06, 2015 at 08:26:32PM +0530, akash.goel@intel.com wrote:
> > +#define FREQ_1_28_US(us)	(((us) * 100) >> 7)
> > +#define FREQ_1_33_US(us)	(((us) * 3)   >> 2)
> > +#define GT_FREQ_FROM_PERIOD(us, dev) (IS_GEN9(dev) ? \
> > +				FREQ_1_33_US(us) : \
> > +				FREQ_1_28_US(us))
> 
> I'm not sure why you call that GT_FREQ when it looks like a time for evaluation intervals.
> 
> --
> Damien
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/7] drm/i915/skl: Added new macros

[Goel, Akash]

Will prefer GT_INTERVAL_FROM_US, as GT_EVALUATION_COUNTER_FROM_US would be more specific.

Best regards
Akash

-----Original Message-----
From: Lespiau, Damien 
Sent: Tuesday, February 17, 2015 8:56 PM
To: Goel, Akash
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 1/7] drm/i915/skl: Added new macros

How about GT_INTERVAL_FROM_US()? GT_EVALUATION_COUNTER_FROM_US()?
something along these lines I guess.

-- 
Damien

On Tue, Feb 17, 2015 at 03:20:53PM +0000, Goel, Akash wrote:
> Thanks for the review. Agree it's not an appropriate name. 
> Please kindly suggest one.  
> 'GT_TIME_COUNTER_UNITS_FROM_PERIOD' ??
> 
> Best regards
> Akash
> 
> -----Original Message-----
> From: Lespiau, Damien 
> Sent: Tuesday, February 17, 2015 8:10 PM
> To: Goel, Akash
> Cc: intel-gfx@lists.freedesktop.org
> Subject: Re: [PATCH 1/7] drm/i915/skl: Added new macros
> 
> On Fri, Feb 06, 2015 at 08:26:32PM +0530, akash.goel@intel.com wrote:
> > +#define FREQ_1_28_US(us)	(((us) * 100) >> 7)
> > +#define FREQ_1_33_US(us)	(((us) * 3)   >> 2)
> > +#define GT_FREQ_FROM_PERIOD(us, dev) (IS_GEN9(dev) ? \
> > +				FREQ_1_33_US(us) : \
> > +				FREQ_1_28_US(us))
> 
> I'm not sure why you call that GT_FREQ when it looks like a time for evaluation intervals.
> 
> --
> Damien
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/7] drm/i915/skl: Added new macros

[Daniel Vetter]

On Tue, Feb 17, 2015 at 03:32:35PM +0000, Goel, Akash wrote:
> Will prefer GT_INTERVAL_FROM_US, as GT_EVALUATION_COUNTER_FROM_US would be more specific.

Is there a new patch with revised #defines? I haven't yet caught up with
mail ...
-Daniel

> 
> Best regards
> Akash
> 
> -----Original Message-----
> From: Lespiau, Damien 
> Sent: Tuesday, February 17, 2015 8:56 PM
> To: Goel, Akash
> Cc: intel-gfx@lists.freedesktop.org
> Subject: Re: [PATCH 1/7] drm/i915/skl: Added new macros
> 
> How about GT_INTERVAL_FROM_US()? GT_EVALUATION_COUNTER_FROM_US()?
> something along these lines I guess.
> 
> -- 
> Damien
> 
> On Tue, Feb 17, 2015 at 03:20:53PM +0000, Goel, Akash wrote:
> > Thanks for the review. Agree it's not an appropriate name. 
> > Please kindly suggest one.  
> > 'GT_TIME_COUNTER_UNITS_FROM_PERIOD' ??
> > 
> > Best regards
> > Akash
> > 
> > -----Original Message-----
> > From: Lespiau, Damien 
> > Sent: Tuesday, February 17, 2015 8:10 PM
> > To: Goel, Akash
> > Cc: intel-gfx@lists.freedesktop.org
> > Subject: Re: [PATCH 1/7] drm/i915/skl: Added new macros
> > 
> > On Fri, Feb 06, 2015 at 08:26:32PM +0530, akash.goel@intel.com wrote:
> > > +#define FREQ_1_28_US(us)	(((us) * 100) >> 7)
> > > +#define FREQ_1_33_US(us)	(((us) * 3)   >> 2)
> > > +#define GT_FREQ_FROM_PERIOD(us, dev) (IS_GEN9(dev) ? \
> > > +				FREQ_1_33_US(us) : \
> > > +				FREQ_1_28_US(us))
> > 
> > I'm not sure why you call that GT_FREQ when it looks like a time for evaluation intervals.
> > 
> > --
> > Damien
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/7] drm/i915/skl: Added new macros

[Damien Lespiau]

On Mon, Feb 23, 2015 at 05:21:35PM +0100, Daniel Vetter wrote:
> On Tue, Feb 17, 2015 at 03:32:35PM +0000, Goel, Akash wrote:
> > Will prefer GT_INTERVAL_FROM_US, as GT_EVALUATION_COUNTER_FROM_US would be more specific.
> 
> Is there a new patch with revised #defines? I haven't yet caught up with
> mail ...

Yes, but it's awfully entangled, each new series being sent as a reply
of another series. Sending a separate thread for a new version is so
much easier for the reader when several patches change.

-- 
Damien
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/7] drm/i915/skl: Updated the gen6_set_rps function

[akash.goel]

From: Akash Goel <akash.goel@intel.com>

On SKL, the frequency programmed in RPNSWREQ (A008) register
has to be in units of 16.66 MHZ. So updated the gen6_set_rps
function, as per this change.

Signed-off-by: Akash Goel <akash.goel@intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index bebefe7..58c8c0e 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3764,7 +3764,10 @@ static void gen6_set_rps(struct drm_device *dev, u8 val)
 	if (val != dev_priv->rps.cur_freq) {
 		gen6_set_rps_thresholds(dev_priv, val);
 
-		if (IS_HASWELL(dev) || IS_BROADWELL(dev))
+		if (IS_GEN9(dev))
+                        I915_WRITE(GEN6_RPNSWREQ,
+                                   GEN9_FREQUENCY(val * GEN9_FREQ_SCALER));
+		else if (IS_HASWELL(dev) || IS_BROADWELL(dev))
 			I915_WRITE(GEN6_RPNSWREQ,
 				   HSW_FREQUENCY(val));
 		else
-- 
1.9.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/7] drm/i915/skl: Updated the gen6_set_rps function

[Damien Lespiau]

On Fri, Feb 06, 2015 at 08:26:33PM +0530, akash.goel@intel.com wrote:
> From: Akash Goel <akash.goel@intel.com>
> 
> On SKL, the frequency programmed in RPNSWREQ (A008) register
> has to be in units of 16.66 MHZ. So updated the gen6_set_rps
> function, as per this change.
> 
> Signed-off-by: Akash Goel <akash.goel@intel.com>
> ---

Right, we suppose here that val in in 16.66 Mhz units. At the very least
we need update the trace point:

  trace_intel_gpu_freq_change(val * 50);

Then val is passed to gen6_rps_limits(). The values of 0xA014 are also
in 16.66 Mhz units, so that part is fine, but the fields of that
register have changed a bit so we also need to update gen6_rps_limits()
for gen9 (if not done by a later patch).

-- 
Damien

>  drivers/gpu/drm/i915/intel_pm.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index bebefe7..58c8c0e 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3764,7 +3764,10 @@ static void gen6_set_rps(struct drm_device *dev, u8 val)
>  	if (val != dev_priv->rps.cur_freq) {
>  		gen6_set_rps_thresholds(dev_priv, val);
>  
> -		if (IS_HASWELL(dev) || IS_BROADWELL(dev))
> +		if (IS_GEN9(dev))
> +                        I915_WRITE(GEN6_RPNSWREQ,
> +                                   GEN9_FREQUENCY(val * GEN9_FREQ_SCALER));
> +		else if (IS_HASWELL(dev) || IS_BROADWELL(dev))
>  			I915_WRITE(GEN6_RPNSWREQ,
>  				   HSW_FREQUENCY(val));
>  		else
> -- 
> 1.9.2
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/7] drm/i915/skl: Updated the gen6_set_rps function

[Damien Lespiau]

On Tue, Feb 17, 2015 at 02:31:08PM +0000, Damien Lespiau wrote:
> On Fri, Feb 06, 2015 at 08:26:33PM +0530, akash.goel@intel.com wrote:
> > From: Akash Goel <akash.goel@intel.com>
> > 
> > On SKL, the frequency programmed in RPNSWREQ (A008) register
> > has to be in units of 16.66 MHZ. So updated the gen6_set_rps
> > function, as per this change.
> > 
> > Signed-off-by: Akash Goel <akash.goel@intel.com>
> > ---
> 
> Right, we suppose here that val in in 16.66 Mhz units. At the very least
> we need update the trace point:
> 
>   trace_intel_gpu_freq_change(val * 50);
> 
> Then val is passed to gen6_rps_limits(). The values of 0xA014 are also
> in 16.66 Mhz units, so that part is fine, but the fields of that
> register have changed a bit so we also need to update gen6_rps_limits()
> for gen9 (if not done by a later patch).

I managed to get quite confused, I blame the lack of sleep. From
RP_STATE_CAP, we get all the limits in 50Mhz and we store them like
this. So everything is done in units of 50Mhz and then converted to
units of 16.66Mhz at write time. Took me the whole series to realize
that, sorry.

So:

Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>

-- 
Damien
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 3/7] drm/i915/skl: Restructured the gen6_set_rps_thresholds function

[akash.goel]

From: Akash Goel <akash.goel@intel.com>

Prior to SKL, the time period programmed in Up/Down EI & Up/Down
threshold registers was in units of 1.28 micro seconds. But for
SKL, the units have changed (1.333 micro seconds).
Have generalized the implementation of gen6_set_rps_thresholds function,
by removing the hard coding done in it as per 1.28 micro seconds.

Signed-off-by: Akash Goel <akash.goel@intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 70 ++++++++++++++++++++---------------------
 1 file changed, 34 insertions(+), 36 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 58c8c0e..215b200 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3643,6 +3643,8 @@ static u32 gen6_rps_limits(struct drm_i915_private *dev_priv, u8 val)
 static void gen6_set_rps_thresholds(struct drm_i915_private *dev_priv, u8 val)
 {
 	int new_power;
+	u32 threshold_up_pct = 0, threshold_down_pct = 0;
+	u32 ei_up = 0, ei_down = 0;
 
 	new_power = dev_priv->rps.power;
 	switch (dev_priv->rps.power) {
@@ -3675,59 +3677,55 @@ static void gen6_set_rps_thresholds(struct drm_i915_private *dev_priv, u8 val)
 	switch (new_power) {
 	case LOW_POWER:
 		/* Upclock if more than 95% busy over 16ms */
-		I915_WRITE(GEN6_RP_UP_EI, 12500);
-		I915_WRITE(GEN6_RP_UP_THRESHOLD, 11800);
+		ei_up = 16000;
+		threshold_up_pct = 95; /* x% */
 
 		/* Downclock if less than 85% busy over 32ms */
-		I915_WRITE(GEN6_RP_DOWN_EI, 25000);
-		I915_WRITE(GEN6_RP_DOWN_THRESHOLD, 21250);
-
-		I915_WRITE(GEN6_RP_CONTROL,
-			   GEN6_RP_MEDIA_TURBO |
-			   GEN6_RP_MEDIA_HW_NORMAL_MODE |
-			   GEN6_RP_MEDIA_IS_GFX |
-			   GEN6_RP_ENABLE |
-			   GEN6_RP_UP_BUSY_AVG |
-			   GEN6_RP_DOWN_IDLE_AVG);
+		ei_down = 32000;
+		threshold_down_pct = 85;
 		break;
 
 	case BETWEEN:
 		/* Upclock if more than 90% busy over 13ms */
-		I915_WRITE(GEN6_RP_UP_EI, 10250);
-		I915_WRITE(GEN6_RP_UP_THRESHOLD, 9225);
+		ei_up = 13000;
+		threshold_up_pct = 90; /* x% */
 
 		/* Downclock if less than 75% busy over 32ms */
-		I915_WRITE(GEN6_RP_DOWN_EI, 25000);
-		I915_WRITE(GEN6_RP_DOWN_THRESHOLD, 18750);
-
-		I915_WRITE(GEN6_RP_CONTROL,
-			   GEN6_RP_MEDIA_TURBO |
-			   GEN6_RP_MEDIA_HW_NORMAL_MODE |
-			   GEN6_RP_MEDIA_IS_GFX |
-			   GEN6_RP_ENABLE |
-			   GEN6_RP_UP_BUSY_AVG |
-			   GEN6_RP_DOWN_IDLE_AVG);
+		ei_down = 32000;
+		threshold_down_pct = 75;
 		break;
 
 	case HIGH_POWER:
 		/* Upclock if more than 85% busy over 10ms */
-		I915_WRITE(GEN6_RP_UP_EI, 8000);
-		I915_WRITE(GEN6_RP_UP_THRESHOLD, 6800);
+		ei_up = 10000;
+		threshold_up_pct = 85; /* x% */
 
 		/* Downclock if less than 60% busy over 32ms */
-		I915_WRITE(GEN6_RP_DOWN_EI, 25000);
-		I915_WRITE(GEN6_RP_DOWN_THRESHOLD, 15000);
-
-		I915_WRITE(GEN6_RP_CONTROL,
-			   GEN6_RP_MEDIA_TURBO |
-			   GEN6_RP_MEDIA_HW_NORMAL_MODE |
-			   GEN6_RP_MEDIA_IS_GFX |
-			   GEN6_RP_ENABLE |
-			   GEN6_RP_UP_BUSY_AVG |
-			   GEN6_RP_DOWN_IDLE_AVG);
+		ei_down = 32000;
+		threshold_down_pct = 60;
 		break;
 	}
 
+	I915_WRITE(GEN6_RP_UP_EI,
+		GT_FREQ_FROM_PERIOD(ei_up, dev_priv->dev));
+	I915_WRITE(GEN6_RP_UP_THRESHOLD,
+		GT_FREQ_FROM_PERIOD((ei_up * threshold_up_pct / 100),
+		dev_priv->dev));
+
+	I915_WRITE(GEN6_RP_DOWN_EI,
+		GT_FREQ_FROM_PERIOD(ei_down, dev_priv->dev));
+	I915_WRITE(GEN6_RP_DOWN_THRESHOLD,
+		GT_FREQ_FROM_PERIOD((ei_down * threshold_down_pct / 100),
+		dev_priv->dev));
+
+	 I915_WRITE(GEN6_RP_CONTROL,
+		    GEN6_RP_MEDIA_TURBO |
+		    GEN6_RP_MEDIA_HW_NORMAL_MODE |
+		    GEN6_RP_MEDIA_IS_GFX |
+		    GEN6_RP_ENABLE |
+		    GEN6_RP_UP_BUSY_AVG |
+		    GEN6_RP_DOWN_IDLE_AVG);
+
 	dev_priv->rps.power = new_power;
 	dev_priv->rps.last_adj = 0;
 }
-- 
1.9.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/7] drm/i915/skl: Restructured the gen6_set_rps_thresholds function

[Chris Wilson]

On Fri, Feb 06, 2015 at 08:26:34PM +0530, akash.goel@intel.com wrote:
> From: Akash Goel <akash.goel@intel.com>
> 
> Prior to SKL, the time period programmed in Up/Down EI & Up/Down
> threshold registers was in units of 1.28 micro seconds. But for
> SKL, the units have changed (1.333 micro seconds).
> Have generalized the implementation of gen6_set_rps_thresholds function,
> by removing the hard coding done in it as per 1.28 micro seconds.
> 
> Signed-off-by: Akash Goel <akash.goel@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 70 ++++++++++++++++++++---------------------
>  1 file changed, 34 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 58c8c0e..215b200 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3643,6 +3643,8 @@ static u32 gen6_rps_limits(struct drm_i915_private *dev_priv, u8 val)
>  static void gen6_set_rps_thresholds(struct drm_i915_private *dev_priv, u8 val)
>  {
>  	int new_power;
> +	u32 threshold_up_pct = 0, threshold_down_pct = 0;

Drop the _pct, unrequired early initialisation, just comment that
up/down are in %.

> +	u32 ei_up = 0, ei_down = 0;
>  
>  	new_power = dev_priv->rps.power;
>  	switch (dev_priv->rps.power) {
> @@ -3675,59 +3677,55 @@ static void gen6_set_rps_thresholds(struct drm_i915_private *dev_priv, u8 val)
>  	switch (new_power) {
>  	case LOW_POWER:
>  		/* Upclock if more than 95% busy over 16ms */
> -		I915_WRITE(GEN6_RP_UP_EI, 12500);
> -		I915_WRITE(GEN6_RP_UP_THRESHOLD, 11800);
> +		ei_up = 16000;
> +		threshold_up_pct = 95; /* x% */

Double comments that this is a %! Really doesn't seem to be required
with the preceeding comment.

> +	I915_WRITE(GEN6_RP_UP_EI,
> +		GT_FREQ_FROM_PERIOD(ei_up, dev_priv->dev));

Just pass dev_priv. It's magic.

> +	I915_WRITE(GEN6_RP_UP_THRESHOLD,
> +		GT_FREQ_FROM_PERIOD((ei_up * threshold_up_pct / 100),

I wonder if it is worth using base 128 instead of 100%.

Otherwise looks good and ties in with using it from vlv. Do you mind
reviewing those patches? They fix a bug in which the manual c0 counting
keeps interrupts alive whilst idle.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/7] drm/i915/skl: Restructured the gen6_set_rps_thresholds function

[Akash Goel]

On Fri, 2015-02-06 at 15:48 +0000, Chris Wilson wrote:
> On Fri, Feb 06, 2015 at 08:26:34PM +0530, akash.goel@intel.com wrote:
> > From: Akash Goel <akash.goel@intel.com>
> > 
> > Prior to SKL, the time period programmed in Up/Down EI & Up/Down
> > threshold registers was in units of 1.28 micro seconds. But for
> > SKL, the units have changed (1.333 micro seconds).
> > Have generalized the implementation of gen6_set_rps_thresholds function,
> > by removing the hard coding done in it as per 1.28 micro seconds.
> > 
> > Signed-off-by: Akash Goel <akash.goel@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_pm.c | 70 ++++++++++++++++++++---------------------
> >  1 file changed, 34 insertions(+), 36 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index 58c8c0e..215b200 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -3643,6 +3643,8 @@ static u32 gen6_rps_limits(struct drm_i915_private *dev_priv, u8 val)
> >  static void gen6_set_rps_thresholds(struct drm_i915_private *dev_priv, u8 val)
> >  {
> >  	int new_power;
> > +	u32 threshold_up_pct = 0, threshold_down_pct = 0;
> 
> Drop the _pct, unrequired early initialisation, just comment that
> up/down are in %.
Fine, will remove the _pct suffix.

> > +	u32 ei_up = 0, ei_down = 0;
> >  
> >  	new_power = dev_priv->rps.power;
> >  	switch (dev_priv->rps.power) {
> > @@ -3675,59 +3677,55 @@ static void gen6_set_rps_thresholds(struct drm_i915_private *dev_priv, u8 val)
> >  	switch (new_power) {
> >  	case LOW_POWER:
> >  		/* Upclock if more than 95% busy over 16ms */
> > -		I915_WRITE(GEN6_RP_UP_EI, 12500);
> > -		I915_WRITE(GEN6_RP_UP_THRESHOLD, 11800);
> > +		ei_up = 16000;
> > +		threshold_up_pct = 95; /* x% */
> 
> Double comments that this is a %! Really doesn't seem to be required
> with the preceeding comment.
Fine, will remove the needless comment

> > +	I915_WRITE(GEN6_RP_UP_EI,
> > +		GT_FREQ_FROM_PERIOD(ei_up, dev_priv->dev));
> 
> Just pass dev_priv. It's magic.
Will modify the macro to accept the dev_priv instead of dev.

> 
> > +	I915_WRITE(GEN6_RP_UP_THRESHOLD,
> > +		GT_FREQ_FROM_PERIOD((ei_up * threshold_up_pct / 100),
> 
> I wonder if it is worth using base 128 instead of 100%.
Sorry couldn't get this comment. 
Actually the macro GT_FREQ_FROM_PERIOD has been used at other places,
where % conversion is not required. So not sure, how using a base of
128 would be better. 

> 
> Otherwise looks good and ties in with using it from vlv. Do you mind
> reviewing those patches? 
Sure, will review that patch, please provide the patch link. 

> They fix a bug in which the manual c0 counting
> keeps interrupts alive whilst idle.


> -Chris
> 


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 4/7] drm/i915/skl: Updated the gen6_rps_limits function

[akash.goel]

From: Akash Goel <akash.goel@intel.com>

RP Interrupt Up/Down Frequency Limits register (A014) definition
has changed for SKL. Updated the gen6_rps_limits function as per that

Signed-off-by: Akash Goel <akash.goel@intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 215b200..db24b48 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3623,7 +3623,7 @@ static void ironlake_disable_drps(struct drm_device *dev)
  * ourselves, instead of doing a rmw cycle (which might result in us clearing
  * all limits and the gpu stuck at whatever frequency it is at atm).
  */
-static u32 gen6_rps_limits(struct drm_i915_private *dev_priv, u8 val)
+static u32 get_rps_limits(struct drm_i915_private *dev_priv, u8 val)
 {
 	u32 limits;
 
@@ -3633,9 +3633,15 @@ static u32 gen6_rps_limits(struct drm_i915_private *dev_priv, u8 val)
 	 * the hw runs at the minimal clock before selecting the desired
 	 * frequency, if the down threshold expires in that window we will not
 	 * receive a down interrupt. */
-	limits = dev_priv->rps.max_freq_softlimit << 24;
-	if (val <= dev_priv->rps.min_freq_softlimit)
-		limits |= dev_priv->rps.min_freq_softlimit << 16;
+	if (IS_GEN9(dev_priv->dev)) {
+		limits = (dev_priv->rps.max_freq_softlimit * GEN9_FREQ_SCALER) << 23;
+		if (val <= dev_priv->rps.min_freq_softlimit)
+			limits |= (dev_priv->rps.min_freq_softlimit * GEN9_FREQ_SCALER) << 14;
+	} else {
+		limits = dev_priv->rps.max_freq_softlimit << 24;
+		if (val <= dev_priv->rps.min_freq_softlimit)
+			limits |= dev_priv->rps.min_freq_softlimit << 16;
+	}
 
 	return limits;
 }
@@ -3778,7 +3784,7 @@ static void gen6_set_rps(struct drm_device *dev, u8 val)
 	/* Make sure we continue to get interrupts
 	 * until we hit the minimum or maximum frequencies.
 	 */
-	I915_WRITE(GEN6_RP_INTERRUPT_LIMITS, gen6_rps_limits(dev_priv, val));
+	I915_WRITE(GEN6_RP_INTERRUPT_LIMITS, get_rps_limits(dev_priv, val));
 	I915_WRITE(GEN6_PMINTRMSK, gen6_rps_pm_mask(dev_priv, val));
 
 	POSTING_READ(GEN6_RPNSWREQ);
-- 
1.9.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/7] drm/i915/skl: Updated the gen6_rps_limits function

[Chris Wilson]

On Fri, Feb 06, 2015 at 08:26:35PM +0530, akash.goel@intel.com wrote:
> From: Akash Goel <akash.goel@intel.com>
> 
> RP Interrupt Up/Down Frequency Limits register (A014) definition
> has changed for SKL. Updated the gen6_rps_limits function as per that
> 
> Signed-off-by: Akash Goel <akash.goel@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 16 +++++++++++-----
>  1 file changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 215b200..db24b48 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3623,7 +3623,7 @@ static void ironlake_disable_drps(struct drm_device *dev)
>   * ourselves, instead of doing a rmw cycle (which might result in us clearing
>   * all limits and the gpu stuck at whatever frequency it is at atm).
>   */
> -static u32 gen6_rps_limits(struct drm_i915_private *dev_priv, u8 val)
> +static u32 get_rps_limits(struct drm_i915_private *dev_priv, u8 val)

Spurious name change, it doesn't seem to add anything or clear up any
confusion with vlv.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/7] drm/i915/skl: Updated the gen6_rps_limits function

[Akash Goel]

On Fri, 2015-02-06 at 15:43 +0000, Chris Wilson wrote:
> On Fri, Feb 06, 2015 at 08:26:35PM +0530, akash.goel@intel.com wrote:
> > From: Akash Goel <akash.goel@intel.com>
> > 
> > RP Interrupt Up/Down Frequency Limits register (A014) definition
> > has changed for SKL. Updated the gen6_rps_limits function as per that
> > 
> > Signed-off-by: Akash Goel <akash.goel@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_pm.c | 16 +++++++++++-----
> >  1 file changed, 11 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index 215b200..db24b48 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -3623,7 +3623,7 @@ static void ironlake_disable_drps(struct drm_device *dev)
> >   * ourselves, instead of doing a rmw cycle (which might result in us clearing
> >   * all limits and the gpu stuck at whatever frequency it is at atm).
> >   */
> > -static u32 gen6_rps_limits(struct drm_i915_private *dev_priv, u8 val)
> > +static u32 get_rps_limits(struct drm_i915_private *dev_priv, u8 val)
> 
> Spurious name change, it doesn't seem to add anything or clear up any
> confusion with vlv.
Fine will keep the original name, thought would be better to give a
generic name to the function and abstract the platform specific
differences inside its definition. 

> -Chris
> 


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/7] drm/i915/skl: Updated the gen6_rps_limits function

[Chris Wilson]

On Mon, Feb 09, 2015 at 10:26:33AM +0530, Akash Goel wrote:
> On Fri, 2015-02-06 at 15:43 +0000, Chris Wilson wrote:
> > On Fri, Feb 06, 2015 at 08:26:35PM +0530, akash.goel@intel.com wrote:
> > > From: Akash Goel <akash.goel@intel.com>
> > > 
> > > RP Interrupt Up/Down Frequency Limits register (A014) definition
> > > has changed for SKL. Updated the gen6_rps_limits function as per that
> > > 
> > > Signed-off-by: Akash Goel <akash.goel@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_pm.c | 16 +++++++++++-----
> > >  1 file changed, 11 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > > index 215b200..db24b48 100644
> > > --- a/drivers/gpu/drm/i915/intel_pm.c
> > > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > > @@ -3623,7 +3623,7 @@ static void ironlake_disable_drps(struct drm_device *dev)
> > >   * ourselves, instead of doing a rmw cycle (which might result in us clearing
> > >   * all limits and the gpu stuck at whatever frequency it is at atm).
> > >   */
> > > -static u32 gen6_rps_limits(struct drm_i915_private *dev_priv, u8 val)
> > > +static u32 get_rps_limits(struct drm_i915_private *dev_priv, u8 val)
> > 
> > Spurious name change, it doesn't seem to add anything or clear up any
> > confusion with vlv.
> Fine will keep the original name, thought would be better to give a
> generic name to the function and abstract the platform specific
> differences inside its definition. 

Generic would be intel_rps_limits(). I am wary of using get(), the
common idiom is for a getter to return ownership as well, e.g.
kref_get(), intel_uncore_forcewake_get(). Also outside of trivial getters
and setters, get is such a generic verb that I don't think it adds much
self-documentating value, especially when breaking established patterns.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/7] drm/i915/skl: Updated the gen6_rps_limits function

[Akash Goel]

On Mon, 2015-02-09 at 11:03 +0000, Chris Wilson wrote:
> On Mon, Feb 09, 2015 at 10:26:33AM +0530, Akash Goel wrote:
> > On Fri, 2015-02-06 at 15:43 +0000, Chris Wilson wrote:
> > > On Fri, Feb 06, 2015 at 08:26:35PM +0530, akash.goel@intel.com wrote:
> > > > From: Akash Goel <akash.goel@intel.com>
> > > > 
> > > > RP Interrupt Up/Down Frequency Limits register (A014) definition
> > > > has changed for SKL. Updated the gen6_rps_limits function as per that
> > > > 
> > > > Signed-off-by: Akash Goel <akash.goel@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_pm.c | 16 +++++++++++-----
> > > >  1 file changed, 11 insertions(+), 5 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > > > index 215b200..db24b48 100644
> > > > --- a/drivers/gpu/drm/i915/intel_pm.c
> > > > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > > > @@ -3623,7 +3623,7 @@ static void ironlake_disable_drps(struct drm_device *dev)
> > > >   * ourselves, instead of doing a rmw cycle (which might result in us clearing
> > > >   * all limits and the gpu stuck at whatever frequency it is at atm).
> > > >   */
> > > > -static u32 gen6_rps_limits(struct drm_i915_private *dev_priv, u8 val)
> > > > +static u32 get_rps_limits(struct drm_i915_private *dev_priv, u8 val)
> > > 
> > > Spurious name change, it doesn't seem to add anything or clear up any
> > > confusion with vlv.
> > Fine will keep the original name, thought would be better to give a
> > generic name to the function and abstract the platform specific
> > differences inside its definition. 
> 
> Generic would be intel_rps_limits(). I am wary of using get(), the
> common idiom is for a getter to return ownership as well, e.g.
> kref_get(), intel_uncore_forcewake_get(). Also outside of trivial getters
> and setters, get is such a generic verb that I don't think it adds much
> self-documentating value, especially when breaking established patterns.

Understood, using get() is a misnomer & would be inconsistent with the
existing naming patterns. Will rename it to 'intel_rps_limits'. Thanks
for the clarification.

> -Chris
> 


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/7] drm/i915/skl: Updated the gen6_rps_limits function

[Damien Lespiau]

On Fri, Feb 06, 2015 at 08:26:35PM +0530, akash.goel@intel.com wrote:
> From: Akash Goel <akash.goel@intel.com>
> 
> RP Interrupt Up/Down Frequency Limits register (A014) definition
> has changed for SKL. Updated the gen6_rps_limits function as per that
> 
> Signed-off-by: Akash Goel <akash.goel@intel.com>

Ah, this is the change I was looking for earlier. Comment below though:

> ---
>  drivers/gpu/drm/i915/intel_pm.c | 16 +++++++++++-----
>  1 file changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 215b200..db24b48 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3623,7 +3623,7 @@ static void ironlake_disable_drps(struct drm_device *dev)
>   * ourselves, instead of doing a rmw cycle (which might result in us clearing
>   * all limits and the gpu stuck at whatever frequency it is at atm).
>   */
> -static u32 gen6_rps_limits(struct drm_i915_private *dev_priv, u8 val)
> +static u32 get_rps_limits(struct drm_i915_private *dev_priv, u8 val)
>  {
>  	u32 limits;
>  
> @@ -3633,9 +3633,15 @@ static u32 gen6_rps_limits(struct drm_i915_private *dev_priv, u8 val)
>  	 * the hw runs at the minimal clock before selecting the desired
>  	 * frequency, if the down threshold expires in that window we will not
>  	 * receive a down interrupt. */
> -	limits = dev_priv->rps.max_freq_softlimit << 24;
> -	if (val <= dev_priv->rps.min_freq_softlimit)
> -		limits |= dev_priv->rps.min_freq_softlimit << 16;
> +	if (IS_GEN9(dev_priv->dev)) {
> +		limits = (dev_priv->rps.max_freq_softlimit * GEN9_FREQ_SCALER) << 23;
> +		if (val <= dev_priv->rps.min_freq_softlimit)
> +			limits |= (dev_priv->rps.min_freq_softlimit * GEN9_FREQ_SCALER) << 14;

I believe the values here are in 16.666 Mhz, the power spec I have gives
examples:
  [31:23]=54d: No interrupt if already >=900MHz
  [22:14]=18d: No interrupt if already <= 300MHz

and 54 * 16.66.. = 900.

> +	} else {
> +		limits = dev_priv->rps.max_freq_softlimit << 24;
> +		if (val <= dev_priv->rps.min_freq_softlimit)
> +			limits |= dev_priv->rps.min_freq_softlimit << 16;
> +	}
>  
>  	return limits;
>  }
> @@ -3778,7 +3784,7 @@ static void gen6_set_rps(struct drm_device *dev, u8 val)
>  	/* Make sure we continue to get interrupts
>  	 * until we hit the minimum or maximum frequencies.
>  	 */
> -	I915_WRITE(GEN6_RP_INTERRUPT_LIMITS, gen6_rps_limits(dev_priv, val));
> +	I915_WRITE(GEN6_RP_INTERRUPT_LIMITS, get_rps_limits(dev_priv, val));
>  	I915_WRITE(GEN6_PMINTRMSK, gen6_rps_pm_mask(dev_priv, val));
>  
>  	POSTING_READ(GEN6_RPNSWREQ);
> -- 
> 1.9.2
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/7] drm/i915/skl: Updated the gen6_rps_limits function

[Damien Lespiau]

On Tue, Feb 17, 2015 at 02:44:56PM +0000, Damien Lespiau wrote:
> On Fri, Feb 06, 2015 at 08:26:35PM +0530, akash.goel@intel.com wrote:
> > +	if (IS_GEN9(dev_priv->dev)) {
> > +		limits = (dev_priv->rps.max_freq_softlimit * GEN9_FREQ_SCALER) << 23;
> > +		if (val <= dev_priv->rps.min_freq_softlimit)
> > +			limits |= (dev_priv->rps.min_freq_softlimit * GEN9_FREQ_SCALER) << 14;
> 
> I believe the values here are in 16.666 Mhz, the power spec I have gives
> examples:
>   [31:23]=54d: No interrupt if already >=900MHz
>   [22:14]=18d: No interrupt if already <= 300MHz
> 
> and 54 * 16.66.. = 900.

Right, so RP_STATE_CAP is documented using 50Mhz units, so the above is
correct. How about unifying all values for SKL to the same units (always
16.66 Mhz units)? that would remove some of the confusion a bit.

-- 
Damien
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/7] drm/i915/skl: Updated the gen6_rps_limits function

[Damien Lespiau]

On Tue, Feb 17, 2015 at 03:01:39PM +0000, Damien Lespiau wrote:
> On Tue, Feb 17, 2015 at 02:44:56PM +0000, Damien Lespiau wrote:
> > On Fri, Feb 06, 2015 at 08:26:35PM +0530, akash.goel@intel.com wrote:
> > > +	if (IS_GEN9(dev_priv->dev)) {
> > > +		limits = (dev_priv->rps.max_freq_softlimit * GEN9_FREQ_SCALER) << 23;
> > > +		if (val <= dev_priv->rps.min_freq_softlimit)
> > > +			limits |= (dev_priv->rps.min_freq_softlimit * GEN9_FREQ_SCALER) << 14;
> > 
> > I believe the values here are in 16.666 Mhz, the power spec I have gives
> > examples:
> >   [31:23]=54d: No interrupt if already >=900MHz
> >   [22:14]=18d: No interrupt if already <= 300MHz
> > 
> > and 54 * 16.66.. = 900.
> 
> Right, so RP_STATE_CAP is documented using 50Mhz units, so the above is
> correct. How about unifying all values for SKL to the same units (always
> 16.66 Mhz units)? that would remove some of the confusion a bit.

Right, so going to back here, that looks correct:

Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>

-- 
Damien
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 5/7] drm/i915/skl: Updated the gen9_enable_rps function

[akash.goel]

From: Akash Goel <akash.goel@intel.com>

On SKL, GT frequency is programmed in units of 16.66 MHZ units compared
to 50 MHZ for older platforms. Also the time value specified for Up/Down EI &
Up/Down thresholds are expressed in units of 1.33 us, compared to 1.28
us for older platforms. So updated the gen9_enable_rps function as per that.

Signed-off-by: Akash Goel <akash.goel@intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 41 ++++++++++++++++++++++++++++++++---------
 1 file changed, 32 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index db24b48..865df1f 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4037,27 +4037,50 @@ static void gen6_init_rps_frequencies(struct drm_device *dev)
 static void gen9_enable_rps(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
+	u32 threshold_up_pct, threshold_down_pct;
+	u32 ei_up, ei_down;
 
 	intel_uncore_forcewake_get(dev_priv, FORCEWAKE_ALL);
 
 	gen6_init_rps_frequencies(dev);
 
-	I915_WRITE(GEN6_RPNSWREQ, 0xc800000);
-	I915_WRITE(GEN6_RC_VIDEO_FREQ, 0xc800000);
+	/* Program defaults and thresholds for RPS*/
+	I915_WRITE(GEN6_RPNSWREQ,
+		GEN9_FREQUENCY(dev_priv->rps.rp1_freq * GEN9_FREQ_SCALER));
+	I915_WRITE(GEN6_RC_VIDEO_FREQ,
+		GEN9_FREQUENCY(dev_priv->rps.rp1_freq * GEN9_FREQ_SCALER));
+
+	ei_up = 84480; /* 84.48ms */
+	ei_down = 448000;
+	threshold_up_pct = 90; /* x% */
+	threshold_down_pct = 70;
+
+	I915_WRITE(GEN6_RP_UP_EI,
+		GT_FREQ_FROM_PERIOD(ei_up, dev));
+	I915_WRITE(GEN6_RP_UP_THRESHOLD,
+		GT_FREQ_FROM_PERIOD((ei_up * threshold_up_pct / 100), dev));
+
+	I915_WRITE(GEN6_RP_DOWN_EI,
+		GT_FREQ_FROM_PERIOD(ei_down, dev));
+	I915_WRITE(GEN6_RP_DOWN_THRESHOLD,
+		GT_FREQ_FROM_PERIOD((ei_down * threshold_down_pct / 100), dev));
+
+	/* 1 second timeout*/
+	I915_WRITE(GEN6_RP_DOWN_TIMEOUT, GT_FREQ_FROM_PERIOD(1000000, dev));
+
+	I915_WRITE(GEN6_RP_INTERRUPT_LIMITS,
+		(dev_priv->rps.max_freq_softlimit * GEN9_FREQ_SCALER) << 23 |
+		(dev_priv->rps.min_freq_softlimit * GEN9_FREQ_SCALER) << 14);
 
-	I915_WRITE(GEN6_RP_DOWN_TIMEOUT, 0xf4240);
-	I915_WRITE(GEN6_RP_INTERRUPT_LIMITS, 0x12060000);
-	I915_WRITE(GEN6_RP_UP_THRESHOLD, 0xe808);
-	I915_WRITE(GEN6_RP_DOWN_THRESHOLD, 0x3bd08);
-	I915_WRITE(GEN6_RP_UP_EI, 0x101d0);
-	I915_WRITE(GEN6_RP_DOWN_EI, 0x55730);
 	I915_WRITE(GEN6_RP_IDLE_HYSTERSIS, 0xa);
-	I915_WRITE(GEN6_PMINTRMSK, 0x6);
 	I915_WRITE(GEN6_RP_CONTROL, GEN6_RP_MEDIA_TURBO |
 		   GEN6_RP_MEDIA_HW_MODE | GEN6_RP_MEDIA_IS_GFX |
 		   GEN6_RP_ENABLE | GEN6_RP_UP_BUSY_AVG |
 		   GEN6_RP_DOWN_IDLE_AVG);
 
+	dev_priv->rps.power = HIGH_POWER; /* force a reset */
+	gen6_set_rps(dev_priv->dev, dev_priv->rps.min_freq_softlimit);
+
 	gen6_enable_rps_interrupts(dev);
 
 	intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
-- 
1.9.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 5/7] drm/i915/skl: Updated the gen9_enable_rps function

[Damien Lespiau]

On Fri, Feb 06, 2015 at 08:26:36PM +0530, akash.goel@intel.com wrote:
> From: Akash Goel <akash.goel@intel.com>
> 
> On SKL, GT frequency is programmed in units of 16.66 MHZ units compared
> to 50 MHZ for older platforms. Also the time value specified for Up/Down EI &
> Up/Down thresholds are expressed in units of 1.33 us, compared to 1.28
> us for older platforms. So updated the gen9_enable_rps function as per that.
> 
> Signed-off-by: Akash Goel <akash.goel@intel.com>

The function was following exactly what is in the PM programming guide,
so it should be correct, not sure we want to change that. Your version
looks more readable though. 

I think I'd rather keep the close to the PM guide, but others may
disagree.

-- 
Damien

> ---
>  drivers/gpu/drm/i915/intel_pm.c | 41 ++++++++++++++++++++++++++++++++---------
>  1 file changed, 32 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index db24b48..865df1f 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -4037,27 +4037,50 @@ static void gen6_init_rps_frequencies(struct drm_device *dev)
>  static void gen9_enable_rps(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> +	u32 threshold_up_pct, threshold_down_pct;
> +	u32 ei_up, ei_down;
>  
>  	intel_uncore_forcewake_get(dev_priv, FORCEWAKE_ALL);
>  
>  	gen6_init_rps_frequencies(dev);
>  
> -	I915_WRITE(GEN6_RPNSWREQ, 0xc800000);
> -	I915_WRITE(GEN6_RC_VIDEO_FREQ, 0xc800000);
> +	/* Program defaults and thresholds for RPS*/
> +	I915_WRITE(GEN6_RPNSWREQ,
> +		GEN9_FREQUENCY(dev_priv->rps.rp1_freq * GEN9_FREQ_SCALER));
> +	I915_WRITE(GEN6_RC_VIDEO_FREQ,
> +		GEN9_FREQUENCY(dev_priv->rps.rp1_freq * GEN9_FREQ_SCALER));
> +
> +	ei_up = 84480; /* 84.48ms */
> +	ei_down = 448000;
> +	threshold_up_pct = 90; /* x% */
> +	threshold_down_pct = 70;
> +
> +	I915_WRITE(GEN6_RP_UP_EI,
> +		GT_FREQ_FROM_PERIOD(ei_up, dev));
> +	I915_WRITE(GEN6_RP_UP_THRESHOLD,
> +		GT_FREQ_FROM_PERIOD((ei_up * threshold_up_pct / 100), dev));
> +
> +	I915_WRITE(GEN6_RP_DOWN_EI,
> +		GT_FREQ_FROM_PERIOD(ei_down, dev));
> +	I915_WRITE(GEN6_RP_DOWN_THRESHOLD,
> +		GT_FREQ_FROM_PERIOD((ei_down * threshold_down_pct / 100), dev));
> +
> +	/* 1 second timeout*/
> +	I915_WRITE(GEN6_RP_DOWN_TIMEOUT, GT_FREQ_FROM_PERIOD(1000000, dev));
> +
> +	I915_WRITE(GEN6_RP_INTERRUPT_LIMITS,
> +		(dev_priv->rps.max_freq_softlimit * GEN9_FREQ_SCALER) << 23 |
> +		(dev_priv->rps.min_freq_softlimit * GEN9_FREQ_SCALER) << 14);
>  
> -	I915_WRITE(GEN6_RP_DOWN_TIMEOUT, 0xf4240);
> -	I915_WRITE(GEN6_RP_INTERRUPT_LIMITS, 0x12060000);
> -	I915_WRITE(GEN6_RP_UP_THRESHOLD, 0xe808);
> -	I915_WRITE(GEN6_RP_DOWN_THRESHOLD, 0x3bd08);
> -	I915_WRITE(GEN6_RP_UP_EI, 0x101d0);
> -	I915_WRITE(GEN6_RP_DOWN_EI, 0x55730);
>  	I915_WRITE(GEN6_RP_IDLE_HYSTERSIS, 0xa);
> -	I915_WRITE(GEN6_PMINTRMSK, 0x6);
>  	I915_WRITE(GEN6_RP_CONTROL, GEN6_RP_MEDIA_TURBO |
>  		   GEN6_RP_MEDIA_HW_MODE | GEN6_RP_MEDIA_IS_GFX |
>  		   GEN6_RP_ENABLE | GEN6_RP_UP_BUSY_AVG |
>  		   GEN6_RP_DOWN_IDLE_AVG);
>  
> +	dev_priv->rps.power = HIGH_POWER; /* force a reset */
> +	gen6_set_rps(dev_priv->dev, dev_priv->rps.min_freq_softlimit);
> +
>  	gen6_enable_rps_interrupts(dev);
>  
>  	intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
> -- 
> 1.9.2
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 5/7] drm/i915/skl: Updated the gen9_enable_rps function

[Akash Goel]

On Tue, 2015-02-17 at 15:47 +0000, Damien Lespiau wrote:
> On Fri, Feb 06, 2015 at 08:26:36PM +0530, akash.goel@intel.com wrote:
> > From: Akash Goel <akash.goel@intel.com>
> > 
> > On SKL, GT frequency is programmed in units of 16.66 MHZ units compared
> > to 50 MHZ for older platforms. Also the time value specified for Up/Down EI &
> > Up/Down thresholds are expressed in units of 1.33 us, compared to 1.28
> > us for older platforms. So updated the gen9_enable_rps function as per that.
> > 
> > Signed-off-by: Akash Goel <akash.goel@intel.com>
> 
> The function was following exactly what is in the PM programming guide,
> so it should be correct, not sure we want to change that. Your version
> looks more readable though. 
> 
> I think I'd rather keep the close to the PM guide, but others may
> disagree.

Actually I am not sure, whether some of the values provided in the
programming guide are correct or not.

As per the values programmed for EI intervals, it seemed that was done
with an assumption of 1.28 us unit.
Like	0x101d0 * 1.28 = 84.8 ms (Standard value of Up EI)
	0x55730 * 1.28 = 448 ms  (Standard value of Down EI)
	0xe808  * 1.28 = 76 ms (90% of Up EI)
	0x3bd08 * 1.28 = 313 ms (70% of Down EI)

Also the value 0xf4240 (1000000 or 1 second), used for
GEN6_RP_DOWN_TIMEOUT, wasn't converted appropriately to time units.


Best regards
Akash

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 6/7] drm/i915/skl: Updated the 'i915_frequency_info' debugs function

[akash.goel]

From: Akash Goel <akash.goel@intel.com>

Added support for SKL in the 'i915_frequency_info' debugfs function

Signed-off-by: Akash Goel <akash.goel@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 9af17fb..32c62a2 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1089,7 +1089,7 @@ static int i915_frequency_info(struct seq_file *m, void *unused)
 		seq_printf(m, "Current P-state: %d\n",
 			   (rgvstat & MEMSTAT_PSTATE_MASK) >> MEMSTAT_PSTATE_SHIFT);
 	} else if (IS_GEN6(dev) || (IS_GEN7(dev) && !IS_VALLEYVIEW(dev)) ||
-		   IS_BROADWELL(dev)) {
+		   IS_BROADWELL(dev) || IS_GEN9(dev)) {
 		u32 gt_perf_status = I915_READ(GEN6_GT_PERF_STATUS);
 		u32 rp_state_limits = I915_READ(GEN6_RP_STATE_LIMITS);
 		u32 rp_state_cap = I915_READ(GEN6_RP_STATE_CAP);
@@ -1109,8 +1109,12 @@ static int i915_frequency_info(struct seq_file *m, void *unused)
 
 		reqf = I915_READ(GEN6_RPNSWREQ);
 		reqf &= ~GEN6_TURBO_DISABLE;
+		if (!IS_GEN9(dev))
+			reqf &= ~GEN6_TURBO_DISABLE;
 		if (IS_HASWELL(dev) || IS_BROADWELL(dev))
 			reqf >>= 24;
+		else if IS_GEN9(dev)
+			reqf >>= 23;
 		else
 			reqf >>= 25;
 		reqf = intel_gpu_freq(dev_priv, reqf);
@@ -1128,6 +1132,8 @@ static int i915_frequency_info(struct seq_file *m, void *unused)
 		rpprevdown = I915_READ(GEN6_RP_PREV_DOWN);
 		if (IS_HASWELL(dev) || IS_BROADWELL(dev))
 			cagf = (rpstat & HSW_CAGF_MASK) >> HSW_CAGF_SHIFT;
+		else if (IS_GEN9(dev))
+			cagf = (rpstat & GEN9_CAGF_MASK) >> GEN9_CAGF_SHIFT;
 		else
 			cagf = (rpstat & GEN6_CAGF_MASK) >> GEN6_CAGF_SHIFT;
 		cagf = intel_gpu_freq(dev_priv, cagf);
@@ -1152,7 +1158,7 @@ static int i915_frequency_info(struct seq_file *m, void *unused)
 			   pm_ier, pm_imr, pm_isr, pm_iir, pm_mask);
 		seq_printf(m, "GT_PERF_STATUS: 0x%08x\n", gt_perf_status);
 		seq_printf(m, "Render p-state ratio: %d\n",
-			   (gt_perf_status & 0xff00) >> 8);
+			   (gt_perf_status & (IS_GEN9(dev) ? 0x1ff00 : 0xff00)) >> 8);
 		seq_printf(m, "Render p-state VID: %d\n",
 			   gt_perf_status & 0xff);
 		seq_printf(m, "Render p-state limit: %d\n",
-- 
1.9.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 6/7] drm/i915/skl: Updated the 'i915_frequency_info' debugs function

[Damien Lespiau]

On Fri, Feb 06, 2015 at 08:26:37PM +0530, akash.goel@intel.com wrote:
> From: Akash Goel <akash.goel@intel.com>
> 
> Added support for SKL in the 'i915_frequency_info' debugfs function
> 
> Signed-off-by: Akash Goel <akash.goel@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 9af17fb..32c62a2 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -1089,7 +1089,7 @@ static int i915_frequency_info(struct seq_file *m, void *unused)
>  		seq_printf(m, "Current P-state: %d\n",
>  			   (rgvstat & MEMSTAT_PSTATE_MASK) >> MEMSTAT_PSTATE_SHIFT);
>  	} else if (IS_GEN6(dev) || (IS_GEN7(dev) && !IS_VALLEYVIEW(dev)) ||
> -		   IS_BROADWELL(dev)) {
> +		   IS_BROADWELL(dev) || IS_GEN9(dev)) {

Can we be optimistic by default (and hope next platform will be no extra
work by having a INTEL_INFO(dev)->gen >= 9)?

>  		u32 gt_perf_status = I915_READ(GEN6_GT_PERF_STATUS);
>  		u32 rp_state_limits = I915_READ(GEN6_RP_STATE_LIMITS);
>  		u32 rp_state_cap = I915_READ(GEN6_RP_STATE_CAP);
> @@ -1109,8 +1109,12 @@ static int i915_frequency_info(struct seq_file *m, void *unused)
>  
>  		reqf = I915_READ(GEN6_RPNSWREQ);
>  		reqf &= ~GEN6_TURBO_DISABLE;
> +		if (!IS_GEN9(dev))
> +			reqf &= ~GEN6_TURBO_DISABLE;

It seems like you to remove one masking of bit 31? (can we have >= 9 as
well?).  

Maybe a simpler way to go about it would be:

		if (INTEL_INFO(dev)->gen >= 9)
			reqf >>= 23;
		else {
			reqf &= ~GEN6_TURBO_DISABLE;
			if (IS_HASWELL(dev) || IS_BROADWELL(dev))
				reqf >>= 24;
			else
				reqf >>= 25;
		}

>  		if (IS_HASWELL(dev) || IS_BROADWELL(dev))
>  			reqf >>= 24;
> +		else if IS_GEN9(dev)
> +			reqf >>= 23;
>  		else
>  			reqf >>= 25;
>  		reqf = intel_gpu_freq(dev_priv, reqf);
> @@ -1128,6 +1132,8 @@ static int i915_frequency_info(struct seq_file *m, void *unused)
>  		rpprevdown = I915_READ(GEN6_RP_PREV_DOWN);
>  		if (IS_HASWELL(dev) || IS_BROADWELL(dev))
>  			cagf = (rpstat & HSW_CAGF_MASK) >> HSW_CAGF_SHIFT;
> +		else if (IS_GEN9(dev))
> +			cagf = (rpstat & GEN9_CAGF_MASK) >> GEN9_CAGF_SHIFT;
>  		else
>  			cagf = (rpstat & GEN6_CAGF_MASK) >> GEN6_CAGF_SHIFT;

cagf (as well as reqf) is(are) used in a printf saying they are Mhz. That looks
wrong.

>  		cagf = intel_gpu_freq(dev_priv, cagf);
> @@ -1152,7 +1158,7 @@ static int i915_frequency_info(struct seq_file *m, void *unused)
>  			   pm_ier, pm_imr, pm_isr, pm_iir, pm_mask);
>  		seq_printf(m, "GT_PERF_STATUS: 0x%08x\n", gt_perf_status);
>  		seq_printf(m, "Render p-state ratio: %d\n",
> -			   (gt_perf_status & 0xff00) >> 8);
> +			   (gt_perf_status & (IS_GEN9(dev) ? 0x1ff00 : 0xff00)) >> 8);

Eeek, that's a weird name to say freq. Here the 16.66 unit strikes back, can we
have at least a comment?

>  		seq_printf(m, "Render p-state VID: %d\n",
>  			   gt_perf_status & 0xff);
>  		seq_printf(m, "Render p-state limit: %d\n",
> -- 
> 1.9.2
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 6/7] drm/i915/skl: Updated the 'i915_frequency_info' debugs function

[Akash Goel]

On Tue, 2015-02-17 at 15:38 +0000, Damien Lespiau wrote:
> On Fri, Feb 06, 2015 at 08:26:37PM +0530, akash.goel@intel.com wrote:
> > From: Akash Goel <akash.goel@intel.com>
> > 
> > Added support for SKL in the 'i915_frequency_info' debugfs function
> > 
> > Signed-off-by: Akash Goel <akash.goel@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_debugfs.c | 10 ++++++++--
> >  1 file changed, 8 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> > index 9af17fb..32c62a2 100644
> > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > @@ -1089,7 +1089,7 @@ static int i915_frequency_info(struct seq_file *m, void *unused)
> >  		seq_printf(m, "Current P-state: %d\n",
> >  			   (rgvstat & MEMSTAT_PSTATE_MASK) >> MEMSTAT_PSTATE_SHIFT);
> >  	} else if (IS_GEN6(dev) || (IS_GEN7(dev) && !IS_VALLEYVIEW(dev)) ||
> > -		   IS_BROADWELL(dev)) {
> > +		   IS_BROADWELL(dev) || IS_GEN9(dev)) {
> 
> Can we be optimistic by default (and hope next platform will be no extra
> work by having a INTEL_INFO(dev)->gen >= 9)?
Sorry I am not sure..
If it turns out, that there is no extra work required for future
platforms, then at that time we can do >= 9 change. 

> 
> >  		u32 gt_perf_status = I915_READ(GEN6_GT_PERF_STATUS);
> >  		u32 rp_state_limits = I915_READ(GEN6_RP_STATE_LIMITS);
> >  		u32 rp_state_cap = I915_READ(GEN6_RP_STATE_CAP);
> > @@ -1109,8 +1109,12 @@ static int i915_frequency_info(struct seq_file *m, void *unused)
> >  
> >  		reqf = I915_READ(GEN6_RPNSWREQ);
> >  		reqf &= ~GEN6_TURBO_DISABLE;
> > +		if (!IS_GEN9(dev))
> > +			reqf &= ~GEN6_TURBO_DISABLE;
> 
> It seems like you to remove one masking of bit 31? (can we have >= 9 as
> well?).  
> 
> Maybe a simpler way to go about it would be:

Thanks for spotting this, will modify it as per your suggestion.
> 
> 		if (INTEL_INFO(dev)->gen >= 9)
> 			reqf >>= 23;
> 		else {
> 			reqf &= ~GEN6_TURBO_DISABLE;
> 			if (IS_HASWELL(dev) || IS_BROADWELL(dev))
> 				reqf >>= 24;
> 			else
> 				reqf >>= 25;
> 		}
> 
> >  		if (IS_HASWELL(dev) || IS_BROADWELL(dev))
> >  			reqf >>= 24;
> > +		else if IS_GEN9(dev)
> > +			reqf >>= 23;
> >  		else
> >  			reqf >>= 25;
> >  		reqf = intel_gpu_freq(dev_priv, reqf);
> > @@ -1128,6 +1132,8 @@ static int i915_frequency_info(struct seq_file *m, void *unused)
> >  		rpprevdown = I915_READ(GEN6_RP_PREV_DOWN);
> >  		if (IS_HASWELL(dev) || IS_BROADWELL(dev))
> >  			cagf = (rpstat & HSW_CAGF_MASK) >> HSW_CAGF_SHIFT;
> > +		else if (IS_GEN9(dev))
> > +			cagf = (rpstat & GEN9_CAGF_MASK) >> GEN9_CAGF_SHIFT;
> >  		else
> >  			cagf = (rpstat & GEN6_CAGF_MASK) >> GEN6_CAGF_SHIFT;
> 
> cagf (as well as reqf) is(are) used in a printf saying they are Mhz. That looks
> wrong.

There is a conversion done below, before the printf, through
"cagf = intel_gpu_freq(dev_priv, cagf)"
So should be alright then ?
But not able to find the precise info, that frequency specified in
0xA01C register, is in which units for GEN9.

For the reqf, there is a conversion missing from 16.667 MHZ units, for
GEN9. Will add that.

> 
> >  		cagf = intel_gpu_freq(dev_priv, cagf);
> > @@ -1152,7 +1158,7 @@ static int i915_frequency_info(struct seq_file *m, void *unused)
> >  			   pm_ier, pm_imr, pm_isr, pm_iir, pm_mask);
> >  		seq_printf(m, "GT_PERF_STATUS: 0x%08x\n", gt_perf_status);
> >  		seq_printf(m, "Render p-state ratio: %d\n",
> > -			   (gt_perf_status & 0xff00) >> 8);
> > +			   (gt_perf_status & (IS_GEN9(dev) ? 0x1ff00 : 0xff00)) >> 8);
> 
> Eeek, that's a weird name to say freq. Here the 16.66 unit strikes back, can we
> have at least a comment?
Sorry didn't get this point. Just printing the raw value of P State
ratio (Un-Slice) in GT_PERF_STATUS register.
> 
> >  		seq_printf(m, "Render p-state VID: %d\n",
> >  			   gt_perf_status & 0xff);
> >  		seq_printf(m, "Render p-state limit: %d\n",
> > -- 
> > 1.9.2
> > 

Best Regards
Akash

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 7/7] drm/i915/skl: Enabling processing of Turbo interrupts

[akash.goel]

From: Akash Goel <akash.goel@intel.com>

Earlier Turbo interrupts were not being processed for SKL,
as something was amiss in turbo programming for SKL.
Now missing changes have been added, so enabling the Turbo
interrupt processing for SKL.

Signed-off-by: Akash Goel <akash.goel@intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index e6342da..333c0f8 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1745,11 +1745,6 @@ static void i9xx_pipe_crc_irq_handler(struct drm_device *dev, enum pipe pipe)
  * the work queue. */
 static void gen6_rps_irq_handler(struct drm_i915_private *dev_priv, u32 pm_iir)
 {
-	/* TODO: RPS on GEN9+ is not supported yet. */
-	if (WARN_ONCE(INTEL_INFO(dev_priv)->gen >= 9,
-		      "GEN9+: unexpected RPS IRQ\n"))
-		return;
-
 	if (pm_iir & dev_priv->pm_rps_events) {
 		spin_lock(&dev_priv->irq_lock);
 		gen6_disable_pm_irq(dev_priv, pm_iir & dev_priv->pm_rps_events);
-- 
1.9.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 7/7] drm/i915/skl: Enabling processing of Turbo interrupts

[shuang.he]

Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 5728
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV              +1                 282/283              283/283
ILK                 -2              316/319              314/319
SNB                                  322/346              322/346
IVB                 -1              382/384              381/384
BYT                 -1              296/296              295/296
HSW                 -2              425/428              423/428
BDW                                  318/333              318/333
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
 PNV  igt_gen3_render_linear_blits      FAIL(3, M7)CRASH(1, M23)PASS(5, M25M23)      PASS(1, M25)
 ILK  igt_drv_suspend_fence-restore-tiled2untiled      DMESG_WARN(2, M37)PASS(1, M26)      DMESG_WARN(1, M37)
 ILK  igt_drv_suspend_fence-restore-untiled      DMESG_WARN(2, M37)PASS(1, M26)      DMESG_WARN(1, M37)
 IVB  igt_gem_pwrite_pread_snooped-pwrite-blt-cpu_mmap-performance      DMESG_WARN(2, M34)PASS(6, M21M34)      DMESG_WARN(1, M21)
*BYT  igt_gem_dummy_reloc_loop_mixed_multi_fd      PASS(2, M50M48)      DMESG_WARN(1, M48)
 HSW  igt_gem_storedw_loop_blt      DMESG_WARN(1, M20)PASS(2, M20)      DMESG_WARN(1, M20)
*HSW  igt_gem_storedw_loop_bsd      PASS(2, M20)      DMESG_WARN(1, M20)
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 7/7] drm/i915/skl: Enabling processing of Turbo interrupts

[Damien Lespiau]

On Fri, Feb 06, 2015 at 08:26:38PM +0530, akash.goel@intel.com wrote:
> From: Akash Goel <akash.goel@intel.com>
> 
> Earlier Turbo interrupts were not being processed for SKL,
> as something was amiss in turbo programming for SKL.
> Now missing changes have been added, so enabling the Turbo
> interrupt processing for SKL.
> 
> Signed-off-by: Akash Goel <akash.goel@intel.com>

Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>


> ---
>  drivers/gpu/drm/i915/i915_irq.c | 5 -----
>  1 file changed, 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index e6342da..333c0f8 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -1745,11 +1745,6 @@ static void i9xx_pipe_crc_irq_handler(struct drm_device *dev, enum pipe pipe)
>   * the work queue. */
>  static void gen6_rps_irq_handler(struct drm_i915_private *dev_priv, u32 pm_iir)
>  {
> -	/* TODO: RPS on GEN9+ is not supported yet. */
> -	if (WARN_ONCE(INTEL_INFO(dev_priv)->gen >= 9,
> -		      "GEN9+: unexpected RPS IRQ\n"))
> -		return;
> -
>  	if (pm_iir & dev_priv->pm_rps_events) {
>  		spin_lock(&dev_priv->irq_lock);
>  		gen6_disable_pm_irq(dev_priv, pm_iir & dev_priv->pm_rps_events);
> -- 
> 1.9.2
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 0/7] Added missing changes for Turbo feature on SKL

[Damien Lespiau]

On Fri, Feb 06, 2015 at 08:26:31PM +0530, akash.goel@intel.com wrote:
> From: Akash Goel <akash.goel@intel.com>
> 
> This patch series add missing changes, required for proper functioning of the
> Turbo feature on SKL.

Something I noticed from reading the PM code is that we get the RPe
frequency from the punit for HSW/BDW, do you know if we should do the
same for SKL? (I'm taking about the couple of if (IS_HSW | IS_BDW) in
gen6_init_rps_frequencies()).

-- 
Damien
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx