Re: [PATCH 1/1] drm/xe: Don't short circuit TDR on jobs not started

[Matthew Brost <matthew.brost@intel.com>]

On Fri, Oct 25, 2024 at 01:32:33PM -0600, Zanoni, Paulo R wrote:
> On Wed, 2024-10-23 at 17:41 +0000, Matthew Brost wrote:
> > On Wed, Oct 23, 2024 at 10:47:05AM -0600, Zanoni, Paulo R wrote:
> > > On Tue, 2024-10-22 at 16:27 -0700, Matthew Brost wrote:
> > > > Short circuiting TDR on jobs not started is an optimization which is not
> > > > required. On LNL we are facing an issue where jobs do not get scheduled
> > > > by the GuC for an unknown reason. Removing this optimization allows jobs
> > > > to get scheduled after TDR fire once which is a big improvement. Remove
> > > > this optimization for now while root causing job scheduling issue on
> > > > LNL.
> > > 
> > > I just tested it and it seems to do what it promises. Thanks! Having a
> > > 5 second hiccup is still horribly bad, but it is - checks math notes -
> > > infinitely better than waiting forever for a syncobj that will never be
> > > signaled.
> > > 
> > > This patch will *tremendously* help Mesa CI, since we can reproduce
> > > this bug all the time with Vulkan CTS tests.
> > > 
> > > Suggestions:
> > > 
> > > - Can we get a message on dmesg every time this hiccup happens? We're
> > > not sure if it's happening on real workloads on people's machines, so
> > > maybe having some sort of indication "oops, we just unstuck the batch
> > > you submitted 300 frames ago!" would help.
> > > 
> > 
> > We will add 'notice' level message if this occurs.
> 
> I may be wrong, but from what I understand, 'notice' level is something
> that will *not* show up on people's dmesg if they are using distros'
> default config. This message signals a bug is happening, we need to
> make sure it appears in dmesg by default. The whole point is to be able
> to figure out if this is happening in the wild. Can we promote this to
> KERN_WARNING?
> 

I'm honestly not sure what shows up where. 'notice' is same level as our
job timeout message though. If we need to raise this level, the job
timeout message should also be raised. To be safe, will roll both of
these changes out in a series - I wanted to refactor my latest rev of
this patch anyways.

Matt 

> > 
> > > - Since we don't know how long until the real fix, can this be tagged
> > > for stable? If it turns out this requires special GuC, it would be even
> > > more valuable to have this in stable since those tend to take more to
> > > propagate to people's machines.
> > 
> > I don't see any reason why this can't be backported, will include required tags.
> > 
> > Matt
> > 
> > > 
> > > Thanks a lot!
> > > 
> > > > 
> > > > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > > Signed-off-by: Matthew Brost <matthew.brost@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/xe/xe_guc_submit.c | 4 ----
> > > >  1 file changed, 4 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/xe/xe_guc_submit.c b/drivers/gpu/drm/xe/xe_guc_submit.c
> > > > index 0b81972ff651..25ab675e9c7d 100644
> > > > --- a/drivers/gpu/drm/xe/xe_guc_submit.c
> > > > +++ b/drivers/gpu/drm/xe/xe_guc_submit.c
> > > > @@ -1052,10 +1052,6 @@ guc_exec_queue_timedout_job(struct drm_sched_job *drm_job)
> > > >  		exec_queue_killed_or_banned_or_wedged(q) ||
> > > >  		exec_queue_destroyed(q);
> > > >  
> > > > -	/* Job hasn't started, can't be timed out */
> > > > -	if (!skip_timeout_check && !xe_sched_job_started(job))
> > > > -		goto rearm;
> > > > -
> > > >  	/*
> > > >  	 * If devcoredump not captured and GuC capture for the job is not ready
> > > >  	 * do manual capture first and decide later if we need to use it
> > > 
>