[PATCH 1/3] drm/xe: Consolidate setting PTE_AE into one place

[Nirmoy Das]

Hi Matt,

On 4/12/2024 11:34 AM, Nirmoy Das wrote:
> Hi Matt,
>
> On 4/12/2024 1:22 AM, Matt Roper wrote:
>> On Wed, Apr 10, 2024 at 07:03:06PM +0200, Nirmoy Das wrote:
>>> Currently decision to set PTE_AE is spread between xe_pt
>>> and xe_vm files and there is no reason to be keep it that
>>> way. Consolidate the logic for better maintainability.
>> Does this series bisect properly?  I.e., if we run the driver with this
>> patch applied, but the other two patches missing, isn't it going to turn
>> on the AE bit in the page table in some BMG SMEM cases where it
>> shouldn't?  It seems like we should at least mention that in the commit
>> message to avoid confusion.
> Sorry for the confusion; it came up because I made a mistake with the 
> if-conditions. I didn't intend to introduce any functional changes 
> with this patch.
>
> Version 2 corrects the if-condition, which now only allows setting 
> PTE_AE on PVC+VRAM, XE2+VRAM, and XE2+iGFX, thus maintaining the 
> functionality as it was


I miss understood your comment. Indeed, this is setting PTE_AE when only 
BO exist  and leaving others. This need to be noted in the commit 
message. I will do that.


Regards,

Nirmoy

>
> Regards,
>
> Nirmoy
>
>>
>>
>> Matt
>>
>>> This also remove the extra care needed for PVC which only
>>> allows setting PTE_AE for LMEM.
>>>
>>> Signed-off-by: Nirmoy Das <nirmoy.das at intel.com>
>>> ---
>>>   drivers/gpu/drm/xe/xe_pt.c | 4 +---
>>>   drivers/gpu/drm/xe/xe_vm.c | 7 ++++---
>>>   2 files changed, 5 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/xe/xe_pt.c b/drivers/gpu/drm/xe/xe_pt.c
>>> index 5b7930f46cf3..7dc13a8bb44f 100644
>>> --- a/drivers/gpu/drm/xe/xe_pt.c
>>> +++ b/drivers/gpu/drm/xe/xe_pt.c
>>> @@ -597,7 +597,6 @@ static int
>>>   xe_pt_stage_bind(struct xe_tile *tile, struct xe_vma *vma,
>>>            struct xe_vm_pgtable_update *entries, u32 *num_entries)
>>>   {
>>> -    struct xe_device *xe = tile_to_xe(tile);
>>>       struct xe_bo *bo = xe_vma_bo(vma);
>>>       bool is_devmem = !xe_vma_is_userptr(vma) && bo &&
>>>           (xe_bo_is_vram(bo) || xe_bo_is_stolen_devmem(bo));
>>> @@ -619,8 +618,7 @@ xe_pt_stage_bind(struct xe_tile *tile, struct 
>>> xe_vma *vma,
>>>       struct xe_pt *pt = xe_vma_vm(vma)->pt_root[tile->id];
>>>       int ret;
>>>   -    if ((vma->gpuva.flags & XE_VMA_ATOMIC_PTE_BIT) &&
>>> -        (is_devmem || !IS_DGFX(xe)))
>>> +    if (vma->gpuva.flags & XE_VMA_ATOMIC_PTE_BIT)
>>>           xe_walk.default_pte |= XE_USM_PPGTT_PTE_AE;
>>>         if (is_devmem) {
>>> diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
>>> index a196dbe65252..8f3474c5f480 100644
>>> --- a/drivers/gpu/drm/xe/xe_vm.c
>>> +++ b/drivers/gpu/drm/xe/xe_vm.c
>>> @@ -904,9 +904,6 @@ static struct xe_vma *xe_vma_create(struct xe_vm 
>>> *vm,
>>>       for_each_tile(tile, vm->xe, id)
>>>           vma->tile_mask |= 0x1 << id;
>>>   -    if (GRAPHICS_VER(vm->xe) >= 20 || vm->xe->info.platform == 
>>> XE_PVC)
>>> -        vma->gpuva.flags |= XE_VMA_ATOMIC_PTE_BIT;
>>> -
>>>       vma->pat_index = pat_index;
>>>         if (bo) {
>>> @@ -914,6 +911,10 @@ static struct xe_vma *xe_vma_create(struct 
>>> xe_vm *vm,
>>>             xe_bo_assert_held(bo);
>>>   +        if (GRAPHICS_VER(vm->xe) >= 20 || xe_bo_is_vram(bo) ||
>>> +            !IS_DGFX(vm->xe))
>>> +            vma->gpuva.flags |= XE_VMA_ATOMIC_PTE_BIT;
>>> +
>>>           vm_bo = drm_gpuvm_bo_obtain(vma->gpuva.vm, &bo->ttm.base);
>>>           if (IS_ERR(vm_bo)) {
>>>               xe_vma_free(vma);
>>> -- 
>>> 2.42.0
>>>