[Intel-gfx] [PATCH 2/2] drm/i915/lnl: Fix check for TC phy
Lucas De Marchi
lucas.demarchi at intel.com
Wed Oct 25 15:44:09 UTC 2023
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 at intel.com>
>>>>---
>>>> drivers/gpu/drm/i915/display/intel_display.c | 29 ++++++++++----------
>>>> 1 file changed, 15 insertions(+), 14 deletions(-)
>>>>
>>>>diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
>>>>index 28d85e1e858e..0797ace31417 100644
>>>>--- a/drivers/gpu/drm/i915/display/intel_display.c
>>>>+++ b/drivers/gpu/drm/i915/display/intel_display.c
>>>>@@ -1784,31 +1784,32 @@ bool intel_phy_is_combo(struct drm_i915_private *dev_priv, enum phy phy)
>>>>
>>>> bool intel_phy_is_tc(struct drm_i915_private *dev_priv, enum phy phy)
>>>> {
>>>>+ /* DG2's "TC1" output uses a SNPS PHY and is handled separately */
>>>> if (IS_DG2(dev_priv))
>>>>- /* DG2's "TC1" output uses a SNPS PHY */
>>>> return false;
>>>>- else if (IS_ALDERLAKE_P(dev_priv) || DISPLAY_VER_FULL(dev_priv) == IP_VER(14, 0))
>>>>+
>>>>+ /*
>>>>+ * TODO: This should mostly match intel_port_to_phy(), considering the
>>>>+ * ports already encode if they are connected to a TC phy in their name.
>>>>+ */
>>>>+ if (IS_LUNARLAKE(dev_priv) || IS_METEORLAKE(dev_priv) ||
>>>>+ IS_ALDERLAKE_P(dev_priv))
>>>
>>>Just like already done with the previous patch, I think we should have a
>>>paragraph in the commit message justifying s/DISPLAY_VER_FULL(dev_priv) ==
>>>IP_VER(14, 0)/IS_METEORLAKE(dev_priv)/.
>>
>>humn... after giving this a second thought, I will take this back.
>>intel_phy_is_tc() is different than the check in the first patch and
>>it's actually something dependent on display engine. Here the check is
>>about is this a DDIA/DDIB or a TC1-TC4? This will change how some
>>registers in the display engine are programmed:
>
>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 at intel.com>
>>Reviewed-by: Gustavo Sousa <gustavo.sousa at intel.com>
>>---
>> drivers/gpu/drm/i915/display/intel_display.c | 28 ++++++++++----------
>> 1 file changed, 14 insertions(+), 14 deletions(-)
>>
>>diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
>>index 28d85e1e858e..1caf46e3e569 100644
>>--- a/drivers/gpu/drm/i915/display/intel_display.c
>>+++ b/drivers/gpu/drm/i915/display/intel_display.c
>>@@ -1784,31 +1784,31 @@ bool intel_phy_is_combo(struct drm_i915_private *dev_priv, enum phy phy)
>>
>> bool intel_phy_is_tc(struct drm_i915_private *dev_priv, enum phy phy)
>> {
>>+ /*
>>+ * DG2's "TC1", although TC-capable output, doesn't share the same flow
>>+ * as other platforms on the display engine side and rather rely on the
>>+ * SNPS PHY, that is programmed separately
>>+ */
>> if (IS_DG2(dev_priv))
>>- /* DG2's "TC1" output uses a SNPS PHY */
>> return false;
>>- else if (IS_ALDERLAKE_P(dev_priv) || DISPLAY_VER_FULL(dev_priv) == IP_VER(14, 0))
>>+
>>+ if (DISPLAY_VER(dev_priv) >= 13)
>> return phy >= PHY_F && phy <= PHY_I;
>> else if (IS_TIGERLAKE(dev_priv))
>> return phy >= PHY_D && phy <= PHY_I;
>> else if (IS_ICELAKE(dev_priv))
>> return phy >= PHY_C && phy <= PHY_F;
>>- else
>>- return false;
>>+
>>+ return false;
>> }
>>
>> bool intel_phy_is_snps(struct drm_i915_private *dev_priv, enum phy phy)
>> {
>>- if (phy == PHY_NONE)
>>- return false;
>>- else if (IS_DG2(dev_priv))
>>- /*
>>- * All four "combo" ports and the TC1 port (PHY E) use
>>- * Synopsis PHYs.
>>- */
>>- return phy <= PHY_E;
>>-
>>- return false;
>>+ /*
>>+ * For DG2, and for DG2 only, all four "combo" ports and the TC1 port
>>+ * (PHY E) use Synopsis PHYs. See intel_phy_is_tc().
>>+ */
>>+ return IS_DG2(dev_priv) && phy > PHY_NONE && phy <= PHY_E;
>> }
>>
>> enum phy intel_port_to_phy(struct drm_i915_private *i915, enum port port)
>>--
>>2.40.1
>>-----------------8<--------------------
>>
>>This would at make intel_phy_is_tc() match intel_port_to_phy(), at least
>>for display version >= 13.
>>
>>Lucas De Marchi
More information about the Intel-gfx
mailing list