intel-gfx.lists.freedesktop.org archive mirror
 help / color / mirror / Atom feed
* [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).