* [Intel-gfx] [PATCH 0/3] i915 fixes to handle hotplug cases on 8K tiled monitor
@ 2019-12-11 21:14 Manasi Navare
2019-12-11 21:14 ` [Intel-gfx] [PATCH 1/3] drm/i915/dp: Make sure all tiled connectors get added to the state with full modeset Manasi Navare
` (5 more replies)
0 siblings, 6 replies; 35+ messages in thread
From: Manasi Navare @ 2019-12-11 21:14 UTC (permalink / raw)
To: intel-gfx
This has some fixes in the transcoder port sync teardown sequence which
ensures now that the hotplug/unplug is handled gracefully
Manasi Navare (3):
drm/i915/dp: Make sure all tiled connectors get added to the state
with full modeset
drm/i915/dp: Make port sync mode assignments only if all tiles present
drm/i915/dp: Disable Port sync mode correctly on teardown
drivers/gpu/drm/i915/display/intel_display.c | 109 ++++++++++++++++++-
1 file changed, 106 insertions(+), 3 deletions(-)
--
2.19.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 35+ messages in thread
* [Intel-gfx] [PATCH 1/3] drm/i915/dp: Make sure all tiled connectors get added to the state with full modeset
2019-12-11 21:14 [Intel-gfx] [PATCH 0/3] i915 fixes to handle hotplug cases on 8K tiled monitor Manasi Navare
@ 2019-12-11 21:14 ` Manasi Navare
2019-12-13 0:32 ` Matt Roper
` (2 more replies)
2019-12-11 21:14 ` [Intel-gfx] [PATCH 2/3] drm/i915/dp: Make port sync mode assignments only if all tiles present Manasi Navare
` (4 subsequent siblings)
5 siblings, 3 replies; 35+ messages in thread
From: Manasi Navare @ 2019-12-11 21:14 UTC (permalink / raw)
To: intel-gfx
In case of tiled displays, all the tiles are linke dto each other
for transcoder port sync. So in intel_atomic_check() we need to make
sure that we add all the tiles to the modeset and if one of the
tiles needs a full modeset then mark all other tiles for a full modeset.
Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: José Roberto de Souza <jose.souza@intel.com>
Bugzilla: https://gitlab.freedesktop.org/drm/intel/issues/5
Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
---
drivers/gpu/drm/i915/display/intel_display.c | 78 ++++++++++++++++++++
1 file changed, 78 insertions(+)
diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index 803993a01ca7..7263eaa66cda 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -14066,6 +14066,80 @@ static int intel_atomic_check_crtcs(struct intel_atomic_state *state)
return 0;
}
+static int
+intel_dp_modeset_all_tiles(struct drm_i915_private *dev_priv,
+ struct intel_atomic_state *state, int tile_grp_id)
+{
+ struct drm_connector *conn_iter;
+ struct drm_connector_list_iter conn_list_iter;
+ struct drm_crtc_state *crtc_state;
+
+ drm_connector_list_iter_begin(&dev_priv->drm, &conn_list_iter);
+ drm_for_each_connector_iter(conn_iter, &conn_list_iter) {
+ struct drm_connector_state *conn_iter_state;
+
+ if (!conn_iter->has_tile)
+ continue;
+ conn_iter_state = drm_atomic_get_connector_state(&state->base,
+ conn_iter);
+ if (IS_ERR(conn_iter_state)) {
+ drm_connector_list_iter_end(&conn_list_iter);
+ return PTR_ERR(conn_iter_state);
+ }
+
+ if (!conn_iter_state->crtc)
+ continue;
+
+ if (conn_iter->tile_group->id != tile_grp_id)
+ continue;
+
+ crtc_state = drm_atomic_get_crtc_state(&state->base, conn_iter_state->crtc);
+ if (IS_ERR(crtc_state)) {
+ drm_connector_list_iter_end(&conn_list_iter);
+ return PTR_ERR(conn_iter_state);
+ }
+ crtc_state->mode_changed = true;
+ }
+ drm_connector_list_iter_end(&conn_list_iter);
+
+ return 0;
+}
+
+static int
+intel_dp_atomic_trans_port_sync_check(struct drm_i915_private *dev_priv,
+ struct intel_atomic_state *state)
+{
+ struct drm_connector *connector;
+ struct drm_crtc_state *crtc_state;
+ struct drm_connector_state *connector_state;
+ int i, ret, tile_grp_id = 0;
+
+ if (INTEL_GEN(dev_priv) < 11)
+ return 0;
+
+ /* Is tiled, mark all other tiled CRTCs as needing a modeset */
+ for_each_new_connector_in_state(&state->base, connector, connector_state, i) {
+ if (!connector->has_tile)
+ continue;
+ if (connector_state->crtc &&
+ tile_grp_id != connector->tile_group->id) {
+ crtc_state = drm_atomic_get_new_crtc_state(&state->base,
+ connector_state->crtc);
+ if (!drm_atomic_crtc_needs_modeset(crtc_state))
+ continue;
+
+ tile_grp_id = connector->tile_group->id;
+ } else
+ continue;
+
+ ret = intel_dp_modeset_all_tiles(dev_priv, state, tile_grp_id);
+ if (ret)
+ return ret;
+ }
+
+ return 0;
+}
+
/**
* intel_atomic_check - validate state object
* @dev: drm device
@@ -14093,6 +14167,10 @@ static int intel_atomic_check(struct drm_device *dev,
if (ret)
goto fail;
+ ret = intel_dp_atomic_trans_port_sync_check(dev_priv, state);
+ if (ret)
+ goto fail;
+
for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
new_crtc_state, i) {
if (!needs_modeset(new_crtc_state)) {
--
2.19.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 35+ messages in thread
* [Intel-gfx] [PATCH 2/3] drm/i915/dp: Make port sync mode assignments only if all tiles present
2019-12-11 21:14 [Intel-gfx] [PATCH 0/3] i915 fixes to handle hotplug cases on 8K tiled monitor Manasi Navare
2019-12-11 21:14 ` [Intel-gfx] [PATCH 1/3] drm/i915/dp: Make sure all tiled connectors get added to the state with full modeset Manasi Navare
@ 2019-12-11 21:14 ` Manasi Navare
2019-12-13 1:03 ` Matt Roper
2019-12-13 20:28 ` Ville Syrjälä
2019-12-11 21:14 ` [Intel-gfx] [PATCH 3/3] drm/i915/dp: Disable Port sync mode correctly on teardown Manasi Navare
` (3 subsequent siblings)
5 siblings, 2 replies; 35+ messages in thread
From: Manasi Navare @ 2019-12-11 21:14 UTC (permalink / raw)
To: intel-gfx
Add an extra check before making master slave assignments for tiled
displays to make sure we make these assignments only if all tiled
connectors are present. If not then initialize the state to defaults
so it does a normal non tiled modeset without transcoder port sync.
Bugzilla: https://gitlab.freedesktop.org/drm/intel/issues/5
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
---
drivers/gpu/drm/i915/display/intel_display.c | 28 ++++++++++++++++++--
1 file changed, 26 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index 7263eaa66cda..c0a2dab3fe67 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -12048,6 +12048,12 @@ static bool c8_planes_changed(const struct intel_crtc_state *new_crtc_state)
return !old_crtc_state->c8_planes != !new_crtc_state->c8_planes;
}
+static void initialize_trans_port_sync_mode_state(struct intel_crtc_state *crtc_state)
+{
+ crtc_state->master_transcoder = INVALID_TRANSCODER;
+ crtc_state->sync_mode_slaves_mask = 0;
+}
+
static int icl_add_sync_mode_crtcs(struct intel_crtc_state *crtc_state)
{
struct drm_crtc *crtc = crtc_state->uapi.crtc;
@@ -12059,11 +12065,22 @@ static int icl_add_sync_mode_crtcs(struct intel_crtc_state *crtc_state)
struct drm_crtc *master_crtc = NULL;
struct drm_crtc_state *master_crtc_state;
struct intel_crtc_state *master_pipe_config;
- int i, tile_group_id;
+ int i, tile_group_id = 0, num_tiled_conns = 0;
if (INTEL_GEN(dev_priv) < 11)
return 0;
+ /* If all tiles not present do not make master slave assignments
+ * Here we assume all tiles belong to the same tile group for now.
+ */
+ for_each_new_connector_in_state(&state->base, connector, connector_state, i) {
+ if (connector->has_tile) {
+ if (!tile_group_id)
+ tile_group_id = connector->tile_group->id;
+ num_tiled_conns++;
+ }
+ }
+
/*
* In case of tiled displays there could be one or more slaves but there is
* only one master. Lets make the CRTC used by the connector corresponding
@@ -12077,8 +12094,15 @@ static int icl_add_sync_mode_crtcs(struct intel_crtc_state *crtc_state)
if (!connector->has_tile)
continue;
if (crtc_state->hw.mode.hdisplay != connector->tile_h_size ||
- crtc_state->hw.mode.vdisplay != connector->tile_v_size)
+ crtc_state->hw.mode.vdisplay != connector->tile_v_size) {
+ initialize_trans_port_sync_mode_state(crtc_state);
return 0;
+ }
+ if (connector->tile_group->id == tile_group_id &&
+ num_tiled_conns < connector->num_h_tile * connector->num_v_tile) {
+ initialize_trans_port_sync_mode_state(crtc_state);
+ return 0;
+ }
if (connector->tile_h_loc == connector->num_h_tile - 1 &&
connector->tile_v_loc == connector->num_v_tile - 1)
continue;
--
2.19.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 35+ messages in thread
* [Intel-gfx] [PATCH 3/3] drm/i915/dp: Disable Port sync mode correctly on teardown
2019-12-11 21:14 [Intel-gfx] [PATCH 0/3] i915 fixes to handle hotplug cases on 8K tiled monitor Manasi Navare
2019-12-11 21:14 ` [Intel-gfx] [PATCH 1/3] drm/i915/dp: Make sure all tiled connectors get added to the state with full modeset Manasi Navare
2019-12-11 21:14 ` [Intel-gfx] [PATCH 2/3] drm/i915/dp: Make port sync mode assignments only if all tiles present Manasi Navare
@ 2019-12-11 21:14 ` Manasi Navare
2019-12-13 3:14 ` Matt Roper
2019-12-13 20:06 ` Ville Syrjälä
2019-12-12 1:53 ` [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for i915 fixes to handle hotplug cases on 8K tiled monitor Patchwork
` (2 subsequent siblings)
5 siblings, 2 replies; 35+ messages in thread
From: Manasi Navare @ 2019-12-11 21:14 UTC (permalink / raw)
To: intel-gfx; +Cc: Jani Nikula
While clearing the Ports ync mode enable and master select bits
we need to make sure that we perform a RMW for disable else
it sets the other bits casuing unwanted sideeffects.
Bugzilla: https://gitlab.freedesktop.org/drm/intel/issues/5
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Jani Nikula <jani.nikula@intel.com>
Fixes: 51528afe7c5e ("drm/i915/display/icl: Disable transcoder port sync as part of crtc_disable() sequence")
Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
---
drivers/gpu/drm/i915/display/intel_display.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index c0a2dab3fe67..3fccda0f1f36 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -4599,7 +4599,8 @@ static void icl_disable_transcoder_port_sync(const struct intel_crtc_state *old_
transcoder_name(old_crtc_state->cpu_transcoder));
reg = TRANS_DDI_FUNC_CTL2(old_crtc_state->cpu_transcoder);
- trans_ddi_func_ctl2_val = ~(PORT_SYNC_MODE_ENABLE |
+ trans_ddi_func_ctl2_val = I915_READ(reg);
+ trans_ddi_func_ctl2_val &= ~(PORT_SYNC_MODE_ENABLE |
PORT_SYNC_MODE_MASTER_SELECT_MASK);
I915_WRITE(reg, trans_ddi_func_ctl2_val);
}
--
2.19.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 35+ messages in thread
* [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for i915 fixes to handle hotplug cases on 8K tiled monitor
2019-12-11 21:14 [Intel-gfx] [PATCH 0/3] i915 fixes to handle hotplug cases on 8K tiled monitor Manasi Navare
` (2 preceding siblings ...)
2019-12-11 21:14 ` [Intel-gfx] [PATCH 3/3] drm/i915/dp: Disable Port sync mode correctly on teardown Manasi Navare
@ 2019-12-12 1:53 ` Patchwork
2019-12-12 2:36 ` [Intel-gfx] ✓ Fi.CI.BAT: success " Patchwork
2019-12-12 13:49 ` [Intel-gfx] ✓ Fi.CI.IGT: " Patchwork
5 siblings, 0 replies; 35+ messages in thread
From: Patchwork @ 2019-12-12 1:53 UTC (permalink / raw)
To: Manasi Navare; +Cc: intel-gfx
== Series Details ==
Series: i915 fixes to handle hotplug cases on 8K tiled monitor
URL : https://patchwork.freedesktop.org/series/70788/
State : warning
== Summary ==
$ dim checkpatch origin/drm-tip
8279f374e563 drm/i915/dp: Make sure all tiled connectors get added to the state with full modeset
-:84: CHECK:BRACES: braces {} should be used on all arms of this statement
#84: FILE: drivers/gpu/drm/i915/display/intel_display.c:14124:
+ if (connector_state->crtc &&
[...]
+ } else
[...]
-:92: CHECK:BRACES: Unbalanced braces around else statement
#92: FILE: drivers/gpu/drm/i915/display/intel_display.c:14132:
+ } else
total: 0 errors, 0 warnings, 2 checks, 90 lines checked
4fe2b74a1644 drm/i915/dp: Make port sync mode assignments only if all tiles present
58e4b57a23e0 drm/i915/dp: Disable Port sync mode correctly on teardown
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 35+ messages in thread
* [Intel-gfx] ✓ Fi.CI.BAT: success for i915 fixes to handle hotplug cases on 8K tiled monitor
2019-12-11 21:14 [Intel-gfx] [PATCH 0/3] i915 fixes to handle hotplug cases on 8K tiled monitor Manasi Navare
` (3 preceding siblings ...)
2019-12-12 1:53 ` [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for i915 fixes to handle hotplug cases on 8K tiled monitor Patchwork
@ 2019-12-12 2:36 ` Patchwork
2019-12-12 13:49 ` [Intel-gfx] ✓ Fi.CI.IGT: " Patchwork
5 siblings, 0 replies; 35+ messages in thread
From: Patchwork @ 2019-12-12 2:36 UTC (permalink / raw)
To: Manasi Navare; +Cc: intel-gfx
== Series Details ==
Series: i915 fixes to handle hotplug cases on 8K tiled monitor
URL : https://patchwork.freedesktop.org/series/70788/
State : success
== Summary ==
CI Bug Log - changes from CI_DRM_7545 -> Patchwork_15700
====================================================
Summary
-------
**SUCCESS**
No regressions found.
External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15700/index.html
Known issues
------------
Here are the changes found in Patchwork_15700 that come from known issues:
### IGT changes ###
#### Issues hit ####
* igt@i915_selftest@live_gem_contexts:
- fi-hsw-peppy: [PASS][1] -> [INCOMPLETE][2] ([i915#694])
[1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7545/fi-hsw-peppy/igt@i915_selftest@live_gem_contexts.html
[2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15700/fi-hsw-peppy/igt@i915_selftest@live_gem_contexts.html
* igt@i915_selftest@live_requests:
- fi-hsw-4770: [PASS][3] -> [INCOMPLETE][4] ([i915#773])
[3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7545/fi-hsw-4770/igt@i915_selftest@live_requests.html
[4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15700/fi-hsw-4770/igt@i915_selftest@live_requests.html
#### Possible fixes ####
* igt@kms_chamelium@hdmi-hpd-fast:
- fi-kbl-7500u: [FAIL][5] ([fdo#111096] / [i915#323]) -> [PASS][6]
[5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7545/fi-kbl-7500u/igt@kms_chamelium@hdmi-hpd-fast.html
[6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15700/fi-kbl-7500u/igt@kms_chamelium@hdmi-hpd-fast.html
#### Warnings ####
* igt@i915_module_load@reload:
- fi-icl-u2: [DMESG-WARN][7] ([i915#109] / [i915#289]) -> [DMESG-WARN][8] ([i915#289])
[7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7545/fi-icl-u2/igt@i915_module_load@reload.html
[8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15700/fi-icl-u2/igt@i915_module_load@reload.html
* igt@i915_pm_rpm@basic-rte:
- fi-kbl-guc: [SKIP][9] ([fdo#109271]) -> [FAIL][10] ([i915#704])
[9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7545/fi-kbl-guc/igt@i915_pm_rpm@basic-rte.html
[10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15700/fi-kbl-guc/igt@i915_pm_rpm@basic-rte.html
* igt@kms_flip@basic-flip-vs-modeset:
- fi-kbl-x1275: [DMESG-WARN][11] ([i915#62] / [i915#92] / [i915#95]) -> [DMESG-WARN][12] ([i915#62] / [i915#92]) +4 similar issues
[11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7545/fi-kbl-x1275/igt@kms_flip@basic-flip-vs-modeset.html
[12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15700/fi-kbl-x1275/igt@kms_flip@basic-flip-vs-modeset.html
* igt@kms_pipe_crc_basic@suspend-read-crc-pipe-a:
- fi-kbl-x1275: [DMESG-WARN][13] ([i915#62] / [i915#92]) -> [DMESG-WARN][14] ([i915#62] / [i915#92] / [i915#95]) +5 similar issues
[13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7545/fi-kbl-x1275/igt@kms_pipe_crc_basic@suspend-read-crc-pipe-a.html
[14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15700/fi-kbl-x1275/igt@kms_pipe_crc_basic@suspend-read-crc-pipe-a.html
{name}: This element is suppressed. This means it is ignored when computing
the status of the difference (SUCCESS, WARNING, or FAILURE).
[fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
[fdo#111096]: https://bugs.freedesktop.org/show_bug.cgi?id=111096
[i915#109]: https://gitlab.freedesktop.org/drm/intel/issues/109
[i915#289]: https://gitlab.freedesktop.org/drm/intel/issues/289
[i915#323]: https://gitlab.freedesktop.org/drm/intel/issues/323
[i915#472]: https://gitlab.freedesktop.org/drm/intel/issues/472
[i915#62]: https://gitlab.freedesktop.org/drm/intel/issues/62
[i915#694]: https://gitlab.freedesktop.org/drm/intel/issues/694
[i915#704]: https://gitlab.freedesktop.org/drm/intel/issues/704
[i915#707]: https://gitlab.freedesktop.org/drm/intel/issues/707
[i915#773]: https://gitlab.freedesktop.org/drm/intel/issues/773
[i915#92]: https://gitlab.freedesktop.org/drm/intel/issues/92
[i915#95]: https://gitlab.freedesktop.org/drm/intel/issues/95
Participating hosts (52 -> 45)
------------------------------
Additional (1): fi-kbl-7560u
Missing (8): fi-hsw-4770r fi-tgl-guc fi-ilk-m540 fi-byt-squawks fi-bsw-cyan fi-ctg-p8600 fi-byt-clapper fi-bdw-samus
Build changes
-------------
* CI: CI-20190529 -> None
* Linux: CI_DRM_7545 -> Patchwork_15700
CI-20190529: 20190529
CI_DRM_7545: b1b808dff985c3c2050b20771050453589a60ca3 @ git://anongit.freedesktop.org/gfx-ci/linux
IGT_5346: 466b0e6cbcbaccff012b484d1fd7676364b37b93 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
Patchwork_15700: 58e4b57a23e011523571977f21a2d476e7f349fb @ git://anongit.freedesktop.org/gfx-ci/linux
== Linux commits ==
58e4b57a23e0 drm/i915/dp: Disable Port sync mode correctly on teardown
4fe2b74a1644 drm/i915/dp: Make port sync mode assignments only if all tiles present
8279f374e563 drm/i915/dp: Make sure all tiled connectors get added to the state with full modeset
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15700/index.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 35+ messages in thread
* [Intel-gfx] ✓ Fi.CI.IGT: success for i915 fixes to handle hotplug cases on 8K tiled monitor
2019-12-11 21:14 [Intel-gfx] [PATCH 0/3] i915 fixes to handle hotplug cases on 8K tiled monitor Manasi Navare
` (4 preceding siblings ...)
2019-12-12 2:36 ` [Intel-gfx] ✓ Fi.CI.BAT: success " Patchwork
@ 2019-12-12 13:49 ` Patchwork
5 siblings, 0 replies; 35+ messages in thread
From: Patchwork @ 2019-12-12 13:49 UTC (permalink / raw)
To: Manasi Navare; +Cc: intel-gfx
== Series Details ==
Series: i915 fixes to handle hotplug cases on 8K tiled monitor
URL : https://patchwork.freedesktop.org/series/70788/
State : success
== Summary ==
CI Bug Log - changes from CI_DRM_7545_full -> Patchwork_15700_full
====================================================
Summary
-------
**SUCCESS**
No regressions found.
Known issues
------------
Here are the changes found in Patchwork_15700_full that come from known issues:
### IGT changes ###
#### Issues hit ####
* igt@gem_busy@extended-parallel-vcs1:
- shard-iclb: [PASS][1] -> [SKIP][2] ([fdo#112080]) +3 similar issues
[1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7545/shard-iclb2/igt@gem_busy@extended-parallel-vcs1.html
[2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15700/shard-iclb8/igt@gem_busy@extended-parallel-vcs1.html
* igt@gem_ctx_shared@exec-single-timeline-bsd:
- shard-iclb: [PASS][3] -> [SKIP][4] ([fdo#110841])
[3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7545/shard-iclb5/igt@gem_ctx_shared@exec-single-timeline-bsd.html
[4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15700/shard-iclb4/igt@gem_ctx_shared@exec-single-timeline-bsd.html
* igt@gem_ctx_shared@q-smoketest-bsd:
- shard-tglb: [PASS][5] -> [INCOMPLETE][6] ([i915#461])
[5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7545/shard-tglb3/igt@gem_ctx_shared@q-smoketest-bsd.html
[6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15700/shard-tglb4/igt@gem_ctx_shared@q-smoketest-bsd.html
* igt@gem_eio@kms:
- shard-kbl: [PASS][7] -> [DMESG-WARN][8] ([i915#180] / [i915#56])
[7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7545/shard-kbl7/igt@gem_eio@kms.html
[8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15700/shard-kbl2/igt@gem_eio@kms.html
* igt@gem_exec_balancer@nop:
- shard-tglb: [PASS][9] -> [INCOMPLETE][10] ([fdo#111736])
[9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7545/shard-tglb3/igt@gem_exec_balancer@nop.html
[10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15700/shard-tglb5/igt@gem_exec_balancer@nop.html
* igt@gem_exec_parallel@fds:
- shard-tglb: [PASS][11] -> [INCOMPLETE][12] ([i915#470])
[11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7545/shard-tglb2/igt@gem_exec_parallel@fds.html
[12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15700/shard-tglb6/igt@gem_exec_parallel@fds.html
* igt@gem_exec_schedule@preemptive-hang-bsd:
- shard-iclb: [PASS][13] -> [SKIP][14] ([fdo#112146]) +2 similar issues
[13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7545/shard-iclb6/igt@gem_exec_schedule@preemptive-hang-bsd.html
[14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15700/shard-iclb1/igt@gem_exec_schedule@preemptive-hang-bsd.html
* igt@gem_softpin@noreloc-s3:
- shard-apl: [PASS][15] -> [DMESG-WARN][16] ([i915#180]) +2 similar issues
[15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7545/shard-apl8/igt@gem_softpin@noreloc-s3.html
[16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15700/shard-apl6/igt@gem_softpin@noreloc-s3.html
* igt@i915_pm_rpm@drm-resources-equal:
- shard-tglb: [PASS][17] -> [DMESG-WARN][18] ([i915#766])
[17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7545/shard-tglb6/igt@i915_pm_rpm@drm-resources-equal.html
[18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15700/shard-tglb8/igt@i915_pm_rpm@drm-resources-equal.html
* igt@i915_pm_rpm@system-suspend-devices:
- shard-tglb: [PASS][19] -> [INCOMPLETE][20] ([i915#456] / [i915#460]) +1 similar issue
[19]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7545/shard-tglb1/igt@i915_pm_rpm@system-suspend-devices.html
[20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15700/shard-tglb5/igt@i915_pm_rpm@system-suspend-devices.html
* igt@i915_pm_rps@waitboost:
- shard-tglb: [PASS][21] -> [FAIL][22] ([i915#413])
[21]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7545/shard-tglb3/igt@i915_pm_rps@waitboost.html
[22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15700/shard-tglb3/igt@i915_pm_rps@waitboost.html
* igt@kms_color@pipe-b-ctm-0-5:
- shard-skl: [PASS][23] -> [DMESG-WARN][24] ([i915#109])
[23]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7545/shard-skl9/igt@kms_color@pipe-b-ctm-0-5.html
[24]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15700/shard-skl4/igt@kms_color@pipe-b-ctm-0-5.html
* igt@kms_cursor_crc@pipe-b-cursor-256x85-random:
- shard-skl: [PASS][25] -> [FAIL][26] ([i915#54]) +1 similar issue
[25]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7545/shard-skl1/igt@kms_cursor_crc@pipe-b-cursor-256x85-random.html
[26]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15700/shard-skl1/igt@kms_cursor_crc@pipe-b-cursor-256x85-random.html
* igt@kms_draw_crc@draw-method-xrgb2101010-mmap-gtt-ytiled:
- shard-skl: [PASS][27] -> [FAIL][28] ([i915#52] / [i915#54])
[27]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7545/shard-skl6/igt@kms_draw_crc@draw-method-xrgb2101010-mmap-gtt-ytiled.html
[28]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15700/shard-skl10/igt@kms_draw_crc@draw-method-xrgb2101010-mmap-gtt-ytiled.html
* igt@kms_draw_crc@draw-method-xrgb2101010-render-ytiled:
- shard-tglb: [PASS][29] -> [INCOMPLETE][30] ([i915#667]) +1 similar issue
[29]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7545/shard-tglb7/igt@kms_draw_crc@draw-method-xrgb2101010-render-ytiled.html
[30]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15700/shard-tglb8/igt@kms_draw_crc@draw-method-xrgb2101010-render-ytiled.html
* igt@kms_flip@flip-vs-expired-vblank-interruptible:
- shard-skl: [PASS][31] -> [FAIL][32] ([i915#79])
[31]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7545/shard-skl3/igt@kms_flip@flip-vs-expired-vblank-interruptible.html
[32]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15700/shard-skl8/igt@kms_flip@flip-vs-expired-vblank-interruptible.html
* igt@kms_flip@flip-vs-suspend:
- shard-skl: [PASS][33] -> [INCOMPLETE][34] ([i915#221])
[33]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7545/shard-skl1/igt@kms_flip@flip-vs-suspend.html
[34]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15700/shard-skl4/igt@kms_flip@flip-vs-suspend.html
* igt@kms_flip@plain-flip-fb-recreate:
- shard-skl: [PASS][35] -> [FAIL][36] ([i915#34])
[35]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7545/shard-skl4/igt@kms_flip@plain-flip-fb-recreate.html
[36]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15700/shard-skl3/igt@kms_flip@plain-flip-fb-recreate.html
* igt@kms_frontbuffer_tracking@fbc-1p-primscrn-cur-indfb-draw-blt:
- shard-tglb: [PASS][37] -> [FAIL][38] ([i915#49]) +2 similar issues
[37]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7545/shard-tglb4/igt@kms_frontbuffer_tracking@fbc-1p-primscrn-cur-indfb-draw-blt.html
[38]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15700/shard-tglb5/igt@kms_frontbuffer_tracking@fbc-1p-primscrn-cur-indfb-draw-blt.html
* igt@kms_frontbuffer_tracking@fbc-1p-rte:
- shard-tglb: [PASS][39] -> [INCOMPLETE][40] ([i915#474] / [i915#667])
[39]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7545/shard-tglb6/igt@kms_frontbuffer_tracking@fbc-1p-rte.html
[40]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15700/shard-tglb2/igt@kms_frontbuffer_tracking@fbc-1p-rte.html
* igt@kms_frontbuffer_tracking@fbc-suspend:
- shard-tglb: [PASS][41] -> [INCOMPLETE][42] ([i915#456] / [i915#460] / [i915#474])
[41]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7545/shard-tglb8/igt@kms_frontbuffer_tracking@fbc-suspend.html
[42]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15700/shard-tglb2/igt@kms_frontbuffer_tracking@fbc-suspend.html
* igt@kms_plane@pixel-format-pipe-a-planes:
- shard-kbl: [PASS][43] -> [INCOMPLETE][44] ([fdo#103665] / [i915#648] / [i915#667])
[43]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7545/shard-kbl4/igt@kms_plane@pixel-format-pipe-a-planes.html
[44]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15700/shard-kbl6/igt@kms_plane@pixel-format-pipe-a-planes.html
* igt@kms_psr@psr2_cursor_render:
- shard-iclb: [PASS][45] -> [SKIP][46] ([fdo#109441]) +1 similar issue
[45]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7545/shard-iclb2/igt@kms_psr@psr2_cursor_render.html
[46]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15700/shard-iclb8/igt@kms_psr@psr2_cursor_render.html
* igt@kms_setmode@basic:
- shard-hsw: [PASS][47] -> [FAIL][48] ([i915#31])
[47]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7545/shard-hsw4/igt@kms_setmode@basic.html
[48]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15700/shard-hsw7/igt@kms_setmode@basic.html
* igt@kms_vblank@pipe-a-ts-continuation-suspend:
- shard-kbl: [PASS][49] -> [DMESG-WARN][50] ([i915#180]) +6 similar issues
[49]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7545/shard-kbl6/igt@kms_vblank@pipe-a-ts-continuation-suspend.html
[50]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15700/shard-kbl1/igt@kms_vblank@pipe-a-ts-continuation-suspend.html
* igt@kms_vblank@pipe-d-ts-continuation-suspend:
- shard-tglb: [PASS][51] -> [INCOMPLETE][52] ([i915#460])
[51]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7545/shard-tglb6/igt@kms_vblank@pipe-d-ts-continuation-suspend.html
[52]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15700/shard-tglb3/igt@kms_vblank@pipe-d-ts-continuation-suspend.html
* igt@prime_vgem@fence-wait-bsd2:
- shard-iclb: [PASS][53] -> [SKIP][54] ([fdo#109276]) +5 similar issues
[53]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7545/shard-iclb2/igt@prime_vgem@fence-wait-bsd2.html
[54]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15700/shard-iclb7/igt@prime_vgem@fence-wait-bsd2.html
#### Possible fixes ####
* igt@gem_ctx_isolation@vcs0-s3:
- shard-skl: [INCOMPLETE][55] ([i915#69]) -> [PASS][56]
[55]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7545/shard-skl5/igt@gem_ctx_isolation@vcs0-s3.html
[56]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15700/shard-skl8/igt@gem_ctx_isolation@vcs0-s3.html
* igt@gem_ctx_persistence@vcs1-queued:
- shard-iclb: [SKIP][57] ([fdo#109276] / [fdo#112080]) -> [PASS][58] +2 similar issues
[57]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7545/shard-iclb7/igt@gem_ctx_persistence@vcs1-queued.html
[58]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15700/shard-iclb2/igt@gem_ctx_persistence@vcs1-queued.html
* igt@gem_eio@unwedge-stress:
- shard-snb: [FAIL][59] ([i915#232]) -> [PASS][60]
[59]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7545/shard-snb6/igt@gem_eio@unwedge-stress.html
[60]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15700/shard-snb2/igt@gem_eio@unwedge-stress.html
* igt@gem_exec_balancer@bonded-slice:
- shard-tglb: [FAIL][61] ([i915#800]) -> [PASS][62]
[61]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7545/shard-tglb4/igt@gem_exec_balancer@bonded-slice.html
[62]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15700/shard-tglb5/igt@gem_exec_balancer@bonded-slice.html
* igt@gem_exec_parallel@basic:
- shard-tglb: [INCOMPLETE][63] ([i915#476]) -> [PASS][64]
[63]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7545/shard-tglb6/igt@gem_exec_parallel@basic.html
[64]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15700/shard-tglb8/igt@gem_exec_parallel@basic.html
* igt@gem_exec_schedule@fifo-bsd:
- shard-iclb: [SKIP][65] ([fdo#112146]) -> [PASS][66] +1 similar issue
[65]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7545/shard-iclb2/igt@gem_exec_schedule@fifo-bsd.html
[66]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15700/shard-iclb7/igt@gem_exec_schedule@fifo-bsd.html
* {igt@gem_exec_schedule@pi-shared-iova-bsd2}:
- shard-iclb: [SKIP][67] ([fdo#109276]) -> [PASS][68] +8 similar issues
[67]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7545/shard-iclb5/igt@gem_exec_schedule@pi-shared-iova-bsd2.html
[68]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15700/shard-iclb1/igt@gem_exec_schedule@pi-shared-iova-bsd2.html
* igt@gem_exec_schedule@preempt-queue-contexts-blt:
- shard-tglb: [INCOMPLETE][69] ([fdo#111606] / [fdo#111677]) -> [PASS][70]
[69]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7545/shard-tglb6/igt@gem_exec_schedule@preempt-queue-contexts-blt.html
[70]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15700/shard-tglb1/igt@gem_exec_schedule@preempt-queue-contexts-blt.html
* igt@gem_exec_suspend@basic-s0:
- shard-iclb: [DMESG-WARN][71] ([fdo#111764]) -> [PASS][72]
[71]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7545/shard-iclb1/igt@gem_exec_suspend@basic-s0.html
[72]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15700/shard-iclb2/igt@gem_exec_suspend@basic-s0.html
* igt@gem_userptr_blits@map-fixed-invalidate-busy:
- shard-snb: [DMESG-WARN][73] ([fdo#111870]) -> [PASS][74]
[73]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7545/shard-snb6/igt@gem_userptr_blits@map-fixed-invalidate-busy.html
[74]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15700/shard-snb2/igt@gem_userptr_blits@map-fixed-invalidate-busy.html
* igt@i915_selftest@live_requests:
- shard-tglb: [INCOMPLETE][75] ([fdo#112057]) -> [PASS][76]
[75]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7545/shard-tglb7/igt@i915_selftest@live_requests.html
[76]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15700/shard-tglb5/igt@i915_selftest@live_requests.html
* igt@i915_selftest@mock_sanitycheck:
- shard-hsw: [DMESG-WARN][77] ([i915#747]) -> [PASS][78]
[77]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7545/shard-hsw4/igt@i915_selftest@mock_sanitycheck.html
[78]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15700/shard-hsw8/igt@i915_selftest@mock_sanitycheck.html
* igt@i915_suspend@debugfs-reader:
- shard-tglb: [INCOMPLETE][79] ([i915#456] / [i915#460]) -> [PASS][80]
[79]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7545/shard-tglb4/igt@i915_suspend@debugfs-reader.html
[80]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15700/shard-tglb8/igt@i915_suspend@debugfs-reader.html
* igt@i915_suspend@fence-restore-tiled2untiled:
- shard-apl: [DMESG-WARN][81] ([i915#180]) -> [PASS][82] +1 similar issue
[81]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7545/shard-apl1/igt@i915_suspend@fence-restore-tiled2untiled.html
[82]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15700/shard-apl3/igt@i915_suspend@fence-restore-tiled2untiled.html
* igt@kms_cursor_crc@pipe-a-cursor-128x42-sliding:
- shard-skl: [FAIL][83] ([i915#54]) -> [PASS][84] +2 similar issues
[83]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7545/shard-skl5/igt@kms_cursor_crc@pipe-a-cursor-128x42-sliding.html
[84]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15700/shard-skl6/igt@kms_cursor_crc@pipe-a-cursor-128x42-sliding.html
* igt@kms_cursor_crc@pipe-c-cursor-256x256-offscreen:
- shard-hsw: [DMESG-WARN][85] ([IGT#6]) -> [PASS][86]
[85]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7545/shard-hsw6/igt@kms_cursor_crc@pipe-c-cursor-256x256-offscreen.html
[86]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15700/shard-hsw2/igt@kms_cursor_crc@pipe-c-cursor-256x256-offscreen.html
* igt@kms_draw_crc@draw-method-xrgb8888-mmap-cpu-untiled:
- shard-skl: [FAIL][87] ([i915#177] / [i915#52] / [i915#54]) -> [PASS][88]
[87]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7545/shard-skl3/igt@kms_draw_crc@draw-method-xrgb8888-mmap-cpu-untiled.html
[88]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15700/shard-skl8/igt@kms_draw_crc@draw-method-xrgb8888-mmap-cpu-untiled.html
* igt@kms_flip@2x-flip-vs-expired-vblank-interruptible:
- shard-glk: [FAIL][89] ([i915#79]) -> [PASS][90]
[89]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7545/shard-glk7/igt@kms_flip@2x-flip-vs-expired-vblank-interruptible.html
[90]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15700/shard-glk4/igt@kms_flip@2x-flip-vs-expired-vblank-interruptible.html
* igt@kms_flip@flip-vs-suspend:
- shard-snb: [INCOMPLETE][91] ([i915#82]) -> [PASS][92]
[91]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7545/shard-snb1/igt@kms_flip@flip-vs-suspend.html
[92]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15700/shard-snb5/igt@kms_flip@flip-vs-suspend.html
* igt@kms_flip@plain-flip-interruptible:
- shard-hsw: [INCOMPLETE][93] ([i915#61]) -> [PASS][94]
[93]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7545/shard-hsw2/igt@kms_flip@plain-flip-interruptible.html
[94]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15700/shard-hsw5/igt@kms_flip@plain-flip-interruptible.html
* igt@kms_frontbuffer_tracking@fbc-suspend:
- shard-kbl: [DMESG-WARN][95] ([i915#180]) -> [PASS][96] +4 similar issues
[95]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7545/shard-kbl4/igt@kms_frontbuffer_tracking@fbc-suspend.html
[96]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15700/shard-kbl6/igt@kms_frontbuffer_tracking@fbc-suspend.html
* igt@kms_frontbuffer_tracking@fbcpsr-rgb565-draw-pwrite:
- shard-tglb: [FAIL][97] ([i915#49]) -> [PASS][98]
[97]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7545/shard-tglb3/igt@kms_frontbuffer_tracking@fbcpsr-rgb565-draw-pwrite.html
[98]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15700/shard-tglb5/igt@kms_frontbuffer_tracking@fbcpsr-rgb565-draw-pwrite.html
* igt@kms_plane_alpha_blend@pipe-a-coverage-7efc:
- shard-skl: [FAIL][99] ([fdo#108145]) -> [PASS][100] +1 similar issue
[99]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7545/shard-skl8/igt@kms_plane_alpha_blend@pipe-a-coverage-7efc.html
[100]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15700/shard-skl2/igt@kms_plane_alpha_blend@pipe-a-coverage-7efc.html
* igt@kms_psr@psr2_cursor_blt:
- shard-iclb: [SKIP][101] ([fdo#109441]) -> [PASS][102]
[101]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7545/shard-iclb1/igt@kms_psr@psr2_cursor_blt.html
[102]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15700/shard-iclb2/igt@kms_psr@psr2_cursor_blt.html
* igt@kms_sequence@get-forked-busy:
- shard-snb: [SKIP][103] ([fdo#109271]) -> [PASS][104] +1 similar issue
[103]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7545/shard-snb2/igt@kms_sequence@get-forked-busy.html
[104]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15700/shard-snb7/igt@kms_sequence@get-forked-busy.html
* igt@perf_pmu@init-busy-vcs1:
- shard-iclb: [SKIP][105] ([fdo#112080]) -> [PASS][106] +6 similar issues
[105]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7545/shard-iclb5/igt@perf_pmu@init-busy-vcs1.html
[106]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15700/shard-iclb4/igt@perf_pmu@init-busy-vcs1.html
#### Warnings ####
* igt@gem_ctx_isolation@vcs1-nonpriv:
- shard-iclb: [FAIL][107] ([IGT#28]) -> [SKIP][108] ([fdo#109276] / [fdo#112080])
[107]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7545/shard-iclb2/igt@gem_ctx_isolation@vcs1-nonpriv.html
[108]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15700/shard-iclb7/igt@gem_ctx_isolation@vcs1-nonpriv.html
* igt@gem_eio@kms:
- shard-snb: [DMESG-WARN][109] ([i915#444]) -> [INCOMPLETE][110] ([i915#82])
[109]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7545/shard-snb6/igt@gem_eio@kms.html
[110]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15700/shard-snb4/igt@gem_eio@kms.html
* igt@kms_plane@pixel-format-pipe-a-planes:
- shard-skl: [INCOMPLETE][111] ([fdo#112347] / [i915#648] / [i915#667]) -> [INCOMPLETE][112] ([fdo#112391] / [i915#648] / [i915#667])
[111]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7545/shard-skl7/igt@kms_plane@pixel-format-pipe-a-planes.html
[112]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15700/shard-skl2/igt@kms_plane@pixel-format-pipe-a-planes.html
* igt@kms_plane@pixel-format-pipe-b-planes:
- shard-skl: [INCOMPLETE][113] ([fdo#112391] / [i915#648] / [i915#667]) -> [INCOMPLETE][114] ([fdo#112347] / [fdo#112391] / [i915#648] / [i915#667])
[113]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7545/shard-skl10/igt@kms_plane@pixel-format-pipe-b-planes.html
[114]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15700/shard-skl1/igt@kms_plane@pixel-format-pipe-b-planes.html
{name}: This element is suppressed. This means it is ignored when computing
the status of the difference (SUCCESS, WARNING, or FAILURE).
[IGT#28]: https://gitlab.freedesktop.org/drm/igt-gpu-tools/issues/28
[IGT#6]: https://gitlab.freedesktop.org/drm/igt-gpu-tools/issues/6
[fdo#103665]: https://bugs.freedesktop.org/show_bug.cgi?id=103665
[fdo#108145]: https://bugs.freedesktop.org/show_bug.cgi?id=108145
[fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
[fdo#109276]: https://bugs.freedesktop.org/show_bug.cgi?id=109276
[fdo#109441]: https://bugs.freedesktop.org/show_bug.cgi?id=109441
[fdo#110841]: https://bugs.freedesktop.org/show_bug.cgi?id=110841
[fdo#111606]: https://bugs.freedesktop.org/show_bug.cgi?id=111606
[fdo#111677]: https://bugs.freedesktop.org/show_bug.cgi?id=111677
[fdo#111736]: https://bugs.freedesktop.org/show_bug.cgi?id=111736
[fdo#111764]: https://bugs.freedesktop.org/show_bug.cgi?id=111764
[fdo#111870]: https://bugs.freedesktop.org/show_bug.cgi?id=111870
[fdo#112057]: https://bugs.freedesktop.org/show_bug.cgi?id=112057
[fdo#112080]: https://bugs.freedesktop.org/show_bug.cgi?id=112080
[fdo#112146]: https://bugs.freedesktop.org/show_bug.cgi?id=112146
[fdo#112347]: https://bugs.freedesktop.org/show_bug.cgi?id=112347
[fdo#112391]: https://bugs.freedesktop.org/show_bug.cgi?id=112391
[i915#109]: https://gitlab.freedesktop.org/drm/intel/issues/109
[i915#177]: https://gitlab.freedesktop.org/drm/intel/issues/177
[i915#180]: https://gitlab.freedesktop.org/drm/intel/issues/180
[i915#221]: https://gitlab.freedesktop.org/drm/intel/issues/221
[i915#232]: https://gitlab.freedesktop.org/drm/intel/issues/232
[i915#31]: https://gitlab.freedesktop.org/drm/intel/issues/31
[i915#34]: https://gitlab.freedesktop.org/drm/intel/issues/34
[i915#413]: https://gitlab.freedesktop.org/drm/intel/issues/413
[i915#444]: https://gitlab.freedesktop.org/drm/intel/issues/444
[i915#456]: https://gitlab.freedesktop.org/drm/intel/issues/456
[i915#460]: https://gitlab.freedesktop.org/drm/intel/issues/460
[i915#461]: https://gitlab.freedesktop.org/drm/intel/issues/461
[i915#470]: https://gitlab.freedesktop.org/drm/intel/issues/470
[i915#474]: https://gitlab.freedesktop.org/drm/intel/issues/474
[i915#476]: https://gitlab.freedesktop.org/drm/intel/issues/476
[i915#49]: https://gitlab.freedesktop.org/drm/intel/issues/49
[i915#52]: https://gitlab.freedesktop.org/drm/intel/issues/52
[i915#54]: https://gitlab.freedesktop.org/drm/intel/issues/54
[i915#56]: https://gitlab.freedesktop.org/drm/intel/issues/56
[i915#61]: https://gitlab.freedesktop.org/drm/intel/issues/61
[i915#648]: https://gitlab.freedesktop.org/drm/intel/issues/648
[i915#667]: https://gitlab.freedesktop.org/drm/intel/issues/667
[i915#69]: https://gitlab.freedesktop.org/drm/intel/issues/69
[i915#747]: https://gitlab.freedesktop.org/drm/intel/issues/747
[i915#766]: https://gitlab.freedesktop.org/drm/intel/issues/766
[i915#79]: https://gitlab.freedesktop.org/drm/intel/issues/79
[i915#800]: https://gitlab.freedesktop.org/drm/intel/issues/800
[i915#82]: https://gitlab.freedesktop.org/drm/intel/issues/82
Participating hosts (11 -> 11)
------------------------------
No changes in participating hosts
Build changes
-------------
* CI: CI-20190529 -> None
* Linux: CI_DRM_7545 -> Patchwork_15700
CI-20190529: 20190529
CI_DRM_7545: b1b808dff985c3c2050b20771050453589a60ca3 @ git://anongit.freedesktop.org/gfx-ci/linux
IGT_5346: 466b0e6cbcbaccff012b484d1fd7676364b37b93 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
Patchwork_15700: 58e4b57a23e011523571977f21a2d476e7f349fb @ 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_15700/index.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [Intel-gfx] [PATCH 1/3] drm/i915/dp: Make sure all tiled connectors get added to the state with full modeset
2019-12-11 21:14 ` [Intel-gfx] [PATCH 1/3] drm/i915/dp: Make sure all tiled connectors get added to the state with full modeset Manasi Navare
@ 2019-12-13 0:32 ` Matt Roper
2019-12-13 1:18 ` Manasi Navare
2019-12-13 20:05 ` Ville Syrjälä
2019-12-16 14:37 ` Ville Syrjälä
2 siblings, 1 reply; 35+ messages in thread
From: Matt Roper @ 2019-12-13 0:32 UTC (permalink / raw)
To: Manasi Navare; +Cc: intel-gfx
On Wed, Dec 11, 2019 at 01:14:23PM -0800, Manasi Navare wrote:
> In case of tiled displays, all the tiles are linke dto each other
Minor typo on "linked to" here.
> for transcoder port sync. So in intel_atomic_check() we need to make
> sure that we add all the tiles to the modeset and if one of the
> tiles needs a full modeset then mark all other tiles for a full modeset.
>
> Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: José Roberto de Souza <jose.souza@intel.com>
> Bugzilla: https://gitlab.freedesktop.org/drm/intel/issues/5
I think we're moving to "Closes:" as the annotation here now that it's
not actually a bugzilla bug database anymore.
> Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> ---
> drivers/gpu/drm/i915/display/intel_display.c | 78 ++++++++++++++++++++
> 1 file changed, 78 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index 803993a01ca7..7263eaa66cda 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -14066,6 +14066,80 @@ static int intel_atomic_check_crtcs(struct intel_atomic_state *state)
> return 0;
> }
>
> +static int
> +intel_dp_modeset_all_tiles(struct drm_i915_private *dev_priv,
> + struct intel_atomic_state *state, int tile_grp_id)
> +{
> + struct drm_connector *conn_iter;
> + struct drm_connector_list_iter conn_list_iter;
> + struct drm_crtc_state *crtc_state;
> +
> + drm_connector_list_iter_begin(&dev_priv->drm, &conn_list_iter);
> + drm_for_each_connector_iter(conn_iter, &conn_list_iter) {
> + struct drm_connector_state *conn_iter_state;
> +
> + if (!conn_iter->has_tile)
> + continue;
> + conn_iter_state = drm_atomic_get_connector_state(&state->base,
> + conn_iter);
> + if (IS_ERR(conn_iter_state)) {
> + drm_connector_list_iter_end(&conn_list_iter);
> + return PTR_ERR(conn_iter_state);
> + }
> +
> + if (!conn_iter_state->crtc)
> + continue;
> +
> + if (conn_iter->tile_group->id != tile_grp_id)
> + continue;
> +
> + crtc_state = drm_atomic_get_crtc_state(&state->base, conn_iter_state->crtc);
> + if (IS_ERR(crtc_state)) {
> + drm_connector_list_iter_end(&conn_list_iter);
> + return PTR_ERR(conn_iter_state);
> + }
> + crtc_state->mode_changed = true;
> + }
> + drm_connector_list_iter_end(&conn_list_iter);
> +
> + return 0;
> +}
> +
> +static int
> +intel_dp_atomic_trans_port_sync_check(struct drm_i915_private *dev_priv,
> + struct intel_atomic_state *state)
> +{
> + struct drm_connector *connector;
> + struct drm_crtc_state *crtc_state;
> + struct drm_connector_state *connector_state;
> + int i, ret, tile_grp_id = 0;
> +
> + if (INTEL_GEN(dev_priv) < 11)
> + return 0;
> +
> + /* Is tiled, mark all other tiled CRTCs as needing a modeset */
> + for_each_new_connector_in_state(&state->base, connector, connector_state, i) {
> + if (!connector->has_tile)
> + continue;
> + if (connector_state->crtc &&
> + tile_grp_id != connector->tile_group->id) {
> + crtc_state = drm_atomic_get_new_crtc_state(&state->base,
> + connector_state->crtc);
> + if (!drm_atomic_crtc_needs_modeset(crtc_state))
> + continue;
> +
> + tile_grp_id = connector->tile_group->id;
> + } else
Minor kernel coding style violation; if we use {} on one branch of an
if, we need to use them on all.
> + continue;
> +
> + ret = intel_dp_modeset_all_tiles(dev_priv, state, tile_grp_id);
> + if (ret)
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> /**
> * intel_atomic_check - validate state object
> * @dev: drm device
> @@ -14093,6 +14167,10 @@ static int intel_atomic_check(struct drm_device *dev,
> if (ret)
> goto fail;
>
> + ret = intel_dp_atomic_trans_port_sync_check(dev_priv, state);
> + if (ret)
> + goto fail;
Should this happen before the drm_atomic_helper_check_modeset() just
above (or should we re-call that function if we flag the other tile as
needing a modeset)? The kerneldoc on that function says:
"""
Drivers which set &drm_crtc_state.mode_changed [...] _must_ call this
function afterwards after that change. It is permitted to call this
function multiple times for the same update ...
"""
Matt
> +
> for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
> new_crtc_state, i) {
> if (!needs_modeset(new_crtc_state)) {
> --
> 2.19.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Matt Roper
Graphics Software Engineer
VTT-OSGC Platform Enablement
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [Intel-gfx] [PATCH 2/3] drm/i915/dp: Make port sync mode assignments only if all tiles present
2019-12-11 21:14 ` [Intel-gfx] [PATCH 2/3] drm/i915/dp: Make port sync mode assignments only if all tiles present Manasi Navare
@ 2019-12-13 1:03 ` Matt Roper
2019-12-13 1:09 ` Manasi Navare
2019-12-13 20:28 ` Ville Syrjälä
1 sibling, 1 reply; 35+ messages in thread
From: Matt Roper @ 2019-12-13 1:03 UTC (permalink / raw)
To: Manasi Navare; +Cc: intel-gfx
On Wed, Dec 11, 2019 at 01:14:24PM -0800, Manasi Navare wrote:
> Add an extra check before making master slave assignments for tiled
> displays to make sure we make these assignments only if all tiled
> connectors are present. If not then initialize the state to defaults
> so it does a normal non tiled modeset without transcoder port sync.
>
> Bugzilla: https://gitlab.freedesktop.org/drm/intel/issues/5
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> ---
> drivers/gpu/drm/i915/display/intel_display.c | 28 ++++++++++++++++++--
> 1 file changed, 26 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index 7263eaa66cda..c0a2dab3fe67 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -12048,6 +12048,12 @@ static bool c8_planes_changed(const struct intel_crtc_state *new_crtc_state)
> return !old_crtc_state->c8_planes != !new_crtc_state->c8_planes;
> }
>
> +static void initialize_trans_port_sync_mode_state(struct intel_crtc_state *crtc_state)
"reset" rather than "initialize" might be more consistent with similar
things elsewhere in the driver?
> +{
> + crtc_state->master_transcoder = INVALID_TRANSCODER;
> + crtc_state->sync_mode_slaves_mask = 0;
> +}
> +
> static int icl_add_sync_mode_crtcs(struct intel_crtc_state *crtc_state)
> {
> struct drm_crtc *crtc = crtc_state->uapi.crtc;
> @@ -12059,11 +12065,22 @@ static int icl_add_sync_mode_crtcs(struct intel_crtc_state *crtc_state)
> struct drm_crtc *master_crtc = NULL;
> struct drm_crtc_state *master_crtc_state;
> struct intel_crtc_state *master_pipe_config;
> - int i, tile_group_id;
> + int i, tile_group_id = 0, num_tiled_conns = 0;
>
> if (INTEL_GEN(dev_priv) < 11)
> return 0;
>
> + /* If all tiles not present do not make master slave assignments
This comment seems like it should go farther down the function; this
block isn't directly responsible for master/slave assignments, so it's a
bit confusing here. Also, you might want to clarify what "present"
means in this case. E.g., explain that since you've already added all
tiles of the same monitor to the transaction (earlier in
intel_atomic_check), then if the number of tiles in the tile group is
smaller than the size of the tile group it means that at least one of
the tiles isn't active.
> + * Here we assume all tiles belong to the same tile group for now.
Should this sentence be a FIXME:? If we plug in 2x 2-tile monitors on
TGL, this would be problematic, right?
The logic changes seem correct (if we assume that only a single tiled
monitor is in use), so
Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
Matt
> + */
> + for_each_new_connector_in_state(&state->base, connector, connector_state, i) {
> + if (connector->has_tile) {
> + if (!tile_group_id)
> + tile_group_id = connector->tile_group->id;
> + num_tiled_conns++;
> + }
> + }
> +
> /*
> * In case of tiled displays there could be one or more slaves but there is
> * only one master. Lets make the CRTC used by the connector corresponding
> @@ -12077,8 +12094,15 @@ static int icl_add_sync_mode_crtcs(struct intel_crtc_state *crtc_state)
> if (!connector->has_tile)
> continue;
> if (crtc_state->hw.mode.hdisplay != connector->tile_h_size ||
> - crtc_state->hw.mode.vdisplay != connector->tile_v_size)
> + crtc_state->hw.mode.vdisplay != connector->tile_v_size) {
> + initialize_trans_port_sync_mode_state(crtc_state);
> return 0;
> + }
> + if (connector->tile_group->id == tile_group_id &&
> + num_tiled_conns < connector->num_h_tile * connector->num_v_tile) {
> + initialize_trans_port_sync_mode_state(crtc_state);
> + return 0;
> + }
> if (connector->tile_h_loc == connector->num_h_tile - 1 &&
> connector->tile_v_loc == connector->num_v_tile - 1)
> continue;
> --
> 2.19.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Matt Roper
Graphics Software Engineer
VTT-OSGC Platform Enablement
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [Intel-gfx] [PATCH 2/3] drm/i915/dp: Make port sync mode assignments only if all tiles present
2019-12-13 1:03 ` Matt Roper
@ 2019-12-13 1:09 ` Manasi Navare
0 siblings, 0 replies; 35+ messages in thread
From: Manasi Navare @ 2019-12-13 1:09 UTC (permalink / raw)
To: Matt Roper; +Cc: intel-gfx
On Thu, Dec 12, 2019 at 05:03:07PM -0800, Matt Roper wrote:
> On Wed, Dec 11, 2019 at 01:14:24PM -0800, Manasi Navare wrote:
> > Add an extra check before making master slave assignments for tiled
> > displays to make sure we make these assignments only if all tiled
> > connectors are present. If not then initialize the state to defaults
> > so it does a normal non tiled modeset without transcoder port sync.
> >
> > Bugzilla: https://gitlab.freedesktop.org/drm/intel/issues/5
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > ---
> > drivers/gpu/drm/i915/display/intel_display.c | 28 ++++++++++++++++++--
> > 1 file changed, 26 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > index 7263eaa66cda..c0a2dab3fe67 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -12048,6 +12048,12 @@ static bool c8_planes_changed(const struct intel_crtc_state *new_crtc_state)
> > return !old_crtc_state->c8_planes != !new_crtc_state->c8_planes;
> > }
> >
> > +static void initialize_trans_port_sync_mode_state(struct intel_crtc_state *crtc_state)
>
> "reset" rather than "initialize" might be more consistent with similar
> things elsewhere in the driver?
Yes I was considering other names like default/reset/initialize, but yes I think
reset sounds better, will change that
>
> > +{
> > + crtc_state->master_transcoder = INVALID_TRANSCODER;
> > + crtc_state->sync_mode_slaves_mask = 0;
> > +}
> > +
> > static int icl_add_sync_mode_crtcs(struct intel_crtc_state *crtc_state)
> > {
> > struct drm_crtc *crtc = crtc_state->uapi.crtc;
> > @@ -12059,11 +12065,22 @@ static int icl_add_sync_mode_crtcs(struct intel_crtc_state *crtc_state)
> > struct drm_crtc *master_crtc = NULL;
> > struct drm_crtc_state *master_crtc_state;
> > struct intel_crtc_state *master_pipe_config;
> > - int i, tile_group_id;
> > + int i, tile_group_id = 0, num_tiled_conns = 0;
> >
> > if (INTEL_GEN(dev_priv) < 11)
> > return 0;
> >
> > + /* If all tiles not present do not make master slave assignments
>
> This comment seems like it should go farther down the function; this
> block isn't directly responsible for master/slave assignments, so it's a
> bit confusing here. Also, you might want to clarify what "present"
> means in this case. E.g., explain that since you've already added all
> tiles of the same monitor to the transaction (earlier in
> intel_atomic_check), then if the number of tiles in the tile group is
> smaller than the size of the tile group it means that at least one of
> the tiles isn't active.
>
Yes will move this comment down
> > + * Here we assume all tiles belong to the same tile group for now.
Will add a FIXME
I will make these changes and add your r-b, thanks Matt
Manasi
>
> Should this sentence be a FIXME:? If we plug in 2x 2-tile monitors on
> TGL, this would be problematic, right?
>
> The logic changes seem correct (if we assume that only a single tiled
> monitor is in use), so
>
> Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
>
>
> Matt
>
> > + */
> > + for_each_new_connector_in_state(&state->base, connector, connector_state, i) {
> > + if (connector->has_tile) {
> > + if (!tile_group_id)
> > + tile_group_id = connector->tile_group->id;
> > + num_tiled_conns++;
> > + }
> > + }
> > +
> > /*
> > * In case of tiled displays there could be one or more slaves but there is
> > * only one master. Lets make the CRTC used by the connector corresponding
> > @@ -12077,8 +12094,15 @@ static int icl_add_sync_mode_crtcs(struct intel_crtc_state *crtc_state)
> > if (!connector->has_tile)
> > continue;
> > if (crtc_state->hw.mode.hdisplay != connector->tile_h_size ||
> > - crtc_state->hw.mode.vdisplay != connector->tile_v_size)
> > + crtc_state->hw.mode.vdisplay != connector->tile_v_size) {
> > + initialize_trans_port_sync_mode_state(crtc_state);
> > return 0;
> > + }
> > + if (connector->tile_group->id == tile_group_id &&
> > + num_tiled_conns < connector->num_h_tile * connector->num_v_tile) {
> > + initialize_trans_port_sync_mode_state(crtc_state);
> > + return 0;
> > + }
> > if (connector->tile_h_loc == connector->num_h_tile - 1 &&
> > connector->tile_v_loc == connector->num_v_tile - 1)
> > continue;
> > --
> > 2.19.1
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Matt Roper
> Graphics Software Engineer
> VTT-OSGC Platform Enablement
> Intel Corporation
> (916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [Intel-gfx] [PATCH 1/3] drm/i915/dp: Make sure all tiled connectors get added to the state with full modeset
2019-12-13 0:32 ` Matt Roper
@ 2019-12-13 1:18 ` Manasi Navare
2019-12-13 3:11 ` Matt Roper
0 siblings, 1 reply; 35+ messages in thread
From: Manasi Navare @ 2019-12-13 1:18 UTC (permalink / raw)
To: Matt Roper; +Cc: intel-gfx
On Thu, Dec 12, 2019 at 04:32:32PM -0800, Matt Roper wrote:
> On Wed, Dec 11, 2019 at 01:14:23PM -0800, Manasi Navare wrote:
> > In case of tiled displays, all the tiles are linke dto each other
>
> Minor typo on "linked to" here.
I will fix it
>
> > for transcoder port sync. So in intel_atomic_check() we need to make
> > sure that we add all the tiles to the modeset and if one of the
> > tiles needs a full modeset then mark all other tiles for a full modeset.
> >
> > Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: José Roberto de Souza <jose.souza@intel.com>
> > Bugzilla: https://gitlab.freedesktop.org/drm/intel/issues/5
>
> I think we're moving to "Closes:" as the annotation here now that it's
> not actually a bugzilla bug database anymore.
Ok cool, will change that to Closes
>
> > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > ---
> > drivers/gpu/drm/i915/display/intel_display.c | 78 ++++++++++++++++++++
> > 1 file changed, 78 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > index 803993a01ca7..7263eaa66cda 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -14066,6 +14066,80 @@ static int intel_atomic_check_crtcs(struct intel_atomic_state *state)
> > return 0;
> > }
> >
> > +static int
> > +intel_dp_modeset_all_tiles(struct drm_i915_private *dev_priv,
> > + struct intel_atomic_state *state, int tile_grp_id)
> > +{
> > + struct drm_connector *conn_iter;
> > + struct drm_connector_list_iter conn_list_iter;
> > + struct drm_crtc_state *crtc_state;
> > +
> > + drm_connector_list_iter_begin(&dev_priv->drm, &conn_list_iter);
> > + drm_for_each_connector_iter(conn_iter, &conn_list_iter) {
> > + struct drm_connector_state *conn_iter_state;
> > +
> > + if (!conn_iter->has_tile)
> > + continue;
> > + conn_iter_state = drm_atomic_get_connector_state(&state->base,
> > + conn_iter);
> > + if (IS_ERR(conn_iter_state)) {
> > + drm_connector_list_iter_end(&conn_list_iter);
> > + return PTR_ERR(conn_iter_state);
> > + }
> > +
> > + if (!conn_iter_state->crtc)
> > + continue;
> > +
> > + if (conn_iter->tile_group->id != tile_grp_id)
> > + continue;
> > +
> > + crtc_state = drm_atomic_get_crtc_state(&state->base, conn_iter_state->crtc);
> > + if (IS_ERR(crtc_state)) {
> > + drm_connector_list_iter_end(&conn_list_iter);
> > + return PTR_ERR(conn_iter_state);
> > + }
> > + crtc_state->mode_changed = true;
> > + }
> > + drm_connector_list_iter_end(&conn_list_iter);
> > +
> > + return 0;
> > +}
> > +
> > +static int
> > +intel_dp_atomic_trans_port_sync_check(struct drm_i915_private *dev_priv,
> > + struct intel_atomic_state *state)
> > +{
> > + struct drm_connector *connector;
> > + struct drm_crtc_state *crtc_state;
> > + struct drm_connector_state *connector_state;
> > + int i, ret, tile_grp_id = 0;
> > +
> > + if (INTEL_GEN(dev_priv) < 11)
> > + return 0;
> > +
> > + /* Is tiled, mark all other tiled CRTCs as needing a modeset */
> > + for_each_new_connector_in_state(&state->base, connector, connector_state, i) {
> > + if (!connector->has_tile)
> > + continue;
> > + if (connector_state->crtc &&
> > + tile_grp_id != connector->tile_group->id) {
> > + crtc_state = drm_atomic_get_new_crtc_state(&state->base,
> > + connector_state->crtc);
> > + if (!drm_atomic_crtc_needs_modeset(crtc_state))
> > + continue;
> > +
> > + tile_grp_id = connector->tile_group->id;
> > + } else
>
> Minor kernel coding style violation; if we use {} on one branch of an
> if, we need to use them on all.
>
Yes i got a checkpatch check warning, will fix it
> > + continue;
> > +
> > + ret = intel_dp_modeset_all_tiles(dev_priv, state, tile_grp_id);
> > + if (ret)
> > + return ret;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > /**
> > * intel_atomic_check - validate state object
> > * @dev: drm device
> > @@ -14093,6 +14167,10 @@ static int intel_atomic_check(struct drm_device *dev,
> > if (ret)
> > goto fail;
> >
> > + ret = intel_dp_atomic_trans_port_sync_check(dev_priv, state);
> > + if (ret)
> > + goto fail;
>
> Should this happen before the drm_atomic_helper_check_modeset() just
> above (or should we re-call that function if we flag the other tile as
> needing a modeset)? The kerneldoc on that function says:
>
> """
> Drivers which set &drm_crtc_state.mode_changed [...] _must_ call this
> function afterwards after that change. It is permitted to call this
> function multiple times for the same update ...
> """
>
IMO, here infact it makes sense to call my function after the drm_atomic_helper_check_modeset()
because it directly sets the new_crtc_state->mode_changed to true for all tiles if 1 of them needs
a full modeset.
And whether that one tile needs a full modeset or not will be decided based on mode changed for that set
in drm_atomic_helper_check_modeset.
Manasi
>
> Matt
>
> > +
> > for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
> > new_crtc_state, i) {
> > if (!needs_modeset(new_crtc_state)) {
> > --
> > 2.19.1
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Matt Roper
> Graphics Software Engineer
> VTT-OSGC Platform Enablement
> Intel Corporation
> (916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [Intel-gfx] [PATCH 1/3] drm/i915/dp: Make sure all tiled connectors get added to the state with full modeset
2019-12-13 1:18 ` Manasi Navare
@ 2019-12-13 3:11 ` Matt Roper
0 siblings, 0 replies; 35+ messages in thread
From: Matt Roper @ 2019-12-13 3:11 UTC (permalink / raw)
To: Manasi Navare; +Cc: intel-gfx
On Thu, Dec 12, 2019 at 05:18:02PM -0800, Manasi Navare wrote:
> On Thu, Dec 12, 2019 at 04:32:32PM -0800, Matt Roper wrote:
> > On Wed, Dec 11, 2019 at 01:14:23PM -0800, Manasi Navare wrote:
> > > In case of tiled displays, all the tiles are linke dto each other
> >
> > Minor typo on "linked to" here.
>
> I will fix it
>
> >
> > > for transcoder port sync. So in intel_atomic_check() we need to make
> > > sure that we add all the tiles to the modeset and if one of the
> > > tiles needs a full modeset then mark all other tiles for a full modeset.
> > >
> > > Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Cc: José Roberto de Souza <jose.souza@intel.com>
> > > Bugzilla: https://gitlab.freedesktop.org/drm/intel/issues/5
> >
> > I think we're moving to "Closes:" as the annotation here now that it's
> > not actually a bugzilla bug database anymore.
>
> Ok cool, will change that to Closes
>
> >
> > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > > ---
> > > drivers/gpu/drm/i915/display/intel_display.c | 78 ++++++++++++++++++++
> > > 1 file changed, 78 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > > index 803993a01ca7..7263eaa66cda 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > @@ -14066,6 +14066,80 @@ static int intel_atomic_check_crtcs(struct intel_atomic_state *state)
> > > return 0;
> > > }
> > >
> > > +static int
> > > +intel_dp_modeset_all_tiles(struct drm_i915_private *dev_priv,
> > > + struct intel_atomic_state *state, int tile_grp_id)
> > > +{
> > > + struct drm_connector *conn_iter;
> > > + struct drm_connector_list_iter conn_list_iter;
> > > + struct drm_crtc_state *crtc_state;
> > > +
> > > + drm_connector_list_iter_begin(&dev_priv->drm, &conn_list_iter);
> > > + drm_for_each_connector_iter(conn_iter, &conn_list_iter) {
> > > + struct drm_connector_state *conn_iter_state;
> > > +
> > > + if (!conn_iter->has_tile)
> > > + continue;
> > > + conn_iter_state = drm_atomic_get_connector_state(&state->base,
> > > + conn_iter);
> > > + if (IS_ERR(conn_iter_state)) {
> > > + drm_connector_list_iter_end(&conn_list_iter);
> > > + return PTR_ERR(conn_iter_state);
> > > + }
> > > +
> > > + if (!conn_iter_state->crtc)
> > > + continue;
> > > +
> > > + if (conn_iter->tile_group->id != tile_grp_id)
> > > + continue;
> > > +
> > > + crtc_state = drm_atomic_get_crtc_state(&state->base, conn_iter_state->crtc);
> > > + if (IS_ERR(crtc_state)) {
> > > + drm_connector_list_iter_end(&conn_list_iter);
> > > + return PTR_ERR(conn_iter_state);
> > > + }
> > > + crtc_state->mode_changed = true;
> > > + }
> > > + drm_connector_list_iter_end(&conn_list_iter);
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static int
> > > +intel_dp_atomic_trans_port_sync_check(struct drm_i915_private *dev_priv,
> > > + struct intel_atomic_state *state)
> > > +{
> > > + struct drm_connector *connector;
> > > + struct drm_crtc_state *crtc_state;
> > > + struct drm_connector_state *connector_state;
> > > + int i, ret, tile_grp_id = 0;
> > > +
> > > + if (INTEL_GEN(dev_priv) < 11)
> > > + return 0;
> > > +
> > > + /* Is tiled, mark all other tiled CRTCs as needing a modeset */
> > > + for_each_new_connector_in_state(&state->base, connector, connector_state, i) {
> > > + if (!connector->has_tile)
> > > + continue;
> > > + if (connector_state->crtc &&
> > > + tile_grp_id != connector->tile_group->id) {
> > > + crtc_state = drm_atomic_get_new_crtc_state(&state->base,
> > > + connector_state->crtc);
> > > + if (!drm_atomic_crtc_needs_modeset(crtc_state))
> > > + continue;
> > > +
> > > + tile_grp_id = connector->tile_group->id;
> > > + } else
> >
> > Minor kernel coding style violation; if we use {} on one branch of an
> > if, we need to use them on all.
> >
>
> Yes i got a checkpatch check warning, will fix it
>
> > > + continue;
> > > +
> > > + ret = intel_dp_modeset_all_tiles(dev_priv, state, tile_grp_id);
> > > + if (ret)
> > > + return ret;
> > > + }
> > > +
> > > + return 0;
> > > +}
> > > +
> > > /**
> > > * intel_atomic_check - validate state object
> > > * @dev: drm device
> > > @@ -14093,6 +14167,10 @@ static int intel_atomic_check(struct drm_device *dev,
> > > if (ret)
> > > goto fail;
> > >
> > > + ret = intel_dp_atomic_trans_port_sync_check(dev_priv, state);
> > > + if (ret)
> > > + goto fail;
> >
> > Should this happen before the drm_atomic_helper_check_modeset() just
> > above (or should we re-call that function if we flag the other tile as
> > needing a modeset)? The kerneldoc on that function says:
> >
> > """
> > Drivers which set &drm_crtc_state.mode_changed [...] _must_ call this
> > function afterwards after that change. It is permitted to call this
> > function multiple times for the same update ...
> > """
> >
>
> IMO, here infact it makes sense to call my function after the drm_atomic_helper_check_modeset()
> because it directly sets the new_crtc_state->mode_changed to true for all tiles if 1 of them needs
> a full modeset.
> And whether that one tile needs a full modeset or not will be decided based on mode changed for that set
> in drm_atomic_helper_check_modeset.
Okay, makes sense. But based on the comment, I believe we still need to
call the helper a second time during/after your function in the event
that we've switched any additional crtcs to mode_changed=true.
I'm also wondering whether we should eventually move most of your logic
here out of i915 and into a general drm helper that other drivers can
utilize too. But I'm okay with doing that as a followup after we've
landed it and verified it as an i915-specific change first.
Matt
>
> Manasi
>
> >
> > Matt
> >
> > > +
> > > for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
> > > new_crtc_state, i) {
> > > if (!needs_modeset(new_crtc_state)) {
> > > --
> > > 2.19.1
> > >
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >
> > --
> > Matt Roper
> > Graphics Software Engineer
> > VTT-OSGC Platform Enablement
> > Intel Corporation
> > (916) 356-2795
--
Matt Roper
Graphics Software Engineer
VTT-OSGC Platform Enablement
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [Intel-gfx] [PATCH 3/3] drm/i915/dp: Disable Port sync mode correctly on teardown
2019-12-11 21:14 ` [Intel-gfx] [PATCH 3/3] drm/i915/dp: Disable Port sync mode correctly on teardown Manasi Navare
@ 2019-12-13 3:14 ` Matt Roper
2019-12-13 20:06 ` Ville Syrjälä
1 sibling, 0 replies; 35+ messages in thread
From: Matt Roper @ 2019-12-13 3:14 UTC (permalink / raw)
To: Manasi Navare; +Cc: Jani Nikula, intel-gfx
On Wed, Dec 11, 2019 at 01:14:25PM -0800, Manasi Navare wrote:
> While clearing the Ports ync mode enable and master select bits
> we need to make sure that we perform a RMW for disable else
> it sets the other bits casuing unwanted sideeffects.
>
> Bugzilla: https://gitlab.freedesktop.org/drm/intel/issues/5
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Fixes: 51528afe7c5e ("drm/i915/display/icl: Disable transcoder port sync as part of crtc_disable() sequence")
> Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
> ---
> drivers/gpu/drm/i915/display/intel_display.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index c0a2dab3fe67..3fccda0f1f36 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -4599,7 +4599,8 @@ static void icl_disable_transcoder_port_sync(const struct intel_crtc_state *old_
> transcoder_name(old_crtc_state->cpu_transcoder));
>
> reg = TRANS_DDI_FUNC_CTL2(old_crtc_state->cpu_transcoder);
> - trans_ddi_func_ctl2_val = ~(PORT_SYNC_MODE_ENABLE |
> + trans_ddi_func_ctl2_val = I915_READ(reg);
> + trans_ddi_func_ctl2_val &= ~(PORT_SYNC_MODE_ENABLE |
> PORT_SYNC_MODE_MASTER_SELECT_MASK);
> I915_WRITE(reg, trans_ddi_func_ctl2_val);
> }
> --
> 2.19.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Matt Roper
Graphics Software Engineer
VTT-OSGC Platform Enablement
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [Intel-gfx] [PATCH 1/3] drm/i915/dp: Make sure all tiled connectors get added to the state with full modeset
2019-12-11 21:14 ` [Intel-gfx] [PATCH 1/3] drm/i915/dp: Make sure all tiled connectors get added to the state with full modeset Manasi Navare
2019-12-13 0:32 ` Matt Roper
@ 2019-12-13 20:05 ` Ville Syrjälä
2019-12-13 21:05 ` Manasi Navare
2019-12-14 2:28 ` Manasi Navare
2019-12-16 14:37 ` Ville Syrjälä
2 siblings, 2 replies; 35+ messages in thread
From: Ville Syrjälä @ 2019-12-13 20:05 UTC (permalink / raw)
To: Manasi Navare; +Cc: intel-gfx
On Wed, Dec 11, 2019 at 01:14:23PM -0800, Manasi Navare wrote:
> In case of tiled displays, all the tiles are linke dto each other
> for transcoder port sync. So in intel_atomic_check() we need to make
> sure that we add all the tiles to the modeset and if one of the
> tiles needs a full modeset then mark all other tiles for a full modeset.
>
> Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: José Roberto de Souza <jose.souza@intel.com>
> Bugzilla: https://gitlab.freedesktop.org/drm/intel/issues/5
> Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> ---
> drivers/gpu/drm/i915/display/intel_display.c | 78 ++++++++++++++++++++
> 1 file changed, 78 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index 803993a01ca7..7263eaa66cda 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -14066,6 +14066,80 @@ static int intel_atomic_check_crtcs(struct intel_atomic_state *state)
> return 0;
> }
>
> +static int
> +intel_dp_modeset_all_tiles(struct drm_i915_private *dev_priv,
> + struct intel_atomic_state *state, int tile_grp_id)
> +{
> + struct drm_connector *conn_iter;
'connector'
> + struct drm_connector_list_iter conn_list_iter;
> + struct drm_crtc_state *crtc_state;
crtc_state has needlessly wide scope.
> +
> + drm_connector_list_iter_begin(&dev_priv->drm, &conn_list_iter);
> + drm_for_each_connector_iter(conn_iter, &conn_list_iter) {
> + struct drm_connector_state *conn_iter_state;
'conn_state' is the most popular name.
> +
> + if (!conn_iter->has_tile)
> + continue;
> + conn_iter_state = drm_atomic_get_connector_state(&state->base,
> + conn_iter);
> + if (IS_ERR(conn_iter_state)) {
> + drm_connector_list_iter_end(&conn_list_iter);
> + return PTR_ERR(conn_iter_state);
> + }
> +
> + if (!conn_iter_state->crtc)
> + continue;
> +
> + if (conn_iter->tile_group->id != tile_grp_id)
> + continue;
The tile group check should be part of the same if with the has_tile
check.
> +
> + crtc_state = drm_atomic_get_crtc_state(&state->base, conn_iter_state->crtc);
> + if (IS_ERR(crtc_state)) {
> + drm_connector_list_iter_end(&conn_list_iter);
> + return PTR_ERR(conn_iter_state);
> + }
> + crtc_state->mode_changed = true;
> + }
> + drm_connector_list_iter_end(&conn_list_iter);
> +
> + return 0;
> +}
> +
> +static int
> +intel_dp_atomic_trans_port_sync_check(struct drm_i915_private *dev_priv,
Pointless variable. Can be extracted from the atomic state.
> + struct intel_atomic_state *state)
> +{
> + struct drm_connector *connector;
> + struct drm_crtc_state *crtc_state;
> + struct drm_connector_state *connector_state;
> + int i, ret, tile_grp_id = 0;
tile_grp_id is rather pointless. crtc_state and ret can move into
tighter scope. And the next suggestion allows you to kill crtc_state
entirely...
> +
> + if (INTEL_GEN(dev_priv) < 11)
> + return 0;
> +
> + /* Is tiled, mark all other tiled CRTCs as needing a modeset */
> + for_each_new_connector_in_state(&state->base, connector, connector_state, i) {
> + if (!connector->has_tile)
> + continue;
> + if (connector_state->crtc &&
> + tile_grp_id != connector->tile_group->id) {
> + crtc_state = drm_atomic_get_new_crtc_state(&state->base,
> + connector_state->crtc);
> + if (!drm_atomic_crtc_needs_modeset(crtc_state))
> + continue;
This should to be able to be shortened to just intel_connector_needs_modeset().
> +
> + tile_grp_id = connector->tile_group->id;
> + } else
> + continue;
> +
> + ret = intel_dp_modeset_all_tiles(dev_priv, state, tile_grp_id);
> + if (ret)
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> /**
> * intel_atomic_check - validate state object
> * @dev: drm device
> @@ -14093,6 +14167,10 @@ static int intel_atomic_check(struct drm_device *dev,
> if (ret)
> goto fail;
>
> + ret = intel_dp_atomic_trans_port_sync_check(dev_priv, state);
> + if (ret)
> + goto fail;
We should probably do this from the connector .atomic_check() hook if
Jose is going to do the MST thing that way. But no real problem doing
here I think.
> +
> for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
> new_crtc_state, i) {
> if (!needs_modeset(new_crtc_state)) {
> --
> 2.19.1
--
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [Intel-gfx] [PATCH 3/3] drm/i915/dp: Disable Port sync mode correctly on teardown
2019-12-11 21:14 ` [Intel-gfx] [PATCH 3/3] drm/i915/dp: Disable Port sync mode correctly on teardown Manasi Navare
2019-12-13 3:14 ` Matt Roper
@ 2019-12-13 20:06 ` Ville Syrjälä
2019-12-13 20:40 ` Manasi Navare
1 sibling, 1 reply; 35+ messages in thread
From: Ville Syrjälä @ 2019-12-13 20:06 UTC (permalink / raw)
To: Manasi Navare; +Cc: Jani Nikula, intel-gfx
On Wed, Dec 11, 2019 at 01:14:25PM -0800, Manasi Navare wrote:
> While clearing the Ports ync mode enable and master select bits
> we need to make sure that we perform a RMW for disable else
> it sets the other bits casuing unwanted sideeffects.
>
> Bugzilla: https://gitlab.freedesktop.org/drm/intel/issues/5
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Fixes: 51528afe7c5e ("drm/i915/display/icl: Disable transcoder port sync as part of crtc_disable() sequence")
> Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> ---
> drivers/gpu/drm/i915/display/intel_display.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index c0a2dab3fe67..3fccda0f1f36 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -4599,7 +4599,8 @@ static void icl_disable_transcoder_port_sync(const struct intel_crtc_state *old_
> transcoder_name(old_crtc_state->cpu_transcoder));
>
> reg = TRANS_DDI_FUNC_CTL2(old_crtc_state->cpu_transcoder);
> - trans_ddi_func_ctl2_val = ~(PORT_SYNC_MODE_ENABLE |
> + trans_ddi_func_ctl2_val = I915_READ(reg);
> + trans_ddi_func_ctl2_val &= ~(PORT_SYNC_MODE_ENABLE |
> PORT_SYNC_MODE_MASTER_SELECT_MASK);
> I915_WRITE(reg, trans_ddi_func_ctl2_val);
I915_WRITE(TRANS_DDI_FUNC_CTL2, 0);
> }
> --
> 2.19.1
--
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [Intel-gfx] [PATCH 2/3] drm/i915/dp: Make port sync mode assignments only if all tiles present
2019-12-11 21:14 ` [Intel-gfx] [PATCH 2/3] drm/i915/dp: Make port sync mode assignments only if all tiles present Manasi Navare
2019-12-13 1:03 ` Matt Roper
@ 2019-12-13 20:28 ` Ville Syrjälä
2019-12-13 20:53 ` Ville Syrjälä
2019-12-13 20:58 ` Manasi Navare
1 sibling, 2 replies; 35+ messages in thread
From: Ville Syrjälä @ 2019-12-13 20:28 UTC (permalink / raw)
To: Manasi Navare; +Cc: intel-gfx
On Wed, Dec 11, 2019 at 01:14:24PM -0800, Manasi Navare wrote:
> Add an extra check before making master slave assignments for tiled
> displays to make sure we make these assignments only if all tiled
> connectors are present. If not then initialize the state to defaults
> so it does a normal non tiled modeset without transcoder port sync.
>
> Bugzilla: https://gitlab.freedesktop.org/drm/intel/issues/5
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> ---
> drivers/gpu/drm/i915/display/intel_display.c | 28 ++++++++++++++++++--
> 1 file changed, 26 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index 7263eaa66cda..c0a2dab3fe67 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -12048,6 +12048,12 @@ static bool c8_planes_changed(const struct intel_crtc_state *new_crtc_state)
> return !old_crtc_state->c8_planes != !new_crtc_state->c8_planes;
> }
>
> +static void initialize_trans_port_sync_mode_state(struct intel_crtc_state *crtc_state)
> +{
> + crtc_state->master_transcoder = INVALID_TRANSCODER;
> + crtc_state->sync_mode_slaves_mask = 0;
> +}
> +
> static int icl_add_sync_mode_crtcs(struct intel_crtc_state *crtc_state)
> {
> struct drm_crtc *crtc = crtc_state->uapi.crtc;
> @@ -12059,11 +12065,22 @@ static int icl_add_sync_mode_crtcs(struct intel_crtc_state *crtc_state)
> struct drm_crtc *master_crtc = NULL;
> struct drm_crtc_state *master_crtc_state;
> struct intel_crtc_state *master_pipe_config;
> - int i, tile_group_id;
> + int i, tile_group_id = 0, num_tiled_conns = 0;
>
> if (INTEL_GEN(dev_priv) < 11)
> return 0;
>
> + /* If all tiles not present do not make master slave assignments
> + * Here we assume all tiles belong to the same tile group for now.
> + */
> + for_each_new_connector_in_state(&state->base, connector, connector_state, i) {
> + if (connector->has_tile) {
> + if (!tile_group_id)
> + tile_group_id = connector->tile_group->id;
Isn't 0 a valid tile group id?
> + num_tiled_conns++;
> + }
This whole thing looks confused. Should it not just look for the same
tile group as what the current connector belongs to?
> + }
> +
> /*
> * In case of tiled displays there could be one or more slaves but there is
> * only one master. Lets make the CRTC used by the connector corresponding
> @@ -12077,8 +12094,15 @@ static int icl_add_sync_mode_crtcs(struct intel_crtc_state *crtc_state)
> if (!connector->has_tile)
> continue;
> if (crtc_state->hw.mode.hdisplay != connector->tile_h_size ||
> - crtc_state->hw.mode.vdisplay != connector->tile_v_size)
> + crtc_state->hw.mode.vdisplay != connector->tile_v_size) {
> + initialize_trans_port_sync_mode_state(crtc_state);
> return 0;
> + }
> + if (connector->tile_group->id == tile_group_id &&
> + num_tiled_conns < connector->num_h_tile * connector->num_v_tile) {
> + initialize_trans_port_sync_mode_state(crtc_state);
> + return 0;
> + }
> if (connector->tile_h_loc == connector->num_h_tile - 1 &&
> connector->tile_v_loc == connector->num_v_tile - 1)
> continue;
This whole thing seems kinda overly complicated. I suggest it should
just blindly go through all connectors of the same tile group and pick
the lowest transcoder as the master, which is the logic Jose is using
for MST. Except I guess we have to special case the EDP transcoder
for port sync since it can't be a slave. So a simple numeric comparison
won't quite do like used for MST.
And then we should probably move this thing to the encoder
.compute_config(). I suppose it should look in the end something like:
compute_config() {
...
crtc_state->master = compute_master_transcoder();
crtc_state->slaves = 0;
if (master_transcoder == cpu_transcoder)
crtc_state->master = INVALID;
crtc_state->slave = compute_slave_transcoders();
}
}
That keeps it very readable and avoids the confusing stuff of
comptue_config() for one pipe randomly mutating the states of
the other pipes.
--
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [Intel-gfx] [PATCH 3/3] drm/i915/dp: Disable Port sync mode correctly on teardown
2019-12-13 20:06 ` Ville Syrjälä
@ 2019-12-13 20:40 ` Manasi Navare
2019-12-13 20:49 ` Ville Syrjälä
0 siblings, 1 reply; 35+ messages in thread
From: Manasi Navare @ 2019-12-13 20:40 UTC (permalink / raw)
To: Ville Syrjälä; +Cc: Jani Nikula, intel-gfx
On Fri, Dec 13, 2019 at 10:06:37PM +0200, Ville Syrjälä wrote:
> On Wed, Dec 11, 2019 at 01:14:25PM -0800, Manasi Navare wrote:
> > While clearing the Ports ync mode enable and master select bits
> > we need to make sure that we perform a RMW for disable else
> > it sets the other bits casuing unwanted sideeffects.
> >
> > Bugzilla: https://gitlab.freedesktop.org/drm/intel/issues/5
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: Jani Nikula <jani.nikula@intel.com>
> > Fixes: 51528afe7c5e ("drm/i915/display/icl: Disable transcoder port sync as part of crtc_disable() sequence")
> > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > ---
> > drivers/gpu/drm/i915/display/intel_display.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > index c0a2dab3fe67..3fccda0f1f36 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -4599,7 +4599,8 @@ static void icl_disable_transcoder_port_sync(const struct intel_crtc_state *old_
> > transcoder_name(old_crtc_state->cpu_transcoder));
> >
> > reg = TRANS_DDI_FUNC_CTL2(old_crtc_state->cpu_transcoder);
> > - trans_ddi_func_ctl2_val = ~(PORT_SYNC_MODE_ENABLE |
> > + trans_ddi_func_ctl2_val = I915_READ(reg);
> > + trans_ddi_func_ctl2_val &= ~(PORT_SYNC_MODE_ENABLE |
> > PORT_SYNC_MODE_MASTER_SELECT_MASK);
> > I915_WRITE(reg, trans_ddi_func_ctl2_val);
>
> I915_WRITE(TRANS_DDI_FUNC_CTL2, 0);
So not even consider the other values that might have been set in this reg?
You would prefer setting this to 0 directly?
Right now i do see that no other bits are set, but things can change when we
start using DSI port sync mode or genlock mode etc.
Manasi
>
> > }
> > --
> > 2.19.1
>
> --
> Ville Syrjälä
> Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [Intel-gfx] [PATCH 3/3] drm/i915/dp: Disable Port sync mode correctly on teardown
2019-12-13 20:40 ` Manasi Navare
@ 2019-12-13 20:49 ` Ville Syrjälä
0 siblings, 0 replies; 35+ messages in thread
From: Ville Syrjälä @ 2019-12-13 20:49 UTC (permalink / raw)
To: Manasi Navare; +Cc: Jani Nikula, intel-gfx
On Fri, Dec 13, 2019 at 12:40:13PM -0800, Manasi Navare wrote:
> On Fri, Dec 13, 2019 at 10:06:37PM +0200, Ville Syrjälä wrote:
> > On Wed, Dec 11, 2019 at 01:14:25PM -0800, Manasi Navare wrote:
> > > While clearing the Ports ync mode enable and master select bits
> > > we need to make sure that we perform a RMW for disable else
> > > it sets the other bits casuing unwanted sideeffects.
> > >
> > > Bugzilla: https://gitlab.freedesktop.org/drm/intel/issues/5
> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Cc: Jani Nikula <jani.nikula@intel.com>
> > > Fixes: 51528afe7c5e ("drm/i915/display/icl: Disable transcoder port sync as part of crtc_disable() sequence")
> > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > > ---
> > > drivers/gpu/drm/i915/display/intel_display.c | 3 ++-
> > > 1 file changed, 2 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > > index c0a2dab3fe67..3fccda0f1f36 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > @@ -4599,7 +4599,8 @@ static void icl_disable_transcoder_port_sync(const struct intel_crtc_state *old_
> > > transcoder_name(old_crtc_state->cpu_transcoder));
> > >
> > > reg = TRANS_DDI_FUNC_CTL2(old_crtc_state->cpu_transcoder);
> > > - trans_ddi_func_ctl2_val = ~(PORT_SYNC_MODE_ENABLE |
> > > + trans_ddi_func_ctl2_val = I915_READ(reg);
> > > + trans_ddi_func_ctl2_val &= ~(PORT_SYNC_MODE_ENABLE |
> > > PORT_SYNC_MODE_MASTER_SELECT_MASK);
> > > I915_WRITE(reg, trans_ddi_func_ctl2_val);
> >
> > I915_WRITE(TRANS_DDI_FUNC_CTL2, 0);
>
> So not even consider the other values that might have been set in this reg?
> You would prefer setting this to 0 directly?
> Right now i do see that no other bits are set, but things can change when we
> start using DSI port sync mode or genlock mode etc.
No point in making it more complicated than it has to be.
In fact after the series I just posted we should probably just
suck this whole thing into intel_ddi_{enable,disable}_transcoder_func()
since on older platforms these bits live in TRANS_DDI_FUNC_CTL itself.
>
> Manasi
>
> >
> > > }
> > > --
> > > 2.19.1
> >
> > --
> > Ville Syrjälä
> > Intel
--
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [Intel-gfx] [PATCH 2/3] drm/i915/dp: Make port sync mode assignments only if all tiles present
2019-12-13 20:28 ` Ville Syrjälä
@ 2019-12-13 20:53 ` Ville Syrjälä
2019-12-13 20:58 ` Manasi Navare
1 sibling, 0 replies; 35+ messages in thread
From: Ville Syrjälä @ 2019-12-13 20:53 UTC (permalink / raw)
To: Manasi Navare; +Cc: intel-gfx
On Fri, Dec 13, 2019 at 10:28:49PM +0200, Ville Syrjälä wrote:
> On Wed, Dec 11, 2019 at 01:14:24PM -0800, Manasi Navare wrote:
> > Add an extra check before making master slave assignments for tiled
> > displays to make sure we make these assignments only if all tiled
> > connectors are present. If not then initialize the state to defaults
> > so it does a normal non tiled modeset without transcoder port sync.
> >
> > Bugzilla: https://gitlab.freedesktop.org/drm/intel/issues/5
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > ---
> > drivers/gpu/drm/i915/display/intel_display.c | 28 ++++++++++++++++++--
> > 1 file changed, 26 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > index 7263eaa66cda..c0a2dab3fe67 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -12048,6 +12048,12 @@ static bool c8_planes_changed(const struct intel_crtc_state *new_crtc_state)
> > return !old_crtc_state->c8_planes != !new_crtc_state->c8_planes;
> > }
> >
> > +static void initialize_trans_port_sync_mode_state(struct intel_crtc_state *crtc_state)
> > +{
> > + crtc_state->master_transcoder = INVALID_TRANSCODER;
> > + crtc_state->sync_mode_slaves_mask = 0;
> > +}
> > +
> > static int icl_add_sync_mode_crtcs(struct intel_crtc_state *crtc_state)
> > {
> > struct drm_crtc *crtc = crtc_state->uapi.crtc;
> > @@ -12059,11 +12065,22 @@ static int icl_add_sync_mode_crtcs(struct intel_crtc_state *crtc_state)
> > struct drm_crtc *master_crtc = NULL;
> > struct drm_crtc_state *master_crtc_state;
> > struct intel_crtc_state *master_pipe_config;
> > - int i, tile_group_id;
> > + int i, tile_group_id = 0, num_tiled_conns = 0;
> >
> > if (INTEL_GEN(dev_priv) < 11)
> > return 0;
> >
> > + /* If all tiles not present do not make master slave assignments
> > + * Here we assume all tiles belong to the same tile group for now.
> > + */
> > + for_each_new_connector_in_state(&state->base, connector, connector_state, i) {
> > + if (connector->has_tile) {
> > + if (!tile_group_id)
> > + tile_group_id = connector->tile_group->id;
>
> Isn't 0 a valid tile group id?
>
> > + num_tiled_conns++;
> > + }
>
> This whole thing looks confused. Should it not just look for the same
> tile group as what the current connector belongs to?
>
> > + }
> > +
> > /*
> > * In case of tiled displays there could be one or more slaves but there is
> > * only one master. Lets make the CRTC used by the connector corresponding
> > @@ -12077,8 +12094,15 @@ static int icl_add_sync_mode_crtcs(struct intel_crtc_state *crtc_state)
> > if (!connector->has_tile)
> > continue;
> > if (crtc_state->hw.mode.hdisplay != connector->tile_h_size ||
> > - crtc_state->hw.mode.vdisplay != connector->tile_v_size)
> > + crtc_state->hw.mode.vdisplay != connector->tile_v_size) {
> > + initialize_trans_port_sync_mode_state(crtc_state);
> > return 0;
> > + }
> > + if (connector->tile_group->id == tile_group_id &&
> > + num_tiled_conns < connector->num_h_tile * connector->num_v_tile) {
> > + initialize_trans_port_sync_mode_state(crtc_state);
> > + return 0;
> > + }
> > if (connector->tile_h_loc == connector->num_h_tile - 1 &&
> > connector->tile_v_loc == connector->num_v_tile - 1)
> > continue;
>
> This whole thing seems kinda overly complicated. I suggest it should
> just blindly go through all connectors of the same tile group and pick
> the lowest transcoder as the master, which is the logic Jose is using
> for MST. Except I guess we have to special case the EDP transcoder
> for port sync since it can't be a slave. So a simple numeric comparison
> won't quite do like used for MST.
Hmm. Except that won't actually work since .cpu_transcoder won't have
been populated yet for the later pipes. So we can't use that in
.compute_config(). So either we do this after .compute_config()
is done for everyone or we just calculate the cpu_transcoder on the
spot (I mean it's just a trivial port==A->EDP else->pipe so no big deal).
>
> And then we should probably move this thing to the encoder
> .compute_config(). I suppose it should look in the end something like:
>
> compute_config() {
> ...
> crtc_state->master = compute_master_transcoder();
> crtc_state->slaves = 0;
> if (master_transcoder == cpu_transcoder)
> crtc_state->master = INVALID;
> crtc_state->slave = compute_slave_transcoders();
> }
> }
>
> That keeps it very readable and avoids the confusing stuff of
> comptue_config() for one pipe randomly mutating the states of
> the other pipes.
>
> --
> Ville Syrjälä
> Intel
--
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [Intel-gfx] [PATCH 2/3] drm/i915/dp: Make port sync mode assignments only if all tiles present
2019-12-13 20:28 ` Ville Syrjälä
2019-12-13 20:53 ` Ville Syrjälä
@ 2019-12-13 20:58 ` Manasi Navare
1 sibling, 0 replies; 35+ messages in thread
From: Manasi Navare @ 2019-12-13 20:58 UTC (permalink / raw)
To: Ville Syrjälä; +Cc: intel-gfx
On Fri, Dec 13, 2019 at 10:28:49PM +0200, Ville Syrjälä wrote:
> On Wed, Dec 11, 2019 at 01:14:24PM -0800, Manasi Navare wrote:
> > Add an extra check before making master slave assignments for tiled
> > displays to make sure we make these assignments only if all tiled
> > connectors are present. If not then initialize the state to defaults
> > so it does a normal non tiled modeset without transcoder port sync.
> >
> > Bugzilla: https://gitlab.freedesktop.org/drm/intel/issues/5
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > ---
> > drivers/gpu/drm/i915/display/intel_display.c | 28 ++++++++++++++++++--
> > 1 file changed, 26 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > index 7263eaa66cda..c0a2dab3fe67 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -12048,6 +12048,12 @@ static bool c8_planes_changed(const struct intel_crtc_state *new_crtc_state)
> > return !old_crtc_state->c8_planes != !new_crtc_state->c8_planes;
> > }
> >
> > +static void initialize_trans_port_sync_mode_state(struct intel_crtc_state *crtc_state)
> > +{
> > + crtc_state->master_transcoder = INVALID_TRANSCODER;
> > + crtc_state->sync_mode_slaves_mask = 0;
> > +}
> > +
> > static int icl_add_sync_mode_crtcs(struct intel_crtc_state *crtc_state)
> > {
> > struct drm_crtc *crtc = crtc_state->uapi.crtc;
> > @@ -12059,11 +12065,22 @@ static int icl_add_sync_mode_crtcs(struct intel_crtc_state *crtc_state)
> > struct drm_crtc *master_crtc = NULL;
> > struct drm_crtc_state *master_crtc_state;
> > struct intel_crtc_state *master_pipe_config;
> > - int i, tile_group_id;
> > + int i, tile_group_id = 0, num_tiled_conns = 0;
> >
> > if (INTEL_GEN(dev_priv) < 11)
> > return 0;
> >
> > + /* If all tiles not present do not make master slave assignments
> > + * Here we assume all tiles belong to the same tile group for now.
> > + */
> > + for_each_new_connector_in_state(&state->base, connector, connector_state, i) {
> > + if (connector->has_tile) {
> > + if (!tile_group_id)
> > + tile_group_id = connector->tile_group->id;
>
> Isn't 0 a valid tile group id?
>
> > + num_tiled_conns++;
> > + }
>
> This whole thing looks confused. Should it not just look for the same
> tile group as what the current connector belongs to?
I was making sure that we dont count the connectors that dont belong to the same tile grp id.
But yes I agree that the tile grp id can be assigned by current connector's tile grp id
and add num_conns only if they are of the same tile grp id.
>
> > + }
> > +
> > /*
> > * In case of tiled displays there could be one or more slaves but there is
> > * only one master. Lets make the CRTC used by the connector corresponding
> > @@ -12077,8 +12094,15 @@ static int icl_add_sync_mode_crtcs(struct intel_crtc_state *crtc_state)
> > if (!connector->has_tile)
> > continue;
> > if (crtc_state->hw.mode.hdisplay != connector->tile_h_size ||
> > - crtc_state->hw.mode.vdisplay != connector->tile_v_size)
> > + crtc_state->hw.mode.vdisplay != connector->tile_v_size) {
> > + initialize_trans_port_sync_mode_state(crtc_state);
> > return 0;
> > + }
> > + if (connector->tile_group->id == tile_group_id &&
> > + num_tiled_conns < connector->num_h_tile * connector->num_v_tile) {
> > + initialize_trans_port_sync_mode_state(crtc_state);
> > + return 0;
> > + }
> > if (connector->tile_h_loc == connector->num_h_tile - 1 &&
> > connector->tile_v_loc == connector->num_v_tile - 1)
> > continue;
>
> This whole thing seems kinda overly complicated. I suggest it should
> just blindly go through all connectors of the same tile group and pick
> the lowest transcoder as the master, which is the logic Jose is using
> for MST. Except I guess we have to special case the EDP transcoder
> for port sync since it can't be a slave. So a simple numeric comparison
> won't quite do like used for MST.
>
> And then we should probably move this thing to the encoder
> .compute_config(). I suppose it should look in the end something like:
>
> compute_config() {
> ...
> crtc_state->master = compute_master_transcoder();
> crtc_state->slaves = 0;
> if (master_transcoder == cpu_transcoder)
> crtc_state->master = INVALID;
> crtc_state->slave = compute_slave_transcoders();
> }
> }
>
> That keeps it very readable and avoids the confusing stuff of
> comptue_config() for one pipe randomly mutating the states of
> the other pipes.
But the logic that we are using is always make the connector corresponding to the
last tile the master and all other connectors as slaves since only 1 master (last tile)
and multiple slaves.
This was agreed upon with discussions with you and Danvet and Maarten almost 6 months back
and the entire enable/disable sequence is based on this.
I would rather keep the logic same for now since the master slave asisgnments which was
designed based on consensus during the initial patches and since its working well now.
The only thing that I wanted to add in this patch is reset the master_trans and slave_bitmask
properly if all tiles not present in order to handle the hotplug case correctly.
I will make changes for the tile grp id that you suggested for now and we can simplify the logic later
as enhncements/optimizations.
Sounds good?
Manasi
>
> --
> Ville Syrjälä
> Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [Intel-gfx] [PATCH 1/3] drm/i915/dp: Make sure all tiled connectors get added to the state with full modeset
2019-12-13 20:05 ` Ville Syrjälä
@ 2019-12-13 21:05 ` Manasi Navare
2019-12-13 21:17 ` Ville Syrjälä
2019-12-14 2:28 ` Manasi Navare
1 sibling, 1 reply; 35+ messages in thread
From: Manasi Navare @ 2019-12-13 21:05 UTC (permalink / raw)
To: Ville Syrjälä; +Cc: intel-gfx
On Fri, Dec 13, 2019 at 10:05:49PM +0200, Ville Syrjälä wrote:
> On Wed, Dec 11, 2019 at 01:14:23PM -0800, Manasi Navare wrote:
> > In case of tiled displays, all the tiles are linke dto each other
> > for transcoder port sync. So in intel_atomic_check() we need to make
> > sure that we add all the tiles to the modeset and if one of the
> > tiles needs a full modeset then mark all other tiles for a full modeset.
> >
> > Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: José Roberto de Souza <jose.souza@intel.com>
> > Bugzilla: https://gitlab.freedesktop.org/drm/intel/issues/5
> > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > ---
> > drivers/gpu/drm/i915/display/intel_display.c | 78 ++++++++++++++++++++
> > 1 file changed, 78 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > index 803993a01ca7..7263eaa66cda 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -14066,6 +14066,80 @@ static int intel_atomic_check_crtcs(struct intel_atomic_state *state)
> > return 0;
> > }
> >
> > +static int
> > +intel_dp_modeset_all_tiles(struct drm_i915_private *dev_priv,
> > + struct intel_atomic_state *state, int tile_grp_id)
> > +{
> > + struct drm_connector *conn_iter;
> 'connector'
Ok
> > + struct drm_connector_list_iter conn_list_iter;
> > + struct drm_crtc_state *crtc_state;
>
> crtc_state has needlessly wide scope.
Ok will add it inside the loop
>
> > +
> > + drm_connector_list_iter_begin(&dev_priv->drm, &conn_list_iter);
> > + drm_for_each_connector_iter(conn_iter, &conn_list_iter) {
> > + struct drm_connector_state *conn_iter_state;
>
> 'conn_state' is the most popular name.
>
> > +
> > + if (!conn_iter->has_tile)
> > + continue;
> > + conn_iter_state = drm_atomic_get_connector_state(&state->base,
> > + conn_iter);
> > + if (IS_ERR(conn_iter_state)) {
> > + drm_connector_list_iter_end(&conn_list_iter);
> > + return PTR_ERR(conn_iter_state);
> > + }
> > +
> > + if (!conn_iter_state->crtc)
> > + continue;
> > +
> > + if (conn_iter->tile_group->id != tile_grp_id)
> > + continue;
>
> The tile group check should be part of the same if with the has_tile
> check.
Ok yes. So not pass the tile grp id at all just compare with current
connector's tile_grp_id ?
>
> > +
> > + crtc_state = drm_atomic_get_crtc_state(&state->base, conn_iter_state->crtc);
> > + if (IS_ERR(crtc_state)) {
> > + drm_connector_list_iter_end(&conn_list_iter);
> > + return PTR_ERR(conn_iter_state);
> > + }
> > + crtc_state->mode_changed = true;
> > + }
> > + drm_connector_list_iter_end(&conn_list_iter);
> > +
> > + return 0;
> > +}
> > +
> > +static int
> > +intel_dp_atomic_trans_port_sync_check(struct drm_i915_private *dev_priv,
>
> Pointless variable. Can be extracted from the atomic state.
>
> > + struct intel_atomic_state *state)
> > +{
> > + struct drm_connector *connector;
> > + struct drm_crtc_state *crtc_state;
> > + struct drm_connector_state *connector_state;
> > + int i, ret, tile_grp_id = 0;
>
> tile_grp_id is rather pointless. crtc_state and ret can move into
> tighter scope. And the next suggestion allows you to kill crtc_state
> entirely...
tile_grp_id is useless because I should just use it from current connector obtained from state?
>
> > +
> > + if (INTEL_GEN(dev_priv) < 11)
> > + return 0;
> > +
> > + /* Is tiled, mark all other tiled CRTCs as needing a modeset */
> > + for_each_new_connector_in_state(&state->base, connector, connector_state, i) {
> > + if (!connector->has_tile)
> > + continue;
> > + if (connector_state->crtc &&
> > + tile_grp_id != connector->tile_group->id) {
> > + crtc_state = drm_atomic_get_new_crtc_state(&state->base,
> > + connector_state->crtc);
> > + if (!drm_atomic_crtc_needs_modeset(crtc_state))
> > + continue;
>
> This should to be able to be shortened to just intel_connector_needs_modeset().
Ok will use that instead.
>
> > +
> > + tile_grp_id = connector->tile_group->id;
> > + } else
> > + continue;
> > +
> > + ret = intel_dp_modeset_all_tiles(dev_priv, state, tile_grp_id);
> > + if (ret)
> > + return ret;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > /**
> > * intel_atomic_check - validate state object
> > * @dev: drm device
> > @@ -14093,6 +14167,10 @@ static int intel_atomic_check(struct drm_device *dev,
> > if (ret)
> > goto fail;
> >
> > + ret = intel_dp_atomic_trans_port_sync_check(dev_priv, state);
> > + if (ret)
> > + goto fail;
>
> We should probably do this from the connector .atomic_check() hook if
> Jose is going to do the MST thing that way. But no real problem doing
> here I think.
If you dont have a strong preference I will leave it here since Jose's code is not in yet.
Manasi
>
> > +
> > for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
> > new_crtc_state, i) {
> > if (!needs_modeset(new_crtc_state)) {
> > --
> > 2.19.1
>
> --
> Ville Syrjälä
> Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [Intel-gfx] [PATCH 1/3] drm/i915/dp: Make sure all tiled connectors get added to the state with full modeset
2019-12-13 21:05 ` Manasi Navare
@ 2019-12-13 21:17 ` Ville Syrjälä
0 siblings, 0 replies; 35+ messages in thread
From: Ville Syrjälä @ 2019-12-13 21:17 UTC (permalink / raw)
To: Manasi Navare; +Cc: intel-gfx
On Fri, Dec 13, 2019 at 01:05:48PM -0800, Manasi Navare wrote:
> On Fri, Dec 13, 2019 at 10:05:49PM +0200, Ville Syrjälä wrote:
> > On Wed, Dec 11, 2019 at 01:14:23PM -0800, Manasi Navare wrote:
> > > In case of tiled displays, all the tiles are linke dto each other
> > > for transcoder port sync. So in intel_atomic_check() we need to make
> > > sure that we add all the tiles to the modeset and if one of the
> > > tiles needs a full modeset then mark all other tiles for a full modeset.
> > >
> > > Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Cc: José Roberto de Souza <jose.souza@intel.com>
> > > Bugzilla: https://gitlab.freedesktop.org/drm/intel/issues/5
> > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > > ---
> > > drivers/gpu/drm/i915/display/intel_display.c | 78 ++++++++++++++++++++
> > > 1 file changed, 78 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > > index 803993a01ca7..7263eaa66cda 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > @@ -14066,6 +14066,80 @@ static int intel_atomic_check_crtcs(struct intel_atomic_state *state)
> > > return 0;
> > > }
> > >
> > > +static int
> > > +intel_dp_modeset_all_tiles(struct drm_i915_private *dev_priv,
> > > + struct intel_atomic_state *state, int tile_grp_id)
> > > +{
> > > + struct drm_connector *conn_iter;
> > 'connector'
>
> Ok
>
> > > + struct drm_connector_list_iter conn_list_iter;
> > > + struct drm_crtc_state *crtc_state;
> >
> > crtc_state has needlessly wide scope.
>
> Ok will add it inside the loop
>
> >
> > > +
> > > + drm_connector_list_iter_begin(&dev_priv->drm, &conn_list_iter);
> > > + drm_for_each_connector_iter(conn_iter, &conn_list_iter) {
> > > + struct drm_connector_state *conn_iter_state;
> >
> > 'conn_state' is the most popular name.
> >
> > > +
> > > + if (!conn_iter->has_tile)
> > > + continue;
> > > + conn_iter_state = drm_atomic_get_connector_state(&state->base,
> > > + conn_iter);
> > > + if (IS_ERR(conn_iter_state)) {
> > > + drm_connector_list_iter_end(&conn_list_iter);
> > > + return PTR_ERR(conn_iter_state);
> > > + }
> > > +
> > > + if (!conn_iter_state->crtc)
> > > + continue;
> > > +
> > > + if (conn_iter->tile_group->id != tile_grp_id)
> > > + continue;
> >
> > The tile group check should be part of the same if with the has_tile
> > check.
>
> Ok yes. So not pass the tile grp id at all just compare with current
> connector's tile_grp_id ?
You still need to pass in the tile_grp_id from the caller's current
connector. What I meant is that you should do
if (!connector->has_tile || connector->tile_grp != tile_grp)
continue;
before you do the drm_atomic_get_connector_state(). We don't want to
needlessly add connectors belonging to some other tile group to our
atomic state.
>
> >
> > > +
> > > + crtc_state = drm_atomic_get_crtc_state(&state->base, conn_iter_state->crtc);
> > > + if (IS_ERR(crtc_state)) {
> > > + drm_connector_list_iter_end(&conn_list_iter);
> > > + return PTR_ERR(conn_iter_state);
> > > + }
> > > + crtc_state->mode_changed = true;
> > > + }
> > > + drm_connector_list_iter_end(&conn_list_iter);
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static int
> > > +intel_dp_atomic_trans_port_sync_check(struct drm_i915_private *dev_priv,
> >
> > Pointless variable. Can be extracted from the atomic state.
> >
> > > + struct intel_atomic_state *state)
> > > +{
> > > + struct drm_connector *connector;
> > > + struct drm_crtc_state *crtc_state;
> > > + struct drm_connector_state *connector_state;
> > > + int i, ret, tile_grp_id = 0;
> >
> > tile_grp_id is rather pointless. crtc_state and ret can move into
> > tighter scope. And the next suggestion allows you to kill crtc_state
> > entirely...
>
> tile_grp_id is useless because I should just use it from current connector obtained from state?
I just meant that you use that variable exactly once. So instead you can
just directly do:
ret = intel_dp_modeset_all_tiles(state, connector->tile_grp_id);
...
>
> >
> > > +
> > > + if (INTEL_GEN(dev_priv) < 11)
> > > + return 0;
> > > +
> > > + /* Is tiled, mark all other tiled CRTCs as needing a modeset */
> > > + for_each_new_connector_in_state(&state->base, connector, connector_state, i) {
> > > + if (!connector->has_tile)
> > > + continue;
> > > + if (connector_state->crtc &&
> > > + tile_grp_id != connector->tile_group->id) {
> > > + crtc_state = drm_atomic_get_new_crtc_state(&state->base,
> > > + connector_state->crtc);
> > > + if (!drm_atomic_crtc_needs_modeset(crtc_state))
> > > + continue;
> >
> > This should to be able to be shortened to just intel_connector_needs_modeset().
>
> Ok will use that instead.
>
> >
> > > +
> > > + tile_grp_id = connector->tile_group->id;
> > > + } else
> > > + continue;
> > > +
> > > + ret = intel_dp_modeset_all_tiles(dev_priv, state, tile_grp_id);
> > > + if (ret)
> > > + return ret;
> > > + }
> > > +
> > > + return 0;
> > > +}
> > > +
> > > /**
> > > * intel_atomic_check - validate state object
> > > * @dev: drm device
> > > @@ -14093,6 +14167,10 @@ static int intel_atomic_check(struct drm_device *dev,
> > > if (ret)
> > > goto fail;
> > >
> > > + ret = intel_dp_atomic_trans_port_sync_check(dev_priv, state);
> > > + if (ret)
> > > + goto fail;
> >
> > We should probably do this from the connector .atomic_check() hook if
> > Jose is going to do the MST thing that way. But no real problem doing
> > here I think.
>
> If you dont have a strong preference I will leave it here since Jose's code is not in yet.
>
> Manasi
>
> >
> > > +
> > > for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
> > > new_crtc_state, i) {
> > > if (!needs_modeset(new_crtc_state)) {
> > > --
> > > 2.19.1
> >
> > --
> > Ville Syrjälä
> > Intel
--
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [Intel-gfx] [PATCH 1/3] drm/i915/dp: Make sure all tiled connectors get added to the state with full modeset
2019-12-13 20:05 ` Ville Syrjälä
2019-12-13 21:05 ` Manasi Navare
@ 2019-12-14 2:28 ` Manasi Navare
2019-12-16 12:03 ` Ville Syrjälä
1 sibling, 1 reply; 35+ messages in thread
From: Manasi Navare @ 2019-12-14 2:28 UTC (permalink / raw)
To: Ville Syrjälä; +Cc: intel-gfx
On Fri, Dec 13, 2019 at 10:05:49PM +0200, Ville Syrjälä wrote:
> On Wed, Dec 11, 2019 at 01:14:23PM -0800, Manasi Navare wrote:
> > In case of tiled displays, all the tiles are linke dto each other
> > for transcoder port sync. So in intel_atomic_check() we need to make
> > sure that we add all the tiles to the modeset and if one of the
> > tiles needs a full modeset then mark all other tiles for a full modeset.
> >
> > Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: José Roberto de Souza <jose.souza@intel.com>
> > Bugzilla: https://gitlab.freedesktop.org/drm/intel/issues/5
> > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > ---
> > drivers/gpu/drm/i915/display/intel_display.c | 78 ++++++++++++++++++++
> > 1 file changed, 78 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > index 803993a01ca7..7263eaa66cda 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -14066,6 +14066,80 @@ static int intel_atomic_check_crtcs(struct intel_atomic_state *state)
> > return 0;
> > }
> >
> > +static int
> > +intel_dp_modeset_all_tiles(struct drm_i915_private *dev_priv,
> > + struct intel_atomic_state *state, int tile_grp_id)
> > +{
> > + struct drm_connector *conn_iter;
> 'connector'
> > + struct drm_connector_list_iter conn_list_iter;
> > + struct drm_crtc_state *crtc_state;
>
> crtc_state has needlessly wide scope.
>
> > +
> > + drm_connector_list_iter_begin(&dev_priv->drm, &conn_list_iter);
> > + drm_for_each_connector_iter(conn_iter, &conn_list_iter) {
> > + struct drm_connector_state *conn_iter_state;
>
> 'conn_state' is the most popular name.
>
> > +
> > + if (!conn_iter->has_tile)
> > + continue;
> > + conn_iter_state = drm_atomic_get_connector_state(&state->base,
> > + conn_iter);
> > + if (IS_ERR(conn_iter_state)) {
> > + drm_connector_list_iter_end(&conn_list_iter);
> > + return PTR_ERR(conn_iter_state);
> > + }
> > +
> > + if (!conn_iter_state->crtc)
> > + continue;
> > +
> > + if (conn_iter->tile_group->id != tile_grp_id)
> > + continue;
>
> The tile group check should be part of the same if with the has_tile
> check.
>
> > +
> > + crtc_state = drm_atomic_get_crtc_state(&state->base, conn_iter_state->crtc);
> > + if (IS_ERR(crtc_state)) {
> > + drm_connector_list_iter_end(&conn_list_iter);
> > + return PTR_ERR(conn_iter_state);
> > + }
> > + crtc_state->mode_changed = true;
> > + }
> > + drm_connector_list_iter_end(&conn_list_iter);
> > +
> > + return 0;
> > +}
> > +
> > +static int
> > +intel_dp_atomic_trans_port_sync_check(struct drm_i915_private *dev_priv,
>
> Pointless variable. Can be extracted from the atomic state.
>
> > + struct intel_atomic_state *state)
> > +{
> > + struct drm_connector *connector;
> > + struct drm_crtc_state *crtc_state;
> > + struct drm_connector_state *connector_state;
> > + int i, ret, tile_grp_id = 0;
>
> tile_grp_id is rather pointless. crtc_state and ret can move into
> tighter scope. And the next suggestion allows you to kill crtc_state
> entirely...
Its not clear why tile_grp_id is pointless, I am using tile_grp_id for the first connector with has_tile
and I make sure that I dont enter into the loop to check modeset again for the connector with
same tile_grp_id because we have already set its mode changed to true in intel_dp_modeset_all_tiles()
How can I achieve this instead?
Manasi
>
> > +
> > + if (INTEL_GEN(dev_priv) < 11)
> > + return 0;
> > +
> > + /* Is tiled, mark all other tiled CRTCs as needing a modeset */
> > + for_each_new_connector_in_state(&state->base, connector, connector_state, i) {
> > + if (!connector->has_tile)
> > + continue;
> > + if (connector_state->crtc &&
> > + tile_grp_id != connector->tile_group->id) {
> > + crtc_state = drm_atomic_get_new_crtc_state(&state->base,
> > + connector_state->crtc);
> > + if (!drm_atomic_crtc_needs_modeset(crtc_state))
> > + continue;
>
> This should to be able to be shortened to just intel_connector_needs_modeset().
>
> > +
> > + tile_grp_id = connector->tile_group->id;
> > + } else
> > + continue;
> > +
> > + ret = intel_dp_modeset_all_tiles(dev_priv, state, tile_grp_id);
> > + if (ret)
> > + return ret;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > /**
> > * intel_atomic_check - validate state object
> > * @dev: drm device
> > @@ -14093,6 +14167,10 @@ static int intel_atomic_check(struct drm_device *dev,
> > if (ret)
> > goto fail;
> >
> > + ret = intel_dp_atomic_trans_port_sync_check(dev_priv, state);
> > + if (ret)
> > + goto fail;
>
> We should probably do this from the connector .atomic_check() hook if
> Jose is going to do the MST thing that way. But no real problem doing
> here I think.
>
> > +
> > for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
> > new_crtc_state, i) {
> > if (!needs_modeset(new_crtc_state)) {
> > --
> > 2.19.1
>
> --
> Ville Syrjälä
> Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [Intel-gfx] [PATCH 1/3] drm/i915/dp: Make sure all tiled connectors get added to the state with full modeset
2019-12-14 2:28 ` Manasi Navare
@ 2019-12-16 12:03 ` Ville Syrjälä
2019-12-16 16:40 ` Manasi Navare
0 siblings, 1 reply; 35+ messages in thread
From: Ville Syrjälä @ 2019-12-16 12:03 UTC (permalink / raw)
To: Manasi Navare; +Cc: intel-gfx
On Fri, Dec 13, 2019 at 06:28:40PM -0800, Manasi Navare wrote:
> On Fri, Dec 13, 2019 at 10:05:49PM +0200, Ville Syrjälä wrote:
> > On Wed, Dec 11, 2019 at 01:14:23PM -0800, Manasi Navare wrote:
> > > In case of tiled displays, all the tiles are linke dto each other
> > > for transcoder port sync. So in intel_atomic_check() we need to make
> > > sure that we add all the tiles to the modeset and if one of the
> > > tiles needs a full modeset then mark all other tiles for a full modeset.
> > >
> > > Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Cc: José Roberto de Souza <jose.souza@intel.com>
> > > Bugzilla: https://gitlab.freedesktop.org/drm/intel/issues/5
> > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > > ---
> > > drivers/gpu/drm/i915/display/intel_display.c | 78 ++++++++++++++++++++
> > > 1 file changed, 78 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > > index 803993a01ca7..7263eaa66cda 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > @@ -14066,6 +14066,80 @@ static int intel_atomic_check_crtcs(struct intel_atomic_state *state)
> > > return 0;
> > > }
> > >
> > > +static int
> > > +intel_dp_modeset_all_tiles(struct drm_i915_private *dev_priv,
> > > + struct intel_atomic_state *state, int tile_grp_id)
> > > +{
> > > + struct drm_connector *conn_iter;
> > 'connector'
> > > + struct drm_connector_list_iter conn_list_iter;
> > > + struct drm_crtc_state *crtc_state;
> >
> > crtc_state has needlessly wide scope.
> >
> > > +
> > > + drm_connector_list_iter_begin(&dev_priv->drm, &conn_list_iter);
> > > + drm_for_each_connector_iter(conn_iter, &conn_list_iter) {
> > > + struct drm_connector_state *conn_iter_state;
> >
> > 'conn_state' is the most popular name.
> >
> > > +
> > > + if (!conn_iter->has_tile)
> > > + continue;
> > > + conn_iter_state = drm_atomic_get_connector_state(&state->base,
> > > + conn_iter);
> > > + if (IS_ERR(conn_iter_state)) {
> > > + drm_connector_list_iter_end(&conn_list_iter);
> > > + return PTR_ERR(conn_iter_state);
> > > + }
> > > +
> > > + if (!conn_iter_state->crtc)
> > > + continue;
> > > +
> > > + if (conn_iter->tile_group->id != tile_grp_id)
> > > + continue;
> >
> > The tile group check should be part of the same if with the has_tile
> > check.
> >
> > > +
> > > + crtc_state = drm_atomic_get_crtc_state(&state->base, conn_iter_state->crtc);
> > > + if (IS_ERR(crtc_state)) {
> > > + drm_connector_list_iter_end(&conn_list_iter);
> > > + return PTR_ERR(conn_iter_state);
> > > + }
> > > + crtc_state->mode_changed = true;
> > > + }
> > > + drm_connector_list_iter_end(&conn_list_iter);
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static int
> > > +intel_dp_atomic_trans_port_sync_check(struct drm_i915_private *dev_priv,
> >
> > Pointless variable. Can be extracted from the atomic state.
> >
> > > + struct intel_atomic_state *state)
> > > +{
> > > + struct drm_connector *connector;
> > > + struct drm_crtc_state *crtc_state;
> > > + struct drm_connector_state *connector_state;
> > > + int i, ret, tile_grp_id = 0;
> >
> > tile_grp_id is rather pointless. crtc_state and ret can move into
> > tighter scope. And the next suggestion allows you to kill crtc_state
> > entirely...
>
> Its not clear why tile_grp_id is pointless, I am using tile_grp_id for the first connector with has_tile
> and I make sure that I dont enter into the loop to check modeset again for the connector with
> same tile_grp_id because we have already set its mode changed to true in intel_dp_modeset_all_tiles()
>
> How can I achieve this instead?
Instead of
foo()
{
tile_grp_id = 0;
for_each() {
tile_grp_id = conn->tile_grp_id;
intel_dp_modeset_all_tiles(tile_grp_id);
}
}
you just do
foo()
{
for_each() {
intel_dp_modeset_all_tiles(conn->tile_grp_id);
}
}
--
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [Intel-gfx] [PATCH 1/3] drm/i915/dp: Make sure all tiled connectors get added to the state with full modeset
2019-12-11 21:14 ` [Intel-gfx] [PATCH 1/3] drm/i915/dp: Make sure all tiled connectors get added to the state with full modeset Manasi Navare
2019-12-13 0:32 ` Matt Roper
2019-12-13 20:05 ` Ville Syrjälä
@ 2019-12-16 14:37 ` Ville Syrjälä
2019-12-16 19:13 ` Manasi Navare
2 siblings, 1 reply; 35+ messages in thread
From: Ville Syrjälä @ 2019-12-16 14:37 UTC (permalink / raw)
To: Manasi Navare; +Cc: intel-gfx
On Wed, Dec 11, 2019 at 01:14:23PM -0800, Manasi Navare wrote:
> In case of tiled displays, all the tiles are linke dto each other
> for transcoder port sync. So in intel_atomic_check() we need to make
> sure that we add all the tiles to the modeset and if one of the
> tiles needs a full modeset then mark all other tiles for a full modeset.
>
> Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: José Roberto de Souza <jose.souza@intel.com>
> Bugzilla: https://gitlab.freedesktop.org/drm/intel/issues/5
> Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> ---
> drivers/gpu/drm/i915/display/intel_display.c | 78 ++++++++++++++++++++
> 1 file changed, 78 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index 803993a01ca7..7263eaa66cda 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -14066,6 +14066,80 @@ static int intel_atomic_check_crtcs(struct intel_atomic_state *state)
> return 0;
> }
>
> +static int
> +intel_dp_modeset_all_tiles(struct drm_i915_private *dev_priv,
> + struct intel_atomic_state *state, int tile_grp_id)
> +{
> + struct drm_connector *conn_iter;
> + struct drm_connector_list_iter conn_list_iter;
> + struct drm_crtc_state *crtc_state;
> +
> + drm_connector_list_iter_begin(&dev_priv->drm, &conn_list_iter);
> + drm_for_each_connector_iter(conn_iter, &conn_list_iter) {
> + struct drm_connector_state *conn_iter_state;
> +
> + if (!conn_iter->has_tile)
> + continue;
> + conn_iter_state = drm_atomic_get_connector_state(&state->base,
> + conn_iter);
> + if (IS_ERR(conn_iter_state)) {
> + drm_connector_list_iter_end(&conn_list_iter);
> + return PTR_ERR(conn_iter_state);
> + }
> +
> + if (!conn_iter_state->crtc)
> + continue;
> +
> + if (conn_iter->tile_group->id != tile_grp_id)
> + continue;
> +
> + crtc_state = drm_atomic_get_crtc_state(&state->base, conn_iter_state->crtc);
> + if (IS_ERR(crtc_state)) {
> + drm_connector_list_iter_end(&conn_list_iter);
> + return PTR_ERR(conn_iter_state);
> + }
> + crtc_state->mode_changed = true;
> + }
> + drm_connector_list_iter_end(&conn_list_iter);
> +
> + return 0;
> +}
> +
> +static int
> +intel_dp_atomic_trans_port_sync_check(struct drm_i915_private *dev_priv,
> + struct intel_atomic_state *state)
> +{
> + struct drm_connector *connector;
> + struct drm_crtc_state *crtc_state;
> + struct drm_connector_state *connector_state;
> + int i, ret, tile_grp_id = 0;
> +
> + if (INTEL_GEN(dev_priv) < 11)
> + return 0;
> +
> + /* Is tiled, mark all other tiled CRTCs as needing a modeset */
> + for_each_new_connector_in_state(&state->base, connector, connector_state, i) {
> + if (!connector->has_tile)
> + continue;
> + if (connector_state->crtc &&
> + tile_grp_id != connector->tile_group->id) {
> + crtc_state = drm_atomic_get_new_crtc_state(&state->base,
> + connector_state->crtc);
> + if (!drm_atomic_crtc_needs_modeset(crtc_state))
> + continue;
> +
> + tile_grp_id = connector->tile_group->id;
> + } else
> + continue;
> +
> + ret = intel_dp_modeset_all_tiles(dev_priv, state, tile_grp_id);
> + if (ret)
> + return ret;
> + }
> +
> + return 0;
> +}
BTW after some more pondering I don't think this alone is sufficient.
The tile information may have already disppeared so I believe we also
need to make sure we mark all currently synced crtcs as needing a
modeset if any of them need a modeset. And I guess that's pretty much
the same function we'll need to handle fastset correctly.
> +
> /**
> * intel_atomic_check - validate state object
> * @dev: drm device
> @@ -14093,6 +14167,10 @@ static int intel_atomic_check(struct drm_device *dev,
> if (ret)
> goto fail;
>
> + ret = intel_dp_atomic_trans_port_sync_check(dev_priv, state);
> + if (ret)
> + goto fail;
> +
> for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
> new_crtc_state, i) {
> if (!needs_modeset(new_crtc_state)) {
> --
> 2.19.1
--
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [Intel-gfx] [PATCH 1/3] drm/i915/dp: Make sure all tiled connectors get added to the state with full modeset
2019-12-16 12:03 ` Ville Syrjälä
@ 2019-12-16 16:40 ` Manasi Navare
2019-12-16 17:11 ` Ville Syrjälä
0 siblings, 1 reply; 35+ messages in thread
From: Manasi Navare @ 2019-12-16 16:40 UTC (permalink / raw)
To: Ville Syrjälä; +Cc: intel-gfx
On Mon, Dec 16, 2019 at 02:03:43PM +0200, Ville Syrjälä wrote:
> On Fri, Dec 13, 2019 at 06:28:40PM -0800, Manasi Navare wrote:
> > On Fri, Dec 13, 2019 at 10:05:49PM +0200, Ville Syrjälä wrote:
> > > On Wed, Dec 11, 2019 at 01:14:23PM -0800, Manasi Navare wrote:
> > > > In case of tiled displays, all the tiles are linke dto each other
> > > > for transcoder port sync. So in intel_atomic_check() we need to make
> > > > sure that we add all the tiles to the modeset and if one of the
> > > > tiles needs a full modeset then mark all other tiles for a full modeset.
> > > >
> > > > Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > Cc: José Roberto de Souza <jose.souza@intel.com>
> > > > Bugzilla: https://gitlab.freedesktop.org/drm/intel/issues/5
> > > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > > > ---
> > > > drivers/gpu/drm/i915/display/intel_display.c | 78 ++++++++++++++++++++
> > > > 1 file changed, 78 insertions(+)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > > > index 803993a01ca7..7263eaa66cda 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > > @@ -14066,6 +14066,80 @@ static int intel_atomic_check_crtcs(struct intel_atomic_state *state)
> > > > return 0;
> > > > }
> > > >
> > > > +static int
> > > > +intel_dp_modeset_all_tiles(struct drm_i915_private *dev_priv,
> > > > + struct intel_atomic_state *state, int tile_grp_id)
> > > > +{
> > > > + struct drm_connector *conn_iter;
> > > 'connector'
> > > > + struct drm_connector_list_iter conn_list_iter;
> > > > + struct drm_crtc_state *crtc_state;
> > >
> > > crtc_state has needlessly wide scope.
> > >
> > > > +
> > > > + drm_connector_list_iter_begin(&dev_priv->drm, &conn_list_iter);
> > > > + drm_for_each_connector_iter(conn_iter, &conn_list_iter) {
> > > > + struct drm_connector_state *conn_iter_state;
> > >
> > > 'conn_state' is the most popular name.
> > >
> > > > +
> > > > + if (!conn_iter->has_tile)
> > > > + continue;
> > > > + conn_iter_state = drm_atomic_get_connector_state(&state->base,
> > > > + conn_iter);
> > > > + if (IS_ERR(conn_iter_state)) {
> > > > + drm_connector_list_iter_end(&conn_list_iter);
> > > > + return PTR_ERR(conn_iter_state);
> > > > + }
> > > > +
> > > > + if (!conn_iter_state->crtc)
> > > > + continue;
> > > > +
> > > > + if (conn_iter->tile_group->id != tile_grp_id)
> > > > + continue;
> > >
> > > The tile group check should be part of the same if with the has_tile
> > > check.
> > >
> > > > +
> > > > + crtc_state = drm_atomic_get_crtc_state(&state->base, conn_iter_state->crtc);
> > > > + if (IS_ERR(crtc_state)) {
> > > > + drm_connector_list_iter_end(&conn_list_iter);
> > > > + return PTR_ERR(conn_iter_state);
> > > > + }
> > > > + crtc_state->mode_changed = true;
> > > > + }
> > > > + drm_connector_list_iter_end(&conn_list_iter);
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > +static int
> > > > +intel_dp_atomic_trans_port_sync_check(struct drm_i915_private *dev_priv,
> > >
> > > Pointless variable. Can be extracted from the atomic state.
> > >
> > > > + struct intel_atomic_state *state)
> > > > +{
> > > > + struct drm_connector *connector;
> > > > + struct drm_crtc_state *crtc_state;
> > > > + struct drm_connector_state *connector_state;
> > > > + int i, ret, tile_grp_id = 0;
> > >
> > > tile_grp_id is rather pointless. crtc_state and ret can move into
> > > tighter scope. And the next suggestion allows you to kill crtc_state
> > > entirely...
> >
> > Its not clear why tile_grp_id is pointless, I am using tile_grp_id for the first connector with has_tile
> > and I make sure that I dont enter into the loop to check modeset again for the connector with
> > same tile_grp_id because we have already set its mode changed to true in intel_dp_modeset_all_tiles()
> >
> > How can I achieve this instead?
>
>
> Instead of
> foo()
> {
> tile_grp_id = 0;
>
> for_each() {
> tile_grp_id = conn->tile_grp_id;
>
> intel_dp_modeset_all_tiles(tile_grp_id);
> }
> }
>
> you just do
> foo()
> {
> for_each() {
> intel_dp_modeset_all_tiles(conn->tile_grp_id);
> }
> }
Yes I understand that we can pass the conn->tile_grp_id directly. But currently I am using tile_grp_id in for_each(), to call intel_dp_modeset_all_tiles()
only if that cnnector is from a differnt tile grp id else continue.
foo()
{
for_each() {
if(tile_grp_id != conn->tile_grp_id)
intel_dpmodeset_all_tiles();
else
continue;
}
}
calling intel_dp_modeset_all_tiles() for all tiles is kind of redundant. How can I do this with your suggestion, or do you think
we should call intel_dp_modeset_all_tiles for each connector anyway and it shouldnt matter?
Regards
Manasi
>
> --
> Ville Syrjälä
> Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [Intel-gfx] [PATCH 1/3] drm/i915/dp: Make sure all tiled connectors get added to the state with full modeset
2019-12-16 16:40 ` Manasi Navare
@ 2019-12-16 17:11 ` Ville Syrjälä
2019-12-16 21:42 ` Manasi Navare
0 siblings, 1 reply; 35+ messages in thread
From: Ville Syrjälä @ 2019-12-16 17:11 UTC (permalink / raw)
To: Manasi Navare; +Cc: intel-gfx
On Mon, Dec 16, 2019 at 08:40:24AM -0800, Manasi Navare wrote:
> On Mon, Dec 16, 2019 at 02:03:43PM +0200, Ville Syrjälä wrote:
> > On Fri, Dec 13, 2019 at 06:28:40PM -0800, Manasi Navare wrote:
> > > On Fri, Dec 13, 2019 at 10:05:49PM +0200, Ville Syrjälä wrote:
> > > > On Wed, Dec 11, 2019 at 01:14:23PM -0800, Manasi Navare wrote:
> > > > > In case of tiled displays, all the tiles are linke dto each other
> > > > > for transcoder port sync. So in intel_atomic_check() we need to make
> > > > > sure that we add all the tiles to the modeset and if one of the
> > > > > tiles needs a full modeset then mark all other tiles for a full modeset.
> > > > >
> > > > > Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > Cc: José Roberto de Souza <jose.souza@intel.com>
> > > > > Bugzilla: https://gitlab.freedesktop.org/drm/intel/issues/5
> > > > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > > > > ---
> > > > > drivers/gpu/drm/i915/display/intel_display.c | 78 ++++++++++++++++++++
> > > > > 1 file changed, 78 insertions(+)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > > > > index 803993a01ca7..7263eaa66cda 100644
> > > > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > > > @@ -14066,6 +14066,80 @@ static int intel_atomic_check_crtcs(struct intel_atomic_state *state)
> > > > > return 0;
> > > > > }
> > > > >
> > > > > +static int
> > > > > +intel_dp_modeset_all_tiles(struct drm_i915_private *dev_priv,
> > > > > + struct intel_atomic_state *state, int tile_grp_id)
> > > > > +{
> > > > > + struct drm_connector *conn_iter;
> > > > 'connector'
> > > > > + struct drm_connector_list_iter conn_list_iter;
> > > > > + struct drm_crtc_state *crtc_state;
> > > >
> > > > crtc_state has needlessly wide scope.
> > > >
> > > > > +
> > > > > + drm_connector_list_iter_begin(&dev_priv->drm, &conn_list_iter);
> > > > > + drm_for_each_connector_iter(conn_iter, &conn_list_iter) {
> > > > > + struct drm_connector_state *conn_iter_state;
> > > >
> > > > 'conn_state' is the most popular name.
> > > >
> > > > > +
> > > > > + if (!conn_iter->has_tile)
> > > > > + continue;
> > > > > + conn_iter_state = drm_atomic_get_connector_state(&state->base,
> > > > > + conn_iter);
> > > > > + if (IS_ERR(conn_iter_state)) {
> > > > > + drm_connector_list_iter_end(&conn_list_iter);
> > > > > + return PTR_ERR(conn_iter_state);
> > > > > + }
> > > > > +
> > > > > + if (!conn_iter_state->crtc)
> > > > > + continue;
> > > > > +
> > > > > + if (conn_iter->tile_group->id != tile_grp_id)
> > > > > + continue;
> > > >
> > > > The tile group check should be part of the same if with the has_tile
> > > > check.
> > > >
> > > > > +
> > > > > + crtc_state = drm_atomic_get_crtc_state(&state->base, conn_iter_state->crtc);
> > > > > + if (IS_ERR(crtc_state)) {
> > > > > + drm_connector_list_iter_end(&conn_list_iter);
> > > > > + return PTR_ERR(conn_iter_state);
> > > > > + }
> > > > > + crtc_state->mode_changed = true;
> > > > > + }
> > > > > + drm_connector_list_iter_end(&conn_list_iter);
> > > > > +
> > > > > + return 0;
> > > > > +}
> > > > > +
> > > > > +static int
> > > > > +intel_dp_atomic_trans_port_sync_check(struct drm_i915_private *dev_priv,
> > > >
> > > > Pointless variable. Can be extracted from the atomic state.
> > > >
> > > > > + struct intel_atomic_state *state)
> > > > > +{
> > > > > + struct drm_connector *connector;
> > > > > + struct drm_crtc_state *crtc_state;
> > > > > + struct drm_connector_state *connector_state;
> > > > > + int i, ret, tile_grp_id = 0;
> > > >
> > > > tile_grp_id is rather pointless. crtc_state and ret can move into
> > > > tighter scope. And the next suggestion allows you to kill crtc_state
> > > > entirely...
> > >
> > > Its not clear why tile_grp_id is pointless, I am using tile_grp_id for the first connector with has_tile
> > > and I make sure that I dont enter into the loop to check modeset again for the connector with
> > > same tile_grp_id because we have already set its mode changed to true in intel_dp_modeset_all_tiles()
> > >
> > > How can I achieve this instead?
> >
> >
> > Instead of
> > foo()
> > {
> > tile_grp_id = 0;
> >
> > for_each() {
> > tile_grp_id = conn->tile_grp_id;
> >
> > intel_dp_modeset_all_tiles(tile_grp_id);
> > }
> > }
> >
> > you just do
> > foo()
> > {
> > for_each() {
> > intel_dp_modeset_all_tiles(conn->tile_grp_id);
> > }
> > }
>
> Yes I understand that we can pass the conn->tile_grp_id directly. But currently I am using tile_grp_id in for_each(), to call intel_dp_modeset_all_tiles()
> only if that cnnector is from a differnt tile grp id else continue.
>
> foo()
> {
> for_each() {
> if(tile_grp_id != conn->tile_grp_id)
> intel_dpmodeset_all_tiles();
> else
> continue;
> }
> }
>
> calling intel_dp_modeset_all_tiles() for all tiles is kind of redundant. How can I do this with your suggestion, or do you think
> we should call intel_dp_modeset_all_tiles for each connector anyway and it shouldnt matter?
Yes if that connector is tiled.
Eg. if you have something like
connector A: tile group 1
connector B: tile group 2
connector C: tile group 3
connector D: tile group 2
then your single tile_grp_id variable doesn't work anyway. You'd need
track which tile groups you've already handled and skip those. But
much easier to just blindly plow ahead.
--
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [Intel-gfx] [PATCH 1/3] drm/i915/dp: Make sure all tiled connectors get added to the state with full modeset
2019-12-16 14:37 ` Ville Syrjälä
@ 2019-12-16 19:13 ` Manasi Navare
2019-12-16 21:37 ` Ville Syrjälä
0 siblings, 1 reply; 35+ messages in thread
From: Manasi Navare @ 2019-12-16 19:13 UTC (permalink / raw)
To: Ville Syrjälä; +Cc: intel-gfx
On Mon, Dec 16, 2019 at 04:37:38PM +0200, Ville Syrjälä wrote:
> On Wed, Dec 11, 2019 at 01:14:23PM -0800, Manasi Navare wrote:
> > In case of tiled displays, all the tiles are linke dto each other
> > for transcoder port sync. So in intel_atomic_check() we need to make
> > sure that we add all the tiles to the modeset and if one of the
> > tiles needs a full modeset then mark all other tiles for a full modeset.
> >
> > Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: José Roberto de Souza <jose.souza@intel.com>
> > Bugzilla: https://gitlab.freedesktop.org/drm/intel/issues/5
> > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > ---
> > drivers/gpu/drm/i915/display/intel_display.c | 78 ++++++++++++++++++++
> > 1 file changed, 78 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > index 803993a01ca7..7263eaa66cda 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -14066,6 +14066,80 @@ static int intel_atomic_check_crtcs(struct intel_atomic_state *state)
> > return 0;
> > }
> >
> > +static int
> > +intel_dp_modeset_all_tiles(struct drm_i915_private *dev_priv,
> > + struct intel_atomic_state *state, int tile_grp_id)
> > +{
> > + struct drm_connector *conn_iter;
> > + struct drm_connector_list_iter conn_list_iter;
> > + struct drm_crtc_state *crtc_state;
> > +
> > + drm_connector_list_iter_begin(&dev_priv->drm, &conn_list_iter);
> > + drm_for_each_connector_iter(conn_iter, &conn_list_iter) {
> > + struct drm_connector_state *conn_iter_state;
> > +
> > + if (!conn_iter->has_tile)
> > + continue;
> > + conn_iter_state = drm_atomic_get_connector_state(&state->base,
> > + conn_iter);
> > + if (IS_ERR(conn_iter_state)) {
> > + drm_connector_list_iter_end(&conn_list_iter);
> > + return PTR_ERR(conn_iter_state);
> > + }
> > +
> > + if (!conn_iter_state->crtc)
> > + continue;
> > +
> > + if (conn_iter->tile_group->id != tile_grp_id)
> > + continue;
> > +
> > + crtc_state = drm_atomic_get_crtc_state(&state->base, conn_iter_state->crtc);
> > + if (IS_ERR(crtc_state)) {
> > + drm_connector_list_iter_end(&conn_list_iter);
> > + return PTR_ERR(conn_iter_state);
> > + }
> > + crtc_state->mode_changed = true;
> > + }
> > + drm_connector_list_iter_end(&conn_list_iter);
> > +
> > + return 0;
> > +}
> > +
> > +static int
> > +intel_dp_atomic_trans_port_sync_check(struct drm_i915_private *dev_priv,
> > + struct intel_atomic_state *state)
> > +{
> > + struct drm_connector *connector;
> > + struct drm_crtc_state *crtc_state;
> > + struct drm_connector_state *connector_state;
> > + int i, ret, tile_grp_id = 0;
> > +
> > + if (INTEL_GEN(dev_priv) < 11)
> > + return 0;
> > +
> > + /* Is tiled, mark all other tiled CRTCs as needing a modeset */
> > + for_each_new_connector_in_state(&state->base, connector, connector_state, i) {
> > + if (!connector->has_tile)
> > + continue;
> > + if (connector_state->crtc &&
> > + tile_grp_id != connector->tile_group->id) {
> > + crtc_state = drm_atomic_get_new_crtc_state(&state->base,
> > + connector_state->crtc);
> > + if (!drm_atomic_crtc_needs_modeset(crtc_state))
> > + continue;
> > +
> > + tile_grp_id = connector->tile_group->id;
> > + } else
> > + continue;
> > +
> > + ret = intel_dp_modeset_all_tiles(dev_priv, state, tile_grp_id);
> > + if (ret)
> > + return ret;
> > + }
> > +
> > + return 0;
> > +}
>
> BTW after some more pondering I don't think this alone is sufficient.
> The tile information may have already disppeared so I believe we also
> need to make sure we mark all currently synced crtcs as needing a
> modeset if any of them need a modeset. And I guess that's pretty much
> the same function we'll need to handle fastset correctly.
>
The crtcs in the current state get synced and master/slave assignments happen in icl_add_sync_mode_crtcs before compute_config call
So are you suggesting basically moving this function after the crtcs are synced and then just setting modeset
to true for all synced crtcs if one of them needs modeset?
And why would the tile information be disappeared?
Manasi
> > +
> > /**
> > * intel_atomic_check - validate state object
> > * @dev: drm device
> > @@ -14093,6 +14167,10 @@ static int intel_atomic_check(struct drm_device *dev,
> > if (ret)
> > goto fail;
> >
> > + ret = intel_dp_atomic_trans_port_sync_check(dev_priv, state);
> > + if (ret)
> > + goto fail;
> > +
> > for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
> > new_crtc_state, i) {
> > if (!needs_modeset(new_crtc_state)) {
> > --
> > 2.19.1
>
> --
> Ville Syrjälä
> Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [Intel-gfx] [PATCH 1/3] drm/i915/dp: Make sure all tiled connectors get added to the state with full modeset
2019-12-16 19:13 ` Manasi Navare
@ 2019-12-16 21:37 ` Ville Syrjälä
2019-12-16 22:33 ` Manasi Navare
0 siblings, 1 reply; 35+ messages in thread
From: Ville Syrjälä @ 2019-12-16 21:37 UTC (permalink / raw)
To: Manasi Navare; +Cc: intel-gfx
On Mon, Dec 16, 2019 at 11:13:09AM -0800, Manasi Navare wrote:
> On Mon, Dec 16, 2019 at 04:37:38PM +0200, Ville Syrjälä wrote:
> > On Wed, Dec 11, 2019 at 01:14:23PM -0800, Manasi Navare wrote:
> > > In case of tiled displays, all the tiles are linke dto each other
> > > for transcoder port sync. So in intel_atomic_check() we need to make
> > > sure that we add all the tiles to the modeset and if one of the
> > > tiles needs a full modeset then mark all other tiles for a full modeset.
> > >
> > > Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Cc: José Roberto de Souza <jose.souza@intel.com>
> > > Bugzilla: https://gitlab.freedesktop.org/drm/intel/issues/5
> > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > > ---
> > > drivers/gpu/drm/i915/display/intel_display.c | 78 ++++++++++++++++++++
> > > 1 file changed, 78 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > > index 803993a01ca7..7263eaa66cda 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > @@ -14066,6 +14066,80 @@ static int intel_atomic_check_crtcs(struct intel_atomic_state *state)
> > > return 0;
> > > }
> > >
> > > +static int
> > > +intel_dp_modeset_all_tiles(struct drm_i915_private *dev_priv,
> > > + struct intel_atomic_state *state, int tile_grp_id)
> > > +{
> > > + struct drm_connector *conn_iter;
> > > + struct drm_connector_list_iter conn_list_iter;
> > > + struct drm_crtc_state *crtc_state;
> > > +
> > > + drm_connector_list_iter_begin(&dev_priv->drm, &conn_list_iter);
> > > + drm_for_each_connector_iter(conn_iter, &conn_list_iter) {
> > > + struct drm_connector_state *conn_iter_state;
> > > +
> > > + if (!conn_iter->has_tile)
> > > + continue;
> > > + conn_iter_state = drm_atomic_get_connector_state(&state->base,
> > > + conn_iter);
> > > + if (IS_ERR(conn_iter_state)) {
> > > + drm_connector_list_iter_end(&conn_list_iter);
> > > + return PTR_ERR(conn_iter_state);
> > > + }
> > > +
> > > + if (!conn_iter_state->crtc)
> > > + continue;
> > > +
> > > + if (conn_iter->tile_group->id != tile_grp_id)
> > > + continue;
> > > +
> > > + crtc_state = drm_atomic_get_crtc_state(&state->base, conn_iter_state->crtc);
> > > + if (IS_ERR(crtc_state)) {
> > > + drm_connector_list_iter_end(&conn_list_iter);
> > > + return PTR_ERR(conn_iter_state);
> > > + }
> > > + crtc_state->mode_changed = true;
> > > + }
> > > + drm_connector_list_iter_end(&conn_list_iter);
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static int
> > > +intel_dp_atomic_trans_port_sync_check(struct drm_i915_private *dev_priv,
> > > + struct intel_atomic_state *state)
> > > +{
> > > + struct drm_connector *connector;
> > > + struct drm_crtc_state *crtc_state;
> > > + struct drm_connector_state *connector_state;
> > > + int i, ret, tile_grp_id = 0;
> > > +
> > > + if (INTEL_GEN(dev_priv) < 11)
> > > + return 0;
> > > +
> > > + /* Is tiled, mark all other tiled CRTCs as needing a modeset */
> > > + for_each_new_connector_in_state(&state->base, connector, connector_state, i) {
> > > + if (!connector->has_tile)
> > > + continue;
> > > + if (connector_state->crtc &&
> > > + tile_grp_id != connector->tile_group->id) {
> > > + crtc_state = drm_atomic_get_new_crtc_state(&state->base,
> > > + connector_state->crtc);
> > > + if (!drm_atomic_crtc_needs_modeset(crtc_state))
> > > + continue;
> > > +
> > > + tile_grp_id = connector->tile_group->id;
> > > + } else
> > > + continue;
> > > +
> > > + ret = intel_dp_modeset_all_tiles(dev_priv, state, tile_grp_id);
> > > + if (ret)
> > > + return ret;
> > > + }
> > > +
> > > + return 0;
> > > +}
> >
> > BTW after some more pondering I don't think this alone is sufficient.
> > The tile information may have already disppeared so I believe we also
> > need to make sure we mark all currently synced crtcs as needing a
> > modeset if any of them need a modeset. And I guess that's pretty much
> > the same function we'll need to handle fastset correctly.
> >
>
> The crtcs in the current state get synced and master/slave assignments happen in icl_add_sync_mode_crtcs before compute_config call
> So are you suggesting basically moving this function after the crtcs are synced and then just setting modeset
> to true for all synced crtcs if one of them needs modeset?
I think it should look something like:
modeset_tiled_things();
modeset_synced_things();
for_each() {
modeset_pipes_config();
if (can_fastset()) {
modeset=false;
fastset=true;
}
}
modeset_synced_things();
for_each() {
if (!modeset && fastset)
copy_state();
}
>
> And why would the tile information be disappeared?
It'll get updated whenever someone does a getconnector() or whatever.
Example:
1. sync pipe A and B, pipe A is master
2. swap pipe B display for something else
3. getconnector() -> tile info goes poof
4. do something on pipe A that needs a modeset
no tile info so we miss that pipe B also needs a modeset
--
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [Intel-gfx] [PATCH 1/3] drm/i915/dp: Make sure all tiled connectors get added to the state with full modeset
2019-12-16 17:11 ` Ville Syrjälä
@ 2019-12-16 21:42 ` Manasi Navare
0 siblings, 0 replies; 35+ messages in thread
From: Manasi Navare @ 2019-12-16 21:42 UTC (permalink / raw)
To: Ville Syrjälä; +Cc: intel-gfx
On Mon, Dec 16, 2019 at 07:11:20PM +0200, Ville Syrjälä wrote:
> On Mon, Dec 16, 2019 at 08:40:24AM -0800, Manasi Navare wrote:
> > On Mon, Dec 16, 2019 at 02:03:43PM +0200, Ville Syrjälä wrote:
> > > On Fri, Dec 13, 2019 at 06:28:40PM -0800, Manasi Navare wrote:
> > > > On Fri, Dec 13, 2019 at 10:05:49PM +0200, Ville Syrjälä wrote:
> > > > > On Wed, Dec 11, 2019 at 01:14:23PM -0800, Manasi Navare wrote:
> > > > > > In case of tiled displays, all the tiles are linke dto each other
> > > > > > for transcoder port sync. So in intel_atomic_check() we need to make
> > > > > > sure that we add all the tiles to the modeset and if one of the
> > > > > > tiles needs a full modeset then mark all other tiles for a full modeset.
> > > > > >
> > > > > > Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > > Cc: José Roberto de Souza <jose.souza@intel.com>
> > > > > > Bugzilla: https://gitlab.freedesktop.org/drm/intel/issues/5
> > > > > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > > > > > ---
> > > > > > drivers/gpu/drm/i915/display/intel_display.c | 78 ++++++++++++++++++++
> > > > > > 1 file changed, 78 insertions(+)
> > > > > >
> > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > > > > > index 803993a01ca7..7263eaa66cda 100644
> > > > > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > > > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > > > > @@ -14066,6 +14066,80 @@ static int intel_atomic_check_crtcs(struct intel_atomic_state *state)
> > > > > > return 0;
> > > > > > }
> > > > > >
> > > > > > +static int
> > > > > > +intel_dp_modeset_all_tiles(struct drm_i915_private *dev_priv,
> > > > > > + struct intel_atomic_state *state, int tile_grp_id)
> > > > > > +{
> > > > > > + struct drm_connector *conn_iter;
> > > > > 'connector'
> > > > > > + struct drm_connector_list_iter conn_list_iter;
> > > > > > + struct drm_crtc_state *crtc_state;
> > > > >
> > > > > crtc_state has needlessly wide scope.
> > > > >
> > > > > > +
> > > > > > + drm_connector_list_iter_begin(&dev_priv->drm, &conn_list_iter);
> > > > > > + drm_for_each_connector_iter(conn_iter, &conn_list_iter) {
> > > > > > + struct drm_connector_state *conn_iter_state;
> > > > >
> > > > > 'conn_state' is the most popular name.
> > > > >
> > > > > > +
> > > > > > + if (!conn_iter->has_tile)
> > > > > > + continue;
> > > > > > + conn_iter_state = drm_atomic_get_connector_state(&state->base,
> > > > > > + conn_iter);
> > > > > > + if (IS_ERR(conn_iter_state)) {
> > > > > > + drm_connector_list_iter_end(&conn_list_iter);
> > > > > > + return PTR_ERR(conn_iter_state);
> > > > > > + }
> > > > > > +
> > > > > > + if (!conn_iter_state->crtc)
> > > > > > + continue;
> > > > > > +
> > > > > > + if (conn_iter->tile_group->id != tile_grp_id)
> > > > > > + continue;
> > > > >
> > > > > The tile group check should be part of the same if with the has_tile
> > > > > check.
> > > > >
> > > > > > +
> > > > > > + crtc_state = drm_atomic_get_crtc_state(&state->base, conn_iter_state->crtc);
> > > > > > + if (IS_ERR(crtc_state)) {
> > > > > > + drm_connector_list_iter_end(&conn_list_iter);
> > > > > > + return PTR_ERR(conn_iter_state);
> > > > > > + }
> > > > > > + crtc_state->mode_changed = true;
> > > > > > + }
> > > > > > + drm_connector_list_iter_end(&conn_list_iter);
> > > > > > +
> > > > > > + return 0;
> > > > > > +}
> > > > > > +
> > > > > > +static int
> > > > > > +intel_dp_atomic_trans_port_sync_check(struct drm_i915_private *dev_priv,
> > > > >
> > > > > Pointless variable. Can be extracted from the atomic state.
> > > > >
> > > > > > + struct intel_atomic_state *state)
> > > > > > +{
> > > > > > + struct drm_connector *connector;
> > > > > > + struct drm_crtc_state *crtc_state;
> > > > > > + struct drm_connector_state *connector_state;
> > > > > > + int i, ret, tile_grp_id = 0;
> > > > >
> > > > > tile_grp_id is rather pointless. crtc_state and ret can move into
> > > > > tighter scope. And the next suggestion allows you to kill crtc_state
> > > > > entirely...
> > > >
> > > > Its not clear why tile_grp_id is pointless, I am using tile_grp_id for the first connector with has_tile
> > > > and I make sure that I dont enter into the loop to check modeset again for the connector with
> > > > same tile_grp_id because we have already set its mode changed to true in intel_dp_modeset_all_tiles()
> > > >
> > > > How can I achieve this instead?
> > >
> > >
> > > Instead of
> > > foo()
> > > {
> > > tile_grp_id = 0;
> > >
> > > for_each() {
> > > tile_grp_id = conn->tile_grp_id;
> > >
> > > intel_dp_modeset_all_tiles(tile_grp_id);
> > > }
> > > }
> > >
> > > you just do
> > > foo()
> > > {
> > > for_each() {
> > > intel_dp_modeset_all_tiles(conn->tile_grp_id);
> > > }
> > > }
> >
> > Yes I understand that we can pass the conn->tile_grp_id directly. But currently I am using tile_grp_id in for_each(), to call intel_dp_modeset_all_tiles()
> > only if that cnnector is from a differnt tile grp id else continue.
> >
> > foo()
> > {
> > for_each() {
> > if(tile_grp_id != conn->tile_grp_id)
> > intel_dpmodeset_all_tiles();
> > else
> > continue;
> > }
> > }
> >
> > calling intel_dp_modeset_all_tiles() for all tiles is kind of redundant. How can I do this with your suggestion, or do you think
> > we should call intel_dp_modeset_all_tiles for each connector anyway and it shouldnt matter?
>
> Yes if that connector is tiled.
>
> Eg. if you have something like
> connector A: tile group 1
> connector B: tile group 2
> connector C: tile group 3
> connector D: tile group 2
>
> then your single tile_grp_id variable doesn't work anyway. You'd need
> track which tile groups you've already handled and skip those. But
> much easier to just blindly plow ahead.
Okay so in our case :
Conn A: tile gro 1
Conn B: tile grp 1
Now foo {
for_each_conn {
//connA
intel_modeset_all_tiles() // this will set mode to true for conn B in iteration 1
}
}
Now in the second iteration
for_each_conn {
// conn B iter 2
intel_modeset_all_tiles() // this will set mode to true for conn A
}
redundant second iteration since both are in same tile grp
Manasi
>
> --
> Ville Syrjälä
> Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [Intel-gfx] [PATCH 1/3] drm/i915/dp: Make sure all tiled connectors get added to the state with full modeset
2019-12-16 21:37 ` Ville Syrjälä
@ 2019-12-16 22:33 ` Manasi Navare
2019-12-16 22:58 ` Manasi Navare
0 siblings, 1 reply; 35+ messages in thread
From: Manasi Navare @ 2019-12-16 22:33 UTC (permalink / raw)
To: Ville Syrjälä; +Cc: intel-gfx
On Mon, Dec 16, 2019 at 11:37:38PM +0200, Ville Syrjälä wrote:
> On Mon, Dec 16, 2019 at 11:13:09AM -0800, Manasi Navare wrote:
> > On Mon, Dec 16, 2019 at 04:37:38PM +0200, Ville Syrjälä wrote:
> > > On Wed, Dec 11, 2019 at 01:14:23PM -0800, Manasi Navare wrote:
> > > > In case of tiled displays, all the tiles are linke dto each other
> > > > for transcoder port sync. So in intel_atomic_check() we need to make
> > > > sure that we add all the tiles to the modeset and if one of the
> > > > tiles needs a full modeset then mark all other tiles for a full modeset.
> > > >
> > > > Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > Cc: José Roberto de Souza <jose.souza@intel.com>
> > > > Bugzilla: https://gitlab.freedesktop.org/drm/intel/issues/5
> > > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > > > ---
> > > > drivers/gpu/drm/i915/display/intel_display.c | 78 ++++++++++++++++++++
> > > > 1 file changed, 78 insertions(+)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > > > index 803993a01ca7..7263eaa66cda 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > > @@ -14066,6 +14066,80 @@ static int intel_atomic_check_crtcs(struct intel_atomic_state *state)
> > > > return 0;
> > > > }
> > > >
> > > > +static int
> > > > +intel_dp_modeset_all_tiles(struct drm_i915_private *dev_priv,
> > > > + struct intel_atomic_state *state, int tile_grp_id)
> > > > +{
> > > > + struct drm_connector *conn_iter;
> > > > + struct drm_connector_list_iter conn_list_iter;
> > > > + struct drm_crtc_state *crtc_state;
> > > > +
> > > > + drm_connector_list_iter_begin(&dev_priv->drm, &conn_list_iter);
> > > > + drm_for_each_connector_iter(conn_iter, &conn_list_iter) {
> > > > + struct drm_connector_state *conn_iter_state;
> > > > +
> > > > + if (!conn_iter->has_tile)
> > > > + continue;
> > > > + conn_iter_state = drm_atomic_get_connector_state(&state->base,
> > > > + conn_iter);
> > > > + if (IS_ERR(conn_iter_state)) {
> > > > + drm_connector_list_iter_end(&conn_list_iter);
> > > > + return PTR_ERR(conn_iter_state);
> > > > + }
> > > > +
> > > > + if (!conn_iter_state->crtc)
> > > > + continue;
> > > > +
> > > > + if (conn_iter->tile_group->id != tile_grp_id)
> > > > + continue;
> > > > +
> > > > + crtc_state = drm_atomic_get_crtc_state(&state->base, conn_iter_state->crtc);
> > > > + if (IS_ERR(crtc_state)) {
> > > > + drm_connector_list_iter_end(&conn_list_iter);
> > > > + return PTR_ERR(conn_iter_state);
> > > > + }
> > > > + crtc_state->mode_changed = true;
> > > > + }
> > > > + drm_connector_list_iter_end(&conn_list_iter);
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > +static int
> > > > +intel_dp_atomic_trans_port_sync_check(struct drm_i915_private *dev_priv,
> > > > + struct intel_atomic_state *state)
> > > > +{
> > > > + struct drm_connector *connector;
> > > > + struct drm_crtc_state *crtc_state;
> > > > + struct drm_connector_state *connector_state;
> > > > + int i, ret, tile_grp_id = 0;
> > > > +
> > > > + if (INTEL_GEN(dev_priv) < 11)
> > > > + return 0;
> > > > +
> > > > + /* Is tiled, mark all other tiled CRTCs as needing a modeset */
> > > > + for_each_new_connector_in_state(&state->base, connector, connector_state, i) {
> > > > + if (!connector->has_tile)
> > > > + continue;
> > > > + if (connector_state->crtc &&
> > > > + tile_grp_id != connector->tile_group->id) {
> > > > + crtc_state = drm_atomic_get_new_crtc_state(&state->base,
> > > > + connector_state->crtc);
> > > > + if (!drm_atomic_crtc_needs_modeset(crtc_state))
> > > > + continue;
> > > > +
> > > > + tile_grp_id = connector->tile_group->id;
> > > > + } else
> > > > + continue;
> > > > +
> > > > + ret = intel_dp_modeset_all_tiles(dev_priv, state, tile_grp_id);
> > > > + if (ret)
> > > > + return ret;
> > > > + }
> > > > +
> > > > + return 0;
> > > > +}
> > >
> > > BTW after some more pondering I don't think this alone is sufficient.
> > > The tile information may have already disppeared so I believe we also
> > > need to make sure we mark all currently synced crtcs as needing a
> > > modeset if any of them need a modeset. And I guess that's pretty much
> > > the same function we'll need to handle fastset correctly.
> > >
> >
> > The crtcs in the current state get synced and master/slave assignments happen in icl_add_sync_mode_crtcs before compute_config call
> > So are you suggesting basically moving this function after the crtcs are synced and then just setting modeset
> > to true for all synced crtcs if one of them needs modeset?
>
> I think it should look something like:
>
> modeset_tiled_things();
> modeset_synced_things();
but modeset_synced_things() should be called after icl_add_sync_crtcs() which as per your review should be called
from within modeset_pipe_config just before compute_config() call.
>
> for_each() {
This is the for_each_crtc loop in intel_atomic_check() right?
> modeset_pipes_config();
So have icl_add_sync_crtcs outside of modeset_pipe_config()?
> if (can_fastset()) {
> modeset=false;
> fastset=true;
> }
> }
>
> modeset_synced_things();
>
> for_each() {
> if (!modeset && fastset)
> copy_state();
> }
We already do this in the code right?
Manasi
>
> >
> > And why would the tile information be disappeared?
>
> It'll get updated whenever someone does a getconnector() or whatever.
>
> Example:
> 1. sync pipe A and B, pipe A is master
> 2. swap pipe B display for something else
> 3. getconnector() -> tile info goes poof
> 4. do something on pipe A that needs a modeset
> no tile info so we miss that pipe B also needs a modeset
>
> --
> Ville Syrjälä
> Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [Intel-gfx] [PATCH 1/3] drm/i915/dp: Make sure all tiled connectors get added to the state with full modeset
2019-12-16 22:33 ` Manasi Navare
@ 2019-12-16 22:58 ` Manasi Navare
2019-12-17 10:50 ` Ville Syrjälä
0 siblings, 1 reply; 35+ messages in thread
From: Manasi Navare @ 2019-12-16 22:58 UTC (permalink / raw)
To: Ville Syrjälä; +Cc: intel-gfx
On Mon, Dec 16, 2019 at 02:33:10PM -0800, Manasi Navare wrote:
> On Mon, Dec 16, 2019 at 11:37:38PM +0200, Ville Syrjälä wrote:
> > On Mon, Dec 16, 2019 at 11:13:09AM -0800, Manasi Navare wrote:
> > > On Mon, Dec 16, 2019 at 04:37:38PM +0200, Ville Syrjälä wrote:
> > > > On Wed, Dec 11, 2019 at 01:14:23PM -0800, Manasi Navare wrote:
> > > > > In case of tiled displays, all the tiles are linke dto each other
> > > > > for transcoder port sync. So in intel_atomic_check() we need to make
> > > > > sure that we add all the tiles to the modeset and if one of the
> > > > > tiles needs a full modeset then mark all other tiles for a full modeset.
> > > > >
> > > > > Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > Cc: José Roberto de Souza <jose.souza@intel.com>
> > > > > Bugzilla: https://gitlab.freedesktop.org/drm/intel/issues/5
> > > > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > > > > ---
> > > > > drivers/gpu/drm/i915/display/intel_display.c | 78 ++++++++++++++++++++
> > > > > 1 file changed, 78 insertions(+)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > > > > index 803993a01ca7..7263eaa66cda 100644
> > > > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > > > @@ -14066,6 +14066,80 @@ static int intel_atomic_check_crtcs(struct intel_atomic_state *state)
> > > > > return 0;
> > > > > }
> > > > >
> > > > > +static int
> > > > > +intel_dp_modeset_all_tiles(struct drm_i915_private *dev_priv,
> > > > > + struct intel_atomic_state *state, int tile_grp_id)
> > > > > +{
> > > > > + struct drm_connector *conn_iter;
> > > > > + struct drm_connector_list_iter conn_list_iter;
> > > > > + struct drm_crtc_state *crtc_state;
> > > > > +
> > > > > + drm_connector_list_iter_begin(&dev_priv->drm, &conn_list_iter);
> > > > > + drm_for_each_connector_iter(conn_iter, &conn_list_iter) {
> > > > > + struct drm_connector_state *conn_iter_state;
> > > > > +
> > > > > + if (!conn_iter->has_tile)
> > > > > + continue;
> > > > > + conn_iter_state = drm_atomic_get_connector_state(&state->base,
> > > > > + conn_iter);
> > > > > + if (IS_ERR(conn_iter_state)) {
> > > > > + drm_connector_list_iter_end(&conn_list_iter);
> > > > > + return PTR_ERR(conn_iter_state);
> > > > > + }
> > > > > +
> > > > > + if (!conn_iter_state->crtc)
> > > > > + continue;
> > > > > +
> > > > > + if (conn_iter->tile_group->id != tile_grp_id)
> > > > > + continue;
> > > > > +
> > > > > + crtc_state = drm_atomic_get_crtc_state(&state->base, conn_iter_state->crtc);
> > > > > + if (IS_ERR(crtc_state)) {
> > > > > + drm_connector_list_iter_end(&conn_list_iter);
> > > > > + return PTR_ERR(conn_iter_state);
> > > > > + }
> > > > > + crtc_state->mode_changed = true;
> > > > > + }
> > > > > + drm_connector_list_iter_end(&conn_list_iter);
> > > > > +
> > > > > + return 0;
> > > > > +}
> > > > > +
> > > > > +static int
> > > > > +intel_dp_atomic_trans_port_sync_check(struct drm_i915_private *dev_priv,
> > > > > + struct intel_atomic_state *state)
> > > > > +{
> > > > > + struct drm_connector *connector;
> > > > > + struct drm_crtc_state *crtc_state;
> > > > > + struct drm_connector_state *connector_state;
> > > > > + int i, ret, tile_grp_id = 0;
> > > > > +
> > > > > + if (INTEL_GEN(dev_priv) < 11)
> > > > > + return 0;
> > > > > +
> > > > > + /* Is tiled, mark all other tiled CRTCs as needing a modeset */
> > > > > + for_each_new_connector_in_state(&state->base, connector, connector_state, i) {
> > > > > + if (!connector->has_tile)
> > > > > + continue;
> > > > > + if (connector_state->crtc &&
> > > > > + tile_grp_id != connector->tile_group->id) {
> > > > > + crtc_state = drm_atomic_get_new_crtc_state(&state->base,
> > > > > + connector_state->crtc);
> > > > > + if (!drm_atomic_crtc_needs_modeset(crtc_state))
> > > > > + continue;
> > > > > +
> > > > > + tile_grp_id = connector->tile_group->id;
> > > > > + } else
> > > > > + continue;
> > > > > +
> > > > > + ret = intel_dp_modeset_all_tiles(dev_priv, state, tile_grp_id);
> > > > > + if (ret)
> > > > > + return ret;
> > > > > + }
> > > > > +
> > > > > + return 0;
> > > > > +}
> > > >
> > > > BTW after some more pondering I don't think this alone is sufficient.
> > > > The tile information may have already disppeared so I believe we also
> > > > need to make sure we mark all currently synced crtcs as needing a
> > > > modeset if any of them need a modeset. And I guess that's pretty much
> > > > the same function we'll need to handle fastset correctly.
> > > >
> > >
> > > The crtcs in the current state get synced and master/slave assignments happen in icl_add_sync_mode_crtcs before compute_config call
> > > So are you suggesting basically moving this function after the crtcs are synced and then just setting modeset
> > > to true for all synced crtcs if one of them needs modeset?
> >
> > I think it should look something like:
> >
> > modeset_tiled_things();
> > modeset_synced_things();
>
> but modeset_synced_things() should be called after icl_add_sync_crtcs() which as per your review should be called
> from within modeset_pipe_config just before compute_config() call.
>
> >
> > for_each() {
>
> This is the for_each_crtc loop in intel_atomic_check() right?
>
> > modeset_pipes_config();
>
> So have icl_add_sync_crtcs outside of modeset_pipe_config()?
>
> > if (can_fastset()) {
> > modeset=false;
> > fastset=true;
> > }
> > }
> >
> > modeset_synced_things();
> >
> > for_each() {
> > if (!modeset && fastset)
> > copy_state();
> > }
> We already do this in the code right?
>
> Manasi
>
> >
> > >
> > > And why would the tile information be disappeared?
> >
> > It'll get updated whenever someone does a getconnector() or whatever.
> >
> > Example:
> > 1. sync pipe A and B, pipe A is master
> > 2. swap pipe B display for something else
If we disconnect and connect other display for pipe B, port sync mode is off and
Pipe A no longer a master and we would reset the master_slave assignments and conn on pipe B would not have tile
so why we wold need to add both pipes to modeset in this case at all
Manasi
> > 3. getconnector() -> tile info goes poof
> > 4. do something on pipe A that needs a modeset
> > no tile info so we miss that pipe B also needs a modeset
> >
> > --
> > Ville Syrjälä
> > Intel
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [Intel-gfx] [PATCH 1/3] drm/i915/dp: Make sure all tiled connectors get added to the state with full modeset
2019-12-16 22:58 ` Manasi Navare
@ 2019-12-17 10:50 ` Ville Syrjälä
2019-12-17 19:04 ` Manasi Navare
0 siblings, 1 reply; 35+ messages in thread
From: Ville Syrjälä @ 2019-12-17 10:50 UTC (permalink / raw)
To: Manasi Navare; +Cc: intel-gfx
On Mon, Dec 16, 2019 at 02:58:13PM -0800, Manasi Navare wrote:
> On Mon, Dec 16, 2019 at 02:33:10PM -0800, Manasi Navare wrote:
> > On Mon, Dec 16, 2019 at 11:37:38PM +0200, Ville Syrjälä wrote:
> > > On Mon, Dec 16, 2019 at 11:13:09AM -0800, Manasi Navare wrote:
> > > > On Mon, Dec 16, 2019 at 04:37:38PM +0200, Ville Syrjälä wrote:
> > > > > On Wed, Dec 11, 2019 at 01:14:23PM -0800, Manasi Navare wrote:
> > > > > > In case of tiled displays, all the tiles are linke dto each other
> > > > > > for transcoder port sync. So in intel_atomic_check() we need to make
> > > > > > sure that we add all the tiles to the modeset and if one of the
> > > > > > tiles needs a full modeset then mark all other tiles for a full modeset.
> > > > > >
> > > > > > Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > > Cc: José Roberto de Souza <jose.souza@intel.com>
> > > > > > Bugzilla: https://gitlab.freedesktop.org/drm/intel/issues/5
> > > > > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > > > > > ---
> > > > > > drivers/gpu/drm/i915/display/intel_display.c | 78 ++++++++++++++++++++
> > > > > > 1 file changed, 78 insertions(+)
> > > > > >
> > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > > > > > index 803993a01ca7..7263eaa66cda 100644
> > > > > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > > > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > > > > @@ -14066,6 +14066,80 @@ static int intel_atomic_check_crtcs(struct intel_atomic_state *state)
> > > > > > return 0;
> > > > > > }
> > > > > >
> > > > > > +static int
> > > > > > +intel_dp_modeset_all_tiles(struct drm_i915_private *dev_priv,
> > > > > > + struct intel_atomic_state *state, int tile_grp_id)
> > > > > > +{
> > > > > > + struct drm_connector *conn_iter;
> > > > > > + struct drm_connector_list_iter conn_list_iter;
> > > > > > + struct drm_crtc_state *crtc_state;
> > > > > > +
> > > > > > + drm_connector_list_iter_begin(&dev_priv->drm, &conn_list_iter);
> > > > > > + drm_for_each_connector_iter(conn_iter, &conn_list_iter) {
> > > > > > + struct drm_connector_state *conn_iter_state;
> > > > > > +
> > > > > > + if (!conn_iter->has_tile)
> > > > > > + continue;
> > > > > > + conn_iter_state = drm_atomic_get_connector_state(&state->base,
> > > > > > + conn_iter);
> > > > > > + if (IS_ERR(conn_iter_state)) {
> > > > > > + drm_connector_list_iter_end(&conn_list_iter);
> > > > > > + return PTR_ERR(conn_iter_state);
> > > > > > + }
> > > > > > +
> > > > > > + if (!conn_iter_state->crtc)
> > > > > > + continue;
> > > > > > +
> > > > > > + if (conn_iter->tile_group->id != tile_grp_id)
> > > > > > + continue;
> > > > > > +
> > > > > > + crtc_state = drm_atomic_get_crtc_state(&state->base, conn_iter_state->crtc);
> > > > > > + if (IS_ERR(crtc_state)) {
> > > > > > + drm_connector_list_iter_end(&conn_list_iter);
> > > > > > + return PTR_ERR(conn_iter_state);
> > > > > > + }
> > > > > > + crtc_state->mode_changed = true;
> > > > > > + }
> > > > > > + drm_connector_list_iter_end(&conn_list_iter);
> > > > > > +
> > > > > > + return 0;
> > > > > > +}
> > > > > > +
> > > > > > +static int
> > > > > > +intel_dp_atomic_trans_port_sync_check(struct drm_i915_private *dev_priv,
> > > > > > + struct intel_atomic_state *state)
> > > > > > +{
> > > > > > + struct drm_connector *connector;
> > > > > > + struct drm_crtc_state *crtc_state;
> > > > > > + struct drm_connector_state *connector_state;
> > > > > > + int i, ret, tile_grp_id = 0;
> > > > > > +
> > > > > > + if (INTEL_GEN(dev_priv) < 11)
> > > > > > + return 0;
> > > > > > +
> > > > > > + /* Is tiled, mark all other tiled CRTCs as needing a modeset */
> > > > > > + for_each_new_connector_in_state(&state->base, connector, connector_state, i) {
> > > > > > + if (!connector->has_tile)
> > > > > > + continue;
> > > > > > + if (connector_state->crtc &&
> > > > > > + tile_grp_id != connector->tile_group->id) {
> > > > > > + crtc_state = drm_atomic_get_new_crtc_state(&state->base,
> > > > > > + connector_state->crtc);
> > > > > > + if (!drm_atomic_crtc_needs_modeset(crtc_state))
> > > > > > + continue;
> > > > > > +
> > > > > > + tile_grp_id = connector->tile_group->id;
> > > > > > + } else
> > > > > > + continue;
> > > > > > +
> > > > > > + ret = intel_dp_modeset_all_tiles(dev_priv, state, tile_grp_id);
> > > > > > + if (ret)
> > > > > > + return ret;
> > > > > > + }
> > > > > > +
> > > > > > + return 0;
> > > > > > +}
> > > > >
> > > > > BTW after some more pondering I don't think this alone is sufficient.
> > > > > The tile information may have already disppeared so I believe we also
> > > > > need to make sure we mark all currently synced crtcs as needing a
> > > > > modeset if any of them need a modeset. And I guess that's pretty much
> > > > > the same function we'll need to handle fastset correctly.
> > > > >
> > > >
> > > > The crtcs in the current state get synced and master/slave assignments happen in icl_add_sync_mode_crtcs before compute_config call
> > > > So are you suggesting basically moving this function after the crtcs are synced and then just setting modeset
> > > > to true for all synced crtcs if one of them needs modeset?
> > >
> > > I think it should look something like:
> > >
> > > modeset_tiled_things();
> > > modeset_synced_things();
> >
> > but modeset_synced_things() should be called after icl_add_sync_crtcs() which as per your review should be called
> > from within modeset_pipe_config just before compute_config() call.
> >
> > >
> > > for_each() {
> >
> > This is the for_each_crtc loop in intel_atomic_check() right?
> >
> > > modeset_pipes_config();
> >
> > So have icl_add_sync_crtcs outside of modeset_pipe_config()?
> >
> > > if (can_fastset()) {
> > > modeset=false;
> > > fastset=true;
> > > }
> > > }
> > >
> > > modeset_synced_things();
> > >
> > > for_each() {
> > > if (!modeset && fastset)
> > > copy_state();
> > > }
> > We already do this in the code right?
> >
> > Manasi
> >
> > >
> > > >
> > > > And why would the tile information be disappeared?
> > >
> > > It'll get updated whenever someone does a getconnector() or whatever.
> > >
> > > Example:
> > > 1. sync pipe A and B, pipe A is master
> > > 2. swap pipe B display for something else
>
> If we disconnect and connect other display for pipe B, port sync mode is off and
> Pipe A no longer a master and we would reset the master_slave assignments and conn on pipe B would not have tile
Port sync will stay enabled until we do a modeset to disable it. In this
example there is never any modeset on pipe B until we add soemthing to
force one in step 4.
>
> so why we wold need to add both pipes to modeset in this case at all
>
> Manasi
>
> > > 3. getconnector() -> tile info goes poof
> > > 4. do something on pipe A that needs a modeset
> > > no tile info so we miss that pipe B also needs a modeset
> > >
> > > --
> > > Ville Syrjälä
> > > Intel
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [Intel-gfx] [PATCH 1/3] drm/i915/dp: Make sure all tiled connectors get added to the state with full modeset
2019-12-17 10:50 ` Ville Syrjälä
@ 2019-12-17 19:04 ` Manasi Navare
2019-12-19 2:37 ` Manasi Navare
0 siblings, 1 reply; 35+ messages in thread
From: Manasi Navare @ 2019-12-17 19:04 UTC (permalink / raw)
To: Ville Syrjälä; +Cc: intel-gfx
On Tue, Dec 17, 2019 at 12:50:31PM +0200, Ville Syrjälä wrote:
> On Mon, Dec 16, 2019 at 02:58:13PM -0800, Manasi Navare wrote:
> > On Mon, Dec 16, 2019 at 02:33:10PM -0800, Manasi Navare wrote:
> > > On Mon, Dec 16, 2019 at 11:37:38PM +0200, Ville Syrjälä wrote:
> > > > On Mon, Dec 16, 2019 at 11:13:09AM -0800, Manasi Navare wrote:
> > > > > On Mon, Dec 16, 2019 at 04:37:38PM +0200, Ville Syrjälä wrote:
> > > > > > On Wed, Dec 11, 2019 at 01:14:23PM -0800, Manasi Navare wrote:
> > > > > > > In case of tiled displays, all the tiles are linke dto each other
> > > > > > > for transcoder port sync. So in intel_atomic_check() we need to make
> > > > > > > sure that we add all the tiles to the modeset and if one of the
> > > > > > > tiles needs a full modeset then mark all other tiles for a full modeset.
> > > > > > >
> > > > > > > Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > > > Cc: José Roberto de Souza <jose.souza@intel.com>
> > > > > > > Bugzilla: https://gitlab.freedesktop.org/drm/intel/issues/5
> > > > > > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > > > > > > ---
> > > > > > > drivers/gpu/drm/i915/display/intel_display.c | 78 ++++++++++++++++++++
> > > > > > > 1 file changed, 78 insertions(+)
> > > > > > >
> > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > > > > > > index 803993a01ca7..7263eaa66cda 100644
> > > > > > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > > > > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > > > > > @@ -14066,6 +14066,80 @@ static int intel_atomic_check_crtcs(struct intel_atomic_state *state)
> > > > > > > return 0;
> > > > > > > }
> > > > > > >
> > > > > > > +static int
> > > > > > > +intel_dp_modeset_all_tiles(struct drm_i915_private *dev_priv,
> > > > > > > + struct intel_atomic_state *state, int tile_grp_id)
> > > > > > > +{
> > > > > > > + struct drm_connector *conn_iter;
> > > > > > > + struct drm_connector_list_iter conn_list_iter;
> > > > > > > + struct drm_crtc_state *crtc_state;
> > > > > > > +
> > > > > > > + drm_connector_list_iter_begin(&dev_priv->drm, &conn_list_iter);
> > > > > > > + drm_for_each_connector_iter(conn_iter, &conn_list_iter) {
> > > > > > > + struct drm_connector_state *conn_iter_state;
> > > > > > > +
> > > > > > > + if (!conn_iter->has_tile)
> > > > > > > + continue;
> > > > > > > + conn_iter_state = drm_atomic_get_connector_state(&state->base,
> > > > > > > + conn_iter);
> > > > > > > + if (IS_ERR(conn_iter_state)) {
> > > > > > > + drm_connector_list_iter_end(&conn_list_iter);
> > > > > > > + return PTR_ERR(conn_iter_state);
> > > > > > > + }
> > > > > > > +
> > > > > > > + if (!conn_iter_state->crtc)
> > > > > > > + continue;
> > > > > > > +
> > > > > > > + if (conn_iter->tile_group->id != tile_grp_id)
> > > > > > > + continue;
> > > > > > > +
> > > > > > > + crtc_state = drm_atomic_get_crtc_state(&state->base, conn_iter_state->crtc);
> > > > > > > + if (IS_ERR(crtc_state)) {
> > > > > > > + drm_connector_list_iter_end(&conn_list_iter);
> > > > > > > + return PTR_ERR(conn_iter_state);
> > > > > > > + }
> > > > > > > + crtc_state->mode_changed = true;
> > > > > > > + }
> > > > > > > + drm_connector_list_iter_end(&conn_list_iter);
> > > > > > > +
> > > > > > > + return 0;
> > > > > > > +}
> > > > > > > +
> > > > > > > +static int
> > > > > > > +intel_dp_atomic_trans_port_sync_check(struct drm_i915_private *dev_priv,
> > > > > > > + struct intel_atomic_state *state)
> > > > > > > +{
> > > > > > > + struct drm_connector *connector;
> > > > > > > + struct drm_crtc_state *crtc_state;
> > > > > > > + struct drm_connector_state *connector_state;
> > > > > > > + int i, ret, tile_grp_id = 0;
> > > > > > > +
> > > > > > > + if (INTEL_GEN(dev_priv) < 11)
> > > > > > > + return 0;
> > > > > > > +
> > > > > > > + /* Is tiled, mark all other tiled CRTCs as needing a modeset */
> > > > > > > + for_each_new_connector_in_state(&state->base, connector, connector_state, i) {
> > > > > > > + if (!connector->has_tile)
> > > > > > > + continue;
> > > > > > > + if (connector_state->crtc &&
> > > > > > > + tile_grp_id != connector->tile_group->id) {
> > > > > > > + crtc_state = drm_atomic_get_new_crtc_state(&state->base,
> > > > > > > + connector_state->crtc);
> > > > > > > + if (!drm_atomic_crtc_needs_modeset(crtc_state))
> > > > > > > + continue;
> > > > > > > +
> > > > > > > + tile_grp_id = connector->tile_group->id;
> > > > > > > + } else
> > > > > > > + continue;
> > > > > > > +
> > > > > > > + ret = intel_dp_modeset_all_tiles(dev_priv, state, tile_grp_id);
> > > > > > > + if (ret)
> > > > > > > + return ret;
> > > > > > > + }
> > > > > > > +
> > > > > > > + return 0;
> > > > > > > +}
> > > > > >
> > > > > > BTW after some more pondering I don't think this alone is sufficient.
> > > > > > The tile information may have already disppeared so I believe we also
> > > > > > need to make sure we mark all currently synced crtcs as needing a
> > > > > > modeset if any of them need a modeset. And I guess that's pretty much
> > > > > > the same function we'll need to handle fastset correctly.
> > > > > >
> > > > >
> > > > > The crtcs in the current state get synced and master/slave assignments happen in icl_add_sync_mode_crtcs before compute_config call
> > > > > So are you suggesting basically moving this function after the crtcs are synced and then just setting modeset
> > > > > to true for all synced crtcs if one of them needs modeset?
> > > >
> > > > I think it should look something like:
> > > >
> > > > modeset_tiled_things();
> > > > modeset_synced_things();
> > >
> > > but modeset_synced_things() should be called after icl_add_sync_crtcs() which as per your review should be called
> > > from within modeset_pipe_config just before compute_config() call.
> > >
> > > >
> > > > for_each() {
> > >
> > > This is the for_each_crtc loop in intel_atomic_check() right?
> > >
> > > > modeset_pipes_config();
> > >
> > > So have icl_add_sync_crtcs outside of modeset_pipe_config()?
> > >
> > > > if (can_fastset()) {
> > > > modeset=false;
> > > > fastset=true;
> > > > }
> > > > }
> > > >
> > > > modeset_synced_things();
> > > >
> > > > for_each() {
> > > > if (!modeset && fastset)
> > > > copy_state();
> > > > }
> > > We already do this in the code right?
> > >
> > > Manasi
> > >
> > > >
> > > > >
> > > > > And why would the tile information be disappeared?
> > > >
> > > > It'll get updated whenever someone does a getconnector() or whatever.
> > > >
> > > > Example:
> > > > 1. sync pipe A and B, pipe A is master
> > > > 2. swap pipe B display for something else
> >
> > If we disconnect and connect other display for pipe B, port sync mode is off and
> > Pipe A no longer a master and we would reset the master_slave assignments and conn on pipe B would not have tile
>
> Port sync will stay enabled until we do a modeset to disable it. In this
> example there is never any modeset on pipe B until we add soemthing to
> force one in step 4.
seems to be working with the current intel_dp_add_modeset_tiles() since the moment
we unplug, it goes through the disable sequence where all the associated tiles get disabled
and port sync mode gets disabled. This happens because the Pipe A which is still connected now indicates
a mode change since it fallsback to a lower non tiled mode.
But to be on safer side, i can check if say Pipe A is a master or slave (in port sync mode) , check its crtc_state which is
still showing the old master slave links since we havent cleared those yet and then add all other synced crtcs
to the modeset?
So the order of calls can still be :
intel_atomic_check() {
intel_dp_atomic_tiled_check() { modeset_all_tiles modeset_synced_crtcs}
intel_modeset_pipe_Config()
icl_add_sync_crtcs
compute_config
fastsetcheck()
modeset_synced_things (here it looks at the new master slave assignments)
Does this look correct, I want send this patch out today
Manasi
>
> >
> > so why we wold need to add both pipes to modeset in this case at all
> >
> > Manasi
> >
> > > > 3. getconnector() -> tile info goes poof
> > > > 4. do something on pipe A that needs a modeset
> > > > no tile info so we miss that pipe B also needs a modeset
> > > >
> > > > --
> > > > Ville Syrjälä
> > > > Intel
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Ville Syrjälä
> Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [Intel-gfx] [PATCH 1/3] drm/i915/dp: Make sure all tiled connectors get added to the state with full modeset
2019-12-17 19:04 ` Manasi Navare
@ 2019-12-19 2:37 ` Manasi Navare
0 siblings, 0 replies; 35+ messages in thread
From: Manasi Navare @ 2019-12-19 2:37 UTC (permalink / raw)
To: Ville Syrjälä; +Cc: intel-gfx
Hi Ville,
I double checke that I can use new_crtc_state in modeset_synced_crtcs to add all synced crtcs to modeset
That is working as expected so that the same function can be reused after fastset check to override
mode_changed to true.
Here's what I have:
static int
intel_dp_modeset_synced_crtcs(struct intel_atomic_state *state)
{
struct drm_i915_private *dev_priv = to_i915(state->base.dev);
struct intel_crtc_state *old_crtc_state, *new_crtc_state;
struct intel_crtc *crtc;
int i;
for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
new_crtc_state, i) {
if(is_trans_port_sync_mode(new_crtc_state))
DRM_DEBUG_KMS("\n Adding CRTC %d:%s to modeset since old crtc state master trans = %s, slave mask = %d, new crtc state master trans = %s slave mask = %d",
crtc->base.base.id,
crtc->base.name,
transcoder_name(old_crtc_state->master_transcoder),
old_crtc_state->sync_mode_slaves_mask,
transcoder_name(new_crtc_state->master_transcoder),
new_crtc_state->sync_mode_slaves_mask);
new_crtc_state->uapi.mode_changed = true;
}
return 0;
}
static int
intel_dp_atomic_check_synced_crtcs(struct intel_atomic_state *state)
{
struct drm_i915_private *dev_priv = to_i915(state->base.dev);
struct intel_crtc_state *old_crtc_state, *new_crtc_state;
struct intel_crtc *crtc;
int i;
for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
new_crtc_state, i) {
if(!is_trans_port_sync_mode(new_crtc_state) ||
!needs_modeset(new_crtc_state))
continue;
intel_dp_modeset_synced_crtcs(state);
}
return 0;
}
I will add this to this patch after thye call to modeset_all_tiles() and submit a new revision.
Regards
Manasi
On Tue, Dec 17, 2019 at 11:04:53AM -0800, Manasi Navare wrote:
> On Tue, Dec 17, 2019 at 12:50:31PM +0200, Ville Syrjälä wrote:
> > On Mon, Dec 16, 2019 at 02:58:13PM -0800, Manasi Navare wrote:
> > > On Mon, Dec 16, 2019 at 02:33:10PM -0800, Manasi Navare wrote:
> > > > On Mon, Dec 16, 2019 at 11:37:38PM +0200, Ville Syrjälä wrote:
> > > > > On Mon, Dec 16, 2019 at 11:13:09AM -0800, Manasi Navare wrote:
> > > > > > On Mon, Dec 16, 2019 at 04:37:38PM +0200, Ville Syrjälä wrote:
> > > > > > > On Wed, Dec 11, 2019 at 01:14:23PM -0800, Manasi Navare wrote:
> > > > > > > > In case of tiled displays, all the tiles are linke dto each other
> > > > > > > > for transcoder port sync. So in intel_atomic_check() we need to make
> > > > > > > > sure that we add all the tiles to the modeset and if one of the
> > > > > > > > tiles needs a full modeset then mark all other tiles for a full modeset.
> > > > > > > >
> > > > > > > > Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > > > > Cc: José Roberto de Souza <jose.souza@intel.com>
> > > > > > > > Bugzilla: https://gitlab.freedesktop.org/drm/intel/issues/5
> > > > > > > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > > > > > > > ---
> > > > > > > > drivers/gpu/drm/i915/display/intel_display.c | 78 ++++++++++++++++++++
> > > > > > > > 1 file changed, 78 insertions(+)
> > > > > > > >
> > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > > > > > > > index 803993a01ca7..7263eaa66cda 100644
> > > > > > > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > > > > > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > > > > > > @@ -14066,6 +14066,80 @@ static int intel_atomic_check_crtcs(struct intel_atomic_state *state)
> > > > > > > > return 0;
> > > > > > > > }
> > > > > > > >
> > > > > > > > +static int
> > > > > > > > +intel_dp_modeset_all_tiles(struct drm_i915_private *dev_priv,
> > > > > > > > + struct intel_atomic_state *state, int tile_grp_id)
> > > > > > > > +{
> > > > > > > > + struct drm_connector *conn_iter;
> > > > > > > > + struct drm_connector_list_iter conn_list_iter;
> > > > > > > > + struct drm_crtc_state *crtc_state;
> > > > > > > > +
> > > > > > > > + drm_connector_list_iter_begin(&dev_priv->drm, &conn_list_iter);
> > > > > > > > + drm_for_each_connector_iter(conn_iter, &conn_list_iter) {
> > > > > > > > + struct drm_connector_state *conn_iter_state;
> > > > > > > > +
> > > > > > > > + if (!conn_iter->has_tile)
> > > > > > > > + continue;
> > > > > > > > + conn_iter_state = drm_atomic_get_connector_state(&state->base,
> > > > > > > > + conn_iter);
> > > > > > > > + if (IS_ERR(conn_iter_state)) {
> > > > > > > > + drm_connector_list_iter_end(&conn_list_iter);
> > > > > > > > + return PTR_ERR(conn_iter_state);
> > > > > > > > + }
> > > > > > > > +
> > > > > > > > + if (!conn_iter_state->crtc)
> > > > > > > > + continue;
> > > > > > > > +
> > > > > > > > + if (conn_iter->tile_group->id != tile_grp_id)
> > > > > > > > + continue;
> > > > > > > > +
> > > > > > > > + crtc_state = drm_atomic_get_crtc_state(&state->base, conn_iter_state->crtc);
> > > > > > > > + if (IS_ERR(crtc_state)) {
> > > > > > > > + drm_connector_list_iter_end(&conn_list_iter);
> > > > > > > > + return PTR_ERR(conn_iter_state);
> > > > > > > > + }
> > > > > > > > + crtc_state->mode_changed = true;
> > > > > > > > + }
> > > > > > > > + drm_connector_list_iter_end(&conn_list_iter);
> > > > > > > > +
> > > > > > > > + return 0;
> > > > > > > > +}
> > > > > > > > +
> > > > > > > > +static int
> > > > > > > > +intel_dp_atomic_trans_port_sync_check(struct drm_i915_private *dev_priv,
> > > > > > > > + struct intel_atomic_state *state)
> > > > > > > > +{
> > > > > > > > + struct drm_connector *connector;
> > > > > > > > + struct drm_crtc_state *crtc_state;
> > > > > > > > + struct drm_connector_state *connector_state;
> > > > > > > > + int i, ret, tile_grp_id = 0;
> > > > > > > > +
> > > > > > > > + if (INTEL_GEN(dev_priv) < 11)
> > > > > > > > + return 0;
> > > > > > > > +
> > > > > > > > + /* Is tiled, mark all other tiled CRTCs as needing a modeset */
> > > > > > > > + for_each_new_connector_in_state(&state->base, connector, connector_state, i) {
> > > > > > > > + if (!connector->has_tile)
> > > > > > > > + continue;
> > > > > > > > + if (connector_state->crtc &&
> > > > > > > > + tile_grp_id != connector->tile_group->id) {
> > > > > > > > + crtc_state = drm_atomic_get_new_crtc_state(&state->base,
> > > > > > > > + connector_state->crtc);
> > > > > > > > + if (!drm_atomic_crtc_needs_modeset(crtc_state))
> > > > > > > > + continue;
> > > > > > > > +
> > > > > > > > + tile_grp_id = connector->tile_group->id;
> > > > > > > > + } else
> > > > > > > > + continue;
> > > > > > > > +
> > > > > > > > + ret = intel_dp_modeset_all_tiles(dev_priv, state, tile_grp_id);
> > > > > > > > + if (ret)
> > > > > > > > + return ret;
> > > > > > > > + }
> > > > > > > > +
> > > > > > > > + return 0;
> > > > > > > > +}
> > > > > > >
> > > > > > > BTW after some more pondering I don't think this alone is sufficient.
> > > > > > > The tile information may have already disppeared so I believe we also
> > > > > > > need to make sure we mark all currently synced crtcs as needing a
> > > > > > > modeset if any of them need a modeset. And I guess that's pretty much
> > > > > > > the same function we'll need to handle fastset correctly.
> > > > > > >
> > > > > >
> > > > > > The crtcs in the current state get synced and master/slave assignments happen in icl_add_sync_mode_crtcs before compute_config call
> > > > > > So are you suggesting basically moving this function after the crtcs are synced and then just setting modeset
> > > > > > to true for all synced crtcs if one of them needs modeset?
> > > > >
> > > > > I think it should look something like:
> > > > >
> > > > > modeset_tiled_things();
> > > > > modeset_synced_things();
> > > >
> > > > but modeset_synced_things() should be called after icl_add_sync_crtcs() which as per your review should be called
> > > > from within modeset_pipe_config just before compute_config() call.
> > > >
> > > > >
> > > > > for_each() {
> > > >
> > > > This is the for_each_crtc loop in intel_atomic_check() right?
> > > >
> > > > > modeset_pipes_config();
> > > >
> > > > So have icl_add_sync_crtcs outside of modeset_pipe_config()?
> > > >
> > > > > if (can_fastset()) {
> > > > > modeset=false;
> > > > > fastset=true;
> > > > > }
> > > > > }
> > > > >
> > > > > modeset_synced_things();
> > > > >
> > > > > for_each() {
> > > > > if (!modeset && fastset)
> > > > > copy_state();
> > > > > }
> > > > We already do this in the code right?
> > > >
> > > > Manasi
> > > >
> > > > >
> > > > > >
> > > > > > And why would the tile information be disappeared?
> > > > >
> > > > > It'll get updated whenever someone does a getconnector() or whatever.
> > > > >
> > > > > Example:
> > > > > 1. sync pipe A and B, pipe A is master
> > > > > 2. swap pipe B display for something else
> > >
> > > If we disconnect and connect other display for pipe B, port sync mode is off and
> > > Pipe A no longer a master and we would reset the master_slave assignments and conn on pipe B would not have tile
> >
> > Port sync will stay enabled until we do a modeset to disable it. In this
> > example there is never any modeset on pipe B until we add soemthing to
> > force one in step 4.
>
> seems to be working with the current intel_dp_add_modeset_tiles() since the moment
> we unplug, it goes through the disable sequence where all the associated tiles get disabled
> and port sync mode gets disabled. This happens because the Pipe A which is still connected now indicates
> a mode change since it fallsback to a lower non tiled mode.
>
> But to be on safer side, i can check if say Pipe A is a master or slave (in port sync mode) , check its crtc_state which is
> still showing the old master slave links since we havent cleared those yet and then add all other synced crtcs
> to the modeset?
>
> So the order of calls can still be :
> intel_atomic_check() {
>
> intel_dp_atomic_tiled_check() { modeset_all_tiles modeset_synced_crtcs}
> intel_modeset_pipe_Config()
> icl_add_sync_crtcs
> compute_config
> fastsetcheck()
> modeset_synced_things (here it looks at the new master slave assignments)
>
> Does this look correct, I want send this patch out today
>
> Manasi
>
> >
> > >
> > > so why we wold need to add both pipes to modeset in this case at all
> > >
> > > Manasi
> > >
> > > > > 3. getconnector() -> tile info goes poof
> > > > > 4. do something on pipe A that needs a modeset
> > > > > no tile info so we miss that pipe B also needs a modeset
> > > > >
> > > > > --
> > > > > Ville Syrjälä
> > > > > Intel
> > > > _______________________________________________
> > > > Intel-gfx mailing list
> > > > Intel-gfx@lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >
> > --
> > Ville Syrjälä
> > Intel
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 35+ messages in thread
end of thread, other threads:[~2019-12-19 2:36 UTC | newest]
Thread overview: 35+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-11 21:14 [Intel-gfx] [PATCH 0/3] i915 fixes to handle hotplug cases on 8K tiled monitor Manasi Navare
2019-12-11 21:14 ` [Intel-gfx] [PATCH 1/3] drm/i915/dp: Make sure all tiled connectors get added to the state with full modeset Manasi Navare
2019-12-13 0:32 ` Matt Roper
2019-12-13 1:18 ` Manasi Navare
2019-12-13 3:11 ` Matt Roper
2019-12-13 20:05 ` Ville Syrjälä
2019-12-13 21:05 ` Manasi Navare
2019-12-13 21:17 ` Ville Syrjälä
2019-12-14 2:28 ` Manasi Navare
2019-12-16 12:03 ` Ville Syrjälä
2019-12-16 16:40 ` Manasi Navare
2019-12-16 17:11 ` Ville Syrjälä
2019-12-16 21:42 ` Manasi Navare
2019-12-16 14:37 ` Ville Syrjälä
2019-12-16 19:13 ` Manasi Navare
2019-12-16 21:37 ` Ville Syrjälä
2019-12-16 22:33 ` Manasi Navare
2019-12-16 22:58 ` Manasi Navare
2019-12-17 10:50 ` Ville Syrjälä
2019-12-17 19:04 ` Manasi Navare
2019-12-19 2:37 ` Manasi Navare
2019-12-11 21:14 ` [Intel-gfx] [PATCH 2/3] drm/i915/dp: Make port sync mode assignments only if all tiles present Manasi Navare
2019-12-13 1:03 ` Matt Roper
2019-12-13 1:09 ` Manasi Navare
2019-12-13 20:28 ` Ville Syrjälä
2019-12-13 20:53 ` Ville Syrjälä
2019-12-13 20:58 ` Manasi Navare
2019-12-11 21:14 ` [Intel-gfx] [PATCH 3/3] drm/i915/dp: Disable Port sync mode correctly on teardown Manasi Navare
2019-12-13 3:14 ` Matt Roper
2019-12-13 20:06 ` Ville Syrjälä
2019-12-13 20:40 ` Manasi Navare
2019-12-13 20:49 ` Ville Syrjälä
2019-12-12 1:53 ` [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for i915 fixes to handle hotplug cases on 8K tiled monitor Patchwork
2019-12-12 2:36 ` [Intel-gfx] ✓ Fi.CI.BAT: success " Patchwork
2019-12-12 13:49 ` [Intel-gfx] ✓ Fi.CI.IGT: " Patchwork
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.