All of lore.kernel.org
 help / color / mirror / Atom feed
* [Intel-gfx] [PATCH v2 1/3] drm/i915/dp: Make sure all tiled connectors get added to the state with full modeset
@ 2019-12-19 21:51 Manasi Navare
  2019-12-19 21:51 ` [Intel-gfx] [PATCH v2 2/3] drm/i915/dp: Make port sync mode assignments only if all tiles present Manasi Navare
                   ` (5 more replies)
  0 siblings, 6 replies; 19+ messages in thread
From: Manasi Navare @ 2019-12-19 21:51 UTC (permalink / raw)
  To: intel-gfx

In case of tiled displays, all the tiles are linke dto each other
for transcoder port sync. So in intel_atomic_check() we need to make
sure that we add all the tiles to the modeset and if one of the
tiles needs a full modeset then mark all other tiles for a full modeset.

v2:
* Change crtc_state scope, remove tile_grp_id (Ville)
* Use intel_connector_needs_modeset() (Ville)
* Add modeset_synced_crtcs (Ville)
* Make sure synced crtcs are forced full modeset
after fastset check (Ville)

Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: José Roberto de Souza <jose.souza@intel.com>
Cc: Matt Roper <matthew.d.roper@intel.com>
Bugzilla: https://gitlab.freedesktop.org/drm/intel/issues/5
Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
---
 drivers/gpu/drm/i915/display/intel_display.c | 143 +++++++++++++++++--
 1 file changed, 131 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index a3f9430493ae..00608d8cef50 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -13910,18 +13910,6 @@ static void intel_crtc_check_fastset(const struct intel_crtc_state *old_crtc_sta
 	new_crtc_state->uapi.mode_changed = false;
 	new_crtc_state->update_pipe = true;
 
-	/*
-	 * If we're not doing the full modeset we want to
-	 * keep the current M/N values as they may be
-	 * sufficiently different to the computed values
-	 * to cause problems.
-	 *
-	 * FIXME: should really copy more fuzzy state here
-	 */
-	new_crtc_state->fdi_m_n = old_crtc_state->fdi_m_n;
-	new_crtc_state->dp_m_n = old_crtc_state->dp_m_n;
-	new_crtc_state->dp_m2_n2 = old_crtc_state->dp_m2_n2;
-	new_crtc_state->has_drrs = old_crtc_state->has_drrs;
 }
 
 static int intel_crtc_add_planes_to_state(struct intel_atomic_state *state,
@@ -14032,6 +14020,105 @@ static int intel_atomic_check_crtcs(struct intel_atomic_state *state)
 	return 0;
 }
 
+static void
+intel_dp_modeset_synced_crtcs(struct intel_atomic_state *state)
+{
+	struct intel_crtc_state *new_crtc_state;
+	struct intel_crtc *crtc;
+	int i;
+
+	for_each_new_intel_crtc_in_state(state, crtc,
+					 new_crtc_state, i) {
+		if (is_trans_port_sync_mode(new_crtc_state)) {
+			new_crtc_state->uapi.mode_changed = true;
+			new_crtc_state->update_pipe = false;
+		}
+	}
+}
+
+static void
+intel_dp_atomic_check_synced_crtcs(struct intel_atomic_state *state)
+{
+	struct intel_crtc_state *new_crtc_state;
+	struct intel_crtc *crtc;
+	int i;
+
+	for_each_new_intel_crtc_in_state(state, crtc,
+					 new_crtc_state, i) {
+		if (!is_trans_port_sync_mode(new_crtc_state) ||
+		    !needs_modeset(new_crtc_state))
+			continue;
+
+		intel_dp_modeset_synced_crtcs(state);
+	}
+}
+
+static int
+intel_dp_modeset_all_tiles(struct intel_atomic_state *state, int tile_grp_id)
+{
+	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
+	struct drm_connector *connector;
+	struct drm_connector_list_iter conn_list_iter;
+
+	drm_connector_list_iter_begin(&dev_priv->drm, &conn_list_iter);
+	drm_for_each_connector_iter(connector, &conn_list_iter) {
+		struct drm_connector_state *conn_iter_state;
+		struct drm_crtc_state *crtc_state;
+
+		if (!(connector->has_tile &&
+		      connector->tile_group->id == tile_grp_id))
+			continue;
+		conn_iter_state = drm_atomic_get_connector_state(&state->base,
+								 connector);
+		if (IS_ERR(conn_iter_state)) {
+			drm_connector_list_iter_end(&conn_list_iter);
+			return PTR_ERR(conn_iter_state);
+		}
+
+		if (!conn_iter_state->crtc)
+			continue;
+
+		crtc_state = drm_atomic_get_crtc_state(&state->base,
+						       conn_iter_state->crtc);
+		if (IS_ERR(crtc_state)) {
+			drm_connector_list_iter_end(&conn_list_iter);
+			return PTR_ERR(conn_iter_state);
+		}
+		crtc_state->mode_changed = true;
+	}
+	drm_connector_list_iter_end(&conn_list_iter);
+
+	return 0;
+}
+
+static int
+intel_dp_atomic_check_tiled_conns(struct intel_atomic_state *state)
+{
+	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
+	struct drm_connector *connector;
+	struct drm_connector_state *old_conn_state, *new_conn_state;
+	int i, ret;
+
+	if (INTEL_GEN(dev_priv) < 11)
+		return 0;
+
+	/* Is tiled, mark all other tiled CRTCs as needing a modeset */
+	for_each_oldnew_connector_in_state(&state->base, connector,
+					   old_conn_state, new_conn_state, i) {
+		if (!connector->has_tile)
+			continue;
+		if (!intel_connector_needs_modeset(state, old_conn_state,
+						   new_conn_state))
+			continue;
+
+		ret = intel_dp_modeset_all_tiles(state, connector->tile_group->id);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
 /**
  * intel_atomic_check - validate state object
  * @dev: drm device
@@ -14059,6 +14146,12 @@ static int intel_atomic_check(struct drm_device *dev,
 	if (ret)
 		goto fail;
 
+	ret = intel_dp_atomic_check_tiled_conns(state);
+	if (ret)
+		goto fail;
+
+	intel_dp_atomic_check_synced_crtcs(state);
+
 	for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
 					    new_crtc_state, i) {
 		if (!needs_modeset(new_crtc_state)) {
@@ -14089,6 +14182,32 @@ static int intel_atomic_check(struct drm_device *dev,
 			any_ms = true;
 	}
 
+	/*
+	 * In case of port synced crtcs, if one of the synced crtcs
+	 * needs a full modeset, all other synced crtcs should be
+	 * forced a full modeset.
+	 */
+	intel_dp_atomic_check_synced_crtcs(state);
+
+	for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
+					    new_crtc_state, i) {
+		if (needs_modeset(new_crtc_state))
+			continue;
+
+		/*
+		 * If we're not doing the full modeset we want to
+		 * keep the current M/N values as they may be
+		 * sufficiently different to the computed values
+		 * to cause problems.
+		 *
+		 * FIXME: should really copy more fuzzy state here
+		 */
+		new_crtc_state->fdi_m_n = old_crtc_state->fdi_m_n;
+		new_crtc_state->dp_m_n = old_crtc_state->dp_m_n;
+		new_crtc_state->dp_m2_n2 = old_crtc_state->dp_m2_n2;
+		new_crtc_state->has_drrs = old_crtc_state->has_drrs;
+	}
+
 	if (any_ms && !check_digital_port_conflicts(state)) {
 		DRM_DEBUG_KMS("rejecting conflicting digital port configuration\n");
 		ret = EINVAL;
-- 
2.19.1

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

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [Intel-gfx] [PATCH v2 2/3] drm/i915/dp: Make port sync mode assignments only if all tiles present
  2019-12-19 21:51 [Intel-gfx] [PATCH v2 1/3] drm/i915/dp: Make sure all tiled connectors get added to the state with full modeset Manasi Navare
@ 2019-12-19 21:51 ` Manasi Navare
  2019-12-20 13:59   ` Ville Syrjälä
  2019-12-20 18:13   ` Ville Syrjälä
  2019-12-19 21:51 ` [Intel-gfx] [PATCH v2 3/3] drm/i915/dp: Disable Port sync mode correctly on teardown Manasi Navare
                   ` (4 subsequent siblings)
  5 siblings, 2 replies; 19+ messages in thread
From: Manasi Navare @ 2019-12-19 21:51 UTC (permalink / raw)
  To: intel-gfx

Add an extra check before making master slave assignments for tiled
displays to make sure we make these assignments only if all tiled
connectors are present. If not then initialize the state to defaults
so it does a normal non tiled modeset without transcoder port sync.

v2:
* Rename icl_add_sync_mode_crtcs
* Move this function just before .compute_config hook
* Check if DP before master slave assignments (Ville)

Bugzilla: https://gitlab.freedesktop.org/drm/intel/issues/5
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
---
 drivers/gpu/drm/i915/display/intel_display.c | 162 +++++++++++--------
 1 file changed, 99 insertions(+), 63 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index 00608d8cef50..9c1b1256be68 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -12014,88 +12014,106 @@ 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 intel_crtc_state *crtc_state)
+static void reset_port_sync_mode_state(struct intel_crtc_state *crtc_state)
+{
+	crtc_state->master_transcoder = INVALID_TRANSCODER;
+	crtc_state->sync_mode_slaves_mask = 0;
+}
+
+static int icl_compute_port_sync_crtc_state(struct drm_connector *connector,
+					    struct intel_crtc_state *crtc_state,
+					    int num_tiled_conns)
 {
 	struct drm_crtc *crtc = crtc_state->uapi.crtc;
 	struct intel_atomic_state *state = to_intel_atomic_state(crtc_state->uapi.state);
 	struct drm_i915_private *dev_priv = to_i915(crtc_state->uapi.crtc->dev);
-	struct drm_connector *master_connector, *connector;
-	struct drm_connector_state *connector_state;
+	struct drm_connector *master_connector;
 	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;
 
+	if (!intel_crtc_has_type(crtc_state, INTEL_OUTPUT_DP))
+		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.
+	 * If all tiles not present do not make master slave assignments.
+	 *
+	 * FIXME: Add support for multiple tile grp ids in the future when such
+	 * panels are available.
 	 */
-	for_each_new_connector_in_state(&state->base, connector, connector_state, i) {
-		if (connector_state->crtc != crtc)
-			continue;
-		if (!connector->has_tile)
+	if (!connector->has_tile ||
+	    crtc_state->hw.mode.hdisplay != connector->tile_h_size ||
+	    crtc_state->hw.mode.vdisplay != connector->tile_v_size ||
+	    num_tiled_conns < connector->num_h_tile * connector->num_v_tile) {
+		reset_port_sync_mode_state(crtc_state);
+		return 0;
+	}
+	/* Last Horizontal and last vertical tile connector is a master
+	 * Master Trans for a Master CRTC is always INVALID.
+	 */
+	if (connector->tile_h_loc == connector->num_h_tile - 1 &&
+	    connector->tile_v_loc == connector->num_v_tile - 1) {
+		crtc_state->master_transcoder = INVALID_TRANSCODER;
+		return 0;
+	}
+
+	/* Loop through all connectors and configure the Slave crtc_state
+	 * to point to the correct master.
+	 */
+	reset_port_sync_mode_state(crtc_state);
+	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 &&
+		      master_connector->tile_group->id == connector->tile_group->id))
 			continue;
-		if (crtc_state->hw.mode.hdisplay != connector->tile_h_size ||
-		    crtc_state->hw.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)
+		if (master_connector->tile_h_loc != master_connector->num_h_tile - 1 ||
+		    master_connector->tile_v_loc != master_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->base,
-									   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;
-			}
+		master_conn_state = drm_atomic_get_connector_state(&state->base,
+								   master_connector);
+		if (IS_ERR(master_conn_state)) {
+			drm_connector_list_iter_end(&conn_iter);
+			return PTR_ERR(master_conn_state);
 		}
-		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;
+		if (master_conn_state->crtc) {
+			master_crtc = master_conn_state->crtc;
+			break;
 		}
+	}
+	drm_connector_list_iter_end(&conn_iter);
 
-		master_crtc_state = drm_atomic_get_crtc_state(&state->base,
-							      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->uapi.crtc->base.id,
-			      master_pipe_config->sync_mode_slaves_mask);
+	if (!master_crtc) {
+		DRM_DEBUG_KMS("Could not find Master CRTC for Slave CRTC %d\n",
+			      crtc->base.id);
+		return -EINVAL;
 	}
 
+	master_crtc_state = drm_atomic_get_crtc_state(&state->base,
+						      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->base.id,
+		      master_pipe_config->sync_mode_slaves_mask);
+
 	return 0;
 }
 
@@ -12660,7 +12678,7 @@ intel_modeset_pipe_config(struct intel_crtc_state *pipe_config)
 	struct drm_connector *connector;
 	struct drm_connector_state *connector_state;
 	int base_bpp, ret;
-	int i;
+	int i, tile_group_id = -1, num_tiled_conns = 0;
 	bool retry = true;
 
 	pipe_config->cpu_transcoder =
@@ -12730,13 +12748,23 @@ intel_modeset_pipe_config(struct intel_crtc_state *pipe_config)
 	drm_mode_set_crtcinfo(&pipe_config->hw.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(pipe_config);
-	if (ret) {
-		DRM_DEBUG_KMS("Cannot assign Sync Mode CRTCs: %d\n",
-			      ret);
-		return ret;
+
+	/* Get tile_group_id of tiled connector */
+	for_each_new_connector_in_state(state, connector, connector_state, i) {
+		if (connector_state->crtc == crtc &&
+		    connector->has_tile) {
+			tile_group_id = connector->tile_group->id;
+			break;
+		}
+	}
+
+	/* Get total number of tiled connectors in state that belong to
+	 * this tile group.
+	 */
+	for_each_new_connector_in_state(state, connector, connector_state, i) {
+		if (connector->has_tile &&
+		    connector->tile_group->id == tile_group_id)
+			num_tiled_conns++;
 	}
 
 	/* Pass our mode to the connectors and the CRTC to give them a chance to
@@ -12747,6 +12775,14 @@ intel_modeset_pipe_config(struct intel_crtc_state *pipe_config)
 		if (connector_state->crtc != crtc)
 			continue;
 
+		ret = icl_compute_port_sync_crtc_state(connector, pipe_config,
+						       num_tiled_conns);
+		if (ret) {
+			DRM_DEBUG_KMS("Cannot assign Sync Mode CRTCs: %d\n",
+				      ret);
+			return ret;
+		}
+
 		encoder = to_intel_encoder(connector_state->best_encoder);
 		ret = encoder->compute_config(encoder, pipe_config,
 					      connector_state);
-- 
2.19.1

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

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [Intel-gfx] [PATCH v2 3/3] drm/i915/dp: Disable Port sync mode correctly on teardown
  2019-12-19 21:51 [Intel-gfx] [PATCH v2 1/3] drm/i915/dp: Make sure all tiled connectors get added to the state with full modeset Manasi Navare
  2019-12-19 21:51 ` [Intel-gfx] [PATCH v2 2/3] drm/i915/dp: Make port sync mode assignments only if all tiles present Manasi Navare
@ 2019-12-19 21:51 ` Manasi Navare
  2019-12-20 13:19   ` Ville Syrjälä
  2019-12-19 21:56 ` [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for series starting with [v2,1/3] drm/i915/dp: Make sure all tiled connectors get added to the state with full modeset Patchwork
                   ` (3 subsequent siblings)
  5 siblings, 1 reply; 19+ messages in thread
From: Manasi Navare @ 2019-12-19 21:51 UTC (permalink / raw)
  To: intel-gfx; +Cc: Jani Nikula

While clearing the Ports ync mode enable and master select bits
we need to clear the register completely instead of using disable masks

v2:
* Just write 0 to the reg (Ville)
* Rebase

Bugzilla: https://gitlab.freedesktop.org/drm/intel/issues/5
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Jani Nikula <jani.nikula@intel.com>
Fixes: 51528afe7c5e ("drm/i915/display/icl: Disable transcoder port sync as part of crtc_disable() sequence")
Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
---
 drivers/gpu/drm/i915/display/intel_ddi.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
index c9ba7d7f3787..c484f6df5d87 100644
--- a/drivers/gpu/drm/i915/display/intel_ddi.c
+++ b/drivers/gpu/drm/i915/display/intel_ddi.c
@@ -3861,7 +3861,6 @@ static void icl_disable_transcoder_port_sync(const struct intel_crtc_state *old_
 	struct intel_crtc *crtc = to_intel_crtc(old_crtc_state->uapi.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;
@@ -3870,9 +3869,7 @@ static void icl_disable_transcoder_port_sync(const struct intel_crtc_state *old_
 		      transcoder_name(old_crtc_state->cpu_transcoder));
 
 	reg = TRANS_DDI_FUNC_CTL2(old_crtc_state->cpu_transcoder);
-	trans_ddi_func_ctl2_val = ~(PORT_SYNC_MODE_ENABLE |
-				    PORT_SYNC_MODE_MASTER_SELECT_MASK);
-	I915_WRITE(reg, trans_ddi_func_ctl2_val);
+	I915_WRITE(reg, 0);
 }
 
 static void intel_ddi_post_disable(struct intel_encoder *encoder,
-- 
2.19.1

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

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for series starting with [v2,1/3] drm/i915/dp: Make sure all tiled connectors get added to the state with full modeset
  2019-12-19 21:51 [Intel-gfx] [PATCH v2 1/3] drm/i915/dp: Make sure all tiled connectors get added to the state with full modeset Manasi Navare
  2019-12-19 21:51 ` [Intel-gfx] [PATCH v2 2/3] drm/i915/dp: Make port sync mode assignments only if all tiles present Manasi Navare
  2019-12-19 21:51 ` [Intel-gfx] [PATCH v2 3/3] drm/i915/dp: Disable Port sync mode correctly on teardown Manasi Navare
@ 2019-12-19 21:56 ` Patchwork
  2019-12-19 22:26 ` [Intel-gfx] ✗ Fi.CI.BAT: failure " Patchwork
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 19+ messages in thread
From: Patchwork @ 2019-12-19 21:56 UTC (permalink / raw)
  To: Manasi Navare; +Cc: intel-gfx

== Series Details ==

Series: series starting with [v2,1/3] drm/i915/dp: Make sure all tiled connectors get added to the state with full modeset
URL   : https://patchwork.freedesktop.org/series/71190/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
ccfce3fe03d1 drm/i915/dp: Make sure all tiled connectors get added to the state with full modeset
28ec6e3386c0 drm/i915/dp: Make port sync mode assignments only if all tiles present
-:210: CHECK:LINE_SPACING: Please don't use multiple blank lines
#210: FILE: drivers/gpu/drm/i915/display/intel_display.c:12751:
 
+

total: 0 errors, 0 warnings, 1 checks, 213 lines checked
331d1b560479 drm/i915/dp: Disable Port sync mode correctly on teardown

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* [Intel-gfx] ✗ Fi.CI.BAT: failure for series starting with [v2,1/3] drm/i915/dp: Make sure all tiled connectors get added to the state with full modeset
  2019-12-19 21:51 [Intel-gfx] [PATCH v2 1/3] drm/i915/dp: Make sure all tiled connectors get added to the state with full modeset Manasi Navare
                   ` (2 preceding siblings ...)
  2019-12-19 21:56 ` [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for series starting with [v2,1/3] drm/i915/dp: Make sure all tiled connectors get added to the state with full modeset Patchwork
@ 2019-12-19 22:26 ` Patchwork
  2019-12-20 13:17 ` [Intel-gfx] [PATCH v2 1/3] " Ville Syrjälä
  2019-12-20 18:41 ` Ville Syrjälä
  5 siblings, 0 replies; 19+ messages in thread
From: Patchwork @ 2019-12-19 22:26 UTC (permalink / raw)
  To: Manasi Navare; +Cc: intel-gfx

== Series Details ==

Series: series starting with [v2,1/3] drm/i915/dp: Make sure all tiled connectors get added to the state with full modeset
URL   : https://patchwork.freedesktop.org/series/71190/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_7610 -> Patchwork_15849
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_15849 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_15849, 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_15849/index.html

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

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

### IGT changes ###

#### Possible regressions ####

  * igt@debugfs_test@read_all_entries:
    - fi-hsw-peppy:       [PASS][1] -> [DMESG-WARN][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7610/fi-hsw-peppy/igt@debugfs_test@read_all_entries.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15849/fi-hsw-peppy/igt@debugfs_test@read_all_entries.html

  * igt@gem_exec_suspend@basic-s0:
    - fi-bdw-5557u:       [PASS][3] -> [DMESG-WARN][4]
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7610/fi-bdw-5557u/igt@gem_exec_suspend@basic-s0.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15849/fi-bdw-5557u/igt@gem_exec_suspend@basic-s0.html
    - fi-bsw-n3050:       [PASS][5] -> [DMESG-WARN][6]
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7610/fi-bsw-n3050/igt@gem_exec_suspend@basic-s0.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15849/fi-bsw-n3050/igt@gem_exec_suspend@basic-s0.html
    - fi-byt-n2820:       [PASS][7] -> [DMESG-WARN][8]
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7610/fi-byt-n2820/igt@gem_exec_suspend@basic-s0.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15849/fi-byt-n2820/igt@gem_exec_suspend@basic-s0.html

  * igt@kms_force_connector_basic@force-connector-state:
    - fi-kbl-guc:         [PASS][9] -> [INCOMPLETE][10]
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7610/fi-kbl-guc/igt@kms_force_connector_basic@force-connector-state.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15849/fi-kbl-guc/igt@kms_force_connector_basic@force-connector-state.html

  * igt@runner@aborted:
    - fi-pnv-d510:        NOTRUN -> [FAIL][11]
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15849/fi-pnv-d510/igt@runner@aborted.html
    - fi-hsw-peppy:       NOTRUN -> [FAIL][12]
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15849/fi-hsw-peppy/igt@runner@aborted.html
    - fi-gdg-551:         NOTRUN -> [FAIL][13]
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15849/fi-gdg-551/igt@runner@aborted.html
    - fi-snb-2520m:       NOTRUN -> [FAIL][14]
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15849/fi-snb-2520m/igt@runner@aborted.html
    - fi-byt-n2820:       NOTRUN -> [FAIL][15]
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15849/fi-byt-n2820/igt@runner@aborted.html
    - fi-snb-2600:        NOTRUN -> [FAIL][16]
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15849/fi-snb-2600/igt@runner@aborted.html
    - fi-bxt-dsi:         NOTRUN -> [FAIL][17]
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15849/fi-bxt-dsi/igt@runner@aborted.html
    - fi-elk-e7500:       NOTRUN -> [FAIL][18]
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15849/fi-elk-e7500/igt@runner@aborted.html

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

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

### IGT changes ###

#### Issues hit ####

  * igt@gem_exec_suspend@basic-s0:
    - fi-cml-u2:          [PASS][19] -> [DMESG-WARN][20] ([i915#109])
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7610/fi-cml-u2/igt@gem_exec_suspend@basic-s0.html
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15849/fi-cml-u2/igt@gem_exec_suspend@basic-s0.html
    - fi-kbl-r:           [PASS][21] -> [DMESG-WARN][22] ([i915#109])
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7610/fi-kbl-r/igt@gem_exec_suspend@basic-s0.html
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15849/fi-kbl-r/igt@gem_exec_suspend@basic-s0.html
    - fi-kbl-soraka:      [PASS][23] -> [DMESG-WARN][24] ([i915#109])
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7610/fi-kbl-soraka/igt@gem_exec_suspend@basic-s0.html
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15849/fi-kbl-soraka/igt@gem_exec_suspend@basic-s0.html
    - fi-skl-6600u:       [PASS][25] -> [DMESG-WARN][26] ([i915#109])
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7610/fi-skl-6600u/igt@gem_exec_suspend@basic-s0.html
   [26]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15849/fi-skl-6600u/igt@gem_exec_suspend@basic-s0.html
    - fi-apl-guc:         [PASS][27] -> [DMESG-WARN][28] ([i915#109])
   [27]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7610/fi-apl-guc/igt@gem_exec_suspend@basic-s0.html
   [28]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15849/fi-apl-guc/igt@gem_exec_suspend@basic-s0.html
    - fi-icl-y:           [PASS][29] -> [INCOMPLETE][30] ([i915#140])
   [29]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7610/fi-icl-y/igt@gem_exec_suspend@basic-s0.html
   [30]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15849/fi-icl-y/igt@gem_exec_suspend@basic-s0.html
    - fi-cml-s:           [PASS][31] -> [DMESG-WARN][32] ([i915#109])
   [31]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7610/fi-cml-s/igt@gem_exec_suspend@basic-s0.html
   [32]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15849/fi-cml-s/igt@gem_exec_suspend@basic-s0.html
    - fi-icl-guc:         [PASS][33] -> [DMESG-WARN][34] ([i915#109])
   [33]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7610/fi-icl-guc/igt@gem_exec_suspend@basic-s0.html
   [34]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15849/fi-icl-guc/igt@gem_exec_suspend@basic-s0.html

  
#### Possible fixes ####

  * igt@gem_exec_create@basic:
    - fi-byt-n2820:       [FAIL][35] -> [PASS][36]
   [35]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7610/fi-byt-n2820/igt@gem_exec_create@basic.html
   [36]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15849/fi-byt-n2820/igt@gem_exec_create@basic.html

  
#### Warnings ####

  * igt@runner@aborted:
    - fi-cml-s:           [FAIL][37] ([fdo#111764] / [i915#577]) -> [FAIL][38] ([i915#577])
   [37]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7610/fi-cml-s/igt@runner@aborted.html
   [38]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15849/fi-cml-s/igt@runner@aborted.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#111735]: https://bugs.freedesktop.org/show_bug.cgi?id=111735
  [fdo#111764]: https://bugs.freedesktop.org/show_bug.cgi?id=111764
  [i915#109]: https://gitlab.freedesktop.org/drm/intel/issues/109
  [i915#140]: https://gitlab.freedesktop.org/drm/intel/issues/140
  [i915#577]: https://gitlab.freedesktop.org/drm/intel/issues/577


Participating hosts (46 -> 36)
------------------------------

  Additional (4): fi-ilk-650 fi-tgl-y fi-gdg-551 fi-snb-2520m 
  Missing    (14): fi-hsw-4770r fi-ilk-m540 fi-bdw-samus fi-hsw-4200u fi-skl-guc fi-glk-dsi fi-byt-squawks fi-cfl-guc fi-ctg-p8600 fi-byt-clapper fi-ivb-3770 fi-blb-e6850 fi-icl-dsi fi-skl-6700k2 


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

  * CI: CI-20190529 -> None
  * Linux: CI_DRM_7610 -> Patchwork_15849

  CI-20190529: 20190529
  CI_DRM_7610: 26117c3187bfcdee1f9501ff304f62252681d279 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_5351: e7fdcef72d1d6b3bb9f3003bbc37571959e6e8bb @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_15849: 331d1b560479c79015f34db9d91492b1bcc7e6f2 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

331d1b560479 drm/i915/dp: Disable Port sync mode correctly on teardown
28ec6e3386c0 drm/i915/dp: Make port sync mode assignments only if all tiles present
ccfce3fe03d1 drm/i915/dp: Make sure all tiled connectors get added to the state with full modeset

== Logs ==

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [Intel-gfx] [PATCH v2 1/3] drm/i915/dp: Make sure all tiled connectors get added to the state with full modeset
  2019-12-19 21:51 [Intel-gfx] [PATCH v2 1/3] drm/i915/dp: Make sure all tiled connectors get added to the state with full modeset Manasi Navare
                   ` (3 preceding siblings ...)
  2019-12-19 22:26 ` [Intel-gfx] ✗ Fi.CI.BAT: failure " Patchwork
@ 2019-12-20 13:17 ` Ville Syrjälä
  2019-12-20 16:35   ` Manasi Navare
  2019-12-20 18:41 ` Ville Syrjälä
  5 siblings, 1 reply; 19+ messages in thread
From: Ville Syrjälä @ 2019-12-20 13:17 UTC (permalink / raw)
  To: Manasi Navare; +Cc: intel-gfx

On Thu, Dec 19, 2019 at 01:51:15PM -0800, Manasi Navare wrote:
> In case of tiled displays, all the tiles are linke dto each other
> for transcoder port sync. So in intel_atomic_check() we need to make
> sure that we add all the tiles to the modeset and if one of the
> tiles needs a full modeset then mark all other tiles for a full modeset.
> 
> v2:
> * Change crtc_state scope, remove tile_grp_id (Ville)
> * Use intel_connector_needs_modeset() (Ville)
> * Add modeset_synced_crtcs (Ville)
> * Make sure synced crtcs are forced full modeset
> after fastset check (Ville)
> 
> Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: José Roberto de Souza <jose.souza@intel.com>
> Cc: Matt Roper <matthew.d.roper@intel.com>
> Bugzilla: https://gitlab.freedesktop.org/drm/intel/issues/5
> Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_display.c | 143 +++++++++++++++++--
>  1 file changed, 131 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index a3f9430493ae..00608d8cef50 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -13910,18 +13910,6 @@ static void intel_crtc_check_fastset(const struct intel_crtc_state *old_crtc_sta
>  	new_crtc_state->uapi.mode_changed = false;
>  	new_crtc_state->update_pipe = true;
>  
> -	/*
> -	 * If we're not doing the full modeset we want to
> -	 * keep the current M/N values as they may be
> -	 * sufficiently different to the computed values
> -	 * to cause problems.
> -	 *
> -	 * FIXME: should really copy more fuzzy state here
> -	 */
> -	new_crtc_state->fdi_m_n = old_crtc_state->fdi_m_n;
> -	new_crtc_state->dp_m_n = old_crtc_state->dp_m_n;
> -	new_crtc_state->dp_m2_n2 = old_crtc_state->dp_m2_n2;
> -	new_crtc_state->has_drrs = old_crtc_state->has_drrs;
>  }

The check vs. copy should really be a separate patch. Otherwise we risk
having to revert the whole thing if something goes wrong.

>  
>  static int intel_crtc_add_planes_to_state(struct intel_atomic_state *state,
> @@ -14032,6 +14020,105 @@ static int intel_atomic_check_crtcs(struct intel_atomic_state *state)
>  	return 0;
>  }
>  
> +static void
> +intel_dp_modeset_synced_crtcs(struct intel_atomic_state *state)

I would remove "dp" from the names of all these functions. The fact
that we only enable port sync on DP outputs is just an implementation
detail we don't have/want to care about in higher level code like this.

> +{
> +	struct intel_crtc_state *new_crtc_state;
> +	struct intel_crtc *crtc;
> +	int i;
> +
> +	for_each_new_intel_crtc_in_state(state, crtc,
> +					 new_crtc_state, i) {
> +		if (is_trans_port_sync_mode(new_crtc_state)) {
> +			new_crtc_state->uapi.mode_changed = true;
> +			new_crtc_state->update_pipe = false;
> +		}
> +	}
> +}
> +
> +static void
> +intel_dp_atomic_check_synced_crtcs(struct intel_atomic_state *state)
> +{
> +	struct intel_crtc_state *new_crtc_state;
> +	struct intel_crtc *crtc;
> +	int i;
> +
> +	for_each_new_intel_crtc_in_state(state, crtc,
> +					 new_crtc_state, i) {
> +		if (!is_trans_port_sync_mode(new_crtc_state) ||
> +		    !needs_modeset(new_crtc_state))
> +			continue;
> +
> +		intel_dp_modeset_synced_crtcs(state);
> +	}

This is not sufficient for the pre-compute_config() check. The point is
that we don't yet necessarily have the synced crtcs in the atomic state
so we have to add them. Please have a look in my branch for what I mean
exactly.

For the check between the fastset check vs. copy this here should work.

> +}
> +
> +static int
> +intel_dp_modeset_all_tiles(struct intel_atomic_state *state, int tile_grp_id)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> +	struct drm_connector *connector;
> +	struct drm_connector_list_iter conn_list_iter;

'conn_iter' is the customary name.

> +
> +	drm_connector_list_iter_begin(&dev_priv->drm, &conn_list_iter);
> +	drm_for_each_connector_iter(connector, &conn_list_iter) {
> +		struct drm_connector_state *conn_iter_state;

'conn_state'

> +		struct drm_crtc_state *crtc_state;
> +
> +		if (!(connector->has_tile &&
> +		      connector->tile_group->id == tile_grp_id))
> +			continue;

or maybe !has_tile || tile_grrp != tile_grp

I find that form a bit easier on the eyes. But either will work.

> +		conn_iter_state = drm_atomic_get_connector_state(&state->base,
> +								 connector);
> +		if (IS_ERR(conn_iter_state)) {
> +			drm_connector_list_iter_end(&conn_list_iter);
> +			return PTR_ERR(conn_iter_state);

nit: could just do 'ret = PTR_ERR(); break;'
     and 'int ret=0;' + 'return ret' at the fars ends of the function.

> +		}
> +
> +		if (!conn_iter_state->crtc)
> +			continue;
> +
> +		crtc_state = drm_atomic_get_crtc_state(&state->base,
> +						       conn_iter_state->crtc);
> +		if (IS_ERR(crtc_state)) {
> +			drm_connector_list_iter_end(&conn_list_iter);
> +			return PTR_ERR(conn_iter_state);
> +		}
> +		crtc_state->mode_changed = true;

Missing drm_atomic_add_affected_planes(). We also need that in the
pre-compute_config() check_synced_crtcs().

> +	}
> +	drm_connector_list_iter_end(&conn_list_iter);
> +
> +	return 0;
> +}
> +
> +static int
> +intel_dp_atomic_check_tiled_conns(struct intel_atomic_state *state)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> +	struct drm_connector *connector;
> +	struct drm_connector_state *old_conn_state, *new_conn_state;
> +	int i, ret;
> +
> +	if (INTEL_GEN(dev_priv) < 11)
> +		return 0;
> +
> +	/* Is tiled, mark all other tiled CRTCs as needing a modeset */
> +	for_each_oldnew_connector_in_state(&state->base, connector,
> +					   old_conn_state, new_conn_state, i) {
> +		if (!connector->has_tile)
> +			continue;
> +		if (!intel_connector_needs_modeset(state, old_conn_state,
> +						   new_conn_state))
> +			continue;
> +
> +		ret = intel_dp_modeset_all_tiles(state, connector->tile_group->id);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
>  /**
>   * intel_atomic_check - validate state object
>   * @dev: drm device
> @@ -14059,6 +14146,12 @@ static int intel_atomic_check(struct drm_device *dev,
>  	if (ret)
>  		goto fail;
>  
> +	ret = intel_dp_atomic_check_tiled_conns(state);
> +	if (ret)
> +		goto fail;
> +
> +	intel_dp_atomic_check_synced_crtcs(state);
> +
>  	for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
>  					    new_crtc_state, i) {
>  		if (!needs_modeset(new_crtc_state)) {
> @@ -14089,6 +14182,32 @@ static int intel_atomic_check(struct drm_device *dev,
>  			any_ms = true;
>  	}
>  
> +	/*
> +	 * In case of port synced crtcs, if one of the synced crtcs
> +	 * needs a full modeset, all other synced crtcs should be
> +	 * forced a full modeset.
> +	 */
> +	intel_dp_atomic_check_synced_crtcs(state);
> +
> +	for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
> +					    new_crtc_state, i) {
> +		if (needs_modeset(new_crtc_state))
> +			continue;
> +
> +		/*
> +		 * If we're not doing the full modeset we want to
> +		 * keep the current M/N values as they may be
> +		 * sufficiently different to the computed values
> +		 * to cause problems.
> +		 *
> +		 * FIXME: should really copy more fuzzy state here
> +		 */
> +		new_crtc_state->fdi_m_n = old_crtc_state->fdi_m_n;
> +		new_crtc_state->dp_m_n = old_crtc_state->dp_m_n;
> +		new_crtc_state->dp_m2_n2 = old_crtc_state->dp_m2_n2;
> +		new_crtc_state->has_drrs = old_crtc_state->has_drrs;

Same comment I gave in Jose's patch: Pls keep this in a separate
function. My branch has an example.

> +	}
> +
>  	if (any_ms && !check_digital_port_conflicts(state)) {
>  		DRM_DEBUG_KMS("rejecting conflicting digital port configuration\n");
>  		ret = EINVAL;
> -- 
> 2.19.1

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [Intel-gfx] [PATCH v2 3/3] drm/i915/dp: Disable Port sync mode correctly on teardown
  2019-12-19 21:51 ` [Intel-gfx] [PATCH v2 3/3] drm/i915/dp: Disable Port sync mode correctly on teardown Manasi Navare
@ 2019-12-20 13:19   ` Ville Syrjälä
  0 siblings, 0 replies; 19+ messages in thread
From: Ville Syrjälä @ 2019-12-20 13:19 UTC (permalink / raw)
  To: Manasi Navare; +Cc: Jani Nikula, intel-gfx

On Thu, Dec 19, 2019 at 01:51:17PM -0800, Manasi Navare wrote:
> While clearing the Ports ync mode enable and master select bits
> we need to clear the register completely instead of using disable masks
> 
> v2:
> * Just write 0 to the reg (Ville)
> * Rebase
> 
> Bugzilla: https://gitlab.freedesktop.org/drm/intel/issues/5
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Fixes: 51528afe7c5e ("drm/i915/display/icl: Disable transcoder port sync as part of crtc_disable() sequence")
> Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_ddi.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
> index c9ba7d7f3787..c484f6df5d87 100644
> --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> @@ -3861,7 +3861,6 @@ static void icl_disable_transcoder_port_sync(const struct intel_crtc_state *old_
>  	struct intel_crtc *crtc = to_intel_crtc(old_crtc_state->uapi.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;
> @@ -3870,9 +3869,7 @@ static void icl_disable_transcoder_port_sync(const struct intel_crtc_state *old_
>  		      transcoder_name(old_crtc_state->cpu_transcoder));
>  
>  	reg = TRANS_DDI_FUNC_CTL2(old_crtc_state->cpu_transcoder);

'reg' is rather pointless now.

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> -	trans_ddi_func_ctl2_val = ~(PORT_SYNC_MODE_ENABLE |
> -				    PORT_SYNC_MODE_MASTER_SELECT_MASK);
> -	I915_WRITE(reg, trans_ddi_func_ctl2_val);
> +	I915_WRITE(reg, 0);
>  }
>  
>  static void intel_ddi_post_disable(struct intel_encoder *encoder,
> -- 
> 2.19.1

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [Intel-gfx] [PATCH v2 2/3] drm/i915/dp: Make port sync mode assignments only if all tiles present
  2019-12-19 21:51 ` [Intel-gfx] [PATCH v2 2/3] drm/i915/dp: Make port sync mode assignments only if all tiles present Manasi Navare
@ 2019-12-20 13:59   ` Ville Syrjälä
  2019-12-20 17:00     ` Manasi Navare
  2019-12-20 18:13   ` Ville Syrjälä
  1 sibling, 1 reply; 19+ messages in thread
From: Ville Syrjälä @ 2019-12-20 13:59 UTC (permalink / raw)
  To: Manasi Navare; +Cc: intel-gfx

On Thu, Dec 19, 2019 at 01:51:16PM -0800, Manasi Navare wrote:
> Add an extra check before making master slave assignments for tiled
> displays to make sure we make these assignments only if all tiled
> connectors are present. If not then initialize the state to defaults
> so it does a normal non tiled modeset without transcoder port sync.
> 
> v2:
> * Rename icl_add_sync_mode_crtcs
> * Move this function just before .compute_config hook
> * Check if DP before master slave assignments (Ville)
> 
> Bugzilla: https://gitlab.freedesktop.org/drm/intel/issues/5
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_display.c | 162 +++++++++++--------
>  1 file changed, 99 insertions(+), 63 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index 00608d8cef50..9c1b1256be68 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -12014,88 +12014,106 @@ 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 intel_crtc_state *crtc_state)
> +static void reset_port_sync_mode_state(struct intel_crtc_state *crtc_state)
> +{
> +	crtc_state->master_transcoder = INVALID_TRANSCODER;
> +	crtc_state->sync_mode_slaves_mask = 0;
> +}
> +
> +static int icl_compute_port_sync_crtc_state(struct drm_connector *connector,
> +					    struct intel_crtc_state *crtc_state,
> +					    int num_tiled_conns)
>  {
>  	struct drm_crtc *crtc = crtc_state->uapi.crtc;
>  	struct intel_atomic_state *state = to_intel_atomic_state(crtc_state->uapi.state);
>  	struct drm_i915_private *dev_priv = to_i915(crtc_state->uapi.crtc->dev);
> -	struct drm_connector *master_connector, *connector;
> -	struct drm_connector_state *connector_state;
> +	struct drm_connector *master_connector;
>  	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;
>  
> +	if (!intel_crtc_has_type(crtc_state, INTEL_OUTPUT_DP))
> +		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.
> +	 * If all tiles not present do not make master slave assignments.
> +	 *
> +	 * FIXME: Add support for multiple tile grp ids in the future when such
> +	 * panels are available.

What does that mean? Why would we treat mutliple groups as a single
tiled display?

>  	 */
> -	for_each_new_connector_in_state(&state->base, connector, connector_state, i) {
> -		if (connector_state->crtc != crtc)
> -			continue;
> -		if (!connector->has_tile)
> +	if (!connector->has_tile ||
> +	    crtc_state->hw.mode.hdisplay != connector->tile_h_size ||
> +	    crtc_state->hw.mode.vdisplay != connector->tile_v_size ||
> +	    num_tiled_conns < connector->num_h_tile * connector->num_v_tile) {
> +		reset_port_sync_mode_state(crtc_state);

I really don't understand this back and forth of setting/clearing
the master/slave state. I think the way to make it not convoluted is
to just clear everything in intel_crtc_prepare_cleared_state() and
then just compute everything from scratch.

> +		return 0;
> +	}
> +	/* Last Horizontal and last vertical tile connector is a master
> +	 * Master Trans for a Master CRTC is always INVALID.
> +	 */
> +	if (connector->tile_h_loc == connector->num_h_tile - 1 &&
> +	    connector->tile_v_loc == connector->num_v_tile - 1) {
> +		crtc_state->master_transcoder = INVALID_TRANSCODER;
> +		return 0;
> +	}
> +
> +	/* Loop through all connectors and configure the Slave crtc_state
> +	 * to point to the correct master.
> +	 */
> +	reset_port_sync_mode_state(crtc_state);
> +	drm_connector_list_iter_begin(&dev_priv->drm, &conn_iter);
> +	drm_for_each_connector_iter(master_connector, &conn_iter) {

Why are we all of a sudden iterating through all connectors? I think this
should just iterate the connectors already in the state. The only place
where we want to look at the full connector list is the modeset_all_tiles()
thing.

> +		struct drm_connector_state *master_conn_state = NULL;
> +
> +		if (!(master_connector->has_tile &&
> +		      master_connector->tile_group->id == connector->tile_group->id))
>  			continue;
> -		if (crtc_state->hw.mode.hdisplay != connector->tile_h_size ||
> -		    crtc_state->hw.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)
> +		if (master_connector->tile_h_loc != master_connector->num_h_tile - 1 ||
> +		    master_connector->tile_v_loc != master_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->base,
> -									   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;
> -			}
> +		master_conn_state = drm_atomic_get_connector_state(&state->base,
> +								   master_connector);
> +		if (IS_ERR(master_conn_state)) {
> +			drm_connector_list_iter_end(&conn_iter);
> +			return PTR_ERR(master_conn_state);
>  		}
> -		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;
> +		if (master_conn_state->crtc) {
> +			master_crtc = master_conn_state->crtc;
> +			break;
>  		}
> +	}
> +	drm_connector_list_iter_end(&conn_iter);
>  
> -		master_crtc_state = drm_atomic_get_crtc_state(&state->base,
> -							      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->uapi.crtc->base.id,
> -			      master_pipe_config->sync_mode_slaves_mask);
> +	if (!master_crtc) {
> +		DRM_DEBUG_KMS("Could not find Master CRTC for Slave CRTC %d\n",
> +			      crtc->base.id);
> +		return -EINVAL;
>  	}
>  
> +	master_crtc_state = drm_atomic_get_crtc_state(&state->base,
> +						      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->base.id,
> +		      master_pipe_config->sync_mode_slaves_mask);
> +
>  	return 0;
>  }
>  
> @@ -12660,7 +12678,7 @@ intel_modeset_pipe_config(struct intel_crtc_state *pipe_config)
>  	struct drm_connector *connector;
>  	struct drm_connector_state *connector_state;
>  	int base_bpp, ret;
> -	int i;
> +	int i, tile_group_id = -1, num_tiled_conns = 0;
>  	bool retry = true;
>  
>  	pipe_config->cpu_transcoder =
> @@ -12730,13 +12748,23 @@ intel_modeset_pipe_config(struct intel_crtc_state *pipe_config)
>  	drm_mode_set_crtcinfo(&pipe_config->hw.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(pipe_config);
> -	if (ret) {
> -		DRM_DEBUG_KMS("Cannot assign Sync Mode CRTCs: %d\n",
> -			      ret);
> -		return ret;
> +
> +	/* Get tile_group_id of tiled connector */
> +	for_each_new_connector_in_state(state, connector, connector_state, i) {
> +		if (connector_state->crtc == crtc &&
> +		    connector->has_tile) {
> +			tile_group_id = connector->tile_group->id;
> +			break;
> +		}
> +	}
> +
> +	/* Get total number of tiled connectors in state that belong to
> +	 * this tile group.
> +	 */
> +	for_each_new_connector_in_state(state, connector, connector_state, i) {
> +		if (connector->has_tile &&
> +		    connector->tile_group->id == tile_group_id)
> +			num_tiled_conns++;
>  	}

I don't see why we would need this num_tiled_conns stuff at all. Just
iterate all connectors in the state that belong to the same group.

>  
>  	/* Pass our mode to the connectors and the CRTC to give them a chance to
> @@ -12747,6 +12775,14 @@ intel_modeset_pipe_config(struct intel_crtc_state *pipe_config)
>  		if (connector_state->crtc != crtc)
>  			continue;
>  
> +		ret = icl_compute_port_sync_crtc_state(connector, pipe_config,
> +						       num_tiled_conns);
> +		if (ret) {
> +			DRM_DEBUG_KMS("Cannot assign Sync Mode CRTCs: %d\n",
> +				      ret);
> +			return ret;
> +		}
> +
>  		encoder = to_intel_encoder(connector_state->best_encoder);
>  		ret = encoder->compute_config(encoder, pipe_config,
>  					      connector_state);
> -- 
> 2.19.1

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [Intel-gfx] [PATCH v2 1/3] drm/i915/dp: Make sure all tiled connectors get added to the state with full modeset
  2019-12-20 13:17 ` [Intel-gfx] [PATCH v2 1/3] " Ville Syrjälä
@ 2019-12-20 16:35   ` Manasi Navare
  2019-12-20 16:47     ` Ville Syrjälä
  0 siblings, 1 reply; 19+ messages in thread
From: Manasi Navare @ 2019-12-20 16:35 UTC (permalink / raw)
  To: Ville Syrjälä; +Cc: intel-gfx

On Fri, Dec 20, 2019 at 03:17:27PM +0200, Ville Syrjälä wrote:
> On Thu, Dec 19, 2019 at 01:51:15PM -0800, Manasi Navare wrote:
> > In case of tiled displays, all the tiles are linke dto each other
> > for transcoder port sync. So in intel_atomic_check() we need to make
> > sure that we add all the tiles to the modeset and if one of the
> > tiles needs a full modeset then mark all other tiles for a full modeset.
> > 
> > v2:
> > * Change crtc_state scope, remove tile_grp_id (Ville)
> > * Use intel_connector_needs_modeset() (Ville)
> > * Add modeset_synced_crtcs (Ville)
> > * Make sure synced crtcs are forced full modeset
> > after fastset check (Ville)
> > 
> > Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: José Roberto de Souza <jose.souza@intel.com>
> > Cc: Matt Roper <matthew.d.roper@intel.com>
> > Bugzilla: https://gitlab.freedesktop.org/drm/intel/issues/5
> > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_display.c | 143 +++++++++++++++++--
> >  1 file changed, 131 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > index a3f9430493ae..00608d8cef50 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -13910,18 +13910,6 @@ static void intel_crtc_check_fastset(const struct intel_crtc_state *old_crtc_sta
> >  	new_crtc_state->uapi.mode_changed = false;
> >  	new_crtc_state->update_pipe = true;
> >  
> > -	/*
> > -	 * If we're not doing the full modeset we want to
> > -	 * keep the current M/N values as they may be
> > -	 * sufficiently different to the computed values
> > -	 * to cause problems.
> > -	 *
> > -	 * FIXME: should really copy more fuzzy state here
> > -	 */
> > -	new_crtc_state->fdi_m_n = old_crtc_state->fdi_m_n;
> > -	new_crtc_state->dp_m_n = old_crtc_state->dp_m_n;
> > -	new_crtc_state->dp_m2_n2 = old_crtc_state->dp_m2_n2;
> > -	new_crtc_state->has_drrs = old_crtc_state->has_drrs;
> >  }
> 
> The check vs. copy should really be a separate patch. Otherwise we risk
> having to revert the whole thing if something goes wrong.
>

Yes will sepaarte it out , also I think Jose's series might get merged before mine so..

 
> >  
> >  static int intel_crtc_add_planes_to_state(struct intel_atomic_state *state,
> > @@ -14032,6 +14020,105 @@ static int intel_atomic_check_crtcs(struct intel_atomic_state *state)
> >  	return 0;
> >  }
> >  
> > +static void
> > +intel_dp_modeset_synced_crtcs(struct intel_atomic_state *state)
> 
> I would remove "dp" from the names of all these functions. The fact
> that we only enable port sync on DP outputs is just an implementation
> detail we don't have/want to care about in higher level code like this.
>

Okay will rename
 
> > +{
> > +	struct intel_crtc_state *new_crtc_state;
> > +	struct intel_crtc *crtc;
> > +	int i;
> > +
> > +	for_each_new_intel_crtc_in_state(state, crtc,
> > +					 new_crtc_state, i) {
> > +		if (is_trans_port_sync_mode(new_crtc_state)) {
> > +			new_crtc_state->uapi.mode_changed = true;
> > +			new_crtc_state->update_pipe = false;
> > +		}
> > +	}
> > +}
> > +
> > +static void
> > +intel_dp_atomic_check_synced_crtcs(struct intel_atomic_state *state)
> > +{
> > +	struct intel_crtc_state *new_crtc_state;
> > +	struct intel_crtc *crtc;
> > +	int i;
> > +
> > +	for_each_new_intel_crtc_in_state(state, crtc,
> > +					 new_crtc_state, i) {
> > +		if (!is_trans_port_sync_mode(new_crtc_state) ||
> > +		    !needs_modeset(new_crtc_state))
> > +			continue;
> > +
> > +		intel_dp_modeset_synced_crtcs(state);
> > +	}
> 
> This is not sufficient for the pre-compute_config() check. The point is
> that we don't yet necessarily have the synced crtcs in the atomic state
> so we have to add them. Please have a look in my branch for what I mean
> exactly.
>

But if I use old crtc state or new crtc state here, I do see that since we havent
cleared the state yet, we do have all synced crtcs in the state , like here after I disconnect 1 conn,
I do see both conns in trans port sync mode and i can set the mode changed to true for both.

I dont understand why what you mean by we would beed to add them?
 
> For the check between the fastset check vs. copy this here should work.
> 
> > +}
> > +
> > +static int
> > +intel_dp_modeset_all_tiles(struct intel_atomic_state *state, int tile_grp_id)
> > +{
> > +	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> > +	struct drm_connector *connector;
> > +	struct drm_connector_list_iter conn_list_iter;
> 
> 'conn_iter' is the customary name.
>

ok
 
> > +
> > +	drm_connector_list_iter_begin(&dev_priv->drm, &conn_list_iter);
> > +	drm_for_each_connector_iter(connector, &conn_list_iter) {
> > +		struct drm_connector_state *conn_iter_state;
> 
> 'conn_state'
>

ok
 
> > +		struct drm_crtc_state *crtc_state;
> > +
> > +		if (!(connector->has_tile &&
> > +		      connector->tile_group->id == tile_grp_id))
> > +			continue;
> 
> or maybe !has_tile || tile_grrp != tile_grp
> 
> I find that form a bit easier on the eyes. But either will work.
>

Ok
 
> > +		conn_iter_state = drm_atomic_get_connector_state(&state->base,
> > +								 connector);
> > +		if (IS_ERR(conn_iter_state)) {
> > +			drm_connector_list_iter_end(&conn_list_iter);
> > +			return PTR_ERR(conn_iter_state);
> 
> nit: could just do 'ret = PTR_ERR(); break;'
>      and 'int ret=0;' + 'return ret' at the fars ends of the function.
>

ok
 
> > +		}
> > +
> > +		if (!conn_iter_state->crtc)
> > +			continue;
> > +
> > +		crtc_state = drm_atomic_get_crtc_state(&state->base,
> > +						       conn_iter_state->crtc);
> > +		if (IS_ERR(crtc_state)) {
> > +			drm_connector_list_iter_end(&conn_list_iter);
> > +			return PTR_ERR(conn_iter_state);
> > +		}
> > +		crtc_state->mode_changed = true;
> 
> Missing drm_atomic_add_affected_planes(). We also need that in the
> pre-compute_config() check_synced_crtcs().
>

Manasi


Oh we need to call this function along with mode_changed = true?
 
> > +	}
> > +	drm_connector_list_iter_end(&conn_list_iter);
> > +
> > +	return 0;
> > +}
> > +
> > +static int
> > +intel_dp_atomic_check_tiled_conns(struct intel_atomic_state *state)
> > +{
> > +	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> > +	struct drm_connector *connector;
> > +	struct drm_connector_state *old_conn_state, *new_conn_state;
> > +	int i, ret;
> > +
> > +	if (INTEL_GEN(dev_priv) < 11)
> > +		return 0;
> > +
> > +	/* Is tiled, mark all other tiled CRTCs as needing a modeset */
> > +	for_each_oldnew_connector_in_state(&state->base, connector,
> > +					   old_conn_state, new_conn_state, i) {
> > +		if (!connector->has_tile)
> > +			continue;
> > +		if (!intel_connector_needs_modeset(state, old_conn_state,
> > +						   new_conn_state))
> > +			continue;
> > +
> > +		ret = intel_dp_modeset_all_tiles(state, connector->tile_group->id);
> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> >  /**
> >   * intel_atomic_check - validate state object
> >   * @dev: drm device
> > @@ -14059,6 +14146,12 @@ static int intel_atomic_check(struct drm_device *dev,
> >  	if (ret)
> >  		goto fail;
> >  
> > +	ret = intel_dp_atomic_check_tiled_conns(state);
> > +	if (ret)
> > +		goto fail;
> > +
> > +	intel_dp_atomic_check_synced_crtcs(state);
> > +
> >  	for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
> >  					    new_crtc_state, i) {
> >  		if (!needs_modeset(new_crtc_state)) {
> > @@ -14089,6 +14182,32 @@ static int intel_atomic_check(struct drm_device *dev,
> >  			any_ms = true;
> >  	}
> >  
> > +	/*
> > +	 * In case of port synced crtcs, if one of the synced crtcs
> > +	 * needs a full modeset, all other synced crtcs should be
> > +	 * forced a full modeset.
> > +	 */
> > +	intel_dp_atomic_check_synced_crtcs(state);
> > +
> > +	for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
> > +					    new_crtc_state, i) {
> > +		if (needs_modeset(new_crtc_state))
> > +			continue;
> > +
> > +		/*
> > +		 * If we're not doing the full modeset we want to
> > +		 * keep the current M/N values as they may be
> > +		 * sufficiently different to the computed values
> > +		 * to cause problems.
> > +		 *
> > +		 * FIXME: should really copy more fuzzy state here
> > +		 */
> > +		new_crtc_state->fdi_m_n = old_crtc_state->fdi_m_n;
> > +		new_crtc_state->dp_m_n = old_crtc_state->dp_m_n;
> > +		new_crtc_state->dp_m2_n2 = old_crtc_state->dp_m2_n2;
> > +		new_crtc_state->has_drrs = old_crtc_state->has_drrs;
> 
> Same comment I gave in Jose's patch: Pls keep this in a separate
> function. My branch has an example.
> 
> > +	}
> > +
> >  	if (any_ms && !check_digital_port_conflicts(state)) {
> >  		DRM_DEBUG_KMS("rejecting conflicting digital port configuration\n");
> >  		ret = EINVAL;
> > -- 
> > 2.19.1
> 
> -- 
> Ville Syrjälä
> Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [Intel-gfx] [PATCH v2 1/3] drm/i915/dp: Make sure all tiled connectors get added to the state with full modeset
  2019-12-20 16:35   ` Manasi Navare
@ 2019-12-20 16:47     ` Ville Syrjälä
  2019-12-20 17:04       ` Manasi Navare
  0 siblings, 1 reply; 19+ messages in thread
From: Ville Syrjälä @ 2019-12-20 16:47 UTC (permalink / raw)
  To: Manasi Navare; +Cc: intel-gfx

On Fri, Dec 20, 2019 at 08:35:58AM -0800, Manasi Navare wrote:
> On Fri, Dec 20, 2019 at 03:17:27PM +0200, Ville Syrjälä wrote:
> > On Thu, Dec 19, 2019 at 01:51:15PM -0800, Manasi Navare wrote:
> > > In case of tiled displays, all the tiles are linke dto each other
> > > for transcoder port sync. So in intel_atomic_check() we need to make
> > > sure that we add all the tiles to the modeset and if one of the
> > > tiles needs a full modeset then mark all other tiles for a full modeset.
> > > 
> > > v2:
> > > * Change crtc_state scope, remove tile_grp_id (Ville)
> > > * Use intel_connector_needs_modeset() (Ville)
> > > * Add modeset_synced_crtcs (Ville)
> > > * Make sure synced crtcs are forced full modeset
> > > after fastset check (Ville)
> > > 
> > > Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Cc: José Roberto de Souza <jose.souza@intel.com>
> > > Cc: Matt Roper <matthew.d.roper@intel.com>
> > > Bugzilla: https://gitlab.freedesktop.org/drm/intel/issues/5
> > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_display.c | 143 +++++++++++++++++--
> > >  1 file changed, 131 insertions(+), 12 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > > index a3f9430493ae..00608d8cef50 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > @@ -13910,18 +13910,6 @@ static void intel_crtc_check_fastset(const struct intel_crtc_state *old_crtc_sta
> > >  	new_crtc_state->uapi.mode_changed = false;
> > >  	new_crtc_state->update_pipe = true;
> > >  
> > > -	/*
> > > -	 * If we're not doing the full modeset we want to
> > > -	 * keep the current M/N values as they may be
> > > -	 * sufficiently different to the computed values
> > > -	 * to cause problems.
> > > -	 *
> > > -	 * FIXME: should really copy more fuzzy state here
> > > -	 */
> > > -	new_crtc_state->fdi_m_n = old_crtc_state->fdi_m_n;
> > > -	new_crtc_state->dp_m_n = old_crtc_state->dp_m_n;
> > > -	new_crtc_state->dp_m2_n2 = old_crtc_state->dp_m2_n2;
> > > -	new_crtc_state->has_drrs = old_crtc_state->has_drrs;
> > >  }
> > 
> > The check vs. copy should really be a separate patch. Otherwise we risk
> > having to revert the whole thing if something goes wrong.
> >
> 
> Yes will sepaarte it out , also I think Jose's series might get merged before mine so..
> 
>  
> > >  
> > >  static int intel_crtc_add_planes_to_state(struct intel_atomic_state *state,
> > > @@ -14032,6 +14020,105 @@ static int intel_atomic_check_crtcs(struct intel_atomic_state *state)
> > >  	return 0;
> > >  }
> > >  
> > > +static void
> > > +intel_dp_modeset_synced_crtcs(struct intel_atomic_state *state)
> > 
> > I would remove "dp" from the names of all these functions. The fact
> > that we only enable port sync on DP outputs is just an implementation
> > detail we don't have/want to care about in higher level code like this.
> >
> 
> Okay will rename
>  
> > > +{
> > > +	struct intel_crtc_state *new_crtc_state;
> > > +	struct intel_crtc *crtc;
> > > +	int i;
> > > +
> > > +	for_each_new_intel_crtc_in_state(state, crtc,
> > > +					 new_crtc_state, i) {
> > > +		if (is_trans_port_sync_mode(new_crtc_state)) {
> > > +			new_crtc_state->uapi.mode_changed = true;
> > > +			new_crtc_state->update_pipe = false;
> > > +		}
> > > +	}
> > > +}
> > > +
> > > +static void
> > > +intel_dp_atomic_check_synced_crtcs(struct intel_atomic_state *state)
> > > +{
> > > +	struct intel_crtc_state *new_crtc_state;
> > > +	struct intel_crtc *crtc;
> > > +	int i;
> > > +
> > > +	for_each_new_intel_crtc_in_state(state, crtc,
> > > +					 new_crtc_state, i) {
> > > +		if (!is_trans_port_sync_mode(new_crtc_state) ||
> > > +		    !needs_modeset(new_crtc_state))
> > > +			continue;
> > > +
> > > +		intel_dp_modeset_synced_crtcs(state);
> > > +	}
> > 
> > This is not sufficient for the pre-compute_config() check. The point is
> > that we don't yet necessarily have the synced crtcs in the atomic state
> > so we have to add them. Please have a look in my branch for what I mean
> > exactly.
> >
> 
> But if I use old crtc state or new crtc state here, I do see that since we havent
> cleared the state yet, we do have all synced crtcs in the state , like here after I disconnect 1 conn,
> I do see both conns in trans port sync mode and i can set the mode changed to true for both.
> 
> I dont understand why what you mean by we would beed to add them?

for_each_new_intel_crtc_in_state() only iterates the crtcs we've already
added to the state, not all crtcs. The whole point of the
pre-compute_config() check is that we add the ones that were previously
synced but weren't yet added to the state. This for the case where the
tile info has gone poof so the function that adds stuff based on the
tile info will have missed them.

>  
> > For the check between the fastset check vs. copy this here should work.
> > 
> > > +}
> > > +
> > > +static int
> > > +intel_dp_modeset_all_tiles(struct intel_atomic_state *state, int tile_grp_id)
> > > +{
> > > +	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> > > +	struct drm_connector *connector;
> > > +	struct drm_connector_list_iter conn_list_iter;
> > 
> > 'conn_iter' is the customary name.
> >
> 
> ok
>  
> > > +
> > > +	drm_connector_list_iter_begin(&dev_priv->drm, &conn_list_iter);
> > > +	drm_for_each_connector_iter(connector, &conn_list_iter) {
> > > +		struct drm_connector_state *conn_iter_state;
> > 
> > 'conn_state'
> >
> 
> ok
>  
> > > +		struct drm_crtc_state *crtc_state;
> > > +
> > > +		if (!(connector->has_tile &&
> > > +		      connector->tile_group->id == tile_grp_id))
> > > +			continue;
> > 
> > or maybe !has_tile || tile_grrp != tile_grp
> > 
> > I find that form a bit easier on the eyes. But either will work.
> >
> 
> Ok
>  
> > > +		conn_iter_state = drm_atomic_get_connector_state(&state->base,
> > > +								 connector);
> > > +		if (IS_ERR(conn_iter_state)) {
> > > +			drm_connector_list_iter_end(&conn_list_iter);
> > > +			return PTR_ERR(conn_iter_state);
> > 
> > nit: could just do 'ret = PTR_ERR(); break;'
> >      and 'int ret=0;' + 'return ret' at the fars ends of the function.
> >
> 
> ok
>  
> > > +		}
> > > +
> > > +		if (!conn_iter_state->crtc)
> > > +			continue;
> > > +
> > > +		crtc_state = drm_atomic_get_crtc_state(&state->base,
> > > +						       conn_iter_state->crtc);
> > > +		if (IS_ERR(crtc_state)) {
> > > +			drm_connector_list_iter_end(&conn_list_iter);
> > > +			return PTR_ERR(conn_iter_state);
> > > +		}
> > > +		crtc_state->mode_changed = true;
> > 
> > Missing drm_atomic_add_affected_planes(). We also need that in the
> > pre-compute_config() check_synced_crtcs().
> >
> 
> Manasi
> 
> 
> Oh we need to call this function along with mode_changed = true?
>  
> > > +	}
> > > +	drm_connector_list_iter_end(&conn_list_iter);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int
> > > +intel_dp_atomic_check_tiled_conns(struct intel_atomic_state *state)
> > > +{
> > > +	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> > > +	struct drm_connector *connector;
> > > +	struct drm_connector_state *old_conn_state, *new_conn_state;
> > > +	int i, ret;
> > > +
> > > +	if (INTEL_GEN(dev_priv) < 11)
> > > +		return 0;
> > > +
> > > +	/* Is tiled, mark all other tiled CRTCs as needing a modeset */
> > > +	for_each_oldnew_connector_in_state(&state->base, connector,
> > > +					   old_conn_state, new_conn_state, i) {
> > > +		if (!connector->has_tile)
> > > +			continue;
> > > +		if (!intel_connector_needs_modeset(state, old_conn_state,
> > > +						   new_conn_state))
> > > +			continue;
> > > +
> > > +		ret = intel_dp_modeset_all_tiles(state, connector->tile_group->id);
> > > +		if (ret)
> > > +			return ret;
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +
> > >  /**
> > >   * intel_atomic_check - validate state object
> > >   * @dev: drm device
> > > @@ -14059,6 +14146,12 @@ static int intel_atomic_check(struct drm_device *dev,
> > >  	if (ret)
> > >  		goto fail;
> > >  
> > > +	ret = intel_dp_atomic_check_tiled_conns(state);
> > > +	if (ret)
> > > +		goto fail;
> > > +
> > > +	intel_dp_atomic_check_synced_crtcs(state);
> > > +
> > >  	for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
> > >  					    new_crtc_state, i) {
> > >  		if (!needs_modeset(new_crtc_state)) {
> > > @@ -14089,6 +14182,32 @@ static int intel_atomic_check(struct drm_device *dev,
> > >  			any_ms = true;
> > >  	}
> > >  
> > > +	/*
> > > +	 * In case of port synced crtcs, if one of the synced crtcs
> > > +	 * needs a full modeset, all other synced crtcs should be
> > > +	 * forced a full modeset.
> > > +	 */
> > > +	intel_dp_atomic_check_synced_crtcs(state);
> > > +
> > > +	for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
> > > +					    new_crtc_state, i) {
> > > +		if (needs_modeset(new_crtc_state))
> > > +			continue;
> > > +
> > > +		/*
> > > +		 * If we're not doing the full modeset we want to
> > > +		 * keep the current M/N values as they may be
> > > +		 * sufficiently different to the computed values
> > > +		 * to cause problems.
> > > +		 *
> > > +		 * FIXME: should really copy more fuzzy state here
> > > +		 */
> > > +		new_crtc_state->fdi_m_n = old_crtc_state->fdi_m_n;
> > > +		new_crtc_state->dp_m_n = old_crtc_state->dp_m_n;
> > > +		new_crtc_state->dp_m2_n2 = old_crtc_state->dp_m2_n2;
> > > +		new_crtc_state->has_drrs = old_crtc_state->has_drrs;
> > 
> > Same comment I gave in Jose's patch: Pls keep this in a separate
> > function. My branch has an example.
> > 
> > > +	}
> > > +
> > >  	if (any_ms && !check_digital_port_conflicts(state)) {
> > >  		DRM_DEBUG_KMS("rejecting conflicting digital port configuration\n");
> > >  		ret = EINVAL;
> > > -- 
> > > 2.19.1
> > 
> > -- 
> > Ville Syrjälä
> > Intel

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [Intel-gfx] [PATCH v2 2/3] drm/i915/dp: Make port sync mode assignments only if all tiles present
  2019-12-20 13:59   ` Ville Syrjälä
@ 2019-12-20 17:00     ` Manasi Navare
  2019-12-20 17:37       ` Ville Syrjälä
  0 siblings, 1 reply; 19+ messages in thread
From: Manasi Navare @ 2019-12-20 17:00 UTC (permalink / raw)
  To: Ville Syrjälä; +Cc: intel-gfx

On Fri, Dec 20, 2019 at 03:59:41PM +0200, Ville Syrjälä wrote:
> On Thu, Dec 19, 2019 at 01:51:16PM -0800, Manasi Navare wrote:
> > Add an extra check before making master slave assignments for tiled
> > displays to make sure we make these assignments only if all tiled
> > connectors are present. If not then initialize the state to defaults
> > so it does a normal non tiled modeset without transcoder port sync.
> > 
> > v2:
> > * Rename icl_add_sync_mode_crtcs
> > * Move this function just before .compute_config hook
> > * Check if DP before master slave assignments (Ville)
> > 
> > Bugzilla: https://gitlab.freedesktop.org/drm/intel/issues/5
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_display.c | 162 +++++++++++--------
> >  1 file changed, 99 insertions(+), 63 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > index 00608d8cef50..9c1b1256be68 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -12014,88 +12014,106 @@ 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 intel_crtc_state *crtc_state)
> > +static void reset_port_sync_mode_state(struct intel_crtc_state *crtc_state)
> > +{
> > +	crtc_state->master_transcoder = INVALID_TRANSCODER;
> > +	crtc_state->sync_mode_slaves_mask = 0;
> > +}
> > +
> > +static int icl_compute_port_sync_crtc_state(struct drm_connector *connector,
> > +					    struct intel_crtc_state *crtc_state,
> > +					    int num_tiled_conns)
> >  {
> >  	struct drm_crtc *crtc = crtc_state->uapi.crtc;
> >  	struct intel_atomic_state *state = to_intel_atomic_state(crtc_state->uapi.state);
> >  	struct drm_i915_private *dev_priv = to_i915(crtc_state->uapi.crtc->dev);
> > -	struct drm_connector *master_connector, *connector;
> > -	struct drm_connector_state *connector_state;
> > +	struct drm_connector *master_connector;
> >  	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;
> >  
> > +	if (!intel_crtc_has_type(crtc_state, INTEL_OUTPUT_DP))
> > +		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.
> > +	 * If all tiles not present do not make master slave assignments.
> > +	 *
> > +	 * FIXME: Add support for multiple tile grp ids in the future when such
> > +	 * panels are available.
> 
> What does that mean? Why would we treat mutliple groups as a single
> tiled display?
>

No we wont treat multiple tile grp ids as a single display but we need to support
multiple pile grp ids in the state so 2 pairs of master and slave.
 
> >  	 */
> > -	for_each_new_connector_in_state(&state->base, connector, connector_state, i) {
> > -		if (connector_state->crtc != crtc)
> > -			continue;
> > -		if (!connector->has_tile)
> > +	if (!connector->has_tile ||
> > +	    crtc_state->hw.mode.hdisplay != connector->tile_h_size ||
> > +	    crtc_state->hw.mode.vdisplay != connector->tile_v_size ||
> > +	    num_tiled_conns < connector->num_h_tile * connector->num_v_tile) {
> > +		reset_port_sync_mode_state(crtc_state);
> 
> I really don't understand this back and forth of setting/clearing
> the master/slave state. I think the way to make it not convoluted is
> to just clear everything in intel_crtc_prepare_cleared_state() and
> then just compute everything from scratch.
>

Hmm i can add that to intel_crtc_prepare_cleared_state(), but since we compute master crtc
and its crtc_state during slave's compute_Config, we cannot clear the state if its master.
 
> > +		return 0;
> > +	}
> > +	/* Last Horizontal and last vertical tile connector is a master
> > +	 * Master Trans for a Master CRTC is always INVALID.
> > +	 */
> > +	if (connector->tile_h_loc == connector->num_h_tile - 1 &&
> > +	    connector->tile_v_loc == connector->num_v_tile - 1) {
> > +		crtc_state->master_transcoder = INVALID_TRANSCODER;
> > +		return 0;
> > +	}
> > +
> > +	/* Loop through all connectors and configure the Slave crtc_state
> > +	 * to point to the correct master.
> > +	 */
> > +	reset_port_sync_mode_state(crtc_state);
> > +	drm_connector_list_iter_begin(&dev_priv->drm, &conn_iter);
> > +	drm_for_each_connector_iter(master_connector, &conn_iter) {
> 
> Why are we all of a sudden iterating through all connectors? I think this
> should just iterate the connectors already in the state. The only place
> where we want to look at the full connector list is the modeset_all_tiles()
> thing.
>

This is how we had decided to do the master slave asignments and adding other crtc
corresponding to last tile (master) to state by iteratin through all connectors.
This was sugested on IRC by Danvet and Maarten when I was finalizing the design in the code
for master slave assignments.

so basically for the master compute config, it just returns and this port sync sonfig compute 
happens for the slave, if master slaves all not present so all tiles not present then we reset the 
asignments and return.

And thats one of the reasons why i need the reset here since the corner cases check happens here like
if all tiles are present if hdisplay vdisplay matches tile size, if not then we should reset the master slave crtc states.

 
> > +		struct drm_connector_state *master_conn_state = NULL;
> > +
> > +		if (!(master_connector->has_tile &&
> > +		      master_connector->tile_group->id == connector->tile_group->id))
> >  			continue;
> > -		if (crtc_state->hw.mode.hdisplay != connector->tile_h_size ||
> > -		    crtc_state->hw.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)
> > +		if (master_connector->tile_h_loc != master_connector->num_h_tile - 1 ||
> > +		    master_connector->tile_v_loc != master_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->base,
> > -									   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;
> > -			}
> > +		master_conn_state = drm_atomic_get_connector_state(&state->base,
> > +								   master_connector);
> > +		if (IS_ERR(master_conn_state)) {
> > +			drm_connector_list_iter_end(&conn_iter);
> > +			return PTR_ERR(master_conn_state);
> >  		}
> > -		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;
> > +		if (master_conn_state->crtc) {
> > +			master_crtc = master_conn_state->crtc;
> > +			break;
> >  		}
> > +	}
> > +	drm_connector_list_iter_end(&conn_iter);
> >  
> > -		master_crtc_state = drm_atomic_get_crtc_state(&state->base,
> > -							      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->uapi.crtc->base.id,
> > -			      master_pipe_config->sync_mode_slaves_mask);
> > +	if (!master_crtc) {
> > +		DRM_DEBUG_KMS("Could not find Master CRTC for Slave CRTC %d\n",
> > +			      crtc->base.id);
> > +		return -EINVAL;
> >  	}
> >  
> > +	master_crtc_state = drm_atomic_get_crtc_state(&state->base,
> > +						      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->base.id,
> > +		      master_pipe_config->sync_mode_slaves_mask);
> > +
> >  	return 0;
> >  }
> >  
> > @@ -12660,7 +12678,7 @@ intel_modeset_pipe_config(struct intel_crtc_state *pipe_config)
> >  	struct drm_connector *connector;
> >  	struct drm_connector_state *connector_state;
> >  	int base_bpp, ret;
> > -	int i;
> > +	int i, tile_group_id = -1, num_tiled_conns = 0;
> >  	bool retry = true;
> >  
> >  	pipe_config->cpu_transcoder =
> > @@ -12730,13 +12748,23 @@ intel_modeset_pipe_config(struct intel_crtc_state *pipe_config)
> >  	drm_mode_set_crtcinfo(&pipe_config->hw.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(pipe_config);
> > -	if (ret) {
> > -		DRM_DEBUG_KMS("Cannot assign Sync Mode CRTCs: %d\n",
> > -			      ret);
> > -		return ret;
> > +
> > +	/* Get tile_group_id of tiled connector */
> > +	for_each_new_connector_in_state(state, connector, connector_state, i) {
> > +		if (connector_state->crtc == crtc &&
> > +		    connector->has_tile) {
> > +			tile_group_id = connector->tile_group->id;
> > +			break;
> > +		}
> > +	}
> > +
> > +	/* Get total number of tiled connectors in state that belong to
> > +	 * this tile group.
> > +	 */
> > +	for_each_new_connector_in_state(state, connector, connector_state, i) {
> > +		if (connector->has_tile &&
> > +		    connector->tile_group->id == tile_group_id)
> > +			num_tiled_conns++;
> >  	}
> 
> I don't see why we would need this num_tiled_conns stuff at all. Just
> iterate all connectors in the state that belong to the same group.
>

Yea but if all tiles are not present meaning num_tiled_conns not equal to num tiles then
we should not make master slave assignments, here the crtcs would not be synced and just operate
as an individual crtc not synced crtc. Like after we disconnect 1 connector, there is only 1 tiled conn connected
and in that case it still displays the lower non tiled mode but we should not make master slave asignments.

May be all these checks for hdisplay , vdisplay, num conns == num_tiles can be moved to intel_atomic_sync_mode_check at the beginning?

Manasi
 
> >  
> >  	/* Pass our mode to the connectors and the CRTC to give them a chance to
> > @@ -12747,6 +12775,14 @@ intel_modeset_pipe_config(struct intel_crtc_state *pipe_config)
> >  		if (connector_state->crtc != crtc)
> >  			continue;
> >  
> > +		ret = icl_compute_port_sync_crtc_state(connector, pipe_config,
> > +						       num_tiled_conns);
> > +		if (ret) {
> > +			DRM_DEBUG_KMS("Cannot assign Sync Mode CRTCs: %d\n",
> > +				      ret);
> > +			return ret;
> > +		}
> > +
> >  		encoder = to_intel_encoder(connector_state->best_encoder);
> >  		ret = encoder->compute_config(encoder, pipe_config,
> >  					      connector_state);
> > -- 
> > 2.19.1
> 
> -- 
> Ville Syrjälä
> Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [Intel-gfx] [PATCH v2 1/3] drm/i915/dp: Make sure all tiled connectors get added to the state with full modeset
  2019-12-20 16:47     ` Ville Syrjälä
@ 2019-12-20 17:04       ` Manasi Navare
  2019-12-20 17:40         ` Ville Syrjälä
  0 siblings, 1 reply; 19+ messages in thread
From: Manasi Navare @ 2019-12-20 17:04 UTC (permalink / raw)
  To: Ville Syrjälä; +Cc: intel-gfx

On Fri, Dec 20, 2019 at 06:47:46PM +0200, Ville Syrjälä wrote:
> On Fri, Dec 20, 2019 at 08:35:58AM -0800, Manasi Navare wrote:
> > On Fri, Dec 20, 2019 at 03:17:27PM +0200, Ville Syrjälä wrote:
> > > On Thu, Dec 19, 2019 at 01:51:15PM -0800, Manasi Navare wrote:
> > > > In case of tiled displays, all the tiles are linke dto each other
> > > > for transcoder port sync. So in intel_atomic_check() we need to make
> > > > sure that we add all the tiles to the modeset and if one of the
> > > > tiles needs a full modeset then mark all other tiles for a full modeset.
> > > > 
> > > > v2:
> > > > * Change crtc_state scope, remove tile_grp_id (Ville)
> > > > * Use intel_connector_needs_modeset() (Ville)
> > > > * Add modeset_synced_crtcs (Ville)
> > > > * Make sure synced crtcs are forced full modeset
> > > > after fastset check (Ville)
> > > > 
> > > > Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > Cc: José Roberto de Souza <jose.souza@intel.com>
> > > > Cc: Matt Roper <matthew.d.roper@intel.com>
> > > > Bugzilla: https://gitlab.freedesktop.org/drm/intel/issues/5
> > > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/display/intel_display.c | 143 +++++++++++++++++--
> > > >  1 file changed, 131 insertions(+), 12 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > > > index a3f9430493ae..00608d8cef50 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > > @@ -13910,18 +13910,6 @@ static void intel_crtc_check_fastset(const struct intel_crtc_state *old_crtc_sta
> > > >  	new_crtc_state->uapi.mode_changed = false;
> > > >  	new_crtc_state->update_pipe = true;
> > > >  
> > > > -	/*
> > > > -	 * If we're not doing the full modeset we want to
> > > > -	 * keep the current M/N values as they may be
> > > > -	 * sufficiently different to the computed values
> > > > -	 * to cause problems.
> > > > -	 *
> > > > -	 * FIXME: should really copy more fuzzy state here
> > > > -	 */
> > > > -	new_crtc_state->fdi_m_n = old_crtc_state->fdi_m_n;
> > > > -	new_crtc_state->dp_m_n = old_crtc_state->dp_m_n;
> > > > -	new_crtc_state->dp_m2_n2 = old_crtc_state->dp_m2_n2;
> > > > -	new_crtc_state->has_drrs = old_crtc_state->has_drrs;
> > > >  }
> > > 
> > > The check vs. copy should really be a separate patch. Otherwise we risk
> > > having to revert the whole thing if something goes wrong.
> > >
> > 
> > Yes will sepaarte it out , also I think Jose's series might get merged before mine so..
> > 
> >  
> > > >  
> > > >  static int intel_crtc_add_planes_to_state(struct intel_atomic_state *state,
> > > > @@ -14032,6 +14020,105 @@ static int intel_atomic_check_crtcs(struct intel_atomic_state *state)
> > > >  	return 0;
> > > >  }
> > > >  
> > > > +static void
> > > > +intel_dp_modeset_synced_crtcs(struct intel_atomic_state *state)
> > > 
> > > I would remove "dp" from the names of all these functions. The fact
> > > that we only enable port sync on DP outputs is just an implementation
> > > detail we don't have/want to care about in higher level code like this.
> > >
> > 
> > Okay will rename
> >  
> > > > +{
> > > > +	struct intel_crtc_state *new_crtc_state;
> > > > +	struct intel_crtc *crtc;
> > > > +	int i;
> > > > +
> > > > +	for_each_new_intel_crtc_in_state(state, crtc,
> > > > +					 new_crtc_state, i) {
> > > > +		if (is_trans_port_sync_mode(new_crtc_state)) {
> > > > +			new_crtc_state->uapi.mode_changed = true;
> > > > +			new_crtc_state->update_pipe = false;
> > > > +		}
> > > > +	}
> > > > +}
> > > > +
> > > > +static void
> > > > +intel_dp_atomic_check_synced_crtcs(struct intel_atomic_state *state)
> > > > +{
> > > > +	struct intel_crtc_state *new_crtc_state;
> > > > +	struct intel_crtc *crtc;
> > > > +	int i;
> > > > +
> > > > +	for_each_new_intel_crtc_in_state(state, crtc,
> > > > +					 new_crtc_state, i) {
> > > > +		if (!is_trans_port_sync_mode(new_crtc_state) ||
> > > > +		    !needs_modeset(new_crtc_state))
> > > > +			continue;
> > > > +
> > > > +		intel_dp_modeset_synced_crtcs(state);
> > > > +	}
> > > 
> > > This is not sufficient for the pre-compute_config() check. The point is
> > > that we don't yet necessarily have the synced crtcs in the atomic state
> > > so we have to add them. Please have a look in my branch for what I mean
> > > exactly.
> > >
> > 
> > But if I use old crtc state or new crtc state here, I do see that since we havent
> > cleared the state yet, we do have all synced crtcs in the state , like here after I disconnect 1 conn,
> > I do see both conns in trans port sync mode and i can set the mode changed to true for both.
> > 
> > I dont understand why what you mean by we would beed to add them?
> 
> for_each_new_intel_crtc_in_state() only iterates the crtcs we've already
> added to the state, not all crtcs. The whole point of the
> pre-compute_config() check is that we add the ones that were previously
> synced but weren't yet added to the state. This for the case where the
> tile info has gone poof so the function that adds stuff based on the
> tile info will have missed them.
>

But it would have been previously synced and that info would still be in new_crtc_state->master_trans and slve_mask
since this is before intel_crtc_prepare_cleared_state()

Manasi
 
> >  
> > > For the check between the fastset check vs. copy this here should work.
> > > 
> > > > +}
> > > > +
> > > > +static int
> > > > +intel_dp_modeset_all_tiles(struct intel_atomic_state *state, int tile_grp_id)
> > > > +{
> > > > +	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> > > > +	struct drm_connector *connector;
> > > > +	struct drm_connector_list_iter conn_list_iter;
> > > 
> > > 'conn_iter' is the customary name.
> > >
> > 
> > ok
> >  
> > > > +
> > > > +	drm_connector_list_iter_begin(&dev_priv->drm, &conn_list_iter);
> > > > +	drm_for_each_connector_iter(connector, &conn_list_iter) {
> > > > +		struct drm_connector_state *conn_iter_state;
> > > 
> > > 'conn_state'
> > >
> > 
> > ok
> >  
> > > > +		struct drm_crtc_state *crtc_state;
> > > > +
> > > > +		if (!(connector->has_tile &&
> > > > +		      connector->tile_group->id == tile_grp_id))
> > > > +			continue;
> > > 
> > > or maybe !has_tile || tile_grrp != tile_grp
> > > 
> > > I find that form a bit easier on the eyes. But either will work.
> > >
> > 
> > Ok
> >  
> > > > +		conn_iter_state = drm_atomic_get_connector_state(&state->base,
> > > > +								 connector);
> > > > +		if (IS_ERR(conn_iter_state)) {
> > > > +			drm_connector_list_iter_end(&conn_list_iter);
> > > > +			return PTR_ERR(conn_iter_state);
> > > 
> > > nit: could just do 'ret = PTR_ERR(); break;'
> > >      and 'int ret=0;' + 'return ret' at the fars ends of the function.
> > >
> > 
> > ok
> >  
> > > > +		}
> > > > +
> > > > +		if (!conn_iter_state->crtc)
> > > > +			continue;
> > > > +
> > > > +		crtc_state = drm_atomic_get_crtc_state(&state->base,
> > > > +						       conn_iter_state->crtc);
> > > > +		if (IS_ERR(crtc_state)) {
> > > > +			drm_connector_list_iter_end(&conn_list_iter);
> > > > +			return PTR_ERR(conn_iter_state);
> > > > +		}
> > > > +		crtc_state->mode_changed = true;
> > > 
> > > Missing drm_atomic_add_affected_planes(). We also need that in the
> > > pre-compute_config() check_synced_crtcs().
> > >
> > 
> > Manasi
> > 
> > 
> > Oh we need to call this function along with mode_changed = true?
> >  
> > > > +	}
> > > > +	drm_connector_list_iter_end(&conn_list_iter);
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static int
> > > > +intel_dp_atomic_check_tiled_conns(struct intel_atomic_state *state)
> > > > +{
> > > > +	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> > > > +	struct drm_connector *connector;
> > > > +	struct drm_connector_state *old_conn_state, *new_conn_state;
> > > > +	int i, ret;
> > > > +
> > > > +	if (INTEL_GEN(dev_priv) < 11)
> > > > +		return 0;
> > > > +
> > > > +	/* Is tiled, mark all other tiled CRTCs as needing a modeset */
> > > > +	for_each_oldnew_connector_in_state(&state->base, connector,
> > > > +					   old_conn_state, new_conn_state, i) {
> > > > +		if (!connector->has_tile)
> > > > +			continue;
> > > > +		if (!intel_connector_needs_modeset(state, old_conn_state,
> > > > +						   new_conn_state))
> > > > +			continue;
> > > > +
> > > > +		ret = intel_dp_modeset_all_tiles(state, connector->tile_group->id);
> > > > +		if (ret)
> > > > +			return ret;
> > > > +	}
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > >  /**
> > > >   * intel_atomic_check - validate state object
> > > >   * @dev: drm device
> > > > @@ -14059,6 +14146,12 @@ static int intel_atomic_check(struct drm_device *dev,
> > > >  	if (ret)
> > > >  		goto fail;
> > > >  
> > > > +	ret = intel_dp_atomic_check_tiled_conns(state);
> > > > +	if (ret)
> > > > +		goto fail;
> > > > +
> > > > +	intel_dp_atomic_check_synced_crtcs(state);
> > > > +
> > > >  	for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
> > > >  					    new_crtc_state, i) {
> > > >  		if (!needs_modeset(new_crtc_state)) {
> > > > @@ -14089,6 +14182,32 @@ static int intel_atomic_check(struct drm_device *dev,
> > > >  			any_ms = true;
> > > >  	}
> > > >  
> > > > +	/*
> > > > +	 * In case of port synced crtcs, if one of the synced crtcs
> > > > +	 * needs a full modeset, all other synced crtcs should be
> > > > +	 * forced a full modeset.
> > > > +	 */
> > > > +	intel_dp_atomic_check_synced_crtcs(state);
> > > > +
> > > > +	for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
> > > > +					    new_crtc_state, i) {
> > > > +		if (needs_modeset(new_crtc_state))
> > > > +			continue;
> > > > +
> > > > +		/*
> > > > +		 * If we're not doing the full modeset we want to
> > > > +		 * keep the current M/N values as they may be
> > > > +		 * sufficiently different to the computed values
> > > > +		 * to cause problems.
> > > > +		 *
> > > > +		 * FIXME: should really copy more fuzzy state here
> > > > +		 */
> > > > +		new_crtc_state->fdi_m_n = old_crtc_state->fdi_m_n;
> > > > +		new_crtc_state->dp_m_n = old_crtc_state->dp_m_n;
> > > > +		new_crtc_state->dp_m2_n2 = old_crtc_state->dp_m2_n2;
> > > > +		new_crtc_state->has_drrs = old_crtc_state->has_drrs;
> > > 
> > > Same comment I gave in Jose's patch: Pls keep this in a separate
> > > function. My branch has an example.
> > > 
> > > > +	}
> > > > +
> > > >  	if (any_ms && !check_digital_port_conflicts(state)) {
> > > >  		DRM_DEBUG_KMS("rejecting conflicting digital port configuration\n");
> > > >  		ret = EINVAL;
> > > > -- 
> > > > 2.19.1
> > > 
> > > -- 
> > > Ville Syrjälä
> > > Intel
> 
> -- 
> Ville Syrjälä
> Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [Intel-gfx] [PATCH v2 2/3] drm/i915/dp: Make port sync mode assignments only if all tiles present
  2019-12-20 17:00     ` Manasi Navare
@ 2019-12-20 17:37       ` Ville Syrjälä
  0 siblings, 0 replies; 19+ messages in thread
From: Ville Syrjälä @ 2019-12-20 17:37 UTC (permalink / raw)
  To: Manasi Navare; +Cc: intel-gfx

On Fri, Dec 20, 2019 at 09:00:18AM -0800, Manasi Navare wrote:
> On Fri, Dec 20, 2019 at 03:59:41PM +0200, Ville Syrjälä wrote:
> > On Thu, Dec 19, 2019 at 01:51:16PM -0800, Manasi Navare wrote:
> > > Add an extra check before making master slave assignments for tiled
> > > displays to make sure we make these assignments only if all tiled
> > > connectors are present. If not then initialize the state to defaults
> > > so it does a normal non tiled modeset without transcoder port sync.
> > > 
> > > v2:
> > > * Rename icl_add_sync_mode_crtcs
> > > * Move this function just before .compute_config hook
> > > * Check if DP before master slave assignments (Ville)
> > > 
> > > Bugzilla: https://gitlab.freedesktop.org/drm/intel/issues/5
> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_display.c | 162 +++++++++++--------
> > >  1 file changed, 99 insertions(+), 63 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > > index 00608d8cef50..9c1b1256be68 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > @@ -12014,88 +12014,106 @@ 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 intel_crtc_state *crtc_state)
> > > +static void reset_port_sync_mode_state(struct intel_crtc_state *crtc_state)
> > > +{
> > > +	crtc_state->master_transcoder = INVALID_TRANSCODER;
> > > +	crtc_state->sync_mode_slaves_mask = 0;
> > > +}
> > > +
> > > +static int icl_compute_port_sync_crtc_state(struct drm_connector *connector,
> > > +					    struct intel_crtc_state *crtc_state,
> > > +					    int num_tiled_conns)
> > >  {
> > >  	struct drm_crtc *crtc = crtc_state->uapi.crtc;
> > >  	struct intel_atomic_state *state = to_intel_atomic_state(crtc_state->uapi.state);
> > >  	struct drm_i915_private *dev_priv = to_i915(crtc_state->uapi.crtc->dev);
> > > -	struct drm_connector *master_connector, *connector;
> > > -	struct drm_connector_state *connector_state;
> > > +	struct drm_connector *master_connector;
> > >  	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;
> > >  
> > > +	if (!intel_crtc_has_type(crtc_state, INTEL_OUTPUT_DP))
> > > +		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.
> > > +	 * If all tiles not present do not make master slave assignments.
> > > +	 *
> > > +	 * FIXME: Add support for multiple tile grp ids in the future when such
> > > +	 * panels are available.
> > 
> > What does that mean? Why would we treat mutliple groups as a single
> > tiled display?
> >
> 
> No we wont treat multiple tile grp ids as a single display but we need to support
> multiple pile grp ids in the state so 2 pairs of master and slave.

That is irrelevant here. Here we just care about a single tile group.
Nothing extra needs to be done to support multiple different tiled
displays, unless there's something crazy going on that I'm not seeing.

>  
> > >  	 */
> > > -	for_each_new_connector_in_state(&state->base, connector, connector_state, i) {
> > > -		if (connector_state->crtc != crtc)
> > > -			continue;
> > > -		if (!connector->has_tile)
> > > +	if (!connector->has_tile ||
> > > +	    crtc_state->hw.mode.hdisplay != connector->tile_h_size ||
> > > +	    crtc_state->hw.mode.vdisplay != connector->tile_v_size ||
> > > +	    num_tiled_conns < connector->num_h_tile * connector->num_v_tile) {
> > > +		reset_port_sync_mode_state(crtc_state);
> > 
> > I really don't understand this back and forth of setting/clearing
> > the master/slave state. I think the way to make it not convoluted is
> > to just clear everything in intel_crtc_prepare_cleared_state() and
> > then just compute everything from scratch.
> >
> 
> Hmm i can add that to intel_crtc_prepare_cleared_state(), but since we compute master crtc
> and its crtc_state during slave's compute_Config, we cannot clear the state if its master.

I suggest you simply make a function that computes for you the set of
synced transcoders for a specific tile group. Make that invariant so
that when called multiple times it always returns the same results.
Then you just call it here for everyone, always pick the same transcoder
from the  set of transcoders as the master (eg. lowest numbered transcoder).
Example in my branch.

That way you don't need to mix up the handling of multiple crtcs at all.
Mixing them up like this makes the code impossible to reason about since
now you end up sometimes changing the state of a crtc who has had its
state already computed and sometimes you change the state of a crtc
whose state hasn't yet been computed. And sometimes you set new bits in
the state and sometimes you clear bits in the state. So it's pretty much
impossible to say whether you've actually managed to compute the state
correctly for everyone in the end.

At least I can't hold all that in my head at once, so can't
understand what this code will end up doing.

>  
> > > +		return 0;
> > > +	}
> > > +	/* Last Horizontal and last vertical tile connector is a master
> > > +	 * Master Trans for a Master CRTC is always INVALID.
> > > +	 */
> > > +	if (connector->tile_h_loc == connector->num_h_tile - 1 &&
> > > +	    connector->tile_v_loc == connector->num_v_tile - 1) {
> > > +		crtc_state->master_transcoder = INVALID_TRANSCODER;
> > > +		return 0;
> > > +	}
> > > +
> > > +	/* Loop through all connectors and configure the Slave crtc_state
> > > +	 * to point to the correct master.
> > > +	 */
> > > +	reset_port_sync_mode_state(crtc_state);
> > > +	drm_connector_list_iter_begin(&dev_priv->drm, &conn_iter);
> > > +	drm_for_each_connector_iter(master_connector, &conn_iter) {
> > 
> > Why are we all of a sudden iterating through all connectors? I think this
> > should just iterate the connectors already in the state. The only place
> > where we want to look at the full connector list is the modeset_all_tiles()
> > thing.
> >
> 
> This is how we had decided to do the master slave asignments and adding other crtc
> corresponding to last tile (master) to state by iteratin through all connectors.
> This was sugested on IRC by Danvet and Maarten when I was finalizing the design in the code
> for master slave assignments.

You've already added all the connectors to the state you care about.
Potentially adding more here will just mean you end up in some
random state that you didn't expect.

> 
> so basically for the master compute config, it just returns and this port sync sonfig compute 
> happens for the slave, if master slaves all not present so all tiles not present then we reset the 
> asignments and return.
> 
> And thats one of the reasons why i need the reset here since the corner cases check happens here like
> if all tiles are present if hdisplay vdisplay matches tile size, if not then we should reset the master slave crtc states.

All that extra complexity with corner cases and whatnot would
totally vanish with the idea of an invariant function to compute
the set of synced transcoders.

> 
>  
> > > +		struct drm_connector_state *master_conn_state = NULL;
> > > +
> > > +		if (!(master_connector->has_tile &&
> > > +		      master_connector->tile_group->id == connector->tile_group->id))
> > >  			continue;
> > > -		if (crtc_state->hw.mode.hdisplay != connector->tile_h_size ||
> > > -		    crtc_state->hw.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)
> > > +		if (master_connector->tile_h_loc != master_connector->num_h_tile - 1 ||
> > > +		    master_connector->tile_v_loc != master_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->base,
> > > -									   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;
> > > -			}
> > > +		master_conn_state = drm_atomic_get_connector_state(&state->base,
> > > +								   master_connector);
> > > +		if (IS_ERR(master_conn_state)) {
> > > +			drm_connector_list_iter_end(&conn_iter);
> > > +			return PTR_ERR(master_conn_state);
> > >  		}
> > > -		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;
> > > +		if (master_conn_state->crtc) {
> > > +			master_crtc = master_conn_state->crtc;
> > > +			break;
> > >  		}
> > > +	}
> > > +	drm_connector_list_iter_end(&conn_iter);
> > >  
> > > -		master_crtc_state = drm_atomic_get_crtc_state(&state->base,
> > > -							      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->uapi.crtc->base.id,
> > > -			      master_pipe_config->sync_mode_slaves_mask);
> > > +	if (!master_crtc) {
> > > +		DRM_DEBUG_KMS("Could not find Master CRTC for Slave CRTC %d\n",
> > > +			      crtc->base.id);
> > > +		return -EINVAL;
> > >  	}
> > >  
> > > +	master_crtc_state = drm_atomic_get_crtc_state(&state->base,
> > > +						      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->base.id,
> > > +		      master_pipe_config->sync_mode_slaves_mask);
> > > +
> > >  	return 0;
> > >  }
> > >  
> > > @@ -12660,7 +12678,7 @@ intel_modeset_pipe_config(struct intel_crtc_state *pipe_config)
> > >  	struct drm_connector *connector;
> > >  	struct drm_connector_state *connector_state;
> > >  	int base_bpp, ret;
> > > -	int i;
> > > +	int i, tile_group_id = -1, num_tiled_conns = 0;
> > >  	bool retry = true;
> > >  
> > >  	pipe_config->cpu_transcoder =
> > > @@ -12730,13 +12748,23 @@ intel_modeset_pipe_config(struct intel_crtc_state *pipe_config)
> > >  	drm_mode_set_crtcinfo(&pipe_config->hw.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(pipe_config);
> > > -	if (ret) {
> > > -		DRM_DEBUG_KMS("Cannot assign Sync Mode CRTCs: %d\n",
> > > -			      ret);
> > > -		return ret;
> > > +
> > > +	/* Get tile_group_id of tiled connector */
> > > +	for_each_new_connector_in_state(state, connector, connector_state, i) {
> > > +		if (connector_state->crtc == crtc &&
> > > +		    connector->has_tile) {
> > > +			tile_group_id = connector->tile_group->id;
> > > +			break;
> > > +		}
> > > +	}
> > > +
> > > +	/* Get total number of tiled connectors in state that belong to
> > > +	 * this tile group.
> > > +	 */
> > > +	for_each_new_connector_in_state(state, connector, connector_state, i) {
> > > +		if (connector->has_tile &&
> > > +		    connector->tile_group->id == tile_group_id)
> > > +			num_tiled_conns++;
> > >  	}
> > 
> > I don't see why we would need this num_tiled_conns stuff at all. Just
> > iterate all connectors in the state that belong to the same group.
> >
> 
> Yea but if all tiles are not present meaning num_tiled_conns not equal to num tiles then
> we should not make master slave assignments, here the crtcs would not be synced and just operate
> as an individual crtc not synced crtc.

I don't at all see why we should make such an exception. It just makes
everything more complicated and means the tiles you do have end up not
being synced.

> Like after we disconnect 1 connector, there is only 1 tiled conn connected
> and in that case it still displays the lower non tiled mode but we should not make master slave asignments.
> 
> May be all these checks for hdisplay , vdisplay, num conns == num_tiles can be moved to intel_atomic_sync_mode_check at the beginning?

I think the only checks that should remain are tile_group_id,
hdisplay and vdisplay. All that business with the tile positions and
whatnot is just making the code extremely complicated.

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [Intel-gfx] [PATCH v2 1/3] drm/i915/dp: Make sure all tiled connectors get added to the state with full modeset
  2019-12-20 17:04       ` Manasi Navare
@ 2019-12-20 17:40         ` Ville Syrjälä
  0 siblings, 0 replies; 19+ messages in thread
From: Ville Syrjälä @ 2019-12-20 17:40 UTC (permalink / raw)
  To: Manasi Navare; +Cc: intel-gfx

On Fri, Dec 20, 2019 at 09:04:00AM -0800, Manasi Navare wrote:
> On Fri, Dec 20, 2019 at 06:47:46PM +0200, Ville Syrjälä wrote:
> > On Fri, Dec 20, 2019 at 08:35:58AM -0800, Manasi Navare wrote:
> > > On Fri, Dec 20, 2019 at 03:17:27PM +0200, Ville Syrjälä wrote:
> > > > On Thu, Dec 19, 2019 at 01:51:15PM -0800, Manasi Navare wrote:
> > > > > In case of tiled displays, all the tiles are linke dto each other
> > > > > for transcoder port sync. So in intel_atomic_check() we need to make
> > > > > sure that we add all the tiles to the modeset and if one of the
> > > > > tiles needs a full modeset then mark all other tiles for a full modeset.
> > > > > 
> > > > > v2:
> > > > > * Change crtc_state scope, remove tile_grp_id (Ville)
> > > > > * Use intel_connector_needs_modeset() (Ville)
> > > > > * Add modeset_synced_crtcs (Ville)
> > > > > * Make sure synced crtcs are forced full modeset
> > > > > after fastset check (Ville)
> > > > > 
> > > > > Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > Cc: José Roberto de Souza <jose.souza@intel.com>
> > > > > Cc: Matt Roper <matthew.d.roper@intel.com>
> > > > > Bugzilla: https://gitlab.freedesktop.org/drm/intel/issues/5
> > > > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > > > > ---
> > > > >  drivers/gpu/drm/i915/display/intel_display.c | 143 +++++++++++++++++--
> > > > >  1 file changed, 131 insertions(+), 12 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > > > > index a3f9430493ae..00608d8cef50 100644
> > > > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > > > @@ -13910,18 +13910,6 @@ static void intel_crtc_check_fastset(const struct intel_crtc_state *old_crtc_sta
> > > > >  	new_crtc_state->uapi.mode_changed = false;
> > > > >  	new_crtc_state->update_pipe = true;
> > > > >  
> > > > > -	/*
> > > > > -	 * If we're not doing the full modeset we want to
> > > > > -	 * keep the current M/N values as they may be
> > > > > -	 * sufficiently different to the computed values
> > > > > -	 * to cause problems.
> > > > > -	 *
> > > > > -	 * FIXME: should really copy more fuzzy state here
> > > > > -	 */
> > > > > -	new_crtc_state->fdi_m_n = old_crtc_state->fdi_m_n;
> > > > > -	new_crtc_state->dp_m_n = old_crtc_state->dp_m_n;
> > > > > -	new_crtc_state->dp_m2_n2 = old_crtc_state->dp_m2_n2;
> > > > > -	new_crtc_state->has_drrs = old_crtc_state->has_drrs;
> > > > >  }
> > > > 
> > > > The check vs. copy should really be a separate patch. Otherwise we risk
> > > > having to revert the whole thing if something goes wrong.
> > > >
> > > 
> > > Yes will sepaarte it out , also I think Jose's series might get merged before mine so..
> > > 
> > >  
> > > > >  
> > > > >  static int intel_crtc_add_planes_to_state(struct intel_atomic_state *state,
> > > > > @@ -14032,6 +14020,105 @@ static int intel_atomic_check_crtcs(struct intel_atomic_state *state)
> > > > >  	return 0;
> > > > >  }
> > > > >  
> > > > > +static void
> > > > > +intel_dp_modeset_synced_crtcs(struct intel_atomic_state *state)
> > > > 
> > > > I would remove "dp" from the names of all these functions. The fact
> > > > that we only enable port sync on DP outputs is just an implementation
> > > > detail we don't have/want to care about in higher level code like this.
> > > >
> > > 
> > > Okay will rename
> > >  
> > > > > +{
> > > > > +	struct intel_crtc_state *new_crtc_state;
> > > > > +	struct intel_crtc *crtc;
> > > > > +	int i;
> > > > > +
> > > > > +	for_each_new_intel_crtc_in_state(state, crtc,
> > > > > +					 new_crtc_state, i) {
> > > > > +		if (is_trans_port_sync_mode(new_crtc_state)) {
> > > > > +			new_crtc_state->uapi.mode_changed = true;
> > > > > +			new_crtc_state->update_pipe = false;
> > > > > +		}
> > > > > +	}
> > > > > +}
> > > > > +
> > > > > +static void
> > > > > +intel_dp_atomic_check_synced_crtcs(struct intel_atomic_state *state)
> > > > > +{
> > > > > +	struct intel_crtc_state *new_crtc_state;
> > > > > +	struct intel_crtc *crtc;
> > > > > +	int i;
> > > > > +
> > > > > +	for_each_new_intel_crtc_in_state(state, crtc,
> > > > > +					 new_crtc_state, i) {
> > > > > +		if (!is_trans_port_sync_mode(new_crtc_state) ||
> > > > > +		    !needs_modeset(new_crtc_state))
> > > > > +			continue;
> > > > > +
> > > > > +		intel_dp_modeset_synced_crtcs(state);
> > > > > +	}
> > > > 
> > > > This is not sufficient for the pre-compute_config() check. The point is
> > > > that we don't yet necessarily have the synced crtcs in the atomic state
> > > > so we have to add them. Please have a look in my branch for what I mean
> > > > exactly.
> > > >
> > > 
> > > But if I use old crtc state or new crtc state here, I do see that since we havent
> > > cleared the state yet, we do have all synced crtcs in the state , like here after I disconnect 1 conn,
> > > I do see both conns in trans port sync mode and i can set the mode changed to true for both.
> > > 
> > > I dont understand why what you mean by we would beed to add them?
> > 
> > for_each_new_intel_crtc_in_state() only iterates the crtcs we've already
> > added to the state, not all crtcs. The whole point of the
> > pre-compute_config() check is that we add the ones that were previously
> > synced but weren't yet added to the state. This for the case where the
> > tile info has gone poof so the function that adds stuff based on the
> > tile info will have missed them.
> >
> 
> But it would have been previously synced and that info would still be in new_crtc_state->master_trans and slve_mask
> since this is before intel_crtc_prepare_cleared_state()

You don't even have that new_crtc_state yet for the crtcs that were
synced previously but whose tile info vanished in the meantime.
That's the whole point here.

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [Intel-gfx] [PATCH v2 2/3] drm/i915/dp: Make port sync mode assignments only if all tiles present
  2019-12-19 21:51 ` [Intel-gfx] [PATCH v2 2/3] drm/i915/dp: Make port sync mode assignments only if all tiles present Manasi Navare
  2019-12-20 13:59   ` Ville Syrjälä
@ 2019-12-20 18:13   ` Ville Syrjälä
  1 sibling, 0 replies; 19+ messages in thread
From: Ville Syrjälä @ 2019-12-20 18:13 UTC (permalink / raw)
  To: Manasi Navare; +Cc: intel-gfx

On Thu, Dec 19, 2019 at 01:51:16PM -0800, Manasi Navare wrote:
> Add an extra check before making master slave assignments for tiled
> displays to make sure we make these assignments only if all tiled
> connectors are present. If not then initialize the state to defaults
> so it does a normal non tiled modeset without transcoder port sync.
> 
> v2:
> * Rename icl_add_sync_mode_crtcs
> * Move this function just before .compute_config hook
> * Check if DP before master slave assignments (Ville)
> 
> Bugzilla: https://gitlab.freedesktop.org/drm/intel/issues/5
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>

So, I don't really understnad this code but it seems safe enough (esp.
considering it's icl+ only and no one likely has a tiled display on
those).

So to get things moving
Acked-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> ---
>  drivers/gpu/drm/i915/display/intel_display.c | 162 +++++++++++--------
>  1 file changed, 99 insertions(+), 63 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index 00608d8cef50..9c1b1256be68 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -12014,88 +12014,106 @@ 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 intel_crtc_state *crtc_state)
> +static void reset_port_sync_mode_state(struct intel_crtc_state *crtc_state)
> +{
> +	crtc_state->master_transcoder = INVALID_TRANSCODER;
> +	crtc_state->sync_mode_slaves_mask = 0;
> +}
> +
> +static int icl_compute_port_sync_crtc_state(struct drm_connector *connector,
> +					    struct intel_crtc_state *crtc_state,
> +					    int num_tiled_conns)
>  {
>  	struct drm_crtc *crtc = crtc_state->uapi.crtc;
>  	struct intel_atomic_state *state = to_intel_atomic_state(crtc_state->uapi.state);
>  	struct drm_i915_private *dev_priv = to_i915(crtc_state->uapi.crtc->dev);
> -	struct drm_connector *master_connector, *connector;
> -	struct drm_connector_state *connector_state;
> +	struct drm_connector *master_connector;
>  	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;
>  
> +	if (!intel_crtc_has_type(crtc_state, INTEL_OUTPUT_DP))
> +		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.
> +	 * If all tiles not present do not make master slave assignments.
> +	 *
> +	 * FIXME: Add support for multiple tile grp ids in the future when such
> +	 * panels are available.
>  	 */
> -	for_each_new_connector_in_state(&state->base, connector, connector_state, i) {
> -		if (connector_state->crtc != crtc)
> -			continue;
> -		if (!connector->has_tile)
> +	if (!connector->has_tile ||
> +	    crtc_state->hw.mode.hdisplay != connector->tile_h_size ||
> +	    crtc_state->hw.mode.vdisplay != connector->tile_v_size ||
> +	    num_tiled_conns < connector->num_h_tile * connector->num_v_tile) {
> +		reset_port_sync_mode_state(crtc_state);
> +		return 0;
> +	}
> +	/* Last Horizontal and last vertical tile connector is a master
> +	 * Master Trans for a Master CRTC is always INVALID.
> +	 */
> +	if (connector->tile_h_loc == connector->num_h_tile - 1 &&
> +	    connector->tile_v_loc == connector->num_v_tile - 1) {
> +		crtc_state->master_transcoder = INVALID_TRANSCODER;
> +		return 0;
> +	}
> +
> +	/* Loop through all connectors and configure the Slave crtc_state
> +	 * to point to the correct master.
> +	 */
> +	reset_port_sync_mode_state(crtc_state);
> +	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 &&
> +		      master_connector->tile_group->id == connector->tile_group->id))
>  			continue;
> -		if (crtc_state->hw.mode.hdisplay != connector->tile_h_size ||
> -		    crtc_state->hw.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)
> +		if (master_connector->tile_h_loc != master_connector->num_h_tile - 1 ||
> +		    master_connector->tile_v_loc != master_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->base,
> -									   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;
> -			}
> +		master_conn_state = drm_atomic_get_connector_state(&state->base,
> +								   master_connector);
> +		if (IS_ERR(master_conn_state)) {
> +			drm_connector_list_iter_end(&conn_iter);
> +			return PTR_ERR(master_conn_state);
>  		}
> -		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;
> +		if (master_conn_state->crtc) {
> +			master_crtc = master_conn_state->crtc;
> +			break;
>  		}
> +	}
> +	drm_connector_list_iter_end(&conn_iter);
>  
> -		master_crtc_state = drm_atomic_get_crtc_state(&state->base,
> -							      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->uapi.crtc->base.id,
> -			      master_pipe_config->sync_mode_slaves_mask);
> +	if (!master_crtc) {
> +		DRM_DEBUG_KMS("Could not find Master CRTC for Slave CRTC %d\n",
> +			      crtc->base.id);
> +		return -EINVAL;
>  	}
>  
> +	master_crtc_state = drm_atomic_get_crtc_state(&state->base,
> +						      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->base.id,
> +		      master_pipe_config->sync_mode_slaves_mask);
> +
>  	return 0;
>  }
>  
> @@ -12660,7 +12678,7 @@ intel_modeset_pipe_config(struct intel_crtc_state *pipe_config)
>  	struct drm_connector *connector;
>  	struct drm_connector_state *connector_state;
>  	int base_bpp, ret;
> -	int i;
> +	int i, tile_group_id = -1, num_tiled_conns = 0;
>  	bool retry = true;
>  
>  	pipe_config->cpu_transcoder =
> @@ -12730,13 +12748,23 @@ intel_modeset_pipe_config(struct intel_crtc_state *pipe_config)
>  	drm_mode_set_crtcinfo(&pipe_config->hw.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(pipe_config);
> -	if (ret) {
> -		DRM_DEBUG_KMS("Cannot assign Sync Mode CRTCs: %d\n",
> -			      ret);
> -		return ret;
> +
> +	/* Get tile_group_id of tiled connector */
> +	for_each_new_connector_in_state(state, connector, connector_state, i) {
> +		if (connector_state->crtc == crtc &&
> +		    connector->has_tile) {
> +			tile_group_id = connector->tile_group->id;
> +			break;
> +		}
> +	}
> +
> +	/* Get total number of tiled connectors in state that belong to
> +	 * this tile group.
> +	 */
> +	for_each_new_connector_in_state(state, connector, connector_state, i) {
> +		if (connector->has_tile &&
> +		    connector->tile_group->id == tile_group_id)
> +			num_tiled_conns++;
>  	}
>  
>  	/* Pass our mode to the connectors and the CRTC to give them a chance to
> @@ -12747,6 +12775,14 @@ intel_modeset_pipe_config(struct intel_crtc_state *pipe_config)
>  		if (connector_state->crtc != crtc)
>  			continue;
>  
> +		ret = icl_compute_port_sync_crtc_state(connector, pipe_config,
> +						       num_tiled_conns);
> +		if (ret) {
> +			DRM_DEBUG_KMS("Cannot assign Sync Mode CRTCs: %d\n",
> +				      ret);
> +			return ret;
> +		}
> +
>  		encoder = to_intel_encoder(connector_state->best_encoder);
>  		ret = encoder->compute_config(encoder, pipe_config,
>  					      connector_state);
> -- 
> 2.19.1

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [Intel-gfx] [PATCH v2 1/3] drm/i915/dp: Make sure all tiled connectors get added to the state with full modeset
  2019-12-19 21:51 [Intel-gfx] [PATCH v2 1/3] drm/i915/dp: Make sure all tiled connectors get added to the state with full modeset Manasi Navare
                   ` (4 preceding siblings ...)
  2019-12-20 13:17 ` [Intel-gfx] [PATCH v2 1/3] " Ville Syrjälä
@ 2019-12-20 18:41 ` Ville Syrjälä
  2019-12-20 19:05   ` Manasi Navare
  5 siblings, 1 reply; 19+ messages in thread
From: Ville Syrjälä @ 2019-12-20 18:41 UTC (permalink / raw)
  To: Manasi Navare; +Cc: intel-gfx

On Thu, Dec 19, 2019 at 01:51:15PM -0800, Manasi Navare wrote:
> In case of tiled displays, all the tiles are linke dto each other
> for transcoder port sync. So in intel_atomic_check() we need to make
> sure that we add all the tiles to the modeset and if one of the
> tiles needs a full modeset then mark all other tiles for a full modeset.
> 
> v2:
> * Change crtc_state scope, remove tile_grp_id (Ville)
> * Use intel_connector_needs_modeset() (Ville)
> * Add modeset_synced_crtcs (Ville)
> * Make sure synced crtcs are forced full modeset
> after fastset check (Ville)
> 
> Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: José Roberto de Souza <jose.souza@intel.com>
> Cc: Matt Roper <matthew.d.roper@intel.com>
> Bugzilla: https://gitlab.freedesktop.org/drm/intel/issues/5
> Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_display.c | 143 +++++++++++++++++--
>  1 file changed, 131 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index a3f9430493ae..00608d8cef50 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -13910,18 +13910,6 @@ static void intel_crtc_check_fastset(const struct intel_crtc_state *old_crtc_sta
>  	new_crtc_state->uapi.mode_changed = false;
>  	new_crtc_state->update_pipe = true;
>  
> -	/*
> -	 * If we're not doing the full modeset we want to
> -	 * keep the current M/N values as they may be
> -	 * sufficiently different to the computed values
> -	 * to cause problems.
> -	 *
> -	 * FIXME: should really copy more fuzzy state here
> -	 */
> -	new_crtc_state->fdi_m_n = old_crtc_state->fdi_m_n;
> -	new_crtc_state->dp_m_n = old_crtc_state->dp_m_n;
> -	new_crtc_state->dp_m2_n2 = old_crtc_state->dp_m2_n2;
> -	new_crtc_state->has_drrs = old_crtc_state->has_drrs;
>  }
>  
>  static int intel_crtc_add_planes_to_state(struct intel_atomic_state *state,
> @@ -14032,6 +14020,105 @@ static int intel_atomic_check_crtcs(struct intel_atomic_state *state)
>  	return 0;
>  }
>  
> +static void
> +intel_dp_modeset_synced_crtcs(struct intel_atomic_state *state)
> +{
> +	struct intel_crtc_state *new_crtc_state;
> +	struct intel_crtc *crtc;
> +	int i;
> +
> +	for_each_new_intel_crtc_in_state(state, crtc,
> +					 new_crtc_state, i) {
> +		if (is_trans_port_sync_mode(new_crtc_state)) {
> +			new_crtc_state->uapi.mode_changed = true;
> +			new_crtc_state->update_pipe = false;
> +		}

Upon further thinking this is not quite right even for the post fastset
check part. You're not checking at all whether these crtcs here are
actually synced to the crtc that still had its needs_modeset() flagged.
So this can end up forcing a modeset on an entirely unrelated crtc. We
don't want that.

You could fix that by passing in a transcoder bitmask, and then only
flagging a modeset on crtcs whose cpu_transcoder matches the bitmask.
(see intel_modeset_transcoders() in my branch).

> +	}
> +}
> +
> +static void
> +intel_dp_atomic_check_synced_crtcs(struct intel_atomic_state *state)
> +{
> +	struct intel_crtc_state *new_crtc_state;
> +	struct intel_crtc *crtc;
> +	int i;
> +
> +	for_each_new_intel_crtc_in_state(state, crtc,
> +					 new_crtc_state, i) {
> +		if (!is_trans_port_sync_mode(new_crtc_state) ||
> +		    !needs_modeset(new_crtc_state))
> +			continue;
> +
> +		intel_dp_modeset_synced_crtcs(state);
> +	}
> +}
> +
> +static int
> +intel_dp_modeset_all_tiles(struct intel_atomic_state *state, int tile_grp_id)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> +	struct drm_connector *connector;
> +	struct drm_connector_list_iter conn_list_iter;
> +
> +	drm_connector_list_iter_begin(&dev_priv->drm, &conn_list_iter);
> +	drm_for_each_connector_iter(connector, &conn_list_iter) {
> +		struct drm_connector_state *conn_iter_state;
> +		struct drm_crtc_state *crtc_state;
> +
> +		if (!(connector->has_tile &&
> +		      connector->tile_group->id == tile_grp_id))
> +			continue;
> +		conn_iter_state = drm_atomic_get_connector_state(&state->base,
> +								 connector);
> +		if (IS_ERR(conn_iter_state)) {
> +			drm_connector_list_iter_end(&conn_list_iter);
> +			return PTR_ERR(conn_iter_state);
> +		}
> +
> +		if (!conn_iter_state->crtc)
> +			continue;
> +
> +		crtc_state = drm_atomic_get_crtc_state(&state->base,
> +						       conn_iter_state->crtc);
> +		if (IS_ERR(crtc_state)) {
> +			drm_connector_list_iter_end(&conn_list_iter);
> +			return PTR_ERR(conn_iter_state);
> +		}
> +		crtc_state->mode_changed = true;
> +	}
> +	drm_connector_list_iter_end(&conn_list_iter);
> +
> +	return 0;
> +}
> +
> +static int
> +intel_dp_atomic_check_tiled_conns(struct intel_atomic_state *state)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> +	struct drm_connector *connector;
> +	struct drm_connector_state *old_conn_state, *new_conn_state;
> +	int i, ret;
> +
> +	if (INTEL_GEN(dev_priv) < 11)
> +		return 0;
> +
> +	/* Is tiled, mark all other tiled CRTCs as needing a modeset */
> +	for_each_oldnew_connector_in_state(&state->base, connector,
> +					   old_conn_state, new_conn_state, i) {
> +		if (!connector->has_tile)
> +			continue;
> +		if (!intel_connector_needs_modeset(state, old_conn_state,
> +						   new_conn_state))
> +			continue;
> +
> +		ret = intel_dp_modeset_all_tiles(state, connector->tile_group->id);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
>  /**
>   * intel_atomic_check - validate state object
>   * @dev: drm device
> @@ -14059,6 +14146,12 @@ static int intel_atomic_check(struct drm_device *dev,
>  	if (ret)
>  		goto fail;
>  
> +	ret = intel_dp_atomic_check_tiled_conns(state);
> +	if (ret)
> +		goto fail;

Since this is not sufficient for the pre-compute_config() check I think
we should drop this part for now. It's a somewhat unlikely scenario
after all so we can do it as a followup.

> +
> +	intel_dp_atomic_check_synced_crtcs(state);
> +
>  	for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
>  					    new_crtc_state, i) {
>  		if (!needs_modeset(new_crtc_state)) {
> @@ -14089,6 +14182,32 @@ static int intel_atomic_check(struct drm_device *dev,
>  			any_ms = true;
>  	}
>  
> +	/*
> +	 * In case of port synced crtcs, if one of the synced crtcs
> +	 * needs a full modeset, all other synced crtcs should be
> +	 * forced a full modeset.
> +	 */
> +	intel_dp_atomic_check_synced_crtcs(state);
> +
> +	for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
> +					    new_crtc_state, i) {
> +		if (needs_modeset(new_crtc_state))
> +			continue;
> +
> +		/*
> +		 * If we're not doing the full modeset we want to
> +		 * keep the current M/N values as they may be
> +		 * sufficiently different to the computed values
> +		 * to cause problems.
> +		 *
> +		 * FIXME: should really copy more fuzzy state here
> +		 */
> +		new_crtc_state->fdi_m_n = old_crtc_state->fdi_m_n;
> +		new_crtc_state->dp_m_n = old_crtc_state->dp_m_n;
> +		new_crtc_state->dp_m2_n2 = old_crtc_state->dp_m2_n2;
> +		new_crtc_state->has_drrs = old_crtc_state->has_drrs;
> +	}
> +
>  	if (any_ms && !check_digital_port_conflicts(state)) {
>  		DRM_DEBUG_KMS("rejecting conflicting digital port configuration\n");
>  		ret = EINVAL;
> -- 
> 2.19.1

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [Intel-gfx] [PATCH v2 1/3] drm/i915/dp: Make sure all tiled connectors get added to the state with full modeset
  2019-12-20 18:41 ` Ville Syrjälä
@ 2019-12-20 19:05   ` Manasi Navare
  2019-12-20 19:35     ` Ville Syrjälä
  0 siblings, 1 reply; 19+ messages in thread
From: Manasi Navare @ 2019-12-20 19:05 UTC (permalink / raw)
  To: Ville Syrjälä; +Cc: intel-gfx

On Fri, Dec 20, 2019 at 08:41:12PM +0200, Ville Syrjälä wrote:
> On Thu, Dec 19, 2019 at 01:51:15PM -0800, Manasi Navare wrote:
> > In case of tiled displays, all the tiles are linke dto each other
> > for transcoder port sync. So in intel_atomic_check() we need to make
> > sure that we add all the tiles to the modeset and if one of the
> > tiles needs a full modeset then mark all other tiles for a full modeset.
> > 
> > v2:
> > * Change crtc_state scope, remove tile_grp_id (Ville)
> > * Use intel_connector_needs_modeset() (Ville)
> > * Add modeset_synced_crtcs (Ville)
> > * Make sure synced crtcs are forced full modeset
> > after fastset check (Ville)
> > 
> > Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: José Roberto de Souza <jose.souza@intel.com>
> > Cc: Matt Roper <matthew.d.roper@intel.com>
> > Bugzilla: https://gitlab.freedesktop.org/drm/intel/issues/5
> > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_display.c | 143 +++++++++++++++++--
> >  1 file changed, 131 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > index a3f9430493ae..00608d8cef50 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -13910,18 +13910,6 @@ static void intel_crtc_check_fastset(const struct intel_crtc_state *old_crtc_sta
> >  	new_crtc_state->uapi.mode_changed = false;
> >  	new_crtc_state->update_pipe = true;
> >  
> > -	/*
> > -	 * If we're not doing the full modeset we want to
> > -	 * keep the current M/N values as they may be
> > -	 * sufficiently different to the computed values
> > -	 * to cause problems.
> > -	 *
> > -	 * FIXME: should really copy more fuzzy state here
> > -	 */
> > -	new_crtc_state->fdi_m_n = old_crtc_state->fdi_m_n;
> > -	new_crtc_state->dp_m_n = old_crtc_state->dp_m_n;
> > -	new_crtc_state->dp_m2_n2 = old_crtc_state->dp_m2_n2;
> > -	new_crtc_state->has_drrs = old_crtc_state->has_drrs;
> >  }
> >  
> >  static int intel_crtc_add_planes_to_state(struct intel_atomic_state *state,
> > @@ -14032,6 +14020,105 @@ static int intel_atomic_check_crtcs(struct intel_atomic_state *state)
> >  	return 0;
> >  }
> >  
> > +static void
> > +intel_dp_modeset_synced_crtcs(struct intel_atomic_state *state)
> > +{
> > +	struct intel_crtc_state *new_crtc_state;
> > +	struct intel_crtc *crtc;
> > +	int i;
> > +
> > +	for_each_new_intel_crtc_in_state(state, crtc,
> > +					 new_crtc_state, i) {
> > +		if (is_trans_port_sync_mode(new_crtc_state)) {
> > +			new_crtc_state->uapi.mode_changed = true;
> > +			new_crtc_state->update_pipe = false;
> > +		}
> 
> Upon further thinking this is not quite right even for the post fastset
> check part. You're not checking at all whether these crtcs here are
> actually synced to the crtc that still had its needs_modeset() flagged.
> So this can end up forcing a modeset on an entirely unrelated crtc. We
> don't want that.
> 
> You could fix that by passing in a transcoder bitmask, and then only
> flagging a modeset on crtcs whose cpu_transcoder matches the bitmask.
> (see intel_modeset_transcoders() in my branch).
> 
> > +	}
> > +}
> > +
> > +static void
> > +intel_dp_atomic_check_synced_crtcs(struct intel_atomic_state *state)
> > +{
> > +	struct intel_crtc_state *new_crtc_state;
> > +	struct intel_crtc *crtc;
> > +	int i;
> > +
> > +	for_each_new_intel_crtc_in_state(state, crtc,
> > +					 new_crtc_state, i) {
> > +		if (!is_trans_port_sync_mode(new_crtc_state) ||
> > +		    !needs_modeset(new_crtc_state))
> > +			continue;
> > +
> > +		intel_dp_modeset_synced_crtcs(state);

I guess in this call I can add master trans and sync_mode_slaves_bitmask

And then if the crtcs in state are in sync mode and their cpu_trans matches the master trans
or transcoders in the slave mask, add them to the modeset?



> > +	}
> > +}
> > +
> > +static int
> > +intel_dp_modeset_all_tiles(struct intel_atomic_state *state, int tile_grp_id)
> > +{
> > +	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> > +	struct drm_connector *connector;
> > +	struct drm_connector_list_iter conn_list_iter;
> > +
> > +	drm_connector_list_iter_begin(&dev_priv->drm, &conn_list_iter);
> > +	drm_for_each_connector_iter(connector, &conn_list_iter) {
> > +		struct drm_connector_state *conn_iter_state;
> > +		struct drm_crtc_state *crtc_state;
> > +
> > +		if (!(connector->has_tile &&
> > +		      connector->tile_group->id == tile_grp_id))
> > +			continue;
> > +		conn_iter_state = drm_atomic_get_connector_state(&state->base,
> > +								 connector);
> > +		if (IS_ERR(conn_iter_state)) {
> > +			drm_connector_list_iter_end(&conn_list_iter);
> > +			return PTR_ERR(conn_iter_state);
> > +		}
> > +
> > +		if (!conn_iter_state->crtc)
> > +			continue;
> > +
> > +		crtc_state = drm_atomic_get_crtc_state(&state->base,
> > +						       conn_iter_state->crtc);
> > +		if (IS_ERR(crtc_state)) {
> > +			drm_connector_list_iter_end(&conn_list_iter);
> > +			return PTR_ERR(conn_iter_state);
> > +		}
> > +		crtc_state->mode_changed = true;
> > +	}
> > +	drm_connector_list_iter_end(&conn_list_iter);
> > +
> > +	return 0;
> > +}
> > +
> > +static int
> > +intel_dp_atomic_check_tiled_conns(struct intel_atomic_state *state)
> > +{
> > +	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> > +	struct drm_connector *connector;
> > +	struct drm_connector_state *old_conn_state, *new_conn_state;
> > +	int i, ret;
> > +
> > +	if (INTEL_GEN(dev_priv) < 11)
> > +		return 0;
> > +
> > +	/* Is tiled, mark all other tiled CRTCs as needing a modeset */
> > +	for_each_oldnew_connector_in_state(&state->base, connector,
> > +					   old_conn_state, new_conn_state, i) {
> > +		if (!connector->has_tile)
> > +			continue;
> > +		if (!intel_connector_needs_modeset(state, old_conn_state,
> > +						   new_conn_state))
> > +			continue;
> > +
> > +		ret = intel_dp_modeset_all_tiles(state, connector->tile_group->id);
> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> >  /**
> >   * intel_atomic_check - validate state object
> >   * @dev: drm device
> > @@ -14059,6 +14146,12 @@ static int intel_atomic_check(struct drm_device *dev,
> >  	if (ret)
> >  		goto fail;
> >  
> > +	ret = intel_dp_atomic_check_tiled_conns(state);
> > +	if (ret)
> > +		goto fail;
> 
> Since this is not sufficient for the pre-compute_config() check I think
> we should drop this part for now. It's a somewhat unlikely scenario
> after all so we can do it as a followup.
>

Well we do need this for handling the hotplug cases correctly so that the connector disconnected
are added to the modeset which is working well with this function.

Manasi
 
> > +
> > +	intel_dp_atomic_check_synced_crtcs(state);
> > +
> >  	for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
> >  					    new_crtc_state, i) {
> >  		if (!needs_modeset(new_crtc_state)) {
> > @@ -14089,6 +14182,32 @@ static int intel_atomic_check(struct drm_device *dev,
> >  			any_ms = true;
> >  	}
> >  
> > +	/*
> > +	 * In case of port synced crtcs, if one of the synced crtcs
> > +	 * needs a full modeset, all other synced crtcs should be
> > +	 * forced a full modeset.
> > +	 */
> > +	intel_dp_atomic_check_synced_crtcs(state);
> > +
> > +	for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
> > +					    new_crtc_state, i) {
> > +		if (needs_modeset(new_crtc_state))
> > +			continue;
> > +
> > +		/*
> > +		 * If we're not doing the full modeset we want to
> > +		 * keep the current M/N values as they may be
> > +		 * sufficiently different to the computed values
> > +		 * to cause problems.
> > +		 *
> > +		 * FIXME: should really copy more fuzzy state here
> > +		 */
> > +		new_crtc_state->fdi_m_n = old_crtc_state->fdi_m_n;
> > +		new_crtc_state->dp_m_n = old_crtc_state->dp_m_n;
> > +		new_crtc_state->dp_m2_n2 = old_crtc_state->dp_m2_n2;
> > +		new_crtc_state->has_drrs = old_crtc_state->has_drrs;
> > +	}
> > +
> >  	if (any_ms && !check_digital_port_conflicts(state)) {
> >  		DRM_DEBUG_KMS("rejecting conflicting digital port configuration\n");
> >  		ret = EINVAL;
> > -- 
> > 2.19.1
> 
> -- 
> Ville Syrjälä
> Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [Intel-gfx] [PATCH v2 1/3] drm/i915/dp: Make sure all tiled connectors get added to the state with full modeset
  2019-12-20 19:05   ` Manasi Navare
@ 2019-12-20 19:35     ` Ville Syrjälä
  2019-12-20 20:13       ` Manasi Navare
  0 siblings, 1 reply; 19+ messages in thread
From: Ville Syrjälä @ 2019-12-20 19:35 UTC (permalink / raw)
  To: Manasi Navare; +Cc: intel-gfx

On Fri, Dec 20, 2019 at 11:05:31AM -0800, Manasi Navare wrote:
> On Fri, Dec 20, 2019 at 08:41:12PM +0200, Ville Syrjälä wrote:
> > On Thu, Dec 19, 2019 at 01:51:15PM -0800, Manasi Navare wrote:
> > > In case of tiled displays, all the tiles are linke dto each other
> > > for transcoder port sync. So in intel_atomic_check() we need to make
> > > sure that we add all the tiles to the modeset and if one of the
> > > tiles needs a full modeset then mark all other tiles for a full modeset.
> > > 
> > > v2:
> > > * Change crtc_state scope, remove tile_grp_id (Ville)
> > > * Use intel_connector_needs_modeset() (Ville)
> > > * Add modeset_synced_crtcs (Ville)
> > > * Make sure synced crtcs are forced full modeset
> > > after fastset check (Ville)
> > > 
> > > Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Cc: José Roberto de Souza <jose.souza@intel.com>
> > > Cc: Matt Roper <matthew.d.roper@intel.com>
> > > Bugzilla: https://gitlab.freedesktop.org/drm/intel/issues/5
> > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_display.c | 143 +++++++++++++++++--
> > >  1 file changed, 131 insertions(+), 12 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > > index a3f9430493ae..00608d8cef50 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > @@ -13910,18 +13910,6 @@ static void intel_crtc_check_fastset(const struct intel_crtc_state *old_crtc_sta
> > >  	new_crtc_state->uapi.mode_changed = false;
> > >  	new_crtc_state->update_pipe = true;
> > >  
> > > -	/*
> > > -	 * If we're not doing the full modeset we want to
> > > -	 * keep the current M/N values as they may be
> > > -	 * sufficiently different to the computed values
> > > -	 * to cause problems.
> > > -	 *
> > > -	 * FIXME: should really copy more fuzzy state here
> > > -	 */
> > > -	new_crtc_state->fdi_m_n = old_crtc_state->fdi_m_n;
> > > -	new_crtc_state->dp_m_n = old_crtc_state->dp_m_n;
> > > -	new_crtc_state->dp_m2_n2 = old_crtc_state->dp_m2_n2;
> > > -	new_crtc_state->has_drrs = old_crtc_state->has_drrs;
> > >  }
> > >  
> > >  static int intel_crtc_add_planes_to_state(struct intel_atomic_state *state,
> > > @@ -14032,6 +14020,105 @@ static int intel_atomic_check_crtcs(struct intel_atomic_state *state)
> > >  	return 0;
> > >  }
> > >  
> > > +static void
> > > +intel_dp_modeset_synced_crtcs(struct intel_atomic_state *state)
> > > +{
> > > +	struct intel_crtc_state *new_crtc_state;
> > > +	struct intel_crtc *crtc;
> > > +	int i;
> > > +
> > > +	for_each_new_intel_crtc_in_state(state, crtc,
> > > +					 new_crtc_state, i) {
> > > +		if (is_trans_port_sync_mode(new_crtc_state)) {
> > > +			new_crtc_state->uapi.mode_changed = true;
> > > +			new_crtc_state->update_pipe = false;
> > > +		}
> > 
> > Upon further thinking this is not quite right even for the post fastset
> > check part. You're not checking at all whether these crtcs here are
> > actually synced to the crtc that still had its needs_modeset() flagged.
> > So this can end up forcing a modeset on an entirely unrelated crtc. We
> > don't want that.
> > 
> > You could fix that by passing in a transcoder bitmask, and then only
> > flagging a modeset on crtcs whose cpu_transcoder matches the bitmask.
> > (see intel_modeset_transcoders() in my branch).
> > 
> > > +	}
> > > +}
> > > +
> > > +static void
> > > +intel_dp_atomic_check_synced_crtcs(struct intel_atomic_state *state)
> > > +{
> > > +	struct intel_crtc_state *new_crtc_state;
> > > +	struct intel_crtc *crtc;
> > > +	int i;
> > > +
> > > +	for_each_new_intel_crtc_in_state(state, crtc,
> > > +					 new_crtc_state, i) {
> > > +		if (!is_trans_port_sync_mode(new_crtc_state) ||
> > > +		    !needs_modeset(new_crtc_state))
> > > +			continue;
> > > +
> > > +		intel_dp_modeset_synced_crtcs(state);
> 
> I guess in this call I can add master trans and sync_mode_slaves_bitmask
> 
> And then if the crtcs in state are in sync mode and their cpu_trans matches the master trans
> or transcoders in the slave mask, add them to the modeset?

It can be as simple as:

modeset_transcoders(state, transcoders)
{
	for_each_crtc_in() {
		if (transcoders & BIT(cpu_transcoder))
			modeset = true;
	}	
}

intel_dp_atomic_check_synced_crtcs()
{
	for_each_crtc_in() {
		if (is_port_sync_maseter() && needs_modeset())
			modeset_transcoders(state, slave_mask);
		else if (is_port_sync_slave() && needs_modeset())
			modeset_transcoders(state, BIT(master_transcoder));
	}
}

> 
> 
> 
> > > +	}
> > > +}
> > > +
> > > +static int
> > > +intel_dp_modeset_all_tiles(struct intel_atomic_state *state, int tile_grp_id)
> > > +{
> > > +	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> > > +	struct drm_connector *connector;
> > > +	struct drm_connector_list_iter conn_list_iter;
> > > +
> > > +	drm_connector_list_iter_begin(&dev_priv->drm, &conn_list_iter);
> > > +	drm_for_each_connector_iter(connector, &conn_list_iter) {
> > > +		struct drm_connector_state *conn_iter_state;
> > > +		struct drm_crtc_state *crtc_state;
> > > +
> > > +		if (!(connector->has_tile &&
> > > +		      connector->tile_group->id == tile_grp_id))
> > > +			continue;
> > > +		conn_iter_state = drm_atomic_get_connector_state(&state->base,
> > > +								 connector);
> > > +		if (IS_ERR(conn_iter_state)) {
> > > +			drm_connector_list_iter_end(&conn_list_iter);
> > > +			return PTR_ERR(conn_iter_state);
> > > +		}
> > > +
> > > +		if (!conn_iter_state->crtc)
> > > +			continue;
> > > +
> > > +		crtc_state = drm_atomic_get_crtc_state(&state->base,
> > > +						       conn_iter_state->crtc);
> > > +		if (IS_ERR(crtc_state)) {
> > > +			drm_connector_list_iter_end(&conn_list_iter);
> > > +			return PTR_ERR(conn_iter_state);
> > > +		}
> > > +		crtc_state->mode_changed = true;
> > > +	}
> > > +	drm_connector_list_iter_end(&conn_list_iter);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int
> > > +intel_dp_atomic_check_tiled_conns(struct intel_atomic_state *state)
> > > +{
> > > +	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> > > +	struct drm_connector *connector;
> > > +	struct drm_connector_state *old_conn_state, *new_conn_state;
> > > +	int i, ret;
> > > +
> > > +	if (INTEL_GEN(dev_priv) < 11)
> > > +		return 0;
> > > +
> > > +	/* Is tiled, mark all other tiled CRTCs as needing a modeset */
> > > +	for_each_oldnew_connector_in_state(&state->base, connector,
> > > +					   old_conn_state, new_conn_state, i) {
> > > +		if (!connector->has_tile)
> > > +			continue;
> > > +		if (!intel_connector_needs_modeset(state, old_conn_state,
> > > +						   new_conn_state))
> > > +			continue;
> > > +
> > > +		ret = intel_dp_modeset_all_tiles(state, connector->tile_group->id);
> > > +		if (ret)
> > > +			return ret;
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +
> > >  /**
> > >   * intel_atomic_check - validate state object
> > >   * @dev: drm device
> > > @@ -14059,6 +14146,12 @@ static int intel_atomic_check(struct drm_device *dev,
> > >  	if (ret)
> > >  		goto fail;
> > >  
> > > +	ret = intel_dp_atomic_check_tiled_conns(state);
> > > +	if (ret)
> > > +		goto fail;
> > 
> > Since this is not sufficient for the pre-compute_config() check I think
> > we should drop this part for now. It's a somewhat unlikely scenario
> > after all so we can do it as a followup.
> >
> 
> Well we do need this for handling the hotplug cases correctly so that the connector disconnected
> are added to the modeset which is working well with this function.a

Did I reply here?

> 
> Manasi
>  
> > > +
> > > +	intel_dp_atomic_check_synced_crtcs(state);

I meant to reply here. Ie. we should keep intel_dp_atomic_check_tiled_conns()
but drop the intel_dp_atomic_check_synced_crtcs() call here (we will want to
keep the other callsite after the fastset check).

> > > +
> > >  	for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
> > >  					    new_crtc_state, i) {
> > >  		if (!needs_modeset(new_crtc_state)) {
> > > @@ -14089,6 +14182,32 @@ static int intel_atomic_check(struct drm_device *dev,
> > >  			any_ms = true;
> > >  	}
> > >  
> > > +	/*
> > > +	 * In case of port synced crtcs, if one of the synced crtcs
> > > +	 * needs a full modeset, all other synced crtcs should be
> > > +	 * forced a full modeset.
> > > +	 */
> > > +	intel_dp_atomic_check_synced_crtcs(state);
> > > +
> > > +	for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
> > > +					    new_crtc_state, i) {
> > > +		if (needs_modeset(new_crtc_state))
> > > +			continue;
> > > +
> > > +		/*
> > > +		 * If we're not doing the full modeset we want to
> > > +		 * keep the current M/N values as they may be
> > > +		 * sufficiently different to the computed values
> > > +		 * to cause problems.
> > > +		 *
> > > +		 * FIXME: should really copy more fuzzy state here
> > > +		 */
> > > +		new_crtc_state->fdi_m_n = old_crtc_state->fdi_m_n;
> > > +		new_crtc_state->dp_m_n = old_crtc_state->dp_m_n;
> > > +		new_crtc_state->dp_m2_n2 = old_crtc_state->dp_m2_n2;
> > > +		new_crtc_state->has_drrs = old_crtc_state->has_drrs;
> > > +	}
> > > +
> > >  	if (any_ms && !check_digital_port_conflicts(state)) {
> > >  		DRM_DEBUG_KMS("rejecting conflicting digital port configuration\n");
> > >  		ret = EINVAL;
> > > -- 
> > > 2.19.1
> > 
> > -- 
> > Ville Syrjälä
> > Intel

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [Intel-gfx] [PATCH v2 1/3] drm/i915/dp: Make sure all tiled connectors get added to the state with full modeset
  2019-12-20 19:35     ` Ville Syrjälä
@ 2019-12-20 20:13       ` Manasi Navare
  0 siblings, 0 replies; 19+ messages in thread
From: Manasi Navare @ 2019-12-20 20:13 UTC (permalink / raw)
  To: Ville Syrjälä; +Cc: intel-gfx

On Fri, Dec 20, 2019 at 09:35:43PM +0200, Ville Syrjälä wrote:
> On Fri, Dec 20, 2019 at 11:05:31AM -0800, Manasi Navare wrote:
> > On Fri, Dec 20, 2019 at 08:41:12PM +0200, Ville Syrjälä wrote:
> > > On Thu, Dec 19, 2019 at 01:51:15PM -0800, Manasi Navare wrote:
> > > > In case of tiled displays, all the tiles are linke dto each other
> > > > for transcoder port sync. So in intel_atomic_check() we need to make
> > > > sure that we add all the tiles to the modeset and if one of the
> > > > tiles needs a full modeset then mark all other tiles for a full modeset.
> > > > 
> > > > v2:
> > > > * Change crtc_state scope, remove tile_grp_id (Ville)
> > > > * Use intel_connector_needs_modeset() (Ville)
> > > > * Add modeset_synced_crtcs (Ville)
> > > > * Make sure synced crtcs are forced full modeset
> > > > after fastset check (Ville)
> > > > 
> > > > Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > Cc: José Roberto de Souza <jose.souza@intel.com>
> > > > Cc: Matt Roper <matthew.d.roper@intel.com>
> > > > Bugzilla: https://gitlab.freedesktop.org/drm/intel/issues/5
> > > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/display/intel_display.c | 143 +++++++++++++++++--
> > > >  1 file changed, 131 insertions(+), 12 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > > > index a3f9430493ae..00608d8cef50 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > > @@ -13910,18 +13910,6 @@ static void intel_crtc_check_fastset(const struct intel_crtc_state *old_crtc_sta
> > > >  	new_crtc_state->uapi.mode_changed = false;
> > > >  	new_crtc_state->update_pipe = true;
> > > >  
> > > > -	/*
> > > > -	 * If we're not doing the full modeset we want to
> > > > -	 * keep the current M/N values as they may be
> > > > -	 * sufficiently different to the computed values
> > > > -	 * to cause problems.
> > > > -	 *
> > > > -	 * FIXME: should really copy more fuzzy state here
> > > > -	 */
> > > > -	new_crtc_state->fdi_m_n = old_crtc_state->fdi_m_n;
> > > > -	new_crtc_state->dp_m_n = old_crtc_state->dp_m_n;
> > > > -	new_crtc_state->dp_m2_n2 = old_crtc_state->dp_m2_n2;
> > > > -	new_crtc_state->has_drrs = old_crtc_state->has_drrs;
> > > >  }
> > > >  
> > > >  static int intel_crtc_add_planes_to_state(struct intel_atomic_state *state,
> > > > @@ -14032,6 +14020,105 @@ static int intel_atomic_check_crtcs(struct intel_atomic_state *state)
> > > >  	return 0;
> > > >  }
> > > >  
> > > > +static void
> > > > +intel_dp_modeset_synced_crtcs(struct intel_atomic_state *state)
> > > > +{
> > > > +	struct intel_crtc_state *new_crtc_state;
> > > > +	struct intel_crtc *crtc;
> > > > +	int i;
> > > > +
> > > > +	for_each_new_intel_crtc_in_state(state, crtc,
> > > > +					 new_crtc_state, i) {
> > > > +		if (is_trans_port_sync_mode(new_crtc_state)) {
> > > > +			new_crtc_state->uapi.mode_changed = true;
> > > > +			new_crtc_state->update_pipe = false;
> > > > +		}
> > > 
> > > Upon further thinking this is not quite right even for the post fastset
> > > check part. You're not checking at all whether these crtcs here are
> > > actually synced to the crtc that still had its needs_modeset() flagged.
> > > So this can end up forcing a modeset on an entirely unrelated crtc. We
> > > don't want that.
> > > 
> > > You could fix that by passing in a transcoder bitmask, and then only
> > > flagging a modeset on crtcs whose cpu_transcoder matches the bitmask.
> > > (see intel_modeset_transcoders() in my branch).
> > > 
> > > > +	}
> > > > +}
> > > > +
> > > > +static void
> > > > +intel_dp_atomic_check_synced_crtcs(struct intel_atomic_state *state)
> > > > +{
> > > > +	struct intel_crtc_state *new_crtc_state;
> > > > +	struct intel_crtc *crtc;
> > > > +	int i;
> > > > +
> > > > +	for_each_new_intel_crtc_in_state(state, crtc,
> > > > +					 new_crtc_state, i) {
> > > > +		if (!is_trans_port_sync_mode(new_crtc_state) ||
> > > > +		    !needs_modeset(new_crtc_state))
> > > > +			continue;
> > > > +
> > > > +		intel_dp_modeset_synced_crtcs(state);
> > 
> > I guess in this call I can add master trans and sync_mode_slaves_bitmask
> > 
> > And then if the crtcs in state are in sync mode and their cpu_trans matches the master trans
> > or transcoders in the slave mask, add them to the modeset?
> 
> It can be as simple as:
> 
> modeset_transcoders(state, transcoders)
> {
> 	for_each_crtc_in() {
> 		if (transcoders & BIT(cpu_transcoder))
> 			modeset = true;
> 	}	
> }
> 
> intel_dp_atomic_check_synced_crtcs()
> {
> 	for_each_crtc_in() {
> 		if (is_port_sync_maseter() && needs_modeset())
> 			modeset_transcoders(state, slave_mask);
> 		else if (is_port_sync_slave() && needs_modeset())
> 			modeset_transcoders(state, BIT(master_transcoder));
> 	}
> }
> 
> > 
> > 
> > 
> > > > +	}
> > > > +}
> > > > +
> > > > +static int
> > > > +intel_dp_modeset_all_tiles(struct intel_atomic_state *state, int tile_grp_id)
> > > > +{
> > > > +	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> > > > +	struct drm_connector *connector;
> > > > +	struct drm_connector_list_iter conn_list_iter;
> > > > +
> > > > +	drm_connector_list_iter_begin(&dev_priv->drm, &conn_list_iter);
> > > > +	drm_for_each_connector_iter(connector, &conn_list_iter) {
> > > > +		struct drm_connector_state *conn_iter_state;
> > > > +		struct drm_crtc_state *crtc_state;
> > > > +
> > > > +		if (!(connector->has_tile &&
> > > > +		      connector->tile_group->id == tile_grp_id))
> > > > +			continue;
> > > > +		conn_iter_state = drm_atomic_get_connector_state(&state->base,
> > > > +								 connector);
> > > > +		if (IS_ERR(conn_iter_state)) {
> > > > +			drm_connector_list_iter_end(&conn_list_iter);
> > > > +			return PTR_ERR(conn_iter_state);
> > > > +		}
> > > > +
> > > > +		if (!conn_iter_state->crtc)
> > > > +			continue;
> > > > +
> > > > +		crtc_state = drm_atomic_get_crtc_state(&state->base,
> > > > +						       conn_iter_state->crtc);
> > > > +		if (IS_ERR(crtc_state)) {
> > > > +			drm_connector_list_iter_end(&conn_list_iter);
> > > > +			return PTR_ERR(conn_iter_state);
> > > > +		}
> > > > +		crtc_state->mode_changed = true;
> > > > +	}
> > > > +	drm_connector_list_iter_end(&conn_list_iter);
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static int
> > > > +intel_dp_atomic_check_tiled_conns(struct intel_atomic_state *state)
> > > > +{
> > > > +	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> > > > +	struct drm_connector *connector;
> > > > +	struct drm_connector_state *old_conn_state, *new_conn_state;
> > > > +	int i, ret;
> > > > +
> > > > +	if (INTEL_GEN(dev_priv) < 11)
> > > > +		return 0;
> > > > +
> > > > +	/* Is tiled, mark all other tiled CRTCs as needing a modeset */
> > > > +	for_each_oldnew_connector_in_state(&state->base, connector,
> > > > +					   old_conn_state, new_conn_state, i) {
> > > > +		if (!connector->has_tile)
> > > > +			continue;
> > > > +		if (!intel_connector_needs_modeset(state, old_conn_state,
> > > > +						   new_conn_state))
> > > > +			continue;
> > > > +
> > > > +		ret = intel_dp_modeset_all_tiles(state, connector->tile_group->id);
> > > > +		if (ret)
> > > > +			return ret;
> > > > +	}
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > >  /**
> > > >   * intel_atomic_check - validate state object
> > > >   * @dev: drm device
> > > > @@ -14059,6 +14146,12 @@ static int intel_atomic_check(struct drm_device *dev,
> > > >  	if (ret)
> > > >  		goto fail;
> > > >  
> > > > +	ret = intel_dp_atomic_check_tiled_conns(state);
> > > > +	if (ret)
> > > > +		goto fail;
> > > 
> > > Since this is not sufficient for the pre-compute_config() check I think
> > > we should drop this part for now. It's a somewhat unlikely scenario
> > > after all so we can do it as a followup.
> > >
> > 
> > Well we do need this for handling the hotplug cases correctly so that the connector disconnected
> > are added to the modeset which is working well with this function.a
> 
> Did I reply here?
> 
> > 
> > Manasi
> >  
> > > > +
> > > > +	intel_dp_atomic_check_synced_crtcs(state);
> 
> I meant to reply here. Ie. we should keep intel_dp_atomic_check_tiled_conns()
> but drop the intel_dp_atomic_check_synced_crtcs() call here (we will want to
> keep the other callsite after the fastset check).
>

Yes got it thank you so much, I will now resubmit this with these fixes just in a bit

Manasi
 
> > > > +
> > > >  	for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
> > > >  					    new_crtc_state, i) {
> > > >  		if (!needs_modeset(new_crtc_state)) {
> > > > @@ -14089,6 +14182,32 @@ static int intel_atomic_check(struct drm_device *dev,
> > > >  			any_ms = true;
> > > >  	}
> > > >  
> > > > +	/*
> > > > +	 * In case of port synced crtcs, if one of the synced crtcs
> > > > +	 * needs a full modeset, all other synced crtcs should be
> > > > +	 * forced a full modeset.
> > > > +	 */
> > > > +	intel_dp_atomic_check_synced_crtcs(state);
> > > > +
> > > > +	for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
> > > > +					    new_crtc_state, i) {
> > > > +		if (needs_modeset(new_crtc_state))
> > > > +			continue;
> > > > +
> > > > +		/*
> > > > +		 * If we're not doing the full modeset we want to
> > > > +		 * keep the current M/N values as they may be
> > > > +		 * sufficiently different to the computed values
> > > > +		 * to cause problems.
> > > > +		 *
> > > > +		 * FIXME: should really copy more fuzzy state here
> > > > +		 */
> > > > +		new_crtc_state->fdi_m_n = old_crtc_state->fdi_m_n;
> > > > +		new_crtc_state->dp_m_n = old_crtc_state->dp_m_n;
> > > > +		new_crtc_state->dp_m2_n2 = old_crtc_state->dp_m2_n2;
> > > > +		new_crtc_state->has_drrs = old_crtc_state->has_drrs;
> > > > +	}
> > > > +
> > > >  	if (any_ms && !check_digital_port_conflicts(state)) {
> > > >  		DRM_DEBUG_KMS("rejecting conflicting digital port configuration\n");
> > > >  		ret = EINVAL;
> > > > -- 
> > > > 2.19.1
> > > 
> > > -- 
> > > Ville Syrjälä
> > > Intel
> 
> -- 
> Ville Syrjälä
> Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 19+ messages in thread

end of thread, other threads:[~2019-12-20 20:11 UTC | newest]

Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-19 21:51 [Intel-gfx] [PATCH v2 1/3] drm/i915/dp: Make sure all tiled connectors get added to the state with full modeset Manasi Navare
2019-12-19 21:51 ` [Intel-gfx] [PATCH v2 2/3] drm/i915/dp: Make port sync mode assignments only if all tiles present Manasi Navare
2019-12-20 13:59   ` Ville Syrjälä
2019-12-20 17:00     ` Manasi Navare
2019-12-20 17:37       ` Ville Syrjälä
2019-12-20 18:13   ` Ville Syrjälä
2019-12-19 21:51 ` [Intel-gfx] [PATCH v2 3/3] drm/i915/dp: Disable Port sync mode correctly on teardown Manasi Navare
2019-12-20 13:19   ` Ville Syrjälä
2019-12-19 21:56 ` [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for series starting with [v2,1/3] drm/i915/dp: Make sure all tiled connectors get added to the state with full modeset Patchwork
2019-12-19 22:26 ` [Intel-gfx] ✗ Fi.CI.BAT: failure " Patchwork
2019-12-20 13:17 ` [Intel-gfx] [PATCH v2 1/3] " Ville Syrjälä
2019-12-20 16:35   ` Manasi Navare
2019-12-20 16:47     ` Ville Syrjälä
2019-12-20 17:04       ` Manasi Navare
2019-12-20 17:40         ` Ville Syrjälä
2019-12-20 18:41 ` Ville Syrjälä
2019-12-20 19:05   ` Manasi Navare
2019-12-20 19:35     ` Ville Syrjälä
2019-12-20 20:13       ` Manasi Navare

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.