Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit d93c0372 authored by Matt Roper's avatar Matt Roper Committed by Maarten Lankhorst
Browse files

drm/i915: Sanitize watermarks after hardware state readout (v4) 



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 <maarten.lankhorst@linux.intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: default avatarMatt Roper <matthew.d.roper@intel.com>
Signed-off-by: default avatarMaarten Lankhorst <maarten.lankhorst@linux.intel.com>
parent 15126882

drivers/gpu/drm/i915/i915_drv.h

+1 −0
Original line number Diff line number Diff line
@@ -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);

drivers/gpu/drm/i915/intel_display.c

+79 −0
Original line number Diff line number Diff line
@@ -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)

drivers/gpu/drm/i915/intel_pm.c

+6 −4
Original line number Diff line number Diff line
@@ -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");