| Message ID | 20240223203216.15210-1-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] drm/i915: Don't explode when the dig port we don't have an AUX CH | expand |
On Fri, Feb 23, 2024 at 10:32:15PM +0200, Ville Syrjala wrote: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > The icl+ power well code currently assumes that every AUX power > well maps to an encoder which is using said power well. That is > by no menas guaranteed as we: > - only register encoders for ports declared in the VBT > - combo PHY HDMI-only encoder no longer get an AUX CH since > commit 9856308c94ca ("drm/i915: Only populate aux_ch if really needed") > > However we have places such as intel_power_domains_sanitize_state() > that blindly traverse all the possible power wells. So these bits > of code may very well encounbter an aux power well with no associated > encoder. > > In this particular case the BIOS seems to have left one AUX power > well enabled even though we're dealing with a HDMI only encoder > on a combo PHY. We then proceed to turn off said power well and > explode when we can't find a matching encoder. As a short term fix > we should be able to just skip the PHY related parts of the power > well programming since we know this situation can only happen with > combo PHYs. > > Another option might be to go back to always picking an AUX CH for > all encoders. However I'm a bit wary about that since we might in > theory end up conflicting with the VBT AUX CH assignment. Also > that wouldn't help with encoders not declared in the VBT, should > we ever need to poke the corresponding power wells. > > Longer term we need to figure out what the actual relationship > is between the PHY vs. AUX CH vs. AUX power well. Currently this > is entirely unclear. This is unspecified, so the only way would be to test an actual platform with an alternative AUX CH VBT setting (on a DDI platform). My current assumption is that this alternative AUX CH determines: - The AUX power well to be enabled for an AUX transfer - The AUX CH data/ctl registers to be used for an AUX transfer Otoh, for the (overloaded) power control for the main lane functionality it is not the alternative AUX CH power well, rather the given DDIs direct mapped/own AUX CH power well what would be required. The driver doesn't make a distinction in this now, since it's unspecified. I think cross connecting AUX CHs wouldn't really work on DDI platforms for this reason (at least on TC DDIs/PHYs/AUX CHs). For the PHY workarounds which are part of the AUX power well programming sequence, it is the PHY connected to the given DDI (so not affected by any alternative AUX CH setting), which needs to be programmed. As above this is also unspeficied, so just my assumption. > Cc: stable@vger.kernel.org > Fixes: 9856308c94ca ("drm/i915: Only populate aux_ch if really needed") > Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/10184 > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > .../drm/i915/display/intel_display_power_well.c | 17 ++++++++++++++--- > 1 file changed, 14 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_display_power_well.c b/drivers/gpu/drm/i915/display/intel_display_power_well.c > index 47cd6bb04366..06900ff307b2 100644 > --- a/drivers/gpu/drm/i915/display/intel_display_power_well.c > +++ b/drivers/gpu/drm/i915/display/intel_display_power_well.c > @@ -246,7 +246,14 @@ static enum phy icl_aux_pw_to_phy(struct drm_i915_private *i915, > enum aux_ch aux_ch = icl_aux_pw_to_ch(power_well); > struct intel_digital_port *dig_port = aux_ch_to_digital_port(i915, aux_ch); > > - return intel_port_to_phy(i915, dig_port->base.port); > + /* > + * FIXME should we care about the (VBT defined) dig_port->aux_ch > + * relationship or should this be purely defined by the hardware layout? > + * Currently if the port doesn't appear in the VBT, or if it's declared > + * as HDMI-only and routed to a combo PHY, the encoder either won't be > + * present at all or it will not have an aux_ch assigned. > + */ > + return dig_port ? intel_port_to_phy(i915, dig_port->base.port) : PHY_NONE; Yes, if it's not known which PHY is used in connection with an AUX CH (which in theory could be remapped via VBT), then the WA can't be applied either; so on both patches: Reviewed-by: Imre Deak <imre.deak@intel.com> > } > > static void hsw_wait_for_power_well_enable(struct drm_i915_private *dev_priv, > @@ -414,7 +421,8 @@ icl_combo_phy_aux_power_well_enable(struct drm_i915_private *dev_priv, > > intel_de_rmw(dev_priv, regs->driver, 0, HSW_PWR_WELL_CTL_REQ(pw_idx)); > > - if (DISPLAY_VER(dev_priv) < 12) > + /* FIXME this is a mess */ > + if (phy != PHY_NONE) > intel_de_rmw(dev_priv, ICL_PORT_CL_DW12(phy), > 0, ICL_LANE_ENABLE_AUX); > > @@ -437,7 +445,10 @@ icl_combo_phy_aux_power_well_disable(struct drm_i915_private *dev_priv, > > drm_WARN_ON(&dev_priv->drm, !IS_ICELAKE(dev_priv)); > > - intel_de_rmw(dev_priv, ICL_PORT_CL_DW12(phy), ICL_LANE_ENABLE_AUX, 0); > + /* FIXME this is a mess */ > + if (phy != PHY_NONE) > + intel_de_rmw(dev_priv, ICL_PORT_CL_DW12(phy), > + ICL_LANE_ENABLE_AUX, 0); > > intel_de_rmw(dev_priv, regs->driver, HSW_PWR_WELL_CTL_REQ(pw_idx), 0); > > -- > 2.43.0 >
diff --git a/drivers/gpu/drm/i915/display/intel_display_power_well.c b/drivers/gpu/drm/i915/display/intel_display_power_well.c index 47cd6bb04366..06900ff307b2 100644 --- a/drivers/gpu/drm/i915/display/intel_display_power_well.c +++ b/drivers/gpu/drm/i915/display/intel_display_power_well.c @@ -246,7 +246,14 @@ static enum phy icl_aux_pw_to_phy(struct drm_i915_private *i915, enum aux_ch aux_ch = icl_aux_pw_to_ch(power_well); struct intel_digital_port *dig_port = aux_ch_to_digital_port(i915, aux_ch); - return intel_port_to_phy(i915, dig_port->base.port); + /* + * FIXME should we care about the (VBT defined) dig_port->aux_ch + * relationship or should this be purely defined by the hardware layout? + * Currently if the port doesn't appear in the VBT, or if it's declared + * as HDMI-only and routed to a combo PHY, the encoder either won't be + * present at all or it will not have an aux_ch assigned. + */ + return dig_port ? intel_port_to_phy(i915, dig_port->base.port) : PHY_NONE; } static void hsw_wait_for_power_well_enable(struct drm_i915_private *dev_priv, @@ -414,7 +421,8 @@ icl_combo_phy_aux_power_well_enable(struct drm_i915_private *dev_priv, intel_de_rmw(dev_priv, regs->driver, 0, HSW_PWR_WELL_CTL_REQ(pw_idx)); - if (DISPLAY_VER(dev_priv) < 12) + /* FIXME this is a mess */ + if (phy != PHY_NONE) intel_de_rmw(dev_priv, ICL_PORT_CL_DW12(phy), 0, ICL_LANE_ENABLE_AUX); @@ -437,7 +445,10 @@ icl_combo_phy_aux_power_well_disable(struct drm_i915_private *dev_priv, drm_WARN_ON(&dev_priv->drm, !IS_ICELAKE(dev_priv)); - intel_de_rmw(dev_priv, ICL_PORT_CL_DW12(phy), ICL_LANE_ENABLE_AUX, 0); + /* FIXME this is a mess */ + if (phy != PHY_NONE) + intel_de_rmw(dev_priv, ICL_PORT_CL_DW12(phy), + ICL_LANE_ENABLE_AUX, 0); intel_de_rmw(dev_priv, regs->driver, HSW_PWR_WELL_CTL_REQ(pw_idx), 0);