> -----Original Message-----
> From: Intel-gfx <intel-gfx-bounces@xxxxxxxxxxxxxxxxxxxxx> On Behalf Of Imre
> Deak
> Sent: Thursday, September 22, 2022 8:22 PM
> To: intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> Subject: [PATCH] drm/i915: Fix TypeC mode initialization during
> system resume
>
> During system resume DP MST requires AUX to be working already before the
> HW state readout of the given encoder. Since AUX requires the encoder/PHY
> TypeC mode to be initialized, which atm only happens during HW state readout,
> these AUX transfers can change the TypeC mode incorrectly (disconnecting the
> PHY for an enabled encoder) and trigger the state check WARNs in
> intel_tc_port_sanitize().
>
> Fix this by initializing the TypeC mode earlier both during driver loading and
> system resume and making sure that the mode can't change until the encoder's
> state is read out. While at it add the missing DocBook comments and rename
> intel_tc_port_sanitize()->intel_tc_port_sanitize_mode() for consistency.
>
> Signed-off-by: Imre Deak <imre.deak@xxxxxxxxx>
> ---
> drivers/gpu/drm/i915/display/intel_ddi.c | 8 ++-
> drivers/gpu/drm/i915/display/intel_tc.c | 68 ++++++++++++++++++------
> drivers/gpu/drm/i915/display/intel_tc.h | 3 +-
> 3 files changed, 61 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c
> b/drivers/gpu/drm/i915/display/intel_ddi.c
> index 643832d55c282..5ce5e885694c8 100644
> --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> @@ -3591,7 +3591,7 @@ static void intel_ddi_sync_state(struct intel_encoder
> *encoder,
> enum phy phy = intel_port_to_phy(i915, encoder->port);
>
> if (intel_phy_is_tc(i915, phy))
> - intel_tc_port_sanitize(enc_to_dig_port(encoder));
> + intel_tc_port_sanitize_mode(enc_to_dig_port(encoder));
>
> if (crtc_state && intel_crtc_has_dp_encoder(crtc_state))
> intel_dp_sync_state(encoder, crtc_state); @@ -3789,11
> +3789,17 @@ static void intel_ddi_encoder_destroy(struct drm_encoder
> *encoder)
>
> static void intel_ddi_encoder_reset(struct drm_encoder *encoder) {
> + struct drm_i915_private *i915 = to_i915(encoder->dev);
> struct intel_dp *intel_dp = enc_to_intel_dp(to_intel_encoder(encoder));
> + struct intel_digital_port *dig_port =
> enc_to_dig_port(to_intel_encoder(encoder));
> + enum phy phy = intel_port_to_phy(i915, dig_port->base.port);
>
> intel_dp->reset_link_params = true;
>
> intel_pps_encoder_reset(intel_dp);
> +
> + if (intel_phy_is_tc(i915, phy))
> + intel_tc_port_init_mode(dig_port);
> }
>
> static const struct drm_encoder_funcs intel_ddi_funcs = { diff --git
> a/drivers/gpu/drm/i915/display/intel_tc.c
> b/drivers/gpu/drm/i915/display/intel_tc.c
> index e5af955b5600f..b0aa1edd83028 100644
> --- a/drivers/gpu/drm/i915/display/intel_tc.c
> +++ b/drivers/gpu/drm/i915/display/intel_tc.c
> @@ -687,18 +687,58 @@ static void
> intel_tc_port_link_init_refcount(struct intel_digital_port *dig_port,
> int refcount)
> {
> - struct drm_i915_private *i915 = to_i915(dig_port->base.base.dev);
> -
> - drm_WARN_ON(&i915->drm, dig_port->tc_link_refcount);
I was thinking whether we should drop this function or not? This is now reduced as one liner.
Anyway, patch looks ok to me.
Reviewed-by: Mika Kahola <mika.kahola@xxxxxxxxx>
> dig_port->tc_link_refcount = refcount; }
>
> -void intel_tc_port_sanitize(struct intel_digital_port *dig_port)
> +/**
> + * intel_tc_port_init_mode: Read out HW state and init the given port's
> +TypeC mode
> + * @dig_port: digital port
> + *
> + * Read out the HW state and initialize the TypeC mode of @dig_port.
> +The mode
> + * will be locked until intel_tc_port_sanitize_mode() is called.
> + */
> +void intel_tc_port_init_mode(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;
> enum intel_display_power_domain domain;
> +
> + mutex_lock(&dig_port->tc_lock);
> +
> + drm_WARN_ON(&i915->drm, dig_port->tc_mode !=
> TC_PORT_DISCONNECTED);
> + drm_WARN_ON(&i915->drm, dig_port->tc_lock_wakeref);
> + drm_WARN_ON(&i915->drm, dig_port->tc_link_refcount);
> +
> + tc_cold_wref = tc_cold_block(dig_port, &domain);
> +
> + dig_port->tc_mode = intel_tc_port_get_current_mode(dig_port);
> + /* Prevent changing dig_port->tc_mode until
> intel_tc_port_sanitize_mode() is called. */
> + intel_tc_port_link_init_refcount(dig_port, 1);
> + dig_port->tc_lock_wakeref = tc_cold_block(dig_port,
> +&dig_port->tc_lock_power_domain);
> +
> + tc_cold_unblock(dig_port, domain, tc_cold_wref);
> +
> + drm_dbg_kms(&i915->drm, "Port %s: init mode (%s)\n",
> + dig_port->tc_port_name,
> + tc_port_mode_name(dig_port->tc_mode));
> +
> + mutex_unlock(&dig_port->tc_lock);
> +}
> +
> +/**
> + * intel_tc_port_sanitize_mode: Sanitize the given port's TypeC mode
> + * @dig_port: digital port
> + *
> + * Sanitize @dig_port's TypeC mode wrt. the encoder's state right after
> +driver
> + * loading and system resume:
> + * If the encoder is enabled keep the TypeC mode/PHY connected state
> +locked until
> + * the encoder is disabled.
> + * If the encoder is disabled make sure the PHY is disconnected.
> + */
> +void intel_tc_port_sanitize_mode(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;
> int active_links = 0;
>
> mutex_lock(&dig_port->tc_lock);
> @@ -708,21 +748,14 @@ void intel_tc_port_sanitize(struct intel_digital_port
> *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);
> - drm_WARN_ON(&i915->drm, dig_port->tc_lock_wakeref);
> + drm_WARN_ON(&i915->drm, dig_port->tc_link_refcount != 1);
> + intel_tc_port_link_init_refcount(dig_port, active_links);
>
> - tc_cold_wref = tc_cold_block(dig_port, &domain);
> -
> - dig_port->tc_mode = intel_tc_port_get_current_mode(dig_port);
> if (active_links) {
> 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);
> -
> - dig_port->tc_lock_wakeref = tc_cold_block(dig_port,
> - &dig_port-
> >tc_lock_power_domain);
> } else {
> /*
> * TBT-alt is the default mode in any case the PHY ownership is
> not @@ -736,9 +769,10 @@ void intel_tc_port_sanitize(struct intel_digital_port
> *dig_port)
> dig_port->tc_port_name,
> tc_port_mode_name(dig_port->tc_mode));
> icl_tc_phy_disconnect(dig_port);
> - }
>
> - tc_cold_unblock(dig_port, domain, tc_cold_wref);
> + tc_cold_unblock(dig_port, dig_port->tc_lock_power_domain,
> + fetch_and_zero(&dig_port->tc_lock_wakeref));
> + }
>
> drm_dbg_kms(&i915->drm, "Port %s: sanitize mode (%s)\n",
> dig_port->tc_port_name,
> @@ -923,4 +957,6 @@ void intel_tc_port_init(struct intel_digital_port
> *dig_port, bool is_legacy)
> dig_port->tc_mode = TC_PORT_DISCONNECTED;
> dig_port->tc_link_refcount = 0;
> tc_port_load_fia_params(i915, dig_port);
> +
> + intel_tc_port_init_mode(dig_port);
> }
> diff --git a/drivers/gpu/drm/i915/display/intel_tc.h
> b/drivers/gpu/drm/i915/display/intel_tc.h
> index 6b47b29f551c9..d54082e2d5e8d 100644
> --- a/drivers/gpu/drm/i915/display/intel_tc.h
> +++ b/drivers/gpu/drm/i915/display/intel_tc.h
> @@ -24,7 +24,8 @@ int intel_tc_port_fia_max_lane_count(struct
> intel_digital_port *dig_port); void intel_tc_port_set_fia_lane_count(struct
> intel_digital_port *dig_port,
> int required_lanes);
>
> -void intel_tc_port_sanitize(struct intel_digital_port *dig_port);
> +void intel_tc_port_init_mode(struct intel_digital_port *dig_port); void
> +intel_tc_port_sanitize_mode(struct intel_digital_port *dig_port);
> void intel_tc_port_lock(struct intel_digital_port *dig_port); void
> intel_tc_port_unlock(struct intel_digital_port *dig_port); void
> intel_tc_port_flush_work(struct intel_digital_port *dig_port);
> --
> 2.37.1
