[Intel-gfx] [PATCH v3 0/4] drm/i915/tc: some clean-ups in max lane count handling code

[Intel-gfx] [PATCH v3 0/4] drm/i915/tc: some clean-ups in max lane count handling code

[Luca Coelho]

Hi,

Here are four patches with some clean-ups in the code that handles the
max lane count of Type-C connections.

This is done mostly in preparation for a new way to read the pin
assignments and lane count in future devices.

In v2:
   * Fix some rebasing damage.

In v3:
   * Fixed "assigment" typo, as reported by checkpatch.

Please review.

Cheers,
Luca.


Luca Coelho (4):
  drm/i915/tc: rename mtl_tc_port_get_pin_assignment_mask()
  drm/i915/tc: make intel_tc_port_get_lane_mask() static
  drm/i915/tc: move legacy code out of the main _max_lane_count() func
  drm/i915/tc: remove "fia" from intel_tc_port_fia_max_lane_count()

 drivers/gpu/drm/i915/display/intel_cx0_phy.c |  2 +-
 drivers/gpu/drm/i915/display/intel_dp.c      |  4 +--
 drivers/gpu/drm/i915/display/intel_tc.c      | 38 +++++++++++---------
 drivers/gpu/drm/i915/display/intel_tc.h      |  3 +-
 4 files changed, 26 insertions(+), 21 deletions(-)

-- 
2.39.2

[Intel-gfx] [PATCH v3 1/4] drm/i915/tc: rename mtl_tc_port_get_pin_assignment_mask()

[Luca Coelho]

This function doesn't really return the pin assignment mask, but the
max lane count derived from that.  So rename the function to
mtl_tc_port_get_max_lane_count() to better reflect what it really does.

Signed-off-by: Luca Coelho <luciano.coelho@intel.com>
---
 drivers/gpu/drm/i915/display/intel_tc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_tc.c b/drivers/gpu/drm/i915/display/intel_tc.c
index 3ebf41859043..71bbc2b16a0e 100644
--- a/drivers/gpu/drm/i915/display/intel_tc.c
+++ b/drivers/gpu/drm/i915/display/intel_tc.c
@@ -290,7 +290,7 @@ u32 intel_tc_port_get_pin_assignment_mask(struct intel_digital_port *dig_port)
 	       DP_PIN_ASSIGNMENT_SHIFT(tc->phy_fia_idx);
 }
 
-static int mtl_tc_port_get_pin_assignment_mask(struct intel_digital_port *dig_port)
+static int mtl_tc_port_get_max_lane_count(struct intel_digital_port *dig_port)
 {
 	struct drm_i915_private *i915 = to_i915(dig_port->base.base.dev);
 	intel_wakeref_t wakeref;
@@ -325,7 +325,7 @@ int intel_tc_port_fia_max_lane_count(struct intel_digital_port *dig_port)
 	assert_tc_cold_blocked(tc);
 
 	if (DISPLAY_VER(i915) >= 14)
-		return mtl_tc_port_get_pin_assignment_mask(dig_port);
+		return mtl_tc_port_get_max_lane_count(dig_port);
 
 	lane_mask = 0;
 	with_intel_display_power(i915, POWER_DOMAIN_DISPLAY_CORE, wakeref)
-- 
2.39.2

[Intel-gfx] [PATCH v3 2/4] drm/i915/tc: make intel_tc_port_get_lane_mask() static

[Luca Coelho]

This function is only used locally, so make it static and remove the
definition from the header file.

Signed-off-by: Luca Coelho <luciano.coelho@intel.com>
---
 drivers/gpu/drm/i915/display/intel_tc.c | 2 +-
 drivers/gpu/drm/i915/display/intel_tc.h | 1 -
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_tc.c b/drivers/gpu/drm/i915/display/intel_tc.c
index 71bbc2b16a0e..de848b329f4b 100644
--- a/drivers/gpu/drm/i915/display/intel_tc.c
+++ b/drivers/gpu/drm/i915/display/intel_tc.c
@@ -260,7 +260,7 @@ assert_tc_port_power_enabled(struct intel_tc_port *tc)
 		    !intel_display_power_is_enabled(i915, tc_port_power_domain(tc)));
 }
 
-u32 intel_tc_port_get_lane_mask(struct intel_digital_port *dig_port)
+static u32 intel_tc_port_get_lane_mask(struct intel_digital_port *dig_port)
 {
 	struct drm_i915_private *i915 = to_i915(dig_port->base.base.dev);
 	struct intel_tc_port *tc = to_tc_port(dig_port);
diff --git a/drivers/gpu/drm/i915/display/intel_tc.h b/drivers/gpu/drm/i915/display/intel_tc.h
index 3b16491925fa..ffc0e2a74e43 100644
--- a/drivers/gpu/drm/i915/display/intel_tc.h
+++ b/drivers/gpu/drm/i915/display/intel_tc.h
@@ -19,7 +19,6 @@ bool intel_tc_port_in_legacy_mode(struct intel_digital_port *dig_port);
 bool intel_tc_port_connected(struct intel_encoder *encoder);
 bool intel_tc_port_connected_locked(struct intel_encoder *encoder);
 
-u32 intel_tc_port_get_lane_mask(struct intel_digital_port *dig_port);
 u32 intel_tc_port_get_pin_assignment_mask(struct intel_digital_port *dig_port);
 int intel_tc_port_fia_max_lane_count(struct intel_digital_port *dig_port);
 void intel_tc_port_set_fia_lane_count(struct intel_digital_port *dig_port,
-- 
2.39.2

[Intel-gfx] [PATCH v3 3/4] drm/i915/tc: move legacy code out of the main _max_lane_count() func

[Luca Coelho]

This makes the code a bit more symmetric and readable, especially when
we start adding more display version-specific alternatives.

Signed-off-by: Luca Coelho <luciano.coelho@intel.com>
---
 drivers/gpu/drm/i915/display/intel_tc.c | 32 +++++++++++++++----------
 1 file changed, 19 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_tc.c b/drivers/gpu/drm/i915/display/intel_tc.c
index de848b329f4b..43b8eeba26f8 100644
--- a/drivers/gpu/drm/i915/display/intel_tc.c
+++ b/drivers/gpu/drm/i915/display/intel_tc.c
@@ -311,23 +311,12 @@ static int mtl_tc_port_get_max_lane_count(struct intel_digital_port *dig_port)
 	}
 }
 
-int intel_tc_port_fia_max_lane_count(struct intel_digital_port *dig_port)
+static int intel_tc_port_get_max_lane_count(struct intel_digital_port *dig_port)
 {
 	struct drm_i915_private *i915 = to_i915(dig_port->base.base.dev);
-	struct intel_tc_port *tc = to_tc_port(dig_port);
-	enum phy phy = intel_port_to_phy(i915, dig_port->base.port);
 	intel_wakeref_t wakeref;
-	u32 lane_mask;
-
-	if (!intel_phy_is_tc(i915, phy) || tc->mode != TC_PORT_DP_ALT)
-		return 4;
+	u32 lane_mask = 0;
 
-	assert_tc_cold_blocked(tc);
-
-	if (DISPLAY_VER(i915) >= 14)
-		return mtl_tc_port_get_max_lane_count(dig_port);
-
-	lane_mask = 0;
 	with_intel_display_power(i915, POWER_DOMAIN_DISPLAY_CORE, wakeref)
 		lane_mask = intel_tc_port_get_lane_mask(dig_port);
 
@@ -348,6 +337,23 @@ int intel_tc_port_fia_max_lane_count(struct intel_digital_port *dig_port)
 	}
 }
 
+int intel_tc_port_fia_max_lane_count(struct intel_digital_port *dig_port)
+{
+	struct drm_i915_private *i915 = to_i915(dig_port->base.base.dev);
+	struct intel_tc_port *tc = to_tc_port(dig_port);
+	enum phy phy = intel_port_to_phy(i915, dig_port->base.port);
+
+	if (!intel_phy_is_tc(i915, phy) || tc->mode != TC_PORT_DP_ALT)
+		return 4;
+
+	assert_tc_cold_blocked(tc);
+
+	if (DISPLAY_VER(i915) >= 14)
+		return mtl_tc_port_get_max_lane_count(dig_port);
+
+	return intel_tc_port_get_max_lane_count(dig_port);
+}
+
 void intel_tc_port_set_fia_lane_count(struct intel_digital_port *dig_port,
 				      int required_lanes)
 {
-- 
2.39.2

[Intel-gfx] [PATCH v3 4/4] drm/i915/tc: remove "fia" from intel_tc_port_fia_max_lane_count()

[Luca Coelho]

It is irrelevant for the caller that the max lane count is being
derived from a FIA register, so having "fia" in the function name is
irrelevant.  Rename the function accordingly.

Signed-off-by: Luca Coelho <luciano.coelho@intel.com>
---
 drivers/gpu/drm/i915/display/intel_cx0_phy.c | 2 +-
 drivers/gpu/drm/i915/display/intel_dp.c      | 4 ++--
 drivers/gpu/drm/i915/display/intel_tc.c      | 4 ++--
 drivers/gpu/drm/i915/display/intel_tc.h      | 2 +-
 4 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy.c b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
index 1b00ef2c6185..6d4f7b20ce85 100644
--- a/drivers/gpu/drm/i915/display/intel_cx0_phy.c
+++ b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
@@ -2534,7 +2534,7 @@ static void intel_cx0_phy_lane_reset(struct drm_i915_private *i915,
 {
 	enum port port = encoder->port;
 	enum phy phy = intel_port_to_phy(i915, port);
-	bool both_lanes =  intel_tc_port_fia_max_lane_count(enc_to_dig_port(encoder)) > 2;
+	bool both_lanes =  intel_tc_port_max_lane_count(enc_to_dig_port(encoder)) > 2;
 	u8 lane_mask = lane_reversal ? INTEL_CX0_LANE1 :
 				  INTEL_CX0_LANE0;
 	u32 lane_pipe_reset = both_lanes ?
diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
index 03675620e3ea..b974af839acb 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -306,13 +306,13 @@ static int intel_dp_max_common_lane_count(struct intel_dp *intel_dp)
 	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
 	int source_max = intel_dp_max_source_lane_count(dig_port);
 	int sink_max = intel_dp->max_sink_lane_count;
-	int fia_max = intel_tc_port_fia_max_lane_count(dig_port);
+	int port_max = intel_tc_port_max_lane_count(dig_port);
 	int lttpr_max = drm_dp_lttpr_max_lane_count(intel_dp->lttpr_common_caps);
 
 	if (lttpr_max)
 		sink_max = min(sink_max, lttpr_max);
 
-	return min3(source_max, sink_max, fia_max);
+	return min3(source_max, sink_max, port_max);
 }
 
 int intel_dp_max_lane_count(struct intel_dp *intel_dp)
diff --git a/drivers/gpu/drm/i915/display/intel_tc.c b/drivers/gpu/drm/i915/display/intel_tc.c
index 43b8eeba26f8..3c94bbcb5497 100644
--- a/drivers/gpu/drm/i915/display/intel_tc.c
+++ b/drivers/gpu/drm/i915/display/intel_tc.c
@@ -337,7 +337,7 @@ static int intel_tc_port_get_max_lane_count(struct intel_digital_port *dig_port)
 	}
 }
 
-int intel_tc_port_fia_max_lane_count(struct intel_digital_port *dig_port)
+int intel_tc_port_max_lane_count(struct intel_digital_port *dig_port)
 {
 	struct drm_i915_private *i915 = to_i915(dig_port->base.base.dev);
 	struct intel_tc_port *tc = to_tc_port(dig_port);
@@ -589,7 +589,7 @@ static bool tc_phy_verify_legacy_or_dp_alt_mode(struct intel_tc_port *tc,
 	struct intel_digital_port *dig_port = tc->dig_port;
 	int max_lanes;
 
-	max_lanes = intel_tc_port_fia_max_lane_count(dig_port);
+	max_lanes = intel_tc_port_max_lane_count(dig_port);
 	if (tc->mode == TC_PORT_LEGACY) {
 		drm_WARN_ON(&i915->drm, max_lanes != 4);
 		return true;
diff --git a/drivers/gpu/drm/i915/display/intel_tc.h b/drivers/gpu/drm/i915/display/intel_tc.h
index ffc0e2a74e43..80a61e52850e 100644
--- a/drivers/gpu/drm/i915/display/intel_tc.h
+++ b/drivers/gpu/drm/i915/display/intel_tc.h
@@ -20,7 +20,7 @@ bool intel_tc_port_connected(struct intel_encoder *encoder);
 bool intel_tc_port_connected_locked(struct intel_encoder *encoder);
 
 u32 intel_tc_port_get_pin_assignment_mask(struct intel_digital_port *dig_port);
-int intel_tc_port_fia_max_lane_count(struct intel_digital_port *dig_port);
+int intel_tc_port_max_lane_count(struct intel_digital_port *dig_port);
 void intel_tc_port_set_fia_lane_count(struct intel_digital_port *dig_port,
 				      int required_lanes);
 
-- 
2.39.2

[Intel-gfx] ✗ Fi.CI.BAT: failure for drm/i915/tc: some clean-ups in max lane count handling code (rev3)

[Patchwork]

[-- Attachment #1: Type: text/plain, Size: 14249 bytes --]

== Series Details ==

Series: drm/i915/tc: some clean-ups in max lane count handling code (rev3)
URL   : https://patchwork.freedesktop.org/series/120980/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_13403 -> Patchwork_120980v3
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_120980v3 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_120980v3, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_120980v3/index.html

Participating hosts (42 -> 43)
------------------------------

  Additional (1): bat-rpls-2 

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in Patchwork_120980v3:

### IGT changes ###

#### Possible regressions ####

  * igt@i915_pm_rpm@module-reload:
    - bat-dg1-7:          [PASS][1] -> [FAIL][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13403/bat-dg1-7/igt@i915_pm_rpm@module-reload.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_120980v3/bat-dg1-7/igt@i915_pm_rpm@module-reload.html

  
Known issues
------------

  Here are the changes found in Patchwork_120980v3 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@core_auth@basic-auth:
    - bat-adlp-11:        NOTRUN -> [ABORT][3] ([i915#8011])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_120980v3/bat-adlp-11/igt@core_auth@basic-auth.html

  * igt@debugfs_test@basic-hwmon:
    - bat-rpls-2:         NOTRUN -> [SKIP][4] ([i915#7456])
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_120980v3/bat-rpls-2/igt@debugfs_test@basic-hwmon.html

  * igt@fbdev@info:
    - bat-rpls-2:         NOTRUN -> [SKIP][5] ([i915#1849] / [i915#2582])
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_120980v3/bat-rpls-2/igt@fbdev@info.html

  * igt@fbdev@read:
    - bat-rpls-2:         NOTRUN -> [SKIP][6] ([i915#2582]) +3 similar issues
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_120980v3/bat-rpls-2/igt@fbdev@read.html

  * igt@gem_lmem_swapping@verify-random:
    - bat-rpls-2:         NOTRUN -> [SKIP][7] ([i915#4613]) +3 similar issues
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_120980v3/bat-rpls-2/igt@gem_lmem_swapping@verify-random.html

  * igt@gem_tiled_pread_basic:
    - bat-rpls-2:         NOTRUN -> [SKIP][8] ([i915#3282])
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_120980v3/bat-rpls-2/igt@gem_tiled_pread_basic.html

  * igt@i915_pm_backlight@basic-brightness:
    - bat-rpls-2:         NOTRUN -> [SKIP][9] ([i915#7561])
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_120980v3/bat-rpls-2/igt@i915_pm_backlight@basic-brightness.html

  * igt@i915_pm_rpm@basic-pci-d3-state:
    - fi-tgl-1115g4:      [PASS][10] -> [FAIL][11] ([i915#7940])
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13403/fi-tgl-1115g4/igt@i915_pm_rpm@basic-pci-d3-state.html
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_120980v3/fi-tgl-1115g4/igt@i915_pm_rpm@basic-pci-d3-state.html

  * igt@i915_pm_rpm@basic-rte:
    - fi-cfl-8109u:       [PASS][12] -> [FAIL][13] ([i915#7940])
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13403/fi-cfl-8109u/igt@i915_pm_rpm@basic-rte.html
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_120980v3/fi-cfl-8109u/igt@i915_pm_rpm@basic-rte.html
    - bat-adlp-9:         [PASS][14] -> [ABORT][15] ([i915#7977] / [i915#8668])
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13403/bat-adlp-9/igt@i915_pm_rpm@basic-rte.html
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_120980v3/bat-adlp-9/igt@i915_pm_rpm@basic-rte.html

  * igt@i915_pm_rps@basic-api:
    - bat-rpls-2:         NOTRUN -> [SKIP][16] ([i915#6621])
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_120980v3/bat-rpls-2/igt@i915_pm_rps@basic-api.html

  * igt@i915_selftest@live@gt_heartbeat:
    - fi-apl-guc:         [PASS][17] -> [DMESG-FAIL][18] ([i915#5334])
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13403/fi-apl-guc/igt@i915_selftest@live@gt_heartbeat.html
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_120980v3/fi-apl-guc/igt@i915_selftest@live@gt_heartbeat.html

  * igt@i915_selftest@live@gt_pm:
    - bat-rpls-2:         NOTRUN -> [DMESG-FAIL][19] ([i915#4258] / [i915#7913])
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_120980v3/bat-rpls-2/igt@i915_selftest@live@gt_pm.html

  * igt@i915_selftest@live@slpc:
    - bat-rpls-2:         NOTRUN -> [DMESG-WARN][20] ([i915#6367])
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_120980v3/bat-rpls-2/igt@i915_selftest@live@slpc.html

  * igt@i915_suspend@basic-s3-without-i915:
    - bat-mtlp-6:         NOTRUN -> [SKIP][21] ([i915#6645])
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_120980v3/bat-mtlp-6/igt@i915_suspend@basic-s3-without-i915.html
    - bat-rpls-2:         NOTRUN -> [ABORT][22] ([i915#6687] / [i915#7978] / [i915#8668])
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_120980v3/bat-rpls-2/igt@i915_suspend@basic-s3-without-i915.html

  * igt@kms_busy@basic:
    - bat-rpls-2:         NOTRUN -> [SKIP][23] ([i915#1845]) +14 similar issues
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_120980v3/bat-rpls-2/igt@kms_busy@basic.html

  * igt@kms_chamelium_edid@hdmi-edid-read:
    - bat-rpls-2:         NOTRUN -> [SKIP][24] ([i915#7828]) +7 similar issues
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_120980v3/bat-rpls-2/igt@kms_chamelium_edid@hdmi-edid-read.html

  * igt@kms_chamelium_hpd@common-hpd-after-suspend:
    - bat-mtlp-6:         NOTRUN -> [SKIP][25] ([i915#7828])
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_120980v3/bat-mtlp-6/igt@kms_chamelium_hpd@common-hpd-after-suspend.html

  * igt@kms_flip@basic-flip-vs-dpms:
    - bat-rpls-2:         NOTRUN -> [SKIP][26] ([i915#3637]) +3 similar issues
   [26]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_120980v3/bat-rpls-2/igt@kms_flip@basic-flip-vs-dpms.html

  * igt@kms_force_connector_basic@force-load-detect:
    - bat-rpls-2:         NOTRUN -> [SKIP][27] ([fdo#109285])
   [27]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_120980v3/bat-rpls-2/igt@kms_force_connector_basic@force-load-detect.html

  * igt@kms_frontbuffer_tracking@basic:
    - bat-rpls-2:         NOTRUN -> [SKIP][28] ([i915#1849])
   [28]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_120980v3/bat-rpls-2/igt@kms_frontbuffer_tracking@basic.html

  * igt@kms_pipe_crc_basic@suspend-read-crc:
    - bat-mtlp-6:         NOTRUN -> [SKIP][29] ([i915#1845] / [i915#4078])
   [29]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_120980v3/bat-mtlp-6/igt@kms_pipe_crc_basic@suspend-read-crc.html

  * igt@kms_psr@primary_mmap_gtt:
    - bat-rplp-1:         NOTRUN -> [SKIP][30] ([i915#1072]) +1 similar issue
   [30]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_120980v3/bat-rplp-1/igt@kms_psr@primary_mmap_gtt.html

  * igt@kms_psr@sprite_plane_onoff:
    - bat-rpls-2:         NOTRUN -> [SKIP][31] ([i915#1072]) +3 similar issues
   [31]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_120980v3/bat-rpls-2/igt@kms_psr@sprite_plane_onoff.html

  * igt@kms_setmode@basic-clone-single-crtc:
    - bat-rplp-1:         NOTRUN -> [ABORT][32] ([i915#8260] / [i915#8668])
   [32]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_120980v3/bat-rplp-1/igt@kms_setmode@basic-clone-single-crtc.html
    - bat-rpls-2:         NOTRUN -> [SKIP][33] ([i915#3555])
   [33]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_120980v3/bat-rpls-2/igt@kms_setmode@basic-clone-single-crtc.html

  * igt@prime_vgem@basic-fence-flip:
    - bat-rpls-2:         NOTRUN -> [SKIP][34] ([fdo#109295] / [i915#1845] / [i915#3708])
   [34]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_120980v3/bat-rpls-2/igt@prime_vgem@basic-fence-flip.html

  * igt@prime_vgem@basic-write:
    - bat-rpls-2:         NOTRUN -> [SKIP][35] ([fdo#109295] / [i915#3708]) +2 similar issues
   [35]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_120980v3/bat-rpls-2/igt@prime_vgem@basic-write.html

  
#### Possible fixes ####

  * igt@i915_pm_rpm@basic-pci-d3-state:
    - fi-skl-guc:         [FAIL][36] ([i915#7940]) -> [PASS][37]
   [36]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13403/fi-skl-guc/igt@i915_pm_rpm@basic-pci-d3-state.html
   [37]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_120980v3/fi-skl-guc/igt@i915_pm_rpm@basic-pci-d3-state.html
    - fi-cfl-8109u:       [FAIL][38] ([i915#7940]) -> [PASS][39]
   [38]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13403/fi-cfl-8109u/igt@i915_pm_rpm@basic-pci-d3-state.html
   [39]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_120980v3/fi-cfl-8109u/igt@i915_pm_rpm@basic-pci-d3-state.html

  * igt@i915_selftest@live@gt_heartbeat:
    - bat-jsl-1:          [DMESG-FAIL][40] ([i915#5334]) -> [PASS][41]
   [40]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13403/bat-jsl-1/igt@i915_selftest@live@gt_heartbeat.html
   [41]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_120980v3/bat-jsl-1/igt@i915_selftest@live@gt_heartbeat.html

  * igt@i915_selftest@live@requests:
    - bat-mtlp-8:         [DMESG-FAIL][42] ([i915#8497]) -> [PASS][43]
   [42]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13403/bat-mtlp-8/igt@i915_selftest@live@requests.html
   [43]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_120980v3/bat-mtlp-8/igt@i915_selftest@live@requests.html
    - bat-mtlp-6:         [ABORT][44] ([i915#7982]) -> [PASS][45]
   [44]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13403/bat-mtlp-6/igt@i915_selftest@live@requests.html
   [45]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_120980v3/bat-mtlp-6/igt@i915_selftest@live@requests.html

  * igt@i915_selftest@live@slpc:
    - bat-mtlp-8:         [DMESG-WARN][46] ([i915#6367]) -> [PASS][47]
   [46]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13403/bat-mtlp-8/igt@i915_selftest@live@slpc.html
   [47]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_120980v3/bat-mtlp-8/igt@i915_selftest@live@slpc.html

  
#### Warnings ####

  * igt@i915_module_load@load:
    - bat-adlp-11:        [ABORT][48] ([i915#4423]) -> [DMESG-WARN][49] ([i915#4423])
   [48]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13403/bat-adlp-11/igt@i915_module_load@load.html
   [49]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_120980v3/bat-adlp-11/igt@i915_module_load@load.html

  * igt@kms_psr@cursor_plane_move:
    - bat-rplp-1:         [ABORT][50] ([i915#8434] / [i915#8668]) -> [SKIP][51] ([i915#1072])
   [50]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13403/bat-rplp-1/igt@kms_psr@cursor_plane_move.html
   [51]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_120980v3/bat-rplp-1/igt@kms_psr@cursor_plane_move.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#109285]: https://bugs.freedesktop.org/show_bug.cgi?id=109285
  [fdo#109295]: https://bugs.freedesktop.org/show_bug.cgi?id=109295
  [i915#1072]: https://gitlab.freedesktop.org/drm/intel/issues/1072
  [i915#1845]: https://gitlab.freedesktop.org/drm/intel/issues/1845
  [i915#1849]: https://gitlab.freedesktop.org/drm/intel/issues/1849
  [i915#2582]: https://gitlab.freedesktop.org/drm/intel/issues/2582
  [i915#3282]: https://gitlab.freedesktop.org/drm/intel/issues/3282
  [i915#3555]: https://gitlab.freedesktop.org/drm/intel/issues/3555
  [i915#3637]: https://gitlab.freedesktop.org/drm/intel/issues/3637
  [i915#3708]: https://gitlab.freedesktop.org/drm/intel/issues/3708
  [i915#4078]: https://gitlab.freedesktop.org/drm/intel/issues/4078
  [i915#4258]: https://gitlab.freedesktop.org/drm/intel/issues/4258
  [i915#4423]: https://gitlab.freedesktop.org/drm/intel/issues/4423
  [i915#4613]: https://gitlab.freedesktop.org/drm/intel/issues/4613
  [i915#5334]: https://gitlab.freedesktop.org/drm/intel/issues/5334
  [i915#6367]: https://gitlab.freedesktop.org/drm/intel/issues/6367
  [i915#6621]: https://gitlab.freedesktop.org/drm/intel/issues/6621
  [i915#6645]: https://gitlab.freedesktop.org/drm/intel/issues/6645
  [i915#6687]: https://gitlab.freedesktop.org/drm/intel/issues/6687
  [i915#7456]: https://gitlab.freedesktop.org/drm/intel/issues/7456
  [i915#7561]: https://gitlab.freedesktop.org/drm/intel/issues/7561
  [i915#7828]: https://gitlab.freedesktop.org/drm/intel/issues/7828
  [i915#7913]: https://gitlab.freedesktop.org/drm/intel/issues/7913
  [i915#7940]: https://gitlab.freedesktop.org/drm/intel/issues/7940
  [i915#7977]: https://gitlab.freedesktop.org/drm/intel/issues/7977
  [i915#7978]: https://gitlab.freedesktop.org/drm/intel/issues/7978
  [i915#7982]: https://gitlab.freedesktop.org/drm/intel/issues/7982
  [i915#8011]: https://gitlab.freedesktop.org/drm/intel/issues/8011
  [i915#8260]: https://gitlab.freedesktop.org/drm/intel/issues/8260
  [i915#8434]: https://gitlab.freedesktop.org/drm/intel/issues/8434
  [i915#8497]: https://gitlab.freedesktop.org/drm/intel/issues/8497
  [i915#8668]: https://gitlab.freedesktop.org/drm/intel/issues/8668


Build changes
-------------

  * Linux: CI_DRM_13403 -> Patchwork_120980v3

  CI-20190529: 20190529
  CI_DRM_13403: 552b0a7e7498258b719c9e88cfd28558dffc4eec @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_7397: 8d298f3c92ba1afbb43cc7142d3b40c1f681c989 @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
  Patchwork_120980v3: 552b0a7e7498258b719c9e88cfd28558dffc4eec @ git://anongit.freedesktop.org/gfx-ci/linux


### Linux commits

26f6a394c8c6 drm/i915/tc: remove "fia" from intel_tc_port_fia_max_lane_count()
ba7241c60f6d drm/i915/tc: move legacy code out of the main _max_lane_count() func
869d2e60d791 drm/i915/tc: make intel_tc_port_get_lane_mask() static
af509e283d59 drm/i915/tc: rename mtl_tc_port_get_pin_assignment_mask()

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_120980v3/index.html

[-- Attachment #2: Type: text/html, Size: 17021 bytes --]

Re: [Intel-gfx] [PATCH v3 1/4] drm/i915/tc: rename mtl_tc_port_get_pin_assignment_mask()

[Kandpal, Suraj]

> This function doesn't really return the pin assignment mask, but the max lane
> count derived from that.  So rename the function to
> mtl_tc_port_get_max_lane_count() to better reflect what it really does.
>
Maybe also add the version changes on commit messages here as cover letter ends up getting discarded

With that fixed

Reviewed-by: Suraj Kandpal <suraj.kandpal@intel.com>
 
> Signed-off-by: Luca Coelho <luciano.coelho@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_tc.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_tc.c
> b/drivers/gpu/drm/i915/display/intel_tc.c
> index 3ebf41859043..71bbc2b16a0e 100644
> --- a/drivers/gpu/drm/i915/display/intel_tc.c
> +++ b/drivers/gpu/drm/i915/display/intel_tc.c
> @@ -290,7 +290,7 @@ u32 intel_tc_port_get_pin_assignment_mask(struct
> intel_digital_port *dig_port)
>  	       DP_PIN_ASSIGNMENT_SHIFT(tc->phy_fia_idx);
>  }
> 
> -static int mtl_tc_port_get_pin_assignment_mask(struct intel_digital_port
> *dig_port)
> +static int mtl_tc_port_get_max_lane_count(struct intel_digital_port
> +*dig_port)
>  {
>  	struct drm_i915_private *i915 = to_i915(dig_port->base.base.dev);
>  	intel_wakeref_t wakeref;
> @@ -325,7 +325,7 @@ int intel_tc_port_fia_max_lane_count(struct
> intel_digital_port *dig_port)
>  	assert_tc_cold_blocked(tc);
> 
>  	if (DISPLAY_VER(i915) >= 14)
> -		return mtl_tc_port_get_pin_assignment_mask(dig_port);
> +		return mtl_tc_port_get_max_lane_count(dig_port);
> 
>  	lane_mask = 0;
>  	with_intel_display_power(i915, POWER_DOMAIN_DISPLAY_CORE,
> wakeref)
> --
> 2.39.2

Re: [Intel-gfx] [PATCH v3 2/4] drm/i915/tc: make intel_tc_port_get_lane_mask() static

[Kandpal, Suraj]

> This function is only used locally, so make it static and remove the definition
> from the header file.
> 
> Signed-off-by: Luca Coelho <luciano.coelho@intel.com>

LGTM 

Reviewed-by: Suraj Kandpal <suraj.kandpal@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_tc.c | 2 +-
> drivers/gpu/drm/i915/display/intel_tc.h | 1 -
>  2 files changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_tc.c
> b/drivers/gpu/drm/i915/display/intel_tc.c
> index 71bbc2b16a0e..de848b329f4b 100644
> --- a/drivers/gpu/drm/i915/display/intel_tc.c
> +++ b/drivers/gpu/drm/i915/display/intel_tc.c
> @@ -260,7 +260,7 @@ assert_tc_port_power_enabled(struct intel_tc_port
> *tc)
>  		    !intel_display_power_is_enabled(i915,
> tc_port_power_domain(tc)));  }
> 
> -u32 intel_tc_port_get_lane_mask(struct intel_digital_port *dig_port)
> +static u32 intel_tc_port_get_lane_mask(struct intel_digital_port
> +*dig_port)
>  {
>  	struct drm_i915_private *i915 = to_i915(dig_port->base.base.dev);
>  	struct intel_tc_port *tc = to_tc_port(dig_port); diff --git
> a/drivers/gpu/drm/i915/display/intel_tc.h
> b/drivers/gpu/drm/i915/display/intel_tc.h
> index 3b16491925fa..ffc0e2a74e43 100644
> --- a/drivers/gpu/drm/i915/display/intel_tc.h
> +++ b/drivers/gpu/drm/i915/display/intel_tc.h
> @@ -19,7 +19,6 @@ bool intel_tc_port_in_legacy_mode(struct
> intel_digital_port *dig_port);  bool intel_tc_port_connected(struct
> intel_encoder *encoder);  bool intel_tc_port_connected_locked(struct
> intel_encoder *encoder);
> 
> -u32 intel_tc_port_get_lane_mask(struct intel_digital_port *dig_port);
>  u32 intel_tc_port_get_pin_assignment_mask(struct intel_digital_port
> *dig_port);  int intel_tc_port_fia_max_lane_count(struct intel_digital_port
> *dig_port);  void intel_tc_port_set_fia_lane_count(struct intel_digital_port
> *dig_port,
> --
> 2.39.2

Re: [Intel-gfx] [PATCH v3 3/4] drm/i915/tc: move legacy code out of the main _max_lane_count() func

[Kandpal, Suraj]

> This makes the code a bit more symmetric and readable, especially when we
> start adding more display version-specific alternatives.
> 
> Signed-off-by: Luca Coelho <luciano.coelho@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_tc.c | 32 +++++++++++++++----------
>  1 file changed, 19 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_tc.c
> b/drivers/gpu/drm/i915/display/intel_tc.c
> index de848b329f4b..43b8eeba26f8 100644
> --- a/drivers/gpu/drm/i915/display/intel_tc.c
> +++ b/drivers/gpu/drm/i915/display/intel_tc.c
> @@ -311,23 +311,12 @@ static int mtl_tc_port_get_max_lane_count(struct
> intel_digital_port *dig_port)
>  	}
>  }
> 
> -int intel_tc_port_fia_max_lane_count(struct intel_digital_port *dig_port)
> +static int intel_tc_port_get_max_lane_count(struct intel_digital_port
> +*dig_port)
>  {
>  	struct drm_i915_private *i915 = to_i915(dig_port->base.base.dev);
> -	struct intel_tc_port *tc = to_tc_port(dig_port);
> -	enum phy phy = intel_port_to_phy(i915, dig_port->base.port);
>  	intel_wakeref_t wakeref;
> -	u32 lane_mask;
> -
> -	if (!intel_phy_is_tc(i915, phy) || tc->mode != TC_PORT_DP_ALT)
> -		return 4;
> +	u32 lane_mask = 0;
> 
> -	assert_tc_cold_blocked(tc);
> -
> -	if (DISPLAY_VER(i915) >= 14)
> -		return mtl_tc_port_get_max_lane_count(dig_port);
> -
> -	lane_mask = 0;
>  	with_intel_display_power(i915, POWER_DOMAIN_DISPLAY_CORE,
> wakeref)
>  		lane_mask = intel_tc_port_get_lane_mask(dig_port);
> 
> @@ -348,6 +337,23 @@ int intel_tc_port_fia_max_lane_count(struct
> intel_digital_port *dig_port)
>  	}
>  }
> 
> +int intel_tc_port_fia_max_lane_count(struct intel_digital_port
> +*dig_port) {
> +	struct drm_i915_private *i915 = to_i915(dig_port->base.base.dev);
> +	struct intel_tc_port *tc = to_tc_port(dig_port);
> +	enum phy phy = intel_port_to_phy(i915, dig_port->base.port);
> +
> +	if (!intel_phy_is_tc(i915, phy) || tc->mode != TC_PORT_DP_ALT)
> +		return 4;
> +
> +	assert_tc_cold_blocked(tc);
> +
> +	if (DISPLAY_VER(i915) >= 14)
> +		return mtl_tc_port_get_max_lane_count(dig_port);
> +
> +	return intel_tc_port_get_max_lane_count(dig_port);
> +}

Looking at this I think we have more scope of optimization here I think we can go about it in the following way

intel_tc_port_fia_max_lane_count
already calls 
with_intel_display_power(i915, POWER_DOMAIN_DISPLAY_CORE, wakeref)
                lane_mask = intel_tc_port_get_lane_mask(dig_port);

which we also duplicate in  mtl_tc_port_get_pin_assignment_mask
(now mtl_tc_port_get_max_lane_count) and the only difference between them
Is the switch case what if we have a function or repurpose 
mtl_tc_port_get_max_lane_count to only have the switch case block with 
assignment varied by display version and call it after intel_tc_port_get_lane_mask

maybe also move the legacy switch case in its own function?

Regards,
Suraj Kandpal

> +
>  void intel_tc_port_set_fia_lane_count(struct intel_digital_port *dig_port,
>  				      int required_lanes)
>  {
> --
> 2.39.2

Re: [Intel-gfx] [PATCH v3 1/4] drm/i915/tc: rename mtl_tc_port_get_pin_assignment_mask()

[Coelho, Luciano]

On Wed, 2023-08-16 at 08:13 +0000, Kandpal, Suraj wrote:
> > This function doesn't really return the pin assignment mask, but
> > the max lane
> > count derived from that.  So rename the function to
> > mtl_tc_port_get_max_lane_count() to better reflect what it really
> > does.
> > 
> Maybe also add the version changes on commit messages here as cover
> letter ends up getting discarded

Ah, right.  I discussed this with someone else before and we agreed to
disagree. 🙂 I don't really see the point in having the change history
in the commit itself for the mainline.  The discussions should be
openly available in the mailing list archives, so duplicating it in the
commit logs, IMHO, is moot.

A link in the commit log to lore, for instance, would add much more
value IMHO.

But anyway, since this guideline was already in place when I came, I
will (almost grudgingly) comply. 😉

> 
> With that fixed
> 
> Reviewed-by: Suraj Kandpal <suraj.kandpal@intel.com>

Thanks!

--
Cheers,
Luca.

Re: [Intel-gfx] [PATCH v3 0/4] drm/i915/tc: some clean-ups in max lane count handling code

[Lucas De Marchi]

On Fri, Jul 21, 2023 at 02:11:17PM +0300, Luca Coelho wrote:
>Hi,
>
>Here are four patches with some clean-ups in the code that handles the
>max lane count of Type-C connections.
>
>This is done mostly in preparation for a new way to read the pin
>assignments and lane count in future devices.
>
>In v2:
>   * Fix some rebasing damage.
>
>In v3:
>   * Fixed "assigment" typo, as reported by checkpatch.
>
>Please review.

what happened to this series? It seems only the last patch is not
reviewed. Are you going to submit a rebased version?


Lucas De Marchi

Re: [Intel-gfx] [PATCH v3 1/4] drm/i915/tc: rename mtl_tc_port_get_pin_assignment_mask()

[Lucas De Marchi]

On Wed, Aug 16, 2023 at 09:08:44AM +0000, Coelho, Luciano wrote:
>On Wed, 2023-08-16 at 08:13 +0000, Kandpal, Suraj wrote:
>> > This function doesn't really return the pin assignment mask, but
>> > the max lane
>> > count derived from that.  So rename the function to
>> > mtl_tc_port_get_max_lane_count() to better reflect what it really
>> > does.
>> >
>> Maybe also add the version changes on commit messages here as cover
>> letter ends up getting discarded
>
>Ah, right.  I discussed this with someone else before and we agreed to
>disagree. 🙂 I don't really see the point in having the change history
>in the commit itself for the mainline.  The discussions should be
>openly available in the mailing list archives, so duplicating it in the
>commit logs, IMHO, is moot.
>
>A link in the commit log to lore, for instance, would add much more
>value IMHO.
>
>But anyway, since this guideline was already in place when I came, I
>will (almost grudgingly) comply. 😉

some people want them, some people want them removed. A lot of people in
drm like it while people outside will shout loudly if you add that.
Don't let this hold off getting the patch into a mergeable state. 


Reviewed-by: Lucas De Marchi <lucas.demarchi@intel.com>

It may need a rebase though.

Lucas De Marchi

>
>>
>> With that fixed
>>
>> Reviewed-by: Suraj Kandpal <suraj.kandpal@intel.com>
>
>Thanks!
>
>--
>Cheers,
>Luca.

Re: [Intel-gfx] [PATCH v3 4/4] drm/i915/tc: remove "fia" from intel_tc_port_fia_max_lane_count()

[Lucas De Marchi]

On Fri, Jul 21, 2023 at 02:11:21PM +0300, Luca Coelho wrote:
>It is irrelevant for the caller that the max lane count is being
>derived from a FIA register, so having "fia" in the function name is
>irrelevant.  Rename the function accordingly.
>
>Signed-off-by: Luca Coelho <luciano.coelho@intel.com>
>---
> drivers/gpu/drm/i915/display/intel_cx0_phy.c | 2 +-
> drivers/gpu/drm/i915/display/intel_dp.c      | 4 ++--
> drivers/gpu/drm/i915/display/intel_tc.c      | 4 ++--
> drivers/gpu/drm/i915/display/intel_tc.h      | 2 +-
> 4 files changed, 6 insertions(+), 6 deletions(-)
>
>diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy.c b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
>index 1b00ef2c6185..6d4f7b20ce85 100644
>--- a/drivers/gpu/drm/i915/display/intel_cx0_phy.c
>+++ b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
>@@ -2534,7 +2534,7 @@ static void intel_cx0_phy_lane_reset(struct drm_i915_private *i915,
> {
> 	enum port port = encoder->port;
> 	enum phy phy = intel_port_to_phy(i915, port);
>-	bool both_lanes =  intel_tc_port_fia_max_lane_count(enc_to_dig_port(encoder)) > 2;
>+	bool both_lanes =  intel_tc_port_max_lane_count(enc_to_dig_port(encoder)) > 2;
> 	u8 lane_mask = lane_reversal ? INTEL_CX0_LANE1 :
> 				  INTEL_CX0_LANE0;
> 	u32 lane_pipe_reset = both_lanes ?
>diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
>index 03675620e3ea..b974af839acb 100644
>--- a/drivers/gpu/drm/i915/display/intel_dp.c
>+++ b/drivers/gpu/drm/i915/display/intel_dp.c
>@@ -306,13 +306,13 @@ static int intel_dp_max_common_lane_count(struct intel_dp *intel_dp)
> 	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
> 	int source_max = intel_dp_max_source_lane_count(dig_port);
> 	int sink_max = intel_dp->max_sink_lane_count;
>-	int fia_max = intel_tc_port_fia_max_lane_count(dig_port);
>+	int port_max = intel_tc_port_max_lane_count(dig_port);

s/port/lane/?  since this is the lane max that is being returned, not
the port_max... ?

> 	int lttpr_max = drm_dp_lttpr_max_lane_count(intel_dp->lttpr_common_caps);
>
> 	if (lttpr_max)
> 		sink_max = min(sink_max, lttpr_max);
>
>-	return min3(source_max, sink_max, fia_max);
>+	return min3(source_max, sink_max, port_max);
> }
>
> int intel_dp_max_lane_count(struct intel_dp *intel_dp)
>diff --git a/drivers/gpu/drm/i915/display/intel_tc.c b/drivers/gpu/drm/i915/display/intel_tc.c
>index 43b8eeba26f8..3c94bbcb5497 100644
>--- a/drivers/gpu/drm/i915/display/intel_tc.c
>+++ b/drivers/gpu/drm/i915/display/intel_tc.c
>@@ -337,7 +337,7 @@ static int intel_tc_port_get_max_lane_count(struct intel_digital_port *dig_port)
> 	}
> }
>
>-int intel_tc_port_fia_max_lane_count(struct intel_digital_port *dig_port)
>+int intel_tc_port_max_lane_count(struct intel_digital_port *dig_port)
> {
> 	struct drm_i915_private *i915 = to_i915(dig_port->base.base.dev);
> 	struct intel_tc_port *tc = to_tc_port(dig_port);
>@@ -589,7 +589,7 @@ static bool tc_phy_verify_legacy_or_dp_alt_mode(struct intel_tc_port *tc,
> 	struct intel_digital_port *dig_port = tc->dig_port;
> 	int max_lanes;
>
>-	max_lanes = intel_tc_port_fia_max_lane_count(dig_port);
>+	max_lanes = intel_tc_port_max_lane_count(dig_port);

as shown here 

with that var rename:


	Reviewed-by: Lucas De Marchi <lucas.demarchi@intel.com>

thanks
Lucas De Marchi

> 	if (tc->mode == TC_PORT_LEGACY) {
> 		drm_WARN_ON(&i915->drm, max_lanes != 4);
> 		return true;
>diff --git a/drivers/gpu/drm/i915/display/intel_tc.h b/drivers/gpu/drm/i915/display/intel_tc.h
>index ffc0e2a74e43..80a61e52850e 100644
>--- a/drivers/gpu/drm/i915/display/intel_tc.h
>+++ b/drivers/gpu/drm/i915/display/intel_tc.h
>@@ -20,7 +20,7 @@ bool intel_tc_port_connected(struct intel_encoder *encoder);
> bool intel_tc_port_connected_locked(struct intel_encoder *encoder);
>
> u32 intel_tc_port_get_pin_assignment_mask(struct intel_digital_port *dig_port);
>-int intel_tc_port_fia_max_lane_count(struct intel_digital_port *dig_port);
>+int intel_tc_port_max_lane_count(struct intel_digital_port *dig_port);
> void intel_tc_port_set_fia_lane_count(struct intel_digital_port *dig_port,
> 				      int required_lanes);
>
>-- 
>2.39.2
>

Re: [Intel-gfx] [PATCH v3 0/4] drm/i915/tc: some clean-ups in max lane count handling code

[Kandpal, Suraj]

0/4] drm/i915/tc: some clean-ups in max
> lane count handling code
> 
> On Fri, Jul 21, 2023 at 02:11:17PM +0300, Luca Coelho wrote:
> >Hi,
> >
> >Here are four patches with some clean-ups in the code that handles the
> >max lane count of Type-C connections.
> >
> >This is done mostly in preparation for a new way to read the pin
> >assignments and lane count in future devices.
> >
> >In v2:
> >   * Fix some rebasing damage.
> >
> >In v3:
> >   * Fixed "assigment" typo, as reported by checkpatch.
> >
> >Please review.
> 
> what happened to this series? It seems only the last patch is not reviewed.
> Are you going to submit a rebased version?
> 

So I had some review comments on patch 3 was waiting for Luciano to upstream
the latest changes then review the 4th patch

Regards,
Suraj Kandpal

> 
> Lucas De Marchi

Re: [Intel-gfx] [PATCH v3 4/4] drm/i915/tc: remove "fia" from intel_tc_port_fia_max_lane_count()

[Kandpal, Suraj]

> It is irrelevant for the caller that the max lane count is being derived from a FIA
> register, so having "fia" in the function name is irrelevant.  Rename the
> function accordingly.
> 
> Signed-off-by: Luca Coelho <luciano.coelho@intel.com>

LGTM.

Reviewed-by: Suraj Kandpal <suraj.kandpal@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_cx0_phy.c | 2 +-
>  drivers/gpu/drm/i915/display/intel_dp.c      | 4 ++--
>  drivers/gpu/drm/i915/display/intel_tc.c      | 4 ++--
>  drivers/gpu/drm/i915/display/intel_tc.h      | 2 +-
>  4 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> index 1b00ef2c6185..6d4f7b20ce85 100644
> --- a/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> +++ b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> @@ -2534,7 +2534,7 @@ static void intel_cx0_phy_lane_reset(struct
> drm_i915_private *i915,  {
>  	enum port port = encoder->port;
>  	enum phy phy = intel_port_to_phy(i915, port);
> -	bool both_lanes =
> intel_tc_port_fia_max_lane_count(enc_to_dig_port(encoder)) > 2;
> +	bool both_lanes =
> +intel_tc_port_max_lane_count(enc_to_dig_port(encoder)) > 2;
>  	u8 lane_mask = lane_reversal ? INTEL_CX0_LANE1 :
>  				  INTEL_CX0_LANE0;
>  	u32 lane_pipe_reset = both_lanes ?
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
> b/drivers/gpu/drm/i915/display/intel_dp.c
> index 03675620e3ea..b974af839acb 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -306,13 +306,13 @@ static int
> intel_dp_max_common_lane_count(struct intel_dp *intel_dp)
>  	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
>  	int source_max = intel_dp_max_source_lane_count(dig_port);
>  	int sink_max = intel_dp->max_sink_lane_count;
> -	int fia_max = intel_tc_port_fia_max_lane_count(dig_port);
> +	int port_max = intel_tc_port_max_lane_count(dig_port);
>  	int lttpr_max = drm_dp_lttpr_max_lane_count(intel_dp-
> >lttpr_common_caps);
> 
>  	if (lttpr_max)
>  		sink_max = min(sink_max, lttpr_max);
> 
> -	return min3(source_max, sink_max, fia_max);
> +	return min3(source_max, sink_max, port_max);
>  }
> 
>  int intel_dp_max_lane_count(struct intel_dp *intel_dp) diff --git
> a/drivers/gpu/drm/i915/display/intel_tc.c
> b/drivers/gpu/drm/i915/display/intel_tc.c
> index 43b8eeba26f8..3c94bbcb5497 100644
> --- a/drivers/gpu/drm/i915/display/intel_tc.c
> +++ b/drivers/gpu/drm/i915/display/intel_tc.c
> @@ -337,7 +337,7 @@ static int intel_tc_port_get_max_lane_count(struct
> intel_digital_port *dig_port)
>  	}
>  }
> 
> -int intel_tc_port_fia_max_lane_count(struct intel_digital_port *dig_port)
> +int intel_tc_port_max_lane_count(struct intel_digital_port *dig_port)
>  {
>  	struct drm_i915_private *i915 = to_i915(dig_port->base.base.dev);
>  	struct intel_tc_port *tc = to_tc_port(dig_port); @@ -589,7 +589,7
> @@ static bool tc_phy_verify_legacy_or_dp_alt_mode(struct intel_tc_port
> *tc,
>  	struct intel_digital_port *dig_port = tc->dig_port;
>  	int max_lanes;
> 
> -	max_lanes = intel_tc_port_fia_max_lane_count(dig_port);
> +	max_lanes = intel_tc_port_max_lane_count(dig_port);
>  	if (tc->mode == TC_PORT_LEGACY) {
>  		drm_WARN_ON(&i915->drm, max_lanes != 4);
>  		return true;
> diff --git a/drivers/gpu/drm/i915/display/intel_tc.h
> b/drivers/gpu/drm/i915/display/intel_tc.h
> index ffc0e2a74e43..80a61e52850e 100644
> --- a/drivers/gpu/drm/i915/display/intel_tc.h
> +++ b/drivers/gpu/drm/i915/display/intel_tc.h
> @@ -20,7 +20,7 @@ bool intel_tc_port_connected(struct intel_encoder
> *encoder);  bool intel_tc_port_connected_locked(struct intel_encoder
> *encoder);
> 
>  u32 intel_tc_port_get_pin_assignment_mask(struct intel_digital_port
> *dig_port); -int intel_tc_port_fia_max_lane_count(struct intel_digital_port
> *dig_port);
> +int intel_tc_port_max_lane_count(struct intel_digital_port *dig_port);
>  void intel_tc_port_set_fia_lane_count(struct intel_digital_port *dig_port,
>  				      int required_lanes);
> 
> --
> 2.39.2

Re: [Intel-gfx] [PATCH v3 1/4] drm/i915/tc: rename mtl_tc_port_get_pin_assignment_mask()

[Coelho, Luciano]

On Fri, 2023-08-18 at 20:50 -0700, Lucas De Marchi wrote:
> On Wed, Aug 16, 2023 at 09:08:44AM +0000, Coelho, Luciano wrote:
> > On Wed, 2023-08-16 at 08:13 +0000, Kandpal, Suraj wrote:
> > > > This function doesn't really return the pin assignment mask, but
> > > > the max lane
> > > > count derived from that.  So rename the function to
> > > > mtl_tc_port_get_max_lane_count() to better reflect what it really
> > > > does.
> > > > 
> > > Maybe also add the version changes on commit messages here as cover
> > > letter ends up getting discarded
> > 
> > Ah, right.  I discussed this with someone else before and we agreed to
> > disagree. 🙂 I don't really see the point in having the change history
> > in the commit itself for the mainline.  The discussions should be
> > openly available in the mailing list archives, so duplicating it in the
> > commit logs, IMHO, is moot.
> > 
> > A link in the commit log to lore, for instance, would add much more
> > value IMHO.
> > 
> > But anyway, since this guideline was already in place when I came, I
> > will (almost grudgingly) comply. 😉
> 
> some people want them, some people want them removed. A lot of people in
> drm like it while people outside will shout loudly if you add that.
> Don't let this hold off getting the patch into a mergeable state. 
> 
> 
> Reviewed-by: Lucas De Marchi <lucas.demarchi@intel.com>
> 
> It may need a rebase though.

Thanks, Lucas.  I'll keep the version history out then, if that's a
possibility, I prefer it. 🙂

[...]

> > > 
> > > With that fixed
> > > 
> > > Reviewed-by: Suraj Kandpal <suraj.kandpal@intel.com>

Suraj, let me know if you want me to add your r-b tag even though I'm
not going to add the history.

--
Cheers,
Luca.

Re: [Intel-gfx] [PATCH v3 0/4] drm/i915/tc: some clean-ups in max lane count handling code

[Coelho, Luciano]

On Mon, 2023-08-21 at 17:27 +0000, Kandpal, Suraj wrote:
> 0/4] drm/i915/tc: some clean-ups in max
> > lane count handling code
> > 
> > On Fri, Jul 21, 2023 at 02:11:17PM +0300, Luca Coelho wrote:
> > > Hi,
> > > 
> > > Here are four patches with some clean-ups in the code that handles the
> > > max lane count of Type-C connections.
> > > 
> > > This is done mostly in preparation for a new way to read the pin
> > > assignments and lane count in future devices.
> > > 
> > > In v2:
> > >   * Fix some rebasing damage.
> > > 
> > > In v3:
> > >   * Fixed "assigment" typo, as reported by checkpatch.
> > > 
> > > Please review.
> > 
> > what happened to this series? It seems only the last patch is not reviewed.
> > Are you going to submit a rebased version?
> > 
> 
> So I had some review comments on patch 3 was waiting for Luciano to upstream
> the latest changes then review the 4th patch

Sorry for the delay, I've been focusing on a high-priority bug.

I'll send a new version out soon.

--
Cheers,
Luca.

Re: [Intel-gfx] [PATCH v3 3/4] drm/i915/tc: move legacy code out of the main _max_lane_count() func

[Coelho, Luciano]

On Wed, 2023-08-16 at 08:54 +0000, Kandpal, Suraj wrote:
> > This makes the code a bit more symmetric and readable, especially
> > when we
> > start adding more display version-specific alternatives.
> > 
> > Signed-off-by: Luca Coelho <luciano.coelho@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_tc.c | 32 +++++++++++++++------
> > ----
> >  1 file changed, 19 insertions(+), 13 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_tc.c
> > b/drivers/gpu/drm/i915/display/intel_tc.c
> > index de848b329f4b..43b8eeba26f8 100644
> > --- a/drivers/gpu/drm/i915/display/intel_tc.c
> > +++ b/drivers/gpu/drm/i915/display/intel_tc.c
> > @@ -311,23 +311,12 @@ static int
> > mtl_tc_port_get_max_lane_count(struct
> > intel_digital_port *dig_port)
> >  	}
> >  }
> > 
> > -int intel_tc_port_fia_max_lane_count(struct intel_digital_port
> > *dig_port)
> > +static int intel_tc_port_get_max_lane_count(struct
> > intel_digital_port
> > +*dig_port)
> >  {
> >  	struct drm_i915_private *i915 = to_i915(dig_port-
> > >base.base.dev);
> > -	struct intel_tc_port *tc = to_tc_port(dig_port);
> > -	enum phy phy = intel_port_to_phy(i915, dig_port-
> > >base.port);
> >  	intel_wakeref_t wakeref;
> > -	u32 lane_mask;
> > -
> > -	if (!intel_phy_is_tc(i915, phy) || tc->mode !=
> > TC_PORT_DP_ALT)
> > -		return 4;
> > +	u32 lane_mask = 0;
> > 
> > -	assert_tc_cold_blocked(tc);
> > -
> > -	if (DISPLAY_VER(i915) >= 14)
> > -		return mtl_tc_port_get_max_lane_count(dig_port);
> > -
> > -	lane_mask = 0;
> >  	with_intel_display_power(i915, POWER_DOMAIN_DISPLAY_CORE,
> > wakeref)
> >  		lane_mask = intel_tc_port_get_lane_mask(dig_port);
> > 
> > @@ -348,6 +337,23 @@ int intel_tc_port_fia_max_lane_count(struct
> > intel_digital_port *dig_port)
> >  	}
> >  }
> > 
> > +int intel_tc_port_fia_max_lane_count(struct intel_digital_port
> > +*dig_port) {
> > +	struct drm_i915_private *i915 = to_i915(dig_port-
> > >base.base.dev);
> > +	struct intel_tc_port *tc = to_tc_port(dig_port);
> > +	enum phy phy = intel_port_to_phy(i915, dig_port-
> > >base.port);
> > +
> > +	if (!intel_phy_is_tc(i915, phy) || tc->mode !=
> > TC_PORT_DP_ALT)
> > +		return 4;
> > +
> > +	assert_tc_cold_blocked(tc);
> > +
> > +	if (DISPLAY_VER(i915) >= 14)
> > +		return mtl_tc_port_get_max_lane_count(dig_port);
> > +
> > +	return intel_tc_port_get_max_lane_count(dig_port);
> > +}
> 
> Looking at this I think we have more scope of optimization here I
> think we can go about it in the following way
> 
> intel_tc_port_fia_max_lane_count
> already calls 
> with_intel_display_power(i915, POWER_DOMAIN_DISPLAY_CORE, wakeref)
>                 lane_mask = intel_tc_port_get_lane_mask(dig_port);
> 
> which we also duplicate in  mtl_tc_port_get_pin_assignment_mask
> (now mtl_tc_port_get_max_lane_count) and the only difference between
> them
> Is the switch case what if we have a function or repurpose 
> mtl_tc_port_get_max_lane_count to only have the switch case block
> with 
> assignment varied by display version and call it after
> intel_tc_port_get_lane_mask
> 
> maybe also move the legacy switch case in its own function?

This would be another option, but I think it's nicer to keep things
very isolated from each other and this tiny code duplication is not too
bad.

Especially because later, as you can see in my LNL patch that Lucas
sent out[1] we have another combination in a new function.  So we have:

intel_tc_port_get_max_lane_count();
	intel_tc_port_get_lane_mask();
	switch with lane_mask;

mlt_tc_port_get_lane_mask(dig_port);
	intel_tc_port_get_pin_assignment_mask();
	switch with pin_mask;

lnl_tc_port_get_lane_mask():
	intel_uncore_read(uncore, TCSS_DDI_STATUS(tc_port));
	switch with pin_assignment;


So now we have 3 different ways to read and two different ways to parse
(with the switch-case).  I think it's a lot cleaner to keep this all
separate than to try to reuse these small pieces of code, which would
make it a bit harder to read.

Makes sense?

[1] https://patchwork.kernel.org/project/intel-gfx/patch/20230823170740.1180212-28-lucas.demarchi@intel.com/

--
Cheers,
Luca.

Re: [Intel-gfx] [PATCH v3 3/4] drm/i915/tc: move legacy code out of the main _max_lane_count() func

[Kandpal, Suraj]

> On Wed, 2023-08-16 at 08:54 +0000, Kandpal, Suraj wrote:
> > > This makes the code a bit more symmetric and readable, especially
> > > when we start adding more display version-specific alternatives.
> > >
> > > Signed-off-by: Luca Coelho <luciano.coelho@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_tc.c | 32 +++++++++++++++------
> > > ----
> > >  1 file changed, 19 insertions(+), 13 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/display/intel_tc.c
> > > b/drivers/gpu/drm/i915/display/intel_tc.c
> > > index de848b329f4b..43b8eeba26f8 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_tc.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_tc.c
> > > @@ -311,23 +311,12 @@ static int
> > > mtl_tc_port_get_max_lane_count(struct
> > > intel_digital_port *dig_port)
> > >  	}
> > >  }
> > >
> > > -int intel_tc_port_fia_max_lane_count(struct intel_digital_port
> > > *dig_port)
> > > +static int intel_tc_port_get_max_lane_count(struct
> > > intel_digital_port
> > > +*dig_port)
> > >  {
> > >  	struct drm_i915_private *i915 = to_i915(dig_port-
> > > >base.base.dev);
> > > -	struct intel_tc_port *tc = to_tc_port(dig_port);
> > > -	enum phy phy = intel_port_to_phy(i915, dig_port-
> > > >base.port);
> > >  	intel_wakeref_t wakeref;
> > > -	u32 lane_mask;
> > > -
> > > -	if (!intel_phy_is_tc(i915, phy) || tc->mode !=
> > > TC_PORT_DP_ALT)
> > > -		return 4;
> > > +	u32 lane_mask = 0;
> > >
> > > -	assert_tc_cold_blocked(tc);
> > > -
> > > -	if (DISPLAY_VER(i915) >= 14)
> > > -		return mtl_tc_port_get_max_lane_count(dig_port);
> > > -
> > > -	lane_mask = 0;
> > >  	with_intel_display_power(i915, POWER_DOMAIN_DISPLAY_CORE,
> > > wakeref)
> > >  		lane_mask = intel_tc_port_get_lane_mask(dig_port);
> > >
> > > @@ -348,6 +337,23 @@ int intel_tc_port_fia_max_lane_count(struct
> > > intel_digital_port *dig_port)
> > >  	}
> > >  }
> > >
> > > +int intel_tc_port_fia_max_lane_count(struct intel_digital_port
> > > +*dig_port) {
> > > +	struct drm_i915_private *i915 = to_i915(dig_port-
> > > >base.base.dev);
> > > +	struct intel_tc_port *tc = to_tc_port(dig_port);
> > > +	enum phy phy = intel_port_to_phy(i915, dig_port-
> > > >base.port);
> > > +
> > > +	if (!intel_phy_is_tc(i915, phy) || tc->mode !=
> > > TC_PORT_DP_ALT)
> > > +		return 4;
> > > +
> > > +	assert_tc_cold_blocked(tc);
> > > +
> > > +	if (DISPLAY_VER(i915) >= 14)
> > > +		return mtl_tc_port_get_max_lane_count(dig_port);
> > > +
> > > +	return intel_tc_port_get_max_lane_count(dig_port);
> > > +}
> >
> > Looking at this I think we have more scope of optimization here I
> > think we can go about it in the following way
> >
> > intel_tc_port_fia_max_lane_count
> > already calls
> > with_intel_display_power(i915, POWER_DOMAIN_DISPLAY_CORE, wakeref)
> >                 lane_mask = intel_tc_port_get_lane_mask(dig_port);
> >
> > which we also duplicate in  mtl_tc_port_get_pin_assignment_mask
> > (now mtl_tc_port_get_max_lane_count) and the only difference between
> > them Is the switch case what if we have a function or repurpose
> > mtl_tc_port_get_max_lane_count to only have the switch case block with
> > assignment varied by display version and call it after
> > intel_tc_port_get_lane_mask
> >
> > maybe also move the legacy switch case in its own function?
> 
> This would be another option, but I think it's nicer to keep things very isolated
> from each other and this tiny code duplication is not too bad.
> 
> Especially because later, as you can see in my LNL patch that Lucas sent out[1]
> we have another combination in a new function.  So we have:
> 
> intel_tc_port_get_max_lane_count();
> 	intel_tc_port_get_lane_mask();
> 	switch with lane_mask;
> 
> mlt_tc_port_get_lane_mask(dig_port);
> 	intel_tc_port_get_pin_assignment_mask();
> 	switch with pin_mask;
> 
> lnl_tc_port_get_lane_mask():
> 	intel_uncore_read(uncore, TCSS_DDI_STATUS(tc_port));
> 	switch with pin_assignment;
> 
> 
> So now we have 3 different ways to read and two different ways to parse (with
> the switch-case).  I think it's a lot cleaner to keep this all separate than to try
> to reuse these small pieces of code, which would make it a bit harder to read.
> 
> Makes sense?
> 

Yes this actually makes sense I commented the same thing on Lucas's LNL
Display enablement patch

Regards,
Suraj Kandpal

> [1] https://patchwork.kernel.org/project/intel-
> gfx/patch/20230823170740.1180212-28-lucas.demarchi@intel.com/
> 
> --
> Cheers,
> Luca.

Re: [Intel-gfx] [PATCH v3 3/4] drm/i915/tc: move legacy code out of the main _max_lane_count() func

[Kandpal, Suraj]

> On Wed, 2023-08-16 at 08:54 +0000, Kandpal, Suraj wrote:
> > > This makes the code a bit more symmetric and readable, especially
> > > when we start adding more display version-specific alternatives.
> > >
> > > Signed-off-by: Luca Coelho <luciano.coelho@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_tc.c | 32 +++++++++++++++------
> > > ----
> > >  1 file changed, 19 insertions(+), 13 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/display/intel_tc.c
> > > b/drivers/gpu/drm/i915/display/intel_tc.c
> > > index de848b329f4b..43b8eeba26f8 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_tc.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_tc.c
> > > @@ -311,23 +311,12 @@ static int
> > > mtl_tc_port_get_max_lane_count(struct
> > > intel_digital_port *dig_port)
> > >  	}
> > >  }
> > >
> > > -int intel_tc_port_fia_max_lane_count(struct intel_digital_port
> > > *dig_port)
> > > +static int intel_tc_port_get_max_lane_count(struct
> > > intel_digital_port
> > > +*dig_port)
> > >  {
> > >  	struct drm_i915_private *i915 = to_i915(dig_port-
> > > >base.base.dev);
> > > -	struct intel_tc_port *tc = to_tc_port(dig_port);
> > > -	enum phy phy = intel_port_to_phy(i915, dig_port-
> > > >base.port);
> > >  	intel_wakeref_t wakeref;
> > > -	u32 lane_mask;
> > > -
> > > -	if (!intel_phy_is_tc(i915, phy) || tc->mode !=
> > > TC_PORT_DP_ALT)
> > > -		return 4;
> > > +	u32 lane_mask = 0;
> > >
> > > -	assert_tc_cold_blocked(tc);
> > > -
> > > -	if (DISPLAY_VER(i915) >= 14)
> > > -		return mtl_tc_port_get_max_lane_count(dig_port);
> > > -
> > > -	lane_mask = 0;
> > >  	with_intel_display_power(i915, POWER_DOMAIN_DISPLAY_CORE,
> > > wakeref)
> > >  		lane_mask = intel_tc_port_get_lane_mask(dig_port);
> > >
> > > @@ -348,6 +337,23 @@ int intel_tc_port_fia_max_lane_count(struct
> > > intel_digital_port *dig_port)
> > >  	}
> > >  }
> > >
> > > +int intel_tc_port_fia_max_lane_count(struct intel_digital_port
> > > +*dig_port) {
> > > +	struct drm_i915_private *i915 = to_i915(dig_port-
> > > >base.base.dev);
> > > +	struct intel_tc_port *tc = to_tc_port(dig_port);
> > > +	enum phy phy = intel_port_to_phy(i915, dig_port-
> > > >base.port);
> > > +
> > > +	if (!intel_phy_is_tc(i915, phy) || tc->mode !=
> > > TC_PORT_DP_ALT)
> > > +		return 4;
> > > +
> > > +	assert_tc_cold_blocked(tc);
> > > +
> > > +	if (DISPLAY_VER(i915) >= 14)
> > > +		return mtl_tc_port_get_max_lane_count(dig_port);
> > > +
> > > +	return intel_tc_port_get_max_lane_count(dig_port);
> > > +}
> >
> > Looking at this I think we have more scope of optimization here I
> > think we can go about it in the following way
> >
> > intel_tc_port_fia_max_lane_count
> > already calls
> > with_intel_display_power(i915, POWER_DOMAIN_DISPLAY_CORE, wakeref)
> >                 lane_mask = intel_tc_port_get_lane_mask(dig_port);
> >
> > which we also duplicate in  mtl_tc_port_get_pin_assignment_mask
> > (now mtl_tc_port_get_max_lane_count) and the only difference between
> > them Is the switch case what if we have a function or repurpose
> > mtl_tc_port_get_max_lane_count to only have the switch case block with
> > assignment varied by display version and call it after
> > intel_tc_port_get_lane_mask
> >
> > maybe also move the legacy switch case in its own function?
> 
> This would be another option, but I think it's nicer to keep things very isolated
> from each other and this tiny code duplication is not too bad.
> 
> Especially because later, as you can see in my LNL patch that Lucas sent out[1]
> we have another combination in a new function.  So we have:
> 
> intel_tc_port_get_max_lane_count();
> 	intel_tc_port_get_lane_mask();
> 	switch with lane_mask;
> 
> mlt_tc_port_get_lane_mask(dig_port);
> 	intel_tc_port_get_pin_assignment_mask();
> 	switch with pin_mask;
> 
> lnl_tc_port_get_lane_mask():
> 	intel_uncore_read(uncore, TCSS_DDI_STATUS(tc_port));
> 	switch with pin_assignment;
> 
> 
> So now we have 3 different ways to read and two different ways to parse (with
> the switch-case).  I think it's a lot cleaner to keep this all separate than to try
> to reuse these small pieces of code, which would make it a bit harder to read.
> 
> Makes sense?

Good with it

Reviewed-by: Suraj Kandpal <suraj.kandpal@intel.com>
> 
> [1] https://patchwork.kernel.org/project/intel-
> gfx/patch/20230823170740.1180212-28-lucas.demarchi@intel.com/
> 
> --
> Cheers,
> Luca.

Re: [Intel-gfx] [PATCH v3 3/4] drm/i915/tc: move legacy code out of the main _max_lane_count() func

[Coelho, Luciano]

On Thu, 2023-08-24 at 11:12 +0000, Kandpal, Suraj wrote:
> > On Wed, 2023-08-16 at 08:54 +0000, Kandpal, Suraj wrote:
> > > > This makes the code a bit more symmetric and readable,
> > > > especially
> > > > when we start adding more display version-specific
> > > > alternatives.
> > > > 
> > > > Signed-off-by: Luca Coelho <luciano.coelho@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/display/intel_tc.c | 32 +++++++++++++++--
> > > > ----
> > > > ----
> > > >  1 file changed, 19 insertions(+), 13 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_tc.c
> > > > b/drivers/gpu/drm/i915/display/intel_tc.c
> > > > index de848b329f4b..43b8eeba26f8 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_tc.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_tc.c
> > > > @@ -311,23 +311,12 @@ static int
> > > > mtl_tc_port_get_max_lane_count(struct
> > > > intel_digital_port *dig_port)
> > > >  	}
> > > >  }
> > > > 
> > > > -int intel_tc_port_fia_max_lane_count(struct intel_digital_port
> > > > *dig_port)
> > > > +static int intel_tc_port_get_max_lane_count(struct
> > > > intel_digital_port
> > > > +*dig_port)
> > > >  {
> > > >  	struct drm_i915_private *i915 = to_i915(dig_port-
> > > > > base.base.dev);
> > > > -	struct intel_tc_port *tc = to_tc_port(dig_port);
> > > > -	enum phy phy = intel_port_to_phy(i915, dig_port-
> > > > > base.port);
> > > >  	intel_wakeref_t wakeref;
> > > > -	u32 lane_mask;
> > > > -
> > > > -	if (!intel_phy_is_tc(i915, phy) || tc->mode !=
> > > > TC_PORT_DP_ALT)
> > > > -		return 4;
> > > > +	u32 lane_mask = 0;
> > > > 
> > > > -	assert_tc_cold_blocked(tc);
> > > > -
> > > > -	if (DISPLAY_VER(i915) >= 14)
> > > > -		return
> > > > mtl_tc_port_get_max_lane_count(dig_port);
> > > > -
> > > > -	lane_mask = 0;
> > > >  	with_intel_display_power(i915,
> > > > POWER_DOMAIN_DISPLAY_CORE,
> > > > wakeref)
> > > >  		lane_mask =
> > > > intel_tc_port_get_lane_mask(dig_port);
> > > > 
> > > > @@ -348,6 +337,23 @@ int
> > > > intel_tc_port_fia_max_lane_count(struct
> > > > intel_digital_port *dig_port)
> > > >  	}
> > > >  }
> > > > 
> > > > +int intel_tc_port_fia_max_lane_count(struct intel_digital_port
> > > > +*dig_port) {
> > > > +	struct drm_i915_private *i915 = to_i915(dig_port-
> > > > > base.base.dev);
> > > > +	struct intel_tc_port *tc = to_tc_port(dig_port);
> > > > +	enum phy phy = intel_port_to_phy(i915, dig_port-
> > > > > base.port);
> > > > +
> > > > +	if (!intel_phy_is_tc(i915, phy) || tc->mode !=
> > > > TC_PORT_DP_ALT)
> > > > +		return 4;
> > > > +
> > > > +	assert_tc_cold_blocked(tc);
> > > > +
> > > > +	if (DISPLAY_VER(i915) >= 14)
> > > > +		return
> > > > mtl_tc_port_get_max_lane_count(dig_port);
> > > > +
> > > > +	return intel_tc_port_get_max_lane_count(dig_port);
> > > > +}
> > > 
> > > Looking at this I think we have more scope of optimization here I
> > > think we can go about it in the following way
> > > 
> > > intel_tc_port_fia_max_lane_count
> > > already calls
> > > with_intel_display_power(i915, POWER_DOMAIN_DISPLAY_CORE,
> > > wakeref)
> > >                 lane_mask =
> > > intel_tc_port_get_lane_mask(dig_port);
> > > 
> > > which we also duplicate in  mtl_tc_port_get_pin_assignment_mask
> > > (now mtl_tc_port_get_max_lane_count) and the only difference
> > > between
> > > them Is the switch case what if we have a function or repurpose
> > > mtl_tc_port_get_max_lane_count to only have the switch case block
> > > with
> > > assignment varied by display version and call it after
> > > intel_tc_port_get_lane_mask
> > > 
> > > maybe also move the legacy switch case in its own function?
> > 
> > This would be another option, but I think it's nicer to keep things
> > very isolated
> > from each other and this tiny code duplication is not too bad.
> > 
> > Especially because later, as you can see in my LNL patch that Lucas
> > sent out[1]
> > we have another combination in a new function.  So we have:
> > 
> > intel_tc_port_get_max_lane_count();
> > 	intel_tc_port_get_lane_mask();
> > 	switch with lane_mask;
> > 
> > mlt_tc_port_get_lane_mask(dig_port);
> > 	intel_tc_port_get_pin_assignment_mask();
> > 	switch with pin_mask;
> > 
> > lnl_tc_port_get_lane_mask():
> > 	intel_uncore_read(uncore, TCSS_DDI_STATUS(tc_port));
> > 	switch with pin_assignment;
> > 
> > 
> > So now we have 3 different ways to read and two different ways to
> > parse (with
> > the switch-case).  I think it's a lot cleaner to keep this all
> > separate than to try
> > to reuse these small pieces of code, which would make it a bit
> > harder to read.
> > 
> > Makes sense?
> 
> Good with it
> 
> Reviewed-by: Suraj Kandpal <suraj.kandpal@intel.com>

Thanks, Suraj!

--
Cheers,
Luca.