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

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

On Wed, 2025-09-10 at 09:59 +0200, Francois Dugast wrote:
> On Tue, Sep 09, 2025 at 09:08:03PM +0200, 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.
> 
> Yes, absolutely.
> 
> > 
> > > -
> > >  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?
> 
> Yes but I would favor the other approach you are suggesting below.

Yeah no problem, just wanted to give different options there.

> 
> > 
> > >                 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.
> 
> Sure, will do.
> 
> > 
> > > +                           ),
> > > +
> > > +                   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.
> 
> Alright, any info in particular? asid and gpusvm?

Mostly here I think we just want a way to tie these in with either
context submissions or VMs/BOs created by the user. So if we have the
VM traces on, we should be able to see those binds and these faults and
clearly associate the different processes/threads accessing them.

Also would be good to pass anything hardware/guc is sending us, so
asid, vfid, pdata (hardware fault identifier). Also the vm and vma
since we're calculating those to do the page table updates.

On the SVM side, we don't really have any other traces handling
creation right now so the only thing to me that makes sense to add here
is whether or not this particular fault has that CPU mirror associated
(i.e. is this an SVM fault or not: xe_vma_is_cpu_addr_mirror()).

Thanks,
Stuart

> 
> > 
> > > +                           ),
> > > +
> > > +                   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.
> 
> Yes, will do in v2.
> 
> Thanks,
> Francois
> 
> > 
> > Thanks,
> > Stuart
> > 
> > > +);
> > > +
> > >  #endif
> > >  
> > >  /* This part must be outside protection */
> >