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

[Andrzej Hajda <andrzej.hajda@intel.com>]

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/

Regards
Andrzej

> 
> Regards,
> 
> Tvrtko