Re: drm/sched: run queues freed before the TDR that drm_sched_fini() waits for

[Donggeun Yoo]

On 9/10/26 09:32, Christian König wrote:
> Amdgpu shouldn't do that any more.

Correct, and I should have checked before writing it - 182bdd59be41
("drm/amdgpu: deprecate guilty handling") removed it. The callers left are
etnaviv, lima, panfrost and v3d. v3d is the one I should have named.

> That was an extremely ugly hack applied long long time ago because amdgpu
> was broken at that time and didn't waited for
> drm_sched_entity_flush()/drm_sched_entity_fini() before calling
> drm_sched_fini().

Understood, I am dropping that half of the argument.

> No it doesn't. You quoted the wrong code, this is what really matters:
>
>         drm_sched_wqueue_stop(sched);
>
>         for (i = DRM_SCHED_PRIORITY_KERNEL; i < sched->num_rqs; i++)
>                 kfree(sched->sched_rq[i]);

I am not sure I follow this one. If the point is that
drm_sched_wqueue_stop() has already quiesced the users of the run queues
by the time the loop runs, I cannot find where it covers the timeout work:

	WRITE_ONCE(sched->pause_submit, true);
	cancel_work_sync(&sched->work_run_job);
	cancel_work_sync(&sched->work_free_job);

work_tdr is queued on sched->timeout_wq and is only canceled by the
cancel_delayed_work_sync() below the loop, so a timeout handler can still
be running while the run queues are freed. Is there something else that
rules that out? And if I have misread your point, please elaborate.

On how I got there: the KUnit case never signals the hardware fence, which
is what keeps the handler inside timedout_job() while drm_sched_fini() runs.
That breaks the rule that all run_job() fences are signaled before
drm_sched_fini(), so a correct driver should not reach this, and I have no
reproducer that does not cheat that way. The same caveat is in the patch.

I am writing up the patch Philipp asked for. The only change is moving the
kfree loop down beside kfree(sched->sched_rq); no new code.

Regards,
Donggeun