* [Intel-xe] [PATCH] drm/xe: Enable scratch page when page fault is enabled
@ 2023-08-26 0:14 Chang, Bruce
2023-08-26 0:16 ` [Intel-xe] ✗ CI.Patch_applied: failure for drm/xe: Enable scratch page when page fault is enabled (rev2) Patchwork
` (2 more replies)
0 siblings, 3 replies; 18+ messages in thread
From: Chang, Bruce @ 2023-08-26 0:14 UTC (permalink / raw)
To: intel-xe; +Cc: Stuart Summers
i915 can use scratch page even when page fault is enabled, this patch
is trying to port this feature over.
The current i915 solution changes page table directly which may be hard
to make to upstream, so a more complex solution is needed to apply to the
current Xe framework if following the existing i915 solution.
This patch is trying to make the minimum impact to the existing driver,
but still enable the scratch page support.
So, the idea is to bind a scratch vma if the page fault is from an
invalid access. This patch is taking advantage of null pte at this
point, we may introduce a special vma for scratch vma if needed.
After the bind, the user app can continue to run without causing a
fatal failure or reset and stop.
In case the app will bind this scratch vma to a valid address, it will
fail the bind, this patch will handle the failre and unbind the scrach
vma[s], so that the user binding will be retried to the valid address.
This patch only kicks in when there is a failure for both page fault
and bind, so it should has no impact to the exiating code path. On
another hand, it uses actual page tables instead of special scratch
page tables, so it enables potential not to invalidate TLBs when doing
unbind if all upper layer page tables are still being used.
tested on new scratch igt tests which will be sent out for review.
Cc: Oak Zeng <oak.zeng@intel.com>
Cc: Brian Welty <brian.welty@intel.com>
Cc: Niranjana Vishwanathapura <niranjana.vishwanathapura@intel.com>
Cc: Stuart Summers <stuart.summers@intel.com>
Cc: Matthew Brost <matthew.brost@intel.com>
---
drivers/gpu/drm/xe/xe_gt_pagefault.c | 9 ++++--
drivers/gpu/drm/xe/xe_vm.c | 48 +++++++++++++++++++++++-----
drivers/gpu/drm/xe/xe_vm.h | 2 ++
3 files changed, 49 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/xe/xe_gt_pagefault.c b/drivers/gpu/drm/xe/xe_gt_pagefault.c
index b6f781b3d9d7..524b38df3d7a 100644
--- a/drivers/gpu/drm/xe/xe_gt_pagefault.c
+++ b/drivers/gpu/drm/xe/xe_gt_pagefault.c
@@ -137,8 +137,13 @@ static int handle_pagefault(struct xe_gt *gt, struct pagefault *pf)
write_locked = true;
vma = lookup_vma(vm, pf->page_addr);
if (!vma) {
- ret = -EINVAL;
- goto unlock_vm;
+ if (vm->flags & XE_VM_FLAG_SCRATCH_PAGE)
+ vma = xe_bind_scratch_vma(vm, pf->page_addr, SZ_64K);
+
+ if (!vma) {
+ ret = -EINVAL;
+ goto unlock_vm;
+ }
}
if (!xe_vma_is_userptr(vma) || !xe_vma_userptr_check_repin(vma)) {
diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
index 389ac5ba8ddf..4c3d5d781b58 100644
--- a/drivers/gpu/drm/xe/xe_vm.c
+++ b/drivers/gpu/drm/xe/xe_vm.c
@@ -1262,7 +1262,8 @@ struct xe_vm *xe_vm_create(struct xe_device *xe, u32 flags)
}
}
- if (flags & XE_VM_FLAG_SCRATCH_PAGE) {
+ if (flags & XE_VM_FLAG_SCRATCH_PAGE &&
+ (!(flags & XE_VM_FLAG_FAULT_MODE))) {
for_each_tile(tile, xe, id) {
if (!vm->pt_root[id])
continue;
@@ -1998,10 +1999,6 @@ int xe_vm_create_ioctl(struct drm_device *dev, void *data,
if (XE_IOCTL_DBG(xe, args->flags & ~ALL_DRM_XE_VM_CREATE_FLAGS))
return -EINVAL;
- if (XE_IOCTL_DBG(xe, args->flags & DRM_XE_VM_CREATE_SCRATCH_PAGE &&
- args->flags & DRM_XE_VM_CREATE_FAULT_MODE))
- return -EINVAL;
-
if (XE_IOCTL_DBG(xe, args->flags & DRM_XE_VM_CREATE_COMPUTE_MODE &&
args->flags & DRM_XE_VM_CREATE_FAULT_MODE))
return -EINVAL;
@@ -2783,6 +2780,39 @@ static int __xe_vma_op_execute(struct xe_vm *vm, struct xe_vma *vma,
return err;
}
+struct xe_vma *xe_bind_scratch_vma(struct xe_vm *vm, u64 addr, u64 size)
+{
+ struct xe_vma *vma = 0;
+
+ if (!vm->size)
+ return 0;
+
+ vma = xe_vma_create(vm, NULL, 0, addr, addr + size - 1, false, true, 0);
+ if (!vma)
+ return 0;
+ xe_vm_insert_vma(vm, vma);
+
+ /* fault will handle the bind */
+
+ return vma;
+}
+
+int xe_unbind_scratch_vma(struct xe_vm *vm, u64 addr, u64 range)
+{
+ struct xe_vma *vma;
+
+ vma = xe_vm_find_overlapping_vma(vm, addr, range);
+ if (!vma)
+ return -ENXIO;
+
+ if (xe_vma_is_null(vma)) {
+ prep_vma_destroy(vm, vma, true);
+ xe_vm_unbind(vm, vma, NULL, NULL, 0, NULL, true, false);
+ }
+
+ return 0;
+}
+
static int xe_vma_op_execute(struct xe_vm *vm, struct xe_vma_op *op)
{
int ret = 0;
@@ -3205,7 +3235,6 @@ int xe_vm_bind_ioctl(struct drm_device *dev, void *data, struct drm_file *file)
err = vm_bind_ioctl_check_args(xe, args, &bind_ops, &async);
if (err)
return err;
-
if (args->exec_queue_id) {
q = xe_exec_queue_lookup(xef, args->exec_queue_id);
if (XE_IOCTL_DBG(xe, !q)) {
@@ -3352,10 +3381,13 @@ int xe_vm_bind_ioctl(struct drm_device *dev, void *data, struct drm_file *file)
u64 range = bind_ops[i].range;
u64 addr = bind_ops[i].addr;
u32 op = bind_ops[i].op;
-
+retry:
err = vm_bind_ioctl_lookup_vma(vm, bos[i], addr, range, op);
- if (err)
+ if (err) {
+ if (!xe_unbind_scratch_vma(vm, addr, range))
+ goto retry;
goto free_syncs;
+ }
}
for (i = 0; i < args->num_binds; ++i) {
diff --git a/drivers/gpu/drm/xe/xe_vm.h b/drivers/gpu/drm/xe/xe_vm.h
index 6de6e3edb24a..6447bed427b1 100644
--- a/drivers/gpu/drm/xe/xe_vm.h
+++ b/drivers/gpu/drm/xe/xe_vm.h
@@ -212,6 +212,8 @@ int xe_vma_userptr_pin_pages(struct xe_vma *vma);
int xe_vma_userptr_check_repin(struct xe_vma *vma);
+struct xe_vma *xe_bind_scratch_vma(struct xe_vm *vm, u64 addr, u64 size);
+
/*
* XE_ONSTACK_TV is used to size the tv_onstack array that is input
* to xe_vm_lock_dma_resv() and xe_vm_unlock_dma_resv().
--
2.25.1
^ permalink raw reply related [flat|nested] 18+ messages in thread* [Intel-xe] ✗ CI.Patch_applied: failure for drm/xe: Enable scratch page when page fault is enabled (rev2)
2023-08-26 0:14 [Intel-xe] [PATCH] drm/xe: Enable scratch page when page fault is enabled Chang, Bruce
@ 2023-08-26 0:16 ` Patchwork
2023-08-28 17:55 ` [Intel-xe] [PATCH] drm/xe: Enable scratch page when page fault is enabled Summers, Stuart
2023-08-28 19:22 ` Matthew Brost
2 siblings, 0 replies; 18+ messages in thread
From: Patchwork @ 2023-08-26 0:16 UTC (permalink / raw)
To: Chang, Bruce; +Cc: intel-xe
== Series Details ==
Series: drm/xe: Enable scratch page when page fault is enabled (rev2)
URL : https://patchwork.freedesktop.org/series/120480/
State : failure
== Summary ==
=== Applying kernel patches on branch 'drm-xe-next' with base: ===
Base commit: b9c9020fc drm/xe/pvc: Use fast copy engines as migrate engine on PVC
=== git am output follows ===
Applying: drm/xe: Enable scratch page when page fault is enabled
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Intel-xe] [PATCH] drm/xe: Enable scratch page when page fault is enabled
2023-08-26 0:14 [Intel-xe] [PATCH] drm/xe: Enable scratch page when page fault is enabled Chang, Bruce
2023-08-26 0:16 ` [Intel-xe] ✗ CI.Patch_applied: failure for drm/xe: Enable scratch page when page fault is enabled (rev2) Patchwork
@ 2023-08-28 17:55 ` Summers, Stuart
2023-08-28 19:11 ` Chang, Yu bruce
2023-08-28 19:22 ` Matthew Brost
2 siblings, 1 reply; 18+ messages in thread
From: Summers, Stuart @ 2023-08-28 17:55 UTC (permalink / raw)
To: intel-xe@lists.freedesktop.org, Chang, Yu bruce
On Sat, 2023-08-26 at 00:14 +0000, Chang, Bruce wrote:
> i915 can use scratch page even when page fault is enabled, this patch
> is trying to port this feature over.
But why? From a debug perspective maybe this is interesting, but
ideally the failed page fault should give the app everything they need
to recover. Why not give the onus to the application to fix the
addressing problems?
>
> The current i915 solution changes page table directly which may be
> hard
> to make to upstream, so a more complex solution is needed to apply to
> the
> current Xe framework if following the existing i915 solution.
>
> This patch is trying to make the minimum impact to the existing
> driver,
> but still enable the scratch page support.
>
> So, the idea is to bind a scratch vma if the page fault is from an
> invalid access. This patch is taking advantage of null pte at this
> point, we may introduce a special vma for scratch vma if needed.
So basically we want to avoid a cat fault because the cat fault doesn't
give enough information about the bad address? Or is there something
else?
Thanks,
Stuart
> After the bind, the user app can continue to run without causing a
> fatal failure or reset and stop.
>
> In case the app will bind this scratch vma to a valid address, it
> will
> fail the bind, this patch will handle the failre and unbind the
> scrach
> vma[s], so that the user binding will be retried to the valid
> address.
>
> This patch only kicks in when there is a failure for both page fault
> and bind, so it should has no impact to the exiating code path. On
> another hand, it uses actual page tables instead of special scratch
> page tables, so it enables potential not to invalidate TLBs when
> doing
> unbind if all upper layer page tables are still being used.
>
> tested on new scratch igt tests which will be sent out for review.
>
> Cc: Oak Zeng <oak.zeng@intel.com>
> Cc: Brian Welty <brian.welty@intel.com>
> Cc: Niranjana Vishwanathapura <niranjana.vishwanathapura@intel.com>
> Cc: Stuart Summers <stuart.summers@intel.com>
> Cc: Matthew Brost <matthew.brost@intel.com>
> ---
> drivers/gpu/drm/xe/xe_gt_pagefault.c | 9 ++++--
> drivers/gpu/drm/xe/xe_vm.c | 48 +++++++++++++++++++++++---
> --
> drivers/gpu/drm/xe/xe_vm.h | 2 ++
> 3 files changed, 49 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/xe/xe_gt_pagefault.c
> b/drivers/gpu/drm/xe/xe_gt_pagefault.c
> index b6f781b3d9d7..524b38df3d7a 100644
> --- a/drivers/gpu/drm/xe/xe_gt_pagefault.c
> +++ b/drivers/gpu/drm/xe/xe_gt_pagefault.c
> @@ -137,8 +137,13 @@ static int handle_pagefault(struct xe_gt *gt,
> struct pagefault *pf)
> write_locked = true;
> vma = lookup_vma(vm, pf->page_addr);
> if (!vma) {
> - ret = -EINVAL;
> - goto unlock_vm;
> + if (vm->flags & XE_VM_FLAG_SCRATCH_PAGE)
> + vma = xe_bind_scratch_vma(vm, pf->page_addr,
> SZ_64K);
> +
> + if (!vma) {
> + ret = -EINVAL;
> + goto unlock_vm;
> + }
> }
>
> if (!xe_vma_is_userptr(vma) ||
> !xe_vma_userptr_check_repin(vma)) {
> diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
> index 389ac5ba8ddf..4c3d5d781b58 100644
> --- a/drivers/gpu/drm/xe/xe_vm.c
> +++ b/drivers/gpu/drm/xe/xe_vm.c
> @@ -1262,7 +1262,8 @@ struct xe_vm *xe_vm_create(struct xe_device
> *xe, u32 flags)
> }
> }
>
> - if (flags & XE_VM_FLAG_SCRATCH_PAGE) {
> + if (flags & XE_VM_FLAG_SCRATCH_PAGE &&
> + (!(flags & XE_VM_FLAG_FAULT_MODE))) {
> for_each_tile(tile, xe, id) {
> if (!vm->pt_root[id])
> continue;
> @@ -1998,10 +1999,6 @@ int xe_vm_create_ioctl(struct drm_device *dev,
> void *data,
> if (XE_IOCTL_DBG(xe, args->flags &
> ~ALL_DRM_XE_VM_CREATE_FLAGS))
> return -EINVAL;
>
> - if (XE_IOCTL_DBG(xe, args->flags &
> DRM_XE_VM_CREATE_SCRATCH_PAGE &&
> - args->flags & DRM_XE_VM_CREATE_FAULT_MODE))
> - return -EINVAL;
> -
> if (XE_IOCTL_DBG(xe, args->flags &
> DRM_XE_VM_CREATE_COMPUTE_MODE &&
> args->flags & DRM_XE_VM_CREATE_FAULT_MODE))
> return -EINVAL;
> @@ -2783,6 +2780,39 @@ static int __xe_vma_op_execute(struct xe_vm
> *vm, struct xe_vma *vma,
> return err;
> }
>
> +struct xe_vma *xe_bind_scratch_vma(struct xe_vm *vm, u64 addr, u64
> size)
> +{
> + struct xe_vma *vma = 0;
> +
> + if (!vm->size)
> + return 0;
> +
> + vma = xe_vma_create(vm, NULL, 0, addr, addr + size - 1,
> false, true, 0);
> + if (!vma)
> + return 0;
> + xe_vm_insert_vma(vm, vma);
> +
> + /* fault will handle the bind */
> +
> + return vma;
> +}
> +
> +int xe_unbind_scratch_vma(struct xe_vm *vm, u64 addr, u64 range)
> +{
> + struct xe_vma *vma;
> +
> + vma = xe_vm_find_overlapping_vma(vm, addr, range);
> + if (!vma)
> + return -ENXIO;
> +
> + if (xe_vma_is_null(vma)) {
> + prep_vma_destroy(vm, vma, true);
> + xe_vm_unbind(vm, vma, NULL, NULL, 0, NULL, true,
> false);
> + }
> +
> + return 0;
> +}
> +
> static int xe_vma_op_execute(struct xe_vm *vm, struct xe_vma_op *op)
> {
> int ret = 0;
> @@ -3205,7 +3235,6 @@ int xe_vm_bind_ioctl(struct drm_device *dev,
> void *data, struct drm_file *file)
> err = vm_bind_ioctl_check_args(xe, args, &bind_ops, &async);
> if (err)
> return err;
> -
> if (args->exec_queue_id) {
> q = xe_exec_queue_lookup(xef, args->exec_queue_id);
> if (XE_IOCTL_DBG(xe, !q)) {
> @@ -3352,10 +3381,13 @@ int xe_vm_bind_ioctl(struct drm_device *dev,
> void *data, struct drm_file *file)
> u64 range = bind_ops[i].range;
> u64 addr = bind_ops[i].addr;
> u32 op = bind_ops[i].op;
> -
> +retry:
> err = vm_bind_ioctl_lookup_vma(vm, bos[i], addr,
> range, op);
> - if (err)
> + if (err) {
> + if (!xe_unbind_scratch_vma(vm, addr, range))
> + goto retry;
> goto free_syncs;
> + }
> }
>
> for (i = 0; i < args->num_binds; ++i) {
> diff --git a/drivers/gpu/drm/xe/xe_vm.h b/drivers/gpu/drm/xe/xe_vm.h
> index 6de6e3edb24a..6447bed427b1 100644
> --- a/drivers/gpu/drm/xe/xe_vm.h
> +++ b/drivers/gpu/drm/xe/xe_vm.h
> @@ -212,6 +212,8 @@ int xe_vma_userptr_pin_pages(struct xe_vma *vma);
>
> int xe_vma_userptr_check_repin(struct xe_vma *vma);
>
> +struct xe_vma *xe_bind_scratch_vma(struct xe_vm *vm, u64 addr, u64
> size);
> +
> /*
> * XE_ONSTACK_TV is used to size the tv_onstack array that is input
> * to xe_vm_lock_dma_resv() and xe_vm_unlock_dma_resv().
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [Intel-xe] [PATCH] drm/xe: Enable scratch page when page fault is enabled
2023-08-28 17:55 ` [Intel-xe] [PATCH] drm/xe: Enable scratch page when page fault is enabled Summers, Stuart
@ 2023-08-28 19:11 ` Chang, Yu bruce
2023-08-28 22:25 ` Summers, Stuart
0 siblings, 1 reply; 18+ messages in thread
From: Chang, Yu bruce @ 2023-08-28 19:11 UTC (permalink / raw)
To: Summers, Stuart, intel-xe@lists.freedesktop.org
> -----Original Message-----
> From: Summers, Stuart <stuart.summers@intel.com>
> Sent: Monday, August 28, 2023 10:56 AM
> To: intel-xe@lists.freedesktop.org; Chang, Yu bruce
> <yu.bruce.chang@intel.com>
> Cc: Brost, Matthew <matthew.brost@intel.com>; Welty, Brian
> <brian.welty@intel.com>; Vishwanathapura, Niranjana
> <niranjana.vishwanathapura@intel.com>; Zeng, Oak <oak.zeng@intel.com>
> Subject: Re: [PATCH] drm/xe: Enable scratch page when page fault is enabled
>
> On Sat, 2023-08-26 at 00:14 +0000, Chang, Bruce wrote:
> > i915 can use scratch page even when page fault is enabled, this patch
> > is trying to port this feature over.
>
> But why? From a debug perspective maybe this is interesting, but ideally the
> failed page fault should give the app everything they need to recover. Why not
> give the onus to the application to fix the addressing problems?
>
You are correct that this feature was originally designed to help application
to run even they had invalid VA accesses. Now this should not be a goal anymore.
The main ask is for EU debug support, and also can be potential WA if there
Is any prefetch related issue, such as L1.
> >
> > The current i915 solution changes page table directly which may be
> > hard to make to upstream, so a more complex solution is needed to
> > apply to the current Xe framework if following the existing i915
> > solution.
> >
> > This patch is trying to make the minimum impact to the existing
> > driver, but still enable the scratch page support.
> >
> > So, the idea is to bind a scratch vma if the page fault is from an
> > invalid access. This patch is taking advantage of null pte at this
> > point, we may introduce a special vma for scratch vma if needed.
>
> So basically we want to avoid a cat fault because the cat fault doesn't give
> enough information about the bad address? Or is there something else?
>
> Thanks,
> Stuart
>
As mentioned above, mainly for EU debugger support and any potential
prefetch WA.
The issue you mentioned should be fixed with a GuC change along with
corresponding driver change. It is coming, but separately.
Thanks,
Bruce
> > After the bind, the user app can continue to run without causing a
> > fatal failure or reset and stop.
> >
> > In case the app will bind this scratch vma to a valid address, it will
> > fail the bind, this patch will handle the failre and unbind the scrach
> > vma[s], so that the user binding will be retried to the valid address.
> >
> > This patch only kicks in when there is a failure for both page fault
> > and bind, so it should has no impact to the exiating code path. On
> > another hand, it uses actual page tables instead of special scratch
> > page tables, so it enables potential not to invalidate TLBs when doing
> > unbind if all upper layer page tables are still being used.
> >
> > tested on new scratch igt tests which will be sent out for review.
> >
> > Cc: Oak Zeng <oak.zeng@intel.com>
> > Cc: Brian Welty <brian.welty@intel.com>
> > Cc: Niranjana Vishwanathapura <niranjana.vishwanathapura@intel.com>
> > Cc: Stuart Summers <stuart.summers@intel.com>
> > Cc: Matthew Brost <matthew.brost@intel.com>
> > ---
> > drivers/gpu/drm/xe/xe_gt_pagefault.c | 9 ++++--
> > drivers/gpu/drm/xe/xe_vm.c | 48 +++++++++++++++++++++++---
> > --
> > drivers/gpu/drm/xe/xe_vm.h | 2 ++
> > 3 files changed, 49 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/xe/xe_gt_pagefault.c
> > b/drivers/gpu/drm/xe/xe_gt_pagefault.c
> > index b6f781b3d9d7..524b38df3d7a 100644
> > --- a/drivers/gpu/drm/xe/xe_gt_pagefault.c
> > +++ b/drivers/gpu/drm/xe/xe_gt_pagefault.c
> > @@ -137,8 +137,13 @@ static int handle_pagefault(struct xe_gt *gt,
> > struct pagefault *pf)
> > write_locked = true;
> > vma = lookup_vma(vm, pf->page_addr);
> > if (!vma) {
> > - ret = -EINVAL;
> > - goto unlock_vm;
> > + if (vm->flags & XE_VM_FLAG_SCRATCH_PAGE)
> > + vma = xe_bind_scratch_vma(vm, pf->page_addr,
> > SZ_64K);
> > +
> > + if (!vma) {
> > + ret = -EINVAL;
> > + goto unlock_vm;
> > + }
> > }
> >
> > if (!xe_vma_is_userptr(vma) ||
> > !xe_vma_userptr_check_repin(vma)) {
> > diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
> > index 389ac5ba8ddf..4c3d5d781b58 100644
> > --- a/drivers/gpu/drm/xe/xe_vm.c
> > +++ b/drivers/gpu/drm/xe/xe_vm.c
> > @@ -1262,7 +1262,8 @@ struct xe_vm *xe_vm_create(struct xe_device
> *xe,
> > u32 flags)
> > }
> > }
> >
> > - if (flags & XE_VM_FLAG_SCRATCH_PAGE) {
> > + if (flags & XE_VM_FLAG_SCRATCH_PAGE &&
> > + (!(flags & XE_VM_FLAG_FAULT_MODE))) {
> > for_each_tile(tile, xe, id) {
> > if (!vm->pt_root[id])
> > continue; @@ -1998,10 +1999,6 @@ int
> > xe_vm_create_ioctl(struct drm_device *dev, void *data,
> > if (XE_IOCTL_DBG(xe, args->flags &
> > ~ALL_DRM_XE_VM_CREATE_FLAGS))
> > return -EINVAL;
> >
> > - if (XE_IOCTL_DBG(xe, args->flags &
> > DRM_XE_VM_CREATE_SCRATCH_PAGE &&
> > - args->flags & DRM_XE_VM_CREATE_FAULT_MODE))
> > - return -EINVAL;
> > -
> > if (XE_IOCTL_DBG(xe, args->flags &
> > DRM_XE_VM_CREATE_COMPUTE_MODE &&
> > args->flags & DRM_XE_VM_CREATE_FAULT_MODE))
> > return -EINVAL;
> > @@ -2783,6 +2780,39 @@ static int __xe_vma_op_execute(struct xe_vm
> > *vm, struct xe_vma *vma,
> > return err;
> > }
> >
> > +struct xe_vma *xe_bind_scratch_vma(struct xe_vm *vm, u64 addr, u64
> > size)
> > +{
> > + struct xe_vma *vma = 0;
> > +
> > + if (!vm->size)
> > + return 0;
> > +
> > + vma = xe_vma_create(vm, NULL, 0, addr, addr + size - 1,
> > false, true, 0);
> > + if (!vma)
> > + return 0;
> > + xe_vm_insert_vma(vm, vma);
> > +
> > + /* fault will handle the bind */
> > +
> > + return vma;
> > +}
> > +
> > +int xe_unbind_scratch_vma(struct xe_vm *vm, u64 addr, u64 range) {
> > + struct xe_vma *vma;
> > +
> > + vma = xe_vm_find_overlapping_vma(vm, addr, range);
> > + if (!vma)
> > + return -ENXIO;
> > +
> > + if (xe_vma_is_null(vma)) {
> > + prep_vma_destroy(vm, vma, true);
> > + xe_vm_unbind(vm, vma, NULL, NULL, 0, NULL, true,
> > false);
> > + }
> > +
> > + return 0;
> > +}
> > +
> > static int xe_vma_op_execute(struct xe_vm *vm, struct xe_vma_op *op)
> > {
> > int ret = 0;
> > @@ -3205,7 +3235,6 @@ int xe_vm_bind_ioctl(struct drm_device *dev,
> > void *data, struct drm_file *file)
> > err = vm_bind_ioctl_check_args(xe, args, &bind_ops, &async);
> > if (err)
> > return err;
> > -
> > if (args->exec_queue_id) {
> > q = xe_exec_queue_lookup(xef, args->exec_queue_id);
> > if (XE_IOCTL_DBG(xe, !q)) { @@ -3352,10 +3381,13 @@
> > int xe_vm_bind_ioctl(struct drm_device *dev, void *data, struct
> > drm_file *file)
> > u64 range = bind_ops[i].range;
> > u64 addr = bind_ops[i].addr;
> > u32 op = bind_ops[i].op;
> > -
> > +retry:
> > err = vm_bind_ioctl_lookup_vma(vm, bos[i], addr,
> > range, op);
> > - if (err)
> > + if (err) {
> > + if (!xe_unbind_scratch_vma(vm, addr, range))
> > + goto retry;
> > goto free_syncs;
> > + }
> > }
> >
> > for (i = 0; i < args->num_binds; ++i) { diff --git
> > a/drivers/gpu/drm/xe/xe_vm.h b/drivers/gpu/drm/xe/xe_vm.h index
> > 6de6e3edb24a..6447bed427b1 100644
> > --- a/drivers/gpu/drm/xe/xe_vm.h
> > +++ b/drivers/gpu/drm/xe/xe_vm.h
> > @@ -212,6 +212,8 @@ int xe_vma_userptr_pin_pages(struct xe_vma
> *vma);
> >
> > int xe_vma_userptr_check_repin(struct xe_vma *vma);
> >
> > +struct xe_vma *xe_bind_scratch_vma(struct xe_vm *vm, u64 addr, u64
> > size);
> > +
> > /*
> > * XE_ONSTACK_TV is used to size the tv_onstack array that is input
> > * to xe_vm_lock_dma_resv() and xe_vm_unlock_dma_resv().
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [Intel-xe] [PATCH] drm/xe: Enable scratch page when page fault is enabled
2023-08-28 19:11 ` Chang, Yu bruce
@ 2023-08-28 22:25 ` Summers, Stuart
2023-08-28 22:44 ` Chang, Yu bruce
0 siblings, 1 reply; 18+ messages in thread
From: Summers, Stuart @ 2023-08-28 22:25 UTC (permalink / raw)
To: intel-xe@lists.freedesktop.org, Chang, Yu bruce
On Mon, 2023-08-28 at 19:11 +0000, Chang, Yu bruce wrote:
>
>
> > -----Original Message-----
> > From: Summers, Stuart <stuart.summers@intel.com>
> > Sent: Monday, August 28, 2023 10:56 AM
> > To: intel-xe@lists.freedesktop.org; Chang, Yu bruce
> > <yu.bruce.chang@intel.com>
> > Cc: Brost, Matthew <matthew.brost@intel.com>; Welty, Brian
> > <brian.welty@intel.com>; Vishwanathapura, Niranjana
> > <niranjana.vishwanathapura@intel.com>; Zeng, Oak
> > <oak.zeng@intel.com>
> > Subject: Re: [PATCH] drm/xe: Enable scratch page when page fault is
> > enabled
> >
> > On Sat, 2023-08-26 at 00:14 +0000, Chang, Bruce wrote:
> > > i915 can use scratch page even when page fault is enabled, this
> > > patch
> > > is trying to port this feature over.
> >
> > But why? From a debug perspective maybe this is interesting, but
> > ideally the
> > failed page fault should give the app everything they need to
> > recover. Why not
> > give the onus to the application to fix the addressing problems?
> >
> You are correct that this feature was originally designed to help
> application
> to run even they had invalid VA accesses. Now this should not be a
> goal anymore.
>
> The main ask is for EU debug support, and also can be potential WA if
> there
> Is any prefetch related issue, such as L1.
I agree with what Matt B. had said. It would be nice to know why EU
debug needs this. Is there an alternate solution? If not, can we enable
this only if EU debug is active?
We also don't necessarily want a bunch of workarounds added for
potential issues we don't yet know about. That's adding undue burden on
the driver.
Thanks,
Stuart
>
> > >
> > > The current i915 solution changes page table directly which may
> > > be
> > > hard to make to upstream, so a more complex solution is needed to
> > > apply to the current Xe framework if following the existing i915
> > > solution.
> > >
> > > This patch is trying to make the minimum impact to the existing
> > > driver, but still enable the scratch page support.
> > >
> > > So, the idea is to bind a scratch vma if the page fault is from
> > > an
> > > invalid access. This patch is taking advantage of null pte at
> > > this
> > > point, we may introduce a special vma for scratch vma if needed.
> >
> > So basically we want to avoid a cat fault because the cat fault
> > doesn't give
> > enough information about the bad address? Or is there something
> > else?
> >
> > Thanks,
> > Stuart
> >
> As mentioned above, mainly for EU debugger support and any potential
> prefetch WA.
>
> The issue you mentioned should be fixed with a GuC change along with
> corresponding driver change. It is coming, but separately.
>
> Thanks,
> Bruce
>
> > > After the bind, the user app can continue to run without causing
> > > a
> > > fatal failure or reset and stop.
> > >
> > > In case the app will bind this scratch vma to a valid address, it
> > > will
> > > fail the bind, this patch will handle the failre and unbind the
> > > scrach
> > > vma[s], so that the user binding will be retried to the valid
> > > address.
> > >
> > > This patch only kicks in when there is a failure for both page
> > > fault
> > > and bind, so it should has no impact to the exiating code path.
> > > On
> > > another hand, it uses actual page tables instead of special
> > > scratch
> > > page tables, so it enables potential not to invalidate TLBs when
> > > doing
> > > unbind if all upper layer page tables are still being used.
> > >
> > > tested on new scratch igt tests which will be sent out for
> > > review.
> > >
> > > Cc: Oak Zeng <oak.zeng@intel.com>
> > > Cc: Brian Welty <brian.welty@intel.com>
> > > Cc: Niranjana Vishwanathapura
> > > <niranjana.vishwanathapura@intel.com>
> > > Cc: Stuart Summers <stuart.summers@intel.com>
> > > Cc: Matthew Brost <matthew.brost@intel.com>
> > > ---
> > > drivers/gpu/drm/xe/xe_gt_pagefault.c | 9 ++++--
> > > drivers/gpu/drm/xe/xe_vm.c | 48
> > > +++++++++++++++++++++++---
> > > --
> > > drivers/gpu/drm/xe/xe_vm.h | 2 ++
> > > 3 files changed, 49 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/xe/xe_gt_pagefault.c
> > > b/drivers/gpu/drm/xe/xe_gt_pagefault.c
> > > index b6f781b3d9d7..524b38df3d7a 100644
> > > --- a/drivers/gpu/drm/xe/xe_gt_pagefault.c
> > > +++ b/drivers/gpu/drm/xe/xe_gt_pagefault.c
> > > @@ -137,8 +137,13 @@ static int handle_pagefault(struct xe_gt
> > > *gt,
> > > struct pagefault *pf)
> > > write_locked = true;
> > > vma = lookup_vma(vm, pf->page_addr);
> > > if (!vma) {
> > > - ret = -EINVAL;
> > > - goto unlock_vm;
> > > + if (vm->flags & XE_VM_FLAG_SCRATCH_PAGE)
> > > + vma = xe_bind_scratch_vma(vm, pf-
> > > >page_addr,
> > > SZ_64K);
> > > +
> > > + if (!vma) {
> > > + ret = -EINVAL;
> > > + goto unlock_vm;
> > > + }
> > > }
> > >
> > > if (!xe_vma_is_userptr(vma) ||
> > > !xe_vma_userptr_check_repin(vma)) {
> > > diff --git a/drivers/gpu/drm/xe/xe_vm.c
> > > b/drivers/gpu/drm/xe/xe_vm.c
> > > index 389ac5ba8ddf..4c3d5d781b58 100644
> > > --- a/drivers/gpu/drm/xe/xe_vm.c
> > > +++ b/drivers/gpu/drm/xe/xe_vm.c
> > > @@ -1262,7 +1262,8 @@ struct xe_vm *xe_vm_create(struct xe_device
> > *xe,
> > > u32 flags)
> > > }
> > > }
> > >
> > > - if (flags & XE_VM_FLAG_SCRATCH_PAGE) {
> > > + if (flags & XE_VM_FLAG_SCRATCH_PAGE &&
> > > + (!(flags & XE_VM_FLAG_FAULT_MODE))) {
> > > for_each_tile(tile, xe, id) {
> > > if (!vm->pt_root[id])
> > > continue; @@ -1998,10 +1999,6 @@
> > > int
> > > xe_vm_create_ioctl(struct drm_device *dev, void *data,
> > > if (XE_IOCTL_DBG(xe, args->flags &
> > > ~ALL_DRM_XE_VM_CREATE_FLAGS))
> > > return -EINVAL;
> > >
> > > - if (XE_IOCTL_DBG(xe, args->flags &
> > > DRM_XE_VM_CREATE_SCRATCH_PAGE &&
> > > - args->flags &
> > > DRM_XE_VM_CREATE_FAULT_MODE))
> > > - return -EINVAL;
> > > -
> > > if (XE_IOCTL_DBG(xe, args->flags &
> > > DRM_XE_VM_CREATE_COMPUTE_MODE &&
> > > args->flags &
> > > DRM_XE_VM_CREATE_FAULT_MODE))
> > > return -EINVAL;
> > > @@ -2783,6 +2780,39 @@ static int __xe_vma_op_execute(struct
> > > xe_vm
> > > *vm, struct xe_vma *vma,
> > > return err;
> > > }
> > >
> > > +struct xe_vma *xe_bind_scratch_vma(struct xe_vm *vm, u64 addr,
> > > u64
> > > size)
> > > +{
> > > + struct xe_vma *vma = 0;
> > > +
> > > + if (!vm->size)
> > > + return 0;
> > > +
> > > + vma = xe_vma_create(vm, NULL, 0, addr, addr + size - 1,
> > > false, true, 0);
> > > + if (!vma)
> > > + return 0;
> > > + xe_vm_insert_vma(vm, vma);
> > > +
> > > + /* fault will handle the bind */
> > > +
> > > + return vma;
> > > +}
> > > +
> > > +int xe_unbind_scratch_vma(struct xe_vm *vm, u64 addr, u64 range)
> > > {
> > > + struct xe_vma *vma;
> > > +
> > > + vma = xe_vm_find_overlapping_vma(vm, addr, range);
> > > + if (!vma)
> > > + return -ENXIO;
> > > +
> > > + if (xe_vma_is_null(vma)) {
> > > + prep_vma_destroy(vm, vma, true);
> > > + xe_vm_unbind(vm, vma, NULL, NULL, 0, NULL, true,
> > > false);
> > > + }
> > > +
> > > + return 0;
> > > +}
> > > +
> > > static int xe_vma_op_execute(struct xe_vm *vm, struct xe_vma_op
> > > *op)
> > > {
> > > int ret = 0;
> > > @@ -3205,7 +3235,6 @@ int xe_vm_bind_ioctl(struct drm_device
> > > *dev,
> > > void *data, struct drm_file *file)
> > > err = vm_bind_ioctl_check_args(xe, args, &bind_ops,
> > > &async);
> > > if (err)
> > > return err;
> > > -
> > > if (args->exec_queue_id) {
> > > q = xe_exec_queue_lookup(xef, args-
> > > >exec_queue_id);
> > > if (XE_IOCTL_DBG(xe, !q)) { @@ -3352,10 +3381,13
> > > @@
> > > int xe_vm_bind_ioctl(struct drm_device *dev, void *data, struct
> > > drm_file *file)
> > > u64 range = bind_ops[i].range;
> > > u64 addr = bind_ops[i].addr;
> > > u32 op = bind_ops[i].op;
> > > -
> > > +retry:
> > > err = vm_bind_ioctl_lookup_vma(vm, bos[i], addr,
> > > range, op);
> > > - if (err)
> > > + if (err) {
> > > + if (!xe_unbind_scratch_vma(vm, addr,
> > > range))
> > > + goto retry;
> > > goto free_syncs;
> > > + }
> > > }
> > >
> > > for (i = 0; i < args->num_binds; ++i) { diff --git
> > > a/drivers/gpu/drm/xe/xe_vm.h b/drivers/gpu/drm/xe/xe_vm.h index
> > > 6de6e3edb24a..6447bed427b1 100644
> > > --- a/drivers/gpu/drm/xe/xe_vm.h
> > > +++ b/drivers/gpu/drm/xe/xe_vm.h
> > > @@ -212,6 +212,8 @@ int xe_vma_userptr_pin_pages(struct xe_vma
> > *vma);
> > >
> > > int xe_vma_userptr_check_repin(struct xe_vma *vma);
> > >
> > > +struct xe_vma *xe_bind_scratch_vma(struct xe_vm *vm, u64 addr,
> > > u64
> > > size);
> > > +
> > > /*
> > > * XE_ONSTACK_TV is used to size the tv_onstack array that is
> > > input
> > > * to xe_vm_lock_dma_resv() and xe_vm_unlock_dma_resv().
>
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [Intel-xe] [PATCH] drm/xe: Enable scratch page when page fault is enabled
2023-08-28 22:25 ` Summers, Stuart
@ 2023-08-28 22:44 ` Chang, Yu bruce
2023-08-29 16:36 ` Summers, Stuart
0 siblings, 1 reply; 18+ messages in thread
From: Chang, Yu bruce @ 2023-08-28 22:44 UTC (permalink / raw)
To: Summers, Stuart, intel-xe@lists.freedesktop.org
> -----Original Message-----
> From: Summers, Stuart <stuart.summers@intel.com>
> Sent: Monday, August 28, 2023 3:26 PM
> To: intel-xe@lists.freedesktop.org; Chang, Yu bruce <yu.bruce.chang@intel.com>
> Cc: Brost, Matthew <matthew.brost@intel.com>; Welty, Brian
> <brian.welty@intel.com>; Vishwanathapura, Niranjana
> <niranjana.vishwanathapura@intel.com>; Zeng, Oak <oak.zeng@intel.com>
> Subject: Re: [PATCH] drm/xe: Enable scratch page when page fault is enabled
>
> On Mon, 2023-08-28 at 19:11 +0000, Chang, Yu bruce wrote:
> >
> >
> > > -----Original Message-----
> > > From: Summers, Stuart <stuart.summers@intel.com>
> > > Sent: Monday, August 28, 2023 10:56 AM
> > > To: intel-xe@lists.freedesktop.org; Chang, Yu bruce
> > > <yu.bruce.chang@intel.com>
> > > Cc: Brost, Matthew <matthew.brost@intel.com>; Welty, Brian
> > > <brian.welty@intel.com>; Vishwanathapura, Niranjana
> > > <niranjana.vishwanathapura@intel.com>; Zeng, Oak
> > > <oak.zeng@intel.com>
> > > Subject: Re: [PATCH] drm/xe: Enable scratch page when page fault is
> > > enabled
> > >
> > > On Sat, 2023-08-26 at 00:14 +0000, Chang, Bruce wrote:
> > > > i915 can use scratch page even when page fault is enabled, this
> > > > patch is trying to port this feature over.
> > >
> > > But why? From a debug perspective maybe this is interesting, but
> > > ideally the failed page fault should give the app everything they
> > > need to recover. Why not give the onus to the application to fix the
> > > addressing problems?
> > >
> > You are correct that this feature was originally designed to help
> > application to run even they had invalid VA accesses. Now this should
> > not be a goal anymore.
> >
> > The main ask is for EU debug support, and also can be potential WA if
> > there Is any prefetch related issue, such as L1.
>
> I agree with what Matt B. had said. It would be nice to know why EU debug needs
> this. Is there an alternate solution? If not, can we enable this only if EU debug is
> active?
>
> We also don't necessarily want a bunch of workarounds added for potential
> issues we don't yet know about. That's adding undue burden on the driver.
>
> Thanks,
> Stuart
>
The PVC HW has a limitation that the page fault due to invalid accessing will halt
the EUs accessing it, and the default behavior is CAT fault. So, in order to activate
the debugger, kmd needs to setup the scratch pages to unhalt the EUs.
this feature can only be enabled if scratch flag is set. So, once EU debugger is running,
the debugger umd will set the scratch flag, otherwise, this flag should not be set.
So, in regular run, this feature should not be activated.
Will add this details into commit comment to make things more clear.
Thanks,
Bruce
> >
> > > >
> > > > The current i915 solution changes page table directly which may be
> > > > hard to make to upstream, so a more complex solution is needed to
> > > > apply to the current Xe framework if following the existing i915
> > > > solution.
> > > >
> > > > This patch is trying to make the minimum impact to the existing
> > > > driver, but still enable the scratch page support.
> > > >
> > > > So, the idea is to bind a scratch vma if the page fault is from an
> > > > invalid access. This patch is taking advantage of null pte at this
> > > > point, we may introduce a special vma for scratch vma if needed.
> > >
> > > So basically we want to avoid a cat fault because the cat fault
> > > doesn't give enough information about the bad address? Or is there
> > > something else?
> > >
> > > Thanks,
> > > Stuart
> > >
> > As mentioned above, mainly for EU debugger support and any potential
> > prefetch WA.
> >
> > The issue you mentioned should be fixed with a GuC change along with
> > corresponding driver change. It is coming, but separately.
> >
> > Thanks,
> > Bruce
> >
> > > > After the bind, the user app can continue to run without causing a
> > > > fatal failure or reset and stop.
> > > >
> > > > In case the app will bind this scratch vma to a valid address, it
> > > > will fail the bind, this patch will handle the failre and unbind
> > > > the scrach vma[s], so that the user binding will be retried to the
> > > > valid address.
> > > >
> > > > This patch only kicks in when there is a failure for both page
> > > > fault and bind, so it should has no impact to the exiating code
> > > > path.
> > > > On
> > > > another hand, it uses actual page tables instead of special
> > > > scratch page tables, so it enables potential not to invalidate
> > > > TLBs when doing unbind if all upper layer page tables are still
> > > > being used.
> > > >
> > > > tested on new scratch igt tests which will be sent out for review.
> > > >
> > > > Cc: Oak Zeng <oak.zeng@intel.com>
> > > > Cc: Brian Welty <brian.welty@intel.com>
> > > > Cc: Niranjana Vishwanathapura
> > > > <niranjana.vishwanathapura@intel.com>
> > > > Cc: Stuart Summers <stuart.summers@intel.com>
> > > > Cc: Matthew Brost <matthew.brost@intel.com>
> > > > ---
> > > > drivers/gpu/drm/xe/xe_gt_pagefault.c | 9 ++++--
> > > > drivers/gpu/drm/xe/xe_vm.c | 48
> > > > +++++++++++++++++++++++---
> > > > --
> > > > drivers/gpu/drm/xe/xe_vm.h | 2 ++
> > > > 3 files changed, 49 insertions(+), 10 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/xe/xe_gt_pagefault.c
> > > > b/drivers/gpu/drm/xe/xe_gt_pagefault.c
> > > > index b6f781b3d9d7..524b38df3d7a 100644
> > > > --- a/drivers/gpu/drm/xe/xe_gt_pagefault.c
> > > > +++ b/drivers/gpu/drm/xe/xe_gt_pagefault.c
> > > > @@ -137,8 +137,13 @@ static int handle_pagefault(struct xe_gt *gt,
> > > > struct pagefault *pf)
> > > > write_locked = true;
> > > > vma = lookup_vma(vm, pf->page_addr);
> > > > if (!vma) {
> > > > - ret = -EINVAL;
> > > > - goto unlock_vm;
> > > > + if (vm->flags & XE_VM_FLAG_SCRATCH_PAGE)
> > > > + vma = xe_bind_scratch_vma(vm, pf-
> > > > >page_addr,
> > > > SZ_64K);
> > > > +
> > > > + if (!vma) {
> > > > + ret = -EINVAL;
> > > > + goto unlock_vm;
> > > > + }
> > > > }
> > > >
> > > > if (!xe_vma_is_userptr(vma) ||
> > > > !xe_vma_userptr_check_repin(vma)) { diff --git
> > > > a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c index
> > > > 389ac5ba8ddf..4c3d5d781b58 100644
> > > > --- a/drivers/gpu/drm/xe/xe_vm.c
> > > > +++ b/drivers/gpu/drm/xe/xe_vm.c
> > > > @@ -1262,7 +1262,8 @@ struct xe_vm *xe_vm_create(struct xe_device
> > > *xe,
> > > > u32 flags)
> > > > }
> > > > }
> > > >
> > > > - if (flags & XE_VM_FLAG_SCRATCH_PAGE) {
> > > > + if (flags & XE_VM_FLAG_SCRATCH_PAGE &&
> > > > + (!(flags & XE_VM_FLAG_FAULT_MODE))) {
> > > > for_each_tile(tile, xe, id) {
> > > > if (!vm->pt_root[id])
> > > > continue; @@ -1998,10 +1999,6 @@
> > > > int xe_vm_create_ioctl(struct drm_device *dev, void *data,
> > > > if (XE_IOCTL_DBG(xe, args->flags &
> > > > ~ALL_DRM_XE_VM_CREATE_FLAGS))
> > > > return -EINVAL;
> > > >
> > > > - if (XE_IOCTL_DBG(xe, args->flags &
> > > > DRM_XE_VM_CREATE_SCRATCH_PAGE &&
> > > > - args->flags &
> > > > DRM_XE_VM_CREATE_FAULT_MODE))
> > > > - return -EINVAL;
> > > > -
> > > > if (XE_IOCTL_DBG(xe, args->flags &
> > > > DRM_XE_VM_CREATE_COMPUTE_MODE &&
> > > > args->flags &
> > > > DRM_XE_VM_CREATE_FAULT_MODE))
> > > > return -EINVAL;
> > > > @@ -2783,6 +2780,39 @@ static int __xe_vma_op_execute(struct xe_vm
> > > > *vm, struct xe_vma *vma,
> > > > return err;
> > > > }
> > > >
> > > > +struct xe_vma *xe_bind_scratch_vma(struct xe_vm *vm, u64 addr,
> > > > u64
> > > > size)
> > > > +{
> > > > + struct xe_vma *vma = 0;
> > > > +
> > > > + if (!vm->size)
> > > > + return 0;
> > > > +
> > > > + vma = xe_vma_create(vm, NULL, 0, addr, addr + size - 1,
> > > > false, true, 0);
> > > > + if (!vma)
> > > > + return 0;
> > > > + xe_vm_insert_vma(vm, vma);
> > > > +
> > > > + /* fault will handle the bind */
> > > > +
> > > > + return vma;
> > > > +}
> > > > +
> > > > +int xe_unbind_scratch_vma(struct xe_vm *vm, u64 addr, u64 range)
> > > > {
> > > > + struct xe_vma *vma;
> > > > +
> > > > + vma = xe_vm_find_overlapping_vma(vm, addr, range);
> > > > + if (!vma)
> > > > + return -ENXIO;
> > > > +
> > > > + if (xe_vma_is_null(vma)) {
> > > > + prep_vma_destroy(vm, vma, true);
> > > > + xe_vm_unbind(vm, vma, NULL, NULL, 0, NULL, true,
> > > > false);
> > > > + }
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > static int xe_vma_op_execute(struct xe_vm *vm, struct xe_vma_op
> > > > *op)
> > > > {
> > > > int ret = 0;
> > > > @@ -3205,7 +3235,6 @@ int xe_vm_bind_ioctl(struct drm_device *dev,
> > > > void *data, struct drm_file *file)
> > > > err = vm_bind_ioctl_check_args(xe, args, &bind_ops,
> > > > &async);
> > > > if (err)
> > > > return err;
> > > > -
> > > > if (args->exec_queue_id) {
> > > > q = xe_exec_queue_lookup(xef, args-
> > > > >exec_queue_id);
> > > > if (XE_IOCTL_DBG(xe, !q)) { @@ -3352,10 +3381,13
> > > > @@ int xe_vm_bind_ioctl(struct drm_device *dev, void *data, struct
> > > > drm_file *file)
> > > > u64 range = bind_ops[i].range;
> > > > u64 addr = bind_ops[i].addr;
> > > > u32 op = bind_ops[i].op;
> > > > -
> > > > +retry:
> > > > err = vm_bind_ioctl_lookup_vma(vm, bos[i], addr,
> > > > range, op);
> > > > - if (err)
> > > > + if (err) {
> > > > + if (!xe_unbind_scratch_vma(vm, addr,
> > > > range))
> > > > + goto retry;
> > > > goto free_syncs;
> > > > + }
> > > > }
> > > >
> > > > for (i = 0; i < args->num_binds; ++i) { diff --git
> > > > a/drivers/gpu/drm/xe/xe_vm.h b/drivers/gpu/drm/xe/xe_vm.h index
> > > > 6de6e3edb24a..6447bed427b1 100644
> > > > --- a/drivers/gpu/drm/xe/xe_vm.h
> > > > +++ b/drivers/gpu/drm/xe/xe_vm.h
> > > > @@ -212,6 +212,8 @@ int xe_vma_userptr_pin_pages(struct xe_vma
> > > *vma);
> > > >
> > > > int xe_vma_userptr_check_repin(struct xe_vma *vma);
> > > >
> > > > +struct xe_vma *xe_bind_scratch_vma(struct xe_vm *vm, u64 addr,
> > > > u64
> > > > size);
> > > > +
> > > > /*
> > > > * XE_ONSTACK_TV is used to size the tv_onstack array that is
> > > > input
> > > > * to xe_vm_lock_dma_resv() and xe_vm_unlock_dma_resv().
> >
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [Intel-xe] [PATCH] drm/xe: Enable scratch page when page fault is enabled
2023-08-28 22:44 ` Chang, Yu bruce
@ 2023-08-29 16:36 ` Summers, Stuart
0 siblings, 0 replies; 18+ messages in thread
From: Summers, Stuart @ 2023-08-29 16:36 UTC (permalink / raw)
To: intel-xe@lists.freedesktop.org, Chang, Yu bruce
On Mon, 2023-08-28 at 22:44 +0000, Chang, Yu bruce wrote:
>
>
> > -----Original Message-----
> > From: Summers, Stuart <stuart.summers@intel.com>
> > Sent: Monday, August 28, 2023 3:26 PM
> > To: intel-xe@lists.freedesktop.org; Chang, Yu bruce
> > <yu.bruce.chang@intel.com>
> > Cc: Brost, Matthew <matthew.brost@intel.com>; Welty, Brian
> > <brian.welty@intel.com>; Vishwanathapura, Niranjana
> > <niranjana.vishwanathapura@intel.com>; Zeng, Oak
> > <oak.zeng@intel.com>
> > Subject: Re: [PATCH] drm/xe: Enable scratch page when page fault is
> > enabled
> >
> > On Mon, 2023-08-28 at 19:11 +0000, Chang, Yu bruce wrote:
> > >
> > >
> > > > -----Original Message-----
> > > > From: Summers, Stuart <stuart.summers@intel.com>
> > > > Sent: Monday, August 28, 2023 10:56 AM
> > > > To: intel-xe@lists.freedesktop.org; Chang, Yu bruce
> > > > <yu.bruce.chang@intel.com>
> > > > Cc: Brost, Matthew <matthew.brost@intel.com>; Welty, Brian
> > > > <brian.welty@intel.com>; Vishwanathapura, Niranjana
> > > > <niranjana.vishwanathapura@intel.com>; Zeng, Oak
> > > > <oak.zeng@intel.com>
> > > > Subject: Re: [PATCH] drm/xe: Enable scratch page when page
> > > > fault is
> > > > enabled
> > > >
> > > > On Sat, 2023-08-26 at 00:14 +0000, Chang, Bruce wrote:
> > > > > i915 can use scratch page even when page fault is enabled,
> > > > > this
> > > > > patch is trying to port this feature over.
> > > >
> > > > But why? From a debug perspective maybe this is interesting,
> > > > but
> > > > ideally the failed page fault should give the app everything
> > > > they
> > > > need to recover. Why not give the onus to the application to
> > > > fix the
> > > > addressing problems?
> > > >
> > > You are correct that this feature was originally designed to help
> > > application to run even they had invalid VA accesses. Now this
> > > should
> > > not be a goal anymore.
> > >
> > > The main ask is for EU debug support, and also can be potential
> > > WA if
> > > there Is any prefetch related issue, such as L1.
> >
> > I agree with what Matt B. had said. It would be nice to know why EU
> > debug needs
> > this. Is there an alternate solution? If not, can we enable this
> > only if EU debug is
> > active?
> >
> > We also don't necessarily want a bunch of workarounds added for
> > potential
> > issues we don't yet know about. That's adding undue burden on the
> > driver.
> >
> > Thanks,
> > Stuart
> >
> The PVC HW has a limitation that the page fault due to invalid
> accessing will halt
> the EUs accessing it, and the default behavior is CAT fault. So, in
> order to activate
> the debugger, kmd needs to setup the scratch pages to unhalt the EUs.
We should add this as an explicit workaround for PVC then, not a
general feature. Also IMO it's better to hold off merging this until EU
debug is ready to consume it. We shouldn't have dead code sitting here
untested.
Do you have a link to the EU debug series adding this support?
Thanks,
Stuart
>
> this feature can only be enabled if scratch flag is set. So, once EU
> debugger is running,
> the debugger umd will set the scratch flag, otherwise, this flag
> should not be set.
> So, in regular run, this feature should not be activated.
>
> Will add this details into commit comment to make things more clear.
>
> Thanks,
> Bruce
>
> > >
> > > > >
> > > > > The current i915 solution changes page table directly which
> > > > > may be
> > > > > hard to make to upstream, so a more complex solution is
> > > > > needed to
> > > > > apply to the current Xe framework if following the existing
> > > > > i915
> > > > > solution.
> > > > >
> > > > > This patch is trying to make the minimum impact to the
> > > > > existing
> > > > > driver, but still enable the scratch page support.
> > > > >
> > > > > So, the idea is to bind a scratch vma if the page fault is
> > > > > from an
> > > > > invalid access. This patch is taking advantage of null pte at
> > > > > this
> > > > > point, we may introduce a special vma for scratch vma if
> > > > > needed.
> > > >
> > > > So basically we want to avoid a cat fault because the cat fault
> > > > doesn't give enough information about the bad address? Or is
> > > > there
> > > > something else?
> > > >
> > > > Thanks,
> > > > Stuart
> > > >
> > > As mentioned above, mainly for EU debugger support and any
> > > potential
> > > prefetch WA.
> > >
> > > The issue you mentioned should be fixed with a GuC change along
> > > with
> > > corresponding driver change. It is coming, but separately.
> > >
> > > Thanks,
> > > Bruce
> > >
> > > > > After the bind, the user app can continue to run without
> > > > > causing a
> > > > > fatal failure or reset and stop.
> > > > >
> > > > > In case the app will bind this scratch vma to a valid
> > > > > address, it
> > > > > will fail the bind, this patch will handle the failre and
> > > > > unbind
> > > > > the scrach vma[s], so that the user binding will be retried
> > > > > to the
> > > > > valid address.
> > > > >
> > > > > This patch only kicks in when there is a failure for both
> > > > > page
> > > > > fault and bind, so it should has no impact to the exiating
> > > > > code
> > > > > path.
> > > > > On
> > > > > another hand, it uses actual page tables instead of special
> > > > > scratch page tables, so it enables potential not to
> > > > > invalidate
> > > > > TLBs when doing unbind if all upper layer page tables are
> > > > > still
> > > > > being used.
> > > > >
> > > > > tested on new scratch igt tests which will be sent out for
> > > > > review.
> > > > >
> > > > > Cc: Oak Zeng <oak.zeng@intel.com>
> > > > > Cc: Brian Welty <brian.welty@intel.com>
> > > > > Cc: Niranjana Vishwanathapura
> > > > > <niranjana.vishwanathapura@intel.com>
> > > > > Cc: Stuart Summers <stuart.summers@intel.com>
> > > > > Cc: Matthew Brost <matthew.brost@intel.com>
> > > > > ---
> > > > > drivers/gpu/drm/xe/xe_gt_pagefault.c | 9 ++++--
> > > > > drivers/gpu/drm/xe/xe_vm.c | 48
> > > > > +++++++++++++++++++++++---
> > > > > --
> > > > > drivers/gpu/drm/xe/xe_vm.h | 2 ++
> > > > > 3 files changed, 49 insertions(+), 10 deletions(-)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/xe/xe_gt_pagefault.c
> > > > > b/drivers/gpu/drm/xe/xe_gt_pagefault.c
> > > > > index b6f781b3d9d7..524b38df3d7a 100644
> > > > > --- a/drivers/gpu/drm/xe/xe_gt_pagefault.c
> > > > > +++ b/drivers/gpu/drm/xe/xe_gt_pagefault.c
> > > > > @@ -137,8 +137,13 @@ static int handle_pagefault(struct xe_gt
> > > > > *gt,
> > > > > struct pagefault *pf)
> > > > > write_locked = true;
> > > > > vma = lookup_vma(vm, pf->page_addr);
> > > > > if (!vma) {
> > > > > - ret = -EINVAL;
> > > > > - goto unlock_vm;
> > > > > + if (vm->flags & XE_VM_FLAG_SCRATCH_PAGE)
> > > > > + vma = xe_bind_scratch_vma(vm, pf-
> > > > > > page_addr,
> > > > > SZ_64K);
> > > > > +
> > > > > + if (!vma) {
> > > > > + ret = -EINVAL;
> > > > > + goto unlock_vm;
> > > > > + }
> > > > > }
> > > > >
> > > > > if (!xe_vma_is_userptr(vma) ||
> > > > > !xe_vma_userptr_check_repin(vma)) { diff --git
> > > > > a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
> > > > > index
> > > > > 389ac5ba8ddf..4c3d5d781b58 100644
> > > > > --- a/drivers/gpu/drm/xe/xe_vm.c
> > > > > +++ b/drivers/gpu/drm/xe/xe_vm.c
> > > > > @@ -1262,7 +1262,8 @@ struct xe_vm *xe_vm_create(struct
> > > > > xe_device
> > > > *xe,
> > > > > u32 flags)
> > > > > }
> > > > > }
> > > > >
> > > > > - if (flags & XE_VM_FLAG_SCRATCH_PAGE) {
> > > > > + if (flags & XE_VM_FLAG_SCRATCH_PAGE &&
> > > > > + (!(flags & XE_VM_FLAG_FAULT_MODE))) {
> > > > > for_each_tile(tile, xe, id) {
> > > > > if (!vm->pt_root[id])
> > > > > continue; @@ -1998,10 +1999,6
> > > > > @@
> > > > > int xe_vm_create_ioctl(struct drm_device *dev, void *data,
> > > > > if (XE_IOCTL_DBG(xe, args->flags &
> > > > > ~ALL_DRM_XE_VM_CREATE_FLAGS))
> > > > > return -EINVAL;
> > > > >
> > > > > - if (XE_IOCTL_DBG(xe, args->flags &
> > > > > DRM_XE_VM_CREATE_SCRATCH_PAGE &&
> > > > > - args->flags &
> > > > > DRM_XE_VM_CREATE_FAULT_MODE))
> > > > > - return -EINVAL;
> > > > > -
> > > > > if (XE_IOCTL_DBG(xe, args->flags &
> > > > > DRM_XE_VM_CREATE_COMPUTE_MODE &&
> > > > > args->flags &
> > > > > DRM_XE_VM_CREATE_FAULT_MODE))
> > > > > return -EINVAL;
> > > > > @@ -2783,6 +2780,39 @@ static int __xe_vma_op_execute(struct
> > > > > xe_vm
> > > > > *vm, struct xe_vma *vma,
> > > > > return err;
> > > > > }
> > > > >
> > > > > +struct xe_vma *xe_bind_scratch_vma(struct xe_vm *vm, u64
> > > > > addr,
> > > > > u64
> > > > > size)
> > > > > +{
> > > > > + struct xe_vma *vma = 0;
> > > > > +
> > > > > + if (!vm->size)
> > > > > + return 0;
> > > > > +
> > > > > + vma = xe_vma_create(vm, NULL, 0, addr, addr + size -
> > > > > 1,
> > > > > false, true, 0);
> > > > > + if (!vma)
> > > > > + return 0;
> > > > > + xe_vm_insert_vma(vm, vma);
> > > > > +
> > > > > + /* fault will handle the bind */
> > > > > +
> > > > > + return vma;
> > > > > +}
> > > > > +
> > > > > +int xe_unbind_scratch_vma(struct xe_vm *vm, u64 addr, u64
> > > > > range)
> > > > > {
> > > > > + struct xe_vma *vma;
> > > > > +
> > > > > + vma = xe_vm_find_overlapping_vma(vm, addr, range);
> > > > > + if (!vma)
> > > > > + return -ENXIO;
> > > > > +
> > > > > + if (xe_vma_is_null(vma)) {
> > > > > + prep_vma_destroy(vm, vma, true);
> > > > > + xe_vm_unbind(vm, vma, NULL, NULL, 0, NULL,
> > > > > true,
> > > > > false);
> > > > > + }
> > > > > +
> > > > > + return 0;
> > > > > +}
> > > > > +
> > > > > static int xe_vma_op_execute(struct xe_vm *vm, struct
> > > > > xe_vma_op
> > > > > *op)
> > > > > {
> > > > > int ret = 0;
> > > > > @@ -3205,7 +3235,6 @@ int xe_vm_bind_ioctl(struct drm_device
> > > > > *dev,
> > > > > void *data, struct drm_file *file)
> > > > > err = vm_bind_ioctl_check_args(xe, args, &bind_ops,
> > > > > &async);
> > > > > if (err)
> > > > > return err;
> > > > > -
> > > > > if (args->exec_queue_id) {
> > > > > q = xe_exec_queue_lookup(xef, args-
> > > > > > exec_queue_id);
> > > > > if (XE_IOCTL_DBG(xe, !q)) { @@ -3352,10
> > > > > +3381,13
> > > > > @@ int xe_vm_bind_ioctl(struct drm_device *dev, void *data,
> > > > > struct
> > > > > drm_file *file)
> > > > > u64 range = bind_ops[i].range;
> > > > > u64 addr = bind_ops[i].addr;
> > > > > u32 op = bind_ops[i].op;
> > > > > -
> > > > > +retry:
> > > > > err = vm_bind_ioctl_lookup_vma(vm, bos[i],
> > > > > addr,
> > > > > range, op);
> > > > > - if (err)
> > > > > + if (err) {
> > > > > + if (!xe_unbind_scratch_vma(vm, addr,
> > > > > range))
> > > > > + goto retry;
> > > > > goto free_syncs;
> > > > > + }
> > > > > }
> > > > >
> > > > > for (i = 0; i < args->num_binds; ++i) { diff --git
> > > > > a/drivers/gpu/drm/xe/xe_vm.h b/drivers/gpu/drm/xe/xe_vm.h
> > > > > index
> > > > > 6de6e3edb24a..6447bed427b1 100644
> > > > > --- a/drivers/gpu/drm/xe/xe_vm.h
> > > > > +++ b/drivers/gpu/drm/xe/xe_vm.h
> > > > > @@ -212,6 +212,8 @@ int xe_vma_userptr_pin_pages(struct
> > > > > xe_vma
> > > > *vma);
> > > > >
> > > > > int xe_vma_userptr_check_repin(struct xe_vma *vma);
> > > > >
> > > > > +struct xe_vma *xe_bind_scratch_vma(struct xe_vm *vm, u64
> > > > > addr,
> > > > > u64
> > > > > size);
> > > > > +
> > > > > /*
> > > > > * XE_ONSTACK_TV is used to size the tv_onstack array that
> > > > > is
> > > > > input
> > > > > * to xe_vm_lock_dma_resv() and xe_vm_unlock_dma_resv().
> > >
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Intel-xe] [PATCH] drm/xe: Enable scratch page when page fault is enabled
2023-08-26 0:14 [Intel-xe] [PATCH] drm/xe: Enable scratch page when page fault is enabled Chang, Bruce
2023-08-26 0:16 ` [Intel-xe] ✗ CI.Patch_applied: failure for drm/xe: Enable scratch page when page fault is enabled (rev2) Patchwork
2023-08-28 17:55 ` [Intel-xe] [PATCH] drm/xe: Enable scratch page when page fault is enabled Summers, Stuart
@ 2023-08-28 19:22 ` Matthew Brost
2023-08-28 22:34 ` Chang, Yu bruce
2 siblings, 1 reply; 18+ messages in thread
From: Matthew Brost @ 2023-08-28 19:22 UTC (permalink / raw)
To: Chang, Bruce; +Cc: Stuart Summers, intel-xe
On Sat, Aug 26, 2023 at 12:14:12AM +0000, Chang, Bruce wrote:
> i915 can use scratch page even when page fault is enabled, this patch
> is trying to port this feature over.
>
I think we need to ask why do we support this before merging this.
Because the i915 does this is not a valid answer. Please explain why a
UMD needs this feature.
> The current i915 solution changes page table directly which may be hard
> to make to upstream, so a more complex solution is needed to apply to the
> current Xe framework if following the existing i915 solution.
>
> This patch is trying to make the minimum impact to the existing driver,
> but still enable the scratch page support.
>
> So, the idea is to bind a scratch vma if the page fault is from an
> invalid access. This patch is taking advantage of null pte at this
> point, we may introduce a special vma for scratch vma if needed.
> After the bind, the user app can continue to run without causing a
> fatal failure or reset and stop.
>
> In case the app will bind this scratch vma to a valid address, it will
> fail the bind, this patch will handle the failre and unbind the scrach
> vma[s], so that the user binding will be retried to the valid address.
>
> This patch only kicks in when there is a failure for both page fault
> and bind, so it should has no impact to the exiating code path. On
> another hand, it uses actual page tables instead of special scratch
> page tables, so it enables potential not to invalidate TLBs when doing
> unbind if all upper layer page tables are still being used.
>
> tested on new scratch igt tests which will be sent out for review.
>
> Cc: Oak Zeng <oak.zeng@intel.com>
> Cc: Brian Welty <brian.welty@intel.com>
> Cc: Niranjana Vishwanathapura <niranjana.vishwanathapura@intel.com>
> Cc: Stuart Summers <stuart.summers@intel.com>
> Cc: Matthew Brost <matthew.brost@intel.com>
> ---
> drivers/gpu/drm/xe/xe_gt_pagefault.c | 9 ++++--
> drivers/gpu/drm/xe/xe_vm.c | 48 +++++++++++++++++++++++-----
> drivers/gpu/drm/xe/xe_vm.h | 2 ++
> 3 files changed, 49 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/xe/xe_gt_pagefault.c b/drivers/gpu/drm/xe/xe_gt_pagefault.c
> index b6f781b3d9d7..524b38df3d7a 100644
> --- a/drivers/gpu/drm/xe/xe_gt_pagefault.c
> +++ b/drivers/gpu/drm/xe/xe_gt_pagefault.c
> @@ -137,8 +137,13 @@ static int handle_pagefault(struct xe_gt *gt, struct pagefault *pf)
> write_locked = true;
> vma = lookup_vma(vm, pf->page_addr);
> if (!vma) {
> - ret = -EINVAL;
> - goto unlock_vm;
> + if (vm->flags & XE_VM_FLAG_SCRATCH_PAGE)
> + vma = xe_bind_scratch_vma(vm, pf->page_addr, SZ_64K);
I think this would be better.
s/xe_bind_scratch_vma/xe_vm_create_scratch_vma
> +
> + if (!vma) {
> + ret = -EINVAL;
> + goto unlock_vm;
> + }
> }
>
> if (!xe_vma_is_userptr(vma) || !xe_vma_userptr_check_repin(vma)) {
> diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
> index 389ac5ba8ddf..4c3d5d781b58 100644
> --- a/drivers/gpu/drm/xe/xe_vm.c
> +++ b/drivers/gpu/drm/xe/xe_vm.c
> @@ -1262,7 +1262,8 @@ struct xe_vm *xe_vm_create(struct xe_device *xe, u32 flags)
> }
> }
>
> - if (flags & XE_VM_FLAG_SCRATCH_PAGE) {
> + if (flags & XE_VM_FLAG_SCRATCH_PAGE &&
> + (!(flags & XE_VM_FLAG_FAULT_MODE))) {
> for_each_tile(tile, xe, id) {
> if (!vm->pt_root[id])
> continue;
> @@ -1998,10 +1999,6 @@ int xe_vm_create_ioctl(struct drm_device *dev, void *data,
> if (XE_IOCTL_DBG(xe, args->flags & ~ALL_DRM_XE_VM_CREATE_FLAGS))
> return -EINVAL;
>
> - if (XE_IOCTL_DBG(xe, args->flags & DRM_XE_VM_CREATE_SCRATCH_PAGE &&
> - args->flags & DRM_XE_VM_CREATE_FAULT_MODE))
> - return -EINVAL;
> -
> if (XE_IOCTL_DBG(xe, args->flags & DRM_XE_VM_CREATE_COMPUTE_MODE &&
> args->flags & DRM_XE_VM_CREATE_FAULT_MODE))
> return -EINVAL;
> @@ -2783,6 +2780,39 @@ static int __xe_vma_op_execute(struct xe_vm *vm, struct xe_vma *vma,
> return err;
> }
>
> +struct xe_vma *xe_bind_scratch_vma(struct xe_vm *vm, u64 addr, u64 size)
> +{
Nit, the size argument really isn't needed.
> + struct xe_vma *vma = 0;
> +
> + if (!vm->size)
xe_vm_is_closed_or_banned rather than vm->size check.
> + return 0;
Probably a ERR_PTR with correct return codes.
> +
> + vma = xe_vma_create(vm, NULL, 0, addr, addr + size - 1, false, true, 0);
> + if (!vma)
> + return 0;
> + xe_vm_insert_vma(vm, vma);
> +
Need to check the return of xe_vm_insert_vma, also probably WARN on this
as it shouldn't ever fail.
> + /* fault will handle the bind */
> +
> + return vma;
> +}
> +
> +int xe_unbind_scratch_vma(struct xe_vm *vm, u64 addr, u64 range)
> +{
> + struct xe_vma *vma;
> +
> + vma = xe_vm_find_overlapping_vma(vm, addr, range);
> + if (!vma)
> + return -ENXIO;
> +
> + if (xe_vma_is_null(vma)) {
> + prep_vma_destroy(vm, vma, true);
> + xe_vm_unbind(vm, vma, NULL, NULL, 0, NULL, true, false);
> + }
> +
> + return 0;
> +}
> +
> static int xe_vma_op_execute(struct xe_vm *vm, struct xe_vma_op *op)
> {
> int ret = 0;
> @@ -3205,7 +3235,6 @@ int xe_vm_bind_ioctl(struct drm_device *dev, void *data, struct drm_file *file)
> err = vm_bind_ioctl_check_args(xe, args, &bind_ops, &async);
> if (err)
> return err;
> -
Not related.
> if (args->exec_queue_id) {
> q = xe_exec_queue_lookup(xef, args->exec_queue_id);
> if (XE_IOCTL_DBG(xe, !q)) {
> @@ -3352,10 +3381,13 @@ int xe_vm_bind_ioctl(struct drm_device *dev, void *data, struct drm_file *file)
> u64 range = bind_ops[i].range;
> u64 addr = bind_ops[i].addr;
> u32 op = bind_ops[i].op;
> -
> +retry:
> err = vm_bind_ioctl_lookup_vma(vm, bos[i], addr, range, op);
> - if (err)
> + if (err) {
> + if (!xe_unbind_scratch_vma(vm, addr, range))
> + goto retry;
> goto free_syncs;
> + }
You don't need this change, GPUVA handles all of this (e.g. it will
create ops to unbind the old VMA, bind the new one).
Matt
> }
>
> for (i = 0; i < args->num_binds; ++i) {
> diff --git a/drivers/gpu/drm/xe/xe_vm.h b/drivers/gpu/drm/xe/xe_vm.h
> index 6de6e3edb24a..6447bed427b1 100644
> --- a/drivers/gpu/drm/xe/xe_vm.h
> +++ b/drivers/gpu/drm/xe/xe_vm.h
> @@ -212,6 +212,8 @@ int xe_vma_userptr_pin_pages(struct xe_vma *vma);
>
> int xe_vma_userptr_check_repin(struct xe_vma *vma);
>
> +struct xe_vma *xe_bind_scratch_vma(struct xe_vm *vm, u64 addr, u64 size);
> +
> /*
> * XE_ONSTACK_TV is used to size the tv_onstack array that is input
> * to xe_vm_lock_dma_resv() and xe_vm_unlock_dma_resv().
> --
> 2.25.1
>
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [Intel-xe] [PATCH] drm/xe: Enable scratch page when page fault is enabled
2023-08-28 19:22 ` Matthew Brost
@ 2023-08-28 22:34 ` Chang, Yu bruce
2023-08-28 23:44 ` Chang, Yu bruce
0 siblings, 1 reply; 18+ messages in thread
From: Chang, Yu bruce @ 2023-08-28 22:34 UTC (permalink / raw)
To: Brost, Matthew; +Cc: Summers, Stuart, intel-xe@lists.freedesktop.org
> -----Original Message-----
> From: Brost, Matthew <matthew.brost@intel.com>
> Sent: Monday, August 28, 2023 12:22 PM
> To: Chang, Yu bruce <yu.bruce.chang@intel.com>
> Cc: intel-xe@lists.freedesktop.org; Zeng, Oak <oak.zeng@intel.com>; Welty,
> Brian <brian.welty@intel.com>; Vishwanathapura, Niranjana
> <niranjana.vishwanathapura@intel.com>; Summers, Stuart
> <stuart.summers@intel.com>
> Subject: Re: [PATCH] drm/xe: Enable scratch page when page fault is enabled
>
> On Sat, Aug 26, 2023 at 12:14:12AM +0000, Chang, Bruce wrote:
> > i915 can use scratch page even when page fault is enabled, this patch
> > is trying to port this feature over.
> >
>
> I think we need to ask why do we support this before merging this.
> Because the i915 does this is not a valid answer. Please explain why a UMD needs
> this feature.
>
Sure, mainly for EU debugger support and any potential prefetch WA.
Will add this in the commit comment.
> > The current i915 solution changes page table directly which may be
> > hard to make to upstream, so a more complex solution is needed to
> > apply to the current Xe framework if following the existing i915 solution.
> >
> > This patch is trying to make the minimum impact to the existing
> > driver, but still enable the scratch page support.
> >
> > So, the idea is to bind a scratch vma if the page fault is from an
> > invalid access. This patch is taking advantage of null pte at this
> > point, we may introduce a special vma for scratch vma if needed.
> > After the bind, the user app can continue to run without causing a
> > fatal failure or reset and stop.
> >
> > In case the app will bind this scratch vma to a valid address, it will
> > fail the bind, this patch will handle the failre and unbind the scrach
> > vma[s], so that the user binding will be retried to the valid address.
> >
> > This patch only kicks in when there is a failure for both page fault
> > and bind, so it should has no impact to the exiating code path. On
> > another hand, it uses actual page tables instead of special scratch
> > page tables, so it enables potential not to invalidate TLBs when doing
> > unbind if all upper layer page tables are still being used.
> >
> > tested on new scratch igt tests which will be sent out for review.
> >
> > Cc: Oak Zeng <oak.zeng@intel.com>
> > Cc: Brian Welty <brian.welty@intel.com>
> > Cc: Niranjana Vishwanathapura <niranjana.vishwanathapura@intel.com>
> > Cc: Stuart Summers <stuart.summers@intel.com>
> > Cc: Matthew Brost <matthew.brost@intel.com>
> > ---
> > drivers/gpu/drm/xe/xe_gt_pagefault.c | 9 ++++--
> > drivers/gpu/drm/xe/xe_vm.c | 48 +++++++++++++++++++++++-----
> > drivers/gpu/drm/xe/xe_vm.h | 2 ++
> > 3 files changed, 49 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/xe/xe_gt_pagefault.c
> > b/drivers/gpu/drm/xe/xe_gt_pagefault.c
> > index b6f781b3d9d7..524b38df3d7a 100644
> > --- a/drivers/gpu/drm/xe/xe_gt_pagefault.c
> > +++ b/drivers/gpu/drm/xe/xe_gt_pagefault.c
> > @@ -137,8 +137,13 @@ static int handle_pagefault(struct xe_gt *gt, struct
> pagefault *pf)
> > write_locked = true;
> > vma = lookup_vma(vm, pf->page_addr);
> > if (!vma) {
> > - ret = -EINVAL;
> > - goto unlock_vm;
> > + if (vm->flags & XE_VM_FLAG_SCRATCH_PAGE)
> > + vma = xe_bind_scratch_vma(vm, pf->page_addr,
> SZ_64K);
>
> I think this would be better.
>
> s/xe_bind_scratch_vma/xe_vm_create_scratch_vma
>
Will make the change.
>
> > +
> > + if (!vma) {
> > + ret = -EINVAL;
> > + goto unlock_vm;
> > + }
> > }
> >
> > if (!xe_vma_is_userptr(vma) || !xe_vma_userptr_check_repin(vma)) {
> > diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
> > index 389ac5ba8ddf..4c3d5d781b58 100644
> > --- a/drivers/gpu/drm/xe/xe_vm.c
> > +++ b/drivers/gpu/drm/xe/xe_vm.c
> > @@ -1262,7 +1262,8 @@ struct xe_vm *xe_vm_create(struct xe_device *xe,
> u32 flags)
> > }
> > }
> >
> > - if (flags & XE_VM_FLAG_SCRATCH_PAGE) {
> > + if (flags & XE_VM_FLAG_SCRATCH_PAGE &&
> > + (!(flags & XE_VM_FLAG_FAULT_MODE))) {
> > for_each_tile(tile, xe, id) {
> > if (!vm->pt_root[id])
> > continue;
> > @@ -1998,10 +1999,6 @@ int xe_vm_create_ioctl(struct drm_device *dev,
> void *data,
> > if (XE_IOCTL_DBG(xe, args->flags & ~ALL_DRM_XE_VM_CREATE_FLAGS))
> > return -EINVAL;
> >
> > - if (XE_IOCTL_DBG(xe, args->flags &
> DRM_XE_VM_CREATE_SCRATCH_PAGE &&
> > - args->flags & DRM_XE_VM_CREATE_FAULT_MODE))
> > - return -EINVAL;
> > -
> > if (XE_IOCTL_DBG(xe, args->flags &
> DRM_XE_VM_CREATE_COMPUTE_MODE &&
> > args->flags & DRM_XE_VM_CREATE_FAULT_MODE))
> > return -EINVAL;
> > @@ -2783,6 +2780,39 @@ static int __xe_vma_op_execute(struct xe_vm *vm,
> struct xe_vma *vma,
> > return err;
> > }
> >
> > +struct xe_vma *xe_bind_scratch_vma(struct xe_vm *vm, u64 addr, u64
> > +size) {
>
> Nit, the size argument really isn't needed.
>
Sure, can hard code it.
> > + struct xe_vma *vma = 0;
> > +
> > + if (!vm->size)
>
> xe_vm_is_closed_or_banned rather than vm->size check.
>
> > + return 0;
>
> Probably a ERR_PTR with correct return codes.
>
Will add this error check.
> > +
> > + vma = xe_vma_create(vm, NULL, 0, addr, addr + size - 1, false, true, 0);
> > + if (!vma)
> > + return 0;
> > + xe_vm_insert_vma(vm, vma);
> > +
>
> Need to check the return of xe_vm_insert_vma, also probably WARN on this as it
> shouldn't ever fail.
>
Will change it
> > + /* fault will handle the bind */
> > +
> > + return vma;
> > +}
> > +
> > +int xe_unbind_scratch_vma(struct xe_vm *vm, u64 addr, u64 range) {
> > + struct xe_vma *vma;
> > +
> > + vma = xe_vm_find_overlapping_vma(vm, addr, range);
> > + if (!vma)
> > + return -ENXIO;
> > +
> > + if (xe_vma_is_null(vma)) {
> > + prep_vma_destroy(vm, vma, true);
> > + xe_vm_unbind(vm, vma, NULL, NULL, 0, NULL, true, false);
> > + }
> > +
> > + return 0;
> > +}
> > +
> > static int xe_vma_op_execute(struct xe_vm *vm, struct xe_vma_op *op)
> > {
> > int ret = 0;
> > @@ -3205,7 +3235,6 @@ int xe_vm_bind_ioctl(struct drm_device *dev, void
> *data, struct drm_file *file)
> > err = vm_bind_ioctl_check_args(xe, args, &bind_ops, &async);
> > if (err)
> > return err;
> > -
>
> Not related.
>
> > if (args->exec_queue_id) {
> > q = xe_exec_queue_lookup(xef, args->exec_queue_id);
> > if (XE_IOCTL_DBG(xe, !q)) {
> > @@ -3352,10 +3381,13 @@ int xe_vm_bind_ioctl(struct drm_device *dev, void
> *data, struct drm_file *file)
> > u64 range = bind_ops[i].range;
> > u64 addr = bind_ops[i].addr;
> > u32 op = bind_ops[i].op;
> > -
> > +retry:
> > err = vm_bind_ioctl_lookup_vma(vm, bos[i], addr, range, op);
> > - if (err)
> > + if (err) {
> > + if (!xe_unbind_scratch_vma(vm, addr, range))
> > + goto retry;
> > goto free_syncs;
> > + }
>
> You don't need this change, GPUVA handles all of this (e.g. it will create ops to
> unbind the old VMA, bind the new one).
>
> Matt
>
I was also concerned about this change. Good to know GPUVA can help here! Then
the change will be a lot more cleaner.
Thanks!
Bruce
> > }
> >
> > for (i = 0; i < args->num_binds; ++i) { diff --git
> > a/drivers/gpu/drm/xe/xe_vm.h b/drivers/gpu/drm/xe/xe_vm.h index
> > 6de6e3edb24a..6447bed427b1 100644
> > --- a/drivers/gpu/drm/xe/xe_vm.h
> > +++ b/drivers/gpu/drm/xe/xe_vm.h
> > @@ -212,6 +212,8 @@ int xe_vma_userptr_pin_pages(struct xe_vma *vma);
> >
> > int xe_vma_userptr_check_repin(struct xe_vma *vma);
> >
> > +struct xe_vma *xe_bind_scratch_vma(struct xe_vm *vm, u64 addr, u64
> > +size);
> > +
> > /*
> > * XE_ONSTACK_TV is used to size the tv_onstack array that is input
> > * to xe_vm_lock_dma_resv() and xe_vm_unlock_dma_resv().
> > --
> > 2.25.1
> >
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [Intel-xe] [PATCH] drm/xe: Enable scratch page when page fault is enabled
2023-08-28 22:34 ` Chang, Yu bruce
@ 2023-08-28 23:44 ` Chang, Yu bruce
0 siblings, 0 replies; 18+ messages in thread
From: Chang, Yu bruce @ 2023-08-28 23:44 UTC (permalink / raw)
To: Chang, Yu bruce, Brost, Matthew
Cc: Summers, Stuart, intel-xe@lists.freedesktop.org
> -----Original Message-----
> From: Intel-xe <intel-xe-bounces@lists.freedesktop.org> On Behalf Of Chang, Yu
> bruce
> Sent: Monday, August 28, 2023 3:35 PM
> To: Brost, Matthew <matthew.brost@intel.com>
> Cc: Summers, Stuart <stuart.summers@intel.com>; intel-
> xe@lists.freedesktop.org
> Subject: Re: [Intel-xe] [PATCH] drm/xe: Enable scratch page when page fault is
> enabled
>
>
>
> > -----Original Message-----
> > From: Brost, Matthew <matthew.brost@intel.com>
> > Sent: Monday, August 28, 2023 12:22 PM
> > To: Chang, Yu bruce <yu.bruce.chang@intel.com>
> > Cc: intel-xe@lists.freedesktop.org; Zeng, Oak <oak.zeng@intel.com>;
> > Welty, Brian <brian.welty@intel.com>; Vishwanathapura, Niranjana
> > <niranjana.vishwanathapura@intel.com>; Summers, Stuart
> > <stuart.summers@intel.com>
> > Subject: Re: [PATCH] drm/xe: Enable scratch page when page fault is
> > enabled
> >
> > On Sat, Aug 26, 2023 at 12:14:12AM +0000, Chang, Bruce wrote:
> > > i915 can use scratch page even when page fault is enabled, this
> > > patch is trying to port this feature over.
> > >
> >
> > I think we need to ask why do we support this before merging this.
> > Because the i915 does this is not a valid answer. Please explain why a
> > UMD needs this feature.
> >
> Sure, mainly for EU debugger support and any potential prefetch WA.
>
> Will add this in the commit comment.
>
> > > The current i915 solution changes page table directly which may be
> > > hard to make to upstream, so a more complex solution is needed to
> > > apply to the current Xe framework if following the existing i915 solution.
> > >
> > > This patch is trying to make the minimum impact to the existing
> > > driver, but still enable the scratch page support.
> > >
> > > So, the idea is to bind a scratch vma if the page fault is from an
> > > invalid access. This patch is taking advantage of null pte at this
> > > point, we may introduce a special vma for scratch vma if needed.
> > > After the bind, the user app can continue to run without causing a
> > > fatal failure or reset and stop.
> > >
> > > In case the app will bind this scratch vma to a valid address, it
> > > will fail the bind, this patch will handle the failre and unbind the
> > > scrach vma[s], so that the user binding will be retried to the valid address.
> > >
> > > This patch only kicks in when there is a failure for both page fault
> > > and bind, so it should has no impact to the exiating code path. On
> > > another hand, it uses actual page tables instead of special scratch
> > > page tables, so it enables potential not to invalidate TLBs when
> > > doing unbind if all upper layer page tables are still being used.
> > >
> > > tested on new scratch igt tests which will be sent out for review.
> > >
> > > Cc: Oak Zeng <oak.zeng@intel.com>
> > > Cc: Brian Welty <brian.welty@intel.com>
> > > Cc: Niranjana Vishwanathapura <niranjana.vishwanathapura@intel.com>
> > > Cc: Stuart Summers <stuart.summers@intel.com>
> > > Cc: Matthew Brost <matthew.brost@intel.com>
> > > ---
> > > drivers/gpu/drm/xe/xe_gt_pagefault.c | 9 ++++--
> > > drivers/gpu/drm/xe/xe_vm.c | 48 +++++++++++++++++++++++-----
> > > drivers/gpu/drm/xe/xe_vm.h | 2 ++
> > > 3 files changed, 49 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/xe/xe_gt_pagefault.c
> > > b/drivers/gpu/drm/xe/xe_gt_pagefault.c
> > > index b6f781b3d9d7..524b38df3d7a 100644
> > > --- a/drivers/gpu/drm/xe/xe_gt_pagefault.c
> > > +++ b/drivers/gpu/drm/xe/xe_gt_pagefault.c
> > > @@ -137,8 +137,13 @@ static int handle_pagefault(struct xe_gt *gt,
> > > struct
> > pagefault *pf)
> > > write_locked = true;
> > > vma = lookup_vma(vm, pf->page_addr);
> > > if (!vma) {
> > > - ret = -EINVAL;
> > > - goto unlock_vm;
> > > + if (vm->flags & XE_VM_FLAG_SCRATCH_PAGE)
> > > + vma = xe_bind_scratch_vma(vm, pf->page_addr,
> > SZ_64K);
> >
> > I think this would be better.
> >
> > s/xe_bind_scratch_vma/xe_vm_create_scratch_vma
> >
> Will make the change.
>
> >
> > > +
> > > + if (!vma) {
> > > + ret = -EINVAL;
> > > + goto unlock_vm;
> > > + }
> > > }
> > >
> > > if (!xe_vma_is_userptr(vma) || !xe_vma_userptr_check_repin(vma)) {
> > > diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
> > > index 389ac5ba8ddf..4c3d5d781b58 100644
> > > --- a/drivers/gpu/drm/xe/xe_vm.c
> > > +++ b/drivers/gpu/drm/xe/xe_vm.c
> > > @@ -1262,7 +1262,8 @@ struct xe_vm *xe_vm_create(struct xe_device
> > > *xe,
> > u32 flags)
> > > }
> > > }
> > >
> > > - if (flags & XE_VM_FLAG_SCRATCH_PAGE) {
> > > + if (flags & XE_VM_FLAG_SCRATCH_PAGE &&
> > > + (!(flags & XE_VM_FLAG_FAULT_MODE))) {
> > > for_each_tile(tile, xe, id) {
> > > if (!vm->pt_root[id])
> > > continue;
> > > @@ -1998,10 +1999,6 @@ int xe_vm_create_ioctl(struct drm_device
> > > *dev,
> > void *data,
> > > if (XE_IOCTL_DBG(xe, args->flags & ~ALL_DRM_XE_VM_CREATE_FLAGS))
> > > return -EINVAL;
> > >
> > > - if (XE_IOCTL_DBG(xe, args->flags &
> > DRM_XE_VM_CREATE_SCRATCH_PAGE &&
> > > - args->flags & DRM_XE_VM_CREATE_FAULT_MODE))
> > > - return -EINVAL;
> > > -
> > > if (XE_IOCTL_DBG(xe, args->flags &
> > DRM_XE_VM_CREATE_COMPUTE_MODE &&
> > > args->flags & DRM_XE_VM_CREATE_FAULT_MODE))
> > > return -EINVAL;
> > > @@ -2783,6 +2780,39 @@ static int __xe_vma_op_execute(struct xe_vm
> > > *vm,
> > struct xe_vma *vma,
> > > return err;
> > > }
> > >
> > > +struct xe_vma *xe_bind_scratch_vma(struct xe_vm *vm, u64 addr, u64
> > > +size) {
> >
> > Nit, the size argument really isn't needed.
> >
> Sure, can hard code it.
>
> > > + struct xe_vma *vma = 0;
> > > +
> > > + if (!vm->size)
> >
> > xe_vm_is_closed_or_banned rather than vm->size check.
> >
> > > + return 0;
> >
> > Probably a ERR_PTR with correct return codes.
> >
> Will add this error check.
>
> > > +
> > > + vma = xe_vma_create(vm, NULL, 0, addr, addr + size - 1, false, true, 0);
> > > + if (!vma)
> > > + return 0;
> > > + xe_vm_insert_vma(vm, vma);
> > > +
> >
> > Need to check the return of xe_vm_insert_vma, also probably WARN on
> > this as it shouldn't ever fail.
> >
> Will change it
>
> > > + /* fault will handle the bind */
> > > +
> > > + return vma;
> > > +}
> > > +
> > > +int xe_unbind_scratch_vma(struct xe_vm *vm, u64 addr, u64 range) {
> > > + struct xe_vma *vma;
> > > +
> > > + vma = xe_vm_find_overlapping_vma(vm, addr, range);
> > > + if (!vma)
> > > + return -ENXIO;
> > > +
> > > + if (xe_vma_is_null(vma)) {
> > > + prep_vma_destroy(vm, vma, true);
> > > + xe_vm_unbind(vm, vma, NULL, NULL, 0, NULL, true, false);
> > > + }
> > > +
> > > + return 0;
> > > +}
> > > +
> > > static int xe_vma_op_execute(struct xe_vm *vm, struct xe_vma_op
> > > *op) {
> > > int ret = 0;
> > > @@ -3205,7 +3235,6 @@ int xe_vm_bind_ioctl(struct drm_device *dev,
> > > void
> > *data, struct drm_file *file)
> > > err = vm_bind_ioctl_check_args(xe, args, &bind_ops, &async);
> > > if (err)
> > > return err;
> > > -
> >
> > Not related.
> >
> > > if (args->exec_queue_id) {
> > > q = xe_exec_queue_lookup(xef, args->exec_queue_id);
> > > if (XE_IOCTL_DBG(xe, !q)) {
> > > @@ -3352,10 +3381,13 @@ int xe_vm_bind_ioctl(struct drm_device *dev,
> > > void
> > *data, struct drm_file *file)
> > > u64 range = bind_ops[i].range;
> > > u64 addr = bind_ops[i].addr;
> > > u32 op = bind_ops[i].op;
> > > -
> > > +retry:
> > > err = vm_bind_ioctl_lookup_vma(vm, bos[i], addr, range, op);
> > > - if (err)
> > > + if (err) {
> > > + if (!xe_unbind_scratch_vma(vm, addr, range))
> > > + goto retry;
> > > goto free_syncs;
> > > + }
> >
> > You don't need this change, GPUVA handles all of this (e.g. it will
> > create ops to unbind the old VMA, bind the new one).
> >
> > Matt
> >
>
> I was also concerned about this change. Good to know GPUVA can help here!
> Then the change will be a lot more cleaner.
>
> Thanks!
> Bruce
>
It seems
err = vm_bind_ioctl_lookup_vma(vm, bos[i], addr, range, op);
will prevent GPUVA as it will error out firstly. If I remove it, it seems working.
-Bruce
> > > }
> > >
> > > for (i = 0; i < args->num_binds; ++i) { diff --git
> > > a/drivers/gpu/drm/xe/xe_vm.h b/drivers/gpu/drm/xe/xe_vm.h index
> > > 6de6e3edb24a..6447bed427b1 100644
> > > --- a/drivers/gpu/drm/xe/xe_vm.h
> > > +++ b/drivers/gpu/drm/xe/xe_vm.h
> > > @@ -212,6 +212,8 @@ int xe_vma_userptr_pin_pages(struct xe_vma
> > > *vma);
> > >
> > > int xe_vma_userptr_check_repin(struct xe_vma *vma);
> > >
> > > +struct xe_vma *xe_bind_scratch_vma(struct xe_vm *vm, u64 addr, u64
> > > +size);
> > > +
> > > /*
> > > * XE_ONSTACK_TV is used to size the tv_onstack array that is input
> > > * to xe_vm_lock_dma_resv() and xe_vm_unlock_dma_resv().
> > > --
> > > 2.25.1
> > >
^ permalink raw reply [flat|nested] 18+ messages in thread
* [Intel-xe] [PATCH] drm/xe: Enable scratch page when page fault is enabled
@ 2023-08-30 21:34 Chang, Bruce
2023-09-01 0:31 ` Welty, Brian
0 siblings, 1 reply; 18+ messages in thread
From: Chang, Bruce @ 2023-08-30 21:34 UTC (permalink / raw)
To: intel-xe; +Cc: Stuart Summers
The PVC HW has a limitation that the page fault due to invalid access
will halt the corresponding EUs. So, in order to activate the debugger,
kmd needs to setup the scratch pages to unhalt the EUs.
This feature can only be enabled if scratch flag is set per VM. So, once
EU debugger is running, the debugger umd will set the scratch flag,
otherwise, this flag should not be set. So, in regular run, this feature
will not be activated.
The idea is to bind a scratch vma if the page fault is from an
invalid access. This patch is taking advantage of null pte.
After the bind, the user app can continue to run without causing a
fatal failure or reset and stop.
In case the app will bind this scratch vma to a valid address, GPUVA
handles all of this (e.g. it will create ops to unbind the old
VMA, bind the new one).
This patch only kicks in when there is a failure for both page fault
and bind, so it should have no impact to regular code path. On
another hand, it uses actual page tables instead of special scratch
page tables, so it may not require to invalidate TLBs when doing
unbind if all upper layer page tables are still being used.
tested on new scratch igt tests which will be sent out for review.
v2: per Matt's suggestion, remove the scratch page unbind.
v3: correct error handlings.
Cc: Oak Zeng <oak.zeng@intel.com>
Cc: Brian Welty <brian.welty@intel.com>
Cc: Niranjana Vishwanathapura <niranjana.vishwanathapura@intel.com>
Cc: Stuart Summers <stuart.summers@intel.com>
Cc: Matthew Brost <matthew.brost@intel.com>
---
drivers/gpu/drm/xe/xe_gt_pagefault.c | 9 +++++++--
drivers/gpu/drm/xe/xe_vm.c | 30 +++++++++++++++++++++++-----
drivers/gpu/drm/xe/xe_vm.h | 2 ++
3 files changed, 34 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/xe/xe_gt_pagefault.c b/drivers/gpu/drm/xe/xe_gt_pagefault.c
index b6f781b3d9d7..bdd84b109e9e 100644
--- a/drivers/gpu/drm/xe/xe_gt_pagefault.c
+++ b/drivers/gpu/drm/xe/xe_gt_pagefault.c
@@ -137,8 +137,13 @@ static int handle_pagefault(struct xe_gt *gt, struct pagefault *pf)
write_locked = true;
vma = lookup_vma(vm, pf->page_addr);
if (!vma) {
- ret = -EINVAL;
- goto unlock_vm;
+ if (vm->flags & XE_VM_FLAG_SCRATCH_PAGE)
+ vma = xe_vm_create_scratch_vma(vm, pf->page_addr);
+
+ if (IS_ERR_OR_NULL(vma)) {
+ ret = -EINVAL;
+ goto unlock_vm;
+ }
}
if (!xe_vma_is_userptr(vma) || !xe_vma_userptr_check_repin(vma)) {
diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
index 389ac5ba8ddf..58e0309556a0 100644
--- a/drivers/gpu/drm/xe/xe_vm.c
+++ b/drivers/gpu/drm/xe/xe_vm.c
@@ -1262,7 +1262,8 @@ struct xe_vm *xe_vm_create(struct xe_device *xe, u32 flags)
}
}
- if (flags & XE_VM_FLAG_SCRATCH_PAGE) {
+ if (flags & XE_VM_FLAG_SCRATCH_PAGE &&
+ (!(flags & XE_VM_FLAG_FAULT_MODE))) {
for_each_tile(tile, xe, id) {
if (!vm->pt_root[id])
continue;
@@ -1998,10 +1999,6 @@ int xe_vm_create_ioctl(struct drm_device *dev, void *data,
if (XE_IOCTL_DBG(xe, args->flags & ~ALL_DRM_XE_VM_CREATE_FLAGS))
return -EINVAL;
- if (XE_IOCTL_DBG(xe, args->flags & DRM_XE_VM_CREATE_SCRATCH_PAGE &&
- args->flags & DRM_XE_VM_CREATE_FAULT_MODE))
- return -EINVAL;
-
if (XE_IOCTL_DBG(xe, args->flags & DRM_XE_VM_CREATE_COMPUTE_MODE &&
args->flags & DRM_XE_VM_CREATE_FAULT_MODE))
return -EINVAL;
@@ -2783,6 +2780,29 @@ static int __xe_vma_op_execute(struct xe_vm *vm, struct xe_vma *vma,
return err;
}
+struct xe_vma *xe_vm_create_scratch_vma(struct xe_vm *vm, u64 addr)
+{
+ struct xe_vma *vma;
+ int err;
+
+ if (xe_vm_is_closed_or_banned(vm))
+ return ERR_PTR(-ENOENT);
+
+ vma = xe_vma_create(vm, NULL, 0, addr, addr + SZ_64K - 1, false, true, 0);
+ if (IS_ERR_OR_NULL(vma))
+ return vma;
+
+ err = xe_vm_insert_vma(vm, vma);
+ if (err) {
+ xe_vma_destroy_late(vma);
+ return ERR_PTR(err);
+ }
+
+ /* fault will handle the bind */
+
+ return vma;
+}
+
static int xe_vma_op_execute(struct xe_vm *vm, struct xe_vma_op *op)
{
int ret = 0;
diff --git a/drivers/gpu/drm/xe/xe_vm.h b/drivers/gpu/drm/xe/xe_vm.h
index 6de6e3edb24a..ddd387333cd2 100644
--- a/drivers/gpu/drm/xe/xe_vm.h
+++ b/drivers/gpu/drm/xe/xe_vm.h
@@ -212,6 +212,8 @@ int xe_vma_userptr_pin_pages(struct xe_vma *vma);
int xe_vma_userptr_check_repin(struct xe_vma *vma);
+struct xe_vma *xe_vm_create_scratch_vma(struct xe_vm *vm, u64 addr);
+
/*
* XE_ONSTACK_TV is used to size the tv_onstack array that is input
* to xe_vm_lock_dma_resv() and xe_vm_unlock_dma_resv().
--
2.25.1
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [Intel-xe] [PATCH] drm/xe: Enable scratch page when page fault is enabled
2023-08-30 21:34 Chang, Bruce
@ 2023-09-01 0:31 ` Welty, Brian
2023-11-02 21:16 ` Summers, Stuart
0 siblings, 1 reply; 18+ messages in thread
From: Welty, Brian @ 2023-09-01 0:31 UTC (permalink / raw)
To: Chang, Bruce, intel-xe; +Cc: Stuart Summers
On 8/30/2023 2:34 PM, Chang, Bruce wrote:
> The PVC HW has a limitation that the page fault due to invalid access
> will halt the corresponding EUs. So, in order to activate the debugger,
> kmd needs to setup the scratch pages to unhalt the EUs.
>
> This feature can only be enabled if scratch flag is set per VM. So, once
> EU debugger is running, the debugger umd will set the scratch flag,
> otherwise, this flag should not be set. So, in regular run, this feature
> will not be activated.
>
> The idea is to bind a scratch vma if the page fault is from an
> invalid access. This patch is taking advantage of null pte.
> After the bind, the user app can continue to run without causing a
> fatal failure or reset and stop.
>
> In case the app will bind this scratch vma to a valid address, GPUVA
> handles all of this (e.g. it will create ops to unbind the old
> VMA, bind the new one).
>
> This patch only kicks in when there is a failure for both page fault
> and bind, so it should have no impact to regular code path. On
> another hand, it uses actual page tables instead of special scratch
> page tables, so it may not require to invalidate TLBs when doing
> unbind if all upper layer page tables are still being used.
>
> tested on new scratch igt tests which will be sent out for review.
>
> v2: per Matt's suggestion, remove the scratch page unbind.
> v3: correct error handlings.
Looks good to me.
Reviewed-by: Brian Welty <brian.welty@intel.com>
I know Stuart had some mis-givings about merging ahead of EU debugger
support. But you are not adding the scratch feature per se, it's
already in the uAPI. You are just extending it to support fault mode.
Patch is small, so to me, there's enough justification to go ahead
and get this merged now.
>
> Cc: Oak Zeng <oak.zeng@intel.com>
> Cc: Brian Welty <brian.welty@intel.com>
> Cc: Niranjana Vishwanathapura <niranjana.vishwanathapura@intel.com>
> Cc: Stuart Summers <stuart.summers@intel.com>
> Cc: Matthew Brost <matthew.brost@intel.com>
> ---
> drivers/gpu/drm/xe/xe_gt_pagefault.c | 9 +++++++--
> drivers/gpu/drm/xe/xe_vm.c | 30 +++++++++++++++++++++++-----
> drivers/gpu/drm/xe/xe_vm.h | 2 ++
> 3 files changed, 34 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/xe/xe_gt_pagefault.c b/drivers/gpu/drm/xe/xe_gt_pagefault.c
> index b6f781b3d9d7..bdd84b109e9e 100644
> --- a/drivers/gpu/drm/xe/xe_gt_pagefault.c
> +++ b/drivers/gpu/drm/xe/xe_gt_pagefault.c
> @@ -137,8 +137,13 @@ static int handle_pagefault(struct xe_gt *gt, struct pagefault *pf)
> write_locked = true;
> vma = lookup_vma(vm, pf->page_addr);
> if (!vma) {
> - ret = -EINVAL;
> - goto unlock_vm;
> + if (vm->flags & XE_VM_FLAG_SCRATCH_PAGE)
> + vma = xe_vm_create_scratch_vma(vm, pf->page_addr);
> +
> + if (IS_ERR_OR_NULL(vma)) {
> + ret = -EINVAL;
> + goto unlock_vm;
> + }
> }
>
> if (!xe_vma_is_userptr(vma) || !xe_vma_userptr_check_repin(vma)) {
> diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
> index 389ac5ba8ddf..58e0309556a0 100644
> --- a/drivers/gpu/drm/xe/xe_vm.c
> +++ b/drivers/gpu/drm/xe/xe_vm.c
> @@ -1262,7 +1262,8 @@ struct xe_vm *xe_vm_create(struct xe_device *xe, u32 flags)
> }
> }
>
> - if (flags & XE_VM_FLAG_SCRATCH_PAGE) {
> + if (flags & XE_VM_FLAG_SCRATCH_PAGE &&
> + (!(flags & XE_VM_FLAG_FAULT_MODE))) {
> for_each_tile(tile, xe, id) {
> if (!vm->pt_root[id])
> continue;
> @@ -1998,10 +1999,6 @@ int xe_vm_create_ioctl(struct drm_device *dev, void *data,
> if (XE_IOCTL_DBG(xe, args->flags & ~ALL_DRM_XE_VM_CREATE_FLAGS))
> return -EINVAL;
>
> - if (XE_IOCTL_DBG(xe, args->flags & DRM_XE_VM_CREATE_SCRATCH_PAGE &&
> - args->flags & DRM_XE_VM_CREATE_FAULT_MODE))
> - return -EINVAL;
> -
> if (XE_IOCTL_DBG(xe, args->flags & DRM_XE_VM_CREATE_COMPUTE_MODE &&
> args->flags & DRM_XE_VM_CREATE_FAULT_MODE))
> return -EINVAL;
> @@ -2783,6 +2780,29 @@ static int __xe_vma_op_execute(struct xe_vm *vm, struct xe_vma *vma,
> return err;
> }
>
> +struct xe_vma *xe_vm_create_scratch_vma(struct xe_vm *vm, u64 addr)
> +{
> + struct xe_vma *vma;
> + int err;
> +
> + if (xe_vm_is_closed_or_banned(vm))
> + return ERR_PTR(-ENOENT);
> +
> + vma = xe_vma_create(vm, NULL, 0, addr, addr + SZ_64K - 1, false, true, 0);
> + if (IS_ERR_OR_NULL(vma))
> + return vma;
> +
> + err = xe_vm_insert_vma(vm, vma);
> + if (err) {
> + xe_vma_destroy_late(vma);
> + return ERR_PTR(err);
> + }
> +
> + /* fault will handle the bind */
> +
> + return vma;
> +}
> +
> static int xe_vma_op_execute(struct xe_vm *vm, struct xe_vma_op *op)
> {
> int ret = 0;
> diff --git a/drivers/gpu/drm/xe/xe_vm.h b/drivers/gpu/drm/xe/xe_vm.h
> index 6de6e3edb24a..ddd387333cd2 100644
> --- a/drivers/gpu/drm/xe/xe_vm.h
> +++ b/drivers/gpu/drm/xe/xe_vm.h
> @@ -212,6 +212,8 @@ int xe_vma_userptr_pin_pages(struct xe_vma *vma);
>
> int xe_vma_userptr_check_repin(struct xe_vma *vma);
>
> +struct xe_vma *xe_vm_create_scratch_vma(struct xe_vm *vm, u64 addr);
> +
> /*
> * XE_ONSTACK_TV is used to size the tv_onstack array that is input
> * to xe_vm_lock_dma_resv() and xe_vm_unlock_dma_resv().
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [Intel-xe] [PATCH] drm/xe: Enable scratch page when page fault is enabled
2023-09-01 0:31 ` Welty, Brian
@ 2023-11-02 21:16 ` Summers, Stuart
0 siblings, 0 replies; 18+ messages in thread
From: Summers, Stuart @ 2023-11-02 21:16 UTC (permalink / raw)
To: intel-xe@lists.freedesktop.org, Welty, Brian,
yu.bruce.chang@intel.com
On Thu, 2023-08-31 at 17:31 -0700, Welty, Brian wrote:
>
>
> On 8/30/2023 2:34 PM, Chang, Bruce wrote:
> > The PVC HW has a limitation that the page fault due to invalid
> > access
> > will halt the corresponding EUs. So, in order to activate the
> > debugger,
> > kmd needs to setup the scratch pages to unhalt the EUs.
> >
> > This feature can only be enabled if scratch flag is set per VM. So,
> > once
> > EU debugger is running, the debugger umd will set the scratch flag,
> > otherwise, this flag should not be set. So, in regular run, this
> > feature
> > will not be activated.
> >
> > The idea is to bind a scratch vma if the page fault is from an
> > invalid access. This patch is taking advantage of null pte.
> > After the bind, the user app can continue to run without causing a
> > fatal failure or reset and stop.
> >
> > In case the app will bind this scratch vma to a valid address,
> > GPUVA
> > handles all of this (e.g. it will create ops to unbind the old
> > VMA, bind the new one).
> >
> > This patch only kicks in when there is a failure for both page
> > fault
> > and bind, so it should have no impact to regular code path. On
> > another hand, it uses actual page tables instead of special scratch
> > page tables, so it may not require to invalidate TLBs when doing
> > unbind if all upper layer page tables are still being used.
> >
> > tested on new scratch igt tests which will be sent out for review.
> >
> > v2: per Matt's suggestion, remove the scratch page unbind.
> > v3: correct error handlings.
>
> Looks good to me.
> Reviewed-by: Brian Welty <brian.welty@intel.com>
>
> I know Stuart had some mis-givings about merging ahead of EU debugger
> support. But you are not adding the scratch feature per se, it's
> already in the uAPI. You are just extending it to support fault
> mode.
> Patch is small, so to me, there's enough justification to go ahead
> and get this merged now.
Really sorry I missed your reply until now Brian!
My main concern here is adding something that won't be used right away.
Ideally the EU debug feature should available when we merge this.
I know that is actively being worked though so I'm ok with just having
a quick note in the commit message.
Acked-by: Stuart Summers <stuart.summers@intel.com>
>
>
> >
> > Cc: Oak Zeng <oak.zeng@intel.com>
> > Cc: Brian Welty <brian.welty@intel.com>
> > Cc: Niranjana Vishwanathapura <niranjana.vishwanathapura@intel.com>
> > Cc: Stuart Summers <stuart.summers@intel.com>
> > Cc: Matthew Brost <matthew.brost@intel.com>
> > ---
> > drivers/gpu/drm/xe/xe_gt_pagefault.c | 9 +++++++--
> > drivers/gpu/drm/xe/xe_vm.c | 30
> > +++++++++++++++++++++++-----
> > drivers/gpu/drm/xe/xe_vm.h | 2 ++
> > 3 files changed, 34 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/xe/xe_gt_pagefault.c
> > b/drivers/gpu/drm/xe/xe_gt_pagefault.c
> > index b6f781b3d9d7..bdd84b109e9e 100644
> > --- a/drivers/gpu/drm/xe/xe_gt_pagefault.c
> > +++ b/drivers/gpu/drm/xe/xe_gt_pagefault.c
> > @@ -137,8 +137,13 @@ static int handle_pagefault(struct xe_gt *gt,
> > struct pagefault *pf)
> > write_locked = true;
> > vma = lookup_vma(vm, pf->page_addr);
> > if (!vma) {
> > - ret = -EINVAL;
> > - goto unlock_vm;
> > + if (vm->flags & XE_VM_FLAG_SCRATCH_PAGE)
> > + vma = xe_vm_create_scratch_vma(vm, pf-
> > >page_addr);
> > +
> > + if (IS_ERR_OR_NULL(vma)) {
> > + ret = -EINVAL;
> > + goto unlock_vm;
> > + }
> > }
> >
> > if (!xe_vma_is_userptr(vma) ||
> > !xe_vma_userptr_check_repin(vma)) {
> > diff --git a/drivers/gpu/drm/xe/xe_vm.c
> > b/drivers/gpu/drm/xe/xe_vm.c
> > index 389ac5ba8ddf..58e0309556a0 100644
> > --- a/drivers/gpu/drm/xe/xe_vm.c
> > +++ b/drivers/gpu/drm/xe/xe_vm.c
> > @@ -1262,7 +1262,8 @@ struct xe_vm *xe_vm_create(struct xe_device
> > *xe, u32 flags)
> > }
> > }
> >
> > - if (flags & XE_VM_FLAG_SCRATCH_PAGE) {
> > + if (flags & XE_VM_FLAG_SCRATCH_PAGE &&
> > + (!(flags & XE_VM_FLAG_FAULT_MODE))) {
> > for_each_tile(tile, xe, id) {
> > if (!vm->pt_root[id])
> > continue;
> > @@ -1998,10 +1999,6 @@ int xe_vm_create_ioctl(struct drm_device
> > *dev, void *data,
> > if (XE_IOCTL_DBG(xe, args->flags &
> > ~ALL_DRM_XE_VM_CREATE_FLAGS))
> > return -EINVAL;
> >
> > - if (XE_IOCTL_DBG(xe, args->flags &
> > DRM_XE_VM_CREATE_SCRATCH_PAGE &&
> > - args->flags &
> > DRM_XE_VM_CREATE_FAULT_MODE))
> > - return -EINVAL;
> > -
> > if (XE_IOCTL_DBG(xe, args->flags &
> > DRM_XE_VM_CREATE_COMPUTE_MODE &&
> > args->flags &
> > DRM_XE_VM_CREATE_FAULT_MODE))
> > return -EINVAL;
> > @@ -2783,6 +2780,29 @@ static int __xe_vma_op_execute(struct xe_vm
> > *vm, struct xe_vma *vma,
> > return err;
> > }
> >
> > +struct xe_vma *xe_vm_create_scratch_vma(struct xe_vm *vm, u64
> > addr)
> > +{
> > + struct xe_vma *vma;
> > + int err;
> > +
> > + if (xe_vm_is_closed_or_banned(vm))
> > + return ERR_PTR(-ENOENT);
> > +
> > + vma = xe_vma_create(vm, NULL, 0, addr, addr + SZ_64K - 1,
> > false, true, 0);
> > + if (IS_ERR_OR_NULL(vma))
> > + return vma;
> > +
> > + err = xe_vm_insert_vma(vm, vma);
> > + if (err) {
> > + xe_vma_destroy_late(vma);
> > + return ERR_PTR(err);
> > + }
> > +
> > + /* fault will handle the bind */
> > +
> > + return vma;
> > +}
> > +
> > static int xe_vma_op_execute(struct xe_vm *vm, struct xe_vma_op
> > *op)
> > {
> > int ret = 0;
> > diff --git a/drivers/gpu/drm/xe/xe_vm.h
> > b/drivers/gpu/drm/xe/xe_vm.h
> > index 6de6e3edb24a..ddd387333cd2 100644
> > --- a/drivers/gpu/drm/xe/xe_vm.h
> > +++ b/drivers/gpu/drm/xe/xe_vm.h
> > @@ -212,6 +212,8 @@ int xe_vma_userptr_pin_pages(struct xe_vma
> > *vma);
> >
> > int xe_vma_userptr_check_repin(struct xe_vma *vma);
> >
> > +struct xe_vma *xe_vm_create_scratch_vma(struct xe_vm *vm, u64
> > addr);
> > +
> > /*
> > * XE_ONSTACK_TV is used to size the tv_onstack array that is
> > input
> > * to xe_vm_lock_dma_resv() and xe_vm_unlock_dma_resv().
^ permalink raw reply [flat|nested] 18+ messages in thread
* [Intel-xe] [PATCH] drm/xe: Enable scratch page when page fault is enabled
@ 2023-08-29 23:16 Chang, Bruce
2023-08-29 23:58 ` Welty, Brian
0 siblings, 1 reply; 18+ messages in thread
From: Chang, Bruce @ 2023-08-29 23:16 UTC (permalink / raw)
To: intel-xe; +Cc: Stuart Summers
The PVC HW has a limitation that the page fault due to invalid access
will halt the corresponding EUs. So, in order to activate the debugger,
kmd needs to setup the scratch pages to unhalt the EUs.
This feature can only be enabled if scratch flag is set per VM. So, once
EU debugger is running, the debugger umd will set the scratch flag,
otherwise, this flag should not be set. So, in regular run, this feature
will not be activated.
The idea is to bind a scratch vma if the page fault is from an
invalid access. This patch is taking advantage of null pte.
After the bind, the user app can continue to run without causing a
fatal failure or reset and stop.
In case the app will bind this scratch vma to a valid address, GPUVA
handles all of this (e.g. it will create ops to unbind the old
VMA, bind the new one).
This patch only kicks in when there is a failure for both page fault
and bind, so it should have no impact to regular code path. On
another hand, it uses actual page tables instead of special scratch
page tables, so it may not require to invalidate TLBs when doing
unbind if all upper layer page tables are still being used.
tested on new scratch igt tests which will be sent out for review.
v2: per Matt's suggestion, remove the scratch page unbind.
Cc: Oak Zeng <oak.zeng@intel.com>
Cc: Brian Welty <brian.welty@intel.com>
Cc: Niranjana Vishwanathapura <niranjana.vishwanathapura@intel.com>
Cc: Stuart Summers <stuart.summers@intel.com>
Cc: Matthew Brost <matthew.brost@intel.com>
---
drivers/gpu/drm/xe/xe_gt_pagefault.c | 9 +++++++--
drivers/gpu/drm/xe/xe_vm.c | 24 +++++++++++++++++++-----
drivers/gpu/drm/xe/xe_vm.h | 2 ++
3 files changed, 28 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/xe/xe_gt_pagefault.c b/drivers/gpu/drm/xe/xe_gt_pagefault.c
index b6f781b3d9d7..adfd2206b942 100644
--- a/drivers/gpu/drm/xe/xe_gt_pagefault.c
+++ b/drivers/gpu/drm/xe/xe_gt_pagefault.c
@@ -137,8 +137,13 @@ static int handle_pagefault(struct xe_gt *gt, struct pagefault *pf)
write_locked = true;
vma = lookup_vma(vm, pf->page_addr);
if (!vma) {
- ret = -EINVAL;
- goto unlock_vm;
+ if (vm->flags & XE_VM_FLAG_SCRATCH_PAGE)
+ vma = xe_vm_create_scratch_vma(vm, pf->page_addr);
+
+ if (!vma) {
+ ret = -EINVAL;
+ goto unlock_vm;
+ }
}
if (!xe_vma_is_userptr(vma) || !xe_vma_userptr_check_repin(vma)) {
diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
index 389ac5ba8ddf..5190807089e8 100644
--- a/drivers/gpu/drm/xe/xe_vm.c
+++ b/drivers/gpu/drm/xe/xe_vm.c
@@ -1262,7 +1262,8 @@ struct xe_vm *xe_vm_create(struct xe_device *xe, u32 flags)
}
}
- if (flags & XE_VM_FLAG_SCRATCH_PAGE) {
+ if (flags & XE_VM_FLAG_SCRATCH_PAGE &&
+ (!(flags & XE_VM_FLAG_FAULT_MODE))) {
for_each_tile(tile, xe, id) {
if (!vm->pt_root[id])
continue;
@@ -1998,10 +1999,6 @@ int xe_vm_create_ioctl(struct drm_device *dev, void *data,
if (XE_IOCTL_DBG(xe, args->flags & ~ALL_DRM_XE_VM_CREATE_FLAGS))
return -EINVAL;
- if (XE_IOCTL_DBG(xe, args->flags & DRM_XE_VM_CREATE_SCRATCH_PAGE &&
- args->flags & DRM_XE_VM_CREATE_FAULT_MODE))
- return -EINVAL;
-
if (XE_IOCTL_DBG(xe, args->flags & DRM_XE_VM_CREATE_COMPUTE_MODE &&
args->flags & DRM_XE_VM_CREATE_FAULT_MODE))
return -EINVAL;
@@ -2783,6 +2780,23 @@ static int __xe_vma_op_execute(struct xe_vm *vm, struct xe_vma *vma,
return err;
}
+struct xe_vma *xe_vm_create_scratch_vma(struct xe_vm *vm, u64 addr)
+{
+ struct xe_vma *vma = 0;
+
+ if (xe_vm_is_closed_or_banned(vm))
+ return ERR_PTR(-ENOENT);
+
+ vma = xe_vma_create(vm, NULL, 0, addr, addr + SZ_64K - 1, false, true, 0);
+ if (!vma)
+ return 0;
+ XE_WARN_ON(xe_vm_insert_vma(vm, vma));
+
+ /* fault will handle the bind */
+
+ return vma;
+}
+
static int xe_vma_op_execute(struct xe_vm *vm, struct xe_vma_op *op)
{
int ret = 0;
diff --git a/drivers/gpu/drm/xe/xe_vm.h b/drivers/gpu/drm/xe/xe_vm.h
index 6de6e3edb24a..ddd387333cd2 100644
--- a/drivers/gpu/drm/xe/xe_vm.h
+++ b/drivers/gpu/drm/xe/xe_vm.h
@@ -212,6 +212,8 @@ int xe_vma_userptr_pin_pages(struct xe_vma *vma);
int xe_vma_userptr_check_repin(struct xe_vma *vma);
+struct xe_vma *xe_vm_create_scratch_vma(struct xe_vm *vm, u64 addr);
+
/*
* XE_ONSTACK_TV is used to size the tv_onstack array that is input
* to xe_vm_lock_dma_resv() and xe_vm_unlock_dma_resv().
--
2.25.1
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [Intel-xe] [PATCH] drm/xe: Enable scratch page when page fault is enabled
2023-08-29 23:16 Chang, Bruce
@ 2023-08-29 23:58 ` Welty, Brian
2023-08-30 21:37 ` Chang, Yu bruce
0 siblings, 1 reply; 18+ messages in thread
From: Welty, Brian @ 2023-08-29 23:58 UTC (permalink / raw)
To: Chang, Bruce, intel-xe; +Cc: Stuart Summers
On 8/29/2023 4:16 PM, Chang, Bruce wrote:
> The PVC HW has a limitation that the page fault due to invalid access
> will halt the corresponding EUs. So, in order to activate the debugger,
> kmd needs to setup the scratch pages to unhalt the EUs.
>
> This feature can only be enabled if scratch flag is set per VM. So, once
> EU debugger is running, the debugger umd will set the scratch flag,
> otherwise, this flag should not be set. So, in regular run, this feature
> will not be activated.
>
> The idea is to bind a scratch vma if the page fault is from an
> invalid access. This patch is taking advantage of null pte.
> After the bind, the user app can continue to run without causing a
> fatal failure or reset and stop.
>
> In case the app will bind this scratch vma to a valid address, GPUVA
> handles all of this (e.g. it will create ops to unbind the old
> VMA, bind the new one).
>
> This patch only kicks in when there is a failure for both page fault
> and bind, so it should have no impact to regular code path. On
> another hand, it uses actual page tables instead of special scratch
> page tables, so it may not require to invalidate TLBs when doing
> unbind if all upper layer page tables are still being used.
>
> tested on new scratch igt tests which will be sent out for review.
>
> v2: per Matt's suggestion, remove the scratch page unbind.
>
> Cc: Oak Zeng <oak.zeng@intel.com>
> Cc: Brian Welty <brian.welty@intel.com>
> Cc: Niranjana Vishwanathapura <niranjana.vishwanathapura@intel.com>
> Cc: Stuart Summers <stuart.summers@intel.com>
> Cc: Matthew Brost <matthew.brost@intel.com>
> ---
> drivers/gpu/drm/xe/xe_gt_pagefault.c | 9 +++++++--
> drivers/gpu/drm/xe/xe_vm.c | 24 +++++++++++++++++++-----
> drivers/gpu/drm/xe/xe_vm.h | 2 ++
> 3 files changed, 28 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/xe/xe_gt_pagefault.c b/drivers/gpu/drm/xe/xe_gt_pagefault.c
> index b6f781b3d9d7..adfd2206b942 100644
> --- a/drivers/gpu/drm/xe/xe_gt_pagefault.c
> +++ b/drivers/gpu/drm/xe/xe_gt_pagefault.c
> @@ -137,8 +137,13 @@ static int handle_pagefault(struct xe_gt *gt, struct pagefault *pf)
> write_locked = true;
> vma = lookup_vma(vm, pf->page_addr);
> if (!vma) {
> - ret = -EINVAL;
> - goto unlock_vm;
> + if (vm->flags & XE_VM_FLAG_SCRATCH_PAGE)
> + vma = xe_vm_create_scratch_vma(vm, pf->page_addr);
> +
> + if (!vma) {
Looks like vma could have an embedded error. So maybe need
IS_ERR_OR_NULL() ?
> + ret = -EINVAL;
> + goto unlock_vm;
> + }
> }
>
> if (!xe_vma_is_userptr(vma) || !xe_vma_userptr_check_repin(vma)) {
> diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
> index 389ac5ba8ddf..5190807089e8 100644
> --- a/drivers/gpu/drm/xe/xe_vm.c
> +++ b/drivers/gpu/drm/xe/xe_vm.c
> @@ -1262,7 +1262,8 @@ struct xe_vm *xe_vm_create(struct xe_device *xe, u32 flags)
> }
> }
>
> - if (flags & XE_VM_FLAG_SCRATCH_PAGE) {
> + if (flags & XE_VM_FLAG_SCRATCH_PAGE &&
> + (!(flags & XE_VM_FLAG_FAULT_MODE))) {
> for_each_tile(tile, xe, id) {
> if (!vm->pt_root[id])
> continue;
> @@ -1998,10 +1999,6 @@ int xe_vm_create_ioctl(struct drm_device *dev, void *data,
> if (XE_IOCTL_DBG(xe, args->flags & ~ALL_DRM_XE_VM_CREATE_FLAGS))
> return -EINVAL;
>
> - if (XE_IOCTL_DBG(xe, args->flags & DRM_XE_VM_CREATE_SCRATCH_PAGE &&
> - args->flags & DRM_XE_VM_CREATE_FAULT_MODE))
> - return -EINVAL;
> -
> if (XE_IOCTL_DBG(xe, args->flags & DRM_XE_VM_CREATE_COMPUTE_MODE &&
> args->flags & DRM_XE_VM_CREATE_FAULT_MODE))
> return -EINVAL;
> @@ -2783,6 +2780,23 @@ static int __xe_vma_op_execute(struct xe_vm *vm, struct xe_vma *vma,
> return err;
> }
>
> +struct xe_vma *xe_vm_create_scratch_vma(struct xe_vm *vm, u64 addr)
> +{
> + struct xe_vma *vma = 0;
> +
> + if (xe_vm_is_closed_or_banned(vm))
> + return ERR_PTR(-ENOENT);
> +
> + vma = xe_vma_create(vm, NULL, 0, addr, addr + SZ_64K - 1, false, true, 0);
> + if (!vma)
> + return 0;
> + XE_WARN_ON(xe_vm_insert_vma(vm, vma));
I think might as well return the error to caller here?
(let the caller decide to ignore it or not).
And on error, seems best to call xe_vma_destroy_late() to free the
memory allocated for vma structure.
> +
> + /* fault will handle the bind */
> +
> + return vma;
> +}
> +
> static int xe_vma_op_execute(struct xe_vm *vm, struct xe_vma_op *op)
> {
> int ret = 0;
> diff --git a/drivers/gpu/drm/xe/xe_vm.h b/drivers/gpu/drm/xe/xe_vm.h
> index 6de6e3edb24a..ddd387333cd2 100644
> --- a/drivers/gpu/drm/xe/xe_vm.h
> +++ b/drivers/gpu/drm/xe/xe_vm.h
> @@ -212,6 +212,8 @@ int xe_vma_userptr_pin_pages(struct xe_vma *vma);
>
> int xe_vma_userptr_check_repin(struct xe_vma *vma);
>
> +struct xe_vma *xe_vm_create_scratch_vma(struct xe_vm *vm, u64 addr);
> +
> /*
> * XE_ONSTACK_TV is used to size the tv_onstack array that is input
> * to xe_vm_lock_dma_resv() and xe_vm_unlock_dma_resv().
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [Intel-xe] [PATCH] drm/xe: Enable scratch page when page fault is enabled
2023-08-29 23:58 ` Welty, Brian
@ 2023-08-30 21:37 ` Chang, Yu bruce
0 siblings, 0 replies; 18+ messages in thread
From: Chang, Yu bruce @ 2023-08-30 21:37 UTC (permalink / raw)
To: Welty, Brian, intel-xe@lists.freedesktop.org; +Cc: Summers, Stuart
> -----Original Message-----
> From: Welty, Brian <brian.welty@intel.com>
> Sent: Tuesday, August 29, 2023 4:59 PM
> To: Chang, Yu bruce <yu.bruce.chang@intel.com>; intel-
> xe@lists.freedesktop.org
> Cc: Zeng, Oak <oak.zeng@intel.com>; Vishwanathapura, Niranjana
> <niranjana.vishwanathapura@intel.com>; Summers, Stuart
> <stuart.summers@intel.com>; Brost, Matthew <matthew.brost@intel.com>
> Subject: Re: [PATCH] drm/xe: Enable scratch page when page fault is enabled
>
>
>
> On 8/29/2023 4:16 PM, Chang, Bruce wrote:
> > The PVC HW has a limitation that the page fault due to invalid access
> > will halt the corresponding EUs. So, in order to activate the debugger,
> > kmd needs to setup the scratch pages to unhalt the EUs.
> >
> > This feature can only be enabled if scratch flag is set per VM. So, once
> > EU debugger is running, the debugger umd will set the scratch flag,
> > otherwise, this flag should not be set. So, in regular run, this feature
> > will not be activated.
> >
> > The idea is to bind a scratch vma if the page fault is from an
> > invalid access. This patch is taking advantage of null pte.
> > After the bind, the user app can continue to run without causing a
> > fatal failure or reset and stop.
> >
> > In case the app will bind this scratch vma to a valid address, GPUVA
> > handles all of this (e.g. it will create ops to unbind the old
> > VMA, bind the new one).
> >
> > This patch only kicks in when there is a failure for both page fault
> > and bind, so it should have no impact to regular code path. On
> > another hand, it uses actual page tables instead of special scratch
> > page tables, so it may not require to invalidate TLBs when doing
> > unbind if all upper layer page tables are still being used.
> >
> > tested on new scratch igt tests which will be sent out for review.
> >
> > v2: per Matt's suggestion, remove the scratch page unbind.
> >
> > Cc: Oak Zeng <oak.zeng@intel.com>
> > Cc: Brian Welty <brian.welty@intel.com>
> > Cc: Niranjana Vishwanathapura <niranjana.vishwanathapura@intel.com>
> > Cc: Stuart Summers <stuart.summers@intel.com>
> > Cc: Matthew Brost <matthew.brost@intel.com>
> > ---
> > drivers/gpu/drm/xe/xe_gt_pagefault.c | 9 +++++++--
> > drivers/gpu/drm/xe/xe_vm.c | 24 +++++++++++++++++++-----
> > drivers/gpu/drm/xe/xe_vm.h | 2 ++
> > 3 files changed, 28 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/xe/xe_gt_pagefault.c
> b/drivers/gpu/drm/xe/xe_gt_pagefault.c
> > index b6f781b3d9d7..adfd2206b942 100644
> > --- a/drivers/gpu/drm/xe/xe_gt_pagefault.c
> > +++ b/drivers/gpu/drm/xe/xe_gt_pagefault.c
> > @@ -137,8 +137,13 @@ static int handle_pagefault(struct xe_gt *gt, struct
> pagefault *pf)
> > write_locked = true;
> > vma = lookup_vma(vm, pf->page_addr);
> > if (!vma) {
> > - ret = -EINVAL;
> > - goto unlock_vm;
> > + if (vm->flags & XE_VM_FLAG_SCRATCH_PAGE)
> > + vma = xe_vm_create_scratch_vma(vm, pf-
> >page_addr);
> > +
> > + if (!vma) {
>
> Looks like vma could have an embedded error. So maybe need
> IS_ERR_OR_NULL() ?
>
Good catch! Fixed now.
> > + ret = -EINVAL;
> > + goto unlock_vm;
> > + }
> > }
> >
> > if (!xe_vma_is_userptr(vma) || !xe_vma_userptr_check_repin(vma)) {
> > diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
> > index 389ac5ba8ddf..5190807089e8 100644
> > --- a/drivers/gpu/drm/xe/xe_vm.c
> > +++ b/drivers/gpu/drm/xe/xe_vm.c
> > @@ -1262,7 +1262,8 @@ struct xe_vm *xe_vm_create(struct xe_device
> *xe, u32 flags)
> > }
> > }
> >
> > - if (flags & XE_VM_FLAG_SCRATCH_PAGE) {
> > + if (flags & XE_VM_FLAG_SCRATCH_PAGE &&
> > + (!(flags & XE_VM_FLAG_FAULT_MODE))) {
> > for_each_tile(tile, xe, id) {
> > if (!vm->pt_root[id])
> > continue;
> > @@ -1998,10 +1999,6 @@ int xe_vm_create_ioctl(struct drm_device *dev,
> void *data,
> > if (XE_IOCTL_DBG(xe, args->flags &
> ~ALL_DRM_XE_VM_CREATE_FLAGS))
> > return -EINVAL;
> >
> > - if (XE_IOCTL_DBG(xe, args->flags &
> DRM_XE_VM_CREATE_SCRATCH_PAGE &&
> > - args->flags & DRM_XE_VM_CREATE_FAULT_MODE))
> > - return -EINVAL;
> > -
> > if (XE_IOCTL_DBG(xe, args->flags &
> DRM_XE_VM_CREATE_COMPUTE_MODE &&
> > args->flags & DRM_XE_VM_CREATE_FAULT_MODE))
> > return -EINVAL;
> > @@ -2783,6 +2780,23 @@ static int __xe_vma_op_execute(struct xe_vm
> *vm, struct xe_vma *vma,
> > return err;
> > }
> >
> > +struct xe_vma *xe_vm_create_scratch_vma(struct xe_vm *vm, u64 addr)
> > +{
> > + struct xe_vma *vma = 0;
> > +
> > + if (xe_vm_is_closed_or_banned(vm))
> > + return ERR_PTR(-ENOENT);
> > +
> > + vma = xe_vma_create(vm, NULL, 0, addr, addr + SZ_64K - 1, false,
> true, 0);
> > + if (!vma)
> > + return 0;
> > + XE_WARN_ON(xe_vm_insert_vma(vm, vma));
>
> I think might as well return the error to caller here?
> (let the caller decide to ignore it or not).
> And on error, seems best to call xe_vma_destroy_late() to free the
> memory allocated for vma structure.
>
Good suggestions! Made the change and also removed the XE_WARN_ON
Since xe_vm_insert_vma already had it.
Thanks,
Bruce
>
> > +
> > + /* fault will handle the bind */
> > +
> > + return vma;
> > +}
> > +
> > static int xe_vma_op_execute(struct xe_vm *vm, struct xe_vma_op *op)
> > {
> > int ret = 0;
> > diff --git a/drivers/gpu/drm/xe/xe_vm.h b/drivers/gpu/drm/xe/xe_vm.h
> > index 6de6e3edb24a..ddd387333cd2 100644
> > --- a/drivers/gpu/drm/xe/xe_vm.h
> > +++ b/drivers/gpu/drm/xe/xe_vm.h
> > @@ -212,6 +212,8 @@ int xe_vma_userptr_pin_pages(struct xe_vma
> *vma);
> >
> > int xe_vma_userptr_check_repin(struct xe_vma *vma);
> >
> > +struct xe_vma *xe_vm_create_scratch_vma(struct xe_vm *vm, u64 addr);
> > +
> > /*
> > * XE_ONSTACK_TV is used to size the tv_onstack array that is input
> > * to xe_vm_lock_dma_resv() and xe_vm_unlock_dma_resv().
^ permalink raw reply [flat|nested] 18+ messages in thread
* [Intel-xe] [PATCH] drm/xe: Enable scratch page when page fault is enabled
@ 2023-07-10 22:06 Chang, Bruce
2023-07-11 2:11 ` Zeng, Oak
0 siblings, 1 reply; 18+ messages in thread
From: Chang, Bruce @ 2023-07-10 22:06 UTC (permalink / raw)
To: intel-xe; +Cc: Oak Zeng, Stuart Summers
This is just a proposal and not fully tested yet. Once it is got
agreenment, more efforts needed to make it fully working, including igt
test changes.
i915 can use scratch page even when page fault is enabled, this patch
is trying to port this feature over.
The current i915 solution changes page table directly which may be hard to
make to upstream, so a more complex solution is needed to apply to the
current Xe framework if following the existing i915 solution.
This patch is trying to make the minimum impact to the existing driver,
but still enable the scratch page support.
So, the idea is to bind a scratch vma if the page fault is from an invalid
access. This patch is taking advantage of null pte at this point, we may
introduce a special vma for scratch vma if needed. After the bind, the user
app can continue to run without causing a fatal failure or reset and
stop.
In case the app will bind this scratch vma to a valid address, it will
fail the bind, this patch will handle the failre and unbind the scrach
vma[s], so that the user binding will be retried to the valid address.
This patch only kicks in when there is a failure for both page fault
and bind, so it should has no impact to the exiating code path. On
another hand, it uses actual page tables instead of special scratch
page tables, so it enables potential not to invalidate TLBs when doing
unbind if all upper layer page tables are still being used.
Cc: Oak Zeng <oak.zeng@intel.com>
Cc: Brian Welty <brian.welty@intel.com>
Cc: Niranjana Vishwanathapura <niranjana.vishwanathapura@intel.com>
Cc: Stuart Summers <stuart.summers@intel.com>
Cc: Matthew Brost <matthew.brost@intel.com>
Signed-off-by: Bruce Chang <yu.bruce.chang@intel.com>
---
drivers/gpu/drm/xe/xe_gt_pagefault.c | 8 +++--
drivers/gpu/drm/xe/xe_vm.c | 52 ++++++++++++++++++++++++----
drivers/gpu/drm/xe/xe_vm.h | 2 ++
3 files changed, 53 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/xe/xe_gt_pagefault.c b/drivers/gpu/drm/xe/xe_gt_pagefault.c
index 6faebd02f3fb..01d316baf0e4 100644
--- a/drivers/gpu/drm/xe/xe_gt_pagefault.c
+++ b/drivers/gpu/drm/xe/xe_gt_pagefault.c
@@ -138,8 +138,12 @@ static int handle_pagefault(struct xe_gt *gt, struct pagefault *pf)
write_locked = true;
vma = lookup_vma(vm, pf->page_addr);
if (!vma) {
- ret = -EINVAL;
- goto unlock_vm;
+ if (vm->flags & XE_VM_FLAG_SCRATCH_PAGE)
+ ret = xe_bind_scratch_vma(vm, pf->page_addr, SZ_64K);
+ else
+ ret = -EINVAL;
+ if (ret)
+ goto unlock_vm;
}
if (!xe_vma_is_userptr(vma) || !xe_vma_userptr_check_repin(vma)) {
diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
index dbff75a9087b..dc2d0cdfb0dc 100644
--- a/drivers/gpu/drm/xe/xe_vm.c
+++ b/drivers/gpu/drm/xe/xe_vm.c
@@ -1241,7 +1241,8 @@ struct xe_vm *xe_vm_create(struct xe_device *xe, u32 flags)
}
}
- if (flags & XE_VM_FLAG_SCRATCH_PAGE) {
+ if (flags & XE_VM_FLAG_SCRATCH_PAGE &&
+ (!(flags & XE_VM_FLAG_FAULT_MODE))) {
for_each_tile(tile, xe, id) {
if (!vm->pt_root[id])
continue;
@@ -1931,10 +1932,6 @@ int xe_vm_create_ioctl(struct drm_device *dev, void *data,
if (XE_IOCTL_ERR(xe, args->flags & ~ALL_DRM_XE_VM_CREATE_FLAGS))
return -EINVAL;
- if (XE_IOCTL_ERR(xe, args->flags & DRM_XE_VM_CREATE_SCRATCH_PAGE &&
- args->flags & DRM_XE_VM_CREATE_FAULT_MODE))
- return -EINVAL;
-
if (XE_IOCTL_ERR(xe, args->flags & DRM_XE_VM_CREATE_COMPUTE_MODE &&
args->flags & DRM_XE_VM_CREATE_FAULT_MODE))
return -EINVAL;
@@ -2603,6 +2600,44 @@ static int prep_replacement_vma(struct xe_vm *vm, struct xe_vma *vma)
return 0;
}
+int xe_bind_scratch_vma(struct xe_vm *vm, u64 addr, u64 size)
+{
+ struct xe_vma *vma;
+
+ if (!vm->size)
+ return -ENOMEM;
+
+ vma = xe_vma_create(vm, NULL, 0, addr, addr + size - 1, false, true, 0);
+ if (!vma)
+ return -ENOMEM;
+ xe_vm_insert_vma(vm, vma);
+
+ /*
+ * fault will handle the bind
+ */
+
+ return 0;
+}
+
+int xe_unbind_scratch_vma(struct xe_vm *vm, u64 addr, u64 range)
+{
+ struct xe_vma *vma, lookup;
+
+ lookup.start = addr;
+ lookup.end = addr + range - 1;
+
+ vma = xe_vm_find_overlapping_vma(vm, &lookup);
+ if (!vma)
+ return -ENXIO;
+
+ if (xe_vma_is_null(vma)) {
+ prep_vma_destroy(vm, vma);
+ xe_vm_unbind(vm, vma, NULL, NULL, 0, NULL);
+ }
+
+ return 0;
+}
+
/*
* Find all overlapping VMAs in lookup range and add to a list in the returned
* VMA, all of VMAs found will be unbound. Also possibly add 2 new VMAs that
@@ -3220,10 +3255,13 @@ int xe_vm_bind_ioctl(struct drm_device *dev, void *data, struct drm_file *file)
u64 range = bind_ops[i].range;
u64 addr = bind_ops[i].addr;
u32 op = bind_ops[i].op;
-
+retry:
err = __vm_bind_ioctl_lookup_vma(vm, bos[i], addr, range, op);
- if (err)
+ if (err) {
+ if (!xe_unbind_scratch_vma(vm, addr, range))
+ goto retry;
goto release_vm_lock;
+ }
}
for (i = 0; i < args->num_binds; ++i) {
diff --git a/drivers/gpu/drm/xe/xe_vm.h b/drivers/gpu/drm/xe/xe_vm.h
index 5edb7771629c..74f99b38f60d 100644
--- a/drivers/gpu/drm/xe/xe_vm.h
+++ b/drivers/gpu/drm/xe/xe_vm.h
@@ -140,6 +140,8 @@ static inline void xe_vm_queue_rebind_worker(struct xe_vm *vm)
queue_work(vm->xe->ordered_wq, &vm->preempt.rebind_work);
}
+int xe_bind_scratch_vma(struct xe_vm *vm, u64 addr, u64 size);
+
/*
* XE_ONSTACK_TV is used to size the tv_onstack array that is input
* to xe_vm_lock_dma_resv() and xe_vm_unlock_dma_resv().
--
2.25.1
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [Intel-xe] [PATCH] drm/xe: Enable scratch page when page fault is enabled
2023-07-10 22:06 Chang, Bruce
@ 2023-07-11 2:11 ` Zeng, Oak
0 siblings, 0 replies; 18+ messages in thread
From: Zeng, Oak @ 2023-07-11 2:11 UTC (permalink / raw)
To: Chang, Yu bruce, intel-xe@lists.freedesktop.org; +Cc: Summers, Stuart
Hi Bruce,
Why do we want to enable scratch page on xe driver? Mapping whole process address space to scratch page or mapping invalid (aka out of bound) GPU virtual address to scratch page can hide application programming bug. It can even hide kmd or GPU HW bug. My philosophy is, when there is a bug, fail the application immediately and let user know the problem immediately. Hiding issues fails us for long run.
On I915, this feature has caused us a lot of troubles. Once it is enabled, application can depend on it so it is very hard to remove. As you know our plan is to disable scratch page on i915. So I don't understand why we want enable this "feature" in the first place.
Thanks,
Oak
> -----Original Message-----
> From: Chang, Yu bruce <yu.bruce.chang@intel.com>
> Sent: July 10, 2023 6:07 PM
> To: intel-xe@lists.freedesktop.org
> Cc: Chang, Yu bruce <yu.bruce.chang@intel.com>; Zeng, Oak
> <oak.zeng@intel.com>; Welty, Brian <brian.welty@intel.com>; Vishwanathapura,
> Niranjana <niranjana.vishwanathapura@intel.com>; Summers, Stuart
> <stuart.summers@intel.com>; Brost, Matthew <matthew.brost@intel.com>
> Subject: [PATCH] drm/xe: Enable scratch page when page fault is enabled
>
> This is just a proposal and not fully tested yet. Once it is got
> agreenment, more efforts needed to make it fully working, including igt
> test changes.
>
> i915 can use scratch page even when page fault is enabled, this patch
> is trying to port this feature over.
>
> The current i915 solution changes page table directly which may be hard to
> make to upstream, so a more complex solution is needed to apply to the
> current Xe framework if following the existing i915 solution.
>
> This patch is trying to make the minimum impact to the existing driver,
> but still enable the scratch page support.
>
> So, the idea is to bind a scratch vma if the page fault is from an invalid
> access. This patch is taking advantage of null pte at this point, we may
> introduce a special vma for scratch vma if needed. After the bind, the user
> app can continue to run without causing a fatal failure or reset and
> stop.
>
> In case the app will bind this scratch vma to a valid address, it will
> fail the bind, this patch will handle the failre and unbind the scrach
> vma[s], so that the user binding will be retried to the valid address.
>
> This patch only kicks in when there is a failure for both page fault
> and bind, so it should has no impact to the exiating code path. On
> another hand, it uses actual page tables instead of special scratch
> page tables, so it enables potential not to invalidate TLBs when doing
> unbind if all upper layer page tables are still being used.
>
> Cc: Oak Zeng <oak.zeng@intel.com>
> Cc: Brian Welty <brian.welty@intel.com>
> Cc: Niranjana Vishwanathapura <niranjana.vishwanathapura@intel.com>
> Cc: Stuart Summers <stuart.summers@intel.com>
> Cc: Matthew Brost <matthew.brost@intel.com>
> Signed-off-by: Bruce Chang <yu.bruce.chang@intel.com>
> ---
> drivers/gpu/drm/xe/xe_gt_pagefault.c | 8 +++--
> drivers/gpu/drm/xe/xe_vm.c | 52 ++++++++++++++++++++++++----
> drivers/gpu/drm/xe/xe_vm.h | 2 ++
> 3 files changed, 53 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/xe/xe_gt_pagefault.c
> b/drivers/gpu/drm/xe/xe_gt_pagefault.c
> index 6faebd02f3fb..01d316baf0e4 100644
> --- a/drivers/gpu/drm/xe/xe_gt_pagefault.c
> +++ b/drivers/gpu/drm/xe/xe_gt_pagefault.c
> @@ -138,8 +138,12 @@ static int handle_pagefault(struct xe_gt *gt, struct
> pagefault *pf)
> write_locked = true;
> vma = lookup_vma(vm, pf->page_addr);
> if (!vma) {
> - ret = -EINVAL;
> - goto unlock_vm;
> + if (vm->flags & XE_VM_FLAG_SCRATCH_PAGE)
> + ret = xe_bind_scratch_vma(vm, pf->page_addr, SZ_64K);
> + else
> + ret = -EINVAL;
> + if (ret)
> + goto unlock_vm;
> }
>
> if (!xe_vma_is_userptr(vma) || !xe_vma_userptr_check_repin(vma)) {
> diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
> index dbff75a9087b..dc2d0cdfb0dc 100644
> --- a/drivers/gpu/drm/xe/xe_vm.c
> +++ b/drivers/gpu/drm/xe/xe_vm.c
> @@ -1241,7 +1241,8 @@ struct xe_vm *xe_vm_create(struct xe_device *xe, u32
> flags)
> }
> }
>
> - if (flags & XE_VM_FLAG_SCRATCH_PAGE) {
> + if (flags & XE_VM_FLAG_SCRATCH_PAGE &&
> + (!(flags & XE_VM_FLAG_FAULT_MODE))) {
> for_each_tile(tile, xe, id) {
> if (!vm->pt_root[id])
> continue;
> @@ -1931,10 +1932,6 @@ int xe_vm_create_ioctl(struct drm_device *dev, void
> *data,
> if (XE_IOCTL_ERR(xe, args->flags & ~ALL_DRM_XE_VM_CREATE_FLAGS))
> return -EINVAL;
>
> - if (XE_IOCTL_ERR(xe, args->flags &
> DRM_XE_VM_CREATE_SCRATCH_PAGE &&
> - args->flags & DRM_XE_VM_CREATE_FAULT_MODE))
> - return -EINVAL;
> -
> if (XE_IOCTL_ERR(xe, args->flags &
> DRM_XE_VM_CREATE_COMPUTE_MODE &&
> args->flags & DRM_XE_VM_CREATE_FAULT_MODE))
> return -EINVAL;
> @@ -2603,6 +2600,44 @@ static int prep_replacement_vma(struct xe_vm *vm,
> struct xe_vma *vma)
> return 0;
> }
>
> +int xe_bind_scratch_vma(struct xe_vm *vm, u64 addr, u64 size)
> +{
> + struct xe_vma *vma;
> +
> + if (!vm->size)
> + return -ENOMEM;
> +
> + vma = xe_vma_create(vm, NULL, 0, addr, addr + size - 1, false, true, 0);
> + if (!vma)
> + return -ENOMEM;
> + xe_vm_insert_vma(vm, vma);
> +
> + /*
> + * fault will handle the bind
> + */
> +
> + return 0;
> +}
> +
> +int xe_unbind_scratch_vma(struct xe_vm *vm, u64 addr, u64 range)
> +{
> + struct xe_vma *vma, lookup;
> +
> + lookup.start = addr;
> + lookup.end = addr + range - 1;
> +
> + vma = xe_vm_find_overlapping_vma(vm, &lookup);
> + if (!vma)
> + return -ENXIO;
> +
> + if (xe_vma_is_null(vma)) {
> + prep_vma_destroy(vm, vma);
> + xe_vm_unbind(vm, vma, NULL, NULL, 0, NULL);
> + }
> +
> + return 0;
> +}
> +
> /*
> * Find all overlapping VMAs in lookup range and add to a list in the returned
> * VMA, all of VMAs found will be unbound. Also possibly add 2 new VMAs that
> @@ -3220,10 +3255,13 @@ int xe_vm_bind_ioctl(struct drm_device *dev, void
> *data, struct drm_file *file)
> u64 range = bind_ops[i].range;
> u64 addr = bind_ops[i].addr;
> u32 op = bind_ops[i].op;
> -
> +retry:
> err = __vm_bind_ioctl_lookup_vma(vm, bos[i], addr, range, op);
> - if (err)
> + if (err) {
> + if (!xe_unbind_scratch_vma(vm, addr, range))
> + goto retry;
> goto release_vm_lock;
> + }
> }
>
> for (i = 0; i < args->num_binds; ++i) {
> diff --git a/drivers/gpu/drm/xe/xe_vm.h b/drivers/gpu/drm/xe/xe_vm.h
> index 5edb7771629c..74f99b38f60d 100644
> --- a/drivers/gpu/drm/xe/xe_vm.h
> +++ b/drivers/gpu/drm/xe/xe_vm.h
> @@ -140,6 +140,8 @@ static inline void xe_vm_queue_rebind_worker(struct
> xe_vm *vm)
> queue_work(vm->xe->ordered_wq, &vm->preempt.rebind_work);
> }
>
> +int xe_bind_scratch_vma(struct xe_vm *vm, u64 addr, u64 size);
> +
> /*
> * XE_ONSTACK_TV is used to size the tv_onstack array that is input
> * to xe_vm_lock_dma_resv() and xe_vm_unlock_dma_resv().
> --
> 2.25.1
^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2023-11-02 21:16 UTC | newest]
Thread overview: 18+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-08-26 0:14 [Intel-xe] [PATCH] drm/xe: Enable scratch page when page fault is enabled Chang, Bruce
2023-08-26 0:16 ` [Intel-xe] ✗ CI.Patch_applied: failure for drm/xe: Enable scratch page when page fault is enabled (rev2) Patchwork
2023-08-28 17:55 ` [Intel-xe] [PATCH] drm/xe: Enable scratch page when page fault is enabled Summers, Stuart
2023-08-28 19:11 ` Chang, Yu bruce
2023-08-28 22:25 ` Summers, Stuart
2023-08-28 22:44 ` Chang, Yu bruce
2023-08-29 16:36 ` Summers, Stuart
2023-08-28 19:22 ` Matthew Brost
2023-08-28 22:34 ` Chang, Yu bruce
2023-08-28 23:44 ` Chang, Yu bruce
-- strict thread matches above, loose matches on Subject: below --
2023-08-30 21:34 Chang, Bruce
2023-09-01 0:31 ` Welty, Brian
2023-11-02 21:16 ` Summers, Stuart
2023-08-29 23:16 Chang, Bruce
2023-08-29 23:58 ` Welty, Brian
2023-08-30 21:37 ` Chang, Yu bruce
2023-07-10 22:06 Chang, Bruce
2023-07-11 2:11 ` Zeng, Oak
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox