On Tue, Mar 05, 2024 at 05:14:54PM +0200, Imre Deak wrote:
> On Thu, Feb 29, 2024 at 10:03:56PM +0200, Ville Syrjala wrote:
> > From: Ville Syrjälä
> >
> > 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ä
> > ---
> > .../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()).
We can't tell the PHY type from the pw, unless we add that
information to the power well definitions.
>
> On the patchset:
> Reviewed-by: Imre Deak
>
>
> >
> > 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(_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
> >
--
Ville Syrjälä
Intel
