[Intel-gfx] [PATCH 1/3] drm/i915: Introduce encoder->compute_config_late()

[Intel-gfx] [PATCH 1/3] drm/i915: Introduce encoder->compute_config_late()

[Manasi Navare]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Add an optional secondary encoder state compute hook. This gets
called after the normak .compute_config() has been called for
all the encoders in the state. Thus in the new hook we can rely
on all derived state populated by .compute_config() to be already
set up. Should be useful for MST and port sync master/slave
transcoder selection.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/display/intel_display.c  | 39 +++++++++++++++++++
 .../drm/i915/display/intel_display_types.h    |  3 ++
 2 files changed, 42 insertions(+)

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index c0e5002ce64c..de4ab51216f6 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -13416,6 +13416,35 @@ intel_modeset_pipe_config(struct intel_crtc_state *pipe_config)
 	return 0;
 }
 
+static int
+intel_modeset_pipe_config_late(struct intel_crtc_state *crtc_state)
+{
+	struct intel_atomic_state *state =
+		to_intel_atomic_state(crtc_state->uapi.state);
+	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
+	struct drm_connector_state *conn_state;
+	struct drm_connector *connector;
+	int i;
+
+	for_each_new_connector_in_state(&state->base, connector,
+					conn_state, i) {
+		struct intel_encoder *encoder =
+			to_intel_encoder(conn_state->best_encoder);
+		int ret;
+
+		if (conn_state->crtc != &crtc->base ||
+		    !encoder->compute_config_late)
+			continue;
+
+		ret = encoder->compute_config_late(encoder, crtc_state,
+						   conn_state);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
 bool intel_fuzzy_clock_check(int clock1, int clock2)
 {
 	int diff;
@@ -14817,6 +14846,16 @@ static int intel_atomic_check(struct drm_device *dev,
 		ret = intel_modeset_pipe_config(new_crtc_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))
+			continue;
+
+		ret = intel_modeset_pipe_config_late(new_crtc_state);
+		if (ret)
+			goto fail;
 
 		intel_crtc_check_fastset(old_crtc_state, new_crtc_state);
 	}
diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
index 33ba93863488..51f0108a6e09 100644
--- a/drivers/gpu/drm/i915/display/intel_display_types.h
+++ b/drivers/gpu/drm/i915/display/intel_display_types.h
@@ -140,6 +140,9 @@ struct intel_encoder {
 	int (*compute_config)(struct intel_encoder *,
 			      struct intel_crtc_state *,
 			      struct drm_connector_state *);
+	int (*compute_config_late)(struct intel_encoder *,
+				   struct intel_crtc_state *,
+				   struct drm_connector_state *);
 	void (*update_prepare)(struct intel_atomic_state *,
 			       struct intel_encoder *,
 			       struct intel_crtc *);
-- 
2.19.1

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

[Intel-gfx] [PATCH 2/3] drm/i915/dp: Compute port sync crtc states post compute_config()

[Manasi Navare]

This patch pushes out the computation of master and slave
transcoders in crtc states after encoder's compute_config hook.
This ensures that the assigned master slave crtcs have exact same
mode and timings which is a requirement for Port sync mode
to be enabled.

Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
---
 drivers/gpu/drm/i915/display/intel_ddi.c     | 125 +++++++++++++++
 drivers/gpu/drm/i915/display/intel_display.c | 156 -------------------
 2 files changed, 125 insertions(+), 156 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
index c96f629cddc3..4ec36eb5ce28 100644
--- a/drivers/gpu/drm/i915/display/intel_ddi.c
+++ b/drivers/gpu/drm/i915/display/intel_ddi.c
@@ -4440,6 +4440,130 @@ static int intel_ddi_compute_config(struct intel_encoder *encoder,
 	return 0;
 }
 
+static bool mode_equal(const struct drm_display_mode *mode1,
+		       const struct drm_display_mode *mode2)
+{
+	return drm_mode_match(mode1, mode2,
+			      DRM_MODE_MATCH_TIMINGS |
+			      DRM_MODE_MATCH_FLAGS |
+			      DRM_MODE_MATCH_3D_FLAGS) &&
+		mode1->clock == mode2->clock; /* we want an exact match */
+}
+
+static bool m_n_equal(const struct intel_link_m_n *m_n_1,
+		      const struct intel_link_m_n *m_n_2)
+{
+	return m_n_1->tu == m_n_2->tu &&
+		m_n_1->gmch_m == m_n_2->gmch_m &&
+		m_n_1->gmch_n == m_n_2->gmch_n &&
+		m_n_1->link_m == m_n_2->link_m &&
+		m_n_1->link_n == m_n_2->link_n;
+}
+
+static bool crtcs_port_sync_compatible(struct intel_crtc_state *crtc_state1,
+				       struct intel_crtc_state *crtc_state2)
+{
+	return crtc_state1->hw.active && crtc_state2->hw.active &&
+		crtc_state1->output_types == crtc_state2->output_types &&
+		crtc_state1->output_format == crtc_state2->output_format &&
+		crtc_state1->lane_count == crtc_state2->lane_count &&
+		crtc_state1->port_clock == crtc_state2->port_clock &&
+		mode_equal(&crtc_state1->hw.adjusted_mode,
+			   &crtc_state2->hw.adjusted_mode) &&
+		m_n_equal(&crtc_state1->dp_m_n, &crtc_state2->dp_m_n);
+}
+
+static u8
+intel_ddi_compute_port_sync_crtc_state(struct drm_connector *ref_connector,
+				       struct intel_crtc_state *ref_crtc_state)
+{
+	struct drm_connector *connector;
+	struct drm_connector_state *conn_state;
+	struct drm_i915_private *dev_priv = to_i915(ref_crtc_state->uapi.crtc->dev);
+	struct intel_atomic_state *state =
+		to_intel_atomic_state(ref_crtc_state->uapi.state);
+	struct intel_crtc *ref_crtc = to_intel_crtc(ref_crtc_state->uapi.crtc);
+	u8 transcoders = 0;
+	int i;
+
+	DRM_DEBUG_KMS("for [CRTC:%d:%s]\n",
+		      ref_crtc_state->uapi.crtc->base.id,
+		      ref_crtc_state->uapi.crtc->name);
+
+	if (INTEL_GEN(dev_priv) < 11)
+		return 0;
+
+	if (!intel_crtc_has_type(ref_crtc_state, INTEL_OUTPUT_DP))
+		return 0;
+
+	for_each_new_connector_in_state(&state->base, connector, conn_state, i) {
+		struct intel_crtc *crtc = to_intel_crtc(conn_state->crtc);
+		struct intel_crtc_state *crtc_state = NULL;
+
+		if (!crtc)
+			continue;
+
+		if (!(connector->has_tile &&
+		      connector->tile_group->id ==
+		      ref_connector->tile_group->id))
+			continue;
+
+		crtc_state = intel_atomic_get_new_crtc_state(state,
+							     crtc);
+		if (!crtcs_port_sync_compatible(ref_crtc_state,
+						crtc_state)) {
+			DRM_DEBUG_KMS("[CRTC:%d:%s] and [CRTC:%d:%s] not Port Sync Compatible\n",
+				      ref_crtc->base.base.id,
+				      ref_crtc->base.name,
+				      crtc->base.base.id, crtc->base.name);
+			continue;
+		}
+
+		transcoders |= BIT(crtc_state->cpu_transcoder);
+	}
+
+		return transcoders;
+}
+
+static int intel_ddi_compute_config_late(struct intel_encoder *encoder,
+					 struct intel_crtc_state *crtc_state,
+					 struct drm_connector_state *conn_state)
+{
+	struct drm_connector *connector = conn_state->connector;
+	struct drm_i915_private *dev_priv = to_i915(crtc_state->uapi.crtc->dev);
+	u8 port_sync_transcoders = 0;
+
+	DRM_DEBUG_KMS("[ENCODER:%d:%s] [CRTC:%d:%s]",
+		      encoder->base.base.id, encoder->base.name,
+		      crtc_state->uapi.crtc->base.id, crtc_state->uapi.crtc->name);
+
+	if (connector->has_tile)
+		port_sync_transcoders = intel_ddi_compute_port_sync_crtc_state(connector,
+									       crtc_state);
+
+	/*
+	 * EDP Transcoders cannot be ensalved
+	 * make them a master always when present
+	 */
+	if (port_sync_transcoders & BIT(TRANSCODER_EDP))
+		crtc_state->master_transcoder = TRANSCODER_EDP;
+	else
+		crtc_state->master_transcoder = ffs(port_sync_transcoders) - 1;
+
+	if (crtc_state->master_transcoder == crtc_state->cpu_transcoder) {
+		crtc_state->master_transcoder = INVALID_TRANSCODER;
+		crtc_state->sync_mode_slaves_mask =
+			port_sync_transcoders & ~BIT(crtc_state->cpu_transcoder);
+	}
+
+	drm_dbg_kms(&dev_priv->drm,
+		    "Master Transcoder = %s , Slave transcoder bitmask = 0x%08x\n",
+		    transcoder_name(crtc_state->master_transcoder),
+		    crtc_state->sync_mode_slaves_mask);
+
+	return 0;
+}
+
 static void intel_ddi_encoder_destroy(struct drm_encoder *encoder)
 {
 	struct intel_digital_port *dig_port = enc_to_dig_port(to_intel_encoder(encoder));
@@ -4749,6 +4873,7 @@ void intel_ddi_init(struct drm_i915_private *dev_priv, enum port port)
 	encoder->hotplug = intel_ddi_hotplug;
 	encoder->compute_output_type = intel_ddi_compute_output_type;
 	encoder->compute_config = intel_ddi_compute_config;
+	encoder->compute_config_late = intel_ddi_compute_config_late;
 	encoder->enable = intel_enable_ddi;
 	encoder->pre_pll_enable = intel_ddi_pre_pll_enable;
 	encoder->pre_enable = intel_ddi_pre_enable;
diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index de4ab51216f6..e638543f5f87 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -12545,126 +12545,6 @@ 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 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;
-	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;
-
-	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.
-	 */
-	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 (master_connector->tile_h_loc != master_connector->num_h_tile - 1 ||
-		    master_connector->tile_v_loc != master_connector->num_v_tile - 1)
-			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;
-		}
-	}
-	drm_connector_list_iter_end(&conn_iter);
-
-	if (!master_crtc) {
-		drm_dbg_kms(&dev_priv->drm,
-			    "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_dbg_kms(&dev_priv->drm,
-		    "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;
-}
-
 static int intel_crtc_atomic_check(struct intel_atomic_state *state,
 				   struct intel_crtc *crtc)
 {
@@ -13226,15 +13106,6 @@ intel_crtc_prepare_cleared_state(struct intel_crtc_state *crtc_state)
 	if (IS_G4X(dev_priv) ||
 	    IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
 		saved_state->wm = crtc_state->wm;
-	/*
-	 * Save the slave bitmask which gets filled for master crtc state during
-	 * slave atomic check call. For all other CRTCs reset the port sync variables
-	 * crtc_state->master_transcoder needs to be set to INVALID
-	 */
-	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;
 
 	memcpy(crtc_state, saved_state, sizeof(*crtc_state));
 	kfree(saved_state);
@@ -13325,24 +13196,6 @@ intel_modeset_pipe_config(struct intel_crtc_state *pipe_config)
 	drm_mode_set_crtcinfo(&pipe_config->hw.adjusted_mode,
 			      CRTC_STEREO_DOUBLE);
 
-	/* 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
 	 * adjust it according to limitations or connector properties, and also
 	 * a chance to reject the mode entirely.
@@ -13354,15 +13207,6 @@ 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_dbg_kms(&i915->drm,
-				    "Cannot assign Sync Mode CRTCs: %d\n",
-				    ret);
-			return ret;
-		}
-
 		ret = encoder->compute_config(encoder, pipe_config,
 					      connector_state);
 		if (ret < 0) {
-- 
2.19.1

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

[Intel-gfx] [PATCH 3/3] drm/i915/dp: Add all tiled and port sync conns to modeset

[Manasi Navare]

If one of the synced crtcs needs a full modeset, we need
to make sure all the synced crtcs are forced a full
modeset.

Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
---
 drivers/gpu/drm/i915/display/intel_display.c |  88 +------------
 drivers/gpu/drm/i915/display/intel_dp.c      | 131 ++++++++++++++++++-
 2 files changed, 131 insertions(+), 88 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index e638543f5f87..709a737638b6 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -13123,8 +13123,7 @@ intel_modeset_pipe_config(struct intel_crtc_state *pipe_config)
 	struct drm_i915_private *i915 = to_i915(pipe_config->uapi.crtc->dev);
 	struct drm_connector *connector;
 	struct drm_connector_state *connector_state;
-	int base_bpp, ret;
-	int i, tile_group_id = -1, num_tiled_conns = 0;
+	int base_bpp, ret, i;
 	bool retry = true;
 
 	pipe_config->cpu_transcoder =
@@ -14559,76 +14558,6 @@ static bool intel_cpu_transcoders_need_modeset(struct intel_atomic_state *state,
 	return false;
 }
 
-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(crtc_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
@@ -14656,21 +14585,6 @@ static int intel_atomic_check(struct drm_device *dev,
 	if (ret)
 		goto fail;
 
-	/**
-	 * This check adds all the connectors in current state that belong to
-	 * the same tile group to a full modeset.
-	 * This function directly sets the mode_changed to true and we also call
-	 * drm_atomic_add_affected_connectors(). Hence we are not explicitly
-	 * calling drm_atomic_helper_check_modeset() after this.
-	 *
-	 * Fixme: Handle some corner cases where one of the
-	 * tiled connectors gets disconnected and tile info is lost but since it
-	 * was previously synced to other conn, we need to add that to the modeset.
-	 */
-	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)) {
diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
index f4dede6253f8..7eb4b3dbbcb3 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -6582,6 +6582,135 @@ void intel_dp_encoder_reset(struct drm_encoder *encoder)
 	}
 }
 
+static int intel_modeset_tile_group(struct intel_atomic_state *state,
+				    int tile_group_id)
+{
+	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
+	struct drm_connector_list_iter conn_iter;
+	struct drm_connector *connector;
+	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 intel_crtc_state *crtc_state;
+		struct intel_crtc *crtc;
+
+		if (!connector->has_tile ||
+		    connector->tile_group->id != tile_group_id)
+			continue;
+
+		conn_state = drm_atomic_get_connector_state(&state->base,
+							    connector);
+		if (IS_ERR(conn_state)) {
+			ret = PTR_ERR(conn_state);
+			break;
+		}
+
+		crtc = to_intel_crtc(conn_state->crtc);
+
+		if (!crtc)
+			continue;
+
+		crtc_state = intel_atomic_get_crtc_state(&state->base, crtc);
+		if (IS_ERR(crtc_state)) {
+			ret = PTR_ERR(crtc_state);
+			break;
+		}
+
+		crtc_state->uapi.mode_changed = true;
+
+		ret = drm_atomic_add_affected_planes(&state->base, &crtc->base);
+		if (ret)
+			break;
+	}
+	drm_connector_list_iter_begin(&dev_priv->drm, &conn_iter);
+
+	return ret;
+}
+
+static int intel_modeset_affected_transcoders(struct intel_atomic_state *state, u8 transcoders)
+{
+	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
+	struct intel_crtc *crtc;
+
+	if (transcoders == 0)
+		return 0;
+
+	for_each_intel_crtc(&dev_priv->drm, crtc) {
+		struct intel_crtc_state *crtc_state;
+		int ret;
+
+		if ((transcoders & BIT(crtc->pipe)) == 0)
+			continue;
+
+		crtc_state = intel_atomic_get_crtc_state(&state->base, crtc);
+		if (IS_ERR(crtc_state))
+			return PTR_ERR(crtc_state);
+
+		if (!crtc_state->hw.enable)
+			continue;
+
+		crtc_state->uapi.mode_changed = true;
+
+		ret = drm_atomic_add_affected_connectors(&state->base, &crtc->base);
+		if (ret)
+			return ret;
+
+		WARN_ON((enum transcoder)crtc->pipe != crtc_state->cpu_transcoder);
+
+		transcoders &= ~BIT(crtc_state->cpu_transcoder);
+	}
+
+	WARN_ON(transcoders != 0);
+
+	return 0;
+
+}
+
+static int intel_modeset_synced_crtcs(struct intel_atomic_state *state,
+				      struct drm_connector *connector)
+{
+	const struct drm_connector_state *old_conn_state =
+		drm_atomic_get_old_connector_state(&state->base, connector);
+	const struct intel_crtc_state *old_crtc_state;
+	struct intel_crtc *crtc;
+
+	crtc = to_intel_crtc(old_conn_state->crtc);
+	if (!crtc)
+		return 0;
+
+	old_crtc_state = intel_atomic_get_old_crtc_state(state, crtc);
+
+	if (!old_crtc_state->hw.active)
+		return 0;
+
+	return intel_modeset_affected_transcoders(state,
+						  (old_crtc_state->sync_mode_slaves_mask |
+						   BIT(old_crtc_state->master_transcoder)) &
+						  ~BIT(old_crtc_state->cpu_transcoder));
+}
+
+static int intel_dp_connector_atomic_check(struct drm_connector *conn,
+					   struct drm_atomic_state *_state)
+{
+	struct drm_i915_private *dev_priv = to_i915(conn->dev);
+	struct intel_atomic_state *state = to_intel_atomic_state(_state);
+	int ret;
+
+	ret = intel_digital_connector_atomic_check(conn, &state->base);
+	if (ret)
+		return ret;
+
+	if (INTEL_GEN(dev_priv) >= 11 && conn->has_tile) {
+		ret = intel_modeset_tile_group(state, conn->tile_group->id);
+		if (ret)
+			return ret;
+	}
+
+	return intel_modeset_synced_crtcs(state, conn);
+}
+
 static const struct drm_connector_funcs intel_dp_connector_funcs = {
 	.force = intel_dp_force,
 	.fill_modes = drm_helper_probe_single_connector_modes,
@@ -6598,7 +6727,7 @@ static const struct drm_connector_helper_funcs intel_dp_connector_helper_funcs =
 	.detect_ctx = intel_dp_detect,
 	.get_modes = intel_dp_get_modes,
 	.mode_valid = intel_dp_mode_valid,
-	.atomic_check = intel_digital_connector_atomic_check,
+	.atomic_check = intel_dp_connector_atomic_check,
 };
 
 static const struct drm_encoder_funcs intel_dp_enc_funcs = {
-- 
2.19.1

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

Re: [Intel-gfx] [PATCH 2/3] drm/i915/dp: Compute port sync crtc states post compute_config()

[Ville Syrjälä]

On Fri, Jan 31, 2020 at 09:15:46AM -0800, Manasi Navare wrote:
> This patch pushes out the computation of master and slave
> transcoders in crtc states after encoder's compute_config hook.
> This ensures that the assigned master slave crtcs have exact same
> mode and timings which is a requirement for Port sync mode
> to be enabled.
> 
> Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_ddi.c     | 125 +++++++++++++++
>  drivers/gpu/drm/i915/display/intel_display.c | 156 -------------------
>  2 files changed, 125 insertions(+), 156 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
> index c96f629cddc3..4ec36eb5ce28 100644
> --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> @@ -4440,6 +4440,130 @@ static int intel_ddi_compute_config(struct intel_encoder *encoder,
>  	return 0;
>  }
>  
> +static bool mode_equal(const struct drm_display_mode *mode1,
> +		       const struct drm_display_mode *mode2)
> +{
> +	return drm_mode_match(mode1, mode2,
> +			      DRM_MODE_MATCH_TIMINGS |
> +			      DRM_MODE_MATCH_FLAGS |
> +			      DRM_MODE_MATCH_3D_FLAGS) &&
> +		mode1->clock == mode2->clock; /* we want an exact match */

Trying to remember why we chose .clock rather than .crtc_clock... Ah,
because drm_mode_match() anyway looks at the non-crtc timings. Yeah,
should work fine since we make sure the 3D flags match so the crtc
timings vs non-crtc timings should be consistent for each one.

> +}
> +
> +static bool m_n_equal(const struct intel_link_m_n *m_n_1,
> +		      const struct intel_link_m_n *m_n_2)
> +{
> +	return m_n_1->tu == m_n_2->tu &&
> +		m_n_1->gmch_m == m_n_2->gmch_m &&
> +		m_n_1->gmch_n == m_n_2->gmch_n &&
> +		m_n_1->link_m == m_n_2->link_m &&
> +		m_n_1->link_n == m_n_2->link_n;
> +}
> +
> +static bool crtcs_port_sync_compatible(struct intel_crtc_state *crtc_state1,
> +				       struct intel_crtc_state *crtc_state2)
> +{
> +	return crtc_state1->hw.active && crtc_state2->hw.active &&
> +		crtc_state1->output_types == crtc_state2->output_types &&
> +		crtc_state1->output_format == crtc_state2->output_format &&
> +		crtc_state1->lane_count == crtc_state2->lane_count &&
> +		crtc_state1->port_clock == crtc_state2->port_clock &&
> +		mode_equal(&crtc_state1->hw.adjusted_mode,
> +			   &crtc_state2->hw.adjusted_mode) &&
> +		m_n_equal(&crtc_state1->dp_m_n, &crtc_state2->dp_m_n);
> +}
> +
> +static u8
> +intel_ddi_compute_port_sync_crtc_state

The name is misleading. something_port_sync_transcoders() etc. to
better describe what it returns.

>					 (struct drm_connector *ref_connector,

You don't need the whole connector. The tile group id will suffice.

> +				       struct intel_crtc_state *ref_crtc_state)

const

> +{
> +	struct drm_connector *connector;
> +	struct drm_connector_state *conn_state;
> +	struct drm_i915_private *dev_priv = to_i915(ref_crtc_state->uapi.crtc->dev);
> +	struct intel_atomic_state *state =
> +		to_intel_atomic_state(ref_crtc_state->uapi.state);
> +	struct intel_crtc *ref_crtc = to_intel_crtc(ref_crtc_state->uapi.crtc);
> +	u8 transcoders = 0;
> +	int i;
> +
> +	DRM_DEBUG_KMS("for [CRTC:%d:%s]\n",
> +		      ref_crtc_state->uapi.crtc->base.id,
> +		      ref_crtc_state->uapi.crtc->name);

More noise than signal. Pls remove.

> +
> +	if (INTEL_GEN(dev_priv) < 11)
> +		return 0;
> +
> +	if (!intel_crtc_has_type(ref_crtc_state, INTEL_OUTPUT_DP))
> +		return 0;
> +
> +	for_each_new_connector_in_state(&state->base, connector, conn_state, i) {
> +		struct intel_crtc *crtc = to_intel_crtc(conn_state->crtc);
> +		struct intel_crtc_state *crtc_state = NULL;
> +
> +		if (!crtc)
> +			continue;
> +
> +		if (!(connector->has_tile &&
> +		      connector->tile_group->id ==
> +		      ref_connector->tile_group->id))
> +			continue;
> +
> +		crtc_state = intel_atomic_get_new_crtc_state(state,
> +							     crtc);
> +		if (!crtcs_port_sync_compatible(ref_crtc_state,
> +						crtc_state)) {
> +			DRM_DEBUG_KMS("[CRTC:%d:%s] and [CRTC:%d:%s] not Port Sync Compatible\n",
> +				      ref_crtc->base.base.id,
> +				      ref_crtc->base.name,
> +				      crtc->base.base.id, crtc->base.name);

Another noisy debug. The state dump will tell us everything we need to
know. If not we should improve the state dump instead.

> +			continue;
> +		}
> +
> +		transcoders |= BIT(crtc_state->cpu_transcoder);
> +	}
> +
> +		return transcoders;

indent fail

> +}
> +
> +static int intel_ddi_compute_config_late(struct intel_encoder *encoder,
> +					 struct intel_crtc_state *crtc_state,
> +					 struct drm_connector_state *conn_state)
> +{
> +	struct drm_connector *connector = conn_state->connector;
> +	struct drm_i915_private *dev_priv = to_i915(crtc_state->uapi.crtc->dev);
> +	u8 port_sync_transcoders = 0;
> +
> +	DRM_DEBUG_KMS("[ENCODER:%d:%s] [CRTC:%d:%s]",
> +		      encoder->base.base.id, encoder->base.name,
> +		      crtc_state->uapi.crtc->base.id, crtc_state->uapi.crtc->name);
> +
> +	if (connector->has_tile)
> +		port_sync_transcoders = intel_ddi_compute_port_sync_crtc_state(connector,
> +									       crtc_state);
> +
> +	/*
> +	 * EDP Transcoders cannot be ensalved
> +	 * make them a master always when present
> +	 */
> +	if (port_sync_transcoders & BIT(TRANSCODER_EDP))
> +		crtc_state->master_transcoder = TRANSCODER_EDP;
> +	else
> +		crtc_state->master_transcoder = ffs(port_sync_transcoders) - 1;
> +
> +	if (crtc_state->master_transcoder == crtc_state->cpu_transcoder) {
> +		crtc_state->master_transcoder = INVALID_TRANSCODER;
> +		crtc_state->sync_mode_slaves_mask =
> +			port_sync_transcoders & ~BIT(crtc_state->cpu_transcoder);
> +	}
> +
> +	drm_dbg_kms(&dev_priv->drm,
> +		    "Master Transcoder = %s , Slave transcoder bitmask = 0x%08x\n",
> +		    transcoder_name(crtc_state->master_transcoder),
> +		    crtc_state->sync_mode_slaves_mask);

Redundant. Again it's part of the state dump anyway. Pls remove.

> +
> +	return 0;
> +}
> +
>  static void intel_ddi_encoder_destroy(struct drm_encoder *encoder)
>  {
>  	struct intel_digital_port *dig_port = enc_to_dig_port(to_intel_encoder(encoder));
> @@ -4749,6 +4873,7 @@ void intel_ddi_init(struct drm_i915_private *dev_priv, enum port port)
>  	encoder->hotplug = intel_ddi_hotplug;
>  	encoder->compute_output_type = intel_ddi_compute_output_type;
>  	encoder->compute_config = intel_ddi_compute_config;
> +	encoder->compute_config_late = intel_ddi_compute_config_late;
>  	encoder->enable = intel_enable_ddi;
>  	encoder->pre_pll_enable = intel_ddi_pre_pll_enable;
>  	encoder->pre_enable = intel_ddi_pre_enable;
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index de4ab51216f6..e638543f5f87 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -12545,126 +12545,6 @@ 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 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;
> -	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;
> -
> -	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.
> -	 */
> -	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 (master_connector->tile_h_loc != master_connector->num_h_tile - 1 ||
> -		    master_connector->tile_v_loc != master_connector->num_v_tile - 1)
> -			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;
> -		}
> -	}
> -	drm_connector_list_iter_end(&conn_iter);
> -
> -	if (!master_crtc) {
> -		drm_dbg_kms(&dev_priv->drm,
> -			    "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_dbg_kms(&dev_priv->drm,
> -		    "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;
> -}
> -
>  static int intel_crtc_atomic_check(struct intel_atomic_state *state,
>  				   struct intel_crtc *crtc)
>  {
> @@ -13226,15 +13106,6 @@ intel_crtc_prepare_cleared_state(struct intel_crtc_state *crtc_state)
>  	if (IS_G4X(dev_priv) ||
>  	    IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
>  		saved_state->wm = crtc_state->wm;
> -	/*
> -	 * Save the slave bitmask which gets filled for master crtc state during
> -	 * slave atomic check call. For all other CRTCs reset the port sync variables
> -	 * crtc_state->master_transcoder needs to be set to INVALID
> -	 */
> -	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;
>  
>  	memcpy(crtc_state, saved_state, sizeof(*crtc_state));
>  	kfree(saved_state);
> @@ -13325,24 +13196,6 @@ intel_modeset_pipe_config(struct intel_crtc_state *pipe_config)
>  	drm_mode_set_crtcinfo(&pipe_config->hw.adjusted_mode,
>  			      CRTC_STEREO_DOUBLE);
>  
> -	/* 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
>  	 * adjust it according to limitations or connector properties, and also
>  	 * a chance to reject the mode entirely.
> @@ -13354,15 +13207,6 @@ 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_dbg_kms(&i915->drm,
> -				    "Cannot assign Sync Mode CRTCs: %d\n",
> -				    ret);
> -			return ret;
> -		}
> -
>  		ret = encoder->compute_config(encoder, pipe_config,
>  					      connector_state);
>  		if (ret < 0) {
> -- 
> 2.19.1

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

Re: [Intel-gfx] [PATCH 3/3] drm/i915/dp: Add all tiled and port sync conns to modeset

[Ville Syrjälä]

On Fri, Jan 31, 2020 at 09:15:47AM -0800, Manasi Navare wrote:
> If one of the synced crtcs needs a full modeset, we need
> to make sure all the synced crtcs are forced a full
> modeset.
> 
> Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_display.c |  88 +------------
>  drivers/gpu/drm/i915/display/intel_dp.c      | 131 ++++++++++++++++++-
>  2 files changed, 131 insertions(+), 88 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index e638543f5f87..709a737638b6 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -13123,8 +13123,7 @@ intel_modeset_pipe_config(struct intel_crtc_state *pipe_config)
>  	struct drm_i915_private *i915 = to_i915(pipe_config->uapi.crtc->dev);
>  	struct drm_connector *connector;
>  	struct drm_connector_state *connector_state;
> -	int base_bpp, ret;
> -	int i, tile_group_id = -1, num_tiled_conns = 0;
> +	int base_bpp, ret, i;
>  	bool retry = true;
>  
>  	pipe_config->cpu_transcoder =
> @@ -14559,76 +14558,6 @@ static bool intel_cpu_transcoders_need_modeset(struct intel_atomic_state *state,
>  	return false;
>  }
>  
> -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(crtc_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
> @@ -14656,21 +14585,6 @@ static int intel_atomic_check(struct drm_device *dev,
>  	if (ret)
>  		goto fail;
>  
> -	/**
> -	 * This check adds all the connectors in current state that belong to
> -	 * the same tile group to a full modeset.
> -	 * This function directly sets the mode_changed to true and we also call
> -	 * drm_atomic_add_affected_connectors(). Hence we are not explicitly
> -	 * calling drm_atomic_helper_check_modeset() after this.
> -	 *
> -	 * Fixme: Handle some corner cases where one of the
> -	 * tiled connectors gets disconnected and tile info is lost but since it
> -	 * was previously synced to other conn, we need to add that to the modeset.
> -	 */
> -	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)) {
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> index f4dede6253f8..7eb4b3dbbcb3 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -6582,6 +6582,135 @@ void intel_dp_encoder_reset(struct drm_encoder *encoder)
>  	}
>  }
>  
> +static int intel_modeset_tile_group(struct intel_atomic_state *state,
> +				    int tile_group_id)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> +	struct drm_connector_list_iter conn_iter;
> +	struct drm_connector *connector;
> +	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 intel_crtc_state *crtc_state;
> +		struct intel_crtc *crtc;
> +
> +		if (!connector->has_tile ||
> +		    connector->tile_group->id != tile_group_id)
> +			continue;
> +
> +		conn_state = drm_atomic_get_connector_state(&state->base,
> +							    connector);
> +		if (IS_ERR(conn_state)) {
> +			ret = PTR_ERR(conn_state);
> +			break;
> +		}
> +
> +		crtc = to_intel_crtc(conn_state->crtc);
> +
> +		if (!crtc)
> +			continue;
> +
> +		crtc_state = intel_atomic_get_crtc_state(&state->base, crtc);
> +		if (IS_ERR(crtc_state)) {
> +			ret = PTR_ERR(crtc_state);
> +			break;
> +		}
> +
> +		crtc_state->uapi.mode_changed = true;
> +
> +		ret = drm_atomic_add_affected_planes(&state->base, &crtc->base);
> +		if (ret)
> +			break;
> +	}
> +	drm_connector_list_iter_begin(&dev_priv->drm, &conn_iter);
> +
> +	return ret;
> +}
> +
> +static int intel_modeset_affected_transcoders(struct intel_atomic_state *state, u8 transcoders)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> +	struct intel_crtc *crtc;
> +
> +	if (transcoders == 0)
> +		return 0;
> +
> +	for_each_intel_crtc(&dev_priv->drm, crtc) {
> +		struct intel_crtc_state *crtc_state;
> +		int ret;
> +
> +		if ((transcoders & BIT(crtc->pipe)) == 0)
> +			continue;

Dropping the EDP transcoder on the floor here. I think we should just do
the guaranteed correct thing and look at the cpu_transcoder instead.
Yes, that does mean we more or less end up adding all crtcs to the state 
whenever modesetting any synced crtc, but so be it. We can think of ways
to optimize that later.

> +
> +		crtc_state = intel_atomic_get_crtc_state(&state->base, crtc);
> +		if (IS_ERR(crtc_state))
> +			return PTR_ERR(crtc_state);
> +
> +		if (!crtc_state->hw.enable)
> +			continue;
> +
> +		crtc_state->uapi.mode_changed = true;
> +
> +		ret = drm_atomic_add_affected_connectors(&state->base, &crtc->base);
> +		if (ret)
> +			return ret;

Missing add_affected_planes() here I think. Or was that guaranteed to be
done by the helper? Can't recall.

> +
> +		WARN_ON((enum transcoder)crtc->pipe != crtc_state->cpu_transcoder);
> +
> +		transcoders &= ~BIT(crtc_state->cpu_transcoder);
> +	}
> +
> +	WARN_ON(transcoders != 0);
> +
> +	return 0;
> +
> +}
> +
> +static int intel_modeset_synced_crtcs(struct intel_atomic_state *state,
> +				      struct drm_connector *connector)
> +{
> +	const struct drm_connector_state *old_conn_state =
> +		drm_atomic_get_old_connector_state(&state->base, connector);
> +	const struct intel_crtc_state *old_crtc_state;
> +	struct intel_crtc *crtc;
> +
> +	crtc = to_intel_crtc(old_conn_state->crtc);
> +	if (!crtc)
> +		return 0;
> +
> +	old_crtc_state = intel_atomic_get_old_crtc_state(state, crtc);
> +
> +	if (!old_crtc_state->hw.active)
> +		return 0;
> +
> +	return intel_modeset_affected_transcoders(state,
> +						  (old_crtc_state->sync_mode_slaves_mask |
> +						   BIT(old_crtc_state->master_transcoder)) &

This seems to have the same master==INVALID problem that we faced
elsewhere already.

> +						  ~BIT(old_crtc_state->cpu_transcoder));

I guess this part is redundant. Or can we somehow have our own
transcoder be included in sync_mode_slaves_mask/master_transcoder?

> +}
> +
> +static int intel_dp_connector_atomic_check(struct drm_connector *conn,
> +					   struct drm_atomic_state *_state)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(conn->dev);
> +	struct intel_atomic_state *state = to_intel_atomic_state(_state);
> +	int ret;
> +
> +	ret = intel_digital_connector_atomic_check(conn, &state->base);
> +	if (ret)
> +		return ret;
> +
> +	if (INTEL_GEN(dev_priv) >= 11 && conn->has_tile) {
> +		ret = intel_modeset_tile_group(state, conn->tile_group->id);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return intel_modeset_synced_crtcs(state, conn);

No gen check... Ah, yeah we don't need it because the port sync state
will be INVALID/0.

> +}
> +
>  static const struct drm_connector_funcs intel_dp_connector_funcs = {
>  	.force = intel_dp_force,
>  	.fill_modes = drm_helper_probe_single_connector_modes,
> @@ -6598,7 +6727,7 @@ static const struct drm_connector_helper_funcs intel_dp_connector_helper_funcs =
>  	.detect_ctx = intel_dp_detect,
>  	.get_modes = intel_dp_get_modes,
>  	.mode_valid = intel_dp_mode_valid,
> -	.atomic_check = intel_digital_connector_atomic_check,
> +	.atomic_check = intel_dp_connector_atomic_check,
>  };
>  
>  static const struct drm_encoder_funcs intel_dp_enc_funcs = {
> -- 
> 2.19.1

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

Re: [Intel-gfx] [PATCH 3/3] drm/i915/dp: Add all tiled and port sync conns to modeset

[Manasi Navare]

On Fri, Jan 31, 2020 at 07:53:23PM +0200, Ville Syrjälä wrote:
> On Fri, Jan 31, 2020 at 09:15:47AM -0800, Manasi Navare wrote:
> > If one of the synced crtcs needs a full modeset, we need
> > to make sure all the synced crtcs are forced a full
> > modeset.
> > 
> > Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_display.c |  88 +------------
> >  drivers/gpu/drm/i915/display/intel_dp.c      | 131 ++++++++++++++++++-
> >  2 files changed, 131 insertions(+), 88 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > index e638543f5f87..709a737638b6 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -13123,8 +13123,7 @@ intel_modeset_pipe_config(struct intel_crtc_state *pipe_config)
> >  	struct drm_i915_private *i915 = to_i915(pipe_config->uapi.crtc->dev);
> >  	struct drm_connector *connector;
> >  	struct drm_connector_state *connector_state;
> > -	int base_bpp, ret;
> > -	int i, tile_group_id = -1, num_tiled_conns = 0;
> > +	int base_bpp, ret, i;
> >  	bool retry = true;
> >  
> >  	pipe_config->cpu_transcoder =
> > @@ -14559,76 +14558,6 @@ static bool intel_cpu_transcoders_need_modeset(struct intel_atomic_state *state,
> >  	return false;
> >  }
> >  
> > -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(crtc_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
> > @@ -14656,21 +14585,6 @@ static int intel_atomic_check(struct drm_device *dev,
> >  	if (ret)
> >  		goto fail;
> >  
> > -	/**
> > -	 * This check adds all the connectors in current state that belong to
> > -	 * the same tile group to a full modeset.
> > -	 * This function directly sets the mode_changed to true and we also call
> > -	 * drm_atomic_add_affected_connectors(). Hence we are not explicitly
> > -	 * calling drm_atomic_helper_check_modeset() after this.
> > -	 *
> > -	 * Fixme: Handle some corner cases where one of the
> > -	 * tiled connectors gets disconnected and tile info is lost but since it
> > -	 * was previously synced to other conn, we need to add that to the modeset.
> > -	 */
> > -	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)) {
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> > index f4dede6253f8..7eb4b3dbbcb3 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > @@ -6582,6 +6582,135 @@ void intel_dp_encoder_reset(struct drm_encoder *encoder)
> >  	}
> >  }
> >  
> > +static int intel_modeset_tile_group(struct intel_atomic_state *state,
> > +				    int tile_group_id)
> > +{
> > +	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> > +	struct drm_connector_list_iter conn_iter;
> > +	struct drm_connector *connector;
> > +	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 intel_crtc_state *crtc_state;
> > +		struct intel_crtc *crtc;
> > +
> > +		if (!connector->has_tile ||
> > +		    connector->tile_group->id != tile_group_id)
> > +			continue;
> > +
> > +		conn_state = drm_atomic_get_connector_state(&state->base,
> > +							    connector);
> > +		if (IS_ERR(conn_state)) {
> > +			ret = PTR_ERR(conn_state);
> > +			break;
> > +		}
> > +
> > +		crtc = to_intel_crtc(conn_state->crtc);
> > +
> > +		if (!crtc)
> > +			continue;
> > +
> > +		crtc_state = intel_atomic_get_crtc_state(&state->base, crtc);
> > +		if (IS_ERR(crtc_state)) {
> > +			ret = PTR_ERR(crtc_state);
> > +			break;
> > +		}
> > +
> > +		crtc_state->uapi.mode_changed = true;
> > +
> > +		ret = drm_atomic_add_affected_planes(&state->base, &crtc->base);
> > +		if (ret)
> > +			break;
> > +	}
> > +	drm_connector_list_iter_begin(&dev_priv->drm, &conn_iter);
> > +
> > +	return ret;
> > +}
> > +
> > +static int intel_modeset_affected_transcoders(struct intel_atomic_state *state, u8 transcoders)
> > +{
> > +	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> > +	struct intel_crtc *crtc;
> > +
> > +	if (transcoders == 0)
> > +		return 0;
> > +
> > +	for_each_intel_crtc(&dev_priv->drm, crtc) {
> > +		struct intel_crtc_state *crtc_state;
> > +		int ret;
> > +
> > +		if ((transcoders & BIT(crtc->pipe)) == 0)
> > +			continue;
> 
> Dropping the EDP transcoder on the floor here. I think we should just do
> the guaranteed correct thing and look at the cpu_transcoder instead.
> Yes, that does mean we more or less end up adding all crtcs to the state 
> whenever modesetting any synced crtc, but so be it. We can think of ways
> to optimize that later.
>

So should i not even do this check of trans & BIT(crtc->pipe) ? Instead just proceed
to go get the crtc state and then check if trans & BIT(crtc_state->cpu_trans) ?

Or should I keep this as is and then add the second loop like in your branch?
 
> > +
> > +		crtc_state = intel_atomic_get_crtc_state(&state->base, crtc);
> > +		if (IS_ERR(crtc_state))
> > +			return PTR_ERR(crtc_state);
> > +
> > +		if (!crtc_state->hw.enable)
> > +			continue;
> > +
> > +		crtc_state->uapi.mode_changed = true;
> > +
> > +		ret = drm_atomic_add_affected_connectors(&state->base, &crtc->base);
> > +		if (ret)
> > +			return ret;
> 
> Missing add_affected_planes() here I think. Or was that guaranteed to be
> done by the helper? Can't recall.
>

No i think i missed it, will add that
 
> > +
> > +		WARN_ON((enum transcoder)crtc->pipe != crtc_state->cpu_transcoder);
> > +
> > +		transcoders &= ~BIT(crtc_state->cpu_transcoder);
> > +	}
> > +
> > +	WARN_ON(transcoders != 0);
> > +
> > +	return 0;
> > +
> > +}
> > +
> > +static int intel_modeset_synced_crtcs(struct intel_atomic_state *state,
> > +				      struct drm_connector *connector)
> > +{
> > +	const struct drm_connector_state *old_conn_state =
> > +		drm_atomic_get_old_connector_state(&state->base, connector);
> > +	const struct intel_crtc_state *old_crtc_state;
> > +	struct intel_crtc *crtc;
> > +
> > +	crtc = to_intel_crtc(old_conn_state->crtc);
> > +	if (!crtc)
> > +		return 0;
> > +
> > +	old_crtc_state = intel_atomic_get_old_crtc_state(state, crtc);
> > +
> > +	if (!old_crtc_state->hw.active)
> > +		return 0;
> > +
> > +	return intel_modeset_affected_transcoders(state,
> > +						  (old_crtc_state->sync_mode_slaves_mask |
> > +						   BIT(old_crtc_state->master_transcoder)) &
> 
> This seems to have the same master==INVALID problem that we faced
> elsewhere already.
>

Yes will have to add the same check and add it only for master_trans != INVALID, will add that
 
> > +						  ~BIT(old_crtc_state->cpu_transcoder));
> 
> I guess this part is redundant. Or can we somehow have our own
> transcoder be included in sync_mode_slaves_mask/master_transcoder?
>

Actually shouldnt it be:

if old_crtc_state->needs_modeset() {
	 then call intel_modeset_affected_transcoders(state,
							(old_crtc_state->sync_mode_slaves_mask |
							 BIT(old_crtc_state->master_transcoder)) &
							~BIT(old_crtc_state->cpu_transcoder));

dont understand why ~BIT(old_crtc_state->cpu_transcoder) is redundant? why do we need to 
force modeset there if we add this needs_modeset check?

Manasi

> > +}
> > +
> > +static int intel_dp_connector_atomic_check(struct drm_connector *conn,
> > +					   struct drm_atomic_state *_state)
> > +{
> > +	struct drm_i915_private *dev_priv = to_i915(conn->dev);
> > +	struct intel_atomic_state *state = to_intel_atomic_state(_state);
> > +	int ret;
> > +
> > +	ret = intel_digital_connector_atomic_check(conn, &state->base);
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (INTEL_GEN(dev_priv) >= 11 && conn->has_tile) {
> > +		ret = intel_modeset_tile_group(state, conn->tile_group->id);
> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> > +	return intel_modeset_synced_crtcs(state, conn);
> 
> No gen check... Ah, yeah we don't need it because the port sync state
> will be INVALID/0.
> 
> > +}
> > +
> >  static const struct drm_connector_funcs intel_dp_connector_funcs = {
> >  	.force = intel_dp_force,
> >  	.fill_modes = drm_helper_probe_single_connector_modes,
> > @@ -6598,7 +6727,7 @@ static const struct drm_connector_helper_funcs intel_dp_connector_helper_funcs =
> >  	.detect_ctx = intel_dp_detect,
> >  	.get_modes = intel_dp_get_modes,
> >  	.mode_valid = intel_dp_mode_valid,
> > -	.atomic_check = intel_digital_connector_atomic_check,
> > +	.atomic_check = intel_dp_connector_atomic_check,
> >  };
> >  
> >  static const struct drm_encoder_funcs intel_dp_enc_funcs = {
> > -- 
> > 2.19.1
> 
> -- 
> Ville Syrjälä
> Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 3/3] drm/i915/dp: Add all tiled and port sync conns to modeset

[Ville Syrjälä]

On Fri, Jan 31, 2020 at 11:46:25AM -0800, Manasi Navare wrote:
> On Fri, Jan 31, 2020 at 07:53:23PM +0200, Ville Syrjälä wrote:
> > On Fri, Jan 31, 2020 at 09:15:47AM -0800, Manasi Navare wrote:
> > > If one of the synced crtcs needs a full modeset, we need
> > > to make sure all the synced crtcs are forced a full
> > > modeset.
> > > 
> > > Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_display.c |  88 +------------
> > >  drivers/gpu/drm/i915/display/intel_dp.c      | 131 ++++++++++++++++++-
> > >  2 files changed, 131 insertions(+), 88 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > > index e638543f5f87..709a737638b6 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > @@ -13123,8 +13123,7 @@ intel_modeset_pipe_config(struct intel_crtc_state *pipe_config)
> > >  	struct drm_i915_private *i915 = to_i915(pipe_config->uapi.crtc->dev);
> > >  	struct drm_connector *connector;
> > >  	struct drm_connector_state *connector_state;
> > > -	int base_bpp, ret;
> > > -	int i, tile_group_id = -1, num_tiled_conns = 0;
> > > +	int base_bpp, ret, i;
> > >  	bool retry = true;
> > >  
> > >  	pipe_config->cpu_transcoder =
> > > @@ -14559,76 +14558,6 @@ static bool intel_cpu_transcoders_need_modeset(struct intel_atomic_state *state,
> > >  	return false;
> > >  }
> > >  
> > > -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(crtc_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
> > > @@ -14656,21 +14585,6 @@ static int intel_atomic_check(struct drm_device *dev,
> > >  	if (ret)
> > >  		goto fail;
> > >  
> > > -	/**
> > > -	 * This check adds all the connectors in current state that belong to
> > > -	 * the same tile group to a full modeset.
> > > -	 * This function directly sets the mode_changed to true and we also call
> > > -	 * drm_atomic_add_affected_connectors(). Hence we are not explicitly
> > > -	 * calling drm_atomic_helper_check_modeset() after this.
> > > -	 *
> > > -	 * Fixme: Handle some corner cases where one of the
> > > -	 * tiled connectors gets disconnected and tile info is lost but since it
> > > -	 * was previously synced to other conn, we need to add that to the modeset.
> > > -	 */
> > > -	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)) {
> > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> > > index f4dede6253f8..7eb4b3dbbcb3 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > > @@ -6582,6 +6582,135 @@ void intel_dp_encoder_reset(struct drm_encoder *encoder)
> > >  	}
> > >  }
> > >  
> > > +static int intel_modeset_tile_group(struct intel_atomic_state *state,
> > > +				    int tile_group_id)
> > > +{
> > > +	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> > > +	struct drm_connector_list_iter conn_iter;
> > > +	struct drm_connector *connector;
> > > +	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 intel_crtc_state *crtc_state;
> > > +		struct intel_crtc *crtc;
> > > +
> > > +		if (!connector->has_tile ||
> > > +		    connector->tile_group->id != tile_group_id)
> > > +			continue;
> > > +
> > > +		conn_state = drm_atomic_get_connector_state(&state->base,
> > > +							    connector);
> > > +		if (IS_ERR(conn_state)) {
> > > +			ret = PTR_ERR(conn_state);
> > > +			break;
> > > +		}
> > > +
> > > +		crtc = to_intel_crtc(conn_state->crtc);
> > > +
> > > +		if (!crtc)
> > > +			continue;
> > > +
> > > +		crtc_state = intel_atomic_get_crtc_state(&state->base, crtc);
> > > +		if (IS_ERR(crtc_state)) {
> > > +			ret = PTR_ERR(crtc_state);
> > > +			break;
> > > +		}
> > > +
> > > +		crtc_state->uapi.mode_changed = true;
> > > +
> > > +		ret = drm_atomic_add_affected_planes(&state->base, &crtc->base);
> > > +		if (ret)
> > > +			break;
> > > +	}
> > > +	drm_connector_list_iter_begin(&dev_priv->drm, &conn_iter);
> > > +
> > > +	return ret;
> > > +}
> > > +
> > > +static int intel_modeset_affected_transcoders(struct intel_atomic_state *state, u8 transcoders)
> > > +{
> > > +	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> > > +	struct intel_crtc *crtc;
> > > +
> > > +	if (transcoders == 0)
> > > +		return 0;
> > > +
> > > +	for_each_intel_crtc(&dev_priv->drm, crtc) {
> > > +		struct intel_crtc_state *crtc_state;
> > > +		int ret;
> > > +
> > > +		if ((transcoders & BIT(crtc->pipe)) == 0)
> > > +			continue;
> > 
> > Dropping the EDP transcoder on the floor here. I think we should just do
> > the guaranteed correct thing and look at the cpu_transcoder instead.
> > Yes, that does mean we more or less end up adding all crtcs to the state 
> > whenever modesetting any synced crtc, but so be it. We can think of ways
> > to optimize that later.
> >
> 
> So should i not even do this check of trans & BIT(crtc->pipe) ? Instead just proceed
> to go get the crtc state and then check if trans & BIT(crtc_state->cpu_trans) ?

Yeah, I think that's fine for now. We can try to optimize it later if
needed.

> 
> Or should I keep this as is and then add the second loop like in your branch?
>  
> > > +
> > > +		crtc_state = intel_atomic_get_crtc_state(&state->base, crtc);
> > > +		if (IS_ERR(crtc_state))
> > > +			return PTR_ERR(crtc_state);
> > > +
> > > +		if (!crtc_state->hw.enable)
> > > +			continue;
> > > +
> > > +		crtc_state->uapi.mode_changed = true;
> > > +
> > > +		ret = drm_atomic_add_affected_connectors(&state->base, &crtc->base);
> > > +		if (ret)
> > > +			return ret;
> > 
> > Missing add_affected_planes() here I think. Or was that guaranteed to be
> > done by the helper? Can't recall.
> >
> 
> No i think i missed it, will add that
>  
> > > +
> > > +		WARN_ON((enum transcoder)crtc->pipe != crtc_state->cpu_transcoder);
> > > +
> > > +		transcoders &= ~BIT(crtc_state->cpu_transcoder);
> > > +	}
> > > +
> > > +	WARN_ON(transcoders != 0);
> > > +
> > > +	return 0;
> > > +
> > > +}
> > > +
> > > +static int intel_modeset_synced_crtcs(struct intel_atomic_state *state,
> > > +				      struct drm_connector *connector)
> > > +{
> > > +	const struct drm_connector_state *old_conn_state =
> > > +		drm_atomic_get_old_connector_state(&state->base, connector);
> > > +	const struct intel_crtc_state *old_crtc_state;
> > > +	struct intel_crtc *crtc;
> > > +
> > > +	crtc = to_intel_crtc(old_conn_state->crtc);
> > > +	if (!crtc)
> > > +		return 0;
> > > +
> > > +	old_crtc_state = intel_atomic_get_old_crtc_state(state, crtc);
> > > +
> > > +	if (!old_crtc_state->hw.active)
> > > +		return 0;
> > > +
> > > +	return intel_modeset_affected_transcoders(state,
> > > +						  (old_crtc_state->sync_mode_slaves_mask |
> > > +						   BIT(old_crtc_state->master_transcoder)) &
> > 
> > This seems to have the same master==INVALID problem that we faced
> > elsewhere already.
> >
> 
> Yes will have to add the same check and add it only for master_trans != INVALID, will add that
>  
> > > +						  ~BIT(old_crtc_state->cpu_transcoder));
> > 
> > I guess this part is redundant. Or can we somehow have our own
> > transcoder be included in sync_mode_slaves_mask/master_transcoder?
> >
> 
> Actually shouldnt it be:
> 
> if old_crtc_state->needs_modeset() {

That would need to be new_crtc_state. And yes we should skip this
(and also intel_modeset_tile_group()) when there's no modeset.

So the whole thing could probably look something like:
{
	intel_digital_connector_atomic_check();

	if (!intel_connector_needs_modeset(connector))
		return 0;

	if (tile)
		intel_modeset_tile_group()
	
	intel_modeset_synced_crtcs();
}

> 	 then call intel_modeset_affected_transcoders(state,
> 							(old_crtc_state->sync_mode_slaves_mask |
> 							 BIT(old_crtc_state->master_transcoder)) &
> 							~BIT(old_crtc_state->cpu_transcoder));
> 
> dont understand why ~BIT(old_crtc_state->cpu_transcoder) is redundant? why do we need to 
> force modeset there if we add this needs_modeset check?
> 
> Manasi
> 
> > > +}
> > > +
> > > +static int intel_dp_connector_atomic_check(struct drm_connector *conn,
> > > +					   struct drm_atomic_state *_state)
> > > +{
> > > +	struct drm_i915_private *dev_priv = to_i915(conn->dev);
> > > +	struct intel_atomic_state *state = to_intel_atomic_state(_state);
> > > +	int ret;
> > > +
> > > +	ret = intel_digital_connector_atomic_check(conn, &state->base);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	if (INTEL_GEN(dev_priv) >= 11 && conn->has_tile) {
> > > +		ret = intel_modeset_tile_group(state, conn->tile_group->id);
> > > +		if (ret)
> > > +			return ret;
> > > +	}
> > > +
> > > +	return intel_modeset_synced_crtcs(state, conn);
> > 
> > No gen check... Ah, yeah we don't need it because the port sync state
> > will be INVALID/0.
> > 
> > > +}
> > > +
> > >  static const struct drm_connector_funcs intel_dp_connector_funcs = {
> > >  	.force = intel_dp_force,
> > >  	.fill_modes = drm_helper_probe_single_connector_modes,
> > > @@ -6598,7 +6727,7 @@ static const struct drm_connector_helper_funcs intel_dp_connector_helper_funcs =
> > >  	.detect_ctx = intel_dp_detect,
> > >  	.get_modes = intel_dp_get_modes,
> > >  	.mode_valid = intel_dp_mode_valid,
> > > -	.atomic_check = intel_digital_connector_atomic_check,
> > > +	.atomic_check = intel_dp_connector_atomic_check,
> > >  };
> > >  
> > >  static const struct drm_encoder_funcs intel_dp_enc_funcs = {
> > > -- 
> > > 2.19.1
> > 
> > -- 
> > Ville Syrjälä
> > Intel

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

Re: [Intel-gfx] [PATCH 3/3] drm/i915/dp: Add all tiled and port sync conns to modeset

[Manasi Navare]

On Fri, Jan 31, 2020 at 10:05:03PM +0200, Ville Syrjälä wrote:
> On Fri, Jan 31, 2020 at 11:46:25AM -0800, Manasi Navare wrote:
> > On Fri, Jan 31, 2020 at 07:53:23PM +0200, Ville Syrjälä wrote:
> > > On Fri, Jan 31, 2020 at 09:15:47AM -0800, Manasi Navare wrote:
> > > > If one of the synced crtcs needs a full modeset, we need
> > > > to make sure all the synced crtcs are forced a full
> > > > modeset.
> > > > 
> > > > Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/display/intel_display.c |  88 +------------
> > > >  drivers/gpu/drm/i915/display/intel_dp.c      | 131 ++++++++++++++++++-
> > > >  2 files changed, 131 insertions(+), 88 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > > > index e638543f5f87..709a737638b6 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > > @@ -13123,8 +13123,7 @@ intel_modeset_pipe_config(struct intel_crtc_state *pipe_config)
> > > >  	struct drm_i915_private *i915 = to_i915(pipe_config->uapi.crtc->dev);
> > > >  	struct drm_connector *connector;
> > > >  	struct drm_connector_state *connector_state;
> > > > -	int base_bpp, ret;
> > > > -	int i, tile_group_id = -1, num_tiled_conns = 0;
> > > > +	int base_bpp, ret, i;
> > > >  	bool retry = true;
> > > >  
> > > >  	pipe_config->cpu_transcoder =
> > > > @@ -14559,76 +14558,6 @@ static bool intel_cpu_transcoders_need_modeset(struct intel_atomic_state *state,
> > > >  	return false;
> > > >  }
> > > >  
> > > > -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(crtc_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
> > > > @@ -14656,21 +14585,6 @@ static int intel_atomic_check(struct drm_device *dev,
> > > >  	if (ret)
> > > >  		goto fail;
> > > >  
> > > > -	/**
> > > > -	 * This check adds all the connectors in current state that belong to
> > > > -	 * the same tile group to a full modeset.
> > > > -	 * This function directly sets the mode_changed to true and we also call
> > > > -	 * drm_atomic_add_affected_connectors(). Hence we are not explicitly
> > > > -	 * calling drm_atomic_helper_check_modeset() after this.
> > > > -	 *
> > > > -	 * Fixme: Handle some corner cases where one of the
> > > > -	 * tiled connectors gets disconnected and tile info is lost but since it
> > > > -	 * was previously synced to other conn, we need to add that to the modeset.
> > > > -	 */
> > > > -	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)) {
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> > > > index f4dede6253f8..7eb4b3dbbcb3 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > > > @@ -6582,6 +6582,135 @@ void intel_dp_encoder_reset(struct drm_encoder *encoder)
> > > >  	}
> > > >  }
> > > >  
> > > > +static int intel_modeset_tile_group(struct intel_atomic_state *state,
> > > > +				    int tile_group_id)
> > > > +{
> > > > +	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> > > > +	struct drm_connector_list_iter conn_iter;
> > > > +	struct drm_connector *connector;
> > > > +	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 intel_crtc_state *crtc_state;
> > > > +		struct intel_crtc *crtc;
> > > > +
> > > > +		if (!connector->has_tile ||
> > > > +		    connector->tile_group->id != tile_group_id)
> > > > +			continue;
> > > > +
> > > > +		conn_state = drm_atomic_get_connector_state(&state->base,
> > > > +							    connector);
> > > > +		if (IS_ERR(conn_state)) {
> > > > +			ret = PTR_ERR(conn_state);
> > > > +			break;
> > > > +		}
> > > > +
> > > > +		crtc = to_intel_crtc(conn_state->crtc);
> > > > +
> > > > +		if (!crtc)
> > > > +			continue;
> > > > +
> > > > +		crtc_state = intel_atomic_get_crtc_state(&state->base, crtc);
> > > > +		if (IS_ERR(crtc_state)) {
> > > > +			ret = PTR_ERR(crtc_state);
> > > > +			break;
> > > > +		}
> > > > +
> > > > +		crtc_state->uapi.mode_changed = true;
> > > > +
> > > > +		ret = drm_atomic_add_affected_planes(&state->base, &crtc->base);
> > > > +		if (ret)
> > > > +			break;
> > > > +	}
> > > > +	drm_connector_list_iter_begin(&dev_priv->drm, &conn_iter);
> > > > +
> > > > +	return ret;
> > > > +}
> > > > +
> > > > +static int intel_modeset_affected_transcoders(struct intel_atomic_state *state, u8 transcoders)
> > > > +{
> > > > +	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> > > > +	struct intel_crtc *crtc;
> > > > +
> > > > +	if (transcoders == 0)
> > > > +		return 0;
> > > > +
> > > > +	for_each_intel_crtc(&dev_priv->drm, crtc) {
> > > > +		struct intel_crtc_state *crtc_state;
> > > > +		int ret;
> > > > +
> > > > +		if ((transcoders & BIT(crtc->pipe)) == 0)
> > > > +			continue;
> > > 
> > > Dropping the EDP transcoder on the floor here. I think we should just do
> > > the guaranteed correct thing and look at the cpu_transcoder instead.
> > > Yes, that does mean we more or less end up adding all crtcs to the state 
> > > whenever modesetting any synced crtc, but so be it. We can think of ways
> > > to optimize that later.
> > >
> > 
> > So should i not even do this check of trans & BIT(crtc->pipe) ? Instead just proceed
> > to go get the crtc state and then check if trans & BIT(crtc_state->cpu_trans) ?
> 
> Yeah, I think that's fine for now. We can try to optimize it later if
> needed.
> 
> > 
> > Or should I keep this as is and then add the second loop like in your branch?
> >  
> > > > +
> > > > +		crtc_state = intel_atomic_get_crtc_state(&state->base, crtc);
> > > > +		if (IS_ERR(crtc_state))
> > > > +			return PTR_ERR(crtc_state);
> > > > +
> > > > +		if (!crtc_state->hw.enable)
> > > > +			continue;
> > > > +
> > > > +		crtc_state->uapi.mode_changed = true;
> > > > +
> > > > +		ret = drm_atomic_add_affected_connectors(&state->base, &crtc->base);
> > > > +		if (ret)
> > > > +			return ret;
> > > 
> > > Missing add_affected_planes() here I think. Or was that guaranteed to be
> > > done by the helper? Can't recall.
> > >
> > 
> > No i think i missed it, will add that
> >  
> > > > +
> > > > +		WARN_ON((enum transcoder)crtc->pipe != crtc_state->cpu_transcoder);
> > > > +
> > > > +		transcoders &= ~BIT(crtc_state->cpu_transcoder);
> > > > +	}
> > > > +
> > > > +	WARN_ON(transcoders != 0);
> > > > +
> > > > +	return 0;
> > > > +
> > > > +}
> > > > +
> > > > +static int intel_modeset_synced_crtcs(struct intel_atomic_state *state,
> > > > +				      struct drm_connector *connector)
> > > > +{
> > > > +	const struct drm_connector_state *old_conn_state =
> > > > +		drm_atomic_get_old_connector_state(&state->base, connector);
> > > > +	const struct intel_crtc_state *old_crtc_state;
> > > > +	struct intel_crtc *crtc;
> > > > +
> > > > +	crtc = to_intel_crtc(old_conn_state->crtc);
> > > > +	if (!crtc)
> > > > +		return 0;
> > > > +
> > > > +	old_crtc_state = intel_atomic_get_old_crtc_state(state, crtc);
> > > > +
> > > > +	if (!old_crtc_state->hw.active)
> > > > +		return 0;
> > > > +
> > > > +	return intel_modeset_affected_transcoders(state,
> > > > +						  (old_crtc_state->sync_mode_slaves_mask |
> > > > +						   BIT(old_crtc_state->master_transcoder)) &
> > > 
> > > This seems to have the same master==INVALID problem that we faced
> > > elsewhere already.
> > >
> > 
> > Yes will have to add the same check and add it only for master_trans != INVALID, will add that
> >  
> > > > +						  ~BIT(old_crtc_state->cpu_transcoder));
> > > 
> > > I guess this part is redundant. Or can we somehow have our own
> > > transcoder be included in sync_mode_slaves_mask/master_transcoder?
> > >
> > 
> > Actually shouldnt it be:
> > 
> > if old_crtc_state->needs_modeset() {
> 
> That would need to be new_crtc_state. And yes we should skip this
> (and also intel_modeset_tile_group()) when there's no modeset.
> 
> So the whole thing could probably look something like:
> {
> 	intel_digital_connector_atomic_check();
> 
> 	if (!intel_connector_needs_modeset(connector))
> 		return 0;
> 
> 	if (tile)
> 		intel_modeset_tile_group()
> 	
> 	intel_modeset_synced_crtcs();
> }

Great yea this makes sense.
So then keep the ~BIT(old_crtc_state->cpu_transcoder)); right? so we dont forec modeset 
on the current crtc?

Manasi

> 
> > 	 then call intel_modeset_affected_transcoders(state,
> > 							(old_crtc_state->sync_mode_slaves_mask |
> > 							 BIT(old_crtc_state->master_transcoder)) &
> > 							~BIT(old_crtc_state->cpu_transcoder));
> > 
> > dont understand why ~BIT(old_crtc_state->cpu_transcoder) is redundant? why do we need to 
> > force modeset there if we add this needs_modeset check?
> > 
> > Manasi
> > 
> > > > +}
> > > > +
> > > > +static int intel_dp_connector_atomic_check(struct drm_connector *conn,
> > > > +					   struct drm_atomic_state *_state)
> > > > +{
> > > > +	struct drm_i915_private *dev_priv = to_i915(conn->dev);
> > > > +	struct intel_atomic_state *state = to_intel_atomic_state(_state);
> > > > +	int ret;
> > > > +
> > > > +	ret = intel_digital_connector_atomic_check(conn, &state->base);
> > > > +	if (ret)
> > > > +		return ret;
> > > > +
> > > > +	if (INTEL_GEN(dev_priv) >= 11 && conn->has_tile) {
> > > > +		ret = intel_modeset_tile_group(state, conn->tile_group->id);
> > > > +		if (ret)
> > > > +			return ret;
> > > > +	}
> > > > +
> > > > +	return intel_modeset_synced_crtcs(state, conn);
> > > 
> > > No gen check... Ah, yeah we don't need it because the port sync state
> > > will be INVALID/0.
> > > 
> > > > +}
> > > > +
> > > >  static const struct drm_connector_funcs intel_dp_connector_funcs = {
> > > >  	.force = intel_dp_force,
> > > >  	.fill_modes = drm_helper_probe_single_connector_modes,
> > > > @@ -6598,7 +6727,7 @@ static const struct drm_connector_helper_funcs intel_dp_connector_helper_funcs =
> > > >  	.detect_ctx = intel_dp_detect,
> > > >  	.get_modes = intel_dp_get_modes,
> > > >  	.mode_valid = intel_dp_mode_valid,
> > > > -	.atomic_check = intel_digital_connector_atomic_check,
> > > > +	.atomic_check = intel_dp_connector_atomic_check,
> > > >  };
> > > >  
> > > >  static const struct drm_encoder_funcs intel_dp_enc_funcs = {
> > > > -- 
> > > > 2.19.1
> > > 
> > > -- 
> > > Ville Syrjälä
> > > Intel
> 
> -- 
> Ville Syrjälä
> Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] ✗ Fi.CI.BUILD: failure for series starting with [1/3] drm/i915: Introduce encoder->compute_config_late()

[Patchwork]

== Series Details ==

Series: series starting with [1/3] drm/i915: Introduce encoder->compute_config_late()
URL   : https://patchwork.freedesktop.org/series/72836/
State : failure

== Summary ==

Applying: drm/i915: Introduce encoder->compute_config_late()
Applying: drm/i915/dp: Compute port sync crtc states post compute_config()
error: sha1 information is lacking or useless (drivers/gpu/drm/i915/display/intel_display.c).
error: could not build fake ancestor
hint: Use 'git am --show-current-patch' to see the failed patch
Patch failed at 0002 drm/i915/dp: Compute port sync crtc states post compute_config()
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

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

Re: [Intel-gfx] [PATCH 3/3] drm/i915/dp: Add all tiled and port sync conns to modeset

[Ville Syrjälä]

On Fri, Jan 31, 2020 at 12:54:52PM -0800, Manasi Navare wrote:
> On Fri, Jan 31, 2020 at 10:05:03PM +0200, Ville Syrjälä wrote:
> > On Fri, Jan 31, 2020 at 11:46:25AM -0800, Manasi Navare wrote:
> > > On Fri, Jan 31, 2020 at 07:53:23PM +0200, Ville Syrjälä wrote:
> > > > On Fri, Jan 31, 2020 at 09:15:47AM -0800, Manasi Navare wrote:
> > > > > If one of the synced crtcs needs a full modeset, we need
> > > > > to make sure all the synced crtcs are forced a full
> > > > > modeset.
> > > > > 
> > > > > Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > > > > ---
> > > > >  drivers/gpu/drm/i915/display/intel_display.c |  88 +------------
> > > > >  drivers/gpu/drm/i915/display/intel_dp.c      | 131 ++++++++++++++++++-
> > > > >  2 files changed, 131 insertions(+), 88 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > > > > index e638543f5f87..709a737638b6 100644
> > > > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > > > @@ -13123,8 +13123,7 @@ intel_modeset_pipe_config(struct intel_crtc_state *pipe_config)
> > > > >  	struct drm_i915_private *i915 = to_i915(pipe_config->uapi.crtc->dev);
> > > > >  	struct drm_connector *connector;
> > > > >  	struct drm_connector_state *connector_state;
> > > > > -	int base_bpp, ret;
> > > > > -	int i, tile_group_id = -1, num_tiled_conns = 0;
> > > > > +	int base_bpp, ret, i;
> > > > >  	bool retry = true;
> > > > >  
> > > > >  	pipe_config->cpu_transcoder =
> > > > > @@ -14559,76 +14558,6 @@ static bool intel_cpu_transcoders_need_modeset(struct intel_atomic_state *state,
> > > > >  	return false;
> > > > >  }
> > > > >  
> > > > > -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(crtc_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
> > > > > @@ -14656,21 +14585,6 @@ static int intel_atomic_check(struct drm_device *dev,
> > > > >  	if (ret)
> > > > >  		goto fail;
> > > > >  
> > > > > -	/**
> > > > > -	 * This check adds all the connectors in current state that belong to
> > > > > -	 * the same tile group to a full modeset.
> > > > > -	 * This function directly sets the mode_changed to true and we also call
> > > > > -	 * drm_atomic_add_affected_connectors(). Hence we are not explicitly
> > > > > -	 * calling drm_atomic_helper_check_modeset() after this.
> > > > > -	 *
> > > > > -	 * Fixme: Handle some corner cases where one of the
> > > > > -	 * tiled connectors gets disconnected and tile info is lost but since it
> > > > > -	 * was previously synced to other conn, we need to add that to the modeset.
> > > > > -	 */
> > > > > -	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)) {
> > > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> > > > > index f4dede6253f8..7eb4b3dbbcb3 100644
> > > > > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > > > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > > > > @@ -6582,6 +6582,135 @@ void intel_dp_encoder_reset(struct drm_encoder *encoder)
> > > > >  	}
> > > > >  }
> > > > >  
> > > > > +static int intel_modeset_tile_group(struct intel_atomic_state *state,
> > > > > +				    int tile_group_id)
> > > > > +{
> > > > > +	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> > > > > +	struct drm_connector_list_iter conn_iter;
> > > > > +	struct drm_connector *connector;
> > > > > +	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 intel_crtc_state *crtc_state;
> > > > > +		struct intel_crtc *crtc;
> > > > > +
> > > > > +		if (!connector->has_tile ||
> > > > > +		    connector->tile_group->id != tile_group_id)
> > > > > +			continue;
> > > > > +
> > > > > +		conn_state = drm_atomic_get_connector_state(&state->base,
> > > > > +							    connector);
> > > > > +		if (IS_ERR(conn_state)) {
> > > > > +			ret = PTR_ERR(conn_state);
> > > > > +			break;
> > > > > +		}
> > > > > +
> > > > > +		crtc = to_intel_crtc(conn_state->crtc);
> > > > > +
> > > > > +		if (!crtc)
> > > > > +			continue;
> > > > > +
> > > > > +		crtc_state = intel_atomic_get_crtc_state(&state->base, crtc);
> > > > > +		if (IS_ERR(crtc_state)) {
> > > > > +			ret = PTR_ERR(crtc_state);
> > > > > +			break;
> > > > > +		}
> > > > > +
> > > > > +		crtc_state->uapi.mode_changed = true;
> > > > > +
> > > > > +		ret = drm_atomic_add_affected_planes(&state->base, &crtc->base);
> > > > > +		if (ret)
> > > > > +			break;
> > > > > +	}
> > > > > +	drm_connector_list_iter_begin(&dev_priv->drm, &conn_iter);
> > > > > +
> > > > > +	return ret;
> > > > > +}
> > > > > +
> > > > > +static int intel_modeset_affected_transcoders(struct intel_atomic_state *state, u8 transcoders)
> > > > > +{
> > > > > +	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> > > > > +	struct intel_crtc *crtc;
> > > > > +
> > > > > +	if (transcoders == 0)
> > > > > +		return 0;
> > > > > +
> > > > > +	for_each_intel_crtc(&dev_priv->drm, crtc) {
> > > > > +		struct intel_crtc_state *crtc_state;
> > > > > +		int ret;
> > > > > +
> > > > > +		if ((transcoders & BIT(crtc->pipe)) == 0)
> > > > > +			continue;
> > > > 
> > > > Dropping the EDP transcoder on the floor here. I think we should just do
> > > > the guaranteed correct thing and look at the cpu_transcoder instead.
> > > > Yes, that does mean we more or less end up adding all crtcs to the state 
> > > > whenever modesetting any synced crtc, but so be it. We can think of ways
> > > > to optimize that later.
> > > >
> > > 
> > > So should i not even do this check of trans & BIT(crtc->pipe) ? Instead just proceed
> > > to go get the crtc state and then check if trans & BIT(crtc_state->cpu_trans) ?
> > 
> > Yeah, I think that's fine for now. We can try to optimize it later if
> > needed.
> > 
> > > 
> > > Or should I keep this as is and then add the second loop like in your branch?
> > >  
> > > > > +
> > > > > +		crtc_state = intel_atomic_get_crtc_state(&state->base, crtc);
> > > > > +		if (IS_ERR(crtc_state))
> > > > > +			return PTR_ERR(crtc_state);
> > > > > +
> > > > > +		if (!crtc_state->hw.enable)
> > > > > +			continue;
> > > > > +
> > > > > +		crtc_state->uapi.mode_changed = true;
> > > > > +
> > > > > +		ret = drm_atomic_add_affected_connectors(&state->base, &crtc->base);
> > > > > +		if (ret)
> > > > > +			return ret;
> > > > 
> > > > Missing add_affected_planes() here I think. Or was that guaranteed to be
> > > > done by the helper? Can't recall.
> > > >
> > > 
> > > No i think i missed it, will add that
> > >  
> > > > > +
> > > > > +		WARN_ON((enum transcoder)crtc->pipe != crtc_state->cpu_transcoder);
> > > > > +
> > > > > +		transcoders &= ~BIT(crtc_state->cpu_transcoder);
> > > > > +	}
> > > > > +
> > > > > +	WARN_ON(transcoders != 0);
> > > > > +
> > > > > +	return 0;
> > > > > +
> > > > > +}
> > > > > +
> > > > > +static int intel_modeset_synced_crtcs(struct intel_atomic_state *state,
> > > > > +				      struct drm_connector *connector)
> > > > > +{
> > > > > +	const struct drm_connector_state *old_conn_state =
> > > > > +		drm_atomic_get_old_connector_state(&state->base, connector);
> > > > > +	const struct intel_crtc_state *old_crtc_state;
> > > > > +	struct intel_crtc *crtc;
> > > > > +
> > > > > +	crtc = to_intel_crtc(old_conn_state->crtc);
> > > > > +	if (!crtc)
> > > > > +		return 0;
> > > > > +
> > > > > +	old_crtc_state = intel_atomic_get_old_crtc_state(state, crtc);
> > > > > +
> > > > > +	if (!old_crtc_state->hw.active)
> > > > > +		return 0;
> > > > > +
> > > > > +	return intel_modeset_affected_transcoders(state,
> > > > > +						  (old_crtc_state->sync_mode_slaves_mask |
> > > > > +						   BIT(old_crtc_state->master_transcoder)) &
> > > > 
> > > > This seems to have the same master==INVALID problem that we faced
> > > > elsewhere already.
> > > >
> > > 
> > > Yes will have to add the same check and add it only for master_trans != INVALID, will add that
> > >  
> > > > > +						  ~BIT(old_crtc_state->cpu_transcoder));
> > > > 
> > > > I guess this part is redundant. Or can we somehow have our own
> > > > transcoder be included in sync_mode_slaves_mask/master_transcoder?
> > > >
> > > 
> > > Actually shouldnt it be:
> > > 
> > > if old_crtc_state->needs_modeset() {
> > 
> > That would need to be new_crtc_state. And yes we should skip this
> > (and also intel_modeset_tile_group()) when there's no modeset.
> > 
> > So the whole thing could probably look something like:
> > {
> > 	intel_digital_connector_atomic_check();
> > 
> > 	if (!intel_connector_needs_modeset(connector))
> > 		return 0;
> > 
> > 	if (tile)
> > 		intel_modeset_tile_group()
> > 	
> > 	intel_modeset_synced_crtcs();
> > }
> 
> Great yea this makes sense.
> So then keep the ~BIT(old_crtc_state->cpu_transcoder)); right? so we dont forec modeset 
> on the current crtc?

intel_connector_needs_modeset() already told us the old crtc (if any)
needs a modeset. But even if it didn't cpu_transcoder still can't be 
part of master|slaves so still redundant.

> 
> Manasi
> 
> > 
> > > 	 then call intel_modeset_affected_transcoders(state,
> > > 							(old_crtc_state->sync_mode_slaves_mask |
> > > 							 BIT(old_crtc_state->master_transcoder)) &
> > > 							~BIT(old_crtc_state->cpu_transcoder));
> > > 
> > > dont understand why ~BIT(old_crtc_state->cpu_transcoder) is redundant? why do we need to 
> > > force modeset there if we add this needs_modeset check?
> > > 
> > > Manasi
> > > 
> > > > > +}
> > > > > +
> > > > > +static int intel_dp_connector_atomic_check(struct drm_connector *conn,
> > > > > +					   struct drm_atomic_state *_state)
> > > > > +{
> > > > > +	struct drm_i915_private *dev_priv = to_i915(conn->dev);
> > > > > +	struct intel_atomic_state *state = to_intel_atomic_state(_state);
> > > > > +	int ret;
> > > > > +
> > > > > +	ret = intel_digital_connector_atomic_check(conn, &state->base);
> > > > > +	if (ret)
> > > > > +		return ret;
> > > > > +
> > > > > +	if (INTEL_GEN(dev_priv) >= 11 && conn->has_tile) {
> > > > > +		ret = intel_modeset_tile_group(state, conn->tile_group->id);
> > > > > +		if (ret)
> > > > > +			return ret;
> > > > > +	}
> > > > > +
> > > > > +	return intel_modeset_synced_crtcs(state, conn);
> > > > 
> > > > No gen check... Ah, yeah we don't need it because the port sync state
> > > > will be INVALID/0.
> > > > 
> > > > > +}
> > > > > +
> > > > >  static const struct drm_connector_funcs intel_dp_connector_funcs = {
> > > > >  	.force = intel_dp_force,
> > > > >  	.fill_modes = drm_helper_probe_single_connector_modes,
> > > > > @@ -6598,7 +6727,7 @@ static const struct drm_connector_helper_funcs intel_dp_connector_helper_funcs =
> > > > >  	.detect_ctx = intel_dp_detect,
> > > > >  	.get_modes = intel_dp_get_modes,
> > > > >  	.mode_valid = intel_dp_mode_valid,
> > > > > -	.atomic_check = intel_digital_connector_atomic_check,
> > > > > +	.atomic_check = intel_dp_connector_atomic_check,
> > > > >  };
> > > > >  
> > > > >  static const struct drm_encoder_funcs intel_dp_enc_funcs = {
> > > > > -- 
> > > > > 2.19.1
> > > > 
> > > > -- 
> > > > Ville Syrjälä
> > > > Intel
> > 
> > -- 
> > Ville Syrjälä
> > Intel

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