subject:"\[Intel\-gfx\] \[PATCH 04\/10\] drm\/i915\: add RPS configuration for Haswell"

Re: [Intel-gfx] [PATCH 04/10] drm/i915: add RPS configuration for Haswell

2012-07-04 Thread Chris Wilson

On Mon,  2 Jul 2012 11:51:05 -0300, Eugeni Dodonov eugeni.dodo...@intel.com 
wrote:
   I915_WRITE(GEN6_RP_IDLE_HYSTERSIS, 10);
   I915_WRITE(GEN6_RP_CONTROL,
  GEN6_RP_MEDIA_TURBO |
 @@ -2444,7 +2457,7 @@ static void gen6_enable_rps(struct drm_device *dev)
  GEN6_RP_MEDIA_IS_GFX |
  GEN6_RP_ENABLE |
  GEN6_RP_UP_BUSY_AVG |
 -GEN6_RP_DOWN_IDLE_CONT);
 +(IS_HASWELL(dev)) ? GEN7_RP_DOWN_IDLE_AVG : 
 GEN6_RP_DOWN_IDLE_CONT);

And with one small typo does everything break...
-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

[Intel-gfx] [PATCH 04/10] drm/i915: add RPS configuration for Haswell

2012-07-02 Thread Eugeni Dodonov

Most of the RPS and RC6 enabling functionality is similar to what we had
on Gen6/Gen7, so we preserve most of the registers.

Note that Haswell only has RC6, so account for that as well. As suggested
by Daniel Vetter, to reduce the amount of changes in the patch, we still
write the RC6p/RC6pp thresholds, but those are ignored on Haswell.

Signed-off-by: Eugeni Dodonov eugeni.dodo...@intel.com
---
 drivers/gpu/drm/i915/i915_reg.h |  1 +
 drivers/gpu/drm/i915/intel_pm.c | 37 +
 2 files changed, 26 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index f17de3d..9d5bf06 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -4156,6 +4156,7 @@
 #define   GEN6_RP_UP_IDLE_MIN  (0x13)
 #define   GEN6_RP_UP_BUSY_AVG  (0x23)
 #define   GEN6_RP_UP_BUSY_CONT (0x43)
+#define   GEN7_RP_DOWN_IDLE_AVG(0x20)
 #define   GEN6_RP_DOWN_IDLE_CONT   (0x10)
 #define GEN6_RP_UP_THRESHOLD   0xA02C
 #define GEN6_RP_DOWN_THRESHOLD 0xA030
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 29720d2..a3ee1b1 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -2402,20 +2402,24 @@ static void gen6_enable_rps(struct drm_device *dev)
I915_WRITE(GEN6_RC6p_THRESHOLD, 10);
I915_WRITE(GEN6_RC6pp_THRESHOLD, 64000); /* unused */
 
+   /* Check if we are enabling RC6 */
rc6_mode = intel_enable_rc6(dev_priv-dev);
if (rc6_mode  INTEL_RC6_ENABLE)
rc6_mask |= GEN6_RC_CTL_RC6_ENABLE;
 
-   if (rc6_mode  INTEL_RC6p_ENABLE)
-   rc6_mask |= GEN6_RC_CTL_RC6p_ENABLE;
+   /* We don't use those on Haswell */
+   if (!IS_HASWELL(dev)) {
+   if (rc6_mode  INTEL_RC6p_ENABLE)
+   rc6_mask |= GEN6_RC_CTL_RC6p_ENABLE;
 
-   if (rc6_mode  INTEL_RC6pp_ENABLE)
-   rc6_mask |= GEN6_RC_CTL_RC6pp_ENABLE;
+   if (rc6_mode  INTEL_RC6pp_ENABLE)
+   rc6_mask |= GEN6_RC_CTL_RC6pp_ENABLE;
+   }
 
DRM_INFO(Enabling RC6 states: RC6 %s, RC6p %s, RC6pp %s\n,
-   (rc6_mode  INTEL_RC6_ENABLE) ? on : off,
-   (rc6_mode  INTEL_RC6p_ENABLE) ? on : off,
-   (rc6_mode  INTEL_RC6pp_ENABLE) ? on : off);
+   (rc6_mask  GEN6_RC_CTL_RC6_ENABLE) ? on : off,
+   (rc6_mask  GEN6_RC_CTL_RC6p_ENABLE) ? on : off,
+   (rc6_mask  GEN6_RC_CTL_RC6pp_ENABLE) ? on : off);
 
I915_WRITE(GEN6_RC_CONTROL,
   rc6_mask |
@@ -2433,10 +2437,19 @@ static void gen6_enable_rps(struct drm_device *dev)
I915_WRITE(GEN6_RP_INTERRUPT_LIMITS,
   dev_priv-max_delay  24 |
   dev_priv-min_delay  16);
-   I915_WRITE(GEN6_RP_UP_THRESHOLD, 1);
-   I915_WRITE(GEN6_RP_DOWN_THRESHOLD, 100);
-   I915_WRITE(GEN6_RP_UP_EI, 10);
-   I915_WRITE(GEN6_RP_DOWN_EI, 500);
+
+   if (IS_HASWELL(dev)) {
+   I915_WRITE(GEN6_RP_UP_THRESHOLD, 59400);
+   I915_WRITE(GEN6_RP_DOWN_THRESHOLD, 245000);
+   I915_WRITE(GEN6_RP_UP_EI, 66000);
+   I915_WRITE(GEN6_RP_DOWN_EI, 35);
+   } else {
+   I915_WRITE(GEN6_RP_UP_THRESHOLD, 1);
+   I915_WRITE(GEN6_RP_DOWN_THRESHOLD, 100);
+   I915_WRITE(GEN6_RP_UP_EI, 10);
+   I915_WRITE(GEN6_RP_DOWN_EI, 500);
+   }
+
I915_WRITE(GEN6_RP_IDLE_HYSTERSIS, 10);
I915_WRITE(GEN6_RP_CONTROL,
   GEN6_RP_MEDIA_TURBO |
@@ -2444,7 +2457,7 @@ static void gen6_enable_rps(struct drm_device *dev)
   GEN6_RP_MEDIA_IS_GFX |
   GEN6_RP_ENABLE |
   GEN6_RP_UP_BUSY_AVG |
-  GEN6_RP_DOWN_IDLE_CONT);
+  (IS_HASWELL(dev)) ? GEN7_RP_DOWN_IDLE_AVG : 
GEN6_RP_DOWN_IDLE_CONT);
 
if (wait_for((I915_READ(GEN6_PCODE_MAILBOX)  GEN6_PCODE_READY) == 0,
 500))
-- 
1.7.11.1

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

Re: [Intel-gfx] [PATCH 04/10] drm/i915: add RPS configuration for Haswell

2012-07-02 Thread Ben Widawsky

On Mon,  2 Jul 2012 11:51:05 -0300
Eugeni Dodonov eugeni.dodo...@intel.com wrote:

 Most of the RPS and RC6 enabling functionality is similar to what we had
 on Gen6/Gen7, so we preserve most of the registers.
 
 Note that Haswell only has RC6, so account for that as well. As suggested
 by Daniel Vetter, to reduce the amount of changes in the patch, we still
 write the RC6p/RC6pp thresholds, but those are ignored on Haswell.
 
 Signed-off-by: Eugeni Dodonov eugeni.dodo...@intel.com
Reviewed-by: Ben Widawsky b...@bwidawsk.net

 ---
  drivers/gpu/drm/i915/i915_reg.h |  1 +
  drivers/gpu/drm/i915/intel_pm.c | 37 +
  2 files changed, 26 insertions(+), 12 deletions(-)
 
 diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
 index f17de3d..9d5bf06 100644
 --- a/drivers/gpu/drm/i915/i915_reg.h
 +++ b/drivers/gpu/drm/i915/i915_reg.h
 @@ -4156,6 +4156,7 @@
  #define   GEN6_RP_UP_IDLE_MIN(0x13)
  #define   GEN6_RP_UP_BUSY_AVG(0x23)
  #define   GEN6_RP_UP_BUSY_CONT   (0x43)
 +#define   GEN7_RP_DOWN_IDLE_AVG  (0x20)
  #define   GEN6_RP_DOWN_IDLE_CONT (0x10)
  #define GEN6_RP_UP_THRESHOLD 0xA02C
  #define GEN6_RP_DOWN_THRESHOLD   0xA030
 diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
 index 29720d2..a3ee1b1 100644
 --- a/drivers/gpu/drm/i915/intel_pm.c
 +++ b/drivers/gpu/drm/i915/intel_pm.c
 @@ -2402,20 +2402,24 @@ static void gen6_enable_rps(struct drm_device *dev)
   I915_WRITE(GEN6_RC6p_THRESHOLD, 10);
   I915_WRITE(GEN6_RC6pp_THRESHOLD, 64000); /* unused */
  
 + /* Check if we are enabling RC6 */
   rc6_mode = intel_enable_rc6(dev_priv-dev);
   if (rc6_mode  INTEL_RC6_ENABLE)
   rc6_mask |= GEN6_RC_CTL_RC6_ENABLE;
  
 - if (rc6_mode  INTEL_RC6p_ENABLE)
 - rc6_mask |= GEN6_RC_CTL_RC6p_ENABLE;
 + /* We don't use those on Haswell */
 + if (!IS_HASWELL(dev)) {
 + if (rc6_mode  INTEL_RC6p_ENABLE)
 + rc6_mask |= GEN6_RC_CTL_RC6p_ENABLE;
  
 - if (rc6_mode  INTEL_RC6pp_ENABLE)
 - rc6_mask |= GEN6_RC_CTL_RC6pp_ENABLE;
 + if (rc6_mode  INTEL_RC6pp_ENABLE)
 + rc6_mask |= GEN6_RC_CTL_RC6pp_ENABLE;
 + }
  
   DRM_INFO(Enabling RC6 states: RC6 %s, RC6p %s, RC6pp %s\n,
 - (rc6_mode  INTEL_RC6_ENABLE) ? on : off,
 - (rc6_mode  INTEL_RC6p_ENABLE) ? on : off,
 - (rc6_mode  INTEL_RC6pp_ENABLE) ? on : off);
 + (rc6_mask  GEN6_RC_CTL_RC6_ENABLE) ? on : off,
 + (rc6_mask  GEN6_RC_CTL_RC6p_ENABLE) ? on : off,
 + (rc6_mask  GEN6_RC_CTL_RC6pp_ENABLE) ? on : off);

Belongs in a separate patch, but meh.

  
   I915_WRITE(GEN6_RC_CONTROL,
  rc6_mask |
 @@ -2433,10 +2437,19 @@ static void gen6_enable_rps(struct drm_device *dev)
   I915_WRITE(GEN6_RP_INTERRUPT_LIMITS,
  dev_priv-max_delay  24 |
  dev_priv-min_delay  16);
 - I915_WRITE(GEN6_RP_UP_THRESHOLD, 1);
 - I915_WRITE(GEN6_RP_DOWN_THRESHOLD, 100);
 - I915_WRITE(GEN6_RP_UP_EI, 10);
 - I915_WRITE(GEN6_RP_DOWN_EI, 500);
 +
 + if (IS_HASWELL(dev)) {
 + I915_WRITE(GEN6_RP_UP_THRESHOLD, 59400);
 + I915_WRITE(GEN6_RP_DOWN_THRESHOLD, 245000);
 + I915_WRITE(GEN6_RP_UP_EI, 66000);
 + I915_WRITE(GEN6_RP_DOWN_EI, 35);
 + } else {
 + I915_WRITE(GEN6_RP_UP_THRESHOLD, 1);
 + I915_WRITE(GEN6_RP_DOWN_THRESHOLD, 100);
 + I915_WRITE(GEN6_RP_UP_EI, 10);
 + I915_WRITE(GEN6_RP_DOWN_EI, 500);
 + }
 +

I can't really comment much on this other than what I said below. It'd
be nice if the policy didn't change between platforms unless we
know/understand why.

   I915_WRITE(GEN6_RP_IDLE_HYSTERSIS, 10);
   I915_WRITE(GEN6_RP_CONTROL,
  GEN6_RP_MEDIA_TURBO |
 @@ -2444,7 +2457,7 @@ static void gen6_enable_rps(struct drm_device *dev)
  GEN6_RP_MEDIA_IS_GFX |
  GEN6_RP_ENABLE |
  GEN6_RP_UP_BUSY_AVG |
 -GEN6_RP_DOWN_IDLE_CONT);
 +(IS_HASWELL(dev)) ? GEN7_RP_DOWN_IDLE_AVG : 
 GEN6_RP_DOWN_IDLE_CONT);

I think this warrants an explanation. I think we would ideally like to
avoid different algorithms for different platforms.

  
   if (wait_for((I915_READ(GEN6_PCODE_MAILBOX)  GEN6_PCODE_READY) == 0,
500))



-- 
Ben Widawsky, Intel Open Source Technology Center
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 04/10] drm/i915: add RPS configuration for Haswell

2012-07-02 Thread Eugeni Dodonov

On 07/02/2012 02:49 PM, Ben Widawsky wrote:
 On Mon,  2 Jul 2012 11:51:05 -0300
 Eugeni Dodonov eugeni.dodo...@intel.com wrote:
 
 Most of the RPS and RC6 enabling functionality is similar to what we had
 on Gen6/Gen7, so we preserve most of the registers.

 Note that Haswell only has RC6, so account for that as well. As suggested
 by Daniel Vetter, to reduce the amount of changes in the patch, we still
 write the RC6p/RC6pp thresholds, but those are ignored on Haswell.

 Signed-off-by: Eugeni Dodonov eugeni.dodo...@intel.com
 Reviewed-by: Ben Widawsky b...@bwidawsk.net

Daniel, to please Ben Widawsky, you could add an extra paragraph in my
message, saying:

'On Haswell, the suggested values for the RP UP and RP DOWN interrupts
have changed in a way to have lower thresholds for all the operations.
However, the suggested values for pre-Haswell machines stay the same. In
order to follow what the hardware designers suggest, we program those as
suggested for each generation now.

Also, Haswell introduces a new scheduling bit for the RP DOWN
scheduling, which we now use by default'.

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

Re: [Intel-gfx] [PATCH 04/10] drm/i915: add RPS configuration for Haswell

2012-07-02 Thread Ben Widawsky

On Mon, 02 Jul 2012 17:02:59 -0300
Eugeni Dodonov eugeni.dodo...@linux.intel.com wrote:

 On 07/02/2012 02:49 PM, Ben Widawsky wrote:
  On Mon,  2 Jul 2012 11:51:05 -0300
  Eugeni Dodonov eugeni.dodo...@intel.com wrote:
  
  Most of the RPS and RC6 enabling functionality is similar to what we had
  on Gen6/Gen7, so we preserve most of the registers.
 
  Note that Haswell only has RC6, so account for that as well. As suggested
  by Daniel Vetter, to reduce the amount of changes in the patch, we still
  write the RC6p/RC6pp thresholds, but those are ignored on Haswell.
 
  Signed-off-by: Eugeni Dodonov eugeni.dodo...@intel.com
  Reviewed-by: Ben Widawsky b...@bwidawsk.net
 
 Daniel, to please Ben Widawsky, you could add an extra paragraph in my
 message, saying:
 
 'On Haswell, the suggested values for the RP UP and RP DOWN interrupts
 have changed in a way to have lower thresholds for all the operations.
 However, the suggested values for pre-Haswell machines stay the same. In
 order to follow what the hardware designers suggest, we program those as
 suggested for each generation now.
 
 Also, Haswell introduces a new scheduling bit for the RP DOWN
 scheduling, which we now use by default'.
 
 Eugeni

The code itself states the above perfectly well. I was really trying
to understand the, why.


-- 
Ben Widawsky, Intel Open Source Technology Center
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 04/10] drm/i915: add RPS configuration for Haswell

[Intel-gfx] [PATCH 04/10] drm/i915: add RPS configuration for Haswell

Re: [Intel-gfx] [PATCH 04/10] drm/i915: add RPS configuration for Haswell

Re: [Intel-gfx] [PATCH 04/10] drm/i915: add RPS configuration for Haswell

Re: [Intel-gfx] [PATCH 04/10] drm/i915: add RPS configuration for Haswell

5 matches

Site Navigation

Mail list logo

Footer information