From: Daniel Vetter <daniel@ffwll.ch>
To: Eugeni Dodonov <eugeni.dodonov@intel.com>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 13/21] drm/i915: add RPS configuration for Haswell
Date: Fri, 29 Jun 2012 11:56:33 +0200 [thread overview]
Message-ID: <20120629095633.GA5463@phenom.ffwll.local> (raw)
In-Reply-To: <1340909749-15249-14-git-send-email-eugeni.dodonov@intel.com>
On Thu, Jun 28, 2012 at 03:55:41PM -0300, Eugeni Dodonov wrote:
> Split Haswell-specific GT algorithms into its own function.
>
> Note that Haswell only has RC6, so account for that as well.
>
> Signed-off-by: Eugeni Dodonov <eugeni.dodonov@intel.com>
> ---
> drivers/gpu/drm/i915/i915_reg.h | 1 +
> drivers/gpu/drm/i915/intel_pm.c | 160 ++++++++++++++++++++++++++++++++++++++--
> 2 files changed, 155 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 2c4be2e..0c53e4a 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -4131,6 +4131,7 @@
> #define GEN6_RP_UP_IDLE_MIN (0x1<<3)
> #define GEN6_RP_UP_BUSY_AVG (0x2<<3)
> #define GEN6_RP_UP_BUSY_CONT (0x4<<3)
> +#define GEN7_RP_DOWN_IDLE_AVG (0x2<<0)
> #define GEN6_RP_DOWN_IDLE_CONT (0x1<<0)
> #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 0334e42..0733f16 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -2301,6 +2301,26 @@ void gen6_set_rps(struct drm_device *dev, u8 val)
> dev_priv->cur_delay = val;
> }
>
> +static void hsw_disable_rps(struct drm_device *dev)
> +{
> + struct drm_i915_private *dev_priv = dev->dev_private;
> +
> + I915_WRITE(GEN6_RC_CONTROL, 0);
> + I915_WRITE(GEN6_RPNSWREQ, 1 << 31);
> + I915_WRITE(GEN6_PMINTRMSK, 0xffffffff);
> + I915_WRITE(GEN6_PMIER, 0);
> + /* Complete PM interrupt masking here doesn't race with the rps work
> + * item again unmasking PM interrupts because that is using a different
> + * register (PMIMR) to mask PM interrupts. The only risk is in leaving
> + * stale bits in PMIIR and PMIMR which gen6_enable_rps will clean up. */
> +
> + spin_lock_irq(&dev_priv->rps_lock);
> + dev_priv->pm_iir = 0;
> + spin_unlock_irq(&dev_priv->rps_lock);
> +
> + I915_WRITE(GEN6_PMIIR, I915_READ(GEN6_PMIIR));
> +}
The only difference I can see wrt gen6_disable_rps is clearing the
RC_CONTROL reg. And that looks like a bugfix that gen6+ should get, too.
Imo copy&pasting this function hence doesn't make sense.
> +
> static void gen6_disable_rps(struct drm_device *dev)
> {
> struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -2334,9 +2354,10 @@ int intel_enable_rc6(const struct drm_device *dev)
> if (INTEL_INFO(dev)->gen == 5)
> return 0;
>
> - /* Sorry Haswell, no RC6 for you for now. */
> + /* Haswell does not has RC6p nor RC6pp
> + */
> if (IS_HASWELL(dev))
> - return 0;
> + return INTEL_RC6_ENABLE;
>
> /*
> * Disable rc6 on Sandybridge
> @@ -2349,6 +2370,130 @@ int intel_enable_rc6(const struct drm_device *dev)
> return (INTEL_RC6_ENABLE | INTEL_RC6p_ENABLE);
> }
>
> +static void hsw_enable_rps(struct drm_device *dev)
> +{
> + struct drm_i915_private *dev_priv = dev->dev_private;
> + struct intel_ring_buffer *ring;
> + u32 pcu_mbox, rc6_mask = 0;
> + int rc6_mode;
> + int i;
> +
> + WARN_ON(!mutex_is_locked(&dev->struct_mutex));
> +
> + /* Here begins a magic sequence of register writes to enable
> + * auto-downclocking.
> + *
> + * Perhaps there might be some value in exposing these to
> + * userspace...
> + */
> + I915_WRITE(GEN6_RC_STATE, 0);
> +
> + gen6_gt_force_wake_get(dev_priv);
> +
> + /* In units of 100MHz */
> + dev_priv->max_delay = 18;
> + dev_priv->min_delay = 6;
> + dev_priv->cur_delay = 0;
gen6 enable_rps reads these magic values from regs. Shouldn't we do that
for hsw, too?
> +
> + /* disable the counters and set deterministic thresholds */
> + I915_WRITE(GEN6_RC_CONTROL, 0);
> +
> + I915_WRITE(GEN6_RC1_WAKE_RATE_LIMIT, 1000 << 16);
> + I915_WRITE(GEN6_RC6_WAKE_RATE_LIMIT, 40 << 16);
> + I915_WRITE(GEN6_RC_EVALUATION_INTERVAL, 125000);
> + I915_WRITE(GEN6_RC_IDLE_HYSTERSIS, 25);
> +
> + for_each_ring(ring, dev_priv, i)
> + I915_WRITE(RING_MAX_IDLE(ring->mmio_base), 10);
> +
> + I915_WRITE(GEN6_RC_SLEEP, 0);
> + I915_WRITE(GEN6_RC1e_THRESHOLD, 1000);
> + I915_WRITE(GEN6_RC6_THRESHOLD, 50000);
> +
> + rc6_mode = intel_enable_rc6(dev_priv->dev);
> +
> + if (rc6_mode & INTEL_RC6_ENABLE)
> + rc6_mask |= GEN6_RC_CTL_RC6_ENABLE;
> +
> + DRM_INFO("Enabling RC6 states on Haswell: RC6 %s\n",
> + (rc6_mode & INTEL_RC6_ENABLE) ? "on" : "off");
> +
> + I915_WRITE(GEN6_RC_CONTROL,
> + rc6_mask |
> + GEN6_RC_CTL_EI_MODE(1) |
> + GEN6_RC_CTL_HW_ENABLE);
> +
> + I915_WRITE(GEN6_RPNSWREQ,
> + GEN6_FREQUENCY(10) |
> + GEN6_OFFSET(0) |
> + GEN6_AGGRESSIVE_TURBO);
> + I915_WRITE(GEN6_RC_VIDEO_FREQ,
> + GEN6_FREQUENCY(12));
> +
> + I915_WRITE(GEN6_RP_DOWN_TIMEOUT, 1000000);
> + I915_WRITE(GEN6_RP_INTERRUPT_LIMITS,
> + dev_priv->max_delay << 24 |
> + dev_priv->min_delay << 16);
> + 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, 350000);
> + I915_WRITE(GEN6_RP_IDLE_HYSTERSIS, 10);
> + I915_WRITE(GEN6_RP_CONTROL,
> + GEN6_RP_MEDIA_TURBO |
> + GEN6_RP_MEDIA_HW_MODE |
I figure this should be changed like the gen6 rps code in 89ba829e ...
> + GEN6_RP_MEDIA_IS_GFX |
> + GEN6_RP_ENABLE |
> + GEN6_RP_UP_BUSY_AVG |
> + GEN7_RP_DOWN_IDLE_AVG);
> +
> + if (wait_for((I915_READ(GEN6_PCODE_MAILBOX) & GEN6_PCODE_READY) == 0,
> + 500))
> + DRM_ERROR("timeout waiting for pcode mailbox to become idle\n");
> +
> + I915_WRITE(GEN6_PCODE_DATA, 0);
> + I915_WRITE(GEN6_PCODE_MAILBOX,
> + GEN6_PCODE_READY |
> + GEN6_PCODE_WRITE_MIN_FREQ_TABLE);
> + if (wait_for((I915_READ(GEN6_PCODE_MAILBOX) & GEN6_PCODE_READY) == 0,
> + 500))
> + DRM_ERROR("timeout waiting for pcode mailbox to finish\n");
> +
> + /* Check for overclock support */
> + if (wait_for((I915_READ(GEN6_PCODE_MAILBOX) & GEN6_PCODE_READY) == 0,
> + 500))
> + DRM_ERROR("timeout waiting for pcode mailbox to become idle\n");
> + I915_WRITE(GEN6_PCODE_MAILBOX, GEN6_READ_OC_PARAMS);
> + pcu_mbox = I915_READ(GEN6_PCODE_DATA);
> + if (wait_for((I915_READ(GEN6_PCODE_MAILBOX) & GEN6_PCODE_READY) == 0,
> + 500))
> + DRM_ERROR("timeout waiting for pcode mailbox to finish\n");
> + if (pcu_mbox & (1<<31)) { /* OC supported */
> + dev_priv->max_delay = pcu_mbox & 0xff;
> + DRM_DEBUG_DRIVER("overclocking supported, adjusting frequency max to %dMHz\n", pcu_mbox * 50);
> + }
> +
> + /* requires MSI enabled */
> + I915_WRITE(GEN6_PMIER,
> + GEN6_PM_MBOX_EVENT |
> + GEN6_PM_THERMAL_EVENT |
> + GEN6_PM_RP_DOWN_TIMEOUT |
> + GEN6_PM_RP_UP_THRESHOLD |
> + GEN6_PM_RP_DOWN_THRESHOLD |
> + GEN6_PM_RP_UP_EI_EXPIRED |
> + GEN6_PM_RP_DOWN_EI_EXPIRED);
> +
> + spin_lock_irq(&dev_priv->rps_lock);
> + WARN_ON(dev_priv->pm_iir != 0);
> + I915_WRITE(GEN6_PMIMR, 0);
> + spin_unlock_irq(&dev_priv->rps_lock);
> +
> + /* enable all PM interrupts */
> + I915_WRITE(GEN6_PMINTRMSK, 0);
> +
> + gen6_gt_force_wake_put(dev_priv);
> +}
Ok, after reviewing this with the gen6 code and hsw power doc I see very
few differences (besides the 2 things that look like mistakes):
- we don't set a few things related to rc6p&rc6pp
- we have slightly different timeout/threshold values.
When copy&pasting code we should balance the risk of introducing
regressions with the risk to not apply bugfixes everywhere they're needed.
Given that you've missed Jesse's fix while hacking on this and that this
seems to work very much like rps on snb/ivb I'd vote for sprinkling a few
ifs into gen6_enable_rps. Maybe set the timing values in local variables
at the beginning of the function.
-Daniel
> +
> static void gen6_enable_rps(struct drm_device *dev)
> {
> struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -3227,7 +3372,9 @@ void intel_disable_gt_powersave(struct drm_device *dev)
> {
> if (IS_IRONLAKE_M(dev))
> ironlake_disable_drps(dev);
> - if (INTEL_INFO(dev)->gen >= 6 && !IS_VALLEYVIEW(dev))
> + else if (IS_HASWELL(dev))
> + hsw_disable_rps(dev);
> + else if (INTEL_INFO(dev)->gen >= 6 && !IS_VALLEYVIEW(dev))
> gen6_disable_rps(dev);
> }
>
> @@ -3237,9 +3384,10 @@ void intel_enable_gt_powersave(struct drm_device *dev)
> ironlake_enable_drps(dev);
> ironlake_enable_rc6(dev);
> intel_init_emon(dev);
> - }
> -
> - if ((IS_GEN6(dev) || IS_GEN7(dev)) && !IS_VALLEYVIEW(dev)) {
> + } else if (IS_HASWELL(dev)) {
> + hsw_enable_rps(dev);
> + gen6_update_ring_freq(dev);
> + } else if ((IS_GEN6(dev) || IS_GEN7(dev)) && !IS_VALLEYVIEW(dev)) {
> gen6_enable_rps(dev);
> gen6_update_ring_freq(dev);
> }
> --
> 1.7.11.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48
next prev parent reply other threads:[~2012-06-29 9:56 UTC|newest]
Thread overview: 44+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-06-28 18:55 [PATCH 00/21] More Haswell patches Eugeni Dodonov
2012-06-28 18:55 ` [PATCH 01/21] drm/i915: Move DP structs to shared location Eugeni Dodonov
2012-06-28 18:55 ` [PATCH 02/21] drm/i915: Add support for DDI control DP outputs Eugeni Dodonov
2012-06-28 18:55 ` [PATCH 03/21] drm/i915: Add DP Helper functions for Haswell Eugeni Dodonov
2012-06-28 18:55 ` [PATCH 04/21] drm/i915: Haswell specific code for the DP Link Training Eugeni Dodonov
2012-06-28 18:55 ` [PATCH 05/21] drm/i915: Disable DDI Pipe Control on HSW while disabling pipe Eugeni Dodonov
2012-06-28 18:55 ` [PATCH 06/21] drm/i915: Hook DP init in ddi module Eugeni Dodonov
2012-06-28 18:55 ` [PATCH 07/21] drm/i915: re-initialize DDI buffer translations after resume Eugeni Dodonov
2012-07-04 20:07 ` Paulo Zanoni
2012-07-04 20:35 ` Daniel Vetter
2012-07-04 23:13 ` Eugeni Dodonov
2012-06-28 18:55 ` [PATCH 08/21] drm/i915: simplify FDI RX check for LPT Eugeni Dodonov
2012-06-28 18:55 ` [PATCH 09/21] drm/i915: account for only one transcoder on LPT Eugeni Dodonov
2012-06-28 18:55 ` [PATCH 10/21] drm/i915: introduce lpt_enable_pch and cpt_enable_pch Eugeni Dodonov
2012-07-04 18:21 ` Paulo Zanoni
2012-07-06 20:47 ` Eugeni Dodonov
2012-06-28 18:55 ` [PATCH 11/21] drm/i915: program FDI_RX TP and FDI delays Eugeni Dodonov
2012-07-04 21:15 ` Paulo Zanoni
2012-07-04 23:15 ` [PATCH 10/31] " Eugeni Dodonov
2012-07-05 12:58 ` Paulo Zanoni
2012-07-05 13:12 ` Daniel Vetter
2012-06-28 18:55 ` [PATCH 12/21] drm/i915: support Haswell-style force waking Eugeni Dodonov
2012-06-28 19:38 ` Daniel Vetter
2012-06-28 20:06 ` Eugeni Dodonov
2012-06-28 18:55 ` [PATCH 13/21] drm/i915: add RPS configuration for Haswell Eugeni Dodonov
2012-06-29 9:56 ` Daniel Vetter [this message]
2012-06-29 13:49 ` Eugeni Dodonov
2012-06-28 18:55 ` [PATCH 14/21] drm/i915: Add EDP Registers " Eugeni Dodonov
2012-06-28 18:55 ` [PATCH 15/21] drm/i915: Timing initialization for eDP on HSW Eugeni Dodonov
2012-06-28 18:55 ` [PATCH 16/21] drm/i915: Modesetting for eDP on HSw Eugeni Dodonov
2012-06-28 18:55 ` [PATCH 17/21] drm/i915: Hook eDP initialization on DDI A Eugeni Dodonov
2012-06-28 18:55 ` [PATCH 18/21] drm/i915: introduce haswell_init_clock_gating Eugeni Dodonov
2012-06-28 18:55 ` [PATCH 19/21] drm/i915: prevent bogus intel_update_fbc notifications Eugeni Dodonov
2012-06-28 19:24 ` Daniel Vetter
2012-06-28 20:11 ` Eugeni Dodonov
2012-07-04 17:41 ` Paulo Zanoni
2012-07-04 23:19 ` Eugeni Dodonov
2012-07-05 7:47 ` Daniel Vetter
2012-06-28 18:55 ` [PATCH 20/21] drm/i915: fix PIPE_WM_LINETIME definition Eugeni Dodonov
2012-06-28 19:39 ` Daniel Vetter
2012-06-28 18:55 ` [PATCH 21/21] drm/i915: enable RC6 workaround on Haswell Eugeni Dodonov
2012-06-28 19:23 ` Daniel Vetter
2012-06-28 20:10 ` Eugeni Dodonov
2012-06-28 19:22 ` [PATCH 00/21] More Haswell patches Paulo Zanoni
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20120629095633.GA5463@phenom.ffwll.local \
--to=daniel@ffwll.ch \
--cc=eugeni.dodonov@intel.com \
--cc=intel-gfx@lists.freedesktop.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox