[Intel-gfx] [PATCH v3 1/6] drm/i915/tc/tgl: Implement TCCOLD sequences

[Intel-gfx] [PATCH v3 1/6] drm/i915/tc/tgl: Implement TCCOLD sequences

[José Roberto de Souza]

TC ports can enter in TCCOLD to save power and is required to request
to PCODE to exit this state before use or read to TC registers.

For TGL there is a new MBOX command to do that with a parameter to ask
PCODE to exit and block TCCOLD entry or unblock TCCOLD entry.
For GEN11 the sequence is more complex and will be handled in a
separated patch.

BSpec: 49294
Cc: Imre Deak <imre.deak@intel.com>
Cc: Cooper Chiou <cooper.chiou@intel.com>
Cc: Kai-Heng Feng <kai.heng.feng@canonical.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 drivers/gpu/drm/i915/display/intel_tc.c | 61 ++++++++++++++++++++++++-
 drivers/gpu/drm/i915/i915_reg.h         |  3 ++
 drivers/gpu/drm/i915/intel_sideband.c   | 22 +++++++++
 drivers/gpu/drm/i915/intel_sideband.h   |  4 ++
 4 files changed, 88 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_tc.c b/drivers/gpu/drm/i915/display/intel_tc.c
index 9b850c11aa78..e4c5de5ce874 100644
--- a/drivers/gpu/drm/i915/display/intel_tc.c
+++ b/drivers/gpu/drm/i915/display/intel_tc.c
@@ -7,6 +7,7 @@
 #include "intel_display.h"
 #include "intel_display_types.h"
 #include "intel_dp_mst.h"
+#include "intel_sideband.h"
 #include "intel_tc.h"
 
 static const char *tc_port_mode_name(enum tc_port_mode mode)
@@ -496,6 +497,55 @@ bool intel_tc_port_connected(struct intel_digital_port *dig_port)
 	return is_connected;
 }
 
+static inline int tgl_tc_cold_request(struct intel_digital_port *dig_port,
+				      bool block)
+{
+	struct drm_i915_private *i915 = to_i915(dig_port->base.base.dev);
+	u32 low_val, high_val;
+	u8 tries = 0;
+	int ret;
+
+	do {
+		low_val = 0;
+		high_val = block ? 0 : TGL_PCODE_EXIT_TCCOLD_DATA_H_UNBLOCK_REQ;
+
+		ret = sandybridge_pcode_write_read_timeout(i915,
+							   TGL_PCODE_TCCOLD,
+							   &low_val, &high_val,
+							   150, 1);
+		if (ret == 0) {
+			if (block &&
+			    low_val & TGL_PCODE_EXIT_TCCOLD_DATA_L_EXIT_FAILED)
+				ret = -EIO;
+			else
+				break;
+		}
+
+		if (ret != -EAGAIN)
+			tries++;
+	} while (tries < 3);
+
+	return ret;
+}
+
+static int tc_cold_request(struct intel_digital_port *dig_port, bool block)
+{
+	struct drm_i915_private *i915 = to_i915(dig_port->base.base.dev);
+	int ret;
+
+	if (INTEL_GEN(i915) >= 12)
+		ret = tgl_tc_cold_request(dig_port, block);
+	else
+		/* TODO: implement GEN11 TCCOLD sequences */
+		ret = 0;
+
+	drm_dbg_kms(&i915->drm, "Port %s: TCCOLD %sblock %s\n",
+		    dig_port->tc_port_name, (block ? "" : "un"),
+		    (ret == 0 ? "succeeded" : "failed"));
+
+	return ret;
+}
+
 static void __intel_tc_port_lock(struct intel_digital_port *dig_port,
 				 int required_lanes)
 {
@@ -506,9 +556,11 @@ static void __intel_tc_port_lock(struct intel_digital_port *dig_port,
 
 	mutex_lock(&dig_port->tc_lock);
 
-	if (!dig_port->tc_link_refcount &&
-	    intel_tc_port_needs_reset(dig_port))
+	if (dig_port->tc_link_refcount == 0) {
+		tc_cold_request(dig_port, true);
+		intel_tc_port_needs_reset(dig_port);
 		intel_tc_port_reset_mode(dig_port, required_lanes);
+	}
 
 	drm_WARN_ON(&i915->drm, dig_port->tc_lock_wakeref);
 	dig_port->tc_lock_wakeref = wakeref;
@@ -524,6 +576,9 @@ void intel_tc_port_unlock(struct intel_digital_port *dig_port)
 	struct drm_i915_private *i915 = to_i915(dig_port->base.base.dev);
 	intel_wakeref_t wakeref = fetch_and_zero(&dig_port->tc_lock_wakeref);
 
+	if (dig_port->tc_link_refcount == 0)
+		tc_cold_request(dig_port, false);
+
 	mutex_unlock(&dig_port->tc_lock);
 
 	intel_display_power_put_async(i915, POWER_DOMAIN_DISPLAY_CORE,
@@ -548,6 +603,8 @@ void intel_tc_port_put_link(struct intel_digital_port *dig_port)
 {
 	mutex_lock(&dig_port->tc_lock);
 	dig_port->tc_link_refcount--;
+	if (dig_port->tc_link_refcount == 0)
+		tc_cold_request(dig_port, false);
 	mutex_unlock(&dig_port->tc_lock);
 }
 
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 9c53fe918be6..7e341d9945b3 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -9019,6 +9019,9 @@ enum {
 #define   GEN6_PCODE_WRITE_D_COMP		0x11
 #define   HSW_PCODE_DE_WRITE_FREQ_REQ		0x17
 #define   DISPLAY_IPS_CONTROL			0x19
+#define   TGL_PCODE_TCCOLD				0x26
+#define     TGL_PCODE_EXIT_TCCOLD_DATA_L_EXIT_FAILED	REG_BIT(0)
+#define     TGL_PCODE_EXIT_TCCOLD_DATA_H_UNBLOCK_REQ	REG_BIT(0)
             /* See also IPS_CTL */
 #define     IPS_PCODE_CONTROL			(1 << 30)
 #define   HSW_PCODE_DYNAMIC_DUTY_CYCLE_CONTROL	0x1A
diff --git a/drivers/gpu/drm/i915/intel_sideband.c b/drivers/gpu/drm/i915/intel_sideband.c
index 1447e7516cb7..20a9d3970930 100644
--- a/drivers/gpu/drm/i915/intel_sideband.c
+++ b/drivers/gpu/drm/i915/intel_sideband.c
@@ -463,6 +463,28 @@ int sandybridge_pcode_write_timeout(struct drm_i915_private *i915,
 	return err;
 }
 
+int sandybridge_pcode_write_read_timeout(struct drm_i915_private *i915,
+					 u32 mbox, u32 *val, u32 *val1,
+					 int fast_timeout_us,
+					 int slow_timeout_ms)
+{
+	int err;
+
+	mutex_lock(&i915->sb_lock);
+	err = __sandybridge_pcode_rw(i915, mbox, val, val1,
+				     fast_timeout_us, slow_timeout_ms,
+				     true);
+	mutex_unlock(&i915->sb_lock);
+
+	if (err) {
+		drm_dbg(&i915->drm,
+			"warning: pcode (write of 0x%08x to mbox %x) mailbox access failed for %ps: %d\n",
+			*val, mbox, __builtin_return_address(0), err);
+	}
+
+	return err;
+}
+
 static bool skl_pcode_try_request(struct drm_i915_private *i915, u32 mbox,
 				  u32 request, u32 reply_mask, u32 reply,
 				  u32 *status)
diff --git a/drivers/gpu/drm/i915/intel_sideband.h b/drivers/gpu/drm/i915/intel_sideband.h
index 7fb95745a444..1939bebb4e67 100644
--- a/drivers/gpu/drm/i915/intel_sideband.h
+++ b/drivers/gpu/drm/i915/intel_sideband.h
@@ -132,6 +132,10 @@ int sandybridge_pcode_read(struct drm_i915_private *i915, u32 mbox,
 int sandybridge_pcode_write_timeout(struct drm_i915_private *i915, u32 mbox,
 				    u32 val, int fast_timeout_us,
 				    int slow_timeout_ms);
+int sandybridge_pcode_write_read_timeout(struct drm_i915_private *i915,
+					 u32 mbox, u32 *val, u32 *val1,
+					 int fast_timeout_us,
+					 int slow_timeout_ms);
 #define sandybridge_pcode_write(i915, mbox, val)	\
 	sandybridge_pcode_write_timeout(i915, mbox, val, 500, 0)
 
-- 
2.26.0

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

[Intel-gfx] [PATCH v3 2/6] drm/i915/display: Add intel_display_power_get_without_ack()

[José Roberto de Souza]

To implement ICL TC static sequences is required to get the port aux
powerwell without wait for hardware ack.

Cc: Imre Deak <imre.deak@intel.com>
Cc: Cooper Chiou <cooper.chiou@intel.com>
Cc: Kai-Heng Feng <kai.heng.feng@canonical.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 .../drm/i915/display/intel_display_power.c    | 71 +++++++++++++++----
 .../drm/i915/display/intel_display_power.h    | 12 ++++
 2 files changed, 71 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c b/drivers/gpu/drm/i915/display/intel_display_power.c
index 246e406bb385..9035b220dfa0 100644
--- a/drivers/gpu/drm/i915/display/intel_display_power.c
+++ b/drivers/gpu/drm/i915/display/intel_display_power.c
@@ -157,14 +157,24 @@ intel_display_power_domain_str(enum intel_display_power_domain domain)
 	}
 }
 
-static void intel_power_well_enable(struct drm_i915_private *dev_priv,
-				    struct i915_power_well *power_well)
+static void _intel_power_well_enable(struct drm_i915_private *dev_priv,
+				     struct i915_power_well *power_well,
+				     bool wait_ack)
 {
 	drm_dbg_kms(&dev_priv->drm, "enabling %s\n", power_well->desc->name);
-	power_well->desc->ops->enable(dev_priv, power_well);
+	if (wait_ack || !power_well->desc->ops->enable_without_ack)
+		power_well->desc->ops->enable(dev_priv, power_well);
+	else
+		power_well->desc->ops->enable_without_ack(dev_priv, power_well);
 	power_well->hw_enabled = true;
 }
 
+static void intel_power_well_enable(struct drm_i915_private *dev_priv,
+				    struct i915_power_well *power_well)
+{
+	_intel_power_well_enable(dev_priv, power_well, true);
+}
+
 static void intel_power_well_disable(struct drm_i915_private *dev_priv,
 				     struct i915_power_well *power_well)
 {
@@ -174,10 +184,11 @@ static void intel_power_well_disable(struct drm_i915_private *dev_priv,
 }
 
 static void intel_power_well_get(struct drm_i915_private *dev_priv,
-				 struct i915_power_well *power_well)
+				 struct i915_power_well *power_well,
+				 bool wait_ack)
 {
 	if (!power_well->count++)
-		intel_power_well_enable(dev_priv, power_well);
+		_intel_power_well_enable(dev_priv, power_well, wait_ack);
 }
 
 static void intel_power_well_put(struct drm_i915_private *dev_priv,
@@ -353,8 +364,9 @@ static void gen9_wait_for_power_well_fuses(struct drm_i915_private *dev_priv,
 					  SKL_FUSE_PG_DIST_STATUS(pg), 1));
 }
 
-static void hsw_power_well_enable(struct drm_i915_private *dev_priv,
-				  struct i915_power_well *power_well)
+static void _hsw_power_well_enable(struct drm_i915_private *dev_priv,
+				   struct i915_power_well *power_well,
+				   bool wait_ack)
 {
 	const struct i915_power_well_regs *regs = power_well->desc->hsw.regs;
 	int pw_idx = power_well->desc->hsw.idx;
@@ -379,7 +391,8 @@ static void hsw_power_well_enable(struct drm_i915_private *dev_priv,
 	val = intel_de_read(dev_priv, regs->driver);
 	intel_de_write(dev_priv, regs->driver,
 		       val | HSW_PWR_WELL_CTL_REQ(pw_idx));
-	hsw_wait_for_power_well_enable(dev_priv, power_well);
+	if (wait_ack)
+		hsw_wait_for_power_well_enable(dev_priv, power_well);
 
 	/* Display WA #1178: cnl */
 	if (IS_CANNONLAKE(dev_priv) &&
@@ -398,6 +411,12 @@ static void hsw_power_well_enable(struct drm_i915_private *dev_priv,
 				   power_well->desc->hsw.has_vga);
 }
 
+static void hsw_power_well_enable(struct drm_i915_private *dev_priv,
+				  struct i915_power_well *power_well)
+{
+	_hsw_power_well_enable(dev_priv, power_well, true);
+}
+
 static void hsw_power_well_disable(struct drm_i915_private *dev_priv,
 				   struct i915_power_well *power_well)
 {
@@ -1960,7 +1979,8 @@ intel_display_power_grab_async_put_ref(struct drm_i915_private *dev_priv,
 
 static void
 __intel_display_power_get_domain(struct drm_i915_private *dev_priv,
-				 enum intel_display_power_domain domain)
+				 enum intel_display_power_domain domain,
+				 bool wait_ack)
 {
 	struct i915_power_domains *power_domains = &dev_priv->power_domains;
 	struct i915_power_well *power_well;
@@ -1969,7 +1989,7 @@ __intel_display_power_get_domain(struct drm_i915_private *dev_priv,
 		return;
 
 	for_each_power_domain_well(dev_priv, power_well, BIT_ULL(domain))
-		intel_power_well_get(dev_priv, power_well);
+		intel_power_well_get(dev_priv, power_well, wait_ack);
 
 	power_domains->domain_use_count[domain]++;
 }
@@ -1993,7 +2013,34 @@ intel_wakeref_t intel_display_power_get(struct drm_i915_private *dev_priv,
 	intel_wakeref_t wakeref = intel_runtime_pm_get(&dev_priv->runtime_pm);
 
 	mutex_lock(&power_domains->lock);
-	__intel_display_power_get_domain(dev_priv, domain);
+	__intel_display_power_get_domain(dev_priv, domain, true);
+	mutex_unlock(&power_domains->lock);
+
+	return wakeref;
+}
+
+/**
+ * intel_display_power_get_without_ack - grab a power domain reference without
+ * wait for HW ack
+ * @dev_priv: i915 device instance
+ * @domain: power domain to reference
+ *
+ * This function grabs a power domain reference for @domain and ensures that the
+ * power domain and all its parents are powered up but it don't wait for
+ * hardware ack if supported by each powerwell. Users should only grab a
+ * reference to the innermost power domain they need.
+ *
+ * Any power domain reference obtained by this function must have a symmetric
+ * call to intel_display_power_put() to release the reference again.
+ */
+intel_wakeref_t intel_display_power_get_without_ack(struct drm_i915_private *dev_priv,
+						    enum intel_display_power_domain domain)
+{
+	struct i915_power_domains *power_domains = &dev_priv->power_domains;
+	intel_wakeref_t wakeref = intel_runtime_pm_get(&dev_priv->runtime_pm);
+
+	mutex_lock(&power_domains->lock);
+	__intel_display_power_get_domain(dev_priv, domain, false);
 	mutex_unlock(&power_domains->lock);
 
 	return wakeref;
@@ -2026,7 +2073,7 @@ intel_display_power_get_if_enabled(struct drm_i915_private *dev_priv,
 	mutex_lock(&power_domains->lock);
 
 	if (__intel_display_power_is_enabled(dev_priv, domain)) {
-		__intel_display_power_get_domain(dev_priv, domain);
+		__intel_display_power_get_domain(dev_priv, domain, true);
 		is_enabled = true;
 	} else {
 		is_enabled = false;
diff --git a/drivers/gpu/drm/i915/display/intel_display_power.h b/drivers/gpu/drm/i915/display/intel_display_power.h
index da64a5edae7a..5db86cc862c3 100644
--- a/drivers/gpu/drm/i915/display/intel_display_power.h
+++ b/drivers/gpu/drm/i915/display/intel_display_power.h
@@ -129,6 +129,16 @@ struct i915_power_well_ops {
 	 */
 	void (*enable)(struct drm_i915_private *dev_priv,
 		       struct i915_power_well *power_well);
+
+	/*
+	 * Enable the well and resources that depend on it (for example
+	 * interrupts located on the well) without reading HW ack. Called after
+	 * the 0->1 refcount transition.
+	 * This will be used by TC subsystem and it is a optional hook.
+	 */
+	void (*enable_without_ack)(struct drm_i915_private *dev_priv,
+				   struct i915_power_well *power_well);
+
 	/*
 	 * Disable the well and resources that depend on it. Called after
 	 * the 1->0 refcount transition.
@@ -270,6 +280,8 @@ bool __intel_display_power_is_enabled(struct drm_i915_private *dev_priv,
 				      enum intel_display_power_domain domain);
 intel_wakeref_t intel_display_power_get(struct drm_i915_private *dev_priv,
 					enum intel_display_power_domain domain);
+intel_wakeref_t intel_display_power_get_without_ack(struct drm_i915_private *dev_priv,
+						    enum intel_display_power_domain domain);
 intel_wakeref_t
 intel_display_power_get_if_enabled(struct drm_i915_private *dev_priv,
 				   enum intel_display_power_domain domain);
-- 
2.26.0

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

[Intel-gfx] [PATCH v3 3/6] drm/i915/display: Implement intel_display_power_wait_enable_ack()

[José Roberto de Souza]

This function is meant to be used after
intel_display_power_get_without_ack() this way we can be sure that the
HW tied to the powerdomain will be powered and ready.

Cc: Imre Deak <imre.deak@intel.com>
Cc: Cooper Chiou <cooper.chiou@intel.com>
Cc: Kai-Heng Feng <kai.heng.feng@canonical.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 .../drm/i915/display/intel_display_power.c    | 29 +++++++++++++++++++
 .../drm/i915/display/intel_display_power.h    |  9 ++++++
 2 files changed, 38 insertions(+)

diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c b/drivers/gpu/drm/i915/display/intel_display_power.c
index 9035b220dfa0..a7e531b64e16 100644
--- a/drivers/gpu/drm/i915/display/intel_display_power.c
+++ b/drivers/gpu/drm/i915/display/intel_display_power.c
@@ -2328,6 +2328,35 @@ intel_display_power_flush_work_sync(struct drm_i915_private *i915)
 	drm_WARN_ON(&i915->drm, power_domains->async_put_wakeref);
 }
 
+/**
+ * intel_display_power_wait_enable_ack - wait for enabled hardware ack
+ * @dev_priv: i915 device instance
+ * @domain: power domain to reference
+ *
+ * This function must be called after intel_display_power_get_without_ack() and
+ * only in power domains that implements it.
+ */
+void intel_display_power_wait_enable_ack(struct drm_i915_private *dev_priv,
+					 enum intel_display_power_domain domain)
+{
+	struct i915_power_domains *power_domains = &dev_priv->power_domains;
+	struct i915_power_well *power_well;
+
+	mutex_lock(&power_domains->lock);
+
+	for_each_power_domain_well_reverse(dev_priv, power_well,
+					   BIT_ULL(domain)) {
+		if (drm_WARN_ON(&dev_priv->drm,
+				!power_well->desc->ops->wait_enable_ack))
+			break;
+
+		power_well->desc->ops->wait_enable_ack(dev_priv, power_well);
+		break;
+	}
+
+	mutex_unlock(&power_domains->lock);
+}
+
 #if IS_ENABLED(CONFIG_DRM_I915_DEBUG_RUNTIME_PM)
 /**
  * intel_display_power_put - release a power domain reference
diff --git a/drivers/gpu/drm/i915/display/intel_display_power.h b/drivers/gpu/drm/i915/display/intel_display_power.h
index 5db86cc862c3..108096177deb 100644
--- a/drivers/gpu/drm/i915/display/intel_display_power.h
+++ b/drivers/gpu/drm/i915/display/intel_display_power.h
@@ -148,6 +148,13 @@ struct i915_power_well_ops {
 	/* Returns the hw enabled state. */
 	bool (*is_enabled)(struct drm_i915_private *dev_priv,
 			   struct i915_power_well *power_well);
+
+	/*
+	 * Waits for hardware enabling ack, this is meant to be used together
+	 * with enable_without_ack() and also optional.
+	 */
+	void (*wait_enable_ack)(struct drm_i915_private *dev_priv,
+				struct i915_power_well *power_well);
 };
 
 struct i915_power_well_regs {
@@ -291,6 +298,8 @@ void __intel_display_power_put_async(struct drm_i915_private *i915,
 				     enum intel_display_power_domain domain,
 				     intel_wakeref_t wakeref);
 void intel_display_power_flush_work(struct drm_i915_private *i915);
+void intel_display_power_wait_enable_ack(struct drm_i915_private *dev_priv,
+					 enum intel_display_power_domain domain);
 #if IS_ENABLED(CONFIG_DRM_I915_DEBUG_RUNTIME_PM)
 void intel_display_power_put(struct drm_i915_private *dev_priv,
 			     enum intel_display_power_domain domain,
-- 
2.26.0

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

[Intel-gfx] [PATCH v3 4/6] drm/i915/display: Add intel_aux_ch_to_power_domain()

[José Roberto de Souza]

This is a similar function to intel_aux_power_domain() but it do not
care about TBT ports, this will be needed by GEN11 TC sequences.

Cc: Imre Deak <imre.deak@intel.com>
Cc: Cooper Chiou <cooper.chiou@intel.com>
Cc: Kai-Heng Feng <kai.heng.feng@canonical.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 drivers/gpu/drm/i915/display/intel_display.c | 14 ++++++++++++--
 drivers/gpu/drm/i915/display/intel_display.h |  2 ++
 2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index 37bd7ce88ecd..bda35c1337de 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -7312,7 +7312,17 @@ intel_aux_power_domain(struct intel_digital_port *dig_port)
 		}
 	}
 
-	switch (dig_port->aux_ch) {
+	return intel_aux_ch_to_power_domain(dig_port->aux_ch);
+}
+
+/*
+ * Converts aux_ch to power_domain without caring about TBT ports for that use
+ * intel_aux_power_domain()
+ */
+enum intel_display_power_domain
+intel_aux_ch_to_power_domain(enum aux_ch aux_ch)
+{
+	switch (aux_ch) {
 	case AUX_CH_A:
 		return POWER_DOMAIN_AUX_A;
 	case AUX_CH_B:
@@ -7328,7 +7338,7 @@ intel_aux_power_domain(struct intel_digital_port *dig_port)
 	case AUX_CH_G:
 		return POWER_DOMAIN_AUX_G;
 	default:
-		MISSING_CASE(dig_port->aux_ch);
+		MISSING_CASE(aux_ch);
 		return POWER_DOMAIN_AUX_A;
 	}
 }
diff --git a/drivers/gpu/drm/i915/display/intel_display.h b/drivers/gpu/drm/i915/display/intel_display.h
index adb1225a3480..ad50119c0453 100644
--- a/drivers/gpu/drm/i915/display/intel_display.h
+++ b/drivers/gpu/drm/i915/display/intel_display.h
@@ -579,6 +579,8 @@ void hsw_disable_ips(const struct intel_crtc_state *crtc_state);
 enum intel_display_power_domain intel_port_to_power_domain(enum port port);
 enum intel_display_power_domain
 intel_aux_power_domain(struct intel_digital_port *dig_port);
+enum intel_display_power_domain
+intel_aux_ch_to_power_domain(enum aux_ch aux_ch);
 void intel_mode_from_pipe_config(struct drm_display_mode *mode,
 				 struct intel_crtc_state *pipe_config);
 void intel_crtc_arm_fifo_underrun(struct intel_crtc *crtc,
-- 
2.26.0

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

[Intel-gfx] [PATCH v3 5/6] drm/i915/tc/icl: Implement the TC cold exit sequence

[José Roberto de Souza]

This is required for legacy/static TC ports as IOM is not aware of
the connection and will not trigger the TC cold exit.

Just request PCODE to exit TCCOLD is not enough as it could enter
again be driver makes use of the port, to prevent it BSpec states that
aux powerwell should be held.

So before detecting the mode, aux power is requested without wait for
hardware ack, PCODE is requested to exit TCCOLD and the TC detection
sequences follows as normal.
After detection if mode is not static aux can be powered off otherwise
we need to wait for HW ack as future calls to intel_display_power_get()
over aux will not check for HW ack.

v2:
- fixed typo tc_lock_wakeref to tc_cold_wakeref in icl_tc_cold_request()

v3:
- fixed non initialized ret in icl_tc_cold_request()
- added missing sleep step, initially it was not added because I
thought that the aux enable and then HW ack wait would take care of
that but that is not the case

BSpec: 21750
Fixes: https://gitlab.freedesktop.org/drm/intel/issues/1296
Cc: Imre Deak <imre.deak@intel.com>
Cc: Cooper Chiou <cooper.chiou@intel.com>
Cc: Kai-Heng Feng <kai.heng.feng@canonical.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 .../drm/i915/display/intel_display_power.c    | 30 +++++++++-
 .../drm/i915/display/intel_display_types.h    |  1 +
 drivers/gpu/drm/i915/display/intel_tc.c       | 60 +++++++++++++++++--
 drivers/gpu/drm/i915/i915_reg.h               |  1 +
 4 files changed, 84 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c b/drivers/gpu/drm/i915/display/intel_display_power.c
index a7e531b64e16..71a4c5d790ea 100644
--- a/drivers/gpu/drm/i915/display/intel_display_power.c
+++ b/drivers/gpu/drm/i915/display/intel_display_power.c
@@ -573,8 +573,9 @@ static void icl_tc_port_assert_ref_held(struct drm_i915_private *dev_priv,
 #define TGL_AUX_PW_TO_TC_PORT(pw_idx)	((pw_idx) - TGL_PW_CTL_IDX_AUX_TC1)
 
 static void
-icl_tc_phy_aux_power_well_enable(struct drm_i915_private *dev_priv,
-				 struct i915_power_well *power_well)
+_icl_tc_phy_aux_power_well_enable(struct drm_i915_private *dev_priv,
+				  struct i915_power_well *power_well,
+				  bool wait_ack)
 {
 	enum aux_ch aux_ch = icl_tc_phy_aux_ch(dev_priv, power_well);
 	u32 val;
@@ -587,7 +588,7 @@ icl_tc_phy_aux_power_well_enable(struct drm_i915_private *dev_priv,
 		val |= DP_AUX_CH_CTL_TBT_IO;
 	intel_de_write(dev_priv, DP_AUX_CH_CTL(aux_ch), val);
 
-	hsw_power_well_enable(dev_priv, power_well);
+	_hsw_power_well_enable(dev_priv, power_well, wait_ack);
 
 	if (INTEL_GEN(dev_priv) >= 12 && !power_well->desc->hsw.is_tc_tbt) {
 		enum tc_port tc_port;
@@ -603,6 +604,20 @@ icl_tc_phy_aux_power_well_enable(struct drm_i915_private *dev_priv,
 	}
 }
 
+static void
+icl_tc_phy_aux_power_well_enable(struct drm_i915_private *dev_priv,
+				 struct i915_power_well *power_well)
+{
+	_icl_tc_phy_aux_power_well_enable(dev_priv, power_well, true);
+}
+
+static void
+icl_tc_phy_aux_power_well_enable_without_ack(struct drm_i915_private *dev_priv,
+					     struct i915_power_well *power_well)
+{
+	_icl_tc_phy_aux_power_well_enable(dev_priv, power_well, false);
+}
+
 static void
 icl_tc_phy_aux_power_well_disable(struct drm_i915_private *dev_priv,
 				  struct i915_power_well *power_well)
@@ -642,6 +657,13 @@ static bool hsw_power_well_enabled(struct drm_i915_private *dev_priv,
 	return (val & mask) == mask;
 }
 
+static void
+hsw_power_well_wait_ack(struct drm_i915_private *dev_priv,
+			struct i915_power_well *power_well)
+{
+	hsw_wait_for_power_well_enable(dev_priv, power_well);
+}
+
 static void assert_can_enable_dc9(struct drm_i915_private *dev_priv)
 {
 	drm_WARN_ONCE(&dev_priv->drm,
@@ -3582,8 +3604,10 @@ static const struct i915_power_well_ops icl_combo_phy_aux_power_well_ops = {
 static const struct i915_power_well_ops icl_tc_phy_aux_power_well_ops = {
 	.sync_hw = hsw_power_well_sync_hw,
 	.enable = icl_tc_phy_aux_power_well_enable,
+	.enable_without_ack = icl_tc_phy_aux_power_well_enable_without_ack,
 	.disable = icl_tc_phy_aux_power_well_disable,
 	.is_enabled = hsw_power_well_enabled,
+	.wait_enable_ack = hsw_power_well_wait_ack,
 };
 
 static const struct i915_power_well_regs icl_aux_power_well_regs = {
diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
index 176ab5f1e867..42954be80435 100644
--- a/drivers/gpu/drm/i915/display/intel_display_types.h
+++ b/drivers/gpu/drm/i915/display/intel_display_types.h
@@ -1397,6 +1397,7 @@ struct intel_digital_port {
 	enum tc_port_mode tc_mode;
 	enum phy_fia tc_phy_fia;
 	u8 tc_phy_fia_idx;
+	intel_wakeref_t tc_cold_wakeref;
 
 	void (*write_infoframe)(struct intel_encoder *encoder,
 				const struct intel_crtc_state *crtc_state,
diff --git a/drivers/gpu/drm/i915/display/intel_tc.c b/drivers/gpu/drm/i915/display/intel_tc.c
index e4c5de5ce874..588fca873b55 100644
--- a/drivers/gpu/drm/i915/display/intel_tc.c
+++ b/drivers/gpu/drm/i915/display/intel_tc.c
@@ -416,9 +416,6 @@ static void intel_tc_port_reset_mode(struct intel_digital_port *dig_port,
 	enum tc_port_mode old_tc_mode = dig_port->tc_mode;
 
 	intel_display_power_flush_work(i915);
-	drm_WARN_ON(&i915->drm,
-		    intel_display_power_is_enabled(i915,
-					intel_aux_power_domain(dig_port)));
 
 	icl_tc_phy_disconnect(dig_port);
 	icl_tc_phy_connect(dig_port, required_lanes);
@@ -528,6 +525,39 @@ static inline int tgl_tc_cold_request(struct intel_digital_port *dig_port,
 	return ret;
 }
 
+static inline int icl_tc_cold_request(struct intel_digital_port *dig_port,
+				      bool block)
+{
+	struct drm_i915_private *i915 = to_i915(dig_port->base.base.dev);
+	enum intel_display_power_domain aux_domain;
+	int ret;
+
+	aux_domain = intel_aux_ch_to_power_domain(dig_port->aux_ch);
+
+	if (block) {
+		dig_port->tc_cold_wakeref =
+			intel_display_power_get_without_ack(i915, aux_domain);
+
+		do {
+			ret = sandybridge_pcode_write_timeout(i915,
+							      ICL_PCODE_EXIT_TCCOLD,
+							      0, 250, 1);
+
+		} while (ret == -EAGAIN);
+
+		if (!ret)
+			msleep(1);
+	} else if (dig_port->tc_mode == TC_PORT_LEGACY) {
+		drm_WARN_ON(&i915->drm, !dig_port->tc_cold_wakeref);
+		intel_display_power_put(i915, aux_domain,
+					dig_port->tc_cold_wakeref);
+		dig_port->tc_cold_wakeref = 0;
+		ret = 0;
+	}
+
+	return ret;
+}
+
 static int tc_cold_request(struct intel_digital_port *dig_port, bool block)
 {
 	struct drm_i915_private *i915 = to_i915(dig_port->base.base.dev);
@@ -536,8 +566,7 @@ static int tc_cold_request(struct intel_digital_port *dig_port, bool block)
 	if (INTEL_GEN(i915) >= 12)
 		ret = tgl_tc_cold_request(dig_port, block);
 	else
-		/* TODO: implement GEN11 TCCOLD sequences */
-		ret = 0;
+		ret = icl_tc_cold_request(dig_port, block);
 
 	drm_dbg_kms(&i915->drm, "Port %s: TCCOLD %sblock %s\n",
 		    dig_port->tc_port_name, (block ? "" : "un"),
@@ -546,6 +575,26 @@ static int tc_cold_request(struct intel_digital_port *dig_port, bool block)
 	return ret;
 }
 
+static void tc_cold_after_reset_mode(struct intel_digital_port *dig_port)
+{
+	struct drm_i915_private *i915 = to_i915(dig_port->base.base.dev);
+	enum intel_display_power_domain aux_domain;
+
+	if (INTEL_GEN(i915) >= 12)
+		return;
+
+	aux_domain = intel_aux_ch_to_power_domain(dig_port->aux_ch);
+
+	if (dig_port->tc_mode == TC_PORT_LEGACY) {
+		intel_display_power_wait_enable_ack(i915, aux_domain);
+	} else {
+		drm_WARN_ON(&i915->drm, !dig_port->tc_cold_wakeref);
+		intel_display_power_put(i915, aux_domain,
+					dig_port->tc_cold_wakeref);
+		dig_port->tc_cold_wakeref = 0;
+	}
+}
+
 static void __intel_tc_port_lock(struct intel_digital_port *dig_port,
 				 int required_lanes)
 {
@@ -560,6 +609,7 @@ static void __intel_tc_port_lock(struct intel_digital_port *dig_port,
 		tc_cold_request(dig_port, true);
 		intel_tc_port_needs_reset(dig_port);
 		intel_tc_port_reset_mode(dig_port, required_lanes);
+		tc_cold_after_reset_mode(dig_port);
 	}
 
 	drm_WARN_ON(&i915->drm, dig_port->tc_lock_wakeref);
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 7e341d9945b3..8d4f40a70a4d 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -9017,6 +9017,7 @@ enum {
 #define     ICL_PCODE_MEM_SS_READ_QGV_POINT_INFO(point)	(((point) << 16) | (0x1 << 8))
 #define   GEN6_PCODE_READ_D_COMP		0x10
 #define   GEN6_PCODE_WRITE_D_COMP		0x11
+#define   ICL_PCODE_EXIT_TCCOLD			0x12
 #define   HSW_PCODE_DE_WRITE_FREQ_REQ		0x17
 #define   DISPLAY_IPS_CONTROL			0x19
 #define   TGL_PCODE_TCCOLD				0x26
-- 
2.26.0

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

[Intel-gfx] [PATCH v3 6/6] drm/i915/dp: Get TC link reference during DP detection

[José Roberto de Souza]

As now the cost to lock and use a TC port is higher due the
implementation of the TCCOLD sequences it is worty to hold a reference
of the TC port to avoid all this locking at every aux transaction
part of the DisplayPort detection.

Cc: Imre Deak <imre.deak@intel.com>
Cc: Cooper Chiou <cooper.chiou@intel.com>
Cc: Kai-Heng Feng <kai.heng.feng@canonical.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 drivers/gpu/drm/i915/display/intel_dp.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
index 7f1a4e55cda1..6fbf3beee544 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -6041,6 +6041,7 @@ intel_dp_detect(struct drm_connector *connector,
 	struct intel_dp *intel_dp = intel_attached_dp(to_intel_connector(connector));
 	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
 	struct intel_encoder *encoder = &dig_port->base;
+	enum phy phy = intel_port_to_phy(dev_priv, encoder->port);
 	enum drm_connector_status status;
 
 	drm_dbg_kms(&dev_priv->drm, "[CONNECTOR:%d:%s]\n",
@@ -6049,12 +6050,17 @@ intel_dp_detect(struct drm_connector *connector,
 		    !drm_modeset_is_locked(&dev_priv->drm.mode_config.connection_mutex));
 
 	/* Can't disconnect eDP */
-	if (intel_dp_is_edp(intel_dp))
+	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;
+	} else {
+		if (intel_phy_is_tc(dev_priv, phy))
+			intel_tc_port_get_link(dig_port, 1);
+
+		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));
@@ -6132,6 +6138,9 @@ intel_dp_detect(struct drm_connector *connector,
 	if (status != connector_status_connected && !intel_dp->is_mst)
 		intel_dp_unset_edid(intel_dp);
 
+	if (intel_phy_is_tc(dev_priv, phy))
+		intel_tc_port_put_link(dig_port);
+
 	/*
 	 * Make sure the refs for power wells enabled during detect are
 	 * dropped to avoid a new detect cycle triggered by HPD polling.
-- 
2.26.0

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

[Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for series starting with [v3,1/6] drm/i915/tc/tgl: Implement TCCOLD sequences

[Patchwork]

== Series Details ==

Series: series starting with [v3,1/6] drm/i915/tc/tgl: Implement TCCOLD sequences
URL   : https://patchwork.freedesktop.org/series/75034/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
125d3dc52b2a drm/i915/tc/tgl: Implement TCCOLD sequences
ca5b103fb97c drm/i915/display: Add intel_display_power_get_without_ack()
bb6945d8d8cf drm/i915/display: Implement intel_display_power_wait_enable_ack()
4b5d60dcb2a5 drm/i915/display: Add intel_aux_ch_to_power_domain()
3b031f0ded4f drm/i915/tc/icl: Implement the TC cold exit sequence
-:161: WARNING:MSLEEP: msleep < 20ms can sleep for up to 20ms; see Documentation/timers/timers-howto.rst
#161: FILE: drivers/gpu/drm/i915/display/intel_tc.c:549:
+			msleep(1);

total: 0 errors, 1 warnings, 0 checks, 166 lines checked
ae9ffbf9a4cc drm/i915/dp: Get TC link reference during DP detection

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

Re: [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for series starting with [v3,1/6] drm/i915/tc/tgl: Implement TCCOLD sequences

[Souza, Jose]

On Tue, 2020-03-24 at 20:28 +0000, Patchwork wrote:
> == Series Details ==
> 
> Series: series starting with [v3,1/6] drm/i915/tc/tgl: Implement
> TCCOLD sequences
> URL   : https://patchwork.freedesktop.org/series/75034/
> State : warning
> 
> == Summary ==
> 
> $ dim checkpatch origin/drm-tip
> 125d3dc52b2a drm/i915/tc/tgl: Implement TCCOLD sequences
> ca5b103fb97c drm/i915/display: Add
> intel_display_power_get_without_ack()
> bb6945d8d8cf drm/i915/display: Implement
> intel_display_power_wait_enable_ack()
> 4b5d60dcb2a5 drm/i915/display: Add intel_aux_ch_to_power_domain()
> 3b031f0ded4f drm/i915/tc/icl: Implement the TC cold exit sequence
> -:161: WARNING:MSLEEP: msleep < 20ms can sleep for up to 20ms; see
> Documentation/timers/timers-howto.rst
> #161: FILE: drivers/gpu/drm/i915/display/intel_tc.c:549:
> +			msleep(1);

Left as is on purpose a x86 is usually configured as tickless and even
when not it supports HZ=1000 that matches 1ms.

> 
> total: 0 errors, 1 warnings, 0 checks, 166 lines checked
> ae9ffbf9a4cc drm/i915/dp: Get TC link reference during DP detection
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] ✓ Fi.CI.BAT: success for series starting with [v3,1/6] drm/i915/tc/tgl: Implement TCCOLD sequences

[Patchwork]

== Series Details ==

Series: series starting with [v3,1/6] drm/i915/tc/tgl: Implement TCCOLD sequences
URL   : https://patchwork.freedesktop.org/series/75034/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_8183 -> Patchwork_17073
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17073/index.html

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

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

### IGT changes ###

#### Issues hit ####

  * igt@gem_exec_suspend@basic-s3:
    - fi-icl-u2:          [PASS][1] -> [INCOMPLETE][2] ([i915#1185])
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8183/fi-icl-u2/igt@gem_exec_suspend@basic-s3.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17073/fi-icl-u2/igt@gem_exec_suspend@basic-s3.html

  * igt@i915_selftest@live@gtt:
    - fi-kbl-soraka:      [PASS][3] -> [INCOMPLETE][4] ([i915#1493])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8183/fi-kbl-soraka/igt@i915_selftest@live@gtt.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17073/fi-kbl-soraka/igt@i915_selftest@live@gtt.html

  
#### Warnings ####

  * igt@i915_selftest@live@gem_contexts:
    - fi-cfl-guc:         [INCOMPLETE][5] ([fdo#106070] / [i915#424]) -> [DMESG-FAIL][6] ([i915#481])
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8183/fi-cfl-guc/igt@i915_selftest@live@gem_contexts.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17073/fi-cfl-guc/igt@i915_selftest@live@gem_contexts.html

  
  [fdo#106070]: https://bugs.freedesktop.org/show_bug.cgi?id=106070
  [i915#1185]: https://gitlab.freedesktop.org/drm/intel/issues/1185
  [i915#1493]: https://gitlab.freedesktop.org/drm/intel/issues/1493
  [i915#424]: https://gitlab.freedesktop.org/drm/intel/issues/424
  [i915#481]: https://gitlab.freedesktop.org/drm/intel/issues/481


Participating hosts (45 -> 40)
------------------------------

  Additional (4): fi-skl-lmem fi-blb-e6850 fi-cfl-8109u fi-kbl-7500u 
  Missing    (9): fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-cfl-8700k fi-ctg-p8600 fi-gdg-551 fi-byt-clapper fi-bsw-nick fi-bdw-samus 


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

  * CI: CI-20190529 -> None
  * Linux: CI_DRM_8183 -> Patchwork_17073

  CI-20190529: 20190529
  CI_DRM_8183: 795894daf2cc32246af94541733e08649d082470 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_5535: d1dcf40cc6869ac858586c5ad9f09af6617ce2ee @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_17073: ae9ffbf9a4cc21805d5481a736c331b8140b4ba9 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

ae9ffbf9a4cc drm/i915/dp: Get TC link reference during DP detection
3b031f0ded4f drm/i915/tc/icl: Implement the TC cold exit sequence
4b5d60dcb2a5 drm/i915/display: Add intel_aux_ch_to_power_domain()
bb6945d8d8cf drm/i915/display: Implement intel_display_power_wait_enable_ack()
ca5b103fb97c drm/i915/display: Add intel_display_power_get_without_ack()
125d3dc52b2a drm/i915/tc/tgl: Implement TCCOLD sequences

== Logs ==

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

[Intel-gfx] ✓ Fi.CI.IGT: success for series starting with [v3,1/6] drm/i915/tc/tgl: Implement TCCOLD sequences

[Patchwork]

== Series Details ==

Series: series starting with [v3,1/6] drm/i915/tc/tgl: Implement TCCOLD sequences
URL   : https://patchwork.freedesktop.org/series/75034/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_8183_full -> Patchwork_17073_full
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  

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

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

### IGT changes ###

#### Issues hit ####

  * igt@gem_ctx_isolation@vecs0-s3:
    - shard-skl:          [PASS][1] -> [INCOMPLETE][2] ([i915#69]) +1 similar issue
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8183/shard-skl10/igt@gem_ctx_isolation@vecs0-s3.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17073/shard-skl5/igt@gem_ctx_isolation@vecs0-s3.html

  * igt@gem_exec_schedule@implicit-write-read-bsd1:
    - shard-iclb:         [PASS][3] -> [SKIP][4] ([fdo#109276] / [i915#677]) +1 similar issue
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8183/shard-iclb4/igt@gem_exec_schedule@implicit-write-read-bsd1.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17073/shard-iclb6/igt@gem_exec_schedule@implicit-write-read-bsd1.html

  * igt@gem_exec_schedule@in-order-bsd:
    - shard-iclb:         [PASS][5] -> [SKIP][6] ([fdo#112146]) +2 similar issues
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8183/shard-iclb5/igt@gem_exec_schedule@in-order-bsd.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17073/shard-iclb2/igt@gem_exec_schedule@in-order-bsd.html

  * igt@gem_exec_schedule@pi-ringfull-bsd2:
    - shard-iclb:         [PASS][7] -> [SKIP][8] ([fdo#109276]) +8 similar issues
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8183/shard-iclb4/igt@gem_exec_schedule@pi-ringfull-bsd2.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17073/shard-iclb6/igt@gem_exec_schedule@pi-ringfull-bsd2.html

  * igt@gem_linear_blits@interruptible:
    - shard-apl:          [PASS][9] -> [FAIL][10] ([i915#1263])
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8183/shard-apl7/igt@gem_linear_blits@interruptible.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17073/shard-apl4/igt@gem_linear_blits@interruptible.html

  * igt@i915_selftest@live@execlists:
    - shard-apl:          [PASS][11] -> [INCOMPLETE][12] ([fdo#103927] / [i915#656])
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8183/shard-apl4/igt@i915_selftest@live@execlists.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17073/shard-apl7/igt@i915_selftest@live@execlists.html

  * igt@kms_cursor_crc@pipe-b-cursor-256x85-sliding:
    - shard-skl:          [PASS][13] -> [FAIL][14] ([i915#54])
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8183/shard-skl3/igt@kms_cursor_crc@pipe-b-cursor-256x85-sliding.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17073/shard-skl6/igt@kms_cursor_crc@pipe-b-cursor-256x85-sliding.html

  * igt@kms_flip@flip-vs-expired-vblank-interruptible:
    - shard-skl:          [PASS][15] -> [FAIL][16] ([i915#79])
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8183/shard-skl6/igt@kms_flip@flip-vs-expired-vblank-interruptible.html
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17073/shard-skl9/igt@kms_flip@flip-vs-expired-vblank-interruptible.html

  * igt@kms_flip@flip-vs-suspend:
    - shard-skl:          [PASS][17] -> [INCOMPLETE][18] ([i915#221])
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8183/shard-skl2/igt@kms_flip@flip-vs-suspend.html
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17073/shard-skl1/igt@kms_flip@flip-vs-suspend.html

  * igt@kms_flip@flip-vs-suspend-interruptible:
    - shard-kbl:          [PASS][19] -> [DMESG-WARN][20] ([i915#180]) +1 similar issue
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8183/shard-kbl3/igt@kms_flip@flip-vs-suspend-interruptible.html
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17073/shard-kbl7/igt@kms_flip@flip-vs-suspend-interruptible.html

  * igt@kms_plane_alpha_blend@pipe-a-coverage-7efc:
    - shard-skl:          [PASS][21] -> [FAIL][22] ([fdo#108145])
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8183/shard-skl5/igt@kms_plane_alpha_blend@pipe-a-coverage-7efc.html
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17073/shard-skl3/igt@kms_plane_alpha_blend@pipe-a-coverage-7efc.html

  * igt@kms_plane_alpha_blend@pipe-c-coverage-7efc:
    - shard-skl:          [PASS][23] -> [FAIL][24] ([fdo#108145] / [i915#265]) +1 similar issue
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8183/shard-skl9/igt@kms_plane_alpha_blend@pipe-c-coverage-7efc.html
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17073/shard-skl8/igt@kms_plane_alpha_blend@pipe-c-coverage-7efc.html

  * igt@kms_psr2_su@page_flip:
    - shard-iclb:         [PASS][25] -> [SKIP][26] ([fdo#109642] / [fdo#111068])
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8183/shard-iclb2/igt@kms_psr2_su@page_flip.html
   [26]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17073/shard-iclb1/igt@kms_psr2_su@page_flip.html

  * igt@kms_psr@psr2_dpms:
    - shard-iclb:         [PASS][27] -> [SKIP][28] ([fdo#109441]) +1 similar issue
   [27]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8183/shard-iclb2/igt@kms_psr@psr2_dpms.html
   [28]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17073/shard-iclb7/igt@kms_psr@psr2_dpms.html

  * igt@kms_psr@suspend:
    - shard-skl:          [PASS][29] -> [INCOMPLETE][30] ([i915#198])
   [29]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8183/shard-skl7/igt@kms_psr@suspend.html
   [30]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17073/shard-skl5/igt@kms_psr@suspend.html

  * igt@perf_pmu@idle-vcs1:
    - shard-iclb:         [PASS][31] -> [SKIP][32] ([fdo#112080]) +5 similar issues
   [31]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8183/shard-iclb2/igt@perf_pmu@idle-vcs1.html
   [32]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17073/shard-iclb7/igt@perf_pmu@idle-vcs1.html

  
#### Possible fixes ####

  * igt@gem_ctx_shared@exec-single-timeline-bsd:
    - shard-iclb:         [SKIP][33] ([fdo#110841]) -> [PASS][34]
   [33]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8183/shard-iclb2/igt@gem_ctx_shared@exec-single-timeline-bsd.html
   [34]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17073/shard-iclb7/igt@gem_ctx_shared@exec-single-timeline-bsd.html

  * igt@gem_exec_parallel@contexts:
    - shard-tglb:         [INCOMPLETE][35] ([i915#470] / [i915#529]) -> [PASS][36]
   [35]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8183/shard-tglb2/igt@gem_exec_parallel@contexts.html
   [36]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17073/shard-tglb6/igt@gem_exec_parallel@contexts.html

  * igt@gem_exec_schedule@independent-bsd:
    - shard-iclb:         [SKIP][37] ([fdo#112146]) -> [PASS][38]
   [37]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8183/shard-iclb2/igt@gem_exec_schedule@independent-bsd.html
   [38]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17073/shard-iclb7/igt@gem_exec_schedule@independent-bsd.html

  * igt@gem_ringfill@basic-default-hang:
    - shard-tglb:         [INCOMPLETE][39] -> [PASS][40]
   [39]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8183/shard-tglb6/igt@gem_ringfill@basic-default-hang.html
   [40]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17073/shard-tglb2/igt@gem_ringfill@basic-default-hang.html

  * igt@i915_suspend@fence-restore-tiled2untiled:
    - shard-apl:          [DMESG-WARN][41] ([i915#180]) -> [PASS][42] +5 similar issues
   [41]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8183/shard-apl4/igt@i915_suspend@fence-restore-tiled2untiled.html
   [42]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17073/shard-apl1/igt@i915_suspend@fence-restore-tiled2untiled.html

  * igt@kms_cursor_crc@pipe-a-cursor-64x64-random:
    - shard-apl:          [FAIL][43] ([i915#54]) -> [PASS][44]
   [43]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8183/shard-apl3/igt@kms_cursor_crc@pipe-a-cursor-64x64-random.html
   [44]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17073/shard-apl4/igt@kms_cursor_crc@pipe-a-cursor-64x64-random.html

  * igt@kms_cursor_crc@pipe-a-cursor-suspend:
    - shard-kbl:          [DMESG-WARN][45] ([i915#180]) -> [PASS][46] +3 similar issues
   [45]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8183/shard-kbl1/igt@kms_cursor_crc@pipe-a-cursor-suspend.html
   [46]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17073/shard-kbl7/igt@kms_cursor_crc@pipe-a-cursor-suspend.html

  * igt@kms_cursor_crc@pipe-c-cursor-64x21-random:
    - shard-glk:          [FAIL][47] ([i915#54]) -> [PASS][48]
   [47]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8183/shard-glk8/igt@kms_cursor_crc@pipe-c-cursor-64x21-random.html
   [48]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17073/shard-glk4/igt@kms_cursor_crc@pipe-c-cursor-64x21-random.html

  * igt@kms_flip@2x-plain-flip-fb-recreate-interruptible:
    - shard-glk:          [FAIL][49] ([i915#34]) -> [PASS][50]
   [49]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8183/shard-glk4/igt@kms_flip@2x-plain-flip-fb-recreate-interruptible.html
   [50]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17073/shard-glk3/igt@kms_flip@2x-plain-flip-fb-recreate-interruptible.html

  * igt@kms_psr@psr2_basic:
    - shard-iclb:         [SKIP][51] ([fdo#109441]) -> [PASS][52]
   [51]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8183/shard-iclb3/igt@kms_psr@psr2_basic.html
   [52]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17073/shard-iclb2/igt@kms_psr@psr2_basic.html

  * igt@kms_setmode@basic:
    - shard-skl:          [FAIL][53] ([i915#31]) -> [PASS][54]
   [53]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8183/shard-skl6/igt@kms_setmode@basic.html
   [54]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17073/shard-skl9/igt@kms_setmode@basic.html

  * igt@perf_pmu@busy-check-all-vcs1:
    - shard-iclb:         [SKIP][55] ([fdo#112080]) -> [PASS][56] +3 similar issues
   [55]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8183/shard-iclb8/igt@perf_pmu@busy-check-all-vcs1.html
   [56]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17073/shard-iclb4/igt@perf_pmu@busy-check-all-vcs1.html

  * igt@prime_vgem@fence-wait-bsd2:
    - shard-iclb:         [SKIP][57] ([fdo#109276]) -> [PASS][58] +11 similar issues
   [57]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8183/shard-iclb8/igt@prime_vgem@fence-wait-bsd2.html
   [58]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17073/shard-iclb4/igt@prime_vgem@fence-wait-bsd2.html

  
#### Warnings ####

  * igt@i915_pm_rc6_residency@rc6-idle:
    - shard-iclb:         [WARN][59] ([i915#1515]) -> [FAIL][60] ([i915#1515])
   [59]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8183/shard-iclb3/igt@i915_pm_rc6_residency@rc6-idle.html
   [60]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17073/shard-iclb8/igt@i915_pm_rc6_residency@rc6-idle.html

  * igt@i915_pm_rpm@cursor:
    - shard-snb:          [INCOMPLETE][61] ([i915#82]) -> [SKIP][62] ([fdo#109271]) +1 similar issue
   [61]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8183/shard-snb2/igt@i915_pm_rpm@cursor.html
   [62]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17073/shard-snb4/igt@i915_pm_rpm@cursor.html

  * igt@runner@aborted:
    - shard-apl:          [FAIL][63] ([fdo#103927]) -> ([FAIL][64], [FAIL][65]) ([fdo#103927] / [i915#529])
   [63]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8183/shard-apl4/igt@runner@aborted.html
   [64]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17073/shard-apl7/igt@runner@aborted.html
   [65]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17073/shard-apl7/igt@runner@aborted.html

  
  [fdo#103927]: https://bugs.freedesktop.org/show_bug.cgi?id=103927
  [fdo#108145]: https://bugs.freedesktop.org/show_bug.cgi?id=108145
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109276]: https://bugs.freedesktop.org/show_bug.cgi?id=109276
  [fdo#109441]: https://bugs.freedesktop.org/show_bug.cgi?id=109441
  [fdo#109642]: https://bugs.freedesktop.org/show_bug.cgi?id=109642
  [fdo#110841]: https://bugs.freedesktop.org/show_bug.cgi?id=110841
  [fdo#111068]: https://bugs.freedesktop.org/show_bug.cgi?id=111068
  [fdo#112080]: https://bugs.freedesktop.org/show_bug.cgi?id=112080
  [fdo#112146]: https://bugs.freedesktop.org/show_bug.cgi?id=112146
  [i915#1263]: https://gitlab.freedesktop.org/drm/intel/issues/1263
  [i915#1515]: https://gitlab.freedesktop.org/drm/intel/issues/1515
  [i915#180]: https://gitlab.freedesktop.org/drm/intel/issues/180
  [i915#198]: https://gitlab.freedesktop.org/drm/intel/issues/198
  [i915#221]: https://gitlab.freedesktop.org/drm/intel/issues/221
  [i915#265]: https://gitlab.freedesktop.org/drm/intel/issues/265
  [i915#31]: https://gitlab.freedesktop.org/drm/intel/issues/31
  [i915#34]: https://gitlab.freedesktop.org/drm/intel/issues/34
  [i915#470]: https://gitlab.freedesktop.org/drm/intel/issues/470
  [i915#529]: https://gitlab.freedesktop.org/drm/intel/issues/529
  [i915#54]: https://gitlab.freedesktop.org/drm/intel/issues/54
  [i915#656]: https://gitlab.freedesktop.org/drm/intel/issues/656
  [i915#677]: https://gitlab.freedesktop.org/drm/intel/issues/677
  [i915#69]: https://gitlab.freedesktop.org/drm/intel/issues/69
  [i915#79]: https://gitlab.freedesktop.org/drm/intel/issues/79
  [i915#82]: https://gitlab.freedesktop.org/drm/intel/issues/82


Participating hosts (10 -> 10)
------------------------------

  No changes in participating hosts


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

  * CI: CI-20190529 -> None
  * Linux: CI_DRM_8183 -> Patchwork_17073

  CI-20190529: 20190529
  CI_DRM_8183: 795894daf2cc32246af94541733e08649d082470 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_5535: d1dcf40cc6869ac858586c5ad9f09af6617ce2ee @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_17073: ae9ffbf9a4cc21805d5481a736c331b8140b4ba9 @ 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_17073/index.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v3 1/6] drm/i915/tc/tgl: Implement TCCOLD sequences

[You-Sheng Yang]

[-- Attachment #1.1.1: Type: text/plain, Size: 57 bytes --]

Tested-by: You-Sheng Yang <vicamo.yang@canonical.com>


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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

Re: [Intel-gfx] [PATCH v3 1/6] drm/i915/tc/tgl: Implement TCCOLD sequences

[Imre Deak]

Hi José,

On Tue, Mar 24, 2020 at 01:14:24PM -0700, José Roberto de Souza wrote:
> TC ports can enter in TCCOLD to save power and is required to request
> to PCODE to exit this state before use or read to TC registers.
> 
> For TGL there is a new MBOX command to do that with a parameter to ask
> PCODE to exit and block TCCOLD entry or unblock TCCOLD entry.
> For GEN11 the sequence is more complex and will be handled in a
> separated patch.
> 
> BSpec: 49294
> Cc: Imre Deak <imre.deak@intel.com>
> Cc: Cooper Chiou <cooper.chiou@intel.com>
> Cc: Kai-Heng Feng <kai.heng.feng@canonical.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_tc.c | 61 ++++++++++++++++++++++++-
>  drivers/gpu/drm/i915/i915_reg.h         |  3 ++
>  drivers/gpu/drm/i915/intel_sideband.c   | 22 +++++++++
>  drivers/gpu/drm/i915/intel_sideband.h   |  4 ++
>  4 files changed, 88 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_tc.c b/drivers/gpu/drm/i915/display/intel_tc.c
> index 9b850c11aa78..e4c5de5ce874 100644
> --- a/drivers/gpu/drm/i915/display/intel_tc.c
> +++ b/drivers/gpu/drm/i915/display/intel_tc.c
> @@ -7,6 +7,7 @@
>  #include "intel_display.h"
>  #include "intel_display_types.h"
>  #include "intel_dp_mst.h"
> +#include "intel_sideband.h"
>  #include "intel_tc.h"
>  
>  static const char *tc_port_mode_name(enum tc_port_mode mode)
> @@ -496,6 +497,55 @@ bool intel_tc_port_connected(struct intel_digital_port *dig_port)
>  	return is_connected;
>  }
>  
> +static inline int tgl_tc_cold_request(struct intel_digital_port *dig_port,
> +				      bool block)
> +{
> +	struct drm_i915_private *i915 = to_i915(dig_port->base.base.dev);
> +	u32 low_val, high_val;
> +	u8 tries = 0;
> +	int ret;
> +
> +	do {
> +		low_val = 0;
> +		high_val = block ? 0 : TGL_PCODE_EXIT_TCCOLD_DATA_H_UNBLOCK_REQ;
> +
> +		ret = sandybridge_pcode_write_read_timeout(i915,
> +							   TGL_PCODE_TCCOLD,
> +							   &low_val, &high_val,
> +							   150, 1);

The spec says trying 3 times for 200 usec. Is there a problem if we just
reuse sandybridge_pcode_read() here which has a 500 usec timeout?

> +		if (ret == 0) {
> +			if (block &&
> +			    low_val & TGL_PCODE_EXIT_TCCOLD_DATA_L_EXIT_FAILED)
> +				ret = -EIO;
> +			else
> +				break;
> +		}
> +
> +		if (ret != -EAGAIN)
> +			tries++;
> +	} while (tries < 3);
> +
> +	return ret;

The return value isn't used and I think we can't do much about it, so
just make the function a void type and warn about a timeout?

> +}
> +
> +static int tc_cold_request(struct intel_digital_port *dig_port, bool block)
> +{
> +	struct drm_i915_private *i915 = to_i915(dig_port->base.base.dev);
> +	int ret;
> +
> +	if (INTEL_GEN(i915) >= 12)
> +		ret = tgl_tc_cold_request(dig_port, block);
> +	else
> +		/* TODO: implement GEN11 TCCOLD sequences */
> +		ret = 0;
> +
> +	drm_dbg_kms(&i915->drm, "Port %s: TCCOLD %sblock %s\n",
> +		    dig_port->tc_port_name, (block ? "" : "un"),
> +		    (ret == 0 ? "succeeded" : "failed"));
> +
> +	return ret;
> +}
> +
>  static void __intel_tc_port_lock(struct intel_digital_port *dig_port,
>  				 int required_lanes)
>  {
> @@ -506,9 +556,11 @@ static void __intel_tc_port_lock(struct intel_digital_port *dig_port,
>  
>  	mutex_lock(&dig_port->tc_lock);
>  
> -	if (!dig_port->tc_link_refcount &&
> -	    intel_tc_port_needs_reset(dig_port))
> +	if (dig_port->tc_link_refcount == 0) {
> +		tc_cold_request(dig_port, true);

I'm not sure if PCODE really refcounts the requests what this would need
(since we submit the same request for all ports). But for other reasons,
like the overhead of the request and the AUX PW ack trickery we need on
ICL, I think we need a tc_cold_off power well/domain. The tc_cold_off
power ref would be get/put around the FIA access sequence here
(intel_tc_port_reset_mode()) and would be held whenever we hold an AUX
power ref.

ICL legacy ports would hold an AUX power ref around the FIA access here
and the AUX power well code would internally do the PCODE request and
deal with the delayed power well ack (so we don't need to create a new
interface for that).

> +		intel_tc_port_needs_reset(dig_port);
>  		intel_tc_port_reset_mode(dig_port, required_lanes);
> +	}
>  
>  	drm_WARN_ON(&i915->drm, dig_port->tc_lock_wakeref);
>  	dig_port->tc_lock_wakeref = wakeref;
> @@ -524,6 +576,9 @@ void intel_tc_port_unlock(struct intel_digital_port *dig_port)
>  	struct drm_i915_private *i915 = to_i915(dig_port->base.base.dev);
>  	intel_wakeref_t wakeref = fetch_and_zero(&dig_port->tc_lock_wakeref);
>  
> +	if (dig_port->tc_link_refcount == 0)
> +		tc_cold_request(dig_port, false);
> +
>  	mutex_unlock(&dig_port->tc_lock);
>  
>  	intel_display_power_put_async(i915, POWER_DOMAIN_DISPLAY_CORE,
> @@ -548,6 +603,8 @@ void intel_tc_port_put_link(struct intel_digital_port *dig_port)
>  {
>  	mutex_lock(&dig_port->tc_lock);
>  	dig_port->tc_link_refcount--;
> +	if (dig_port->tc_link_refcount == 0)
> +		tc_cold_request(dig_port, false);
>  	mutex_unlock(&dig_port->tc_lock);
>  }
>  
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 9c53fe918be6..7e341d9945b3 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -9019,6 +9019,9 @@ enum {
>  #define   GEN6_PCODE_WRITE_D_COMP		0x11
>  #define   HSW_PCODE_DE_WRITE_FREQ_REQ		0x17
>  #define   DISPLAY_IPS_CONTROL			0x19
> +#define   TGL_PCODE_TCCOLD				0x26
> +#define     TGL_PCODE_EXIT_TCCOLD_DATA_L_EXIT_FAILED	REG_BIT(0)
> +#define     TGL_PCODE_EXIT_TCCOLD_DATA_H_UNBLOCK_REQ	REG_BIT(0)
>              /* See also IPS_CTL */
>  #define     IPS_PCODE_CONTROL			(1 << 30)
>  #define   HSW_PCODE_DYNAMIC_DUTY_CYCLE_CONTROL	0x1A
> diff --git a/drivers/gpu/drm/i915/intel_sideband.c b/drivers/gpu/drm/i915/intel_sideband.c
> index 1447e7516cb7..20a9d3970930 100644
> --- a/drivers/gpu/drm/i915/intel_sideband.c
> +++ b/drivers/gpu/drm/i915/intel_sideband.c
> @@ -463,6 +463,28 @@ int sandybridge_pcode_write_timeout(struct drm_i915_private *i915,
>  	return err;
>  }
>  
> +int sandybridge_pcode_write_read_timeout(struct drm_i915_private *i915,
> +					 u32 mbox, u32 *val, u32 *val1,
> +					 int fast_timeout_us,
> +					 int slow_timeout_ms)
> +{
> +	int err;
> +
> +	mutex_lock(&i915->sb_lock);
> +	err = __sandybridge_pcode_rw(i915, mbox, val, val1,
> +				     fast_timeout_us, slow_timeout_ms,
> +				     true);
> +	mutex_unlock(&i915->sb_lock);
> +
> +	if (err) {
> +		drm_dbg(&i915->drm,
> +			"warning: pcode (write of 0x%08x to mbox %x) mailbox access failed for %ps: %d\n",
> +			*val, mbox, __builtin_return_address(0), err);
> +	}
> +
> +	return err;
> +}
> +
>  static bool skl_pcode_try_request(struct drm_i915_private *i915, u32 mbox,
>  				  u32 request, u32 reply_mask, u32 reply,
>  				  u32 *status)
> diff --git a/drivers/gpu/drm/i915/intel_sideband.h b/drivers/gpu/drm/i915/intel_sideband.h
> index 7fb95745a444..1939bebb4e67 100644
> --- a/drivers/gpu/drm/i915/intel_sideband.h
> +++ b/drivers/gpu/drm/i915/intel_sideband.h
> @@ -132,6 +132,10 @@ int sandybridge_pcode_read(struct drm_i915_private *i915, u32 mbox,
>  int sandybridge_pcode_write_timeout(struct drm_i915_private *i915, u32 mbox,
>  				    u32 val, int fast_timeout_us,
>  				    int slow_timeout_ms);
> +int sandybridge_pcode_write_read_timeout(struct drm_i915_private *i915,
> +					 u32 mbox, u32 *val, u32 *val1,
> +					 int fast_timeout_us,
> +					 int slow_timeout_ms);
>  #define sandybridge_pcode_write(i915, mbox, val)	\
>  	sandybridge_pcode_write_timeout(i915, mbox, val, 500, 0)
>  
> -- 
> 2.26.0
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v3 2/6] drm/i915/display: Add intel_display_power_get_without_ack()

[Imre Deak]

On Tue, Mar 24, 2020 at 01:14:25PM -0700, José Roberto de Souza wrote:
> To implement ICL TC static sequences is required to get the port aux
> powerwell without wait for hardware ack.

I think we should block tc_cold whenever holding a legacy AUX power ref
and handle the delayed ack in the power_well->enable hook.

> 
> Cc: Imre Deak <imre.deak@intel.com>
> Cc: Cooper Chiou <cooper.chiou@intel.com>
> Cc: Kai-Heng Feng <kai.heng.feng@canonical.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
>  .../drm/i915/display/intel_display_power.c    | 71 +++++++++++++++----
>  .../drm/i915/display/intel_display_power.h    | 12 ++++
>  2 files changed, 71 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c b/drivers/gpu/drm/i915/display/intel_display_power.c
> index 246e406bb385..9035b220dfa0 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_power.c
> +++ b/drivers/gpu/drm/i915/display/intel_display_power.c
> @@ -157,14 +157,24 @@ intel_display_power_domain_str(enum intel_display_power_domain domain)
>  	}
>  }
>  
> -static void intel_power_well_enable(struct drm_i915_private *dev_priv,
> -				    struct i915_power_well *power_well)
> +static void _intel_power_well_enable(struct drm_i915_private *dev_priv,
> +				     struct i915_power_well *power_well,
> +				     bool wait_ack)
>  {
>  	drm_dbg_kms(&dev_priv->drm, "enabling %s\n", power_well->desc->name);
> -	power_well->desc->ops->enable(dev_priv, power_well);
> +	if (wait_ack || !power_well->desc->ops->enable_without_ack)
> +		power_well->desc->ops->enable(dev_priv, power_well);
> +	else
> +		power_well->desc->ops->enable_without_ack(dev_priv, power_well);
>  	power_well->hw_enabled = true;
>  }
>  
> +static void intel_power_well_enable(struct drm_i915_private *dev_priv,
> +				    struct i915_power_well *power_well)
> +{
> +	_intel_power_well_enable(dev_priv, power_well, true);
> +}
> +
>  static void intel_power_well_disable(struct drm_i915_private *dev_priv,
>  				     struct i915_power_well *power_well)
>  {
> @@ -174,10 +184,11 @@ static void intel_power_well_disable(struct drm_i915_private *dev_priv,
>  }
>  
>  static void intel_power_well_get(struct drm_i915_private *dev_priv,
> -				 struct i915_power_well *power_well)
> +				 struct i915_power_well *power_well,
> +				 bool wait_ack)
>  {
>  	if (!power_well->count++)
> -		intel_power_well_enable(dev_priv, power_well);
> +		_intel_power_well_enable(dev_priv, power_well, wait_ack);
>  }
>  
>  static void intel_power_well_put(struct drm_i915_private *dev_priv,
> @@ -353,8 +364,9 @@ static void gen9_wait_for_power_well_fuses(struct drm_i915_private *dev_priv,
>  					  SKL_FUSE_PG_DIST_STATUS(pg), 1));
>  }
>  
> -static void hsw_power_well_enable(struct drm_i915_private *dev_priv,
> -				  struct i915_power_well *power_well)
> +static void _hsw_power_well_enable(struct drm_i915_private *dev_priv,
> +				   struct i915_power_well *power_well,
> +				   bool wait_ack)
>  {
>  	const struct i915_power_well_regs *regs = power_well->desc->hsw.regs;
>  	int pw_idx = power_well->desc->hsw.idx;
> @@ -379,7 +391,8 @@ static void hsw_power_well_enable(struct drm_i915_private *dev_priv,
>  	val = intel_de_read(dev_priv, regs->driver);
>  	intel_de_write(dev_priv, regs->driver,
>  		       val | HSW_PWR_WELL_CTL_REQ(pw_idx));
> -	hsw_wait_for_power_well_enable(dev_priv, power_well);
> +	if (wait_ack)
> +		hsw_wait_for_power_well_enable(dev_priv, power_well);
>  
>  	/* Display WA #1178: cnl */
>  	if (IS_CANNONLAKE(dev_priv) &&
> @@ -398,6 +411,12 @@ static void hsw_power_well_enable(struct drm_i915_private *dev_priv,
>  				   power_well->desc->hsw.has_vga);
>  }
>  
> +static void hsw_power_well_enable(struct drm_i915_private *dev_priv,
> +				  struct i915_power_well *power_well)
> +{
> +	_hsw_power_well_enable(dev_priv, power_well, true);
> +}
> +
>  static void hsw_power_well_disable(struct drm_i915_private *dev_priv,
>  				   struct i915_power_well *power_well)
>  {
> @@ -1960,7 +1979,8 @@ intel_display_power_grab_async_put_ref(struct drm_i915_private *dev_priv,
>  
>  static void
>  __intel_display_power_get_domain(struct drm_i915_private *dev_priv,
> -				 enum intel_display_power_domain domain)
> +				 enum intel_display_power_domain domain,
> +				 bool wait_ack)
>  {
>  	struct i915_power_domains *power_domains = &dev_priv->power_domains;
>  	struct i915_power_well *power_well;
> @@ -1969,7 +1989,7 @@ __intel_display_power_get_domain(struct drm_i915_private *dev_priv,
>  		return;
>  
>  	for_each_power_domain_well(dev_priv, power_well, BIT_ULL(domain))
> -		intel_power_well_get(dev_priv, power_well);
> +		intel_power_well_get(dev_priv, power_well, wait_ack);
>  
>  	power_domains->domain_use_count[domain]++;
>  }
> @@ -1993,7 +2013,34 @@ intel_wakeref_t intel_display_power_get(struct drm_i915_private *dev_priv,
>  	intel_wakeref_t wakeref = intel_runtime_pm_get(&dev_priv->runtime_pm);
>  
>  	mutex_lock(&power_domains->lock);
> -	__intel_display_power_get_domain(dev_priv, domain);
> +	__intel_display_power_get_domain(dev_priv, domain, true);
> +	mutex_unlock(&power_domains->lock);
> +
> +	return wakeref;
> +}
> +
> +/**
> + * intel_display_power_get_without_ack - grab a power domain reference without
> + * wait for HW ack
> + * @dev_priv: i915 device instance
> + * @domain: power domain to reference
> + *
> + * This function grabs a power domain reference for @domain and ensures that the
> + * power domain and all its parents are powered up but it don't wait for
> + * hardware ack if supported by each powerwell. Users should only grab a
> + * reference to the innermost power domain they need.
> + *
> + * Any power domain reference obtained by this function must have a symmetric
> + * call to intel_display_power_put() to release the reference again.
> + */
> +intel_wakeref_t intel_display_power_get_without_ack(struct drm_i915_private *dev_priv,
> +						    enum intel_display_power_domain domain)
> +{
> +	struct i915_power_domains *power_domains = &dev_priv->power_domains;
> +	intel_wakeref_t wakeref = intel_runtime_pm_get(&dev_priv->runtime_pm);
> +
> +	mutex_lock(&power_domains->lock);
> +	__intel_display_power_get_domain(dev_priv, domain, false);
>  	mutex_unlock(&power_domains->lock);
>  
>  	return wakeref;
> @@ -2026,7 +2073,7 @@ intel_display_power_get_if_enabled(struct drm_i915_private *dev_priv,
>  	mutex_lock(&power_domains->lock);
>  
>  	if (__intel_display_power_is_enabled(dev_priv, domain)) {
> -		__intel_display_power_get_domain(dev_priv, domain);
> +		__intel_display_power_get_domain(dev_priv, domain, true);
>  		is_enabled = true;
>  	} else {
>  		is_enabled = false;
> diff --git a/drivers/gpu/drm/i915/display/intel_display_power.h b/drivers/gpu/drm/i915/display/intel_display_power.h
> index da64a5edae7a..5db86cc862c3 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_power.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_power.h
> @@ -129,6 +129,16 @@ struct i915_power_well_ops {
>  	 */
>  	void (*enable)(struct drm_i915_private *dev_priv,
>  		       struct i915_power_well *power_well);
> +
> +	/*
> +	 * Enable the well and resources that depend on it (for example
> +	 * interrupts located on the well) without reading HW ack. Called after
> +	 * the 0->1 refcount transition.
> +	 * This will be used by TC subsystem and it is a optional hook.
> +	 */
> +	void (*enable_without_ack)(struct drm_i915_private *dev_priv,
> +				   struct i915_power_well *power_well);
> +
>  	/*
>  	 * Disable the well and resources that depend on it. Called after
>  	 * the 1->0 refcount transition.
> @@ -270,6 +280,8 @@ bool __intel_display_power_is_enabled(struct drm_i915_private *dev_priv,
>  				      enum intel_display_power_domain domain);
>  intel_wakeref_t intel_display_power_get(struct drm_i915_private *dev_priv,
>  					enum intel_display_power_domain domain);
> +intel_wakeref_t intel_display_power_get_without_ack(struct drm_i915_private *dev_priv,
> +						    enum intel_display_power_domain domain);
>  intel_wakeref_t
>  intel_display_power_get_if_enabled(struct drm_i915_private *dev_priv,
>  				   enum intel_display_power_domain domain);
> -- 
> 2.26.0
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v3 5/6] drm/i915/tc/icl: Implement the TC cold exit sequence

[Imre Deak]

On Tue, Mar 24, 2020 at 01:14:28PM -0700, José Roberto de Souza wrote:
> This is required for legacy/static TC ports as IOM is not aware of
> the connection and will not trigger the TC cold exit.
> 
> Just request PCODE to exit TCCOLD is not enough as it could enter
> again be driver makes use of the port, to prevent it BSpec states that
> aux powerwell should be held.
> 
> So before detecting the mode, aux power is requested without wait for
> hardware ack, PCODE is requested to exit TCCOLD and the TC detection
> sequences follows as normal.
> After detection if mode is not static aux can be powered off otherwise
> we need to wait for HW ack as future calls to intel_display_power_get()
> over aux will not check for HW ack.

Based on the earlier comments tc_cold_off would be requested any time a
legacy AUX power ref is acquired, so we wouldn't need a new no_ack power
well interface.

> 
> v2:
> - fixed typo tc_lock_wakeref to tc_cold_wakeref in icl_tc_cold_request()
> 
> v3:
> - fixed non initialized ret in icl_tc_cold_request()
> - added missing sleep step, initially it was not added because I
> thought that the aux enable and then HW ack wait would take care of
> that but that is not the case
> 
> BSpec: 21750
> Fixes: https://gitlab.freedesktop.org/drm/intel/issues/1296
> Cc: Imre Deak <imre.deak@intel.com>
> Cc: Cooper Chiou <cooper.chiou@intel.com>
> Cc: Kai-Heng Feng <kai.heng.feng@canonical.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
>  .../drm/i915/display/intel_display_power.c    | 30 +++++++++-
>  .../drm/i915/display/intel_display_types.h    |  1 +
>  drivers/gpu/drm/i915/display/intel_tc.c       | 60 +++++++++++++++++--
>  drivers/gpu/drm/i915/i915_reg.h               |  1 +
>  4 files changed, 84 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c b/drivers/gpu/drm/i915/display/intel_display_power.c
> index a7e531b64e16..71a4c5d790ea 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_power.c
> +++ b/drivers/gpu/drm/i915/display/intel_display_power.c
> @@ -573,8 +573,9 @@ static void icl_tc_port_assert_ref_held(struct drm_i915_private *dev_priv,
>  #define TGL_AUX_PW_TO_TC_PORT(pw_idx)	((pw_idx) - TGL_PW_CTL_IDX_AUX_TC1)
>  
>  static void
> -icl_tc_phy_aux_power_well_enable(struct drm_i915_private *dev_priv,
> -				 struct i915_power_well *power_well)
> +_icl_tc_phy_aux_power_well_enable(struct drm_i915_private *dev_priv,
> +				  struct i915_power_well *power_well,
> +				  bool wait_ack)
>  {
>  	enum aux_ch aux_ch = icl_tc_phy_aux_ch(dev_priv, power_well);
>  	u32 val;
> @@ -587,7 +588,7 @@ icl_tc_phy_aux_power_well_enable(struct drm_i915_private *dev_priv,
>  		val |= DP_AUX_CH_CTL_TBT_IO;
>  	intel_de_write(dev_priv, DP_AUX_CH_CTL(aux_ch), val);
>  
> -	hsw_power_well_enable(dev_priv, power_well);
> +	_hsw_power_well_enable(dev_priv, power_well, wait_ack);
>  
>  	if (INTEL_GEN(dev_priv) >= 12 && !power_well->desc->hsw.is_tc_tbt) {
>  		enum tc_port tc_port;
> @@ -603,6 +604,20 @@ icl_tc_phy_aux_power_well_enable(struct drm_i915_private *dev_priv,
>  	}
>  }
>  
> +static void
> +icl_tc_phy_aux_power_well_enable(struct drm_i915_private *dev_priv,
> +				 struct i915_power_well *power_well)
> +{
> +	_icl_tc_phy_aux_power_well_enable(dev_priv, power_well, true);
> +}
> +
> +static void
> +icl_tc_phy_aux_power_well_enable_without_ack(struct drm_i915_private *dev_priv,
> +					     struct i915_power_well *power_well)
> +{
> +	_icl_tc_phy_aux_power_well_enable(dev_priv, power_well, false);
> +}
> +
>  static void
>  icl_tc_phy_aux_power_well_disable(struct drm_i915_private *dev_priv,
>  				  struct i915_power_well *power_well)
> @@ -642,6 +657,13 @@ static bool hsw_power_well_enabled(struct drm_i915_private *dev_priv,
>  	return (val & mask) == mask;
>  }
>  
> +static void
> +hsw_power_well_wait_ack(struct drm_i915_private *dev_priv,
> +			struct i915_power_well *power_well)
> +{
> +	hsw_wait_for_power_well_enable(dev_priv, power_well);
> +}
> +
>  static void assert_can_enable_dc9(struct drm_i915_private *dev_priv)
>  {
>  	drm_WARN_ONCE(&dev_priv->drm,
> @@ -3582,8 +3604,10 @@ static const struct i915_power_well_ops icl_combo_phy_aux_power_well_ops = {
>  static const struct i915_power_well_ops icl_tc_phy_aux_power_well_ops = {
>  	.sync_hw = hsw_power_well_sync_hw,
>  	.enable = icl_tc_phy_aux_power_well_enable,
> +	.enable_without_ack = icl_tc_phy_aux_power_well_enable_without_ack,
>  	.disable = icl_tc_phy_aux_power_well_disable,
>  	.is_enabled = hsw_power_well_enabled,
> +	.wait_enable_ack = hsw_power_well_wait_ack,
>  };
>  
>  static const struct i915_power_well_regs icl_aux_power_well_regs = {
> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
> index 176ab5f1e867..42954be80435 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> @@ -1397,6 +1397,7 @@ struct intel_digital_port {
>  	enum tc_port_mode tc_mode;
>  	enum phy_fia tc_phy_fia;
>  	u8 tc_phy_fia_idx;
> +	intel_wakeref_t tc_cold_wakeref;
>  
>  	void (*write_infoframe)(struct intel_encoder *encoder,
>  				const struct intel_crtc_state *crtc_state,
> diff --git a/drivers/gpu/drm/i915/display/intel_tc.c b/drivers/gpu/drm/i915/display/intel_tc.c
> index e4c5de5ce874..588fca873b55 100644
> --- a/drivers/gpu/drm/i915/display/intel_tc.c
> +++ b/drivers/gpu/drm/i915/display/intel_tc.c
> @@ -416,9 +416,6 @@ static void intel_tc_port_reset_mode(struct intel_digital_port *dig_port,
>  	enum tc_port_mode old_tc_mode = dig_port->tc_mode;
>  
>  	intel_display_power_flush_work(i915);
> -	drm_WARN_ON(&i915->drm,
> -		    intel_display_power_is_enabled(i915,
> -					intel_aux_power_domain(dig_port)));

The flush_work/warn is still needed, but we should do that before
getting an AUX power ref around FIA accesses.

>  
>  	icl_tc_phy_disconnect(dig_port);
>  	icl_tc_phy_connect(dig_port, required_lanes);
> @@ -528,6 +525,39 @@ static inline int tgl_tc_cold_request(struct intel_digital_port *dig_port,
>  	return ret;
>  }
>  
> +static inline int icl_tc_cold_request(struct intel_digital_port *dig_port,
> +				      bool block)
> +{
> +	struct drm_i915_private *i915 = to_i915(dig_port->base.base.dev);
> +	enum intel_display_power_domain aux_domain;
> +	int ret;
> +
> +	aux_domain = intel_aux_ch_to_power_domain(dig_port->aux_ch);
> +
> +	if (block) {
> +		dig_port->tc_cold_wakeref =
> +			intel_display_power_get_without_ack(i915, aux_domain);
> +
> +		do {
> +			ret = sandybridge_pcode_write_timeout(i915,
> +							      ICL_PCODE_EXIT_TCCOLD,
> +							      0, 250, 1);
> +
> +		} while (ret == -EAGAIN);
> +
> +		if (!ret)
> +			msleep(1);
> +	} else if (dig_port->tc_mode == TC_PORT_LEGACY) {
> +		drm_WARN_ON(&i915->drm, !dig_port->tc_cold_wakeref);
> +		intel_display_power_put(i915, aux_domain,
> +					dig_port->tc_cold_wakeref);
> +		dig_port->tc_cold_wakeref = 0;
> +		ret = 0;
> +	}
> +
> +	return ret;
> +}
> +
>  static int tc_cold_request(struct intel_digital_port *dig_port, bool block)
>  {
>  	struct drm_i915_private *i915 = to_i915(dig_port->base.base.dev);
> @@ -536,8 +566,7 @@ static int tc_cold_request(struct intel_digital_port *dig_port, bool block)
>  	if (INTEL_GEN(i915) >= 12)
>  		ret = tgl_tc_cold_request(dig_port, block);
>  	else
> -		/* TODO: implement GEN11 TCCOLD sequences */
> -		ret = 0;
> +		ret = icl_tc_cold_request(dig_port, block);
>  
>  	drm_dbg_kms(&i915->drm, "Port %s: TCCOLD %sblock %s\n",
>  		    dig_port->tc_port_name, (block ? "" : "un"),
> @@ -546,6 +575,26 @@ static int tc_cold_request(struct intel_digital_port *dig_port, bool block)
>  	return ret;
>  }
>  
> +static void tc_cold_after_reset_mode(struct intel_digital_port *dig_port)
> +{
> +	struct drm_i915_private *i915 = to_i915(dig_port->base.base.dev);
> +	enum intel_display_power_domain aux_domain;
> +
> +	if (INTEL_GEN(i915) >= 12)
> +		return;
> +
> +	aux_domain = intel_aux_ch_to_power_domain(dig_port->aux_ch);
> +
> +	if (dig_port->tc_mode == TC_PORT_LEGACY) {
> +		intel_display_power_wait_enable_ack(i915, aux_domain);
> +	} else {
> +		drm_WARN_ON(&i915->drm, !dig_port->tc_cold_wakeref);
> +		intel_display_power_put(i915, aux_domain,
> +					dig_port->tc_cold_wakeref);
> +		dig_port->tc_cold_wakeref = 0;
> +	}
> +}
> +
>  static void __intel_tc_port_lock(struct intel_digital_port *dig_port,
>  				 int required_lanes)
>  {
> @@ -560,6 +609,7 @@ static void __intel_tc_port_lock(struct intel_digital_port *dig_port,
>  		tc_cold_request(dig_port, true);
>  		intel_tc_port_needs_reset(dig_port);
>  		intel_tc_port_reset_mode(dig_port, required_lanes);
> +		tc_cold_after_reset_mode(dig_port);
>  	}
>  
>  	drm_WARN_ON(&i915->drm, dig_port->tc_lock_wakeref);
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 7e341d9945b3..8d4f40a70a4d 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -9017,6 +9017,7 @@ enum {
>  #define     ICL_PCODE_MEM_SS_READ_QGV_POINT_INFO(point)	(((point) << 16) | (0x1 << 8))
>  #define   GEN6_PCODE_READ_D_COMP		0x10
>  #define   GEN6_PCODE_WRITE_D_COMP		0x11
> +#define   ICL_PCODE_EXIT_TCCOLD			0x12
>  #define   HSW_PCODE_DE_WRITE_FREQ_REQ		0x17
>  #define   DISPLAY_IPS_CONTROL			0x19
>  #define   TGL_PCODE_TCCOLD				0x26
> -- 
> 2.26.0
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v3 6/6] drm/i915/dp: Get TC link reference during DP detection

[Imre Deak]

On Tue, Mar 24, 2020 at 01:14:29PM -0700, José Roberto de Souza wrote:
> As now the cost to lock and use a TC port is higher due the
> implementation of the TCCOLD sequences it is worty to hold a reference
> of the TC port to avoid all this locking at every aux transaction
> part of the DisplayPort detection.

The problem with locking the port for detection is that it would block
modesets on the port, which we should avoid. By blocking tc-cold
whenever enabling an AUX power well you would avoid the overhead of the
PCODE requests for each AUX transfer, since the AUX power refs are
dropped asynchronously with a delay.

> 
> Cc: Imre Deak <imre.deak@intel.com>
> Cc: Cooper Chiou <cooper.chiou@intel.com>
> Cc: Kai-Heng Feng <kai.heng.feng@canonical.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_dp.c | 19 ++++++++++++++-----
>  1 file changed, 14 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> index 7f1a4e55cda1..6fbf3beee544 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -6041,6 +6041,7 @@ intel_dp_detect(struct drm_connector *connector,
>  	struct intel_dp *intel_dp = intel_attached_dp(to_intel_connector(connector));
>  	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
>  	struct intel_encoder *encoder = &dig_port->base;
> +	enum phy phy = intel_port_to_phy(dev_priv, encoder->port);
>  	enum drm_connector_status status;
>  
>  	drm_dbg_kms(&dev_priv->drm, "[CONNECTOR:%d:%s]\n",
> @@ -6049,12 +6050,17 @@ intel_dp_detect(struct drm_connector *connector,
>  		    !drm_modeset_is_locked(&dev_priv->drm.mode_config.connection_mutex));
>  
>  	/* Can't disconnect eDP */
> -	if (intel_dp_is_edp(intel_dp))
> +	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;
> +	} else {
> +		if (intel_phy_is_tc(dev_priv, phy))
> +			intel_tc_port_get_link(dig_port, 1);
> +
> +		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));
> @@ -6132,6 +6138,9 @@ intel_dp_detect(struct drm_connector *connector,
>  	if (status != connector_status_connected && !intel_dp->is_mst)
>  		intel_dp_unset_edid(intel_dp);
>  
> +	if (intel_phy_is_tc(dev_priv, phy))
> +		intel_tc_port_put_link(dig_port);
> +
>  	/*
>  	 * Make sure the refs for power wells enabled during detect are
>  	 * dropped to avoid a new detect cycle triggered by HPD polling.
> -- 
> 2.26.0
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v3 1/6] drm/i915/tc/tgl: Implement TCCOLD sequences

[Souza, Jose]

On Sat, 2020-03-28 at 11:57 +0200, Imre Deak wrote:
> Hi José,
> 
> On Tue, Mar 24, 2020 at 01:14:24PM -0700, José Roberto de Souza
> wrote:
> > TC ports can enter in TCCOLD to save power and is required to
> > request
> > to PCODE to exit this state before use or read to TC registers.
> > 
> > For TGL there is a new MBOX command to do that with a parameter to
> > ask
> > PCODE to exit and block TCCOLD entry or unblock TCCOLD entry.
> > For GEN11 the sequence is more complex and will be handled in a
> > separated patch.
> > 
> > BSpec: 49294
> > Cc: Imre Deak <imre.deak@intel.com>
> > Cc: Cooper Chiou <cooper.chiou@intel.com>
> > Cc: Kai-Heng Feng <kai.heng.feng@canonical.com>
> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_tc.c | 61
> > ++++++++++++++++++++++++-
> >  drivers/gpu/drm/i915/i915_reg.h         |  3 ++
> >  drivers/gpu/drm/i915/intel_sideband.c   | 22 +++++++++
> >  drivers/gpu/drm/i915/intel_sideband.h   |  4 ++
> >  4 files changed, 88 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_tc.c
> > b/drivers/gpu/drm/i915/display/intel_tc.c
> > index 9b850c11aa78..e4c5de5ce874 100644
> > --- a/drivers/gpu/drm/i915/display/intel_tc.c
> > +++ b/drivers/gpu/drm/i915/display/intel_tc.c
> > @@ -7,6 +7,7 @@
> >  #include "intel_display.h"
> >  #include "intel_display_types.h"
> >  #include "intel_dp_mst.h"
> > +#include "intel_sideband.h"
> >  #include "intel_tc.h"
> >  
> >  static const char *tc_port_mode_name(enum tc_port_mode mode)
> > @@ -496,6 +497,55 @@ bool intel_tc_port_connected(struct
> > intel_digital_port *dig_port)
> >  	return is_connected;
> >  }
> >  
> > +static inline int tgl_tc_cold_request(struct intel_digital_port
> > *dig_port,
> > +				      bool block)
> > +{
> > +	struct drm_i915_private *i915 = to_i915(dig_port-
> > >base.base.dev);
> > +	u32 low_val, high_val;
> > +	u8 tries = 0;
> > +	int ret;
> > +
> > +	do {
> > +		low_val = 0;
> > +		high_val = block ? 0 :
> > TGL_PCODE_EXIT_TCCOLD_DATA_H_UNBLOCK_REQ;
> > +
> > +		ret = sandybridge_pcode_write_read_timeout(i915,
> > +							   TGL_PCODE_TC
> > COLD,
> > +							   &low_val,
> > &high_val,
> > +							   150, 1);
> 
> The spec says trying 3 times for 200 usec. Is there a problem if we
> just
> reuse sandybridge_pcode_read() here which has a 500 usec timeout?

Yeah I guess we can switch to sandybridge_pcode_read().

> 
> > +		if (ret == 0) {
> > +			if (block &&
> > +			    low_val &
> > TGL_PCODE_EXIT_TCCOLD_DATA_L_EXIT_FAILED)
> > +				ret = -EIO;
> > +			else
> > +				break;
> > +		}
> > +
> > +		if (ret != -EAGAIN)
> > +			tries++;
> > +	} while (tries < 3);
> > +
> > +	return ret;
> 
> The return value isn't used and I think we can't do much about it, so
> just make the function a void type and warn about a timeout?

The return is usefull to have just one warm message between ICL and
TGL.

> 
> > +}
> > +
> > +static int tc_cold_request(struct intel_digital_port *dig_port,
> > bool block)
> > +{
> > +	struct drm_i915_private *i915 = to_i915(dig_port-
> > >base.base.dev);
> > +	int ret;
> > +
> > +	if (INTEL_GEN(i915) >= 12)
> > +		ret = tgl_tc_cold_request(dig_port, block);
> > +	else
> > +		/* TODO: implement GEN11 TCCOLD sequences */
> > +		ret = 0;
> > +
> > +	drm_dbg_kms(&i915->drm, "Port %s: TCCOLD %sblock %s\n",
> > +		    dig_port->tc_port_name, (block ? "" : "un"),
> > +		    (ret == 0 ? "succeeded" : "failed"));
> > +
> > +	return ret;
> > +}
> > +
> >  static void __intel_tc_port_lock(struct intel_digital_port
> > *dig_port,
> >  				 int required_lanes)
> >  {
> > @@ -506,9 +556,11 @@ static void __intel_tc_port_lock(struct
> > intel_digital_port *dig_port,
> >  
> >  	mutex_lock(&dig_port->tc_lock);
> >  
> > -	if (!dig_port->tc_link_refcount &&
> > -	    intel_tc_port_needs_reset(dig_port))
> > +	if (dig_port->tc_link_refcount == 0) {
> > +		tc_cold_request(dig_port, true);
> 
> I'm not sure if PCODE really refcounts the requests what this would
> need
> (since we submit the same request for all ports). But for other 

Oh I complete missed the multiple ports handling.

> reasons,
> like the overhead of the request and the AUX PW ack trickery we need
> on
> ICL, I think we need a tc_cold_off power well/domain. The tc_cold_off
> power ref would be get/put around the FIA access sequence here
> (intel_tc_port_reset_mode()) and would be held whenever we hold an
> AUX
> power ref.

For TGL the tc_cold_off power well would work and would be pretty easy
to implement but for ICL I'm not sure.

For ICL, because of preemptions we need to get the aux power of the TC
port before request PCODE to exit TC cold.

So a single tc_cold_off would
need to depend into all aux's?
Even if we have one tc_cold_off per TC
DDI, if we make it depend into aux we would get aux power enable
timeouts. So we would need to enable aux power inside of the
tc_cold_off enable function and the aux enable call would need to not
check the HW status.

Thoughts?

> 
> ICL legacy ports would hold an AUX power ref around the FIA access
> here
> and the AUX power well code would internally do the PCODE request and
> deal with the delayed power well ack (so we don't need to create a
> new
> interface for that).
> 
> > +		intel_tc_port_needs_reset(dig_port);
> >  		intel_tc_port_reset_mode(dig_port, required_lanes);
> > +	}
> >  
> >  	drm_WARN_ON(&i915->drm, dig_port->tc_lock_wakeref);
> >  	dig_port->tc_lock_wakeref = wakeref;
> > @@ -524,6 +576,9 @@ void intel_tc_port_unlock(struct
> > intel_digital_port *dig_port)
> >  	struct drm_i915_private *i915 = to_i915(dig_port-
> > >base.base.dev);
> >  	intel_wakeref_t wakeref = fetch_and_zero(&dig_port-
> > >tc_lock_wakeref);
> >  
> > +	if (dig_port->tc_link_refcount == 0)
> > +		tc_cold_request(dig_port, false);
> > +
> >  	mutex_unlock(&dig_port->tc_lock);
> >  
> >  	intel_display_power_put_async(i915, POWER_DOMAIN_DISPLAY_CORE,
> > @@ -548,6 +603,8 @@ void intel_tc_port_put_link(struct
> > intel_digital_port *dig_port)
> >  {
> >  	mutex_lock(&dig_port->tc_lock);
> >  	dig_port->tc_link_refcount--;
> > +	if (dig_port->tc_link_refcount == 0)
> > +		tc_cold_request(dig_port, false);
> >  	mutex_unlock(&dig_port->tc_lock);
> >  }
> >  
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > b/drivers/gpu/drm/i915/i915_reg.h
> > index 9c53fe918be6..7e341d9945b3 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -9019,6 +9019,9 @@ enum {
> >  #define   GEN6_PCODE_WRITE_D_COMP		0x11
> >  #define   HSW_PCODE_DE_WRITE_FREQ_REQ		0x17
> >  #define   DISPLAY_IPS_CONTROL			0x19
> > +#define   TGL_PCODE_TCCOLD				0x26
> > +#define     TGL_PCODE_EXIT_TCCOLD_DATA_L_EXIT_FAILED	REG_BIT
> > (0)
> > +#define     TGL_PCODE_EXIT_TCCOLD_DATA_H_UNBLOCK_REQ	REG_BIT
> > (0)
> >              /* See also IPS_CTL */
> >  #define     IPS_PCODE_CONTROL			(1 << 30)
> >  #define   HSW_PCODE_DYNAMIC_DUTY_CYCLE_CONTROL	0x1A
> > diff --git a/drivers/gpu/drm/i915/intel_sideband.c
> > b/drivers/gpu/drm/i915/intel_sideband.c
> > index 1447e7516cb7..20a9d3970930 100644
> > --- a/drivers/gpu/drm/i915/intel_sideband.c
> > +++ b/drivers/gpu/drm/i915/intel_sideband.c
> > @@ -463,6 +463,28 @@ int sandybridge_pcode_write_timeout(struct
> > drm_i915_private *i915,
> >  	return err;
> >  }
> >  
> > +int sandybridge_pcode_write_read_timeout(struct drm_i915_private
> > *i915,
> > +					 u32 mbox, u32 *val, u32 *val1,
> > +					 int fast_timeout_us,
> > +					 int slow_timeout_ms)
> > +{
> > +	int err;
> > +
> > +	mutex_lock(&i915->sb_lock);
> > +	err = __sandybridge_pcode_rw(i915, mbox, val, val1,
> > +				     fast_timeout_us, slow_timeout_ms,
> > +				     true);
> > +	mutex_unlock(&i915->sb_lock);
> > +
> > +	if (err) {
> > +		drm_dbg(&i915->drm,
> > +			"warning: pcode (write of 0x%08x to mbox %x)
> > mailbox access failed for %ps: %d\n",
> > +			*val, mbox, __builtin_return_address(0), err);
> > +	}
> > +
> > +	return err;
> > +}
> > +
> >  static bool skl_pcode_try_request(struct drm_i915_private *i915,
> > u32 mbox,
> >  				  u32 request, u32 reply_mask, u32
> > reply,
> >  				  u32 *status)
> > diff --git a/drivers/gpu/drm/i915/intel_sideband.h
> > b/drivers/gpu/drm/i915/intel_sideband.h
> > index 7fb95745a444..1939bebb4e67 100644
> > --- a/drivers/gpu/drm/i915/intel_sideband.h
> > +++ b/drivers/gpu/drm/i915/intel_sideband.h
> > @@ -132,6 +132,10 @@ int sandybridge_pcode_read(struct
> > drm_i915_private *i915, u32 mbox,
> >  int sandybridge_pcode_write_timeout(struct drm_i915_private *i915,
> > u32 mbox,
> >  				    u32 val, int fast_timeout_us,
> >  				    int slow_timeout_ms);
> > +int sandybridge_pcode_write_read_timeout(struct drm_i915_private
> > *i915,
> > +					 u32 mbox, u32 *val, u32 *val1,
> > +					 int fast_timeout_us,
> > +					 int slow_timeout_ms);
> >  #define sandybridge_pcode_write(i915, mbox, val)	\
> >  	sandybridge_pcode_write_timeout(i915, mbox, val, 500, 0)
> >  
> > -- 
> > 2.26.0
> > 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v3 6/6] drm/i915/dp: Get TC link reference during DP detection

[Souza, Jose]

On Sat, 2020-03-28 at 12:26 +0200, Imre Deak wrote:
> On Tue, Mar 24, 2020 at 01:14:29PM -0700, José Roberto de Souza
> wrote:
> > As now the cost to lock and use a TC port is higher due the
> > implementation of the TCCOLD sequences it is worty to hold a
> > reference
> > of the TC port to avoid all this locking at every aux transaction
> > part of the DisplayPort detection.
> 
> The problem with locking the port for detection is that it would
> block
> modesets on the port, which we should avoid. By blocking tc-cold

It will not block modesets on the port, intel_tc_port_get_link and
intel_tc_port_put_link gets locks tc_lock, increment or decrement
tc_link_refcount and then unlock, it would only avoid the TC cold
sequences over and over.

> whenever enabling an AUX power well you would avoid the overhead of
> the
> PCODE requests for each AUX transfer, since the AUX power refs are
> dropped asynchronously with a delay.

Left comments on your proposal in patch 1.

> 
> > Cc: Imre Deak <imre.deak@intel.com>
> > Cc: Cooper Chiou <cooper.chiou@intel.com>
> > Cc: Kai-Heng Feng <kai.heng.feng@canonical.com>
> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_dp.c | 19 ++++++++++++++-----
> >  1 file changed, 14 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
> > b/drivers/gpu/drm/i915/display/intel_dp.c
> > index 7f1a4e55cda1..6fbf3beee544 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > @@ -6041,6 +6041,7 @@ intel_dp_detect(struct drm_connector
> > *connector,
> >  	struct intel_dp *intel_dp =
> > intel_attached_dp(to_intel_connector(connector));
> >  	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
> >  	struct intel_encoder *encoder = &dig_port->base;
> > +	enum phy phy = intel_port_to_phy(dev_priv, encoder->port);
> >  	enum drm_connector_status status;
> >  
> >  	drm_dbg_kms(&dev_priv->drm, "[CONNECTOR:%d:%s]\n",
> > @@ -6049,12 +6050,17 @@ intel_dp_detect(struct drm_connector
> > *connector,
> >  		    !drm_modeset_is_locked(&dev_priv-
> > >drm.mode_config.connection_mutex));
> >  
> >  	/* Can't disconnect eDP */
> > -	if (intel_dp_is_edp(intel_dp))
> > +	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;
> > +	} else {
> > +		if (intel_phy_is_tc(dev_priv, phy))
> > +			intel_tc_port_get_link(dig_port, 1);
> > +
> > +		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));
> > @@ -6132,6 +6138,9 @@ intel_dp_detect(struct drm_connector
> > *connector,
> >  	if (status != connector_status_connected && !intel_dp->is_mst)
> >  		intel_dp_unset_edid(intel_dp);
> >  
> > +	if (intel_phy_is_tc(dev_priv, phy))
> > +		intel_tc_port_put_link(dig_port);
> > +
> >  	/*
> >  	 * Make sure the refs for power wells enabled during detect are
> >  	 * dropped to avoid a new detect cycle triggered by HPD
> > polling.
> > -- 
> > 2.26.0
> > 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v3 1/6] drm/i915/tc/tgl: Implement TCCOLD sequences

[Imre Deak]

On Mon, Mar 30, 2020 at 11:23:24PM +0300, Souza, Jose wrote:
> On Sat, 2020-03-28 at 11:57 +0200, Imre Deak wrote:
> [...]
> > > +		if (ret == 0) {
> > > +			if (block &&
> > > +			    low_val & TGL_PCODE_EXIT_TCCOLD_DATA_L_EXIT_FAILED)
> > > +				ret = -EIO;
> > > +			else
> > > +				break;
> > > +		}
> > > +
> > > +		if (ret != -EAGAIN)
> > > +			tries++;
> > > +	} while (tries < 3);
> > > +
> > > +	return ret;
> > 
> > The return value isn't used and I think we can't do much about it, so
> > just make the function a void type and warn about a timeout?
> 
> The return is usefull to have just one warm message between ICL and
> TGL.

Ah, in tc_cold_request(), but then we won't use the return value from
that.

> > [...]
> > ICL, I think we need a tc_cold_off power well/domain. The tc_cold_off
> > power ref would be get/put around the FIA access sequence here
> > (intel_tc_port_reset_mode()) and would be held whenever we hold an
> > AUX power ref.
> 
> For TGL the tc_cold_off power well would work and would be pretty easy
> to implement but for ICL I'm not sure.
> 
> For ICL, because of preemptions we need to get the aux power of the TC
> port before request PCODE to exit TC cold.
> 
> So a single tc_cold_off would need to depend into all aux's?
> Even if we have one tc_cold_off per TC DDI, if we make it depend into aux
> we would get aux power enable timeouts. So we would need to enable aux
> power inside of the tc_cold_off enable function and the aux enable call
> would need to not check the HW status.
> 
> Thoughts?

On ICL we wouldn't have a power domain/well for tc-cold, since the PCODE
request for it has anyway the strange timeout semantics, without the
proper block/unblock interface like TGL has.

So for ICL you'd need to get an AUX power domain ref here, and the AUX
power well enable hook would do the

	enable AUX
	block tc-cold
	wait for AUX ACK

sequence in the AUX power well's enable hook (for legacy ports).

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

Re: [Intel-gfx] [PATCH v3 6/6] drm/i915/dp: Get TC link reference during DP detection

[Imre Deak]

On Mon, Mar 30, 2020 at 11:27:06PM +0300, Souza, Jose wrote:
> On Sat, 2020-03-28 at 12:26 +0200, Imre Deak wrote:
> > On Tue, Mar 24, 2020 at 01:14:29PM -0700, José Roberto de Souza
> > wrote:
> > > As now the cost to lock and use a TC port is higher due the
> > > implementation of the TCCOLD sequences it is worty to hold a
> > > reference of the TC port to avoid all this locking at every
> > > aux transaction part of the DisplayPort detection.
> > 
> > The problem with locking the port for detection is that it would
> > block modesets on the port, which we should avoid. By blocking
> > tc-cold
> 
> It will not block modesets on the port, intel_tc_port_get_link and
> intel_tc_port_put_link gets locks tc_lock, increment or decrement
> tc_link_refcount and then unlock,

The effect of holding a link_refcount is that it's not possible to
update the TC port mode and select the correct TBT/MG PLL for the mode.
This will only be possible in the middle of the modeset sequence where
an active mode is first disabled on the port and that's the place we
don't want to wait for the detect sequence to finish.

So only an active mode should hold a link_refcount, so that it's
guaranteed that a modeset can update the TC port mode to its current
state.

> it would only avoid the TC cold sequences over and over.

Yes, but that would be also avoided by disabling the AUX power well only
with a delay.

>
> > whenever enabling an AUX power well you would avoid the overhead of
> > the
> > PCODE requests for each AUX transfer, since the AUX power refs are
> > dropped asynchronously with a delay.
> 
> Left comments on your proposal in patch 1.
> 
> > 
> > > Cc: Imre Deak <imre.deak@intel.com>
> > > Cc: Cooper Chiou <cooper.chiou@intel.com>
> > > Cc: Kai-Heng Feng <kai.heng.feng@canonical.com>
> > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_dp.c | 19 ++++++++++++++-----
> > >  1 file changed, 14 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
> > > b/drivers/gpu/drm/i915/display/intel_dp.c
> > > index 7f1a4e55cda1..6fbf3beee544 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > > @@ -6041,6 +6041,7 @@ intel_dp_detect(struct drm_connector
> > > *connector,
> > >  	struct intel_dp *intel_dp =
> > > intel_attached_dp(to_intel_connector(connector));
> > >  	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
> > >  	struct intel_encoder *encoder = &dig_port->base;
> > > +	enum phy phy = intel_port_to_phy(dev_priv, encoder->port);
> > >  	enum drm_connector_status status;
> > >  
> > >  	drm_dbg_kms(&dev_priv->drm, "[CONNECTOR:%d:%s]\n",
> > > @@ -6049,12 +6050,17 @@ intel_dp_detect(struct drm_connector
> > > *connector,
> > >  		    !drm_modeset_is_locked(&dev_priv-
> > > >drm.mode_config.connection_mutex));
> > >  
> > >  	/* Can't disconnect eDP */
> > > -	if (intel_dp_is_edp(intel_dp))
> > > +	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;
> > > +	} else {
> > > +		if (intel_phy_is_tc(dev_priv, phy))
> > > +			intel_tc_port_get_link(dig_port, 1);
> > > +
> > > +		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));
> > > @@ -6132,6 +6138,9 @@ intel_dp_detect(struct drm_connector
> > > *connector,
> > >  	if (status != connector_status_connected && !intel_dp->is_mst)
> > >  		intel_dp_unset_edid(intel_dp);
> > >  
> > > +	if (intel_phy_is_tc(dev_priv, phy))
> > > +		intel_tc_port_put_link(dig_port);
> > > +
> > >  	/*
> > >  	 * Make sure the refs for power wells enabled during detect are
> > >  	 * dropped to avoid a new detect cycle triggered by HPD
> > > polling.
> > > -- 
> > > 2.26.0
> > > 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx