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

Commit Message

Matt Roper July 3, 2019, 11:37 p.m. UTC

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 <[e-mail]>
Cc: José Roberto de Souza <[e-mail]>
Cc: Lucas De Marchi <[e-mail]>
Cc: Ville Syrjälä <[e-mail]>
Cc: Imre Deak <[e-mail]>
Cc: Jani Nikula <[e-mail]>
Signed-off-by: Matt Roper <[e-mail]>
---
 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(-)

Comments

Ville Syrjala July 4, 2019, 9:18 a.m. UTC | #1

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 <[e-mail]>
> Cc: José Roberto de Souza <[e-mail]>
> Cc: Lucas De Marchi <[e-mail]>
> Cc: Ville Syrjälä <[e-mail]>
> Cc: Imre Deak <[e-mail]>
> Cc: Jani Nikula <[e-mail]>
> Signed-off-by: Matt Roper <[e-mail]>
> ---
>  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.

Unfortunaltey I don't have a great idea how to do the
same for the DDIs since there the number of combo DDIs
changes but we still need the PORT_TC1 (assuming we had
one) to be DDI_<last combo DDI> + 1. One random silly
idea was to decouple the enum port from the register
definitions by having just some kind of
encoder->port_index for those. But that doesn't feel
entirely great either.

Anyways, something to think about in the future perhaps.

Patch is
Reviewed-by: Ville Syrjälä <[e-mail]>

> +
> +#define phy_name(a) ((a) + 'A')
> +
>  #define for_each_pipe(__dev_priv, __p) \
>  	for ((__p) = 0; (__p) < INTEL_INFO(__dev_priv)->num_pipes; (__p)++)
>  
> @@ -356,5 +371,6 @@ void lpt_disable_clkout_dp(struct drm_i915_private *dev_priv);
>  u32 intel_plane_fb_max_stride(struct drm_i915_private *dev_priv,
>  			      u32 pixel_format, u64 modifier);
>  bool intel_plane_can_remap(const struct intel_plane_state *plane_state);
> +enum phy intel_port_to_phy(struct drm_i915_private *i915, enum port port);
>  
>  #endif
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 24c63ed45c6f..815c26c0b98c 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1493,7 +1493,9 @@ void intel_encoder_destroy(struct drm_encoder *encoder);
>  struct drm_display_mode *
>  intel_encoder_current_mode(struct intel_encoder *encoder);
>  bool intel_port_is_combophy(struct drm_i915_private *dev_priv, enum port port);
> +bool intel_phy_is_combo(struct drm_i915_private *dev_priv, enum phy phy);
>  bool intel_port_is_tc(struct drm_i915_private *dev_priv, enum port port);
> +bool intel_phy_is_tc(struct drm_i915_private *dev_priv, enum phy phy);
>  enum tc_port intel_port_to_tc(struct drm_i915_private *dev_priv,
>  			      enum port port);
>  int intel_get_pipe_from_crtc_id_ioctl(struct drm_device *dev, void *data,
> -- 
> 2.17.2

Ville Syrjala July 4, 2019, 9:24 a.m. UTC | #2

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 <[e-mail]>
> > Cc: José Roberto de Souza <[e-mail]>
> > Cc: Lucas De Marchi <[e-mail]>
> > Cc: Ville Syrjälä <[e-mail]>
> > Cc: Imre Deak <[e-mail]>
> > Cc: Jani Nikula <[e-mail]>
> > Signed-off-by: Matt Roper <[e-mail]>
> > ---
> >  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 Syrjala July 4, 2019, 3:09 p.m. UTC | #3

On Thu, Jul 04, 2019 at 07:54:26AM -0700, Lucas De Marchi wrote:
> 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 <[e-mail]>
> >> Cc: José Roberto de Souza <[e-mail]>
> >> Cc: Lucas De Marchi <[e-mail]>
> >> Cc: Ville Syrjälä <[e-mail]>
> >> Cc: Imre Deak <[e-mail]>
> >> Cc: Jani Nikula <[e-mail]>
> >> Signed-off-by: Matt Roper <[e-mail]>
> >> ---
> >>  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.
> 
> that's not true. All TC registers are based off the TC phy number.

That's just a trivial (x)-TC1, so I stand by what I said.

> Hence all the conversion we do port_to_tc()... I'd like to remove that
> in future and just stuff the phy index in intel_digital_port, as we
> already do for other tc_phy_* fields (we could add a union there so each
> phy adds its own fields).
> 
> And I'd rather not do the single phy namespace - it doesn't
> play well with TGL and the combo/tc.

I think it would work just fine for that. A single namespace would allow
us to remove all the crazy port->PHY type mapping we have going on
currently. Probably the best alternative would be separate namespaces
for each type with a new enum to identify the type.

> 
> Lucas De Marchi
> 
> >
> >Unfortunaltey I don't have a great idea how to do the
> >same for the DDIs since there the number of combo DDIs
> >changes but we still need the PORT_TC1 (assuming we had
> >one) to be DDI_<last combo DDI> + 1. One random silly
> >idea was to decouple the enum port from the register
> >definitions by having just some kind of
> >encoder->port_index for those. But that doesn't feel
> >entirely great either.
> >
> >Anyways, something to think about in the future perhaps.
> >
> >Patch is
> >Reviewed-by: Ville Syrjälä <[e-mail]>
> >
> >> +
> >> +#define phy_name(a) ((a) + 'A')
> >> +
> >>  #define for_each_pipe(__dev_priv, __p) \
> >>  	for ((__p) = 0; (__p) < INTEL_INFO(__dev_priv)->num_pipes; (__p)++)
> >>
> >> @@ -356,5 +371,6 @@ void lpt_disable_clkout_dp(struct drm_i915_private *dev_priv);
> >>  u32 intel_plane_fb_max_stride(struct drm_i915_private *dev_priv,
> >>  			      u32 pixel_format, u64 modifier);
> >>  bool intel_plane_can_remap(const struct intel_plane_state *plane_state);
> >> +enum phy intel_port_to_phy(struct drm_i915_private *i915, enum port port);
> >>
> >>  #endif
> >> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> >> index 24c63ed45c6f..815c26c0b98c 100644
> >> --- a/drivers/gpu/drm/i915/intel_drv.h
> >> +++ b/drivers/gpu/drm/i915/intel_drv.h
> >> @@ -1493,7 +1493,9 @@ void intel_encoder_destroy(struct drm_encoder *encoder);
> >>  struct drm_display_mode *
> >>  intel_encoder_current_mode(struct intel_encoder *encoder);
> >>  bool intel_port_is_combophy(struct drm_i915_private *dev_priv, enum port port);
> >> +bool intel_phy_is_combo(struct drm_i915_private *dev_priv, enum phy phy);
> >>  bool intel_port_is_tc(struct drm_i915_private *dev_priv, enum port port);
> >> +bool intel_phy_is_tc(struct drm_i915_private *dev_priv, enum phy phy);
> >>  enum tc_port intel_port_to_tc(struct drm_i915_private *dev_priv,
> >>  			      enum port port);
> >>  int intel_get_pipe_from_crtc_id_ioctl(struct drm_device *dev, void *data,
> >> --
> >> 2.17.2
> >
> >-- 
> >Ville Syrjälä
> >Intel

Ville Syrjala July 5, 2019, 10:33 a.m. UTC | #4

On Thu, Jul 04, 2019 at 08:55:51AM -0700, Lucas De Marchi wrote:
> On Thu, Jul 04, 2019 at 06:09:04PM +0300, Ville Syrjälä wrote:
> >On Thu, Jul 04, 2019 at 07:54:26AM -0700, Lucas De Marchi wrote:
> >> 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 <[e-mail]>
> >> >> Cc: José Roberto de Souza <[e-mail]>
> >> >> Cc: Lucas De Marchi <[e-mail]>
> >> >> Cc: Ville Syrjälä <[e-mail]>
> >> >> Cc: Imre Deak <[e-mail]>
> >> >> Cc: Jani Nikula <[e-mail]>
> >> >> Signed-off-by: Matt Roper <[e-mail]>
> >> >> ---
> >> >>  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.
> >>
> >> that's not true. All TC registers are based off the TC phy number.
> >
> >That's just a trivial (x)-TC1, so I stand by what I said.
> 
> EHL and TGL have 3 combo phys. ICL has 2. So TC1 would have to be a
> different value for ICL and the others for this to work.

It would just be an arbitrary number (eg. 8).

> I think we should treat the index as just a number we use to compute the
> right base for the registers in that hw IP.
> 
> >
> >> Hence all the conversion we do port_to_tc()... I'd like to remove that
> >> in future and just stuff the phy index in intel_digital_port, as we
> >> already do for other tc_phy_* fields (we could add a union there so each
> >> phy adds its own fields).
> >>
> >> And I'd rather not do the single phy namespace - it doesn't
> >> play well with TGL and the combo/tc.
> >
> >I think it would work just fine for that. A single namespace would allow
> >us to remove all the crazy port->PHY type mapping we have going on
> >currently. Probably the best alternative would be separate namespaces
> >for each type with a new enum to identify the type.
> 
> My proposal is in the lines of that alternative approach. Save the phy
> type and the phy index in intel_digital_port. And allow each phy to store
> its fields there. Something along:
> 
> struct intel_digital_port {
> 	...
> 	struct {
> 		enum phy_type type;
> 		u8 idx;
> 		union {
> 			struct {
> 				struct mutex lock;   /* protects the TypeC port mode */
> 				intel_wakeref_t lock_wakeref;
> 				int link_refcount;
> 				char tc_port_name[8];
> 				enum tc_port_mode tc_mode;
> 				bool legacy_port;
> 				u8 fia;
> 			} tc;
> 			struct {
> 				...
> 			} combo;
> 			struct {
> 				...
> 			} dpio??;
> 		};
> 	} phy;
> };
> 
> >From a quick look I don't think the idx is relevant enough to have its
> own enum, but I wouldn't mind.
> 
> then no more port->phy everywhere as we have right now and we only
> assign idx to the right value during init. The TC rework we had recently
> makes it nice and I'm starting to do it, but I'd rather merge the TGL
> patches before. For Modular FIA I'm already stashing the fia index
> there (since I had to redo the support due to the TC rework),
> but without the additional struct.
> See https://patchwork.freedesktop.org/series/63175/
> 
> Lucas De Marchi
> 
> >
> >>
> >> Lucas De Marchi
> >>
> >> >
> >> >Unfortunaltey I don't have a great idea how to do the
> >> >same for the DDIs since there the number of combo DDIs
> >> >changes but we still need the PORT_TC1 (assuming we had
> >> >one) to be DDI_<last combo DDI> + 1. One random silly
> >> >idea was to decouple the enum port from the register
> >> >definitions by having just some kind of
> >> >encoder->port_index for those. But that doesn't feel
> >> >entirely great either.
> >> >
> >> >Anyways, something to think about in the future perhaps.
> >> >
> >> >Patch is
> >> >Reviewed-by: Ville Syrjälä <[e-mail]>
> >> >
> >> >> +
> >> >> +#define phy_name(a) ((a) + 'A')
> >> >> +
> >> >>  #define for_each_pipe(__dev_priv, __p) \
> >> >>  	for ((__p) = 0; (__p) < INTEL_INFO(__dev_priv)->num_pipes; (__p)++)
> >> >>
> >> >> @@ -356,5 +371,6 @@ void lpt_disable_clkout_dp(struct drm_i915_private *dev_priv);
> >> >>  u32 intel_plane_fb_max_stride(struct drm_i915_private *dev_priv,
> >> >>  			      u32 pixel_format, u64 modifier);
> >> >>  bool intel_plane_can_remap(const struct intel_plane_state *plane_state);
> >> >> +enum phy intel_port_to_phy(struct drm_i915_private *i915, enum port port);
> >> >>
> >> >>  #endif
> >> >> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> >> >> index 24c63ed45c6f..815c26c0b98c 100644
> >> >> --- a/drivers/gpu/drm/i915/intel_drv.h
> >> >> +++ b/drivers/gpu/drm/i915/intel_drv.h
> >> >> @@ -1493,7 +1493,9 @@ void intel_encoder_destroy(struct drm_encoder *encoder);
> >> >>  struct drm_display_mode *
> >> >>  intel_encoder_current_mode(struct intel_encoder *encoder);
> >> >>  bool intel_port_is_combophy(struct drm_i915_private *dev_priv, enum port port);
> >> >> +bool intel_phy_is_combo(struct drm_i915_private *dev_priv, enum phy phy);
> >> >>  bool intel_port_is_tc(struct drm_i915_private *dev_priv, enum port port);
> >> >> +bool intel_phy_is_tc(struct drm_i915_private *dev_priv, enum phy phy);
> >> >>  enum tc_port intel_port_to_tc(struct drm_i915_private *dev_priv,
> >> >>  			      enum port port);
> >> >>  int intel_get_pipe_from_crtc_id_ioctl(struct drm_device *dev, void *data,
> >> >> --
> >> >> 2.17.2
> >> >
> >> >--
> >> >Ville Syrjälä
> >> >Intel
> >
> >-- 
> >Ville Syrjälä
> >Intel

Lucas De Marchi July 8, 2019, 2:02 p.m. UTC | #5

On Fri, Jul 05, 2019 at 01:33:10PM +0300, Ville Syrjälä wrote:
>On Thu, Jul 04, 2019 at 08:55:51AM -0700, Lucas De Marchi wrote:
>> On Thu, Jul 04, 2019 at 06:09:04PM +0300, Ville Syrjälä wrote:
>> >On Thu, Jul 04, 2019 at 07:54:26AM -0700, Lucas De Marchi wrote:
>> >> 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 <[e-mail]>
>> >> >> Cc: José Roberto de Souza <[e-mail]>
>> >> >> Cc: Lucas De Marchi <[e-mail]>
>> >> >> Cc: Ville Syrjälä <[e-mail]>
>> >> >> Cc: Imre Deak <[e-mail]>
>> >> >> Cc: Jani Nikula <[e-mail]>
>> >> >> Signed-off-by: Matt Roper <[e-mail]>
>> >> >> ---
>> >> >>  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.
>> >>
>> >> that's not true. All TC registers are based off the TC phy number.
>> >
>> >That's just a trivial (x)-TC1, so I stand by what I said.
>>
>> EHL and TGL have 3 combo phys. ICL has 2. So TC1 would have to be a
>> different value for ICL and the others for this to work.
>
>It would just be an arbitrary number (eg. 8).

One that is propagated to every access to those registers, and we need
to keep this enum updated as well the conversion functions because every
platform has a different port->phy mapping. IMO it should be done during
init like in the pseudo code I posted. And in that case TC1 = 0. Always.

Lucas De Marchi

>
>> I think we should treat the index as just a number we use to compute the
>> right base for the registers in that hw IP.
>>
>> >
>> >> Hence all the conversion we do port_to_tc()... I'd like to remove that
>> >> in future and just stuff the phy index in intel_digital_port, as we
>> >> already do for other tc_phy_* fields (we could add a union there so each
>> >> phy adds its own fields).
>> >>
>> >> And I'd rather not do the single phy namespace - it doesn't
>> >> play well with TGL and the combo/tc.
>> >
>> >I think it would work just fine for that. A single namespace would allow
>> >us to remove all the crazy port->PHY type mapping we have going on
>> >currently. Probably the best alternative would be separate namespaces
>> >for each type with a new enum to identify the type.
>>
>> My proposal is in the lines of that alternative approach. Save the phy
>> type and the phy index in intel_digital_port. And allow each phy to store
>> its fields there. Something along:
>>
>> struct intel_digital_port {
>> 	...
>> 	struct {
>> 		enum phy_type type;
>> 		u8 idx;
>> 		union {
>> 			struct {
>> 				struct mutex lock;   /* protects the TypeC port mode */
>> 				intel_wakeref_t lock_wakeref;
>> 				int link_refcount;
>> 				char tc_port_name[8];
>> 				enum tc_port_mode tc_mode;
>> 				bool legacy_port;
>> 				u8 fia;
>> 			} tc;
>> 			struct {
>> 				...
>> 			} combo;
>> 			struct {
>> 				...
>> 			} dpio??;
>> 		};
>> 	} phy;
>> };
>>
>> >From a quick look I don't think the idx is relevant enough to have its
>> own enum, but I wouldn't mind.
>>
>> then no more port->phy everywhere as we have right now and we only
>> assign idx to the right value during init. The TC rework we had recently
>> makes it nice and I'm starting to do it, but I'd rather merge the TGL
>> patches before. For Modular FIA I'm already stashing the fia index
>> there (since I had to redo the support due to the TC rework),
>> but without the additional struct.
>> See https://patchwork.freedesktop.org/series/63175/
>>
>> Lucas De Marchi
>>
>> >
>> >>
>> >> Lucas De Marchi
>> >>
>> >> >
>> >> >Unfortunaltey I don't have a great idea how to do the
>> >> >same for the DDIs since there the number of combo DDIs
>> >> >changes but we still need the PORT_TC1 (assuming we had
>> >> >one) to be DDI_<last combo DDI> + 1. One random silly
>> >> >idea was to decouple the enum port from the register
>> >> >definitions by having just some kind of
>> >> >encoder->port_index for those. But that doesn't feel
>> >> >entirely great either.
>> >> >
>> >> >Anyways, something to think about in the future perhaps.
>> >> >
>> >> >Patch is
>> >> >Reviewed-by: Ville Syrjälä <[e-mail]>
>> >> >
>> >> >> +
>> >> >> +#define phy_name(a) ((a) + 'A')
>> >> >> +
>> >> >>  #define for_each_pipe(__dev_priv, __p) \
>> >> >>  	for ((__p) = 0; (__p) < INTEL_INFO(__dev_priv)->num_pipes; (__p)++)
>> >> >>
>> >> >> @@ -356,5 +371,6 @@ void lpt_disable_clkout_dp(struct drm_i915_private *dev_priv);
>> >> >>  u32 intel_plane_fb_max_stride(struct drm_i915_private *dev_priv,
>> >> >>  			      u32 pixel_format, u64 modifier);
>> >> >>  bool intel_plane_can_remap(const struct intel_plane_state *plane_state);
>> >> >> +enum phy intel_port_to_phy(struct drm_i915_private *i915, enum port port);
>> >> >>
>> >> >>  #endif
>> >> >> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> >> >> index 24c63ed45c6f..815c26c0b98c 100644
>> >> >> --- a/drivers/gpu/drm/i915/intel_drv.h
>> >> >> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> >> >> @@ -1493,7 +1493,9 @@ void intel_encoder_destroy(struct drm_encoder *encoder);
>> >> >>  struct drm_display_mode *
>> >> >>  intel_encoder_current_mode(struct intel_encoder *encoder);
>> >> >>  bool intel_port_is_combophy(struct drm_i915_private *dev_priv, enum port port);
>> >> >> +bool intel_phy_is_combo(struct drm_i915_private *dev_priv, enum phy phy);
>> >> >>  bool intel_port_is_tc(struct drm_i915_private *dev_priv, enum port port);
>> >> >> +bool intel_phy_is_tc(struct drm_i915_private *dev_priv, enum phy phy);
>> >> >>  enum tc_port intel_port_to_tc(struct drm_i915_private *dev_priv,
>> >> >>  			      enum port port);
>> >> >>  int intel_get_pipe_from_crtc_id_ioctl(struct drm_device *dev, void *data,
>> >> >> --
>> >> >> 2.17.2
>> >> >
>> >> >--
>> >> >Ville Syrjälä
>> >> >Intel
>> >
>> >--
>> >Ville Syrjälä
>> >Intel
>
>-- 
>Ville Syrjälä
>Intel

Ville Syrjala July 8, 2019, 2:12 p.m. UTC | #6

On Mon, Jul 08, 2019 at 07:02:49AM -0700, Lucas De Marchi wrote:
> On Fri, Jul 05, 2019 at 01:33:10PM +0300, Ville Syrjälä wrote:
> >On Thu, Jul 04, 2019 at 08:55:51AM -0700, Lucas De Marchi wrote:
> >> On Thu, Jul 04, 2019 at 06:09:04PM +0300, Ville Syrjälä wrote:
> >> >On Thu, Jul 04, 2019 at 07:54:26AM -0700, Lucas De Marchi wrote:
> >> >> 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 <[e-mail]>
> >> >> >> Cc: José Roberto de Souza <[e-mail]>
> >> >> >> Cc: Lucas De Marchi <[e-mail]>
> >> >> >> Cc: Ville Syrjälä <[e-mail]>
> >> >> >> Cc: Imre Deak <[e-mail]>
> >> >> >> Cc: Jani Nikula <[e-mail]>
> >> >> >> Signed-off-by: Matt Roper <[e-mail]>
> >> >> >> ---
> >> >> >>  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.
> >> >>
> >> >> that's not true. All TC registers are based off the TC phy number.
> >> >
> >> >That's just a trivial (x)-TC1, so I stand by what I said.
> >>
> >> EHL and TGL have 3 combo phys. ICL has 2. So TC1 would have to be a
> >> different value for ICL and the others for this to work.
> >
> >It would just be an arbitrary number (eg. 8).
> 
> One that is propagated to every access to those registers, and we need
> to keep this enum updated as well the conversion functions because every
> platform has a different port->phy mapping.

The port->phy mapping doesn't matter. The enum is just about the phy.

And reg macros would be just something like:
#define _TC_PHY_IDX(phy) ((phy)-PHY_TC1)
#define BLAH(phy) _MMIO(..._TC_PHY_IDX(phy)...)

> IMO it should be done during
> init like in the pseudo code I posted. And in that case TC1 = 0. Always.
> 
> Lucas De Marchi
> 
> >
> >> I think we should treat the index as just a number we use to compute the
> >> right base for the registers in that hw IP.
> >>
> >> >
> >> >> Hence all the conversion we do port_to_tc()... I'd like to remove that
> >> >> in future and just stuff the phy index in intel_digital_port, as we
> >> >> already do for other tc_phy_* fields (we could add a union there so each
> >> >> phy adds its own fields).
> >> >>
> >> >> And I'd rather not do the single phy namespace - it doesn't
> >> >> play well with TGL and the combo/tc.
> >> >
> >> >I think it would work just fine for that. A single namespace would allow
> >> >us to remove all the crazy port->PHY type mapping we have going on
> >> >currently. Probably the best alternative would be separate namespaces
> >> >for each type with a new enum to identify the type.
> >>
> >> My proposal is in the lines of that alternative approach. Save the phy
> >> type and the phy index in intel_digital_port. And allow each phy to store
> >> its fields there. Something along:
> >>
> >> struct intel_digital_port {
> >> 	...
> >> 	struct {
> >> 		enum phy_type type;
> >> 		u8 idx;
> >> 		union {
> >> 			struct {
> >> 				struct mutex lock;   /* protects the TypeC port mode */
> >> 				intel_wakeref_t lock_wakeref;
> >> 				int link_refcount;
> >> 				char tc_port_name[8];
> >> 				enum tc_port_mode tc_mode;
> >> 				bool legacy_port;
> >> 				u8 fia;
> >> 			} tc;
> >> 			struct {
> >> 				...
> >> 			} combo;
> >> 			struct {
> >> 				...
> >> 			} dpio??;
> >> 		};
> >> 	} phy;
> >> };
> >>
> >> >From a quick look I don't think the idx is relevant enough to have its
> >> own enum, but I wouldn't mind.
> >>
> >> then no more port->phy everywhere as we have right now and we only
> >> assign idx to the right value during init. The TC rework we had recently
> >> makes it nice and I'm starting to do it, but I'd rather merge the TGL
> >> patches before. For Modular FIA I'm already stashing the fia index
> >> there (since I had to redo the support due to the TC rework),
> >> but without the additional struct.
> >> See https://patchwork.freedesktop.org/series/63175/
> >>
> >> Lucas De Marchi
> >>
> >> >
> >> >>
> >> >> Lucas De Marchi
> >> >>
> >> >> >
> >> >> >Unfortunaltey I don't have a great idea how to do the
> >> >> >same for the DDIs since there the number of combo DDIs
> >> >> >changes but we still need the PORT_TC1 (assuming we had
> >> >> >one) to be DDI_<last combo DDI> + 1. One random silly
> >> >> >idea was to decouple the enum port from the register
> >> >> >definitions by having just some kind of
> >> >> >encoder->port_index for those. But that doesn't feel
> >> >> >entirely great either.
> >> >> >
> >> >> >Anyways, something to think about in the future perhaps.
> >> >> >
> >> >> >Patch is
> >> >> >Reviewed-by: Ville Syrjälä <[e-mail]>
> >> >> >
> >> >> >> +
> >> >> >> +#define phy_name(a) ((a) + 'A')
> >> >> >> +
> >> >> >>  #define for_each_pipe(__dev_priv, __p) \
> >> >> >>  	for ((__p) = 0; (__p) < INTEL_INFO(__dev_priv)->num_pipes; (__p)++)
> >> >> >>
> >> >> >> @@ -356,5 +371,6 @@ void lpt_disable_clkout_dp(struct drm_i915_private *dev_priv);
> >> >> >>  u32 intel_plane_fb_max_stride(struct drm_i915_private *dev_priv,
> >> >> >>  			      u32 pixel_format, u64 modifier);
> >> >> >>  bool intel_plane_can_remap(const struct intel_plane_state *plane_state);
> >> >> >> +enum phy intel_port_to_phy(struct drm_i915_private *i915, enum port port);
> >> >> >>
> >> >> >>  #endif
> >> >> >> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> >> >> >> index 24c63ed45c6f..815c26c0b98c 100644
> >> >> >> --- a/drivers/gpu/drm/i915/intel_drv.h
> >> >> >> +++ b/drivers/gpu/drm/i915/intel_drv.h
> >> >> >> @@ -1493,7 +1493,9 @@ void intel_encoder_destroy(struct drm_encoder *encoder);
> >> >> >>  struct drm_display_mode *
> >> >> >>  intel_encoder_current_mode(struct intel_encoder *encoder);
> >> >> >>  bool intel_port_is_combophy(struct drm_i915_private *dev_priv, enum port port);
> >> >> >> +bool intel_phy_is_combo(struct drm_i915_private *dev_priv, enum phy phy);
> >> >> >>  bool intel_port_is_tc(struct drm_i915_private *dev_priv, enum port port);
> >> >> >> +bool intel_phy_is_tc(struct drm_i915_private *dev_priv, enum phy phy);
> >> >> >>  enum tc_port intel_port_to_tc(struct drm_i915_private *dev_priv,
> >> >> >>  			      enum port port);
> >> >> >>  int intel_get_pipe_from_crtc_id_ioctl(struct drm_device *dev, void *data,
> >> >> >> --
> >> >> >> 2.17.2
> >> >> >
> >> >> >--
> >> >> >Ville Syrjälä
> >> >> >Intel
> >> >
> >> >--
> >> >Ville Syrjälä
> >> >Intel
> >
> >-- 
> >Ville Syrjälä
> >Intel

Souza, Jose July 8, 2019, 11:59 p.m. UTC | #7

On Wed, 2019-07-03 at 16:37 -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 <[e-mail]>
> Cc: José Roberto de Souza <[e-mail]>
> Cc: Lucas De Marchi <[e-mail]>
> Cc: Ville Syrjälä <[e-mail]>
> Cc: Imre Deak <[e-mail]>
> Cc: Jani Nikula <[e-mail]>
> Signed-off-by: Matt Roper <[e-mail]>
> ---
>  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;
>  }

A call to intel_port_is_combophy(PORT_D) would return false on EHL, it
and intel_port_is_tc() should use intel_phy functions, like:

bool intel_port_is_combophy(struct drm_i915_private *dev_priv, enum
port port)
{
	return intel_phy_is_combo(dev_priv, intel_port_to_phy(dev_priv,
port));
}

Even better would be check if we can replace those with intel_phy
counterparts.

>  
> +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
> +};
> +
> +#define phy_name(a) ((a) + 'A')
> +
>  #define for_each_pipe(__dev_priv, __p) \
>  	for ((__p) = 0; (__p) < INTEL_INFO(__dev_priv)->num_pipes;
> (__p)++)
>  
> @@ -356,5 +371,6 @@ void lpt_disable_clkout_dp(struct
> drm_i915_private *dev_priv);
>  u32 intel_plane_fb_max_stride(struct drm_i915_private *dev_priv,
>  			      u32 pixel_format, u64 modifier);
>  bool intel_plane_can_remap(const struct intel_plane_state
> *plane_state);
> +enum phy intel_port_to_phy(struct drm_i915_private *i915, enum port
> port);
>  
>  #endif
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index 24c63ed45c6f..815c26c0b98c 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1493,7 +1493,9 @@ void intel_encoder_destroy(struct drm_encoder
> *encoder);
>  struct drm_display_mode *
>  intel_encoder_current_mode(struct intel_encoder *encoder);
>  bool intel_port_is_combophy(struct drm_i915_private *dev_priv, enum
> port port);
> +bool intel_phy_is_combo(struct drm_i915_private *dev_priv, enum phy
> phy);
>  bool intel_port_is_tc(struct drm_i915_private *dev_priv, enum port
> port);
> +bool intel_phy_is_tc(struct drm_i915_private *dev_priv, enum phy
> phy);
>  enum tc_port intel_port_to_tc(struct drm_i915_private *dev_priv,
>  			      enum port port);
>  int intel_get_pipe_from_crtc_id_ioctl(struct drm_device *dev, void
> *data,

Souza, Jose July 9, 2019, 12:45 a.m. UTC | #8

On Mon, 2019-07-08 at 23:59 +0000, Souza, Jose wrote:
> On Wed, 2019-07-03 at 16:37 -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 <[e-mail]>
> > Cc: José Roberto de Souza <[e-mail]>
> > Cc: Lucas De Marchi <[e-mail]>
> > Cc: Ville Syrjälä <[e-mail]>
> > Cc: Imre Deak <[e-mail]>
> > Cc: Jani Nikula <[e-mail]>
> > Signed-off-by: Matt Roper <[e-mail]>
> > ---
> >  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;
> >  }
> 
> A call to intel_port_is_combophy(PORT_D) would return false on EHL,
> it
> and intel_port_is_tc() should use intel_phy functions, like:
> 
> bool intel_port_is_combophy(struct drm_i915_private *dev_priv, enum
> port port)
> {
> 	return intel_phy_is_combo(dev_priv, intel_port_to_phy(dev_priv,
> port));
> }
> 
> Even better would be check if we can replace those with intel_phy
> counterparts.


You did that on patch 4, so I guess you can disconsider this comments.

> 
> >  
> > +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
> > +};
> > +
> > +#define phy_name(a) ((a) + 'A')
> > +
> >  #define for_each_pipe(__dev_priv, __p) \
> >  	for ((__p) = 0; (__p) < INTEL_INFO(__dev_priv)->num_pipes;
> > (__p)++)
> >  
> > @@ -356,5 +371,6 @@ void lpt_disable_clkout_dp(struct
> > drm_i915_private *dev_priv);
> >  u32 intel_plane_fb_max_stride(struct drm_i915_private *dev_priv,
> >  			      u32 pixel_format, u64 modifier);
> >  bool intel_plane_can_remap(const struct intel_plane_state
> > *plane_state);
> > +enum phy intel_port_to_phy(struct drm_i915_private *i915, enum
> > port
> > port);
> >  
> >  #endif
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > b/drivers/gpu/drm/i915/intel_drv.h
> > index 24c63ed45c6f..815c26c0b98c 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -1493,7 +1493,9 @@ void intel_encoder_destroy(struct drm_encoder
> > *encoder);
> >  struct drm_display_mode *
> >  intel_encoder_current_mode(struct intel_encoder *encoder);
> >  bool intel_port_is_combophy(struct drm_i915_private *dev_priv,
> > enum
> > port port);
> > +bool intel_phy_is_combo(struct drm_i915_private *dev_priv, enum
> > phy
> > phy);
> >  bool intel_port_is_tc(struct drm_i915_private *dev_priv, enum port
> > port);
> > +bool intel_phy_is_tc(struct drm_i915_private *dev_priv, enum phy
> > phy);
> >  enum tc_port intel_port_to_tc(struct drm_i915_private *dev_priv,
> >  			      enum port port);
> >  int intel_get_pipe_from_crtc_id_ioctl(struct drm_device *dev, void
> > *data,
> _______________________________________________
> Intel-gfx mailing list
> [e-mail]
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Patch

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
+};
+
+#define phy_name(a) ((a) + 'A')
+
 #define for_each_pipe(__dev_priv, __p) \
 	for ((__p) = 0; (__p) < INTEL_INFO(__dev_priv)->num_pipes; (__p)++)
 
@@ -356,5 +371,6 @@  void lpt_disable_clkout_dp(struct drm_i915_private *dev_priv);
 u32 intel_plane_fb_max_stride(struct drm_i915_private *dev_priv,
 			      u32 pixel_format, u64 modifier);
 bool intel_plane_can_remap(const struct intel_plane_state *plane_state);
+enum phy intel_port_to_phy(struct drm_i915_private *i915, enum port port);
 
 #endif
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 24c63ed45c6f..815c26c0b98c 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1493,7 +1493,9 @@  void intel_encoder_destroy(struct drm_encoder *encoder);
 struct drm_display_mode *
 intel_encoder_current_mode(struct intel_encoder *encoder);
 bool intel_port_is_combophy(struct drm_i915_private *dev_priv, enum port port);
+bool intel_phy_is_combo(struct drm_i915_private *dev_priv, enum phy phy);
 bool intel_port_is_tc(struct drm_i915_private *dev_priv, enum port port);
+bool intel_phy_is_tc(struct drm_i915_private *dev_priv, enum phy phy);
 enum tc_port intel_port_to_tc(struct drm_i915_private *dev_priv,
 			      enum port port);
 int intel_get_pipe_from_crtc_id_ioctl(struct drm_device *dev, void *data,