[PATCH 29/31] drm/xe/display: Kill crtc commit flush

[Cavitt, Jonathan]

-----Original Message-----
From: Intel-gfx <intel-gfx-bounces at lists.freedesktop.org> On Behalf Of Rodrigo Vivi
Sent: Tuesday, September 24, 2024 1:36 PM
To: intel-gfx at lists.freedesktop.org; intel-xe at lists.freedesktop.org
Cc: Deak, Imre <imre.deak at intel.com>; Vivi, Rodrigo <rodrigo.vivi at intel.com>
Subject: [PATCH 29/31] drm/xe/display: Kill crtc commit flush
> 
> This flush was needed in regular suspend cases in the past.
> After the clean-up and reconciliation with the i915 it was
> not needed anymore and removed. However this remained here
> in the runtime suspend path.
> 
> However, the runtime pm flow ensures that there won't be any
> flying or pending crtc commit when the runtime_suspend is
> called. So this is not needed here. Clean it up.
> 

LGTM, though maybe the commit message could use some
refactoring.  Something like:

"""
xe_display_flush_cleanup_work was originally needed for
regular suspend cases.  After the clean-up and reconciliation
with the i915, however, it was no longer needed and removed.
Despite this, the function remained as a part of the runtime
suspend path.

Since the runtime pm flow ensures that there won't be any
flying or pending crtc commits when the runtime_suspend
is called, calling xe_display_flush_cleanup_work here is no
longer necessary.  With no other use cases, this function
can safely be removed.
"""

Just a suggestion.  Not blocking.

Reviewed-by: Jonathan Cavitt <jonathan.cavitt at intel.com>
-Jonathan Cavitt

> Signed-off-by: Rodrigo Vivi <rodrigo.vivi at intel.com>
> ---
>  drivers/gpu/drm/xe/display/xe_display.c | 23 -----------------------
>  1 file changed, 23 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/display/xe_display.c b/drivers/gpu/drm/xe/display/xe_display.c
> index 780c8d7f392d..23bdd8904c44 100644
> --- a/drivers/gpu/drm/xe/display/xe_display.c
> +++ b/drivers/gpu/drm/xe/display/xe_display.c
> @@ -283,27 +283,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_to_d3cold(struct xe_device *xe)
>  {
>  	struct intel_display *display = &xe->display;
> @@ -311,8 +290,6 @@ static void xe_display_to_d3cold(struct xe_device *xe)
>  	/* We do a lot of poking in a lot of registers, make sure they work properly. */
>  	intel_power_domains_disable(xe);
>  
> -	xe_display_flush_cleanup_work(xe);
> -
>  	intel_hpd_cancel_work(xe);
>  
>  	intel_opregion_suspend(display, PCI_D3cold);
> -- 
> 2.46.0
> 
>