[Intel-gfx] [PATCH] drm/i915/tc: Implement the TC cold exit sequence

[Souza, Jose]

On Thu, 2019-10-03 at 17:50 +0300, Imre Deak wrote:
> On Mon, Sep 30, 2019 at 05:55:36PM -0700, José Roberto de Souza
> wrote:
> > This is required for legacy/static TC ports as IOM is not aware of
> > the connection and will not trigger the TC cold exit.
> > 
> > BSpec: 21750
> > BSpsc: 49294
> > Cc: Imre Deak <imre.deak at intel.com>
> > Cc: Lucas De Marchi <lucas.demarchi at intel.com>
> > Signed-off-by: José Roberto de Souza <jose.souza at intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_tc.c | 34 ++++++++++++++++++++-
> > ----
> >  drivers/gpu/drm/i915/i915_reg.h         |  1 +
> >  2 files changed, 29 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_tc.c
> > b/drivers/gpu/drm/i915/display/intel_tc.c
> > index 7773169b7331..09b78027bdd5 100644
> > --- a/drivers/gpu/drm/i915/display/intel_tc.c
> > +++ b/drivers/gpu/drm/i915/display/intel_tc.c
> > @@ -7,6 +7,7 @@
> >  #include "intel_display.h"
> >  #include "intel_display_types.h"
> >  #include "intel_dp_mst.h"
> > +#include "intel_sideband.h"
> >  #include "intel_tc.h"
> >  
> >  static const char *tc_port_mode_name(enum tc_port_mode mode)
> > @@ -169,6 +170,22 @@ static void tc_port_fixup_legacy_flag(struct
> > intel_digital_port *dig_port,
> >  	dig_port->tc_legacy_port = !dig_port->tc_legacy_port;
> >  }
> >  
> > +static int tc_cold_exit_request(struct drm_i915_private *dev_priv)
> > +{
> > +	int ret;
> > +
> > +	do {
> > +		ret = sandybridge_pcode_write_timeout(dev_priv,
> > +						      ICL_PCODE_EXIT_TC
> > COLD, 0,
> > +						      250, 1);
> > +
> > +	} while (ret == -EAGAIN);
> > +
> > +	DRM_DEBUG_KMS("tccold exit %s\n", ret == 0 ? "succeeded" :
> > "failed");
> > +
> > +	return ret;
> > +}
> > +
> >  static u32 tc_port_live_status_mask(struct intel_digital_port
> > *dig_port)
> >  {
> >  	struct drm_i915_private *i915 = to_i915(dig_port-
> > >base.base.dev);
> > @@ -177,13 +194,21 @@ static u32 tc_port_live_status_mask(struct
> > intel_digital_port *dig_port)
> >  	u32 mask = 0;
> >  	u32 val;
> >  
> > +	if (intel_uncore_read(uncore, SDEISR) &
> > SDE_TC_HOTPLUG_ICP(tc_port))
> > +		mask |= BIT(TC_PORT_LEGACY);
> > +
> >  	val = intel_uncore_read(uncore,
> >  				PORT_TX_DFLEXDPSP(dig_port-
> > >tc_phy_fia));
> >  
> >  	if (val == 0xffffffff) {
> > -		DRM_DEBUG_KMS("Port %s: PHY in TCCOLD, nothing
> > connected\n",
> > -			      dig_port->tc_port_name);
> > -		return mask;
> > +		if (mask)
> > +			tc_cold_exit_request(i915);


Sorry for the delay, was OOO.

> 
> The semantic of the mbox command this needs is inherently racy on
> systems with preemption, the instruction to use it is:
> 
> """
> Issue GT Driver mailbox command to exit TCCOLD, then complete steps
> b-d
> within 500 milliseconds to prevent re-entry.
> """
> 
> We'd have to reach the point where we enable the AUX power in 500ms,
> which can't be guaranteed. Moreover TCCOLD could be entered just
> after
> reading PORT_TX_DFLEXDPSP, so we may not detect it.
> 
> What we can do - after discussing with HW folks - is to first request
> AUX power and then issue the mbox command, which prevents TCCOLD
> re-entry until releasing the AUX power request. This also needs
> ignoring
> the timeout of the AUX power enabling ACK, since it will only be
> ACKed
> after exiting TCCOLD.


Huum looks more robust indeed.

> 
> So I think we should block TCCOLD this way in __intel_tc_port_lock():
> 
> 	if tc_link_refcount==0:
> 		intel_display_power_get(POWER_DOMAIN_AUX_<port>)
> 		tc_cold_exit_request()

So for non-legacy the IOM request to exit TCCOLD could have timed out
at this point. But for most cases this will not happen, why not here
read one FIA register and check if == 0xffffffff if so we call
tc_cold_exit_request().

> 		if intel_tc_port_needs_reset():
> 			intel_tc_port_reset_mode()
> 		if tc_mode != LEGACY:
> 			intel_display_power_put(POWER_DOMAIN_AUX_<port>

Also why power_put() for non-legacy? a preemption could happen after
lock() and cause a TCCOLD before the needed reads/writes to FIA is
done.

Better remove this block and only power_put() when removing the last
reference on intel_tc_port_unlock()

> )
> 
> then unblock TCCOLD in intel_tc_port_unlock():
> 
> 	if tc_link_refcount==0 and tc_mode == LEGACY:
> 		intel_display_power_put(POWER_DOMAIN_AUX_<port>)
> 	
> > +
> > +		if (val == 0xffffffff) {
> > +			DRM_DEBUG_KMS("Port %s: PHY in TCCOLD, nothing
> > connected\n",
> > +				      dig_port->tc_port_name);
> > +			return mask;
> > +		}
> >  	}
> >  
> >  	if (val & TC_LIVE_STATE_TBT(dig_port->tc_phy_fia_idx))
> > @@ -191,9 +216,6 @@ static u32 tc_port_live_status_mask(struct
> > intel_digital_port *dig_port)
> >  	if (val & TC_LIVE_STATE_TC(dig_port->tc_phy_fia_idx))
> >  		mask |= BIT(TC_PORT_DP_ALT);
> >  
> > -	if (intel_uncore_read(uncore, SDEISR) &
> > SDE_TC_HOTPLUG_ICP(tc_port))
> > -		mask |= BIT(TC_PORT_LEGACY);
> > -
> >  	/* The sink can be connected only in a single mode. */
> >  	if (!WARN_ON(hweight32(mask) > 1))
> >  		tc_port_fixup_legacy_flag(dig_port, mask);
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > b/drivers/gpu/drm/i915/i915_reg.h
> > index 058aa5ca8b73..35c3724b7fef 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -8860,6 +8860,7 @@ enum {
> >  #define     ICL_PCODE_MEM_SS_READ_QGV_POINT_INFO(point)	(((poin
> > t) << 16) | (0x1 << 8))
> >  #define   GEN6_PCODE_READ_D_COMP		0x10
> >  #define   GEN6_PCODE_WRITE_D_COMP		0x11
> > +#define   ICL_PCODE_EXIT_TCCOLD			0x12
> >  #define   HSW_PCODE_DE_WRITE_FREQ_REQ		0x17
> >  #define   DISPLAY_IPS_CONTROL			0x19
> >              /* See also IPS_CTL */
> > -- 
> > 2.23.0
> >