Since this patch has been on hold for a little bit, I did a bit of thinking of how we could this a little more cleanly. Unfortunately I couldn't think of a way, however I did think of an alternative solution:

I'm planning on backporting all of the skl wm fixes already, so I'm going to use this patch for that since it's very small. As for mainline, I'm going to do a whole reorganization of the skl wm/ddb structs in i915 like Matt had suggested before. Things might look a little more like this (taken from my half-complete reorganization):

struct skl_plane_ddb_allocation { struct skl_ddb_entry plane; struct skl_ddb_entry y_plane; };

struct skl_plane_wm_values { struct skl_plane_ddb_allocation ddb; uint32_t wm[8]; uint32_t trans_wm; };

struct skl_pipe_wm_values { struct skl_ddb_entry ddb; uint32_t linetime; };

struct skl_hw_wm_values { /* (only used for skl_get_hw_ddb and friends) */ struct skl_pipe_wm_values pipe[I915_MAX_PIPES]; struct skl_plane_wm_values plane[I915_MAX_PIPES][I915_MAX_PLANES]; };

As well, I'm also just going to completely remove the skl_results and skl_hw structs from struct drm_i915_private. This makes sense for a lot of reasons: * This completely gets rid of the need for a global watermark lock (on Skylake at least) and will make things a lot easier for atomic support in the future * Skylake doesn't have any actual global watermark hooks anyway, aside from skl_update_wm() which is now only used for writing watermarks for inactive pipes during haswell_crtc_enable() * This makes passing watermarks around way less of a mess * Saves a tiny bit of data, and so far being able to grab watermarks/ddbs right from the plane states seems to be a lot easier then messing with a large array

As for this fix, I'll probably still need someone to review it so I can get it into 4.7.y.

Let me know what you think.

On Mon, 2016-08-29 at 12:31 -0400, Lyude wrote:

i915 sometimes needs to disable planes in the middle of an atomic commit, and then reenable them later in the same commit. Because of this, we can't make the assumption that the state of the plane actually changed. Since the state of the plane hasn't actually changed, neither have it's watermarks. And if the watermarks hasn't changed then we haven't populated skl_results with anything, which means we'll end up zeroing out a plane's watermarks in the middle of the atomic commit without restoring them later.

Simple reproduction recipe:  - Get a SKL laptop, launch any kind of X session  - Get two extra monitors  - Keep hotplugging both displays (so that the display configuration    jumps from 1 active pipe to 3 active pipes and back)  - Eventually underrun

Changes since v1:  - Fix incorrect use of "it's" Changes since v2:  - Add reproduction recipe

Signed-off-by: Lyude cpaul@redhat.com Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Fixes: 62e0fb880123 ("drm/i915/skl: Update plane watermarks atomically during plane updates")

Signed-off-by: Lyude cpaul@redhat.com

drivers/gpu/drm/i915/intel_display.c | 7 ++++++-  drivers/gpu/drm/i915/intel_sprite.c  | 9 +++++++--  2 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index e4e6141..13e47a7 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -3448,7 +3448,12 @@ static void skylake_disable_primary_plane(struct drm_plane *primary,   struct intel_crtc *intel_crtc = to_intel_crtc(crtc);   int pipe = intel_crtc->pipe;

• skl_write_plane_wm(intel_crtc, &dev_priv->wm.skl_results,

0);

• /*
• * We only populate skl_results on watermark updates, and if

the

• * plane's visiblity isn't actually changing neither is its

watermarks.

• */
• if (!crtc->primary->state->visible)

• skl_write_plane_wm(intel_crtc, &dev_priv-
  

...

wm.skl_results, 0);

I915_WRITE(PLANE_CTL(pipe, 0), 0);   I915_WRITE(PLANE_SURF(pipe, 0), 0); diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c index 0df783a..73a521f 100644 --- a/drivers/gpu/drm/i915/intel_sprite.c +++ b/drivers/gpu/drm/i915/intel_sprite.c @@ -292,8 +292,13 @@ skl_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc)   const int pipe = intel_plane->pipe;   const int plane = intel_plane->plane + 1;

• skl_write_plane_wm(to_intel_crtc(crtc), &dev_priv-

...

wm.skl_results,

• 	   plane);
  

• /*
• * We only populate skl_results on watermark updates, and if

the

• * plane's visiblity isn't actually changing neither is its

watermarks.

• */
• if (!dplane->state->visible)

• skl_write_plane_wm(to_intel_crtc(crtc),
  

• 		   &dev_priv->wm.skl_results,
  

plane);     I915_WRITE(PLANE_CTL(pipe, plane), 0);