| Message ID | 20191001005536.123020-1-jose.souza@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/tc: Implement the TC cold exit sequence | expand |
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@intel.com> > Cc: Lucas De Marchi <lucas.demarchi@intel.com> > Signed-off-by: José Roberto de Souza <jose.souza@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_TCCOLD, 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); 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. 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() if intel_tc_port_needs_reset(): intel_tc_port_reset_mode() if tc_mode != LEGACY: intel_display_power_put(POWER_DOMAIN_AUX_<port>) 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) (((point) << 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 >
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@intel.com> > > Cc: Lucas De Marchi <lucas.demarchi@intel.com> > > Signed-off-by: José Roberto de Souza <jose.souza@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 > >
On Sat, Oct 19, 2019 at 02:59:14AM +0300, Souza, Jose wrote: > 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@intel.com> > > > Cc: Lucas De Marchi <lucas.demarchi@intel.com> > > > Signed-off-by: José Roberto de Souza <jose.souza@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(). That would be racy, TCCOLD could be entered right after doing the FIA reg check. Getting the AUX power ref alone won't prevent a TCCOLD entry until the PCODE command has completed. > > > 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. The new PCODE command was only added for legacy and the AUX power reference makes a difference only in that mode. In DP-alt mode the DPCSSS.DPPMSTC flag prevents TCCOLD. In TBT mode the TBT AUX power ref would not prevent TCCOLD. In that mode we don't access FIA regs afterwards and "the TBT controller prevents TCCOLD while it uses the connection". > > 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 > > >
On Sat, 2019-10-19 at 13:49 +0300, Imre Deak wrote: > On Sat, Oct 19, 2019 at 02:59:14AM +0300, Souza, Jose wrote: > > 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@intel.com> > > > > Cc: Lucas De Marchi <lucas.demarchi@intel.com> > > > > Signed-off-by: José Roberto de Souza <jose.souza@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(). > > That would be racy, TCCOLD could be entered right after doing the FIA > reg check. Getting the AUX power ref alone won't prevent a TCCOLD > entry > until the PCODE command has completed. > > > > 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. > > The new PCODE command was only added for legacy and the AUX power > reference makes a difference only in that mode. In DP-alt mode the > DPCSSS.DPPMSTC flag prevents TCCOLD. > > In TBT mode the TBT AUX power ref would not prevent TCCOLD. In that > mode > we don't access FIA regs afterwards and > "the TBT controller prevents TCCOLD while it uses the connection". Makes sense, well the only doubt that I have is if getting POWER_DOMAIN_AUX_<port>_TBT will prevent TCCOLD for legacy and alt- mode, as at this point we still don't know if it is alt-mode, TBT or legacy(we know that is legacy but live status could be reporting something else tc_port_fixup_legacy_flag()) > > > 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 > > > >
On Tue, Oct 22, 2019 at 03:03:18AM +0300, Souza, Jose wrote: > [...] > Makes sense, well the only doubt that I have is if getting > POWER_DOMAIN_AUX_<port>_TBT will prevent TCCOLD for legacy and alt- > mode, Why do we need to care about POWER_DOMAIN_AUX_<port>_TBT? It doesn't affect the TCCOLD state and we shouldn't get it here. > as at this point we still don't know if it is alt-mode, TBT or > legacy(we know that is legacy but live status could be reporting > something else tc_port_fixup_legacy_flag()) We enable the non-TBT AUX power and then drop after the mode is already known and it's other than legacy. --Imre
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_TCCOLD, 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); + + 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) (((point) << 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 */
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@intel.com> Cc: Lucas De Marchi <lucas.demarchi@intel.com> Signed-off-by: José Roberto de Souza <jose.souza@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(-)