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

[Imre Deak <imre.deak@intel.com>]

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()).

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
>