On Wed, 13 May 2026, Imre Deak <[email protected]> wrote: > On Wed, May 13, 2026 at 05:19:19PM +0300, Jani Nikula wrote: >> On Wed, 13 May 2026, Ville Syrjälä <[email protected]> wrote: >> > On Wed, May 13, 2026 at 10:58:35AM +0300, Jani Nikula wrote: >> >> xe_display_flush_cleanup_work() is a bit of an oddball function in xe >> >> display code. There shouldn't be anything this specific or xe >> >> specific. While I'm not sure what the correct refactor for the function >> >> should be, move it to shared display code for starters, next to the >> >> eerily similar but slightly different intel_has_pending_fb_unpin() that >> >> is only called from i915 core. >> >> >> >> The main goal here is to unblock some refactors on >> >> for_each_intel_crtc(). >> >> >> >> Signed-off-by: Jani Nikula <[email protected]> >> >> --- >> >> drivers/gpu/drm/i915/display/intel_display.c | 21 +++++++++++++++ >> >> drivers/gpu/drm/i915/display/intel_display.h | 1 + >> >> drivers/gpu/drm/xe/display/xe_display.c | 27 +++----------------- >> >> 3 files changed, 25 insertions(+), 24 deletions(-) >> >> >> >> diff --git a/drivers/gpu/drm/i915/display/intel_display.c >> >> b/drivers/gpu/drm/i915/display/intel_display.c >> >> index d5cf1476c7b9..50feca52b962 100644 >> >> --- a/drivers/gpu/drm/i915/display/intel_display.c >> >> +++ b/drivers/gpu/drm/i915/display/intel_display.c >> >> @@ -737,6 +737,27 @@ bool intel_has_pending_fb_unpin(struct intel_display >> >> *display) >> >> return false; >> >> } >> >> >> >> +void intel_display_flush_cleanup_work(struct intel_display *display) >> >> +{ >> >> + struct intel_crtc *crtc; >> >> + >> >> + for_each_intel_crtc(display->drm, crtc) { >> >> + struct drm_crtc_commit *commit; >> >> + >> >> + spin_lock(&crtc->base.commit_lock); >> >> + commit = list_first_entry_or_null(&crtc->base.commit_list, >> >> + struct drm_crtc_commit, >> >> commit_entry); >> >> + if (commit) >> >> + drm_crtc_commit_get(commit); >> >> + spin_unlock(&crtc->base.commit_lock); >> >> + >> >> + if (commit) { >> >> + wait_for_completion(&commit->cleanup_done); >> >> + drm_crtc_commit_put(commit); >> >> + } >> >> + } >> >> +} >> >> + >> >> /* >> >> * Finds the encoder associated with the given CRTC. This can only be >> >> * used when we know that the CRTC isn't feeding multiple encoders! >> >> diff --git a/drivers/gpu/drm/i915/display/intel_display.h >> >> b/drivers/gpu/drm/i915/display/intel_display.h >> >> index a43ada0c0502..65f8c81a7bae 100644 >> >> --- a/drivers/gpu/drm/i915/display/intel_display.h >> >> +++ b/drivers/gpu/drm/i915/display/intel_display.h >> >> @@ -402,6 +402,7 @@ void intel_disable_transcoder(const struct >> >> intel_crtc_state *old_crtc_state); >> >> void i830_enable_pipe(struct intel_display *display, enum pipe pipe); >> >> void i830_disable_pipe(struct intel_display *display, enum pipe pipe); >> >> bool intel_has_pending_fb_unpin(struct intel_display *display); >> >> +void intel_display_flush_cleanup_work(struct intel_display *display); >> >> void intel_encoder_destroy(struct drm_encoder *encoder); >> >> struct drm_display_mode * >> >> intel_encoder_current_mode(struct intel_encoder *encoder); >> >> diff --git a/drivers/gpu/drm/xe/display/xe_display.c >> >> b/drivers/gpu/drm/xe/display/xe_display.c >> >> index aa73023b7398..ef27fdfdbab2 100644 >> >> --- a/drivers/gpu/drm/xe/display/xe_display.c >> >> +++ b/drivers/gpu/drm/xe/display/xe_display.c >> >> @@ -258,27 +258,6 @@ static bool suspend_to_idle(void) >> >> return false; >> >> } >> >> >> >> -static void xe_display_flush_cleanup_work(struct xe_device *xe) >> >> -{ >> >> - struct intel_crtc *crtc; >> >> - >> >> - for_each_intel_crtc(&xe->drm, crtc) { >> >> - struct drm_crtc_commit *commit; >> >> - >> >> - spin_lock(&crtc->base.commit_lock); >> >> - commit = list_first_entry_or_null(&crtc->base.commit_list, >> >> - struct drm_crtc_commit, >> >> commit_entry); >> >> - if (commit) >> >> - drm_crtc_commit_get(commit); >> >> - spin_unlock(&crtc->base.commit_lock); >> >> - >> >> - if (commit) { >> >> - wait_for_completion(&commit->cleanup_done); >> >> - drm_crtc_commit_put(commit); >> >> - } >> >> - } >> >> -} >> >> - >> >> static void xe_display_enable_d3cold(struct xe_device *xe) >> >> { >> >> struct intel_display *display = xe->display; >> >> @@ -292,7 +271,7 @@ static void xe_display_enable_d3cold(struct xe_device >> >> *xe) >> >> */ >> >> intel_power_domains_disable(display); >> >> >> >> - xe_display_flush_cleanup_work(xe); >> >> + intel_display_flush_cleanup_work(display); >> >> >> >> intel_opregion_suspend(display, PCI_D3cold); >> >> >> >> @@ -347,7 +326,7 @@ void xe_display_pm_suspend(struct xe_device *xe) >> >> intel_display_driver_suspend(display); >> >> } >> >> >> >> - xe_display_flush_cleanup_work(xe); >> >> + intel_display_flush_cleanup_work(display); >> > >> > intel_display_driver_suspend() already flushes the cleanup wq. So I >> > think this is doing nothing. The correct answer seems to be to nuke >> > the whole thing. We are missing the wq flush from the shutdown() path >> > in i915 however, so I suppose we should add it there. >> > >> >> >> >> intel_encoder_block_all_hpds(display); >> >> >> >> @@ -379,7 +358,7 @@ void xe_display_pm_shutdown(struct xe_device *xe) >> >> intel_display_driver_suspend(display); >> > >> > This should rather be the same atomic helper shutdown that i915 uses. >> > I guess what we want is a intel_display_driver_shutdown() to pair >> > up with intel_display_driver_suspend(). >> >> Yeah, well, another "Hal fixes a light bulb" moment. I just wanted to >> clean up the iterators, but I can't do that with xe having crtc >> iteration, which it never should have had in the first place. >> >> I think all of the i915/xe/display probe/cleanup/suspend/resume paths >> are a gigantic mess. It was a mess with just i915, and xe added another, >> *different* mess. They both do things differently, but *neither* should >> be calling low-level display stuff directly. >> >> I'll try to cook something up for this. > > Fwiw, this came up already earlier [1] and then I came up with > https://github.com/ideak/linux/commits/suspend-shutdown-refactor > > but haven't followed up with it. For reference I rebased it now on > drm-tip. > > [1] https://lore.kernel.org/all/aIjizdet9ZUXB-yx@ideak-desk
I've started on this, but I'm planning on taking a slightly different route from what Imre has there. The direction is the same, move more of this detailed stuff to display. But Imre also has a change similar to the patch at hand as the first thing. And that's the thing. I feel like starting to fix this one in xe code blocks the remainder of this series, and blocks the straighforward refactors of the probe/remove/suspend/remove parts. IMO it's easier to unify with all this code moved to display first. Ville, as I wrote in the commit message, can we just move this out of the way to unblock further changes? It's a non-functional change, doesn't make anything better or worse, apart from removing CRTC iteration from xe code, and unblocks further work. BR, Jani. > >> >> >> BR, >> Jani. >> >> >> > >> >> } >> >> >> >> - xe_display_flush_cleanup_work(xe); >> >> + intel_display_flush_cleanup_work(display); >> >> intel_dp_mst_suspend(display); >> >> intel_encoder_block_all_hpds(display); >> >> intel_hpd_cancel_work(display); >> >> -- >> >> 2.47.3 >> >> -- >> Jani Nikula, Intel -- Jani Nikula, Intel
