[PATCH v2] drm/xe: Use resevered copy engine for user binds on faulting devices
Cavitt, Jonathan
jonathan.cavitt at intel.com
Thu Aug 15 22:11:16 UTC 2024
-----Original Message-----
From: Intel-xe <intel-xe-bounces at lists.freedesktop.org> On Behalf Of Matthew Brost
Sent: Thursday, August 15, 2024 1:42 PM
To: intel-xe at lists.freedesktop.org
Cc: thomas.hellstrom at linux.intel.com
Subject: [PATCH v2] drm/xe: Use resevered copy engine for user binds on faulting devices
>
> User binds map to engines with can fault, faults depend on user binds
> completion, thus we can deadlock. Avoid this by using resevered copy
> engine for user binds on faulting devices.
>
> While we are here, normalize bind queue creation with a helper.
>
> v2:
> - Pass in extensions to bind queue creation (CI)
>
> Fixes: dd08ebf6c352 ("drm/xe: Introduce a new DRM driver for Intel GPUs")
> Cc: Thomas Hellström <thomas.hellstrom at linux.intel.com>
> Signed-off-by: Matthew Brost <matthew.brost at intel.com>
> ---
> drivers/gpu/drm/xe/xe_exec_queue.c | 122 +++++++++++++++--------------
> drivers/gpu/drm/xe/xe_exec_queue.h | 6 +-
> drivers/gpu/drm/xe/xe_migrate.c | 2 +-
> drivers/gpu/drm/xe/xe_vm.c | 8 +-
> 4 files changed, 70 insertions(+), 68 deletions(-)
>
> diff --git a/drivers/gpu/drm/xe/xe_exec_queue.c b/drivers/gpu/drm/xe/xe_exec_queue.c
> index 971e1234b8ea..a3a5685a4d57 100644
> --- a/drivers/gpu/drm/xe/xe_exec_queue.c
> +++ b/drivers/gpu/drm/xe/xe_exec_queue.c
> @@ -166,7 +166,8 @@ struct xe_exec_queue *xe_exec_queue_create(struct xe_device *xe, struct xe_vm *v
>
> struct xe_exec_queue *xe_exec_queue_create_class(struct xe_device *xe, struct xe_gt *gt,
> struct xe_vm *vm,
> - enum xe_engine_class class, u32 flags)
> + enum xe_engine_class class,
> + u32 flags, u64 extensions)
> {
> struct xe_hw_engine *hwe, *hwe0 = NULL;
> enum xe_hw_engine_id id;
> @@ -186,7 +187,54 @@ struct xe_exec_queue *xe_exec_queue_create_class(struct xe_device *xe, struct xe
> if (!logical_mask)
> return ERR_PTR(-ENODEV);
>
> - return xe_exec_queue_create(xe, vm, logical_mask, 1, hwe0, flags, 0);
> + return xe_exec_queue_create(xe, vm, logical_mask, 1, hwe0, flags, extensions);
> +}
> +
> +/**
> + * xe_exec_queue_create_bind() - Create bind exec queue.
> + * @xe: Xe device.
> + * @tile: tile which bind exec queue belongs to.
> + * @flags: exec queue creation flags
> + * @extensions: exec queue creation extensions
> + *
> + * Normalize bind exec queue creation. Bind exec queue is tied to migration VM
> + * for access to physical memory required for page table programming. On a
> + * faulting devices the reserved copy engine instance must be used to avoid
> + * deadlocking (user binds cannot get stuck behind faults as kernel binds which
> + * resolve faults depend on user binds). On non-faulting devices any copy engine
> + * can be used.
> + *
> + * Returns exec queue on success, ERR_PTR on failure
> + */
> +struct xe_exec_queue *xe_exec_queue_create_bind(struct xe_device *xe,
> + struct xe_tile *tile,
> + u32 flags, u64 extensions)
> +{
> + struct xe_gt *gt = tile->primary_gt;
> + struct xe_exec_queue *q;
> + struct xe_vm *migrate_vm;
> +
> + migrate_vm = xe_migrate_get_vm(tile->migrate);
> + if (xe->info.has_usm) {
> + struct xe_hw_engine *hwe = xe_gt_hw_engine(gt,
> + XE_ENGINE_CLASS_COPY,
> + gt->usm.reserved_bcs_instance,
> + false);
> + u32 logical_mask = BIT(hwe->logical_instance);
> +
> + if (!hwe || !logical_mask)
If we're concerned that hwe might be NULL, shouldn't we check that earlier to
prevent a potential null pointer dereference above? Specifically:
"""
struct xe_hw_engine *hwe = xe_gt_hw_engine(gt,
XE_ENGINE_CLASS_COPY,
gt->usm.reserved_bcs_instance,
false);
u32 logical_mask = 0;
if (hwe)
logical_mask = BIT(hwe->logical_instance);
if (!hwe || !logical_mask)
return ERR_PTR(-EINVAL);
"""
Everything else looks fine, though:
Reviewed-by: Jonathan Cavitt <jonathan.cavitt at intel.com>
-Jonathan Cavitt
> + return ERR_PTR(-EINVAL);
> +
> + q = xe_exec_queue_create(xe, migrate_vm, logical_mask, 1, hwe,
> + flags, extensions);
> + } else {
> + q = xe_exec_queue_create_class(xe, gt, migrate_vm,
> + XE_ENGINE_CLASS_COPY, flags,
> + extensions);
> + }
> + xe_vm_put(migrate_vm);
> +
> + return q;
> }
>
> void xe_exec_queue_destroy(struct kref *ref)
> @@ -418,34 +466,6 @@ static int exec_queue_user_extensions(struct xe_device *xe, struct xe_exec_queue
> return 0;
> }
>
> -static u32 bind_exec_queue_logical_mask(struct xe_device *xe, struct xe_gt *gt,
> - struct drm_xe_engine_class_instance *eci,
> - u16 width, u16 num_placements)
> -{
> - struct xe_hw_engine *hwe;
> - enum xe_hw_engine_id id;
> - u32 logical_mask = 0;
> -
> - if (XE_IOCTL_DBG(xe, width != 1))
> - return 0;
> - if (XE_IOCTL_DBG(xe, num_placements != 1))
> - return 0;
> - if (XE_IOCTL_DBG(xe, eci[0].engine_instance != 0))
> - return 0;
> -
> - eci[0].engine_class = DRM_XE_ENGINE_CLASS_COPY;
> -
> - for_each_hw_engine(hwe, gt, id) {
> - if (xe_hw_engine_is_reserved(hwe))
> - continue;
> -
> - if (hwe->class == XE_ENGINE_CLASS_COPY)
> - logical_mask |= BIT(hwe->logical_instance);
> - }
> -
> - return logical_mask;
> -}
> -
> static u32 calc_validate_logical_mask(struct xe_device *xe, struct xe_gt *gt,
> struct drm_xe_engine_class_instance *eci,
> u16 width, u16 num_placements)
> @@ -507,8 +527,9 @@ int xe_exec_queue_create_ioctl(struct drm_device *dev, void *data,
> struct drm_xe_engine_class_instance __user *user_eci =
> u64_to_user_ptr(args->instances);
> struct xe_hw_engine *hwe;
> - struct xe_vm *vm, *migrate_vm;
> + struct xe_vm *vm;
> struct xe_gt *gt;
> + struct xe_tile *tile;
> struct xe_exec_queue *q = NULL;
> u32 logical_mask;
> u32 id;
> @@ -533,37 +554,20 @@ int xe_exec_queue_create_ioctl(struct drm_device *dev, void *data,
> return -EINVAL;
>
> if (eci[0].engine_class == DRM_XE_ENGINE_CLASS_VM_BIND) {
> - for_each_gt(gt, xe, id) {
> - struct xe_exec_queue *new;
> - u32 flags;
> -
> - if (xe_gt_is_media_type(gt))
> - continue;
> -
> - eci[0].gt_id = gt->info.id;
> - logical_mask = bind_exec_queue_logical_mask(xe, gt, eci,
> - args->width,
> - args->num_placements);
> - if (XE_IOCTL_DBG(xe, !logical_mask))
> - return -EINVAL;
> -
> - hwe = xe_hw_engine_lookup(xe, eci[0]);
> - if (XE_IOCTL_DBG(xe, !hwe))
> - return -EINVAL;
> -
> - /* The migration vm doesn't hold rpm ref */
> - xe_pm_runtime_get_noresume(xe);
> -
> - flags = EXEC_QUEUE_FLAG_VM | (id ? EXEC_QUEUE_FLAG_BIND_ENGINE_CHILD : 0);
> + if (XE_IOCTL_DBG(xe, args->width != 1) ||
> + XE_IOCTL_DBG(xe, args->num_placements != 1) ||
> + XE_IOCTL_DBG(xe, eci[0].engine_instance != 0))
> + return -EINVAL;
>
> - migrate_vm = xe_migrate_get_vm(gt_to_tile(gt)->migrate);
> - new = xe_exec_queue_create(xe, migrate_vm, logical_mask,
> - args->width, hwe, flags,
> - args->extensions);
> + for_each_tile(tile, xe, id) {
> + struct xe_exec_queue *new;
> + u32 flags = EXEC_QUEUE_FLAG_VM;
>
> - xe_pm_runtime_put(xe); /* now held by engine */
> + if (id)
> + flags |= EXEC_QUEUE_FLAG_BIND_ENGINE_CHILD;
>
> - xe_vm_put(migrate_vm);
> + new = xe_exec_queue_create_bind(xe, tile, flags,
> + args->extensions);
> if (IS_ERR(new)) {
> err = PTR_ERR(new);
> if (q)
> diff --git a/drivers/gpu/drm/xe/xe_exec_queue.h b/drivers/gpu/drm/xe/xe_exec_queue.h
> index ded77b0f3b90..99139368ba6e 100644
> --- a/drivers/gpu/drm/xe/xe_exec_queue.h
> +++ b/drivers/gpu/drm/xe/xe_exec_queue.h
> @@ -20,7 +20,11 @@ struct xe_exec_queue *xe_exec_queue_create(struct xe_device *xe, struct xe_vm *v
> u64 extensions);
> struct xe_exec_queue *xe_exec_queue_create_class(struct xe_device *xe, struct xe_gt *gt,
> struct xe_vm *vm,
> - enum xe_engine_class class, u32 flags);
> + enum xe_engine_class class,
> + u32 flags, u64 extensions);
> +struct xe_exec_queue *xe_exec_queue_create_bind(struct xe_device *xe,
> + struct xe_tile *tile,
> + u32 flags, u64 extensions);
>
> void xe_exec_queue_fini(struct xe_exec_queue *q);
> void xe_exec_queue_destroy(struct kref *ref);
> diff --git a/drivers/gpu/drm/xe/xe_migrate.c b/drivers/gpu/drm/xe/xe_migrate.c
> index a2d0ce3c59bf..cbf54be224c9 100644
> --- a/drivers/gpu/drm/xe/xe_migrate.c
> +++ b/drivers/gpu/drm/xe/xe_migrate.c
> @@ -442,7 +442,7 @@ struct xe_migrate *xe_migrate_init(struct xe_tile *tile)
> m->q = xe_exec_queue_create_class(xe, primary_gt, vm,
> XE_ENGINE_CLASS_COPY,
> EXEC_QUEUE_FLAG_KERNEL |
> - EXEC_QUEUE_FLAG_PERMANENT);
> + EXEC_QUEUE_FLAG_PERMANENT, 0);
> }
> if (IS_ERR(m->q)) {
> xe_vm_close_and_put(vm);
> diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
> index 22add5167564..7efabdd16abd 100644
> --- a/drivers/gpu/drm/xe/xe_vm.c
> +++ b/drivers/gpu/drm/xe/xe_vm.c
> @@ -1478,19 +1478,13 @@ struct xe_vm *xe_vm_create(struct xe_device *xe, u32 flags)
> /* Kernel migration VM shouldn't have a circular loop.. */
> if (!(flags & XE_VM_FLAG_MIGRATION)) {
> for_each_tile(tile, xe, id) {
> - struct xe_gt *gt = tile->primary_gt;
> - struct xe_vm *migrate_vm;
> struct xe_exec_queue *q;
> u32 create_flags = EXEC_QUEUE_FLAG_VM;
>
> if (!vm->pt_root[id])
> continue;
>
> - migrate_vm = xe_migrate_get_vm(tile->migrate);
> - q = xe_exec_queue_create_class(xe, gt, migrate_vm,
> - XE_ENGINE_CLASS_COPY,
> - create_flags);
> - xe_vm_put(migrate_vm);
> + q = xe_exec_queue_create_bind(xe, tile, create_flags, 0);
> if (IS_ERR(q)) {
> err = PTR_ERR(q);
> goto err_close;
> --
> 2.34.1
>
>
More information about the Intel-xe
mailing list