[Intel-gfx] [PATCH v3] drm/i915/execlists: Use coherent writes into the context image

[Chris Wilson]

Quoting Tvrtko Ursulin (2018-09-14 09:14:54)
> 
> On 13/09/2018 20:33, Chris Wilson wrote:
> > diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> > index d7fcbba8e982..7b1f322f232b 100644
> > --- a/drivers/gpu/drm/i915/intel_lrc.c
> > +++ b/drivers/gpu/drm/i915/intel_lrc.c
> > @@ -1294,7 +1294,7 @@ static int __context_pin(struct i915_gem_context *ctx, struct i915_vma *vma)
> >        * on an active context (which by nature is already on the GPU).
> >        */
> >       if (!(vma->flags & I915_VMA_GLOBAL_BIND)) {
> > -             err = i915_gem_object_set_to_gtt_domain(vma->obj, true);
> > +             err = i915_gem_object_set_to_wc_domain(vma->obj, true);
> 
> I am still confused by this. Cache flushing effects of the old and new 
> call seem the same due object being in CPU write domain at this point. 
> What changes is that it will be marked differently from this point one. 
> Does that come into play later in the objects lifetime and where?

No, just taking the opportunity to use a more correct domain now that it
exists and logically ties in with using WC.

> >               if (err)
> >                       return err;
> >       }
> > @@ -1322,7 +1322,9 @@ __execlists_context_pin(struct intel_engine_cs *engine,
> >       if (ret)
> >               goto err;
> >   
> > -     vaddr = i915_gem_object_pin_map(ce->state->obj, I915_MAP_WB);
> > +     vaddr = i915_gem_object_pin_map(ce->state->obj,
> > +                                     i915_coherent_map_type(ctx->i915) |
> > +                                     I915_MAP_OVERRIDE);
> 
> Override MAP_WB from populate_lr_context - OK I think.
> 
> >       if (IS_ERR(vaddr)) {
> >               ret = PTR_ERR(vaddr);
> >               goto unpin_vma;
> > @@ -2753,7 +2755,7 @@ populate_lr_context(struct i915_gem_context *ctx,
> >               void *defaults;
> >   
> >               defaults = i915_gem_object_pin_map(engine->default_state,
> > -                                                I915_MAP_WB);
> > +                                                I915_MAP_FORCE_WB);
> >               if (IS_ERR(defaults)) {
> >                       ret = PTR_ERR(defaults);
> >                       goto err_unpin_ctx;
> > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> > index d0ef50bf930a..1eb68d77b66c 100644
> > --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> > @@ -1288,7 +1288,7 @@ alloc_context_vma(struct intel_engine_cs *engine)
> >               }
> >   
> >               defaults = i915_gem_object_pin_map(engine->default_state,
> > -                                                I915_MAP_WB);
> > +                                                I915_MAP_FORCE_WB);
> >               if (IS_ERR(defaults)) {
> >                       err = PTR_ERR(defaults);
> >                       goto err_map;
> > 
> 
> These two do not need to be changed AFAICT.

I think we cannot rely on engine->default_state always being MAP_WB
already at this point, due to not having an idle cycle between creation
of engine->default_state on module_load and first use.

Having thought of that last night, I did mean to add a call to
__i915_gem_park() during init so we forced ourselves to idle.
-Chris