On 14/09/2018 10:17, Chris Wilson wrote:
Quoting Tvrtko Ursulin (2018-09-14 10:12:15)
On 14/09/2018 09:21, Chris Wilson wrote:
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.
Ok.
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.
I don't follow - all places where we map it use MAP_WB. Isn't force flag
just to override the existing different mapping?
Yes, but we may not have done the force from MAP_WC to MAP_WB in
i915_gem_unpark->intel_engines_unpark by this point, so
engine->default_state may still have a WC mapping on it. To be sure, we
need to validate that we can acquire that mapping on init.
Okay I missed the fact we just transfer the engine->default_state object
ownership from ce->state->obj. Somehow I assumed it is a dedicated buffer.
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
