Quoting Lucas De Marchi (2023-10-20 13:04:48-03:00) >On Thu, Oct 19, 2023 at 01:04:40PM -0300, Gustavo Sousa wrote: >>Quoting Lucas De Marchi (2023-10-18 19:24:41-03:00) >>>With MTL adding PICA between the port and the real phy, the path >>>add for DG2 stopped being followed and newer platforms are simply using >>>the older path for TC phys. LNL is no different than MTL in this aspect, >>>so just add it to the mess. In future the phy and port designation and >>>deciding if it's TC should better be cleaned up. >>> >>>To make it just a bit better, also change intel_phy_is_snps() to show >>>this is DG2-only. >>> >>>Signed-off-by: Lucas De Marchi <lucas.demarchi@xxxxxxxxx> >>>--- >>> drivers/gpu/drm/i915/display/intel_display.c | 29 ++++++++++---------- >>> 1 file changed, 15 insertions(+), 14 deletions(-) >>> >>>diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c >>>index 28d85e1e858e..0797ace31417 100644 >>>--- a/drivers/gpu/drm/i915/display/intel_display.c >>>+++ b/drivers/gpu/drm/i915/display/intel_display.c >>>@@ -1784,31 +1784,32 @@ bool intel_phy_is_combo(struct drm_i915_private *dev_priv, enum phy phy) >>> >>> bool intel_phy_is_tc(struct drm_i915_private *dev_priv, enum phy phy) >>> { >>>+ /* DG2's "TC1" output uses a SNPS PHY and is handled separately */ >>> if (IS_DG2(dev_priv)) >>>- /* DG2's "TC1" output uses a SNPS PHY */ >>> return false; >>>- else if (IS_ALDERLAKE_P(dev_priv) || DISPLAY_VER_FULL(dev_priv) == IP_VER(14, 0)) >>>+ >>>+ /* >>>+ * TODO: This should mostly match intel_port_to_phy(), considering the >>>+ * ports already encode if they are connected to a TC phy in their name. >>>+ */ >>>+ if (IS_LUNARLAKE(dev_priv) || IS_METEORLAKE(dev_priv) || >>>+ IS_ALDERLAKE_P(dev_priv)) >> >>Just like already done with the previous patch, I think we should have a >>paragraph in the commit message justifying s/DISPLAY_VER_FULL(dev_priv) == >>IP_VER(14, 0)/IS_METEORLAKE(dev_priv)/. > >humn... after giving this a second thought, I will take this back. >intel_phy_is_tc() is different than the check in the first patch and >it's actually something dependent on display engine. Here the check is >about is this a DDIA/DDIB or a TC1-TC4? This will change how some >registers in the display engine are programmed: Hm, yeah. I overlooked that... But we are looking into the PHY regardless. Is the mapping "phy number -> port type" really associated to the display engine rather than to the SoC? -- Gustavo Sousa > > $ git grep intel_phy_is_tc -- drivers/gpu/drm/i915/display/intel_ddi.c > drivers/gpu/drm/i915/display/intel_ddi.c: if (intel_phy_is_tc(dev_priv, phy)) > drivers/gpu/drm/i915/display/intel_ddi.c: if (IS_ALDERLAKE_P(i915) && intel_phy_is_tc(i915, phy)) { > drivers/gpu/drm/i915/display/intel_ddi.c: intel_phy_is_tc(i915, phy))) > drivers/gpu/drm/i915/display/intel_ddi.c: if (!intel_phy_is_tc(dev_priv, phy) || > drivers/gpu/drm/i915/display/intel_ddi.c: bool is_tc_port = intel_phy_is_tc(i915, phy); > drivers/gpu/drm/i915/display/intel_ddi.c: } else if (IS_ALDERLAKE_P(dev_priv) && intel_phy_is_tc(dev_priv, phy)) { > drivers/gpu/drm/i915/display/intel_ddi.c: if (DISPLAY_VER(i915) >= 14 || !intel_phy_is_tc(i915, phy)) > drivers/gpu/drm/i915/display/intel_ddi.c: bool is_tc_port = intel_phy_is_tc(dev_priv, phy); > drivers/gpu/drm/i915/display/intel_ddi.c: if (intel_phy_is_tc(i915, phy)) > drivers/gpu/drm/i915/display/intel_ddi.c: if (intel_phy_is_tc(i915, phy)) { > drivers/gpu/drm/i915/display/intel_ddi.c: if (intel_phy_is_tc(i915, phy)) > drivers/gpu/drm/i915/display/intel_ddi.c: if (intel_phy_is_tc(i915, phy)) > drivers/gpu/drm/i915/display/intel_ddi.c: bool is_tc = intel_phy_is_tc(i915, phy); > drivers/gpu/drm/i915/display/intel_ddi.c: return init_dp || intel_phy_is_tc(i915, phy); > drivers/gpu/drm/i915/display/intel_ddi.c: if (intel_phy_is_tc(dev_priv, phy)) { > drivers/gpu/drm/i915/display/intel_ddi.c: if (intel_phy_is_tc(dev_priv, phy)) > >and particularly the creation of intel_tc, which we do want to happen. > >I think we will really need to rollback the port -> phy conversions all >around the code and simplify it. While we don't do that, my proposal >here is to turn this commit into: > >-----------------8<-------------------- >Subject: [PATCH] drm/i915/lnl: Fix check for TC phy > >With MTL adding PICA between the port and the real phy, the path >add for DG2 stopped being followed and newer platforms are simply using >the older path for TC phys. LNL is no different than MTL in this aspect, >so just add it to the mess. In future the phy and port designation and >deciding if it's TC should better be cleaned up. > >To make it just a bit better, also change intel_phy_is_snps() to show >this is DG2-only. > >Signed-off-by: Lucas De Marchi <lucas.demarchi@xxxxxxxxx> >Reviewed-by: Gustavo Sousa <gustavo.sousa@xxxxxxxxx> >--- > drivers/gpu/drm/i915/display/intel_display.c | 28 ++++++++++---------- > 1 file changed, 14 insertions(+), 14 deletions(-) > >diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c >index 28d85e1e858e..1caf46e3e569 100644 >--- a/drivers/gpu/drm/i915/display/intel_display.c >+++ b/drivers/gpu/drm/i915/display/intel_display.c >@@ -1784,31 +1784,31 @@ bool intel_phy_is_combo(struct drm_i915_private *dev_priv, enum phy phy) > > bool intel_phy_is_tc(struct drm_i915_private *dev_priv, enum phy phy) > { >+ /* >+ * DG2's "TC1", although TC-capable output, doesn't share the same flow >+ * as other platforms on the display engine side and rather rely on the >+ * SNPS PHY, that is programmed separately >+ */ > if (IS_DG2(dev_priv)) >- /* DG2's "TC1" output uses a SNPS PHY */ > return false; >- else if (IS_ALDERLAKE_P(dev_priv) || DISPLAY_VER_FULL(dev_priv) == IP_VER(14, 0)) >+ >+ if (DISPLAY_VER(dev_priv) >= 13) > return phy >= PHY_F && phy <= PHY_I; > else if (IS_TIGERLAKE(dev_priv)) > return phy >= PHY_D && phy <= PHY_I; > else if (IS_ICELAKE(dev_priv)) > return phy >= PHY_C && phy <= PHY_F; >- else >- return false; >+ >+ return false; > } > > bool intel_phy_is_snps(struct drm_i915_private *dev_priv, enum phy phy) > { >- if (phy == PHY_NONE) >- return false; >- else if (IS_DG2(dev_priv)) >- /* >- * All four "combo" ports and the TC1 port (PHY E) use >- * Synopsis PHYs. >- */ >- return phy <= PHY_E; >- >- return false; >+ /* >+ * For DG2, and for DG2 only, all four "combo" ports and the TC1 port >+ * (PHY E) use Synopsis PHYs. See intel_phy_is_tc(). >+ */ >+ return IS_DG2(dev_priv) && phy > PHY_NONE && phy <= PHY_E; > } > > enum phy intel_port_to_phy(struct drm_i915_private *i915, enum port port) >-- >2.40.1 >-----------------8<-------------------- > >This would at make intel_phy_is_tc() match intel_port_to_phy(), at least >for display version >= 13. > >Lucas De Marchi