[PATCH 0/5] drm/i915/skl: Backport watermark fixes for 4.8.y

[PATCH 0/5] drm/i915/skl: Backport watermark fixes for 4.8.y

[Lyude]

Now that these have finally made it into 4.9, it's time to finally backport
these fixes. Skylake has been a mess in multi-monitor setups for a while now
because up until recently we've been updating the watermarks on Skylake just
like we would for previous generations of Intel GPUs. This means updating
attributes for each plane, and then only after they've been updated writing
their new watermark values.

The problem with this approach is Skylake has double buffered watermark
registers that are flipped at the same time as the rest of the plane registers.
This means that the original approach will leave planes active with new
attributes but without the required watermark programming that would ensure the
display pipe reads enough data from each plane. As a result, pipes start to
underrun and the user's displays starts to flicker heavily. Usually in response
to plugging in new monitors, or moving cursors from one screen to another
(which triggers a plane and watermark update).

Additionally, issues were found with the original code for configuring ddb,
display data buffer, allocations between display planes on Skylake. On Skylake
all planes have space allocated to them in the ddb, and the hardware requires
that these allocations never overlap at any point in time. Because ddb
allocations were not updated alongside plane attributes despite also being
double buffered registers armed by plane updates, planes were likely to get
stuck momentarily with ddb allocations that overlapped one another. This would
also lead to pipe underruns and display flickering.

The new approach fixes this problem by making sure that on Skylake, attributes
for display planes are always updated at the same time as the watermarks, and
pipes are updated in an order that ensures their ddb allocations don't
overlap at any point between plane updates. This ensures the display pipes are
always programmed correctly, and dramatically reduces the chance of display
flickering.

(note: my e-mail has changed since these patches were upstreamed, and I updated
the e-mails in these patches to reflect this. if this is wrong I will be happy
to update and resend the patches).

Lyude (4):
  drm/i915/skl: Update plane watermarks atomically during plane updates
  drm/i915: Move CRTC updating in atomic_commit into it's own hook
  drm/i915/skl: Update DDB values atomically with wms/plane attrs
  drm/i915/skl: Don't try to update plane watermarks if they haven't
    changed

Paulo Zanoni (1):
  drm/i915/gen9: only add the planes actually affected by ddb changes

 drivers/gpu/drm/i915/i915_drv.h      |   2 +
 drivers/gpu/drm/i915/intel_display.c | 189 +++++++++++++++++++-----
 drivers/gpu/drm/i915/intel_drv.h     |  12 ++
 drivers/gpu/drm/i915/intel_pm.c      | 271 ++++++++++++++---------------------
 drivers/gpu/drm/i915/intel_sprite.c  |  14 ++
 5 files changed, 289 insertions(+), 199 deletions(-)

-- 
2.7.4

[PATCH 1/5] drm/i915/skl: Update plane watermarks atomically during plane updates

[Lyude]

commit c145d3be1c313184be71d2629fd575561b7e38d4 upstream

Thanks to Ville for suggesting this as a potential solution to pipe
underruns on Skylake.

On Skylake all of the registers for configuring planes, including the
registers for configuring their watermarks, are double buffered. New
values written to them won't take effect until said registers are
"armed", which is done by writing to the PLANE_SURF (or in the case of
cursor planes, the CURBASE register) register.

With this in mind, up until now we've been updating watermarks on skl
like this:

  non-modeset {
   - calculate (during atomic check phase)
   - finish_atomic_commit:
     - intel_pre_plane_update:
        - intel_update_watermarks()
     - {vblank happens; new watermarks + old plane values => underrun }
     - drm_atomic_helper_commit_planes_on_crtc:
        - start vblank evasion
        - write new plane registers
        - end vblank evasion
  }

  or

  modeset {
   - calculate (during atomic check phase)
   - finish_atomic_commit:
     - crtc_enable:
        - intel_update_watermarks()
     - {vblank happens; new watermarks + old plane values => underrun }
     - drm_atomic_helper_commit_planes_on_crtc:
        - start vblank evasion
        - write new plane registers
        - end vblank evasion
  }

Now we update watermarks atomically like this:

  non-modeset {
   - calculate (during atomic check phase)
   - finish_atomic_commit:
     - intel_pre_plane_update:
        - intel_update_watermarks() (wm values aren't written yet)
     - drm_atomic_helper_commit_planes_on_crtc:
        - start vblank evasion
        - write new plane registers
        - write new wm values
        - end vblank evasion
  }

  modeset {
   - calculate (during atomic check phase)
   - finish_atomic_commit:
     - crtc_enable:
        - intel_update_watermarks() (actual wm values aren't written
          yet)
     - drm_atomic_helper_commit_planes_on_crtc:
        - start vblank evasion
        - write new plane registers
	- write new wm values
        - end vblank evasion
  }

So this patch moves all of the watermark writes into the right place;
inside of the vblank evasion where we update all of the registers for
each plane. While this patch doesn't fix everything, it does allow us to
update the watermark values in the way the hardware expects us to.

Changes since original patch series:
 - Remove mutex_lock/mutex_unlock since they don't do anything and we're
   not touching global state
 - Move skl_write_cursor_wm/skl_write_plane_wm functions into
   intel_pm.c, make externally visible
 - Add skl_write_plane_wm calls to skl_update_plane
 - Fix conditional for for loop in skl_write_plane_wm (level < max_level
   should be level <= max_level)
 - Make diagram in commit more accurate to what's actually happening
 - Add Fixes:

Changes since v1:
 - Use IS_GEN9() instead of IS_SKYLAKE() since these fixes apply to more
   then just Skylake
 - Update description to make it clear this patch doesn't fix everything
 - Check if pipes were actually changed before writing watermarks

Changes since v2:
 - Write PIPE_WM_LINETIME during vblank evasion

Changes since v3:
 - Rebase against new SAGV patch changes

Changes since v4:
 - Add a parameter to choose what skl_wm_values struct to use when
   writing new plane watermarks

Changes since v5:
 - Remove cursor ddb entry write in skl_write_cursor_wm(), defer until
   patch 6
 - Write WM_LINETIME in intel_begin_crtc_commit()

Changes since v6:
 - Remove redundant dirty_pipes check in skl_write_plane_wm (we check
   this in all places where we call this function, and it was supposed
   to have been removed earlier anyway)
 - In i9xx_update_cursor(), use dev_priv->info.gen >= 9 instead of
   IS_GEN9(dev_priv). We do this everywhere else and I'd imagine this
   needs to be done for gen10 as well

Changes since v7:
 - Fix rebase fail (unused variable obj)
 - Make struct skl_wm_values *wm const
 - Fix indenting
 - Use INTEL_GEN() instead of dev_priv->info.gen

Changes since v8:
 - Don't forget calls to skl_write_plane_wm() when disabling planes
 - Use INTEL_GEN(), not INTEL_INFO()->gen in intel_begin_crtc_commit()

Fixes: 2d41c0b59afc ("drm/i915/skl: SKL Watermark Computation")
Signed-off-by: Lyude <lyude@redhat.com>
Reviewed-by: Matt Roper <matthew.d.roper@intel.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>
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Link: http://patchwork.freedesktop.org/patch/msgid/1471884608-10671-1-git-send-email-cpaul@redhat.com
Link: http://patchwork.freedesktop.org/patch/msgid/1471884608-10671-1-git-send-email-cpaul@redhat.com
---
 drivers/gpu/drm/i915/intel_display.c | 21 +++++++++++++--
 drivers/gpu/drm/i915/intel_drv.h     |  5 ++++
 drivers/gpu/drm/i915/intel_pm.c      | 50 ++++++++++++++++++++++++------------
 drivers/gpu/drm/i915/intel_sprite.c  |  6 +++++
 4 files changed, 64 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 175595f..1ae587f 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2980,6 +2980,7 @@ static void skylake_update_primary_plane(struct drm_plane *plane,
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc_state->base.crtc);
 	struct drm_framebuffer *fb = plane_state->base.fb;
 	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
+	const struct skl_wm_values *wm = &dev_priv->wm.skl_results;
 	int pipe = intel_crtc->pipe;
 	u32 plane_ctl, stride_div, stride;
 	u32 tile_height, plane_offset, plane_size;
@@ -3031,6 +3032,9 @@ static void skylake_update_primary_plane(struct drm_plane *plane,
 	intel_crtc->adjusted_x = x_offset;
 	intel_crtc->adjusted_y = y_offset;
 
+	if (wm->dirty_pipes & drm_crtc_mask(&intel_crtc->base))
+		skl_write_plane_wm(intel_crtc, wm, 0);
+
 	I915_WRITE(PLANE_CTL(pipe, 0), plane_ctl);
 	I915_WRITE(PLANE_OFFSET(pipe, 0), plane_offset);
 	I915_WRITE(PLANE_SIZE(pipe, 0), plane_size);
@@ -3061,7 +3065,10 @@ static void skylake_disable_primary_plane(struct drm_plane *primary,
 {
 	struct drm_device *dev = crtc->dev;
 	struct drm_i915_private *dev_priv = to_i915(dev);
-	int pipe = to_intel_crtc(crtc)->pipe;
+	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);
 
 	I915_WRITE(PLANE_CTL(pipe, 0), 0);
 	I915_WRITE(PLANE_SURF(pipe, 0), 0);
@@ -10306,9 +10313,13 @@ static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base,
 	struct drm_device *dev = crtc->dev;
 	struct drm_i915_private *dev_priv = to_i915(dev);
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	const struct skl_wm_values *wm = &dev_priv->wm.skl_results;
 	int pipe = intel_crtc->pipe;
 	uint32_t cntl = 0;
 
+	if (INTEL_GEN(dev_priv) >= 9 && wm->dirty_pipes & drm_crtc_mask(crtc))
+		skl_write_cursor_wm(intel_crtc, wm);
+
 	if (plane_state && plane_state->visible) {
 		cntl = MCURSOR_GAMMA_ENABLE;
 		switch (plane_state->base.crtc_w) {
@@ -14221,10 +14232,12 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc,
 				    struct drm_crtc_state *old_crtc_state)
 {
 	struct drm_device *dev = crtc->dev;
+	struct drm_i915_private *dev_priv = to_i915(dev);
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	struct intel_crtc_state *old_intel_state =
 		to_intel_crtc_state(old_crtc_state);
 	bool modeset = needs_modeset(crtc->state);
+	enum pipe pipe = intel_crtc->pipe;
 
 	/* Perform vblank evasion around commit operation */
 	intel_pipe_update_start(intel_crtc);
@@ -14239,8 +14252,12 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc,
 
 	if (to_intel_crtc_state(crtc->state)->update_pipe)
 		intel_update_pipe_config(intel_crtc, old_intel_state);
-	else if (INTEL_INFO(dev)->gen >= 9)
+	else if (INTEL_GEN(dev_priv) >= 9) {
 		skl_detach_scalers(intel_crtc);
+
+		I915_WRITE(PIPE_WM_LINETIME(pipe),
+			   dev_priv->wm.skl_hw.wm_linetime[pipe]);
+	}
 }
 
 static void intel_finish_crtc_commit(struct drm_crtc *crtc,
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index ff399b9..58a8152 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1719,6 +1719,11 @@ void skl_ddb_get_hw_state(struct drm_i915_private *dev_priv,
 bool skl_can_enable_sagv(struct drm_atomic_state *state);
 int skl_enable_sagv(struct drm_i915_private *dev_priv);
 int skl_disable_sagv(struct drm_i915_private *dev_priv);
+void skl_write_cursor_wm(struct intel_crtc *intel_crtc,
+			 const struct skl_wm_values *wm);
+void skl_write_plane_wm(struct intel_crtc *intel_crtc,
+			const struct skl_wm_values *wm,
+			int plane);
 uint32_t ilk_pipe_pixel_rate(const struct intel_crtc_state *pipe_config);
 bool ilk_disable_lp_wm(struct drm_device *dev);
 int sanitize_rc6_option(struct drm_i915_private *dev_priv, int enable_rc6);
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 2d24813..29bed77 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3828,6 +3828,39 @@ static void skl_ddb_entry_write(struct drm_i915_private *dev_priv,
 		I915_WRITE(reg, 0);
 }
 
+void skl_write_plane_wm(struct intel_crtc *intel_crtc,
+			const struct skl_wm_values *wm,
+			int plane)
+{
+	struct drm_crtc *crtc = &intel_crtc->base;
+	struct drm_device *dev = crtc->dev;
+	struct drm_i915_private *dev_priv = to_i915(dev);
+	int level, max_level = ilk_wm_max_level(dev);
+	enum pipe pipe = intel_crtc->pipe;
+
+	for (level = 0; level <= max_level; level++) {
+		I915_WRITE(PLANE_WM(pipe, plane, level),
+			   wm->plane[pipe][plane][level]);
+	}
+	I915_WRITE(PLANE_WM_TRANS(pipe, plane), wm->plane_trans[pipe][plane]);
+}
+
+void skl_write_cursor_wm(struct intel_crtc *intel_crtc,
+			 const struct skl_wm_values *wm)
+{
+	struct drm_crtc *crtc = &intel_crtc->base;
+	struct drm_device *dev = crtc->dev;
+	struct drm_i915_private *dev_priv = to_i915(dev);
+	int level, max_level = ilk_wm_max_level(dev);
+	enum pipe pipe = intel_crtc->pipe;
+
+	for (level = 0; level <= max_level; level++) {
+		I915_WRITE(CUR_WM(pipe, level),
+			   wm->plane[pipe][PLANE_CURSOR][level]);
+	}
+	I915_WRITE(CUR_WM_TRANS(pipe), wm->plane_trans[pipe][PLANE_CURSOR]);
+}
+
 static void skl_write_wm_values(struct drm_i915_private *dev_priv,
 				const struct skl_wm_values *new)
 {
@@ -3835,7 +3868,7 @@ static void skl_write_wm_values(struct drm_i915_private *dev_priv,
 	struct intel_crtc *crtc;
 
 	for_each_intel_crtc(dev, crtc) {
-		int i, level, max_level = ilk_wm_max_level(dev);
+		int i;
 		enum pipe pipe = crtc->pipe;
 
 		if ((new->dirty_pipes & drm_crtc_mask(&crtc->base)) == 0)
@@ -3843,21 +3876,6 @@ static void skl_write_wm_values(struct drm_i915_private *dev_priv,
 		if (!crtc->active)
 			continue;
 
-		I915_WRITE(PIPE_WM_LINETIME(pipe), new->wm_linetime[pipe]);
-
-		for (level = 0; level <= max_level; level++) {
-			for (i = 0; i < intel_num_planes(crtc); i++)
-				I915_WRITE(PLANE_WM(pipe, i, level),
-					   new->plane[pipe][i][level]);
-			I915_WRITE(CUR_WM(pipe, level),
-				   new->plane[pipe][PLANE_CURSOR][level]);
-		}
-		for (i = 0; i < intel_num_planes(crtc); i++)
-			I915_WRITE(PLANE_WM_TRANS(pipe, i),
-				   new->plane_trans[pipe][i]);
-		I915_WRITE(CUR_WM_TRANS(pipe),
-			   new->plane_trans[pipe][PLANE_CURSOR]);
-
 		for (i = 0; i < intel_num_planes(crtc); i++) {
 			skl_ddb_entry_write(dev_priv,
 					    PLANE_BUF_CFG(pipe, i),
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 7c08e4f..d8bfff1 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -203,6 +203,9 @@ skl_update_plane(struct drm_plane *drm_plane,
 	struct intel_plane *intel_plane = to_intel_plane(drm_plane);
 	struct drm_framebuffer *fb = plane_state->base.fb;
 	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
+	const struct skl_wm_values *wm = &dev_priv->wm.skl_results;
+	struct drm_crtc *crtc = crtc_state->base.crtc;
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	const int pipe = intel_plane->pipe;
 	const int plane = intel_plane->plane + 1;
 	u32 plane_ctl, stride_div, stride;
@@ -238,6 +241,9 @@ skl_update_plane(struct drm_plane *drm_plane,
 	crtc_w--;
 	crtc_h--;
 
+	if (wm->dirty_pipes & drm_crtc_mask(crtc))
+		skl_write_plane_wm(intel_crtc, wm, plane);
+
 	if (key->flags) {
 		I915_WRITE(PLANE_KEYVAL(pipe, plane), key->min_value);
 		I915_WRITE(PLANE_KEYMAX(pipe, plane), key->max_value);
-- 
2.7.4

[PATCH 2/5] drm/i915: Move CRTC updating in atomic_commit into it's own hook

[Lyude]

commit 896e5bb022bce64e29ce2e1b2fc2a7476d311a15 upstream

Since we have to write ddb allocations at the same time as we do other
plane updates, we're going to need to be able to control the order in
which we execute modesets on each pipe. The easiest way to do this is to
just factor this section of intel_atomic_commit_tail()
(intel_atomic_commit() for stable branches) into it's own function, and
add an appropriate display function hook for it.

Based off of Matt Rope's suggestions

Changes since v1:
 - Drop pipe_config->base.active check in intel_update_crtcs() since we
   check that before calling the function

Signed-off-by: Lyude <cpaul@redhat.com>
Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
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>

Signed-off-by: Lyude <lyude@redhat.com>
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Link: http://patchwork.freedesktop.org/patch/msgid/1471961565-28540-1-git-send-email-cpaul@redhat.com
---
 drivers/gpu/drm/i915/i915_drv.h      |  2 +
 drivers/gpu/drm/i915/intel_display.c | 74 +++++++++++++++++++++++++-----------
 2 files changed, 54 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index f68c789..ad75dbd 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -631,6 +631,8 @@ struct drm_i915_display_funcs {
 				  struct intel_crtc_state *crtc_state);
 	void (*crtc_enable)(struct drm_crtc *crtc);
 	void (*crtc_disable)(struct drm_crtc *crtc);
+	void (*update_crtcs)(struct drm_atomic_state *state,
+			     unsigned int *crtc_vblank_mask);
 	void (*audio_codec_enable)(struct drm_connector *connector,
 				   struct intel_encoder *encoder,
 				   const struct drm_display_mode *adjusted_mode);
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 1ae587f..e2c46eb 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13682,6 +13682,52 @@ static bool needs_vblank_wait(struct intel_crtc_state *crtc_state)
 	return false;
 }
 
+static void intel_update_crtc(struct drm_crtc *crtc,
+			      struct drm_atomic_state *state,
+			      struct drm_crtc_state *old_crtc_state,
+			      unsigned int *crtc_vblank_mask)
+{
+	struct drm_device *dev = crtc->dev;
+	struct drm_i915_private *dev_priv = to_i915(dev);
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	struct intel_crtc_state *pipe_config = to_intel_crtc_state(crtc->state);
+	bool modeset = needs_modeset(crtc->state);
+
+	if (modeset) {
+		update_scanline_offset(intel_crtc);
+		dev_priv->display.crtc_enable(crtc);
+	} else {
+		intel_pre_plane_update(to_intel_crtc_state(old_crtc_state));
+	}
+
+	if (drm_atomic_get_existing_plane_state(state, crtc->primary)) {
+		intel_fbc_enable(
+		    intel_crtc, pipe_config,
+		    to_intel_plane_state(crtc->primary->state));
+	}
+
+	drm_atomic_helper_commit_planes_on_crtc(old_crtc_state);
+
+	if (needs_vblank_wait(pipe_config))
+		*crtc_vblank_mask |= drm_crtc_mask(crtc);
+}
+
+static void intel_update_crtcs(struct drm_atomic_state *state,
+			       unsigned int *crtc_vblank_mask)
+{
+	struct drm_crtc *crtc;
+	struct drm_crtc_state *old_crtc_state;
+	int i;
+
+	for_each_crtc_in_state(state, crtc, old_crtc_state, i) {
+		if (!crtc->state->active)
+			continue;
+
+		intel_update_crtc(crtc, state, old_crtc_state,
+				  crtc_vblank_mask);
+	}
+}
+
 static void intel_atomic_commit_tail(struct drm_atomic_state *state)
 {
 	struct drm_device *dev = state->dev;
@@ -13780,17 +13826,9 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
 		intel_modeset_verify_disabled(dev);
 	}
 
-	/* Now enable the clocks, plane, pipe, and connectors that we set up. */
+	/* Complete the events for pipes that have now been disabled */
 	for_each_crtc_in_state(state, crtc, old_crtc_state, i) {
-		struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 		bool modeset = needs_modeset(crtc->state);
-		struct intel_crtc_state *pipe_config =
-			to_intel_crtc_state(crtc->state);
-
-		if (modeset && crtc->state->active) {
-			update_scanline_offset(to_intel_crtc(crtc));
-			dev_priv->display.crtc_enable(crtc);
-		}
 
 		/* Complete events for now disable pipes here. */
 		if (modeset && !crtc->state->active && crtc->state->event) {
@@ -13800,21 +13838,11 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
 
 			crtc->state->event = NULL;
 		}
-
-		if (!modeset)
-			intel_pre_plane_update(to_intel_crtc_state(old_crtc_state));
-
-		if (crtc->state->active &&
-		    drm_atomic_get_existing_plane_state(state, crtc->primary))
-			intel_fbc_enable(intel_crtc, pipe_config, to_intel_plane_state(crtc->primary->state));
-
-		if (crtc->state->active)
-			drm_atomic_helper_commit_planes_on_crtc(old_crtc_state);
-
-		if (pipe_config->base.active && needs_vblank_wait(pipe_config))
-			crtc_vblank_mask |= 1 << i;
 	}
 
+	/* Now enable the clocks, plane, pipe, and connectors that we set up. */
+	dev_priv->display.update_crtcs(state, &crtc_vblank_mask);
+
 	/* FIXME: We should call drm_atomic_helper_commit_hw_done() here
 	 * already, but still need the state for the delayed optimization. To
 	 * fix this:
@@ -15275,6 +15303,8 @@ void intel_init_display_hooks(struct drm_i915_private *dev_priv)
 		dev_priv->display.crtc_disable = i9xx_crtc_disable;
 	}
 
+	dev_priv->display.update_crtcs = intel_update_crtcs;
+
 	/* Returns the core display clock speed */
 	if (IS_SKYLAKE(dev_priv) || IS_KABYLAKE(dev_priv))
 		dev_priv->display.get_display_clock_speed =
-- 
2.7.4

[PATCH 3/5] drm/i915/skl: Update DDB values atomically with wms/plane attrs

[Lyude]

commit 27082493e9c6371b05370a619ab9d2877c5f4726 upstream

Now that we can hook into update_crtcs and control the order in which we
update CRTCs at each modeset, we can finish the final step of fixing
Skylake's watermark handling by performing DDB updates at the same time
as plane updates and watermark updates.

The first major change in this patch is skl_update_crtcs(), which
handles ensuring that we order each CRTC update in our atomic commits
properly so that they honor the DDB flush order.

The second major change in this patch is the order in which we flush the
pipes. While the previous order may have worked, it can't be used in
this approach since it no longer will do the right thing. For example,
using the old ddb flush order:

We have pipes A, B, and C enabled, and we're disabling C. Initial ddb
allocation looks like this:

|   A   |   B   |xxxxxxx|

Since we're performing the ddb updates after performing any CRTC
disablements in intel_atomic_commit_tail(), the space to the right of
pipe B is unallocated.

1. Flush pipes with new allocation contained into old space. None
   apply, so we skip this
2. Flush pipes having their allocation reduced, but overlapping with a
   previous allocation. None apply, so we also skip this
3. Flush pipes that got more space allocated. This applies to A and B,
   giving us the following update order: A, B

This is wrong, since updating pipe A first will cause it to overlap with
B and potentially burst into flames. Our new order (see the code
comments for details) would update the pipes in the proper order: B, A.

As well, we calculate the order for each DDB update during the check
phase, and reference it later in the commit phase when we hit
skl_update_crtcs().

This long overdue patch fixes the rest of the underruns on Skylake.

Changes since v1:
 - Add skl_ddb_entry_write() for cursor into skl_write_cursor_wm()
Changes since v2:
 - Use the method for updating CRTCs that Ville suggested
 - In skl_update_wm(), only copy the watermarks for the crtc that was
   passed to us
Changes since v3:
 - Small comment fix in skl_ddb_allocation_overlaps()
Changes since v4:
 - Remove the second loop in intel_update_crtcs() and use Ville's
   suggestion for updating the ddb allocations in the right order
 - Get rid of the second loop and just use the ddb state as it updates
   to determine what order to update everything in (thanks for the
   suggestion Ville)
 - Simplify skl_ddb_allocation_overlaps()
 - Split actual overlap checking into it's own helper

Fixes: 0e8fb7ba7ca5 ("drm/i915/skl: Flush the WM configuration")
Fixes: 8211bd5bdf5e ("drm/i915/skl: Program the DDB allocation")
[omitting CC for stable, since this patch will need to be changed for
such backports first]

Testcase: kms_cursor_legacy
Testcase: plane-all-modeset-transition
Signed-off-by: Lyude <lyude@redhat.com>
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>
Cc: Matt Roper <matthew.d.roper@intel.com>
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Link: http://patchwork.freedesktop.org/patch/msgid/1471961565-28540-2-git-send-email-cpaul@redhat.com
---
 drivers/gpu/drm/i915/intel_display.c |  93 +++++++++++++---
 drivers/gpu/drm/i915/intel_drv.h     |   7 ++
 drivers/gpu/drm/i915/intel_pm.c      | 200 ++++++++---------------------------
 3 files changed, 132 insertions(+), 168 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index e2c46eb..8086e62 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12967,16 +12967,23 @@ static void verify_wm_state(struct drm_crtc *crtc,
 			  hw_entry->start, hw_entry->end);
 	}
 
-	/* cursor */
-	hw_entry = &hw_ddb.plane[pipe][PLANE_CURSOR];
-	sw_entry = &sw_ddb->plane[pipe][PLANE_CURSOR];
-
-	if (!skl_ddb_entry_equal(hw_entry, sw_entry)) {
-		DRM_ERROR("mismatch in DDB state pipe %c cursor "
-			  "(expected (%u,%u), found (%u,%u))\n",
-			  pipe_name(pipe),
-			  sw_entry->start, sw_entry->end,
-			  hw_entry->start, hw_entry->end);
+	/*
+	 * cursor
+	 * If the cursor plane isn't active, we may not have updated it's ddb
+	 * allocation. In that case since the ddb allocation will be updated
+	 * once the plane becomes visible, we can skip this check
+	 */
+	if (intel_crtc->cursor_addr) {
+		hw_entry = &hw_ddb.plane[pipe][PLANE_CURSOR];
+		sw_entry = &sw_ddb->plane[pipe][PLANE_CURSOR];
+
+		if (!skl_ddb_entry_equal(hw_entry, sw_entry)) {
+			DRM_ERROR("mismatch in DDB state pipe %c cursor "
+				  "(expected (%u,%u), found (%u,%u))\n",
+				  pipe_name(pipe),
+				  sw_entry->start, sw_entry->end,
+				  hw_entry->start, hw_entry->end);
+		}
 	}
 }
 
@@ -13728,6 +13735,65 @@ static void intel_update_crtcs(struct drm_atomic_state *state,
 	}
 }
 
+static void skl_update_crtcs(struct drm_atomic_state *state,
+			     unsigned int *crtc_vblank_mask)
+{
+	struct drm_device *dev = state->dev;
+	struct drm_i915_private *dev_priv = to_i915(dev);
+	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
+	struct drm_crtc *crtc;
+	struct drm_crtc_state *old_crtc_state;
+	struct skl_ddb_allocation *new_ddb = &intel_state->wm_results.ddb;
+	struct skl_ddb_allocation *cur_ddb = &dev_priv->wm.skl_hw.ddb;
+	unsigned int updated = 0;
+	bool progress;
+	enum pipe pipe;
+
+	/*
+	 * Whenever the number of active pipes changes, we need to make sure we
+	 * update the pipes in the right order so that their ddb allocations
+	 * never overlap with eachother inbetween CRTC updates. Otherwise we'll
+	 * cause pipe underruns and other bad stuff.
+	 */
+	do {
+		int i;
+		progress = false;
+
+		for_each_crtc_in_state(state, crtc, old_crtc_state, i) {
+			bool vbl_wait = false;
+			unsigned int cmask = drm_crtc_mask(crtc);
+			pipe = to_intel_crtc(crtc)->pipe;
+
+			if (updated & cmask || !crtc->state->active)
+				continue;
+			if (skl_ddb_allocation_overlaps(state, cur_ddb, new_ddb,
+							pipe))
+				continue;
+
+			updated |= cmask;
+
+			/*
+			 * If this is an already active pipe, it's DDB changed,
+			 * and this isn't the last pipe that needs updating
+			 * then we need to wait for a vblank to pass for the
+			 * new ddb allocation to take effect.
+			 */
+			if (!skl_ddb_allocation_equals(cur_ddb, new_ddb, pipe) &&
+			    !crtc->state->active_changed &&
+			    intel_state->wm_results.dirty_pipes != updated)
+				vbl_wait = true;
+
+			intel_update_crtc(crtc, state, old_crtc_state,
+					  crtc_vblank_mask);
+
+			if (vbl_wait)
+				intel_wait_for_vblank(dev, pipe);
+
+			progress = true;
+		}
+	} while (progress);
+}
+
 static void intel_atomic_commit_tail(struct drm_atomic_state *state)
 {
 	struct drm_device *dev = state->dev;
@@ -15303,8 +15369,6 @@ void intel_init_display_hooks(struct drm_i915_private *dev_priv)
 		dev_priv->display.crtc_disable = i9xx_crtc_disable;
 	}
 
-	dev_priv->display.update_crtcs = intel_update_crtcs;
-
 	/* Returns the core display clock speed */
 	if (IS_SKYLAKE(dev_priv) || IS_KABYLAKE(dev_priv))
 		dev_priv->display.get_display_clock_speed =
@@ -15394,6 +15458,11 @@ void intel_init_display_hooks(struct drm_i915_private *dev_priv)
 			skl_modeset_calc_cdclk;
 	}
 
+	if (dev_priv->info.gen >= 9)
+		dev_priv->display.update_crtcs = skl_update_crtcs;
+	else
+		dev_priv->display.update_crtcs = intel_update_crtcs;
+
 	switch (INTEL_INFO(dev_priv)->gen) {
 	case 2:
 		dev_priv->display.queue_flip = intel_gen2_queue_flip;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 58a8152..83674c6 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1719,6 +1719,13 @@ void skl_ddb_get_hw_state(struct drm_i915_private *dev_priv,
 bool skl_can_enable_sagv(struct drm_atomic_state *state);
 int skl_enable_sagv(struct drm_i915_private *dev_priv);
 int skl_disable_sagv(struct drm_i915_private *dev_priv);
+bool skl_ddb_allocation_equals(const struct skl_ddb_allocation *old,
+			       const struct skl_ddb_allocation *new,
+			       enum pipe pipe);
+bool skl_ddb_allocation_overlaps(struct drm_atomic_state *state,
+				 const struct skl_ddb_allocation *old,
+				 const struct skl_ddb_allocation *new,
+				 enum pipe pipe);
 void skl_write_cursor_wm(struct intel_crtc *intel_crtc,
 			 const struct skl_wm_values *wm);
 void skl_write_plane_wm(struct intel_crtc *intel_crtc,
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 29bed77..a0c5c4e 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3843,6 +3843,11 @@ void skl_write_plane_wm(struct intel_crtc *intel_crtc,
 			   wm->plane[pipe][plane][level]);
 	}
 	I915_WRITE(PLANE_WM_TRANS(pipe, plane), wm->plane_trans[pipe][plane]);
+
+	skl_ddb_entry_write(dev_priv, PLANE_BUF_CFG(pipe, plane),
+			    &wm->ddb.plane[pipe][plane]);
+	skl_ddb_entry_write(dev_priv, PLANE_NV12_BUF_CFG(pipe, plane),
+			    &wm->ddb.y_plane[pipe][plane]);
 }
 
 void skl_write_cursor_wm(struct intel_crtc *intel_crtc,
@@ -3859,170 +3864,46 @@ void skl_write_cursor_wm(struct intel_crtc *intel_crtc,
 			   wm->plane[pipe][PLANE_CURSOR][level]);
 	}
 	I915_WRITE(CUR_WM_TRANS(pipe), wm->plane_trans[pipe][PLANE_CURSOR]);
-}
-
-static void skl_write_wm_values(struct drm_i915_private *dev_priv,
-				const struct skl_wm_values *new)
-{
-	struct drm_device *dev = &dev_priv->drm;
-	struct intel_crtc *crtc;
-
-	for_each_intel_crtc(dev, crtc) {
-		int i;
-		enum pipe pipe = crtc->pipe;
-
-		if ((new->dirty_pipes & drm_crtc_mask(&crtc->base)) == 0)
-			continue;
-		if (!crtc->active)
-			continue;
-
-		for (i = 0; i < intel_num_planes(crtc); i++) {
-			skl_ddb_entry_write(dev_priv,
-					    PLANE_BUF_CFG(pipe, i),
-					    &new->ddb.plane[pipe][i]);
-			skl_ddb_entry_write(dev_priv,
-					    PLANE_NV12_BUF_CFG(pipe, i),
-					    &new->ddb.y_plane[pipe][i]);
-		}
 
-		skl_ddb_entry_write(dev_priv, CUR_BUF_CFG(pipe),
-				    &new->ddb.plane[pipe][PLANE_CURSOR]);
-	}
+	skl_ddb_entry_write(dev_priv, CUR_BUF_CFG(pipe),
+			    &wm->ddb.plane[pipe][PLANE_CURSOR]);
 }
 
-/*
- * When setting up a new DDB allocation arrangement, we need to correctly
- * sequence the times at which the new allocations for the pipes are taken into
- * account or we'll have pipes fetching from space previously allocated to
- * another pipe.
- *
- * Roughly the sequence looks like:
- *  1. re-allocate the pipe(s) with the allocation being reduced and not
- *     overlapping with a previous light-up pipe (another way to put it is:
- *     pipes with their new allocation strickly included into their old ones).
- *  2. re-allocate the other pipes that get their allocation reduced
- *  3. allocate the pipes having their allocation increased
- *
- * Steps 1. and 2. are here to take care of the following case:
- * - Initially DDB looks like this:
- *     |   B    |   C    |
- * - enable pipe A.
- * - pipe B has a reduced DDB allocation that overlaps with the old pipe C
- *   allocation
- *     |  A  |  B  |  C  |
- *
- * We need to sequence the re-allocation: C, B, A (and not B, C, A).
- */
-
-static void
-skl_wm_flush_pipe(struct drm_i915_private *dev_priv, enum pipe pipe, int pass)
+bool skl_ddb_allocation_equals(const struct skl_ddb_allocation *old,
+			       const struct skl_ddb_allocation *new,
+			       enum pipe pipe)
 {
-	int plane;
-
-	DRM_DEBUG_KMS("flush pipe %c (pass %d)\n", pipe_name(pipe), pass);
-
-	for_each_plane(dev_priv, pipe, plane) {
-		I915_WRITE(PLANE_SURF(pipe, plane),
-			   I915_READ(PLANE_SURF(pipe, plane)));
-	}
-	I915_WRITE(CURBASE(pipe), I915_READ(CURBASE(pipe)));
+	return new->pipe[pipe].start == old->pipe[pipe].start &&
+	       new->pipe[pipe].end == old->pipe[pipe].end;
 }
 
-static bool
-skl_ddb_allocation_included(const struct skl_ddb_allocation *old,
-			    const struct skl_ddb_allocation *new,
-			    enum pipe pipe)
+static inline bool skl_ddb_entries_overlap(const struct skl_ddb_entry *a,
+					   const struct skl_ddb_entry *b)
 {
-	uint16_t old_size, new_size;
-
-	old_size = skl_ddb_entry_size(&old->pipe[pipe]);
-	new_size = skl_ddb_entry_size(&new->pipe[pipe]);
-
-	return old_size != new_size &&
-	       new->pipe[pipe].start >= old->pipe[pipe].start &&
-	       new->pipe[pipe].end <= old->pipe[pipe].end;
+	return a->start < b->end && b->start < a->end;
 }
 
-static void skl_flush_wm_values(struct drm_i915_private *dev_priv,
-				struct skl_wm_values *new_values)
+bool skl_ddb_allocation_overlaps(struct drm_atomic_state *state,
+				 const struct skl_ddb_allocation *old,
+				 const struct skl_ddb_allocation *new,
+				 enum pipe pipe)
 {
-	struct drm_device *dev = &dev_priv->drm;
-	struct skl_ddb_allocation *cur_ddb, *new_ddb;
-	bool reallocated[I915_MAX_PIPES] = {};
-	struct intel_crtc *crtc;
-	enum pipe pipe;
-
-	new_ddb = &new_values->ddb;
-	cur_ddb = &dev_priv->wm.skl_hw.ddb;
-
-	/*
-	 * First pass: flush the pipes with the new allocation contained into
-	 * the old space.
-	 *
-	 * We'll wait for the vblank on those pipes to ensure we can safely
-	 * re-allocate the freed space without this pipe fetching from it.
-	 */
-	for_each_intel_crtc(dev, crtc) {
-		if (!crtc->active)
-			continue;
-
-		pipe = crtc->pipe;
-
-		if (!skl_ddb_allocation_included(cur_ddb, new_ddb, pipe))
-			continue;
-
-		skl_wm_flush_pipe(dev_priv, pipe, 1);
-		intel_wait_for_vblank(dev, pipe);
-
-		reallocated[pipe] = true;
-	}
-
-
-	/*
-	 * Second pass: flush the pipes that are having their allocation
-	 * reduced, but overlapping with a previous allocation.
-	 *
-	 * Here as well we need to wait for the vblank to make sure the freed
-	 * space is not used anymore.
-	 */
-	for_each_intel_crtc(dev, crtc) {
-		if (!crtc->active)
-			continue;
+	struct drm_device *dev = state->dev;
+	struct intel_crtc *intel_crtc;
+	enum pipe otherp;
 
-		pipe = crtc->pipe;
+	for_each_intel_crtc(dev, intel_crtc) {
+		otherp = intel_crtc->pipe;
 
-		if (reallocated[pipe])
+		if (otherp == pipe)
 			continue;
 
-		if (skl_ddb_entry_size(&new_ddb->pipe[pipe]) <
-		    skl_ddb_entry_size(&cur_ddb->pipe[pipe])) {
-			skl_wm_flush_pipe(dev_priv, pipe, 2);
-			intel_wait_for_vblank(dev, pipe);
-			reallocated[pipe] = true;
-		}
+		if (skl_ddb_entries_overlap(&new->pipe[pipe],
+					    &old->pipe[otherp]))
+			return true;
 	}
 
-	/*
-	 * 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.
-	 */
-	for_each_intel_crtc(dev, crtc) {
-		if (!crtc->active)
-			continue;
-
-		pipe = crtc->pipe;
-
-		/*
-		 * At this point, only the pipes more space than before are
-		 * left to re-allocate.
-		 */
-		if (reallocated[pipe])
-			continue;
-
-		skl_wm_flush_pipe(dev_priv, pipe, 3);
-	}
+	return false;
 }
 
 static int skl_update_pipe_wm(struct drm_crtc_state *cstate,
@@ -4224,7 +4105,7 @@ static void skl_update_wm(struct drm_crtc *crtc)
 	struct skl_wm_values *hw_vals = &dev_priv->wm.skl_hw;
 	struct intel_crtc_state *cstate = to_intel_crtc_state(crtc->state);
 	struct skl_pipe_wm *pipe_wm = &cstate->wm.skl.optimal;
-	int pipe;
+	enum pipe pipe = intel_crtc->pipe;
 
 	if ((results->dirty_pipes & drm_crtc_mask(crtc)) == 0)
 		return;
@@ -4233,15 +4114,22 @@ static void skl_update_wm(struct drm_crtc *crtc)
 
 	mutex_lock(&dev_priv->wm.wm_mutex);
 
-	skl_write_wm_values(dev_priv, results);
-	skl_flush_wm_values(dev_priv, results);
-
 	/*
-	 * Store the new configuration (but only for the pipes that have
-	 * changed; the other values weren't recomputed).
+	 * If this pipe isn't active already, we're going to be enabling it
+	 * very soon. Since it's safe to update a pipe's ddb allocation while
+	 * the pipe's shut off, just do so here. Already active pipes will have
+	 * their watermarks updated once we update their planes.
 	 */
-	for_each_pipe_masked(dev_priv, pipe, results->dirty_pipes)
-		skl_copy_wm_for_pipe(hw_vals, results, pipe);
+	if (crtc->state->active_changed) {
+		int plane;
+
+		for (plane = 0; plane < intel_num_planes(intel_crtc); plane++)
+			skl_write_plane_wm(intel_crtc, results, plane);
+
+		skl_write_cursor_wm(intel_crtc, results);
+	}
+
+	skl_copy_wm_for_pipe(hw_vals, results, pipe);
 
 	mutex_unlock(&dev_priv->wm.wm_mutex);
 }
-- 
2.7.4

[PATCH 4/5] drm/i915/skl: Don't try to update plane watermarks if they haven't changed

[Lyude]

commit ccebc23b57c313229526dc76383ce82f5e0b9001 upstream

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 <lyude@redhat.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Fixes: 62e0fb880123 ("drm/i915/skl: Update plane watermarks atomically during plane updates")
Testcase: kms_plane
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Link: http://patchwork.freedesktop.org/patch/msgid/1472488288-27280-1-git-send-email-cpaul@redhat.com
Cc: drm-intel-fixes@lists.freedesktop.org
---
 drivers/gpu/drm/i915/intel_display.c | 7 ++++++-
 drivers/gpu/drm/i915/intel_sprite.c  | 8 ++++++++
 2 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 8086e62..85d161a 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3068,7 +3068,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 (!to_intel_plane_state(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 d8bfff1..4178849 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -314,6 +314,14 @@ skl_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc)
 	const int pipe = intel_plane->pipe;
 	const int plane = intel_plane->plane + 1;
 
+	/*
+	 * We only populate skl_results on watermark updates, and if the
+	 * plane's visiblity isn't actually changing neither is its watermarks.
+	 */
+	if (!to_intel_plane_state(dplane->state)->visible)
+		skl_write_plane_wm(to_intel_crtc(crtc),
+				   &dev_priv->wm.skl_results, plane);
+
 	I915_WRITE(PLANE_CTL(pipe, plane), 0);
 
 	I915_WRITE(PLANE_SURF(pipe, plane), 0);
-- 
2.7.4

[PATCH 5/5] drm/i915/gen9: only add the planes actually affected by ddb changes

[Lyude]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

commit be5c571b2ff3a164d2e14ccc100cb5b2b3d3fb7c upstream

We were previously adding all the planes owned by the CRTC even when
the ddb partitioning didn't change for them. As a consequence, a lot
of functions were being called when we were just moving the cursor
around the screen, such as skylake_update_primary_plane().

This was causing flickering on the primary plane when moving the
cursor. I'm not 100% sure which operation caused the flickering, but
we were writing to a lot of registers, so it could be any of these
writes. With this patch, just moving the mouse won't add the primary
plane to the commit since it won't trigger a change in DDB
partitioning.

v2: Use skl_ddb_entry_equal() (Lyude).
v3: Change Reported-and-bisected-by: to Reported-by: for checkpatch

Fixes: 05a76d3d6ad1 ("drm/i915/skl: Ensure pipes with changed wms get added to the state")
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97888
Cc: Mike Lothian <mike@fireburn.co.uk>
Cc: stable@vger.kernel.org
Reported-by: Mike Lothian <mike@fireburn.co.uk>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
Signed-off-by: Lyude <lyude@redhat.com>
Link: http://patchwork.freedesktop.org/patch/msgid/1475177808-29955-1-git-send-email-paulo.r.zanoni@intel.com
(cherry picked from commit 7f60e200e254cd53ad1bd74a56bdd23e813ac4b7)
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 37 ++++++++++++++++++++++++++++++++++++-
 1 file changed, 36 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index a0c5c4e..5f763f4 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3940,6 +3940,41 @@ pipes_modified(struct drm_atomic_state *state)
 	return ret;
 }
 
+int
+skl_ddb_add_affected_planes(struct intel_crtc_state *cstate)
+{
+	struct drm_atomic_state *state = cstate->base.state;
+	struct drm_device *dev = state->dev;
+	struct drm_crtc *crtc = cstate->base.crtc;
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	struct drm_i915_private *dev_priv = to_i915(dev);
+	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
+	struct skl_ddb_allocation *new_ddb = &intel_state->wm_results.ddb;
+	struct skl_ddb_allocation *cur_ddb = &dev_priv->wm.skl_hw.ddb;
+	struct drm_plane_state *plane_state;
+	struct drm_plane *plane;
+	enum pipe pipe = intel_crtc->pipe;
+	int id;
+
+	WARN_ON(!drm_atomic_get_existing_crtc_state(state, crtc));
+
+	drm_for_each_plane_mask(plane, dev, crtc->state->plane_mask) {
+		id = skl_wm_plane_id(to_intel_plane(plane));
+
+		if (skl_ddb_entry_equal(&cur_ddb->plane[pipe][id],
+					&new_ddb->plane[pipe][id]) &&
+		    skl_ddb_entry_equal(&cur_ddb->y_plane[pipe][id],
+					&new_ddb->y_plane[pipe][id]))
+			continue;
+
+		plane_state = drm_atomic_get_plane_state(state, plane);
+		if (IS_ERR(plane_state))
+			return PTR_ERR(plane_state);
+	}
+
+	return 0;
+}
+
 static int
 skl_compute_ddb(struct drm_atomic_state *state)
 {
@@ -4004,7 +4039,7 @@ skl_compute_ddb(struct drm_atomic_state *state)
 		if (ret)
 			return ret;
 
-		ret = drm_atomic_add_affected_planes(state, &intel_crtc->base);
+		ret = skl_ddb_add_affected_planes(cstate);
 		if (ret)
 			return ret;
 	}
-- 
2.7.4

Re: [Intel-gfx] [PATCH 0/5] drm/i915/skl: Backport watermark fixes for 4.8.y

[Daniel Vetter]

On Wed, Oct 26, 2016 at 03:36:32PM -0400, Lyude wrote:
> Now that these have finally made it into 4.9, it's time to finally backport
> these fixes. Skylake has been a mess in multi-monitor setups for a while now
> because up until recently we've been updating the watermarks on Skylake just
> like we would for previous generations of Intel GPUs. This means updating
> attributes for each plane, and then only after they've been updated writing
> their new watermark values.
> 
> The problem with this approach is Skylake has double buffered watermark
> registers that are flipped at the same time as the rest of the plane registers.
> This means that the original approach will leave planes active with new
> attributes but without the required watermark programming that would ensure the
> display pipe reads enough data from each plane. As a result, pipes start to
> underrun and the user's displays starts to flicker heavily. Usually in response
> to plugging in new monitors, or moving cursors from one screen to another
> (which triggers a plane and watermark update).
> 
> Additionally, issues were found with the original code for configuring ddb,
> display data buffer, allocations between display planes on Skylake. On Skylake
> all planes have space allocated to them in the ddb, and the hardware requires
> that these allocations never overlap at any point in time. Because ddb
> allocations were not updated alongside plane attributes despite also being
> double buffered registers armed by plane updates, planes were likely to get
> stuck momentarily with ddb allocations that overlapped one another. This would
> also lead to pipe underruns and display flickering.
> 
> The new approach fixes this problem by making sure that on Skylake, attributes
> for display planes are always updated at the same time as the watermarks, and
> pipes are updated in an order that ensures their ddb allocations don't
> overlap at any point between plane updates. This ensures the display pipes are
> always programmed correctly, and dramatically reduces the chance of display
> flickering.
> 
> (note: my e-mail has changed since these patches were upstreamed, and I updated
> the e-mails in these patches to reflect this. if this is wrong I will be happy
> to update and resend the patches).
> 
> Lyude (4):
>   drm/i915/skl: Update plane watermarks atomically during plane updates
>   drm/i915: Move CRTC updating in atomic_commit into it's own hook
>   drm/i915/skl: Update DDB values atomically with wms/plane attrs
>   drm/i915/skl: Don't try to update plane watermarks if they haven't
>     changed
> 
> Paulo Zanoni (1):
>   drm/i915/gen9: only add the planes actually affected by ddb changes

fwiw ack from my side.
-Daniel

> 
>  drivers/gpu/drm/i915/i915_drv.h      |   2 +
>  drivers/gpu/drm/i915/intel_display.c | 189 +++++++++++++++++++-----
>  drivers/gpu/drm/i915/intel_drv.h     |  12 ++
>  drivers/gpu/drm/i915/intel_pm.c      | 271 ++++++++++++++---------------------
>  drivers/gpu/drm/i915/intel_sprite.c  |  14 ++
>  5 files changed, 289 insertions(+), 199 deletions(-)
> 
> -- 
> 2.7.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 0/5] drm/i915/skl: Backport watermark fixes for 4.8.y

[Greg KH]

On Wed, Oct 26, 2016 at 03:36:32PM -0400, Lyude wrote:
> Now that these have finally made it into 4.9, it's time to finally backport
> these fixes. Skylake has been a mess in multi-monitor setups for a while now
> because up until recently we've been updating the watermarks on Skylake just
> like we would for previous generations of Intel GPUs. This means updating
> attributes for each plane, and then only after they've been updated writing
> their new watermark values.
> 
> The problem with this approach is Skylake has double buffered watermark
> registers that are flipped at the same time as the rest of the plane registers.
> This means that the original approach will leave planes active with new
> attributes but without the required watermark programming that would ensure the
> display pipe reads enough data from each plane. As a result, pipes start to
> underrun and the user's displays starts to flicker heavily. Usually in response
> to plugging in new monitors, or moving cursors from one screen to another
> (which triggers a plane and watermark update).
> 
> Additionally, issues were found with the original code for configuring ddb,
> display data buffer, allocations between display planes on Skylake. On Skylake
> all planes have space allocated to them in the ddb, and the hardware requires
> that these allocations never overlap at any point in time. Because ddb
> allocations were not updated alongside plane attributes despite also being
> double buffered registers armed by plane updates, planes were likely to get
> stuck momentarily with ddb allocations that overlapped one another. This would
> also lead to pipe underruns and display flickering.
> 
> The new approach fixes this problem by making sure that on Skylake, attributes
> for display planes are always updated at the same time as the watermarks, and
> pipes are updated in an order that ensures their ddb allocations don't
> overlap at any point between plane updates. This ensures the display pipes are
> always programmed correctly, and dramatically reduces the chance of display
> flickering.
> 
> (note: my e-mail has changed since these patches were upstreamed, and I updated
> the e-mails in these patches to reflect this. if this is wrong I will be happy
> to update and resend the patches).

Thanks for these, all now queued up.

greg k-h