From d93c037246104e403436ffe339bcb832185d0627 Mon Sep 17 00:00:00 2001 From: Matt Roper Date: Thu, 3 Dec 2015 11:37:41 -0800 Subject: [PATCH] drm/i915: Sanitize watermarks after hardware state readout (v4) MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit Although we can do a good job of reading out hardware state, the graphics firmware may have programmed the watermarks in a creative way that doesn't match how i915 would have chosen to program them. We shouldn't trust the firmware's watermark programming, but should rather re-calculate how we think WM's should be programmed and then shove those values into the hardware. We can do this pretty easily by creating a dummy top-level state, running it through the check process to calculate all the values, and then just programming the watermarks for each CRTC. v2: Move watermark sanitization after our BIOS fb reconstruction; the watermark calculations that we do here need to look at pstate->fb, which isn't setup yet in intel_modeset_setup_hw_state(), even though we have an enabled & visible plane. v3: - Don't move 'active = optimal' watermark assignment; we just undo that change in the next patch anyway. (Ville) - Move atomic helper locking fix to separate patch. (Maarten) v4: - Grab connection_mutex before calling atomic helper to duplicate state. The connector loop inside the helper will throw a WARN if we don't hold something to protect the connector list (and the helper itself doesn't try to lock the list). - Make failure to calculate watermarks for inherited state a WARN() since it probably indicates a serious problem in either our state readout code or our watermark code for this platform. Cc: Maarten Lankhorst Cc: Ville Syrjälä Signed-off-by: Matt Roper Signed-off-by: Maarten Lankhorst --- drivers/gpu/drm/i915/i915_drv.h | 1 + drivers/gpu/drm/i915/intel_display.c | 79 ++++++++++++++++++++++++++++ drivers/gpu/drm/i915/intel_pm.c | 10 ++-- 3 files changed, 86 insertions(+), 4 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index c6dd4db29cea..bde9c764b415 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -623,6 +623,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 c1a3969f3d4c..043adfa65203 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -15276,6 +15276,78 @@ 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. + * + * We can calculate what we think WM's should be by creating a duplicate of the + * current state (which was constructed during hardware readout) and running it + * through the atomic check code to calculate new watermark values in the + * state object. + */ +static void sanitize_watermarks(struct drm_device *dev) +{ + struct drm_i915_private *dev_priv = to_i915(dev); + struct drm_atomic_state *state; + struct drm_crtc *crtc; + struct drm_crtc_state *cstate; + struct drm_modeset_acquire_ctx ctx; + int ret; + int i; + + /* Only supported on platforms that use atomic watermark design */ + if (!dev_priv->display.program_watermarks) + return; + + /* + * We need to hold connection_mutex before calling duplicate_state so + * that the connector loop is protected. + */ + drm_modeset_acquire_init(&ctx, 0); +retry: + ret = drm_modeset_lock(&dev->mode_config.connection_mutex, &ctx); + if (ret == -EDEADLK) { + drm_modeset_backoff(&ctx); + goto retry; + } else if (WARN_ON(ret)) { + return; + } + + state = drm_atomic_helper_duplicate_state(dev, &ctx); + if (WARN_ON(IS_ERR(state))) + return; + + ret = intel_atomic_check(dev, state); + if (ret) { + /* + * If we fail here, it means that the hardware appears to be + * programmed in a way that shouldn't be possible, given our + * understanding of watermark requirements. This might mean a + * mistake in the hardware readout code or a mistake in the + * watermark calculations for a given platform. Raise a WARN + * so that this is noticeable. + * + * If this actually happens, we'll have to just leave the + * BIOS-programmed watermarks untouched and hope for the best. + */ + WARN(true, "Could not determine valid watermarks for inherited state\n"); + return; + } + + /* Write calculated watermark values back */ + to_i915(dev)->wm.config = to_intel_atomic_state(state)->wm_config; + for_each_crtc_in_state(state, crtc, cstate, i) { + struct intel_crtc_state *cs = to_intel_crtc_state(cstate); + + dev_priv->display.program_watermarks(cs); + } + + drm_atomic_state_free(state); + drm_modeset_drop_locks(&ctx); + drm_modeset_acquire_fini(&ctx); +} + void intel_modeset_init(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev->dev_private; @@ -15396,6 +15468,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 f9cbd2e81f54..db3ca41ce16b 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -3617,9 +3617,11 @@ static void skl_update_wm(struct drm_crtc *crtc) dev_priv->wm.skl_hw = *results; } -static void ilk_program_watermarks(struct drm_i915_private *dev_priv) +static void ilk_program_watermarks(struct intel_crtc_state *cstate) { - struct drm_device *dev = dev_priv->dev; + struct drm_crtc *crtc = cstate->base.crtc; + struct drm_device *dev = crtc->dev; + struct drm_i915_private *dev_priv = to_i915(dev); struct intel_pipe_wm lp_wm_1_2 = {}, lp_wm_5_6 = {}, *best_lp_wm; struct ilk_wm_maximums max; struct intel_wm_config *config = &dev_priv->wm.config; @@ -3650,7 +3652,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); @@ -3670,7 +3671,7 @@ static void ilk_update_wm(struct drm_crtc *crtc) intel_crtc->wm.active.ilk = cstate->wm.optimal.ilk; - ilk_program_watermarks(dev_priv); + ilk_program_watermarks(cstate); } static void skl_pipe_wm_active_state(uint32_t val, @@ -7000,6 +7001,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.20.1