Message ID | 20181010175326.21504-1-ville.syrjala@linux.intel.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | drm/i915: Don't apply the 16Gb DIMM wm latency w/a to BXT/GLK | expand |
Hi, On Wed, Oct 10, 2018 at 11:25 PM Ville Syrjala <ville.syrjala@linux.intel.com> wrote: > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > The 16Gb DIMM w/a is not applicable to BXT or GLK. Limit it to > the appropriate platforms. > > This was especially harsh on GLK since we don't even try to read > the DIMM information on that platforms, hence valid_dimm was > always false and thus we always tried to apply the w/a. > Furthermore the w/a pushed the level 0 latency above the > level 1 latency, which doesn't really make sense. > > Cc: Mahesh Kumar <mahesh1.kumar@intel.com> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Fixes: 86b592876cb6 ("drm/i915: Implement 16GB dimm wa for latency level-0") > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/i915/intel_pm.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > index 1392aa56a55a..a51cd09bbf75 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -2881,8 +2881,9 @@ static void intel_read_wm_latency(struct drm_i915_private *dev_priv, > * any underrun. If not able to get Dimm info assume 16GB dimm > * to avoid any underrun. > */ > - if (!dev_priv->dram_info.valid_dimm || > - dev_priv->dram_info.is_16gb_dimm) > + if (!IS_GEN9_LP(dev_priv) && > + (!dev_priv->dram_info.valid_dimm || > + dev_priv->dram_info.is_16gb_dimm)) > wm[0] += 1; I would rather prefer to update "intel_get_dram_info" to fill valid_dimm and is_16gb_dimm info properly - if (INTEL_GEN(dev_priv) < 9 || IS_GEMINILAKE(dev_priv)) + if (INTEL_GEN(dev_priv) < 9 ) return; + if (IS_GEN9_LP(dev_priv)) { + dram_info->valid_dimm = true; + dram_info->is_16gb_dimm = false; + } + + -Mahesh > > } else if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) { > -- > 2.18.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Thu, Oct 11, 2018 at 2:19 AM Mahesh Kumar <mahesh1.sh.kumar@gmail.com> wrote: > > Hi, > > On Wed, Oct 10, 2018 at 11:25 PM Ville Syrjala > <ville.syrjala@linux.intel.com> wrote: > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > The 16Gb DIMM w/a is not applicable to BXT or GLK. Limit it to > > the appropriate platforms. > > > > This was especially harsh on GLK since we don't even try to read > > the DIMM information on that platforms, hence valid_dimm was > > always false and thus we always tried to apply the w/a. > > Furthermore the w/a pushed the level 0 latency above the > > level 1 latency, which doesn't really make sense. > > > > Cc: Mahesh Kumar <mahesh1.kumar@intel.com> > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > Fixes: 86b592876cb6 ("drm/i915: Implement 16GB dimm wa for latency level-0") > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > --- > > drivers/gpu/drm/i915/intel_pm.c | 5 +++-- > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > > index 1392aa56a55a..a51cd09bbf75 100644 > > --- a/drivers/gpu/drm/i915/intel_pm.c > > +++ b/drivers/gpu/drm/i915/intel_pm.c > > @@ -2881,8 +2881,9 @@ static void intel_read_wm_latency(struct drm_i915_private *dev_priv, > > * any underrun. If not able to get Dimm info assume 16GB dimm > > * to avoid any underrun. > > */ > > - if (!dev_priv->dram_info.valid_dimm || > > - dev_priv->dram_info.is_16gb_dimm) > > + if (!IS_GEN9_LP(dev_priv) && > > + (!dev_priv->dram_info.valid_dimm || > > + dev_priv->dram_info.is_16gb_dimm)) > > wm[0] += 1; > > I would rather prefer to update "intel_get_dram_info" to fill > valid_dimm and is_16gb_dimm info properly > > - if (INTEL_GEN(dev_priv) < 9 || IS_GEMINILAKE(dev_priv)) > + if (INTEL_GEN(dev_priv) < 9 ) > return; > > + if (IS_GEN9_LP(dev_priv)) { > + dram_info->valid_dimm = true; > + dram_info->is_16gb_dimm = false; > + } > + > + We don't want to proceed for GLK. So, something like: + if (IS_GEN9_LP(dev_priv)) { + dram_info->valid_dimm = true; + dram_info->is_16gb_dimm = false; + } + if (INTEL_GEN(dev_priv) < 9 || IS_GEMINILAKE(dev_priv)) return; -Mahesh > > -Mahesh > > > > > } else if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) { > > -- > > 2.18.1 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Thu, Oct 11, 2018 at 02:30:41AM +0530, Mahesh Kumar wrote: > On Thu, Oct 11, 2018 at 2:19 AM Mahesh Kumar <mahesh1.sh.kumar@gmail.com> wrote: > > > > Hi, > > > > On Wed, Oct 10, 2018 at 11:25 PM Ville Syrjala > > <ville.syrjala@linux.intel.com> wrote: > > > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > The 16Gb DIMM w/a is not applicable to BXT or GLK. Limit it to > > > the appropriate platforms. > > > > > > This was especially harsh on GLK since we don't even try to read > > > the DIMM information on that platforms, hence valid_dimm was > > > always false and thus we always tried to apply the w/a. > > > Furthermore the w/a pushed the level 0 latency above the > > > level 1 latency, which doesn't really make sense. > > > > > > Cc: Mahesh Kumar <mahesh1.kumar@intel.com> > > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > > Fixes: 86b592876cb6 ("drm/i915: Implement 16GB dimm wa for latency level-0") > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > --- > > > drivers/gpu/drm/i915/intel_pm.c | 5 +++-- > > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > > > index 1392aa56a55a..a51cd09bbf75 100644 > > > --- a/drivers/gpu/drm/i915/intel_pm.c > > > +++ b/drivers/gpu/drm/i915/intel_pm.c > > > @@ -2881,8 +2881,9 @@ static void intel_read_wm_latency(struct drm_i915_private *dev_priv, > > > * any underrun. If not able to get Dimm info assume 16GB dimm > > > * to avoid any underrun. > > > */ > > > - if (!dev_priv->dram_info.valid_dimm || > > > - dev_priv->dram_info.is_16gb_dimm) > > > + if (!IS_GEN9_LP(dev_priv) && > > > + (!dev_priv->dram_info.valid_dimm || > > > + dev_priv->dram_info.is_16gb_dimm)) > > > wm[0] += 1; > > > > I would rather prefer to update "intel_get_dram_info" to fill > > valid_dimm and is_16gb_dimm info properly > > > > - if (INTEL_GEN(dev_priv) < 9 || IS_GEMINILAKE(dev_priv)) > > + if (INTEL_GEN(dev_priv) < 9 ) > > return; > > > > + if (IS_GEN9_LP(dev_priv)) { > > + dram_info->valid_dimm = true; > > + dram_info->is_16gb_dimm = false; > > + } > > + > > + > We don't want to proceed for GLK. So, something like: > > + if (IS_GEN9_LP(dev_priv)) { > + dram_info->valid_dimm = true; > + dram_info->is_16gb_dimm = false; > + } > + I was going to do this initially but then I thought that it might cause someone else to assume the dram info is actually valid and rely on other stale data in there. I guess a compromise is to ignore valid_dimm entirely and make sure is_16gb_dimm is populated like we want for every platform. Would probably need a comment to make sure no one extends the 16Gb dimm detection to cover BXT/GLK and accidentally re-enable the w/a on those platforms (if they can even have 16Gb DIMMs?). There's a bit of confusion in other parts of the dram detection code as well. Eg. why does skl_dram_get_channels_info() set valid_dimm=true before it configures is_16gb_dimm? There's a return statement in between so we can be left with some kind of bogus dram information, no? > if (INTEL_GEN(dev_priv) < 9 || IS_GEMINILAKE(dev_priv)) > return; > > -Mahesh > > > > -Mahesh > > > > > > > > } else if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) { > > > -- > > > 2.18.1 > > > > > > _______________________________________________ > > > Intel-gfx mailing list > > > Intel-gfx@lists.freedesktop.org > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 1392aa56a55a..a51cd09bbf75 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -2881,8 +2881,9 @@ static void intel_read_wm_latency(struct drm_i915_private *dev_priv, * any underrun. If not able to get Dimm info assume 16GB dimm * to avoid any underrun. */ - if (!dev_priv->dram_info.valid_dimm || - dev_priv->dram_info.is_16gb_dimm) + if (!IS_GEN9_LP(dev_priv) && + (!dev_priv->dram_info.valid_dimm || + dev_priv->dram_info.is_16gb_dimm)) wm[0] += 1; } else if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) {