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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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@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@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@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.

-- 
Ville Syrjälä
Intel