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

[Matthew Brost <matthew.brost@intel.com>]

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.

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 */
>