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@xxxxxxxxx>
> > Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > Signed-off-by: José Roberto de Souza <jose.souza@xxxxxxxxx>
>
> 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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
