[Intel-gfx] [PATCH] drm/i915/psr: Split sel fetch plane configuration into arm and noarm

[Intel-gfx] [PATCH] drm/i915/psr: Split sel fetch plane configuration into arm and noarm

[Jouni Högander]

SEL_FETCH_CTL registers are armed immediately when plane is disabled.
SEL_FETCH_* instances of plane configuration are used when doing
selective update and normal plane register instances for full updates.
Currently all SEL_FETCH_* registers are written as a part of noarm
plane configuration. If noarm and arm plane configuration are not
happening within same vblank we may end up having plane as a part of
selective update before it's PLANE_SURF register is written.

Fix this by splitting plane selective fetch configuration into arm and
noarm versions and call them accordingly. Write SEL_FETCH_CTL in arm
version.

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: José Roberto de Souza <jose.souza@intel.com>
Cc: Mika Kahola <mika.kahola@intel.com>
Cc: Vinod Govindapillai <vinod.govindapillai@intel.com>
Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
Signed-off-by: Jouni Högander <jouni.hogander@intel.com>
---
 drivers/gpu/drm/i915/display/intel_cursor.c   |  2 +-
 drivers/gpu/drm/i915/display/intel_psr.c      | 29 ++++++++++++++-----
 drivers/gpu/drm/i915/display/intel_psr.h      |  6 +++-
 .../drm/i915/display/skl_universal_plane.c    |  4 ++-
 4 files changed, 30 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_cursor.c b/drivers/gpu/drm/i915/display/intel_cursor.c
index d190fa0d393b..50232cec48e0 100644
--- a/drivers/gpu/drm/i915/display/intel_cursor.c
+++ b/drivers/gpu/drm/i915/display/intel_cursor.c
@@ -532,7 +532,7 @@ static void i9xx_cursor_update_arm(struct intel_plane *plane,
 		skl_write_cursor_wm(plane, crtc_state);
 
 	if (plane_state)
-		intel_psr2_program_plane_sel_fetch(plane, crtc_state, plane_state, 0);
+		intel_psr2_program_plane_sel_fetch_arm(plane, crtc_state, plane_state, 0);
 	else
 		intel_psr2_disable_plane_sel_fetch(plane, crtc_state);
 
diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c
index 7d4a15a283a0..63b79c611932 100644
--- a/drivers/gpu/drm/i915/display/intel_psr.c
+++ b/drivers/gpu/drm/i915/display/intel_psr.c
@@ -1559,7 +1559,26 @@ void intel_psr2_disable_plane_sel_fetch(struct intel_plane *plane,
 	intel_de_write_fw(dev_priv, PLANE_SEL_FETCH_CTL(pipe, plane->id), 0);
 }
 
-void intel_psr2_program_plane_sel_fetch(struct intel_plane *plane,
+void intel_psr2_program_plane_sel_fetch_arm(struct intel_plane *plane,
+					const struct intel_crtc_state *crtc_state,
+					const struct intel_plane_state *plane_state,
+					int color_plane)
+{
+	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
+	enum pipe pipe = plane->pipe;
+
+	if (!crtc_state->enable_psr2_sel_fetch)
+		return;
+
+	if (plane->id == PLANE_CURSOR)
+		intel_de_write_fw(dev_priv, PLANE_SEL_FETCH_CTL(pipe, plane->id),
+				  plane_state->ctl);
+	else
+		intel_de_write_fw(dev_priv, PLANE_SEL_FETCH_CTL(pipe, plane->id),
+				  PLANE_SEL_FETCH_CTL_ENABLE);
+}
+
+void intel_psr2_program_plane_sel_fetch_noarm(struct intel_plane *plane,
 					const struct intel_crtc_state *crtc_state,
 					const struct intel_plane_state *plane_state,
 					int color_plane)
@@ -1573,11 +1592,8 @@ void intel_psr2_program_plane_sel_fetch(struct intel_plane *plane,
 	if (!crtc_state->enable_psr2_sel_fetch)
 		return;
 
-	if (plane->id == PLANE_CURSOR) {
-		intel_de_write_fw(dev_priv, PLANE_SEL_FETCH_CTL(pipe, plane->id),
-				  plane_state->ctl);
+	if (plane->id == PLANE_CURSOR)
 		return;
-	}
 
 	clip = &plane_state->psr2_sel_fetch_area;
 
@@ -1605,9 +1621,6 @@ void intel_psr2_program_plane_sel_fetch(struct intel_plane *plane,
 	val = (drm_rect_height(clip) - 1) << 16;
 	val |= (drm_rect_width(&plane_state->uapi.src) >> 16) - 1;
 	intel_de_write_fw(dev_priv, PLANE_SEL_FETCH_SIZE(pipe, plane->id), val);
-
-	intel_de_write_fw(dev_priv, PLANE_SEL_FETCH_CTL(pipe, plane->id),
-			  PLANE_SEL_FETCH_CTL_ENABLE);
 }
 
 void intel_psr2_program_trans_man_trk_ctl(const struct intel_crtc_state *crtc_state)
diff --git a/drivers/gpu/drm/i915/display/intel_psr.h b/drivers/gpu/drm/i915/display/intel_psr.h
index 2ac3a46cccc5..49cd5beacf98 100644
--- a/drivers/gpu/drm/i915/display/intel_psr.h
+++ b/drivers/gpu/drm/i915/display/intel_psr.h
@@ -46,7 +46,11 @@ bool intel_psr_enabled(struct intel_dp *intel_dp);
 int intel_psr2_sel_fetch_update(struct intel_atomic_state *state,
 				struct intel_crtc *crtc);
 void intel_psr2_program_trans_man_trk_ctl(const struct intel_crtc_state *crtc_state);
-void intel_psr2_program_plane_sel_fetch(struct intel_plane *plane,
+void intel_psr2_program_plane_sel_fetch_noarm(struct intel_plane *plane,
+					const struct intel_crtc_state *crtc_state,
+					const struct intel_plane_state *plane_state,
+					int color_plane);
+void intel_psr2_program_plane_sel_fetch_arm(struct intel_plane *plane,
 					const struct intel_crtc_state *crtc_state,
 					const struct intel_plane_state *plane_state,
 					int color_plane);
diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c b/drivers/gpu/drm/i915/display/skl_universal_plane.c
index 9b172a1e90de..5a309a3e2812 100644
--- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
+++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
@@ -1260,7 +1260,7 @@ icl_plane_update_noarm(struct intel_plane *plane,
 	if (plane_state->force_black)
 		icl_plane_csc_load_black(plane);
 
-	intel_psr2_program_plane_sel_fetch(plane, crtc_state, plane_state, color_plane);
+	intel_psr2_program_plane_sel_fetch_noarm(plane, crtc_state, plane_state, color_plane);
 }
 
 static void
@@ -1287,6 +1287,8 @@ icl_plane_update_arm(struct intel_plane *plane,
 	if (plane_state->scaler_id >= 0)
 		skl_program_plane_scaler(plane, crtc_state, plane_state);
 
+	intel_psr2_program_plane_sel_fetch_arm(plane, crtc_state, plane_state, color_plane);
+
 	/*
 	 * The control register self-arms if the plane was previously
 	 * disabled. Try to make the plane enable atomic by writing
-- 
2.34.1

[Intel-gfx] ✓ Fi.CI.BAT: success for drm/i915/psr: Split sel fetch plane configuration into arm and noarm

[Patchwork]

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

== Series Details ==

Series: drm/i915/psr: Split sel fetch plane configuration into arm and noarm
URL   : https://patchwork.freedesktop.org/series/113321/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_12639 -> Patchwork_113321v1
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

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

Participating hosts (39 -> 38)
------------------------------

  Missing    (1): fi-tgl-dsi 

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

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

### IGT changes ###

#### Issues hit ####

  * igt@gem_exec_suspend@basic-s3@smem:
    - fi-rkl-11600:       NOTRUN -> [FAIL][1] ([fdo#103375])
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113321v1/fi-rkl-11600/igt@gem_exec_suspend@basic-s3@smem.html

  * igt@kms_chamelium_hpd@common-hpd-after-suspend:
    - fi-rkl-11600:       NOTRUN -> [SKIP][2] ([i915#7828])
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113321v1/fi-rkl-11600/igt@kms_chamelium_hpd@common-hpd-after-suspend.html

  
#### Possible fixes ####

  * igt@i915_selftest@live@migrate:
    - {bat-adlp-6}:       [DMESG-FAIL][3] ([i915#7699]) -> [PASS][4]
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12639/bat-adlp-6/igt@i915_selftest@live@migrate.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113321v1/bat-adlp-6/igt@i915_selftest@live@migrate.html

  * igt@i915_suspend@basic-s3-without-i915:
    - fi-rkl-11600:       [INCOMPLETE][5] ([i915#4817]) -> [PASS][6]
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12639/fi-rkl-11600/igt@i915_suspend@basic-s3-without-i915.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113321v1/fi-rkl-11600/igt@i915_suspend@basic-s3-without-i915.html

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

  [fdo#103375]: https://bugs.freedesktop.org/show_bug.cgi?id=103375
  [i915#4817]: https://gitlab.freedesktop.org/drm/intel/issues/4817
  [i915#4983]: https://gitlab.freedesktop.org/drm/intel/issues/4983
  [i915#6367]: https://gitlab.freedesktop.org/drm/intel/issues/6367
  [i915#6997]: https://gitlab.freedesktop.org/drm/intel/issues/6997
  [i915#7699]: https://gitlab.freedesktop.org/drm/intel/issues/7699
  [i915#7828]: https://gitlab.freedesktop.org/drm/intel/issues/7828
  [i915#7852]: https://gitlab.freedesktop.org/drm/intel/issues/7852


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

  * Linux: CI_DRM_12639 -> Patchwork_113321v1

  CI-20190529: 20190529
  CI_DRM_12639: 81c7c4a9c86dac434bc6d8c97fdc440866ca0d65 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_7136: 31b6af91747ad8c705399c9006cdb81cb1864146 @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
  Patchwork_113321v1: 81c7c4a9c86dac434bc6d8c97fdc440866ca0d65 @ git://anongit.freedesktop.org/gfx-ci/linux


### Linux commits

aa86bc3316f9 drm/i915/psr: Split sel fetch plane configuration into arm and noarm

== Logs ==

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

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

[Intel-gfx] ✓ Fi.CI.IGT: success for drm/i915/psr: Split sel fetch plane configuration into arm and noarm

[Patchwork]

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

== Series Details ==

Series: drm/i915/psr: Split sel fetch plane configuration into arm and noarm
URL   : https://patchwork.freedesktop.org/series/113321/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_12639_full -> Patchwork_113321v1_full
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

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

Participating hosts (13 -> 10)
------------------------------

  Missing    (3): shard-rkl0 pig-kbl-iris pig-skl-6260u 

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

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

### IGT changes ###

#### Issues hit ####

  * igt@gem_exec_fair@basic-deadline:
    - shard-glk:          [PASS][1] -> [FAIL][2] ([i915#2846])
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12639/shard-glk3/igt@gem_exec_fair@basic-deadline.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113321v1/shard-glk1/igt@gem_exec_fair@basic-deadline.html

  * igt@gem_exec_fair@basic-pace-share@rcs0:
    - shard-glk:          [PASS][3] -> [FAIL][4] ([i915#2842])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12639/shard-glk6/igt@gem_exec_fair@basic-pace-share@rcs0.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113321v1/shard-glk8/igt@gem_exec_fair@basic-pace-share@rcs0.html

  * igt@gem_lmem_swapping@heavy-random:
    - shard-glk:          NOTRUN -> [SKIP][5] ([fdo#109271] / [i915#4613])
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113321v1/shard-glk3/igt@gem_lmem_swapping@heavy-random.html

  * igt@kms_ccs@pipe-c-bad-aux-stride-y_tiled_gen12_mc_ccs:
    - shard-glk:          NOTRUN -> [SKIP][6] ([fdo#109271] / [i915#3886]) +1 similar issue
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113321v1/shard-glk3/igt@kms_ccs@pipe-c-bad-aux-stride-y_tiled_gen12_mc_ccs.html

  * igt@kms_flip_scaled_crc@flip-64bpp-yftile-to-16bpp-yftile-upscaling@pipe-a-valid-mode:
    - shard-glk:          NOTRUN -> [SKIP][7] ([fdo#109271]) +34 similar issues
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113321v1/shard-glk3/igt@kms_flip_scaled_crc@flip-64bpp-yftile-to-16bpp-yftile-upscaling@pipe-a-valid-mode.html

  
#### Possible fixes ####

  * igt@fbdev@unaligned-read:
    - {shard-rkl}:        [SKIP][8] ([i915#2582]) -> [PASS][9]
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12639/shard-rkl-2/igt@fbdev@unaligned-read.html
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113321v1/shard-rkl-6/igt@fbdev@unaligned-read.html

  * igt@gem_ctx_exec@basic-nohangcheck:
    - {shard-rkl}:        [FAIL][10] ([i915#6268]) -> [PASS][11]
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12639/shard-rkl-2/igt@gem_ctx_exec@basic-nohangcheck.html
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113321v1/shard-rkl-5/igt@gem_ctx_exec@basic-nohangcheck.html

  * igt@gem_eio@reset-stress:
    - {shard-dg1}:        [FAIL][12] ([i915#5784]) -> [PASS][13] +1 similar issue
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12639/shard-dg1-16/igt@gem_eio@reset-stress.html
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113321v1/shard-dg1-15/igt@gem_eio@reset-stress.html

  * igt@gem_exec_fair@basic-pace-share@rcs0:
    - {shard-rkl}:        [FAIL][14] ([i915#2842]) -> [PASS][15]
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12639/shard-rkl-4/igt@gem_exec_fair@basic-pace-share@rcs0.html
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113321v1/shard-rkl-6/igt@gem_exec_fair@basic-pace-share@rcs0.html
    - {shard-tglu}:       [FAIL][16] ([i915#2842]) -> [PASS][17]
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12639/shard-tglu-6/igt@gem_exec_fair@basic-pace-share@rcs0.html
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113321v1/shard-tglu-5/igt@gem_exec_fair@basic-pace-share@rcs0.html

  * igt@gem_exec_fair@basic-pace@vcs0:
    - shard-glk:          [FAIL][18] ([i915#2842]) -> [PASS][19]
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12639/shard-glk6/igt@gem_exec_fair@basic-pace@vcs0.html
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113321v1/shard-glk8/igt@gem_exec_fair@basic-pace@vcs0.html

  * igt@gem_exec_reloc@basic-wc-read-noreloc:
    - {shard-rkl}:        [SKIP][20] ([i915#3281]) -> [PASS][21] +9 similar issues
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12639/shard-rkl-6/igt@gem_exec_reloc@basic-wc-read-noreloc.html
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113321v1/shard-rkl-5/igt@gem_exec_reloc@basic-wc-read-noreloc.html

  * igt@gem_partial_pwrite_pread@write-display:
    - {shard-rkl}:        [SKIP][22] ([i915#3282]) -> [PASS][23] +1 similar issue
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12639/shard-rkl-3/igt@gem_partial_pwrite_pread@write-display.html
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113321v1/shard-rkl-5/igt@gem_partial_pwrite_pread@write-display.html

  * igt@gen9_exec_parse@bb-start-param:
    - {shard-rkl}:        [SKIP][24] ([i915#2527]) -> [PASS][25] +2 similar issues
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12639/shard-rkl-3/igt@gen9_exec_parse@bb-start-param.html
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113321v1/shard-rkl-5/igt@gen9_exec_parse@bb-start-param.html

  * igt@i915_pm_dc@dc6-dpms:
    - {shard-rkl}:        [SKIP][26] ([i915#3361]) -> [PASS][27]
   [26]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12639/shard-rkl-5/igt@i915_pm_dc@dc6-dpms.html
   [27]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113321v1/shard-rkl-1/igt@i915_pm_dc@dc6-dpms.html

  * igt@i915_pm_rpm@cursor-dpms:
    - {shard-tglu}:       [SKIP][28] ([i915#1849]) -> [PASS][29] +1 similar issue
   [28]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12639/shard-tglu-6/igt@i915_pm_rpm@cursor-dpms.html
   [29]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113321v1/shard-tglu-5/igt@i915_pm_rpm@cursor-dpms.html

  * igt@i915_pm_rpm@fences-dpms:
    - {shard-rkl}:        [SKIP][30] ([i915#1849]) -> [PASS][31] +1 similar issue
   [30]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12639/shard-rkl-2/igt@i915_pm_rpm@fences-dpms.html
   [31]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113321v1/shard-rkl-6/igt@i915_pm_rpm@fences-dpms.html

  * igt@kms_atomic@atomic_plane_damage:
    - {shard-rkl}:        [SKIP][32] ([i915#4098]) -> [PASS][33] +1 similar issue
   [32]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12639/shard-rkl-5/igt@kms_atomic@atomic_plane_damage.html
   [33]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113321v1/shard-rkl-6/igt@kms_atomic@atomic_plane_damage.html

  * igt@kms_big_fb@linear-addfb:
    - {shard-tglu}:       [SKIP][34] ([i915#7651]) -> [PASS][35] +6 similar issues
   [34]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12639/shard-tglu-6/igt@kms_big_fb@linear-addfb.html
   [35]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113321v1/shard-tglu-5/igt@kms_big_fb@linear-addfb.html

  * igt@kms_big_fb@x-tiled-addfb-size-offset-overflow:
    - {shard-tglu}:       [SKIP][36] ([i915#1845] / [i915#7651]) -> [PASS][37] +2 similar issues
   [36]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12639/shard-tglu-6/igt@kms_big_fb@x-tiled-addfb-size-offset-overflow.html
   [37]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113321v1/shard-tglu-5/igt@kms_big_fb@x-tiled-addfb-size-offset-overflow.html

  * igt@kms_cursor_legacy@flip-vs-cursor@atomic-transitions-varying-size:
    - shard-glk:          [FAIL][38] ([i915#2346]) -> [PASS][39]
   [38]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12639/shard-glk6/igt@kms_cursor_legacy@flip-vs-cursor@atomic-transitions-varying-size.html
   [39]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113321v1/shard-glk8/igt@kms_cursor_legacy@flip-vs-cursor@atomic-transitions-varying-size.html

  * igt@kms_fence_pin_leak:
    - {shard-tglu}:       [SKIP][40] ([fdo#109274] / [i915#1845]) -> [PASS][41]
   [40]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12639/shard-tglu-6/igt@kms_fence_pin_leak.html
   [41]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113321v1/shard-tglu-5/igt@kms_fence_pin_leak.html

  * igt@kms_frontbuffer_tracking@fbc-tiling-linear:
    - {shard-rkl}:        [SKIP][42] ([i915#1849] / [i915#4098]) -> [PASS][43] +20 similar issues
   [42]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12639/shard-rkl-5/igt@kms_frontbuffer_tracking@fbc-tiling-linear.html
   [43]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113321v1/shard-rkl-6/igt@kms_frontbuffer_tracking@fbc-tiling-linear.html

  * igt@kms_psr@cursor_mmap_gtt:
    - {shard-rkl}:        [SKIP][44] ([i915#1072]) -> [PASS][45] +3 similar issues
   [44]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12639/shard-rkl-5/igt@kms_psr@cursor_mmap_gtt.html
   [45]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113321v1/shard-rkl-6/igt@kms_psr@cursor_mmap_gtt.html

  * igt@kms_rotation_crc@exhaust-fences:
    - {shard-rkl}:        [SKIP][46] ([i915#1845] / [i915#4098]) -> [PASS][47] +35 similar issues
   [46]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12639/shard-rkl-5/igt@kms_rotation_crc@exhaust-fences.html
   [47]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113321v1/shard-rkl-6/igt@kms_rotation_crc@exhaust-fences.html

  * igt@perf@gen12-oa-tlb-invalidate:
    - {shard-rkl}:        [SKIP][48] ([fdo#109289]) -> [PASS][49] +1 similar issue
   [48]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12639/shard-rkl-5/igt@perf@gen12-oa-tlb-invalidate.html
   [49]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113321v1/shard-rkl-6/igt@perf@gen12-oa-tlb-invalidate.html

  * igt@perf@stress-open-close:
    - shard-glk:          [INCOMPLETE][50] ([i915#5213]) -> [PASS][51]
   [50]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12639/shard-glk7/igt@perf@stress-open-close.html
   [51]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113321v1/shard-glk3/igt@perf@stress-open-close.html

  * igt@prime_vgem@basic-fence-flip:
    - {shard-rkl}:        [SKIP][52] ([fdo#109295] / [i915#3708] / [i915#4098]) -> [PASS][53]
   [52]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12639/shard-rkl-5/igt@prime_vgem@basic-fence-flip.html
   [53]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113321v1/shard-rkl-6/igt@prime_vgem@basic-fence-flip.html

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

  [fdo#103375]: https://bugs.freedesktop.org/show_bug.cgi?id=103375
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109274]: https://bugs.freedesktop.org/show_bug.cgi?id=109274
  [fdo#109279]: https://bugs.freedesktop.org/show_bug.cgi?id=109279
  [fdo#109280]: https://bugs.freedesktop.org/show_bug.cgi?id=109280
  [fdo#109283]: https://bugs.freedesktop.org/show_bug.cgi?id=109283
  [fdo#109289]: https://bugs.freedesktop.org/show_bug.cgi?id=109289
  [fdo#109291]: https://bugs.freedesktop.org/show_bug.cgi?id=109291
  [fdo#109295]: https://bugs.freedesktop.org/show_bug.cgi?id=109295
  [fdo#109302]: https://bugs.freedesktop.org/show_bug.cgi?id=109302
  [fdo#109303]: https://bugs.freedesktop.org/show_bug.cgi?id=109303
  [fdo#109309]: https://bugs.freedesktop.org/show_bug.cgi?id=109309
  [fdo#109313]: https://bugs.freedesktop.org/show_bug.cgi?id=109313
  [fdo#109314]: https://bugs.freedesktop.org/show_bug.cgi?id=109314
  [fdo#109315]: https://bugs.freedesktop.org/show_bug.cgi?id=109315
  [fdo#109642]: https://bugs.freedesktop.org/show_bug.cgi?id=109642
  [fdo#110189]: https://bugs.freedesktop.org/show_bug.cgi?id=110189
  [fdo#110723]: https://bugs.freedesktop.org/show_bug.cgi?id=110723
  [fdo#111068]: https://bugs.freedesktop.org/show_bug.cgi?id=111068
  [fdo#111614]: https://bugs.freedesktop.org/show_bug.cgi?id=111614
  [fdo#111615]: https://bugs.freedesktop.org/show_bug.cgi?id=111615
  [fdo#111644]: https://bugs.freedesktop.org/show_bug.cgi?id=111644
  [fdo#111656]: https://bugs.freedesktop.org/show_bug.cgi?id=111656
  [fdo#111825]: https://bugs.freedesktop.org/show_bug.cgi?id=111825
  [fdo#111827]: https://bugs.freedesktop.org/show_bug.cgi?id=111827
  [fdo#112054]: https://bugs.freedesktop.org/show_bug.cgi?id=112054
  [fdo#112283]: https://bugs.freedesktop.org/show_bug.cgi?id=112283
  [i915#1072]: https://gitlab.freedesktop.org/drm/intel/issues/1072
  [i915#132]: https://gitlab.freedesktop.org/drm/intel/issues/132
  [i915#1397]: https://gitlab.freedesktop.org/drm/intel/issues/1397
  [i915#1825]: https://gitlab.freedesktop.org/drm/intel/issues/1825
  [i915#1839]: https://gitlab.freedesktop.org/drm/intel/issues/1839
  [i915#1845]: https://gitlab.freedesktop.org/drm/intel/issues/1845
  [i915#1849]: https://gitlab.freedesktop.org/drm/intel/issues/1849
  [i915#2190]: https://gitlab.freedesktop.org/drm/intel/issues/2190
  [i915#2232]: https://gitlab.freedesktop.org/drm/intel/issues/2232
  [i915#2346]: https://gitlab.freedesktop.org/drm/intel/issues/2346
  [i915#2433]: https://gitlab.freedesktop.org/drm/intel/issues/2433
  [i915#2437]: https://gitlab.freedesktop.org/drm/intel/issues/2437
  [i915#2527]: https://gitlab.freedesktop.org/drm/intel/issues/2527
  [i915#2575]: https://gitlab.freedesktop.org/drm/intel/issues/2575
  [i915#2582]: https://gitlab.freedesktop.org/drm/intel/issues/2582
  [i915#2587]: https://gitlab.freedesktop.org/drm/intel/issues/2587
  [i915#2658]: https://gitlab.freedesktop.org/drm/intel/issues/2658
  [i915#2672]: https://gitlab.freedesktop.org/drm/intel/issues/2672
  [i915#2705]: https://gitlab.freedesktop.org/drm/intel/issues/2705
  [i915#280]: https://gitlab.freedesktop.org/drm/intel/issues/280
  [i915#2842]: https://gitlab.freedesktop.org/drm/intel/issues/2842
  [i915#2846]: https://gitlab.freedesktop.org/drm/intel/issues/2846
  [i915#2856]: https://gitlab.freedesktop.org/drm/intel/issues/2856
  [i915#2920]: https://gitlab.freedesktop.org/drm/intel/issues/2920
  [i915#3116]: https://gitlab.freedesktop.org/drm/intel/issues/3116
  [i915#315]: https://gitlab.freedesktop.org/drm/intel/issues/315
  [i915#3281]: https://gitlab.freedesktop.org/drm/intel/issues/3281
  [i915#3282]: https://gitlab.freedesktop.org/drm/intel/issues/3282
  [i915#3297]: https://gitlab.freedesktop.org/drm/intel/issues/3297
  [i915#3299]: https://gitlab.freedesktop.org/drm/intel/issues/3299
  [i915#3318]: https://gitlab.freedesktop.org/drm/intel/issues/3318
  [i915#3359]: https://gitlab.freedesktop.org/drm/intel/issues/3359
  [i915#3361]: https://gitlab.freedesktop.org/drm/intel/issues/3361
  [i915#3458]: https://gitlab.freedesktop.org/drm/intel/issues/3458
  [i915#3469]: https://gitlab.freedesktop.org/drm/intel/issues/3469
  [i915#3539]: https://gitlab.freedesktop.org/drm/intel/issues/3539
  [i915#3546]: https://gitlab.freedesktop.org/drm/intel/issues/3546
  [i915#3555]: https://gitlab.freedesktop.org/drm/intel/issues/3555
  [i915#3558]: https://gitlab.freedesktop.org/drm/intel/issues/3558
  [i915#3637]: https://gitlab.freedesktop.org/drm/intel/issues/3637
  [i915#3638]: https://gitlab.freedesktop.org/drm/intel/issues/3638
  [i915#3689]: https://gitlab.freedesktop.org/drm/intel/issues/3689
  [i915#3708]: https://gitlab.freedesktop.org/drm/intel/issues/3708
  [i915#3734]: https://gitlab.freedesktop.org/drm/intel/issues/3734
  [i915#3742]: https://gitlab.freedesktop.org/drm/intel/issues/3742
  [i915#3826]: https://gitlab.freedesktop.org/drm/intel/issues/3826
  [i915#3840]: https://gitlab.freedesktop.org/drm/intel/issues/3840
  [i915#3886]: https://gitlab.freedesktop.org/drm/intel/issues/3886
  [i915#3938]: https://gitlab.freedesktop.org/drm/intel/issues/3938
  [i915#3955]: https://gitlab.freedesktop.org/drm/intel/issues/3955
  [i915#4070]: https://gitlab.freedesktop.org/drm/intel/issues/4070
  [i915#4077]: https://gitlab.freedesktop.org/drm/intel/issues/4077
  [i915#4078]: https://gitlab.freedesktop.org/drm/intel/issues/4078
  [i915#4079]: https://gitlab.freedesktop.org/drm/intel/issues/4079
  [i915#4083]: https://gitlab.freedesktop.org/drm/intel/issues/4083
  [i915#4098]: https://gitlab.freedesktop.org/drm/intel/issues/4098
  [i915#4103]: https://gitlab.freedesktop.org/drm/intel/issues/4103
  [i915#4212]: https://gitlab.freedesktop.org/drm/intel/issues/4212
  [i915#4213]: https://gitlab.freedesktop.org/drm/intel/issues/4213
  [i915#4270]: https://gitlab.freedesktop.org/drm/intel/issues/4270
  [i915#4312]: https://gitlab.freedesktop.org/drm/intel/issues/4312
  [i915#4387]: https://gitlab.freedesktop.org/drm/intel/issues/4387
  [i915#4525]: https://gitlab.freedesktop.org/drm/intel/issues/4525
  [i915#4538]: https://gitlab.freedesktop.org/drm/intel/issues/4538
  [i915#4565]: https://gitlab.freedesktop.org/drm/intel/issues/4565
  [i915#4613]: https://gitlab.freedesktop.org/drm/intel/issues/4613
  [i915#4767]: https://gitlab.freedesktop.org/drm/intel/issues/4767
  [i915#4812]: https://gitlab.freedesktop.org/drm/intel/issues/4812
  [i915#4833]: https://gitlab.freedesktop.org/drm/intel/issues/4833
  [i915#4852]: https://gitlab.freedesktop.org/drm/intel/issues/4852
  [i915#4860]: https://gitlab.freedesktop.org/drm/intel/issues/4860
  [i915#4880]: https://gitlab.freedesktop.org/drm/intel/issues/4880
  [i915#4881]: https://gitlab.freedesktop.org/drm/intel/issues/4881
  [i915#5176]: https://gitlab.freedesktop.org/drm/intel/issues/5176
  [i915#5213]: https://gitlab.freedesktop.org/drm/intel/issues/5213
  [i915#5235]: https://gitlab.freedesktop.org/drm/intel/issues/5235
  [i915#5286]: https://gitlab.freedesktop.org/drm/intel/issues/5286
  [i915#5288]: https://gitlab.freedesktop.org/drm/intel/issues/5288
  [i915#5289]: https://gitlab.freedesktop.org/drm/intel/issues/5289
  [i915#5325]: https://gitlab.freedesktop.org/drm/intel/issues/5325
  [i915#533]: https://gitlab.freedesktop.org/drm/intel/issues/533
  [i915#5439]: https://gitlab.freedesktop.org/drm/intel/issues/5439
  [i915#5461]: https://gitlab.freedesktop.org/drm/intel/issues/5461
  [i915#5563]: https://gitlab.freedesktop.org/drm/intel/issues/5563
  [i915#5784]: https://gitlab.freedesktop.org/drm/intel/issues/5784
  [i915#6095]: https://gitlab.freedesktop.org/drm/intel/issues/6095
  [i915#6230]: https://gitlab.freedesktop.org/drm/intel/issues/6230
  [i915#6247]: https://gitlab.freedesktop.org/drm/intel/issues/6247
  [i915#6248]: https://gitlab.freedesktop.org/drm/intel/issues/6248
  [i915#6268]: https://gitlab.freedesktop.org/drm/intel/issues/6268
  [i915#6301]: https://gitlab.freedesktop.org/drm/intel/issues/6301
  [i915#6335]: https://gitlab.freedesktop.org/drm/intel/issues/6335
  [i915#6433]: https://gitlab.freedesktop.org/drm/intel/issues/6433
  [i915#6497]: https://gitlab.freedesktop.org/drm/intel/issues/6497
  [i915#6524]: https://gitlab.freedesktop.org/drm/intel/issues/6524
  [i915#658]: https://gitlab.freedesktop.org/drm/intel/issues/658
  [i915#6590]: https://gitlab.freedesktop.org/drm/intel/issues/6590
  [i915#6621]: https://gitlab.freedesktop.org/drm/intel/issues/6621
  [i915#6768]: https://gitlab.freedesktop.org/drm/intel/issues/6768
  [i915#6944]: https://gitlab.freedesktop.org/drm/intel/issues/6944
  [i915#6946]: https://gitlab.freedesktop.org/drm/intel/issues/6946
  [i915#7116]: https://gitlab.freedesktop.org/drm/intel/issues/7116
  [i915#7118]: https://gitlab.freedesktop.org/drm/intel/issues/7118
  [i915#7456]: https://gitlab.freedesktop.org/drm/intel/issues/7456
  [i915#7561]: https://gitlab.freedesktop.org/drm/intel/issues/7561
  [i915#7651]: https://gitlab.freedesktop.org/drm/intel/issues/7651
  [i915#7697]: https://gitlab.freedesktop.org/drm/intel/issues/7697
  [i915#7701]: https://gitlab.freedesktop.org/drm/intel/issues/7701
  [i915#7707]: https://gitlab.freedesktop.org/drm/intel/issues/7707
  [i915#7711]: https://gitlab.freedesktop.org/drm/intel/issues/7711
  [i915#7742]: https://gitlab.freedesktop.org/drm/intel/issues/7742
  [i915#7828]: https://gitlab.freedesktop.org/drm/intel/issues/7828
  [i915#7949]: https://gitlab.freedesktop.org/drm/intel/issues/7949


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

  * Linux: CI_DRM_12639 -> Patchwork_113321v1
  * Piglit: piglit_4509 -> None

  CI-20190529: 20190529
  CI_DRM_12639: 81c7c4a9c86dac434bc6d8c97fdc440866ca0d65 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_7136: 31b6af91747ad8c705399c9006cdb81cb1864146 @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
  Patchwork_113321v1: 81c7c4a9c86dac434bc6d8c97fdc440866ca0d65 @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

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

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

Re: [Intel-gfx] [PATCH] drm/i915/psr: Split sel fetch plane configuration into arm and noarm

[Luca Coelho]

On Wed, 2023-01-25 at 12:44 +0200, Jouni Högander wrote:
> > SEL_FETCH_CTL registers are armed immediately when plane is disabled.
> > SEL_FETCH_* instances of plane configuration are used when doing
> > selective update and normal plane register instances for full updates.
> > Currently all SEL_FETCH_* registers are written as a part of noarm
> > plane configuration. If noarm and arm plane configuration are not
> > happening within same vblank we may end up having plane as a part of
> > selective update before it's PLANE_SURF register is written.
> > 
> > Fix this by splitting plane selective fetch configuration into arm and
> > noarm versions and call them accordingly. Write SEL_FETCH_CTL in arm
> > version.
> > 
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: José Roberto de Souza <jose.souza@intel.com>
> > Cc: Mika Kahola <mika.kahola@intel.com>
> > Cc: Vinod Govindapillai <vinod.govindapillai@intel.com>
> > Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> > Signed-off-by: Jouni Högander <jouni.hogander@intel.com>
> > ---

Looks fine to me. A couple of nitpicks.


> >  drivers/gpu/drm/i915/display/intel_cursor.c   |  2 +-
> >  drivers/gpu/drm/i915/display/intel_psr.c      | 29 ++++++++++++++-----
> >  drivers/gpu/drm/i915/display/intel_psr.h      |  6 +++-
> >  .../drm/i915/display/skl_universal_plane.c    |  4 ++-
> >  4 files changed, 30 insertions(+), 11 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_cursor.c b/drivers/gpu/drm/i915/display/intel_cursor.c
> > index d190fa0d393b..50232cec48e0 100644
> > --- a/drivers/gpu/drm/i915/display/intel_cursor.c
> > +++ b/drivers/gpu/drm/i915/display/intel_cursor.c
> > @@ -532,7 +532,7 @@ static void i9xx_cursor_update_arm(struct intel_plane *plane,
> >  		skl_write_cursor_wm(plane, crtc_state);
> >  
> >  	if (plane_state)
> > -		intel_psr2_program_plane_sel_fetch(plane, crtc_state, plane_state, 0);
> > +		intel_psr2_program_plane_sel_fetch_arm(plane, crtc_state, plane_state, 0);

This goes well over 80 chars.  Even though it's accepted to go over
that nowadays, I think it's still preferred to keep it shorter and this
line is easily breakable.


> >  	else
> >  		intel_psr2_disable_plane_sel_fetch(plane, crtc_state);
> >  
> > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c
> > index 7d4a15a283a0..63b79c611932 100644
> > --- a/drivers/gpu/drm/i915/display/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> > @@ -1559,7 +1559,26 @@ void intel_psr2_disable_plane_sel_fetch(struct intel_plane *plane,
> >  	intel_de_write_fw(dev_priv, PLANE_SEL_FETCH_CTL(pipe, plane->id), 0);
> >  }
> >  
> > -void intel_psr2_program_plane_sel_fetch(struct intel_plane *plane,
> > +void intel_psr2_program_plane_sel_fetch_arm(struct intel_plane *plane,
> > +					const struct intel_crtc_state *crtc_state,
> > +					const struct intel_plane_state *plane_state,
> > +					int color_plane)
> > +{
> > +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);

Should you use i915 instead of dev_priv? I've heard and read elsewhere
that this is generally a desired change.  Much easier to use always the
same local name for this kind of thing.  Though this file is already
interspersed with both versions...

Regardless of these nitpicks (change them if you want):

Reviewed-by: Luca Coelho <luciano.coelho@intel.com>

--
Cheers,
Luca.

Re: [Intel-gfx] [PATCH] drm/i915/psr: Split sel fetch plane configuration into arm and noarm

[Jani Nikula]

On Thu, 26 Jan 2023, Luca Coelho <luca@coelho.fi> wrote:
> On Wed, 2023-01-25 at 12:44 +0200, Jouni Högander wrote:
>> > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c
>> > index 7d4a15a283a0..63b79c611932 100644
>> > --- a/drivers/gpu/drm/i915/display/intel_psr.c
>> > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
>> > @@ -1559,7 +1559,26 @@ void intel_psr2_disable_plane_sel_fetch(struct intel_plane *plane,
>> >  	intel_de_write_fw(dev_priv, PLANE_SEL_FETCH_CTL(pipe, plane->id), 0);
>> >  }
>> >  
>> > -void intel_psr2_program_plane_sel_fetch(struct intel_plane *plane,
>> > +void intel_psr2_program_plane_sel_fetch_arm(struct intel_plane *plane,
>> > +					const struct intel_crtc_state *crtc_state,
>> > +					const struct intel_plane_state *plane_state,
>> > +					int color_plane)
>> > +{
>> > +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
>
> Should you use i915 instead of dev_priv? I've heard and read elsewhere
> that this is generally a desired change.  Much easier to use always the
> same local name for this kind of thing.  Though this file is already
> interspersed with both versions...

Basically the only reason to use dev_priv for new code is to deal with
some register macros that still have implicit dev_priv in
them. Otherwise, i915 should be used, and when convenient, dev_priv
should be converted to i915 while touching the code anyway (in a
separate patch, but while you're there).

The implicit dev_priv dependencies in the register macros are a bit
annoying to fix, and it's been going slow. In retrospect maybe the right
thing would have been to just sed the parameter to all of them
everywhere and be done with it for good. Not too late now, I guess, and
I'd take the patches in a heartbeat if someone were to step up and do
it.


BR,
Jani.


-- 
Jani Nikula, Intel Open Source Graphics Center

Re: [Intel-gfx] [PATCH] drm/i915/psr: Split sel fetch plane configuration into arm and noarm

[Luca Coelho]

On Thu, 2023-01-26 at 14:00 +0200, Jani Nikula wrote:
> On Thu, 26 Jan 2023, Luca Coelho <luca@coelho.fi> wrote:
> > On Wed, 2023-01-25 at 12:44 +0200, Jouni Högander wrote:
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c
> > > > index 7d4a15a283a0..63b79c611932 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_psr.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> > > > @@ -1559,7 +1559,26 @@ void intel_psr2_disable_plane_sel_fetch(struct intel_plane *plane,
> > > >  	intel_de_write_fw(dev_priv, PLANE_SEL_FETCH_CTL(pipe, plane->id), 0);
> > > >  }
> > > >  
> > > > -void intel_psr2_program_plane_sel_fetch(struct intel_plane *plane,
> > > > +void intel_psr2_program_plane_sel_fetch_arm(struct intel_plane *plane,
> > > > +					const struct intel_crtc_state *crtc_state,
> > > > +					const struct intel_plane_state *plane_state,
> > > > +					int color_plane)
> > > > +{
> > > > +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> > 
> > Should you use i915 instead of dev_priv? I've heard and read elsewhere
> > that this is generally a desired change.  Much easier to use always the
> > same local name for this kind of thing.  Though this file is already
> > interspersed with both versions...
> 
> Basically the only reason to use dev_priv for new code is to deal with
> some register macros that still have implicit dev_priv in
> them. Otherwise, i915 should be used, and when convenient, dev_priv
> should be converted to i915 while touching the code anyway (in a
> separate patch, but while you're there).

Thanks for the clarification! In this case we're not using any of the
macros, AFAICT, so I guess it's better to go with i915 already? And I
think it should even be in this same patch, since it's a new function
anyway.


> The implicit dev_priv dependencies in the register macros are a bit
> annoying to fix, and it's been going slow. In retrospect maybe the right
> thing would have been to just sed the parameter to all of them
> everywhere and be done with it for good. Not too late now, I guess, and
> I'd take the patches in a heartbeat if someone were to step up and do
> it.

I see that there is a boatload of register macros using it... I won't
promise, but I think it would be a good exercise for a n00b like me to
make this patch, though I already foresee another boatload of conflicts
with the internal trees and everything...

--
Cheers,
Luca.

Re: [Intel-gfx] [PATCH] drm/i915/psr: Split sel fetch plane configuration into arm and noarm

[Luca Coelho]

On Thu, 2023-01-26 at 14:11 +0200, Luca Coelho wrote:
> On Thu, 2023-01-26 at 14:00 +0200, Jani Nikula wrote:
> > On Thu, 26 Jan 2023, Luca Coelho <luca@coelho.fi> wrote:
> > > On Wed, 2023-01-25 at 12:44 +0200, Jouni Högander wrote:
> > > > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c
> > > > > index 7d4a15a283a0..63b79c611932 100644
> > > > > --- a/drivers/gpu/drm/i915/display/intel_psr.c
> > > > > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> > > > > @@ -1559,7 +1559,26 @@ void intel_psr2_disable_plane_sel_fetch(struct intel_plane *plane,
> > > > >  	intel_de_write_fw(dev_priv, PLANE_SEL_FETCH_CTL(pipe, plane->id), 0);
> > > > >  }
> > > > >  
> > > > > -void intel_psr2_program_plane_sel_fetch(struct intel_plane *plane,
> > > > > +void intel_psr2_program_plane_sel_fetch_arm(struct intel_plane *plane,
> > > > > +					const struct intel_crtc_state *crtc_state,
> > > > > +					const struct intel_plane_state *plane_state,
> > > > > +					int color_plane)
> > > > > +{
> > > > > +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> > > 
> > > Should you use i915 instead of dev_priv? I've heard and read elsewhere
> > > that this is generally a desired change.  Much easier to use always the
> > > same local name for this kind of thing.  Though this file is already
> > > interspersed with both versions...
> > 
> > Basically the only reason to use dev_priv for new code is to deal with
> > some register macros that still have implicit dev_priv in
> > them. Otherwise, i915 should be used, and when convenient, dev_priv
> > should be converted to i915 while touching the code anyway (in a
> > separate patch, but while you're there).
> 
> Thanks for the clarification! In this case we're not using any of the
> macros, AFAICT, so I guess it's better to go with i915 already? And I
> think it should even be in this same patch, since it's a new function
> anyway.
> 
> 
> > The implicit dev_priv dependencies in the register macros are a bit
> > annoying to fix, and it's been going slow. In retrospect maybe the right
> > thing would have been to just sed the parameter to all of them
> > everywhere and be done with it for good. Not too late now, I guess, and
> > I'd take the patches in a heartbeat if someone were to step up and do
> > it.
> 
> I see that there is a boatload of register macros using it... I won't
> promise, but I think it would be a good exercise for a n00b like me to
> make this patch, though I already foresee another boatload of conflicts
> with the internal trees and everything...

There were actually 10 boatloads of places to change:

 187 files changed, 12104 insertions(+), 12104 deletions(-)

...but it _does_ compile. 😄

Do you think this is fine? Lots of shuffle, but if you think it's okay,
I can send the patch out now.

--
Cheers,
Luca.

Re: [Intel-gfx] [PATCH] drm/i915/psr: Split sel fetch plane configuration into arm and noarm

[Jani Nikula]

On Thu, 26 Jan 2023, Luca Coelho <luca@coelho.fi> wrote:
> On Thu, 2023-01-26 at 14:11 +0200, Luca Coelho wrote:
>> On Thu, 2023-01-26 at 14:00 +0200, Jani Nikula wrote:
>> > On Thu, 26 Jan 2023, Luca Coelho <luca@coelho.fi> wrote:
>> > > On Wed, 2023-01-25 at 12:44 +0200, Jouni Högander wrote:
>> > > > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c
>> > > > > index 7d4a15a283a0..63b79c611932 100644
>> > > > > --- a/drivers/gpu/drm/i915/display/intel_psr.c
>> > > > > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
>> > > > > @@ -1559,7 +1559,26 @@ void intel_psr2_disable_plane_sel_fetch(struct intel_plane *plane,
>> > > > >  	intel_de_write_fw(dev_priv, PLANE_SEL_FETCH_CTL(pipe, plane->id), 0);
>> > > > >  }
>> > > > >  
>> > > > > -void intel_psr2_program_plane_sel_fetch(struct intel_plane *plane,
>> > > > > +void intel_psr2_program_plane_sel_fetch_arm(struct intel_plane *plane,
>> > > > > +					const struct intel_crtc_state *crtc_state,
>> > > > > +					const struct intel_plane_state *plane_state,
>> > > > > +					int color_plane)
>> > > > > +{
>> > > > > +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
>> > > 
>> > > Should you use i915 instead of dev_priv? I've heard and read elsewhere
>> > > that this is generally a desired change.  Much easier to use always the
>> > > same local name for this kind of thing.  Though this file is already
>> > > interspersed with both versions...
>> > 
>> > Basically the only reason to use dev_priv for new code is to deal with
>> > some register macros that still have implicit dev_priv in
>> > them. Otherwise, i915 should be used, and when convenient, dev_priv
>> > should be converted to i915 while touching the code anyway (in a
>> > separate patch, but while you're there).
>> 
>> Thanks for the clarification! In this case we're not using any of the
>> macros, AFAICT, so I guess it's better to go with i915 already? And I
>> think it should even be in this same patch, since it's a new function
>> anyway.
>> 
>> 
>> > The implicit dev_priv dependencies in the register macros are a bit
>> > annoying to fix, and it's been going slow. In retrospect maybe the right
>> > thing would have been to just sed the parameter to all of them
>> > everywhere and be done with it for good. Not too late now, I guess, and
>> > I'd take the patches in a heartbeat if someone were to step up and do
>> > it.
>> 
>> I see that there is a boatload of register macros using it... I won't
>> promise, but I think it would be a good exercise for a n00b like me to
>> make this patch, though I already foresee another boatload of conflicts
>> with the internal trees and everything...
>
> There were actually 10 boatloads of places to change:
>
>  187 files changed, 12104 insertions(+), 12104 deletions(-)
>
> ...but it _does_ compile. 😄
>
> Do you think this is fine? Lots of shuffle, but if you think it's okay,
> I can send the patch out now.

Heh, I said I'd take patchES, not everything together! ;)

Rodrigo, Tvrtko, Joonas, thoughts?


BR,
Jani.


-- 
Jani Nikula, Intel Open Source Graphics Center

Re: [Intel-gfx] [PATCH] drm/i915/psr: Split sel fetch plane configuration into arm and noarm

[Lucas De Marchi]

On Thu, Jan 26, 2023 at 06:05:32PM +0200, Jani Nikula wrote:
>On Thu, 26 Jan 2023, Luca Coelho <luca@coelho.fi> wrote:
>> On Thu, 2023-01-26 at 14:11 +0200, Luca Coelho wrote:
>>> On Thu, 2023-01-26 at 14:00 +0200, Jani Nikula wrote:
>>> > On Thu, 26 Jan 2023, Luca Coelho <luca@coelho.fi> wrote:
>>> > > On Wed, 2023-01-25 at 12:44 +0200, Jouni Högander wrote:
>>> > > > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c
>>> > > > > index 7d4a15a283a0..63b79c611932 100644
>>> > > > > --- a/drivers/gpu/drm/i915/display/intel_psr.c
>>> > > > > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
>>> > > > > @@ -1559,7 +1559,26 @@ void intel_psr2_disable_plane_sel_fetch(struct intel_plane *plane,
>>> > > > >  	intel_de_write_fw(dev_priv, PLANE_SEL_FETCH_CTL(pipe, plane->id), 0);
>>> > > > >  }
>>> > > > >
>>> > > > > -void intel_psr2_program_plane_sel_fetch(struct intel_plane *plane,
>>> > > > > +void intel_psr2_program_plane_sel_fetch_arm(struct intel_plane *plane,
>>> > > > > +					const struct intel_crtc_state *crtc_state,
>>> > > > > +					const struct intel_plane_state *plane_state,
>>> > > > > +					int color_plane)
>>> > > > > +{
>>> > > > > +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
>>> > >
>>> > > Should you use i915 instead of dev_priv? I've heard and read elsewhere
>>> > > that this is generally a desired change.  Much easier to use always the
>>> > > same local name for this kind of thing.  Though this file is already
>>> > > interspersed with both versions...
>>> >
>>> > Basically the only reason to use dev_priv for new code is to deal with
>>> > some register macros that still have implicit dev_priv in
>>> > them. Otherwise, i915 should be used, and when convenient, dev_priv
>>> > should be converted to i915 while touching the code anyway (in a
>>> > separate patch, but while you're there).
>>>
>>> Thanks for the clarification! In this case we're not using any of the
>>> macros, AFAICT, so I guess it's better to go with i915 already? And I
>>> think it should even be in this same patch, since it's a new function
>>> anyway.
>>>
>>>
>>> > The implicit dev_priv dependencies in the register macros are a bit
>>> > annoying to fix, and it's been going slow. In retrospect maybe the right
>>> > thing would have been to just sed the parameter to all of them
>>> > everywhere and be done with it for good. Not too late now, I guess, and
>>> > I'd take the patches in a heartbeat if someone were to step up and do
>>> > it.
>>>
>>> I see that there is a boatload of register macros using it... I won't
>>> promise, but I think it would be a good exercise for a n00b like me to
>>> make this patch, though I already foresee another boatload of conflicts
>>> with the internal trees and everything...
>>
>> There were actually 10 boatloads of places to change:
>>
>>  187 files changed, 12104 insertions(+), 12104 deletions(-)
>>
>> ...but it _does_ compile. 😄
>>
>> Do you think this is fine? Lots of shuffle, but if you think it's okay,
>> I can send the patch out now.
>
>Heh, I said I'd take patchES, not everything together! ;)
>
>Rodrigo, Tvrtko, Joonas, thoughts?

If it's a sed or something that can be automated, I think it could be
ok as single patch as long as we find the right time to generate it,
when the trees are in sync.

I do remember doing a sed s/dev_priv/i915/ (or it was with a cocci
script, don't remember) a few years ago, and I'm
glad we are giving up the slow conversion and just ripping the
bandaid.

Lucas De Marchi

Re: [Intel-gfx] [PATCH] drm/i915/psr: Split sel fetch plane configuration into arm and noarm

[Rodrigo Vivi]

On Thu, Jan 26, 2023 at 08:36:42AM -0800, Lucas De Marchi wrote:
> On Thu, Jan 26, 2023 at 06:05:32PM +0200, Jani Nikula wrote:
> > On Thu, 26 Jan 2023, Luca Coelho <luca@coelho.fi> wrote:
> > > On Thu, 2023-01-26 at 14:11 +0200, Luca Coelho wrote:
> > > > On Thu, 2023-01-26 at 14:00 +0200, Jani Nikula wrote:
> > > > > On Thu, 26 Jan 2023, Luca Coelho <luca@coelho.fi> wrote:
> > > > > > On Wed, 2023-01-25 at 12:44 +0200, Jouni Högander wrote:
> > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c
> > > > > > > > index 7d4a15a283a0..63b79c611932 100644
> > > > > > > > --- a/drivers/gpu/drm/i915/display/intel_psr.c
> > > > > > > > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> > > > > > > > @@ -1559,7 +1559,26 @@ void intel_psr2_disable_plane_sel_fetch(struct intel_plane *plane,
> > > > > > > >  	intel_de_write_fw(dev_priv, PLANE_SEL_FETCH_CTL(pipe, plane->id), 0);
> > > > > > > >  }
> > > > > > > >
> > > > > > > > -void intel_psr2_program_plane_sel_fetch(struct intel_plane *plane,
> > > > > > > > +void intel_psr2_program_plane_sel_fetch_arm(struct intel_plane *plane,
> > > > > > > > +					const struct intel_crtc_state *crtc_state,
> > > > > > > > +					const struct intel_plane_state *plane_state,
> > > > > > > > +					int color_plane)
> > > > > > > > +{
> > > > > > > > +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> > > > > >
> > > > > > Should you use i915 instead of dev_priv? I've heard and read elsewhere
> > > > > > that this is generally a desired change.  Much easier to use always the
> > > > > > same local name for this kind of thing.  Though this file is already
> > > > > > interspersed with both versions...
> > > > >
> > > > > Basically the only reason to use dev_priv for new code is to deal with
> > > > > some register macros that still have implicit dev_priv in
> > > > > them. Otherwise, i915 should be used, and when convenient, dev_priv
> > > > > should be converted to i915 while touching the code anyway (in a
> > > > > separate patch, but while you're there).
> > > > 
> > > > Thanks for the clarification! In this case we're not using any of the
> > > > macros, AFAICT, so I guess it's better to go with i915 already? And I
> > > > think it should even be in this same patch, since it's a new function
> > > > anyway.
> > > > 
> > > > 
> > > > > The implicit dev_priv dependencies in the register macros are a bit
> > > > > annoying to fix, and it's been going slow. In retrospect maybe the right
> > > > > thing would have been to just sed the parameter to all of them
> > > > > everywhere and be done with it for good. Not too late now, I guess, and
> > > > > I'd take the patches in a heartbeat if someone were to step up and do
> > > > > it.
> > > > 
> > > > I see that there is a boatload of register macros using it... I won't
> > > > promise, but I think it would be a good exercise for a n00b like me to
> > > > make this patch, though I already foresee another boatload of conflicts
> > > > with the internal trees and everything...
> > > 
> > > There were actually 10 boatloads of places to change:
> > > 
> > >  187 files changed, 12104 insertions(+), 12104 deletions(-)
> > > 
> > > ...but it _does_ compile. 😄
> > > 
> > > Do you think this is fine? Lots of shuffle, but if you think it's okay,
> > > I can send the patch out now.
> > 
> > Heh, I said I'd take patchES, not everything together! ;)
> > 
> > Rodrigo, Tvrtko, Joonas, thoughts?
> 
> If it's a sed or something that can be automated, I think it could be
> ok as single patch as long as we find the right time to generate it,
> when the trees are in sync.
> 
> I do remember doing a sed s/dev_priv/i915/ (or it was with a cocci
> script, don't remember) a few years ago, and I'm
> glad we are giving up the slow conversion and just ripping the
> bandaid.

Well, I honestly was always in favor of this approach if possible
to automate and all... But I do have 2 big concerns here:

1. If we do this we will never ever remove the implicit dependency

2. there will be so many more failures on automagic stable backports.
We will need to scrutinize all the failures and track if the developers
are really following up on the backports. We are already bad at it btw.

Jani, you mentioned offline you were afraid of some implications on
xe if we don't do the sed, what would that be?

Thanks,
Rodrigo.

> 
> Lucas De Marchi

Re: [Intel-gfx] [PATCH] drm/i915/psr: Split sel fetch plane configuration into arm and noarm

[Lucas De Marchi]

On Thu, Jan 26, 2023 at 01:34:40PM -0500, Rodrigo Vivi wrote:
>On Thu, Jan 26, 2023 at 08:36:42AM -0800, Lucas De Marchi wrote:
>> On Thu, Jan 26, 2023 at 06:05:32PM +0200, Jani Nikula wrote:
>> > On Thu, 26 Jan 2023, Luca Coelho <luca@coelho.fi> wrote:
>> > > On Thu, 2023-01-26 at 14:11 +0200, Luca Coelho wrote:
>> > > > On Thu, 2023-01-26 at 14:00 +0200, Jani Nikula wrote:
>> > > > > On Thu, 26 Jan 2023, Luca Coelho <luca@coelho.fi> wrote:
>> > > > > > On Wed, 2023-01-25 at 12:44 +0200, Jouni Högander wrote:
>> > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c
>> > > > > > > > index 7d4a15a283a0..63b79c611932 100644
>> > > > > > > > --- a/drivers/gpu/drm/i915/display/intel_psr.c
>> > > > > > > > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
>> > > > > > > > @@ -1559,7 +1559,26 @@ void intel_psr2_disable_plane_sel_fetch(struct intel_plane *plane,
>> > > > > > > >  	intel_de_write_fw(dev_priv, PLANE_SEL_FETCH_CTL(pipe, plane->id), 0);
>> > > > > > > >  }
>> > > > > > > >
>> > > > > > > > -void intel_psr2_program_plane_sel_fetch(struct intel_plane *plane,
>> > > > > > > > +void intel_psr2_program_plane_sel_fetch_arm(struct intel_plane *plane,
>> > > > > > > > +					const struct intel_crtc_state *crtc_state,
>> > > > > > > > +					const struct intel_plane_state *plane_state,
>> > > > > > > > +					int color_plane)
>> > > > > > > > +{
>> > > > > > > > +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
>> > > > > >
>> > > > > > Should you use i915 instead of dev_priv? I've heard and read elsewhere
>> > > > > > that this is generally a desired change.  Much easier to use always the
>> > > > > > same local name for this kind of thing.  Though this file is already
>> > > > > > interspersed with both versions...
>> > > > >
>> > > > > Basically the only reason to use dev_priv for new code is to deal with
>> > > > > some register macros that still have implicit dev_priv in
>> > > > > them. Otherwise, i915 should be used, and when convenient, dev_priv
>> > > > > should be converted to i915 while touching the code anyway (in a
>> > > > > separate patch, but while you're there).
>> > > >
>> > > > Thanks for the clarification! In this case we're not using any of the
>> > > > macros, AFAICT, so I guess it's better to go with i915 already? And I
>> > > > think it should even be in this same patch, since it's a new function
>> > > > anyway.
>> > > >
>> > > >
>> > > > > The implicit dev_priv dependencies in the register macros are a bit
>> > > > > annoying to fix, and it's been going slow. In retrospect maybe the right
>> > > > > thing would have been to just sed the parameter to all of them
>> > > > > everywhere and be done with it for good. Not too late now, I guess, and
>> > > > > I'd take the patches in a heartbeat if someone were to step up and do
>> > > > > it.
>> > > >
>> > > > I see that there is a boatload of register macros using it... I won't
>> > > > promise, but I think it would be a good exercise for a n00b like me to
>> > > > make this patch, though I already foresee another boatload of conflicts
>> > > > with the internal trees and everything...
>> > >
>> > > There were actually 10 boatloads of places to change:
>> > >
>> > >  187 files changed, 12104 insertions(+), 12104 deletions(-)
>> > >
>> > > ...but it _does_ compile. 😄
>> > >
>> > > Do you think this is fine? Lots of shuffle, but if you think it's okay,
>> > > I can send the patch out now.
>> >
>> > Heh, I said I'd take patchES, not everything together! ;)
>> >
>> > Rodrigo, Tvrtko, Joonas, thoughts?
>>
>> If it's a sed or something that can be automated, I think it could be
>> ok as single patch as long as we find the right time to generate it,
>> when the trees are in sync.
>>
>> I do remember doing a sed s/dev_priv/i915/ (or it was with a cocci
>> script, don't remember) a few years ago, and I'm
>> glad we are giving up the slow conversion and just ripping the
>> bandaid.
>
>Well, I honestly was always in favor of this approach if possible
>to automate and all... But I do have 2 big concerns here:
>
>1. If we do this we will never ever remove the implicit dependency

why? it's pretty easy to see what are the macros with implicity
dependencies, regardless if that implicit dep is called dev_priv or
i915.  Fixing the implicit dependency is the nasty part as it will
touch a lot of places with hard-to-automate-patches.

I still think these are orthogonal issues and we shouldn't block the
dev_priv->i915 rename due to that.

Anyway, I will take a look on what an automated removal of the implicit
dependency would look like.

>2. there will be so many more failures on automagic stable backports.
>We will need to scrutinize all the failures and track if the developers
>are really following up on the backports. We are already bad at it btw.

an stable backport that fails to build due to that but that has a script
to run to fix things up, isn't that bad.

Lucas De Marchi

>Jani, you mentioned offline you were afraid of some implications on
>xe if we don't do the sed, what would that be?
>
>Thanks,
>Rodrigo.
>
>>
>> Lucas De Marchi

Re: [Intel-gfx] [PATCH] drm/i915/psr: Split sel fetch plane configuration into arm and noarm

[Luca Coelho]

On Thu, 2023-01-26 at 11:12 -0800, Lucas De Marchi wrote:
> On Thu, Jan 26, 2023 at 01:34:40PM -0500, Rodrigo Vivi wrote:
> > On Thu, Jan 26, 2023 at 08:36:42AM -0800, Lucas De Marchi wrote:
> > > On Thu, Jan 26, 2023 at 06:05:32PM +0200, Jani Nikula wrote:
> > > > On Thu, 26 Jan 2023, Luca Coelho <luca@coelho.fi> wrote:
> > > > > On Thu, 2023-01-26 at 14:11 +0200, Luca Coelho wrote:
> > > > > > On Thu, 2023-01-26 at 14:00 +0200, Jani Nikula wrote:
> > > > > > > On Thu, 26 Jan 2023, Luca Coelho <luca@coelho.fi> wrote:
> > > > > > > > On Wed, 2023-01-25 at 12:44 +0200, Jouni Högander wrote:
> > > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c
> > > > > > > > > > index 7d4a15a283a0..63b79c611932 100644
> > > > > > > > > > --- a/drivers/gpu/drm/i915/display/intel_psr.c
> > > > > > > > > > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> > > > > > > > > > @@ -1559,7 +1559,26 @@ void intel_psr2_disable_plane_sel_fetch(struct intel_plane *plane,
> > > > > > > > > >  	intel_de_write_fw(dev_priv, PLANE_SEL_FETCH_CTL(pipe, plane->id), 0);
> > > > > > > > > >  }
> > > > > > > > > > 
> > > > > > > > > > -void intel_psr2_program_plane_sel_fetch(struct intel_plane *plane,
> > > > > > > > > > +void intel_psr2_program_plane_sel_fetch_arm(struct intel_plane *plane,
> > > > > > > > > > +					const struct intel_crtc_state *crtc_state,
> > > > > > > > > > +					const struct intel_plane_state *plane_state,
> > > > > > > > > > +					int color_plane)
> > > > > > > > > > +{
> > > > > > > > > > +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> > > > > > > > 
> > > > > > > > Should you use i915 instead of dev_priv? I've heard and read elsewhere
> > > > > > > > that this is generally a desired change.  Much easier to use always the
> > > > > > > > same local name for this kind of thing.  Though this file is already
> > > > > > > > interspersed with both versions...
> > > > > > > 
> > > > > > > Basically the only reason to use dev_priv for new code is to deal with
> > > > > > > some register macros that still have implicit dev_priv in
> > > > > > > them. Otherwise, i915 should be used, and when convenient, dev_priv
> > > > > > > should be converted to i915 while touching the code anyway (in a
> > > > > > > separate patch, but while you're there).
> > > > > > 
> > > > > > Thanks for the clarification! In this case we're not using any of the
> > > > > > macros, AFAICT, so I guess it's better to go with i915 already? And I
> > > > > > think it should even be in this same patch, since it's a new function
> > > > > > anyway.
> > > > > > 
> > > > > > 
> > > > > > > The implicit dev_priv dependencies in the register macros are a bit
> > > > > > > annoying to fix, and it's been going slow. In retrospect maybe the right
> > > > > > > thing would have been to just sed the parameter to all of them
> > > > > > > everywhere and be done with it for good. Not too late now, I guess, and
> > > > > > > I'd take the patches in a heartbeat if someone were to step up and do
> > > > > > > it.
> > > > > > 
> > > > > > I see that there is a boatload of register macros using it... I won't
> > > > > > promise, but I think it would be a good exercise for a n00b like me to
> > > > > > make this patch, though I already foresee another boatload of conflicts
> > > > > > with the internal trees and everything...
> > > > > 
> > > > > There were actually 10 boatloads of places to change:
> > > > > 
> > > > >  187 files changed, 12104 insertions(+), 12104 deletions(-)
> > > > > 
> > > > > ...but it _does_ compile. 😄
> > > > > 
> > > > > Do you think this is fine? Lots of shuffle, but if you think it's okay,
> > > > > I can send the patch out now.
> > > > 
> > > > Heh, I said I'd take patchES, not everything together! ;)
> > > > 
> > > > Rodrigo, Tvrtko, Joonas, thoughts?
> > > 
> > > If it's a sed or something that can be automated, I think it could be
> > > ok as single patch as long as we find the right time to generate it,
> > > when the trees are in sync.
> > > 
> > > I do remember doing a sed s/dev_priv/i915/ (or it was with a cocci
> > > script, don't remember) a few years ago, and I'm
> > > glad we are giving up the slow conversion and just ripping the
> > > bandaid.
> > 
> > Well, I honestly was always in favor of this approach if possible
> > to automate and all... But I do have 2 big concerns here:
> > 
> > 1. If we do this we will never ever remove the implicit dependency
> 
> why? it's pretty easy to see what are the macros with implicity
> dependencies, regardless if that implicit dep is called dev_priv or
> i915.  Fixing the implicit dependency is the nasty part as it will
> touch a lot of places with hard-to-automate-patches.

I don't understand enough to say what these dependencies are, but I
don't see how renaming the symbol, while keeping the type, makes any
difference...


> I still think these are orthogonal issues and we shouldn't block the
> dev_priv->i915 rename due to that.
> 
> Anyway, I will take a look on what an automated removal of the implicit
> dependency would look like.

Orthogonal, I agree.


> > 2. there will be so many more failures on automagic stable backports.
> > We will need to scrutinize all the failures and track if the developers
> > are really following up on the backports. We are already bad at it btw.
> 
> an stable backport that fails to build due to that but that has a script
> to run to fix things up, isn't that bad.

The change is really simple.  If backporting, or any other kind of
patch shuffling happens, it will be very easy to spot and correct.

The only annoying thing in doing this is in git-blame cases, IMHO,
because it's annoying to track back patches just because the line was
changed from dev_priv to i915.


> > Jani, you mentioned offline you were afraid of some implications on
> > xe if we don't do the sed, what would that be?

One big sed (I like the rip off the bandaid metaphor!) is the easiest
way to do this, I think.  Very easy to automate.

Anyway, maybe I should just send the big sed patch, so we can continue
eventual discussions there.  We already hijacked Jouni's patch and took
it to a completely unrelated planet. 😉

--
Cheers,
Luca.

Re: [Intel-gfx] [PATCH] drm/i915/psr: Split sel fetch plane configuration into arm and noarm

[Luca Coelho]

On Thu, 2023-01-26 at 08:36 -0800, Lucas De Marchi wrote:
> On Thu, Jan 26, 2023 at 06:05:32PM +0200, Jani Nikula wrote:
> > On Thu, 26 Jan 2023, Luca Coelho <luca@coelho.fi> wrote:
> > > On Thu, 2023-01-26 at 14:11 +0200, Luca Coelho wrote:
> > > > On Thu, 2023-01-26 at 14:00 +0200, Jani Nikula wrote:
> > > > > On Thu, 26 Jan 2023, Luca Coelho <luca@coelho.fi> wrote:
> > > > > > On Wed, 2023-01-25 at 12:44 +0200, Jouni Högander wrote:
> > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c
> > > > > > > > index 7d4a15a283a0..63b79c611932 100644
> > > > > > > > --- a/drivers/gpu/drm/i915/display/intel_psr.c
> > > > > > > > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> > > > > > > > @@ -1559,7 +1559,26 @@ void intel_psr2_disable_plane_sel_fetch(struct intel_plane *plane,
> > > > > > > >  	intel_de_write_fw(dev_priv, PLANE_SEL_FETCH_CTL(pipe, plane->id), 0);
> > > > > > > >  }
> > > > > > > > 
> > > > > > > > -void intel_psr2_program_plane_sel_fetch(struct intel_plane *plane,
> > > > > > > > +void intel_psr2_program_plane_sel_fetch_arm(struct intel_plane *plane,
> > > > > > > > +					const struct intel_crtc_state *crtc_state,
> > > > > > > > +					const struct intel_plane_state *plane_state,
> > > > > > > > +					int color_plane)
> > > > > > > > +{
> > > > > > > > +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> > > > > > 
> > > > > > Should you use i915 instead of dev_priv? I've heard and read elsewhere
> > > > > > that this is generally a desired change.  Much easier to use always the
> > > > > > same local name for this kind of thing.  Though this file is already
> > > > > > interspersed with both versions...
> > > > > 
> > > > > Basically the only reason to use dev_priv for new code is to deal with
> > > > > some register macros that still have implicit dev_priv in
> > > > > them. Otherwise, i915 should be used, and when convenient, dev_priv
> > > > > should be converted to i915 while touching the code anyway (in a
> > > > > separate patch, but while you're there).
> > > > 
> > > > Thanks for the clarification! In this case we're not using any of the
> > > > macros, AFAICT, so I guess it's better to go with i915 already? And I
> > > > think it should even be in this same patch, since it's a new function
> > > > anyway.
> > > > 
> > > > 
> > > > > The implicit dev_priv dependencies in the register macros are a bit
> > > > > annoying to fix, and it's been going slow. In retrospect maybe the right
> > > > > thing would have been to just sed the parameter to all of them
> > > > > everywhere and be done with it for good. Not too late now, I guess, and
> > > > > I'd take the patches in a heartbeat if someone were to step up and do
> > > > > it.
> > > > 
> > > > I see that there is a boatload of register macros using it... I won't
> > > > promise, but I think it would be a good exercise for a n00b like me to
> > > > make this patch, though I already foresee another boatload of conflicts
> > > > with the internal trees and everything...
> > > 
> > > There were actually 10 boatloads of places to change:
> > > 
> > >  187 files changed, 12104 insertions(+), 12104 deletions(-)
> > > 
> > > ...but it _does_ compile. 😄
> > > 
> > > Do you think this is fine? Lots of shuffle, but if you think it's okay,
> > > I can send the patch out now.
> > 
> > Heh, I said I'd take patchES, not everything together! ;)
> > 
> > Rodrigo, Tvrtko, Joonas, thoughts?
> 
> If it's a sed or something that can be automated, I think it could be
> ok as single patch as long as we find the right time to generate it,
> when the trees are in sync.
> 
> I do remember doing a sed s/dev_priv/i915/ (or it was with a cocci
> script, don't remember) a few years ago, and I'm
> glad we are giving up the slow conversion and just ripping the
> bandaid.

I first started doing it with coccinelle, but it just required too many
rules to justify it.  My idea was to do it bit by bit with coccinelle
in separate patches, but it didn't look great.

A simple sed seems to have done the job very nicely and, since that's
the case, I do agree that doing it in one patch is the way to go.  I
don't see the reason to artificially break it down into several patches
and manually handle all the interdependencies.

--
Cheers,
Luca.

Re: [Intel-gfx] [PATCH] drm/i915/psr: Split sel fetch plane configuration into arm and noarm

[Tvrtko Ursulin]

On 26/01/2023 16:05, Jani Nikula wrote:
> On Thu, 26 Jan 2023, Luca Coelho <luca@coelho.fi> wrote:
>> On Thu, 2023-01-26 at 14:11 +0200, Luca Coelho wrote:
>>> On Thu, 2023-01-26 at 14:00 +0200, Jani Nikula wrote:
>>>> On Thu, 26 Jan 2023, Luca Coelho <luca@coelho.fi> wrote:
>>>>> On Wed, 2023-01-25 at 12:44 +0200, Jouni Högander wrote:
>>>>>>> diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c
>>>>>>> index 7d4a15a283a0..63b79c611932 100644
>>>>>>> --- a/drivers/gpu/drm/i915/display/intel_psr.c
>>>>>>> +++ b/drivers/gpu/drm/i915/display/intel_psr.c
>>>>>>> @@ -1559,7 +1559,26 @@ void intel_psr2_disable_plane_sel_fetch(struct intel_plane *plane,
>>>>>>>   	intel_de_write_fw(dev_priv, PLANE_SEL_FETCH_CTL(pipe, plane->id), 0);
>>>>>>>   }
>>>>>>>   
>>>>>>> -void intel_psr2_program_plane_sel_fetch(struct intel_plane *plane,
>>>>>>> +void intel_psr2_program_plane_sel_fetch_arm(struct intel_plane *plane,
>>>>>>> +					const struct intel_crtc_state *crtc_state,
>>>>>>> +					const struct intel_plane_state *plane_state,
>>>>>>> +					int color_plane)
>>>>>>> +{
>>>>>>> +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
>>>>>
>>>>> Should you use i915 instead of dev_priv? I've heard and read elsewhere
>>>>> that this is generally a desired change.  Much easier to use always the
>>>>> same local name for this kind of thing.  Though this file is already
>>>>> interspersed with both versions...
>>>>
>>>> Basically the only reason to use dev_priv for new code is to deal with
>>>> some register macros that still have implicit dev_priv in
>>>> them. Otherwise, i915 should be used, and when convenient, dev_priv
>>>> should be converted to i915 while touching the code anyway (in a
>>>> separate patch, but while you're there).
>>>
>>> Thanks for the clarification! In this case we're not using any of the
>>> macros, AFAICT, so I guess it's better to go with i915 already? And I
>>> think it should even be in this same patch, since it's a new function
>>> anyway.
>>>
>>>
>>>> The implicit dev_priv dependencies in the register macros are a bit
>>>> annoying to fix, and it's been going slow. In retrospect maybe the right
>>>> thing would have been to just sed the parameter to all of them
>>>> everywhere and be done with it for good. Not too late now, I guess, and
>>>> I'd take the patches in a heartbeat if someone were to step up and do
>>>> it.
>>>
>>> I see that there is a boatload of register macros using it... I won't
>>> promise, but I think it would be a good exercise for a n00b like me to
>>> make this patch, though I already foresee another boatload of conflicts
>>> with the internal trees and everything...
>>
>> There were actually 10 boatloads of places to change:
>>
>>   187 files changed, 12104 insertions(+), 12104 deletions(-)
>>
>> ...but it _does_ compile. 😄
>>
>> Do you think this is fine? Lots of shuffle, but if you think it's okay,
>> I can send the patch out now.
> 
> Heh, I said I'd take patchES, not everything together! ;)
> 
> Rodrigo, Tvrtko, Joonas, thoughts?

IMO if the elimination of implicit dev_priv is not included then I am 
not sure the churn is worth the effort.

I think one trap is that it is easy to assume solving those conflicts is 
easy because there is a script, somewhere, whatever, but one needs to be 
careful with assuming a random person hitting a merge conflict will 
realize there is a script, know where to find it, and know how to use it 
against a state where conflict markers are sitting in their local tree. 
That's a lot of assumed knowledge which my experience tells me is not 
universally there.

Having said all that, I looked at the occurrence histogram for the 
proposed churn and gut feel says conflicts wouldn't even be that bad 
since they seem heavily localized in a handful of files plus the display 
subdir.

Plus it is upstream.. so we are allowed not to care too much about 
backporting woes. I would still hope implicit dev_priv, albeit 
orthogonal, would be coming somewhat together with the rename. For that 
warm fuzzy feeling that the churn was really really worth it.

Regards,

Tvrtko

Re: [Intel-gfx] [PATCH] drm/i915/psr: Split sel fetch plane configuration into arm and noarm

[Jani Nikula]

On Fri, 27 Jan 2023, Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote:
> On 26/01/2023 16:05, Jani Nikula wrote:
>> On Thu, 26 Jan 2023, Luca Coelho <luca@coelho.fi> wrote:
>>> On Thu, 2023-01-26 at 14:11 +0200, Luca Coelho wrote:
>>>> On Thu, 2023-01-26 at 14:00 +0200, Jani Nikula wrote:
>>>>> On Thu, 26 Jan 2023, Luca Coelho <luca@coelho.fi> wrote:
>>>>>> On Wed, 2023-01-25 at 12:44 +0200, Jouni Högander wrote:
>>>>>>>> diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c
>>>>>>>> index 7d4a15a283a0..63b79c611932 100644
>>>>>>>> --- a/drivers/gpu/drm/i915/display/intel_psr.c
>>>>>>>> +++ b/drivers/gpu/drm/i915/display/intel_psr.c
>>>>>>>> @@ -1559,7 +1559,26 @@ void intel_psr2_disable_plane_sel_fetch(struct intel_plane *plane,
>>>>>>>>   	intel_de_write_fw(dev_priv, PLANE_SEL_FETCH_CTL(pipe, plane->id), 0);
>>>>>>>>   }
>>>>>>>>   
>>>>>>>> -void intel_psr2_program_plane_sel_fetch(struct intel_plane *plane,
>>>>>>>> +void intel_psr2_program_plane_sel_fetch_arm(struct intel_plane *plane,
>>>>>>>> +					const struct intel_crtc_state *crtc_state,
>>>>>>>> +					const struct intel_plane_state *plane_state,
>>>>>>>> +					int color_plane)
>>>>>>>> +{
>>>>>>>> +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
>>>>>>
>>>>>> Should you use i915 instead of dev_priv? I've heard and read elsewhere
>>>>>> that this is generally a desired change.  Much easier to use always the
>>>>>> same local name for this kind of thing.  Though this file is already
>>>>>> interspersed with both versions...
>>>>>
>>>>> Basically the only reason to use dev_priv for new code is to deal with
>>>>> some register macros that still have implicit dev_priv in
>>>>> them. Otherwise, i915 should be used, and when convenient, dev_priv
>>>>> should be converted to i915 while touching the code anyway (in a
>>>>> separate patch, but while you're there).
>>>>
>>>> Thanks for the clarification! In this case we're not using any of the
>>>> macros, AFAICT, so I guess it's better to go with i915 already? And I
>>>> think it should even be in this same patch, since it's a new function
>>>> anyway.
>>>>
>>>>
>>>>> The implicit dev_priv dependencies in the register macros are a bit
>>>>> annoying to fix, and it's been going slow. In retrospect maybe the right
>>>>> thing would have been to just sed the parameter to all of them
>>>>> everywhere and be done with it for good. Not too late now, I guess, and
>>>>> I'd take the patches in a heartbeat if someone were to step up and do
>>>>> it.
>>>>
>>>> I see that there is a boatload of register macros using it... I won't
>>>> promise, but I think it would be a good exercise for a n00b like me to
>>>> make this patch, though I already foresee another boatload of conflicts
>>>> with the internal trees and everything...
>>>
>>> There were actually 10 boatloads of places to change:
>>>
>>>   187 files changed, 12104 insertions(+), 12104 deletions(-)
>>>
>>> ...but it _does_ compile. 😄
>>>
>>> Do you think this is fine? Lots of shuffle, but if you think it's okay,
>>> I can send the patch out now.
>> 
>> Heh, I said I'd take patchES, not everything together! ;)
>> 
>> Rodrigo, Tvrtko, Joonas, thoughts?
>
> IMO if the elimination of implicit dev_priv is not included then I am 
> not sure the churn is worth the effort.
>
> I think one trap is that it is easy to assume solving those conflicts is 
> easy because there is a script, somewhere, whatever, but one needs to be 
> careful with assuming a random person hitting a merge conflict will 
> realize there is a script, know where to find it, and know how to use it 
> against a state where conflict markers are sitting in their local tree. 
> That's a lot of assumed knowledge which my experience tells me is not 
> universally there.
>
> Having said all that, I looked at the occurrence histogram for the 
> proposed churn and gut feel says conflicts wouldn't even be that bad 
> since they seem heavily localized in a handful of files plus the display 
> subdir.
>
> Plus it is upstream.. so we are allowed not to care too much about 
> backporting woes. I would still hope implicit dev_priv, albeit 
> orthogonal, would be coming somewhat together with the rename. For that 
> warm fuzzy feeling that the churn was really really worth it.

I was mostly talking about the implicit dev_priv removal. It's somewhat
easy, because you can always assume dev_priv is around when the macros
in question are used.

The above is a dependency to any renames. I don't think the renames are
as important as removing the implicit dev_priv, and the renames are
easier to handle piecemeal, say a file at a time or something.

BR,
Jani.


-- 
Jani Nikula, Intel Open Source Graphics Center

Re: [Intel-gfx] [PATCH] drm/i915/psr: Split sel fetch plane configuration into arm and noarm

[Luca Coelho]

On Fri, 2023-01-27 at 16:37 +0200, Jani Nikula wrote:
> On Fri, 27 Jan 2023, Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote:
> > On 26/01/2023 16:05, Jani Nikula wrote:
> > > On Thu, 26 Jan 2023, Luca Coelho <luca@coelho.fi> wrote:
> > > > On Thu, 2023-01-26 at 14:11 +0200, Luca Coelho wrote:
> > > > > On Thu, 2023-01-26 at 14:00 +0200, Jani Nikula wrote:
> > > > > > On Thu, 26 Jan 2023, Luca Coelho <luca@coelho.fi> wrote:
> > > > > > > On Wed, 2023-01-25 at 12:44 +0200, Jouni Högander wrote:
> > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c
> > > > > > > > > index 7d4a15a283a0..63b79c611932 100644
> > > > > > > > > --- a/drivers/gpu/drm/i915/display/intel_psr.c
> > > > > > > > > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> > > > > > > > > @@ -1559,7 +1559,26 @@ void intel_psr2_disable_plane_sel_fetch(struct intel_plane *plane,
> > > > > > > > >   	intel_de_write_fw(dev_priv, PLANE_SEL_FETCH_CTL(pipe, plane->id), 0);
> > > > > > > > >   }
> > > > > > > > >   
> > > > > > > > > -void intel_psr2_program_plane_sel_fetch(struct intel_plane *plane,
> > > > > > > > > +void intel_psr2_program_plane_sel_fetch_arm(struct intel_plane *plane,
> > > > > > > > > +					const struct intel_crtc_state *crtc_state,
> > > > > > > > > +					const struct intel_plane_state *plane_state,
> > > > > > > > > +					int color_plane)
> > > > > > > > > +{
> > > > > > > > > +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> > > > > > > 
> > > > > > > Should you use i915 instead of dev_priv? I've heard and read elsewhere
> > > > > > > that this is generally a desired change.  Much easier to use always the
> > > > > > > same local name for this kind of thing.  Though this file is already
> > > > > > > interspersed with both versions...
> > > > > > 
> > > > > > Basically the only reason to use dev_priv for new code is to deal with
> > > > > > some register macros that still have implicit dev_priv in
> > > > > > them. Otherwise, i915 should be used, and when convenient, dev_priv
> > > > > > should be converted to i915 while touching the code anyway (in a
> > > > > > separate patch, but while you're there).
> > > > > 
> > > > > Thanks for the clarification! In this case we're not using any of the
> > > > > macros, AFAICT, so I guess it's better to go with i915 already? And I
> > > > > think it should even be in this same patch, since it's a new function
> > > > > anyway.
> > > > > 
> > > > > 
> > > > > > The implicit dev_priv dependencies in the register macros are a bit
> > > > > > annoying to fix, and it's been going slow. In retrospect maybe the right
> > > > > > thing would have been to just sed the parameter to all of them
> > > > > > everywhere and be done with it for good. Not too late now, I guess, and
> > > > > > I'd take the patches in a heartbeat if someone were to step up and do
> > > > > > it.
> > > > > 
> > > > > I see that there is a boatload of register macros using it... I won't
> > > > > promise, but I think it would be a good exercise for a n00b like me to
> > > > > make this patch, though I already foresee another boatload of conflicts
> > > > > with the internal trees and everything...
> > > > 
> > > > There were actually 10 boatloads of places to change:
> > > > 
> > > >   187 files changed, 12104 insertions(+), 12104 deletions(-)
> > > > 
> > > > ...but it _does_ compile. 😄
> > > > 
> > > > Do you think this is fine? Lots of shuffle, but if you think it's okay,
> > > > I can send the patch out now.
> > > 
> > > Heh, I said I'd take patchES, not everything together! ;)
> > > 
> > > Rodrigo, Tvrtko, Joonas, thoughts?
> > 
> > IMO if the elimination of implicit dev_priv is not included then I am 
> > not sure the churn is worth the effort.
> > 
> > I think one trap is that it is easy to assume solving those conflicts is 
> > easy because there is a script, somewhere, whatever, but one needs to be 
> > careful with assuming a random person hitting a merge conflict will 
> > realize there is a script, know where to find it, and know how to use it 
> > against a state where conflict markers are sitting in their local tree. 
> > That's a lot of assumed knowledge which my experience tells me is not 
> > universally there.
> > 
> > Having said all that, I looked at the occurrence histogram for the 
> > proposed churn and gut feel says conflicts wouldn't even be that bad 
> > since they seem heavily localized in a handful of files plus the display 
> > subdir.
> > 
> > Plus it is upstream.. so we are allowed not to care too much about 
> > backporting woes. I would still hope implicit dev_priv, albeit 
> > orthogonal, would be coming somewhat together with the rename. For that 
> > warm fuzzy feeling that the churn was really really worth it.
> 
> I was mostly talking about the implicit dev_priv removal. It's somewhat
> easy, because you can always assume dev_priv is around when the macros
> in question are used.
> 
> The above is a dependency to any renames. I don't think the renames are
> as important as removing the implicit dev_priv, and the renames are
> easier to handle piecemeal, say a file at a time or something.

I'm trying to write a semantic patch to convert this stuff.  But
coccinelle is problematic when it comes to macros, so it turned out not
to be as trivial as I though.

Now that I've been looking at the code more, so I see the issue with
the implicit dev_priv in some of the macros.  But I think that is
really trivial to solve.  It shouldn't be an issue to add a parameter
to those macros.  It will probably need some manual work, but I'm on it
and hopefully will be able to send some patches as RFC.

--
Cheers,
Luca.

Re: [Intel-gfx] [PATCH] drm/i915/psr: Split sel fetch plane configuration into arm and noarm

[Lucas De Marchi]

On Fri, Jan 27, 2023 at 07:12:29PM +0200, Luca Coelho wrote:
>On Fri, 2023-01-27 at 16:37 +0200, Jani Nikula wrote:
>> On Fri, 27 Jan 2023, Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote:
>> > On 26/01/2023 16:05, Jani Nikula wrote:
>> > > On Thu, 26 Jan 2023, Luca Coelho <luca@coelho.fi> wrote:
>> > > > On Thu, 2023-01-26 at 14:11 +0200, Luca Coelho wrote:
>> > > > > On Thu, 2023-01-26 at 14:00 +0200, Jani Nikula wrote:
>> > > > > > On Thu, 26 Jan 2023, Luca Coelho <luca@coelho.fi> wrote:
>> > > > > > > On Wed, 2023-01-25 at 12:44 +0200, Jouni Högander wrote:
>> > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c
>> > > > > > > > > index 7d4a15a283a0..63b79c611932 100644
>> > > > > > > > > --- a/drivers/gpu/drm/i915/display/intel_psr.c
>> > > > > > > > > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
>> > > > > > > > > @@ -1559,7 +1559,26 @@ void intel_psr2_disable_plane_sel_fetch(struct intel_plane *plane,
>> > > > > > > > >   	intel_de_write_fw(dev_priv, PLANE_SEL_FETCH_CTL(pipe, plane->id), 0);
>> > > > > > > > >   }
>> > > > > > > > >
>> > > > > > > > > -void intel_psr2_program_plane_sel_fetch(struct intel_plane *plane,
>> > > > > > > > > +void intel_psr2_program_plane_sel_fetch_arm(struct intel_plane *plane,
>> > > > > > > > > +					const struct intel_crtc_state *crtc_state,
>> > > > > > > > > +					const struct intel_plane_state *plane_state,
>> > > > > > > > > +					int color_plane)
>> > > > > > > > > +{
>> > > > > > > > > +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
>> > > > > > >
>> > > > > > > Should you use i915 instead of dev_priv? I've heard and read elsewhere
>> > > > > > > that this is generally a desired change.  Much easier to use always the
>> > > > > > > same local name for this kind of thing.  Though this file is already
>> > > > > > > interspersed with both versions...
>> > > > > >
>> > > > > > Basically the only reason to use dev_priv for new code is to deal with
>> > > > > > some register macros that still have implicit dev_priv in
>> > > > > > them. Otherwise, i915 should be used, and when convenient, dev_priv
>> > > > > > should be converted to i915 while touching the code anyway (in a
>> > > > > > separate patch, but while you're there).
>> > > > >
>> > > > > Thanks for the clarification! In this case we're not using any of the
>> > > > > macros, AFAICT, so I guess it's better to go with i915 already? And I
>> > > > > think it should even be in this same patch, since it's a new function
>> > > > > anyway.
>> > > > >
>> > > > >
>> > > > > > The implicit dev_priv dependencies in the register macros are a bit
>> > > > > > annoying to fix, and it's been going slow. In retrospect maybe the right
>> > > > > > thing would have been to just sed the parameter to all of them
>> > > > > > everywhere and be done with it for good. Not too late now, I guess, and
>> > > > > > I'd take the patches in a heartbeat if someone were to step up and do
>> > > > > > it.
>> > > > >
>> > > > > I see that there is a boatload of register macros using it... I won't
>> > > > > promise, but I think it would be a good exercise for a n00b like me to
>> > > > > make this patch, though I already foresee another boatload of conflicts
>> > > > > with the internal trees and everything...
>> > > >
>> > > > There were actually 10 boatloads of places to change:
>> > > >
>> > > >   187 files changed, 12104 insertions(+), 12104 deletions(-)
>> > > >
>> > > > ...but it _does_ compile. 😄
>> > > >
>> > > > Do you think this is fine? Lots of shuffle, but if you think it's okay,
>> > > > I can send the patch out now.
>> > >
>> > > Heh, I said I'd take patchES, not everything together! ;)
>> > >
>> > > Rodrigo, Tvrtko, Joonas, thoughts?
>> >
>> > IMO if the elimination of implicit dev_priv is not included then I am
>> > not sure the churn is worth the effort.
>> >
>> > I think one trap is that it is easy to assume solving those conflicts is
>> > easy because there is a script, somewhere, whatever, but one needs to be
>> > careful with assuming a random person hitting a merge conflict will
>> > realize there is a script, know where to find it, and know how to use it
>> > against a state where conflict markers are sitting in their local tree.
>> > That's a lot of assumed knowledge which my experience tells me is not
>> > universally there.
>> >
>> > Having said all that, I looked at the occurrence histogram for the
>> > proposed churn and gut feel says conflicts wouldn't even be that bad
>> > since they seem heavily localized in a handful of files plus the display
>> > subdir.
>> >
>> > Plus it is upstream.. so we are allowed not to care too much about
>> > backporting woes. I would still hope implicit dev_priv, albeit
>> > orthogonal, would be coming somewhat together with the rename. For that
>> > warm fuzzy feeling that the churn was really really worth it.
>>
>> I was mostly talking about the implicit dev_priv removal. It's somewhat
>> easy, because you can always assume dev_priv is around when the macros
>> in question are used.
>>
>> The above is a dependency to any renames. I don't think the renames are
>> as important as removing the implicit dev_priv, and the renames are
>> easier to handle piecemeal, say a file at a time or something.
>
>I'm trying to write a semantic patch to convert this stuff.  But
>coccinelle is problematic when it comes to macros, so it turned out not
>to be as trivial as I though.

I think that the definition in the header is easier to do manually and
let coccinelle change only the users. I started this and it seems to be
going the right direction:

2 prerequisite commits:
https://git.kernel.org/pub/scm/linux/kernel/git/demarchi/linux.git/log/?h=tip-drm-intel-dev-priv

$ cat /tmp/a.cocci
virtual patch

@@
expression e;
@@
- DPLL(e)
+ DPLL(dev_priv, e)

@@
expression e;
@@
- DPLL_MD(e)
+ DPLL_MD(dev_priv, e)

@@
expression e1, e2;
@@
- PALETTE(e1, e2)
+ PALETTE(dev_priv, e1, e2)

... simply continuing with the same pattern for the other macros
I *think* would produce a good result. I slightly tested it with
`make coccicheck MODE=patch COCCI=/tmp/a.cocci  M=./drivers/gpu/drm/i915`

Then if we change the macro in i915_reg.h we could remove all the
implicit deps. Wether we should pass dev_priv or mmio_base I think will
vary from macro to macro.  The rename s/dev_priv/i915/ being the cherry
on top.

Lucas De Marchi

>
>Now that I've been looking at the code more, so I see the issue with
>the implicit dev_priv in some of the macros.  But I think that is
>really trivial to solve.  It shouldn't be an issue to add a parameter
>to those macros.  It will probably need some manual work, but I'm on it
>and hopefully will be able to send some patches as RFC.
>
>--
>Cheers,
>Luca.

Re: [Intel-gfx] [PATCH] drm/i915/psr: Split sel fetch plane configuration into arm and noarm

[Luca Coelho]

On Fri, 2023-01-27 at 11:33 -0800, Lucas De Marchi wrote:
> On Fri, Jan 27, 2023 at 07:12:29PM +0200, Luca Coelho wrote:
> > On Fri, 2023-01-27 at 16:37 +0200, Jani Nikula wrote:
> > > On Fri, 27 Jan 2023, Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote:
> > > > On 26/01/2023 16:05, Jani Nikula wrote:
> > > > > On Thu, 26 Jan 2023, Luca Coelho <luca@coelho.fi> wrote:
> > > > > > On Thu, 2023-01-26 at 14:11 +0200, Luca Coelho wrote:
> > > > > > > On Thu, 2023-01-26 at 14:00 +0200, Jani Nikula wrote:
> > > > > > > > On Thu, 26 Jan 2023, Luca Coelho <luca@coelho.fi> wrote:
> > > > > > > > > On Wed, 2023-01-25 at 12:44 +0200, Jouni Högander wrote:
> > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c
> > > > > > > > > > > index 7d4a15a283a0..63b79c611932 100644
> > > > > > > > > > > --- a/drivers/gpu/drm/i915/display/intel_psr.c
> > > > > > > > > > > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> > > > > > > > > > > @@ -1559,7 +1559,26 @@ void intel_psr2_disable_plane_sel_fetch(struct intel_plane *plane,
> > > > > > > > > > >   	intel_de_write_fw(dev_priv, PLANE_SEL_FETCH_CTL(pipe, plane->id), 0);
> > > > > > > > > > >   }
> > > > > > > > > > > 
> > > > > > > > > > > -void intel_psr2_program_plane_sel_fetch(struct intel_plane *plane,
> > > > > > > > > > > +void intel_psr2_program_plane_sel_fetch_arm(struct intel_plane *plane,
> > > > > > > > > > > +					const struct intel_crtc_state *crtc_state,
> > > > > > > > > > > +					const struct intel_plane_state *plane_state,
> > > > > > > > > > > +					int color_plane)
> > > > > > > > > > > +{
> > > > > > > > > > > +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> > > > > > > > > 
> > > > > > > > > Should you use i915 instead of dev_priv? I've heard and read elsewhere
> > > > > > > > > that this is generally a desired change.  Much easier to use always the
> > > > > > > > > same local name for this kind of thing.  Though this file is already
> > > > > > > > > interspersed with both versions...
> > > > > > > > 
> > > > > > > > Basically the only reason to use dev_priv for new code is to deal with
> > > > > > > > some register macros that still have implicit dev_priv in
> > > > > > > > them. Otherwise, i915 should be used, and when convenient, dev_priv
> > > > > > > > should be converted to i915 while touching the code anyway (in a
> > > > > > > > separate patch, but while you're there).
> > > > > > > 
> > > > > > > Thanks for the clarification! In this case we're not using any of the
> > > > > > > macros, AFAICT, so I guess it's better to go with i915 already? And I
> > > > > > > think it should even be in this same patch, since it's a new function
> > > > > > > anyway.
> > > > > > > 
> > > > > > > 
> > > > > > > > The implicit dev_priv dependencies in the register macros are a bit
> > > > > > > > annoying to fix, and it's been going slow. In retrospect maybe the right
> > > > > > > > thing would have been to just sed the parameter to all of them
> > > > > > > > everywhere and be done with it for good. Not too late now, I guess, and
> > > > > > > > I'd take the patches in a heartbeat if someone were to step up and do
> > > > > > > > it.
> > > > > > > 
> > > > > > > I see that there is a boatload of register macros using it... I won't
> > > > > > > promise, but I think it would be a good exercise for a n00b like me to
> > > > > > > make this patch, though I already foresee another boatload of conflicts
> > > > > > > with the internal trees and everything...
> > > > > > 
> > > > > > There were actually 10 boatloads of places to change:
> > > > > > 
> > > > > >   187 files changed, 12104 insertions(+), 12104 deletions(-)
> > > > > > 
> > > > > > ...but it _does_ compile. 😄
> > > > > > 
> > > > > > Do you think this is fine? Lots of shuffle, but if you think it's okay,
> > > > > > I can send the patch out now.
> > > > > 
> > > > > Heh, I said I'd take patchES, not everything together! ;)
> > > > > 
> > > > > Rodrigo, Tvrtko, Joonas, thoughts?
> > > > 
> > > > IMO if the elimination of implicit dev_priv is not included then I am
> > > > not sure the churn is worth the effort.
> > > > 
> > > > I think one trap is that it is easy to assume solving those conflicts is
> > > > easy because there is a script, somewhere, whatever, but one needs to be
> > > > careful with assuming a random person hitting a merge conflict will
> > > > realize there is a script, know where to find it, and know how to use it
> > > > against a state where conflict markers are sitting in their local tree.
> > > > That's a lot of assumed knowledge which my experience tells me is not
> > > > universally there.
> > > > 
> > > > Having said all that, I looked at the occurrence histogram for the
> > > > proposed churn and gut feel says conflicts wouldn't even be that bad
> > > > since they seem heavily localized in a handful of files plus the display
> > > > subdir.
> > > > 
> > > > Plus it is upstream.. so we are allowed not to care too much about
> > > > backporting woes. I would still hope implicit dev_priv, albeit
> > > > orthogonal, would be coming somewhat together with the rename. For that
> > > > warm fuzzy feeling that the churn was really really worth it.
> > > 
> > > I was mostly talking about the implicit dev_priv removal. It's somewhat
> > > easy, because you can always assume dev_priv is around when the macros
> > > in question are used.
> > > 
> > > The above is a dependency to any renames. I don't think the renames are
> > > as important as removing the implicit dev_priv, and the renames are
> > > easier to handle piecemeal, say a file at a time or something.
> > 
> > I'm trying to write a semantic patch to convert this stuff.  But
> > coccinelle is problematic when it comes to macros, so it turned out not
> > to be as trivial as I though.
> 
> I think that the definition in the header is easier to do manually and
> let coccinelle change only the users. I started this and it seems to be
> going the right direction:
> 
> 2 prerequisite commits:
> https://git.kernel.org/pub/scm/linux/kernel/git/demarchi/linux.git/log/?h=tip-drm-intel-dev-priv
> 
> $ cat /tmp/a.cocci
> virtual patch
> 
> @@
> expression e;
> @@
> - DPLL(e)
> + DPLL(dev_priv, e)
> 
> @@
> expression e;
> @@
> - DPLL_MD(e)
> + DPLL_MD(dev_priv, e)
> 
> @@
> expression e1, e2;
> @@
> - PALETTE(e1, e2)
> + PALETTE(dev_priv, e1, e2)
> 
> ... simply continuing with the same pattern for the other macros
> I *think* would produce a good result. I slightly tested it with
> `make coccicheck MODE=patch COCCI=/tmp/a.cocci  M=./drivers/gpu/drm/i915`

Yes, this probably works, but it is more "seddy" than semantic.  My
goal was to find rules that will change all these occurrences without
having to specify each different patch individually. ;)


> Then if we change the macro in i915_reg.h we could remove all the
> implicit deps. Wether we should pass dev_priv or mmio_base I think will
> vary from macro to macro.  The rename s/dev_priv/i915/ being the cherry
> on top.

Yes, the changes are a bit independent, but we'll have to do one on top
of the other, of course.

--
Cheers,
Luca.

Re: [Intel-gfx] [PATCH] drm/i915/psr: Split sel fetch plane configuration into arm and noarm

[Hogander, Jouni]

On Thu, 2023-01-26 at 13:29 +0200, Luca Coelho wrote:
> On Wed, 2023-01-25 at 12:44 +0200, Jouni Högander wrote:
> > > SEL_FETCH_CTL registers are armed immediately when plane is
> > > disabled.
> > > SEL_FETCH_* instances of plane configuration are used when doing
> > > selective update and normal plane register instances for full
> > > updates.
> > > Currently all SEL_FETCH_* registers are written as a part of
> > > noarm
> > > plane configuration. If noarm and arm plane configuration are not
> > > happening within same vblank we may end up having plane as a part
> > > of
> > > selective update before it's PLANE_SURF register is written.
> > > 
> > > Fix this by splitting plane selective fetch configuration into
> > > arm and
> > > noarm versions and call them accordingly. Write SEL_FETCH_CTL in
> > > arm
> > > version.
> > > 
> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Cc: José Roberto de Souza <jose.souza@intel.com>
> > > Cc: Mika Kahola <mika.kahola@intel.com>
> > > Cc: Vinod Govindapillai <vinod.govindapillai@intel.com>
> > > Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> > > Signed-off-by: Jouni Högander <jouni.hogander@intel.com>
> > > ---
> 
> Looks fine to me. A couple of nitpicks.
> 
> 
> > >  drivers/gpu/drm/i915/display/intel_cursor.c   |  2 +-
> > >  drivers/gpu/drm/i915/display/intel_psr.c      | 29
> > > ++++++++++++++-----
> > >  drivers/gpu/drm/i915/display/intel_psr.h      |  6 +++-
> > >  .../drm/i915/display/skl_universal_plane.c    |  4 ++-
> > >  4 files changed, 30 insertions(+), 11 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/intel_cursor.c
> > > b/drivers/gpu/drm/i915/display/intel_cursor.c
> > > index d190fa0d393b..50232cec48e0 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_cursor.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_cursor.c
> > > @@ -532,7 +532,7 @@ static void i9xx_cursor_update_arm(struct
> > > intel_plane *plane,
> > >                 skl_write_cursor_wm(plane, crtc_state);
> > >  
> > >         if (plane_state)
> > > -               intel_psr2_program_plane_sel_fetch(plane,
> > > crtc_state, plane_state, 0);
> > > +               intel_psr2_program_plane_sel_fetch_arm(plane,
> > > crtc_state, plane_state, 0);
> 
> This goes well over 80 chars.  Even though it's accepted to go over
> that nowadays, I think it's still preferred to keep it shorter and
> this
> line is easily breakable.
> 
> 
> > >         else
> > >                 intel_psr2_disable_plane_sel_fetch(plane,
> > > crtc_state);
> > >  
> > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c
> > > b/drivers/gpu/drm/i915/display/intel_psr.c
> > > index 7d4a15a283a0..63b79c611932 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_psr.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> > > @@ -1559,7 +1559,26 @@ void
> > > intel_psr2_disable_plane_sel_fetch(struct intel_plane *plane,
> > >         intel_de_write_fw(dev_priv, PLANE_SEL_FETCH_CTL(pipe,
> > > plane->id), 0);
> > >  }
> > >  
> > > -void intel_psr2_program_plane_sel_fetch(struct intel_plane
> > > *plane,
> > > +void intel_psr2_program_plane_sel_fetch_arm(struct intel_plane
> > > *plane,
> > > +                                       const struct
> > > intel_crtc_state *crtc_state,
> > > +                                       const struct
> > > intel_plane_state *plane_state,
> > > +                                       int color_plane)
> > > +{
> > > +       struct drm_i915_private *dev_priv = to_i915(plane-
> > > >base.dev);
> 
> Should you use i915 instead of dev_priv? I've heard and read
> elsewhere
> that this is generally a desired change.  Much easier to use always
> the
> same local name for this kind of thing.  Though this file is already
> interspersed with both versions...
> 
> Regardless of these nitpicks (change them if you want):

Thank you Luca for checking my patch. Sent new version addressing your
comments.


> Reviewed-by: Luca Coelho <luciano.coelho@intel.com>
> 
> --
> Cheers,
> Luca.

BR,

Jouni Högander

Re: [Intel-gfx] [PATCH] drm/i915/psr: Split sel fetch plane configuration into arm and noarm

[Govindapillai, Vinod]

On Wed, 2023-01-25 at 12:44 +0200, Jouni Högander wrote:
> SEL_FETCH_CTL registers are armed immediately when plane is disabled.
> SEL_FETCH_* instances of plane configuration are used when doing
> selective update and normal plane register instances for full updates.
> Currently all SEL_FETCH_* registers are written as a part of noarm
> plane configuration. If noarm and arm plane configuration are not
> happening within same vblank we may end up having plane as a part of
> selective update before it's PLANE_SURF register is written.
> 
> Fix this by splitting plane selective fetch configuration into arm and
> noarm versions and call them accordingly. Write SEL_FETCH_CTL in arm
> version.
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: José Roberto de Souza <jose.souza@intel.com>
> Cc: Mika Kahola <mika.kahola@intel.com>
> Cc: Vinod Govindapillai <vinod.govindapillai@intel.com>
> Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> Signed-off-by: Jouni Högander <jouni.hogander@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_cursor.c   |  2 +-
>  drivers/gpu/drm/i915/display/intel_psr.c      | 29 ++++++++++++++-----
>  drivers/gpu/drm/i915/display/intel_psr.h      |  6 +++-
>  .../drm/i915/display/skl_universal_plane.c    |  4 ++-
>  4 files changed, 30 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_cursor.c
> b/drivers/gpu/drm/i915/display/intel_cursor.c
> index d190fa0d393b..50232cec48e0 100644
> --- a/drivers/gpu/drm/i915/display/intel_cursor.c
> +++ b/drivers/gpu/drm/i915/display/intel_cursor.c
> @@ -532,7 +532,7 @@ static void i9xx_cursor_update_arm(struct intel_plane *plane,
>                 skl_write_cursor_wm(plane, crtc_state);
>  
>         if (plane_state)
> -               intel_psr2_program_plane_sel_fetch(plane, crtc_state, plane_state, 0);
> +               intel_psr2_program_plane_sel_fetch_arm(plane, crtc_state, plane_state, 0);

>         else
>                 intel_psr2_disable_plane_sel_fetch(plane, crtc_state);
>  
> diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c
> index 7d4a15a283a0..63b79c611932 100644
> --- a/drivers/gpu/drm/i915/display/intel_psr.c
> +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> @@ -1559,7 +1559,26 @@ void intel_psr2_disable_plane_sel_fetch(struct intel_plane *plane,
>         intel_de_write_fw(dev_priv, PLANE_SEL_FETCH_CTL(pipe, plane->id), 0);
>  }
>  
> -void intel_psr2_program_plane_sel_fetch(struct intel_plane *plane,
> +void intel_psr2_program_plane_sel_fetch_arm(struct intel_plane *plane,
> +                                       const struct intel_crtc_state *crtc_state,
> +                                       const struct intel_plane_state *plane_state,
> +                                       int color_plane)
Looks like color_plane is redundant here.

Otherwise, looks good to me.

Reviewed-by: Vinod Govindapillai <vinod.govindapillai@intel.com>

> +{
> +       struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> +       enum pipe pipe = plane->pipe;
> +
> +       if (!crtc_state->enable_psr2_sel_fetch)
> +               return;
> +
> +       if (plane->id == PLANE_CURSOR)
> +               intel_de_write_fw(dev_priv, PLANE_SEL_FETCH_CTL(pipe, plane->id),
> +                                 plane_state->ctl);
> +       else
> +               intel_de_write_fw(dev_priv, PLANE_SEL_FETCH_CTL(pipe, plane->id),
> +                                 PLANE_SEL_FETCH_CTL_ENABLE);
> +}
> +
> +void intel_psr2_program_plane_sel_fetch_noarm(struct intel_plane *plane,
>                                         const struct intel_crtc_state *crtc_state,
>                                         const struct intel_plane_state *plane_state,
>                                         int color_plane)
> @@ -1573,11 +1592,8 @@ void intel_psr2_program_plane_sel_fetch(struct intel_plane *plane,
>         if (!crtc_state->enable_psr2_sel_fetch)
>                 return;
>  
> -       if (plane->id == PLANE_CURSOR) {
> -               intel_de_write_fw(dev_priv, PLANE_SEL_FETCH_CTL(pipe, plane->id),
> -                                 plane_state->ctl);
> +       if (plane->id == PLANE_CURSOR)
>                 return;
> -       }
>  
>         clip = &plane_state->psr2_sel_fetch_area;
>  
> @@ -1605,9 +1621,6 @@ void intel_psr2_program_plane_sel_fetch(struct intel_plane *plane,
>         val = (drm_rect_height(clip) - 1) << 16;
>         val |= (drm_rect_width(&plane_state->uapi.src) >> 16) - 1;
>         intel_de_write_fw(dev_priv, PLANE_SEL_FETCH_SIZE(pipe, plane->id), val);
> -
> -       intel_de_write_fw(dev_priv, PLANE_SEL_FETCH_CTL(pipe, plane->id),
> -                         PLANE_SEL_FETCH_CTL_ENABLE);
>  }
>  
>  void intel_psr2_program_trans_man_trk_ctl(const struct intel_crtc_state *crtc_state)
> diff --git a/drivers/gpu/drm/i915/display/intel_psr.h b/drivers/gpu/drm/i915/display/intel_psr.h
> index 2ac3a46cccc5..49cd5beacf98 100644
> --- a/drivers/gpu/drm/i915/display/intel_psr.h
> +++ b/drivers/gpu/drm/i915/display/intel_psr.h
> @@ -46,7 +46,11 @@ bool intel_psr_enabled(struct intel_dp *intel_dp);
>  int intel_psr2_sel_fetch_update(struct intel_atomic_state *state,
>                                 struct intel_crtc *crtc);
>  void intel_psr2_program_trans_man_trk_ctl(const struct intel_crtc_state *crtc_state);
> -void intel_psr2_program_plane_sel_fetch(struct intel_plane *plane,
> +void intel_psr2_program_plane_sel_fetch_noarm(struct intel_plane *plane,
> +                                       const struct intel_crtc_state *crtc_state,
> +                                       const struct intel_plane_state *plane_state,
> +                                       int color_plane);
> +void intel_psr2_program_plane_sel_fetch_arm(struct intel_plane *plane,
>                                         const struct intel_crtc_state *crtc_state,
>                                         const struct intel_plane_state *plane_state,
>                                         int color_plane);
> diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c
> b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> index 9b172a1e90de..5a309a3e2812 100644
> --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
> +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> @@ -1260,7 +1260,7 @@ icl_plane_update_noarm(struct intel_plane *plane,
>         if (plane_state->force_black)
>                 icl_plane_csc_load_black(plane);
>  
> -       intel_psr2_program_plane_sel_fetch(plane, crtc_state, plane_state, color_plane);
> +       intel_psr2_program_plane_sel_fetch_noarm(plane, crtc_state, plane_state, color_plane);
>  }
>  
>  static void
> @@ -1287,6 +1287,8 @@ icl_plane_update_arm(struct intel_plane *plane,
>         if (plane_state->scaler_id >= 0)
>                 skl_program_plane_scaler(plane, crtc_state, plane_state);
>  
> +       intel_psr2_program_plane_sel_fetch_arm(plane, crtc_state, plane_state, color_plane);
> +
>         /*
>          * The control register self-arms if the plane was previously
>          * disabled. Try to make the plane enable atomic by writing

Re: [Intel-gfx] [PATCH] drm/i915/psr: Split sel fetch plane configuration into arm and noarm

[Hogander, Jouni]

On Thu, 2023-01-26 at 13:01 +0000, Govindapillai, Vinod wrote:
> On Wed, 2023-01-25 at 12:44 +0200, Jouni Högander wrote:
> > SEL_FETCH_CTL registers are armed immediately when plane is
> > disabled.
> > SEL_FETCH_* instances of plane configuration are used when doing
> > selective update and normal plane register instances for full
> > updates.
> > Currently all SEL_FETCH_* registers are written as a part of noarm
> > plane configuration. If noarm and arm plane configuration are not
> > happening within same vblank we may end up having plane as a part
> > of
> > selective update before it's PLANE_SURF register is written.
> > 
> > Fix this by splitting plane selective fetch configuration into arm
> > and
> > noarm versions and call them accordingly. Write SEL_FETCH_CTL in
> > arm
> > version.
> > 
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: José Roberto de Souza <jose.souza@intel.com>
> > Cc: Mika Kahola <mika.kahola@intel.com>
> > Cc: Vinod Govindapillai <vinod.govindapillai@intel.com>
> > Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> > Signed-off-by: Jouni Högander <jouni.hogander@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_cursor.c   |  2 +-
> >  drivers/gpu/drm/i915/display/intel_psr.c      | 29 ++++++++++++++-
> > ----
> >  drivers/gpu/drm/i915/display/intel_psr.h      |  6 +++-
> >  .../drm/i915/display/skl_universal_plane.c    |  4 ++-
> >  4 files changed, 30 insertions(+), 11 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_cursor.c
> > b/drivers/gpu/drm/i915/display/intel_cursor.c
> > index d190fa0d393b..50232cec48e0 100644
> > --- a/drivers/gpu/drm/i915/display/intel_cursor.c
> > +++ b/drivers/gpu/drm/i915/display/intel_cursor.c
> > @@ -532,7 +532,7 @@ static void i9xx_cursor_update_arm(struct
> > intel_plane *plane,
> >                 skl_write_cursor_wm(plane, crtc_state);
> >  
> >         if (plane_state)
> > -               intel_psr2_program_plane_sel_fetch(plane,
> > crtc_state, plane_state, 0);
> > +               intel_psr2_program_plane_sel_fetch_arm(plane,
> > crtc_state, plane_state, 0);
> 
> >         else
> >                 intel_psr2_disable_plane_sel_fetch(plane,
> > crtc_state);
> >  
> > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c
> > b/drivers/gpu/drm/i915/display/intel_psr.c
> > index 7d4a15a283a0..63b79c611932 100644
> > --- a/drivers/gpu/drm/i915/display/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> > @@ -1559,7 +1559,26 @@ void
> > intel_psr2_disable_plane_sel_fetch(struct intel_plane *plane,
> >         intel_de_write_fw(dev_priv, PLANE_SEL_FETCH_CTL(pipe,
> > plane->id), 0);
> >  }
> >  
> > -void intel_psr2_program_plane_sel_fetch(struct intel_plane *plane,
> > +void intel_psr2_program_plane_sel_fetch_arm(struct intel_plane
> > *plane,
> > +                                       const struct
> > intel_crtc_state *crtc_state,
> > +                                       const struct
> > intel_plane_state *plane_state,
> > +                                       int color_plane)
> Looks like color_plane is redundant here.
> 
> Otherwise, looks good to me.

Thank you Vinod for checking my patch. There is a new version
addressing your comment.

> 
> Reviewed-by: Vinod Govindapillai <vinod.govindapillai@intel.com>
> 
> > +{
> > +       struct drm_i915_private *dev_priv = to_i915(plane-
> > >base.dev);
> > +       enum pipe pipe = plane->pipe;
> > +
> > +       if (!crtc_state->enable_psr2_sel_fetch)
> > +               return;
> > +
> > +       if (plane->id == PLANE_CURSOR)
> > +               intel_de_write_fw(dev_priv,
> > PLANE_SEL_FETCH_CTL(pipe, plane->id),
> > +                                 plane_state->ctl);
> > +       else
> > +               intel_de_write_fw(dev_priv,
> > PLANE_SEL_FETCH_CTL(pipe, plane->id),
> > +                                 PLANE_SEL_FETCH_CTL_ENABLE);
> > +}
> > +
> > +void intel_psr2_program_plane_sel_fetch_noarm(struct intel_plane
> > *plane,
> >                                         const struct
> > intel_crtc_state *crtc_state,
> >                                         const struct
> > intel_plane_state *plane_state,
> >                                         int color_plane)
> > @@ -1573,11 +1592,8 @@ void
> > intel_psr2_program_plane_sel_fetch(struct intel_plane *plane,
> >         if (!crtc_state->enable_psr2_sel_fetch)
> >                 return;
> >  
> > -       if (plane->id == PLANE_CURSOR) {
> > -               intel_de_write_fw(dev_priv,
> > PLANE_SEL_FETCH_CTL(pipe, plane->id),
> > -                                 plane_state->ctl);
> > +       if (plane->id == PLANE_CURSOR)
> >                 return;
> > -       }
> >  
> >         clip = &plane_state->psr2_sel_fetch_area;
> >  
> > @@ -1605,9 +1621,6 @@ void
> > intel_psr2_program_plane_sel_fetch(struct intel_plane *plane,
> >         val = (drm_rect_height(clip) - 1) << 16;
> >         val |= (drm_rect_width(&plane_state->uapi.src) >> 16) - 1;
> >         intel_de_write_fw(dev_priv, PLANE_SEL_FETCH_SIZE(pipe,
> > plane->id), val);
> > -
> > -       intel_de_write_fw(dev_priv, PLANE_SEL_FETCH_CTL(pipe,
> > plane->id),
> > -                         PLANE_SEL_FETCH_CTL_ENABLE);
> >  }
> >  
> >  void intel_psr2_program_trans_man_trk_ctl(const struct
> > intel_crtc_state *crtc_state)
> > diff --git a/drivers/gpu/drm/i915/display/intel_psr.h
> > b/drivers/gpu/drm/i915/display/intel_psr.h
> > index 2ac3a46cccc5..49cd5beacf98 100644
> > --- a/drivers/gpu/drm/i915/display/intel_psr.h
> > +++ b/drivers/gpu/drm/i915/display/intel_psr.h
> > @@ -46,7 +46,11 @@ bool intel_psr_enabled(struct intel_dp
> > *intel_dp);
> >  int intel_psr2_sel_fetch_update(struct intel_atomic_state *state,
> >                                 struct intel_crtc *crtc);
> >  void intel_psr2_program_trans_man_trk_ctl(const struct
> > intel_crtc_state *crtc_state);
> > -void intel_psr2_program_plane_sel_fetch(struct intel_plane *plane,
> > +void intel_psr2_program_plane_sel_fetch_noarm(struct intel_plane
> > *plane,
> > +                                       const struct
> > intel_crtc_state *crtc_state,
> > +                                       const struct
> > intel_plane_state *plane_state,
> > +                                       int color_plane);
> > +void intel_psr2_program_plane_sel_fetch_arm(struct intel_plane
> > *plane,
> >                                         const struct
> > intel_crtc_state *crtc_state,
> >                                         const struct
> > intel_plane_state *plane_state,
> >                                         int color_plane);
> > diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > index 9b172a1e90de..5a309a3e2812 100644
> > --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > @@ -1260,7 +1260,7 @@ icl_plane_update_noarm(struct intel_plane
> > *plane,
> >         if (plane_state->force_black)
> >                 icl_plane_csc_load_black(plane);
> >  
> > -       intel_psr2_program_plane_sel_fetch(plane, crtc_state,
> > plane_state, color_plane);
> > +       intel_psr2_program_plane_sel_fetch_noarm(plane, crtc_state,
> > plane_state, color_plane);
> >  }
> >  
> >  static void
> > @@ -1287,6 +1287,8 @@ icl_plane_update_arm(struct intel_plane
> > *plane,
> >         if (plane_state->scaler_id >= 0)
> >                 skl_program_plane_scaler(plane, crtc_state,
> > plane_state);
> >  
> > +       intel_psr2_program_plane_sel_fetch_arm(plane, crtc_state,
> > plane_state, color_plane);
> > +
> >         /*
> >          * The control register self-arms if the plane was
> > previously
> >          * disabled. Try to make the plane enable atomic by writing
> 

BR,

Jouni Högander