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

[Nirmoy Das <nirmoy.das@linux.intel.com>]

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

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@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
>>