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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Tue, Sep 23, 2025 at 03:40:55PM +0100, Tvrtko Ursulin wrote:
> 
> 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?

On the GPU.

Can't think of anything nice off the top of my head. 

I'd probably just write a quick igt:
1. create a compressed fb
2. flip to it
3. manually clflush the whole thing to be sure it's clean
4. rendercopy some junk around, and keep an eye out for cache dirt

-- 
Ville Syrjälä
Intel