Re: [PATCH] drm/i915: Don't apply the 16Gb DIMM wm latency w/a to BXT/GLK

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx