Re: [PATCH v12 11/13] drm/xe: Force flush system memory AuxCCS framebuffers before scan out

[Tvrtko Ursulin <tvrtko.ursulin@igalia.com>]

On 23/09/2025 11:19, Ville Syrjälä wrote:
> On Tue, Sep 23, 2025 at 11:08:04AM +0100, 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 | 14 +++++++++++++-
>>   drivers/gpu/drm/xe/xe_bo_types.h       | 14 +++++++++-----
>>   2 files changed, 22 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xe/display/xe_fb_pin.c b/drivers/gpu/drm/xe/display/xe_fb_pin.c
>> index d8aa23b8cf14..f247c0da6b9e 100644
>> --- a/drivers/gpu/drm/xe/display/xe_fb_pin.c
>> +++ b/drivers/gpu/drm/xe/display/xe_fb_pin.c
>> @@ -382,6 +382,7 @@ static struct i915_vma *__xe_pin_fb_vma(const struct intel_framebuffer *fb,
>>   	struct xe_bo *bo = gem_to_xe_bo(obj);
>>   	struct xe_validation_ctx ctx;
>>   	struct drm_exec exec;
>> +	bool first_pin;
>>   	int ret = 0;
>>   
>>   	if (!vma)
>> @@ -422,8 +423,11 @@ static struct i915_vma *__xe_pin_fb_vma(const struct intel_framebuffer *fb,
>>   			ret = xe_bo_validate(bo, NULL, true, &exec);
>>   		drm_exec_retry_on_contention(&exec);
>>   		xe_validation_retry_on_oom(&ctx, &ret);
>> -		if (!ret)
>> +		if (!ret) {
>>   			ttm_bo_pin(&bo->ttm);
>> +			first_pin = !bo->display_pin;
>> +			bo->display_pin = true;
>> +		}
>>   	}
>>   	if (ret)
>>   		goto err;
>> @@ -436,6 +440,14 @@ static struct i915_vma *__xe_pin_fb_vma(const struct intel_framebuffer *fb,
>>   	if (ret)
>>   		goto err_unpin;
>>   
>> +	/*
>> +	 * 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));
> 
> You still haven't found the actual bug that causes the dirty cache?

Sadly no. I cross referenced everything numerous times, including 
workarounds, tried pretty much 1:1 i915 vs xe ring buffer programming, 
but this extra flush always remains required. I also heard that how 
flushing of the aux metadata works isn't documented anywhere and i915 
does have this flush, by design or accident I don't know.

Regards,

Tvrtko

>> +
>>   	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 d4fe3c8dca5b..8119d8d6d174 100644
>> --- a/drivers/gpu/drm/xe/xe_bo_types.h
>> +++ b/drivers/gpu/drm/xe/xe_bo_types.h
>> @@ -81,11 +81,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: true means that CCS region of BO is already cleared */
>> -	bool ccs_cleared;
>>   
>>   	/** @bb_ccs: BB instructions of CCS read/write. Valid only for VF */
>>   	struct xe_bb *bb_ccs[XE_SRIOV_VF_CCS_CTX_COUNT];
>> @@ -100,6 +95,15 @@ struct xe_bo {
>>   	/** @devmem_allocation: SVM device memory allocation */
>>   	struct drm_pagemap_devmem devmem_allocation;
>>   
>> +	/** @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;
>> +
>>   	/** @vram_userfault_link: Link into @mem_access.vram_userfault.list */
>>   	struct list_head vram_userfault_link;
>>   
>> -- 
>> 2.48.0
>