[PATCH v4 0/8] Kill off intel_crtc->atomic!

[PATCH v4 0/8] Kill off intel_crtc->atomic!

[Maarten Lankhorst]

This is a rebased version of the original series.
The upstream fbc changes required a rework of some of the
patches, but with atomic encoder masks this series will work
as intended.

v4 because v3 had a small issue with a compiler error in wait_vblank removal, and
I forgot to send it to the ml.

Maarten Lankhorst (8):
  drm/i915: Pass crtc state to modeset_get_crtc_power_domains.
  drm/i915: Unify power domain handling.
  drm/i915: Kill off intel_crtc->atomic.wait_vblank, v4.
  drm/i915: Remove update_sprite_watermarks.
  drm/i915: Remove some post-commit members from intel_crtc->atomic, v2.
  drm/i915: Nuke fbc members from intel_crtc->atomic, v2.
  drm/i915: Do not compute watermarks on a noop.
  drm/i915: Remove vblank wait from hsw_enable_ips.

 drivers/gpu/drm/i915/intel_atomic.c  |   2 +
 drivers/gpu/drm/i915/intel_display.c | 265 +++++++++++++++++++----------------
 drivers/gpu/drm/i915/intel_drv.h     |  36 ++---
 drivers/gpu/drm/i915/intel_pm.c      |  61 ++++----
 4 files changed, 196 insertions(+), 168 deletions(-)

-- 
2.1.0

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

[PATCH v4 1/8] drm/i915: Pass crtc state to modeset_get_crtc_power_domains.

[Maarten Lankhorst]

Use our newly created encoder_mask to iterate over the encoders.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 33 +++++++++++++++++++++------------
 1 file changed, 21 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 30d4db0d776f..b479ba6238d7 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5301,31 +5301,37 @@ intel_display_port_aux_power_domain(struct intel_encoder *intel_encoder)
 	}
 }
 
-static unsigned long get_crtc_power_domains(struct drm_crtc *crtc)
+static unsigned long get_crtc_power_domains(struct drm_crtc *crtc,
+					    struct intel_crtc_state *crtc_state)
 {
 	struct drm_device *dev = crtc->dev;
-	struct intel_encoder *intel_encoder;
+	struct drm_encoder *encoder;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	enum pipe pipe = intel_crtc->pipe;
 	unsigned long mask;
-	enum transcoder transcoder = intel_crtc->config->cpu_transcoder;
+	enum transcoder transcoder = crtc_state->cpu_transcoder;
 
-	if (!crtc->state->active)
+	if (!crtc_state->base.active)
 		return 0;
 
 	mask = BIT(POWER_DOMAIN_PIPE(pipe));
 	mask |= BIT(POWER_DOMAIN_TRANSCODER(transcoder));
-	if (intel_crtc->config->pch_pfit.enabled ||
-	    intel_crtc->config->pch_pfit.force_thru)
+	if (crtc_state->pch_pfit.enabled ||
+	    crtc_state->pch_pfit.force_thru)
 		mask |= BIT(POWER_DOMAIN_PIPE_PANEL_FITTER(pipe));
 
-	for_each_encoder_on_crtc(dev, crtc, intel_encoder)
+	drm_for_each_encoder_mask(encoder, dev, crtc_state->base.encoder_mask) {
+		struct intel_encoder *intel_encoder = to_intel_encoder(encoder);
+
 		mask |= BIT(intel_display_port_power_domain(intel_encoder));
+	}
 
 	return mask;
 }
 
-static unsigned long modeset_get_crtc_power_domains(struct drm_crtc *crtc)
+static unsigned long
+modeset_get_crtc_power_domains(struct drm_crtc *crtc,
+			       struct intel_crtc_state *crtc_state)
 {
 	struct drm_i915_private *dev_priv = crtc->dev->dev_private;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
@@ -5333,7 +5339,8 @@ static unsigned long modeset_get_crtc_power_domains(struct drm_crtc *crtc)
 	unsigned long domains, new_domains, old_domains;
 
 	old_domains = intel_crtc->enabled_power_domains;
-	intel_crtc->enabled_power_domains = new_domains = get_crtc_power_domains(crtc);
+	intel_crtc->enabled_power_domains = new_domains =
+		get_crtc_power_domains(crtc, crtc_state);
 
 	domains = new_domains & ~old_domains;
 
@@ -5365,7 +5372,8 @@ static void modeset_update_crtc_power_domains(struct drm_atomic_state *state)
 	for_each_crtc_in_state(state, crtc, crtc_state, i) {
 		if (needs_modeset(crtc->state))
 			put_domains[to_intel_crtc(crtc)->pipe] =
-				modeset_get_crtc_power_domains(crtc);
+				modeset_get_crtc_power_domains(crtc,
+					to_intel_crtc_state(crtc->state));
 	}
 
 	if (dev_priv->display.modeset_commit_cdclk &&
@@ -13486,7 +13494,8 @@ static int intel_atomic_commit(struct drm_device *dev,
 		}
 
 		if (update_pipe) {
-			put_domains = modeset_get_crtc_power_domains(crtc);
+			put_domains = modeset_get_crtc_power_domains(crtc,
+					      to_intel_crtc_state(crtc->state));
 
 			/* make sure intel_modeset_check_state runs */
 			hw_check = true;
@@ -15830,7 +15839,7 @@ intel_modeset_setup_hw_state(struct drm_device *dev)
 	for_each_intel_crtc(dev, crtc) {
 		unsigned long put_domains;
 
-		put_domains = modeset_get_crtc_power_domains(&crtc->base);
+		put_domains = modeset_get_crtc_power_domains(&crtc->base, crtc->config);
 		if (WARN_ON(put_domains))
 			modeset_put_power_domains(dev_priv, put_domains);
 	}
-- 
2.1.0

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

[PATCH v4 2/8] drm/i915: Unify power domain handling.

[Maarten Lankhorst]

Right now there's separate power domain handling for update_pipe and
modesets. Unify this and only grab POWER_DOMAIN_MODESET once.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 69 +++++++++++++-----------------------
 1 file changed, 24 insertions(+), 45 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index b479ba6238d7..804f2c6f260d 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5359,32 +5359,6 @@ static void modeset_put_power_domains(struct drm_i915_private *dev_priv,
 		intel_display_power_put(dev_priv, domain);
 }
 
-static void modeset_update_crtc_power_domains(struct drm_atomic_state *state)
-{
-	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
-	struct drm_device *dev = state->dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	unsigned long put_domains[I915_MAX_PIPES] = {};
-	struct drm_crtc_state *crtc_state;
-	struct drm_crtc *crtc;
-	int i;
-
-	for_each_crtc_in_state(state, crtc, crtc_state, i) {
-		if (needs_modeset(crtc->state))
-			put_domains[to_intel_crtc(crtc)->pipe] =
-				modeset_get_crtc_power_domains(crtc,
-					to_intel_crtc_state(crtc->state));
-	}
-
-	if (dev_priv->display.modeset_commit_cdclk &&
-	    intel_state->dev_cdclk != dev_priv->cdclk_freq)
-		dev_priv->display.modeset_commit_cdclk(state);
-
-	for (i = 0; i < I915_MAX_PIPES; i++)
-		if (put_domains[i])
-			modeset_put_power_domains(dev_priv, put_domains[i]);
-}
-
 static int intel_compute_max_dotclk(struct drm_i915_private *dev_priv)
 {
 	int max_cdclk_freq = dev_priv->max_cdclk_freq;
@@ -13422,6 +13396,7 @@ static int intel_atomic_commit(struct drm_device *dev,
 	struct drm_crtc *crtc;
 	int ret = 0, i;
 	bool hw_check = intel_state->modeset;
+	unsigned long put_domains[I915_MAX_PIPES] = {};
 
 	ret = intel_atomic_prepare_commit(dev, state, async);
 	if (ret) {
@@ -13437,11 +13412,22 @@ static int intel_atomic_commit(struct drm_device *dev,
 		       sizeof(intel_state->min_pixclk));
 		dev_priv->active_crtcs = intel_state->active_crtcs;
 		dev_priv->atomic_cdclk_freq = intel_state->cdclk;
+
+		intel_display_power_get(dev_priv, POWER_DOMAIN_MODESET);
 	}
 
 	for_each_crtc_in_state(state, crtc, crtc_state, i) {
 		struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 
+		if (needs_modeset(crtc->state) ||
+		    to_intel_crtc_state(crtc->state)->update_pipe) {
+			hw_check = true;
+
+			put_domains[to_intel_crtc(crtc)->pipe] =
+				modeset_get_crtc_power_domains(crtc,
+					to_intel_crtc_state(crtc->state));
+		}
+
 		if (!needs_modeset(crtc->state))
 			continue;
 
@@ -13474,7 +13460,10 @@ static int intel_atomic_commit(struct drm_device *dev,
 		intel_shared_dpll_commit(state);
 
 		drm_atomic_helper_update_legacy_modeset_state(state->dev, state);
-		modeset_update_crtc_power_domains(state);
+
+		if (dev_priv->display.modeset_commit_cdclk &&
+		    intel_state->dev_cdclk != dev_priv->cdclk_freq)
+			dev_priv->display.modeset_commit_cdclk(state);
 	}
 
 	/* Now enable the clocks, plane, pipe, and connectors that we set up. */
@@ -13483,24 +13472,12 @@ static int intel_atomic_commit(struct drm_device *dev,
 		bool modeset = needs_modeset(crtc->state);
 		bool update_pipe = !modeset &&
 			to_intel_crtc_state(crtc->state)->update_pipe;
-		unsigned long put_domains = 0;
-
-		if (modeset)
-			intel_display_power_get(dev_priv, POWER_DOMAIN_MODESET);
 
 		if (modeset && crtc->state->active) {
 			update_scanline_offset(to_intel_crtc(crtc));
 			dev_priv->display.crtc_enable(crtc);
 		}
 
-		if (update_pipe) {
-			put_domains = modeset_get_crtc_power_domains(crtc,
-					      to_intel_crtc_state(crtc->state));
-
-			/* make sure intel_modeset_check_state runs */
-			hw_check = true;
-		}
-
 		if (!modeset)
 			intel_pre_plane_update(to_intel_crtc_state(crtc_state));
 
@@ -13511,19 +13488,21 @@ static int intel_atomic_commit(struct drm_device *dev,
 		    (crtc->state->planes_changed || update_pipe))
 			drm_atomic_helper_commit_planes_on_crtc(crtc_state);
 
-		if (put_domains)
-			modeset_put_power_domains(dev_priv, put_domains);
-
 		intel_post_plane_update(intel_crtc);
-
-		if (modeset)
-			intel_display_power_put(dev_priv, POWER_DOMAIN_MODESET);
 	}
 
 	/* FIXME: add subpixel order */
 
 	drm_atomic_helper_wait_for_vblanks(dev, state);
 
+	for_each_crtc_in_state(state, crtc, crtc_state, i) {
+		if (put_domains[i])
+			modeset_put_power_domains(dev_priv, put_domains[i]);
+	}
+
+	if (intel_state->modeset)
+		intel_display_power_put(dev_priv, POWER_DOMAIN_MODESET);
+
 	mutex_lock(&dev->struct_mutex);
 	drm_atomic_helper_cleanup_planes(dev, state);
 	mutex_unlock(&dev->struct_mutex);
-- 
2.1.0

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

[PATCH v4 3/8] drm/i915: Kill off intel_crtc->atomic.wait_vblank, v4.

[Maarten Lankhorst]

Currently we perform our own wait in post_plane_update,
but the atomic core performs another one in wait_for_vblanks.
This means that 2 vblanks are done when a fb is changed,
which is a bit overkill.

Merge them by creating a helper function that takes a crtc mask
for the planes to wait on.

The broadwell vblank workaround may look gone entirely but this is
not the case. pipe_config->wm_changed is set to true
when any plane is turned on, which forces a vblank wait.

Changes since v1:
- Removing the double vblank wait on broadwell moved to its own commit.
Changes since v2:
- Move out POWER_DOMAIN_MODESET handling to its own commit.
Changes since v3:
- Do not wait for vblank on legacy cursor updates. (Ville)
- Move broadwell vblank workaround comment to page_flip_finished. (Ville)
Changes since v4:
- Compile fix, legacy_cursor_flip -> *_update.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_atomic.c  |  1 +
 drivers/gpu/drm/i915/intel_display.c | 86 +++++++++++++++++++++++++++---------
 drivers/gpu/drm/i915/intel_drv.h     |  2 +-
 3 files changed, 67 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
index 4625f8a9ba12..8e579a8505ac 100644
--- a/drivers/gpu/drm/i915/intel_atomic.c
+++ b/drivers/gpu/drm/i915/intel_atomic.c
@@ -97,6 +97,7 @@ intel_crtc_duplicate_state(struct drm_crtc *crtc)
 	crtc_state->disable_lp_wm = false;
 	crtc_state->disable_cxsr = false;
 	crtc_state->wm_changed = false;
+	crtc_state->fb_changed = false;
 
 	return &crtc_state->base;
 }
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 804f2c6f260d..4d4dddc1f970 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4785,9 +4785,6 @@ static void intel_post_plane_update(struct intel_crtc *crtc)
 		to_intel_crtc_state(crtc->base.state);
 	struct drm_device *dev = crtc->base.dev;
 
-	if (atomic->wait_vblank)
-		intel_wait_for_vblank(dev, crtc->pipe);
-
 	intel_frontbuffer_flip(dev, atomic->fb_bits);
 
 	crtc->wm.cxsr_allowed = true;
@@ -10902,6 +10899,12 @@ static bool page_flip_finished(struct intel_crtc *crtc)
 		return true;
 
 	/*
+	 * BDW signals flip done immediately if the plane
+	 * is disabled, even if the plane enable is already
+	 * armed to occur at the next vblank :(
+	 */
+
+	/*
 	 * A DSPSURFLIVE check isn't enough in case the mmio and CS flips
 	 * used the same base address. In that case the mmio flip might
 	 * have completed, but the CS hasn't even executed the flip yet.
@@ -11778,6 +11781,9 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
 	if (!was_visible && !visible)
 		return 0;
 
+	if (fb != old_plane_state->base.fb)
+		pipe_config->fb_changed = true;
+
 	turn_off = was_visible && (!visible || mode_changed);
 	turn_on = visible && (!was_visible || mode_changed);
 
@@ -11793,8 +11799,6 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
 
 		/* must disable cxsr around plane enable/disable */
 		if (plane->type != DRM_PLANE_TYPE_CURSOR) {
-			if (is_crtc_enabled)
-				intel_crtc->atomic.wait_vblank = true;
 			pipe_config->disable_cxsr = true;
 		}
 	} else if (intel_wm_need_update(plane, plane_state)) {
@@ -11810,14 +11814,6 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
 		intel_crtc->atomic.post_enable_primary = turn_on;
 		intel_crtc->atomic.update_fbc = true;
 
-		/*
-		 * BDW signals flip done immediately if the plane
-		 * is disabled, even if the plane enable is already
-		 * armed to occur at the next vblank :(
-		 */
-		if (turn_on && IS_BROADWELL(dev))
-			intel_crtc->atomic.wait_vblank = true;
-
 		break;
 	case DRM_PLANE_TYPE_CURSOR:
 		break;
@@ -11831,12 +11827,10 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
 		if (IS_IVYBRIDGE(dev) &&
 		    needs_scaling(to_intel_plane_state(plane_state)) &&
 		    !needs_scaling(old_plane_state)) {
-			to_intel_crtc_state(crtc_state)->disable_lp_wm = true;
-		} else if (turn_off && !mode_changed) {
-			intel_crtc->atomic.wait_vblank = true;
+			pipe_config->disable_lp_wm = true;
+		} else if (turn_off && !mode_changed)
 			intel_crtc->atomic.update_sprite_watermarks |=
 				1 << i;
-		}
 
 		break;
 	}
@@ -13370,6 +13364,48 @@ static int intel_atomic_prepare_commit(struct drm_device *dev,
 	return ret;
 }
 
+static void intel_atomic_wait_for_vblanks(struct drm_device *dev,
+					  struct drm_i915_private *dev_priv,
+					  unsigned crtc_mask)
+{
+	unsigned last_vblank_count[I915_MAX_PIPES];
+	enum pipe pipe;
+	int ret;
+
+	if (!crtc_mask)
+		return;
+
+	for_each_pipe(dev_priv, pipe) {
+		struct drm_crtc *crtc = dev_priv->pipe_to_crtc_mapping[pipe];
+
+		if (!((1 << pipe) & crtc_mask))
+			continue;
+
+		ret = drm_crtc_vblank_get(crtc);
+		if (ret != 0) {
+			crtc_mask &= ~(1 << pipe);
+			continue;
+		}
+
+		last_vblank_count[pipe] = drm_crtc_vblank_count(crtc);
+	}
+
+	for_each_pipe(dev_priv, pipe) {
+		struct drm_crtc *crtc = dev_priv->pipe_to_crtc_mapping[pipe];
+
+		if (!((1 << pipe) & crtc_mask))
+			continue;
+
+		wait_event_timeout(dev->vblank[pipe].queue,
+				last_vblank_count[pipe] !=
+					drm_crtc_vblank_count(crtc),
+				msecs_to_jiffies(50));
+
+		drm_crtc_vblank_put(crtc);
+	}
+}
+
+
 /**
  * intel_atomic_commit - commit validated state object
  * @dev: DRM device
@@ -13397,6 +13433,7 @@ static int intel_atomic_commit(struct drm_device *dev,
 	int ret = 0, i;
 	bool hw_check = intel_state->modeset;
 	unsigned long put_domains[I915_MAX_PIPES] = {};
+	unsigned crtc_vblank_mask = 0;
 
 	ret = intel_atomic_prepare_commit(dev, state, async);
 	if (ret) {
@@ -13470,8 +13507,9 @@ static int intel_atomic_commit(struct drm_device *dev,
 	for_each_crtc_in_state(state, crtc, crtc_state, i) {
 		struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 		bool modeset = needs_modeset(crtc->state);
-		bool update_pipe = !modeset &&
-			to_intel_crtc_state(crtc->state)->update_pipe;
+		struct intel_crtc_state *pipe_config =
+			to_intel_crtc_state(crtc->state);
+		bool update_pipe = !modeset && pipe_config->update_pipe;
 
 		if (modeset && crtc->state->active) {
 			update_scanline_offset(to_intel_crtc(crtc));
@@ -13488,14 +13526,20 @@ static int intel_atomic_commit(struct drm_device *dev,
 		    (crtc->state->planes_changed || update_pipe))
 			drm_atomic_helper_commit_planes_on_crtc(crtc_state);
 
-		intel_post_plane_update(intel_crtc);
+		if (pipe_config->base.active &&
+		    (pipe_config->wm_changed || pipe_config->disable_cxsr ||
+		     pipe_config->fb_changed))
+			crtc_vblank_mask |= 1 << i;
 	}
 
 	/* FIXME: add subpixel order */
 
-	drm_atomic_helper_wait_for_vblanks(dev, state);
+	if (!state->legacy_cursor_update)
+		intel_atomic_wait_for_vblanks(dev, dev_priv, crtc_vblank_mask);
 
 	for_each_crtc_in_state(state, crtc, crtc_state, i) {
+		intel_post_plane_update(to_intel_crtc(crtc));
+
 		if (put_domains[i])
 			modeset_put_power_domains(dev_priv, put_domains[i]);
 	}
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 335e6b24b0bc..e911c86f873b 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -379,6 +379,7 @@ struct intel_crtc_state {
 	bool update_pipe; /* can a fast modeset be performed? */
 	bool disable_cxsr;
 	bool wm_changed; /* watermarks are updated */
+	bool fb_changed; /* fb on any of the planes is changed */
 
 	/* Pipe source size (ie. panel fitter input size)
 	 * All planes will be positioned inside this space,
@@ -547,7 +548,6 @@ struct intel_crtc_atomic_commit {
 
 	/* Sleepable operations to perform after commit */
 	unsigned fb_bits;
-	bool wait_vblank;
 	bool post_enable_primary;
 	unsigned update_sprite_watermarks;
 
-- 
2.1.0

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

[PATCH v4 4/8] drm/i915: Remove update_sprite_watermarks.

[Maarten Lankhorst]

Commit 791a32be6eb2 ("drm/i915: Drop intel_update_sprite_watermarks")
removes the use of this variable, but forgot to remove it.

Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 6 +-----
 drivers/gpu/drm/i915/intel_drv.h     | 1 -
 2 files changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 4d4dddc1f970..c1f94eee8028 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11748,7 +11748,6 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
 	struct intel_plane_state *old_plane_state =
 		to_intel_plane_state(plane->state);
 	int idx = intel_crtc->base.base.id, ret;
-	int i = drm_plane_index(plane);
 	bool mode_changed = needs_modeset(crtc_state);
 	bool was_crtc_enabled = crtc->state->active;
 	bool is_crtc_enabled = crtc_state->active;
@@ -11826,11 +11825,8 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
 		 */
 		if (IS_IVYBRIDGE(dev) &&
 		    needs_scaling(to_intel_plane_state(plane_state)) &&
-		    !needs_scaling(old_plane_state)) {
+		    !needs_scaling(old_plane_state))
 			pipe_config->disable_lp_wm = true;
-		} else if (turn_off && !mode_changed)
-			intel_crtc->atomic.update_sprite_watermarks |=
-				1 << i;
 
 		break;
 	}
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index e911c86f873b..d4e441f3aecf 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -549,7 +549,6 @@ struct intel_crtc_atomic_commit {
 	/* Sleepable operations to perform after commit */
 	unsigned fb_bits;
 	bool post_enable_primary;
-	unsigned update_sprite_watermarks;
 
 	/* Sleepable operations to perform before and after commit */
 	bool update_fbc;
-- 
2.1.0

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

[PATCH v4 5/8] drm/i915: Remove some post-commit members from intel_crtc->atomic, v2.

[Maarten Lankhorst]

fb_bits is useful to have in the crtc_state for cs flips when
the code is updated to use intel_frontbuffer_flip_prepare/complete.
So calculate it in advance and move it to crtc_state. The other stuff
can be calculated in post_plane_update, and aren't useful elsewhere.

Changes since v1:
- Changing wording, remove comment about loop.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Reviewed-by: Ander Conselvan de Oliveira <conselvan2@gmail.com>
---
 drivers/gpu/drm/i915/intel_atomic.c  |  1 +
 drivers/gpu/drm/i915/intel_display.c | 29 +++++++++++++++++++++--------
 drivers/gpu/drm/i915/intel_drv.h     |  5 +----
 3 files changed, 23 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
index 8e579a8505ac..9a45cff26767 100644
--- a/drivers/gpu/drm/i915/intel_atomic.c
+++ b/drivers/gpu/drm/i915/intel_atomic.c
@@ -98,6 +98,7 @@ intel_crtc_duplicate_state(struct drm_crtc *crtc)
 	crtc_state->disable_cxsr = false;
 	crtc_state->wm_changed = false;
 	crtc_state->fb_changed = false;
+	crtc_state->fb_bits = 0;
 
 	return &crtc_state->base;
 }
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index c1f94eee8028..54be8a255f1f 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4778,14 +4778,20 @@ intel_pre_disable_primary(struct drm_crtc *crtc)
 	hsw_disable_ips(intel_crtc);
 }
 
-static void intel_post_plane_update(struct intel_crtc *crtc)
+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_atomic_state *old_state = old_crtc_state->base.state;
 	struct intel_crtc_atomic_commit *atomic = &crtc->atomic;
 	struct intel_crtc_state *pipe_config =
 		to_intel_crtc_state(crtc->base.state);
 	struct drm_device *dev = crtc->base.dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct drm_plane *primary = crtc->base.primary;
+	struct drm_plane_state *old_pri_state =
+		drm_atomic_get_existing_plane_state(old_state, primary);
 
-	intel_frontbuffer_flip(dev, atomic->fb_bits);
+	intel_frontbuffer_flip(dev, pipe_config->fb_bits);
 
 	crtc->wm.cxsr_allowed = true;
 
@@ -4795,8 +4801,17 @@ static void intel_post_plane_update(struct intel_crtc *crtc)
 	if (atomic->update_fbc)
 		intel_fbc_post_update(crtc);
 
-	if (atomic->post_enable_primary)
-		intel_post_enable_primary(&crtc->base);
+	if (old_pri_state) {
+		struct intel_plane_state *primary_state =
+			to_intel_plane_state(primary->state);
+		struct intel_plane_state *old_primary_state =
+			to_intel_plane_state(old_pri_state);
+
+		if (primary_state->visible &&
+		    (needs_modeset(&pipe_config->base) ||
+		     !old_primary_state->visible))
+			intel_post_enable_primary(&crtc->base);
+	}
 
 	memset(atomic, 0, sizeof(*atomic));
 }
@@ -11805,12 +11820,10 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
 	}
 
 	if (visible || was_visible)
-		intel_crtc->atomic.fb_bits |=
-			to_intel_plane(plane)->frontbuffer_bit;
+		pipe_config->fb_bits |= to_intel_plane(plane)->frontbuffer_bit;
 
 	switch (plane->type) {
 	case DRM_PLANE_TYPE_PRIMARY:
-		intel_crtc->atomic.post_enable_primary = turn_on;
 		intel_crtc->atomic.update_fbc = true;
 
 		break;
@@ -13534,7 +13547,7 @@ static int intel_atomic_commit(struct drm_device *dev,
 		intel_atomic_wait_for_vblanks(dev, dev_priv, crtc_vblank_mask);
 
 	for_each_crtc_in_state(state, crtc, crtc_state, i) {
-		intel_post_plane_update(to_intel_crtc(crtc));
+		intel_post_plane_update(to_intel_crtc_state(crtc_state));
 
 		if (put_domains[i])
 			modeset_put_power_domains(dev_priv, put_domains[i]);
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index d4e441f3aecf..803591d1e613 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -376,6 +376,7 @@ struct intel_crtc_state {
 #define PIPE_CONFIG_QUIRK_MODE_SYNC_FLAGS	(1<<0) /* unreliable sync mode.flags */
 	unsigned long quirks;
 
+	unsigned fb_bits; /* framebuffers to flip */
 	bool update_pipe; /* can a fast modeset be performed? */
 	bool disable_cxsr;
 	bool wm_changed; /* watermarks are updated */
@@ -547,10 +548,6 @@ struct intel_crtc_atomic_commit {
 	/* Sleepable operations to perform before commit */
 
 	/* Sleepable operations to perform after commit */
-	unsigned fb_bits;
-	bool post_enable_primary;
-
-	/* Sleepable operations to perform before and after commit */
 	bool update_fbc;
 };
 
-- 
2.1.0

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

[PATCH v4 6/8] drm/i915: Nuke fbc members from intel_crtc->atomic, v2.

[Maarten Lankhorst]

Factor out intel_fbc_supports_rotation and use it in
pre_plane_update as well. This leaves intel_crtc->atomic
empty, so remove it too.

Changes since v1:
- Add a intel_fbc_supports_rotation helper.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 58 +++++++++++++-----------------------
 drivers/gpu/drm/i915/intel_drv.h     | 15 ----------
 2 files changed, 20 insertions(+), 53 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 54be8a255f1f..00cb261c6787 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4782,11 +4782,9 @@ 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_atomic_state *old_state = old_crtc_state->base.state;
-	struct intel_crtc_atomic_commit *atomic = &crtc->atomic;
 	struct intel_crtc_state *pipe_config =
 		to_intel_crtc_state(crtc->base.state);
 	struct drm_device *dev = crtc->base.dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_plane *primary = crtc->base.primary;
 	struct drm_plane_state *old_pri_state =
 		drm_atomic_get_existing_plane_state(old_state, primary);
@@ -4798,22 +4796,19 @@ static void intel_post_plane_update(struct intel_crtc_state *old_crtc_state)
 	if (pipe_config->wm_changed && pipe_config->base.active)
 		intel_update_watermarks(&crtc->base);
 
-	if (atomic->update_fbc)
-		intel_fbc_post_update(crtc);
-
 	if (old_pri_state) {
 		struct intel_plane_state *primary_state =
 			to_intel_plane_state(primary->state);
 		struct intel_plane_state *old_primary_state =
 			to_intel_plane_state(old_pri_state);
 
+		intel_fbc_post_update(crtc);
+
 		if (primary_state->visible &&
 		    (needs_modeset(&pipe_config->base) ||
 		     !old_primary_state->visible))
 			intel_post_enable_primary(&crtc->base);
 	}
-
-	memset(atomic, 0, sizeof(*atomic));
 }
 
 static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state)
@@ -4821,7 +4816,6 @@ static void intel_pre_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 = dev->dev_private;
-	struct intel_crtc_atomic_commit *atomic = &crtc->atomic;
 	struct intel_crtc_state *pipe_config =
 		to_intel_crtc_state(crtc->base.state);
 	struct drm_atomic_state *old_state = old_crtc_state->base.state;
@@ -4830,17 +4824,17 @@ static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state)
 		drm_atomic_get_existing_plane_state(old_state, primary);
 	bool modeset = needs_modeset(&pipe_config->base);
 
-	if (atomic->update_fbc)
-		intel_fbc_pre_update(crtc);
-
 	if (old_pri_state) {
 		struct intel_plane_state *primary_state =
 			to_intel_plane_state(primary->state);
 		struct intel_plane_state *old_primary_state =
 			to_intel_plane_state(old_pri_state);
+		bool turn_off = old_primary_state->visible &&
+		    (modeset || !primary_state->visible);
+
+		intel_fbc_pre_update(crtc);
 
-		if (old_primary_state->visible &&
-		    (modeset || !primary_state->visible))
+		if (turn_off)
 			intel_pre_disable_primary(&crtc->base);
 	}
 
@@ -11822,27 +11816,17 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
 	if (visible || was_visible)
 		pipe_config->fb_bits |= to_intel_plane(plane)->frontbuffer_bit;
 
-	switch (plane->type) {
-	case DRM_PLANE_TYPE_PRIMARY:
-		intel_crtc->atomic.update_fbc = true;
-
-		break;
-	case DRM_PLANE_TYPE_CURSOR:
-		break;
-	case DRM_PLANE_TYPE_OVERLAY:
-		/*
-		 * WaCxSRDisabledForSpriteScaling:ivb
-		 *
-		 * cstate->update_wm was already set above, so this flag will
-		 * take effect when we commit and program watermarks.
-		 */
-		if (IS_IVYBRIDGE(dev) &&
-		    needs_scaling(to_intel_plane_state(plane_state)) &&
-		    !needs_scaling(old_plane_state))
-			pipe_config->disable_lp_wm = true;
+	/*
+	 * WaCxSRDisabledForSpriteScaling:ivb
+	 *
+	 * cstate->update_wm was already set above, so this flag will
+	 * take effect when we commit and program watermarks.
+	 */
+	if (plane->type == DRM_PLANE_TYPE_OVERLAY && IS_IVYBRIDGE(dev) &&
+	    needs_scaling(to_intel_plane_state(plane_state)) &&
+	    !needs_scaling(old_plane_state))
+		pipe_config->disable_lp_wm = true;
 
-		break;
-	}
 	return 0;
 }
 
@@ -13241,9 +13225,6 @@ static int intel_atomic_check(struct drm_device *dev,
 		struct intel_crtc_state *pipe_config =
 			to_intel_crtc_state(crtc_state);
 
-		memset(&to_intel_crtc(crtc)->atomic, 0,
-		       sizeof(struct intel_crtc_atomic_commit));
-
 		/* Catch I915_MODE_FLAG_INHERITED */
 		if (crtc_state->mode.private_flags != crtc->state->mode.private_flags)
 			crtc_state->mode_changed = true;
@@ -13528,12 +13509,13 @@ static int intel_atomic_commit(struct drm_device *dev,
 		if (!modeset)
 			intel_pre_plane_update(to_intel_crtc_state(crtc_state));
 
-		if (crtc->state->active && intel_crtc->atomic.update_fbc)
+		if ((modeset || update_pipe) && crtc->state->active)
 			intel_fbc_enable(intel_crtc);
 
 		if (crtc->state->active &&
-		    (crtc->state->planes_changed || update_pipe))
+		    (crtc->state->planes_changed || update_pipe)) {
 			drm_atomic_helper_commit_planes_on_crtc(crtc_state);
+		}
 
 		if (pipe_config->base.active &&
 		    (pipe_config->wm_changed || pipe_config->disable_cxsr ||
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 803591d1e613..8effb9ece21e 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -538,19 +538,6 @@ struct intel_mmio_flip {
 	unsigned int rotation;
 };
 
-/*
- * Tracking of operations that need to be performed at the beginning/end of an
- * atomic commit, outside the atomic section where interrupts are disabled.
- * These are generally operations that grab mutexes or might otherwise sleep
- * and thus can't be run with interrupts disabled.
- */
-struct intel_crtc_atomic_commit {
-	/* Sleepable operations to perform before commit */
-
-	/* Sleepable operations to perform after commit */
-	bool update_fbc;
-};
-
 struct intel_crtc {
 	struct drm_crtc base;
 	enum pipe pipe;
@@ -610,8 +597,6 @@ struct intel_crtc {
 		int scanline_start;
 	} debug;
 
-	struct intel_crtc_atomic_commit atomic;
-
 	/* scalers available on this crtc */
 	int num_scalers;
 
-- 
2.1.0

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

[PATCH v4 7/8] drm/i915: Do not compute watermarks on a noop.

[Maarten Lankhorst]

No atomic state should be included after all validation when nothing has
changed. During modeset all active planes will be added to the state,
while disabled planes won't change their state.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c |  3 +-
 drivers/gpu/drm/i915/intel_drv.h     | 13 ++++++++
 drivers/gpu/drm/i915/intel_pm.c      | 61 +++++++++++++++++++++---------------
 3 files changed, 51 insertions(+), 26 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 00cb261c6787..6bb1f5dbc7a0 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11910,7 +11910,8 @@ static int intel_crtc_atomic_check(struct drm_crtc *crtc,
 	}
 
 	ret = 0;
-	if (dev_priv->display.compute_pipe_wm) {
+	if (dev_priv->display.compute_pipe_wm &&
+	    (mode_changed || pipe_config->update_pipe || crtc_state->planes_changed)) {
 		ret = dev_priv->display.compute_pipe_wm(intel_crtc, state);
 		if (ret)
 			return ret;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 8effb9ece21e..144597ac74e3 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1583,6 +1583,19 @@ intel_atomic_get_crtc_state(struct drm_atomic_state *state,
 
 	return to_intel_crtc_state(crtc_state);
 }
+
+static inline struct intel_plane_state *
+intel_atomic_get_existing_plane_state(struct drm_atomic_state *state,
+				      struct intel_plane *plane)
+{
+	struct drm_plane_state *plane_state;
+
+	plane_state = drm_atomic_get_existing_plane_state(state, &plane->base);
+
+	return to_intel_plane_state(plane_state);
+}
+
+
 int intel_atomic_setup_scalers(struct drm_device *dev,
 	struct intel_crtc *intel_crtc,
 	struct intel_crtc_state *crtc_state);
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 379eabe093cb..8fb8c6891ae6 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -2010,11 +2010,18 @@ static void ilk_compute_wm_level(const struct drm_i915_private *dev_priv,
 		cur_latency *= 5;
 	}
 
-	result->pri_val = ilk_compute_pri_wm(cstate, pristate,
-					     pri_latency, level);
-	result->spr_val = ilk_compute_spr_wm(cstate, sprstate, spr_latency);
-	result->cur_val = ilk_compute_cur_wm(cstate, curstate, cur_latency);
-	result->fbc_val = ilk_compute_fbc_wm(cstate, pristate, result->pri_val);
+	if (pristate) {
+		result->pri_val = ilk_compute_pri_wm(cstate, pristate,
+						     pri_latency, level);
+		result->fbc_val = ilk_compute_fbc_wm(cstate, pristate, result->pri_val);
+	}
+
+	if (sprstate)
+		result->spr_val = ilk_compute_spr_wm(cstate, sprstate, spr_latency);
+
+	if (curstate)
+		result->cur_val = ilk_compute_cur_wm(cstate, curstate, cur_latency);
+
 	result->enable = true;
 }
 
@@ -2287,7 +2294,6 @@ static int ilk_compute_pipe_wm(struct intel_crtc *intel_crtc,
 	const struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc_state *cstate = NULL;
 	struct intel_plane *intel_plane;
-	struct drm_plane_state *ps;
 	struct intel_plane_state *pristate = NULL;
 	struct intel_plane_state *sprstate = NULL;
 	struct intel_plane_state *curstate = NULL;
@@ -2306,30 +2312,37 @@ static int ilk_compute_pipe_wm(struct intel_crtc *intel_crtc,
 	memset(pipe_wm, 0, sizeof(*pipe_wm));
 
 	for_each_intel_plane_on_crtc(dev, intel_crtc, intel_plane) {
-		ps = drm_atomic_get_plane_state(state,
-						&intel_plane->base);
-		if (IS_ERR(ps))
-			return PTR_ERR(ps);
+		struct intel_plane_state *ps;
+
+		ps = intel_atomic_get_existing_plane_state(state,
+							   intel_plane);
+		if (!ps)
+			continue;
 
 		if (intel_plane->base.type == DRM_PLANE_TYPE_PRIMARY)
-			pristate = to_intel_plane_state(ps);
-		else if (intel_plane->base.type == DRM_PLANE_TYPE_OVERLAY)
-			sprstate = to_intel_plane_state(ps);
-		else if (intel_plane->base.type == DRM_PLANE_TYPE_CURSOR)
-			curstate = to_intel_plane_state(ps);
+			pristate = ps;
+		else if (intel_plane->base.type == DRM_PLANE_TYPE_OVERLAY) {
+			sprstate = ps;
+
+			if (ps) {
+				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);
+			}
+		} else if (intel_plane->base.type == DRM_PLANE_TYPE_CURSOR)
+			curstate = ps;
 	}
 
-	config.sprites_enabled = sprstate->visible;
-	config.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);
+	config.sprites_enabled = pipe_wm->sprites_enabled;
+	config.sprites_scaled = pipe_wm->sprites_scaled;
 
 	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)
+	if (INTEL_INFO(dev)->gen <= 6 && pipe_wm->sprites_enabled)
 		max_level = 1;
 
 	/* ILK/SNB/IVB: LP1+ watermarks only w/o scaling */
@@ -2352,20 +2365,18 @@ static int ilk_compute_pipe_wm(struct intel_crtc *intel_crtc,
 	ilk_compute_wm_reg_maximums(dev, 1, &max);
 
 	for (level = 1; level <= max_level; level++) {
-		struct intel_wm_level wm = {};
+		struct intel_wm_level *wm = &pipe_wm->wm[level];
 
 		ilk_compute_wm_level(dev_priv, intel_crtc, level, cstate,
-				     pristate, sprstate, curstate, &wm);
+				     pristate, sprstate, curstate, wm);
 
 		/*
 		 * Disable any watermark level that exceeds the
 		 * register maximums since such watermarks are
 		 * always invalid.
 		 */
-		if (!ilk_validate_wm_level(level, &max, &wm))
+		if (!ilk_validate_wm_level(level, &max, wm))
 			break;
-
-		pipe_wm->wm[level] = wm;
 	}
 
 	return 0;
-- 
2.1.0

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

[PATCH v4 8/8] drm/i915: Remove vblank wait from hsw_enable_ips.

[Maarten Lankhorst]

intel_post_plane_update did an extra vblank wait that's no longer needed when enabling ips.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 6bb1f5dbc7a0..19a8d376d63e 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4569,9 +4569,6 @@ void hsw_enable_ips(struct intel_crtc *crtc)
 	if (!crtc->config->ips_enabled)
 		return;
 
-	/* We can only enable IPS after we enable a plane and wait for a vblank */
-	intel_wait_for_vblank(dev, crtc->pipe);
-
 	assert_plane_enabled(dev_priv, crtc->plane);
 	if (IS_BROADWELL(dev)) {
 		mutex_lock(&dev_priv->rps.hw_lock);
-- 
2.1.0

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

Re: [PATCH v4 8/8] drm/i915: Remove vblank wait from hsw_enable_ips.

[Zanoni, Paulo R]

Em Qua, 2016-02-10 às 13:49 +0100, Maarten Lankhorst escreveu:
> intel_post_plane_update did an extra vblank wait that's no longer
> needed when enabling ips.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index 6bb1f5dbc7a0..19a8d376d63e 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4569,9 +4569,6 @@ void hsw_enable_ips(struct intel_crtc *crtc)
>  	if (!crtc->config->ips_enabled)
>  		return;
>  
> -	/* We can only enable IPS after we enable a plane and wait
> for a vblank */

Perhaps we can keep the comment, but change it into something like:

/* We can only enable IPS after we enable a plane and wait for a
vblank, but we don't need the wait here because of XYZ. */

> -	intel_wait_for_vblank(dev, crtc->pipe);
> -
>  	assert_plane_enabled(dev_priv, crtc->plane);
>  	if (IS_BROADWELL(dev)) {
>  		mutex_lock(&dev_priv->rps.hw_lock);
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v4 6/8] drm/i915: Nuke fbc members from intel_crtc->atomic, v2.

[Zanoni, Paulo R]

Em Qua, 2016-02-10 às 13:49 +0100, Maarten Lankhorst escreveu:
> Factor out intel_fbc_supports_rotation

^ not anymore


>  and use it in
> pre_plane_update as well. This leaves intel_crtc->atomic
> empty, so remove it too.
> 
> Changes since v1:
> - Add a intel_fbc_supports_rotation helper.

Changes since v2:
- No more need for rotation special-casing.

(I suppose you also have to edit the commit title to be v3)

> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 58 +++++++++++++-------------
> ----------
>  drivers/gpu/drm/i915/intel_drv.h     | 15 ----------
>  2 files changed, 20 insertions(+), 53 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index 54be8a255f1f..00cb261c6787 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4782,11 +4782,9 @@ 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_atomic_state *old_state = old_crtc_state-
> >base.state;
> -	struct intel_crtc_atomic_commit *atomic = &crtc->atomic;
>  	struct intel_crtc_state *pipe_config =
>  		to_intel_crtc_state(crtc->base.state);
>  	struct drm_device *dev = crtc->base.dev;
> -	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct drm_plane *primary = crtc->base.primary;
>  	struct drm_plane_state *old_pri_state =
>  		drm_atomic_get_existing_plane_state(old_state,
> primary);
> @@ -4798,22 +4796,19 @@ static void intel_post_plane_update(struct
> intel_crtc_state *old_crtc_state)
>  	if (pipe_config->wm_changed && pipe_config->base.active)
>  		intel_update_watermarks(&crtc->base);
>  
> -	if (atomic->update_fbc)
> -		intel_fbc_post_update(crtc);
> -
>  	if (old_pri_state) {

For a code reader that is not very familiar with all the atomic history
and its details, it's not trivial to conclude that "if
(drm_atomic_get_existing_plane_state(primary))", then we're updating
the primary plane on this atomic commit. And before this patch, it's
much much easier to conclude that update_fbc will be true when an
atomic update is touching the primary plane because that's explicitly
stated by the cod.

So although "let's kill this redundant struct" sounds good, it seems to
me that code clarity is going away with this patch, so I wonder if the
benefits of the patch outweigh the downsides. Isn't there something
else we could do about this, such as some renaming, or adding some
function aliases or just extra commenting?

>  		struct intel_plane_state *primary_state =
>  			to_intel_plane_state(primary->state);
>  		struct intel_plane_state *old_primary_state =
>  			to_intel_plane_state(old_pri_state);
>  
> +		intel_fbc_post_update(crtc);
> +
>  		if (primary_state->visible &&
>  		    (needs_modeset(&pipe_config->base) ||
>  		     !old_primary_state->visible))
>  			intel_post_enable_primary(&crtc->base);
>  	}
> -
> -	memset(atomic, 0, sizeof(*atomic));
>  }
>  
>  static void intel_pre_plane_update(struct intel_crtc_state
> *old_crtc_state)
> @@ -4821,7 +4816,6 @@ static void intel_pre_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 = dev->dev_private;
> -	struct intel_crtc_atomic_commit *atomic = &crtc->atomic;
>  	struct intel_crtc_state *pipe_config =
>  		to_intel_crtc_state(crtc->base.state);
>  	struct drm_atomic_state *old_state = old_crtc_state-
> >base.state;
> @@ -4830,17 +4824,17 @@ static void intel_pre_plane_update(struct
> intel_crtc_state *old_crtc_state)
>  		drm_atomic_get_existing_plane_state(old_state,
> primary);
>  	bool modeset = needs_modeset(&pipe_config->base);
>  
> -	if (atomic->update_fbc)
> -		intel_fbc_pre_update(crtc);
> -
>  	if (old_pri_state) {
>  		struct intel_plane_state *primary_state =
>  			to_intel_plane_state(primary->state);
>  		struct intel_plane_state *old_primary_state =
>  			to_intel_plane_state(old_pri_state);
> +		bool turn_off = old_primary_state->visible &&
> +		    (modeset || !primary_state->visible); 

Not really related to the patch, but ok to keep since it's trivial...

> +
> +		intel_fbc_pre_update(crtc);
>  
> -		if (old_primary_state->visible &&
> -		    (modeset || !primary_state->visible))
> +		if (turn_off)
>  			intel_pre_disable_primary(&crtc->base);
>  	}
>  
> @@ -11822,27 +11816,17 @@ int intel_plane_atomic_calc_changes(struct
> drm_crtc_state *crtc_state,
>  	if (visible || was_visible)
>  		pipe_config->fb_bits |= to_intel_plane(plane)-
> >frontbuffer_bit;
>  
> -	switch (plane->type) {
> -	case DRM_PLANE_TYPE_PRIMARY:
> -		intel_crtc->atomic.update_fbc = true;
> -
> -		break;
> -	case DRM_PLANE_TYPE_CURSOR:
> -		break;
> -	case DRM_PLANE_TYPE_OVERLAY:
> -		/*
> -		 * WaCxSRDisabledForSpriteScaling:ivb
> -		 *
> -		 * cstate->update_wm was already set above, so this
> flag will
> -		 * take effect when we commit and program
> watermarks.
> -		 */
> -		if (IS_IVYBRIDGE(dev) &&
> -		    needs_scaling(to_intel_plane_state(plane_state))
> &&
> -		    !needs_scaling(old_plane_state))
> -			pipe_config->disable_lp_wm = true;
> +	/*
> +	 * WaCxSRDisabledForSpriteScaling:ivb
> +	 *
> +	 * cstate->update_wm was already set above, so this flag
> will
> +	 * take effect when we commit and program watermarks.
> +	 */
> +	if (plane->type == DRM_PLANE_TYPE_OVERLAY &&
> IS_IVYBRIDGE(dev) &&
> +	    needs_scaling(to_intel_plane_state(plane_state)) &&
> +	    !needs_scaling(old_plane_state))
> +		pipe_config->disable_lp_wm = true;
>  
> -		break;
> -	}
>  	return 0;
>  }
>  
> @@ -13241,9 +13225,6 @@ static int intel_atomic_check(struct
> drm_device *dev,
>  		struct intel_crtc_state *pipe_config =
>  			to_intel_crtc_state(crtc_state);
>  
> -		memset(&to_intel_crtc(crtc)->atomic, 0,
> -		       sizeof(struct intel_crtc_atomic_commit));
> -
>  		/* Catch I915_MODE_FLAG_INHERITED */
>  		if (crtc_state->mode.private_flags != crtc->state-
> >mode.private_flags)
>  			crtc_state->mode_changed = true;
> @@ -13528,12 +13509,13 @@ static int intel_atomic_commit(struct
> drm_device *dev,
>  		if (!modeset)
>  			intel_pre_plane_update(to_intel_crtc_state(c
> rtc_state));
>  
> -		if (crtc->state->active && intel_crtc-
> >atomic.update_fbc)
> +		if ((modeset || update_pipe) && crtc->state->active)

Same here: not easy for me to verify things are the same now. Even
worse: the check for "is this atomic commit touching the primary
plane?" is now written in a completely different way than the one
introduced above. Maybe there's something we could do to make the code
easier to read.


>  			intel_fbc_enable(intel_crtc);
>  
>  		if (crtc->state->active &&
> -		    (crtc->state->planes_changed || update_pipe))
> +		    (crtc->state->planes_changed || update_pipe)) {
>  			drm_atomic_helper_commit_planes_on_crtc(crtc
> _state);
> +		}

???


Besides the points mentioned above, the patch seems to pass the FBC
tests, which is good :)

My suggestions are sorta bikesheds and although I'm not 100% confident,
the patch seems to be correct, so:
Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

But please consider the readability concerns pointed on this review,
maybe by proposing v4 or a follow-up patch :)

>  
>  		if (pipe_config->base.active &&
>  		    (pipe_config->wm_changed || pipe_config-
> >disable_cxsr ||
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index 803591d1e613..8effb9ece21e 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -538,19 +538,6 @@ struct intel_mmio_flip {
>  	unsigned int rotation;
>  };
>  
> -/*
> - * Tracking of operations that need to be performed at the
> beginning/end of an
> - * atomic commit, outside the atomic section where interrupts are
> disabled.
> - * These are generally operations that grab mutexes or might
> otherwise sleep
> - * and thus can't be run with interrupts disabled.
> - */
> -struct intel_crtc_atomic_commit {
> -	/* Sleepable operations to perform before commit */
> -
> -	/* Sleepable operations to perform after commit */
> -	bool update_fbc;
> -};
> -
>  struct intel_crtc {
>  	struct drm_crtc base;
>  	enum pipe pipe;
> @@ -610,8 +597,6 @@ struct intel_crtc {
>  		int scanline_start;
>  	} debug;
>  
> -	struct intel_crtc_atomic_commit atomic;
> -
>  	/* scalers available on this crtc */
>  	int num_scalers;
>  
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BAT: failure for Kill off intel_crtc->atomic! (rev8)

[Patchwork]

== Summary ==

Series 77v8 Kill off intel_crtc->atomic!
http://patchwork.freedesktop.org/api/1.0/series/77/revisions/8/mbox/

Test kms_flip:
        Subgroup basic-flip-vs-dpms:
                pass       -> DMESG-WARN (skl-i5k-2)
                pass       -> DMESG-WARN (ilk-hp8440p) UNSTABLE
        Subgroup basic-flip-vs-wf_vblank:
                pass       -> DMESG-WARN (skl-i5k-2)
Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-c:
                dmesg-warn -> PASS       (bsw-nuc-2)
Test pm_rpm:
        Subgroup basic-pci-d3-state:
                pass       -> FAIL       (hsw-brixbox)
                fail       -> PASS       (bdw-nuci7)
        Subgroup basic-rte:
                dmesg-warn -> PASS       (bsw-nuc-2)
                pass       -> FAIL       (bdw-ultra)

bdw-nuci7        total:162  pass:152  dwarn:0   dfail:0   fail:0   skip:10 
bdw-ultra        total:165  pass:151  dwarn:0   dfail:0   fail:1   skip:13 
bsw-nuc-2        total:165  pass:136  dwarn:0   dfail:0   fail:0   skip:29 
byt-nuc          total:165  pass:141  dwarn:0   dfail:0   fail:0   skip:24 
hsw-brixbox      total:165  pass:150  dwarn:0   dfail:0   fail:1   skip:14 
hsw-gt2          total:165  pass:154  dwarn:0   dfail:0   fail:1   skip:10 
ilk-hp8440p      total:165  pass:115  dwarn:1   dfail:0   fail:1   skip:48 
ivb-t430s        total:165  pass:150  dwarn:0   dfail:0   fail:1   skip:14 
skl-i5k-2        total:165  pass:147  dwarn:3   dfail:0   fail:0   skip:15 
snb-dellxps      total:165  pass:142  dwarn:0   dfail:0   fail:1   skip:22 
snb-x220t        total:165  pass:142  dwarn:0   dfail:0   fail:2   skip:21 

Results at /archive/results/CI_IGT_test/Patchwork_1389/

f2110d8eac120416f8f5669f2aa561d9ab330a77 drm-intel-nightly: 2016y-02m-15d-09h-53m-04s UTC integration manifest
6c3070b739e39258955200bc894f404624443512 drm/i915: Remove vblank wait from hsw_enable_ips.
a2efbd40f02b67ccfad95fe1aaa15ee16d064587 drm/i915: Do not compute watermarks on a noop.
d4bd3c52cb02bc40d649a9f6a894ea9cea39cd5f drm/i915: Nuke fbc members from intel_crtc->atomic, v2.
e18153d768dc4e148e64593a6426c7c17f189299 drm/i915: Remove some post-commit members from intel_crtc->atomic, v2.
bbea33f7e7ddb289c194963cf8ebd2e6ff2dc800 drm/i915: Remove update_sprite_watermarks.
e593cced664d88417e24324260b9458904268c3d drm/i915: Kill off intel_crtc->atomic.wait_vblank, v4.
3b74695aaa8dd202baec985c3338b36083648c17 drm/i915: Unify power domain handling.
a1af80d9af7f7bbd18b60c0e633436ff2bc53c92 drm/i915: Pass crtc state to modeset_get_crtc_power_domains.

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

Re: [PATCH v4 6/8] drm/i915: Nuke fbc members from intel_crtc->atomic, v2.

[Maarten Lankhorst]

Op 12-02-16 om 14:56 schreef Zanoni, Paulo R:
> Em Qua, 2016-02-10 às 13:49 +0100, Maarten Lankhorst escreveu:
>> Factor out intel_fbc_supports_rotation
> ^ not anymore
>
>
>>  and use it in
>> pre_plane_update as well. This leaves intel_crtc->atomic
>> empty, so remove it too.
>>
>> Changes since v1:
>> - Add a intel_fbc_supports_rotation helper.
> Changes since v2:
> - No more need for rotation special-casing.
>
> (I suppose you also have to edit the commit title to be v3)
>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_display.c | 58 +++++++++++++-------------
>> ----------
>>  drivers/gpu/drm/i915/intel_drv.h     | 15 ----------
>>  2 files changed, 20 insertions(+), 53 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c
>> b/drivers/gpu/drm/i915/intel_display.c
>> index 54be8a255f1f..00cb261c6787 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -4782,11 +4782,9 @@ 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_atomic_state *old_state = old_crtc_state-
>>> base.state;
>> -	struct intel_crtc_atomic_commit *atomic = &crtc->atomic;
>>  	struct intel_crtc_state *pipe_config =
>>  		to_intel_crtc_state(crtc->base.state);
>>  	struct drm_device *dev = crtc->base.dev;
>> -	struct drm_i915_private *dev_priv = dev->dev_private;
>>  	struct drm_plane *primary = crtc->base.primary;
>>  	struct drm_plane_state *old_pri_state =
>>  		drm_atomic_get_existing_plane_state(old_state,
>> primary);
>> @@ -4798,22 +4796,19 @@ static void intel_post_plane_update(struct
>> intel_crtc_state *old_crtc_state)
>>  	if (pipe_config->wm_changed && pipe_config->base.active)
>>  		intel_update_watermarks(&crtc->base);
>>  
>> -	if (atomic->update_fbc)
>> -		intel_fbc_post_update(crtc);
>> -
>>  	if (old_pri_state) {
> For a code reader that is not very familiar with all the atomic history
> and its details, it's not trivial to conclude that "if
> (drm_atomic_get_existing_plane_state(primary))", then we're updating
> the primary plane on this atomic commit. And before this patch, it's
> much much easier to conclude that update_fbc will be true when an
> atomic update is touching the primary plane because that's explicitly
> stated by the cod.
>
> So although "let's kill this redundant struct" sounds good, it seems to
> me that code clarity is going away with this patch, so I wonder if the
> benefits of the patch outweigh the downsides. Isn't there something
> else we could do about this, such as some renaming, or adding some
> function aliases or just extra commenting?
>
>>  		struct intel_plane_state *primary_state =
>>  			to_intel_plane_state(primary->state);
>>  		struct intel_plane_state *old_primary_state =
>>  			to_intel_plane_state(old_pri_state);
>>  
>> +		intel_fbc_post_update(crtc);
>> +
>>  		if (primary_state->visible &&
>>  		    (needs_modeset(&pipe_config->base) ||
>>  		     !old_primary_state->visible))
>>  			intel_post_enable_primary(&crtc->base);
>>  	}
>> -
>> -	memset(atomic, 0, sizeof(*atomic));
>>  }
>>  
>>  static void intel_pre_plane_update(struct intel_crtc_state
>> *old_crtc_state)
>> @@ -4821,7 +4816,6 @@ static void intel_pre_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 = dev->dev_private;
>> -	struct intel_crtc_atomic_commit *atomic = &crtc->atomic;
>>  	struct intel_crtc_state *pipe_config =
>>  		to_intel_crtc_state(crtc->base.state);
>>  	struct drm_atomic_state *old_state = old_crtc_state-
>>> base.state;
>> @@ -4830,17 +4824,17 @@ static void intel_pre_plane_update(struct
>> intel_crtc_state *old_crtc_state)
>>  		drm_atomic_get_existing_plane_state(old_state,
>> primary);
>>  	bool modeset = needs_modeset(&pipe_config->base);
>>  
>> -	if (atomic->update_fbc)
>> -		intel_fbc_pre_update(crtc);
>> -
>>  	if (old_pri_state) {
>>  		struct intel_plane_state *primary_state =
>>  			to_intel_plane_state(primary->state);
>>  		struct intel_plane_state *old_primary_state =
>>  			to_intel_plane_state(old_pri_state);
>> +		bool turn_off = old_primary_state->visible &&
>> +		    (modeset || !primary_state->visible); 
> Not really related to the patch, but ok to keep since it's trivial...
>
>> +
>> +		intel_fbc_pre_update(crtc);
>>  
>> -		if (old_primary_state->visible &&
>> -		    (modeset || !primary_state->visible))
>> +		if (turn_off)
>>  			intel_pre_disable_primary(&crtc->base);
>>  	}
>>  
>> @@ -11822,27 +11816,17 @@ int intel_plane_atomic_calc_changes(struct
>> drm_crtc_state *crtc_state,
>>  	if (visible || was_visible)
>>  		pipe_config->fb_bits |= to_intel_plane(plane)-
>>> frontbuffer_bit;
>>  
>> -	switch (plane->type) {
>> -	case DRM_PLANE_TYPE_PRIMARY:
>> -		intel_crtc->atomic.update_fbc = true;
>> -
>> -		break;
>> -	case DRM_PLANE_TYPE_CURSOR:
>> -		break;
>> -	case DRM_PLANE_TYPE_OVERLAY:
>> -		/*
>> -		 * WaCxSRDisabledForSpriteScaling:ivb
>> -		 *
>> -		 * cstate->update_wm was already set above, so this
>> flag will
>> -		 * take effect when we commit and program
>> watermarks.
>> -		 */
>> -		if (IS_IVYBRIDGE(dev) &&
>> -		    needs_scaling(to_intel_plane_state(plane_state))
>> &&
>> -		    !needs_scaling(old_plane_state))
>> -			pipe_config->disable_lp_wm = true;
>> +	/*
>> +	 * WaCxSRDisabledForSpriteScaling:ivb
>> +	 *
>> +	 * cstate->update_wm was already set above, so this flag
>> will
>> +	 * take effect when we commit and program watermarks.
>> +	 */
>> +	if (plane->type == DRM_PLANE_TYPE_OVERLAY &&
>> IS_IVYBRIDGE(dev) &&
>> +	    needs_scaling(to_intel_plane_state(plane_state)) &&
>> +	    !needs_scaling(old_plane_state))
>> +		pipe_config->disable_lp_wm = true;
>>  
>> -		break;
>> -	}
>>  	return 0;
>>  }
>>  
>> @@ -13241,9 +13225,6 @@ static int intel_atomic_check(struct
>> drm_device *dev,
>>  		struct intel_crtc_state *pipe_config =
>>  			to_intel_crtc_state(crtc_state);
>>  
>> -		memset(&to_intel_crtc(crtc)->atomic, 0,
>> -		       sizeof(struct intel_crtc_atomic_commit));
>> -
>>  		/* Catch I915_MODE_FLAG_INHERITED */
>>  		if (crtc_state->mode.private_flags != crtc->state-
>>> mode.private_flags)
>>  			crtc_state->mode_changed = true;
>> @@ -13528,12 +13509,13 @@ static int intel_atomic_commit(struct
>> drm_device *dev,
>>  		if (!modeset)
>>  			intel_pre_plane_update(to_intel_crtc_state(c
>> rtc_state));
>>  
>> -		if (crtc->state->active && intel_crtc-
>>> atomic.update_fbc)
>> +		if ((modeset || update_pipe) && crtc->state->active)
> Same here: not easy for me to verify things are the same now. Even
> worse: the check for "is this atomic commit touching the primary
> plane?" is now written in a completely different way than the one
> introduced above. Maybe there's something we could do to make the code
> easier to read.
No, this is called now every time when the crtc has a modeset or update_pipe, even with no primary plane.
This is because atomic may set a mode without having a primary plane, and in that case fbc_enable may never be called.
Seemed like a bug in the original code..

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

Re: [PATCH v4 8/8] drm/i915: Remove vblank wait from hsw_enable_ips.

[Maarten Lankhorst]

Hey,

Op 12-02-16 om 13:06 schreef Zanoni, Paulo R:
> Em Qua, 2016-02-10 às 13:49 +0100, Maarten Lankhorst escreveu:
>> intel_post_plane_update did an extra vblank wait that's no longer
>> needed when enabling ips.
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_display.c | 3 ---
>>  1 file changed, 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c
>> b/drivers/gpu/drm/i915/intel_display.c
>> index 6bb1f5dbc7a0..19a8d376d63e 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -4569,9 +4569,6 @@ void hsw_enable_ips(struct intel_crtc *crtc)
>>  	if (!crtc->config->ips_enabled)
>>  		return;
>>  
>> -	/* We can only enable IPS after we enable a plane and wait
>> for a vblank */
> Perhaps we can keep the comment, but change it into something like:
>
> /* We can only enable IPS after we enable a plane and wait for a
> vblank, but we don't need the wait here because of XYZ. */
>
post_plane_update only gets run after a vblank wait now, so I think adding a comment here is optional.
But yeah I'll make it more clear in the commit.

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

Re: [PATCH v4 1/8] drm/i915: Pass crtc state to modeset_get_crtc_power_domains.

[Zanoni, Paulo R]

Em Qua, 2016-02-10 às 13:49 +0100, Maarten Lankhorst escreveu:
> Use our newly created encoder_mask to iterate over the encoders.

As someone who was not paying attention to the discussion of the
previous patches related to this, I think it would be really good if
your commit message could tell me why we should use the newly created
encoder_mask instead of the current patch. What's bad about the current
version? Please sell me your patch. If you think the answer is trivial,
remember that it's not trivial to many people, and that random people
may find this patch through git-bisect and have to judge its
importance. Also, an explanation really helps the reviewers :)

The patch looks correct, so if you improve the commit message:
Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 33 +++++++++++++++++++++-----
> -------
>  1 file changed, 21 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index 30d4db0d776f..b479ba6238d7 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5301,31 +5301,37 @@ intel_display_port_aux_power_domain(struct
> intel_encoder *intel_encoder)
>  	}
>  }
>  
> -static unsigned long get_crtc_power_domains(struct drm_crtc *crtc)
> +static unsigned long get_crtc_power_domains(struct drm_crtc *crtc,
> +					    struct intel_crtc_state
> *crtc_state)
>  {
>  	struct drm_device *dev = crtc->dev;
> -	struct intel_encoder *intel_encoder;
> +	struct drm_encoder *encoder;
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  	enum pipe pipe = intel_crtc->pipe;
>  	unsigned long mask;
> -	enum transcoder transcoder = intel_crtc->config-
> >cpu_transcoder;
> +	enum transcoder transcoder = crtc_state->cpu_transcoder;
>  
> -	if (!crtc->state->active)
> +	if (!crtc_state->base.active)
>  		return 0;
>  
>  	mask = BIT(POWER_DOMAIN_PIPE(pipe));
>  	mask |= BIT(POWER_DOMAIN_TRANSCODER(transcoder));
> -	if (intel_crtc->config->pch_pfit.enabled ||
> -	    intel_crtc->config->pch_pfit.force_thru)
> +	if (crtc_state->pch_pfit.enabled ||
> +	    crtc_state->pch_pfit.force_thru)
>  		mask |= BIT(POWER_DOMAIN_PIPE_PANEL_FITTER(pipe));
>  
> -	for_each_encoder_on_crtc(dev, crtc, intel_encoder)
> +	drm_for_each_encoder_mask(encoder, dev, crtc_state-
> >base.encoder_mask) {
> +		struct intel_encoder *intel_encoder =
> to_intel_encoder(encoder);
> +
>  		mask |=
> BIT(intel_display_port_power_domain(intel_encoder));
> +	}
>  
>  	return mask;
>  }
>  
> -static unsigned long modeset_get_crtc_power_domains(struct drm_crtc
> *crtc)
> +static unsigned long
> +modeset_get_crtc_power_domains(struct drm_crtc *crtc,
> +			       struct intel_crtc_state *crtc_state)
>  {
>  	struct drm_i915_private *dev_priv = crtc->dev->dev_private;
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> @@ -5333,7 +5339,8 @@ static unsigned long
> modeset_get_crtc_power_domains(struct drm_crtc *crtc)
>  	unsigned long domains, new_domains, old_domains;
>  
>  	old_domains = intel_crtc->enabled_power_domains;
> -	intel_crtc->enabled_power_domains = new_domains =
> get_crtc_power_domains(crtc);
> +	intel_crtc->enabled_power_domains = new_domains =
> +		get_crtc_power_domains(crtc, crtc_state);
>  
>  	domains = new_domains & ~old_domains;
>  
> @@ -5365,7 +5372,8 @@ static void
> modeset_update_crtc_power_domains(struct drm_atomic_state *state)
>  	for_each_crtc_in_state(state, crtc, crtc_state, i) {
>  		if (needs_modeset(crtc->state))
>  			put_domains[to_intel_crtc(crtc)->pipe] =
> -				modeset_get_crtc_power_domains(crtc)
> ;
> +				modeset_get_crtc_power_domains(crtc,
> +					to_intel_crtc_state(crtc-
> >state));
>  	}
>  
>  	if (dev_priv->display.modeset_commit_cdclk &&
> @@ -13486,7 +13494,8 @@ static int intel_atomic_commit(struct
> drm_device *dev,
>  		}
>  
>  		if (update_pipe) {
> -			put_domains =
> modeset_get_crtc_power_domains(crtc);
> +			put_domains =
> modeset_get_crtc_power_domains(crtc,
> +					      to_intel_crtc_state(cr
> tc->state));
>  
>  			/* make sure intel_modeset_check_state runs
> */
>  			hw_check = true;
> @@ -15830,7 +15839,7 @@ intel_modeset_setup_hw_state(struct
> drm_device *dev)
>  	for_each_intel_crtc(dev, crtc) {
>  		unsigned long put_domains;
>  
> -		put_domains = modeset_get_crtc_power_domains(&crtc-
> >base);
> +		put_domains = modeset_get_crtc_power_domains(&crtc-
> >base, crtc->config);
>  		if (WARN_ON(put_domains))
>  			modeset_put_power_domains(dev_priv,
> put_domains);
>  	}
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v4 2/8] drm/i915: Unify power domain handling.

[Zanoni, Paulo R]

Em Qua, 2016-02-10 às 13:49 +0100, Maarten Lankhorst escreveu:
> Right now there's separate power domain handling for update_pipe and
> modesets. Unify this and only grab POWER_DOMAIN_MODESET once.

I would have split this in two separate commits: one for
POWER_DOMAIN_MODESET, and the other for killing
intel_modeset_update_crtc_power_domains. Let's hope no one bisects
anything to this patch :)

With or without the split:
Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 69 +++++++++++++-------------
> ----------
>  1 file changed, 24 insertions(+), 45 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index b479ba6238d7..804f2c6f260d 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5359,32 +5359,6 @@ static void modeset_put_power_domains(struct
> drm_i915_private *dev_priv,
>  		intel_display_power_put(dev_priv, domain);
>  }
>  
> -static void modeset_update_crtc_power_domains(struct
> drm_atomic_state *state)
> -{
> -	struct intel_atomic_state *intel_state =
> to_intel_atomic_state(state);
> -	struct drm_device *dev = state->dev;
> -	struct drm_i915_private *dev_priv = dev->dev_private;
> -	unsigned long put_domains[I915_MAX_PIPES] = {};
> -	struct drm_crtc_state *crtc_state;
> -	struct drm_crtc *crtc;
> -	int i;
> -
> -	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> -		if (needs_modeset(crtc->state))
> -			put_domains[to_intel_crtc(crtc)->pipe] =
> -				modeset_get_crtc_power_domains(crtc,
> -					to_intel_crtc_state(crtc-
> >state));
> -	}
> -
> -	if (dev_priv->display.modeset_commit_cdclk &&
> -	    intel_state->dev_cdclk != dev_priv->cdclk_freq)
> -		dev_priv->display.modeset_commit_cdclk(state);
> -
> -	for (i = 0; i < I915_MAX_PIPES; i++)
> -		if (put_domains[i])
> -			modeset_put_power_domains(dev_priv,
> put_domains[i]);
> -}
> -
>  static int intel_compute_max_dotclk(struct drm_i915_private
> *dev_priv)
>  {
>  	int max_cdclk_freq = dev_priv->max_cdclk_freq;
> @@ -13422,6 +13396,7 @@ static int intel_atomic_commit(struct
> drm_device *dev,
>  	struct drm_crtc *crtc;
>  	int ret = 0, i;
>  	bool hw_check = intel_state->modeset;
> +	unsigned long put_domains[I915_MAX_PIPES] = {};
>  
>  	ret = intel_atomic_prepare_commit(dev, state, async);
>  	if (ret) {
> @@ -13437,11 +13412,22 @@ static int intel_atomic_commit(struct
> drm_device *dev,
>  		       sizeof(intel_state->min_pixclk));
>  		dev_priv->active_crtcs = intel_state->active_crtcs;
>  		dev_priv->atomic_cdclk_freq = intel_state->cdclk;
> +
> +		intel_display_power_get(dev_priv,
> POWER_DOMAIN_MODESET);
>  	}
>  
>  	for_each_crtc_in_state(state, crtc, crtc_state, i) {
>  		struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  
> +		if (needs_modeset(crtc->state) ||
> +		    to_intel_crtc_state(crtc->state)->update_pipe) {
> +			hw_check = true;
> +
> +			put_domains[to_intel_crtc(crtc)->pipe] =
> +				modeset_get_crtc_power_domains(crtc,
> +					to_intel_crtc_state(crtc-
> >state));
> +		}
> +
>  		if (!needs_modeset(crtc->state))
>  			continue;
>  
> @@ -13474,7 +13460,10 @@ static int intel_atomic_commit(struct
> drm_device *dev,
>  		intel_shared_dpll_commit(state);
>  
>  		drm_atomic_helper_update_legacy_modeset_state(state-
> >dev, state);
> -		modeset_update_crtc_power_domains(state);
> +
> +		if (dev_priv->display.modeset_commit_cdclk &&
> +		    intel_state->dev_cdclk != dev_priv->cdclk_freq)
> +			dev_priv-
> >display.modeset_commit_cdclk(state);
>  	}
>  
>  	/* Now enable the clocks, plane, pipe, and connectors that
> we set up. */
> @@ -13483,24 +13472,12 @@ static int intel_atomic_commit(struct
> drm_device *dev,
>  		bool modeset = needs_modeset(crtc->state);
>  		bool update_pipe = !modeset &&
>  			to_intel_crtc_state(crtc->state)-
> >update_pipe;
> -		unsigned long put_domains = 0;
> -
> -		if (modeset)
> -			intel_display_power_get(dev_priv,
> POWER_DOMAIN_MODESET);
>  
>  		if (modeset && crtc->state->active) {
>  			update_scanline_offset(to_intel_crtc(crtc));
>  			dev_priv->display.crtc_enable(crtc);
>  		}
>  
> -		if (update_pipe) {
> -			put_domains =
> modeset_get_crtc_power_domains(crtc,
> -					      to_intel_crtc_state(cr
> tc->state));
> -
> -			/* make sure intel_modeset_check_state runs
> */
> -			hw_check = true;
> -		}
> -
>  		if (!modeset)
>  			intel_pre_plane_update(to_intel_crtc_state(c
> rtc_state));
>  
> @@ -13511,19 +13488,21 @@ static int intel_atomic_commit(struct
> drm_device *dev,
>  		    (crtc->state->planes_changed || update_pipe))
>  			drm_atomic_helper_commit_planes_on_crtc(crtc
> _state);
>  
> -		if (put_domains)
> -			modeset_put_power_domains(dev_priv,
> put_domains);
> -
>  		intel_post_plane_update(intel_crtc);
> -
> -		if (modeset)
> -			intel_display_power_put(dev_priv,
> POWER_DOMAIN_MODESET);
>  	}
>  
>  	/* FIXME: add subpixel order */
>  
>  	drm_atomic_helper_wait_for_vblanks(dev, state);
>  
> +	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> +		if (put_domains[i])
> +			modeset_put_power_domains(dev_priv,
> put_domains[i]);
> +	}
> +
> +	if (intel_state->modeset)
> +		intel_display_power_put(dev_priv,
> POWER_DOMAIN_MODESET);
> +
>  	mutex_lock(&dev->struct_mutex);
>  	drm_atomic_helper_cleanup_planes(dev, state);
>  	mutex_unlock(&dev->struct_mutex);
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v4 3/8] drm/i915: Kill off intel_crtc->atomic.wait_vblank, v4.

[Zanoni, Paulo R]

Em Qua, 2016-02-10 às 13:49 +0100, Maarten Lankhorst escreveu:
> Currently we perform our own wait in post_plane_update,
> but the atomic core performs another one in wait_for_vblanks.
> This means that 2 vblanks are done when a fb is changed,
> which is a bit overkill.
> 
> Merge them by creating a helper function that takes a crtc mask
> for the planes to wait on.
> 
> The broadwell vblank workaround may look gone entirely but this is
> not the case. pipe_config->wm_changed is set to true
> when any plane is turned on, which forces a vblank wait.
> 
> Changes since v1:
> - Removing the double vblank wait on broadwell moved to its own
> commit.
> Changes since v2:
> - Move out POWER_DOMAIN_MODESET handling to its own commit.
> Changes since v3:
> - Do not wait for vblank on legacy cursor updates. (Ville)
> - Move broadwell vblank workaround comment to page_flip_finished.
> (Ville)
> Changes since v4:
> - Compile fix, legacy_cursor_flip -> *_update.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_atomic.c  |  1 +
>  drivers/gpu/drm/i915/intel_display.c | 86
> +++++++++++++++++++++++++++---------
>  drivers/gpu/drm/i915/intel_drv.h     |  2 +-
>  3 files changed, 67 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_atomic.c
> b/drivers/gpu/drm/i915/intel_atomic.c
> index 4625f8a9ba12..8e579a8505ac 100644
> --- a/drivers/gpu/drm/i915/intel_atomic.c
> +++ b/drivers/gpu/drm/i915/intel_atomic.c
> @@ -97,6 +97,7 @@ intel_crtc_duplicate_state(struct drm_crtc *crtc)
>  	crtc_state->disable_lp_wm = false;
>  	crtc_state->disable_cxsr = false;
>  	crtc_state->wm_changed = false;
> +	crtc_state->fb_changed = false;
>  
>  	return &crtc_state->base;
>  }
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index 804f2c6f260d..4d4dddc1f970 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4785,9 +4785,6 @@ static void intel_post_plane_update(struct
> intel_crtc *crtc)
>  		to_intel_crtc_state(crtc->base.state);
>  	struct drm_device *dev = crtc->base.dev;
>  
> -	if (atomic->wait_vblank)
> -		intel_wait_for_vblank(dev, crtc->pipe);
> -
>  	intel_frontbuffer_flip(dev, atomic->fb_bits);
>  
>  	crtc->wm.cxsr_allowed = true;
> @@ -10902,6 +10899,12 @@ static bool page_flip_finished(struct
> intel_crtc *crtc)
>  		return true;
>  
>  	/*
> +	 * BDW signals flip done immediately if the plane
> +	 * is disabled, even if the plane enable is already
> +	 * armed to occur at the next vblank :(
> +	 */

Having this comment here is just... weird. I think it removes a lot of
the context that was present before.

> +
> +	/*
>  	 * A DSPSURFLIVE check isn't enough in case the mmio and CS
> flips
>  	 * used the same base address. In that case the mmio flip
> might
>  	 * have completed, but the CS hasn't even executed the flip
> yet.
> @@ -11778,6 +11781,9 @@ int intel_plane_atomic_calc_changes(struct
> drm_crtc_state *crtc_state,
>  	if (!was_visible && !visible)
>  		return 0;
>  
> +	if (fb != old_plane_state->base.fb)
> +		pipe_config->fb_changed = true;
> +
>  	turn_off = was_visible && (!visible || mode_changed);
>  	turn_on = visible && (!was_visible || mode_changed);
>  
> @@ -11793,8 +11799,6 @@ int intel_plane_atomic_calc_changes(struct
> drm_crtc_state *crtc_state,
>  
>  		/* must disable cxsr around plane enable/disable */
>  		if (plane->type != DRM_PLANE_TYPE_CURSOR) {
> -			if (is_crtc_enabled)
> -				intel_crtc->atomic.wait_vblank =
> true;
>  			pipe_config->disable_cxsr = true;
>  		}

We could have killed the brackets here :)

>  	} else if (intel_wm_need_update(plane, plane_state)) {
> @@ -11810,14 +11814,6 @@ int intel_plane_atomic_calc_changes(struct
> drm_crtc_state *crtc_state,
>  		intel_crtc->atomic.post_enable_primary = turn_on;
>  		intel_crtc->atomic.update_fbc = true;
>  
> -		/*
> -		 * BDW signals flip done immediately if the plane
> -		 * is disabled, even if the plane enable is already
> -		 * armed to occur at the next vblank :(
> -		 */
> -		if (turn_on && IS_BROADWELL(dev))
> -			intel_crtc->atomic.wait_vblank = true;
> -
>  		break;
>  	case DRM_PLANE_TYPE_CURSOR:
>  		break;
> @@ -11831,12 +11827,10 @@ int intel_plane_atomic_calc_changes(struct
> drm_crtc_state *crtc_state,
>  		if (IS_IVYBRIDGE(dev) &&
>  		    needs_scaling(to_intel_plane_state(plane_state))
> &&
>  		    !needs_scaling(old_plane_state)) {
> -			to_intel_crtc_state(crtc_state)-
> >disable_lp_wm = true;
> -		} else if (turn_off && !mode_changed) {
> -			intel_crtc->atomic.wait_vblank = true;
> +			pipe_config->disable_lp_wm = true;
> +		} else if (turn_off && !mode_changed)
>  			intel_crtc->atomic.update_sprite_watermarks
> |=
>  				1 << i;
> -		}

Weird brackets here. Either kill on both sides of the if statement (the
preferred way), or none.

Also, shouldn't we introduce pipe_config->sprite_changed? What's
guaranteeing that we're going to definitely wait for a vblank during
sprite updates? I like explicit dumb-proof code instead of implications
such as "we're doing waits during sprite updates because whenever we
update sprites, a specific unrelated variable that's used for a
different purpose gets set to true, and we check for this variable".

>  
>  		break;
>  	}
> @@ -13370,6 +13364,48 @@ static int
> intel_atomic_prepare_commit(struct drm_device *dev,
>  	return ret;
>  }
>  
> +static void intel_atomic_wait_for_vblanks(struct drm_device *dev,
> +					  struct drm_i915_private
> *dev_priv,
> +					  unsigned crtc_mask)
> +{
> +	unsigned last_vblank_count[I915_MAX_PIPES];
> +	enum pipe pipe;
> +	int ret;
> +
> +	if (!crtc_mask)
> +		return;
> +
> +	for_each_pipe(dev_priv, pipe) {
> +		struct drm_crtc *crtc = dev_priv-
> >pipe_to_crtc_mapping[pipe];
> +
> +		if (!((1 << pipe) & crtc_mask))
> +			continue;
> +
> +		ret = drm_crtc_vblank_get(crtc);
> +		if (ret != 0) {
> +			crtc_mask &= ~(1 << pipe);
> +			continue;

Shouldn't we DRM_ERROR here?

> +		}
> +
> +		last_vblank_count[pipe] =
> drm_crtc_vblank_count(crtc);
> +	}
> +
> +	for_each_pipe(dev_priv, pipe) {
> +		struct drm_crtc *crtc = dev_priv-
> >pipe_to_crtc_mapping[pipe];
> +
> +		if (!((1 << pipe) & crtc_mask))
> +			continue;
> +
> +		wait_event_timeout(dev->vblank[pipe].queue,
> +				last_vblank_count[pipe] !=
> +					drm_crtc_vblank_count(crtc),
> +				msecs_to_jiffies(50));

Also maybe DRM_ERROR in case we hit the timeout?

I know the original code doesn't have this, but now that we're doing or
own thing, we may scream in unexpected cases.

> +
> +		drm_crtc_vblank_put(crtc);
> +	}
> +}
> +
> +

Two newlines :)

>  /**
>   * intel_atomic_commit - commit validated state object
>   * @dev: DRM device
> @@ -13397,6 +13433,7 @@ static int intel_atomic_commit(struct
> drm_device *dev,
>  	int ret = 0, i;
>  	bool hw_check = intel_state->modeset;
>  	unsigned long put_domains[I915_MAX_PIPES] = {};
> +	unsigned crtc_vblank_mask = 0;
>  
>  	ret = intel_atomic_prepare_commit(dev, state, async);
>  	if (ret) {
> @@ -13470,8 +13507,9 @@ static int intel_atomic_commit(struct
> drm_device *dev,
>  	for_each_crtc_in_state(state, crtc, crtc_state, i) {
>  		struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  		bool modeset = needs_modeset(crtc->state);
> -		bool update_pipe = !modeset &&
> -			to_intel_crtc_state(crtc->state)-
> >update_pipe;
> +		struct intel_crtc_state *pipe_config =
> +			to_intel_crtc_state(crtc->state);
> +		bool update_pipe = !modeset && pipe_config-
> >update_pipe;
>  
>  		if (modeset && crtc->state->active) {
>  			update_scanline_offset(to_intel_crtc(crtc));
> @@ -13488,14 +13526,20 @@ static int intel_atomic_commit(struct
> drm_device *dev,
>  		    (crtc->state->planes_changed || update_pipe))
>  			drm_atomic_helper_commit_planes_on_crtc(crtc
> _state);
>  
> -		intel_post_plane_update(intel_crtc);
> +		if (pipe_config->base.active &&
> +		    (pipe_config->wm_changed || pipe_config-
> >disable_cxsr ||
> +		     pipe_config->fb_changed))

So the wm_changed is just for the BDW workaround + sprites? What about
this disable_cxsr, why is it here? Also see my comment above about
sprite_changed. Maybe we need some comments here to explain the complex
checks.


> +			crtc_vblank_mask |= 1 << i;
>  	}
>  
>  	/* FIXME: add subpixel order */
>  
> -	drm_atomic_helper_wait_for_vblanks(dev, state);
> +	if (!state->legacy_cursor_update)
> +		intel_atomic_wait_for_vblanks(dev, dev_priv,
> crtc_vblank_mask);
>  
>  	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> +		intel_post_plane_update(to_intel_crtc(crtc));
> +
>  		if (put_domains[i])
>  			modeset_put_power_domains(dev_priv,
> put_domains[i]);
>  	}
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index 335e6b24b0bc..e911c86f873b 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -379,6 +379,7 @@ struct intel_crtc_state {
>  	bool update_pipe; /* can a fast modeset be performed? */
>  	bool disable_cxsr;
>  	bool wm_changed; /* watermarks are updated */
> +	bool fb_changed; /* fb on any of the planes is changed */
>  
>  	/* Pipe source size (ie. panel fitter input size)
>  	 * All planes will be positioned inside this space,
> @@ -547,7 +548,6 @@ struct intel_crtc_atomic_commit {
>  
>  	/* Sleepable operations to perform after commit */
>  	unsigned fb_bits;
> -	bool wait_vblank;

One of the things that I like about the code without this patch is that
it's very explicit on when we need to wait for vblanks (except for the
problem where we wait twice). The new code is not as easy to
read/understand as the old one. This comment is similar to the one I
made in patch 6: I'm not sure if sacrificing readability is worth it.


I wonder that maybe the cleanest fix to the "we're waiting 2 vblanks"
problem is to just remove the call to
drm_atomic_helper_wait_for_vblanks(), which would be a first patch.
Then we'd have a second patch introducing
intel_atomic_wait_for_vblanks() for the "wait for all vblanks at the
same time" optimization, and then a last commit possibly replacing
commit->wait_vblank for state->fb_changed. Just an idea, not a request.


I'll wait for your answers before reaching any conclusions of what I
think should be done, since I may be misunderstanding something.

>  	bool post_enable_primary;
>  	unsigned update_sprite_watermarks;
>  
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v4 1/8] drm/i915: Pass crtc state to modeset_get_crtc_power_domains.

[Maarten Lankhorst]

Op 17-02-16 om 18:54 schreef Zanoni, Paulo R:
> Em Qua, 2016-02-10 às 13:49 +0100, Maarten Lankhorst escreveu:
>> Use our newly created encoder_mask to iterate over the encoders.
> As someone who was not paying attention to the discussion of the
> previous patches related to this, I think it would be really good if
> your commit message could tell me why we should use the newly created
> encoder_mask instead of the current patch. What's bad about the current
> version? Please sell me your patch. If you think the answer is trivial,
> remember that it's not trivial to many people, and that random people
> may find this patch through git-bisect and have to judge its
> importance. Also, an explanation really helps the reviewers :)
>
> The patch looks correct, so if you improve the commit message:
> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

Does this sound good?

Use our newly created encoder_mask to iterate over the encoders.
This makes it possible to get the crtc power domains from the
crtc_state at any time, without any locks or having to look at
the legacy state.

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

Re: [PATCH v4 1/8] drm/i915: Pass crtc state to modeset_get_crtc_power_domains.

[Zanoni, Paulo R]

Em Qui, 2016-02-18 às 10:51 +0100, Maarten Lankhorst escreveu:
> Op 17-02-16 om 18:54 schreef Zanoni, Paulo R:
> > Em Qua, 2016-02-10 às 13:49 +0100, Maarten Lankhorst escreveu:
> > > Use our newly created encoder_mask to iterate over the encoders.
> > As someone who was not paying attention to the discussion of the
> > previous patches related to this, I think it would be really good
> > if
> > your commit message could tell me why we should use the newly
> > created
> > encoder_mask instead of the current patch. What's bad about the
> > current
> > version? Please sell me your patch. If you think the answer is
> > trivial,
> > remember that it's not trivial to many people, and that random
> > people
> > may find this patch through git-bisect and have to judge its
> > importance. Also, an explanation really helps the reviewers :)
> > 
> > The patch looks correct, so if you improve the commit message:
> > Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> Does this sound good?
> 
> Use our newly created encoder_mask to iterate over the encoders.
> This makes it possible to get the crtc power domains from the
> crtc_state at any time, without any locks or having to look at
> the legacy state.

But we were already not grabbing any locks. Is this a bug fix? It would
be good to point it if it's an actual fix.

Anyway, it's definitely better now :)

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

Re: [PATCH v4 1/8] drm/i915: Pass crtc state to modeset_get_crtc_power_domains.

[Maarten Lankhorst]

Op 18-02-16 om 14:08 schreef Zanoni, Paulo R:
> Em Qui, 2016-02-18 às 10:51 +0100, Maarten Lankhorst escreveu:
>> Op 17-02-16 om 18:54 schreef Zanoni, Paulo R:
>>> Em Qua, 2016-02-10 às 13:49 +0100, Maarten Lankhorst escreveu:
>>>> Use our newly created encoder_mask to iterate over the encoders.
>>> As someone who was not paying attention to the discussion of the
>>> previous patches related to this, I think it would be really good
>>> if
>>> your commit message could tell me why we should use the newly
>>> created
>>> encoder_mask instead of the current patch. What's bad about the
>>> current
>>> version? Please sell me your patch. If you think the answer is
>>> trivial,
>>> remember that it's not trivial to many people, and that random
>>> people
>>> may find this patch through git-bisect and have to judge its
>>> importance. Also, an explanation really helps the reviewers :)
>>>
>>> The patch looks correct, so if you improve the commit message:
>>> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> Does this sound good?
>>
>> Use our newly created encoder_mask to iterate over the encoders.
>> This makes it possible to get the crtc power domains from the
>> crtc_state at any time, without any locks or having to look at
>> the legacy state.
> But we were already not grabbing any locks. Is this a bug fix? It would
> be good to point it if it's an actual fix.
>
> Anyway, it's definitely better now :)
>
But we're holding locks, crtc.mutex and mode_config.connection_mutex.

The changes allows us to remove the need of the locks, just a crtc_state is enough.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v4 3/8] drm/i915: Kill off intel_crtc->atomic.wait_vblank, v4.

[Maarten Lankhorst]

Op 17-02-16 om 22:20 schreef Zanoni, Paulo R:
> Em Qua, 2016-02-10 às 13:49 +0100, Maarten Lankhorst escreveu:
>> Currently we perform our own wait in post_plane_update,
>> but the atomic core performs another one in wait_for_vblanks.
>> This means that 2 vblanks are done when a fb is changed,
>> which is a bit overkill.
>>
>> Merge them by creating a helper function that takes a crtc mask
>> for the planes to wait on.
>>
>> The broadwell vblank workaround may look gone entirely but this is
>> not the case. pipe_config->wm_changed is set to true
>> when any plane is turned on, which forces a vblank wait.
>>
>> Changes since v1:
>> - Removing the double vblank wait on broadwell moved to its own
>> commit.
>> Changes since v2:
>> - Move out POWER_DOMAIN_MODESET handling to its own commit.
>> Changes since v3:
>> - Do not wait for vblank on legacy cursor updates. (Ville)
>> - Move broadwell vblank workaround comment to page_flip_finished.
>> (Ville)
>> Changes since v4:
>> - Compile fix, legacy_cursor_flip -> *_update.
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_atomic.c  |  1 +
>>  drivers/gpu/drm/i915/intel_display.c | 86
>> +++++++++++++++++++++++++++---------
>>  drivers/gpu/drm/i915/intel_drv.h     |  2 +-
>>  3 files changed, 67 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_atomic.c
>> b/drivers/gpu/drm/i915/intel_atomic.c
>> index 4625f8a9ba12..8e579a8505ac 100644
>> --- a/drivers/gpu/drm/i915/intel_atomic.c
>> +++ b/drivers/gpu/drm/i915/intel_atomic.c
>> @@ -97,6 +97,7 @@ intel_crtc_duplicate_state(struct drm_crtc *crtc)
>>  	crtc_state->disable_lp_wm = false;
>>  	crtc_state->disable_cxsr = false;
>>  	crtc_state->wm_changed = false;
>> +	crtc_state->fb_changed = false;
>>  
>>  	return &crtc_state->base;
>>  }
>> diff --git a/drivers/gpu/drm/i915/intel_display.c
>> b/drivers/gpu/drm/i915/intel_display.c
>> index 804f2c6f260d..4d4dddc1f970 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -4785,9 +4785,6 @@ static void intel_post_plane_update(struct
>> intel_crtc *crtc)
>>  		to_intel_crtc_state(crtc->base.state);
>>  	struct drm_device *dev = crtc->base.dev;
>>  
>> -	if (atomic->wait_vblank)
>> -		intel_wait_for_vblank(dev, crtc->pipe);
>> -
>>  	intel_frontbuffer_flip(dev, atomic->fb_bits);
>>  
>>  	crtc->wm.cxsr_allowed = true;
>> @@ -10902,6 +10899,12 @@ static bool page_flip_finished(struct
>> intel_crtc *crtc)
>>  		return true;
>>  
>>  	/*
>> +	 * BDW signals flip done immediately if the plane
>> +	 * is disabled, even if the plane enable is already
>> +	 * armed to occur at the next vblank :(
>> +	 */
> Having this comment here is just... weird. I think it removes a lot of
> the context that was present before.
>
>> +
>> +	/*
>>  	 * A DSPSURFLIVE check isn't enough in case the mmio and CS
>> flips
>>  	 * used the same base address. In that case the mmio flip
>> might
>>  	 * have completed, but the CS hasn't even executed the flip
>> yet.
>> @@ -11778,6 +11781,9 @@ int intel_plane_atomic_calc_changes(struct
>> drm_crtc_state *crtc_state,
>>  	if (!was_visible && !visible)
>>  		return 0;
>>  
>> +	if (fb != old_plane_state->base.fb)
>> +		pipe_config->fb_changed = true;
>> +
>>  	turn_off = was_visible && (!visible || mode_changed);
>>  	turn_on = visible && (!was_visible || mode_changed);
>>  
>> @@ -11793,8 +11799,6 @@ int intel_plane_atomic_calc_changes(struct
>> drm_crtc_state *crtc_state,
>>  
>>  		/* must disable cxsr around plane enable/disable */
>>  		if (plane->type != DRM_PLANE_TYPE_CURSOR) {
>> -			if (is_crtc_enabled)
>> -				intel_crtc->atomic.wait_vblank =
>> true;
>>  			pipe_config->disable_cxsr = true;
>>  		}
> We could have killed the brackets here :)
Indeed, will do so in next version.
>>  	} else if (intel_wm_need_update(plane, plane_state)) {
>> @@ -11810,14 +11814,6 @@ int intel_plane_atomic_calc_changes(struct
>> drm_crtc_state *crtc_state,
>>  		intel_crtc->atomic.post_enable_primary = turn_on;
>>  		intel_crtc->atomic.update_fbc = true;
>>  
>> -		/*
>> -		 * BDW signals flip done immediately if the plane
>> -		 * is disabled, even if the plane enable is already
>> -		 * armed to occur at the next vblank :(
>> -		 */
>> -		if (turn_on && IS_BROADWELL(dev))
>> -			intel_crtc->atomic.wait_vblank = true;
>> -
>>  		break;
>>  	case DRM_PLANE_TYPE_CURSOR:
>>  		break;
>> @@ -11831,12 +11827,10 @@ int intel_plane_atomic_calc_changes(struct
>> drm_crtc_state *crtc_state,
>>  		if (IS_IVYBRIDGE(dev) &&
>>  		    needs_scaling(to_intel_plane_state(plane_state))
>> &&
>>  		    !needs_scaling(old_plane_state)) {
>> -			to_intel_crtc_state(crtc_state)-
>>> disable_lp_wm = true;
>> -		} else if (turn_off && !mode_changed) {
>> -			intel_crtc->atomic.wait_vblank = true;
>> +			pipe_config->disable_lp_wm = true;
>> +		} else if (turn_off && !mode_changed)
>>  			intel_crtc->atomic.update_sprite_watermarks
>> |=
>>  				1 << i;
>> -		}
> Weird brackets here. Either kill on both sides of the if statement (the
> preferred way), or none.
>
> Also, shouldn't we introduce pipe_config->sprite_changed? What's
> guaranteeing that we're going to definitely wait for a vblank during
> sprite updates? I like explicit dumb-proof code instead of implications
> such as "we're doing waits during sprite updates because whenever we
> update sprites, a specific unrelated variable that's used for a
> different purpose gets set to true, and we check for this variable".
sprite_changed would be same as fb_changed + wm_changed, and update_sprite_watermarks gets removed in this series
because nothing uses it.
>>  
>>  		break;
>>  	}
>> @@ -13370,6 +13364,48 @@ static int
>> intel_atomic_prepare_commit(struct drm_device *dev,
>>  	return ret;
>>  }
>>  
>> +static void intel_atomic_wait_for_vblanks(struct drm_device *dev,
>> +					  struct drm_i915_private
>> *dev_priv,
>> +					  unsigned crtc_mask)
>> +{
>> +	unsigned last_vblank_count[I915_MAX_PIPES];
>> +	enum pipe pipe;
>> +	int ret;
>> +
>> +	if (!crtc_mask)
>> +		return;
>> +
>> +	for_each_pipe(dev_priv, pipe) {
>> +		struct drm_crtc *crtc = dev_priv-
>>> pipe_to_crtc_mapping[pipe];
>> +
>> +		if (!((1 << pipe) & crtc_mask))
>> +			continue;
>> +
>> +		ret = drm_crtc_vblank_get(crtc);
>> +		if (ret != 0) {
>> +			crtc_mask &= ~(1 << pipe);
>> +			continue;
> Shouldn't we DRM_ERROR here?
WARN_ON is enough, this shouldn't ever happen.
>> +		}
>> +
>> +		last_vblank_count[pipe] =
>> drm_crtc_vblank_count(crtc);
>> +	}
>> +
>> +	for_each_pipe(dev_priv, pipe) {
>> +		struct drm_crtc *crtc = dev_priv-
>>> pipe_to_crtc_mapping[pipe];
>> +
>> +		if (!((1 << pipe) & crtc_mask))
>> +			continue;
>> +
>> +		wait_event_timeout(dev->vblank[pipe].queue,
>> +				last_vblank_count[pipe] !=
>> +					drm_crtc_vblank_count(crtc),
>> +				msecs_to_jiffies(50));
> Also maybe DRM_ERROR in case we hit the timeout?
>
> I know the original code doesn't have this, but now that we're doing or
> own thing, we may scream in unexpected cases.
I'll make it a WARN_ON, shouldn't happen.
>> +
>> +		drm_crtc_vblank_put(crtc);
>> +	}
>> +}
>> +
>> +
> Two newlines :)
Indeed!
>>  /**
>>   * intel_atomic_commit - commit validated state object
>>   * @dev: DRM device
>> @@ -13397,6 +13433,7 @@ static int intel_atomic_commit(struct
>> drm_device *dev,
>>  	int ret = 0, i;
>>  	bool hw_check = intel_state->modeset;
>>  	unsigned long put_domains[I915_MAX_PIPES] = {};
>> +	unsigned crtc_vblank_mask = 0;
>>  
>>  	ret = intel_atomic_prepare_commit(dev, state, async);
>>  	if (ret) {
>> @@ -13470,8 +13507,9 @@ static int intel_atomic_commit(struct
>> drm_device *dev,
>>  	for_each_crtc_in_state(state, crtc, crtc_state, i) {
>>  		struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>>  		bool modeset = needs_modeset(crtc->state);
>> -		bool update_pipe = !modeset &&
>> -			to_intel_crtc_state(crtc->state)-
>>> update_pipe;
>> +		struct intel_crtc_state *pipe_config =
>> +			to_intel_crtc_state(crtc->state);
>> +		bool update_pipe = !modeset && pipe_config-
>>> update_pipe;
>>  
>>  		if (modeset && crtc->state->active) {
>>  			update_scanline_offset(to_intel_crtc(crtc));
>> @@ -13488,14 +13526,20 @@ static int intel_atomic_commit(struct
>> drm_device *dev,
>>  		    (crtc->state->planes_changed || update_pipe))
>>  			drm_atomic_helper_commit_planes_on_crtc(crtc
>> _state);
>>  
>> -		intel_post_plane_update(intel_crtc);
>> +		if (pipe_config->base.active &&
>> +		    (pipe_config->wm_changed || pipe_config-
>>> disable_cxsr ||
>> +		     pipe_config->fb_changed))
> So the wm_changed is just for the BDW workaround + sprites? What about
> this disable_cxsr, why is it here? Also see my comment above about
> sprite_changed. Maybe we need some comments here to explain the complex
> checks.
No it's waiting for a vblank for post_plane_update so all optimized wm values
can get written, it's not just for the BDW workaround.
It just happens to be that the BDW w/a gets applied too as a side effect.
>> +			crtc_vblank_mask |= 1 << i;
>>  	}
>>  
>>  	/* FIXME: add subpixel order */
>>  
>> -	drm_atomic_helper_wait_for_vblanks(dev, state);
>> +	if (!state->legacy_cursor_update)
>> +		intel_atomic_wait_for_vblanks(dev, dev_priv,
>> crtc_vblank_mask);
>>  
>>  	for_each_crtc_in_state(state, crtc, crtc_state, i) {
>> +		intel_post_plane_update(to_intel_crtc(crtc));
>> +
>>  		if (put_domains[i])
>>  			modeset_put_power_domains(dev_priv,
>> put_domains[i]);
>>  	}
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h
>> b/drivers/gpu/drm/i915/intel_drv.h
>> index 335e6b24b0bc..e911c86f873b 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -379,6 +379,7 @@ struct intel_crtc_state {
>>  	bool update_pipe; /* can a fast modeset be performed? */
>>  	bool disable_cxsr;
>>  	bool wm_changed; /* watermarks are updated */
>> +	bool fb_changed; /* fb on any of the planes is changed */
>>  
>>  	/* Pipe source size (ie. panel fitter input size)
>>  	 * All planes will be positioned inside this space,
>> @@ -547,7 +548,6 @@ struct intel_crtc_atomic_commit {
>>  
>>  	/* Sleepable operations to perform after commit */
>>  	unsigned fb_bits;
>> -	bool wait_vblank;
> One of the things that I like about the code without this patch is that
> it's very explicit on when we need to wait for vblanks (except for the
> problem where we wait twice). The new code is not as easy to
> read/understand as the old one. This comment is similar to the one I
> made in patch 6: I'm not sure if sacrificing readability is worth it.
>
> I wonder that maybe the cleanest fix to the "we're waiting 2 vblanks"
> problem is to just remove the call to
> drm_atomic_helper_wait_for_vblanks(), which would be a first patch.
> Then we'd have a second patch introducing
> intel_atomic_wait_for_vblanks() for the "wait for all vblanks at the
> same time" optimization, and then a last commit possibly replacing
> commit->wait_vblank for state->fb_changed. Just an idea, not a request.
There were cases in which the atomic helper would wait for vblanks which
wouldn't trigger any of the other changes, so it can't be blindly removed. Mostly when
updating a plane with a same sized fb.
Wait for vblank is required for unpinning, it would be bad if the current fb is unpinned.

> I'll wait for your answers before reaching any conclusions of what I
> think should be done, since I may be misunderstanding something.
I want to call all post flip work scheduled later on so it happens after the next vblank.
That will remove all confusion on when a vblank is required, because all post_plane_update
and unpin will always wait until next vblank.

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

Re: [PATCH v4 3/8] drm/i915: Kill off intel_crtc->atomic.wait_vblank, v4.

[Zanoni, Paulo R]

Em Qui, 2016-02-18 às 14:22 +0100, Maarten Lankhorst escreveu:
> Op 17-02-16 om 22:20 schreef Zanoni, Paulo R:
> > Em Qua, 2016-02-10 às 13:49 +0100, Maarten Lankhorst escreveu:
> > > Currently we perform our own wait in post_plane_update,
> > > but the atomic core performs another one in wait_for_vblanks.
> > > This means that 2 vblanks are done when a fb is changed,
> > > which is a bit overkill.
> > > 
> > > Merge them by creating a helper function that takes a crtc mask
> > > for the planes to wait on.
> > > 
> > > The broadwell vblank workaround may look gone entirely but this
> > > is
> > > not the case. pipe_config->wm_changed is set to true
> > > when any plane is turned on, which forces a vblank wait.
> > > 
> > > Changes since v1:
> > > - Removing the double vblank wait on broadwell moved to its own
> > > commit.
> > > Changes since v2:
> > > - Move out POWER_DOMAIN_MODESET handling to its own commit.
> > > Changes since v3:
> > > - Do not wait for vblank on legacy cursor updates. (Ville)
> > > - Move broadwell vblank workaround comment to page_flip_finished.
> > > (Ville)
> > > Changes since v4:
> > > - Compile fix, legacy_cursor_flip -> *_update.
> > > 
> > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.c
> > > om>
> > > ---
> > >  drivers/gpu/drm/i915/intel_atomic.c  |  1 +
> > >  drivers/gpu/drm/i915/intel_display.c | 86
> > > +++++++++++++++++++++++++++---------
> > >  drivers/gpu/drm/i915/intel_drv.h     |  2 +-
> > >  3 files changed, 67 insertions(+), 22 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_atomic.c
> > > b/drivers/gpu/drm/i915/intel_atomic.c
> > > index 4625f8a9ba12..8e579a8505ac 100644
> > > --- a/drivers/gpu/drm/i915/intel_atomic.c
> > > +++ b/drivers/gpu/drm/i915/intel_atomic.c
> > > @@ -97,6 +97,7 @@ intel_crtc_duplicate_state(struct drm_crtc
> > > *crtc)
> > >  	crtc_state->disable_lp_wm = false;
> > >  	crtc_state->disable_cxsr = false;
> > >  	crtc_state->wm_changed = false;
> > > +	crtc_state->fb_changed = false;
> > >  
> > >  	return &crtc_state->base;
> > >  }
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > > b/drivers/gpu/drm/i915/intel_display.c
> > > index 804f2c6f260d..4d4dddc1f970 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -4785,9 +4785,6 @@ static void intel_post_plane_update(struct
> > > intel_crtc *crtc)
> > >  		to_intel_crtc_state(crtc->base.state);
> > >  	struct drm_device *dev = crtc->base.dev;
> > >  
> > > -	if (atomic->wait_vblank)
> > > -		intel_wait_for_vblank(dev, crtc->pipe);
> > > -
> > >  	intel_frontbuffer_flip(dev, atomic->fb_bits);
> > >  
> > >  	crtc->wm.cxsr_allowed = true;
> > > @@ -10902,6 +10899,12 @@ static bool page_flip_finished(struct
> > > intel_crtc *crtc)
> > >  		return true;
> > >  
> > >  	/*
> > > +	 * BDW signals flip done immediately if the plane
> > > +	 * is disabled, even if the plane enable is already
> > > +	 * armed to occur at the next vblank :(
> > > +	 */
> > Having this comment here is just... weird. I think it removes a lot
> > of
> > the context that was present before.
> > 
> > > +
> > > +	/*
> > >  	 * A DSPSURFLIVE check isn't enough in case the mmio and
> > > CS
> > > flips
> > >  	 * used the same base address. In that case the mmio
> > > flip
> > > might
> > >  	 * have completed, but the CS hasn't even executed the
> > > flip
> > > yet.
> > > @@ -11778,6 +11781,9 @@ int
> > > intel_plane_atomic_calc_changes(struct
> > > drm_crtc_state *crtc_state,
> > >  	if (!was_visible && !visible)
> > >  		return 0;
> > >  
> > > +	if (fb != old_plane_state->base.fb)
> > > +		pipe_config->fb_changed = true;
> > > +
> > >  	turn_off = was_visible && (!visible || mode_changed);
> > >  	turn_on = visible && (!was_visible || mode_changed);
> > >  
> > > @@ -11793,8 +11799,6 @@ int
> > > intel_plane_atomic_calc_changes(struct
> > > drm_crtc_state *crtc_state,
> > >  
> > >  		/* must disable cxsr around plane enable/disable
> > > */
> > >  		if (plane->type != DRM_PLANE_TYPE_CURSOR) {
> > > -			if (is_crtc_enabled)
> > > -				intel_crtc->atomic.wait_vblank =
> > > true;
> > >  			pipe_config->disable_cxsr = true;
> > >  		}
> > We could have killed the brackets here :)
> Indeed, will do so in next version.
> > >  	} else if (intel_wm_need_update(plane, plane_state)) {
> > > @@ -11810,14 +11814,6 @@ int
> > > intel_plane_atomic_calc_changes(struct
> > > drm_crtc_state *crtc_state,
> > >  		intel_crtc->atomic.post_enable_primary =
> > > turn_on;
> > >  		intel_crtc->atomic.update_fbc = true;
> > >  
> > > -		/*
> > > -		 * BDW signals flip done immediately if the
> > > plane
> > > -		 * is disabled, even if the plane enable is
> > > already
> > > -		 * armed to occur at the next vblank :(
> > > -		 */
> > > -		if (turn_on && IS_BROADWELL(dev))
> > > -			intel_crtc->atomic.wait_vblank = true;
> > > -
> > >  		break;
> > >  	case DRM_PLANE_TYPE_CURSOR:
> > >  		break;
> > > @@ -11831,12 +11827,10 @@ int
> > > intel_plane_atomic_calc_changes(struct
> > > drm_crtc_state *crtc_state,
> > >  		if (IS_IVYBRIDGE(dev) &&
> > >  		    needs_scaling(to_intel_plane_state(plane_sta
> > > te))
> > > &&
> > >  		    !needs_scaling(old_plane_state)) {
> > > -			to_intel_crtc_state(crtc_state)-
> > > > disable_lp_wm = true;
> > > -		} else if (turn_off && !mode_changed) {
> > > -			intel_crtc->atomic.wait_vblank = true;
> > > +			pipe_config->disable_lp_wm = true;
> > > +		} else if (turn_off && !mode_changed)
> > >  			intel_crtc-
> > > >atomic.update_sprite_watermarks
> > > > =
> > >  				1 << i;
> > > -		}
> > Weird brackets here. Either kill on both sides of the if statement
> > (the
> > preferred way), or none.
> > 
> > Also, shouldn't we introduce pipe_config->sprite_changed? What's
> > guaranteeing that we're going to definitely wait for a vblank
> > during
> > sprite updates? I like explicit dumb-proof code instead of
> > implications
> > such as "we're doing waits during sprite updates because whenever
> > we
> > update sprites, a specific unrelated variable that's used for a
> > different purpose gets set to true, and we check for this
> > variable".
> sprite_changed would be same as fb_changed + wm_changed, and
> update_sprite_watermarks gets removed in this series
> because nothing uses it.

Hmmm, right. For some reason, I was interpreting fb_changed as being
only valid for the primary fb, not for spr/cur. My bad. So fb_changed
means "if any of pri/cur/spr changed" I guess. Maybe planes_changed
could be a better name, or fbs_changed (plural), since we're talking
about more then one possible plane here. Not a requirement, just
throwing the idea.

> > >  
> > >  		break;
> > >  	}
> > > @@ -13370,6 +13364,48 @@ static int
> > > intel_atomic_prepare_commit(struct drm_device *dev,
> > >  	return ret;
> > >  }
> > >  
> > > +static void intel_atomic_wait_for_vblanks(struct drm_device
> > > *dev,
> > > +					  struct
> > > drm_i915_private
> > > *dev_priv,
> > > +					  unsigned crtc_mask)
> > > +{
> > > +	unsigned last_vblank_count[I915_MAX_PIPES];
> > > +	enum pipe pipe;
> > > +	int ret;
> > > +
> > > +	if (!crtc_mask)
> > > +		return;
> > > +
> > > +	for_each_pipe(dev_priv, pipe) {
> > > +		struct drm_crtc *crtc = dev_priv-
> > > > pipe_to_crtc_mapping[pipe];
> > > +
> > > +		if (!((1 << pipe) & crtc_mask))
> > > +			continue;
> > > +
> > > +		ret = drm_crtc_vblank_get(crtc);
> > > +		if (ret != 0) {
> > > +			crtc_mask &= ~(1 << pipe);
> > > +			continue;
> > Shouldn't we DRM_ERROR here?
> WARN_ON is enough, this shouldn't ever happen.

Even better :)

> > > +		}
> > > +
> > > +		last_vblank_count[pipe] =
> > > drm_crtc_vblank_count(crtc);
> > > +	}
> > > +
> > > +	for_each_pipe(dev_priv, pipe) {
> > > +		struct drm_crtc *crtc = dev_priv-
> > > > pipe_to_crtc_mapping[pipe];
> > > +
> > > +		if (!((1 << pipe) & crtc_mask))
> > > +			continue;
> > > +
> > > +		wait_event_timeout(dev->vblank[pipe].queue,
> > > +				last_vblank_count[pipe] !=
> > > +					drm_crtc_vblank_count(cr
> > > tc),
> > > +				msecs_to_jiffies(50));
> > Also maybe DRM_ERROR in case we hit the timeout?
> > 
> > I know the original code doesn't have this, but now that we're
> > doing or
> > own thing, we may scream in unexpected cases.
> I'll make it a WARN_ON, shouldn't happen.
> > > +
> > > +		drm_crtc_vblank_put(crtc);
> > > +	}
> > > +}
> > > +
> > > +
> > Two newlines :)
> Indeed!
> > >  /**
> > >   * intel_atomic_commit - commit validated state object
> > >   * @dev: DRM device
> > > @@ -13397,6 +13433,7 @@ static int intel_atomic_commit(struct
> > > drm_device *dev,
> > >  	int ret = 0, i;
> > >  	bool hw_check = intel_state->modeset;
> > >  	unsigned long put_domains[I915_MAX_PIPES] = {};
> > > +	unsigned crtc_vblank_mask = 0;
> > >  
> > >  	ret = intel_atomic_prepare_commit(dev, state, async);
> > >  	if (ret) {
> > > @@ -13470,8 +13507,9 @@ static int intel_atomic_commit(struct
> > > drm_device *dev,
> > >  	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> > >  		struct intel_crtc *intel_crtc =
> > > to_intel_crtc(crtc);
> > >  		bool modeset = needs_modeset(crtc->state);
> > > -		bool update_pipe = !modeset &&
> > > -			to_intel_crtc_state(crtc->state)-
> > > > update_pipe;
> > > +		struct intel_crtc_state *pipe_config =
> > > +			to_intel_crtc_state(crtc->state);
> > > +		bool update_pipe = !modeset && pipe_config-
> > > > update_pipe;
> > >  
> > >  		if (modeset && crtc->state->active) {
> > >  			update_scanline_offset(to_intel_crtc(crt
> > > c));
> > > @@ -13488,14 +13526,20 @@ static int intel_atomic_commit(struct
> > > drm_device *dev,
> > >  		    (crtc->state->planes_changed ||
> > > update_pipe))
> > >  			drm_atomic_helper_commit_planes_on_crtc(
> > > crtc
> > > _state);
> > >  
> > > -		intel_post_plane_update(intel_crtc);
> > > +		if (pipe_config->base.active &&
> > > +		    (pipe_config->wm_changed || pipe_config-
> > > > disable_cxsr ||
> > > +		     pipe_config->fb_changed))
> > So the wm_changed is just for the BDW workaround + sprites? What
> > about
> > this disable_cxsr, why is it here? Also see my comment above about
> > sprite_changed. Maybe we need some comments here to explain the
> > complex
> > checks.
> No it's waiting for a vblank for post_plane_update so all optimized
> wm values
> can get written, it's not just for the BDW workaround.
> It just happens to be that the BDW w/a gets applied too as a side
> effect.

So maybe some comment regarding the BDW WA could be here.

What about disable_cxsr? Why is this here?

> > > +			crtc_vblank_mask |= 1 << i;
> > >  	}
> > >  
> > >  	/* FIXME: add subpixel order */
> > >  
> > > -	drm_atomic_helper_wait_for_vblanks(dev, state);
> > > +	if (!state->legacy_cursor_update)
> > > +		intel_atomic_wait_for_vblanks(dev, dev_priv,
> > > crtc_vblank_mask);
> > >  
> > >  	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> > > +		intel_post_plane_update(to_intel_crtc(crtc));
> > > +
> > >  		if (put_domains[i])
> > >  			modeset_put_power_domains(dev_priv,
> > > put_domains[i]);
> > >  	}
> > > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > > b/drivers/gpu/drm/i915/intel_drv.h
> > > index 335e6b24b0bc..e911c86f873b 100644
> > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > @@ -379,6 +379,7 @@ struct intel_crtc_state {
> > >  	bool update_pipe; /* can a fast modeset be performed? */
> > >  	bool disable_cxsr;
> > >  	bool wm_changed; /* watermarks are updated */
> > > +	bool fb_changed; /* fb on any of the planes is changed
> > > */
> > >  
> > >  	/* Pipe source size (ie. panel fitter input size)
> > >  	 * All planes will be positioned inside this space,
> > > @@ -547,7 +548,6 @@ struct intel_crtc_atomic_commit {
> > >  
> > >  	/* Sleepable operations to perform after commit */
> > >  	unsigned fb_bits;
> > > -	bool wait_vblank;
> > One of the things that I like about the code without this patch is
> > that
> > it's very explicit on when we need to wait for vblanks (except for
> > the
> > problem where we wait twice). The new code is not as easy to
> > read/understand as the old one. This comment is similar to the one
> > I
> > made in patch 6: I'm not sure if sacrificing readability is worth
> > it.
> > 
> > I wonder that maybe the cleanest fix to the "we're waiting 2
> > vblanks"
> > problem is to just remove the call to
> > drm_atomic_helper_wait_for_vblanks(), which would be a first patch.
> > Then we'd have a second patch introducing
> > intel_atomic_wait_for_vblanks() for the "wait for all vblanks at
> > the
> > same time" optimization, and then a last commit possibly replacing
> > commit->wait_vblank for state->fb_changed. Just an idea, not a
> > request.
> There were cases in which the atomic helper would wait for vblanks
> which
> wouldn't trigger any of the other changes, so it can't be blindly
> removed. Mostly when
> updating a plane with a same sized fb.

Those could be specifically addressed on their own patches, then :)

> Wait for vblank is required for unpinning, it would be bad if the
> current fb is unpinned.
> 
> > I'll wait for your answers before reaching any conclusions of what
> > I
> > think should be done, since I may be misunderstanding something.
> I want to call all post flip work scheduled later on so it happens
> after the next vblank.
> That will remove all confusion on when a vblank is required, because
> all post_plane_update
> and unpin will always wait until next vblank.
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v4 3/8] drm/i915: Kill off intel_crtc->atomic.wait_vblank, v4.

[Maarten Lankhorst]

Op 18-02-16 om 15:14 schreef Zanoni, Paulo R:
> Em Qui, 2016-02-18 às 14:22 +0100, Maarten Lankhorst escreveu:
>> Op 17-02-16 om 22:20 schreef Zanoni, Paulo R:
>>> Em Qua, 2016-02-10 às 13:49 +0100, Maarten Lankhorst escreveu:
>>>> Currently we perform our own wait in post_plane_update,
>>>> but the atomic core performs another one in wait_for_vblanks.
>>>> This means that 2 vblanks are done when a fb is changed,
>>>> which is a bit overkill.
>>>>
>>>> Merge them by creating a helper function that takes a crtc mask
>>>> for the planes to wait on.
>>>>
>>>> The broadwell vblank workaround may look gone entirely but this
>>>> is
>>>> not the case. pipe_config->wm_changed is set to true
>>>> when any plane is turned on, which forces a vblank wait.
>>>>
>>>> Changes since v1:
>>>> - Removing the double vblank wait on broadwell moved to its own
>>>> commit.
>>>> Changes since v2:
>>>> - Move out POWER_DOMAIN_MODESET handling to its own commit.
>>>> Changes since v3:
>>>> - Do not wait for vblank on legacy cursor updates. (Ville)
>>>> - Move broadwell vblank workaround comment to page_flip_finished.
>>>> (Ville)
>>>> Changes since v4:
>>>> - Compile fix, legacy_cursor_flip -> *_update.
>>>>
>>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.c
>>>> om>
>>>> ---
>>>>  drivers/gpu/drm/i915/intel_atomic.c  |  1 +
>>>>  drivers/gpu/drm/i915/intel_display.c | 86
>>>> +++++++++++++++++++++++++++---------
>>>>  drivers/gpu/drm/i915/intel_drv.h     |  2 +-
>>>>  3 files changed, 67 insertions(+), 22 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/intel_atomic.c
>>>> b/drivers/gpu/drm/i915/intel_atomic.c
>>>> index 4625f8a9ba12..8e579a8505ac 100644
>>>> --- a/drivers/gpu/drm/i915/intel_atomic.c
>>>> +++ b/drivers/gpu/drm/i915/intel_atomic.c
>>>> @@ -97,6 +97,7 @@ intel_crtc_duplicate_state(struct drm_crtc
>>>> *crtc)
>>>>  	crtc_state->disable_lp_wm = false;
>>>>  	crtc_state->disable_cxsr = false;
>>>>  	crtc_state->wm_changed = false;
>>>> +	crtc_state->fb_changed = false;
>>>>  
>>>>  	return &crtc_state->base;
>>>>  }
>>>> diff --git a/drivers/gpu/drm/i915/intel_display.c
>>>> b/drivers/gpu/drm/i915/intel_display.c
>>>> index 804f2c6f260d..4d4dddc1f970 100644
>>>> --- a/drivers/gpu/drm/i915/intel_display.c
>>>> +++ b/drivers/gpu/drm/i915/intel_display.c
>>>> @@ -4785,9 +4785,6 @@ static void intel_post_plane_update(struct
>>>> intel_crtc *crtc)
>>>>  		to_intel_crtc_state(crtc->base.state);
>>>>  	struct drm_device *dev = crtc->base.dev;
>>>>  
>>>> -	if (atomic->wait_vblank)
>>>> -		intel_wait_for_vblank(dev, crtc->pipe);
>>>> -
>>>>  	intel_frontbuffer_flip(dev, atomic->fb_bits);
>>>>  
>>>>  	crtc->wm.cxsr_allowed = true;
>>>> @@ -10902,6 +10899,12 @@ static bool page_flip_finished(struct
>>>> intel_crtc *crtc)
>>>>  		return true;
>>>>  
>>>>  	/*
>>>> +	 * BDW signals flip done immediately if the plane
>>>> +	 * is disabled, even if the plane enable is already
>>>> +	 * armed to occur at the next vblank :(
>>>> +	 */
>>> Having this comment here is just... weird. I think it removes a lot
>>> of
>>> the context that was present before.
>>>
>>>> +
>>>> +	/*
>>>>  	 * A DSPSURFLIVE check isn't enough in case the mmio and
>>>> CS
>>>> flips
>>>>  	 * used the same base address. In that case the mmio
>>>> flip
>>>> might
>>>>  	 * have completed, but the CS hasn't even executed the
>>>> flip
>>>> yet.
>>>> @@ -11778,6 +11781,9 @@ int
>>>> intel_plane_atomic_calc_changes(struct
>>>> drm_crtc_state *crtc_state,
>>>>  	if (!was_visible && !visible)
>>>>  		return 0;
>>>>  
>>>> +	if (fb != old_plane_state->base.fb)
>>>> +		pipe_config->fb_changed = true;
>>>> +
>>>>  	turn_off = was_visible && (!visible || mode_changed);
>>>>  	turn_on = visible && (!was_visible || mode_changed);
>>>>  
>>>> @@ -11793,8 +11799,6 @@ int
>>>> intel_plane_atomic_calc_changes(struct
>>>> drm_crtc_state *crtc_state,
>>>>  
>>>>  		/* must disable cxsr around plane enable/disable
>>>> */
>>>>  		if (plane->type != DRM_PLANE_TYPE_CURSOR) {
>>>> -			if (is_crtc_enabled)
>>>> -				intel_crtc->atomic.wait_vblank =
>>>> true;
>>>>  			pipe_config->disable_cxsr = true;
>>>>  		}
>>> We could have killed the brackets here :)
>> Indeed, will do so in next version.
>>>>  	} else if (intel_wm_need_update(plane, plane_state)) {
>>>> @@ -11810,14 +11814,6 @@ int
>>>> intel_plane_atomic_calc_changes(struct
>>>> drm_crtc_state *crtc_state,
>>>>  		intel_crtc->atomic.post_enable_primary =
>>>> turn_on;
>>>>  		intel_crtc->atomic.update_fbc = true;
>>>>  
>>>> -		/*
>>>> -		 * BDW signals flip done immediately if the
>>>> plane
>>>> -		 * is disabled, even if the plane enable is
>>>> already
>>>> -		 * armed to occur at the next vblank :(
>>>> -		 */
>>>> -		if (turn_on && IS_BROADWELL(dev))
>>>> -			intel_crtc->atomic.wait_vblank = true;
>>>> -
>>>>  		break;
>>>>  	case DRM_PLANE_TYPE_CURSOR:
>>>>  		break;
>>>> @@ -11831,12 +11827,10 @@ int
>>>> intel_plane_atomic_calc_changes(struct
>>>> drm_crtc_state *crtc_state,
>>>>  		if (IS_IVYBRIDGE(dev) &&
>>>>  		    needs_scaling(to_intel_plane_state(plane_sta
>>>> te))
>>>> &&
>>>>  		    !needs_scaling(old_plane_state)) {
>>>> -			to_intel_crtc_state(crtc_state)-
>>>>> disable_lp_wm = true;
>>>> -		} else if (turn_off && !mode_changed) {
>>>> -			intel_crtc->atomic.wait_vblank = true;
>>>> +			pipe_config->disable_lp_wm = true;
>>>> +		} else if (turn_off && !mode_changed)
>>>>  			intel_crtc-
>>>>> atomic.update_sprite_watermarks
>>>>> =
>>>>  				1 << i;
>>>> -		}
>>> Weird brackets here. Either kill on both sides of the if statement
>>> (the
>>> preferred way), or none.
>>>
>>> Also, shouldn't we introduce pipe_config->sprite_changed? What's
>>> guaranteeing that we're going to definitely wait for a vblank
>>> during
>>> sprite updates? I like explicit dumb-proof code instead of
>>> implications
>>> such as "we're doing waits during sprite updates because whenever
>>> we
>>> update sprites, a specific unrelated variable that's used for a
>>> different purpose gets set to true, and we check for this
>>> variable".
>> sprite_changed would be same as fb_changed + wm_changed, and
>> update_sprite_watermarks gets removed in this series
>> because nothing uses it.
> Hmmm, right. For some reason, I was interpreting fb_changed as being
> only valid for the primary fb, not for spr/cur. My bad. So fb_changed
> means "if any of pri/cur/spr changed" I guess. Maybe planes_changed
> could be a better name, or fbs_changed (plural), since we're talking
> about more then one possible plane here. Not a requirement, just
> throwing the idea.
planes_changed is already used, it means that any plane_state is being updated for this crtc.
fbs_changed could work though, most other *_changed are plural.
>>>>  
>>>>  		break;
>>>>  	}
>>>> @@ -13370,6 +13364,48 @@ static int
>>>> intel_atomic_prepare_commit(struct drm_device *dev,
>>>>  	return ret;
>>>>  }
>>>>  
>>>> +static void intel_atomic_wait_for_vblanks(struct drm_device
>>>> *dev,
>>>> +					  struct
>>>> drm_i915_private
>>>> *dev_priv,
>>>> +					  unsigned crtc_mask)
>>>> +{
>>>> +	unsigned last_vblank_count[I915_MAX_PIPES];
>>>> +	enum pipe pipe;
>>>> +	int ret;
>>>> +
>>>> +	if (!crtc_mask)
>>>> +		return;
>>>> +
>>>> +	for_each_pipe(dev_priv, pipe) {
>>>> +		struct drm_crtc *crtc = dev_priv-
>>>>> pipe_to_crtc_mapping[pipe];
>>>> +
>>>> +		if (!((1 << pipe) & crtc_mask))
>>>> +			continue;
>>>> +
>>>> +		ret = drm_crtc_vblank_get(crtc);
>>>> +		if (ret != 0) {
>>>> +			crtc_mask &= ~(1 << pipe);
>>>> +			continue;
>>> Shouldn't we DRM_ERROR here?
>> WARN_ON is enough, this shouldn't ever happen.
> Even better :)
>
>>>> +		}
>>>> +
>>>> +		last_vblank_count[pipe] =
>>>> drm_crtc_vblank_count(crtc);
>>>> +	}
>>>> +
>>>> +	for_each_pipe(dev_priv, pipe) {
>>>> +		struct drm_crtc *crtc = dev_priv-
>>>>> pipe_to_crtc_mapping[pipe];
>>>> +
>>>> +		if (!((1 << pipe) & crtc_mask))
>>>> +			continue;
>>>> +
>>>> +		wait_event_timeout(dev->vblank[pipe].queue,
>>>> +				last_vblank_count[pipe] !=
>>>> +					drm_crtc_vblank_count(cr
>>>> tc),
>>>> +				msecs_to_jiffies(50));
>>> Also maybe DRM_ERROR in case we hit the timeout?
>>>
>>> I know the original code doesn't have this, but now that we're
>>> doing or
>>> own thing, we may scream in unexpected cases.
>> I'll make it a WARN_ON, shouldn't happen.
>>>> +
>>>> +		drm_crtc_vblank_put(crtc);
>>>> +	}
>>>> +}
>>>> +
>>>> +
>>> Two newlines :)
>> Indeed!
>>>>  /**
>>>>   * intel_atomic_commit - commit validated state object
>>>>   * @dev: DRM device
>>>> @@ -13397,6 +13433,7 @@ static int intel_atomic_commit(struct
>>>> drm_device *dev,
>>>>  	int ret = 0, i;
>>>>  	bool hw_check = intel_state->modeset;
>>>>  	unsigned long put_domains[I915_MAX_PIPES] = {};
>>>> +	unsigned crtc_vblank_mask = 0;
>>>>  
>>>>  	ret = intel_atomic_prepare_commit(dev, state, async);
>>>>  	if (ret) {
>>>> @@ -13470,8 +13507,9 @@ static int intel_atomic_commit(struct
>>>> drm_device *dev,
>>>>  	for_each_crtc_in_state(state, crtc, crtc_state, i) {
>>>>  		struct intel_crtc *intel_crtc =
>>>> to_intel_crtc(crtc);
>>>>  		bool modeset = needs_modeset(crtc->state);
>>>> -		bool update_pipe = !modeset &&
>>>> -			to_intel_crtc_state(crtc->state)-
>>>>> update_pipe;
>>>> +		struct intel_crtc_state *pipe_config =
>>>> +			to_intel_crtc_state(crtc->state);
>>>> +		bool update_pipe = !modeset && pipe_config-
>>>>> update_pipe;
>>>>  
>>>>  		if (modeset && crtc->state->active) {
>>>>  			update_scanline_offset(to_intel_crtc(crt
>>>> c));
>>>> @@ -13488,14 +13526,20 @@ static int intel_atomic_commit(struct
>>>> drm_device *dev,
>>>>  		    (crtc->state->planes_changed ||
>>>> update_pipe))
>>>>  			drm_atomic_helper_commit_planes_on_crtc(
>>>> crtc
>>>> _state);
>>>>  
>>>> -		intel_post_plane_update(intel_crtc);
>>>> +		if (pipe_config->base.active &&
>>>> +		    (pipe_config->wm_changed || pipe_config-
>>>>> disable_cxsr ||
>>>> +		     pipe_config->fb_changed))
>>> So the wm_changed is just for the BDW workaround + sprites? What
>>> about
>>> this disable_cxsr, why is it here? Also see my comment above about
>>> sprite_changed. Maybe we need some comments here to explain the
>>> complex
>>> checks.
>> No it's waiting for a vblank for post_plane_update so all optimized
>> wm values
>> can get written, it's not just for the BDW workaround.
>> It just happens to be that the BDW w/a gets applied too as a side
>> effect.
> So maybe some comment regarding the BDW WA could be here.
>
> What about disable_cxsr? Why is this here?
That's what matches the current code, see the calc_changes hunk which had a vblank_wait = true.
>>>> +			crtc_vblank_mask |= 1 << i;
>>>>  	}
>>>>  
>>>>  	/* FIXME: add subpixel order */
>>>>  
>>>> -	drm_atomic_helper_wait_for_vblanks(dev, state);
>>>> +	if (!state->legacy_cursor_update)
>>>> +		intel_atomic_wait_for_vblanks(dev, dev_priv,
>>>> crtc_vblank_mask);
>>>>  
>>>>  	for_each_crtc_in_state(state, crtc, crtc_state, i) {
>>>> +		intel_post_plane_update(to_intel_crtc(crtc));
>>>> +
>>>>  		if (put_domains[i])
>>>>  			modeset_put_power_domains(dev_priv,
>>>> put_domains[i]);
>>>>  	}
>>>> diff --git a/drivers/gpu/drm/i915/intel_drv.h
>>>> b/drivers/gpu/drm/i915/intel_drv.h
>>>> index 335e6b24b0bc..e911c86f873b 100644
>>>> --- a/drivers/gpu/drm/i915/intel_drv.h
>>>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>>>> @@ -379,6 +379,7 @@ struct intel_crtc_state {
>>>>  	bool update_pipe; /* can a fast modeset be performed? */
>>>>  	bool disable_cxsr;
>>>>  	bool wm_changed; /* watermarks are updated */
>>>> +	bool fb_changed; /* fb on any of the planes is changed
>>>> */
>>>>  
>>>>  	/* Pipe source size (ie. panel fitter input size)
>>>>  	 * All planes will be positioned inside this space,
>>>> @@ -547,7 +548,6 @@ struct intel_crtc_atomic_commit {
>>>>  
>>>>  	/* Sleepable operations to perform after commit */
>>>>  	unsigned fb_bits;
>>>> -	bool wait_vblank;
>>> One of the things that I like about the code without this patch is
>>> that
>>> it's very explicit on when we need to wait for vblanks (except for
>>> the
>>> problem where we wait twice). The new code is not as easy to
>>> read/understand as the old one. This comment is similar to the one
>>> I
>>> made in patch 6: I'm not sure if sacrificing readability is worth
>>> it.
>>>
>>> I wonder that maybe the cleanest fix to the "we're waiting 2
>>> vblanks"
>>> problem is to just remove the call to
>>> drm_atomic_helper_wait_for_vblanks(), which would be a first patch.
>>> Then we'd have a second patch introducing
>>> intel_atomic_wait_for_vblanks() for the "wait for all vblanks at
>>> the
>>> same time" optimization, and then a last commit possibly replacing
>>> commit->wait_vblank for state->fb_changed. Just an idea, not a
>>> request.
>> There were cases in which the atomic helper would wait for vblanks
>> which
>> wouldn't trigger any of the other changes, so it can't be blindly
>> removed. Mostly when
>> updating a plane with a same sized fb.
> Those could be specifically addressed on their own patches, then :)
It would break things though.

I think what might make the most sense is adding a inline function for needs_vblank,
with comments why certain flags require a vblank wait.
>> Wait for vblank is required for unpinning, it would be bad if the
>> current fb is unpinned.
>>
>>> I'll wait for your answers before reaching any conclusions of what
>>> I
>>> think should be done, since I may be misunderstanding something.
>> I want to call all post flip work scheduled later on so it happens
>> after the next vblank.
>> That will remove all confusion on when a vblank is required, because
>> all post_plane_update
>> and unpin will always wait until next vblank.
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH v4 3/8] drm/i915: Kill off intel_crtc->atomic.wait_vblank, v4.

[Zanoni, Paulo R]

Em Qui, 2016-02-18 às 15:46 +0100, Maarten Lankhorst escreveu:
> Op 18-02-16 om 15:14 schreef Zanoni, Paulo R:
> > Em Qui, 2016-02-18 às 14:22 +0100, Maarten Lankhorst escreveu:
> > > Op 17-02-16 om 22:20 schreef Zanoni, Paulo R:
> > > > Em Qua, 2016-02-10 às 13:49 +0100, Maarten Lankhorst escreveu:
> > > > > Currently we perform our own wait in post_plane_update,
> > > > > but the atomic core performs another one in wait_for_vblanks.
> > > > > This means that 2 vblanks are done when a fb is changed,
> > > > > which is a bit overkill.
> > > > > 
> > > > > Merge them by creating a helper function that takes a crtc
> > > > > mask
> > > > > for the planes to wait on.
> > > > > 
> > > > > The broadwell vblank workaround may look gone entirely but
> > > > > this
> > > > > is
> > > > > not the case. pipe_config->wm_changed is set to true
> > > > > when any plane is turned on, which forces a vblank wait.
> > > > > 
> > > > > Changes since v1:
> > > > > - Removing the double vblank wait on broadwell moved to its
> > > > > own
> > > > > commit.
> > > > > Changes since v2:
> > > > > - Move out POWER_DOMAIN_MODESET handling to its own commit.
> > > > > Changes since v3:
> > > > > - Do not wait for vblank on legacy cursor updates. (Ville)
> > > > > - Move broadwell vblank workaround comment to
> > > > > page_flip_finished.
> > > > > (Ville)
> > > > > Changes since v4:
> > > > > - Compile fix, legacy_cursor_flip -> *_update.
> > > > > 
> > > > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.int
> > > > > el.c
> > > > > om>
> > > > > ---
> > > > >  drivers/gpu/drm/i915/intel_atomic.c  |  1 +
> > > > >  drivers/gpu/drm/i915/intel_display.c | 86
> > > > > +++++++++++++++++++++++++++---------
> > > > >  drivers/gpu/drm/i915/intel_drv.h     |  2 +-
> > > > >  3 files changed, 67 insertions(+), 22 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/intel_atomic.c
> > > > > b/drivers/gpu/drm/i915/intel_atomic.c
> > > > > index 4625f8a9ba12..8e579a8505ac 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_atomic.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_atomic.c
> > > > > @@ -97,6 +97,7 @@ intel_crtc_duplicate_state(struct drm_crtc
> > > > > *crtc)
> > > > >  	crtc_state->disable_lp_wm = false;
> > > > >  	crtc_state->disable_cxsr = false;
> > > > >  	crtc_state->wm_changed = false;
> > > > > +	crtc_state->fb_changed = false;
> > > > >  
> > > > >  	return &crtc_state->base;
> > > > >  }
> > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > > > > b/drivers/gpu/drm/i915/intel_display.c
> > > > > index 804f2c6f260d..4d4dddc1f970 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > > > @@ -4785,9 +4785,6 @@ static void
> > > > > intel_post_plane_update(struct
> > > > > intel_crtc *crtc)
> > > > >  		to_intel_crtc_state(crtc->base.state);
> > > > >  	struct drm_device *dev = crtc->base.dev;
> > > > >  
> > > > > -	if (atomic->wait_vblank)
> > > > > -		intel_wait_for_vblank(dev, crtc->pipe);
> > > > > -
> > > > >  	intel_frontbuffer_flip(dev, atomic->fb_bits);
> > > > >  
> > > > >  	crtc->wm.cxsr_allowed = true;
> > > > > @@ -10902,6 +10899,12 @@ static bool
> > > > > page_flip_finished(struct
> > > > > intel_crtc *crtc)
> > > > >  		return true;
> > > > >  
> > > > >  	/*
> > > > > +	 * BDW signals flip done immediately if the plane
> > > > > +	 * is disabled, even if the plane enable is already
> > > > > +	 * armed to occur at the next vblank :(
> > > > > +	 */
> > > > Having this comment here is just... weird. I think it removes a
> > > > lot
> > > > of
> > > > the context that was present before.
> > > > 
> > > > > +
> > > > > +	/*
> > > > >  	 * A DSPSURFLIVE check isn't enough in case the mmio
> > > > > and
> > > > > CS
> > > > > flips
> > > > >  	 * used the same base address. In that case the mmio
> > > > > flip
> > > > > might
> > > > >  	 * have completed, but the CS hasn't even executed
> > > > > the
> > > > > flip
> > > > > yet.
> > > > > @@ -11778,6 +11781,9 @@ int
> > > > > intel_plane_atomic_calc_changes(struct
> > > > > drm_crtc_state *crtc_state,
> > > > >  	if (!was_visible && !visible)
> > > > >  		return 0;
> > > > >  
> > > > > +	if (fb != old_plane_state->base.fb)
> > > > > +		pipe_config->fb_changed = true;
> > > > > +
> > > > >  	turn_off = was_visible && (!visible ||
> > > > > mode_changed);
> > > > >  	turn_on = visible && (!was_visible || mode_changed);
> > > > >  
> > > > > @@ -11793,8 +11799,6 @@ int
> > > > > intel_plane_atomic_calc_changes(struct
> > > > > drm_crtc_state *crtc_state,
> > > > >  
> > > > >  		/* must disable cxsr around plane
> > > > > enable/disable
> > > > > */
> > > > >  		if (plane->type != DRM_PLANE_TYPE_CURSOR) {
> > > > > -			if (is_crtc_enabled)
> > > > > -				intel_crtc-
> > > > > >atomic.wait_vblank =
> > > > > true;
> > > > >  			pipe_config->disable_cxsr = true;
> > > > >  		}
> > > > We could have killed the brackets here :)
> > > Indeed, will do so in next version.
> > > > >  	} else if (intel_wm_need_update(plane, plane_state))
> > > > > {
> > > > > @@ -11810,14 +11814,6 @@ int
> > > > > intel_plane_atomic_calc_changes(struct
> > > > > drm_crtc_state *crtc_state,
> > > > >  		intel_crtc->atomic.post_enable_primary =
> > > > > turn_on;
> > > > >  		intel_crtc->atomic.update_fbc = true;
> > > > >  
> > > > > -		/*
> > > > > -		 * BDW signals flip done immediately if the
> > > > > plane
> > > > > -		 * is disabled, even if the plane enable is
> > > > > already
> > > > > -		 * armed to occur at the next vblank :(
> > > > > -		 */
> > > > > -		if (turn_on && IS_BROADWELL(dev))
> > > > > -			intel_crtc->atomic.wait_vblank =
> > > > > true;
> > > > > -
> > > > >  		break;
> > > > >  	case DRM_PLANE_TYPE_CURSOR:
> > > > >  		break;
> > > > > @@ -11831,12 +11827,10 @@ int
> > > > > intel_plane_atomic_calc_changes(struct
> > > > > drm_crtc_state *crtc_state,
> > > > >  		if (IS_IVYBRIDGE(dev) &&
> > > > >  		    needs_scaling(to_intel_plane_state(plane
> > > > > _sta
> > > > > te))
> > > > > &&
> > > > >  		    !needs_scaling(old_plane_state)) {
> > > > > -			to_intel_crtc_state(crtc_state)-
> > > > > > disable_lp_wm = true;
> > > > > -		} else if (turn_off && !mode_changed) {
> > > > > -			intel_crtc->atomic.wait_vblank =
> > > > > true;
> > > > > +			pipe_config->disable_lp_wm = true;
> > > > > +		} else if (turn_off && !mode_changed)
> > > > >  			intel_crtc-
> > > > > > atomic.update_sprite_watermarks
> > > > > > =
> > > > >  				1 << i;
> > > > > -		}
> > > > Weird brackets here. Either kill on both sides of the if
> > > > statement
> > > > (the
> > > > preferred way), or none.

I noticed that you fix the brackets on patch 4/8 by removing the "else"
part. So feel free to keep this chunk like this, so you won't need to
resend patch 4.


> > > > 
> > > > Also, shouldn't we introduce pipe_config->sprite_changed?
> > > > What's
> > > > guaranteeing that we're going to definitely wait for a vblank
> > > > during
> > > > sprite updates? I like explicit dumb-proof code instead of
> > > > implications
> > > > such as "we're doing waits during sprite updates because
> > > > whenever
> > > > we
> > > > update sprites, a specific unrelated variable that's used for a
> > > > different purpose gets set to true, and we check for this
> > > > variable".
> > > sprite_changed would be same as fb_changed + wm_changed, and
> > > update_sprite_watermarks gets removed in this series
> > > because nothing uses it.
> > Hmmm, right. For some reason, I was interpreting fb_changed as
> > being
> > only valid for the primary fb, not for spr/cur. My bad. So
> > fb_changed
> > means "if any of pri/cur/spr changed" I guess. Maybe planes_changed
> > could be a better name, or fbs_changed (plural), since we're
> > talking
> > about more then one possible plane here. Not a requirement, just
> > throwing the idea.
> planes_changed is already used, it means that any plane_state is
> being updated for this crtc.
> fbs_changed could work though, most other *_changed are plural.
> > > > >  
> > > > >  		break;
> > > > >  	}
> > > > > @@ -13370,6 +13364,48 @@ static int
> > > > > intel_atomic_prepare_commit(struct drm_device *dev,
> > > > >  	return ret;
> > > > >  }
> > > > >  
> > > > > +static void intel_atomic_wait_for_vblanks(struct drm_device
> > > > > *dev,
> > > > > +					  struct
> > > > > drm_i915_private
> > > > > *dev_priv,
> > > > > +					  unsigned
> > > > > crtc_mask)
> > > > > +{
> > > > > +	unsigned last_vblank_count[I915_MAX_PIPES];
> > > > > +	enum pipe pipe;
> > > > > +	int ret;
> > > > > +
> > > > > +	if (!crtc_mask)
> > > > > +		return;
> > > > > +
> > > > > +	for_each_pipe(dev_priv, pipe) {
> > > > > +		struct drm_crtc *crtc = dev_priv-
> > > > > > pipe_to_crtc_mapping[pipe];
> > > > > +
> > > > > +		if (!((1 << pipe) & crtc_mask))
> > > > > +			continue;
> > > > > +
> > > > > +		ret = drm_crtc_vblank_get(crtc);
> > > > > +		if (ret != 0) {
> > > > > +			crtc_mask &= ~(1 << pipe);
> > > > > +			continue;
> > > > Shouldn't we DRM_ERROR here?
> > > WARN_ON is enough, this shouldn't ever happen.
> > Even better :)
> > 
> > > > > +		}
> > > > > +
> > > > > +		last_vblank_count[pipe] =
> > > > > drm_crtc_vblank_count(crtc);
> > > > > +	}
> > > > > +
> > > > > +	for_each_pipe(dev_priv, pipe) {
> > > > > +		struct drm_crtc *crtc = dev_priv-
> > > > > > pipe_to_crtc_mapping[pipe];
> > > > > +
> > > > > +		if (!((1 << pipe) & crtc_mask))
> > > > > +			continue;
> > > > > +
> > > > > +		wait_event_timeout(dev->vblank[pipe].queue,
> > > > > +				last_vblank_count[pipe] !=
> > > > > +					drm_crtc_vblank_coun
> > > > > t(cr
> > > > > tc),
> > > > > +				msecs_to_jiffies(50));
> > > > Also maybe DRM_ERROR in case we hit the timeout?
> > > > 
> > > > I know the original code doesn't have this, but now that we're
> > > > doing or
> > > > own thing, we may scream in unexpected cases.
> > > I'll make it a WARN_ON, shouldn't happen.
> > > > > +
> > > > > +		drm_crtc_vblank_put(crtc);
> > > > > +	}
> > > > > +}
> > > > > +
> > > > > +
> > > > Two newlines :)
> > > Indeed!
> > > > >  /**
> > > > >   * intel_atomic_commit - commit validated state object
> > > > >   * @dev: DRM device
> > > > > @@ -13397,6 +13433,7 @@ static int intel_atomic_commit(struct
> > > > > drm_device *dev,
> > > > >  	int ret = 0, i;
> > > > >  	bool hw_check = intel_state->modeset;
> > > > >  	unsigned long put_domains[I915_MAX_PIPES] = {};
> > > > > +	unsigned crtc_vblank_mask = 0;
> > > > >  
> > > > >  	ret = intel_atomic_prepare_commit(dev, state,
> > > > > async);
> > > > >  	if (ret) {
> > > > > @@ -13470,8 +13507,9 @@ static int intel_atomic_commit(struct
> > > > > drm_device *dev,
> > > > >  	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> > > > >  		struct intel_crtc *intel_crtc =
> > > > > to_intel_crtc(crtc);
> > > > >  		bool modeset = needs_modeset(crtc->state);
> > > > > -		bool update_pipe = !modeset &&
> > > > > -			to_intel_crtc_state(crtc->state)-
> > > > > > update_pipe;
> > > > > +		struct intel_crtc_state *pipe_config =
> > > > > +			to_intel_crtc_state(crtc->state);
> > > > > +		bool update_pipe = !modeset && pipe_config-
> > > > > > update_pipe;
> > > > >  
> > > > >  		if (modeset && crtc->state->active) {
> > > > >  			update_scanline_offset(to_intel_crtc
> > > > > (crt
> > > > > c));
> > > > > @@ -13488,14 +13526,20 @@ static int
> > > > > intel_atomic_commit(struct
> > > > > drm_device *dev,
> > > > >  		    (crtc->state->planes_changed ||
> > > > > update_pipe))
> > > > >  			drm_atomic_helper_commit_planes_on_c
> > > > > rtc(
> > > > > crtc
> > > > > _state);
> > > > >  
> > > > > -		intel_post_plane_update(intel_crtc);
> > > > > +		if (pipe_config->base.active &&
> > > > > +		    (pipe_config->wm_changed || pipe_config-
> > > > > > disable_cxsr ||
> > > > > +		     pipe_config->fb_changed))
> > > > So the wm_changed is just for the BDW workaround + sprites?
> > > > What
> > > > about
> > > > this disable_cxsr, why is it here? Also see my comment above
> > > > about
> > > > sprite_changed. Maybe we need some comments here to explain the
> > > > complex
> > > > checks.
> > > No it's waiting for a vblank for post_plane_update so all
> > > optimized
> > > wm values
> > > can get written, it's not just for the BDW workaround.
> > > It just happens to be that the BDW w/a gets applied too as a side
> > > effect.
> > So maybe some comment regarding the BDW WA could be here.
> > 
> > What about disable_cxsr? Why is this here?
> That's what matches the current code, see the calc_changes hunk which
> had a vblank_wait = true.

Hmm, right. I missed that, sorry.

> > > > > +			crtc_vblank_mask |= 1 << i;
> > > > >  	}
> > > > >  
> > > > >  	/* FIXME: add subpixel order */
> > > > >  
> > > > > -	drm_atomic_helper_wait_for_vblanks(dev, state);
> > > > > +	if (!state->legacy_cursor_update)
> > > > > +		intel_atomic_wait_for_vblanks(dev, dev_priv,
> > > > > crtc_vblank_mask);
> > > > >  
> > > > >  	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> > > > > +		intel_post_plane_update(to_intel_crtc(crtc))
> > > > > ;
> > > > > +
> > > > >  		if (put_domains[i])
> > > > >  			modeset_put_power_domains(dev_priv,
> > > > > put_domains[i]);
> > > > >  	}
> > > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > > > > b/drivers/gpu/drm/i915/intel_drv.h
> > > > > index 335e6b24b0bc..e911c86f873b 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > > > @@ -379,6 +379,7 @@ struct intel_crtc_state {
> > > > >  	bool update_pipe; /* can a fast modeset be
> > > > > performed? */
> > > > >  	bool disable_cxsr;
> > > > >  	bool wm_changed; /* watermarks are updated */
> > > > > +	bool fb_changed; /* fb on any of the planes is
> > > > > changed
> > > > > */
> > > > >  
> > > > >  	/* Pipe source size (ie. panel fitter input size)
> > > > >  	 * All planes will be positioned inside this space,
> > > > > @@ -547,7 +548,6 @@ struct intel_crtc_atomic_commit {
> > > > >  
> > > > >  	/* Sleepable operations to perform after commit */
> > > > >  	unsigned fb_bits;
> > > > > -	bool wait_vblank;
> > > > One of the things that I like about the code without this patch
> > > > is
> > > > that
> > > > it's very explicit on when we need to wait for vblanks (except
> > > > for
> > > > the
> > > > problem where we wait twice). The new code is not as easy to
> > > > read/understand as the old one. This comment is similar to the
> > > > one
> > > > I
> > > > made in patch 6: I'm not sure if sacrificing readability is
> > > > worth
> > > > it.
> > > > 
> > > > I wonder that maybe the cleanest fix to the "we're waiting 2
> > > > vblanks"
> > > > problem is to just remove the call to
> > > > drm_atomic_helper_wait_for_vblanks(), which would be a first
> > > > patch.
> > > > Then we'd have a second patch introducing
> > > > intel_atomic_wait_for_vblanks() for the "wait for all vblanks
> > > > at
> > > > the
> > > > same time" optimization, and then a last commit possibly
> > > > replacing
> > > > commit->wait_vblank for state->fb_changed. Just an idea, not a
> > > > request.
> > > There were cases in which the atomic helper would wait for
> > > vblanks
> > > which
> > > wouldn't trigger any of the other changes, so it can't be blindly
> > > removed. Mostly when
> > > updating a plane with a same sized fb.
> > Those could be specifically addressed on their own patches, then :)
> It would break things though.

Ok, let's discard the idea then.

> 
> I think what might make the most sense is adding a inline function
> for needs_vblank,
> with comments why certain flags require a vblank wait.

That could be good.

I have no further questions/requests for this patch. In the end, it
seems the changes needed are small :). I'll wait for the next version.

> > > Wait for vblank is required for unpinning, it would be bad if the
> > > current fb is unpinned.
> > > 
> > > > I'll wait for your answers before reaching any conclusions of
> > > > what
> > > > I
> > > > think should be done, since I may be misunderstanding
> > > > something.
> > > I want to call all post flip work scheduled later on so it
> > > happens
> > > after the next vblank.
> > > That will remove all confusion on when a vblank is required,
> > > because
> > > all post_plane_update
> > > and unpin will always wait until next vblank.
> > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v4 6/8] drm/i915: Nuke fbc members from intel_crtc->atomic, v2.

[Zanoni, Paulo R]

Em Seg, 2016-02-15 às 15:31 +0100, Maarten Lankhorst escreveu:
> Op 12-02-16 om 14:56 schreef Zanoni, Paulo R:
> > Em Qua, 2016-02-10 às 13:49 +0100, Maarten Lankhorst escreveu:
> > > Factor out intel_fbc_supports_rotation
> > ^ not anymore
> > 
> > 
> > >  and use it in
> > > pre_plane_update as well. This leaves intel_crtc->atomic
> > > empty, so remove it too.
> > > 
> > > Changes since v1:
> > > - Add a intel_fbc_supports_rotation helper.
> > Changes since v2:
> > - No more need for rotation special-casing.
> > 
> > (I suppose you also have to edit the commit title to be v3)
> > 
> > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.c
> > > om>
> > > ---
> > >  drivers/gpu/drm/i915/intel_display.c | 58 +++++++++++++---------
> > > ----
> > > ----------
> > >  drivers/gpu/drm/i915/intel_drv.h     | 15 ----------
> > >  2 files changed, 20 insertions(+), 53 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > > b/drivers/gpu/drm/i915/intel_display.c
> > > index 54be8a255f1f..00cb261c6787 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -4782,11 +4782,9 @@ 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_atomic_state *old_state = old_crtc_state-
> > > > base.state;
> > > -	struct intel_crtc_atomic_commit *atomic = &crtc->atomic;
> > >  	struct intel_crtc_state *pipe_config =
> > >  		to_intel_crtc_state(crtc->base.state);
> > >  	struct drm_device *dev = crtc->base.dev;
> > > -	struct drm_i915_private *dev_priv = dev->dev_private;
> > >  	struct drm_plane *primary = crtc->base.primary;
> > >  	struct drm_plane_state *old_pri_state =
> > >  		drm_atomic_get_existing_plane_state(old_state,
> > > primary);
> > > @@ -4798,22 +4796,19 @@ static void
> > > intel_post_plane_update(struct
> > > intel_crtc_state *old_crtc_state)
> > >  	if (pipe_config->wm_changed && pipe_config->base.active)
> > >  		intel_update_watermarks(&crtc->base);
> > >  
> > > -	if (atomic->update_fbc)
> > > -		intel_fbc_post_update(crtc);
> > > -
> > >  	if (old_pri_state) {
> > For a code reader that is not very familiar with all the atomic
> > history
> > and its details, it's not trivial to conclude that "if
> > (drm_atomic_get_existing_plane_state(primary))", then we're
> > updating
> > the primary plane on this atomic commit. And before this patch,
> > it's
> > much much easier to conclude that update_fbc will be true when an
> > atomic update is touching the primary plane because that's
> > explicitly
> > stated by the cod.
> > 
> > So although "let's kill this redundant struct" sounds good, it
> > seems to
> > me that code clarity is going away with this patch, so I wonder if
> > the
> > benefits of the patch outweigh the downsides. Isn't there something
> > else we could do about this, such as some renaming, or adding some
> > function aliases or just extra commenting?
> > 
> > >  		struct intel_plane_state *primary_state =
> > >  			to_intel_plane_state(primary->state);
> > >  		struct intel_plane_state *old_primary_state =
> > >  			to_intel_plane_state(old_pri_state);
> > >  
> > > +		intel_fbc_post_update(crtc);
> > > +
> > >  		if (primary_state->visible &&
> > >  		    (needs_modeset(&pipe_config->base) ||
> > >  		     !old_primary_state->visible))
> > >  			intel_post_enable_primary(&crtc->base);
> > >  	}
> > > -
> > > -	memset(atomic, 0, sizeof(*atomic));
> > >  }
> > >  
> > >  static void intel_pre_plane_update(struct intel_crtc_state
> > > *old_crtc_state)
> > > @@ -4821,7 +4816,6 @@ static void intel_pre_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 = dev->dev_private;
> > > -	struct intel_crtc_atomic_commit *atomic = &crtc->atomic;
> > >  	struct intel_crtc_state *pipe_config =
> > >  		to_intel_crtc_state(crtc->base.state);
> > >  	struct drm_atomic_state *old_state = old_crtc_state-
> > > > base.state;
> > > @@ -4830,17 +4824,17 @@ static void intel_pre_plane_update(struct
> > > intel_crtc_state *old_crtc_state)
> > >  		drm_atomic_get_existing_plane_state(old_state,
> > > primary);
> > >  	bool modeset = needs_modeset(&pipe_config->base);
> > >  
> > > -	if (atomic->update_fbc)
> > > -		intel_fbc_pre_update(crtc);
> > > -
> > >  	if (old_pri_state) {
> > >  		struct intel_plane_state *primary_state =
> > >  			to_intel_plane_state(primary->state);
> > >  		struct intel_plane_state *old_primary_state =
> > >  			to_intel_plane_state(old_pri_state);
> > > +		bool turn_off = old_primary_state->visible &&
> > > +		    (modeset || !primary_state->visible); 
> > Not really related to the patch, but ok to keep since it's
> > trivial...
> > 
> > > +
> > > +		intel_fbc_pre_update(crtc);
> > >  
> > > -		if (old_primary_state->visible &&
> > > -		    (modeset || !primary_state->visible))
> > > +		if (turn_off)
> > >  			intel_pre_disable_primary(&crtc->base);
> > >  	}
> > >  
> > > @@ -11822,27 +11816,17 @@ int
> > > intel_plane_atomic_calc_changes(struct
> > > drm_crtc_state *crtc_state,
> > >  	if (visible || was_visible)
> > >  		pipe_config->fb_bits |= to_intel_plane(plane)-
> > > > frontbuffer_bit;
> > >  
> > > -	switch (plane->type) {
> > > -	case DRM_PLANE_TYPE_PRIMARY:
> > > -		intel_crtc->atomic.update_fbc = true;
> > > -
> > > -		break;
> > > -	case DRM_PLANE_TYPE_CURSOR:
> > > -		break;
> > > -	case DRM_PLANE_TYPE_OVERLAY:
> > > -		/*
> > > -		 * WaCxSRDisabledForSpriteScaling:ivb
> > > -		 *
> > > -		 * cstate->update_wm was already set above, so
> > > this
> > > flag will
> > > -		 * take effect when we commit and program
> > > watermarks.
> > > -		 */
> > > -		if (IS_IVYBRIDGE(dev) &&
> > > -		    needs_scaling(to_intel_plane_state(plane_sta
> > > te))
> > > &&
> > > -		    !needs_scaling(old_plane_state))
> > > -			pipe_config->disable_lp_wm = true;
> > > +	/*
> > > +	 * WaCxSRDisabledForSpriteScaling:ivb
> > > +	 *
> > > +	 * cstate->update_wm was already set above, so this flag
> > > will
> > > +	 * take effect when we commit and program watermarks.
> > > +	 */
> > > +	if (plane->type == DRM_PLANE_TYPE_OVERLAY &&
> > > IS_IVYBRIDGE(dev) &&
> > > +	    needs_scaling(to_intel_plane_state(plane_state)) &&
> > > +	    !needs_scaling(old_plane_state))
> > > +		pipe_config->disable_lp_wm = true;
> > >  
> > > -		break;
> > > -	}
> > >  	return 0;
> > >  }
> > >  
> > > @@ -13241,9 +13225,6 @@ static int intel_atomic_check(struct
> > > drm_device *dev,
> > >  		struct intel_crtc_state *pipe_config =
> > >  			to_intel_crtc_state(crtc_state);
> > >  
> > > -		memset(&to_intel_crtc(crtc)->atomic, 0,
> > > -		       sizeof(struct intel_crtc_atomic_commit));
> > > -
> > >  		/* Catch I915_MODE_FLAG_INHERITED */
> > >  		if (crtc_state->mode.private_flags != crtc-
> > > >state-
> > > > mode.private_flags)
> > >  			crtc_state->mode_changed = true;
> > > @@ -13528,12 +13509,13 @@ static int intel_atomic_commit(struct
> > > drm_device *dev,
> > >  		if (!modeset)
> > >  			intel_pre_plane_update(to_intel_crtc_sta
> > > te(c
> > > rtc_state));
> > >  
> > > -		if (crtc->state->active && intel_crtc-
> > > > atomic.update_fbc)
> > > +		if ((modeset || update_pipe) && crtc->state-
> > > >active)
> > Same here: not easy for me to verify things are the same now. Even
> > worse: the check for "is this atomic commit touching the primary
> > plane?" is now written in a completely different way than the one
> > introduced above. Maybe there's something we could do to make the
> > code
> > easier to read.
> No, this is called now every time when the crtc has a modeset or
> update_pipe, even with no primary plane.
> This is because atomic may set a mode without having a primary plane,
> and in that case fbc_enable may never be called.
> Seemed like a bug in the original code..

I suppose that proves my point that this code with all those similar-
but-different boolean checks is really hard to read :)

FBC can only happen when there's a primary plane, so AFAIU even if we
don't call intel_fbc_enable here because the primary is disable, we'll
end up calling it later when we enable the primary plane.

Anyway, a change in behavior like this (which was supposed to be a bug
fix) should be on its own separate patch, not hidden inside a refactor.

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

Re: [PATCH v4 7/8] drm/i915: Do not compute watermarks on a noop.

[Zanoni, Paulo R]

Em Qua, 2016-02-10 às 13:49 +0100, Maarten Lankhorst escreveu:
> No atomic state should be included after all validation when nothing
> has
> changed. During modeset all active planes will be added to the state,
> while disabled planes won't change their state.

As someone who is also not super familiar with the new watermarks code,
I really really wish I had a more detailed commit message to allow me
to understand your train of thought. I'll ask some questions below to
validate my understanding.

> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Matt Roper <matthew.d.roper@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c |  3 +-
>  drivers/gpu/drm/i915/intel_drv.h     | 13 ++++++++
>  drivers/gpu/drm/i915/intel_pm.c      | 61 +++++++++++++++++++++-----
> ----------
>  3 files changed, 51 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index 00cb261c6787..6bb1f5dbc7a0 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11910,7 +11910,8 @@ static int intel_crtc_atomic_check(struct
> drm_crtc *crtc,
>  	}
>  
>  	ret = 0;
> -	if (dev_priv->display.compute_pipe_wm) {
> +	if (dev_priv->display.compute_pipe_wm &&
> +	    (mode_changed || pipe_config->update_pipe || crtc_state-
> >planes_changed)) {
>  		ret = dev_priv->display.compute_pipe_wm(intel_crtc,
> state);
>  		if (ret)
>  			return ret;

Can't this chunk be on its own separate commit? I'm not sure why the
rest of the commit is related to this change.

It seems the rest of the commit is aimed reducing the number of planes
we have to lock, not about not computing WMs if nothing in the pipe has
changed.

> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index 8effb9ece21e..144597ac74e3 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1583,6 +1583,19 @@ intel_atomic_get_crtc_state(struct
> drm_atomic_state *state,
>  
>  	return to_intel_crtc_state(crtc_state);
>  }
> +
> +static inline struct intel_plane_state *
> +intel_atomic_get_existing_plane_state(struct drm_atomic_state
> *state,
> +				      struct intel_plane *plane)
> +{
> +	struct drm_plane_state *plane_state;
> +
> +	plane_state = drm_atomic_get_existing_plane_state(state,
> &plane->base);
> +
> +	return to_intel_plane_state(plane_state);
> +}
> +
> +

Two newlines above.

It seems you'll be able to simplify a lot of stuff with this new
function. I'm looking forward to review that patch :)


>  int intel_atomic_setup_scalers(struct drm_device *dev,
>  	struct intel_crtc *intel_crtc,
>  	struct intel_crtc_state *crtc_state);
> diff --git a/drivers/gpu/drm/i915/intel_pm.c
> b/drivers/gpu/drm/i915/intel_pm.c
> index 379eabe093cb..8fb8c6891ae6 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -2010,11 +2010,18 @@ static void ilk_compute_wm_level(const struct
> drm_i915_private *dev_priv,
>  		cur_latency *= 5;
>  	}
>  
> -	result->pri_val = ilk_compute_pri_wm(cstate, pristate,
> -					     pri_latency, level);
> -	result->spr_val = ilk_compute_spr_wm(cstate, sprstate,
> spr_latency);
> -	result->cur_val = ilk_compute_cur_wm(cstate, curstate,
> cur_latency);
> -	result->fbc_val = ilk_compute_fbc_wm(cstate, pristate,
> result->pri_val);
> +	if (pristate) {
> +		result->pri_val = ilk_compute_pri_wm(cstate,
> pristate,
> +						     pri_latency,
> level);
> +		result->fbc_val = ilk_compute_fbc_wm(cstate,
> pristate, result->pri_val);
> +	}
> +
> +	if (sprstate)
> +		result->spr_val = ilk_compute_spr_wm(cstate,
> sprstate, spr_latency);
> +
> +	if (curstate)
> +		result->cur_val = ilk_compute_cur_wm(cstate,
> curstate, cur_latency);
> +
>  	result->enable = true;
>  }
>  
> @@ -2287,7 +2294,6 @@ static int ilk_compute_pipe_wm(struct
> intel_crtc *intel_crtc,
>  	const struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_crtc_state *cstate = NULL;
>  	struct intel_plane *intel_plane;
> -	struct drm_plane_state *ps;
>  	struct intel_plane_state *pristate = NULL;
>  	struct intel_plane_state *sprstate = NULL;
>  	struct intel_plane_state *curstate = NULL;
> @@ -2306,30 +2312,37 @@ static int ilk_compute_pipe_wm(struct
> intel_crtc *intel_crtc,
>  	memset(pipe_wm, 0, sizeof(*pipe_wm));
>  
>  	for_each_intel_plane_on_crtc(dev, intel_crtc, intel_plane) {
> -		ps = drm_atomic_get_plane_state(state,
> -						&intel_plane->base);
> -		if (IS_ERR(ps))
> -			return PTR_ERR(ps);
> +		struct intel_plane_state *ps;
> +
> +		ps = intel_atomic_get_existing_plane_state(state,
> +							   intel_pla
> ne);
> +		if (!ps)
> +			continue;

Ok, let me see if I understood: if some of these planes is not part of
the atomic commit, you're not going to include it in the calculations
since its value is not going to change. This would allow us lock less
planes, which is the main advantage. Is this correct?

If yes, how much do we really gain by saving some plane locking?

So by not assigning values to result->x_val at ilk_compute_wm_level()
when NULL is passed you're going to preserve whatever correct values
were already set earlier, so you don't need to recompute them. Is this
correct?

If the answer to the above is "yes", then don't we need to remove the
memset(pipe_wm, 0) at the beginning of ilk_compute_wm_level? Otherwise,
nothing will be preserved.

There's also the zero-initialization of "config" to consider.

Or maybe instead of all of the above, we're working under the
assumption that if some of the planes is not part of the atomic commit,
then all its relevant WM values will be zero?

>  
>  		if (intel_plane->base.type ==
> DRM_PLANE_TYPE_PRIMARY)
> -			pristate = to_intel_plane_state(ps);
> -		else if (intel_plane->base.type ==
> DRM_PLANE_TYPE_OVERLAY)
> -			sprstate = to_intel_plane_state(ps);
> -		else if (intel_plane->base.type ==
> DRM_PLANE_TYPE_CURSOR)
> -			curstate = to_intel_plane_state(ps);
> +			pristate = ps;
> +		else if (intel_plane->base.type ==
> DRM_PLANE_TYPE_OVERLAY) {
> +			sprstate = ps;
> +
> +			if (ps) {
> +				pipe_wm->sprites_enabled = sprstate-
> >visible;
> +				pipe_wm->sprites_scaled = sprstate-
> >visible &&

You're setting these values here...

> +					(drm_rect_width(&sprstate-
> >dst) != drm_rect_width(&sprstate->src) >> 16 ||
> +					drm_rect_height(&sprstate-
> >dst) != drm_rect_height(&sprstate->src) >> 16);
> +			}
> +		} else if (intel_plane->base.type ==
> DRM_PLANE_TYPE_CURSOR)

(missing brackets here, but if you follow my suggestion below you won't
need them)

> +			curstate = ps;
>  	}
>  
> -	config.sprites_enabled = sprstate->visible;
> -	config.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);
> +	config.sprites_enabled = pipe_wm->sprites_enabled;
> +	config.sprites_scaled = pipe_wm->sprites_scaled;
>  
>  	pipe_wm->pipe_enabled = cstate->base.active;
>  	pipe_wm->sprites_enabled = config.sprites_enabled;
>  	pipe_wm->sprites_scaled = config.sprites_scaled;

...but then we re-set them here.

Either you could remove these assignments here, or you could move the
"if (ps)" from the loop to outside it, becoming "if (sprstate)", making
the post-patch code similar to the pre-patch code. Since both pipe_wm
and config are zero-initialized you don't even need to think about the
!sprstate case.

>  
>  	/* ILK/SNB: LP2+ watermarks only w/o sprites */
> -	if (INTEL_INFO(dev)->gen <= 6 && sprstate->visible)
> +	if (INTEL_INFO(dev)->gen <= 6 && pipe_wm->sprites_enabled)
>  		max_level = 1;
>  
>  	/* ILK/SNB/IVB: LP1+ watermarks only w/o scaling */
> @@ -2352,20 +2365,18 @@ static int ilk_compute_pipe_wm(struct
> intel_crtc *intel_crtc,
>  	ilk_compute_wm_reg_maximums(dev, 1, &max);
>  
>  	for (level = 1; level <= max_level; level++) {
> -		struct intel_wm_level wm = {};
> +		struct intel_wm_level *wm = &pipe_wm->wm[level];
>  
>  		ilk_compute_wm_level(dev_priv, intel_crtc, level,
> cstate,
> -				     pristate, sprstate, curstate,
> &wm);
> +				     pristate, sprstate, curstate,
> wm);
>  
>  		/*
>  		 * Disable any watermark level that exceeds the
>  		 * register maximums since such watermarks are
>  		 * always invalid.
>  		 */
> -		if (!ilk_validate_wm_level(level, &max, &wm))
> +		if (!ilk_validate_wm_level(level, &max, wm))
>  			break;
> -
> -		pipe_wm->wm[level] = wm;

The change in the chunk above is that we're now leaving with non-zero
wm->{pri,spr,cur}_val if some level is invalid. Given the current
complexity of the code, it's not trivial verify whether nothing later
is assuming that invalid levels are all-zero, but it looks like we're
safe. Anyway, could you please move this to a separate patch that comes
before the other changes? It seems this change would be safe alone, and
we're seeing problems with WMs these days, so having nice bisectability
is a huge plus.

Thanks,
Paulo

>  	}
>  
>  	return 0;
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v6 3/8] drm/i915: Kill off intel_crtc->atomic.wait_vblank, v6.

[Maarten Lankhorst]

Currently we perform our own wait in post_plane_update,
but the atomic core performs another one in wait_for_vblanks.
This means that 2 vblanks are done when a fb is changed,
which is a bit overkill.

Merge them by creating a helper function that takes a crtc mask
for the planes to wait on.

The broadwell vblank workaround may look gone entirely but this is
not the case. pipe_config->wm_changed is set to true
when any plane is turned on, which forces a vblank wait.

Changes since v1:
- Removing the double vblank wait on broadwell moved to its own commit.
Changes since v2:
- Move out POWER_DOMAIN_MODESET handling to its own commit.
Changes since v3:
- Do not wait for vblank on legacy cursor updates. (Ville)
- Move broadwell vblank workaround comment to page_flip_finished. (Ville)
Changes since v4:
- Compile fix, legacy_cursor_flip -> *_update.
Changes since v5:
- Kill brackets.
- Add WARN_ON when wait_for_vblanks fails.
- Remove extra newlines.
- Split the checks whether vblank is needed to a separate function,
  with comments why a vblank is needed.
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
v5 was skipped, previous version was v5 because it had the compile fix.

diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
index 4625f8a9ba12..8e579a8505ac 100644
--- a/drivers/gpu/drm/i915/intel_atomic.c
+++ b/drivers/gpu/drm/i915/intel_atomic.c
@@ -97,6 +97,7 @@ intel_crtc_duplicate_state(struct drm_crtc *crtc)
 	crtc_state->disable_lp_wm = false;
 	crtc_state->disable_cxsr = false;
 	crtc_state->wm_changed = false;
+	crtc_state->fb_changed = false;
 
 	return &crtc_state->base;
 }
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 2107e324cd9e..9f32cb0bf978 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4792,9 +4792,6 @@ static void intel_post_plane_update(struct intel_crtc *crtc)
 		to_intel_crtc_state(crtc->base.state);
 	struct drm_device *dev = crtc->base.dev;
 
-	if (atomic->wait_vblank)
-		intel_wait_for_vblank(dev, crtc->pipe);
-
 	intel_frontbuffer_flip(dev, atomic->fb_bits);
 
 	crtc->wm.cxsr_allowed = true;
@@ -10957,6 +10954,12 @@ static bool page_flip_finished(struct intel_crtc *crtc)
 		return true;
 
 	/*
+	 * BDW signals flip done immediately if the plane
+	 * is disabled, even if the plane enable is already
+	 * armed to occur at the next vblank :(
+	 */
+
+	/*
 	 * A DSPSURFLIVE check isn't enough in case the mmio and CS flips
 	 * used the same base address. In that case the mmio flip might
 	 * have completed, but the CS hasn't even executed the flip yet.
@@ -11833,6 +11836,9 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
 	if (!was_visible && !visible)
 		return 0;
 
+	if (fb != old_plane_state->base.fb)
+		pipe_config->fb_changed = true;
+
 	turn_off = was_visible && (!visible || mode_changed);
 	turn_on = visible && (!was_visible || mode_changed);
 
@@ -11847,11 +11853,8 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
 		pipe_config->wm_changed = true;
 
 		/* must disable cxsr around plane enable/disable */
-		if (plane->type != DRM_PLANE_TYPE_CURSOR) {
-			if (is_crtc_enabled)
-				intel_crtc->atomic.wait_vblank = true;
+		if (plane->type != DRM_PLANE_TYPE_CURSOR)
 			pipe_config->disable_cxsr = true;
-		}
 	} else if (intel_wm_need_update(plane, plane_state)) {
 		pipe_config->wm_changed = true;
 	}
@@ -11865,14 +11868,6 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
 		intel_crtc->atomic.post_enable_primary = turn_on;
 		intel_crtc->atomic.update_fbc = true;
 
-		/*
-		 * BDW signals flip done immediately if the plane
-		 * is disabled, even if the plane enable is already
-		 * armed to occur at the next vblank :(
-		 */
-		if (turn_on && IS_BROADWELL(dev))
-			intel_crtc->atomic.wait_vblank = true;
-
 		break;
 	case DRM_PLANE_TYPE_CURSOR:
 		break;
@@ -11885,13 +11880,11 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
 		 */
 		if (IS_IVYBRIDGE(dev) &&
 		    needs_scaling(to_intel_plane_state(plane_state)) &&
-		    !needs_scaling(old_plane_state)) {
-			to_intel_crtc_state(crtc_state)->disable_lp_wm = true;
-		} else if (turn_off && !mode_changed) {
-			intel_crtc->atomic.wait_vblank = true;
+		    !needs_scaling(old_plane_state))
+			pipe_config->disable_lp_wm = true;
+		else if (turn_off && !mode_changed)
 			intel_crtc->atomic.update_sprite_watermarks |=
 				1 << i;
-		}
 
 		break;
 	}
@@ -13425,6 +13418,71 @@ static int intel_atomic_prepare_commit(struct drm_device *dev,
 	return ret;
 }
 
+static void intel_atomic_wait_for_vblanks(struct drm_device *dev,
+					  struct drm_i915_private *dev_priv,
+					  unsigned crtc_mask)
+{
+	unsigned last_vblank_count[I915_MAX_PIPES];
+	enum pipe pipe;
+	int ret;
+
+	if (!crtc_mask)
+		return;
+
+	for_each_pipe(dev_priv, pipe) {
+		struct drm_crtc *crtc = dev_priv->pipe_to_crtc_mapping[pipe];
+
+		if (!((1 << pipe) & crtc_mask))
+			continue;
+
+		ret = drm_crtc_vblank_get(crtc);
+		if (WARN_ON(ret != 0)) {
+			crtc_mask &= ~(1 << pipe);
+			continue;
+		}
+
+		last_vblank_count[pipe] = drm_crtc_vblank_count(crtc);
+	}
+
+	for_each_pipe(dev_priv, pipe) {
+		struct drm_crtc *crtc = dev_priv->pipe_to_crtc_mapping[pipe];
+		long lret;
+
+		if (!((1 << pipe) & crtc_mask))
+			continue;
+
+		lret = wait_event_timeout(dev->vblank[pipe].queue,
+				last_vblank_count[pipe] !=
+					drm_crtc_vblank_count(crtc),
+				msecs_to_jiffies(50));
+
+		WARN_ON(!lret);
+
+		drm_crtc_vblank_put(crtc);
+	}
+}
+
+static bool needs_vblank_wait(struct intel_crtc_state *crtc_state)
+{
+	/* fb updated, need to unpin old fb */
+	if (crtc_state->fb_changed)
+		return true;
+
+	/* wm changes, need vblank before final wm's */
+	if (crtc_state->wm_changed)
+		return true;
+
+	/*
+	 * cxsr is re-enabled after vblank.
+	 * This is already handled by crtc_state->wm_changed,
+	 * but added for clarity.
+	 */
+	if (crtc_state->disable_cxsr)
+		return true;
+
+	return false;
+}
+
 /**
  * intel_atomic_commit - commit validated state object
  * @dev: DRM device
@@ -13452,6 +13510,7 @@ static int intel_atomic_commit(struct drm_device *dev,
 	int ret = 0, i;
 	bool hw_check = intel_state->modeset;
 	unsigned long put_domains[I915_MAX_PIPES] = {};
+	unsigned crtc_vblank_mask = 0;
 
 	ret = intel_atomic_prepare_commit(dev, state, async);
 	if (ret) {
@@ -13525,8 +13584,9 @@ static int intel_atomic_commit(struct drm_device *dev,
 	for_each_crtc_in_state(state, crtc, crtc_state, i) {
 		struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 		bool modeset = needs_modeset(crtc->state);
-		bool update_pipe = !modeset &&
-			to_intel_crtc_state(crtc->state)->update_pipe;
+		struct intel_crtc_state *pipe_config =
+			to_intel_crtc_state(crtc->state);
+		bool update_pipe = !modeset && pipe_config->update_pipe;
 
 		if (modeset && crtc->state->active) {
 			update_scanline_offset(to_intel_crtc(crtc));
@@ -13543,14 +13603,18 @@ static int intel_atomic_commit(struct drm_device *dev,
 		    (crtc->state->planes_changed || update_pipe))
 			drm_atomic_helper_commit_planes_on_crtc(crtc_state);
 
-		intel_post_plane_update(intel_crtc);
+		if (pipe_config->base.active && needs_vblank_wait(pipe_config))
+			crtc_vblank_mask |= 1 << i;
 	}
 
 	/* FIXME: add subpixel order */
 
-	drm_atomic_helper_wait_for_vblanks(dev, state);
+	if (!state->legacy_cursor_update)
+		intel_atomic_wait_for_vblanks(dev, dev_priv, crtc_vblank_mask);
 
 	for_each_crtc_in_state(state, crtc, crtc_state, i) {
+		intel_post_plane_update(to_intel_crtc(crtc));
+
 		if (put_domains[i])
 			modeset_put_power_domains(dev_priv, put_domains[i]);
 	}
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 285b0570be9c..74df4f818e03 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -379,6 +379,7 @@ struct intel_crtc_state {
 	bool update_pipe; /* can a fast modeset be performed? */
 	bool disable_cxsr;
 	bool wm_changed; /* watermarks are updated */
+	bool fb_changed; /* fb on any of the planes is changed */
 
 	/* Pipe source size (ie. panel fitter input size)
 	 * All planes will be positioned inside this space,
@@ -547,7 +548,6 @@ struct intel_crtc_atomic_commit {
 
 	/* Sleepable operations to perform after commit */
 	unsigned fb_bits;
-	bool wait_vblank;
 	bool post_enable_primary;
 	unsigned update_sprite_watermarks;
 

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

Re: [PATCH v6 3/8] drm/i915: Kill off intel_crtc->atomic.wait_vblank, v6.

[Zanoni, Paulo R]

Em Qua, 2016-02-24 às 11:24 +0100, Maarten Lankhorst escreveu:
> Currently we perform our own wait in post_plane_update,
> but the atomic core performs another one in wait_for_vblanks.
> This means that 2 vblanks are done when a fb is changed,
> which is a bit overkill.
> 
> Merge them by creating a helper function that takes a crtc mask
> for the planes to wait on.
> 
> The broadwell vblank workaround may look gone entirely but this is
> not the case. pipe_config->wm_changed is set to true
> when any plane is turned on, which forces a vblank wait.
> 
> Changes since v1:
> - Removing the double vblank wait on broadwell moved to its own
> commit.
> Changes since v2:
> - Move out POWER_DOMAIN_MODESET handling to its own commit.
> Changes since v3:
> - Do not wait for vblank on legacy cursor updates. (Ville)
> - Move broadwell vblank workaround comment to page_flip_finished.
> (Ville)
> Changes since v4:
> - Compile fix, legacy_cursor_flip -> *_update.
> Changes since v5:
> - Kill brackets.
> - Add WARN_ON when wait_for_vblanks fails.
> - Remove extra newlines.
> - Split the checks whether vblank is needed to a separate function,
>   with comments why a vblank is needed.
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

> ---
> v5 was skipped, previous version was v5 because it had the compile
> fix.
> 
> diff --git a/drivers/gpu/drm/i915/intel_atomic.c
> b/drivers/gpu/drm/i915/intel_atomic.c
> index 4625f8a9ba12..8e579a8505ac 100644
> --- a/drivers/gpu/drm/i915/intel_atomic.c
> +++ b/drivers/gpu/drm/i915/intel_atomic.c
> @@ -97,6 +97,7 @@ intel_crtc_duplicate_state(struct drm_crtc *crtc)
>  	crtc_state->disable_lp_wm = false;
>  	crtc_state->disable_cxsr = false;
>  	crtc_state->wm_changed = false;
> +	crtc_state->fb_changed = false;
>  
>  	return &crtc_state->base;
>  }
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index 2107e324cd9e..9f32cb0bf978 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4792,9 +4792,6 @@ static void intel_post_plane_update(struct
> intel_crtc *crtc)
>  		to_intel_crtc_state(crtc->base.state);
>  	struct drm_device *dev = crtc->base.dev;
>  
> -	if (atomic->wait_vblank)
> -		intel_wait_for_vblank(dev, crtc->pipe);
> -
>  	intel_frontbuffer_flip(dev, atomic->fb_bits);
>  
>  	crtc->wm.cxsr_allowed = true;
> @@ -10957,6 +10954,12 @@ static bool page_flip_finished(struct
> intel_crtc *crtc)
>  		return true;
>  
>  	/*
> +	 * BDW signals flip done immediately if the plane
> +	 * is disabled, even if the plane enable is already
> +	 * armed to occur at the next vblank :(
> +	 */
> +
> +	/*
>  	 * A DSPSURFLIVE check isn't enough in case the mmio and CS
> flips
>  	 * used the same base address. In that case the mmio flip
> might
>  	 * have completed, but the CS hasn't even executed the flip
> yet.
> @@ -11833,6 +11836,9 @@ int intel_plane_atomic_calc_changes(struct
> drm_crtc_state *crtc_state,
>  	if (!was_visible && !visible)
>  		return 0;
>  
> +	if (fb != old_plane_state->base.fb)
> +		pipe_config->fb_changed = true;
> +
>  	turn_off = was_visible && (!visible || mode_changed);
>  	turn_on = visible && (!was_visible || mode_changed);
>  
> @@ -11847,11 +11853,8 @@ int intel_plane_atomic_calc_changes(struct
> drm_crtc_state *crtc_state,
>  		pipe_config->wm_changed = true;
>  
>  		/* must disable cxsr around plane enable/disable */
> -		if (plane->type != DRM_PLANE_TYPE_CURSOR) {
> -			if (is_crtc_enabled)
> -				intel_crtc->atomic.wait_vblank =
> true;
> +		if (plane->type != DRM_PLANE_TYPE_CURSOR)
>  			pipe_config->disable_cxsr = true;
> -		}
>  	} else if (intel_wm_need_update(plane, plane_state)) {
>  		pipe_config->wm_changed = true;
>  	}
> @@ -11865,14 +11868,6 @@ int intel_plane_atomic_calc_changes(struct
> drm_crtc_state *crtc_state,
>  		intel_crtc->atomic.post_enable_primary = turn_on;
>  		intel_crtc->atomic.update_fbc = true;
>  
> -		/*
> -		 * BDW signals flip done immediately if the plane
> -		 * is disabled, even if the plane enable is already
> -		 * armed to occur at the next vblank :(
> -		 */
> -		if (turn_on && IS_BROADWELL(dev))
> -			intel_crtc->atomic.wait_vblank = true;
> -
>  		break;
>  	case DRM_PLANE_TYPE_CURSOR:
>  		break;
> @@ -11885,13 +11880,11 @@ int intel_plane_atomic_calc_changes(struct
> drm_crtc_state *crtc_state,
>  		 */
>  		if (IS_IVYBRIDGE(dev) &&
>  		    needs_scaling(to_intel_plane_state(plane_state))
> &&
> -		    !needs_scaling(old_plane_state)) {
> -			to_intel_crtc_state(crtc_state)-
> >disable_lp_wm = true;
> -		} else if (turn_off && !mode_changed) {
> -			intel_crtc->atomic.wait_vblank = true;
> +		    !needs_scaling(old_plane_state))
> +			pipe_config->disable_lp_wm = true;
> +		else if (turn_off && !mode_changed)
>  			intel_crtc->atomic.update_sprite_watermarks
> |=
>  				1 << i;
> -		}
>  
>  		break;
>  	}
> @@ -13425,6 +13418,71 @@ static int
> intel_atomic_prepare_commit(struct drm_device *dev,
>  	return ret;
>  }
>  
> +static void intel_atomic_wait_for_vblanks(struct drm_device *dev,
> +					  struct drm_i915_private
> *dev_priv,
> +					  unsigned crtc_mask)
> +{
> +	unsigned last_vblank_count[I915_MAX_PIPES];
> +	enum pipe pipe;
> +	int ret;
> +
> +	if (!crtc_mask)
> +		return;
> +
> +	for_each_pipe(dev_priv, pipe) {
> +		struct drm_crtc *crtc = dev_priv-
> >pipe_to_crtc_mapping[pipe];
> +
> +		if (!((1 << pipe) & crtc_mask))
> +			continue;
> +
> +		ret = drm_crtc_vblank_get(crtc);
> +		if (WARN_ON(ret != 0)) {
> +			crtc_mask &= ~(1 << pipe);
> +			continue;
> +		}
> +
> +		last_vblank_count[pipe] =
> drm_crtc_vblank_count(crtc);
> +	}
> +
> +	for_each_pipe(dev_priv, pipe) {
> +		struct drm_crtc *crtc = dev_priv-
> >pipe_to_crtc_mapping[pipe];
> +		long lret;
> +
> +		if (!((1 << pipe) & crtc_mask))
> +			continue;
> +
> +		lret = wait_event_timeout(dev->vblank[pipe].queue,
> +				last_vblank_count[pipe] !=
> +					drm_crtc_vblank_count(crtc),
> +				msecs_to_jiffies(50));
> +
> +		WARN_ON(!lret);
> +
> +		drm_crtc_vblank_put(crtc);
> +	}
> +}
> +
> +static bool needs_vblank_wait(struct intel_crtc_state *crtc_state)
> +{
> +	/* fb updated, need to unpin old fb */
> +	if (crtc_state->fb_changed)
> +		return true;
> +
> +	/* wm changes, need vblank before final wm's */
> +	if (crtc_state->wm_changed)
> +		return true;
> +
> +	/*
> +	 * cxsr is re-enabled after vblank.
> +	 * This is already handled by crtc_state->wm_changed,
> +	 * but added for clarity.
> +	 */
> +	if (crtc_state->disable_cxsr)
> +		return true;
> +
> +	return false;
> +}
> +
>  /**
>   * intel_atomic_commit - commit validated state object
>   * @dev: DRM device
> @@ -13452,6 +13510,7 @@ static int intel_atomic_commit(struct
> drm_device *dev,
>  	int ret = 0, i;
>  	bool hw_check = intel_state->modeset;
>  	unsigned long put_domains[I915_MAX_PIPES] = {};
> +	unsigned crtc_vblank_mask = 0;
>  
>  	ret = intel_atomic_prepare_commit(dev, state, async);
>  	if (ret) {
> @@ -13525,8 +13584,9 @@ static int intel_atomic_commit(struct
> drm_device *dev,
>  	for_each_crtc_in_state(state, crtc, crtc_state, i) {
>  		struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  		bool modeset = needs_modeset(crtc->state);
> -		bool update_pipe = !modeset &&
> -			to_intel_crtc_state(crtc->state)-
> >update_pipe;
> +		struct intel_crtc_state *pipe_config =
> +			to_intel_crtc_state(crtc->state);
> +		bool update_pipe = !modeset && pipe_config-
> >update_pipe;
>  
>  		if (modeset && crtc->state->active) {
>  			update_scanline_offset(to_intel_crtc(crtc));
> @@ -13543,14 +13603,18 @@ static int intel_atomic_commit(struct
> drm_device *dev,
>  		    (crtc->state->planes_changed || update_pipe))
>  			drm_atomic_helper_commit_planes_on_crtc(crtc
> _state);
>  
> -		intel_post_plane_update(intel_crtc);
> +		if (pipe_config->base.active &&
> needs_vblank_wait(pipe_config))
> +			crtc_vblank_mask |= 1 << i;
>  	}
>  
>  	/* FIXME: add subpixel order */
>  
> -	drm_atomic_helper_wait_for_vblanks(dev, state);
> +	if (!state->legacy_cursor_update)
> +		intel_atomic_wait_for_vblanks(dev, dev_priv,
> crtc_vblank_mask);
>  
>  	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> +		intel_post_plane_update(to_intel_crtc(crtc));
> +
>  		if (put_domains[i])
>  			modeset_put_power_domains(dev_priv,
> put_domains[i]);
>  	}
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index 285b0570be9c..74df4f818e03 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -379,6 +379,7 @@ struct intel_crtc_state {
>  	bool update_pipe; /* can a fast modeset be performed? */
>  	bool disable_cxsr;
>  	bool wm_changed; /* watermarks are updated */
> +	bool fb_changed; /* fb on any of the planes is changed */
>  
>  	/* Pipe source size (ie. panel fitter input size)
>  	 * All planes will be positioned inside this space,
> @@ -547,7 +548,6 @@ struct intel_crtc_atomic_commit {
>  
>  	/* Sleepable operations to perform after commit */
>  	unsigned fb_bits;
> -	bool wait_vblank;
>  	bool post_enable_primary;
>  	unsigned update_sprite_watermarks;
>  
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v4.1 6/8] drm/i915: Nuke fbc members from intel_crtc->atomic, v3.

[Maarten Lankhorst]

Whenever there's an update to the primary plane,
fbc_pre_update and fbc_post_update are called. Kill off
intel_crtc->atomic.update_fbc and now that intel_crtc->atomic
is empty, kill it off too.

Changes since v1:
- Add a intel_fbc_supports_rotation helper.
Changes sinec v2:
- Remove intel_fbc_supports_rotation_helper.
- Remove unrelated changes.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
This version doesn't change intel_fbc_enable any more!

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 78f72bef25aa..c8404092aee9 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4789,11 +4789,9 @@ 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_atomic_state *old_state = old_crtc_state->base.state;
-	struct intel_crtc_atomic_commit *atomic = &crtc->atomic;
 	struct intel_crtc_state *pipe_config =
 		to_intel_crtc_state(crtc->base.state);
 	struct drm_device *dev = crtc->base.dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_plane *primary = crtc->base.primary;
 	struct drm_plane_state *old_pri_state =
 		drm_atomic_get_existing_plane_state(old_state, primary);
@@ -4805,22 +4803,19 @@ static void intel_post_plane_update(struct intel_crtc_state *old_crtc_state)
 	if (pipe_config->wm_changed && pipe_config->base.active)
 		intel_update_watermarks(&crtc->base);
 
-	if (atomic->update_fbc)
-		intel_fbc_post_update(crtc);
-
 	if (old_pri_state) {
 		struct intel_plane_state *primary_state =
 			to_intel_plane_state(primary->state);
 		struct intel_plane_state *old_primary_state =
 			to_intel_plane_state(old_pri_state);
 
+		intel_fbc_post_update(crtc);
+
 		if (primary_state->visible &&
 		    (needs_modeset(&pipe_config->base) ||
 		     !old_primary_state->visible))
 			intel_post_enable_primary(&crtc->base);
 	}
-
-	memset(atomic, 0, sizeof(*atomic));
 }
 
 static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state)
@@ -4828,7 +4823,6 @@ static void intel_pre_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 = dev->dev_private;
-	struct intel_crtc_atomic_commit *atomic = &crtc->atomic;
 	struct intel_crtc_state *pipe_config =
 		to_intel_crtc_state(crtc->base.state);
 	struct drm_atomic_state *old_state = old_crtc_state->base.state;
@@ -4837,15 +4831,14 @@ static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state)
 		drm_atomic_get_existing_plane_state(old_state, primary);
 	bool modeset = needs_modeset(&pipe_config->base);
 
-	if (atomic->update_fbc)
-		intel_fbc_pre_update(crtc);
-
 	if (old_pri_state) {
 		struct intel_plane_state *primary_state =
 			to_intel_plane_state(primary->state);
 		struct intel_plane_state *old_primary_state =
 			to_intel_plane_state(old_pri_state);
 
+		intel_fbc_pre_update(crtc);
+
 		if (old_primary_state->visible &&
 		    (modeset || !primary_state->visible))
 			intel_pre_disable_primary(&crtc->base);
@@ -11877,27 +11870,17 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
 	if (visible || was_visible)
 		pipe_config->fb_bits |= to_intel_plane(plane)->frontbuffer_bit;
 
-	switch (plane->type) {
-	case DRM_PLANE_TYPE_PRIMARY:
-		intel_crtc->atomic.update_fbc = true;
-
-		break;
-	case DRM_PLANE_TYPE_CURSOR:
-		break;
-	case DRM_PLANE_TYPE_OVERLAY:
-		/*
-		 * WaCxSRDisabledForSpriteScaling:ivb
-		 *
-		 * cstate->update_wm was already set above, so this flag will
-		 * take effect when we commit and program watermarks.
-		 */
-		if (IS_IVYBRIDGE(dev) &&
-		    needs_scaling(to_intel_plane_state(plane_state)) &&
-		    !needs_scaling(old_plane_state))
-			pipe_config->disable_lp_wm = true;
+	/*
+	 * WaCxSRDisabledForSpriteScaling:ivb
+	 *
+	 * cstate->update_wm was already set above, so this flag will
+	 * take effect when we commit and program watermarks.
+	 */
+	if (plane->type == DRM_PLANE_TYPE_OVERLAY && IS_IVYBRIDGE(dev) &&
+	    needs_scaling(to_intel_plane_state(plane_state)) &&
+	    !needs_scaling(old_plane_state))
+		pipe_config->disable_lp_wm = true;
 
-		break;
-	}
 	return 0;
 }
 
@@ -13296,9 +13279,6 @@ static int intel_atomic_check(struct drm_device *dev,
 		struct intel_crtc_state *pipe_config =
 			to_intel_crtc_state(crtc_state);
 
-		memset(&to_intel_crtc(crtc)->atomic, 0,
-		       sizeof(struct intel_crtc_atomic_commit));
-
 		/* Catch I915_MODE_FLAG_INHERITED */
 		if (crtc_state->mode.private_flags != crtc->state->mode.private_flags)
 			crtc_state->mode_changed = true;
@@ -13606,7 +13586,8 @@ static int intel_atomic_commit(struct drm_device *dev,
 		if (!modeset)
 			intel_pre_plane_update(to_intel_crtc_state(crtc_state));
 
-		if (crtc->state->active && intel_crtc->atomic.update_fbc)
+		if (crtc->state->active &&
+		    drm_atomic_get_existing_plane_state(state, crtc->primary))
 			intel_fbc_enable(intel_crtc);
 
 		if (crtc->state->active &&
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 058bb10e53ae..5e5b64fff30d 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -538,19 +538,6 @@ struct intel_mmio_flip {
 	unsigned int rotation;
 };
 
-/*
- * Tracking of operations that need to be performed at the beginning/end of an
- * atomic commit, outside the atomic section where interrupts are disabled.
- * These are generally operations that grab mutexes or might otherwise sleep
- * and thus can't be run with interrupts disabled.
- */
-struct intel_crtc_atomic_commit {
-	/* Sleepable operations to perform before commit */
-
-	/* Sleepable operations to perform after commit */
-	bool update_fbc;
-};
-
 struct intel_crtc {
 	struct drm_crtc base;
 	enum pipe pipe;
@@ -610,8 +597,6 @@ struct intel_crtc {
 		int scanline_start;
 	} debug;
 
-	struct intel_crtc_atomic_commit atomic;
-
 	/* scalers available on this crtc */
 	int num_scalers;
 

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

Re: [PATCH v4.1 6/8] drm/i915: Nuke fbc members from intel_crtc->atomic, v3.

[Zanoni, Paulo R]

Em Seg, 2016-02-29 às 10:58 +0100, Maarten Lankhorst escreveu:
> Whenever there's an update to the primary plane,
> fbc_pre_update and fbc_post_update are called. Kill off
> intel_crtc->atomic.update_fbc and now that intel_crtc->atomic
> is empty, kill it off too.
> 
> Changes since v1:
> - Add a intel_fbc_supports_rotation helper.
> Changes sinec v2:
> - Remove intel_fbc_supports_rotation_helper.
> - Remove unrelated changes.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
> This version doesn't change intel_fbc_enable any more!

Ok, so I decided to test this.

First of all, patch 05 needs a rebase too. I did my own local rebase
and I suppose I got things right.

Second, after I did actual testing, I discovered that, contrary to my
previous belief, "old_pri_state" will also be valid when we're only
changing the sprite or cursor planes. So if we apply this patch, we'll
be calling pre_update+enable+post_update for every plane/cursor
enable/disable operation, which is not what we want since these are
unnecessary calls, as FBC only deals with the primary plane. I think we
need to fix this.

The tests I've been using to check this were:
- kms_frontbuffer_tracking/fbc-1p-primscrn-cur-indfb-onoff
- kms_frontbuffer_tracking/fbc-1p-primscrn-spr-indfb-onoff


> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index 78f72bef25aa..c8404092aee9 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4789,11 +4789,9 @@ 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_atomic_state *old_state = old_crtc_state-
> >base.state;
> -	struct intel_crtc_atomic_commit *atomic = &crtc->atomic;
>  	struct intel_crtc_state *pipe_config =
>  		to_intel_crtc_state(crtc->base.state);
>  	struct drm_device *dev = crtc->base.dev;
> -	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct drm_plane *primary = crtc->base.primary;
>  	struct drm_plane_state *old_pri_state =
>  		drm_atomic_get_existing_plane_state(old_state,
> primary);
> @@ -4805,22 +4803,19 @@ static void intel_post_plane_update(struct
> intel_crtc_state *old_crtc_state)
>  	if (pipe_config->wm_changed && pipe_config->base.active)
>  		intel_update_watermarks(&crtc->base);
>  
> -	if (atomic->update_fbc)
> -		intel_fbc_post_update(crtc);
> -
>  	if (old_pri_state) {
>  		struct intel_plane_state *primary_state =
>  			to_intel_plane_state(primary->state);
>  		struct intel_plane_state *old_primary_state =
>  			to_intel_plane_state(old_pri_state);
>  
> +		intel_fbc_post_update(crtc);
> +
>  		if (primary_state->visible &&
>  		    (needs_modeset(&pipe_config->base) ||
>  		     !old_primary_state->visible))
>  			intel_post_enable_primary(&crtc->base);
>  	}
> -
> -	memset(atomic, 0, sizeof(*atomic));
>  }
>  
>  static void intel_pre_plane_update(struct intel_crtc_state
> *old_crtc_state)
> @@ -4828,7 +4823,6 @@ static void intel_pre_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 = dev->dev_private;
> -	struct intel_crtc_atomic_commit *atomic = &crtc->atomic;
>  	struct intel_crtc_state *pipe_config =
>  		to_intel_crtc_state(crtc->base.state);
>  	struct drm_atomic_state *old_state = old_crtc_state-
> >base.state;
> @@ -4837,15 +4831,14 @@ static void intel_pre_plane_update(struct
> intel_crtc_state *old_crtc_state)
>  		drm_atomic_get_existing_plane_state(old_state,
> primary);
>  	bool modeset = needs_modeset(&pipe_config->base);
>  
> -	if (atomic->update_fbc)
> -		intel_fbc_pre_update(crtc);
> -
>  	if (old_pri_state) {
>  		struct intel_plane_state *primary_state =
>  			to_intel_plane_state(primary->state);
>  		struct intel_plane_state *old_primary_state =
>  			to_intel_plane_state(old_pri_state);
>  
> +		intel_fbc_pre_update(crtc);
> +
>  		if (old_primary_state->visible &&
>  		    (modeset || !primary_state->visible))
>  			intel_pre_disable_primary(&crtc->base);
> @@ -11877,27 +11870,17 @@ int intel_plane_atomic_calc_changes(struct
> drm_crtc_state *crtc_state,
>  	if (visible || was_visible)
>  		pipe_config->fb_bits |= to_intel_plane(plane)-
> >frontbuffer_bit;
>  
> -	switch (plane->type) {
> -	case DRM_PLANE_TYPE_PRIMARY:
> -		intel_crtc->atomic.update_fbc = true;
> -
> -		break;
> -	case DRM_PLANE_TYPE_CURSOR:
> -		break;
> -	case DRM_PLANE_TYPE_OVERLAY:
> -		/*
> -		 * WaCxSRDisabledForSpriteScaling:ivb
> -		 *
> -		 * cstate->update_wm was already set above, so this
> flag will
> -		 * take effect when we commit and program
> watermarks.
> -		 */
> -		if (IS_IVYBRIDGE(dev) &&
> -		    needs_scaling(to_intel_plane_state(plane_state))
> &&
> -		    !needs_scaling(old_plane_state))
> -			pipe_config->disable_lp_wm = true;
> +	/*
> +	 * WaCxSRDisabledForSpriteScaling:ivb
> +	 *
> +	 * cstate->update_wm was already set above, so this flag
> will
> +	 * take effect when we commit and program watermarks.
> +	 */
> +	if (plane->type == DRM_PLANE_TYPE_OVERLAY &&
> IS_IVYBRIDGE(dev) &&
> +	    needs_scaling(to_intel_plane_state(plane_state)) &&
> +	    !needs_scaling(old_plane_state))
> +		pipe_config->disable_lp_wm = true;
>  
> -		break;
> -	}
>  	return 0;
>  }
>  
> @@ -13296,9 +13279,6 @@ static int intel_atomic_check(struct
> drm_device *dev,
>  		struct intel_crtc_state *pipe_config =
>  			to_intel_crtc_state(crtc_state);
>  
> -		memset(&to_intel_crtc(crtc)->atomic, 0,
> -		       sizeof(struct intel_crtc_atomic_commit));
> -
>  		/* Catch I915_MODE_FLAG_INHERITED */
>  		if (crtc_state->mode.private_flags != crtc->state-
> >mode.private_flags)
>  			crtc_state->mode_changed = true;
> @@ -13606,7 +13586,8 @@ static int intel_atomic_commit(struct
> drm_device *dev,
>  		if (!modeset)
>  			intel_pre_plane_update(to_intel_crtc_state(c
> rtc_state));
>  
> -		if (crtc->state->active && intel_crtc-
> >atomic.update_fbc)
> +		if (crtc->state->active &&
> +		    drm_atomic_get_existing_plane_state(state, crtc-
> >primary))
>  			intel_fbc_enable(intel_crtc);
>  
>  		if (crtc->state->active &&
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index 058bb10e53ae..5e5b64fff30d 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -538,19 +538,6 @@ struct intel_mmio_flip {
>  	unsigned int rotation;
>  };
>  
> -/*
> - * Tracking of operations that need to be performed at the
> beginning/end of an
> - * atomic commit, outside the atomic section where interrupts are
> disabled.
> - * These are generally operations that grab mutexes or might
> otherwise sleep
> - * and thus can't be run with interrupts disabled.
> - */
> -struct intel_crtc_atomic_commit {
> -	/* Sleepable operations to perform before commit */
> -
> -	/* Sleepable operations to perform after commit */
> -	bool update_fbc;
> -};
> -
>  struct intel_crtc {
>  	struct drm_crtc base;
>  	enum pipe pipe;
> @@ -610,8 +597,6 @@ struct intel_crtc {
>  		int scanline_start;
>  	} debug;
>  
> -	struct intel_crtc_atomic_commit atomic;
> -
>  	/* scalers available on this crtc */
>  	int num_scalers;
>  
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v4.1 6/8] drm/i915: Nuke fbc members from intel_crtc->atomic, v3.

[Maarten Lankhorst]

Op 29-02-16 om 22:06 schreef Zanoni, Paulo R:
> Em Seg, 2016-02-29 às 10:58 +0100, Maarten Lankhorst escreveu:
>> Whenever there's an update to the primary plane,
>> fbc_pre_update and fbc_post_update are called. Kill off
>> intel_crtc->atomic.update_fbc and now that intel_crtc->atomic
>> is empty, kill it off too.
>>
>> Changes since v1:
>> - Add a intel_fbc_supports_rotation helper.
>> Changes sinec v2:
>> - Remove intel_fbc_supports_rotation_helper.
>> - Remove unrelated changes.
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>> This version doesn't change intel_fbc_enable any more!
> Ok, so I decided to test this.
>
> First of all, patch 05 needs a rebase too. I did my own local rebase
> and I suppose I got things right.
>
> Second, after I did actual testing, I discovered that, contrary to my
> previous belief, "old_pri_state" will also be valid when we're only
> changing the sprite or cursor planes. So if we apply this patch, we'll
> be calling pre_update+enable+post_update for every plane/cursor
> enable/disable operation, which is not what we want since these are
> unnecessary calls, as FBC only deals with the primary plane. I think we
> need to fix this.
This is a bug addressed by the next patch.

drm/i915: Do not compute watermarks on a noop.

I'll send the improved version for that. I didn't do it yet because I was waiting for the ILK wm patch to land.

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

Re: [PATCH v4 7/8] drm/i915: Do not compute watermarks on a noop.

[Maarten Lankhorst]

Op 18-02-16 om 21:51 schreef Zanoni, Paulo R:
> Em Qua, 2016-02-10 às 13:49 +0100, Maarten Lankhorst escreveu:
>> No atomic state should be included after all validation when nothing
>> has
>> changed. During modeset all active planes will be added to the state,
>> while disabled planes won't change their state.
> As someone who is also not super familiar with the new watermarks code,
> I really really wish I had a more detailed commit message to allow me
> to understand your train of thought. I'll ask some questions below to
> validate my understanding.
>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> Cc: Matt Roper <matthew.d.roper@intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_display.c |  3 +-
>>  drivers/gpu/drm/i915/intel_drv.h     | 13 ++++++++
>>  drivers/gpu/drm/i915/intel_pm.c      | 61 +++++++++++++++++++++-----
>> ----------
>>  3 files changed, 51 insertions(+), 26 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c
>> b/drivers/gpu/drm/i915/intel_display.c
>> index 00cb261c6787..6bb1f5dbc7a0 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -11910,7 +11910,8 @@ static int intel_crtc_atomic_check(struct
>> drm_crtc *crtc,
>>  	}
>>  
>>  	ret = 0;
>> -	if (dev_priv->display.compute_pipe_wm) {
>> +	if (dev_priv->display.compute_pipe_wm &&
>> +	    (mode_changed || pipe_config->update_pipe || crtc_state-
>>> planes_changed)) {
>>  		ret = dev_priv->display.compute_pipe_wm(intel_crtc,
>> state);
>>  		if (ret)
>>  			return ret;
> Can't this chunk be on its own separate commit? I'm not sure why the
> rest of the commit is related to this change.
>
> It seems the rest of the commit is aimed reducing the number of planes
> we have to lock, not about not computing WMs if nothing in the pipe has
> changed.
>
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h
>> b/drivers/gpu/drm/i915/intel_drv.h
>> index 8effb9ece21e..144597ac74e3 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -1583,6 +1583,19 @@ intel_atomic_get_crtc_state(struct
>> drm_atomic_state *state,
>>  
>>  	return to_intel_crtc_state(crtc_state);
>>  }
>> +
>> +static inline struct intel_plane_state *
>> +intel_atomic_get_existing_plane_state(struct drm_atomic_state
>> *state,
>> +				      struct intel_plane *plane)
>> +{
>> +	struct drm_plane_state *plane_state;
>> +
>> +	plane_state = drm_atomic_get_existing_plane_state(state,
>> &plane->base);
>> +
>> +	return to_intel_plane_state(plane_state);
>> +}
>> +
>> +
> Two newlines above.
>
> It seems you'll be able to simplify a lot of stuff with this new
> function. I'm looking forward to review that patch :)
>
>
>>  int intel_atomic_setup_scalers(struct drm_device *dev,
>>  	struct intel_crtc *intel_crtc,
>>  	struct intel_crtc_state *crtc_state);
>> diff --git a/drivers/gpu/drm/i915/intel_pm.c
>> b/drivers/gpu/drm/i915/intel_pm.c
>> index 379eabe093cb..8fb8c6891ae6 100644
>> --- a/drivers/gpu/drm/i915/intel_pm.c
>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>> @@ -2010,11 +2010,18 @@ static void ilk_compute_wm_level(const struct
>> drm_i915_private *dev_priv,
>>  		cur_latency *= 5;
>>  	}
>>  
>> -	result->pri_val = ilk_compute_pri_wm(cstate, pristate,
>> -					     pri_latency, level);
>> -	result->spr_val = ilk_compute_spr_wm(cstate, sprstate,
>> spr_latency);
>> -	result->cur_val = ilk_compute_cur_wm(cstate, curstate,
>> cur_latency);
>> -	result->fbc_val = ilk_compute_fbc_wm(cstate, pristate,
>> result->pri_val);
>> +	if (pristate) {
>> +		result->pri_val = ilk_compute_pri_wm(cstate,
>> pristate,
>> +						     pri_latency,
>> level);
>> +		result->fbc_val = ilk_compute_fbc_wm(cstate,
>> pristate, result->pri_val);
>> +	}
>> +
>> +	if (sprstate)
>> +		result->spr_val = ilk_compute_spr_wm(cstate,
>> sprstate, spr_latency);
>> +
>> +	if (curstate)
>> +		result->cur_val = ilk_compute_cur_wm(cstate,
>> curstate, cur_latency);
>> +
>>  	result->enable = true;
>>  }
>>  
>> @@ -2287,7 +2294,6 @@ static int ilk_compute_pipe_wm(struct
>> intel_crtc *intel_crtc,
>>  	const struct drm_i915_private *dev_priv = dev->dev_private;
>>  	struct intel_crtc_state *cstate = NULL;
>>  	struct intel_plane *intel_plane;
>> -	struct drm_plane_state *ps;
>>  	struct intel_plane_state *pristate = NULL;
>>  	struct intel_plane_state *sprstate = NULL;
>>  	struct intel_plane_state *curstate = NULL;
>> @@ -2306,30 +2312,37 @@ static int ilk_compute_pipe_wm(struct
>> intel_crtc *intel_crtc,
>>  	memset(pipe_wm, 0, sizeof(*pipe_wm));
>>  
>>  	for_each_intel_plane_on_crtc(dev, intel_crtc, intel_plane) {
>> -		ps = drm_atomic_get_plane_state(state,
>> -						&intel_plane->base);
>> -		if (IS_ERR(ps))
>> -			return PTR_ERR(ps);
>> +		struct intel_plane_state *ps;
>> +
>> +		ps = intel_atomic_get_existing_plane_state(state,
>> +							   intel_pla
>> ne);
>> +		if (!ps)
>> +			continue;
> Ok, let me see if I understood: if some of these planes is not part of
> the atomic commit, you're not going to include it in the calculations
> since its value is not going to change. This would allow us lock less
> planes, which is the main advantage. Is this correct?
>
> If yes, how much do we really gain by saving some plane locking?
>
> So by not assigning values to result->x_val at ilk_compute_wm_level()
> when NULL is passed you're going to preserve whatever correct values
> were already set earlier, so you don't need to recompute them. Is this
> correct?
>
> If the answer to the above is "yes", then don't we need to remove the
> memset(pipe_wm, 0) at the beginning of ilk_compute_wm_level? Otherwise,
> nothing will be preserved.
>
> There's also the zero-initialization of "config" to consider.
>
> Or maybe instead of all of the above, we're working under the
> assumption that if some of the planes is not part of the atomic commit,
> then all its relevant WM values will be zero?
>
>>  
>>  		if (intel_plane->base.type ==
>> DRM_PLANE_TYPE_PRIMARY)
>> -			pristate = to_intel_plane_state(ps);
>> -		else if (intel_plane->base.type ==
>> DRM_PLANE_TYPE_OVERLAY)
>> -			sprstate = to_intel_plane_state(ps);
>> -		else if (intel_plane->base.type ==
>> DRM_PLANE_TYPE_CURSOR)
>> -			curstate = to_intel_plane_state(ps);
>> +			pristate = ps;
>> +		else if (intel_plane->base.type ==
>> DRM_PLANE_TYPE_OVERLAY) {
>> +			sprstate = ps;
>> +
>> +			if (ps) {
>> +				pipe_wm->sprites_enabled = sprstate-
>>> visible;
>> +				pipe_wm->sprites_scaled = sprstate-
>>> visible &&
> You're setting these values here...
>
>> +					(drm_rect_width(&sprstate-
>>> dst) != drm_rect_width(&sprstate->src) >> 16 ||
>> +					drm_rect_height(&sprstate-
>>> dst) != drm_rect_height(&sprstate->src) >> 16);
>> +			}
>> +		} else if (intel_plane->base.type ==
>> DRM_PLANE_TYPE_CURSOR)
> (missing brackets here, but if you follow my suggestion below you won't
> need them)
>
>> +			curstate = ps;
>>  	}
>>  
>> -	config.sprites_enabled = sprstate->visible;
>> -	config.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);
>> +	config.sprites_enabled = pipe_wm->sprites_enabled;
>> +	config.sprites_scaled = pipe_wm->sprites_scaled;
>>  
>>  	pipe_wm->pipe_enabled = cstate->base.active;
>>  	pipe_wm->sprites_enabled = config.sprites_enabled;
>>  	pipe_wm->sprites_scaled = config.sprites_scaled;
> ...but then we re-set them here.
>
> Either you could remove these assignments here, or you could move the
> "if (ps)" from the loop to outside it, becoming "if (sprstate)", making
> the post-patch code similar to the pre-patch code. Since both pipe_wm
> and config are zero-initialized you don't even need to think about the
> !sprstate case.
Makes sense, will do so.
>>  
>>  	/* ILK/SNB: LP2+ watermarks only w/o sprites */
>> -	if (INTEL_INFO(dev)->gen <= 6 && sprstate->visible)
>> +	if (INTEL_INFO(dev)->gen <= 6 && pipe_wm->sprites_enabled)
>>  		max_level = 1;
>>  
>>  	/* ILK/SNB/IVB: LP1+ watermarks only w/o scaling */
>> @@ -2352,20 +2365,18 @@ static int ilk_compute_pipe_wm(struct
>> intel_crtc *intel_crtc,
>>  	ilk_compute_wm_reg_maximums(dev, 1, &max);
>>  
>>  	for (level = 1; level <= max_level; level++) {
>> -		struct intel_wm_level wm = {};
>> +		struct intel_wm_level *wm = &pipe_wm->wm[level];
>>  
>>  		ilk_compute_wm_level(dev_priv, intel_crtc, level,
>> cstate,
>> -				     pristate, sprstate, curstate,
>> &wm);
>> +				     pristate, sprstate, curstate,
>> wm);
>>  
>>  		/*
>>  		 * Disable any watermark level that exceeds the
>>  		 * register maximums since such watermarks are
>>  		 * always invalid.
>>  		 */
>> -		if (!ilk_validate_wm_level(level, &max, &wm))
>> +		if (!ilk_validate_wm_level(level, &max, wm))
>>  			break;
>> -
>> -		pipe_wm->wm[level] = wm;
> The change in the chunk above is that we're now leaving with non-zero
> wm->{pri,spr,cur}_val if some level is invalid. Given the current
> complexity of the code, it's not trivial verify whether nothing later
> is assuming that invalid levels are all-zero, but it looks like we're
> safe. Anyway, could you please move this to a separate patch that comes
> before the other changes? It seems this change would be safe alone, and
> we're seeing problems with WMs these days, so having nice bisectability
> is a huge plus.
>
Well caught, I think we need to calculate even invalid values, but set ->enable = false in that case.
That is the only way we can ensure that the wm levels are calculated correctly.
I've sent those as separate patches, so I get a full CI run from them.

[PATCH 1/2] drm/i915: Allow preservation of watermarks.
[PATCH 2/2] drm/i915: Only recalculate wm's for planes part of the state, v2.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v5 8/8] drm/i915: Remove vblank wait from hsw_enable_ips, v2.

[Maarten Lankhorst]

intel_post_plane_update did an extra vblank wait that's no longer needed when enabling ips.

Changes since v1:
- Add comment explaining why vblank wait is performed. (Paulo)

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
Noticed this one still in my branch, does this look ok?

~Maarten

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index dc920cfc3fd7..6dcc159ce6ac 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4454,8 +4454,11 @@ void hsw_enable_ips(struct intel_crtc *crtc)
 	if (!crtc->config->ips_enabled)
 		return;
 
-	/* We can only enable IPS after we enable a plane and wait for a vblank */
-	intel_wait_for_vblank(dev, crtc->pipe);
+	/*
+	 * We can only enable IPS after we enable a plane and wait for a vblank
+	 * This function is called from post_plane_update, which is run after
+	 * a vblank wait.
+	 */
 
 	assert_plane_enabled(dev_priv, crtc->plane);
 	if (IS_BROADWELL(dev)) {

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

Re: [PATCH v5 8/8] drm/i915: Remove vblank wait from hsw_enable_ips, v2.

[Zanoni, Paulo R]

Em Qua, 2016-03-23 às 14:33 +0100, Maarten Lankhorst escreveu:
> intel_post_plane_update did an extra vblank wait that's no longer
> needed when enabling ips.
> 
> Changes since v1:
> - Add comment explaining why vblank wait is performed. (Paulo)
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
> Noticed this one still in my branch, does this look ok?
> 
> ~Maarten
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index dc920cfc3fd7..6dcc159ce6ac 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4454,8 +4454,11 @@ void hsw_enable_ips(struct intel_crtc *crtc)
>  	if (!crtc->config->ips_enabled)
>  		return;
>  
> -	/* We can only enable IPS after we enable a plane and wait
> for a vblank */
> -	intel_wait_for_vblank(dev, crtc->pipe);
> +	/*
> +	 * We can only enable IPS after we enable a plane and wait
> for a vblank
> +	 * This function is called from post_plane_update, which is
> run after
> +	 * a vblank wait.
> +	 */

There are actually other callers, such as intel_dp.c, but I suppose
they are correct since they can only be called when the planes are
already enabled, so:

Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

>  
>  	assert_plane_enabled(dev_priv, crtc->plane);
>  	if (IS_BROADWELL(dev)) {
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx