Re: [PATCH] drm/i915: Fix TypeC mode initialization during system resume

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



> -----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





[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux