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

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