[Intel-gfx] [PATCH] drm/i915/vlv: Add VLV specific force wake routines.
From: Deepak S
Added media/render/common well VLV force wake routines to help
bring up the WELLS before access the register
- Refactor current vlv_forcewake get/put and added MEDIA or
RENDER specific Forcewake.
- Added VLV Check to bring up MEDIA and RENDER WELL base
on the register accessed in vlv_read##x (in intel_uncore.c)
Signed-off-by: Deepak S
---
drivers/gpu/drm/i915/i915_debugfs.c | 8 +-
drivers/gpu/drm/i915/i915_drv.h | 31 -
drivers/gpu/drm/i915/i915_gpu_error.c | 2 +-
drivers/gpu/drm/i915/i915_reg.h | 4 +-
drivers/gpu/drm/i915/intel_display.c| 4 +-
drivers/gpu/drm/i915/intel_pm.c | 18 ++-
drivers/gpu/drm/i915/intel_ringbuffer.c | 14 +-
drivers/gpu/drm/i915/intel_uncore.c | 223 +---
8 files changed, 237 insertions(+), 67 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
b/drivers/gpu/drm/i915/i915_debugfs.c
index 506f8ef..0c1d6ce 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -901,7 +901,7 @@ static int i915_cur_delayinfo(struct seq_file *m, void
*unused)
if (ret)
return ret;
- gen6_gt_force_wake_get(dev_priv);
+ gen6_gt_force_wake_get(dev_priv, FORCEWAKE_ALL);
reqf = I915_READ(GEN6_RPNSWREQ);
reqf &= ~GEN6_TURBO_DISABLE;
@@ -924,7 +924,7 @@ static int i915_cur_delayinfo(struct seq_file *m, void
*unused)
cagf = (rpstat & GEN6_CAGF_MASK) >> GEN6_CAGF_SHIFT;
cagf *= GT_FREQUENCY_MULTIPLIER;
- gen6_gt_force_wake_put(dev_priv);
+ gen6_gt_force_wake_put(dev_priv, FORCEWAKE_ALL);
mutex_unlock(&dev->struct_mutex);
seq_printf(m, "GT_PERF_STATUS: 0x%08x\n", gt_perf_status);
@@ -2898,7 +2898,7 @@ static int i915_forcewake_open(struct inode *inode,
struct file *file)
if (INTEL_INFO(dev)->gen < 6)
return 0;
- gen6_gt_force_wake_get(dev_priv);
+ gen6_gt_force_wake_get(dev_priv, FORCEWAKE_ALL);
return 0;
}
@@ -2911,7 +2911,7 @@ static int i915_forcewake_release(struct inode *inode,
struct file *file)
if (INTEL_INFO(dev)->gen < 6)
return 0;
- gen6_gt_force_wake_put(dev_priv);
+ gen6_gt_force_wake_put(dev_priv, FORCEWAKE_ALL);
return 0;
}
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 6d2a9a1..e06e376 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -422,8 +422,10 @@ struct drm_i915_display_funcs {
};
struct intel_uncore_funcs {
- void (*force_wake_get)(struct drm_i915_private *dev_priv);
- void (*force_wake_put)(struct drm_i915_private *dev_priv);
+ void (*force_wake_get)(struct drm_i915_private *dev_priv,
+ int fw_engine);
+ void (*force_wake_put)(struct drm_i915_private *dev_priv,
+ int fw_engine);
uint8_t (*mmio_readb)(struct drm_i915_private *dev_priv, off_t offset,
bool trace);
uint16_t (*mmio_readw)(struct drm_i915_private *dev_priv, off_t offset,
bool trace);
@@ -448,6 +450,8 @@ struct intel_uncore {
unsigned fifo_count;
unsigned forcewake_count;
+ unsigned fw_rendercount;
+ unsigned fw_mediacount;
struct delayed_work force_wake_work;
};
@@ -2399,8 +2403,8 @@ extern void intel_display_print_error_state(struct
drm_i915_error_state_buf *e,
* must be set to prevent GT core from power down and stale values being
* returned.
*/
-void gen6_gt_force_wake_get(struct drm_i915_private *dev_priv);
-void gen6_gt_force_wake_put(struct drm_i915_private *dev_priv);
+void gen6_gt_force_wake_get(struct drm_i915_private *dev_priv, int fw_engine);
+void gen6_gt_force_wake_put(struct drm_i915_private *dev_priv, int fw_engine);
int sandybridge_pcode_read(struct drm_i915_private *dev_priv, u8 mbox, u32
*val);
int sandybridge_pcode_write(struct drm_i915_private *dev_priv, u8 mbox, u32
val);
@@ -2429,6 +2433,25 @@ void intel_sbi_write(struct drm_i915_private *dev_priv,
u16 reg, u32 value,
int vlv_gpu_freq(struct drm_i915_private *dev_priv, int val);
int vlv_freq_opcode(struct drm_i915_private *dev_priv, int val);
+void vlv_force_wake_get(struct drm_i915_private *dev_priv, int fw_engine);
+void vlv_force_wake_put(struct drm_i915_private *dev_priv, int fw_engine);
+
+#define FORCEWAKE_VLV_RENDER_RANGE_OFFSET(reg) \
+ (((reg) >= 0x2000 && (reg) < 0x4000) ||\
+ ((reg) >= 0x5000 && (reg) < 0x8000) ||\
+ ((reg) >= 0xB000 && (reg) < 0x12000) ||\
+ ((reg) >= 0x2E000 && (reg) < 0x3))
+
+#define FORCEWAKE_VLV_MEDIA_RANGE_OFFSET(reg)\
+ (((reg) >= 0x12000 && (reg) < 0x14000) ||\
+ ((reg) >= 0x22000 && (reg) < 0x24000) ||\
+ ((reg) >= 0x3 && (reg
Re: [Intel-gfx] [PATCH] drm/i915/vlv: Add VLV specific force wake routines.
On Thu, 14 Nov 2013 19:02:15 +0530 deepa...@intel.com wrote: > From: Deepak S > > Added media/render/common well VLV force wake routines to help > bring up the WELLS before access the register > - Refactor current vlv_forcewake get/put and added MEDIA or > RENDER specific Forcewake. > - Added VLV Check to bring up MEDIA and RENDER WELL base > on the register accessed in vlv_read##x (in intel_uncore.c) This patch is pretty big and so a bit hard to review. A couple of questions: - why not use the callback to __vlv_force_wake_* from gen6_gt_force_wake_*? i.e. why is VLV special here? - having a new gen7_media_force_wake function may be better than passing an engine around, and would touch fewer pieces of code - have you done measurements on this? given how infrequently we ought to be waking the wells when they're idle, and how long we generally keep them awake, is this a real power win? Thanks, Jesse ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915/vlv: Add VLV specific force wake routines.
Hi Jesse, Thanks for the review. Below is my response. > - why not use the callback to __vlv_force_wake_* from gen6_gt_force_wake_*? i.e. why is VLV special here? [Deepak] Gen6 has a single power well whereas the VLV is has spate wells. This was the reason for the separate function > - having a new gen7_media_force_wake function may be better than passing an engine around, and would touch fewer pieces of code [Deepak] Even Gen7 is also as single Power Well. Having common function between gen7 and vlv might be difficult to individually handle the wells. >- have you done measurements on this? given how infrequently we ought to be waking the wells when they're idle, and how long we generally keep them awake, is this a real power win? [Deepak] By Individually controlling the wells we observed around 100mW - 200mW saving in different scenarios (GL Beanchmark & Media playback). Thanks, Deepak -Original Message- From: Jesse Barnes [mailto:jbar...@virtuousgeek.org] Sent: Tuesday, November 19, 2013 11:36 PM To: S, Deepak Cc: intel-gfx@lists.freedesktop.org Subject: Re: [Intel-gfx] [PATCH] drm/i915/vlv: Add VLV specific force wake routines. On Thu, 14 Nov 2013 19:02:15 +0530 deepa...@intel.com wrote: > From: Deepak S > > Added media/render/common well VLV force wake routines to help bring > up the WELLS before access the register > - Refactor current vlv_forcewake get/put and added MEDIA or > RENDER specific Forcewake. > - Added VLV Check to bring up MEDIA and RENDER WELL base > on the register accessed in vlv_read##x (in intel_uncore.c) This patch is pretty big and so a bit hard to review. A couple of questions: - why not use the callback to __vlv_force_wake_* from gen6_gt_force_wake_*? i.e. why is VLV special here? - having a new gen7_media_force_wake function may be better than passing an engine around, and would touch fewer pieces of code - have you done measurements on this? given how infrequently we ought to be waking the wells when they're idle, and how long we generally keep them awake, is this a real power win? Thanks, Jesse ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915/vlv: Add VLV specific force wake routines.
On Wed, Nov 20, 2013 at 7:00 AM, S, Deepak wrote: >>- have you done measurements on this? given how infrequently we > ought to be waking the wells when they're idle, and how long we > generally keep them awake, is this a real power win? > [Deepak] By Individually controlling the wells we observed around 100mW - > 200mW saving in different scenarios (GL Beanchmark & Media playback). This kind of information is really important and should be part of the commit message. This rule holds generally for performance/power tuning work - the commit message should at least mention the order of magnitude of the improvements seen and which workloads have been tested. If you're afraid to disclose confidential information (e.g. for power savings) just make the language fuzzy enough, e.g. here "We've seen power savings in the lower sub-1W range on workloads that only neeed on of the power wells, e.g. glbenchmark, media playback." Thanks, Daniel -- 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: [Intel-gfx] [PATCH] drm/i915/vlv: Add VLV specific force wake routines.
Thanks Jesse, I wil split the patches and send it for review. Thanks Deepak -Original Message- From: Jesse Barnes [mailto:jbar...@virtuousgeek.org] Sent: Wednesday, November 20, 2013 9:35 PM To: S, Deepak Cc: intel-gfx@lists.freedesktop.org Subject: Re: [Intel-gfx] [PATCH] drm/i915/vlv: Add VLV specific force wake routines. On Wed, 20 Nov 2013 06:00:24 + "S, Deepak" wrote: > Hi Jesse, > > Thanks for the review. Below is my response. > > > - why not use the callback to __vlv_force_wake_* from > gen6_gt_force_wake_*? i.e. why is VLV special here? > [Deepak] Gen6 has a single power well whereas the VLV is has spate wells. > This was the reason for the separate function > > > - having a new gen7_media_force_wake function may be better than > passing an engine around, and would touch fewer pieces of code > [Deepak] Even Gen7 is also as single Power Well. Having common function > between gen7 and vlv might be difficult to individually handle the wells. > > >- have you done measurements on this? given how infrequently we > ought to be waking the wells when they're idle, and how long we > generally keep them awake, is this a real power win? > [Deepak] By Individually controlling the wells we observed around 100mW - > 200mW saving in different scenarios (GL Beanchmark & Media playback). wow, that's a significant savings. Can you split the patch into one that adds the power well arg, and another that adds the VLV support for the split? That would make it easier to review. Thanks, -- Jesse Barnes, 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] drm/i915/vlv: Add VLV specific force wake routines.
On Wed, 20 Nov 2013 06:00:24 + "S, Deepak" wrote: > Hi Jesse, > > Thanks for the review. Below is my response. > > > - why not use the callback to __vlv_force_wake_* from > gen6_gt_force_wake_*? i.e. why is VLV special here? > [Deepak] Gen6 has a single power well whereas the VLV is has spate wells. > This was the reason for the separate function > > > - having a new gen7_media_force_wake function may be better than > passing an engine around, and would touch fewer pieces of code > [Deepak] Even Gen7 is also as single Power Well. Having common function > between gen7 and vlv might be difficult to individually handle the wells. > > >- have you done measurements on this? given how infrequently we > ought to be waking the wells when they're idle, and how long we > generally keep them awake, is this a real power win? > [Deepak] By Individually controlling the wells we observed around 100mW - > 200mW saving in different scenarios (GL Beanchmark & Media playback). wow, that's a significant savings. Can you split the patch into one that adds the power well arg, and another that adds the VLV support for the split? That would make it easier to review. Thanks, -- Jesse Barnes, 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] drm/i915/vlv: Add VLV specific force wake routines.
Also, you might re-run your power numbers with Chris's patch to drop the force wake ref around the irq get/put. That's the only one we normally take at runtime, so getting rid of it should give you even greater power savings in conjunction with the RC6 timeout update patch I already pushed. Thanks, Jesse On Wed, 20 Nov 2013 16:33:22 + "S, Deepak" wrote: > Thanks Jesse, I wil split the patches and send it for review. > > Thanks > Deepak > > -Original Message- > From: Jesse Barnes [mailto:jbar...@virtuousgeek.org] > Sent: Wednesday, November 20, 2013 9:35 PM > To: S, Deepak > Cc: intel-gfx@lists.freedesktop.org > Subject: Re: [Intel-gfx] [PATCH] drm/i915/vlv: Add VLV specific force wake > routines. > > On Wed, 20 Nov 2013 06:00:24 + > "S, Deepak" wrote: > > > Hi Jesse, > > > > Thanks for the review. Below is my response. > > > > > - why not use the callback to __vlv_force_wake_* from > > gen6_gt_force_wake_*? i.e. why is VLV special here? > > [Deepak] Gen6 has a single power well whereas the VLV is has spate > > wells. This was the reason for the separate function > > > > > - having a new gen7_media_force_wake function may be better than > > passing an engine around, and would touch fewer pieces of code > > [Deepak] Even Gen7 is also as single Power Well. Having common function > > between gen7 and vlv might be difficult to individually handle the wells. > > > > >- have you done measurements on this? given how infrequently we > > ought to be waking the wells when they're idle, and how long we > > generally keep them awake, is this a real power win? > > [Deepak] By Individually controlling the wells we observed around 100mW - > > 200mW saving in different scenarios (GL Beanchmark & Media playback). > > wow, that's a significant savings. > > Can you split the patch into one that adds the power well arg, and another > that adds the VLV support for the split? That would make it easier to review. > > Thanks, > -- > Jesse Barnes, Intel Open Source Technology Center > -- Jesse Barnes, 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] drm/i915/vlv: Add VLV specific force wake routines.
Thanks Jesse. We will re-run and compare the power numbers. -Original Message- From: Jesse Barnes [mailto:jbar...@virtuousgeek.org] Sent: Saturday, November 23, 2013 2:43 AM To: S, Deepak Cc: intel-gfx@lists.freedesktop.org Subject: Re: [Intel-gfx] [PATCH] drm/i915/vlv: Add VLV specific force wake routines. Also, you might re-run your power numbers with Chris's patch to drop the force wake ref around the irq get/put. That's the only one we normally take at runtime, so getting rid of it should give you even greater power savings in conjunction with the RC6 timeout update patch I already pushed. Thanks, Jesse On Wed, 20 Nov 2013 16:33:22 + "S, Deepak" wrote: > Thanks Jesse, I wil split the patches and send it for review. > > Thanks > Deepak > > -Original Message- > From: Jesse Barnes [mailto:jbar...@virtuousgeek.org] > Sent: Wednesday, November 20, 2013 9:35 PM > To: S, Deepak > Cc: intel-gfx@lists.freedesktop.org > Subject: Re: [Intel-gfx] [PATCH] drm/i915/vlv: Add VLV specific force wake > routines. > > On Wed, 20 Nov 2013 06:00:24 + > "S, Deepak" wrote: > > > Hi Jesse, > > > > Thanks for the review. Below is my response. > > > > > - why not use the callback to __vlv_force_wake_* from > > gen6_gt_force_wake_*? i.e. why is VLV special here? > > [Deepak] Gen6 has a single power well whereas the VLV is has spate > > wells. This was the reason for the separate function > > > > > - having a new gen7_media_force_wake function may be better than > > passing an engine around, and would touch fewer pieces of code > > [Deepak] Even Gen7 is also as single Power Well. Having common function > > between gen7 and vlv might be difficult to individually handle the wells. > > > > >- have you done measurements on this? given how infrequently we > > ought to be waking the wells when they're idle, and how long we > > generally keep them awake, is this a real power win? > > [Deepak] By Individually controlling the wells we observed around 100mW - > > 200mW saving in different scenarios (GL Beanchmark & Media playback). > > wow, that's a significant savings. > > Can you split the patch into one that adds the power well arg, and another > that adds the VLV support for the split? That would make it easier to review. > > Thanks, > -- > Jesse Barnes, Intel Open Source Technology Center > -- Jesse Barnes, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
