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@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 > > > > 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 > > > > > _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
