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@xxxxxxxxx> wrote:
> >
> > Hi,
> >
> > On Wed, Oct 10, 2018 at 11:25 PM Ville Syrjala
> > <ville.syrjala@xxxxxxxxxxxxxxx> wrote:
> > >
> > > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > >
> > > 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@xxxxxxxxx>
> > > Cc: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
> > > Fixes: 86b592876cb6 ("drm/i915: Implement 16GB dimm wa for latency level-0")
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > > ---
> > > 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@xxxxxxxxxxxxxxxxxxxxx
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
