Re: [Intel-xe] [PATCH 2/3] drm/xe: Add missing TLB invalidation to emit_pipe_invalidate()

["Thomas Hellström" <thomas.hellstrom@linux.intel.com>]

On 5/29/23 16:56, Thomas Hellström wrote:
>
> On 5/29/23 16:48, Souza, Jose wrote:
>> On Mon, 2023-05-29 at 11:08 +0200, Thomas Hellström wrote:
>>> On Fri, 2023-05-26 at 12:06 -0700, José Roberto de Souza wrote:
>>>> i915 invalidates TLB before emit BB start, so doing the same in Xe.
>>> Hi, José,
>>>
>>> Do you see an issue because of missing TLB flushes? In that case that
>>> needs to be added to the commit message. We do TLB flushes on unbind,
>>> but not sure if we do them on rebind, so if that's the issue we need to
>>> figure out whether we should do them also on rebind or, like in this
>>> patch, on each exec.
>> I have a group of tests that results flips randomly. It fails when 
>> the rendered buffer is compared to expected result.
>> Anything that add a bit of delay after the exec fixes those tests so 
>> I was looking for any missing flush in Xe KMD and Mesa.
>>
>> This one did not fixed it but as i915 was doing it I thought would be 
>> good to do in Xe too.
>
> I think missing TLB invalidations are more likely to cause random 
> overwrites of freed memory. Let me do a quick check on these. But the 
> problem you're describing indeed sounds more like a missing render 
> cache flush.
>
It indeed looks like we're doing proper TLB invalidation on both rebind 
and unbind, so this patch shouldn't really be needed.

(look for "invalidation_fence_init()")

/Thomas

> /Thomas
>
>>
>>> Thanks,
>>> Thomas
>>>
>>>
>>>> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
>>>> ---
>>>>   drivers/gpu/drm/xe/regs/xe_gpu_commands.h | 1 +
>>>>   drivers/gpu/drm/xe/xe_ring_ops.c          | 6 ++++--
>>>>   2 files changed, 5 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/xe/regs/xe_gpu_commands.h
>>>> b/drivers/gpu/drm/xe/regs/xe_gpu_commands.h
>>>> index 0f9c5b0b8a3ba..7c7320efea739 100644
>>>> --- a/drivers/gpu/drm/xe/regs/xe_gpu_commands.h
>>>> +++ b/drivers/gpu/drm/xe/regs/xe_gpu_commands.h
>>>> @@ -73,6 +73,7 @@
>>>>   #define
>>>> PIPE_CONTROL_STORE_DATA_INDEX                        (1<<21)
>>>>   #define
>>>> PIPE_CONTROL_CS_STALL                                (1<<20)
>>>>   #define PIPE_CONTROL_GLOBAL_SNAPSHOT_RESET           (1<<19)
>>>> +#define PIPE_CONTROL_TLB_INVALIDATE                  (1<<18)
>>>>   #define
>>>> PIPE_CONTROL_PSD_SYNC                                (1<<17)
>>>>   #define
>>>> PIPE_CONTROL_QW_WRITE                                (1<<14)
>>>>   #define PIPE_CONTROL_DEPTH_STALL                     (1<<13)
>>>> diff --git a/drivers/gpu/drm/xe/xe_ring_ops.c
>>>> b/drivers/gpu/drm/xe/xe_ring_ops.c
>>>> index d2fa0b4c8bcc0..4f3ef39109b9b 100644
>>>> --- a/drivers/gpu/drm/xe/xe_ring_ops.c
>>>> +++ b/drivers/gpu/drm/xe/xe_ring_ops.c
>>>> @@ -37,7 +37,8 @@
>>>>                  PIPE_CONTROL_INDIRECT_STATE_DISABLE | \
>>>>                  PIPE_CONTROL_FLUSH_ENABLE | \
>>>>                  PIPE_CONTROL_TEXTURE_CACHE_INVALIDATE | \
>>>> -               PIPE_CONTROL_DC_FLUSH_ENABLE)
>>>> +               PIPE_CONTROL_DC_FLUSH_ENABLE | \
>>>> +               PIPE_CONTROL_TLB_INVALIDATE)
>>>>     static u32 preparser_disable(bool state)
>>>>   {
>>>> @@ -117,7 +118,8 @@ static int emit_pipe_invalidate(u32 mask_flags,
>>>> u32 *dw, int i)
>>>>                  PIPE_CONTROL_CONST_CACHE_INVALIDATE |
>>>>                  PIPE_CONTROL_STATE_CACHE_INVALIDATE |
>>>>                  PIPE_CONTROL_QW_WRITE |
>>>> -               PIPE_CONTROL_STORE_DATA_INDEX;
>>>> +               PIPE_CONTROL_STORE_DATA_INDEX |
>>>> +               PIPE_CONTROL_TLB_INVALIDATE;
>>>>            flags &= ~mask_flags;