[PATCH 1/2] drm/i915/skl: Retrieve the Rpe value from Pcode

[PATCH 1/2] drm/i915/skl: Retrieve the Rpe value from Pcode

[akash.goel]

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

Read the efficient frequency (aka RPe) value through the the mailbox
command (0x1A) from the pcode, as done on Haswell and Broadwell.
The turbo minimum frequency softlimit is not revised as per the
efficient frequency value.

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

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index a7516ed..421b78d 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4274,16 +4274,20 @@ static void gen6_init_rps_frequencies(struct drm_device *dev)
 	dev_priv->rps.max_freq		= dev_priv->rps.rp0_freq;
 
 	dev_priv->rps.efficient_freq = dev_priv->rps.rp1_freq;
-	if (IS_HASWELL(dev) || IS_BROADWELL(dev)) {
+	if (IS_HASWELL(dev) || IS_BROADWELL(dev) || IS_SKYLAKE(dev)) {
 		ret = sandybridge_pcode_read(dev_priv,
 					HSW_PCODE_DYNAMIC_DUTY_CYCLE_CONTROL,
 					&ddcc_status);
-		if (0 == ret)
+		if (0 == ret) {
 			dev_priv->rps.efficient_freq =
 				clamp_t(u8,
 					((ddcc_status >> 8) & 0xff),
 					dev_priv->rps.min_freq,
 					dev_priv->rps.max_freq);
+
+			dev_priv->rps.efficient_freq *=
+				(IS_SKYLAKE(dev) ? GEN9_FREQ_SCALER : 1);
+		}
 	}
 
 	dev_priv->rps.idle_freq = dev_priv->rps.min_freq;
-- 
1.9.2

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

[PATCH 2/2] drm/i915/skl: Ring frequency table programming changes

[akash.goel]

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

Ring frequency table programming changes for SKL. No need for a
floor on ring frequency, as the issue of performance impact with
ring running below DDR frequency, is believed to be fixed on SKL

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

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 421b78d..d1bdea7 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4582,6 +4582,7 @@ static void __gen6_update_ring_freq(struct drm_device *dev)
 	int min_freq = 15;
 	unsigned int gpu_freq;
 	unsigned int max_ia_freq, min_ring_freq;
+	unsigned int max_gpu_freq, min_gpu_freq;
 	int scaling_factor = 180;
 	struct cpufreq_policy *policy;
 
@@ -4606,17 +4607,31 @@ static void __gen6_update_ring_freq(struct drm_device *dev)
 	/* convert DDR frequency from units of 266.6MHz to bandwidth */
 	min_ring_freq = mult_frac(min_ring_freq, 8, 3);
 
+	if (IS_SKYLAKE(dev)) {
+		/* Convert GT frequency to 50 HZ units */
+		min_gpu_freq = dev_priv->rps.min_freq / GEN9_FREQ_SCALER;
+		max_gpu_freq = dev_priv->rps.max_freq / GEN9_FREQ_SCALER;
+	} else {
+		min_gpu_freq = dev_priv->rps.min_freq;
+		max_gpu_freq = dev_priv->rps.max_freq;
+	}
+
 	/*
 	 * For each potential GPU frequency, load a ring frequency we'd like
 	 * to use for memory access.  We do this by specifying the IA frequency
 	 * the PCU should use as a reference to determine the ring frequency.
 	 */
-	for (gpu_freq = dev_priv->rps.max_freq; gpu_freq >= dev_priv->rps.min_freq;
-	     gpu_freq--) {
-		int diff = dev_priv->rps.max_freq - gpu_freq;
+	for (gpu_freq = max_gpu_freq; gpu_freq >= min_gpu_freq; gpu_freq--) {
+		int diff = max_gpu_freq - gpu_freq;
 		unsigned int ia_freq = 0, ring_freq = 0;
 
-		if (INTEL_INFO(dev)->gen >= 8) {
+		if (IS_SKYLAKE(dev)) {
+			/*
+			 * ring_freq = 2 * GT. ring_freq is in 100MHz units
+			 * No floor required for ring frequency on SKL.
+			 */
+			ring_freq = gpu_freq;
+		} else if (INTEL_INFO(dev)->gen >= 8) {
 			/* max(2 * GT, DDR). NB: GT is 50MHz units */
 			ring_freq = max(min_ring_freq, gpu_freq);
 		} else if (IS_HASWELL(dev)) {
@@ -5770,7 +5785,8 @@ static void intel_gen6_powersave_work(struct work_struct *work)
 	} else if (INTEL_INFO(dev)->gen >= 9) {
 		gen9_enable_rc6(dev);
 		gen9_enable_rps(dev);
-		__gen6_update_ring_freq(dev);
+		if (IS_SKYLAKE(dev))
+			__gen6_update_ring_freq(dev);
 	} else if (IS_BROADWELL(dev)) {
 		gen8_enable_rps(dev);
 		__gen6_update_ring_freq(dev);
-- 
1.9.2

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

Re: [PATCH 2/2] drm/i915/skl: Ring frequency table programming changes

[shuang.he]

Tested-By: Intel Graphics QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 6320
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV                                  276/276              276/276
ILK                 -1              302/302              301/302
SNB                                  316/316              316/316
IVB                                  342/342              342/342
BYT                                  286/286              286/286
BDW                                  318/318              318/318
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
*ILK  igt@kms_flip@flip-vs-dpms-interruptible      PASS(2)      DMESG_WARN(1)PASS(1)
(dmesg patch applied)drm:intel_pch_fifo_underrun_irq_handler[i915]]*ERROR*PCH_transcoder_A_FIFO_underrun@PCH transcoder A FIFO underrun
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

[PATCH 3/3] drm/i915/skl: Updated the i915_ring_freq_table debugfs function

[akash.goel]

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

Updated the i915_ring_freq_table debugfs function to allow read of ring
frequency table through Punit interface, for SKL also.

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

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 9e5a56c..32527de 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1769,8 +1769,9 @@ static int i915_ring_freq_table(struct seq_file *m, void *unused)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	int ret = 0;
 	int gpu_freq, ia_freq;
+	unsigned int max_gpu_freq, min_gpu_freq;
 
-	if (!(IS_GEN6(dev) || IS_GEN7(dev))) {
+	if (!(IS_GEN6(dev) || IS_GEN7(dev) || IS_SKYLAKE(dev))) {
 		seq_puts(m, "unsupported on this chipset\n");
 		return 0;
 	}
@@ -1783,17 +1784,27 @@ static int i915_ring_freq_table(struct seq_file *m, void *unused)
 	if (ret)
 		goto out;
 
+	if (IS_SKYLAKE(dev)) {
+		/* Convert GT frequency to 50 HZ units */
+		min_gpu_freq =
+			dev_priv->rps.min_freq_softlimit / GEN9_FREQ_SCALER;
+		max_gpu_freq =
+			dev_priv->rps.max_freq_softlimit / GEN9_FREQ_SCALER;
+	} else {
+		min_gpu_freq = dev_priv->rps.min_freq_softlimit;
+		max_gpu_freq = dev_priv->rps.max_freq_softlimit;
+	}
+
 	seq_puts(m, "GPU freq (MHz)\tEffective CPU freq (MHz)\tEffective Ring freq (MHz)\n");
 
-	for (gpu_freq = dev_priv->rps.min_freq_softlimit;
-	     gpu_freq <= dev_priv->rps.max_freq_softlimit;
-	     gpu_freq++) {
+	for (gpu_freq = min_gpu_freq; gpu_freq <= max_gpu_freq; gpu_freq++) {
 		ia_freq = gpu_freq;
 		sandybridge_pcode_read(dev_priv,
 				       GEN6_PCODE_READ_MIN_FREQ_TABLE,
 				       &ia_freq);
 		seq_printf(m, "%d\t\t%d\t\t\t\t%d\n",
-			   intel_gpu_freq(dev_priv, gpu_freq),
+			   intel_gpu_freq(dev_priv, (gpu_freq *
+				(IS_SKYLAKE(dev) ? GEN9_FREQ_SCALER : 1))),
 			   ((ia_freq >> 0) & 0xff) * 100,
 			   ((ia_freq >> 8) & 0xff) * 100);
 	}
-- 
1.9.2

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

Re: [PATCH 1/2] drm/i915/skl: Retrieve the Rpe value from Pcode

[Rodrigo Vivi]

On Tue, May 5, 2015 at 4:30 AM,  <akash.goel@intel.com> wrote:
> From: Akash Goel <akash.goel@intel.com>
>
> Read the efficient frequency (aka RPe) value through the the mailbox
> command (0x1A) from the pcode, as done on Haswell and Broadwell.
> The turbo minimum frequency softlimit is not revised as per the
> efficient frequency value.
>
> Issue: VIZ-5143
> Signed-off-by: Akash Goel <akash.goel@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index a7516ed..421b78d 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -4274,16 +4274,20 @@ static void gen6_init_rps_frequencies(struct drm_device *dev)
>         dev_priv->rps.max_freq          = dev_priv->rps.rp0_freq;
>
>         dev_priv->rps.efficient_freq = dev_priv->rps.rp1_freq;
> -       if (IS_HASWELL(dev) || IS_BROADWELL(dev)) {
> +       if (IS_HASWELL(dev) || IS_BROADWELL(dev) || IS_SKYLAKE(dev)) {

Could you please give me a pointer in the spec to this? Couldn't find
on new one.

>                 ret = sandybridge_pcode_read(dev_priv,
>                                         HSW_PCODE_DYNAMIC_DUTY_CYCLE_CONTROL,
>                                         &ddcc_status);
> -               if (0 == ret)
> +               if (0 == ret) {
>                         dev_priv->rps.efficient_freq =
>                                 clamp_t(u8,
>                                         ((ddcc_status >> 8) & 0xff),
>                                         dev_priv->rps.min_freq,
>                                         dev_priv->rps.max_freq);
> +
> +                       dev_priv->rps.efficient_freq *=
> +                               (IS_SKYLAKE(dev) ? GEN9_FREQ_SCALER : 1);

I thought you were managing everything on 50MHz and converting to 16.6
only when writing. So this here seems to me that will compare to
values in different bases..

> +               }
>         }
>
>         dev_priv->rps.idle_freq = dev_priv->rps.min_freq;
> --
> 1.9.2
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Rodrigo Vivi
Blog: http://blog.vivi.eng.br
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915/skl: Ring frequency table programming changes

[Rodrigo Vivi]

On Tue, May 5, 2015 at 4:30 AM,  <akash.goel@intel.com> wrote:
> From: Akash Goel <akash.goel@intel.com>
>
> Ring frequency table programming changes for SKL. No need for a
> floor on ring frequency, as the issue of performance impact with
> ring running below DDR frequency, is believed to be fixed on SKL
>
> Issue: VIZ-5144
> Signed-off-by: Akash Goel <akash.goel@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 26 +++++++++++++++++++++-----
>  1 file changed, 21 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 421b78d..d1bdea7 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -4582,6 +4582,7 @@ static void __gen6_update_ring_freq(struct drm_device *dev)
>         int min_freq = 15;
>         unsigned int gpu_freq;
>         unsigned int max_ia_freq, min_ring_freq;
> +       unsigned int max_gpu_freq, min_gpu_freq;
>         int scaling_factor = 180;
>         struct cpufreq_policy *policy;
>
> @@ -4606,17 +4607,31 @@ static void __gen6_update_ring_freq(struct drm_device *dev)
>         /* convert DDR frequency from units of 266.6MHz to bandwidth */
>         min_ring_freq = mult_frac(min_ring_freq, 8, 3);
>
> +       if (IS_SKYLAKE(dev)) {
> +               /* Convert GT frequency to 50 HZ units */
> +               min_gpu_freq = dev_priv->rps.min_freq / GEN9_FREQ_SCALER;
> +               max_gpu_freq = dev_priv->rps.max_freq / GEN9_FREQ_SCALER;

I got even more confused here with the scale differences... Doesn't
look so consistent..

> +       } else {
> +               min_gpu_freq = dev_priv->rps.min_freq;
> +               max_gpu_freq = dev_priv->rps.max_freq;
> +       }
> +
>         /*
>          * For each potential GPU frequency, load a ring frequency we'd like
>          * to use for memory access.  We do this by specifying the IA frequency
>          * the PCU should use as a reference to determine the ring frequency.
>          */
> -       for (gpu_freq = dev_priv->rps.max_freq; gpu_freq >= dev_priv->rps.min_freq;
> -            gpu_freq--) {
> -               int diff = dev_priv->rps.max_freq - gpu_freq;
> +       for (gpu_freq = max_gpu_freq; gpu_freq >= min_gpu_freq; gpu_freq--) {
> +               int diff = max_gpu_freq - gpu_freq;
>                 unsigned int ia_freq = 0, ring_freq = 0;
>
> -               if (INTEL_INFO(dev)->gen >= 8) {
> +               if (IS_SKYLAKE(dev)) {
> +                       /*
> +                        * ring_freq = 2 * GT. ring_freq is in 100MHz units
> +                        * No floor required for ring frequency on SKL.
> +                        */
> +                       ring_freq = gpu_freq;

Aren't we using the 16.6 now? Shouldn't it be converted anyway?

> +               } else if (INTEL_INFO(dev)->gen >= 8) {
>                         /* max(2 * GT, DDR). NB: GT is 50MHz units */
>                         ring_freq = max(min_ring_freq, gpu_freq);
>                 } else if (IS_HASWELL(dev)) {
> @@ -5770,7 +5785,8 @@ static void intel_gen6_powersave_work(struct work_struct *work)
>         } else if (INTEL_INFO(dev)->gen >= 9) {
>                 gen9_enable_rc6(dev);
>                 gen9_enable_rps(dev);
> -               __gen6_update_ring_freq(dev);
> +               if (IS_SKYLAKE(dev))
why this if here?

If it is a bxt restriction shouldn't it be in a separated patch?

> +                       __gen6_update_ring_freq(dev);
>         } else if (IS_BROADWELL(dev)) {
>                 gen8_enable_rps(dev);
>                 __gen6_update_ring_freq(dev);
> --
> 1.9.2
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Rodrigo Vivi
Blog: http://blog.vivi.eng.br
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/3] drm/i915/skl: Updated the i915_ring_freq_table debugfs function

[Rodrigo Vivi]

On Tue, May 12, 2015 at 12:49 AM,  <akash.goel@intel.com> wrote:
> From: Akash Goel <akash.goel@intel.com>
>
> Updated the i915_ring_freq_table debugfs function to allow read of ring
> frequency table through Punit interface, for SKL also.
>
> Signed-off-by: Akash Goel <akash.goel@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c | 21 ++++++++++++++++-----
>  1 file changed, 16 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 9e5a56c..32527de 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -1769,8 +1769,9 @@ static int i915_ring_freq_table(struct seq_file *m, void *unused)
>         struct drm_i915_private *dev_priv = dev->dev_private;
>         int ret = 0;
>         int gpu_freq, ia_freq;
> +       unsigned int max_gpu_freq, min_gpu_freq;
>
> -       if (!(IS_GEN6(dev) || IS_GEN7(dev))) {
> +       if (!(IS_GEN6(dev) || IS_GEN7(dev) || IS_SKYLAKE(dev))) {

Why HSW and BDW aren't here and SKL is back?
Again, I'm without doc for this, sorry...

>                 seq_puts(m, "unsupported on this chipset\n");
>                 return 0;
>         }
> @@ -1783,17 +1784,27 @@ static int i915_ring_freq_table(struct seq_file *m, void *unused)
>         if (ret)
>                 goto out;
>
> +       if (IS_SKYLAKE(dev)) {
> +               /* Convert GT frequency to 50 HZ units */
> +               min_gpu_freq =
> +                       dev_priv->rps.min_freq_softlimit / GEN9_FREQ_SCALER;
> +               max_gpu_freq =
> +                       dev_priv->rps.max_freq_softlimit / GEN9_FREQ_SCALER;
> +       } else {
> +               min_gpu_freq = dev_priv->rps.min_freq_softlimit;
> +               max_gpu_freq = dev_priv->rps.max_freq_softlimit;
> +       }
> +
>         seq_puts(m, "GPU freq (MHz)\tEffective CPU freq (MHz)\tEffective Ring freq (MHz)\n");
>
> -       for (gpu_freq = dev_priv->rps.min_freq_softlimit;
> -            gpu_freq <= dev_priv->rps.max_freq_softlimit;
> -            gpu_freq++) {
> +       for (gpu_freq = min_gpu_freq; gpu_freq <= max_gpu_freq; gpu_freq++) {
>                 ia_freq = gpu_freq;
>                 sandybridge_pcode_read(dev_priv,
>                                        GEN6_PCODE_READ_MIN_FREQ_TABLE,
>                                        &ia_freq);
>                 seq_printf(m, "%d\t\t%d\t\t\t\t%d\n",
> -                          intel_gpu_freq(dev_priv, gpu_freq),
> +                          intel_gpu_freq(dev_priv, (gpu_freq *
> +                               (IS_SKYLAKE(dev) ? GEN9_FREQ_SCALER : 1))),

here the scaler conversion made sense to me...

>                            ((ia_freq >> 0) & 0xff) * 100,
>                            ((ia_freq >> 8) & 0xff) * 100);
>         }
> --
> 1.9.2
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Rodrigo Vivi
Blog: http://blog.vivi.eng.br
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm/i915/skl: Retrieve the Rpe value from Pcode

[Akash Goel]

On Wed, 2015-06-03 at 14:19 -0700, Rodrigo Vivi wrote:
> On Tue, May 5, 2015 at 4:30 AM,  <akash.goel@intel.com> wrote:
> > From: Akash Goel <akash.goel@intel.com>
> >
> > Read the efficient frequency (aka RPe) value through the the mailbox
> > command (0x1A) from the pcode, as done on Haswell and Broadwell.
> > The turbo minimum frequency softlimit is not revised as per the
> > efficient frequency value.
> >
> > Issue: VIZ-5143
> > Signed-off-by: Akash Goel <akash.goel@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_pm.c | 8 ++++++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index a7516ed..421b78d 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -4274,16 +4274,20 @@ static void gen6_init_rps_frequencies(struct drm_device *dev)
> >         dev_priv->rps.max_freq          = dev_priv->rps.rp0_freq;
> >
> >         dev_priv->rps.efficient_freq = dev_priv->rps.rp1_freq;
> > -       if (IS_HASWELL(dev) || IS_BROADWELL(dev)) {
> > +       if (IS_HASWELL(dev) || IS_BROADWELL(dev) || IS_SKYLAKE(dev)) {
> 
> Could you please give me a pointer in the spec to this? Couldn't find
> on new one.

Actually got this piece of info from fellow VPG engineers. Sorry I
myself don't have the access to the Punit spec.


> >                 ret = sandybridge_pcode_read(dev_priv,
> >                                         HSW_PCODE_DYNAMIC_DUTY_CYCLE_CONTROL,
> >                                         &ddcc_status);
> > -               if (0 == ret)
> > +               if (0 == ret) {
> >                         dev_priv->rps.efficient_freq =
> >                                 clamp_t(u8,
> >                                         ((ddcc_status >> 8) & 0xff),
> >                                         dev_priv->rps.min_freq,
> >                                         dev_priv->rps.max_freq);
> > +
> > +                       dev_priv->rps.efficient_freq *=
> > +                               (IS_SKYLAKE(dev) ? GEN9_FREQ_SCALER : 1);
> 
> I thought you were managing everything on 50MHz and converting to 16.6
> only when writing. So this here seems to me that will compare to
> values in different bases..

The frequency values are being internally maintained in their natural
hardware units only (and functions intel_gpu_freq & intel_freq_opcode
help in the conversion).
For SKL, although unit is 16.667 MHz, but there are few exceptions like
RP_STATE register, which still reports frequency in 50 MHZ units, so an
extra conversion is required there to internally store the values in
16.667 MHZ. The Ring frequency table programming is another instance
where update has to be done as per 50 MHZ unit.

Best regards
Akash

> 
> > +               }
> >         }
> >
> >         dev_priv->rps.idle_freq = dev_priv->rps.min_freq;
> > --
> > 1.9.2
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> 


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

Re: [PATCH 2/2] drm/i915/skl: Ring frequency table programming changes

[Akash Goel]

On Wed, 2015-06-03 at 14:24 -0700, Rodrigo Vivi wrote:
> On Tue, May 5, 2015 at 4:30 AM,  <akash.goel@intel.com> wrote:
> > From: Akash Goel <akash.goel@intel.com>
> >
> > Ring frequency table programming changes for SKL. No need for a
> > floor on ring frequency, as the issue of performance impact with
> > ring running below DDR frequency, is believed to be fixed on SKL
> >
> > Issue: VIZ-5144
> > Signed-off-by: Akash Goel <akash.goel@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_pm.c | 26 +++++++++++++++++++++-----
> >  1 file changed, 21 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index 421b78d..d1bdea7 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -4582,6 +4582,7 @@ static void __gen6_update_ring_freq(struct drm_device *dev)
> >         int min_freq = 15;
> >         unsigned int gpu_freq;
> >         unsigned int max_ia_freq, min_ring_freq;
> > +       unsigned int max_gpu_freq, min_gpu_freq;
> >         int scaling_factor = 180;
> >         struct cpufreq_policy *policy;
> >
> > @@ -4606,17 +4607,31 @@ static void __gen6_update_ring_freq(struct drm_device *dev)
> >         /* convert DDR frequency from units of 266.6MHz to bandwidth */
> >         min_ring_freq = mult_frac(min_ring_freq, 8, 3);
> >
> > +       if (IS_SKYLAKE(dev)) {
> > +               /* Convert GT frequency to 50 HZ units */
> > +               min_gpu_freq = dev_priv->rps.min_freq / GEN9_FREQ_SCALER;
> > +               max_gpu_freq = dev_priv->rps.max_freq / GEN9_FREQ_SCALER;
> 
> I got even more confused here with the scale differences... Doesn't
> look so consistent..

For Ring frequency table programming, as an input GT frequency has to be
specified in units of 50 MHZ only. For SKL natural hardware unit is
16.667 MHZ and Driver is internally storing in the same unit only, hence
an extra conversion required here.

> 
> > +       } else {
> > +               min_gpu_freq = dev_priv->rps.min_freq;
> > +               max_gpu_freq = dev_priv->rps.max_freq;
> > +       }
> > +
> >         /*
> >          * For each potential GPU frequency, load a ring frequency we'd like
> >          * to use for memory access.  We do this by specifying the IA frequency
> >          * the PCU should use as a reference to determine the ring frequency.
> >          */
> > -       for (gpu_freq = dev_priv->rps.max_freq; gpu_freq >= dev_priv->rps.min_freq;
> > -            gpu_freq--) {
> > -               int diff = dev_priv->rps.max_freq - gpu_freq;
> > +       for (gpu_freq = max_gpu_freq; gpu_freq >= min_gpu_freq; gpu_freq--) {
> > +               int diff = max_gpu_freq - gpu_freq;
> >                 unsigned int ia_freq = 0, ring_freq = 0;
> >
> > -               if (INTEL_INFO(dev)->gen >= 8) {
> > +               if (IS_SKYLAKE(dev)) {
> > +                       /*
> > +                        * ring_freq = 2 * GT. ring_freq is in 100MHz units
> > +                        * No floor required for ring frequency on SKL.
> > +                        */
> > +                       ring_freq = gpu_freq;
> 
> Aren't we using the 16.6 now? Shouldn't it be converted anyway?
> 
> > +               } else if (INTEL_INFO(dev)->gen >= 8) {
> >                         /* max(2 * GT, DDR). NB: GT is 50MHz units */
> >                         ring_freq = max(min_ring_freq, gpu_freq);
> >                 } else if (IS_HASWELL(dev)) {
> > @@ -5770,7 +5785,8 @@ static void intel_gen6_powersave_work(struct work_struct *work)
> >         } else if (INTEL_INFO(dev)->gen >= 9) {
> >                 gen9_enable_rc6(dev);
> >                 gen9_enable_rps(dev);
> > -               __gen6_update_ring_freq(dev);
> > +               if (IS_SKYLAKE(dev))
> why this if here?
> 
> If it is a bxt restriction shouldn't it be in a separated patch?

Yes for BXT, its not supported. Would move it to separate patch.

> 
> > +                       __gen6_update_ring_freq(dev);
> >         } else if (IS_BROADWELL(dev)) {
> >                 gen8_enable_rps(dev);
> >                 __gen6_update_ring_freq(dev);
> > --
> > 1.9.2
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> 


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

Re: [PATCH 3/3] drm/i915/skl: Updated the i915_ring_freq_table debugfs function

[Akash Goel]

On Wed, 2015-06-03 at 14:27 -0700, Rodrigo Vivi wrote:
> On Tue, May 12, 2015 at 12:49 AM,  <akash.goel@intel.com> wrote:
> > From: Akash Goel <akash.goel@intel.com>
> >
> > Updated the i915_ring_freq_table debugfs function to allow read of ring
> > frequency table through Punit interface, for SKL also.
> >
> > Signed-off-by: Akash Goel <akash.goel@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_debugfs.c | 21 ++++++++++++++++-----
> >  1 file changed, 16 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> > index 9e5a56c..32527de 100644
> > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > @@ -1769,8 +1769,9 @@ static int i915_ring_freq_table(struct seq_file *m, void *unused)
> >         struct drm_i915_private *dev_priv = dev->dev_private;
> >         int ret = 0;
> >         int gpu_freq, ia_freq;
> > +       unsigned int max_gpu_freq, min_gpu_freq;
> >
> > -       if (!(IS_GEN6(dev) || IS_GEN7(dev))) {
> > +       if (!(IS_GEN6(dev) || IS_GEN7(dev) || IS_SKYLAKE(dev))) {
> 
> Why HSW and BDW aren't here and SKL is back?
> Again, I'm without doc for this, sorry...

Sorry not sure that why HSW/BDW aren't used here. For them also this
interface should work.

> 
> >                 seq_puts(m, "unsupported on this chipset\n");
> >                 return 0;
> >         }
> > @@ -1783,17 +1784,27 @@ static int i915_ring_freq_table(struct seq_file *m, void *unused)
> >         if (ret)
> >                 goto out;
> >
> > +       if (IS_SKYLAKE(dev)) {
> > +               /* Convert GT frequency to 50 HZ units */
> > +               min_gpu_freq =
> > +                       dev_priv->rps.min_freq_softlimit / GEN9_FREQ_SCALER;
> > +               max_gpu_freq =
> > +                       dev_priv->rps.max_freq_softlimit / GEN9_FREQ_SCALER;
> > +       } else {
> > +               min_gpu_freq = dev_priv->rps.min_freq_softlimit;
> > +               max_gpu_freq = dev_priv->rps.max_freq_softlimit;
> > +       }
> > +
> >         seq_puts(m, "GPU freq (MHz)\tEffective CPU freq (MHz)\tEffective Ring freq (MHz)\n");
> >
> > -       for (gpu_freq = dev_priv->rps.min_freq_softlimit;
> > -            gpu_freq <= dev_priv->rps.max_freq_softlimit;
> > -            gpu_freq++) {
> > +       for (gpu_freq = min_gpu_freq; gpu_freq <= max_gpu_freq; gpu_freq++) {
> >                 ia_freq = gpu_freq;
> >                 sandybridge_pcode_read(dev_priv,
> >                                        GEN6_PCODE_READ_MIN_FREQ_TABLE,
> >                                        &ia_freq);
> >                 seq_printf(m, "%d\t\t%d\t\t\t\t%d\n",
> > -                          intel_gpu_freq(dev_priv, gpu_freq),
> > +                          intel_gpu_freq(dev_priv, (gpu_freq *
> > +                               (IS_SKYLAKE(dev) ? GEN9_FREQ_SCALER : 1))),
> 
> here the scaler conversion made sense to me...
> 
> >                            ((ia_freq >> 0) & 0xff) * 100,
> >                            ((ia_freq >> 8) & 0xff) * 100);
> >         }
> > --
> > 1.9.2
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> 


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

Re: [PATCH 3/3] drm/i915/skl: Updated the i915_ring_freq_table debugfs function

[Ville Syrjälä]

On Thu, Jun 04, 2015 at 01:36:36PM +0530, Akash Goel wrote:
> On Wed, 2015-06-03 at 14:27 -0700, Rodrigo Vivi wrote:
> > On Tue, May 12, 2015 at 12:49 AM,  <akash.goel@intel.com> wrote:
> > > From: Akash Goel <akash.goel@intel.com>
> > >
> > > Updated the i915_ring_freq_table debugfs function to allow read of ring
> > > frequency table through Punit interface, for SKL also.
> > >
> > > Signed-off-by: Akash Goel <akash.goel@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_debugfs.c | 21 ++++++++++++++++-----
> > >  1 file changed, 16 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> > > index 9e5a56c..32527de 100644
> > > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > > @@ -1769,8 +1769,9 @@ static int i915_ring_freq_table(struct seq_file *m, void *unused)
> > >         struct drm_i915_private *dev_priv = dev->dev_private;
> > >         int ret = 0;
> > >         int gpu_freq, ia_freq;
> > > +       unsigned int max_gpu_freq, min_gpu_freq;
> > >
> > > -       if (!(IS_GEN6(dev) || IS_GEN7(dev))) {
> > > +       if (!(IS_GEN6(dev) || IS_GEN7(dev) || IS_SKYLAKE(dev))) {
> > 
> > Why HSW and BDW aren't here and SKL is back?
> > Again, I'm without doc for this, sorry...
> 
> Sorry not sure that why HSW/BDW aren't used here. For them also this
> interface should work.

HSW is included in the GEN7 check. But so is VLV and that one should be
rejected. BDW is genuinely misising. Can you fix it all up while you're
poking at it?

> 
> > 
> > >                 seq_puts(m, "unsupported on this chipset\n");
> > >                 return 0;
> > >         }
> > > @@ -1783,17 +1784,27 @@ static int i915_ring_freq_table(struct seq_file *m, void *unused)
> > >         if (ret)
> > >                 goto out;
> > >
> > > +       if (IS_SKYLAKE(dev)) {
> > > +               /* Convert GT frequency to 50 HZ units */
> > > +               min_gpu_freq =
> > > +                       dev_priv->rps.min_freq_softlimit / GEN9_FREQ_SCALER;
> > > +               max_gpu_freq =
> > > +                       dev_priv->rps.max_freq_softlimit / GEN9_FREQ_SCALER;
> > > +       } else {
> > > +               min_gpu_freq = dev_priv->rps.min_freq_softlimit;
> > > +               max_gpu_freq = dev_priv->rps.max_freq_softlimit;
> > > +       }
> > > +
> > >         seq_puts(m, "GPU freq (MHz)\tEffective CPU freq (MHz)\tEffective Ring freq (MHz)\n");
> > >
> > > -       for (gpu_freq = dev_priv->rps.min_freq_softlimit;
> > > -            gpu_freq <= dev_priv->rps.max_freq_softlimit;
> > > -            gpu_freq++) {
> > > +       for (gpu_freq = min_gpu_freq; gpu_freq <= max_gpu_freq; gpu_freq++) {
> > >                 ia_freq = gpu_freq;
> > >                 sandybridge_pcode_read(dev_priv,
> > >                                        GEN6_PCODE_READ_MIN_FREQ_TABLE,
> > >                                        &ia_freq);
> > >                 seq_printf(m, "%d\t\t%d\t\t\t\t%d\n",
> > > -                          intel_gpu_freq(dev_priv, gpu_freq),
> > > +                          intel_gpu_freq(dev_priv, (gpu_freq *
> > > +                               (IS_SKYLAKE(dev) ? GEN9_FREQ_SCALER : 1))),
> > 
> > here the scaler conversion made sense to me...
> > 
> > >                            ((ia_freq >> 0) & 0xff) * 100,
> > >                            ((ia_freq >> 8) & 0xff) * 100);
> > >         }
> > > --
> > > 1.9.2
> > >
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
> > 
> > 
> 
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx