Re: [PATCH v4 1/5] drm/i915/gen11: Start distinguishing 'phy' from 'port'

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

On Thu, Jul 04, 2019 at 12:18:11PM +0300, Ville Syrjälä wrote:
> On Wed, Jul 03, 2019 at 04:37:32PM -0700, Matt Roper wrote:
> > Our past DDI-based Intel platforms have had a fixed DDI<->PHY mapping.
> > Because of this, both the bspec documentation and our i915 code has used
> > the term "port" when talking about either DDI's or PHY's; it was always
> > easy to tell what terms like "Port A" were referring to from the
> > context.
> > 
> > Unfortunately this is starting to break down now that EHL allows PHY-A
> > to be driven by either DDI-A or DDI-D.  Is a setup with DDI-D driving
> > PHY-A considered "Port A" or "Port D?"  The answer depends on which
> > register we're working with, and even the bspec doesn't do a great job
> > of clarifying this.
> > 
> > Let's try to be more explicit about whether we're talking about the DDI
> > or the PHY on gen11+ by using 'port' to refer to the DDI and creating a
> > new 'enum phy' namespace to refer to the PHY in use.
> > 
> > This patch just adds the new PHY namespace, new phy-based versions of
> > intel_port_is_*(), and a helper to convert a port to a PHY.
> > Transitioning various areas of the code over to using the PHY namespace
> > will be done in subsequent patches to make review easier.  We'll remove
> > the intel_port_is_*() functions at the end of the series when we
> > transition all callers over to using the PHY-based versions.
> > 
> > v2:
> >  - Convert a few more 'port' uses to 'phy.' (Sparse)
> > 
> > v3:
> >  - Switch DDI_CLK_SEL() back to 'port.' (Jose)
> >  - Add a code comment clarifying why DPCLKA_CFGCR0_ICL needs to use PHY
> >    for its bit definitions, even though the register description is
> >    given in terms of DDI.
> >  - To avoid confusion, switch CNL's DPCLKA_CFGCR0 defines back to using
> >    port and create separate ICL+ definitions that work in terms of PHY.
> > 
> > v4:
> >  - Rebase and resolve conflicts with Imre's TC series.
> >  - This patch now just adds the namespace and a few convenience
> >    functions; the important changes are now split out into separate
> >    patches to make review easier.
> > 
> > Suggested-by: Ville Syrjala <ville.syrjala@linux.intel.com>
> > Cc: José Roberto de Souza <jose.souza@intel.com>
> > Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: Imre Deak <imre.deak@intel.com>
> > Cc: Jani Nikula <jani.nikula@intel.com>
> > Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_display.c | 32 +++++++++++++++++++-
> >  drivers/gpu/drm/i915/display/intel_display.h | 16 ++++++++++
> >  drivers/gpu/drm/i915/intel_drv.h             |  2 ++
> >  3 files changed, 49 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > index 919f5ac844c8..4a85abef93e7 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -6663,6 +6663,20 @@ bool intel_port_is_combophy(struct drm_i915_private *dev_priv, enum port port)
> >  	return false;
> >  }
> >  
> > +bool intel_phy_is_combo(struct drm_i915_private *dev_priv, enum phy phy)
> > +{
> > +	if (phy == PHY_NONE)
> > +		return false;
> > +
> > +	if (IS_ELKHARTLAKE(dev_priv))
> > +		return phy <= PHY_C;
> > +
> > +	if (INTEL_GEN(dev_priv) >= 11)
> > +		return phy <= PHY_B;
> > +
> > +	return false;
> > +}
> > +
> >  bool intel_port_is_tc(struct drm_i915_private *dev_priv, enum port port)
> >  {
> >  	if (INTEL_GEN(dev_priv) >= 11 && !IS_ELKHARTLAKE(dev_priv))
> > @@ -6671,9 +6685,25 @@ bool intel_port_is_tc(struct drm_i915_private *dev_priv, enum port port)
> >  	return false;
> >  }
> >  
> > +bool intel_phy_is_tc(struct drm_i915_private *dev_priv, enum phy phy)
> > +{
> > +	if (INTEL_GEN(dev_priv) >= 11 && !IS_ELKHARTLAKE(dev_priv))
> > +		return phy >= PHY_C && phy <= PHY_F;
> > +
> > +	return false;
> > +}
> > +
> > +enum phy intel_port_to_phy(struct drm_i915_private *i915, enum port port)
> > +{
> > +	if (IS_ELKHARTLAKE(i915) && port == PORT_D)
> > +		return PHY_A;
> > +
> > +	return (enum phy)port;
> > +}
> > +
> >  enum tc_port intel_port_to_tc(struct drm_i915_private *dev_priv, enum port port)
> >  {
> > -	if (!intel_port_is_tc(dev_priv, port))
> > +	if (!intel_phy_is_tc(dev_priv, intel_port_to_phy(dev_priv, port)))
> >  		return PORT_TC_NONE;
> >  
> >  	return port - PORT_C;
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.h b/drivers/gpu/drm/i915/display/intel_display.h
> > index d296556ed82e..d53285fb883f 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.h
> > +++ b/drivers/gpu/drm/i915/display/intel_display.h
> > @@ -228,6 +228,21 @@ struct intel_link_m_n {
> >  	u32 link_n;
> >  };
> >  
> > +enum phy {
> > +	PHY_NONE = -1,
> > +
> > +	PHY_A = 0,
> > +	PHY_B,
> > +	PHY_C,
> > +	PHY_D,
> > +	PHY_E,
> > +	PHY_F,
> > +
> > +	I915_MAX_PHYS
> > +};
> 
> I was pondering if we could eventually do something like:
> 
> enum phy {
> 	PHY_COMBO_A = 0,
> 	PHY_COMBO_B,
> 	...
> 
> 	PHY_TC_1,
> 	PHY_TC_2,
> 	...
> };
> 
> and probably also add encoder->phy so we can contain
> that port<->phy mapping logic in the encoder init.
> I think that should work more or less fine since I
> don't think PHY_TC_1 needs to have any specific value.

Another option might be to have overlapping values such that
COMBO_A == TC1 and just have a separate flag to indicate which
type we're dealing with.

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx