Re: [Intel-gfx] [PATCH 4/5] drm/i915: Suppress context restore w/a when using MI_RESTORE_INHIBIT

[Chris Wilson]

On Fri, Feb 24, 2017 at 12:45:54PM +0200, Mika Kuoppala wrote:
> Chris Wilson  writes:
> 
> > If we are setting the context and do inhibit the restoration from the
> > context image, also forgo applying the set-context w/a.
> >
> > Signed-off-by: Chris Wilson 
> > ---
> >  drivers/gpu/drm/i915/i915_gem_context.c | 12 ++--
> >  1 file changed, 6 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_gem_context.c 
> > b/drivers/gpu/drm/i915/i915_gem_context.c
> > index baceca14f5e0..25e8db73542a 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_context.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> > @@ -577,17 +577,17 @@ void i915_gem_context_close(struct drm_device *dev, 
> > struct drm_file *file)
> >  }
> >  
> >  static inline int
> > -mi_set_context(struct drm_i915_gem_request *req, u32 hw_flags)
> > +mi_set_context(struct drm_i915_gem_request *req, u32 flags)
> >  {
> > struct drm_i915_private *dev_priv = req->i915;
> > struct intel_engine_cs *engine = req->engine;
> > enum intel_engine_id id;
> > -   u32 *cs, flags = hw_flags | MI_MM_SPACE_GTT;
> > const int num_rings =
> > /* Use an extended w/a on ivb+ if signalling from other rings */
> > i915.semaphores ?
> > INTEL_INFO(dev_priv)->num_rings - 1 :
> > 0;
> > +   u32 *cs;
> > int len;
> >  
> > /* These flags are for resource streamer on HSW+ */
> > @@ -595,10 +595,10 @@ mi_set_context(struct drm_i915_gem_request *req, u32 
> > hw_flags)
> > flags |= (HSW_MI_RS_SAVE_STATE_EN | HSW_MI_RS_RESTORE_STATE_EN);
> > else if (INTEL_GEN(dev_priv) < 8)
> > flags |= (MI_SAVE_EXT_STATE_EN | MI_RESTORE_EXT_STATE_EN);
> > -
> > +   flags |= MI_MM_SPACE_GTT;
> >  
> > len = 4;
> > -   if (INTEL_GEN(dev_priv) >= 7)
> > +   if (INTEL_GEN(dev_priv) >= 7 && !(flags & MI_RESTORE_INHIBIT))
> 
> If we are restoring from the context image, we dont need the
> workarounds. So should this be 

I merged up to the previous patch as we ran into an uncertainity as to
whether some of the w/a applied to restore or to save.

The next patch is still interesting, but merits some soak testing to see
if cures the last full-ppgtt corruption on hsw.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 4/5] drm/i915: Suppress context restore w/a when using MI_RESTORE_INHIBIT

[Chris Wilson]

On Fri, Feb 24, 2017 at 12:45:54PM +0200, Mika Kuoppala wrote:
> Chris Wilson  writes:
> 
> > If we are setting the context and do inhibit the restoration from the
> > context image, also forgo applying the set-context w/a.
> >
> > Signed-off-by: Chris Wilson 
> > ---
> >  drivers/gpu/drm/i915/i915_gem_context.c | 12 ++--
> >  1 file changed, 6 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_gem_context.c 
> > b/drivers/gpu/drm/i915/i915_gem_context.c
> > index baceca14f5e0..25e8db73542a 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_context.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> > @@ -577,17 +577,17 @@ void i915_gem_context_close(struct drm_device *dev, 
> > struct drm_file *file)
> >  }
> >  
> >  static inline int
> > -mi_set_context(struct drm_i915_gem_request *req, u32 hw_flags)
> > +mi_set_context(struct drm_i915_gem_request *req, u32 flags)
> >  {
> > struct drm_i915_private *dev_priv = req->i915;
> > struct intel_engine_cs *engine = req->engine;
> > enum intel_engine_id id;
> > -   u32 *cs, flags = hw_flags | MI_MM_SPACE_GTT;
> > const int num_rings =
> > /* Use an extended w/a on ivb+ if signalling from other rings */
> > i915.semaphores ?
> > INTEL_INFO(dev_priv)->num_rings - 1 :
> > 0;
> > +   u32 *cs;
> > int len;
> >  
> > /* These flags are for resource streamer on HSW+ */
> > @@ -595,10 +595,10 @@ mi_set_context(struct drm_i915_gem_request *req, u32 
> > hw_flags)
> > flags |= (HSW_MI_RS_SAVE_STATE_EN | HSW_MI_RS_RESTORE_STATE_EN);
> > else if (INTEL_GEN(dev_priv) < 8)
> > flags |= (MI_SAVE_EXT_STATE_EN | MI_RESTORE_EXT_STATE_EN);
> > -
> > +   flags |= MI_MM_SPACE_GTT;
> >  
> > len = 4;
> > -   if (INTEL_GEN(dev_priv) >= 7)
> > +   if (INTEL_GEN(dev_priv) >= 7 && !(flags & MI_RESTORE_INHIBIT))
> 
> If we are restoring from the context image, we dont need the
> workarounds. So should this be 
> 
> if (INTEL_GEN(dev_priv) >= 7 && flags & MI_RESTORE_INHIBIT)

If we are restoring, we need the w/a. Afaik, it's the execution of the
context image itself that's troublesome.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 4/5] drm/i915: Suppress context restore w/a when using MI_RESTORE_INHIBIT

[Mika Kuoppala]

Chris Wilson  writes:

> If we are setting the context and do inhibit the restoration from the
> context image, also forgo applying the set-context w/a.
>
> Signed-off-by: Chris Wilson 
> ---
>  drivers/gpu/drm/i915/i915_gem_context.c | 12 ++--
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c 
> b/drivers/gpu/drm/i915/i915_gem_context.c
> index baceca14f5e0..25e8db73542a 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -577,17 +577,17 @@ void i915_gem_context_close(struct drm_device *dev, 
> struct drm_file *file)
>  }
>  
>  static inline int
> -mi_set_context(struct drm_i915_gem_request *req, u32 hw_flags)
> +mi_set_context(struct drm_i915_gem_request *req, u32 flags)
>  {
>   struct drm_i915_private *dev_priv = req->i915;
>   struct intel_engine_cs *engine = req->engine;
>   enum intel_engine_id id;
> - u32 *cs, flags = hw_flags | MI_MM_SPACE_GTT;
>   const int num_rings =
>   /* Use an extended w/a on ivb+ if signalling from other rings */
>   i915.semaphores ?
>   INTEL_INFO(dev_priv)->num_rings - 1 :
>   0;
> + u32 *cs;
>   int len;
>  
>   /* These flags are for resource streamer on HSW+ */
> @@ -595,10 +595,10 @@ mi_set_context(struct drm_i915_gem_request *req, u32 
> hw_flags)
>   flags |= (HSW_MI_RS_SAVE_STATE_EN | HSW_MI_RS_RESTORE_STATE_EN);
>   else if (INTEL_GEN(dev_priv) < 8)
>   flags |= (MI_SAVE_EXT_STATE_EN | MI_RESTORE_EXT_STATE_EN);
> -
> + flags |= MI_MM_SPACE_GTT;
>  
>   len = 4;
> - if (INTEL_GEN(dev_priv) >= 7)
> + if (INTEL_GEN(dev_priv) >= 7 && !(flags & MI_RESTORE_INHIBIT))

If we are restoring from the context image, we dont need the
workarounds. So should this be 

if (INTEL_GEN(dev_priv) >= 7 && flags & MI_RESTORE_INHIBIT)

?

-Mika

>   len += 2 + (num_rings ? 4*num_rings + 6 : 0);
>  
>   cs = intel_ring_begin(req, len);
> @@ -606,7 +606,7 @@ mi_set_context(struct drm_i915_gem_request *req, u32 
> hw_flags)
>   return PTR_ERR(cs);
>  
>   /* WaProgramMiArbOnOffAroundMiSetContext:ivb,vlv,hsw,bdw,chv */
> - if (INTEL_GEN(dev_priv) >= 7) {
> + if (INTEL_GEN(dev_priv) >= 7 && !(flags & MI_RESTORE_INHIBIT)) {
>   *cs++ = MI_ARB_ON_OFF | MI_ARB_DISABLE;
>   if (num_rings) {
>   struct intel_engine_cs *signaller;
> @@ -633,7 +633,7 @@ mi_set_context(struct drm_i915_gem_request *req, u32 
> hw_flags)
>*/
>   *cs++ = MI_NOOP;
>  
> - if (INTEL_GEN(dev_priv) >= 7) {
> + if (INTEL_GEN(dev_priv) >= 7 && !(flags & MI_RESTORE_INHIBIT)) {
>   if (num_rings) {
>   struct intel_engine_cs *signaller;
>   i915_reg_t last_reg = {}; /* keep gcc quiet */
> -- 
> 2.11.0
>
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx