[Intel-gfx] [PATCH 2/2] drm/i915/lnl: Fix check for TC phy

[Lucas De Marchi]

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 at intel.com>
>>---
>> 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:

	$ 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 at intel.com>
Reviewed-by: Gustavo Sousa <gustavo.sousa at intel.com>
---
  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