<div dir="ltr">Yeah, this kind of overlaps ben's patch, but also remove unecessary sandybridge_pcode_write.</div><div class="gmail_extra"> <div class="gmail_quote">On Tue, Mar 26, 2013 at 5:02 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 class="HOEnZb"><div class="h5">On Mon, Mar 25, 2013 at 05:55:51PM -0300, Rodrigo Vivi wrote: > Yet according to pm spec pcode read/write operations aren't necessary for HSW. > > CC: Paulo Zanoni <<a href="mailto:paulo.r.zanoni@intel.com">paulo.r.zanoni@intel.com</a>> > Signed-off-by: Rodrigo Vivi <<a href="mailto:rodrigo.vivi@gmail.com">rodrigo.vivi@gmail.com</a>> > --- > drivers/gpu/drm/i915/intel_pm.c | 50 ++++++++++++++++++++++------------------- > 1 file changed, 27 insertions(+), 23 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > index aea5fac..f6a7366 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -2645,18 +2645,20 @@ static void gen6_enable_rps(struct drm_device *dev) > GEN6_RP_UP_BUSY_AVG | > (IS_HASWELL(dev) ? GEN7_RP_DOWN_IDLE_AVG : 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; > + 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"); > } > - } else { > - DRM_DEBUG_DRIVER("Failed to set the min frequency\n"); </div></div>Note that the turbo code only runs on snb/ivb - Ben has promised to supply the hsw turbo support. So imo adding this check isn't required really. <div><div class="h5"> > } > > gen6_set_rps(dev_priv->dev, (gt_perf_status & 0xff00) >> 8); > @@ -2670,18 +2672,20 @@ static void gen6_enable_rps(struct drm_device *dev) > /* enable all PM interrupts */ > I915_WRITE(GEN6_PMINTRMSK, 0); > > - 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"); > + 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"); > + } </div></div>This is a w/a only for snb - it would be much clearer to slap an if (IS_GEN6) check around it (and remove the funny logic with the existing GEN6 checks). Maybe resend just that? -Daniel > } > > gen6_gt_force_wake_put(dev_priv); > -- > 1.8.1.4 > > _______________________________________________ > Intel-gfx mailing list > <a href="mailto:Intel-gfx@lists.freedesktop.org">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">+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>Rodrigo Vivi</div><div>Blog: <a href="http://blog.vivi.eng.br" target="_blank">http://blog.vivi.eng.br</a></div><div> </div> </div>