Re: [Intel-gfx] [PATCH v4 1/5] drm/i915/gen11: Start distinguishing 'phy' from 'port'
On Mon, 2019-07-08 at 23:59 +, 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
> > Cc: José Roberto de Souza
> > Cc: Lucas De Marchi
> > Cc: Ville Syrjälä
> > Cc: Imre Deak
> > Cc: Jani Nikula
> > Signed-off-by: Matt Roper
> > ---
> > 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_dis
Re: [Intel-gfx] [PATCH v4 1/5] drm/i915/gen11: Start distinguishing 'phy' from 'port'
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
> Cc: José Roberto de Souza
> Cc: Lucas De Marchi
> Cc: Ville Syrjälä
> Cc: Imre Deak
> Cc: Jani Nikula
> Signed-off-by: Matt Roper
> ---
> 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,
>
Re: [Intel-gfx] [PATCH v4 1/5] drm/i915/gen11: Start distinguishing 'phy' from 'port'
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
> >> >> >> Cc: José Roberto de Souza
> >> >> >> Cc: Lucas De Marchi
> >> >> >> Cc: Ville Syrjälä
> >> >> >> Cc: Imre Deak
> >> >> >> Cc: Jani Nikula
> >> >> >> Signed-off-by: Matt Roper
> >> >> >> ---
> >> >> >> 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;
>
Re: [Intel-gfx] [PATCH v4 1/5] drm/i915/gen11: Start distinguishing 'phy' from 'port'
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
>> >> Cc: José Roberto de Souza
>> >> Cc: Lucas De Marchi
>> >> Cc: Ville Syrjälä
>> >> Cc: Imre Deak
>> >> Cc: Jani Nikula
>> >> Signed-off-by: Matt Roper
>> >> ---
>> >> 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 d296556e
Re: [Intel-gfx] [PATCH v4 1/5] drm/i915/gen11: Start distinguishing 'phy' from 'port'
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
> >> >> Cc: José Roberto de Souza
> >> >> Cc: Lucas De Marchi
> >> >> Cc: Ville Syrjälä
> >> >> Cc: Imre Deak
> >> >> Cc: Jani Nikula
> >> >> Signed-off-by: Matt Roper
> >> >> ---
> >> >> 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_po
Re: [Intel-gfx] [PATCH v4 1/5] drm/i915/gen11: Start distinguishing 'phy' from 'port'
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
>> Cc: José Roberto de Souza
>> Cc: Lucas De Marchi
>> Cc: Ville Syrjälä
>> Cc: Imre Deak
>> Cc: Jani Nikula
>> Signed-off-by: Matt Roper
>> ---
>> 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 even
Re: [Intel-gfx] [PATCH v4 1/5] drm/i915/gen11: Start distinguishing 'phy' from 'port'
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
> >> Cc: José Roberto de Souza
> >> Cc: Lucas De Marchi
> >> Cc: Ville Syrjälä
> >> Cc: Imre Deak
> >> Cc: Jani Nikula
> >> Signed-off-by: Matt Roper
> >> ---
> >> 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,
> >> +
Re: [Intel-gfx] [PATCH v4 1/5] drm/i915/gen11: Start distinguishing 'phy' from 'port'
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
Cc: José Roberto de Souza
Cc: Lucas De Marchi
Cc: Ville Syrjälä
Cc: Imre Deak
Cc: Jani Nikula
Signed-off-by: Matt Roper
---
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.
Hence all the conversion we do port_
Re: [Intel-gfx] [PATCH v4 1/5] drm/i915/gen11: Start distinguishing 'phy' from 'port'
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
> > Cc: José Roberto de Souza
> > Cc: Lucas De Marchi
> > Cc: Ville Syrjälä
> > Cc: Imre Deak
> > Cc: Jani Nikula
> > Signed-off-by: Matt Roper
> > ---
> > 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_
Re: [Intel-gfx] [PATCH v4 1/5] drm/i915/gen11: Start distinguishing 'phy' from 'port'
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
> Cc: José Roberto de Souza
> Cc: Lucas De Marchi
> Cc: Ville Syrjälä
> Cc: Imre Deak
> Cc: Jani Nikula
> Signed-off-by: Matt Roper
> ---
> 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
[Intel-gfx] [PATCH v4 1/5] drm/i915/gen11: Start distinguishing 'phy' from 'port'
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
Cc: José Roberto de Souza
Cc: Lucas De Marchi
Cc: Ville Syrjälä
Cc: Imre Deak
Cc: Jani Nikula
Signed-off-by: Matt Roper
---
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
+};
+
+#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 24c
