[PATCH 1/4] drm/i915: Reuse the aux_domain cached

[PATCH 1/4] drm/i915: Reuse the aux_domain cached

[José Roberto de Souza]

intel_dp_detect() caches the aux_domain in the beginning of the
function as it is used twice, so lets also use it as the aux_domain
don't change in runtime.

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

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index e7233dfa1794..52a54ef746af 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -5289,8 +5289,7 @@ intel_dp_detect(struct drm_connector *connector,
 
 		ret = intel_dp_retrain_link(encoder, ctx);
 		if (ret) {
-			intel_display_power_put(dev_priv,
-						intel_aux_power_domain(dig_port));
+			intel_display_power_put(dev_priv, aux_domain);
 			return ret;
 		}
 	}
-- 
2.19.1

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

[PATCH 2/4] drm/i915: Release DDI power well references in MST ports

[José Roberto de Souza]

MST ports did not had the post_pll_disable() hook causing the
references get in pre_pll_enable() never being released causing
DDI and AUX CH being enabled all the times.

Cc: Imre Deak <imre.deak@intel.com>
Cc: Manasi Navare <manasi.d.navare@intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 drivers/gpu/drm/i915/intel_dp_mst.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
index 8b71d64ebd9d..8e3d0b04580b 100644
--- a/drivers/gpu/drm/i915/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/intel_dp_mst.c
@@ -215,6 +215,20 @@ static void intel_mst_pre_pll_enable_dp(struct intel_encoder *encoder,
 						    pipe_config, NULL);
 }
 
+static void intel_mst_post_pll_disable_dp(struct intel_encoder *encoder,
+					  const struct intel_crtc_state *pipe_config,
+					  const struct drm_connector_state *conn_state)
+{
+	struct intel_dp_mst_encoder *intel_mst = enc_to_mst(&encoder->base);
+	struct intel_digital_port *intel_dig_port = intel_mst->primary;
+	struct intel_dp *intel_dp = &intel_dig_port->dp;
+
+	if (intel_dp->active_mst_links == 0 &&
+	    intel_dig_port->base.post_pll_disable)
+		intel_dig_port->base.post_pll_disable(&intel_dig_port->base,
+						      pipe_config, NULL);
+}
+
 static void intel_mst_pre_enable_dp(struct intel_encoder *encoder,
 				    const struct intel_crtc_state *pipe_config,
 				    const struct drm_connector_state *conn_state)
@@ -549,6 +563,7 @@ intel_dp_create_fake_mst_encoder(struct intel_digital_port *intel_dig_port, enum
 	intel_encoder->disable = intel_mst_disable_dp;
 	intel_encoder->post_disable = intel_mst_post_disable_dp;
 	intel_encoder->pre_pll_enable = intel_mst_pre_pll_enable_dp;
+	intel_encoder->post_pll_disable = intel_mst_post_pll_disable_dp;
 	intel_encoder->pre_enable = intel_mst_pre_enable_dp;
 	intel_encoder->enable = intel_mst_enable_dp;
 	intel_encoder->get_hw_state = intel_dp_mst_enc_get_hw_state;
-- 
2.19.1

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

Re: [PATCH 2/4] drm/i915: Release DDI power well references in MST ports

[Ville Syrjälä]

On Fri, Nov 02, 2018 at 01:39:22PM -0700, José Roberto de Souza wrote:
> MST ports did not had the post_pll_disable() hook causing the
> references get in pre_pll_enable() never being released causing
> DDI and AUX CH being enabled all the times.
> 
> Cc: Imre Deak <imre.deak@intel.com>
> Cc: Manasi Navare <manasi.d.navare@intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>

Patches 1 and 2 are 
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> ---
>  drivers/gpu/drm/i915/intel_dp_mst.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
> index 8b71d64ebd9d..8e3d0b04580b 100644
> --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> @@ -215,6 +215,20 @@ static void intel_mst_pre_pll_enable_dp(struct intel_encoder *encoder,
>  						    pipe_config, NULL);
>  }
>  
> +static void intel_mst_post_pll_disable_dp(struct intel_encoder *encoder,
> +					  const struct intel_crtc_state *pipe_config,
> +					  const struct drm_connector_state *conn_state)
> +{
> +	struct intel_dp_mst_encoder *intel_mst = enc_to_mst(&encoder->base);
> +	struct intel_digital_port *intel_dig_port = intel_mst->primary;
> +	struct intel_dp *intel_dp = &intel_dig_port->dp;
> +
> +	if (intel_dp->active_mst_links == 0 &&
> +	    intel_dig_port->base.post_pll_disable)
> +		intel_dig_port->base.post_pll_disable(&intel_dig_port->base,
> +						      pipe_config, NULL);
> +}
> +
>  static void intel_mst_pre_enable_dp(struct intel_encoder *encoder,
>  				    const struct intel_crtc_state *pipe_config,
>  				    const struct drm_connector_state *conn_state)
> @@ -549,6 +563,7 @@ intel_dp_create_fake_mst_encoder(struct intel_digital_port *intel_dig_port, enum
>  	intel_encoder->disable = intel_mst_disable_dp;
>  	intel_encoder->post_disable = intel_mst_post_disable_dp;
>  	intel_encoder->pre_pll_enable = intel_mst_pre_pll_enable_dp;
> +	intel_encoder->post_pll_disable = intel_mst_post_pll_disable_dp;
>  	intel_encoder->pre_enable = intel_mst_pre_enable_dp;
>  	intel_encoder->enable = intel_mst_enable_dp;
>  	intel_encoder->get_hw_state = intel_dp_mst_enc_get_hw_state;
> -- 
> 2.19.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH 2/4] drm/i915: Release DDI power well references in MST ports

[Imre Deak]

On Fri, Nov 02, 2018 at 01:39:22PM -0700, José Roberto de Souza wrote:
> MST ports did not had the post_pll_disable() hook causing the
> references get in pre_pll_enable() never being released causing
> DDI and AUX CH being enabled all the times.
> 
> Cc: Imre Deak <imre.deak@intel.com>
> Cc: Manasi Navare <manasi.d.navare@intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>

Err, thanks for catching it. We really need more testing on MST.. You
could also add the Fixes: line.

> ---
>  drivers/gpu/drm/i915/intel_dp_mst.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
> index 8b71d64ebd9d..8e3d0b04580b 100644
> --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> @@ -215,6 +215,20 @@ static void intel_mst_pre_pll_enable_dp(struct intel_encoder *encoder,
>  						    pipe_config, NULL);
>  }
>  
> +static void intel_mst_post_pll_disable_dp(struct intel_encoder *encoder,
> +					  const struct intel_crtc_state *pipe_config,
> +					  const struct drm_connector_state *conn_state)
> +{

Nit: these are better called old_crtc_state, old_conn_state.

> +	struct intel_dp_mst_encoder *intel_mst = enc_to_mst(&encoder->base);
> +	struct intel_digital_port *intel_dig_port = intel_mst->primary;
> +	struct intel_dp *intel_dp = &intel_dig_port->dp;
> +
> +	if (intel_dp->active_mst_links == 0 &&
> +	    intel_dig_port->base.post_pll_disable)

Since by now all DDI platforms (which are the only platforms supporting
MST) define the .pre_pll_enable() .post_pll_disable() hooks you could
remove the NULL check for them (in a follow-up patch).

Either with or without changing the above nit:

Reviewed-by: Imre Deak <imre.deak@intel.com>

> +		intel_dig_port->base.post_pll_disable(&intel_dig_port->base,
> +						      pipe_config, NULL);
> +}
> +
>  static void intel_mst_pre_enable_dp(struct intel_encoder *encoder,
>  				    const struct intel_crtc_state *pipe_config,
>  				    const struct drm_connector_state *conn_state)
> @@ -549,6 +563,7 @@ intel_dp_create_fake_mst_encoder(struct intel_digital_port *intel_dig_port, enum
>  	intel_encoder->disable = intel_mst_disable_dp;
>  	intel_encoder->post_disable = intel_mst_post_disable_dp;
>  	intel_encoder->pre_pll_enable = intel_mst_pre_pll_enable_dp;
> +	intel_encoder->post_pll_disable = intel_mst_post_pll_disable_dp;
>  	intel_encoder->pre_enable = intel_mst_pre_enable_dp;
>  	intel_encoder->enable = intel_mst_enable_dp;
>  	intel_encoder->get_hw_state = intel_dp_mst_enc_get_hw_state;
> -- 
> 2.19.1
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/4] drm/i915: Release DDI power well references in MST ports

[Imre Deak]

On Fri, Nov 02, 2018 at 01:39:22PM -0700, José Roberto de Souza wrote:
> MST ports did not had the post_pll_disable() hook causing the
> references get in pre_pll_enable() never being released causing
> DDI and AUX CH being enabled all the times.
> 
> Cc: Imre Deak <imre.deak@intel.com>
> Cc: Manasi Navare <manasi.d.navare@intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>

Could you resend this and the first patch separately?

Thanks,
Imre

> ---
>  drivers/gpu/drm/i915/intel_dp_mst.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
> index 8b71d64ebd9d..8e3d0b04580b 100644
> --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> @@ -215,6 +215,20 @@ static void intel_mst_pre_pll_enable_dp(struct intel_encoder *encoder,
>  						    pipe_config, NULL);
>  }
>  
> +static void intel_mst_post_pll_disable_dp(struct intel_encoder *encoder,
> +					  const struct intel_crtc_state *pipe_config,
> +					  const struct drm_connector_state *conn_state)
> +{
> +	struct intel_dp_mst_encoder *intel_mst = enc_to_mst(&encoder->base);
> +	struct intel_digital_port *intel_dig_port = intel_mst->primary;
> +	struct intel_dp *intel_dp = &intel_dig_port->dp;
> +
> +	if (intel_dp->active_mst_links == 0 &&
> +	    intel_dig_port->base.post_pll_disable)
> +		intel_dig_port->base.post_pll_disable(&intel_dig_port->base,
> +						      pipe_config, NULL);
> +}
> +
>  static void intel_mst_pre_enable_dp(struct intel_encoder *encoder,
>  				    const struct intel_crtc_state *pipe_config,
>  				    const struct drm_connector_state *conn_state)
> @@ -549,6 +563,7 @@ intel_dp_create_fake_mst_encoder(struct intel_digital_port *intel_dig_port, enum
>  	intel_encoder->disable = intel_mst_disable_dp;
>  	intel_encoder->post_disable = intel_mst_post_disable_dp;
>  	intel_encoder->pre_pll_enable = intel_mst_pre_pll_enable_dp;
> +	intel_encoder->post_pll_disable = intel_mst_post_pll_disable_dp;
>  	intel_encoder->pre_enable = intel_mst_pre_enable_dp;
>  	intel_encoder->enable = intel_mst_enable_dp;
>  	intel_encoder->get_hw_state = intel_dp_mst_enc_get_hw_state;
> -- 
> 2.19.1
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 3/4] drm/i915/icl: Only grab TC ports when using it

[José Roberto de Souza]

When suspending or unloading the driver, it needs to release the
TC ports so HW can change it state without wait for driver handshake.
Spec also state that if the port is not used by driver it should
release TC access, so here only grabbing control of the TC ports and
marking as unsafe when aux power is needed as have aux power well is
a requirement to have DDI enabled in TC ports, the pre_pll_enable and
post_pll_disable hooks takes care of getting and releasing it.

BSpec: 21750

Cc: Imre Deak <imre.deak@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c         | 28 -------------
 drivers/gpu/drm/i915/intel_runtime_pm.c | 55 ++++++++++++++++++++++++-
 2 files changed, 54 insertions(+), 29 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 52a54ef746af..d978127e7208 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -5013,16 +5013,6 @@ static bool icl_tc_phy_connect(struct drm_i915_private *dev_priv,
 		return false;
 	}
 
-	/*
-	 * This function may be called many times in a row without an HPD event
-	 * in between, so try to avoid the write when we can.
-	 */
-	val = I915_READ(PORT_TX_DFLEXDPCSSS);
-	if (!(val & DP_PHY_MODE_STATUS_NOT_SAFE(tc_port))) {
-		val |= DP_PHY_MODE_STATUS_NOT_SAFE(tc_port);
-		I915_WRITE(PORT_TX_DFLEXDPCSSS, val);
-	}
-
 	/*
 	 * Now we have to re-check the live state, in case the port recently
 	 * became disconnected. Not necessary for legacy mode.
@@ -5044,24 +5034,6 @@ static bool icl_tc_phy_connect(struct drm_i915_private *dev_priv,
 static void icl_tc_phy_disconnect(struct drm_i915_private *dev_priv,
 				  struct intel_digital_port *dig_port)
 {
-	enum tc_port tc_port = intel_port_to_tc(dev_priv, dig_port->base.port);
-
-	if (dig_port->tc_type == TC_PORT_UNKNOWN)
-		return;
-
-	/*
-	 * TBT disconnection flow is read the live status, what was done in
-	 * caller.
-	 */
-	if (dig_port->tc_type == TC_PORT_TYPEC ||
-	    dig_port->tc_type == TC_PORT_LEGACY) {
-		u32 val;
-
-		val = I915_READ(PORT_TX_DFLEXDPCSSS);
-		val &= ~DP_PHY_MODE_STATUS_NOT_SAFE(tc_port);
-		I915_WRITE(PORT_TX_DFLEXDPCSSS, val);
-	}
-
 	dig_port->tc_type = TC_PORT_UNKNOWN;
 }
 
diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
index 6c453366cd24..dab5f90646c4 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -465,6 +465,48 @@ icl_combo_phy_aux_power_well_disable(struct drm_i915_private *dev_priv,
 	hsw_wait_for_power_well_disable(dev_priv, power_well);
 }
 
+static void icl_tc_grab_control(struct drm_i915_private *dev_priv,
+				enum aux_ch aux_ch, bool grab)
+{
+	struct drm_device *dev = &dev_priv->drm;
+	struct drm_connector_list_iter conn_iter;
+	struct drm_connector *connector;
+
+	drm_connector_list_iter_begin(dev, &conn_iter);
+	drm_for_each_connector_iter(connector, &conn_iter) {
+		struct intel_connector *intel_connector;
+		struct intel_encoder *intel_encoder;
+		struct intel_digital_port *dig_port;
+		enum tc_port tc_port;
+
+		intel_connector = to_intel_connector(connector);
+		if (!intel_connector->encoder)
+			continue;
+		intel_encoder = intel_connector->encoder;
+		dig_port = enc_to_dig_port(&intel_encoder->base);
+
+		if (!dig_port || dig_port->aux_ch != aux_ch)
+			continue;
+
+		tc_port = intel_port_to_tc(dev_priv, dig_port->base.port);
+
+		if (dig_port->tc_type == TC_PORT_TYPEC ||
+		    dig_port->tc_type == TC_PORT_LEGACY) {
+			u32 val;
+
+			val = I915_READ(PORT_TX_DFLEXDPCSSS);
+			if (grab)
+				val |= DP_PHY_MODE_STATUS_NOT_SAFE(tc_port);
+			else
+				val &= ~DP_PHY_MODE_STATUS_NOT_SAFE(tc_port);
+			I915_WRITE(PORT_TX_DFLEXDPCSSS, val);
+		}
+
+		break;
+	}
+	drm_connector_list_iter_end(&conn_iter);
+}
+
 #define ICL_AUX_PW_TO_CH(pw_idx)	\
 	((pw_idx) - ICL_PW_CTL_IDX_AUX_A + AUX_CH_A)
 
@@ -475,6 +517,8 @@ icl_tc_phy_aux_power_well_enable(struct drm_i915_private *dev_priv,
 	enum aux_ch aux_ch = ICL_AUX_PW_TO_CH(power_well->desc->hsw.idx);
 	u32 val;
 
+	icl_tc_grab_control(dev_priv, aux_ch, true);
+
 	val = I915_READ(DP_AUX_CH_CTL(aux_ch));
 	val &= ~DP_AUX_CH_CTL_TBT_IO;
 	if (power_well->desc->hsw.is_tc_tbt)
@@ -484,6 +528,15 @@ icl_tc_phy_aux_power_well_enable(struct drm_i915_private *dev_priv,
 	hsw_power_well_enable(dev_priv, power_well);
 }
 
+static void icl_tc_phy_aux_power_well_disable(struct drm_i915_private *dev_priv,
+					      struct i915_power_well *power_well)
+{
+	enum aux_ch aux_ch = ICL_AUX_PW_TO_CH(power_well->desc->hsw.idx);
+
+	icl_tc_grab_control(dev_priv, aux_ch, false);
+	hsw_power_well_disable(dev_priv, power_well);
+}
+
 /*
  * We should only use the power well if we explicitly asked the hardware to
  * enable it, so check if it's enabled and also check if we've requested it to
@@ -2754,7 +2807,7 @@ 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,
-	.disable = hsw_power_well_disable,
+	.disable = icl_tc_phy_aux_power_well_disable,
 	.is_enabled = hsw_power_well_enabled,
 };
 
-- 
2.19.1

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

Re: [PATCH 3/4] drm/i915/icl: Only grab TC ports when using it

[Imre Deak]

On Fri, Nov 02, 2018 at 01:39:23PM -0700, José Roberto de Souza wrote:
> When suspending or unloading the driver, it needs to release the
> TC ports so HW can change it state without wait for driver handshake.
> Spec also state that if the port is not used by driver it should
> release TC access, so here only grabbing control of the TC ports and
> marking as unsafe when aux power is needed as have aux power well is
> a requirement to have DDI enabled in TC ports, the pre_pll_enable and
> post_pll_disable hooks takes care of getting and releasing it.
> 
> BSpec: 21750
> 
> Cc: Imre Deak <imre.deak@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>

Agreed that we should force a manual disconnect before entering
low-power states and driver unloading, but I don't think this should be
done from the power well code. We could perform multiple AUX transfers
after a connect event around each of which we would enable/disable the
AUX power well. We would then likely continue doing a modeset. During
this whole sequence I don't think we should do forced
connects/disconnects due to the AUX power well getting enabled/disabled.

I think normally we should change the connection status (that is the
safe/unsafe mode you're setting here) in response to HPD events, also
considering that we may have to delay changing the state as discussed
earlier with Ville (due to an ongoing AUX transfer or active mode in the
opposite TypeC mode). Then only during system/runtime suspend and unload
should we do a forced disconnect, which would be safe since at those
points we don't have any pending AUX transfers or active outputs.

--Imre

> ---
>  drivers/gpu/drm/i915/intel_dp.c         | 28 -------------
>  drivers/gpu/drm/i915/intel_runtime_pm.c | 55 ++++++++++++++++++++++++-
>  2 files changed, 54 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 52a54ef746af..d978127e7208 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -5013,16 +5013,6 @@ static bool icl_tc_phy_connect(struct drm_i915_private *dev_priv,
>  		return false;
>  	}
>  
> -	/*
> -	 * This function may be called many times in a row without an HPD event
> -	 * in between, so try to avoid the write when we can.
> -	 */
> -	val = I915_READ(PORT_TX_DFLEXDPCSSS);
> -	if (!(val & DP_PHY_MODE_STATUS_NOT_SAFE(tc_port))) {
> -		val |= DP_PHY_MODE_STATUS_NOT_SAFE(tc_port);
> -		I915_WRITE(PORT_TX_DFLEXDPCSSS, val);
> -	}
> -
>  	/*
>  	 * Now we have to re-check the live state, in case the port recently
>  	 * became disconnected. Not necessary for legacy mode.
> @@ -5044,24 +5034,6 @@ static bool icl_tc_phy_connect(struct drm_i915_private *dev_priv,
>  static void icl_tc_phy_disconnect(struct drm_i915_private *dev_priv,
>  				  struct intel_digital_port *dig_port)
>  {
> -	enum tc_port tc_port = intel_port_to_tc(dev_priv, dig_port->base.port);
> -
> -	if (dig_port->tc_type == TC_PORT_UNKNOWN)
> -		return;
> -
> -	/*
> -	 * TBT disconnection flow is read the live status, what was done in
> -	 * caller.
> -	 */
> -	if (dig_port->tc_type == TC_PORT_TYPEC ||
> -	    dig_port->tc_type == TC_PORT_LEGACY) {
> -		u32 val;
> -
> -		val = I915_READ(PORT_TX_DFLEXDPCSSS);
> -		val &= ~DP_PHY_MODE_STATUS_NOT_SAFE(tc_port);
> -		I915_WRITE(PORT_TX_DFLEXDPCSSS, val);
> -	}
> -
>  	dig_port->tc_type = TC_PORT_UNKNOWN;
>  }
>  
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index 6c453366cd24..dab5f90646c4 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -465,6 +465,48 @@ icl_combo_phy_aux_power_well_disable(struct drm_i915_private *dev_priv,
>  	hsw_wait_for_power_well_disable(dev_priv, power_well);
>  }
>  
> +static void icl_tc_grab_control(struct drm_i915_private *dev_priv,
> +				enum aux_ch aux_ch, bool grab)
> +{
> +	struct drm_device *dev = &dev_priv->drm;
> +	struct drm_connector_list_iter conn_iter;
> +	struct drm_connector *connector;
> +
> +	drm_connector_list_iter_begin(dev, &conn_iter);
> +	drm_for_each_connector_iter(connector, &conn_iter) {
> +		struct intel_connector *intel_connector;
> +		struct intel_encoder *intel_encoder;
> +		struct intel_digital_port *dig_port;
> +		enum tc_port tc_port;
> +
> +		intel_connector = to_intel_connector(connector);
> +		if (!intel_connector->encoder)
> +			continue;
> +		intel_encoder = intel_connector->encoder;
> +		dig_port = enc_to_dig_port(&intel_encoder->base);
> +
> +		if (!dig_port || dig_port->aux_ch != aux_ch)
> +			continue;
> +
> +		tc_port = intel_port_to_tc(dev_priv, dig_port->base.port);
> +
> +		if (dig_port->tc_type == TC_PORT_TYPEC ||
> +		    dig_port->tc_type == TC_PORT_LEGACY) {
> +			u32 val;
> +
> +			val = I915_READ(PORT_TX_DFLEXDPCSSS);
> +			if (grab)
> +				val |= DP_PHY_MODE_STATUS_NOT_SAFE(tc_port);
> +			else
> +				val &= ~DP_PHY_MODE_STATUS_NOT_SAFE(tc_port);
> +			I915_WRITE(PORT_TX_DFLEXDPCSSS, val);
> +		}
> +
> +		break;
> +	}
> +	drm_connector_list_iter_end(&conn_iter);
> +}
> +
>  #define ICL_AUX_PW_TO_CH(pw_idx)	\
>  	((pw_idx) - ICL_PW_CTL_IDX_AUX_A + AUX_CH_A)
>  
> @@ -475,6 +517,8 @@ icl_tc_phy_aux_power_well_enable(struct drm_i915_private *dev_priv,
>  	enum aux_ch aux_ch = ICL_AUX_PW_TO_CH(power_well->desc->hsw.idx);
>  	u32 val;
>  
> +	icl_tc_grab_control(dev_priv, aux_ch, true);
> +
>  	val = I915_READ(DP_AUX_CH_CTL(aux_ch));
>  	val &= ~DP_AUX_CH_CTL_TBT_IO;
>  	if (power_well->desc->hsw.is_tc_tbt)
> @@ -484,6 +528,15 @@ icl_tc_phy_aux_power_well_enable(struct drm_i915_private *dev_priv,
>  	hsw_power_well_enable(dev_priv, power_well);
>  }
>  
> +static void icl_tc_phy_aux_power_well_disable(struct drm_i915_private *dev_priv,
> +					      struct i915_power_well *power_well)
> +{
> +	enum aux_ch aux_ch = ICL_AUX_PW_TO_CH(power_well->desc->hsw.idx);
> +
> +	icl_tc_grab_control(dev_priv, aux_ch, false);
> +	hsw_power_well_disable(dev_priv, power_well);
> +}
> +
>  /*
>   * We should only use the power well if we explicitly asked the hardware to
>   * enable it, so check if it's enabled and also check if we've requested it to
> @@ -2754,7 +2807,7 @@ 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,
> -	.disable = hsw_power_well_disable,
> +	.disable = icl_tc_phy_aux_power_well_disable,
>  	.is_enabled = hsw_power_well_enabled,
>  };
>  
> -- 
> 2.19.1
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/4] drm/i915/icl: Only grab TC ports when using it

[Souza, Jose]

On Sat, 2018-11-03 at 01:06 +0200, Imre Deak wrote:
> On Fri, Nov 02, 2018 at 01:39:23PM -0700, José Roberto de Souza
> wrote:
> > When suspending or unloading the driver, it needs to release the
> > TC ports so HW can change it state without wait for driver
> > handshake.
> > Spec also state that if the port is not used by driver it should
> > release TC access, so here only grabbing control of the TC ports
> > and
> > marking as unsafe when aux power is needed as have aux power well
> > is
> > a requirement to have DDI enabled in TC ports, the pre_pll_enable
> > and
> > post_pll_disable hooks takes care of getting and releasing it.
> > 
> > BSpec: 21750
> > 
> > Cc: Imre Deak <imre.deak@intel.com>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> 
> Agreed that we should force a manual disconnect before entering
> low-power states and driver unloading, but I don't think this should
> be
> done from the power well code. We could perform multiple AUX
> transfers
> after a connect event around each of which we would enable/disable
> the
> AUX power well. We would then likely continue doing a modeset. During
> this whole sequence I don't think we should do forced
> connects/disconnects due to the AUX power well getting
> enabled/disabled.
> 
> I think normally we should change the connection status (that is the
> safe/unsafe mode you're setting here) in response to HPD events, also
> considering that we may have to delay changing the state as discussed
> earlier with Ville (due to an ongoing AUX transfer or active mode in
> the

What do you think about use power_well->count to delay when type-
c/legacy is disconnected?
Then when the last reference is taken
icl_tc_phy_aux_power_well_disable() check if the TC live status is
disconnected and mark as unsafe.


> opposite TypeC mode). Then only during system/runtime suspend and
> unload
> should we do a forced disconnect, which would be safe since at those
> points we don't have any pending AUX transfers or active outputs.
> 
> --Imre
> 
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c         | 28 -------------
> >  drivers/gpu/drm/i915/intel_runtime_pm.c | 55
> > ++++++++++++++++++++++++-
> >  2 files changed, 54 insertions(+), 29 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > b/drivers/gpu/drm/i915/intel_dp.c
> > index 52a54ef746af..d978127e7208 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -5013,16 +5013,6 @@ static bool icl_tc_phy_connect(struct
> > drm_i915_private *dev_priv,
> >  		return false;
> >  	}
> >  
> > -	/*
> > -	 * This function may be called many times in a row without an
> > HPD event
> > -	 * in between, so try to avoid the write when we can.
> > -	 */
> > -	val = I915_READ(PORT_TX_DFLEXDPCSSS);
> > -	if (!(val & DP_PHY_MODE_STATUS_NOT_SAFE(tc_port))) {
> > -		val |= DP_PHY_MODE_STATUS_NOT_SAFE(tc_port);
> > -		I915_WRITE(PORT_TX_DFLEXDPCSSS, val);
> > -	}
> > -
> >  	/*
> >  	 * Now we have to re-check the live state, in case the port
> > recently
> >  	 * became disconnected. Not necessary for legacy mode.
> > @@ -5044,24 +5034,6 @@ static bool icl_tc_phy_connect(struct
> > drm_i915_private *dev_priv,
> >  static void icl_tc_phy_disconnect(struct drm_i915_private
> > *dev_priv,
> >  				  struct intel_digital_port *dig_port)
> >  {
> > -	enum tc_port tc_port = intel_port_to_tc(dev_priv, dig_port-
> > >base.port);
> > -
> > -	if (dig_port->tc_type == TC_PORT_UNKNOWN)
> > -		return;
> > -
> > -	/*
> > -	 * TBT disconnection flow is read the live status, what was
> > done in
> > -	 * caller.
> > -	 */
> > -	if (dig_port->tc_type == TC_PORT_TYPEC ||
> > -	    dig_port->tc_type == TC_PORT_LEGACY) {
> > -		u32 val;
> > -
> > -		val = I915_READ(PORT_TX_DFLEXDPCSSS);
> > -		val &= ~DP_PHY_MODE_STATUS_NOT_SAFE(tc_port);
> > -		I915_WRITE(PORT_TX_DFLEXDPCSSS, val);
> > -	}
> > -
> >  	dig_port->tc_type = TC_PORT_UNKNOWN;
> >  }
> >  
> > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > index 6c453366cd24..dab5f90646c4 100644
> > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > @@ -465,6 +465,48 @@ icl_combo_phy_aux_power_well_disable(struct
> > drm_i915_private *dev_priv,
> >  	hsw_wait_for_power_well_disable(dev_priv, power_well);
> >  }
> >  
> > +static void icl_tc_grab_control(struct drm_i915_private *dev_priv,
> > +				enum aux_ch aux_ch, bool grab)
> > +{
> > +	struct drm_device *dev = &dev_priv->drm;
> > +	struct drm_connector_list_iter conn_iter;
> > +	struct drm_connector *connector;
> > +
> > +	drm_connector_list_iter_begin(dev, &conn_iter);
> > +	drm_for_each_connector_iter(connector, &conn_iter) {
> > +		struct intel_connector *intel_connector;
> > +		struct intel_encoder *intel_encoder;
> > +		struct intel_digital_port *dig_port;
> > +		enum tc_port tc_port;
> > +
> > +		intel_connector = to_intel_connector(connector);
> > +		if (!intel_connector->encoder)
> > +			continue;
> > +		intel_encoder = intel_connector->encoder;
> > +		dig_port = enc_to_dig_port(&intel_encoder->base);
> > +
> > +		if (!dig_port || dig_port->aux_ch != aux_ch)
> > +			continue;
> > +
> > +		tc_port = intel_port_to_tc(dev_priv, dig_port-
> > >base.port);
> > +
> > +		if (dig_port->tc_type == TC_PORT_TYPEC ||
> > +		    dig_port->tc_type == TC_PORT_LEGACY) {
> > +			u32 val;
> > +
> > +			val = I915_READ(PORT_TX_DFLEXDPCSSS);
> > +			if (grab)
> > +				val |=
> > DP_PHY_MODE_STATUS_NOT_SAFE(tc_port);
> > +			else
> > +				val &=
> > ~DP_PHY_MODE_STATUS_NOT_SAFE(tc_port);
> > +			I915_WRITE(PORT_TX_DFLEXDPCSSS, val);
> > +		}
> > +
> > +		break;
> > +	}
> > +	drm_connector_list_iter_end(&conn_iter);
> > +}
> > +
> >  #define ICL_AUX_PW_TO_CH(pw_idx)	\
> >  	((pw_idx) - ICL_PW_CTL_IDX_AUX_A + AUX_CH_A)
> >  
> > @@ -475,6 +517,8 @@ icl_tc_phy_aux_power_well_enable(struct
> > drm_i915_private *dev_priv,
> >  	enum aux_ch aux_ch = ICL_AUX_PW_TO_CH(power_well->desc-
> > >hsw.idx);
> >  	u32 val;
> >  
> > +	icl_tc_grab_control(dev_priv, aux_ch, true);
> > +
> >  	val = I915_READ(DP_AUX_CH_CTL(aux_ch));
> >  	val &= ~DP_AUX_CH_CTL_TBT_IO;
> >  	if (power_well->desc->hsw.is_tc_tbt)
> > @@ -484,6 +528,15 @@ icl_tc_phy_aux_power_well_enable(struct
> > drm_i915_private *dev_priv,
> >  	hsw_power_well_enable(dev_priv, power_well);
> >  }
> >  
> > +static void icl_tc_phy_aux_power_well_disable(struct
> > drm_i915_private *dev_priv,
> > +					      struct i915_power_well
> > *power_well)
> > +{
> > +	enum aux_ch aux_ch = ICL_AUX_PW_TO_CH(power_well->desc-
> > >hsw.idx);
> > +
> > +	icl_tc_grab_control(dev_priv, aux_ch, false);
> > +	hsw_power_well_disable(dev_priv, power_well);
> > +}
> > +
> >  /*
> >   * We should only use the power well if we explicitly asked the
> > hardware to
> >   * enable it, so check if it's enabled and also check if we've
> > requested it to
> > @@ -2754,7 +2807,7 @@ 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,
> > -	.disable = hsw_power_well_disable,
> > +	.disable = icl_tc_phy_aux_power_well_disable,
> >  	.is_enabled = hsw_power_well_enabled,
> >  };
> >  
> > -- 
> > 2.19.1
> > 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/4] drm/i915/icl: Only grab TC ports when using it

[Imre Deak]

On Sat, Nov 03, 2018 at 01:41:12AM +0200, Souza, Jose wrote:
> On Sat, 2018-11-03 at 01:06 +0200, Imre Deak wrote:
> > On Fri, Nov 02, 2018 at 01:39:23PM -0700, José Roberto de Souza
> > wrote:
> > > When suspending or unloading the driver, it needs to release the
> > > TC ports so HW can change it state without wait for driver
> > > handshake.
> > > Spec also state that if the port is not used by driver it should
> > > release TC access, so here only grabbing control of the TC ports
> > > and
> > > marking as unsafe when aux power is needed as have aux power well
> > > is
> > > a requirement to have DDI enabled in TC ports, the pre_pll_enable
> > > and
> > > post_pll_disable hooks takes care of getting and releasing it.
> > > 
> > > BSpec: 21750
> > > 
> > > Cc: Imre Deak <imre.deak@intel.com>
> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > 
> > Agreed that we should force a manual disconnect before entering
> > low-power states and driver unloading, but I don't think this should
> > be
> > done from the power well code. We could perform multiple AUX
> > transfers
> > after a connect event around each of which we would enable/disable
> > the
> > AUX power well. We would then likely continue doing a modeset. During
> > this whole sequence I don't think we should do forced
> > connects/disconnects due to the AUX power well getting
> > enabled/disabled.
> > 
> > I think normally we should change the connection status (that is the
> > safe/unsafe mode you're setting here) in response to HPD events, also
> > considering that we may have to delay changing the state as discussed
> > earlier with Ville (due to an ongoing AUX transfer or active mode in
> > the
> 
> What do you think about use power_well->count to delay when type-
> c/legacy is disconnected?  Then when the last reference is taken
> icl_tc_phy_aux_power_well_disable() check if the TC live status is
> disconnected and mark as unsafe.
                   ^mark as safe if I read the spec correctly, whatever
		   that means.

I think this should be done from the suspend/unload handlers. At those
places we should put the controller into safe state regardless of the
live status.

> > opposite TypeC mode). Then only during system/runtime suspend and
> > unload
> > should we do a forced disconnect, which would be safe since at those
> > points we don't have any pending AUX transfers or active outputs.
> > 
> > --Imre
> > 
> > > ---
> > >  drivers/gpu/drm/i915/intel_dp.c         | 28 -------------
> > >  drivers/gpu/drm/i915/intel_runtime_pm.c | 55
> > > ++++++++++++++++++++++++-
> > >  2 files changed, 54 insertions(+), 29 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > > b/drivers/gpu/drm/i915/intel_dp.c
> > > index 52a54ef746af..d978127e7208 100644
> > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > @@ -5013,16 +5013,6 @@ static bool icl_tc_phy_connect(struct
> > > drm_i915_private *dev_priv,
> > >  		return false;
> > >  	}
> > >  
> > > -	/*
> > > -	 * This function may be called many times in a row without an
> > > HPD event
> > > -	 * in between, so try to avoid the write when we can.
> > > -	 */
> > > -	val = I915_READ(PORT_TX_DFLEXDPCSSS);
> > > -	if (!(val & DP_PHY_MODE_STATUS_NOT_SAFE(tc_port))) {
> > > -		val |= DP_PHY_MODE_STATUS_NOT_SAFE(tc_port);
> > > -		I915_WRITE(PORT_TX_DFLEXDPCSSS, val);
> > > -	}
> > > -
> > >  	/*
> > >  	 * Now we have to re-check the live state, in case the port
> > > recently
> > >  	 * became disconnected. Not necessary for legacy mode.
> > > @@ -5044,24 +5034,6 @@ static bool icl_tc_phy_connect(struct
> > > drm_i915_private *dev_priv,
> > >  static void icl_tc_phy_disconnect(struct drm_i915_private
> > > *dev_priv,
> > >  				  struct intel_digital_port *dig_port)
> > >  {
> > > -	enum tc_port tc_port = intel_port_to_tc(dev_priv, dig_port-
> > > >base.port);
> > > -
> > > -	if (dig_port->tc_type == TC_PORT_UNKNOWN)
> > > -		return;
> > > -
> > > -	/*
> > > -	 * TBT disconnection flow is read the live status, what was
> > > done in
> > > -	 * caller.
> > > -	 */
> > > -	if (dig_port->tc_type == TC_PORT_TYPEC ||
> > > -	    dig_port->tc_type == TC_PORT_LEGACY) {
> > > -		u32 val;
> > > -
> > > -		val = I915_READ(PORT_TX_DFLEXDPCSSS);
> > > -		val &= ~DP_PHY_MODE_STATUS_NOT_SAFE(tc_port);
> > > -		I915_WRITE(PORT_TX_DFLEXDPCSSS, val);
> > > -	}
> > > -
> > >  	dig_port->tc_type = TC_PORT_UNKNOWN;
> > >  }
> > >  
> > > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > index 6c453366cd24..dab5f90646c4 100644
> > > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > @@ -465,6 +465,48 @@ icl_combo_phy_aux_power_well_disable(struct
> > > drm_i915_private *dev_priv,
> > >  	hsw_wait_for_power_well_disable(dev_priv, power_well);
> > >  }
> > >  
> > > +static void icl_tc_grab_control(struct drm_i915_private *dev_priv,
> > > +				enum aux_ch aux_ch, bool grab)
> > > +{
> > > +	struct drm_device *dev = &dev_priv->drm;
> > > +	struct drm_connector_list_iter conn_iter;
> > > +	struct drm_connector *connector;
> > > +
> > > +	drm_connector_list_iter_begin(dev, &conn_iter);
> > > +	drm_for_each_connector_iter(connector, &conn_iter) {
> > > +		struct intel_connector *intel_connector;
> > > +		struct intel_encoder *intel_encoder;
> > > +		struct intel_digital_port *dig_port;
> > > +		enum tc_port tc_port;
> > > +
> > > +		intel_connector = to_intel_connector(connector);
> > > +		if (!intel_connector->encoder)
> > > +			continue;
> > > +		intel_encoder = intel_connector->encoder;
> > > +		dig_port = enc_to_dig_port(&intel_encoder->base);
> > > +
> > > +		if (!dig_port || dig_port->aux_ch != aux_ch)
> > > +			continue;
> > > +
> > > +		tc_port = intel_port_to_tc(dev_priv, dig_port-
> > > >base.port);
> > > +
> > > +		if (dig_port->tc_type == TC_PORT_TYPEC ||
> > > +		    dig_port->tc_type == TC_PORT_LEGACY) {
> > > +			u32 val;
> > > +
> > > +			val = I915_READ(PORT_TX_DFLEXDPCSSS);
> > > +			if (grab)
> > > +				val |=
> > > DP_PHY_MODE_STATUS_NOT_SAFE(tc_port);
> > > +			else
> > > +				val &=
> > > ~DP_PHY_MODE_STATUS_NOT_SAFE(tc_port);
> > > +			I915_WRITE(PORT_TX_DFLEXDPCSSS, val);
> > > +		}
> > > +
> > > +		break;
> > > +	}
> > > +	drm_connector_list_iter_end(&conn_iter);
> > > +}
> > > +
> > >  #define ICL_AUX_PW_TO_CH(pw_idx)	\
> > >  	((pw_idx) - ICL_PW_CTL_IDX_AUX_A + AUX_CH_A)
> > >  
> > > @@ -475,6 +517,8 @@ icl_tc_phy_aux_power_well_enable(struct
> > > drm_i915_private *dev_priv,
> > >  	enum aux_ch aux_ch = ICL_AUX_PW_TO_CH(power_well->desc-
> > > >hsw.idx);
> > >  	u32 val;
> > >  
> > > +	icl_tc_grab_control(dev_priv, aux_ch, true);
> > > +
> > >  	val = I915_READ(DP_AUX_CH_CTL(aux_ch));
> > >  	val &= ~DP_AUX_CH_CTL_TBT_IO;
> > >  	if (power_well->desc->hsw.is_tc_tbt)
> > > @@ -484,6 +528,15 @@ icl_tc_phy_aux_power_well_enable(struct
> > > drm_i915_private *dev_priv,
> > >  	hsw_power_well_enable(dev_priv, power_well);
> > >  }
> > >  
> > > +static void icl_tc_phy_aux_power_well_disable(struct
> > > drm_i915_private *dev_priv,
> > > +					      struct i915_power_well
> > > *power_well)
> > > +{
> > > +	enum aux_ch aux_ch = ICL_AUX_PW_TO_CH(power_well->desc-
> > > >hsw.idx);
> > > +
> > > +	icl_tc_grab_control(dev_priv, aux_ch, false);
> > > +	hsw_power_well_disable(dev_priv, power_well);
> > > +}
> > > +
> > >  /*
> > >   * We should only use the power well if we explicitly asked the
> > > hardware to
> > >   * enable it, so check if it's enabled and also check if we've
> > > requested it to
> > > @@ -2754,7 +2807,7 @@ 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,
> > > -	.disable = hsw_power_well_disable,
> > > +	.disable = icl_tc_phy_aux_power_well_disable,
> > >  	.is_enabled = hsw_power_well_enabled,
> > >  };
> > >  
> > > -- 
> > > 2.19.1
> > > 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[José Roberto de Souza]

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

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

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

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 2a88a7eb871b..b81f8b55750e 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2789,6 +2789,7 @@ enum hpd_pin intel_hpd_pin_default(struct drm_i915_private *dev_priv,
 				   enum port port);
 bool intel_hpd_disable(struct drm_i915_private *dev_priv, enum hpd_pin pin);
 void intel_hpd_enable(struct drm_i915_private *dev_priv, enum hpd_pin pin);
+void intel_hotplug_tc_wa_work(struct work_struct *__work);
 
 /* i915_irq.c */
 static inline void i915_queue_hangcheck(struct drm_i915_private *dev_priv)
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index d978127e7208..2042711887f1 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -5379,6 +5379,7 @@ void intel_dp_encoder_destroy(struct drm_encoder *encoder)
 {
 	struct intel_digital_port *intel_dig_port = enc_to_dig_port(encoder);
 	struct intel_dp *intel_dp = &intel_dig_port->dp;
+	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
 
 	intel_dp_mst_encoder_cleanup(intel_dig_port);
 	if (intel_dp_is_edp(intel_dp)) {
@@ -5395,6 +5396,8 @@ void intel_dp_encoder_destroy(struct drm_encoder *encoder)
 			unregister_reboot_notifier(&intel_dp->edp_notifier);
 			intel_dp->edp_notifier.notifier_call = NULL;
 		}
+	} else if (IS_ICELAKE(dev_priv)) {
+		cancel_delayed_work_sync(&intel_dp->tc_wa_work);
 	}
 
 	intel_dp_aux_fini(intel_dp);
@@ -6758,6 +6761,10 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
 		I915_WRITE(PEG_BAND_GAP_DATA, (temp & ~0xf) | 0xd);
 	}
 
+	if (IS_ICELAKE(dev_priv) && !intel_dp_is_edp(intel_dp))
+		INIT_DELAYED_WORK(&intel_dp->tc_wa_work,
+				  intel_hotplug_tc_wa_work);
+
 	return true;
 
 fail:
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 191c26e17f2d..06d282e4ecb7 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1137,7 +1137,11 @@ struct intel_dp {
 	int panel_power_cycle_delay;
 	int backlight_on_delay;
 	int backlight_off_delay;
-	struct delayed_work panel_vdd_work;
+	union {
+		struct delayed_work panel_vdd_work;
+		struct delayed_work tc_wa_work;
+	};
+	u8 tc_wa_count;
 	bool want_panel_vdd;
 	unsigned long last_power_on;
 	unsigned long last_backlight_off;
diff --git a/drivers/gpu/drm/i915/intel_hotplug.c b/drivers/gpu/drm/i915/intel_hotplug.c
index 648a13c6043c..d4094db1a3c3 100644
--- a/drivers/gpu/drm/i915/intel_hotplug.c
+++ b/drivers/gpu/drm/i915/intel_hotplug.c
@@ -323,6 +323,64 @@ static void i915_digport_work_func(struct work_struct *work)
 	}
 }
 
+#define TC_WA_DELAY_MSEC 150
+#define TC_WA_TRIES 5
+
+/*
+ * Test if TC dongle is responsive return true if so otherwise schedule a
+ * work to try again and return false
+ */
+static bool intel_hotplug_tc_wa_test(struct intel_dp *intel_dp)
+{
+	u8 buff;
+
+	intel_dp->tc_wa_count++;
+
+	if (drm_dp_dpcd_read(&intel_dp->aux, DP_DPCD_REV, &buff, 1) != 1)
+		goto not_responsive;
+
+	if (!drm_probe_ddc(&intel_dp->aux.ddc))
+		goto not_responsive;
+
+	return true;
+
+not_responsive:
+	if (intel_dp->tc_wa_count < TC_WA_TRIES) {
+		unsigned long delay;
+
+		delay = msecs_to_jiffies(TC_WA_DELAY_MSEC);
+		schedule_delayed_work(&intel_dp->tc_wa_work, delay);
+	} else {
+		DRM_DEBUG_KMS("TC not responsive, giving up\n");
+	}
+
+	return false;
+}
+
+void intel_hotplug_tc_wa_work(struct work_struct *__work)
+{
+	struct intel_dp *intel_dp = container_of(to_delayed_work(__work),
+						 struct intel_dp, tc_wa_work);
+	struct intel_connector *intel_connector = intel_dp->attached_connector;
+	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
+	struct intel_encoder *intel_encoder = &intel_dig_port->base;
+	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
+	struct drm_device *dev = &dev_priv->drm;
+	bool ret;
+
+	if (!intel_digital_port_connected(intel_encoder))
+		return;
+
+	mutex_lock(&dev->mode_config.mutex);
+	ret = intel_hotplug_tc_wa_test(intel_dp);
+	if (ret)
+		ret = intel_encoder->hotplug(intel_encoder, intel_connector);
+	mutex_unlock(&dev->mode_config.mutex);
+
+	if (ret)
+		drm_kms_helper_hotplug_event(dev);
+}
+
 /*
  * Handle hotplug events outside the interrupt handler proper.
  */
@@ -358,9 +416,27 @@ static void i915_hotplug_work_func(struct work_struct *work)
 			continue;
 		intel_encoder = intel_connector->encoder;
 		if (hpd_event_bits & (1 << intel_encoder->hpd_pin)) {
+			struct intel_digital_port *dig_port = enc_to_dig_port(&intel_encoder->base);
+
 			DRM_DEBUG_KMS("Connector %s (pin %i) received hotplug event.\n",
 				      connector->name, intel_encoder->hpd_pin);
 
+			/*
+			 * TC WA: TC dongles can takes some time to be
+			 * responsible, so let's try to do a DPCD read to check
+			 * if it is ready, otherwise try again in a few msecs.
+			 */
+			if (IS_ICELAKE(dev_priv) &&
+			    intel_digital_port_connected(intel_encoder) &&
+			    dig_port->tc_type != TC_PORT_LEGACY) {
+				struct intel_dp *intel_dp;
+
+				intel_dp = enc_to_intel_dp(&intel_encoder->base);
+				intel_dp->tc_wa_count = 0;
+				if (!intel_hotplug_tc_wa_test(intel_dp))
+					continue;
+			}
+
 			changed |= intel_encoder->hotplug(intel_encoder,
 							  intel_connector);
 		}
-- 
2.19.1

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

✗ Fi.CI.SPARSE: warning for series starting with [1/4] drm/i915: Reuse the aux_domain cached

[Patchwork]

== Series Details ==

Series: series starting with [1/4] drm/i915: Reuse the aux_domain cached
URL   : https://patchwork.freedesktop.org/series/51983/
State : warning

== Summary ==

$ dim sparse origin/drm-tip
Sparse version: v0.5.2
Commit: drm/i915: Reuse the aux_domain cached
Okay!

Commit: drm/i915: Release DDI power well references in MST ports
Okay!

Commit: drm/i915/icl: Only grab TC ports when using it
Okay!

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

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

✗ Fi.CI.BAT: failure for series starting with [1/4] drm/i915: Reuse the aux_domain cached

[Patchwork]

== Series Details ==

Series: series starting with [1/4] drm/i915: Reuse the aux_domain cached
URL   : https://patchwork.freedesktop.org/series/51983/
State : failure

== Summary ==

= CI Bug Log - changes from CI_DRM_5081 -> Patchwork_10717 =

== Summary - FAILURE ==

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

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

== Possible new issues ==

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

  === IGT changes ===

    ==== Possible regressions ====

    igt@drv_module_reload@basic-no-display:
      fi-icl-u2:          PASS -> DMESG-WARN +2

    igt@drv_module_reload@basic-reload:
      fi-icl-u:           PASS -> DMESG-WARN +2

    
== Known issues ==

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

  === IGT changes ===

    ==== Issues hit ====

    igt@gem_exec_suspend@basic-s3:
      fi-kbl-soraka:      NOTRUN -> INCOMPLETE (fdo#107859, fdo#107774, fdo#107556)

    igt@prime_vgem@basic-fence-flip:
      fi-apl-guc:         PASS -> FAIL (fdo#104008)

    
    ==== Possible fixes ====

    igt@kms_flip@basic-flip-vs-modeset:
      fi-skl-6700hq:      DMESG-WARN (fdo#105998) -> PASS

    
  fdo#104008 https://bugs.freedesktop.org/show_bug.cgi?id=104008
  fdo#105998 https://bugs.freedesktop.org/show_bug.cgi?id=105998
  fdo#107556 https://bugs.freedesktop.org/show_bug.cgi?id=107556
  fdo#107774 https://bugs.freedesktop.org/show_bug.cgi?id=107774
  fdo#107859 https://bugs.freedesktop.org/show_bug.cgi?id=107859


== Participating hosts (50 -> 45) ==

  Additional (1): fi-kbl-soraka 
  Missing    (6): fi-ilk-m540 fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-ctg-p8600 fi-kbl-7560u 


== Build changes ==

    * Linux: CI_DRM_5081 -> Patchwork_10717

  CI_DRM_5081: f5e16acf6c85d38756c3efdb77ec6aede55df0ba @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4705: 7983e19ed62ec8db1884f55e07e458a62cc51e37 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_10717: 722af784c0a60ffb61cd71fd9fcbdba7bfcd1b48 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

722af784c0a6 drm/i915/icl: Delay hotplug processing for tc ports
9c011275643e drm/i915/icl: Only grab TC ports when using it
61201ac1c365 drm/i915: Release DDI power well references in MST ports
d81abcdc9b49 drm/i915: Reuse the aux_domain cached

== Logs ==

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

Re: [PATCH 1/4] drm/i915: Reuse the aux_domain cached

[Imre Deak]

On Fri, Nov 02, 2018 at 01:39:21PM -0700, José Roberto de Souza wrote:
> intel_dp_detect() caches the aux_domain in the beginning of the
> function as it is used twice, so lets also use it as the aux_domain
> don't change in runtime.
> 
> Cc: Imre Deak <imre.deak@intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index e7233dfa1794..52a54ef746af 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -5289,8 +5289,7 @@ intel_dp_detect(struct drm_connector *connector,
>  
>  		ret = intel_dp_retrain_link(encoder, ctx);
>  		if (ret) {
> -			intel_display_power_put(dev_priv,
> -						intel_aux_power_domain(dig_port));
> +			intel_display_power_put(dev_priv, aux_domain);

Thanks for catching it.

			status = ret;
			goto out_put_power;

and adding the label at the end would be cleaner. Either way:

Reviewed-by: Imre Deak <imre.deak@intel.com>

>  			return ret;
>  		}
>  	}
> -- 
> 2.19.1
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx