On Thu, Feb 29, 2024 at 10:03:56PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
>
> We don't actually know whether we should be picking the PHY
> simply based on the AUX_CH/power well, or based on the VBT
> defined AUX_CH->DDI->PHY relationship. At the moment we are
> doing the former for the ANAOVRD workaround, and the latter
> for the ICL_LANE_ENABLE_AUX override. Windows seems to use the
> first approach for everything. So let's unify this to follow
> that same approach for both.
>
> Eventually we should try to figure out which is actually
> correct, or whether any of this even matters (ie. whether there
> are any real machines where the DDI and its AUX_CH do not match
> 1:1).
>
> Note that this also changes the behaviour if we do end up
> poking an AUX power well not associated with any port (as
> per VBT). Previously we would have skipped the PHY register
> write, but now we always write it.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> ---
> .../i915/display/intel_display_power_well.c | 21 +++++++++++--------
> 1 file changed, 12 insertions(+), 9 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 a1edac6ce31f..f25ad7d2c784 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_power_well.c
> +++ b/drivers/gpu/drm/i915/display/intel_display_power_well.c
> @@ -419,10 +419,12 @@ 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));
>
> - /* FIXME this is a mess */
> - if (phy != PHY_NONE)
> - intel_de_rmw(dev_priv, ICL_PORT_CL_DW12(phy),
> - 0, ICL_LANE_ENABLE_AUX);
> + /*
> + * FIXME not sure if we should derive the PHY from the pw_idx, or
> + * from the VBT defined AUX_CH->DDI->PHY mapping.
> + */
> + intel_de_rmw(dev_priv, ICL_PORT_CL_DW12(ICL_AUX_PW_TO_PHY(pw_idx)),
> + 0, ICL_LANE_ENABLE_AUX);
I wonder if the same PW -> PHY mapping could be used in
icl_aux_power_well_enable/disable() as well (to make it more consistent
if there is no encoder using the PW). Cross connecting combo, TC PHY AUX
channels doesn't work anyway I think (maybe this could be actually
checked in intel_bios_dp_aux_ch()).
On the patchset:
Reviewed-by: Imre Deak <imre.deak@xxxxxxxxx>
>
> hsw_wait_for_power_well_enable(dev_priv, power_well, false);
>
> @@ -439,14 +441,15 @@ icl_combo_phy_aux_power_well_disable(struct drm_i915_private *dev_priv,
> {
> const struct i915_power_well_regs *regs = power_well->desc->ops->regs;
> int pw_idx = i915_power_well_instance(power_well)->hsw.idx;
> - enum phy phy = icl_aux_pw_to_phy(dev_priv, power_well);
>
> drm_WARN_ON(&dev_priv->drm, !IS_ICELAKE(dev_priv));
>
> - /* FIXME this is a mess */
> - if (phy != PHY_NONE)
> - intel_de_rmw(dev_priv, ICL_PORT_CL_DW12(phy),
> - ICL_LANE_ENABLE_AUX, 0);
> + /*
> + * FIXME not sure if we should derive the PHY from the pw_idx, or
> + * from the VBT defined AUX_CH->DDI->PHY mapping.
> + */
> + intel_de_rmw(dev_priv, ICL_PORT_CL_DW12(ICL_AUX_PW_TO_PHY(pw_idx)),
> + ICL_LANE_ENABLE_AUX, 0);
>
> intel_de_rmw(dev_priv, regs->driver, HSW_PWR_WELL_CTL_REQ(pw_idx), 0);
>
> --
> 2.43.0
>
