All of lore.kernel.org
 help / color / mirror / Atom feed
* [Intel-gfx] [PATCH v5 1/3] drm/i915/dp: Make sure all tiled connectors get added to the state with full modeset
@ 2019-12-23 22:44 Manasi Navare
  2019-12-23 22:44 ` [Intel-gfx] [PATCH v5 2/3] drm/i915/dp: Make port sync mode assignments only if all tiles present Manasi Navare
                   ` (5 more replies)
  0 siblings, 6 replies; 13+ messages in thread
From: Manasi Navare @ 2019-12-23 22:44 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.

We also need to force modeset for all synced crtcs after fastset check.

v5:
* Rebase
v4:
* Fix logic for modeset_synced_crtcs (Ville)
v3:
* Add tile checks only for Gen >11
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 | 123 +++++++++++++++++++
 1 file changed, 123 insertions(+)

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index 94fc4b5bacc0..45a699bac34a 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -14304,6 +14304,118 @@ static bool intel_cpu_transcoder_needs_modeset(struct intel_atomic_state *state,
 	return false;
 }
 
+static void
+intel_modeset_synced_crtcs(struct intel_atomic_state *state,
+			   u8 transcoders)
+{
+	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 (transcoders & BIT(new_crtc_state->cpu_transcoder)) {
+			new_crtc_state->uapi.mode_changed = true;
+			new_crtc_state->update_pipe = false;
+		}
+	}
+}
+
+static void
+intel_atomic_check_synced_crtcs(struct intel_atomic_state *state)
+{
+	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
+	struct intel_crtc_state *new_crtc_state;
+	struct intel_crtc *crtc;
+	int i;
+
+	if (INTEL_GEN(dev_priv) < 11)
+		return;
+
+	for_each_new_intel_crtc_in_state(state, crtc,
+					 new_crtc_state, i) {
+		if (is_trans_port_sync_master(new_crtc_state) &&
+		    needs_modeset(new_crtc_state)) {
+			intel_modeset_synced_crtcs(state,
+						   new_crtc_state->sync_mode_slaves_mask);
+		} else if (is_trans_port_sync_slave(new_crtc_state) &&
+			   needs_modeset(new_crtc_state)) {
+			intel_modeset_synced_crtcs(state,
+						   BIT(new_crtc_state->master_transcoder));
+		}
+	}
+}
+
+static int
+intel_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_iter;
+	int ret = 0;
+
+	drm_connector_list_iter_begin(&dev_priv->drm, &conn_iter);
+	drm_for_each_connector_iter(connector, &conn_iter) {
+		struct drm_connector_state *conn_state;
+		struct drm_crtc_state *crtc_state;
+
+		if (!connector->has_tile ||
+		    connector->tile_group->id != tile_grp_id)
+			continue;
+		conn_state = drm_atomic_get_connector_state(&state->base,
+							    connector);
+		if (IS_ERR(conn_state)) {
+			ret =  PTR_ERR(conn_state);
+			break;
+		}
+
+		if (!conn_state->crtc)
+			continue;
+
+		crtc_state = drm_atomic_get_crtc_state(&state->base,
+						       conn_state->crtc);
+		if (IS_ERR(crtc_state)) {
+			ret = PTR_ERR(conn_state);
+			break;
+		}
+		crtc_state->mode_changed = true;
+		ret = drm_atomic_add_affected_connectors(&state->base,
+							 conn_state->crtc);
+		if (ret)
+			break;
+	}
+	drm_connector_list_iter_end(&conn_iter);
+
+	return ret;
+}
+
+static int
+intel_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, connector))
+			continue;
+
+		ret = intel_modeset_all_tiles(state, connector->tile_group->id);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
 /**
  * intel_atomic_check - validate state object
  * @dev: drm device
@@ -14331,6 +14443,10 @@ static int intel_atomic_check(struct drm_device *dev,
 	if (ret)
 		goto fail;
 
+	ret = intel_atomic_check_tiled_conns(state);
+	if (ret)
+		goto fail;
+
 	for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
 					    new_crtc_state, i) {
 		if (!needs_modeset(new_crtc_state)) {
@@ -14378,6 +14494,13 @@ static int intel_atomic_check(struct drm_device *dev,
 		}
 	}
 
+	/*
+	 * 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_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)) {
-- 
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] 13+ messages in thread

* [Intel-gfx] [PATCH v5 2/3] drm/i915/dp: Make port sync mode assignments only if all tiles present
  2019-12-23 22:44 [Intel-gfx] [PATCH v5 1/3] drm/i915/dp: Make sure all tiled connectors get added to the state with full modeset Manasi Navare
@ 2019-12-23 22:44 ` Manasi Navare
  2019-12-23 22:44 ` [Intel-gfx] [PATCH v5 3/3] drm/i915/dp: Disable Port sync mode correctly on teardown Manasi Navare
                   ` (4 subsequent siblings)
  5 siblings, 0 replies; 13+ messages in thread
From: Manasi Navare @ 2019-12-23 22:44 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.

v4:
deafulat port sync values in prepare_cleared_state (Ville)
v3:
* Default master trans to INVALID to avoid pipe mismatch
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>
Acked-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/display/intel_display.c | 182 ++++++++++++-------
 1 file changed, 117 insertions(+), 65 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index 45a699bac34a..1eae8e08c740 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -12263,88 +12263,121 @@ 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 bool
+intel_atomic_is_master_connector(struct intel_crtc_state *crtc_state)
+{
+	struct drm_crtc *crtc = crtc_state->uapi.crtc;
+	struct drm_atomic_state *state = crtc_state->uapi.state;
+	struct drm_connector *connector;
+	struct drm_connector_state *connector_state;
+	int i;
+
+	for_each_new_connector_in_state(state, connector, connector_state, i) {
+		if (connector_state->crtc != crtc)
+			continue;
+		if (connector->has_tile &&
+		    connector->tile_h_loc == connector->num_h_tile - 1 &&
+		    connector->tile_v_loc == connector->num_v_tile - 1)
+			return true;
+	}
+
+	return false;
+}
+
+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.
 	 */
-	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's crtc state is already populated in slave for port sync
+	 */
+	if (connector->tile_h_loc == connector->num_h_tile - 1 &&
+	    connector->tile_v_loc == connector->num_v_tile - 1)
+		return 0;
+
+	/* Loop through all connectors and configure the Slave crtc_state
+	 * to point to the correct master.
+	 */
+	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;
 }
 
@@ -12889,9 +12922,11 @@ intel_crtc_prepare_cleared_state(struct intel_crtc_state *crtc_state)
 		saved_state->wm = crtc_state->wm;
 	/*
 	 * Save the slave bitmask which gets filled for master crtc state during
-	 * slave atomic check call.
+	 * slave atomic check call. For all other CRTCs reset the port sync variables
+	 * crtc_state->master_transcoder needs to be set to INVALID
 	 */
-	if (is_trans_port_sync_master(crtc_state))
+	reset_port_sync_mode_state(saved_state);
+	if (intel_atomic_is_master_connector(crtc_state))
 		saved_state->sync_mode_slaves_mask =
 			crtc_state->sync_mode_slaves_mask;
 
@@ -12912,7 +12947,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 =
@@ -12982,13 +13017,22 @@ 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
@@ -12999,6 +13043,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] 13+ messages in thread

* [Intel-gfx] [PATCH v5 3/3] drm/i915/dp: Disable Port sync mode correctly on teardown
  2019-12-23 22:44 [Intel-gfx] [PATCH v5 1/3] drm/i915/dp: Make sure all tiled connectors get added to the state with full modeset Manasi Navare
  2019-12-23 22:44 ` [Intel-gfx] [PATCH v5 2/3] drm/i915/dp: Make port sync mode assignments only if all tiles present Manasi Navare
@ 2019-12-23 22:44 ` Manasi Navare
  2019-12-24  0:35 ` [Intel-gfx] ✗ Fi.CI.BAT: failure for series starting with [v5,1/3] drm/i915/dp: Make sure all tiled connectors get added to the state with full modeset Patchwork
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 13+ messages in thread
From: Manasi Navare @ 2019-12-23 22:44 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

v3:
* Remove reg variable (Matt)
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>
Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/display/intel_ddi.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
index 3a538789c585..3838b37185de 100644
--- a/drivers/gpu/drm/i915/display/intel_ddi.c
+++ b/drivers/gpu/drm/i915/display/intel_ddi.c
@@ -3878,8 +3878,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;
@@ -3887,10 +3885,7 @@ static void icl_disable_transcoder_port_sync(const struct intel_crtc_state *old_
 	DRM_DEBUG_KMS("Disabling Transcoder Port Sync on Slave Transcoder %s\n",
 		      transcoder_name(old_crtc_state->cpu_transcoder));
 
-	reg = TRANS_DDI_FUNC_CTL2(old_crtc_state->cpu_transcoder);
-	trans_ddi_func_ctl2_val = ~(PORT_SYNC_MODE_ENABLE |
-				    PORT_SYNC_MODE_MASTER_SELECT_MASK);
-	I915_WRITE(reg, trans_ddi_func_ctl2_val);
+	I915_WRITE(TRANS_DDI_FUNC_CTL2(old_crtc_state->cpu_transcoder), 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] 13+ messages in thread

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

== Series Details ==

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

== Summary ==

CI Bug Log - changes from CI_DRM_7630 -> Patchwork_15907
====================================================

Summary
-------

  **FAILURE**

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

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

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

### IGT changes ###

#### Possible regressions ####

  * igt@i915_selftest@live_gt_pm:
    - fi-icl-u3:          [PASS][1] -> [DMESG-FAIL][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7630/fi-icl-u3/igt@i915_selftest@live_gt_pm.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15907/fi-icl-u3/igt@i915_selftest@live_gt_pm.html

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

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

### IGT changes ###

#### Issues hit ####

  * igt@kms_flip@basic-flip-vs-wf_vblank:
    - fi-bsw-n3050:       [PASS][3] -> [FAIL][4] ([i915#34])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7630/fi-bsw-n3050/igt@kms_flip@basic-flip-vs-wf_vblank.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15907/fi-bsw-n3050/igt@kms_flip@basic-flip-vs-wf_vblank.html

  
#### Possible fixes ####

  * igt@gem_exec_create@basic:
    - {fi-tgl-u}:         [INCOMPLETE][5] ([fdo#111736]) -> [PASS][6]
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7630/fi-tgl-u/igt@gem_exec_create@basic.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15907/fi-tgl-u/igt@gem_exec_create@basic.html

  * igt@i915_selftest@live_blt:
    - fi-hsw-4770:        [DMESG-FAIL][7] ([i915#725]) -> [PASS][8]
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7630/fi-hsw-4770/igt@i915_selftest@live_blt.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15907/fi-hsw-4770/igt@i915_selftest@live_blt.html

  * igt@i915_selftest@live_execlists:
    - fi-kbl-soraka:      [DMESG-FAIL][9] ([i915#656]) -> [PASS][10]
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7630/fi-kbl-soraka/igt@i915_selftest@live_execlists.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15907/fi-kbl-soraka/igt@i915_selftest@live_execlists.html

  
#### Warnings ####

  * igt@kms_busy@basic-flip-pipe-b:
    - fi-kbl-x1275:       [DMESG-WARN][11] ([i915#62] / [i915#92] / [i915#95]) -> [DMESG-WARN][12] ([i915#62] / [i915#92]) +5 similar issues
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7630/fi-kbl-x1275/igt@kms_busy@basic-flip-pipe-b.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15907/fi-kbl-x1275/igt@kms_busy@basic-flip-pipe-b.html

  * igt@kms_pipe_crc_basic@suspend-read-crc-pipe-a:
    - fi-kbl-x1275:       [DMESG-WARN][13] ([i915#62] / [i915#92]) -> [DMESG-WARN][14] ([i915#62] / [i915#92] / [i915#95]) +2 similar issues
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7630/fi-kbl-x1275/igt@kms_pipe_crc_basic@suspend-read-crc-pipe-a.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15907/fi-kbl-x1275/igt@kms_pipe_crc_basic@suspend-read-crc-pipe-a.html

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

  [fdo#111736]: https://bugs.freedesktop.org/show_bug.cgi?id=111736
  [i915#34]: https://gitlab.freedesktop.org/drm/intel/issues/34
  [i915#62]: https://gitlab.freedesktop.org/drm/intel/issues/62
  [i915#656]: https://gitlab.freedesktop.org/drm/intel/issues/656
  [i915#725]: https://gitlab.freedesktop.org/drm/intel/issues/725
  [i915#92]: https://gitlab.freedesktop.org/drm/intel/issues/92
  [i915#95]: https://gitlab.freedesktop.org/drm/intel/issues/95


Participating hosts (43 -> 40)
------------------------------

  Additional (8): fi-hsw-4770r fi-byt-j1900 fi-skl-6770hq fi-gdg-551 fi-ivb-3770 fi-kbl-7560u fi-bsw-nick fi-snb-2600 
  Missing    (11): fi-ilk-m540 fi-byt-squawks fi-bsw-cyan fi-bwr-2160 fi-cfl-8700k fi-kbl-7500u fi-ctg-p8600 fi-skl-lmem fi-bdw-samus fi-byt-clapper fi-skl-6600u 


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

  * CI: CI-20190529 -> None
  * Linux: CI_DRM_7630 -> Patchwork_15907

  CI-20190529: 20190529
  CI_DRM_7630: 28a2aa0ebf1520ea8a0dd89299f7ceea80dfd96f @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_5352: 0586d205f651674e575351c2d5a7d0760716c9f1 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_15907: 21c7a9863854e11a0818bad63a91e713963c9b9c @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

21c7a9863854 drm/i915/dp: Disable Port sync mode correctly on teardown
9def46fe47bd drm/i915/dp: Make port sync mode assignments only if all tiles present
705cf254c3b1 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_15907/index.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

* [Intel-gfx] ✗ Fi.CI.BAT: failure for series starting with [v5,1/3] drm/i915/dp: Make sure all tiled connectors get added to the state with full modeset (rev2)
  2019-12-23 22:44 [Intel-gfx] [PATCH v5 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-24  0:35 ` [Intel-gfx] ✗ Fi.CI.BAT: failure for series starting with [v5,1/3] drm/i915/dp: Make sure all tiled connectors get added to the state with full modeset Patchwork
@ 2019-12-24  1:24 ` Patchwork
  2019-12-24  3:25 ` [Intel-gfx] ✗ Fi.CI.BAT: failure for series starting with [v5,1/3] drm/i915/dp: Make sure all tiled connectors get added to the state with full modeset (rev3) Patchwork
  2019-12-27 18:36 ` [Intel-gfx] [PATCH v5 1/3] drm/i915/dp: Make sure all tiled connectors get added to the state with full modeset Matt Roper
  5 siblings, 0 replies; 13+ messages in thread
From: Patchwork @ 2019-12-24  1:24 UTC (permalink / raw)
  To: Manasi Navare; +Cc: intel-gfx

== Series Details ==

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

== Summary ==

CI Bug Log - changes from CI_DRM_7630 -> Patchwork_15909
====================================================

Summary
-------

  **FAILURE**

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

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

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

### IGT changes ###

#### Possible regressions ####

  * igt@i915_module_load@reload-with-fault-injection:
    - fi-skl-lmem:        [PASS][1] -> [DMESG-WARN][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7630/fi-skl-lmem/igt@i915_module_load@reload-with-fault-injection.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15909/fi-skl-lmem/igt@i915_module_load@reload-with-fault-injection.html

  * igt@i915_pm_rpm@module-reload:
    - fi-skl-lmem:        [PASS][3] -> [FAIL][4]
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7630/fi-skl-lmem/igt@i915_pm_rpm@module-reload.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15909/fi-skl-lmem/igt@i915_pm_rpm@module-reload.html

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

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

### IGT changes ###

#### Possible fixes ####

  * igt@gem_exec_create@basic:
    - {fi-tgl-u}:         [INCOMPLETE][5] ([fdo#111736]) -> [PASS][6]
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7630/fi-tgl-u/igt@gem_exec_create@basic.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15909/fi-tgl-u/igt@gem_exec_create@basic.html

  * igt@i915_selftest@live_blt:
    - fi-hsw-4770:        [DMESG-FAIL][7] ([i915#725]) -> [PASS][8]
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7630/fi-hsw-4770/igt@i915_selftest@live_blt.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15909/fi-hsw-4770/igt@i915_selftest@live_blt.html

  * igt@i915_selftest@live_execlists:
    - fi-kbl-soraka:      [DMESG-FAIL][9] ([i915#656]) -> [PASS][10]
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7630/fi-kbl-soraka/igt@i915_selftest@live_execlists.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15909/fi-kbl-soraka/igt@i915_selftest@live_execlists.html

  * igt@i915_selftest@live_memory_region:
    - fi-bwr-2160:        [FAIL][11] -> [PASS][12]
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7630/fi-bwr-2160/igt@i915_selftest@live_memory_region.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15909/fi-bwr-2160/igt@i915_selftest@live_memory_region.html

  
#### Warnings ####

  * igt@i915_module_load@reload-with-fault-injection:
    - fi-kbl-x1275:       [DMESG-WARN][13] ([i915#62] / [i915#92]) -> [INCOMPLETE][14] ([i915#879])
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7630/fi-kbl-x1275/igt@i915_module_load@reload-with-fault-injection.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15909/fi-kbl-x1275/igt@i915_module_load@reload-with-fault-injection.html

  * igt@kms_cursor_legacy@basic-busy-flip-before-cursor-legacy:
    - fi-kbl-x1275:       [DMESG-WARN][15] ([i915#62] / [i915#92] / [i915#95]) -> [DMESG-WARN][16] ([i915#62] / [i915#92]) +7 similar issues
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7630/fi-kbl-x1275/igt@kms_cursor_legacy@basic-busy-flip-before-cursor-legacy.html
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15909/fi-kbl-x1275/igt@kms_cursor_legacy@basic-busy-flip-before-cursor-legacy.html

  * igt@kms_pipe_crc_basic@nonblocking-crc-pipe-a:
    - fi-kbl-x1275:       [DMESG-WARN][17] ([i915#62] / [i915#92]) -> [DMESG-WARN][18] ([i915#62] / [i915#92] / [i915#95]) +5 similar issues
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7630/fi-kbl-x1275/igt@kms_pipe_crc_basic@nonblocking-crc-pipe-a.html
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15909/fi-kbl-x1275/igt@kms_pipe_crc_basic@nonblocking-crc-pipe-a.html

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

  [fdo#111736]: https://bugs.freedesktop.org/show_bug.cgi?id=111736
  [i915#62]: https://gitlab.freedesktop.org/drm/intel/issues/62
  [i915#656]: https://gitlab.freedesktop.org/drm/intel/issues/656
  [i915#725]: https://gitlab.freedesktop.org/drm/intel/issues/725
  [i915#879]: https://gitlab.freedesktop.org/drm/intel/issues/879
  [i915#92]: https://gitlab.freedesktop.org/drm/intel/issues/92
  [i915#95]: https://gitlab.freedesktop.org/drm/intel/issues/95


Participating hosts (43 -> 36)
------------------------------

  Additional (5): fi-skl-6770hq fi-glk-dsi fi-whl-u fi-gdg-551 fi-bsw-nick 
  Missing    (12): fi-tgl-guc fi-ilk-m540 fi-hsw-peppy fi-byt-squawks fi-bsw-cyan fi-snb-2520m fi-ctg-p8600 fi-bsw-kefka fi-bdw-samus fi-byt-clapper fi-skl-6700k2 fi-kbl-r 


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

  * CI: CI-20190529 -> None
  * Linux: CI_DRM_7630 -> Patchwork_15909

  CI-20190529: 20190529
  CI_DRM_7630: 28a2aa0ebf1520ea8a0dd89299f7ceea80dfd96f @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_5352: 0586d205f651674e575351c2d5a7d0760716c9f1 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_15909: d9efa05ce0caeb62339e60bf69d9eff91babbe41 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

d9efa05ce0ca drm/i915/dp: Disable Port sync mode correctly on teardown
48b2631ee29c drm/i915/dp: Make port sync mode assignments only if all tiles present
339b57a56faf 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_15909/index.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

* [Intel-gfx] ✗ Fi.CI.BAT: failure for series starting with [v5,1/3] drm/i915/dp: Make sure all tiled connectors get added to the state with full modeset (rev3)
  2019-12-23 22:44 [Intel-gfx] [PATCH v5 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-24  1:24 ` [Intel-gfx] ✗ Fi.CI.BAT: failure for series starting with [v5,1/3] drm/i915/dp: Make sure all tiled connectors get added to the state with full modeset (rev2) Patchwork
@ 2019-12-24  3:25 ` Patchwork
  2019-12-27 18:36 ` [Intel-gfx] [PATCH v5 1/3] drm/i915/dp: Make sure all tiled connectors get added to the state with full modeset Matt Roper
  5 siblings, 0 replies; 13+ messages in thread
From: Patchwork @ 2019-12-24  3:25 UTC (permalink / raw)
  To: Manasi Navare; +Cc: intel-gfx

== Series Details ==

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

== Summary ==

CI Bug Log - changes from CI_DRM_7630 -> Patchwork_15911
====================================================

Summary
-------

  **FAILURE**

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

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

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

### IGT changes ###

#### Possible regressions ####

  * igt@i915_module_load@reload-with-fault-injection:
    - fi-skl-6600u:       [PASS][1] -> [DMESG-WARN][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7630/fi-skl-6600u/igt@i915_module_load@reload-with-fault-injection.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15911/fi-skl-6600u/igt@i915_module_load@reload-with-fault-injection.html

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

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

### IGT changes ###

#### Issues hit ####

  * igt@gem_close_race@basic-threads:
    - fi-byt-n2820:       [PASS][3] -> [TIMEOUT][4] ([i915#816])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7630/fi-byt-n2820/igt@gem_close_race@basic-threads.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15911/fi-byt-n2820/igt@gem_close_race@basic-threads.html

  * igt@gem_exec_suspend@basic-s3:
    - fi-icl-guc:         [PASS][5] -> [INCOMPLETE][6] ([i915#140] / [i915#184])
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7630/fi-icl-guc/igt@gem_exec_suspend@basic-s3.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15911/fi-icl-guc/igt@gem_exec_suspend@basic-s3.html

  * igt@i915_module_load@reload-with-fault-injection:
    - fi-cfl-guc:         [PASS][7] -> [INCOMPLETE][8] ([i915#505] / [i915#671])
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7630/fi-cfl-guc/igt@i915_module_load@reload-with-fault-injection.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15911/fi-cfl-guc/igt@i915_module_load@reload-with-fault-injection.html

  * igt@i915_pm_rpm@module-reload:
    - fi-skl-6600u:       [PASS][9] -> [INCOMPLETE][10] ([i915#151])
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7630/fi-skl-6600u/igt@i915_pm_rpm@module-reload.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15911/fi-skl-6600u/igt@i915_pm_rpm@module-reload.html

  * igt@i915_selftest@live_gt_heartbeat:
    - fi-kbl-guc:         [PASS][11] -> [DMESG-FAIL][12] ([fdo#112406])
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7630/fi-kbl-guc/igt@i915_selftest@live_gt_heartbeat.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15911/fi-kbl-guc/igt@i915_selftest@live_gt_heartbeat.html

  * igt@kms_frontbuffer_tracking@basic:
    - fi-hsw-peppy:       [PASS][13] -> [DMESG-WARN][14] ([i915#44])
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7630/fi-hsw-peppy/igt@kms_frontbuffer_tracking@basic.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15911/fi-hsw-peppy/igt@kms_frontbuffer_tracking@basic.html

  
#### Possible fixes ####

  * igt@i915_selftest@live_memory_region:
    - fi-bwr-2160:        [FAIL][15] -> [PASS][16]
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7630/fi-bwr-2160/igt@i915_selftest@live_memory_region.html
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15911/fi-bwr-2160/igt@i915_selftest@live_memory_region.html

  
#### Warnings ####

  * igt@i915_selftest@live_blt:
    - fi-hsw-4770:        [DMESG-FAIL][17] ([i915#725]) -> [DMESG-FAIL][18] ([i915#563])
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7630/fi-hsw-4770/igt@i915_selftest@live_blt.html
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15911/fi-hsw-4770/igt@i915_selftest@live_blt.html

  * igt@kms_busy@basic-flip-pipe-b:
    - fi-kbl-x1275:       [DMESG-WARN][19] ([i915#62] / [i915#92] / [i915#95]) -> [DMESG-WARN][20] ([i915#62] / [i915#92]) +4 similar issues
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7630/fi-kbl-x1275/igt@kms_busy@basic-flip-pipe-b.html
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15911/fi-kbl-x1275/igt@kms_busy@basic-flip-pipe-b.html

  * igt@kms_flip@basic-flip-vs-modeset:
    - fi-kbl-x1275:       [DMESG-WARN][21] ([i915#62] / [i915#92]) -> [DMESG-WARN][22] ([i915#62] / [i915#92] / [i915#95]) +6 similar issues
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7630/fi-kbl-x1275/igt@kms_flip@basic-flip-vs-modeset.html
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15911/fi-kbl-x1275/igt@kms_flip@basic-flip-vs-modeset.html

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

  [fdo#112406]: https://bugs.freedesktop.org/show_bug.cgi?id=112406
  [i915#140]: https://gitlab.freedesktop.org/drm/intel/issues/140
  [i915#151]: https://gitlab.freedesktop.org/drm/intel/issues/151
  [i915#184]: https://gitlab.freedesktop.org/drm/intel/issues/184
  [i915#435]: https://gitlab.freedesktop.org/drm/intel/issues/435
  [i915#44]: https://gitlab.freedesktop.org/drm/intel/issues/44
  [i915#505]: https://gitlab.freedesktop.org/drm/intel/issues/505
  [i915#563]: https://gitlab.freedesktop.org/drm/intel/issues/563
  [i915#62]: https://gitlab.freedesktop.org/drm/intel/issues/62
  [i915#671]: https://gitlab.freedesktop.org/drm/intel/issues/671
  [i915#725]: https://gitlab.freedesktop.org/drm/intel/issues/725
  [i915#816]: https://gitlab.freedesktop.org/drm/intel/issues/816
  [i915#92]: https://gitlab.freedesktop.org/drm/intel/issues/92
  [i915#95]: https://gitlab.freedesktop.org/drm/intel/issues/95


Participating hosts (43 -> 42)
------------------------------

  Additional (7): fi-hsw-4770r fi-byt-j1900 fi-glk-dsi fi-whl-u fi-gdg-551 fi-bsw-nick fi-snb-2600 
  Missing    (8): fi-kbl-soraka fi-ilk-m540 fi-byt-squawks fi-bsw-cyan fi-ctg-p8600 fi-tgl-y fi-byt-clapper fi-bdw-samus 


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

  * CI: CI-20190529 -> None
  * Linux: CI_DRM_7630 -> Patchwork_15911

  CI-20190529: 20190529
  CI_DRM_7630: 28a2aa0ebf1520ea8a0dd89299f7ceea80dfd96f @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_5352: 0586d205f651674e575351c2d5a7d0760716c9f1 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_15911: 6e7646605f299e40742a58df0b948a998e5baace @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

6e7646605f29 drm/i915/dp: Disable Port sync mode correctly on teardown
cc09de0f13ad drm/i915/dp: Make port sync mode assignments only if all tiles present
bbffa5789563 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_15911/index.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

* Re: [Intel-gfx] [PATCH v5 1/3] drm/i915/dp: Make sure all tiled connectors get added to the state with full modeset
  2019-12-23 22:44 [Intel-gfx] [PATCH v5 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-24  3:25 ` [Intel-gfx] ✗ Fi.CI.BAT: failure for series starting with [v5,1/3] drm/i915/dp: Make sure all tiled connectors get added to the state with full modeset (rev3) Patchwork
@ 2019-12-27 18:36 ` Matt Roper
  2019-12-27 19:27   ` Manasi Navare
  5 siblings, 1 reply; 13+ messages in thread
From: Matt Roper @ 2019-12-27 18:36 UTC (permalink / raw)
  To: Manasi Navare; +Cc: intel-gfx

On Mon, Dec 23, 2019 at 02:44:27PM -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.
> 
> We also need to force modeset for all synced crtcs after fastset check.
> 
> v5:
> * Rebase

I sent a reply to your v4 of this right at the same time you sent out
v5, but I'm not sure if my reply went through since it doesn't show up
in patchwork.  I've included the same feedback I gave on v4 below in
case it got lost, plus a few more comments.

> v4:
> * Fix logic for modeset_synced_crtcs (Ville)
> v3:
> * Add tile checks only for Gen >11
> 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 | 123 +++++++++++++++++++
>  1 file changed, 123 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index 94fc4b5bacc0..45a699bac34a 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -14304,6 +14304,118 @@ static bool intel_cpu_transcoder_needs_modeset(struct intel_atomic_state *state,
>  	return false;
>  }
>  
> +static void
> +intel_modeset_synced_crtcs(struct intel_atomic_state *state,
> +			   u8 transcoders)
> +{
> +	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) {

Are we guaranteed to have the other CRTC in the state at this point?  If
the tile information was gone at the beginning of the transaction, then
it wouldn't have known to bring in the other CRTC.  So if a modeset was
only submitted on one of the two synchronized CRTCs then I don't see
where the other one would be added to the transaction before this point?

> +		if (transcoders & BIT(new_crtc_state->cpu_transcoder)) {
> +			new_crtc_state->uapi.mode_changed = true;
> +			new_crtc_state->update_pipe = false;
> +		}
> +	}
> +}
> +
> +static void
> +intel_atomic_check_synced_crtcs(struct intel_atomic_state *state)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> +	struct intel_crtc_state *new_crtc_state;
> +	struct intel_crtc *crtc;
> +	int i;
> +
> +	if (INTEL_GEN(dev_priv) < 11)
> +		return;
> +
> +	for_each_new_intel_crtc_in_state(state, crtc,
> +					 new_crtc_state, i) {
> +		if (is_trans_port_sync_master(new_crtc_state) &&
> +		    needs_modeset(new_crtc_state)) {
> +			intel_modeset_synced_crtcs(state,
> +						   new_crtc_state->sync_mode_slaves_mask);
> +		} else if (is_trans_port_sync_slave(new_crtc_state) &&
> +			   needs_modeset(new_crtc_state)) {
> +			intel_modeset_synced_crtcs(state,
> +						   BIT(new_crtc_state->master_transcoder));
> +		}
> +	}
> +}
> +
> +static int
> +intel_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_iter;
> +	int ret = 0;
> +
> +	drm_connector_list_iter_begin(&dev_priv->drm, &conn_iter);
> +	drm_for_each_connector_iter(connector, &conn_iter) {
> +		struct drm_connector_state *conn_state;
> +		struct drm_crtc_state *crtc_state;
> +
> +		if (!connector->has_tile ||
> +		    connector->tile_group->id != tile_grp_id)
> +			continue;
> +		conn_state = drm_atomic_get_connector_state(&state->base,
> +							    connector);
> +		if (IS_ERR(conn_state)) {
> +			ret =  PTR_ERR(conn_state);
> +			break;
> +		}
> +
> +		if (!conn_state->crtc)
> +			continue;
> +
> +		crtc_state = drm_atomic_get_crtc_state(&state->base,
> +						       conn_state->crtc);
> +		if (IS_ERR(crtc_state)) {
> +			ret = PTR_ERR(conn_state);
> +			break;
> +		}
> +		crtc_state->mode_changed = true;
> +		ret = drm_atomic_add_affected_connectors(&state->base,
> +							 conn_state->crtc);
> +		if (ret)
> +			break;
> +	}
> +	drm_connector_list_iter_end(&conn_iter);
> +
> +	return ret;
> +}

There isn't really anything i915-specific in this function and it feels
like something other drivers may need as well if they treat tiled
monitors in a similar manner.  We may want to consider pulling this out
to a DRM core helper, although we can do that in a future patch.

With a bit of extra work, the function below could also potentially be
moved to the core too.  Do you know if there's other hardware with port
sync capabilities that could benefit from these?

> +
> +static int
> +intel_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, connector))
> +			continue;
> +
> +		ret = intel_modeset_all_tiles(state, connector->tile_group->id);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
>  /**
>   * intel_atomic_check - validate state object
>   * @dev: drm device
> @@ -14331,6 +14443,10 @@ static int intel_atomic_check(struct drm_device *dev,
>  	if (ret)
>  		goto fail;
>  
> +	ret = intel_atomic_check_tiled_conns(state);
> +	if (ret)
> +		goto fail;
> +
>  	for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
>  					    new_crtc_state, i) {
>  		if (!needs_modeset(new_crtc_state)) {
> @@ -14378,6 +14494,13 @@ static int intel_atomic_check(struct drm_device *dev,
>  		}
>  	}
>  
> +	/*
> +	 * 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.

Since it's somewhat non-intuitive, I'd add a little bit of extra
explanation for why intel_atomic_check_tiled_conns above didn't already
take care of this.  I.e., if you've plugged in a different monitor, the
tile information may have already vanished from drm_connector by the
time we start this atomic transaction, so we still need to deal with
connectors that used to be tiled (and thus were port-synced) but no
longer are.


> +	 */
> +	intel_atomic_check_synced_crtcs(state);

Although I can't think of a reason why it would cause a problem in this
case, we do seem to be violating the directions in the big kerneldoc
warning attached to drm_atomic_helper_check_modeset().  I.e. if we set
mode_changed in our own check functions, then we're supposed to re-call
drm_atomic_helper_check_modeset() to make sure everything is properly
handled.  If we're not going to follow those directions, we should
probably be clear why we don't think it's necessary.

Actually, I wonder if some of this tiling handling should eventually
migrate into that core helper function in the future...


Matt

> +
>  	for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
>  					    new_crtc_state, i) {
>  		if (needs_modeset(new_crtc_state)) {
> -- 
> 2.19.1
> 

-- 
Matt Roper
Graphics Software Engineer
VTT-OSGC Platform Enablement
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

* Re: [Intel-gfx] [PATCH v5 1/3] drm/i915/dp: Make sure all tiled connectors get added to the state with full modeset
  2019-12-27 18:36 ` [Intel-gfx] [PATCH v5 1/3] drm/i915/dp: Make sure all tiled connectors get added to the state with full modeset Matt Roper
@ 2019-12-27 19:27   ` Manasi Navare
  2019-12-27 20:03     ` Matt Roper
  0 siblings, 1 reply; 13+ messages in thread
From: Manasi Navare @ 2019-12-27 19:27 UTC (permalink / raw)
  To: Matt Roper; +Cc: intel-gfx

On Fri, Dec 27, 2019 at 10:36:52AM -0800, Matt Roper wrote:
> On Mon, Dec 23, 2019 at 02:44:27PM -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.
> > 
> > We also need to force modeset for all synced crtcs after fastset check.
> > 
> > v5:
> > * Rebase
> 
> I sent a reply to your v4 of this right at the same time you sent out
> v5, but I'm not sure if my reply went through since it doesn't show up
> in patchwork.  I've included the same feedback I gave on v4 below in
> case it got lost, plus a few more comments.
> 
> > v4:
> > * Fix logic for modeset_synced_crtcs (Ville)
> > v3:
> > * Add tile checks only for Gen >11
> > 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 | 123 +++++++++++++++++++
> >  1 file changed, 123 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > index 94fc4b5bacc0..45a699bac34a 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -14304,6 +14304,118 @@ static bool intel_cpu_transcoder_needs_modeset(struct intel_atomic_state *state,
> >  	return false;
> >  }
> >  
> > +static void
> > +intel_modeset_synced_crtcs(struct intel_atomic_state *state,
> > +			   u8 transcoders)
> > +{
> > +	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) {
> 
> Are we guaranteed to have the other CRTC in the state at this point?  If
> the tile information was gone at the beginning of the transaction, then
> it wouldn't have known to bring in the other CRTC.  So if a modeset was
> only submitted on one of the two synchronized CRTCs then I don't see
> where the other one would be added to the transaction before this point?
>

Yes basically at this point the new crtc state is not cleared so we use the 
previously synced crtcs through new crtc state master slave assignments from previous
modeset.

 
> > +		if (transcoders & BIT(new_crtc_state->cpu_transcoder)) {
> > +			new_crtc_state->uapi.mode_changed = true;
> > +			new_crtc_state->update_pipe = false;
> > +		}
> > +	}
> > +}
> > +
> > +static void
> > +intel_atomic_check_synced_crtcs(struct intel_atomic_state *state)
> > +{
> > +	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> > +	struct intel_crtc_state *new_crtc_state;
> > +	struct intel_crtc *crtc;
> > +	int i;
> > +
> > +	if (INTEL_GEN(dev_priv) < 11)
> > +		return;
> > +
> > +	for_each_new_intel_crtc_in_state(state, crtc,
> > +					 new_crtc_state, i) {
> > +		if (is_trans_port_sync_master(new_crtc_state) &&
> > +		    needs_modeset(new_crtc_state)) {
> > +			intel_modeset_synced_crtcs(state,
> > +						   new_crtc_state->sync_mode_slaves_mask);
> > +		} else if (is_trans_port_sync_slave(new_crtc_state) &&
> > +			   needs_modeset(new_crtc_state)) {
> > +			intel_modeset_synced_crtcs(state,
> > +						   BIT(new_crtc_state->master_transcoder));
> > +		}
> > +	}
> > +}
> > +
> > +static int
> > +intel_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_iter;
> > +	int ret = 0;
> > +
> > +	drm_connector_list_iter_begin(&dev_priv->drm, &conn_iter);
> > +	drm_for_each_connector_iter(connector, &conn_iter) {
> > +		struct drm_connector_state *conn_state;
> > +		struct drm_crtc_state *crtc_state;
> > +
> > +		if (!connector->has_tile ||
> > +		    connector->tile_group->id != tile_grp_id)
> > +			continue;
> > +		conn_state = drm_atomic_get_connector_state(&state->base,
> > +							    connector);
> > +		if (IS_ERR(conn_state)) {
> > +			ret =  PTR_ERR(conn_state);
> > +			break;
> > +		}
> > +
> > +		if (!conn_state->crtc)
> > +			continue;
> > +
> > +		crtc_state = drm_atomic_get_crtc_state(&state->base,
> > +						       conn_state->crtc);
> > +		if (IS_ERR(crtc_state)) {
> > +			ret = PTR_ERR(conn_state);
> > +			break;
> > +		}
> > +		crtc_state->mode_changed = true;
> > +		ret = drm_atomic_add_affected_connectors(&state->base,
> > +							 conn_state->crtc);
> > +		if (ret)
> > +			break;
> > +	}
> > +	drm_connector_list_iter_end(&conn_iter);
> > +
> > +	return ret;
> > +}
> 
> There isn't really anything i915-specific in this function and it feels
> like something other drivers may need as well if they treat tiled
> monitors in a similar manner.  We may want to consider pulling this out
> to a DRM core helper, although we can do that in a future patch.
> 
> With a bit of extra work, the function below could also potentially be
> moved to the core too.  Do you know if there's other hardware with port
> sync capabilities that could benefit from these?
>

Yes definitely can eventually make this a drm helper even though the whole master slave
thing is i915 specific for now
 
> > +
> > +static int
> > +intel_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, connector))
> > +			continue;
> > +
> > +		ret = intel_modeset_all_tiles(state, connector->tile_group->id);
> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> >  /**
> >   * intel_atomic_check - validate state object
> >   * @dev: drm device
> > @@ -14331,6 +14443,10 @@ static int intel_atomic_check(struct drm_device *dev,
> >  	if (ret)
> >  		goto fail;
> >  
> > +	ret = intel_atomic_check_tiled_conns(state);
> > +	if (ret)
> > +		goto fail;
> > +
> >  	for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
> >  					    new_crtc_state, i) {
> >  		if (!needs_modeset(new_crtc_state)) {
> > @@ -14378,6 +14494,13 @@ static int intel_atomic_check(struct drm_device *dev,
> >  		}
> >  	}
> >  
> > +	/*
> > +	 * 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.
> 
> Since it's somewhat non-intuitive, I'd add a little bit of extra
> explanation for why intel_atomic_check_tiled_conns above didn't already
> take care of this.  I.e., if you've plugged in a different monitor, the
> tile information may have already vanished from drm_connector by the
> time we start this atomic transaction, so we still need to deal with
> connectors that used to be tiled (and thus were port-synced) but no
> longer are.
>

Yes will add this in the comments
 
> 
> > +	 */
> > +	intel_atomic_check_synced_crtcs(state);
> 
> Although I can't think of a reason why it would cause a problem in this
> case, we do seem to be violating the directions in the big kerneldoc
> warning attached to drm_atomic_helper_check_modeset().  I.e. if we set
> mode_changed in our own check functions, then we're supposed to re-call
> drm_atomic_helper_check_modeset() to make sure everything is properly
> handled.  If we're not going to follow those directions, we should
> probably be clear why we don't think it's necessary.
> 
> Actually, I wonder if some of this tiling handling should eventually
> migrate into that core helper function in the future...
>

We are setting the mode hanged to true directly in our function so yes it makes
sense to eventually do that as part of the core helper function but i still
dont see the need for us to call drm_atomic_helper_check_modeset() again
since our function is essentially checking modeset based on the tiled conditions

Manasi
 
> 
> Matt
> 
> > +
> >  	for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
> >  					    new_crtc_state, i) {
> >  		if (needs_modeset(new_crtc_state)) {
> > -- 
> > 2.19.1
> > 
> 
> -- 
> Matt Roper
> Graphics Software Engineer
> VTT-OSGC Platform Enablement
> Intel Corporation
> (916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

* Re: [Intel-gfx] [PATCH v5 1/3] drm/i915/dp: Make sure all tiled connectors get added to the state with full modeset
  2019-12-27 19:27   ` Manasi Navare
@ 2019-12-27 20:03     ` Matt Roper
  2019-12-27 22:51       ` Manasi Navare
  0 siblings, 1 reply; 13+ messages in thread
From: Matt Roper @ 2019-12-27 20:03 UTC (permalink / raw)
  To: Manasi Navare; +Cc: intel-gfx

On Fri, Dec 27, 2019 at 11:27:50AM -0800, Manasi Navare wrote:
> On Fri, Dec 27, 2019 at 10:36:52AM -0800, Matt Roper wrote:
> > On Mon, Dec 23, 2019 at 02:44:27PM -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.
> > > 
> > > We also need to force modeset for all synced crtcs after fastset check.
> > > 
> > > v5:
> > > * Rebase
> > 
> > I sent a reply to your v4 of this right at the same time you sent out
> > v5, but I'm not sure if my reply went through since it doesn't show up
> > in patchwork.  I've included the same feedback I gave on v4 below in
> > case it got lost, plus a few more comments.
> > 
> > > v4:
> > > * Fix logic for modeset_synced_crtcs (Ville)
> > > v3:
> > > * Add tile checks only for Gen >11
> > > 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 | 123 +++++++++++++++++++
> > >  1 file changed, 123 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > > index 94fc4b5bacc0..45a699bac34a 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > @@ -14304,6 +14304,118 @@ static bool intel_cpu_transcoder_needs_modeset(struct intel_atomic_state *state,
> > >  	return false;
> > >  }
> > >  
> > > +static void
> > > +intel_modeset_synced_crtcs(struct intel_atomic_state *state,
> > > +			   u8 transcoders)
> > > +{
> > > +	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) {
> > 
> > Are we guaranteed to have the other CRTC in the state at this point?  If
> > the tile information was gone at the beginning of the transaction, then
> > it wouldn't have known to bring in the other CRTC.  So if a modeset was
> > only submitted on one of the two synchronized CRTCs then I don't see
> > where the other one would be added to the transaction before this point?
> >
> 
> Yes basically at this point the new crtc state is not cleared so we use the 
> previously synced crtcs through new crtc state master slave assignments from previous
> modeset.

I'm not sure we're talking about the same thing here.  Initially an
atomic state (transaction) is empty, and all mode objects have a
currently-committed state, but no new state associated with this
specific transaction.  If userspace requests a modeset on CRTC-A (e.g.,
master), that will add CRTC-A to the drm_atomic_state, but won't
automatically add CRTC-B (slave).  CRTC B's committed state does indeed
indicate that it's part of a port-sync pair, but it won't get iterated
over in the loop above because it still hasn't been added with an
explicit drm_atomic_get_crtc_state().  So we'll never set mode_changed
on CRTC-B as far as I can see because nothing ever brought it into the
transaction (the only place where we bring in new CRTCs in this patch is
when handling the tiles).

> 
>  
> > > +		if (transcoders & BIT(new_crtc_state->cpu_transcoder)) {
> > > +			new_crtc_state->uapi.mode_changed = true;
> > > +			new_crtc_state->update_pipe = false;
> > > +		}
> > > +	}
> > > +}
> > > +
> > > +static void
> > > +intel_atomic_check_synced_crtcs(struct intel_atomic_state *state)
> > > +{
> > > +	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> > > +	struct intel_crtc_state *new_crtc_state;
> > > +	struct intel_crtc *crtc;
> > > +	int i;
> > > +
> > > +	if (INTEL_GEN(dev_priv) < 11)
> > > +		return;
> > > +
> > > +	for_each_new_intel_crtc_in_state(state, crtc,
> > > +					 new_crtc_state, i) {
> > > +		if (is_trans_port_sync_master(new_crtc_state) &&
> > > +		    needs_modeset(new_crtc_state)) {
> > > +			intel_modeset_synced_crtcs(state,
> > > +						   new_crtc_state->sync_mode_slaves_mask);
> > > +		} else if (is_trans_port_sync_slave(new_crtc_state) &&
> > > +			   needs_modeset(new_crtc_state)) {
> > > +			intel_modeset_synced_crtcs(state,
> > > +						   BIT(new_crtc_state->master_transcoder));
> > > +		}
> > > +	}
> > > +}
> > > +
> > > +static int
> > > +intel_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_iter;
> > > +	int ret = 0;
> > > +
> > > +	drm_connector_list_iter_begin(&dev_priv->drm, &conn_iter);
> > > +	drm_for_each_connector_iter(connector, &conn_iter) {
> > > +		struct drm_connector_state *conn_state;
> > > +		struct drm_crtc_state *crtc_state;
> > > +
> > > +		if (!connector->has_tile ||
> > > +		    connector->tile_group->id != tile_grp_id)
> > > +			continue;
> > > +		conn_state = drm_atomic_get_connector_state(&state->base,
> > > +							    connector);
> > > +		if (IS_ERR(conn_state)) {
> > > +			ret =  PTR_ERR(conn_state);
> > > +			break;
> > > +		}
> > > +
> > > +		if (!conn_state->crtc)
> > > +			continue;
> > > +
> > > +		crtc_state = drm_atomic_get_crtc_state(&state->base,
> > > +						       conn_state->crtc);
> > > +		if (IS_ERR(crtc_state)) {
> > > +			ret = PTR_ERR(conn_state);
> > > +			break;
> > > +		}
> > > +		crtc_state->mode_changed = true;
> > > +		ret = drm_atomic_add_affected_connectors(&state->base,
> > > +							 conn_state->crtc);
> > > +		if (ret)
> > > +			break;
> > > +	}
> > > +	drm_connector_list_iter_end(&conn_iter);
> > > +
> > > +	return ret;
> > > +}
> > 
> > There isn't really anything i915-specific in this function and it feels
> > like something other drivers may need as well if they treat tiled
> > monitors in a similar manner.  We may want to consider pulling this out
> > to a DRM core helper, although we can do that in a future patch.
> > 
> > With a bit of extra work, the function below could also potentially be
> > moved to the core too.  Do you know if there's other hardware with port
> > sync capabilities that could benefit from these?
> >
> 
> Yes definitely can eventually make this a drm helper even though the whole master slave
> thing is i915 specific for now
>  
> > > +
> > > +static int
> > > +intel_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, connector))
> > > +			continue;
> > > +
> > > +		ret = intel_modeset_all_tiles(state, connector->tile_group->id);
> > > +		if (ret)
> > > +			return ret;
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +
> > >  /**
> > >   * intel_atomic_check - validate state object
> > >   * @dev: drm device
> > > @@ -14331,6 +14443,10 @@ static int intel_atomic_check(struct drm_device *dev,
> > >  	if (ret)
> > >  		goto fail;
> > >  
> > > +	ret = intel_atomic_check_tiled_conns(state);
> > > +	if (ret)
> > > +		goto fail;
> > > +
> > >  	for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
> > >  					    new_crtc_state, i) {
> > >  		if (!needs_modeset(new_crtc_state)) {
> > > @@ -14378,6 +14494,13 @@ static int intel_atomic_check(struct drm_device *dev,
> > >  		}
> > >  	}
> > >  
> > > +	/*
> > > +	 * 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.
> > 
> > Since it's somewhat non-intuitive, I'd add a little bit of extra
> > explanation for why intel_atomic_check_tiled_conns above didn't already
> > take care of this.  I.e., if you've plugged in a different monitor, the
> > tile information may have already vanished from drm_connector by the
> > time we start this atomic transaction, so we still need to deal with
> > connectors that used to be tiled (and thus were port-synced) but no
> > longer are.
> >
> 
> Yes will add this in the comments
>  
> > 
> > > +	 */
> > > +	intel_atomic_check_synced_crtcs(state);
> > 
> > Although I can't think of a reason why it would cause a problem in this
> > case, we do seem to be violating the directions in the big kerneldoc
> > warning attached to drm_atomic_helper_check_modeset().  I.e. if we set
> > mode_changed in our own check functions, then we're supposed to re-call
> > drm_atomic_helper_check_modeset() to make sure everything is properly
> > handled.  If we're not going to follow those directions, we should
> > probably be clear why we don't think it's necessary.
> > 
> > Actually, I wonder if some of this tiling handling should eventually
> > migrate into that core helper function in the future...
> >
> 
> We are setting the mode hanged to true directly in our function so yes it makes
> sense to eventually do that as part of the core helper function but i still
> dont see the need for us to call drm_atomic_helper_check_modeset() again
> since our function is essentially checking modeset based on the tiled conditions

I'd still like to see either a commit message note or a code comment
justifying this.  The instructions say we need to re-invoke that
function in cases like what we're doing here, so if we're intentionally
not following those directions we should explain why for future
reference.  It's also possible that function may expand its role in the
future and start doing something new that we really do need to worry
about, so it would be good to have the history and justification for not
calling it documented.


Matt

> 
> Manasi
>  
> > 
> > Matt
> > 
> > > +
> > >  	for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
> > >  					    new_crtc_state, i) {
> > >  		if (needs_modeset(new_crtc_state)) {
> > > -- 
> > > 2.19.1
> > > 
> > 
> > -- 
> > Matt Roper
> > Graphics Software Engineer
> > VTT-OSGC Platform Enablement
> > Intel Corporation
> > (916) 356-2795

-- 
Matt Roper
Graphics Software Engineer
VTT-OSGC Platform Enablement
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

* Re: [Intel-gfx] [PATCH v5 1/3] drm/i915/dp: Make sure all tiled connectors get added to the state with full modeset
  2019-12-27 20:03     ` Matt Roper
@ 2019-12-27 22:51       ` Manasi Navare
  2019-12-27 23:17         ` Matt Roper
  0 siblings, 1 reply; 13+ messages in thread
From: Manasi Navare @ 2019-12-27 22:51 UTC (permalink / raw)
  To: Matt Roper; +Cc: intel-gfx

On Fri, Dec 27, 2019 at 12:03:33PM -0800, Matt Roper wrote:
> On Fri, Dec 27, 2019 at 11:27:50AM -0800, Manasi Navare wrote:
> > On Fri, Dec 27, 2019 at 10:36:52AM -0800, Matt Roper wrote:
> > > On Mon, Dec 23, 2019 at 02:44:27PM -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.
> > > > 
> > > > We also need to force modeset for all synced crtcs after fastset check.
> > > > 
> > > > v5:
> > > > * Rebase
> > > 
> > > I sent a reply to your v4 of this right at the same time you sent out
> > > v5, but I'm not sure if my reply went through since it doesn't show up
> > > in patchwork.  I've included the same feedback I gave on v4 below in
> > > case it got lost, plus a few more comments.
> > > 
> > > > v4:
> > > > * Fix logic for modeset_synced_crtcs (Ville)
> > > > v3:
> > > > * Add tile checks only for Gen >11
> > > > 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 | 123 +++++++++++++++++++
> > > >  1 file changed, 123 insertions(+)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > > > index 94fc4b5bacc0..45a699bac34a 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > > @@ -14304,6 +14304,118 @@ static bool intel_cpu_transcoder_needs_modeset(struct intel_atomic_state *state,
> > > >  	return false;
> > > >  }
> > > >  
> > > > +static void
> > > > +intel_modeset_synced_crtcs(struct intel_atomic_state *state,
> > > > +			   u8 transcoders)
> > > > +{
> > > > +	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) {
> > > 
> > > Are we guaranteed to have the other CRTC in the state at this point?  If
> > > the tile information was gone at the beginning of the transaction, then
> > > it wouldn't have known to bring in the other CRTC.  So if a modeset was
> > > only submitted on one of the two synchronized CRTCs then I don't see
> > > where the other one would be added to the transaction before this point?
> > >
> > 
> > Yes basically at this point the new crtc state is not cleared so we use the 
> > previously synced crtcs through new crtc state master slave assignments from previous
> > modeset.
> 
> I'm not sure we're talking about the same thing here.  Initially an
> atomic state (transaction) is empty, and all mode objects have a
> currently-committed state, but no new state associated with this
> specific transaction.  If userspace requests a modeset on CRTC-A (e.g.,
> master), that will add CRTC-A to the drm_atomic_state, but won't
> automatically add CRTC-B (slave).  CRTC B's committed state does indeed
> indicate that it's part of a port-sync pair, but it won't get iterated
> over in the loop above because it still hasn't been added with an
> explicit drm_atomic_get_crtc_state().  So we'll never set mode_changed
> on CRTC-B as far as I can see because nothing ever brought it into the
> transaction (the only place where we bring in new CRTCs in this patch is
> when handling the tiles).
>

Ah okay I understand your concern now. So if you see the first call intel_atomic_check_tiled_conns(), that 
one loops through all the connectors not ust the ones in the state and adds all connectors with tiles to the
modeset. If we unplug a conn an connect a new monitor that is untiled, the conn state goes from conn to unconn
and that should triggera full modeset even though the tile info does not exist.

Now this modeset_synced_crtcs() is called after new master slave assignments are done and the only purpose
of this call is to make sure that we dont override the full modeset with fastset and that if one either master
or slave needs full modeset other one also shd be forced modeset.

This patch as of now just fixes the current hotplug issues, more enhancements to cover other corner cases
with get_Connector overriding the tile info etc will be taken care of later as per discussions with Ville
 
> > 
> >  
> > > > +		if (transcoders & BIT(new_crtc_state->cpu_transcoder)) {
> > > > +			new_crtc_state->uapi.mode_changed = true;
> > > > +			new_crtc_state->update_pipe = false;
> > > > +		}
> > > > +	}
> > > > +}
> > > > +
> > > > +static void
> > > > +intel_atomic_check_synced_crtcs(struct intel_atomic_state *state)
> > > > +{
> > > > +	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> > > > +	struct intel_crtc_state *new_crtc_state;
> > > > +	struct intel_crtc *crtc;
> > > > +	int i;
> > > > +
> > > > +	if (INTEL_GEN(dev_priv) < 11)
> > > > +		return;
> > > > +
> > > > +	for_each_new_intel_crtc_in_state(state, crtc,
> > > > +					 new_crtc_state, i) {
> > > > +		if (is_trans_port_sync_master(new_crtc_state) &&
> > > > +		    needs_modeset(new_crtc_state)) {
> > > > +			intel_modeset_synced_crtcs(state,
> > > > +						   new_crtc_state->sync_mode_slaves_mask);
> > > > +		} else if (is_trans_port_sync_slave(new_crtc_state) &&
> > > > +			   needs_modeset(new_crtc_state)) {
> > > > +			intel_modeset_synced_crtcs(state,
> > > > +						   BIT(new_crtc_state->master_transcoder));
> > > > +		}
> > > > +	}
> > > > +}
> > > > +
> > > > +static int
> > > > +intel_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_iter;
> > > > +	int ret = 0;
> > > > +
> > > > +	drm_connector_list_iter_begin(&dev_priv->drm, &conn_iter);
> > > > +	drm_for_each_connector_iter(connector, &conn_iter) {
> > > > +		struct drm_connector_state *conn_state;
> > > > +		struct drm_crtc_state *crtc_state;
> > > > +
> > > > +		if (!connector->has_tile ||
> > > > +		    connector->tile_group->id != tile_grp_id)
> > > > +			continue;
> > > > +		conn_state = drm_atomic_get_connector_state(&state->base,
> > > > +							    connector);
> > > > +		if (IS_ERR(conn_state)) {
> > > > +			ret =  PTR_ERR(conn_state);
> > > > +			break;
> > > > +		}
> > > > +
> > > > +		if (!conn_state->crtc)
> > > > +			continue;
> > > > +
> > > > +		crtc_state = drm_atomic_get_crtc_state(&state->base,
> > > > +						       conn_state->crtc);
> > > > +		if (IS_ERR(crtc_state)) {
> > > > +			ret = PTR_ERR(conn_state);
> > > > +			break;
> > > > +		}
> > > > +		crtc_state->mode_changed = true;
> > > > +		ret = drm_atomic_add_affected_connectors(&state->base,
> > > > +							 conn_state->crtc);
> > > > +		if (ret)
> > > > +			break;
> > > > +	}
> > > > +	drm_connector_list_iter_end(&conn_iter);
> > > > +
> > > > +	return ret;
> > > > +}
> > > 
> > > There isn't really anything i915-specific in this function and it feels
> > > like something other drivers may need as well if they treat tiled
> > > monitors in a similar manner.  We may want to consider pulling this out
> > > to a DRM core helper, although we can do that in a future patch.
> > > 
> > > With a bit of extra work, the function below could also potentially be
> > > moved to the core too.  Do you know if there's other hardware with port
> > > sync capabilities that could benefit from these?
> > >
> > 
> > Yes definitely can eventually make this a drm helper even though the whole master slave
> > thing is i915 specific for now
> >  
> > > > +
> > > > +static int
> > > > +intel_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, connector))
> > > > +			continue;
> > > > +
> > > > +		ret = intel_modeset_all_tiles(state, connector->tile_group->id);
> > > > +		if (ret)
> > > > +			return ret;
> > > > +	}
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > >  /**
> > > >   * intel_atomic_check - validate state object
> > > >   * @dev: drm device
> > > > @@ -14331,6 +14443,10 @@ static int intel_atomic_check(struct drm_device *dev,
> > > >  	if (ret)
> > > >  		goto fail;
> > > >  
> > > > +	ret = intel_atomic_check_tiled_conns(state);
> > > > +	if (ret)
> > > > +		goto fail;
> > > > +
> > > >  	for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
> > > >  					    new_crtc_state, i) {
> > > >  		if (!needs_modeset(new_crtc_state)) {
> > > > @@ -14378,6 +14494,13 @@ static int intel_atomic_check(struct drm_device *dev,
> > > >  		}
> > > >  	}
> > > >  
> > > > +	/*
> > > > +	 * 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.
> > > 
> > > Since it's somewhat non-intuitive, I'd add a little bit of extra
> > > explanation for why intel_atomic_check_tiled_conns above didn't already
> > > take care of this.  I.e., if you've plugged in a different monitor, the
> > > tile information may have already vanished from drm_connector by the
> > > time we start this atomic transaction, so we still need to deal with
> > > connectors that used to be tiled (and thus were port-synced) but no
> > > longer are.
> > >
> > 
> > Yes will add this in the comments
> >  
> > > 
> > > > +	 */
> > > > +	intel_atomic_check_synced_crtcs(state);
> > > 
> > > Although I can't think of a reason why it would cause a problem in this
> > > case, we do seem to be violating the directions in the big kerneldoc
> > > warning attached to drm_atomic_helper_check_modeset().  I.e. if we set
> > > mode_changed in our own check functions, then we're supposed to re-call
> > > drm_atomic_helper_check_modeset() to make sure everything is properly
> > > handled.  If we're not going to follow those directions, we should
> > > probably be clear why we don't think it's necessary.
> > > 
> > > Actually, I wonder if some of this tiling handling should eventually
> > > migrate into that core helper function in the future...
> > >
> > 
> > We are setting the mode hanged to true directly in our function so yes it makes
> > sense to eventually do that as part of the core helper function but i still
> > dont see the need for us to call drm_atomic_helper_check_modeset() again
> > since our function is essentially checking modeset based on the tiled conditions
> 
> I'd still like to see either a commit message note or a code comment
> justifying this.  The instructions say we need to re-invoke that
> function in cases like what we're doing here, so if we're intentionally
> not following those directions we should explain why for future
> reference.  It's also possible that function may expand its role in the
> future and start doing something new that we really do need to worry
> about, so it would be good to have the history and justification for not
> calling it documented.
>

Yes adding a bigger comment explaining this is a good idea.

With these changes do you think this is a r-b from you?

Manasi 
> 
> Matt
> 
> > 
> > Manasi
> >  
> > > 
> > > Matt
> > > 
> > > > +
> > > >  	for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
> > > >  					    new_crtc_state, i) {
> > > >  		if (needs_modeset(new_crtc_state)) {
> > > > -- 
> > > > 2.19.1
> > > > 
> > > 
> > > -- 
> > > Matt Roper
> > > Graphics Software Engineer
> > > VTT-OSGC Platform Enablement
> > > Intel Corporation
> > > (916) 356-2795
> 
> -- 
> Matt Roper
> Graphics Software Engineer
> VTT-OSGC Platform Enablement
> Intel Corporation
> (916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

* Re: [Intel-gfx] [PATCH v5 1/3] drm/i915/dp: Make sure all tiled connectors get added to the state with full modeset
  2019-12-27 22:51       ` Manasi Navare
@ 2019-12-27 23:17         ` Matt Roper
  2019-12-27 23:49           ` Manasi Navare
  0 siblings, 1 reply; 13+ messages in thread
From: Matt Roper @ 2019-12-27 23:17 UTC (permalink / raw)
  To: Manasi Navare; +Cc: intel-gfx

On Fri, Dec 27, 2019 at 02:51:28PM -0800, Manasi Navare wrote:
> On Fri, Dec 27, 2019 at 12:03:33PM -0800, Matt Roper wrote:
> > On Fri, Dec 27, 2019 at 11:27:50AM -0800, Manasi Navare wrote:
> > > On Fri, Dec 27, 2019 at 10:36:52AM -0800, Matt Roper wrote:
> > > > On Mon, Dec 23, 2019 at 02:44:27PM -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.
> > > > > 
> > > > > We also need to force modeset for all synced crtcs after fastset check.
> > > > > 
> > > > > v5:
> > > > > * Rebase
> > > > 
> > > > I sent a reply to your v4 of this right at the same time you sent out
> > > > v5, but I'm not sure if my reply went through since it doesn't show up
> > > > in patchwork.  I've included the same feedback I gave on v4 below in
> > > > case it got lost, plus a few more comments.
> > > > 
> > > > > v4:
> > > > > * Fix logic for modeset_synced_crtcs (Ville)
> > > > > v3:
> > > > > * Add tile checks only for Gen >11
> > > > > 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 | 123 +++++++++++++++++++
> > > > >  1 file changed, 123 insertions(+)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > > > > index 94fc4b5bacc0..45a699bac34a 100644
> > > > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > > > @@ -14304,6 +14304,118 @@ static bool intel_cpu_transcoder_needs_modeset(struct intel_atomic_state *state,
> > > > >  	return false;
> > > > >  }
> > > > >  
> > > > > +static void
> > > > > +intel_modeset_synced_crtcs(struct intel_atomic_state *state,
> > > > > +			   u8 transcoders)
> > > > > +{
> > > > > +	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) {
> > > > 
> > > > Are we guaranteed to have the other CRTC in the state at this point?  If
> > > > the tile information was gone at the beginning of the transaction, then
> > > > it wouldn't have known to bring in the other CRTC.  So if a modeset was
> > > > only submitted on one of the two synchronized CRTCs then I don't see
> > > > where the other one would be added to the transaction before this point?
> > > >
> > > 
> > > Yes basically at this point the new crtc state is not cleared so we use the 
> > > previously synced crtcs through new crtc state master slave assignments from previous
> > > modeset.
> > 
> > I'm not sure we're talking about the same thing here.  Initially an
> > atomic state (transaction) is empty, and all mode objects have a
> > currently-committed state, but no new state associated with this
> > specific transaction.  If userspace requests a modeset on CRTC-A (e.g.,
> > master), that will add CRTC-A to the drm_atomic_state, but won't
> > automatically add CRTC-B (slave).  CRTC B's committed state does indeed
> > indicate that it's part of a port-sync pair, but it won't get iterated
> > over in the loop above because it still hasn't been added with an
> > explicit drm_atomic_get_crtc_state().  So we'll never set mode_changed
> > on CRTC-B as far as I can see because nothing ever brought it into the
> > transaction (the only place where we bring in new CRTCs in this patch is
> > when handling the tiles).
> >
> 
> Ah okay I understand your concern now. So if you see the first call
> intel_atomic_check_tiled_conns(), that one loops through all the
> connectors not ust the ones in the state and adds all connectors with
> tiles to the modeset. If we unplug a conn an connect a new monitor
> that is untiled, the conn state goes from conn to unconn and that
> should triggera full modeset even though the tile info does not exist.

Right, intel_atomic_check_tiled_conns loops through all connectors, so
it can pick stuff up that isn't already part of our atomic transaction.
However it won't know it needs to pick up the partner connector in cases
where we've already lost the tile information for the connector that was
re-plugged.

For example, let's say we have connector-A/crtc-A (tile grp 1) and
connector-B/crtc-B (tile grp 1).  The current state for both indicates
they're synced and the tile information for both is still present.

 - Now we unplug connector-B and plug in a new monitor that's either
   untiled or part of a different tile group.  The committed state still
   shows both as being synchronized, but the tile information for
   connector-B has now vanished.

 - Userspace reacts to the hotplug event and performs a modeset on
   connector-B.
     * the initial atomic state contains only crtc-B / connector-B since
       userspace didn't see a need to update A.
     * intel_atomic_check_tiled_conns loops over connectors that are
       already in the state, so it only processes connector-B.  It either
       sees !connector->has_tile and never calls intel_modeset_all_tiles
       _or_ it sees a completely different tile group ID that won't
       match against connector-A.

So just looking at tiles isn't enough to bring in the missing partner in
cases where the tile information has vanished before our commit begins;
i.e., that's why we need to also look at the current port sync status to
find old partners that we're still sync'd against and bring them into
the transaction as well.

> 
> Now this modeset_synced_crtcs() is called after new master slave
> assignments are done and the only purpose of this call is to make sure
> that we dont override the full modeset with fastset and that if one
> either master or slave needs full modeset other one also shd be forced
> modeset.

Ah, so if modeset_synced_crtcs() is only intended to prevent problematic
demotions to fastset, then it seems like the loop just above your call
that Jose just added for the MST stuff is already intended for that
purpose.  Maybe we can build your intel_atomic_check_synced_crtcs() into
that existing loop rather than handling port sync as an independent
loop?  I'm fine with doing that as part of a follow-up patch though so
we don't delay getting these fixes landed.

But that means we still need some processing of synced CRTC's up at the
top of the function around the same place (and with the same general
approach) as where you handle the tiled connectors.  I.e., something
that will notice a pre-existing port sync on the old state, then loop
over all crtcs (not just the ones already in the state) to bring in the
old partner.

> 
> This patch as of now just fixes the current hotplug issues, more
> enhancements to cover other corner cases with get_Connector overriding
> the tile info etc will be taken care of later as per discussions with
> Ville

Okay, if this is a known shortcoming that's still planned for the
future, then I guess we can just add a FIXME comment or something for
now so it's clear why some cases still aren't expected to work properly.

>  
> > > 
> > >  
> > > > > +		if (transcoders & BIT(new_crtc_state->cpu_transcoder)) {
> > > > > +			new_crtc_state->uapi.mode_changed = true;
> > > > > +			new_crtc_state->update_pipe = false;
> > > > > +		}
> > > > > +	}
> > > > > +}
> > > > > +
> > > > > +static void
> > > > > +intel_atomic_check_synced_crtcs(struct intel_atomic_state *state)
> > > > > +{
> > > > > +	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> > > > > +	struct intel_crtc_state *new_crtc_state;
> > > > > +	struct intel_crtc *crtc;
> > > > > +	int i;
> > > > > +
> > > > > +	if (INTEL_GEN(dev_priv) < 11)
> > > > > +		return;
> > > > > +
> > > > > +	for_each_new_intel_crtc_in_state(state, crtc,
> > > > > +					 new_crtc_state, i) {
> > > > > +		if (is_trans_port_sync_master(new_crtc_state) &&
> > > > > +		    needs_modeset(new_crtc_state)) {
> > > > > +			intel_modeset_synced_crtcs(state,
> > > > > +						   new_crtc_state->sync_mode_slaves_mask);
> > > > > +		} else if (is_trans_port_sync_slave(new_crtc_state) &&
> > > > > +			   needs_modeset(new_crtc_state)) {
> > > > > +			intel_modeset_synced_crtcs(state,
> > > > > +						   BIT(new_crtc_state->master_transcoder));
> > > > > +		}
> > > > > +	}
> > > > > +}
> > > > > +
> > > > > +static int
> > > > > +intel_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_iter;
> > > > > +	int ret = 0;
> > > > > +
> > > > > +	drm_connector_list_iter_begin(&dev_priv->drm, &conn_iter);
> > > > > +	drm_for_each_connector_iter(connector, &conn_iter) {
> > > > > +		struct drm_connector_state *conn_state;
> > > > > +		struct drm_crtc_state *crtc_state;
> > > > > +
> > > > > +		if (!connector->has_tile ||
> > > > > +		    connector->tile_group->id != tile_grp_id)
> > > > > +			continue;
> > > > > +		conn_state = drm_atomic_get_connector_state(&state->base,
> > > > > +							    connector);
> > > > > +		if (IS_ERR(conn_state)) {
> > > > > +			ret =  PTR_ERR(conn_state);
> > > > > +			break;
> > > > > +		}
> > > > > +
> > > > > +		if (!conn_state->crtc)
> > > > > +			continue;
> > > > > +
> > > > > +		crtc_state = drm_atomic_get_crtc_state(&state->base,
> > > > > +						       conn_state->crtc);
> > > > > +		if (IS_ERR(crtc_state)) {
> > > > > +			ret = PTR_ERR(conn_state);
> > > > > +			break;
> > > > > +		}
> > > > > +		crtc_state->mode_changed = true;
> > > > > +		ret = drm_atomic_add_affected_connectors(&state->base,
> > > > > +							 conn_state->crtc);
> > > > > +		if (ret)
> > > > > +			break;
> > > > > +	}
> > > > > +	drm_connector_list_iter_end(&conn_iter);
> > > > > +
> > > > > +	return ret;
> > > > > +}
> > > > 
> > > > There isn't really anything i915-specific in this function and it feels
> > > > like something other drivers may need as well if they treat tiled
> > > > monitors in a similar manner.  We may want to consider pulling this out
> > > > to a DRM core helper, although we can do that in a future patch.
> > > > 
> > > > With a bit of extra work, the function below could also potentially be
> > > > moved to the core too.  Do you know if there's other hardware with port
> > > > sync capabilities that could benefit from these?
> > > >
> > > 
> > > Yes definitely can eventually make this a drm helper even though the whole master slave
> > > thing is i915 specific for now
> > >  
> > > > > +
> > > > > +static int
> > > > > +intel_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, connector))
> > > > > +			continue;
> > > > > +
> > > > > +		ret = intel_modeset_all_tiles(state, connector->tile_group->id);
> > > > > +		if (ret)
> > > > > +			return ret;
> > > > > +	}
> > > > > +
> > > > > +	return 0;
> > > > > +}
> > > > > +
> > > > >  /**
> > > > >   * intel_atomic_check - validate state object
> > > > >   * @dev: drm device
> > > > > @@ -14331,6 +14443,10 @@ static int intel_atomic_check(struct drm_device *dev,
> > > > >  	if (ret)
> > > > >  		goto fail;
> > > > >  
> > > > > +	ret = intel_atomic_check_tiled_conns(state);
> > > > > +	if (ret)
> > > > > +		goto fail;
> > > > > +
> > > > >  	for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
> > > > >  					    new_crtc_state, i) {
> > > > >  		if (!needs_modeset(new_crtc_state)) {
> > > > > @@ -14378,6 +14494,13 @@ static int intel_atomic_check(struct drm_device *dev,
> > > > >  		}
> > > > >  	}
> > > > >  
> > > > > +	/*
> > > > > +	 * 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.
> > > > 
> > > > Since it's somewhat non-intuitive, I'd add a little bit of extra
> > > > explanation for why intel_atomic_check_tiled_conns above didn't already
> > > > take care of this.  I.e., if you've plugged in a different monitor, the
> > > > tile information may have already vanished from drm_connector by the
> > > > time we start this atomic transaction, so we still need to deal with
> > > > connectors that used to be tiled (and thus were port-synced) but no
> > > > longer are.
> > > >
> > > 
> > > Yes will add this in the comments
> > >  
> > > > 
> > > > > +	 */
> > > > > +	intel_atomic_check_synced_crtcs(state);
> > > > 
> > > > Although I can't think of a reason why it would cause a problem in this
> > > > case, we do seem to be violating the directions in the big kerneldoc
> > > > warning attached to drm_atomic_helper_check_modeset().  I.e. if we set
> > > > mode_changed in our own check functions, then we're supposed to re-call
> > > > drm_atomic_helper_check_modeset() to make sure everything is properly
> > > > handled.  If we're not going to follow those directions, we should
> > > > probably be clear why we don't think it's necessary.
> > > > 
> > > > Actually, I wonder if some of this tiling handling should eventually
> > > > migrate into that core helper function in the future...
> > > >
> > > 
> > > We are setting the mode hanged to true directly in our function so yes it makes
> > > sense to eventually do that as part of the core helper function but i still
> > > dont see the need for us to call drm_atomic_helper_check_modeset() again
> > > since our function is essentially checking modeset based on the tiled conditions
> > 
> > I'd still like to see either a commit message note or a code comment
> > justifying this.  The instructions say we need to re-invoke that
> > function in cases like what we're doing here, so if we're intentionally
> > not following those directions we should explain why for future
> > reference.  It's also possible that function may expand its role in the
> > future and start doing something new that we really do need to worry
> > about, so it would be good to have the history and justification for not
> > calling it documented.
> >
> 
> Yes adding a bigger comment explaining this is a good idea.
> 
> With these changes do you think this is a r-b from you?

Yeah, if we add a FIXME for the cases that we're still missing (i.e.,
when tile information is gone and we never pull in the old sync
partner), you can consider this

Reviewed-by: Matt Roper <matthew.d.roper@intel.com>

since even if it's not 100% complete it fixes some cases and shouldn't
cause new regressions, so it makes sense to land it early and then
follow up with the rest of the fixes when they're ready.


Matt

> 
> Manasi 
> > 
> > Matt
> > 
> > > 
> > > Manasi
> > >  
> > > > 
> > > > Matt
> > > > 
> > > > > +
> > > > >  	for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
> > > > >  					    new_crtc_state, i) {
> > > > >  		if (needs_modeset(new_crtc_state)) {
> > > > > -- 
> > > > > 2.19.1
> > > > > 
> > > > 
> > > > -- 
> > > > Matt Roper
> > > > Graphics Software Engineer
> > > > VTT-OSGC Platform Enablement
> > > > Intel Corporation
> > > > (916) 356-2795
> > 
> > -- 
> > Matt Roper
> > Graphics Software Engineer
> > VTT-OSGC Platform Enablement
> > Intel Corporation
> > (916) 356-2795

-- 
Matt Roper
Graphics Software Engineer
VTT-OSGC Platform Enablement
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

* Re: [Intel-gfx] [PATCH v5 1/3] drm/i915/dp: Make sure all tiled connectors get added to the state with full modeset
  2019-12-27 23:17         ` Matt Roper
@ 2019-12-27 23:49           ` Manasi Navare
  2019-12-28  0:23             ` Matt Roper
  0 siblings, 1 reply; 13+ messages in thread
From: Manasi Navare @ 2019-12-27 23:49 UTC (permalink / raw)
  To: Matt Roper; +Cc: intel-gfx

On Fri, Dec 27, 2019 at 03:17:07PM -0800, Matt Roper wrote:
> On Fri, Dec 27, 2019 at 02:51:28PM -0800, Manasi Navare wrote:
> > On Fri, Dec 27, 2019 at 12:03:33PM -0800, Matt Roper wrote:
> > > On Fri, Dec 27, 2019 at 11:27:50AM -0800, Manasi Navare wrote:
> > > > On Fri, Dec 27, 2019 at 10:36:52AM -0800, Matt Roper wrote:
> > > > > On Mon, Dec 23, 2019 at 02:44:27PM -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.
> > > > > > 
> > > > > > We also need to force modeset for all synced crtcs after fastset check.
> > > > > > 
> > > > > > v5:
> > > > > > * Rebase
> > > > > 
> > > > > I sent a reply to your v4 of this right at the same time you sent out
> > > > > v5, but I'm not sure if my reply went through since it doesn't show up
> > > > > in patchwork.  I've included the same feedback I gave on v4 below in
> > > > > case it got lost, plus a few more comments.
> > > > > 
> > > > > > v4:
> > > > > > * Fix logic for modeset_synced_crtcs (Ville)
> > > > > > v3:
> > > > > > * Add tile checks only for Gen >11
> > > > > > 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 | 123 +++++++++++++++++++
> > > > > >  1 file changed, 123 insertions(+)
> > > > > > 
> > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > > > > > index 94fc4b5bacc0..45a699bac34a 100644
> > > > > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > > > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > > > > @@ -14304,6 +14304,118 @@ static bool intel_cpu_transcoder_needs_modeset(struct intel_atomic_state *state,
> > > > > >  	return false;
> > > > > >  }
> > > > > >  
> > > > > > +static void
> > > > > > +intel_modeset_synced_crtcs(struct intel_atomic_state *state,
> > > > > > +			   u8 transcoders)
> > > > > > +{
> > > > > > +	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) {
> > > > > 
> > > > > Are we guaranteed to have the other CRTC in the state at this point?  If
> > > > > the tile information was gone at the beginning of the transaction, then
> > > > > it wouldn't have known to bring in the other CRTC.  So if a modeset was
> > > > > only submitted on one of the two synchronized CRTCs then I don't see
> > > > > where the other one would be added to the transaction before this point?
> > > > >
> > > > 
> > > > Yes basically at this point the new crtc state is not cleared so we use the 
> > > > previously synced crtcs through new crtc state master slave assignments from previous
> > > > modeset.
> > > 
> > > I'm not sure we're talking about the same thing here.  Initially an
> > > atomic state (transaction) is empty, and all mode objects have a
> > > currently-committed state, but no new state associated with this
> > > specific transaction.  If userspace requests a modeset on CRTC-A (e.g.,
> > > master), that will add CRTC-A to the drm_atomic_state, but won't
> > > automatically add CRTC-B (slave).  CRTC B's committed state does indeed
> > > indicate that it's part of a port-sync pair, but it won't get iterated
> > > over in the loop above because it still hasn't been added with an
> > > explicit drm_atomic_get_crtc_state().  So we'll never set mode_changed
> > > on CRTC-B as far as I can see because nothing ever brought it into the
> > > transaction (the only place where we bring in new CRTCs in this patch is
> > > when handling the tiles).
> > >
> > 
> > Ah okay I understand your concern now. So if you see the first call
> > intel_atomic_check_tiled_conns(), that one loops through all the
> > connectors not ust the ones in the state and adds all connectors with
> > tiles to the modeset. If we unplug a conn an connect a new monitor
> > that is untiled, the conn state goes from conn to unconn and that
> > should triggera full modeset even though the tile info does not exist.
> 
> Right, intel_atomic_check_tiled_conns loops through all connectors, so
> it can pick stuff up that isn't already part of our atomic transaction.
> However it won't know it needs to pick up the partner connector in cases
> where we've already lost the tile information for the connector that was
> re-plugged.
> 
> For example, let's say we have connector-A/crtc-A (tile grp 1) and
> connector-B/crtc-B (tile grp 1).  The current state for both indicates
> they're synced and the tile information for both is still present.
> 
>  - Now we unplug connector-B and plug in a new monitor that's either
>    untiled or part of a different tile group.  The committed state still
>    shows both as being synchronized, but the tile information for
>    connector-B has now vanished.
> 
>  - Userspace reacts to the hotplug event and performs a modeset on
>    connector-B.
>      * the initial atomic state contains only crtc-B / connector-B since
>        userspace didn't see a need to update A.
>      * intel_atomic_check_tiled_conns loops over connectors that are
>        already in the state, so it only processes connector-B.  It either
>        sees !connector->has_tile and never calls intel_modeset_all_tiles
>        _or_ it sees a completely different tile group ID that won't
>        match against connector-A.
> 
> So just looking at tiles isn't enough to bring in the missing partner in
> cases where the tile information has vanished before our commit begins;
> i.e., that's why we need to also look at the current port sync status to
> find old partners that we're still sync'd against and bring them into
> the transaction as well.
>

Yes this above example makes complete sense. I think now the DRM patches make sure that
if we lose one of the tiles through hotplug it also fallsback the mode on remaining conn
so conn A here to a lower non tiled mode so I see that it also gets added to the state
with mode changed and the other disable due to conn B unplug goes through disable sequence
and disables all associated master/slaves as well.

But from what you are saying is it would be safe to check if any of the conns in state that needs a modeset is involved in port sync
and then loop through all drm connectors and  force modeset on all other conns in the same port sync?
So kind of like the intel_modeset_synced_crtcs() right after modeset_all_tiles() ?

But may be for now, add a FIXME for this case and land this patch to make sure that it fixes
the current hotplug/unplug issues?

Regards
Manasi

> > 
> > Now this modeset_synced_crtcs() is called after new master slave
> > assignments are done and the only purpose of this call is to make sure
> > that we dont override the full modeset with fastset and that if one
> > either master or slave needs full modeset other one also shd be forced
> > modeset.
> 
> Ah, so if modeset_synced_crtcs() is only intended to prevent problematic
> demotions to fastset, then it seems like the loop just above your call
> that Jose just added for the MST stuff is already intended for that
> purpose.  Maybe we can build your intel_atomic_check_synced_crtcs() into
> that existing loop rather than handling port sync as an independent
> loop?  I'm fine with doing that as part of a follow-up patch though so
> we don't delay getting these fixes landed.
> 
> But that means we still need some processing of synced CRTC's up at the
> top of the function around the same place (and with the same general
> approach) as where you handle the tiled connectors.  I.e., something
> that will notice a pre-existing port sync on the old state, then loop
> over all crtcs (not just the ones already in the state) to bring in the
> old partner.
> 
> > 
> > This patch as of now just fixes the current hotplug issues, more
> > enhancements to cover other corner cases with get_Connector overriding
> > the tile info etc will be taken care of later as per discussions with
> > Ville
> 
> Okay, if this is a known shortcoming that's still planned for the
> future, then I guess we can just add a FIXME comment or something for
> now so it's clear why some cases still aren't expected to work properly.
> 
> >  
> > > > 
> > > >  
> > > > > > +		if (transcoders & BIT(new_crtc_state->cpu_transcoder)) {
> > > > > > +			new_crtc_state->uapi.mode_changed = true;
> > > > > > +			new_crtc_state->update_pipe = false;
> > > > > > +		}
> > > > > > +	}
> > > > > > +}
> > > > > > +
> > > > > > +static void
> > > > > > +intel_atomic_check_synced_crtcs(struct intel_atomic_state *state)
> > > > > > +{
> > > > > > +	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> > > > > > +	struct intel_crtc_state *new_crtc_state;
> > > > > > +	struct intel_crtc *crtc;
> > > > > > +	int i;
> > > > > > +
> > > > > > +	if (INTEL_GEN(dev_priv) < 11)
> > > > > > +		return;
> > > > > > +
> > > > > > +	for_each_new_intel_crtc_in_state(state, crtc,
> > > > > > +					 new_crtc_state, i) {
> > > > > > +		if (is_trans_port_sync_master(new_crtc_state) &&
> > > > > > +		    needs_modeset(new_crtc_state)) {
> > > > > > +			intel_modeset_synced_crtcs(state,
> > > > > > +						   new_crtc_state->sync_mode_slaves_mask);
> > > > > > +		} else if (is_trans_port_sync_slave(new_crtc_state) &&
> > > > > > +			   needs_modeset(new_crtc_state)) {
> > > > > > +			intel_modeset_synced_crtcs(state,
> > > > > > +						   BIT(new_crtc_state->master_transcoder));
> > > > > > +		}
> > > > > > +	}
> > > > > > +}
> > > > > > +
> > > > > > +static int
> > > > > > +intel_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_iter;
> > > > > > +	int ret = 0;
> > > > > > +
> > > > > > +	drm_connector_list_iter_begin(&dev_priv->drm, &conn_iter);
> > > > > > +	drm_for_each_connector_iter(connector, &conn_iter) {
> > > > > > +		struct drm_connector_state *conn_state;
> > > > > > +		struct drm_crtc_state *crtc_state;
> > > > > > +
> > > > > > +		if (!connector->has_tile ||
> > > > > > +		    connector->tile_group->id != tile_grp_id)
> > > > > > +			continue;
> > > > > > +		conn_state = drm_atomic_get_connector_state(&state->base,
> > > > > > +							    connector);
> > > > > > +		if (IS_ERR(conn_state)) {
> > > > > > +			ret =  PTR_ERR(conn_state);
> > > > > > +			break;
> > > > > > +		}
> > > > > > +
> > > > > > +		if (!conn_state->crtc)
> > > > > > +			continue;
> > > > > > +
> > > > > > +		crtc_state = drm_atomic_get_crtc_state(&state->base,
> > > > > > +						       conn_state->crtc);
> > > > > > +		if (IS_ERR(crtc_state)) {
> > > > > > +			ret = PTR_ERR(conn_state);
> > > > > > +			break;
> > > > > > +		}
> > > > > > +		crtc_state->mode_changed = true;
> > > > > > +		ret = drm_atomic_add_affected_connectors(&state->base,
> > > > > > +							 conn_state->crtc);
> > > > > > +		if (ret)
> > > > > > +			break;
> > > > > > +	}
> > > > > > +	drm_connector_list_iter_end(&conn_iter);
> > > > > > +
> > > > > > +	return ret;
> > > > > > +}
> > > > > 
> > > > > There isn't really anything i915-specific in this function and it feels
> > > > > like something other drivers may need as well if they treat tiled
> > > > > monitors in a similar manner.  We may want to consider pulling this out
> > > > > to a DRM core helper, although we can do that in a future patch.
> > > > > 
> > > > > With a bit of extra work, the function below could also potentially be
> > > > > moved to the core too.  Do you know if there's other hardware with port
> > > > > sync capabilities that could benefit from these?
> > > > >
> > > > 
> > > > Yes definitely can eventually make this a drm helper even though the whole master slave
> > > > thing is i915 specific for now
> > > >  
> > > > > > +
> > > > > > +static int
> > > > > > +intel_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, connector))
> > > > > > +			continue;
> > > > > > +
> > > > > > +		ret = intel_modeset_all_tiles(state, connector->tile_group->id);
> > > > > > +		if (ret)
> > > > > > +			return ret;
> > > > > > +	}
> > > > > > +
> > > > > > +	return 0;
> > > > > > +}
> > > > > > +
> > > > > >  /**
> > > > > >   * intel_atomic_check - validate state object
> > > > > >   * @dev: drm device
> > > > > > @@ -14331,6 +14443,10 @@ static int intel_atomic_check(struct drm_device *dev,
> > > > > >  	if (ret)
> > > > > >  		goto fail;
> > > > > >  
> > > > > > +	ret = intel_atomic_check_tiled_conns(state);
> > > > > > +	if (ret)
> > > > > > +		goto fail;
> > > > > > +
> > > > > >  	for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
> > > > > >  					    new_crtc_state, i) {
> > > > > >  		if (!needs_modeset(new_crtc_state)) {
> > > > > > @@ -14378,6 +14494,13 @@ static int intel_atomic_check(struct drm_device *dev,
> > > > > >  		}
> > > > > >  	}
> > > > > >  
> > > > > > +	/*
> > > > > > +	 * 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.
> > > > > 
> > > > > Since it's somewhat non-intuitive, I'd add a little bit of extra
> > > > > explanation for why intel_atomic_check_tiled_conns above didn't already
> > > > > take care of this.  I.e., if you've plugged in a different monitor, the
> > > > > tile information may have already vanished from drm_connector by the
> > > > > time we start this atomic transaction, so we still need to deal with
> > > > > connectors that used to be tiled (and thus were port-synced) but no
> > > > > longer are.
> > > > >
> > > > 
> > > > Yes will add this in the comments
> > > >  
> > > > > 
> > > > > > +	 */
> > > > > > +	intel_atomic_check_synced_crtcs(state);
> > > > > 
> > > > > Although I can't think of a reason why it would cause a problem in this
> > > > > case, we do seem to be violating the directions in the big kerneldoc
> > > > > warning attached to drm_atomic_helper_check_modeset().  I.e. if we set
> > > > > mode_changed in our own check functions, then we're supposed to re-call
> > > > > drm_atomic_helper_check_modeset() to make sure everything is properly
> > > > > handled.  If we're not going to follow those directions, we should
> > > > > probably be clear why we don't think it's necessary.
> > > > > 
> > > > > Actually, I wonder if some of this tiling handling should eventually
> > > > > migrate into that core helper function in the future...
> > > > >
> > > > 
> > > > We are setting the mode hanged to true directly in our function so yes it makes
> > > > sense to eventually do that as part of the core helper function but i still
> > > > dont see the need for us to call drm_atomic_helper_check_modeset() again
> > > > since our function is essentially checking modeset based on the tiled conditions
> > > 
> > > I'd still like to see either a commit message note or a code comment
> > > justifying this.  The instructions say we need to re-invoke that
> > > function in cases like what we're doing here, so if we're intentionally
> > > not following those directions we should explain why for future
> > > reference.  It's also possible that function may expand its role in the
> > > future and start doing something new that we really do need to worry
> > > about, so it would be good to have the history and justification for not
> > > calling it documented.
> > >
> > 
> > Yes adding a bigger comment explaining this is a good idea.
> > 
> > With these changes do you think this is a r-b from you?
> 
> Yeah, if we add a FIXME for the cases that we're still missing (i.e.,
> when tile information is gone and we never pull in the old sync
> partner), you can consider this
> 
> Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
> 
> since even if it's not 100% complete it fixes some cases and shouldn't
> cause new regressions, so it makes sense to land it early and then
> follow up with the rest of the fixes when they're ready.
> 
> 
> Matt
> 
> > 
> > Manasi 
> > > 
> > > Matt
> > > 
> > > > 
> > > > Manasi
> > > >  
> > > > > 
> > > > > Matt
> > > > > 
> > > > > > +
> > > > > >  	for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
> > > > > >  					    new_crtc_state, i) {
> > > > > >  		if (needs_modeset(new_crtc_state)) {
> > > > > > -- 
> > > > > > 2.19.1
> > > > > > 
> > > > > 
> > > > > -- 
> > > > > Matt Roper
> > > > > Graphics Software Engineer
> > > > > VTT-OSGC Platform Enablement
> > > > > Intel Corporation
> > > > > (916) 356-2795
> > > 
> > > -- 
> > > Matt Roper
> > > Graphics Software Engineer
> > > VTT-OSGC Platform Enablement
> > > Intel Corporation
> > > (916) 356-2795
> 
> -- 
> Matt Roper
> Graphics Software Engineer
> VTT-OSGC Platform Enablement
> Intel Corporation
> (916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

* Re: [Intel-gfx] [PATCH v5 1/3] drm/i915/dp: Make sure all tiled connectors get added to the state with full modeset
  2019-12-27 23:49           ` Manasi Navare
@ 2019-12-28  0:23             ` Matt Roper
  0 siblings, 0 replies; 13+ messages in thread
From: Matt Roper @ 2019-12-28  0:23 UTC (permalink / raw)
  To: Manasi Navare; +Cc: intel-gfx

On Fri, Dec 27, 2019 at 03:49:15PM -0800, Manasi Navare wrote:
> On Fri, Dec 27, 2019 at 03:17:07PM -0800, Matt Roper wrote:
> > On Fri, Dec 27, 2019 at 02:51:28PM -0800, Manasi Navare wrote:
> > > On Fri, Dec 27, 2019 at 12:03:33PM -0800, Matt Roper wrote:
> > > > On Fri, Dec 27, 2019 at 11:27:50AM -0800, Manasi Navare wrote:
> > > > > On Fri, Dec 27, 2019 at 10:36:52AM -0800, Matt Roper wrote:
> > > > > > On Mon, Dec 23, 2019 at 02:44:27PM -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.
> > > > > > > 
> > > > > > > We also need to force modeset for all synced crtcs after fastset check.
> > > > > > > 
> > > > > > > v5:
> > > > > > > * Rebase
> > > > > > 
> > > > > > I sent a reply to your v4 of this right at the same time you sent out
> > > > > > v5, but I'm not sure if my reply went through since it doesn't show up
> > > > > > in patchwork.  I've included the same feedback I gave on v4 below in
> > > > > > case it got lost, plus a few more comments.
> > > > > > 
> > > > > > > v4:
> > > > > > > * Fix logic for modeset_synced_crtcs (Ville)
> > > > > > > v3:
> > > > > > > * Add tile checks only for Gen >11
> > > > > > > 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 | 123 +++++++++++++++++++
> > > > > > >  1 file changed, 123 insertions(+)
> > > > > > > 
> > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > > > > > > index 94fc4b5bacc0..45a699bac34a 100644
> > > > > > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > > > > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > > > > > @@ -14304,6 +14304,118 @@ static bool intel_cpu_transcoder_needs_modeset(struct intel_atomic_state *state,
> > > > > > >  	return false;
> > > > > > >  }
> > > > > > >  
> > > > > > > +static void
> > > > > > > +intel_modeset_synced_crtcs(struct intel_atomic_state *state,
> > > > > > > +			   u8 transcoders)
> > > > > > > +{
> > > > > > > +	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) {
> > > > > > 
> > > > > > Are we guaranteed to have the other CRTC in the state at this point?  If
> > > > > > the tile information was gone at the beginning of the transaction, then
> > > > > > it wouldn't have known to bring in the other CRTC.  So if a modeset was
> > > > > > only submitted on one of the two synchronized CRTCs then I don't see
> > > > > > where the other one would be added to the transaction before this point?
> > > > > >
> > > > > 
> > > > > Yes basically at this point the new crtc state is not cleared so we use the 
> > > > > previously synced crtcs through new crtc state master slave assignments from previous
> > > > > modeset.
> > > > 
> > > > I'm not sure we're talking about the same thing here.  Initially an
> > > > atomic state (transaction) is empty, and all mode objects have a
> > > > currently-committed state, but no new state associated with this
> > > > specific transaction.  If userspace requests a modeset on CRTC-A (e.g.,
> > > > master), that will add CRTC-A to the drm_atomic_state, but won't
> > > > automatically add CRTC-B (slave).  CRTC B's committed state does indeed
> > > > indicate that it's part of a port-sync pair, but it won't get iterated
> > > > over in the loop above because it still hasn't been added with an
> > > > explicit drm_atomic_get_crtc_state().  So we'll never set mode_changed
> > > > on CRTC-B as far as I can see because nothing ever brought it into the
> > > > transaction (the only place where we bring in new CRTCs in this patch is
> > > > when handling the tiles).
> > > >
> > > 
> > > Ah okay I understand your concern now. So if you see the first call
> > > intel_atomic_check_tiled_conns(), that one loops through all the
> > > connectors not ust the ones in the state and adds all connectors with
> > > tiles to the modeset. If we unplug a conn an connect a new monitor
> > > that is untiled, the conn state goes from conn to unconn and that
> > > should triggera full modeset even though the tile info does not exist.
> > 
> > Right, intel_atomic_check_tiled_conns loops through all connectors, so
> > it can pick stuff up that isn't already part of our atomic transaction.
> > However it won't know it needs to pick up the partner connector in cases
> > where we've already lost the tile information for the connector that was
> > re-plugged.
> > 
> > For example, let's say we have connector-A/crtc-A (tile grp 1) and
> > connector-B/crtc-B (tile grp 1).  The current state for both indicates
> > they're synced and the tile information for both is still present.
> > 
> >  - Now we unplug connector-B and plug in a new monitor that's either
> >    untiled or part of a different tile group.  The committed state still
> >    shows both as being synchronized, but the tile information for
> >    connector-B has now vanished.
> > 
> >  - Userspace reacts to the hotplug event and performs a modeset on
> >    connector-B.
> >      * the initial atomic state contains only crtc-B / connector-B since
> >        userspace didn't see a need to update A.
> >      * intel_atomic_check_tiled_conns loops over connectors that are
> >        already in the state, so it only processes connector-B.  It either
> >        sees !connector->has_tile and never calls intel_modeset_all_tiles
> >        _or_ it sees a completely different tile group ID that won't
> >        match against connector-A.
> > 
> > So just looking at tiles isn't enough to bring in the missing partner in
> > cases where the tile information has vanished before our commit begins;
> > i.e., that's why we need to also look at the current port sync status to
> > find old partners that we're still sync'd against and bring them into
> > the transaction as well.
> >
> 
> Yes this above example makes complete sense. I think now the DRM patches make sure that
> if we lose one of the tiles through hotplug it also fallsback the mode on remaining conn
> so conn A here to a lower non tiled mode so I see that it also gets added to the state
> with mode changed and the other disable due to conn B unplug goes through disable sequence
> and disables all associated master/slaves as well.

I'm not sure what the drm fbcon hotplug policy is without looking, but
that's only only one potential client sending us atomic transactions in
reaction to hotplugs.  If you have a userspace DRM master running, then
nothing will happen automatically in the kernel/drm except what
userspace tells us to do.  Some compositors might recognize that you did
something to one tile and add both tiles to the next transaction they
submit, but other compositors may not be so smart.

> 
> But from what you are saying is it would be safe to check if any of the conns in state that needs a modeset is involved in port sync
> and then loop through all drm connectors and  force modeset on all other conns in the same port sync?
> So kind of like the intel_modeset_synced_crtcs() right after modeset_all_tiles() ?
> 
> But may be for now, add a FIXME for this case and land this patch to make sure that it fixes
> the current hotplug/unplug issues?

Yeah, it sounds like what you have here does fix the most immediate
bugs, even if there are some corner cases that still aren't covered.  So
I'm okay landing what we have with a FIXME so that we unblock some usage
while we keep working on the fixes for other corner cases.


Matt

> 
> Regards
> Manasi
> 
> > > 
> > > Now this modeset_synced_crtcs() is called after new master slave
> > > assignments are done and the only purpose of this call is to make sure
> > > that we dont override the full modeset with fastset and that if one
> > > either master or slave needs full modeset other one also shd be forced
> > > modeset.
> > 
> > Ah, so if modeset_synced_crtcs() is only intended to prevent problematic
> > demotions to fastset, then it seems like the loop just above your call
> > that Jose just added for the MST stuff is already intended for that
> > purpose.  Maybe we can build your intel_atomic_check_synced_crtcs() into
> > that existing loop rather than handling port sync as an independent
> > loop?  I'm fine with doing that as part of a follow-up patch though so
> > we don't delay getting these fixes landed.
> > 
> > But that means we still need some processing of synced CRTC's up at the
> > top of the function around the same place (and with the same general
> > approach) as where you handle the tiled connectors.  I.e., something
> > that will notice a pre-existing port sync on the old state, then loop
> > over all crtcs (not just the ones already in the state) to bring in the
> > old partner.
> > 
> > > 
> > > This patch as of now just fixes the current hotplug issues, more
> > > enhancements to cover other corner cases with get_Connector overriding
> > > the tile info etc will be taken care of later as per discussions with
> > > Ville
> > 
> > Okay, if this is a known shortcoming that's still planned for the
> > future, then I guess we can just add a FIXME comment or something for
> > now so it's clear why some cases still aren't expected to work properly.
> > 
> > >  
> > > > > 
> > > > >  
> > > > > > > +		if (transcoders & BIT(new_crtc_state->cpu_transcoder)) {
> > > > > > > +			new_crtc_state->uapi.mode_changed = true;
> > > > > > > +			new_crtc_state->update_pipe = false;
> > > > > > > +		}
> > > > > > > +	}
> > > > > > > +}
> > > > > > > +
> > > > > > > +static void
> > > > > > > +intel_atomic_check_synced_crtcs(struct intel_atomic_state *state)
> > > > > > > +{
> > > > > > > +	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> > > > > > > +	struct intel_crtc_state *new_crtc_state;
> > > > > > > +	struct intel_crtc *crtc;
> > > > > > > +	int i;
> > > > > > > +
> > > > > > > +	if (INTEL_GEN(dev_priv) < 11)
> > > > > > > +		return;
> > > > > > > +
> > > > > > > +	for_each_new_intel_crtc_in_state(state, crtc,
> > > > > > > +					 new_crtc_state, i) {
> > > > > > > +		if (is_trans_port_sync_master(new_crtc_state) &&
> > > > > > > +		    needs_modeset(new_crtc_state)) {
> > > > > > > +			intel_modeset_synced_crtcs(state,
> > > > > > > +						   new_crtc_state->sync_mode_slaves_mask);
> > > > > > > +		} else if (is_trans_port_sync_slave(new_crtc_state) &&
> > > > > > > +			   needs_modeset(new_crtc_state)) {
> > > > > > > +			intel_modeset_synced_crtcs(state,
> > > > > > > +						   BIT(new_crtc_state->master_transcoder));
> > > > > > > +		}
> > > > > > > +	}
> > > > > > > +}
> > > > > > > +
> > > > > > > +static int
> > > > > > > +intel_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_iter;
> > > > > > > +	int ret = 0;
> > > > > > > +
> > > > > > > +	drm_connector_list_iter_begin(&dev_priv->drm, &conn_iter);
> > > > > > > +	drm_for_each_connector_iter(connector, &conn_iter) {
> > > > > > > +		struct drm_connector_state *conn_state;
> > > > > > > +		struct drm_crtc_state *crtc_state;
> > > > > > > +
> > > > > > > +		if (!connector->has_tile ||
> > > > > > > +		    connector->tile_group->id != tile_grp_id)
> > > > > > > +			continue;
> > > > > > > +		conn_state = drm_atomic_get_connector_state(&state->base,
> > > > > > > +							    connector);
> > > > > > > +		if (IS_ERR(conn_state)) {
> > > > > > > +			ret =  PTR_ERR(conn_state);
> > > > > > > +			break;
> > > > > > > +		}
> > > > > > > +
> > > > > > > +		if (!conn_state->crtc)
> > > > > > > +			continue;
> > > > > > > +
> > > > > > > +		crtc_state = drm_atomic_get_crtc_state(&state->base,
> > > > > > > +						       conn_state->crtc);
> > > > > > > +		if (IS_ERR(crtc_state)) {
> > > > > > > +			ret = PTR_ERR(conn_state);
> > > > > > > +			break;
> > > > > > > +		}
> > > > > > > +		crtc_state->mode_changed = true;
> > > > > > > +		ret = drm_atomic_add_affected_connectors(&state->base,
> > > > > > > +							 conn_state->crtc);
> > > > > > > +		if (ret)
> > > > > > > +			break;
> > > > > > > +	}
> > > > > > > +	drm_connector_list_iter_end(&conn_iter);
> > > > > > > +
> > > > > > > +	return ret;
> > > > > > > +}
> > > > > > 
> > > > > > There isn't really anything i915-specific in this function and it feels
> > > > > > like something other drivers may need as well if they treat tiled
> > > > > > monitors in a similar manner.  We may want to consider pulling this out
> > > > > > to a DRM core helper, although we can do that in a future patch.
> > > > > > 
> > > > > > With a bit of extra work, the function below could also potentially be
> > > > > > moved to the core too.  Do you know if there's other hardware with port
> > > > > > sync capabilities that could benefit from these?
> > > > > >
> > > > > 
> > > > > Yes definitely can eventually make this a drm helper even though the whole master slave
> > > > > thing is i915 specific for now
> > > > >  
> > > > > > > +
> > > > > > > +static int
> > > > > > > +intel_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, connector))
> > > > > > > +			continue;
> > > > > > > +
> > > > > > > +		ret = intel_modeset_all_tiles(state, connector->tile_group->id);
> > > > > > > +		if (ret)
> > > > > > > +			return ret;
> > > > > > > +	}
> > > > > > > +
> > > > > > > +	return 0;
> > > > > > > +}
> > > > > > > +
> > > > > > >  /**
> > > > > > >   * intel_atomic_check - validate state object
> > > > > > >   * @dev: drm device
> > > > > > > @@ -14331,6 +14443,10 @@ static int intel_atomic_check(struct drm_device *dev,
> > > > > > >  	if (ret)
> > > > > > >  		goto fail;
> > > > > > >  
> > > > > > > +	ret = intel_atomic_check_tiled_conns(state);
> > > > > > > +	if (ret)
> > > > > > > +		goto fail;
> > > > > > > +
> > > > > > >  	for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
> > > > > > >  					    new_crtc_state, i) {
> > > > > > >  		if (!needs_modeset(new_crtc_state)) {
> > > > > > > @@ -14378,6 +14494,13 @@ static int intel_atomic_check(struct drm_device *dev,
> > > > > > >  		}
> > > > > > >  	}
> > > > > > >  
> > > > > > > +	/*
> > > > > > > +	 * 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.
> > > > > > 
> > > > > > Since it's somewhat non-intuitive, I'd add a little bit of extra
> > > > > > explanation for why intel_atomic_check_tiled_conns above didn't already
> > > > > > take care of this.  I.e., if you've plugged in a different monitor, the
> > > > > > tile information may have already vanished from drm_connector by the
> > > > > > time we start this atomic transaction, so we still need to deal with
> > > > > > connectors that used to be tiled (and thus were port-synced) but no
> > > > > > longer are.
> > > > > >
> > > > > 
> > > > > Yes will add this in the comments
> > > > >  
> > > > > > 
> > > > > > > +	 */
> > > > > > > +	intel_atomic_check_synced_crtcs(state);
> > > > > > 
> > > > > > Although I can't think of a reason why it would cause a problem in this
> > > > > > case, we do seem to be violating the directions in the big kerneldoc
> > > > > > warning attached to drm_atomic_helper_check_modeset().  I.e. if we set
> > > > > > mode_changed in our own check functions, then we're supposed to re-call
> > > > > > drm_atomic_helper_check_modeset() to make sure everything is properly
> > > > > > handled.  If we're not going to follow those directions, we should
> > > > > > probably be clear why we don't think it's necessary.
> > > > > > 
> > > > > > Actually, I wonder if some of this tiling handling should eventually
> > > > > > migrate into that core helper function in the future...
> > > > > >
> > > > > 
> > > > > We are setting the mode hanged to true directly in our function so yes it makes
> > > > > sense to eventually do that as part of the core helper function but i still
> > > > > dont see the need for us to call drm_atomic_helper_check_modeset() again
> > > > > since our function is essentially checking modeset based on the tiled conditions
> > > > 
> > > > I'd still like to see either a commit message note or a code comment
> > > > justifying this.  The instructions say we need to re-invoke that
> > > > function in cases like what we're doing here, so if we're intentionally
> > > > not following those directions we should explain why for future
> > > > reference.  It's also possible that function may expand its role in the
> > > > future and start doing something new that we really do need to worry
> > > > about, so it would be good to have the history and justification for not
> > > > calling it documented.
> > > >
> > > 
> > > Yes adding a bigger comment explaining this is a good idea.
> > > 
> > > With these changes do you think this is a r-b from you?
> > 
> > Yeah, if we add a FIXME for the cases that we're still missing (i.e.,
> > when tile information is gone and we never pull in the old sync
> > partner), you can consider this
> > 
> > Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
> > 
> > since even if it's not 100% complete it fixes some cases and shouldn't
> > cause new regressions, so it makes sense to land it early and then
> > follow up with the rest of the fixes when they're ready.
> > 
> > 
> > Matt
> > 
> > > 
> > > Manasi 
> > > > 
> > > > Matt
> > > > 
> > > > > 
> > > > > Manasi
> > > > >  
> > > > > > 
> > > > > > Matt
> > > > > > 
> > > > > > > +
> > > > > > >  	for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
> > > > > > >  					    new_crtc_state, i) {
> > > > > > >  		if (needs_modeset(new_crtc_state)) {
> > > > > > > -- 
> > > > > > > 2.19.1
> > > > > > > 
> > > > > > 
> > > > > > -- 
> > > > > > Matt Roper
> > > > > > Graphics Software Engineer
> > > > > > VTT-OSGC Platform Enablement
> > > > > > Intel Corporation
> > > > > > (916) 356-2795
> > > > 
> > > > -- 
> > > > Matt Roper
> > > > Graphics Software Engineer
> > > > VTT-OSGC Platform Enablement
> > > > Intel Corporation
> > > > (916) 356-2795
> > 
> > -- 
> > Matt Roper
> > Graphics Software Engineer
> > VTT-OSGC Platform Enablement
> > Intel Corporation
> > (916) 356-2795

-- 
Matt Roper
Graphics Software Engineer
VTT-OSGC Platform Enablement
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

end of thread, other threads:[~2019-12-28  0:23 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-23 22:44 [Intel-gfx] [PATCH v5 1/3] drm/i915/dp: Make sure all tiled connectors get added to the state with full modeset Manasi Navare
2019-12-23 22:44 ` [Intel-gfx] [PATCH v5 2/3] drm/i915/dp: Make port sync mode assignments only if all tiles present Manasi Navare
2019-12-23 22:44 ` [Intel-gfx] [PATCH v5 3/3] drm/i915/dp: Disable Port sync mode correctly on teardown Manasi Navare
2019-12-24  0:35 ` [Intel-gfx] ✗ Fi.CI.BAT: failure for series starting with [v5,1/3] drm/i915/dp: Make sure all tiled connectors get added to the state with full modeset Patchwork
2019-12-24  1:24 ` [Intel-gfx] ✗ Fi.CI.BAT: failure for series starting with [v5,1/3] drm/i915/dp: Make sure all tiled connectors get added to the state with full modeset (rev2) Patchwork
2019-12-24  3:25 ` [Intel-gfx] ✗ Fi.CI.BAT: failure for series starting with [v5,1/3] drm/i915/dp: Make sure all tiled connectors get added to the state with full modeset (rev3) Patchwork
2019-12-27 18:36 ` [Intel-gfx] [PATCH v5 1/3] drm/i915/dp: Make sure all tiled connectors get added to the state with full modeset Matt Roper
2019-12-27 19:27   ` Manasi Navare
2019-12-27 20:03     ` Matt Roper
2019-12-27 22:51       ` Manasi Navare
2019-12-27 23:17         ` Matt Roper
2019-12-27 23:49           ` Manasi Navare
2019-12-28  0:23             ` Matt Roper

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.