[PATCH] drm/xe/vm: Subclass userptr vmas

Thomas Hellström thomas.hellstrom at linux.intel.com
Tue Jan 30 17:51:22 UTC 2024


On 1/30/24 18:29, Matthew Brost wrote:
> On Tue, Jan 30, 2024 at 06:02:07PM +0100, Thomas Hellström wrote:
>> The construct allocating only parts of the vma structure when
>> the userptr part is not needed is very fragile. A developer could
>> add additional fields below the userptr part, and the code could
>> easily attempt to access the userptr part even if its not persent.
> I think we do something similar xe_sched_job wrt to the batch_addr
> field. We might want to clean that up too.
OK, yes we might want to try to get that in in the rc series, we got 
some comments from Linus and others on this, (see the xe chat for pointers).
>> So introduce xe_userptr_vma which subclasses struct xe_vma the
>> proper way, and accordingly modify a couple of interfaces.
>> This should also help if adding userptr helpers to drm_gpuvm.
>>
> Patch itself makes sense to me, good cleanup. One nit below.

Thanks, I'll fix that and the checkpatch.pl check, and add some links in 
the commit message.

/Thomas


>
>> Fixes: a4cc60a55fd9 ("drm/xe: Only alloc userptr part of xe_vma for userptrs")
>> Cc: Rodrigo Vivi <rodrigo.vivi at intel.com>
>> Cc: Matthew Brost <matthew.brost at intel.com>
>> Cc: Lucas De Marchi <lucas.demarchi at intel.com>
>> Signed-off-by: Thomas Hellström <thomas.hellstrom at linux.intel.com>
>> ---
>>   drivers/gpu/drm/xe/xe_gt_pagefault.c |  11 ++-
>>   drivers/gpu/drm/xe/xe_pt.c           |  32 ++++----
>>   drivers/gpu/drm/xe/xe_vm.c           | 114 ++++++++++++++-------------
>>   drivers/gpu/drm/xe/xe_vm.h           |  16 +++-
>>   drivers/gpu/drm/xe/xe_vm_types.h     |  16 ++--
>>   5 files changed, 108 insertions(+), 81 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_gt_pagefault.c b/drivers/gpu/drm/xe/xe_gt_pagefault.c
>> index 7ce67c9d30a7..78970259cea9 100644
>> --- a/drivers/gpu/drm/xe/xe_gt_pagefault.c
>> +++ b/drivers/gpu/drm/xe/xe_gt_pagefault.c
>> @@ -165,7 +165,8 @@ static int handle_pagefault(struct xe_gt *gt, struct pagefault *pf)
>>   		goto unlock_vm;
>>   	}
>>   
>> -	if (!xe_vma_is_userptr(vma) || !xe_vma_userptr_check_repin(vma)) {
>> +	if (!xe_vma_is_userptr(vma) ||
>> +	    !xe_vma_userptr_check_repin(to_userptr_vma(vma))) {
>>   		downgrade_write(&vm->lock);
>>   		write_locked = false;
>>   	}
>> @@ -181,11 +182,13 @@ static int handle_pagefault(struct xe_gt *gt, struct pagefault *pf)
>>   	/* TODO: Validate fault */
>>   
>>   	if (xe_vma_is_userptr(vma) && write_locked) {
>> +		struct xe_userptr_vma *uvma = to_userptr_vma(vma);
>> +
>>   		spin_lock(&vm->userptr.invalidated_lock);
>> -		list_del_init(&vma->userptr.invalidate_link);
>> +		list_del_init(&uvma->userptr.invalidate_link);
>>   		spin_unlock(&vm->userptr.invalidated_lock);
>>   
>> -		ret = xe_vma_userptr_pin_pages(vma);
>> +		ret = xe_vma_userptr_pin_pages(uvma);
>>   		if (ret)
>>   			goto unlock_vm;
>>   
>> @@ -220,7 +223,7 @@ static int handle_pagefault(struct xe_gt *gt, struct pagefault *pf)
>>   	dma_fence_put(fence);
>>   
>>   	if (xe_vma_is_userptr(vma))
>> -		ret = xe_vma_userptr_check_repin(vma);
>> +		ret = xe_vma_userptr_check_repin(to_userptr_vma(vma));
>>   	vma->usm.tile_invalidated &= ~BIT(tile->id);
>>   
>>   unlock_dma_resv:
>> diff --git a/drivers/gpu/drm/xe/xe_pt.c b/drivers/gpu/drm/xe/xe_pt.c
>> index de1030a47588..e45b37c3f0c2 100644
>> --- a/drivers/gpu/drm/xe/xe_pt.c
>> +++ b/drivers/gpu/drm/xe/xe_pt.c
>> @@ -618,8 +618,8 @@ xe_pt_stage_bind(struct xe_tile *tile, struct xe_vma *vma,
>>   
>>   	if (!xe_vma_is_null(vma)) {
>>   		if (xe_vma_is_userptr(vma))
>> -			xe_res_first_sg(vma->userptr.sg, 0, xe_vma_size(vma),
>> -					&curs);
>> +			xe_res_first_sg(to_userptr_vma(vma)->userptr.sg, 0,
>> +					xe_vma_size(vma), &curs);
>>   		else if (xe_bo_is_vram(bo) || xe_bo_is_stolen(bo))
>>   			xe_res_first(bo->ttm.resource, xe_vma_bo_offset(vma),
>>   				     xe_vma_size(vma), &curs);
>> @@ -906,17 +906,17 @@ static void xe_vm_dbg_print_entries(struct xe_device *xe,
>>   
>>   #ifdef CONFIG_DRM_XE_USERPTR_INVAL_INJECT
>>   
>> -static int xe_pt_userptr_inject_eagain(struct xe_vma *vma)
>> +static int xe_pt_userptr_inject_eagain(struct xe_userptr_vma *uvma)
>>   {
>> -	u32 divisor = vma->userptr.divisor ? vma->userptr.divisor : 2;
>> +	u32 divisor = uvma->userptr.divisor ? uvma->userptr.divisor : 2;
>>   	static u32 count;
>>   
>>   	if (count++ % divisor == divisor - 1) {
>> -		struct xe_vm *vm = xe_vma_vm(vma);
>> +		struct xe_vm *vm = xe_vma_vm(&uvma->vma);
>>   
>> -		vma->userptr.divisor = divisor << 1;
>> +		uvma->userptr.divisor = divisor << 1;
>>   		spin_lock(&vm->userptr.invalidated_lock);
>> -		list_move_tail(&vma->userptr.invalidate_link,
>> +		list_move_tail(&uvma->userptr.invalidate_link,
>>   			       &vm->userptr.invalidated);
>>   		spin_unlock(&vm->userptr.invalidated_lock);
>>   		return true;
>> @@ -927,7 +927,7 @@ static int xe_pt_userptr_inject_eagain(struct xe_vma *vma)
>>   
>>   #else
>>   
>> -static bool xe_pt_userptr_inject_eagain(struct xe_vma *vma)
>> +static bool xe_pt_userptr_inject_eagain(struct xe_userptr_vma *uvma)
>>   {
>>   	return false;
>>   }
>> @@ -1000,9 +1000,9 @@ static int xe_pt_userptr_pre_commit(struct xe_migrate_pt_update *pt_update)
>>   {
>>   	struct xe_pt_migrate_pt_update *userptr_update =
>>   		container_of(pt_update, typeof(*userptr_update), base);
>> -	struct xe_vma *vma = pt_update->vma;
>> -	unsigned long notifier_seq = vma->userptr.notifier_seq;
>> -	struct xe_vm *vm = xe_vma_vm(vma);
>> +	struct xe_userptr_vma *uvma = to_userptr_vma(pt_update->vma);
>> +	unsigned long notifier_seq = uvma->userptr.notifier_seq;
>> +	struct xe_vm *vm = xe_vma_vm(&uvma->vma);
>>   	int err = xe_pt_vm_dependencies(pt_update->job,
>>   					&vm->rftree[pt_update->tile_id],
>>   					pt_update->start,
>> @@ -1023,7 +1023,7 @@ static int xe_pt_userptr_pre_commit(struct xe_migrate_pt_update *pt_update)
>>   	 */
>>   	do {
>>   		down_read(&vm->userptr.notifier_lock);
>> -		if (!mmu_interval_read_retry(&vma->userptr.notifier,
>> +		if (!mmu_interval_read_retry(&uvma->userptr.notifier,
>>   					     notifier_seq))
>>   			break;
>>   
>> @@ -1032,11 +1032,11 @@ static int xe_pt_userptr_pre_commit(struct xe_migrate_pt_update *pt_update)
>>   		if (userptr_update->bind)
>>   			return -EAGAIN;
>>   
>> -		notifier_seq = mmu_interval_read_begin(&vma->userptr.notifier);
>> +		notifier_seq = mmu_interval_read_begin(&uvma->userptr.notifier);
>>   	} while (true);
>>   
>>   	/* Inject errors to test_whether they are handled correctly */
>> -	if (userptr_update->bind && xe_pt_userptr_inject_eagain(vma)) {
>> +	if (userptr_update->bind && xe_pt_userptr_inject_eagain(uvma)) {
>>   		up_read(&vm->userptr.notifier_lock);
>>   		return -EAGAIN;
>>   	}
>> @@ -1297,7 +1297,7 @@ __xe_pt_bind_vma(struct xe_tile *tile, struct xe_vma *vma, struct xe_exec_queue
>>   		vma->tile_present |= BIT(tile->id);
>>   
>>   		if (bind_pt_update.locked) {
>> -			vma->userptr.initial_bind = true;
>> +			to_userptr_vma(vma)->userptr.initial_bind = true;
>>   			up_read(&vm->userptr.notifier_lock);
>>   			xe_bo_put_commit(&deferred);
>>   		}
>> @@ -1642,7 +1642,7 @@ __xe_pt_unbind_vma(struct xe_tile *tile, struct xe_vma *vma, struct xe_exec_queu
>>   
>>   		if (!vma->tile_present) {
>>   			spin_lock(&vm->userptr.invalidated_lock);
>> -			list_del_init(&vma->userptr.invalidate_link);
>> +			list_del_init(&to_userptr_vma(vma)->userptr.invalidate_link);
>>   			spin_unlock(&vm->userptr.invalidated_lock);
>>   		}
>>   		up_read(&vm->userptr.notifier_lock);
>> diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
>> index d096a8c00bd4..66593d841fa9 100644
>> --- a/drivers/gpu/drm/xe/xe_vm.c
>> +++ b/drivers/gpu/drm/xe/xe_vm.c
>> @@ -46,7 +46,7 @@ static struct drm_gem_object *xe_vm_obj(struct xe_vm *vm)
>>   
>>   /**
>>    * xe_vma_userptr_check_repin() - Advisory check for repin needed
>> - * @vma: The userptr vma
>> + * @uvma: The userptr vma
>>    *
>>    * Check if the userptr vma has been invalidated since last successful
>>    * repin. The check is advisory only and can the function can be called
>> @@ -56,15 +56,17 @@ static struct drm_gem_object *xe_vm_obj(struct xe_vm *vm)
>>    *
>>    * Return: 0 if userptr vma is valid, -EAGAIN otherwise; repin recommended.
>>    */
>> -int xe_vma_userptr_check_repin(struct xe_vma *vma)
>> +int xe_vma_userptr_check_repin(struct xe_userptr_vma *uvma)
>>   {
>> -	return mmu_interval_check_retry(&vma->userptr.notifier,
>> -					vma->userptr.notifier_seq) ?
>> +	return mmu_interval_check_retry(&uvma->userptr.notifier,
>> +					uvma->userptr.notifier_seq) ?
>>   		-EAGAIN : 0;
>>   }
>>   
>> -int xe_vma_userptr_pin_pages(struct xe_vma *vma)
>> +int xe_vma_userptr_pin_pages(struct xe_userptr_vma *uvma)
>>   {
>> +	struct xe_userptr *userptr = &uvma->userptr;
>> +	struct xe_vma *vma = &uvma->vma;
>>   	struct xe_vm *vm = xe_vma_vm(vma);
>>   	struct xe_device *xe = vm->xe;
>>   	const unsigned long num_pages = xe_vma_size(vma) >> PAGE_SHIFT;
>> @@ -80,30 +82,30 @@ int xe_vma_userptr_pin_pages(struct xe_vma *vma)
>>   	if (vma->gpuva.flags & XE_VMA_DESTROYED)
>>   		return 0;
>>   
>> -	notifier_seq = mmu_interval_read_begin(&vma->userptr.notifier);
>> -	if (notifier_seq == vma->userptr.notifier_seq)
>> +	notifier_seq = mmu_interval_read_begin(&userptr->notifier);
>> +	if (notifier_seq == userptr->notifier_seq)
>>   		return 0;
>>   
>>   	pages = kvmalloc_array(num_pages, sizeof(*pages), GFP_KERNEL);
>>   	if (!pages)
>>   		return -ENOMEM;
>>   
>> -	if (vma->userptr.sg) {
>> +	if (userptr->sg) {
>>   		dma_unmap_sgtable(xe->drm.dev,
>> -				  vma->userptr.sg,
>> +				  userptr->sg,
>>   				  read_only ? DMA_TO_DEVICE :
>>   				  DMA_BIDIRECTIONAL, 0);
>> -		sg_free_table(vma->userptr.sg);
>> -		vma->userptr.sg = NULL;
>> +		sg_free_table(userptr->sg);
>> +		userptr->sg = NULL;
>>   	}
>>   
>>   	pinned = ret = 0;
>>   	if (in_kthread) {
>> -		if (!mmget_not_zero(vma->userptr.notifier.mm)) {
>> +		if (!mmget_not_zero(userptr->notifier.mm)) {
>>   			ret = -EFAULT;
>>   			goto mm_closed;
>>   		}
>> -		kthread_use_mm(vma->userptr.notifier.mm);
>> +		kthread_use_mm(userptr->notifier.mm);
>>   	}
>>   
>>   	while (pinned < num_pages) {
>> @@ -123,32 +125,32 @@ int xe_vma_userptr_pin_pages(struct xe_vma *vma)
>>   	}
>>   
>>   	if (in_kthread) {
>> -		kthread_unuse_mm(vma->userptr.notifier.mm);
>> -		mmput(vma->userptr.notifier.mm);
>> +		kthread_unuse_mm(userptr->notifier.mm);
>> +		mmput(userptr->notifier.mm);
>>   	}
>>   mm_closed:
>>   	if (ret)
>>   		goto out;
>>   
>> -	ret = sg_alloc_table_from_pages_segment(&vma->userptr.sgt, pages,
>> +	ret = sg_alloc_table_from_pages_segment(&userptr->sgt, pages,
>>   						pinned, 0,
>>   						(u64)pinned << PAGE_SHIFT,
>>   						xe_sg_segment_size(xe->drm.dev),
>>   						GFP_KERNEL);
>>   	if (ret) {
>> -		vma->userptr.sg = NULL;
>> +		userptr->sg = NULL;
>>   		goto out;
>>   	}
>> -	vma->userptr.sg = &vma->userptr.sgt;
>> +	userptr->sg = &userptr->sgt;
>>   
>> -	ret = dma_map_sgtable(xe->drm.dev, vma->userptr.sg,
>> +	ret = dma_map_sgtable(xe->drm.dev, userptr->sg,
>>   			      read_only ? DMA_TO_DEVICE :
>>   			      DMA_BIDIRECTIONAL,
>>   			      DMA_ATTR_SKIP_CPU_SYNC |
>>   			      DMA_ATTR_NO_KERNEL_MAPPING);
>>   	if (ret) {
>> -		sg_free_table(vma->userptr.sg);
>> -		vma->userptr.sg = NULL;
>> +		sg_free_table(userptr->sg);
>> +		userptr->sg = NULL;
>>   		goto out;
>>   	}
>>   
>> @@ -167,8 +169,8 @@ int xe_vma_userptr_pin_pages(struct xe_vma *vma)
>>   	kvfree(pages);
>>   
>>   	if (!(ret < 0)) {
>> -		vma->userptr.notifier_seq = notifier_seq;
>> -		if (xe_vma_userptr_check_repin(vma) == -EAGAIN)
>> +		userptr->notifier_seq = notifier_seq;
>> +		if (xe_vma_userptr_check_repin(uvma) == -EAGAIN)
>>   			goto retry;
>>   	}
>>   
>> @@ -635,7 +637,9 @@ static bool vma_userptr_invalidate(struct mmu_interval_notifier *mni,
>>   				   const struct mmu_notifier_range *range,
>>   				   unsigned long cur_seq)
>>   {
>> -	struct xe_vma *vma = container_of(mni, struct xe_vma, userptr.notifier);
>> +	struct xe_userptr *userptr = container_of(mni, typeof(*userptr), notifier);
>> +	struct xe_userptr_vma *uvma = container_of(userptr, typeof(*uvma), userptr);
>> +	struct xe_vma *vma = &uvma->vma;
>>   	struct xe_vm *vm = xe_vma_vm(vma);
>>   	struct dma_resv_iter cursor;
>>   	struct dma_fence *fence;
>> @@ -651,7 +655,7 @@ static bool vma_userptr_invalidate(struct mmu_interval_notifier *mni,
>>   	mmu_interval_set_seq(mni, cur_seq);
>>   
>>   	/* No need to stop gpu access if the userptr is not yet bound. */
>> -	if (!vma->userptr.initial_bind) {
>> +	if (!userptr->initial_bind) {
>>   		up_write(&vm->userptr.notifier_lock);
>>   		return true;
>>   	}
>> @@ -663,7 +667,7 @@ static bool vma_userptr_invalidate(struct mmu_interval_notifier *mni,
>>   	if (!xe_vm_in_fault_mode(vm) &&
>>   	    !(vma->gpuva.flags & XE_VMA_DESTROYED) && vma->tile_present) {
>>   		spin_lock(&vm->userptr.invalidated_lock);
>> -		list_move_tail(&vma->userptr.invalidate_link,
>> +		list_move_tail(&userptr->invalidate_link,
>>   			       &vm->userptr.invalidated);
>>   		spin_unlock(&vm->userptr.invalidated_lock);
>>   	}
>> @@ -703,7 +707,7 @@ static const struct mmu_interval_notifier_ops vma_userptr_notifier_ops = {
>>   
>>   int xe_vm_userptr_pin(struct xe_vm *vm)
>>   {
>> -	struct xe_vma *vma, *next;
>> +	struct xe_userptr_vma *uvma, *next;
>>   	int err = 0;
>>   	LIST_HEAD(tmp_evict);
>>   
>> @@ -711,22 +715,23 @@ int xe_vm_userptr_pin(struct xe_vm *vm)
>>   
>>   	/* Collect invalidated userptrs */
>>   	spin_lock(&vm->userptr.invalidated_lock);
>> -	list_for_each_entry_safe(vma, next, &vm->userptr.invalidated,
>> +	list_for_each_entry_safe(uvma, next, &vm->userptr.invalidated,
>>   				 userptr.invalidate_link) {
>> -		list_del_init(&vma->userptr.invalidate_link);
>> -		list_move_tail(&vma->combined_links.userptr,
>> +		list_del_init(&uvma->userptr.invalidate_link);
>> +		list_move_tail(&uvma->userptr.repin_link,
>>   			       &vm->userptr.repin_list);
>>   	}
>>   	spin_unlock(&vm->userptr.invalidated_lock);
>>   
>>   	/* Pin and move to temporary list */
>> -	list_for_each_entry_safe(vma, next, &vm->userptr.repin_list,
>> -				 combined_links.userptr) {
>> -		err = xe_vma_userptr_pin_pages(vma);
>> +	list_for_each_entry_safe(uvma, next, &vm->userptr.repin_list,
>> +				 userptr.repin_link) {
>> +		err = xe_vma_userptr_pin_pages(uvma);
>>   		if (err < 0)
>>   			return err;
>>   
>> -		list_move_tail(&vma->combined_links.userptr, &vm->rebind_list);
>> +		list_del_init(&uvma->userptr.repin_link);
>> +		list_move_tail(&uvma->vma.combined_links.rebind, &vm->rebind_list);
>>   	}
>>   
>>   	return 0;
>> @@ -801,10 +806,9 @@ static struct xe_vma *xe_vma_create(struct xe_vm *vm,
>>   	xe_assert(vm->xe, end < vm->size);
>>   
>>   	if (!bo && !is_null)	/* userptr */
>> -		vma = kzalloc(sizeof(*vma), GFP_KERNEL);
>> +		vma = kzalloc(sizeof(struct xe_userptr_vma), GFP_KERNEL);
>>   	else
>> -		vma = kzalloc(sizeof(*vma) - sizeof(struct xe_userptr),
>> -			      GFP_KERNEL);
>> +		vma = kzalloc(sizeof(*vma), GFP_KERNEL);
>>   	if (!vma) {
>>   		vma = ERR_PTR(-ENOMEM);
>>   		return vma;
>> @@ -848,13 +852,15 @@ static struct xe_vma *xe_vma_create(struct xe_vm *vm,
>>   		drm_gpuvm_bo_put(vm_bo);
>>   	} else /* userptr or null */ {
>>   		if (!is_null) {
>> +			struct xe_userptr *userptr = &to_userptr_vma(vma)->userptr;
>>   			u64 size = end - start + 1;
>>   			int err;
>>   
>> -			INIT_LIST_HEAD(&vma->userptr.invalidate_link);
>> +			INIT_LIST_HEAD(&userptr->invalidate_link);
>> +			INIT_LIST_HEAD(&userptr->repin_link);
>>   			vma->gpuva.gem.offset = bo_offset_or_userptr;
>>   
>> -			err = mmu_interval_notifier_insert(&vma->userptr.notifier,
>> +			err = mmu_interval_notifier_insert(&userptr->notifier,
>>   							   current->mm,
>>   							   xe_vma_userptr(vma), size,
>>   							   &vma_userptr_notifier_ops);
>> @@ -864,7 +870,7 @@ static struct xe_vma *xe_vma_create(struct xe_vm *vm,
>>   				return vma;
>>   			}
>>   
>> -			vma->userptr.notifier_seq = LONG_MAX;
>> +			userptr->notifier_seq = LONG_MAX;
>>   		}
>>   
>>   		xe_vm_get(vm);
>> @@ -880,13 +886,15 @@ static void xe_vma_destroy_late(struct xe_vma *vma)
>>   	bool read_only = xe_vma_read_only(vma);
>>   
>>   	if (xe_vma_is_userptr(vma)) {
>> -		if (vma->userptr.sg) {
>> +		struct xe_userptr *userptr = &to_userptr_vma(vma)->userptr;
>> +
>> +		if (userptr->sg) {
>>   			dma_unmap_sgtable(xe->drm.dev,
>> -					  vma->userptr.sg,
>> +					  userptr->sg,
>>   					  read_only ? DMA_TO_DEVICE :
>>   					  DMA_BIDIRECTIONAL, 0);
>> -			sg_free_table(vma->userptr.sg);
>> -			vma->userptr.sg = NULL;
>> +			sg_free_table(userptr->sg);
>> +			userptr->sg = NULL;
>>   		}
>>   
>>   		/*
>> @@ -894,7 +902,7 @@ static void xe_vma_destroy_late(struct xe_vma *vma)
>>   		 * the notifer until we're sure the GPU is not accessing
>>   		 * them anymore
>>   		 */
>> -		mmu_interval_notifier_remove(&vma->userptr.notifier);
>> +		mmu_interval_notifier_remove(&userptr->notifier);
>>   		xe_vm_put(vm);
>>   	} else if (xe_vma_is_null(vma)) {
>>   		xe_vm_put(vm);
>> @@ -933,7 +941,7 @@ static void xe_vma_destroy(struct xe_vma *vma, struct dma_fence *fence)
>>   		xe_assert(vm->xe, vma->gpuva.flags & XE_VMA_DESTROYED);
>>   
>>   		spin_lock(&vm->userptr.invalidated_lock);
>> -		list_del(&vma->userptr.invalidate_link);
>> +		list_del(&to_userptr_vma(vma)->userptr.invalidate_link);
>>   		spin_unlock(&vm->userptr.invalidated_lock);
>>   	} else if (!xe_vma_is_null(vma)) {
>>   		xe_bo_assert_held(xe_vma_bo(vma));
>> @@ -2150,7 +2158,7 @@ static struct xe_vma *new_vma(struct xe_vm *vm, struct drm_gpuva_op_map *op,
>>   		drm_exec_fini(&exec);
>>   
>>   	if (xe_vma_is_userptr(vma)) {
>> -		err = xe_vma_userptr_pin_pages(vma);
>> +		err = xe_vma_userptr_pin_pages(to_userptr_vma(vma));
>>   		if (err) {
>>   			prep_vma_destroy(vm, vma, false);
>>   			xe_vma_destroy_unlocked(vma);
>> @@ -2507,7 +2515,7 @@ static int __xe_vma_op_execute(struct xe_vm *vm, struct xe_vma *vma,
>>   
>>   	if (err == -EAGAIN && xe_vma_is_userptr(vma)) {
>>   		lockdep_assert_held_write(&vm->lock);
>> -		err = xe_vma_userptr_pin_pages(vma);
>> +		err = xe_vma_userptr_pin_pages(to_userptr_vma(vma));
>>   		if (!err)
>>   			goto retry_userptr;
>>   
>> @@ -3130,8 +3138,8 @@ int xe_vm_invalidate_vma(struct xe_vma *vma)
>>   	if (IS_ENABLED(CONFIG_PROVE_LOCKING)) {
>>   		if (xe_vma_is_userptr(vma)) {
>>   			WARN_ON_ONCE(!mmu_interval_check_retry
>> -				     (&vma->userptr.notifier,
>> -				      vma->userptr.notifier_seq));
>> +				     (&to_userptr_vma(vma)->userptr.notifier,
>> +				      to_userptr_vma(vma)->userptr.notifier_seq));
>>   			WARN_ON_ONCE(!dma_resv_test_signaled(xe_vm_resv(xe_vma_vm(vma)),
>>   							     DMA_RESV_USAGE_BOOKKEEP));
>>   
>> @@ -3192,11 +3200,11 @@ int xe_analyze_vm(struct drm_printer *p, struct xe_vm *vm, int gt_id)
>>   		if (is_null) {
>>   			addr = 0;
>>   		} else if (is_userptr) {
>> +			struct sg_table *sg = to_userptr_vma(vma)->userptr.sg;
>>   			struct xe_res_cursor cur;
>>   
>> -			if (vma->userptr.sg) {
>> -				xe_res_first_sg(vma->userptr.sg, 0, XE_PAGE_SIZE,
>> -						&cur);
>> +			if (sg) {
>> +				xe_res_first_sg(sg, 0, XE_PAGE_SIZE, &cur);
>>   				addr = xe_res_dma(&cur);
>>   			} else {
>>   				addr = 0;
>> diff --git a/drivers/gpu/drm/xe/xe_vm.h b/drivers/gpu/drm/xe/xe_vm.h
>> index e9c907cbcd89..c1884f974a72 100644
>> --- a/drivers/gpu/drm/xe/xe_vm.h
>> +++ b/drivers/gpu/drm/xe/xe_vm.h
>> @@ -160,6 +160,18 @@ static inline bool xe_vma_is_userptr(struct xe_vma *vma)
>>   	return xe_vma_has_no_bo(vma) && !xe_vma_is_null(vma);
>>   }
>>   
>> +/**
>> + * xe_userptr_vma() - Return a pointer to an embedding userptr vma
> s/xe_userptr_vma/to_userptr_vma
>
> Other than this, LGTM. With that:
> Reviewed-by: Matthew Brost <matthew.brost at intel.com>
>
>> + * @vma: Pointer to the embedded struct xe_vma
>> + *
>> + * Return: Pointer to the embedding userptr vma
>> + */
>> +static inline struct xe_userptr_vma *to_userptr_vma(struct xe_vma *vma)
>> +{
>> +	xe_assert(xe_vma_vm(vma)->xe, xe_vma_is_userptr(vma));
>> +	return container_of(vma, struct xe_userptr_vma, vma);
>> +}
>> +
>>   u64 xe_vm_pdp4_descriptor(struct xe_vm *vm, struct xe_tile *tile);
>>   
>>   int xe_vm_create_ioctl(struct drm_device *dev, void *data,
>> @@ -222,9 +234,9 @@ static inline void xe_vm_reactivate_rebind(struct xe_vm *vm)
>>   	}
>>   }
>>   
>> -int xe_vma_userptr_pin_pages(struct xe_vma *vma);
>> +int xe_vma_userptr_pin_pages(struct xe_userptr_vma *uvma);
>>   
>> -int xe_vma_userptr_check_repin(struct xe_vma *vma);
>> +int xe_vma_userptr_check_repin(struct xe_userptr_vma *uvma);
>>   
>>   bool xe_vm_validate_should_retry(struct drm_exec *exec, int err, ktime_t *end);
>>   
>> diff --git a/drivers/gpu/drm/xe/xe_vm_types.h b/drivers/gpu/drm/xe/xe_vm_types.h
>> index 63e8a50b88e9..1fec66ae2eb2 100644
>> --- a/drivers/gpu/drm/xe/xe_vm_types.h
>> +++ b/drivers/gpu/drm/xe/xe_vm_types.h
>> @@ -37,6 +37,8 @@ struct xe_vm;
>>   struct xe_userptr {
>>   	/** @invalidate_link: Link for the vm::userptr.invalidated list */
>>   	struct list_head invalidate_link;
>> +	/** @userptr: link into VM repin list if userptr. */
>> +	struct list_head repin_link;
>>   	/**
>>   	 * @notifier: MMU notifier for user pointer (invalidation call back)
>>   	 */
>> @@ -68,8 +70,6 @@ struct xe_vma {
>>   	 * resv.
>>   	 */
>>   	union {
>> -		/** @userptr: link into VM repin list if userptr. */
>> -		struct list_head userptr;
>>   		/** @rebind: link into VM if this VMA needs rebinding. */
>>   		struct list_head rebind;
>>   		/** @destroy: link to contested list when VM is being closed. */
>> @@ -105,11 +105,15 @@ struct xe_vma {
>>   	 * @pat_index: The pat index to use when encoding the PTEs for this vma.
>>   	 */
>>   	u16 pat_index;
>> +};
>>   
>> -	/**
>> -	 * @userptr: user pointer state, only allocated for VMAs that are
>> -	 * user pointers
>> -	 */
>> +/**
>> + * struct xe_userptr_vma - A userptr vma subclass
>> + * @vma: The vma.
>> + * @userptr: Additional userptr information.
>> + */
>> +struct xe_userptr_vma {
>> +	struct xe_vma vma;
>>   	struct xe_userptr userptr;
>>   };
>>   
>> -- 
>> 2.43.0
>>


More information about the Intel-xe mailing list