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

["Souza, Jose" <jose.souza@intel.com>]

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