RE: [PATCH v2 4/4] drm/i915: Separate tc check for legacy and non legacy tc phys
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
> Sent: Wednesday, January 3, 2024 8:51 AM
> To: Sripada, Radhakrishna
> 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
> > > Sent: Friday, December 22, 2023 3:44 AM
> > > To: Sripada, Radhakrishna
> > > 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
> > > > ---
> > > > 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;
> > > >
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
> > Sent: Friday, December 22, 2023 3:44 AM
> > To: Sripada, Radhakrishna
> > 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
> > > ---
> > > 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_
RE: [PATCH v2 4/4] drm/i915: Separate tc check for legacy and non legacy tc phys
Hi Imre,
I have question related to tc legacy handling. I am looking in the context of
discrete cards.
> -Original Message-
> From: Deak, Imre
> Sent: Friday, December 22, 2023 3:44 AM
> To: Sripada, Radhakrishna
> 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
> > ---
> > 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:
>
> -
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
> ---
> 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.
> !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.
> +}
> +
> +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)
