| Message ID | 20231018222441.4131237-3-lucas.demarchi@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/lnl: Assign correct phys | expand |
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 >
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)
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
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
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
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)
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(-)