Re: [PATCH 10/12] drm/xe: Force flush system memory AuxCCS framebuffers before scan out

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Fri, Feb 21, 2025 at 10:17:29AM +0000, Tvrtko Ursulin wrote:
> Even though frame buffer objects are created as write-combined, in
> practice, on top of all the ring buffer flushing, an additional clflush
> seems to be needed before display engine can coherently scan out the
> AuxCCS compressed data without transient artifacts.
> 
> If for comparison we look at how i915 handles things (where AuxCCS works
> fine), as it happens it has this same clflush before a frame buffer is
> pinned for display for the first time, courtesy the dynamic tracking of
> the buffer cache mode and setting the latter to uncached before handing
> to display.
> 
> Since xe considers the buffer object caching mode as static we can
> implement the same approach by adding a flag telling us if the buffer
> was ever pinned for display and flush on the first pin. Subsequent re-pins
> will not repeat the clflush but so far I have not observed any glitching
> after the first pin.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
> ---
>  drivers/gpu/drm/xe/display/xe_fb_pin.c | 13 +++++++++++++
>  drivers/gpu/drm/xe/xe_bo_types.h       | 14 +++++++++-----
>  2 files changed, 22 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/display/xe_fb_pin.c b/drivers/gpu/drm/xe/display/xe_fb_pin.c
> index a32f3603751a..5e7813154733 100644
> --- a/drivers/gpu/drm/xe/display/xe_fb_pin.c
> +++ b/drivers/gpu/drm/xe/display/xe_fb_pin.c
> @@ -331,6 +331,7 @@ static struct i915_vma *__xe_pin_fb_vma(const struct intel_framebuffer *fb,
>  	struct i915_vma *vma = kzalloc(sizeof(*vma), GFP_KERNEL);
>  	struct drm_gem_object *obj = intel_fb_bo(&fb->base);
>  	struct xe_bo *bo = gem_to_xe_bo(obj);
> +	bool first_pin;
>  	int ret;
>  
>  	if (!vma)
> @@ -362,6 +363,9 @@ static struct i915_vma *__xe_pin_fb_vma(const struct intel_framebuffer *fb,
>  	if (ret)
>  		goto err;
>  
> +	first_pin = !bo->display_pin;
> +	bo->display_pin = true;
> +
>  	if (IS_DGFX(xe))
>  		ret = xe_bo_migrate(bo, XE_PL_VRAM0);
>  	else
> @@ -382,6 +386,15 @@ static struct i915_vma *__xe_pin_fb_vma(const struct intel_framebuffer *fb,
>  
>  	/* Ensure DPT writes are flushed */
>  	xe_device_l2_flush(xe);
> +
> +	/*
> +	 * Force flush frame buffer data for non-coherent display access when
> +	 * AuxCCS formats are used.
> +	 */
> +	if (first_pin && !xe_bo_is_vram(bo) && !xe_bo_is_stolen(bo) &&
> +	    intel_fb_is_ccs_modifier(fb->base.modifier))
> +		drm_clflush_sg(xe_bo_sg(bo));
> +
>  	return vma;
>  
>  err_unpin:
> diff --git a/drivers/gpu/drm/xe/xe_bo_types.h b/drivers/gpu/drm/xe/xe_bo_types.h
> index 60c522866500..f518becb78c9 100644
> --- a/drivers/gpu/drm/xe/xe_bo_types.h
> +++ b/drivers/gpu/drm/xe/xe_bo_types.h
> @@ -67,11 +67,6 @@ struct xe_bo {
>  	struct llist_node freed;
>  	/** @update_index: Update index if PT BO */
>  	int update_index;
> -	/** @created: Whether the bo has passed initial creation */
> -	bool created;
> -
> -	/** @ccs_cleared */
> -	bool ccs_cleared;
>  
>  	/**
>  	 * @cpu_caching: CPU caching mode. Currently only used for userspace
> @@ -80,6 +75,15 @@ struct xe_bo {
>  	 */
>  	u16 cpu_caching;
>  
> +	/** @created: Whether the bo has passed initial creation */
> +	bool created : 1;
> +
> +	/** @ccs_cleared */
> +	bool ccs_cleared : 1;
> +
> +	/** @display_pin: Was it ever pinned to display */
> +	bool display_pin : 1;

Is there any way that we could have this contained within display domains
instead of spreading display specific things to the core part?

> +
>  	/** @vram_userfault_link: Link into @mem_access.vram_userfault.list */
>  		struct list_head vram_userfault_link;
>  
> -- 
> 2.48.0
>