[Intel-gfx] [PATCH v3 2/2] drm/i915: Make sure we have enough memory bandwidth on ICL
Ville Syrjälä
ville.syrjala at linux.intel.com
Mon May 13 14:16:17 UTC 2019
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.
--
Ville Syrjälä
Intel
More information about the Intel-gfx
mailing list