[Intel-gfx] [PATCH 5/6] drm/i915/tc/icl: Implement TC cold sequences
Souza, Jose
jose.souza at intel.com
Fri Apr 3 01:18:54 UTC 2020
Hi Imre
I guess I did all the requested changes but trybot got some
warnings that I will need more time to understand and fix it.
If you have time please check if is this way that you are asking:
https://github.com/zehortigoza/linux/tree/tccold-v3
Trybot warnings:
https://intel-gfx-ci.01.org/tree/drm-tip/Trybot_5784/bat-all.html
Thanks
On Thu, 2020-04-02 at 04:02 +0300, Imre Deak wrote:
> On Thu, Apr 02, 2020 at 01:35:30AM +0300, Souza, Jose wrote:
> > 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 at intel.com>
> > > > Cc: Cooper Chiou <cooper.chiou at intel.com>
> > > > Cc: Kai-Heng Feng <kai.heng.feng at canonical.com>
> > > > Signed-off-by: José Roberto de Souza <jose.souza at intel.com>
> > > > ---
> > > >
> > > > 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
>
> Yes, it's a loss of the vendor shipping a corrupt VBT. But we'll
> print
> an error and fix up the flag.
>
> > > > + 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())
>
> What I thought is that the legacy AUX power request will ack after
> the
> PCODE request completes, regardless of whether the sink is connected
> or
> not. If that is not the case then let's just suppress the timeout for
> legacy AUX power wells the same way we do that for TBT AUX. I don't
> like
> to make the above call conditional on the live status flag, as that
> can
> change at any moment. So in either case let's make the above call
> unconditional.
>
> > > >
> > > > if (INTEL_GEN(dev_priv) >= 12 && !power_well->desc-
> > > > > hsw.is_tc_tbt) {
>
> Could you check your email client, so that it doesn't wrap lines?
>
> > > > 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.
>
> Yes, hence blocking it here whenever accessing the HW and elsewhere
> (AUX
> transfers, modeset) whenever holding an AUX reference. See my comment
> below about the other two functions in intel_tc that needs the
> block/unblock.
>
> > > 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
> > > >
More information about the Intel-gfx
mailing list