| Message ID | 1362768363-3710-15-git-send-email-jbarnes@virtuousgeek.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Mar 08, 2013 at 10:45:58AM -0800, Jesse Barnes wrote: > From: Ben Widawsky <ben@bwidawsk.net> > > Uses slightly different interfaces than other platforms. > > v2: track actual set freq, not requested (Rohit) > fix debug prints in init code (Jesse) > > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> One important question is how is our punit code going to interact with the other potential users? It seems a bunch of power management drivers would also want to touch this uC. Aside from that, I have quite a few things below which as long as you address, I'll happily add an r-b. > --- > drivers/gpu/drm/i915/i915_drv.h | 1 + > drivers/gpu/drm/i915/i915_irq.c | 5 +- > drivers/gpu/drm/i915/intel_pm.c | 150 +++++++++++++++++++++++++++++++++++++-- > 3 files changed, 151 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 592e944..34414d1 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -1820,6 +1820,7 @@ extern void intel_disable_fbc(struct drm_device *dev); > extern bool ironlake_set_drps(struct drm_device *dev, u8 val); > extern void intel_init_pch_refclk(struct drm_device *dev); > extern void gen6_set_rps(struct drm_device *dev, u8 val); > +extern void valleyview_set_rps(struct drm_device *dev, u8 val); > extern void intel_detect_pch(struct drm_device *dev); > extern int intel_trans_dp_port_sel(struct drm_crtc *crtc); > extern int intel_enable_rc6(const struct drm_device *dev); > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c > index 2139714..65120e1 100644 > --- a/drivers/gpu/drm/i915/i915_irq.c > +++ b/drivers/gpu/drm/i915/i915_irq.c > @@ -395,7 +395,10 @@ static void gen6_pm_rps_work(struct work_struct *work) > */ > if (!(new_delay > dev_priv->rps.max_delay || > new_delay < dev_priv->rps.min_delay)) { > - gen6_set_rps(dev_priv->dev, new_delay); > + if (IS_VALLEYVIEW(dev_priv->dev)) > + valleyview_set_rps(dev_priv->dev, new_delay); > + else > + gen6_set_rps(dev_priv->dev, new_delay); > } > > mutex_unlock(&dev_priv->rps.hw_lock); > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > index 70eab45..d0b8d58 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -2477,6 +2477,47 @@ void gen6_set_rps(struct drm_device *dev, u8 val) > trace_intel_gpu_freq_change(val * 50); > } > > +void valleyview_set_rps(struct drm_device *dev, u8 val) > +{ > + struct drm_i915_private *dev_priv = dev->dev_private; > + unsigned long timeout = jiffies + msecs_to_jiffies(100); > + u32 limits = gen6_rps_limits(dev_priv, &val); > + u32 pval; > + > + WARN_ON(!mutex_is_locked(&dev_priv->rps.hw_lock)); > + WARN_ON(val > dev_priv->rps.max_delay); > + WARN_ON(val < dev_priv->rps.min_delay); > + > + if (val == dev_priv->rps.cur_delay) > + return; > + > + valleyview_punit_write(dev_priv, PUNIT_REG_GPU_FREQ_REQ, val); > + > + do { > + valleyview_punit_read(dev_priv, PUNIT_REG_GPU_FREQ_STS, &pval); > + if (time_after(jiffies, timeout)) { > + DRM_DEBUG_DRIVER("timed out waiting for Punit\n"); > + break; > + } > + udelay(10); > + } while (pval & 1); > + > + valleyview_punit_read(dev_priv, PUNIT_REG_GPU_FREQ_STS, &pval); > + if ((pval >> 8) != val) > + DRM_DEBUG_DRIVER("punit overrode freq: %d requested, but got %d\n", > + val, pval >> 8); I'm debating whether this is a useful thing to do here... You either get the frequency or you don't. Really it would seem more useful to me to check things are sane when you first enter the function (ie. did the last request do what you want). But I don't care what you end up with. I suppose if you wanted to make things a bit cleaner you could extract just the frequency setting part, since most of this function is identical to gen6 set rps. > + > + /* Make sure we continue to get interrupts > + * until we hit the minimum or maximum frequencies. > + */ > + I915_WRITE(GEN6_RP_INTERRUPT_LIMITS, limits); > + > + dev_priv->rps.cur_delay = pval >> 8; > + > + trace_intel_gpu_freq_change(val * 50); Based on our IRC discussion, I believe the value in this trace is wrong. > +} > + > + > static void gen6_disable_rps(struct drm_device *dev) > { > struct drm_i915_private *dev_priv = dev->dev_private; > @@ -2714,6 +2755,102 @@ static void gen6_update_ring_freq(struct drm_device *dev) > } > } > > +static void valleyview_enable_rps(struct drm_device *dev) > +{ > + struct drm_i915_private *dev_priv = dev->dev_private; > + struct intel_ring_buffer *ring; > + u32 gtfifodbg, val; > + int i; > + > + WARN_ON(!mutex_is_locked(&dev_priv->rps.hw_lock)); Should we check FB_GFX_TURBO_EN_FUSE here? > + > + 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); > + > + I915_WRITE(GEN6_RC_SLEEP, 0); You've dropped GEN6_RC1_WAKE_RATE_LIMIT. Intentional? > + I915_WRITE(GEN6_RC6_WAKE_RATE_LIMIT, 0x00280000); > + I915_WRITE(GEN6_RC_EVALUATION_INTERVAL, 125000); > + I915_WRITE(GEN6_RC_IDLE_HYSTERSIS, 0x19); Don't use 0x19, use 25 since we use 25 in the gen6 path > + > + for_each_ring(ring, dev_priv, i) > + I915_WRITE(RING_MAX_IDLE(ring->mmio_base), 10); > + > + I915_WRITE(GEN6_RC1e_THRESHOLD, 1000); > + I915_WRITE(GEN6_RC6_THRESHOLD, 0xc350); > + > + 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); I can't find most of the above values, but, I'll assume they're correct. I'd also suggest converging on either all decimal, or all hex, just to make things less confusing. > + > + 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 | > + GEN6_RP_DOWN_IDLE_CONT); > + > + /* allows RC6 residency counter to work */ > + I915_WRITE(0x138104, 0xffff00ff); This is writing read only registers. Should be: I915_WRITE(0x138104, 0x800000ff); Also, just for though, we may want to use the upper bits instead of the lower ones... > + I915_WRITE(GEN6_RC_CONTROL, GEN6_RC_CTL_HW_ENABLE); > + So the following stuff doesn't seem to jive in the docs I'm reading. Can you double check your docs, and the silicon. I tried to punt this to IRC but you didn't respond. These IOSF registers are all 32 bits, so I'm not sure how this is supposed to work. > + valleyview_punit_read(dev_priv, PUNIT_FUSE_BUS1, &val); > + DRM_DEBUG_DRIVER("max GPU freq: %d\n", val); > + dev_priv->rps.max_delay = val; val >> 16 && 0xff? > + > + valleyview_punit_read(dev_priv, PUNIT_REG_GPU_LFM, &val); > + DRM_DEBUG_DRIVER("min GPU freq: %d\n", val); > + dev_priv->rps.min_delay = val; val & 0xff? > + > + valleyview_punit_read(dev_priv, PUNIT_FUSE_BUS2, &val); > + DRM_DEBUG_DRIVER("max GPLL freq: %d\n", val); One doc suggests this is actually offset 0x87, bits 7:0 > + > + valleyview_punit_read(dev_priv, PUNIT_REG_GPU_FREQ_STS, &val); > + DRM_DEBUG_DRIVER("DDR speed: "); > + if (drm_debug & DRM_UT_DRIVER) { > + if (((val >> 6) & 3) == 0) { > + dev_priv->mem_freq = 800; > + printk("800 MHz\n"); > + } else if (((val >> 6) & 3) == 1) { > + printk("1066 MHz\n"); > + dev_priv->mem_freq = 1066; > + } else if (((val >> 6) & 3) == 2) { > + printk("1333 MHz\n"); > + dev_priv->mem_freq = 1333; > + } else if (((val >> 6) & 3) == 3) > + printk("invalid\n"); > + } printk? could be one line, but would print 1332: dev_priv->mem_freq = 800 + (266 * (val >> 6) & 3)) > + DRM_DEBUG_DRIVER("GPLL enabled? %s\n", val & 8 ? "yes" : "no"); > + DRM_DEBUG_DRIVER("GPU status: 0x%08x\n", val); The docs match on this one \o/ > + > + DRM_DEBUG_DRIVER("current GPU freq: %x\n", (val >> 8) & 0xff); > + dev_priv->rps.cur_delay = (val >> 8) & 0xff; Both docs match here too. > + > + val = 0xd500; > + DRM_DEBUG_DRIVER("setting GPU freq to %d\n", (val >> 8) & 0xff); > + > + valleyview_set_rps(dev_priv->dev, (val >> 8) & 0xff); > + > + /* 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); > +} > + > void ironlake_teardown_rc6(struct drm_device *dev) > { > struct drm_i915_private *dev_priv = dev->dev_private; > @@ -3440,7 +3577,7 @@ void intel_disable_gt_powersave(struct drm_device *dev) > if (IS_IRONLAKE_M(dev)) { > ironlake_disable_drps(dev); > ironlake_disable_rc6(dev); > - } else if (INTEL_INFO(dev)->gen >= 6 && !IS_VALLEYVIEW(dev)) { > + } else if (INTEL_INFO(dev)->gen >= 6) { > cancel_delayed_work_sync(&dev_priv->rps.delayed_resume_work); > mutex_lock(&dev_priv->rps.hw_lock); > gen6_disable_rps(dev); > @@ -3456,8 +3593,13 @@ 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); > - gen6_update_ring_freq(dev); > + > + if (IS_VALLEYVIEW(dev)) { > + valleyview_enable_rps(dev); > + } else { > + gen6_enable_rps(dev); > + gen6_update_ring_freq(dev); > + } > mutex_unlock(&dev_priv->rps.hw_lock); > } > > @@ -3469,7 +3611,7 @@ void intel_enable_gt_powersave(struct drm_device *dev) > ironlake_enable_drps(dev); > ironlake_enable_rc6(dev); > intel_init_emon(dev); > - } else if ((IS_GEN6(dev) || IS_GEN7(dev)) && !IS_VALLEYVIEW(dev)) { > + } else if (IS_GEN6(dev) || IS_GEN7(dev)) { > /* > * PCU communication is slow and this doesn't need to be > * done at any specific time, so do this out of our fast path > -- > 1.7.10.4 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Tue, 19 Mar 2013 15:27:36 -0700 Ben Widawsky <ben@bwidawsk.net> wrote: > On Fri, Mar 08, 2013 at 10:45:58AM -0800, Jesse Barnes wrote: > > From: Ben Widawsky <ben@bwidawsk.net> > > > > Uses slightly different interfaces than other platforms. > > > > v2: track actual set freq, not requested (Rohit) > > fix debug prints in init code (Jesse) > > > > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> > > One important question is how is our punit code going to interact with > the other potential users? It seems a bunch of power management drivers > would also want to touch this uC. Pretty sure the PUnit has to deal with concurrent access... if not we'll have to add common routines for all drivers to use and do proper locking. > > + > > + valleyview_punit_read(dev_priv, PUNIT_REG_GPU_FREQ_STS, &pval); > > + if ((pval >> 8) != val) > > + DRM_DEBUG_DRIVER("punit overrode freq: %d requested, but got %d\n", > > + val, pval >> 8); > I'm debating whether this is a useful thing to do here... You either get > the frequency or you don't. Really it would seem more useful to me to > check things are sane when you first enter the function (ie. did the > last request do what you want). But I don't care what you end up with. It's not really a matter of sanity, it's more about what state the platform is in. If the Punit decides things are getting too hot for example, it may clamp your freq down. That's totally ok and normal though, and may change in future calls. > I suppose if you wanted to make things a bit cleaner you could extract > just the frequency setting part, since most of this function is > identical to gen6 set rps. I was afraid more changes would creep in over time, but yeah that's a possibility. I have a couple more changes here before I consider that. > > + > > + /* Make sure we continue to get interrupts > > + * until we hit the minimum or maximum frequencies. > > + */ > > + I915_WRITE(GEN6_RP_INTERRUPT_LIMITS, limits); > > + > > + dev_priv->rps.cur_delay = pval >> 8; > > + > > + trace_intel_gpu_freq_change(val * 50); > Based on our IRC discussion, I believe the value in this trace is wrong. Ah yeah, correct. I can just trace val here maybe, or I"ll have to put this off until I post the real frequencies. > > +static void valleyview_enable_rps(struct drm_device *dev) > > +{ > > + struct drm_i915_private *dev_priv = dev->dev_private; > > + struct intel_ring_buffer *ring; > > + u32 gtfifodbg, val; > > + int i; > > + > > + WARN_ON(!mutex_is_locked(&dev_priv->rps.hw_lock)); > > Should we check FB_GFX_TURBO_EN_FUSE here? I guess it wouldn't hurt. > > + > > + 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); > > + > > + I915_WRITE(GEN6_RC_SLEEP, 0); > You've dropped GEN6_RC1_WAKE_RATE_LIMIT. Intentional? Uh I should have commented that. I *think* it was intentional since RC1 doesn't exist on VLV, but I'll have to check. > > + I915_WRITE(GEN6_RC6_WAKE_RATE_LIMIT, 0x00280000); > > + I915_WRITE(GEN6_RC_EVALUATION_INTERVAL, 125000); > > + I915_WRITE(GEN6_RC_IDLE_HYSTERSIS, 0x19); > > Don't use 0x19, use 25 since we use 25 in the gen6 path Ok. > > + > > + for_each_ring(ring, dev_priv, i) > > + I915_WRITE(RING_MAX_IDLE(ring->mmio_base), 10); > > + > > + I915_WRITE(GEN6_RC1e_THRESHOLD, 1000); > > + I915_WRITE(GEN6_RC6_THRESHOLD, 0xc350); > > + > > + 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); > > I can't find most of the above values, but, I'll assume they're correct. > I'd also suggest converging on either all decimal, or all hex, just to > make things less confusing. A comment about units probably wouldn't hurt either. Ok. > > + > > + 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 | > > + GEN6_RP_DOWN_IDLE_CONT); > > + > > > + /* allows RC6 residency counter to work */ > > + I915_WRITE(0x138104, 0xffff00ff); > This is writing read only registers. Should be: > I915_WRITE(0x138104, 0x800000ff); > > Also, just for though, we may want to use the upper bits instead of the > lower ones... I think the upper bits are a mask, and all the low 8 bits are writeable, so maybe 0x00ff00ff? I'll have to test that. > > > + I915_WRITE(GEN6_RC_CONTROL, GEN6_RC_CTL_HW_ENABLE); > > + > > So the following stuff doesn't seem to jive in the docs I'm reading. Can > you double check your docs, and the silicon. I tried to punt this to IRC > but you didn't respond. These IOSF registers are all 32 bits, so I'm not > sure how this is supposed to work. > > > + valleyview_punit_read(dev_priv, PUNIT_FUSE_BUS1, &val); > > + DRM_DEBUG_DRIVER("max GPU freq: %d\n", val); > > + dev_priv->rps.max_delay = val; > val >> 16 && 0xff? > > > > + > > + valleyview_punit_read(dev_priv, PUNIT_REG_GPU_LFM, &val); > > + DRM_DEBUG_DRIVER("min GPU freq: %d\n", val); > > + dev_priv->rps.min_delay = val; > val & 0xff? > > + > > + valleyview_punit_read(dev_priv, PUNIT_FUSE_BUS2, &val); > > + DRM_DEBUG_DRIVER("max GPLL freq: %d\n", val); > > One doc suggests this is actually offset 0x87, bits 7:0 Hm I'll check these out; the different docs talk about different bit fields, but I think they're offsets into the fuse bus array. > > > + > > + valleyview_punit_read(dev_priv, PUNIT_REG_GPU_FREQ_STS, &val); > > + DRM_DEBUG_DRIVER("DDR speed: "); > > + if (drm_debug & DRM_UT_DRIVER) { > > + if (((val >> 6) & 3) == 0) { > > + dev_priv->mem_freq = 800; > > + printk("800 MHz\n"); > > + } else if (((val >> 6) & 3) == 1) { > > + printk("1066 MHz\n"); > > + dev_priv->mem_freq = 1066; > > + } else if (((val >> 6) & 3) == 2) { > > + printk("1333 MHz\n"); > > + dev_priv->mem_freq = 1333; > > + } else if (((val >> 6) & 3) == 3) > > + printk("invalid\n"); > > + } > > printk? > could be one line, but would print 1332: > dev_priv->mem_freq = 800 + (266 * (val >> 6) & 3)) Yeah because the DRM_DEBUG above doesn't have a newline. I like your calculation better. Thanks for checking these out; I know wading through these docs is no fun.
On Tue, Mar 19, 2013 at 03:35:55PM -0700, Jesse Barnes wrote: > On Tue, 19 Mar 2013 15:27:36 -0700 > Ben Widawsky <ben@bwidawsk.net> wrote: > > > On Fri, Mar 08, 2013 at 10:45:58AM -0800, Jesse Barnes wrote: > > > From: Ben Widawsky <ben@bwidawsk.net> > > > > > > Uses slightly different interfaces than other platforms. > > > > > > v2: track actual set freq, not requested (Rohit) > > > fix debug prints in init code (Jesse) > > > > > > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> > > > > One important question is how is our punit code going to interact with > > the other potential users? It seems a bunch of power management drivers > > would also want to touch this uC. > > Pretty sure the PUnit has to deal with concurrent access... if not > we'll have to add common routines for all drivers to use and do proper > locking. > > > > + > > > + valleyview_punit_read(dev_priv, PUNIT_REG_GPU_FREQ_STS, &pval); > > > + if ((pval >> 8) != val) > > > + DRM_DEBUG_DRIVER("punit overrode freq: %d requested, but got %d\n", > > > + val, pval >> 8); > > I'm debating whether this is a useful thing to do here... You either get > > the frequency or you don't. Really it would seem more useful to me to > > check things are sane when you first enter the function (ie. did the > > last request do what you want). But I don't care what you end up with. > > It's not really a matter of sanity, it's more about what state the > platform is in. If the Punit decides things are getting too hot for > example, it may clamp your freq down. That's totally ok and normal > though, and may change in future calls. > > > I suppose if you wanted to make things a bit cleaner you could extract > > just the frequency setting part, since most of this function is > > identical to gen6 set rps. > > I was afraid more changes would creep in over time, but yeah that's a > possibility. I have a couple more changes here before I consider that. > > > > + > > > + /* Make sure we continue to get interrupts > > > + * until we hit the minimum or maximum frequencies. > > > + */ > > > + I915_WRITE(GEN6_RP_INTERRUPT_LIMITS, limits); > > > + > > > + dev_priv->rps.cur_delay = pval >> 8; > > > + > > > + trace_intel_gpu_freq_change(val * 50); > > Based on our IRC discussion, I believe the value in this trace is wrong. > > Ah yeah, correct. I can just trace val here maybe, or I"ll have to put > this off until I post the real frequencies. > > > > +static void valleyview_enable_rps(struct drm_device *dev) > > > +{ > > > + struct drm_i915_private *dev_priv = dev->dev_private; > > > + struct intel_ring_buffer *ring; > > > + u32 gtfifodbg, val; > > > + int i; > > > + > > > + WARN_ON(!mutex_is_locked(&dev_priv->rps.hw_lock)); > > > > Should we check FB_GFX_TURBO_EN_FUSE here? > > I guess it wouldn't hurt. > > > > + > > > + 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); > > > + > > > + I915_WRITE(GEN6_RC_SLEEP, 0); > > You've dropped GEN6_RC1_WAKE_RATE_LIMIT. Intentional? > > Uh I should have commented that. I *think* it was intentional since > RC1 doesn't exist on VLV, but I'll have to check. > > > > + I915_WRITE(GEN6_RC6_WAKE_RATE_LIMIT, 0x00280000); > > > + I915_WRITE(GEN6_RC_EVALUATION_INTERVAL, 125000); > > > + I915_WRITE(GEN6_RC_IDLE_HYSTERSIS, 0x19); > > > > Don't use 0x19, use 25 since we use 25 in the gen6 path > > Ok. > > > > + > > > + for_each_ring(ring, dev_priv, i) > > > + I915_WRITE(RING_MAX_IDLE(ring->mmio_base), 10); > > > + > > > + I915_WRITE(GEN6_RC1e_THRESHOLD, 1000); > > > + I915_WRITE(GEN6_RC6_THRESHOLD, 0xc350); > > > + > > > + 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); > > > > I can't find most of the above values, but, I'll assume they're correct. > > I'd also suggest converging on either all decimal, or all hex, just to > > make things less confusing. > > A comment about units probably wouldn't hurt either. Ok. > > > > + > > > + 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 | > > > + GEN6_RP_DOWN_IDLE_CONT); > > > + > > > > > + /* allows RC6 residency counter to work */ > > > + I915_WRITE(0x138104, 0xffff00ff); > > This is writing read only registers. Should be: > > I915_WRITE(0x138104, 0x800000ff); > > > > Also, just for though, we may want to use the upper bits instead of the > > lower ones... > > I think the upper bits are a mask, and all the low 8 bits are > writeable, so maybe 0x00ff00ff? I'll have to test that. This was a typo on my part, it should be 0x80ff00ff. Sorry about that. I haven't read the rest of the email yet. It popped into my head while I was out. > > > > > > + I915_WRITE(GEN6_RC_CONTROL, GEN6_RC_CTL_HW_ENABLE); > > > + > > > > So the following stuff doesn't seem to jive in the docs I'm reading. Can > > you double check your docs, and the silicon. I tried to punt this to IRC > > but you didn't respond. These IOSF registers are all 32 bits, so I'm not > > sure how this is supposed to work. > > > > > + valleyview_punit_read(dev_priv, PUNIT_FUSE_BUS1, &val); > > > + DRM_DEBUG_DRIVER("max GPU freq: %d\n", val); > > > + dev_priv->rps.max_delay = val; > > val >> 16 && 0xff? > > > > > > > + > > > + valleyview_punit_read(dev_priv, PUNIT_REG_GPU_LFM, &val); > > > + DRM_DEBUG_DRIVER("min GPU freq: %d\n", val); > > > + dev_priv->rps.min_delay = val; > > val & 0xff? > > > + > > > + valleyview_punit_read(dev_priv, PUNIT_FUSE_BUS2, &val); > > > + DRM_DEBUG_DRIVER("max GPLL freq: %d\n", val); > > > > One doc suggests this is actually offset 0x87, bits 7:0 > > Hm I'll check these out; the different docs talk about different bit > fields, but I think they're offsets into the fuse bus array. > > > > > > + > > > + valleyview_punit_read(dev_priv, PUNIT_REG_GPU_FREQ_STS, &val); > > > + DRM_DEBUG_DRIVER("DDR speed: "); > > > + if (drm_debug & DRM_UT_DRIVER) { > > > + if (((val >> 6) & 3) == 0) { > > > + dev_priv->mem_freq = 800; > > > + printk("800 MHz\n"); > > > + } else if (((val >> 6) & 3) == 1) { > > > + printk("1066 MHz\n"); > > > + dev_priv->mem_freq = 1066; > > > + } else if (((val >> 6) & 3) == 2) { > > > + printk("1333 MHz\n"); > > > + dev_priv->mem_freq = 1333; > > > + } else if (((val >> 6) & 3) == 3) > > > + printk("invalid\n"); > > > + } > > > > printk? > > could be one line, but would print 1332: > > dev_priv->mem_freq = 800 + (266 * (val >> 6) & 3)) > > Yeah because the DRM_DEBUG above doesn't have a newline. I like your > calculation better. > > Thanks for checking these out; I know wading through these docs is no > fun. > > -- > Jesse Barnes, Intel Open Source Technology Center > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Tue, Mar 19, 2013 at 03:35:55PM -0700, Jesse Barnes wrote: > On Tue, 19 Mar 2013 15:27:36 -0700 > Ben Widawsky <ben@bwidawsk.net> wrote: > > > On Fri, Mar 08, 2013 at 10:45:58AM -0800, Jesse Barnes wrote: > > > From: Ben Widawsky <ben@bwidawsk.net> > > > > > > Uses slightly different interfaces than other platforms. > > > > > > v2: track actual set freq, not requested (Rohit) > > > fix debug prints in init code (Jesse) > > > > > > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> > > > > One important question is how is our punit code going to interact with > > the other potential users? It seems a bunch of power management drivers > > would also want to touch this uC. > > Pretty sure the PUnit has to deal with concurrent access... if not > we'll have to add common routines for all drivers to use and do proper > locking. I don't see how it could unless there are extra data/address/command registers for simultaneous access. Anyway, I don't think you need to change anything, just something we need to notify the other people writing code about. Also something we need to keep in mind if things mysteriously blow up at some point. > > > > + > > > + valleyview_punit_read(dev_priv, PUNIT_REG_GPU_FREQ_STS, &pval); > > > + if ((pval >> 8) != val) > > > + DRM_DEBUG_DRIVER("punit overrode freq: %d requested, but got %d\n", > > > + val, pval >> 8); > > I'm debating whether this is a useful thing to do here... You either get > > the frequency or you don't. Really it would seem more useful to me to > > check things are sane when you first enter the function (ie. did the > > last request do what you want). But I don't care what you end up with. > > It's not really a matter of sanity, it's more about what state the > platform is in. If the Punit decides things are getting too hot for > example, it may clamp your freq down. That's totally ok and normal > though, and may change in future calls. I agree, but I wasn't referring tot his part of the hunk, and you snipped the part I was referring to, so adding it back: + valleyview_punit_write(dev_priv, PUNIT_REG_GPU_FREQ_REQ, val); + + do { + valleyview_punit_read(dev_priv, PUNIT_REG_GPU_FREQ_STS, &pval); + if (time_after(jiffies, timeout)) { + DRM_DEBUG_DRIVER("timed out waiting for Punit\n"); + break; + } + udelay(10); + } while (pval & 1); This is what seems unnecessary to me. > > > I suppose if you wanted to make things a bit cleaner you could extract > > just the frequency setting part, since most of this function is > > identical to gen6 set rps. > > I was afraid more changes would creep in over time, but yeah that's a > possibility. I have a couple more changes here before I consider that. > It won't effect my adding the r-b, just that I noticed it because it would have made review a bit easier ;-) > > > + > > > + /* Make sure we continue to get interrupts > > > + * until we hit the minimum or maximum frequencies. > > > + */ > > > + I915_WRITE(GEN6_RP_INTERRUPT_LIMITS, limits); > > > + > > > + dev_priv->rps.cur_delay = pval >> 8; > > > + > > > + trace_intel_gpu_freq_change(val * 50); > > Based on our IRC discussion, I believe the value in this trace is wrong. > > Ah yeah, correct. I can just trace val here maybe, or I"ll have to put > this off until I post the real frequencies. > > > > +static void valleyview_enable_rps(struct drm_device *dev) > > > +{ > > > + struct drm_i915_private *dev_priv = dev->dev_private; > > > + struct intel_ring_buffer *ring; > > > + u32 gtfifodbg, val; > > > + int i; > > > + > > > + WARN_ON(!mutex_is_locked(&dev_priv->rps.hw_lock)); > > > > Should we check FB_GFX_TURBO_EN_FUSE here? > > I guess it wouldn't hurt. > > > > + > > > + 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); > > > + > > > + I915_WRITE(GEN6_RC_SLEEP, 0); > > You've dropped GEN6_RC1_WAKE_RATE_LIMIT. Intentional? > > Uh I should have commented that. I *think* it was intentional since > RC1 doesn't exist on VLV, but I'll have to check. I was just confused because you set GEN6_RC1e_THRESHOLD below. Is that one valid? > > > > + I915_WRITE(GEN6_RC6_WAKE_RATE_LIMIT, 0x00280000); > > > + I915_WRITE(GEN6_RC_EVALUATION_INTERVAL, 125000); > > > + I915_WRITE(GEN6_RC_IDLE_HYSTERSIS, 0x19); > > > > Don't use 0x19, use 25 since we use 25 in the gen6 path > > Ok. > > > > + > > > + for_each_ring(ring, dev_priv, i) > > > + I915_WRITE(RING_MAX_IDLE(ring->mmio_base), 10); > > > + > > > + I915_WRITE(GEN6_RC1e_THRESHOLD, 1000); > > > + I915_WRITE(GEN6_RC6_THRESHOLD, 0xc350); > > > + > > > + 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); > > > > I can't find most of the above values, but, I'll assume they're correct. > > I'd also suggest converging on either all decimal, or all hex, just to > > make things less confusing. > > A comment about units probably wouldn't hurt either. Ok. > > > > + > > > + 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 | > > > + GEN6_RP_DOWN_IDLE_CONT); > > > + > > > > > + /* allows RC6 residency counter to work */ > > > + I915_WRITE(0x138104, 0xffff00ff); > > This is writing read only registers. Should be: > > I915_WRITE(0x138104, 0x800000ff); > > > > Also, just for though, we may want to use the upper bits instead of the > > lower ones... > > I think the upper bits are a mask, and all the low 8 bits are > writeable, so maybe 0x00ff00ff? I'll have to test that. > Sent a separate email already. The value I suggested should be 0x80ff00ff I believe. > > > > > + I915_WRITE(GEN6_RC_CONTROL, GEN6_RC_CTL_HW_ENABLE); > > > + > > > > So the following stuff doesn't seem to jive in the docs I'm reading. Can > > you double check your docs, and the silicon. I tried to punt this to IRC > > but you didn't respond. These IOSF registers are all 32 bits, so I'm not > > sure how this is supposed to work. > > > > > + valleyview_punit_read(dev_priv, PUNIT_FUSE_BUS1, &val); > > > + DRM_DEBUG_DRIVER("max GPU freq: %d\n", val); > > > + dev_priv->rps.max_delay = val; > > val >> 16 && 0xff? > > > > > > > + > > > + valleyview_punit_read(dev_priv, PUNIT_REG_GPU_LFM, &val); > > > + DRM_DEBUG_DRIVER("min GPU freq: %d\n", val); > > > + dev_priv->rps.min_delay = val; > > val & 0xff? > > > + > > > + valleyview_punit_read(dev_priv, PUNIT_FUSE_BUS2, &val); > > > + DRM_DEBUG_DRIVER("max GPLL freq: %d\n", val); > > > > One doc suggests this is actually offset 0x87, bits 7:0 > > Hm I'll check these out; the different docs talk about different bit > fields, but I think they're offsets into the fuse bus array. Just double check it on silicon and it's fine with me. > > > > > > + > > > + valleyview_punit_read(dev_priv, PUNIT_REG_GPU_FREQ_STS, &val); > > > + DRM_DEBUG_DRIVER("DDR speed: "); > > > + if (drm_debug & DRM_UT_DRIVER) { > > > + if (((val >> 6) & 3) == 0) { > > > + dev_priv->mem_freq = 800; > > > + printk("800 MHz\n"); > > > + } else if (((val >> 6) & 3) == 1) { > > > + printk("1066 MHz\n"); > > > + dev_priv->mem_freq = 1066; > > > + } else if (((val >> 6) & 3) == 2) { > > > + printk("1333 MHz\n"); > > > + dev_priv->mem_freq = 1333; > > > + } else if (((val >> 6) & 3) == 3) > > > + printk("invalid\n"); > > > + } > > > > printk? > > could be one line, but would print 1332: > > dev_priv->mem_freq = 800 + (266 * (val >> 6) & 3)) > > Yeah because the DRM_DEBUG above doesn't have a newline. I like your > calculation better. > > Thanks for checking these out; I know wading through these docs is no > fun. Given you have, or will address all my concerns. Feel free to add Reviewed-by: Ben Widawsky <ben@bwidawsk.net> with the fixed patch. > > -- > Jesse Barnes, Intel Open Source Technology Center
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 592e944..34414d1 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1820,6 +1820,7 @@ extern void intel_disable_fbc(struct drm_device *dev); extern bool ironlake_set_drps(struct drm_device *dev, u8 val); extern void intel_init_pch_refclk(struct drm_device *dev); extern void gen6_set_rps(struct drm_device *dev, u8 val); +extern void valleyview_set_rps(struct drm_device *dev, u8 val); extern void intel_detect_pch(struct drm_device *dev); extern int intel_trans_dp_port_sel(struct drm_crtc *crtc); extern int intel_enable_rc6(const struct drm_device *dev); diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c index 2139714..65120e1 100644 --- a/drivers/gpu/drm/i915/i915_irq.c +++ b/drivers/gpu/drm/i915/i915_irq.c @@ -395,7 +395,10 @@ static void gen6_pm_rps_work(struct work_struct *work) */ if (!(new_delay > dev_priv->rps.max_delay || new_delay < dev_priv->rps.min_delay)) { - gen6_set_rps(dev_priv->dev, new_delay); + if (IS_VALLEYVIEW(dev_priv->dev)) + valleyview_set_rps(dev_priv->dev, new_delay); + else + gen6_set_rps(dev_priv->dev, new_delay); } mutex_unlock(&dev_priv->rps.hw_lock); diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 70eab45..d0b8d58 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -2477,6 +2477,47 @@ void gen6_set_rps(struct drm_device *dev, u8 val) trace_intel_gpu_freq_change(val * 50); } +void valleyview_set_rps(struct drm_device *dev, u8 val) +{ + struct drm_i915_private *dev_priv = dev->dev_private; + unsigned long timeout = jiffies + msecs_to_jiffies(100); + u32 limits = gen6_rps_limits(dev_priv, &val); + u32 pval; + + WARN_ON(!mutex_is_locked(&dev_priv->rps.hw_lock)); + WARN_ON(val > dev_priv->rps.max_delay); + WARN_ON(val < dev_priv->rps.min_delay); + + if (val == dev_priv->rps.cur_delay) + return; + + valleyview_punit_write(dev_priv, PUNIT_REG_GPU_FREQ_REQ, val); + + do { + valleyview_punit_read(dev_priv, PUNIT_REG_GPU_FREQ_STS, &pval); + if (time_after(jiffies, timeout)) { + DRM_DEBUG_DRIVER("timed out waiting for Punit\n"); + break; + } + udelay(10); + } while (pval & 1); + + valleyview_punit_read(dev_priv, PUNIT_REG_GPU_FREQ_STS, &pval); + if ((pval >> 8) != val) + DRM_DEBUG_DRIVER("punit overrode freq: %d requested, but got %d\n", + val, pval >> 8); + + /* Make sure we continue to get interrupts + * until we hit the minimum or maximum frequencies. + */ + I915_WRITE(GEN6_RP_INTERRUPT_LIMITS, limits); + + dev_priv->rps.cur_delay = pval >> 8; + + trace_intel_gpu_freq_change(val * 50); +} + + static void gen6_disable_rps(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev->dev_private; @@ -2714,6 +2755,102 @@ static void gen6_update_ring_freq(struct drm_device *dev) } } +static void valleyview_enable_rps(struct drm_device *dev) +{ + struct drm_i915_private *dev_priv = dev->dev_private; + struct intel_ring_buffer *ring; + u32 gtfifodbg, val; + int i; + + WARN_ON(!mutex_is_locked(&dev_priv->rps.hw_lock)); + + 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); + + I915_WRITE(GEN6_RC_SLEEP, 0); + + I915_WRITE(GEN6_RC6_WAKE_RATE_LIMIT, 0x00280000); + I915_WRITE(GEN6_RC_EVALUATION_INTERVAL, 125000); + I915_WRITE(GEN6_RC_IDLE_HYSTERSIS, 0x19); + + for_each_ring(ring, dev_priv, i) + I915_WRITE(RING_MAX_IDLE(ring->mmio_base), 10); + + I915_WRITE(GEN6_RC1e_THRESHOLD, 1000); + I915_WRITE(GEN6_RC6_THRESHOLD, 0xc350); + + 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 | + GEN6_RP_DOWN_IDLE_CONT); + + /* allows RC6 residency counter to work */ + I915_WRITE(0x138104, 0xffff00ff); + I915_WRITE(GEN6_RC_CONTROL, GEN6_RC_CTL_HW_ENABLE); + + valleyview_punit_read(dev_priv, PUNIT_FUSE_BUS1, &val); + DRM_DEBUG_DRIVER("max GPU freq: %d\n", val); + dev_priv->rps.max_delay = val; + + valleyview_punit_read(dev_priv, PUNIT_REG_GPU_LFM, &val); + DRM_DEBUG_DRIVER("min GPU freq: %d\n", val); + dev_priv->rps.min_delay = val; + + valleyview_punit_read(dev_priv, PUNIT_FUSE_BUS2, &val); + DRM_DEBUG_DRIVER("max GPLL freq: %d\n", val); + + valleyview_punit_read(dev_priv, PUNIT_REG_GPU_FREQ_STS, &val); + DRM_DEBUG_DRIVER("DDR speed: "); + if (drm_debug & DRM_UT_DRIVER) { + if (((val >> 6) & 3) == 0) { + dev_priv->mem_freq = 800; + printk("800 MHz\n"); + } else if (((val >> 6) & 3) == 1) { + printk("1066 MHz\n"); + dev_priv->mem_freq = 1066; + } else if (((val >> 6) & 3) == 2) { + printk("1333 MHz\n"); + dev_priv->mem_freq = 1333; + } else if (((val >> 6) & 3) == 3) + printk("invalid\n"); + } + DRM_DEBUG_DRIVER("GPLL enabled? %s\n", val & 8 ? "yes" : "no"); + DRM_DEBUG_DRIVER("GPU status: 0x%08x\n", val); + + DRM_DEBUG_DRIVER("current GPU freq: %x\n", (val >> 8) & 0xff); + dev_priv->rps.cur_delay = (val >> 8) & 0xff; + + val = 0xd500; + DRM_DEBUG_DRIVER("setting GPU freq to %d\n", (val >> 8) & 0xff); + + valleyview_set_rps(dev_priv->dev, (val >> 8) & 0xff); + + /* 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); +} + void ironlake_teardown_rc6(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev->dev_private; @@ -3440,7 +3577,7 @@ void intel_disable_gt_powersave(struct drm_device *dev) if (IS_IRONLAKE_M(dev)) { ironlake_disable_drps(dev); ironlake_disable_rc6(dev); - } else if (INTEL_INFO(dev)->gen >= 6 && !IS_VALLEYVIEW(dev)) { + } else if (INTEL_INFO(dev)->gen >= 6) { cancel_delayed_work_sync(&dev_priv->rps.delayed_resume_work); mutex_lock(&dev_priv->rps.hw_lock); gen6_disable_rps(dev); @@ -3456,8 +3593,13 @@ 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); - gen6_update_ring_freq(dev); + + if (IS_VALLEYVIEW(dev)) { + valleyview_enable_rps(dev); + } else { + gen6_enable_rps(dev); + gen6_update_ring_freq(dev); + } mutex_unlock(&dev_priv->rps.hw_lock); } @@ -3469,7 +3611,7 @@ void intel_enable_gt_powersave(struct drm_device *dev) ironlake_enable_drps(dev); ironlake_enable_rc6(dev); intel_init_emon(dev); - } else if ((IS_GEN6(dev) || IS_GEN7(dev)) && !IS_VALLEYVIEW(dev)) { + } else if (IS_GEN6(dev) || IS_GEN7(dev)) { /* * PCU communication is slow and this doesn't need to be * done at any specific time, so do this out of our fast path