On Wed, 2020-04-01 at 15:43 +0300, Imre Deak wrote: > On Tue, Mar 31, 2020 at 05:41:19PM -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. > > > > Just request PCODE to exit TCCOLD is not enough as it could enter > > again be driver makes use of the port, to prevent it BSpec states > > that > > aux powerwell should be held. > > > > So here embedding the TC cold exit sequence into ICL aux enable, > > it will enable aux, request tc cold exit and depending in the TC > > live > > state continue with the regular aux enable sequence. > > > > And then turning on aux power well during tc lock and turning off > > during unlock both depending into the TC port refcount. > > > > BSpec: 21750 > > Fixes: https://gitlab.freedesktop.org/drm/intel/issues/1296 > > Cc: Imre Deak <imre.deak@xxxxxxxxx> > > Cc: Cooper Chiou <cooper.chiou@xxxxxxxxx> > > Cc: Kai-Heng Feng <kai.heng.feng@xxxxxxxxxxxxx> > > Signed-off-by: José Roberto de Souza <jose.souza@xxxxxxxxx> > > --- > > > > Will run some tests in the office with TBT dockstation to check if > > it will be a issue keep both aux enabled. Otherwise more changes > > will > > be required here. > > > > .../drm/i915/display/intel_display_power.c | 12 ++++- > > .../drm/i915/display/intel_display_types.h | 1 + > > drivers/gpu/drm/i915/display/intel_tc.c | 47 > > ++++++++++++++++++- > > drivers/gpu/drm/i915/display/intel_tc.h | 2 + > > drivers/gpu/drm/i915/i915_reg.h | 1 + > > 5 files changed, 59 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c > > b/drivers/gpu/drm/i915/display/intel_display_power.c > > index dbd61517ba63..1ccd57d645c7 100644 > > --- a/drivers/gpu/drm/i915/display/intel_display_power.c > > +++ b/drivers/gpu/drm/i915/display/intel_display_power.c > > @@ -592,9 +592,17 @@ icl_tc_phy_aux_power_well_enable(struct > > drm_i915_private *dev_priv, > > > > _hsw_power_well_enable(dev_priv, power_well); > > > > - /* TODO ICL TC cold handling */ > > + if (INTEL_GEN(dev_priv) == 11) > > Should be if (ICL && dig_port->tc_legacy_port) Makes sence. Oh so we could use it on __intel_tc_port_lock() too and don't do this stuff for non legacy ports. Until we get a report of a system with a wrong VBT :P > > > + intel_tc_icl_tc_cold_exit(dev_priv); > > > > - _hsw_power_well_continue_enable(dev_priv, power_well); > > + /* > > + * To avoid power well enable timeouts when disconnected or in > > TBT mode > > + * when doing the TC cold exit sequence for GEN11 > > + */ > > + if (INTEL_GEN(dev_priv) != 11 || > > + (intel_tc_port_live_status_mask(dig_port) & > > + (TC_PORT_LEGACY | TC_PORT_DP_ALT))) > > + _hsw_power_well_continue_enable(dev_priv, power_well); > > Why can't we call this unconditionally? Because we are requesting aux power of regular TC ports as part of tc cold exit sequence, if the port is disconnected it will timeout in hsw_wait_for_power_well_enable(). Anyways it is wrong as it is not taking care of TBT ports, so changing to: if (INTEL_GEN(dev_priv) != 11 || !dig_port->tc_legacy_port || intel_tc_port_live_status_mask()) > > > > > if (INTEL_GEN(dev_priv) >= 12 && !power_well->desc- > > >hsw.is_tc_tbt) { > > enum tc_port tc_port; > > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h > > b/drivers/gpu/drm/i915/display/intel_display_types.h > > index 176ab5f1e867..a9a4a3c1b4d7 100644 > > --- a/drivers/gpu/drm/i915/display/intel_display_types.h > > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h > > @@ -1391,6 +1391,7 @@ struct intel_digital_port { > > enum intel_display_power_domain ddi_io_power_domain; > > struct mutex tc_lock; /* protects the TypeC port mode */ > > intel_wakeref_t tc_lock_wakeref; > > + intel_wakeref_t tc_cold_wakeref; > > 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 d944be935423..b6d67f069ef7 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) > > @@ -506,6 +507,13 @@ static void __intel_tc_port_lock(struct > > intel_digital_port *dig_port, > > > > mutex_lock(&dig_port->tc_lock); > > > > + if (INTEL_GEN(i915) == 11 && dig_port->tc_link_refcount == 0) { > > + enum intel_display_power_domain aux_domain; > > + > > + aux_domain = intel_aux_ch_to_power_domain(dig_port- > > >aux_ch); > > + dig_port->tc_cold_wakeref = > > intel_display_power_get(i915, aux_domain); > > + } > > + > > It would be enough to hold this ref only for the time we access FIA > regs. Anything else later will hold its own AUX reference, which > takes > care of blocking tc-cold. So here something like: > According to BSpec we need to keep TC cold blocked while accessing TC PHY registers too. > tc_cold_wakeref = block_tc_cold(dig_port); > > where block_tc_cold() would return a non-NULL wakeref only for > ICL/dig_port->tc_legacy_port and TGL. > > > if (!dig_port->tc_link_refcount && > > intel_tc_port_needs_reset(dig_port)) > > intel_tc_port_reset_mode(dig_port, required_lanes); > > unblock_tc_cold(tc_cold_wakeref); > > We need to call block/unblock_tc_cold() also in > intel_tc_port_sanitize() > and intel_tc_port_connected(). > > > @@ -519,15 +527,30 @@ void intel_tc_port_lock(struct > > intel_digital_port *dig_port) > > __intel_tc_port_lock(dig_port, 1); > > } > > > > +static void icl_tc_cold_unblock(struct intel_digital_port > > *dig_port) > > +{ > > + struct drm_i915_private *i915 = to_i915(dig_port- > > >base.base.dev); > > + enum intel_display_power_domain aux_domain; > > + intel_wakeref_t tc_cold_wakeref; > > + > > + if (INTEL_GEN(i915) != 11 || dig_port->tc_link_refcount > 0) > > + return; > > + > > + tc_cold_wakeref = fetch_and_zero(&dig_port->tc_cold_wakeref); > > + aux_domain = intel_aux_ch_to_power_domain(dig_port->aux_ch); > > + intel_display_power_put_async(i915, aux_domain, > > tc_cold_wakeref); > > +} > > + > > void intel_tc_port_unlock(struct intel_digital_port *dig_port) > > { > > struct drm_i915_private *i915 = to_i915(dig_port- > > >base.base.dev); > > intel_wakeref_t wakeref = fetch_and_zero(&dig_port- > > >tc_lock_wakeref); > > > > + icl_tc_cold_unblock(dig_port); > > + > > mutex_unlock(&dig_port->tc_lock); > > > > - intel_display_power_put_async(i915, POWER_DOMAIN_DISPLAY_CORE, > > - wakeref); > > + intel_display_power_put_async(i915, POWER_DOMAIN_DISPLAY_CORE, > > wakeref); > > } > > > > bool intel_tc_port_ref_held(struct intel_digital_port *dig_port) > > @@ -548,6 +571,7 @@ void intel_tc_port_put_link(struct > > intel_digital_port *dig_port) > > { > > mutex_lock(&dig_port->tc_lock); > > dig_port->tc_link_refcount--; > > + icl_tc_cold_unblock(dig_port); > > mutex_unlock(&dig_port->tc_lock); > > } > > > > @@ -568,3 +592,22 @@ 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); > > } > > + > > +void intel_tc_icl_tc_cold_exit(struct drm_i915_private *i915) > > This could be in intel_display_power.c now, so we don't need to > export > it. Okay. > > > +{ > > + int ret; > > + > > + do { > > + ret = sandybridge_pcode_write_timeout(i915, > > + ICL_PCODE_EXIT_TC > > COLD, > > + 0, 250, 1); > > + > > + } while (ret == -EAGAIN); > > + > > + if (!ret) > > + msleep(1); > > Could you add a comment explaining that we need the sleep, since > according to BSpec the above request may not have completed even > though > it returned success? Sure > > > + > > + if (ret) > > + drm_dbg_kms(&i915->drm, "TC cold block %s\n", > > + (ret == 0 ? "succeeded" : "failed")); > > +} > > diff --git a/drivers/gpu/drm/i915/display/intel_tc.h > > b/drivers/gpu/drm/i915/display/intel_tc.h > > index a1afcee48818..168d8896fcfd 100644 > > --- a/drivers/gpu/drm/i915/display/intel_tc.h > > +++ b/drivers/gpu/drm/i915/display/intel_tc.h > > @@ -9,6 +9,7 @@ > > #include <linux/mutex.h> > > #include <linux/types.h> > > > > +struct drm_i915_private; > > struct intel_digital_port; > > > > bool intel_tc_port_connected(struct intel_digital_port *dig_port); > > @@ -29,5 +30,6 @@ bool intel_tc_port_ref_held(struct > > intel_digital_port *dig_port); > > void intel_tc_port_init(struct intel_digital_port *dig_port, bool > > is_legacy); > > > > u32 intel_tc_port_live_status_mask(struct intel_digital_port > > *dig_port); > > +void intel_tc_icl_tc_cold_exit(struct drm_i915_private *i915); > > > > #endif /* __INTEL_TC_H__ */ > > diff --git a/drivers/gpu/drm/i915/i915_reg.h > > b/drivers/gpu/drm/i915/i915_reg.h > > index 17484345cb80..b111815d6596 100644 > > --- a/drivers/gpu/drm/i915/i915_reg.h > > +++ b/drivers/gpu/drm/i915/i915_reg.h > > @@ -9107,6 +9107,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.26.0 > > _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
