[PATCH 0/9] Wrap up ILK-style atomic watermarks

[PATCH 0/9] Wrap up ILK-style atomic watermarks

[Matt Roper]

We've been chasing a regression[1] that prevented us from merging the last
couple patches of the ILK-style atomic watermark series.  We've finally
identified the culprit --- if we fail to reconstruct the BIOS FB, our internal
driver state was left in an inconsistent state which caused confusion for the
watermark code.  Here's a series that collects that fix (along with a couple
other bugfixes that were found while debugging), a few debugging enhancements,
and the completion of the ILK atomic watermark series.

 * The first patch in this series addresses that issue and
   ensures that we cleanly turn off the primary plane if we're unable to inherit
   the framebuffer.  
 * Patches 2-4 just clarify/expand some of the information dumped by the
   driver while debugging.
 * Patches 5 and 6 fix two additional bugs that were discovered while
   searching for the cause of the regression.
 * Patch 7 just adds some extra paranoia and invariant checking to the
   watermark code.
 * Patches 8 and 9 are the two final patches from the original atomic watermark
   series; with the fixes earlier in the series, we've confirmed that they no
   longer cause regressions on Jani's machine.

[1] http://lists.freedesktop.org/archives/intel-gfx/2015-October/077113.html

Matt Roper (9):
  drm/i915: Disable primary plane if we fail to reconstruct BIOS fb
  drm/i915: Dump in-flight plane state while dumping in-flight CRTC
    state
  drm/i915: Clarify plane state during CRTC state dumps (v2)
  drm/i915: Dump pipe config after initial FB is reconstructed
  drm/i915: Setup clipped src/dest coordinates during FB reconstruction
    (v2)
  drm/i915: Convert hsw_compute_linetime_wm to use in-flight state
  drm/i915: Add extra paranoia to ILK watermark calculations
  drm/i915: Sanitize watermarks after hardware state readout (v2)
  drm/i915: Add two-stage ILK-style watermark programming (v7)

 drivers/gpu/drm/drm_atomic_helper.c  |   1 +
 drivers/gpu/drm/i915/i915_drv.h      |   5 +
 drivers/gpu/drm/i915/intel_atomic.c  |   1 +
 drivers/gpu/drm/i915/intel_display.c | 195 +++++++++++++++++++++++++++++++++--
 drivers/gpu/drm/i915/intel_drv.h     |  31 +++++-
 drivers/gpu/drm/i915/intel_pm.c      | 186 ++++++++++++++++++++++++---------
 6 files changed, 359 insertions(+), 60 deletions(-)

-- 
2.1.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 1/9] drm/i915: Disable primary plane if we fail to reconstruct BIOS fb

[Matt Roper]

If we fail to reconstruct the BIOS fb (e.g., because the FB is too
large), we'll be left with plane state that indicates the primary plane
is visible yet has a NULL fb.  This mismatch causes problems later on
(e.g., for the watermark code).  Since we've failed to reconstruct the
BIOS FB, the best solution is to just disable the primary plane and
pretend the BIOS never had it enabled.

Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: Ville Syrjälä <ville.syrjala@intel.com>
Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
With this patch, the rest of this series now runs without problems on Jani's
system where the regressions were originally reported.

Chris pointed out that this might also fix some of the other bugzillas we have
on older platforms where there's a GPU hang on failed FB takeover.  I don't
think I have any platforms that can reproduce those types of problems to
verify, but he listed candidate bugs as:

        https://bugs.freedesktop.org/show_bug.cgi?id=89319
        https://bugs.freedesktop.org/show_bug.cgi?id=87677
        https://bugs.freedesktop.org/show_bug.cgi?id=89146
        https://bugs.freedesktop.org/show_bug.cgi?id=91653

 drivers/gpu/drm/i915/intel_display.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 4b21d5e..d03a235 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2597,6 +2597,8 @@ intel_find_initial_plane_obj(struct intel_crtc *intel_crtc,
 	struct drm_i915_gem_object *obj;
 	struct drm_plane *primary = intel_crtc->base.primary;
 	struct drm_plane_state *plane_state = primary->state;
+	struct drm_crtc_state *crtc_state = intel_crtc->base.state;
+	struct intel_plane *intel_plane = to_intel_plane(primary);
 	struct drm_framebuffer *fb;
 
 	if (!plane_config->fb)
@@ -2633,6 +2635,17 @@ intel_find_initial_plane_obj(struct intel_crtc *intel_crtc,
 		}
 	}
 
+	/*
+	 * We've failed to reconstruct the BIOS FB.  Current display state
+	 * indicates that the primary plane is visible, but has a NULL FB,
+	 * which will lead to problems later if we don't fix it up.  The
+	 * simplest solution is to just disable the primary plane now and
+	 * pretend the BIOS never had it enabled.
+	 */
+	to_intel_plane_state(plane_state)->visible = false;
+	crtc_state->plane_mask &= ~(1 << drm_plane_index(primary));
+	intel_plane->disable_plane(primary, &intel_crtc->base);
+
 	return;
 
 valid_fb:
-- 
2.1.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/9] drm/i915: Dump in-flight plane state while dumping in-flight CRTC state

[Matt Roper]

The intel_dump_pipe_config() always dumps the currently active plane
state for each plane on a CRTC.  If we're calling this function to dump
CRTC state that is in-flight (not yet active), then this mismatch can be
misleading and confusing.  Let's pay attention to whether the state
we're dumping is part of an in-flight transaction (because
crtc_state->state is non-NULL); if it is, we'll dump the corresponding
in-flight plane state instead of the active state.

Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index d03a235..0e74287 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12162,11 +12162,23 @@ static void intel_dump_pipe_config(struct intel_crtc *crtc,
 
 	DRM_DEBUG_KMS("planes on this crtc\n");
 	list_for_each_entry(plane, &dev->mode_config.plane_list, head) {
+		struct drm_plane_state *ps = NULL;
+
 		intel_plane = to_intel_plane(plane);
 		if (intel_plane->pipe != crtc->pipe)
 			continue;
 
-		state = to_intel_plane_state(plane->state);
+		/* Get in-flight plane state, if any */
+		if (pipe_config->base.state)
+			ps = drm_atomic_get_existing_plane_state(pipe_config->base.state,
+								 plane);
+
+		/* If no in-flight state, use active state instead */
+		if (!ps)
+			ps = plane->state;
+
+		state = to_intel_plane_state(ps);
+
 		fb = state->base.fb;
 		if (!fb) {
 			DRM_DEBUG_KMS("%s PLANE:%d plane: %u.%u idx: %d "
-- 
2.1.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 3/9] drm/i915: Clarify plane state during CRTC state dumps (v2)

[Matt Roper]

During state dumping, list planes that have an FB but are invisible
(e.g., because they're offscreen or clipped by other planes) as "not
visible" rather than "enabled."  While we're at it, dump the FB format
as a human-readable string rather than a hex format code.

v2: Don't add bpp; make format human-readable instead. (Ville)

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 0e74287..e5c522b 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12190,13 +12190,15 @@ static void intel_dump_pipe_config(struct intel_crtc *crtc,
 			continue;
 		}
 
-		DRM_DEBUG_KMS("%s PLANE:%d plane: %u.%u idx: %d enabled",
+		DRM_DEBUG_KMS("%s PLANE:%d plane: %u.%u idx: %d %s",
 			plane->type == DRM_PLANE_TYPE_CURSOR ? "CURSOR" : "STANDARD",
 			plane->base.id, intel_plane->pipe,
 			crtc->base.primary == plane ? 0 : intel_plane->plane + 1,
-			drm_plane_index(plane));
-		DRM_DEBUG_KMS("\tFB:%d, fb = %ux%u format = 0x%x",
-			fb->base.id, fb->width, fb->height, fb->pixel_format);
+			drm_plane_index(plane),
+			state->visible ? "enabled" : "not visible");
+		DRM_DEBUG_KMS("\tFB:%d, fb = %ux%u format = %s",
+			fb->base.id, fb->width, fb->height,
+			drm_get_format_name(fb->pixel_format));
 		DRM_DEBUG_KMS("\tscaler:%d src (%u, %u) %ux%u dst (%u, %u) %ux%u\n",
 			state->scaler_id,
 			state->src.x1 >> 16, state->src.y1 >> 16,
-- 
2.1.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 4/9] drm/i915: Dump pipe config after initial FB is reconstructed

[Matt Roper]

We dump during HW state readout, but that's too early to reflect how the
primary plane is actually configured.

In the future it would be nice if we could just read out HW state and
reconstruct FB's at the same point, but at the moment we don't have GEM
initialized sufficiently during hardware readout.

Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index e5c522b..73e9bf9 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -15228,6 +15228,9 @@ void intel_modeset_init(struct drm_device *dev)
 		 * just get the first one.
 		 */
 		intel_find_initial_plane_obj(crtc, &plane_config);
+
+		intel_dump_pipe_config(crtc, crtc->config,
+				       "[state after init fb reconstruction]");
 	}
 }
 
-- 
2.1.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 5/9] drm/i915: Setup clipped src/dest coordinates during FB reconstruction (v2)

[Matt Roper]

Plane state objects contain two copies of src/dest coordinates:  the
original (requested by userspace) coordinates in the base
drm_plane_state object, and a second, clipped copy (i.e., what we
actually want to program to the hardware) in intel_plane_state.  We've
only been setting up the former set of values during boot time FB
reconstruction, but we should really be initializing both.

Note that the code here probably still needs some more work since we
make a lot of assumptions about how the BIOS programmed the hardware
that may not always be true, especially on gen9+; e.g.,
 * Primary plane might not be positioned at 0,0
 * Primary plane could have been rotated by the BIOS
 * Primary plane might be scaled
 * The BIOS fb might be a single "extended mode" FB that spans
   multiple displays.
 * ...etc...

v2: Reword/expand commit message description of assumptions we make

Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
Reviewed-by(v1): Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 73e9bf9..00e4c37 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2599,6 +2599,8 @@ intel_find_initial_plane_obj(struct intel_crtc *intel_crtc,
 	struct drm_plane_state *plane_state = primary->state;
 	struct drm_crtc_state *crtc_state = intel_crtc->base.state;
 	struct intel_plane *intel_plane = to_intel_plane(primary);
+	struct intel_plane_state *intel_state =
+		to_intel_plane_state(plane_state);
 	struct drm_framebuffer *fb;
 
 	if (!plane_config->fb)
@@ -2659,6 +2661,15 @@ valid_fb:
 	plane_state->crtc_w = fb->width;
 	plane_state->crtc_h = fb->height;
 
+	intel_state->src.x1 = plane_state->src_x;
+	intel_state->src.y1 = plane_state->src_y;
+	intel_state->src.x2 = plane_state->src_x + plane_state->src_w;
+	intel_state->src.y2 = plane_state->src_y + plane_state->src_h;
+	intel_state->dst.x1 = plane_state->crtc_x;
+	intel_state->dst.y1 = plane_state->crtc_y;
+	intel_state->dst.x2 = plane_state->crtc_x + plane_state->crtc_w;
+	intel_state->dst.y2 = plane_state->crtc_y + plane_state->crtc_h;
+
 	obj = intel_fb_obj(fb);
 	if (obj->tiling_mode != I915_TILING_NONE)
 		dev_priv->preserve_bios_swizzle = true;
-- 
2.1.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 6/9] drm/i915: Convert hsw_compute_linetime_wm to use in-flight state

[Matt Roper]

When watermark calculation was moved up to the atomic check phase, the
code was updated to calculate based on in-flight atomic state rather
than already-committed state.  However the hsw_compute_linetime_wm()
didn't get updated and continued to pull values out of the
currently-committed CRTC state.  On platforms that call this function
(HSW/BDW only), this will cause problems when we go to enable the CRTC
since we'll pull the current mode (off) rather than the mode we're
calculating for and wind up with a divide by zero error.

This was an oversight in commit:

        commit a28170f3389f4e42db95e595b0d86384a79de696
        Author: Matt Roper <matthew.d.roper@intel.com>
        Date:   Thu Sep 24 15:53:16 2015 -0700

            drm/i915: Calculate ILK-style watermarks during atomic check (v3)

Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 96f45d7..6efb908 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -1990,14 +1990,19 @@ static void ilk_compute_wm_level(const struct drm_i915_private *dev_priv,
 }
 
 static uint32_t
-hsw_compute_linetime_wm(struct drm_device *dev, struct drm_crtc *crtc)
+hsw_compute_linetime_wm(struct drm_device *dev,
+			struct intel_crtc_state *cstate)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-	const struct drm_display_mode *adjusted_mode = &intel_crtc->config->base.adjusted_mode;
+	const struct drm_display_mode *adjusted_mode =
+		&cstate->base.adjusted_mode;
 	u32 linetime, ips_linetime;
 
-	if (!intel_crtc->active)
+	if (!cstate->base.active)
+		return 0;
+	if (WARN_ON(adjusted_mode->crtc_clock == 0))
+		return 0;
+	if (WARN_ON(dev_priv->cdclk_freq == 0))
 		return 0;
 
 	/* The WM are computed with base on how long it takes to fill a single
@@ -2305,8 +2310,7 @@ static int ilk_compute_pipe_wm(struct intel_crtc *intel_crtc,
 			     pristate, sprstate, curstate, &pipe_wm->wm[0]);
 
 	if (IS_HASWELL(dev) || IS_BROADWELL(dev))
-		pipe_wm->linetime = hsw_compute_linetime_wm(dev,
-							    &intel_crtc->base);
+		pipe_wm->linetime = hsw_compute_linetime_wm(dev, cstate);
 
 	/* LP0 watermarks always use 1/2 DDB partitioning */
 	ilk_compute_wm_maximums(dev, 0, &config, INTEL_DDB_PART_1_2, &max);
-- 
2.1.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 7/9] drm/i915: Add extra paranoia to ILK watermark calculations

[Matt Roper]

Our low-level watermark calculation functions don't get called when the
CRTC is disabled or the relevant plane is invisible, so they should
never see a zero htotal or zero bpp.  However add some checks to ensure
this is true so that we don't wind up dividing by zero if we make a
mistake elsewhere in the driver (which the atomic watermark series has
revealed we might be).

References: http://lists.freedesktop.org/archives/intel-gfx/2015-October/077370.html
Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 6efb908..d4cd5d5 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -1664,6 +1664,9 @@ uint32_t ilk_pipe_pixel_rate(const struct intel_crtc_state *pipe_config)
 		if (pipe_h < pfit_h)
 			pipe_h = pfit_h;
 
+		if (WARN_ON(!pfit_w || !pfit_h))
+			return pixel_rate;
+
 		pixel_rate = div_u64((uint64_t) pixel_rate * pipe_w * pipe_h,
 				     pfit_w * pfit_h);
 	}
@@ -1695,6 +1698,8 @@ static uint32_t ilk_wm_method2(uint32_t pixel_rate, uint32_t pipe_htotal,
 
 	if (WARN(latency == 0, "Latency value missing\n"))
 		return UINT_MAX;
+	if (WARN_ON(!pipe_htotal))
+		return UINT_MAX;
 
 	ret = (latency * pixel_rate) / (pipe_htotal * 10000);
 	ret = (ret + 1) * horiz_pixels * bytes_per_pixel;
@@ -1705,6 +1710,17 @@ static uint32_t ilk_wm_method2(uint32_t pixel_rate, uint32_t pipe_htotal,
 static uint32_t ilk_wm_fbc(uint32_t pri_val, uint32_t horiz_pixels,
 			   uint8_t bytes_per_pixel)
 {
+	/*
+	 * Neither of these should be possible since this function shouldn't be
+	 * called if the CRTC is off or the plane is invisible.  But let's be
+	 * extra paranoid to avoid a potential divide-by-zero if we screw up
+	 * elsewhere in the driver.
+	 */
+	if (WARN_ON(!bytes_per_pixel))
+		return 0;
+	if (WARN_ON(!horiz_pixels))
+		return 0;
+
 	return DIV_ROUND_UP(pri_val * 64, horiz_pixels * bytes_per_pixel) + 2;
 }
 
-- 
2.1.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 8/9] drm/i915: Sanitize watermarks after hardware state readout (v2)

[Matt Roper]

Although we can do a good job of reading out hardware state, the
graphics firmware may have programmed the watermarks in a creative way
that doesn't match how i915 would have chosen to program them.  We
shouldn't trust the firmware's watermark programming, but should rather
re-calculate how we think WM's should be programmed and then shove those
values into the hardware.

We can do this pretty easily by creating a dummy top-level state,
running it through the check process to calculate all the values, and
then just programming the watermarks for each CRTC.

v2:  Move watermark sanitization after our BIOS fb reconstruction; the
     watermark calculations that we do here need to look at pstate->fb,
     which isn't setup yet in intel_modeset_setup_hw_state(), even
     though we have an enabled & visible plane.

Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/drm_atomic_helper.c  |  1 +
 drivers/gpu/drm/i915/i915_drv.h      |  1 +
 drivers/gpu/drm/i915/intel_display.c | 58 ++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_pm.c      | 14 +++++----
 4 files changed, 68 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 3731a26..8a98e0c 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -2478,6 +2478,7 @@ drm_atomic_helper_duplicate_state(struct drm_device *dev,
 		}
 	}
 
+	drm_modeset_lock(&dev->mode_config.connection_mutex, ctx);
 	drm_for_each_connector(conn, dev) {
 		struct drm_connector_state *conn_state;
 
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 11ae5a5..5172604 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -630,6 +630,7 @@ struct drm_i915_display_funcs {
 			  struct dpll *best_clock);
 	int (*compute_pipe_wm)(struct intel_crtc *crtc,
 			       struct drm_atomic_state *state);
+	void (*program_watermarks)(struct intel_crtc_state *cstate);
 	void (*update_wm)(struct drm_crtc *crtc);
 	int (*modeset_calc_cdclk)(struct drm_atomic_state *state);
 	void (*modeset_commit_cdclk)(struct drm_atomic_state *state);
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 00e4c37..eb52afa 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -15120,6 +15120,57 @@ void intel_modeset_init_hw(struct drm_device *dev)
 	intel_enable_gt_powersave(dev);
 }
 
+/*
+ * Calculate what we think the watermarks should be for the state we've read
+ * out of the hardware and then immediately program those watermarks so that
+ * we ensure the hardware settings match our internal state.
+ */
+static void sanitize_watermarks(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = to_i915(dev);
+	struct drm_atomic_state *state;
+	struct drm_crtc *crtc;
+	struct drm_crtc_state *cstate;
+	struct drm_modeset_acquire_ctx ctx;
+	int ret;
+	int i;
+
+	/* Only supported on platforms that use atomic watermark design */
+	if (!dev_priv->display.program_watermarks)
+		return;
+
+	/*
+	 * Calculate what we think WM's should be by creating a dummy state and
+	 * running it through the atomic check code.
+	 */
+	drm_modeset_acquire_init(&ctx, 0);
+	state = drm_atomic_helper_duplicate_state(dev, &ctx);
+	if (WARN_ON(IS_ERR(state)))
+		return;
+
+	ret = intel_atomic_check(dev, state);
+	if (ret) {
+		/*
+		 * Just give up and leave watermarks untouched if we get an
+		 * error back from 'check'
+		 */
+		DRM_DEBUG_KMS("Could not determine valid watermarks for inherited state\n");
+		return;
+	}
+
+	/* Write calculated watermark values back */
+	to_i915(dev)->wm.config = to_intel_atomic_state(state)->wm_config;
+	for_each_crtc_in_state(state, crtc, cstate, i) {
+		struct intel_crtc_state *cs = to_intel_crtc_state(cstate);
+
+		dev_priv->display.program_watermarks(cs);
+	}
+
+	drm_atomic_state_free(state);
+	drm_modeset_drop_locks(&ctx);
+	drm_modeset_acquire_fini(&ctx);
+}
+
 void intel_modeset_init(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -15243,6 +15294,13 @@ void intel_modeset_init(struct drm_device *dev)
 		intel_dump_pipe_config(crtc, crtc->config,
 				       "[state after init fb reconstruction]");
 	}
+
+	/*
+	 * Make sure hardware watermarks really match the state we read out.
+	 * Note that we need to do this after reconstructing the BIOS fb's
+	 * since the watermark calculation done here will use pstate->fb.
+	 */
+	sanitize_watermarks(dev);
 }
 
 static void intel_enable_pipe_a(struct drm_device *dev)
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index d4cd5d5..c209a69 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3610,15 +3610,19 @@ static void skl_update_wm(struct drm_crtc *crtc)
 	dev_priv->wm.skl_hw = *results;
 }
 
-static void ilk_program_watermarks(struct drm_i915_private *dev_priv)
+static void ilk_program_watermarks(struct intel_crtc_state *cstate)
 {
-	struct drm_device *dev = dev_priv->dev;
+	struct drm_crtc *crtc = cstate->base.crtc;
+	struct drm_device *dev = crtc->dev;
+	struct drm_i915_private *dev_priv = to_i915(dev);
 	struct intel_pipe_wm lp_wm_1_2 = {}, lp_wm_5_6 = {}, *best_lp_wm;
 	struct ilk_wm_maximums max;
 	struct intel_wm_config *config = &dev_priv->wm.config;
 	struct ilk_wm_values results = {};
 	enum intel_ddb_partitioning partitioning;
 
+	to_intel_crtc(crtc)->wm.active.ilk = cstate->wm.optimal.ilk;
+
 	ilk_compute_wm_maximums(dev, 1, config, INTEL_DDB_PART_1_2, &max);
 	ilk_wm_merge(dev, config, &max, &lp_wm_1_2);
 
@@ -3643,7 +3647,6 @@ static void ilk_program_watermarks(struct drm_i915_private *dev_priv)
 
 static void ilk_update_wm(struct drm_crtc *crtc)
 {
-	struct drm_i915_private *dev_priv = to_i915(crtc->dev);
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	struct intel_crtc_state *cstate = to_intel_crtc_state(crtc->state);
 
@@ -3661,9 +3664,7 @@ static void ilk_update_wm(struct drm_crtc *crtc)
 		intel_wait_for_vblank(crtc->dev, intel_crtc->pipe);
 	}
 
-	intel_crtc->wm.active.ilk = cstate->wm.optimal.ilk;
-
-	ilk_program_watermarks(dev_priv);
+	ilk_program_watermarks(cstate);
 }
 
 static void skl_pipe_wm_active_state(uint32_t val,
@@ -6971,6 +6972,7 @@ void intel_init_pm(struct drm_device *dev)
 		     dev_priv->wm.spr_latency[0] && dev_priv->wm.cur_latency[0])) {
 			dev_priv->display.update_wm = ilk_update_wm;
 			dev_priv->display.compute_pipe_wm = ilk_compute_pipe_wm;
+			dev_priv->display.program_watermarks = ilk_program_watermarks;
 		} else {
 			DRM_DEBUG_KMS("Failed to read display plane latency. "
 				      "Disable CxSR\n");
-- 
2.1.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 9/9] drm/i915: Add two-stage ILK-style watermark programming (v7)

[Matt Roper]

In addition to calculating final watermarks, let's also pre-calculate a
set of intermediate watermark values at atomic check time.  These
intermediate watermarks are a combination of the watermarks for the old
state and the new state; they should satisfy the requirements of both
states which means they can be programmed immediately when we commit the
atomic state (without waiting for a vblank).  Once the vblank does
happen, we can then re-program watermarks to the more optimal final
value.

v2: Significant rebasing/rewriting.

v3:
 - Move 'need_postvbl_update' flag to CRTC state (Daniel)
 - Don't forget to check intermediate watermark values for validity
   (Maarten)
 - Don't due async watermark optimization; just do it at the end of the
   atomic transaction, after waiting for vblanks.  We do want it to be
   async eventually, but adding that now will cause more trouble for
   Maarten's in-progress work.  (Maarten)
 - Don't allocate space in crtc_state for intermediate watermarks on
   platforms that don't need it (gen9+).
 - Move WaCxSRDisabledForSpriteScaling:ivb into intel_begin_crtc_commit
   now that ilk_update_wm is gone.

v4:
 - Add a wm_mutex to cover updates to intel_crtc->active and the
   need_postvbl_update flag.  Since we don't have async yet it isn't
   terribly important yet, but might as well add it now.
 - Change interface to program watermarks.  Platforms will now expose
   .initial_watermarks() and .optimize_watermarks() functions to do
   watermark programming.  These should lock wm_mutex, copy the
   appropriate state values into intel_crtc->active, and then call
   the internal program watermarks function.

v5:
 - Skip intermediate watermark calculation/check during initial hardware
   readout since we don't trust the existing HW values (and don't have
   valid values of our own yet).
 - Don't try to call .optimize_watermarks() on platforms that don't have
   atomic watermarks yet.  (Maarten)

v6:
 - Rebase

v7:
 - Further rebase

Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
Reviewed-by(v5): Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h      |   6 +-
 drivers/gpu/drm/i915/intel_atomic.c  |   1 +
 drivers/gpu/drm/i915/intel_display.c |  90 +++++++++++++++++++-
 drivers/gpu/drm/i915/intel_drv.h     |  31 ++++++-
 drivers/gpu/drm/i915/intel_pm.c      | 160 ++++++++++++++++++++++++-----------
 5 files changed, 232 insertions(+), 56 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 5172604..427b488 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -630,7 +630,11 @@ struct drm_i915_display_funcs {
 			  struct dpll *best_clock);
 	int (*compute_pipe_wm)(struct intel_crtc *crtc,
 			       struct drm_atomic_state *state);
-	void (*program_watermarks)(struct intel_crtc_state *cstate);
+	int (*compute_intermediate_wm)(struct drm_device *dev,
+				       struct intel_crtc *intel_crtc,
+				       struct intel_crtc_state *newstate);
+	void (*initial_watermarks)(struct intel_crtc_state *cstate);
+	void (*optimize_watermarks)(struct intel_crtc_state *cstate);
 	void (*update_wm)(struct drm_crtc *crtc);
 	int (*modeset_calc_cdclk)(struct drm_atomic_state *state);
 	void (*modeset_commit_cdclk)(struct drm_atomic_state *state);
diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
index 643f342..b91e166 100644
--- a/drivers/gpu/drm/i915/intel_atomic.c
+++ b/drivers/gpu/drm/i915/intel_atomic.c
@@ -95,6 +95,7 @@ intel_crtc_duplicate_state(struct drm_crtc *crtc)
 
 	crtc_state->update_pipe = false;
 	crtc_state->disable_lp_wm = false;
+	crtc_state->wm.need_postvbl_update = false;
 
 	return &crtc_state->base;
 }
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index eb52afa..8db0486 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11801,6 +11801,12 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
 		intel_crtc->atomic.update_wm_pre = true;
 	}
 
+	/* Pre-gen9 platforms need two-step watermark updates */
+	if ((intel_crtc->atomic.update_wm_pre || intel_crtc->atomic.update_wm_post) &&
+	    INTEL_INFO(dev)->gen < 9 &&
+	    dev_priv->display.optimize_watermarks)
+		to_intel_crtc_state(crtc_state)->wm.need_postvbl_update = true;
+
 	if (visible || was_visible)
 		intel_crtc->atomic.fb_bits |=
 			to_intel_plane(plane)->frontbuffer_bit;
@@ -11957,8 +11963,29 @@ static int intel_crtc_atomic_check(struct drm_crtc *crtc,
 	ret = 0;
 	if (dev_priv->display.compute_pipe_wm) {
 		ret = dev_priv->display.compute_pipe_wm(intel_crtc, state);
-		if (ret)
+		if (ret) {
+			DRM_DEBUG_KMS("Target pipe watermarks are invalid\n");
 			return ret;
+		}
+	}
+
+	if (dev_priv->display.compute_intermediate_wm &&
+	    !to_intel_atomic_state(state)->skip_intermediate_wm) {
+		if (WARN_ON(!dev_priv->display.compute_pipe_wm))
+			return 0;
+
+		/*
+		 * Calculate 'intermediate' watermarks that satisfy both the
+		 * old state and the new state.  We can program these
+		 * immediately.
+		 */
+		ret = dev_priv->display.compute_intermediate_wm(crtc->dev,
+								intel_crtc,
+								pipe_config);
+		if (ret) {
+			DRM_DEBUG_KMS("No valid intermediate pipe watermarks are possible\n");
+			return ret;
+		}
 	}
 
 	if (INTEL_INFO(dev)->gen >= 9) {
@@ -13409,6 +13436,7 @@ static int intel_atomic_commit(struct drm_device *dev,
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_crtc_state *crtc_state;
 	struct drm_crtc *crtc;
+	struct intel_crtc_state *intel_cstate;
 	int ret = 0;
 	int i;
 	bool any_ms = false;
@@ -13500,6 +13528,20 @@ static int intel_atomic_commit(struct drm_device *dev,
 
 	drm_atomic_helper_wait_for_vblanks(dev, state);
 
+	/*
+	 * Now that the vblank has passed, we can go ahead and program the
+	 * optimal watermarks on platforms that need two-step watermark
+	 * programming.
+	 *
+	 * TODO: Move this (and other cleanup) to an async worker eventually.
+	 */
+	for_each_crtc_in_state(state, crtc, crtc_state, i) {
+		intel_cstate = to_intel_crtc_state(crtc->state);
+
+		if (dev_priv->display.optimize_watermarks)
+		    dev_priv->display.optimize_watermarks(intel_cstate);
+	}
+
 	mutex_lock(&dev->struct_mutex);
 	drm_atomic_helper_cleanup_planes(dev, state);
 	mutex_unlock(&dev->struct_mutex);
@@ -13862,13 +13904,44 @@ 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 *cstate = to_intel_crtc_state(crtc->state);
 	struct intel_crtc_state *old_intel_state =
 		to_intel_crtc_state(old_crtc_state);
 	bool modeset = needs_modeset(crtc->state);
 
-	if (intel_crtc->atomic.update_wm_pre)
+	/*
+	 * IVB workaround: must disable low power watermarks for at least
+	 * one frame before enabling scaling.  LP watermarks can be re-enabled
+	 * when scaling is disabled.
+	 *
+	 * WaCxSRDisabledForSpriteScaling:ivb
+	 */
+	if (cstate->disable_lp_wm) {
+		ilk_disable_lp_wm(crtc->dev);
+		intel_wait_for_vblank(crtc->dev, intel_crtc->pipe);
+	}
+
+	/*
+	 * For platforms that support atomic watermarks, program the
+	 * 'intermediate' watermarks immediately.  On pre-gen9 platforms, these
+	 * will be the intermediate values that are safe for both pre- and
+	 * post- vblank; when vblank happens, the 'active' values will be set
+	 * to the final 'target' values and we'll do this again to get the
+	 * optimal watermarks.  For gen9+ platforms, the values we program here
+	 * will be the final target values which will get automatically latched
+	 * at vblank time; no further programming will be necessary.
+	 *
+	 * If a platform hasn't been transitioned to atomic watermarks yet,
+	 * we'll continue to update watermarks the old way, if flags tell
+	 * us to.
+	 */
+	if (dev_priv->display.initial_watermarks != NULL) {
+		dev_priv->display.initial_watermarks(cstate);
+	} else if (intel_crtc->atomic.update_wm_pre) {
 		intel_update_watermarks(crtc);
+	}
 
 	/* Perform vblank evasion around commit operation */
 	intel_pipe_update_start(intel_crtc);
@@ -14207,6 +14280,7 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
 	intel_crtc->cursor_size = ~0;
 
 	intel_crtc->wm.cxsr_allowed = true;
+	mutex_init(&intel_crtc->wm.wm_mutex);
 
 	BUG_ON(pipe >= ARRAY_SIZE(dev_priv->plane_to_crtc_mapping) ||
 	       dev_priv->plane_to_crtc_mapping[intel_crtc->plane] != NULL);
@@ -15136,7 +15210,7 @@ static void sanitize_watermarks(struct drm_device *dev)
 	int i;
 
 	/* Only supported on platforms that use atomic watermark design */
-	if (!dev_priv->display.program_watermarks)
+	if (!dev_priv->display.optimize_watermarks)
 		return;
 
 	/*
@@ -15148,6 +15222,13 @@ static void sanitize_watermarks(struct drm_device *dev)
 	if (WARN_ON(IS_ERR(state)))
 		return;
 
+	/*
+	 * Hardware readout is the only time we don't want to calculate
+	 * intermediate watermarks (since we don't trust the current
+	 * watermarks).
+	 */
+	to_intel_atomic_state(state)->skip_intermediate_wm = true;
+
 	ret = intel_atomic_check(dev, state);
 	if (ret) {
 		/*
@@ -15163,7 +15244,8 @@ static void sanitize_watermarks(struct drm_device *dev)
 	for_each_crtc_in_state(state, crtc, cstate, i) {
 		struct intel_crtc_state *cs = to_intel_crtc_state(cstate);
 
-		dev_priv->display.program_watermarks(cs);
+		cs->wm.need_postvbl_update = true;
+		dev_priv->display.optimize_watermarks(cs);
 	}
 
 	drm_atomic_state_free(state);
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 8fae824..c53eb10 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -249,6 +249,12 @@ struct intel_atomic_state {
 	bool dpll_set;
 	struct intel_shared_dpll_config shared_dpll[I915_NUM_PLLS];
 	struct intel_wm_config wm_config;
+
+	/*
+	 * Current watermarks can't be trusted during hardware readout, so
+	 * don't bother calculating intermediate watermarks.
+	 */
+	bool skip_intermediate_wm;
 };
 
 struct intel_plane_state {
@@ -491,13 +497,29 @@ struct intel_crtc_state {
 
 	struct {
 		/*
-		 * optimal watermarks, programmed post-vblank when this state
-		 * is committed
+		 * Optimal watermarks, programmed post-vblank when this state
+		 * is committed.
 		 */
 		union {
 			struct intel_pipe_wm ilk;
 			struct skl_pipe_wm skl;
 		} optimal;
+
+		/*
+		 * Intermediate watermarks; these can be programmed immediately
+		 * since they satisfy both the current configuration we're
+		 * switching away from and the new configuration we're switching
+		 * to.
+		 */
+		struct intel_pipe_wm intermediate;
+
+		/*
+		 * Platforms with two-step watermark programming will need to
+		 * update watermark programming post-vblank to switch from the
+		 * safe intermediate watermarks to the optimal final
+		 * watermarks.
+		 */
+		bool need_postvbl_update;
 	} wm;
 };
 
@@ -587,8 +609,12 @@ struct intel_crtc {
 			struct intel_pipe_wm ilk;
 			struct skl_pipe_wm skl;
 		} active;
+
 		/* allow CxSR on this pipe */
 		bool cxsr_allowed;
+
+		/* Protects active and need_postvbl_update */
+		struct mutex wm_mutex;
 	} wm;
 
 	int scanline_offset;
@@ -1463,6 +1489,7 @@ void skl_wm_get_hw_state(struct drm_device *dev);
 void skl_ddb_get_hw_state(struct drm_i915_private *dev_priv,
 			  struct skl_ddb_allocation *ddb /* out */);
 uint32_t ilk_pipe_pixel_rate(const struct intel_crtc_state *pipe_config);
+bool ilk_disable_lp_wm(struct drm_device *dev);
 
 /* intel_sdvo.c */
 bool intel_sdvo_init(struct drm_device *dev,
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index c209a69..29d725e 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -2265,6 +2265,29 @@ static void skl_setup_wm_latency(struct drm_device *dev)
 	intel_print_wm_latency(dev, "Gen9 Plane", dev_priv->wm.skl_latency);
 }
 
+static bool ilk_validate_pipe_wm(struct drm_device *dev,
+				 struct intel_pipe_wm *pipe_wm)
+{
+	/* LP0 watermark maximums depend on this pipe alone */
+	const struct intel_wm_config config = {
+		.num_pipes_active = 1,
+		.sprites_enabled = pipe_wm->sprites_enabled,
+		.sprites_scaled = pipe_wm->sprites_scaled,
+	};
+	struct ilk_wm_maximums max;
+
+	/* LP0 watermarks always use 1/2 DDB partitioning */
+	ilk_compute_wm_maximums(dev, 0, &config, INTEL_DDB_PART_1_2, &max);
+
+	/* At least LP0 must be valid */
+	if (!ilk_validate_wm_level(0, &max, &pipe_wm->wm[0])) {
+		DRM_DEBUG_KMS("LP0 watermark invalid\n");
+		return false;
+	}
+
+	return true;
+}
+
 /* Compute new watermarks for the pipe */
 static int ilk_compute_pipe_wm(struct intel_crtc *intel_crtc,
 			       struct drm_atomic_state *state)
@@ -2279,10 +2302,6 @@ static int ilk_compute_pipe_wm(struct intel_crtc *intel_crtc,
 	struct intel_plane_state *sprstate = NULL;
 	struct intel_plane_state *curstate = NULL;
 	int level, max_level = ilk_wm_max_level(dev);
-	/* LP0 watermark maximums depend on this pipe alone */
-	struct intel_wm_config config = {
-		.num_pipes_active = 1,
-	};
 	struct ilk_wm_maximums max;
 
 	cstate = intel_atomic_get_crtc_state(state, intel_crtc);
@@ -2305,21 +2324,18 @@ static int ilk_compute_pipe_wm(struct intel_crtc *intel_crtc,
 			curstate = to_intel_plane_state(ps);
 	}
 
-	config.sprites_enabled = sprstate->visible;
-	config.sprites_scaled = sprstate->visible &&
+	pipe_wm->pipe_enabled = cstate->base.active;
+	pipe_wm->sprites_enabled = sprstate->visible;
+	pipe_wm->sprites_scaled = sprstate->visible &&
 		(drm_rect_width(&sprstate->dst) != drm_rect_width(&sprstate->src) >> 16 ||
 		drm_rect_height(&sprstate->dst) != drm_rect_height(&sprstate->src) >> 16);
 
-	pipe_wm->pipe_enabled = cstate->base.active;
-	pipe_wm->sprites_enabled = config.sprites_enabled;
-	pipe_wm->sprites_scaled = config.sprites_scaled;
-
 	/* ILK/SNB: LP2+ watermarks only w/o sprites */
 	if (INTEL_INFO(dev)->gen <= 6 && sprstate->visible)
 		max_level = 1;
 
 	/* ILK/SNB/IVB: LP1+ watermarks only w/o scaling */
-	if (config.sprites_scaled)
+	if (pipe_wm->sprites_scaled)
 		max_level = 0;
 
 	ilk_compute_wm_level(dev_priv, intel_crtc, 0, cstate,
@@ -2328,12 +2344,8 @@ static int ilk_compute_pipe_wm(struct intel_crtc *intel_crtc,
 	if (IS_HASWELL(dev) || IS_BROADWELL(dev))
 		pipe_wm->linetime = hsw_compute_linetime_wm(dev, cstate);
 
-	/* LP0 watermarks always use 1/2 DDB partitioning */
-	ilk_compute_wm_maximums(dev, 0, &config, INTEL_DDB_PART_1_2, &max);
-
-	/* At least LP0 must be valid */
-	if (!ilk_validate_wm_level(0, &max, &pipe_wm->wm[0]))
-		return -EINVAL;
+	if (!ilk_validate_pipe_wm(dev, pipe_wm))
+		return false;
 
 	ilk_compute_wm_reg_maximums(dev, 1, &max);
 
@@ -2358,6 +2370,59 @@ static int ilk_compute_pipe_wm(struct intel_crtc *intel_crtc,
 }
 
 /*
+ * Build a set of 'intermediate' watermark values that satisfy both the old
+ * state and the new state.  These can be programmed to the hardware
+ * immediately.
+ */
+static int ilk_compute_intermediate_wm(struct drm_device *dev,
+				       struct intel_crtc *intel_crtc,
+				       struct intel_crtc_state *newstate)
+{
+	struct intel_pipe_wm *a = &newstate->wm.intermediate;
+	struct intel_pipe_wm *b = &intel_crtc->wm.active.ilk;
+	int level, max_level = ilk_wm_max_level(dev);
+
+	/*
+	 * Start with the final, target watermarks, then combine with the
+	 * currently active watermarks to get values that are safe both before
+	 * and after the vblank.
+	 */
+	*a = newstate->wm.optimal.ilk;
+	a->pipe_enabled |= b->pipe_enabled;
+	a->sprites_enabled |= b->sprites_enabled;
+	a->sprites_scaled |= b->sprites_scaled;
+
+	for (level = 0; level <= max_level; level++) {
+		struct intel_wm_level *a_wm = &a->wm[level];
+		const struct intel_wm_level *b_wm = &b->wm[level];
+
+		a_wm->enable &= b_wm->enable;
+		a_wm->pri_val = max(a_wm->pri_val, b_wm->pri_val);
+		a_wm->spr_val = max(a_wm->spr_val, b_wm->spr_val);
+		a_wm->cur_val = max(a_wm->cur_val, b_wm->cur_val);
+		a_wm->fbc_val = max(a_wm->fbc_val, b_wm->fbc_val);
+	}
+
+	/*
+	 * We need to make sure that these merged watermark values are
+	 * actually a valid configuration themselves.  If they're not,
+	 * there's no safe way to transition from the old state to
+	 * the new state, so we need to fail the atomic transaction.
+	 */
+	if (!ilk_validate_pipe_wm(dev, a))
+		return -EINVAL;
+
+	/*
+	 * If our intermediate WM are identical to the final WM, then we can
+	 * omit the post-vblank programming; only update if it's different.
+	 */
+	if (memcmp(a, &newstate->wm.optimal.ilk, sizeof *a) != 0)
+		newstate->wm.need_postvbl_update = false;
+
+	return 0;
+}
+
+/*
  * Merge the watermarks from all active pipes for a specific level.
  */
 static void ilk_merge_wm_level(struct drm_device *dev,
@@ -2369,9 +2434,7 @@ static void ilk_merge_wm_level(struct drm_device *dev,
 	ret_wm->enable = true;
 
 	for_each_intel_crtc(dev, intel_crtc) {
-		const struct intel_crtc_state *cstate =
-			to_intel_crtc_state(intel_crtc->base.state);
-		const struct intel_pipe_wm *active = &cstate->wm.optimal.ilk;
+		const struct intel_pipe_wm *active = &intel_crtc->wm.active.ilk;
 		const struct intel_wm_level *wm = &active->wm[level];
 
 		if (!active->pipe_enabled)
@@ -2519,15 +2582,13 @@ static void ilk_compute_wm_results(struct drm_device *dev,
 
 	/* LP0 register values */
 	for_each_intel_crtc(dev, intel_crtc) {
-		const struct intel_crtc_state *cstate =
-			to_intel_crtc_state(intel_crtc->base.state);
 		enum pipe pipe = intel_crtc->pipe;
-		const struct intel_wm_level *r = &cstate->wm.optimal.ilk.wm[0];
+		const struct intel_wm_level *r = &intel_crtc->wm.active.ilk.wm[0];
 
 		if (WARN_ON(!r->enable))
 			continue;
 
-		results->wm_linetime[pipe] = cstate->wm.optimal.ilk.linetime;
+		results->wm_linetime[pipe] = intel_crtc->wm.active.ilk.linetime;
 
 		results->wm_pipe[pipe] =
 			(r->pri_val << WM0_PIPE_PLANE_SHIFT) |
@@ -2734,7 +2795,7 @@ static void ilk_write_wm_values(struct drm_i915_private *dev_priv,
 	dev_priv->wm.hw = *results;
 }
 
-static bool ilk_disable_lp_wm(struct drm_device *dev)
+bool ilk_disable_lp_wm(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
@@ -3610,19 +3671,15 @@ static void skl_update_wm(struct drm_crtc *crtc)
 	dev_priv->wm.skl_hw = *results;
 }
 
-static void ilk_program_watermarks(struct intel_crtc_state *cstate)
+static void ilk_program_watermarks(struct drm_i915_private *dev_priv)
 {
-	struct drm_crtc *crtc = cstate->base.crtc;
-	struct drm_device *dev = crtc->dev;
-	struct drm_i915_private *dev_priv = to_i915(dev);
+	struct drm_device *dev = dev_priv->dev;
 	struct intel_pipe_wm lp_wm_1_2 = {}, lp_wm_5_6 = {}, *best_lp_wm;
 	struct ilk_wm_maximums max;
 	struct intel_wm_config *config = &dev_priv->wm.config;
 	struct ilk_wm_values results = {};
 	enum intel_ddb_partitioning partitioning;
 
-	to_intel_crtc(crtc)->wm.active.ilk = cstate->wm.optimal.ilk;
-
 	ilk_compute_wm_maximums(dev, 1, config, INTEL_DDB_PART_1_2, &max);
 	ilk_wm_merge(dev, config, &max, &lp_wm_1_2);
 
@@ -3645,26 +3702,29 @@ static void ilk_program_watermarks(struct intel_crtc_state *cstate)
 	ilk_write_wm_values(dev_priv, &results);
 }
 
-static void ilk_update_wm(struct drm_crtc *crtc)
+static void ilk_initial_watermarks(struct intel_crtc_state *cstate)
 {
-	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-	struct intel_crtc_state *cstate = to_intel_crtc_state(crtc->state);
+	struct drm_i915_private *dev_priv = to_i915(cstate->base.crtc->dev);
+	struct intel_crtc *intel_crtc = to_intel_crtc(cstate->base.crtc);
 
-	WARN_ON(cstate->base.active != intel_crtc->active);
+	mutex_lock(&intel_crtc->wm.wm_mutex);
+	intel_crtc->wm.active.ilk = cstate->wm.intermediate;
+	ilk_program_watermarks(dev_priv);
+	mutex_unlock(&intel_crtc->wm.wm_mutex);
+}
 
-	/*
-	 * IVB workaround: must disable low power watermarks for at least
-	 * one frame before enabling scaling.  LP watermarks can be re-enabled
-	 * when scaling is disabled.
-	 *
-	 * WaCxSRDisabledForSpriteScaling:ivb
-	 */
-	if (cstate->disable_lp_wm) {
-		ilk_disable_lp_wm(crtc->dev);
-		intel_wait_for_vblank(crtc->dev, intel_crtc->pipe);
-	}
+static void ilk_optimize_watermarks(struct intel_crtc_state *cstate)
+{
+	struct drm_i915_private *dev_priv = to_i915(cstate->base.crtc->dev);
+	struct intel_crtc *intel_crtc = to_intel_crtc(cstate->base.crtc);
 
-	ilk_program_watermarks(cstate);
+	mutex_lock(&intel_crtc->wm.wm_mutex);
+	if (cstate->wm.need_postvbl_update) {
+		intel_crtc->wm.active.ilk = cstate->wm.optimal.ilk;
+		ilk_program_watermarks(dev_priv);
+		cstate->wm.need_postvbl_update = false;
+	}
+	mutex_unlock(&intel_crtc->wm.wm_mutex);
 }
 
 static void skl_pipe_wm_active_state(uint32_t val,
@@ -6970,9 +7030,11 @@ void intel_init_pm(struct drm_device *dev)
 		     dev_priv->wm.spr_latency[1] && dev_priv->wm.cur_latency[1]) ||
 		    (!IS_GEN5(dev) && dev_priv->wm.pri_latency[0] &&
 		     dev_priv->wm.spr_latency[0] && dev_priv->wm.cur_latency[0])) {
-			dev_priv->display.update_wm = ilk_update_wm;
 			dev_priv->display.compute_pipe_wm = ilk_compute_pipe_wm;
-			dev_priv->display.program_watermarks = ilk_program_watermarks;
+			dev_priv->display.compute_intermediate_wm =
+				ilk_compute_intermediate_wm;
+			dev_priv->display.initial_watermarks = ilk_initial_watermarks;
+			dev_priv->display.optimize_watermarks = ilk_optimize_watermarks;
 		} else {
 			DRM_DEBUG_KMS("Failed to read display plane latency. "
 				      "Disable CxSR\n");
-- 
2.1.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 0/9] Wrap up ILK-style atomic watermarks

[Ville Syrjälä]

On Wed, Nov 25, 2015 at 08:48:26AM -0800, Matt Roper wrote:
> We've been chasing a regression[1] that prevented us from merging the last
> couple patches of the ILK-style atomic watermark series.  We've finally
> identified the culprit --- if we fail to reconstruct the BIOS FB, our internal
> driver state was left in an inconsistent state which caused confusion for the
> watermark code.  Here's a series that collects that fix (along with a couple
> other bugfixes that were found while debugging), a few debugging enhancements,
> and the completion of the ILK atomic watermark series.
> 
>  * The first patch in this series addresses that issue and
>    ensures that we cleanly turn off the primary plane if we're unable to inherit
>    the framebuffer.  
>  * Patches 2-4 just clarify/expand some of the information dumped by the
>    driver while debugging.
>  * Patches 5 and 6 fix two additional bugs that were discovered while
>    searching for the cause of the regression.
>  * Patch 7 just adds some extra paranoia and invariant checking to the
>    watermark code.
>  * Patches 8 and 9 are the two final patches from the original atomic watermark
>    series; with the fixes earlier in the series, we've confirmed that they no
>    longer cause regressions on Jani's machine.
> 
> [1] http://lists.freedesktop.org/archives/intel-gfx/2015-October/077113.html

Quickly skimmed through the series to see if there was a fix to
the "merging optimal wms instead of merging active wms like we
should" bug we have in the current code right now. Nothing
relevant caught my eye at least.

> 
> Matt Roper (9):
>   drm/i915: Disable primary plane if we fail to reconstruct BIOS fb
>   drm/i915: Dump in-flight plane state while dumping in-flight CRTC
>     state
>   drm/i915: Clarify plane state during CRTC state dumps (v2)
>   drm/i915: Dump pipe config after initial FB is reconstructed
>   drm/i915: Setup clipped src/dest coordinates during FB reconstruction
>     (v2)
>   drm/i915: Convert hsw_compute_linetime_wm to use in-flight state
>   drm/i915: Add extra paranoia to ILK watermark calculations
>   drm/i915: Sanitize watermarks after hardware state readout (v2)
>   drm/i915: Add two-stage ILK-style watermark programming (v7)
> 
>  drivers/gpu/drm/drm_atomic_helper.c  |   1 +
>  drivers/gpu/drm/i915/i915_drv.h      |   5 +
>  drivers/gpu/drm/i915/intel_atomic.c  |   1 +
>  drivers/gpu/drm/i915/intel_display.c | 195 +++++++++++++++++++++++++++++++++--
>  drivers/gpu/drm/i915/intel_drv.h     |  31 +++++-
>  drivers/gpu/drm/i915/intel_pm.c      | 186 ++++++++++++++++++++++++---------
>  6 files changed, 359 insertions(+), 60 deletions(-)
> 
> -- 
> 2.1.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 0/9] Wrap up ILK-style atomic watermarks

[Matt Roper]

On Wed, Nov 25, 2015 at 07:00:11PM +0200, Ville Syrjälä wrote:
> On Wed, Nov 25, 2015 at 08:48:26AM -0800, Matt Roper wrote:
> > We've been chasing a regression[1] that prevented us from merging the last
> > couple patches of the ILK-style atomic watermark series.  We've finally
> > identified the culprit --- if we fail to reconstruct the BIOS FB, our internal
> > driver state was left in an inconsistent state which caused confusion for the
> > watermark code.  Here's a series that collects that fix (along with a couple
> > other bugfixes that were found while debugging), a few debugging enhancements,
> > and the completion of the ILK atomic watermark series.
> > 
> >  * The first patch in this series addresses that issue and
> >    ensures that we cleanly turn off the primary plane if we're unable to inherit
> >    the framebuffer.  
> >  * Patches 2-4 just clarify/expand some of the information dumped by the
> >    driver while debugging.
> >  * Patches 5 and 6 fix two additional bugs that were discovered while
> >    searching for the cause of the regression.
> >  * Patch 7 just adds some extra paranoia and invariant checking to the
> >    watermark code.
> >  * Patches 8 and 9 are the two final patches from the original atomic watermark
> >    series; with the fixes earlier in the series, we've confirmed that they no
> >    longer cause regressions on Jani's machine.
> > 
> > [1] http://lists.freedesktop.org/archives/intel-gfx/2015-October/077113.html
> 
> Quickly skimmed through the series to see if there was a fix to
> the "merging optimal wms instead of merging active wms like we
> should" bug we have in the current code right now. Nothing
> relevant caught my eye at least.

I'm not sure I'm aware of this problem; is there a bugzilla or mailing
list thread that describes the issue?


Matt

> 
> > 
> > Matt Roper (9):
> >   drm/i915: Disable primary plane if we fail to reconstruct BIOS fb
> >   drm/i915: Dump in-flight plane state while dumping in-flight CRTC
> >     state
> >   drm/i915: Clarify plane state during CRTC state dumps (v2)
> >   drm/i915: Dump pipe config after initial FB is reconstructed
> >   drm/i915: Setup clipped src/dest coordinates during FB reconstruction
> >     (v2)
> >   drm/i915: Convert hsw_compute_linetime_wm to use in-flight state
> >   drm/i915: Add extra paranoia to ILK watermark calculations
> >   drm/i915: Sanitize watermarks after hardware state readout (v2)
> >   drm/i915: Add two-stage ILK-style watermark programming (v7)
> > 
> >  drivers/gpu/drm/drm_atomic_helper.c  |   1 +
> >  drivers/gpu/drm/i915/i915_drv.h      |   5 +
> >  drivers/gpu/drm/i915/intel_atomic.c  |   1 +
> >  drivers/gpu/drm/i915/intel_display.c | 195 +++++++++++++++++++++++++++++++++--
> >  drivers/gpu/drm/i915/intel_drv.h     |  31 +++++-
> >  drivers/gpu/drm/i915/intel_pm.c      | 186 ++++++++++++++++++++++++---------
> >  6 files changed, 359 insertions(+), 60 deletions(-)
> > 
> > -- 
> > 2.1.4
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Ville Syrjälä
> Intel OTC

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 8/9] drm/i915: Sanitize watermarks after hardware state readout (v2)

[Ville Syrjälä]

On Wed, Nov 25, 2015 at 08:48:34AM -0800, Matt Roper wrote:
> Although we can do a good job of reading out hardware state, the
> graphics firmware may have programmed the watermarks in a creative way
> that doesn't match how i915 would have chosen to program them.  We
> shouldn't trust the firmware's watermark programming, but should rather
> re-calculate how we think WM's should be programmed and then shove those
> values into the hardware.
> 
> We can do this pretty easily by creating a dummy top-level state,
> running it through the check process to calculate all the values, and
> then just programming the watermarks for each CRTC.
> 
> v2:  Move watermark sanitization after our BIOS fb reconstruction; the
>      watermark calculations that we do here need to look at pstate->fb,
>      which isn't setup yet in intel_modeset_setup_hw_state(), even
>      though we have an enabled & visible plane.
> 
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
>  drivers/gpu/drm/drm_atomic_helper.c  |  1 +
>  drivers/gpu/drm/i915/i915_drv.h      |  1 +
>  drivers/gpu/drm/i915/intel_display.c | 58 ++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_pm.c      | 14 +++++----
>  4 files changed, 68 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 3731a26..8a98e0c 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -2478,6 +2478,7 @@ drm_atomic_helper_duplicate_state(struct drm_device *dev,
>  		}
>  	}
>  
> +	drm_modeset_lock(&dev->mode_config.connection_mutex, ctx);
>  	drm_for_each_connector(conn, dev) {
>  		struct drm_connector_state *conn_state;
>  
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 11ae5a5..5172604 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -630,6 +630,7 @@ struct drm_i915_display_funcs {
>  			  struct dpll *best_clock);
>  	int (*compute_pipe_wm)(struct intel_crtc *crtc,
>  			       struct drm_atomic_state *state);
> +	void (*program_watermarks)(struct intel_crtc_state *cstate);
>  	void (*update_wm)(struct drm_crtc *crtc);
>  	int (*modeset_calc_cdclk)(struct drm_atomic_state *state);
>  	void (*modeset_commit_cdclk)(struct drm_atomic_state *state);
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 00e4c37..eb52afa 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -15120,6 +15120,57 @@ void intel_modeset_init_hw(struct drm_device *dev)
>  	intel_enable_gt_powersave(dev);
>  }
>  
> +/*
> + * Calculate what we think the watermarks should be for the state we've read
> + * out of the hardware and then immediately program those watermarks so that
> + * we ensure the hardware settings match our internal state.
> + */
> +static void sanitize_watermarks(struct drm_device *dev)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(dev);
> +	struct drm_atomic_state *state;
> +	struct drm_crtc *crtc;
> +	struct drm_crtc_state *cstate;
> +	struct drm_modeset_acquire_ctx ctx;
> +	int ret;
> +	int i;
> +
> +	/* Only supported on platforms that use atomic watermark design */
> +	if (!dev_priv->display.program_watermarks)
> +		return;
> +
> +	/*
> +	 * Calculate what we think WM's should be by creating a dummy state and
> +	 * running it through the atomic check code.
> +	 */
> +	drm_modeset_acquire_init(&ctx, 0);
> +	state = drm_atomic_helper_duplicate_state(dev, &ctx);
> +	if (WARN_ON(IS_ERR(state)))
> +		return;
> +
> +	ret = intel_atomic_check(dev, state);
> +	if (ret) {
> +		/*
> +		 * Just give up and leave watermarks untouched if we get an
> +		 * error back from 'check'
> +		 */
> +		DRM_DEBUG_KMS("Could not determine valid watermarks for inherited state\n");
> +		return;
> +	}
> +
> +	/* Write calculated watermark values back */
> +	to_i915(dev)->wm.config = to_intel_atomic_state(state)->wm_config;
> +	for_each_crtc_in_state(state, crtc, cstate, i) {
> +		struct intel_crtc_state *cs = to_intel_crtc_state(cstate);
> +
> +		dev_priv->display.program_watermarks(cs);
> +	}
> +
> +	drm_atomic_state_free(state);
> +	drm_modeset_drop_locks(&ctx);
> +	drm_modeset_acquire_fini(&ctx);
> +}
> +
>  void intel_modeset_init(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -15243,6 +15294,13 @@ void intel_modeset_init(struct drm_device *dev)
>  		intel_dump_pipe_config(crtc, crtc->config,
>  				       "[state after init fb reconstruction]");
>  	}
> +
> +	/*
> +	 * Make sure hardware watermarks really match the state we read out.
> +	 * Note that we need to do this after reconstructing the BIOS fb's
> +	 * since the watermark calculation done here will use pstate->fb.
> +	 */
> +	sanitize_watermarks(dev);
>  }
>  
>  static void intel_enable_pipe_a(struct drm_device *dev)
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index d4cd5d5..c209a69 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3610,15 +3610,19 @@ static void skl_update_wm(struct drm_crtc *crtc)
>  	dev_priv->wm.skl_hw = *results;
>  }
>  
> -static void ilk_program_watermarks(struct drm_i915_private *dev_priv)
> +static void ilk_program_watermarks(struct intel_crtc_state *cstate)
>  {
> -	struct drm_device *dev = dev_priv->dev;
> +	struct drm_crtc *crtc = cstate->base.crtc;
> +	struct drm_device *dev = crtc->dev;
> +	struct drm_i915_private *dev_priv = to_i915(dev);
>  	struct intel_pipe_wm lp_wm_1_2 = {}, lp_wm_5_6 = {}, *best_lp_wm;
>  	struct ilk_wm_maximums max;
>  	struct intel_wm_config *config = &dev_priv->wm.config;
>  	struct ilk_wm_values results = {};
>  	enum intel_ddb_partitioning partitioning;
>  
> +	to_intel_crtc(crtc)->wm.active.ilk = cstate->wm.optimal.ilk;
> +

NAK. We want to use this function to program both the intermediate and
optimal wms. So the original place for the assignment was correct.

>  	ilk_compute_wm_maximums(dev, 1, config, INTEL_DDB_PART_1_2, &max);
>  	ilk_wm_merge(dev, config, &max, &lp_wm_1_2);
>  
> @@ -3643,7 +3647,6 @@ static void ilk_program_watermarks(struct drm_i915_private *dev_priv)
>  
>  static void ilk_update_wm(struct drm_crtc *crtc)
>  {
> -	struct drm_i915_private *dev_priv = to_i915(crtc->dev);
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  	struct intel_crtc_state *cstate = to_intel_crtc_state(crtc->state);
>  
> @@ -3661,9 +3664,7 @@ static void ilk_update_wm(struct drm_crtc *crtc)
>  		intel_wait_for_vblank(crtc->dev, intel_crtc->pipe);
>  	}
>  
> -	intel_crtc->wm.active.ilk = cstate->wm.optimal.ilk;
> -
> -	ilk_program_watermarks(dev_priv);
> +	ilk_program_watermarks(cstate);
>  }
>  
>  static void skl_pipe_wm_active_state(uint32_t val,
> @@ -6971,6 +6972,7 @@ void intel_init_pm(struct drm_device *dev)
>  		     dev_priv->wm.spr_latency[0] && dev_priv->wm.cur_latency[0])) {
>  			dev_priv->display.update_wm = ilk_update_wm;
>  			dev_priv->display.compute_pipe_wm = ilk_compute_pipe_wm;
> +			dev_priv->display.program_watermarks = ilk_program_watermarks;
>  		} else {
>  			DRM_DEBUG_KMS("Failed to read display plane latency. "
>  				      "Disable CxSR\n");
> -- 
> 2.1.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 9/9] drm/i915: Add two-stage ILK-style watermark programming (v7)

[Ville Syrjälä]

On Wed, Nov 25, 2015 at 08:48:35AM -0800, Matt Roper wrote:
> In addition to calculating final watermarks, let's also pre-calculate a
> set of intermediate watermark values at atomic check time.  These
> intermediate watermarks are a combination of the watermarks for the old
> state and the new state; they should satisfy the requirements of both
> states which means they can be programmed immediately when we commit the
> atomic state (without waiting for a vblank).  Once the vblank does
> happen, we can then re-program watermarks to the more optimal final
> value.
> 
> v2: Significant rebasing/rewriting.
> 
> v3:
>  - Move 'need_postvbl_update' flag to CRTC state (Daniel)
>  - Don't forget to check intermediate watermark values for validity
>    (Maarten)
>  - Don't due async watermark optimization; just do it at the end of the
>    atomic transaction, after waiting for vblanks.  We do want it to be
>    async eventually, but adding that now will cause more trouble for
>    Maarten's in-progress work.  (Maarten)
>  - Don't allocate space in crtc_state for intermediate watermarks on
>    platforms that don't need it (gen9+).
>  - Move WaCxSRDisabledForSpriteScaling:ivb into intel_begin_crtc_commit
>    now that ilk_update_wm is gone.
> 
> v4:
>  - Add a wm_mutex to cover updates to intel_crtc->active and the
>    need_postvbl_update flag.  Since we don't have async yet it isn't
>    terribly important yet, but might as well add it now.
>  - Change interface to program watermarks.  Platforms will now expose
>    .initial_watermarks() and .optimize_watermarks() functions to do
>    watermark programming.  These should lock wm_mutex, copy the
>    appropriate state values into intel_crtc->active, and then call
>    the internal program watermarks function.
> 
> v5:
>  - Skip intermediate watermark calculation/check during initial hardware
>    readout since we don't trust the existing HW values (and don't have
>    valid values of our own yet).
>  - Don't try to call .optimize_watermarks() on platforms that don't have
>    atomic watermarks yet.  (Maarten)
> 
> v6:
>  - Rebase
> 
> v7:
>  - Further rebase
> 
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> Reviewed-by(v5): Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h      |   6 +-
>  drivers/gpu/drm/i915/intel_atomic.c  |   1 +
>  drivers/gpu/drm/i915/intel_display.c |  90 +++++++++++++++++++-
>  drivers/gpu/drm/i915/intel_drv.h     |  31 ++++++-
>  drivers/gpu/drm/i915/intel_pm.c      | 160 ++++++++++++++++++++++++-----------
>  5 files changed, 232 insertions(+), 56 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 5172604..427b488 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -630,7 +630,11 @@ struct drm_i915_display_funcs {
>  			  struct dpll *best_clock);
>  	int (*compute_pipe_wm)(struct intel_crtc *crtc,
>  			       struct drm_atomic_state *state);
> -	void (*program_watermarks)(struct intel_crtc_state *cstate);
> +	int (*compute_intermediate_wm)(struct drm_device *dev,
> +				       struct intel_crtc *intel_crtc,
> +				       struct intel_crtc_state *newstate);
> +	void (*initial_watermarks)(struct intel_crtc_state *cstate);
> +	void (*optimize_watermarks)(struct intel_crtc_state *cstate);
>  	void (*update_wm)(struct drm_crtc *crtc);
>  	int (*modeset_calc_cdclk)(struct drm_atomic_state *state);
>  	void (*modeset_commit_cdclk)(struct drm_atomic_state *state);
> diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
> index 643f342..b91e166 100644
> --- a/drivers/gpu/drm/i915/intel_atomic.c
> +++ b/drivers/gpu/drm/i915/intel_atomic.c
> @@ -95,6 +95,7 @@ intel_crtc_duplicate_state(struct drm_crtc *crtc)
>  
>  	crtc_state->update_pipe = false;
>  	crtc_state->disable_lp_wm = false;
> +	crtc_state->wm.need_postvbl_update = false;
>  
>  	return &crtc_state->base;
>  }
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index eb52afa..8db0486 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11801,6 +11801,12 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
>  		intel_crtc->atomic.update_wm_pre = true;
>  	}
>  
> +	/* Pre-gen9 platforms need two-step watermark updates */
> +	if ((intel_crtc->atomic.update_wm_pre || intel_crtc->atomic.update_wm_post) &&
> +	    INTEL_INFO(dev)->gen < 9 &&
> +	    dev_priv->display.optimize_watermarks)
> +		to_intel_crtc_state(crtc_state)->wm.need_postvbl_update = true;
> +
>  	if (visible || was_visible)
>  		intel_crtc->atomic.fb_bits |=
>  			to_intel_plane(plane)->frontbuffer_bit;
> @@ -11957,8 +11963,29 @@ static int intel_crtc_atomic_check(struct drm_crtc *crtc,
>  	ret = 0;
>  	if (dev_priv->display.compute_pipe_wm) {
>  		ret = dev_priv->display.compute_pipe_wm(intel_crtc, state);
> -		if (ret)
> +		if (ret) {
> +			DRM_DEBUG_KMS("Target pipe watermarks are invalid\n");
>  			return ret;
> +		}
> +	}
> +
> +	if (dev_priv->display.compute_intermediate_wm &&
> +	    !to_intel_atomic_state(state)->skip_intermediate_wm) {
> +		if (WARN_ON(!dev_priv->display.compute_pipe_wm))
> +			return 0;
> +
> +		/*
> +		 * Calculate 'intermediate' watermarks that satisfy both the
> +		 * old state and the new state.  We can program these
> +		 * immediately.
> +		 */
> +		ret = dev_priv->display.compute_intermediate_wm(crtc->dev,
> +								intel_crtc,
> +								pipe_config);
> +		if (ret) {
> +			DRM_DEBUG_KMS("No valid intermediate pipe watermarks are possible\n");
> +			return ret;
> +		}
>  	}
>  
>  	if (INTEL_INFO(dev)->gen >= 9) {
> @@ -13409,6 +13436,7 @@ static int intel_atomic_commit(struct drm_device *dev,
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct drm_crtc_state *crtc_state;
>  	struct drm_crtc *crtc;
> +	struct intel_crtc_state *intel_cstate;
>  	int ret = 0;
>  	int i;
>  	bool any_ms = false;
> @@ -13500,6 +13528,20 @@ static int intel_atomic_commit(struct drm_device *dev,
>  
>  	drm_atomic_helper_wait_for_vblanks(dev, state);
>  
> +	/*
> +	 * Now that the vblank has passed, we can go ahead and program the
> +	 * optimal watermarks on platforms that need two-step watermark
> +	 * programming.
> +	 *
> +	 * TODO: Move this (and other cleanup) to an async worker eventually.
> +	 */
> +	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> +		intel_cstate = to_intel_crtc_state(crtc->state);
> +
> +		if (dev_priv->display.optimize_watermarks)
> +		    dev_priv->display.optimize_watermarks(intel_cstate);
> +	}
> +
>  	mutex_lock(&dev->struct_mutex);
>  	drm_atomic_helper_cleanup_planes(dev, state);
>  	mutex_unlock(&dev->struct_mutex);
> @@ -13862,13 +13904,44 @@ 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 *cstate = to_intel_crtc_state(crtc->state);
>  	struct intel_crtc_state *old_intel_state =
>  		to_intel_crtc_state(old_crtc_state);
>  	bool modeset = needs_modeset(crtc->state);
>  
> -	if (intel_crtc->atomic.update_wm_pre)
> +	/*
> +	 * IVB workaround: must disable low power watermarks for at least
> +	 * one frame before enabling scaling.  LP watermarks can be re-enabled
> +	 * when scaling is disabled.
> +	 *
> +	 * WaCxSRDisabledForSpriteScaling:ivb
> +	 */
> +	if (cstate->disable_lp_wm) {
> +		ilk_disable_lp_wm(crtc->dev);
> +		intel_wait_for_vblank(crtc->dev, intel_crtc->pipe);
> +	}
> +
> +	/*
> +	 * For platforms that support atomic watermarks, program the
> +	 * 'intermediate' watermarks immediately.  On pre-gen9 platforms, these
> +	 * will be the intermediate values that are safe for both pre- and
> +	 * post- vblank; when vblank happens, the 'active' values will be set
> +	 * to the final 'target' values and we'll do this again to get the
> +	 * optimal watermarks.  For gen9+ platforms, the values we program here
> +	 * will be the final target values which will get automatically latched
> +	 * at vblank time; no further programming will be necessary.
> +	 *
> +	 * If a platform hasn't been transitioned to atomic watermarks yet,
> +	 * we'll continue to update watermarks the old way, if flags tell
> +	 * us to.
> +	 */
> +	if (dev_priv->display.initial_watermarks != NULL) {
> +		dev_priv->display.initial_watermarks(cstate);
> +	} else if (intel_crtc->atomic.update_wm_pre) {
>  		intel_update_watermarks(crtc);
> +	}
>  
>  	/* Perform vblank evasion around commit operation */
>  	intel_pipe_update_start(intel_crtc);
> @@ -14207,6 +14280,7 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
>  	intel_crtc->cursor_size = ~0;
>  
>  	intel_crtc->wm.cxsr_allowed = true;
> +	mutex_init(&intel_crtc->wm.wm_mutex);
>  
>  	BUG_ON(pipe >= ARRAY_SIZE(dev_priv->plane_to_crtc_mapping) ||
>  	       dev_priv->plane_to_crtc_mapping[intel_crtc->plane] != NULL);
> @@ -15136,7 +15210,7 @@ static void sanitize_watermarks(struct drm_device *dev)
>  	int i;
>  
>  	/* Only supported on platforms that use atomic watermark design */
> -	if (!dev_priv->display.program_watermarks)
> +	if (!dev_priv->display.optimize_watermarks)
>  		return;
>  
>  	/*
> @@ -15148,6 +15222,13 @@ static void sanitize_watermarks(struct drm_device *dev)
>  	if (WARN_ON(IS_ERR(state)))
>  		return;
>  
> +	/*
> +	 * Hardware readout is the only time we don't want to calculate
> +	 * intermediate watermarks (since we don't trust the current
> +	 * watermarks).
> +	 */
> +	to_intel_atomic_state(state)->skip_intermediate_wm = true;
> +
>  	ret = intel_atomic_check(dev, state);
>  	if (ret) {
>  		/*
> @@ -15163,7 +15244,8 @@ static void sanitize_watermarks(struct drm_device *dev)
>  	for_each_crtc_in_state(state, crtc, cstate, i) {
>  		struct intel_crtc_state *cs = to_intel_crtc_state(cstate);
>  
> -		dev_priv->display.program_watermarks(cs);
> +		cs->wm.need_postvbl_update = true;
> +		dev_priv->display.optimize_watermarks(cs);
>  	}
>  
>  	drm_atomic_state_free(state);
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 8fae824..c53eb10 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -249,6 +249,12 @@ struct intel_atomic_state {
>  	bool dpll_set;
>  	struct intel_shared_dpll_config shared_dpll[I915_NUM_PLLS];
>  	struct intel_wm_config wm_config;
> +
> +	/*
> +	 * Current watermarks can't be trusted during hardware readout, so
> +	 * don't bother calculating intermediate watermarks.
> +	 */
> +	bool skip_intermediate_wm;
>  };
>  
>  struct intel_plane_state {
> @@ -491,13 +497,29 @@ struct intel_crtc_state {
>  
>  	struct {
>  		/*
> -		 * optimal watermarks, programmed post-vblank when this state
> -		 * is committed
> +		 * Optimal watermarks, programmed post-vblank when this state
> +		 * is committed.
>  		 */
>  		union {
>  			struct intel_pipe_wm ilk;
>  			struct skl_pipe_wm skl;
>  		} optimal;
> +
> +		/*
> +		 * Intermediate watermarks; these can be programmed immediately
> +		 * since they satisfy both the current configuration we're
> +		 * switching away from and the new configuration we're switching
> +		 * to.
> +		 */
> +		struct intel_pipe_wm intermediate;
> +
> +		/*
> +		 * Platforms with two-step watermark programming will need to
> +		 * update watermark programming post-vblank to switch from the
> +		 * safe intermediate watermarks to the optimal final
> +		 * watermarks.
> +		 */
> +		bool need_postvbl_update;
>  	} wm;
>  };
>  
> @@ -587,8 +609,12 @@ struct intel_crtc {
>  			struct intel_pipe_wm ilk;
>  			struct skl_pipe_wm skl;
>  		} active;
> +
>  		/* allow CxSR on this pipe */
>  		bool cxsr_allowed;
> +
> +		/* Protects active and need_postvbl_update */
> +		struct mutex wm_mutex;
>  	} wm;
>  
>  	int scanline_offset;
> @@ -1463,6 +1489,7 @@ void skl_wm_get_hw_state(struct drm_device *dev);
>  void skl_ddb_get_hw_state(struct drm_i915_private *dev_priv,
>  			  struct skl_ddb_allocation *ddb /* out */);
>  uint32_t ilk_pipe_pixel_rate(const struct intel_crtc_state *pipe_config);
> +bool ilk_disable_lp_wm(struct drm_device *dev);
>  
>  /* intel_sdvo.c */
>  bool intel_sdvo_init(struct drm_device *dev,
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index c209a69..29d725e 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -2265,6 +2265,29 @@ static void skl_setup_wm_latency(struct drm_device *dev)
>  	intel_print_wm_latency(dev, "Gen9 Plane", dev_priv->wm.skl_latency);
>  }
>  
> +static bool ilk_validate_pipe_wm(struct drm_device *dev,
> +				 struct intel_pipe_wm *pipe_wm)
> +{
> +	/* LP0 watermark maximums depend on this pipe alone */
> +	const struct intel_wm_config config = {
> +		.num_pipes_active = 1,
> +		.sprites_enabled = pipe_wm->sprites_enabled,
> +		.sprites_scaled = pipe_wm->sprites_scaled,
> +	};
> +	struct ilk_wm_maximums max;
> +
> +	/* LP0 watermarks always use 1/2 DDB partitioning */
> +	ilk_compute_wm_maximums(dev, 0, &config, INTEL_DDB_PART_1_2, &max);
> +
> +	/* At least LP0 must be valid */
> +	if (!ilk_validate_wm_level(0, &max, &pipe_wm->wm[0])) {
> +		DRM_DEBUG_KMS("LP0 watermark invalid\n");
> +		return false;
> +	}
> +
> +	return true;
> +}
> +
>  /* Compute new watermarks for the pipe */
>  static int ilk_compute_pipe_wm(struct intel_crtc *intel_crtc,
>  			       struct drm_atomic_state *state)
> @@ -2279,10 +2302,6 @@ static int ilk_compute_pipe_wm(struct intel_crtc *intel_crtc,
>  	struct intel_plane_state *sprstate = NULL;
>  	struct intel_plane_state *curstate = NULL;
>  	int level, max_level = ilk_wm_max_level(dev);
> -	/* LP0 watermark maximums depend on this pipe alone */
> -	struct intel_wm_config config = {
> -		.num_pipes_active = 1,
> -	};
>  	struct ilk_wm_maximums max;
>  
>  	cstate = intel_atomic_get_crtc_state(state, intel_crtc);
> @@ -2305,21 +2324,18 @@ static int ilk_compute_pipe_wm(struct intel_crtc *intel_crtc,
>  			curstate = to_intel_plane_state(ps);
>  	}
>  
> -	config.sprites_enabled = sprstate->visible;
> -	config.sprites_scaled = sprstate->visible &&
> +	pipe_wm->pipe_enabled = cstate->base.active;
> +	pipe_wm->sprites_enabled = sprstate->visible;
> +	pipe_wm->sprites_scaled = sprstate->visible &&
>  		(drm_rect_width(&sprstate->dst) != drm_rect_width(&sprstate->src) >> 16 ||
>  		drm_rect_height(&sprstate->dst) != drm_rect_height(&sprstate->src) >> 16);
>  
> -	pipe_wm->pipe_enabled = cstate->base.active;
> -	pipe_wm->sprites_enabled = config.sprites_enabled;
> -	pipe_wm->sprites_scaled = config.sprites_scaled;
> -
>  	/* ILK/SNB: LP2+ watermarks only w/o sprites */
>  	if (INTEL_INFO(dev)->gen <= 6 && sprstate->visible)
>  		max_level = 1;
>  
>  	/* ILK/SNB/IVB: LP1+ watermarks only w/o scaling */
> -	if (config.sprites_scaled)
> +	if (pipe_wm->sprites_scaled)
>  		max_level = 0;
>  
>  	ilk_compute_wm_level(dev_priv, intel_crtc, 0, cstate,
> @@ -2328,12 +2344,8 @@ static int ilk_compute_pipe_wm(struct intel_crtc *intel_crtc,
>  	if (IS_HASWELL(dev) || IS_BROADWELL(dev))
>  		pipe_wm->linetime = hsw_compute_linetime_wm(dev, cstate);
>  
> -	/* LP0 watermarks always use 1/2 DDB partitioning */
> -	ilk_compute_wm_maximums(dev, 0, &config, INTEL_DDB_PART_1_2, &max);
> -
> -	/* At least LP0 must be valid */
> -	if (!ilk_validate_wm_level(0, &max, &pipe_wm->wm[0]))
> -		return -EINVAL;
> +	if (!ilk_validate_pipe_wm(dev, pipe_wm))
> +		return false;
>  
>  	ilk_compute_wm_reg_maximums(dev, 1, &max);
>  
> @@ -2358,6 +2370,59 @@ static int ilk_compute_pipe_wm(struct intel_crtc *intel_crtc,
>  }
>  
>  /*
> + * Build a set of 'intermediate' watermark values that satisfy both the old
> + * state and the new state.  These can be programmed to the hardware
> + * immediately.
> + */
> +static int ilk_compute_intermediate_wm(struct drm_device *dev,
> +				       struct intel_crtc *intel_crtc,
> +				       struct intel_crtc_state *newstate)
> +{
> +	struct intel_pipe_wm *a = &newstate->wm.intermediate;
> +	struct intel_pipe_wm *b = &intel_crtc->wm.active.ilk;
> +	int level, max_level = ilk_wm_max_level(dev);
> +
> +	/*
> +	 * Start with the final, target watermarks, then combine with the
> +	 * currently active watermarks to get values that are safe both before
> +	 * and after the vblank.
> +	 */
> +	*a = newstate->wm.optimal.ilk;
> +	a->pipe_enabled |= b->pipe_enabled;
> +	a->sprites_enabled |= b->sprites_enabled;
> +	a->sprites_scaled |= b->sprites_scaled;
> +
> +	for (level = 0; level <= max_level; level++) {
> +		struct intel_wm_level *a_wm = &a->wm[level];
> +		const struct intel_wm_level *b_wm = &b->wm[level];
> +
> +		a_wm->enable &= b_wm->enable;
> +		a_wm->pri_val = max(a_wm->pri_val, b_wm->pri_val);
> +		a_wm->spr_val = max(a_wm->spr_val, b_wm->spr_val);
> +		a_wm->cur_val = max(a_wm->cur_val, b_wm->cur_val);
> +		a_wm->fbc_val = max(a_wm->fbc_val, b_wm->fbc_val);
> +	}
> +
> +	/*
> +	 * We need to make sure that these merged watermark values are
> +	 * actually a valid configuration themselves.  If they're not,
> +	 * there's no safe way to transition from the old state to
> +	 * the new state, so we need to fail the atomic transaction.
> +	 */
> +	if (!ilk_validate_pipe_wm(dev, a))
> +		return -EINVAL;
> +
> +	/*
> +	 * If our intermediate WM are identical to the final WM, then we can
> +	 * omit the post-vblank programming; only update if it's different.
> +	 */
> +	if (memcmp(a, &newstate->wm.optimal.ilk, sizeof *a) != 0)
> +		newstate->wm.need_postvbl_update = false;
> +
> +	return 0;
> +}
> +
> +/*
>   * Merge the watermarks from all active pipes for a specific level.
>   */
>  static void ilk_merge_wm_level(struct drm_device *dev,
> @@ -2369,9 +2434,7 @@ static void ilk_merge_wm_level(struct drm_device *dev,
>  	ret_wm->enable = true;
>  
>  	for_each_intel_crtc(dev, intel_crtc) {
> -		const struct intel_crtc_state *cstate =
> -			to_intel_crtc_state(intel_crtc->base.state);
> -		const struct intel_pipe_wm *active = &cstate->wm.optimal.ilk;
> +		const struct intel_pipe_wm *active = &intel_crtc->wm.active.ilk;
>  		const struct intel_wm_level *wm = &active->wm[level];
>  
>  		if (!active->pipe_enabled)
> @@ -2519,15 +2582,13 @@ static void ilk_compute_wm_results(struct drm_device *dev,
>  
>  	/* LP0 register values */
>  	for_each_intel_crtc(dev, intel_crtc) {
> -		const struct intel_crtc_state *cstate =
> -			to_intel_crtc_state(intel_crtc->base.state);
>  		enum pipe pipe = intel_crtc->pipe;
> -		const struct intel_wm_level *r = &cstate->wm.optimal.ilk.wm[0];
> +		const struct intel_wm_level *r = &intel_crtc->wm.active.ilk.wm[0];
>  
>  		if (WARN_ON(!r->enable))
>  			continue;
>  
> -		results->wm_linetime[pipe] = cstate->wm.optimal.ilk.linetime;
> +		results->wm_linetime[pipe] = intel_crtc->wm.active.ilk.linetime;
>  
>  		results->wm_pipe[pipe] =
>  			(r->pri_val << WM0_PIPE_PLANE_SHIFT) |
> @@ -2734,7 +2795,7 @@ static void ilk_write_wm_values(struct drm_i915_private *dev_priv,
>  	dev_priv->wm.hw = *results;
>  }
>  
> -static bool ilk_disable_lp_wm(struct drm_device *dev)
> +bool ilk_disable_lp_wm(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  
> @@ -3610,19 +3671,15 @@ static void skl_update_wm(struct drm_crtc *crtc)
>  	dev_priv->wm.skl_hw = *results;
>  }
>  
> -static void ilk_program_watermarks(struct intel_crtc_state *cstate)
> +static void ilk_program_watermarks(struct drm_i915_private *dev_priv)
>  {
> -	struct drm_crtc *crtc = cstate->base.crtc;
> -	struct drm_device *dev = crtc->dev;
> -	struct drm_i915_private *dev_priv = to_i915(dev);
> +	struct drm_device *dev = dev_priv->dev;
>  	struct intel_pipe_wm lp_wm_1_2 = {}, lp_wm_5_6 = {}, *best_lp_wm;
>  	struct ilk_wm_maximums max;
>  	struct intel_wm_config *config = &dev_priv->wm.config;
>  	struct ilk_wm_values results = {};
>  	enum intel_ddb_partitioning partitioning;
>  
> -	to_intel_crtc(crtc)->wm.active.ilk = cstate->wm.optimal.ilk;
> -

:) So even you yourself NAKed the previous patch.

>  	ilk_compute_wm_maximums(dev, 1, config, INTEL_DDB_PART_1_2, &max);
>  	ilk_wm_merge(dev, config, &max, &lp_wm_1_2);
>  
> @@ -3645,26 +3702,29 @@ static void ilk_program_watermarks(struct intel_crtc_state *cstate)
>  	ilk_write_wm_values(dev_priv, &results);
>  }
>  
> -static void ilk_update_wm(struct drm_crtc *crtc)
> +static void ilk_initial_watermarks(struct intel_crtc_state *cstate)
>  {
> -	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> -	struct intel_crtc_state *cstate = to_intel_crtc_state(crtc->state);
> +	struct drm_i915_private *dev_priv = to_i915(cstate->base.crtc->dev);
> +	struct intel_crtc *intel_crtc = to_intel_crtc(cstate->base.crtc);
>  
> -	WARN_ON(cstate->base.active != intel_crtc->active);
> +	mutex_lock(&intel_crtc->wm.wm_mutex);
> +	intel_crtc->wm.active.ilk = cstate->wm.intermediate;
> +	ilk_program_watermarks(dev_priv);
> +	mutex_unlock(&intel_crtc->wm.wm_mutex);
> +}
>  
> -	/*
> -	 * IVB workaround: must disable low power watermarks for at least
> -	 * one frame before enabling scaling.  LP watermarks can be re-enabled
> -	 * when scaling is disabled.
> -	 *
> -	 * WaCxSRDisabledForSpriteScaling:ivb
> -	 */
> -	if (cstate->disable_lp_wm) {
> -		ilk_disable_lp_wm(crtc->dev);
> -		intel_wait_for_vblank(crtc->dev, intel_crtc->pipe);
> -	}
> +static void ilk_optimize_watermarks(struct intel_crtc_state *cstate)

I don't really like that name. Should just have foo_pre and foo_post or
something along those lines IMO.

> +{
> +	struct drm_i915_private *dev_priv = to_i915(cstate->base.crtc->dev);
> +	struct intel_crtc *intel_crtc = to_intel_crtc(cstate->base.crtc);
>  
> -	ilk_program_watermarks(cstate);
> +	mutex_lock(&intel_crtc->wm.wm_mutex);
> +	if (cstate->wm.need_postvbl_update) {
> +		intel_crtc->wm.active.ilk = cstate->wm.optimal.ilk;
> +		ilk_program_watermarks(dev_priv);
> +		cstate->wm.need_postvbl_update = false;
> +	}
> +	mutex_unlock(&intel_crtc->wm.wm_mutex);
>  }
>  
>  static void skl_pipe_wm_active_state(uint32_t val,
> @@ -6970,9 +7030,11 @@ void intel_init_pm(struct drm_device *dev)
>  		     dev_priv->wm.spr_latency[1] && dev_priv->wm.cur_latency[1]) ||
>  		    (!IS_GEN5(dev) && dev_priv->wm.pri_latency[0] &&
>  		     dev_priv->wm.spr_latency[0] && dev_priv->wm.cur_latency[0])) {
> -			dev_priv->display.update_wm = ilk_update_wm;
>  			dev_priv->display.compute_pipe_wm = ilk_compute_pipe_wm;
> -			dev_priv->display.program_watermarks = ilk_program_watermarks;
> +			dev_priv->display.compute_intermediate_wm =
> +				ilk_compute_intermediate_wm;
> +			dev_priv->display.initial_watermarks = ilk_initial_watermarks;
> +			dev_priv->display.optimize_watermarks = ilk_optimize_watermarks;
>  		} else {
>  			DRM_DEBUG_KMS("Failed to read display plane latency. "
>  				      "Disable CxSR\n");
> -- 
> 2.1.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 0/9] Wrap up ILK-style atomic watermarks

[Ville Syrjälä]

On Wed, Nov 25, 2015 at 09:04:11AM -0800, Matt Roper wrote:
> On Wed, Nov 25, 2015 at 07:00:11PM +0200, Ville Syrjälä wrote:
> > On Wed, Nov 25, 2015 at 08:48:26AM -0800, Matt Roper wrote:
> > > We've been chasing a regression[1] that prevented us from merging the last
> > > couple patches of the ILK-style atomic watermark series.  We've finally
> > > identified the culprit --- if we fail to reconstruct the BIOS FB, our internal
> > > driver state was left in an inconsistent state which caused confusion for the
> > > watermark code.  Here's a series that collects that fix (along with a couple
> > > other bugfixes that were found while debugging), a few debugging enhancements,
> > > and the completion of the ILK atomic watermark series.
> > > 
> > >  * The first patch in this series addresses that issue and
> > >    ensures that we cleanly turn off the primary plane if we're unable to inherit
> > >    the framebuffer.  
> > >  * Patches 2-4 just clarify/expand some of the information dumped by the
> > >    driver while debugging.
> > >  * Patches 5 and 6 fix two additional bugs that were discovered while
> > >    searching for the cause of the regression.
> > >  * Patch 7 just adds some extra paranoia and invariant checking to the
> > >    watermark code.
> > >  * Patches 8 and 9 are the two final patches from the original atomic watermark
> > >    series; with the fixes earlier in the series, we've confirmed that they no
> > >    longer cause regressions on Jani's machine.
> > > 
> > > [1] http://lists.freedesktop.org/archives/intel-gfx/2015-October/077113.html
> > 
> > Quickly skimmed through the series to see if there was a fix to
> > the "merging optimal wms instead of merging active wms like we
> > should" bug we have in the current code right now. Nothing
> > relevant caught my eye at least.
> 
> I'm not sure I'm aware of this problem; is there a bugzilla or mailing
> list thread that describes the issue?

Maybe not. The problem is that ilk_wm_merge() and co. use the optimal
wms when merging from all pipes. They should use the active wms
instead.

> 
> 
> Matt
> 
> > 
> > > 
> > > Matt Roper (9):
> > >   drm/i915: Disable primary plane if we fail to reconstruct BIOS fb
> > >   drm/i915: Dump in-flight plane state while dumping in-flight CRTC
> > >     state
> > >   drm/i915: Clarify plane state during CRTC state dumps (v2)
> > >   drm/i915: Dump pipe config after initial FB is reconstructed
> > >   drm/i915: Setup clipped src/dest coordinates during FB reconstruction
> > >     (v2)
> > >   drm/i915: Convert hsw_compute_linetime_wm to use in-flight state
> > >   drm/i915: Add extra paranoia to ILK watermark calculations
> > >   drm/i915: Sanitize watermarks after hardware state readout (v2)
> > >   drm/i915: Add two-stage ILK-style watermark programming (v7)
> > > 
> > >  drivers/gpu/drm/drm_atomic_helper.c  |   1 +
> > >  drivers/gpu/drm/i915/i915_drv.h      |   5 +
> > >  drivers/gpu/drm/i915/intel_atomic.c  |   1 +
> > >  drivers/gpu/drm/i915/intel_display.c | 195 +++++++++++++++++++++++++++++++++--
> > >  drivers/gpu/drm/i915/intel_drv.h     |  31 +++++-
> > >  drivers/gpu/drm/i915/intel_pm.c      | 186 ++++++++++++++++++++++++---------
> > >  6 files changed, 359 insertions(+), 60 deletions(-)
> > > 
> > > -- 
> > > 2.1.4
> > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
> > -- 
> > Ville Syrjälä
> > Intel OTC
> 
> -- 
> Matt Roper
> Graphics Software Engineer
> IoTG Platform Enabling & Development
> Intel Corporation
> (916) 356-2795

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 8/9] drm/i915: Sanitize watermarks after hardware state readout (v2)

[Maarten Lankhorst]

Op 25-11-15 om 17:48 schreef Matt Roper:
> Although we can do a good job of reading out hardware state, the
> graphics firmware may have programmed the watermarks in a creative way
> that doesn't match how i915 would have chosen to program them.  We
> shouldn't trust the firmware's watermark programming, but should rather
> re-calculate how we think WM's should be programmed and then shove those
> values into the hardware.
>
> We can do this pretty easily by creating a dummy top-level state,
> running it through the check process to calculate all the values, and
> then just programming the watermarks for each CRTC.
>
> v2:  Move watermark sanitization after our BIOS fb reconstruction; the
>      watermark calculations that we do here need to look at pstate->fb,
>      which isn't setup yet in intel_modeset_setup_hw_state(), even
>      though we have an enabled & visible plane.
>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
>  drivers/gpu/drm/drm_atomic_helper.c  |  1 +
>  drivers/gpu/drm/i915/i915_drv.h      |  1 +
>  drivers/gpu/drm/i915/intel_display.c | 58 ++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_pm.c      | 14 +++++----
>  4 files changed, 68 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 3731a26..8a98e0c 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -2478,6 +2478,7 @@ drm_atomic_helper_duplicate_state(struct drm_device *dev,
>  		}
>  	}
>  
> +	drm_modeset_lock(&dev->mode_config.connection_mutex, ctx);
>  	drm_for_each_connector(conn, dev) {
>  		struct drm_connector_state *conn_state;
Seems this hunk doesn't belong to this patch?
Also that locking is already taken care of by drm_atomic_get_connector_state,
and it doesn't check for drm_modeset_lock returning an error code..

> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 11ae5a5..5172604 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -630,6 +630,7 @@ struct drm_i915_display_funcs {
>  			  struct dpll *best_clock);
>  	int (*compute_pipe_wm)(struct intel_crtc *crtc,
>  			       struct drm_atomic_state *state);
> +	void (*program_watermarks)(struct intel_crtc_state *cstate);
>  	void (*update_wm)(struct drm_crtc *crtc);
>  	int (*modeset_calc_cdclk)(struct drm_atomic_state *state);
>  	void (*modeset_commit_cdclk)(struct drm_atomic_state *state);
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 00e4c37..eb52afa 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -15120,6 +15120,57 @@ void intel_modeset_init_hw(struct drm_device *dev)
>  	intel_enable_gt_powersave(dev);
>  }
>  
> +/*
> + * Calculate what we think the watermarks should be for the state we've read
> + * out of the hardware and then immediately program those watermarks so that
> + * we ensure the hardware settings match our internal state.
> + */
> +static void sanitize_watermarks(struct drm_device *dev)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(dev);
> +	struct drm_atomic_state *state;
> +	struct drm_crtc *crtc;
> +	struct drm_crtc_state *cstate;
> +	struct drm_modeset_acquire_ctx ctx;
> +	int ret;
> +	int i;
> +
> +	/* Only supported on platforms that use atomic watermark design */
> +	if (!dev_priv->display.program_watermarks)
> +		return;
> +
> +	/*
> +	 * Calculate what we think WM's should be by creating a dummy state and
> +	 * running it through the atomic check code.
> +	 */
> +	drm_modeset_acquire_init(&ctx, 0);
> +	state = drm_atomic_helper_duplicate_state(dev, &ctx);
> +	if (WARN_ON(IS_ERR(state)))
> +		return;
> +
> +	ret = intel_atomic_check(dev, state);
> +	if (ret) {
> +		/*
> +		 * Just give up and leave watermarks untouched if we get an
> +		 * error back from 'check'
> +		 */
> +		DRM_DEBUG_KMS("Could not determine valid watermarks for inherited state\n");
> +		return;
> +	}
> +
> +	/* Write calculated watermark values back */
> +	to_i915(dev)->wm.config = to_intel_atomic_state(state)->wm_config;
> +	for_each_crtc_in_state(state, crtc, cstate, i) {
> +		struct intel_crtc_state *cs = to_intel_crtc_state(cstate);
> +
> +		dev_priv->display.program_watermarks(cs);
> +	}
> +
> +	drm_atomic_state_free(state);
> +	drm_modeset_drop_locks(&ctx);
> +	drm_modeset_acquire_fini(&ctx);
> +}
> +
>  void intel_modeset_init(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -15243,6 +15294,13 @@ void intel_modeset_init(struct drm_device *dev)
>  		intel_dump_pipe_config(crtc, crtc->config,
>  				       "[state after init fb reconstruction]");
>  	}
> +
> +	/*
> +	 * Make sure hardware watermarks really match the state we read out.
> +	 * Note that we need to do this after reconstructing the BIOS fb's
> +	 * since the watermark calculation done here will use pstate->fb.
> +	 */
> +	sanitize_watermarks(dev);
>  }
>  
>  static void intel_enable_pipe_a(struct drm_device *dev)
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index d4cd5d5..c209a69 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3610,15 +3610,19 @@ static void skl_update_wm(struct drm_crtc *crtc)
>  	dev_priv->wm.skl_hw = *results;
>  }
>  
> -static void ilk_program_watermarks(struct drm_i915_private *dev_priv)
> +static void ilk_program_watermarks(struct intel_crtc_state *cstate)
>  {
> -	struct drm_device *dev = dev_priv->dev;
> +	struct drm_crtc *crtc = cstate->base.crtc;
> +	struct drm_device *dev = crtc->dev;
> +	struct drm_i915_private *dev_priv = to_i915(dev);
>  	struct intel_pipe_wm lp_wm_1_2 = {}, lp_wm_5_6 = {}, *best_lp_wm;
>  	struct ilk_wm_maximums max;
>  	struct intel_wm_config *config = &dev_priv->wm.config;
>  	struct ilk_wm_values results = {};
>  	enum intel_ddb_partitioning partitioning;
>  
> +	to_intel_crtc(crtc)->wm.active.ilk = cstate->wm.optimal.ilk;
> +
>  	ilk_compute_wm_maximums(dev, 1, config, INTEL_DDB_PART_1_2, &max);
>  	ilk_wm_merge(dev, config, &max, &lp_wm_1_2);
>  
> @@ -3643,7 +3647,6 @@ static void ilk_program_watermarks(struct drm_i915_private *dev_priv)
>  
>  static void ilk_update_wm(struct drm_crtc *crtc)
>  {
> -	struct drm_i915_private *dev_priv = to_i915(crtc->dev);
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  	struct intel_crtc_state *cstate = to_intel_crtc_state(crtc->state);
>  
> @@ -3661,9 +3664,7 @@ static void ilk_update_wm(struct drm_crtc *crtc)
>  		intel_wait_for_vblank(crtc->dev, intel_crtc->pipe);
>  	}
>  
> -	intel_crtc->wm.active.ilk = cstate->wm.optimal.ilk;
> -
> -	ilk_program_watermarks(dev_priv);
> +	ilk_program_watermarks(cstate);
>  }
>  
>  static void skl_pipe_wm_active_state(uint32_t val,
> @@ -6971,6 +6972,7 @@ void intel_init_pm(struct drm_device *dev)
>  		     dev_priv->wm.spr_latency[0] && dev_priv->wm.cur_latency[0])) {
>  			dev_priv->display.update_wm = ilk_update_wm;
>  			dev_priv->display.compute_pipe_wm = ilk_compute_pipe_wm;
> +			dev_priv->display.program_watermarks = ilk_program_watermarks;
>  		} else {
>  			DRM_DEBUG_KMS("Failed to read display plane latency. "
>  				      "Disable CxSR\n");

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 8/9] drm/i915: Sanitize watermarks after hardware state readout (v2)

[Matt Roper]

On Mon, Nov 30, 2015 at 10:50:26AM +0100, Maarten Lankhorst wrote:
> Op 25-11-15 om 17:48 schreef Matt Roper:
> > Although we can do a good job of reading out hardware state, the
> > graphics firmware may have programmed the watermarks in a creative way
> > that doesn't match how i915 would have chosen to program them.  We
> > shouldn't trust the firmware's watermark programming, but should rather
> > re-calculate how we think WM's should be programmed and then shove those
> > values into the hardware.
> >
> > We can do this pretty easily by creating a dummy top-level state,
> > running it through the check process to calculate all the values, and
> > then just programming the watermarks for each CRTC.
> >
> > v2:  Move watermark sanitization after our BIOS fb reconstruction; the
> >      watermark calculations that we do here need to look at pstate->fb,
> >      which isn't setup yet in intel_modeset_setup_hw_state(), even
> >      though we have an enabled & visible plane.
> >
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> > ---
> >  drivers/gpu/drm/drm_atomic_helper.c  |  1 +
> >  drivers/gpu/drm/i915/i915_drv.h      |  1 +
> >  drivers/gpu/drm/i915/intel_display.c | 58 ++++++++++++++++++++++++++++++++++++
> >  drivers/gpu/drm/i915/intel_pm.c      | 14 +++++----
> >  4 files changed, 68 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> > index 3731a26..8a98e0c 100644
> > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > @@ -2478,6 +2478,7 @@ drm_atomic_helper_duplicate_state(struct drm_device *dev,
> >  		}
> >  	}
> >  
> > +	drm_modeset_lock(&dev->mode_config.connection_mutex, ctx);
> >  	drm_for_each_connector(conn, dev) {
> >  		struct drm_connector_state *conn_state;
> Seems this hunk doesn't belong to this patch?
> Also that locking is already taken care of by drm_atomic_get_connector_state,
> and it doesn't check for drm_modeset_lock returning an error code..

This should be a separate patch, but it is indeed necessary.  We hit the
locking error before we call drm_atomic_get_connector_state...the loop
iterator itself (drm_for_each_connector) will throw a warning if we're
not already holding connection_mutex as of

        commit 7a3f3d6667f5f9ffd1517f6b21d64bbf5312042c
        Author: Daniel Vetter <daniel.vetter@ffwll.ch>
        Date:   Thu Jul 9 23:44:28 2015 +0200

            drm: Check locking in drm_for_each_connector

I'll pull this out and send it separately.


Matt

> 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 11ae5a5..5172604 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -630,6 +630,7 @@ struct drm_i915_display_funcs {
> >  			  struct dpll *best_clock);
> >  	int (*compute_pipe_wm)(struct intel_crtc *crtc,
> >  			       struct drm_atomic_state *state);
> > +	void (*program_watermarks)(struct intel_crtc_state *cstate);
> >  	void (*update_wm)(struct drm_crtc *crtc);
> >  	int (*modeset_calc_cdclk)(struct drm_atomic_state *state);
> >  	void (*modeset_commit_cdclk)(struct drm_atomic_state *state);
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 00e4c37..eb52afa 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -15120,6 +15120,57 @@ void intel_modeset_init_hw(struct drm_device *dev)
> >  	intel_enable_gt_powersave(dev);
> >  }
> >  
> > +/*
> > + * Calculate what we think the watermarks should be for the state we've read
> > + * out of the hardware and then immediately program those watermarks so that
> > + * we ensure the hardware settings match our internal state.
> > + */
> > +static void sanitize_watermarks(struct drm_device *dev)
> > +{
> > +	struct drm_i915_private *dev_priv = to_i915(dev);
> > +	struct drm_atomic_state *state;
> > +	struct drm_crtc *crtc;
> > +	struct drm_crtc_state *cstate;
> > +	struct drm_modeset_acquire_ctx ctx;
> > +	int ret;
> > +	int i;
> > +
> > +	/* Only supported on platforms that use atomic watermark design */
> > +	if (!dev_priv->display.program_watermarks)
> > +		return;
> > +
> > +	/*
> > +	 * Calculate what we think WM's should be by creating a dummy state and
> > +	 * running it through the atomic check code.
> > +	 */
> > +	drm_modeset_acquire_init(&ctx, 0);
> > +	state = drm_atomic_helper_duplicate_state(dev, &ctx);
> > +	if (WARN_ON(IS_ERR(state)))
> > +		return;
> > +
> > +	ret = intel_atomic_check(dev, state);
> > +	if (ret) {
> > +		/*
> > +		 * Just give up and leave watermarks untouched if we get an
> > +		 * error back from 'check'
> > +		 */
> > +		DRM_DEBUG_KMS("Could not determine valid watermarks for inherited state\n");
> > +		return;
> > +	}
> > +
> > +	/* Write calculated watermark values back */
> > +	to_i915(dev)->wm.config = to_intel_atomic_state(state)->wm_config;
> > +	for_each_crtc_in_state(state, crtc, cstate, i) {
> > +		struct intel_crtc_state *cs = to_intel_crtc_state(cstate);
> > +
> > +		dev_priv->display.program_watermarks(cs);
> > +	}
> > +
> > +	drm_atomic_state_free(state);
> > +	drm_modeset_drop_locks(&ctx);
> > +	drm_modeset_acquire_fini(&ctx);
> > +}
> > +
> >  void intel_modeset_init(struct drm_device *dev)
> >  {
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > @@ -15243,6 +15294,13 @@ void intel_modeset_init(struct drm_device *dev)
> >  		intel_dump_pipe_config(crtc, crtc->config,
> >  				       "[state after init fb reconstruction]");
> >  	}
> > +
> > +	/*
> > +	 * Make sure hardware watermarks really match the state we read out.
> > +	 * Note that we need to do this after reconstructing the BIOS fb's
> > +	 * since the watermark calculation done here will use pstate->fb.
> > +	 */
> > +	sanitize_watermarks(dev);
> >  }
> >  
> >  static void intel_enable_pipe_a(struct drm_device *dev)
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index d4cd5d5..c209a69 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -3610,15 +3610,19 @@ static void skl_update_wm(struct drm_crtc *crtc)
> >  	dev_priv->wm.skl_hw = *results;
> >  }
> >  
> > -static void ilk_program_watermarks(struct drm_i915_private *dev_priv)
> > +static void ilk_program_watermarks(struct intel_crtc_state *cstate)
> >  {
> > -	struct drm_device *dev = dev_priv->dev;
> > +	struct drm_crtc *crtc = cstate->base.crtc;
> > +	struct drm_device *dev = crtc->dev;
> > +	struct drm_i915_private *dev_priv = to_i915(dev);
> >  	struct intel_pipe_wm lp_wm_1_2 = {}, lp_wm_5_6 = {}, *best_lp_wm;
> >  	struct ilk_wm_maximums max;
> >  	struct intel_wm_config *config = &dev_priv->wm.config;
> >  	struct ilk_wm_values results = {};
> >  	enum intel_ddb_partitioning partitioning;
> >  
> > +	to_intel_crtc(crtc)->wm.active.ilk = cstate->wm.optimal.ilk;
> > +
> >  	ilk_compute_wm_maximums(dev, 1, config, INTEL_DDB_PART_1_2, &max);
> >  	ilk_wm_merge(dev, config, &max, &lp_wm_1_2);
> >  
> > @@ -3643,7 +3647,6 @@ static void ilk_program_watermarks(struct drm_i915_private *dev_priv)
> >  
> >  static void ilk_update_wm(struct drm_crtc *crtc)
> >  {
> > -	struct drm_i915_private *dev_priv = to_i915(crtc->dev);
> >  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> >  	struct intel_crtc_state *cstate = to_intel_crtc_state(crtc->state);
> >  
> > @@ -3661,9 +3664,7 @@ static void ilk_update_wm(struct drm_crtc *crtc)
> >  		intel_wait_for_vblank(crtc->dev, intel_crtc->pipe);
> >  	}
> >  
> > -	intel_crtc->wm.active.ilk = cstate->wm.optimal.ilk;
> > -
> > -	ilk_program_watermarks(dev_priv);
> > +	ilk_program_watermarks(cstate);
> >  }
> >  
> >  static void skl_pipe_wm_active_state(uint32_t val,
> > @@ -6971,6 +6972,7 @@ void intel_init_pm(struct drm_device *dev)
> >  		     dev_priv->wm.spr_latency[0] && dev_priv->wm.cur_latency[0])) {
> >  			dev_priv->display.update_wm = ilk_update_wm;
> >  			dev_priv->display.compute_pipe_wm = ilk_compute_pipe_wm;
> > +			dev_priv->display.program_watermarks = ilk_program_watermarks;
> >  		} else {
> >  			DRM_DEBUG_KMS("Failed to read display plane latency. "
> >  				      "Disable CxSR\n");
> 

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH] drm/atomic-helper: Grab connection_mutex while duplicating state

[Matt Roper]

Callers of drm_atomic_helper_duplicate_state() may not have grabbed
locks before calling this function.  We're not supposed to iterate over
connectors without holding connection_mutex (since MST allows new
connectors to be spawned at hotplug), so make sure we grab that
ourselves before invoking drm_for_each_connector().  Failure to grab
this lock would cause us to stumble over the assertion added in commit:

        commit 7a3f3d6667f5f9ffd1517f6b21d64bbf5312042c
        Author: Daniel Vetter <daniel.vetter@ffwll.ch>
        Date:   Thu Jul 9 23:44:28 2015 +0200

            drm: Check locking in drm_for_each_connector

Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/drm_atomic_helper.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 3731a26..e5d0b21 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -2478,6 +2478,10 @@ drm_atomic_helper_duplicate_state(struct drm_device *dev,
 		}
 	}
 
+	err = drm_modeset_lock(&dev->mode_config.connection_mutex, ctx);
+	if (err)
+		goto free;
+
 	drm_for_each_connector(conn, dev) {
 		struct drm_connector_state *conn_state;
 
-- 
2.1.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] drm/i915: Sanitize watermarks after hardware state readout (v3)

[Matt Roper]

Although we can do a good job of reading out hardware state, the
graphics firmware may have programmed the watermarks in a creative way
that doesn't match how i915 would have chosen to program them.  We
shouldn't trust the firmware's watermark programming, but should rather
re-calculate how we think WM's should be programmed and then shove those
values into the hardware.

We can do this pretty easily by creating a dummy top-level state,
running it through the check process to calculate all the values, and
then just programming the watermarks for each CRTC.

v2:  Move watermark sanitization after our BIOS fb reconstruction; the
     watermark calculations that we do here need to look at pstate->fb,
     which isn't setup yet in intel_modeset_setup_hw_state(), even
     though we have an enabled & visible plane.

v3:
 - Don't move 'active = optimal' watermark assignment; we just undo
   that change in the next patch anyway.  (Ville)
 - Move atomic helper locking fix to separate patch.  (Maarten)

Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
The call to drm_atomic_helper_duplicate_state() in this patch will throw a
warning; atomic helper patch "drm/atomic-helper: Grab connection_mutex while
duplicating state" is necessary to fix that.

 drivers/gpu/drm/i915/i915_drv.h      |  1 +
 drivers/gpu/drm/i915/intel_display.c | 58 ++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_pm.c      | 10 ++++---
 3 files changed, 65 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 11ae5a5..5172604 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -630,6 +630,7 @@ struct drm_i915_display_funcs {
 			  struct dpll *best_clock);
 	int (*compute_pipe_wm)(struct intel_crtc *crtc,
 			       struct drm_atomic_state *state);
+	void (*program_watermarks)(struct intel_crtc_state *cstate);
 	void (*update_wm)(struct drm_crtc *crtc);
 	int (*modeset_calc_cdclk)(struct drm_atomic_state *state);
 	void (*modeset_commit_cdclk)(struct drm_atomic_state *state);
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 00e4c37..eb52afa 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -15120,6 +15120,57 @@ void intel_modeset_init_hw(struct drm_device *dev)
 	intel_enable_gt_powersave(dev);
 }
 
+/*
+ * Calculate what we think the watermarks should be for the state we've read
+ * out of the hardware and then immediately program those watermarks so that
+ * we ensure the hardware settings match our internal state.
+ */
+static void sanitize_watermarks(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = to_i915(dev);
+	struct drm_atomic_state *state;
+	struct drm_crtc *crtc;
+	struct drm_crtc_state *cstate;
+	struct drm_modeset_acquire_ctx ctx;
+	int ret;
+	int i;
+
+	/* Only supported on platforms that use atomic watermark design */
+	if (!dev_priv->display.program_watermarks)
+		return;
+
+	/*
+	 * Calculate what we think WM's should be by creating a dummy state and
+	 * running it through the atomic check code.
+	 */
+	drm_modeset_acquire_init(&ctx, 0);
+	state = drm_atomic_helper_duplicate_state(dev, &ctx);
+	if (WARN_ON(IS_ERR(state)))
+		return;
+
+	ret = intel_atomic_check(dev, state);
+	if (ret) {
+		/*
+		 * Just give up and leave watermarks untouched if we get an
+		 * error back from 'check'
+		 */
+		DRM_DEBUG_KMS("Could not determine valid watermarks for inherited state\n");
+		return;
+	}
+
+	/* Write calculated watermark values back */
+	to_i915(dev)->wm.config = to_intel_atomic_state(state)->wm_config;
+	for_each_crtc_in_state(state, crtc, cstate, i) {
+		struct intel_crtc_state *cs = to_intel_crtc_state(cstate);
+
+		dev_priv->display.program_watermarks(cs);
+	}
+
+	drm_atomic_state_free(state);
+	drm_modeset_drop_locks(&ctx);
+	drm_modeset_acquire_fini(&ctx);
+}
+
 void intel_modeset_init(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -15243,6 +15294,13 @@ void intel_modeset_init(struct drm_device *dev)
 		intel_dump_pipe_config(crtc, crtc->config,
 				       "[state after init fb reconstruction]");
 	}
+
+	/*
+	 * Make sure hardware watermarks really match the state we read out.
+	 * Note that we need to do this after reconstructing the BIOS fb's
+	 * since the watermark calculation done here will use pstate->fb.
+	 */
+	sanitize_watermarks(dev);
 }
 
 static void intel_enable_pipe_a(struct drm_device *dev)
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index d4cd5d5..6337cbf 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3610,9 +3610,11 @@ static void skl_update_wm(struct drm_crtc *crtc)
 	dev_priv->wm.skl_hw = *results;
 }
 
-static void ilk_program_watermarks(struct drm_i915_private *dev_priv)
+static void ilk_program_watermarks(struct intel_crtc_state *cstate)
 {
-	struct drm_device *dev = dev_priv->dev;
+	struct drm_crtc *crtc = cstate->base.crtc;
+	struct drm_device *dev = crtc->dev;
+	struct drm_i915_private *dev_priv = to_i915(dev);
 	struct intel_pipe_wm lp_wm_1_2 = {}, lp_wm_5_6 = {}, *best_lp_wm;
 	struct ilk_wm_maximums max;
 	struct intel_wm_config *config = &dev_priv->wm.config;
@@ -3643,7 +3645,6 @@ static void ilk_program_watermarks(struct drm_i915_private *dev_priv)
 
 static void ilk_update_wm(struct drm_crtc *crtc)
 {
-	struct drm_i915_private *dev_priv = to_i915(crtc->dev);
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	struct intel_crtc_state *cstate = to_intel_crtc_state(crtc->state);
 
@@ -3663,7 +3664,7 @@ static void ilk_update_wm(struct drm_crtc *crtc)
 
 	intel_crtc->wm.active.ilk = cstate->wm.optimal.ilk;
 
-	ilk_program_watermarks(dev_priv);
+	ilk_program_watermarks(cstate);
 }
 
 static void skl_pipe_wm_active_state(uint32_t val,
@@ -6971,6 +6972,7 @@ void intel_init_pm(struct drm_device *dev)
 		     dev_priv->wm.spr_latency[0] && dev_priv->wm.cur_latency[0])) {
 			dev_priv->display.update_wm = ilk_update_wm;
 			dev_priv->display.compute_pipe_wm = ilk_compute_pipe_wm;
+			dev_priv->display.program_watermarks = ilk_program_watermarks;
 		} else {
 			DRM_DEBUG_KMS("Failed to read display plane latency. "
 				      "Disable CxSR\n");
-- 
2.1.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 9/9] drm/i915: Add two-stage ILK-style watermark programming (v7)

[Matt Roper]

On Wed, Nov 25, 2015 at 07:08:53PM +0200, Ville Syrjälä wrote:
> On Wed, Nov 25, 2015 at 08:48:35AM -0800, Matt Roper wrote:
> > In addition to calculating final watermarks, let's also pre-calculate a
> > set of intermediate watermark values at atomic check time.  These
> > intermediate watermarks are a combination of the watermarks for the old
> > state and the new state; they should satisfy the requirements of both
> > states which means they can be programmed immediately when we commit the
> > atomic state (without waiting for a vblank).  Once the vblank does
> > happen, we can then re-program watermarks to the more optimal final
> > value.
> > 
> > v2: Significant rebasing/rewriting.
> > 
> > v3:
> >  - Move 'need_postvbl_update' flag to CRTC state (Daniel)
> >  - Don't forget to check intermediate watermark values for validity
> >    (Maarten)
> >  - Don't due async watermark optimization; just do it at the end of the
> >    atomic transaction, after waiting for vblanks.  We do want it to be
> >    async eventually, but adding that now will cause more trouble for
> >    Maarten's in-progress work.  (Maarten)
> >  - Don't allocate space in crtc_state for intermediate watermarks on
> >    platforms that don't need it (gen9+).
> >  - Move WaCxSRDisabledForSpriteScaling:ivb into intel_begin_crtc_commit
> >    now that ilk_update_wm is gone.
> > 
> > v4:
> >  - Add a wm_mutex to cover updates to intel_crtc->active and the
> >    need_postvbl_update flag.  Since we don't have async yet it isn't
> >    terribly important yet, but might as well add it now.
> >  - Change interface to program watermarks.  Platforms will now expose
> >    .initial_watermarks() and .optimize_watermarks() functions to do
> >    watermark programming.  These should lock wm_mutex, copy the
> >    appropriate state values into intel_crtc->active, and then call
> >    the internal program watermarks function.
> > 
> > v5:
> >  - Skip intermediate watermark calculation/check during initial hardware
> >    readout since we don't trust the existing HW values (and don't have
> >    valid values of our own yet).
> >  - Don't try to call .optimize_watermarks() on platforms that don't have
> >    atomic watermarks yet.  (Maarten)
> > 
> > v6:
> >  - Rebase
> > 
> > v7:
> >  - Further rebase
> > 
> > Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> > Reviewed-by(v5): Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h      |   6 +-
> >  drivers/gpu/drm/i915/intel_atomic.c  |   1 +
> >  drivers/gpu/drm/i915/intel_display.c |  90 +++++++++++++++++++-
> >  drivers/gpu/drm/i915/intel_drv.h     |  31 ++++++-
> >  drivers/gpu/drm/i915/intel_pm.c      | 160 ++++++++++++++++++++++++-----------
> >  5 files changed, 232 insertions(+), 56 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 5172604..427b488 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -630,7 +630,11 @@ struct drm_i915_display_funcs {
> >  			  struct dpll *best_clock);
> >  	int (*compute_pipe_wm)(struct intel_crtc *crtc,
> >  			       struct drm_atomic_state *state);
> > -	void (*program_watermarks)(struct intel_crtc_state *cstate);
> > +	int (*compute_intermediate_wm)(struct drm_device *dev,
> > +				       struct intel_crtc *intel_crtc,
> > +				       struct intel_crtc_state *newstate);
> > +	void (*initial_watermarks)(struct intel_crtc_state *cstate);
> > +	void (*optimize_watermarks)(struct intel_crtc_state *cstate);
> >  	void (*update_wm)(struct drm_crtc *crtc);
> >  	int (*modeset_calc_cdclk)(struct drm_atomic_state *state);
> >  	void (*modeset_commit_cdclk)(struct drm_atomic_state *state);
> > diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
> > index 643f342..b91e166 100644
> > --- a/drivers/gpu/drm/i915/intel_atomic.c
> > +++ b/drivers/gpu/drm/i915/intel_atomic.c
> > @@ -95,6 +95,7 @@ intel_crtc_duplicate_state(struct drm_crtc *crtc)
> >  
> >  	crtc_state->update_pipe = false;
> >  	crtc_state->disable_lp_wm = false;
> > +	crtc_state->wm.need_postvbl_update = false;
> >  
> >  	return &crtc_state->base;
> >  }
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index eb52afa..8db0486 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -11801,6 +11801,12 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
> >  		intel_crtc->atomic.update_wm_pre = true;
> >  	}
> >  
> > +	/* Pre-gen9 platforms need two-step watermark updates */
> > +	if ((intel_crtc->atomic.update_wm_pre || intel_crtc->atomic.update_wm_post) &&
> > +	    INTEL_INFO(dev)->gen < 9 &&
> > +	    dev_priv->display.optimize_watermarks)
> > +		to_intel_crtc_state(crtc_state)->wm.need_postvbl_update = true;
> > +
> >  	if (visible || was_visible)
> >  		intel_crtc->atomic.fb_bits |=
> >  			to_intel_plane(plane)->frontbuffer_bit;
> > @@ -11957,8 +11963,29 @@ static int intel_crtc_atomic_check(struct drm_crtc *crtc,
> >  	ret = 0;
> >  	if (dev_priv->display.compute_pipe_wm) {
> >  		ret = dev_priv->display.compute_pipe_wm(intel_crtc, state);
> > -		if (ret)
> > +		if (ret) {
> > +			DRM_DEBUG_KMS("Target pipe watermarks are invalid\n");
> >  			return ret;
> > +		}
> > +	}
> > +
> > +	if (dev_priv->display.compute_intermediate_wm &&
> > +	    !to_intel_atomic_state(state)->skip_intermediate_wm) {
> > +		if (WARN_ON(!dev_priv->display.compute_pipe_wm))
> > +			return 0;
> > +
> > +		/*
> > +		 * Calculate 'intermediate' watermarks that satisfy both the
> > +		 * old state and the new state.  We can program these
> > +		 * immediately.
> > +		 */
> > +		ret = dev_priv->display.compute_intermediate_wm(crtc->dev,
> > +								intel_crtc,
> > +								pipe_config);
> > +		if (ret) {
> > +			DRM_DEBUG_KMS("No valid intermediate pipe watermarks are possible\n");
> > +			return ret;
> > +		}
> >  	}
> >  
> >  	if (INTEL_INFO(dev)->gen >= 9) {
> > @@ -13409,6 +13436,7 @@ static int intel_atomic_commit(struct drm_device *dev,
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  	struct drm_crtc_state *crtc_state;
> >  	struct drm_crtc *crtc;
> > +	struct intel_crtc_state *intel_cstate;
> >  	int ret = 0;
> >  	int i;
> >  	bool any_ms = false;
> > @@ -13500,6 +13528,20 @@ static int intel_atomic_commit(struct drm_device *dev,
> >  
> >  	drm_atomic_helper_wait_for_vblanks(dev, state);
> >  
> > +	/*
> > +	 * Now that the vblank has passed, we can go ahead and program the
> > +	 * optimal watermarks on platforms that need two-step watermark
> > +	 * programming.
> > +	 *
> > +	 * TODO: Move this (and other cleanup) to an async worker eventually.
> > +	 */
> > +	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> > +		intel_cstate = to_intel_crtc_state(crtc->state);
> > +
> > +		if (dev_priv->display.optimize_watermarks)
> > +		    dev_priv->display.optimize_watermarks(intel_cstate);
> > +	}
> > +
> >  	mutex_lock(&dev->struct_mutex);
> >  	drm_atomic_helper_cleanup_planes(dev, state);
> >  	mutex_unlock(&dev->struct_mutex);
> > @@ -13862,13 +13904,44 @@ 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 *cstate = to_intel_crtc_state(crtc->state);
> >  	struct intel_crtc_state *old_intel_state =
> >  		to_intel_crtc_state(old_crtc_state);
> >  	bool modeset = needs_modeset(crtc->state);
> >  
> > -	if (intel_crtc->atomic.update_wm_pre)
> > +	/*
> > +	 * IVB workaround: must disable low power watermarks for at least
> > +	 * one frame before enabling scaling.  LP watermarks can be re-enabled
> > +	 * when scaling is disabled.
> > +	 *
> > +	 * WaCxSRDisabledForSpriteScaling:ivb
> > +	 */
> > +	if (cstate->disable_lp_wm) {
> > +		ilk_disable_lp_wm(crtc->dev);
> > +		intel_wait_for_vblank(crtc->dev, intel_crtc->pipe);
> > +	}
> > +
> > +	/*
> > +	 * For platforms that support atomic watermarks, program the
> > +	 * 'intermediate' watermarks immediately.  On pre-gen9 platforms, these
> > +	 * will be the intermediate values that are safe for both pre- and
> > +	 * post- vblank; when vblank happens, the 'active' values will be set
> > +	 * to the final 'target' values and we'll do this again to get the
> > +	 * optimal watermarks.  For gen9+ platforms, the values we program here
> > +	 * will be the final target values which will get automatically latched
> > +	 * at vblank time; no further programming will be necessary.
> > +	 *
> > +	 * If a platform hasn't been transitioned to atomic watermarks yet,
> > +	 * we'll continue to update watermarks the old way, if flags tell
> > +	 * us to.
> > +	 */
> > +	if (dev_priv->display.initial_watermarks != NULL) {
> > +		dev_priv->display.initial_watermarks(cstate);
> > +	} else if (intel_crtc->atomic.update_wm_pre) {
> >  		intel_update_watermarks(crtc);
> > +	}
> >  
> >  	/* Perform vblank evasion around commit operation */
> >  	intel_pipe_update_start(intel_crtc);
> > @@ -14207,6 +14280,7 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
> >  	intel_crtc->cursor_size = ~0;
> >  
> >  	intel_crtc->wm.cxsr_allowed = true;
> > +	mutex_init(&intel_crtc->wm.wm_mutex);
> >  
> >  	BUG_ON(pipe >= ARRAY_SIZE(dev_priv->plane_to_crtc_mapping) ||
> >  	       dev_priv->plane_to_crtc_mapping[intel_crtc->plane] != NULL);
> > @@ -15136,7 +15210,7 @@ static void sanitize_watermarks(struct drm_device *dev)
> >  	int i;
> >  
> >  	/* Only supported on platforms that use atomic watermark design */
> > -	if (!dev_priv->display.program_watermarks)
> > +	if (!dev_priv->display.optimize_watermarks)
> >  		return;
> >  
> >  	/*
> > @@ -15148,6 +15222,13 @@ static void sanitize_watermarks(struct drm_device *dev)
> >  	if (WARN_ON(IS_ERR(state)))
> >  		return;
> >  
> > +	/*
> > +	 * Hardware readout is the only time we don't want to calculate
> > +	 * intermediate watermarks (since we don't trust the current
> > +	 * watermarks).
> > +	 */
> > +	to_intel_atomic_state(state)->skip_intermediate_wm = true;
> > +
> >  	ret = intel_atomic_check(dev, state);
> >  	if (ret) {
> >  		/*
> > @@ -15163,7 +15244,8 @@ static void sanitize_watermarks(struct drm_device *dev)
> >  	for_each_crtc_in_state(state, crtc, cstate, i) {
> >  		struct intel_crtc_state *cs = to_intel_crtc_state(cstate);
> >  
> > -		dev_priv->display.program_watermarks(cs);
> > +		cs->wm.need_postvbl_update = true;
> > +		dev_priv->display.optimize_watermarks(cs);
> >  	}
> >  
> >  	drm_atomic_state_free(state);
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index 8fae824..c53eb10 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -249,6 +249,12 @@ struct intel_atomic_state {
> >  	bool dpll_set;
> >  	struct intel_shared_dpll_config shared_dpll[I915_NUM_PLLS];
> >  	struct intel_wm_config wm_config;
> > +
> > +	/*
> > +	 * Current watermarks can't be trusted during hardware readout, so
> > +	 * don't bother calculating intermediate watermarks.
> > +	 */
> > +	bool skip_intermediate_wm;
> >  };
> >  
> >  struct intel_plane_state {
> > @@ -491,13 +497,29 @@ struct intel_crtc_state {
> >  
> >  	struct {
> >  		/*
> > -		 * optimal watermarks, programmed post-vblank when this state
> > -		 * is committed
> > +		 * Optimal watermarks, programmed post-vblank when this state
> > +		 * is committed.
> >  		 */
> >  		union {
> >  			struct intel_pipe_wm ilk;
> >  			struct skl_pipe_wm skl;
> >  		} optimal;
> > +
> > +		/*
> > +		 * Intermediate watermarks; these can be programmed immediately
> > +		 * since they satisfy both the current configuration we're
> > +		 * switching away from and the new configuration we're switching
> > +		 * to.
> > +		 */
> > +		struct intel_pipe_wm intermediate;
> > +
> > +		/*
> > +		 * Platforms with two-step watermark programming will need to
> > +		 * update watermark programming post-vblank to switch from the
> > +		 * safe intermediate watermarks to the optimal final
> > +		 * watermarks.
> > +		 */
> > +		bool need_postvbl_update;
> >  	} wm;
> >  };
> >  
> > @@ -587,8 +609,12 @@ struct intel_crtc {
> >  			struct intel_pipe_wm ilk;
> >  			struct skl_pipe_wm skl;
> >  		} active;
> > +
> >  		/* allow CxSR on this pipe */
> >  		bool cxsr_allowed;
> > +
> > +		/* Protects active and need_postvbl_update */
> > +		struct mutex wm_mutex;
> >  	} wm;
> >  
> >  	int scanline_offset;
> > @@ -1463,6 +1489,7 @@ void skl_wm_get_hw_state(struct drm_device *dev);
> >  void skl_ddb_get_hw_state(struct drm_i915_private *dev_priv,
> >  			  struct skl_ddb_allocation *ddb /* out */);
> >  uint32_t ilk_pipe_pixel_rate(const struct intel_crtc_state *pipe_config);
> > +bool ilk_disable_lp_wm(struct drm_device *dev);
> >  
> >  /* intel_sdvo.c */
> >  bool intel_sdvo_init(struct drm_device *dev,
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index c209a69..29d725e 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -2265,6 +2265,29 @@ static void skl_setup_wm_latency(struct drm_device *dev)
> >  	intel_print_wm_latency(dev, "Gen9 Plane", dev_priv->wm.skl_latency);
> >  }
> >  
> > +static bool ilk_validate_pipe_wm(struct drm_device *dev,
> > +				 struct intel_pipe_wm *pipe_wm)
> > +{
> > +	/* LP0 watermark maximums depend on this pipe alone */
> > +	const struct intel_wm_config config = {
> > +		.num_pipes_active = 1,
> > +		.sprites_enabled = pipe_wm->sprites_enabled,
> > +		.sprites_scaled = pipe_wm->sprites_scaled,
> > +	};
> > +	struct ilk_wm_maximums max;
> > +
> > +	/* LP0 watermarks always use 1/2 DDB partitioning */
> > +	ilk_compute_wm_maximums(dev, 0, &config, INTEL_DDB_PART_1_2, &max);
> > +
> > +	/* At least LP0 must be valid */
> > +	if (!ilk_validate_wm_level(0, &max, &pipe_wm->wm[0])) {
> > +		DRM_DEBUG_KMS("LP0 watermark invalid\n");
> > +		return false;
> > +	}
> > +
> > +	return true;
> > +}
> > +
> >  /* Compute new watermarks for the pipe */
> >  static int ilk_compute_pipe_wm(struct intel_crtc *intel_crtc,
> >  			       struct drm_atomic_state *state)
> > @@ -2279,10 +2302,6 @@ static int ilk_compute_pipe_wm(struct intel_crtc *intel_crtc,
> >  	struct intel_plane_state *sprstate = NULL;
> >  	struct intel_plane_state *curstate = NULL;
> >  	int level, max_level = ilk_wm_max_level(dev);
> > -	/* LP0 watermark maximums depend on this pipe alone */
> > -	struct intel_wm_config config = {
> > -		.num_pipes_active = 1,
> > -	};
> >  	struct ilk_wm_maximums max;
> >  
> >  	cstate = intel_atomic_get_crtc_state(state, intel_crtc);
> > @@ -2305,21 +2324,18 @@ static int ilk_compute_pipe_wm(struct intel_crtc *intel_crtc,
> >  			curstate = to_intel_plane_state(ps);
> >  	}
> >  
> > -	config.sprites_enabled = sprstate->visible;
> > -	config.sprites_scaled = sprstate->visible &&
> > +	pipe_wm->pipe_enabled = cstate->base.active;
> > +	pipe_wm->sprites_enabled = sprstate->visible;
> > +	pipe_wm->sprites_scaled = sprstate->visible &&
> >  		(drm_rect_width(&sprstate->dst) != drm_rect_width(&sprstate->src) >> 16 ||
> >  		drm_rect_height(&sprstate->dst) != drm_rect_height(&sprstate->src) >> 16);
> >  
> > -	pipe_wm->pipe_enabled = cstate->base.active;
> > -	pipe_wm->sprites_enabled = config.sprites_enabled;
> > -	pipe_wm->sprites_scaled = config.sprites_scaled;
> > -
> >  	/* ILK/SNB: LP2+ watermarks only w/o sprites */
> >  	if (INTEL_INFO(dev)->gen <= 6 && sprstate->visible)
> >  		max_level = 1;
> >  
> >  	/* ILK/SNB/IVB: LP1+ watermarks only w/o scaling */
> > -	if (config.sprites_scaled)
> > +	if (pipe_wm->sprites_scaled)
> >  		max_level = 0;
> >  
> >  	ilk_compute_wm_level(dev_priv, intel_crtc, 0, cstate,
> > @@ -2328,12 +2344,8 @@ static int ilk_compute_pipe_wm(struct intel_crtc *intel_crtc,
> >  	if (IS_HASWELL(dev) || IS_BROADWELL(dev))
> >  		pipe_wm->linetime = hsw_compute_linetime_wm(dev, cstate);
> >  
> > -	/* LP0 watermarks always use 1/2 DDB partitioning */
> > -	ilk_compute_wm_maximums(dev, 0, &config, INTEL_DDB_PART_1_2, &max);
> > -
> > -	/* At least LP0 must be valid */
> > -	if (!ilk_validate_wm_level(0, &max, &pipe_wm->wm[0]))
> > -		return -EINVAL;
> > +	if (!ilk_validate_pipe_wm(dev, pipe_wm))
> > +		return false;
> >  
> >  	ilk_compute_wm_reg_maximums(dev, 1, &max);
> >  
> > @@ -2358,6 +2370,59 @@ static int ilk_compute_pipe_wm(struct intel_crtc *intel_crtc,
> >  }
> >  
> >  /*
> > + * Build a set of 'intermediate' watermark values that satisfy both the old
> > + * state and the new state.  These can be programmed to the hardware
> > + * immediately.
> > + */
> > +static int ilk_compute_intermediate_wm(struct drm_device *dev,
> > +				       struct intel_crtc *intel_crtc,
> > +				       struct intel_crtc_state *newstate)
> > +{
> > +	struct intel_pipe_wm *a = &newstate->wm.intermediate;
> > +	struct intel_pipe_wm *b = &intel_crtc->wm.active.ilk;
> > +	int level, max_level = ilk_wm_max_level(dev);
> > +
> > +	/*
> > +	 * Start with the final, target watermarks, then combine with the
> > +	 * currently active watermarks to get values that are safe both before
> > +	 * and after the vblank.
> > +	 */
> > +	*a = newstate->wm.optimal.ilk;
> > +	a->pipe_enabled |= b->pipe_enabled;
> > +	a->sprites_enabled |= b->sprites_enabled;
> > +	a->sprites_scaled |= b->sprites_scaled;
> > +
> > +	for (level = 0; level <= max_level; level++) {
> > +		struct intel_wm_level *a_wm = &a->wm[level];
> > +		const struct intel_wm_level *b_wm = &b->wm[level];
> > +
> > +		a_wm->enable &= b_wm->enable;
> > +		a_wm->pri_val = max(a_wm->pri_val, b_wm->pri_val);
> > +		a_wm->spr_val = max(a_wm->spr_val, b_wm->spr_val);
> > +		a_wm->cur_val = max(a_wm->cur_val, b_wm->cur_val);
> > +		a_wm->fbc_val = max(a_wm->fbc_val, b_wm->fbc_val);
> > +	}
> > +
> > +	/*
> > +	 * We need to make sure that these merged watermark values are
> > +	 * actually a valid configuration themselves.  If they're not,
> > +	 * there's no safe way to transition from the old state to
> > +	 * the new state, so we need to fail the atomic transaction.
> > +	 */
> > +	if (!ilk_validate_pipe_wm(dev, a))
> > +		return -EINVAL;
> > +
> > +	/*
> > +	 * If our intermediate WM are identical to the final WM, then we can
> > +	 * omit the post-vblank programming; only update if it's different.
> > +	 */
> > +	if (memcmp(a, &newstate->wm.optimal.ilk, sizeof *a) != 0)
> > +		newstate->wm.need_postvbl_update = false;
> > +
> > +	return 0;
> > +}
> > +
> > +/*
> >   * Merge the watermarks from all active pipes for a specific level.
> >   */
> >  static void ilk_merge_wm_level(struct drm_device *dev,
> > @@ -2369,9 +2434,7 @@ static void ilk_merge_wm_level(struct drm_device *dev,
> >  	ret_wm->enable = true;
> >  
> >  	for_each_intel_crtc(dev, intel_crtc) {
> > -		const struct intel_crtc_state *cstate =
> > -			to_intel_crtc_state(intel_crtc->base.state);
> > -		const struct intel_pipe_wm *active = &cstate->wm.optimal.ilk;
> > +		const struct intel_pipe_wm *active = &intel_crtc->wm.active.ilk;
> >  		const struct intel_wm_level *wm = &active->wm[level];
> >  
> >  		if (!active->pipe_enabled)
> > @@ -2519,15 +2582,13 @@ static void ilk_compute_wm_results(struct drm_device *dev,
> >  
> >  	/* LP0 register values */
> >  	for_each_intel_crtc(dev, intel_crtc) {
> > -		const struct intel_crtc_state *cstate =
> > -			to_intel_crtc_state(intel_crtc->base.state);
> >  		enum pipe pipe = intel_crtc->pipe;
> > -		const struct intel_wm_level *r = &cstate->wm.optimal.ilk.wm[0];
> > +		const struct intel_wm_level *r = &intel_crtc->wm.active.ilk.wm[0];
> >  
> >  		if (WARN_ON(!r->enable))
> >  			continue;
> >  
> > -		results->wm_linetime[pipe] = cstate->wm.optimal.ilk.linetime;
> > +		results->wm_linetime[pipe] = intel_crtc->wm.active.ilk.linetime;
> >  
> >  		results->wm_pipe[pipe] =
> >  			(r->pri_val << WM0_PIPE_PLANE_SHIFT) |
> > @@ -2734,7 +2795,7 @@ static void ilk_write_wm_values(struct drm_i915_private *dev_priv,
> >  	dev_priv->wm.hw = *results;
> >  }
> >  
> > -static bool ilk_disable_lp_wm(struct drm_device *dev)
> > +bool ilk_disable_lp_wm(struct drm_device *dev)
> >  {
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  
> > @@ -3610,19 +3671,15 @@ static void skl_update_wm(struct drm_crtc *crtc)
> >  	dev_priv->wm.skl_hw = *results;
> >  }
> >  
> > -static void ilk_program_watermarks(struct intel_crtc_state *cstate)
> > +static void ilk_program_watermarks(struct drm_i915_private *dev_priv)
> >  {
> > -	struct drm_crtc *crtc = cstate->base.crtc;
> > -	struct drm_device *dev = crtc->dev;
> > -	struct drm_i915_private *dev_priv = to_i915(dev);
> > +	struct drm_device *dev = dev_priv->dev;
> >  	struct intel_pipe_wm lp_wm_1_2 = {}, lp_wm_5_6 = {}, *best_lp_wm;
> >  	struct ilk_wm_maximums max;
> >  	struct intel_wm_config *config = &dev_priv->wm.config;
> >  	struct ilk_wm_values results = {};
> >  	enum intel_ddb_partitioning partitioning;
> >  
> > -	to_intel_crtc(crtc)->wm.active.ilk = cstate->wm.optimal.ilk;
> > -
> 
> :) So even you yourself NAKed the previous patch.
> 
> >  	ilk_compute_wm_maximums(dev, 1, config, INTEL_DDB_PART_1_2, &max);
> >  	ilk_wm_merge(dev, config, &max, &lp_wm_1_2);
> >  
> > @@ -3645,26 +3702,29 @@ static void ilk_program_watermarks(struct intel_crtc_state *cstate)
> >  	ilk_write_wm_values(dev_priv, &results);
> >  }
> >  
> > -static void ilk_update_wm(struct drm_crtc *crtc)
> > +static void ilk_initial_watermarks(struct intel_crtc_state *cstate)
> >  {
> > -	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > -	struct intel_crtc_state *cstate = to_intel_crtc_state(crtc->state);
> > +	struct drm_i915_private *dev_priv = to_i915(cstate->base.crtc->dev);
> > +	struct intel_crtc *intel_crtc = to_intel_crtc(cstate->base.crtc);
> >  
> > -	WARN_ON(cstate->base.active != intel_crtc->active);
> > +	mutex_lock(&intel_crtc->wm.wm_mutex);
> > +	intel_crtc->wm.active.ilk = cstate->wm.intermediate;
> > +	ilk_program_watermarks(dev_priv);
> > +	mutex_unlock(&intel_crtc->wm.wm_mutex);
> > +}
> >  
> > -	/*
> > -	 * IVB workaround: must disable low power watermarks for at least
> > -	 * one frame before enabling scaling.  LP watermarks can be re-enabled
> > -	 * when scaling is disabled.
> > -	 *
> > -	 * WaCxSRDisabledForSpriteScaling:ivb
> > -	 */
> > -	if (cstate->disable_lp_wm) {
> > -		ilk_disable_lp_wm(crtc->dev);
> > -		intel_wait_for_vblank(crtc->dev, intel_crtc->pipe);
> > -	}
> > +static void ilk_optimize_watermarks(struct intel_crtc_state *cstate)
> 
> I don't really like that name. Should just have foo_pre and foo_post or
> something along those lines IMO.

My thinking was actually the opposite...things like foo_pre() and
foo_post() are a bit opaque/meaningless to anyone who doesn't know the
intimate details of a specific platform's watermark handling, whereas
"initial_watermarks" and "optimize_watermarks" give a little bit more
indication of why we're bothing to program stuff twice and how they
differ.  I don't feel super strongly about it though, so I can change
the names to something else if you feel it makes more sense.

I guess ultimately the important thing is to split this all out into its
own files and write some detailed kerneldocs on it all; that's supposed
to be my next task anyway.


Matt

> 
> > +{
> > +	struct drm_i915_private *dev_priv = to_i915(cstate->base.crtc->dev);
> > +	struct intel_crtc *intel_crtc = to_intel_crtc(cstate->base.crtc);
> >  
> > -	ilk_program_watermarks(cstate);
> > +	mutex_lock(&intel_crtc->wm.wm_mutex);
> > +	if (cstate->wm.need_postvbl_update) {
> > +		intel_crtc->wm.active.ilk = cstate->wm.optimal.ilk;
> > +		ilk_program_watermarks(dev_priv);
> > +		cstate->wm.need_postvbl_update = false;
> > +	}
> > +	mutex_unlock(&intel_crtc->wm.wm_mutex);
> >  }
> >  
> >  static void skl_pipe_wm_active_state(uint32_t val,
> > @@ -6970,9 +7030,11 @@ void intel_init_pm(struct drm_device *dev)
> >  		     dev_priv->wm.spr_latency[1] && dev_priv->wm.cur_latency[1]) ||
> >  		    (!IS_GEN5(dev) && dev_priv->wm.pri_latency[0] &&
> >  		     dev_priv->wm.spr_latency[0] && dev_priv->wm.cur_latency[0])) {
> > -			dev_priv->display.update_wm = ilk_update_wm;
> >  			dev_priv->display.compute_pipe_wm = ilk_compute_pipe_wm;
> > -			dev_priv->display.program_watermarks = ilk_program_watermarks;
> > +			dev_priv->display.compute_intermediate_wm =
> > +				ilk_compute_intermediate_wm;
> > +			dev_priv->display.initial_watermarks = ilk_initial_watermarks;
> > +			dev_priv->display.optimize_watermarks = ilk_optimize_watermarks;
> >  		} else {
> >  			DRM_DEBUG_KMS("Failed to read display plane latency. "
> >  				      "Disable CxSR\n");
> > -- 
> > 2.1.4
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Ville Syrjälä
> Intel OTC

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/atomic-helper: Grab connection_mutex while duplicating state

[Daniel Vetter]

On Mon, Nov 30, 2015 at 03:22:49PM -0800, Matt Roper wrote:
> Callers of drm_atomic_helper_duplicate_state() may not have grabbed
> locks before calling this function.  We're not supposed to iterate over
> connectors without holding connection_mutex (since MST allows new
> connectors to be spawned at hotplug), so make sure we grab that
> ourselves before invoking drm_for_each_connector().  Failure to grab
> this lock would cause us to stumble over the assertion added in commit:
> 
>         commit 7a3f3d6667f5f9ffd1517f6b21d64bbf5312042c
>         Author: Daniel Vetter <daniel.vetter@ffwll.ch>
>         Date:   Thu Jul 9 23:44:28 2015 +0200
> 
>             drm: Check locking in drm_for_each_connector
> 
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>

These helpers are wip and Thierry is working on some additional
higher-level wrappers for bullet proof save/restore for atomic drivers.
It's very much intentinoal that this function here does _not_ grab any
locks - if it does the higher-level magic would be impossible.
-Daniel

> ---
>  drivers/gpu/drm/drm_atomic_helper.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 3731a26..e5d0b21 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -2478,6 +2478,10 @@ drm_atomic_helper_duplicate_state(struct drm_device *dev,
>  		}
>  	}
>  
> +	err = drm_modeset_lock(&dev->mode_config.connection_mutex, ctx);
> +	if (err)
> +		goto free;
> +
>  	drm_for_each_connector(conn, dev) {
>  		struct drm_connector_state *conn_state;
>  
> -- 
> 2.1.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/atomic-helper: Grab connection_mutex while duplicating state

[Maarten Lankhorst]

Op 01-12-15 om 00:22 schreef Matt Roper:
> Callers of drm_atomic_helper_duplicate_state() may not have grabbed
> locks before calling this function.  We're not supposed to iterate over
> connectors without holding connection_mutex (since MST allows new
> connectors to be spawned at hotplug), so make sure we grab that
> ourselves before invoking drm_for_each_connector().  Failure to grab
> this lock would cause us to stumble over the assertion added in commit:
>
>         commit 7a3f3d6667f5f9ffd1517f6b21d64bbf5312042c
>         Author: Daniel Vetter <daniel.vetter@ffwll.ch>
>         Date:   Thu Jul 9 23:44:28 2015 +0200
>
>             drm: Check locking in drm_for_each_connector
>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
>  drivers/gpu/drm/drm_atomic_helper.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 3731a26..e5d0b21 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -2478,6 +2478,10 @@ drm_atomic_helper_duplicate_state(struct drm_device *dev,
>  		}
>  	}
>  
> +	err = drm_modeset_lock(&dev->mode_config.connection_mutex, ctx);
> +	if (err)
> +		goto free;
> +
>  	drm_for_each_connector(conn, dev) {
>  		struct drm_connector_state *conn_state;
>  
Thanks, that's a lot better than just a random hunk without explanation.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Sanitize watermarks after hardware state readout (v3)

[Maarten Lankhorst]

Op 01-12-15 om 00:56 schreef Matt Roper:
> Although we can do a good job of reading out hardware state, the
> graphics firmware may have programmed the watermarks in a creative way
> that doesn't match how i915 would have chosen to program them.  We
> shouldn't trust the firmware's watermark programming, but should rather
> re-calculate how we think WM's should be programmed and then shove those
> values into the hardware.
>
> We can do this pretty easily by creating a dummy top-level state,
> running it through the check process to calculate all the values, and
> then just programming the watermarks for each CRTC.
>
> v2:  Move watermark sanitization after our BIOS fb reconstruction; the
>      watermark calculations that we do here need to look at pstate->fb,
>      which isn't setup yet in intel_modeset_setup_hw_state(), even
>      though we have an enabled & visible plane.
>
> v3:
>  - Don't move 'active = optimal' watermark assignment; we just undo
>    that change in the next patch anyway.  (Ville)
>  - Move atomic helper locking fix to separate patch.  (Maarten)
>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
> The call to drm_atomic_helper_duplicate_state() in this patch will throw a
> warning; atomic helper patch "drm/atomic-helper: Grab connection_mutex while
> duplicating state" is necessary to fix that.
>
>  drivers/gpu/drm/i915/i915_drv.h      |  1 +
>  drivers/gpu/drm/i915/intel_display.c | 58 ++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_pm.c      | 10 ++++---
>  3 files changed, 65 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 11ae5a5..5172604 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -630,6 +630,7 @@ struct drm_i915_display_funcs {
>  			  struct dpll *best_clock);
>  	int (*compute_pipe_wm)(struct intel_crtc *crtc,
>  			       struct drm_atomic_state *state);
> +	void (*program_watermarks)(struct intel_crtc_state *cstate);
>  	void (*update_wm)(struct drm_crtc *crtc);
>  	int (*modeset_calc_cdclk)(struct drm_atomic_state *state);
>  	void (*modeset_commit_cdclk)(struct drm_atomic_state *state);
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 00e4c37..eb52afa 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -15120,6 +15120,57 @@ void intel_modeset_init_hw(struct drm_device *dev)
>  	intel_enable_gt_powersave(dev);
>  }
>  
> +/*
> + * Calculate what we think the watermarks should be for the state we've read
> + * out of the hardware and then immediately program those watermarks so that
> + * we ensure the hardware settings match our internal state.
> + */
> +static void sanitize_watermarks(struct drm_device *dev)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(dev);
> +	struct drm_atomic_state *state;
> +	struct drm_crtc *crtc;
> +	struct drm_crtc_state *cstate;
> +	struct drm_modeset_acquire_ctx ctx;
> +	int ret;
> +	int i;
> +
> +	/* Only supported on platforms that use atomic watermark design */
> +	if (!dev_priv->display.program_watermarks)
> +		return;
> +
> +	/*
> +	 * Calculate what we think WM's should be by creating a dummy state and
> +	 * running it through the atomic check code.
> +	 */
> +	drm_modeset_acquire_init(&ctx, 0);
> +	state = drm_atomic_helper_duplicate_state(dev, &ctx);
> +	if (WARN_ON(IS_ERR(state)))
> +		return;
> +
> +	ret = intel_atomic_check(dev, state);
> +	if (ret) {
> +		/*
> +		 * Just give up and leave watermarks untouched if we get an
> +		 * error back from 'check'
> +		 */
> +		DRM_DEBUG_KMS("Could not determine valid watermarks for inherited state\n");
Seems like a serious error that should not be shuffled away, maybe print the ret too?
> +		return;
> +	}
> +
> +	/* Write calculated watermark values back */
> +	to_i915(dev)->wm.config = to_intel_atomic_state(state)->wm_config;
> +	for_each_crtc_in_state(state, crtc, cstate, i) {
> +		struct intel_crtc_state *cs = to_intel_crtc_state(cstate);
> +
> +		dev_priv->display.program_watermarks(cs);
> +	}
> +
> +	drm_atomic_state_free(state);
> +	drm_modeset_drop_locks(&ctx);
> +	drm_modeset_acquire_fini(&ctx);
> +}
> +
>  void intel_modeset_init(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -15243,6 +15294,13 @@ void intel_modeset_init(struct drm_device *dev)
>  		intel_dump_pipe_config(crtc, crtc->config,
>  				       "[state after init fb reconstruction]");
>  	}
> +
> +	/*
> +	 * Make sure hardware watermarks really match the state we read out.
> +	 * Note that we need to do this after reconstructing the BIOS fb's
> +	 * since the watermark calculation done here will use pstate->fb.
> +	 */
> +	sanitize_watermarks(dev);
>  }
>  
>  static void intel_enable_pipe_a(struct drm_device *dev)
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index d4cd5d5..6337cbf 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3610,9 +3610,11 @@ static void skl_update_wm(struct drm_crtc *crtc)
>  	dev_priv->wm.skl_hw = *results;
>  }
>  
> -static void ilk_program_watermarks(struct drm_i915_private *dev_priv)
> +static void ilk_program_watermarks(struct intel_crtc_state *cstate)
>  {
> -	struct drm_device *dev = dev_priv->dev;
> +	struct drm_crtc *crtc = cstate->base.crtc;
> +	struct drm_device *dev = crtc->dev;
> +	struct drm_i915_private *dev_priv = to_i915(dev);
>  	struct intel_pipe_wm lp_wm_1_2 = {}, lp_wm_5_6 = {}, *best_lp_wm;
>  	struct ilk_wm_maximums max;
>  	struct intel_wm_config *config = &dev_priv->wm.config;
> @@ -3643,7 +3645,6 @@ static void ilk_program_watermarks(struct drm_i915_private *dev_priv)
>  
>  static void ilk_update_wm(struct drm_crtc *crtc)
>  {
> -	struct drm_i915_private *dev_priv = to_i915(crtc->dev);
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  	struct intel_crtc_state *cstate = to_intel_crtc_state(crtc->state);
>  
> @@ -3663,7 +3664,7 @@ static void ilk_update_wm(struct drm_crtc *crtc)
>  
>  	intel_crtc->wm.active.ilk = cstate->wm.optimal.ilk;
>  
> -	ilk_program_watermarks(dev_priv);
> +	ilk_program_watermarks(cstate);
>  }
>  
>  static void skl_pipe_wm_active_state(uint32_t val,
> @@ -6971,6 +6972,7 @@ void intel_init_pm(struct drm_device *dev)
>  		     dev_priv->wm.spr_latency[0] && dev_priv->wm.cur_latency[0])) {
>  			dev_priv->display.update_wm = ilk_update_wm;
>  			dev_priv->display.compute_pipe_wm = ilk_compute_pipe_wm;
> +			dev_priv->display.program_watermarks = ilk_program_watermarks;
>
Any plans for getting rid of update_wm() ? I really like passing a cstate. :)

~Maarten
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/9] drm/i915: Disable primary plane if we fail to reconstruct BIOS fb

[Maarten Lankhorst]

Op 25-11-15 om 17:48 schreef Matt Roper:
> If we fail to reconstruct the BIOS fb (e.g., because the FB is too
> large), we'll be left with plane state that indicates the primary plane
> is visible yet has a NULL fb.  This mismatch causes problems later on
> (e.g., for the watermark code).  Since we've failed to reconstruct the
> BIOS FB, the best solution is to just disable the primary plane and
> pretend the BIOS never had it enabled.
>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: Ville Syrjälä <ville.syrjala@intel.com>
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
> With this patch, the rest of this series now runs without problems on Jani's
> system where the regressions were originally reported.
>
> Chris pointed out that this might also fix some of the other bugzillas we have
> on older platforms where there's a GPU hang on failed FB takeover.  I don't
> think I have any platforms that can reproduce those types of problems to
> verify, but he listed candidate bugs as:
>
>         https://bugs.freedesktop.org/show_bug.cgi?id=89319
>         https://bugs.freedesktop.org/show_bug.cgi?id=87677
>         https://bugs.freedesktop.org/show_bug.cgi?id=89146
>         https://bugs.freedesktop.org/show_bug.cgi?id=91653
>
>  drivers/gpu/drm/i915/intel_display.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 4b21d5e..d03a235 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2597,6 +2597,8 @@ intel_find_initial_plane_obj(struct intel_crtc *intel_crtc,
>  	struct drm_i915_gem_object *obj;
>  	struct drm_plane *primary = intel_crtc->base.primary;
>  	struct drm_plane_state *plane_state = primary->state;
> +	struct drm_crtc_state *crtc_state = intel_crtc->base.state;
> +	struct intel_plane *intel_plane = to_intel_plane(primary);
>  	struct drm_framebuffer *fb;
>  
>  	if (!plane_config->fb)
> @@ -2633,6 +2635,17 @@ intel_find_initial_plane_obj(struct intel_crtc *intel_crtc,
>  		}
>  	}
>  
> +	/*
> +	 * We've failed to reconstruct the BIOS FB.  Current display state
> +	 * indicates that the primary plane is visible, but has a NULL FB,
> +	 * which will lead to problems later if we don't fix it up.  The
> +	 * simplest solution is to just disable the primary plane now and
> +	 * pretend the BIOS never had it enabled.
> +	 */
> +	to_intel_plane_state(plane_state)->visible = false;
> +	crtc_state->plane_mask &= ~(1 << drm_plane_index(primary));
> +	intel_plane->disable_plane(primary, &intel_crtc->base);
>
I think this should be for -fixes, also can you add add a call to intel_pre_disable_primary ?

If so, Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 5/9] drm/i915: Setup clipped src/dest coordinates during FB reconstruction (v2)

[Maarten Lankhorst]

Op 25-11-15 om 17:48 schreef Matt Roper:
> Plane state objects contain two copies of src/dest coordinates:  the
> original (requested by userspace) coordinates in the base
> drm_plane_state object, and a second, clipped copy (i.e., what we
> actually want to program to the hardware) in intel_plane_state.  We've
> only been setting up the former set of values during boot time FB
> reconstruction, but we should really be initializing both.
>
> Note that the code here probably still needs some more work since we
> make a lot of assumptions about how the BIOS programmed the hardware
> that may not always be true, especially on gen9+; e.g.,
>  * Primary plane might not be positioned at 0,0
>  * Primary plane could have been rotated by the BIOS
>  * Primary plane might be scaled
>  * The BIOS fb might be a single "extended mode" FB that spans
>    multiple displays.
>  * ...etc...
>
> v2: Reword/expand commit message description of assumptions we make
>
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> Reviewed-by(v1): Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 73e9bf9..00e4c37 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2599,6 +2599,8 @@ intel_find_initial_plane_obj(struct intel_crtc *intel_crtc,
>  	struct drm_plane_state *plane_state = primary->state;
>  	struct drm_crtc_state *crtc_state = intel_crtc->base.state;
>  	struct intel_plane *intel_plane = to_intel_plane(primary);
> +	struct intel_plane_state *intel_state =
> +		to_intel_plane_state(plane_state);
>  	struct drm_framebuffer *fb;
>  
>  	if (!plane_config->fb)
> @@ -2659,6 +2661,15 @@ valid_fb:
>  	plane_state->crtc_w = fb->width;
>  	plane_state->crtc_h = fb->height;
>  
> +	intel_state->src.x1 = plane_state->src_x;
> +	intel_state->src.y1 = plane_state->src_y;
> +	intel_state->src.x2 = plane_state->src_x + plane_state->src_w;
> +	intel_state->src.y2 = plane_state->src_y + plane_state->src_h;
> +	intel_state->dst.x1 = plane_state->crtc_x;
> +	intel_state->dst.y1 = plane_state->crtc_y;
> +	intel_state->dst.x2 = plane_state->crtc_x + plane_state->crtc_w;
> +	intel_state->dst.y2 = plane_state->crtc_y + plane_state->crtc_h;
>
Why does it matter that those coordinates are set up? The first atomic commit would overwrite those anyway..
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/9] drm/i915: Dump in-flight plane state while dumping in-flight CRTC state

[Maarten Lankhorst]

Op 25-11-15 om 17:48 schreef Matt Roper:
> The intel_dump_pipe_config() always dumps the currently active plane
> state for each plane on a CRTC.  If we're calling this function to dump
> CRTC state that is in-flight (not yet active), then this mismatch can be
> misleading and confusing.  Let's pay attention to whether the state
> we're dumping is part of an in-flight transaction (because
> crtc_state->state is non-NULL); if it is, we'll dump the corresponding
> in-flight plane state instead of the active state.
>
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index d03a235..0e74287 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -12162,11 +12162,23 @@ static void intel_dump_pipe_config(struct intel_crtc *crtc,
>  
>  	DRM_DEBUG_KMS("planes on this crtc\n");
>  	list_for_each_entry(plane, &dev->mode_config.plane_list, head) {
> +		struct drm_plane_state *ps = NULL;
> +
>  		intel_plane = to_intel_plane(plane);
>  		if (intel_plane->pipe != crtc->pipe)
>  			continue;
>  
> -		state = to_intel_plane_state(plane->state);
> +		/* Get in-flight plane state, if any */
> +		if (pipe_config->base.state)
> +			ps = drm_atomic_get_existing_plane_state(pipe_config->base.state,
> +								 plane);
> +
> +		/* If no in-flight state, use active state instead */
> +		if (!ps)
> +			ps = plane->state;
> +
Could plane state be removed from dump_pipe_config instead? There's no good way to do it race free and
it's better to do it in intel_plane_atomic_calc_changes.

~Maarten
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/9] drm/i915: Clarify plane state during CRTC state dumps (v2)

[Maarten Lankhorst]

Op 25-11-15 om 17:48 schreef Matt Roper:
> During state dumping, list planes that have an FB but are invisible
> (e.g., because they're offscreen or clipped by other planes) as "not
> visible" rather than "enabled."  While we're at it, dump the FB format
> as a human-readable string rather than a hex format code.
>
> v2: Don't add bpp; make format human-readable instead. (Ville)
>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 0e74287..e5c522b 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -12190,13 +12190,15 @@ static void intel_dump_pipe_config(struct intel_crtc *crtc,
>  			continue;
>  		}
>  
> -		DRM_DEBUG_KMS("%s PLANE:%d plane: %u.%u idx: %d enabled",
> +		DRM_DEBUG_KMS("%s PLANE:%d plane: %u.%u idx: %d %s",
>  			plane->type == DRM_PLANE_TYPE_CURSOR ? "CURSOR" : "STANDARD",
>  			plane->base.id, intel_plane->pipe,
>  			crtc->base.primary == plane ? 0 : intel_plane->plane + 1,
> -			drm_plane_index(plane));
> -		DRM_DEBUG_KMS("\tFB:%d, fb = %ux%u format = 0x%x",
> -			fb->base.id, fb->width, fb->height, fb->pixel_format);
> +			drm_plane_index(plane),
> +			state->visible ? "enabled" : "not visible");
> +		DRM_DEBUG_KMS("\tFB:%d, fb = %ux%u format = %s",
> +			fb->base.id, fb->width, fb->height,
> +			drm_get_format_name(fb->pixel_format));
>  		DRM_DEBUG_KMS("\tscaler:%d src (%u, %u) %ux%u dst (%u, %u) %ux%u\n",
>  			state->scaler_id,
>  			state->src.x1 >> 16, state->src.y1 >> 16,
See previous patch. :)
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/9] drm/i915: Dump pipe config after initial FB is reconstructed

[Maarten Lankhorst]

Op 25-11-15 om 17:48 schreef Matt Roper:
> We dump during HW state readout, but that's too early to reflect how the
> primary plane is actually configured.
>
> In the future it would be nice if we could just read out HW state and
> reconstruct FB's at the same point, but at the moment we don't have GEM
> initialized sufficiently during hardware readout.
>
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index e5c522b..73e9bf9 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -15228,6 +15228,9 @@ void intel_modeset_init(struct drm_device *dev)
>  		 * just get the first one.
>  		 */
>  		intel_find_initial_plane_obj(crtc, &plane_config);
> +
> +		intel_dump_pipe_config(crtc, crtc->config,
> +				       "[state after init fb reconstruction]");
>  	}
>  }
>  
If you want to dump pipe_config for this, use a different function for dumping plane state..

~Maarten
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 5/9] drm/i915: Setup clipped src/dest coordinates during FB reconstruction (v2)

[Matt Roper]

On Thu, Dec 03, 2015 at 01:06:44PM +0100, Maarten Lankhorst wrote:
> Op 25-11-15 om 17:48 schreef Matt Roper:
> > Plane state objects contain two copies of src/dest coordinates:  the
> > original (requested by userspace) coordinates in the base
> > drm_plane_state object, and a second, clipped copy (i.e., what we
> > actually want to program to the hardware) in intel_plane_state.  We've
> > only been setting up the former set of values during boot time FB
> > reconstruction, but we should really be initializing both.
> >
> > Note that the code here probably still needs some more work since we
> > make a lot of assumptions about how the BIOS programmed the hardware
> > that may not always be true, especially on gen9+; e.g.,
> >  * Primary plane might not be positioned at 0,0
> >  * Primary plane could have been rotated by the BIOS
> >  * Primary plane might be scaled
> >  * The BIOS fb might be a single "extended mode" FB that spans
> >    multiple displays.
> >  * ...etc...
> >
> > v2: Reword/expand commit message description of assumptions we make
> >
> > Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> > Reviewed-by(v1): Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 11 +++++++++++
> >  1 file changed, 11 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 73e9bf9..00e4c37 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -2599,6 +2599,8 @@ intel_find_initial_plane_obj(struct intel_crtc *intel_crtc,
> >  	struct drm_plane_state *plane_state = primary->state;
> >  	struct drm_crtc_state *crtc_state = intel_crtc->base.state;
> >  	struct intel_plane *intel_plane = to_intel_plane(primary);
> > +	struct intel_plane_state *intel_state =
> > +		to_intel_plane_state(plane_state);
> >  	struct drm_framebuffer *fb;
> >  
> >  	if (!plane_config->fb)
> > @@ -2659,6 +2661,15 @@ valid_fb:
> >  	plane_state->crtc_w = fb->width;
> >  	plane_state->crtc_h = fb->height;
> >  
> > +	intel_state->src.x1 = plane_state->src_x;
> > +	intel_state->src.y1 = plane_state->src_y;
> > +	intel_state->src.x2 = plane_state->src_x + plane_state->src_w;
> > +	intel_state->src.y2 = plane_state->src_y + plane_state->src_h;
> > +	intel_state->dst.x1 = plane_state->crtc_x;
> > +	intel_state->dst.y1 = plane_state->crtc_y;
> > +	intel_state->dst.x2 = plane_state->crtc_x + plane_state->crtc_w;
> > +	intel_state->dst.y2 = plane_state->crtc_y + plane_state->crtc_h;
> >
> Why does it matter that those coordinates are set up? The first atomic commit would overwrite those anyway..

We potentially use these during watermark calculations when trying to
calculate what we think sane watermark values would be for the
hardware-readout state, which is before we get to our first commit.


Matt

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/9] drm/i915: Dump in-flight plane state while dumping in-flight CRTC state

[Daniel Vetter]

On Thu, Dec 03, 2015 at 01:27:52PM +0100, Maarten Lankhorst wrote:
> Op 25-11-15 om 17:48 schreef Matt Roper:
> > The intel_dump_pipe_config() always dumps the currently active plane
> > state for each plane on a CRTC.  If we're calling this function to dump
> > CRTC state that is in-flight (not yet active), then this mismatch can be
> > misleading and confusing.  Let's pay attention to whether the state
> > we're dumping is part of an in-flight transaction (because
> > crtc_state->state is non-NULL); if it is, we'll dump the corresponding
> > in-flight plane state instead of the active state.
> >
> > Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 14 +++++++++++++-
> >  1 file changed, 13 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index d03a235..0e74287 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -12162,11 +12162,23 @@ static void intel_dump_pipe_config(struct intel_crtc *crtc,
> >  
> >  	DRM_DEBUG_KMS("planes on this crtc\n");
> >  	list_for_each_entry(plane, &dev->mode_config.plane_list, head) {
> > +		struct drm_plane_state *ps = NULL;
> > +
> >  		intel_plane = to_intel_plane(plane);
> >  		if (intel_plane->pipe != crtc->pipe)
> >  			continue;
> >  
> > -		state = to_intel_plane_state(plane->state);
> > +		/* Get in-flight plane state, if any */
> > +		if (pipe_config->base.state)
> > +			ps = drm_atomic_get_existing_plane_state(pipe_config->base.state,
> > +								 plane);
> > +
> > +		/* If no in-flight state, use active state instead */
> > +		if (!ps)
> > +			ps = plane->state;
> > +
> Could plane state be removed from dump_pipe_config instead? There's no good way to do it race free and
> it's better to do it in intel_plane_atomic_calc_changes.

Yeah dump_pipe_config is meant for debugging crtc_config mismatches. No
point in dumping plane state. Especially since we can validate all the
plane stuff through CRC based testcases anyway.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx