[Intel-xe] [PATCH] drm/xe: Remove PIPE_CONTROL_TILE_CACHE_FLUSH from PIPE_CONTROL in gfx20+

[Lucas De Marchi]

On Fri, Oct 06, 2023 at 05:23:36PM +0000, Jose Souza wrote:
>On Fri, 2023-10-06 at 12:16 -0500, Lucas De Marchi wrote:
>> On Fri, Oct 06, 2023 at 09:50:53AM -0700, Jose Souza wrote:
>> > PIPE_CONTROL_TILE_CACHE_FLUSH/bit 28 is now reserved bit in PIPE_CONTROL
>> > instruction for gfx20+, so here only setting it for older graphics
>> > versions.
>> >
>> > Cc: Lucas De Marchi <lucas.demarchi at intel.com>
>> > Signed-off-by: José Roberto de Souza <jose.souza at intel.com>
>> > ---
>> > drivers/gpu/drm/xe/xe_ring_ops.c | 5 ++++-
>> > 1 file changed, 4 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/drivers/gpu/drm/xe/xe_ring_ops.c b/drivers/gpu/drm/xe/xe_ring_ops.c
>> > index 6eec7c7e4bc56..c2aec32fc46ad 100644
>> > --- a/drivers/gpu/drm/xe/xe_ring_ops.c
>> > +++ b/drivers/gpu/drm/xe/xe_ring_ops.c
>> > @@ -157,16 +157,19 @@ static int emit_store_imm_ppgtt_posted(u64 addr, u64 value,
>> > static int emit_render_cache_flush(struct xe_sched_job *job, u32 *dw, int i)
>> > {
>> > 	struct xe_gt *gt = job->q->gt;
>> > +	struct xe_device *xe = gt->tile->xe;
>>
>> gt_to_xe()
>>
>> > 	bool lacks_render = !(gt->info.engine_mask & XE_HW_ENGINE_RCS_MASK);
>> > 	u32 flags;
>> >
>> > 	flags = (PIPE_CONTROL_CS_STALL |
>> > -		 PIPE_CONTROL_TILE_CACHE_FLUSH |
>> > 		 PIPE_CONTROL_RENDER_TARGET_CACHE_FLUSH |
>> > 		 PIPE_CONTROL_DEPTH_CACHE_FLUSH |
>> > 		 PIPE_CONTROL_DC_FLUSH_ENABLE |
>> > 		 PIPE_CONTROL_FLUSH_ENABLE);
>> >
>> > +	if (GRAPHICS_VERx100(xe) < 2000)
>> > +		flags |= PIPE_CONTROL_TILE_CACHE_FLUSH;
>>
>> Looks correct that it doesn't exist in xe2. But looking at bspec 56551,
>> it doesn't seem it can't be set, but rather that it doesn't have any
>> effect on xe2. Is this not the case?
>
>Yeah I don't see any tests failing because this bit is set in Xe or Mesa but better not set reserved bits.

Why better? We do that all the time in the driver when the bit is just
"not valid for the platform" rather than "can't be set".  When it's
explicitely marked as reserved then it's a case by case analysis.

Lucas De Marchi

>
>>
>> Lucas De Marchi
>>
>> > +
>> > 	if (XE_WA(gt, 1409600907))
>> > 		flags |= PIPE_CONTROL_DEPTH_STALL;
>> >
>> > --
>> > 2.42.0
>> >
>