| Message ID | 20190620140600.11357-22-imre.deak@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: Fix TypeC port mode switching | expand |
On Thu, Jun 20, 2019 at 05:05:58PM +0300, Imre Deak wrote: > Add state verification for the TypeC port mode wrt. the port's AUX power > well enabling/disabling. Also check the correctness of changing the port > mode: > - When enabling/disabling the AUX power well for a TypeC port we must hold > already the TypeC port lock. > - When changing the TypeC port mode the port's AUX power domain must be > disabled. > > Cc: José Roberto de Souza <jose.souza@intel.com> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > Signed-off-by: Imre Deak <imre.deak@intel.com> > --- > .../drm/i915/display/intel_display_power.c | 99 +++++++++++++++++-- > drivers/gpu/drm/i915/display/intel_tc.c | 2 + > drivers/gpu/drm/i915/display/intel_tc.h | 10 +- > 3 files changed, 102 insertions(+), 9 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c b/drivers/gpu/drm/i915/display/intel_display_power.c > index fd13cd68deae..f4613e10c3b3 100644 > --- a/drivers/gpu/drm/i915/display/intel_display_power.c > +++ b/drivers/gpu/drm/i915/display/intel_display_power.c > @@ -17,6 +17,7 @@ > #include "intel_drv.h" > #include "intel_hotplug.h" > #include "intel_sideband.h" > +#include "intel_tc.h" > > bool intel_display_power_well_is_enabled(struct drm_i915_private *dev_priv, > enum i915_power_well_id power_well_id); > @@ -447,26 +448,110 @@ icl_combo_phy_aux_power_well_disable(struct drm_i915_private *dev_priv, > #define ICL_TBT_AUX_PW_TO_CH(pw_idx) \ > ((pw_idx) - ICL_PW_CTL_IDX_AUX_TBT1 + AUX_CH_C) > > +static enum aux_ch icl_tc_phy_aux_ch(struct drm_i915_private *dev_priv, > + struct i915_power_well *power_well) > +{ > + int pw_idx = power_well->desc->hsw.idx; > + > + return power_well->desc->hsw.is_tc_tbt ? ICL_TBT_AUX_PW_TO_CH(pw_idx) : > + ICL_AUX_PW_TO_CH(pw_idx); > +} > + > +#if IS_ENABLED(CONFIG_DRM_I915_DEBUG_RUNTIME_PM) > + > +static u64 async_put_domains_mask(struct i915_power_domains *power_domains); > + > +static int power_well_async_ref_count(struct drm_i915_private *dev_priv, > + struct i915_power_well *power_well) > +{ > + enum intel_display_power_domain domain; > + u64 async_domains = async_put_domains_mask(&dev_priv->power_domains); > + int refs = 0; > + > + for_each_power_domain(domain, power_well->desc->domains) > + refs += !!(async_domains & BIT_ULL(domain)); > + > + WARN_ON(refs > power_well->count); hweight64() not sufficient here? > + > + return refs; > +} > + > +static void icl_tc_port_assert_ref_held(struct drm_i915_private *dev_priv, > + struct i915_power_well *power_well) > +{ > + enum aux_ch aux_ch = icl_tc_phy_aux_ch(dev_priv, power_well); > + struct intel_digital_port *dig_port = NULL; > + struct intel_encoder *encoder; > + > + /* Bypass the check if all references are released asynchronously */ > + if (power_well_async_ref_count(dev_priv, power_well) == > + power_well->count) > + return; > + > + aux_ch = icl_tc_phy_aux_ch(dev_priv, power_well); > + > + for_each_intel_encoder(&dev_priv->drm, encoder) { > + if (!intel_port_is_tc(dev_priv, encoder->port)) > + continue; > + > + /* We'll check the MST primary port */ > + if (encoder->type == INTEL_OUTPUT_DP_MST) > + continue; > + > + dig_port = enc_to_dig_port(&encoder->base); > + if (WARN_ON(!dig_port)) > + continue; > + > + if (dig_port->aux_ch != aux_ch) { > + dig_port = NULL; > + continue; > + } > + > + break; > + } > + > + if (WARN_ON(!dig_port)) > + return; > + > + WARN_ON(!intel_tc_port_ref_held(dig_port)); > +} > + > +#else > + > +static void icl_tc_port_assert_ref_held(struct drm_i915_private *dev_priv, > + struct i915_power_well *power_well) > +{ > +} > + > +#endif > + > static void > icl_tc_phy_aux_power_well_enable(struct drm_i915_private *dev_priv, > struct i915_power_well *power_well) > { > - int pw_idx = power_well->desc->hsw.idx; > - bool is_tbt = power_well->desc->hsw.is_tc_tbt; > - enum aux_ch aux_ch; > + enum aux_ch aux_ch = icl_tc_phy_aux_ch(dev_priv, power_well); > u32 val; > > - aux_ch = is_tbt ? ICL_TBT_AUX_PW_TO_CH(pw_idx) : > - ICL_AUX_PW_TO_CH(pw_idx); > + icl_tc_port_assert_ref_held(dev_priv, power_well); > + > val = I915_READ(DP_AUX_CH_CTL(aux_ch)); > val &= ~DP_AUX_CH_CTL_TBT_IO; > - if (is_tbt) > + if (power_well->desc->hsw.is_tc_tbt) > val |= DP_AUX_CH_CTL_TBT_IO; > I915_WRITE(DP_AUX_CH_CTL(aux_ch), val); > > hsw_power_well_enable(dev_priv, power_well); > } > > +static void > +icl_tc_phy_aux_power_well_disable(struct drm_i915_private *dev_priv, > + struct i915_power_well *power_well) > +{ > + icl_tc_port_assert_ref_held(dev_priv, power_well); > + > + hsw_power_well_disable(dev_priv, power_well); > +} > + > /* > * We should only use the power well if we explicitly asked the hardware to > * enable it, so check if it's enabled and also check if we've requested it to > @@ -3119,7 +3204,7 @@ static const struct i915_power_well_ops icl_combo_phy_aux_power_well_ops = { > static const struct i915_power_well_ops icl_tc_phy_aux_power_well_ops = { > .sync_hw = hsw_power_well_sync_hw, > .enable = icl_tc_phy_aux_power_well_enable, > - .disable = hsw_power_well_disable, > + .disable = icl_tc_phy_aux_power_well_disable, > .is_enabled = hsw_power_well_enabled, > }; > > diff --git a/drivers/gpu/drm/i915/display/intel_tc.c b/drivers/gpu/drm/i915/display/intel_tc.c > index c8904911e841..6c43becf97f7 100644 > --- a/drivers/gpu/drm/i915/display/intel_tc.c > +++ b/drivers/gpu/drm/i915/display/intel_tc.c > @@ -303,6 +303,8 @@ static void intel_tc_port_reset_mode(struct intel_digital_port *dig_port, > enum tc_port_mode old_tc_mode = dig_port->tc_mode; > > intel_display_power_flush_work(dev_priv); > + WARN_ON(intel_display_power_is_enabled(dev_priv, > + intel_aux_power_domain(dig_port))); > > icl_tc_phy_disconnect(dig_port); > icl_tc_phy_connect(dig_port, required_lanes); > diff --git a/drivers/gpu/drm/i915/display/intel_tc.h b/drivers/gpu/drm/i915/display/intel_tc.h > index 31af7be96070..8adc107cdbcb 100644 > --- a/drivers/gpu/drm/i915/display/intel_tc.h > +++ b/drivers/gpu/drm/i915/display/intel_tc.h > @@ -7,8 +7,8 @@ > #define __INTEL_TC_H__ > > #include <linux/types.h> > - > -struct intel_digital_port; > +#include <linux/mutex.h> > +#include "intel_drv.h" > > void icl_tc_phy_disconnect(struct intel_digital_port *dig_port); > > @@ -23,6 +23,12 @@ void intel_tc_port_get_link(struct intel_digital_port *dig_port, > int required_lanes); > void intel_tc_port_put_link(struct intel_digital_port *dig_port); > > +static inline int intel_tc_port_ref_held(struct intel_digital_port *dig_port) > +{ > + return mutex_is_locked(&dig_port->tc_lock) || > + dig_port->tc_link_refcount; This will allow tc_lock to not be held if the refcount is elevated. Was that the intention? The commit message says the lock should always be held. Should we do a lockdep assert here if lockdep is enabled? > +} > + > void intel_tc_port_init(struct intel_digital_port *dig_port, bool is_legacy); > > #endif /* __INTEL_TC_H__ */ > -- > 2.17.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Tue, Jun 25, 2019 at 05:12:52PM +0300, Ville Syrjälä wrote: > On Thu, Jun 20, 2019 at 05:05:58PM +0300, Imre Deak wrote: > > Add state verification for the TypeC port mode wrt. the port's AUX power > > well enabling/disabling. Also check the correctness of changing the port > > mode: > > - When enabling/disabling the AUX power well for a TypeC port we must hold > > already the TypeC port lock. > > - When changing the TypeC port mode the port's AUX power domain must be > > disabled. > > > > Cc: José Roberto de Souza <jose.souza@intel.com> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > Signed-off-by: Imre Deak <imre.deak@intel.com> > > --- > > .../drm/i915/display/intel_display_power.c | 99 +++++++++++++++++-- > > drivers/gpu/drm/i915/display/intel_tc.c | 2 + > > drivers/gpu/drm/i915/display/intel_tc.h | 10 +- > > 3 files changed, 102 insertions(+), 9 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c b/drivers/gpu/drm/i915/display/intel_display_power.c > > index fd13cd68deae..f4613e10c3b3 100644 > > --- a/drivers/gpu/drm/i915/display/intel_display_power.c > > +++ b/drivers/gpu/drm/i915/display/intel_display_power.c > > @@ -17,6 +17,7 @@ > > #include "intel_drv.h" > > #include "intel_hotplug.h" > > #include "intel_sideband.h" > > +#include "intel_tc.h" > > > > bool intel_display_power_well_is_enabled(struct drm_i915_private *dev_priv, > > enum i915_power_well_id power_well_id); > > @@ -447,26 +448,110 @@ icl_combo_phy_aux_power_well_disable(struct drm_i915_private *dev_priv, > > #define ICL_TBT_AUX_PW_TO_CH(pw_idx) \ > > ((pw_idx) - ICL_PW_CTL_IDX_AUX_TBT1 + AUX_CH_C) > > > > +static enum aux_ch icl_tc_phy_aux_ch(struct drm_i915_private *dev_priv, > > + struct i915_power_well *power_well) > > +{ > > + int pw_idx = power_well->desc->hsw.idx; > > + > > + return power_well->desc->hsw.is_tc_tbt ? ICL_TBT_AUX_PW_TO_CH(pw_idx) : > > + ICL_AUX_PW_TO_CH(pw_idx); > > +} > > + > > +#if IS_ENABLED(CONFIG_DRM_I915_DEBUG_RUNTIME_PM) > > + > > +static u64 async_put_domains_mask(struct i915_power_domains *power_domains); > > + > > +static int power_well_async_ref_count(struct drm_i915_private *dev_priv, > > + struct i915_power_well *power_well) > > +{ > > + enum intel_display_power_domain domain; > > + u64 async_domains = async_put_domains_mask(&dev_priv->power_domains); > > + int refs = 0; > > + > > + for_each_power_domain(domain, power_well->desc->domains) > > + refs += !!(async_domains & BIT_ULL(domain)); > > + > > + WARN_ON(refs > power_well->count); > > hweight64() not sufficient here? Ah, just return hweight64(power_well->desc->domains & async_domains)? Yep, that would work, didn't occur to me.. > > > + > > + return refs; > > +} > > + > > +static void icl_tc_port_assert_ref_held(struct drm_i915_private *dev_priv, > > + struct i915_power_well *power_well) > > +{ > > + enum aux_ch aux_ch = icl_tc_phy_aux_ch(dev_priv, power_well); > > + struct intel_digital_port *dig_port = NULL; > > + struct intel_encoder *encoder; > > + > > + /* Bypass the check if all references are released asynchronously */ > > + if (power_well_async_ref_count(dev_priv, power_well) == > > + power_well->count) > > + return; > > + > > + aux_ch = icl_tc_phy_aux_ch(dev_priv, power_well); > > + > > + for_each_intel_encoder(&dev_priv->drm, encoder) { > > + if (!intel_port_is_tc(dev_priv, encoder->port)) > > + continue; > > + > > + /* We'll check the MST primary port */ > > + if (encoder->type == INTEL_OUTPUT_DP_MST) > > + continue; > > + > > + dig_port = enc_to_dig_port(&encoder->base); > > + if (WARN_ON(!dig_port)) > > + continue; > > + > > + if (dig_port->aux_ch != aux_ch) { > > + dig_port = NULL; > > + continue; > > + } > > + > > + break; > > + } > > + > > + if (WARN_ON(!dig_port)) > > + return; > > + > > + WARN_ON(!intel_tc_port_ref_held(dig_port)); > > +} > > + > > +#else > > + > > +static void icl_tc_port_assert_ref_held(struct drm_i915_private *dev_priv, > > + struct i915_power_well *power_well) > > +{ > > +} > > + > > +#endif > > + > > static void > > icl_tc_phy_aux_power_well_enable(struct drm_i915_private *dev_priv, > > struct i915_power_well *power_well) > > { > > - int pw_idx = power_well->desc->hsw.idx; > > - bool is_tbt = power_well->desc->hsw.is_tc_tbt; > > - enum aux_ch aux_ch; > > + enum aux_ch aux_ch = icl_tc_phy_aux_ch(dev_priv, power_well); > > u32 val; > > > > - aux_ch = is_tbt ? ICL_TBT_AUX_PW_TO_CH(pw_idx) : > > - ICL_AUX_PW_TO_CH(pw_idx); > > + icl_tc_port_assert_ref_held(dev_priv, power_well); > > + > > val = I915_READ(DP_AUX_CH_CTL(aux_ch)); > > val &= ~DP_AUX_CH_CTL_TBT_IO; > > - if (is_tbt) > > + if (power_well->desc->hsw.is_tc_tbt) > > val |= DP_AUX_CH_CTL_TBT_IO; > > I915_WRITE(DP_AUX_CH_CTL(aux_ch), val); > > > > hsw_power_well_enable(dev_priv, power_well); > > } > > > > +static void > > +icl_tc_phy_aux_power_well_disable(struct drm_i915_private *dev_priv, > > + struct i915_power_well *power_well) > > +{ > > + icl_tc_port_assert_ref_held(dev_priv, power_well); > > + > > + hsw_power_well_disable(dev_priv, power_well); > > +} > > + > > /* > > * We should only use the power well if we explicitly asked the hardware to > > * enable it, so check if it's enabled and also check if we've requested it to > > @@ -3119,7 +3204,7 @@ static const struct i915_power_well_ops icl_combo_phy_aux_power_well_ops = { > > static const struct i915_power_well_ops icl_tc_phy_aux_power_well_ops = { > > .sync_hw = hsw_power_well_sync_hw, > > .enable = icl_tc_phy_aux_power_well_enable, > > - .disable = hsw_power_well_disable, > > + .disable = icl_tc_phy_aux_power_well_disable, > > .is_enabled = hsw_power_well_enabled, > > }; > > > > diff --git a/drivers/gpu/drm/i915/display/intel_tc.c b/drivers/gpu/drm/i915/display/intel_tc.c > > index c8904911e841..6c43becf97f7 100644 > > --- a/drivers/gpu/drm/i915/display/intel_tc.c > > +++ b/drivers/gpu/drm/i915/display/intel_tc.c > > @@ -303,6 +303,8 @@ static void intel_tc_port_reset_mode(struct intel_digital_port *dig_port, > > enum tc_port_mode old_tc_mode = dig_port->tc_mode; > > > > intel_display_power_flush_work(dev_priv); > > + WARN_ON(intel_display_power_is_enabled(dev_priv, > > + intel_aux_power_domain(dig_port))); > > > > icl_tc_phy_disconnect(dig_port); > > icl_tc_phy_connect(dig_port, required_lanes); > > diff --git a/drivers/gpu/drm/i915/display/intel_tc.h b/drivers/gpu/drm/i915/display/intel_tc.h > > index 31af7be96070..8adc107cdbcb 100644 > > --- a/drivers/gpu/drm/i915/display/intel_tc.h > > +++ b/drivers/gpu/drm/i915/display/intel_tc.h > > @@ -7,8 +7,8 @@ > > #define __INTEL_TC_H__ > > > > #include <linux/types.h> > > - > > -struct intel_digital_port; > > +#include <linux/mutex.h> > > +#include "intel_drv.h" > > > > void icl_tc_phy_disconnect(struct intel_digital_port *dig_port); > > > > @@ -23,6 +23,12 @@ void intel_tc_port_get_link(struct intel_digital_port *dig_port, > > int required_lanes); > > void intel_tc_port_put_link(struct intel_digital_port *dig_port); > > > > +static inline int intel_tc_port_ref_held(struct intel_digital_port *dig_port) > > +{ > > + return mutex_is_locked(&dig_port->tc_lock) || > > + dig_port->tc_link_refcount; > > This will allow tc_lock to not be held if the refcount is elevated. Was > that the intention? The commit message says the lock should always be > held. Right, the commit message is not precise, it should be: - When enabling/disabling the AUX power well for a TypeC port we must hold the TypeC port lock - the case for AUX transfers - or hold a Type C port link reference - the case for modeset enabling/disabling. Does that make sense? > > Should we do a lockdep assert here if lockdep is enabled? If the above criteria for enabling/disabling the power wells sounds ok, then mutex_is_locked(tc_lock) || tc_link_refcount would be enough. > > > +} > > + > > void intel_tc_port_init(struct intel_digital_port *dig_port, bool is_legacy); > > > > #endif /* __INTEL_TC_H__ */ > > -- > > 2.17.1 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Ville Syrjälä > Intel
On Tue, Jun 25, 2019 at 10:23:51PM +0300, Imre Deak wrote: > On Tue, Jun 25, 2019 at 05:12:52PM +0300, Ville Syrjälä wrote: > > On Thu, Jun 20, 2019 at 05:05:58PM +0300, Imre Deak wrote: > > > Add state verification for the TypeC port mode wrt. the port's AUX power > > > well enabling/disabling. Also check the correctness of changing the port > > > mode: > > > - When enabling/disabling the AUX power well for a TypeC port we must hold > > > already the TypeC port lock. > > > - When changing the TypeC port mode the port's AUX power domain must be > > > disabled. > > > > > > Cc: José Roberto de Souza <jose.souza@intel.com> > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > Signed-off-by: Imre Deak <imre.deak@intel.com> > > > --- > > > .../drm/i915/display/intel_display_power.c | 99 +++++++++++++++++-- > > > drivers/gpu/drm/i915/display/intel_tc.c | 2 + > > > drivers/gpu/drm/i915/display/intel_tc.h | 10 +- > > > 3 files changed, 102 insertions(+), 9 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c b/drivers/gpu/drm/i915/display/intel_display_power.c > > > index fd13cd68deae..f4613e10c3b3 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_display_power.c > > > +++ b/drivers/gpu/drm/i915/display/intel_display_power.c > > > @@ -17,6 +17,7 @@ > > > #include "intel_drv.h" > > > #include "intel_hotplug.h" > > > #include "intel_sideband.h" > > > +#include "intel_tc.h" > > > > > > bool intel_display_power_well_is_enabled(struct drm_i915_private *dev_priv, > > > enum i915_power_well_id power_well_id); > > > @@ -447,26 +448,110 @@ icl_combo_phy_aux_power_well_disable(struct drm_i915_private *dev_priv, > > > #define ICL_TBT_AUX_PW_TO_CH(pw_idx) \ > > > ((pw_idx) - ICL_PW_CTL_IDX_AUX_TBT1 + AUX_CH_C) > > > > > > +static enum aux_ch icl_tc_phy_aux_ch(struct drm_i915_private *dev_priv, > > > + struct i915_power_well *power_well) > > > +{ > > > + int pw_idx = power_well->desc->hsw.idx; > > > + > > > + return power_well->desc->hsw.is_tc_tbt ? ICL_TBT_AUX_PW_TO_CH(pw_idx) : > > > + ICL_AUX_PW_TO_CH(pw_idx); > > > +} > > > + > > > +#if IS_ENABLED(CONFIG_DRM_I915_DEBUG_RUNTIME_PM) > > > + > > > +static u64 async_put_domains_mask(struct i915_power_domains *power_domains); > > > + > > > +static int power_well_async_ref_count(struct drm_i915_private *dev_priv, > > > + struct i915_power_well *power_well) > > > +{ > > > + enum intel_display_power_domain domain; > > > + u64 async_domains = async_put_domains_mask(&dev_priv->power_domains); > > > + int refs = 0; > > > + > > > + for_each_power_domain(domain, power_well->desc->domains) > > > + refs += !!(async_domains & BIT_ULL(domain)); > > > + > > > + WARN_ON(refs > power_well->count); > > > > hweight64() not sufficient here? > > Ah, just return hweight64(power_well->desc->domains & async_domains)? > Yep, that would work, didn't occur to me.. > > > > > > + > > > + return refs; > > > +} > > > + > > > +static void icl_tc_port_assert_ref_held(struct drm_i915_private *dev_priv, > > > + struct i915_power_well *power_well) > > > +{ > > > + enum aux_ch aux_ch = icl_tc_phy_aux_ch(dev_priv, power_well); > > > + struct intel_digital_port *dig_port = NULL; > > > + struct intel_encoder *encoder; > > > + > > > + /* Bypass the check if all references are released asynchronously */ > > > + if (power_well_async_ref_count(dev_priv, power_well) == > > > + power_well->count) > > > + return; > > > + > > > + aux_ch = icl_tc_phy_aux_ch(dev_priv, power_well); > > > + > > > + for_each_intel_encoder(&dev_priv->drm, encoder) { > > > + if (!intel_port_is_tc(dev_priv, encoder->port)) > > > + continue; > > > + > > > + /* We'll check the MST primary port */ > > > + if (encoder->type == INTEL_OUTPUT_DP_MST) > > > + continue; > > > + > > > + dig_port = enc_to_dig_port(&encoder->base); > > > + if (WARN_ON(!dig_port)) > > > + continue; > > > + > > > + if (dig_port->aux_ch != aux_ch) { > > > + dig_port = NULL; > > > + continue; > > > + } > > > + > > > + break; > > > + } > > > + > > > + if (WARN_ON(!dig_port)) > > > + return; > > > + > > > + WARN_ON(!intel_tc_port_ref_held(dig_port)); > > > +} > > > + > > > +#else > > > + > > > +static void icl_tc_port_assert_ref_held(struct drm_i915_private *dev_priv, > > > + struct i915_power_well *power_well) > > > +{ > > > +} > > > + > > > +#endif > > > + > > > static void > > > icl_tc_phy_aux_power_well_enable(struct drm_i915_private *dev_priv, > > > struct i915_power_well *power_well) > > > { > > > - int pw_idx = power_well->desc->hsw.idx; > > > - bool is_tbt = power_well->desc->hsw.is_tc_tbt; > > > - enum aux_ch aux_ch; > > > + enum aux_ch aux_ch = icl_tc_phy_aux_ch(dev_priv, power_well); > > > u32 val; > > > > > > - aux_ch = is_tbt ? ICL_TBT_AUX_PW_TO_CH(pw_idx) : > > > - ICL_AUX_PW_TO_CH(pw_idx); > > > + icl_tc_port_assert_ref_held(dev_priv, power_well); > > > + > > > val = I915_READ(DP_AUX_CH_CTL(aux_ch)); > > > val &= ~DP_AUX_CH_CTL_TBT_IO; > > > - if (is_tbt) > > > + if (power_well->desc->hsw.is_tc_tbt) > > > val |= DP_AUX_CH_CTL_TBT_IO; > > > I915_WRITE(DP_AUX_CH_CTL(aux_ch), val); > > > > > > hsw_power_well_enable(dev_priv, power_well); > > > } > > > > > > +static void > > > +icl_tc_phy_aux_power_well_disable(struct drm_i915_private *dev_priv, > > > + struct i915_power_well *power_well) > > > +{ > > > + icl_tc_port_assert_ref_held(dev_priv, power_well); > > > + > > > + hsw_power_well_disable(dev_priv, power_well); > > > +} > > > + > > > /* > > > * We should only use the power well if we explicitly asked the hardware to > > > * enable it, so check if it's enabled and also check if we've requested it to > > > @@ -3119,7 +3204,7 @@ static const struct i915_power_well_ops icl_combo_phy_aux_power_well_ops = { > > > static const struct i915_power_well_ops icl_tc_phy_aux_power_well_ops = { > > > .sync_hw = hsw_power_well_sync_hw, > > > .enable = icl_tc_phy_aux_power_well_enable, > > > - .disable = hsw_power_well_disable, > > > + .disable = icl_tc_phy_aux_power_well_disable, > > > .is_enabled = hsw_power_well_enabled, > > > }; > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_tc.c b/drivers/gpu/drm/i915/display/intel_tc.c > > > index c8904911e841..6c43becf97f7 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_tc.c > > > +++ b/drivers/gpu/drm/i915/display/intel_tc.c > > > @@ -303,6 +303,8 @@ static void intel_tc_port_reset_mode(struct intel_digital_port *dig_port, > > > enum tc_port_mode old_tc_mode = dig_port->tc_mode; > > > > > > intel_display_power_flush_work(dev_priv); > > > + WARN_ON(intel_display_power_is_enabled(dev_priv, > > > + intel_aux_power_domain(dig_port))); > > > > > > icl_tc_phy_disconnect(dig_port); > > > icl_tc_phy_connect(dig_port, required_lanes); > > > diff --git a/drivers/gpu/drm/i915/display/intel_tc.h b/drivers/gpu/drm/i915/display/intel_tc.h > > > index 31af7be96070..8adc107cdbcb 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_tc.h > > > +++ b/drivers/gpu/drm/i915/display/intel_tc.h > > > @@ -7,8 +7,8 @@ > > > #define __INTEL_TC_H__ > > > > > > #include <linux/types.h> > > > - > > > -struct intel_digital_port; > > > +#include <linux/mutex.h> > > > +#include "intel_drv.h" > > > > > > void icl_tc_phy_disconnect(struct intel_digital_port *dig_port); > > > > > > @@ -23,6 +23,12 @@ void intel_tc_port_get_link(struct intel_digital_port *dig_port, > > > int required_lanes); > > > void intel_tc_port_put_link(struct intel_digital_port *dig_port); > > > > > > +static inline int intel_tc_port_ref_held(struct intel_digital_port *dig_port) > > > +{ > > > + return mutex_is_locked(&dig_port->tc_lock) || > > > + dig_port->tc_link_refcount; > > > > This will allow tc_lock to not be held if the refcount is elevated. Was > > that the intention? The commit message says the lock should always be > > held. > > Right, the commit message is not precise, it should be: > > - When enabling/disabling the AUX power well for a TypeC port we must hold > the TypeC port lock - the case for AUX transfers - or hold a Type C > port link reference - the case for modeset enabling/disabling. > > Does that make sense? Yeah, I guess that's fine. Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > Should we do a lockdep assert here if lockdep is enabled? > > If the above criteria for enabling/disabling the power wells sounds ok, then > mutex_is_locked(tc_lock) || tc_link_refcount would be enough. > > > > > > +} > > > + > > > void intel_tc_port_init(struct intel_digital_port *dig_port, bool is_legacy); > > > > > > #endif /* __INTEL_TC_H__ */ > > > -- > > > 2.17.1 > > > > > > _______________________________________________ > > > Intel-gfx mailing list > > > Intel-gfx@lists.freedesktop.org > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > -- > > Ville Syrjälä > > Intel
diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c b/drivers/gpu/drm/i915/display/intel_display_power.c index fd13cd68deae..f4613e10c3b3 100644 --- a/drivers/gpu/drm/i915/display/intel_display_power.c +++ b/drivers/gpu/drm/i915/display/intel_display_power.c @@ -17,6 +17,7 @@ #include "intel_drv.h" #include "intel_hotplug.h" #include "intel_sideband.h" +#include "intel_tc.h" bool intel_display_power_well_is_enabled(struct drm_i915_private *dev_priv, enum i915_power_well_id power_well_id); @@ -447,26 +448,110 @@ icl_combo_phy_aux_power_well_disable(struct drm_i915_private *dev_priv, #define ICL_TBT_AUX_PW_TO_CH(pw_idx) \ ((pw_idx) - ICL_PW_CTL_IDX_AUX_TBT1 + AUX_CH_C) +static enum aux_ch icl_tc_phy_aux_ch(struct drm_i915_private *dev_priv, + struct i915_power_well *power_well) +{ + int pw_idx = power_well->desc->hsw.idx; + + return power_well->desc->hsw.is_tc_tbt ? ICL_TBT_AUX_PW_TO_CH(pw_idx) : + ICL_AUX_PW_TO_CH(pw_idx); +} + +#if IS_ENABLED(CONFIG_DRM_I915_DEBUG_RUNTIME_PM) + +static u64 async_put_domains_mask(struct i915_power_domains *power_domains); + +static int power_well_async_ref_count(struct drm_i915_private *dev_priv, + struct i915_power_well *power_well) +{ + enum intel_display_power_domain domain; + u64 async_domains = async_put_domains_mask(&dev_priv->power_domains); + int refs = 0; + + for_each_power_domain(domain, power_well->desc->domains) + refs += !!(async_domains & BIT_ULL(domain)); + + WARN_ON(refs > power_well->count); + + return refs; +} + +static void icl_tc_port_assert_ref_held(struct drm_i915_private *dev_priv, + struct i915_power_well *power_well) +{ + enum aux_ch aux_ch = icl_tc_phy_aux_ch(dev_priv, power_well); + struct intel_digital_port *dig_port = NULL; + struct intel_encoder *encoder; + + /* Bypass the check if all references are released asynchronously */ + if (power_well_async_ref_count(dev_priv, power_well) == + power_well->count) + return; + + aux_ch = icl_tc_phy_aux_ch(dev_priv, power_well); + + for_each_intel_encoder(&dev_priv->drm, encoder) { + if (!intel_port_is_tc(dev_priv, encoder->port)) + continue; + + /* We'll check the MST primary port */ + if (encoder->type == INTEL_OUTPUT_DP_MST) + continue; + + dig_port = enc_to_dig_port(&encoder->base); + if (WARN_ON(!dig_port)) + continue; + + if (dig_port->aux_ch != aux_ch) { + dig_port = NULL; + continue; + } + + break; + } + + if (WARN_ON(!dig_port)) + return; + + WARN_ON(!intel_tc_port_ref_held(dig_port)); +} + +#else + +static void icl_tc_port_assert_ref_held(struct drm_i915_private *dev_priv, + struct i915_power_well *power_well) +{ +} + +#endif + static void icl_tc_phy_aux_power_well_enable(struct drm_i915_private *dev_priv, struct i915_power_well *power_well) { - int pw_idx = power_well->desc->hsw.idx; - bool is_tbt = power_well->desc->hsw.is_tc_tbt; - enum aux_ch aux_ch; + enum aux_ch aux_ch = icl_tc_phy_aux_ch(dev_priv, power_well); u32 val; - aux_ch = is_tbt ? ICL_TBT_AUX_PW_TO_CH(pw_idx) : - ICL_AUX_PW_TO_CH(pw_idx); + icl_tc_port_assert_ref_held(dev_priv, power_well); + val = I915_READ(DP_AUX_CH_CTL(aux_ch)); val &= ~DP_AUX_CH_CTL_TBT_IO; - if (is_tbt) + if (power_well->desc->hsw.is_tc_tbt) val |= DP_AUX_CH_CTL_TBT_IO; I915_WRITE(DP_AUX_CH_CTL(aux_ch), val); hsw_power_well_enable(dev_priv, power_well); } +static void +icl_tc_phy_aux_power_well_disable(struct drm_i915_private *dev_priv, + struct i915_power_well *power_well) +{ + icl_tc_port_assert_ref_held(dev_priv, power_well); + + hsw_power_well_disable(dev_priv, power_well); +} + /* * We should only use the power well if we explicitly asked the hardware to * enable it, so check if it's enabled and also check if we've requested it to @@ -3119,7 +3204,7 @@ static const struct i915_power_well_ops icl_combo_phy_aux_power_well_ops = { static const struct i915_power_well_ops icl_tc_phy_aux_power_well_ops = { .sync_hw = hsw_power_well_sync_hw, .enable = icl_tc_phy_aux_power_well_enable, - .disable = hsw_power_well_disable, + .disable = icl_tc_phy_aux_power_well_disable, .is_enabled = hsw_power_well_enabled, }; diff --git a/drivers/gpu/drm/i915/display/intel_tc.c b/drivers/gpu/drm/i915/display/intel_tc.c index c8904911e841..6c43becf97f7 100644 --- a/drivers/gpu/drm/i915/display/intel_tc.c +++ b/drivers/gpu/drm/i915/display/intel_tc.c @@ -303,6 +303,8 @@ static void intel_tc_port_reset_mode(struct intel_digital_port *dig_port, enum tc_port_mode old_tc_mode = dig_port->tc_mode; intel_display_power_flush_work(dev_priv); + WARN_ON(intel_display_power_is_enabled(dev_priv, + intel_aux_power_domain(dig_port))); icl_tc_phy_disconnect(dig_port); icl_tc_phy_connect(dig_port, required_lanes); diff --git a/drivers/gpu/drm/i915/display/intel_tc.h b/drivers/gpu/drm/i915/display/intel_tc.h index 31af7be96070..8adc107cdbcb 100644 --- a/drivers/gpu/drm/i915/display/intel_tc.h +++ b/drivers/gpu/drm/i915/display/intel_tc.h @@ -7,8 +7,8 @@ #define __INTEL_TC_H__ #include <linux/types.h> - -struct intel_digital_port; +#include <linux/mutex.h> +#include "intel_drv.h" void icl_tc_phy_disconnect(struct intel_digital_port *dig_port); @@ -23,6 +23,12 @@ void intel_tc_port_get_link(struct intel_digital_port *dig_port, int required_lanes); void intel_tc_port_put_link(struct intel_digital_port *dig_port); +static inline int intel_tc_port_ref_held(struct intel_digital_port *dig_port) +{ + return mutex_is_locked(&dig_port->tc_lock) || + dig_port->tc_link_refcount; +} + void intel_tc_port_init(struct intel_digital_port *dig_port, bool is_legacy); #endif /* __INTEL_TC_H__ */
Add state verification for the TypeC port mode wrt. the port's AUX power well enabling/disabling. Also check the correctness of changing the port mode: - When enabling/disabling the AUX power well for a TypeC port we must hold already the TypeC port lock. - When changing the TypeC port mode the port's AUX power domain must be disabled. Cc: José Roberto de Souza <jose.souza@intel.com> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> Signed-off-by: Imre Deak <imre.deak@intel.com> --- .../drm/i915/display/intel_display_power.c | 99 +++++++++++++++++-- drivers/gpu/drm/i915/display/intel_tc.c | 2 + drivers/gpu/drm/i915/display/intel_tc.h | 10 +- 3 files changed, 102 insertions(+), 9 deletions(-)