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@redhat.com Cc: stable@vger.kernel.org Cc: Ville Syrjälä ville.syrjala@linux.intel.com Cc: Daniel Vetter daniel.vetter@intel.com Cc: Radhakrishna Sripada radhakrishna.sripada@intel.com Cc: Hans de Goede hdegoede@redhat.com cpaul@redhat.com Cc: Matt Roper matthew.d.roper@intel.com
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.@@ -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