* [Intel-gfx] [PATCH] drm/i915: Fix the disabling sequence for Bigjoiner
@ 2023-05-25 10:10 Stanislav Lisovskiy
2023-05-25 19:08 ` [Intel-gfx] ✓ Fi.CI.BAT: success for " Patchwork
` (3 more replies)
0 siblings, 4 replies; 12+ messages in thread
From: Stanislav Lisovskiy @ 2023-05-25 10:10 UTC (permalink / raw)
To: intel-gfx; +Cc: jani.nikula
According to BSpec 49190, when enabling crtcs, we first setup
slave and then master crtc, however for disabling it should go
vice versa, i.e first master, then slave, however current code
does disabling in a same way as enabling. Fix this, by skipping
non-master crtcs, instead of non-slaves.
Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
---
drivers/gpu/drm/i915/display/intel_display.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index 0490c6412ab5..68958ba0ef49 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -6662,7 +6662,7 @@ static void intel_commit_modeset_disables(struct intel_atomic_state *state)
*/
if (!is_trans_port_sync_slave(old_crtc_state) &&
!intel_dp_mst_is_slave_trans(old_crtc_state) &&
- !intel_crtc_is_bigjoiner_slave(old_crtc_state))
+ !intel_crtc_is_bigjoiner_master(old_crtc_state))
continue;
intel_old_crtc_state_disables(state, old_crtc_state,
--
2.37.3
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [Intel-gfx] ✓ Fi.CI.BAT: success for drm/i915: Fix the disabling sequence for Bigjoiner
2023-05-25 10:10 [Intel-gfx] [PATCH] drm/i915: Fix the disabling sequence for Bigjoiner Stanislav Lisovskiy
@ 2023-05-25 19:08 ` Patchwork
2023-05-26 4:15 ` [Intel-gfx] ✓ Fi.CI.IGT: " Patchwork
` (2 subsequent siblings)
3 siblings, 0 replies; 12+ messages in thread
From: Patchwork @ 2023-05-25 19:08 UTC (permalink / raw)
To: Stanislav Lisovskiy; +Cc: intel-gfx
[-- Attachment #1: Type: text/plain, Size: 8167 bytes --]
== Series Details ==
Series: drm/i915: Fix the disabling sequence for Bigjoiner
URL : https://patchwork.freedesktop.org/series/118360/
State : success
== Summary ==
CI Bug Log - changes from CI_DRM_13187 -> Patchwork_118360v1
====================================================
Summary
-------
**SUCCESS**
No regressions found.
External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118360v1/index.html
Participating hosts (39 -> 38)
------------------------------
Additional (1): fi-tgl-1115g4
Missing (2): bat-mtlp-8 fi-snb-2520m
Known issues
------------
Here are the changes found in Patchwork_118360v1 that come from known issues:
### IGT changes ###
#### Issues hit ####
* igt@debugfs_test@basic-hwmon:
- fi-tgl-1115g4: NOTRUN -> [SKIP][1] ([i915#7456])
[1]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118360v1/fi-tgl-1115g4/igt@debugfs_test@basic-hwmon.html
* igt@gem_huc_copy@huc-copy:
- fi-tgl-1115g4: NOTRUN -> [SKIP][2] ([i915#2190])
[2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118360v1/fi-tgl-1115g4/igt@gem_huc_copy@huc-copy.html
* igt@gem_lmem_swapping@parallel-random-engines:
- fi-tgl-1115g4: NOTRUN -> [SKIP][3] ([i915#4613]) +3 similar issues
[3]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118360v1/fi-tgl-1115g4/igt@gem_lmem_swapping@parallel-random-engines.html
* igt@i915_pm_backlight@basic-brightness:
- fi-tgl-1115g4: NOTRUN -> [SKIP][4] ([i915#3546] / [i915#7561])
[4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118360v1/fi-tgl-1115g4/igt@i915_pm_backlight@basic-brightness.html
* igt@i915_pm_backlight@basic-brightness@edp-1:
- bat-rplp-1: NOTRUN -> [ABORT][5] ([i915#7077])
[5]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118360v1/bat-rplp-1/igt@i915_pm_backlight@basic-brightness@edp-1.html
* igt@i915_selftest@live@slpc:
- bat-rpls-2: NOTRUN -> [DMESG-WARN][6] ([i915#6367])
[6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118360v1/bat-rpls-2/igt@i915_selftest@live@slpc.html
* igt@i915_suspend@basic-s3-without-i915:
- fi-tgl-1115g4: NOTRUN -> [INCOMPLETE][7] ([i915#7443] / [i915#8102])
[7]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118360v1/fi-tgl-1115g4/igt@i915_suspend@basic-s3-without-i915.html
* igt@kms_chamelium_edid@dp-edid-read:
- fi-tgl-1115g4: NOTRUN -> [SKIP][8] ([i915#7828]) +7 similar issues
[8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118360v1/fi-tgl-1115g4/igt@kms_chamelium_edid@dp-edid-read.html
* igt@kms_cursor_legacy@basic-busy-flip-before-cursor-atomic:
- fi-tgl-1115g4: NOTRUN -> [SKIP][9] ([i915#4103]) +1 similar issue
[9]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118360v1/fi-tgl-1115g4/igt@kms_cursor_legacy@basic-busy-flip-before-cursor-atomic.html
* igt@kms_force_connector_basic@force-load-detect:
- fi-tgl-1115g4: NOTRUN -> [SKIP][10] ([fdo#109285])
[10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118360v1/fi-tgl-1115g4/igt@kms_force_connector_basic@force-load-detect.html
* igt@kms_pipe_crc_basic@nonblocking-crc-frame-sequence:
- bat-dg2-11: NOTRUN -> [SKIP][11] ([i915#1845] / [i915#5354]) +2 similar issues
[11]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118360v1/bat-dg2-11/igt@kms_pipe_crc_basic@nonblocking-crc-frame-sequence.html
* igt@kms_pipe_crc_basic@read-crc:
- bat-adlp-9: NOTRUN -> [SKIP][12] ([i915#3546]) +1 similar issue
[12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118360v1/bat-adlp-9/igt@kms_pipe_crc_basic@read-crc.html
* igt@kms_psr@cursor_plane_move:
- fi-tgl-1115g4: NOTRUN -> [SKIP][13] ([fdo#110189]) +3 similar issues
[13]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118360v1/fi-tgl-1115g4/igt@kms_psr@cursor_plane_move.html
* igt@kms_setmode@basic-clone-single-crtc:
- fi-tgl-1115g4: NOTRUN -> [SKIP][14] ([i915#3555] / [i915#4579])
[14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118360v1/fi-tgl-1115g4/igt@kms_setmode@basic-clone-single-crtc.html
#### Possible fixes ####
* igt@i915_selftest@live@guc:
- bat-rpls-1: [DMESG-WARN][15] ([i915#7852]) -> [PASS][16]
[15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13187/bat-rpls-1/igt@i915_selftest@live@guc.html
[16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118360v1/bat-rpls-1/igt@i915_selftest@live@guc.html
* igt@i915_selftest@live@migrate:
- bat-dg2-11: [DMESG-WARN][17] ([i915#7699]) -> [PASS][18]
[17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13187/bat-dg2-11/igt@i915_selftest@live@migrate.html
[18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118360v1/bat-dg2-11/igt@i915_selftest@live@migrate.html
* igt@i915_selftest@live@mman:
- bat-rpls-2: [TIMEOUT][19] ([i915#6794] / [i915#7392]) -> [PASS][20]
[19]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13187/bat-rpls-2/igt@i915_selftest@live@mman.html
[20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118360v1/bat-rpls-2/igt@i915_selftest@live@mman.html
* igt@i915_selftest@live@slpc:
- {bat-mtlp-6}: [DMESG-WARN][21] ([i915#6367]) -> [PASS][22]
[21]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13187/bat-mtlp-6/igt@i915_selftest@live@slpc.html
[22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118360v1/bat-mtlp-6/igt@i915_selftest@live@slpc.html
#### Warnings ####
* igt@kms_setmode@basic-clone-single-crtc:
- bat-rplp-1: [ABORT][23] ([i915#4579] / [i915#8260]) -> [SKIP][24] ([i915#3555] / [i915#4579])
[23]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13187/bat-rplp-1/igt@kms_setmode@basic-clone-single-crtc.html
[24]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118360v1/bat-rplp-1/igt@kms_setmode@basic-clone-single-crtc.html
{name}: This element is suppressed. This means it is ignored when computing
the status of the difference (SUCCESS, WARNING, or FAILURE).
[fdo#109285]: https://bugs.freedesktop.org/show_bug.cgi?id=109285
[fdo#110189]: https://bugs.freedesktop.org/show_bug.cgi?id=110189
[i915#1845]: https://gitlab.freedesktop.org/drm/intel/issues/1845
[i915#2190]: https://gitlab.freedesktop.org/drm/intel/issues/2190
[i915#3546]: https://gitlab.freedesktop.org/drm/intel/issues/3546
[i915#3555]: https://gitlab.freedesktop.org/drm/intel/issues/3555
[i915#4103]: https://gitlab.freedesktop.org/drm/intel/issues/4103
[i915#4579]: https://gitlab.freedesktop.org/drm/intel/issues/4579
[i915#4613]: https://gitlab.freedesktop.org/drm/intel/issues/4613
[i915#5354]: https://gitlab.freedesktop.org/drm/intel/issues/5354
[i915#6367]: https://gitlab.freedesktop.org/drm/intel/issues/6367
[i915#6794]: https://gitlab.freedesktop.org/drm/intel/issues/6794
[i915#7077]: https://gitlab.freedesktop.org/drm/intel/issues/7077
[i915#7392]: https://gitlab.freedesktop.org/drm/intel/issues/7392
[i915#7443]: https://gitlab.freedesktop.org/drm/intel/issues/7443
[i915#7456]: https://gitlab.freedesktop.org/drm/intel/issues/7456
[i915#7561]: https://gitlab.freedesktop.org/drm/intel/issues/7561
[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
[i915#8102]: https://gitlab.freedesktop.org/drm/intel/issues/8102
[i915#8260]: https://gitlab.freedesktop.org/drm/intel/issues/8260
Build changes
-------------
* Linux: CI_DRM_13187 -> Patchwork_118360v1
CI-20190529: 20190529
CI_DRM_13187: e72bc131968e21d9deeae208605481c93581f142 @ git://anongit.freedesktop.org/gfx-ci/linux
IGT_7303: 8f09a9f1da506db907b549bb477f3233b5416733 @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
Patchwork_118360v1: e72bc131968e21d9deeae208605481c93581f142 @ git://anongit.freedesktop.org/gfx-ci/linux
### Linux commits
4982904265ae drm/i915: Fix the disabling sequence for Bigjoiner
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118360v1/index.html
[-- Attachment #2: Type: text/html, Size: 9582 bytes --]
^ permalink raw reply [flat|nested] 12+ messages in thread
* [Intel-gfx] ✓ Fi.CI.IGT: success for drm/i915: Fix the disabling sequence for Bigjoiner
2023-05-25 10:10 [Intel-gfx] [PATCH] drm/i915: Fix the disabling sequence for Bigjoiner Stanislav Lisovskiy
2023-05-25 19:08 ` [Intel-gfx] ✓ Fi.CI.BAT: success for " Patchwork
@ 2023-05-26 4:15 ` Patchwork
2023-07-05 13:26 ` [Intel-gfx] [PATCH] " Luca Coelho
2023-07-05 15:32 ` Imre Deak
3 siblings, 0 replies; 12+ messages in thread
From: Patchwork @ 2023-05-26 4:15 UTC (permalink / raw)
To: Stanislav Lisovskiy; +Cc: intel-gfx
[-- Attachment #1: Type: text/plain, Size: 9940 bytes --]
== Series Details ==
Series: drm/i915: Fix the disabling sequence for Bigjoiner
URL : https://patchwork.freedesktop.org/series/118360/
State : success
== Summary ==
CI Bug Log - changes from CI_DRM_13187_full -> Patchwork_118360v1_full
====================================================
Summary
-------
**SUCCESS**
No regressions found.
Participating hosts (7 -> 7)
------------------------------
No changes in participating hosts
Possible new issues
-------------------
Here are the unknown changes that may have been introduced in Patchwork_118360v1_full:
### IGT changes ###
#### Suppressed ####
The following results come from untrusted machines, tests, or statuses.
They do not affect the overall result.
* igt@gem_ctx_shared@create-shared-gtt:
- {shard-rkl}: [PASS][1] -> [ABORT][2]
[1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13187/shard-rkl-4/igt@gem_ctx_shared@create-shared-gtt.html
[2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118360v1/shard-rkl-4/igt@gem_ctx_shared@create-shared-gtt.html
New tests
---------
New tests have been introduced between CI_DRM_13187_full and Patchwork_118360v1_full:
### New IGT tests (8) ###
* igt@kms_flip@2x-dpms-vs-vblank-race@ab-hdmi-a1-hdmi-a2:
- Statuses : 1 pass(s)
- Exec time: [0.0] s
* igt@kms_flip@2x-dpms-vs-vblank-race@ac-hdmi-a1-hdmi-a2:
- Statuses : 1 pass(s)
- Exec time: [0.0] s
* igt@kms_flip@2x-dpms-vs-vblank-race@bc-hdmi-a1-hdmi-a2:
- Statuses : 1 pass(s)
- Exec time: [0.0] s
* igt@kms_plane_scaling@plane-downscale-with-modifiers-factor-0-75@pipe-a-vga-1:
- Statuses : 1 skip(s)
- Exec time: [0.0] s
* igt@kms_plane_scaling@plane-downscale-with-modifiers-factor-0-75@pipe-b-vga-1:
- Statuses : 1 skip(s)
- Exec time: [0.0] s
* igt@kms_plane_scaling@plane-upscale-with-modifiers-factor-0-25@pipe-a-vga-1:
- Statuses : 1 skip(s)
- Exec time: [0.0] s
* igt@kms_plane_scaling@plane-upscale-with-modifiers-factor-0-25@pipe-b-vga-1:
- Statuses : 1 skip(s)
- Exec time: [0.0] s
* igt@kms_psr2_sf:
- Statuses :
- Exec time: [None] s
Known issues
------------
Here are the changes found in Patchwork_118360v1_full that come from known issues:
### IGT changes ###
#### Issues hit ####
* igt@gem_exec_fair@basic-none-solo@rcs0:
- shard-apl: [PASS][3] -> [FAIL][4] ([i915#2842])
[3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13187/shard-apl2/igt@gem_exec_fair@basic-none-solo@rcs0.html
[4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118360v1/shard-apl7/igt@gem_exec_fair@basic-none-solo@rcs0.html
* igt@i915_selftest@live@gt_heartbeat:
- shard-apl: [PASS][5] -> [DMESG-FAIL][6] ([i915#5334])
[5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13187/shard-apl2/igt@i915_selftest@live@gt_heartbeat.html
[6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118360v1/shard-apl7/igt@i915_selftest@live@gt_heartbeat.html
* igt@kms_plane_scaling@plane-upscale-with-modifiers-20x20@pipe-b-hdmi-a-1:
- shard-snb: NOTRUN -> [SKIP][7] ([fdo#109271] / [i915#4579]) +9 similar issues
[7]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118360v1/shard-snb1/igt@kms_plane_scaling@plane-upscale-with-modifiers-20x20@pipe-b-hdmi-a-1.html
* igt@kms_plane_scaling@planes-upscale-20x20-downscale-factor-0-75@pipe-a-hdmi-a-1:
- shard-snb: NOTRUN -> [SKIP][8] ([fdo#109271]) +12 similar issues
[8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118360v1/shard-snb1/igt@kms_plane_scaling@planes-upscale-20x20-downscale-factor-0-75@pipe-a-hdmi-a-1.html
* igt@perf@stress-open-close@0-rcs0:
- shard-glk: [PASS][9] -> [ABORT][10] ([i915#5213] / [i915#7941])
[9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13187/shard-glk7/igt@perf@stress-open-close@0-rcs0.html
[10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118360v1/shard-glk4/igt@perf@stress-open-close@0-rcs0.html
#### Possible fixes ####
* igt@drm_fdinfo@most-busy-check-all@rcs0:
- {shard-rkl}: [FAIL][11] ([i915#7742]) -> [PASS][12]
[11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13187/shard-rkl-6/igt@drm_fdinfo@most-busy-check-all@rcs0.html
[12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118360v1/shard-rkl-6/igt@drm_fdinfo@most-busy-check-all@rcs0.html
* igt@gem_exec_fair@basic-deadline:
- shard-glk: [FAIL][13] ([i915#2846]) -> [PASS][14]
[13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13187/shard-glk6/igt@gem_exec_fair@basic-deadline.html
[14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118360v1/shard-glk5/igt@gem_exec_fair@basic-deadline.html
* igt@gem_exec_fair@basic-none@vecs0:
- {shard-rkl}: [FAIL][15] ([i915#2842]) -> [PASS][16] +1 similar issue
[15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13187/shard-rkl-1/igt@gem_exec_fair@basic-none@vecs0.html
[16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118360v1/shard-rkl-3/igt@gem_exec_fair@basic-none@vecs0.html
* igt@gem_spin_batch@user-each:
- shard-apl: [FAIL][17] ([i915#2898]) -> [PASS][18]
[17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13187/shard-apl3/igt@gem_spin_batch@user-each.html
[18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118360v1/shard-apl1/igt@gem_spin_batch@user-each.html
* igt@i915_pm_dc@dc6-dpms:
- {shard-tglu}: [FAIL][19] ([i915#3989] / [i915#454]) -> [PASS][20]
[19]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13187/shard-tglu-5/igt@i915_pm_dc@dc6-dpms.html
[20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118360v1/shard-tglu-10/igt@i915_pm_dc@dc6-dpms.html
* igt@i915_pm_rpm@modeset-lpsp-stress-no-wait:
- {shard-rkl}: [SKIP][21] ([i915#1397]) -> [PASS][22]
[21]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13187/shard-rkl-3/igt@i915_pm_rpm@modeset-lpsp-stress-no-wait.html
[22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118360v1/shard-rkl-7/igt@i915_pm_rpm@modeset-lpsp-stress-no-wait.html
* igt@kms_cursor_legacy@flip-vs-cursor-atomic-transitions:
- shard-glk: [FAIL][23] ([IGT#6] / [i915#2346]) -> [PASS][24]
[23]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13187/shard-glk9/igt@kms_cursor_legacy@flip-vs-cursor-atomic-transitions.html
[24]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118360v1/shard-glk6/igt@kms_cursor_legacy@flip-vs-cursor-atomic-transitions.html
* igt@kms_flip@2x-flip-vs-expired-vblank-interruptible@bc-hdmi-a1-hdmi-a2:
- shard-glk: [FAIL][25] ([i915#79]) -> [PASS][26] +1 similar issue
[25]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13187/shard-glk4/igt@kms_flip@2x-flip-vs-expired-vblank-interruptible@bc-hdmi-a1-hdmi-a2.html
[26]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118360v1/shard-glk8/igt@kms_flip@2x-flip-vs-expired-vblank-interruptible@bc-hdmi-a1-hdmi-a2.html
{name}: This element is suppressed. This means it is ignored when computing
the status of the difference (SUCCESS, WARNING, or FAILURE).
[IGT#6]: https://gitlab.freedesktop.org/drm/igt-gpu-tools/issues/6
[fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
[fdo#109285]: https://bugs.freedesktop.org/show_bug.cgi?id=109285
[fdo#110189]: https://bugs.freedesktop.org/show_bug.cgi?id=110189
[i915#1072]: https://gitlab.freedesktop.org/drm/intel/issues/1072
[i915#1397]: https://gitlab.freedesktop.org/drm/intel/issues/1397
[i915#2346]: https://gitlab.freedesktop.org/drm/intel/issues/2346
[i915#2842]: https://gitlab.freedesktop.org/drm/intel/issues/2842
[i915#2846]: https://gitlab.freedesktop.org/drm/intel/issues/2846
[i915#2898]: https://gitlab.freedesktop.org/drm/intel/issues/2898
[i915#315]: https://gitlab.freedesktop.org/drm/intel/issues/315
[i915#3555]: https://gitlab.freedesktop.org/drm/intel/issues/3555
[i915#3591]: https://gitlab.freedesktop.org/drm/intel/issues/3591
[i915#3778]: https://gitlab.freedesktop.org/drm/intel/issues/3778
[i915#3804]: https://gitlab.freedesktop.org/drm/intel/issues/3804
[i915#3955]: https://gitlab.freedesktop.org/drm/intel/issues/3955
[i915#3989]: https://gitlab.freedesktop.org/drm/intel/issues/3989
[i915#4070]: https://gitlab.freedesktop.org/drm/intel/issues/4070
[i915#4078]: https://gitlab.freedesktop.org/drm/intel/issues/4078
[i915#4098]: https://gitlab.freedesktop.org/drm/intel/issues/4098
[i915#454]: https://gitlab.freedesktop.org/drm/intel/issues/454
[i915#4579]: https://gitlab.freedesktop.org/drm/intel/issues/4579
[i915#4816]: https://gitlab.freedesktop.org/drm/intel/issues/4816
[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#5334]: https://gitlab.freedesktop.org/drm/intel/issues/5334
[i915#5784]: https://gitlab.freedesktop.org/drm/intel/issues/5784
[i915#7742]: https://gitlab.freedesktop.org/drm/intel/issues/7742
[i915#79]: https://gitlab.freedesktop.org/drm/intel/issues/79
[i915#7941]: https://gitlab.freedesktop.org/drm/intel/issues/7941
[i915#8304]: https://gitlab.freedesktop.org/drm/intel/issues/8304
Build changes
-------------
* Linux: CI_DRM_13187 -> Patchwork_118360v1
CI-20190529: 20190529
CI_DRM_13187: e72bc131968e21d9deeae208605481c93581f142 @ git://anongit.freedesktop.org/gfx-ci/linux
IGT_7303: 8f09a9f1da506db907b549bb477f3233b5416733 @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
Patchwork_118360v1: e72bc131968e21d9deeae208605481c93581f142 @ 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_118360v1/index.html
[-- Attachment #2: Type: text/html, Size: 10116 bytes --]
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915: Fix the disabling sequence for Bigjoiner
2023-05-25 10:10 [Intel-gfx] [PATCH] drm/i915: Fix the disabling sequence for Bigjoiner Stanislav Lisovskiy
2023-05-25 19:08 ` [Intel-gfx] ✓ Fi.CI.BAT: success for " Patchwork
2023-05-26 4:15 ` [Intel-gfx] ✓ Fi.CI.IGT: " Patchwork
@ 2023-07-05 13:26 ` Luca Coelho
2023-07-05 15:32 ` Imre Deak
3 siblings, 0 replies; 12+ messages in thread
From: Luca Coelho @ 2023-07-05 13:26 UTC (permalink / raw)
To: Stanislav Lisovskiy, intel-gfx; +Cc: jani.nikula
On Thu, 2023-05-25 at 13:10 +0300, Stanislav Lisovskiy wrote:
> According to BSpec 49190, when enabling crtcs, we first setup
> slave and then master crtc, however for disabling it should go
> vice versa, i.e first master, then slave, however current code
> does disabling in a same way as enabling. Fix this, by skipping
> non-master crtcs, instead of non-slaves.
>
> Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> ---
> drivers/gpu/drm/i915/display/intel_display.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index 0490c6412ab5..68958ba0ef49 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -6662,7 +6662,7 @@ static void intel_commit_modeset_disables(struct intel_atomic_state *state)
> */
> if (!is_trans_port_sync_slave(old_crtc_state) &&
> !intel_dp_mst_is_slave_trans(old_crtc_state) &&
> - !intel_crtc_is_bigjoiner_slave(old_crtc_state))
> + !intel_crtc_is_bigjoiner_master(old_crtc_state))
> continue;
>
> intel_old_crtc_state_disables(state, old_crtc_state,
Looks good to me.
Reviewed-by: Luca Coelho <luciano.coelho@intel.com>
--
Cheers,
Luca.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915: Fix the disabling sequence for Bigjoiner
2023-05-25 10:10 [Intel-gfx] [PATCH] drm/i915: Fix the disabling sequence for Bigjoiner Stanislav Lisovskiy
` (2 preceding siblings ...)
2023-07-05 13:26 ` [Intel-gfx] [PATCH] " Luca Coelho
@ 2023-07-05 15:32 ` Imre Deak
2023-07-06 8:24 ` Lisovskiy, Stanislav
2023-07-06 8:33 ` Lisovskiy, Stanislav
3 siblings, 2 replies; 12+ messages in thread
From: Imre Deak @ 2023-07-05 15:32 UTC (permalink / raw)
To: Stanislav Lisovskiy; +Cc: jani.nikula, intel-gfx
On Thu, May 25, 2023 at 01:10:36PM +0300, Stanislav Lisovskiy wrote:
> According to BSpec 49190, when enabling crtcs, we first setup
> slave and then master crtc, however for disabling it should go
> vice versa, i.e first master, then slave, however current code
> does disabling in a same way as enabling. Fix this, by skipping
> non-master crtcs, instead of non-slaves.
>
> Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> ---
> drivers/gpu/drm/i915/display/intel_display.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index 0490c6412ab5..68958ba0ef49 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -6662,7 +6662,7 @@ static void intel_commit_modeset_disables(struct intel_atomic_state *state)
> */
> if (!is_trans_port_sync_slave(old_crtc_state) &&
> !intel_dp_mst_is_slave_trans(old_crtc_state) &&
> - !intel_crtc_is_bigjoiner_slave(old_crtc_state))
> + !intel_crtc_is_bigjoiner_master(old_crtc_state))
I don't see what does this fix. The sequence is correct at the moment
and this change would break it, leaving the encoder PLL enabled
incorrectly when the encoder->post_pll_disable() hook is called. Hence
it's NAK from side.
> continue;
>
> intel_old_crtc_state_disables(state, old_crtc_state,
> --
> 2.37.3
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915: Fix the disabling sequence for Bigjoiner
2023-07-05 15:32 ` Imre Deak
@ 2023-07-06 8:24 ` Lisovskiy, Stanislav
2023-07-06 8:47 ` Imre Deak
2023-07-06 8:33 ` Lisovskiy, Stanislav
1 sibling, 1 reply; 12+ messages in thread
From: Lisovskiy, Stanislav @ 2023-07-06 8:24 UTC (permalink / raw)
To: Imre Deak; +Cc: jani.nikula, intel-gfx
On Wed, Jul 05, 2023 at 06:32:51PM +0300, Imre Deak wrote:
> On Thu, May 25, 2023 at 01:10:36PM +0300, Stanislav Lisovskiy wrote:
> > According to BSpec 49190, when enabling crtcs, we first setup
> > slave and then master crtc, however for disabling it should go
> > vice versa, i.e first master, then slave, however current code
> > does disabling in a same way as enabling. Fix this, by skipping
> > non-master crtcs, instead of non-slaves.
> >
> > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> > ---
> > drivers/gpu/drm/i915/display/intel_display.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > index 0490c6412ab5..68958ba0ef49 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -6662,7 +6662,7 @@ static void intel_commit_modeset_disables(struct intel_atomic_state *state)
> > */
> > if (!is_trans_port_sync_slave(old_crtc_state) &&
> > !intel_dp_mst_is_slave_trans(old_crtc_state) &&
> > - !intel_crtc_is_bigjoiner_slave(old_crtc_state))
> > + !intel_crtc_is_bigjoiner_master(old_crtc_state))
>
> I don't see what does this fix. The sequence is correct at the moment
> and this change would break it, leaving the encoder PLL enabled
> incorrectly when the encoder->post_pll_disable() hook is called. Hence
> it's NAK from side.
Well, as I pointed out the BSpec 49190 instructs us to disable master first,
then slave. Current code skips all non-slaves in first cycle, i.e it disables first slaves
and then masters. Which is _wrong_.
Anything else in particular, where do you need clarifications?
Stan
>
> > continue;
> >
> > intel_old_crtc_state_disables(state, old_crtc_state,
> > --
> > 2.37.3
> >
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915: Fix the disabling sequence for Bigjoiner
2023-07-05 15:32 ` Imre Deak
2023-07-06 8:24 ` Lisovskiy, Stanislav
@ 2023-07-06 8:33 ` Lisovskiy, Stanislav
1 sibling, 0 replies; 12+ messages in thread
From: Lisovskiy, Stanislav @ 2023-07-06 8:33 UTC (permalink / raw)
To: Imre Deak; +Cc: jani.nikula, intel-gfx
On Wed, Jul 05, 2023 at 06:32:51PM +0300, Imre Deak wrote:
> On Thu, May 25, 2023 at 01:10:36PM +0300, Stanislav Lisovskiy wrote:
> > According to BSpec 49190, when enabling crtcs, we first setup
> > slave and then master crtc, however for disabling it should go
> > vice versa, i.e first master, then slave, however current code
> > does disabling in a same way as enabling. Fix this, by skipping
> > non-master crtcs, instead of non-slaves.
> >
> > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> > ---
> > drivers/gpu/drm/i915/display/intel_display.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > index 0490c6412ab5..68958ba0ef49 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -6662,7 +6662,7 @@ static void intel_commit_modeset_disables(struct intel_atomic_state *state)
> > */
> > if (!is_trans_port_sync_slave(old_crtc_state) &&
> > !intel_dp_mst_is_slave_trans(old_crtc_state) &&
> > - !intel_crtc_is_bigjoiner_slave(old_crtc_state))
> > + !intel_crtc_is_bigjoiner_master(old_crtc_state))
>
> I don't see what does this fix. The sequence is correct at the moment
> and this change would break it, leaving the encoder PLL enabled
> incorrectly when the encoder->post_pll_disable() hook is called. Hence
> it's NAK from side.
Citing BSpec:
Follow the steps to disable Planes, Pipe, and Transcoder for pipe B and transcoder B(master)
Follow the steps to Disable Planes and Pipe for pipe C(slave)
Disable port associated with Transcoder B
however because of !intel_crtc_is_bigjoiner_slave(old_crtc_state), we are skipping masters
and do disabling for the slave crtc first.
If fixing that going to break something - that is another story(anyway doesn't mean we shouldn't fix)
Stan
>
> > continue;
> >
> > intel_old_crtc_state_disables(state, old_crtc_state,
> > --
> > 2.37.3
> >
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915: Fix the disabling sequence for Bigjoiner
2023-07-06 8:24 ` Lisovskiy, Stanislav
@ 2023-07-06 8:47 ` Imre Deak
2023-07-06 8:50 ` Lisovskiy, Stanislav
2023-07-06 10:32 ` Lisovskiy, Stanislav
0 siblings, 2 replies; 12+ messages in thread
From: Imre Deak @ 2023-07-06 8:47 UTC (permalink / raw)
To: Lisovskiy, Stanislav; +Cc: jani.nikula, intel-gfx
On Thu, Jul 06, 2023 at 11:24:21AM +0300, Lisovskiy, Stanislav wrote:
> On Wed, Jul 05, 2023 at 06:32:51PM +0300, Imre Deak wrote:
> > On Thu, May 25, 2023 at 01:10:36PM +0300, Stanislav Lisovskiy wrote:
> > > According to BSpec 49190, when enabling crtcs, we first setup
> > > slave and then master crtc, however for disabling it should go
> > > vice versa, i.e first master, then slave, however current code
> > > does disabling in a same way as enabling. Fix this, by skipping
> > > non-master crtcs, instead of non-slaves.
> > >
> > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> > > ---
> > > drivers/gpu/drm/i915/display/intel_display.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > > index 0490c6412ab5..68958ba0ef49 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > @@ -6662,7 +6662,7 @@ static void intel_commit_modeset_disables(struct intel_atomic_state *state)
> > > */
> > > if (!is_trans_port_sync_slave(old_crtc_state) &&
> > > !intel_dp_mst_is_slave_trans(old_crtc_state) &&
> > > - !intel_crtc_is_bigjoiner_slave(old_crtc_state))
> > > + !intel_crtc_is_bigjoiner_master(old_crtc_state))
> >
> > I don't see what does this fix. The sequence is correct at the moment
> > and this change would break it, leaving the encoder PLL enabled
> > incorrectly when the encoder->post_pll_disable() hook is called. Hence
> > it's NAK from side.
>
> Well, as I pointed out the BSpec 49190 instructs us to disable master
> first, then slave. Current code skips all non-slaves in first cycle,
> i.e it disables first slaves and then masters. Which is _wrong_.
This is correct at the moment, followed in the encoder's disable hook
which is only assigned to the master CRTC.
> Anything else in particular, where do you need clarifications?
>
> Stan
>
> >
> > > continue;
> > >
> > > intel_old_crtc_state_disables(state, old_crtc_state,
> > > --
> > > 2.37.3
> > >
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915: Fix the disabling sequence for Bigjoiner
2023-07-06 8:47 ` Imre Deak
@ 2023-07-06 8:50 ` Lisovskiy, Stanislav
2023-07-06 10:32 ` Lisovskiy, Stanislav
1 sibling, 0 replies; 12+ messages in thread
From: Lisovskiy, Stanislav @ 2023-07-06 8:50 UTC (permalink / raw)
To: Imre Deak; +Cc: jani.nikula, intel-gfx
On Thu, Jul 06, 2023 at 11:47:26AM +0300, Imre Deak wrote:
> On Thu, Jul 06, 2023 at 11:24:21AM +0300, Lisovskiy, Stanislav wrote:
> > On Wed, Jul 05, 2023 at 06:32:51PM +0300, Imre Deak wrote:
> > > On Thu, May 25, 2023 at 01:10:36PM +0300, Stanislav Lisovskiy wrote:
> > > > According to BSpec 49190, when enabling crtcs, we first setup
> > > > slave and then master crtc, however for disabling it should go
> > > > vice versa, i.e first master, then slave, however current code
> > > > does disabling in a same way as enabling. Fix this, by skipping
> > > > non-master crtcs, instead of non-slaves.
> > > >
> > > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> > > > ---
> > > > drivers/gpu/drm/i915/display/intel_display.c | 2 +-
> > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > > > index 0490c6412ab5..68958ba0ef49 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > > @@ -6662,7 +6662,7 @@ static void intel_commit_modeset_disables(struct intel_atomic_state *state)
> > > > */
> > > > if (!is_trans_port_sync_slave(old_crtc_state) &&
> > > > !intel_dp_mst_is_slave_trans(old_crtc_state) &&
> > > > - !intel_crtc_is_bigjoiner_slave(old_crtc_state))
> > > > + !intel_crtc_is_bigjoiner_master(old_crtc_state))
> > >
> > > I don't see what does this fix. The sequence is correct at the moment
> > > and this change would break it, leaving the encoder PLL enabled
> > > incorrectly when the encoder->post_pll_disable() hook is called. Hence
> > > it's NAK from side.
> >
> > Well, as I pointed out the BSpec 49190 instructs us to disable master
> > first, then slave. Current code skips all non-slaves in first cycle,
> > i.e it disables first slaves and then masters. Which is _wrong_.
>
> This is correct at the moment, followed in the encoder's disable hook
> which is only assigned to the master CRTC.
Hmmm... I will check it.
>
> > Anything else in particular, where do you need clarifications?
> >
> > Stan
> >
> > >
> > > > continue;
> > > >
> > > > intel_old_crtc_state_disables(state, old_crtc_state,
> > > > --
> > > > 2.37.3
> > > >
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915: Fix the disabling sequence for Bigjoiner
2023-07-06 8:47 ` Imre Deak
2023-07-06 8:50 ` Lisovskiy, Stanislav
@ 2023-07-06 10:32 ` Lisovskiy, Stanislav
2023-07-06 18:02 ` Ville Syrjälä
1 sibling, 1 reply; 12+ messages in thread
From: Lisovskiy, Stanislav @ 2023-07-06 10:32 UTC (permalink / raw)
To: Imre Deak; +Cc: jani.nikula, intel-gfx
On Thu, Jul 06, 2023 at 11:47:26AM +0300, Imre Deak wrote:
> On Thu, Jul 06, 2023 at 11:24:21AM +0300, Lisovskiy, Stanislav wrote:
> > On Wed, Jul 05, 2023 at 06:32:51PM +0300, Imre Deak wrote:
> > > On Thu, May 25, 2023 at 01:10:36PM +0300, Stanislav Lisovskiy wrote:
> > > > According to BSpec 49190, when enabling crtcs, we first setup
> > > > slave and then master crtc, however for disabling it should go
> > > > vice versa, i.e first master, then slave, however current code
> > > > does disabling in a same way as enabling. Fix this, by skipping
> > > > non-master crtcs, instead of non-slaves.
> > > >
> > > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> > > > ---
> > > > drivers/gpu/drm/i915/display/intel_display.c | 2 +-
> > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > > > index 0490c6412ab5..68958ba0ef49 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > > @@ -6662,7 +6662,7 @@ static void intel_commit_modeset_disables(struct intel_atomic_state *state)
> > > > */
> > > > if (!is_trans_port_sync_slave(old_crtc_state) &&
> > > > !intel_dp_mst_is_slave_trans(old_crtc_state) &&
> > > > - !intel_crtc_is_bigjoiner_slave(old_crtc_state))
> > > > + !intel_crtc_is_bigjoiner_master(old_crtc_state))
> > >
> > > I don't see what does this fix. The sequence is correct at the moment
> > > and this change would break it, leaving the encoder PLL enabled
> > > incorrectly when the encoder->post_pll_disable() hook is called. Hence
> > > it's NAK from side.
> >
> > Well, as I pointed out the BSpec 49190 instructs us to disable master
> > first, then slave. Current code skips all non-slaves in first cycle,
> > i.e it disables first slaves and then masters. Which is _wrong_.
>
> This is correct at the moment, followed in the encoder's disable hook
> which is only assigned to the master CRTC.
Yep, I see now why it was implemented this way.
We basically handle everything in a single hook, taking care of the correct
sequence. As I understood otherwise we are going to have problems with the pll
subsystem, i.e we can't disable pll for master before the slaves(basically means
our pll subsystem contradicts what the crtc/pipe/encoder sequence requires).
I still think this is bery counterintuitive implementation, i.e when there is a single
hook for master taking care of everything, while slaves are just noop.
This makes the whole thing very prone for screwing things up.
Ideally we should still have fully functional hooks for all slaves.
If the pll stuff requires special treatment, that probably should be dealt somehow
separately(don't have any solution for that yet), but definitely we shouldn't live
further like that. Things might get even more complicated in future.
Stan
>
> > Anything else in particular, where do you need clarifications?
> >
> > Stan
> >
> > >
> > > > continue;
> > > >
> > > > intel_old_crtc_state_disables(state, old_crtc_state,
> > > > --
> > > > 2.37.3
> > > >
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915: Fix the disabling sequence for Bigjoiner
2023-07-06 10:32 ` Lisovskiy, Stanislav
@ 2023-07-06 18:02 ` Ville Syrjälä
2023-07-07 9:53 ` Lisovskiy, Stanislav
0 siblings, 1 reply; 12+ messages in thread
From: Ville Syrjälä @ 2023-07-06 18:02 UTC (permalink / raw)
To: Lisovskiy, Stanislav; +Cc: jani.nikula, intel-gfx
On Thu, Jul 06, 2023 at 01:32:17PM +0300, Lisovskiy, Stanislav wrote:
> On Thu, Jul 06, 2023 at 11:47:26AM +0300, Imre Deak wrote:
> > On Thu, Jul 06, 2023 at 11:24:21AM +0300, Lisovskiy, Stanislav wrote:
> > > On Wed, Jul 05, 2023 at 06:32:51PM +0300, Imre Deak wrote:
> > > > On Thu, May 25, 2023 at 01:10:36PM +0300, Stanislav Lisovskiy wrote:
> > > > > According to BSpec 49190, when enabling crtcs, we first setup
> > > > > slave and then master crtc, however for disabling it should go
> > > > > vice versa, i.e first master, then slave, however current code
> > > > > does disabling in a same way as enabling. Fix this, by skipping
> > > > > non-master crtcs, instead of non-slaves.
> > > > >
> > > > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> > > > > ---
> > > > > drivers/gpu/drm/i915/display/intel_display.c | 2 +-
> > > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > > > > index 0490c6412ab5..68958ba0ef49 100644
> > > > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > > > @@ -6662,7 +6662,7 @@ static void intel_commit_modeset_disables(struct intel_atomic_state *state)
> > > > > */
> > > > > if (!is_trans_port_sync_slave(old_crtc_state) &&
> > > > > !intel_dp_mst_is_slave_trans(old_crtc_state) &&
> > > > > - !intel_crtc_is_bigjoiner_slave(old_crtc_state))
> > > > > + !intel_crtc_is_bigjoiner_master(old_crtc_state))
> > > >
> > > > I don't see what does this fix. The sequence is correct at the moment
> > > > and this change would break it, leaving the encoder PLL enabled
> > > > incorrectly when the encoder->post_pll_disable() hook is called. Hence
> > > > it's NAK from side.
> > >
> > > Well, as I pointed out the BSpec 49190 instructs us to disable master
> > > first, then slave. Current code skips all non-slaves in first cycle,
> > > i.e it disables first slaves and then masters. Which is _wrong_.
> >
> > This is correct at the moment, followed in the encoder's disable hook
> > which is only assigned to the master CRTC.
>
> Yep, I see now why it was implemented this way.
> We basically handle everything in a single hook, taking care of the correct
> sequence. As I understood otherwise we are going to have problems with the pll
> subsystem, i.e we can't disable pll for master before the slaves(basically means
> our pll subsystem contradicts what the crtc/pipe/encoder sequence requires).
>
> I still think this is bery counterintuitive implementation, i.e when there is a single
> hook for master taking care of everything, while slaves are just noop.
> This makes the whole thing very prone for screwing things up.
> Ideally we should still have fully functional hooks for all slaves.
> If the pll stuff requires special treatment, that probably should be dealt somehow
> separately(don't have any solution for that yet), but definitely we shouldn't live
> further like that. Things might get even more complicated in future.
IMO what we should aim for is to call the high level crtc hooks only for
the master crtc (ie. essentially the transcoder), and then iterate through
the pipes inside the hooks at the approptiate points. To do that
cleanly we want to split the code along the pipe-transcoder boundary
as much as possible.
--
Ville Syrjälä
Intel
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915: Fix the disabling sequence for Bigjoiner
2023-07-06 18:02 ` Ville Syrjälä
@ 2023-07-07 9:53 ` Lisovskiy, Stanislav
0 siblings, 0 replies; 12+ messages in thread
From: Lisovskiy, Stanislav @ 2023-07-07 9:53 UTC (permalink / raw)
To: Ville Syrjälä; +Cc: jani.nikula, intel-gfx
On Thu, Jul 06, 2023 at 09:02:29PM +0300, Ville Syrjälä wrote:
> On Thu, Jul 06, 2023 at 01:32:17PM +0300, Lisovskiy, Stanislav wrote:
> > On Thu, Jul 06, 2023 at 11:47:26AM +0300, Imre Deak wrote:
> > > On Thu, Jul 06, 2023 at 11:24:21AM +0300, Lisovskiy, Stanislav wrote:
> > > > On Wed, Jul 05, 2023 at 06:32:51PM +0300, Imre Deak wrote:
> > > > > On Thu, May 25, 2023 at 01:10:36PM +0300, Stanislav Lisovskiy wrote:
> > > > > > According to BSpec 49190, when enabling crtcs, we first setup
> > > > > > slave and then master crtc, however for disabling it should go
> > > > > > vice versa, i.e first master, then slave, however current code
> > > > > > does disabling in a same way as enabling. Fix this, by skipping
> > > > > > non-master crtcs, instead of non-slaves.
> > > > > >
> > > > > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> > > > > > ---
> > > > > > drivers/gpu/drm/i915/display/intel_display.c | 2 +-
> > > > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > > > > > index 0490c6412ab5..68958ba0ef49 100644
> > > > > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > > > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > > > > @@ -6662,7 +6662,7 @@ static void intel_commit_modeset_disables(struct intel_atomic_state *state)
> > > > > > */
> > > > > > if (!is_trans_port_sync_slave(old_crtc_state) &&
> > > > > > !intel_dp_mst_is_slave_trans(old_crtc_state) &&
> > > > > > - !intel_crtc_is_bigjoiner_slave(old_crtc_state))
> > > > > > + !intel_crtc_is_bigjoiner_master(old_crtc_state))
> > > > >
> > > > > I don't see what does this fix. The sequence is correct at the moment
> > > > > and this change would break it, leaving the encoder PLL enabled
> > > > > incorrectly when the encoder->post_pll_disable() hook is called. Hence
> > > > > it's NAK from side.
> > > >
> > > > Well, as I pointed out the BSpec 49190 instructs us to disable master
> > > > first, then slave. Current code skips all non-slaves in first cycle,
> > > > i.e it disables first slaves and then masters. Which is _wrong_.
> > >
> > > This is correct at the moment, followed in the encoder's disable hook
> > > which is only assigned to the master CRTC.
> >
> > Yep, I see now why it was implemented this way.
> > We basically handle everything in a single hook, taking care of the correct
> > sequence. As I understood otherwise we are going to have problems with the pll
> > subsystem, i.e we can't disable pll for master before the slaves(basically means
> > our pll subsystem contradicts what the crtc/pipe/encoder sequence requires).
> >
> > I still think this is bery counterintuitive implementation, i.e when there is a single
> > hook for master taking care of everything, while slaves are just noop.
> > This makes the whole thing very prone for screwing things up.
> > Ideally we should still have fully functional hooks for all slaves.
> > If the pll stuff requires special treatment, that probably should be dealt somehow
> > separately(don't have any solution for that yet), but definitely we shouldn't live
> > further like that. Things might get even more complicated in future.
>
> IMO what we should aim for is to call the high level crtc hooks only for
> the master crtc (ie. essentially the transcoder), and then iterate through
> the pipes inside the hooks at the approptiate points. To do that
> cleanly we want to split the code along the pipe-transcoder boundary
> as much as possible.
Problem is that we are still iterating slave crtcs in other places, just
as regular pipes, which can create some additional mess.
If we choose this approach then, it would be nice not to even try treating
slave crtcs like regular pipe in places like skl_modeset_disables/enables.
We should probably just skip them if we detect that they are bigjoiner slaves.
So we go through only regular or joiner master pipes there.
And all the joiner enable/disable logic would be then done in correspondent master
crtc enable/disable hook.
Otherwise we are at risk of messing things up. Also we will then have to use all
those weird checks, all over the place.
Stan
>
> --
> Ville Syrjälä
> Intel
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2023-07-07 9:54 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-05-25 10:10 [Intel-gfx] [PATCH] drm/i915: Fix the disabling sequence for Bigjoiner Stanislav Lisovskiy
2023-05-25 19:08 ` [Intel-gfx] ✓ Fi.CI.BAT: success for " Patchwork
2023-05-26 4:15 ` [Intel-gfx] ✓ Fi.CI.IGT: " Patchwork
2023-07-05 13:26 ` [Intel-gfx] [PATCH] " Luca Coelho
2023-07-05 15:32 ` Imre Deak
2023-07-06 8:24 ` Lisovskiy, Stanislav
2023-07-06 8:47 ` Imre Deak
2023-07-06 8:50 ` Lisovskiy, Stanislav
2023-07-06 10:32 ` Lisovskiy, Stanislav
2023-07-06 18:02 ` Ville Syrjälä
2023-07-07 9:53 ` Lisovskiy, Stanislav
2023-07-06 8:33 ` Lisovskiy, Stanislav
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.