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

Message ID 1430825455-28382-2-git-send-email-akash.goel@intel.com
State New
Headers show

Commit Message

akash.goel@intel.com May 5, 2015, 11:30 a.m. UTC
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(-)

Comments

Shuang He May 5, 2015, 7:20 p.m. UTC | #1
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 '*'
Rodrigo Vivi June 3, 2015, 9:24 p.m. UTC | #2
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
akash.goel@intel.com June 4, 2015, 8:03 a.m. UTC | #3
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
> 
> 
>

Patch
diff mbox

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