On Tue, 2021-09-28 at 22:34 +0300, Imre Deak wrote:
> On Tue, Sep 28, 2021 at 10:18:25PM +0300, Souza, Jose wrote:
> > On Tue, 2021-09-28 at 00:46 +0300, Imre Deak wrote:
> > > On Tue, Sep 28, 2021 at 12:16:45AM +0300, Souza, Jose wrote:
> > > > On Tue, 2021-09-21 at 03:23 +0300, Imre Deak wrote:
> > > > > A follow-up change will start to disconnect/re-connect PHYs around AUX
> > > > > transfers and modeset enable/disables. To prepare for that add a new
> > > > > TypeC PHY disconnected mode, to help tracking the TC-cold blocking power
> > > > > domain status (no power domain in disconnected state, mode dependent
> > > > > power domain in connected state).
> > > > >
> > > > > Cc: José Roberto de Souza <jose.souza@xxxxxxxxx>
> > > > > Signed-off-by: Imre Deak <imre.deak@xxxxxxxxx>
> > > > > ---
> > > > > drivers/gpu/drm/i915/display/intel_display.h | 1 +
> > > > > drivers/gpu/drm/i915/display/intel_tc.c | 26 ++++++++++++++------
> > > > > 2 files changed, 19 insertions(+), 8 deletions(-)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.h b/drivers/gpu/drm/i915/display/intel_display.h
> > > > > index d425ee77aad71..87b96fed5e0ba 100644
> > > > > --- a/drivers/gpu/drm/i915/display/intel_display.h
> > > > > +++ b/drivers/gpu/drm/i915/display/intel_display.h
> > > > > @@ -270,6 +270,7 @@ enum tc_port {
> > > > > };
> > > > >
> > > > > enum tc_port_mode {
> > > > > + TC_PORT_DISCONNECTED,
> > > > > TC_PORT_TBT_ALT,
> > > > > TC_PORT_DP_ALT,
> > > > > TC_PORT_LEGACY,
> > > > > diff --git a/drivers/gpu/drm/i915/display/intel_tc.c b/drivers/gpu/drm/i915/display/intel_tc.c
> > > > > index aa4c1e5e0c002..77b16a7c43466 100644
> > > > > --- a/drivers/gpu/drm/i915/display/intel_tc.c
> > > > > +++ b/drivers/gpu/drm/i915/display/intel_tc.c
> > > > > @@ -12,13 +12,14 @@
> > > > > static const char *tc_port_mode_name(enum tc_port_mode mode)
> > > > > {
> > > > > static const char * const names[] = {
> > > > > + [TC_PORT_DISCONNECTED] = "disconnected",
> > > > > [TC_PORT_TBT_ALT] = "tbt-alt",
> > > > > [TC_PORT_DP_ALT] = "dp-alt",
> > > > > [TC_PORT_LEGACY] = "legacy",
> > > > > };
> > > > >
> > > > > if (WARN_ON(mode >= ARRAY_SIZE(names)))
> > > > > - mode = TC_PORT_TBT_ALT;
> > > > > + mode = TC_PORT_DISCONNECTED;
> > > > >
> > > > > return names[mode];
> > > > > }
> > > > > @@ -513,10 +514,11 @@ static void icl_tc_phy_disconnect(struct intel_digital_port *dig_port)
> > > > > case TC_PORT_LEGACY:
> > > > > case TC_PORT_DP_ALT:
> > > > > tc_phy_take_ownership(dig_port, false);
> > > > > - dig_port->tc_mode = TC_PORT_TBT_ALT;
> > > > > - break;
> > > > > + fallthrough;
> > > > > case TC_PORT_TBT_ALT:
> > > > > - /* Nothing to do, we stay in TBT-alt mode */
> > > > > + dig_port->tc_mode = TC_PORT_DISCONNECTED;
> > > > > + fallthrough;
> > > > > + case TC_PORT_DISCONNECTED:
> > > > > break;
> > > > > default:
> > > > > MISSING_CASE(dig_port->tc_mode);
> > > > > @@ -621,31 +623,34 @@ void intel_tc_port_sanitize(struct intel_digital_port *dig_port)
> > > > > {
> > > > > struct drm_i915_private *i915 = to_i915(dig_port->base.base.dev);
> > > > > struct intel_encoder *encoder = &dig_port->base;
> > > > > - intel_wakeref_t tc_cold_wref;
> > > > > int active_links = 0;
> > > > >
> > > > > mutex_lock(&dig_port->tc_lock);
> > > > > - tc_cold_wref = tc_cold_block(dig_port);
> > > > >
> > > > > - dig_port->tc_mode = intel_tc_port_get_current_mode(dig_port);
> > > > > if (dig_port->dp.is_mst)
> > > > > active_links = intel_dp_mst_encoder_active_links(dig_port);
> > > > > else if (encoder->base.crtc)
> > > > > active_links = to_intel_crtc(encoder->base.crtc)->active;
> > > > >
> > > > > + drm_WARN_ON(&i915->drm, dig_port->tc_mode != TC_PORT_DISCONNECTED);
> > > > > if (active_links) {
> > > > > + intel_wakeref_t tc_cold_wref = tc_cold_block(dig_port);
> > > > > +
> > > > > + dig_port->tc_mode = intel_tc_port_get_current_mode(dig_port);
> > > > > +
> > > > > if (!icl_tc_phy_is_connected(dig_port))
> > > > > drm_dbg_kms(&i915->drm,
> > > > > "Port %s: PHY disconnected with %d active link(s)\n",
> > > > > dig_port->tc_port_name, active_links);
> > > > > intel_tc_port_link_init_refcount(dig_port, active_links);
> > > > > +
> > > > > + tc_cold_unblock(dig_port, tc_cold_wref);
> > > > > }
> > > > >
> > > > > drm_dbg_kms(&i915->drm, "Port %s: sanitize mode (%s)\n",
> > > > > dig_port->tc_port_name,
> > > > > tc_port_mode_name(dig_port->tc_mode));
> > > > >
> > > > > - tc_cold_unblock(dig_port, tc_cold_wref);
> > > > > mutex_unlock(&dig_port->tc_lock);
> > > > > }
> > > > >
> > > > > @@ -704,6 +709,10 @@ static void __intel_tc_port_lock(struct intel_digital_port *dig_port,
> > > > > tc_cold_unblock(dig_port, tc_cold_wref);
> > > > > }
> > > > >
> > > > > + drm_WARN_ON(&i915->drm, dig_port->tc_mode == TC_PORT_DISCONNECTED);
> > > >
> > > > This warning will be printed everytime it goes to suspend, other than that lgtm.
> > >
> > > By the end of the patchset this shouldn't warn. But yes, for bisect to
> > > work this should've been added only in patch 11, I'll move it there.
> >
> > Would not be possible to use TC_PORT_DISCONNECTED when really
> > disconnected and dropping the use of TC_PORT_TBT_ALT for it?
>
> TC_PORT_DISCONNECTED is the state when the PHY ownership is not held and
> we don't hold any power domains.
>
> TC_PORT_TBT_ALT is the state when the PHY ownership is not held (like
> above), and we hold the power domain needed to block TC-cold.
Swapping it would make modes names do what their names intend to.
Up to the point that we only had TBT, TC alt and legacy it was fine to keep into TBT mode when disconnected but now with a disconnected state it do
not make sense to keep it in TBT mode when disconnected.
Or you rename it to TC_PORT_UNKNOWN, as it sets to TC_PORT_DISCONNECTED mode during tc_init() and when going to suspend.
>
> So the TBT_ALT state is not used for the disconnected state.
>
> > Further patches does the proper tc cold exit sequence when mode is
> > TC_PORT_DISCONNECTED, that would cover all the cases from when going
> > from TC_PORT_DISCONNECTED to any other state and would make the code
> > easier to understand.
>
> Tc-cold will always get properly blocked, as bspec requires, when going
> to other states from both the DISCONNECTED and the TBT_ALT state.
>
> > > > > + drm_WARN_ON(&i915->drm, dig_port->tc_mode != TC_PORT_TBT_ALT &&
> > > > > + !tc_phy_is_owned(dig_port));
> > > > > +
> > > > > drm_WARN_ON(&i915->drm, dig_port->tc_lock_wakeref);
> > > > > dig_port->tc_lock_wakeref = wakeref;
> > > > > }
> > > > > @@ -816,6 +825,7 @@ void intel_tc_port_init(struct intel_digital_port *dig_port, bool is_legacy)
> > > > >
> > > > > mutex_init(&dig_port->tc_lock);
> > > > > dig_port->tc_legacy_port = is_legacy;
> > > > > + dig_port->tc_mode = TC_PORT_DISCONNECTED;
> > > > > dig_port->tc_link_refcount = 0;
> > > > > tc_port_load_fia_params(i915, dig_port);
> > > > > }
> > > >
> >
