From mboxrd@z Thu Jan 1 00:00:00 1970 From: Lyude Paul Subject: Re: [PATCH 4/6] drm/i915/skl: Always wait for pipes to update after a flush Date: Thu, 21 Jul 2016 13:01:36 -0400 Message-ID: <1469120496.4337.2.camel@redhat.com> References: <1469048403-32016-1-git-send-email-cpaul@redhat.com> <1469048403-32016-5-git-send-email-cpaul@redhat.com> <20160721002738.GE1579@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: QUOTED-PRINTABLE Return-path: In-Reply-To: <20160721002738.GE1579@intel.com> Sender: stable-owner@vger.kernel.org To: Matt Roper Cc: intel-gfx@lists.freedesktop.org, stable@vger.kernel.org, Ville =?ISO-8859-1?Q?Syrj=E4l=E4?= , Daniel Vetter , Radhakrishna Sripada , Jani Nikula , David Airlie , "open list:INTEL DRM DRIVERS (excluding Poulsbo, Moorestow...), linux-kernel@vger.kernel.org (open list)" List-Id: intel-gfx@lists.freedesktop.org Two things for this one: 1. I completely messed up the description on this patchset, this was fo= r fixing underruns on pipe disablement. I'm impressed I wrote up that whole desc= ription without realizing that at all, lol. 2. This patch may not actually be necessary. On the original git branch= I was testing this on it was required to prevent underruns on pipe disables, = but trying this on nightly I don't seem to reproduce those underruns even w= hen I remove this patch, so I guess we can drop this from the series On Wed, 2016-07-20 at 17:27 -0700, Matt Roper wrote: > On Wed, Jul 20, 2016 at 05:00:00PM -0400, Lyude wrote: > >=20 > > As we've learned, all watermark updates on Skylake have to be stric= tly > > atomic or things fail. While the bspec doesn't mandate that we need= to > > wait for pipes to finish after the third iteration of flushes, not = doing > > so gives us the opportunity to break this atomicity later. This exa= mple > > assumes that we're lucky enough not to be interrupted by the schedu= ler > > at any point during this: > >=20 > > =C2=A0- Start with pipe A and pipe B enabled > > =C2=A0- Enable pipe C > > =C2=A0- Flush pipe A in pass 1, wait until update finishes > > =C2=A0- Flush pipe B in pass 3, continue without waiting for next v= blank > > =C2=A0- Start another wm update > > =C2=A0- We enter the next vblank for pipe B before we finish writin= g all the > > =C2=A0=C2=A0=C2=A0vm values > > =C2=A0- *Underrun* > >=20 > > As such, we always need to wait for each pipe we flush to update so= as > > to never break this atomicity. >=20 > I'm not sure I follow this commit.=C2=A0=C2=A0If we're enabling a new= pipe, the > the allocation for A and B are generally going to shrink, so they'll > usually be flushed in passes 1 and 2, not 3. >=20 > My understanding is that the problem that still remains (now that you= r > first three patches have made progress towards fixing underruns) is t= hat > our DDB updates and flushes (which come from update_watermarks) happe= n > pre-evasion, whereas the watermarks themselves now happen under evasi= on. > We really want both the new DDB value and the new watermark value to = be > written together and take effect on the same vblank.=C2=A0=C2=A0I thi= nk the > problem is that you might have a shrinking DDB allocation (e.g., beca= use > a new pipe was added or you changed a mode that changed the DDB balan= ce) > which some of the existing WM values exceed.=C2=A0=C2=A0You can have = a sequence > like this: >=20 > =C2=A0 - update_wm: > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0- write new (smaller) DDB > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0- flush DDB > =C2=A0 - vblank happens, old (big) wm + new (small) ddb =3D underrun > =C2=A0 - vblank evasion: > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0- write new plane regs and WM's > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0- flush > =C2=A0 - post-evasion vblank happens, underrun is corrected >=20 > I think ultimately we want to move the DDB register writes into the > update functions that happen under evasion, just like you did for the= WM > registers.=C2=A0=C2=A0However just doing this the straightforward way= won't > satisfy our requirements about pipe update ordering (the three passes > you see today in skl_flush_wm_values).=C2=A0=C2=A0To make that work, = I think the > general approach is that we need to basically replace the > for_each_crtc_in_state() loop in intel_atomic_commit_tail() with some > new CRTC iterator that processes CRTC's in a more intelligent orderin= g. > We've computed our DDB changes during the atomic check phase, so we > already know which allocations are shrinking, growing, etc. and we > should be able to calculate an appropriate CRTC ordering at the same > time. >=20 > With an intelligent CRTC iterator that follows the pre-computed pipe > ordering rules (and adds the necessary vblank waits between each > "phase"), I think we should be able to just write both DDB and WM val= ues > in the skl_update_primary_plane() and similar functions and let the > existing flushes that happen take care of flushing them out at the > appropriate time.=C2=A0=C2=A0Of course I've kicked that idea around i= n my head for > a while, but haven't had time to actually write any code for it, so I > may be completely overlooking some stumbling block that makes it much > more complicated than I'm envisioning. >=20 >=20 > Matt >=20 > >=20 > >=20 > > Fixes: 0e8fb7ba7ca5 ("drm/i915/skl: Flush the WM configuration") > > Signed-off-by: Lyude > > Cc: stable@vger.kernel.org > > Cc: Ville Syrj=C3=A4l=C3=A4 > > Cc: Daniel Vetter > > Cc: Radhakrishna Sripada > > Cc: Hans de Goede > > Cc: Matt Roper > > --- > > =C2=A0drivers/gpu/drm/i915/intel_pm.c | 17 +++++++++++++++-- > > =C2=A01 file changed, 15 insertions(+), 2 deletions(-) > >=20 > > diff --git a/drivers/gpu/drm/i915/intel_pm.c > > b/drivers/gpu/drm/i915/intel_pm.c > > index 788db86..2e31df4 100644 > > --- a/drivers/gpu/drm/i915/intel_pm.c > > +++ b/drivers/gpu/drm/i915/intel_pm.c > > @@ -3859,8 +3859,11 @@ static void skl_flush_wm_values(struct > > drm_i915_private *dev_priv, > > =C2=A0 /* > > =C2=A0 =C2=A0* Third pass: flush the pipes that got more space allo= cated. > > =C2=A0 =C2=A0* > > - =C2=A0* We don't need to actively wait for the update here, next = vblank > > - =C2=A0* will just get more DDB space with the correct WM values. > > + =C2=A0* While the hardware doesn't require to wait for the next v= blank > > here, > > + =C2=A0* continuing before the pipe finishes updating could result= in us > > + =C2=A0* trying to update the wm values again before the pipe fini= shes > > + =C2=A0* updating, which results in the hardware using intermediat= e wm > > values > > + =C2=A0* and subsequently underrunning pipes. > > =C2=A0 =C2=A0*/ > > =C2=A0 for_each_intel_crtc(dev, crtc) { > > =C2=A0 if (!crtc->active) > > @@ -3876,6 +3879,16 @@ static void skl_flush_wm_values(struct > > drm_i915_private *dev_priv, > > =C2=A0 continue; > > =C2=A0 > > =C2=A0 skl_wm_flush_pipe(dev_priv, pipe, 3); > > + > > + /* > > + =C2=A0* The only time we can get away with not waiting for an > > update > > + =C2=A0* is when we just enabled the pipe, e.g. when it doesn't > > have > > + =C2=A0* vblanks enabled anyway. > > + =C2=A0*/ > > + if (drm_crtc_vblank_get(&crtc->base) =3D=3D 0) { > > + intel_wait_for_vblank(dev, pipe); > > + drm_crtc_vblank_put(&crtc->base); > > + } > > =C2=A0 } > > =C2=A0} > > =C2=A0 > > --=C2=A0 > > 2.7.4 > >=20 >=20