Re: [PATCH 3/4] drm/i915: Use pw_idx to derive PHY for ICL_LANE_ENABLE_AUX override

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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ä <ville.syrjala@linux.intel.com>
> > 
> > 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@linux.intel.com>
> > ---
> >  .../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 <imre.deak@intel.com>
> 
> 
> >  
> >  	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
> > 

-- 
Ville Syrjälä
Intel