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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

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




[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux