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

[Francois Dugast <francois.dugast@intel.com>]

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.

> 
> >                 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?

> 
> > +                           ),
> > +
> > +                   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 */
>