| Message ID | 20170928185148.4829-5-rodrigo.vivi@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@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) { But at that point, why not just define has_ipc as a gen10 feature? You can have a comment before gen9 feature that although IPC was introduced for gen9, it is recommended (w/a) to leave disabled. -Chris
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@intel.com> > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@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) { > > 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
Quoting Rodrigo Vivi (2017-09-28 20:12:14) > 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@intel.com> > > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@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) { > > > > 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... Just the gen9 platforms, gen10+ enables it. -Chris
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@intel.com> >>> Cc: Chris Wilson <chris@chris-wilson.co.uk> >>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >>> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@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. -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
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@intel.com> > > > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > > > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@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 >
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) { dev_priv->ipc_enabled = false; return; }
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@intel.com> Cc: Chris Wilson <chris@chris-wilson.co.uk> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> --- drivers/gpu/drm/i915/intel_pm.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)