[Intel-gfx] [PATCH v3 2/2] drm/i915: Make sure we have enough memory bandwidth on ICL

Sripada, Radhakrishna radhakrishna.sripada at intel.com
Tue May 14 00:59:26 UTC 2019


On Mon, 2019-05-13 at 17:16 +0300, Ville Syrjälä wrote:
> On Wed, May 08, 2019 at 09:05:06PM +0000, Sripada, Radhakrishna
> wrote:
> > On Fri, 2019-05-03 at 22:08 +0300, Ville Syrjala wrote:
> > > From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > > 
> > > ICL has so many planes that it can easily exceed the maximum
> > > effective memory bandwidth of the system. We must therefore check
> > > that we don't exceed that limit.
> > > 
> > > The algorithm is very magic number heavy and lacks sufficient
> > > explanation for now. We also have no sane way to query the
> > > memory clock and timings, so we must rely on a combination of
> > > raw readout from the memory controller and hardcoded assumptions.
> > > The memory controller values obviously change as the system
> > > jumps between the different SAGV points, so we try to stabilize
> > > it first by disabling SAGV for the duration of the readout.
> > > 
> > > The utilized bandwidth is tracked via a device wide atomic
> > > private object. That is actually not robust because we can't
> > > afford to enforce strict global ordering between the pipes.
> > > Thus I think I'll need to change this to simply chop up the
> > > available bandwidth between all the active pipes. Each pipe
> > > can then do whatever it wants as long as it doesn't exceed
> > > its budget. That scheme will also require that we assume that
> > > any number of planes could be active at any time.
> > > 
> > > TODO: make it robust and deal with all the open questions
> > > 
> > > v2: Sleep longer after disabling SAGV
> > > v3: Poll for the dclk to get raised (seen it take 250ms!)
> > >     If the system has 2133MT/s memory then we pointlessly
> > >     wait one full second :(
> > > v4: Use the new pcode interface to get the qgv points rather
> > >     that using hardcoded numbers
> > > 
> > > Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/Makefile             |   1 +
> > >  drivers/gpu/drm/i915/i915_drv.c           | 229
> > > ++++++++++++++++++++++
> > >  drivers/gpu/drm/i915/i915_drv.h           |  10 +
> > >  drivers/gpu/drm/i915/i915_reg.h           |   3 +
> > >  drivers/gpu/drm/i915/intel_atomic_plane.c |  20 ++
> > >  drivers/gpu/drm/i915/intel_atomic_plane.h |   2 +
> > >  drivers/gpu/drm/i915/intel_bw.c           | 181
> > > +++++++++++++++++
> > >  drivers/gpu/drm/i915/intel_bw.h           |  46 +++++
> > >  drivers/gpu/drm/i915/intel_display.c      |  40 +++-
> > >  drivers/gpu/drm/i915/intel_drv.h          |   2 +
> > >  10 files changed, 533 insertions(+), 1 deletion(-)
> > >  create mode 100644 drivers/gpu/drm/i915/intel_bw.c
> > >  create mode 100644 drivers/gpu/drm/i915/intel_bw.h
> > > 
> > > diff --git a/drivers/gpu/drm/i915/Makefile
> > > b/drivers/gpu/drm/i915/Makefile
> > > index 68106fe35a04..139a0fc19390 100644
> > > --- a/drivers/gpu/drm/i915/Makefile
> > > +++ b/drivers/gpu/drm/i915/Makefile
> > > @@ -138,6 +138,7 @@ i915-y += intel_audio.o \
> > >  	  intel_atomic.o \
> > >  	  intel_atomic_plane.o \
> > >  	  intel_bios.o \
> > > +	  intel_bw.o \
> > >  	  intel_cdclk.o \
> > >  	  intel_color.o \
> > >  	  intel_combo_phy.o \
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.c
> > > b/drivers/gpu/drm/i915/i915_drv.c
> > > index 5ed864752c7b..b7fa7b51c2e2 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.c
> > > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > > @@ -70,6 +70,7 @@
> > >  #include "intel_overlay.h"
> > >  #include "intel_pipe_crc.h"
> > >  #include "intel_pm.h"
> > > +#include "intel_sideband.h"
> > >  #include "intel_sprite.h"
> > >  #include "intel_uc.h"
> > >  
> > > @@ -1435,6 +1436,232 @@ bxt_get_dram_info(struct drm_i915_private
> > > *dev_priv)
> > >  	return 0;
> > >  }
> > >  
> > > +struct intel_qgv_point {
> > > +	u16 dclk, t_rp, t_rdpre, t_rc, t_ras, t_rcd;
> > > +};
> > > +
> > > +struct intel_sagv_info {
> > > +	struct intel_qgv_point points[3];
> > > +	u8 num_points;
> > > +	u8 num_channels;
> > > +	u8 t_bl;
> > > +	enum intel_dram_type dram_type;
> > > +};
> > > +
> > > +static int icl_pcode_read_mem_global_info(struct
> > > drm_i915_private
> > > *dev_priv,
> > > +					  struct intel_sagv_info *si)
> > > +{
> > > +	u32 val = 0;
> > > +	int ret;
> > > +
> > > +	ret = sandybridge_pcode_read(dev_priv,
> > > +				     ICL_PCODE_MEM_SUBSYSYSTEM_INFO |
> > > +				     ICL_PCODE_MEM_SS_READ_GLOBAL_INFO,
> > > +				     &val, NULL);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	switch (val & 0xf) {
> > > +	case 0:
> > > +		si->dram_type = INTEL_DRAM_DDR4;
> > > +		break;
> > > +	case 1:
> > > +		si->dram_type = INTEL_DRAM_DDR3;
> > > +		break;
> > > +	case 2:
> > > +		si->dram_type = INTEL_DRAM_LPDDR3;
> > > +		break;
> > > +	case 3:
> > > +		si->dram_type = INTEL_DRAM_LPDDR3;
> > > +		break;
> > > +	default:
> > > +		MISSING_CASE(val & 0xf);
> > > +		break;
> > > +	}
> > > +
> > > +	si->num_channels = (val & 0xf0) >> 4;
> > > +	si->num_points = (val & 0xf00) >> 8;
> > > +
> > > +	si->t_bl = si->dram_type == INTEL_DRAM_DDR4 ? 4 : 8;
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int icl_pcode_read_qgv_point_info(struct drm_i915_private
> > > *dev_priv,
> > > +					 struct intel_qgv_point *sp,
> > > +					 int point)
> > Are we trying to retrieve the dram timing parameters to calculate
> > the
> > latency? If so can that be seperated as latency calculation instead
> > of
> > using it under bw info below?
> > > +{
> > > +	u32 val = 0, val2;
> > > +	int ret;
> > > +
> > > +	ret = sandybridge_pcode_read(dev_priv,
> > > +				     ICL_PCODE_MEM_SUBSYSYSTEM_INFO |
> > > +				     ICL_PCODE_MEM_SS_READ_QGV_POINT_IN
> > > FO(point),
> > > +				     &val, &val2);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	sp->dclk = val & 0xffff;
> > > +	sp->t_rp = (val & 0xff0000) >> 16;
> > > +	sp->t_rcd = (val & 0xff000000) >> 24;
> > > +
> > > +	sp->t_rdpre = val2 & 0xff;
> > > +	sp->t_ras = (val2 & 0xff00) >> 8;
> > > +
> > > +	sp->t_rc = sp->t_rp + sp->t_ras;
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int icl_get_qgv_points(struct drm_i915_private *dev_priv,
> > > +			      struct intel_sagv_info *si)
> > > +{
> > > +	int i, ret;
> > > +
> > > +	ret = icl_pcode_read_mem_global_info(dev_priv, si);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	if (WARN_ON(si->num_points > ARRAY_SIZE(si->points)))
> > > +		si->num_points = ARRAY_SIZE(si->points);
> > > +
> > > +	for (i = 0; i < si->num_points; i++) {
> > > +		struct intel_qgv_point *sp = &si->points[i];
> > > +
> > > +		ret = icl_pcode_read_qgv_point_info(dev_priv, sp, i);
> > > +		if (ret)
> > > +			return ret;
> > > +
> > > +		DRM_DEBUG_KMS("QGV %d: DCLK=%d tRP=%d tRDPRE=%d tRAS=%d
> > > tRCD=%d tRC=%d\n",
> > > +			      i, sp->dclk, sp->t_rp, sp->t_rdpre, sp-
> > > > t_ras,
> > > +			      sp->t_rcd, sp->t_rc);
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int icl_calc_bw(int dclk, int num, int den)
> > > +{
> > > +	/* multiples of 16.666MHz (100/6) */
> > > +	return DIV_ROUND_CLOSEST(num * dclk * 100, den * 6);
> > > +}
> > > +
> > > +static int icl_sagv_max_dclk(const struct intel_sagv_info *si)
> > > +{
> > > +	u16 dclk = 0;
> > > +	int i;
> > > +
> > > +	for (i = 0; i < si->num_points; i++)
> > > +		dclk = max(dclk, si->points[i].dclk);
> > > +
> > > +	return dclk;
> > > +}
> > > +
> > > +struct intel_sa_info {
> > > +	u8 deburst, mpagesize, deprogbwlimit, displayrtids;
> > > +};
> > > +
> > > +static const struct intel_sa_info icl_sa_info = {
> > > +	.deburst = 8,
> > > +	.mpagesize = 16,
> > > +	.deprogbwlimit = 25, /* GB/s */
> > > +	.displayrtids = 128,
> > > +};
> > > +
> > > +static int icl_get_bw_info(struct drm_i915_private *dev_priv)
> > > +{
> > > +	struct intel_sagv_info si = {};
> > > +	const struct intel_sa_info *sa = &icl_sa_info;
> > > +	bool is_y_tile = true; /* assume y tile may be used */
> > > +	int num_channels;
> > > +	int deinterleave;
> > > +	int ipqdepth, ipqdepthpch;
> > > +	int dclk_max;
> > > +	int maxdebw;
> > > +	int i, ret;
> > > +
> > > +	ret = icl_get_qgv_points(dev_priv, &si);
> > > +	if (ret)
> > > +		return ret;
> > > +	num_channels = si.num_channels;
> > > +
> > > +	deinterleave = DIV_ROUND_UP(num_channels, is_y_tile ? 4 : 2);
> > > +	dclk_max = icl_sagv_max_dclk(&si);
> > > +
> > > +	ipqdepthpch = 16;
> > > +
> > > +	maxdebw = min(sa->deprogbwlimit * 1000,
> > > +		      icl_calc_bw(dclk_max, 16, 1) * 6 / 10); /* 60% */
> > > +	ipqdepth = min(ipqdepthpch, sa->displayrtids / num_channels);
> > > +
> > > +	for (i = 0; i < ARRAY_SIZE(dev_priv->max_bw); i++) {
> > > +		struct intel_bw_info *bi = &dev_priv->max_bw[i];
> > > +		int clpchgroup;
> > > +		int j;
> > > +
> > > +		clpchgroup = (sa->deburst * deinterleave /
> > > num_channels) << i;
> > > +		bi->num_planes = (ipqdepth - clpchgroup) / clpchgroup +
> > > 1;
> > > +
> > > +		for (j = 0; j < si.num_points; j++) {
> > > +			const struct intel_qgv_point *sp =
> > > &si.points[j];
> > > +			int ct, bw;
> > > +
> > > +			/*
> > > +			 * Max row cycle time
> > > +			 *
> > > +			 * FIXME what is the logic behind the
> > > +			 * assumed burst length?
> > > +			 */
> > > +			ct = max_t(int, sp->t_rc, sp->t_rp + sp->t_rcd
> > > +
> > > +				   (clpchgroup - 1) * si.t_bl + sp-
> > > > t_rdpre);
> > For logical flow can we move the above timing related calculations
> > to a
> > seperate function along with fixme to delink bandwidth and latency
> > calculations?
> 
> I don't see what you would want to delink. This is all just
> calculating the effective memory bandwidth limit.
I am assuming the variables t_rc, t_rp, t_rcd are related to dram
latency/timing information. I was wondering if this latency calculation
is moved to a seperate inline function for readability purposes.

- Radhakrishna(RK) Sripada 
> 


More information about the Intel-gfx mailing list