[PATCH v2 0/4] TC phy check cleanup

[PATCH v2 0/4] TC phy check cleanup

[Radhakrishna Sripada]

We are relying on end-less if-else ladders for a type-c phy
capabilities check. Though it made sense when platforms supported
legacy type-c support, modern platforms rely on the information
passed by vbt. This cleanup restricts the if-else ladder to the
platforms supporting legacy type-c phys and relies on vbt info
for modern client and discrete platforms.

v2: Move cleanup vbt later to handle safe encoder removal

Radhakrishna Sripada (4):
  drm/i915: Move intel_bios_driver_remove later
  drm/i915: Rename intel_bios_encoder_data_lookup as a port variant
  drm/i915: Introduce intel_encoder_phy_data_lookup
  drm/i915: Separate tc check for legacy and non legacy tc phys

 drivers/gpu/drm/i915/display/g4x_dp.c         |  2 +-
 drivers/gpu/drm/i915/display/g4x_hdmi.c       |  2 +-
 drivers/gpu/drm/i915/display/intel_bios.c     | 15 +++++++++-
 drivers/gpu/drm/i915/display/intel_bios.h     |  5 +++-
 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 +
 .../drm/i915/display/intel_display_driver.c   |  4 +--
 drivers/gpu/drm/i915/display/intel_dp.c       |  2 +-
 9 files changed, 44 insertions(+), 18 deletions(-)

-- 
2.34.1

[PATCH v2 1/4] drm/i915: Move intel_bios_driver_remove later

[Radhakrishna Sripada]

Vbt structures will be used during mode config cleanup. Move
the vbt structures cleanup to later time to accommodate cleaner
mode config removal.

Signed-off-by: Radhakrishna Sripada <radhakrishna.sripada@intel.com>
---
 drivers/gpu/drm/i915/display/intel_display_driver.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_display_driver.c b/drivers/gpu/drm/i915/display/intel_display_driver.c
index 9df9097a0255..002bf7caa7eb 100644
--- a/drivers/gpu/drm/i915/display/intel_display_driver.c
+++ b/drivers/gpu/drm/i915/display/intel_display_driver.c
@@ -466,6 +466,8 @@ void intel_display_driver_remove_noirq(struct drm_i915_private *i915)
 	destroy_workqueue(i915->display.wq.modeset);
 
 	intel_fbc_cleanup(i915);
+
+	intel_bios_driver_remove(i915);
 }
 
 /* part #3: call after gem init */
@@ -476,8 +478,6 @@ void intel_display_driver_remove_nogem(struct drm_i915_private *i915)
 	intel_power_domains_driver_remove(i915);
 
 	intel_vga_unregister(i915);
-
-	intel_bios_driver_remove(i915);
 }
 
 void intel_display_driver_unregister(struct drm_i915_private *i915)
-- 
2.34.1

[PATCH v2 2/4] drm/i915: Rename intel_bios_encoder_data_lookup as a port variant

[Radhakrishna Sripada]

intel_bios_encoder_data_lookup takes enum port as an argument. A variant
based out of phy based lookup is required to be used out of vbt code which
will be introduced in a later patch. Hence indicate the current variant as
intel_bios_encoder_port_data_lookup.

Signed-off-by: Radhakrishna Sripada <radhakrishna.sripada@intel.com>
---
 drivers/gpu/drm/i915/display/g4x_dp.c     | 2 +-
 drivers/gpu/drm/i915/display/g4x_hdmi.c   | 2 +-
 drivers/gpu/drm/i915/display/intel_bios.c | 2 +-
 drivers/gpu/drm/i915/display/intel_bios.h | 2 +-
 drivers/gpu/drm/i915/display/intel_dp.c   | 2 +-
 5 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/g4x_dp.c b/drivers/gpu/drm/i915/display/g4x_dp.c
index dfe0b07a122d..ecfa55d6f9f5 100644
--- a/drivers/gpu/drm/i915/display/g4x_dp.c
+++ b/drivers/gpu/drm/i915/display/g4x_dp.c
@@ -1298,7 +1298,7 @@ bool g4x_dp_init(struct drm_i915_private *dev_priv,
 	if (!assert_port_valid(dev_priv, port))
 		return false;
 
-	devdata = intel_bios_encoder_data_lookup(dev_priv, port);
+	devdata = intel_bios_encoder_port_data_lookup(dev_priv, port);
 
 	/* FIXME bail? */
 	if (!devdata)
diff --git a/drivers/gpu/drm/i915/display/g4x_hdmi.c b/drivers/gpu/drm/i915/display/g4x_hdmi.c
index 8096492b3fad..3aedbb6f13c8 100644
--- a/drivers/gpu/drm/i915/display/g4x_hdmi.c
+++ b/drivers/gpu/drm/i915/display/g4x_hdmi.c
@@ -697,7 +697,7 @@ void g4x_hdmi_init(struct drm_i915_private *dev_priv,
 	if (!assert_hdmi_port_valid(dev_priv, port))
 		return;
 
-	devdata = intel_bios_encoder_data_lookup(dev_priv, port);
+	devdata = intel_bios_encoder_port_data_lookup(dev_priv, port);
 
 	/* FIXME bail? */
 	if (!devdata)
diff --git a/drivers/gpu/drm/i915/display/intel_bios.c b/drivers/gpu/drm/i915/display/intel_bios.c
index aa169b0055e9..febc607267f0 100644
--- a/drivers/gpu/drm/i915/display/intel_bios.c
+++ b/drivers/gpu/drm/i915/display/intel_bios.c
@@ -3636,7 +3636,7 @@ bool intel_bios_encoder_hpd_invert(const struct intel_bios_encoder_data *devdata
 }
 
 const struct intel_bios_encoder_data *
-intel_bios_encoder_data_lookup(struct drm_i915_private *i915, enum port port)
+intel_bios_encoder_port_data_lookup(struct drm_i915_private *i915, enum port port)
 {
 	struct intel_bios_encoder_data *devdata;
 
diff --git a/drivers/gpu/drm/i915/display/intel_bios.h b/drivers/gpu/drm/i915/display/intel_bios.h
index 49e24b7cf675..a296aad7f545 100644
--- a/drivers/gpu/drm/i915/display/intel_bios.h
+++ b/drivers/gpu/drm/i915/display/intel_bios.h
@@ -255,7 +255,7 @@ bool intel_bios_port_supports_typec_usb(struct drm_i915_private *i915, enum port
 bool intel_bios_port_supports_tbt(struct drm_i915_private *i915, enum port port);
 
 const struct intel_bios_encoder_data *
-intel_bios_encoder_data_lookup(struct drm_i915_private *i915, enum port port);
+intel_bios_encoder_port_data_lookup(struct drm_i915_private *i915, enum port port);
 
 bool intel_bios_encoder_supports_dvi(const struct intel_bios_encoder_data *devdata);
 bool intel_bios_encoder_supports_hdmi(const struct intel_bios_encoder_data *devdata);
diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
index 3dfca8d88a6f..76f1b8828df8 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -6155,7 +6155,7 @@ static bool _intel_dp_is_port_edp(struct drm_i915_private *dev_priv,
 bool intel_dp_is_port_edp(struct drm_i915_private *i915, enum port port)
 {
 	const struct intel_bios_encoder_data *devdata =
-		intel_bios_encoder_data_lookup(i915, port);
+		intel_bios_encoder_port_data_lookup(i915, port);
 
 	return _intel_dp_is_port_edp(i915, devdata, port);
 }
-- 
2.34.1

[PATCH v2 3/4] drm/i915: Introduce intel_encoder_phy_data_lookup

[Radhakrishna Sripada]

This patch introduces phy version of intel_encoder_port_data_lookup.
Port based variant is dependent on vbt child data extraction and
conversion to port data to be used further. Port data is not immediately
available and is difficult to be determined from phy info.

Signed-off-by: Radhakrishna Sripada <radhakrishna.sripada@intel.com>
---
 drivers/gpu/drm/i915/display/intel_bios.c | 13 +++++++++++++
 drivers/gpu/drm/i915/display/intel_bios.h |  3 +++
 2 files changed, 16 insertions(+)

diff --git a/drivers/gpu/drm/i915/display/intel_bios.c b/drivers/gpu/drm/i915/display/intel_bios.c
index febc607267f0..0b21364b2bdf 100644
--- a/drivers/gpu/drm/i915/display/intel_bios.c
+++ b/drivers/gpu/drm/i915/display/intel_bios.c
@@ -3648,6 +3648,19 @@ intel_bios_encoder_port_data_lookup(struct drm_i915_private *i915, enum port por
 	return NULL;
 }
 
+const struct intel_bios_encoder_data *
+intel_bios_encoder_phy_data_lookup(struct drm_i915_private *i915, enum phy phy)
+{
+	struct intel_bios_encoder_data *devdata;
+
+	list_for_each_entry(devdata, &i915->display.vbt.display_devices, node) {
+		if (intel_port_to_phy(i915, intel_bios_encoder_port(devdata)) == phy)
+			return devdata;
+	}
+
+	return NULL;
+}
+
 void intel_bios_for_each_encoder(struct drm_i915_private *i915,
 				 void (*func)(struct drm_i915_private *i915,
 					      const struct intel_bios_encoder_data *devdata))
diff --git a/drivers/gpu/drm/i915/display/intel_bios.h b/drivers/gpu/drm/i915/display/intel_bios.h
index a296aad7f545..2861ebb13909 100644
--- a/drivers/gpu/drm/i915/display/intel_bios.h
+++ b/drivers/gpu/drm/i915/display/intel_bios.h
@@ -39,6 +39,7 @@ struct intel_crtc_state;
 struct intel_encoder;
 struct intel_panel;
 enum aux_ch;
+enum phy;
 enum port;
 
 enum intel_backlight_type {
@@ -254,6 +255,8 @@ bool intel_bios_get_dsc_params(struct intel_encoder *encoder,
 bool intel_bios_port_supports_typec_usb(struct drm_i915_private *i915, enum port port);
 bool intel_bios_port_supports_tbt(struct drm_i915_private *i915, enum port port);
 
+const struct intel_bios_encoder_data *
+intel_bios_encoder_phy_data_lookup(struct drm_i915_private *i915, enum phy phy);
 const struct intel_bios_encoder_data *
 intel_bios_encoder_port_data_lookup(struct drm_i915_private *i915, enum port port);
 
-- 
2.34.1

[PATCH v2 4/4] drm/i915: Separate tc check for legacy and non legacy tc phys

[Radhakrishna Sripada]

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@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) &&
 			!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);
+}
+
+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

Re: [PATCH v2 0/4] TC phy check cleanup

[Jani Nikula]

On Wed, 20 Dec 2023, Radhakrishna Sripada <radhakrishna.sripada@intel.com> wrote:
> We are relying on end-less if-else ladders for a type-c phy
> capabilities check. Though it made sense when platforms supported
> legacy type-c support, modern platforms rely on the information
> passed by vbt. This cleanup restricts the if-else ladder to the
> platforms supporting legacy type-c phys and relies on vbt info
> for modern client and discrete platforms.

This series is moving the vbt handling in the wrong direction.

We want to *avoid* new lookups. The idea is that you do the lookup
*once* when initializing the encoder, and after that you use
encoder->devdata.

If you look at the intel_phy_is_tc() call sites, you'll observe that
almost all of the places have the encoder (or devdata) already
available. They get the port from encoder->port, and the phy from
intel_port_to_phy().

So this throws away the information that's already available, and then
goes to look it up again in the worst possible way.

You should convert intel_phy_is_tc() to something like
intel_encoder_is_tc(), and pass encoder to it instead of phy. Similarly,
intel_port_to_tc() should be converted to intel_encoder_to_tc().

There are a couple of special cases that only have devdata or phy. But
we can handle the special cases after making the common case
straightforward.


BR,
Jani.


>
> v2: Move cleanup vbt later to handle safe encoder removal
>
> Radhakrishna Sripada (4):
>   drm/i915: Move intel_bios_driver_remove later
>   drm/i915: Rename intel_bios_encoder_data_lookup as a port variant
>   drm/i915: Introduce intel_encoder_phy_data_lookup
>   drm/i915: Separate tc check for legacy and non legacy tc phys
>
>  drivers/gpu/drm/i915/display/g4x_dp.c         |  2 +-
>  drivers/gpu/drm/i915/display/g4x_hdmi.c       |  2 +-
>  drivers/gpu/drm/i915/display/intel_bios.c     | 15 +++++++++-
>  drivers/gpu/drm/i915/display/intel_bios.h     |  5 +++-
>  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 +
>  .../drm/i915/display/intel_display_driver.c   |  4 +--
>  drivers/gpu/drm/i915/display/intel_dp.c       |  2 +-
>  9 files changed, 44 insertions(+), 18 deletions(-)

-- 
Jani Nikula, Intel

RE: [PATCH v2 0/4] TC phy check cleanup

[Sripada, Radhakrishna]

Hi Jani,

> -----Original Message-----
> From: Jani Nikula <jani.nikula@linux.intel.com>
> Sent: Thursday, December 21, 2023 2:04 AM
> To: Sripada, Radhakrishna <radhakrishna.sripada@intel.com>; intel-
> gfx@lists.freedesktop.org
> Subject: Re: [PATCH v2 0/4] TC phy check cleanup
> 
> On Wed, 20 Dec 2023, Radhakrishna Sripada <radhakrishna.sripada@intel.com>
> wrote:
> > We are relying on end-less if-else ladders for a type-c phy
> > capabilities check. Though it made sense when platforms supported
> > legacy type-c support, modern platforms rely on the information
> > passed by vbt. This cleanup restricts the if-else ladder to the
> > platforms supporting legacy type-c phys and relies on vbt info
> > for modern client and discrete platforms.
> 
> This series is moving the vbt handling in the wrong direction.
> 
> We want to *avoid* new lookups. The idea is that you do the lookup
> *once* when initializing the encoder, and after that you use
> encoder->devdata.
> 
> If you look at the intel_phy_is_tc() call sites, you'll observe that
> almost all of the places have the encoder (or devdata) already
> available. They get the port from encoder->port, and the phy from
> intel_port_to_phy().
> 
> So this throws away the information that's already available, and then
> goes to look it up again in the worst possible way.
> 
> You should convert intel_phy_is_tc() to something like
> intel_encoder_is_tc(), and pass encoder to it instead of phy. Similarly,
> intel_port_to_tc() should be converted to intel_encoder_to_tc().
> 
> There are a couple of special cases that only have devdata or phy. But
> we can handle the special cases after making the common case
> straightforward.
Common case making a tc check out of encoder sure makes lot of sense
And agreed to you point. The question that arises in the special cases.
For ex. during vbt_defaults init in intel_bios.c, I can only handle for legacy tc ports.

In other cases where only phy info is available, I may have to iterate over all the
drm_encoders to obtain port info and compare it with phy info adding lot of lookup
overhead. Or we may have to use encoder based lookups in all associated lookups like
icl_port_to_ddc_pin etc.

Thoughts?

Radhakrishna(RK) Sripada
> 
> 
> BR,
> Jani.
> 
> 
> >
> > v2: Move cleanup vbt later to handle safe encoder removal
> >
> > Radhakrishna Sripada (4):
> >   drm/i915: Move intel_bios_driver_remove later
> >   drm/i915: Rename intel_bios_encoder_data_lookup as a port variant
> >   drm/i915: Introduce intel_encoder_phy_data_lookup
> >   drm/i915: Separate tc check for legacy and non legacy tc phys
> >
> >  drivers/gpu/drm/i915/display/g4x_dp.c         |  2 +-
> >  drivers/gpu/drm/i915/display/g4x_hdmi.c       |  2 +-
> >  drivers/gpu/drm/i915/display/intel_bios.c     | 15 +++++++++-
> >  drivers/gpu/drm/i915/display/intel_bios.h     |  5 +++-
> >  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 +
> >  .../drm/i915/display/intel_display_driver.c   |  4 +--
> >  drivers/gpu/drm/i915/display/intel_dp.c       |  2 +-
> >  9 files changed, 44 insertions(+), 18 deletions(-)
> 
> --
> Jani Nikula, Intel

RE: [PATCH v2 0/4] TC phy check cleanup

[Jani Nikula]

On Fri, 22 Dec 2023, "Sripada, Radhakrishna" <radhakrishna.sripada@intel.com> wrote:
> Hi Jani,
>
>> -----Original Message-----
>> From: Jani Nikula <jani.nikula@linux.intel.com>
>> Sent: Thursday, December 21, 2023 2:04 AM
>> To: Sripada, Radhakrishna <radhakrishna.sripada@intel.com>; intel-
>> gfx@lists.freedesktop.org
>> Subject: Re: [PATCH v2 0/4] TC phy check cleanup
>> 
>> On Wed, 20 Dec 2023, Radhakrishna Sripada <radhakrishna.sripada@intel.com>
>> wrote:
>> > We are relying on end-less if-else ladders for a type-c phy
>> > capabilities check. Though it made sense when platforms supported
>> > legacy type-c support, modern platforms rely on the information
>> > passed by vbt. This cleanup restricts the if-else ladder to the
>> > platforms supporting legacy type-c phys and relies on vbt info
>> > for modern client and discrete platforms.
>> 
>> This series is moving the vbt handling in the wrong direction.
>> 
>> We want to *avoid* new lookups. The idea is that you do the lookup
>> *once* when initializing the encoder, and after that you use
>> encoder->devdata.
>> 
>> If you look at the intel_phy_is_tc() call sites, you'll observe that
>> almost all of the places have the encoder (or devdata) already
>> available. They get the port from encoder->port, and the phy from
>> intel_port_to_phy().
>> 
>> So this throws away the information that's already available, and then
>> goes to look it up again in the worst possible way.
>> 
>> You should convert intel_phy_is_tc() to something like
>> intel_encoder_is_tc(), and pass encoder to it instead of phy. Similarly,
>> intel_port_to_tc() should be converted to intel_encoder_to_tc().
>> 
>> There are a couple of special cases that only have devdata or phy. But
>> we can handle the special cases after making the common case
>> straightforward.
> Common case making a tc check out of encoder sure makes lot of sense
> And agreed to you point. The question that arises in the special cases.
> For ex. during vbt_defaults init in intel_bios.c, I can only handle for legacy tc ports.
>
> In other cases where only phy info is available, I may have to iterate over all the
> drm_encoders to obtain port info and compare it with phy info adding lot of lookup
> overhead. Or we may have to use encoder based lookups in all associated lookups like
> icl_port_to_ddc_pin etc.
>
> Thoughts?

Frankly, the way I often handle stuff like this is just start making the
changes for the things that obviously make sense. Such as looking the tc
info up via the encoder. You can add the new function(s) and gradually
switch over. It's mostly mechanical changes and pretty quick to do. I
think it'll even clean up a bunch of local enum port and enum phy
usages.

And often the answer to the rest just presents itself.

Sometimes the answer is, well, to leave an ugly wart in 1% of the cases
while making 99% of the cases pretty. And that's still better than
having 100% poor design.

Sometimes you find out you have to redo some of the stuff you did at
first, but it's because you figure things out along the way. For me at
least, this is how the bright ideas come to mind more often than via up
front design attempts.

HTH,
Jani.


>
> Radhakrishna(RK) Sripada
>> 
>> 
>> BR,
>> Jani.
>> 
>> 
>> >
>> > v2: Move cleanup vbt later to handle safe encoder removal
>> >
>> > Radhakrishna Sripada (4):
>> >   drm/i915: Move intel_bios_driver_remove later
>> >   drm/i915: Rename intel_bios_encoder_data_lookup as a port variant
>> >   drm/i915: Introduce intel_encoder_phy_data_lookup
>> >   drm/i915: Separate tc check for legacy and non legacy tc phys
>> >
>> >  drivers/gpu/drm/i915/display/g4x_dp.c         |  2 +-
>> >  drivers/gpu/drm/i915/display/g4x_hdmi.c       |  2 +-
>> >  drivers/gpu/drm/i915/display/intel_bios.c     | 15 +++++++++-
>> >  drivers/gpu/drm/i915/display/intel_bios.h     |  5 +++-
>> >  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 +
>> >  .../drm/i915/display/intel_display_driver.c   |  4 +--
>> >  drivers/gpu/drm/i915/display/intel_dp.c       |  2 +-
>> >  9 files changed, 44 insertions(+), 18 deletions(-)
>> 
>> --
>> Jani Nikula, Intel

-- 
Jani Nikula, Intel

Re: [PATCH v2 4/4] drm/i915: Separate tc check for legacy and non legacy tc phys

[Imre Deak]

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@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.

>  			!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)		(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
> 

RE: [PATCH v2 4/4] drm/i915: Separate tc check for legacy and non legacy tc phys

[Sripada, Radhakrishna]

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@intel.com>
> Sent: Friday, December 22, 2023 3:44 AM
> To: Sripada, Radhakrishna <radhakrishna.sripada@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.sripada@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?

> 
> >  			!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
> >

Re: [PATCH v2 4/4] drm/i915: Separate tc check for legacy and non legacy tc phys

[Imre Deak]

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.deak@intel.com>
> > Sent: Friday, December 22, 2023 3:44 AM
> > To: Sripada, Radhakrishna <radhakrishna.sripada@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.sripada@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
> > >

RE: [PATCH v2 4/4] drm/i915: Separate tc check for legacy and non legacy tc phys

[Sripada, Radhakrishna]

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.deak@intel.com>
> Sent: Wednesday, January 3, 2024 8:51 AM
> To: Sripada, Radhakrishna <radhakrishna.sripada@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.deak@intel.com>
> > > Sent: Friday, December 22, 2023 3:44 AM
> > > To: Sripada, Radhakrishna <radhakrishna.sripada@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.sripada@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
> > > >

RE: [PATCH v2 0/4] TC phy check cleanup

[Sripada, Radhakrishna]

Hi Jani,

Thank you for that insight. I will use it for future reference. And as in this case the 1%wart looks better.
Need to evaluate if having a tc_phy_mask(as pointed by Imre) in platform info will make things pretty in this case.

Regards,
Radhakrishna(RK) Sripada

> -----Original Message-----
> From: Jani Nikula <jani.nikula@linux.intel.com>
> Sent: Friday, December 22, 2023 2:04 AM
> To: Sripada, Radhakrishna <radhakrishna.sripada@intel.com>; intel-
> gfx@lists.freedesktop.org
> Subject: RE: [PATCH v2 0/4] TC phy check cleanup
> 
> On Fri, 22 Dec 2023, "Sripada, Radhakrishna" <radhakrishna.sripada@intel.com>
> wrote:
> > Hi Jani,
> >
> >> -----Original Message-----
> >> From: Jani Nikula <jani.nikula@linux.intel.com>
> >> Sent: Thursday, December 21, 2023 2:04 AM
> >> To: Sripada, Radhakrishna <radhakrishna.sripada@intel.com>; intel-
> >> gfx@lists.freedesktop.org
> >> Subject: Re: [PATCH v2 0/4] TC phy check cleanup
> >>
> >> On Wed, 20 Dec 2023, Radhakrishna Sripada
> <radhakrishna.sripada@intel.com>
> >> wrote:
> >> > We are relying on end-less if-else ladders for a type-c phy
> >> > capabilities check. Though it made sense when platforms supported
> >> > legacy type-c support, modern platforms rely on the information
> >> > passed by vbt. This cleanup restricts the if-else ladder to the
> >> > platforms supporting legacy type-c phys and relies on vbt info
> >> > for modern client and discrete platforms.
> >>
> >> This series is moving the vbt handling in the wrong direction.
> >>
> >> We want to *avoid* new lookups. The idea is that you do the lookup
> >> *once* when initializing the encoder, and after that you use
> >> encoder->devdata.
> >>
> >> If you look at the intel_phy_is_tc() call sites, you'll observe that
> >> almost all of the places have the encoder (or devdata) already
> >> available. They get the port from encoder->port, and the phy from
> >> intel_port_to_phy().
> >>
> >> So this throws away the information that's already available, and then
> >> goes to look it up again in the worst possible way.
> >>
> >> You should convert intel_phy_is_tc() to something like
> >> intel_encoder_is_tc(), and pass encoder to it instead of phy. Similarly,
> >> intel_port_to_tc() should be converted to intel_encoder_to_tc().
> >>
> >> There are a couple of special cases that only have devdata or phy. But
> >> we can handle the special cases after making the common case
> >> straightforward.
> > Common case making a tc check out of encoder sure makes lot of sense
> > And agreed to you point. The question that arises in the special cases.
> > For ex. during vbt_defaults init in intel_bios.c, I can only handle for legacy tc
> ports.
> >
> > In other cases where only phy info is available, I may have to iterate over all the
> > drm_encoders to obtain port info and compare it with phy info adding lot of
> lookup
> > overhead. Or we may have to use encoder based lookups in all associated
> lookups like
> > icl_port_to_ddc_pin etc.
> >
> > Thoughts?
> 
> Frankly, the way I often handle stuff like this is just start making the
> changes for the things that obviously make sense. Such as looking the tc
> info up via the encoder. You can add the new function(s) and gradually
> switch over. It's mostly mechanical changes and pretty quick to do. I
> think it'll even clean up a bunch of local enum port and enum phy
> usages.
> 
> And often the answer to the rest just presents itself.
> 
> Sometimes the answer is, well, to leave an ugly wart in 1% of the cases
> while making 99% of the cases pretty. And that's still better than
> having 100% poor design.
> 
> Sometimes you find out you have to redo some of the stuff you did at
> first, but it's because you figure things out along the way. For me at
> least, this is how the bright ideas come to mind more often than via up
> front design attempts.
> 
> HTH,
> Jani.
> 
> 
> >
> > Radhakrishna(RK) Sripada
> >>
> >>
> >> BR,
> >> Jani.
> >>
> >>
> >> >
> >> > v2: Move cleanup vbt later to handle safe encoder removal
> >> >
> >> > Radhakrishna Sripada (4):
> >> >   drm/i915: Move intel_bios_driver_remove later
> >> >   drm/i915: Rename intel_bios_encoder_data_lookup as a port variant
> >> >   drm/i915: Introduce intel_encoder_phy_data_lookup
> >> >   drm/i915: Separate tc check for legacy and non legacy tc phys
> >> >
> >> >  drivers/gpu/drm/i915/display/g4x_dp.c         |  2 +-
> >> >  drivers/gpu/drm/i915/display/g4x_hdmi.c       |  2 +-
> >> >  drivers/gpu/drm/i915/display/intel_bios.c     | 15 +++++++++-
> >> >  drivers/gpu/drm/i915/display/intel_bios.h     |  5 +++-
> >> >  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 +
> >> >  .../drm/i915/display/intel_display_driver.c   |  4 +--
> >> >  drivers/gpu/drm/i915/display/intel_dp.c       |  2 +-
> >> >  9 files changed, 44 insertions(+), 18 deletions(-)
> >>
> >> --
> >> Jani Nikula, Intel
> 
> --
> Jani Nikula, Intel