Re: [Intel-xe] [PATCH] drm/xe: Enable scratch page when page fault is enabled

["Summers, Stuart" <stuart.summers@intel.com>]

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().
> > > 
>