[Intel-gfx] [PATCH] drm/i915: Disable LP3 watermarks on all SNB machines

[Chris Wilson]

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 at linux.intel.com>
> > > 
> > > 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 at vger.kernel.org
> > > 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 at linux.intel.com>
> > > ---
> > >  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 at chris-wilson.co.uk>
-Chris