From: Rodrigo Vivi <rodrigo.vivi@gmail.com>
To: Daniel Vetter <daniel@ffwll.ch>
Cc: intel-gfx <intel-gfx@lists.freedesktop.org>
Subject: Re: [PATCH 4/4] drm/i915: creating Haswell rc6 function
Date: Tue, 26 Mar 2013 13:25:30 -0300 [thread overview]
Message-ID: <CABVU7+sV5cJTX_6z-tpR1LKSyjE-wCtigi2rkMjC5p4sfOjoow@mail.gmail.com> (raw)
In-Reply-To: <CABVU7+vOTvBEBO-AdktvRk9w4LLyY1Gou_sz9+KE7pS9Q_jn2w@mail.gmail.com>
[-- Attachment #1.1: Type: text/plain, Size: 14017 bytes --]
Hi Daniel,
I just checked the code and this patch looks right for me.
it doesn't add any if block... just remove them.
What am I missing?
Thanks,
Rodrigo.
On Tue, Mar 26, 2013 at 10:54 AM, Rodrigo Vivi <rodrigo.vivi@gmail.com>wrote:
> ops, when reworking to let the split as last patch I missed this one...
> I'll resend it soon
>
>
> On Tue, Mar 26, 2013 at 5:05 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
>
>> On Mon, Mar 25, 2013 at 05:55:52PM -0300, Rodrigo Vivi wrote:
>> > Power management, in special RC6 enabling, differs across platforms.
>> > This patch just split out enabling function for HSW.
>> > This is an attempt to make pm code more clean without multiple
>> IS_HASWELL
>> > inside enable_rps function. This actually tends to get worse with
>> upcoming
>> > platforms.
>> >
>> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
>>
>> I think the split starts to make sense, but ...
>>
>> > ---
>> > drivers/gpu/drm/i915/intel_pm.c | 211
>> +++++++++++++++++++++++++++-------------
>> > 1 file changed, 146 insertions(+), 65 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/intel_pm.c
>> b/drivers/gpu/drm/i915/intel_pm.c
>> > index f6a7366..814846d 100644
>> > --- a/drivers/gpu/drm/i915/intel_pm.c
>> > +++ b/drivers/gpu/drm/i915/intel_pm.c
>> > @@ -2566,13 +2566,9 @@ static void gen6_enable_rps(struct drm_device
>> *dev)
>> > /* disable the counters and set deterministic thresholds */
>> > I915_WRITE(GEN6_RC_CONTROL, 0);
>> >
>> > - if (IS_HASWELL(dev))
>> > - I915_WRITE(GEN6_RC6_WAKE_RATE_LIMIT, 40 << 16);
>> > - else {
>> > - I915_WRITE(GEN6_RC1_WAKE_RATE_LIMIT, 1000 << 16);
>> > - I915_WRITE(GEN6_RC6_WAKE_RATE_LIMIT, 40 << 16 | 30);
>> > - I915_WRITE(GEN6_RC6pp_WAKE_RATE_LIMIT, 30);
>> > - }
>>
>> Adding an if block and then again killing it looks strange.
>> -Daniel
>>
>> > + I915_WRITE(GEN6_RC1_WAKE_RATE_LIMIT, 1000 << 16);
>> > + I915_WRITE(GEN6_RC6_WAKE_RATE_LIMIT, 40 << 16 | 30);
>> > + I915_WRITE(GEN6_RC6pp_WAKE_RATE_LIMIT, 30);
>> > I915_WRITE(GEN6_RC_EVALUATION_INTERVAL, 125000);
>> > I915_WRITE(GEN6_RC_IDLE_HYSTERSIS, 25);
>> >
>> > @@ -2580,27 +2576,19 @@ static void gen6_enable_rps(struct drm_device
>> *dev)
>> > I915_WRITE(RING_MAX_IDLE(ring->mmio_base), 10);
>> >
>> > I915_WRITE(GEN6_RC_SLEEP, 0);
>> > - if (!IS_HASWELL(dev))
>> > - I915_WRITE(GEN6_RC1e_THRESHOLD, 1000);
>> > + I915_WRITE(GEN6_RC1e_THRESHOLD, 1000);
>> > I915_WRITE(GEN6_RC6_THRESHOLD, 50000);
>> > - if (!IS_HASWELL(dev)) {
>> > - I915_WRITE(GEN6_RC6p_THRESHOLD, 150000);
>> > - I915_WRITE(GEN6_RC6pp_THRESHOLD, 64000); /* unused */
>> > - }
>> > + I915_WRITE(GEN6_RC6p_THRESHOLD, 150000);
>> > + 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;
>> > -
>> > - /* 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_RC6p_ENABLE)
>> > + rc6_mask |= GEN6_RC_CTL_RC6p_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_mask & GEN6_RC_CTL_RC6_ENABLE) ? "on" :
>> "off",
>> > @@ -2612,19 +2600,12 @@ static void gen6_enable_rps(struct drm_device
>> *dev)
>> > GEN6_RC_CTL_EI_MODE(1) |
>> > GEN6_RC_CTL_HW_ENABLE);
>> >
>> > - if (IS_HASWELL(dev)) {
>> > - I915_WRITE(GEN6_RPNSWREQ,
>> > - HSW_FREQUENCY(10));
>> > - I915_WRITE(GEN6_RC_VIDEO_FREQ,
>> > - HSW_FREQUENCY(12));
>> > - } else {
>> > - 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_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,
>> > @@ -2643,22 +2624,20 @@ static void gen6_enable_rps(struct drm_device
>> *dev)
>> > GEN6_RP_MEDIA_IS_GFX |
>> > GEN6_RP_ENABLE |
>> > GEN6_RP_UP_BUSY_AVG |
>> > - (IS_HASWELL(dev) ? GEN7_RP_DOWN_IDLE_AVG :
>> GEN6_RP_DOWN_IDLE_CONT));
>> > -
>> > - if (!IS_HASWELL(dev)) {
>> > - ret = sandybridge_pcode_write(dev_priv,
>> GEN6_PCODE_WRITE_MIN_FREQ_TABLE, 0);
>> > - if (!ret && (IS_GEN6(dev) || IS_IVYBRIDGE(dev))) {
>> > - pcu_mbox = 0;
>> > - ret = sandybridge_pcode_read(dev_priv,
>> GEN6_READ_OC_PARAMS, &pcu_mbox);
>> > - if (!ret && (pcu_mbox & (1<<31))) { /* OC
>> supported */
>> > - DRM_DEBUG_DRIVER("overclocking supported,
>> adjusting frequency max from %dMHz to %dMHz\n",
>> > - (dev_priv->rps.max_delay
>> & 0xff) * 50,
>> > - (pcu_mbox & 0xff) * 50);
>> > - dev_priv->rps.max_delay = pcu_mbox & 0xff;
>> > - }
>> > - } else {
>> > - DRM_DEBUG_DRIVER("Failed to set the min
>> frequency\n");
>> > + GEN6_RP_DOWN_IDLE_CONT);
>> > +
>> > + ret = sandybridge_pcode_write(dev_priv,
>> GEN6_PCODE_WRITE_MIN_FREQ_TABLE, 0);
>> > + if (!ret && (IS_GEN6(dev) || IS_IVYBRIDGE(dev))) {
>> > + pcu_mbox = 0;
>> > + ret = sandybridge_pcode_read(dev_priv,
>> GEN6_READ_OC_PARAMS, &pcu_mbox);
>> > + if (!ret && (pcu_mbox & (1<<31))) { /* OC supported */
>> > + DRM_DEBUG_DRIVER("overclocking supported,
>> adjusting frequency max from %dMHz to %dMHz\n",
>> > + (dev_priv->rps.max_delay & 0xff)
>> * 50,
>> > + (pcu_mbox & 0xff) * 50);
>> > + dev_priv->rps.max_delay = pcu_mbox & 0xff;
>> > }
>> > + } else {
>> > + DRM_DEBUG_DRIVER("Failed to set the min frequency\n");
>> > }
>> >
>> > gen6_set_rps(dev_priv->dev, (gt_perf_status & 0xff00) >> 8);
>> > @@ -2672,25 +2651,124 @@ static void gen6_enable_rps(struct drm_device
>> *dev)
>> > /* enable all PM interrupts */
>> > I915_WRITE(GEN6_PMINTRMSK, 0);
>> >
>> > - if (!IS_HASWELL(dev)) {
>> > - rc6vids = 0;
>> > - ret = sandybridge_pcode_read(dev_priv,
>> GEN6_PCODE_READ_RC6VIDS, &rc6vids);
>> > - if (IS_GEN6(dev) && ret) {
>> > - DRM_DEBUG_DRIVER("Couldn't check for BIOS
>> workaround\n");
>> > - } else if (IS_GEN6(dev) && (GEN6_DECODE_RC6_VID(rc6vids &
>> 0xff) < 450)) {
>> > - DRM_DEBUG_DRIVER("You should update your BIOS.
>> Correcting minimum rc6 voltage (%dmV->%dmV)\n",
>> > - GEN6_DECODE_RC6_VID(rc6vids &
>> 0xff), 450);
>> > - rc6vids &= 0xffff00;
>> > - rc6vids |= GEN6_ENCODE_RC6_VID(450);
>> > - ret = sandybridge_pcode_write(dev_priv,
>> GEN6_PCODE_WRITE_RC6VIDS, rc6vids);
>> > - if (ret)
>> > - DRM_ERROR("Couldn't fix incorrect rc6
>> voltage\n");
>> > - }
>> > + rc6vids = 0;
>> > + ret = sandybridge_pcode_read(dev_priv, GEN6_PCODE_READ_RC6VIDS,
>> &rc6vids);
>> > + if (IS_GEN6(dev) && ret) {
>> > + DRM_DEBUG_DRIVER("Couldn't check for BIOS workaround\n");
>> > + } else if (IS_GEN6(dev) && (GEN6_DECODE_RC6_VID(rc6vids & 0xff) <
>> 450)) {
>> > + DRM_DEBUG_DRIVER("You should update your BIOS. Correcting
>> minimum rc6 voltage (%dmV->%dmV)\n",
>> > + GEN6_DECODE_RC6_VID(rc6vids & 0xff), 450);
>> > + rc6vids &= 0xffff00;
>> > + rc6vids |= GEN6_ENCODE_RC6_VID(450);
>> > + ret = sandybridge_pcode_write(dev_priv,
>> GEN6_PCODE_WRITE_RC6VIDS, rc6vids);
>> > + if (ret)
>> > + DRM_ERROR("Couldn't fix incorrect rc6 voltage\n");
>> > }
>> >
>> > gen6_gt_force_wake_put(dev_priv);
>> > }
>> >
>> > +static void hsw_enable_rps(struct drm_device *dev)
>> > +{
>> > + struct drm_i915_private *dev_priv = dev->dev_private;
>> > + struct intel_ring_buffer *ring;
>> > + u32 rp_state_cap;
>> > + u32 gt_perf_status;
>> > + u32 rc6_mask = 0;
>> > + u32 gtfifodbg;
>> > + int rc6_mode;
>> > + int i;
>> > +
>> > + WARN_ON(!mutex_is_locked(&dev_priv->rps.hw_lock));
>> > +
>> > + /* 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);
>> > +
>> > + /* Clear the DBG now so we don't confuse earlier errors */
>> > + if ((gtfifodbg = I915_READ(GTFIFODBG))) {
>> > + DRM_ERROR("GT fifo had a previous error %x\n", gtfifodbg);
>> > + I915_WRITE(GTFIFODBG, gtfifodbg);
>> > + }
>> > +
>> > + gen6_gt_force_wake_get(dev_priv);
>> > +
>> > + rp_state_cap = I915_READ(GEN6_RP_STATE_CAP);
>> > + gt_perf_status = I915_READ(GEN6_GT_PERF_STATUS);
>> > +
>> > + /* In units of 100MHz */
>> > + dev_priv->rps.max_delay = rp_state_cap & 0xff;
>> > + dev_priv->rps.min_delay = (rp_state_cap & 0xff0000) >> 16;
>> > + dev_priv->rps.cur_delay = 0;
>> > +
>> > + /* disable the counters and set deterministic thresholds */
>> > + I915_WRITE(GEN6_RC_CONTROL, 0);
>> > +
>> > + 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_RC6_THRESHOLD, 50000);
>> > +
>> > + /* 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;
>> > +
>> > + DRM_INFO("Enabling RC6 states: RC6 %s\n",
>> > + (rc6_mask & GEN6_RC_CTL_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,
>> > + HSW_FREQUENCY(10));
>> > + I915_WRITE(GEN6_RC_VIDEO_FREQ,
>> > + HSW_FREQUENCY(12));
>> > +
>> > + I915_WRITE(GEN6_RP_DOWN_TIMEOUT, 1000000);
>> > + I915_WRITE(GEN6_RP_INTERRUPT_LIMITS,
>> > + dev_priv->rps.max_delay << 24 |
>> > + dev_priv->rps.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_NORMAL_MODE |
>> > + GEN6_RP_MEDIA_IS_GFX |
>> > + GEN6_RP_ENABLE |
>> > + GEN6_RP_UP_BUSY_AVG |
>> > + GEN7_RP_DOWN_IDLE_AVG);
>> > +
>> > + gen6_set_rps(dev_priv->dev, (gt_perf_status & 0xff00) >> 8);
>> > +
>> > + /* requires MSI enabled */
>> > + I915_WRITE(GEN6_PMIER, GEN6_PM_DEFERRED_EVENTS);
>> > + spin_lock_irq(&dev_priv->rps.lock);
>> > + WARN_ON(dev_priv->rps.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);
>> > +}
>> > +
>> > static void gen6_update_ring_freq(struct drm_device *dev)
>> > {
>> > struct drm_i915_private *dev_priv = dev->dev_private;
>> > @@ -3480,7 +3558,10 @@ static void intel_gen6_powersave_work(struct
>> work_struct *work)
>> > struct drm_device *dev = dev_priv->dev;
>> >
>> > mutex_lock(&dev_priv->rps.hw_lock);
>> > - gen6_enable_rps(dev);
>> > + if (IS_HASWELL(dev))
>> > + hsw_enable_rps(dev);
>> > + else
>> > + gen6_enable_rps(dev);
>> > gen6_update_ring_freq(dev);
>> > mutex_unlock(&dev_priv->rps.hw_lock);
>> > }
>> > --
>> > 1.8.1.4
>> >
>> > _______________________________________________
>> > Intel-gfx mailing list
>> > Intel-gfx@lists.freedesktop.org
>> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>
>> --
>> Daniel Vetter
>> Software Engineer, Intel Corporation
>> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
>>
>
>
>
> --
> Rodrigo Vivi
> Blog: http://blog.vivi.eng.br
>
>
--
Rodrigo Vivi
Blog: http://blog.vivi.eng.br
[-- Attachment #1.2: Type: text/html, Size: 17434 bytes --]
[-- Attachment #2: Type: text/plain, Size: 159 bytes --]
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
next prev parent reply other threads:[~2013-03-26 16:25 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-03-25 20:55 [PATCH 0/4] HSW PM - RC6 fixes, clean up and split Rodrigo Vivi
2013-03-25 20:55 ` [PATCH 1/4] drm/i915: HSW PM Frequency bits fix Rodrigo Vivi
2013-03-25 22:15 ` Ben Widawsky
2013-03-26 8:04 ` Daniel Vetter
2013-03-25 20:55 ` [PATCH 2/4] drm/i915: HSW PM Cleaning - Removing unecessary register/bits set Rodrigo Vivi
2013-03-25 20:55 ` [PATCH 3/4] drm/i915: HSW PM - removing pcode read/write Rodrigo Vivi
2013-03-26 8:02 ` Daniel Vetter
2013-03-26 16:30 ` Rodrigo Vivi
2013-03-25 20:55 ` [PATCH 4/4] drm/i915: creating Haswell rc6 function Rodrigo Vivi
2013-03-26 8:05 ` Daniel Vetter
2013-03-26 13:54 ` Rodrigo Vivi
2013-03-26 16:25 ` Rodrigo Vivi [this message]
2013-03-26 16:30 ` Daniel Vetter
2013-03-26 16:32 ` Rodrigo Vivi
2013-03-26 19:00 ` Daniel Vetter
2013-03-26 19:32 ` Rodrigo Vivi
2013-03-26 19:37 ` Rodrigo Vivi
2013-03-26 19:49 ` Daniel Vetter
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=CABVU7+sV5cJTX_6z-tpR1LKSyjE-wCtigi2rkMjC5p4sfOjoow@mail.gmail.com \
--to=rodrigo.vivi@gmail.com \
--cc=daniel@ffwll.ch \
--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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.