Re: [RFC 2/2] drm/xe/gt_pagefault: Add tracepoints when handing page faults

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

On Tue, 2025-09-09 at 13:29 -0700, Matthew Brost wrote:
> On Tue, Sep 09, 2025 at 01:08:03PM -0600, Summers, Stuart wrote:
> > On Tue, 2025-09-09 at 17:03 +0200, Francois Dugast wrote:
> > > Add a new tracepoints when handling VMA and SVM page faults.
> > > 
> > > Signed-off-by: Francois Dugast <francois.dugast@intel.com>
> > > ---
> > >  drivers/gpu/drm/xe/xe_gt_pagefault.c       | 22 ++++--------
> > >  drivers/gpu/drm/xe/xe_gt_pagefault_types.h | 25 ++++++++++++++
> > >  drivers/gpu/drm/xe/xe_trace.h              | 40
> > > ++++++++++++++++++++++
> > >  3 files changed, 71 insertions(+), 16 deletions(-)
> > >  create mode 100644 drivers/gpu/drm/xe/xe_gt_pagefault_types.h
> > > 
> > > diff --git a/drivers/gpu/drm/xe/xe_gt_pagefault.c
> > > b/drivers/gpu/drm/xe/xe_gt_pagefault.c
> > > index d02d22fb3659..50836df79950 100644
> > > --- a/drivers/gpu/drm/xe/xe_gt_pagefault.c
> > > +++ b/drivers/gpu/drm/xe/xe_gt_pagefault.c
> > > @@ -20,24 +20,11 @@
> > >  #include "xe_guc_ct.h"
> > >  #include "xe_migrate.h"
> > >  #include "xe_svm.h"
> > > +#include "xe_trace.h"
> > >  #include "xe_trace_bo.h"
> > >  #include "xe_vm.h"
> > >  #include "xe_vram_types.h"
> > >  
> > > -struct pagefault {
> > > -       u64 page_addr;
> > > -       u32 asid;
> > > -       u16 pdata;
> > > -       u8 vfid;
> > > -       u8 access_type;
> > > -       u8 fault_type;
> > > -       u8 fault_level;
> > > -       u8 engine_class;
> > > -       u8 engine_instance;
> > > -       u8 fault_unsuccessful;
> > > -       bool trva_fault;
> > > -};
> > 
> > I'd rather wait to add this on top of the page fault series Matt
> > has on
> > the list.
> > 
> 
> I agree that we shouldn't be moving this structure to common file
> which
> we will just clobber soon. I'm not sure when I'll have time to rebase
> that code though, getting dragged into a couple of other hot topics,
> it
> may be a few weeks.
> 
> If anyone wants to take the series over, feel free.

Let me see what I can do there. I'll try to drive that like the TLB
inval series recently.

I'm also a little worried about dropping the access counter support we
have in there, even without a uapi currently.

Thanks,
Stuart

> 
> Matt
> 
> > > -
> > >  enum access_type {
> > >         ACCESS_TYPE_READ = 0,
> > >         ACCESS_TYPE_WRITE = 1,
> > > @@ -209,11 +196,14 @@ static int handle_pagefault(struct xe_gt
> > > *gt,
> > > struct pagefault *pf)
> > >  
> > >         atomic = access_is_atomic(pf->access_type);
> > >  
> > > -       if (xe_vma_is_cpu_addr_mirror(vma))
> > > +       if (xe_vma_is_cpu_addr_mirror(vma)) {
> > > +               trace_xe_handle_svm_pagefault(xe, pf);
> > 
> > Can you add this directly into the xe_svm_handle_pagefault routine
> > instead of in the outer layer here?
> > 
> > >                 err = xe_svm_handle_pagefault(vm, vma, gt,
> > >                                               pf->page_addr,
> > > atomic);
> > > -       else
> > > +       } else {
> > > +               trace_xe_handle_vma_pagefault(xe, pf);
> > >                 err = handle_vma_pagefault(gt, vma, atomic);
> > > +       }
> > >  
> > >  unlock_vm:
> > >         if (!err)
> > > diff --git a/drivers/gpu/drm/xe/xe_gt_pagefault_types.h
> > > b/drivers/gpu/drm/xe/xe_gt_pagefault_types.h
> > > new file mode 100644
> > > index 000000000000..431b1469d79b
> > > --- /dev/null
> > > +++ b/drivers/gpu/drm/xe/xe_gt_pagefault_types.h
> > > @@ -0,0 +1,25 @@
> > > +/* SPDX-License-Identifier: MIT */
> > > +/*
> > > + * Copyright © 2025 Intel Corporation
> > > + */
> > > +
> > > +#ifndef _XE_GT_PAGEFAULT_TYPES_H_
> > > +#define _XE_GT_PAGEFAULT_TYPES_H_
> > > +
> > > +#include <linux/types.h>
> > > +
> > > +struct pagefault {
> > > +       u64 page_addr;
> > > +       u32 asid;
> > > +       u16 pdata;
> > > +       u8 vfid;
> > > +       u8 access_type;
> > > +       u8 fault_type;
> > > +       u8 fault_level;
> > > +       u8 engine_class;
> > > +       u8 engine_instance;
> > > +       u8 fault_unsuccessful;
> > > +       bool trva_fault;
> > > +};
> > > +
> > > +#endif /* _XE_GT_PAGEFAULT_TYPES_ */
> > > diff --git a/drivers/gpu/drm/xe/xe_trace.h
> > > b/drivers/gpu/drm/xe/xe_trace.h
> > > index b9f59651771e..8587982e7eee 100644
> > > --- a/drivers/gpu/drm/xe/xe_trace.h
> > > +++ b/drivers/gpu/drm/xe/xe_trace.h
> > > @@ -14,8 +14,10 @@
> > >  
> > >  #include "xe_exec_queue_types.h"
> > >  #include "xe_gpu_scheduler_types.h"
> > > +#include "xe_gt_pagefault_types.h"
> > >  #include "xe_gt_types.h"
> > >  #include "xe_guc_exec_queue_types.h"
> > > +#include "xe_hw_engine.h"
> > >  #include "xe_migrate_types.h"
> > >  #include "xe_sched_job.h"
> > >  #include "xe_tlb_inval_types.h"
> > > @@ -473,6 +475,44 @@ DEFINE_EVENT(xe_migrate, xe_migrate_vram,
> > >              TP_ARGS(xe, len, vram_addr, dir)
> > >  );
> > >  
> > > +DECLARE_EVENT_CLASS(xe_pagefault,
> > > +                   TP_PROTO(struct xe_device *xe, struct
> > > pagefault
> > > *pf),
> > > +                   TP_ARGS(xe, pf),
> > > +
> > > +                   TP_STRUCT__entry(
> > > +                           __string(dev, __dev_name_xe(xe))
> > > +                           __field(u64, page_addr)
> > > +                           __field(u8, fault_type)
> > > +                           __field(u8, access_type)
> > > +                           __field(u8, fault_level)
> > > +                           __field(u8, engine_class)
> > > +                           __field(u8, engine_instance)
> > 
> > Also this information is more pagefault specific and not really to
> > SVM
> > specifically. Instead of moving to xe_svm.c, you could just put
> > this
> > outside of the svm if condition above to cover all cases.
> > 
> > > +                           ),
> > > +
> > > +                   TP_fast_assign(
> > > +                           __assign_str(dev);
> > > +                           __entry->page_addr = pf->page_addr;
> > 
> > And yeah something about the VM we found would also be interesting
> > here.
> > 
> > > +                           ),
> > > +
> > > +                   TP_printk("dev=%s addr=%llx fault_type=%d
> > > access_type=%d fault_level=%d engine_class=%d %s
> > > engine_instance=%d",
> > > +                             __get_str(dev), __entry->page_addr,
> > > +                             __entry->fault_type, __entry-
> > > > access_type,
> > > +                             __entry->fault_level, __entry-
> > > > engine_class,
> > > +                             xe_hw_engine_class_to_str(
> > > +                                     __entry->engine_class),
> > > +                             __entry->engine_instance)
> > > +);
> > > +
> > > +DEFINE_EVENT(xe_pagefault, xe_handle_vma_pagefault,
> > > +            TP_PROTO(struct xe_device *xe, struct pagefault
> > > *pf),
> > > +            TP_ARGS(xe, pf)
> > > +);
> > > +
> > > +DEFINE_EVENT(xe_pagefault, xe_handle_svm_pagefault,
> > > +            TP_PROTO(struct xe_device *xe, struct pagefault
> > > *pf),
> > > +            TP_ARGS(xe, pf)
> > 
> > Yeah I just don't see why we need both of these from your patch,
> > other
> > than just to tell whether the cpu mirror support is set.
> > 
> > Thanks,
> > Stuart
> > 
> > > +);
> > > +
> > >  #endif
> > >  
> > >  /* This part must be outside protection */
> >