Re: [RESEND 1/6] drm/{i915, xe}: move xe_display_flush_cleanup_work() to i915 display

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Fri, May 22, 2026 at 11:34:14AM +0300, Jani Nikula wrote:
> On Wed, 13 May 2026, Imre Deak <imre.deak@intel.com> wrote:
> > On Wed, May 13, 2026 at 05:19:19PM +0300, Jani Nikula wrote:
> >> On Wed, 13 May 2026, Ville Syrjälä <ville.syrjala@linux.intel.com> 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 <jani.nikula@intel.com>
> >> >> ---
> >> >>  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.

I guess. Maybe just toss in a FIXME to delete this later.

-- 
Ville Syrjälä
Intel