[Intel-gfx] [PATCH 5/6] drm/i915/tc/icl: Implement TC cold sequences
Imre Deak
imre.deak at intel.com
Mon Apr 6 12:57:31 UTC 2020
On Fri, Apr 03, 2020 at 04:18:54AM +0300, Souza, Jose wrote:
> 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
Yes looks ok overall, I added a few comments on the trybot patches.
> Trybot warnings:
> https://intel-gfx-ci.01.org/tree/drm-tip/Trybot_5784/bat-all.html
I see here
<3> [428.836598] [drm:tc_port_live_status_mask [i915]] *ERROR* Port D/TC#2: live status 00000004 mismatches the legacy port flag, fix flag
which is a known buggy VBT issue and
i915 0000:00:02.0: drm_WARN_ON(!power_well->desc->hsw.is_tc_tbt || !(((&(dev_priv)->__info)->gen) == 11 && dig_port->tc_legacy_port))
which is just a typo.
> 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