[PATCH v23 2/5] drm/xe/xe_gt_pagefault: Move pagefault struct to header
Cavitt, Jonathan
jonathan.cavitt at intel.com
Tue May 6 21:30:11 UTC 2025
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 <jonathan.cavitt at intel.com>
Sent: Wednesday, April 30, 2025 11:42 AM
To: Hirschfeld, Dafna <dafna.hirschfeld at intel.com>; Brost, Matthew <matthew.brost at intel.com>; Wajdeczko, Michal <Michal.Wajdeczko at intel.com>
Cc: intel-xe at lists.freedesktop.org; Gupta, saurabhg <saurabhg.gupta at intel.com>; Zuo, Alex <alex.zuo at intel.com>; joonas.lahtinen at linux.intel.com; Zhang, Jianxun <jianxun.zhang at intel.com>; Lin, Shuicheng <shuicheng.lin at intel.com>; dri-devel at lists.freedesktop.org; Mrozek, Michal <michal.mrozek at intel.com>; Jadav, Raag <raag.jadav at intel.com>; Harrison, John C <john.c.harrison at intel.com>; Briano, Ivan <ivan.briano at intel.com>; Auld, Matthew <matthew.auld at intel.com>; Cavitt, Jonathan <jonathan.cavitt at intel.com>
Subject: RE: [PATCH v23 2/5] drm/xe/xe_gt_pagefault: Move pagefault struct to header
>
> -----Original Message-----
> From: Hirschfeld, Dafna <dafna.hirschfeld at intel.com>
> Sent: Wednesday, April 30, 2025 12:07 AM
> To: Cavitt, Jonathan <jonathan.cavitt at intel.com>
> Cc: intel-xe at lists.freedesktop.org; Gupta, saurabhg <saurabhg.gupta at intel.com>; Zuo, Alex <alex.zuo at intel.com>; joonas.lahtinen at linux.intel.com; Brost, Matthew <matthew.brost at intel.com>; Zhang, Jianxun <jianxun.zhang at intel.com>; Lin, Shuicheng <shuicheng.lin at intel.com>; dri-devel at lists.freedesktop.org; Wajdeczko, Michal <Michal.Wajdeczko at intel.com>; Mrozek, Michal <michal.mrozek at intel.com>; Jadav, Raag <raag.jadav at intel.com>; Harrison, John C <john.c.harrison at intel.com>; Briano, Ivan <ivan.briano at intel.com>; Auld, Matthew <matthew.auld at intel.com>
> 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 <jonathan.cavitt at intel.com>
> > >>Reviewed-by: Shuicheng Lin <shuicheng.lin at intel.com>
> > >>Acked-by: Matthew Brost <matthew.brost at intel.com>
> > >>Cc: Michal Wajdeczko <Michal.Wajdeczko at intel.com>
> > >>---
> > >>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
> > >>
> >
>
More information about the Intel-xe
mailing list