| Message ID | 20180221015147.31859-1-rodrigo.vivi@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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
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;
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(-)