-----Original Message-----
From: Wajdeczko, Michal <Michal.Wajdeczko@xxxxxxxxx>
Sent: Tuesday, March 18, 2025 3:02 PM
To: Cavitt, Jonathan <jonathan.cavitt@xxxxxxxxx>; intel-xe@xxxxxxxxxxxxxxxxxxxxx
Cc: Gupta, saurabhg <saurabhg.gupta@xxxxxxxxx>; Zuo, Alex <alex.zuo@xxxxxxxxx>; joonas.lahtinen@xxxxxxxxxxxxxxx; Brost, Matthew <matthew.brost@xxxxxxxxx>; Zhang, Jianxun <jianxun.zhang@xxxxxxxxx>; Lin, Shuicheng <shuicheng.lin@xxxxxxxxx>; dri-devel@xxxxxxxxxxxxxxxxxxxxx; Mrozek, Michal <michal.mrozek@xxxxxxxxx>; Roper, Matthew D <matthew.d.roper@xxxxxxxxx>
Subject: Re: [PATCH v9 2/6] drm/xe/xe_gt_pagefault: Move pagefault struct to header
>
> On 18.03.2025 21:12, Cavitt, Jonathan wrote:
> > -----Original Message-----
> > From: Wajdeczko, Michal <Michal.Wajdeczko@xxxxxxxxx>
> > Sent: Tuesday, March 18, 2025 11:38 AM
> > To: Cavitt, Jonathan <jonathan.cavitt@xxxxxxxxx>; intel-xe@xxxxxxxxxxxxxxxxxxxxx; Roper, Matthew D <matthew.d.roper@xxxxxxxxx>
> > Cc: Gupta, saurabhg <saurabhg.gupta@xxxxxxxxx>; Zuo, Alex <alex.zuo@xxxxxxxxx>; joonas.lahtinen@xxxxxxxxxxxxxxx; Brost, Matthew <matthew.brost@xxxxxxxxx>; Zhang, Jianxun <jianxun.zhang@xxxxxxxxx>; Lin, Shuicheng <shuicheng.lin@xxxxxxxxx>; dri-devel@xxxxxxxxxxxxxxxxxxxxx; Mrozek, Michal <michal.mrozek@xxxxxxxxx>
> > Subject: Re: [PATCH v9 2/6] drm/xe/xe_gt_pagefault: Move pagefault struct to header
> >>
> >> On 18.03.2025 18:11, Jonathan Cavitt wrote:
> >>> Move the pagefault struct from xe_gt_pagefault.c to the
> >>> xe_gt_pagefault_types.h header file, along with the associated enum values.
> >>>
> >>> v2:
> >>> - Normalize names for common header (Matt Brost)
> >>>
> >>> v3:
> >>> - s/Migrate/Move (Michal W)
> >>> - s/xe_pagefault/xe_gt_pagefault (Michal W)
> >>> - Create new header file, xe_gt_pagefault_types.h (Michal W)
> >>> - Add kernel docs (Michal W)
> >>>
> >>> v4:
> >>> - Fix includes usage (Michal W)
> >>> - Reference Bspec (Michal W)
> >>>
> >>> Bspec: 77412
> >>>
> >>
> >> keep all tags together
> >>
> >>> Signed-off-by: Jonathan Cavitt <jonathan.cavitt@xxxxxxxxx>
> >>> Cc: Michal Wajdeczko <Michal.Wajdeczko@xxxxxxxxx>
> >>> ---
> >>> drivers/gpu/drm/xe/xe_gt_pagefault.c | 42 +++------------
> >>> drivers/gpu/drm/xe/xe_gt_pagefault_types.h | 61 ++++++++++++++++++++++
> >>> 2 files changed, 69 insertions(+), 34 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 3240890aac07..37b4ab5135a8 100644
> >>> --- a/drivers/gpu/drm/xe/xe_gt_pagefault.c
> >>> +++ b/drivers/gpu/drm/xe/xe_gt_pagefault.c
> >>> @@ -14,6 +14,7 @@
> >>> #include "abi/guc_actions_abi.h"
> >>> #include "xe_bo.h"
> >>> #include "xe_gt.h"
> >>> +#include "xe_gt_pagefault_types.h"
> >>> #include "xe_gt_stats.h"
> >>> #include "xe_gt_tlb_invalidation.h"
> >>> #include "xe_guc.h"
> >>> @@ -23,33 +24,6 @@
> >>> #include "xe_trace_bo.h"
> >>> #include "xe_vm.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;
> >>> -};
> >>> -
> >>> -enum access_type {
> >>> - ACCESS_TYPE_READ = 0,
> >>> - ACCESS_TYPE_WRITE = 1,
> >>> - ACCESS_TYPE_ATOMIC = 2,
> >>> - ACCESS_TYPE_RESERVED = 3,
> >>> -};
> >>> -
> >>> -enum fault_type {
> >>> - NOT_PRESENT = 0,
> >>> - WRITE_ACCESS_VIOLATION = 1,
> >>> - ATOMIC_ACCESS_VIOLATION = 2,
> >>> -};
> >>> -
> >>> struct acc {
> >>> u64 va_range_base;
> >>> u32 asid;
> >>> @@ -61,9 +35,9 @@ struct acc {
> >>> u8 engine_instance;
> >>> };
> >>>
> >>> -static bool access_is_atomic(enum access_type access_type)
> >>> +static bool access_is_atomic(enum xe_gt_pagefault_access_type access_type)
> >>> {
> >>> - return access_type == ACCESS_TYPE_ATOMIC;
> >>> + return access_type == XE_GT_PAGEFAULT_ACCESS_TYPE_ATOMIC;
> >>> }
> >>>
> >>> static bool vma_is_valid(struct xe_tile *tile, struct xe_vma *vma)
> >>> @@ -205,7 +179,7 @@ static struct xe_vm *asid_to_vm(struct xe_device *xe, u32 asid)
> >>> return vm;
> >>> }
> >>>
> >>> -static int handle_pagefault(struct xe_gt *gt, struct pagefault *pf)
> >>> +static int handle_pagefault(struct xe_gt *gt, struct xe_gt_pagefault *pf)
> >>> {
> >>> struct xe_device *xe = gt_to_xe(gt);
> >>> struct xe_vm *vm;
> >>> @@ -237,7 +211,7 @@ static int handle_pagefault(struct xe_gt *gt, struct pagefault *pf)
> >>> goto unlock_vm;
> >>> }
> >>>
> >>> - if (xe_vma_read_only(vma) && pf->access_type != ACCESS_TYPE_READ) {
> >>> + if (xe_vma_read_only(vma) && pf->access_type != XE_GT_PAGEFAULT_ACCESS_TYPE_READ) {
> >>> err = -EPERM;
> >>> goto unlock_vm;
> >>> }
> >>> @@ -271,7 +245,7 @@ static int send_pagefault_reply(struct xe_guc *guc,
> >>> return xe_guc_ct_send(&guc->ct, action, ARRAY_SIZE(action), 0, 0);
> >>> }
> >>>
> >>> -static void print_pagefault(struct xe_device *xe, struct pagefault *pf)
> >>> +static void print_pagefault(struct xe_device *xe, struct xe_gt_pagefault *pf)
> >>> {
> >>> drm_dbg(&xe->drm, "\n\tASID: %d\n"
> >>> "\tVFID: %d\n"
> >>> @@ -291,7 +265,7 @@ static void print_pagefault(struct xe_device *xe, struct pagefault *pf)
> >>>
> >>> #define PF_MSG_LEN_DW 4
> >>>
> >>> -static bool get_pagefault(struct pf_queue *pf_queue, struct pagefault *pf)
> >>> +static bool get_pagefault(struct pf_queue *pf_queue, struct xe_gt_pagefault *pf)
> >>> {
> >>> const struct xe_guc_pagefault_desc *desc;
> >>> bool ret = false;
> >>> @@ -378,7 +352,7 @@ static void pf_queue_work_func(struct work_struct *w)
> >>> struct xe_gt *gt = pf_queue->gt;
> >>> struct xe_device *xe = gt_to_xe(gt);
> >>> struct xe_guc_pagefault_reply reply = {};
> >>> - struct pagefault pf = {};
> >>> + struct xe_gt_pagefault pf = {};
> >>> unsigned long threshold;
> >>> int ret;
> >>>
> >>> 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..55c598c4a1bc
> >>> --- /dev/null
> >>> +++ b/drivers/gpu/drm/xe/xe_gt_pagefault_types.h
> >>> @@ -0,0 +1,61 @@
> >>> +/* SPDX-License-Identifier: MIT */
> >>> +/*
> >>> + * Copyright © 2022-2025 Intel Corporation
> >>> + */
> >>> +
> >>> +#ifndef _XE_GT_PAGEFAULT_TYPES_H_
> >>> +#define _XE_GT_PAGEFAULT_TYPES_H_
> >>> +
> >>> +#include <linux/types.h>
> >>> +
> >>> +/**
> >>> + * struct xe_gt_pagefault - Structure of pagefaults returned by the
> >>> + * pagefault handler
> >>> + */
> >>> +struct xe_gt_pagefault {
> >>> + /** @page_addr: faulted address of this pagefault */
> >>> + u64 page_addr;
> >>> + /** @asid: ASID of this pagefault */
> >>> + u32 asid;
> >>> + /** @pdata: PDATA of this pagefault */
> >>> + u16 pdata;
> >>> + /** @vfid: VFID of this pagefault */
> >>> + u8 vfid;
> >>> + /**
> >>> + * @access_type: access type of this pagefault, as a value
> >>> + * from xe_gt_pagefault_access_type
> >>> + */
> >>> + u8 access_type;
> >>> + /**
> >>> + * @fault_type: fault type of this pagefault, as a value
> >>> + * from xe_gt_pagefault_fault_type
> >>> + */
> >>> + u8 fault_type;
> >>> + /** @fault_level: fault level of this pagefault */
> >>> + u8 fault_level;
> >>> + /** @engine_class: engine class this pagefault was reported on */
> >>> + u8 engine_class;
> >>> + /** @engine_instance: engine instance this pagefault was reported on */
> >>> + u8 engine_instance;
> >>> + /** @fault_unsuccessful: flag for if the pagefault recovered or not */
> >>> + u8 fault_unsuccessful;
> >>> + /** @prefetch: unused */
> >>> + bool prefetch;
> >>> + /** @trva_fault: is set if this is a TRTT fault */
> >>> + bool trva_fault;
> >>> +};
> >>> +
> >>> +enum xe_gt_pagefault_access_type {
> >>> + XE_GT_PAGEFAULT_ACCESS_TYPE_READ = 0,
> >>> + XE_GT_PAGEFAULT_ACCESS_TYPE_WRITE = 1,
> >>> + XE_GT_PAGEFAULT_ACCESS_TYPE_ATOMIC = 2,
> >>> + XE_GT_PAGEFAULT_ACCESS_TYPE_RESERVED = 3,
> >>> +};
> >>> +
> >>> +enum xe_gt_pagefault_fault_type {
> >>> + XE_GT_PAGEFAULT_TYPE_NOT_PRESENT = 0,
> >>> + XE_GT_PAGEFAULT_TYPE_WRITE_ACCESS_VIOLATION = 1,
> >>> + XE_GT_PAGEFAULT_TYPE_ATOMIC_ACCESS_VIOLATION = 2,
> >>> +};
> >>
> >> as those two enums seems to be reflecting HW definitions, IMO they
> >> should be defined separately from the SW definitions
> >>
> >> most of the HW definitions we keep in the regs/ folder, even if not
> >> being pure register definitions, see:
> >> - xe_bars.h
> >> - xe_lrc_layout.h
> >> - xe_gtt_defs.h
> >>
> >> we can add page faults defs to new regs/xe_pagefault_desc.h *) file,
> >> additionally we can define there format of the page fault descriptor,
> >> and remove it from the xe_guc_fwif.h, as GuC seems to pass data in HW
> >> format anyway
> >
> > For clarification, when I move these enum definitions to the regs folder,
>
> I'm not sure that "move as-is" would be ok
>
> right now most (all?) hardware definitions are using #define not the
> non-anonymous enums, also I would try to follow names used in the Bspec:
>
> #define FLT_TYPE_NOT_PRESENT_FAULT 0u
> #define FLT_TYPE_WRITE_ACCESS_VIOLATION 1u
> #define FLT_TYPE_ATOMIC_ACCESS_VIOLATION 2u
>
> #define ACC_TYPE_READ_ACCESS 0u
> ...
>
> so the "Normalize names for common header" request does not apply for
> them, only name of struct xe_gt_pagefault must be aligned
>
> > I'm going to assume you want me to include the created header file as
> > an includes in the relevant .c files,
>
> you likely need only few of these HW defs in xe_gt_pagefault.c
>
> in other places only struct definition is needed (which does not have
> any of these "enums" on board)
>
> > and not just staple it into
> > xe_gt_pagefault.h or xe_gt_pagefault_types.h. Is this correct?
>
> the rule is:
> if you don't need any of these defs in .h, don't include them there
These revision notes have been applied. I'll wait for 6 hours before posting in
case I get additional revision notes, though.
-Jonathan Cavitt
>
> >
> > -Jonathan Cavitt
> >
> >>
> >> or maybe @MattR has a better idea where these defs should go
> >>
> >>
> >> *) please don't be tempted to use regs/xe_pf_desc.h since we want to use
> >> "PF" term only to mean "Physical Function", not "page fault" ;)
> >>
> >>> +
> >>> +#endif
> >>
> >>
>
>
