[Intel-xe] [PATCH 1/2] drm/xe: Enable atomics on smem

[Nirmoy Das]

On 7/27/2023 2:33 AM, Welty, Brian wrote:
>
> On 7/26/2023 12:42 AM, Nirmoy Das wrote:
>> Atomics works on smem as well on supported platforms so
>> set AE PTE bit on smem as well if XE_VMA_ATOMIC_PTE_BIT
>> flag is set.
>>
>> v2: Remove platform check(Lucas)
>>
>> Signed-off-by: Nirmoy Das <nirmoy.das at intel.com>
>> ---
>>   drivers/gpu/drm/xe/xe_pt.c | 7 ++++---
>>   1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_pt.c b/drivers/gpu/drm/xe/xe_pt.c
>> index d4660520ac2c..3bd6dd048305 100644
>> --- a/drivers/gpu/drm/xe/xe_pt.c
>> +++ b/drivers/gpu/drm/xe/xe_pt.c
>> @@ -768,10 +768,11 @@ xe_pt_stage_bind(struct xe_tile *tile, struct 
>> xe_vma *vma,
>
> Why only changing this function and change is not also needed in 
> xe_pte_encode() ?

I didn't see userspace vmas are passed through xe_pte_encode() so I was 
avoiding that. Also xe_pte_encode() is always being

called with NULL vma.

>
>
>>       struct xe_pt *pt = xe_vma_vm(vma)->pt_root[tile->id];
>>       int ret;
>
> You need a xe_walk.default_pte = 0 upfront with way code is structured 
> now....

thanks, I have to fix that.

>
>
>> +    if (vma && vma->gpuva.flags & XE_VMA_ATOMIC_PTE_BIT)
>> +        xe_walk.default_pte = XE_USM_PPGTT_PTE_AE;
>> +
>
> How is this supposed to be enable on non-PVC platform?
> Meaning when is XE_VMA_ATOMIC_PTE_BIT set in the xe_vma.gpuva.flags ?
> Are you wanting it to be controlled by the xe_vm_madvise ioctl?


I am looking for opinions here but  this is how I see it as there is a 
vm_madvise option for device atomics  but I might be wrong.

One option is to enable AE for all vmas depending if platform supports 
and and  umd can use vm_madvise option to check if the platfrom

supports it or not.

Hi Joonas, what is your thought on this?

> At least on i915, those were intended to only be used on platforms 
> with pagefault support as the intent was just to influence migration 
> policy.
>
>
> I think this breaks PVC?  We do still want AE bit clear on PVC when 
> backing store is !vram. 

Thanks, I didn't know that.

Regards,

Nirmoy

> I don't see how you can do this right way
> here without a platform check.   Possibly you could restore the code 
> below.   And then the new code above is only executed on platforms 
> without VRAM.   Not sure.
>
>
>>       if (is_vram) {
>> -        xe_walk.default_pte = XE_PPGTT_PTE_LM;
>> -        if (vma && vma->gpuva.flags & XE_VMA_ATOMIC_PTE_BIT)
>> -            xe_walk.default_pte |= XE_USM_PPGTT_PTE_AE;
>> +        xe_walk.default_pte |= XE_PPGTT_PTE_LM;
>>           xe_walk.dma_offset = vram_region_gpu_offset(bo->ttm.resource);
>>           xe_walk.cache = XE_CACHE_WB;
>>       } else {