Hi Imre,
I have question related to tc legacy handling. I am looking in the context of discrete cards.
> -----Original Message-----
> From: Deak, Imre <imre.deak@xxxxxxxxx>
> Sent: Friday, December 22, 2023 3:44 AM
> To: Sripada, Radhakrishna <radhakrishna.sripada@xxxxxxxxx>
> Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> Subject: Re: [PATCH v2 4/4] drm/i915: Separate tc check for legacy and non
> legacy tc phys
>
> On Wed, Dec 20, 2023 at 02:13:41PM -0800, Radhakrishna Sripada wrote:
> > Starting MTL and DG2 if a phy is not marked as USB-typeC or TBT capable
> > by vbt we should not consider it as a Legacy type-c phy.
> >
> > The concept of Legacy-tc existed in platforms from Icelake to Alder lake
> > where an external FIA can be routed to one of the phy's thus making the phy
> > tc capable without being marked in the vbt.
> >
> > Discrete cards have native ports and client products post MTL have a 1:1
> > mapping with type-C subsystem which is advertised by the bios. So rely on
> > the vbt info regarding type-c capability.
> >
> > Signed-off-by: Radhakrishna Sripada <radhakrishna.sripada@xxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/display/intel_ddi.c | 2 +-
> > drivers/gpu/drm/i915/display/intel_display.c | 29 ++++++++++++-------
> > .../drm/i915/display/intel_display_device.h | 1 +
> > 3 files changed, 21 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c
> b/drivers/gpu/drm/i915/display/intel_ddi.c
> > index 12a29363e5df..7d5b95f97d5f 100644
> > --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> > @@ -5100,7 +5100,7 @@ void intel_ddi_init(struct drm_i915_private
> *dev_priv,
> > }
> >
> > if (intel_phy_is_tc(dev_priv, phy)) {
> > - bool is_legacy =
> > + bool is_legacy = HAS_LEGACY_TC(dev_priv) &&
>
> This doesn't make sense to me. PHYs are either TC or non-TC (aka combo
> PHY) and TC PHYs can be configured to work either in legacy (a TC PHY
> wired to a native DP connector) or non-legacy mode (a TC PHY wired to a
> TC connector). So this would break the functionality on MTL native DP
> connectors and all future platforms I looked at which also support this.
I understand. I want to figure out a way to determine if a phy is connected to
FIA. Like in DG2, the phy's are not connected to FIA. I am assuming that will be
the case for all future discrete cards as well. So instead of looking/building an
if-else ladder for the phy check, is there something that we can rely on viz. vbt, register etc.
to determine if FIA is connected to a phy?
>
> > !intel_bios_encoder_supports_typec_usb(devdata) &&
> > !intel_bios_encoder_supports_tbt(devdata);
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> b/drivers/gpu/drm/i915/display/intel_display.c
> > index b10aad15a63d..03006c9af824 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -1854,17 +1854,9 @@ bool intel_phy_is_combo(struct drm_i915_private
> *dev_priv, enum phy phy)
> > return false;
> > }
> >
> > -bool intel_phy_is_tc(struct drm_i915_private *dev_priv, enum phy phy)
> > +static bool intel_phy_is_legacy_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))
> > - return false;
> > -
> > - if (DISPLAY_VER(dev_priv) >= 13)
> > + 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;
> > @@ -1874,6 +1866,23 @@ bool intel_phy_is_tc(struct drm_i915_private
> *dev_priv, enum phy phy)
> > return false;
> > }
> >
> > +static bool intel_phy_is_vbt_tc(struct drm_i915_private *dev_priv, enum phy
> phy)
> > +{
> > + const struct intel_bios_encoder_data *data =
> > + intel_bios_encoder_phy_data_lookup(dev_priv, phy);
> > +
> > + return intel_bios_encoder_supports_typec_usb(data) &&
> > + intel_bios_encoder_supports_tbt(data);
>
> Based on the above, this doesn't look correct: a TC PHY can be
> configured by the vendor (reflected by the above typec_usb and tbt flags
> in VBT) in any of the following ways:
>
> - legacy mode (wired to a native DP connector): typec_usb:false, tbt:false
> - tbt-alt + dp-alt support (wired to a TC connector): typec_usb:true, tbt:true
> - tbt-alt only support (wired to a TC connector): typec_usb:false, tbt:true
> - dp-alt only support (wired to a TC connector): typec_usb:true, tbt:false
>
> For all the above cases intel_phy_is_tc() should return true. So I don't
> think this (is a PHY TC or non-TC) can be determined based on the above
> VBT flags.
Thanks for this clarification Imre. I am looking to see from the driver if we can make a determination if
a phy is connected to a FIA to make this decision. Is there a way that you could suggest?
Thanks,
Radhakrishna Sripada
>
> > +}
> > +
> > +bool intel_phy_is_tc(struct drm_i915_private *dev_priv, enum phy phy)
> > +{
> > + if (!HAS_LEGACY_TC(dev_priv))
> > + return intel_phy_is_vbt_tc(dev_priv, phy);
> > + else
> > + return intel_phy_is_legacy_tc(dev_priv, phy);
> > +}
> > +
> > bool intel_phy_is_snps(struct drm_i915_private *dev_priv, enum phy phy)
> > {
> > /*
> > diff --git a/drivers/gpu/drm/i915/display/intel_display_device.h
> b/drivers/gpu/drm/i915/display/intel_display_device.h
> > index fe4268813786..9450e263c873 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display_device.h
> > +++ b/drivers/gpu/drm/i915/display/intel_display_device.h
> > @@ -58,6 +58,7 @@ struct drm_printer;
> > #define HAS_IPS(i915) (IS_HASWELL_ULT(i915) ||
> IS_BROADWELL(i915))
> > #define HAS_LRR(i915) (DISPLAY_VER(i915) >= 12)
> > #define HAS_LSPCON(i915) (IS_DISPLAY_VER(i915, 9, 10))
> > +#define HAS_LEGACY_TC(i915) (IS_DISPLAY_VER(i915, 11, 13)
> && !IS_DGFX(i915))
> > #define HAS_MBUS_JOINING(i915) (IS_ALDERLAKE_P(i915) ||
> DISPLAY_VER(i915) >= 14)
> > #define HAS_MSO(i915) (DISPLAY_VER(i915) >= 12)
> > #define HAS_OVERLAY(i915) (DISPLAY_INFO(i915)->has_overlay)
> > --
> > 2.34.1
> >
