So I've been working on trying to fix this entirely again (e.g. writing the ddb properly), since from bug reports it still doesn't sound like we've got enough workarounds to make this tolerable. I've shown this to matt roper, but I should probably post what I've been trying to do for you as well.
So the approach I came up with is here
https://github.com/lyude/linux/tree/wip/skl-fix-wms-v5r2
My approach differs a little bit from what the bspec recommends, but I think it's going to be a bit easier to implement. At the end of all the changes I'm attempting it should look like this:
* We no longer have a global watermark update for skl * A new hook called "update_ddbs" is added to i915_display_funcs. This gets called in intel_atomic_commit_tail() after we've disabled any CRTCs that needed disabling, and before we begin enabling/updating CRTCs * Because pipe ddb allocations (not the inner-pipe ddb allocations that apply to each pipe's planes) only change while enabling/disabling crtcs: * Pass 1: Find which pipe's new ddb allocation won't overlap with another pipe's previous allocation, and update that pipe first * Pass 2: Update the allocation of the remaining pipe
Here's an illustration of what this looks like. Parts of the ddb not being used by any CRTCs are marked out with 'x's:
With pipe A enabled, we enable pipe B: Initial DDB: | A | update_ddbs: | A |xxxxxxxxxxx| Pass 1 Enable pipe B: | A | B |
With pipes B and A active, we enable pipe C:
Initial DDB: | B | A | update_ddbs: | B |xxx| A | Pass 1 update_ddbs: | B | A |xxxxxxx| Pass 2 Enable pipe C: | B | A | C |
With pipes A, B, and C active, we disable B: Initial DDB: | A | B | C | Disable pipe B: | A |xxxxxxx| C | update_ddbs: | A | C | Pass 1 Since neither pipe's new allocation overlapped, we skip pass 2
Another allocation with A, B, and C active, disabling A: Initial DDB: | A | B | C | Disable pipe A: |xxxxxxx| B | C | update_ddbs: | B |xxx| C | Pass 1 update_ddbs: | B | C | Pass 2
This should ensure we can always move around the allocations of pipes without them ever overlapping and exploding.
This branch doesn't entirely fix underrun issues, but I'm mostly sure that's the fault of still not having removed the global wm update hook yet (which is leading to additional pipe flushes in places they shouldn't be):
https://github.com/lyude/linux/tree/wip/skl-fix-wms-v5r2
As for updating inner-pipe ddb allocations for each plane on a pipe, we should be able to just do that at the same time we update each pipe's watermarks
Let me know what you think Lyude
On Fri, 2016-07-29 at 12:39 +0300, Ville Syrjälä wrote:
On Thu, Jul 28, 2016 at 05:03:52PM -0700, Matt Roper wrote:
This is completely untested (and probably horribly broken/buggy), but here's a quick mockup of the general approach I was thinking for ensuring DDB & WM's can be updated together while ensuring the three-step pipe flushing process is honored:
https://github.com/mattrope/kernel/commits/experimental/lyu de_ddb
Basically the idea is to take note of what's happening to the pipe's DDB allocation (shrinking, growing, unchanged, etc.) during the atomic check phase;
Didn't look too closely, but I think you can't actually do that unless you lock all the crtcs whenever the number of active pipes is goind to change. Meaning we'd essentially be back to the one-big-modeset-lock apporach, which will cause missed flips and whanot on the other pipes.
The alternative I think would consist of:
- make sure level 0 watermark never exceeds total_ddb_size/max_pipes,
so that a modeset doesn't have to care about the wms for the other pipes not fitting in
- level 1+ watermarks would be checked against total_ddb_size
- protect the plane/pipe commit with the wm mutex whenever the wms
need to be reprogrammed
- keep the flush_wm thing around for the case when ddb size does get
changed, protect it with the wm lock
- when programming wms, we will first filter out any level that
doesn't fit in with the current ddb size, and then program the rest in
- potentially introduce per-pipe wm locks if the one big lock looks
like an issue, which it might if the flush_wm holds it all the way through
then during the commit phase, we loop over the CRTC's three times instead of just once, but only operate on a subset of the CRTC's in each loop. While operating on each CRTC, the plane, WM, and DDB all get programmed together and have a single flush for all three.
Matt
On Tue, Jul 26, 2016 at 01:34:36PM -0400, Lyude wrote:
Latest version of https://lkml.org/lkml/2016/7/26/290 . Resending the whole thing to keep it in one place.
Lyude (5): drm/i915/skl: Add support for the SAGV, fix underrun hangs drm/i915/skl: Only flush pipes when we change the ddb allocation drm/i915/skl: Fix extra whitespace in skl_flush_wm_values() drm/i915/skl: Update plane watermarks atomically during plane updates drm/i915/skl: Always wait for pipes to update after a flush
Matt Roper (1): drm/i915/gen9: Only copy WM results for changed pipes to skl_hw
drivers/gpu/drm/i915/i915_drv.h | 3 + drivers/gpu/drm/i915/i915_reg.h | 5 + drivers/gpu/drm/i915/intel_display.c | 24 ++++ drivers/gpu/drm/i915/intel_drv.h | 4 + drivers/gpu/drm/i915/intel_pm.c | 240 +++++++++++++++++++++++++++++++---- drivers/gpu/drm/i915/intel_sprite.c | 2 + 6 files changed, 255 insertions(+), 23 deletions(-)
-- 2.7.4
Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
-- Matt Roper Graphics Software Engineer IoTG Platform Enabling & Development Intel Corporation (916) 356-2795