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

[Tvrtko Ursulin]

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 at intel.com>

Regards,

Tvrtko