Two things for this one:
1. I completely messed up the description on this patchset, this was for fixing
underruns on pipe disablement. I'm impressed I wrote up that whole description
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 when 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:
> >
> > As we've learned, all watermark updates on Skylake have to be strictly
> > 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 example
> > assumes that we're lucky enough not to be interrupted by the scheduler
> > at any point during this:
> >
> > - Start with pipe A and pipe B enabled
> > - Enable pipe C
> > - Flush pipe A in pass 1, wait until update finishes
> > - Flush pipe B in pass 3, continue without waiting for next vblank
> > - Start another wm update
> > - We enter the next vblank for pipe B before we finish writing all the
> > vm values
> > - *Underrun*
> >
> > As such, we always need to wait for each pipe we flush to update so as
> > to never break this atomicity.
>
> I'm not sure I follow this commit. If 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.
>
> My understanding is that the problem that still remains (now that your
> first three patches have made progress towards fixing underruns) is that
> our DDB updates and flushes (which come from update_watermarks) happen
> pre-evasion, whereas the watermarks themselves now happen under evasion.
> We really want both the new DDB value and the new watermark value to be
> written together and take effect on the same vblank. I think the
> problem is that you might have a shrinking DDB allocation (e.g., because
> a new pipe was added or you changed a mode that changed the DDB balance)
> which some of the existing WM values exceed. You can have a sequence
> like this:
>
> - update_wm:
> - write new (smaller) DDB
> - flush DDB
> - vblank happens, old (big) wm + new (small) ddb = underrun
> - vblank evasion:
> - write new plane regs and WM's
> - flush
> - post-evasion vblank happens, underrun is corrected
>
> 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. However 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). To 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 ordering.
> 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.
>
> 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 values
> 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. Of course I've kicked that idea around in 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.
>
>
> Matt
>
> >
> >
> > Fixes: 0e8fb7ba7ca5 ("drm/i915/skl: Flush the WM configuration")
> > Signed-off-by: Lyude <cpaul@xxxxxxxxxx>
> > Cc: stable@xxxxxxxxxxxxxxx
> > Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > Cc: Daniel Vetter <daniel.vetter@xxxxxxxxx>
> > Cc: Radhakrishna Sripada <radhakrishna.sripada@xxxxxxxxx>
> > Cc: Hans de Goede <hdegoede@xxxxxxxxxx> <cpaul@xxxxxxxxxx>
> > Cc: Matt Roper <matthew.d.roper@xxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/intel_pm.c | 17 +++++++++++++++--
> > 1 file changed, 15 insertions(+), 2 deletions(-)
> >
> > 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,
> > /*
> > * Third pass: flush the pipes that got more space allocated.
> > *
> > - * We don't need to actively wait for the update here, next vblank
> > - * will just get more DDB space with the correct WM values.
> > + * While the hardware doesn't require to wait for the next vblank
> > here,
> > + * continuing before the pipe finishes updating could result in us
> > + * trying to update the wm values again before the pipe finishes
> > + * updating, which results in the hardware using intermediate wm
> > values
> > + * and subsequently underrunning pipes.
> > */
> > for_each_intel_crtc(dev, crtc) {
> > if (!crtc->active)
> > @@ -3876,6 +3879,16 @@ static void skl_flush_wm_values(struct
> > drm_i915_private *dev_priv,
> > continue;
> >
> > skl_wm_flush_pipe(dev_priv, pipe, 3);
> > +
> > + /*
> > + * The only time we can get away with not waiting for an
> > update
> > + * is when we just enabled the pipe, e.g. when it doesn't
> > have
> > + * vblanks enabled anyway.
> > + */
> > + if (drm_crtc_vblank_get(&crtc->base) == 0) {
> > + intel_wait_for_vblank(dev, pipe);
> > + drm_crtc_vblank_put(&crtc->base);
> > + }
> > }
> > }
> >
> > --
> > 2.7.4
> >
>
--
To unsubscribe from this list: send the line "unsubscribe stable" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html