-----Original Message-----
From: Wajdeczko, Michal <Michal.Wajdeczko@xxxxxxxxx>
Sent: Monday, March 10, 2025 11:20 AM
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
Subject: Re: [PATCH v7 2/6] drm/xe/xe_gt_pagefault: Migrate pagefault struct to header
> >
> > On 10.03.2025 18:18, Jonathan Cavitt wrote:
> > > Migrate the pagefault struct from xe_gt_pagefault.c to the
> >
> > nit: we use "migrate" verb for different purposes.
> > maybe here (and in the title) the plain "move" will be better?
> >
> > > xe_gt_pagefault.h header file, along with the associated enum values.
> >
> > hmm, all other components have foo_types.h header file as place for own
> > types and use foo.h header only for function declarations.
> >
> > why are we doing it differently here?
xe_gt_pagefault_types.h does not exist and would have to be created to satisfy this
request. It would also basically only contain the below xe_pagefault struct and its
related enums. Is this amenable?
-Jonathan Cavitt
> >
> > >
> > > v2: Normalize names for common header (Matt Brost)
> > >
> > > Signed-off-by: Jonathan Cavitt <jonathan.cavitt@xxxxxxxxx>
> > > ---
> > > drivers/gpu/drm/xe/xe_gt_pagefault.c | 41 +++++-----------------------
> > > drivers/gpu/drm/xe/xe_gt_pagefault.h | 28 +++++++++++++++++++
> > > 2 files changed, 35 insertions(+), 34 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/xe/xe_gt_pagefault.c b/drivers/gpu/drm/xe/xe_gt_pagefault.c
> > > index a4e688e72efd..c8a9058aa09f 100644
> > > --- a/drivers/gpu/drm/xe/xe_gt_pagefault.c
> > > +++ b/drivers/gpu/drm/xe/xe_gt_pagefault.c
> > > @@ -23,33 +23,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 +34,9 @@ struct acc {
> > > u8 engine_instance;
> > > };
> > >
> > > -static bool access_is_atomic(enum access_type access_type)
> > > +static bool access_is_atomic(enum xe_pagefault_access_type access_type)
> > > {
> > > - return access_type == ACCESS_TYPE_ATOMIC;
> > > + return access_type == XE_PAGEFAULT_ACCESS_TYPE_ATOMIC;
> > > }
> > >
> > > static bool vma_is_valid(struct xe_tile *tile, struct xe_vma *vma)
> > > @@ -205,7 +178,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_pagefault *pf)
> > > {
> > > struct xe_device *xe = gt_to_xe(gt);
> > > struct xe_vm *vm;
> > > @@ -237,7 +210,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_PAGEFAULT_ACCESS_TYPE_READ) {
> > > err = -EPERM;
> > > goto unlock_vm;
> > > }
> > > @@ -271,7 +244,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_pagefault *pf)
> > > {
> > > drm_dbg(&xe->drm, "\n\tASID: %d\n"
> > > "\tVFID: %d\n"
> > > @@ -291,7 +264,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_pagefault *pf)
> > > {
> > > const struct xe_guc_pagefault_desc *desc;
> > > bool ret = false;
> > > @@ -378,7 +351,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_pagefault pf = {};
> > > unsigned long threshold;
> > > int ret;
> > >
> > > diff --git a/drivers/gpu/drm/xe/xe_gt_pagefault.h b/drivers/gpu/drm/xe/xe_gt_pagefault.h
> > > index 839c065a5e4c..33616043d17a 100644
> > > --- a/drivers/gpu/drm/xe/xe_gt_pagefault.h
> > > +++ b/drivers/gpu/drm/xe/xe_gt_pagefault.h
> > > @@ -11,6 +11,34 @@
> > > struct xe_gt;
> > > struct xe_guc;
> > >
> > > +struct xe_pagefault {
> >
> > shouldn't this be "xe_gt_pagefault" ?
> > all functions seem to be related to the gt
> >
> > > + 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 prefetch;
> > > + bool trva_fault;
> > > +};
> >
> > missing kernel-doc (for struct and members)
> > and for below enums
> >
> > > +
> > > +enum xe_pagefault_access_type {
> > > + XE_PAGEFAULT_ACCESS_TYPE_READ = 0,
> > > + XE_PAGEFAULT_ACCESS_TYPE_WRITE = 1,
> > > + XE_PAGEFAULT_ACCESS_TYPE_ATOMIC = 2,
> > > + XE_PAGEFAULT_ACCESS_TYPE_RESERVED = 3,
> > > +};
> > > +
> > > +enum xe_pagefault_type {
> > > + XE_PAGEFAULT_TYPE_NOT_PRESENT = 0,
> > > + XE_PAGEFAULT_TYPE_WRITE_ACCESS_VIOLATION = 1,
> > > + XE_PAGEFAULT_TYPE_ATOMIC_ACCESS_VIOLATION = 2,
> > > +};
> > > +
> > > int xe_gt_pagefault_init(struct xe_gt *gt);
> > > void xe_gt_pagefault_reset(struct xe_gt *gt);
> > > int xe_guc_pagefault_handler(struct xe_guc *guc, u32 *msg, u32 len);
> >
> >
>
