Hi Imre,
Thank you for the pointer. Let me evaluate and see if it is worth taking that
trouble.
Thanks,
Radhakrishna(RK) Sripada
> -----Original Message-----
> From: Deak, Imre <imre.d...@intel.com>
> Sent: Wednesday, January 3, 2024 8:51 AM
> To: Sripada, Radhakrishna <radhakrishna.srip...@intel.com>
> Cc: intel-gfx@lists.freedesktop.org
> Subject: Re: [PATCH v2 4/4] drm/i915: Separate tc check for legacy and non
> legacy tc phys
>
> On Fri, Dec 22, 2023 at 09:47:49PM +0200, Sripada, Radhakrishna wrote:
> > 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.d...@intel.com>
> > > Sent: Friday, December 22, 2023 3:44 AM
> > > To: Sripada, Radhakrishna <radhakrishna.srip...@intel.com>
> > > Cc: intel-gfx@lists.freedesktop.org
> > > 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.srip...@intel.com>
> > > > ---
> > > > 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?
>
> I suppose this question is if a PHY is TypeC or not, TypeC requiring
> some kind of mux (which can be FIA or something else). One other way
> instead of the if-ladder in intel_phy_is_tc() would be adding a
> tc_phy_mask to intel_display_runtime_info, similarly to the port_mask
> there. Not sure how much that would improve things over the current way.
>
> > > > !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
> > > >
