* [PATCH] drm/i915: Explicit Raw and Adjusted WM latencies.
@ 2018-02-21 1:51 Rodrigo Vivi
2018-02-21 2:13 ` ✗ Fi.CI.CHECKPATCH: warning for " Patchwork
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Rodrigo Vivi @ 2018-02-21 1:51 UTC (permalink / raw)
To: intel-gfx; +Cc: Ashar Shaikh, Rodrigo Vivi
Current code has some limitations:
1. debugfs only shows raw latency we read from PCODE,
not the ones we are configuring.
2. When determining if SAGV can be enabled we only
apply adjusted wa, but we don't apply the IPC one.
So there is the risk of enabling SAGV when we should
actually disable it.
Cc: Mahesh Kumar <mahesh1.kumar@intel.com>
Cc: Ashar Shaikh <azhar.shaikh@intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
drivers/gpu/drm/i915/i915_debugfs.c | 48 ++++++++++++++++++-------------------
drivers/gpu/drm/i915/i915_drv.h | 7 ++++--
drivers/gpu/drm/i915/intel_pm.c | 31 ++++++++++++------------
3 files changed, 43 insertions(+), 43 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index f260bb39d733..94fcb0360b14 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -3664,7 +3664,8 @@ static const struct file_operations i915_displayport_test_type_fops = {
.release = single_release
};
-static void wm_latency_show(struct seq_file *m, const uint16_t wm[8])
+static void wm_latency_show(struct seq_file *m, const uint16_t wm[8],
+ const char *header)
{
struct drm_i915_private *dev_priv = m->private;
struct drm_device *dev = &dev_priv->drm;
@@ -3680,6 +3681,9 @@ static void wm_latency_show(struct seq_file *m, const uint16_t wm[8])
else
num_levels = ilk_wm_max_level(dev_priv) + 1;
+ if (header)
+ seq_printf(m, "%s\n", header);
+
drm_modeset_lock_all(dev);
for (level = 0; level < num_levels; level++) {
@@ -3707,14 +3711,12 @@ static void wm_latency_show(struct seq_file *m, const uint16_t wm[8])
static int pri_wm_latency_show(struct seq_file *m, void *data)
{
struct drm_i915_private *dev_priv = m->private;
- const uint16_t *latencies;
-
- if (INTEL_GEN(dev_priv) >= 9)
- latencies = dev_priv->wm.skl_latency;
- else
- latencies = dev_priv->wm.pri_latency;
- wm_latency_show(m, latencies);
+ if (INTEL_GEN(dev_priv) >= 9) {
+ wm_latency_show(m, dev_priv->wm.skl_latency.raw, "Raw");
+ wm_latency_show(m, dev_priv->wm.skl_latency.adjusted, "Adjusted");
+ } else
+ wm_latency_show(m, dev_priv->wm.pri_latency, NULL);
return 0;
}
@@ -3722,14 +3724,12 @@ static int pri_wm_latency_show(struct seq_file *m, void *data)
static int spr_wm_latency_show(struct seq_file *m, void *data)
{
struct drm_i915_private *dev_priv = m->private;
- const uint16_t *latencies;
-
- if (INTEL_GEN(dev_priv) >= 9)
- latencies = dev_priv->wm.skl_latency;
- else
- latencies = dev_priv->wm.spr_latency;
- wm_latency_show(m, latencies);
+ if (INTEL_GEN(dev_priv) >= 9) {
+ wm_latency_show(m, dev_priv->wm.skl_latency.raw, "Raw");
+ wm_latency_show(m, dev_priv->wm.skl_latency.adjusted, "Adjusted");
+ } else
+ wm_latency_show(m, dev_priv->wm.spr_latency, NULL);
return 0;
}
@@ -3737,14 +3737,12 @@ static int spr_wm_latency_show(struct seq_file *m, void *data)
static int cur_wm_latency_show(struct seq_file *m, void *data)
{
struct drm_i915_private *dev_priv = m->private;
- const uint16_t *latencies;
-
- if (INTEL_GEN(dev_priv) >= 9)
- latencies = dev_priv->wm.skl_latency;
- else
- latencies = dev_priv->wm.cur_latency;
- wm_latency_show(m, latencies);
+ if (INTEL_GEN(dev_priv) >= 9) {
+ wm_latency_show(m, dev_priv->wm.skl_latency.raw, "Raw");
+ wm_latency_show(m, dev_priv->wm.skl_latency.adjusted, "Adjusted");
+ } else
+ wm_latency_show(m, dev_priv->wm.cur_latency, NULL);
return 0;
}
@@ -3833,7 +3831,7 @@ static ssize_t pri_wm_latency_write(struct file *file, const char __user *ubuf,
uint16_t *latencies;
if (INTEL_GEN(dev_priv) >= 9)
- latencies = dev_priv->wm.skl_latency;
+ latencies = dev_priv->wm.skl_latency.raw;
else
latencies = dev_priv->wm.pri_latency;
@@ -3848,7 +3846,7 @@ static ssize_t spr_wm_latency_write(struct file *file, const char __user *ubuf,
uint16_t *latencies;
if (INTEL_GEN(dev_priv) >= 9)
- latencies = dev_priv->wm.skl_latency;
+ latencies = dev_priv->wm.skl_latency.raw;
else
latencies = dev_priv->wm.spr_latency;
@@ -3863,7 +3861,7 @@ static ssize_t cur_wm_latency_write(struct file *file, const char __user *ubuf,
uint16_t *latencies;
if (INTEL_GEN(dev_priv) >= 9)
- latencies = dev_priv->wm.skl_latency;
+ latencies = dev_priv->wm.skl_latency.raw;
else
latencies = dev_priv->wm.cur_latency;
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 490ff041fb1e..6969bbb203bc 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2136,11 +2136,14 @@ struct drm_i915_private {
/* cursor */
uint16_t cur_latency[5];
/*
- * Raw watermark memory latency values
+ * Watermark memory latency values
* for SKL for all 8 levels
* in 1us units.
*/
- uint16_t skl_latency[8];
+ struct {
+ uint16_t raw[8];
+ uint16_t adjusted[8];
+ } skl_latency;
/* current hardware state */
union {
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index a0a6b4b7c47b..78b52e5a1f8e 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3030,8 +3030,9 @@ static void ilk_setup_wm_latency(struct drm_i915_private *dev_priv)
static void skl_setup_wm_latency(struct drm_i915_private *dev_priv)
{
- intel_read_wm_latency(dev_priv, dev_priv->wm.skl_latency);
- intel_print_wm_latency(dev_priv, "Gen9 Plane", dev_priv->wm.skl_latency);
+ intel_read_wm_latency(dev_priv, dev_priv->wm.skl_latency.raw);
+ intel_print_wm_latency(dev_priv, "Gen9 Plane",
+ dev_priv->wm.skl_latency.raw);
}
static bool ilk_validate_pipe_wm(struct drm_device *dev,
@@ -3745,12 +3746,7 @@ bool intel_can_enable_sagv(struct drm_atomic_state *state)
!wm->wm[level].plane_en; --level)
{ }
- latency = dev_priv->wm.skl_latency[level];
-
- if (skl_needs_memory_bw_wa(intel_state) &&
- plane->base.state->fb->modifier ==
- I915_FORMAT_MOD_X_TILED)
- latency += 15;
+ latency = dev_priv->wm.skl_latency.adjusted[level];
/*
* If any of the planes on this pipe don't enable wm levels that
@@ -4503,7 +4499,7 @@ skl_compute_plane_wm_params(const struct drm_i915_private *dev_priv,
return 0;
}
-static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
+static int skl_compute_plane_wm(struct drm_i915_private *dev_priv,
struct intel_crtc_state *cstate,
const struct intel_plane_state *intel_pstate,
uint16_t ddb_allocation,
@@ -4514,7 +4510,7 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
bool *enabled /* out */)
{
const struct drm_plane_state *pstate = &intel_pstate->base;
- uint32_t latency = dev_priv->wm.skl_latency[level];
+ uint16_t latency = dev_priv->wm.skl_latency.raw[level];
uint_fixed_16_16_t method1, method2;
uint_fixed_16_16_t selected_result;
uint32_t res_blocks, res_lines;
@@ -4538,11 +4534,14 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
if (apply_memory_bw_wa && wp->x_tiled)
latency += 15;
- method1 = skl_wm_method1(dev_priv, wp->plane_pixel_rate,
- wp->cpp, latency, wp->dbuf_block_size);
+ dev_priv->wm.skl_latency.adjusted[level] = latency;
+
+ method1 = skl_wm_method1(dev_priv, wp->plane_pixel_rate, wp->cpp,
+ dev_priv->wm.skl_latency.adjusted[level],
+ wp->dbuf_block_size);
method2 = skl_wm_method2(wp->plane_pixel_rate,
cstate->base.adjusted_mode.crtc_htotal,
- latency,
+ dev_priv->wm.skl_latency.adjusted[level],
wp->plane_blocks_per_line);
if (wp->y_tiled) {
@@ -4555,7 +4554,7 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
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)
+ else if (dev_priv->wm.skl_latency.adjusted[level] >= wp->linetime_us)
selected_result = min_fixed16(method1, method2);
else
selected_result = method1;
@@ -4634,7 +4633,7 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
}
static int
-skl_compute_wm_levels(const struct drm_i915_private *dev_priv,
+skl_compute_wm_levels(struct drm_i915_private *dev_priv,
struct skl_ddb_allocation *ddb,
struct intel_crtc_state *cstate,
const struct intel_plane_state *intel_pstate,
@@ -4756,7 +4755,7 @@ static int skl_build_pipe_wm(struct intel_crtc_state *cstate,
{
struct drm_device *dev = cstate->base.crtc->dev;
struct drm_crtc_state *crtc_state = &cstate->base;
- const struct drm_i915_private *dev_priv = to_i915(dev);
+ struct drm_i915_private *dev_priv = to_i915(dev);
struct drm_plane *plane;
const struct drm_plane_state *pstate;
struct skl_plane_wm *wm;
--
2.13.6
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 6+ messages in thread
* ✗ Fi.CI.CHECKPATCH: warning for drm/i915: Explicit Raw and Adjusted WM latencies.
2018-02-21 1:51 [PATCH] drm/i915: Explicit Raw and Adjusted WM latencies Rodrigo Vivi
@ 2018-02-21 2:13 ` Patchwork
2018-02-21 2:27 ` ✗ Fi.CI.BAT: failure " Patchwork
2018-02-21 13:09 ` [PATCH] " Ville Syrjälä
2 siblings, 0 replies; 6+ messages in thread
From: Patchwork @ 2018-02-21 2:13 UTC (permalink / raw)
To: Rodrigo Vivi; +Cc: intel-gfx
== Series Details ==
Series: drm/i915: Explicit Raw and Adjusted WM latencies.
URL : https://patchwork.freedesktop.org/series/38652/
State : warning
== Summary ==
$ dim checkpatch origin/drm-tip
8568ad0cbdd4 drm/i915: Explicit Raw and Adjusted WM latencies.
-:56: CHECK: braces {} should be used on all arms of this statement
#56: FILE: drivers/gpu/drm/i915/i915_debugfs.c:3711:
+ if (INTEL_GEN(dev_priv) >= 9) {
[...]
+ } else
[...]
-:58: WARNING: line over 80 characters
#58: FILE: drivers/gpu/drm/i915/i915_debugfs.c:3713:
+ wm_latency_show(m, dev_priv->wm.skl_latency.adjusted, "Adjusted");
-:59: CHECK: Unbalanced braces around else statement
#59: FILE: drivers/gpu/drm/i915/i915_debugfs.c:3714:
+ } else
-:76: CHECK: braces {} should be used on all arms of this statement
#76: FILE: drivers/gpu/drm/i915/i915_debugfs.c:3724:
+ if (INTEL_GEN(dev_priv) >= 9) {
[...]
+ } else
[...]
-:78: WARNING: line over 80 characters
#78: FILE: drivers/gpu/drm/i915/i915_debugfs.c:3726:
+ wm_latency_show(m, dev_priv->wm.skl_latency.adjusted, "Adjusted");
-:79: CHECK: Unbalanced braces around else statement
#79: FILE: drivers/gpu/drm/i915/i915_debugfs.c:3727:
+ } else
-:96: CHECK: braces {} should be used on all arms of this statement
#96: FILE: drivers/gpu/drm/i915/i915_debugfs.c:3737:
+ if (INTEL_GEN(dev_priv) >= 9) {
[...]
+ } else
[...]
-:98: WARNING: line over 80 characters
#98: FILE: drivers/gpu/drm/i915/i915_debugfs.c:3739:
+ wm_latency_show(m, dev_priv->wm.skl_latency.adjusted, "Adjusted");
-:99: CHECK: Unbalanced braces around else statement
#99: FILE: drivers/gpu/drm/i915/i915_debugfs.c:3740:
+ } else
-:146: CHECK: Prefer kernel type 'u16' over 'uint16_t'
#146: FILE: drivers/gpu/drm/i915/i915_drv.h:2150:
+ uint16_t raw[8];
-:147: CHECK: Prefer kernel type 'u16' over 'uint16_t'
#147: FILE: drivers/gpu/drm/i915/i915_drv.h:2151:
+ uint16_t adjusted[8];
-:196: CHECK: Prefer kernel type 'u16' over 'uint16_t'
#196: FILE: drivers/gpu/drm/i915/intel_pm.c:4503:
+ uint16_t latency = dev_priv->wm.skl_latency.raw[level];
-:223: WARNING: line over 80 characters
#223: FILE: drivers/gpu/drm/i915/intel_pm.c:4547:
+ else if (dev_priv->wm.skl_latency.adjusted[level] >= wp->linetime_us)
total: 0 errors, 4 warnings, 9 checks, 196 lines checked
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 6+ messages in thread
* ✗ Fi.CI.BAT: failure for drm/i915: Explicit Raw and Adjusted WM latencies.
2018-02-21 1:51 [PATCH] drm/i915: Explicit Raw and Adjusted WM latencies Rodrigo Vivi
2018-02-21 2:13 ` ✗ Fi.CI.CHECKPATCH: warning for " Patchwork
@ 2018-02-21 2:27 ` Patchwork
2018-02-21 13:09 ` [PATCH] " Ville Syrjälä
2 siblings, 0 replies; 6+ messages in thread
From: Patchwork @ 2018-02-21 2:27 UTC (permalink / raw)
To: Rodrigo Vivi; +Cc: intel-gfx
== Series Details ==
Series: drm/i915: Explicit Raw and Adjusted WM latencies.
URL : https://patchwork.freedesktop.org/series/38652/
State : failure
== Summary ==
Series 38652v1 drm/i915: Explicit Raw and Adjusted WM latencies.
https://patchwork.freedesktop.org/api/1.0/series/38652/revisions/1/mbox/
Test gem_mmap_gtt:
Subgroup basic-small-bo-tiledx:
pass -> FAIL (fi-gdg-551) fdo#102575
Test prime_vgem:
Subgroup basic-fence-flip:
pass -> FAIL (fi-skl-6260u)
fdo#102575 https://bugs.freedesktop.org/show_bug.cgi?id=102575
fi-bdw-5557u total:288 pass:267 dwarn:0 dfail:0 fail:0 skip:21 time:414s
fi-bdw-gvtdvm total:288 pass:264 dwarn:0 dfail:0 fail:0 skip:24 time:421s
fi-blb-e6850 total:288 pass:223 dwarn:1 dfail:0 fail:0 skip:64 time:372s
fi-bsw-n3050 total:288 pass:242 dwarn:0 dfail:0 fail:0 skip:46 time:486s
fi-bwr-2160 total:288 pass:183 dwarn:0 dfail:0 fail:0 skip:105 time:283s
fi-bxt-dsi total:288 pass:258 dwarn:0 dfail:0 fail:0 skip:30 time:476s
fi-bxt-j4205 total:288 pass:259 dwarn:0 dfail:0 fail:0 skip:29 time:479s
fi-byt-j1900 total:288 pass:253 dwarn:0 dfail:0 fail:0 skip:35 time:465s
fi-byt-n2820 total:288 pass:249 dwarn:0 dfail:0 fail:0 skip:39 time:451s
fi-cfl-s2 total:288 pass:262 dwarn:0 dfail:0 fail:0 skip:26 time:559s
fi-elk-e7500 total:288 pass:229 dwarn:0 dfail:0 fail:0 skip:59 time:415s
fi-gdg-551 total:288 pass:179 dwarn:0 dfail:0 fail:1 skip:108 time:282s
fi-glk-1 total:288 pass:260 dwarn:0 dfail:0 fail:0 skip:28 time:507s
fi-hsw-4770 total:288 pass:261 dwarn:0 dfail:0 fail:0 skip:27 time:384s
fi-ilk-650 total:288 pass:228 dwarn:0 dfail:0 fail:0 skip:60 time:407s
fi-ivb-3520m total:288 pass:259 dwarn:0 dfail:0 fail:0 skip:29 time:453s
fi-ivb-3770 total:288 pass:255 dwarn:0 dfail:0 fail:0 skip:33 time:416s
fi-kbl-7500u total:288 pass:263 dwarn:1 dfail:0 fail:0 skip:24 time:448s
fi-kbl-7560u total:288 pass:269 dwarn:0 dfail:0 fail:0 skip:19 time:490s
fi-kbl-7567u total:288 pass:268 dwarn:0 dfail:0 fail:0 skip:20 time:447s
fi-kbl-r total:288 pass:261 dwarn:0 dfail:0 fail:0 skip:27 time:491s
fi-pnv-d510 total:288 pass:222 dwarn:1 dfail:0 fail:0 skip:65 time:583s
fi-skl-6260u total:288 pass:267 dwarn:0 dfail:0 fail:1 skip:20 time:427s
fi-skl-6600u total:288 pass:261 dwarn:0 dfail:0 fail:0 skip:27 time:500s
fi-skl-6700hq total:288 pass:262 dwarn:0 dfail:0 fail:0 skip:26 time:516s
fi-skl-6700k2 total:288 pass:264 dwarn:0 dfail:0 fail:0 skip:24 time:482s
fi-skl-6770hq total:288 pass:268 dwarn:0 dfail:0 fail:0 skip:20 time:473s
fi-skl-guc total:288 pass:260 dwarn:0 dfail:0 fail:0 skip:28 time:407s
fi-skl-gvtdvm total:288 pass:265 dwarn:0 dfail:0 fail:0 skip:23 time:427s
fi-snb-2520m total:288 pass:248 dwarn:0 dfail:0 fail:0 skip:40 time:527s
fi-snb-2600 total:288 pass:248 dwarn:0 dfail:0 fail:0 skip:40 time:393s
f727568c3b37c1349f635efcc67d64ac3cf77108 drm-tip: 2018y-02m-20d-20h-39m-03s UTC integration manifest
8568ad0cbdd4 drm/i915: Explicit Raw and Adjusted WM latencies.
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_8092/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/i915: Explicit Raw and Adjusted WM latencies.
2018-02-21 1:51 [PATCH] drm/i915: Explicit Raw and Adjusted WM latencies Rodrigo Vivi
2018-02-21 2:13 ` ✗ Fi.CI.CHECKPATCH: warning for " Patchwork
2018-02-21 2:27 ` ✗ Fi.CI.BAT: failure " Patchwork
@ 2018-02-21 13:09 ` Ville Syrjälä
2018-02-21 15:53 ` Rodrigo Vivi
2 siblings, 1 reply; 6+ messages in thread
From: Ville Syrjälä @ 2018-02-21 13:09 UTC (permalink / raw)
To: Rodrigo Vivi; +Cc: intel-gfx, Ashar Shaikh
On Tue, Feb 20, 2018 at 05:51:47PM -0800, Rodrigo Vivi wrote:
> Current code has some limitations:
>
> 1. debugfs only shows raw latency we read from PCODE,
> not the ones we are configuring.
>
> 2. When determining if SAGV can be enabled we only
> apply adjusted wa, but we don't apply the IPC one.
> So there is the risk of enabling SAGV when we should
> actually disable it.
>
> Cc: Mahesh Kumar <mahesh1.kumar@intel.com>
> Cc: Ashar Shaikh <azhar.shaikh@intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
> drivers/gpu/drm/i915/i915_debugfs.c | 48 ++++++++++++++++++-------------------
> drivers/gpu/drm/i915/i915_drv.h | 7 ++++--
> drivers/gpu/drm/i915/intel_pm.c | 31 ++++++++++++------------
> 3 files changed, 43 insertions(+), 43 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index f260bb39d733..94fcb0360b14 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -3664,7 +3664,8 @@ static const struct file_operations i915_displayport_test_type_fops = {
> .release = single_release
> };
>
> -static void wm_latency_show(struct seq_file *m, const uint16_t wm[8])
> +static void wm_latency_show(struct seq_file *m, const uint16_t wm[8],
> + const char *header)
> {
> struct drm_i915_private *dev_priv = m->private;
> struct drm_device *dev = &dev_priv->drm;
> @@ -3680,6 +3681,9 @@ static void wm_latency_show(struct seq_file *m, const uint16_t wm[8])
> else
> num_levels = ilk_wm_max_level(dev_priv) + 1;
>
> + if (header)
> + seq_printf(m, "%s\n", header);
> +
> drm_modeset_lock_all(dev);
>
> for (level = 0; level < num_levels; level++) {
> @@ -3707,14 +3711,12 @@ static void wm_latency_show(struct seq_file *m, const uint16_t wm[8])
> static int pri_wm_latency_show(struct seq_file *m, void *data)
> {
> struct drm_i915_private *dev_priv = m->private;
> - const uint16_t *latencies;
> -
> - if (INTEL_GEN(dev_priv) >= 9)
> - latencies = dev_priv->wm.skl_latency;
> - else
> - latencies = dev_priv->wm.pri_latency;
>
> - wm_latency_show(m, latencies);
> + if (INTEL_GEN(dev_priv) >= 9) {
> + wm_latency_show(m, dev_priv->wm.skl_latency.raw, "Raw");
> + wm_latency_show(m, dev_priv->wm.skl_latency.adjusted, "Adjusted");
> + } else
> + wm_latency_show(m, dev_priv->wm.pri_latency, NULL);
>
> return 0;
> }
> @@ -3722,14 +3724,12 @@ static int pri_wm_latency_show(struct seq_file *m, void *data)
> static int spr_wm_latency_show(struct seq_file *m, void *data)
> {
> struct drm_i915_private *dev_priv = m->private;
> - const uint16_t *latencies;
> -
> - if (INTEL_GEN(dev_priv) >= 9)
> - latencies = dev_priv->wm.skl_latency;
> - else
> - latencies = dev_priv->wm.spr_latency;
>
> - wm_latency_show(m, latencies);
> + if (INTEL_GEN(dev_priv) >= 9) {
> + wm_latency_show(m, dev_priv->wm.skl_latency.raw, "Raw");
> + wm_latency_show(m, dev_priv->wm.skl_latency.adjusted, "Adjusted");
> + } else
> + wm_latency_show(m, dev_priv->wm.spr_latency, NULL);
>
> return 0;
> }
> @@ -3737,14 +3737,12 @@ static int spr_wm_latency_show(struct seq_file *m, void *data)
> static int cur_wm_latency_show(struct seq_file *m, void *data)
> {
> struct drm_i915_private *dev_priv = m->private;
> - const uint16_t *latencies;
> -
> - if (INTEL_GEN(dev_priv) >= 9)
> - latencies = dev_priv->wm.skl_latency;
> - else
> - latencies = dev_priv->wm.cur_latency;
>
> - wm_latency_show(m, latencies);
> + if (INTEL_GEN(dev_priv) >= 9) {
> + wm_latency_show(m, dev_priv->wm.skl_latency.raw, "Raw");
> + wm_latency_show(m, dev_priv->wm.skl_latency.adjusted, "Adjusted");
> + } else
> + wm_latency_show(m, dev_priv->wm.cur_latency, NULL);
>
> return 0;
> }
> @@ -3833,7 +3831,7 @@ static ssize_t pri_wm_latency_write(struct file *file, const char __user *ubuf,
> uint16_t *latencies;
>
> if (INTEL_GEN(dev_priv) >= 9)
> - latencies = dev_priv->wm.skl_latency;
> + latencies = dev_priv->wm.skl_latency.raw;
> else
> latencies = dev_priv->wm.pri_latency;
>
> @@ -3848,7 +3846,7 @@ static ssize_t spr_wm_latency_write(struct file *file, const char __user *ubuf,
> uint16_t *latencies;
>
> if (INTEL_GEN(dev_priv) >= 9)
> - latencies = dev_priv->wm.skl_latency;
> + latencies = dev_priv->wm.skl_latency.raw;
> else
> latencies = dev_priv->wm.spr_latency;
>
> @@ -3863,7 +3861,7 @@ static ssize_t cur_wm_latency_write(struct file *file, const char __user *ubuf,
> uint16_t *latencies;
>
> if (INTEL_GEN(dev_priv) >= 9)
> - latencies = dev_priv->wm.skl_latency;
> + latencies = dev_priv->wm.skl_latency.raw;
> else
> latencies = dev_priv->wm.cur_latency;
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 490ff041fb1e..6969bbb203bc 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2136,11 +2136,14 @@ struct drm_i915_private {
> /* cursor */
> uint16_t cur_latency[5];
> /*
> - * Raw watermark memory latency values
> + * Watermark memory latency values
> * for SKL for all 8 levels
> * in 1us units.
> */
> - uint16_t skl_latency[8];
> + struct {
> + uint16_t raw[8];
> + uint16_t adjusted[8];
> + } skl_latency;
>
> /* current hardware state */
> union {
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index a0a6b4b7c47b..78b52e5a1f8e 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3030,8 +3030,9 @@ static void ilk_setup_wm_latency(struct drm_i915_private *dev_priv)
>
> static void skl_setup_wm_latency(struct drm_i915_private *dev_priv)
> {
> - intel_read_wm_latency(dev_priv, dev_priv->wm.skl_latency);
> - intel_print_wm_latency(dev_priv, "Gen9 Plane", dev_priv->wm.skl_latency);
> + intel_read_wm_latency(dev_priv, dev_priv->wm.skl_latency.raw);
> + intel_print_wm_latency(dev_priv, "Gen9 Plane",
> + dev_priv->wm.skl_latency.raw);
> }
>
> static bool ilk_validate_pipe_wm(struct drm_device *dev,
> @@ -3745,12 +3746,7 @@ bool intel_can_enable_sagv(struct drm_atomic_state *state)
> !wm->wm[level].plane_en; --level)
> { }
>
> - latency = dev_priv->wm.skl_latency[level];
> -
> - if (skl_needs_memory_bw_wa(intel_state) &&
> - plane->base.state->fb->modifier ==
> - I915_FORMAT_MOD_X_TILED)
> - latency += 15;
> + latency = dev_priv->wm.skl_latency.adjusted[level];
The latency adjustment depends on plane configuratiuon so we can't just
stick into a global array. I would suggest that for cases like this we
should either stuck into the plane state and dump it out alongside the
rest (if we had decent plane state dumps... as usual I have a branch
somewhere that converts us over to the atomic state dump framework), or
we just have a debug print when computing the thing.
>
> /*
> * If any of the planes on this pipe don't enable wm levels that
> @@ -4503,7 +4499,7 @@ skl_compute_plane_wm_params(const struct drm_i915_private *dev_priv,
> return 0;
> }
>
> -static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
> +static int skl_compute_plane_wm(struct drm_i915_private *dev_priv,
> struct intel_crtc_state *cstate,
> const struct intel_plane_state *intel_pstate,
> uint16_t ddb_allocation,
> @@ -4514,7 +4510,7 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
> bool *enabled /* out */)
> {
> const struct drm_plane_state *pstate = &intel_pstate->base;
> - uint32_t latency = dev_priv->wm.skl_latency[level];
> + uint16_t latency = dev_priv->wm.skl_latency.raw[level];
> uint_fixed_16_16_t method1, method2;
> uint_fixed_16_16_t selected_result;
> uint32_t res_blocks, res_lines;
> @@ -4538,11 +4534,14 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
> if (apply_memory_bw_wa && wp->x_tiled)
> latency += 15;
>
> - method1 = skl_wm_method1(dev_priv, wp->plane_pixel_rate,
> - wp->cpp, latency, wp->dbuf_block_size);
> + dev_priv->wm.skl_latency.adjusted[level] = latency;
> +
> + method1 = skl_wm_method1(dev_priv, wp->plane_pixel_rate, wp->cpp,
> + dev_priv->wm.skl_latency.adjusted[level],
> + wp->dbuf_block_size);
> method2 = skl_wm_method2(wp->plane_pixel_rate,
> cstate->base.adjusted_mode.crtc_htotal,
> - latency,
> + dev_priv->wm.skl_latency.adjusted[level],
> wp->plane_blocks_per_line);
>
> if (wp->y_tiled) {
> @@ -4555,7 +4554,7 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
> 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)
> + else if (dev_priv->wm.skl_latency.adjusted[level] >= wp->linetime_us)
> selected_result = min_fixed16(method1, method2);
> else
> selected_result = method1;
> @@ -4634,7 +4633,7 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
> }
>
> static int
> -skl_compute_wm_levels(const struct drm_i915_private *dev_priv,
> +skl_compute_wm_levels(struct drm_i915_private *dev_priv,
> struct skl_ddb_allocation *ddb,
> struct intel_crtc_state *cstate,
> const struct intel_plane_state *intel_pstate,
> @@ -4756,7 +4755,7 @@ static int skl_build_pipe_wm(struct intel_crtc_state *cstate,
> {
> struct drm_device *dev = cstate->base.crtc->dev;
> struct drm_crtc_state *crtc_state = &cstate->base;
> - const struct drm_i915_private *dev_priv = to_i915(dev);
> + struct drm_i915_private *dev_priv = to_i915(dev);
> struct drm_plane *plane;
> const struct drm_plane_state *pstate;
> struct skl_plane_wm *wm;
> --
> 2.13.6
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
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] 6+ messages in thread
* Re: [PATCH] drm/i915: Explicit Raw and Adjusted WM latencies.
2018-02-21 13:09 ` [PATCH] " Ville Syrjälä
@ 2018-02-21 15:53 ` Rodrigo Vivi
2018-02-21 16:26 ` Ville Syrjälä
0 siblings, 1 reply; 6+ messages in thread
From: Rodrigo Vivi @ 2018-02-21 15:53 UTC (permalink / raw)
To: Ville Syrjälä; +Cc: intel-gfx, Ashar Shaikh
On Wed, Feb 21, 2018 at 03:09:30PM +0200, Ville Syrjälä wrote:
> On Tue, Feb 20, 2018 at 05:51:47PM -0800, Rodrigo Vivi wrote:
> > Current code has some limitations:
> >
> > 1. debugfs only shows raw latency we read from PCODE,
> > not the ones we are configuring.
> >
> > 2. When determining if SAGV can be enabled we only
> > apply adjusted wa, but we don't apply the IPC one.
> > So there is the risk of enabling SAGV when we should
> > actually disable it.
> >
> > Cc: Mahesh Kumar <mahesh1.kumar@intel.com>
> > Cc: Ashar Shaikh <azhar.shaikh@intel.com>
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > ---
> > drivers/gpu/drm/i915/i915_debugfs.c | 48 ++++++++++++++++++-------------------
> > drivers/gpu/drm/i915/i915_drv.h | 7 ++++--
> > drivers/gpu/drm/i915/intel_pm.c | 31 ++++++++++++------------
> > 3 files changed, 43 insertions(+), 43 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> > index f260bb39d733..94fcb0360b14 100644
> > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > @@ -3664,7 +3664,8 @@ static const struct file_operations i915_displayport_test_type_fops = {
> > .release = single_release
> > };
> >
> > -static void wm_latency_show(struct seq_file *m, const uint16_t wm[8])
> > +static void wm_latency_show(struct seq_file *m, const uint16_t wm[8],
> > + const char *header)
> > {
> > struct drm_i915_private *dev_priv = m->private;
> > struct drm_device *dev = &dev_priv->drm;
> > @@ -3680,6 +3681,9 @@ static void wm_latency_show(struct seq_file *m, const uint16_t wm[8])
> > else
> > num_levels = ilk_wm_max_level(dev_priv) + 1;
> >
> > + if (header)
> > + seq_printf(m, "%s\n", header);
> > +
> > drm_modeset_lock_all(dev);
> >
> > for (level = 0; level < num_levels; level++) {
> > @@ -3707,14 +3711,12 @@ static void wm_latency_show(struct seq_file *m, const uint16_t wm[8])
> > static int pri_wm_latency_show(struct seq_file *m, void *data)
> > {
> > struct drm_i915_private *dev_priv = m->private;
> > - const uint16_t *latencies;
> > -
> > - if (INTEL_GEN(dev_priv) >= 9)
> > - latencies = dev_priv->wm.skl_latency;
> > - else
> > - latencies = dev_priv->wm.pri_latency;
> >
> > - wm_latency_show(m, latencies);
> > + if (INTEL_GEN(dev_priv) >= 9) {
> > + wm_latency_show(m, dev_priv->wm.skl_latency.raw, "Raw");
> > + wm_latency_show(m, dev_priv->wm.skl_latency.adjusted, "Adjusted");
> > + } else
> > + wm_latency_show(m, dev_priv->wm.pri_latency, NULL);
> >
> > return 0;
> > }
> > @@ -3722,14 +3724,12 @@ static int pri_wm_latency_show(struct seq_file *m, void *data)
> > static int spr_wm_latency_show(struct seq_file *m, void *data)
> > {
> > struct drm_i915_private *dev_priv = m->private;
> > - const uint16_t *latencies;
> > -
> > - if (INTEL_GEN(dev_priv) >= 9)
> > - latencies = dev_priv->wm.skl_latency;
> > - else
> > - latencies = dev_priv->wm.spr_latency;
> >
> > - wm_latency_show(m, latencies);
> > + if (INTEL_GEN(dev_priv) >= 9) {
> > + wm_latency_show(m, dev_priv->wm.skl_latency.raw, "Raw");
> > + wm_latency_show(m, dev_priv->wm.skl_latency.adjusted, "Adjusted");
> > + } else
> > + wm_latency_show(m, dev_priv->wm.spr_latency, NULL);
> >
> > return 0;
> > }
> > @@ -3737,14 +3737,12 @@ static int spr_wm_latency_show(struct seq_file *m, void *data)
> > static int cur_wm_latency_show(struct seq_file *m, void *data)
> > {
> > struct drm_i915_private *dev_priv = m->private;
> > - const uint16_t *latencies;
> > -
> > - if (INTEL_GEN(dev_priv) >= 9)
> > - latencies = dev_priv->wm.skl_latency;
> > - else
> > - latencies = dev_priv->wm.cur_latency;
> >
> > - wm_latency_show(m, latencies);
> > + if (INTEL_GEN(dev_priv) >= 9) {
> > + wm_latency_show(m, dev_priv->wm.skl_latency.raw, "Raw");
> > + wm_latency_show(m, dev_priv->wm.skl_latency.adjusted, "Adjusted");
> > + } else
> > + wm_latency_show(m, dev_priv->wm.cur_latency, NULL);
> >
> > return 0;
> > }
> > @@ -3833,7 +3831,7 @@ static ssize_t pri_wm_latency_write(struct file *file, const char __user *ubuf,
> > uint16_t *latencies;
> >
> > if (INTEL_GEN(dev_priv) >= 9)
> > - latencies = dev_priv->wm.skl_latency;
> > + latencies = dev_priv->wm.skl_latency.raw;
> > else
> > latencies = dev_priv->wm.pri_latency;
> >
> > @@ -3848,7 +3846,7 @@ static ssize_t spr_wm_latency_write(struct file *file, const char __user *ubuf,
> > uint16_t *latencies;
> >
> > if (INTEL_GEN(dev_priv) >= 9)
> > - latencies = dev_priv->wm.skl_latency;
> > + latencies = dev_priv->wm.skl_latency.raw;
> > else
> > latencies = dev_priv->wm.spr_latency;
> >
> > @@ -3863,7 +3861,7 @@ static ssize_t cur_wm_latency_write(struct file *file, const char __user *ubuf,
> > uint16_t *latencies;
> >
> > if (INTEL_GEN(dev_priv) >= 9)
> > - latencies = dev_priv->wm.skl_latency;
> > + latencies = dev_priv->wm.skl_latency.raw;
> > else
> > latencies = dev_priv->wm.cur_latency;
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 490ff041fb1e..6969bbb203bc 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -2136,11 +2136,14 @@ struct drm_i915_private {
> > /* cursor */
> > uint16_t cur_latency[5];
> > /*
> > - * Raw watermark memory latency values
> > + * Watermark memory latency values
> > * for SKL for all 8 levels
> > * in 1us units.
> > */
> > - uint16_t skl_latency[8];
> > + struct {
> > + uint16_t raw[8];
> > + uint16_t adjusted[8];
> > + } skl_latency;
> >
> > /* current hardware state */
> > union {
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index a0a6b4b7c47b..78b52e5a1f8e 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -3030,8 +3030,9 @@ static void ilk_setup_wm_latency(struct drm_i915_private *dev_priv)
> >
> > static void skl_setup_wm_latency(struct drm_i915_private *dev_priv)
> > {
> > - intel_read_wm_latency(dev_priv, dev_priv->wm.skl_latency);
> > - intel_print_wm_latency(dev_priv, "Gen9 Plane", dev_priv->wm.skl_latency);
> > + intel_read_wm_latency(dev_priv, dev_priv->wm.skl_latency.raw);
> > + intel_print_wm_latency(dev_priv, "Gen9 Plane",
> > + dev_priv->wm.skl_latency.raw);
> > }
> >
> > static bool ilk_validate_pipe_wm(struct drm_device *dev,
> > @@ -3745,12 +3746,7 @@ bool intel_can_enable_sagv(struct drm_atomic_state *state)
> > !wm->wm[level].plane_en; --level)
> > { }
> >
> > - latency = dev_priv->wm.skl_latency[level];
> > -
> > - if (skl_needs_memory_bw_wa(intel_state) &&
> > - plane->base.state->fb->modifier ==
> > - I915_FORMAT_MOD_X_TILED)
> > - latency += 15;
> > + latency = dev_priv->wm.skl_latency.adjusted[level];
>
> The latency adjustment depends on plane configuratiuon so we can't just
> stick into a global array.
Duh! Indeed...
> I would suggest that for cases like this we
> should either stuck into the plane state and dump it out alongside the
> rest (if we had decent plane state dumps... as usual I have a branch
> somewhere that converts us over to the atomic state dump framework), or
> we just have a debug print when computing the thing.
on debug side a debug message is ok.
But I see other bugs on the current code:
SAGV adjusted latency is missing some cases that we consider for latency++
on plane calculation.
So I thought that for this we could simply have an unified function.
But also I'm not sure if this will actually be effective. I don't believe
we are executing the disable sequence as expected when some plane has latency below
that threshhold.
So, maybe add this unified function but also on atomic_commit_tail we do more of
if (!intel_can_enable_sagv(state))
intel_disable_sagv(dev_priv);
not only on full modeset...
maybe something like:
if (intel_can_enable_sagv(state)) {
if (disabled) intel_enable_sagv()
} else {
if (enabled) intel_disable_sagv()
}
?! :/
I really believe we can be smarter there... I'm just out of good ideas.
>
> >
> > /*
> > * If any of the planes on this pipe don't enable wm levels that
> > @@ -4503,7 +4499,7 @@ skl_compute_plane_wm_params(const struct drm_i915_private *dev_priv,
> > return 0;
> > }
> >
> > -static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
> > +static int skl_compute_plane_wm(struct drm_i915_private *dev_priv,
> > struct intel_crtc_state *cstate,
> > const struct intel_plane_state *intel_pstate,
> > uint16_t ddb_allocation,
> > @@ -4514,7 +4510,7 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
> > bool *enabled /* out */)
> > {
> > const struct drm_plane_state *pstate = &intel_pstate->base;
> > - uint32_t latency = dev_priv->wm.skl_latency[level];
> > + uint16_t latency = dev_priv->wm.skl_latency.raw[level];
> > uint_fixed_16_16_t method1, method2;
> > uint_fixed_16_16_t selected_result;
> > uint32_t res_blocks, res_lines;
> > @@ -4538,11 +4534,14 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
> > if (apply_memory_bw_wa && wp->x_tiled)
> > latency += 15;
> >
> > - method1 = skl_wm_method1(dev_priv, wp->plane_pixel_rate,
> > - wp->cpp, latency, wp->dbuf_block_size);
> > + dev_priv->wm.skl_latency.adjusted[level] = latency;
> > +
> > + method1 = skl_wm_method1(dev_priv, wp->plane_pixel_rate, wp->cpp,
> > + dev_priv->wm.skl_latency.adjusted[level],
> > + wp->dbuf_block_size);
> > method2 = skl_wm_method2(wp->plane_pixel_rate,
> > cstate->base.adjusted_mode.crtc_htotal,
> > - latency,
> > + dev_priv->wm.skl_latency.adjusted[level],
> > wp->plane_blocks_per_line);
> >
> > if (wp->y_tiled) {
> > @@ -4555,7 +4554,7 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
> > 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)
> > + else if (dev_priv->wm.skl_latency.adjusted[level] >= wp->linetime_us)
> > selected_result = min_fixed16(method1, method2);
> > else
> > selected_result = method1;
> > @@ -4634,7 +4633,7 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
> > }
> >
> > static int
> > -skl_compute_wm_levels(const struct drm_i915_private *dev_priv,
> > +skl_compute_wm_levels(struct drm_i915_private *dev_priv,
> > struct skl_ddb_allocation *ddb,
> > struct intel_crtc_state *cstate,
> > const struct intel_plane_state *intel_pstate,
> > @@ -4756,7 +4755,7 @@ static int skl_build_pipe_wm(struct intel_crtc_state *cstate,
> > {
> > struct drm_device *dev = cstate->base.crtc->dev;
> > struct drm_crtc_state *crtc_state = &cstate->base;
> > - const struct drm_i915_private *dev_priv = to_i915(dev);
> > + struct drm_i915_private *dev_priv = to_i915(dev);
> > struct drm_plane *plane;
> > const struct drm_plane_state *pstate;
> > struct skl_plane_wm *wm;
> > --
> > 2.13.6
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> 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] 6+ messages in thread
* Re: [PATCH] drm/i915: Explicit Raw and Adjusted WM latencies.
2018-02-21 15:53 ` Rodrigo Vivi
@ 2018-02-21 16:26 ` Ville Syrjälä
0 siblings, 0 replies; 6+ messages in thread
From: Ville Syrjälä @ 2018-02-21 16:26 UTC (permalink / raw)
To: Rodrigo Vivi; +Cc: intel-gfx, Ashar Shaikh
On Wed, Feb 21, 2018 at 07:53:12AM -0800, Rodrigo Vivi wrote:
> On Wed, Feb 21, 2018 at 03:09:30PM +0200, Ville Syrjälä wrote:
> > On Tue, Feb 20, 2018 at 05:51:47PM -0800, Rodrigo Vivi wrote:
> > > Current code has some limitations:
> > >
> > > 1. debugfs only shows raw latency we read from PCODE,
> > > not the ones we are configuring.
> > >
> > > 2. When determining if SAGV can be enabled we only
> > > apply adjusted wa, but we don't apply the IPC one.
> > > So there is the risk of enabling SAGV when we should
> > > actually disable it.
> > >
> > > Cc: Mahesh Kumar <mahesh1.kumar@intel.com>
> > > Cc: Ashar Shaikh <azhar.shaikh@intel.com>
> > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > ---
> > > drivers/gpu/drm/i915/i915_debugfs.c | 48 ++++++++++++++++++-------------------
> > > drivers/gpu/drm/i915/i915_drv.h | 7 ++++--
> > > drivers/gpu/drm/i915/intel_pm.c | 31 ++++++++++++------------
> > > 3 files changed, 43 insertions(+), 43 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> > > index f260bb39d733..94fcb0360b14 100644
> > > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > > @@ -3664,7 +3664,8 @@ static const struct file_operations i915_displayport_test_type_fops = {
> > > .release = single_release
> > > };
> > >
> > > -static void wm_latency_show(struct seq_file *m, const uint16_t wm[8])
> > > +static void wm_latency_show(struct seq_file *m, const uint16_t wm[8],
> > > + const char *header)
> > > {
> > > struct drm_i915_private *dev_priv = m->private;
> > > struct drm_device *dev = &dev_priv->drm;
> > > @@ -3680,6 +3681,9 @@ static void wm_latency_show(struct seq_file *m, const uint16_t wm[8])
> > > else
> > > num_levels = ilk_wm_max_level(dev_priv) + 1;
> > >
> > > + if (header)
> > > + seq_printf(m, "%s\n", header);
> > > +
> > > drm_modeset_lock_all(dev);
> > >
> > > for (level = 0; level < num_levels; level++) {
> > > @@ -3707,14 +3711,12 @@ static void wm_latency_show(struct seq_file *m, const uint16_t wm[8])
> > > static int pri_wm_latency_show(struct seq_file *m, void *data)
> > > {
> > > struct drm_i915_private *dev_priv = m->private;
> > > - const uint16_t *latencies;
> > > -
> > > - if (INTEL_GEN(dev_priv) >= 9)
> > > - latencies = dev_priv->wm.skl_latency;
> > > - else
> > > - latencies = dev_priv->wm.pri_latency;
> > >
> > > - wm_latency_show(m, latencies);
> > > + if (INTEL_GEN(dev_priv) >= 9) {
> > > + wm_latency_show(m, dev_priv->wm.skl_latency.raw, "Raw");
> > > + wm_latency_show(m, dev_priv->wm.skl_latency.adjusted, "Adjusted");
> > > + } else
> > > + wm_latency_show(m, dev_priv->wm.pri_latency, NULL);
> > >
> > > return 0;
> > > }
> > > @@ -3722,14 +3724,12 @@ static int pri_wm_latency_show(struct seq_file *m, void *data)
> > > static int spr_wm_latency_show(struct seq_file *m, void *data)
> > > {
> > > struct drm_i915_private *dev_priv = m->private;
> > > - const uint16_t *latencies;
> > > -
> > > - if (INTEL_GEN(dev_priv) >= 9)
> > > - latencies = dev_priv->wm.skl_latency;
> > > - else
> > > - latencies = dev_priv->wm.spr_latency;
> > >
> > > - wm_latency_show(m, latencies);
> > > + if (INTEL_GEN(dev_priv) >= 9) {
> > > + wm_latency_show(m, dev_priv->wm.skl_latency.raw, "Raw");
> > > + wm_latency_show(m, dev_priv->wm.skl_latency.adjusted, "Adjusted");
> > > + } else
> > > + wm_latency_show(m, dev_priv->wm.spr_latency, NULL);
> > >
> > > return 0;
> > > }
> > > @@ -3737,14 +3737,12 @@ static int spr_wm_latency_show(struct seq_file *m, void *data)
> > > static int cur_wm_latency_show(struct seq_file *m, void *data)
> > > {
> > > struct drm_i915_private *dev_priv = m->private;
> > > - const uint16_t *latencies;
> > > -
> > > - if (INTEL_GEN(dev_priv) >= 9)
> > > - latencies = dev_priv->wm.skl_latency;
> > > - else
> > > - latencies = dev_priv->wm.cur_latency;
> > >
> > > - wm_latency_show(m, latencies);
> > > + if (INTEL_GEN(dev_priv) >= 9) {
> > > + wm_latency_show(m, dev_priv->wm.skl_latency.raw, "Raw");
> > > + wm_latency_show(m, dev_priv->wm.skl_latency.adjusted, "Adjusted");
> > > + } else
> > > + wm_latency_show(m, dev_priv->wm.cur_latency, NULL);
> > >
> > > return 0;
> > > }
> > > @@ -3833,7 +3831,7 @@ static ssize_t pri_wm_latency_write(struct file *file, const char __user *ubuf,
> > > uint16_t *latencies;
> > >
> > > if (INTEL_GEN(dev_priv) >= 9)
> > > - latencies = dev_priv->wm.skl_latency;
> > > + latencies = dev_priv->wm.skl_latency.raw;
> > > else
> > > latencies = dev_priv->wm.pri_latency;
> > >
> > > @@ -3848,7 +3846,7 @@ static ssize_t spr_wm_latency_write(struct file *file, const char __user *ubuf,
> > > uint16_t *latencies;
> > >
> > > if (INTEL_GEN(dev_priv) >= 9)
> > > - latencies = dev_priv->wm.skl_latency;
> > > + latencies = dev_priv->wm.skl_latency.raw;
> > > else
> > > latencies = dev_priv->wm.spr_latency;
> > >
> > > @@ -3863,7 +3861,7 @@ static ssize_t cur_wm_latency_write(struct file *file, const char __user *ubuf,
> > > uint16_t *latencies;
> > >
> > > if (INTEL_GEN(dev_priv) >= 9)
> > > - latencies = dev_priv->wm.skl_latency;
> > > + latencies = dev_priv->wm.skl_latency.raw;
> > > else
> > > latencies = dev_priv->wm.cur_latency;
> > >
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > > index 490ff041fb1e..6969bbb203bc 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > @@ -2136,11 +2136,14 @@ struct drm_i915_private {
> > > /* cursor */
> > > uint16_t cur_latency[5];
> > > /*
> > > - * Raw watermark memory latency values
> > > + * Watermark memory latency values
> > > * for SKL for all 8 levels
> > > * in 1us units.
> > > */
> > > - uint16_t skl_latency[8];
> > > + struct {
> > > + uint16_t raw[8];
> > > + uint16_t adjusted[8];
> > > + } skl_latency;
> > >
> > > /* current hardware state */
> > > union {
> > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > > index a0a6b4b7c47b..78b52e5a1f8e 100644
> > > --- a/drivers/gpu/drm/i915/intel_pm.c
> > > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > > @@ -3030,8 +3030,9 @@ static void ilk_setup_wm_latency(struct drm_i915_private *dev_priv)
> > >
> > > static void skl_setup_wm_latency(struct drm_i915_private *dev_priv)
> > > {
> > > - intel_read_wm_latency(dev_priv, dev_priv->wm.skl_latency);
> > > - intel_print_wm_latency(dev_priv, "Gen9 Plane", dev_priv->wm.skl_latency);
> > > + intel_read_wm_latency(dev_priv, dev_priv->wm.skl_latency.raw);
> > > + intel_print_wm_latency(dev_priv, "Gen9 Plane",
> > > + dev_priv->wm.skl_latency.raw);
> > > }
> > >
> > > static bool ilk_validate_pipe_wm(struct drm_device *dev,
> > > @@ -3745,12 +3746,7 @@ bool intel_can_enable_sagv(struct drm_atomic_state *state)
> > > !wm->wm[level].plane_en; --level)
> > > { }
> > >
> > > - latency = dev_priv->wm.skl_latency[level];
> > > -
> > > - if (skl_needs_memory_bw_wa(intel_state) &&
> > > - plane->base.state->fb->modifier ==
> > > - I915_FORMAT_MOD_X_TILED)
> > > - latency += 15;
> > > + latency = dev_priv->wm.skl_latency.adjusted[level];
> >
> > The latency adjustment depends on plane configuratiuon so we can't just
> > stick into a global array.
>
> Duh! Indeed...
>
> > I would suggest that for cases like this we
> > should either stuck into the plane state and dump it out alongside the
> > rest (if we had decent plane state dumps... as usual I have a branch
> > somewhere that converts us over to the atomic state dump framework), or
> > we just have a debug print when computing the thing.
>
> on debug side a debug message is ok.
>
> But I see other bugs on the current code:
>
> SAGV adjusted latency is missing some cases that we consider for latency++
> on plane calculation.
>
> So I thought that for this we could simply have an unified function.
>
> But also I'm not sure if this will actually be effective. I don't believe
> we are executing the disable sequence as expected when some plane has latency below
> that threshhold.
>
> So, maybe add this unified function but also on atomic_commit_tail we do more of
> if (!intel_can_enable_sagv(state))
> intel_disable_sagv(dev_priv);
>
> not only on full modeset...
>
> maybe something like:
> if (intel_can_enable_sagv(state)) {
> if (disabled) intel_enable_sagv()
> } else {
> if (enabled) intel_disable_sagv()
> }
>
> ?! :/
Yeah, seems it should be done from pre_plane_update basically. Or before
it since it's global thing. Not entirely happy with the fact that sagv
is done totally differently to cxsr even though they're kinda the same
thing (cxsr does change the actual FIFO sizes on some platforms so it's
not quite a 100% match though).
We probably should pre-compute the "sagv? yes/no" answer for each crtc
(and/or plane?) during the normal wm compute, then can_enable_sagv()
should turn into something quite trivial.
>
> >
> > >
> > > /*
> > > * If any of the planes on this pipe don't enable wm levels that
> > > @@ -4503,7 +4499,7 @@ skl_compute_plane_wm_params(const struct drm_i915_private *dev_priv,
> > > return 0;
> > > }
> > >
> > > -static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
> > > +static int skl_compute_plane_wm(struct drm_i915_private *dev_priv,
> > > struct intel_crtc_state *cstate,
> > > const struct intel_plane_state *intel_pstate,
> > > uint16_t ddb_allocation,
> > > @@ -4514,7 +4510,7 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
> > > bool *enabled /* out */)
> > > {
> > > const struct drm_plane_state *pstate = &intel_pstate->base;
> > > - uint32_t latency = dev_priv->wm.skl_latency[level];
> > > + uint16_t latency = dev_priv->wm.skl_latency.raw[level];
> > > uint_fixed_16_16_t method1, method2;
> > > uint_fixed_16_16_t selected_result;
> > > uint32_t res_blocks, res_lines;
> > > @@ -4538,11 +4534,14 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
> > > if (apply_memory_bw_wa && wp->x_tiled)
> > > latency += 15;
> > >
> > > - method1 = skl_wm_method1(dev_priv, wp->plane_pixel_rate,
> > > - wp->cpp, latency, wp->dbuf_block_size);
> > > + dev_priv->wm.skl_latency.adjusted[level] = latency;
> > > +
> > > + method1 = skl_wm_method1(dev_priv, wp->plane_pixel_rate, wp->cpp,
> > > + dev_priv->wm.skl_latency.adjusted[level],
> > > + wp->dbuf_block_size);
> > > method2 = skl_wm_method2(wp->plane_pixel_rate,
> > > cstate->base.adjusted_mode.crtc_htotal,
> > > - latency,
> > > + dev_priv->wm.skl_latency.adjusted[level],
> > > wp->plane_blocks_per_line);
> > >
> > > if (wp->y_tiled) {
> > > @@ -4555,7 +4554,7 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
> > > 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)
> > > + else if (dev_priv->wm.skl_latency.adjusted[level] >= wp->linetime_us)
> > > selected_result = min_fixed16(method1, method2);
> > > else
> > > selected_result = method1;
> > > @@ -4634,7 +4633,7 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
> > > }
> > >
> > > static int
> > > -skl_compute_wm_levels(const struct drm_i915_private *dev_priv,
> > > +skl_compute_wm_levels(struct drm_i915_private *dev_priv,
> > > struct skl_ddb_allocation *ddb,
> > > struct intel_crtc_state *cstate,
> > > const struct intel_plane_state *intel_pstate,
> > > @@ -4756,7 +4755,7 @@ static int skl_build_pipe_wm(struct intel_crtc_state *cstate,
> > > {
> > > struct drm_device *dev = cstate->base.crtc->dev;
> > > struct drm_crtc_state *crtc_state = &cstate->base;
> > > - const struct drm_i915_private *dev_priv = to_i915(dev);
> > > + struct drm_i915_private *dev_priv = to_i915(dev);
> > > struct drm_plane *plane;
> > > const struct drm_plane_state *pstate;
> > > struct skl_plane_wm *wm;
> > > --
> > > 2.13.6
> > >
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >
> > --
> > Ville Syrjälä
> > Intel OTC
--
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] 6+ messages in thread
end of thread, other threads:[~2018-02-21 16:26 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-02-21 1:51 [PATCH] drm/i915: Explicit Raw and Adjusted WM latencies Rodrigo Vivi
2018-02-21 2:13 ` ✗ Fi.CI.CHECKPATCH: warning for " Patchwork
2018-02-21 2:27 ` ✗ Fi.CI.BAT: failure " Patchwork
2018-02-21 13:09 ` [PATCH] " Ville Syrjälä
2018-02-21 15:53 ` Rodrigo Vivi
2018-02-21 16:26 ` Ville Syrjälä
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).