Re: [PATCH v2] drm/{i915,xe}: move xe_display_flush_cleanup_work() to i915 display

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

On Tue, May 26, 2026 at 09:09:28PM +0300, Jani Nikula wrote:
> On Tue, 26 May 2026, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> > On Mon, May 25, 2026 at 02:05:53PM +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().
> >> 
> >> v2: Add FIXME comment (Ville)
> >> 
> >> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/display/intel_display.c | 22 ++++++++++++++++
> >>  drivers/gpu/drm/i915/display/intel_display.h |  1 +
> >>  drivers/gpu/drm/xe/display/xe_display.c      | 27 +++-----------------
> >>  3 files changed, 26 insertions(+), 24 deletions(-)
> >> 
> >> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> >> index 6c8935f69db1..a6cee0f81358 100644
> >> --- a/drivers/gpu/drm/i915/display/intel_display.c
> >> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> >> @@ -737,6 +737,28 @@ bool intel_has_pending_fb_unpin(struct intel_display *display)
> >>  	return false;
> >>  }
> >>  
> >> +/* FIXME: All callers need to be audited and unified between i915 and xe */
> >
> > That makes me think we want to keep this. I was more thinking of
> > something like
> > /* FIXME remove this and just flush the cleanup wq where appropriate */
> 
> Fair enough. Can I push with that?

Acked-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> 
> >
> >> +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 45a90d2fe6ec..72f33113a5a3 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 8d08da60336d..a5066de3d789 100644
> >> --- a/drivers/gpu/drm/xe/display/xe_display.c
> >> +++ b/drivers/gpu/drm/xe/display/xe_display.c
> >> @@ -244,27 +244,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;
> >> @@ -278,7 +257,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);
> >>  
> >> @@ -333,7 +312,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_encoder_block_all_hpds(display);
> >>  
> >> @@ -365,7 +344,7 @@ void xe_display_pm_shutdown(struct xe_device *xe)
> >>  		intel_display_driver_suspend(display);
> >>  	}
> >>  
> >> -	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

-- 
Ville Syrjälä
Intel