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

[Tvrtko Ursulin]

On 28/02/2025 19:34, Rodrigo Vivi wrote:
> 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 at 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?

On a mechanical level could put it in struct intel_framebuffer->"was 
this fb ever pinned before". (Assuming fb->bo is invariant for its 
lifetime?) But would that be any better I don't have a strong opinion. 
Perhaps partly because it is currently unknown to be why is this extra 
CPU flush even needed on the first pin. Like which HW component is the 
root cause.

Regards,

Tvrtko

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