[PATCH v23 2/5] drm/xe/xe_gt_pagefault: Move pagefault struct to header

[Cavitt, Jonathan]

-----Original Message-----
From: Wajdeczko, Michal <Michal.Wajdeczko at intel.com> 
Sent: Wednesday, May 7, 2025 9:34 AM
To: Hirschfeld, Dafna <dafna.hirschfeld at intel.com>; 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; 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 16: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
> 
> maybe also mention 59654 here?
> 
> >> 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.
> 
> IMO 'guc' tag is not applicable here
> 
> my understanding was that GuC sends data as it was read from the HW
> regs, so the origin of the struct layout is HW, hence we put it in regs/
> where we have all the HW defs, not in the abi/ folder which is for FW defs.
> 
> unless we want to make this struct part of the stable GuC pagefault ABI,
> and then even if HW definition/layout would change, we will stick with
> current layout. Then agree, we should move all this to abi/ and also
> drop the Bspec reference as n/a

So, to summarize, you want me to *not* change the name of the regs/xe_pagefault_desc.h file?

I'll get on reverting that change shortly.

> 
> > 
> > Also, the define 'PF_MSG_LEN_DW    4' relates to the
> > length of this struct so should move here.
> 
> any G2H message related definitions should be in the abi/ GuC files, not
> here in regs/ where we keep HW definitions.
> 
> and please don't start define name with "PF", use "GUC" instead
> 
> and btw, in many places by message length we assume also HXG header
> length, so the actual length of the G2H 6002 message is 5 as it includes
> 1 DW of header with DATA0 and 4 DWs of payload with DATA1..4
> 
> #define GUC2HOST_NOTIFY_PAGE_FAULT_REQ_DESC_MSG_LEN \
> 	(GUC_HXG_REQUEST_MSG_MIN_LEN + 4u)

Should I replace all instances of PF_MSG_LEN_DW with
GUC2HOST_NOTIFY_PAGE_FAULT_REQ_DESC_MSG_LEN, instead of attempting
to relocate and rename the value?

If not, which abi  file should I relocate the value to,
and what should the value be renamed to?
GUC_PF_MSG_LEN_DW?

-Jonathan Cavitt

> 
> > 
> > Thanks,
> > Dafna
> > 
>