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@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.
^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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
