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 17:38 +0000, Summers, Stuart wrote:
> 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...

Just to add quickly that I see the message queue you're talking about
and I agree this should still be working. I still haven't figured out
why those messages get lost in my scenario. I'll report back as soon as
I have something there.

Thanks,
Stuart

> 
> 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
> > > > > 
> > > 
>