[PATCH 01/10] drm: Do not call drm_dp_cec_set_edid() while registering DP connectors

[PATCH 01/10] drm: Do not call drm_dp_cec_set_edid() while registering DP connectors

[José Roberto de Souza]

drm_dp_cec_register_connector() is called when registering each DP
connector in DRM, while sounds a good idea register CEC adapters as
earlier as possible, it causes some driver initialization delay
trying to do DPCD transactions in disconnected connectors.

This change will cause no regressions as drm_dp_cec_set_edid() will
still be called in further detection of connected connectors with a
valid edid parameter.

This change reduced the module load of i915 by average 0.5sec in a
machine with just one DP port disconnected while reducing more than
3sec in a machine with 4 DP ports disconnected.

Cc: Hans Verkuil <hans.verkuil@cisco.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 drivers/gpu/drm/drm_dp_cec.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_dp_cec.c b/drivers/gpu/drm/drm_dp_cec.c
index 8a718f85079a..b15cee85b702 100644
--- a/drivers/gpu/drm/drm_dp_cec.c
+++ b/drivers/gpu/drm/drm_dp_cec.c
@@ -424,8 +424,6 @@ void drm_dp_cec_register_connector(struct drm_dp_aux *aux, const char *name,
 	aux->cec.parent = parent;
 	INIT_DELAYED_WORK(&aux->cec.unregister_work,
 			  drm_dp_cec_unregister_work);
-
-	drm_dp_cec_set_edid(aux, NULL);
 }
 EXPORT_SYMBOL(drm_dp_cec_register_connector);
 
-- 
2.19.0

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

[PATCH 02/10] drm/i915: Make intel_dp_detect() more clear to read

[José Roberto de Souza]

Moving all the error handling to the end of the function and ealier
returning for connected MST ports make this function way more easy to
read.
No functional changed is intended here.

Cc: Jani Nikula <jani.nikula@linux.intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 55 +++++++++++++++++++--------------
 1 file changed, 31 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 15a981ef5966..5a84a929bc7d 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -5077,28 +5077,20 @@ intel_dp_detect(struct drm_connector *connector,
 
 	intel_display_power_get(dev_priv, intel_dp->aux_power_domain);
 
-	/* Can't disconnect eDP */
+	/* Is port connected? eDP can't be disconnected */
+	if (!intel_dp_is_edp(intel_dp) &&
+	    !intel_digital_port_connected(encoder)) {
+		status = connector_status_disconnected;
+		goto port_disconnected;
+	}
+
 	if (intel_dp_is_edp(intel_dp))
 		status = edp_detect(intel_dp);
-	else if (intel_digital_port_connected(encoder))
-		status = intel_dp_detect_dpcd(intel_dp);
 	else
-		status = connector_status_disconnected;
-
-	if (status == connector_status_disconnected) {
-		memset(&intel_dp->compliance, 0, sizeof(intel_dp->compliance));
-
-		if (intel_dp->is_mst) {
-			DRM_DEBUG_KMS("MST device may have disappeared %d vs %d\n",
-				      intel_dp->is_mst,
-				      intel_dp->mst_mgr.mst_state);
-			intel_dp->is_mst = false;
-			drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr,
-							intel_dp->is_mst);
-		}
+		status = intel_dp_detect_dpcd(intel_dp);
 
-		goto out;
-	}
+	if (status == connector_status_disconnected)
+		goto port_not_detected;
 
 	if (intel_dp->reset_link_params) {
 		/* Initial max link lane count */
@@ -5123,8 +5115,8 @@ intel_dp_detect(struct drm_connector *connector,
 		 * won't appear connected or have anything
 		 * with EDID on it
 		 */
-		status = connector_status_disconnected;
-		goto out;
+		intel_display_power_put(dev_priv, intel_dp->aux_power_domain);
+		return connector_status_disconnected;
 	}
 
 	/*
@@ -5151,14 +5143,29 @@ intel_dp_detect(struct drm_connector *connector,
 	intel_dp->aux.i2c_defer_count = 0;
 
 	intel_dp_set_edid(intel_dp);
-	if (intel_dp_is_edp(intel_dp) ||
-	    to_intel_connector(connector)->detect_edid)
+	/*
+	 * intel_dp_detect_dpcd() will return connector_status_unknown for some
+	 * type of DP devices, if edid can be read set status as connected
+	 */
+	if (to_intel_connector(connector)->detect_edid)
 		status = connector_status_connected;
 
 	intel_dp_check_service_irq(intel_dp);
 
-out:
-	if (status != connector_status_connected && !intel_dp->is_mst)
+	intel_display_power_put(dev_priv, intel_dp->aux_power_domain);
+	return status;
+
+port_not_detected:
+port_disconnected:
+	memset(&intel_dp->compliance, 0, sizeof(intel_dp->compliance));
+
+	if (intel_dp->is_mst) {
+		DRM_DEBUG_KMS("MST device may have disappeared %d vs %d\n",
+			      intel_dp->is_mst, intel_dp->mst_mgr.mst_state);
+		intel_dp->is_mst = false;
+		drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr,
+						intel_dp->is_mst);
+	} else
 		intel_dp_unset_edid(intel_dp);
 
 	intel_display_power_put(dev_priv, intel_dp->aux_power_domain);
-- 
2.19.0

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

[PATCH 03/10] drm/i915: Do not get aux power for disconnected DP ports

[José Roberto de Souza]

For ICL type-c ports there is a aux power restriction, it can only be
enabled while there is sink connected.

BSpec: 21750

v2:
- rebased on top of the refactored version of intel_dp_detect()
- fixing CI errors by getting runtime_pm(), for VLV/CHV it is also
necessary get the DPIO power well reference

Cc: Jani Nikula <jani.nikula@linux.intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Imre Deak <imre.deak@intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 32 ++++++++++++++++++++++++++++----
 1 file changed, 28 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 5a84a929bc7d..2cd2dc564181 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4996,10 +4996,28 @@ bool intel_digital_port_connected(struct intel_encoder *encoder)
 	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
 
 	if (HAS_GMCH_DISPLAY(dev_priv)) {
+		enum intel_display_power_domain domain = 0;
+		bool ret;
+
+		/*
+		 * Oddly VLV/CHV needs the DPIO power well on to be able to
+		 * read the hotplug status, otherwise it will read a bogus value
+		 * and throw a unclaimed register warning
+		 */
+		if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
+			domain = intel_port_to_power_domain(encoder->port);
+			intel_display_power_get(dev_priv, domain);
+		}
+
 		if (IS_GM45(dev_priv))
-			return gm45_digital_port_connected(encoder);
+			ret = gm45_digital_port_connected(encoder);
 		else
-			return g4x_digital_port_connected(encoder);
+			ret = g4x_digital_port_connected(encoder);
+
+		if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
+			intel_display_power_put(dev_priv, domain);
+
+		return ret;
 	}
 
 	if (IS_GEN5(dev_priv))
@@ -5075,7 +5093,7 @@ intel_dp_detect(struct drm_connector *connector,
 		      connector->base.id, connector->name);
 	WARN_ON(!drm_modeset_is_locked(&dev_priv->drm.mode_config.connection_mutex));
 
-	intel_display_power_get(dev_priv, intel_dp->aux_power_domain);
+	intel_runtime_pm_get(dev_priv);
 
 	/* Is port connected? eDP can't be disconnected */
 	if (!intel_dp_is_edp(intel_dp) &&
@@ -5084,6 +5102,8 @@ intel_dp_detect(struct drm_connector *connector,
 		goto port_disconnected;
 	}
 
+	intel_display_power_get(dev_priv, intel_dp->aux_power_domain);
+
 	if (intel_dp_is_edp(intel_dp))
 		status = edp_detect(intel_dp);
 	else
@@ -5116,6 +5136,7 @@ intel_dp_detect(struct drm_connector *connector,
 		 * with EDID on it
 		 */
 		intel_display_power_put(dev_priv, intel_dp->aux_power_domain);
+		intel_runtime_pm_put(dev_priv);
 		return connector_status_disconnected;
 	}
 
@@ -5130,6 +5151,7 @@ intel_dp_detect(struct drm_connector *connector,
 		if (ret) {
 			intel_display_power_put(dev_priv,
 						intel_dp->aux_power_domain);
+			intel_runtime_pm_put(dev_priv);
 			return ret;
 		}
 	}
@@ -5153,9 +5175,11 @@ intel_dp_detect(struct drm_connector *connector,
 	intel_dp_check_service_irq(intel_dp);
 
 	intel_display_power_put(dev_priv, intel_dp->aux_power_domain);
+	intel_runtime_pm_put(dev_priv);
 	return status;
 
 port_not_detected:
+	intel_display_power_put(dev_priv, intel_dp->aux_power_domain);
 port_disconnected:
 	memset(&intel_dp->compliance, 0, sizeof(intel_dp->compliance));
 
@@ -5168,7 +5192,7 @@ intel_dp_detect(struct drm_connector *connector,
 	} else
 		intel_dp_unset_edid(intel_dp);
 
-	intel_display_power_put(dev_priv, intel_dp->aux_power_domain);
+	intel_runtime_pm_put(dev_priv);
 	return status;
 }
 
-- 
2.19.0

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

[PATCH 04/10] drm/i915/debugfs: Do not print cached information of a disconnected sink

[José Roberto de Souza]

Besides of give the expected output of i915_display_info it will also
avoid some aux ch transactions that would timeout by obvious reasons.

Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index f42e93b71e67..5786521cc2b0 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -3065,16 +3065,17 @@ static void intel_connector_info(struct seq_file *m,
 	seq_printf(m, "connector %d: type %s, status: %s\n",
 		   connector->base.id, connector->name,
 		   drm_get_connector_status_name(connector->status));
-	if (connector->status == connector_status_connected) {
-		seq_printf(m, "\tname: %s\n", connector->display_info.name);
-		seq_printf(m, "\tphysical dimensions: %dx%dmm\n",
-			   connector->display_info.width_mm,
-			   connector->display_info.height_mm);
-		seq_printf(m, "\tsubpixel order: %s\n",
-			   drm_get_subpixel_order_name(connector->display_info.subpixel_order));
-		seq_printf(m, "\tCEA rev: %d\n",
-			   connector->display_info.cea_rev);
-	}
+
+	if (connector->status == connector_status_disconnected)
+		return;
+
+	seq_printf(m, "\tname: %s\n", connector->display_info.name);
+	seq_printf(m, "\tphysical dimensions: %dx%dmm\n",
+		   connector->display_info.width_mm,
+		   connector->display_info.height_mm);
+	seq_printf(m, "\tsubpixel order: %s\n",
+		   drm_get_subpixel_order_name(connector->display_info.subpixel_order));
+	seq_printf(m, "\tCEA rev: %d\n", connector->display_info.cea_rev);
 
 	if (!intel_encoder)
 		return;
-- 
2.19.0

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

[PATCH 05/10] drm/i915: Do not try to set DPMS if sink is disconnected

[José Roberto de Souza]

intel_dp_sink_dpms() is also called in the DP port disconnection
flow, causing the DPCD transactions to fail due obvious reaons.
So lets check the connector state before waste any time trying
to do DPCD in a disconnected sink.

Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 2cd2dc564181..46ac603da91d 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -2818,6 +2818,10 @@ void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode)
 	if (intel_dp->dpcd[DP_DPCD_REV] < 0x11)
 		return;
 
+	if (intel_dp->attached_connector->base.status !=
+	    connector_status_connected)
+		return;
+
 	if (mode != DRM_MODE_DPMS_ON) {
 		if (downstream_hpd_needs_d0(intel_dp))
 			return;
-- 
2.19.0

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

[PATCH 06/10] drm/i915/icl: Mark TC port as safe when interruptions are disabled

[José Roberto de Souza]

Before enter in power saving states or before unload the driver
spec states that display driver is required to to mark TC ports as
safe so hardware can do the disconnection flow without wait for
display driver handshake.
When driver is resumed or loaded again, if the TC live state is
still set as connected driver will mark again TC port as not safe as
required by spec.

BSpec: 2175

Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 2e242270e270..58616690f45f 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -3640,6 +3640,7 @@ static void gen11_irq_reset(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	int pipe;
+	u32 val;
 
 	I915_WRITE(GEN11_GFX_MSTR_IRQ, 0);
 	POSTING_READ(GEN11_GFX_MSTR_IRQ);
@@ -3661,6 +3662,15 @@ static void gen11_irq_reset(struct drm_device *dev)
 
 	if (HAS_PCH_ICP(dev_priv))
 		GEN3_IRQ_RESET(SDE);
+
+	/*
+	 * Mark TC ports as safe so hardware can do the disconnect flow without
+	 * wait for driver to do the handshake
+	 */
+	val = I915_READ(PORT_TX_DFLEXDPCSSS);
+	for (pipe = 0; pipe < 4; pipe++)
+		val &= ~(DP_PHY_MODE_STATUS_NOT_SAFE(pipe));
+	I915_WRITE(PORT_TX_DFLEXDPCSSS, val);
 }
 
 void gen8_irq_power_well_post_enable(struct drm_i915_private *dev_priv,
-- 
2.19.0

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

[PATCH 07/10] drm/i915/icl: Set TC type to unknown in the disconnection flow

[José Roberto de Souza]

Otherwise it would be in a inconsistent state as port is disconnected
but with a valid tc type.

Also setting it to unknown will earlier return
icl_tc_phy_disconnect() for any future calls to
intel_digital_port_connected(), this way we don't need to check if
port is marked as safe everytime.

Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 46ac603da91d..88b5d1d0d46b 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4905,21 +4905,25 @@ static void icl_tc_phy_disconnect(struct drm_i915_private *dev_priv,
 				  struct intel_digital_port *dig_port)
 {
 	enum tc_port tc_port = intel_port_to_tc(dev_priv, dig_port->base.port);
-	u32 val;
 
-	if (dig_port->tc_type != TC_PORT_LEGACY &&
-	    dig_port->tc_type != TC_PORT_TYPEC)
+
+	if (dig_port->tc_type == TC_PORT_UNKNOWN)
 		return;
 
 	/*
-	 * This function may be called many times in a row without an HPD event
-	 * in between, so try to avoid the write when we can.
+	 * TBT disconnection flow is read the live status, what was done in
+	 * caller.
 	 */
-	val = I915_READ(PORT_TX_DFLEXDPCSSS);
-	if (val & DP_PHY_MODE_STATUS_NOT_SAFE(tc_port)) {
+	if (dig_port->tc_type == TC_PORT_TYPEC ||
+	    dig_port->tc_type == TC_PORT_LEGACY) {
+		u32 val;
+
+		val = I915_READ(PORT_TX_DFLEXDPCSSS);
 		val &= ~DP_PHY_MODE_STATUS_NOT_SAFE(tc_port);
 		I915_WRITE(PORT_TX_DFLEXDPCSSS, val);
 	}
+
+	dig_port->tc_type = TC_PORT_UNKNOWN;
 }
 
 /*
-- 
2.19.0

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

[PATCH 08/10] drm/i915/icl: Set TC type to unknown when a sudden disconnection happen

[José Roberto de Souza]

Otherwise it would be in a inconsistent state as port is disconnected
but with a valid tc type.

Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 88b5d1d0d46b..df7fff9776b9 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4834,6 +4834,9 @@ static void icl_update_tc_port_type(struct drm_i915_private *dev_priv,
 			      type_str);
 }
 
+static void icl_tc_phy_disconnect(struct drm_i915_private *dev_priv,
+				  struct intel_digital_port *dig_port);
+
 /*
  * This function implements the first part of the Connect Flow described by our
  * specification, Gen11 TypeC Programming chapter. The rest of the flow (reading
@@ -4888,9 +4891,7 @@ static bool icl_tc_phy_connect(struct drm_i915_private *dev_priv,
 	if (dig_port->tc_type == TC_PORT_TYPEC &&
 	    !(I915_READ(PORT_TX_DFLEXDPSP) & TC_LIVE_STATE_TC(tc_port))) {
 		DRM_DEBUG_KMS("TC PHY %d sudden disconnect.\n", tc_port);
-		val = I915_READ(PORT_TX_DFLEXDPCSSS);
-		val &= ~DP_PHY_MODE_STATUS_NOT_SAFE(tc_port);
-		I915_WRITE(PORT_TX_DFLEXDPCSSS, val);
+		icl_tc_phy_disconnect(dev_priv, dig_port);
 		return false;
 	}
 
-- 
2.19.0

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

[PATCH 09/10] drm/i915: Initialize panel_vdd_work only for eDP ports

[José Roberto de Souza]

It is only used by eDP ports so no need to initialize it for each DP
port.

Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index df7fff9776b9..3bc7e306b4a6 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -6442,6 +6442,8 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
 	if (!intel_dp_is_edp(intel_dp))
 		return true;
 
+	INIT_DELAYED_WORK(&intel_dp->panel_vdd_work, edp_panel_vdd_work);
+
 	/*
 	 * On IBX/CPT we may get here with LVDS already registered. Since the
 	 * driver uses the only internal power sequencer available for both
@@ -6648,9 +6650,6 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
 
 	intel_dp_aux_init(intel_dp);
 
-	INIT_DELAYED_WORK(&intel_dp->panel_vdd_work,
-			  edp_panel_vdd_work);
-
 	intel_connector_attach_encoder(intel_connector, intel_encoder);
 
 	if (HAS_DDI(dev_priv))
-- 
2.19.0

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

[PATCH 10/10] drm/i915/icl: Delay hotplug processing for tc ports

[José Roberto de Souza]

Some USB type-C dongles requires some time to power on before being
able to process aux channel transactions.
It was not a problem for older gens because there was a bridge
between DP port and USB-C controller adding some delay but ICL
handles type-C native.

So here trying to do a aux channel transaction at each 150ms for up 5
times, before giving up.

Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h      |  1 +
 drivers/gpu/drm/i915/intel_dp.c      |  7 ++++
 drivers/gpu/drm/i915/intel_drv.h     |  6 ++-
 drivers/gpu/drm/i915/intel_hotplug.c | 58 ++++++++++++++++++++++++++++
 4 files changed, 71 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 2264b30ce51a..293eac325a48 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2809,6 +2809,7 @@ enum hpd_pin intel_hpd_pin_default(struct drm_i915_private *dev_priv,
 				   enum port port);
 bool intel_hpd_disable(struct drm_i915_private *dev_priv, enum hpd_pin pin);
 void intel_hpd_enable(struct drm_i915_private *dev_priv, enum hpd_pin pin);
+void intel_hotplug_tc_wa_work(struct work_struct *__work);
 
 /* i915_irq.c */
 static inline void i915_queue_hangcheck(struct drm_i915_private *dev_priv)
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 3bc7e306b4a6..e3ce62fdf94f 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -5313,6 +5313,7 @@ void intel_dp_encoder_destroy(struct drm_encoder *encoder)
 {
 	struct intel_digital_port *intel_dig_port = enc_to_dig_port(encoder);
 	struct intel_dp *intel_dp = &intel_dig_port->dp;
+	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
 
 	intel_dp_mst_encoder_cleanup(intel_dig_port);
 	if (intel_dp_is_edp(intel_dp)) {
@@ -5329,6 +5330,8 @@ void intel_dp_encoder_destroy(struct drm_encoder *encoder)
 			unregister_reboot_notifier(&intel_dp->edp_notifier);
 			intel_dp->edp_notifier.notifier_call = NULL;
 		}
+	} else if (IS_ICELAKE(dev_priv)) {
+		cancel_delayed_work_sync(&intel_dp->tc_wa_work);
 	}
 
 	intel_dp_aux_fini(intel_dp);
@@ -6687,6 +6690,10 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
 		I915_WRITE(PEG_BAND_GAP_DATA, (temp & ~0xf) | 0xd);
 	}
 
+	if (IS_ICELAKE(dev_priv) && !intel_dp_is_edp(intel_dp))
+		INIT_DELAYED_WORK(&intel_dp->tc_wa_work,
+				  intel_hotplug_tc_wa_work);
+
 	return true;
 
 fail:
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 43190c6e9ef2..44f219088640 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1100,7 +1100,11 @@ struct intel_dp {
 	int panel_power_cycle_delay;
 	int backlight_on_delay;
 	int backlight_off_delay;
-	struct delayed_work panel_vdd_work;
+	union {
+		struct delayed_work panel_vdd_work;
+		struct delayed_work tc_wa_work;
+	};
+	u8 tc_wa_count;
 	bool want_panel_vdd;
 	unsigned long last_power_on;
 	unsigned long last_backlight_off;
diff --git a/drivers/gpu/drm/i915/intel_hotplug.c b/drivers/gpu/drm/i915/intel_hotplug.c
index 648a13c6043c..96546067f832 100644
--- a/drivers/gpu/drm/i915/intel_hotplug.c
+++ b/drivers/gpu/drm/i915/intel_hotplug.c
@@ -323,6 +323,45 @@ static void i915_digport_work_func(struct work_struct *work)
 	}
 }
 
+#define TC_WA_DELAY_MSEC 150
+#define TC_WA_TRIES 5
+
+void intel_hotplug_tc_wa_work(struct work_struct *__work)
+{
+	struct intel_dp *intel_dp = container_of(to_delayed_work(__work),
+						 struct intel_dp, tc_wa_work);
+	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
+	struct intel_encoder *intel_encoder = &intel_dig_port->base;
+	struct intel_connector *intel_connector = intel_dp->attached_connector;
+	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
+	struct drm_device *dev = &dev_priv->drm;
+	u8 val;
+
+	if (!intel_port_is_tc(dev_priv, intel_encoder->port) ||
+	    !intel_digital_port_connected(intel_encoder))
+		return;
+
+	if (drm_dp_dpcd_read(&intel_dp->aux, DP_DPCD_REV, &val, 1) < 1) {
+		intel_dp->tc_wa_count++;
+
+		if (intel_dp->tc_wa_count < TC_WA_TRIES) {
+			unsigned long delay;
+
+			delay = msecs_to_jiffies(TC_WA_DELAY_MSEC);
+			schedule_delayed_work(&intel_dp->tc_wa_work, delay);
+		} else {
+			DRM_DEBUG_KMS("TC not responsive, giving up\n");
+		}
+	} else {
+		mutex_lock(&dev->mode_config.mutex);
+		val = intel_encoder->hotplug(intel_encoder, intel_connector);
+		mutex_unlock(&dev->mode_config.mutex);
+
+		if (val)
+			drm_kms_helper_hotplug_event(dev);
+	}
+}
+
 /*
  * Handle hotplug events outside the interrupt handler proper.
  */
@@ -361,6 +400,25 @@ static void i915_hotplug_work_func(struct work_struct *work)
 			DRM_DEBUG_KMS("Connector %s (pin %i) received hotplug event.\n",
 				      connector->name, intel_encoder->hpd_pin);
 
+			/*
+			 * TC WA: TC dongles takes some type to be
+			 * responsible
+			 */
+			if (IS_ICELAKE(dev_priv) &&
+			    intel_port_is_tc(dev_priv, intel_encoder->port) &&
+			    intel_digital_port_connected(intel_encoder)) {
+				struct intel_dp *intel_dp;
+				unsigned long delay;
+
+				intel_dp = enc_to_intel_dp(&intel_encoder->base);
+
+				intel_dp->tc_wa_count = 0;
+				delay = msecs_to_jiffies(TC_WA_DELAY_MSEC);
+				schedule_delayed_work(&intel_dp->tc_wa_work,
+						      delay);
+				continue;
+			}
+
 			changed |= intel_encoder->hotplug(intel_encoder,
 							  intel_connector);
 		}
-- 
2.19.0

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

✗ Fi.CI.CHECKPATCH: warning for series starting with [01/10] drm: Do not call drm_dp_cec_set_edid() while registering DP connectors

[Patchwork]

== Series Details ==

Series: series starting with [01/10] drm: Do not call drm_dp_cec_set_edid() while registering DP connectors
URL   : https://patchwork.freedesktop.org/series/50456/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
c5f04ccd7a0a drm: Do not call drm_dp_cec_set_edid() while registering DP connectors
b95b31b5d5a4 drm/i915: Make intel_dp_detect() more clear to read
-:97: CHECK:BRACES: braces {} should be used on all arms of this statement
#97: FILE: drivers/gpu/drm/i915/intel_dp.c:5162:
+	if (intel_dp->is_mst) {
[...]
+	} else
[...]

-:103: CHECK:BRACES: Unbalanced braces around else statement
#103: FILE: drivers/gpu/drm/i915/intel_dp.c:5168:
+	} else

total: 0 errors, 0 warnings, 2 checks, 81 lines checked
53c3cdbd6585 drm/i915: Do not get aux power for disconnected DP ports
a483ebea3311 drm/i915/debugfs: Do not print cached information of a disconnected sink
5213b4214ad0 drm/i915: Do not try to set DPMS if sink is disconnected
b0f833c0cfdd drm/i915/icl: Mark TC port as safe when interruptions are disabled
68417a6aa3e4 drm/i915/icl: Set TC type to unknown in the disconnection flow
-:33: CHECK:LINE_SPACING: Please don't use multiple blank lines
#33: FILE: drivers/gpu/drm/i915/intel_dp.c:4909:
 
+

total: 0 errors, 0 warnings, 1 checks, 32 lines checked
7e577060a48e drm/i915/icl: Set TC type to unknown when a sudden disconnection happen
85f6efd86104 drm/i915: Initialize panel_vdd_work only for eDP ports
74612a1286e9 drm/i915/icl: Delay hotplug processing for tc ports

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

✗ Fi.CI.SPARSE: warning for series starting with [01/10] drm: Do not call drm_dp_cec_set_edid() while registering DP connectors

[Patchwork]

== Series Details ==

Series: series starting with [01/10] drm: Do not call drm_dp_cec_set_edid() while registering DP connectors
URL   : https://patchwork.freedesktop.org/series/50456/
State : warning

== Summary ==

$ dim sparse origin/drm-tip
Commit: drm: Do not call drm_dp_cec_set_edid() while registering DP connectors
Okay!

Commit: drm/i915: Make intel_dp_detect() more clear to read
Okay!

Commit: drm/i915: Do not get aux power for disconnected DP ports
Okay!

Commit: drm/i915/debugfs: Do not print cached information of a disconnected sink
Okay!

Commit: drm/i915: Do not try to set DPMS if sink is disconnected
Okay!

Commit: drm/i915/icl: Mark TC port as safe when interruptions are disabled
Okay!

Commit: drm/i915/icl: Set TC type to unknown in the disconnection flow
Okay!

Commit: drm/i915/icl: Set TC type to unknown when a sudden disconnection happen
Okay!

Commit: drm/i915: Initialize panel_vdd_work only for eDP ports
Okay!

Commit: drm/i915/icl: Delay hotplug processing for tc ports
-drivers/gpu/drm/i915/selftests/../i915_drv.h:3726:16: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/selftests/../i915_drv.h:3727:16: warning: expression using sizeof(void)

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

✓ Fi.CI.BAT: success for series starting with [01/10] drm: Do not call drm_dp_cec_set_edid() while registering DP connectors

[Patchwork]

== Series Details ==

Series: series starting with [01/10] drm: Do not call drm_dp_cec_set_edid() while registering DP connectors
URL   : https://patchwork.freedesktop.org/series/50456/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4915 -> Patchwork_10328 =

== Summary - WARNING ==

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

  External URL: https://patchwork.freedesktop.org/api/1.0/series/50456/revisions/1/mbox/

== Possible new issues ==

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

  === IGT changes ===

    ==== Warnings ====

    igt@drv_selftest@live_guc:
      fi-glk-j4005:       SKIP -> PASS

    
== Known issues ==

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

  === IGT changes ===

    ==== Issues hit ====

    igt@gem_exec_suspend@basic-s3:
      fi-blb-e6850:       PASS -> INCOMPLETE (fdo#107718)

    
    ==== Possible fixes ====

    igt@drv_selftest@live_coherency:
      fi-gdg-551:         DMESG-FAIL (fdo#107164) -> PASS

    igt@drv_selftest@live_execlists:
      fi-glk-j4005:       INCOMPLETE (k.org#198133, fdo#103359) -> PASS

    igt@kms_pipe_crc_basic@read-crc-pipe-a-frame-sequence:
      fi-byt-clapper:     FAIL (fdo#107362, fdo#103191) -> PASS

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-b:
      fi-skl-guc:         FAIL (fdo#103191) -> PASS

    
  fdo#103191 https://bugs.freedesktop.org/show_bug.cgi?id=103191
  fdo#103359 https://bugs.freedesktop.org/show_bug.cgi?id=103359
  fdo#107164 https://bugs.freedesktop.org/show_bug.cgi?id=107164
  fdo#107362 https://bugs.freedesktop.org/show_bug.cgi?id=107362
  fdo#107718 https://bugs.freedesktop.org/show_bug.cgi?id=107718
  k.org#198133 https://bugzilla.kernel.org/show_bug.cgi?id=198133


== Participating hosts (46 -> 43) ==

  Missing    (3): fi-bsw-cyan fi-byt-squawks fi-icl-u2 


== Build changes ==

    * Linux: CI_DRM_4915 -> Patchwork_10328

  CI_DRM_4915: 26e7a7d954a9c28b97af8ca7813f430fd9117232 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4660: d0975646c50568e66e65b44b81d28232d059b94e @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_10328: 74612a1286e9756d22b434d21fbf4607da498684 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

74612a1286e9 drm/i915/icl: Delay hotplug processing for tc ports
85f6efd86104 drm/i915: Initialize panel_vdd_work only for eDP ports
7e577060a48e drm/i915/icl: Set TC type to unknown when a sudden disconnection happen
68417a6aa3e4 drm/i915/icl: Set TC type to unknown in the disconnection flow
b0f833c0cfdd drm/i915/icl: Mark TC port as safe when interruptions are disabled
5213b4214ad0 drm/i915: Do not try to set DPMS if sink is disconnected
a483ebea3311 drm/i915/debugfs: Do not print cached information of a disconnected sink
53c3cdbd6585 drm/i915: Do not get aux power for disconnected DP ports
b95b31b5d5a4 drm/i915: Make intel_dp_detect() more clear to read
c5f04ccd7a0a drm: Do not call drm_dp_cec_set_edid() while registering DP connectors

== Logs ==

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

Re: [PATCH 03/10] drm/i915: Do not get aux power for disconnected DP ports

[Ville Syrjälä]

On Tue, Oct 02, 2018 at 10:50:47AM -0700, José Roberto de Souza wrote:
> For ICL type-c ports there is a aux power restriction, it can only be
> enabled while there is sink connected.
> 
> BSpec: 21750
> 
> v2:
> - rebased on top of the refactored version of intel_dp_detect()
> - fixing CI errors by getting runtime_pm(), for VLV/CHV it is also
> necessary get the DPIO power well reference

The hpd logic is in the disp2d well.

> 
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Imre Deak <imre.deak@intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 32 ++++++++++++++++++++++++++++----
>  1 file changed, 28 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 5a84a929bc7d..2cd2dc564181 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -4996,10 +4996,28 @@ bool intel_digital_port_connected(struct intel_encoder *encoder)
>  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
>  
>  	if (HAS_GMCH_DISPLAY(dev_priv)) {
> +		enum intel_display_power_domain domain = 0;
> +		bool ret;
> +
> +		/*
> +		 * Oddly VLV/CHV needs the DPIO power well on to be able to
> +		 * read the hotplug status, otherwise it will read a bogus value
> +		 * and throw a unclaimed register warning
> +		 */
> +		if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
> +			domain = intel_port_to_power_domain(encoder->port);
> +			intel_display_power_get(dev_priv, domain);
> +		}
> +
>  		if (IS_GM45(dev_priv))
> -			return gm45_digital_port_connected(encoder);
> +			ret = gm45_digital_port_connected(encoder);
>  		else
> -			return g4x_digital_port_connected(encoder);
> +			ret = g4x_digital_port_connected(encoder);
> +
> +		if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> +			intel_display_power_put(dev_priv, domain);
> +
> +		return ret;
>  	}
>  
>  	if (IS_GEN5(dev_priv))
> @@ -5075,7 +5093,7 @@ intel_dp_detect(struct drm_connector *connector,
>  		      connector->base.id, connector->name);
>  	WARN_ON(!drm_modeset_is_locked(&dev_priv->drm.mode_config.connection_mutex));
>  
> -	intel_display_power_get(dev_priv, intel_dp->aux_power_domain);
> +	intel_runtime_pm_get(dev_priv);
>  
>  	/* Is port connected? eDP can't be disconnected */
>  	if (!intel_dp_is_edp(intel_dp) &&
> @@ -5084,6 +5102,8 @@ intel_dp_detect(struct drm_connector *connector,
>  		goto port_disconnected;
>  	}
>  
> +	intel_display_power_get(dev_priv, intel_dp->aux_power_domain);
> +
>  	if (intel_dp_is_edp(intel_dp))
>  		status = edp_detect(intel_dp);
>  	else
> @@ -5116,6 +5136,7 @@ intel_dp_detect(struct drm_connector *connector,
>  		 * with EDID on it
>  		 */
>  		intel_display_power_put(dev_priv, intel_dp->aux_power_domain);
> +		intel_runtime_pm_put(dev_priv);
>  		return connector_status_disconnected;
>  	}
>  
> @@ -5130,6 +5151,7 @@ intel_dp_detect(struct drm_connector *connector,
>  		if (ret) {
>  			intel_display_power_put(dev_priv,
>  						intel_dp->aux_power_domain);
> +			intel_runtime_pm_put(dev_priv);
>  			return ret;
>  		}
>  	}
> @@ -5153,9 +5175,11 @@ intel_dp_detect(struct drm_connector *connector,
>  	intel_dp_check_service_irq(intel_dp);
>  
>  	intel_display_power_put(dev_priv, intel_dp->aux_power_domain);
> +	intel_runtime_pm_put(dev_priv);
>  	return status;
>  
>  port_not_detected:
> +	intel_display_power_put(dev_priv, intel_dp->aux_power_domain);
>  port_disconnected:
>  	memset(&intel_dp->compliance, 0, sizeof(intel_dp->compliance));
>  
> @@ -5168,7 +5192,7 @@ intel_dp_detect(struct drm_connector *connector,
>  	} else
>  		intel_dp_unset_edid(intel_dp);
>  
> -	intel_display_power_put(dev_priv, intel_dp->aux_power_domain);
> +	intel_runtime_pm_put(dev_priv);
>  	return status;
>  }
>  
> -- 
> 2.19.0

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

Re: [PATCH 05/10] drm/i915: Do not try to set DPMS if sink is disconnected

[Ville Syrjälä]

On Tue, Oct 02, 2018 at 10:50:49AM -0700, José Roberto de Souza wrote:
> intel_dp_sink_dpms() is also called in the DP port disconnection
> flow, causing the DPCD transactions to fail due obvious reaons.
> So lets check the connector state before waste any time trying
> to do DPCD in a disconnected sink.
> 
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 2cd2dc564181..46ac603da91d 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -2818,6 +2818,10 @@ void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode)
>  	if (intel_dp->dpcd[DP_DPCD_REV] < 0x11)
>  		return;
>  
> +	if (intel_dp->attached_connector->base.status !=
> +	    connector_status_connected)
> +		return;
> +

The user can stil yank the cable out at any time. So I don't see much 
point in trying to avoid this stuff. IMO better go through with the 
whole thing as normal rather than adding exceptions for things we
can't even control.

>  	if (mode != DRM_MODE_DPMS_ON) {
>  		if (downstream_hpd_needs_d0(intel_dp))
>  			return;
> -- 
> 2.19.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH 06/10] drm/i915/icl: Mark TC port as safe when interruptions are disabled

[Ville Syrjälä]

On Tue, Oct 02, 2018 at 10:50:50AM -0700, José Roberto de Souza wrote:
> Before enter in power saving states or before unload the driver
> spec states that display driver is required to to mark TC ports as
> safe so hardware can do the disconnection flow without wait for
> display driver handshake.
> When driver is resumed or loaded again, if the TC live state is
> still set as connected driver will mark again TC port as not safe as
> required by spec.
> 
> BSpec: 2175
> 
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_irq.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 2e242270e270..58616690f45f 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -3640,6 +3640,7 @@ static void gen11_irq_reset(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	int pipe;
> +	u32 val;
>  
>  	I915_WRITE(GEN11_GFX_MSTR_IRQ, 0);
>  	POSTING_READ(GEN11_GFX_MSTR_IRQ);
> @@ -3661,6 +3662,15 @@ static void gen11_irq_reset(struct drm_device *dev)
>  
>  	if (HAS_PCH_ICP(dev_priv))
>  		GEN3_IRQ_RESET(SDE);
> +
> +	/*
> +	 * Mark TC ports as safe so hardware can do the disconnect flow without
> +	 * wait for driver to do the handshake
> +	 */
> +	val = I915_READ(PORT_TX_DFLEXDPCSSS);
> +	for (pipe = 0; pipe < 4; pipe++)
> +		val &= ~(DP_PHY_MODE_STATUS_NOT_SAFE(pipe));
> +	I915_WRITE(PORT_TX_DFLEXDPCSSS, val);

Why would we have to do this here? The driver should relinquish control
if and when it has shut down the pipes etc. Sounds like a bug if we're
hanging on when we have no need for the port.

>  }
>  
>  void gen8_irq_power_well_post_enable(struct drm_i915_private *dev_priv,
> -- 
> 2.19.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH 03/10] drm/i915: Do not get aux power for disconnected DP ports

[Ville Syrjälä]

On Tue, Oct 02, 2018 at 10:50:47AM -0700, José Roberto de Souza wrote:
> For ICL type-c ports there is a aux power restriction, it can only be
> enabled while there is sink connected.
> 
> BSpec: 21750
> 
> v2:
> - rebased on top of the refactored version of intel_dp_detect()
> - fixing CI errors by getting runtime_pm(), for VLV/CHV it is also
> necessary get the DPIO power well reference
> 
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Imre Deak <imre.deak@intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 32 ++++++++++++++++++++++++++++----
>  1 file changed, 28 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 5a84a929bc7d..2cd2dc564181 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -4996,10 +4996,28 @@ bool intel_digital_port_connected(struct intel_encoder *encoder)
>  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
>  
>  	if (HAS_GMCH_DISPLAY(dev_priv)) {
> +		enum intel_display_power_domain domain = 0;
> +		bool ret;
> +
> +		/*
> +		 * Oddly VLV/CHV needs the DPIO power well on to be able to
> +		 * read the hotplug status, otherwise it will read a bogus value
> +		 * and throw a unclaimed register warning
> +		 */
> +		if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
> +			domain = intel_port_to_power_domain(encoder->port);
> +			intel_display_power_get(dev_priv, domain);
> +		}
> +
>  		if (IS_GM45(dev_priv))
> -			return gm45_digital_port_connected(encoder);
> +			ret = gm45_digital_port_connected(encoder);
>  		else
> -			return g4x_digital_port_connected(encoder);
> +			ret = g4x_digital_port_connected(encoder);
> +
> +		if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> +			intel_display_power_put(dev_priv, domain);
> +
> +		return ret;
>  	}
>  
>  	if (IS_GEN5(dev_priv))
> @@ -5075,7 +5093,7 @@ intel_dp_detect(struct drm_connector *connector,
>  		      connector->base.id, connector->name);
>  	WARN_ON(!drm_modeset_is_locked(&dev_priv->drm.mode_config.connection_mutex));
>  
> -	intel_display_power_get(dev_priv, intel_dp->aux_power_domain);
> +	intel_runtime_pm_get(dev_priv);

I still maintain that it can't be the "is there a sink or not?" thing
that makes this fail. The only thing that makes sense to me is the
port tbt vs. tc (or whatever it was) knob. Otherwise we can race
with display hotplug all day long.

Also this is hardly the only place where we grab the power reference
so we'd need the duct tape applied quite a bit more liberally if we
wanted it to work.

So there seems to be a fundemental problem with the whole tbt vs. tc
mode thing in the current code. We should fix that before we add
piles of duct tape:
- correctly track which mode the port is used in
- probably split the connector into tbt vs. tc variants (to make it
  clear which mode is being used)
- make sure it stays in that mode until current users are done
- block/reject other uses of the port while it's in the wrong mode
- do the approriate flows when acquiring/reqlinguishing control
  of the port
- test the heck out of it with eg. genconnector, forced connctor
  status, /dev/aux thing, i2c, etc.

>  
>  	/* Is port connected? eDP can't be disconnected */
>  	if (!intel_dp_is_edp(intel_dp) &&
> @@ -5084,6 +5102,8 @@ intel_dp_detect(struct drm_connector *connector,
>  		goto port_disconnected;
>  	}
>  
> +	intel_display_power_get(dev_priv, intel_dp->aux_power_domain);
> +
>  	if (intel_dp_is_edp(intel_dp))
>  		status = edp_detect(intel_dp);
>  	else
> @@ -5116,6 +5136,7 @@ intel_dp_detect(struct drm_connector *connector,
>  		 * with EDID on it
>  		 */
>  		intel_display_power_put(dev_priv, intel_dp->aux_power_domain);
> +		intel_runtime_pm_put(dev_priv);
>  		return connector_status_disconnected;
>  	}
>  
> @@ -5130,6 +5151,7 @@ intel_dp_detect(struct drm_connector *connector,
>  		if (ret) {
>  			intel_display_power_put(dev_priv,
>  						intel_dp->aux_power_domain);
> +			intel_runtime_pm_put(dev_priv);
>  			return ret;
>  		}
>  	}
> @@ -5153,9 +5175,11 @@ intel_dp_detect(struct drm_connector *connector,
>  	intel_dp_check_service_irq(intel_dp);
>  
>  	intel_display_power_put(dev_priv, intel_dp->aux_power_domain);
> +	intel_runtime_pm_put(dev_priv);
>  	return status;
>  
>  port_not_detected:
> +	intel_display_power_put(dev_priv, intel_dp->aux_power_domain);
>  port_disconnected:
>  	memset(&intel_dp->compliance, 0, sizeof(intel_dp->compliance));
>  
> @@ -5168,7 +5192,7 @@ intel_dp_detect(struct drm_connector *connector,
>  	} else
>  		intel_dp_unset_edid(intel_dp);
>  
> -	intel_display_power_put(dev_priv, intel_dp->aux_power_domain);
> +	intel_runtime_pm_put(dev_priv);
>  	return status;
>  }
>  
> -- 
> 2.19.0

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

Re: [PATCH 02/10] drm/i915: Make intel_dp_detect() more clear to read

[Jani Nikula]

On Tue, 02 Oct 2018, José Roberto de Souza <jose.souza@intel.com> wrote:
> Moving all the error handling to the end of the function and ealier
> returning for connected MST ports make this function way more easy to
> read.
> No functional changed is intended here.

Honestly, I disagree that the end result would be more clear to
read. I'm all for improving clarity, but there does have to be a clear
improvement to justify changing this.

I'm especially suspicious of duplicating the intel_display_power_put()
calls and the returns. It sets a precedent to duplicate them again, and
someone's bound to forget to put.

BR,
Jani,



>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 55 +++++++++++++++++++--------------
>  1 file changed, 31 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 15a981ef5966..5a84a929bc7d 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -5077,28 +5077,20 @@ intel_dp_detect(struct drm_connector *connector,
>  
>  	intel_display_power_get(dev_priv, intel_dp->aux_power_domain);
>  
> -	/* Can't disconnect eDP */
> +	/* Is port connected? eDP can't be disconnected */
> +	if (!intel_dp_is_edp(intel_dp) &&
> +	    !intel_digital_port_connected(encoder)) {
> +		status = connector_status_disconnected;
> +		goto port_disconnected;
> +	}
> +
>  	if (intel_dp_is_edp(intel_dp))
>  		status = edp_detect(intel_dp);
> -	else if (intel_digital_port_connected(encoder))
> -		status = intel_dp_detect_dpcd(intel_dp);
>  	else
> -		status = connector_status_disconnected;
> -
> -	if (status == connector_status_disconnected) {
> -		memset(&intel_dp->compliance, 0, sizeof(intel_dp->compliance));
> -
> -		if (intel_dp->is_mst) {
> -			DRM_DEBUG_KMS("MST device may have disappeared %d vs %d\n",
> -				      intel_dp->is_mst,
> -				      intel_dp->mst_mgr.mst_state);
> -			intel_dp->is_mst = false;
> -			drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr,
> -							intel_dp->is_mst);
> -		}
> +		status = intel_dp_detect_dpcd(intel_dp);
>  
> -		goto out;
> -	}
> +	if (status == connector_status_disconnected)
> +		goto port_not_detected;
>  
>  	if (intel_dp->reset_link_params) {
>  		/* Initial max link lane count */
> @@ -5123,8 +5115,8 @@ intel_dp_detect(struct drm_connector *connector,
>  		 * won't appear connected or have anything
>  		 * with EDID on it
>  		 */
> -		status = connector_status_disconnected;
> -		goto out;
> +		intel_display_power_put(dev_priv, intel_dp->aux_power_domain);
> +		return connector_status_disconnected;
>  	}
>  
>  	/*
> @@ -5151,14 +5143,29 @@ intel_dp_detect(struct drm_connector *connector,
>  	intel_dp->aux.i2c_defer_count = 0;
>  
>  	intel_dp_set_edid(intel_dp);
> -	if (intel_dp_is_edp(intel_dp) ||
> -	    to_intel_connector(connector)->detect_edid)
> +	/*
> +	 * intel_dp_detect_dpcd() will return connector_status_unknown for some
> +	 * type of DP devices, if edid can be read set status as connected
> +	 */
> +	if (to_intel_connector(connector)->detect_edid)
>  		status = connector_status_connected;
>  
>  	intel_dp_check_service_irq(intel_dp);
>  
> -out:
> -	if (status != connector_status_connected && !intel_dp->is_mst)
> +	intel_display_power_put(dev_priv, intel_dp->aux_power_domain);
> +	return status;
> +
> +port_not_detected:
> +port_disconnected:
> +	memset(&intel_dp->compliance, 0, sizeof(intel_dp->compliance));
> +
> +	if (intel_dp->is_mst) {
> +		DRM_DEBUG_KMS("MST device may have disappeared %d vs %d\n",
> +			      intel_dp->is_mst, intel_dp->mst_mgr.mst_state);
> +		intel_dp->is_mst = false;
> +		drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr,
> +						intel_dp->is_mst);
> +	} else
>  		intel_dp_unset_edid(intel_dp);
>  
>  	intel_display_power_put(dev_priv, intel_dp->aux_power_domain);

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

✗ Fi.CI.IGT: failure for series starting with [01/10] drm: Do not call drm_dp_cec_set_edid() while registering DP connectors

[Patchwork]

== Series Details ==

Series: series starting with [01/10] drm: Do not call drm_dp_cec_set_edid() while registering DP connectors
URL   : https://patchwork.freedesktop.org/series/50456/
State : failure

== Summary ==

= CI Bug Log - changes from CI_DRM_4915_full -> Patchwork_10328_full =

== Summary - FAILURE ==

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

  

== Possible new issues ==

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

  === IGT changes ===

    ==== Possible regressions ====

    igt@kms_color@pipe-c-degamma:
      shard-apl:          PASS -> FAIL

    {igt@kms_plane_alpha_blend@pipe-b-coverage-7efc}:
      shard-skl:          NOTRUN -> FAIL

    
    ==== Warnings ====

    igt@pm_rc6_residency@rc6-accuracy:
      shard-kbl:          PASS -> SKIP
      shard-snb:          SKIP -> PASS

    
== Known issues ==

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

  === IGT changes ===

    ==== Issues hit ====

    igt@drv_suspend@shrink:
      shard-snb:          PASS -> INCOMPLETE (fdo#105411, fdo#106886)

    igt@gem_exec_schedule@pi-ringfull-bsd:
      shard-skl:          NOTRUN -> FAIL (fdo#103158)

    igt@gem_softpin@noreloc-s3:
      shard-skl:          PASS -> INCOMPLETE (fdo#104108, fdo#107773)

    igt@kms_busy@extended-pageflip-modeset-hang-oldfb-render-c:
      shard-skl:          NOTRUN -> DMESG-WARN (fdo#107956)

    igt@kms_cursor_crc@cursor-64x21-random:
      shard-apl:          PASS -> FAIL (fdo#103232) +2

    igt@kms_draw_crc@draw-method-rgb565-mmap-wc-xtiled:
      shard-skl:          PASS -> FAIL (fdo#103184)

    igt@kms_frontbuffer_tracking@fbc-1p-primscrn-cur-indfb-draw-pwrite:
      shard-apl:          PASS -> FAIL (fdo#103167) +3

    igt@kms_plane@pixel-format-pipe-a-planes:
      shard-apl:          PASS -> FAIL (fdo#103166)

    igt@kms_plane@plane-panning-bottom-right-suspend-pipe-a-planes:
      shard-kbl:          PASS -> INCOMPLETE (fdo#103665)

    igt@pm_rpm@dpms-mode-unset-non-lpsp:
      shard-skl:          SKIP -> INCOMPLETE (fdo#107807)

    
    ==== Possible fixes ====

    igt@kms_ccs@pipe-b-missing-ccs-buffer:
      shard-kbl:          DMESG-WARN (fdo#105602, fdo#103558) -> PASS +14

    igt@kms_cursor_crc@cursor-256x85-sliding:
      shard-apl:          FAIL (fdo#103232) -> PASS

    igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-draw-pwrite:
      shard-apl:          FAIL (fdo#103167) -> PASS +1

    igt@kms_plane_multiple@atomic-pipe-a-tiling-x:
      shard-apl:          FAIL (fdo#103166) -> PASS +1

    igt@kms_setmode@basic:
      shard-hsw:          FAIL (fdo#99912) -> PASS

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

  fdo#103158 https://bugs.freedesktop.org/show_bug.cgi?id=103158
  fdo#103166 https://bugs.freedesktop.org/show_bug.cgi?id=103166
  fdo#103167 https://bugs.freedesktop.org/show_bug.cgi?id=103167
  fdo#103184 https://bugs.freedesktop.org/show_bug.cgi?id=103184
  fdo#103232 https://bugs.freedesktop.org/show_bug.cgi?id=103232
  fdo#103558 https://bugs.freedesktop.org/show_bug.cgi?id=103558
  fdo#103665 https://bugs.freedesktop.org/show_bug.cgi?id=103665
  fdo#104108 https://bugs.freedesktop.org/show_bug.cgi?id=104108
  fdo#105411 https://bugs.freedesktop.org/show_bug.cgi?id=105411
  fdo#105602 https://bugs.freedesktop.org/show_bug.cgi?id=105602
  fdo#106886 https://bugs.freedesktop.org/show_bug.cgi?id=106886
  fdo#107773 https://bugs.freedesktop.org/show_bug.cgi?id=107773
  fdo#107807 https://bugs.freedesktop.org/show_bug.cgi?id=107807
  fdo#107956 https://bugs.freedesktop.org/show_bug.cgi?id=107956
  fdo#99912 https://bugs.freedesktop.org/show_bug.cgi?id=99912


== Participating hosts (6 -> 5) ==

  Missing    (1): shard-glk 


== Build changes ==

    * Linux: CI_DRM_4915 -> Patchwork_10328

  CI_DRM_4915: 26e7a7d954a9c28b97af8ca7813f430fd9117232 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4660: d0975646c50568e66e65b44b81d28232d059b94e @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_10328: 74612a1286e9756d22b434d21fbf4607da498684 @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

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

Re: [PATCH 06/10] drm/i915/icl: Mark TC port as safe when interruptions are disabled

[Souza, Jose]

On Tue, 2018-10-02 at 23:35 +0300, Ville Syrjälä wrote:
> On Tue, Oct 02, 2018 at 10:50:50AM -0700, José Roberto de Souza
> wrote:
> > Before enter in power saving states or before unload the driver
> > spec states that display driver is required to to mark TC ports as
> > safe so hardware can do the disconnection flow without wait for
> > display driver handshake.
> > When driver is resumed or loaded again, if the TC live state is
> > still set as connected driver will mark again TC port as not safe
> > as
> > required by spec.
> > 
> > BSpec: 2175
> > 
> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_irq.c | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c
> > b/drivers/gpu/drm/i915/i915_irq.c
> > index 2e242270e270..58616690f45f 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -3640,6 +3640,7 @@ static void gen11_irq_reset(struct drm_device
> > *dev)
> >  {
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  	int pipe;
> > +	u32 val;
> >  
> >  	I915_WRITE(GEN11_GFX_MSTR_IRQ, 0);
> >  	POSTING_READ(GEN11_GFX_MSTR_IRQ);
> > @@ -3661,6 +3662,15 @@ static void gen11_irq_reset(struct
> > drm_device *dev)
> >  
> >  	if (HAS_PCH_ICP(dev_priv))
> >  		GEN3_IRQ_RESET(SDE);
> > +
> > +	/*
> > +	 * Mark TC ports as safe so hardware can do the disconnect flow
> > without
> > +	 * wait for driver to do the handshake
> > +	 */
> > +	val = I915_READ(PORT_TX_DFLEXDPCSSS);
> > +	for (pipe = 0; pipe < 4; pipe++)
> > +		val &= ~(DP_PHY_MODE_STATUS_NOT_SAFE(pipe));
> > +	I915_WRITE(PORT_TX_DFLEXDPCSSS, val);
> 
> Why would we have to do this here? The driver should relinquish
> control
> if and when it has shut down the pipes etc. Sounds like a bug if
> we're
> hanging on when we have no need for the port.

Right now we take control and only release it when port is
disconnected.

You mean release it in intel_dp_link_down()?
I guess it could be moved to that function but we would need:
- make icl_tc_port_connected() not grab control in any further calls to
intel_digital_port_connected() when driver is not using it
- to add special case to handle MST
- release control when a error happen when doing the modeset

In my opinion that would make it way more complicated with no benefit.

Also keeping it in gen11_irq_reset() would give back to HW the control
when loading the module, fixing possible problems with a BIOS that do
not release control of it before handing out system to bootloader.

> 
> >  }
> >  
> >  void gen8_irq_power_well_post_enable(struct drm_i915_private
> > *dev_priv,
> > -- 
> > 2.19.0
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 06/10] drm/i915/icl: Mark TC port as safe when interruptions are disabled

[Ville Syrjälä]

On Wed, Oct 10, 2018 at 12:55:07AM +0000, Souza, Jose wrote:
> On Tue, 2018-10-02 at 23:35 +0300, Ville Syrjälä wrote:
> > On Tue, Oct 02, 2018 at 10:50:50AM -0700, José Roberto de Souza
> > wrote:
> > > Before enter in power saving states or before unload the driver
> > > spec states that display driver is required to to mark TC ports as
> > > safe so hardware can do the disconnection flow without wait for
> > > display driver handshake.
> > > When driver is resumed or loaded again, if the TC live state is
> > > still set as connected driver will mark again TC port as not safe
> > > as
> > > required by spec.
> > > 
> > > BSpec: 2175
> > > 
> > > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_irq.c | 10 ++++++++++
> > >  1 file changed, 10 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_irq.c
> > > b/drivers/gpu/drm/i915/i915_irq.c
> > > index 2e242270e270..58616690f45f 100644
> > > --- a/drivers/gpu/drm/i915/i915_irq.c
> > > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > > @@ -3640,6 +3640,7 @@ static void gen11_irq_reset(struct drm_device
> > > *dev)
> > >  {
> > >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > >  	int pipe;
> > > +	u32 val;
> > >  
> > >  	I915_WRITE(GEN11_GFX_MSTR_IRQ, 0);
> > >  	POSTING_READ(GEN11_GFX_MSTR_IRQ);
> > > @@ -3661,6 +3662,15 @@ static void gen11_irq_reset(struct
> > > drm_device *dev)
> > >  
> > >  	if (HAS_PCH_ICP(dev_priv))
> > >  		GEN3_IRQ_RESET(SDE);
> > > +
> > > +	/*
> > > +	 * Mark TC ports as safe so hardware can do the disconnect flow
> > > without
> > > +	 * wait for driver to do the handshake
> > > +	 */
> > > +	val = I915_READ(PORT_TX_DFLEXDPCSSS);
> > > +	for (pipe = 0; pipe < 4; pipe++)
> > > +		val &= ~(DP_PHY_MODE_STATUS_NOT_SAFE(pipe));
> > > +	I915_WRITE(PORT_TX_DFLEXDPCSSS, val);
> > 
> > Why would we have to do this here? The driver should relinquish
> > control
> > if and when it has shut down the pipes etc. Sounds like a bug if
> > we're
> > hanging on when we have no need for the port.
> 
> Right now we take control and only release it when port is
> disconnected.

Disconnection is totally asynchronous. Someone could be using the
port/aux for anything when the disconnect irq happens. Presumably
bad things will happen if we just go and yank the control away
when someone is doing stuff. I believe even the spec tells us
to properly shut things down during the disconnect flow and make
sure eg. the aux power wells have been fully shut down before
relinquishing control.

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

Re: [PATCH 06/10] drm/i915/icl: Mark TC port as safe when interruptions are disabled

[Souza, Jose]

On Wed, 2018-10-10 at 20:15 +0300, Ville Syrjälä wrote:
> On Wed, Oct 10, 2018 at 12:55:07AM +0000, Souza, Jose wrote:
> > On Tue, 2018-10-02 at 23:35 +0300, Ville Syrjälä wrote:
> > > On Tue, Oct 02, 2018 at 10:50:50AM -0700, José Roberto de Souza
> > > wrote:
> > > > Before enter in power saving states or before unload the driver
> > > > spec states that display driver is required to to mark TC ports
> > > > as
> > > > safe so hardware can do the disconnection flow without wait for
> > > > display driver handshake.
> > > > When driver is resumed or loaded again, if the TC live state is
> > > > still set as connected driver will mark again TC port as not
> > > > safe
> > > > as
> > > > required by spec.
> > > > 
> > > > BSpec: 2175
> > > > 
> > > > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/i915_irq.c | 10 ++++++++++
> > > >  1 file changed, 10 insertions(+)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/i915_irq.c
> > > > b/drivers/gpu/drm/i915/i915_irq.c
> > > > index 2e242270e270..58616690f45f 100644
> > > > --- a/drivers/gpu/drm/i915/i915_irq.c
> > > > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > > > @@ -3640,6 +3640,7 @@ static void gen11_irq_reset(struct
> > > > drm_device
> > > > *dev)
> > > >  {
> > > >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > > >  	int pipe;
> > > > +	u32 val;
> > > >  
> > > >  	I915_WRITE(GEN11_GFX_MSTR_IRQ, 0);
> > > >  	POSTING_READ(GEN11_GFX_MSTR_IRQ);
> > > > @@ -3661,6 +3662,15 @@ static void gen11_irq_reset(struct
> > > > drm_device *dev)
> > > >  
> > > >  	if (HAS_PCH_ICP(dev_priv))
> > > >  		GEN3_IRQ_RESET(SDE);
> > > > +
> > > > +	/*
> > > > +	 * Mark TC ports as safe so hardware can do the
> > > > disconnect flow
> > > > without
> > > > +	 * wait for driver to do the handshake
> > > > +	 */
> > > > +	val = I915_READ(PORT_TX_DFLEXDPCSSS);
> > > > +	for (pipe = 0; pipe < 4; pipe++)
> > > > +		val &= ~(DP_PHY_MODE_STATUS_NOT_SAFE(pipe));
> > > > +	I915_WRITE(PORT_TX_DFLEXDPCSSS, val);
> > > 
> > > Why would we have to do this here? The driver should relinquish
> > > control
> > > if and when it has shut down the pipes etc. Sounds like a bug if
> > > we're
> > > hanging on when we have no need for the port.
> > 
> > Right now we take control and only release it when port is
> > disconnected.
> 
> Disconnection is totally asynchronous. Someone could be using the
> port/aux for anything when the disconnect irq happens. Presumably
> bad things will happen if we just go and yank the control away
> when someone is doing stuff. I believe even the spec tells us
> to properly shut things down during the disconnect flow and make
> sure eg. the aux power wells have been fully shut down before
> relinquishing control.

In my understanding at the point of the gen11_irq_reset() call all DDI
DDI ports and aux channels are already off.

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

Re: [PATCH 06/10] drm/i915/icl: Mark TC port as safe when interruptions are disabled

[Ville Syrjälä]

On Wed, Oct 10, 2018 at 05:23:30PM +0000, Souza, Jose wrote:
> On Wed, 2018-10-10 at 20:15 +0300, Ville Syrjälä wrote:
> > On Wed, Oct 10, 2018 at 12:55:07AM +0000, Souza, Jose wrote:
> > > On Tue, 2018-10-02 at 23:35 +0300, Ville Syrjälä wrote:
> > > > On Tue, Oct 02, 2018 at 10:50:50AM -0700, José Roberto de Souza
> > > > wrote:
> > > > > Before enter in power saving states or before unload the driver
> > > > > spec states that display driver is required to to mark TC ports
> > > > > as
> > > > > safe so hardware can do the disconnection flow without wait for
> > > > > display driver handshake.
> > > > > When driver is resumed or loaded again, if the TC live state is
> > > > > still set as connected driver will mark again TC port as not
> > > > > safe
> > > > > as
> > > > > required by spec.
> > > > > 
> > > > > BSpec: 2175
> > > > > 
> > > > > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > > > ---
> > > > >  drivers/gpu/drm/i915/i915_irq.c | 10 ++++++++++
> > > > >  1 file changed, 10 insertions(+)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/i915_irq.c
> > > > > b/drivers/gpu/drm/i915/i915_irq.c
> > > > > index 2e242270e270..58616690f45f 100644
> > > > > --- a/drivers/gpu/drm/i915/i915_irq.c
> > > > > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > > > > @@ -3640,6 +3640,7 @@ static void gen11_irq_reset(struct
> > > > > drm_device
> > > > > *dev)
> > > > >  {
> > > > >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > > > >  	int pipe;
> > > > > +	u32 val;
> > > > >  
> > > > >  	I915_WRITE(GEN11_GFX_MSTR_IRQ, 0);
> > > > >  	POSTING_READ(GEN11_GFX_MSTR_IRQ);
> > > > > @@ -3661,6 +3662,15 @@ static void gen11_irq_reset(struct
> > > > > drm_device *dev)
> > > > >  
> > > > >  	if (HAS_PCH_ICP(dev_priv))
> > > > >  		GEN3_IRQ_RESET(SDE);
> > > > > +
> > > > > +	/*
> > > > > +	 * Mark TC ports as safe so hardware can do the
> > > > > disconnect flow
> > > > > without
> > > > > +	 * wait for driver to do the handshake
> > > > > +	 */
> > > > > +	val = I915_READ(PORT_TX_DFLEXDPCSSS);
> > > > > +	for (pipe = 0; pipe < 4; pipe++)
> > > > > +		val &= ~(DP_PHY_MODE_STATUS_NOT_SAFE(pipe));
> > > > > +	I915_WRITE(PORT_TX_DFLEXDPCSSS, val);
> > > > 
> > > > Why would we have to do this here? The driver should relinquish
> > > > control
> > > > if and when it has shut down the pipes etc. Sounds like a bug if
> > > > we're
> > > > hanging on when we have no need for the port.
> > > 
> > > Right now we take control and only release it when port is
> > > disconnected.
> > 
> > Disconnection is totally asynchronous. Someone could be using the
> > port/aux for anything when the disconnect irq happens. Presumably
> > bad things will happen if we just go and yank the control away
> > when someone is doing stuff. I believe even the spec tells us
> > to properly shut things down during the disconnect flow and make
> > sure eg. the aux power wells have been fully shut down before
> > relinquishing control.
> 
> In my understanding at the point of the gen11_irq_reset() call all DDI
> DDI ports and aux channels are already off.

I'm not talking about gen11_irq_reset(). But if we are then that gets
called during driver load too and everything could be up and running.
Though I'm not sure what the BIOS/GOP will do w.r.t. the safe mode
knob.

Anyways, I don't think poking at some display stuff from irq_reset()
is a particularly clean apporoach. The irq code should only concern
itself with irqs. If we properly track which mode the port is in then
I presume we'd just put it back into the tbt mode when the last
typec mode user goes away. Or if we try to keep it in typec mode even
when there are no users (as some kind of optimimization perhaps?) then
we should probably switch it back to tbt mode during some display
suspend/shutdown sequence.

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

Re: [PATCH 06/10] drm/i915/icl: Mark TC port as safe when interruptions are disabled

[Souza, Jose]

On Wed, 2018-10-10 at 20:52 +0300, Ville Syrjälä wrote:
> On Wed, Oct 10, 2018 at 05:23:30PM +0000, Souza, Jose wrote:
> > On Wed, 2018-10-10 at 20:15 +0300, Ville Syrjälä wrote:
> > > On Wed, Oct 10, 2018 at 12:55:07AM +0000, Souza, Jose wrote:
> > > > On Tue, 2018-10-02 at 23:35 +0300, Ville Syrjälä wrote:
> > > > > On Tue, Oct 02, 2018 at 10:50:50AM -0700, José Roberto de
> > > > > Souza
> > > > > wrote:
> > > > > > Before enter in power saving states or before unload the
> > > > > > driver
> > > > > > spec states that display driver is required to to mark TC
> > > > > > ports
> > > > > > as
> > > > > > safe so hardware can do the disconnection flow without wait
> > > > > > for
> > > > > > display driver handshake.
> > > > > > When driver is resumed or loaded again, if the TC live
> > > > > > state is
> > > > > > still set as connected driver will mark again TC port as
> > > > > > not
> > > > > > safe
> > > > > > as
> > > > > > required by spec.
> > > > > > 
> > > > > > BSpec: 2175
> > > > > > 
> > > > > > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > > > > ---
> > > > > >  drivers/gpu/drm/i915/i915_irq.c | 10 ++++++++++
> > > > > >  1 file changed, 10 insertions(+)
> > > > > > 
> > > > > > diff --git a/drivers/gpu/drm/i915/i915_irq.c
> > > > > > b/drivers/gpu/drm/i915/i915_irq.c
> > > > > > index 2e242270e270..58616690f45f 100644
> > > > > > --- a/drivers/gpu/drm/i915/i915_irq.c
> > > > > > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > > > > > @@ -3640,6 +3640,7 @@ static void gen11_irq_reset(struct
> > > > > > drm_device
> > > > > > *dev)
> > > > > >  {
> > > > > >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > > > > >  	int pipe;
> > > > > > +	u32 val;
> > > > > >  
> > > > > >  	I915_WRITE(GEN11_GFX_MSTR_IRQ, 0);
> > > > > >  	POSTING_READ(GEN11_GFX_MSTR_IRQ);
> > > > > > @@ -3661,6 +3662,15 @@ static void gen11_irq_reset(struct
> > > > > > drm_device *dev)
> > > > > >  
> > > > > >  	if (HAS_PCH_ICP(dev_priv))
> > > > > >  		GEN3_IRQ_RESET(SDE);
> > > > > > +
> > > > > > +	/*
> > > > > > +	 * Mark TC ports as safe so hardware can do the
> > > > > > disconnect flow
> > > > > > without
> > > > > > +	 * wait for driver to do the handshake
> > > > > > +	 */
> > > > > > +	val = I915_READ(PORT_TX_DFLEXDPCSSS);
> > > > > > +	for (pipe = 0; pipe < 4; pipe++)
> > > > > > +		val &= ~(DP_PHY_MODE_STATUS_NOT_SAFE(pipe));
> > > > > > +	I915_WRITE(PORT_TX_DFLEXDPCSSS, val);
> > > > > 
> > > > > Why would we have to do this here? The driver should
> > > > > relinquish
> > > > > control
> > > > > if and when it has shut down the pipes etc. Sounds like a bug
> > > > > if
> > > > > we're
> > > > > hanging on when we have no need for the port.
> > > > 
> > > > Right now we take control and only release it when port is
> > > > disconnected.
> > > 
> > > Disconnection is totally asynchronous. Someone could be using the
> > > port/aux for anything when the disconnect irq happens. Presumably
> > > bad things will happen if we just go and yank the control away
> > > when someone is doing stuff. I believe even the spec tells us
> > > to properly shut things down during the disconnect flow and make
> > > sure eg. the aux power wells have been fully shut down before
> > > relinquishing control.
> > 
> > In my understanding at the point of the gen11_irq_reset() call all
> > DDI
> > DDI ports and aux channels are already off.
> 
> I'm not talking about gen11_irq_reset(). But if we are then that gets
> called during driver load too and everything could be up and running.
> Though I'm not sure what the BIOS/GOP will do w.r.t. the safe mode
> knob.

BIOS should do the same connection flow to use sinks in tc ports.

> 
> Anyways, I don't think poking at some display stuff from irq_reset()
> is a particularly clean apporoach. The irq code should only concern

Move it to a function? Or move everything reseting display irq to a
function like is done for gen11_gt_irq_reset().

> itself with irqs. If we properly track which mode the port is in then
> I presume we'd just put it back into the tbt mode when the last
> typec mode user goes away. Or if we try to keep it in typec mode even
> when there are no users (as some kind of optimimization perhaps?)
> then
> we should probably switch it back to tbt mode during some display
> suspend/shutdown sequence.

We don't control what mode the connector is in, HW is the one that tell
us if it is in: type-c, TBT, legacy or disconnected state our only job
is grab and release this knob.

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

Re: [PATCH 06/10] drm/i915/icl: Mark TC port as safe when interruptions are disabled

[Ville Syrjälä]

On Wed, Oct 10, 2018 at 06:27:07PM +0000, Souza, Jose wrote:
> On Wed, 2018-10-10 at 20:52 +0300, Ville Syrjälä wrote:
> > On Wed, Oct 10, 2018 at 05:23:30PM +0000, Souza, Jose wrote:
> > > On Wed, 2018-10-10 at 20:15 +0300, Ville Syrjälä wrote:
> > > > On Wed, Oct 10, 2018 at 12:55:07AM +0000, Souza, Jose wrote:
> > > > > On Tue, 2018-10-02 at 23:35 +0300, Ville Syrjälä wrote:
> > > > > > On Tue, Oct 02, 2018 at 10:50:50AM -0700, José Roberto de
> > > > > > Souza
> > > > > > wrote:
> > > > > > > Before enter in power saving states or before unload the
> > > > > > > driver
> > > > > > > spec states that display driver is required to to mark TC
> > > > > > > ports
> > > > > > > as
> > > > > > > safe so hardware can do the disconnection flow without wait
> > > > > > > for
> > > > > > > display driver handshake.
> > > > > > > When driver is resumed or loaded again, if the TC live
> > > > > > > state is
> > > > > > > still set as connected driver will mark again TC port as
> > > > > > > not
> > > > > > > safe
> > > > > > > as
> > > > > > > required by spec.
> > > > > > > 
> > > > > > > BSpec: 2175
> > > > > > > 
> > > > > > > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > > > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > > > > > ---
> > > > > > >  drivers/gpu/drm/i915/i915_irq.c | 10 ++++++++++
> > > > > > >  1 file changed, 10 insertions(+)
> > > > > > > 
> > > > > > > diff --git a/drivers/gpu/drm/i915/i915_irq.c
> > > > > > > b/drivers/gpu/drm/i915/i915_irq.c
> > > > > > > index 2e242270e270..58616690f45f 100644
> > > > > > > --- a/drivers/gpu/drm/i915/i915_irq.c
> > > > > > > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > > > > > > @@ -3640,6 +3640,7 @@ static void gen11_irq_reset(struct
> > > > > > > drm_device
> > > > > > > *dev)
> > > > > > >  {
> > > > > > >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > > > > > >  	int pipe;
> > > > > > > +	u32 val;
> > > > > > >  
> > > > > > >  	I915_WRITE(GEN11_GFX_MSTR_IRQ, 0);
> > > > > > >  	POSTING_READ(GEN11_GFX_MSTR_IRQ);
> > > > > > > @@ -3661,6 +3662,15 @@ static void gen11_irq_reset(struct
> > > > > > > drm_device *dev)
> > > > > > >  
> > > > > > >  	if (HAS_PCH_ICP(dev_priv))
> > > > > > >  		GEN3_IRQ_RESET(SDE);
> > > > > > > +
> > > > > > > +	/*
> > > > > > > +	 * Mark TC ports as safe so hardware can do the
> > > > > > > disconnect flow
> > > > > > > without
> > > > > > > +	 * wait for driver to do the handshake
> > > > > > > +	 */
> > > > > > > +	val = I915_READ(PORT_TX_DFLEXDPCSSS);
> > > > > > > +	for (pipe = 0; pipe < 4; pipe++)
> > > > > > > +		val &= ~(DP_PHY_MODE_STATUS_NOT_SAFE(pipe));
> > > > > > > +	I915_WRITE(PORT_TX_DFLEXDPCSSS, val);
> > > > > > 
> > > > > > Why would we have to do this here? The driver should
> > > > > > relinquish
> > > > > > control
> > > > > > if and when it has shut down the pipes etc. Sounds like a bug
> > > > > > if
> > > > > > we're
> > > > > > hanging on when we have no need for the port.
> > > > > 
> > > > > Right now we take control and only release it when port is
> > > > > disconnected.
> > > > 
> > > > Disconnection is totally asynchronous. Someone could be using the
> > > > port/aux for anything when the disconnect irq happens. Presumably
> > > > bad things will happen if we just go and yank the control away
> > > > when someone is doing stuff. I believe even the spec tells us
> > > > to properly shut things down during the disconnect flow and make
> > > > sure eg. the aux power wells have been fully shut down before
> > > > relinquishing control.
> > > 
> > > In my understanding at the point of the gen11_irq_reset() call all
> > > DDI
> > > DDI ports and aux channels are already off.
> > 
> > I'm not talking about gen11_irq_reset(). But if we are then that gets
> > called during driver load too and everything could be up and running.
> > Though I'm not sure what the BIOS/GOP will do w.r.t. the safe mode
> > knob.
> 
> BIOS should do the same connection flow to use sinks in tc ports.
> 
> > 
> > Anyways, I don't think poking at some display stuff from irq_reset()
> > is a particularly clean apporoach. The irq code should only concern
> 
> Move it to a function? Or move everything reseting display irq to a
> function like is done for gen11_gt_irq_reset().
> 
> > itself with irqs. If we properly track which mode the port is in then
> > I presume we'd just put it back into the tbt mode when the last
> > typec mode user goes away. Or if we try to keep it in typec mode even
> > when there are no users (as some kind of optimimization perhaps?)
> > then
> > we should probably switch it back to tbt mode during some display
> > suspend/shutdown sequence.
> 
> We don't control what mode the connector is in, HW is the one that tell
> us if it is in: type-c, TBT, legacy or disconnected state our only job
> is grab and release this knob.

So s/tbt mode/safe mode/ or whatever. Semantics.

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