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 14:20, Ville Syrjälä wrote:
> On Tue, Sep 23, 2025 at 01:25:58PM +0100, Tvrtko Ursulin wrote:
>>
>> On 23/09/2025 13:01, Ville Syrjälä wrote:
>>> On Tue, Sep 23, 2025 at 11:48:59AM +0100, Tvrtko Ursulin wrote:
>>>>
>>>> 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.
>>>
>>> i915 has the flush for the *whole* bo because it started out as
>>> cached. As soon as it undergoes a cached->uncached change (either
>>> due to set_caching ioctl or becoming a display scanout buffer) we
>>> clflush it and switch the GPU page tables from WB to UC. After
>>> that the bo stays uncached, and will need no further clflushes,
>>> assuming that everyone follows the rules:
>>>
>>> - CPU accesses via a WB mapping to an uncached bo will explicitly
>>>     clflush before (invalidate) and after (write-back) the access
>>> - userspace always sets the GPU to use use "consult the PTE" MOCS
>>>     setting for any potential scanout buffer. That way the GPU will
>>>     use WB as long as the bo is cached, and once it becomes
>>>     uncached the GPU also switches to UC accesses
>>>
>>> With xe I presume the BO should already start out as UC/WC with
>>> clean caches, and nothing should be dirtying the caches unless
>>> there is a real bug somewhere.
>>
>> Correct, it is all WC and MOCS are correctly set as WC.
>>
>> For example for the media compression flavour that was broken and needed
>> fixing in b412b144685f ("lib/intel/veboxcopy: Respect buffer MOCS
>> index"). (For the render compression flavour MOCS was already correct.)
>> But MOCS is not enough on its own for some reason.
>>
>> I also looked into the rendercopy surface state programming and that too
>> looks okay. In fact it is possible to see how using the wrong MOCS makes
>> things worse. But it also appears the surface state MOCS only applies to
>> the main surface, while the aux state is the one which appears to
>> contain cached/unflushed data.
> 
> Are you saying that if you start scanning out a compressed buffer,
> then clean the caches, and then do frontbuffer rendering you get
> more cache dirt?

Render with the GPU or CPU? Do you know of any tests or userspaces which 
do that?

Regards,

Tvrtko