[PATCH v3 0/8] Enable Transcoder Port Sync feature for tiled displays

[PATCH v3 0/8] Enable Transcoder Port Sync feature for tiled displays

[Manasi Navare]

First two patches are cleanup patches with no functional changes to
rename the existing update_crtc() hook to commit_modeset_enables() to better
match the drm_atomic naming conventions.
This also prepares to add new hook for ICL since that adds support for
transcoder port sync feature.

Rest of the patches enable transcoder port sync feature starting GEN 11
required to synchronize the two tile sent across two DP SST connectors.

This has been validated using IGT to enable Dell tiled monitor in a 2 pipe
2 port configuration.

Manasi Navare (8):
  drm/i915/display: Rename update_crtcs() to commit_modeset_enables()
  drm/i915/display: Move the commit_tail() disable sequence to
    commit_modeset_disables() hook
  drm/i915/display/icl: Save Master transcoder in slave's crtc_state for
    Transcoder Port Sync
  drm/i915/display/icl: Enable TRANSCODER PORT SYNC for tiled displays
    across separate ports
  drm/i915/display/icl: HW state readout for transcoder port sync config
  drm/i915/display/icl: Enable master-slaves in trans port sync mode in
    correct order
  drm/i915/display/icl: Disable transcoder port sync as part of
    crtc_disable() sequence
  drm/i915/display/icl: In port sync mode disable slaves first then
    masters

 drivers/gpu/drm/i915/display/intel_ddi.c     |   3 +-
 drivers/gpu/drm/i915/display/intel_display.c | 647 +++++++++++++++++--
 drivers/gpu/drm/i915/display/intel_display.h |   5 +
 drivers/gpu/drm/i915/i915_drv.h              |   3 +-
 drivers/gpu/drm/i915/intel_drv.h             |   6 +
 5 files changed, 616 insertions(+), 48 deletions(-)

-- 
2.19.1

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

[PATCH v3 1/8] drm/i915/display: Rename update_crtcs() to commit_modeset_enables()

[Manasi Navare]

This patch has no functional changes. This just renames the update_crtcs()
hooks to commit_modeset_enables() to match the drm_atomic helper naming
conventions.

Suggested-by: Daniel Vetter <daniel.vetter@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Matt Roper <matthew.d.roper@intel.com>
Cc: Jani Nikula <jani.nikula@intel.com>
Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
---
 drivers/gpu/drm/i915/display/intel_display.c | 10 +++++-----
 drivers/gpu/drm/i915/i915_drv.h              |  2 +-
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index 73fe1bcfcd99..71e86e2f0f90 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -13613,7 +13613,7 @@ static void intel_update_crtc(struct drm_crtc *crtc,
 	intel_finish_crtc_commit(to_intel_atomic_state(state), intel_crtc);
 }
 
-static void intel_update_crtcs(struct drm_atomic_state *state)
+static void intel_commit_modeset_enables(struct drm_atomic_state *state)
 {
 	struct drm_crtc *crtc;
 	struct drm_crtc_state *old_crtc_state, *new_crtc_state;
@@ -13628,7 +13628,7 @@ static void intel_update_crtcs(struct drm_atomic_state *state)
 	}
 }
 
-static void skl_update_crtcs(struct drm_atomic_state *state)
+static void skl_commit_modeset_enables(struct drm_atomic_state *state)
 {
 	struct drm_i915_private *dev_priv = to_i915(state->dev);
 	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
@@ -13868,7 +13868,7 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
 	}
 
 	/* Now enable the clocks, plane, pipe, and connectors that we set up. */
-	dev_priv->display.update_crtcs(state);
+	dev_priv->display.commit_modeset_enables(state);
 
 	if (intel_state->modeset)
 		intel_set_cdclk_post_plane_update(dev_priv,
@@ -15719,9 +15719,9 @@ void intel_init_display_hooks(struct drm_i915_private *dev_priv)
 	}
 
 	if (INTEL_GEN(dev_priv) >= 9)
-		dev_priv->display.update_crtcs = skl_update_crtcs;
+		dev_priv->display.commit_modeset_enables = skl_commit_modeset_enables;
 	else
-		dev_priv->display.update_crtcs = intel_update_crtcs;
+		dev_priv->display.commit_modeset_enables = intel_commit_modeset_enables;
 }
 
 static i915_reg_t i915_vgacntrl_reg(struct drm_i915_private *dev_priv)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 62e7c5e8aee5..075d7eb3c3f2 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -310,7 +310,7 @@ struct drm_i915_display_funcs {
 			    struct drm_atomic_state *old_state);
 	void (*crtc_disable)(struct intel_crtc_state *old_crtc_state,
 			     struct drm_atomic_state *old_state);
-	void (*update_crtcs)(struct drm_atomic_state *state);
+	void (*commit_modeset_enables)(struct drm_atomic_state *state);
 	void (*audio_codec_enable)(struct intel_encoder *encoder,
 				   const struct intel_crtc_state *crtc_state,
 				   const struct drm_connector_state *conn_state);
-- 
2.19.1

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

[PATCH v3 2/8] drm/i915/display: Move the commit_tail() disable sequence to commit_modeset_disables() hook

[Manasi Navare]

Create a new hook commit_modeset_disables() consistent with the naming
in drm atomic helpers and similar to the commit_modeset_enables() hook.
This helps better organize the disable sequence in atomic_commit_tail()
and move that to this disable hook.

No functional change

Suggested-by: Daniel Vetter <daniel.vetter@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Matt Roper <matthew.d.roper@intel.com>
Cc: Jani Nikula <jani.nikula@intel.com>
Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
---
 drivers/gpu/drm/i915/display/intel_display.c | 105 +++++++++++--------
 drivers/gpu/drm/i915/i915_drv.h              |   1 +
 2 files changed, 64 insertions(+), 42 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index 71e86e2f0f90..c34118998ace 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -13613,6 +13613,58 @@ static void intel_update_crtc(struct drm_crtc *crtc,
 	intel_finish_crtc_commit(to_intel_atomic_state(state), intel_crtc);
 }
 
+static void intel_commit_modeset_disables(struct drm_atomic_state *state)
+{
+	struct drm_device *dev = state->dev;
+	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
+	struct drm_i915_private *dev_priv = to_i915(dev);
+	struct drm_crtc_state *old_crtc_state, *new_crtc_state;
+	struct intel_crtc_state *new_intel_crtc_state, *old_intel_crtc_state;
+	struct drm_crtc *crtc;
+	struct intel_crtc *intel_crtc;
+	int i;
+
+	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
+		old_intel_crtc_state = to_intel_crtc_state(old_crtc_state);
+		new_intel_crtc_state = to_intel_crtc_state(new_crtc_state);
+		intel_crtc = to_intel_crtc(crtc);
+
+		if (!needs_modeset(new_crtc_state))
+			continue;
+
+		intel_pre_plane_update(old_intel_crtc_state, new_intel_crtc_state);
+
+		if (old_crtc_state->active) {
+			intel_crtc_disable_planes(intel_state, intel_crtc);
+
+			/*
+			 * We need to disable pipe CRC before disabling the pipe,
+			 * or we race against vblank off.
+			 */
+			intel_crtc_disable_pipe_crc(intel_crtc);
+
+			dev_priv->display.crtc_disable(old_intel_crtc_state, state);
+			intel_crtc->active = false;
+			intel_fbc_disable(intel_crtc);
+			intel_disable_shared_dpll(old_intel_crtc_state);
+
+			/*
+			 * Underruns don't always raise interrupts,
+			 * so check manually.
+			 */
+			intel_check_cpu_fifo_underruns(dev_priv);
+			intel_check_pch_fifo_underruns(dev_priv);
+
+			/* FIXME unify this for all platforms */
+			if (!new_crtc_state->active &&
+			    !HAS_GMCH(dev_priv) &&
+			    dev_priv->display.initial_watermarks)
+				dev_priv->display.initial_watermarks(intel_state,
+								     new_intel_crtc_state);
+		}
+	}
+}
+
 static void intel_commit_modeset_enables(struct drm_atomic_state *state)
 {
 	struct drm_crtc *crtc;
@@ -13769,7 +13821,7 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
 	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
 	struct drm_i915_private *dev_priv = to_i915(dev);
 	struct drm_crtc_state *old_crtc_state, *new_crtc_state;
-	struct intel_crtc_state *new_intel_crtc_state, *old_intel_crtc_state;
+	struct intel_crtc_state *new_intel_crtc_state;
 	struct drm_crtc *crtc;
 	struct intel_crtc *intel_crtc;
 	u64 put_domains[I915_MAX_PIPES] = {};
@@ -13778,58 +13830,24 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
 
 	intel_atomic_commit_fence_wait(intel_state);
 
-	drm_atomic_helper_wait_for_dependencies(state);
-
-	if (intel_state->modeset)
-		wakeref = intel_display_power_get(dev_priv, POWER_DOMAIN_MODESET);
-
 	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
-		old_intel_crtc_state = to_intel_crtc_state(old_crtc_state);
 		new_intel_crtc_state = to_intel_crtc_state(new_crtc_state);
 		intel_crtc = to_intel_crtc(crtc);
 
 		if (needs_modeset(new_crtc_state) ||
-		    to_intel_crtc_state(new_crtc_state)->update_pipe) {
-
+		    new_intel_crtc_state->update_pipe) {
 			put_domains[intel_crtc->pipe] =
 				modeset_get_crtc_power_domains(crtc,
-					new_intel_crtc_state);
+							       new_intel_crtc_state);
 		}
+	}
 
-		if (!needs_modeset(new_crtc_state))
-			continue;
-
-		intel_pre_plane_update(old_intel_crtc_state, new_intel_crtc_state);
-
-		if (old_crtc_state->active) {
-			intel_crtc_disable_planes(intel_state, intel_crtc);
-
-			/*
-			 * We need to disable pipe CRC before disabling the pipe,
-			 * or we race against vblank off.
-			 */
-			intel_crtc_disable_pipe_crc(intel_crtc);
-
-			dev_priv->display.crtc_disable(old_intel_crtc_state, state);
-			intel_crtc->active = false;
-			intel_fbc_disable(intel_crtc);
-			intel_disable_shared_dpll(old_intel_crtc_state);
+	drm_atomic_helper_wait_for_dependencies(state);
 
-			/*
-			 * Underruns don't always raise
-			 * interrupts, so check manually.
-			 */
-			intel_check_cpu_fifo_underruns(dev_priv);
-			intel_check_pch_fifo_underruns(dev_priv);
+	if (intel_state->modeset)
+		wakeref = intel_display_power_get(dev_priv, POWER_DOMAIN_MODESET);
 
-			/* FIXME unify this for all platforms */
-			if (!new_crtc_state->active &&
-			    !HAS_GMCH(dev_priv) &&
-			    dev_priv->display.initial_watermarks)
-				dev_priv->display.initial_watermarks(intel_state,
-								     new_intel_crtc_state);
-		}
-	}
+	dev_priv->display.commit_modeset_disables(state);
 
 	/* FIXME: Eventually get rid of our intel_crtc->config pointer */
 	for_each_new_crtc_in_state(state, crtc, new_crtc_state, i)
@@ -15722,6 +15740,9 @@ void intel_init_display_hooks(struct drm_i915_private *dev_priv)
 		dev_priv->display.commit_modeset_enables = skl_commit_modeset_enables;
 	else
 		dev_priv->display.commit_modeset_enables = intel_commit_modeset_enables;
+
+	dev_priv->display.commit_modeset_disables = intel_commit_modeset_disables;
+
 }
 
 static i915_reg_t i915_vgacntrl_reg(struct drm_i915_private *dev_priv)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 075d7eb3c3f2..edb6b431f90c 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -311,6 +311,7 @@ struct drm_i915_display_funcs {
 	void (*crtc_disable)(struct intel_crtc_state *old_crtc_state,
 			     struct drm_atomic_state *old_state);
 	void (*commit_modeset_enables)(struct drm_atomic_state *state);
+	void (*commit_modeset_disables)(struct drm_atomic_state *state);
 	void (*audio_codec_enable)(struct intel_encoder *encoder,
 				   const struct intel_crtc_state *crtc_state,
 				   const struct drm_connector_state *conn_state);
-- 
2.19.1

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

[PATCH v3 3/8] drm/i915/display/icl: Save Master transcoder in slave's crtc_state for Transcoder Port Sync

[Manasi Navare]

In case of tiled displays when the two tiles are sent across two CRTCs
over two separate DP SST connectors, we need a mechanism to synchronize
the two CRTCs and their corresponding transcoders.
So use the master-slave mode where there is one master corresponding
to last horizontal and vertical tile that needs to be genlocked with
all other slave tiles.
This patch identifies saves the master transcoder in all the slave
CRTC states. This is needed to select the master CRTC/transcoder
while configuring transcoder port sync for the corresponding slaves.

v3:
* Use master_tramscoder instead of master_crtc for valid
HW state readouts (Ville)
v2:
* Move this to intel_mode_set_pipe_config(Jani N, Ville)
* Use slave_bitmask to save associated slaves in master crtc state (Ville)

Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Matt Roper <matthew.d.roper@intel.com>
Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
---
 drivers/gpu/drm/i915/display/intel_display.c | 102 +++++++++++++++++++
 drivers/gpu/drm/i915/display/intel_display.h |   1 +
 drivers/gpu/drm/i915/intel_drv.h             |   6 ++
 3 files changed, 109 insertions(+)

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index c34118998ace..367c7fb2be88 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -11600,6 +11600,91 @@ static bool c8_planes_changed(const struct intel_crtc_state *new_crtc_state)
 	return !old_crtc_state->c8_planes != !new_crtc_state->c8_planes;
 }
 
+static int icl_add_sync_mode_crtcs(struct drm_crtc *crtc,
+				   struct intel_crtc_state *crtc_state,
+				   struct drm_atomic_state *state)
+{
+	struct drm_i915_private *dev_priv = to_i915(crtc_state->base.crtc->dev);
+	struct drm_connector *master_connector, *connector;
+	struct drm_connector_state *connector_state;
+	struct drm_connector_list_iter conn_iter;
+	struct drm_crtc *master_crtc = NULL;
+	struct drm_crtc_state *master_crtc_state;
+	struct intel_crtc_state *master_pipe_config;
+	int i, tile_group_id;
+
+	if (INTEL_GEN(dev_priv) < 11)
+		return 0;
+
+	/*
+	 * In case of tiled displays there could be one or more slaves but there is
+	 * only one master. Lets make the CRTC used by the connector corresponding
+	 * to the last horizonal and last vertical tile a master/genlock CRTC.
+	 * All the other CRTCs corresponding to other tiles of the same Tile group
+	 * are the slave CRTCs and hold a pointer to their genlock CRTC.
+	 */
+	for_each_new_connector_in_state(state, connector, connector_state, i) {
+		if (connector_state->crtc != crtc)
+			continue;
+		if (!connector->has_tile)
+			continue;
+		if (crtc_state->base.mode.hdisplay != connector->tile_h_size ||
+		    crtc_state->base.mode.vdisplay != connector->tile_v_size)
+			return 0;
+		if (connector->tile_h_loc == connector->num_h_tile - 1 &&
+		    connector->tile_v_loc == connector->num_v_tile - 1)
+			continue;
+		crtc_state->sync_mode_slaves_mask = 0;
+		tile_group_id = connector->tile_group->id;
+		drm_connector_list_iter_begin(&dev_priv->drm, &conn_iter);
+		drm_for_each_connector_iter(master_connector, &conn_iter) {
+			struct drm_connector_state *master_conn_state = NULL;
+
+			if (!master_connector->has_tile)
+				continue;
+			if (master_connector->tile_h_loc != master_connector->num_h_tile - 1 ||
+			    master_connector->tile_v_loc != master_connector->num_v_tile - 1)
+				continue;
+			if (master_connector->tile_group->id != tile_group_id)
+				continue;
+
+			master_conn_state = drm_atomic_get_connector_state(state,
+									   master_connector);
+			if (IS_ERR(master_conn_state)) {
+				drm_connector_list_iter_end(&conn_iter);
+				return PTR_ERR(master_conn_state);
+			}
+			if (master_conn_state->crtc) {
+				master_crtc = master_conn_state->crtc;
+				break;
+			}
+		}
+		drm_connector_list_iter_end(&conn_iter);
+
+		if (!master_crtc) {
+			DRM_DEBUG_KMS("Could not find Master CRTC for Slave CRTC %d\n",
+				      connector_state->crtc->base.id);
+			return -EINVAL;
+		}
+
+		master_crtc_state = drm_atomic_get_crtc_state(state,
+							      master_crtc);
+		if (IS_ERR(master_crtc_state))
+			return PTR_ERR(master_crtc_state);
+
+		master_pipe_config = to_intel_crtc_state(master_crtc_state);
+		crtc_state->master_transcoder = master_pipe_config->cpu_transcoder;
+		master_pipe_config->sync_mode_slaves_mask |=
+			BIT(crtc_state->cpu_transcoder);
+		DRM_DEBUG_KMS("Master Transcoder = %s added for Slave CRTC = %d, slave transcoder bitmask = %d\n",
+			      transcoder_name(crtc_state->master_transcoder),
+			      crtc_state->base.crtc->base.id,
+			      master_pipe_config->sync_mode_slaves_mask);
+	}
+
+	return 0;
+}
+
 static int intel_crtc_atomic_check(struct drm_crtc *crtc,
 				   struct drm_crtc_state *crtc_state)
 {
@@ -12094,6 +12179,9 @@ clear_intel_crtc_state(struct intel_crtc_state *crtc_state)
 	if (IS_G4X(dev_priv) ||
 	    IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
 		saved_state->wm = crtc_state->wm;
+	if (INTEL_GEN(dev_priv) >= 11)
+		saved_state->sync_mode_slaves_mask =
+			crtc_state->sync_mode_slaves_mask;
 
 	/* Keep base drm_crtc_state intact, only clear our extended struct */
 	BUILD_BUG_ON(offsetof(struct intel_crtc_state, base));
@@ -12187,6 +12275,15 @@ intel_modeset_pipe_config(struct intel_crtc_state *pipe_config)
 	drm_mode_set_crtcinfo(&pipe_config->base.adjusted_mode,
 			      CRTC_STEREO_DOUBLE);
 
+	/* Set the crtc_state defaults for trans_port_sync */
+	pipe_config->master_transcoder = INVALID_TRANSCODER;
+	ret = icl_add_sync_mode_crtcs(crtc, pipe_config, state);
+	if (ret) {
+		DRM_DEBUG_KMS("Cannot assign Sync Mode CRTCs: %d\n",
+			      ret);
+		return ret;
+	}
+
 	/* Pass our mode to the connectors and the CRTC to give them a chance to
 	 * adjust it according to limitations or connector properties, and also
 	 * a chance to reject the mode entirely.
@@ -12676,6 +12773,11 @@ intel_pipe_config_compare(const struct intel_crtc_state *current_config,
 	PIPE_CONF_CHECK_INFOFRAME(hdmi);
 	PIPE_CONF_CHECK_INFOFRAME(drm);
 
+	if (INTEL_GEN(dev_priv) >= 11) {
+		PIPE_CONF_CHECK_I(sync_mode_slaves_mask);
+		PIPE_CONF_CHECK_I(master_transcoder);
+	}
+
 #undef PIPE_CONF_CHECK_X
 #undef PIPE_CONF_CHECK_I
 #undef PIPE_CONF_CHECK_BOOL
diff --git a/drivers/gpu/drm/i915/display/intel_display.h b/drivers/gpu/drm/i915/display/intel_display.h
index ee6b8194a459..40054fbec82c 100644
--- a/drivers/gpu/drm/i915/display/intel_display.h
+++ b/drivers/gpu/drm/i915/display/intel_display.h
@@ -66,6 +66,7 @@ enum pipe {
 #define pipe_name(p) ((p) + 'A')
 
 enum transcoder {
+	INVALID_TRANSCODER = -1,
 	/*
 	 * The following transcoders have a 1:1 transcoder -> pipe mapping,
 	 * keep their values fixed: the code assumes that TRANSCODER_A=0, the
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 1d58f7ec5d84..6e42cf31e5ce 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -946,6 +946,12 @@ struct intel_crtc_state {
 
 	/* Forward Error correction State */
 	bool fec_enable;
+
+	/* Pointer to master transcoder in case of tiled displays */
+	enum transcoder master_transcoder;
+
+	/* Bitmask to indicate slaves attached */
+	u8 sync_mode_slaves_mask;
 };
 
 struct intel_crtc {
-- 
2.19.1

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

[PATCH v3 4/8] drm/i915/display/icl: Enable TRANSCODER PORT SYNC for tiled displays across separate ports

[Manasi Navare]

In case of tiled displays where different tiles are displayed across
different ports, we need to synchronize the transcoders involved.
This patch implements the transcoder port sync feature for
synchronizing one master transcoder with one or more slave
transcoders. This is only enbaled in slave transcoder
and the master transcoder is unaware that it is operating
in this mode.
This has been tested with tiled display connected to ICL.

v3:
* Check of DP_MST moved to atomic_check (Maarten)
v2:
* Do not use RMW, just write to the register in commit (Jani N)

Cc: Daniel Vetter <daniel.vetter@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Matt Roper <matthew.d.roper@intel.com>
Cc: Jani Nikula <jani.nikula@intel.com>
Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
---
 drivers/gpu/drm/i915/display/intel_display.c | 44 ++++++++++++++++++++
 1 file changed, 44 insertions(+)

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index 367c7fb2be88..f359a6212574 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -4366,6 +4366,47 @@ static void icl_set_pipe_chicken(struct intel_crtc *crtc)
 	I915_WRITE(PIPE_CHICKEN(pipe), tmp);
 }
 
+static void icl_enable_trans_port_sync(struct intel_atomic_state *old_intel_state,
+				       const struct intel_crtc_state *crtc_state)
+{
+	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
+	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
+	u32 trans_ddi_func_ctl2_val;
+	u8 master_select;
+
+	/*
+	 * Configure the master select and enable Transcoder Port Sync for
+	 * Slave CRTCs transcoder.
+	 */
+	if (crtc_state->master_transcoder == INVALID_TRANSCODER)
+		return;
+
+	switch (crtc_state->master_transcoder) {
+	case TRANSCODER_A:
+		master_select = 1;
+		break;
+	case TRANSCODER_B:
+		master_select = 2;
+		break;
+	case TRANSCODER_C:
+		master_select = 3;
+		break;
+	case TRANSCODER_EDP:
+	default:
+		master_select = 0;
+		break;
+	}
+	/* Set the master select bits for Tranascoder Port Sync */
+	trans_ddi_func_ctl2_val = (PORT_SYNC_MODE_MASTER_SELECT(master_select) &
+				   PORT_SYNC_MODE_MASTER_SELECT_MASK) <<
+		PORT_SYNC_MODE_MASTER_SELECT_SHIFT;
+	/* Enable Transcoder Port Sync */
+	trans_ddi_func_ctl2_val |= PORT_SYNC_MODE_ENABLE;
+
+	I915_WRITE(TRANS_DDI_FUNC_CTL2(crtc_state->cpu_transcoder),
+		   trans_ddi_func_ctl2_val);
+}
+
 static void intel_update_pipe_config(const struct intel_crtc_state *old_crtc_state,
 				     const struct intel_crtc_state *new_crtc_state)
 {
@@ -6344,6 +6385,9 @@ static void haswell_crtc_enable(struct intel_crtc_state *pipe_config,
 	if (!transcoder_is_dsi(cpu_transcoder))
 		intel_set_pipe_timings(pipe_config);
 
+	if (INTEL_GEN(dev_priv) >= 11)
+		icl_enable_trans_port_sync(old_intel_state, pipe_config);
+
 	intel_set_pipe_src_size(pipe_config);
 
 	if (cpu_transcoder != TRANSCODER_EDP &&
-- 
2.19.1

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

[PATCH v3 5/8] drm/i915/display/icl: HW state readout for transcoder port sync config

[Manasi Navare]

After the state is committed, we readout the HW registers and compare
the HW state with the SW state that we just committed.
For Transcdoer port sync, we add master_transcoder and the
salves bitmask to the crtc_state, hence we need to read those during
the HW state readout to avoid pipe state mismatch.

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Matt Roper <matthew.d.roper@intel.com>
Cc: Jani Nikula <jani.nikula@intel.com>
Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
---
 drivers/gpu/drm/i915/display/intel_display.c | 44 ++++++++++++++++++++
 1 file changed, 44 insertions(+)

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index f359a6212574..7156b1b4c6c5 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -10271,6 +10271,47 @@ static void haswell_get_ddi_port_state(struct intel_crtc *crtc,
 	}
 }
 
+static void icelake_get_trans_port_sync_config(struct intel_crtc *crtc,
+					       struct intel_crtc_state *pipe_config)
+{
+	struct drm_device *dev = crtc->base.dev;
+	struct drm_i915_private *dev_priv = to_i915(dev);
+	u32 trans_port_sync, transcoders, master_select;
+	enum transcoder cpu_transcoder;
+
+	trans_port_sync = I915_READ(TRANS_DDI_FUNC_CTL2(pipe_config->cpu_transcoder));
+	if (trans_port_sync & PORT_SYNC_MODE_ENABLE) {
+		master_select = trans_port_sync &
+			PORT_SYNC_MODE_MASTER_SELECT_MASK;
+		switch (master_select) {
+		case 1:
+			pipe_config->master_transcoder = TRANSCODER_A;
+			break;
+		case 2:
+			pipe_config->master_transcoder = TRANSCODER_B;
+			break;
+		case 3:
+			pipe_config->master_transcoder = TRANSCODER_C;
+			break;
+		default:
+			pipe_config->master_transcoder = TRANSCODER_EDP;
+			break;
+		}
+	} else
+		pipe_config->master_transcoder = INVALID_TRANSCODER;
+
+	transcoders = BIT(TRANSCODER_EDP) |
+		BIT(TRANSCODER_A) |
+		BIT(TRANSCODER_B) |
+		BIT(TRANSCODER_C);
+	for_each_cpu_transcoder_masked(dev_priv, cpu_transcoder, transcoders) {
+		trans_port_sync = I915_READ(TRANS_DDI_FUNC_CTL2(cpu_transcoder));
+
+		if (trans_port_sync & PORT_SYNC_MODE_ENABLE)
+			pipe_config->sync_mode_slaves_mask |= BIT(cpu_transcoder);
+	}
+}
+
 static bool haswell_get_pipe_config(struct intel_crtc *crtc,
 				    struct intel_crtc_state *pipe_config)
 {
@@ -10367,6 +10408,9 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc,
 		pipe_config->pixel_multiplier = 1;
 	}
 
+	if (INTEL_GEN(dev_priv) >= 11)
+		icelake_get_trans_port_sync_config(crtc, pipe_config);
+
 out:
 	for_each_power_domain(power_domain, power_domain_mask)
 		intel_display_power_put(dev_priv,
-- 
2.19.1

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

[PATCH v3 6/8] drm/i915/display/icl: Enable master-slaves in trans port sync mode in correct order

[Manasi Navare]

As per the display enable sequence, we need to follow the enable sequence
for slaves first with DP_TP_CTL set to Idle and configure the transcoder
port sync register to select the corersponding master, then follow the
enable sequence for master leaving DP_TP_CTL to idle.
At this point the transcoder port sync mode is configured and enabled
and the Vblanks of both ports are synchronized so then set DP_TP_CTL
for the slave and master to Normal and do post crtc enable updates.

v2:
* Create a icl_update_crtcs hook (Maarten, Danvet)
* This sequence only for CRTCs in trans port sync mode (Maarten)

Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Matt Roper <matthew.d.roper@intel.com>
Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
---
 drivers/gpu/drm/i915/display/intel_ddi.c     |   3 +-
 drivers/gpu/drm/i915/display/intel_display.c | 217 ++++++++++++++++++-
 drivers/gpu/drm/i915/display/intel_display.h |   4 +
 3 files changed, 221 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
index 7925a176f900..bceb7e4b1877 100644
--- a/drivers/gpu/drm/i915/display/intel_ddi.c
+++ b/drivers/gpu/drm/i915/display/intel_ddi.c
@@ -3154,7 +3154,8 @@ static void intel_ddi_pre_enable_dp(struct intel_encoder *encoder,
 					      true);
 	intel_dp_sink_set_fec_ready(intel_dp, crtc_state);
 	intel_dp_start_link_train(intel_dp);
-	if (port != PORT_A || INTEL_GEN(dev_priv) >= 9)
+	if ((port != PORT_A || INTEL_GEN(dev_priv) >= 9) &&
+	    !is_trans_port_sync_mode(crtc_state))
 		intel_dp_stop_link_train(intel_dp);
 
 	intel_ddi_enable_fec(encoder, crtc_state);
diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index 7156b1b4c6c5..f88d3a929e36 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -520,6 +520,26 @@ needs_modeset(const struct drm_crtc_state *state)
 	return drm_atomic_crtc_needs_modeset(state);
 }
 
+bool
+is_trans_port_sync_mode(const struct intel_crtc_state *state)
+{
+	return (state->master_transcoder != INVALID_TRANSCODER ||
+		state->sync_mode_slaves_mask);
+}
+
+static bool
+is_trans_port_sync_slave(const struct intel_crtc_state *state)
+{
+	return state->master_transcoder != INVALID_TRANSCODER;
+}
+
+static bool
+is_trans_port_sync_master(const struct intel_crtc_state *state)
+{
+	return (state->master_transcoder == INVALID_TRANSCODER &&
+		state->sync_mode_slaves_mask);
+}
+
 /*
  * Platform specific helpers to calculate the port PLL loopback- (clock.m),
  * and post-divider (clock.p) values, pre- (clock.vco) and post-divided fast
@@ -13944,9 +13964,200 @@ static void skl_commit_modeset_enables(struct drm_atomic_state *state)
 			progress = true;
 		}
 	} while (progress);
+}
 
+static void icl_commit_modeset_enables(struct drm_atomic_state *state)
+{
+	struct drm_i915_private *dev_priv = to_i915(state->dev);
+	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
+	struct drm_crtc *crtc;
+	struct intel_crtc *intel_crtc;
+	struct drm_crtc_state *old_crtc_state, *new_crtc_state;
+	struct intel_crtc_state *cstate;
+	unsigned int updated = 0;
+	bool progress;
+	enum pipe pipe;
+	int i;
+	u8 hw_enabled_slices = dev_priv->wm.skl_hw.ddb.enabled_slices;
+	u8 required_slices = intel_state->wm_results.ddb.enabled_slices;
+	struct skl_ddb_entry entries[I915_MAX_PIPES] = {};
+
+	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i)
+		/* ignore allocations for crtc's that have been turned off. */
+		if (new_crtc_state->active)
+			entries[i] = to_intel_crtc_state(old_crtc_state)->wm.skl.ddb;
+
+	/* If 2nd DBuf slice required, enable it here */
+	if (required_slices > hw_enabled_slices)
+		icl_dbuf_slices_update(dev_priv, required_slices);
+
+	/*
+	 * Whenever the number of active pipes changes, we need to make sure we
+	 * update the pipes in the right order so that their ddb allocations
+	 * never overlap with eachother inbetween CRTC updates. Otherwise we'll
+	 * cause pipe underruns and other bad stuff.
+	 */
+	do {
+		progress = false;
+
+		for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
+			bool vbl_wait = false;
+			unsigned int cmask = drm_crtc_mask(crtc);
+			bool modeset = needs_modeset(new_crtc_state);
+
+			intel_crtc = to_intel_crtc(crtc);
+			cstate = to_intel_crtc_state(new_crtc_state);
+			pipe = intel_crtc->pipe;
+
+			if (updated & cmask || !cstate->base.active)
+				continue;
+
+			if (modeset && is_trans_port_sync_mode(cstate)) {
+				DRM_DEBUG_KMS("Pushing the Sync Mode CRTC %d %s that needs update to separate loop\n",
+					      intel_crtc->base.base.id, intel_crtc->base.name);
+				continue;
+			}
+
+			if (skl_ddb_allocation_overlaps(&cstate->wm.skl.ddb,
+							entries,
+							INTEL_INFO(dev_priv)->num_pipes, i))
+				continue;
+
+			updated |= cmask;
+			entries[i] = cstate->wm.skl.ddb;
+
+			/*
+			 * If this is an already active pipe, it's DDB changed,
+			 * and this isn't the last pipe that needs updating
+			 * then we need to wait for a vblank to pass for the
+			 * new ddb allocation to take effect.
+			 */
+			if (!skl_ddb_entry_equal(&cstate->wm.skl.ddb,
+						 &to_intel_crtc_state(old_crtc_state)->wm.skl.ddb) &&
+			    !new_crtc_state->active_changed &&
+			    intel_state->wm_results.dirty_pipes != updated)
+				vbl_wait = true;
+
+			intel_update_crtc(crtc, state, old_crtc_state,
+					  new_crtc_state);
+
+			if (vbl_wait)
+				intel_wait_for_vblank(dev_priv, pipe);
+
+			progress = true;
+		}
+	} while (progress);
+
+	/* Separate loop for updating Slave CRTCs that need modeset.
+	 * We need to loop through all slaves of tiled display first and
+	 * follow enable sequence with DP_TP_CTL left Idle until the master
+	 * is ready.
+	 */
+	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
+		bool modeset = needs_modeset(new_crtc_state);
+		struct intel_crtc_state *pipe_config =
+			to_intel_crtc_state(new_crtc_state);
+
+		intel_crtc = to_intel_crtc(crtc);
+
+		if (!pipe_config->base.active || !modeset ||
+		    !is_trans_port_sync_slave(pipe_config))
+			continue;
+
+		update_scanline_offset(pipe_config);
+		dev_priv->display.crtc_enable(pipe_config, state);
+		intel_crtc_enable_pipe_crc(intel_crtc);
+	}
+	/* Now do the display enable sequence for master CRTC */
+	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
+		bool modeset = needs_modeset(new_crtc_state);
+		struct intel_crtc_state *pipe_config =
+			to_intel_crtc_state(new_crtc_state);
+
+		intel_crtc = to_intel_crtc(crtc);
+
+		if (!pipe_config->base.active || !modeset ||
+		    !is_trans_port_sync_master(pipe_config))
+			continue;
+
+		update_scanline_offset(pipe_config);
+		dev_priv->display.crtc_enable(pipe_config, state);
+		intel_crtc_enable_pipe_crc(intel_crtc);
+	}
+	/* Set Slave's DP_TP_CTL to Normal */
+	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
+		int j;
+		struct drm_connector_state *conn_state;
+		struct drm_connector *conn;
+		bool modeset = needs_modeset(new_crtc_state);
+		struct intel_dp *intel_dp;
+		struct intel_crtc_state *pipe_config =
+			to_intel_crtc_state(new_crtc_state);
+
+		intel_crtc = to_intel_crtc(crtc);
+
+		if (!pipe_config->base.active || !modeset ||
+		    !is_trans_port_sync_slave(pipe_config))
+			continue;
+
+		for_each_new_connector_in_state(state, conn, conn_state, j) {
+			if (conn_state->crtc == crtc)
+				break;
+		}
+		intel_dp = enc_to_intel_dp(&intel_attached_encoder(conn)->base);
+		intel_dp_stop_link_train(intel_dp);
+	}
+	/* Set Master's DP_TP_CTL to Normal */
+	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
+		int j;
+		struct drm_connector_state *conn_state;
+		struct drm_connector *conn;
+		bool modeset = needs_modeset(new_crtc_state);
+		struct intel_dp *intel_dp;
+		struct intel_crtc_state *pipe_config =
+			to_intel_crtc_state(new_crtc_state);
+
+		intel_crtc = to_intel_crtc(crtc);
+
+		if (!pipe_config->base.active || !modeset ||
+		    !is_trans_port_sync_master(pipe_config))
+			continue;
+
+		/* Wait for 200us before setting the master DP_TP_TCL to Normal */
+		usleep_range(200, 400);
+		for_each_new_connector_in_state(state, conn, conn_state, j) {
+			if (conn_state->crtc == crtc)
+				break;
+		}
+		intel_dp = enc_to_intel_dp(&intel_attached_encoder(conn)->base);
+		intel_dp_stop_link_train(intel_dp);
+	}
+	/* Now do the post crtc enable for all master and slaves */
+	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
+		bool modeset = needs_modeset(new_crtc_state);
+		struct intel_plane_state *new_plane_state;
+		struct intel_crtc_state *pipe_config = to_intel_crtc_state(new_crtc_state);
+
+		intel_crtc = to_intel_crtc(crtc);
+
+		if (!pipe_config->base.active || !modeset ||
+		    !is_trans_port_sync_mode(pipe_config))
+			continue;
+
+		new_plane_state =
+			intel_atomic_get_new_plane_state(to_intel_atomic_state(state),
+							 to_intel_plane(crtc->primary));
+		if (pipe_config->update_pipe && !pipe_config->enable_fbc)
+			intel_fbc_disable(intel_crtc);
+		else if (new_plane_state)
+			intel_fbc_enable(intel_crtc, pipe_config, new_plane_state);
+
+		intel_begin_crtc_commit(to_intel_atomic_state(state), intel_crtc);
+		skl_update_planes_on_crtc(to_intel_atomic_state(state), intel_crtc);
+		intel_finish_crtc_commit(to_intel_atomic_state(state), intel_crtc);
+	}
 	/* If 2nd DBuf slice is no more required disable it */
-	if (INTEL_GEN(dev_priv) >= 11 && required_slices < hw_enabled_slices)
+	if (required_slices < hw_enabled_slices)
 		icl_dbuf_slices_update(dev_priv, required_slices);
 }
 
@@ -15926,7 +16137,9 @@ void intel_init_display_hooks(struct drm_i915_private *dev_priv)
 		dev_priv->display.fdi_link_train = hsw_fdi_link_train;
 	}
 
-	if (INTEL_GEN(dev_priv) >= 9)
+	if (INTEL_GEN(dev_priv) >= 11)
+		dev_priv->display.commit_modeset_enables = icl_commit_modeset_enables;
+	else if (INTEL_GEN(dev_priv) >= 9)
 		dev_priv->display.commit_modeset_enables = skl_commit_modeset_enables;
 	else
 		dev_priv->display.commit_modeset_enables = intel_commit_modeset_enables;
diff --git a/drivers/gpu/drm/i915/display/intel_display.h b/drivers/gpu/drm/i915/display/intel_display.h
index 40054fbec82c..29f6c4c91d53 100644
--- a/drivers/gpu/drm/i915/display/intel_display.h
+++ b/drivers/gpu/drm/i915/display/intel_display.h
@@ -28,8 +28,11 @@
 #include <drm/drm_util.h>
 #include <drm/i915_drm.h>
 
+#include "intel_dp_link_training.h"
+
 struct drm_i915_private;
 struct intel_plane_state;
+struct intel_crtc_state;
 
 enum i915_gpio {
 	GPIOA,
@@ -358,5 +361,6 @@ void lpt_disable_clkout_dp(struct drm_i915_private *dev_priv);
 u32 intel_plane_fb_max_stride(struct drm_i915_private *dev_priv,
 			      u32 pixel_format, u64 modifier);
 bool intel_plane_can_remap(const struct intel_plane_state *plane_state);
+bool is_trans_port_sync_mode(const struct intel_crtc_state *state);
 
 #endif
-- 
2.19.1

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

[PATCH v3 7/8] drm/i915/display/icl: Disable transcoder port sync as part of crtc_disable() sequence

[Manasi Navare]

This clears the transcoder port sync bits of the TRANS_DDI_FUNC_CTL2
register during crtc_disable().

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Matt Roper <matthew.d.roper@intel.com>
Cc: Jani Nikula <jani.nikula@intel.com>
Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
---
 drivers/gpu/drm/i915/display/intel_display.c | 44 ++++++++++++++++----
 1 file changed, 35 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index f88d3a929e36..0a0d97ef03d6 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -4427,6 +4427,26 @@ static void icl_enable_trans_port_sync(struct intel_atomic_state *old_intel_stat
 		   trans_ddi_func_ctl2_val);
 }
 
+static void icl_disable_transcoder_port_sync(const struct intel_crtc_state *old_crtc_state)
+{
+	struct intel_crtc *crtc = to_intel_crtc(old_crtc_state->base.crtc);
+	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
+	i915_reg_t reg;
+	u32 trans_ddi_func_ctl2_val;
+
+	if (old_crtc_state->master_transcoder == INVALID_TRANSCODER)
+		return;
+
+	DRM_DEBUG_KMS("Disabling Transcoder Port Sync on Slave Transcoder %s\n",
+		      transcoder_name(old_crtc_state->cpu_transcoder));
+
+	reg = TRANS_DDI_FUNC_CTL2(old_crtc_state->cpu_transcoder);
+	trans_ddi_func_ctl2_val = I915_READ(reg);
+	trans_ddi_func_ctl2_val &= ~(PORT_SYNC_MODE_ENABLE |
+				     PORT_SYNC_MODE_MASTER_SELECT_MASK);
+	I915_WRITE(reg, trans_ddi_func_ctl2_val);
+}
+
 static void intel_update_pipe_config(const struct intel_crtc_state *old_crtc_state,
 				     const struct intel_crtc_state *new_crtc_state)
 {
@@ -6586,6 +6606,9 @@ static void haswell_crtc_disable(struct intel_crtc_state *old_crtc_state,
 	if (intel_crtc_has_type(old_crtc_state, INTEL_OUTPUT_DP_MST))
 		intel_ddi_set_vc_payload_alloc(old_crtc_state, false);
 
+	if (INTEL_GEN(dev_priv) >= 11)
+		icl_disable_transcoder_port_sync(old_crtc_state);
+
 	if (!transcoder_is_dsi(cpu_transcoder))
 		intel_ddi_disable_transcoder_func(old_crtc_state);
 
@@ -10317,18 +10340,21 @@ static void icelake_get_trans_port_sync_config(struct intel_crtc *crtc,
 			pipe_config->master_transcoder = TRANSCODER_EDP;
 			break;
 		}
-	} else
+
+		pipe_config->sync_mode_slaves_mask = 0;
+	} else {
 		pipe_config->master_transcoder = INVALID_TRANSCODER;
 
-	transcoders = BIT(TRANSCODER_EDP) |
-		BIT(TRANSCODER_A) |
-		BIT(TRANSCODER_B) |
-		BIT(TRANSCODER_C);
-	for_each_cpu_transcoder_masked(dev_priv, cpu_transcoder, transcoders) {
-		trans_port_sync = I915_READ(TRANS_DDI_FUNC_CTL2(cpu_transcoder));
+		transcoders = BIT(TRANSCODER_EDP) |
+			BIT(TRANSCODER_A) |
+			BIT(TRANSCODER_B) |
+			BIT(TRANSCODER_C);
+		for_each_cpu_transcoder_masked(dev_priv, cpu_transcoder, transcoders) {
+			trans_port_sync = I915_READ(TRANS_DDI_FUNC_CTL2(cpu_transcoder));
 
-		if (trans_port_sync & PORT_SYNC_MODE_ENABLE)
-			pipe_config->sync_mode_slaves_mask |= BIT(cpu_transcoder);
+			if (trans_port_sync & PORT_SYNC_MODE_ENABLE)
+				pipe_config->sync_mode_slaves_mask |= BIT(cpu_transcoder);
+		}
 	}
 }
 
-- 
2.19.1

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

[PATCH v3 8/8] drm/i915/display/icl: In port sync mode disable slaves first then masters

[Manasi Navare]

In the transcoder port sync mode, the slave transcoders mask their vblanks
until master transcoder's vblank so while disabling them, make
sure slaves are disabled first and then the masters.

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Matt Roper <matthew.d.roper@intel.com>
Cc: Jani Nikula <jani.nikula@intel.com>
Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
---
 drivers/gpu/drm/i915/display/intel_display.c | 117 ++++++++++++++++++-
 1 file changed, 111 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index 0a0d97ef03d6..85746a26d0e0 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -13901,6 +13901,106 @@ static void intel_commit_modeset_disables(struct drm_atomic_state *state)
 	}
 }
 
+static void icl_commit_modeset_disables(struct drm_atomic_state *state)
+{
+	struct drm_device *dev = state->dev;
+	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
+	struct drm_i915_private *dev_priv = to_i915(dev);
+	struct drm_crtc_state *old_crtc_state, *new_crtc_state;
+	struct intel_crtc_state *new_intel_crtc_state, *old_intel_crtc_state;
+	struct drm_crtc *crtc;
+	struct intel_crtc *intel_crtc;
+	int i;
+
+	/*
+	 * Disable all the Port Sync Slaves first
+	 */
+	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
+		old_intel_crtc_state = to_intel_crtc_state(old_crtc_state);
+		new_intel_crtc_state = to_intel_crtc_state(new_crtc_state);
+		intel_crtc = to_intel_crtc(crtc);
+
+		if (!needs_modeset(new_crtc_state) ||
+		    !is_trans_port_sync_slave(old_intel_crtc_state))
+			continue;
+
+		intel_pre_plane_update(old_intel_crtc_state, new_intel_crtc_state);
+
+		if (old_crtc_state->active) {
+			intel_crtc_disable_planes(intel_state, intel_crtc);
+
+			/*
+			 * We need to disable pipe CRC before disabling the pipe,
+			 * or we race against vblank off.
+			 */
+			intel_crtc_disable_pipe_crc(intel_crtc);
+
+			dev_priv->display.crtc_disable(old_intel_crtc_state, state);
+			intel_crtc->active = false;
+			intel_fbc_disable(intel_crtc);
+			intel_disable_shared_dpll(old_intel_crtc_state);
+
+			/*
+			 * Underruns don't always raise interrupts,
+			 * so check manually.
+			 */
+			intel_check_cpu_fifo_underruns(dev_priv);
+			intel_check_pch_fifo_underruns(dev_priv);
+
+			/* FIXME unify this for all platforms */
+			if (!new_crtc_state->active &&
+			    !HAS_GMCH(dev_priv) &&
+			    dev_priv->display.initial_watermarks)
+				dev_priv->display.initial_watermarks(intel_state,
+								     new_intel_crtc_state);
+		}
+	}
+
+	/*
+	 * Disable rest of the CRTCs other than slaves
+	 */
+	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
+		old_intel_crtc_state = to_intel_crtc_state(old_crtc_state);
+		new_intel_crtc_state = to_intel_crtc_state(new_crtc_state);
+		intel_crtc = to_intel_crtc(crtc);
+
+		if (!needs_modeset(new_crtc_state) ||
+		    is_trans_port_sync_slave(old_intel_crtc_state))
+			continue;
+
+		intel_pre_plane_update(old_intel_crtc_state, new_intel_crtc_state);
+
+		if (old_crtc_state->active) {
+			intel_crtc_disable_planes(intel_state, intel_crtc);
+
+			/*
+			 * We need to disable pipe CRC before disabling the pipe,
+			 * or we race against vblank off.
+			 */
+			intel_crtc_disable_pipe_crc(intel_crtc);
+
+			dev_priv->display.crtc_disable(old_intel_crtc_state, state);
+			intel_crtc->active = false;
+			intel_fbc_disable(intel_crtc);
+			intel_disable_shared_dpll(old_intel_crtc_state);
+
+			/*
+			 * Underruns don't always raise interrupts,
+			 * so check manually.
+			 */
+			intel_check_cpu_fifo_underruns(dev_priv);
+			intel_check_pch_fifo_underruns(dev_priv);
+
+			/* FIXME unify this for all platforms */
+			if (!new_crtc_state->active &&
+			    !HAS_GMCH(dev_priv) &&
+			    dev_priv->display.initial_watermarks)
+				dev_priv->display.initial_watermarks(intel_state,
+								     new_intel_crtc_state);
+		}
+	}
+}
+
 static void intel_commit_modeset_enables(struct drm_atomic_state *state)
 {
 	struct drm_crtc *crtc;
@@ -14257,6 +14357,11 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
 
 	intel_atomic_commit_fence_wait(intel_state);
 
+	drm_atomic_helper_wait_for_dependencies(state);
+
+	if (intel_state->modeset)
+		wakeref = intel_display_power_get(dev_priv, POWER_DOMAIN_MODESET);
+
 	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
 		new_intel_crtc_state = to_intel_crtc_state(new_crtc_state);
 		intel_crtc = to_intel_crtc(crtc);
@@ -14269,11 +14374,6 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
 		}
 	}
 
-	drm_atomic_helper_wait_for_dependencies(state);
-
-	if (intel_state->modeset)
-		wakeref = intel_display_power_get(dev_priv, POWER_DOMAIN_MODESET);
-
 	dev_priv->display.commit_modeset_disables(state);
 
 	/* FIXME: Eventually get rid of our intel_crtc->config pointer */
@@ -16170,7 +16270,12 @@ void intel_init_display_hooks(struct drm_i915_private *dev_priv)
 	else
 		dev_priv->display.commit_modeset_enables = intel_commit_modeset_enables;
 
-	dev_priv->display.commit_modeset_disables = intel_commit_modeset_disables;
+	if (INTEL_GEN(dev_priv) >= 11)
+		dev_priv->display.commit_modeset_disables =
+			icl_commit_modeset_disables;
+	else
+		dev_priv->display.commit_modeset_disables =
+			intel_commit_modeset_disables;
 
 }
 
-- 
2.19.1

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

✗ Fi.CI.CHECKPATCH: warning for Enable Transcoder Port Sync feature for Tiled displays (rev2)

[Patchwork]

== Series Details ==

Series: Enable Transcoder Port Sync feature for Tiled displays (rev2)
URL   : https://patchwork.freedesktop.org/series/59837/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
e1dacdba0522 drm/i915/display: Rename update_crtcs() to commit_modeset_enables()
86c171d8590a drm/i915/display: Move the commit_tail() disable sequence to commit_modeset_disables() hook
f428b4bebc07 drm/i915/display/icl: Save Master transcoder in slave's crtc_state for Transcoder Port Sync
2fedf54378a8 drm/i915/display/icl: Enable TRANSCODER PORT SYNC for tiled displays across separate ports
105509334ec9 drm/i915/display/icl: HW state readout for transcoder port sync config
-:39: CHECK:BRACES: braces {} should be used on all arms of this statement
#39: FILE: drivers/gpu/drm/i915/display/intel_display.c:10283:
+	if (trans_port_sync & PORT_SYNC_MODE_ENABLE) {
[...]
+	} else
[...]

-:56: CHECK:BRACES: Unbalanced braces around else statement
#56: FILE: drivers/gpu/drm/i915/display/intel_display.c:10300:
+	} else

total: 0 errors, 0 warnings, 2 checks, 56 lines checked
cfc8af932092 drm/i915/display/icl: Enable master-slaves in trans port sync mode in correct order
-:146: WARNING:LONG_LINE: line over 100 characters
#146: FILE: drivers/gpu/drm/i915/display/intel_display.c:14036:
+						 &to_intel_crtc_state(old_crtc_state)->wm.skl.ddb) &&

total: 0 errors, 1 warnings, 0 checks, 263 lines checked
86a0ab130772 drm/i915/display/icl: Disable transcoder port sync as part of crtc_disable() sequence
6aa445344e23 drm/i915/display/icl: In port sync mode disable slaves first then masters

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

✗ Fi.CI.BAT: failure for Enable Transcoder Port Sync feature for Tiled displays (rev2)

[Patchwork]

== Series Details ==

Series: Enable Transcoder Port Sync feature for Tiled displays (rev2)
URL   : https://patchwork.freedesktop.org/series/59837/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_6337 -> Patchwork_13409
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_13409 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_13409, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13409/

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in Patchwork_13409:

### IGT changes ###

#### Possible regressions ####

  * igt@kms_busy@basic-flip-a:
    - fi-icl-u3:          [PASS][1] -> [DMESG-WARN][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6337/fi-icl-u3/igt@kms_busy@basic-flip-a.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13409/fi-icl-u3/igt@kms_busy@basic-flip-a.html
    - fi-icl-u2:          [PASS][3] -> [DMESG-WARN][4]
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6337/fi-icl-u2/igt@kms_busy@basic-flip-a.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13409/fi-icl-u2/igt@kms_busy@basic-flip-a.html

  
Known issues
------------

  Here are the changes found in Patchwork_13409 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@i915_module_load@reload-with-fault-injection:
    - fi-bsw-kefka:       [PASS][5] -> [DMESG-WARN][6] ([fdo#110900])
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6337/fi-bsw-kefka/igt@i915_module_load@reload-with-fault-injection.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13409/fi-bsw-kefka/igt@i915_module_load@reload-with-fault-injection.html

  * igt@kms_chamelium@hdmi-hpd-fast:
    - fi-kbl-7500u:       [PASS][7] -> [FAIL][8] ([fdo#109485])
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6337/fi-kbl-7500u/igt@kms_chamelium@hdmi-hpd-fast.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13409/fi-kbl-7500u/igt@kms_chamelium@hdmi-hpd-fast.html

  
#### Possible fixes ####

  * igt@i915_selftest@live_blt:
    - fi-skl-iommu:       [INCOMPLETE][9] ([fdo#108602]) -> [PASS][10]
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6337/fi-skl-iommu/igt@i915_selftest@live_blt.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13409/fi-skl-iommu/igt@i915_selftest@live_blt.html

  * igt@kms_busy@basic-flip-c:
    - fi-kbl-7500u:       [SKIP][11] ([fdo#109271] / [fdo#109278]) -> [PASS][12] +2 similar issues
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6337/fi-kbl-7500u/igt@kms_busy@basic-flip-c.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13409/fi-kbl-7500u/igt@kms_busy@basic-flip-c.html

  * igt@kms_chamelium@dp-hpd-fast:
    - fi-kbl-7500u:       [DMESG-WARN][13] ([fdo#103558] / [fdo#105602]) -> [PASS][14]
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6337/fi-kbl-7500u/igt@kms_chamelium@dp-hpd-fast.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13409/fi-kbl-7500u/igt@kms_chamelium@dp-hpd-fast.html

  
  [fdo#103558]: https://bugs.freedesktop.org/show_bug.cgi?id=103558
  [fdo#105602]: https://bugs.freedesktop.org/show_bug.cgi?id=105602
  [fdo#108602]: https://bugs.freedesktop.org/show_bug.cgi?id=108602
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109278]: https://bugs.freedesktop.org/show_bug.cgi?id=109278
  [fdo#109485]: https://bugs.freedesktop.org/show_bug.cgi?id=109485
  [fdo#110900]: https://bugs.freedesktop.org/show_bug.cgi?id=110900


Participating hosts (50 -> 43)
------------------------------

  Additional (1): fi-gdg-551 
  Missing    (8): fi-kbl-soraka fi-ilk-m540 fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-icl-y fi-byt-clapper fi-bdw-samus 


Build changes
-------------

  * Linux: CI_DRM_6337 -> Patchwork_13409

  CI_DRM_6337: 294d5056c1f3fc839b6c3bbd2aa6f451ac316991 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_5066: a6f5cc854efb4b7dfed7f0a2c1039a9ddd1a35a5 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_13409: 6aa445344e2356d77d86e93fdb1d79f6d2673232 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

6aa445344e23 drm/i915/display/icl: In port sync mode disable slaves first then masters
86a0ab130772 drm/i915/display/icl: Disable transcoder port sync as part of crtc_disable() sequence
cfc8af932092 drm/i915/display/icl: Enable master-slaves in trans port sync mode in correct order
105509334ec9 drm/i915/display/icl: HW state readout for transcoder port sync config
2fedf54378a8 drm/i915/display/icl: Enable TRANSCODER PORT SYNC for tiled displays across separate ports
f428b4bebc07 drm/i915/display/icl: Save Master transcoder in slave's crtc_state for Transcoder Port Sync
86c171d8590a drm/i915/display: Move the commit_tail() disable sequence to commit_modeset_disables() hook
e1dacdba0522 drm/i915/display: Rename update_crtcs() to commit_modeset_enables()

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13409/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 8/8] drm/i915/display/icl: In port sync mode disable slaves first then masters

[Lucas De Marchi]

On Mon, Jun 24, 2019 at 2:07 PM Manasi Navare <manasi.d.navare@intel.com> wrote:
>
> In the transcoder port sync mode, the slave transcoders mask their vblanks
> until master transcoder's vblank so while disabling them, make
> sure slaves are disabled first and then the masters.
>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Matt Roper <matthew.d.roper@intel.com>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_display.c | 117 ++++++++++++++++++-
>  1 file changed, 111 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index 0a0d97ef03d6..85746a26d0e0 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -13901,6 +13901,106 @@ static void intel_commit_modeset_disables(struct drm_atomic_state *state)
>         }
>  }
>
> +static void icl_commit_modeset_disables(struct drm_atomic_state *state)
> +{
> +       struct drm_device *dev = state->dev;
> +       struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
> +       struct drm_i915_private *dev_priv = to_i915(dev);
> +       struct drm_crtc_state *old_crtc_state, *new_crtc_state;
> +       struct intel_crtc_state *new_intel_crtc_state, *old_intel_crtc_state;
> +       struct drm_crtc *crtc;
> +       struct intel_crtc *intel_crtc;
> +       int i;
> +
> +       /*
> +        * Disable all the Port Sync Slaves first
> +        */
> +       for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
> +               old_intel_crtc_state = to_intel_crtc_state(old_crtc_state);
> +               new_intel_crtc_state = to_intel_crtc_state(new_crtc_state);
> +               intel_crtc = to_intel_crtc(crtc);
> +
> +               if (!needs_modeset(new_crtc_state) ||
> +                   !is_trans_port_sync_slave(old_intel_crtc_state))
> +                       continue;
> +
> +               intel_pre_plane_update(old_intel_crtc_state, new_intel_crtc_state);
> +
> +               if (old_crtc_state->active) {
> +                       intel_crtc_disable_planes(intel_state, intel_crtc);
> +
> +                       /*
> +                        * We need to disable pipe CRC before disabling the pipe,
> +                        * or we race against vblank off.
> +                        */
> +                       intel_crtc_disable_pipe_crc(intel_crtc);
> +
> +                       dev_priv->display.crtc_disable(old_intel_crtc_state, state);
> +                       intel_crtc->active = false;
> +                       intel_fbc_disable(intel_crtc);
> +                       intel_disable_shared_dpll(old_intel_crtc_state);
> +
> +                       /*
> +                        * Underruns don't always raise interrupts,
> +                        * so check manually.
> +                        */
> +                       intel_check_cpu_fifo_underruns(dev_priv);
> +                       intel_check_pch_fifo_underruns(dev_priv);
> +
> +                       /* FIXME unify this for all platforms */
> +                       if (!new_crtc_state->active &&
> +                           !HAS_GMCH(dev_priv) &&
> +                           dev_priv->display.initial_watermarks)
> +                               dev_priv->display.initial_watermarks(intel_state,
> +                                                                    new_intel_crtc_state);
> +               }
> +       }
> +
> +       /*
> +        * Disable rest of the CRTCs other than slaves
> +        */
> +       for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
> +               old_intel_crtc_state = to_intel_crtc_state(old_crtc_state);
> +               new_intel_crtc_state = to_intel_crtc_state(new_crtc_state);
> +               intel_crtc = to_intel_crtc(crtc);
> +
> +               if (!needs_modeset(new_crtc_state) ||
> +                   is_trans_port_sync_slave(old_intel_crtc_state))
> +                       continue;
> +
> +               intel_pre_plane_update(old_intel_crtc_state, new_intel_crtc_state);
> +
> +               if (old_crtc_state->active) {
> +                       intel_crtc_disable_planes(intel_state, intel_crtc);
> +
> +                       /*
> +                        * We need to disable pipe CRC before disabling the pipe,
> +                        * or we race against vblank off.
> +                        */
> +                       intel_crtc_disable_pipe_crc(intel_crtc);
> +
> +                       dev_priv->display.crtc_disable(old_intel_crtc_state, state);
> +                       intel_crtc->active = false;
> +                       intel_fbc_disable(intel_crtc);
> +                       intel_disable_shared_dpll(old_intel_crtc_state);
> +
> +                       /*
> +                        * Underruns don't always raise interrupts,
> +                        * so check manually.
> +                        */
> +                       intel_check_cpu_fifo_underruns(dev_priv);
> +                       intel_check_pch_fifo_underruns(dev_priv);
> +
> +                       /* FIXME unify this for all platforms */
> +                       if (!new_crtc_state->active &&
> +                           !HAS_GMCH(dev_priv) &&
> +                           dev_priv->display.initial_watermarks)
> +                               dev_priv->display.initial_watermarks(intel_state,
> +                                                                    new_intel_crtc_state);
> +               }
> +       }

Not a fan of this duplicate code here. At least a helper function
would be needed.

Jose, didn't you have a pending patch to disable in reverse order to
solve this same problem?

Lucas De Marchi

> +}
> +
>  static void intel_commit_modeset_enables(struct drm_atomic_state *state)
>  {
>         struct drm_crtc *crtc;
> @@ -14257,6 +14357,11 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
>
>         intel_atomic_commit_fence_wait(intel_state);
>
> +       drm_atomic_helper_wait_for_dependencies(state);
> +
> +       if (intel_state->modeset)
> +               wakeref = intel_display_power_get(dev_priv, POWER_DOMAIN_MODESET);
> +
>         for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
>                 new_intel_crtc_state = to_intel_crtc_state(new_crtc_state);
>                 intel_crtc = to_intel_crtc(crtc);
> @@ -14269,11 +14374,6 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
>                 }
>         }
>
> -       drm_atomic_helper_wait_for_dependencies(state);
> -
> -       if (intel_state->modeset)
> -               wakeref = intel_display_power_get(dev_priv, POWER_DOMAIN_MODESET);
> -
>         dev_priv->display.commit_modeset_disables(state);
>
>         /* FIXME: Eventually get rid of our intel_crtc->config pointer */
> @@ -16170,7 +16270,12 @@ void intel_init_display_hooks(struct drm_i915_private *dev_priv)
>         else
>                 dev_priv->display.commit_modeset_enables = intel_commit_modeset_enables;
>
> -       dev_priv->display.commit_modeset_disables = intel_commit_modeset_disables;
> +       if (INTEL_GEN(dev_priv) >= 11)
> +               dev_priv->display.commit_modeset_disables =
> +                       icl_commit_modeset_disables;
> +       else
> +               dev_priv->display.commit_modeset_disables =
> +                       intel_commit_modeset_disables;
>
>  }
>
> --
> 2.19.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx



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

Re: [PATCH v3 8/8] drm/i915/display/icl: In port sync mode disable slaves first then masters

[Manasi Navare]

On Mon, Jun 24, 2019 at 11:34:42PM -0700, Lucas De Marchi wrote:
> On Mon, Jun 24, 2019 at 2:07 PM Manasi Navare <manasi.d.navare@intel.com> wrote:
> >
> > In the transcoder port sync mode, the slave transcoders mask their vblanks
> > until master transcoder's vblank so while disabling them, make
> > sure slaves are disabled first and then the masters.
> >
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Cc: Matt Roper <matthew.d.roper@intel.com>
> > Cc: Jani Nikula <jani.nikula@intel.com>
> > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_display.c | 117 ++++++++++++++++++-
> >  1 file changed, 111 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > index 0a0d97ef03d6..85746a26d0e0 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -13901,6 +13901,106 @@ static void intel_commit_modeset_disables(struct drm_atomic_state *state)
> >         }
> >  }
> >
> > +static void icl_commit_modeset_disables(struct drm_atomic_state *state)
> > +{
> > +       struct drm_device *dev = state->dev;
> > +       struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
> > +       struct drm_i915_private *dev_priv = to_i915(dev);
> > +       struct drm_crtc_state *old_crtc_state, *new_crtc_state;
> > +       struct intel_crtc_state *new_intel_crtc_state, *old_intel_crtc_state;
> > +       struct drm_crtc *crtc;
> > +       struct intel_crtc *intel_crtc;
> > +       int i;
> > +
> > +       /*
> > +        * Disable all the Port Sync Slaves first
> > +        */
> > +       for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
> > +               old_intel_crtc_state = to_intel_crtc_state(old_crtc_state);
> > +               new_intel_crtc_state = to_intel_crtc_state(new_crtc_state);
> > +               intel_crtc = to_intel_crtc(crtc);
> > +
> > +               if (!needs_modeset(new_crtc_state) ||
> > +                   !is_trans_port_sync_slave(old_intel_crtc_state))
> > +                       continue;
> > +
> > +               intel_pre_plane_update(old_intel_crtc_state, new_intel_crtc_state);
> > +
> > +               if (old_crtc_state->active) {
> > +                       intel_crtc_disable_planes(intel_state, intel_crtc);
> > +
> > +                       /*
> > +                        * We need to disable pipe CRC before disabling the pipe,
> > +                        * or we race against vblank off.
> > +                        */
> > +                       intel_crtc_disable_pipe_crc(intel_crtc);
> > +
> > +                       dev_priv->display.crtc_disable(old_intel_crtc_state, state);
> > +                       intel_crtc->active = false;
> > +                       intel_fbc_disable(intel_crtc);
> > +                       intel_disable_shared_dpll(old_intel_crtc_state);
> > +
> > +                       /*
> > +                        * Underruns don't always raise interrupts,
> > +                        * so check manually.
> > +                        */
> > +                       intel_check_cpu_fifo_underruns(dev_priv);
> > +                       intel_check_pch_fifo_underruns(dev_priv);
> > +
> > +                       /* FIXME unify this for all platforms */
> > +                       if (!new_crtc_state->active &&
> > +                           !HAS_GMCH(dev_priv) &&
> > +                           dev_priv->display.initial_watermarks)
> > +                               dev_priv->display.initial_watermarks(intel_state,
> > +                                                                    new_intel_crtc_state);
> > +               }
> > +       }
> > +
> > +       /*
> > +        * Disable rest of the CRTCs other than slaves
> > +        */
> > +       for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
> > +               old_intel_crtc_state = to_intel_crtc_state(old_crtc_state);
> > +               new_intel_crtc_state = to_intel_crtc_state(new_crtc_state);
> > +               intel_crtc = to_intel_crtc(crtc);
> > +
> > +               if (!needs_modeset(new_crtc_state) ||
> > +                   is_trans_port_sync_slave(old_intel_crtc_state))
> > +                       continue;
> > +
> > +               intel_pre_plane_update(old_intel_crtc_state, new_intel_crtc_state);
> > +
> > +               if (old_crtc_state->active) {
> > +                       intel_crtc_disable_planes(intel_state, intel_crtc);
> > +
> > +                       /*
> > +                        * We need to disable pipe CRC before disabling the pipe,
> > +                        * or we race against vblank off.
> > +                        */
> > +                       intel_crtc_disable_pipe_crc(intel_crtc);
> > +
> > +                       dev_priv->display.crtc_disable(old_intel_crtc_state, state);
> > +                       intel_crtc->active = false;
> > +                       intel_fbc_disable(intel_crtc);
> > +                       intel_disable_shared_dpll(old_intel_crtc_state);
> > +
> > +                       /*
> > +                        * Underruns don't always raise interrupts,
> > +                        * so check manually.
> > +                        */
> > +                       intel_check_cpu_fifo_underruns(dev_priv);
> > +                       intel_check_pch_fifo_underruns(dev_priv);
> > +
> > +                       /* FIXME unify this for all platforms */
> > +                       if (!new_crtc_state->active &&
> > +                           !HAS_GMCH(dev_priv) &&
> > +                           dev_priv->display.initial_watermarks)
> > +                               dev_priv->display.initial_watermarks(intel_state,
> > +                                                                    new_intel_crtc_state);
> > +               }
> > +       }
> 
> Not a fan of this duplicate code here. At least a helper function
> would be needed.

Hmm yea creating a helper is a good idea, will do that

> 
> Jose, didn't you have a pending patch to disable in reverse order to
> solve this same problem?

This is not necessarily in the reverse order, it just does it for all slaves
first and then all masters

Manasi

> 
> Lucas De Marchi
> 
> > +}
> > +
> >  static void intel_commit_modeset_enables(struct drm_atomic_state *state)
> >  {
> >         struct drm_crtc *crtc;
> > @@ -14257,6 +14357,11 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
> >
> >         intel_atomic_commit_fence_wait(intel_state);
> >
> > +       drm_atomic_helper_wait_for_dependencies(state);
> > +
> > +       if (intel_state->modeset)
> > +               wakeref = intel_display_power_get(dev_priv, POWER_DOMAIN_MODESET);
> > +
> >         for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
> >                 new_intel_crtc_state = to_intel_crtc_state(new_crtc_state);
> >                 intel_crtc = to_intel_crtc(crtc);
> > @@ -14269,11 +14374,6 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
> >                 }
> >         }
> >
> > -       drm_atomic_helper_wait_for_dependencies(state);
> > -
> > -       if (intel_state->modeset)
> > -               wakeref = intel_display_power_get(dev_priv, POWER_DOMAIN_MODESET);
> > -
> >         dev_priv->display.commit_modeset_disables(state);
> >
> >         /* FIXME: Eventually get rid of our intel_crtc->config pointer */
> > @@ -16170,7 +16270,12 @@ void intel_init_display_hooks(struct drm_i915_private *dev_priv)
> >         else
> >                 dev_priv->display.commit_modeset_enables = intel_commit_modeset_enables;
> >
> > -       dev_priv->display.commit_modeset_disables = intel_commit_modeset_disables;
> > +       if (INTEL_GEN(dev_priv) >= 11)
> > +               dev_priv->display.commit_modeset_disables =
> > +                       icl_commit_modeset_disables;
> > +       else
> > +               dev_priv->display.commit_modeset_disables =
> > +                       intel_commit_modeset_disables;
> >
> >  }
> >
> > --
> > 2.19.1
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> 
> -- 
> Lucas De Marchi
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 8/8] drm/i915/display/icl: In port sync mode disable slaves first then masters

[Manasi Navare]

On Mon, Jun 24, 2019 at 11:34:42PM -0700, Lucas De Marchi wrote:
> On Mon, Jun 24, 2019 at 2:07 PM Manasi Navare <manasi.d.navare@intel.com> wrote:
> >
> > In the transcoder port sync mode, the slave transcoders mask their vblanks
> > until master transcoder's vblank so while disabling them, make
> > sure slaves are disabled first and then the masters.
> >
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Cc: Matt Roper <matthew.d.roper@intel.com>
> > Cc: Jani Nikula <jani.nikula@intel.com>
> > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_display.c | 117 ++++++++++++++++++-
> >  1 file changed, 111 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > index 0a0d97ef03d6..85746a26d0e0 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -13901,6 +13901,106 @@ static void intel_commit_modeset_disables(struct drm_atomic_state *state)
> >         }
> >  }
> >
> > +static void icl_commit_modeset_disables(struct drm_atomic_state *state)
> > +{
> > +       struct drm_device *dev = state->dev;
> > +       struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
> > +       struct drm_i915_private *dev_priv = to_i915(dev);
> > +       struct drm_crtc_state *old_crtc_state, *new_crtc_state;
> > +       struct intel_crtc_state *new_intel_crtc_state, *old_intel_crtc_state;
> > +       struct drm_crtc *crtc;
> > +       struct intel_crtc *intel_crtc;
> > +       int i;
> > +
> > +       /*
> > +        * Disable all the Port Sync Slaves first
> > +        */
> > +       for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
> > +               old_intel_crtc_state = to_intel_crtc_state(old_crtc_state);
> > +               new_intel_crtc_state = to_intel_crtc_state(new_crtc_state);
> > +               intel_crtc = to_intel_crtc(crtc);
> > +
> > +               if (!needs_modeset(new_crtc_state) ||
> > +                   !is_trans_port_sync_slave(old_intel_crtc_state))
> > +                       continue;
> > +
> > +               intel_pre_plane_update(old_intel_crtc_state, new_intel_crtc_state);
> > +
> > +               if (old_crtc_state->active) {
> > +                       intel_crtc_disable_planes(intel_state, intel_crtc);
> > +
> > +                       /*
> > +                        * We need to disable pipe CRC before disabling the pipe,
> > +                        * or we race against vblank off.
> > +                        */
> > +                       intel_crtc_disable_pipe_crc(intel_crtc);
> > +
> > +                       dev_priv->display.crtc_disable(old_intel_crtc_state, state);
> > +                       intel_crtc->active = false;
> > +                       intel_fbc_disable(intel_crtc);
> > +                       intel_disable_shared_dpll(old_intel_crtc_state);
> > +
> > +                       /*
> > +                        * Underruns don't always raise interrupts,
> > +                        * so check manually.
> > +                        */
> > +                       intel_check_cpu_fifo_underruns(dev_priv);
> > +                       intel_check_pch_fifo_underruns(dev_priv);
> > +
> > +                       /* FIXME unify this for all platforms */
> > +                       if (!new_crtc_state->active &&
> > +                           !HAS_GMCH(dev_priv) &&
> > +                           dev_priv->display.initial_watermarks)
> > +                               dev_priv->display.initial_watermarks(intel_state,
> > +                                                                    new_intel_crtc_state);
> > +               }

I will move this part inside if (old_crtc_state->active) to a helper function.
Any suggestions on the name of this helper?

Manasi

> > +       }
> > +
> > +       /*
> > +        * Disable rest of the CRTCs other than slaves
> > +        */
> > +       for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
> > +               old_intel_crtc_state = to_intel_crtc_state(old_crtc_state);
> > +               new_intel_crtc_state = to_intel_crtc_state(new_crtc_state);
> > +               intel_crtc = to_intel_crtc(crtc);
> > +
> > +               if (!needs_modeset(new_crtc_state) ||
> > +                   is_trans_port_sync_slave(old_intel_crtc_state))
> > +                       continue;
> > +
> > +               intel_pre_plane_update(old_intel_crtc_state, new_intel_crtc_state);
> > +
> > +               if (old_crtc_state->active) {
> > +                       intel_crtc_disable_planes(intel_state, intel_crtc);
> > +
> > +                       /*
> > +                        * We need to disable pipe CRC before disabling the pipe,
> > +                        * or we race against vblank off.
> > +                        */
> > +                       intel_crtc_disable_pipe_crc(intel_crtc);
> > +
> > +                       dev_priv->display.crtc_disable(old_intel_crtc_state, state);
> > +                       intel_crtc->active = false;
> > +                       intel_fbc_disable(intel_crtc);
> > +                       intel_disable_shared_dpll(old_intel_crtc_state);
> > +
> > +                       /*
> > +                        * Underruns don't always raise interrupts,
> > +                        * so check manually.
> > +                        */
> > +                       intel_check_cpu_fifo_underruns(dev_priv);
> > +                       intel_check_pch_fifo_underruns(dev_priv);
> > +
> > +                       /* FIXME unify this for all platforms */
> > +                       if (!new_crtc_state->active &&
> > +                           !HAS_GMCH(dev_priv) &&
> > +                           dev_priv->display.initial_watermarks)
> > +                               dev_priv->display.initial_watermarks(intel_state,
> > +                                                                    new_intel_crtc_state);
> > +               }
> > +       }
> 
> Not a fan of this duplicate code here. At least a helper function
> would be needed.
> 
> Jose, didn't you have a pending patch to disable in reverse order to
> solve this same problem?
> 
> Lucas De Marchi
> 
> > +}
> > +
> >  static void intel_commit_modeset_enables(struct drm_atomic_state *state)
> >  {
> >         struct drm_crtc *crtc;
> > @@ -14257,6 +14357,11 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
> >
> >         intel_atomic_commit_fence_wait(intel_state);
> >
> > +       drm_atomic_helper_wait_for_dependencies(state);
> > +
> > +       if (intel_state->modeset)
> > +               wakeref = intel_display_power_get(dev_priv, POWER_DOMAIN_MODESET);
> > +
> >         for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
> >                 new_intel_crtc_state = to_intel_crtc_state(new_crtc_state);
> >                 intel_crtc = to_intel_crtc(crtc);
> > @@ -14269,11 +14374,6 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
> >                 }
> >         }
> >
> > -       drm_atomic_helper_wait_for_dependencies(state);
> > -
> > -       if (intel_state->modeset)
> > -               wakeref = intel_display_power_get(dev_priv, POWER_DOMAIN_MODESET);
> > -
> >         dev_priv->display.commit_modeset_disables(state);
> >
> >         /* FIXME: Eventually get rid of our intel_crtc->config pointer */
> > @@ -16170,7 +16270,12 @@ void intel_init_display_hooks(struct drm_i915_private *dev_priv)
> >         else
> >                 dev_priv->display.commit_modeset_enables = intel_commit_modeset_enables;
> >
> > -       dev_priv->display.commit_modeset_disables = intel_commit_modeset_disables;
> > +       if (INTEL_GEN(dev_priv) >= 11)
> > +               dev_priv->display.commit_modeset_disables =
> > +                       icl_commit_modeset_disables;
> > +       else
> > +               dev_priv->display.commit_modeset_disables =
> > +                       intel_commit_modeset_disables;
> >
> >  }
> >
> > --
> > 2.19.1
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> 
> -- 
> Lucas De Marchi
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v4 8/8] drm/i915/display/icl: In port sync mode disable slaves first then masters

[Manasi Navare]

In the transcoder port sync mode, the slave transcoders mask their vblanks
until master transcoder's vblank so while disabling them, make
sure slaves are disabled first and then the masters.

v2:
* Use the intel_old_crtc_state_disables() helper

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Matt Roper <matthew.d.roper@intel.com>
Cc: Jani Nikula <jani.nikula@intel.com>
Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
---
 drivers/gpu/drm/i915/display/intel_display.c | 68 ++++++++++++++++++--
 1 file changed, 62 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index 3ccba539cce0..e244d9cb4e33 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -13912,6 +13912,57 @@ static void intel_commit_modeset_disables(struct drm_atomic_state *state)
 	}
 }
 
+static void icl_commit_modeset_disables(struct drm_atomic_state *state)
+{
+	struct drm_crtc_state *old_crtc_state, *new_crtc_state;
+	struct intel_crtc_state *new_intel_crtc_state, *old_intel_crtc_state;
+	struct drm_crtc *crtc;
+	struct intel_crtc *intel_crtc;
+	int i;
+
+	/*
+	 * Disable all the Port Sync Slaves first
+	 */
+	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
+		old_intel_crtc_state = to_intel_crtc_state(old_crtc_state);
+		new_intel_crtc_state = to_intel_crtc_state(new_crtc_state);
+		intel_crtc = to_intel_crtc(crtc);
+
+		if (!needs_modeset(new_crtc_state) ||
+		    !is_trans_port_sync_slave(old_intel_crtc_state))
+			continue;
+
+		intel_pre_plane_update(old_intel_crtc_state, new_intel_crtc_state);
+
+		if (old_crtc_state->active)
+			intel_old_crtc_state_disables(state,
+						      old_intel_crtc_state,
+						      new_intel_crtc_state,
+						      intel_crtc);
+	}
+
+	/*
+	 * Disable rest of the CRTCs other than slaves
+	 */
+	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
+		old_intel_crtc_state = to_intel_crtc_state(old_crtc_state);
+		new_intel_crtc_state = to_intel_crtc_state(new_crtc_state);
+		intel_crtc = to_intel_crtc(crtc);
+
+		if (!needs_modeset(new_crtc_state) ||
+		    is_trans_port_sync_slave(old_intel_crtc_state))
+			continue;
+
+		intel_pre_plane_update(old_intel_crtc_state, new_intel_crtc_state);
+
+		if (old_crtc_state->active)
+			intel_old_crtc_state_disables(state,
+						      old_intel_crtc_state,
+						      new_intel_crtc_state,
+						      intel_crtc);
+	}
+}
+
 static void intel_commit_modeset_enables(struct drm_atomic_state *state)
 {
 	struct drm_crtc *crtc;
@@ -14268,6 +14319,11 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
 
 	intel_atomic_commit_fence_wait(intel_state);
 
+	drm_atomic_helper_wait_for_dependencies(state);
+
+	if (intel_state->modeset)
+		wakeref = intel_display_power_get(dev_priv, POWER_DOMAIN_MODESET);
+
 	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
 		new_intel_crtc_state = to_intel_crtc_state(new_crtc_state);
 		intel_crtc = to_intel_crtc(crtc);
@@ -14280,11 +14336,6 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
 		}
 	}
 
-	drm_atomic_helper_wait_for_dependencies(state);
-
-	if (intel_state->modeset)
-		wakeref = intel_display_power_get(dev_priv, POWER_DOMAIN_MODESET);
-
 	dev_priv->display.commit_modeset_disables(state);
 
 	/* FIXME: Eventually get rid of our intel_crtc->config pointer */
@@ -16181,7 +16232,12 @@ void intel_init_display_hooks(struct drm_i915_private *dev_priv)
 	else
 		dev_priv->display.commit_modeset_enables = intel_commit_modeset_enables;
 
-	dev_priv->display.commit_modeset_disables = intel_commit_modeset_disables;
+	if (INTEL_GEN(dev_priv) >= 11)
+		dev_priv->display.commit_modeset_disables =
+			icl_commit_modeset_disables;
+	else
+		dev_priv->display.commit_modeset_disables =
+			intel_commit_modeset_disables;
 
 }
 
-- 
2.19.1

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

[PATCH v4 2/8] drm/i915/display: Move the commit_tail() disable sequence to commit_modeset_disables() hook

[Manasi Navare]

Create a new hook commit_modeset_disables() consistent with the naming
in drm atomic helpers and similar to the commit_modeset_enables() hook.
This helps better organize the disable sequence in atomic_commit_tail()
and move that to this disable hook.

No functional change

v2:
* Create a helper for old_crtc_state disables (Lucas)

Suggested-by: Daniel Vetter <daniel.vetter@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Matt Roper <matthew.d.roper@intel.com>
Cc: Jani Nikula <jani.nikula@intel.com>
Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
---
 drivers/gpu/drm/i915/display/intel_display.c | 116 ++++++++++++-------
 drivers/gpu/drm/i915/i915_drv.h              |   1 +
 2 files changed, 75 insertions(+), 42 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index 71e86e2f0f90..4b088fa24aad 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -13613,6 +13613,69 @@ static void intel_update_crtc(struct drm_crtc *crtc,
 	intel_finish_crtc_commit(to_intel_atomic_state(state), intel_crtc);
 }
 
+static void intel_old_crtc_state_disables(struct drm_atomic_state *state,
+					  struct intel_crtc_state *old_intel_crtc_state,
+					  struct intel_crtc_state *new_intel_crtc_state,
+					  struct intel_crtc *intel_crtc)
+{
+	struct drm_i915_private *dev_priv = to_i915(state->dev);
+	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
+	struct drm_crtc_state *new_crtc_state = &new_intel_crtc_state->base;
+
+	intel_crtc_disable_planes(intel_state, intel_crtc);
+
+	/*
+	 * We need to disable pipe CRC before disabling the pipe,
+	 * or we race against vblank off.
+	 */
+	intel_crtc_disable_pipe_crc(intel_crtc);
+
+	dev_priv->display.crtc_disable(old_intel_crtc_state, state);
+	intel_crtc->active = false;
+	intel_fbc_disable(intel_crtc);
+	intel_disable_shared_dpll(old_intel_crtc_state);
+
+	/*
+	 * Underruns don't always raise interrupts,
+	 * so check manually.
+	 */
+	intel_check_cpu_fifo_underruns(dev_priv);
+	intel_check_pch_fifo_underruns(dev_priv);
+
+	/* FIXME unify this for all platforms */
+	if (!new_crtc_state->active &&
+	    !HAS_GMCH(dev_priv) &&
+	    dev_priv->display.initial_watermarks)
+		dev_priv->display.initial_watermarks(intel_state,
+						     new_intel_crtc_state);
+}
+
+static void intel_commit_modeset_disables(struct drm_atomic_state *state)
+{
+	struct drm_crtc_state *old_crtc_state, *new_crtc_state;
+	struct intel_crtc_state *new_intel_crtc_state, *old_intel_crtc_state;
+	struct drm_crtc *crtc;
+	struct intel_crtc *intel_crtc;
+	int i;
+
+	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
+		old_intel_crtc_state = to_intel_crtc_state(old_crtc_state);
+		new_intel_crtc_state = to_intel_crtc_state(new_crtc_state);
+		intel_crtc = to_intel_crtc(crtc);
+
+		if (!needs_modeset(new_crtc_state))
+			continue;
+
+		intel_pre_plane_update(old_intel_crtc_state, new_intel_crtc_state);
+
+		if (old_crtc_state->active)
+			intel_old_crtc_state_disables(state,
+						      old_intel_crtc_state,
+						      new_intel_crtc_state,
+						      intel_crtc);
+	}
+}
+
 static void intel_commit_modeset_enables(struct drm_atomic_state *state)
 {
 	struct drm_crtc *crtc;
@@ -13769,7 +13832,7 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
 	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
 	struct drm_i915_private *dev_priv = to_i915(dev);
 	struct drm_crtc_state *old_crtc_state, *new_crtc_state;
-	struct intel_crtc_state *new_intel_crtc_state, *old_intel_crtc_state;
+	struct intel_crtc_state *new_intel_crtc_state;
 	struct drm_crtc *crtc;
 	struct intel_crtc *intel_crtc;
 	u64 put_domains[I915_MAX_PIPES] = {};
@@ -13778,58 +13841,24 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
 
 	intel_atomic_commit_fence_wait(intel_state);
 
-	drm_atomic_helper_wait_for_dependencies(state);
-
-	if (intel_state->modeset)
-		wakeref = intel_display_power_get(dev_priv, POWER_DOMAIN_MODESET);
-
 	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
-		old_intel_crtc_state = to_intel_crtc_state(old_crtc_state);
 		new_intel_crtc_state = to_intel_crtc_state(new_crtc_state);
 		intel_crtc = to_intel_crtc(crtc);
 
 		if (needs_modeset(new_crtc_state) ||
-		    to_intel_crtc_state(new_crtc_state)->update_pipe) {
-
+		    new_intel_crtc_state->update_pipe) {
 			put_domains[intel_crtc->pipe] =
 				modeset_get_crtc_power_domains(crtc,
-					new_intel_crtc_state);
+							       new_intel_crtc_state);
 		}
+	}
 
-		if (!needs_modeset(new_crtc_state))
-			continue;
-
-		intel_pre_plane_update(old_intel_crtc_state, new_intel_crtc_state);
-
-		if (old_crtc_state->active) {
-			intel_crtc_disable_planes(intel_state, intel_crtc);
-
-			/*
-			 * We need to disable pipe CRC before disabling the pipe,
-			 * or we race against vblank off.
-			 */
-			intel_crtc_disable_pipe_crc(intel_crtc);
+	drm_atomic_helper_wait_for_dependencies(state);
 
-			dev_priv->display.crtc_disable(old_intel_crtc_state, state);
-			intel_crtc->active = false;
-			intel_fbc_disable(intel_crtc);
-			intel_disable_shared_dpll(old_intel_crtc_state);
+	if (intel_state->modeset)
+		wakeref = intel_display_power_get(dev_priv, POWER_DOMAIN_MODESET);
 
-			/*
-			 * Underruns don't always raise
-			 * interrupts, so check manually.
-			 */
-			intel_check_cpu_fifo_underruns(dev_priv);
-			intel_check_pch_fifo_underruns(dev_priv);
-
-			/* FIXME unify this for all platforms */
-			if (!new_crtc_state->active &&
-			    !HAS_GMCH(dev_priv) &&
-			    dev_priv->display.initial_watermarks)
-				dev_priv->display.initial_watermarks(intel_state,
-								     new_intel_crtc_state);
-		}
-	}
+	dev_priv->display.commit_modeset_disables(state);
 
 	/* FIXME: Eventually get rid of our intel_crtc->config pointer */
 	for_each_new_crtc_in_state(state, crtc, new_crtc_state, i)
@@ -15722,6 +15751,9 @@ void intel_init_display_hooks(struct drm_i915_private *dev_priv)
 		dev_priv->display.commit_modeset_enables = skl_commit_modeset_enables;
 	else
 		dev_priv->display.commit_modeset_enables = intel_commit_modeset_enables;
+
+	dev_priv->display.commit_modeset_disables = intel_commit_modeset_disables;
+
 }
 
 static i915_reg_t i915_vgacntrl_reg(struct drm_i915_private *dev_priv)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 075d7eb3c3f2..edb6b431f90c 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -311,6 +311,7 @@ struct drm_i915_display_funcs {
 	void (*crtc_disable)(struct intel_crtc_state *old_crtc_state,
 			     struct drm_atomic_state *old_state);
 	void (*commit_modeset_enables)(struct drm_atomic_state *state);
+	void (*commit_modeset_disables)(struct drm_atomic_state *state);
 	void (*audio_codec_enable)(struct intel_encoder *encoder,
 				   const struct intel_crtc_state *crtc_state,
 				   const struct drm_connector_state *conn_state);
-- 
2.19.1

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

✗ Fi.CI.CHECKPATCH: warning for Enable Transcoder Port Sync feature for Tiled displays (rev4)

[Patchwork]

== Series Details ==

Series: Enable Transcoder Port Sync feature for Tiled displays (rev4)
URL   : https://patchwork.freedesktop.org/series/59837/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
1ed185770840 drm/i915/display: Rename update_crtcs() to commit_modeset_enables()
1df0691a4636 drm/i915/display: Move the commit_tail() disable sequence to commit_modeset_disables() hook
321c24b27347 drm/i915/display/icl: Save Master transcoder in slave's crtc_state for Transcoder Port Sync
0e45d45ced7e drm/i915/display/icl: Enable TRANSCODER PORT SYNC for tiled displays across separate ports
6acc068f0bef drm/i915/display/icl: HW state readout for transcoder port sync config
-:39: CHECK:BRACES: braces {} should be used on all arms of this statement
#39: FILE: drivers/gpu/drm/i915/display/intel_display.c:10283:
+	if (trans_port_sync & PORT_SYNC_MODE_ENABLE) {
[...]
+	} else
[...]

-:56: CHECK:BRACES: Unbalanced braces around else statement
#56: FILE: drivers/gpu/drm/i915/display/intel_display.c:10300:
+	} else

total: 0 errors, 0 warnings, 2 checks, 56 lines checked
e1d605f6dfff drm/i915/display/icl: Enable master-slaves in trans port sync mode in correct order
-:146: WARNING:LONG_LINE: line over 100 characters
#146: FILE: drivers/gpu/drm/i915/display/intel_display.c:14047:
+						 &to_intel_crtc_state(old_crtc_state)->wm.skl.ddb) &&

total: 0 errors, 1 warnings, 0 checks, 263 lines checked
352a9304b1ba drm/i915/display/icl: Disable transcoder port sync as part of crtc_disable() sequence
802f2b59efd1 drm/i915/display/icl: In port sync mode disable slaves first then masters

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

✓ Fi.CI.BAT: success for Enable Transcoder Port Sync feature for Tiled displays (rev4)

[Patchwork]

== Series Details ==

Series: Enable Transcoder Port Sync feature for Tiled displays (rev4)
URL   : https://patchwork.freedesktop.org/series/59837/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_6377 -> Patchwork_13463
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13463/

Known issues
------------

  Here are the changes found in Patchwork_13463 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@gem_ctx_switch@basic-default:
    - fi-icl-u2:          [PASS][1] -> [INCOMPLETE][2] ([fdo#107713] / [fdo#108569])
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6377/fi-icl-u2/igt@gem_ctx_switch@basic-default.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13463/fi-icl-u2/igt@gem_ctx_switch@basic-default.html

  * igt@gem_exec_create@basic:
    - fi-cml-u2:          [PASS][3] -> [INCOMPLETE][4] ([fdo#110566])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6377/fi-cml-u2/igt@gem_exec_create@basic.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13463/fi-cml-u2/igt@gem_exec_create@basic.html

  * igt@i915_pm_rpm@module-reload:
    - fi-kbl-r:           [PASS][5] -> [DMESG-WARN][6] ([fdo#111012])
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6377/fi-kbl-r/igt@i915_pm_rpm@module-reload.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13463/fi-kbl-r/igt@i915_pm_rpm@module-reload.html

  * igt@kms_chamelium@hdmi-hpd-fast:
    - fi-kbl-7500u:       [PASS][7] -> [FAIL][8] ([fdo#109485])
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6377/fi-kbl-7500u/igt@kms_chamelium@hdmi-hpd-fast.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13463/fi-kbl-7500u/igt@kms_chamelium@hdmi-hpd-fast.html

  
  [fdo#107713]: https://bugs.freedesktop.org/show_bug.cgi?id=107713
  [fdo#108569]: https://bugs.freedesktop.org/show_bug.cgi?id=108569
  [fdo#109485]: https://bugs.freedesktop.org/show_bug.cgi?id=109485
  [fdo#110566]: https://bugs.freedesktop.org/show_bug.cgi?id=110566
  [fdo#111012]: https://bugs.freedesktop.org/show_bug.cgi?id=111012


Participating hosts (53 -> 44)
------------------------------

  Additional (1): fi-icl-guc 
  Missing    (10): fi-kbl-soraka fi-ilk-m540 fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-apl-guc fi-kbl-x1275 fi-icl-y fi-byt-clapper fi-bdw-samus 


Build changes
-------------

  * Linux: CI_DRM_6377 -> Patchwork_13463

  CI_DRM_6377: ede9edee003f46e3dbe0c6e603328c234632c393 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_5071: 3c4edeba35ac699db5b39600eb17f4151c6b42fd @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_13463: 802f2b59efd11bf43314818b07a5cbbb264f8e91 @ git://anongit.freedesktop.org/gfx-ci/linux


== Kernel 32bit build ==

Warning: Kernel 32bit buildtest failed:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13463/build_32bit.log

  CALL    scripts/checksyscalls.sh
  CALL    scripts/atomic/check-atomics.sh
  CHK     include/generated/compile.h
Kernel: arch/x86/boot/bzImage is ready  (#1)
  Building modules, stage 2.
  MODPOST 112 modules
ERROR: "__udivdi3" [drivers/gpu/drm/amd/amdgpu/amdgpu.ko] undefined!
ERROR: "__divdi3" [drivers/gpu/drm/amd/amdgpu/amdgpu.ko] undefined!
scripts/Makefile.modpost:91: recipe for target '__modpost' failed
make[1]: *** [__modpost] Error 1
Makefile:1287: recipe for target 'modules' failed
make: *** [modules] Error 2


== Linux commits ==

802f2b59efd1 drm/i915/display/icl: In port sync mode disable slaves first then masters
352a9304b1ba drm/i915/display/icl: Disable transcoder port sync as part of crtc_disable() sequence
e1d605f6dfff drm/i915/display/icl: Enable master-slaves in trans port sync mode in correct order
6acc068f0bef drm/i915/display/icl: HW state readout for transcoder port sync config
0e45d45ced7e drm/i915/display/icl: Enable TRANSCODER PORT SYNC for tiled displays across separate ports
321c24b27347 drm/i915/display/icl: Save Master transcoder in slave's crtc_state for Transcoder Port Sync
1df0691a4636 drm/i915/display: Move the commit_tail() disable sequence to commit_modeset_disables() hook
1ed185770840 drm/i915/display: Rename update_crtcs() to commit_modeset_enables()

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13463/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v4 8/8] drm/i915/display/icl: In port sync mode disable slaves first then masters

[Maarten Lankhorst]

Op 28-06-2019 om 00:57 schreef Manasi Navare:
> In the transcoder port sync mode, the slave transcoders mask their vblanks
> until master transcoder's vblank so while disabling them, make
> sure slaves are disabled first and then the masters.
>
> v2:
> * Use the intel_old_crtc_state_disables() helper

Not convinced that splitting up icl_commit and skl_commit is required. You could do this in skl_update_crtcs. Only is_trans_port_sync_slave would never evaluate to true. :)


I would probably also add integrate the bumping crtc limits series, and reject the raw 8k width in DDI encoder check.


> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Matt Roper <matthew.d.roper@intel.com>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_display.c | 68 ++++++++++++++++++--
>  1 file changed, 62 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index 3ccba539cce0..e244d9cb4e33 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -13912,6 +13912,57 @@ static void intel_commit_modeset_disables(struct drm_atomic_state *state)
>  	}
>  }
>  
> +static void icl_commit_modeset_disables(struct drm_atomic_state *state)
> +{
> +	struct drm_crtc_state *old_crtc_state, *new_crtc_state;
> +	struct intel_crtc_state *new_intel_crtc_state, *old_intel_crtc_state;
> +	struct drm_crtc *crtc;
> +	struct intel_crtc *intel_crtc;
> +	int i;
> +
> +	/*
> +	 * Disable all the Port Sync Slaves first
> +	 */
> +	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
> +		old_intel_crtc_state = to_intel_crtc_state(old_crtc_state);
> +		new_intel_crtc_state = to_intel_crtc_state(new_crtc_state);
> +		intel_crtc = to_intel_crtc(crtc);
> +
> +		if (!needs_modeset(new_crtc_state) ||
> +		    !is_trans_port_sync_slave(old_intel_crtc_state))
> +			continue;
> +
> +		intel_pre_plane_update(old_intel_crtc_state, new_intel_crtc_state);
> +
> +		if (old_crtc_state->active)
> +			intel_old_crtc_state_disables(state,
> +						      old_intel_crtc_state,
> +						      new_intel_crtc_state,
> +						      intel_crtc);
> +	}
> +
> +	/*
> +	 * Disable rest of the CRTCs other than slaves
> +	 */
> +	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
> +		old_intel_crtc_state = to_intel_crtc_state(old_crtc_state);
> +		new_intel_crtc_state = to_intel_crtc_state(new_crtc_state);
> +		intel_crtc = to_intel_crtc(crtc);
> +
> +		if (!needs_modeset(new_crtc_state) ||
> +		    is_trans_port_sync_slave(old_intel_crtc_state))
> +			continue;
> +
> +		intel_pre_plane_update(old_intel_crtc_state, new_intel_crtc_state);
> +
> +		if (old_crtc_state->active)
> +			intel_old_crtc_state_disables(state,
> +						      old_intel_crtc_state,
> +						      new_intel_crtc_state,
> +						      intel_crtc);
> +	}
> +}
> +
>  static void intel_commit_modeset_enables(struct drm_atomic_state *state)
>  {
>  	struct drm_crtc *crtc;
> @@ -14268,6 +14319,11 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
>  
>  	intel_atomic_commit_fence_wait(intel_state);
>  
> +	drm_atomic_helper_wait_for_dependencies(state);
> +
> +	if (intel_state->modeset)
> +		wakeref = intel_display_power_get(dev_priv, POWER_DOMAIN_MODESET);
> +
>  	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
>  		new_intel_crtc_state = to_intel_crtc_state(new_crtc_state);
>  		intel_crtc = to_intel_crtc(crtc);
> @@ -14280,11 +14336,6 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
>  		}
>  	}
>  
> -	drm_atomic_helper_wait_for_dependencies(state);
> -
> -	if (intel_state->modeset)
> -		wakeref = intel_display_power_get(dev_priv, POWER_DOMAIN_MODESET);
> -
>  	dev_priv->display.commit_modeset_disables(state);
>  
>  	/* FIXME: Eventually get rid of our intel_crtc->config pointer */
> @@ -16181,7 +16232,12 @@ void intel_init_display_hooks(struct drm_i915_private *dev_priv)
>  	else
>  		dev_priv->display.commit_modeset_enables = intel_commit_modeset_enables;
>  
> -	dev_priv->display.commit_modeset_disables = intel_commit_modeset_disables;
> +	if (INTEL_GEN(dev_priv) >= 11)
> +		dev_priv->display.commit_modeset_disables =
> +			icl_commit_modeset_disables;
> +	else
> +		dev_priv->display.commit_modeset_disables =
> +			intel_commit_modeset_disables;
>  
>  }
>  


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

Re: [PATCH v3 6/8] drm/i915/display/icl: Enable master-slaves in trans port sync mode in correct order

[Maarten Lankhorst]

Op 24-06-2019 om 23:08 schreef Manasi Navare:
> As per the display enable sequence, we need to follow the enable sequence
> for slaves first with DP_TP_CTL set to Idle and configure the transcoder
> port sync register to select the corersponding master, then follow the
> enable sequence for master leaving DP_TP_CTL to idle.
> At this point the transcoder port sync mode is configured and enabled
> and the Vblanks of both ports are synchronized so then set DP_TP_CTL
> for the slave and master to Normal and do post crtc enable updates.
>
> v2:
> * Create a icl_update_crtcs hook (Maarten, Danvet)
> * This sequence only for CRTCs in trans port sync mode (Maarten)
>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Matt Roper <matthew.d.roper@intel.com>
> Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_ddi.c     |   3 +-
>  drivers/gpu/drm/i915/display/intel_display.c | 217 ++++++++++++++++++-
>  drivers/gpu/drm/i915/display/intel_display.h |   4 +
>  3 files changed, 221 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
> index 7925a176f900..bceb7e4b1877 100644
> --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> @@ -3154,7 +3154,8 @@ static void intel_ddi_pre_enable_dp(struct intel_encoder *encoder,
>  					      true);
>  	intel_dp_sink_set_fec_ready(intel_dp, crtc_state);
>  	intel_dp_start_link_train(intel_dp);
> -	if (port != PORT_A || INTEL_GEN(dev_priv) >= 9)
> +	if ((port != PORT_A || INTEL_GEN(dev_priv) >= 9) &&
> +	    !is_trans_port_sync_mode(crtc_state))
>  		intel_dp_stop_link_train(intel_dp);
>  
>  	intel_ddi_enable_fec(encoder, crtc_state);
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index 7156b1b4c6c5..f88d3a929e36 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -520,6 +520,26 @@ needs_modeset(const struct drm_crtc_state *state)
>  	return drm_atomic_crtc_needs_modeset(state);
>  }
>  
> +bool
> +is_trans_port_sync_mode(const struct intel_crtc_state *state)
> +{
> +	return (state->master_transcoder != INVALID_TRANSCODER ||
> +		state->sync_mode_slaves_mask);
> +}
> +
> +static bool
> +is_trans_port_sync_slave(const struct intel_crtc_state *state)
> +{
> +	return state->master_transcoder != INVALID_TRANSCODER;
> +}
> +
> +static bool
> +is_trans_port_sync_master(const struct intel_crtc_state *state)
> +{
> +	return (state->master_transcoder == INVALID_TRANSCODER &&
> +		state->sync_mode_slaves_mask);
> +}
> +
>  /*
>   * Platform specific helpers to calculate the port PLL loopback- (clock.m),
>   * and post-divider (clock.p) values, pre- (clock.vco) and post-divided fast
> @@ -13944,9 +13964,200 @@ static void skl_commit_modeset_enables(struct drm_atomic_state *state)
>  			progress = true;
>  		}
>  	} while (progress);
> +}
>  
> +static void icl_commit_modeset_enables(struct drm_atomic_state *state)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(state->dev);
> +	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
> +	struct drm_crtc *crtc;
> +	struct intel_crtc *intel_crtc;
> +	struct drm_crtc_state *old_crtc_state, *new_crtc_state;
> +	struct intel_crtc_state *cstate;
> +	unsigned int updated = 0;
> +	bool progress;
> +	enum pipe pipe;
> +	int i;
> +	u8 hw_enabled_slices = dev_priv->wm.skl_hw.ddb.enabled_slices;
> +	u8 required_slices = intel_state->wm_results.ddb.enabled_slices;
> +	struct skl_ddb_entry entries[I915_MAX_PIPES] = {};
Add old_entries as well, merge master + slave
> +
> +	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i)
> +		/* ignore allocations for crtc's that have been turned off. */
> +		if (new_crtc_state->active)
> +			entries[i] = to_intel_crtc_state(old_crtc_state)->wm.skl.ddb;

Can be changed to: if (new_crtc_state->active && !needs_modeset(new_crtc_state)) ?

Small refinement to the algorithm in general, I dislike the current handling.

What I would do:
- Make a new_entries array as well, that contains (for the master crtc, during modeset only) master_ddb.begin + slave_ddb.end,
  and nothing for the slave ddb in that case, the ddb allocation for all other cases including master/slave non-modeset should be the default wm.skl.ddb.
  Use it for comparing instead of the one from crtc_state. This way we know we can always enable both at the same time.
- Ignore the slave crtc when needs_modeset() is true, called from master instead.
- If a modeset happens on master crtc, do the special enabling dance on both in a separate function.
- Also ensure that the slave crtc is marked as updated, and update both entries to correct values in the entries again.

You should be able to get the slave crtc with intel_get_crtc_for_pipe(dev_priv, intel_crtc->pipe + 1);

With these changes you should be able to get rid of the icl special handling, it's confined to a single function for enabling,
only called when needs_modeset() && is_trans_port_sync_master is true, without the problem of overlapping allocations.



> +	/* If 2nd DBuf slice required, enable it here */
> +	if (required_slices > hw_enabled_slices)
> +		icl_dbuf_slices_update(dev_priv, required_slices);
> +
> +	/*
> +	 * Whenever the number of active pipes changes, we need to make sure we
> +	 * update the pipes in the right order so that their ddb allocations
> +	 * never overlap with eachother inbetween CRTC updates. Otherwise we'll
> +	 * cause pipe underruns and other bad stuff.
> +	 */
> +	do {
> +		progress = false;
> +
> +		for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
> +			bool vbl_wait = false;
> +			unsigned int cmask = drm_crtc_mask(crtc);
> +			bool modeset = needs_modeset(new_crtc_state);
> +
> +			intel_crtc = to_intel_crtc(crtc);
> +			cstate = to_intel_crtc_state(new_crtc_state);
> +			pipe = intel_crtc->pipe;
> +
> +			if (updated & cmask || !cstate->base.active)
> +				continue;
> +
> +			if (modeset && is_trans_port_sync_mode(cstate)) {
> +				DRM_DEBUG_KMS("Pushing the Sync Mode CRTC %d %s that needs update to separate loop\n",
> +					      intel_crtc->base.base.id, intel_crtc->base.name);
> +				continue;
> +			}



> +
> +			if (skl_ddb_allocation_overlaps(&cstate->wm.skl.ddb,
> +							entries,
> +							INTEL_INFO(dev_priv)->num_pipes, i))
> +				continue;
> +
> +			updated |= cmask;
> +			entries[i] = cstate->wm.skl.ddb;
> +
> +			/*
> +			 * If this is an already active pipe, it's DDB changed,
> +			 * and this isn't the last pipe that needs updating
> +			 * then we need to wait for a vblank to pass for the
> +			 * new ddb allocation to take effect.
> +			 */
> +			if (!skl_ddb_entry_equal(&cstate->wm.skl.ddb,
> +						 &to_intel_crtc_state(old_crtc_state)->wm.skl.ddb) &&
> +			    !new_crtc_state->active_changed &&
> +			    intel_state->wm_results.dirty_pipes != updated)
> +				vbl_wait = true;
> +
> +			intel_update_crtc(crtc, state, old_crtc_state,
> +					  new_crtc_state);
> +
> +			if (vbl_wait)
> +				intel_wait_for_vblank(dev_priv, pipe);
> +
> +			progress = true;
> +		}
> +	} while (progress);
> +	/* Set Slave's DP_TP_CTL to Normal */
> +	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
> +		int j;
> +		struct drm_connector_state *conn_state;
> +		struct drm_connector *conn;
> +		bool modeset = needs_modeset(new_crtc_state);
> +		struct intel_dp *intel_dp;
> +		struct intel_crtc_state *pipe_config =
> +			to_intel_crtc_state(new_crtc_state);
> +
> +		intel_crtc = to_intel_crtc(crtc);
> +
> +		if (!pipe_config->base.active || !modeset ||
> +		    !is_trans_port_sync_slave(pipe_config))
> +			continue;
> +
> +		for_each_new_connector_in_state(state, conn, conn_state, j) {
> +			if (conn_state->crtc == crtc)
> +				break;
> +		}
> +		intel_dp = enc_to_intel_dp(&intel_attached_encoder(conn)->base);

enc_to_intel_dp(conn_state->best_encoder). :)

Make a small helper here, stop_link_training_for_crtc, call it for slave_crtc, then usleep, then master_crtc in the function described above?


> +
> +		new_plane_state =
> +			intel_atomic_get_new_plane_state(to_intel_atomic_state(state),
> +							 to_intel_plane(crtc->primary));
> +		if (pipe_config->update_pipe && !pipe_config->enable_fbc)
> +			intel_fbc_disable(intel_crtc);
> +		else if (new_plane_state)
> +			intel_fbc_enable(intel_crtc, pipe_config, new_plane_state);
> +
> +		intel_begin_crtc_commit(to_intel_atomic_state(state), intel_crtc);
> +		skl_update_planes_on_crtc(to_intel_atomic_state(state), intel_crtc);
> +		intel_finish_crtc_commit(to_intel_atomic_state(state), intel_crtc);
> +	}

intel_commit_planes()


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

Re: [PATCH v3 6/8] drm/i915/display/icl: Enable master-slaves in trans port sync mode in correct order

[Manasi Navare]

Thanks Maarten for your review comments, please see my responses/questions below:

On Tue, Jul 30, 2019 at 12:53:30PM +0200, Maarten Lankhorst wrote:
> Op 24-06-2019 om 23:08 schreef Manasi Navare:
> > As per the display enable sequence, we need to follow the enable sequence
> > for slaves first with DP_TP_CTL set to Idle and configure the transcoder
> > port sync register to select the corersponding master, then follow the
> > enable sequence for master leaving DP_TP_CTL to idle.
> > At this point the transcoder port sync mode is configured and enabled
> > and the Vblanks of both ports are synchronized so then set DP_TP_CTL
> > for the slave and master to Normal and do post crtc enable updates.
> >
> > v2:
> > * Create a icl_update_crtcs hook (Maarten, Danvet)
> > * This sequence only for CRTCs in trans port sync mode (Maarten)
> >
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Cc: Matt Roper <matthew.d.roper@intel.com>
> > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_ddi.c     |   3 +-
> >  drivers/gpu/drm/i915/display/intel_display.c | 217 ++++++++++++++++++-
> >  drivers/gpu/drm/i915/display/intel_display.h |   4 +
> >  3 files changed, 221 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
> > index 7925a176f900..bceb7e4b1877 100644
> > --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> > @@ -3154,7 +3154,8 @@ static void intel_ddi_pre_enable_dp(struct intel_encoder *encoder,
> >  					      true);
> >  	intel_dp_sink_set_fec_ready(intel_dp, crtc_state);
> >  	intel_dp_start_link_train(intel_dp);
> > -	if (port != PORT_A || INTEL_GEN(dev_priv) >= 9)
> > +	if ((port != PORT_A || INTEL_GEN(dev_priv) >= 9) &&
> > +	    !is_trans_port_sync_mode(crtc_state))
> >  		intel_dp_stop_link_train(intel_dp);
> >  
> >  	intel_ddi_enable_fec(encoder, crtc_state);
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > index 7156b1b4c6c5..f88d3a929e36 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -520,6 +520,26 @@ needs_modeset(const struct drm_crtc_state *state)
> >  	return drm_atomic_crtc_needs_modeset(state);
> >  }
> >  
> > +bool
> > +is_trans_port_sync_mode(const struct intel_crtc_state *state)
> > +{
> > +	return (state->master_transcoder != INVALID_TRANSCODER ||
> > +		state->sync_mode_slaves_mask);
> > +}
> > +
> > +static bool
> > +is_trans_port_sync_slave(const struct intel_crtc_state *state)
> > +{
> > +	return state->master_transcoder != INVALID_TRANSCODER;
> > +}
> > +
> > +static bool
> > +is_trans_port_sync_master(const struct intel_crtc_state *state)
> > +{
> > +	return (state->master_transcoder == INVALID_TRANSCODER &&
> > +		state->sync_mode_slaves_mask);
> > +}
> > +
> >  /*
> >   * Platform specific helpers to calculate the port PLL loopback- (clock.m),
> >   * and post-divider (clock.p) values, pre- (clock.vco) and post-divided fast
> > @@ -13944,9 +13964,200 @@ static void skl_commit_modeset_enables(struct drm_atomic_state *state)
> >  			progress = true;
> >  		}
> >  	} while (progress);
> > +}
> >  
> > +static void icl_commit_modeset_enables(struct drm_atomic_state *state)
> > +{
> > +	struct drm_i915_private *dev_priv = to_i915(state->dev);
> > +	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
> > +	struct drm_crtc *crtc;
> > +	struct intel_crtc *intel_crtc;
> > +	struct drm_crtc_state *old_crtc_state, *new_crtc_state;
> > +	struct intel_crtc_state *cstate;
> > +	unsigned int updated = 0;
> > +	bool progress;
> > +	enum pipe pipe;
> > +	int i;
> > +	u8 hw_enabled_slices = dev_priv->wm.skl_hw.ddb.enabled_slices;
> > +	u8 required_slices = intel_state->wm_results.ddb.enabled_slices;
> > +	struct skl_ddb_entry entries[I915_MAX_PIPES] = {};
> Add old_entries as well, merge master + slave

I didnt understand what you meant by merge master+slaves? You mean add also the 
master and slave that are already enabled?

> > +
> > +	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i)
> > +		/* ignore allocations for crtc's that have been turned off. */
> > +		if (new_crtc_state->active)
> > +			entries[i] = to_intel_crtc_state(old_crtc_state)->wm.skl.ddb;
> 
> Can be changed to: if (new_crtc_state->active && !needs_modeset(new_crtc_state)) ?

We need !needs_modeset() also? That was not intially there in the original skl_update_crtcs(), 
so why do we need it here?

> 
> Small refinement to the algorithm in general, I dislike the current handling.
> 
> What I would do:
> - Make a new_entries array as well, that contains (for the master crtc, during modeset only) master_ddb.begin + slave_ddb.end,
>   and nothing for the slave ddb in that case, the ddb allocation for all other cases including master/slave non-modeset should be the default wm.skl.ddb.
>   Use it for comparing instead of the one from crtc_state. This way we know we can always enable both at the same time.

So in the the case of modeset on master or slave, we shd create another entries similar to default one and have the allocations for master + slave and make sure that doesnt overlap with the already active crtc allocations?

> - Ignore the slave crtc when needs_modeset() is true, called from master instead.
> - If a modeset happens on master crtc, do the special enabling dance on both in a separate function.

So you are saying that if it is slave crtc and needs_modeset just skip and dont do anything,
But in case of master crtc modeset, thats where we will need these additional 4 loops and thats where we access the slave crtcs from the slave_sync_mask and then do the correct enabling sequence etc?
 
> - Also ensure that the slave crtc is marked as updated, and update both entries to correct values in the entries again.

This again I am not 100% clear on how to implement might need to discuss on IRC

> 
> You should be able to get the slave crtc with intel_get_crtc_for_pipe(dev_priv, intel_crtc->pipe + 1);

But the problem is that ther could be multiple slaves not just 1 and IMO accessing the slaves during the master modeset is more complicated
where as the current logic takes care of handling the correct enabling sequence for any number of slaves and master and modesets
in any order.
Will still have to check for proper ddb allocations and ensure no overlap.

Manasi

> 
> With these changes you should be able to get rid of the icl special handling, it's confined to a single function for enabling,
> only called when needs_modeset() && is_trans_port_sync_master is true, without the problem of overlapping allocations.
> 
> 
> 
> > +	/* If 2nd DBuf slice required, enable it here */
> > +	if (required_slices > hw_enabled_slices)
> > +		icl_dbuf_slices_update(dev_priv, required_slices);
> > +
> > +	/*
> > +	 * Whenever the number of active pipes changes, we need to make sure we
> > +	 * update the pipes in the right order so that their ddb allocations
> > +	 * never overlap with eachother inbetween CRTC updates. Otherwise we'll
> > +	 * cause pipe underruns and other bad stuff.
> > +	 */
> > +	do {
> > +		progress = false;
> > +
> > +		for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
> > +			bool vbl_wait = false;
> > +			unsigned int cmask = drm_crtc_mask(crtc);
> > +			bool modeset = needs_modeset(new_crtc_state);
> > +
> > +			intel_crtc = to_intel_crtc(crtc);
> > +			cstate = to_intel_crtc_state(new_crtc_state);
> > +			pipe = intel_crtc->pipe;
> > +
> > +			if (updated & cmask || !cstate->base.active)
> > +				continue;
> > +
> > +			if (modeset && is_trans_port_sync_mode(cstate)) {
> > +				DRM_DEBUG_KMS("Pushing the Sync Mode CRTC %d %s that needs update to separate loop\n",
> > +					      intel_crtc->base.base.id, intel_crtc->base.name);
> > +				continue;
> > +			}
> 
> 
> 
> > +
> > +			if (skl_ddb_allocation_overlaps(&cstate->wm.skl.ddb,
> > +							entries,
> > +							INTEL_INFO(dev_priv)->num_pipes, i))
> > +				continue;
> > +
> > +			updated |= cmask;
> > +			entries[i] = cstate->wm.skl.ddb;
> > +
> > +			/*
> > +			 * If this is an already active pipe, it's DDB changed,
> > +			 * and this isn't the last pipe that needs updating
> > +			 * then we need to wait for a vblank to pass for the
> > +			 * new ddb allocation to take effect.
> > +			 */
> > +			if (!skl_ddb_entry_equal(&cstate->wm.skl.ddb,
> > +						 &to_intel_crtc_state(old_crtc_state)->wm.skl.ddb) &&
> > +			    !new_crtc_state->active_changed &&
> > +			    intel_state->wm_results.dirty_pipes != updated)
> > +				vbl_wait = true;
> > +
> > +			intel_update_crtc(crtc, state, old_crtc_state,
> > +					  new_crtc_state);
> > +
> > +			if (vbl_wait)
> > +				intel_wait_for_vblank(dev_priv, pipe);
> > +
> > +			progress = true;
> > +		}
> > +	} while (progress);
> > +	/* Set Slave's DP_TP_CTL to Normal */
> > +	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
> > +		int j;
> > +		struct drm_connector_state *conn_state;
> > +		struct drm_connector *conn;
> > +		bool modeset = needs_modeset(new_crtc_state);
> > +		struct intel_dp *intel_dp;
> > +		struct intel_crtc_state *pipe_config =
> > +			to_intel_crtc_state(new_crtc_state);
> > +
> > +		intel_crtc = to_intel_crtc(crtc);
> > +
> > +		if (!pipe_config->base.active || !modeset ||
> > +		    !is_trans_port_sync_slave(pipe_config))
> > +			continue;
> > +
> > +		for_each_new_connector_in_state(state, conn, conn_state, j) {
> > +			if (conn_state->crtc == crtc)
> > +				break;
> > +		}
> > +		intel_dp = enc_to_intel_dp(&intel_attached_encoder(conn)->base);
> 
> enc_to_intel_dp(conn_state->best_encoder). :)
> 
> Make a small helper here, stop_link_training_for_crtc, call it for slave_crtc, then usleep, then master_crtc in the function described above?
> 
> 
> > +
> > +		new_plane_state =
> > +			intel_atomic_get_new_plane_state(to_intel_atomic_state(state),
> > +							 to_intel_plane(crtc->primary));
> > +		if (pipe_config->update_pipe && !pipe_config->enable_fbc)
> > +			intel_fbc_disable(intel_crtc);
> > +		else if (new_plane_state)
> > +			intel_fbc_enable(intel_crtc, pipe_config, new_plane_state);
> > +
> > +		intel_begin_crtc_commit(to_intel_atomic_state(state), intel_crtc);
> > +		skl_update_planes_on_crtc(to_intel_atomic_state(state), intel_crtc);
> > +		intel_finish_crtc_commit(to_intel_atomic_state(state), intel_crtc);
> > +	}
> 
> intel_commit_planes()
> 
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 6/8] drm/i915/display/icl: Enable master-slaves in trans port sync mode in correct order

[Maarten Lankhorst]

Op 01-08-2019 om 01:24 schreef Manasi Navare:
> Thanks Maarten for your review comments, please see my responses/questions below:
>
> On Tue, Jul 30, 2019 at 12:53:30PM +0200, Maarten Lankhorst wrote:
>> Op 24-06-2019 om 23:08 schreef Manasi Navare:
>>> As per the display enable sequence, we need to follow the enable sequence
>>> for slaves first with DP_TP_CTL set to Idle and configure the transcoder
>>> port sync register to select the corersponding master, then follow the
>>> enable sequence for master leaving DP_TP_CTL to idle.
>>> At this point the transcoder port sync mode is configured and enabled
>>> and the Vblanks of both ports are synchronized so then set DP_TP_CTL
>>> for the slave and master to Normal and do post crtc enable updates.
>>>
>>> v2:
>>> * Create a icl_update_crtcs hook (Maarten, Danvet)
>>> * This sequence only for CRTCs in trans port sync mode (Maarten)
>>>
>>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>> Cc: Matt Roper <matthew.d.roper@intel.com>
>>> Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
>>> ---
>>>  drivers/gpu/drm/i915/display/intel_ddi.c     |   3 +-
>>>  drivers/gpu/drm/i915/display/intel_display.c | 217 ++++++++++++++++++-
>>>  drivers/gpu/drm/i915/display/intel_display.h |   4 +
>>>  3 files changed, 221 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
>>> index 7925a176f900..bceb7e4b1877 100644
>>> --- a/drivers/gpu/drm/i915/display/intel_ddi.c
>>> +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
>>> @@ -3154,7 +3154,8 @@ static void intel_ddi_pre_enable_dp(struct intel_encoder *encoder,
>>>  					      true);
>>>  	intel_dp_sink_set_fec_ready(intel_dp, crtc_state);
>>>  	intel_dp_start_link_train(intel_dp);
>>> -	if (port != PORT_A || INTEL_GEN(dev_priv) >= 9)
>>> +	if ((port != PORT_A || INTEL_GEN(dev_priv) >= 9) &&
>>> +	    !is_trans_port_sync_mode(crtc_state))
>>>  		intel_dp_stop_link_train(intel_dp);
>>>  
>>>  	intel_ddi_enable_fec(encoder, crtc_state);
>>> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
>>> index 7156b1b4c6c5..f88d3a929e36 100644
>>> --- a/drivers/gpu/drm/i915/display/intel_display.c
>>> +++ b/drivers/gpu/drm/i915/display/intel_display.c
>>> @@ -520,6 +520,26 @@ needs_modeset(const struct drm_crtc_state *state)
>>>  	return drm_atomic_crtc_needs_modeset(state);
>>>  }
>>>  
>>> +bool
>>> +is_trans_port_sync_mode(const struct intel_crtc_state *state)
>>> +{
>>> +	return (state->master_transcoder != INVALID_TRANSCODER ||
>>> +		state->sync_mode_slaves_mask);
>>> +}
>>> +
>>> +static bool
>>> +is_trans_port_sync_slave(const struct intel_crtc_state *state)
>>> +{
>>> +	return state->master_transcoder != INVALID_TRANSCODER;
>>> +}
>>> +
>>> +static bool
>>> +is_trans_port_sync_master(const struct intel_crtc_state *state)
>>> +{
>>> +	return (state->master_transcoder == INVALID_TRANSCODER &&
>>> +		state->sync_mode_slaves_mask);
>>> +}
>>> +
>>>  /*
>>>   * Platform specific helpers to calculate the port PLL loopback- (clock.m),
>>>   * and post-divider (clock.p) values, pre- (clock.vco) and post-divided fast
>>> @@ -13944,9 +13964,200 @@ static void skl_commit_modeset_enables(struct drm_atomic_state *state)
>>>  			progress = true;
>>>  		}
>>>  	} while (progress);
>>> +}
>>>  
>>> +static void icl_commit_modeset_enables(struct drm_atomic_state *state)
>>> +{
>>> +	struct drm_i915_private *dev_priv = to_i915(state->dev);
>>> +	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
>>> +	struct drm_crtc *crtc;
>>> +	struct intel_crtc *intel_crtc;
>>> +	struct drm_crtc_state *old_crtc_state, *new_crtc_state;
>>> +	struct intel_crtc_state *cstate;
>>> +	unsigned int updated = 0;
>>> +	bool progress;
>>> +	enum pipe pipe;
>>> +	int i;
>>> +	u8 hw_enabled_slices = dev_priv->wm.skl_hw.ddb.enabled_slices;
>>> +	u8 required_slices = intel_state->wm_results.ddb.enabled_slices;
>>> +	struct skl_ddb_entry entries[I915_MAX_PIPES] = {};
>> Add old_entries as well, merge master + slave
> I didnt understand what you meant by merge master+slaves? You mean add also the 
> master and slave that are already enabled?

Instead of 2 separate allocations, only have a single allocation that contains the slave and master
ddb during modeset/fastset.

This will allow it to be updated as a single crtc. This is useful for modeset enable/disable as a single sequence, and could
potentiallybe useful for normal page flips as well to reduce tearing.

>>> +
>>> +	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i)
>>> +		/* ignore allocations for crtc's that have been turned off. */
>>> +		if (new_crtc_state->active)
>>> +			entries[i] = to_intel_crtc_state(old_crtc_state)->wm.skl.ddb;
>> Can be changed to: if (new_crtc_state->active && !needs_modeset(new_crtc_state)) ?
> We need !needs_modeset() also? That was not intially there in the original skl_update_crtcs(), 
> so why do we need it here?

It's not really needed, a minor optimization.

If needs_modeset is true, we can be guaranteed that we are always enabling, so the initial DDB allocation is always zero.

>
>> Small refinement to the algorithm in general, I dislike the current handling.
>>
>> What I would do:
>> - Make a new_entries array as well, that contains (for the master crtc, during modeset only) master_ddb.begin + slave_ddb.end,
>>   and nothing for the slave ddb in that case, the ddb allocation for all other cases including master/slave non-modeset should be the default wm.skl.ddb.
>>   Use it for comparing instead of the one from crtc_state. This way we know we can always enable both at the same time.
> So in the the case of modeset on master or slave, we shd create another entries similar to default one and have the allocations for master + slave and make sure that doesnt overlap with the already active crtc allocations?
That's the idea. :)
>
>> - Ignore the slave crtc when needs_modeset() is true, called from master instead.
>> - If a modeset happens on master crtc, do the special enabling dance on both in a separate function.
> So you are saying that if it is slave crtc and needs_modeset just skip and dont do anything,
> But in case of master crtc modeset, thats where we will need these additional 4 loops and thats where we access the slave crtcs from the slave_sync_mask and then do the correct enabling sequence etc?
>  
>> - Also ensure that the slave crtc is marked as updated, and update both entries to correct values in the entries again.
> This again I am not 100% clear on how to implement might need to discuss on IRC
>
>> You should be able to get the slave crtc with intel_get_crtc_for_pipe(dev_priv, intel_crtc->pipe + 1);
> But the problem is that ther could be multiple slaves not just 1 and IMO accessing the slaves during the master modeset is more complicated
> where as the current logic takes care of handling the correct enabling sequence for any number of slaves and master and modesets
> in any order.
> Will still have to check for proper ddb allocations and ensure no overlap.
>
Yeah but with the ddb allocations you make sure that we can always use the correct order. Because the master + slave ddb are sequential, if an allocation that encompasses both doesn't overlap, we know for sure we can just do both. :) 

~Maarten

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

Re: [PATCH v3 6/8] drm/i915/display/icl: Enable master-slaves in trans port sync mode in correct order

[Manasi Navare]

On Thu, Aug 01, 2019 at 05:07:48PM +0200, Maarten Lankhorst wrote:
> Op 01-08-2019 om 01:24 schreef Manasi Navare:
> > Thanks Maarten for your review comments, please see my responses/questions below:
> >
> > On Tue, Jul 30, 2019 at 12:53:30PM +0200, Maarten Lankhorst wrote:
> >> Op 24-06-2019 om 23:08 schreef Manasi Navare:
> >>> As per the display enable sequence, we need to follow the enable sequence
> >>> for slaves first with DP_TP_CTL set to Idle and configure the transcoder
> >>> port sync register to select the corersponding master, then follow the
> >>> enable sequence for master leaving DP_TP_CTL to idle.
> >>> At this point the transcoder port sync mode is configured and enabled
> >>> and the Vblanks of both ports are synchronized so then set DP_TP_CTL
> >>> for the slave and master to Normal and do post crtc enable updates.
> >>>
> >>> v2:
> >>> * Create a icl_update_crtcs hook (Maarten, Danvet)
> >>> * This sequence only for CRTCs in trans port sync mode (Maarten)
> >>>
> >>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> >>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >>> Cc: Matt Roper <matthew.d.roper@intel.com>
> >>> Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> >>> ---
> >>>  drivers/gpu/drm/i915/display/intel_ddi.c     |   3 +-
> >>>  drivers/gpu/drm/i915/display/intel_display.c | 217 ++++++++++++++++++-
> >>>  drivers/gpu/drm/i915/display/intel_display.h |   4 +
> >>>  3 files changed, 221 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
> >>> index 7925a176f900..bceb7e4b1877 100644
> >>> --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> >>> +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> >>> @@ -3154,7 +3154,8 @@ static void intel_ddi_pre_enable_dp(struct intel_encoder *encoder,
> >>>  					      true);
> >>>  	intel_dp_sink_set_fec_ready(intel_dp, crtc_state);
> >>>  	intel_dp_start_link_train(intel_dp);
> >>> -	if (port != PORT_A || INTEL_GEN(dev_priv) >= 9)
> >>> +	if ((port != PORT_A || INTEL_GEN(dev_priv) >= 9) &&
> >>> +	    !is_trans_port_sync_mode(crtc_state))
> >>>  		intel_dp_stop_link_train(intel_dp);
> >>>  
> >>>  	intel_ddi_enable_fec(encoder, crtc_state);
> >>> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> >>> index 7156b1b4c6c5..f88d3a929e36 100644
> >>> --- a/drivers/gpu/drm/i915/display/intel_display.c
> >>> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> >>> @@ -520,6 +520,26 @@ needs_modeset(const struct drm_crtc_state *state)
> >>>  	return drm_atomic_crtc_needs_modeset(state);
> >>>  }
> >>>  
> >>> +bool
> >>> +is_trans_port_sync_mode(const struct intel_crtc_state *state)
> >>> +{
> >>> +	return (state->master_transcoder != INVALID_TRANSCODER ||
> >>> +		state->sync_mode_slaves_mask);
> >>> +}
> >>> +
> >>> +static bool
> >>> +is_trans_port_sync_slave(const struct intel_crtc_state *state)
> >>> +{
> >>> +	return state->master_transcoder != INVALID_TRANSCODER;
> >>> +}
> >>> +
> >>> +static bool
> >>> +is_trans_port_sync_master(const struct intel_crtc_state *state)
> >>> +{
> >>> +	return (state->master_transcoder == INVALID_TRANSCODER &&
> >>> +		state->sync_mode_slaves_mask);
> >>> +}
> >>> +
> >>>  /*
> >>>   * Platform specific helpers to calculate the port PLL loopback- (clock.m),
> >>>   * and post-divider (clock.p) values, pre- (clock.vco) and post-divided fast
> >>> @@ -13944,9 +13964,200 @@ static void skl_commit_modeset_enables(struct drm_atomic_state *state)
> >>>  			progress = true;
> >>>  		}
> >>>  	} while (progress);
> >>> +}
> >>>  
> >>> +static void icl_commit_modeset_enables(struct drm_atomic_state *state)
> >>> +{
> >>> +	struct drm_i915_private *dev_priv = to_i915(state->dev);
> >>> +	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
> >>> +	struct drm_crtc *crtc;
> >>> +	struct intel_crtc *intel_crtc;
> >>> +	struct drm_crtc_state *old_crtc_state, *new_crtc_state;
> >>> +	struct intel_crtc_state *cstate;
> >>> +	unsigned int updated = 0;
> >>> +	bool progress;
> >>> +	enum pipe pipe;
> >>> +	int i;
> >>> +	u8 hw_enabled_slices = dev_priv->wm.skl_hw.ddb.enabled_slices;
> >>> +	u8 required_slices = intel_state->wm_results.ddb.enabled_slices;
> >>> +	struct skl_ddb_entry entries[I915_MAX_PIPES] = {};
> >> Add old_entries as well, merge master + slave
> > I didnt understand what you meant by merge master+slaves? You mean add also the 
> > master and slave that are already enabled?
> 
> Instead of 2 separate allocations, only have a single allocation that contains the slave and master
> ddb during modeset/fastset.

So I will call this master_slave_entries[I915_MAX_PIPES] and have a separate for loop for
ddb allocations of the master and slaves that involved in the current modeset correct?

if (new_crtc_state->active && needs_modeset() && master_or_slave)
	Add to master_slave_entries

Sounds good?

Now this call if (skl_ddb_allocation_overlaps(&cstate->wm.skl.ddb)) will have to be done for if( is_trans_port_sync_mode(cstate))
Now the overlap will check if each non modeset crtc entries overlap with master+slave together, if they do just continue if not then
we call all the 4 for loops as is correct? That should fix the allocations issue?

updated and entries should then add mask and entries for both master +slave correct?

Also the last part of the loop where we wait for a vblank is not clear, do we need that at all?

Manasi

> 
> This will allow it to be updated as a single crtc. This is useful for modeset enable/disable as a single sequence, and could
> potentiallybe useful for normal page flips as well to reduce tearing.
> 
> >>> +
> >>> +	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i)
> >>> +		/* ignore allocations for crtc's that have been turned off. */
> >>> +		if (new_crtc_state->active)
> >>> +			entries[i] = to_intel_crtc_state(old_crtc_state)->wm.skl.ddb;
> >> Can be changed to: if (new_crtc_state->active && !needs_modeset(new_crtc_state)) ?
> > We need !needs_modeset() also? That was not intially there in the original skl_update_crtcs(), 
> > so why do we need it here?
> 
> It's not really needed, a minor optimization.
> 
> If needs_modeset is true, we can be guaranteed that we are always enabling, so the initial DDB allocation is always zero.
> 
> >
> >> Small refinement to the algorithm in general, I dislike the current handling.
> >>
> >> What I would do:
> >> - Make a new_entries array as well, that contains (for the master crtc, during modeset only) master_ddb.begin + slave_ddb.end,
> >>   and nothing for the slave ddb in that case, the ddb allocation for all other cases including master/slave non-modeset should be the default wm.skl.ddb.
> >>   Use it for comparing instead of the one from crtc_state. This way we know we can always enable both at the same time.
> > So in the the case of modeset on master or slave, we shd create another entries similar to default one and have the allocations for master + slave and make sure that doesnt overlap with the already active crtc allocations?
> That's the idea. :)
> >
> >> - Ignore the slave crtc when needs_modeset() is true, called from master instead.
> >> - If a modeset happens on master crtc, do the special enabling dance on both in a separate function.
> > So you are saying that if it is slave crtc and needs_modeset just skip and dont do anything,
> > But in case of master crtc modeset, thats where we will need these additional 4 loops and thats where we access the slave crtcs from the slave_sync_mask and then do the correct enabling sequence etc?
> >  
> >> - Also ensure that the slave crtc is marked as updated, and update both entries to correct values in the entries again.
> > This again I am not 100% clear on how to implement might need to discuss on IRC
> >
> >> You should be able to get the slave crtc with intel_get_crtc_for_pipe(dev_priv, intel_crtc->pipe + 1);
> > But the problem is that ther could be multiple slaves not just 1 and IMO accessing the slaves during the master modeset is more complicated
> > where as the current logic takes care of handling the correct enabling sequence for any number of slaves and master and modesets
> > in any order.
> > Will still have to check for proper ddb allocations and ensure no overlap.
> >
> Yeah but with the ddb allocations you make sure that we can always use the correct order. Because the master + slave ddb are sequential, if an allocation that encompasses both doesn't overlap, we know for sure we can just do both. :) 
> 
> ~Maarten
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 1/8] drm/i915/display: Rename update_crtcs() to commit_modeset_enables()

[Manasi Navare]

Ville/Maarten/Daniel, could you review this, this is just a rename no functional change
as suggested by Danvet and Bille on IRC.
So that I dont have to keep on ebasing this and it can be merged to base my transcoder port
sync patches on top of this.

Manasi

On Mon, Jun 24, 2019 at 02:08:43PM -0700, Manasi Navare wrote:
> This patch has no functional changes. This just renames the update_crtcs()
> hooks to commit_modeset_enables() to match the drm_atomic helper naming
> conventions.
> 
> Suggested-by: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Matt Roper <matthew.d.roper@intel.com>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_display.c | 10 +++++-----
>  drivers/gpu/drm/i915/i915_drv.h              |  2 +-
>  2 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index 73fe1bcfcd99..71e86e2f0f90 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -13613,7 +13613,7 @@ static void intel_update_crtc(struct drm_crtc *crtc,
>  	intel_finish_crtc_commit(to_intel_atomic_state(state), intel_crtc);
>  }
>  
> -static void intel_update_crtcs(struct drm_atomic_state *state)
> +static void intel_commit_modeset_enables(struct drm_atomic_state *state)
>  {
>  	struct drm_crtc *crtc;
>  	struct drm_crtc_state *old_crtc_state, *new_crtc_state;
> @@ -13628,7 +13628,7 @@ static void intel_update_crtcs(struct drm_atomic_state *state)
>  	}
>  }
>  
> -static void skl_update_crtcs(struct drm_atomic_state *state)
> +static void skl_commit_modeset_enables(struct drm_atomic_state *state)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(state->dev);
>  	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
> @@ -13868,7 +13868,7 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
>  	}
>  
>  	/* Now enable the clocks, plane, pipe, and connectors that we set up. */
> -	dev_priv->display.update_crtcs(state);
> +	dev_priv->display.commit_modeset_enables(state);
>  
>  	if (intel_state->modeset)
>  		intel_set_cdclk_post_plane_update(dev_priv,
> @@ -15719,9 +15719,9 @@ void intel_init_display_hooks(struct drm_i915_private *dev_priv)
>  	}
>  
>  	if (INTEL_GEN(dev_priv) >= 9)
> -		dev_priv->display.update_crtcs = skl_update_crtcs;
> +		dev_priv->display.commit_modeset_enables = skl_commit_modeset_enables;
>  	else
> -		dev_priv->display.update_crtcs = intel_update_crtcs;
> +		dev_priv->display.commit_modeset_enables = intel_commit_modeset_enables;
>  }
>  
>  static i915_reg_t i915_vgacntrl_reg(struct drm_i915_private *dev_priv)
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 62e7c5e8aee5..075d7eb3c3f2 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -310,7 +310,7 @@ struct drm_i915_display_funcs {
>  			    struct drm_atomic_state *old_state);
>  	void (*crtc_disable)(struct intel_crtc_state *old_crtc_state,
>  			     struct drm_atomic_state *old_state);
> -	void (*update_crtcs)(struct drm_atomic_state *state);
> +	void (*commit_modeset_enables)(struct drm_atomic_state *state);
>  	void (*audio_codec_enable)(struct intel_encoder *encoder,
>  				   const struct intel_crtc_state *crtc_state,
>  				   const struct drm_connector_state *conn_state);
> -- 
> 2.19.1
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v4 2/8] drm/i915/display: Move the commit_tail() disable sequence to commit_modeset_disables() hook

[Manasi Navare]

Lucas/Maarten/Daniel, could you please review this so this can be upstreamed
and I can base my tran port sync series on top without carrying this over?

This is a cleanup patch with no functional change and addresses the review
commen from Daniel.

Manasi

On Thu, Jun 27, 2019 at 03:57:36PM -0700, Manasi Navare wrote:
> Create a new hook commit_modeset_disables() consistent with the naming
> in drm atomic helpers and similar to the commit_modeset_enables() hook.
> This helps better organize the disable sequence in atomic_commit_tail()
> and move that to this disable hook.
> 
> No functional change
> 
> v2:
> * Create a helper for old_crtc_state disables (Lucas)
> 
> Suggested-by: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Matt Roper <matthew.d.roper@intel.com>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_display.c | 116 ++++++++++++-------
>  drivers/gpu/drm/i915/i915_drv.h              |   1 +
>  2 files changed, 75 insertions(+), 42 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index 71e86e2f0f90..4b088fa24aad 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -13613,6 +13613,69 @@ static void intel_update_crtc(struct drm_crtc *crtc,
>  	intel_finish_crtc_commit(to_intel_atomic_state(state), intel_crtc);
>  }
>  
> +static void intel_old_crtc_state_disables(struct drm_atomic_state *state,
> +					  struct intel_crtc_state *old_intel_crtc_state,
> +					  struct intel_crtc_state *new_intel_crtc_state,
> +					  struct intel_crtc *intel_crtc)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(state->dev);
> +	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
> +	struct drm_crtc_state *new_crtc_state = &new_intel_crtc_state->base;
> +
> +	intel_crtc_disable_planes(intel_state, intel_crtc);
> +
> +	/*
> +	 * We need to disable pipe CRC before disabling the pipe,
> +	 * or we race against vblank off.
> +	 */
> +	intel_crtc_disable_pipe_crc(intel_crtc);
> +
> +	dev_priv->display.crtc_disable(old_intel_crtc_state, state);
> +	intel_crtc->active = false;
> +	intel_fbc_disable(intel_crtc);
> +	intel_disable_shared_dpll(old_intel_crtc_state);
> +
> +	/*
> +	 * Underruns don't always raise interrupts,
> +	 * so check manually.
> +	 */
> +	intel_check_cpu_fifo_underruns(dev_priv);
> +	intel_check_pch_fifo_underruns(dev_priv);
> +
> +	/* FIXME unify this for all platforms */
> +	if (!new_crtc_state->active &&
> +	    !HAS_GMCH(dev_priv) &&
> +	    dev_priv->display.initial_watermarks)
> +		dev_priv->display.initial_watermarks(intel_state,
> +						     new_intel_crtc_state);
> +}
> +
> +static void intel_commit_modeset_disables(struct drm_atomic_state *state)
> +{
> +	struct drm_crtc_state *old_crtc_state, *new_crtc_state;
> +	struct intel_crtc_state *new_intel_crtc_state, *old_intel_crtc_state;
> +	struct drm_crtc *crtc;
> +	struct intel_crtc *intel_crtc;
> +	int i;
> +
> +	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
> +		old_intel_crtc_state = to_intel_crtc_state(old_crtc_state);
> +		new_intel_crtc_state = to_intel_crtc_state(new_crtc_state);
> +		intel_crtc = to_intel_crtc(crtc);
> +
> +		if (!needs_modeset(new_crtc_state))
> +			continue;
> +
> +		intel_pre_plane_update(old_intel_crtc_state, new_intel_crtc_state);
> +
> +		if (old_crtc_state->active)
> +			intel_old_crtc_state_disables(state,
> +						      old_intel_crtc_state,
> +						      new_intel_crtc_state,
> +						      intel_crtc);
> +	}
> +}
> +
>  static void intel_commit_modeset_enables(struct drm_atomic_state *state)
>  {
>  	struct drm_crtc *crtc;
> @@ -13769,7 +13832,7 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
>  	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  	struct drm_crtc_state *old_crtc_state, *new_crtc_state;
> -	struct intel_crtc_state *new_intel_crtc_state, *old_intel_crtc_state;
> +	struct intel_crtc_state *new_intel_crtc_state;
>  	struct drm_crtc *crtc;
>  	struct intel_crtc *intel_crtc;
>  	u64 put_domains[I915_MAX_PIPES] = {};
> @@ -13778,58 +13841,24 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
>  
>  	intel_atomic_commit_fence_wait(intel_state);
>  
> -	drm_atomic_helper_wait_for_dependencies(state);
> -
> -	if (intel_state->modeset)
> -		wakeref = intel_display_power_get(dev_priv, POWER_DOMAIN_MODESET);
> -
>  	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
> -		old_intel_crtc_state = to_intel_crtc_state(old_crtc_state);
>  		new_intel_crtc_state = to_intel_crtc_state(new_crtc_state);
>  		intel_crtc = to_intel_crtc(crtc);
>  
>  		if (needs_modeset(new_crtc_state) ||
> -		    to_intel_crtc_state(new_crtc_state)->update_pipe) {
> -
> +		    new_intel_crtc_state->update_pipe) {
>  			put_domains[intel_crtc->pipe] =
>  				modeset_get_crtc_power_domains(crtc,
> -					new_intel_crtc_state);
> +							       new_intel_crtc_state);
>  		}
> +	}
>  
> -		if (!needs_modeset(new_crtc_state))
> -			continue;
> -
> -		intel_pre_plane_update(old_intel_crtc_state, new_intel_crtc_state);
> -
> -		if (old_crtc_state->active) {
> -			intel_crtc_disable_planes(intel_state, intel_crtc);
> -
> -			/*
> -			 * We need to disable pipe CRC before disabling the pipe,
> -			 * or we race against vblank off.
> -			 */
> -			intel_crtc_disable_pipe_crc(intel_crtc);
> +	drm_atomic_helper_wait_for_dependencies(state);
>  
> -			dev_priv->display.crtc_disable(old_intel_crtc_state, state);
> -			intel_crtc->active = false;
> -			intel_fbc_disable(intel_crtc);
> -			intel_disable_shared_dpll(old_intel_crtc_state);
> +	if (intel_state->modeset)
> +		wakeref = intel_display_power_get(dev_priv, POWER_DOMAIN_MODESET);
>  
> -			/*
> -			 * Underruns don't always raise
> -			 * interrupts, so check manually.
> -			 */
> -			intel_check_cpu_fifo_underruns(dev_priv);
> -			intel_check_pch_fifo_underruns(dev_priv);
> -
> -			/* FIXME unify this for all platforms */
> -			if (!new_crtc_state->active &&
> -			    !HAS_GMCH(dev_priv) &&
> -			    dev_priv->display.initial_watermarks)
> -				dev_priv->display.initial_watermarks(intel_state,
> -								     new_intel_crtc_state);
> -		}
> -	}
> +	dev_priv->display.commit_modeset_disables(state);
>  
>  	/* FIXME: Eventually get rid of our intel_crtc->config pointer */
>  	for_each_new_crtc_in_state(state, crtc, new_crtc_state, i)
> @@ -15722,6 +15751,9 @@ void intel_init_display_hooks(struct drm_i915_private *dev_priv)
>  		dev_priv->display.commit_modeset_enables = skl_commit_modeset_enables;
>  	else
>  		dev_priv->display.commit_modeset_enables = intel_commit_modeset_enables;
> +
> +	dev_priv->display.commit_modeset_disables = intel_commit_modeset_disables;
> +
>  }
>  
>  static i915_reg_t i915_vgacntrl_reg(struct drm_i915_private *dev_priv)
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 075d7eb3c3f2..edb6b431f90c 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -311,6 +311,7 @@ struct drm_i915_display_funcs {
>  	void (*crtc_disable)(struct intel_crtc_state *old_crtc_state,
>  			     struct drm_atomic_state *old_state);
>  	void (*commit_modeset_enables)(struct drm_atomic_state *state);
> +	void (*commit_modeset_disables)(struct drm_atomic_state *state);
>  	void (*audio_codec_enable)(struct intel_encoder *encoder,
>  				   const struct intel_crtc_state *crtc_state,
>  				   const struct drm_connector_state *conn_state);
> -- 
> 2.19.1
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 1/8] drm/i915/display: Rename update_crtcs() to commit_modeset_enables()

[Tolakanahalli Pradeep, Madhumitha]

On Mon, 2019-06-24 at 14:08 -0700, Manasi Navare wrote:
> This patch has no functional changes. This just renames the
> update_crtcs()
> hooks to commit_modeset_enables() to match the drm_atomic helper
> naming
> conventions.
> 
> Suggested-by: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Matt Roper <matthew.d.roper@intel.com>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>


Looks good to me.

Reviewed-by: Madhumitha Tolakanahalli Pradeep <
madhumitha.tolakanahalli.pradeep@intel.com>

Madhumitha

> ---
>  drivers/gpu/drm/i915/display/intel_display.c | 10 +++++-----
>  drivers/gpu/drm/i915/i915_drv.h              |  2 +-
>  2 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> b/drivers/gpu/drm/i915/display/intel_display.c
> index 73fe1bcfcd99..71e86e2f0f90 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -13613,7 +13613,7 @@ static void intel_update_crtc(struct drm_crtc
> *crtc,
>  	intel_finish_crtc_commit(to_intel_atomic_state(state),
> intel_crtc);
>  }
>  
> -static void intel_update_crtcs(struct drm_atomic_state *state)
> +static void intel_commit_modeset_enables(struct drm_atomic_state
> *state)
>  {
>  	struct drm_crtc *crtc;
>  	struct drm_crtc_state *old_crtc_state, *new_crtc_state;
> @@ -13628,7 +13628,7 @@ static void intel_update_crtcs(struct
> drm_atomic_state *state)
>  	}
>  }
>  
> -static void skl_update_crtcs(struct drm_atomic_state *state)
> +static void skl_commit_modeset_enables(struct drm_atomic_state
> *state)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(state->dev);
>  	struct intel_atomic_state *intel_state =
> to_intel_atomic_state(state);
> @@ -13868,7 +13868,7 @@ static void intel_atomic_commit_tail(struct
> drm_atomic_state *state)
>  	}
>  
>  	/* Now enable the clocks, plane, pipe, and connectors that we
> set up. */
> -	dev_priv->display.update_crtcs(state);
> +	dev_priv->display.commit_modeset_enables(state);
>  
>  	if (intel_state->modeset)
>  		intel_set_cdclk_post_plane_update(dev_priv,
> @@ -15719,9 +15719,9 @@ void intel_init_display_hooks(struct
> drm_i915_private *dev_priv)
>  	}
>  
>  	if (INTEL_GEN(dev_priv) >= 9)
> -		dev_priv->display.update_crtcs = skl_update_crtcs;
> +		dev_priv->display.commit_modeset_enables =
> skl_commit_modeset_enables;
>  	else
> -		dev_priv->display.update_crtcs = intel_update_crtcs;
> +		dev_priv->display.commit_modeset_enables =
> intel_commit_modeset_enables;
>  }
>  
>  static i915_reg_t i915_vgacntrl_reg(struct drm_i915_private
> *dev_priv)
> diff --git a/drivers/gpu/drm/i915/i915_drv.h
> b/drivers/gpu/drm/i915/i915_drv.h
> index 62e7c5e8aee5..075d7eb3c3f2 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -310,7 +310,7 @@ struct drm_i915_display_funcs {
>  			    struct drm_atomic_state *old_state);
>  	void (*crtc_disable)(struct intel_crtc_state *old_crtc_state,
>  			     struct drm_atomic_state *old_state);
> -	void (*update_crtcs)(struct drm_atomic_state *state);
> +	void (*commit_modeset_enables)(struct drm_atomic_state *state);
>  	void (*audio_codec_enable)(struct intel_encoder *encoder,
>  				   const struct intel_crtc_state
> *crtc_state,
>  				   const struct drm_connector_state
> *conn_state);
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 6/8] drm/i915/display/icl: Enable master-slaves in trans port sync mode in correct order

[Manasi Navare]

Hi Maarten,

For this patch, you want me to modify it such that if (slave && needs_modeset) then
dont do anything since the slave update crtc and pipe an dplane updates will happen with master.

So if(master && needs_modeset) {
obtain slaves from slave_mask
obtain corresponding slave crtc state
Now update crtc and link train for slaves first
then master
then upstae planes, pipe_start, pipe_end
}

Is this is the correct logic that addresses your review comments and aligns
with the 8K 2p1p approach?

Manasi



On Thu, Aug 01, 2019 at 05:07:48PM +0200, Maarten Lankhorst wrote:
> Op 01-08-2019 om 01:24 schreef Manasi Navare:
> > Thanks Maarten for your review comments, please see my responses/questions below:
> >
> > On Tue, Jul 30, 2019 at 12:53:30PM +0200, Maarten Lankhorst wrote:
> >> Op 24-06-2019 om 23:08 schreef Manasi Navare:
> >>> As per the display enable sequence, we need to follow the enable sequence
> >>> for slaves first with DP_TP_CTL set to Idle and configure the transcoder
> >>> port sync register to select the corersponding master, then follow the
> >>> enable sequence for master leaving DP_TP_CTL to idle.
> >>> At this point the transcoder port sync mode is configured and enabled
> >>> and the Vblanks of both ports are synchronized so then set DP_TP_CTL
> >>> for the slave and master to Normal and do post crtc enable updates.
> >>>
> >>> v2:
> >>> * Create a icl_update_crtcs hook (Maarten, Danvet)
> >>> * This sequence only for CRTCs in trans port sync mode (Maarten)
> >>>
> >>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> >>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >>> Cc: Matt Roper <matthew.d.roper@intel.com>
> >>> Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> >>> ---
> >>>  drivers/gpu/drm/i915/display/intel_ddi.c     |   3 +-
> >>>  drivers/gpu/drm/i915/display/intel_display.c | 217 ++++++++++++++++++-
> >>>  drivers/gpu/drm/i915/display/intel_display.h |   4 +
> >>>  3 files changed, 221 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
> >>> index 7925a176f900..bceb7e4b1877 100644
> >>> --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> >>> +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> >>> @@ -3154,7 +3154,8 @@ static void intel_ddi_pre_enable_dp(struct intel_encoder *encoder,
> >>>  					      true);
> >>>  	intel_dp_sink_set_fec_ready(intel_dp, crtc_state);
> >>>  	intel_dp_start_link_train(intel_dp);
> >>> -	if (port != PORT_A || INTEL_GEN(dev_priv) >= 9)
> >>> +	if ((port != PORT_A || INTEL_GEN(dev_priv) >= 9) &&
> >>> +	    !is_trans_port_sync_mode(crtc_state))
> >>>  		intel_dp_stop_link_train(intel_dp);
> >>>  
> >>>  	intel_ddi_enable_fec(encoder, crtc_state);
> >>> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> >>> index 7156b1b4c6c5..f88d3a929e36 100644
> >>> --- a/drivers/gpu/drm/i915/display/intel_display.c
> >>> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> >>> @@ -520,6 +520,26 @@ needs_modeset(const struct drm_crtc_state *state)
> >>>  	return drm_atomic_crtc_needs_modeset(state);
> >>>  }
> >>>  
> >>> +bool
> >>> +is_trans_port_sync_mode(const struct intel_crtc_state *state)
> >>> +{
> >>> +	return (state->master_transcoder != INVALID_TRANSCODER ||
> >>> +		state->sync_mode_slaves_mask);
> >>> +}
> >>> +
> >>> +static bool
> >>> +is_trans_port_sync_slave(const struct intel_crtc_state *state)
> >>> +{
> >>> +	return state->master_transcoder != INVALID_TRANSCODER;
> >>> +}
> >>> +
> >>> +static bool
> >>> +is_trans_port_sync_master(const struct intel_crtc_state *state)
> >>> +{
> >>> +	return (state->master_transcoder == INVALID_TRANSCODER &&
> >>> +		state->sync_mode_slaves_mask);
> >>> +}
> >>> +
> >>>  /*
> >>>   * Platform specific helpers to calculate the port PLL loopback- (clock.m),
> >>>   * and post-divider (clock.p) values, pre- (clock.vco) and post-divided fast
> >>> @@ -13944,9 +13964,200 @@ static void skl_commit_modeset_enables(struct drm_atomic_state *state)
> >>>  			progress = true;
> >>>  		}
> >>>  	} while (progress);
> >>> +}
> >>>  
> >>> +static void icl_commit_modeset_enables(struct drm_atomic_state *state)
> >>> +{
> >>> +	struct drm_i915_private *dev_priv = to_i915(state->dev);
> >>> +	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
> >>> +	struct drm_crtc *crtc;
> >>> +	struct intel_crtc *intel_crtc;
> >>> +	struct drm_crtc_state *old_crtc_state, *new_crtc_state;
> >>> +	struct intel_crtc_state *cstate;
> >>> +	unsigned int updated = 0;
> >>> +	bool progress;
> >>> +	enum pipe pipe;
> >>> +	int i;
> >>> +	u8 hw_enabled_slices = dev_priv->wm.skl_hw.ddb.enabled_slices;
> >>> +	u8 required_slices = intel_state->wm_results.ddb.enabled_slices;
> >>> +	struct skl_ddb_entry entries[I915_MAX_PIPES] = {};
> >> Add old_entries as well, merge master + slave
> > I didnt understand what you meant by merge master+slaves? You mean add also the 
> > master and slave that are already enabled?
> 
> Instead of 2 separate allocations, only have a single allocation that contains the slave and master
> ddb during modeset/fastset.
> 
> This will allow it to be updated as a single crtc. This is useful for modeset enable/disable as a single sequence, and could
> potentiallybe useful for normal page flips as well to reduce tearing.
> 
> >>> +
> >>> +	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i)
> >>> +		/* ignore allocations for crtc's that have been turned off. */
> >>> +		if (new_crtc_state->active)
> >>> +			entries[i] = to_intel_crtc_state(old_crtc_state)->wm.skl.ddb;
> >> Can be changed to: if (new_crtc_state->active && !needs_modeset(new_crtc_state)) ?
> > We need !needs_modeset() also? That was not intially there in the original skl_update_crtcs(), 
> > so why do we need it here?
> 
> It's not really needed, a minor optimization.
> 
> If needs_modeset is true, we can be guaranteed that we are always enabling, so the initial DDB allocation is always zero.
> 
> >
> >> Small refinement to the algorithm in general, I dislike the current handling.
> >>
> >> What I would do:
> >> - Make a new_entries array as well, that contains (for the master crtc, during modeset only) master_ddb.begin + slave_ddb.end,
> >>   and nothing for the slave ddb in that case, the ddb allocation for all other cases including master/slave non-modeset should be the default wm.skl.ddb.
> >>   Use it for comparing instead of the one from crtc_state. This way we know we can always enable both at the same time.
> > So in the the case of modeset on master or slave, we shd create another entries similar to default one and have the allocations for master + slave and make sure that doesnt overlap with the already active crtc allocations?
> That's the idea. :)
> >
> >> - Ignore the slave crtc when needs_modeset() is true, called from master instead.
> >> - If a modeset happens on master crtc, do the special enabling dance on both in a separate function.
> > So you are saying that if it is slave crtc and needs_modeset just skip and dont do anything,
> > But in case of master crtc modeset, thats where we will need these additional 4 loops and thats where we access the slave crtcs from the slave_sync_mask and then do the correct enabling sequence etc?
> >  
> >> - Also ensure that the slave crtc is marked as updated, and update both entries to correct values in the entries again.
> > This again I am not 100% clear on how to implement might need to discuss on IRC
> >
> >> You should be able to get the slave crtc with intel_get_crtc_for_pipe(dev_priv, intel_crtc->pipe + 1);
> > But the problem is that ther could be multiple slaves not just 1 and IMO accessing the slaves during the master modeset is more complicated
> > where as the current logic takes care of handling the correct enabling sequence for any number of slaves and master and modesets
> > in any order.
> > Will still have to check for proper ddb allocations and ensure no overlap.
> >
> Yeah but with the ddb allocations you make sure that we can always use the correct order. Because the master + slave ddb are sequential, if an allocation that encompasses both doesn't overlap, we know for sure we can just do both. :) 
> 
> ~Maarten
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 1/8] drm/i915/display: Rename update_crtcs() to commit_modeset_enables()

[Manasi Navare]

Ville/Maarten/Daniel,

This is a simple rename patch as suggested by danvet, can any one of you review this?
So that rest of Maarten's patches can also use these new functions.

Manasi



On Mon, Jun 24, 2019 at 02:08:43PM -0700, Manasi Navare wrote:
> This patch has no functional changes. This just renames the update_crtcs()
> hooks to commit_modeset_enables() to match the drm_atomic helper naming
> conventions.
> 
> Suggested-by: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Matt Roper <matthew.d.roper@intel.com>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_display.c | 10 +++++-----
>  drivers/gpu/drm/i915/i915_drv.h              |  2 +-
>  2 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index 73fe1bcfcd99..71e86e2f0f90 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -13613,7 +13613,7 @@ static void intel_update_crtc(struct drm_crtc *crtc,
>  	intel_finish_crtc_commit(to_intel_atomic_state(state), intel_crtc);
>  }
>  
> -static void intel_update_crtcs(struct drm_atomic_state *state)
> +static void intel_commit_modeset_enables(struct drm_atomic_state *state)
>  {
>  	struct drm_crtc *crtc;
>  	struct drm_crtc_state *old_crtc_state, *new_crtc_state;
> @@ -13628,7 +13628,7 @@ static void intel_update_crtcs(struct drm_atomic_state *state)
>  	}
>  }
>  
> -static void skl_update_crtcs(struct drm_atomic_state *state)
> +static void skl_commit_modeset_enables(struct drm_atomic_state *state)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(state->dev);
>  	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
> @@ -13868,7 +13868,7 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
>  	}
>  
>  	/* Now enable the clocks, plane, pipe, and connectors that we set up. */
> -	dev_priv->display.update_crtcs(state);
> +	dev_priv->display.commit_modeset_enables(state);
>  
>  	if (intel_state->modeset)
>  		intel_set_cdclk_post_plane_update(dev_priv,
> @@ -15719,9 +15719,9 @@ void intel_init_display_hooks(struct drm_i915_private *dev_priv)
>  	}
>  
>  	if (INTEL_GEN(dev_priv) >= 9)
> -		dev_priv->display.update_crtcs = skl_update_crtcs;
> +		dev_priv->display.commit_modeset_enables = skl_commit_modeset_enables;
>  	else
> -		dev_priv->display.update_crtcs = intel_update_crtcs;
> +		dev_priv->display.commit_modeset_enables = intel_commit_modeset_enables;
>  }
>  
>  static i915_reg_t i915_vgacntrl_reg(struct drm_i915_private *dev_priv)
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 62e7c5e8aee5..075d7eb3c3f2 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -310,7 +310,7 @@ struct drm_i915_display_funcs {
>  			    struct drm_atomic_state *old_state);
>  	void (*crtc_disable)(struct intel_crtc_state *old_crtc_state,
>  			     struct drm_atomic_state *old_state);
> -	void (*update_crtcs)(struct drm_atomic_state *state);
> +	void (*commit_modeset_enables)(struct drm_atomic_state *state);
>  	void (*audio_codec_enable)(struct intel_encoder *encoder,
>  				   const struct intel_crtc_state *crtc_state,
>  				   const struct drm_connector_state *conn_state);
> -- 
> 2.19.1
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v4 2/8] drm/i915/display: Move the commit_tail() disable sequence to commit_modeset_disables() hook

[Manasi Navare]

Ville/Maarten/Daniel,

Could you please review this? There is no functional changes
just moving the disable part to a separate hook

Manasi


On Thu, Jun 27, 2019 at 03:57:36PM -0700, Manasi Navare wrote:
> Create a new hook commit_modeset_disables() consistent with the naming
> in drm atomic helpers and similar to the commit_modeset_enables() hook.
> This helps better organize the disable sequence in atomic_commit_tail()
> and move that to this disable hook.
> 
> No functional change
> 
> v2:
> * Create a helper for old_crtc_state disables (Lucas)
> 
> Suggested-by: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Matt Roper <matthew.d.roper@intel.com>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_display.c | 116 ++++++++++++-------
>  drivers/gpu/drm/i915/i915_drv.h              |   1 +
>  2 files changed, 75 insertions(+), 42 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index 71e86e2f0f90..4b088fa24aad 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -13613,6 +13613,69 @@ static void intel_update_crtc(struct drm_crtc *crtc,
>  	intel_finish_crtc_commit(to_intel_atomic_state(state), intel_crtc);
>  }
>  
> +static void intel_old_crtc_state_disables(struct drm_atomic_state *state,
> +					  struct intel_crtc_state *old_intel_crtc_state,
> +					  struct intel_crtc_state *new_intel_crtc_state,
> +					  struct intel_crtc *intel_crtc)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(state->dev);
> +	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
> +	struct drm_crtc_state *new_crtc_state = &new_intel_crtc_state->base;
> +
> +	intel_crtc_disable_planes(intel_state, intel_crtc);
> +
> +	/*
> +	 * We need to disable pipe CRC before disabling the pipe,
> +	 * or we race against vblank off.
> +	 */
> +	intel_crtc_disable_pipe_crc(intel_crtc);
> +
> +	dev_priv->display.crtc_disable(old_intel_crtc_state, state);
> +	intel_crtc->active = false;
> +	intel_fbc_disable(intel_crtc);
> +	intel_disable_shared_dpll(old_intel_crtc_state);
> +
> +	/*
> +	 * Underruns don't always raise interrupts,
> +	 * so check manually.
> +	 */
> +	intel_check_cpu_fifo_underruns(dev_priv);
> +	intel_check_pch_fifo_underruns(dev_priv);
> +
> +	/* FIXME unify this for all platforms */
> +	if (!new_crtc_state->active &&
> +	    !HAS_GMCH(dev_priv) &&
> +	    dev_priv->display.initial_watermarks)
> +		dev_priv->display.initial_watermarks(intel_state,
> +						     new_intel_crtc_state);
> +}
> +
> +static void intel_commit_modeset_disables(struct drm_atomic_state *state)
> +{
> +	struct drm_crtc_state *old_crtc_state, *new_crtc_state;
> +	struct intel_crtc_state *new_intel_crtc_state, *old_intel_crtc_state;
> +	struct drm_crtc *crtc;
> +	struct intel_crtc *intel_crtc;
> +	int i;
> +
> +	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
> +		old_intel_crtc_state = to_intel_crtc_state(old_crtc_state);
> +		new_intel_crtc_state = to_intel_crtc_state(new_crtc_state);
> +		intel_crtc = to_intel_crtc(crtc);
> +
> +		if (!needs_modeset(new_crtc_state))
> +			continue;
> +
> +		intel_pre_plane_update(old_intel_crtc_state, new_intel_crtc_state);
> +
> +		if (old_crtc_state->active)
> +			intel_old_crtc_state_disables(state,
> +						      old_intel_crtc_state,
> +						      new_intel_crtc_state,
> +						      intel_crtc);
> +	}
> +}
> +
>  static void intel_commit_modeset_enables(struct drm_atomic_state *state)
>  {
>  	struct drm_crtc *crtc;
> @@ -13769,7 +13832,7 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
>  	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  	struct drm_crtc_state *old_crtc_state, *new_crtc_state;
> -	struct intel_crtc_state *new_intel_crtc_state, *old_intel_crtc_state;
> +	struct intel_crtc_state *new_intel_crtc_state;
>  	struct drm_crtc *crtc;
>  	struct intel_crtc *intel_crtc;
>  	u64 put_domains[I915_MAX_PIPES] = {};
> @@ -13778,58 +13841,24 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
>  
>  	intel_atomic_commit_fence_wait(intel_state);
>  
> -	drm_atomic_helper_wait_for_dependencies(state);
> -
> -	if (intel_state->modeset)
> -		wakeref = intel_display_power_get(dev_priv, POWER_DOMAIN_MODESET);
> -
>  	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
> -		old_intel_crtc_state = to_intel_crtc_state(old_crtc_state);
>  		new_intel_crtc_state = to_intel_crtc_state(new_crtc_state);
>  		intel_crtc = to_intel_crtc(crtc);
>  
>  		if (needs_modeset(new_crtc_state) ||
> -		    to_intel_crtc_state(new_crtc_state)->update_pipe) {
> -
> +		    new_intel_crtc_state->update_pipe) {
>  			put_domains[intel_crtc->pipe] =
>  				modeset_get_crtc_power_domains(crtc,
> -					new_intel_crtc_state);
> +							       new_intel_crtc_state);
>  		}
> +	}
>  
> -		if (!needs_modeset(new_crtc_state))
> -			continue;
> -
> -		intel_pre_plane_update(old_intel_crtc_state, new_intel_crtc_state);
> -
> -		if (old_crtc_state->active) {
> -			intel_crtc_disable_planes(intel_state, intel_crtc);
> -
> -			/*
> -			 * We need to disable pipe CRC before disabling the pipe,
> -			 * or we race against vblank off.
> -			 */
> -			intel_crtc_disable_pipe_crc(intel_crtc);
> +	drm_atomic_helper_wait_for_dependencies(state);
>  
> -			dev_priv->display.crtc_disable(old_intel_crtc_state, state);
> -			intel_crtc->active = false;
> -			intel_fbc_disable(intel_crtc);
> -			intel_disable_shared_dpll(old_intel_crtc_state);
> +	if (intel_state->modeset)
> +		wakeref = intel_display_power_get(dev_priv, POWER_DOMAIN_MODESET);
>  
> -			/*
> -			 * Underruns don't always raise
> -			 * interrupts, so check manually.
> -			 */
> -			intel_check_cpu_fifo_underruns(dev_priv);
> -			intel_check_pch_fifo_underruns(dev_priv);
> -
> -			/* FIXME unify this for all platforms */
> -			if (!new_crtc_state->active &&
> -			    !HAS_GMCH(dev_priv) &&
> -			    dev_priv->display.initial_watermarks)
> -				dev_priv->display.initial_watermarks(intel_state,
> -								     new_intel_crtc_state);
> -		}
> -	}
> +	dev_priv->display.commit_modeset_disables(state);
>  
>  	/* FIXME: Eventually get rid of our intel_crtc->config pointer */
>  	for_each_new_crtc_in_state(state, crtc, new_crtc_state, i)
> @@ -15722,6 +15751,9 @@ void intel_init_display_hooks(struct drm_i915_private *dev_priv)
>  		dev_priv->display.commit_modeset_enables = skl_commit_modeset_enables;
>  	else
>  		dev_priv->display.commit_modeset_enables = intel_commit_modeset_enables;
> +
> +	dev_priv->display.commit_modeset_disables = intel_commit_modeset_disables;
> +
>  }
>  
>  static i915_reg_t i915_vgacntrl_reg(struct drm_i915_private *dev_priv)
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 075d7eb3c3f2..edb6b431f90c 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -311,6 +311,7 @@ struct drm_i915_display_funcs {
>  	void (*crtc_disable)(struct intel_crtc_state *old_crtc_state,
>  			     struct drm_atomic_state *old_state);
>  	void (*commit_modeset_enables)(struct drm_atomic_state *state);
> +	void (*commit_modeset_disables)(struct drm_atomic_state *state);
>  	void (*audio_codec_enable)(struct intel_encoder *encoder,
>  				   const struct intel_crtc_state *crtc_state,
>  				   const struct drm_connector_state *conn_state);
> -- 
> 2.19.1
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 1/8] drm/i915/display: Rename update_crtcs() to commit_modeset_enables()

[Jani Nikula]

On Thu, 22 Aug 2019, Manasi Navare <manasi.d.navare@intel.com> wrote:
> Ville/Maarten/Daniel,
>
> This is a simple rename patch as suggested by danvet, can any one of
> you review this?  So that rest of Maarten's patches can also use these
> new functions.

Please repost the patches. They were posted two months ago, and never
got full IGT results from CI. No idea if they'll apply anymore.

BR,
Jani.



>
> Manasi
>
>
>
> On Mon, Jun 24, 2019 at 02:08:43PM -0700, Manasi Navare wrote:
>> This patch has no functional changes. This just renames the update_crtcs()
>> hooks to commit_modeset_enables() to match the drm_atomic helper naming
>> conventions.
>> 
>> Suggested-by: Daniel Vetter <daniel.vetter@intel.com>
>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> Cc: Matt Roper <matthew.d.roper@intel.com>
>> Cc: Jani Nikula <jani.nikula@intel.com>
>> Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
>> ---
>>  drivers/gpu/drm/i915/display/intel_display.c | 10 +++++-----
>>  drivers/gpu/drm/i915/i915_drv.h              |  2 +-
>>  2 files changed, 6 insertions(+), 6 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
>> index 73fe1bcfcd99..71e86e2f0f90 100644
>> --- a/drivers/gpu/drm/i915/display/intel_display.c
>> +++ b/drivers/gpu/drm/i915/display/intel_display.c
>> @@ -13613,7 +13613,7 @@ static void intel_update_crtc(struct drm_crtc *crtc,
>>  	intel_finish_crtc_commit(to_intel_atomic_state(state), intel_crtc);
>>  }
>>  
>> -static void intel_update_crtcs(struct drm_atomic_state *state)
>> +static void intel_commit_modeset_enables(struct drm_atomic_state *state)
>>  {
>>  	struct drm_crtc *crtc;
>>  	struct drm_crtc_state *old_crtc_state, *new_crtc_state;
>> @@ -13628,7 +13628,7 @@ static void intel_update_crtcs(struct drm_atomic_state *state)
>>  	}
>>  }
>>  
>> -static void skl_update_crtcs(struct drm_atomic_state *state)
>> +static void skl_commit_modeset_enables(struct drm_atomic_state *state)
>>  {
>>  	struct drm_i915_private *dev_priv = to_i915(state->dev);
>>  	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
>> @@ -13868,7 +13868,7 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
>>  	}
>>  
>>  	/* Now enable the clocks, plane, pipe, and connectors that we set up. */
>> -	dev_priv->display.update_crtcs(state);
>> +	dev_priv->display.commit_modeset_enables(state);
>>  
>>  	if (intel_state->modeset)
>>  		intel_set_cdclk_post_plane_update(dev_priv,
>> @@ -15719,9 +15719,9 @@ void intel_init_display_hooks(struct drm_i915_private *dev_priv)
>>  	}
>>  
>>  	if (INTEL_GEN(dev_priv) >= 9)
>> -		dev_priv->display.update_crtcs = skl_update_crtcs;
>> +		dev_priv->display.commit_modeset_enables = skl_commit_modeset_enables;
>>  	else
>> -		dev_priv->display.update_crtcs = intel_update_crtcs;
>> +		dev_priv->display.commit_modeset_enables = intel_commit_modeset_enables;
>>  }
>>  
>>  static i915_reg_t i915_vgacntrl_reg(struct drm_i915_private *dev_priv)
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index 62e7c5e8aee5..075d7eb3c3f2 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -310,7 +310,7 @@ struct drm_i915_display_funcs {
>>  			    struct drm_atomic_state *old_state);
>>  	void (*crtc_disable)(struct intel_crtc_state *old_crtc_state,
>>  			     struct drm_atomic_state *old_state);
>> -	void (*update_crtcs)(struct drm_atomic_state *state);
>> +	void (*commit_modeset_enables)(struct drm_atomic_state *state);
>>  	void (*audio_codec_enable)(struct intel_encoder *encoder,
>>  				   const struct intel_crtc_state *crtc_state,
>>  				   const struct drm_connector_state *conn_state);
>> -- 
>> 2.19.1
>> 

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

Re: [PATCH v3 1/8] drm/i915/display: Rename update_crtcs() to commit_modeset_enables()

[Manasi Navare]

On Fri, Aug 23, 2019 at 09:20:38AM +0300, Jani Nikula wrote:
> On Thu, 22 Aug 2019, Manasi Navare <manasi.d.navare@intel.com> wrote:
> > Ville/Maarten/Daniel,
> >
> > This is a simple rename patch as suggested by danvet, can any one of
> > you review this?  So that rest of Maarten's patches can also use these
> > new functions.
> 
> Please repost the patches. They were posted two months ago, and never
> got full IGT results from CI. No idea if they'll apply anymore.
> 
> BR,
> Jani.

I have rebased them in my local tree but was working on review comments on Patch 8.
But I guess I can break down the series and repost these first 2 refactor no func change
pathes first

Manasi

> 
> 
> 
> >
> > Manasi
> >
> >
> >
> > On Mon, Jun 24, 2019 at 02:08:43PM -0700, Manasi Navare wrote:
> >> This patch has no functional changes. This just renames the update_crtcs()
> >> hooks to commit_modeset_enables() to match the drm_atomic helper naming
> >> conventions.
> >> 
> >> Suggested-by: Daniel Vetter <daniel.vetter@intel.com>
> >> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >> Cc: Matt Roper <matthew.d.roper@intel.com>
> >> Cc: Jani Nikula <jani.nikula@intel.com>
> >> Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/display/intel_display.c | 10 +++++-----
> >>  drivers/gpu/drm/i915/i915_drv.h              |  2 +-
> >>  2 files changed, 6 insertions(+), 6 deletions(-)
> >> 
> >> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> >> index 73fe1bcfcd99..71e86e2f0f90 100644
> >> --- a/drivers/gpu/drm/i915/display/intel_display.c
> >> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> >> @@ -13613,7 +13613,7 @@ static void intel_update_crtc(struct drm_crtc *crtc,
> >>  	intel_finish_crtc_commit(to_intel_atomic_state(state), intel_crtc);
> >>  }
> >>  
> >> -static void intel_update_crtcs(struct drm_atomic_state *state)
> >> +static void intel_commit_modeset_enables(struct drm_atomic_state *state)
> >>  {
> >>  	struct drm_crtc *crtc;
> >>  	struct drm_crtc_state *old_crtc_state, *new_crtc_state;
> >> @@ -13628,7 +13628,7 @@ static void intel_update_crtcs(struct drm_atomic_state *state)
> >>  	}
> >>  }
> >>  
> >> -static void skl_update_crtcs(struct drm_atomic_state *state)
> >> +static void skl_commit_modeset_enables(struct drm_atomic_state *state)
> >>  {
> >>  	struct drm_i915_private *dev_priv = to_i915(state->dev);
> >>  	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
> >> @@ -13868,7 +13868,7 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
> >>  	}
> >>  
> >>  	/* Now enable the clocks, plane, pipe, and connectors that we set up. */
> >> -	dev_priv->display.update_crtcs(state);
> >> +	dev_priv->display.commit_modeset_enables(state);
> >>  
> >>  	if (intel_state->modeset)
> >>  		intel_set_cdclk_post_plane_update(dev_priv,
> >> @@ -15719,9 +15719,9 @@ void intel_init_display_hooks(struct drm_i915_private *dev_priv)
> >>  	}
> >>  
> >>  	if (INTEL_GEN(dev_priv) >= 9)
> >> -		dev_priv->display.update_crtcs = skl_update_crtcs;
> >> +		dev_priv->display.commit_modeset_enables = skl_commit_modeset_enables;
> >>  	else
> >> -		dev_priv->display.update_crtcs = intel_update_crtcs;
> >> +		dev_priv->display.commit_modeset_enables = intel_commit_modeset_enables;
> >>  }
> >>  
> >>  static i915_reg_t i915_vgacntrl_reg(struct drm_i915_private *dev_priv)
> >> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> >> index 62e7c5e8aee5..075d7eb3c3f2 100644
> >> --- a/drivers/gpu/drm/i915/i915_drv.h
> >> +++ b/drivers/gpu/drm/i915/i915_drv.h
> >> @@ -310,7 +310,7 @@ struct drm_i915_display_funcs {
> >>  			    struct drm_atomic_state *old_state);
> >>  	void (*crtc_disable)(struct intel_crtc_state *old_crtc_state,
> >>  			     struct drm_atomic_state *old_state);
> >> -	void (*update_crtcs)(struct drm_atomic_state *state);
> >> +	void (*commit_modeset_enables)(struct drm_atomic_state *state);
> >>  	void (*audio_codec_enable)(struct intel_encoder *encoder,
> >>  				   const struct intel_crtc_state *crtc_state,
> >>  				   const struct drm_connector_state *conn_state);
> >> -- 
> >> 2.19.1
> >> 
> 
> -- 
> Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx