| Message ID | s5htxsqiyt5.wl%tiwai@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Nov 15, 2012 at 6:03 PM, Takashi Iwai <tiwai@suse.de> wrote: > Some IVB machines seem to need non-MT forcewake. Using MT forcewake > on such machines result in broken or blank screens. > > Signed-off-by: Takashi Iwai <tiwai@suse.de> > --- > The commit 36ec8f877481449bdfa072e6adf2060869e2b970 triggered a > regression on my test IVY laptops. Instead of reverting the commit, > I cooked a bit to make fitting only for IS_IVYBRIDGE(). Hm, that's rather bad since proper usage of DERRMR requires that we enable rc6 on ivb, which is only really possible when we have multi-threaded forcewake. Now I've thought that all production machines have that enabled (it's something the bios should do), so the big question: Is this a pre-production hw? If so, a bios upgrade should fix it since historically we've killed pre-production workarounds once the real hw ships, there are simply too many ... -Daniel > > drivers/gpu/drm/i915/intel_pm.c | 29 ++++++++++++++++++++++++++++- > 1 file changed, 28 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > index 0cbc0e6..944911e 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -4239,9 +4239,36 @@ void intel_gt_init(struct drm_device *dev) > if (IS_VALLEYVIEW(dev)) { > dev_priv->gt.force_wake_get = vlv_force_wake_get; > dev_priv->gt.force_wake_put = vlv_force_wake_put; > - } else if (IS_IVYBRIDGE(dev) || IS_HASWELL(dev)) { > + } else if (IS_HASWELL(dev)) { > dev_priv->gt.force_wake_get = __gen6_gt_force_wake_mt_get; > dev_priv->gt.force_wake_put = __gen6_gt_force_wake_mt_put; > + } else if (IS_IVYBRIDGE(dev)) { > + /* IVB configs may use multi-threaded forcewake */ > + u32 ecobus; > + > + /* A small trick here - if the bios hasn't configured > + * MT forcewake, and if the device is in RC6, then > + * force_wake_mt_get will not wake the device and the > + * ECOBUS read will return zero. Which will be > + * (correctly) interpreted by the test below as MT > + * forcewake being disabled. > + */ > + mutex_lock(&dev->struct_mutex); > + __gen6_gt_force_wake_mt_get(dev_priv); > + ecobus = I915_READ_NOTRACE(ECOBUS); > + __gen6_gt_force_wake_mt_put(dev_priv); > + mutex_unlock(&dev->struct_mutex); > + > + if (ecobus & FORCEWAKE_MT_ENABLE) { > + DRM_DEBUG_KMS("Using MT version of forcewake\n"); > + dev_priv->gt.force_wake_get = > + __gen6_gt_force_wake_mt_get; > + dev_priv->gt.force_wake_put = > + __gen6_gt_force_wake_mt_put; > + } else { > + dev_priv->gt.force_wake_get = __gen6_gt_force_wake_get; > + dev_priv->gt.force_wake_put = __gen6_gt_force_wake_put; > + } > } else if (IS_GEN6(dev)) { > dev_priv->gt.force_wake_get = __gen6_gt_force_wake_get; > dev_priv->gt.force_wake_put = __gen6_gt_force_wake_put; > -- > 1.8.0 >
At Thu, 15 Nov 2012 21:07:33 +0100, Daniel Vetter wrote: > > On Thu, Nov 15, 2012 at 6:03 PM, Takashi Iwai <tiwai@suse.de> wrote: > > Some IVB machines seem to need non-MT forcewake. Using MT forcewake > > on such machines result in broken or blank screens. > > > > Signed-off-by: Takashi Iwai <tiwai@suse.de> > > --- > > The commit 36ec8f877481449bdfa072e6adf2060869e2b970 triggered a > > regression on my test IVY laptops. Instead of reverting the commit, > > I cooked a bit to make fitting only for IS_IVYBRIDGE(). > > Hm, that's rather bad since proper usage of DERRMR requires that we > enable rc6 on ivb, which is only really possible when we have > multi-threaded forcewake. Hm, so just disabling RC6 doesn't suffice? > Now I've thought that all production > machines have that enabled (it's something the bios should do), so the > big question: Is this a pre-production hw? If so, a bios upgrade > should fix it since historically we've killed pre-production > workarounds once the real hw ships, there are simply too many ... Indeed a machine showing the issue is a prototype machine, but it's a product-ready unit, so I don't think CPU/GPU is different from the same model in the market. Though, the BIOS version on that machine is fairly old, so of course this can be the cause. But still I feel uneasy that the bug depends on BIOS version, not the hardware. In anyway, I'll check other IVB models whether this problem is present. thanks, Takashi > -Daniel > > > > > > drivers/gpu/drm/i915/intel_pm.c | 29 ++++++++++++++++++++++++++++- > > 1 file changed, 28 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > > index 0cbc0e6..944911e 100644 > > --- a/drivers/gpu/drm/i915/intel_pm.c > > +++ b/drivers/gpu/drm/i915/intel_pm.c > > @@ -4239,9 +4239,36 @@ void intel_gt_init(struct drm_device *dev) > > if (IS_VALLEYVIEW(dev)) { > > dev_priv->gt.force_wake_get = vlv_force_wake_get; > > dev_priv->gt.force_wake_put = vlv_force_wake_put; > > - } else if (IS_IVYBRIDGE(dev) || IS_HASWELL(dev)) { > > + } else if (IS_HASWELL(dev)) { > > dev_priv->gt.force_wake_get = __gen6_gt_force_wake_mt_get; > > dev_priv->gt.force_wake_put = __gen6_gt_force_wake_mt_put; > > + } else if (IS_IVYBRIDGE(dev)) { > > + /* IVB configs may use multi-threaded forcewake */ > > + u32 ecobus; > > + > > + /* A small trick here - if the bios hasn't configured > > + * MT forcewake, and if the device is in RC6, then > > + * force_wake_mt_get will not wake the device and the > > + * ECOBUS read will return zero. Which will be > > + * (correctly) interpreted by the test below as MT > > + * forcewake being disabled. > > + */ > > + mutex_lock(&dev->struct_mutex); > > + __gen6_gt_force_wake_mt_get(dev_priv); > > + ecobus = I915_READ_NOTRACE(ECOBUS); > > + __gen6_gt_force_wake_mt_put(dev_priv); > > + mutex_unlock(&dev->struct_mutex); > > + > > + if (ecobus & FORCEWAKE_MT_ENABLE) { > > + DRM_DEBUG_KMS("Using MT version of forcewake\n"); > > + dev_priv->gt.force_wake_get = > > + __gen6_gt_force_wake_mt_get; > > + dev_priv->gt.force_wake_put = > > + __gen6_gt_force_wake_mt_put; > > + } else { > > + dev_priv->gt.force_wake_get = __gen6_gt_force_wake_get; > > + dev_priv->gt.force_wake_put = __gen6_gt_force_wake_put; > > + } > > } else if (IS_GEN6(dev)) { > > dev_priv->gt.force_wake_get = __gen6_gt_force_wake_get; > > dev_priv->gt.force_wake_put = __gen6_gt_force_wake_put; > > -- > > 1.8.0 > > > > > > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch >
At Thu, 15 Nov 2012 21:22:41 +0100, Takashi Iwai wrote: > > At Thu, 15 Nov 2012 21:07:33 +0100, > Daniel Vetter wrote: > > > > On Thu, Nov 15, 2012 at 6:03 PM, Takashi Iwai <tiwai@suse.de> wrote: > > > Some IVB machines seem to need non-MT forcewake. Using MT forcewake > > > on such machines result in broken or blank screens. > > > > > > Signed-off-by: Takashi Iwai <tiwai@suse.de> > > > --- > > > The commit 36ec8f877481449bdfa072e6adf2060869e2b970 triggered a > > > regression on my test IVY laptops. Instead of reverting the commit, > > > I cooked a bit to make fitting only for IS_IVYBRIDGE(). > > > > Hm, that's rather bad since proper usage of DERRMR requires that we > > enable rc6 on ivb, which is only really possible when we have > > multi-threaded forcewake. > > Hm, so just disabling RC6 doesn't suffice? > > > Now I've thought that all production > > machines have that enabled (it's something the bios should do), so the > > big question: Is this a pre-production hw? If so, a bios upgrade > > should fix it since historically we've killed pre-production > > workarounds once the real hw ships, there are simply too many ... > > Indeed a machine showing the issue is a prototype machine, but it's a > product-ready unit, so I don't think CPU/GPU is different from the > same model in the market. It seems that I've looked at a lspci output on a wrong machine while working remotely. The machines showing the problem have CPU revisions less than 8. So, you are right, it must be specific to a pre-production CPU. Assuming it being not in the market, I can live without that. Time to throw away a crap :) Sorry for the noise. Takashi
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 0cbc0e6..944911e 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -4239,9 +4239,36 @@ void intel_gt_init(struct drm_device *dev) if (IS_VALLEYVIEW(dev)) { dev_priv->gt.force_wake_get = vlv_force_wake_get; dev_priv->gt.force_wake_put = vlv_force_wake_put; - } else if (IS_IVYBRIDGE(dev) || IS_HASWELL(dev)) { + } else if (IS_HASWELL(dev)) { dev_priv->gt.force_wake_get = __gen6_gt_force_wake_mt_get; dev_priv->gt.force_wake_put = __gen6_gt_force_wake_mt_put; + } else if (IS_IVYBRIDGE(dev)) { + /* IVB configs may use multi-threaded forcewake */ + u32 ecobus; + + /* A small trick here - if the bios hasn't configured + * MT forcewake, and if the device is in RC6, then + * force_wake_mt_get will not wake the device and the + * ECOBUS read will return zero. Which will be + * (correctly) interpreted by the test below as MT + * forcewake being disabled. + */ + mutex_lock(&dev->struct_mutex); + __gen6_gt_force_wake_mt_get(dev_priv); + ecobus = I915_READ_NOTRACE(ECOBUS); + __gen6_gt_force_wake_mt_put(dev_priv); + mutex_unlock(&dev->struct_mutex); + + if (ecobus & FORCEWAKE_MT_ENABLE) { + DRM_DEBUG_KMS("Using MT version of forcewake\n"); + dev_priv->gt.force_wake_get = + __gen6_gt_force_wake_mt_get; + dev_priv->gt.force_wake_put = + __gen6_gt_force_wake_mt_put; + } else { + dev_priv->gt.force_wake_get = __gen6_gt_force_wake_get; + dev_priv->gt.force_wake_put = __gen6_gt_force_wake_put; + } } else if (IS_GEN6(dev)) { dev_priv->gt.force_wake_get = __gen6_gt_force_wake_get; dev_priv->gt.force_wake_put = __gen6_gt_force_wake_put;
Some IVB machines seem to need non-MT forcewake. Using MT forcewake on such machines result in broken or blank screens. Signed-off-by: Takashi Iwai <tiwai@suse.de> --- The commit 36ec8f877481449bdfa072e6adf2060869e2b970 triggered a regression on my test IVY laptops. Instead of reverting the commit, I cooked a bit to make fitting only for IS_IVYBRIDGE(). drivers/gpu/drm/i915/intel_pm.c | 29 ++++++++++++++++++++++++++++- 1 file changed, 28 insertions(+), 1 deletion(-)