[PATCH 0/4] Wrap up ILK-style atomic watermarks, try two

[PATCH 0/4] Wrap up ILK-style atomic watermarks, try two

[Matt Roper]

Patches #3 and #4 here are the final two patches from the atomic watermark
series that was posted here:

   http://lists.freedesktop.org/archives/intel-gfx/2015-September/076634.html

We had to pull those out when Jani reported a BDW boot regression (divide by
zero during watermark calculation).  Although we never found a smoking gun for
that divide by zero, I haven't been able to reproduce the issue on a similar
system.  There's been a lot of code churn since that time, so I'm hoping that
we've either already fixed the issue without realizing it, or that the extra
paranoia added in patch #2 here will avoid the crash and highlight the culprit.

The first patch here solves a legitimate bug that could cause a divide-by-zero
(just not the one Jani was seeing).  The second patch adds extra guards on
divide operations to verify our invariants and ensure that bugs elsewhere in
the driver can't lead to a fatal divide-by-zero (at least on the ILK
codepaths).

Please don't merge #3 or #4 here until we at least get a positive test result
from Jani.


Matt Roper (4):
  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
  drm/i915: Add two-stage ILK-style watermark programming (v6)

 drivers/gpu/drm/i915/i915_drv.h      |   5 +
 drivers/gpu/drm/i915/intel_atomic.c  |   1 +
 drivers/gpu/drm/i915/intel_display.c | 137 +++++++++++++++++++++++++-
 drivers/gpu/drm/i915/intel_drv.h     |  31 +++++-
 drivers/gpu/drm/i915/intel_pm.c      | 186 +++++++++++++++++++++++++----------
 5 files changed, 305 insertions(+), 55 deletions(-)

-- 
2.1.4

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

[PATCH 1/4] 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 647c0ff..d9506e2 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 2/4] 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 d9506e2..180348b 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 3/4] drm/i915: Sanitize watermarks after hardware state readout

[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.

Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h      |  1 +
 drivers/gpu/drm/i915/intel_display.c | 51 ++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_pm.c      | 14 +++++-----
 3 files changed, 60 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 20cd6d8..09807c8 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -629,6 +629,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 7b3cfb6..47a67e0 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -15436,6 +15436,54 @@ static void intel_modeset_readout_hw_state(struct drm_device *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;
+	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.
+	 */
+	state = drm_atomic_helper_duplicate_state(dev,
+						  dev->mode_config.acquire_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);
+}
+
 /* Scan out the current hw modeset state,
  * and sanitizes it to the current state
  */
@@ -15491,6 +15539,9 @@ intel_modeset_setup_hw_state(struct drm_device *dev)
 			modeset_put_power_domains(dev_priv, put_domains);
 	}
 	intel_display_set_init_power(dev_priv, false);
+
+	/* Make sure hardware watermarks really match the state we read out */
+	sanitize_watermarks(dev);
 }
 
 void intel_display_resume(struct drm_device *dev)
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 180348b..fbcb072 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3611,15 +3611,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);
 
@@ -3644,7 +3648,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);
 
@@ -3662,9 +3665,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,
@@ -6972,6 +6973,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 4/4] drm/i915: Add two-stage ILK-style watermark programming (v6)

[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

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 09807c8..e9a07d8 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -629,7 +629,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 47a67e0..d50884d 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11659,6 +11659,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;
@@ -11815,8 +11821,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) {
@@ -13247,6 +13274,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;
@@ -13325,6 +13353,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);
@@ -13687,13 +13729,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);
@@ -14032,6 +14105,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);
@@ -15451,7 +15525,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;
 
 	/*
@@ -15463,6 +15537,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) {
 		/*
@@ -15478,7 +15559,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 d1a6071..8392d57 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -251,6 +251,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 {
@@ -493,13 +499,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;
 };
 
@@ -589,8 +611,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;
@@ -1439,6 +1465,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, uint32_t sdvo_reg, bool is_sdvob);
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index fbcb072..ac4cddb 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;
 
@@ -3611,19 +3672,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);
 
@@ -3646,26 +3703,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,
@@ -6971,9 +7031,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

[PATCH 4/4] 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 09807c8..e9a07d8 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -629,7 +629,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 e289311..29ad378 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11659,6 +11659,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;
@@ -11815,8 +11821,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) {
@@ -13247,6 +13274,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;
@@ -13325,6 +13353,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);
@@ -13687,13 +13729,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);
@@ -14032,6 +14105,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);
@@ -14951,7 +15025,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;
 
 	/*
@@ -14963,6 +15037,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) {
 		/*
@@ -14978,7 +15059,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 d1a6071..8392d57 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -251,6 +251,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 {
@@ -493,13 +499,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;
 };
 
@@ -589,8 +611,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;
@@ -1439,6 +1465,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, uint32_t sdvo_reg, bool is_sdvob);
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index fbcb072..ac4cddb 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;
 
@@ -3611,19 +3672,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);
 
@@ -3646,26 +3703,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,
@@ -6971,9 +7031,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/4] Wrap up ILK-style atomic watermarks, try two

[Jani Nikula]

On Tue, 03 Nov 2015, Matt Roper <matthew.d.roper@intel.com> wrote:
> Patches #3 and #4 here are the final two patches from the atomic watermark
> series that was posted here:
>
>    http://lists.freedesktop.org/archives/intel-gfx/2015-September/076634.html
>
> We had to pull those out when Jani reported a BDW boot regression (divide by
> zero during watermark calculation).  Although we never found a smoking gun for
> that divide by zero, I haven't been able to reproduce the issue on a similar
> system.  There's been a lot of code churn since that time, so I'm hoping that
> we've either already fixed the issue without realizing it, or that the extra
> paranoia added in patch #2 here will avoid the crash and highlight the culprit.
>
> The first patch here solves a legitimate bug that could cause a divide-by-zero
> (just not the one Jani was seeing).  The second patch adds extra guards on
> divide operations to verify our invariants and ensure that bugs elsewhere in
> the driver can't lead to a fatal divide-by-zero (at least on the ILK
> codepaths).
>
> Please don't merge #3 or #4 here until we at least get a positive test result
> from Jani.

Still gives me warnings. I didn't try one patch at a time, but here are
the dmesgs before http://pastebin.com/GVG9UC1U and after
http://pastebin.com/1HaXTJ3Z applying the series on top of today's
nightly.

BR,
Jani.


-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 3/4] 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: Jani Nikula <jani.nikula@intel.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
Jani, based on your logs it looks like the culprit is that we're calculating
watermarks at startup time after we've read out preliminary hardware state (so
we know the primary plane is enabled & visible), but before we reconstruct the
BIOS fb (so pstate->fb is NULL).  I think moving the watermark sanitization
later in the process so that we'll have a proper fb setup should hopefully
solve the issue.  Can you test this version when you get a chance?  I'll also
send a rebased patch #4 since the code movement here means that the previous
version won't apply cleanly.

 drivers/gpu/drm/i915/i915_drv.h      |  1 +
 drivers/gpu/drm/i915/intel_display.c | 55 ++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_pm.c      | 14 +++++----
 3 files changed, 64 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 20cd6d8..09807c8 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -629,6 +629,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 7b3cfb6..e289311 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -14936,6 +14936,54 @@ 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;
+	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.
+	 */
+	state = drm_atomic_helper_duplicate_state(dev,
+						  dev->mode_config.acquire_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);
+}
+
 void intel_modeset_init(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -15059,6 +15107,13 @@ void intel_modeset_init(struct drm_device *dev)
 		 */
 		intel_find_initial_plane_obj(crtc, &plane_config);
 	}
+
+	/*
+	 * 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 180348b..fbcb072 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3611,15 +3611,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);
 
@@ -3644,7 +3648,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);
 
@@ -3662,9 +3665,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,
@@ -6972,6 +6973,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 3/4] drm/i915: Sanitize watermarks after hardware state readout (v2)

[Jani Nikula]

On Tue, 03 Nov 2015, Matt Roper <matthew.d.roper@intel.com> 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: Jani Nikula <jani.nikula@intel.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
> Jani, based on your logs it looks like the culprit is that we're calculating
> watermarks at startup time after we've read out preliminary hardware state (so
> we know the primary plane is enabled & visible), but before we reconstruct the
> BIOS fb (so pstate->fb is NULL).  I think moving the watermark sanitization
> later in the process so that we'll have a proper fb setup should hopefully
> solve the issue.  Can you test this version when you get a chance?  I'll also
> send a rebased patch #4 since the code movement here means that the previous
> version won't apply cleanly.

Sorry Matt, black screen with new versions of patches 3-4. Dmesg at
http://pastebin.com/3LXZwvWu

BR,
Jani.



>
>  drivers/gpu/drm/i915/i915_drv.h      |  1 +
>  drivers/gpu/drm/i915/intel_display.c | 55 ++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_pm.c      | 14 +++++----
>  3 files changed, 64 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 20cd6d8..09807c8 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -629,6 +629,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 7b3cfb6..e289311 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -14936,6 +14936,54 @@ 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;
> +	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.
> +	 */
> +	state = drm_atomic_helper_duplicate_state(dev,
> +						  dev->mode_config.acquire_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);
> +}
> +
>  void intel_modeset_init(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -15059,6 +15107,13 @@ void intel_modeset_init(struct drm_device *dev)
>  		 */
>  		intel_find_initial_plane_obj(crtc, &plane_config);
>  	}
> +
> +	/*
> +	 * 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 180348b..fbcb072 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3611,15 +3611,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);
>  
> @@ -3644,7 +3648,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);
>  
> @@ -3662,9 +3665,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,
> @@ -6972,6 +6973,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");

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Matt Roper]

On Wed, Nov 04, 2015 at 04:12:33PM +0200, Jani Nikula wrote:
> On Tue, 03 Nov 2015, Matt Roper <matthew.d.roper@intel.com> 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: Jani Nikula <jani.nikula@intel.com>
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> > ---
> > Jani, based on your logs it looks like the culprit is that we're calculating
> > watermarks at startup time after we've read out preliminary hardware state (so
> > we know the primary plane is enabled & visible), but before we reconstruct the
> > BIOS fb (so pstate->fb is NULL).  I think moving the watermark sanitization
> > later in the process so that we'll have a proper fb setup should hopefully
> > solve the issue.  Can you test this version when you get a chance?  I'll also
> > send a rebased patch #4 since the code movement here means that the previous
> > version won't apply cleanly.
> 
> Sorry Matt, black screen with new versions of patches 3-4. Dmesg at
> http://pastebin.com/3LXZwvWu

Hmm, okay, looks like we're getting closer (successfully avoided the
divide by zero problem), but I neglected to grab the necessary locks so
the sanitization doesn't actually happen.  Does applying
http://paste.debian.net/322932 on top of the series work any better for
you?  I haven't had time to pull back out an ILK-style system, so that's
only compile-tested at the moment.


Matt

> 
> BR,
> Jani.
> 
> 
> 
> >
> >  drivers/gpu/drm/i915/i915_drv.h      |  1 +
> >  drivers/gpu/drm/i915/intel_display.c | 55 ++++++++++++++++++++++++++++++++++++
> >  drivers/gpu/drm/i915/intel_pm.c      | 14 +++++----
> >  3 files changed, 64 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 20cd6d8..09807c8 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -629,6 +629,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 7b3cfb6..e289311 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -14936,6 +14936,54 @@ 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;
> > +	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.
> > +	 */
> > +	state = drm_atomic_helper_duplicate_state(dev,
> > +						  dev->mode_config.acquire_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);
> > +}
> > +
> >  void intel_modeset_init(struct drm_device *dev)
> >  {
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > @@ -15059,6 +15107,13 @@ void intel_modeset_init(struct drm_device *dev)
> >  		 */
> >  		intel_find_initial_plane_obj(crtc, &plane_config);
> >  	}
> > +
> > +	/*
> > +	 * 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 180348b..fbcb072 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -3611,15 +3611,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);
> >  
> > @@ -3644,7 +3648,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);
> >  
> > @@ -3662,9 +3665,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,
> > @@ -6972,6 +6973,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");
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center

-- 
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 3/4] drm/i915: Sanitize watermarks after hardware state readout (v2)

[Jani Nikula]

On Thu, 05 Nov 2015, Matt Roper <matthew.d.roper@intel.com> wrote:
> On Wed, Nov 04, 2015 at 04:12:33PM +0200, Jani Nikula wrote:
>> On Tue, 03 Nov 2015, Matt Roper <matthew.d.roper@intel.com> 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: Jani Nikula <jani.nikula@intel.com>
>> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> > Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
>> > ---
>> > Jani, based on your logs it looks like the culprit is that we're calculating
>> > watermarks at startup time after we've read out preliminary hardware state (so
>> > we know the primary plane is enabled & visible), but before we reconstruct the
>> > BIOS fb (so pstate->fb is NULL).  I think moving the watermark sanitization
>> > later in the process so that we'll have a proper fb setup should hopefully
>> > solve the issue.  Can you test this version when you get a chance?  I'll also
>> > send a rebased patch #4 since the code movement here means that the previous
>> > version won't apply cleanly.
>> 
>> Sorry Matt, black screen with new versions of patches 3-4. Dmesg at
>> http://pastebin.com/3LXZwvWu
>
> Hmm, okay, looks like we're getting closer (successfully avoided the
> divide by zero problem), but I neglected to grab the necessary locks so
> the sanitization doesn't actually happen.  Does applying
> http://paste.debian.net/322932 on top of the series work any better for
> you?  I haven't had time to pull back out an ILK-style system, so that's
> only compile-tested at the moment.

Still warns http://pastebin.com/yGtde5X2

BR,
Jani.


>
>
> Matt
>
>> 
>> BR,
>> Jani.
>> 
>> 
>> 
>> >
>> >  drivers/gpu/drm/i915/i915_drv.h      |  1 +
>> >  drivers/gpu/drm/i915/intel_display.c | 55 ++++++++++++++++++++++++++++++++++++
>> >  drivers/gpu/drm/i915/intel_pm.c      | 14 +++++----
>> >  3 files changed, 64 insertions(+), 6 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> > index 20cd6d8..09807c8 100644
>> > --- a/drivers/gpu/drm/i915/i915_drv.h
>> > +++ b/drivers/gpu/drm/i915/i915_drv.h
>> > @@ -629,6 +629,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 7b3cfb6..e289311 100644
>> > --- a/drivers/gpu/drm/i915/intel_display.c
>> > +++ b/drivers/gpu/drm/i915/intel_display.c
>> > @@ -14936,6 +14936,54 @@ 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;
>> > +	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.
>> > +	 */
>> > +	state = drm_atomic_helper_duplicate_state(dev,
>> > +						  dev->mode_config.acquire_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);
>> > +}
>> > +
>> >  void intel_modeset_init(struct drm_device *dev)
>> >  {
>> >  	struct drm_i915_private *dev_priv = dev->dev_private;
>> > @@ -15059,6 +15107,13 @@ void intel_modeset_init(struct drm_device *dev)
>> >  		 */
>> >  		intel_find_initial_plane_obj(crtc, &plane_config);
>> >  	}
>> > +
>> > +	/*
>> > +	 * 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 180348b..fbcb072 100644
>> > --- a/drivers/gpu/drm/i915/intel_pm.c
>> > +++ b/drivers/gpu/drm/i915/intel_pm.c
>> > @@ -3611,15 +3611,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);
>> >  
>> > @@ -3644,7 +3648,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);
>> >  
>> > @@ -3662,9 +3665,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,
>> > @@ -6972,6 +6973,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");
>> 
>> -- 
>> Jani Nikula, Intel Open Source Technology Center

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Matt Roper]

On Thu, Nov 05, 2015 at 02:55:20PM +0200, Jani Nikula wrote:
> On Thu, 05 Nov 2015, Matt Roper <matthew.d.roper@intel.com> wrote:
> > On Wed, Nov 04, 2015 at 04:12:33PM +0200, Jani Nikula wrote:
> >> On Tue, 03 Nov 2015, Matt Roper <matthew.d.roper@intel.com> 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: Jani Nikula <jani.nikula@intel.com>
> >> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >> > Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> >> > ---
> >> > Jani, based on your logs it looks like the culprit is that we're calculating
> >> > watermarks at startup time after we've read out preliminary hardware state (so
> >> > we know the primary plane is enabled & visible), but before we reconstruct the
> >> > BIOS fb (so pstate->fb is NULL).  I think moving the watermark sanitization
> >> > later in the process so that we'll have a proper fb setup should hopefully
> >> > solve the issue.  Can you test this version when you get a chance?  I'll also
> >> > send a rebased patch #4 since the code movement here means that the previous
> >> > version won't apply cleanly.
> >> 
> >> Sorry Matt, black screen with new versions of patches 3-4. Dmesg at
> >> http://pastebin.com/3LXZwvWu
> >
> > Hmm, okay, looks like we're getting closer (successfully avoided the
> > divide by zero problem), but I neglected to grab the necessary locks so
> > the sanitization doesn't actually happen.  Does applying
> > http://paste.debian.net/322932 on top of the series work any better for
> > you?  I haven't had time to pull back out an ILK-style system, so that's
> > only compile-tested at the moment.
> 
> Still warns http://pastebin.com/yGtde5X2
> 
> BR,
> Jani.

Sorry this is taking so long to zero in on.  When you have some free
time would you mind running tag 'forjani/watermark_debug-v1' from

        https://github.com/mattrope/kernel.git

with debug messages turned on?  I was a bit confused by your earlier
logs because apparently some of our startup-time state dumping was
giving misleading/inaccurate information; hopefully with the changes
here, I'll get more useful debug information and better understand
what's leading to the failures on your system.

Thanks again for your help.


Matt

> 
> 
> >
> >
> > Matt
> >
> >> 
> >> BR,
> >> Jani.
> >> 
> >> 
> >> 
> >> >
> >> >  drivers/gpu/drm/i915/i915_drv.h      |  1 +
> >> >  drivers/gpu/drm/i915/intel_display.c | 55 ++++++++++++++++++++++++++++++++++++
> >> >  drivers/gpu/drm/i915/intel_pm.c      | 14 +++++----
> >> >  3 files changed, 64 insertions(+), 6 deletions(-)
> >> >
> >> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> >> > index 20cd6d8..09807c8 100644
> >> > --- a/drivers/gpu/drm/i915/i915_drv.h
> >> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> >> > @@ -629,6 +629,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 7b3cfb6..e289311 100644
> >> > --- a/drivers/gpu/drm/i915/intel_display.c
> >> > +++ b/drivers/gpu/drm/i915/intel_display.c
> >> > @@ -14936,6 +14936,54 @@ 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;
> >> > +	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.
> >> > +	 */
> >> > +	state = drm_atomic_helper_duplicate_state(dev,
> >> > +						  dev->mode_config.acquire_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);
> >> > +}
> >> > +
> >> >  void intel_modeset_init(struct drm_device *dev)
> >> >  {
> >> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >> > @@ -15059,6 +15107,13 @@ void intel_modeset_init(struct drm_device *dev)
> >> >  		 */
> >> >  		intel_find_initial_plane_obj(crtc, &plane_config);
> >> >  	}
> >> > +
> >> > +	/*
> >> > +	 * 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 180348b..fbcb072 100644
> >> > --- a/drivers/gpu/drm/i915/intel_pm.c
> >> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> >> > @@ -3611,15 +3611,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);
> >> >  
> >> > @@ -3644,7 +3648,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);
> >> >  
> >> > @@ -3662,9 +3665,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,
> >> > @@ -6972,6 +6973,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");
> >> 
> >> -- 
> >> Jani Nikula, Intel Open Source Technology Center
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center

-- 
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 3/4] drm/i915: Sanitize watermarks after hardware state readout (v2)

[Matt Roper]

On Thu, Nov 05, 2015 at 02:55:20PM +0200, Jani Nikula wrote:
> On Thu, 05 Nov 2015, Matt Roper <matthew.d.roper@intel.com> wrote:
> > On Wed, Nov 04, 2015 at 04:12:33PM +0200, Jani Nikula wrote:
> >> On Tue, 03 Nov 2015, Matt Roper <matthew.d.roper@intel.com> 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: Jani Nikula <jani.nikula@intel.com>
> >> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >> > Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> >> > ---
> >> > Jani, based on your logs it looks like the culprit is that we're calculating
> >> > watermarks at startup time after we've read out preliminary hardware state (so
> >> > we know the primary plane is enabled & visible), but before we reconstruct the
> >> > BIOS fb (so pstate->fb is NULL).  I think moving the watermark sanitization
> >> > later in the process so that we'll have a proper fb setup should hopefully
> >> > solve the issue.  Can you test this version when you get a chance?  I'll also
> >> > send a rebased patch #4 since the code movement here means that the previous
> >> > version won't apply cleanly.
> >> 
> >> Sorry Matt, black screen with new versions of patches 3-4. Dmesg at
> >> http://pastebin.com/3LXZwvWu
> >
> > Hmm, okay, looks like we're getting closer (successfully avoided the
> > divide by zero problem), but I neglected to grab the necessary locks so
> > the sanitization doesn't actually happen.  Does applying
> > http://paste.debian.net/322932 on top of the series work any better for
> > you?  I haven't had time to pull back out an ILK-style system, so that's
> > only compile-tested at the moment.
> 
> Still warns http://pastebin.com/yGtde5X2
> 
> BR,
> Jani.

Okay, thanks to Jani setting up his machine so that I can debug
remotely, I think I understand where we're going wrong now.  The missing
piece of the puzzle is that Jani's display is 3840x2160.  That means the
BIOS FB we need to inherit is deemed too large by our initial hardware
readout code (i.e., greater than half of
dev_priv->gtt.stolen_usable_size), so we don't even bother inheriting
the FB and pstate->fb stays NULL, even though pstate->visible = true and
the plane is part of the CRTC's plane_mask.

Adding Maarten, Ville, and Ander to Cc since I think they may have some
insights on how best to handle this.  Seems like our options are:
 * If our watermark code comes up with a NULL FB while pstate->visible,
   just pretend we're working on a 32-bit framebuffer that matches the
   CRTC mode dimensions.  We'll then calculate appropriate watermark
   values, even though we don't have any real information for the FB
   that the hardware is scanning out of.
 * Set pstate->visible = false and clear the bit from plane_mask.  I
   think we'll run into state verification warnings then ("plane A
   should be disabled but is not") since our HW state and SW state don't
   match.
 * Actually turn off the primary plane (hardware-wise) if we fail to
   inherit the BIOS FB.

Seems like the first option may be the simplest.   Thoughts?  Anything
I'm overlooking?


Matt

> 
> 
> >
> >
> > Matt
> >
> >> 
> >> BR,
> >> Jani.
> >> 
> >> 
> >> 
> >> >
> >> >  drivers/gpu/drm/i915/i915_drv.h      |  1 +
> >> >  drivers/gpu/drm/i915/intel_display.c | 55 ++++++++++++++++++++++++++++++++++++
> >> >  drivers/gpu/drm/i915/intel_pm.c      | 14 +++++----
> >> >  3 files changed, 64 insertions(+), 6 deletions(-)
> >> >
> >> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> >> > index 20cd6d8..09807c8 100644
> >> > --- a/drivers/gpu/drm/i915/i915_drv.h
> >> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> >> > @@ -629,6 +629,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 7b3cfb6..e289311 100644
> >> > --- a/drivers/gpu/drm/i915/intel_display.c
> >> > +++ b/drivers/gpu/drm/i915/intel_display.c
> >> > @@ -14936,6 +14936,54 @@ 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;
> >> > +	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.
> >> > +	 */
> >> > +	state = drm_atomic_helper_duplicate_state(dev,
> >> > +						  dev->mode_config.acquire_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);
> >> > +}
> >> > +
> >> >  void intel_modeset_init(struct drm_device *dev)
> >> >  {
> >> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >> > @@ -15059,6 +15107,13 @@ void intel_modeset_init(struct drm_device *dev)
> >> >  		 */
> >> >  		intel_find_initial_plane_obj(crtc, &plane_config);
> >> >  	}
> >> > +
> >> > +	/*
> >> > +	 * 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 180348b..fbcb072 100644
> >> > --- a/drivers/gpu/drm/i915/intel_pm.c
> >> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> >> > @@ -3611,15 +3611,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);
> >> >  
> >> > @@ -3644,7 +3648,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);
> >> >  
> >> > @@ -3662,9 +3665,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,
> >> > @@ -6972,6 +6973,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");
> >> 
> >> -- 
> >> Jani Nikula, Intel Open Source Technology Center
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center

-- 
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 3/4] drm/i915: Sanitize watermarks after hardware state readout (v2)

[Daniel Vetter]

On Thu, Nov 19, 2015 at 08:01:41PM -0800, Matt Roper wrote:
> On Thu, Nov 05, 2015 at 02:55:20PM +0200, Jani Nikula wrote:
> > On Thu, 05 Nov 2015, Matt Roper <matthew.d.roper@intel.com> wrote:
> > > On Wed, Nov 04, 2015 at 04:12:33PM +0200, Jani Nikula wrote:
> > >> On Tue, 03 Nov 2015, Matt Roper <matthew.d.roper@intel.com> 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: Jani Nikula <jani.nikula@intel.com>
> > >> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > >> > Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> > >> > ---
> > >> > Jani, based on your logs it looks like the culprit is that we're calculating
> > >> > watermarks at startup time after we've read out preliminary hardware state (so
> > >> > we know the primary plane is enabled & visible), but before we reconstruct the
> > >> > BIOS fb (so pstate->fb is NULL).  I think moving the watermark sanitization
> > >> > later in the process so that we'll have a proper fb setup should hopefully
> > >> > solve the issue.  Can you test this version when you get a chance?  I'll also
> > >> > send a rebased patch #4 since the code movement here means that the previous
> > >> > version won't apply cleanly.
> > >> 
> > >> Sorry Matt, black screen with new versions of patches 3-4. Dmesg at
> > >> http://pastebin.com/3LXZwvWu
> > >
> > > Hmm, okay, looks like we're getting closer (successfully avoided the
> > > divide by zero problem), but I neglected to grab the necessary locks so
> > > the sanitization doesn't actually happen.  Does applying
> > > http://paste.debian.net/322932 on top of the series work any better for
> > > you?  I haven't had time to pull back out an ILK-style system, so that's
> > > only compile-tested at the moment.
> > 
> > Still warns http://pastebin.com/yGtde5X2
> > 
> > BR,
> > Jani.
> 
> Okay, thanks to Jani setting up his machine so that I can debug
> remotely, I think I understand where we're going wrong now.  The missing
> piece of the puzzle is that Jani's display is 3840x2160.  That means the
> BIOS FB we need to inherit is deemed too large by our initial hardware
> readout code (i.e., greater than half of
> dev_priv->gtt.stolen_usable_size), so we don't even bother inheriting
> the FB and pstate->fb stays NULL, even though pstate->visible = true and
> the plane is part of the CRTC's plane_mask.
> 
> Adding Maarten, Ville, and Ander to Cc since I think they may have some
> insights on how best to handle this.  Seems like our options are:
>  * If our watermark code comes up with a NULL FB while pstate->visible,
>    just pretend we're working on a 32-bit framebuffer that matches the
>    CRTC mode dimensions.  We'll then calculate appropriate watermark
>    values, even though we don't have any real information for the FB
>    that the hardware is scanning out of.
>  * Set pstate->visible = false and clear the bit from plane_mask.  I
>    think we'll run into state verification warnings then ("plane A
>    should be disabled but is not") since our HW state and SW state don't
>    match.
>  * Actually turn off the primary plane (hardware-wise) if we fail to
>    inherit the BIOS FB.

This one. We do clear crtc_state->plane_mask already, but we fail to
disable the hw plane and we also don't reset plane_state->visble.

For added chaos the plane readout/sanitize code is splattered over at
least intel_find_initial_plane_obj, readout_plane_state and
intel_modeset_gem_init. It would be really great if we can at least
somewhat consolidate this.
-Daniel

> 
> Seems like the first option may be the simplest.   Thoughts?  Anything
> I'm overlooking?
> 
> 
> Matt
> 
> > 
> > 
> > >
> > >
> > > Matt
> > >
> > >> 
> > >> BR,
> > >> Jani.
> > >> 
> > >> 
> > >> 
> > >> >
> > >> >  drivers/gpu/drm/i915/i915_drv.h      |  1 +
> > >> >  drivers/gpu/drm/i915/intel_display.c | 55 ++++++++++++++++++++++++++++++++++++
> > >> >  drivers/gpu/drm/i915/intel_pm.c      | 14 +++++----
> > >> >  3 files changed, 64 insertions(+), 6 deletions(-)
> > >> >
> > >> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > >> > index 20cd6d8..09807c8 100644
> > >> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > >> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > >> > @@ -629,6 +629,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 7b3cfb6..e289311 100644
> > >> > --- a/drivers/gpu/drm/i915/intel_display.c
> > >> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > >> > @@ -14936,6 +14936,54 @@ 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;
> > >> > +	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.
> > >> > +	 */
> > >> > +	state = drm_atomic_helper_duplicate_state(dev,
> > >> > +						  dev->mode_config.acquire_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);
> > >> > +}
> > >> > +
> > >> >  void intel_modeset_init(struct drm_device *dev)
> > >> >  {
> > >> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > >> > @@ -15059,6 +15107,13 @@ void intel_modeset_init(struct drm_device *dev)
> > >> >  		 */
> > >> >  		intel_find_initial_plane_obj(crtc, &plane_config);
> > >> >  	}
> > >> > +
> > >> > +	/*
> > >> > +	 * 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 180348b..fbcb072 100644
> > >> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > >> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > >> > @@ -3611,15 +3611,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);
> > >> >  
> > >> > @@ -3644,7 +3648,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);
> > >> >  
> > >> > @@ -3662,9 +3665,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,
> > >> > @@ -6972,6 +6973,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");
> > >> 
> > >> -- 
> > >> Jani Nikula, Intel Open Source Technology Center
> > 
> > -- 
> > Jani Nikula, Intel Open Source Technology Center
> 
> -- 
> 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

-- 
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 3/4] drm/i915: Sanitize watermarks after hardware state readout (v2)

[Ville Syrjälä]

On Thu, Nov 19, 2015 at 08:01:41PM -0800, Matt Roper wrote:
> On Thu, Nov 05, 2015 at 02:55:20PM +0200, Jani Nikula wrote:
> > On Thu, 05 Nov 2015, Matt Roper <matthew.d.roper@intel.com> wrote:
> > > On Wed, Nov 04, 2015 at 04:12:33PM +0200, Jani Nikula wrote:
> > >> On Tue, 03 Nov 2015, Matt Roper <matthew.d.roper@intel.com> 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: Jani Nikula <jani.nikula@intel.com>
> > >> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > >> > Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> > >> > ---
> > >> > Jani, based on your logs it looks like the culprit is that we're calculating
> > >> > watermarks at startup time after we've read out preliminary hardware state (so
> > >> > we know the primary plane is enabled & visible), but before we reconstruct the
> > >> > BIOS fb (so pstate->fb is NULL).  I think moving the watermark sanitization
> > >> > later in the process so that we'll have a proper fb setup should hopefully
> > >> > solve the issue.  Can you test this version when you get a chance?  I'll also
> > >> > send a rebased patch #4 since the code movement here means that the previous
> > >> > version won't apply cleanly.
> > >> 
> > >> Sorry Matt, black screen with new versions of patches 3-4. Dmesg at
> > >> http://pastebin.com/3LXZwvWu
> > >
> > > Hmm, okay, looks like we're getting closer (successfully avoided the
> > > divide by zero problem), but I neglected to grab the necessary locks so
> > > the sanitization doesn't actually happen.  Does applying
> > > http://paste.debian.net/322932 on top of the series work any better for
> > > you?  I haven't had time to pull back out an ILK-style system, so that's
> > > only compile-tested at the moment.
> > 
> > Still warns http://pastebin.com/yGtde5X2
> > 
> > BR,
> > Jani.
> 
> Okay, thanks to Jani setting up his machine so that I can debug
> remotely, I think I understand where we're going wrong now.  The missing
> piece of the puzzle is that Jani's display is 3840x2160.  That means the
> BIOS FB we need to inherit is deemed too large by our initial hardware
> readout code (i.e., greater than half of
> dev_priv->gtt.stolen_usable_size),

I would just revert the patch that added that restriction. IMO it makes
no sense as long as FBC remains disabled by default. And even worse it
doesn't even check that FBC is even supported by the platform. We're
just wasting memory now.

> so we don't even bother inheriting
> the FB and pstate->fb stays NULL, even though pstate->visible = true and
> the plane is part of the CRTC's plane_mask.
> 
> Adding Maarten, Ville, and Ander to Cc since I think they may have some
> insights on how best to handle this.  Seems like our options are:
>  * If our watermark code comes up with a NULL FB while pstate->visible,
>    just pretend we're working on a 32-bit framebuffer that matches the
>    CRTC mode dimensions.  We'll then calculate appropriate watermark
>    values, even though we don't have any real information for the FB
>    that the hardware is scanning out of.
>  * Set pstate->visible = false and clear the bit from plane_mask.  I
>    think we'll run into state verification warnings then ("plane A
>    should be disabled but is not") since our HW state and SW state don't
>    match.
>  * Actually turn off the primary plane (hardware-wise) if we fail to
>    inherit the BIOS FB.

And yes, option 3 is what I also think we should do.

-- 
Ville Syrjälä
Intel OTC
---------------------------------------------------------------------
Intel Finland Oy
Registered Address: PL 281, 00181 Helsinki 
Business Identity Code: 0357606 - 4 
Domiciled in Helsinki 

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

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