All of lore.kernel.org
 help / color / mirror / Atom feed
* [Intel-gfx] [PATCH] drm/i915/display/: Refactor hsw_crtc_enable for bigjoiner cleanup
@ 2022-03-15 23:38 Manasi Navare
  2022-03-16  0:41 ` [Intel-gfx] ✓ Fi.CI.BAT: success for " Patchwork
                   ` (3 more replies)
  0 siblings, 4 replies; 14+ messages in thread
From: Manasi Navare @ 2022-03-15 23:38 UTC (permalink / raw)
  To: intel-gfx

This patch abstracts pieces of hsw_crtc_enable corresponding to different
Bspec enable sequence steps into separate functions.
This helps to call them in a specific order for bigjoiner master/slave
in a cleaner fashion.

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Animesh Manna <animesh.manna@intel.com>
Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
---
 drivers/gpu/drm/i915/display/intel_display.c | 125 ++++++++++---------
 1 file changed, 66 insertions(+), 59 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index eb49973621f0..d8e6466c9fa0 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -1865,24 +1865,6 @@ static void hsw_set_frame_start_delay(const struct intel_crtc_state *crtc_state)
 	intel_de_write(dev_priv, reg, val);
 }
 
-static void icl_ddi_bigjoiner_pre_enable(struct intel_atomic_state *state,
-					 const struct intel_crtc_state *crtc_state)
-{
-	struct intel_crtc *master_crtc = intel_master_crtc(crtc_state);
-
-	/*
-	 * Enable sequence steps 1-7 on bigjoiner master
-	 */
-	if (intel_crtc_is_bigjoiner_slave(crtc_state))
-		intel_encoders_pre_pll_enable(state, master_crtc);
-
-	if (crtc_state->shared_dpll)
-		intel_enable_shared_dpll(crtc_state);
-
-	if (intel_crtc_is_bigjoiner_slave(crtc_state))
-		intel_encoders_pre_enable(state, master_crtc);
-}
-
 static void hsw_configure_cpu_transcoder(const struct intel_crtc_state *crtc_state)
 {
 	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
@@ -1910,70 +1892,73 @@ static void hsw_configure_cpu_transcoder(const struct intel_crtc_state *crtc_sta
 	hsw_set_transconf(crtc_state);
 }
 
-static void hsw_crtc_enable(struct intel_atomic_state *state,
-			    struct intel_crtc *crtc)
+static void hsw_crtc_pre_pll_enable(struct intel_atomic_state *state,
+				    const struct intel_crtc_state *crtc_state)
 {
-	const struct intel_crtc_state *new_crtc_state =
-		intel_atomic_get_new_crtc_state(state, crtc);
-	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
-	enum pipe pipe = crtc->pipe, hsw_workaround_pipe;
-	enum transcoder cpu_transcoder = new_crtc_state->cpu_transcoder;
-	bool psl_clkgate_wa;
-
-	if (drm_WARN_ON(&dev_priv->drm, crtc->active))
-		return;
+	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
 
-	if (!new_crtc_state->bigjoiner_pipes) {
-		intel_encoders_pre_pll_enable(state, crtc);
+	/*
+	 * Enable sequence steps 1 - 7 on all pipes
+	 */
+	intel_encoders_pre_pll_enable(state, crtc);
+	if (crtc_state->shared_dpll)
+		intel_enable_shared_dpll(crtc_state);
 
-		if (new_crtc_state->shared_dpll)
-			intel_enable_shared_dpll(new_crtc_state);
+	intel_encoders_pre_enable(state, crtc);
+}
 
-		intel_encoders_pre_enable(state, crtc);
-	} else {
-		icl_ddi_bigjoiner_pre_enable(state, new_crtc_state);
-	}
+static void hsw_crtc_post_pll_enable(struct intel_atomic_state *state,
+				     const struct intel_crtc_state *crtc_state)
+{
+	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
+	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
+	enum pipe pipe = crtc->pipe, hsw_workaround_pipe;
+	enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
+	bool psl_clkgate_wa;
 
-	intel_dsc_enable(new_crtc_state);
+	/*
+	 * Enable sequence step 8
+	 */
+	intel_dsc_enable(crtc_state);
 
 	if (DISPLAY_VER(dev_priv) >= 13)
-		intel_uncompressed_joiner_enable(new_crtc_state);
+		intel_uncompressed_joiner_enable(crtc_state);
 
-	intel_set_pipe_src_size(new_crtc_state);
+	intel_set_pipe_src_size(crtc_state);
 	if (DISPLAY_VER(dev_priv) >= 9 || IS_BROADWELL(dev_priv))
-		bdw_set_pipemisc(new_crtc_state);
+		bdw_set_pipemisc(crtc_state);
 
-	if (!intel_crtc_is_bigjoiner_slave(new_crtc_state) &&
+	if (!intel_crtc_is_bigjoiner_slave(crtc_state) &&
 	    !transcoder_is_dsi(cpu_transcoder))
-		hsw_configure_cpu_transcoder(new_crtc_state);
+		hsw_configure_cpu_transcoder(crtc_state);
 
 	crtc->active = true;
 
 	/* Display WA #1180: WaDisableScalarClockGating: glk */
 	psl_clkgate_wa = DISPLAY_VER(dev_priv) == 10 &&
-		new_crtc_state->pch_pfit.enabled;
+		crtc_state->pch_pfit.enabled;
 	if (psl_clkgate_wa)
 		glk_pipe_scaler_clock_gating_wa(dev_priv, pipe, true);
 
 	if (DISPLAY_VER(dev_priv) >= 9)
-		skl_pfit_enable(new_crtc_state);
+		skl_pfit_enable(crtc_state);
 	else
-		ilk_pfit_enable(new_crtc_state);
+		ilk_pfit_enable(crtc_state);
 
 	/*
 	 * On ILK+ LUT must be loaded before the pipe is running but with
 	 * clocks enabled
 	 */
-	intel_color_load_luts(new_crtc_state);
-	intel_color_commit(new_crtc_state);
+	intel_color_load_luts(crtc_state);
+	intel_color_commit(crtc_state);
 	/* update DSPCNTR to configure gamma/csc for pipe bottom color */
 	if (DISPLAY_VER(dev_priv) < 9)
-		intel_disable_primary_plane(new_crtc_state);
+		intel_disable_primary_plane(crtc_state);
 
-	hsw_set_linetime_wm(new_crtc_state);
+	hsw_set_linetime_wm(crtc_state);
 
 	if (DISPLAY_VER(dev_priv) >= 11)
-		icl_set_pipe_chicken(new_crtc_state);
+		icl_set_pipe_chicken(crtc_state);
 
 	intel_initial_watermarks(state, crtc);
 
@@ -1984,8 +1969,8 @@ static void hsw_crtc_enable(struct intel_atomic_state *state,
 		icl_pipe_mbus_enable(crtc, dbuf_state->joined_mbus);
 	}
 
-	if (intel_crtc_is_bigjoiner_slave(new_crtc_state))
-		intel_crtc_vblank_on(new_crtc_state);
+	if (intel_crtc_is_bigjoiner_slave(crtc_state))
+		intel_crtc_vblank_on(crtc_state);
 
 	intel_encoders_enable(state, crtc);
 
@@ -1996,7 +1981,7 @@ static void hsw_crtc_enable(struct intel_atomic_state *state,
 
 	/* If we change the relative order between pipe/planes enabling, we need
 	 * to change the workaround. */
-	hsw_workaround_pipe = new_crtc_state->hsw_workaround_pipe;
+	hsw_workaround_pipe = crtc_state->hsw_workaround_pipe;
 	if (IS_HASWELL(dev_priv) && hsw_workaround_pipe != INVALID_PIPE) {
 		struct intel_crtc *wa_crtc;
 
@@ -2007,6 +1992,29 @@ static void hsw_crtc_enable(struct intel_atomic_state *state,
 	}
 }
 
+static void hsw_crtc_enable(struct intel_atomic_state *state,
+			    struct intel_crtc *crtc)
+{
+	const struct intel_crtc_state *new_crtc_state =
+		intel_atomic_get_new_crtc_state(state, crtc);
+	struct intel_crtc *slave_crtc;
+	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
+
+	if (drm_WARN_ON(&dev_priv->drm, crtc->active))
+		return;
+
+	hsw_crtc_pre_pll_enable(state, new_crtc_state);
+
+	for_each_intel_crtc_in_pipe_mask(&dev_priv->drm, slave_crtc,
+					 intel_crtc_bigjoiner_slave_pipes(new_crtc_state)) {
+		struct intel_crtc_state *slave_crtc_state =
+			intel_atomic_get_new_crtc_state(state, slave_crtc);
+
+		hsw_crtc_post_pll_enable(state, slave_crtc_state);
+	}
+	hsw_crtc_post_pll_enable(state, new_crtc_state);
+}
+
 void ilk_pfit_disable(const struct intel_crtc_state *old_crtc_state)
 {
 	struct intel_crtc *crtc = to_intel_crtc(old_crtc_state->uapi.crtc);
@@ -8122,11 +8130,11 @@ static void intel_enable_crtc(struct intel_atomic_state *state,
 
 	intel_crtc_update_active_timings(new_crtc_state);
 
-	dev_priv->display->crtc_enable(state, crtc);
-
 	if (intel_crtc_is_bigjoiner_slave(new_crtc_state))
 		return;
 
+	dev_priv->display->crtc_enable(state, crtc);
+
 	intel_drrs_enable(new_crtc_state);
 
 	/* vblanks work again, re-enable pipe CRC. */
@@ -8360,8 +8368,7 @@ static void skl_commit_modeset_enables(struct intel_atomic_state *state)
 			continue;
 
 		if (intel_dp_mst_is_slave_trans(new_crtc_state) ||
-		    is_trans_port_sync_master(new_crtc_state) ||
-		    intel_crtc_is_bigjoiner_master(new_crtc_state))
+		    is_trans_port_sync_master(new_crtc_state))
 			continue;
 
 		modeset_pipes &= ~BIT(pipe);
@@ -8371,7 +8378,7 @@ static void skl_commit_modeset_enables(struct intel_atomic_state *state)
 
 	/*
 	 * Then we enable all remaining pipes that depend on other
-	 * pipes: MST slaves and port sync masters, big joiner master
+	 * pipes: MST slaves and port sync masters
 	 */
 	for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state, i) {
 		enum pipe pipe = crtc->pipe;
-- 
2.19.1


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [Intel-gfx] ✓ Fi.CI.BAT: success for drm/i915/display/: Refactor hsw_crtc_enable for bigjoiner cleanup
  2022-03-15 23:38 [Intel-gfx] [PATCH] drm/i915/display/: Refactor hsw_crtc_enable for bigjoiner cleanup Manasi Navare
@ 2022-03-16  0:41 ` Patchwork
  2022-03-16  3:21 ` [Intel-gfx] ✗ Fi.CI.IGT: failure " Patchwork
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 14+ messages in thread
From: Patchwork @ 2022-03-16  0:41 UTC (permalink / raw)
  To: Manasi Navare; +Cc: intel-gfx

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

== Series Details ==

Series: drm/i915/display/: Refactor hsw_crtc_enable for bigjoiner cleanup
URL   : https://patchwork.freedesktop.org/series/101409/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_11365 -> Patchwork_22580
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

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

Participating hosts (48 -> 39)
------------------------------

  Additional (1): bat-dg2-8 
  Missing    (10): shard-tglu bat-dg1-6 fi-hsw-4200u bat-dg2-9 fi-bsw-cyan fi-ctg-p8600 bat-rpls-1 shard-rkl shard-dg1 fi-bdw-samus 

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

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

### IGT changes ###

#### Suppressed ####

  The following results come from untrusted machines, tests, or statuses.
  They do not affect the overall result.

  * igt@i915_selftest@live@gt_lrc:
    - {bat-dg2-8}:        NOTRUN -> [INCOMPLETE][1]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22580/bat-dg2-8/igt@i915_selftest@live@gt_lrc.html

  * igt@kms_flip@basic-flip-vs-wf_vblank:
    - {bat-dg2-8}:        NOTRUN -> [SKIP][2] +22 similar issues
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22580/bat-dg2-8/igt@kms_flip@basic-flip-vs-wf_vblank.html

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

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

### IGT changes ###

#### Issues hit ####

  * igt@i915_selftest@live@requests:
    - fi-blb-e6850:       [PASS][3] -> [DMESG-FAIL][4] ([i915#4528])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11365/fi-blb-e6850/igt@i915_selftest@live@requests.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22580/fi-blb-e6850/igt@i915_selftest@live@requests.html

  * igt@runner@aborted:
    - fi-blb-e6850:       NOTRUN -> [FAIL][5] ([fdo#109271] / [i915#2403] / [i915#4312])
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22580/fi-blb-e6850/igt@runner@aborted.html
    - fi-bdw-5557u:       NOTRUN -> [FAIL][6] ([i915#2426] / [i915#4312])
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22580/fi-bdw-5557u/igt@runner@aborted.html

  
#### Possible fixes ####

  * igt@i915_selftest@live@migrate:
    - {bat-rpls-2}:       [DMESG-WARN][7] ([i915#4391]) -> [PASS][8]
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11365/bat-rpls-2/igt@i915_selftest@live@migrate.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22580/bat-rpls-2/igt@i915_selftest@live@migrate.html

  * igt@kms_flip@basic-flip-vs-wf_vblank@a-edp1:
    - {bat-adlp-6}:       [DMESG-WARN][9] ([i915#3576]) -> [PASS][10]
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11365/bat-adlp-6/igt@kms_flip@basic-flip-vs-wf_vblank@a-edp1.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22580/bat-adlp-6/igt@kms_flip@basic-flip-vs-wf_vblank@a-edp1.html

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

  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109285]: https://bugs.freedesktop.org/show_bug.cgi?id=109285
  [fdo#111827]: https://bugs.freedesktop.org/show_bug.cgi?id=111827
  [i915#1072]: https://gitlab.freedesktop.org/drm/intel/issues/1072
  [i915#1155]: https://gitlab.freedesktop.org/drm/intel/issues/1155
  [i915#2403]: https://gitlab.freedesktop.org/drm/intel/issues/2403
  [i915#2426]: https://gitlab.freedesktop.org/drm/intel/issues/2426
  [i915#2582]: https://gitlab.freedesktop.org/drm/intel/issues/2582
  [i915#3291]: https://gitlab.freedesktop.org/drm/intel/issues/3291
  [i915#3555]: https://gitlab.freedesktop.org/drm/intel/issues/3555
  [i915#3576]: https://gitlab.freedesktop.org/drm/intel/issues/3576
  [i915#3595]: https://gitlab.freedesktop.org/drm/intel/issues/3595
  [i915#3708]: https://gitlab.freedesktop.org/drm/intel/issues/3708
  [i915#4077]: https://gitlab.freedesktop.org/drm/intel/issues/4077
  [i915#4079]: https://gitlab.freedesktop.org/drm/intel/issues/4079
  [i915#4083]: https://gitlab.freedesktop.org/drm/intel/issues/4083
  [i915#4212]: https://gitlab.freedesktop.org/drm/intel/issues/4212
  [i915#4213]: https://gitlab.freedesktop.org/drm/intel/issues/4213
  [i915#4215]: https://gitlab.freedesktop.org/drm/intel/issues/4215
  [i915#4312]: https://gitlab.freedesktop.org/drm/intel/issues/4312
  [i915#4391]: https://gitlab.freedesktop.org/drm/intel/issues/4391
  [i915#4528]: https://gitlab.freedesktop.org/drm/intel/issues/4528
  [i915#4983]: https://gitlab.freedesktop.org/drm/intel/issues/4983
  [i915#5190]: https://gitlab.freedesktop.org/drm/intel/issues/5190
  [i915#5192]: https://gitlab.freedesktop.org/drm/intel/issues/5192
  [i915#5193]: https://gitlab.freedesktop.org/drm/intel/issues/5193
  [i915#5274]: https://gitlab.freedesktop.org/drm/intel/issues/5274
  [i915#5275]: https://gitlab.freedesktop.org/drm/intel/issues/5275
  [i915#5341]: https://gitlab.freedesktop.org/drm/intel/issues/5341
  [i915#5342]: https://gitlab.freedesktop.org/drm/intel/issues/5342


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

  * Linux: CI_DRM_11365 -> Patchwork_22580

  CI-20190529: 20190529
  CI_DRM_11365: 5a27c2b120b176a313edbea33224847ea76d6c21 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_6382: a6a5a178cb1cbe0dab8d8d092a4aee932ccb93cc @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
  Patchwork_22580: b583d9b8b5cc13a13fbb5342c11f8fe6f1a2d967 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

b583d9b8b5cc drm/i915/display/: Refactor hsw_crtc_enable for bigjoiner cleanup

== Logs ==

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

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [Intel-gfx] ✗ Fi.CI.IGT: failure for drm/i915/display/: Refactor hsw_crtc_enable for bigjoiner cleanup
  2022-03-15 23:38 [Intel-gfx] [PATCH] drm/i915/display/: Refactor hsw_crtc_enable for bigjoiner cleanup Manasi Navare
  2022-03-16  0:41 ` [Intel-gfx] ✓ Fi.CI.BAT: success for " Patchwork
@ 2022-03-16  3:21 ` Patchwork
  2022-03-16  7:48 ` [Intel-gfx] [PATCH] " Jani Nikula
  2022-03-17 18:52 ` Ville Syrjälä
  3 siblings, 0 replies; 14+ messages in thread
From: Patchwork @ 2022-03-16  3:21 UTC (permalink / raw)
  To: Manasi Navare; +Cc: intel-gfx

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

== Series Details ==

Series: drm/i915/display/: Refactor hsw_crtc_enable for bigjoiner cleanup
URL   : https://patchwork.freedesktop.org/series/101409/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_11365_full -> Patchwork_22580_full
====================================================

Summary
-------

  **FAILURE**

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

  

Participating hosts (13 -> 12)
------------------------------

  Missing    (1): shard-dg1 

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

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

### IGT changes ###

#### Possible regressions ####

  * igt@kms_plane_scaling@scaler-with-pixel-format-unity-scaling@pipe-b-edp-1-scaler-with-pixel-format:
    - shard-iclb:         [PASS][1] -> [INCOMPLETE][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11365/shard-iclb1/igt@kms_plane_scaling@scaler-with-pixel-format-unity-scaling@pipe-b-edp-1-scaler-with-pixel-format.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22580/shard-iclb2/igt@kms_plane_scaling@scaler-with-pixel-format-unity-scaling@pipe-b-edp-1-scaler-with-pixel-format.html

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

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

### CI changes ###

#### Issues hit ####

  * boot:
    - shard-glk:          ([PASS][3], [PASS][4], [PASS][5], [PASS][6], [PASS][7], [PASS][8], [PASS][9], [PASS][10], [PASS][11], [PASS][12], [PASS][13], [PASS][14], [PASS][15], [PASS][16], [PASS][17], [PASS][18], [PASS][19], [PASS][20], [PASS][21], [PASS][22], [PASS][23], [PASS][24], [PASS][25], [PASS][26], [PASS][27]) -> ([PASS][28], [PASS][29], [PASS][30], [PASS][31], [FAIL][32], [PASS][33], [PASS][34], [PASS][35], [PASS][36], [PASS][37], [PASS][38], [PASS][39], [PASS][40], [PASS][41], [PASS][42], [PASS][43], [PASS][44], [PASS][45], [PASS][46], [PASS][47], [PASS][48], [PASS][49], [PASS][50], [PASS][51], [PASS][52]) ([i915#4392])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11365/shard-glk1/boot.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11365/shard-glk1/boot.html
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11365/shard-glk1/boot.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11365/shard-glk2/boot.html
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11365/shard-glk2/boot.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11365/shard-glk2/boot.html
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11365/shard-glk3/boot.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11365/shard-glk3/boot.html
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11365/shard-glk4/boot.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11365/shard-glk4/boot.html
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11365/shard-glk4/boot.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11365/shard-glk5/boot.html
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11365/shard-glk5/boot.html
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11365/shard-glk6/boot.html
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11365/shard-glk6/boot.html
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11365/shard-glk6/boot.html
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11365/shard-glk7/boot.html
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11365/shard-glk7/boot.html
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11365/shard-glk7/boot.html
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11365/shard-glk8/boot.html
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11365/shard-glk8/boot.html
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11365/shard-glk8/boot.html
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11365/shard-glk9/boot.html
   [26]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11365/shard-glk9/boot.html
   [27]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11365/shard-glk9/boot.html
   [28]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22580/shard-glk9/boot.html
   [29]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22580/shard-glk9/boot.html
   [30]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22580/shard-glk8/boot.html
   [31]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22580/shard-glk8/boot.html
   [32]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22580/shard-glk8/boot.html
   [33]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22580/shard-glk8/boot.html
   [34]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22580/shard-glk7/boot.html
   [35]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22580/shard-glk7/boot.html
   [36]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22580/shard-glk7/boot.html
   [37]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22580/shard-glk6/boot.html
   [38]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22580/shard-glk6/boot.html
   [39]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22580/shard-glk6/boot.html
   [40]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22580/shard-glk5/boot.html
   [41]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22580/shard-glk5/boot.html
   [42]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22580/shard-glk5/boot.html
   [43]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22580/shard-glk4/boot.html
   [44]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22580/shard-glk4/boot.html
   [45]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22580/shard-glk3/boot.html
   [46]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22580/shard-glk3/boot.html
   [47]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22580/shard-glk2/boot.html
   [48]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22580/shard-glk2/boot.html
   [49]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22580/shard-glk2/boot.html
   [50]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22580/shard-glk1/boot.html
   [51]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22580/shard-glk1/boot.html
   [52]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22580/shard-glk1/boot.html

  

### IGT changes ###

#### Issues hit ####

  * igt@feature_discovery@chamelium:
    - shard-iclb:         NOTRUN -> [SKIP][53] ([fdo#111827])
   [53]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22580/shard-iclb4/igt@feature_discovery@chamelium.html

  * igt@gem_eio@unwedge-stress:
    - shard-tglb:         [PASS][54] -> [FAIL][55] ([i915#232])
   [54]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11365/shard-tglb7/igt@gem_eio@unwedge-stress.html
   [55]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22580/shard-tglb1/igt@gem_eio@unwedge-stress.html

  * igt@gem_exec_fair@basic-deadline:
    - shard-skl:          NOTRUN -> [FAIL][56] ([i915#2846])
   [56]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22580/shard-skl4/igt@gem_exec_fair@basic-deadline.html
    - shard-apl:          NOTRUN -> [FAIL][57] ([i915#2846])
   [57]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22580/shard-apl7/igt@gem_exec_fair@basic-deadline.html

  * igt@gem_exec_fair@basic-none-vip@rcs0:
    - shard-tglb:         [PASS][58] -> [FAIL][59] ([i915#2842])
   [58]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11365/shard-tglb6/igt@gem_exec_fair@basic-none-vip@rcs0.html
   [59]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22580/shard-tglb3/igt@gem_exec_fair@basic-none-vip@rcs0.html

  * igt@gem_exec_fair@basic-pace-solo@rcs0:
    - shard-iclb:         [PASS][60] -> [FAIL][61] ([i915#2842]) +1 similar issue
   [60]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11365/shard-iclb3/igt@gem_exec_fair@basic-pace-solo@rcs0.html
   [61]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22580/shard-iclb8/igt@gem_exec_fair@basic-pace-solo@rcs0.html

  * igt@gem_exec_fair@basic-pace@rcs0:
    - shard-kbl:          [PASS][62] -> [FAIL][63] ([i915#2842]) +2 similar issues
   [62]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11365/shard-kbl4/igt@gem_exec_fair@basic-pace@rcs0.html
   [63]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22580/shard-kbl6/igt@gem_exec_fair@basic-pace@rcs0.html

  * igt@gem_exec_params@secure-non-root:
    - shard-iclb:         NOTRUN -> [SKIP][64] ([fdo#112283])
   [64]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22580/shard-iclb4/igt@gem_exec_params@secure-non-root.html

  * igt@gem_lmem_swapping@heavy-multi:
    - shard-apl:          NOTRUN -> [SKIP][65] ([fdo#109271] / [i915#4613]) +2 similar issues
   [65]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22580/shard-apl6/igt@gem_lmem_swapping@heavy-multi.html

  * igt@gem_lmem_swapping@heavy-random:
    - shard-iclb:         NOTRUN -> [SKIP][66] ([i915#4613])
   [66]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22580/shard-iclb4/igt@gem_lmem_swapping@heavy-random.html

  * igt@gem_lmem_swapping@parallel-random:
    - shard-skl:          NOTRUN -> [SKIP][67] ([fdo#109271] / [i915#4613]) +3 similar issues
   [67]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22580/shard-skl9/igt@gem_lmem_swapping@parallel-random.html

  * igt@gem_ppgtt@flink-and-close-vma-leak:
    - shard-skl:          [PASS][68] -> [FAIL][69] ([i915#644])
   [68]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11365/shard-skl8/igt@gem_ppgtt@flink-and-close-vma-leak.html
   [69]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22580/shard-skl7/igt@gem_ppgtt@flink-and-close-vma-leak.html

  * igt@gen3_render_mixed_blits:
    - shard-iclb:         NOTRUN -> [SKIP][70] ([fdo#109289])
   [70]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22580/shard-iclb4/igt@gen3_render_mixed_blits.html

  * igt@gen9_exec_parse@allowed-single:
    - shard-skl:          [PASS][71] -> [DMESG-WARN][72] ([i915#1436] / [i915#716])
   [71]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11365/shard-skl7/igt@gen9_exec_parse@allowed-single.html
   [72]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22580/shard-skl8/igt@gen9_exec_parse@allowed-single.html

  * igt@gen9_exec_parse@bb-secure:
    - shard-iclb:         NOTRUN -> [SKIP][73] ([i915#2856])
   [73]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22580/shard-iclb4/igt@gen9_exec_parse@bb-secure.html

  * igt@i915_pm_dc@dc6-dpms:
    - shard-skl:          NOTRUN -> [FAIL][74] ([i915#454])
   [74]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22580/shard-skl4/igt@i915_pm_dc@dc6-dpms.html

  * igt@i915_pm_rpm@gem-execbuf-stress:
    - shard-iclb:         NOTRUN -> [INCOMPLETE][75] ([i915#5324])
   [75]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22580/shard-iclb4/igt@i915_pm_rpm@gem-execbuf-stress.html

  * igt@i915_suspend@fence-restore-tiled2untiled:
    - shard-skl:          [PASS][76] -> [INCOMPLETE][77] ([i915#4817] / [i915#4939])
   [76]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11365/shard-skl10/igt@i915_suspend@fence-restore-tiled2untiled.html
   [77]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22580/shard-skl7/igt@i915_suspend@fence-restore-tiled2untiled.html

  * igt@kms_big_fb@4-tiled-max-hw-stride-64bpp-rotate-180:
    - shard-iclb:         NOTRUN -> [SKIP][78] ([i915#5286])
   [78]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22580/shard-iclb4/igt@kms_big_fb@4-tiled-max-hw-stride-64bpp-rotate-180.html

  * igt@kms_big_fb@y-tiled-64bpp-rotate-270:
    - shard-iclb:         NOTRUN -> [SKIP][79] ([fdo#110725] / [fdo#111614])
   [79]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22580/shard-iclb4/igt@kms_big_fb@y-tiled-64bpp-rotate-270.html

  * igt@kms_big_fb@y-tiled-max-hw-stride-32bpp-rotate-180-hflip:
    - shard-skl:          NOTRUN -> [SKIP][80] ([fdo#109271] / [i915#3777]) +1 similar issue
   [80]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22580/shard-skl1/igt@kms_big_fb@y-tiled-max-hw-stride-32bpp-rotate-180-hflip.html

  * igt@kms_big_fb@yf-tiled-max-hw-stride-32bpp-rotate-0-async-flip:
    - shard-skl:          NOTRUN -> [FAIL][81] ([i915#3743])
   [81]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22580/shard-skl1/igt@kms_big_fb@yf-tiled-max-hw-stride-32bpp-rotate-0-async-flip.html

  * igt@kms_big_fb@yf-tiled-max-hw-stride-32bpp-rotate-180-hflip-async-flip:
    - shard-apl:          NOTRUN -> [SKIP][82] ([fdo#109271] / [i915#3777]) +3 similar issues
   [82]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22580/shard-apl6/igt@kms_big_fb@yf-tiled-max-hw-stride-32bpp-rotate-180-hflip-async-flip.html

  * igt@kms_ccs@pipe-b-ccs-on-another-bo-y_tiled_gen12_mc_ccs:
    - shard-skl:          NOTRUN -> [SKIP][83] ([fdo#109271] / [i915#3886]) +13 similar issues
   [83]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22580/shard-skl4/igt@kms_ccs@pipe-b-ccs-on-another-bo-y_tiled_gen12_mc_ccs.html
    - shard-apl:          NOTRUN -> [SKIP][84] ([fdo#109271] / [i915#3886]) +5 similar issues
   [84]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22580/shard-apl7/igt@kms_ccs@pipe-b-ccs-on-another-bo-y_tiled_gen12_mc_ccs.html

  * igt@kms_ccs@pipe-c-bad-pixel-format-y_tiled_gen12_mc_ccs:
    - shard-iclb:         NOTRUN -> [SKIP][85] ([fdo#109278] / [i915#3886]) +1 similar issue
   [85]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22580/shard-iclb4/igt@kms_ccs@pipe-c-bad-pixel-format-y_tiled_gen12_mc_ccs.html

  * igt@kms_chamelium@dp-hpd-with-enabled-mode:
    - shard-iclb:         NOTRUN -> [SKIP][86] ([fdo#109284] / [fdo#111827]) +3 similar issues
   [86]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22580/shard-iclb4/igt@kms_chamelium@dp-hpd-with-enabled-mode.html

  * igt@kms_chamelium@vga-edid-read:
    - shard-apl:          NOTRUN -> [SKIP][87] ([fdo#109271] / [fdo#111827]) +14 similar issues
   [87]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22580/shard-apl6/igt@kms_chamelium@vga-edid-read.html

  * igt@kms_color@pipe-d-ctm-0-75:
    - shard-iclb:         NOTRUN -> [SKIP][88] ([fdo#109278] / [i915#1149])
   [88]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22580/shard-iclb4/igt@kms_color@pipe-d-ctm-0-75.html

  * igt@kms_color_chamelium@pipe-b-ctm-max:
    - shard-skl:          NOTRUN -> [SKIP][89] ([fdo#109271] / [fdo#111827]) +23 similar issues
   [89]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22580/shard-skl2/igt@kms_color_chamelium@pipe-b-ctm-max.html

  * igt@kms_cursor_crc@pipe-a-cursor-max-size-sliding:
    - shard-iclb:         NOTRUN -> [SKIP][90] ([fdo#109278]) +9 similar issues
   [90]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22580/shard-iclb4/igt@kms_cursor_crc@pipe-a-cursor-max-size-sliding.html

  * igt@kms_cursor_crc@pipe-a-cursor-suspend:
    - shard-kbl:          [PASS][91] -> [DMESG-WARN][92] ([i915#180]) +2 similar issues
   [91]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11365/shard-kbl4/igt@kms_cursor_crc@pipe-a-cursor-suspend.html
   [92]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22580/shard-kbl4/igt@kms_cursor_crc@pipe-a-cursor-suspend.html

  * igt@kms_cursor_crc@pipe-c-cursor-512x512-offscreen:
    - shard-iclb:         NOTRUN -> [SKIP][93] ([fdo#109278] / [fdo#109279])
   [93]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22580/shard-iclb4/igt@kms_cursor_crc@pipe-c-cursor-512x512-offscreen.html

  * igt@kms_cursor_legacy@flip-vs-cursor-atomic-transitions-varying-size:
    - shard-skl:          NOTRUN -> [FAIL][94] ([i915#2346] / [i915#533])
   [94]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22580/shard-skl4/igt@kms_cursor_legacy@flip-vs-cursor-atomic-transitions-varying-size.html

  * igt@kms_fbcon_fbt@fbc-suspend:
    - shard-apl:          [PASS][95] -> [INCOMPLETE][96] ([i915#180] / [i915#1982])
   [95]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11365/shard-apl4/igt@kms_fbcon_fbt@fbc-suspend.html
   [96]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22580/shard-apl3/igt@kms_fbcon_fbt@fbc-suspend.html

  * igt@kms_flip@flip-vs-expired-vblank-interruptible@a-hdmi-a1:
    - shard-glk:          [PASS][97] -> [FAIL][98] ([i915#2122])
   [97]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11365/shard-glk9/igt@kms_flip@flip-vs-expired-vblank-interruptible@a-hdmi-a1.html
   [98]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22580/shard-glk3/igt@kms_flip@flip-vs-expired-vblank-interruptible@a-hdmi-a1.html

  * igt@kms_flip@flip-vs-expired-vblank@a-edp1:
    - shard-skl:          [PASS][99] -> [FAIL][100] ([i915#79]) +1 similar issue
   [99]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11365/shard-skl7/igt@kms_flip@flip-vs-expired-vblank@a-edp1.html
   [100]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22580/shard-skl8/igt@kms_flip@flip-vs-expired-vblank@a-edp1.html

  * igt@kms_flip@flip-vs-suspend-interruptible@c-dp1:
    - shard-apl:          [PASS][101] -> [DMESG-WARN][102] ([i915#180]) +3 similar issues
   [101]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11365/shard-apl6/igt@kms_flip@flip-vs-suspend-interruptible@c-dp1.html
   [102]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22580/shard-apl2/igt@kms_flip@flip-vs-suspend-interruptible@c-dp1.html

  * igt@kms_frontbuffer_tracking@fbc-2p-primscrn-cur-indfb-draw-render:
    - shard-iclb:         NOTRUN -> [SKIP][103] ([fdo#109280]) +5 similar issues
   [103]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22580/shard-iclb4/igt@kms_frontbuffer_tracking@fbc-2p-primscrn-cur-indfb-draw-render.html

  * igt@kms_frontbuffer_tracking@fbc-2p-primscrn-shrfb-plflip-blt:
    - shard-glk:          [PASS][104] -> [FAIL][105] ([i915#2546])
   [104]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11365/shard-glk5/igt@kms_frontbuffer_tracking@fbc-2p-primscrn-shrfb-plflip-blt.html
   [105]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22580/shard-glk5/igt@kms_frontbuffer_tracking@fbc-2p-primscrn-shrfb-plflip-blt.html

  * igt@kms_frontbuffer_tracking@fbc-2p-scndscrn-spr-indfb-draw-mmap-wc:
    - shard-apl:          NOTRUN -> [SKIP][106] ([fdo#109271]) +161 similar issues
   [106]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22580/shard-apl3/igt@kms_frontbuffer_tracking@fbc-2p-scndscrn-spr-indfb-draw-mmap-wc.html

  * igt@kms_hdr@bpc-switch-suspend@bpc-switch-suspend-edp-1-pipe-a:
    - shard-skl:          NOTRUN -> [FAIL][107] ([i915#1188])
   [107]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22580/shard-skl2/igt@kms_hdr@bpc-switch-suspend@bpc-switch-suspend-edp-1-pipe-a.html

  * igt@kms_pipe_crc_basic@hang-read-crc-pipe-d:
    - shard-skl:          NOTRUN -> [SKIP][108] ([fdo#109271] / [i915#533]) +3 similar issues
   [108]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22580/shard-skl4/igt@kms_pipe_crc_basic@hang-read-crc-pipe-d.html

  * igt@kms_pipe_crc_basic@read-crc-pipe-d:
    - shard-apl:          NOTRUN -> [SKIP][109] ([fdo#109271] / [i915#533]) +1 similar issue
   [109]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22580/shard-apl2/igt@kms_pipe_crc_basic@read-crc-pipe-d.html

  * igt@kms_pipe_crc_basic@suspend-read-crc-pipe-b:
    - shard-skl:          [PASS][110] -> [INCOMPLETE][111] ([i915#4939])
   [110]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11365/shard-skl7/igt@kms_pipe_crc_basic@suspend-read-crc-pipe-b.html
   [111]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22580/shard-skl6/igt@kms_pipe_crc_basic@suspend-read-crc-pipe-b.html

  * igt@kms_plane_alpha_blend@pipe-b-alpha-opaque-fb:
    - shard-skl:          NOTRUN -> [FAIL][112] ([fdo#108145] / [i915#265]) +1 similar issue
   [112]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22580/shard-skl1/igt@kms_plane_alpha_blend@pipe-b-alpha-opaque-fb.html

  * igt@kms_plane_scaling@planes-upscale-factor-0-25-downscale-factor-0-25@pipe-a-edp-1-planes-upscale-downscale:
    - shard-skl:          NOTRUN -> [SKIP][113] ([fdo#109271]) +257 similar issues
   [113]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22580/shard-skl4/igt@kms_plane_scaling@planes-upscale-factor-0-25-downscale-factor-0-25@pipe-a-edp-1-planes-upscale-downscale.html

  * igt@kms_psr2_sf@overlay-plane-update-sf-dmg-area:
    - shard-skl:          NOTRUN -> [SKIP][114] ([fdo#109271] / [i915#658]) +4 similar issues
   [114]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22580/shard-skl4/igt@kms_psr2_sf@overlay-plane-update-sf-dmg-area.html

  * igt@kms_psr2_su@page_flip-xrgb8888:
    - shard-apl:          NOTRUN -> [SKIP][115] ([fdo#109271] / [i915#658])
   [115]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22580/shard-apl7/igt@kms_psr2_su@page_flip-xrgb8888.html

  * igt@kms_psr@psr2_cursor_blt:
    - shard-iclb:         NOTRUN -> [SKIP][116] ([fdo#109441])
   [116]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22580/shard-iclb4/igt@kms_psr@psr2_cursor_blt.html

  * igt@kms_psr@psr2_suspend:
    - shard-iclb:         [PASS][117] -> [SKIP][118] ([fdo#109441]) +2 similar issues
   [117]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11365/shard-iclb2/igt@kms_psr@psr2_suspend.html
   [118]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22580/shard-iclb7/igt@kms_psr@psr2_suspend.html

  * igt@kms_sysfs_edid_timing:
    - shard-apl:          NOTRUN -> [FAIL][119] ([IGT#2])
   [119]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22580/shard-apl6/igt@kms_sysfs_edid_timing.html

  * igt@kms_vblank@pipe-c-accuracy-idle:
    - shard-skl:          [PASS][120] -> [FAIL][121] ([i915#43])
   [120]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11365/shard-skl7/igt@kms_vblank@pipe-c-accuracy-idle.html
   [121]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22580/shard-skl8/igt@kms_vblank@pipe-c-accuracy-idle.html

  * igt@kms_writeback@writeback-pixel-formats:
    - shard-skl:          NOTRUN -> [SKIP][122] ([fdo#109271] / [i915#2437]) +1 similar issue
   [122]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22580/shard-skl4/igt@kms_writeback@writeback-pixel-formats.html
    - shard-apl:          NOTRUN -> [SKIP][123] ([fdo#109271] / [i915#2437])
   [123]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22580/shard-apl7/igt@kms_writeback@writeback-pixel-formats.html

  * igt@perf@polling-parameterized:
    - shard-skl:          NOTRUN -> [FAIL][124] ([i915#1542])
   [124]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22580/shard-skl2/igt@perf@polling-parameterized.html

  * igt@perf@short-reads:
    - shard-skl:          [PASS][125] -> [FAIL][126] ([i915#51])
   [125]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11365/shard-skl6/igt@perf@short-reads.html
   [126]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22580/shard-skl1/igt@perf@short-reads.html

  * igt@prime_nv_api@i915_nv_reimport_twice_check_flink_name:
    - shard-iclb:         NOTRUN -> [SKIP][127] ([fdo#109291])
   [127]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22580/shard-iclb4/igt@prime_nv_api@i915_nv_reimport_twice_check_flink_name.html

  * igt@syncobj_timeline@transfer-timeline-point:
    - shard-skl:          NOTRUN -> [DMESG-FAIL][128] ([i915#5098])
   [128]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22580/shard-skl1/igt@syncobj_timeline@transfer-timeline-point.html

  * igt@sysfs_clients@fair-0:
    - shard-skl:          NOTRUN -> [SKIP][129] ([fdo#109271] / [i915#2994]) +4 similar issues
   [129]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22580/shard-skl4/igt@sysfs_clients@fair-0.html

  * igt@sysfs_clients@recycle-many:
    - shard-apl:          NOTRUN -> [SKIP][130] ([fdo#109271] / [i915#2994]) +2 similar issues
   [130]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22580/shard-apl7/igt@sysfs_clients@recycle-many.html

  
#### Possible fixes ####

  * igt@fbdev@nullptr:
    - {shard-rkl}:        [SKIP][131] ([i915#2582]) -> [PASS][132]
   [131]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11365/shard-rkl-1/igt@fbdev@nullptr.html
   [132]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22580/shard-rkl-6/igt@fbdev@nullptr.html

  * igt@gem_exec_fair@basic-flow@rcs0:
    - shard-tglb:         [FAIL][133] ([i915#2842]) -> [PASS][134] +1 similar issue
   [133]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11365/shard-tglb7/igt@gem_exec_fair@basic-flow@rcs0.html
   [134]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22580/shard-tglb1/igt@gem_exec_fair@basic-flow@rcs0.html
    - {shard-rkl}:        [FAIL][135] ([i915#2842]) -> [PASS][136]
   [135]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11365/shard-rkl-2/igt@gem_exec_fair@basic-flow@rcs0.html
   [136]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22580/shard-rkl-4/igt@gem_exec_fair@basic-flow@rcs0.html

  * igt@gem_exec_fair@basic-pace-solo@rcs0:
    - shard-apl:          [FAIL][137] ([i915#2842]) -> [PASS][138]
   [137]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11365/shard-apl7/igt@gem_exec_fair@basic-pace-solo@rcs0.html
   [138]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22580/shard-apl1/igt@gem_exec_fair@basic-pace-solo@rcs0.html

  * igt@gem_exec_fair@basic-pace@vecs0:
    - shard-kbl:          [FAIL][139] ([i915#2842]) -> [PASS][140] +1 similar issue
   [139]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11365/shard-kbl4/igt@gem_exec_fair@basic-pace@vecs0.html
   [140]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22580/shard-kbl6/igt@gem_exec_fair@basic-pace@vecs0.html

  * igt@gem_exec_schedule@smoketest@rcs0:
    - {shard-rkl}:        [DMESG-WARN][141] -> [PASS][142]
   [141]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11365/shard-rkl-5/igt@gem_exec_schedule@smoketest@rcs0.html
   [142]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22580/shard-rkl-4/igt@gem_exec_schedule@smoketest@rcs0.html

  * igt@gem_exec_whisper@basic-fds-forked-all:
    - shard-iclb:         [INCOMPLETE][143] ([i915#1895]) -> [PASS][144]
   [143]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11365/shard-iclb7/igt@gem_exec_whisper@basic-fds-forked-all.html
   [144]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22580/shard-iclb4/igt@gem_exec_whisper@basic-fds-forked-all.html

  * igt@i915_pm_backlight@basic-brightness:
    - {shard-rkl}:        [SKIP][145] ([i915#3012]) -> [PASS][146]
   [145]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11365/shard-rkl-1/igt@i915_pm_backlight@basic-brightness.html
   [146]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22580/shard-rkl-6/igt@i915_pm_backlight@basic-brightness.html

  * igt@i915_pm_dc@dc6-dpms:
    - {shard-rkl}:        [FAIL][147] ([i915#3989]) -> [PASS][148]
   [147]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11365/shard-rkl-5/igt@i915_pm_dc@dc6-dpms.html
   [148]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22580/shard-rkl-1/igt@i915_pm_dc@dc6-dpms.html

  * igt@i915_pm_dc@dc6-psr:
    - {shard-rkl}:        [SKIP][149] ([i915#658]) -> [PASS][150]
   [149]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11365/shard-rkl-4/igt@i915_pm_dc@dc6-psr.html
   [150]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22580/shard-rkl-6/igt@i915_pm_dc@dc6-psr.html

  * igt@i915_pm_dc@dc9-dpms:
    - {shard-tglu}:       [SKIP][151] ([i915#4281]) -> [PASS][152]
   [151]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11365/shard-tglu-6/igt@i915_pm_dc@dc9-dpms.html
   [152]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22580/shard-tglu-2/igt@i915_pm_dc@dc9-dpms.html

  * igt@kms_cursor_crc@pipe-a-cursor-64x64-rapid-movement:
    - {shard-rkl}:        [SKIP][153] ([fdo#112022] / [i915#4070]) -> [PASS][154] +5 similar issues
   [153]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11365/shard-rkl-4/igt@kms_cursor_crc@pipe-a-cursor-64x64-rapid-movement.html
   [154]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22580/shard-rkl-6/igt@kms_cursor_crc@pipe-a-cursor-64x64-rapid-movement.html

  * igt@kms_cursor_legacy@flip-vs-cursor-crc-atomic:
    - {shard-rkl}:        [SKIP][155] ([fdo#111825] / [i915#4070]) -> [PASS][156] +3 similar issues
   [155]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11365/shard-rkl-4/igt@kms_cursor_legacy@flip-vs-cursor-crc-atomic.html
   [156]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22580/shard-rkl-6/igt@kms_cursor_legacy@flip-vs-cursor-crc-atomic.html

  * igt@kms_cursor_legacy@pipe-c-forked-bo:
    - {shard-rkl}:        [SKIP][157] ([i915#4070]) -> [PASS][158]
   [157]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11365/shard-rkl-1/igt@kms_cursor_legacy@pipe-c-forked-bo.html
   [158]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22580/shard-rkl-5/igt@kms_cursor_legacy@pipe-c-forked-bo.html

  * igt@kms_draw_crc@draw-method-xrgb8888-render-untiled:
    - {shard-rkl}:        [SKIP][159] ([i915#4098] / [i915#4369]) -> [PASS][160] +2 similar issues
   [159]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11365/shard-rkl-4/igt@kms_draw_crc@draw-method-xrgb8888-render-untiled.html
   [160]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22580/shard-rkl-6/igt@kms_draw_crc@draw-method-xrgb8888-render-untiled.html

  * igt@kms_draw_crc@fill-fb:
    - {shard-rkl}:        [SKIP][161] ([fdo#111314] / [i915#4098] / [i915#4369]) -> [PASS][162]
   [161]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11365/shard-rkl-1/igt@kms_draw_crc@fill-fb.html
   [162]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22580/shard-rkl-6/igt@kms_draw_crc@fill-fb.html

  * igt@kms_fbcon_fbt@psr:
    - {shard-rkl}:        [SKIP][163] ([fdo#110189] / [i915#3955]) -> [PASS][164]
   [163]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11365/shard-rkl-1/igt@kms_fbcon_fbt@psr.html
   [164]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22580/shard-rkl-6/igt@kms_fbcon_fbt@psr.html

  * igt@kms_flip@2x-flip-vs-expired-vblank-interruptible@ab-hdmi-a1-hdmi-a2:
    - shard-glk:          [FAIL][165] ([i915#79]) -> [PASS][166] +1 similar issue
   [165]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11365/shard-glk6/igt@kms_flip@2x-flip-vs-expired-vblank-interruptible@

== Logs ==

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

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [Intel-gfx] [PATCH] drm/i915/display/: Refactor hsw_crtc_enable for bigjoiner cleanup
  2022-03-15 23:38 [Intel-gfx] [PATCH] drm/i915/display/: Refactor hsw_crtc_enable for bigjoiner cleanup Manasi Navare
  2022-03-16  0:41 ` [Intel-gfx] ✓ Fi.CI.BAT: success for " Patchwork
  2022-03-16  3:21 ` [Intel-gfx] ✗ Fi.CI.IGT: failure " Patchwork
@ 2022-03-16  7:48 ` Jani Nikula
  2022-03-16 17:10   ` Navare, Manasi
  2022-03-17 18:52 ` Ville Syrjälä
  3 siblings, 1 reply; 14+ messages in thread
From: Jani Nikula @ 2022-03-16  7:48 UTC (permalink / raw)
  To: Manasi Navare, intel-gfx

On Tue, 15 Mar 2022, Manasi Navare <manasi.d.navare@intel.com> wrote:
> This patch abstracts pieces of hsw_crtc_enable corresponding to different
> Bspec enable sequence steps into separate functions.
> This helps to call them in a specific order for bigjoiner master/slave
> in a cleaner fashion.

So does this contain only refactoring or functional changes also? One or
the other at a time, please, not both.

Also looks like hsw_crtc_* have accumulated just way too many checks for
platforms instead of having a clean break at e.g. display 9 and/or 11.

Adding references to "enable sequence step 8" is not helpful because
it's platform specific. (Yeah, I know there are existing references like
this, but they're also suspect.)


BR,
Jani.


>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Animesh Manna <animesh.manna@intel.com>
> Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_display.c | 125 ++++++++++---------
>  1 file changed, 66 insertions(+), 59 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index eb49973621f0..d8e6466c9fa0 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -1865,24 +1865,6 @@ static void hsw_set_frame_start_delay(const struct intel_crtc_state *crtc_state)
>  	intel_de_write(dev_priv, reg, val);
>  }
>  
> -static void icl_ddi_bigjoiner_pre_enable(struct intel_atomic_state *state,
> -					 const struct intel_crtc_state *crtc_state)
> -{
> -	struct intel_crtc *master_crtc = intel_master_crtc(crtc_state);
> -
> -	/*
> -	 * Enable sequence steps 1-7 on bigjoiner master
> -	 */
> -	if (intel_crtc_is_bigjoiner_slave(crtc_state))
> -		intel_encoders_pre_pll_enable(state, master_crtc);
> -
> -	if (crtc_state->shared_dpll)
> -		intel_enable_shared_dpll(crtc_state);
> -
> -	if (intel_crtc_is_bigjoiner_slave(crtc_state))
> -		intel_encoders_pre_enable(state, master_crtc);
> -}
> -
>  static void hsw_configure_cpu_transcoder(const struct intel_crtc_state *crtc_state)
>  {
>  	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> @@ -1910,70 +1892,73 @@ static void hsw_configure_cpu_transcoder(const struct intel_crtc_state *crtc_sta
>  	hsw_set_transconf(crtc_state);
>  }
>  
> -static void hsw_crtc_enable(struct intel_atomic_state *state,
> -			    struct intel_crtc *crtc)
> +static void hsw_crtc_pre_pll_enable(struct intel_atomic_state *state,
> +				    const struct intel_crtc_state *crtc_state)
>  {
> -	const struct intel_crtc_state *new_crtc_state =
> -		intel_atomic_get_new_crtc_state(state, crtc);
> -	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> -	enum pipe pipe = crtc->pipe, hsw_workaround_pipe;
> -	enum transcoder cpu_transcoder = new_crtc_state->cpu_transcoder;
> -	bool psl_clkgate_wa;
> -
> -	if (drm_WARN_ON(&dev_priv->drm, crtc->active))
> -		return;
> +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
>  
> -	if (!new_crtc_state->bigjoiner_pipes) {
> -		intel_encoders_pre_pll_enable(state, crtc);
> +	/*
> +	 * Enable sequence steps 1 - 7 on all pipes
> +	 */
> +	intel_encoders_pre_pll_enable(state, crtc);
> +	if (crtc_state->shared_dpll)
> +		intel_enable_shared_dpll(crtc_state);
>  
> -		if (new_crtc_state->shared_dpll)
> -			intel_enable_shared_dpll(new_crtc_state);
> +	intel_encoders_pre_enable(state, crtc);
> +}
>  
> -		intel_encoders_pre_enable(state, crtc);
> -	} else {
> -		icl_ddi_bigjoiner_pre_enable(state, new_crtc_state);
> -	}
> +static void hsw_crtc_post_pll_enable(struct intel_atomic_state *state,
> +				     const struct intel_crtc_state *crtc_state)
> +{
> +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> +	enum pipe pipe = crtc->pipe, hsw_workaround_pipe;
> +	enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
> +	bool psl_clkgate_wa;
>  
> -	intel_dsc_enable(new_crtc_state);
> +	/*
> +	 * Enable sequence step 8
> +	 */
> +	intel_dsc_enable(crtc_state);
>  
>  	if (DISPLAY_VER(dev_priv) >= 13)
> -		intel_uncompressed_joiner_enable(new_crtc_state);
> +		intel_uncompressed_joiner_enable(crtc_state);
>  
> -	intel_set_pipe_src_size(new_crtc_state);
> +	intel_set_pipe_src_size(crtc_state);
>  	if (DISPLAY_VER(dev_priv) >= 9 || IS_BROADWELL(dev_priv))
> -		bdw_set_pipemisc(new_crtc_state);
> +		bdw_set_pipemisc(crtc_state);
>  
> -	if (!intel_crtc_is_bigjoiner_slave(new_crtc_state) &&
> +	if (!intel_crtc_is_bigjoiner_slave(crtc_state) &&
>  	    !transcoder_is_dsi(cpu_transcoder))
> -		hsw_configure_cpu_transcoder(new_crtc_state);
> +		hsw_configure_cpu_transcoder(crtc_state);
>  
>  	crtc->active = true;
>  
>  	/* Display WA #1180: WaDisableScalarClockGating: glk */
>  	psl_clkgate_wa = DISPLAY_VER(dev_priv) == 10 &&
> -		new_crtc_state->pch_pfit.enabled;
> +		crtc_state->pch_pfit.enabled;
>  	if (psl_clkgate_wa)
>  		glk_pipe_scaler_clock_gating_wa(dev_priv, pipe, true);
>  
>  	if (DISPLAY_VER(dev_priv) >= 9)
> -		skl_pfit_enable(new_crtc_state);
> +		skl_pfit_enable(crtc_state);
>  	else
> -		ilk_pfit_enable(new_crtc_state);
> +		ilk_pfit_enable(crtc_state);
>  
>  	/*
>  	 * On ILK+ LUT must be loaded before the pipe is running but with
>  	 * clocks enabled
>  	 */
> -	intel_color_load_luts(new_crtc_state);
> -	intel_color_commit(new_crtc_state);
> +	intel_color_load_luts(crtc_state);
> +	intel_color_commit(crtc_state);
>  	/* update DSPCNTR to configure gamma/csc for pipe bottom color */
>  	if (DISPLAY_VER(dev_priv) < 9)
> -		intel_disable_primary_plane(new_crtc_state);
> +		intel_disable_primary_plane(crtc_state);
>  
> -	hsw_set_linetime_wm(new_crtc_state);
> +	hsw_set_linetime_wm(crtc_state);
>  
>  	if (DISPLAY_VER(dev_priv) >= 11)
> -		icl_set_pipe_chicken(new_crtc_state);
> +		icl_set_pipe_chicken(crtc_state);
>  
>  	intel_initial_watermarks(state, crtc);
>  
> @@ -1984,8 +1969,8 @@ static void hsw_crtc_enable(struct intel_atomic_state *state,
>  		icl_pipe_mbus_enable(crtc, dbuf_state->joined_mbus);
>  	}
>  
> -	if (intel_crtc_is_bigjoiner_slave(new_crtc_state))
> -		intel_crtc_vblank_on(new_crtc_state);
> +	if (intel_crtc_is_bigjoiner_slave(crtc_state))
> +		intel_crtc_vblank_on(crtc_state);
>  
>  	intel_encoders_enable(state, crtc);
>  
> @@ -1996,7 +1981,7 @@ static void hsw_crtc_enable(struct intel_atomic_state *state,
>  
>  	/* If we change the relative order between pipe/planes enabling, we need
>  	 * to change the workaround. */
> -	hsw_workaround_pipe = new_crtc_state->hsw_workaround_pipe;
> +	hsw_workaround_pipe = crtc_state->hsw_workaround_pipe;
>  	if (IS_HASWELL(dev_priv) && hsw_workaround_pipe != INVALID_PIPE) {
>  		struct intel_crtc *wa_crtc;
>  
> @@ -2007,6 +1992,29 @@ static void hsw_crtc_enable(struct intel_atomic_state *state,
>  	}
>  }
>  
> +static void hsw_crtc_enable(struct intel_atomic_state *state,
> +			    struct intel_crtc *crtc)
> +{
> +	const struct intel_crtc_state *new_crtc_state =
> +		intel_atomic_get_new_crtc_state(state, crtc);
> +	struct intel_crtc *slave_crtc;
> +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> +
> +	if (drm_WARN_ON(&dev_priv->drm, crtc->active))
> +		return;
> +
> +	hsw_crtc_pre_pll_enable(state, new_crtc_state);
> +
> +	for_each_intel_crtc_in_pipe_mask(&dev_priv->drm, slave_crtc,
> +					 intel_crtc_bigjoiner_slave_pipes(new_crtc_state)) {
> +		struct intel_crtc_state *slave_crtc_state =
> +			intel_atomic_get_new_crtc_state(state, slave_crtc);
> +
> +		hsw_crtc_post_pll_enable(state, slave_crtc_state);
> +	}
> +	hsw_crtc_post_pll_enable(state, new_crtc_state);
> +}
> +
>  void ilk_pfit_disable(const struct intel_crtc_state *old_crtc_state)
>  {
>  	struct intel_crtc *crtc = to_intel_crtc(old_crtc_state->uapi.crtc);
> @@ -8122,11 +8130,11 @@ static void intel_enable_crtc(struct intel_atomic_state *state,
>  
>  	intel_crtc_update_active_timings(new_crtc_state);
>  
> -	dev_priv->display->crtc_enable(state, crtc);
> -
>  	if (intel_crtc_is_bigjoiner_slave(new_crtc_state))
>  		return;
>  
> +	dev_priv->display->crtc_enable(state, crtc);
> +
>  	intel_drrs_enable(new_crtc_state);
>  
>  	/* vblanks work again, re-enable pipe CRC. */
> @@ -8360,8 +8368,7 @@ static void skl_commit_modeset_enables(struct intel_atomic_state *state)
>  			continue;
>  
>  		if (intel_dp_mst_is_slave_trans(new_crtc_state) ||
> -		    is_trans_port_sync_master(new_crtc_state) ||
> -		    intel_crtc_is_bigjoiner_master(new_crtc_state))
> +		    is_trans_port_sync_master(new_crtc_state))
>  			continue;
>  
>  		modeset_pipes &= ~BIT(pipe);
> @@ -8371,7 +8378,7 @@ static void skl_commit_modeset_enables(struct intel_atomic_state *state)
>  
>  	/*
>  	 * Then we enable all remaining pipes that depend on other
> -	 * pipes: MST slaves and port sync masters, big joiner master
> +	 * pipes: MST slaves and port sync masters
>  	 */
>  	for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state, i) {
>  		enum pipe pipe = crtc->pipe;

-- 
Jani Nikula, Intel Open Source Graphics Center

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [Intel-gfx] [PATCH] drm/i915/display/: Refactor hsw_crtc_enable for bigjoiner cleanup
  2022-03-16  7:48 ` [Intel-gfx] [PATCH] " Jani Nikula
@ 2022-03-16 17:10   ` Navare, Manasi
  2022-03-17 15:35     ` Jani Nikula
  0 siblings, 1 reply; 14+ messages in thread
From: Navare, Manasi @ 2022-03-16 17:10 UTC (permalink / raw)
  To: Jani Nikula; +Cc: intel-gfx

On Wed, Mar 16, 2022 at 09:48:17AM +0200, Jani Nikula wrote:
> On Tue, 15 Mar 2022, Manasi Navare <manasi.d.navare@intel.com> wrote:
> > This patch abstracts pieces of hsw_crtc_enable corresponding to different
> > Bspec enable sequence steps into separate functions.
> > This helps to call them in a specific order for bigjoiner master/slave
> > in a cleaner fashion.
> 
> So does this contain only refactoring or functional changes also? One or
> the other at a time, please, not both.

No this is only refactor, no functional changes here

> 
> Also looks like hsw_crtc_* have accumulated just way too many checks for
> platforms instead of having a clean break at e.g. display 9 and/or 11.

These checks were already part of hsw_crtc_enable() function that I have just moved to a separate
function. How do you recommend removing these checks?

> 
> Adding references to "enable sequence step 8" is not helpful because
> it's platform specific. (Yeah, I know there are existing references like
> this, but they're also suspect.)

Yes agree, may be I will add the comment for what actually the step 7/8 does?

Manasi

> 
> 
> BR,
> Jani.
> 
> 
> >
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: Animesh Manna <animesh.manna@intel.com>
> > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_display.c | 125 ++++++++++---------
> >  1 file changed, 66 insertions(+), 59 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > index eb49973621f0..d8e6466c9fa0 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -1865,24 +1865,6 @@ static void hsw_set_frame_start_delay(const struct intel_crtc_state *crtc_state)
> >  	intel_de_write(dev_priv, reg, val);
> >  }
> >  
> > -static void icl_ddi_bigjoiner_pre_enable(struct intel_atomic_state *state,
> > -					 const struct intel_crtc_state *crtc_state)
> > -{
> > -	struct intel_crtc *master_crtc = intel_master_crtc(crtc_state);
> > -
> > -	/*
> > -	 * Enable sequence steps 1-7 on bigjoiner master
> > -	 */
> > -	if (intel_crtc_is_bigjoiner_slave(crtc_state))
> > -		intel_encoders_pre_pll_enable(state, master_crtc);
> > -
> > -	if (crtc_state->shared_dpll)
> > -		intel_enable_shared_dpll(crtc_state);
> > -
> > -	if (intel_crtc_is_bigjoiner_slave(crtc_state))
> > -		intel_encoders_pre_enable(state, master_crtc);
> > -}
> > -
> >  static void hsw_configure_cpu_transcoder(const struct intel_crtc_state *crtc_state)
> >  {
> >  	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> > @@ -1910,70 +1892,73 @@ static void hsw_configure_cpu_transcoder(const struct intel_crtc_state *crtc_sta
> >  	hsw_set_transconf(crtc_state);
> >  }
> >  
> > -static void hsw_crtc_enable(struct intel_atomic_state *state,
> > -			    struct intel_crtc *crtc)
> > +static void hsw_crtc_pre_pll_enable(struct intel_atomic_state *state,
> > +				    const struct intel_crtc_state *crtc_state)
> >  {
> > -	const struct intel_crtc_state *new_crtc_state =
> > -		intel_atomic_get_new_crtc_state(state, crtc);
> > -	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> > -	enum pipe pipe = crtc->pipe, hsw_workaround_pipe;
> > -	enum transcoder cpu_transcoder = new_crtc_state->cpu_transcoder;
> > -	bool psl_clkgate_wa;
> > -
> > -	if (drm_WARN_ON(&dev_priv->drm, crtc->active))
> > -		return;
> > +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> >  
> > -	if (!new_crtc_state->bigjoiner_pipes) {
> > -		intel_encoders_pre_pll_enable(state, crtc);
> > +	/*
> > +	 * Enable sequence steps 1 - 7 on all pipes
> > +	 */
> > +	intel_encoders_pre_pll_enable(state, crtc);
> > +	if (crtc_state->shared_dpll)
> > +		intel_enable_shared_dpll(crtc_state);
> >  
> > -		if (new_crtc_state->shared_dpll)
> > -			intel_enable_shared_dpll(new_crtc_state);
> > +	intel_encoders_pre_enable(state, crtc);
> > +}
> >  
> > -		intel_encoders_pre_enable(state, crtc);
> > -	} else {
> > -		icl_ddi_bigjoiner_pre_enable(state, new_crtc_state);
> > -	}
> > +static void hsw_crtc_post_pll_enable(struct intel_atomic_state *state,
> > +				     const struct intel_crtc_state *crtc_state)
> > +{
> > +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> > +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> > +	enum pipe pipe = crtc->pipe, hsw_workaround_pipe;
> > +	enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
> > +	bool psl_clkgate_wa;
> >  
> > -	intel_dsc_enable(new_crtc_state);
> > +	/*
> > +	 * Enable sequence step 8
> > +	 */
> > +	intel_dsc_enable(crtc_state);
> >  
> >  	if (DISPLAY_VER(dev_priv) >= 13)
> > -		intel_uncompressed_joiner_enable(new_crtc_state);
> > +		intel_uncompressed_joiner_enable(crtc_state);
> >  
> > -	intel_set_pipe_src_size(new_crtc_state);
> > +	intel_set_pipe_src_size(crtc_state);
> >  	if (DISPLAY_VER(dev_priv) >= 9 || IS_BROADWELL(dev_priv))
> > -		bdw_set_pipemisc(new_crtc_state);
> > +		bdw_set_pipemisc(crtc_state);
> >  
> > -	if (!intel_crtc_is_bigjoiner_slave(new_crtc_state) &&
> > +	if (!intel_crtc_is_bigjoiner_slave(crtc_state) &&
> >  	    !transcoder_is_dsi(cpu_transcoder))
> > -		hsw_configure_cpu_transcoder(new_crtc_state);
> > +		hsw_configure_cpu_transcoder(crtc_state);
> >  
> >  	crtc->active = true;
> >  
> >  	/* Display WA #1180: WaDisableScalarClockGating: glk */
> >  	psl_clkgate_wa = DISPLAY_VER(dev_priv) == 10 &&
> > -		new_crtc_state->pch_pfit.enabled;
> > +		crtc_state->pch_pfit.enabled;
> >  	if (psl_clkgate_wa)
> >  		glk_pipe_scaler_clock_gating_wa(dev_priv, pipe, true);
> >  
> >  	if (DISPLAY_VER(dev_priv) >= 9)
> > -		skl_pfit_enable(new_crtc_state);
> > +		skl_pfit_enable(crtc_state);
> >  	else
> > -		ilk_pfit_enable(new_crtc_state);
> > +		ilk_pfit_enable(crtc_state);
> >  
> >  	/*
> >  	 * On ILK+ LUT must be loaded before the pipe is running but with
> >  	 * clocks enabled
> >  	 */
> > -	intel_color_load_luts(new_crtc_state);
> > -	intel_color_commit(new_crtc_state);
> > +	intel_color_load_luts(crtc_state);
> > +	intel_color_commit(crtc_state);
> >  	/* update DSPCNTR to configure gamma/csc for pipe bottom color */
> >  	if (DISPLAY_VER(dev_priv) < 9)
> > -		intel_disable_primary_plane(new_crtc_state);
> > +		intel_disable_primary_plane(crtc_state);
> >  
> > -	hsw_set_linetime_wm(new_crtc_state);
> > +	hsw_set_linetime_wm(crtc_state);
> >  
> >  	if (DISPLAY_VER(dev_priv) >= 11)
> > -		icl_set_pipe_chicken(new_crtc_state);
> > +		icl_set_pipe_chicken(crtc_state);
> >  
> >  	intel_initial_watermarks(state, crtc);
> >  
> > @@ -1984,8 +1969,8 @@ static void hsw_crtc_enable(struct intel_atomic_state *state,
> >  		icl_pipe_mbus_enable(crtc, dbuf_state->joined_mbus);
> >  	}
> >  
> > -	if (intel_crtc_is_bigjoiner_slave(new_crtc_state))
> > -		intel_crtc_vblank_on(new_crtc_state);
> > +	if (intel_crtc_is_bigjoiner_slave(crtc_state))
> > +		intel_crtc_vblank_on(crtc_state);
> >  
> >  	intel_encoders_enable(state, crtc);
> >  
> > @@ -1996,7 +1981,7 @@ static void hsw_crtc_enable(struct intel_atomic_state *state,
> >  
> >  	/* If we change the relative order between pipe/planes enabling, we need
> >  	 * to change the workaround. */
> > -	hsw_workaround_pipe = new_crtc_state->hsw_workaround_pipe;
> > +	hsw_workaround_pipe = crtc_state->hsw_workaround_pipe;
> >  	if (IS_HASWELL(dev_priv) && hsw_workaround_pipe != INVALID_PIPE) {
> >  		struct intel_crtc *wa_crtc;
> >  
> > @@ -2007,6 +1992,29 @@ static void hsw_crtc_enable(struct intel_atomic_state *state,
> >  	}
> >  }
> >  
> > +static void hsw_crtc_enable(struct intel_atomic_state *state,
> > +			    struct intel_crtc *crtc)
> > +{
> > +	const struct intel_crtc_state *new_crtc_state =
> > +		intel_atomic_get_new_crtc_state(state, crtc);
> > +	struct intel_crtc *slave_crtc;
> > +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> > +
> > +	if (drm_WARN_ON(&dev_priv->drm, crtc->active))
> > +		return;
> > +
> > +	hsw_crtc_pre_pll_enable(state, new_crtc_state);
> > +
> > +	for_each_intel_crtc_in_pipe_mask(&dev_priv->drm, slave_crtc,
> > +					 intel_crtc_bigjoiner_slave_pipes(new_crtc_state)) {
> > +		struct intel_crtc_state *slave_crtc_state =
> > +			intel_atomic_get_new_crtc_state(state, slave_crtc);
> > +
> > +		hsw_crtc_post_pll_enable(state, slave_crtc_state);
> > +	}
> > +	hsw_crtc_post_pll_enable(state, new_crtc_state);
> > +}
> > +
> >  void ilk_pfit_disable(const struct intel_crtc_state *old_crtc_state)
> >  {
> >  	struct intel_crtc *crtc = to_intel_crtc(old_crtc_state->uapi.crtc);
> > @@ -8122,11 +8130,11 @@ static void intel_enable_crtc(struct intel_atomic_state *state,
> >  
> >  	intel_crtc_update_active_timings(new_crtc_state);
> >  
> > -	dev_priv->display->crtc_enable(state, crtc);
> > -
> >  	if (intel_crtc_is_bigjoiner_slave(new_crtc_state))
> >  		return;
> >  
> > +	dev_priv->display->crtc_enable(state, crtc);
> > +
> >  	intel_drrs_enable(new_crtc_state);
> >  
> >  	/* vblanks work again, re-enable pipe CRC. */
> > @@ -8360,8 +8368,7 @@ static void skl_commit_modeset_enables(struct intel_atomic_state *state)
> >  			continue;
> >  
> >  		if (intel_dp_mst_is_slave_trans(new_crtc_state) ||
> > -		    is_trans_port_sync_master(new_crtc_state) ||
> > -		    intel_crtc_is_bigjoiner_master(new_crtc_state))
> > +		    is_trans_port_sync_master(new_crtc_state))
> >  			continue;
> >  
> >  		modeset_pipes &= ~BIT(pipe);
> > @@ -8371,7 +8378,7 @@ static void skl_commit_modeset_enables(struct intel_atomic_state *state)
> >  
> >  	/*
> >  	 * Then we enable all remaining pipes that depend on other
> > -	 * pipes: MST slaves and port sync masters, big joiner master
> > +	 * pipes: MST slaves and port sync masters
> >  	 */
> >  	for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state, i) {
> >  		enum pipe pipe = crtc->pipe;
> 
> -- 
> Jani Nikula, Intel Open Source Graphics Center

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [Intel-gfx] [PATCH] drm/i915/display/: Refactor hsw_crtc_enable for bigjoiner cleanup
  2022-03-16 17:10   ` Navare, Manasi
@ 2022-03-17 15:35     ` Jani Nikula
  2022-03-17 18:28       ` Navare, Manasi
  0 siblings, 1 reply; 14+ messages in thread
From: Jani Nikula @ 2022-03-17 15:35 UTC (permalink / raw)
  To: Navare, Manasi; +Cc: intel-gfx

On Wed, 16 Mar 2022, "Navare, Manasi" <manasi.d.navare@intel.com> wrote:
> On Wed, Mar 16, 2022 at 09:48:17AM +0200, Jani Nikula wrote:
>> On Tue, 15 Mar 2022, Manasi Navare <manasi.d.navare@intel.com> wrote:
>> > This patch abstracts pieces of hsw_crtc_enable corresponding to different
>> > Bspec enable sequence steps into separate functions.
>> > This helps to call them in a specific order for bigjoiner master/slave
>> > in a cleaner fashion.
>> 
>> So does this contain only refactoring or functional changes also? One or
>> the other at a time, please, not both.
>
> No this is only refactor, no functional changes here
>
>> 
>> Also looks like hsw_crtc_* have accumulated just way too many checks for
>> platforms instead of having a clean break at e.g. display 9 and/or 11.
>
> These checks were already part of hsw_crtc_enable() function that I have just moved to a separate
> function. How do you recommend removing these checks?

We use hsw_crtc_enable for all DDI platforms and later. We do have the
difference between skl_display_funcs and ddi_display_funcs, but they
both point to hsw_crtc_enable. Maybe it's time for them to have their
own functions that don't have to take so many platform differences into
account.

BR,
Jani.




>
>> 
>> Adding references to "enable sequence step 8" is not helpful because
>> it's platform specific. (Yeah, I know there are existing references like
>> this, but they're also suspect.)
>
> Yes agree, may be I will add the comment for what actually the step 7/8 does?
>
> Manasi
>
>> 
>> 
>> BR,
>> Jani.
>> 
>> 
>> >
>> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> > Cc: Animesh Manna <animesh.manna@intel.com>
>> > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
>> > ---
>> >  drivers/gpu/drm/i915/display/intel_display.c | 125 ++++++++++---------
>> >  1 file changed, 66 insertions(+), 59 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
>> > index eb49973621f0..d8e6466c9fa0 100644
>> > --- a/drivers/gpu/drm/i915/display/intel_display.c
>> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
>> > @@ -1865,24 +1865,6 @@ static void hsw_set_frame_start_delay(const struct intel_crtc_state *crtc_state)
>> >  	intel_de_write(dev_priv, reg, val);
>> >  }
>> >  
>> > -static void icl_ddi_bigjoiner_pre_enable(struct intel_atomic_state *state,
>> > -					 const struct intel_crtc_state *crtc_state)
>> > -{
>> > -	struct intel_crtc *master_crtc = intel_master_crtc(crtc_state);
>> > -
>> > -	/*
>> > -	 * Enable sequence steps 1-7 on bigjoiner master
>> > -	 */
>> > -	if (intel_crtc_is_bigjoiner_slave(crtc_state))
>> > -		intel_encoders_pre_pll_enable(state, master_crtc);
>> > -
>> > -	if (crtc_state->shared_dpll)
>> > -		intel_enable_shared_dpll(crtc_state);
>> > -
>> > -	if (intel_crtc_is_bigjoiner_slave(crtc_state))
>> > -		intel_encoders_pre_enable(state, master_crtc);
>> > -}
>> > -
>> >  static void hsw_configure_cpu_transcoder(const struct intel_crtc_state *crtc_state)
>> >  {
>> >  	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
>> > @@ -1910,70 +1892,73 @@ static void hsw_configure_cpu_transcoder(const struct intel_crtc_state *crtc_sta
>> >  	hsw_set_transconf(crtc_state);
>> >  }
>> >  
>> > -static void hsw_crtc_enable(struct intel_atomic_state *state,
>> > -			    struct intel_crtc *crtc)
>> > +static void hsw_crtc_pre_pll_enable(struct intel_atomic_state *state,
>> > +				    const struct intel_crtc_state *crtc_state)
>> >  {
>> > -	const struct intel_crtc_state *new_crtc_state =
>> > -		intel_atomic_get_new_crtc_state(state, crtc);
>> > -	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>> > -	enum pipe pipe = crtc->pipe, hsw_workaround_pipe;
>> > -	enum transcoder cpu_transcoder = new_crtc_state->cpu_transcoder;
>> > -	bool psl_clkgate_wa;
>> > -
>> > -	if (drm_WARN_ON(&dev_priv->drm, crtc->active))
>> > -		return;
>> > +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
>> >  
>> > -	if (!new_crtc_state->bigjoiner_pipes) {
>> > -		intel_encoders_pre_pll_enable(state, crtc);
>> > +	/*
>> > +	 * Enable sequence steps 1 - 7 on all pipes
>> > +	 */
>> > +	intel_encoders_pre_pll_enable(state, crtc);
>> > +	if (crtc_state->shared_dpll)
>> > +		intel_enable_shared_dpll(crtc_state);
>> >  
>> > -		if (new_crtc_state->shared_dpll)
>> > -			intel_enable_shared_dpll(new_crtc_state);
>> > +	intel_encoders_pre_enable(state, crtc);
>> > +}
>> >  
>> > -		intel_encoders_pre_enable(state, crtc);
>> > -	} else {
>> > -		icl_ddi_bigjoiner_pre_enable(state, new_crtc_state);
>> > -	}
>> > +static void hsw_crtc_post_pll_enable(struct intel_atomic_state *state,
>> > +				     const struct intel_crtc_state *crtc_state)
>> > +{
>> > +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
>> > +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>> > +	enum pipe pipe = crtc->pipe, hsw_workaround_pipe;
>> > +	enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
>> > +	bool psl_clkgate_wa;
>> >  
>> > -	intel_dsc_enable(new_crtc_state);
>> > +	/*
>> > +	 * Enable sequence step 8
>> > +	 */
>> > +	intel_dsc_enable(crtc_state);
>> >  
>> >  	if (DISPLAY_VER(dev_priv) >= 13)
>> > -		intel_uncompressed_joiner_enable(new_crtc_state);
>> > +		intel_uncompressed_joiner_enable(crtc_state);
>> >  
>> > -	intel_set_pipe_src_size(new_crtc_state);
>> > +	intel_set_pipe_src_size(crtc_state);
>> >  	if (DISPLAY_VER(dev_priv) >= 9 || IS_BROADWELL(dev_priv))
>> > -		bdw_set_pipemisc(new_crtc_state);
>> > +		bdw_set_pipemisc(crtc_state);
>> >  
>> > -	if (!intel_crtc_is_bigjoiner_slave(new_crtc_state) &&
>> > +	if (!intel_crtc_is_bigjoiner_slave(crtc_state) &&
>> >  	    !transcoder_is_dsi(cpu_transcoder))
>> > -		hsw_configure_cpu_transcoder(new_crtc_state);
>> > +		hsw_configure_cpu_transcoder(crtc_state);
>> >  
>> >  	crtc->active = true;
>> >  
>> >  	/* Display WA #1180: WaDisableScalarClockGating: glk */
>> >  	psl_clkgate_wa = DISPLAY_VER(dev_priv) == 10 &&
>> > -		new_crtc_state->pch_pfit.enabled;
>> > +		crtc_state->pch_pfit.enabled;
>> >  	if (psl_clkgate_wa)
>> >  		glk_pipe_scaler_clock_gating_wa(dev_priv, pipe, true);
>> >  
>> >  	if (DISPLAY_VER(dev_priv) >= 9)
>> > -		skl_pfit_enable(new_crtc_state);
>> > +		skl_pfit_enable(crtc_state);
>> >  	else
>> > -		ilk_pfit_enable(new_crtc_state);
>> > +		ilk_pfit_enable(crtc_state);
>> >  
>> >  	/*
>> >  	 * On ILK+ LUT must be loaded before the pipe is running but with
>> >  	 * clocks enabled
>> >  	 */
>> > -	intel_color_load_luts(new_crtc_state);
>> > -	intel_color_commit(new_crtc_state);
>> > +	intel_color_load_luts(crtc_state);
>> > +	intel_color_commit(crtc_state);
>> >  	/* update DSPCNTR to configure gamma/csc for pipe bottom color */
>> >  	if (DISPLAY_VER(dev_priv) < 9)
>> > -		intel_disable_primary_plane(new_crtc_state);
>> > +		intel_disable_primary_plane(crtc_state);
>> >  
>> > -	hsw_set_linetime_wm(new_crtc_state);
>> > +	hsw_set_linetime_wm(crtc_state);
>> >  
>> >  	if (DISPLAY_VER(dev_priv) >= 11)
>> > -		icl_set_pipe_chicken(new_crtc_state);
>> > +		icl_set_pipe_chicken(crtc_state);
>> >  
>> >  	intel_initial_watermarks(state, crtc);
>> >  
>> > @@ -1984,8 +1969,8 @@ static void hsw_crtc_enable(struct intel_atomic_state *state,
>> >  		icl_pipe_mbus_enable(crtc, dbuf_state->joined_mbus);
>> >  	}
>> >  
>> > -	if (intel_crtc_is_bigjoiner_slave(new_crtc_state))
>> > -		intel_crtc_vblank_on(new_crtc_state);
>> > +	if (intel_crtc_is_bigjoiner_slave(crtc_state))
>> > +		intel_crtc_vblank_on(crtc_state);
>> >  
>> >  	intel_encoders_enable(state, crtc);
>> >  
>> > @@ -1996,7 +1981,7 @@ static void hsw_crtc_enable(struct intel_atomic_state *state,
>> >  
>> >  	/* If we change the relative order between pipe/planes enabling, we need
>> >  	 * to change the workaround. */
>> > -	hsw_workaround_pipe = new_crtc_state->hsw_workaround_pipe;
>> > +	hsw_workaround_pipe = crtc_state->hsw_workaround_pipe;
>> >  	if (IS_HASWELL(dev_priv) && hsw_workaround_pipe != INVALID_PIPE) {
>> >  		struct intel_crtc *wa_crtc;
>> >  
>> > @@ -2007,6 +1992,29 @@ static void hsw_crtc_enable(struct intel_atomic_state *state,
>> >  	}
>> >  }
>> >  
>> > +static void hsw_crtc_enable(struct intel_atomic_state *state,
>> > +			    struct intel_crtc *crtc)
>> > +{
>> > +	const struct intel_crtc_state *new_crtc_state =
>> > +		intel_atomic_get_new_crtc_state(state, crtc);
>> > +	struct intel_crtc *slave_crtc;
>> > +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>> > +
>> > +	if (drm_WARN_ON(&dev_priv->drm, crtc->active))
>> > +		return;
>> > +
>> > +	hsw_crtc_pre_pll_enable(state, new_crtc_state);
>> > +
>> > +	for_each_intel_crtc_in_pipe_mask(&dev_priv->drm, slave_crtc,
>> > +					 intel_crtc_bigjoiner_slave_pipes(new_crtc_state)) {
>> > +		struct intel_crtc_state *slave_crtc_state =
>> > +			intel_atomic_get_new_crtc_state(state, slave_crtc);
>> > +
>> > +		hsw_crtc_post_pll_enable(state, slave_crtc_state);
>> > +	}
>> > +	hsw_crtc_post_pll_enable(state, new_crtc_state);
>> > +}
>> > +
>> >  void ilk_pfit_disable(const struct intel_crtc_state *old_crtc_state)
>> >  {
>> >  	struct intel_crtc *crtc = to_intel_crtc(old_crtc_state->uapi.crtc);
>> > @@ -8122,11 +8130,11 @@ static void intel_enable_crtc(struct intel_atomic_state *state,
>> >  
>> >  	intel_crtc_update_active_timings(new_crtc_state);
>> >  
>> > -	dev_priv->display->crtc_enable(state, crtc);
>> > -
>> >  	if (intel_crtc_is_bigjoiner_slave(new_crtc_state))
>> >  		return;
>> >  
>> > +	dev_priv->display->crtc_enable(state, crtc);
>> > +
>> >  	intel_drrs_enable(new_crtc_state);
>> >  
>> >  	/* vblanks work again, re-enable pipe CRC. */
>> > @@ -8360,8 +8368,7 @@ static void skl_commit_modeset_enables(struct intel_atomic_state *state)
>> >  			continue;
>> >  
>> >  		if (intel_dp_mst_is_slave_trans(new_crtc_state) ||
>> > -		    is_trans_port_sync_master(new_crtc_state) ||
>> > -		    intel_crtc_is_bigjoiner_master(new_crtc_state))
>> > +		    is_trans_port_sync_master(new_crtc_state))
>> >  			continue;
>> >  
>> >  		modeset_pipes &= ~BIT(pipe);
>> > @@ -8371,7 +8378,7 @@ static void skl_commit_modeset_enables(struct intel_atomic_state *state)
>> >  
>> >  	/*
>> >  	 * Then we enable all remaining pipes that depend on other
>> > -	 * pipes: MST slaves and port sync masters, big joiner master
>> > +	 * pipes: MST slaves and port sync masters
>> >  	 */
>> >  	for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state, i) {
>> >  		enum pipe pipe = crtc->pipe;
>> 
>> -- 
>> Jani Nikula, Intel Open Source Graphics Center

-- 
Jani Nikula, Intel Open Source Graphics Center

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [Intel-gfx] [PATCH] drm/i915/display/: Refactor hsw_crtc_enable for bigjoiner cleanup
  2022-03-17 15:35     ` Jani Nikula
@ 2022-03-17 18:28       ` Navare, Manasi
  2022-03-17 18:31         ` Navare, Manasi
  0 siblings, 1 reply; 14+ messages in thread
From: Navare, Manasi @ 2022-03-17 18:28 UTC (permalink / raw)
  To: Jani Nikula; +Cc: intel-gfx

On Thu, Mar 17, 2022 at 05:35:43PM +0200, Jani Nikula wrote:
> On Wed, 16 Mar 2022, "Navare, Manasi" <manasi.d.navare@intel.com> wrote:
> > On Wed, Mar 16, 2022 at 09:48:17AM +0200, Jani Nikula wrote:
> >> On Tue, 15 Mar 2022, Manasi Navare <manasi.d.navare@intel.com> wrote:
> >> > This patch abstracts pieces of hsw_crtc_enable corresponding to different
> >> > Bspec enable sequence steps into separate functions.
> >> > This helps to call them in a specific order for bigjoiner master/slave
> >> > in a cleaner fashion.
> >> 
> >> So does this contain only refactoring or functional changes also? One or
> >> the other at a time, please, not both.
> >
> > No this is only refactor, no functional changes here
> >
> >> 
> >> Also looks like hsw_crtc_* have accumulated just way too many checks for
> >> platforms instead of having a clean break at e.g. display 9 and/or 11.
> >
> > These checks were already part of hsw_crtc_enable() function that I have just moved to a separate
> > function. How do you recommend removing these checks?
> 
> We use hsw_crtc_enable for all DDI platforms and later. We do have the
> difference between skl_display_funcs and ddi_display_funcs, but they
> both point to hsw_crtc_enable. Maybe it's time for them to have their
> own functions that don't have to take so many platform differences into
> account.

Yes I could perhaps have a separate skl_crtc_enable() function which would then have all parts that apply to DISPLAy >=9
everything else can stay in hsw_crtc_enable

That makes sense?

Manasi

> 
> BR,
> Jani.
> 
> 
> 
> 
> >
> >> 
> >> Adding references to "enable sequence step 8" is not helpful because
> >> it's platform specific. (Yeah, I know there are existing references like
> >> this, but they're also suspect.)
> >
> > Yes agree, may be I will add the comment for what actually the step 7/8 does?
> >
> > Manasi
> >
> >> 
> >> 
> >> BR,
> >> Jani.
> >> 
> >> 
> >> >
> >> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >> > Cc: Animesh Manna <animesh.manna@intel.com>
> >> > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> >> > ---
> >> >  drivers/gpu/drm/i915/display/intel_display.c | 125 ++++++++++---------
> >> >  1 file changed, 66 insertions(+), 59 deletions(-)
> >> >
> >> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> >> > index eb49973621f0..d8e6466c9fa0 100644
> >> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> >> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> >> > @@ -1865,24 +1865,6 @@ static void hsw_set_frame_start_delay(const struct intel_crtc_state *crtc_state)
> >> >  	intel_de_write(dev_priv, reg, val);
> >> >  }
> >> >  
> >> > -static void icl_ddi_bigjoiner_pre_enable(struct intel_atomic_state *state,
> >> > -					 const struct intel_crtc_state *crtc_state)
> >> > -{
> >> > -	struct intel_crtc *master_crtc = intel_master_crtc(crtc_state);
> >> > -
> >> > -	/*
> >> > -	 * Enable sequence steps 1-7 on bigjoiner master
> >> > -	 */
> >> > -	if (intel_crtc_is_bigjoiner_slave(crtc_state))
> >> > -		intel_encoders_pre_pll_enable(state, master_crtc);
> >> > -
> >> > -	if (crtc_state->shared_dpll)
> >> > -		intel_enable_shared_dpll(crtc_state);
> >> > -
> >> > -	if (intel_crtc_is_bigjoiner_slave(crtc_state))
> >> > -		intel_encoders_pre_enable(state, master_crtc);
> >> > -}
> >> > -
> >> >  static void hsw_configure_cpu_transcoder(const struct intel_crtc_state *crtc_state)
> >> >  {
> >> >  	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> >> > @@ -1910,70 +1892,73 @@ static void hsw_configure_cpu_transcoder(const struct intel_crtc_state *crtc_sta
> >> >  	hsw_set_transconf(crtc_state);
> >> >  }
> >> >  
> >> > -static void hsw_crtc_enable(struct intel_atomic_state *state,
> >> > -			    struct intel_crtc *crtc)
> >> > +static void hsw_crtc_pre_pll_enable(struct intel_atomic_state *state,
> >> > +				    const struct intel_crtc_state *crtc_state)
> >> >  {
> >> > -	const struct intel_crtc_state *new_crtc_state =
> >> > -		intel_atomic_get_new_crtc_state(state, crtc);
> >> > -	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> >> > -	enum pipe pipe = crtc->pipe, hsw_workaround_pipe;
> >> > -	enum transcoder cpu_transcoder = new_crtc_state->cpu_transcoder;
> >> > -	bool psl_clkgate_wa;
> >> > -
> >> > -	if (drm_WARN_ON(&dev_priv->drm, crtc->active))
> >> > -		return;
> >> > +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> >> >  
> >> > -	if (!new_crtc_state->bigjoiner_pipes) {
> >> > -		intel_encoders_pre_pll_enable(state, crtc);
> >> > +	/*
> >> > +	 * Enable sequence steps 1 - 7 on all pipes
> >> > +	 */
> >> > +	intel_encoders_pre_pll_enable(state, crtc);
> >> > +	if (crtc_state->shared_dpll)
> >> > +		intel_enable_shared_dpll(crtc_state);
> >> >  
> >> > -		if (new_crtc_state->shared_dpll)
> >> > -			intel_enable_shared_dpll(new_crtc_state);
> >> > +	intel_encoders_pre_enable(state, crtc);
> >> > +}
> >> >  
> >> > -		intel_encoders_pre_enable(state, crtc);
> >> > -	} else {
> >> > -		icl_ddi_bigjoiner_pre_enable(state, new_crtc_state);
> >> > -	}
> >> > +static void hsw_crtc_post_pll_enable(struct intel_atomic_state *state,
> >> > +				     const struct intel_crtc_state *crtc_state)
> >> > +{
> >> > +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> >> > +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> >> > +	enum pipe pipe = crtc->pipe, hsw_workaround_pipe;
> >> > +	enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
> >> > +	bool psl_clkgate_wa;
> >> >  
> >> > -	intel_dsc_enable(new_crtc_state);
> >> > +	/*
> >> > +	 * Enable sequence step 8
> >> > +	 */
> >> > +	intel_dsc_enable(crtc_state);
> >> >  
> >> >  	if (DISPLAY_VER(dev_priv) >= 13)
> >> > -		intel_uncompressed_joiner_enable(new_crtc_state);
> >> > +		intel_uncompressed_joiner_enable(crtc_state);
> >> >  
> >> > -	intel_set_pipe_src_size(new_crtc_state);
> >> > +	intel_set_pipe_src_size(crtc_state);
> >> >  	if (DISPLAY_VER(dev_priv) >= 9 || IS_BROADWELL(dev_priv))
> >> > -		bdw_set_pipemisc(new_crtc_state);
> >> > +		bdw_set_pipemisc(crtc_state);
> >> >  
> >> > -	if (!intel_crtc_is_bigjoiner_slave(new_crtc_state) &&
> >> > +	if (!intel_crtc_is_bigjoiner_slave(crtc_state) &&
> >> >  	    !transcoder_is_dsi(cpu_transcoder))
> >> > -		hsw_configure_cpu_transcoder(new_crtc_state);
> >> > +		hsw_configure_cpu_transcoder(crtc_state);
> >> >  
> >> >  	crtc->active = true;
> >> >  
> >> >  	/* Display WA #1180: WaDisableScalarClockGating: glk */
> >> >  	psl_clkgate_wa = DISPLAY_VER(dev_priv) == 10 &&
> >> > -		new_crtc_state->pch_pfit.enabled;
> >> > +		crtc_state->pch_pfit.enabled;
> >> >  	if (psl_clkgate_wa)
> >> >  		glk_pipe_scaler_clock_gating_wa(dev_priv, pipe, true);
> >> >  
> >> >  	if (DISPLAY_VER(dev_priv) >= 9)
> >> > -		skl_pfit_enable(new_crtc_state);
> >> > +		skl_pfit_enable(crtc_state);
> >> >  	else
> >> > -		ilk_pfit_enable(new_crtc_state);
> >> > +		ilk_pfit_enable(crtc_state);
> >> >  
> >> >  	/*
> >> >  	 * On ILK+ LUT must be loaded before the pipe is running but with
> >> >  	 * clocks enabled
> >> >  	 */
> >> > -	intel_color_load_luts(new_crtc_state);
> >> > -	intel_color_commit(new_crtc_state);
> >> > +	intel_color_load_luts(crtc_state);
> >> > +	intel_color_commit(crtc_state);
> >> >  	/* update DSPCNTR to configure gamma/csc for pipe bottom color */
> >> >  	if (DISPLAY_VER(dev_priv) < 9)
> >> > -		intel_disable_primary_plane(new_crtc_state);
> >> > +		intel_disable_primary_plane(crtc_state);
> >> >  
> >> > -	hsw_set_linetime_wm(new_crtc_state);
> >> > +	hsw_set_linetime_wm(crtc_state);
> >> >  
> >> >  	if (DISPLAY_VER(dev_priv) >= 11)
> >> > -		icl_set_pipe_chicken(new_crtc_state);
> >> > +		icl_set_pipe_chicken(crtc_state);
> >> >  
> >> >  	intel_initial_watermarks(state, crtc);
> >> >  
> >> > @@ -1984,8 +1969,8 @@ static void hsw_crtc_enable(struct intel_atomic_state *state,
> >> >  		icl_pipe_mbus_enable(crtc, dbuf_state->joined_mbus);
> >> >  	}
> >> >  
> >> > -	if (intel_crtc_is_bigjoiner_slave(new_crtc_state))
> >> > -		intel_crtc_vblank_on(new_crtc_state);
> >> > +	if (intel_crtc_is_bigjoiner_slave(crtc_state))
> >> > +		intel_crtc_vblank_on(crtc_state);
> >> >  
> >> >  	intel_encoders_enable(state, crtc);
> >> >  
> >> > @@ -1996,7 +1981,7 @@ static void hsw_crtc_enable(struct intel_atomic_state *state,
> >> >  
> >> >  	/* If we change the relative order between pipe/planes enabling, we need
> >> >  	 * to change the workaround. */
> >> > -	hsw_workaround_pipe = new_crtc_state->hsw_workaround_pipe;
> >> > +	hsw_workaround_pipe = crtc_state->hsw_workaround_pipe;
> >> >  	if (IS_HASWELL(dev_priv) && hsw_workaround_pipe != INVALID_PIPE) {
> >> >  		struct intel_crtc *wa_crtc;
> >> >  
> >> > @@ -2007,6 +1992,29 @@ static void hsw_crtc_enable(struct intel_atomic_state *state,
> >> >  	}
> >> >  }
> >> >  
> >> > +static void hsw_crtc_enable(struct intel_atomic_state *state,
> >> > +			    struct intel_crtc *crtc)
> >> > +{
> >> > +	const struct intel_crtc_state *new_crtc_state =
> >> > +		intel_atomic_get_new_crtc_state(state, crtc);
> >> > +	struct intel_crtc *slave_crtc;
> >> > +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> >> > +
> >> > +	if (drm_WARN_ON(&dev_priv->drm, crtc->active))
> >> > +		return;
> >> > +
> >> > +	hsw_crtc_pre_pll_enable(state, new_crtc_state);
> >> > +
> >> > +	for_each_intel_crtc_in_pipe_mask(&dev_priv->drm, slave_crtc,
> >> > +					 intel_crtc_bigjoiner_slave_pipes(new_crtc_state)) {
> >> > +		struct intel_crtc_state *slave_crtc_state =
> >> > +			intel_atomic_get_new_crtc_state(state, slave_crtc);
> >> > +
> >> > +		hsw_crtc_post_pll_enable(state, slave_crtc_state);
> >> > +	}
> >> > +	hsw_crtc_post_pll_enable(state, new_crtc_state);
> >> > +}
> >> > +
> >> >  void ilk_pfit_disable(const struct intel_crtc_state *old_crtc_state)
> >> >  {
> >> >  	struct intel_crtc *crtc = to_intel_crtc(old_crtc_state->uapi.crtc);
> >> > @@ -8122,11 +8130,11 @@ static void intel_enable_crtc(struct intel_atomic_state *state,
> >> >  
> >> >  	intel_crtc_update_active_timings(new_crtc_state);
> >> >  
> >> > -	dev_priv->display->crtc_enable(state, crtc);
> >> > -
> >> >  	if (intel_crtc_is_bigjoiner_slave(new_crtc_state))
> >> >  		return;
> >> >  
> >> > +	dev_priv->display->crtc_enable(state, crtc);
> >> > +
> >> >  	intel_drrs_enable(new_crtc_state);
> >> >  
> >> >  	/* vblanks work again, re-enable pipe CRC. */
> >> > @@ -8360,8 +8368,7 @@ static void skl_commit_modeset_enables(struct intel_atomic_state *state)
> >> >  			continue;
> >> >  
> >> >  		if (intel_dp_mst_is_slave_trans(new_crtc_state) ||
> >> > -		    is_trans_port_sync_master(new_crtc_state) ||
> >> > -		    intel_crtc_is_bigjoiner_master(new_crtc_state))
> >> > +		    is_trans_port_sync_master(new_crtc_state))
> >> >  			continue;
> >> >  
> >> >  		modeset_pipes &= ~BIT(pipe);
> >> > @@ -8371,7 +8378,7 @@ static void skl_commit_modeset_enables(struct intel_atomic_state *state)
> >> >  
> >> >  	/*
> >> >  	 * Then we enable all remaining pipes that depend on other
> >> > -	 * pipes: MST slaves and port sync masters, big joiner master
> >> > +	 * pipes: MST slaves and port sync masters
> >> >  	 */
> >> >  	for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state, i) {
> >> >  		enum pipe pipe = crtc->pipe;
> >> 
> >> -- 
> >> Jani Nikula, Intel Open Source Graphics Center
> 
> -- 
> Jani Nikula, Intel Open Source Graphics Center

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [Intel-gfx] [PATCH] drm/i915/display/: Refactor hsw_crtc_enable for bigjoiner cleanup
  2022-03-17 18:28       ` Navare, Manasi
@ 2022-03-17 18:31         ` Navare, Manasi
  2022-03-17 19:15           ` Jani Nikula
  0 siblings, 1 reply; 14+ messages in thread
From: Navare, Manasi @ 2022-03-17 18:31 UTC (permalink / raw)
  To: Jani Nikula; +Cc: intel-gfx

On Thu, Mar 17, 2022 at 11:28:03AM -0700, Navare, Manasi wrote:
> On Thu, Mar 17, 2022 at 05:35:43PM +0200, Jani Nikula wrote:
> > On Wed, 16 Mar 2022, "Navare, Manasi" <manasi.d.navare@intel.com> wrote:
> > > On Wed, Mar 16, 2022 at 09:48:17AM +0200, Jani Nikula wrote:
> > >> On Tue, 15 Mar 2022, Manasi Navare <manasi.d.navare@intel.com> wrote:
> > >> > This patch abstracts pieces of hsw_crtc_enable corresponding to different
> > >> > Bspec enable sequence steps into separate functions.
> > >> > This helps to call them in a specific order for bigjoiner master/slave
> > >> > in a cleaner fashion.
> > >> 
> > >> So does this contain only refactoring or functional changes also? One or
> > >> the other at a time, please, not both.
> > >
> > > No this is only refactor, no functional changes here
> > >
> > >> 
> > >> Also looks like hsw_crtc_* have accumulated just way too many checks for
> > >> platforms instead of having a clean break at e.g. display 9 and/or 11.
> > >
> > > These checks were already part of hsw_crtc_enable() function that I have just moved to a separate
> > > function. How do you recommend removing these checks?
> > 
> > We use hsw_crtc_enable for all DDI platforms and later. We do have the
> > difference between skl_display_funcs and ddi_display_funcs, but they
> > both point to hsw_crtc_enable. Maybe it's time for them to have their
> > own functions that don't have to take so many platform differences into
> > account.
> 
> Yes I could perhaps have a separate skl_crtc_enable() function which would then have all parts that apply to DISPLAy >=9
> everything else can stay in hsw_crtc_enable
> 
> That makes sense?

Or actually we could have a separate one for >=11 starting ICL, since there are a lot of
checks for >=11. Also bigjoiner support only from ICL, so then this bigjoiner sequence then only becomes part
of icl_crtc_enable()
Which one of the above 2 do you recommend?

Manasi

> 
> Manasi
> 
> > 
> > BR,
> > Jani.
> > 
> > 
> > 
> > 
> > >
> > >> 
> > >> Adding references to "enable sequence step 8" is not helpful because
> > >> it's platform specific. (Yeah, I know there are existing references like
> > >> this, but they're also suspect.)
> > >
> > > Yes agree, may be I will add the comment for what actually the step 7/8 does?
> > >
> > > Manasi
> > >
> > >> 
> > >> 
> > >> BR,
> > >> Jani.
> > >> 
> > >> 
> > >> >
> > >> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > >> > Cc: Animesh Manna <animesh.manna@intel.com>
> > >> > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > >> > ---
> > >> >  drivers/gpu/drm/i915/display/intel_display.c | 125 ++++++++++---------
> > >> >  1 file changed, 66 insertions(+), 59 deletions(-)
> > >> >
> > >> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > >> > index eb49973621f0..d8e6466c9fa0 100644
> > >> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > >> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > >> > @@ -1865,24 +1865,6 @@ static void hsw_set_frame_start_delay(const struct intel_crtc_state *crtc_state)
> > >> >  	intel_de_write(dev_priv, reg, val);
> > >> >  }
> > >> >  
> > >> > -static void icl_ddi_bigjoiner_pre_enable(struct intel_atomic_state *state,
> > >> > -					 const struct intel_crtc_state *crtc_state)
> > >> > -{
> > >> > -	struct intel_crtc *master_crtc = intel_master_crtc(crtc_state);
> > >> > -
> > >> > -	/*
> > >> > -	 * Enable sequence steps 1-7 on bigjoiner master
> > >> > -	 */
> > >> > -	if (intel_crtc_is_bigjoiner_slave(crtc_state))
> > >> > -		intel_encoders_pre_pll_enable(state, master_crtc);
> > >> > -
> > >> > -	if (crtc_state->shared_dpll)
> > >> > -		intel_enable_shared_dpll(crtc_state);
> > >> > -
> > >> > -	if (intel_crtc_is_bigjoiner_slave(crtc_state))
> > >> > -		intel_encoders_pre_enable(state, master_crtc);
> > >> > -}
> > >> > -
> > >> >  static void hsw_configure_cpu_transcoder(const struct intel_crtc_state *crtc_state)
> > >> >  {
> > >> >  	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> > >> > @@ -1910,70 +1892,73 @@ static void hsw_configure_cpu_transcoder(const struct intel_crtc_state *crtc_sta
> > >> >  	hsw_set_transconf(crtc_state);
> > >> >  }
> > >> >  
> > >> > -static void hsw_crtc_enable(struct intel_atomic_state *state,
> > >> > -			    struct intel_crtc *crtc)
> > >> > +static void hsw_crtc_pre_pll_enable(struct intel_atomic_state *state,
> > >> > +				    const struct intel_crtc_state *crtc_state)
> > >> >  {
> > >> > -	const struct intel_crtc_state *new_crtc_state =
> > >> > -		intel_atomic_get_new_crtc_state(state, crtc);
> > >> > -	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> > >> > -	enum pipe pipe = crtc->pipe, hsw_workaround_pipe;
> > >> > -	enum transcoder cpu_transcoder = new_crtc_state->cpu_transcoder;
> > >> > -	bool psl_clkgate_wa;
> > >> > -
> > >> > -	if (drm_WARN_ON(&dev_priv->drm, crtc->active))
> > >> > -		return;
> > >> > +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> > >> >  
> > >> > -	if (!new_crtc_state->bigjoiner_pipes) {
> > >> > -		intel_encoders_pre_pll_enable(state, crtc);
> > >> > +	/*
> > >> > +	 * Enable sequence steps 1 - 7 on all pipes
> > >> > +	 */
> > >> > +	intel_encoders_pre_pll_enable(state, crtc);
> > >> > +	if (crtc_state->shared_dpll)
> > >> > +		intel_enable_shared_dpll(crtc_state);
> > >> >  
> > >> > -		if (new_crtc_state->shared_dpll)
> > >> > -			intel_enable_shared_dpll(new_crtc_state);
> > >> > +	intel_encoders_pre_enable(state, crtc);
> > >> > +}
> > >> >  
> > >> > -		intel_encoders_pre_enable(state, crtc);
> > >> > -	} else {
> > >> > -		icl_ddi_bigjoiner_pre_enable(state, new_crtc_state);
> > >> > -	}
> > >> > +static void hsw_crtc_post_pll_enable(struct intel_atomic_state *state,
> > >> > +				     const struct intel_crtc_state *crtc_state)
> > >> > +{
> > >> > +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> > >> > +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> > >> > +	enum pipe pipe = crtc->pipe, hsw_workaround_pipe;
> > >> > +	enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
> > >> > +	bool psl_clkgate_wa;
> > >> >  
> > >> > -	intel_dsc_enable(new_crtc_state);
> > >> > +	/*
> > >> > +	 * Enable sequence step 8
> > >> > +	 */
> > >> > +	intel_dsc_enable(crtc_state);
> > >> >  
> > >> >  	if (DISPLAY_VER(dev_priv) >= 13)
> > >> > -		intel_uncompressed_joiner_enable(new_crtc_state);
> > >> > +		intel_uncompressed_joiner_enable(crtc_state);
> > >> >  
> > >> > -	intel_set_pipe_src_size(new_crtc_state);
> > >> > +	intel_set_pipe_src_size(crtc_state);
> > >> >  	if (DISPLAY_VER(dev_priv) >= 9 || IS_BROADWELL(dev_priv))
> > >> > -		bdw_set_pipemisc(new_crtc_state);
> > >> > +		bdw_set_pipemisc(crtc_state);
> > >> >  
> > >> > -	if (!intel_crtc_is_bigjoiner_slave(new_crtc_state) &&
> > >> > +	if (!intel_crtc_is_bigjoiner_slave(crtc_state) &&
> > >> >  	    !transcoder_is_dsi(cpu_transcoder))
> > >> > -		hsw_configure_cpu_transcoder(new_crtc_state);
> > >> > +		hsw_configure_cpu_transcoder(crtc_state);
> > >> >  
> > >> >  	crtc->active = true;
> > >> >  
> > >> >  	/* Display WA #1180: WaDisableScalarClockGating: glk */
> > >> >  	psl_clkgate_wa = DISPLAY_VER(dev_priv) == 10 &&
> > >> > -		new_crtc_state->pch_pfit.enabled;
> > >> > +		crtc_state->pch_pfit.enabled;
> > >> >  	if (psl_clkgate_wa)
> > >> >  		glk_pipe_scaler_clock_gating_wa(dev_priv, pipe, true);
> > >> >  
> > >> >  	if (DISPLAY_VER(dev_priv) >= 9)
> > >> > -		skl_pfit_enable(new_crtc_state);
> > >> > +		skl_pfit_enable(crtc_state);
> > >> >  	else
> > >> > -		ilk_pfit_enable(new_crtc_state);
> > >> > +		ilk_pfit_enable(crtc_state);
> > >> >  
> > >> >  	/*
> > >> >  	 * On ILK+ LUT must be loaded before the pipe is running but with
> > >> >  	 * clocks enabled
> > >> >  	 */
> > >> > -	intel_color_load_luts(new_crtc_state);
> > >> > -	intel_color_commit(new_crtc_state);
> > >> > +	intel_color_load_luts(crtc_state);
> > >> > +	intel_color_commit(crtc_state);
> > >> >  	/* update DSPCNTR to configure gamma/csc for pipe bottom color */
> > >> >  	if (DISPLAY_VER(dev_priv) < 9)
> > >> > -		intel_disable_primary_plane(new_crtc_state);
> > >> > +		intel_disable_primary_plane(crtc_state);
> > >> >  
> > >> > -	hsw_set_linetime_wm(new_crtc_state);
> > >> > +	hsw_set_linetime_wm(crtc_state);
> > >> >  
> > >> >  	if (DISPLAY_VER(dev_priv) >= 11)
> > >> > -		icl_set_pipe_chicken(new_crtc_state);
> > >> > +		icl_set_pipe_chicken(crtc_state);
> > >> >  
> > >> >  	intel_initial_watermarks(state, crtc);
> > >> >  
> > >> > @@ -1984,8 +1969,8 @@ static void hsw_crtc_enable(struct intel_atomic_state *state,
> > >> >  		icl_pipe_mbus_enable(crtc, dbuf_state->joined_mbus);
> > >> >  	}
> > >> >  
> > >> > -	if (intel_crtc_is_bigjoiner_slave(new_crtc_state))
> > >> > -		intel_crtc_vblank_on(new_crtc_state);
> > >> > +	if (intel_crtc_is_bigjoiner_slave(crtc_state))
> > >> > +		intel_crtc_vblank_on(crtc_state);
> > >> >  
> > >> >  	intel_encoders_enable(state, crtc);
> > >> >  
> > >> > @@ -1996,7 +1981,7 @@ static void hsw_crtc_enable(struct intel_atomic_state *state,
> > >> >  
> > >> >  	/* If we change the relative order between pipe/planes enabling, we need
> > >> >  	 * to change the workaround. */
> > >> > -	hsw_workaround_pipe = new_crtc_state->hsw_workaround_pipe;
> > >> > +	hsw_workaround_pipe = crtc_state->hsw_workaround_pipe;
> > >> >  	if (IS_HASWELL(dev_priv) && hsw_workaround_pipe != INVALID_PIPE) {
> > >> >  		struct intel_crtc *wa_crtc;
> > >> >  
> > >> > @@ -2007,6 +1992,29 @@ static void hsw_crtc_enable(struct intel_atomic_state *state,
> > >> >  	}
> > >> >  }
> > >> >  
> > >> > +static void hsw_crtc_enable(struct intel_atomic_state *state,
> > >> > +			    struct intel_crtc *crtc)
> > >> > +{
> > >> > +	const struct intel_crtc_state *new_crtc_state =
> > >> > +		intel_atomic_get_new_crtc_state(state, crtc);
> > >> > +	struct intel_crtc *slave_crtc;
> > >> > +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> > >> > +
> > >> > +	if (drm_WARN_ON(&dev_priv->drm, crtc->active))
> > >> > +		return;
> > >> > +
> > >> > +	hsw_crtc_pre_pll_enable(state, new_crtc_state);
> > >> > +
> > >> > +	for_each_intel_crtc_in_pipe_mask(&dev_priv->drm, slave_crtc,
> > >> > +					 intel_crtc_bigjoiner_slave_pipes(new_crtc_state)) {
> > >> > +		struct intel_crtc_state *slave_crtc_state =
> > >> > +			intel_atomic_get_new_crtc_state(state, slave_crtc);
> > >> > +
> > >> > +		hsw_crtc_post_pll_enable(state, slave_crtc_state);
> > >> > +	}
> > >> > +	hsw_crtc_post_pll_enable(state, new_crtc_state);
> > >> > +}
> > >> > +
> > >> >  void ilk_pfit_disable(const struct intel_crtc_state *old_crtc_state)
> > >> >  {
> > >> >  	struct intel_crtc *crtc = to_intel_crtc(old_crtc_state->uapi.crtc);
> > >> > @@ -8122,11 +8130,11 @@ static void intel_enable_crtc(struct intel_atomic_state *state,
> > >> >  
> > >> >  	intel_crtc_update_active_timings(new_crtc_state);
> > >> >  
> > >> > -	dev_priv->display->crtc_enable(state, crtc);
> > >> > -
> > >> >  	if (intel_crtc_is_bigjoiner_slave(new_crtc_state))
> > >> >  		return;
> > >> >  
> > >> > +	dev_priv->display->crtc_enable(state, crtc);
> > >> > +
> > >> >  	intel_drrs_enable(new_crtc_state);
> > >> >  
> > >> >  	/* vblanks work again, re-enable pipe CRC. */
> > >> > @@ -8360,8 +8368,7 @@ static void skl_commit_modeset_enables(struct intel_atomic_state *state)
> > >> >  			continue;
> > >> >  
> > >> >  		if (intel_dp_mst_is_slave_trans(new_crtc_state) ||
> > >> > -		    is_trans_port_sync_master(new_crtc_state) ||
> > >> > -		    intel_crtc_is_bigjoiner_master(new_crtc_state))
> > >> > +		    is_trans_port_sync_master(new_crtc_state))
> > >> >  			continue;
> > >> >  
> > >> >  		modeset_pipes &= ~BIT(pipe);
> > >> > @@ -8371,7 +8378,7 @@ static void skl_commit_modeset_enables(struct intel_atomic_state *state)
> > >> >  
> > >> >  	/*
> > >> >  	 * Then we enable all remaining pipes that depend on other
> > >> > -	 * pipes: MST slaves and port sync masters, big joiner master
> > >> > +	 * pipes: MST slaves and port sync masters
> > >> >  	 */
> > >> >  	for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state, i) {
> > >> >  		enum pipe pipe = crtc->pipe;
> > >> 
> > >> -- 
> > >> Jani Nikula, Intel Open Source Graphics Center
> > 
> > -- 
> > Jani Nikula, Intel Open Source Graphics Center

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [Intel-gfx] [PATCH] drm/i915/display/: Refactor hsw_crtc_enable for bigjoiner cleanup
  2022-03-15 23:38 [Intel-gfx] [PATCH] drm/i915/display/: Refactor hsw_crtc_enable for bigjoiner cleanup Manasi Navare
                   ` (2 preceding siblings ...)
  2022-03-16  7:48 ` [Intel-gfx] [PATCH] " Jani Nikula
@ 2022-03-17 18:52 ` Ville Syrjälä
  2022-03-17 19:05   ` Navare, Manasi
  3 siblings, 1 reply; 14+ messages in thread
From: Ville Syrjälä @ 2022-03-17 18:52 UTC (permalink / raw)
  To: Manasi Navare; +Cc: intel-gfx

On Tue, Mar 15, 2022 at 04:38:56PM -0700, Manasi Navare wrote:
> This patch abstracts pieces of hsw_crtc_enable corresponding to different
> Bspec enable sequence steps into separate functions.
> This helps to call them in a specific order for bigjoiner master/slave
> in a cleaner fashion.
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Animesh Manna <animesh.manna@intel.com>
> Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_display.c | 125 ++++++++++---------
>  1 file changed, 66 insertions(+), 59 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index eb49973621f0..d8e6466c9fa0 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -1865,24 +1865,6 @@ static void hsw_set_frame_start_delay(const struct intel_crtc_state *crtc_state)
>  	intel_de_write(dev_priv, reg, val);
>  }
>  
> -static void icl_ddi_bigjoiner_pre_enable(struct intel_atomic_state *state,
> -					 const struct intel_crtc_state *crtc_state)
> -{
> -	struct intel_crtc *master_crtc = intel_master_crtc(crtc_state);
> -
> -	/*
> -	 * Enable sequence steps 1-7 on bigjoiner master
> -	 */
> -	if (intel_crtc_is_bigjoiner_slave(crtc_state))
> -		intel_encoders_pre_pll_enable(state, master_crtc);
> -
> -	if (crtc_state->shared_dpll)
> -		intel_enable_shared_dpll(crtc_state);
> -
> -	if (intel_crtc_is_bigjoiner_slave(crtc_state))
> -		intel_encoders_pre_enable(state, master_crtc);
> -}
> -
>  static void hsw_configure_cpu_transcoder(const struct intel_crtc_state *crtc_state)
>  {
>  	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> @@ -1910,70 +1892,73 @@ static void hsw_configure_cpu_transcoder(const struct intel_crtc_state *crtc_sta
>  	hsw_set_transconf(crtc_state);
>  }
>  
> -static void hsw_crtc_enable(struct intel_atomic_state *state,
> -			    struct intel_crtc *crtc)
> +static void hsw_crtc_pre_pll_enable(struct intel_atomic_state *state,
> +				    const struct intel_crtc_state *crtc_state)
>  {
> -	const struct intel_crtc_state *new_crtc_state =
> -		intel_atomic_get_new_crtc_state(state, crtc);
> -	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> -	enum pipe pipe = crtc->pipe, hsw_workaround_pipe;
> -	enum transcoder cpu_transcoder = new_crtc_state->cpu_transcoder;
> -	bool psl_clkgate_wa;
> -
> -	if (drm_WARN_ON(&dev_priv->drm, crtc->active))
> -		return;
> +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
>  
> -	if (!new_crtc_state->bigjoiner_pipes) {
> -		intel_encoders_pre_pll_enable(state, crtc);
> +	/*
> +	 * Enable sequence steps 1 - 7 on all pipes
> +	 */
> +	intel_encoders_pre_pll_enable(state, crtc);
> +	if (crtc_state->shared_dpll)
> +		intel_enable_shared_dpll(crtc_state);
>  
> -		if (new_crtc_state->shared_dpll)
> -			intel_enable_shared_dpll(new_crtc_state);
> +	intel_encoders_pre_enable(state, crtc);
> +}
>  
> -		intel_encoders_pre_enable(state, crtc);
> -	} else {
> -		icl_ddi_bigjoiner_pre_enable(state, new_crtc_state);
> -	}
> +static void hsw_crtc_post_pll_enable(struct intel_atomic_state *state,
> +				     const struct intel_crtc_state *crtc_state)
> +{
> +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> +	enum pipe pipe = crtc->pipe, hsw_workaround_pipe;
> +	enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
> +	bool psl_clkgate_wa;
>  
> -	intel_dsc_enable(new_crtc_state);
> +	/*
> +	 * Enable sequence step 8
> +	 */
> +	intel_dsc_enable(crtc_state);
>  
>  	if (DISPLAY_VER(dev_priv) >= 13)
> -		intel_uncompressed_joiner_enable(new_crtc_state);
> +		intel_uncompressed_joiner_enable(crtc_state);
>  
> -	intel_set_pipe_src_size(new_crtc_state);
> +	intel_set_pipe_src_size(crtc_state);
>  	if (DISPLAY_VER(dev_priv) >= 9 || IS_BROADWELL(dev_priv))
> -		bdw_set_pipemisc(new_crtc_state);
> +		bdw_set_pipemisc(crtc_state);
>  
> -	if (!intel_crtc_is_bigjoiner_slave(new_crtc_state) &&
> +	if (!intel_crtc_is_bigjoiner_slave(crtc_state) &&
>  	    !transcoder_is_dsi(cpu_transcoder))
> -		hsw_configure_cpu_transcoder(new_crtc_state);
> +		hsw_configure_cpu_transcoder(crtc_state);
>  
>  	crtc->active = true;
>  
>  	/* Display WA #1180: WaDisableScalarClockGating: glk */
>  	psl_clkgate_wa = DISPLAY_VER(dev_priv) == 10 &&
> -		new_crtc_state->pch_pfit.enabled;
> +		crtc_state->pch_pfit.enabled;
>  	if (psl_clkgate_wa)
>  		glk_pipe_scaler_clock_gating_wa(dev_priv, pipe, true);
>  
>  	if (DISPLAY_VER(dev_priv) >= 9)
> -		skl_pfit_enable(new_crtc_state);
> +		skl_pfit_enable(crtc_state);
>  	else
> -		ilk_pfit_enable(new_crtc_state);
> +		ilk_pfit_enable(crtc_state);
>  
>  	/*
>  	 * On ILK+ LUT must be loaded before the pipe is running but with
>  	 * clocks enabled
>  	 */
> -	intel_color_load_luts(new_crtc_state);
> -	intel_color_commit(new_crtc_state);
> +	intel_color_load_luts(crtc_state);
> +	intel_color_commit(crtc_state);
>  	/* update DSPCNTR to configure gamma/csc for pipe bottom color */
>  	if (DISPLAY_VER(dev_priv) < 9)
> -		intel_disable_primary_plane(new_crtc_state);
> +		intel_disable_primary_plane(crtc_state);
>  
> -	hsw_set_linetime_wm(new_crtc_state);
> +	hsw_set_linetime_wm(crtc_state);
>  
>  	if (DISPLAY_VER(dev_priv) >= 11)
> -		icl_set_pipe_chicken(new_crtc_state);
> +		icl_set_pipe_chicken(crtc_state);
>  
>  	intel_initial_watermarks(state, crtc);
>  
> @@ -1984,8 +1969,8 @@ static void hsw_crtc_enable(struct intel_atomic_state *state,
>  		icl_pipe_mbus_enable(crtc, dbuf_state->joined_mbus);
>  	}
>  
> -	if (intel_crtc_is_bigjoiner_slave(new_crtc_state))
> -		intel_crtc_vblank_on(new_crtc_state);
> +	if (intel_crtc_is_bigjoiner_slave(crtc_state))
> +		intel_crtc_vblank_on(crtc_state);
>  
>  	intel_encoders_enable(state, crtc);
>  
> @@ -1996,7 +1981,7 @@ static void hsw_crtc_enable(struct intel_atomic_state *state,
>  
>  	/* If we change the relative order between pipe/planes enabling, we need
>  	 * to change the workaround. */
> -	hsw_workaround_pipe = new_crtc_state->hsw_workaround_pipe;
> +	hsw_workaround_pipe = crtc_state->hsw_workaround_pipe;
>  	if (IS_HASWELL(dev_priv) && hsw_workaround_pipe != INVALID_PIPE) {
>  		struct intel_crtc *wa_crtc;
>  
> @@ -2007,6 +1992,29 @@ static void hsw_crtc_enable(struct intel_atomic_state *state,
>  	}
>  }
>  
> +static void hsw_crtc_enable(struct intel_atomic_state *state,
> +			    struct intel_crtc *crtc)
> +{
> +	const struct intel_crtc_state *new_crtc_state =
> +		intel_atomic_get_new_crtc_state(state, crtc);
> +	struct intel_crtc *slave_crtc;
> +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> +
> +	if (drm_WARN_ON(&dev_priv->drm, crtc->active))
> +		return;
> +
> +	hsw_crtc_pre_pll_enable(state, new_crtc_state);
> +
> +	for_each_intel_crtc_in_pipe_mask(&dev_priv->drm, slave_crtc,
> +					 intel_crtc_bigjoiner_slave_pipes(new_crtc_state)) {
> +		struct intel_crtc_state *slave_crtc_state =
> +			intel_atomic_get_new_crtc_state(state, slave_crtc);
> +
> +		hsw_crtc_post_pll_enable(state, slave_crtc_state);
> +	}
> +	hsw_crtc_post_pll_enable(state, new_crtc_state);
> +}

I suspect this is far too high level for bigjoiner. Eg. there's a bunch
of things already in the disable sequence that seem to need much more
low level sequencing between the joined pipes. So my gut feeling still
is that we want to continue the per-pipe vs. per-transcoder split and
do the joiner loops in lower level code.

> +
>  void ilk_pfit_disable(const struct intel_crtc_state *old_crtc_state)
>  {
>  	struct intel_crtc *crtc = to_intel_crtc(old_crtc_state->uapi.crtc);
> @@ -8122,11 +8130,11 @@ static void intel_enable_crtc(struct intel_atomic_state *state,
>  
>  	intel_crtc_update_active_timings(new_crtc_state);
>  
> -	dev_priv->display->crtc_enable(state, crtc);
> -
>  	if (intel_crtc_is_bigjoiner_slave(new_crtc_state))
>  		return;
>  
> +	dev_priv->display->crtc_enable(state, crtc);
> +
>  	intel_drrs_enable(new_crtc_state);
>  
>  	/* vblanks work again, re-enable pipe CRC. */
> @@ -8360,8 +8368,7 @@ static void skl_commit_modeset_enables(struct intel_atomic_state *state)
>  			continue;
>  
>  		if (intel_dp_mst_is_slave_trans(new_crtc_state) ||
> -		    is_trans_port_sync_master(new_crtc_state) ||
> -		    intel_crtc_is_bigjoiner_master(new_crtc_state))
> +		    is_trans_port_sync_master(new_crtc_state))
>  			continue;
>  
>  		modeset_pipes &= ~BIT(pipe);
> @@ -8371,7 +8378,7 @@ static void skl_commit_modeset_enables(struct intel_atomic_state *state)
>  
>  	/*
>  	 * Then we enable all remaining pipes that depend on other
> -	 * pipes: MST slaves and port sync masters, big joiner master
> +	 * pipes: MST slaves and port sync masters
>  	 */
>  	for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state, i) {
>  		enum pipe pipe = crtc->pipe;
> -- 
> 2.19.1

-- 
Ville Syrjälä
Intel

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [Intel-gfx] [PATCH] drm/i915/display/: Refactor hsw_crtc_enable for bigjoiner cleanup
  2022-03-17 18:52 ` Ville Syrjälä
@ 2022-03-17 19:05   ` Navare, Manasi
  2022-03-17 19:14     ` Ville Syrjälä
  0 siblings, 1 reply; 14+ messages in thread
From: Navare, Manasi @ 2022-03-17 19:05 UTC (permalink / raw)
  To: Ville Syrjälä; +Cc: intel-gfx

On Thu, Mar 17, 2022 at 08:52:52PM +0200, Ville Syrjälä wrote:
> On Tue, Mar 15, 2022 at 04:38:56PM -0700, Manasi Navare wrote:
> > This patch abstracts pieces of hsw_crtc_enable corresponding to different
> > Bspec enable sequence steps into separate functions.
> > This helps to call them in a specific order for bigjoiner master/slave
> > in a cleaner fashion.
> > 
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: Animesh Manna <animesh.manna@intel.com>
> > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_display.c | 125 ++++++++++---------
> >  1 file changed, 66 insertions(+), 59 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > index eb49973621f0..d8e6466c9fa0 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -1865,24 +1865,6 @@ static void hsw_set_frame_start_delay(const struct intel_crtc_state *crtc_state)
> >  	intel_de_write(dev_priv, reg, val);
> >  }
> >  
> > -static void icl_ddi_bigjoiner_pre_enable(struct intel_atomic_state *state,
> > -					 const struct intel_crtc_state *crtc_state)
> > -{
> > -	struct intel_crtc *master_crtc = intel_master_crtc(crtc_state);
> > -
> > -	/*
> > -	 * Enable sequence steps 1-7 on bigjoiner master
> > -	 */
> > -	if (intel_crtc_is_bigjoiner_slave(crtc_state))
> > -		intel_encoders_pre_pll_enable(state, master_crtc);
> > -
> > -	if (crtc_state->shared_dpll)
> > -		intel_enable_shared_dpll(crtc_state);
> > -
> > -	if (intel_crtc_is_bigjoiner_slave(crtc_state))
> > -		intel_encoders_pre_enable(state, master_crtc);
> > -}
> > -
> >  static void hsw_configure_cpu_transcoder(const struct intel_crtc_state *crtc_state)
> >  {
> >  	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> > @@ -1910,70 +1892,73 @@ static void hsw_configure_cpu_transcoder(const struct intel_crtc_state *crtc_sta
> >  	hsw_set_transconf(crtc_state);
> >  }
> >  
> > -static void hsw_crtc_enable(struct intel_atomic_state *state,
> > -			    struct intel_crtc *crtc)
> > +static void hsw_crtc_pre_pll_enable(struct intel_atomic_state *state,
> > +				    const struct intel_crtc_state *crtc_state)
> >  {
> > -	const struct intel_crtc_state *new_crtc_state =
> > -		intel_atomic_get_new_crtc_state(state, crtc);
> > -	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> > -	enum pipe pipe = crtc->pipe, hsw_workaround_pipe;
> > -	enum transcoder cpu_transcoder = new_crtc_state->cpu_transcoder;
> > -	bool psl_clkgate_wa;
> > -
> > -	if (drm_WARN_ON(&dev_priv->drm, crtc->active))
> > -		return;
> > +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> >  
> > -	if (!new_crtc_state->bigjoiner_pipes) {
> > -		intel_encoders_pre_pll_enable(state, crtc);
> > +	/*
> > +	 * Enable sequence steps 1 - 7 on all pipes
> > +	 */
> > +	intel_encoders_pre_pll_enable(state, crtc);
> > +	if (crtc_state->shared_dpll)
> > +		intel_enable_shared_dpll(crtc_state);
> >  
> > -		if (new_crtc_state->shared_dpll)
> > -			intel_enable_shared_dpll(new_crtc_state);
> > +	intel_encoders_pre_enable(state, crtc);
> > +}
> >  
> > -		intel_encoders_pre_enable(state, crtc);
> > -	} else {
> > -		icl_ddi_bigjoiner_pre_enable(state, new_crtc_state);
> > -	}
> > +static void hsw_crtc_post_pll_enable(struct intel_atomic_state *state,
> > +				     const struct intel_crtc_state *crtc_state)
> > +{
> > +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> > +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> > +	enum pipe pipe = crtc->pipe, hsw_workaround_pipe;
> > +	enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
> > +	bool psl_clkgate_wa;
> >  
> > -	intel_dsc_enable(new_crtc_state);
> > +	/*
> > +	 * Enable sequence step 8
> > +	 */
> > +	intel_dsc_enable(crtc_state);
> >  
> >  	if (DISPLAY_VER(dev_priv) >= 13)
> > -		intel_uncompressed_joiner_enable(new_crtc_state);
> > +		intel_uncompressed_joiner_enable(crtc_state);
> >  
> > -	intel_set_pipe_src_size(new_crtc_state);
> > +	intel_set_pipe_src_size(crtc_state);
> >  	if (DISPLAY_VER(dev_priv) >= 9 || IS_BROADWELL(dev_priv))
> > -		bdw_set_pipemisc(new_crtc_state);
> > +		bdw_set_pipemisc(crtc_state);
> >  
> > -	if (!intel_crtc_is_bigjoiner_slave(new_crtc_state) &&
> > +	if (!intel_crtc_is_bigjoiner_slave(crtc_state) &&
> >  	    !transcoder_is_dsi(cpu_transcoder))
> > -		hsw_configure_cpu_transcoder(new_crtc_state);
> > +		hsw_configure_cpu_transcoder(crtc_state);
> >  
> >  	crtc->active = true;
> >  
> >  	/* Display WA #1180: WaDisableScalarClockGating: glk */
> >  	psl_clkgate_wa = DISPLAY_VER(dev_priv) == 10 &&
> > -		new_crtc_state->pch_pfit.enabled;
> > +		crtc_state->pch_pfit.enabled;
> >  	if (psl_clkgate_wa)
> >  		glk_pipe_scaler_clock_gating_wa(dev_priv, pipe, true);
> >  
> >  	if (DISPLAY_VER(dev_priv) >= 9)
> > -		skl_pfit_enable(new_crtc_state);
> > +		skl_pfit_enable(crtc_state);
> >  	else
> > -		ilk_pfit_enable(new_crtc_state);
> > +		ilk_pfit_enable(crtc_state);
> >  
> >  	/*
> >  	 * On ILK+ LUT must be loaded before the pipe is running but with
> >  	 * clocks enabled
> >  	 */
> > -	intel_color_load_luts(new_crtc_state);
> > -	intel_color_commit(new_crtc_state);
> > +	intel_color_load_luts(crtc_state);
> > +	intel_color_commit(crtc_state);
> >  	/* update DSPCNTR to configure gamma/csc for pipe bottom color */
> >  	if (DISPLAY_VER(dev_priv) < 9)
> > -		intel_disable_primary_plane(new_crtc_state);
> > +		intel_disable_primary_plane(crtc_state);
> >  
> > -	hsw_set_linetime_wm(new_crtc_state);
> > +	hsw_set_linetime_wm(crtc_state);
> >  
> >  	if (DISPLAY_VER(dev_priv) >= 11)
> > -		icl_set_pipe_chicken(new_crtc_state);
> > +		icl_set_pipe_chicken(crtc_state);
> >  
> >  	intel_initial_watermarks(state, crtc);
> >  
> > @@ -1984,8 +1969,8 @@ static void hsw_crtc_enable(struct intel_atomic_state *state,
> >  		icl_pipe_mbus_enable(crtc, dbuf_state->joined_mbus);
> >  	}
> >  
> > -	if (intel_crtc_is_bigjoiner_slave(new_crtc_state))
> > -		intel_crtc_vblank_on(new_crtc_state);
> > +	if (intel_crtc_is_bigjoiner_slave(crtc_state))
> > +		intel_crtc_vblank_on(crtc_state);
> >  
> >  	intel_encoders_enable(state, crtc);
> >  
> > @@ -1996,7 +1981,7 @@ static void hsw_crtc_enable(struct intel_atomic_state *state,
> >  
> >  	/* If we change the relative order between pipe/planes enabling, we need
> >  	 * to change the workaround. */
> > -	hsw_workaround_pipe = new_crtc_state->hsw_workaround_pipe;
> > +	hsw_workaround_pipe = crtc_state->hsw_workaround_pipe;
> >  	if (IS_HASWELL(dev_priv) && hsw_workaround_pipe != INVALID_PIPE) {
> >  		struct intel_crtc *wa_crtc;
> >  
> > @@ -2007,6 +1992,29 @@ static void hsw_crtc_enable(struct intel_atomic_state *state,
> >  	}
> >  }
> >  
> > +static void hsw_crtc_enable(struct intel_atomic_state *state,
> > +			    struct intel_crtc *crtc)
> > +{
> > +	const struct intel_crtc_state *new_crtc_state =
> > +		intel_atomic_get_new_crtc_state(state, crtc);
> > +	struct intel_crtc *slave_crtc;
> > +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> > +
> > +	if (drm_WARN_ON(&dev_priv->drm, crtc->active))
> > +		return;
> > +
> > +	hsw_crtc_pre_pll_enable(state, new_crtc_state);
> > +
> > +	for_each_intel_crtc_in_pipe_mask(&dev_priv->drm, slave_crtc,
> > +					 intel_crtc_bigjoiner_slave_pipes(new_crtc_state)) {
> > +		struct intel_crtc_state *slave_crtc_state =
> > +			intel_atomic_get_new_crtc_state(state, slave_crtc);
> > +
> > +		hsw_crtc_post_pll_enable(state, slave_crtc_state);
> > +	}
> > +	hsw_crtc_post_pll_enable(state, new_crtc_state);
> > +}
> 
> I suspect this is far too high level for bigjoiner. Eg. there's a bunch
> of things already in the disable sequence that seem to need much more
> low level sequencing between the joined pipes. So my gut feeling still
> is that we want to continue the per-pipe vs. per-transcoder split and
> do the joiner loops in lower level code.
>

But for enable sequence upto all pre_pll_enable needs to happen for master and then post_pll_enable for all slaves first and then master.
Hence added the sequencing here. Because like I have split here the post_pll_enable covers Bspec step 8 and hence that needs to happen for all
slaves first and then master.
I believe this cleans up a lot of bigjoiner special casing.

Further like Jani said, I can just make a separate icl_crtc_enable hook and only that will have this sequence.

Does this make sense as a first refacoring step? This should to scale the code to multiple joiners.

Manasi
 
> > +
> >  void ilk_pfit_disable(const struct intel_crtc_state *old_crtc_state)
> >  {
> >  	struct intel_crtc *crtc = to_intel_crtc(old_crtc_state->uapi.crtc);
> > @@ -8122,11 +8130,11 @@ static void intel_enable_crtc(struct intel_atomic_state *state,
> >  
> >  	intel_crtc_update_active_timings(new_crtc_state);
> >  
> > -	dev_priv->display->crtc_enable(state, crtc);
> > -
> >  	if (intel_crtc_is_bigjoiner_slave(new_crtc_state))
> >  		return;
> >  
> > +	dev_priv->display->crtc_enable(state, crtc);
> > +
> >  	intel_drrs_enable(new_crtc_state);
> >  
> >  	/* vblanks work again, re-enable pipe CRC. */
> > @@ -8360,8 +8368,7 @@ static void skl_commit_modeset_enables(struct intel_atomic_state *state)
> >  			continue;
> >  
> >  		if (intel_dp_mst_is_slave_trans(new_crtc_state) ||
> > -		    is_trans_port_sync_master(new_crtc_state) ||
> > -		    intel_crtc_is_bigjoiner_master(new_crtc_state))
> > +		    is_trans_port_sync_master(new_crtc_state))
> >  			continue;
> >  
> >  		modeset_pipes &= ~BIT(pipe);
> > @@ -8371,7 +8378,7 @@ static void skl_commit_modeset_enables(struct intel_atomic_state *state)
> >  
> >  	/*
> >  	 * Then we enable all remaining pipes that depend on other
> > -	 * pipes: MST slaves and port sync masters, big joiner master
> > +	 * pipes: MST slaves and port sync masters
> >  	 */
> >  	for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state, i) {
> >  		enum pipe pipe = crtc->pipe;
> > -- 
> > 2.19.1
> 
> -- 
> Ville Syrjälä
> Intel

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [Intel-gfx] [PATCH] drm/i915/display/: Refactor hsw_crtc_enable for bigjoiner cleanup
  2022-03-17 19:05   ` Navare, Manasi
@ 2022-03-17 19:14     ` Ville Syrjälä
  2022-03-30  0:00       ` Navare, Manasi
  0 siblings, 1 reply; 14+ messages in thread
From: Ville Syrjälä @ 2022-03-17 19:14 UTC (permalink / raw)
  To: Navare, Manasi; +Cc: intel-gfx

On Thu, Mar 17, 2022 at 12:05:47PM -0700, Navare, Manasi wrote:
> On Thu, Mar 17, 2022 at 08:52:52PM +0200, Ville Syrjälä wrote:
> > On Tue, Mar 15, 2022 at 04:38:56PM -0700, Manasi Navare wrote:
> > > This patch abstracts pieces of hsw_crtc_enable corresponding to different
> > > Bspec enable sequence steps into separate functions.
> > > This helps to call them in a specific order for bigjoiner master/slave
> > > in a cleaner fashion.
> > > 
> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Cc: Animesh Manna <animesh.manna@intel.com>
> > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_display.c | 125 ++++++++++---------
> > >  1 file changed, 66 insertions(+), 59 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > > index eb49973621f0..d8e6466c9fa0 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > @@ -1865,24 +1865,6 @@ static void hsw_set_frame_start_delay(const struct intel_crtc_state *crtc_state)
> > >  	intel_de_write(dev_priv, reg, val);
> > >  }
> > >  
> > > -static void icl_ddi_bigjoiner_pre_enable(struct intel_atomic_state *state,
> > > -					 const struct intel_crtc_state *crtc_state)
> > > -{
> > > -	struct intel_crtc *master_crtc = intel_master_crtc(crtc_state);
> > > -
> > > -	/*
> > > -	 * Enable sequence steps 1-7 on bigjoiner master
> > > -	 */
> > > -	if (intel_crtc_is_bigjoiner_slave(crtc_state))
> > > -		intel_encoders_pre_pll_enable(state, master_crtc);
> > > -
> > > -	if (crtc_state->shared_dpll)
> > > -		intel_enable_shared_dpll(crtc_state);
> > > -
> > > -	if (intel_crtc_is_bigjoiner_slave(crtc_state))
> > > -		intel_encoders_pre_enable(state, master_crtc);
> > > -}
> > > -
> > >  static void hsw_configure_cpu_transcoder(const struct intel_crtc_state *crtc_state)
> > >  {
> > >  	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> > > @@ -1910,70 +1892,73 @@ static void hsw_configure_cpu_transcoder(const struct intel_crtc_state *crtc_sta
> > >  	hsw_set_transconf(crtc_state);
> > >  }
> > >  
> > > -static void hsw_crtc_enable(struct intel_atomic_state *state,
> > > -			    struct intel_crtc *crtc)
> > > +static void hsw_crtc_pre_pll_enable(struct intel_atomic_state *state,
> > > +				    const struct intel_crtc_state *crtc_state)
> > >  {
> > > -	const struct intel_crtc_state *new_crtc_state =
> > > -		intel_atomic_get_new_crtc_state(state, crtc);
> > > -	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> > > -	enum pipe pipe = crtc->pipe, hsw_workaround_pipe;
> > > -	enum transcoder cpu_transcoder = new_crtc_state->cpu_transcoder;
> > > -	bool psl_clkgate_wa;
> > > -
> > > -	if (drm_WARN_ON(&dev_priv->drm, crtc->active))
> > > -		return;
> > > +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> > >  
> > > -	if (!new_crtc_state->bigjoiner_pipes) {
> > > -		intel_encoders_pre_pll_enable(state, crtc);
> > > +	/*
> > > +	 * Enable sequence steps 1 - 7 on all pipes
> > > +	 */
> > > +	intel_encoders_pre_pll_enable(state, crtc);
> > > +	if (crtc_state->shared_dpll)
> > > +		intel_enable_shared_dpll(crtc_state);
> > >  
> > > -		if (new_crtc_state->shared_dpll)
> > > -			intel_enable_shared_dpll(new_crtc_state);
> > > +	intel_encoders_pre_enable(state, crtc);
> > > +}
> > >  
> > > -		intel_encoders_pre_enable(state, crtc);
> > > -	} else {
> > > -		icl_ddi_bigjoiner_pre_enable(state, new_crtc_state);
> > > -	}
> > > +static void hsw_crtc_post_pll_enable(struct intel_atomic_state *state,
> > > +				     const struct intel_crtc_state *crtc_state)
> > > +{
> > > +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> > > +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> > > +	enum pipe pipe = crtc->pipe, hsw_workaround_pipe;
> > > +	enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
> > > +	bool psl_clkgate_wa;
> > >  
> > > -	intel_dsc_enable(new_crtc_state);
> > > +	/*
> > > +	 * Enable sequence step 8
> > > +	 */
> > > +	intel_dsc_enable(crtc_state);
> > >  
> > >  	if (DISPLAY_VER(dev_priv) >= 13)
> > > -		intel_uncompressed_joiner_enable(new_crtc_state);
> > > +		intel_uncompressed_joiner_enable(crtc_state);
> > >  
> > > -	intel_set_pipe_src_size(new_crtc_state);
> > > +	intel_set_pipe_src_size(crtc_state);
> > >  	if (DISPLAY_VER(dev_priv) >= 9 || IS_BROADWELL(dev_priv))
> > > -		bdw_set_pipemisc(new_crtc_state);
> > > +		bdw_set_pipemisc(crtc_state);
> > >  
> > > -	if (!intel_crtc_is_bigjoiner_slave(new_crtc_state) &&
> > > +	if (!intel_crtc_is_bigjoiner_slave(crtc_state) &&
> > >  	    !transcoder_is_dsi(cpu_transcoder))
> > > -		hsw_configure_cpu_transcoder(new_crtc_state);
> > > +		hsw_configure_cpu_transcoder(crtc_state);
> > >  
> > >  	crtc->active = true;
> > >  
> > >  	/* Display WA #1180: WaDisableScalarClockGating: glk */
> > >  	psl_clkgate_wa = DISPLAY_VER(dev_priv) == 10 &&
> > > -		new_crtc_state->pch_pfit.enabled;
> > > +		crtc_state->pch_pfit.enabled;
> > >  	if (psl_clkgate_wa)
> > >  		glk_pipe_scaler_clock_gating_wa(dev_priv, pipe, true);
> > >  
> > >  	if (DISPLAY_VER(dev_priv) >= 9)
> > > -		skl_pfit_enable(new_crtc_state);
> > > +		skl_pfit_enable(crtc_state);
> > >  	else
> > > -		ilk_pfit_enable(new_crtc_state);
> > > +		ilk_pfit_enable(crtc_state);
> > >  
> > >  	/*
> > >  	 * On ILK+ LUT must be loaded before the pipe is running but with
> > >  	 * clocks enabled
> > >  	 */
> > > -	intel_color_load_luts(new_crtc_state);
> > > -	intel_color_commit(new_crtc_state);
> > > +	intel_color_load_luts(crtc_state);
> > > +	intel_color_commit(crtc_state);
> > >  	/* update DSPCNTR to configure gamma/csc for pipe bottom color */
> > >  	if (DISPLAY_VER(dev_priv) < 9)
> > > -		intel_disable_primary_plane(new_crtc_state);
> > > +		intel_disable_primary_plane(crtc_state);
> > >  
> > > -	hsw_set_linetime_wm(new_crtc_state);
> > > +	hsw_set_linetime_wm(crtc_state);
> > >  
> > >  	if (DISPLAY_VER(dev_priv) >= 11)
> > > -		icl_set_pipe_chicken(new_crtc_state);
> > > +		icl_set_pipe_chicken(crtc_state);
> > >  
> > >  	intel_initial_watermarks(state, crtc);
> > >  
> > > @@ -1984,8 +1969,8 @@ static void hsw_crtc_enable(struct intel_atomic_state *state,
> > >  		icl_pipe_mbus_enable(crtc, dbuf_state->joined_mbus);
> > >  	}
> > >  
> > > -	if (intel_crtc_is_bigjoiner_slave(new_crtc_state))
> > > -		intel_crtc_vblank_on(new_crtc_state);
> > > +	if (intel_crtc_is_bigjoiner_slave(crtc_state))
> > > +		intel_crtc_vblank_on(crtc_state);
> > >  
> > >  	intel_encoders_enable(state, crtc);
> > >  
> > > @@ -1996,7 +1981,7 @@ static void hsw_crtc_enable(struct intel_atomic_state *state,
> > >  
> > >  	/* If we change the relative order between pipe/planes enabling, we need
> > >  	 * to change the workaround. */
> > > -	hsw_workaround_pipe = new_crtc_state->hsw_workaround_pipe;
> > > +	hsw_workaround_pipe = crtc_state->hsw_workaround_pipe;
> > >  	if (IS_HASWELL(dev_priv) && hsw_workaround_pipe != INVALID_PIPE) {
> > >  		struct intel_crtc *wa_crtc;
> > >  
> > > @@ -2007,6 +1992,29 @@ static void hsw_crtc_enable(struct intel_atomic_state *state,
> > >  	}
> > >  }
> > >  
> > > +static void hsw_crtc_enable(struct intel_atomic_state *state,
> > > +			    struct intel_crtc *crtc)
> > > +{
> > > +	const struct intel_crtc_state *new_crtc_state =
> > > +		intel_atomic_get_new_crtc_state(state, crtc);
> > > +	struct intel_crtc *slave_crtc;
> > > +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> > > +
> > > +	if (drm_WARN_ON(&dev_priv->drm, crtc->active))
> > > +		return;
> > > +
> > > +	hsw_crtc_pre_pll_enable(state, new_crtc_state);
> > > +
> > > +	for_each_intel_crtc_in_pipe_mask(&dev_priv->drm, slave_crtc,
> > > +					 intel_crtc_bigjoiner_slave_pipes(new_crtc_state)) {
> > > +		struct intel_crtc_state *slave_crtc_state =
> > > +			intel_atomic_get_new_crtc_state(state, slave_crtc);
> > > +
> > > +		hsw_crtc_post_pll_enable(state, slave_crtc_state);
> > > +	}
> > > +	hsw_crtc_post_pll_enable(state, new_crtc_state);
> > > +}
> > 
> > I suspect this is far too high level for bigjoiner. Eg. there's a bunch
> > of things already in the disable sequence that seem to need much more
> > low level sequencing between the joined pipes. So my gut feeling still
> > is that we want to continue the per-pipe vs. per-transcoder split and
> > do the joiner loops in lower level code.
> >
> 
> But for enable sequence upto all pre_pll_enable needs to happen for master and then post_pll_enable for all slaves first and then master.

We want the enable and disable to be as symmetric as possible. So you
can't just blindly stare at the enable sequence and ignore what's going
on in the disable sequence. Otherwise the end result will be a confusing
mess.

> Hence added the sequencing here. Because like I have split here the post_pll_enable covers Bspec step 8 and hence that needs to happen for all
> slaves first and then master.
> I believe this cleans up a lot of bigjoiner special casing.
> 
> Further like Jani said, I can just make a separate icl_crtc_enable hook and only that will have this sequence.

That doesn't seem particularly helpful if it just means copy-pasting
the whole hsw_crtc_enable() into two places and removing one or two
little things from the original.

> 
> Does this make sense as a first refacoring step? This should to scale the code to multiple joiners.
> 
> Manasi
>  
> > > +
> > >  void ilk_pfit_disable(const struct intel_crtc_state *old_crtc_state)
> > >  {
> > >  	struct intel_crtc *crtc = to_intel_crtc(old_crtc_state->uapi.crtc);
> > > @@ -8122,11 +8130,11 @@ static void intel_enable_crtc(struct intel_atomic_state *state,
> > >  
> > >  	intel_crtc_update_active_timings(new_crtc_state);
> > >  
> > > -	dev_priv->display->crtc_enable(state, crtc);
> > > -
> > >  	if (intel_crtc_is_bigjoiner_slave(new_crtc_state))
> > >  		return;
> > >  
> > > +	dev_priv->display->crtc_enable(state, crtc);
> > > +
> > >  	intel_drrs_enable(new_crtc_state);
> > >  
> > >  	/* vblanks work again, re-enable pipe CRC. */
> > > @@ -8360,8 +8368,7 @@ static void skl_commit_modeset_enables(struct intel_atomic_state *state)
> > >  			continue;
> > >  
> > >  		if (intel_dp_mst_is_slave_trans(new_crtc_state) ||
> > > -		    is_trans_port_sync_master(new_crtc_state) ||
> > > -		    intel_crtc_is_bigjoiner_master(new_crtc_state))
> > > +		    is_trans_port_sync_master(new_crtc_state))
> > >  			continue;
> > >  
> > >  		modeset_pipes &= ~BIT(pipe);
> > > @@ -8371,7 +8378,7 @@ static void skl_commit_modeset_enables(struct intel_atomic_state *state)
> > >  
> > >  	/*
> > >  	 * Then we enable all remaining pipes that depend on other
> > > -	 * pipes: MST slaves and port sync masters, big joiner master
> > > +	 * pipes: MST slaves and port sync masters
> > >  	 */
> > >  	for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state, i) {
> > >  		enum pipe pipe = crtc->pipe;
> > > -- 
> > > 2.19.1
> > 
> > -- 
> > Ville Syrjälä
> > Intel

-- 
Ville Syrjälä
Intel

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [Intel-gfx] [PATCH] drm/i915/display/: Refactor hsw_crtc_enable for bigjoiner cleanup
  2022-03-17 18:31         ` Navare, Manasi
@ 2022-03-17 19:15           ` Jani Nikula
  0 siblings, 0 replies; 14+ messages in thread
From: Jani Nikula @ 2022-03-17 19:15 UTC (permalink / raw)
  To: Navare, Manasi; +Cc: intel-gfx

On Thu, 17 Mar 2022, "Navare, Manasi" <manasi.d.navare@intel.com> wrote:
> On Thu, Mar 17, 2022 at 11:28:03AM -0700, Navare, Manasi wrote:
>> On Thu, Mar 17, 2022 at 05:35:43PM +0200, Jani Nikula wrote:
>> > On Wed, 16 Mar 2022, "Navare, Manasi" <manasi.d.navare@intel.com> wrote:
>> > > On Wed, Mar 16, 2022 at 09:48:17AM +0200, Jani Nikula wrote:
>> > >> On Tue, 15 Mar 2022, Manasi Navare <manasi.d.navare@intel.com> wrote:
>> > >> > This patch abstracts pieces of hsw_crtc_enable corresponding to different
>> > >> > Bspec enable sequence steps into separate functions.
>> > >> > This helps to call them in a specific order for bigjoiner master/slave
>> > >> > in a cleaner fashion.
>> > >> 
>> > >> So does this contain only refactoring or functional changes also? One or
>> > >> the other at a time, please, not both.
>> > >
>> > > No this is only refactor, no functional changes here
>> > >
>> > >> 
>> > >> Also looks like hsw_crtc_* have accumulated just way too many checks for
>> > >> platforms instead of having a clean break at e.g. display 9 and/or 11.
>> > >
>> > > These checks were already part of hsw_crtc_enable() function that I have just moved to a separate
>> > > function. How do you recommend removing these checks?
>> > 
>> > We use hsw_crtc_enable for all DDI platforms and later. We do have the
>> > difference between skl_display_funcs and ddi_display_funcs, but they
>> > both point to hsw_crtc_enable. Maybe it's time for them to have their
>> > own functions that don't have to take so many platform differences into
>> > account.
>> 
>> Yes I could perhaps have a separate skl_crtc_enable() function which would then have all parts that apply to DISPLAy >=9
>> everything else can stay in hsw_crtc_enable
>> 
>> That makes sense?
>
> Or actually we could have a separate one for >=11 starting ICL, since there are a lot of
> checks for >=11. Also bigjoiner support only from ICL, so then this bigjoiner sequence then only becomes part
> of icl_crtc_enable()
> Which one of the above 2 do you recommend?

Could be both! Depends on the improved clarity vs. amount of code
duplication.

BR,
Jani.


>
> Manasi
>
>> 
>> Manasi
>> 
>> > 
>> > BR,
>> > Jani.
>> > 
>> > 
>> > 
>> > 
>> > >
>> > >> 
>> > >> Adding references to "enable sequence step 8" is not helpful because
>> > >> it's platform specific. (Yeah, I know there are existing references like
>> > >> this, but they're also suspect.)
>> > >
>> > > Yes agree, may be I will add the comment for what actually the step 7/8 does?
>> > >
>> > > Manasi
>> > >
>> > >> 
>> > >> 
>> > >> BR,
>> > >> Jani.
>> > >> 
>> > >> 
>> > >> >
>> > >> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> > >> > Cc: Animesh Manna <animesh.manna@intel.com>
>> > >> > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
>> > >> > ---
>> > >> >  drivers/gpu/drm/i915/display/intel_display.c | 125 ++++++++++---------
>> > >> >  1 file changed, 66 insertions(+), 59 deletions(-)
>> > >> >
>> > >> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
>> > >> > index eb49973621f0..d8e6466c9fa0 100644
>> > >> > --- a/drivers/gpu/drm/i915/display/intel_display.c
>> > >> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
>> > >> > @@ -1865,24 +1865,6 @@ static void hsw_set_frame_start_delay(const struct intel_crtc_state *crtc_state)
>> > >> >  	intel_de_write(dev_priv, reg, val);
>> > >> >  }
>> > >> >  
>> > >> > -static void icl_ddi_bigjoiner_pre_enable(struct intel_atomic_state *state,
>> > >> > -					 const struct intel_crtc_state *crtc_state)
>> > >> > -{
>> > >> > -	struct intel_crtc *master_crtc = intel_master_crtc(crtc_state);
>> > >> > -
>> > >> > -	/*
>> > >> > -	 * Enable sequence steps 1-7 on bigjoiner master
>> > >> > -	 */
>> > >> > -	if (intel_crtc_is_bigjoiner_slave(crtc_state))
>> > >> > -		intel_encoders_pre_pll_enable(state, master_crtc);
>> > >> > -
>> > >> > -	if (crtc_state->shared_dpll)
>> > >> > -		intel_enable_shared_dpll(crtc_state);
>> > >> > -
>> > >> > -	if (intel_crtc_is_bigjoiner_slave(crtc_state))
>> > >> > -		intel_encoders_pre_enable(state, master_crtc);
>> > >> > -}
>> > >> > -
>> > >> >  static void hsw_configure_cpu_transcoder(const struct intel_crtc_state *crtc_state)
>> > >> >  {
>> > >> >  	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
>> > >> > @@ -1910,70 +1892,73 @@ static void hsw_configure_cpu_transcoder(const struct intel_crtc_state *crtc_sta
>> > >> >  	hsw_set_transconf(crtc_state);
>> > >> >  }
>> > >> >  
>> > >> > -static void hsw_crtc_enable(struct intel_atomic_state *state,
>> > >> > -			    struct intel_crtc *crtc)
>> > >> > +static void hsw_crtc_pre_pll_enable(struct intel_atomic_state *state,
>> > >> > +				    const struct intel_crtc_state *crtc_state)
>> > >> >  {
>> > >> > -	const struct intel_crtc_state *new_crtc_state =
>> > >> > -		intel_atomic_get_new_crtc_state(state, crtc);
>> > >> > -	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>> > >> > -	enum pipe pipe = crtc->pipe, hsw_workaround_pipe;
>> > >> > -	enum transcoder cpu_transcoder = new_crtc_state->cpu_transcoder;
>> > >> > -	bool psl_clkgate_wa;
>> > >> > -
>> > >> > -	if (drm_WARN_ON(&dev_priv->drm, crtc->active))
>> > >> > -		return;
>> > >> > +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
>> > >> >  
>> > >> > -	if (!new_crtc_state->bigjoiner_pipes) {
>> > >> > -		intel_encoders_pre_pll_enable(state, crtc);
>> > >> > +	/*
>> > >> > +	 * Enable sequence steps 1 - 7 on all pipes
>> > >> > +	 */
>> > >> > +	intel_encoders_pre_pll_enable(state, crtc);
>> > >> > +	if (crtc_state->shared_dpll)
>> > >> > +		intel_enable_shared_dpll(crtc_state);
>> > >> >  
>> > >> > -		if (new_crtc_state->shared_dpll)
>> > >> > -			intel_enable_shared_dpll(new_crtc_state);
>> > >> > +	intel_encoders_pre_enable(state, crtc);
>> > >> > +}
>> > >> >  
>> > >> > -		intel_encoders_pre_enable(state, crtc);
>> > >> > -	} else {
>> > >> > -		icl_ddi_bigjoiner_pre_enable(state, new_crtc_state);
>> > >> > -	}
>> > >> > +static void hsw_crtc_post_pll_enable(struct intel_atomic_state *state,
>> > >> > +				     const struct intel_crtc_state *crtc_state)
>> > >> > +{
>> > >> > +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
>> > >> > +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>> > >> > +	enum pipe pipe = crtc->pipe, hsw_workaround_pipe;
>> > >> > +	enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
>> > >> > +	bool psl_clkgate_wa;
>> > >> >  
>> > >> > -	intel_dsc_enable(new_crtc_state);
>> > >> > +	/*
>> > >> > +	 * Enable sequence step 8
>> > >> > +	 */
>> > >> > +	intel_dsc_enable(crtc_state);
>> > >> >  
>> > >> >  	if (DISPLAY_VER(dev_priv) >= 13)
>> > >> > -		intel_uncompressed_joiner_enable(new_crtc_state);
>> > >> > +		intel_uncompressed_joiner_enable(crtc_state);
>> > >> >  
>> > >> > -	intel_set_pipe_src_size(new_crtc_state);
>> > >> > +	intel_set_pipe_src_size(crtc_state);
>> > >> >  	if (DISPLAY_VER(dev_priv) >= 9 || IS_BROADWELL(dev_priv))
>> > >> > -		bdw_set_pipemisc(new_crtc_state);
>> > >> > +		bdw_set_pipemisc(crtc_state);
>> > >> >  
>> > >> > -	if (!intel_crtc_is_bigjoiner_slave(new_crtc_state) &&
>> > >> > +	if (!intel_crtc_is_bigjoiner_slave(crtc_state) &&
>> > >> >  	    !transcoder_is_dsi(cpu_transcoder))
>> > >> > -		hsw_configure_cpu_transcoder(new_crtc_state);
>> > >> > +		hsw_configure_cpu_transcoder(crtc_state);
>> > >> >  
>> > >> >  	crtc->active = true;
>> > >> >  
>> > >> >  	/* Display WA #1180: WaDisableScalarClockGating: glk */
>> > >> >  	psl_clkgate_wa = DISPLAY_VER(dev_priv) == 10 &&
>> > >> > -		new_crtc_state->pch_pfit.enabled;
>> > >> > +		crtc_state->pch_pfit.enabled;
>> > >> >  	if (psl_clkgate_wa)
>> > >> >  		glk_pipe_scaler_clock_gating_wa(dev_priv, pipe, true);
>> > >> >  
>> > >> >  	if (DISPLAY_VER(dev_priv) >= 9)
>> > >> > -		skl_pfit_enable(new_crtc_state);
>> > >> > +		skl_pfit_enable(crtc_state);
>> > >> >  	else
>> > >> > -		ilk_pfit_enable(new_crtc_state);
>> > >> > +		ilk_pfit_enable(crtc_state);
>> > >> >  
>> > >> >  	/*
>> > >> >  	 * On ILK+ LUT must be loaded before the pipe is running but with
>> > >> >  	 * clocks enabled
>> > >> >  	 */
>> > >> > -	intel_color_load_luts(new_crtc_state);
>> > >> > -	intel_color_commit(new_crtc_state);
>> > >> > +	intel_color_load_luts(crtc_state);
>> > >> > +	intel_color_commit(crtc_state);
>> > >> >  	/* update DSPCNTR to configure gamma/csc for pipe bottom color */
>> > >> >  	if (DISPLAY_VER(dev_priv) < 9)
>> > >> > -		intel_disable_primary_plane(new_crtc_state);
>> > >> > +		intel_disable_primary_plane(crtc_state);
>> > >> >  
>> > >> > -	hsw_set_linetime_wm(new_crtc_state);
>> > >> > +	hsw_set_linetime_wm(crtc_state);
>> > >> >  
>> > >> >  	if (DISPLAY_VER(dev_priv) >= 11)
>> > >> > -		icl_set_pipe_chicken(new_crtc_state);
>> > >> > +		icl_set_pipe_chicken(crtc_state);
>> > >> >  
>> > >> >  	intel_initial_watermarks(state, crtc);
>> > >> >  
>> > >> > @@ -1984,8 +1969,8 @@ static void hsw_crtc_enable(struct intel_atomic_state *state,
>> > >> >  		icl_pipe_mbus_enable(crtc, dbuf_state->joined_mbus);
>> > >> >  	}
>> > >> >  
>> > >> > -	if (intel_crtc_is_bigjoiner_slave(new_crtc_state))
>> > >> > -		intel_crtc_vblank_on(new_crtc_state);
>> > >> > +	if (intel_crtc_is_bigjoiner_slave(crtc_state))
>> > >> > +		intel_crtc_vblank_on(crtc_state);
>> > >> >  
>> > >> >  	intel_encoders_enable(state, crtc);
>> > >> >  
>> > >> > @@ -1996,7 +1981,7 @@ static void hsw_crtc_enable(struct intel_atomic_state *state,
>> > >> >  
>> > >> >  	/* If we change the relative order between pipe/planes enabling, we need
>> > >> >  	 * to change the workaround. */
>> > >> > -	hsw_workaround_pipe = new_crtc_state->hsw_workaround_pipe;
>> > >> > +	hsw_workaround_pipe = crtc_state->hsw_workaround_pipe;
>> > >> >  	if (IS_HASWELL(dev_priv) && hsw_workaround_pipe != INVALID_PIPE) {
>> > >> >  		struct intel_crtc *wa_crtc;
>> > >> >  
>> > >> > @@ -2007,6 +1992,29 @@ static void hsw_crtc_enable(struct intel_atomic_state *state,
>> > >> >  	}
>> > >> >  }
>> > >> >  
>> > >> > +static void hsw_crtc_enable(struct intel_atomic_state *state,
>> > >> > +			    struct intel_crtc *crtc)
>> > >> > +{
>> > >> > +	const struct intel_crtc_state *new_crtc_state =
>> > >> > +		intel_atomic_get_new_crtc_state(state, crtc);
>> > >> > +	struct intel_crtc *slave_crtc;
>> > >> > +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>> > >> > +
>> > >> > +	if (drm_WARN_ON(&dev_priv->drm, crtc->active))
>> > >> > +		return;
>> > >> > +
>> > >> > +	hsw_crtc_pre_pll_enable(state, new_crtc_state);
>> > >> > +
>> > >> > +	for_each_intel_crtc_in_pipe_mask(&dev_priv->drm, slave_crtc,
>> > >> > +					 intel_crtc_bigjoiner_slave_pipes(new_crtc_state)) {
>> > >> > +		struct intel_crtc_state *slave_crtc_state =
>> > >> > +			intel_atomic_get_new_crtc_state(state, slave_crtc);
>> > >> > +
>> > >> > +		hsw_crtc_post_pll_enable(state, slave_crtc_state);
>> > >> > +	}
>> > >> > +	hsw_crtc_post_pll_enable(state, new_crtc_state);
>> > >> > +}
>> > >> > +
>> > >> >  void ilk_pfit_disable(const struct intel_crtc_state *old_crtc_state)
>> > >> >  {
>> > >> >  	struct intel_crtc *crtc = to_intel_crtc(old_crtc_state->uapi.crtc);
>> > >> > @@ -8122,11 +8130,11 @@ static void intel_enable_crtc(struct intel_atomic_state *state,
>> > >> >  
>> > >> >  	intel_crtc_update_active_timings(new_crtc_state);
>> > >> >  
>> > >> > -	dev_priv->display->crtc_enable(state, crtc);
>> > >> > -
>> > >> >  	if (intel_crtc_is_bigjoiner_slave(new_crtc_state))
>> > >> >  		return;
>> > >> >  
>> > >> > +	dev_priv->display->crtc_enable(state, crtc);
>> > >> > +
>> > >> >  	intel_drrs_enable(new_crtc_state);
>> > >> >  
>> > >> >  	/* vblanks work again, re-enable pipe CRC. */
>> > >> > @@ -8360,8 +8368,7 @@ static void skl_commit_modeset_enables(struct intel_atomic_state *state)
>> > >> >  			continue;
>> > >> >  
>> > >> >  		if (intel_dp_mst_is_slave_trans(new_crtc_state) ||
>> > >> > -		    is_trans_port_sync_master(new_crtc_state) ||
>> > >> > -		    intel_crtc_is_bigjoiner_master(new_crtc_state))
>> > >> > +		    is_trans_port_sync_master(new_crtc_state))
>> > >> >  			continue;
>> > >> >  
>> > >> >  		modeset_pipes &= ~BIT(pipe);
>> > >> > @@ -8371,7 +8378,7 @@ static void skl_commit_modeset_enables(struct intel_atomic_state *state)
>> > >> >  
>> > >> >  	/*
>> > >> >  	 * Then we enable all remaining pipes that depend on other
>> > >> > -	 * pipes: MST slaves and port sync masters, big joiner master
>> > >> > +	 * pipes: MST slaves and port sync masters
>> > >> >  	 */
>> > >> >  	for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state, i) {
>> > >> >  		enum pipe pipe = crtc->pipe;
>> > >> 
>> > >> -- 
>> > >> Jani Nikula, Intel Open Source Graphics Center
>> > 
>> > -- 
>> > Jani Nikula, Intel Open Source Graphics Center

-- 
Jani Nikula, Intel Open Source Graphics Center

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [Intel-gfx] [PATCH] drm/i915/display/: Refactor hsw_crtc_enable for bigjoiner cleanup
  2022-03-17 19:14     ` Ville Syrjälä
@ 2022-03-30  0:00       ` Navare, Manasi
  2022-03-30  9:53         ` Ville Syrjälä
  0 siblings, 1 reply; 14+ messages in thread
From: Navare, Manasi @ 2022-03-30  0:00 UTC (permalink / raw)
  To: Ville Syrjälä; +Cc: intel-gfx

Hi Ville,

I was looking at your suggestion of extracting the per pipe stuff out.
Currently in hsw_crtc_enable: the Only non per pipe stuff which gets enabled for the encoders is :
encoder specific is pre_pll_enable(), enable_shared_dpll, encoders_pre_enable and configure_cpu_transcoder() - All of this 
can be put in a function hsw_encoder_configure() or something that can still be called from with hsw_crtc_enable

Then the remaining can go into a per pipe function that can be called for each slave pipe

But it means still pretty much splitting the current hsw_crtc_enable into 2 separate functions

Does this refactoring make sense?

Manasi


On Thu, Mar 17, 2022 at 09:14:16PM +0200, Ville Syrjälä wrote:
> On Thu, Mar 17, 2022 at 12:05:47PM -0700, Navare, Manasi wrote:
> > On Thu, Mar 17, 2022 at 08:52:52PM +0200, Ville Syrjälä wrote:
> > > On Tue, Mar 15, 2022 at 04:38:56PM -0700, Manasi Navare wrote:
> > > > This patch abstracts pieces of hsw_crtc_enable corresponding to different
> > > > Bspec enable sequence steps into separate functions.
> > > > This helps to call them in a specific order for bigjoiner master/slave
> > > > in a cleaner fashion.
> > > > 
> > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > Cc: Animesh Manna <animesh.manna@intel.com>
> > > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/display/intel_display.c | 125 ++++++++++---------
> > > >  1 file changed, 66 insertions(+), 59 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > > > index eb49973621f0..d8e6466c9fa0 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > > @@ -1865,24 +1865,6 @@ static void hsw_set_frame_start_delay(const struct intel_crtc_state *crtc_state)
> > > >  	intel_de_write(dev_priv, reg, val);
> > > >  }
> > > >  
> > > > -static void icl_ddi_bigjoiner_pre_enable(struct intel_atomic_state *state,
> > > > -					 const struct intel_crtc_state *crtc_state)
> > > > -{
> > > > -	struct intel_crtc *master_crtc = intel_master_crtc(crtc_state);
> > > > -
> > > > -	/*
> > > > -	 * Enable sequence steps 1-7 on bigjoiner master
> > > > -	 */
> > > > -	if (intel_crtc_is_bigjoiner_slave(crtc_state))
> > > > -		intel_encoders_pre_pll_enable(state, master_crtc);
> > > > -
> > > > -	if (crtc_state->shared_dpll)
> > > > -		intel_enable_shared_dpll(crtc_state);
> > > > -
> > > > -	if (intel_crtc_is_bigjoiner_slave(crtc_state))
> > > > -		intel_encoders_pre_enable(state, master_crtc);
> > > > -}
> > > > -
> > > >  static void hsw_configure_cpu_transcoder(const struct intel_crtc_state *crtc_state)
> > > >  {
> > > >  	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> > > > @@ -1910,70 +1892,73 @@ static void hsw_configure_cpu_transcoder(const struct intel_crtc_state *crtc_sta
> > > >  	hsw_set_transconf(crtc_state);
> > > >  }
> > > >  
> > > > -static void hsw_crtc_enable(struct intel_atomic_state *state,
> > > > -			    struct intel_crtc *crtc)
> > > > +static void hsw_crtc_pre_pll_enable(struct intel_atomic_state *state,
> > > > +				    const struct intel_crtc_state *crtc_state)
> > > >  {
> > > > -	const struct intel_crtc_state *new_crtc_state =
> > > > -		intel_atomic_get_new_crtc_state(state, crtc);
> > > > -	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> > > > -	enum pipe pipe = crtc->pipe, hsw_workaround_pipe;
> > > > -	enum transcoder cpu_transcoder = new_crtc_state->cpu_transcoder;
> > > > -	bool psl_clkgate_wa;
> > > > -
> > > > -	if (drm_WARN_ON(&dev_priv->drm, crtc->active))
> > > > -		return;
> > > > +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> > > >  
> > > > -	if (!new_crtc_state->bigjoiner_pipes) {
> > > > -		intel_encoders_pre_pll_enable(state, crtc);
> > > > +	/*
> > > > +	 * Enable sequence steps 1 - 7 on all pipes
> > > > +	 */
> > > > +	intel_encoders_pre_pll_enable(state, crtc);
> > > > +	if (crtc_state->shared_dpll)
> > > > +		intel_enable_shared_dpll(crtc_state);
> > > >  
> > > > -		if (new_crtc_state->shared_dpll)
> > > > -			intel_enable_shared_dpll(new_crtc_state);
> > > > +	intel_encoders_pre_enable(state, crtc);
> > > > +}
> > > >  
> > > > -		intel_encoders_pre_enable(state, crtc);
> > > > -	} else {
> > > > -		icl_ddi_bigjoiner_pre_enable(state, new_crtc_state);
> > > > -	}
> > > > +static void hsw_crtc_post_pll_enable(struct intel_atomic_state *state,
> > > > +				     const struct intel_crtc_state *crtc_state)
> > > > +{
> > > > +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> > > > +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> > > > +	enum pipe pipe = crtc->pipe, hsw_workaround_pipe;
> > > > +	enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
> > > > +	bool psl_clkgate_wa;
> > > >  
> > > > -	intel_dsc_enable(new_crtc_state);
> > > > +	/*
> > > > +	 * Enable sequence step 8
> > > > +	 */
> > > > +	intel_dsc_enable(crtc_state);
> > > >  
> > > >  	if (DISPLAY_VER(dev_priv) >= 13)
> > > > -		intel_uncompressed_joiner_enable(new_crtc_state);
> > > > +		intel_uncompressed_joiner_enable(crtc_state);
> > > >  
> > > > -	intel_set_pipe_src_size(new_crtc_state);
> > > > +	intel_set_pipe_src_size(crtc_state);
> > > >  	if (DISPLAY_VER(dev_priv) >= 9 || IS_BROADWELL(dev_priv))
> > > > -		bdw_set_pipemisc(new_crtc_state);
> > > > +		bdw_set_pipemisc(crtc_state);
> > > >  
> > > > -	if (!intel_crtc_is_bigjoiner_slave(new_crtc_state) &&
> > > > +	if (!intel_crtc_is_bigjoiner_slave(crtc_state) &&
> > > >  	    !transcoder_is_dsi(cpu_transcoder))
> > > > -		hsw_configure_cpu_transcoder(new_crtc_state);
> > > > +		hsw_configure_cpu_transcoder(crtc_state);
> > > >  
> > > >  	crtc->active = true;
> > > >  
> > > >  	/* Display WA #1180: WaDisableScalarClockGating: glk */
> > > >  	psl_clkgate_wa = DISPLAY_VER(dev_priv) == 10 &&
> > > > -		new_crtc_state->pch_pfit.enabled;
> > > > +		crtc_state->pch_pfit.enabled;
> > > >  	if (psl_clkgate_wa)
> > > >  		glk_pipe_scaler_clock_gating_wa(dev_priv, pipe, true);
> > > >  
> > > >  	if (DISPLAY_VER(dev_priv) >= 9)
> > > > -		skl_pfit_enable(new_crtc_state);
> > > > +		skl_pfit_enable(crtc_state);
> > > >  	else
> > > > -		ilk_pfit_enable(new_crtc_state);
> > > > +		ilk_pfit_enable(crtc_state);
> > > >  
> > > >  	/*
> > > >  	 * On ILK+ LUT must be loaded before the pipe is running but with
> > > >  	 * clocks enabled
> > > >  	 */
> > > > -	intel_color_load_luts(new_crtc_state);
> > > > -	intel_color_commit(new_crtc_state);
> > > > +	intel_color_load_luts(crtc_state);
> > > > +	intel_color_commit(crtc_state);
> > > >  	/* update DSPCNTR to configure gamma/csc for pipe bottom color */
> > > >  	if (DISPLAY_VER(dev_priv) < 9)
> > > > -		intel_disable_primary_plane(new_crtc_state);
> > > > +		intel_disable_primary_plane(crtc_state);
> > > >  
> > > > -	hsw_set_linetime_wm(new_crtc_state);
> > > > +	hsw_set_linetime_wm(crtc_state);
> > > >  
> > > >  	if (DISPLAY_VER(dev_priv) >= 11)
> > > > -		icl_set_pipe_chicken(new_crtc_state);
> > > > +		icl_set_pipe_chicken(crtc_state);
> > > >  
> > > >  	intel_initial_watermarks(state, crtc);
> > > >  
> > > > @@ -1984,8 +1969,8 @@ static void hsw_crtc_enable(struct intel_atomic_state *state,
> > > >  		icl_pipe_mbus_enable(crtc, dbuf_state->joined_mbus);
> > > >  	}
> > > >  
> > > > -	if (intel_crtc_is_bigjoiner_slave(new_crtc_state))
> > > > -		intel_crtc_vblank_on(new_crtc_state);
> > > > +	if (intel_crtc_is_bigjoiner_slave(crtc_state))
> > > > +		intel_crtc_vblank_on(crtc_state);
> > > >  
> > > >  	intel_encoders_enable(state, crtc);
> > > >  
> > > > @@ -1996,7 +1981,7 @@ static void hsw_crtc_enable(struct intel_atomic_state *state,
> > > >  
> > > >  	/* If we change the relative order between pipe/planes enabling, we need
> > > >  	 * to change the workaround. */
> > > > -	hsw_workaround_pipe = new_crtc_state->hsw_workaround_pipe;
> > > > +	hsw_workaround_pipe = crtc_state->hsw_workaround_pipe;
> > > >  	if (IS_HASWELL(dev_priv) && hsw_workaround_pipe != INVALID_PIPE) {
> > > >  		struct intel_crtc *wa_crtc;
> > > >  
> > > > @@ -2007,6 +1992,29 @@ static void hsw_crtc_enable(struct intel_atomic_state *state,
> > > >  	}
> > > >  }
> > > >  
> > > > +static void hsw_crtc_enable(struct intel_atomic_state *state,
> > > > +			    struct intel_crtc *crtc)
> > > > +{
> > > > +	const struct intel_crtc_state *new_crtc_state =
> > > > +		intel_atomic_get_new_crtc_state(state, crtc);
> > > > +	struct intel_crtc *slave_crtc;
> > > > +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> > > > +
> > > > +	if (drm_WARN_ON(&dev_priv->drm, crtc->active))
> > > > +		return;
> > > > +
> > > > +	hsw_crtc_pre_pll_enable(state, new_crtc_state);
> > > > +
> > > > +	for_each_intel_crtc_in_pipe_mask(&dev_priv->drm, slave_crtc,
> > > > +					 intel_crtc_bigjoiner_slave_pipes(new_crtc_state)) {
> > > > +		struct intel_crtc_state *slave_crtc_state =
> > > > +			intel_atomic_get_new_crtc_state(state, slave_crtc);
> > > > +
> > > > +		hsw_crtc_post_pll_enable(state, slave_crtc_state);
> > > > +	}
> > > > +	hsw_crtc_post_pll_enable(state, new_crtc_state);
> > > > +}
> > > 
> > > I suspect this is far too high level for bigjoiner. Eg. there's a bunch
> > > of things already in the disable sequence that seem to need much more
> > > low level sequencing between the joined pipes. So my gut feeling still
> > > is that we want to continue the per-pipe vs. per-transcoder split and
> > > do the joiner loops in lower level code.
> > >
> > 
> > But for enable sequence upto all pre_pll_enable needs to happen for master and then post_pll_enable for all slaves first and then master.
> 
> We want the enable and disable to be as symmetric as possible. So you
> can't just blindly stare at the enable sequence and ignore what's going
> on in the disable sequence. Otherwise the end result will be a confusing
> mess.
> 
> > Hence added the sequencing here. Because like I have split here the post_pll_enable covers Bspec step 8 and hence that needs to happen for all
> > slaves first and then master.
> > I believe this cleans up a lot of bigjoiner special casing.
> > 
> > Further like Jani said, I can just make a separate icl_crtc_enable hook and only that will have this sequence.
> 
> That doesn't seem particularly helpful if it just means copy-pasting
> the whole hsw_crtc_enable() into two places and removing one or two
> little things from the original.
> 
> > 
> > Does this make sense as a first refacoring step? This should to scale the code to multiple joiners.
> > 
> > Manasi
> >  
> > > > +
> > > >  void ilk_pfit_disable(const struct intel_crtc_state *old_crtc_state)
> > > >  {
> > > >  	struct intel_crtc *crtc = to_intel_crtc(old_crtc_state->uapi.crtc);
> > > > @@ -8122,11 +8130,11 @@ static void intel_enable_crtc(struct intel_atomic_state *state,
> > > >  
> > > >  	intel_crtc_update_active_timings(new_crtc_state);
> > > >  
> > > > -	dev_priv->display->crtc_enable(state, crtc);
> > > > -
> > > >  	if (intel_crtc_is_bigjoiner_slave(new_crtc_state))
> > > >  		return;
> > > >  
> > > > +	dev_priv->display->crtc_enable(state, crtc);
> > > > +
> > > >  	intel_drrs_enable(new_crtc_state);
> > > >  
> > > >  	/* vblanks work again, re-enable pipe CRC. */
> > > > @@ -8360,8 +8368,7 @@ static void skl_commit_modeset_enables(struct intel_atomic_state *state)
> > > >  			continue;
> > > >  
> > > >  		if (intel_dp_mst_is_slave_trans(new_crtc_state) ||
> > > > -		    is_trans_port_sync_master(new_crtc_state) ||
> > > > -		    intel_crtc_is_bigjoiner_master(new_crtc_state))
> > > > +		    is_trans_port_sync_master(new_crtc_state))
> > > >  			continue;
> > > >  
> > > >  		modeset_pipes &= ~BIT(pipe);
> > > > @@ -8371,7 +8378,7 @@ static void skl_commit_modeset_enables(struct intel_atomic_state *state)
> > > >  
> > > >  	/*
> > > >  	 * Then we enable all remaining pipes that depend on other
> > > > -	 * pipes: MST slaves and port sync masters, big joiner master
> > > > +	 * pipes: MST slaves and port sync masters
> > > >  	 */
> > > >  	for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state, i) {
> > > >  		enum pipe pipe = crtc->pipe;
> > > > -- 
> > > > 2.19.1
> > > 
> > > -- 
> > > Ville Syrjälä
> > > Intel
> 
> -- 
> Ville Syrjälä
> Intel

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [Intel-gfx] [PATCH] drm/i915/display/: Refactor hsw_crtc_enable for bigjoiner cleanup
  2022-03-30  0:00       ` Navare, Manasi
@ 2022-03-30  9:53         ` Ville Syrjälä
  0 siblings, 0 replies; 14+ messages in thread
From: Ville Syrjälä @ 2022-03-30  9:53 UTC (permalink / raw)
  To: Navare, Manasi; +Cc: intel-gfx

On Tue, Mar 29, 2022 at 05:00:39PM -0700, Navare, Manasi wrote:
> Hi Ville,
> 
> I was looking at your suggestion of extracting the per pipe stuff out.
> Currently in hsw_crtc_enable: the Only non per pipe stuff which gets enabled for the encoders is :
> encoder specific is pre_pll_enable(), enable_shared_dpll, encoders_pre_enable and configure_cpu_transcoder() - All of this 
> can be put in a function hsw_encoder_configure() or something that can still be called from with hsw_crtc_enable

I don't see a need to move that stuff anywhere. Just leave it
in hsw_crtc_enable() IMO.

> 
> Then the remaining can go into a per pipe function that can be called for each slave pipe

This is what I have been suggesting.

But I think there's also some per-pipe stuff currently in the
the encoder hooks, and some of that is only done for the master
there whereaas the slave part I think are somewhere else. We
should attempt to fix that as well so that every step is done
in a consistent manner for every pipe be it master or slave.

-- 
Ville Syrjälä
Intel

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2022-03-30  9:53 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-03-15 23:38 [Intel-gfx] [PATCH] drm/i915/display/: Refactor hsw_crtc_enable for bigjoiner cleanup Manasi Navare
2022-03-16  0:41 ` [Intel-gfx] ✓ Fi.CI.BAT: success for " Patchwork
2022-03-16  3:21 ` [Intel-gfx] ✗ Fi.CI.IGT: failure " Patchwork
2022-03-16  7:48 ` [Intel-gfx] [PATCH] " Jani Nikula
2022-03-16 17:10   ` Navare, Manasi
2022-03-17 15:35     ` Jani Nikula
2022-03-17 18:28       ` Navare, Manasi
2022-03-17 18:31         ` Navare, Manasi
2022-03-17 19:15           ` Jani Nikula
2022-03-17 18:52 ` Ville Syrjälä
2022-03-17 19:05   ` Navare, Manasi
2022-03-17 19:14     ` Ville Syrjälä
2022-03-30  0:00       ` Navare, Manasi
2022-03-30  9:53         ` Ville Syrjälä

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.