* [PATCH 0/6] Watermarks small fixes/improvements
@ 2018-10-04 23:15 Paulo Zanoni
2018-10-04 23:15 ` [PATCH 1/6] drm/i915: don't apply Display WAs 1125 and 1126 to GLK/CNL+ Paulo Zanoni
` (8 more replies)
0 siblings, 9 replies; 18+ messages in thread
From: Paulo Zanoni @ 2018-10-04 23:15 UTC (permalink / raw)
To: intel-gfx; +Cc: Paulo Zanoni
I'm investigating ICL watermarks failures and these are some of the immediate
problems I was able to find in the watermarks code. I don't think they're enough
to fix the problems our CI is able to reproduce, but I do think these changes
are worth having.
Paulo Zanoni (6):
drm/i915: don't apply Display WAs 1125 and 1126 to GLK/CNL+
drm/i915: fix the transition minimums for gen9+ watermarks
drm/i915: fix the watermark result selection on glk/gen10+
drm/i915: transition WMs ask for Selected Result Blocks
drm/i915: don't write PLANE_BUF_CFG twice every time
drm/i915: promote ddb update message to DRM_DEBUG_KMS
drivers/gpu/drm/i915/intel_pm.c | 100 ++++++++++++++++++++++++----------------
1 file changed, 61 insertions(+), 39 deletions(-)
--
2.14.4
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 1/6] drm/i915: don't apply Display WAs 1125 and 1126 to GLK/CNL+
2018-10-04 23:15 [PATCH 0/6] Watermarks small fixes/improvements Paulo Zanoni
@ 2018-10-04 23:15 ` Paulo Zanoni
2018-10-09 23:55 ` Matt Roper
2018-10-04 23:15 ` [PATCH 2/6] drm/i915: fix the transition minimums for gen9+ watermarks Paulo Zanoni
` (7 subsequent siblings)
8 siblings, 1 reply; 18+ messages in thread
From: Paulo Zanoni @ 2018-10-04 23:15 UTC (permalink / raw)
To: intel-gfx; +Cc: Paulo Zanoni
BSpec does not show these WAs as applicable to GLK, and for CNL it
only shows them applicable for a super early pre-production stepping
we shouldn't be caring about anymore. Remove these so we can avoid
them on ICL too.
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
drivers/gpu/drm/i915/intel_pm.c | 43 ++++++++++++++++++++++-------------------
1 file changed, 23 insertions(+), 20 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 1392aa56a55a..910551e04d16 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4687,28 +4687,31 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
res_lines = div_round_up_fixed16(selected_result,
wp->plane_blocks_per_line);
- /* Display WA #1125: skl,bxt,kbl,glk */
- if (level == 0 && wp->rc_surface)
- res_blocks += fixed16_to_u32_round_up(wp->y_tile_minimum);
+ if (IS_GEN9(dev_priv) && !IS_GEMINILAKE(dev_priv)) {
+ /* Display WA #1125: skl,bxt,kbl */
+ if (level == 0 && wp->rc_surface)
+ res_blocks +=
+ fixed16_to_u32_round_up(wp->y_tile_minimum);
+
+ /* Display WA #1126: skl,bxt,kbl */
+ if (level >= 1 && level <= 7) {
+ if (wp->y_tiled) {
+ res_blocks +=
+ fixed16_to_u32_round_up(wp->y_tile_minimum);
+ res_lines += wp->y_min_scanlines;
+ } else {
+ res_blocks++;
+ }
- /* Display WA #1126: skl,bxt,kbl,glk */
- if (level >= 1 && level <= 7) {
- if (wp->y_tiled) {
- res_blocks += fixed16_to_u32_round_up(
- wp->y_tile_minimum);
- res_lines += wp->y_min_scanlines;
- } else {
- res_blocks++;
+ /*
+ * Make sure result blocks for higher latency levels are
+ * atleast as high as level below the current level.
+ * Assumption in DDB algorithm optimization for special
+ * cases. Also covers Display WA #1125 for RC.
+ */
+ if (result_prev->plane_res_b > res_blocks)
+ res_blocks = result_prev->plane_res_b;
}
-
- /*
- * Make sure result blocks for higher latency levels are atleast
- * as high as level below the current level.
- * Assumption in DDB algorithm optimization for special cases.
- * Also covers Display WA #1125 for RC.
- */
- if (result_prev->plane_res_b > res_blocks)
- res_blocks = result_prev->plane_res_b;
}
if (INTEL_GEN(dev_priv) >= 11) {
--
2.14.4
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH 2/6] drm/i915: fix the transition minimums for gen9+ watermarks
2018-10-04 23:15 [PATCH 0/6] Watermarks small fixes/improvements Paulo Zanoni
2018-10-04 23:15 ` [PATCH 1/6] drm/i915: don't apply Display WAs 1125 and 1126 to GLK/CNL+ Paulo Zanoni
@ 2018-10-04 23:15 ` Paulo Zanoni
2018-10-09 23:55 ` Matt Roper
2018-10-04 23:15 ` [PATCH 3/6] drm/i915: fix the watermark result selection on glk/gen10+ Paulo Zanoni
` (6 subsequent siblings)
8 siblings, 1 reply; 18+ messages in thread
From: Paulo Zanoni @ 2018-10-04 23:15 UTC (permalink / raw)
To: intel-gfx; +Cc: Paulo Zanoni
The transition minimum is 14 blocks for gens 9 and 10, and 4 blocks
for gen 11. This minimum value is supposed to be added to the
configurable trans_amount. This matches both BSpec and additional
information provided by our HW engineers.
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
drivers/gpu/drm/i915/intel_pm.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 910551e04d16..cab86690a0ba 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4878,8 +4878,8 @@ static void skl_compute_transition_wm(struct intel_crtc_state *cstate,
if (!dev_priv->ipc_enabled)
goto exit;
- trans_min = 0;
- if (INTEL_GEN(dev_priv) >= 10)
+ trans_min = 14;
+ if (INTEL_GEN(dev_priv) >= 11)
trans_min = 4;
trans_offset_b = trans_min + trans_amount;
--
2.14.4
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH 3/6] drm/i915: fix the watermark result selection on glk/gen10+
2018-10-04 23:15 [PATCH 0/6] Watermarks small fixes/improvements Paulo Zanoni
2018-10-04 23:15 ` [PATCH 1/6] drm/i915: don't apply Display WAs 1125 and 1126 to GLK/CNL+ Paulo Zanoni
2018-10-04 23:15 ` [PATCH 2/6] drm/i915: fix the transition minimums for gen9+ watermarks Paulo Zanoni
@ 2018-10-04 23:15 ` Paulo Zanoni
2018-10-09 23:55 ` Matt Roper
2018-10-04 23:15 ` [PATCH 4/6] drm/i915: transition WMs ask for Selected Result Blocks Paulo Zanoni
` (5 subsequent siblings)
8 siblings, 1 reply; 18+ messages in thread
From: Paulo Zanoni @ 2018-10-04 23:15 UTC (permalink / raw)
To: intel-gfx; +Cc: Paulo Zanoni
On these platforms we're supposed to unconditonally pick the method 2
result instead of the minimum.
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
drivers/gpu/drm/i915/intel_pm.c | 23 ++++++++++++++++-------
1 file changed, 16 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index cab86690a0ba..40ce99c455f3 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4672,15 +4672,24 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
} else {
if ((wp->cpp * cstate->base.adjusted_mode.crtc_htotal /
wp->dbuf_block_size < 1) &&
- (wp->plane_bytes_per_line / wp->dbuf_block_size < 1))
+ (wp->plane_bytes_per_line / wp->dbuf_block_size < 1)) {
selected_result = method2;
- else if (ddb_allocation >=
- fixed16_to_u32_round_up(wp->plane_blocks_per_line))
- selected_result = min_fixed16(method1, method2);
- else if (latency >= wp->linetime_us)
- selected_result = min_fixed16(method1, method2);
- else
+ } else if (ddb_allocation >=
+ fixed16_to_u32_round_up(wp->plane_blocks_per_line)) {
+ if (INTEL_GEN(dev_priv) == 9 &&
+ !IS_GEMINILAKE(dev_priv))
+ selected_result = min_fixed16(method1, method2);
+ else
+ selected_result = method2;
+ } else if (latency >= wp->linetime_us) {
+ if (INTEL_GEN(dev_priv) == 9 &&
+ !IS_GEMINILAKE(dev_priv))
+ selected_result = min_fixed16(method1, method2);
+ else
+ selected_result = method2;
+ } else {
selected_result = method1;
+ }
}
res_blocks = fixed16_to_u32_round_up(selected_result) + 1;
--
2.14.4
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH 4/6] drm/i915: transition WMs ask for Selected Result Blocks
2018-10-04 23:15 [PATCH 0/6] Watermarks small fixes/improvements Paulo Zanoni
` (2 preceding siblings ...)
2018-10-04 23:15 ` [PATCH 3/6] drm/i915: fix the watermark result selection on glk/gen10+ Paulo Zanoni
@ 2018-10-04 23:15 ` Paulo Zanoni
2018-10-10 1:36 ` Matt Roper
2018-10-04 23:15 ` [PATCH 5/6] drm/i915: don't write PLANE_BUF_CFG twice every time Paulo Zanoni
` (4 subsequent siblings)
8 siblings, 1 reply; 18+ messages in thread
From: Paulo Zanoni @ 2018-10-04 23:15 UTC (permalink / raw)
To: intel-gfx; +Cc: Paulo Zanoni
The transition watermarks ask for Selected Result Blocks (the real
value), not Result Blocks (the integer value). Given how ceilings are
applied in both the non-transition and the transition watermarks
calculations, we can get away with assuming that Selected Result
Blocks is actually Result Blocks minus 1 without any rounding errors.
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
drivers/gpu/drm/i915/intel_pm.c | 18 +++++++++++++++---
1 file changed, 15 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 40ce99c455f3..14f13a371989 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4874,7 +4874,7 @@ static void skl_compute_transition_wm(struct intel_crtc_state *cstate,
const struct drm_i915_private *dev_priv = to_i915(dev);
uint16_t trans_min, trans_y_tile_min;
const uint16_t trans_amount = 10; /* This is configurable amount */
- uint16_t trans_offset_b, res_blocks;
+ uint16_t wm0_sel_res_b, trans_offset_b, res_blocks;
if (!cstate->base.active)
goto exit;
@@ -4893,13 +4893,25 @@ static void skl_compute_transition_wm(struct intel_crtc_state *cstate,
trans_offset_b = trans_min + trans_amount;
+ /*
+ * The spec asks for Selected Result Blocks for wm0 (the real value),
+ * not Result Blocks (the integer value). Pay attention to the capital
+ * letters. The value wm_l0->plane_res_b is actually Result Blocks, but
+ * since Result Blocks is the ceiling of Selected Result Blocks plus 1,
+ * and since we later will have to get the ceiling of the sum in the
+ * transition watermarks calculation, we can just pretend Selected
+ * Result Blocks is Result Blocks minus 1 and it should work for the
+ * current platforms.
+ */
+ wm0_sel_res_b = wm_l0->plane_res_b - 1;
+
if (wp->y_tiled) {
trans_y_tile_min = (uint16_t) mul_round_up_u32_fixed16(2,
wp->y_tile_minimum);
- res_blocks = max(wm_l0->plane_res_b, trans_y_tile_min) +
+ res_blocks = max(wm0_sel_res_b, trans_y_tile_min) +
trans_offset_b;
} else {
- res_blocks = wm_l0->plane_res_b + trans_offset_b;
+ res_blocks = wm0_sel_res_b + trans_offset_b;
/* WA BUG:1938466 add one block for non y-tile planes */
if (IS_CNL_REVID(dev_priv, CNL_REVID_A0, CNL_REVID_A0))
--
2.14.4
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH 5/6] drm/i915: don't write PLANE_BUF_CFG twice every time
2018-10-04 23:15 [PATCH 0/6] Watermarks small fixes/improvements Paulo Zanoni
` (3 preceding siblings ...)
2018-10-04 23:15 ` [PATCH 4/6] drm/i915: transition WMs ask for Selected Result Blocks Paulo Zanoni
@ 2018-10-04 23:15 ` Paulo Zanoni
2018-10-10 1:51 ` Matt Roper
2018-10-04 23:16 ` [PATCH 6/6] drm/i915: promote ddb update message to DRM_DEBUG_KMS Paulo Zanoni
` (3 subsequent siblings)
8 siblings, 1 reply; 18+ messages in thread
From: Paulo Zanoni @ 2018-10-04 23:15 UTC (permalink / raw)
To: intel-gfx; +Cc: Paulo Zanoni
We were writing to PLANE_BUF_CFG(pipe, plane_id) twice for every
platform, and we were even using different values on the gen10- planar
case. The first write is useless since it just gets replaced with the
next one, so kill it.
There's a lot to improve in the DDB code, but let's start by avoiding
the double write.
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
drivers/gpu/drm/i915/intel_pm.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 14f13a371989..53b4a9a2de69 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -5040,8 +5040,6 @@ static void skl_write_plane_wm(struct intel_crtc *intel_crtc,
skl_write_wm_level(dev_priv, PLANE_WM_TRANS(pipe, plane_id),
&wm->trans_wm);
- skl_ddb_entry_write(dev_priv, PLANE_BUF_CFG(pipe, plane_id),
- &ddb->plane[pipe][plane_id]);
/* FIXME: add proper NV12 support for ICL. */
if (INTEL_GEN(dev_priv) >= 11)
return skl_ddb_entry_write(dev_priv,
--
2.14.4
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH 6/6] drm/i915: promote ddb update message to DRM_DEBUG_KMS
2018-10-04 23:15 [PATCH 0/6] Watermarks small fixes/improvements Paulo Zanoni
` (4 preceding siblings ...)
2018-10-04 23:15 ` [PATCH 5/6] drm/i915: don't write PLANE_BUF_CFG twice every time Paulo Zanoni
@ 2018-10-04 23:16 ` Paulo Zanoni
2018-10-10 1:55 ` Matt Roper
2018-10-04 23:37 ` ✗ Fi.CI.SPARSE: warning for Watermarks small fixes/improvements Patchwork
` (2 subsequent siblings)
8 siblings, 1 reply; 18+ messages in thread
From: Paulo Zanoni @ 2018-10-04 23:16 UTC (permalink / raw)
To: intel-gfx; +Cc: Paulo Zanoni
This message is currently marked as DRM_DEBUG_ATOMIC. I would like it
to be DRM_DEBUG_KMS since it is more KMS than atomic, and this will
also make the message appear in the CI logs, which may or may not help
us with some FIFO underrun bugs.
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
drivers/gpu/drm/i915/intel_pm.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 53b4a9a2de69..6cacddd16010 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -5233,11 +5233,11 @@ skl_print_wm_changes(const struct drm_atomic_state *state)
if (skl_ddb_entry_equal(old, new))
continue;
- DRM_DEBUG_ATOMIC("[PLANE:%d:%s] ddb (%d - %d) -> (%d - %d)\n",
- intel_plane->base.base.id,
- intel_plane->base.name,
- old->start, old->end,
- new->start, new->end);
+ DRM_DEBUG_KMS("[PLANE:%d:%s] ddb (%d - %d) -> (%d - %d)\n",
+ intel_plane->base.base.id,
+ intel_plane->base.name,
+ old->start, old->end,
+ new->start, new->end);
}
}
}
--
2.14.4
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 18+ messages in thread
* ✗ Fi.CI.SPARSE: warning for Watermarks small fixes/improvements
2018-10-04 23:15 [PATCH 0/6] Watermarks small fixes/improvements Paulo Zanoni
` (5 preceding siblings ...)
2018-10-04 23:16 ` [PATCH 6/6] drm/i915: promote ddb update message to DRM_DEBUG_KMS Paulo Zanoni
@ 2018-10-04 23:37 ` Patchwork
2018-10-04 23:51 ` ✓ Fi.CI.BAT: success " Patchwork
2018-10-05 7:15 ` ✓ Fi.CI.IGT: " Patchwork
8 siblings, 0 replies; 18+ messages in thread
From: Patchwork @ 2018-10-04 23:37 UTC (permalink / raw)
To: Paulo Zanoni; +Cc: intel-gfx
== Series Details ==
Series: Watermarks small fixes/improvements
URL : https://patchwork.freedesktop.org/series/50579/
State : warning
== Summary ==
$ dim sparse origin/drm-tip
Commit: drm/i915: don't apply Display WAs 1125 and 1126 to GLK/CNL+
Okay!
Commit: drm/i915: fix the transition minimums for gen9+ watermarks
Okay!
Commit: drm/i915: fix the watermark result selection on glk/gen10+
Okay!
Commit: drm/i915: transition WMs ask for Selected Result Blocks
-O:drivers/gpu/drm/i915/intel_pm.c:4899:30: warning: expression using sizeof(void)
-O:drivers/gpu/drm/i915/intel_pm.c:4899:30: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/intel_pm.c:4911:30: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/intel_pm.c:4911:30: warning: expression using sizeof(void)
Commit: drm/i915: don't write PLANE_BUF_CFG twice every time
Okay!
Commit: drm/i915: promote ddb update message to DRM_DEBUG_KMS
Okay!
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 18+ messages in thread
* ✓ Fi.CI.BAT: success for Watermarks small fixes/improvements
2018-10-04 23:15 [PATCH 0/6] Watermarks small fixes/improvements Paulo Zanoni
` (6 preceding siblings ...)
2018-10-04 23:37 ` ✗ Fi.CI.SPARSE: warning for Watermarks small fixes/improvements Patchwork
@ 2018-10-04 23:51 ` Patchwork
2018-10-05 7:15 ` ✓ Fi.CI.IGT: " Patchwork
8 siblings, 0 replies; 18+ messages in thread
From: Patchwork @ 2018-10-04 23:51 UTC (permalink / raw)
To: Paulo Zanoni; +Cc: intel-gfx
== Series Details ==
Series: Watermarks small fixes/improvements
URL : https://patchwork.freedesktop.org/series/50579/
State : success
== Summary ==
= CI Bug Log - changes from CI_DRM_4933 -> Patchwork_10368 =
== Summary - SUCCESS ==
No regressions found.
External URL: https://patchwork.freedesktop.org/api/1.0/series/50579/revisions/1/mbox/
== Known issues ==
Here are the changes found in Patchwork_10368 that come from known issues:
=== IGT changes ===
==== Issues hit ====
igt@gem_exec_suspend@basic-s4-devices:
fi-kbl-7500u: PASS -> DMESG-WARN (fdo#105128, fdo#107139)
igt@kms_flip@basic-plain-flip:
fi-ilk-650: PASS -> DMESG-WARN (fdo#106387)
igt@kms_frontbuffer_tracking@basic:
fi-byt-clapper: PASS -> FAIL (fdo#103167)
igt@kms_pipe_crc_basic@suspend-read-crc-pipe-a:
fi-byt-clapper: PASS -> FAIL (fdo#103191, fdo#107362)
==== Possible fixes ====
igt@kms_pipe_crc_basic@nonblocking-crc-pipe-a:
fi-byt-clapper: FAIL (fdo#107362) -> PASS
fdo#103167 https://bugs.freedesktop.org/show_bug.cgi?id=103167
fdo#103191 https://bugs.freedesktop.org/show_bug.cgi?id=103191
fdo#105128 https://bugs.freedesktop.org/show_bug.cgi?id=105128
fdo#106387 https://bugs.freedesktop.org/show_bug.cgi?id=106387
fdo#107139 https://bugs.freedesktop.org/show_bug.cgi?id=107139
fdo#107362 https://bugs.freedesktop.org/show_bug.cgi?id=107362
== Participating hosts (44 -> 40) ==
Additional (2): fi-glk-j4005 fi-snb-2520m
Missing (6): fi-ilk-m540 fi-byt-squawks fi-bsw-cyan fi-icl-u2 fi-ctg-p8600 fi-pnv-d510
== Build changes ==
* Linux: CI_DRM_4933 -> Patchwork_10368
CI_DRM_4933: 6b7a44d1597791524f46d7ea17620db54dffdc8c @ git://anongit.freedesktop.org/gfx-ci/linux
IGT_4669: 5f40e617cd9c1e089f4a2d79c53a417d891e3e3c @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
Patchwork_10368: cb6ffc487ae82bef586cce9617a7f4b150118efc @ git://anongit.freedesktop.org/gfx-ci/linux
== Linux commits ==
cb6ffc487ae8 drm/i915: promote ddb update message to DRM_DEBUG_KMS
ba4585dfc75c drm/i915: don't write PLANE_BUF_CFG twice every time
bc9212a3b57e drm/i915: transition WMs ask for Selected Result Blocks
c1245720e828 drm/i915: fix the watermark result selection on glk/gen10+
47839a11088f drm/i915: fix the transition minimums for gen9+ watermarks
99b1569417cb drm/i915: don't apply Display WAs 1125 and 1126 to GLK/CNL+
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_10368/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 18+ messages in thread
* ✓ Fi.CI.IGT: success for Watermarks small fixes/improvements
2018-10-04 23:15 [PATCH 0/6] Watermarks small fixes/improvements Paulo Zanoni
` (7 preceding siblings ...)
2018-10-04 23:51 ` ✓ Fi.CI.BAT: success " Patchwork
@ 2018-10-05 7:15 ` Patchwork
8 siblings, 0 replies; 18+ messages in thread
From: Patchwork @ 2018-10-05 7:15 UTC (permalink / raw)
To: Paulo Zanoni; +Cc: intel-gfx
== Series Details ==
Series: Watermarks small fixes/improvements
URL : https://patchwork.freedesktop.org/series/50579/
State : success
== Summary ==
= CI Bug Log - changes from CI_DRM_4933_full -> Patchwork_10368_full =
== Summary - WARNING ==
Minor unknown changes coming with Patchwork_10368_full need to be verified
manually.
If you think the reported changes have nothing to do with the changes
introduced in Patchwork_10368_full, please notify your bug team to allow them
to document this new failure mode, which will reduce false positives in CI.
== Possible new issues ==
Here are the unknown changes that may have been introduced in Patchwork_10368_full:
=== IGT changes ===
==== Warnings ====
igt@kms_frontbuffer_tracking@fbc-2p-primscrn-pri-shrfb-draw-blt:
shard-hsw: PASS -> SKIP
igt@pm_rc6_residency@rc6-accuracy:
shard-snb: PASS -> SKIP
== Known issues ==
Here are the changes found in Patchwork_10368_full that come from known issues:
=== IGT changes ===
==== Issues hit ====
igt@gem_ppgtt@blt-vs-render-ctxn:
shard-skl: NOTRUN -> TIMEOUT (fdo#108039)
igt@gem_userptr_blits@readonly-unsync:
shard-skl: PASS -> INCOMPLETE (fdo#108074)
igt@kms_busy@extended-modeset-hang-newfb-with-reset-render-a:
shard-skl: NOTRUN -> DMESG-WARN (fdo#107956)
igt@kms_busy@extended-pageflip-hang-newfb-render-a:
shard-apl: PASS -> DMESG-WARN (fdo#107956)
igt@kms_color@pipe-a-ctm-max:
shard-apl: PASS -> FAIL (fdo#108147)
igt@kms_cursor_legacy@2x-long-cursor-vs-flip-legacy:
shard-hsw: PASS -> FAIL (fdo#105767)
igt@kms_cursor_legacy@cursorb-vs-flipb-toggle:
shard-glk: PASS -> DMESG-WARN (fdo#106538, fdo#105763)
igt@kms_frontbuffer_tracking@fbc-1p-offscren-pri-indfb-draw-mmap-wc:
shard-skl: PASS -> FAIL (fdo#105682) +2
igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-draw-pwrite:
shard-apl: PASS -> FAIL (fdo#103167) +3
igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-fullscreen:
shard-glk: PASS -> FAIL (fdo#103167) +2
igt@kms_plane@pixel-format-pipe-a-planes:
shard-skl: NOTRUN -> DMESG-WARN (fdo#106885)
{igt@kms_plane_alpha_blend@pipe-a-coverage-7efc}:
shard-skl: PASS -> FAIL (fdo#108145)
{igt@kms_plane_alpha_blend@pipe-b-coverage-7efc}:
shard-skl: NOTRUN -> FAIL (fdo#108146)
{igt@kms_plane_alpha_blend@pipe-c-alpha-transparant-fb}:
shard-skl: NOTRUN -> FAIL (fdo#108145) +3
igt@kms_plane_multiple@atomic-pipe-b-tiling-yf:
shard-apl: PASS -> FAIL (fdo#103166)
igt@kms_rotation_crc@exhaust-fences:
shard-skl: NOTRUN -> DMESG-WARN (fdo#105748)
igt@kms_setmode@basic:
shard-kbl: PASS -> FAIL (fdo#99912)
igt@kms_sysfs_edid_timing:
shard-skl: NOTRUN -> FAIL (fdo#100047)
==== Possible fixes ====
igt@gem_cpu_reloc@full:
shard-skl: INCOMPLETE (fdo#108073) -> PASS
igt@kms_busy@extended-modeset-hang-oldfb-with-reset-render-b:
shard-apl: DMESG-WARN (fdo#103558, fdo#105602) -> PASS +1
igt@kms_cursor_crc@cursor-128x128-random:
shard-glk: FAIL (fdo#103232) -> PASS
igt@kms_cursor_crc@cursor-128x128-suspend:
shard-apl: FAIL (fdo#103191, fdo#103232) -> PASS
igt@kms_cursor_crc@cursor-64x64-random:
shard-apl: FAIL (fdo#103232) -> PASS +1
igt@kms_cursor_legacy@2x-long-nonblocking-modeset-vs-cursor-atomic:
shard-glk: FAIL (fdo#105454, fdo#106509) -> PASS
igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-draw-mmap-wc:
shard-glk: FAIL (fdo#103167) -> PASS
igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-fullscreen:
shard-apl: FAIL (fdo#103167) -> PASS +1
igt@kms_plane@plane-position-covered-pipe-b-planes:
shard-apl: FAIL (fdo#103166) -> PASS
{igt@kms_plane_alpha_blend@pipe-c-alpha-opaque-fb}:
shard-glk: FAIL (fdo#108145) -> PASS
igt@kms_plane_multiple@atomic-pipe-a-tiling-x:
shard-glk: DMESG-WARN (fdo#106538, fdo#105763) -> PASS +3
igt@kms_plane_multiple@atomic-pipe-b-tiling-yf:
shard-glk: FAIL (fdo#103166) -> PASS
igt@kms_setmode@basic:
shard-apl: FAIL (fdo#99912) -> PASS
igt@pm_rpm@pm-tiling:
shard-skl: INCOMPLETE (fdo#107807) -> PASS +1
{name}: This element is suppressed. This means it is ignored when computing
the status of the difference (SUCCESS, WARNING, or FAILURE).
fdo#100047 https://bugs.freedesktop.org/show_bug.cgi?id=100047
fdo#103166 https://bugs.freedesktop.org/show_bug.cgi?id=103166
fdo#103167 https://bugs.freedesktop.org/show_bug.cgi?id=103167
fdo#103191 https://bugs.freedesktop.org/show_bug.cgi?id=103191
fdo#103232 https://bugs.freedesktop.org/show_bug.cgi?id=103232
fdo#103558 https://bugs.freedesktop.org/show_bug.cgi?id=103558
fdo#105454 https://bugs.freedesktop.org/show_bug.cgi?id=105454
fdo#105602 https://bugs.freedesktop.org/show_bug.cgi?id=105602
fdo#105682 https://bugs.freedesktop.org/show_bug.cgi?id=105682
fdo#105748 https://bugs.freedesktop.org/show_bug.cgi?id=105748
fdo#105763 https://bugs.freedesktop.org/show_bug.cgi?id=105763
fdo#105767 https://bugs.freedesktop.org/show_bug.cgi?id=105767
fdo#106509 https://bugs.freedesktop.org/show_bug.cgi?id=106509
fdo#106538 https://bugs.freedesktop.org/show_bug.cgi?id=106538
fdo#106885 https://bugs.freedesktop.org/show_bug.cgi?id=106885
fdo#107807 https://bugs.freedesktop.org/show_bug.cgi?id=107807
fdo#107956 https://bugs.freedesktop.org/show_bug.cgi?id=107956
fdo#108039 https://bugs.freedesktop.org/show_bug.cgi?id=108039
fdo#108073 https://bugs.freedesktop.org/show_bug.cgi?id=108073
fdo#108074 https://bugs.freedesktop.org/show_bug.cgi?id=108074
fdo#108145 https://bugs.freedesktop.org/show_bug.cgi?id=108145
fdo#108146 https://bugs.freedesktop.org/show_bug.cgi?id=108146
fdo#108147 https://bugs.freedesktop.org/show_bug.cgi?id=108147
fdo#99912 https://bugs.freedesktop.org/show_bug.cgi?id=99912
== Participating hosts (6 -> 6) ==
No changes in participating hosts
== Build changes ==
* Linux: CI_DRM_4933 -> Patchwork_10368
CI_DRM_4933: 6b7a44d1597791524f46d7ea17620db54dffdc8c @ git://anongit.freedesktop.org/gfx-ci/linux
IGT_4669: 5f40e617cd9c1e089f4a2d79c53a417d891e3e3c @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
Patchwork_10368: cb6ffc487ae82bef586cce9617a7f4b150118efc @ git://anongit.freedesktop.org/gfx-ci/linux
piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_10368/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/6] drm/i915: don't apply Display WAs 1125 and 1126 to GLK/CNL+
2018-10-04 23:15 ` [PATCH 1/6] drm/i915: don't apply Display WAs 1125 and 1126 to GLK/CNL+ Paulo Zanoni
@ 2018-10-09 23:55 ` Matt Roper
2018-10-11 17:22 ` Paulo Zanoni
0 siblings, 1 reply; 18+ messages in thread
From: Matt Roper @ 2018-10-09 23:55 UTC (permalink / raw)
To: Paulo Zanoni; +Cc: intel-gfx
On Thu, Oct 04, 2018 at 04:15:55PM -0700, Paulo Zanoni wrote:
> BSpec does not show these WAs as applicable to GLK, and for CNL it
> only shows them applicable for a super early pre-production stepping
> we shouldn't be caring about anymore. Remove these so we can avoid
> them on ICL too.
>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
From a quick grep, it looks like there's a couple other CNL A0-specific
workarounds in the codebase (in the watermark code). If we don't want
to handle CNL A0 in this patch, then I think we should first land a
patch that removes the others and updates
intel_detect_preproduction_hw() to make it explicit that this is no
longer a supported stepping.
> ---
> drivers/gpu/drm/i915/intel_pm.c | 43 ++++++++++++++++++++++-------------------
> 1 file changed, 23 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 1392aa56a55a..910551e04d16 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -4687,28 +4687,31 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
> res_lines = div_round_up_fixed16(selected_result,
> wp->plane_blocks_per_line);
>
> - /* Display WA #1125: skl,bxt,kbl,glk */
> - if (level == 0 && wp->rc_surface)
> - res_blocks += fixed16_to_u32_round_up(wp->y_tile_minimum);
> + if (IS_GEN9(dev_priv) && !IS_GEMINILAKE(dev_priv)) {
> + /* Display WA #1125: skl,bxt,kbl */
Although the code is right, should we just write "gen9" (or gen9display)
in this comment (and the one for 1126)? That's how the bspec lists it
(GEN9:All), and removes the ambiguity of whether "kbl" in this context
is also supposed to cover CFL, AML, WHL (which it does).
Matt
> + if (level == 0 && wp->rc_surface)
> + res_blocks +=
> + fixed16_to_u32_round_up(wp->y_tile_minimum);
> +
> + /* Display WA #1126: skl,bxt,kbl */
> + if (level >= 1 && level <= 7) {
> + if (wp->y_tiled) {
> + res_blocks +=
> + fixed16_to_u32_round_up(wp->y_tile_minimum);
> + res_lines += wp->y_min_scanlines;
> + } else {
> + res_blocks++;
> + }
>
> - /* Display WA #1126: skl,bxt,kbl,glk */
> - if (level >= 1 && level <= 7) {
> - if (wp->y_tiled) {
> - res_blocks += fixed16_to_u32_round_up(
> - wp->y_tile_minimum);
> - res_lines += wp->y_min_scanlines;
> - } else {
> - res_blocks++;
> + /*
> + * Make sure result blocks for higher latency levels are
> + * atleast as high as level below the current level.
> + * Assumption in DDB algorithm optimization for special
> + * cases. Also covers Display WA #1125 for RC.
> + */
> + if (result_prev->plane_res_b > res_blocks)
> + res_blocks = result_prev->plane_res_b;
> }
> -
> - /*
> - * Make sure result blocks for higher latency levels are atleast
> - * as high as level below the current level.
> - * Assumption in DDB algorithm optimization for special cases.
> - * Also covers Display WA #1125 for RC.
> - */
> - if (result_prev->plane_res_b > res_blocks)
> - res_blocks = result_prev->plane_res_b;
> }
>
> if (INTEL_GEN(dev_priv) >= 11) {
> --
> 2.14.4
>
> _______________________________________________
> 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] 18+ messages in thread
* Re: [PATCH 2/6] drm/i915: fix the transition minimums for gen9+ watermarks
2018-10-04 23:15 ` [PATCH 2/6] drm/i915: fix the transition minimums for gen9+ watermarks Paulo Zanoni
@ 2018-10-09 23:55 ` Matt Roper
0 siblings, 0 replies; 18+ messages in thread
From: Matt Roper @ 2018-10-09 23:55 UTC (permalink / raw)
To: Paulo Zanoni; +Cc: intel-gfx
On Thu, Oct 04, 2018 at 04:15:56PM -0700, Paulo Zanoni wrote:
> The transition minimum is 14 blocks for gens 9 and 10, and 4 blocks
> for gen 11. This minimum value is supposed to be added to the
> configurable trans_amount. This matches both BSpec and additional
> information provided by our HW engineers.
>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
Matches what I see in the bspec.
Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
> ---
> drivers/gpu/drm/i915/intel_pm.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 910551e04d16..cab86690a0ba 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -4878,8 +4878,8 @@ static void skl_compute_transition_wm(struct intel_crtc_state *cstate,
> if (!dev_priv->ipc_enabled)
> goto exit;
>
> - trans_min = 0;
> - if (INTEL_GEN(dev_priv) >= 10)
> + trans_min = 14;
> + if (INTEL_GEN(dev_priv) >= 11)
> trans_min = 4;
>
> trans_offset_b = trans_min + trans_amount;
> --
> 2.14.4
>
> _______________________________________________
> 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] 18+ messages in thread
* Re: [PATCH 3/6] drm/i915: fix the watermark result selection on glk/gen10+
2018-10-04 23:15 ` [PATCH 3/6] drm/i915: fix the watermark result selection on glk/gen10+ Paulo Zanoni
@ 2018-10-09 23:55 ` Matt Roper
2018-10-10 1:38 ` Matt Roper
0 siblings, 1 reply; 18+ messages in thread
From: Matt Roper @ 2018-10-09 23:55 UTC (permalink / raw)
To: Paulo Zanoni; +Cc: intel-gfx
On Thu, Oct 04, 2018 at 04:15:57PM -0700, Paulo Zanoni wrote:
> On these platforms we're supposed to unconditonally pick the method 2
> result instead of the minimum.
In addition to this, it looks like the calculations for method 1 and
method 2 need a slight update. gen10/gen11 adds an extra "+1" to the
end of the method1 calculation and also to the interm_pbpl used in
various method 2 calculations.
Matt
>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
> drivers/gpu/drm/i915/intel_pm.c | 23 ++++++++++++++++-------
> 1 file changed, 16 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index cab86690a0ba..40ce99c455f3 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -4672,15 +4672,24 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
> } else {
> if ((wp->cpp * cstate->base.adjusted_mode.crtc_htotal /
> wp->dbuf_block_size < 1) &&
> - (wp->plane_bytes_per_line / wp->dbuf_block_size < 1))
> + (wp->plane_bytes_per_line / wp->dbuf_block_size < 1)) {
> selected_result = method2;
> - else if (ddb_allocation >=
> - fixed16_to_u32_round_up(wp->plane_blocks_per_line))
> - selected_result = min_fixed16(method1, method2);
> - else if (latency >= wp->linetime_us)
> - selected_result = min_fixed16(method1, method2);
> - else
> + } else if (ddb_allocation >=
> + fixed16_to_u32_round_up(wp->plane_blocks_per_line)) {
> + if (INTEL_GEN(dev_priv) == 9 &&
> + !IS_GEMINILAKE(dev_priv))
> + selected_result = min_fixed16(method1, method2);
> + else
> + selected_result = method2;
> + } else if (latency >= wp->linetime_us) {
> + if (INTEL_GEN(dev_priv) == 9 &&
> + !IS_GEMINILAKE(dev_priv))
> + selected_result = min_fixed16(method1, method2);
> + else
> + selected_result = method2;
> + } else {
> selected_result = method1;
> + }
> }
>
> res_blocks = fixed16_to_u32_round_up(selected_result) + 1;
> --
> 2.14.4
>
> _______________________________________________
> 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] 18+ messages in thread
* Re: [PATCH 4/6] drm/i915: transition WMs ask for Selected Result Blocks
2018-10-04 23:15 ` [PATCH 4/6] drm/i915: transition WMs ask for Selected Result Blocks Paulo Zanoni
@ 2018-10-10 1:36 ` Matt Roper
0 siblings, 0 replies; 18+ messages in thread
From: Matt Roper @ 2018-10-10 1:36 UTC (permalink / raw)
To: Paulo Zanoni; +Cc: intel-gfx
On Thu, Oct 04, 2018 at 04:15:58PM -0700, Paulo Zanoni wrote:
> The transition watermarks ask for Selected Result Blocks (the real
> value), not Result Blocks (the integer value). Given how ceilings are
> applied in both the non-transition and the transition watermarks
> calculations, we can get away with assuming that Selected Result
> Blocks is actually Result Blocks minus 1 without any rounding errors.
>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
Matches current bspec.
Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
I wonder if we should make trans_amount configurable via debugfs so that
we can experiment with the impact of transitional watermarks.
Matt
> ---
> drivers/gpu/drm/i915/intel_pm.c | 18 +++++++++++++++---
> 1 file changed, 15 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 40ce99c455f3..14f13a371989 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -4874,7 +4874,7 @@ static void skl_compute_transition_wm(struct intel_crtc_state *cstate,
> const struct drm_i915_private *dev_priv = to_i915(dev);
> uint16_t trans_min, trans_y_tile_min;
> const uint16_t trans_amount = 10; /* This is configurable amount */
> - uint16_t trans_offset_b, res_blocks;
> + uint16_t wm0_sel_res_b, trans_offset_b, res_blocks;
>
> if (!cstate->base.active)
> goto exit;
> @@ -4893,13 +4893,25 @@ static void skl_compute_transition_wm(struct intel_crtc_state *cstate,
>
> trans_offset_b = trans_min + trans_amount;
>
> + /*
> + * The spec asks for Selected Result Blocks for wm0 (the real value),
> + * not Result Blocks (the integer value). Pay attention to the capital
> + * letters. The value wm_l0->plane_res_b is actually Result Blocks, but
> + * since Result Blocks is the ceiling of Selected Result Blocks plus 1,
> + * and since we later will have to get the ceiling of the sum in the
> + * transition watermarks calculation, we can just pretend Selected
> + * Result Blocks is Result Blocks minus 1 and it should work for the
> + * current platforms.
> + */
> + wm0_sel_res_b = wm_l0->plane_res_b - 1;
> +
> if (wp->y_tiled) {
> trans_y_tile_min = (uint16_t) mul_round_up_u32_fixed16(2,
> wp->y_tile_minimum);
> - res_blocks = max(wm_l0->plane_res_b, trans_y_tile_min) +
> + res_blocks = max(wm0_sel_res_b, trans_y_tile_min) +
> trans_offset_b;
> } else {
> - res_blocks = wm_l0->plane_res_b + trans_offset_b;
> + res_blocks = wm0_sel_res_b + trans_offset_b;
>
> /* WA BUG:1938466 add one block for non y-tile planes */
> if (IS_CNL_REVID(dev_priv, CNL_REVID_A0, CNL_REVID_A0))
> --
> 2.14.4
>
> _______________________________________________
> 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] 18+ messages in thread
* Re: [PATCH 3/6] drm/i915: fix the watermark result selection on glk/gen10+
2018-10-09 23:55 ` Matt Roper
@ 2018-10-10 1:38 ` Matt Roper
0 siblings, 0 replies; 18+ messages in thread
From: Matt Roper @ 2018-10-10 1:38 UTC (permalink / raw)
To: Paulo Zanoni; +Cc: intel-gfx
On Tue, Oct 09, 2018 at 04:55:15PM -0700, Matt Roper wrote:
> On Thu, Oct 04, 2018 at 04:15:57PM -0700, Paulo Zanoni wrote:
> > On these platforms we're supposed to unconditonally pick the method 2
> > result instead of the minimum.
>
> In addition to this, it looks like the calculations for method 1 and
> method 2 need a slight update. gen10/gen11 adds an extra "+1" to the
> end of the method1 calculation and also to the interm_pbpl used in
> various method 2 calculations.
>
Actually, it looks like you've already made these updates in a previous
patch; disregard this comment.
The rest of this patch looks good.
Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
>
> Matt
>
> >
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > ---
> > drivers/gpu/drm/i915/intel_pm.c | 23 ++++++++++++++++-------
> > 1 file changed, 16 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index cab86690a0ba..40ce99c455f3 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -4672,15 +4672,24 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
> > } else {
> > if ((wp->cpp * cstate->base.adjusted_mode.crtc_htotal /
> > wp->dbuf_block_size < 1) &&
> > - (wp->plane_bytes_per_line / wp->dbuf_block_size < 1))
> > + (wp->plane_bytes_per_line / wp->dbuf_block_size < 1)) {
> > selected_result = method2;
> > - else if (ddb_allocation >=
> > - fixed16_to_u32_round_up(wp->plane_blocks_per_line))
> > - selected_result = min_fixed16(method1, method2);
> > - else if (latency >= wp->linetime_us)
> > - selected_result = min_fixed16(method1, method2);
> > - else
> > + } else if (ddb_allocation >=
> > + fixed16_to_u32_round_up(wp->plane_blocks_per_line)) {
> > + if (INTEL_GEN(dev_priv) == 9 &&
> > + !IS_GEMINILAKE(dev_priv))
> > + selected_result = min_fixed16(method1, method2);
> > + else
> > + selected_result = method2;
> > + } else if (latency >= wp->linetime_us) {
> > + if (INTEL_GEN(dev_priv) == 9 &&
> > + !IS_GEMINILAKE(dev_priv))
> > + selected_result = min_fixed16(method1, method2);
> > + else
> > + selected_result = method2;
> > + } else {
> > selected_result = method1;
> > + }
> > }
> >
> > res_blocks = fixed16_to_u32_round_up(selected_result) + 1;
> > --
> > 2.14.4
> >
> > _______________________________________________
> > 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
--
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] 18+ messages in thread
* Re: [PATCH 5/6] drm/i915: don't write PLANE_BUF_CFG twice every time
2018-10-04 23:15 ` [PATCH 5/6] drm/i915: don't write PLANE_BUF_CFG twice every time Paulo Zanoni
@ 2018-10-10 1:51 ` Matt Roper
0 siblings, 0 replies; 18+ messages in thread
From: Matt Roper @ 2018-10-10 1:51 UTC (permalink / raw)
To: Paulo Zanoni; +Cc: intel-gfx
On Thu, Oct 04, 2018 at 04:15:59PM -0700, Paulo Zanoni wrote:
> We were writing to PLANE_BUF_CFG(pipe, plane_id) twice for every
> platform, and we were even using different values on the gen10- planar
> case. The first write is useless since it just gets replaced with the
> next one, so kill it.
>
> There's a lot to improve in the DDB code, but let's start by avoiding
> the double write.
>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
Looks like leftover code that was intended for removal in commit
b879d58ff31ba ("drm/i915/skl+: refactor WM calculation for NV12").
Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
One other comment below.
> ---
> drivers/gpu/drm/i915/intel_pm.c | 2 --
> 1 file changed, 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 14f13a371989..53b4a9a2de69 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -5040,8 +5040,6 @@ static void skl_write_plane_wm(struct intel_crtc *intel_crtc,
> skl_write_wm_level(dev_priv, PLANE_WM_TRANS(pipe, plane_id),
> &wm->trans_wm);
>
> - skl_ddb_entry_write(dev_priv, PLANE_BUF_CFG(pipe, plane_id),
> - &ddb->plane[pipe][plane_id]);
> /* FIXME: add proper NV12 support for ICL. */
> if (INTEL_GEN(dev_priv) >= 11)
> return skl_ddb_entry_write(dev_priv,
Neither this function nor skl_ddb_entry_write() have return values, so
I'm surprised this doesn't raise a compiler warning. Maybe while we're
touching this area of the code we should drop the 'return' here and
convert the next 'if' to an 'else if.'
Matt
> --
> 2.14.4
>
> _______________________________________________
> 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] 18+ messages in thread
* Re: [PATCH 6/6] drm/i915: promote ddb update message to DRM_DEBUG_KMS
2018-10-04 23:16 ` [PATCH 6/6] drm/i915: promote ddb update message to DRM_DEBUG_KMS Paulo Zanoni
@ 2018-10-10 1:55 ` Matt Roper
0 siblings, 0 replies; 18+ messages in thread
From: Matt Roper @ 2018-10-10 1:55 UTC (permalink / raw)
To: Paulo Zanoni; +Cc: intel-gfx
On Thu, Oct 04, 2018 at 04:16:00PM -0700, Paulo Zanoni wrote:
> This message is currently marked as DRM_DEBUG_ATOMIC. I would like it
> to be DRM_DEBUG_KMS since it is more KMS than atomic, and this will
> also make the message appear in the CI logs, which may or may not help
> us with some FIFO underrun bugs.
>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
> ---
> drivers/gpu/drm/i915/intel_pm.c | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 53b4a9a2de69..6cacddd16010 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -5233,11 +5233,11 @@ skl_print_wm_changes(const struct drm_atomic_state *state)
> if (skl_ddb_entry_equal(old, new))
> continue;
>
> - DRM_DEBUG_ATOMIC("[PLANE:%d:%s] ddb (%d - %d) -> (%d - %d)\n",
> - intel_plane->base.base.id,
> - intel_plane->base.name,
> - old->start, old->end,
> - new->start, new->end);
> + DRM_DEBUG_KMS("[PLANE:%d:%s] ddb (%d - %d) -> (%d - %d)\n",
> + intel_plane->base.base.id,
> + intel_plane->base.name,
> + old->start, old->end,
> + new->start, new->end);
> }
> }
> }
> --
> 2.14.4
>
> _______________________________________________
> 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] 18+ messages in thread
* Re: [PATCH 1/6] drm/i915: don't apply Display WAs 1125 and 1126 to GLK/CNL+
2018-10-09 23:55 ` Matt Roper
@ 2018-10-11 17:22 ` Paulo Zanoni
0 siblings, 0 replies; 18+ messages in thread
From: Paulo Zanoni @ 2018-10-11 17:22 UTC (permalink / raw)
To: Matt Roper; +Cc: Jani Nikula, intel-gfx, Vivi, Rodrigo
Em Ter, 2018-10-09 às 16:55 -0700, Matt Roper escreveu:
> On Thu, Oct 04, 2018 at 04:15:55PM -0700, Paulo Zanoni wrote:
> > BSpec does not show these WAs as applicable to GLK, and for CNL it
> > only shows them applicable for a super early pre-production
> > stepping
> > we shouldn't be caring about anymore. Remove these so we can avoid
> > them on ICL too.
> >
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>
> From a quick grep, it looks like there's a couple other CNL A0-
> specific
> workarounds in the codebase (in the watermark code). If we don't
> want
> to handle CNL A0 in this patch, then I think we should first land a
> patch that removes the others and updates
> intel_detect_preproduction_hw() to make it explicit that this is no
> longer a supported stepping.
This patch (and series) is potentially a "bug fix" due to changes in
real-world not-A0 hardware (although I didn't mark this one as a fix
since I doubt it will close anything in Bugzilla). The patch to remove
the implementation of WAs in CNL:A0 is not a bug fix, but a rework.
Fixes should always come before the reworks in order to facilitate
backporting efforts.
I do intend to propose patches removing the CNL A0 watermarks code
(along with other reworks I have in mind), but I wanted to sort the
bugs first.
>
> > ---
> > drivers/gpu/drm/i915/intel_pm.c | 43 ++++++++++++++++++++++-------
> > ------------
> > 1 file changed, 23 insertions(+), 20 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c
> > b/drivers/gpu/drm/i915/intel_pm.c
> > index 1392aa56a55a..910551e04d16 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -4687,28 +4687,31 @@ static int skl_compute_plane_wm(const
> > struct drm_i915_private *dev_priv,
> > res_lines = div_round_up_fixed16(selected_result,
> > wp-
> > >plane_blocks_per_line);
> >
> > - /* Display WA #1125: skl,bxt,kbl,glk */
> > - if (level == 0 && wp->rc_surface)
> > - res_blocks += fixed16_to_u32_round_up(wp-
> > >y_tile_minimum);
> > + if (IS_GEN9(dev_priv) && !IS_GEMINILAKE(dev_priv)) {
> > + /* Display WA #1125: skl,bxt,kbl */
>
> Although the code is right, should we just write "gen9" (or
> gen9display)
> in this comment (and the one for 1126)? That's how the bspec lists
> it
> (GEN9:All), and removes the ambiguity of whether "kbl" in this
> context
> is also supposed to cover CFL, AML, WHL (which it does).
Although that makes sense, it doesn't seem to follow the current
(undocumented?) standard we have for display WAs. I can totally do
this, but I would like some more opinions first. CC'ing the
maintainers.
Thanks for the reviews,
Paulo
>
>
> Matt
>
> > + if (level == 0 && wp->rc_surface)
> > + res_blocks +=
> > + fixed16_to_u32_round_up(wp-
> > >y_tile_minimum);
> > +
> > + /* Display WA #1126: skl,bxt,kbl */
> > + if (level >= 1 && level <= 7) {
> > + if (wp->y_tiled) {
> > + res_blocks +=
> > + fixed16_to_u32_round_up(wp-
> > >y_tile_minimum);
> > + res_lines += wp->y_min_scanlines;
> > + } else {
> > + res_blocks++;
> > + }
> >
> > - /* Display WA #1126: skl,bxt,kbl,glk */
> > - if (level >= 1 && level <= 7) {
> > - if (wp->y_tiled) {
> > - res_blocks += fixed16_to_u32_round_up(
> > - wp-
> > >y_tile_minimum);
> > - res_lines += wp->y_min_scanlines;
> > - } else {
> > - res_blocks++;
> > + /*
> > + * Make sure result blocks for higher
> > latency levels are
> > + * atleast as high as level below the
> > current level.
> > + * Assumption in DDB algorithm
> > optimization for special
> > + * cases. Also covers Display WA #1125 for
> > RC.
> > + */
> > + if (result_prev->plane_res_b > res_blocks)
> > + res_blocks = result_prev-
> > >plane_res_b;
> > }
> > -
> > - /*
> > - * Make sure result blocks for higher latency
> > levels are atleast
> > - * as high as level below the current level.
> > - * Assumption in DDB algorithm optimization for
> > special cases.
> > - * Also covers Display WA #1125 for RC.
> > - */
> > - if (result_prev->plane_res_b > res_blocks)
> > - res_blocks = result_prev->plane_res_b;
> > }
> >
> > if (INTEL_GEN(dev_priv) >= 11) {
> > --
> > 2.14.4
> >
> > _______________________________________________
> > 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] 18+ messages in thread
end of thread, other threads:[~2018-10-11 17:22 UTC | newest]
Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-10-04 23:15 [PATCH 0/6] Watermarks small fixes/improvements Paulo Zanoni
2018-10-04 23:15 ` [PATCH 1/6] drm/i915: don't apply Display WAs 1125 and 1126 to GLK/CNL+ Paulo Zanoni
2018-10-09 23:55 ` Matt Roper
2018-10-11 17:22 ` Paulo Zanoni
2018-10-04 23:15 ` [PATCH 2/6] drm/i915: fix the transition minimums for gen9+ watermarks Paulo Zanoni
2018-10-09 23:55 ` Matt Roper
2018-10-04 23:15 ` [PATCH 3/6] drm/i915: fix the watermark result selection on glk/gen10+ Paulo Zanoni
2018-10-09 23:55 ` Matt Roper
2018-10-10 1:38 ` Matt Roper
2018-10-04 23:15 ` [PATCH 4/6] drm/i915: transition WMs ask for Selected Result Blocks Paulo Zanoni
2018-10-10 1:36 ` Matt Roper
2018-10-04 23:15 ` [PATCH 5/6] drm/i915: don't write PLANE_BUF_CFG twice every time Paulo Zanoni
2018-10-10 1:51 ` Matt Roper
2018-10-04 23:16 ` [PATCH 6/6] drm/i915: promote ddb update message to DRM_DEBUG_KMS Paulo Zanoni
2018-10-10 1:55 ` Matt Roper
2018-10-04 23:37 ` ✗ Fi.CI.SPARSE: warning for Watermarks small fixes/improvements Patchwork
2018-10-04 23:51 ` ✓ Fi.CI.BAT: success " Patchwork
2018-10-05 7:15 ` ✓ Fi.CI.IGT: " Patchwork
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.