diff mbox series

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

Message ID 20240229200357.7969-3-ville.syrjala@linux.intel.com (mailing list archive)
State New, archived
Headers show
Series [1/4] drm/i915: Rename ICL_AUX_ANAOVRD1 to ICL_PORT_TX_DW6_AUX | expand

Commit Message

Ville Syrjälä Feb. 29, 2024, 8:03 p.m. UTC 
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(-)

Comments

Imre Deak March 5, 2024, 3:14 p.m. UTC | #1 
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
>
Ville Syrjälä March 7, 2024, 5:58 p.m. UTC | #2 
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
> >
diff mbox series

Patch

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);
 
 	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);