I think this patch series is still blocked on these requests. Do you want me to implement these suggested changes, or should they be dropped? @Brost, Matthew and @Wajdeczko, Michal -Jonathan Cavitt
-----Original Message----- From: Cavitt, Jonathan <[email protected]> Sent: Wednesday, April 30, 2025 11:42 AM To: Hirschfeld, Dafna <[email protected]>; Brost, Matthew <[email protected]>; Wajdeczko, Michal <[email protected]> Cc: [email protected]; Gupta, saurabhg <[email protected]>; Zuo, Alex <[email protected]>; [email protected]; Zhang, Jianxun <[email protected]>; Lin, Shuicheng <[email protected]>; [email protected]; Mrozek, Michal <[email protected]>; Jadav, Raag <[email protected]>; Harrison, John C <[email protected]>; Briano, Ivan <[email protected]>; Auld, Matthew <[email protected]>; Cavitt, Jonathan <[email protected]> Subject: RE: [PATCH v23 2/5] drm/xe/xe_gt_pagefault: Move pagefault struct to header > > -----Original Message----- > From: Hirschfeld, Dafna <[email protected]> > Sent: Wednesday, April 30, 2025 12:07 AM > To: Cavitt, Jonathan <[email protected]> > Cc: [email protected]; Gupta, saurabhg > <[email protected]>; Zuo, Alex <[email protected]>; > [email protected]; Brost, Matthew <[email protected]>; > Zhang, Jianxun <[email protected]>; Lin, Shuicheng > <[email protected]>; [email protected]; Wajdeczko, Michal > <[email protected]>; Mrozek, Michal <[email protected]>; > Jadav, Raag <[email protected]>; Harrison, John C > <[email protected]>; Briano, Ivan <[email protected]>; Auld, > Matthew <[email protected]> > Subject: Re: [PATCH v23 2/5] drm/xe/xe_gt_pagefault: Move pagefault struct to > header > > > > On 29.04.2025 17:22, Dafna Hirschfeld wrote: > > >On 24.04.2025 20:49, Jonathan Cavitt wrote: > > >>Move the pagefault struct from xe_gt_pagefault.c to the > > >>xe_gt_pagefault_types.h header file, and move the associated enum values > > >>into the regs folder under xe_pagefault_desc.h > > >> > > >>Since xe_pagefault_desc.h is being initialized here, also move the > > >>xe_guc_pagefault_desc hardware formats to the new file. > > >> > > >>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) > > >> > > >>v5: > > >>- Convert enums to defines in regs folder (Michal W) > > >>- Move xe_guc_pagefault_desc to regs folder (Michal W) > > >> > > >>Bspec: 77412 > > >>Signed-off-by: Jonathan Cavitt <[email protected]> > > >>Reviewed-by: Shuicheng Lin <[email protected]> > > >>Acked-by: Matthew Brost <[email protected]> > > >>Cc: Michal Wajdeczko <[email protected]> > > >>--- > > >>drivers/gpu/drm/xe/regs/xe_pagefault_desc.h | 49 +++++++++++++++++++++ > > >>drivers/gpu/drm/xe/xe_gt_pagefault.c | 43 ++++-------------- > > >>drivers/gpu/drm/xe/xe_gt_pagefault_types.h | 42 ++++++++++++++++++ > > >>drivers/gpu/drm/xe/xe_guc_fwif.h | 28 ------------ > > >>4 files changed, 100 insertions(+), 62 deletions(-) > > >>create mode 100644 drivers/gpu/drm/xe/regs/xe_pagefault_desc.h > > >>create mode 100644 drivers/gpu/drm/xe/xe_gt_pagefault_types.h > > >> > > >>diff --git a/drivers/gpu/drm/xe/regs/xe_pagefault_desc.h > > >>b/drivers/gpu/drm/xe/regs/xe_pagefault_desc.h > > >>new file mode 100644 > > >>index 000000000000..a169ac274e14 > > >>--- /dev/null > > >>+++ b/drivers/gpu/drm/xe/regs/xe_pagefault_desc.h > > > > > >Maybe change the file name to xe_guc_pagefault_desc.h , > > >since this is currently guc specific. > > On paper, this shouldn't be a difficult change, though I'd like to ask > @Brost, Matthew and @Wajdeczko, Michal if this request is > amenable. > > > > > > >Also, the define 'PF_MSG_LEN_DW 4' relates to the > > >length of this struct so should move here. > > I don't see the harm in moving it, though I'd like to wait for a second > opinion before proceeding. > > > > > Actually, the struct 'xe_guc_acc_desc' and its defines should > > also move here. > > IMO, I think that if xe_guc_acc_desc needed to be extracted from the > xe_gt_pagefault.c file, it should be put in its own header file in the regs > folder, rather than sharing a file with xe_guc_pagefault_desc.h. Say, > regs/xe_guc_acc_desc.h? > > Though at that point, I think the migration would be out of scope for > this series. > -Jonathan Cavitt > > > > > Thanks, > > Dafna > > > > > > > >Thanks, > > >Dafna > > > > > >>@@ -0,0 +1,49 @@ > > >>+/* SPDX-License-Identifier: MIT */ > > >>+/* > > >>+ * Copyright (c) 2025 Intel Corporation > > >>+ */ > > >>+ > > >>+#ifndef _XE_PAGEFAULT_DESC_H_ > > >>+#define _XE_PAGEFAULT_DESC_H_ > > >>+ > > >>+#include <linux/bits.h> > > >>+#include <linux/types.h> > > >>+ > > >>+struct xe_guc_pagefault_desc { > > >>+ u32 dw0; > > >>+#define PFD_FAULT_LEVEL GENMASK(2, 0) > > >>+#define PFD_SRC_ID GENMASK(10, 3) > > >>+#define PFD_RSVD_0 GENMASK(17, 11) > > >>+#define XE2_PFD_TRVA_FAULT BIT(18) > > >>+#define PFD_ENG_INSTANCE GENMASK(24, 19) > > >>+#define PFD_ENG_CLASS GENMASK(27, 25) > > >>+#define PFD_PDATA_LO GENMASK(31, 28) > > >>+ > > >>+ u32 dw1; > > >>+#define PFD_PDATA_HI GENMASK(11, 0) > > >>+#define PFD_PDATA_HI_SHIFT 4 > > >>+#define PFD_ASID GENMASK(31, 12) > > >>+ > > >>+ u32 dw2; > > >>+#define PFD_ACCESS_TYPE GENMASK(1, 0) > > >>+#define PFD_FAULT_TYPE GENMASK(3, 2) > > >>+#define PFD_VFID GENMASK(9, 4) > > >>+#define PFD_RSVD_1 GENMASK(11, 10) > > >>+#define PFD_VIRTUAL_ADDR_LO GENMASK(31, 12) > > >>+#define PFD_VIRTUAL_ADDR_LO_SHIFT 12 > > >>+ > > >>+ u32 dw3; > > >>+#define PFD_VIRTUAL_ADDR_HI GENMASK(31, 0) > > >>+#define PFD_VIRTUAL_ADDR_HI_SHIFT 32 > > >>+} __packed; > > >>+ > > >>+#define FLT_ACCESS_TYPE_READ 0u > > >>+#define FLT_ACCESS_TYPE_WRITE 1u > > >>+#define FLT_ACCESS_TYPE_ATOMIC 2u > > >>+#define FLT_ACCESS_TYPE_RESERVED 3u > > >>+ > > >>+#define FLT_TYPE_NOT_PRESENT_FAULT 0u > > >>+#define FLT_TYPE_WRITE_ACCESS_VIOLATION 1u > > >>+#define FLT_TYPE_ATOMIC_ACCESS_VIOLATION 2u > > >>+ > > >>+#endif > > >>diff --git a/drivers/gpu/drm/xe/xe_gt_pagefault.c > > >>b/drivers/gpu/drm/xe/xe_gt_pagefault.c > > >>index d4e3b7eb165a..93afa54c8780 100644 > > >>--- a/drivers/gpu/drm/xe/xe_gt_pagefault.c > > >>+++ b/drivers/gpu/drm/xe/xe_gt_pagefault.c > > >>@@ -12,8 +12,10 @@ > > >>#include <drm/drm_managed.h> > > >> > > >>#include "abi/guc_actions_abi.h" > > >>+#include "regs/xe_pagefault_desc.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 +25,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 +36,9 @@ struct acc { > > >> u8 engine_instance; > > >>}; > > >> > > >>-static bool access_is_atomic(enum access_type access_type) > > >>+static bool access_is_atomic(u32 access_type) > > >>{ > > >>- return access_type == ACCESS_TYPE_ATOMIC; > > >>+ return access_type == FLT_ACCESS_TYPE_ATOMIC; > > >>} > > >> > > >>static bool vma_is_valid(struct xe_tile *tile, struct xe_vma *vma) > > >>@@ -205,7 +180,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 +212,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 != FLT_ACCESS_TYPE_READ) { > > >> err = -EPERM; > > >> goto unlock_vm; > > >> } > > >>@@ -271,7 +246,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 +266,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 +353,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..b7d41b558de3 > > >>--- /dev/null > > >>+++ b/drivers/gpu/drm/xe/xe_gt_pagefault_types.h > > >>@@ -0,0 +1,42 @@ > > >>+/* SPDX-License-Identifier: MIT */ > > >>+/* > > >>+ * Copyright (c) 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 */ > > >>+ u8 access_type; > > >>+ /** @fault_type: fault type of this pagefault */ > > >>+ 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; > > >>+}; > > >>+ > > >>+#endif > > >>diff --git a/drivers/gpu/drm/xe/xe_guc_fwif.h > > >>b/drivers/gpu/drm/xe/xe_guc_fwif.h > > >>index 6f57578b07cb..30ac21bb4f15 100644 > > >>--- a/drivers/gpu/drm/xe/xe_guc_fwif.h > > >>+++ b/drivers/gpu/drm/xe/xe_guc_fwif.h > > >>@@ -290,34 +290,6 @@ enum xe_guc_response_desc_type { > > >> FAULT_RESPONSE_DESC > > >>}; > > >> > > >>-struct xe_guc_pagefault_desc { > > >>- u32 dw0; > > >>-#define PFD_FAULT_LEVEL GENMASK(2, 0) > > >>-#define PFD_SRC_ID GENMASK(10, 3) > > >>-#define PFD_RSVD_0 GENMASK(17, 11) > > >>-#define XE2_PFD_TRVA_FAULT BIT(18) > > >>-#define PFD_ENG_INSTANCE GENMASK(24, 19) > > >>-#define PFD_ENG_CLASS GENMASK(27, 25) > > >>-#define PFD_PDATA_LO GENMASK(31, 28) > > >>- > > >>- u32 dw1; > > >>-#define PFD_PDATA_HI GENMASK(11, 0) > > >>-#define PFD_PDATA_HI_SHIFT 4 > > >>-#define PFD_ASID GENMASK(31, 12) > > >>- > > >>- u32 dw2; > > >>-#define PFD_ACCESS_TYPE GENMASK(1, 0) > > >>-#define PFD_FAULT_TYPE GENMASK(3, 2) > > >>-#define PFD_VFID GENMASK(9, 4) > > >>-#define PFD_RSVD_1 GENMASK(11, 10) > > >>-#define PFD_VIRTUAL_ADDR_LO GENMASK(31, 12) > > >>-#define PFD_VIRTUAL_ADDR_LO_SHIFT 12 > > >>- > > >>- u32 dw3; > > >>-#define PFD_VIRTUAL_ADDR_HI GENMASK(31, 0) > > >>-#define PFD_VIRTUAL_ADDR_HI_SHIFT 32 > > >>-} __packed; > > >>- > > >>struct xe_guc_pagefault_reply { > > >> u32 dw0; > > >>#define PFR_VALID BIT(0) > > >>-- > > >>2.43.0 > > >> > > >
