[Intel-gfx] looks like a issue in do_switch() and mi_set_context() in i915_gem_context.c?

[Zhi Wang]

Hi Chris:
     Thanks for the reply. :) I can understand that the backing storage 
is pinned at this time, as the reference counter of context object 
should not be zero. But for VMA, is there any chance that the vma will 
be unbinded from GGTT at this time by shrinker? I saw that 
i915_gem_object_ggtt_unpin() will decrease the VMA reference counter...

Thanks,
Zhi.

On 04/01/15 23:58, Chris Wilson wrote:
> On Wed, Apr 01, 2015 at 03:52:17PM +0000, Wang, Zhi A wrote:
>> Hi Experts and Gurus:
>>     I'm learning i915 and It looks like in i915_gem_context.c: mi_set_context(), the last intel_ring_advance(); is a lazy ring tail write. So I think the ring buffer contains MI_SET_CONTEXT will not be submitted at this time, but in the caller: do_switch() it will unpin the backing memory of the context GEM object. From this time the backing memory may be swapped out? Then in the execbuffer routines, the MI_SET_CONTEXT may save current HW state into an unexpected location I guess?
>>
>> do_switch()
>>      -> mi_set_context()
>>          -> intel_ring_begin()
>>           ->emit MI_SET_CONTEXT (save current HW state to "prev" context and load engine state from "next" context, but it won't be submitted at this time)
>>           ->intel_ring_advance()
>>      -> unpin and unreference "prev" context(from this time the "prev" context can be swapped by shrinker I think)
>>
>> Then we come into the time of real submission:
>>
>> i915_gem_execbuffer_retire_commands()
>>      -> __intel_ring_advance() (MI_SET_CONTEXT got submitted at this time, but the "prev" context in do_switch() may be swapped out and turned into invalid? If GPU save current engine state into "prev" context, the "prev" context should be corrupted? When it got switch back, there should be problem I think.)
>>
>> I think a solution should be:
>> - Changing intel_ring_advance() to __intel_ring_davnce() in mi_set_context()-> submit the MI_SET_CONTEXT ring buffer directly at  this time.
>> - Polling CCID register until the new engine state is load into HW.
>>
>> Then the "prev" context can be unpinned and unreferenced safely?
>>
>> I'm just a beginner, If you can confirm this is an issue or a potential problem, I can cook a patch. :)
>
> It's not a problem. The old context is pinned by the active reference
> until the MI_SET_CONTEXT switch is completed by hardware at an
> indefinite point in the future.
> -Chris
>