[PATCH 4/4] drm/xe/uapi: Support pinning of userptr vmas

Thomas Hellström thomas.hellstrom at linux.intel.com
Tue Aug 22 08:25:24 UTC 2023


On 8/20/23 05:54, Matthew Brost wrote:
> On Fri, Aug 18, 2023 at 05:08:45PM +0200, Thomas Hellström wrote:
>> Support pinning of vmas using XE_VM_BIND_FLAG_PIN, initially for userptr
>> only. Pinned memory becomes accounted against RLIMIT_MEMLOCK and processes
>> with CAP_IPC_LOCK will not apply the limit. This is pretty similar to
>> mlock()'ing userptr memory with the added benefit that the driver is
>> aware and can ignore some actions in the MMU invalidation notifier.
>>
>> This will initially become useful for compute VMs on hardware without
>> mid-thread-preemption capability since with pinned pages, the MMU
>> invalidation notifier never tries to preempt a running compute kernel.
>>
>> If that were the only usage we could restrict this to a flag that always
>> pins userptr VMAs on compute VMs on such hardware, but there are
>> indications that this may become needed in other situations as well.
>>
>>  From a more general point of view, the usage pattern of a system may be
>> such that in most cases it only ever runs a single workload per system
>> and then the sysadmin would want to configure the system to allow
>> extensive pinning for performance reasons.
>>
>> Hence we might want to extend the pinning capability to bo-backed VMAs
>> as well. How that pinning will be accounted remains an open but to build
>> on the current drm CGROUP work would be an option.
>>
>> Signed-off-by: Thomas Hellström <thomas.hellstrom at linux.intel.com>
> Patch LGTM but a few comments that are currently out of scope but want
> to get out there for future work.
>
>> ---
>>   drivers/gpu/drm/xe/xe_vm.c       | 33 +++++++++++++++++++++++++-------
>>   drivers/gpu/drm/xe/xe_vm_types.h |  2 ++
>>   include/uapi/drm/xe_drm.h        | 18 +++++++++++++++++
>>   3 files changed, 46 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
>> index d9c000689002..3832f1f21def 100644
>> --- a/drivers/gpu/drm/xe/xe_vm.c
>> +++ b/drivers/gpu/drm/xe/xe_vm.c
>> @@ -936,6 +936,7 @@ static struct xe_vma *xe_vma_create(struct xe_vm *vm,
>>   				    u64 start, u64 end,
>>   				    bool read_only,
>>   				    bool is_null,
>> +				    bool pin,
>>   				    u8 tile_mask)
>>   {
>>   	struct xe_vma *vma;
>> @@ -967,6 +968,8 @@ static struct xe_vma *xe_vma_create(struct xe_vm *vm,
>>   		vma->gpuva.flags |= XE_VMA_READ_ONLY;
>>   	if (is_null)
>>   		vma->gpuva.flags |= DRM_GPUVA_SPARSE;
>> +	if (pin)
>> +		vma->gpuva.flags |= XE_VMA_PINNED;
>>   
>>   	if (tile_mask) {
>>   		vma->tile_mask = tile_mask;
>> @@ -2367,6 +2370,7 @@ vm_bind_ioctl_ops_create(struct xe_vm *vm, struct xe_bo *bo,
>>   			op->map.read_only =
>>   				operation & XE_VM_BIND_FLAG_READONLY;
>>   			op->map.is_null = operation & XE_VM_BIND_FLAG_NULL;
>> +			op->map.pin = operation & XE_VM_BIND_FLAG_PIN;
>>   		}
>>   		break;
>>   	case XE_VM_BIND_OP_UNMAP:
>> @@ -2431,7 +2435,8 @@ vm_bind_ioctl_ops_create(struct xe_vm *vm, struct xe_bo *bo,
>>   }
>>   
>>   static struct xe_vma *new_vma(struct xe_vm *vm, struct drm_gpuva_op_map *op,
>> -			      u8 tile_mask, bool read_only, bool is_null)
>> +			      u8 tile_mask, bool read_only, bool is_null,
>> +			      bool pin)
>>   {
>>   	struct xe_bo *bo = op->gem.obj ? gem_to_xe_bo(op->gem.obj) : NULL;
>>   	struct xe_vma *vma;
>> @@ -2447,7 +2452,7 @@ static struct xe_vma *new_vma(struct xe_vm *vm, struct drm_gpuva_op_map *op,
>>   	}
>>   	vma = xe_vma_create(vm, bo, op->gem.offset,
>>   			    op->va.addr, op->va.addr +
>> -			    op->va.range - 1, read_only, is_null,
>> +			    op->va.range - 1, read_only, is_null, pin,
>>   			    tile_mask);
>>   	if (bo)
>>   		xe_bo_unlock(bo, &ww);
>> @@ -2562,7 +2567,7 @@ static int vm_bind_ioctl_ops_parse(struct xe_vm *vm, struct xe_exec_queue *q,
>>   
>>   				vma = new_vma(vm, &op->base.map,
>>   					      op->tile_mask, op->map.read_only,
>> -					      op->map.is_null);
>> +					      op->map.is_null, op->map.pin);
>>   				if (IS_ERR(vma)) {
>>   					err = PTR_ERR(vma);
>>   					goto free_fence;
>> @@ -2587,10 +2592,13 @@ static int vm_bind_ioctl_ops_parse(struct xe_vm *vm, struct xe_exec_queue *q,
>>   					bool is_null =
>>   						op->base.remap.unmap->va->flags &
>>   						DRM_GPUVA_SPARSE;
>> +					bool pin =
>> +						op->base.remap.unmap->va->flags &
>> +						XE_VMA_PINNED;
> We probably should move the read_only, is_null, and pin check out of the
> next / prev if statements to just below the DRM_GPUVA_OP_REMAP case
> statement.
>
>>   
>>   					vma = new_vma(vm, op->base.remap.prev,
>>   						      op->tile_mask, read_only,
>> -						      is_null);
>> +						      is_null, pin);
>>   					if (IS_ERR(vma)) {
>>   						err = PTR_ERR(vma);
>>   						goto free_fence;
>> @@ -2623,10 +2631,13 @@ static int vm_bind_ioctl_ops_parse(struct xe_vm *vm, struct xe_exec_queue *q,
>>   					bool is_null =
>>   						op->base.remap.unmap->va->flags &
>>   						DRM_GPUVA_SPARSE;
>> +					bool pin =
>> +						op->base.remap.unmap->va->flags &
>> +						XE_VMA_PINNED;
>>   
>>   					vma = new_vma(vm, op->base.remap.next,
>>   						      op->tile_mask, read_only,
>> -						      is_null);
>> +						      is_null, pin);
>>   					if (IS_ERR(vma)) {
>>   						err = PTR_ERR(vma);
>>   						goto free_fence;
>> @@ -3131,11 +3142,12 @@ static void vm_bind_ioctl_ops_unwind(struct xe_vm *vm,
>>   #define SUPPORTED_FLAGS	\
>>   	(FORCE_ASYNC_OP_ERROR | XE_VM_BIND_FLAG_ASYNC | \
>>   	 XE_VM_BIND_FLAG_READONLY | XE_VM_BIND_FLAG_IMMEDIATE | \
>> -	 XE_VM_BIND_FLAG_NULL | 0xffff)
>> +	 XE_VM_BIND_FLAG_NULL | XE_VM_BIND_FLAG_PIN | 0xffff)
>>   #else
>>   #define SUPPORTED_FLAGS	\
>>   	(XE_VM_BIND_FLAG_ASYNC | XE_VM_BIND_FLAG_READONLY | \
>> -	 XE_VM_BIND_FLAG_IMMEDIATE | XE_VM_BIND_FLAG_NULL | 0xffff)
>> +	 XE_VM_BIND_FLAG_IMMEDIATE | XE_VM_BIND_FLAG_NULL | \
>> +	 XE_VM_BIND_FLAG_PIN | 0xffff)
>>   #endif
>>   #define XE_64K_PAGE_MASK 0xffffull
>>   
>> @@ -3205,6 +3217,13 @@ static int vm_bind_ioctl_check_args(struct xe_device *xe,
>>   			goto free_bind_ops;
>>   		}
>>   
>> +		/* TODO: Support OP_PREFETCH, OP_MAP */
>> +		if (XE_IOCTL_DBG(xe, (op & XE_VM_BIND_FLAG_PIN) &&
>> +				 VM_BIND_OP(op) != XE_VM_BIND_OP_MAP_USERPTR)) {
>> +			err = -EINVAL;
>> +			goto free_bind_ops;
>> +		}
>> +
>>   		if (XE_IOCTL_DBG(xe, VM_BIND_OP(op) >
>>   				 XE_VM_BIND_OP_PREFETCH) ||
>>   		    XE_IOCTL_DBG(xe, op & ~SUPPORTED_FLAGS) ||
>> diff --git a/drivers/gpu/drm/xe/xe_vm_types.h b/drivers/gpu/drm/xe/xe_vm_types.h
>> index 9b90e649cd69..024ccabadd12 100644
>> --- a/drivers/gpu/drm/xe/xe_vm_types.h
>> +++ b/drivers/gpu/drm/xe/xe_vm_types.h
>> @@ -360,6 +360,8 @@ struct xe_vma_op_map {
>>   	bool read_only;
>>   	/** @is_null: is NULL binding */
>>   	bool is_null;
>> +	/** @pin: pin underlying memory */
>> +	bool pin;
>>   };
>>   
>>   /** struct xe_vma_op_remap - VMA remap operation */
>> diff --git a/include/uapi/drm/xe_drm.h b/include/uapi/drm/xe_drm.h
>> index 86f16d50e9cc..fc3d9cd4f8d0 100644
>> --- a/include/uapi/drm/xe_drm.h
>> +++ b/include/uapi/drm/xe_drm.h
>> @@ -631,6 +631,24 @@ struct drm_xe_vm_bind_op {
>>   	 * intended to implement VK sparse bindings.
>>   	 */
>>   #define XE_VM_BIND_FLAG_NULL		(0x1 << 19)
>> +	 /*
>> +	  * When the PIN flag is set, the user requests the underlying
>> +	  * backing store of the vma to be pinned, that is, it will be
>> +	  * resident while bound and the underlying physical memory
>> +	  * will not change. For userptr VMAs this means that if the
>> +	  * user performs an operation that changes the underlying
>> +	  * pages of the CPU virtual space, the corresponding pinned
>> +	  * GPU virtual space will not pick up the new memory unless
>> +	  * an OP_UNMAP followed by a OP_MAP_USERPTR is performed.
>> +	  * Pinned userptr memory is accounted in the same way as
>> +	  * mlock(2), and if pinning fails the following error codes
>> +	  * may be returned:
>> +	  * -EINVAL: The memory region does not support pinning.
>> +	  * -EPERM: The process is not permitted to pin.
>> +	  * -ENOMEM: The pinning limit does not allow pinning.
>> +	  * For userptr memory, CAP_IPC_LOCK will bypass the limit checking.
>> +	  */
>> +#define XE_VM_BIND_FLAG_PIN		(0x1 << 20)
> We are quickly using a lot of the upper bits, maybe we change the op
> field to a __u64 soon? We have to break the VM bind api when removing
> the async worker + updating sync mode to align with VM bind doc, maybe
> we change this then too?

What about a separate flags field?

/Thomas


>
> Anyways this patch LGTM:
> Reviewed-by: Matthew Brost <matthew.brost at intel.com>
>
>>   	/** @op: Operation to perform (lower 16 bits) and flags (upper 16 bits) */
>>   	__u32 op;
>>   
>> -- 
>> 2.41.0
>>


More information about the dri-devel mailing list