| Message ID | 1348680842-3036-2-git-send-email-ben@bwidawsk.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Sep 26, 2012 at 10:34 AM, Ben Widawsky <ben@bwidawsk.net> wrote: > BIOS should be setting the minimum voltage for rc6 to be 450mV. Old or > buggy BIOSen may not be doing this, so we correct it for them. Ideally > customers should update the BIOS as only it would know the optimal > values for the platform, so we leave that fact as a DRM_ERROR for the > user to see. > > Unfortunately this isn't fixing any of the issues it was targeted to > fix, but it is documented that we must do it. > > CC: Jesse Barnes <jbarnes@virtuousgeek.org> > CC: Matt Turner <mattst88@gmail.com> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net> > --- > drivers/gpu/drm/i915/i915_reg.h | 4 ++++ > drivers/gpu/drm/i915/intel_pm.c | 18 ++++++++++++++++-- > 2 files changed, 20 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index a828e90..13aafa5 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -4213,6 +4213,10 @@ > #define GEN6_READ_OC_PARAMS 0xc > #define GEN6_PCODE_WRITE_MIN_FREQ_TABLE 0x8 > #define GEN6_PCODE_READ_MIN_FREQ_TABLE 0x9 > +#define GEN6_PCODE_WRITE_RC6VIDS 0x4 > +#define GEN6_PCODE_READ_RC6VIDS 0x5 > +#define GEN6_ENCODE_RC6_VID(mv) (((mv) / 5) - 245) < 0 ?: 0 > +#define GEN6_DECODE_RC6_VID(vids) (((vids) * 5) > 0 ? ((vids) * 5) + 245 : 0) > #define GEN6_PCODE_DATA 0x138128 > #define GEN6_PCODE_FREQ_IA_RATIO_SHIFT 8 > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > index 6488cd0..9a6edf0 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -2404,10 +2404,10 @@ static void gen6_enable_rps(struct drm_device *dev) > struct intel_ring_buffer *ring; > u32 rp_state_cap; > u32 gt_perf_status; > - u32 pcu_mbox, rc6_mask = 0; > + u32 rc6vids, pcu_mbox, rc6_mask = 0; > u32 gtfifodbg; > int rc6_mode; > - int i; > + int i, ret; > > WARN_ON(!mutex_is_locked(&dev->struct_mutex)); > > @@ -2526,6 +2526,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_ERROR("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); > } > > -- > 1.7.12.1 > Tested-by: Matt Turner <mattst88@gmail.com> (feel free to apply that to the whole series) Too bad it's not this bug that affects my system. :(
On Wed, 26 Sep 2012 10:34:01 -0700 Ben Widawsky <ben@bwidawsk.net> wrote: > BIOS should be setting the minimum voltage for rc6 to be 450mV. Old or > buggy BIOSen may not be doing this, so we correct it for them. Ideally > customers should update the BIOS as only it would know the optimal > values for the platform, so we leave that fact as a DRM_ERROR for the > user to see. > > Unfortunately this isn't fixing any of the issues it was targeted to > fix, but it is documented that we must do it. > > CC: Jesse Barnes <jbarnes@virtuousgeek.org> > CC: Matt Turner <mattst88@gmail.com> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net> > --- > drivers/gpu/drm/i915/i915_reg.h | 4 ++++ > drivers/gpu/drm/i915/intel_pm.c | 18 ++++++++++++++++-- > 2 files changed, 20 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index a828e90..13aafa5 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -4213,6 +4213,10 @@ > #define GEN6_READ_OC_PARAMS 0xc > #define GEN6_PCODE_WRITE_MIN_FREQ_TABLE 0x8 > #define GEN6_PCODE_READ_MIN_FREQ_TABLE 0x9 > +#define GEN6_PCODE_WRITE_RC6VIDS 0x4 > +#define GEN6_PCODE_READ_RC6VIDS 0x5 > +#define GEN6_ENCODE_RC6_VID(mv) (((mv) / 5) - 245) < 0 ?: 0 > +#define GEN6_DECODE_RC6_VID(vids) (((vids) * 5) > 0 ? ((vids) * 5) + 245 : 0) > #define GEN6_PCODE_DATA 0x138128 > #define GEN6_PCODE_FREQ_IA_RATIO_SHIFT 8 > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > index 6488cd0..9a6edf0 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -2404,10 +2404,10 @@ static void gen6_enable_rps(struct drm_device *dev) > struct intel_ring_buffer *ring; > u32 rp_state_cap; > u32 gt_perf_status; > - u32 pcu_mbox, rc6_mask = 0; > + u32 rc6vids, pcu_mbox, rc6_mask = 0; > u32 gtfifodbg; > int rc6_mode; > - int i; > + int i, ret; > > WARN_ON(!mutex_is_locked(&dev->struct_mutex)); > > @@ -2526,6 +2526,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_ERROR("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); > } > Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
On Fri, Sep 28, 2012 at 4:05 AM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote: > On Wed, 26 Sep 2012 10:34:01 -0700 > Ben Widawsky <ben@bwidawsk.net> wrote: > >> BIOS should be setting the minimum voltage for rc6 to be 450mV. Old or >> buggy BIOSen may not be doing this, so we correct it for them. Ideally >> customers should update the BIOS as only it would know the optimal >> values for the platform, so we leave that fact as a DRM_ERROR for the >> user to see. >> >> Unfortunately this isn't fixing any of the issues it was targeted to >> fix, but it is documented that we must do it. Yeah drop the DRM_ERROR, it should be DRM_INFO at worst, and non-existant at best. Who gives a shit if you have to upgrade your BIOS, lots of vendors won't provide updates and I don't see Intel doing anything to solve that. Dave.
On Fri, 28 Sep 2012 10:13:26 +1000 Dave Airlie <airlied@gmail.com> wrote: > On Fri, Sep 28, 2012 at 4:05 AM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote: > > On Wed, 26 Sep 2012 10:34:01 -0700 > > Ben Widawsky <ben@bwidawsk.net> wrote: > > > >> BIOS should be setting the minimum voltage for rc6 to be 450mV. Old or > >> buggy BIOSen may not be doing this, so we correct it for them. Ideally > >> customers should update the BIOS as only it would know the optimal > >> values for the platform, so we leave that fact as a DRM_ERROR for the > >> user to see. > >> > >> Unfortunately this isn't fixing any of the issues it was targeted to > >> fix, but it is documented that we must do it. > > Yeah drop the DRM_ERROR, it should be DRM_INFO at worst, and > non-existant at best. > > Who gives a shit if you have to upgrade your BIOS, lots of vendors > won't provide updates and I don't see Intel doing anything to solve > that. > > Dave. I dunno, messing with voltages seems pretty scary to me. I'd really like a loud warning when the driver does it. I perhaps agree that telling the user to update the BIOS isn't a message we want to classify as an ERROR. I think at least a DRM_DEBUG_DRIVER, but I give Daniel permission to change it as he sees fit assuming the rest of the patch is fine for everyone.
On Sat, Sep 29, 2012 at 01:07:35PM -0700, Ben Widawsky wrote: > On Fri, 28 Sep 2012 10:13:26 +1000 > Dave Airlie <airlied@gmail.com> wrote: > > > On Fri, Sep 28, 2012 at 4:05 AM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote: > > > On Wed, 26 Sep 2012 10:34:01 -0700 > > > Ben Widawsky <ben@bwidawsk.net> wrote: > > > > > >> BIOS should be setting the minimum voltage for rc6 to be 450mV. Old or > > >> buggy BIOSen may not be doing this, so we correct it for them. Ideally > > >> customers should update the BIOS as only it would know the optimal > > >> values for the platform, so we leave that fact as a DRM_ERROR for the > > >> user to see. > > >> > > >> Unfortunately this isn't fixing any of the issues it was targeted to > > >> fix, but it is documented that we must do it. > > > > Yeah drop the DRM_ERROR, it should be DRM_INFO at worst, and > > non-existant at best. > > > > Who gives a shit if you have to upgrade your BIOS, lots of vendors > > won't provide updates and I don't see Intel doing anything to solve > > that. > > > > Dave. > > > I dunno, messing with voltages seems pretty scary to me. I'd really like > a loud warning when the driver does it. I perhaps agree that telling the > user to update the BIOS isn't a message we want to classify as an ERROR. > I think at least a DRM_DEBUG_DRIVER, but I give Daniel permission to > change it as he sees fit assuming the rest of the patch is fine for > everyone. Merged these 2 patches, too, with the error tuned down to a debug message. Thanks, Daniel
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index a828e90..13aafa5 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -4213,6 +4213,10 @@ #define GEN6_READ_OC_PARAMS 0xc #define GEN6_PCODE_WRITE_MIN_FREQ_TABLE 0x8 #define GEN6_PCODE_READ_MIN_FREQ_TABLE 0x9 +#define GEN6_PCODE_WRITE_RC6VIDS 0x4 +#define GEN6_PCODE_READ_RC6VIDS 0x5 +#define GEN6_ENCODE_RC6_VID(mv) (((mv) / 5) - 245) < 0 ?: 0 +#define GEN6_DECODE_RC6_VID(vids) (((vids) * 5) > 0 ? ((vids) * 5) + 245 : 0) #define GEN6_PCODE_DATA 0x138128 #define GEN6_PCODE_FREQ_IA_RATIO_SHIFT 8 diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 6488cd0..9a6edf0 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -2404,10 +2404,10 @@ static void gen6_enable_rps(struct drm_device *dev) struct intel_ring_buffer *ring; u32 rp_state_cap; u32 gt_perf_status; - u32 pcu_mbox, rc6_mask = 0; + u32 rc6vids, pcu_mbox, rc6_mask = 0; u32 gtfifodbg; int rc6_mode; - int i; + int i, ret; WARN_ON(!mutex_is_locked(&dev->struct_mutex)); @@ -2526,6 +2526,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_ERROR("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); }
BIOS should be setting the minimum voltage for rc6 to be 450mV. Old or buggy BIOSen may not be doing this, so we correct it for them. Ideally customers should update the BIOS as only it would know the optimal values for the platform, so we leave that fact as a DRM_ERROR for the user to see. Unfortunately this isn't fixing any of the issues it was targeted to fix, but it is documented that we must do it. CC: Jesse Barnes <jbarnes@virtuousgeek.org> CC: Matt Turner <mattst88@gmail.com> Signed-off-by: Ben Widawsky <ben@bwidawsk.net> --- drivers/gpu/drm/i915/i915_reg.h | 4 ++++ drivers/gpu/drm/i915/intel_pm.c | 18 ++++++++++++++++-- 2 files changed, 20 insertions(+), 2 deletions(-)