| Message ID | 20220923142934.29528-1-tvrtko.ursulin@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: Stop using flush_scheduled_work on driver remove | expand |
On Fri, Sep 23, 2022 at 03:29:34PM +0100, Tvrtko Ursulin wrote: > From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > Kernel is trying to eliminate callers of flush_scheduled_work so lets > try to accommodate. > > We currently call it from intel_modeset_driver_remove_noirq on the driver > remove path but the comment next to it does not tell me what exact work it > wants to flush. > > I can spot three (or four) works using the system_wq: > > ..hotplug.reenable_work > ..hotplug.hotplug_work Looks like we at least try to shoot those down via intel_irq_uninstall() ->intel_hpd_cancel_work() ->cancel_delayed_work_sync() But I'm not sure how broken the hpd disable path is here. I know hpd cancel vs. irq disable has some known ordering issues during suspend at least, some of which I think may have gotten fixed recently. But hpd cancel is still a bit of a mess in general. Here we at least do cancel all the hpd works after irqs have been disabled, so I don't think any further flushing should help with whatever races we have left in there. > ..psr.dc3co_work I think the whole dc3co thing should be disabled atm, so nothing should ever schedule this. We should probably garbage collect the whole thing... > ..crtc->drrs.work That one should have been killed in intel_display_driver_unregister() ->drm_atomic_helper_shutdown() ->... ->intel_drrs_deactivate() ->cancel_delayed_work_sync() > So if I replace it with intel_hpd_cancel_work() that appears would handle > the first two. What about the other two? Other stuff that comes to mind is the pps vdd_off work. But looks like that should get taken down in the encoder->destroy() hook at the latest (via intel_mode_config_cleanup()). psr.work at least has a cancel_work_sync() in intel_psr_disable(), so should hopefully get killed the same way as drrs. opregion.asle_work seems to get cancelled from the unregister path. The ones that look broken to me are dmc.work and fbc underrun_work. > > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > Cc: Jani Nikula <jani.nikula@intel.com> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> > --- > I am clueless about the display paths and only send this because Jani > convinced me to send a patch to kick off the discussion. No expectations > whatsoever this is correct or complete. > --- > drivers/gpu/drm/i915/display/intel_display.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c > index 2d0018ae34b1..0eb72530a003 100644 > --- a/drivers/gpu/drm/i915/display/intel_display.c > +++ b/drivers/gpu/drm/i915/display/intel_display.c > @@ -8980,7 +8980,7 @@ void intel_modeset_driver_remove_noirq(struct drm_i915_private *i915) > intel_unregister_dsm_handler(); > > /* flush any delayed tasks or pending work */ > - flush_scheduled_work(); > + intel_hpd_cancel_work(i915); > > intel_hdcp_component_fini(i915); > > -- > 2.34.1
On 23/09/2022 17:16, Ville Syrjälä wrote: > On Fri, Sep 23, 2022 at 03:29:34PM +0100, Tvrtko Ursulin wrote: >> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >> >> Kernel is trying to eliminate callers of flush_scheduled_work so lets >> try to accommodate. >> >> We currently call it from intel_modeset_driver_remove_noirq on the driver >> remove path but the comment next to it does not tell me what exact work it >> wants to flush. >> >> I can spot three (or four) works using the system_wq: >> >> ..hotplug.reenable_work >> ..hotplug.hotplug_work > > Looks like we at least try to shoot those down via > intel_irq_uninstall() > ->intel_hpd_cancel_work() > ->cancel_delayed_work_sync() > > But I'm not sure how broken the hpd disable path is here. > I know hpd cancel vs. irq disable has some known ordering > issues during suspend at least, some of which I think may > have gotten fixed recently. But hpd cancel is still a bit > of a mess in general. > > Here we at least do cancel all the hpd works after irqs > have been disabled, so I don't think any further flushing > should help with whatever races we have left in there. > >> ..psr.dc3co_work > > I think the whole dc3co thing should be disabled atm, > so nothing should ever schedule this. We should > probably garbage collect the whole thing... > >> ..crtc->drrs.work > > That one should have been killed in > intel_display_driver_unregister() > ->drm_atomic_helper_shutdown() > ->... > ->intel_drrs_deactivate() > ->cancel_delayed_work_sync() > >> So if I replace it with intel_hpd_cancel_work() that appears would handle >> the first two. What about the other two? > > Other stuff that comes to mind is the pps vdd_off work. > But looks like that should get taken down in the > encoder->destroy() hook at the latest (via > intel_mode_config_cleanup()). > > psr.work at least has a cancel_work_sync() in intel_psr_disable(), > so should hopefully get killed the same way as drrs. > > opregion.asle_work seems to get cancelled from the unregister path. > > The ones that look broken to me are dmc.work and fbc underrun_work. Right, so I missed some and things are a bit more complicated. Okay to leave this with you, even if on a backlog? Regards, Tvrtko > >> >> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >> Cc: Jani Nikula <jani.nikula@intel.com> >> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> >> Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> >> --- >> I am clueless about the display paths and only send this because Jani >> convinced me to send a patch to kick off the discussion. No expectations >> whatsoever this is correct or complete. >> --- >> drivers/gpu/drm/i915/display/intel_display.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c >> index 2d0018ae34b1..0eb72530a003 100644 >> --- a/drivers/gpu/drm/i915/display/intel_display.c >> +++ b/drivers/gpu/drm/i915/display/intel_display.c >> @@ -8980,7 +8980,7 @@ void intel_modeset_driver_remove_noirq(struct drm_i915_private *i915) >> intel_unregister_dsm_handler(); >> >> /* flush any delayed tasks or pending work */ >> - flush_scheduled_work(); >> + intel_hpd_cancel_work(i915); >> >> intel_hdcp_component_fini(i915); >> >> -- >> 2.34.1 >
diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c index 2d0018ae34b1..0eb72530a003 100644 --- a/drivers/gpu/drm/i915/display/intel_display.c +++ b/drivers/gpu/drm/i915/display/intel_display.c @@ -8980,7 +8980,7 @@ void intel_modeset_driver_remove_noirq(struct drm_i915_private *i915) intel_unregister_dsm_handler(); /* flush any delayed tasks or pending work */ - flush_scheduled_work(); + intel_hpd_cancel_work(i915); intel_hdcp_component_fini(i915);