Quoting Ville Syrjälä (2018-11-13 18:40:20) > On Tue, Nov 13, 2018 at 06:25:47PM +0000, Chris Wilson wrote: > > Quoting Ville Syrjala (2018-11-13 18:10:23) > > > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> > > > > > > I have a Thinkpad X220 Tablet in my hands that is losing vblank > > > interrupts whenever LP3 watermarks are used. > > > > > > If I nudge the latency value written to the WM3 register just > > > by one in either direction the problem disappears. That to me > > > suggests that the punit will not enter the corrsponding > > > powersave mode (MPLL shutdown IIRC) unless the latency value > > > in the register matches exactly what we read from SSKPD. Ie. > > > it's not really a latency value but rather just a cookie > > > by which the punit can identify the desired power saving state. > > > On HSW/BDW this was changed such that we actually just write > > > the WM level number into those bits, which makes much more > > > sense given the observed behaviour. > > > > > > We could try to handle this by disallowing LP3 watermarks > > > only when vblank interrupts are enabled but we'd first have > > > to prove that only vblank interrupts are affected, which > > > seems unlikely. Also we can't grab the wm mutex from the > > > vblank enable/disable hooks because those are called with > > > various spinlocks held. Thus we'd have to redesigne the > > > watermark locking. So to play it safe and keep the code > > > simple we simply disable LP3 watermarks on all SNB machines. > > > > > > To do that we simply zero out the latency values for > > > watermark level 3, and we adjust the watermark computation > > > to check for that. The behaviour now matches that of the > > > g4x/vlv/skl wm code in the presence of a zeroed latency > > > value. > > > > > > Cc: stable@xxxxxxxxxxxxxxx > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=101269 > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=103713 > > > Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> > > > --- > > > drivers/gpu/drm/i915/intel_pm.c | 41 ++++++++++++++++++++++++++++++++- > > > 1 file changed, 40 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > > > index 27498ded4949..ef1ae2ede379 100644 > > > --- a/drivers/gpu/drm/i915/intel_pm.c > > > +++ b/drivers/gpu/drm/i915/intel_pm.c > > > @@ -2493,6 +2493,9 @@ static uint32_t ilk_compute_pri_wm(const struct intel_crtc_state *cstate, > > > uint32_t method1, method2; > > > int cpp; > > > > > > + if (mem_value == 0) > > > + return USHRT_MAX; > > > + > > > if (!intel_wm_plane_visible(cstate, pstate)) > > > return 0; > > > > > > @@ -2522,6 +2525,9 @@ static uint32_t ilk_compute_spr_wm(const struct intel_crtc_state *cstate, > > > uint32_t method1, method2; > > > int cpp; > > > > > > + if (mem_value == 0) > > > + return USHRT_MAX; > > > + > > > if (!intel_wm_plane_visible(cstate, pstate)) > > > return 0; > > > > > > @@ -2545,6 +2551,9 @@ static uint32_t ilk_compute_cur_wm(const struct intel_crtc_state *cstate, > > > { > > > int cpp; > > > > > > + if (mem_value == 0) > > > + return USHRT_MAX; > > > + > > > if (!intel_wm_plane_visible(cstate, pstate)) > > > return 0; > > > > > > @@ -3008,6 +3017,34 @@ static void snb_wm_latency_quirk(struct drm_i915_private *dev_priv) > > > intel_print_wm_latency(dev_priv, "Cursor", dev_priv->wm.cur_latency); > > > } > > > > > > +static void snb_wm_lp3_irq_quirk(struct drm_i915_private *dev_priv) > > > +{ > > > + /* > > > + * On some SNB machines (Thinkpad X220 Tablet at least) > > > + * LP3 usage can cause vblank interrupts to be lost. > > > + * The DEIIR bit will go high but it looks like the CPU > > > + * never gets interrupted. > > > + * > > > + * It's not clear whether other interrupt source could > > > + * be affected or if this is somehow limited to vblank > > > + * interrupts only. To play it safe we disable LP3 > > > + * watermarks entirely. > > > + */ > > > + if (dev_priv->wm.pri_latency[3] == 0 && > > > + dev_priv->wm.spr_latency[3] == 0 && > > > + dev_priv->wm.cur_latency[3] == 0) > > > + return; > > > + > > > + dev_priv->wm.pri_latency[3] = 0; > > > + dev_priv->wm.spr_latency[3] = 0; > > > + dev_priv->wm.cur_latency[3] = 0; > > > > -> return USHRT_MAX for pri/spr/cur planes. > > -> result->enable = false; > > > > Only question then why USHRT_MAX for a u32 parameter? Seems to be copied > > from gm45 where it is a u16 parameter instead. > > A good question. My excuse is that I was expecting it to be a u16. > The max value the registers can hold is 11 bits, so u16 would be > more than enough for our needs. > > Looks like we store these as u32 in the struct as well so we end > up wasting a bit of memory. I'll go write a patch to shrink it > a bit. That's all fine. I'd like to see this patch use U32_MAX for consistency with itself in the meantime, but as far as the code doing what you claim, Acked-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> -Chris
