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@xxxxxxxxx>
> > Cc: Ashar Shaikh <azhar.shaikh@xxxxxxxxx>
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
> > ---
> > 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@xxxxxxxxxxxxxxxxxxxxx
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Ville Syrjälä
> Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
