* [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.