Re: [PATCH 6/7] drm/xe: Don't block messages to the GPU scheduler

["Summers, Stuart" <stuart.summers@intel.com>]

On Mon, 2025-10-13 at 10:31 -0700, Matthew Brost wrote:
> On Mon, Oct 13, 2025 at 11:17:58AM -0600, Summers, Stuart wrote:
> > On Mon, 2025-10-13 at 09:56 -0700, Matthew Brost wrote:
> > > On Mon, Oct 13, 2025 at 04:25:03PM +0000, Stuart Summers wrote:
> > > > Right now we are using the state of the GPU scheduler
> > > > to determine whether we send and receive messages. There
> > > > are some states, however, where we might intentionally
> > > > pause the scheduler, like a device wedge, and expect that
> > > > messages are resumed later once the user has taken the
> > > > hardware state and is attempting to reset, like an unbind.
> > > > 
> > > > Remove these checks in the XeKMD and let the GPU scheduler
> > > > handle state checks internally.
> > > > 
> > > 
> > > We can't do this. The entire queue stop / starting mechanism
> > > relies
> > > on
> > > getting exclusive access to the queue by ensuring the scheduler
> > > is
> > > fully
> > > stopped - this includes messages. This will break job timeouts,
> > > GT
> > > reset
> > > flows, and VF migration.
> > 
> > I'm not sure I full understand here. The scheduler should be
> > stopped as
> > it was before, it just means we keep sending messages right? I can
> > test
> > the job timeout piece to make sure...
> > 
> 
> This will show up as an obscure race condition — 99% of the time it
> will
> work just fine, but I can assure you it will break the entire design
> of
> submission.

Ok

> 
> > Basically I'm arguing the start/stop mechanics should be inside the
> > scheduler and not in the calling driver.
> > 
> 
> The message interface is built on top of the DRM scheduler, rather
> than
> integrated into it. Originally, I had it built into the scheduler,
> but
> based on feedback, I moved the channel to the driver side. Therefore,
> we
> need to hook into the stopping mechanism on the driver side. The
> layering could use some cleanup, but the functionality will remain on
> the driver side.

Ah, thanks for the history there. I wasn't aware of that.

> 
> > > 
> > > What exactly is the problem you are trying to solve? The device
> > > is
> > > wedged and queues are stopped, then an unbind occurs? That is
> > > probably a
> > > bug. IIRC even wedging a device / tearing down a queue we should
> > > always
> > > start the queue again. We could assert in guc_submit_wedged_fini
> > > that
> > 
> > I think there's basically a race between sending the cleanup
> > message
> > and stopping the scheduler. And once we send that message, we don't
> > really track it on the xe side. So if we artificially pause things
> > on
> > the xe side (by adding the checks I'm removing in this patch), we
> > can
> > get into a scenario where the cleanup message is sent *after* the
> > scheduler is paused and thus that cleanup message gets dropped, and
> > we
> > never issue the deregistration for that particular exec queue.
> > 
> 
> That's not how stopping works. Stopping prevents future work items
> from
> being queued and flushes all in-flight work items. These work items
> include running jobs, freeing jobs, or processing messages. When
> stopped,
> each of these interfaces can set up state so that when the scheduler
> is
> started again, the work items are requeued for processing (e.g.,
> messages are stored in a linked list). The key point is that when the
> scheduler is stopped, work items that could be modifying the queue
> state
> are not running, so the entity that stopped the scheduler has
> exclusive
> access—without requiring any locks.

Ok so basically we still need to keep interacting with the scheduler
when paused, we just aren't starting the work queue. Then once the
queue starts we expect all of those messages to be processed.

Let me look a little harder at why that isn't happening here and I'll
get back...

Thanks,
Stuart

> 
> Matt
> 
> > Thanks,
> > Stuart
> > 
> > > all queues are not paused.
> > > 
> > > Also if you having issues on unbind - there is this patch [1]
> > > which
> > > fixes an issue too. I'm going to merge [1] now.
> > > 
> > > Matt
> > > 
> > > [1] https://patchwork.freedesktop.org/series/155417/
> > > 
> > > > Signed-off-by: Stuart Summers <stuart.summers@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/xe/xe_gpu_scheduler.c | 6 +-----
> > > >  1 file changed, 1 insertion(+), 5 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/xe/xe_gpu_scheduler.c
> > > > b/drivers/gpu/drm/xe/xe_gpu_scheduler.c
> > > > index f91e06d03511..d9d6fb641188 100644
> > > > --- a/drivers/gpu/drm/xe/xe_gpu_scheduler.c
> > > > +++ b/drivers/gpu/drm/xe/xe_gpu_scheduler.c
> > > > @@ -7,8 +7,7 @@
> > > >  
> > > >  static void xe_sched_process_msg_queue(struct xe_gpu_scheduler
> > > > *sched)
> > > >  {
> > > > -       if (!READ_ONCE(sched->base.pause_submit))
> > > > -               queue_work(sched->base.submit_wq, &sched-
> > > > > work_process_msg);
> > > > +       queue_work(sched->base.submit_wq, &sched-
> > > > > work_process_msg);
> > > >  }
> > > >  
> > > >  static void xe_sched_process_msg_queue_if_ready(struct
> > > > xe_gpu_scheduler *sched)
> > > > @@ -43,9 +42,6 @@ static void xe_sched_process_msg_work(struct
> > > > work_struct *w)
> > > >                 container_of(w, struct xe_gpu_scheduler,
> > > > work_process_msg);
> > > >         struct xe_sched_msg *msg;
> > > >  
> > > > -       if (READ_ONCE(sched->base.pause_submit))
> > > > -               return;
> > > > -
> > > >         msg = xe_sched_get_msg(sched);
> > > >         if (msg) {
> > > >                 sched->ops->process_msg(msg);
> > > > -- 
> > > > 2.34.1
> > > > 
> >