[Intel-gfx] [PATCH v2] drm/i915/gt: update request engine before removing virtual GuC engine

[Tvrtko Ursulin]

On 12/07/2023 13:18, Andrzej Hajda wrote:
> On 11.07.2023 17:27, Tvrtko Ursulin wrote:
>>
>> On 11/07/2023 14:58, Andrzej Hajda wrote:
>>> On 11.07.2023 13:34, Andi Shyti wrote:
>>>> Hi Andrzej,
>>>>
>>>>>           drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c | 11 
>>>>> +++++++++++
>>>>>           1 file changed, 11 insertions(+)
>>>>>
>>>>>          diff --git 
>>>>> a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c 
>>>>> b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
>>>>>          index a0e3ef1c65d246..2c877ea5eda6f0 100644
>>>>>          --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
>>>>>          +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
>>>>>          @@ -3461,6 +3461,8 @@ static void guc_prio_fini(struct 
>>>>> i915_request *rq, struct intel_context *ce)
>>>>>           static void remove_from_context(struct i915_request *rq)
>>>>>           {
>>>>>                  struct intel_context *ce = 
>>>>> request_to_scheduling_context(rq);
>>>>>          +       struct intel_engine_cs *engine;
>>>>>          +       intel_engine_mask_t tmp;
>>>>>
>>>>>                  GEM_BUG_ON(intel_context_is_child(ce));
>>>>>
>>>>>          @@ -3478,6 +3480,15 @@ static void 
>>>>> remove_from_context(struct i915_request *rq)
>>>>>
>>>>>                  atomic_dec(&ce->guc_id.ref);
>>>>>                  i915_request_notify_execute_cb_imm(rq);
>>>>>          +
>>>>>          +       /*
>>>>>          +        * GuC virtual engine can disappear after this 
>>>>> call, so let's assign
>>>>>          +        * something valid, as driver expects this to be 
>>>>> always valid pointer.
>>>>>          +        */
>>>>>          +       for_each_engine_masked(engine, rq->engine->gt, 
>>>>> rq->execution_mask, tmp) {
>>>>>          +               rq->engine = engine;
>>>>>
>>>>>      yes... here the context might lose the virtual engine... I wonder
>>>>>      whether this is the rigth solution, though. Maybe we should set
>>>>>      rq->engine = NULL; and check for NULL? Don't know.
>>>>>
>>>>>
>>>>> Setting NULL causes occasional null page de-reference in
>>>>>
>>>>> i915_request_wait_timeout:
>>>>>
>>>>> mutex_release(&rq->engine->gt->reset.mutex.dep_map, _THIS_IP_)
>>>>>
>>>>> rq->engine after removing rq from context is (IMHO) used as a set 
>>>>> of aliases
>>>>> for gt and i915 (despite rq itself contains the alias to i915).
>>>> without investigating further, but maybe that code is not even
>>>> supposed to be executed, at this point, if the request's assigned
>>>> virtual engine is removed.
>>>
>>> Real tests show it is executed and the function 
>>> i915_request_wait_timeout is quite generic
>>> I guess it is quite typical use-case, the only question is about 
>>> timings - what happens earlier -
>>> finalization of i915_request_wait_timeout or context removal.
>>>
>>> The other point rq->engine is accessed after context removal is 
>>> i915_fence_release -
>>> there is long comment there regarding virtual context and reuse 
>>> retired rq.
>>> Anyway calling there "intel_engine_is_virtual(rq->engine)" is risky 
>>> without this patch and KASAN complains clearly about it:
>>> http://gfx-ci.igk.intel.com/tree/drm-tip/kasan.html?testfilter=gem_exec_balancer
>>
>> Looks like a bug introduced in bcb9aa45d5a0 ("Revert "drm/i915: Hold 
>> reference to intel_context over life of i915_request""), which was a 
>> partial revert of 1e98d8c52ed5 ("drm/i915: Hold reference to 
>> intel_context over life of i915_request").
>>
>> Ie. if 1e98d8c52ed5 recognised the problem with disappearing 
>> rq->engine, then I am confused how bcb9aa45d5a0 left the rq->engine 
>> dereference in there after removing the extra reference.
>>
>> Could it be that the intel_engine_is_virtual check simply needs to be 
>> removed from i915_fence_release, restoring things to how they were 
>> before 1e98d8c52ed5? Could you try it out?
> 
> 
> I have already tried something similar [1] and KASAN bugs disappeared, 
> or more precisely gem_exec_balance tests passed. But I have been warned 
> by Nirmoy guc virtual engines can be created for only one real engine 
> (ie. is_power_of_2(rq->execution_mask) is true but rq->engine points to 
> virtual engine).
> 
> [1]: https://patchwork.freedesktop.org/series/118879/

Ugh.. Try involving media umd folks to see if they need a single engine 
virtual engine? Or we could always just not create it in the driver, I 
mean just use the physical one.

Regards,

Tvrtko