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@xxxxxxxxx>
> > > > Cc: Lucas De Marchi <lucas.demarchi@xxxxxxxxx>
> > > > Signed-off-by: José Roberto de Souza <jose.souza@xxxxxxxxx>
> > > > ---
> > > > 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
> > > >
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
