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