<div dir="ltr">Hi Daniel,<div> </div><div style>I just checked the code and this patch looks right for me. </div><div style>it doesn't add any if block... just remove them.</div><div style>What am I missing?</div><div style> </div><div style>Thanks,</div><div style>Rodrigo.</div></div><div class="gmail_extra"> <div class="gmail_quote">On Tue, Mar 26, 2013 at 10:54 AM, Rodrigo Vivi <<a href="mailto:rodrigo.vivi@gmail.com" target="_blank">rodrigo.vivi@gmail.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">ops, when reworking to let the split as last patch I missed this one...<div>I'll resend it soon</div> </div><div class="gmail_extra"><div><div class="h5"> <div class="gmail_quote">On Tue, Mar 26, 2013 at 5:05 AM, Daniel Vetter <<a href="mailto:daniel@ffwll.ch" target="_blank">daniel@ffwll.ch</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div>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 <<a href="mailto:rodrigo.vivi@gmail.com" target="_blank">rodrigo.vivi@gmail.com</a>> </div>I think the split starts to make sense, but ... <div> > --- > 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); > - } </div>Adding an if block and then again killing it looks strange. -Daniel <div><div> > + 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 > </div></div>> _______________________________________________ > Intel-gfx mailing list > <a href="mailto:Intel-gfx@lists.freedesktop.org" target="_blank">Intel-gfx@lists.freedesktop.org</a> > <a href="http://lists.freedesktop.org/mailman/listinfo/intel-gfx" target="_blank">http://lists.freedesktop.org/mailman/listinfo/intel-gfx</a> -- Daniel Vetter Software Engineer, Intel Corporation <a href="tel:%2B41%20%280%29%2079%20365%2057%2048" value="+41793655748" target="_blank">+41 (0) 79 365 57 48</a> - <a href="http://blog.ffwll.ch" target="_blank">http://blog.ffwll.ch</a> </blockquote></div> <div> </div></div></div>-- <div>Rodrigo Vivi</div><div>Blog: <a href="http://blog.vivi.eng.br" target="_blank">http://blog.vivi.eng.br</a></div> <div> </div> </div> </blockquote></div> <div> </div>-- <div>Rodrigo Vivi</div><div>Blog: <a href="http://blog.vivi.eng.br" target="_blank">http://blog.vivi.eng.br</a></div><div> </div> </div>