Re: [Intel-gfx] [PATCH v4 6/9] drm/i915/tc/tgl: Implement TC cold sequences

[Imre Deak <imre.deak@intel.com>]

On Tue, Apr 14, 2020 at 08:41:50PM +0300, Souza, Jose wrote:
> On Tue, 2020-04-14 at 20:39 +0300, Imre Deak wrote:
> > On Mon, Apr 13, 2020 at 09:45:12AM -0700, José Roberto de Souza
> > wrote:
> > > TC ports can enter in TCCOLD to save power and is required to
> > > request
> > > to PCODE to exit this state before use or read to TC registers.
> > > 
> > > For TGL there is a new MBOX command to do that with a parameter to
> > > ask
> > > PCODE to exit and block TCCOLD entry or unblock TCCOLD entry.
> > > 
> > > So adding a new power domain to reuse the refcount and only allow
> > > TC cold when all TC ports are not in use.
> > > 
> > > v2:
> > > - fixed missing case in intel_display_power_domain_str()
> > > - moved tgl_tc_cold_request to intel_display_power.c
> > > - renamed TGL_TC_COLD_OFF to TGL_TC_COLD_OFF_POWER_DOMAINS
> > > - added all TC and TBT aux power domains to
> > > TGL_TC_COLD_OFF_POWER_DOMAINS
> > > 
> > > v3:
> > > - added one msec sleep when PCODE returns -EAGAIN
> > > - added timeout of 5msec to not loop forever if
> > > sandybridge_pcode_write_timeout() keeps returning -EAGAIN
> > > 
> > > v4:
> > > - Made failure to block or unblock TC cold a error
> > > - removed 5msec timeout, intead giving PCODE 1msec by up 3 times to
> > > recover from the internal error
> > > 
> > > BSpec: 49294
> > > Cc: Imre Deak <imre.deak@intel.com>
> > > Cc: Cooper Chiou <cooper.chiou@intel.com>
> > > Cc: Kai-Heng Feng <kai.heng.feng@canonical.com>
> > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > ---
> > >  .../drm/i915/display/intel_display_power.c    | 107
> > > ++++++++++++++++++
> > >  .../drm/i915/display/intel_display_power.h    |   1 +
> > >  drivers/gpu/drm/i915/display/intel_tc.c       |  17 ++-
> > >  drivers/gpu/drm/i915/i915_reg.h               |   4 +
> > >  4 files changed, 126 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c
> > > b/drivers/gpu/drm/i915/display/intel_display_power.c
> > > index 50bed2d1dd13..00de926aaccf 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display_power.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_display_power.c
> > > @@ -151,6 +151,8 @@ intel_display_power_domain_str(enum
> > > intel_display_power_domain domain)
> > >  		return "GT_IRQ";
> > >  	case POWER_DOMAIN_DPLL_DC_OFF:
> > >  		return "DPLL_DC_OFF";
> > > +	case POWER_DOMAIN_TC_COLD_OFF:
> > > +		return "TC_COLD_OFF";
> > >  	default:
> > >  		MISSING_CASE(domain);
> > >  		return "?";
> > > @@ -2861,6 +2863,21 @@ void intel_display_power_put(struct
> > > drm_i915_private *dev_priv,
> > >  #define TGL_AUX_I_TBT6_IO_POWER_DOMAINS (	\
> > >  	BIT_ULL(POWER_DOMAIN_AUX_I_TBT))
> > >  
> > > +#define TGL_TC_COLD_OFF_POWER_DOMAINS (		\
> > > +	BIT_ULL(POWER_DOMAIN_AUX_D)	|	\
> > > +	BIT_ULL(POWER_DOMAIN_AUX_E)	|	\
> > > +	BIT_ULL(POWER_DOMAIN_AUX_F)	|	\
> > > +	BIT_ULL(POWER_DOMAIN_AUX_G)	|	\
> > > +	BIT_ULL(POWER_DOMAIN_AUX_H)	|	\
> > > +	BIT_ULL(POWER_DOMAIN_AUX_I)	|	\
> > > +	BIT_ULL(POWER_DOMAIN_AUX_D_TBT)	|	\
> > > +	BIT_ULL(POWER_DOMAIN_AUX_E_TBT)	|	\
> > > +	BIT_ULL(POWER_DOMAIN_AUX_F_TBT)	|	\
> > > +	BIT_ULL(POWER_DOMAIN_AUX_G_TBT)	|	\
> > > +	BIT_ULL(POWER_DOMAIN_AUX_H_TBT)	|	\
> > > +	BIT_ULL(POWER_DOMAIN_AUX_I_TBT)	|	\
> > > +	BIT_ULL(POWER_DOMAIN_TC_COLD_OFF))
> > > +
> > >  static const struct i915_power_well_ops
> > > i9xx_always_on_power_well_ops = {
> > >  	.sync_hw = i9xx_power_well_sync_hw_noop,
> > >  	.enable = i9xx_always_on_power_well_noop,
> > > @@ -3963,6 +3980,90 @@ static const struct i915_power_well_desc
> > > ehl_power_wells[] = {
> > >  	},
> > >  };
> > >  
> > > +static void
> > > +tgl_tc_cold_request(struct drm_i915_private *i915, bool block)
> > > +{
> > > +	u8 tries = 0;
> > > +	int ret;
> > > +
> > > +	while (1) {
> > > +		u32 low_val = 0, high_val;
> > > +
> > > +		if (block)
> > > +			high_val = TGL_PCODE_EXIT_TCCOLD_DATA_H_BLOCK_REQ;
> > > +		else
> > > +			high_val = TGL_PCODE_EXIT_TCCOLD_DATA_H_UNBLOCK_REQ;
> > > +
> > > +		/*
> > > +		 * Spec states that we should timeout the request after
> > > 200us
> > > +		 * but the function below will timeout after 500us
> > > +		 */
> > > +		ret = sandybridge_pcode_read(i915, TGL_PCODE_TCCOLD,
> > > &low_val,
> > > +					     &high_val);
> > > +		if (ret == 0) {
> > > +			if (block &&
> > > +			    (low_val &
> > > TGL_PCODE_EXIT_TCCOLD_DATA_L_EXIT_FAILED))
> > > +				ret = -EIO;
> > > +			else
> > > +				break;
> > > +		}
> > > +
> > > +		if (++tries == 3)
> > > +			break;
> > > +
> > > +		msleep(1);
> > 
> > Why did this go back to msleep unconditionally? Doing that only for
> > ret == -EAGAIN made sense to me, since that is only the case where
> > you'd
> > want to avoid busy looping.
> 
> Mostly thinking about give PCODE some time to recover it selft when it
> returns low_val == TGL_PCODE_EXIT_TCCOLD_DATA_L_EXIT_FAILED

The spec says to retry the request in that case immediately. Please put
some comment at least here what the msleep() is for.

> > > +
> > > +	if (ret)
> > > +		drm_err(&i915->drm, "TC cold %sblock failed\n",
> > > +			block ? "" : "un");

Could've printed the error code too.

Reviewed-by: Imre Deak <imre.deak@intel.com>

> > > +	else
> > > +		drm_dbg_kms(&i915->drm, "TC cold %sblock succeeded\n",
> > > +			    block ? "" : "un");
> > > +}
> > > +
> > > +static void
> > > +tgl_tc_cold_off_power_well_enable(struct drm_i915_private *i915,
> > > +				  struct i915_power_well *power_well)
> > > +{
> > > +	tgl_tc_cold_request(i915, true);
> > > +}
> > > +
> > > +static void
> > > +tgl_tc_cold_off_power_well_disable(struct drm_i915_private *i915,
> > > +				   struct i915_power_well *power_well)
> > > +{
> > > +	tgl_tc_cold_request(i915, false);
> > > +}
> > > +
> > > +static void
> > > +tgl_tc_cold_off_power_well_sync_hw(struct drm_i915_private *i915,
> > > +				   struct i915_power_well *power_well)
> > > +{
> > > +	if (power_well->count > 0)
> > > +		tgl_tc_cold_off_power_well_enable(i915, power_well);
> > > +	else
> > > +		tgl_tc_cold_off_power_well_disable(i915, power_well);
> > > +}
> > > +
> > > +static bool
> > > +tgl_tc_cold_off_power_well_is_enabled(struct drm_i915_private
> > > *dev_priv,
> > > +				      struct i915_power_well
> > > *power_well)
> > > +{
> > > +	/*
> > > +	 * Not the correctly implementation but there is no way to just
> > > read it
> > > +	 * from PCODE, so returning count to avoid state mismatch
> > > errors
> > > +	 */
> > > +	return power_well->count;
> > > +}
> > > +
> > > +static const struct i915_power_well_ops tgl_tc_cold_off_ops = {
> > > +	.sync_hw = tgl_tc_cold_off_power_well_sync_hw,
> > > +	.enable = tgl_tc_cold_off_power_well_enable,
> > > +	.disable = tgl_tc_cold_off_power_well_disable,
> > > +	.is_enabled = tgl_tc_cold_off_power_well_is_enabled,
> > > +};
> > > +
> > >  static const struct i915_power_well_desc tgl_power_wells[] = {
> > >  	{
> > >  		.name = "always-on",
> > > @@ -4290,6 +4391,12 @@ static const struct i915_power_well_desc
> > > tgl_power_wells[] = {
> > >  			.hsw.irq_pipe_mask = BIT(PIPE_D),
> > >  		},
> > >  	},
> > > +	{
> > > +		.name = "TC cold off",
> > > +		.domains = TGL_TC_COLD_OFF_POWER_DOMAINS,
> > > +		.ops = &tgl_tc_cold_off_ops,
> > > +		.id = DISP_PW_ID_NONE,
> > > +	},
> > >  };
> > >  
> > >  static int
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display_power.h
> > > b/drivers/gpu/drm/i915/display/intel_display_power.h
> > > index da64a5edae7a..070457e7b948 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display_power.h
> > > +++ b/drivers/gpu/drm/i915/display/intel_display_power.h
> > > @@ -76,6 +76,7 @@ enum intel_display_power_domain {
> > >  	POWER_DOMAIN_MODESET,
> > >  	POWER_DOMAIN_GT_IRQ,
> > >  	POWER_DOMAIN_DPLL_DC_OFF,
> > > +	POWER_DOMAIN_TC_COLD_OFF,
> > >  	POWER_DOMAIN_INIT,
> > >  
> > >  	POWER_DOMAIN_NUM,
> > > diff --git a/drivers/gpu/drm/i915/display/intel_tc.c
> > > b/drivers/gpu/drm/i915/display/intel_tc.c
> > > index 0cf33d4d21c3..521a94c63640 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_tc.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_tc.c
> > > @@ -53,16 +53,27 @@ tc_port_load_fia_params(struct drm_i915_private
> > > *i915,
> > >  	}
> > >  }
> > >  
> > > +static enum intel_display_power_domain
> > > +tc_cold_get_power_domain(struct intel_digital_port *dig_port)
> > > +{
> > > +	struct drm_i915_private *i915 = to_i915(dig_port-
> > > >base.base.dev);
> > > +
> > > +	if (INTEL_GEN(i915) == 11)
> > > +		return intel_legacy_aux_to_power_domain(dig_port-
> > > >aux_ch);
> > > +	else
> > > +		return POWER_DOMAIN_TC_COLD_OFF;
> > > +}
> > > +
> > >  static intel_wakeref_t
> > >  tc_cold_block(struct intel_digital_port *dig_port)
> > >  {
> > >  	struct drm_i915_private *i915 = to_i915(dig_port-
> > > >base.base.dev);
> > >  	enum intel_display_power_domain domain;
> > >  
> > > -	if (INTEL_GEN(i915) != 11 || !dig_port->tc_legacy_port)
> > > +	if (INTEL_GEN(i915) == 11 && !dig_port->tc_legacy_port)
> > >  		return 0;
> > >  
> > > -	domain = intel_legacy_aux_to_power_domain(dig_port->aux_ch);
> > > +	domain = tc_cold_get_power_domain(dig_port);
> > >  	return intel_display_power_get(i915, domain);
> > >  }
> > >  
> > > @@ -80,7 +91,7 @@ tc_cold_unblock(struct intel_digital_port
> > > *dig_port, intel_wakeref_t wakeref)
> > >  	if (wakeref == 0)
> > >  		return;
> > >  
> > > -	domain = intel_legacy_aux_to_power_domain(dig_port->aux_ch);
> > > +	domain = tc_cold_get_power_domain(dig_port);
> > >  	intel_display_power_put_async(i915, domain, wakeref);
> > >  }
> > >  
> > > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > > b/drivers/gpu/drm/i915/i915_reg.h
> > > index e4667add70b0..39f281fe6d6c 100644
> > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > @@ -9111,6 +9111,10 @@ enum {
> > >  #define   ICL_PCODE_EXIT_TCCOLD			0x12
> > >  #define   HSW_PCODE_DE_WRITE_FREQ_REQ		0x17
> > >  #define   DISPLAY_IPS_CONTROL			0x19
> > > +#define   TGL_PCODE_TCCOLD			0x26
> > > +#define     TGL_PCODE_EXIT_TCCOLD_DATA_L_EXIT_FAILED	REG_BIT
> > > (0)
> > > +#define     TGL_PCODE_EXIT_TCCOLD_DATA_H_BLOCK_REQ	0
> > > +#define     TGL_PCODE_EXIT_TCCOLD_DATA_H_UNBLOCK_REQ	REG_BIT
> > > (0)
> > >              /* See also IPS_CTL */
> > >  #define     IPS_PCODE_CONTROL			(1 << 30)
> > >  #define   HSW_PCODE_DYNAMIC_DUTY_CYCLE_CONTROL	0x1A
> > > -- 
> > > 2.26.0
> > > 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx