On Fri, Jun 13, 2014 at 05:56:41PM +0530, Deepak S wrote:
>
> On Friday 13 June 2014 05:27 PM, Ville Syrjälä wrote:
> >On Fri, Jun 13, 2014 at 02:33:44PM +0300, Ville Syrjälä wrote:
> >>On Fri, Jun 13, 2014 at 03:46:14PM +0530, deepak.s@xxxxxxxxxxxxxxx wrote:
> >>>From: Deepak S <deepak.s@xxxxxxxxxxxxxxx>
> >>>
> >>>Workaround fixed in BYT. Forcing Gfx clk up not needed, and Requesting the
> >>>min freq should bring bring the voltage Vnn.
> >>>
> >>>Signed-off-by: Deepak S <deepak.s@xxxxxxxxxxxxxxx>
> >>>---
> >>> drivers/gpu/drm/i915/intel_pm.c | 40 +---------------------------------------
> >>> 1 file changed, 1 insertion(+), 39 deletions(-)
> >>>
> >>>diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> >>>index 0b088fe..9aee28b 100644
> >>>--- a/drivers/gpu/drm/i915/intel_pm.c
> >>>+++ b/drivers/gpu/drm/i915/intel_pm.c
> >>>@@ -3198,44 +3198,6 @@ void gen6_set_rps(struct drm_device *dev, u8 val)
> >>> trace_intel_gpu_freq_change(val * 50);
> >>> }
> >>>-/* vlv_set_rps_idle: Set the frequency to Rpn if Gfx clocks are down
> >>>- *
> >>>- * * If Gfx is Idle, then
> >>>- * 1. Mask Turbo interrupts
> >>>- * 2. Bring up Gfx clock
> >>>- * 3. Change the freq to Rpn and wait till P-Unit updates freq
> >>>- * 4. Clear the Force GFX CLK ON bit so that Gfx can down
> >>>- * 5. Unmask Turbo interrupts
> >>>-*/
> >>>-static void vlv_set_rps_idle(struct drm_i915_private *dev_priv)
> >>>-{
> >>>- /*
> >>>- * When we are idle. Drop to min voltage state.
> >>>- */
> >>>-
> >>>- if (dev_priv->rps.cur_freq <= dev_priv->rps.min_freq_softlimit)
> >>>- return;
> >>>-
> >>>- /* Mask turbo interrupt so that they will not come in between */
> >>>- I915_WRITE(GEN6_PMINTRMSK, 0xffffffff);
> >>>-
> >>>- vlv_force_gfx_clock(dev_priv, true);
> >>>-
> >>>- dev_priv->rps.cur_freq = dev_priv->rps.min_freq_softlimit;
> >>>-
> >>>- vlv_punit_write(dev_priv, PUNIT_REG_GPU_FREQ_REQ,
> >>>- dev_priv->rps.min_freq_softlimit);
> >>>-
> >>>- if (wait_for(((vlv_punit_read(dev_priv, PUNIT_REG_GPU_FREQ_STS))
> >>>- & GENFREQSTATUS) == 0, 5))
> >>>- DRM_ERROR("timed out waiting for Punit\n");
> >>>-
> >>>- vlv_force_gfx_clock(dev_priv, false);
> >>>-
> >>>- I915_WRITE(GEN6_PMINTRMSK,
> >>>- gen6_rps_pm_mask(dev_priv, dev_priv->rps.cur_freq));
> >>>-}
> >>>-
> >>> void gen6_rps_idle(struct drm_i915_private *dev_priv)
> >>> {
> >>> struct drm_device *dev = dev_priv->dev;
> >>>@@ -3243,7 +3205,7 @@ void gen6_rps_idle(struct drm_i915_private *dev_priv)
> >>> mutex_lock(&dev_priv->rps.hw_lock);
> >>> if (dev_priv->rps.enabled) {
> >>> if (IS_VALLEYVIEW(dev))
> >>>- vlv_set_rps_idle(dev_priv);
> >>>+ valleyview_set_rps(dev_priv->dev, dev_priv->rps.min_freq_softlimit);
> >>This should take care of https://bugs.freedesktop.org/show_bug.cgi?id=75244
> >>
> >>I don't know when the hardware got fixed so I'm hesitant to r-b it, but
> >>at least my C0 works fine without this stuff, so:
> >>Acked-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> >>
> >>However to avoid future mishaps I think we should have some kind of a
> >>comment before the valleyview_set_rps() call to let the reader know that
> >>we really need this on VLV to drop the voltage.
>
> hmm, Yes we might need this for other stepping. I will add a comment
Please don't put the stepping info in the comment or commit message
though, that freaks out people ;-) Usually we go with "pre-production" or
"early revisions" or something non-specific.
>
> Thanks for the review
>
> >>Also it now occurs to me that we might be leaving the GPU frequency (and
> >>thus Vnn) high during a system suspend. I think we need an explicit
> >>rps_idle() call in the suspend path somewhere. Runtime suspend should be
> >>fine already since it depends on intel_mark_idle() getting called before
> >>the last rpm reference is dropped.
> >Maybe this is all we need?
> >
> >diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> >index 3768199..fabd852 100644
> >--- a/drivers/gpu/drm/i915/i915_gem.c
> >+++ b/drivers/gpu/drm/i915/i915_gem.c
> >@@ -4530,7 +4530,7 @@ i915_gem_suspend(struct drm_device *dev)
> > del_timer_sync(&dev_priv->gpu_error.hangcheck_timer);
> > cancel_delayed_work_sync(&dev_priv->mm.retire_work);
> >- cancel_delayed_work_sync(&dev_priv->mm.idle_work);
> >+ flush_delayed_work(&dev_priv->mm.idle_work);
> > return 0;
>
> Yes, while suspending we need move GPU to min_freq. flush_delayed_work
> should be fine. Let me create a patch for this.
Since this is gt powersave related can you please also check whether we
shouldn't move this to the intel_suspend_gt_powersave function Jesse
recently added to -nightly?
Thanks, Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
