[08/13] drm/i915/tc: Refactor TC-cold block/unblock helpers

Message ID	20210921002313.1132357-9-imre.deak@intel.com (mailing list archive)
State	New, archived
Headers	show Return-Path: <SRS0=FeUR=OL=lists.freedesktop.org=intel-gfx-bounces@kernel.org> DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org 6F8A2611ED From: Imre Deak <imre.deak@intel.com> To: intel-gfx@lists.freedesktop.org Cc: =?utf-8?q?Jos=C3=A9_Roberto_de_Souza?= <jose.souza@intel.com> Date: Tue, 21 Sep 2021 03:23:08 +0300 Message-Id: <20210921002313.1132357-9-imre.deak@intel.com> In-Reply-To: <20210921002313.1132357-1-imre.deak@intel.com> References: <20210921002313.1132357-1-imre.deak@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Subject: [Intel-gfx] [PATCH 08/13] drm/i915/tc: Refactor TC-cold block/unblock helpers Precedence: list Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" <intel-gfx-bounces@lists.freedesktop.org>
Series	drm/i915/tc: Fix TypeC connect/disconnect sequences \| expand [00/13] drm/i915/tc: Fix TypeC connect/disconnect sequences [01/13] drm/i915/tc: Fix TypeC port init/resume time sanitization [02/13] drm/i915/adlp/tc: Fix PHY connected check for Thunderbolt mode [03/13] drm/i915/tc: Remove waiting for PHY complete during releasing ownership [04/13] drm/i915/tc: Check for DP-alt, legacy sinks before taking PHY ownership [05/13] drm/i915/tc: Add/use helpers to retrieve TypeC port properties [06/13] drm/i915/tc: Don't keep legacy TypeC ports in connected state w/o a sink [07/13] drm/i915/tc: Add a mode for the TypeC PHY's disconnected state [08/13] drm/i915/tc: Refactor TC-cold block/unblock helpers [09/13] drm/i915/tc: Avoid using legacy AUX PW in TBT mode [10/13] drm/i915/icl/tc: Remove the ICL special casing during TC-cold blocking [11/13] drm/i915/tc: Fix TypeC PHY connect/disconnect logic on ADL-P [12/13] drm/i915/tc: Drop extra TC cold blocking from intel_tc_port_connected() [13/13] drm/i915/tc: Fix system hang on ADL-P during TypeC PHY disconnect

Imre Deak Sept. 21, 2021, 12:23 a.m. UTC

A follow-up change will select the TC-cold blocking power domain based
on the TypeC mode, prepare for that here.

Also bring intel_tc_cold_requires_aux_pw() earlier to its logical place
for readability.

No functional change.

Cc: José Roberto de Souza <jose.souza@intel.com>
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 .../drm/i915/display/intel_display_types.h    |  2 +
 drivers/gpu/drm/i915/display/intel_tc.c       | 63 +++++++++++--------
 2 files changed, 39 insertions(+), 26 deletions(-)

Souza, Jose Sept. 27, 2021, 9:56 p.m. UTC | #1

On Tue, 2021-09-21 at 03:23 +0300, Imre Deak wrote:
> A follow-up change will select the TC-cold blocking power domain based
> on the TypeC mode, prepare for that here.
> 
> Also bring intel_tc_cold_requires_aux_pw() earlier to its logical place
> for readability.
> 
> No functional change.
> 
> Cc: José Roberto de Souza <jose.souza@intel.com>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  .../drm/i915/display/intel_display_types.h    |  2 +
>  drivers/gpu/drm/i915/display/intel_tc.c       | 63 +++++++++++--------
>  2 files changed, 39 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
> index e9e806d90eec4..08a73ffded957 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> @@ -1663,8 +1663,10 @@ struct intel_digital_port {
>  	enum intel_display_power_domain ddi_io_power_domain;
>  	intel_wakeref_t ddi_io_wakeref;
>  	intel_wakeref_t aux_wakeref;
> +
>  	struct mutex tc_lock;	/* protects the TypeC port mode */
>  	intel_wakeref_t tc_lock_wakeref;
> +	enum intel_display_power_domain tc_lock_power_domain;
>  	int tc_link_refcount;
>  	bool tc_legacy_port:1;
>  	char tc_port_name[8];
> diff --git a/drivers/gpu/drm/i915/display/intel_tc.c b/drivers/gpu/drm/i915/display/intel_tc.c
> index 77b16a7c43466..24d2dc2e19a7d 100644
> --- a/drivers/gpu/drm/i915/display/intel_tc.c
> +++ b/drivers/gpu/drm/i915/display/intel_tc.c
> @@ -48,8 +48,16 @@ bool intel_tc_port_in_legacy_mode(struct intel_digital_port *dig_port)
>  	return intel_tc_port_in_mode(dig_port, TC_PORT_LEGACY);
>  }
>  
> +bool intel_tc_cold_requires_aux_pw(struct intel_digital_port *dig_port)
> +{
> +	struct drm_i915_private *i915 = to_i915(dig_port->base.base.dev);
> +
> +	return (DISPLAY_VER(i915) == 11 && dig_port->tc_legacy_port) ||
> +		IS_ALDERLAKE_P(i915);
> +}
> +
>  static enum intel_display_power_domain
> -tc_cold_get_power_domain(struct intel_digital_port *dig_port)
> +tc_cold_get_power_domain(struct intel_digital_port *dig_port, enum tc_port_mode mode)
>  {
>  	if (intel_tc_cold_requires_aux_pw(dig_port))
>  		return intel_legacy_aux_to_power_domain(dig_port->aux_ch);
> @@ -58,23 +66,30 @@ tc_cold_get_power_domain(struct intel_digital_port *dig_port)
>  }
>  
>  static intel_wakeref_t
> -tc_cold_block(struct intel_digital_port *dig_port)
> +tc_cold_block_in_mode(struct intel_digital_port *dig_port, enum tc_port_mode mode,
> +		      enum intel_display_power_domain *domain)
>  {
>  	struct drm_i915_private *i915 = to_i915(dig_port->base.base.dev);
> -	enum intel_display_power_domain domain;
>  
>  	if (DISPLAY_VER(i915) == 11 && !dig_port->tc_legacy_port)
>  		return 0;
>  
> -	domain = tc_cold_get_power_domain(dig_port);
> -	return intel_display_power_get(i915, domain);
> +	*domain = tc_cold_get_power_domain(dig_port, mode);

I see problems with this approach.
If there is a TC alt-mode is connected and current software state is TBT or disconnected it will not get the power domain to exit TC cold, what could
case invalid reads of registers.

From the next patch commit message: 'For the ADL-P TBT mode the spec doesn't require blocking TC-cold by
using the legacy AUX power domain'.

It is not required for TBT but when there is nothing connected, hardware is not in TBT mode so it can still get into TC cold.

> +
> +	return intel_display_power_get(i915, *domain);
> +}
> +
> +static intel_wakeref_t
> +tc_cold_block(struct intel_digital_port *dig_port, enum intel_display_power_domain *domain)
> +{
> +	return tc_cold_block_in_mode(dig_port, dig_port->tc_mode, domain);
>  }
>  
>  static void
> -tc_cold_unblock(struct intel_digital_port *dig_port, intel_wakeref_t wakeref)
> +tc_cold_unblock(struct intel_digital_port *dig_port, enum intel_display_power_domain domain,
> +		intel_wakeref_t wakeref)
>  {
>  	struct drm_i915_private *i915 = to_i915(dig_port->base.base.dev);
> -	enum intel_display_power_domain domain;
>  
>  	/*
>  	 * wakeref == -1, means some error happened saving save_depot_stack but
> @@ -84,8 +99,7 @@ tc_cold_unblock(struct intel_digital_port *dig_port, intel_wakeref_t wakeref)
>  	if (wakeref == 0)
>  		return;
>  
> -	domain = tc_cold_get_power_domain(dig_port);
> -	intel_display_power_put_async(i915, domain, wakeref);
> +	intel_display_power_put(i915, domain, wakeref);
>  }
>  
>  static void
> @@ -98,7 +112,8 @@ assert_tc_cold_blocked(struct intel_digital_port *dig_port)
>  		return;
>  
>  	enabled = intel_display_power_is_enabled(i915,
> -						 tc_cold_get_power_domain(dig_port));
> +						 tc_cold_get_power_domain(dig_port,
> +									  dig_port->tc_mode));
>  	drm_WARN_ON(&i915->drm, !enabled);
>  }
>  
> @@ -634,7 +649,8 @@ void intel_tc_port_sanitize(struct intel_digital_port *dig_port)
>  
>  	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);
> +		enum intel_display_power_domain domain;
> +		intel_wakeref_t tc_cold_wref = tc_cold_block(dig_port, &domain);
>  
>  		dig_port->tc_mode = intel_tc_port_get_current_mode(dig_port);
>  
> @@ -644,7 +660,7 @@ void intel_tc_port_sanitize(struct intel_digital_port *dig_port)
>  				    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);
> +		tc_cold_unblock(dig_port, domain, tc_cold_wref);
>  	}
>  
>  	drm_dbg_kms(&i915->drm, "Port %s: sanitize mode (%s)\n",
> @@ -673,15 +689,16 @@ bool intel_tc_port_connected(struct intel_encoder *encoder)
>  {
>  	struct intel_digital_port *dig_port = enc_to_dig_port(encoder);
>  	bool is_connected;
> +	enum intel_display_power_domain domain;
>  	intel_wakeref_t tc_cold_wref;
>  
>  	intel_tc_port_lock(dig_port);
> -	tc_cold_wref = tc_cold_block(dig_port);
> +	tc_cold_wref = tc_cold_block(dig_port, &domain);
>  
>  	is_connected = tc_port_live_status_mask(dig_port) &
>  		       BIT(dig_port->tc_mode);
>  
> -	tc_cold_unblock(dig_port, tc_cold_wref);
> +	tc_cold_unblock(dig_port, domain, tc_cold_wref);
>  	intel_tc_port_unlock(dig_port);
>  
>  	return is_connected;
> @@ -698,15 +715,16 @@ static void __intel_tc_port_lock(struct intel_digital_port *dig_port,
>  	mutex_lock(&dig_port->tc_lock);
>  
>  	if (!dig_port->tc_link_refcount) {
> +		enum intel_display_power_domain domain;
>  		intel_wakeref_t tc_cold_wref;
>  
> -		tc_cold_wref = tc_cold_block(dig_port);
> +		tc_cold_wref = tc_cold_block(dig_port, &domain);
>  
>  		if (force_disconnect || intel_tc_port_needs_reset(dig_port))
>  			intel_tc_port_reset_mode(dig_port, required_lanes,
>  						 force_disconnect);
>  
> -		tc_cold_unblock(dig_port, tc_cold_wref);
> +		tc_cold_unblock(dig_port, domain, tc_cold_wref);
>  	}
>  
>  	drm_WARN_ON(&i915->drm, dig_port->tc_mode == TC_PORT_DISCONNECTED);
> @@ -775,6 +793,7 @@ void intel_tc_port_put_link(struct intel_digital_port *dig_port)
>  static bool
>  tc_has_modular_fia(struct drm_i915_private *i915, struct intel_digital_port *dig_port)
>  {
> +	enum intel_display_power_domain domain;
>  	intel_wakeref_t wakeref;
>  	u32 val;
>  
> @@ -782,9 +801,9 @@ tc_has_modular_fia(struct drm_i915_private *i915, struct intel_digital_port *dig
>  		return false;
>  
>  	mutex_lock(&dig_port->tc_lock);
> -	wakeref = tc_cold_block(dig_port);
> +	wakeref = tc_cold_block(dig_port, &domain);
>  	val = intel_uncore_read(&i915->uncore, PORT_TX_DFLEXDPSP(FIA1));
> -	tc_cold_unblock(dig_port, wakeref);
> +	tc_cold_unblock(dig_port, domain, wakeref);
>  	mutex_unlock(&dig_port->tc_lock);
>  
>  	drm_WARN_ON(&i915->drm, val == 0xffffffff);
> @@ -829,11 +848,3 @@ void intel_tc_port_init(struct intel_digital_port *dig_port, bool is_legacy)
>  	dig_port->tc_link_refcount = 0;
>  	tc_port_load_fia_params(i915, dig_port);
>  }
> -
> -bool intel_tc_cold_requires_aux_pw(struct intel_digital_port *dig_port)
> -{
> -	struct drm_i915_private *i915 = to_i915(dig_port->base.base.dev);
> -
> -	return (DISPLAY_VER(i915) == 11 && dig_port->tc_legacy_port) ||
> -		IS_ALDERLAKE_P(i915);
> -}

Imre Deak Sept. 27, 2021, 10:13 p.m. UTC | #2

On Tue, Sep 28, 2021 at 12:56:24AM +0300, Souza, Jose wrote:
> On Tue, 2021-09-21 at 03:23 +0300, Imre Deak wrote:
> > A follow-up change will select the TC-cold blocking power domain based
> > on the TypeC mode, prepare for that here.
> > 
> > Also bring intel_tc_cold_requires_aux_pw() earlier to its logical place
> > for readability.
> > 
> > No functional change.
> > 
> > Cc: José Roberto de Souza <jose.souza@intel.com>
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  .../drm/i915/display/intel_display_types.h    |  2 +
> >  drivers/gpu/drm/i915/display/intel_tc.c       | 63 +++++++++++--------
> >  2 files changed, 39 insertions(+), 26 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
> > index e9e806d90eec4..08a73ffded957 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> > @@ -1663,8 +1663,10 @@ struct intel_digital_port {
> >  	enum intel_display_power_domain ddi_io_power_domain;
> >  	intel_wakeref_t ddi_io_wakeref;
> >  	intel_wakeref_t aux_wakeref;
> > +
> >  	struct mutex tc_lock;	/* protects the TypeC port mode */
> >  	intel_wakeref_t tc_lock_wakeref;
> > +	enum intel_display_power_domain tc_lock_power_domain;
> >  	int tc_link_refcount;
> >  	bool tc_legacy_port:1;
> >  	char tc_port_name[8];
> > diff --git a/drivers/gpu/drm/i915/display/intel_tc.c b/drivers/gpu/drm/i915/display/intel_tc.c
> > index 77b16a7c43466..24d2dc2e19a7d 100644
> > --- a/drivers/gpu/drm/i915/display/intel_tc.c
> > +++ b/drivers/gpu/drm/i915/display/intel_tc.c
> > @@ -48,8 +48,16 @@ bool intel_tc_port_in_legacy_mode(struct intel_digital_port *dig_port)
> >  	return intel_tc_port_in_mode(dig_port, TC_PORT_LEGACY);
> >  }
> >  
> > +bool intel_tc_cold_requires_aux_pw(struct intel_digital_port *dig_port)
> > +{
> > +	struct drm_i915_private *i915 = to_i915(dig_port->base.base.dev);
> > +
> > +	return (DISPLAY_VER(i915) == 11 && dig_port->tc_legacy_port) ||
> > +		IS_ALDERLAKE_P(i915);
> > +}
> > +
> >  static enum intel_display_power_domain
> > -tc_cold_get_power_domain(struct intel_digital_port *dig_port)
> > +tc_cold_get_power_domain(struct intel_digital_port *dig_port, enum tc_port_mode mode)
> >  {
> >  	if (intel_tc_cold_requires_aux_pw(dig_port))
> >  		return intel_legacy_aux_to_power_domain(dig_port->aux_ch);
> > @@ -58,23 +66,30 @@ tc_cold_get_power_domain(struct intel_digital_port *dig_port)
> >  }
> >  
> >  static intel_wakeref_t
> > -tc_cold_block(struct intel_digital_port *dig_port)
> > +tc_cold_block_in_mode(struct intel_digital_port *dig_port, enum tc_port_mode mode,
> > +		      enum intel_display_power_domain *domain)
> >  {
> >  	struct drm_i915_private *i915 = to_i915(dig_port->base.base.dev);
> > -	enum intel_display_power_domain domain;
> >  
> >  	if (DISPLAY_VER(i915) == 11 && !dig_port->tc_legacy_port)
> >  		return 0;
> >  
> > -	domain = tc_cold_get_power_domain(dig_port);
> > -	return intel_display_power_get(i915, domain);
> > +	*domain = tc_cold_get_power_domain(dig_port, mode);
> 
> I see problems with this approach.
>
> If there is a TC alt-mode is connected and current software state is
> TBT or disconnected it will not get the power domain to exit TC cold,
> what could case invalid reads of registers.

If the mode needs to be changed, like in the above cases, the power
domain required to exit TC cold will be taken.

> From the next patch commit message: 'For the ADL-P TBT mode the spec
> doesn't require blocking TC-cold by using the legacy AUX power
> domain'.
>
> It is not required for TBT but when there is nothing connected,
> hardware is not in TBT mode so it can still get into TC cold.

If there is nothing connected the required power domain will be taken to
exit TC cold.

> > +
> > +	return intel_display_power_get(i915, *domain);
> > +}
> > +
> > +static intel_wakeref_t
> > +tc_cold_block(struct intel_digital_port *dig_port, enum intel_display_power_domain *domain)
> > +{
> > +	return tc_cold_block_in_mode(dig_port, dig_port->tc_mode, domain);
> >  }
> >  
> >  static void
> > -tc_cold_unblock(struct intel_digital_port *dig_port, intel_wakeref_t wakeref)
> > +tc_cold_unblock(struct intel_digital_port *dig_port, enum intel_display_power_domain domain,
> > +		intel_wakeref_t wakeref)
> >  {
> >  	struct drm_i915_private *i915 = to_i915(dig_port->base.base.dev);
> > -	enum intel_display_power_domain domain;
> >  
> >  	/*
> >  	 * wakeref == -1, means some error happened saving save_depot_stack but
> > @@ -84,8 +99,7 @@ tc_cold_unblock(struct intel_digital_port *dig_port, intel_wakeref_t wakeref)
> >  	if (wakeref == 0)
> >  		return;
> >  
> > -	domain = tc_cold_get_power_domain(dig_port);
> > -	intel_display_power_put_async(i915, domain, wakeref);
> > +	intel_display_power_put(i915, domain, wakeref);
> >  }
> >  
> >  static void
> > @@ -98,7 +112,8 @@ assert_tc_cold_blocked(struct intel_digital_port *dig_port)
> >  		return;
> >  
> >  	enabled = intel_display_power_is_enabled(i915,
> > -						 tc_cold_get_power_domain(dig_port));
> > +						 tc_cold_get_power_domain(dig_port,
> > +									  dig_port->tc_mode));
> >  	drm_WARN_ON(&i915->drm, !enabled);
> >  }
> >  
> > @@ -634,7 +649,8 @@ void intel_tc_port_sanitize(struct intel_digital_port *dig_port)
> >  
> >  	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);
> > +		enum intel_display_power_domain domain;
> > +		intel_wakeref_t tc_cold_wref = tc_cold_block(dig_port, &domain);
> >  
> >  		dig_port->tc_mode = intel_tc_port_get_current_mode(dig_port);
> >  
> > @@ -644,7 +660,7 @@ void intel_tc_port_sanitize(struct intel_digital_port *dig_port)
> >  				    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);
> > +		tc_cold_unblock(dig_port, domain, tc_cold_wref);
> >  	}
> >  
> >  	drm_dbg_kms(&i915->drm, "Port %s: sanitize mode (%s)\n",
> > @@ -673,15 +689,16 @@ bool intel_tc_port_connected(struct intel_encoder *encoder)
> >  {
> >  	struct intel_digital_port *dig_port = enc_to_dig_port(encoder);
> >  	bool is_connected;
> > +	enum intel_display_power_domain domain;
> >  	intel_wakeref_t tc_cold_wref;
> >  
> >  	intel_tc_port_lock(dig_port);
> > -	tc_cold_wref = tc_cold_block(dig_port);
> > +	tc_cold_wref = tc_cold_block(dig_port, &domain);
> >  
> >  	is_connected = tc_port_live_status_mask(dig_port) &
> >  		       BIT(dig_port->tc_mode);
> >  
> > -	tc_cold_unblock(dig_port, tc_cold_wref);
> > +	tc_cold_unblock(dig_port, domain, tc_cold_wref);
> >  	intel_tc_port_unlock(dig_port);
> >  
> >  	return is_connected;
> > @@ -698,15 +715,16 @@ static void __intel_tc_port_lock(struct intel_digital_port *dig_port,
> >  	mutex_lock(&dig_port->tc_lock);
> >  
> >  	if (!dig_port->tc_link_refcount) {
> > +		enum intel_display_power_domain domain;
> >  		intel_wakeref_t tc_cold_wref;
> >  
> > -		tc_cold_wref = tc_cold_block(dig_port);
> > +		tc_cold_wref = tc_cold_block(dig_port, &domain);
> >  
> >  		if (force_disconnect || intel_tc_port_needs_reset(dig_port))
> >  			intel_tc_port_reset_mode(dig_port, required_lanes,
> >  						 force_disconnect);
> >  
> > -		tc_cold_unblock(dig_port, tc_cold_wref);
> > +		tc_cold_unblock(dig_port, domain, tc_cold_wref);
> >  	}
> >  
> >  	drm_WARN_ON(&i915->drm, dig_port->tc_mode == TC_PORT_DISCONNECTED);
> > @@ -775,6 +793,7 @@ void intel_tc_port_put_link(struct intel_digital_port *dig_port)
> >  static bool
> >  tc_has_modular_fia(struct drm_i915_private *i915, struct intel_digital_port *dig_port)
> >  {
> > +	enum intel_display_power_domain domain;
> >  	intel_wakeref_t wakeref;
> >  	u32 val;
> >  
> > @@ -782,9 +801,9 @@ tc_has_modular_fia(struct drm_i915_private *i915, struct intel_digital_port *dig
> >  		return false;
> >  
> >  	mutex_lock(&dig_port->tc_lock);
> > -	wakeref = tc_cold_block(dig_port);
> > +	wakeref = tc_cold_block(dig_port, &domain);
> >  	val = intel_uncore_read(&i915->uncore, PORT_TX_DFLEXDPSP(FIA1));
> > -	tc_cold_unblock(dig_port, wakeref);
> > +	tc_cold_unblock(dig_port, domain, wakeref);
> >  	mutex_unlock(&dig_port->tc_lock);
> >  
> >  	drm_WARN_ON(&i915->drm, val == 0xffffffff);
> > @@ -829,11 +848,3 @@ void intel_tc_port_init(struct intel_digital_port *dig_port, bool is_legacy)
> >  	dig_port->tc_link_refcount = 0;
> >  	tc_port_load_fia_params(i915, dig_port);
> >  }
> > -
> > -bool intel_tc_cold_requires_aux_pw(struct intel_digital_port *dig_port)
> > -{
> > -	struct drm_i915_private *i915 = to_i915(dig_port->base.base.dev);
> > -
> > -	return (DISPLAY_VER(i915) == 11 && dig_port->tc_legacy_port) ||
> > -		IS_ALDERLAKE_P(i915);
> > -}
>

Souza, Jose Sept. 27, 2021, 10:21 p.m. UTC | #3

On Tue, 2021-09-28 at 01:13 +0300, Imre Deak wrote:
> On Tue, Sep 28, 2021 at 12:56:24AM +0300, Souza, Jose wrote:
> > On Tue, 2021-09-21 at 03:23 +0300, Imre Deak wrote:
> > > A follow-up change will select the TC-cold blocking power domain based
> > > on the TypeC mode, prepare for that here.
> > > 
> > > Also bring intel_tc_cold_requires_aux_pw() earlier to its logical place
> > > for readability.
> > > 
> > > No functional change.
> > > 
> > > Cc: José Roberto de Souza <jose.souza@intel.com>
> > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > ---
> > >  .../drm/i915/display/intel_display_types.h    |  2 +
> > >  drivers/gpu/drm/i915/display/intel_tc.c       | 63 +++++++++++--------
> > >  2 files changed, 39 insertions(+), 26 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
> > > index e9e806d90eec4..08a73ffded957 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> > > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> > > @@ -1663,8 +1663,10 @@ struct intel_digital_port {
> > >  	enum intel_display_power_domain ddi_io_power_domain;
> > >  	intel_wakeref_t ddi_io_wakeref;
> > >  	intel_wakeref_t aux_wakeref;
> > > +
> > >  	struct mutex tc_lock;	/* protects the TypeC port mode */
> > >  	intel_wakeref_t tc_lock_wakeref;
> > > +	enum intel_display_power_domain tc_lock_power_domain;
> > >  	int tc_link_refcount;
> > >  	bool tc_legacy_port:1;
> > >  	char tc_port_name[8];
> > > diff --git a/drivers/gpu/drm/i915/display/intel_tc.c b/drivers/gpu/drm/i915/display/intel_tc.c
> > > index 77b16a7c43466..24d2dc2e19a7d 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_tc.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_tc.c
> > > @@ -48,8 +48,16 @@ bool intel_tc_port_in_legacy_mode(struct intel_digital_port *dig_port)
> > >  	return intel_tc_port_in_mode(dig_port, TC_PORT_LEGACY);
> > >  }
> > >  
> > > +bool intel_tc_cold_requires_aux_pw(struct intel_digital_port *dig_port)
> > > +{
> > > +	struct drm_i915_private *i915 = to_i915(dig_port->base.base.dev);
> > > +
> > > +	return (DISPLAY_VER(i915) == 11 && dig_port->tc_legacy_port) ||
> > > +		IS_ALDERLAKE_P(i915);
> > > +}
> > > +
> > >  static enum intel_display_power_domain
> > > -tc_cold_get_power_domain(struct intel_digital_port *dig_port)
> > > +tc_cold_get_power_domain(struct intel_digital_port *dig_port, enum tc_port_mode mode)
> > >  {
> > >  	if (intel_tc_cold_requires_aux_pw(dig_port))
> > >  		return intel_legacy_aux_to_power_domain(dig_port->aux_ch);
> > > @@ -58,23 +66,30 @@ tc_cold_get_power_domain(struct intel_digital_port *dig_port)
> > >  }
> > >  
> > >  static intel_wakeref_t
> > > -tc_cold_block(struct intel_digital_port *dig_port)
> > > +tc_cold_block_in_mode(struct intel_digital_port *dig_port, enum tc_port_mode mode,
> > > +		      enum intel_display_power_domain *domain)
> > >  {
> > >  	struct drm_i915_private *i915 = to_i915(dig_port->base.base.dev);
> > > -	enum intel_display_power_domain domain;
> > >  
> > >  	if (DISPLAY_VER(i915) == 11 && !dig_port->tc_legacy_port)
> > >  		return 0;
> > >  
> > > -	domain = tc_cold_get_power_domain(dig_port);
> > > -	return intel_display_power_get(i915, domain);
> > > +	*domain = tc_cold_get_power_domain(dig_port, mode);
> > 
> > I see problems with this approach.
> > 
> > If there is a TC alt-mode is connected and current software state is
> > TBT or disconnected it will not get the power domain to exit TC cold,
> > what could case invalid reads of registers.
> 
> If the mode needs to be changed, like in the above cases, the power
> domain required to exit TC cold will be taken.

How?
I see at least this 2 problematic cases.

intel_tc_port_sanitize()
	tc_cold_block(mode=TBT)

	dig_port->tc_mode = intel_tc_port_get_current_mode(dig_port);
	...

	tc_cold_unblock(dig_port, domain, tc_cold_wref);

Other case

intel_tc_port_connected()

	tc_cold_block(mode=TBT)

	is_connected = tc_port_live_status_mask(dig_port) &
		       BIT(dig_port->tc_mode);

	tc_cold_unblock(dig_port, domain, tc_cold_wref);


> 
> > From the next patch commit message: 'For the ADL-P TBT mode the spec
> > doesn't require blocking TC-cold by using the legacy AUX power
> > domain'.
> > 
> > It is not required for TBT but when there is nothing connected,
> > hardware is not in TBT mode so it can still get into TC cold.
> 
> If there is nothing connected the required power domain will be taken to
> exit TC cold.
> 
> > > +
> > > +	return intel_display_power_get(i915, *domain);
> > > +}
> > > +
> > > +static intel_wakeref_t
> > > +tc_cold_block(struct intel_digital_port *dig_port, enum intel_display_power_domain *domain)
> > > +{
> > > +	return tc_cold_block_in_mode(dig_port, dig_port->tc_mode, domain);
> > >  }
> > >  
> > >  static void
> > > -tc_cold_unblock(struct intel_digital_port *dig_port, intel_wakeref_t wakeref)
> > > +tc_cold_unblock(struct intel_digital_port *dig_port, enum intel_display_power_domain domain,
> > > +		intel_wakeref_t wakeref)
> > >  {
> > >  	struct drm_i915_private *i915 = to_i915(dig_port->base.base.dev);
> > > -	enum intel_display_power_domain domain;
> > >  
> > >  	/*
> > >  	 * wakeref == -1, means some error happened saving save_depot_stack but
> > > @@ -84,8 +99,7 @@ tc_cold_unblock(struct intel_digital_port *dig_port, intel_wakeref_t wakeref)
> > >  	if (wakeref == 0)
> > >  		return;
> > >  
> > > -	domain = tc_cold_get_power_domain(dig_port);
> > > -	intel_display_power_put_async(i915, domain, wakeref);
> > > +	intel_display_power_put(i915, domain, wakeref);
> > >  }
> > >  
> > >  static void
> > > @@ -98,7 +112,8 @@ assert_tc_cold_blocked(struct intel_digital_port *dig_port)
> > >  		return;
> > >  
> > >  	enabled = intel_display_power_is_enabled(i915,
> > > -						 tc_cold_get_power_domain(dig_port));
> > > +						 tc_cold_get_power_domain(dig_port,
> > > +									  dig_port->tc_mode));
> > >  	drm_WARN_ON(&i915->drm, !enabled);
> > >  }
> > >  
> > > @@ -634,7 +649,8 @@ void intel_tc_port_sanitize(struct intel_digital_port *dig_port)
> > >  
> > >  	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);
> > > +		enum intel_display_power_domain domain;
> > > +		intel_wakeref_t tc_cold_wref = tc_cold_block(dig_port, &domain);
> > >  
> > >  		dig_port->tc_mode = intel_tc_port_get_current_mode(dig_port);
> > >  
> > > @@ -644,7 +660,7 @@ void intel_tc_port_sanitize(struct intel_digital_port *dig_port)
> > >  				    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);
> > > +		tc_cold_unblock(dig_port, domain, tc_cold_wref);
> > >  	}
> > >  
> > >  	drm_dbg_kms(&i915->drm, "Port %s: sanitize mode (%s)\n",
> > > @@ -673,15 +689,16 @@ bool intel_tc_port_connected(struct intel_encoder *encoder)
> > >  {
> > >  	struct intel_digital_port *dig_port = enc_to_dig_port(encoder);
> > >  	bool is_connected;
> > > +	enum intel_display_power_domain domain;
> > >  	intel_wakeref_t tc_cold_wref;
> > >  
> > >  	intel_tc_port_lock(dig_port);
> > > -	tc_cold_wref = tc_cold_block(dig_port);
> > > +	tc_cold_wref = tc_cold_block(dig_port, &domain);
> > >  
> > >  	is_connected = tc_port_live_status_mask(dig_port) &
> > >  		       BIT(dig_port->tc_mode);
> > >  
> > > -	tc_cold_unblock(dig_port, tc_cold_wref);
> > > +	tc_cold_unblock(dig_port, domain, tc_cold_wref);
> > >  	intel_tc_port_unlock(dig_port);
> > >  
> > >  	return is_connected;
> > > @@ -698,15 +715,16 @@ static void __intel_tc_port_lock(struct intel_digital_port *dig_port,
> > >  	mutex_lock(&dig_port->tc_lock);
> > >  
> > >  	if (!dig_port->tc_link_refcount) {
> > > +		enum intel_display_power_domain domain;
> > >  		intel_wakeref_t tc_cold_wref;
> > >  
> > > -		tc_cold_wref = tc_cold_block(dig_port);
> > > +		tc_cold_wref = tc_cold_block(dig_port, &domain);
> > >  
> > >  		if (force_disconnect || intel_tc_port_needs_reset(dig_port))
> > >  			intel_tc_port_reset_mode(dig_port, required_lanes,
> > >  						 force_disconnect);
> > >  
> > > -		tc_cold_unblock(dig_port, tc_cold_wref);
> > > +		tc_cold_unblock(dig_port, domain, tc_cold_wref);
> > >  	}
> > >  
> > >  	drm_WARN_ON(&i915->drm, dig_port->tc_mode == TC_PORT_DISCONNECTED);
> > > @@ -775,6 +793,7 @@ void intel_tc_port_put_link(struct intel_digital_port *dig_port)
> > >  static bool
> > >  tc_has_modular_fia(struct drm_i915_private *i915, struct intel_digital_port *dig_port)
> > >  {
> > > +	enum intel_display_power_domain domain;
> > >  	intel_wakeref_t wakeref;
> > >  	u32 val;
> > >  
> > > @@ -782,9 +801,9 @@ tc_has_modular_fia(struct drm_i915_private *i915, struct intel_digital_port *dig
> > >  		return false;
> > >  
> > >  	mutex_lock(&dig_port->tc_lock);
> > > -	wakeref = tc_cold_block(dig_port);
> > > +	wakeref = tc_cold_block(dig_port, &domain);
> > >  	val = intel_uncore_read(&i915->uncore, PORT_TX_DFLEXDPSP(FIA1));
> > > -	tc_cold_unblock(dig_port, wakeref);
> > > +	tc_cold_unblock(dig_port, domain, wakeref);
> > >  	mutex_unlock(&dig_port->tc_lock);
> > >  
> > >  	drm_WARN_ON(&i915->drm, val == 0xffffffff);
> > > @@ -829,11 +848,3 @@ void intel_tc_port_init(struct intel_digital_port *dig_port, bool is_legacy)
> > >  	dig_port->tc_link_refcount = 0;
> > >  	tc_port_load_fia_params(i915, dig_port);
> > >  }
> > > -
> > > -bool intel_tc_cold_requires_aux_pw(struct intel_digital_port *dig_port)
> > > -{
> > > -	struct drm_i915_private *i915 = to_i915(dig_port->base.base.dev);
> > > -
> > > -	return (DISPLAY_VER(i915) == 11 && dig_port->tc_legacy_port) ||
> > > -		IS_ALDERLAKE_P(i915);
> > > -}
> >

Imre Deak Sept. 27, 2021, 10:28 p.m. UTC | #4

On Tue, Sep 28, 2021 at 01:21:21AM +0300, Souza, Jose wrote:
> On Tue, 2021-09-28 at 01:13 +0300, Imre Deak wrote:
> > On Tue, Sep 28, 2021 at 12:56:24AM +0300, Souza, Jose wrote:
> > > On Tue, 2021-09-21 at 03:23 +0300, Imre Deak wrote:
> > > > A follow-up change will select the TC-cold blocking power domain based
> > > > on the TypeC mode, prepare for that here.
> > > > 
> > > > Also bring intel_tc_cold_requires_aux_pw() earlier to its logical place
> > > > for readability.
> > > > 
> > > > No functional change.
> > > > 
> > > > Cc: José Roberto de Souza <jose.souza@intel.com>
> > > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > > ---
> > > >  .../drm/i915/display/intel_display_types.h    |  2 +
> > > >  drivers/gpu/drm/i915/display/intel_tc.c       | 63 +++++++++++--------
> > > >  2 files changed, 39 insertions(+), 26 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
> > > > index e9e806d90eec4..08a73ffded957 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> > > > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> > > > @@ -1663,8 +1663,10 @@ struct intel_digital_port {
> > > >  	enum intel_display_power_domain ddi_io_power_domain;
> > > >  	intel_wakeref_t ddi_io_wakeref;
> > > >  	intel_wakeref_t aux_wakeref;
> > > > +
> > > >  	struct mutex tc_lock;	/* protects the TypeC port mode */
> > > >  	intel_wakeref_t tc_lock_wakeref;
> > > > +	enum intel_display_power_domain tc_lock_power_domain;
> > > >  	int tc_link_refcount;
> > > >  	bool tc_legacy_port:1;
> > > >  	char tc_port_name[8];
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_tc.c b/drivers/gpu/drm/i915/display/intel_tc.c
> > > > index 77b16a7c43466..24d2dc2e19a7d 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_tc.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_tc.c
> > > > @@ -48,8 +48,16 @@ bool intel_tc_port_in_legacy_mode(struct intel_digital_port *dig_port)
> > > >  	return intel_tc_port_in_mode(dig_port, TC_PORT_LEGACY);
> > > >  }
> > > >  
> > > > +bool intel_tc_cold_requires_aux_pw(struct intel_digital_port *dig_port)
> > > > +{
> > > > +	struct drm_i915_private *i915 = to_i915(dig_port->base.base.dev);
> > > > +
> > > > +	return (DISPLAY_VER(i915) == 11 && dig_port->tc_legacy_port) ||
> > > > +		IS_ALDERLAKE_P(i915);
> > > > +}
> > > > +
> > > >  static enum intel_display_power_domain
> > > > -tc_cold_get_power_domain(struct intel_digital_port *dig_port)
> > > > +tc_cold_get_power_domain(struct intel_digital_port *dig_port, enum tc_port_mode mode)
> > > >  {
> > > >  	if (intel_tc_cold_requires_aux_pw(dig_port))
> > > >  		return intel_legacy_aux_to_power_domain(dig_port->aux_ch);
> > > > @@ -58,23 +66,30 @@ tc_cold_get_power_domain(struct intel_digital_port *dig_port)
> > > >  }
> > > >  
> > > >  static intel_wakeref_t
> > > > -tc_cold_block(struct intel_digital_port *dig_port)
> > > > +tc_cold_block_in_mode(struct intel_digital_port *dig_port, enum tc_port_mode mode,
> > > > +		      enum intel_display_power_domain *domain)
> > > >  {
> > > >  	struct drm_i915_private *i915 = to_i915(dig_port->base.base.dev);
> > > > -	enum intel_display_power_domain domain;
> > > >  
> > > >  	if (DISPLAY_VER(i915) == 11 && !dig_port->tc_legacy_port)
> > > >  		return 0;
> > > >  
> > > > -	domain = tc_cold_get_power_domain(dig_port);
> > > > -	return intel_display_power_get(i915, domain);
> > > > +	*domain = tc_cold_get_power_domain(dig_port, mode);
> > > 
> > > I see problems with this approach.
> > > 
> > > If there is a TC alt-mode is connected and current software state is
> > > TBT or disconnected it will not get the power domain to exit TC cold,
> > > what could case invalid reads of registers.
> > 
> > If the mode needs to be changed, like in the above cases, the power
> > domain required to exit TC cold will be taken.
> 
> How?
> I see at least this 2 problematic cases.
> 
> intel_tc_port_sanitize()
> 	tc_cold_block(mode=TBT)

It's in disconnected mode at this point.

> 
> 	dig_port->tc_mode = intel_tc_port_get_current_mode(dig_port);
> 	...
> 
> 	tc_cold_unblock(dig_port, domain, tc_cold_wref);
> 
> Other case
> 
> intel_tc_port_connected()
> 
> 	tc_cold_block(mode=TBT)

Reading PORT_TX_DFLEXDPSP while in TC-cold returns 0xffffffff, and it's
handled when reading it (which is also required by the spec).

> 	is_connected = tc_port_live_status_mask(dig_port) &
> 		       BIT(dig_port->tc_mode);
> 
> 	tc_cold_unblock(dig_port, domain, tc_cold_wref);
> 
> 
> > 
> > > From the next patch commit message: 'For the ADL-P TBT mode the spec
> > > doesn't require blocking TC-cold by using the legacy AUX power
> > > domain'.
> > > 
> > > It is not required for TBT but when there is nothing connected,
> > > hardware is not in TBT mode so it can still get into TC cold.
> > 
> > If there is nothing connected the required power domain will be taken to
> > exit TC cold.
> > 
> > > > +
> > > > +	return intel_display_power_get(i915, *domain);
> > > > +}
> > > > +
> > > > +static intel_wakeref_t
> > > > +tc_cold_block(struct intel_digital_port *dig_port, enum intel_display_power_domain *domain)
> > > > +{
> > > > +	return tc_cold_block_in_mode(dig_port, dig_port->tc_mode, domain);
> > > >  }
> > > >  
> > > >  static void
> > > > -tc_cold_unblock(struct intel_digital_port *dig_port, intel_wakeref_t wakeref)
> > > > +tc_cold_unblock(struct intel_digital_port *dig_port, enum intel_display_power_domain domain,
> > > > +		intel_wakeref_t wakeref)
> > > >  {
> > > >  	struct drm_i915_private *i915 = to_i915(dig_port->base.base.dev);
> > > > -	enum intel_display_power_domain domain;
> > > >  
> > > >  	/*
> > > >  	 * wakeref == -1, means some error happened saving save_depot_stack but
> > > > @@ -84,8 +99,7 @@ tc_cold_unblock(struct intel_digital_port *dig_port, intel_wakeref_t wakeref)
> > > >  	if (wakeref == 0)
> > > >  		return;
> > > >  
> > > > -	domain = tc_cold_get_power_domain(dig_port);
> > > > -	intel_display_power_put_async(i915, domain, wakeref);
> > > > +	intel_display_power_put(i915, domain, wakeref);
> > > >  }
> > > >  
> > > >  static void
> > > > @@ -98,7 +112,8 @@ assert_tc_cold_blocked(struct intel_digital_port *dig_port)
> > > >  		return;
> > > >  
> > > >  	enabled = intel_display_power_is_enabled(i915,
> > > > -						 tc_cold_get_power_domain(dig_port));
> > > > +						 tc_cold_get_power_domain(dig_port,
> > > > +									  dig_port->tc_mode));
> > > >  	drm_WARN_ON(&i915->drm, !enabled);
> > > >  }
> > > >  
> > > > @@ -634,7 +649,8 @@ void intel_tc_port_sanitize(struct intel_digital_port *dig_port)
> > > >  
> > > >  	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);
> > > > +		enum intel_display_power_domain domain;
> > > > +		intel_wakeref_t tc_cold_wref = tc_cold_block(dig_port, &domain);
> > > >  
> > > >  		dig_port->tc_mode = intel_tc_port_get_current_mode(dig_port);
> > > >  
> > > > @@ -644,7 +660,7 @@ void intel_tc_port_sanitize(struct intel_digital_port *dig_port)
> > > >  				    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);
> > > > +		tc_cold_unblock(dig_port, domain, tc_cold_wref);
> > > >  	}
> > > >  
> > > >  	drm_dbg_kms(&i915->drm, "Port %s: sanitize mode (%s)\n",
> > > > @@ -673,15 +689,16 @@ bool intel_tc_port_connected(struct intel_encoder *encoder)
> > > >  {
> > > >  	struct intel_digital_port *dig_port = enc_to_dig_port(encoder);
> > > >  	bool is_connected;
> > > > +	enum intel_display_power_domain domain;
> > > >  	intel_wakeref_t tc_cold_wref;
> > > >  
> > > >  	intel_tc_port_lock(dig_port);
> > > > -	tc_cold_wref = tc_cold_block(dig_port);
> > > > +	tc_cold_wref = tc_cold_block(dig_port, &domain);
> > > >  
> > > >  	is_connected = tc_port_live_status_mask(dig_port) &
> > > >  		       BIT(dig_port->tc_mode);
> > > >  
> > > > -	tc_cold_unblock(dig_port, tc_cold_wref);
> > > > +	tc_cold_unblock(dig_port, domain, tc_cold_wref);
> > > >  	intel_tc_port_unlock(dig_port);
> > > >  
> > > >  	return is_connected;
> > > > @@ -698,15 +715,16 @@ static void __intel_tc_port_lock(struct intel_digital_port *dig_port,
> > > >  	mutex_lock(&dig_port->tc_lock);
> > > >  
> > > >  	if (!dig_port->tc_link_refcount) {
> > > > +		enum intel_display_power_domain domain;
> > > >  		intel_wakeref_t tc_cold_wref;
> > > >  
> > > > -		tc_cold_wref = tc_cold_block(dig_port);
> > > > +		tc_cold_wref = tc_cold_block(dig_port, &domain);
> > > >  
> > > >  		if (force_disconnect || intel_tc_port_needs_reset(dig_port))
> > > >  			intel_tc_port_reset_mode(dig_port, required_lanes,
> > > >  						 force_disconnect);
> > > >  
> > > > -		tc_cold_unblock(dig_port, tc_cold_wref);
> > > > +		tc_cold_unblock(dig_port, domain, tc_cold_wref);
> > > >  	}
> > > >  
> > > >  	drm_WARN_ON(&i915->drm, dig_port->tc_mode == TC_PORT_DISCONNECTED);
> > > > @@ -775,6 +793,7 @@ void intel_tc_port_put_link(struct intel_digital_port *dig_port)
> > > >  static bool
> > > >  tc_has_modular_fia(struct drm_i915_private *i915, struct intel_digital_port *dig_port)
> > > >  {
> > > > +	enum intel_display_power_domain domain;
> > > >  	intel_wakeref_t wakeref;
> > > >  	u32 val;
> > > >  
> > > > @@ -782,9 +801,9 @@ tc_has_modular_fia(struct drm_i915_private *i915, struct intel_digital_port *dig
> > > >  		return false;
> > > >  
> > > >  	mutex_lock(&dig_port->tc_lock);
> > > > -	wakeref = tc_cold_block(dig_port);
> > > > +	wakeref = tc_cold_block(dig_port, &domain);
> > > >  	val = intel_uncore_read(&i915->uncore, PORT_TX_DFLEXDPSP(FIA1));
> > > > -	tc_cold_unblock(dig_port, wakeref);
> > > > +	tc_cold_unblock(dig_port, domain, wakeref);
> > > >  	mutex_unlock(&dig_port->tc_lock);
> > > >  
> > > >  	drm_WARN_ON(&i915->drm, val == 0xffffffff);
> > > > @@ -829,11 +848,3 @@ void intel_tc_port_init(struct intel_digital_port *dig_port, bool is_legacy)
> > > >  	dig_port->tc_link_refcount = 0;
> > > >  	tc_port_load_fia_params(i915, dig_port);
> > > >  }
> > > > -
> > > > -bool intel_tc_cold_requires_aux_pw(struct intel_digital_port *dig_port)
> > > > -{
> > > > -	struct drm_i915_private *i915 = to_i915(dig_port->base.base.dev);
> > > > -
> > > > -	return (DISPLAY_VER(i915) == 11 && dig_port->tc_legacy_port) ||
> > > > -		IS_ALDERLAKE_P(i915);
> > > > -}
> > > 
>

Souza, Jose Sept. 27, 2021, 11:33 p.m. UTC | #5

On Tue, 2021-09-28 at 01:28 +0300, Imre Deak wrote:
> On Tue, Sep 28, 2021 at 01:21:21AM +0300, Souza, Jose wrote:
> > On Tue, 2021-09-28 at 01:13 +0300, Imre Deak wrote:
> > > On Tue, Sep 28, 2021 at 12:56:24AM +0300, Souza, Jose wrote:
> > > > On Tue, 2021-09-21 at 03:23 +0300, Imre Deak wrote:
> > > > > A follow-up change will select the TC-cold blocking power domain based
> > > > > on the TypeC mode, prepare for that here.
> > > > > 
> > > > > Also bring intel_tc_cold_requires_aux_pw() earlier to its logical place
> > > > > for readability.
> > > > > 
> > > > > No functional change.
> > > > > 
> > > > > Cc: José Roberto de Souza <jose.souza@intel.com>
> > > > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > > > ---
> > > > >  .../drm/i915/display/intel_display_types.h    |  2 +
> > > > >  drivers/gpu/drm/i915/display/intel_tc.c       | 63 +++++++++++--------
> > > > >  2 files changed, 39 insertions(+), 26 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
> > > > > index e9e806d90eec4..08a73ffded957 100644
> > > > > --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> > > > > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> > > > > @@ -1663,8 +1663,10 @@ struct intel_digital_port {
> > > > >  	enum intel_display_power_domain ddi_io_power_domain;
> > > > >  	intel_wakeref_t ddi_io_wakeref;
> > > > >  	intel_wakeref_t aux_wakeref;
> > > > > +
> > > > >  	struct mutex tc_lock;	/* protects the TypeC port mode */
> > > > >  	intel_wakeref_t tc_lock_wakeref;
> > > > > +	enum intel_display_power_domain tc_lock_power_domain;
> > > > >  	int tc_link_refcount;
> > > > >  	bool tc_legacy_port:1;
> > > > >  	char tc_port_name[8];
> > > > > diff --git a/drivers/gpu/drm/i915/display/intel_tc.c b/drivers/gpu/drm/i915/display/intel_tc.c
> > > > > index 77b16a7c43466..24d2dc2e19a7d 100644
> > > > > --- a/drivers/gpu/drm/i915/display/intel_tc.c
> > > > > +++ b/drivers/gpu/drm/i915/display/intel_tc.c
> > > > > @@ -48,8 +48,16 @@ bool intel_tc_port_in_legacy_mode(struct intel_digital_port *dig_port)
> > > > >  	return intel_tc_port_in_mode(dig_port, TC_PORT_LEGACY);
> > > > >  }
> > > > >  
> > > > > +bool intel_tc_cold_requires_aux_pw(struct intel_digital_port *dig_port)
> > > > > +{
> > > > > +	struct drm_i915_private *i915 = to_i915(dig_port->base.base.dev);
> > > > > +
> > > > > +	return (DISPLAY_VER(i915) == 11 && dig_port->tc_legacy_port) ||
> > > > > +		IS_ALDERLAKE_P(i915);
> > > > > +}
> > > > > +
> > > > >  static enum intel_display_power_domain
> > > > > -tc_cold_get_power_domain(struct intel_digital_port *dig_port)
> > > > > +tc_cold_get_power_domain(struct intel_digital_port *dig_port, enum tc_port_mode mode)
> > > > >  {
> > > > >  	if (intel_tc_cold_requires_aux_pw(dig_port))
> > > > >  		return intel_legacy_aux_to_power_domain(dig_port->aux_ch);
> > > > > @@ -58,23 +66,30 @@ tc_cold_get_power_domain(struct intel_digital_port *dig_port)
> > > > >  }
> > > > >  
> > > > >  static intel_wakeref_t
> > > > > -tc_cold_block(struct intel_digital_port *dig_port)
> > > > > +tc_cold_block_in_mode(struct intel_digital_port *dig_port, enum tc_port_mode mode,
> > > > > +		      enum intel_display_power_domain *domain)
> > > > >  {
> > > > >  	struct drm_i915_private *i915 = to_i915(dig_port->base.base.dev);
> > > > > -	enum intel_display_power_domain domain;
> > > > >  
> > > > >  	if (DISPLAY_VER(i915) == 11 && !dig_port->tc_legacy_port)
> > > > >  		return 0;
> > > > >  
> > > > > -	domain = tc_cold_get_power_domain(dig_port);
> > > > > -	return intel_display_power_get(i915, domain);
> > > > > +	*domain = tc_cold_get_power_domain(dig_port, mode);
> > > > 
> > > > I see problems with this approach.
> > > > 
> > > > If there is a TC alt-mode is connected and current software state is
> > > > TBT or disconnected it will not get the power domain to exit TC cold,
> > > > what could case invalid reads of registers.
> > > 
> > > If the mode needs to be changed, like in the above cases, the power
> > > domain required to exit TC cold will be taken.
> > 
> > How?
> > I see at least this 2 problematic cases.
> > 
> > intel_tc_port_sanitize()
> > 	tc_cold_block(mode=TBT)
> 
> It's in disconnected mode at this point.

Okay.

> 
> > 
> > 	dig_port->tc_mode = intel_tc_port_get_current_mode(dig_port);
> > 	...
> > 
> > 	tc_cold_unblock(dig_port, domain, tc_cold_wref);
> > 
> > Other case
> > 
> > intel_tc_port_connected()
> > 
> > 	tc_cold_block(mode=TBT)
> 
> Reading PORT_TX_DFLEXDPSP while in TC-cold returns 0xffffffff, and it's
> handled when reading it (which is also required by the spec).

Will only follow adl-p flow for this example, https://pastebin.com/raw/TVXdK1Et (pastebin just to make sure indentation is not lost between email
clients)

> 
> > 	is_connected = tc_port_live_status_mask(dig_port) &
> > 		       BIT(dig_port->tc_mode);
> > 
> > 	tc_cold_unblock(dig_port, domain, tc_cold_wref);
> > 
> > 
> > > 
> > > > From the next patch commit message: 'For the ADL-P TBT mode the spec
> > > > doesn't require blocking TC-cold by using the legacy AUX power
> > > > domain'.
> > > > 
> > > > It is not required for TBT but when there is nothing connected,
> > > > hardware is not in TBT mode so it can still get into TC cold.
> > > 
> > > If there is nothing connected the required power domain will be taken to
> > > exit TC cold.
> > > 
> > > > > +
> > > > > +	return intel_display_power_get(i915, *domain);
> > > > > +}
> > > > > +
> > > > > +static intel_wakeref_t
> > > > > +tc_cold_block(struct intel_digital_port *dig_port, enum intel_display_power_domain *domain)
> > > > > +{
> > > > > +	return tc_cold_block_in_mode(dig_port, dig_port->tc_mode, domain);
> > > > >  }
> > > > >  
> > > > >  static void
> > > > > -tc_cold_unblock(struct intel_digital_port *dig_port, intel_wakeref_t wakeref)
> > > > > +tc_cold_unblock(struct intel_digital_port *dig_port, enum intel_display_power_domain domain,
> > > > > +		intel_wakeref_t wakeref)
> > > > >  {
> > > > >  	struct drm_i915_private *i915 = to_i915(dig_port->base.base.dev);
> > > > > -	enum intel_display_power_domain domain;
> > > > >  
> > > > >  	/*
> > > > >  	 * wakeref == -1, means some error happened saving save_depot_stack but
> > > > > @@ -84,8 +99,7 @@ tc_cold_unblock(struct intel_digital_port *dig_port, intel_wakeref_t wakeref)
> > > > >  	if (wakeref == 0)
> > > > >  		return;
> > > > >  
> > > > > -	domain = tc_cold_get_power_domain(dig_port);
> > > > > -	intel_display_power_put_async(i915, domain, wakeref);
> > > > > +	intel_display_power_put(i915, domain, wakeref);
> > > > >  }
> > > > >  
> > > > >  static void
> > > > > @@ -98,7 +112,8 @@ assert_tc_cold_blocked(struct intel_digital_port *dig_port)
> > > > >  		return;
> > > > >  
> > > > >  	enabled = intel_display_power_is_enabled(i915,
> > > > > -						 tc_cold_get_power_domain(dig_port));
> > > > > +						 tc_cold_get_power_domain(dig_port,
> > > > > +									  dig_port->tc_mode));
> > > > >  	drm_WARN_ON(&i915->drm, !enabled);
> > > > >  }
> > > > >  
> > > > > @@ -634,7 +649,8 @@ void intel_tc_port_sanitize(struct intel_digital_port *dig_port)
> > > > >  
> > > > >  	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);
> > > > > +		enum intel_display_power_domain domain;
> > > > > +		intel_wakeref_t tc_cold_wref = tc_cold_block(dig_port, &domain);
> > > > >  
> > > > >  		dig_port->tc_mode = intel_tc_port_get_current_mode(dig_port);
> > > > >  
> > > > > @@ -644,7 +660,7 @@ void intel_tc_port_sanitize(struct intel_digital_port *dig_port)
> > > > >  				    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);
> > > > > +		tc_cold_unblock(dig_port, domain, tc_cold_wref);
> > > > >  	}
> > > > >  
> > > > >  	drm_dbg_kms(&i915->drm, "Port %s: sanitize mode (%s)\n",
> > > > > @@ -673,15 +689,16 @@ bool intel_tc_port_connected(struct intel_encoder *encoder)
> > > > >  {
> > > > >  	struct intel_digital_port *dig_port = enc_to_dig_port(encoder);
> > > > >  	bool is_connected;
> > > > > +	enum intel_display_power_domain domain;
> > > > >  	intel_wakeref_t tc_cold_wref;
> > > > >  
> > > > >  	intel_tc_port_lock(dig_port);
> > > > > -	tc_cold_wref = tc_cold_block(dig_port);
> > > > > +	tc_cold_wref = tc_cold_block(dig_port, &domain);
> > > > >  
> > > > >  	is_connected = tc_port_live_status_mask(dig_port) &
> > > > >  		       BIT(dig_port->tc_mode);
> > > > >  
> > > > > -	tc_cold_unblock(dig_port, tc_cold_wref);
> > > > > +	tc_cold_unblock(dig_port, domain, tc_cold_wref);
> > > > >  	intel_tc_port_unlock(dig_port);
> > > > >  
> > > > >  	return is_connected;
> > > > > @@ -698,15 +715,16 @@ static void __intel_tc_port_lock(struct intel_digital_port *dig_port,
> > > > >  	mutex_lock(&dig_port->tc_lock);
> > > > >  
> > > > >  	if (!dig_port->tc_link_refcount) {
> > > > > +		enum intel_display_power_domain domain;
> > > > >  		intel_wakeref_t tc_cold_wref;
> > > > >  
> > > > > -		tc_cold_wref = tc_cold_block(dig_port);
> > > > > +		tc_cold_wref = tc_cold_block(dig_port, &domain);
> > > > >  
> > > > >  		if (force_disconnect || intel_tc_port_needs_reset(dig_port))
> > > > >  			intel_tc_port_reset_mode(dig_port, required_lanes,
> > > > >  						 force_disconnect);
> > > > >  
> > > > > -		tc_cold_unblock(dig_port, tc_cold_wref);
> > > > > +		tc_cold_unblock(dig_port, domain, tc_cold_wref);
> > > > >  	}
> > > > >  
> > > > >  	drm_WARN_ON(&i915->drm, dig_port->tc_mode == TC_PORT_DISCONNECTED);
> > > > > @@ -775,6 +793,7 @@ void intel_tc_port_put_link(struct intel_digital_port *dig_port)
> > > > >  static bool
> > > > >  tc_has_modular_fia(struct drm_i915_private *i915, struct intel_digital_port *dig_port)
> > > > >  {
> > > > > +	enum intel_display_power_domain domain;
> > > > >  	intel_wakeref_t wakeref;
> > > > >  	u32 val;
> > > > >  
> > > > > @@ -782,9 +801,9 @@ tc_has_modular_fia(struct drm_i915_private *i915, struct intel_digital_port *dig
> > > > >  		return false;
> > > > >  
> > > > >  	mutex_lock(&dig_port->tc_lock);
> > > > > -	wakeref = tc_cold_block(dig_port);
> > > > > +	wakeref = tc_cold_block(dig_port, &domain);
> > > > >  	val = intel_uncore_read(&i915->uncore, PORT_TX_DFLEXDPSP(FIA1));
> > > > > -	tc_cold_unblock(dig_port, wakeref);
> > > > > +	tc_cold_unblock(dig_port, domain, wakeref);
> > > > >  	mutex_unlock(&dig_port->tc_lock);
> > > > >  
> > > > >  	drm_WARN_ON(&i915->drm, val == 0xffffffff);
> > > > > @@ -829,11 +848,3 @@ void intel_tc_port_init(struct intel_digital_port *dig_port, bool is_legacy)
> > > > >  	dig_port->tc_link_refcount = 0;
> > > > >  	tc_port_load_fia_params(i915, dig_port);
> > > > >  }
> > > > > -
> > > > > -bool intel_tc_cold_requires_aux_pw(struct intel_digital_port *dig_port)
> > > > > -{
> > > > > -	struct drm_i915_private *i915 = to_i915(dig_port->base.base.dev);
> > > > > -
> > > > > -	return (DISPLAY_VER(i915) == 11 && dig_port->tc_legacy_port) ||
> > > > > -		IS_ALDERLAKE_P(i915);
> > > > > -}
> > > > 
> >

Imre Deak Sept. 27, 2021, 11:51 p.m. UTC | #6

On Tue, Sep 28, 2021 at 02:33:27AM +0300, Souza, Jose wrote:
> On Tue, 2021-09-28 at 01:28 +0300, Imre Deak wrote:
> > On Tue, Sep 28, 2021 at 01:21:21AM +0300, Souza, Jose wrote:
> > > On Tue, 2021-09-28 at 01:13 +0300, Imre Deak wrote:
> > > > On Tue, Sep 28, 2021 at 12:56:24AM +0300, Souza, Jose wrote:
> > > > > On Tue, 2021-09-21 at 03:23 +0300, Imre Deak wrote:
> > > > > > A follow-up change will select the TC-cold blocking power domain based
> > > > > > on the TypeC mode, prepare for that here.
> > > > > > 
> > > > > > Also bring intel_tc_cold_requires_aux_pw() earlier to its logical place
> > > > > > for readability.
> > > > > > 
> > > > > > No functional change.
> > > > > > 
> > > > > > Cc: José Roberto de Souza <jose.souza@intel.com>
> > > > > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > > > > ---
> > > > > >  .../drm/i915/display/intel_display_types.h    |  2 +
> > > > > >  drivers/gpu/drm/i915/display/intel_tc.c       | 63 +++++++++++--------
> > > > > >  2 files changed, 39 insertions(+), 26 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
> > > > > > index e9e806d90eec4..08a73ffded957 100644
> > > > > > --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> > > > > > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> > > > > > @@ -1663,8 +1663,10 @@ struct intel_digital_port {
> > > > > >  	enum intel_display_power_domain ddi_io_power_domain;
> > > > > >  	intel_wakeref_t ddi_io_wakeref;
> > > > > >  	intel_wakeref_t aux_wakeref;
> > > > > > +
> > > > > >  	struct mutex tc_lock;	/* protects the TypeC port mode */
> > > > > >  	intel_wakeref_t tc_lock_wakeref;
> > > > > > +	enum intel_display_power_domain tc_lock_power_domain;
> > > > > >  	int tc_link_refcount;
> > > > > >  	bool tc_legacy_port:1;
> > > > > >  	char tc_port_name[8];
> > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_tc.c b/drivers/gpu/drm/i915/display/intel_tc.c
> > > > > > index 77b16a7c43466..24d2dc2e19a7d 100644
> > > > > > --- a/drivers/gpu/drm/i915/display/intel_tc.c
> > > > > > +++ b/drivers/gpu/drm/i915/display/intel_tc.c
> > > > > > @@ -48,8 +48,16 @@ bool intel_tc_port_in_legacy_mode(struct intel_digital_port *dig_port)
> > > > > >  	return intel_tc_port_in_mode(dig_port, TC_PORT_LEGACY);
> > > > > >  }
> > > > > >  
> > > > > > +bool intel_tc_cold_requires_aux_pw(struct intel_digital_port *dig_port)
> > > > > > +{
> > > > > > +	struct drm_i915_private *i915 = to_i915(dig_port->base.base.dev);
> > > > > > +
> > > > > > +	return (DISPLAY_VER(i915) == 11 && dig_port->tc_legacy_port) ||
> > > > > > +		IS_ALDERLAKE_P(i915);
> > > > > > +}
> > > > > > +
> > > > > >  static enum intel_display_power_domain
> > > > > > -tc_cold_get_power_domain(struct intel_digital_port *dig_port)
> > > > > > +tc_cold_get_power_domain(struct intel_digital_port *dig_port, enum tc_port_mode mode)
> > > > > >  {
> > > > > >  	if (intel_tc_cold_requires_aux_pw(dig_port))
> > > > > >  		return intel_legacy_aux_to_power_domain(dig_port->aux_ch);
> > > > > > @@ -58,23 +66,30 @@ tc_cold_get_power_domain(struct intel_digital_port *dig_port)
> > > > > >  }
> > > > > >  
> > > > > >  static intel_wakeref_t
> > > > > > -tc_cold_block(struct intel_digital_port *dig_port)
> > > > > > +tc_cold_block_in_mode(struct intel_digital_port *dig_port, enum tc_port_mode mode,
> > > > > > +		      enum intel_display_power_domain *domain)
> > > > > >  {
> > > > > >  	struct drm_i915_private *i915 = to_i915(dig_port->base.base.dev);
> > > > > > -	enum intel_display_power_domain domain;
> > > > > >  
> > > > > >  	if (DISPLAY_VER(i915) == 11 && !dig_port->tc_legacy_port)
> > > > > >  		return 0;
> > > > > >  
> > > > > > -	domain = tc_cold_get_power_domain(dig_port);
> > > > > > -	return intel_display_power_get(i915, domain);
> > > > > > +	*domain = tc_cold_get_power_domain(dig_port, mode);
> > > > > 
> > > > > I see problems with this approach.
> > > > > 
> > > > > If there is a TC alt-mode is connected and current software state is
> > > > > TBT or disconnected it will not get the power domain to exit TC cold,
> > > > > what could case invalid reads of registers.
> > > > 
> > > > If the mode needs to be changed, like in the above cases, the power
> > > > domain required to exit TC cold will be taken.
> > > 
> > > How?
> > > I see at least this 2 problematic cases.
> > > 
> > > intel_tc_port_sanitize()
> > > 	tc_cold_block(mode=TBT)
> > 
> > It's in disconnected mode at this point.
> 
> Okay.
> 
> > 
> > > 
> > > 	dig_port->tc_mode = intel_tc_port_get_current_mode(dig_port);
> > > 	...
> > > 
> > > 	tc_cold_unblock(dig_port, domain, tc_cold_wref);
> > > 
> > > Other case
> > > 
> > > intel_tc_port_connected()
> > > 
> > > 	tc_cold_block(mode=TBT)
> > 
> > Reading PORT_TX_DFLEXDPSP while in TC-cold returns 0xffffffff, and it's
> > handled when reading it (which is also required by the spec).
> 
> Will only follow adl-p flow for this
> example, https://pastebin.com/raw/TVXdK1Et (pastebin just to make sure
> indentation is not lost between email clients)

TCSS_DDI_STATUS is in IOM (so neither a FIA or a PHY register). From Bspec 55480:
"""
Type-C Flow Registers Used by Display Software
...
Location          Register
IOM               TCSS_DDI_STATUS
"""

and IOM registers are accessible in TC-cold:

"""
TCCOLD
The type-C subsystem can enter the TCCOLD power saving state where FIA and type-C
PHY registers become inaccessible and respond to reads with all 1s data,
while IOM remains accessible. 
"""

> > > 		       BIT(dig_port->tc_mode);
> > > 
> > > 	tc_cold_unblock(dig_port, domain, tc_cold_wref);
> > > 
> > > 
> > > > 
> > > > > From the next patch commit message: 'For the ADL-P TBT mode the spec
> > > > > doesn't require blocking TC-cold by using the legacy AUX power
> > > > > domain'.
> > > > > 
> > > > > It is not required for TBT but when there is nothing connected,
> > > > > hardware is not in TBT mode so it can still get into TC cold.
> > > > 
> > > > If there is nothing connected the required power domain will be taken to
> > > > exit TC cold.
> > > > 
> > > > > > +
> > > > > > +	return intel_display_power_get(i915, *domain);
> > > > > > +}
> > > > > > +
> > > > > > +static intel_wakeref_t
> > > > > > +tc_cold_block(struct intel_digital_port *dig_port, enum intel_display_power_domain *domain)
> > > > > > +{
> > > > > > +	return tc_cold_block_in_mode(dig_port, dig_port->tc_mode, domain);
> > > > > >  }
> > > > > >  
> > > > > >  static void
> > > > > > -tc_cold_unblock(struct intel_digital_port *dig_port, intel_wakeref_t wakeref)
> > > > > > +tc_cold_unblock(struct intel_digital_port *dig_port, enum intel_display_power_domain domain,
> > > > > > +		intel_wakeref_t wakeref)
> > > > > >  {
> > > > > >  	struct drm_i915_private *i915 = to_i915(dig_port->base.base.dev);
> > > > > > -	enum intel_display_power_domain domain;
> > > > > >  
> > > > > >  	/*
> > > > > >  	 * wakeref == -1, means some error happened saving save_depot_stack but
> > > > > > @@ -84,8 +99,7 @@ tc_cold_unblock(struct intel_digital_port *dig_port, intel_wakeref_t wakeref)
> > > > > >  	if (wakeref == 0)
> > > > > >  		return;
> > > > > >  
> > > > > > -	domain = tc_cold_get_power_domain(dig_port);
> > > > > > -	intel_display_power_put_async(i915, domain, wakeref);
> > > > > > +	intel_display_power_put(i915, domain, wakeref);
> > > > > >  }
> > > > > >  
> > > > > >  static void
> > > > > > @@ -98,7 +112,8 @@ assert_tc_cold_blocked(struct intel_digital_port *dig_port)
> > > > > >  		return;
> > > > > >  
> > > > > >  	enabled = intel_display_power_is_enabled(i915,
> > > > > > -						 tc_cold_get_power_domain(dig_port));
> > > > > > +						 tc_cold_get_power_domain(dig_port,
> > > > > > +									  dig_port->tc_mode));
> > > > > >  	drm_WARN_ON(&i915->drm, !enabled);
> > > > > >  }
> > > > > >  
> > > > > > @@ -634,7 +649,8 @@ void intel_tc_port_sanitize(struct intel_digital_port *dig_port)
> > > > > >  
> > > > > >  	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);
> > > > > > +		enum intel_display_power_domain domain;
> > > > > > +		intel_wakeref_t tc_cold_wref = tc_cold_block(dig_port, &domain);
> > > > > >  
> > > > > >  		dig_port->tc_mode = intel_tc_port_get_current_mode(dig_port);
> > > > > >  
> > > > > > @@ -644,7 +660,7 @@ void intel_tc_port_sanitize(struct intel_digital_port *dig_port)
> > > > > >  				    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);
> > > > > > +		tc_cold_unblock(dig_port, domain, tc_cold_wref);
> > > > > >  	}
> > > > > >  
> > > > > >  	drm_dbg_kms(&i915->drm, "Port %s: sanitize mode (%s)\n",
> > > > > > @@ -673,15 +689,16 @@ bool intel_tc_port_connected(struct intel_encoder *encoder)
> > > > > >  {
> > > > > >  	struct intel_digital_port *dig_port = enc_to_dig_port(encoder);
> > > > > >  	bool is_connected;
> > > > > > +	enum intel_display_power_domain domain;
> > > > > >  	intel_wakeref_t tc_cold_wref;
> > > > > >  
> > > > > >  	intel_tc_port_lock(dig_port);
> > > > > > -	tc_cold_wref = tc_cold_block(dig_port);
> > > > > > +	tc_cold_wref = tc_cold_block(dig_port, &domain);
> > > > > >  
> > > > > >  	is_connected = tc_port_live_status_mask(dig_port) &
> > > > > >  		       BIT(dig_port->tc_mode);
> > > > > >  
> > > > > > -	tc_cold_unblock(dig_port, tc_cold_wref);
> > > > > > +	tc_cold_unblock(dig_port, domain, tc_cold_wref);
> > > > > >  	intel_tc_port_unlock(dig_port);
> > > > > >  
> > > > > >  	return is_connected;
> > > > > > @@ -698,15 +715,16 @@ static void __intel_tc_port_lock(struct intel_digital_port *dig_port,
> > > > > >  	mutex_lock(&dig_port->tc_lock);
> > > > > >  
> > > > > >  	if (!dig_port->tc_link_refcount) {
> > > > > > +		enum intel_display_power_domain domain;
> > > > > >  		intel_wakeref_t tc_cold_wref;
> > > > > >  
> > > > > > -		tc_cold_wref = tc_cold_block(dig_port);
> > > > > > +		tc_cold_wref = tc_cold_block(dig_port, &domain);
> > > > > >  
> > > > > >  		if (force_disconnect || intel_tc_port_needs_reset(dig_port))
> > > > > >  			intel_tc_port_reset_mode(dig_port, required_lanes,
> > > > > >  						 force_disconnect);
> > > > > >  
> > > > > > -		tc_cold_unblock(dig_port, tc_cold_wref);
> > > > > > +		tc_cold_unblock(dig_port, domain, tc_cold_wref);
> > > > > >  	}
> > > > > >  
> > > > > >  	drm_WARN_ON(&i915->drm, dig_port->tc_mode == TC_PORT_DISCONNECTED);
> > > > > > @@ -775,6 +793,7 @@ void intel_tc_port_put_link(struct intel_digital_port *dig_port)
> > > > > >  static bool
> > > > > >  tc_has_modular_fia(struct drm_i915_private *i915, struct intel_digital_port *dig_port)
> > > > > >  {
> > > > > > +	enum intel_display_power_domain domain;
> > > > > >  	intel_wakeref_t wakeref;
> > > > > >  	u32 val;
> > > > > >  
> > > > > > @@ -782,9 +801,9 @@ tc_has_modular_fia(struct drm_i915_private *i915, struct intel_digital_port *dig
> > > > > >  		return false;
> > > > > >  
> > > > > >  	mutex_lock(&dig_port->tc_lock);
> > > > > > -	wakeref = tc_cold_block(dig_port);
> > > > > > +	wakeref = tc_cold_block(dig_port, &domain);
> > > > > >  	val = intel_uncore_read(&i915->uncore, PORT_TX_DFLEXDPSP(FIA1));
> > > > > > -	tc_cold_unblock(dig_port, wakeref);
> > > > > > +	tc_cold_unblock(dig_port, domain, wakeref);
> > > > > >  	mutex_unlock(&dig_port->tc_lock);
> > > > > >  
> > > > > >  	drm_WARN_ON(&i915->drm, val == 0xffffffff);
> > > > > > @@ -829,11 +848,3 @@ void intel_tc_port_init(struct intel_digital_port *dig_port, bool is_legacy)
> > > > > >  	dig_port->tc_link_refcount = 0;
> > > > > >  	tc_port_load_fia_params(i915, dig_port);
> > > > > >  }
> > > > > > -
> > > > > > -bool intel_tc_cold_requires_aux_pw(struct intel_digital_port *dig_port)
> > > > > > -{
> > > > > > -	struct drm_i915_private *i915 = to_i915(dig_port->base.base.dev);
> > > > > > -
> > > > > > -	return (DISPLAY_VER(i915) == 11 && dig_port->tc_legacy_port) ||
> > > > > > -		IS_ALDERLAKE_P(i915);
> > > > > > -}
> > > > > 
> > > 
>

Souza, Jose Sept. 28, 2021, 12:14 a.m. UTC | #7

On Tue, 2021-09-28 at 02:51 +0300, Imre Deak wrote:
> On Tue, Sep 28, 2021 at 02:33:27AM +0300, Souza, Jose wrote:
> > On Tue, 2021-09-28 at 01:28 +0300, Imre Deak wrote:
> > > On Tue, Sep 28, 2021 at 01:21:21AM +0300, Souza, Jose wrote:
> > > > On Tue, 2021-09-28 at 01:13 +0300, Imre Deak wrote:
> > > > > On Tue, Sep 28, 2021 at 12:56:24AM +0300, Souza, Jose wrote:
> > > > > > On Tue, 2021-09-21 at 03:23 +0300, Imre Deak wrote:
> > > > > > > A follow-up change will select the TC-cold blocking power domain based
> > > > > > > on the TypeC mode, prepare for that here.
> > > > > > > 
> > > > > > > Also bring intel_tc_cold_requires_aux_pw() earlier to its logical place
> > > > > > > for readability.
> > > > > > > 
> > > > > > > No functional change.
> > > > > > > 
> > > > > > > Cc: José Roberto de Souza <jose.souza@intel.com>
> > > > > > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > > > > > ---
> > > > > > >  .../drm/i915/display/intel_display_types.h    |  2 +
> > > > > > >  drivers/gpu/drm/i915/display/intel_tc.c       | 63 +++++++++++--------
> > > > > > >  2 files changed, 39 insertions(+), 26 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
> > > > > > > index e9e806d90eec4..08a73ffded957 100644
> > > > > > > --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> > > > > > > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> > > > > > > @@ -1663,8 +1663,10 @@ struct intel_digital_port {
> > > > > > >  	enum intel_display_power_domain ddi_io_power_domain;
> > > > > > >  	intel_wakeref_t ddi_io_wakeref;
> > > > > > >  	intel_wakeref_t aux_wakeref;
> > > > > > > +
> > > > > > >  	struct mutex tc_lock;	/* protects the TypeC port mode */
> > > > > > >  	intel_wakeref_t tc_lock_wakeref;
> > > > > > > +	enum intel_display_power_domain tc_lock_power_domain;
> > > > > > >  	int tc_link_refcount;
> > > > > > >  	bool tc_legacy_port:1;
> > > > > > >  	char tc_port_name[8];
> > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_tc.c b/drivers/gpu/drm/i915/display/intel_tc.c
> > > > > > > index 77b16a7c43466..24d2dc2e19a7d 100644
> > > > > > > --- a/drivers/gpu/drm/i915/display/intel_tc.c
> > > > > > > +++ b/drivers/gpu/drm/i915/display/intel_tc.c
> > > > > > > @@ -48,8 +48,16 @@ bool intel_tc_port_in_legacy_mode(struct intel_digital_port *dig_port)
> > > > > > >  	return intel_tc_port_in_mode(dig_port, TC_PORT_LEGACY);
> > > > > > >  }
> > > > > > >  
> > > > > > > +bool intel_tc_cold_requires_aux_pw(struct intel_digital_port *dig_port)
> > > > > > > +{
> > > > > > > +	struct drm_i915_private *i915 = to_i915(dig_port->base.base.dev);
> > > > > > > +
> > > > > > > +	return (DISPLAY_VER(i915) == 11 && dig_port->tc_legacy_port) ||
> > > > > > > +		IS_ALDERLAKE_P(i915);
> > > > > > > +}
> > > > > > > +
> > > > > > >  static enum intel_display_power_domain
> > > > > > > -tc_cold_get_power_domain(struct intel_digital_port *dig_port)
> > > > > > > +tc_cold_get_power_domain(struct intel_digital_port *dig_port, enum tc_port_mode mode)
> > > > > > >  {
> > > > > > >  	if (intel_tc_cold_requires_aux_pw(dig_port))
> > > > > > >  		return intel_legacy_aux_to_power_domain(dig_port->aux_ch);
> > > > > > > @@ -58,23 +66,30 @@ tc_cold_get_power_domain(struct intel_digital_port *dig_port)
> > > > > > >  }
> > > > > > >  
> > > > > > >  static intel_wakeref_t
> > > > > > > -tc_cold_block(struct intel_digital_port *dig_port)
> > > > > > > +tc_cold_block_in_mode(struct intel_digital_port *dig_port, enum tc_port_mode mode,
> > > > > > > +		      enum intel_display_power_domain *domain)
> > > > > > >  {
> > > > > > >  	struct drm_i915_private *i915 = to_i915(dig_port->base.base.dev);
> > > > > > > -	enum intel_display_power_domain domain;
> > > > > > >  
> > > > > > >  	if (DISPLAY_VER(i915) == 11 && !dig_port->tc_legacy_port)
> > > > > > >  		return 0;
> > > > > > >  
> > > > > > > -	domain = tc_cold_get_power_domain(dig_port);
> > > > > > > -	return intel_display_power_get(i915, domain);
> > > > > > > +	*domain = tc_cold_get_power_domain(dig_port, mode);
> > > > > > 
> > > > > > I see problems with this approach.
> > > > > > 
> > > > > > If there is a TC alt-mode is connected and current software state is
> > > > > > TBT or disconnected it will not get the power domain to exit TC cold,
> > > > > > what could case invalid reads of registers.
> > > > > 
> > > > > If the mode needs to be changed, like in the above cases, the power
> > > > > domain required to exit TC cold will be taken.
> > > > 
> > > > How?
> > > > I see at least this 2 problematic cases.
> > > > 
> > > > intel_tc_port_sanitize()
> > > > 	tc_cold_block(mode=TBT)
> > > 
> > > It's in disconnected mode at this point.
> > 
> > Okay.
> > 
> > > 
> > > > 
> > > > 	dig_port->tc_mode = intel_tc_port_get_current_mode(dig_port);
> > > > 	...
> > > > 
> > > > 	tc_cold_unblock(dig_port, domain, tc_cold_wref);
> > > > 
> > > > Other case
> > > > 
> > > > intel_tc_port_connected()
> > > > 
> > > > 	tc_cold_block(mode=TBT)
> > > 
> > > Reading PORT_TX_DFLEXDPSP while in TC-cold returns 0xffffffff, and it's
> > > handled when reading it (which is also required by the spec).
> > 
> > Will only follow adl-p flow for this
> > example, https://pastebin.com/raw/TVXdK1Et (pastebin just to make sure
> > indentation is not lost between email clients)
> 
> TCSS_DDI_STATUS is in IOM (so neither a FIA or a PHY register). From Bspec 55480:
> """
> Type-C Flow Registers Used by Display Software
> ...
> Location          Register
> IOM               TCSS_DDI_STATUS
> """
> 
> and IOM registers are accessible in TC-cold:
> 
> """
> TCCOLD
> The type-C subsystem can enter the TCCOLD power saving state where FIA and type-C
> PHY registers become inaccessible and respond to reads with all 1s data,
> while IOM remains accessible. 

Ohh okay, maybe add some comments about the above.

For TGL taking POWER_DOMAIN_TC_COLD_OFF will solve everything.

What about ICL in legacy mode, when going from TC_PORT_TBT_ALT to TC_PORT_LEGACY.
Reading PORT_TX_DFLEXDPSP in icl_tc_port_live_status_mask() will set mask to BIT(TC_PORT_TBT_ALT) | BIT(TC_PORT_DP_ALT) | BIT(TC_PORT_LEGACY).

That would not be a problem if SDEISR bit for TC port is always set when in legacy mode.
Do you know what is the behavior here?
The handling for going from TC_PORT_DISCONNECTED to TC_PORT_LEGACY is properly handled.


> """
> 
> > > > 		       BIT(dig_port->tc_mode);
> > > > 
> > > > 	tc_cold_unblock(dig_port, domain, tc_cold_wref);
> > > > 
> > > > 
> > > > > 
> > > > > > From the next patch commit message: 'For the ADL-P TBT mode the spec
> > > > > > doesn't require blocking TC-cold by using the legacy AUX power
> > > > > > domain'.
> > > > > > 
> > > > > > It is not required for TBT but when there is nothing connected,
> > > > > > hardware is not in TBT mode so it can still get into TC cold.
> > > > > 
> > > > > If there is nothing connected the required power domain will be taken to
> > > > > exit TC cold.
> > > > > 
> > > > > > > +
> > > > > > > +	return intel_display_power_get(i915, *domain);
> > > > > > > +}
> > > > > > > +
> > > > > > > +static intel_wakeref_t
> > > > > > > +tc_cold_block(struct intel_digital_port *dig_port, enum intel_display_power_domain *domain)
> > > > > > > +{
> > > > > > > +	return tc_cold_block_in_mode(dig_port, dig_port->tc_mode, domain);
> > > > > > >  }
> > > > > > >  
> > > > > > >  static void
> > > > > > > -tc_cold_unblock(struct intel_digital_port *dig_port, intel_wakeref_t wakeref)
> > > > > > > +tc_cold_unblock(struct intel_digital_port *dig_port, enum intel_display_power_domain domain,
> > > > > > > +		intel_wakeref_t wakeref)
> > > > > > >  {
> > > > > > >  	struct drm_i915_private *i915 = to_i915(dig_port->base.base.dev);
> > > > > > > -	enum intel_display_power_domain domain;
> > > > > > >  
> > > > > > >  	/*
> > > > > > >  	 * wakeref == -1, means some error happened saving save_depot_stack but
> > > > > > > @@ -84,8 +99,7 @@ tc_cold_unblock(struct intel_digital_port *dig_port, intel_wakeref_t wakeref)
> > > > > > >  	if (wakeref == 0)
> > > > > > >  		return;
> > > > > > >  
> > > > > > > -	domain = tc_cold_get_power_domain(dig_port);
> > > > > > > -	intel_display_power_put_async(i915, domain, wakeref);
> > > > > > > +	intel_display_power_put(i915, domain, wakeref);
> > > > > > >  }
> > > > > > >  
> > > > > > >  static void
> > > > > > > @@ -98,7 +112,8 @@ assert_tc_cold_blocked(struct intel_digital_port *dig_port)
> > > > > > >  		return;
> > > > > > >  
> > > > > > >  	enabled = intel_display_power_is_enabled(i915,
> > > > > > > -						 tc_cold_get_power_domain(dig_port));
> > > > > > > +						 tc_cold_get_power_domain(dig_port,
> > > > > > > +									  dig_port->tc_mode));
> > > > > > >  	drm_WARN_ON(&i915->drm, !enabled);
> > > > > > >  }
> > > > > > >  
> > > > > > > @@ -634,7 +649,8 @@ void intel_tc_port_sanitize(struct intel_digital_port *dig_port)
> > > > > > >  
> > > > > > >  	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);
> > > > > > > +		enum intel_display_power_domain domain;
> > > > > > > +		intel_wakeref_t tc_cold_wref = tc_cold_block(dig_port, &domain);
> > > > > > >  
> > > > > > >  		dig_port->tc_mode = intel_tc_port_get_current_mode(dig_port);
> > > > > > >  
> > > > > > > @@ -644,7 +660,7 @@ void intel_tc_port_sanitize(struct intel_digital_port *dig_port)
> > > > > > >  				    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);
> > > > > > > +		tc_cold_unblock(dig_port, domain, tc_cold_wref);
> > > > > > >  	}
> > > > > > >  
> > > > > > >  	drm_dbg_kms(&i915->drm, "Port %s: sanitize mode (%s)\n",
> > > > > > > @@ -673,15 +689,16 @@ bool intel_tc_port_connected(struct intel_encoder *encoder)
> > > > > > >  {
> > > > > > >  	struct intel_digital_port *dig_port = enc_to_dig_port(encoder);
> > > > > > >  	bool is_connected;
> > > > > > > +	enum intel_display_power_domain domain;
> > > > > > >  	intel_wakeref_t tc_cold_wref;
> > > > > > >  
> > > > > > >  	intel_tc_port_lock(dig_port);
> > > > > > > -	tc_cold_wref = tc_cold_block(dig_port);
> > > > > > > +	tc_cold_wref = tc_cold_block(dig_port, &domain);
> > > > > > >  
> > > > > > >  	is_connected = tc_port_live_status_mask(dig_port) &
> > > > > > >  		       BIT(dig_port->tc_mode);
> > > > > > >  
> > > > > > > -	tc_cold_unblock(dig_port, tc_cold_wref);
> > > > > > > +	tc_cold_unblock(dig_port, domain, tc_cold_wref);
> > > > > > >  	intel_tc_port_unlock(dig_port);
> > > > > > >  
> > > > > > >  	return is_connected;
> > > > > > > @@ -698,15 +715,16 @@ static void __intel_tc_port_lock(struct intel_digital_port *dig_port,
> > > > > > >  	mutex_lock(&dig_port->tc_lock);
> > > > > > >  
> > > > > > >  	if (!dig_port->tc_link_refcount) {
> > > > > > > +		enum intel_display_power_domain domain;
> > > > > > >  		intel_wakeref_t tc_cold_wref;
> > > > > > >  
> > > > > > > -		tc_cold_wref = tc_cold_block(dig_port);
> > > > > > > +		tc_cold_wref = tc_cold_block(dig_port, &domain);
> > > > > > >  
> > > > > > >  		if (force_disconnect || intel_tc_port_needs_reset(dig_port))
> > > > > > >  			intel_tc_port_reset_mode(dig_port, required_lanes,
> > > > > > >  						 force_disconnect);
> > > > > > >  
> > > > > > > -		tc_cold_unblock(dig_port, tc_cold_wref);
> > > > > > > +		tc_cold_unblock(dig_port, domain, tc_cold_wref);
> > > > > > >  	}
> > > > > > >  
> > > > > > >  	drm_WARN_ON(&i915->drm, dig_port->tc_mode == TC_PORT_DISCONNECTED);
> > > > > > > @@ -775,6 +793,7 @@ void intel_tc_port_put_link(struct intel_digital_port *dig_port)
> > > > > > >  static bool
> > > > > > >  tc_has_modular_fia(struct drm_i915_private *i915, struct intel_digital_port *dig_port)
> > > > > > >  {
> > > > > > > +	enum intel_display_power_domain domain;
> > > > > > >  	intel_wakeref_t wakeref;
> > > > > > >  	u32 val;
> > > > > > >  
> > > > > > > @@ -782,9 +801,9 @@ tc_has_modular_fia(struct drm_i915_private *i915, struct intel_digital_port *dig
> > > > > > >  		return false;
> > > > > > >  
> > > > > > >  	mutex_lock(&dig_port->tc_lock);
> > > > > > > -	wakeref = tc_cold_block(dig_port);
> > > > > > > +	wakeref = tc_cold_block(dig_port, &domain);
> > > > > > >  	val = intel_uncore_read(&i915->uncore, PORT_TX_DFLEXDPSP(FIA1));
> > > > > > > -	tc_cold_unblock(dig_port, wakeref);
> > > > > > > +	tc_cold_unblock(dig_port, domain, wakeref);
> > > > > > >  	mutex_unlock(&dig_port->tc_lock);
> > > > > > >  
> > > > > > >  	drm_WARN_ON(&i915->drm, val == 0xffffffff);
> > > > > > > @@ -829,11 +848,3 @@ void intel_tc_port_init(struct intel_digital_port *dig_port, bool is_legacy)
> > > > > > >  	dig_port->tc_link_refcount = 0;
> > > > > > >  	tc_port_load_fia_params(i915, dig_port);
> > > > > > >  }
> > > > > > > -
> > > > > > > -bool intel_tc_cold_requires_aux_pw(struct intel_digital_port *dig_port)
> > > > > > > -{
> > > > > > > -	struct drm_i915_private *i915 = to_i915(dig_port->base.base.dev);
> > > > > > > -
> > > > > > > -	return (DISPLAY_VER(i915) == 11 && dig_port->tc_legacy_port) ||
> > > > > > > -		IS_ALDERLAKE_P(i915);
> > > > > > > -}
> > > > > > 
> > > > 
> >

Imre Deak Sept. 28, 2021, 12:45 a.m. UTC | #8

On Tue, Sep 28, 2021 at 03:14:45AM +0300, Souza, Jose wrote:
> On Tue, 2021-09-28 at 02:51 +0300, Imre Deak wrote:
> > On Tue, Sep 28, 2021 at 02:33:27AM +0300, Souza, Jose wrote:
> > > On Tue, 2021-09-28 at 01:28 +0300, Imre Deak wrote:
> > > > On Tue, Sep 28, 2021 at 01:21:21AM +0300, Souza, Jose wrote:
> > > > > On Tue, 2021-09-28 at 01:13 +0300, Imre Deak wrote:
> > > > > > On Tue, Sep 28, 2021 at 12:56:24AM +0300, Souza, Jose wrote:
> > > > > > > On Tue, 2021-09-21 at 03:23 +0300, Imre Deak wrote:
> > > > > > > > A follow-up change will select the TC-cold blocking power domain based
> > > > > > > > on the TypeC mode, prepare for that here.
> > > > > > > > 
> > > > > > > > Also bring intel_tc_cold_requires_aux_pw() earlier to its logical place
> > > > > > > > for readability.
> > > > > > > > 
> > > > > > > > No functional change.
> > > > > > > > 
> > > > > > > > Cc: José Roberto de Souza <jose.souza@intel.com>
> > > > > > > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > > > > > > ---
> > > > > > > >  .../drm/i915/display/intel_display_types.h    |  2 +
> > > > > > > >  drivers/gpu/drm/i915/display/intel_tc.c       | 63 +++++++++++--------
> > > > > > > >  2 files changed, 39 insertions(+), 26 deletions(-)
> > > > > > > > 
> > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
> > > > > > > > index e9e806d90eec4..08a73ffded957 100644
> > > > > > > > --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> > > > > > > > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> > > > > > > > @@ -1663,8 +1663,10 @@ struct intel_digital_port {
> > > > > > > >  	enum intel_display_power_domain ddi_io_power_domain;
> > > > > > > >  	intel_wakeref_t ddi_io_wakeref;
> > > > > > > >  	intel_wakeref_t aux_wakeref;
> > > > > > > > +
> > > > > > > >  	struct mutex tc_lock;	/* protects the TypeC port mode */
> > > > > > > >  	intel_wakeref_t tc_lock_wakeref;
> > > > > > > > +	enum intel_display_power_domain tc_lock_power_domain;
> > > > > > > >  	int tc_link_refcount;
> > > > > > > >  	bool tc_legacy_port:1;
> > > > > > > >  	char tc_port_name[8];
> > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_tc.c b/drivers/gpu/drm/i915/display/intel_tc.c
> > > > > > > > index 77b16a7c43466..24d2dc2e19a7d 100644
> > > > > > > > --- a/drivers/gpu/drm/i915/display/intel_tc.c
> > > > > > > > +++ b/drivers/gpu/drm/i915/display/intel_tc.c
> > > > > > > > @@ -48,8 +48,16 @@ bool intel_tc_port_in_legacy_mode(struct intel_digital_port *dig_port)
> > > > > > > >  	return intel_tc_port_in_mode(dig_port, TC_PORT_LEGACY);
> > > > > > > >  }
> > > > > > > >  
> > > > > > > > +bool intel_tc_cold_requires_aux_pw(struct intel_digital_port *dig_port)
> > > > > > > > +{
> > > > > > > > +	struct drm_i915_private *i915 = to_i915(dig_port->base.base.dev);
> > > > > > > > +
> > > > > > > > +	return (DISPLAY_VER(i915) == 11 && dig_port->tc_legacy_port) ||
> > > > > > > > +		IS_ALDERLAKE_P(i915);
> > > > > > > > +}
> > > > > > > > +
> > > > > > > >  static enum intel_display_power_domain
> > > > > > > > -tc_cold_get_power_domain(struct intel_digital_port *dig_port)
> > > > > > > > +tc_cold_get_power_domain(struct intel_digital_port *dig_port, enum tc_port_mode mode)
> > > > > > > >  {
> > > > > > > >  	if (intel_tc_cold_requires_aux_pw(dig_port))
> > > > > > > >  		return intel_legacy_aux_to_power_domain(dig_port->aux_ch);
> > > > > > > > @@ -58,23 +66,30 @@ tc_cold_get_power_domain(struct intel_digital_port *dig_port)
> > > > > > > >  }
> > > > > > > >  
> > > > > > > >  static intel_wakeref_t
> > > > > > > > -tc_cold_block(struct intel_digital_port *dig_port)
> > > > > > > > +tc_cold_block_in_mode(struct intel_digital_port *dig_port, enum tc_port_mode mode,
> > > > > > > > +		      enum intel_display_power_domain *domain)
> > > > > > > >  {
> > > > > > > >  	struct drm_i915_private *i915 = to_i915(dig_port->base.base.dev);
> > > > > > > > -	enum intel_display_power_domain domain;
> > > > > > > >  
> > > > > > > >  	if (DISPLAY_VER(i915) == 11 && !dig_port->tc_legacy_port)
> > > > > > > >  		return 0;
> > > > > > > >  
> > > > > > > > -	domain = tc_cold_get_power_domain(dig_port);
> > > > > > > > -	return intel_display_power_get(i915, domain);
> > > > > > > > +	*domain = tc_cold_get_power_domain(dig_port, mode);
> > > > > > > 
> > > > > > > I see problems with this approach.
> > > > > > > 
> > > > > > > If there is a TC alt-mode is connected and current software state is
> > > > > > > TBT or disconnected it will not get the power domain to exit TC cold,
> > > > > > > what could case invalid reads of registers.
> > > > > > 
> > > > > > If the mode needs to be changed, like in the above cases, the power
> > > > > > domain required to exit TC cold will be taken.
> > > > > 
> > > > > How?
> > > > > I see at least this 2 problematic cases.
> > > > > 
> > > > > intel_tc_port_sanitize()
> > > > > 	tc_cold_block(mode=TBT)
> > > > 
> > > > It's in disconnected mode at this point.
> > > 
> > > Okay.
> > > 
> > > > 
> > > > > 
> > > > > 	dig_port->tc_mode = intel_tc_port_get_current_mode(dig_port);
> > > > > 	...
> > > > > 
> > > > > 	tc_cold_unblock(dig_port, domain, tc_cold_wref);
> > > > > 
> > > > > Other case
> > > > > 
> > > > > intel_tc_port_connected()
> > > > > 
> > > > > 	tc_cold_block(mode=TBT)
> > > > 
> > > > Reading PORT_TX_DFLEXDPSP while in TC-cold returns 0xffffffff, and it's
> > > > handled when reading it (which is also required by the spec).
> > > 
> > > Will only follow adl-p flow for this
> > > example, https://pastebin.com/raw/TVXdK1Et (pastebin just to make sure
> > > indentation is not lost between email clients)
> > 
> > TCSS_DDI_STATUS is in IOM (so neither a FIA or a PHY register). From Bspec 55480:
> > """
> > Type-C Flow Registers Used by Display Software
> > ...
> > Location          Register
> > IOM               TCSS_DDI_STATUS
> > """
> > 
> > and IOM registers are accessible in TC-cold:
> > 
> > """
> > TCCOLD
> > The type-C subsystem can enter the TCCOLD power saving state where FIA and type-C
> > PHY registers become inaccessible and respond to reads with all 1s data,
> > while IOM remains accessible. 
> 
> Ohh okay, maybe add some comments about the above.

Ok, can add something to adl_tc_port_live_status_mask().

> For TGL taking POWER_DOMAIN_TC_COLD_OFF will solve everything.
> 
> What about ICL in legacy mode, when going from TC_PORT_TBT_ALT to
> TC_PORT_LEGACY.  Reading PORT_TX_DFLEXDPSP in icl_tc_port_live_status_mask()
> will set mask to BIT(TC_PORT_TBT_ALT) | BIT(TC_PORT_DP_ALT) | BIT(TC_PORT_LEGACY).

Yes on ICL, we don't have any way to block TC-cold in TBT mode (on
systems with TBT/DP-alt ports, there is no TC-cold blocking PUNIT
command in the firmwares). Hence we compare the reads against all 1s. 

In any case normally it's not possible to switch from TBT/DP-alt to
legacy, since legacy is fixed/hardwired.

> That would not be a problem if SDEISR bit for TC port is always set
> when in legacy mode.  Do you know what is the behavior here?

SDEISR is in the south display, so shouldn't be affected by TC-cold.

We could check this bit even if DFLEXDPSP returns all 1s, to account for
incorrect VBTs (and let tc_port_fixup_legacy_flag() fix things up). But
that would be for a follow-up since it's not handled in the current code
either.

> The handling for going from TC_PORT_DISCONNECTED to TC_PORT_LEGACY is properly handled.
> 
> 
> > """
> > 
> > > > > 		       BIT(dig_port->tc_mode);
> > > > > 
> > > > > 	tc_cold_unblock(dig_port, domain, tc_cold_wref);
> > > > > 
> > > > > 
> > > > > > 
> > > > > > > From the next patch commit message: 'For the ADL-P TBT mode the spec
> > > > > > > doesn't require blocking TC-cold by using the legacy AUX power
> > > > > > > domain'.
> > > > > > > 
> > > > > > > It is not required for TBT but when there is nothing connected,
> > > > > > > hardware is not in TBT mode so it can still get into TC cold.
> > > > > > 
> > > > > > If there is nothing connected the required power domain will be taken to
> > > > > > exit TC cold.
> > > > > > 
> > > > > > > > +
> > > > > > > > +	return intel_display_power_get(i915, *domain);
> > > > > > > > +}
> > > > > > > > +
> > > > > > > > +static intel_wakeref_t
> > > > > > > > +tc_cold_block(struct intel_digital_port *dig_port, enum intel_display_power_domain *domain)
> > > > > > > > +{
> > > > > > > > +	return tc_cold_block_in_mode(dig_port, dig_port->tc_mode, domain);
> > > > > > > >  }
> > > > > > > >  
> > > > > > > >  static void
> > > > > > > > -tc_cold_unblock(struct intel_digital_port *dig_port, intel_wakeref_t wakeref)
> > > > > > > > +tc_cold_unblock(struct intel_digital_port *dig_port, enum intel_display_power_domain domain,
> > > > > > > > +		intel_wakeref_t wakeref)
> > > > > > > >  {
> > > > > > > >  	struct drm_i915_private *i915 = to_i915(dig_port->base.base.dev);
> > > > > > > > -	enum intel_display_power_domain domain;
> > > > > > > >  
> > > > > > > >  	/*
> > > > > > > >  	 * wakeref == -1, means some error happened saving save_depot_stack but
> > > > > > > > @@ -84,8 +99,7 @@ tc_cold_unblock(struct intel_digital_port *dig_port, intel_wakeref_t wakeref)
> > > > > > > >  	if (wakeref == 0)
> > > > > > > >  		return;
> > > > > > > >  
> > > > > > > > -	domain = tc_cold_get_power_domain(dig_port);
> > > > > > > > -	intel_display_power_put_async(i915, domain, wakeref);
> > > > > > > > +	intel_display_power_put(i915, domain, wakeref);
> > > > > > > >  }
> > > > > > > >  
> > > > > > > >  static void
> > > > > > > > @@ -98,7 +112,8 @@ assert_tc_cold_blocked(struct intel_digital_port *dig_port)
> > > > > > > >  		return;
> > > > > > > >  
> > > > > > > >  	enabled = intel_display_power_is_enabled(i915,
> > > > > > > > -						 tc_cold_get_power_domain(dig_port));
> > > > > > > > +						 tc_cold_get_power_domain(dig_port,
> > > > > > > > +									  dig_port->tc_mode));
> > > > > > > >  	drm_WARN_ON(&i915->drm, !enabled);
> > > > > > > >  }
> > > > > > > >  
> > > > > > > > @@ -634,7 +649,8 @@ void intel_tc_port_sanitize(struct intel_digital_port *dig_port)
> > > > > > > >  
> > > > > > > >  	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);
> > > > > > > > +		enum intel_display_power_domain domain;
> > > > > > > > +		intel_wakeref_t tc_cold_wref = tc_cold_block(dig_port, &domain);
> > > > > > > >  
> > > > > > > >  		dig_port->tc_mode = intel_tc_port_get_current_mode(dig_port);
> > > > > > > >  
> > > > > > > > @@ -644,7 +660,7 @@ void intel_tc_port_sanitize(struct intel_digital_port *dig_port)
> > > > > > > >  				    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);
> > > > > > > > +		tc_cold_unblock(dig_port, domain, tc_cold_wref);
> > > > > > > >  	}
> > > > > > > >  
> > > > > > > >  	drm_dbg_kms(&i915->drm, "Port %s: sanitize mode (%s)\n",
> > > > > > > > @@ -673,15 +689,16 @@ bool intel_tc_port_connected(struct intel_encoder *encoder)
> > > > > > > >  {
> > > > > > > >  	struct intel_digital_port *dig_port = enc_to_dig_port(encoder);
> > > > > > > >  	bool is_connected;
> > > > > > > > +	enum intel_display_power_domain domain;
> > > > > > > >  	intel_wakeref_t tc_cold_wref;
> > > > > > > >  
> > > > > > > >  	intel_tc_port_lock(dig_port);
> > > > > > > > -	tc_cold_wref = tc_cold_block(dig_port);
> > > > > > > > +	tc_cold_wref = tc_cold_block(dig_port, &domain);
> > > > > > > >  
> > > > > > > >  	is_connected = tc_port_live_status_mask(dig_port) &
> > > > > > > >  		       BIT(dig_port->tc_mode);
> > > > > > > >  
> > > > > > > > -	tc_cold_unblock(dig_port, tc_cold_wref);
> > > > > > > > +	tc_cold_unblock(dig_port, domain, tc_cold_wref);
> > > > > > > >  	intel_tc_port_unlock(dig_port);
> > > > > > > >  
> > > > > > > >  	return is_connected;
> > > > > > > > @@ -698,15 +715,16 @@ static void __intel_tc_port_lock(struct intel_digital_port *dig_port,
> > > > > > > >  	mutex_lock(&dig_port->tc_lock);
> > > > > > > >  
> > > > > > > >  	if (!dig_port->tc_link_refcount) {
> > > > > > > > +		enum intel_display_power_domain domain;
> > > > > > > >  		intel_wakeref_t tc_cold_wref;
> > > > > > > >  
> > > > > > > > -		tc_cold_wref = tc_cold_block(dig_port);
> > > > > > > > +		tc_cold_wref = tc_cold_block(dig_port, &domain);
> > > > > > > >  
> > > > > > > >  		if (force_disconnect || intel_tc_port_needs_reset(dig_port))
> > > > > > > >  			intel_tc_port_reset_mode(dig_port, required_lanes,
> > > > > > > >  						 force_disconnect);
> > > > > > > >  
> > > > > > > > -		tc_cold_unblock(dig_port, tc_cold_wref);
> > > > > > > > +		tc_cold_unblock(dig_port, domain, tc_cold_wref);
> > > > > > > >  	}
> > > > > > > >  
> > > > > > > >  	drm_WARN_ON(&i915->drm, dig_port->tc_mode == TC_PORT_DISCONNECTED);
> > > > > > > > @@ -775,6 +793,7 @@ void intel_tc_port_put_link(struct intel_digital_port *dig_port)
> > > > > > > >  static bool
> > > > > > > >  tc_has_modular_fia(struct drm_i915_private *i915, struct intel_digital_port *dig_port)
> > > > > > > >  {
> > > > > > > > +	enum intel_display_power_domain domain;
> > > > > > > >  	intel_wakeref_t wakeref;
> > > > > > > >  	u32 val;
> > > > > > > >  
> > > > > > > > @@ -782,9 +801,9 @@ tc_has_modular_fia(struct drm_i915_private *i915, struct intel_digital_port *dig
> > > > > > > >  		return false;
> > > > > > > >  
> > > > > > > >  	mutex_lock(&dig_port->tc_lock);
> > > > > > > > -	wakeref = tc_cold_block(dig_port);
> > > > > > > > +	wakeref = tc_cold_block(dig_port, &domain);
> > > > > > > >  	val = intel_uncore_read(&i915->uncore, PORT_TX_DFLEXDPSP(FIA1));
> > > > > > > > -	tc_cold_unblock(dig_port, wakeref);
> > > > > > > > +	tc_cold_unblock(dig_port, domain, wakeref);
> > > > > > > >  	mutex_unlock(&dig_port->tc_lock);
> > > > > > > >  
> > > > > > > >  	drm_WARN_ON(&i915->drm, val == 0xffffffff);
> > > > > > > > @@ -829,11 +848,3 @@ void intel_tc_port_init(struct intel_digital_port *dig_port, bool is_legacy)
> > > > > > > >  	dig_port->tc_link_refcount = 0;
> > > > > > > >  	tc_port_load_fia_params(i915, dig_port);
> > > > > > > >  }
> > > > > > > > -
> > > > > > > > -bool intel_tc_cold_requires_aux_pw(struct intel_digital_port *dig_port)
> > > > > > > > -{
> > > > > > > > -	struct drm_i915_private *i915 = to_i915(dig_port->base.base.dev);
> > > > > > > > -
> > > > > > > > -	return (DISPLAY_VER(i915) == 11 && dig_port->tc_legacy_port) ||
> > > > > > > > -		IS_ALDERLAKE_P(i915);
> > > > > > > > -}
> > > > > > > 
> > > > > 
> > > 
>

Souza, Jose Sept. 28, 2021, 1:03 a.m. UTC | #9

On Tue, 2021-09-28 at 03:45 +0300, Imre Deak wrote:
> On Tue, Sep 28, 2021 at 03:14:45AM +0300, Souza, Jose wrote:
> > On Tue, 2021-09-28 at 02:51 +0300, Imre Deak wrote:
> > > On Tue, Sep 28, 2021 at 02:33:27AM +0300, Souza, Jose wrote:
> > > > On Tue, 2021-09-28 at 01:28 +0300, Imre Deak wrote:
> > > > > On Tue, Sep 28, 2021 at 01:21:21AM +0300, Souza, Jose wrote:
> > > > > > On Tue, 2021-09-28 at 01:13 +0300, Imre Deak wrote:
> > > > > > > On Tue, Sep 28, 2021 at 12:56:24AM +0300, Souza, Jose wrote:
> > > > > > > > On Tue, 2021-09-21 at 03:23 +0300, Imre Deak wrote:
> > > > > > > > > A follow-up change will select the TC-cold blocking power domain based
> > > > > > > > > on the TypeC mode, prepare for that here.
> > > > > > > > > 
> > > > > > > > > Also bring intel_tc_cold_requires_aux_pw() earlier to its logical place
> > > > > > > > > for readability.
> > > > > > > > > 
> > > > > > > > > No functional change.
> > > > > > > > > 
> > > > > > > > > Cc: José Roberto de Souza <jose.souza@intel.com>
> > > > > > > > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > > > > > > > ---
> > > > > > > > >  .../drm/i915/display/intel_display_types.h    |  2 +
> > > > > > > > >  drivers/gpu/drm/i915/display/intel_tc.c       | 63 +++++++++++--------
> > > > > > > > >  2 files changed, 39 insertions(+), 26 deletions(-)
> > > > > > > > > 
> > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
> > > > > > > > > index e9e806d90eec4..08a73ffded957 100644
> > > > > > > > > --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> > > > > > > > > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> > > > > > > > > @@ -1663,8 +1663,10 @@ struct intel_digital_port {
> > > > > > > > >  	enum intel_display_power_domain ddi_io_power_domain;
> > > > > > > > >  	intel_wakeref_t ddi_io_wakeref;
> > > > > > > > >  	intel_wakeref_t aux_wakeref;
> > > > > > > > > +
> > > > > > > > >  	struct mutex tc_lock;	/* protects the TypeC port mode */
> > > > > > > > >  	intel_wakeref_t tc_lock_wakeref;
> > > > > > > > > +	enum intel_display_power_domain tc_lock_power_domain;
> > > > > > > > >  	int tc_link_refcount;
> > > > > > > > >  	bool tc_legacy_port:1;
> > > > > > > > >  	char tc_port_name[8];
> > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_tc.c b/drivers/gpu/drm/i915/display/intel_tc.c
> > > > > > > > > index 77b16a7c43466..24d2dc2e19a7d 100644
> > > > > > > > > --- a/drivers/gpu/drm/i915/display/intel_tc.c
> > > > > > > > > +++ b/drivers/gpu/drm/i915/display/intel_tc.c
> > > > > > > > > @@ -48,8 +48,16 @@ bool intel_tc_port_in_legacy_mode(struct intel_digital_port *dig_port)
> > > > > > > > >  	return intel_tc_port_in_mode(dig_port, TC_PORT_LEGACY);
> > > > > > > > >  }
> > > > > > > > >  
> > > > > > > > > +bool intel_tc_cold_requires_aux_pw(struct intel_digital_port *dig_port)
> > > > > > > > > +{
> > > > > > > > > +	struct drm_i915_private *i915 = to_i915(dig_port->base.base.dev);
> > > > > > > > > +
> > > > > > > > > +	return (DISPLAY_VER(i915) == 11 && dig_port->tc_legacy_port) ||
> > > > > > > > > +		IS_ALDERLAKE_P(i915);
> > > > > > > > > +}
> > > > > > > > > +
> > > > > > > > >  static enum intel_display_power_domain
> > > > > > > > > -tc_cold_get_power_domain(struct intel_digital_port *dig_port)
> > > > > > > > > +tc_cold_get_power_domain(struct intel_digital_port *dig_port, enum tc_port_mode mode)
> > > > > > > > >  {
> > > > > > > > >  	if (intel_tc_cold_requires_aux_pw(dig_port))
> > > > > > > > >  		return intel_legacy_aux_to_power_domain(dig_port->aux_ch);
> > > > > > > > > @@ -58,23 +66,30 @@ tc_cold_get_power_domain(struct intel_digital_port *dig_port)
> > > > > > > > >  }
> > > > > > > > >  
> > > > > > > > >  static intel_wakeref_t
> > > > > > > > > -tc_cold_block(struct intel_digital_port *dig_port)
> > > > > > > > > +tc_cold_block_in_mode(struct intel_digital_port *dig_port, enum tc_port_mode mode,
> > > > > > > > > +		      enum intel_display_power_domain *domain)
> > > > > > > > >  {
> > > > > > > > >  	struct drm_i915_private *i915 = to_i915(dig_port->base.base.dev);
> > > > > > > > > -	enum intel_display_power_domain domain;
> > > > > > > > >  
> > > > > > > > >  	if (DISPLAY_VER(i915) == 11 && !dig_port->tc_legacy_port)
> > > > > > > > >  		return 0;
> > > > > > > > >  
> > > > > > > > > -	domain = tc_cold_get_power_domain(dig_port);
> > > > > > > > > -	return intel_display_power_get(i915, domain);
> > > > > > > > > +	*domain = tc_cold_get_power_domain(dig_port, mode);
> > > > > > > > 
> > > > > > > > I see problems with this approach.
> > > > > > > > 
> > > > > > > > If there is a TC alt-mode is connected and current software state is
> > > > > > > > TBT or disconnected it will not get the power domain to exit TC cold,
> > > > > > > > what could case invalid reads of registers.
> > > > > > > 
> > > > > > > If the mode needs to be changed, like in the above cases, the power
> > > > > > > domain required to exit TC cold will be taken.
> > > > > > 
> > > > > > How?
> > > > > > I see at least this 2 problematic cases.
> > > > > > 
> > > > > > intel_tc_port_sanitize()
> > > > > > 	tc_cold_block(mode=TBT)
> > > > > 
> > > > > It's in disconnected mode at this point.
> > > > 
> > > > Okay.
> > > > 
> > > > > 
> > > > > > 
> > > > > > 	dig_port->tc_mode = intel_tc_port_get_current_mode(dig_port);
> > > > > > 	...
> > > > > > 
> > > > > > 	tc_cold_unblock(dig_port, domain, tc_cold_wref);
> > > > > > 
> > > > > > Other case
> > > > > > 
> > > > > > intel_tc_port_connected()
> > > > > > 
> > > > > > 	tc_cold_block(mode=TBT)
> > > > > 
> > > > > Reading PORT_TX_DFLEXDPSP while in TC-cold returns 0xffffffff, and it's
> > > > > handled when reading it (which is also required by the spec).
> > > > 
> > > > Will only follow adl-p flow for this
> > > > example, https://pastebin.com/raw/TVXdK1Et (pastebin just to make sure
> > > > indentation is not lost between email clients)
> > > 
> > > TCSS_DDI_STATUS is in IOM (so neither a FIA or a PHY register). From Bspec 55480:
> > > """
> > > Type-C Flow Registers Used by Display Software
> > > ...
> > > Location          Register
> > > IOM               TCSS_DDI_STATUS
> > > """
> > > 
> > > and IOM registers are accessible in TC-cold:
> > > 
> > > """
> > > TCCOLD
> > > The type-C subsystem can enter the TCCOLD power saving state where FIA and type-C
> > > PHY registers become inaccessible and respond to reads with all 1s data,
> > > while IOM remains accessible. 
> > 
> > Ohh okay, maybe add some comments about the above.
> 
> Ok, can add something to adl_tc_port_live_status_mask().
> 
> > For TGL taking POWER_DOMAIN_TC_COLD_OFF will solve everything.
> > 
> > What about ICL in legacy mode, when going from TC_PORT_TBT_ALT to
> > TC_PORT_LEGACY.  Reading PORT_TX_DFLEXDPSP in icl_tc_port_live_status_mask()
> > will set mask to BIT(TC_PORT_TBT_ALT) | BIT(TC_PORT_DP_ALT) | BIT(TC_PORT_LEGACY).
> 
> Yes on ICL, we don't have any way to block TC-cold in TBT mode (on
> systems with TBT/DP-alt ports, there is no TC-cold blocking PUNIT
> command in the firmwares). Hence we compare the reads against all 1s. 
> 
> In any case normally it's not possible to switch from TBT/DP-alt to
> legacy, since legacy is fixed/hardwired.

Okay, with comment in adl_tc_port_live_status_mask().
Reviewed-by: José Roberto de Souza <jose.souza@intel.com>

> 
> > That would not be a problem if SDEISR bit for TC port is always set
> > when in legacy mode.  Do you know what is the behavior here?
> 
> SDEISR is in the south display, so shouldn't be affected by TC-cold.
> 
> We could check this bit even if DFLEXDPSP returns all 1s, to account for
> incorrect VBTs (and let tc_port_fixup_legacy_flag() fix things up). But
> that would be for a follow-up since it's not handled in the current code
> either.
> 
> > The handling for going from TC_PORT_DISCONNECTED to TC_PORT_LEGACY is properly handled.
> > 
> > 
> > > """
> > > 
> > > > > > 		       BIT(dig_port->tc_mode);
> > > > > > 
> > > > > > 	tc_cold_unblock(dig_port, domain, tc_cold_wref);
> > > > > > 
> > > > > > 
> > > > > > > 
> > > > > > > > From the next patch commit message: 'For the ADL-P TBT mode the spec
> > > > > > > > doesn't require blocking TC-cold by using the legacy AUX power
> > > > > > > > domain'.
> > > > > > > > 
> > > > > > > > It is not required for TBT but when there is nothing connected,
> > > > > > > > hardware is not in TBT mode so it can still get into TC cold.
> > > > > > > 
> > > > > > > If there is nothing connected the required power domain will be taken to
> > > > > > > exit TC cold.
> > > > > > > 
> > > > > > > > > +
> > > > > > > > > +	return intel_display_power_get(i915, *domain);
> > > > > > > > > +}
> > > > > > > > > +
> > > > > > > > > +static intel_wakeref_t
> > > > > > > > > +tc_cold_block(struct intel_digital_port *dig_port, enum intel_display_power_domain *domain)
> > > > > > > > > +{
> > > > > > > > > +	return tc_cold_block_in_mode(dig_port, dig_port->tc_mode, domain);
> > > > > > > > >  }
> > > > > > > > >  
> > > > > > > > >  static void
> > > > > > > > > -tc_cold_unblock(struct intel_digital_port *dig_port, intel_wakeref_t wakeref)
> > > > > > > > > +tc_cold_unblock(struct intel_digital_port *dig_port, enum intel_display_power_domain domain,
> > > > > > > > > +		intel_wakeref_t wakeref)
> > > > > > > > >  {
> > > > > > > > >  	struct drm_i915_private *i915 = to_i915(dig_port->base.base.dev);
> > > > > > > > > -	enum intel_display_power_domain domain;
> > > > > > > > >  
> > > > > > > > >  	/*
> > > > > > > > >  	 * wakeref == -1, means some error happened saving save_depot_stack but
> > > > > > > > > @@ -84,8 +99,7 @@ tc_cold_unblock(struct intel_digital_port *dig_port, intel_wakeref_t wakeref)
> > > > > > > > >  	if (wakeref == 0)
> > > > > > > > >  		return;
> > > > > > > > >  
> > > > > > > > > -	domain = tc_cold_get_power_domain(dig_port);
> > > > > > > > > -	intel_display_power_put_async(i915, domain, wakeref);
> > > > > > > > > +	intel_display_power_put(i915, domain, wakeref);
> > > > > > > > >  }
> > > > > > > > >  
> > > > > > > > >  static void
> > > > > > > > > @@ -98,7 +112,8 @@ assert_tc_cold_blocked(struct intel_digital_port *dig_port)
> > > > > > > > >  		return;
> > > > > > > > >  
> > > > > > > > >  	enabled = intel_display_power_is_enabled(i915,
> > > > > > > > > -						 tc_cold_get_power_domain(dig_port));
> > > > > > > > > +						 tc_cold_get_power_domain(dig_port,
> > > > > > > > > +									  dig_port->tc_mode));
> > > > > > > > >  	drm_WARN_ON(&i915->drm, !enabled);
> > > > > > > > >  }
> > > > > > > > >  
> > > > > > > > > @@ -634,7 +649,8 @@ void intel_tc_port_sanitize(struct intel_digital_port *dig_port)
> > > > > > > > >  
> > > > > > > > >  	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);
> > > > > > > > > +		enum intel_display_power_domain domain;
> > > > > > > > > +		intel_wakeref_t tc_cold_wref = tc_cold_block(dig_port, &domain);
> > > > > > > > >  
> > > > > > > > >  		dig_port->tc_mode = intel_tc_port_get_current_mode(dig_port);
> > > > > > > > >  
> > > > > > > > > @@ -644,7 +660,7 @@ void intel_tc_port_sanitize(struct intel_digital_port *dig_port)
> > > > > > > > >  				    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);
> > > > > > > > > +		tc_cold_unblock(dig_port, domain, tc_cold_wref);
> > > > > > > > >  	}
> > > > > > > > >  
> > > > > > > > >  	drm_dbg_kms(&i915->drm, "Port %s: sanitize mode (%s)\n",
> > > > > > > > > @@ -673,15 +689,16 @@ bool intel_tc_port_connected(struct intel_encoder *encoder)
> > > > > > > > >  {
> > > > > > > > >  	struct intel_digital_port *dig_port = enc_to_dig_port(encoder);
> > > > > > > > >  	bool is_connected;
> > > > > > > > > +	enum intel_display_power_domain domain;
> > > > > > > > >  	intel_wakeref_t tc_cold_wref;
> > > > > > > > >  
> > > > > > > > >  	intel_tc_port_lock(dig_port);
> > > > > > > > > -	tc_cold_wref = tc_cold_block(dig_port);
> > > > > > > > > +	tc_cold_wref = tc_cold_block(dig_port, &domain);
> > > > > > > > >  
> > > > > > > > >  	is_connected = tc_port_live_status_mask(dig_port) &
> > > > > > > > >  		       BIT(dig_port->tc_mode);
> > > > > > > > >  
> > > > > > > > > -	tc_cold_unblock(dig_port, tc_cold_wref);
> > > > > > > > > +	tc_cold_unblock(dig_port, domain, tc_cold_wref);
> > > > > > > > >  	intel_tc_port_unlock(dig_port);
> > > > > > > > >  
> > > > > > > > >  	return is_connected;
> > > > > > > > > @@ -698,15 +715,16 @@ static void __intel_tc_port_lock(struct intel_digital_port *dig_port,
> > > > > > > > >  	mutex_lock(&dig_port->tc_lock);
> > > > > > > > >  
> > > > > > > > >  	if (!dig_port->tc_link_refcount) {
> > > > > > > > > +		enum intel_display_power_domain domain;
> > > > > > > > >  		intel_wakeref_t tc_cold_wref;
> > > > > > > > >  
> > > > > > > > > -		tc_cold_wref = tc_cold_block(dig_port);
> > > > > > > > > +		tc_cold_wref = tc_cold_block(dig_port, &domain);
> > > > > > > > >  
> > > > > > > > >  		if (force_disconnect || intel_tc_port_needs_reset(dig_port))
> > > > > > > > >  			intel_tc_port_reset_mode(dig_port, required_lanes,
> > > > > > > > >  						 force_disconnect);
> > > > > > > > >  
> > > > > > > > > -		tc_cold_unblock(dig_port, tc_cold_wref);
> > > > > > > > > +		tc_cold_unblock(dig_port, domain, tc_cold_wref);
> > > > > > > > >  	}
> > > > > > > > >  
> > > > > > > > >  	drm_WARN_ON(&i915->drm, dig_port->tc_mode == TC_PORT_DISCONNECTED);
> > > > > > > > > @@ -775,6 +793,7 @@ void intel_tc_port_put_link(struct intel_digital_port *dig_port)
> > > > > > > > >  static bool
> > > > > > > > >  tc_has_modular_fia(struct drm_i915_private *i915, struct intel_digital_port *dig_port)
> > > > > > > > >  {
> > > > > > > > > +	enum intel_display_power_domain domain;
> > > > > > > > >  	intel_wakeref_t wakeref;
> > > > > > > > >  	u32 val;
> > > > > > > > >  
> > > > > > > > > @@ -782,9 +801,9 @@ tc_has_modular_fia(struct drm_i915_private *i915, struct intel_digital_port *dig
> > > > > > > > >  		return false;
> > > > > > > > >  
> > > > > > > > >  	mutex_lock(&dig_port->tc_lock);
> > > > > > > > > -	wakeref = tc_cold_block(dig_port);
> > > > > > > > > +	wakeref = tc_cold_block(dig_port, &domain);
> > > > > > > > >  	val = intel_uncore_read(&i915->uncore, PORT_TX_DFLEXDPSP(FIA1));
> > > > > > > > > -	tc_cold_unblock(dig_port, wakeref);
> > > > > > > > > +	tc_cold_unblock(dig_port, domain, wakeref);
> > > > > > > > >  	mutex_unlock(&dig_port->tc_lock);
> > > > > > > > >  
> > > > > > > > >  	drm_WARN_ON(&i915->drm, val == 0xffffffff);
> > > > > > > > > @@ -829,11 +848,3 @@ void intel_tc_port_init(struct intel_digital_port *dig_port, bool is_legacy)
> > > > > > > > >  	dig_port->tc_link_refcount = 0;
> > > > > > > > >  	tc_port_load_fia_params(i915, dig_port);
> > > > > > > > >  }
> > > > > > > > > -
> > > > > > > > > -bool intel_tc_cold_requires_aux_pw(struct intel_digital_port *dig_port)
> > > > > > > > > -{
> > > > > > > > > -	struct drm_i915_private *i915 = to_i915(dig_port->base.base.dev);
> > > > > > > > > -
> > > > > > > > > -	return (DISPLAY_VER(i915) == 11 && dig_port->tc_legacy_port) ||
> > > > > > > > > -		IS_ALDERLAKE_P(i915);
> > > > > > > > > -}
> > > > > > > > 
> > > > > > 
> > > > 
> >

[08/13] drm/i915/tc: Refactor TC-cold block/unblock helpers

Commit Message

Comments

Patch