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

On Fri, Aug 07, 2026 at 07:38:40AM -0600, Gote, Nitin R wrote:
> Hi Matt,
> 
> > -----Original Message-----
> > From: Brost, Matthew <[email protected]>
> > Sent: Friday, August 7, 2026 12:32 AM
> > To: Jadav, Raag <[email protected]>
> > Cc: Gote, Nitin R <[email protected]>; [email protected]; Auld,
> > Matthew <[email protected]>
> > Subject: 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.
> > >
> 
> Yes, it was. Initially, I thought to send this as a separate patch once this issue was fixed. 
> However, I think it's better to include it in this patch itself.
> 
> > > 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.
> 
> Regarding why xe_pm_runtime_get_noresume() doesn't work after hot-unplug, I did some debugging. 
> You were right that this is not a zero refcount case. At the warning observed: usage_count=3, status=2(RPM_SUSPENDED) and disable_depth=1.
> 
> During hot-unplug, the PCI remove path goes through: device_del() -> pm_runtime_remove() -> __pm_runtime_disable(), which disables runtime pm 
> and increments disable_depth from 0 to 1. The old struct device stays alive because the old drm_device is still referenced by an open fd. 
> When that fd is finally closed, guc_exec_queue_destroy() runs on the old device where runtime PM is already disabled (disable_depth=1). 
> In that state, pm_runtime_get_if_in_use() returns -EINVAL regardless of the non-zero usage count, which causes xe_pm_runtime_get_noresume() 
> to hit the "Missing outer runtime PM protection" WARN.
> 

Thanks for looking ino this part. Make sure to put this into the commit
and bonus points for updating our xe_pm.c doc around hazard of PM refs
after the device is hotplugged. I suspect this isn't the only case in Xe
where we have missing drm_dev_enter/exits.

Matt 

> So this does not appear to be an actual missing runtime PM reference. The failure is caused by runtime PM already being disabled during device removal. 
> I'll move the handling to the message submission layer as you suggested, taking the PM reference only while drm_dev_enter() indicates the device is still bound, 
> instead of trying to gate the warning.
> 
> To confirm this, I added some debug log and saw:
> disable_depth=1, status=2(RPM_SUSPENDED),  usage_count=3
> 
> Thank you for the rough sketch, Matt. I'll work on that approach.
> 
> Nitin
> 
> > 
> > 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
> > > > >