Re: [PATCH] drm/i915: Explicit Raw and Adjusted WM latencies.

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux