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 <ville.syrjala@xxxxxxxxxxxxxxx>
>> >> Cc: José Roberto de Souza <jose.souza@xxxxxxxxx>
>> >> Cc: Lucas De Marchi <lucas.demarchi@xxxxxxxxx>
>> >> Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
>> >> Cc: Imre Deak <imre.deak@xxxxxxxxx>
>> >> Cc: Jani Nikula <jani.nikula@xxxxxxxxx>
>> >> Signed-off-by: Matt Roper <matthew.d.roper@xxxxxxxxx>
>> >> ---
>> >> 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.
