Re: [PATCH v4 20/20] drm/xe/madvise: Skip vma invalidation if mem attr are unchanged

["Ghimiray, Himal Prasad" <himal.prasad.ghimiray@intel.com>]

On 24-06-2025 03:58, Matthew Brost wrote:
> On Fri, Jun 13, 2025 at 06:25:58PM +0530, Himal Prasad Ghimiray wrote:
>> If a VMA within the madvise input range already has the same memory
>> attribute as the one requested by the user, skip PTE zapping for that
>> VMA to avoid unnecessary invalidation.
>>
>> Suggested-by: Matthew Brost <matthew.brost@intel.com>
>> Signed-off-by: Himal Prasad Ghimiray <himal.prasad.ghimiray@intel.com>
>> ---
>>   drivers/gpu/drm/xe/xe_vm.c         |  1 +
>>   drivers/gpu/drm/xe/xe_vm_madvise.c | 57 ++++++++++++++++++------------
>>   drivers/gpu/drm/xe/xe_vm_types.h   |  6 ++++
>>   3 files changed, 42 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
>> index d9ce25f3abf4..56d6c286e3d3 100644
>> --- a/drivers/gpu/drm/xe/xe_vm.c
>> +++ b/drivers/gpu/drm/xe/xe_vm.c
>> @@ -1226,6 +1226,7 @@ static struct xe_vma *xe_vma_create(struct xe_vm *vm,
>>   		vma->gpuva.flags |= XE_VMA_ATOMIC_PTE_BIT;
>>   
>>   	vma->attr = *attr;
>> +	vma->skip_invalidation = 0;
> 
> This kzalloc'd so not needed.
> 
>>   
>>   	if (bo) {
>>   		struct drm_gpuvm_bo *vm_bo;
>> diff --git a/drivers/gpu/drm/xe/xe_vm_madvise.c b/drivers/gpu/drm/xe/xe_vm_madvise.c
>> index 5b96c8fc73a5..06e40ab0970e 100644
>> --- a/drivers/gpu/drm/xe/xe_vm_madvise.c
>> +++ b/drivers/gpu/drm/xe/xe_vm_madvise.c
>> @@ -88,13 +88,18 @@ static void madvise_preferred_mem_loc(struct xe_device *xe, struct xe_vm *vm,
>>   	xe_assert(vm->xe, op->type == DRM_XE_VMA_ATTR_PREFERRED_LOC);
>>   
>>   	for (i = 0; i < num_vmas; i++) {
>> -		vmas[i]->attr.preferred_loc.devmem_fd = op->preferred_mem_loc.devmem_fd;
>> -
>> -		/* Till multi-device support is not added migration_policy
>> -		 * is of no use and can be ignored.
>> -		 */
>> -		vmas[i]->attr.preferred_loc.migration_policy =
>> +		if (vmas[i]->attr.preferred_loc.devmem_fd == op->preferred_mem_loc.devmem_fd &&
>> +		    vmas[i]->attr.preferred_loc.migration_policy ==
>> +		    op->preferred_mem_loc.migration_policy) {
>> +			vmas[i]->skip_invalidation = 1;
> 
> In the else statement, you need to clear this so subsequent madvise
> which change properties do issue invalidations.

Missed it. Will fix

> 
>> +		} else {
>> +			vmas[i]->attr.preferred_loc.devmem_fd = op->preferred_mem_loc.devmem_fd;
>> +			/* Till multi-device support is not added migration_policy
>> +			 * is of no use and can be ignored.
>> +			 */
>> +			vmas[i]->attr.preferred_loc.migration_policy =
>>   						op->preferred_mem_loc.migration_policy;
>> +		}
>>   	}
>>   }
>>   
>> @@ -109,7 +114,10 @@ static void madvise_atomic(struct xe_device *xe, struct xe_vm *vm,
>>   	xe_assert(vm->xe, op->atomic.val <= DRM_XE_VMA_ATOMIC_CPU);
>>   
>>   	for (i = 0; i < num_vmas; i++) {
>> -		vmas[i]->attr.atomic_access = op->atomic.val;
>> +		if (vmas[i]->attr.atomic_access == op->atomic.val)
>> +			vmas[i]->skip_invalidation = 1;
>> +		else
>> +			vmas[i]->attr.atomic_access = op->atomic.val;
>>   
>>   		bo = xe_vma_bo(vmas[i]);
>>   		if (!bo)
>> @@ -134,9 +142,12 @@ static void madvise_pat_index(struct xe_device *xe, struct xe_vm *vm,
>>   
>>   	xe_assert(vm->xe, op->type == DRM_XE_VMA_ATTR_PAT);
>>   
>> -	for (i = 0; i < num_vmas; i++)
>> -		vmas[i]->attr.pat_index = op->pat_index.val;
>> -
>> +	for (i = 0; i < num_vmas; i++) {
>> +		if (vmas[i]->attr.pat_index == op->pat_index.val)
>> +			vmas[i]->skip_invalidation = 1;
>> +		else
>> +			vmas[i]->attr.pat_index = op->pat_index.val;
>> +	}
>>   }
>>   
>>   typedef void (*madvise_func)(struct xe_device *xe, struct xe_vm *vm,
>> @@ -161,23 +172,25 @@ static void xe_zap_ptes_in_madvise_range(struct xe_vm *vm, u64 start, u64 end, u
>>   				  false, MAX_SCHEDULE_TIMEOUT) <= 0)
>>   		XE_WARN_ON(1);
>>   
>> -	*tile_mask = xe_svm_ranges_zap_ptes_in_range(vm, start, end);
>> -
>>   	drm_gpuvm_for_each_va_range(gpuva, &vm->gpuvm, start, end) {
>>   		struct xe_vma *vma = gpuva_to_vma(gpuva);
>>   
>> -		if (xe_vma_is_cpu_addr_mirror(vma))
>> +		if (vma->skip_invalidation)
>>   			continue;
>>   
>> -		if (xe_vma_is_userptr(vma)) {
>> -			WARN_ON_ONCE(!dma_resv_test_signaled(xe_vm_resv(xe_vma_vm(vma)),
>> -							     DMA_RESV_USAGE_BOOKKEEP));
>> -		}
>> -
>> -		for_each_tile(tile, vm->xe, id) {
>> -			if (xe_pt_zap_ptes(tile, vma)) {
>> -				*tile_mask |= BIT(id);
>> -				vma->tile_invalidated |= BIT(id);
>> +		if (xe_vma_is_cpu_addr_mirror(vma)) {
>> +			*tile_mask |= xe_svm_ranges_zap_ptes_in_range(vm,
>> +								      xe_vma_start(vma),
>> +								      xe_vma_end(vma));
>> +		} else {
>> +			if (xe_vma_is_userptr(vma))
>> +				WARN_ON_ONCE(!dma_resv_test_signaled(xe_vm_resv(xe_vma_vm(vma)),
>> +								     DMA_RESV_USAGE_BOOKKEEP));
>> +			for_each_tile(tile, vm->xe, id) {
>> +				if (xe_pt_zap_ptes(tile, vma)) {
>> +					*tile_mask |= BIT(id);
>> +					vma->tile_invalidated |= BIT(id);
>> +				}
>>   			}
>>   		}
>>   	}
>> diff --git a/drivers/gpu/drm/xe/xe_vm_types.h b/drivers/gpu/drm/xe/xe_vm_types.h
>> index c7156f8e2ed2..3b3019ecbfab 100644
>> --- a/drivers/gpu/drm/xe/xe_vm_types.h
>> +++ b/drivers/gpu/drm/xe/xe_vm_types.h
>> @@ -151,6 +151,12 @@ struct xe_vma {
>>   	/** @tile_staged: bind is staged for this VMA */
>>   	u8 tile_staged;
>>   
>> +	/**
>> +	 * @skip_invalidation: Used in madvise to avoid invalidation
>> +	 * if mem attributes doesn't change
>> +	 */
>> +	u32 skip_invalidation;
> 
> bool or I think you can safely stuff this into gpuva flags (e.g.,
> DRM_GPUVA_USERBITS) as this only set / cleared / viewed by a single
> thread and all other flags are set at init time or bind time (i.e., no
> races).

would change it to bool.

  If you choose the latter, or regardless, we should probably
> document the gpuva usage.

do you mean gpuva userbit flags ?
  >
> Matt
> 
>> +
>>   	/**
>>   	 * @ufence: The user fence that was provided with MAP.
>>   	 * Needs to be signalled before UNMAP can be processed.
>> -- 
>> 2.34.1
>>