[Intel-gfx] [PATCH 0/2] drm/i915/lnl: Assign correct phys

[Intel-gfx] [PATCH 0/2] drm/i915/lnl: Assign correct phys

[Lucas De Marchi]

For this series to work, we still need a separate patch on the xe side
so it defines the LNL platform macro to be used by display.

One thing missing for LNL during the previous patches was the
port <-> phy assignment. With the bspec now clarified, this is the
minimum changes needed for LNL.  As the commit messages say and after
looking at the history of the code, it seems we were thinking to go one
direction abstraction-wise with DG2, but reverted course with MTL. The
end result right now is a very confusing mix of port/phy/tc_port.
I was hoping to do a cleanup now, but we probably need some consensus on
the approach as it'd be an intrusive change.

Here are some thoughts after looking again at the current state of the
code:

1) What is the port -> phy conversion for? AFAIR this was because from
the display engine side we want, some registers have bit offsets based
on the port and others are based on the PHY. I think now we can

	a) Remove enum tc_port and have only `enum port` and `enum phy`.
	   Those should be sufficient for all platform needs afaics

	b) Add phy to intel_encoder (or intel_digital_port). It's
	   appalling number of places we convert from port to phy. That
	   would just be initialized during init.

2) It looks we need to better abstract the phy handling. Right now it's
very confusing with dkl, c10/c20 (that leak the abstraction from
intel_cx0_phy.c to everywhere in the driver), snps and the older
combo/tc being a superset of them.  I'm still not sure what to do here.
One thing that we can probably do is to remove the dg2-special case and
let the "is tc" be about the **port being connected to a TC-capable phy**.
Bspec always refer to those as TC<N> / USBC<N>. Then dkl, c10, c20, snps
would all be in the same abstraction layer. Any thoughts?

Lucas De Marchi (2):
  drm/i915/lnl: Extend C10/C20 phy
  drm/i915/lnl: Fix check for TC phy

 drivers/gpu/drm/i915/display/intel_cx0_phy.c |  2 +-
 drivers/gpu/drm/i915/display/intel_display.c | 29 ++++++++++----------
 drivers/gpu/drm/i915/i915_drv.h              |  1 +
 3 files changed, 17 insertions(+), 15 deletions(-)

-- 
2.40.1

[Intel-gfx] [PATCH 1/2] drm/i915/lnl: Extend C10/C20 phy

[Lucas De Marchi]

For Lunar Lake, DDI-A is connected to C10 PHY, while TC1-TC3 are connected
to C20 phy, like in Meteor Lake. Update the check in intel_is_c10phy()
accordingly.

This reverts the change in commit e388ae97e225 ("drm/i915/display:
Eliminate IS_METEORLAKE checks") that turned that into a display engine
version check. The phy <-> port connection is very SoC-specific and not
related to that version.

IS_LUNARLAKE() is defined to 0 in i915 as it's expected that the
(upcoming) xe driver is the one defining the platform, with i915 only
driving the display side.

Bspec: 70818
Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
---
 drivers/gpu/drm/i915/display/intel_cx0_phy.c | 2 +-
 drivers/gpu/drm/i915/i915_drv.h              | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy.c b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
index d414f6b7f993..e775f4721158 100644
--- a/drivers/gpu/drm/i915/display/intel_cx0_phy.c
+++ b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
@@ -31,7 +31,7 @@
 
 bool intel_is_c10phy(struct drm_i915_private *i915, enum phy phy)
 {
-	if (DISPLAY_VER_FULL(i915) == IP_VER(14, 0) && phy < PHY_C)
+	if ((IS_LUNARLAKE(i915) || IS_METEORLAKE(i915)) && phy < PHY_C)
 		return true;
 
 	return false;
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 6a2a78c61f21..259884b10d9a 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -575,6 +575,7 @@ IS_SUBPLATFORM(const struct drm_i915_private *i915,
 #define IS_DG2(i915)	IS_PLATFORM(i915, INTEL_DG2)
 #define IS_PONTEVECCHIO(i915) IS_PLATFORM(i915, INTEL_PONTEVECCHIO)
 #define IS_METEORLAKE(i915) IS_PLATFORM(i915, INTEL_METEORLAKE)
+#define IS_LUNARLAKE(i915) 0
 
 #define IS_DG2_G10(i915) \
 	IS_SUBPLATFORM(i915, INTEL_DG2, INTEL_SUBPLATFORM_G10)
-- 
2.40.1

Re: [Intel-gfx] [PATCH 1/2] drm/i915/lnl: Extend C10/C20 phy

[Gustavo Sousa]

Quoting Lucas De Marchi (2023-10-18 19:24:40-03:00)
>For Lunar Lake, DDI-A is connected to C10 PHY, while TC1-TC3 are connected
>to C20 phy, like in Meteor Lake. Update the check in intel_is_c10phy()
>accordingly.
>
>This reverts the change in commit e388ae97e225 ("drm/i915/display:
>Eliminate IS_METEORLAKE checks") that turned that into a display engine
>version check. The phy <-> port connection is very SoC-specific and not
>related to that version.
>
>IS_LUNARLAKE() is defined to 0 in i915 as it's expected that the
>(upcoming) xe driver is the one defining the platform, with i915 only
>driving the display side.
>
>Bspec: 70818
>Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>

Reviewed-by: Gustavo Sousa <gustavo.sousa@intel.com>

>---
> drivers/gpu/drm/i915/display/intel_cx0_phy.c | 2 +-
> drivers/gpu/drm/i915/i915_drv.h              | 1 +
> 2 files changed, 2 insertions(+), 1 deletion(-)
>
>diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy.c b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
>index d414f6b7f993..e775f4721158 100644
>--- a/drivers/gpu/drm/i915/display/intel_cx0_phy.c
>+++ b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
>@@ -31,7 +31,7 @@
> 
> bool intel_is_c10phy(struct drm_i915_private *i915, enum phy phy)
> {
>-        if (DISPLAY_VER_FULL(i915) == IP_VER(14, 0) && phy < PHY_C)
>+        if ((IS_LUNARLAKE(i915) || IS_METEORLAKE(i915)) && phy < PHY_C)
>                 return true;
> 
>         return false;
>diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>index 6a2a78c61f21..259884b10d9a 100644
>--- a/drivers/gpu/drm/i915/i915_drv.h
>+++ b/drivers/gpu/drm/i915/i915_drv.h
>@@ -575,6 +575,7 @@ IS_SUBPLATFORM(const struct drm_i915_private *i915,
> #define IS_DG2(i915)        IS_PLATFORM(i915, INTEL_DG2)
> #define IS_PONTEVECCHIO(i915) IS_PLATFORM(i915, INTEL_PONTEVECCHIO)
> #define IS_METEORLAKE(i915) IS_PLATFORM(i915, INTEL_METEORLAKE)
>+#define IS_LUNARLAKE(i915) 0
> 
> #define IS_DG2_G10(i915) \
>         IS_SUBPLATFORM(i915, INTEL_DG2, INTEL_SUBPLATFORM_G10)
>-- 
>2.40.1
>

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

[Lucas De Marchi]

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@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))
 		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.
+	 */
+	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

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

[Gustavo Sousa]

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@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)/.

With that in place,

Reviewed-by: Gustavo Sousa <gustavo.sousa@intel.com>

>                 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.
>+         */
>+        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
>

Re: [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@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@intel.com>
Reviewed-by: Gustavo Sousa <gustavo.sousa@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

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

[Gustavo Sousa]

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@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:

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

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

[Lucas De Marchi]

On Mon, Oct 23, 2023 at 12:28:46PM -0300, Gustavo Sousa wrote:
>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@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:
>
>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?

we are converting back and forth. The phy number always come from the
port by using intel_port_to_phy(). See intel_ddi_init() for example:

	intel_ddi_init()
	{
		port = intel_bios_encoder_port(devdata);
		...
		phy = intel_port_to_phy(dev_priv, port);
	}

intel_port_to_phy() does use the display engine version and a
platform-based check in a few cases. Looking at the history, this was
added for EHL, where the ports DDI-A and DDI-D are muxed to one PHY,
called PHY-A.  Then some registers need to use that number to configure
the registers.

4+ years later I don't see the bspec doing any better job on the
registers that are using the phy vs port and this is derived mostly on a
case by case basis :(

Looking at intel_port_to_phy() and ignoring EHL/JSL as outlier, all the
others are basically answering the question "from the display pov, where
does the native/combo port end and we start the ports connected to "TC
ports". From those, then DG2 starts to be the outlier as it identifies
itself as neither combo nor tc, but rather snps. XeLPD is very
"creative" as we assigned a PORT_D_XELPD = PORT_TC5 to make it work
with the register offsets from the display engine pov they replaced
TC5/TC6. Then the phy_is_tc() also has to workaround that, as those are
not TC phys :-/

I think a better abstraction looking back would be to nuke this
intel_port_to_* / intel_phy_to_* / intel_phy_is_tc. Then we only set
that during ddi init.

Note that this is all different than the is this a C10 or C20 phy
question.  The display engine has no idea about that and doesn't care.
Until a few days ago it was not even documented in bspec as this is a
SoC characteristics.

To summarize: I think here we should keep the display engine version
check, resorting to platform checks for the exceptions to match what
intel_port_to_phy() does.  Long term we need to better abstract/document
that, but that is for another day.

Lucas De Marchi

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

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

[Gustavo Sousa]

Quoting Lucas De Marchi (2023-10-25 12:44:09-03:00)
>On Mon, Oct 23, 2023 at 12:28:46PM -0300, Gustavo Sousa wrote:
>>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@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:
>>
>>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?
>
>we are converting back and forth. The phy number always come from the
>port by using intel_port_to_phy(). See intel_ddi_init() for example:
>
>        intel_ddi_init()
>        {
>                port = intel_bios_encoder_port(devdata);
>                ...
>                phy = intel_port_to_phy(dev_priv, port);
>        }
>
>intel_port_to_phy() does use the display engine version and a
>platform-based check in a few cases. Looking at the history, this was
>added for EHL, where the ports DDI-A and DDI-D are muxed to one PHY,
>called PHY-A.  Then some registers need to use that number to configure
>the registers.
>
>4+ years later I don't see the bspec doing any better job on the
>registers that are using the phy vs port and this is derived mostly on a
>case by case basis :(
>
>Looking at intel_port_to_phy() and ignoring EHL/JSL as outlier, all the
>others are basically answering the question "from the display pov, where
>does the native/combo port end and we start the ports connected to "TC
>ports". From those, then DG2 starts to be the outlier as it identifies
>itself as neither combo nor tc, but rather snps. XeLPD is very
>"creative" as we assigned a PORT_D_XELPD = PORT_TC5 to make it work
>with the register offsets from the display engine pov they replaced
>TC5/TC6. Then the phy_is_tc() also has to workaround that, as those are
>not TC phys :-/

Thanks for the history! :-)

>
>I think a better abstraction looking back would be to nuke this
>intel_port_to_* / intel_phy_to_* / intel_phy_is_tc. Then we only set
>that during ddi init.
>
>Note that this is all different than the is this a C10 or C20 phy
>question.  The display engine has no idea about that and doesn't care.
>Until a few days ago it was not even documented in bspec as this is a
>SoC characteristics.

Got it.

>
>To summarize: I think here we should keep the display engine version
>check, resorting to platform checks for the exceptions to match what
>intel_port_to_phy() does.  Long term we need to better abstract/document
>that, but that is for another day.
>
>Lucas De Marchi
>
>>
>>--
>>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@intel.com>
>>>Reviewed-by: Gustavo Sousa <gustavo.sousa@intel.com>

Given the above explanation, the r-b above stands.

--
Gustavo Sousa

>>>---
>>>  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

[Intel-gfx] ✗ Fi.CI.SPARSE: warning for drm/i915/lnl: Assign correct phys

[Patchwork]

== Series Details ==

Series: drm/i915/lnl: Assign correct phys
URL   : https://patchwork.freedesktop.org/series/125322/
State : warning

== Summary ==

Error: dim sparse failed
Sparse version: v0.6.2
Fast mode used, each commit won't be checked separately.

[Intel-gfx] ✗ Fi.CI.BAT: failure for drm/i915/lnl: Assign correct phys

[Patchwork]

[-- Attachment #1: Type: text/plain, Size: 11860 bytes --]

== Series Details ==

Series: drm/i915/lnl: Assign correct phys
URL   : https://patchwork.freedesktop.org/series/125322/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_13774 -> Patchwork_125322v1
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_125322v1 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_125322v1, please notify your bug team (lgci.bug.filing@intel.com) to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_125322v1/index.html

Participating hosts (31 -> 31)
------------------------------

  Additional (2): bat-dg2-9 fi-cfl-8109u 
  Missing    (2): fi-kbl-x1275 fi-snb-2520m 

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in Patchwork_125322v1:

### IGT changes ###

#### Possible regressions ####

  * igt@i915_module_load@load:
    - fi-cfl-8109u:       NOTRUN -> [INCOMPLETE][1]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_125322v1/fi-cfl-8109u/igt@i915_module_load@load.html

  
Known issues
------------

  Here are the changes found in Patchwork_125322v1 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@debugfs_test@basic-hwmon:
    - bat-jsl-1:          NOTRUN -> [SKIP][2] ([i915#9318])
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_125322v1/bat-jsl-1/igt@debugfs_test@basic-hwmon.html

  * igt@gem_huc_copy@huc-copy:
    - bat-jsl-1:          NOTRUN -> [SKIP][3] ([i915#2190])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_125322v1/bat-jsl-1/igt@gem_huc_copy@huc-copy.html

  * igt@gem_lmem_swapping@verify-random:
    - bat-jsl-1:          NOTRUN -> [SKIP][4] ([i915#4613]) +3 other tests skip
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_125322v1/bat-jsl-1/igt@gem_lmem_swapping@verify-random.html

  * igt@gem_mmap@basic:
    - bat-dg2-9:          NOTRUN -> [SKIP][5] ([i915#4083])
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_125322v1/bat-dg2-9/igt@gem_mmap@basic.html

  * igt@gem_mmap_gtt@basic:
    - bat-dg2-9:          NOTRUN -> [SKIP][6] ([i915#4077]) +2 other tests skip
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_125322v1/bat-dg2-9/igt@gem_mmap_gtt@basic.html

  * igt@gem_render_tiled_blits@basic:
    - bat-dg2-9:          NOTRUN -> [SKIP][7] ([i915#4079]) +1 other test skip
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_125322v1/bat-dg2-9/igt@gem_render_tiled_blits@basic.html

  * igt@i915_pm_rps@basic-api:
    - bat-dg2-9:          NOTRUN -> [SKIP][8] ([i915#6621])
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_125322v1/bat-dg2-9/igt@i915_pm_rps@basic-api.html

  * igt@kms_addfb_basic@addfb25-y-tiled-small-legacy:
    - bat-dg2-9:          NOTRUN -> [SKIP][9] ([i915#5190])
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_125322v1/bat-dg2-9/igt@kms_addfb_basic@addfb25-y-tiled-small-legacy.html

  * igt@kms_addfb_basic@basic-y-tiled-legacy:
    - bat-dg2-9:          NOTRUN -> [SKIP][10] ([i915#4215] / [i915#5190])
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_125322v1/bat-dg2-9/igt@kms_addfb_basic@basic-y-tiled-legacy.html

  * igt@kms_addfb_basic@framebuffer-vs-set-tiling:
    - bat-dg2-9:          NOTRUN -> [SKIP][11] ([i915#4212]) +6 other tests skip
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_125322v1/bat-dg2-9/igt@kms_addfb_basic@framebuffer-vs-set-tiling.html

  * igt@kms_addfb_basic@tile-pitch-mismatch:
    - bat-dg2-9:          NOTRUN -> [SKIP][12] ([i915#4212] / [i915#5608])
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_125322v1/bat-dg2-9/igt@kms_addfb_basic@tile-pitch-mismatch.html

  * igt@kms_cursor_legacy@basic-busy-flip-before-cursor-legacy:
    - bat-dg2-9:          NOTRUN -> [SKIP][13] ([i915#4103] / [i915#4213] / [i915#5608]) +1 other test skip
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_125322v1/bat-dg2-9/igt@kms_cursor_legacy@basic-busy-flip-before-cursor-legacy.html
    - bat-jsl-1:          NOTRUN -> [SKIP][14] ([i915#4103]) +1 other test skip
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_125322v1/bat-jsl-1/igt@kms_cursor_legacy@basic-busy-flip-before-cursor-legacy.html

  * igt@kms_dsc@dsc-basic:
    - bat-jsl-1:          NOTRUN -> [SKIP][15] ([i915#3555]) +1 other test skip
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_125322v1/bat-jsl-1/igt@kms_dsc@dsc-basic.html

  * igt@kms_force_connector_basic@force-load-detect:
    - bat-dg2-9:          NOTRUN -> [SKIP][16] ([fdo#109285])
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_125322v1/bat-dg2-9/igt@kms_force_connector_basic@force-load-detect.html
    - bat-jsl-1:          NOTRUN -> [SKIP][17] ([fdo#109285])
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_125322v1/bat-jsl-1/igt@kms_force_connector_basic@force-load-detect.html

  * igt@kms_force_connector_basic@prune-stale-modes:
    - bat-dg2-9:          NOTRUN -> [SKIP][18] ([i915#5274])
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_125322v1/bat-dg2-9/igt@kms_force_connector_basic@prune-stale-modes.html

  * igt@kms_pipe_crc_basic@nonblocking-crc-frame-sequence:
    - bat-dg2-11:         NOTRUN -> [SKIP][19] ([i915#1845] / [i915#9197]) +2 other tests skip
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_125322v1/bat-dg2-11/igt@kms_pipe_crc_basic@nonblocking-crc-frame-sequence.html

  * igt@kms_pipe_crc_basic@read-crc-frame-sequence@pipe-c-dp-5:
    - bat-adlp-11:        NOTRUN -> [ABORT][20] ([i915#8668] / [i915#9451])
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_125322v1/bat-adlp-11/igt@kms_pipe_crc_basic@read-crc-frame-sequence@pipe-c-dp-5.html

  * igt@kms_psr@cursor_plane_move:
    - fi-ilk-650:         NOTRUN -> [SKIP][21] ([fdo#109271]) +19 other tests skip
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_125322v1/fi-ilk-650/igt@kms_psr@cursor_plane_move.html

  * igt@kms_psr@sprite_plane_onoff:
    - bat-dg2-9:          NOTRUN -> [SKIP][22] ([i915#1072]) +3 other tests skip
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_125322v1/bat-dg2-9/igt@kms_psr@sprite_plane_onoff.html

  * igt@kms_setmode@basic-clone-single-crtc:
    - bat-dg2-9:          NOTRUN -> [SKIP][23] ([i915#3555] / [i915#4098])
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_125322v1/bat-dg2-9/igt@kms_setmode@basic-clone-single-crtc.html

  * igt@prime_vgem@basic-fence-flip:
    - bat-dg2-9:          NOTRUN -> [SKIP][24] ([i915#3708])
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_125322v1/bat-dg2-9/igt@prime_vgem@basic-fence-flip.html

  * igt@prime_vgem@basic-fence-mmap:
    - bat-dg2-9:          NOTRUN -> [SKIP][25] ([i915#3708] / [i915#4077]) +1 other test skip
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_125322v1/bat-dg2-9/igt@prime_vgem@basic-fence-mmap.html

  * igt@prime_vgem@basic-write:
    - bat-dg2-9:          NOTRUN -> [SKIP][26] ([i915#3291] / [i915#3708]) +2 other tests skip
   [26]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_125322v1/bat-dg2-9/igt@prime_vgem@basic-write.html

  
#### Possible fixes ####

  * igt@i915_module_load@load:
    - fi-ilk-650:         [INCOMPLETE][27] -> [PASS][28]
   [27]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13774/fi-ilk-650/igt@i915_module_load@load.html
   [28]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_125322v1/fi-ilk-650/igt@i915_module_load@load.html
    - bat-jsl-1:          [INCOMPLETE][29] -> [PASS][30]
   [29]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13774/bat-jsl-1/igt@i915_module_load@load.html
   [30]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_125322v1/bat-jsl-1/igt@i915_module_load@load.html

  * igt@kms_hdmi_inject@inject-audio:
    - fi-kbl-guc:         [FAIL][31] ([IGT#3]) -> [PASS][32]
   [31]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13774/fi-kbl-guc/igt@kms_hdmi_inject@inject-audio.html
   [32]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_125322v1/fi-kbl-guc/igt@kms_hdmi_inject@inject-audio.html

  * igt@kms_pipe_crc_basic@nonblocking-crc@pipe-a-dp-5:
    - bat-adlp-11:        [DMESG-FAIL][33] ([i915#6868]) -> [PASS][34]
   [33]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13774/bat-adlp-11/igt@kms_pipe_crc_basic@nonblocking-crc@pipe-a-dp-5.html
   [34]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_125322v1/bat-adlp-11/igt@kms_pipe_crc_basic@nonblocking-crc@pipe-a-dp-5.html

  * igt@kms_pipe_crc_basic@nonblocking-crc@pipe-c-dp-5:
    - bat-adlp-11:        [FAIL][35] ([i915#9047]) -> [PASS][36] +2 other tests pass
   [35]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13774/bat-adlp-11/igt@kms_pipe_crc_basic@nonblocking-crc@pipe-c-dp-5.html
   [36]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_125322v1/bat-adlp-11/igt@kms_pipe_crc_basic@nonblocking-crc@pipe-c-dp-5.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [IGT#3]: https://gitlab.freedesktop.org/drm/igt-gpu-tools/issues/3
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109285]: https://bugs.freedesktop.org/show_bug.cgi?id=109285
  [i915#1072]: https://gitlab.freedesktop.org/drm/intel/issues/1072
  [i915#1845]: https://gitlab.freedesktop.org/drm/intel/issues/1845
  [i915#2190]: https://gitlab.freedesktop.org/drm/intel/issues/2190
  [i915#3291]: https://gitlab.freedesktop.org/drm/intel/issues/3291
  [i915#3555]: https://gitlab.freedesktop.org/drm/intel/issues/3555
  [i915#3708]: https://gitlab.freedesktop.org/drm/intel/issues/3708
  [i915#4077]: https://gitlab.freedesktop.org/drm/intel/issues/4077
  [i915#4079]: https://gitlab.freedesktop.org/drm/intel/issues/4079
  [i915#4083]: https://gitlab.freedesktop.org/drm/intel/issues/4083
  [i915#4098]: https://gitlab.freedesktop.org/drm/intel/issues/4098
  [i915#4103]: https://gitlab.freedesktop.org/drm/intel/issues/4103
  [i915#4212]: https://gitlab.freedesktop.org/drm/intel/issues/4212
  [i915#4213]: https://gitlab.freedesktop.org/drm/intel/issues/4213
  [i915#4215]: https://gitlab.freedesktop.org/drm/intel/issues/4215
  [i915#4613]: https://gitlab.freedesktop.org/drm/intel/issues/4613
  [i915#5190]: https://gitlab.freedesktop.org/drm/intel/issues/5190
  [i915#5274]: https://gitlab.freedesktop.org/drm/intel/issues/5274
  [i915#5354]: https://gitlab.freedesktop.org/drm/intel/issues/5354
  [i915#5608]: https://gitlab.freedesktop.org/drm/intel/issues/5608
  [i915#6621]: https://gitlab.freedesktop.org/drm/intel/issues/6621
  [i915#6868]: https://gitlab.freedesktop.org/drm/intel/issues/6868
  [i915#8668]: https://gitlab.freedesktop.org/drm/intel/issues/8668
  [i915#9047]: https://gitlab.freedesktop.org/drm/intel/issues/9047
  [i915#9197]: https://gitlab.freedesktop.org/drm/intel/issues/9197
  [i915#9318]: https://gitlab.freedesktop.org/drm/intel/issues/9318
  [i915#9451]: https://gitlab.freedesktop.org/drm/intel/issues/9451


Build changes
-------------

  * Linux: CI_DRM_13774 -> Patchwork_125322v1

  CI-20190529: 20190529
  CI_DRM_13774: a42b04c09fcd7ddcd3e2206952d49b38e8fb4405 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_7545: 4aac61139d076775a173a75a15156e408a366546 @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
  Patchwork_125322v1: a42b04c09fcd7ddcd3e2206952d49b38e8fb4405 @ git://anongit.freedesktop.org/gfx-ci/linux


### Linux commits

06c66565cf35 drm/i915/lnl: Fix check for TC phy
b0e65b5c9207 drm/i915/lnl: Extend C10/C20 phy

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_125322v1/index.html

[-- Attachment #2: Type: text/html, Size: 13800 bytes --]

[Intel-gfx] ✗ Fi.CI.BUILD: failure for drm/i915/lnl: Assign correct phys (rev2)

[Patchwork]

== Series Details ==

Series: drm/i915/lnl: Assign correct phys (rev2)
URL   : https://patchwork.freedesktop.org/series/125322/
State : failure

== Summary ==

Error: patch https://patchwork.freedesktop.org/api/1.0/series/125322/revisions/2/mbox/ not applied
Applying: drm/i915/lnl: Extend C10/C20 phy
Applying: drm/i915/lnl: Fix check for TC phy
error: patch failed: drivers/gpu/drm/i915/display/intel_display.c:1784
error: drivers/gpu/drm/i915/display/intel_display.c: patch does not apply
error: Did you hand edit your patch?
It does not apply to blobs recorded in its index.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Using index info to reconstruct a base tree...
Patch failed at 0002 drm/i915/lnl: Fix check for TC phy
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Build failed, no error log produced