On 2023-10-04 23:11, Matthew Brost wrote: > On Sat, Sep 30, 2023 at 03:48:07PM -0400, Luben Tuikov wrote: >> On 2023-09-29 17:53, Luben Tuikov wrote: >>> Hi, >>> >>> On 2023-09-19 01:01, Matthew Brost wrote: >>>> If the TDR is set to a very small value it can fire before the >>>> submission is started in the function drm_sched_start. The submission is >>>> expected to running when the TDR fires, fix this ordering so this >>>> expectation is always met. >>>> >>>> Signed-off-by: Matthew Brost <matthew.brost@xxxxxxxxx> >>>> --- >>>> drivers/gpu/drm/scheduler/sched_main.c | 4 ++-- >>>> 1 file changed, 2 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c >>>> index 09ef07b9e9d5..a5cc9b6c2faa 100644 >>>> --- a/drivers/gpu/drm/scheduler/sched_main.c >>>> +++ b/drivers/gpu/drm/scheduler/sched_main.c >>>> @@ -684,10 +684,10 @@ void drm_sched_start(struct drm_gpu_scheduler *sched, bool full_recovery) >>>> drm_sched_job_done(s_job, -ECANCELED); >>>> } >>>> >>>> + drm_sched_submit_start(sched); >>>> + >>>> if (full_recovery) >>>> drm_sched_start_timeout_unlocked(sched); >>>> - >>>> - drm_sched_submit_start(sched); >>>> } >>>> EXPORT_SYMBOL(drm_sched_start); >>> >>> No. >>> > > I don't think we will ever agree on this but I pulled out this patch and > the next in Xe. It seems to work without these changes, I believe > understand why and think it should actually work without this change. If > for some reason it didn't work, I know how I can work around this in the > Xe submission backend. > > With this, I will drop these in the next rev. > > But more on why I disagree below... > >>> A timeout timer should be started before we submit anything down to the hardware. >>> See Message-ID: <ed3aca10-8a9f-4698-92f4-21558fa6cfe3@xxxxxxx>, >>> and Message-ID: <8e5eab14-9e55-42c9-b6ea-02fcc591266d@xxxxxxx>. >>> >>> You shouldn't start TDR at an arbitrarily late time after job >>> submission to the hardware. To close this, the timer is started >>> before jobs are submitted to the hardware. >>> >>> One possibility is to increase the timeout timer value. > > No matter what the timeout value is there will always be a race of TDR > firing before run_job() is called. It's not a "race". In all software and firmware I've seen, a timeout timer is started _before_ a command is submitted to firmware or hardware, respectively. > >> >> If we went with this general change as we see here and in the subsequent patch--starting >> the TDR _after_ submitting jobs for execution to the hardware--this is what generally happens, >> 1. submit one or many jobs for execution; >> 2. one or many jobs may execute, complete, hang, etc.; >> 3. at some arbitrary time in the future, start TDR. >> Which means that the timeout doesn't necessarily track the time allotted for a job to finish >> executing in the hardware. It ends up larger than intended. > > Yes, conversely it can be smaller the way it is coded now. Kinda just a > matter of opinion on which one to prefer. It should be large enough to contain the command/task/job making it to the hardware. We want to make sure there's no runaway job, _for_ the amount of time allotted to each job. -- Regards, Luben
