diff mbox

[v2] drm/i915: Disable SAGV on pre plane update.

Message ID 20180223225200.3301-1-rodrigo.vivi@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Rodrigo Vivi Feb. 23, 2018, 10:52 p.m. UTC
According to Spec "Requirement before plane enabling or
configuration change: Disable SAGV if any enabled plane will not
be able to enable watermarks for memory latency >= SAGV block
time, or any transcoder is interlaced. Else, enable SAGV."

Currently we are only enabling and disabling SAGV on full
modeset. If we end up changing plane configurations and
sagv remains enabled when latency is higher than sagv block
time the machine can hang.

Also we are computing the latency values in different places
and following different conditions/rules. So let's move the
the sagv block time limit check to be inside skl_compute_wm
and, if necessary, disable SAGV on pre plane updates.

SAGV defaults to enabled by the BIOS, so we need to be more
careful and disable everytime we see a mismatch on its
conditions.

v2: - Consider only highest enabled wm level for SAGV block
      time limitation.
    - Handle sagv bool in a way that we properly consider all
      the planes. So we don't end up always disabling SAGV.
    - (Ville) Don't enabled on post_plane update, otherwise one
      pipe ends up enabling without checking for others. So keep
      the old enable/disable solution on atomic commit tail
      so we continue following the spec to disable on multiple
      pipe or interlaced and re-enable on modeset of a single
      pipe non interlaced. Always respecting the latency.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=104975
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Azhar Shaikh <azhar.shaikh@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c |   6 +-
 drivers/gpu/drm/i915/intel_drv.h     |   2 +
 drivers/gpu/drm/i915/intel_pm.c      | 107 +++++++++++++++++------------------
 3 files changed, 60 insertions(+), 55 deletions(-)

Comments

kernel test robot Feb. 26, 2018, 4:45 a.m. UTC | #1
Hi Rodrigo,

I love your patch! Yet something to improve:

[auto build test ERROR on drm-intel/for-linux-next]
[also build test ERROR on next-20180223]
[cannot apply to v4.16-rc3]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Rodrigo-Vivi/drm-i915-Disable-SAGV-on-pre-plane-update/20180226-094030
base:   git://anongit.freedesktop.org/drm-intel for-linux-next
config: i386-randconfig-x003-02260939 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All errors (new ones prefixed by >>):

   drivers/gpu//drm/i915/intel_display.c: In function 'intel_post_plane_update':
>> drivers/gpu//drm/i915/intel_display.c:5094:27: error: unused variable 'dev_priv' [-Werror=unused-variable]
     struct drm_i915_private *dev_priv = to_i915(dev);
                              ^~~~~~~~
   cc1: all warnings being treated as errors

vim +/dev_priv +5094 drivers/gpu//drm/i915/intel_display.c

  5089	
  5090	static void intel_post_plane_update(struct intel_crtc_state *old_crtc_state)
  5091	{
  5092		struct intel_crtc *crtc = to_intel_crtc(old_crtc_state->base.crtc);
  5093		struct drm_device *dev = crtc->base.dev;
> 5094		struct drm_i915_private *dev_priv = to_i915(dev);
  5095		struct drm_atomic_state *old_state = old_crtc_state->base.state;
  5096		struct intel_crtc_state *pipe_config =
  5097			intel_atomic_get_new_crtc_state(to_intel_atomic_state(old_state),
  5098							crtc);
  5099		struct drm_plane *primary = crtc->base.primary;
  5100		struct drm_plane_state *old_pri_state =
  5101			drm_atomic_get_existing_plane_state(old_state, primary);
  5102	
  5103		intel_frontbuffer_flip(to_i915(crtc->base.dev), pipe_config->fb_bits);
  5104	
  5105		if (pipe_config->update_wm_post && pipe_config->base.active)
  5106			intel_update_watermarks(crtc);
  5107	
  5108		if (hsw_post_update_enable_ips(old_crtc_state, pipe_config))
  5109			hsw_enable_ips(pipe_config);
  5110	
  5111		if (old_pri_state) {
  5112			struct intel_plane_state *primary_state =
  5113				intel_atomic_get_new_plane_state(to_intel_atomic_state(old_state),
  5114								 to_intel_plane(primary));
  5115			struct intel_plane_state *old_primary_state =
  5116				to_intel_plane_state(old_pri_state);
  5117	
  5118			intel_fbc_post_update(crtc);
  5119	
  5120			if (primary_state->base.visible &&
  5121			    (needs_modeset(&pipe_config->base) ||
  5122			     !old_primary_state->base.visible))
  5123				intel_post_enable_primary(&crtc->base, pipe_config);
  5124		}
  5125	}
  5126	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 65c8487be7c7..d98ff5916c85 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5090,6 +5090,8 @@  static bool hsw_post_update_enable_ips(const struct intel_crtc_state *old_crtc_s
 static void intel_post_plane_update(struct intel_crtc_state *old_crtc_state)
 {
 	struct intel_crtc *crtc = to_intel_crtc(old_crtc_state->base.crtc);
+	struct drm_device *dev = crtc->base.dev;
+	struct drm_i915_private *dev_priv = to_i915(dev);
 	struct drm_atomic_state *old_state = old_crtc_state->base.state;
 	struct intel_crtc_state *pipe_config =
 		intel_atomic_get_new_crtc_state(to_intel_atomic_state(old_state),
@@ -5205,6 +5207,9 @@  static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state,
 						     pipe_config);
 	else if (pipe_config->update_wm_pre)
 		intel_update_watermarks(crtc);
+
+	if (!pipe_config->sagv)
+		intel_disable_sagv(dev_priv);
 }
 
 static void intel_crtc_disable_planes(struct drm_crtc *crtc, unsigned plane_mask)
@@ -12372,7 +12377,6 @@  static void intel_atomic_commit_tail(struct drm_atomic_state *state)
 		 */
 		if (!intel_can_enable_sagv(state))
 			intel_disable_sagv(dev_priv);
-
 		intel_modeset_verify_disabled(dev, state);
 	}
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 1535bfb7ea40..294b6ac6c72c 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -712,6 +712,7 @@  struct intel_crtc_state {
 	bool update_pipe; /* can a fast modeset be performed? */
 	bool disable_cxsr;
 	bool update_wm_pre, update_wm_post; /* watermarks are updated */
+	bool sagv; /* Disable SAGV on any latency higher than its block time */
 	bool fb_changed; /* fb on any of the planes is changed */
 	bool fifo_changed; /* FIFO split is changed */
 
@@ -2002,6 +2003,7 @@  void skl_pipe_wm_get_hw_state(struct drm_crtc *crtc,
 void g4x_wm_sanitize(struct drm_i915_private *dev_priv);
 void vlv_wm_sanitize(struct drm_i915_private *dev_priv);
 bool intel_can_enable_sagv(struct drm_atomic_state *state);
+u8 intel_sagv_block_time(const struct drm_i915_private *dev_priv);
 int intel_enable_sagv(struct drm_i915_private *dev_priv);
 int intel_disable_sagv(struct drm_i915_private *dev_priv);
 bool skl_wm_level_equals(const struct skl_wm_level *l1,
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 21dac6ebc202..0bf3bf386def 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3687,22 +3687,12 @@  bool intel_can_enable_sagv(struct drm_atomic_state *state)
 	struct drm_i915_private *dev_priv = to_i915(dev);
 	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
 	struct intel_crtc *crtc;
-	struct intel_plane *plane;
 	struct intel_crtc_state *cstate;
 	enum pipe pipe;
-	int level, latency;
-	int sagv_block_time_us;
 
 	if (!intel_has_sagv(dev_priv))
 		return false;
 
-	if (IS_GEN9(dev_priv))
-		sagv_block_time_us = 30;
-	else if (IS_GEN10(dev_priv))
-		sagv_block_time_us = 20;
-	else
-		sagv_block_time_us = 10;
-
 	/*
 	 * SKL+ workaround: bspec recommends we disable the SAGV when we have
 	 * more then one pipe enabled
@@ -3719,41 +3709,27 @@  bool intel_can_enable_sagv(struct drm_atomic_state *state)
 	crtc = intel_get_crtc_for_pipe(dev_priv, pipe);
 	cstate = to_intel_crtc_state(crtc->base.state);
 
-	if (crtc->base.state->adjusted_mode.flags & DRM_MODE_FLAG_INTERLACE)
+	/* Also check for latency here */
+	cstate = to_intel_crtc_state(crtc->base.state);
+	if (!cstate->sagv)
 		return false;
 
-	for_each_intel_plane_on_crtc(dev, crtc, plane) {
-		struct skl_plane_wm *wm =
-			&cstate->wm.skl.optimal.planes[plane->id];
-
-		/* Skip this plane if it's not enabled */
-		if (!wm->wm[0].plane_en)
-			continue;
-
-		/* Find the highest enabled wm level for this plane */
-		for (level = ilk_wm_max_level(dev_priv);
-		     !wm->wm[level].plane_en; --level)
-		     { }
-
-		latency = dev_priv->wm.skl_latency[level];
-
-		if (skl_needs_memory_bw_wa(intel_state) &&
-		    plane->base.state->fb->modifier ==
-		    I915_FORMAT_MOD_X_TILED)
-			latency += 15;
-
-		/*
-		 * If any of the planes on this pipe don't enable wm levels that
-		 * incur memory latencies higher than sagv_block_time_us we
-		 * can't enable the SAGV.
-		 */
-		if (latency < sagv_block_time_us)
-			return false;
-	}
+	if (crtc->base.state->adjusted_mode.flags & DRM_MODE_FLAG_INTERLACE)
+		return false;
 
 	return true;
 }
 
+u8 intel_sagv_block_time(const struct drm_i915_private *dev_priv)
+{
+	if (INTEL_GEN(dev_priv) >= 11)
+		return 10;
+	else if (IS_GEN10(dev_priv))
+		return 20;
+	else
+		return 30;
+}
+
 static void
 skl_ddb_get_pipe_allocation_limits(struct drm_device *dev,
 				   const struct intel_crtc_state *cstate,
@@ -4501,10 +4477,10 @@  static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
 				const struct skl_wm_params *wp,
 				uint16_t *out_blocks, /* out */
 				uint8_t *out_lines, /* out */
-				bool *enabled /* out */)
+				bool *enabled, /* out */
+				uint32_t *latency /* out */)
 {
 	const struct drm_plane_state *pstate = &intel_pstate->base;
-	uint32_t latency = dev_priv->wm.skl_latency[level];
 	uint_fixed_16_16_t method1, method2;
 	uint_fixed_16_16_t selected_result;
 	uint32_t res_blocks, res_lines;
@@ -4513,7 +4489,9 @@  static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
 	bool apply_memory_bw_wa = skl_needs_memory_bw_wa(state);
 	uint32_t min_disp_buf_needed;
 
-	if (latency == 0 ||
+	*latency = dev_priv->wm.skl_latency[level];
+
+	if (*latency == 0 ||
 	    !intel_wm_plane_visible(cstate, intel_pstate)) {
 		*enabled = false;
 		return 0;
@@ -4523,16 +4501,16 @@  static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
 	if ((IS_KABYLAKE(dev_priv) || IS_COFFEELAKE(dev_priv) ||
 	    IS_CNL_REVID(dev_priv, CNL_REVID_A0, CNL_REVID_B0)) &&
 	    dev_priv->ipc_enabled)
-		latency += 4;
+		*latency += 4;
 
 	if (apply_memory_bw_wa && wp->x_tiled)
-		latency += 15;
+		*latency += 15;
 
 	method1 = skl_wm_method1(dev_priv, wp->plane_pixel_rate,
-				 wp->cpp, latency, wp->dbuf_block_size);
+				 wp->cpp, *latency, wp->dbuf_block_size);
 	method2 = skl_wm_method2(wp->plane_pixel_rate,
 				 cstate->base.adjusted_mode.crtc_htotal,
-				 latency,
+				 *latency,
 				 wp->plane_blocks_per_line);
 
 	if (wp->y_tiled) {
@@ -4545,7 +4523,7 @@  static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
 		else if (ddb_allocation >=
 			 fixed16_to_u32_round_up(wp->plane_blocks_per_line))
 			selected_result = min_fixed16(method1, method2);
-		else if (latency >= wp->linetime_us)
+		else if (*latency >= wp->linetime_us)
 			selected_result = min_fixed16(method1, method2);
 		else
 			selected_result = method1;
@@ -4629,7 +4607,8 @@  skl_compute_wm_levels(const struct drm_i915_private *dev_priv,
 		      struct intel_crtc_state *cstate,
 		      const struct intel_plane_state *intel_pstate,
 		      const struct skl_wm_params *wm_params,
-		      struct skl_plane_wm *wm)
+		      struct skl_plane_wm *wm,
+		      bool *plane_sagv)
 {
 	struct intel_crtc *intel_crtc = to_intel_crtc(cstate->base.crtc);
 	struct drm_plane *plane = intel_pstate->base.plane;
@@ -4638,6 +4617,7 @@  skl_compute_wm_levels(const struct drm_i915_private *dev_priv,
 	enum pipe pipe = intel_crtc->pipe;
 	int level, max_level = ilk_wm_max_level(dev_priv);
 	int ret;
+	u32 latency;
 
 	if (WARN_ON(!intel_pstate->base.fb))
 		return -EINVAL;
@@ -4655,9 +4635,18 @@  skl_compute_wm_levels(const struct drm_i915_private *dev_priv,
 					   wm_params,
 					   &result->plane_res_b,
 					   &result->plane_res_l,
-					   &result->plane_en);
+					   &result->plane_en,
+					   &latency);
+
 		if (ret)
 			return ret;
+
+		/*
+		 * Only consider the latency of highest enabled wm level
+		 * on the plane for SAGV block time limitation.
+		 */
+		if (result->plane_en)
+			*plane_sagv = latency >= intel_sagv_block_time(dev_priv);
 	}
 
 	return 0;
@@ -4743,7 +4732,8 @@  static void skl_compute_transition_wm(struct intel_crtc_state *cstate,
 
 static int skl_build_pipe_wm(struct intel_crtc_state *cstate,
 			     struct skl_ddb_allocation *ddb,
-			     struct skl_pipe_wm *pipe_wm)
+			     struct skl_pipe_wm *pipe_wm,
+			     bool *sagv)
 {
 	struct drm_device *dev = cstate->base.crtc->dev;
 	struct drm_crtc_state *crtc_state = &cstate->base;
@@ -4751,6 +4741,7 @@  static int skl_build_pipe_wm(struct intel_crtc_state *cstate,
 	struct drm_plane *plane;
 	const struct drm_plane_state *pstate;
 	struct skl_plane_wm *wm;
+	bool plane_sagv = false;
 	int ret;
 
 	/*
@@ -4777,9 +4768,13 @@  static int skl_build_pipe_wm(struct intel_crtc_state *cstate,
 			return ret;
 
 		ret = skl_compute_wm_levels(dev_priv, ddb, cstate,
-					    intel_pstate, &wm_params, wm);
+					    intel_pstate, &wm_params, wm, &plane_sagv);
 		if (ret)
 			return ret;
+
+		/* Take all planes in consideration for SAGV block time check */
+		*sagv &= plane_sagv;
+
 		skl_compute_transition_wm(cstate, &wm_params, &wm->wm[0],
 					  ddb_blocks, &wm->trans_wm);
 	}
@@ -4899,12 +4894,13 @@  static int skl_update_pipe_wm(struct drm_crtc_state *cstate,
 			      const struct skl_pipe_wm *old_pipe_wm,
 			      struct skl_pipe_wm *pipe_wm, /* out */
 			      struct skl_ddb_allocation *ddb, /* out */
-			      bool *changed /* out */)
+			      bool *changed /* out */,
+			      bool *sagv /* out */)
 {
 	struct intel_crtc_state *intel_cstate = to_intel_crtc_state(cstate);
 	int ret;
 
-	ret = skl_build_pipe_wm(intel_cstate, ddb, pipe_wm);
+	ret = skl_build_pipe_wm(intel_cstate, ddb, pipe_wm, sagv);
 	if (ret)
 		return ret;
 
@@ -5099,6 +5095,7 @@  skl_compute_wm(struct drm_atomic_state *state)
 	struct drm_device *dev = state->dev;
 	struct skl_pipe_wm *pipe_wm;
 	bool changed = false;
+	bool sagv = true;
 	int ret, i;
 
 	/*
@@ -5147,10 +5144,12 @@  skl_compute_wm(struct drm_atomic_state *state)
 
 		pipe_wm = &intel_cstate->wm.skl.optimal;
 		ret = skl_update_pipe_wm(cstate, old_pipe_wm, pipe_wm,
-					 &results->ddb, &changed);
+					 &results->ddb, &changed, &sagv);
 		if (ret)
 			return ret;
 
+		intel_cstate->sagv = sagv;
+
 		if (changed)
 			results->dirty_pipes |= drm_crtc_mask(crtc);