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.ku...@intel.com>
> Cc: Ashar Shaikh <azhar.sha...@intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.v...@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
