Re: [PATCH v6 07/11] drm/xe: Assert runnable state in handle_sched_done

[John Harrison <john.c.harrison@intel.com>]

On 6/12/2024 18:47, Matthew Brost wrote:
> On Wed, Jun 12, 2024 at 05:54:30PM -0700, John Harrison wrote:
>> On 6/12/2024 15:27, Matthew Brost wrote:
>>> On Wed, Jun 12, 2024 at 02:25:40PM -0700, John Harrison wrote:
>>>> On 6/11/2024 07:40, Matthew Brost wrote:
>>>>> Ensure G2H and KMD GuC machine match.
>>>>>
>>>>> Signed-off-by: Matthew Brost <matthew.brost@intel.com>
>>>>> ---
>>>>>     drivers/gpu/drm/xe/xe_guc_submit.c | 9 +++++++--
>>>>>     1 file changed, 7 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/xe/xe_guc_submit.c b/drivers/gpu/drm/xe/xe_guc_submit.c
>>>>> index afd22a8d815d..ab0dc93d7740 100644
>>>>> --- a/drivers/gpu/drm/xe/xe_guc_submit.c
>>>>> +++ b/drivers/gpu/drm/xe/xe_guc_submit.c
>>>>> @@ -1592,16 +1592,21 @@ static void deregister_exec_queue(struct xe_guc *guc, struct xe_exec_queue *q)
>>>>>     	xe_guc_ct_send_g2h_handler(&guc->ct, action, ARRAY_SIZE(action));
>>>>>     }
>>>>> -static void handle_sched_done(struct xe_guc *guc, struct xe_exec_queue *q)
>>>>> +static void handle_sched_done(struct xe_guc *guc, struct xe_exec_queue *q,
>>>>> +			      u32 runnable_state)
>>>>>     {
>>>>>     	trace_xe_exec_queue_scheduling_done(q);
>>>>>     	if (exec_queue_pending_enable(q)) {
>>>>> +		xe_gt_assert(guc_to_gt(guc), runnable_state == 1);
>>>>> +
>>>>>     		q->guc->resume_time = ktime_get();
>>>>>     		clear_exec_queue_pending_enable(q);
>>>>>     		smp_wmb();
>>>>>     		wake_up_all(&guc->ct.wq);
>>>>>     	} else {
>>>>> +		xe_gt_assert(guc_to_gt(guc), runnable_state == 0);
>>>>> +
>>>> Isn't this the wrong way around?
>>>>
>>> These asserts are per my testing and CI.
>>>
>>>> You made an earlier comment that sounded like it is legal for an enable to
>>>> be queued while a disable is still pending? If so, then you would get in
>>> Other way around, a disable can be sent when an enable is still in
>>> flight in the case of a preempt fence.
>>>
>>> Enables cannot be issued when a pending disable is in flight.
>>>
>>> So I believe this patch is correct.
>> It might work but it does not feel correct.
>>
>> On receipt of a disable notification, the code first checks to see if there
>> is a pending enable. That just seems backwards. It is more logical to
>> process the message according to the message type actually received rather
>> than according to the message type that is expected.
>>
>> If there is ever a valid reason for sending back to back
>> disable-then-enable, then this will break. Whereas, coding it according to
>> the notification type rather than the internal state would allow that
>> sequence to work just fine.
>>
> Can I do this in a follow up? Like immediately after merging this? I
> suggest this as this series has green CI, RBs, will help stablize the
> stack for VK (issue #799), ack'd by Paulo, and fixes a known gap in our
> job timeout mechanism.
>
> Matt
Sure. Like you say, the current version will work as the driver stands.

John.

>
>> As I mentioned earlier, this code is basically broken in i915 and can't be
>> fixed without a significant re-write of the upper layers. It would be
>> greatly preferable to do it properly in Xe.
>>
>> John.
>>
>>
>>> Matt
>>>
>>>> here for the disable notification but with both enable_pending and
>>>> disable_pending flags set. That would hit the assert. Whereas, if the if
>>>> checks the runnable_state parameter and the assert then checks for pending,
>>>> you will not hit the assert and the code will do the correct thing.
>>>>
>>>> John.
>>>>
>>>>>     		clear_exec_queue_pending_disable(q);
>>>>>     		if (q->guc->suspend_pending) {
>>>>>     			suspend_fence_signal(q);
>>>>> @@ -1640,7 +1645,7 @@ int xe_guc_sched_done_handler(struct xe_guc *guc, u32 *msg, u32 len)
>>>>>     		return -EPROTO;
>>>>>     	}
>>>>> -	handle_sched_done(guc, q);
>>>>> +	handle_sched_done(guc, q, runnable_state);
>>>>>     	return 0;
>>>>>     }