[Intel-gfx] [PATCH 3/4] drm/i915/icl: Only grab TC ports when using it
Imre Deak
imre.deak at intel.com
Fri Nov 2 23:06:03 UTC 2018
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 at intel.com>
> Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza at 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
>
More information about the Intel-gfx
mailing list