Re: [PATCH] drm/xe/pm: do not warn about missing runtime PM protection after hot-unplug

On Thu, Aug 06, 2026 at 02:38:03AM -0700, Matthew Brost wrote:
> On Thu, Aug 06, 2026 at 10:30:20AM +0200, Raag Jadav wrote:
> > On Thu, Aug 06, 2026 at 02:27:12PM +0530, Nitin Gote wrote:
> > > Exec queues are owned by user fds and are destroyed when the fd is closed.
> > > After hot-unplug this can happen from a deferred close during process
> > > exit, after the device has been removed and runtime PM has been disabled.
> > > The queue destroy path can call xe_pm_runtime_get_noresume(), and since
> > > runtime PM is disabled pm_runtime_get_if_in_use() returns no reference,
> > > so it warns about "Missing outer runtime PM protection".
> > > 
> > > This is a false positive for the hot-unplug teardown case. The device is
> > > already unplugged, and the queue destroy path that triggered this warning
> > > was checked and does not touch hardware state after unplug. GuC has already
> > > been sanitized by guc_fini_hw(), so no H2G is sent and the hardware
> > > teardown path is no longer reachable.
> > > 
> > > Skip the warning when the DRM device is already unplugged.
> > > 
> > > Observed with new IGT core_hotunplug subtests:
> > >   igt@core_hotunplug@hotreplug-with-load
> > >   igt@core_hotunplug@hotunplug-rescan-with-load
> > > 
> > > v2:
> > >  - Drop the drm_dev_is_unplugged() bypass from guc_exec_queue_destroy()
> > >    and instead exclude hot-unplug from the WARN in
> > >    xe_pm_runtime_get_noresume(). (Matthew Brost)
> > > 
> 
> I don't think this was suggestion but also I believe I misunderstood the
> problem, but maybe this works.
> 
> > > v3:
> > >  - Clarify that the queue destroy path was checked and does not touch
> > >    hardware state after unplug. (Matthew Auld)
> > > 
> > > Link: https://patchwork.freedesktop.org/patch/725773/?series=166744&rev=4
> > > Cc: Matthew Brost <[email protected]>
> > > Cc: Matthew Auld <[email protected]>
> > > Assisted-by: GitHub-Copilot:claude-opus-4.8
> > > Signed-off-by: Nitin Gote <[email protected]>
> > > ---
> > >  drivers/gpu/drm/xe/xe_pm.c | 5 ++++-
> > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/xe/xe_pm.c b/drivers/gpu/drm/xe/xe_pm.c
> > > index a5289a9df8d2..a038687d5b2c 100644
> > > --- a/drivers/gpu/drm/xe/xe_pm.c
> > > +++ b/drivers/gpu/drm/xe/xe_pm.c
> > > @@ -10,6 +10,7 @@
> > >  #include <linux/suspend.h>
> > >  #include <linux/dmi.h>
> > >  
> > > +#include <drm/drm_drv.h>
> > >  #include <drm/drm_managed.h>
> > >  #include <drm/ttm/ttm_placement.h>
> > >  
> > > @@ -914,7 +915,9 @@ void xe_pm_runtime_get_noresume(struct xe_device *xe)
> > 
> > From the documentation
> > 
> >  * This function should be used in inner places where it is surely already
> >  * protected by outer-bound callers of `xe_pm_runtime_get`.
> >
> 
> We should have those.
> 
> > >  	if (!ref) {
> > >  		pm_runtime_get_noresume(xe->drm.dev);
> > > -		drm_WARN(&xe->drm, !xe_pm_suspending_or_resuming(xe),
> > > +		drm_WARN(&xe->drm,
> > > +			 !drm_dev_is_unplugged(&xe->drm) &&
> > > +			 !xe_pm_suspending_or_resuming(xe),
> > 
> > So IMO this is more of a band-aid.
> > 
> 
> So if I'm understanding the problem correctly,
> pm_runtime_get_if_in_use() (via guc_exec_queue_add_msg()) fails when the
> device is unplugged? I don't see how the PM reference count could be
> zero, regardless of whether the device is plugged in or unplugged.
> 
> My suggestion was (and still is) that guc_exec_queue_destroy() should
> always add a message and determine whether the device/firmware is still
> alive while processing that message, then either kick the destory or
> issue H2G. Other messages likely are also missing proper device alive
> checks too.
> 
> If, for some reason, obtaining a PM reference via
> xe_pm_runtime_get_noresume() doesn't work when the device is unplugged,
> then that logic should be handled in the message submission layer. For
> example, use drm_dev_enter()/drm_dev_exit() for hot-unplug protection,
> only take a PM reference if the device is still bound, encode that
> information into the message, and then determine the appropriate next
> steps when the message is processed.
> 

Below is rough sketch of what I was thinking. We can also probably rip
out EXEC_QUEUE_FLAG_PERMANENT then too. We'd also need solid
explaination how why xe_pm_runtime_get_noresume doesn't work if device
is unplugged, as that part it unclear to me too.

diff --git a/drivers/gpu/drm/xe/xe_guc_submit.c b/drivers/gpu/drm/xe/xe_guc_submit.c
index 9036f89dff7d..08000f8db28a 100644
--- a/drivers/gpu/drm/xe/xe_guc_submit.c
+++ b/drivers/gpu/drm/xe/xe_guc_submit.c
@@ -1812,32 +1812,15 @@ static void __guc_exec_queue_destroy_async(struct work_struct *w)
 static void guc_exec_queue_destroy_async(struct xe_exec_queue *q)
 {
        INIT_WORK(&q->guc->destroy_async, __guc_exec_queue_destroy_async);
-
-       /* We must block on kernel engines so slabs are empty on driver unload */
-       if (q->flags & EXEC_QUEUE_FLAG_PERMANENT || exec_queue_wedged(q))
-               guc_exec_queue_do_destroy(q);
-       else
-               xe_destroy_wq_queue(&q->guc->destroy_async);
+       xe_destroy_wq_queue(&q->guc->destroy_async);
 }

-static void __guc_exec_queue_destroy(struct xe_guc *guc, struct xe_exec_queue *q)
-{
-       /*
-        * Might be done from within the GPU scheduler, need to do async as we
-        * fini the scheduler when the engine is fini'd, the scheduler can't
-        * complete fini within itself (circular dependency). Async resolves
-        * this we and don't really care when everything is fini'd, just that it
-        * is.
-        */
-       guc_exec_queue_destroy_async(q);
-}
-
-static void __guc_exec_queue_process_msg_cleanup(struct xe_sched_msg *msg)
+static void __guc_exec_queue_process_msg_cleanup(struct xe_sched_msg *msg,
+                                                bool bound)
 {
        struct xe_exec_queue *q = msg->private_data;
        struct xe_guc *guc = exec_queue_to_guc(q);

-       xe_gt_assert(guc_to_gt(guc), !(q->flags & EXEC_QUEUE_FLAG_PERMANENT));
        trace_xe_exec_queue_cleanup_entity(q);

        /*
@@ -1850,10 +1833,10 @@ static void __guc_exec_queue_process_msg_cleanup(struct xe_sched_msg *msg)
         *   it is safe to directly destroy the exec queue on driver side, as the GuC
         *   will not process further requests and all resources must be cleaned up locally.
         */
-       if (exec_queue_registered(q) && xe_uc_fw_is_running(&guc->fw))
+       if (bound && exec_queue_registered(q) && xe_uc_fw_is_running(&guc->fw))
                disable_scheduling_deregister(guc, q);
        else
-               __guc_exec_queue_destroy(guc, q);
+               guc_exec_queue_destroy_async(q);
 }

 static bool guc_exec_queue_allowed_to_change_state(struct xe_exec_queue *q)
@@ -1861,12 +1844,13 @@ static bool guc_exec_queue_allowed_to_change_state(struct xe_exec_queue *q)
        return !exec_queue_killed_or_banned_or_wedged(q) && exec_queue_registered(q);
 }

-static void __guc_exec_queue_process_msg_set_sched_props(struct xe_sched_msg *msg)
+static void __guc_exec_queue_process_msg_set_sched_props(struct xe_sched_msg *msg,
+                                                        bool bound)
 {
        struct xe_exec_queue *q = msg->private_data;
        struct xe_guc *guc = exec_queue_to_guc(q);

-       if (guc_exec_queue_allowed_to_change_state(q))
+       if (guc_exec_queue_allowed_to_change_state(q) && bound)
                init_policies(guc, q);
        kfree(msg);
 }
@@ -1904,13 +1888,14 @@ static void suspend_fence_signal(struct xe_exec_queue *q)
        __suspend_fence_signal(q);
 }

-static void __guc_exec_queue_process_msg_suspend(struct xe_sched_msg *msg)
+static void __guc_exec_queue_process_msg_suspend(struct xe_sched_msg *msg,
+                                                bool bound)
 {
        struct xe_exec_queue *q = msg->private_data;
        struct xe_guc *guc = exec_queue_to_guc(q);

        if (guc_exec_queue_allowed_to_change_state(q) && !exec_queue_suspended(q) &&
-           exec_queue_enabled(q)) {
+           exec_queue_enabled(q) && bound) {
                wait_event(guc->ct.wq, vf_recovery(guc) ||
                           ((q->guc->resume_time != RESUME_PENDING ||
                           xe_guc_read_stopped(guc)) && !exec_queue_pending_disable(q)));
@@ -1934,11 +1919,12 @@ static void __guc_exec_queue_process_msg_suspend(struct xe_sched_msg *msg)
        }
 }

-static void __guc_exec_queue_process_msg_resume(struct xe_sched_msg *msg)
+static void __guc_exec_queue_process_msg_resume(struct xe_sched_msg *msg,
+                                               bool bound)
 {
        struct xe_exec_queue *q = msg->private_data;

-       if (guc_exec_queue_allowed_to_change_state(q)) {
+       if (guc_exec_queue_allowed_to_change_state(q) && bound) {
                clear_exec_queue_suspended(q);
                if (!exec_queue_enabled(q)) {
                        q->guc->resume_time = RESUME_PENDING;
@@ -1950,17 +1936,20 @@ static void __guc_exec_queue_process_msg_resume(struct xe_sched_msg *msg)
        }
 }

-static void __guc_exec_queue_process_msg_set_multi_queue_priority(struct xe_sched_msg *msg)
+static void
+__guc_exec_queue_process_msg_set_multi_queue_priority(struct xe_sched_msg *msg,
+                                                     bool bound)
 {
        struct xe_exec_queue *q = msg->private_data;

-       if (guc_exec_queue_allowed_to_change_state(q))
+       if (guc_exec_queue_allowed_to_change_state(q) && bound)
                guc_exec_queue_send_cgp_sync(q, 0);

        kfree(msg);
 }

-static void __guc_exec_queue_process_msg_cgp_sync(struct xe_sched_msg *msg)
+static void __guc_exec_queue_process_msg_cgp_sync(struct xe_sched_msg *msg,
+                                                 bool bound)
 {
        struct xe_exec_queue *q = msg->private_data;

@@ -1969,7 +1958,7 @@ static void __guc_exec_queue_process_msg_cgp_sync(struct xe_sched_msg *msg)
         * CGP update + CGP_SYNC (re-applies the current priority from
         * q->multi_queue.priority).
         */
-       if (guc_exec_queue_allowed_to_change_state(q))
+       if (guc_exec_queue_allowed_to_change_state(q) && bound)
                guc_exec_queue_send_cgp_sync(q, 0);
 }

@@ -1982,37 +1971,46 @@ static void __guc_exec_queue_process_msg_cgp_sync(struct xe_sched_msg *msg)
 #define OPCODE_MASK    0xf
 #define MSG_LOCKED     BIT(8)
 #define MSG_HEAD       BIT(9)
+#define MSG_PM_REF     BIT(10)

 static void guc_exec_queue_process_msg(struct xe_sched_msg *msg)
 {
        struct xe_device *xe = guc_to_xe(exec_queue_to_guc(msg->private_data));
+       int idx;
+       bool pm_ref = !!(msg->opcode & MSG_PM_REF),
+            bound = drm_dev_enter(&xe->drm, &idx);

        trace_xe_sched_msg_recv(msg);

-       switch (msg->opcode) {
+       switch (msg->opcode & OPCODE_MASK) {
        case CLEANUP:
-               __guc_exec_queue_process_msg_cleanup(msg);
+               __guc_exec_queue_process_msg_cleanup(msg, bound);
                break;
        case SET_SCHED_PROPS:
-               __guc_exec_queue_process_msg_set_sched_props(msg);
+               __guc_exec_queue_process_msg_set_sched_props(msg, bound);
                break;
        case SUSPEND:
-               __guc_exec_queue_process_msg_suspend(msg);
+               __guc_exec_queue_process_msg_suspend(msg, bound);
                break;
        case RESUME:
-               __guc_exec_queue_process_msg_resume(msg);
+               __guc_exec_queue_process_msg_resume(msg, bound);
                break;
        case SET_MULTI_QUEUE_PRIORITY:
-               __guc_exec_queue_process_msg_set_multi_queue_priority(msg);
+               __guc_exec_queue_process_msg_set_multi_queue_priority(msg,
+                                                                     bound);
                break;
        case CGP_SYNC_MSG:
-               __guc_exec_queue_process_msg_cgp_sync(msg);
+               __guc_exec_queue_process_msg_cgp_sync(msg, bound);
                break;
        default:
                XE_WARN_ON("Unknown message type");
        }

-       xe_pm_runtime_put(xe);
+       if (pm_ref)
+               xe_pm_runtime_put(xe);
+
+       if (bound)
+               drm_dev_exit(idx);
 }

 static const struct drm_sched_backend_ops drm_sched_ops = {
@@ -2137,10 +2135,17 @@ static void guc_exec_queue_kill(struct xe_exec_queue *q)
 static void guc_exec_queue_add_msg(struct xe_exec_queue *q, struct xe_sched_msg *msg,
                                   u32 opcode)
 {
-       xe_pm_runtime_get_noresume(guc_to_xe(exec_queue_to_guc(q)));
+       struct xe_guc *guc = exec_queue_to_guc(q);
+       struct xe_device *xe = guc_to_xe(guc);
+       int idx;
+       bool bound = drm_dev_enter(&xe->drm, &idx);

        INIT_LIST_HEAD(&msg->link);
        msg->opcode = opcode & OPCODE_MASK;
+       if (bound) {
+               xe_pm_runtime_get_noresume(guc_to_xe(exec_queue_to_guc(q)));
+               msg->opcode |= MSG_PM_REF;
+       }
        msg->private_data = q;

        trace_xe_sched_msg_add(msg);
@@ -2150,6 +2155,9 @@ static void guc_exec_queue_add_msg(struct xe_exec_queue *q, struct xe_sched_msg
                xe_sched_add_msg_locked(&q->guc->sched, msg);
        else
                xe_sched_add_msg(&q->guc->sched, msg);
+
+       if (bound)
+               drm_dev_exit(idx);
 }

 static void guc_exec_queue_try_add_msg_head(struct xe_exec_queue *q,
@@ -2182,10 +2190,7 @@ static void guc_exec_queue_destroy(struct xe_exec_queue *q)
 {
        struct xe_sched_msg *msg = q->guc->static_msgs + STATIC_MSG_CLEANUP;

-       if (!(q->flags & EXEC_QUEUE_FLAG_PERMANENT) && !exec_queue_wedged(q))
-               guc_exec_queue_add_msg(q, msg, CLEANUP);
-       else
-               __guc_exec_queue_destroy(exec_queue_to_guc(q), q);
+       guc_exec_queue_add_msg(q, msg, CLEANUP);
 }

 static int guc_exec_queue_set_priority(struct xe_exec_queue *q,
@@ -2650,7 +2655,7 @@ static void guc_exec_queue_stop(struct xe_guc *guc, struct xe_exec_queue *q)
        }

        if (do_destroy)
-               __guc_exec_queue_destroy(guc, q);
+               guc_exec_queue_destroy_async(q);
 }

 static int guc_submit_reset_prepare(struct xe_guc *guc)
@@ -3296,7 +3301,7 @@ static void handle_deregister_done(struct xe_guc *guc, struct xe_exec_queue *q)
        trace_xe_exec_queue_deregister_done(q);

        clear_exec_queue_registered(q);
-       __guc_exec_queue_destroy(guc, q);
+       guc_exec_queue_destroy_async(q);
 }

> Sorry a lot of this stuff in xe_guc_submit.c around corner cases /
> teardwons is a mess from early Xe work I did, we should aim to clean
> this up with clear semantics.
> 
> Matt
> 
> > Raag
> > 
> > >  			 "Missing outer runtime PM protection\n");
> > >  	}
> > >  }
> > > -- 
> > > 2.50.1
> > >