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

[Imre Deak <imre.deak@intel.com>]

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