[Intel-gfx] [PATCH 4/4] drm/i915: Extend WaDisableIPC to all platforms.

[Rodrigo Vivi]

On Tue, Oct 03, 2017 at 05:27:29AM +0000, Mahesh Kumar wrote:
> Hi,
> 
> sorry for late reply, it was India site holiday.
> 
> On Friday 29 September 2017 12:42 AM, Rodrigo Vivi wrote:
> > On Thu, Sep 28, 2017 at 07:02:59PM +0000, Chris Wilson wrote:
> > > Quoting Rodrigo Vivi (2017-09-28 19:51:48)
> > > > Although Bspec state this Workaround is only relevant for SKL:All.
> > > > 
> > > > The wa_database state this is needed for All platforms from SKL to CNL.
> > > > 
> > > > So let's pick the safest case.
> > > > 
> > > > Cc: Mahesh Kumar <mahesh1.kumar at intel.com>
> > > > Cc: Chris Wilson <chris at chris-wilson.co.uk>
> > > > Cc: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> > > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi at intel.com>
> > > > ---
> > > >   drivers/gpu/drm/i915/intel_pm.c | 4 ++--
> > > >   1 file changed, 2 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > > > index ede871b7982e..27f9d5ab2d23 100644
> > > > --- a/drivers/gpu/drm/i915/intel_pm.c
> > > > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > > > @@ -5828,8 +5828,8 @@ void intel_enable_ipc(struct drm_i915_private *dev_priv)
> > > >   {
> > > >          u32 val;
> > > > -       /* Display WA #0477 WaDisableIPC: skl */
> > > > -       if (IS_SKYLAKE(dev_priv)) {
> > > > +       /* Display WA #0477 WaDisableIPC: skl,kbl,bxt,glk,cfl,cnl */
> > > > +       if (INTEL_GEN(dev_priv) <= 10) {
> WA #0477 asks us to disable IPC for SKL:ALL, for all other GEN9 variant it
> should be enabled.
> Enabling IPC improves performance of other memory clients, as without IPC
> enabled display always fetches from memory with High-priority.

Well, recently Samu's team noticed a perf difference from CFL compared to SKL...
related to memory latency... And this here seems to be the only difference that I
could spot...
And it is globally disabled according to wa_database... what would explain the gap
even further...

Anyways I re-submit the series without this patch so we can continue tests and
discussions only around this patch and we move along with other patches on that
series.

Thanks for the review and comments,
Rodrigo.

> 
> -Mahesh
> > > But at that point, why not just define has_ipc as a gen10 feature?
> > it's already there...
> > 
> > > You
> > > can have a comment before gen9 feature that although IPC was introduced
> > > for gen9, it is recommended (w/a) to leave disabled.
> > so you mean moving this Wa from here to set has_ipc = 0 to all platforms?
> > 
> > Anyways I'd like to hear from Manesh about it since this basically revert
> > all IPC work for all platforms...
> > 
> > > -Chris
>