All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/2] drm/i915: Do not rely on wm preservation for ILK watermarks
@ 2017-10-19 15:13 Maarten Lankhorst
  2017-10-19 15:13 ` [PATCH 2/2] drm/i915: Calculate ironlake intermediate watermarks correctly, v2 Maarten Lankhorst
                   ` (6 more replies)
  0 siblings, 7 replies; 12+ messages in thread
From: Maarten Lankhorst @ 2017-10-19 15:13 UTC (permalink / raw)
  To: intel-gfx

The original intent was to preserve watermarks as much as possible
in intel_pipe_wm.raw_wm, and put the validated ones in intel_pipe_wm.wm.

It seems this approach is insufficient and we don't always preserve
the raw watermarks, so just use the atomic iterator we're already using
to get a const pointer to all bound planes on the crtc.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=102373
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: stable@vger.kernel.org #v4.8+
---
 drivers/gpu/drm/i915/intel_drv.h |  1 -
 drivers/gpu/drm/i915/intel_pm.c  | 51 +++++++++++++++++-----------------------
 2 files changed, 21 insertions(+), 31 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 47d022d48718..7bc60c848940 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -499,7 +499,6 @@ struct intel_crtc_scaler_state {
 
 struct intel_pipe_wm {
 	struct intel_wm_level wm[5];
-	struct intel_wm_level raw_wm[5];
 	uint32_t linetime;
 	bool fbc_wm_enabled;
 	bool pipe_enabled;
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index c42a65a93b3a..ea70c720f492 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -2721,9 +2721,9 @@ static void ilk_compute_wm_level(const struct drm_i915_private *dev_priv,
 				 const struct intel_crtc *intel_crtc,
 				 int level,
 				 struct intel_crtc_state *cstate,
-				 struct intel_plane_state *pristate,
-				 struct intel_plane_state *sprstate,
-				 struct intel_plane_state *curstate,
+				 const struct intel_plane_state *pristate,
+				 const struct intel_plane_state *sprstate,
+				 const struct intel_plane_state *curstate,
 				 struct intel_wm_level *result)
 {
 	uint16_t pri_latency = dev_priv->wm.pri_latency[level];
@@ -3043,28 +3043,24 @@ static int ilk_compute_pipe_wm(struct intel_crtc_state *cstate)
 	struct intel_pipe_wm *pipe_wm;
 	struct drm_device *dev = state->dev;
 	const struct drm_i915_private *dev_priv = to_i915(dev);
-	struct intel_plane *intel_plane;
-	struct intel_plane_state *pristate = NULL;
-	struct intel_plane_state *sprstate = NULL;
-	struct intel_plane_state *curstate = NULL;
+	struct drm_plane *plane;
+	const struct drm_plane_state *plane_state;
+	const struct intel_plane_state *pristate = NULL;
+	const struct intel_plane_state *sprstate = NULL;
+	const struct intel_plane_state *curstate = NULL;
 	int level, max_level = ilk_wm_max_level(dev_priv), usable_level;
 	struct ilk_wm_maximums max;
 
 	pipe_wm = &cstate->wm.ilk.optimal;
 
-	for_each_intel_plane_on_crtc(dev, intel_crtc, intel_plane) {
-		struct intel_plane_state *ps;
-
-		ps = intel_atomic_get_existing_plane_state(state,
-							   intel_plane);
-		if (!ps)
-			continue;
+	drm_atomic_crtc_state_for_each_plane_state(plane, plane_state, &cstate->base) {
+		const struct intel_plane_state *ps = to_intel_plane_state(plane_state);
 
-		if (intel_plane->base.type == DRM_PLANE_TYPE_PRIMARY)
+		if (plane->type == DRM_PLANE_TYPE_PRIMARY)
 			pristate = ps;
-		else if (intel_plane->base.type == DRM_PLANE_TYPE_OVERLAY)
+		else if (plane->type == DRM_PLANE_TYPE_OVERLAY)
 			sprstate = ps;
-		else if (intel_plane->base.type == DRM_PLANE_TYPE_CURSOR)
+		else if (plane->type == DRM_PLANE_TYPE_CURSOR)
 			curstate = ps;
 	}
 
@@ -3086,11 +3082,9 @@ static int ilk_compute_pipe_wm(struct intel_crtc_state *cstate)
 	if (pipe_wm->sprites_scaled)
 		usable_level = 0;
 
-	ilk_compute_wm_level(dev_priv, intel_crtc, 0, cstate,
-			     pristate, sprstate, curstate, &pipe_wm->raw_wm[0]);
-
 	memset(&pipe_wm->wm, 0, sizeof(pipe_wm->wm));
-	pipe_wm->wm[0] = pipe_wm->raw_wm[0];
+	ilk_compute_wm_level(dev_priv, intel_crtc, 0, cstate,
+			     pristate, sprstate, curstate, &pipe_wm->wm[0]);
 
 	if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv))
 		pipe_wm->linetime = hsw_compute_linetime_wm(cstate);
@@ -3100,8 +3094,8 @@ static int ilk_compute_pipe_wm(struct intel_crtc_state *cstate)
 
 	ilk_compute_wm_reg_maximums(dev_priv, 1, &max);
 
-	for (level = 1; level <= max_level; level++) {
-		struct intel_wm_level *wm = &pipe_wm->raw_wm[level];
+	for (level = 1; level <= usable_level; level++) {
+		struct intel_wm_level *wm = &pipe_wm->wm[level];
 
 		ilk_compute_wm_level(dev_priv, intel_crtc, level, cstate,
 				     pristate, sprstate, curstate, wm);
@@ -3111,13 +3105,10 @@ static int ilk_compute_pipe_wm(struct intel_crtc_state *cstate)
 		 * register maximums since such watermarks are
 		 * always invalid.
 		 */
-		if (level > usable_level)
-			continue;
-
-		if (ilk_validate_wm_level(level, &max, wm))
-			pipe_wm->wm[level] = *wm;
-		else
-			usable_level = level;
+		if (!ilk_validate_wm_level(level, &max, wm)) {
+			memset(wm, 0, sizeof(*wm));
+			break;
+		}
 	}
 
 	return 0;
-- 
2.14.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 2/2] drm/i915: Calculate ironlake intermediate watermarks correctly, v2.
  2017-10-19 15:13 [PATCH 1/2] drm/i915: Do not rely on wm preservation for ILK watermarks Maarten Lankhorst
@ 2017-10-19 15:13 ` Maarten Lankhorst
  2017-10-24 23:10   ` Matt Roper
  2017-10-19 15:54 ` ✗ Fi.CI.BAT: warning for series starting with [1/2] drm/i915: Do not rely on wm preservation for ILK watermarks Patchwork
                   ` (5 subsequent siblings)
  6 siblings, 1 reply; 12+ messages in thread
From: Maarten Lankhorst @ 2017-10-19 15:13 UTC (permalink / raw)
  To: intel-gfx

The watermarks it should calculate against are the old optimal watermarks.
The currently active crtc watermarks are pure fiction, and are invalid in
case of a nonblocking modeset, page flip enabling/disabling planes or any
other reason.

When the crtc is disabled or during a modeset the intermediate watermarks
don't need to be programmed separately, and could be directly assigned
to the optimal watermarks.

Changes since v1:
- Use intel_atomic_get_old_crtc_state. (ville)

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index ea70c720f492..e181dfc36200 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3124,7 +3124,11 @@ static int ilk_compute_intermediate_wm(struct drm_device *dev,
 				       struct intel_crtc_state *newstate)
 {
 	struct intel_pipe_wm *a = &newstate->wm.ilk.intermediate;
-	struct intel_pipe_wm *b = &intel_crtc->wm.active.ilk;
+	struct intel_atomic_state *intel_state =
+		to_intel_atomic_state(newstate->base.state);
+	const struct intel_crtc_state *oldstate =
+		intel_atomic_get_old_crtc_state(intel_state, intel_crtc);
+	const struct intel_pipe_wm *b = &oldstate->wm.ilk.optimal;
 	int level, max_level = ilk_wm_max_level(to_i915(dev));
 
 	/*
@@ -3133,6 +3137,9 @@ static int ilk_compute_intermediate_wm(struct drm_device *dev,
 	 * and after the vblank.
 	 */
 	*a = newstate->wm.ilk.optimal;
+	if (!newstate->base.active || drm_atomic_crtc_needs_modeset(&newstate->base))
+		return 0;
+
 	a->pipe_enabled |= b->pipe_enabled;
 	a->sprites_enabled |= b->sprites_enabled;
 	a->sprites_scaled |= b->sprites_scaled;
-- 
2.14.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* ✗ Fi.CI.BAT: warning for series starting with [1/2] drm/i915: Do not rely on wm preservation for ILK watermarks
  2017-10-19 15:13 [PATCH 1/2] drm/i915: Do not rely on wm preservation for ILK watermarks Maarten Lankhorst
  2017-10-19 15:13 ` [PATCH 2/2] drm/i915: Calculate ironlake intermediate watermarks correctly, v2 Maarten Lankhorst
@ 2017-10-19 15:54 ` Patchwork
  2017-10-19 17:59 ` ✓ Fi.CI.BAT: success " Patchwork
                   ` (4 subsequent siblings)
  6 siblings, 0 replies; 12+ messages in thread
From: Patchwork @ 2017-10-19 15:54 UTC (permalink / raw)
  To: Maarten Lankhorst; +Cc: intel-gfx

== Series Details ==

Series: series starting with [1/2] drm/i915: Do not rely on wm preservation for ILK watermarks
URL   : https://patchwork.freedesktop.org/series/32301/
State : warning

== Summary ==

Series 32301v1 series starting with [1/2] drm/i915: Do not rely on wm preservation for ILK watermarks
https://patchwork.freedesktop.org/api/1.0/series/32301/revisions/1/mbox/

Test kms_busy:
        Subgroup basic-flip-b:
                fail       -> PASS       (fi-gdg-551) fdo#102654
Test kms_cursor_legacy:
        Subgroup basic-busy-flip-before-cursor-legacy:
                fail       -> PASS       (fi-gdg-551) fdo#102618
        Subgroup basic-flip-after-cursor-varying-size:
                incomplete -> PASS       (fi-skl-6260u)
Test kms_flip:
        Subgroup basic-flip-vs-dpms:
                pass       -> WARN       (fi-hsw-4770)
                pass       -> INCOMPLETE (fi-skl-6700hq) fdo#103124

fdo#102654 https://bugs.freedesktop.org/show_bug.cgi?id=102654
fdo#102618 https://bugs.freedesktop.org/show_bug.cgi?id=102618
fdo#103124 https://bugs.freedesktop.org/show_bug.cgi?id=103124

fi-bdw-5557u     total:289  pass:268  dwarn:0   dfail:0   fail:0   skip:21  time:441s
fi-bdw-gvtdvm    total:289  pass:265  dwarn:0   dfail:0   fail:0   skip:24  time:458s
fi-blb-e6850     total:289  pass:223  dwarn:1   dfail:0   fail:0   skip:65  time:374s
fi-bsw-n3050     total:289  pass:243  dwarn:0   dfail:0   fail:0   skip:46  time:528s
fi-bwr-2160      total:289  pass:183  dwarn:0   dfail:0   fail:0   skip:106 time:262s
fi-bxt-dsi       total:289  pass:259  dwarn:0   dfail:0   fail:0   skip:30  time:493s
fi-bxt-j4205     total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:502s
fi-byt-j1900     total:289  pass:253  dwarn:1   dfail:0   fail:0   skip:35  time:492s
fi-byt-n2820     total:289  pass:249  dwarn:1   dfail:0   fail:0   skip:39  time:474s
fi-cfl-s         total:289  pass:253  dwarn:4   dfail:0   fail:0   skip:32  time:558s
fi-elk-e7500     total:289  pass:229  dwarn:0   dfail:0   fail:0   skip:60  time:420s
fi-gdg-551       total:289  pass:178  dwarn:1   dfail:0   fail:1   skip:109 time:250s
fi-glk-1         total:289  pass:261  dwarn:0   dfail:0   fail:0   skip:28  time:580s
fi-hsw-4770      total:289  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:449s
fi-hsw-4770r     total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:428s
fi-ilk-650       total:289  pass:228  dwarn:0   dfail:0   fail:0   skip:61  time:430s
fi-ivb-3520m     total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:489s
fi-ivb-3770      total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:457s
fi-kbl-7500u     total:289  pass:264  dwarn:1   dfail:0   fail:0   skip:24  time:485s
fi-kbl-7560u     total:289  pass:270  dwarn:0   dfail:0   fail:0   skip:19  time:566s
fi-kbl-7567u     total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:476s
fi-kbl-r         total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:583s
fi-pnv-d510      total:289  pass:222  dwarn:1   dfail:0   fail:0   skip:66  time:548s
fi-skl-6260u     total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:445s
fi-skl-6700hq    total:217  pass:196  dwarn:0   dfail:0   fail:0   skip:20 
fi-skl-6700k     total:289  pass:265  dwarn:0   dfail:0   fail:0   skip:24  time:515s
fi-skl-6770hq    total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:497s
fi-skl-gvtdvm    total:289  pass:266  dwarn:0   dfail:0   fail:0   skip:23  time:458s
fi-snb-2520m     total:289  pass:250  dwarn:0   dfail:0   fail:0   skip:39  time:569s
fi-snb-2600      total:289  pass:249  dwarn:0   dfail:0   fail:0   skip:40  time:428s

9024f1a2827aa921560ee3f985a5b418ef296435 drm-tip: 2017y-10m-19d-12h-57m-03s UTC integration manifest
10ebfdb43f85 drm/i915: Calculate ironlake intermediate watermarks correctly, v2.
ce0e2864d502 drm/i915: Do not rely on wm preservation for ILK watermarks

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_6105/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 12+ messages in thread

* ✓ Fi.CI.BAT: success for series starting with [1/2] drm/i915: Do not rely on wm preservation for ILK watermarks
  2017-10-19 15:13 [PATCH 1/2] drm/i915: Do not rely on wm preservation for ILK watermarks Maarten Lankhorst
  2017-10-19 15:13 ` [PATCH 2/2] drm/i915: Calculate ironlake intermediate watermarks correctly, v2 Maarten Lankhorst
  2017-10-19 15:54 ` ✗ Fi.CI.BAT: warning for series starting with [1/2] drm/i915: Do not rely on wm preservation for ILK watermarks Patchwork
@ 2017-10-19 17:59 ` Patchwork
  2017-10-19 18:48 ` ✓ Fi.CI.IGT: " Patchwork
                   ` (3 subsequent siblings)
  6 siblings, 0 replies; 12+ messages in thread
From: Patchwork @ 2017-10-19 17:59 UTC (permalink / raw)
  To: Maarten Lankhorst; +Cc: intel-gfx

== Series Details ==

Series: series starting with [1/2] drm/i915: Do not rely on wm preservation for ILK watermarks
URL   : https://patchwork.freedesktop.org/series/32301/
State : success

== Summary ==

Series 32301v1 series starting with [1/2] drm/i915: Do not rely on wm preservation for ILK watermarks
https://patchwork.freedesktop.org/api/1.0/series/32301/revisions/1/mbox/

Test kms_busy:
        Subgroup basic-flip-b:
                fail       -> PASS       (fi-gdg-551) fdo#102654
Test kms_cursor_legacy:
        Subgroup basic-busy-flip-before-cursor-legacy:
                fail       -> PASS       (fi-gdg-551) fdo#102618
        Subgroup basic-flip-after-cursor-varying-size:
                incomplete -> PASS       (fi-skl-6260u)

fdo#102654 https://bugs.freedesktop.org/show_bug.cgi?id=102654
fdo#102618 https://bugs.freedesktop.org/show_bug.cgi?id=102618

fi-bdw-5557u     total:289  pass:268  dwarn:0   dfail:0   fail:0   skip:21  time:440s
fi-bdw-gvtdvm    total:289  pass:265  dwarn:0   dfail:0   fail:0   skip:24  time:453s
fi-blb-e6850     total:289  pass:223  dwarn:1   dfail:0   fail:0   skip:65  time:373s
fi-bsw-n3050     total:289  pass:243  dwarn:0   dfail:0   fail:0   skip:46  time:519s
fi-bwr-2160      total:289  pass:183  dwarn:0   dfail:0   fail:0   skip:106 time:267s
fi-bxt-dsi       total:289  pass:259  dwarn:0   dfail:0   fail:0   skip:30  time:497s
fi-bxt-j4205     total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:499s
fi-byt-j1900     total:289  pass:253  dwarn:1   dfail:0   fail:0   skip:35  time:497s
fi-byt-n2820     total:289  pass:249  dwarn:1   dfail:0   fail:0   skip:39  time:489s
fi-cfl-s         total:289  pass:253  dwarn:4   dfail:0   fail:0   skip:32  time:561s
fi-elk-e7500     total:289  pass:229  dwarn:0   dfail:0   fail:0   skip:60  time:419s
fi-gdg-551       total:289  pass:178  dwarn:1   dfail:0   fail:1   skip:109 time:249s
fi-glk-1         total:289  pass:261  dwarn:0   dfail:0   fail:0   skip:28  time:578s
fi-hsw-4770      total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:450s
fi-hsw-4770r     total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:427s
fi-ilk-650       total:289  pass:228  dwarn:0   dfail:0   fail:0   skip:61  time:434s
fi-ivb-3520m     total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:497s
fi-ivb-3770      total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:461s
fi-kbl-7500u     total:289  pass:264  dwarn:1   dfail:0   fail:0   skip:24  time:489s
fi-kbl-7560u     total:289  pass:270  dwarn:0   dfail:0   fail:0   skip:19  time:580s
fi-kbl-7567u     total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:478s
fi-kbl-r         total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:584s
fi-pnv-d510      total:289  pass:222  dwarn:1   dfail:0   fail:0   skip:66  time:549s
fi-skl-6260u     total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:459s
fi-skl-6700hq    total:289  pass:263  dwarn:0   dfail:0   fail:0   skip:26  time:646s
fi-skl-6700k     total:289  pass:265  dwarn:0   dfail:0   fail:0   skip:24  time:515s
fi-skl-6770hq    total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:507s
fi-skl-gvtdvm    total:289  pass:266  dwarn:0   dfail:0   fail:0   skip:23  time:462s
fi-snb-2520m     total:289  pass:250  dwarn:0   dfail:0   fail:0   skip:39  time:566s
fi-snb-2600      total:289  pass:249  dwarn:0   dfail:0   fail:0   skip:40  time:418s

9024f1a2827aa921560ee3f985a5b418ef296435 drm-tip: 2017y-10m-19d-12h-57m-03s UTC integration manifest
7a40e80cb14f drm/i915: Calculate ironlake intermediate watermarks correctly, v2.
a8cc9f2f099f drm/i915: Do not rely on wm preservation for ILK watermarks

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_6108/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 12+ messages in thread

* ✓ Fi.CI.IGT: success for series starting with [1/2] drm/i915: Do not rely on wm preservation for ILK watermarks
  2017-10-19 15:13 [PATCH 1/2] drm/i915: Do not rely on wm preservation for ILK watermarks Maarten Lankhorst
                   ` (2 preceding siblings ...)
  2017-10-19 17:59 ` ✓ Fi.CI.BAT: success " Patchwork
@ 2017-10-19 18:48 ` Patchwork
  2017-10-24 23:01 ` [PATCH 1/2] " Matt Roper
                   ` (2 subsequent siblings)
  6 siblings, 0 replies; 12+ messages in thread
From: Patchwork @ 2017-10-19 18:48 UTC (permalink / raw)
  To: Maarten Lankhorst; +Cc: intel-gfx

== Series Details ==

Series: series starting with [1/2] drm/i915: Do not rely on wm preservation for ILK watermarks
URL   : https://patchwork.freedesktop.org/series/32301/
State : success

== Summary ==

Test kms_flip:
        Subgroup plain-flip-fb-recreate-interruptible:
                pass       -> FAIL       (shard-hsw) fdo#100368
        Subgroup flip-vs-expired-vblank-interruptible:
                pass       -> FAIL       (shard-hsw) fdo#102887
Test kms_busy:
        Subgroup extended-modeset-hang-oldfb-with-reset-render-B:
                dmesg-warn -> PASS       (shard-hsw) fdo#102249 +1
        Subgroup extended-modeset-hang-newfb-with-reset-render-B:
                pass       -> DMESG-WARN (shard-hsw) fdo#103038

fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
fdo#102887 https://bugs.freedesktop.org/show_bug.cgi?id=102887
fdo#102249 https://bugs.freedesktop.org/show_bug.cgi?id=102249
fdo#103038 https://bugs.freedesktop.org/show_bug.cgi?id=103038

shard-hsw        total:2540 pass:1426 dwarn:3   dfail:0   fail:10  skip:1101 time:9178s

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_6108/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/2] drm/i915: Do not rely on wm preservation for ILK watermarks
  2017-10-19 15:13 [PATCH 1/2] drm/i915: Do not rely on wm preservation for ILK watermarks Maarten Lankhorst
                   ` (3 preceding siblings ...)
  2017-10-19 18:48 ` ✓ Fi.CI.IGT: " Patchwork
@ 2017-10-24 23:01 ` Matt Roper
  2017-10-25  6:03   ` Maarten Lankhorst
  2017-10-25 10:23 ` Ville Syrjälä
  2017-10-27  8:03 ` ✗ Fi.CI.BAT: failure for series starting with [1/2] " Patchwork
  6 siblings, 1 reply; 12+ messages in thread
From: Matt Roper @ 2017-10-24 23:01 UTC (permalink / raw)
  To: Maarten Lankhorst; +Cc: intel-gfx

On Thu, Oct 19, 2017 at 05:13:40PM +0200, Maarten Lankhorst wrote:
> The original intent was to preserve watermarks as much as possible
> in intel_pipe_wm.raw_wm, and put the validated ones in intel_pipe_wm.wm.
> 
> It seems this approach is insufficient and we don't always preserve
> the raw watermarks, so just use the atomic iterator we're already using
> to get a const pointer to all bound planes on the crtc.
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=102373
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: stable@vger.kernel.org #v4.8+

It's been a while since I looked at this code, so I'm not sure I'm
following all the context/history here.  Before this patch, we had
calculated watermarks for all levels (even those above the maximum
usable watermark level as determined by sprite usage) into raw_wm.  But
as far as I can tell, we never actually used those values for anything
so that was no different than just throwing the values away?

Is my understanding correct that this is mostly a revert of 71f0a62614
("drm/i915: Only use sanitized values for ILK watermarks) except that it
also modifies the level validation loop so that we're only calling
ilk_compute_wm_level() on the levels up to usable_level whereas before
that patch we were calling it on all levels, but then
setting wm->enable = false for the unusable levels?

By my understanding, this looks like a safe simplification of the
current logic, but I don't see where the functional change is here.
Was the Buzilla: reference supposed to be tied to patch #2 of this
series or am I missing something important?


Matt

> ---
>  drivers/gpu/drm/i915/intel_drv.h |  1 -
>  drivers/gpu/drm/i915/intel_pm.c  | 51 +++++++++++++++++-----------------------
>  2 files changed, 21 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 47d022d48718..7bc60c848940 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -499,7 +499,6 @@ struct intel_crtc_scaler_state {
>  
>  struct intel_pipe_wm {
>  	struct intel_wm_level wm[5];
> -	struct intel_wm_level raw_wm[5];
>  	uint32_t linetime;
>  	bool fbc_wm_enabled;
>  	bool pipe_enabled;
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index c42a65a93b3a..ea70c720f492 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -2721,9 +2721,9 @@ static void ilk_compute_wm_level(const struct drm_i915_private *dev_priv,
>  				 const struct intel_crtc *intel_crtc,
>  				 int level,
>  				 struct intel_crtc_state *cstate,
> -				 struct intel_plane_state *pristate,
> -				 struct intel_plane_state *sprstate,
> -				 struct intel_plane_state *curstate,
> +				 const struct intel_plane_state *pristate,
> +				 const struct intel_plane_state *sprstate,
> +				 const struct intel_plane_state *curstate,
>  				 struct intel_wm_level *result)
>  {
>  	uint16_t pri_latency = dev_priv->wm.pri_latency[level];
> @@ -3043,28 +3043,24 @@ static int ilk_compute_pipe_wm(struct intel_crtc_state *cstate)
>  	struct intel_pipe_wm *pipe_wm;
>  	struct drm_device *dev = state->dev;
>  	const struct drm_i915_private *dev_priv = to_i915(dev);
> -	struct intel_plane *intel_plane;
> -	struct intel_plane_state *pristate = NULL;
> -	struct intel_plane_state *sprstate = NULL;
> -	struct intel_plane_state *curstate = NULL;
> +	struct drm_plane *plane;
> +	const struct drm_plane_state *plane_state;
> +	const struct intel_plane_state *pristate = NULL;
> +	const struct intel_plane_state *sprstate = NULL;
> +	const struct intel_plane_state *curstate = NULL;
>  	int level, max_level = ilk_wm_max_level(dev_priv), usable_level;
>  	struct ilk_wm_maximums max;
>  
>  	pipe_wm = &cstate->wm.ilk.optimal;
>  
> -	for_each_intel_plane_on_crtc(dev, intel_crtc, intel_plane) {
> -		struct intel_plane_state *ps;
> -
> -		ps = intel_atomic_get_existing_plane_state(state,
> -							   intel_plane);
> -		if (!ps)
> -			continue;
> +	drm_atomic_crtc_state_for_each_plane_state(plane, plane_state, &cstate->base) {
> +		const struct intel_plane_state *ps = to_intel_plane_state(plane_state);
>  
> -		if (intel_plane->base.type == DRM_PLANE_TYPE_PRIMARY)
> +		if (plane->type == DRM_PLANE_TYPE_PRIMARY)
>  			pristate = ps;
> -		else if (intel_plane->base.type == DRM_PLANE_TYPE_OVERLAY)
> +		else if (plane->type == DRM_PLANE_TYPE_OVERLAY)
>  			sprstate = ps;
> -		else if (intel_plane->base.type == DRM_PLANE_TYPE_CURSOR)
> +		else if (plane->type == DRM_PLANE_TYPE_CURSOR)
>  			curstate = ps;
>  	}
>  
> @@ -3086,11 +3082,9 @@ static int ilk_compute_pipe_wm(struct intel_crtc_state *cstate)
>  	if (pipe_wm->sprites_scaled)
>  		usable_level = 0;
>  
> -	ilk_compute_wm_level(dev_priv, intel_crtc, 0, cstate,
> -			     pristate, sprstate, curstate, &pipe_wm->raw_wm[0]);
> -
>  	memset(&pipe_wm->wm, 0, sizeof(pipe_wm->wm));
> -	pipe_wm->wm[0] = pipe_wm->raw_wm[0];
> +	ilk_compute_wm_level(dev_priv, intel_crtc, 0, cstate,
> +			     pristate, sprstate, curstate, &pipe_wm->wm[0]);
>  
>  	if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv))
>  		pipe_wm->linetime = hsw_compute_linetime_wm(cstate);
> @@ -3100,8 +3094,8 @@ static int ilk_compute_pipe_wm(struct intel_crtc_state *cstate)
>  
>  	ilk_compute_wm_reg_maximums(dev_priv, 1, &max);
>  
> -	for (level = 1; level <= max_level; level++) {
> -		struct intel_wm_level *wm = &pipe_wm->raw_wm[level];
> +	for (level = 1; level <= usable_level; level++) {
> +		struct intel_wm_level *wm = &pipe_wm->wm[level];
>  
>  		ilk_compute_wm_level(dev_priv, intel_crtc, level, cstate,
>  				     pristate, sprstate, curstate, wm);
> @@ -3111,13 +3105,10 @@ static int ilk_compute_pipe_wm(struct intel_crtc_state *cstate)
>  		 * register maximums since such watermarks are
>  		 * always invalid.
>  		 */
> -		if (level > usable_level)
> -			continue;
> -
> -		if (ilk_validate_wm_level(level, &max, wm))
> -			pipe_wm->wm[level] = *wm;
> -		else
> -			usable_level = level;
> +		if (!ilk_validate_wm_level(level, &max, wm)) {
> +			memset(wm, 0, sizeof(*wm));
> +			break;
> +		}
>  	}
>  
>  	return 0;
> -- 
> 2.14.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/2] drm/i915: Calculate ironlake intermediate watermarks correctly, v2.
  2017-10-19 15:13 ` [PATCH 2/2] drm/i915: Calculate ironlake intermediate watermarks correctly, v2 Maarten Lankhorst
@ 2017-10-24 23:10   ` Matt Roper
  0 siblings, 0 replies; 12+ messages in thread
From: Matt Roper @ 2017-10-24 23:10 UTC (permalink / raw)
  To: Maarten Lankhorst; +Cc: intel-gfx

On Thu, Oct 19, 2017 at 05:13:41PM +0200, Maarten Lankhorst wrote:
> The watermarks it should calculate against are the old optimal watermarks.
> The currently active crtc watermarks are pure fiction, and are invalid in
> case of a nonblocking modeset, page flip enabling/disabling planes or any
> other reason.
> 
> When the crtc is disabled or during a modeset the intermediate watermarks
> don't need to be programmed separately, and could be directly assigned
> to the optimal watermarks.
> 
> Changes since v1:
> - Use intel_atomic_get_old_crtc_state. (ville)
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Reviewed-by: Matt Roper <matthew.d.roper@intel.com>

> ---
>  drivers/gpu/drm/i915/intel_pm.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index ea70c720f492..e181dfc36200 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3124,7 +3124,11 @@ static int ilk_compute_intermediate_wm(struct drm_device *dev,
>  				       struct intel_crtc_state *newstate)
>  {
>  	struct intel_pipe_wm *a = &newstate->wm.ilk.intermediate;
> -	struct intel_pipe_wm *b = &intel_crtc->wm.active.ilk;
> +	struct intel_atomic_state *intel_state =
> +		to_intel_atomic_state(newstate->base.state);
> +	const struct intel_crtc_state *oldstate =
> +		intel_atomic_get_old_crtc_state(intel_state, intel_crtc);
> +	const struct intel_pipe_wm *b = &oldstate->wm.ilk.optimal;
>  	int level, max_level = ilk_wm_max_level(to_i915(dev));
>  
>  	/*
> @@ -3133,6 +3137,9 @@ static int ilk_compute_intermediate_wm(struct drm_device *dev,
>  	 * and after the vblank.
>  	 */
>  	*a = newstate->wm.ilk.optimal;
> +	if (!newstate->base.active || drm_atomic_crtc_needs_modeset(&newstate->base))
> +		return 0;
> +
>  	a->pipe_enabled |= b->pipe_enabled;
>  	a->sprites_enabled |= b->sprites_enabled;
>  	a->sprites_scaled |= b->sprites_scaled;
> -- 
> 2.14.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/2] drm/i915: Do not rely on wm preservation for ILK watermarks
  2017-10-24 23:01 ` [PATCH 1/2] " Matt Roper
@ 2017-10-25  6:03   ` Maarten Lankhorst
  2017-10-27  1:09     ` Matt Roper
  0 siblings, 1 reply; 12+ messages in thread
From: Maarten Lankhorst @ 2017-10-25  6:03 UTC (permalink / raw)
  To: Matt Roper; +Cc: intel-gfx

Op 25-10-17 om 01:01 schreef Matt Roper:
> On Thu, Oct 19, 2017 at 05:13:40PM +0200, Maarten Lankhorst wrote:
>> The original intent was to preserve watermarks as much as possible
>> in intel_pipe_wm.raw_wm, and put the validated ones in intel_pipe_wm.wm.
>>
>> It seems this approach is insufficient and we don't always preserve
>> the raw watermarks, so just use the atomic iterator we're already using
>> to get a const pointer to all bound planes on the crtc.
>>
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=102373
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> Cc: stable@vger.kernel.org #v4.8+
> It's been a while since I looked at this code, so I'm not sure I'm
> following all the context/history here.  Before this patch, we had
> calculated watermarks for all levels (even those above the maximum
> usable watermark level as determined by sprite usage) into raw_wm.  But
> as far as I can tell, we never actually used those values for anything
> so that was no different than just throwing the values away?

No, the trick introduced there was that we preserved the raw watermarks for future
calculations to not include all planes in a commit. This is now solved by using the
read-only drm_atomic_crtc_state_for_each_plane_state iterator, to get all
const plane states without having to include them.

> Is my understanding correct that this is mostly a revert of 71f0a62614
> ("drm/i915: Only use sanitized values for ILK watermarks) except that it
> also modifies the level validation loop so that we're only calling
> ilk_compute_wm_level() on the levels up to usable_level whereas before
> that patch we were calling it on all levels, but then
> setting wm->enable = false for the unusable levels?
>
> By my understanding, this looks like a safe simplification of the
> current logic, but I don't see where the functional change is here.
> Was the Buzilla: reference supposed to be tied to patch #2 of this
> series or am I missing something important?

We now throw away all watermarks, and use drm_atomic_crtc_state_for_each_plane_state
to get the plane states. As a result we can throw away the raw watermarks, which have
proven to be insufficiently preserved. :)

>> ---
>>  drivers/gpu/drm/i915/intel_drv.h |  1 -
>>  drivers/gpu/drm/i915/intel_pm.c  | 51 +++++++++++++++++-----------------------
>>  2 files changed, 21 insertions(+), 31 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index 47d022d48718..7bc60c848940 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -499,7 +499,6 @@ struct intel_crtc_scaler_state {
>>  
>>  struct intel_pipe_wm {
>>  	struct intel_wm_level wm[5];
>> -	struct intel_wm_level raw_wm[5];
>>  	uint32_t linetime;
>>  	bool fbc_wm_enabled;
>>  	bool pipe_enabled;
>> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
>> index c42a65a93b3a..ea70c720f492 100644
>> --- a/drivers/gpu/drm/i915/intel_pm.c
>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>> @@ -2721,9 +2721,9 @@ static void ilk_compute_wm_level(const struct drm_i915_private *dev_priv,
>>  				 const struct intel_crtc *intel_crtc,
>>  				 int level,
>>  				 struct intel_crtc_state *cstate,
>> -				 struct intel_plane_state *pristate,
>> -				 struct intel_plane_state *sprstate,
>> -				 struct intel_plane_state *curstate,
>> +				 const struct intel_plane_state *pristate,
>> +				 const struct intel_plane_state *sprstate,
>> +				 const struct intel_plane_state *curstate,
>>  				 struct intel_wm_level *result)
>>  {
>>  	uint16_t pri_latency = dev_priv->wm.pri_latency[level];
>> @@ -3043,28 +3043,24 @@ static int ilk_compute_pipe_wm(struct intel_crtc_state *cstate)
>>  	struct intel_pipe_wm *pipe_wm;
>>  	struct drm_device *dev = state->dev;
>>  	const struct drm_i915_private *dev_priv = to_i915(dev);
>> -	struct intel_plane *intel_plane;
>> -	struct intel_plane_state *pristate = NULL;
>> -	struct intel_plane_state *sprstate = NULL;
>> -	struct intel_plane_state *curstate = NULL;
>> +	struct drm_plane *plane;
>> +	const struct drm_plane_state *plane_state;
>> +	const struct intel_plane_state *pristate = NULL;
>> +	const struct intel_plane_state *sprstate = NULL;
>> +	const struct intel_plane_state *curstate = NULL;
>>  	int level, max_level = ilk_wm_max_level(dev_priv), usable_level;
>>  	struct ilk_wm_maximums max;
>>  
>>  	pipe_wm = &cstate->wm.ilk.optimal;
>>  
>> -	for_each_intel_plane_on_crtc(dev, intel_crtc, intel_plane) {
>> -		struct intel_plane_state *ps;
>> -
>> -		ps = intel_atomic_get_existing_plane_state(state,
>> -							   intel_plane);
>> -		if (!ps)
>> -			continue;
>> +	drm_atomic_crtc_state_for_each_plane_state(plane, plane_state, &cstate->base) {
>> +		const struct intel_plane_state *ps = to_intel_plane_state(plane_state);
>>  
>> -		if (intel_plane->base.type == DRM_PLANE_TYPE_PRIMARY)
>> +		if (plane->type == DRM_PLANE_TYPE_PRIMARY)
>>  			pristate = ps;
>> -		else if (intel_plane->base.type == DRM_PLANE_TYPE_OVERLAY)
>> +		else if (plane->type == DRM_PLANE_TYPE_OVERLAY)
>>  			sprstate = ps;
>> -		else if (intel_plane->base.type == DRM_PLANE_TYPE_CURSOR)
>> +		else if (plane->type == DRM_PLANE_TYPE_CURSOR)
>>  			curstate = ps;
>>  	}
>>  
>> @@ -3086,11 +3082,9 @@ static int ilk_compute_pipe_wm(struct intel_crtc_state *cstate)
>>  	if (pipe_wm->sprites_scaled)
>>  		usable_level = 0;
>>  
>> -	ilk_compute_wm_level(dev_priv, intel_crtc, 0, cstate,
>> -			     pristate, sprstate, curstate, &pipe_wm->raw_wm[0]);
>> -
>>  	memset(&pipe_wm->wm, 0, sizeof(pipe_wm->wm));
>> -	pipe_wm->wm[0] = pipe_wm->raw_wm[0];
>> +	ilk_compute_wm_level(dev_priv, intel_crtc, 0, cstate,
>> +			     pristate, sprstate, curstate, &pipe_wm->wm[0]);
>>  
>>  	if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv))
>>  		pipe_wm->linetime = hsw_compute_linetime_wm(cstate);
>> @@ -3100,8 +3094,8 @@ static int ilk_compute_pipe_wm(struct intel_crtc_state *cstate)
>>  
>>  	ilk_compute_wm_reg_maximums(dev_priv, 1, &max);
>>  
>> -	for (level = 1; level <= max_level; level++) {
>> -		struct intel_wm_level *wm = &pipe_wm->raw_wm[level];
>> +	for (level = 1; level <= usable_level; level++) {
>> +		struct intel_wm_level *wm = &pipe_wm->wm[level];
>>  
>>  		ilk_compute_wm_level(dev_priv, intel_crtc, level, cstate,
>>  				     pristate, sprstate, curstate, wm);
>> @@ -3111,13 +3105,10 @@ static int ilk_compute_pipe_wm(struct intel_crtc_state *cstate)
>>  		 * register maximums since such watermarks are
>>  		 * always invalid.
>>  		 */
>> -		if (level > usable_level)
>> -			continue;
>> -
>> -		if (ilk_validate_wm_level(level, &max, wm))
>> -			pipe_wm->wm[level] = *wm;
>> -		else
>> -			usable_level = level;
>> +		if (!ilk_validate_wm_level(level, &max, wm)) {
>> +			memset(wm, 0, sizeof(*wm));
>> +			break;
>> +		}
>>  	}
>>  
>>  	return 0;
>> -- 
>> 2.14.1
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/2] drm/i915: Do not rely on wm preservation for ILK watermarks
  2017-10-19 15:13 [PATCH 1/2] drm/i915: Do not rely on wm preservation for ILK watermarks Maarten Lankhorst
                   ` (4 preceding siblings ...)
  2017-10-24 23:01 ` [PATCH 1/2] " Matt Roper
@ 2017-10-25 10:23 ` Ville Syrjälä
  2017-10-27  8:03 ` ✗ Fi.CI.BAT: failure for series starting with [1/2] " Patchwork
  6 siblings, 0 replies; 12+ messages in thread
From: Ville Syrjälä @ 2017-10-25 10:23 UTC (permalink / raw)
  To: Maarten Lankhorst; +Cc: intel-gfx

On Thu, Oct 19, 2017 at 05:13:40PM +0200, Maarten Lankhorst wrote:
> The original intent was to preserve watermarks as much as possible
> in intel_pipe_wm.raw_wm, and put the validated ones in intel_pipe_wm.wm.
> 
> It seems this approach is insufficient and we don't always preserve
> the raw watermarks, so just use the atomic iterator we're already using
> to get a const pointer to all bound planes on the crtc.

I don't really like that trick. But I gave this series a try on my IVB
and so far things seem to work correctly. IIRC just patch 2 (or something
similar) alone resulted in failure to compute intermediate watermarks
and thus a black screen.

So if it fixes things
Acked-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=102373
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: stable@vger.kernel.org #v4.8+
> ---
>  drivers/gpu/drm/i915/intel_drv.h |  1 -
>  drivers/gpu/drm/i915/intel_pm.c  | 51 +++++++++++++++++-----------------------
>  2 files changed, 21 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 47d022d48718..7bc60c848940 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -499,7 +499,6 @@ struct intel_crtc_scaler_state {
>  
>  struct intel_pipe_wm {
>  	struct intel_wm_level wm[5];
> -	struct intel_wm_level raw_wm[5];
>  	uint32_t linetime;
>  	bool fbc_wm_enabled;
>  	bool pipe_enabled;
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index c42a65a93b3a..ea70c720f492 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -2721,9 +2721,9 @@ static void ilk_compute_wm_level(const struct drm_i915_private *dev_priv,
>  				 const struct intel_crtc *intel_crtc,
>  				 int level,
>  				 struct intel_crtc_state *cstate,
> -				 struct intel_plane_state *pristate,
> -				 struct intel_plane_state *sprstate,
> -				 struct intel_plane_state *curstate,
> +				 const struct intel_plane_state *pristate,
> +				 const struct intel_plane_state *sprstate,
> +				 const struct intel_plane_state *curstate,
>  				 struct intel_wm_level *result)
>  {
>  	uint16_t pri_latency = dev_priv->wm.pri_latency[level];
> @@ -3043,28 +3043,24 @@ static int ilk_compute_pipe_wm(struct intel_crtc_state *cstate)
>  	struct intel_pipe_wm *pipe_wm;
>  	struct drm_device *dev = state->dev;
>  	const struct drm_i915_private *dev_priv = to_i915(dev);
> -	struct intel_plane *intel_plane;
> -	struct intel_plane_state *pristate = NULL;
> -	struct intel_plane_state *sprstate = NULL;
> -	struct intel_plane_state *curstate = NULL;
> +	struct drm_plane *plane;
> +	const struct drm_plane_state *plane_state;
> +	const struct intel_plane_state *pristate = NULL;
> +	const struct intel_plane_state *sprstate = NULL;
> +	const struct intel_plane_state *curstate = NULL;
>  	int level, max_level = ilk_wm_max_level(dev_priv), usable_level;
>  	struct ilk_wm_maximums max;
>  
>  	pipe_wm = &cstate->wm.ilk.optimal;
>  
> -	for_each_intel_plane_on_crtc(dev, intel_crtc, intel_plane) {
> -		struct intel_plane_state *ps;
> -
> -		ps = intel_atomic_get_existing_plane_state(state,
> -							   intel_plane);
> -		if (!ps)
> -			continue;
> +	drm_atomic_crtc_state_for_each_plane_state(plane, plane_state, &cstate->base) {
> +		const struct intel_plane_state *ps = to_intel_plane_state(plane_state);
>  
> -		if (intel_plane->base.type == DRM_PLANE_TYPE_PRIMARY)
> +		if (plane->type == DRM_PLANE_TYPE_PRIMARY)
>  			pristate = ps;
> -		else if (intel_plane->base.type == DRM_PLANE_TYPE_OVERLAY)
> +		else if (plane->type == DRM_PLANE_TYPE_OVERLAY)
>  			sprstate = ps;
> -		else if (intel_plane->base.type == DRM_PLANE_TYPE_CURSOR)
> +		else if (plane->type == DRM_PLANE_TYPE_CURSOR)
>  			curstate = ps;
>  	}
>  
> @@ -3086,11 +3082,9 @@ static int ilk_compute_pipe_wm(struct intel_crtc_state *cstate)
>  	if (pipe_wm->sprites_scaled)
>  		usable_level = 0;
>  
> -	ilk_compute_wm_level(dev_priv, intel_crtc, 0, cstate,
> -			     pristate, sprstate, curstate, &pipe_wm->raw_wm[0]);
> -
>  	memset(&pipe_wm->wm, 0, sizeof(pipe_wm->wm));
> -	pipe_wm->wm[0] = pipe_wm->raw_wm[0];
> +	ilk_compute_wm_level(dev_priv, intel_crtc, 0, cstate,
> +			     pristate, sprstate, curstate, &pipe_wm->wm[0]);
>  
>  	if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv))
>  		pipe_wm->linetime = hsw_compute_linetime_wm(cstate);
> @@ -3100,8 +3094,8 @@ static int ilk_compute_pipe_wm(struct intel_crtc_state *cstate)
>  
>  	ilk_compute_wm_reg_maximums(dev_priv, 1, &max);
>  
> -	for (level = 1; level <= max_level; level++) {
> -		struct intel_wm_level *wm = &pipe_wm->raw_wm[level];
> +	for (level = 1; level <= usable_level; level++) {
> +		struct intel_wm_level *wm = &pipe_wm->wm[level];
>  
>  		ilk_compute_wm_level(dev_priv, intel_crtc, level, cstate,
>  				     pristate, sprstate, curstate, wm);
> @@ -3111,13 +3105,10 @@ static int ilk_compute_pipe_wm(struct intel_crtc_state *cstate)
>  		 * register maximums since such watermarks are
>  		 * always invalid.
>  		 */
> -		if (level > usable_level)
> -			continue;
> -
> -		if (ilk_validate_wm_level(level, &max, wm))
> -			pipe_wm->wm[level] = *wm;
> -		else
> -			usable_level = level;
> +		if (!ilk_validate_wm_level(level, &max, wm)) {
> +			memset(wm, 0, sizeof(*wm));
> +			break;
> +		}
>  	}
>  
>  	return 0;
> -- 
> 2.14.1

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/2] drm/i915: Do not rely on wm preservation for ILK watermarks
  2017-10-25  6:03   ` Maarten Lankhorst
@ 2017-10-27  1:09     ` Matt Roper
  2017-10-27  7:23       ` Maarten Lankhorst
  0 siblings, 1 reply; 12+ messages in thread
From: Matt Roper @ 2017-10-27  1:09 UTC (permalink / raw)
  To: Maarten Lankhorst; +Cc: intel-gfx

On Wed, Oct 25, 2017 at 08:03:47AM +0200, Maarten Lankhorst wrote:
> Op 25-10-17 om 01:01 schreef Matt Roper:
> > On Thu, Oct 19, 2017 at 05:13:40PM +0200, Maarten Lankhorst wrote:
> >> The original intent was to preserve watermarks as much as possible
> >> in intel_pipe_wm.raw_wm, and put the validated ones in intel_pipe_wm.wm.
> >>
> >> It seems this approach is insufficient and we don't always preserve
> >> the raw watermarks, so just use the atomic iterator we're already using
> >> to get a const pointer to all bound planes on the crtc.
> >>
> >> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=102373
> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >> Cc: stable@vger.kernel.org #v4.8+
> > It's been a while since I looked at this code, so I'm not sure I'm
> > following all the context/history here.  Before this patch, we had
> > calculated watermarks for all levels (even those above the maximum
> > usable watermark level as determined by sprite usage) into raw_wm.  But
> > as far as I can tell, we never actually used those values for anything
> > so that was no different than just throwing the values away?
> 
> No, the trick introduced there was that we preserved the raw watermarks for future
> calculations to not include all planes in a commit. This is now solved by using the
> read-only drm_atomic_crtc_state_for_each_plane_state iterator, to get all
> const plane states without having to include them.
> 
> > Is my understanding correct that this is mostly a revert of 71f0a62614
> > ("drm/i915: Only use sanitized values for ILK watermarks) except that it
> > also modifies the level validation loop so that we're only calling
> > ilk_compute_wm_level() on the levels up to usable_level whereas before
> > that patch we were calling it on all levels, but then
> > setting wm->enable = false for the unusable levels?
> >
> > By my understanding, this looks like a safe simplification of the
> > current logic, but I don't see where the functional change is here.
> > Was the Buzilla: reference supposed to be tied to patch #2 of this
> > series or am I missing something important?
> 
> We now throw away all watermarks, and use drm_atomic_crtc_state_for_each_plane_state
> to get the plane states. As a result we can throw away the raw watermarks, which have
> proven to be insufficiently preserved. :)

Okay, makes more sense now.  Thanks for clarifying.

Reviewed-by: Matt Roper <matthew.d.roper@intel.com>


> 
> >> ---
> >>  drivers/gpu/drm/i915/intel_drv.h |  1 -
> >>  drivers/gpu/drm/i915/intel_pm.c  | 51 +++++++++++++++++-----------------------
> >>  2 files changed, 21 insertions(+), 31 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> >> index 47d022d48718..7bc60c848940 100644
> >> --- a/drivers/gpu/drm/i915/intel_drv.h
> >> +++ b/drivers/gpu/drm/i915/intel_drv.h
> >> @@ -499,7 +499,6 @@ struct intel_crtc_scaler_state {
> >>  
> >>  struct intel_pipe_wm {
> >>  	struct intel_wm_level wm[5];
> >> -	struct intel_wm_level raw_wm[5];
> >>  	uint32_t linetime;
> >>  	bool fbc_wm_enabled;
> >>  	bool pipe_enabled;
> >> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> >> index c42a65a93b3a..ea70c720f492 100644
> >> --- a/drivers/gpu/drm/i915/intel_pm.c
> >> +++ b/drivers/gpu/drm/i915/intel_pm.c
> >> @@ -2721,9 +2721,9 @@ static void ilk_compute_wm_level(const struct drm_i915_private *dev_priv,
> >>  				 const struct intel_crtc *intel_crtc,
> >>  				 int level,
> >>  				 struct intel_crtc_state *cstate,
> >> -				 struct intel_plane_state *pristate,
> >> -				 struct intel_plane_state *sprstate,
> >> -				 struct intel_plane_state *curstate,
> >> +				 const struct intel_plane_state *pristate,
> >> +				 const struct intel_plane_state *sprstate,
> >> +				 const struct intel_plane_state *curstate,
> >>  				 struct intel_wm_level *result)
> >>  {
> >>  	uint16_t pri_latency = dev_priv->wm.pri_latency[level];
> >> @@ -3043,28 +3043,24 @@ static int ilk_compute_pipe_wm(struct intel_crtc_state *cstate)
> >>  	struct intel_pipe_wm *pipe_wm;
> >>  	struct drm_device *dev = state->dev;
> >>  	const struct drm_i915_private *dev_priv = to_i915(dev);
> >> -	struct intel_plane *intel_plane;
> >> -	struct intel_plane_state *pristate = NULL;
> >> -	struct intel_plane_state *sprstate = NULL;
> >> -	struct intel_plane_state *curstate = NULL;
> >> +	struct drm_plane *plane;
> >> +	const struct drm_plane_state *plane_state;
> >> +	const struct intel_plane_state *pristate = NULL;
> >> +	const struct intel_plane_state *sprstate = NULL;
> >> +	const struct intel_plane_state *curstate = NULL;
> >>  	int level, max_level = ilk_wm_max_level(dev_priv), usable_level;
> >>  	struct ilk_wm_maximums max;
> >>  
> >>  	pipe_wm = &cstate->wm.ilk.optimal;
> >>  
> >> -	for_each_intel_plane_on_crtc(dev, intel_crtc, intel_plane) {
> >> -		struct intel_plane_state *ps;
> >> -
> >> -		ps = intel_atomic_get_existing_plane_state(state,
> >> -							   intel_plane);
> >> -		if (!ps)
> >> -			continue;
> >> +	drm_atomic_crtc_state_for_each_plane_state(plane, plane_state, &cstate->base) {
> >> +		const struct intel_plane_state *ps = to_intel_plane_state(plane_state);
> >>  
> >> -		if (intel_plane->base.type == DRM_PLANE_TYPE_PRIMARY)
> >> +		if (plane->type == DRM_PLANE_TYPE_PRIMARY)
> >>  			pristate = ps;
> >> -		else if (intel_plane->base.type == DRM_PLANE_TYPE_OVERLAY)
> >> +		else if (plane->type == DRM_PLANE_TYPE_OVERLAY)
> >>  			sprstate = ps;
> >> -		else if (intel_plane->base.type == DRM_PLANE_TYPE_CURSOR)
> >> +		else if (plane->type == DRM_PLANE_TYPE_CURSOR)
> >>  			curstate = ps;
> >>  	}
> >>  
> >> @@ -3086,11 +3082,9 @@ static int ilk_compute_pipe_wm(struct intel_crtc_state *cstate)
> >>  	if (pipe_wm->sprites_scaled)
> >>  		usable_level = 0;
> >>  
> >> -	ilk_compute_wm_level(dev_priv, intel_crtc, 0, cstate,
> >> -			     pristate, sprstate, curstate, &pipe_wm->raw_wm[0]);
> >> -
> >>  	memset(&pipe_wm->wm, 0, sizeof(pipe_wm->wm));
> >> -	pipe_wm->wm[0] = pipe_wm->raw_wm[0];
> >> +	ilk_compute_wm_level(dev_priv, intel_crtc, 0, cstate,
> >> +			     pristate, sprstate, curstate, &pipe_wm->wm[0]);
> >>  
> >>  	if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv))
> >>  		pipe_wm->linetime = hsw_compute_linetime_wm(cstate);
> >> @@ -3100,8 +3094,8 @@ static int ilk_compute_pipe_wm(struct intel_crtc_state *cstate)
> >>  
> >>  	ilk_compute_wm_reg_maximums(dev_priv, 1, &max);
> >>  
> >> -	for (level = 1; level <= max_level; level++) {
> >> -		struct intel_wm_level *wm = &pipe_wm->raw_wm[level];
> >> +	for (level = 1; level <= usable_level; level++) {
> >> +		struct intel_wm_level *wm = &pipe_wm->wm[level];
> >>  
> >>  		ilk_compute_wm_level(dev_priv, intel_crtc, level, cstate,
> >>  				     pristate, sprstate, curstate, wm);
> >> @@ -3111,13 +3105,10 @@ static int ilk_compute_pipe_wm(struct intel_crtc_state *cstate)
> >>  		 * register maximums since such watermarks are
> >>  		 * always invalid.
> >>  		 */
> >> -		if (level > usable_level)
> >> -			continue;
> >> -
> >> -		if (ilk_validate_wm_level(level, &max, wm))
> >> -			pipe_wm->wm[level] = *wm;
> >> -		else
> >> -			usable_level = level;
> >> +		if (!ilk_validate_wm_level(level, &max, wm)) {
> >> +			memset(wm, 0, sizeof(*wm));
> >> +			break;
> >> +		}
> >>  	}
> >>  
> >>  	return 0;
> >> -- 
> >> 2.14.1
> >>
> >> _______________________________________________
> >> Intel-gfx mailing list
> >> Intel-gfx@lists.freedesktop.org
> >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/2] drm/i915: Do not rely on wm preservation for ILK watermarks
  2017-10-27  1:09     ` Matt Roper
@ 2017-10-27  7:23       ` Maarten Lankhorst
  0 siblings, 0 replies; 12+ messages in thread
From: Maarten Lankhorst @ 2017-10-27  7:23 UTC (permalink / raw)
  To: Matt Roper; +Cc: intel-gfx

Op 27-10-17 om 03:09 schreef Matt Roper:
> On Wed, Oct 25, 2017 at 08:03:47AM +0200, Maarten Lankhorst wrote:
>> Op 25-10-17 om 01:01 schreef Matt Roper:
>>> On Thu, Oct 19, 2017 at 05:13:40PM +0200, Maarten Lankhorst wrote:
>>>> The original intent was to preserve watermarks as much as possible
>>>> in intel_pipe_wm.raw_wm, and put the validated ones in intel_pipe_wm.wm.
>>>>
>>>> It seems this approach is insufficient and we don't always preserve
>>>> the raw watermarks, so just use the atomic iterator we're already using
>>>> to get a const pointer to all bound planes on the crtc.
>>>>
>>>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=102373
>>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>>> Cc: stable@vger.kernel.org #v4.8+
>>> It's been a while since I looked at this code, so I'm not sure I'm
>>> following all the context/history here.  Before this patch, we had
>>> calculated watermarks for all levels (even those above the maximum
>>> usable watermark level as determined by sprite usage) into raw_wm.  But
>>> as far as I can tell, we never actually used those values for anything
>>> so that was no different than just throwing the values away?
>> No, the trick introduced there was that we preserved the raw watermarks for future
>> calculations to not include all planes in a commit. This is now solved by using the
>> read-only drm_atomic_crtc_state_for_each_plane_state iterator, to get all
>> const plane states without having to include them.
>>
>>> Is my understanding correct that this is mostly a revert of 71f0a62614
>>> ("drm/i915: Only use sanitized values for ILK watermarks) except that it
>>> also modifies the level validation loop so that we're only calling
>>> ilk_compute_wm_level() on the levels up to usable_level whereas before
>>> that patch we were calling it on all levels, but then
>>> setting wm->enable = false for the unusable levels?
>>>
>>> By my understanding, this looks like a safe simplification of the
>>> current logic, but I don't see where the functional change is here.
>>> Was the Buzilla: reference supposed to be tied to patch #2 of this
>>> series or am I missing something important?
>> We now throw away all watermarks, and use drm_atomic_crtc_state_for_each_plane_state
>> to get the plane states. As a result we can throw away the raw watermarks, which have
>> proven to be insufficiently preserved. :)
> Okay, makes more sense now.  Thanks for clarifying.
>
> Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
Thanks, pushed both patches. Hopefully issues are gone now. :)
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 12+ messages in thread

* ✗ Fi.CI.BAT: failure for series starting with [1/2] drm/i915: Do not rely on wm preservation for ILK watermarks
  2017-10-19 15:13 [PATCH 1/2] drm/i915: Do not rely on wm preservation for ILK watermarks Maarten Lankhorst
                   ` (5 preceding siblings ...)
  2017-10-25 10:23 ` Ville Syrjälä
@ 2017-10-27  8:03 ` Patchwork
  6 siblings, 0 replies; 12+ messages in thread
From: Patchwork @ 2017-10-27  8:03 UTC (permalink / raw)
  To: Maarten Lankhorst; +Cc: intel-gfx

== Series Details ==

Series: series starting with [1/2] drm/i915: Do not rely on wm preservation for ILK watermarks
URL   : https://patchwork.freedesktop.org/series/32301/
State : failure

== Summary ==

Series 32301 revision 1 was fully merged or fully failed: no git log

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_6108/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2017-10-27  8:03 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-10-19 15:13 [PATCH 1/2] drm/i915: Do not rely on wm preservation for ILK watermarks Maarten Lankhorst
2017-10-19 15:13 ` [PATCH 2/2] drm/i915: Calculate ironlake intermediate watermarks correctly, v2 Maarten Lankhorst
2017-10-24 23:10   ` Matt Roper
2017-10-19 15:54 ` ✗ Fi.CI.BAT: warning for series starting with [1/2] drm/i915: Do not rely on wm preservation for ILK watermarks Patchwork
2017-10-19 17:59 ` ✓ Fi.CI.BAT: success " Patchwork
2017-10-19 18:48 ` ✓ Fi.CI.IGT: " Patchwork
2017-10-24 23:01 ` [PATCH 1/2] " Matt Roper
2017-10-25  6:03   ` Maarten Lankhorst
2017-10-27  1:09     ` Matt Roper
2017-10-27  7:23       ` Maarten Lankhorst
2017-10-25 10:23 ` Ville Syrjälä
2017-10-27  8:03 ` ✗ Fi.CI.BAT: failure for series starting with [1/2] " Patchwork

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.