* [Intel-gfx] [PATCH 1/4] drm/i915/display/tgl+: Dispatch atomic commits instead of front buffer modifications
@ 2021-07-31 0:10 José Roberto de Souza
2021-07-31 0:10 ` [Intel-gfx] [PATCH 2/4] drm/i915/display: Fix sel fetch plane offset calculation José Roberto de Souza
` (5 more replies)
0 siblings, 6 replies; 16+ messages in thread
From: José Roberto de Souza @ 2021-07-31 0:10 UTC (permalink / raw)
To: intel-gfx
Cc: Daniel Vetter, Gwan-gyeong Mun, Ville Syrjälä,
José Roberto de Souza
PSR2 selective fetch requires plane and transcoder registers to
be programed during the vblank to properly update the display and
there is no way around it.
We could disable PSR2 at every notification of dirty front buffer from
user space but that would hurt the power savings and it would still
cause some race conditions between PSR2 exit sequence and atomic
commits that causes underruns and glitches.
So from display 12 and newer we will start to do atomic commits
every time user space notify that front buffer is dirty and ignore
all frontbuffer flushes and invalidates on the PSR side.
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
drivers/gpu/drm/i915/display/intel_cursor.c | 3 ++-
drivers/gpu/drm/i915/display/intel_display.c | 7 ++++++-
drivers/gpu/drm/i915/display/intel_psr.c | 6 ++++++
3 files changed, 14 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_cursor.c b/drivers/gpu/drm/i915/display/intel_cursor.c
index c7618fef01439..d44022cb46a65 100644
--- a/drivers/gpu/drm/i915/display/intel_cursor.c
+++ b/drivers/gpu/drm/i915/display/intel_cursor.c
@@ -617,6 +617,7 @@ intel_legacy_cursor_update(struct drm_plane *_plane,
u32 src_w, u32 src_h,
struct drm_modeset_acquire_ctx *ctx)
{
+ struct drm_i915_private *i915 = to_i915(_crtc->dev);
struct intel_plane *plane = to_intel_plane(_plane);
struct intel_crtc *crtc = to_intel_crtc(_crtc);
struct intel_plane_state *old_plane_state =
@@ -638,7 +639,7 @@ intel_legacy_cursor_update(struct drm_plane *_plane,
*/
if (!crtc_state->hw.active || intel_crtc_needs_modeset(crtc_state) ||
crtc_state->update_pipe || crtc_state->bigjoiner ||
- crtc_state->enable_psr2_sel_fetch)
+ DISPLAY_VER(i915) >= 12)
goto slow;
/*
diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index 5ff0a011b28eb..4a936e1e7fa82 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -11720,10 +11720,15 @@ static int intel_user_framebuffer_dirty(struct drm_framebuffer *fb,
unsigned num_clips)
{
struct drm_i915_gem_object *obj = intel_fb_obj(fb);
+ struct drm_i915_private *i915 = to_i915(obj->base.dev);
i915_gem_object_flush_if_display(obj);
- intel_frontbuffer_flush(to_intel_frontbuffer(fb), ORIGIN_DIRTYFB);
+ if (DISPLAY_VER(i915) >= 12)
+ return drm_atomic_helper_dirtyfb(fb, file, flags, color, clips,
+ num_clips);
+
+ intel_frontbuffer_flush(to_intel_frontbuffer(fb), ORIGIN_DIRTYFB);
return 0;
}
diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c
index 1b0daf649e823..caf92f414a6e7 100644
--- a/drivers/gpu/drm/i915/display/intel_psr.c
+++ b/drivers/gpu/drm/i915/display/intel_psr.c
@@ -2039,6 +2039,9 @@ void intel_psr_invalidate(struct drm_i915_private *dev_priv,
{
struct intel_encoder *encoder;
+ if (DISPLAY_VER(dev_priv) >= 12)
+ return;
+
if (origin == ORIGIN_FLIP)
return;
@@ -2123,6 +2126,9 @@ void intel_psr_flush(struct drm_i915_private *dev_priv,
continue;
}
+ if (DISPLAY_VER(dev_priv) >= 12)
+ continue;
+
mutex_lock(&intel_dp->psr.lock);
if (!intel_dp->psr.enabled) {
mutex_unlock(&intel_dp->psr.lock);
--
2.32.0
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [Intel-gfx] [PATCH 2/4] drm/i915/display: Fix sel fetch plane offset calculation
2021-07-31 0:10 [Intel-gfx] [PATCH 1/4] drm/i915/display/tgl+: Dispatch atomic commits instead of front buffer modifications José Roberto de Souza
@ 2021-07-31 0:10 ` José Roberto de Souza
2021-08-13 7:40 ` Gwan-gyeong Mun
2021-07-31 0:10 ` [Intel-gfx] [PATCH 3/4] drm/i915: Nuke ORIGIN_GTT José Roberto de Souza
` (4 subsequent siblings)
5 siblings, 1 reply; 16+ messages in thread
From: José Roberto de Souza @ 2021-07-31 0:10 UTC (permalink / raw)
To: intel-gfx
Cc: Gwan-gyeong Mun, Ville Syrjälä, José Roberto de Souza
skl_calc_main_surface_offset() is used to calculate an aligned plane
surface address considering the inner framebuffer x and y offset.
It can not be used by selective fetch functions becase there is no
PLANE_SEL_FETCH_SURF.
So the PLANE_SEL_FETCH_OFFSET.y should only be PLANE_OFFSET.y +
damaged_area_within_plane.y1.
This fixes glitches seen in fbcon caused by typing something in
the terminal.
BSpec: 55229
Cc: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
drivers/gpu/drm/i915/display/intel_psr.c | 8 ++------
1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c
index caf92f414a6e7..894a2d35668a2 100644
--- a/drivers/gpu/drm/i915/display/intel_psr.c
+++ b/drivers/gpu/drm/i915/display/intel_psr.c
@@ -1487,8 +1487,8 @@ void intel_psr2_program_plane_sel_fetch(struct intel_plane *plane,
struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
enum pipe pipe = plane->pipe;
const struct drm_rect *clip;
- u32 val, offset;
- int ret, x, y;
+ u32 val;
+ int x, y;
if (!crtc_state->enable_psr2_sel_fetch)
return;
@@ -1508,10 +1508,6 @@ void intel_psr2_program_plane_sel_fetch(struct intel_plane *plane,
/* TODO: consider auxiliary surfaces */
x = plane_state->uapi.src.x1 >> 16;
y = (plane_state->uapi.src.y1 >> 16) + clip->y1;
- ret = skl_calc_main_surface_offset(plane_state, &x, &y, &offset);
- if (ret)
- drm_warn_once(&dev_priv->drm, "skl_calc_main_surface_offset() returned %i\n",
- ret);
val = y << 16 | x;
intel_de_write_fw(dev_priv, PLANE_SEL_FETCH_OFFSET(pipe, plane->id),
val);
--
2.32.0
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [Intel-gfx] [PATCH 3/4] drm/i915: Nuke ORIGIN_GTT
2021-07-31 0:10 [Intel-gfx] [PATCH 1/4] drm/i915/display/tgl+: Dispatch atomic commits instead of front buffer modifications José Roberto de Souza
2021-07-31 0:10 ` [Intel-gfx] [PATCH 2/4] drm/i915/display: Fix sel fetch plane offset calculation José Roberto de Souza
@ 2021-07-31 0:10 ` José Roberto de Souza
2021-08-03 11:20 ` Gwan-gyeong Mun
2021-07-31 0:10 ` [Intel-gfx] [PATCH 4/4] DO_NOT_MERGE: drm/i915/display: Enable PSR2 selective fetch by default José Roberto de Souza
` (3 subsequent siblings)
5 siblings, 1 reply; 16+ messages in thread
From: José Roberto de Souza @ 2021-07-31 0:10 UTC (permalink / raw)
To: intel-gfx; +Cc: Gwan-gyeong Mun, José Roberto de Souza
There is no users of it, so no need to keep handling for it.
Cc: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
drivers/gpu/drm/i915/display/intel_fbc.c | 10 +---------
drivers/gpu/drm/i915/display/intel_frontbuffer.h | 3 +--
2 files changed, 2 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c b/drivers/gpu/drm/i915/display/intel_fbc.c
index ddfc17e21668a..e4d412d395c34 100644
--- a/drivers/gpu/drm/i915/display/intel_fbc.c
+++ b/drivers/gpu/drm/i915/display/intel_fbc.c
@@ -1129,7 +1129,7 @@ void intel_fbc_invalidate(struct drm_i915_private *dev_priv,
if (!HAS_FBC(dev_priv))
return;
- if (origin == ORIGIN_GTT || origin == ORIGIN_FLIP)
+ if (origin == ORIGIN_FLIP)
return;
mutex_lock(&fbc->lock);
@@ -1150,14 +1150,6 @@ void intel_fbc_flush(struct drm_i915_private *dev_priv,
if (!HAS_FBC(dev_priv))
return;
- /*
- * GTT tracking does not nuke the entire cfb
- * so don't clear busy_bits set for some other
- * reason.
- */
- if (origin == ORIGIN_GTT)
- return;
-
mutex_lock(&fbc->lock);
fbc->busy_bits &= ~frontbuffer_bits;
diff --git a/drivers/gpu/drm/i915/display/intel_frontbuffer.h b/drivers/gpu/drm/i915/display/intel_frontbuffer.h
index 6d41f53944250..4b977c1e4d52b 100644
--- a/drivers/gpu/drm/i915/display/intel_frontbuffer.h
+++ b/drivers/gpu/drm/i915/display/intel_frontbuffer.h
@@ -33,8 +33,7 @@
struct drm_i915_private;
enum fb_op_origin {
- ORIGIN_GTT,
- ORIGIN_CPU,
+ ORIGIN_CPU = 0,
ORIGIN_CS,
ORIGIN_FLIP,
ORIGIN_DIRTYFB,
--
2.32.0
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [Intel-gfx] [PATCH 4/4] DO_NOT_MERGE: drm/i915/display: Enable PSR2 selective fetch by default
2021-07-31 0:10 [Intel-gfx] [PATCH 1/4] drm/i915/display/tgl+: Dispatch atomic commits instead of front buffer modifications José Roberto de Souza
2021-07-31 0:10 ` [Intel-gfx] [PATCH 2/4] drm/i915/display: Fix sel fetch plane offset calculation José Roberto de Souza
2021-07-31 0:10 ` [Intel-gfx] [PATCH 3/4] drm/i915: Nuke ORIGIN_GTT José Roberto de Souza
@ 2021-07-31 0:10 ` José Roberto de Souza
2021-08-03 11:17 ` Gwan-gyeong Mun
2021-07-31 0:14 ` [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for series starting with [1/4] drm/i915/display/tgl+: Dispatch atomic commits instead of front buffer modifications Patchwork
` (2 subsequent siblings)
5 siblings, 1 reply; 16+ messages in thread
From: José Roberto de Souza @ 2021-07-31 0:10 UTC (permalink / raw)
To: intel-gfx; +Cc: José Roberto de Souza
Only to execute tests with PSR2 selective fetch enabled and check what
is broken.
IGT tests know to fail with this:
- kms_cursor_legacy: all tests that checks if evasion happend, I have
fix for it making cursor_slowpath() returns true for display 12+.
- kms_psr2_su: The pageflip test, it needs to have the damage clip set
otherwise it will update the whole screen and the selective blocks
will not match with expected.
- kms_psr: psr2_*_(mmap_gtt, mmap_cpu, blt and render), all those
tests should be dropped or skipped for display 12+.
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
drivers/gpu/drm/i915/display/intel_psr.c | 9 ---------
drivers/gpu/drm/i915/i915_params.h | 2 +-
2 files changed, 1 insertion(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c
index 894a2d35668a2..e128f0c2aeecc 100644
--- a/drivers/gpu/drm/i915/display/intel_psr.c
+++ b/drivers/gpu/drm/i915/display/intel_psr.c
@@ -877,15 +877,6 @@ static bool intel_psr2_config_valid(struct intel_dp *intel_dp,
return false;
}
- /*
- * We are missing the implementation of some workarounds to enabled PSR2
- * in Alderlake_P, until ready PSR2 should be kept disabled.
- */
- if (IS_ALDERLAKE_P(dev_priv)) {
- drm_dbg_kms(&dev_priv->drm, "PSR2 is missing the implementation of workarounds\n");
- return false;
- }
-
if (!transcoder_has_psr2(dev_priv, crtc_state->cpu_transcoder)) {
drm_dbg_kms(&dev_priv->drm,
"PSR2 not supported in transcoder %s\n",
diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h
index f27eceb82c0f5..8d725b64592d8 100644
--- a/drivers/gpu/drm/i915/i915_params.h
+++ b/drivers/gpu/drm/i915/i915_params.h
@@ -55,7 +55,7 @@ struct drm_printer;
param(int, enable_fbc, -1, 0600) \
param(int, enable_psr, -1, 0600) \
param(bool, psr_safest_params, false, 0400) \
- param(bool, enable_psr2_sel_fetch, false, 0400) \
+ param(bool, enable_psr2_sel_fetch, true, 0400) \
param(int, disable_power_well, -1, 0400) \
param(int, enable_ips, 1, 0600) \
param(int, invert_brightness, 0, 0600) \
--
2.32.0
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for series starting with [1/4] drm/i915/display/tgl+: Dispatch atomic commits instead of front buffer modifications
2021-07-31 0:10 [Intel-gfx] [PATCH 1/4] drm/i915/display/tgl+: Dispatch atomic commits instead of front buffer modifications José Roberto de Souza
` (2 preceding siblings ...)
2021-07-31 0:10 ` [Intel-gfx] [PATCH 4/4] DO_NOT_MERGE: drm/i915/display: Enable PSR2 selective fetch by default José Roberto de Souza
@ 2021-07-31 0:14 ` Patchwork
2021-07-31 0:45 ` [Intel-gfx] ✗ Fi.CI.BAT: failure " Patchwork
2021-08-12 17:24 ` [Intel-gfx] [PATCH 1/4] " Souza, Jose
5 siblings, 0 replies; 16+ messages in thread
From: Patchwork @ 2021-07-31 0:14 UTC (permalink / raw)
To: José Roberto de Souza; +Cc: intel-gfx
== Series Details ==
Series: series starting with [1/4] drm/i915/display/tgl+: Dispatch atomic commits instead of front buffer modifications
URL : https://patchwork.freedesktop.org/series/93252/
State : warning
== Summary ==
$ dim checkpatch origin/drm-tip
762853c5a7c2 drm/i915/display/tgl+: Dispatch atomic commits instead of front buffer modifications
d75f9be16572 drm/i915/display: Fix sel fetch plane offset calculation
06de84df5401 drm/i915: Nuke ORIGIN_GTT
87fee2709b10 DO_NOT_MERGE: drm/i915/display: Enable PSR2 selective fetch by default
-:14: WARNING:TYPO_SPELLING: 'happend' may be misspelled - perhaps 'happened'?
#14:
- kms_cursor_legacy: all tests that checks if evasion happend, I have
^^^^^^^
total: 0 errors, 1 warnings, 0 checks, 23 lines checked
^ permalink raw reply [flat|nested] 16+ messages in thread
* [Intel-gfx] ✗ Fi.CI.BAT: failure for series starting with [1/4] drm/i915/display/tgl+: Dispatch atomic commits instead of front buffer modifications
2021-07-31 0:10 [Intel-gfx] [PATCH 1/4] drm/i915/display/tgl+: Dispatch atomic commits instead of front buffer modifications José Roberto de Souza
` (3 preceding siblings ...)
2021-07-31 0:14 ` [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for series starting with [1/4] drm/i915/display/tgl+: Dispatch atomic commits instead of front buffer modifications Patchwork
@ 2021-07-31 0:45 ` Patchwork
2021-08-12 17:24 ` [Intel-gfx] [PATCH 1/4] " Souza, Jose
5 siblings, 0 replies; 16+ messages in thread
From: Patchwork @ 2021-07-31 0:45 UTC (permalink / raw)
To: José Roberto de Souza; +Cc: intel-gfx
[-- Attachment #1: Type: text/plain, Size: 5125 bytes --]
== Series Details ==
Series: series starting with [1/4] drm/i915/display/tgl+: Dispatch atomic commits instead of front buffer modifications
URL : https://patchwork.freedesktop.org/series/93252/
State : failure
== Summary ==
CI Bug Log - changes from CI_DRM_10427 -> Patchwork_20753
====================================================
Summary
-------
**FAILURE**
Serious unknown changes coming with Patchwork_20753 absolutely need to be
verified manually.
If you think the reported changes have nothing to do with the changes
introduced in Patchwork_20753, please notify your bug team to allow them
to document this new failure mode, which will reduce false positives in CI.
External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20753/index.html
Possible new issues
-------------------
Here are the unknown changes that may have been introduced in Patchwork_20753:
### IGT changes ###
#### Possible regressions ####
* igt@kms_cursor_legacy@basic-flip-before-cursor-atomic:
- fi-tgl-1115g4: [PASS][1] -> [FAIL][2] +3 similar issues
[1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10427/fi-tgl-1115g4/igt@kms_cursor_legacy@basic-flip-before-cursor-atomic.html
[2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20753/fi-tgl-1115g4/igt@kms_cursor_legacy@basic-flip-before-cursor-atomic.html
* igt@kms_cursor_legacy@basic-flip-before-cursor-legacy:
- fi-tgl-u2: [PASS][3] -> [FAIL][4] +3 similar issues
[3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10427/fi-tgl-u2/igt@kms_cursor_legacy@basic-flip-before-cursor-legacy.html
[4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20753/fi-tgl-u2/igt@kms_cursor_legacy@basic-flip-before-cursor-legacy.html
#### Suppressed ####
The following results come from untrusted machines, tests, or statuses.
They do not affect the overall result.
* igt@kms_cursor_legacy@basic-flip-before-cursor-atomic:
- {fi-tgl-dsi}: [PASS][5] -> [FAIL][6] +3 similar issues
[5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10427/fi-tgl-dsi/igt@kms_cursor_legacy@basic-flip-before-cursor-atomic.html
[6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20753/fi-tgl-dsi/igt@kms_cursor_legacy@basic-flip-before-cursor-atomic.html
Known issues
------------
Here are the changes found in Patchwork_20753 that come from known issues:
### IGT changes ###
#### Issues hit ####
* igt@amdgpu/amd_cs_nop@sync-fork-compute0:
- fi-kbl-soraka: NOTRUN -> [SKIP][7] ([fdo#109271]) +10 similar issues
[7]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20753/fi-kbl-soraka/igt@amdgpu/amd_cs_nop@sync-fork-compute0.html
* igt@i915_selftest@live@execlists:
- fi-icl-y: [PASS][8] -> [DMESG-FAIL][9] ([i915#1993])
[8]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10427/fi-icl-y/igt@i915_selftest@live@execlists.html
[9]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20753/fi-icl-y/igt@i915_selftest@live@execlists.html
#### Possible fixes ####
* igt@gem_exec_suspend@basic-s3:
- fi-tgl-1115g4: [FAIL][10] ([i915#1888]) -> [PASS][11]
[10]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10427/fi-tgl-1115g4/igt@gem_exec_suspend@basic-s3.html
[11]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20753/fi-tgl-1115g4/igt@gem_exec_suspend@basic-s3.html
* igt@i915_selftest@live@workarounds:
- fi-tgl-u2: [DMESG-WARN][12] ([i915#2867]) -> [PASS][13] +16 similar issues
[12]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10427/fi-tgl-u2/igt@i915_selftest@live@workarounds.html
[13]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20753/fi-tgl-u2/igt@i915_selftest@live@workarounds.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
[i915#1888]: https://gitlab.freedesktop.org/drm/intel/issues/1888
[i915#1993]: https://gitlab.freedesktop.org/drm/intel/issues/1993
[i915#2867]: https://gitlab.freedesktop.org/drm/intel/issues/2867
Participating hosts (37 -> 33)
------------------------------
Missing (4): fi-bdw-samus fi-bsw-cyan bat-jsl-1 fi-hsw-4200u
Build changes
-------------
* Linux: CI_DRM_10427 -> Patchwork_20753
CI-20190529: 20190529
CI_DRM_10427: e5efc20bce6b7996df9bdc720796f8713a45701a @ git://anongit.freedesktop.org/gfx-ci/linux
IGT_6159: 6135b9cc319ed965e3aafb5b2ae2abf4762a06b2 @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
Patchwork_20753: 87fee2709b10c39c5cd3c558f356f6821d01df03 @ git://anongit.freedesktop.org/gfx-ci/linux
== Linux commits ==
87fee2709b10 DO_NOT_MERGE: drm/i915/display: Enable PSR2 selective fetch by default
06de84df5401 drm/i915: Nuke ORIGIN_GTT
d75f9be16572 drm/i915/display: Fix sel fetch plane offset calculation
762853c5a7c2 drm/i915/display/tgl+: Dispatch atomic commits instead of front buffer modifications
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20753/index.html
[-- Attachment #2: Type: text/html, Size: 5958 bytes --]
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Intel-gfx] [PATCH 4/4] DO_NOT_MERGE: drm/i915/display: Enable PSR2 selective fetch by default
2021-07-31 0:10 ` [Intel-gfx] [PATCH 4/4] DO_NOT_MERGE: drm/i915/display: Enable PSR2 selective fetch by default José Roberto de Souza
@ 2021-08-03 11:17 ` Gwan-gyeong Mun
2021-08-03 17:18 ` Souza, Jose
0 siblings, 1 reply; 16+ messages in thread
From: Gwan-gyeong Mun @ 2021-08-03 11:17 UTC (permalink / raw)
To: José Roberto de Souza, intel-gfx
On 7/31/21 3:10 AM, José Roberto de Souza wrote:
> Only to execute tests with PSR2 selective fetch enabled and check what
> is broken.
>
> IGT tests know to fail with this:
> - kms_cursor_legacy: all tests that checks if evasion happend, I have
> fix for it making cursor_slowpath() returns true for display 12+.
>
> - kms_psr2_su: The pageflip test, it needs to have the damage clip set
> otherwise it will update the whole screen and the selective blocks
> will not match with expected.
>
kms_psr2_su is a test case for intel PSR2 HW tracking and kms_psr2_sf is
used as a test for intel PSR2 manual tracking. Is it necessary to modify
kms_psr2_su for testing PSR2 manual tracking?
> - kms_psr: psr2_*_(mmap_gtt, mmap_cpu, blt and render), all those
> tests should be dropped or skipped for display 12+.
>
Could you explain in more detail why we need to skip on display 12+?
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
> drivers/gpu/drm/i915/display/intel_psr.c | 9 ---------
> drivers/gpu/drm/i915/i915_params.h | 2 +-
> 2 files changed, 1 insertion(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c
> index 894a2d35668a2..e128f0c2aeecc 100644
> --- a/drivers/gpu/drm/i915/display/intel_psr.c
> +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> @@ -877,15 +877,6 @@ static bool intel_psr2_config_valid(struct intel_dp *intel_dp,
> return false;
> }
>
> - /*
> - * We are missing the implementation of some workarounds to enabled PSR2
> - * in Alderlake_P, until ready PSR2 should be kept disabled.
> - */
> - if (IS_ALDERLAKE_P(dev_priv)) {
> - drm_dbg_kms(&dev_priv->drm, "PSR2 is missing the implementation of workarounds\n");
> - return false;
> - }
> -
> if (!transcoder_has_psr2(dev_priv, crtc_state->cpu_transcoder)) {
> drm_dbg_kms(&dev_priv->drm,
> "PSR2 not supported in transcoder %s\n",
> diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h
> index f27eceb82c0f5..8d725b64592d8 100644
> --- a/drivers/gpu/drm/i915/i915_params.h
> +++ b/drivers/gpu/drm/i915/i915_params.h
> @@ -55,7 +55,7 @@ struct drm_printer;
> param(int, enable_fbc, -1, 0600) \
> param(int, enable_psr, -1, 0600) \
> param(bool, psr_safest_params, false, 0400) \
> - param(bool, enable_psr2_sel_fetch, false, 0400) \
> + param(bool, enable_psr2_sel_fetch, true, 0400) \
> param(int, disable_power_well, -1, 0400) \
> param(int, enable_ips, 1, 0600) \
> param(int, invert_brightness, 0, 0600) \
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Intel-gfx] [PATCH 3/4] drm/i915: Nuke ORIGIN_GTT
2021-07-31 0:10 ` [Intel-gfx] [PATCH 3/4] drm/i915: Nuke ORIGIN_GTT José Roberto de Souza
@ 2021-08-03 11:20 ` Gwan-gyeong Mun
2021-08-03 17:19 ` Souza, Jose
0 siblings, 1 reply; 16+ messages in thread
From: Gwan-gyeong Mun @ 2021-08-03 11:20 UTC (permalink / raw)
To: José Roberto de Souza, intel-gfx
On 7/31/21 3:10 AM, José Roberto de Souza wrote:
> There is no users of it, so no need to keep handling for it.
>
> Cc: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
> drivers/gpu/drm/i915/display/intel_fbc.c | 10 +---------
> drivers/gpu/drm/i915/display/intel_frontbuffer.h | 3 +--
> 2 files changed, 2 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c b/drivers/gpu/drm/i915/display/intel_fbc.c
> index ddfc17e21668a..e4d412d395c34 100644
> --- a/drivers/gpu/drm/i915/display/intel_fbc.c
> +++ b/drivers/gpu/drm/i915/display/intel_fbc.c
> @@ -1129,7 +1129,7 @@ void intel_fbc_invalidate(struct drm_i915_private *dev_priv,
> if (!HAS_FBC(dev_priv))
> return;
>
> - if (origin == ORIGIN_GTT || origin == ORIGIN_FLIP)
> + if (origin == ORIGIN_FLIP)
> return;
>
> mutex_lock(&fbc->lock);
> @@ -1150,14 +1150,6 @@ void intel_fbc_flush(struct drm_i915_private *dev_priv,
> if (!HAS_FBC(dev_priv))
> return;
>
> - /*
> - * GTT tracking does not nuke the entire cfb
> - * so don't clear busy_bits set for some other
> - * reason.
> - */
> - if (origin == ORIGIN_GTT)
> - return;
> -
> mutex_lock(&fbc->lock);
>
> fbc->busy_bits &= ~frontbuffer_bits;
> diff --git a/drivers/gpu/drm/i915/display/intel_frontbuffer.h b/drivers/gpu/drm/i915/display/intel_frontbuffer.h
> index 6d41f53944250..4b977c1e4d52b 100644
> --- a/drivers/gpu/drm/i915/display/intel_frontbuffer.h
> +++ b/drivers/gpu/drm/i915/display/intel_frontbuffer.h
> @@ -33,8 +33,7 @@
> struct drm_i915_private;
>
> enum fb_op_origin {
> - ORIGIN_GTT,
> - ORIGIN_CPU,
> + ORIGIN_CPU = 0,
> ORIGIN_CS,
> ORIGIN_FLIP,
> ORIGIN_DIRTYFB,
>
Is this patch absolutely necessary for this series?
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Intel-gfx] [PATCH 4/4] DO_NOT_MERGE: drm/i915/display: Enable PSR2 selective fetch by default
2021-08-03 11:17 ` Gwan-gyeong Mun
@ 2021-08-03 17:18 ` Souza, Jose
2021-08-05 18:26 ` Gwan-gyeong Mun
0 siblings, 1 reply; 16+ messages in thread
From: Souza, Jose @ 2021-08-03 17:18 UTC (permalink / raw)
To: Mun, Gwan-gyeong, intel-gfx
On Tue, 2021-08-03 at 14:17 +0300, Gwan-gyeong Mun wrote:
>
> On 7/31/21 3:10 AM, José Roberto de Souza wrote:
> > Only to execute tests with PSR2 selective fetch enabled and check what
> > is broken.
> >
> > IGT tests know to fail with this:
> > - kms_cursor_legacy: all tests that checks if evasion happend, I have
> > fix for it making cursor_slowpath() returns true for display 12+.
> >
> > - kms_psr2_su: The pageflip test, it needs to have the damage clip set
> > otherwise it will update the whole screen and the selective blocks
> > will not match with expected.
> >
> kms_psr2_su is a test case for intel PSR2 HW tracking and kms_psr2_sf is
> used as a test for intel PSR2 manual tracking. Is it necessary to modify
> kms_psr2_su for testing PSR2 manual tracking?
kms_psr2_su is to test that PSR2 is sending selective updates, just adding a couple of lines we can make it work with selective fetch.
> > - kms_psr: psr2_*_(mmap_gtt, mmap_cpu, blt and render), all those
> > tests should be dropped or skipped for display 12+.
> >
> Could you explain in more detail why we need to skip on display 12+?
This are stuff that would end up calling intel_psr_invalidate/flush().
>
> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > ---
> > drivers/gpu/drm/i915/display/intel_psr.c | 9 ---------
> > drivers/gpu/drm/i915/i915_params.h | 2 +-
> > 2 files changed, 1 insertion(+), 10 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c
> > index 894a2d35668a2..e128f0c2aeecc 100644
> > --- a/drivers/gpu/drm/i915/display/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> > @@ -877,15 +877,6 @@ static bool intel_psr2_config_valid(struct intel_dp *intel_dp,
> > return false;
> > }
> >
> > - /*
> > - * We are missing the implementation of some workarounds to enabled PSR2
> > - * in Alderlake_P, until ready PSR2 should be kept disabled.
> > - */
> > - if (IS_ALDERLAKE_P(dev_priv)) {
> > - drm_dbg_kms(&dev_priv->drm, "PSR2 is missing the implementation of workarounds\n");
> > - return false;
> > - }
> > -
> > if (!transcoder_has_psr2(dev_priv, crtc_state->cpu_transcoder)) {
> > drm_dbg_kms(&dev_priv->drm,
> > "PSR2 not supported in transcoder %s\n",
> > diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h
> > index f27eceb82c0f5..8d725b64592d8 100644
> > --- a/drivers/gpu/drm/i915/i915_params.h
> > +++ b/drivers/gpu/drm/i915/i915_params.h
> > @@ -55,7 +55,7 @@ struct drm_printer;
> > param(int, enable_fbc, -1, 0600) \
> > param(int, enable_psr, -1, 0600) \
> > param(bool, psr_safest_params, false, 0400) \
> > - param(bool, enable_psr2_sel_fetch, false, 0400) \
> > + param(bool, enable_psr2_sel_fetch, true, 0400) \
> > param(int, disable_power_well, -1, 0400) \
> > param(int, enable_ips, 1, 0600) \
> > param(int, invert_brightness, 0, 0600) \
> >
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Intel-gfx] [PATCH 3/4] drm/i915: Nuke ORIGIN_GTT
2021-08-03 11:20 ` Gwan-gyeong Mun
@ 2021-08-03 17:19 ` Souza, Jose
2021-08-13 7:41 ` Gwan-gyeong Mun
0 siblings, 1 reply; 16+ messages in thread
From: Souza, Jose @ 2021-08-03 17:19 UTC (permalink / raw)
To: Mun, Gwan-gyeong, intel-gfx
On Tue, 2021-08-03 at 14:20 +0300, Gwan-gyeong Mun wrote:
>
> On 7/31/21 3:10 AM, José Roberto de Souza wrote:
> > There is no users of it, so no need to keep handling for it.
> >
> > Cc: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com>
> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > ---
> > drivers/gpu/drm/i915/display/intel_fbc.c | 10 +---------
> > drivers/gpu/drm/i915/display/intel_frontbuffer.h | 3 +--
> > 2 files changed, 2 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c b/drivers/gpu/drm/i915/display/intel_fbc.c
> > index ddfc17e21668a..e4d412d395c34 100644
> > --- a/drivers/gpu/drm/i915/display/intel_fbc.c
> > +++ b/drivers/gpu/drm/i915/display/intel_fbc.c
> > @@ -1129,7 +1129,7 @@ void intel_fbc_invalidate(struct drm_i915_private *dev_priv,
> > if (!HAS_FBC(dev_priv))
> > return;
> >
> > - if (origin == ORIGIN_GTT || origin == ORIGIN_FLIP)
> > + if (origin == ORIGIN_FLIP)
> > return;
> >
> > mutex_lock(&fbc->lock);
> > @@ -1150,14 +1150,6 @@ void intel_fbc_flush(struct drm_i915_private *dev_priv,
> > if (!HAS_FBC(dev_priv))
> > return;
> >
> > - /*
> > - * GTT tracking does not nuke the entire cfb
> > - * so don't clear busy_bits set for some other
> > - * reason.
> > - */
> > - if (origin == ORIGIN_GTT)
> > - return;
> > -
> > mutex_lock(&fbc->lock);
> >
> > fbc->busy_bits &= ~frontbuffer_bits;
> > diff --git a/drivers/gpu/drm/i915/display/intel_frontbuffer.h b/drivers/gpu/drm/i915/display/intel_frontbuffer.h
> > index 6d41f53944250..4b977c1e4d52b 100644
> > --- a/drivers/gpu/drm/i915/display/intel_frontbuffer.h
> > +++ b/drivers/gpu/drm/i915/display/intel_frontbuffer.h
> > @@ -33,8 +33,7 @@
> > struct drm_i915_private;
> >
> > enum fb_op_origin {
> > - ORIGIN_GTT,
> > - ORIGIN_CPU,
> > + ORIGIN_CPU = 0,
> > ORIGIN_CS,
> > ORIGIN_FLIP,
> > ORIGIN_DIRTYFB,
> >
> Is this patch absolutely necessary for this series?
Nope, just notice this unused ORIGIN_GTT while debugging.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Intel-gfx] [PATCH 4/4] DO_NOT_MERGE: drm/i915/display: Enable PSR2 selective fetch by default
2021-08-03 17:18 ` Souza, Jose
@ 2021-08-05 18:26 ` Gwan-gyeong Mun
2021-08-05 21:47 ` Souza, Jose
0 siblings, 1 reply; 16+ messages in thread
From: Gwan-gyeong Mun @ 2021-08-05 18:26 UTC (permalink / raw)
To: Souza, Jose, intel-gfx
On 8/3/21 8:18 PM, Souza, Jose wrote:
> On Tue, 2021-08-03 at 14:17 +0300, Gwan-gyeong Mun wrote:
>>
>> On 7/31/21 3:10 AM, José Roberto de Souza wrote:
>>> Only to execute tests with PSR2 selective fetch enabled and check what
>>> is broken.
>>>
>>> IGT tests know to fail with this:
>>> - kms_cursor_legacy: all tests that checks if evasion happend, I have
>>> fix for it making cursor_slowpath() returns true for display 12+.
>>>
>>> - kms_psr2_su: The pageflip test, it needs to have the damage clip set
>>> otherwise it will update the whole screen and the selective blocks
>>> will not match with expected.
>>>
>> kms_psr2_su is a test case for intel PSR2 HW tracking and kms_psr2_sf is
>> used as a test for intel PSR2 manual tracking. Is it necessary to modify
>> kms_psr2_su for testing PSR2 manual tracking?
>
> kms_psr2_su is to test that PSR2 is sending selective updates, just adding a couple of lines we can make it work with selective fetch.
>
>>> - kms_psr: psr2_*_(mmap_gtt, mmap_cpu, blt and render), all those
>>> tests should be dropped or skipped for display 12+.
>>>
>> Could you explain in more detail why we need to skip on display 12+?
>
> This are stuff that would end up calling intel_psr_invalidate/flush().
>
Thanks for the explanation.
And there is an issue confirmed in local tests, so I leave additional
comments.
>>
>>> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
>>> ---
>>> drivers/gpu/drm/i915/display/intel_psr.c | 9 ---------
>>> drivers/gpu/drm/i915/i915_params.h | 2 +-
>>> 2 files changed, 1 insertion(+), 10 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c
>>> index 894a2d35668a2..e128f0c2aeecc 100644
>>> --- a/drivers/gpu/drm/i915/display/intel_psr.c
>>> +++ b/drivers/gpu/drm/i915/display/intel_psr.c
>>> @@ -877,15 +877,6 @@ static bool intel_psr2_config_valid(struct intel_dp *intel_dp,
>>> return false;
>>> }
>>>
>>> -/*
>>> - * We are missing the implementation of some workarounds to enabled PSR2
>>> - * in Alderlake_P, until ready PSR2 should be kept disabled.
>>> - */
>>> -if (IS_ALDERLAKE_P(dev_priv)) {
>>> -drm_dbg_kms(&dev_priv->drm, "PSR2 is missing the implementation of workarounds\n");
>>> -return false;
>>> -}
>>> -
>>> if (!transcoder_has_psr2(dev_priv, crtc_state->cpu_transcoder)) {
>>> drm_dbg_kms(&dev_priv->drm,
>>> "PSR2 not supported in transcoder %s\n",
>>> diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h
>>> index f27eceb82c0f5..8d725b64592d8 100644
>>> --- a/drivers/gpu/drm/i915/i915_params.h
>>> +++ b/drivers/gpu/drm/i915/i915_params.h
>>> @@ -55,7 +55,7 @@ struct drm_printer;
>>> param(int, enable_fbc, -1, 0600) \
>>> param(int, enable_psr, -1, 0600) \
>>> param(bool, psr_safest_params, false, 0400) \
>>> -param(bool, enable_psr2_sel_fetch, false, 0400) \
>>> +param(bool, enable_psr2_sel_fetch, true, 0400) \
If we do not modify this part and do not enable it by default at boot
time as shown in the original code below,
param(bool, enable_psr2_sel_fetch, false, 0400) \
when we execute the kms_psr2_sf test case of igt, the FIFO underrun as
below still occurs.
i915 0000:00:02.0: [drm] *ERROR* CPU pipe A FIFO underrun: port,transcoder,
When PSR2 panel is used, PSR1 is enabled by default when
enable_psr2_sel_fetch is not enabled by default.
And when kms_psr2_sf is executed, the mode is changed to PSR2, and when
kms_psr2_sf is terminated, PSR2 is deactivated and PSR1 is re-enabled.
At this point. I suspect there is a problem.
>>> param(int, disable_power_well, -1, 0400) \
>>> param(int, enable_ips, 1, 0600) \
>>> param(int, invert_brightness, 0, 0600) \
>>>
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Intel-gfx] [PATCH 4/4] DO_NOT_MERGE: drm/i915/display: Enable PSR2 selective fetch by default
2021-08-05 18:26 ` Gwan-gyeong Mun
@ 2021-08-05 21:47 ` Souza, Jose
0 siblings, 0 replies; 16+ messages in thread
From: Souza, Jose @ 2021-08-05 21:47 UTC (permalink / raw)
To: Mun, Gwan-gyeong, intel-gfx
On Thu, 2021-08-05 at 21:26 +0300, Gwan-gyeong Mun wrote:
>
> On 8/3/21 8:18 PM, Souza, Jose wrote:
> > On Tue, 2021-08-03 at 14:17 +0300, Gwan-gyeong Mun wrote:
> > >
> > > On 7/31/21 3:10 AM, José Roberto de Souza wrote:
> > > > Only to execute tests with PSR2 selective fetch enabled and check what
> > > > is broken.
> > > >
> > > > IGT tests know to fail with this:
> > > > - kms_cursor_legacy: all tests that checks if evasion happend, I have
> > > > fix for it making cursor_slowpath() returns true for display 12+.
> > > >
> > > > - kms_psr2_su: The pageflip test, it needs to have the damage clip set
> > > > otherwise it will update the whole screen and the selective blocks
> > > > will not match with expected.
> > > >
> > > kms_psr2_su is a test case for intel PSR2 HW tracking and kms_psr2_sf is
> > > used as a test for intel PSR2 manual tracking. Is it necessary to modify
> > > kms_psr2_su for testing PSR2 manual tracking?
> >
> > kms_psr2_su is to test that PSR2 is sending selective updates, just adding a couple of lines we can make it work with selective fetch.
> >
> > > > - kms_psr: psr2_*_(mmap_gtt, mmap_cpu, blt and render), all those
> > > > tests should be dropped or skipped for display 12+.
> > > >
> > > Could you explain in more detail why we need to skip on display 12+?
> >
> > This are stuff that would end up calling intel_psr_invalidate/flush().
> >
>
> Thanks for the explanation.
> And there is an issue confirmed in local tests, so I leave additional
> comments.
> > >
> > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > > ---
> > > > drivers/gpu/drm/i915/display/intel_psr.c | 9 ---------
> > > > drivers/gpu/drm/i915/i915_params.h | 2 +-
> > > > 2 files changed, 1 insertion(+), 10 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c
> > > > index 894a2d35668a2..e128f0c2aeecc 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_psr.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> > > > @@ -877,15 +877,6 @@ static bool intel_psr2_config_valid(struct intel_dp *intel_dp,
> > > > return false;
> > > > }
> > > >
> > > > -/*
> > > > - * We are missing the implementation of some workarounds to enabled PSR2
> > > > - * in Alderlake_P, until ready PSR2 should be kept disabled.
> > > > - */
> > > > -if (IS_ALDERLAKE_P(dev_priv)) {
> > > > -drm_dbg_kms(&dev_priv->drm, "PSR2 is missing the implementation of workarounds\n");
> > > > -return false;
> > > > -}
> > > > -
> > > > if (!transcoder_has_psr2(dev_priv, crtc_state->cpu_transcoder)) {
> > > > drm_dbg_kms(&dev_priv->drm,
> > > > "PSR2 not supported in transcoder %s\n",
> > > > diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h
> > > > index f27eceb82c0f5..8d725b64592d8 100644
> > > > --- a/drivers/gpu/drm/i915/i915_params.h
> > > > +++ b/drivers/gpu/drm/i915/i915_params.h
> > > > @@ -55,7 +55,7 @@ struct drm_printer;
> > > > param(int, enable_fbc, -1, 0600) \
> > > > param(int, enable_psr, -1, 0600) \
> > > > param(bool, psr_safest_params, false, 0400) \
> > > > -param(bool, enable_psr2_sel_fetch, false, 0400) \
> > > > +param(bool, enable_psr2_sel_fetch, true, 0400) \
> If we do not modify this part and do not enable it by default at boot
> time as shown in the original code below,
> param(bool, enable_psr2_sel_fetch, false, 0400) \
>
> when we execute the kms_psr2_sf test case of igt, the FIFO underrun as
> below still occurs.
>
> i915 0000:00:02.0: [drm] *ERROR* CPU pipe A FIFO underrun: port,transcoder,
>
> When PSR2 panel is used, PSR1 is enabled by default when
> enable_psr2_sel_fetch is not enabled by default.
> And when kms_psr2_sf is executed, the mode is changed to PSR2, and when
> kms_psr2_sf is terminated, PSR2 is deactivated and PSR1 is re-enabled.
> At this point. I suspect there is a problem.
Was able to reproduce this even with enable_psr2_sel_fetch set to true.
Added some debug messages to intel_psr_exit() and intel_psr_activate() and those functions are not called and the underrun still happens.
Could be a regression recently introduced because I was not seeing this underrun a few weeks ago.
Anyways this underrun happens with and without(just doing the changes to allow PSR2 in alderlake-P in intel_psr2_config_valid()) this patches.
>
> > > > param(int, disable_power_well, -1, 0400) \
> > > > param(int, enable_ips, 1, 0600) \
> > > > param(int, invert_brightness, 0, 0600) \
> > > >
> >
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Intel-gfx] [PATCH 1/4] drm/i915/display/tgl+: Dispatch atomic commits instead of front buffer modifications
2021-07-31 0:10 [Intel-gfx] [PATCH 1/4] drm/i915/display/tgl+: Dispatch atomic commits instead of front buffer modifications José Roberto de Souza
` (4 preceding siblings ...)
2021-07-31 0:45 ` [Intel-gfx] ✗ Fi.CI.BAT: failure " Patchwork
@ 2021-08-12 17:24 ` Souza, Jose
2021-08-12 19:02 ` Daniel Vetter
5 siblings, 1 reply; 16+ messages in thread
From: Souza, Jose @ 2021-08-12 17:24 UTC (permalink / raw)
To: Vivi, Rodrigo, Nikula, Jani, intel-gfx
Cc: daniel, Mun, Gwan-gyeong, ville.syrjala
On Fri, 2021-07-30 at 17:10 -0700, José Roberto de Souza wrote:
> PSR2 selective fetch requires plane and transcoder registers to
> be programed during the vblank to properly update the display and
> there is no way around it.
>
> We could disable PSR2 at every notification of dirty front buffer from
> user space but that would hurt the power savings and it would still
> cause some race conditions between PSR2 exit sequence and atomic
> commits that causes underruns and glitches.
>
> So from display 12 and newer we will start to do atomic commits
> every time user space notify that front buffer is dirty and ignore
> all frontbuffer flushes and invalidates on the PSR side.
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Jani Nikula <jani.nikula@intel.com>
>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
> drivers/gpu/drm/i915/display/intel_cursor.c | 3 ++-
> drivers/gpu/drm/i915/display/intel_display.c | 7 ++++++-
> drivers/gpu/drm/i915/display/intel_psr.c | 6 ++++++
> 3 files changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_cursor.c b/drivers/gpu/drm/i915/display/intel_cursor.c
> index c7618fef01439..d44022cb46a65 100644
> --- a/drivers/gpu/drm/i915/display/intel_cursor.c
> +++ b/drivers/gpu/drm/i915/display/intel_cursor.c
> @@ -617,6 +617,7 @@ intel_legacy_cursor_update(struct drm_plane *_plane,
> u32 src_w, u32 src_h,
> struct drm_modeset_acquire_ctx *ctx)
> {
> + struct drm_i915_private *i915 = to_i915(_crtc->dev);
> struct intel_plane *plane = to_intel_plane(_plane);
> struct intel_crtc *crtc = to_intel_crtc(_crtc);
> struct intel_plane_state *old_plane_state =
> @@ -638,7 +639,7 @@ intel_legacy_cursor_update(struct drm_plane *_plane,
> */
> if (!crtc_state->hw.active || intel_crtc_needs_modeset(crtc_state) ||
> crtc_state->update_pipe || crtc_state->bigjoiner ||
> - crtc_state->enable_psr2_sel_fetch)
> + DISPLAY_VER(i915) >= 12)
> goto slow;
>
> /*
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index 5ff0a011b28eb..4a936e1e7fa82 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -11720,10 +11720,15 @@ static int intel_user_framebuffer_dirty(struct drm_framebuffer *fb,
> unsigned num_clips)
> {
> struct drm_i915_gem_object *obj = intel_fb_obj(fb);
> + struct drm_i915_private *i915 = to_i915(obj->base.dev);
>
> i915_gem_object_flush_if_display(obj);
> - intel_frontbuffer_flush(to_intel_frontbuffer(fb), ORIGIN_DIRTYFB);
>
> + if (DISPLAY_VER(i915) >= 12)
> + return drm_atomic_helper_dirtyfb(fb, file, flags, color, clips,
> + num_clips);
> +
> + intel_frontbuffer_flush(to_intel_frontbuffer(fb), ORIGIN_DIRTYFB);
> return 0;
> }
>
> diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c
> index 1b0daf649e823..caf92f414a6e7 100644
> --- a/drivers/gpu/drm/i915/display/intel_psr.c
> +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> @@ -2039,6 +2039,9 @@ void intel_psr_invalidate(struct drm_i915_private *dev_priv,
> {
> struct intel_encoder *encoder;
>
> + if (DISPLAY_VER(dev_priv) >= 12)
> + return;
> +
> if (origin == ORIGIN_FLIP)
> return;
>
> @@ -2123,6 +2126,9 @@ void intel_psr_flush(struct drm_i915_private *dev_priv,
> continue;
> }
>
> + if (DISPLAY_VER(dev_priv) >= 12)
> + continue;
> +
> mutex_lock(&intel_dp->psr.lock);
> if (!intel_dp->psr.enabled) {
> mutex_unlock(&intel_dp->psr.lock);
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Intel-gfx] [PATCH 1/4] drm/i915/display/tgl+: Dispatch atomic commits instead of front buffer modifications
2021-08-12 17:24 ` [Intel-gfx] [PATCH 1/4] " Souza, Jose
@ 2021-08-12 19:02 ` Daniel Vetter
0 siblings, 0 replies; 16+ messages in thread
From: Daniel Vetter @ 2021-08-12 19:02 UTC (permalink / raw)
To: Souza, Jose
Cc: Vivi, Rodrigo, Nikula, Jani, intel-gfx, Mun, Gwan-gyeong, ville.syrjala
On Thu, Aug 12, 2021 at 7:24 PM Souza, Jose <jose.souza@intel.com> wrote:
>
> On Fri, 2021-07-30 at 17:10 -0700, José Roberto de Souza wrote:
> > PSR2 selective fetch requires plane and transcoder registers to
> > be programed during the vblank to properly update the display and
> > there is no way around it.
> >
> > We could disable PSR2 at every notification of dirty front buffer from
> > user space but that would hurt the power savings and it would still
> > cause some race conditions between PSR2 exit sequence and atomic
> > commits that causes underruns and glitches.
> >
> > So from display 12 and newer we will start to do atomic commits
> > every time user space notify that front buffer is dirty and ignore
> > all frontbuffer flushes and invalidates on the PSR side.
So I filed a JIRA about this a while ago, and:
- This should be done on gen9 (or maybe gen10+), because we stopped
having userspace that was busted. Would be good to check whether we
can't push this down further even.
- We need to disable the entire intel_frontbuffer.c interface/uapi for
_all_ things. It's a horrendous uapi, and it needs to die.
You're already touching intel_user_framebuffer_dirty, so the other
pieces really should be done too. Maybe make a macro or function or
something so that we have a consistent check for which platforms still
have to support frontbuffer tracking.
-Daniel
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Jani Nikula <jani.nikula@intel.com>
>
> >
> > Cc: Daniel Vetter <daniel@ffwll.ch>
> > Cc: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > ---
> > drivers/gpu/drm/i915/display/intel_cursor.c | 3 ++-
> > drivers/gpu/drm/i915/display/intel_display.c | 7 ++++++-
> > drivers/gpu/drm/i915/display/intel_psr.c | 6 ++++++
> > 3 files changed, 14 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_cursor.c b/drivers/gpu/drm/i915/display/intel_cursor.c
> > index c7618fef01439..d44022cb46a65 100644
> > --- a/drivers/gpu/drm/i915/display/intel_cursor.c
> > +++ b/drivers/gpu/drm/i915/display/intel_cursor.c
> > @@ -617,6 +617,7 @@ intel_legacy_cursor_update(struct drm_plane *_plane,
> > u32 src_w, u32 src_h,
> > struct drm_modeset_acquire_ctx *ctx)
> > {
> > + struct drm_i915_private *i915 = to_i915(_crtc->dev);
> > struct intel_plane *plane = to_intel_plane(_plane);
> > struct intel_crtc *crtc = to_intel_crtc(_crtc);
> > struct intel_plane_state *old_plane_state =
> > @@ -638,7 +639,7 @@ intel_legacy_cursor_update(struct drm_plane *_plane,
> > */
> > if (!crtc_state->hw.active || intel_crtc_needs_modeset(crtc_state) ||
> > crtc_state->update_pipe || crtc_state->bigjoiner ||
> > - crtc_state->enable_psr2_sel_fetch)
> > + DISPLAY_VER(i915) >= 12)
> > goto slow;
> >
> > /*
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > index 5ff0a011b28eb..4a936e1e7fa82 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -11720,10 +11720,15 @@ static int intel_user_framebuffer_dirty(struct drm_framebuffer *fb,
> > unsigned num_clips)
> > {
> > struct drm_i915_gem_object *obj = intel_fb_obj(fb);
> > + struct drm_i915_private *i915 = to_i915(obj->base.dev);
> >
> > i915_gem_object_flush_if_display(obj);
> > - intel_frontbuffer_flush(to_intel_frontbuffer(fb), ORIGIN_DIRTYFB);
> >
> > + if (DISPLAY_VER(i915) >= 12)
> > + return drm_atomic_helper_dirtyfb(fb, file, flags, color, clips,
> > + num_clips);
> > +
> > + intel_frontbuffer_flush(to_intel_frontbuffer(fb), ORIGIN_DIRTYFB);
> > return 0;
> > }
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c
> > index 1b0daf649e823..caf92f414a6e7 100644
> > --- a/drivers/gpu/drm/i915/display/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> > @@ -2039,6 +2039,9 @@ void intel_psr_invalidate(struct drm_i915_private *dev_priv,
> > {
> > struct intel_encoder *encoder;
> >
> > + if (DISPLAY_VER(dev_priv) >= 12)
> > + return;
> > +
> > if (origin == ORIGIN_FLIP)
> > return;
> >
> > @@ -2123,6 +2126,9 @@ void intel_psr_flush(struct drm_i915_private *dev_priv,
> > continue;
> > }
> >
> > + if (DISPLAY_VER(dev_priv) >= 12)
> > + continue;
> > +
> > mutex_lock(&intel_dp->psr.lock);
> > if (!intel_dp->psr.enabled) {
> > mutex_unlock(&intel_dp->psr.lock);
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Intel-gfx] [PATCH 2/4] drm/i915/display: Fix sel fetch plane offset calculation
2021-07-31 0:10 ` [Intel-gfx] [PATCH 2/4] drm/i915/display: Fix sel fetch plane offset calculation José Roberto de Souza
@ 2021-08-13 7:40 ` Gwan-gyeong Mun
0 siblings, 0 replies; 16+ messages in thread
From: Gwan-gyeong Mun @ 2021-08-13 7:40 UTC (permalink / raw)
To: José Roberto de Souza, intel-gfx; +Cc: Ville Syrjälä
Reviewed-by: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com>
On 7/31/21 3:10 AM, José Roberto de Souza wrote:
> skl_calc_main_surface_offset() is used to calculate an aligned plane
> surface address considering the inner framebuffer x and y offset.
> It can not be used by selective fetch functions becase there is no
> PLANE_SEL_FETCH_SURF.
> So the PLANE_SEL_FETCH_OFFSET.y should only be PLANE_OFFSET.y +
> damaged_area_within_plane.y1.
>
> This fixes glitches seen in fbcon caused by typing something in
> the terminal.
>
> BSpec: 55229
> Cc: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
> drivers/gpu/drm/i915/display/intel_psr.c | 8 ++------
> 1 file changed, 2 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c
> index caf92f414a6e7..894a2d35668a2 100644
> --- a/drivers/gpu/drm/i915/display/intel_psr.c
> +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> @@ -1487,8 +1487,8 @@ void intel_psr2_program_plane_sel_fetch(struct intel_plane *plane,
> struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> enum pipe pipe = plane->pipe;
> const struct drm_rect *clip;
> - u32 val, offset;
> - int ret, x, y;
> + u32 val;
> + int x, y;
>
> if (!crtc_state->enable_psr2_sel_fetch)
> return;
> @@ -1508,10 +1508,6 @@ void intel_psr2_program_plane_sel_fetch(struct intel_plane *plane,
> /* TODO: consider auxiliary surfaces */
> x = plane_state->uapi.src.x1 >> 16;
> y = (plane_state->uapi.src.y1 >> 16) + clip->y1;
> - ret = skl_calc_main_surface_offset(plane_state, &x, &y, &offset);
> - if (ret)
> - drm_warn_once(&dev_priv->drm, "skl_calc_main_surface_offset() returned %i\n",
> - ret);
> val = y << 16 | x;
> intel_de_write_fw(dev_priv, PLANE_SEL_FETCH_OFFSET(pipe, plane->id),
> val);
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Intel-gfx] [PATCH 3/4] drm/i915: Nuke ORIGIN_GTT
2021-08-03 17:19 ` Souza, Jose
@ 2021-08-13 7:41 ` Gwan-gyeong Mun
0 siblings, 0 replies; 16+ messages in thread
From: Gwan-gyeong Mun @ 2021-08-13 7:41 UTC (permalink / raw)
To: Souza, Jose, intel-gfx
Reviewed-by: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com>
On 8/3/21 8:19 PM, Souza, Jose wrote:
> On Tue, 2021-08-03 at 14:20 +0300, Gwan-gyeong Mun wrote:
>>
>> On 7/31/21 3:10 AM, José Roberto de Souza wrote:
>>> There is no users of it, so no need to keep handling for it.
>>>
>>> Cc: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com>
>>> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
>>> ---
>>> drivers/gpu/drm/i915/display/intel_fbc.c | 10 +---------
>>> drivers/gpu/drm/i915/display/intel_frontbuffer.h | 3 +--
>>> 2 files changed, 2 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c b/drivers/gpu/drm/i915/display/intel_fbc.c
>>> index ddfc17e21668a..e4d412d395c34 100644
>>> --- a/drivers/gpu/drm/i915/display/intel_fbc.c
>>> +++ b/drivers/gpu/drm/i915/display/intel_fbc.c
>>> @@ -1129,7 +1129,7 @@ void intel_fbc_invalidate(struct drm_i915_private *dev_priv,
>>> if (!HAS_FBC(dev_priv))
>>> return;
>>>
>>> -if (origin == ORIGIN_GTT || origin == ORIGIN_FLIP)
>>> +if (origin == ORIGIN_FLIP)
>>> return;
>>>
>>> mutex_lock(&fbc->lock);
>>> @@ -1150,14 +1150,6 @@ void intel_fbc_flush(struct drm_i915_private *dev_priv,
>>> if (!HAS_FBC(dev_priv))
>>> return;
>>>
>>> -/*
>>> - * GTT tracking does not nuke the entire cfb
>>> - * so don't clear busy_bits set for some other
>>> - * reason.
>>> - */
>>> -if (origin == ORIGIN_GTT)
>>> -return;
>>> -
>>> mutex_lock(&fbc->lock);
>>>
>>> fbc->busy_bits &= ~frontbuffer_bits;
>>> diff --git a/drivers/gpu/drm/i915/display/intel_frontbuffer.h b/drivers/gpu/drm/i915/display/intel_frontbuffer.h
>>> index 6d41f53944250..4b977c1e4d52b 100644
>>> --- a/drivers/gpu/drm/i915/display/intel_frontbuffer.h
>>> +++ b/drivers/gpu/drm/i915/display/intel_frontbuffer.h
>>> @@ -33,8 +33,7 @@
>>> struct drm_i915_private;
>>>
>>> enum fb_op_origin {
>>> -ORIGIN_GTT,
>>> -ORIGIN_CPU,
>>> +ORIGIN_CPU = 0,
>>> ORIGIN_CS,
>>> ORIGIN_FLIP,
>>> ORIGIN_DIRTYFB,
>>>
>> Is this patch absolutely necessary for this series?
>
> Nope, just notice this unused ORIGIN_GTT while debugging.
>
^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2021-08-13 7:41 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-31 0:10 [Intel-gfx] [PATCH 1/4] drm/i915/display/tgl+: Dispatch atomic commits instead of front buffer modifications José Roberto de Souza
2021-07-31 0:10 ` [Intel-gfx] [PATCH 2/4] drm/i915/display: Fix sel fetch plane offset calculation José Roberto de Souza
2021-08-13 7:40 ` Gwan-gyeong Mun
2021-07-31 0:10 ` [Intel-gfx] [PATCH 3/4] drm/i915: Nuke ORIGIN_GTT José Roberto de Souza
2021-08-03 11:20 ` Gwan-gyeong Mun
2021-08-03 17:19 ` Souza, Jose
2021-08-13 7:41 ` Gwan-gyeong Mun
2021-07-31 0:10 ` [Intel-gfx] [PATCH 4/4] DO_NOT_MERGE: drm/i915/display: Enable PSR2 selective fetch by default José Roberto de Souza
2021-08-03 11:17 ` Gwan-gyeong Mun
2021-08-03 17:18 ` Souza, Jose
2021-08-05 18:26 ` Gwan-gyeong Mun
2021-08-05 21:47 ` Souza, Jose
2021-07-31 0:14 ` [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for series starting with [1/4] drm/i915/display/tgl+: Dispatch atomic commits instead of front buffer modifications Patchwork
2021-07-31 0:45 ` [Intel-gfx] ✗ Fi.CI.BAT: failure " Patchwork
2021-08-12 17:24 ` [Intel-gfx] [PATCH 1/4] " Souza, Jose
2021-08-12 19:02 ` Daniel Vetter
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).