Re: [PATCH v4 6/7] drm/xe: Add xe_guc_pagefault layer

[Matthew Brost <matthew.brost@intel.com>]

On Mon, Nov 03, 2025 at 01:11:34PM -0600, Lucas De Marchi wrote:
> On Mon, Nov 03, 2025 at 10:56:06AM -0800, Matthew Brost wrote:
> > On Mon, Nov 03, 2025 at 12:28:50PM -0600, Lucas De Marchi wrote:
> > > On Fri, Oct 31, 2025 at 09:54:15AM -0700, Matthew Brost wrote:
> > > > Add xe_guc_pagefault layer (producer) which parses G2H fault messages
> > > > messages into struct xe_pagefault, forwards them to the page fault layer
> > > > (consumer) for servicing, and provides a vfunc to acknowledge faults to
> > > > the GuC upon completion. Replace the old (and incorrect) GT page fault
> > > > layer with this new layer throughout the driver.
> > > >
> > > > v2:
> > > > - Include engine instance (Stuart)
> > > >
> > > > Signed-off-by: Matthew Brost <matthew.brost@intel.com>
> > > > ---
> > > > drivers/gpu/drm/xe/Makefile           |  2 +-
> > > > drivers/gpu/drm/xe/xe_gt.c            |  6 --
> > > > drivers/gpu/drm/xe/xe_guc_ct.c        |  6 +-
> > > > drivers/gpu/drm/xe/xe_guc_pagefault.c | 95 +++++++++++++++++++++++++++
> > > > drivers/gpu/drm/xe/xe_guc_pagefault.h | 15 +++++
> > > > drivers/gpu/drm/xe/xe_svm.c           |  3 +-
> > > > drivers/gpu/drm/xe/xe_vm.c            |  1 -
> > > > 7 files changed, 113 insertions(+), 15 deletions(-)
> > > > create mode 100644 drivers/gpu/drm/xe/xe_guc_pagefault.c
> > > > create mode 100644 drivers/gpu/drm/xe/xe_guc_pagefault.h
> > > >
> > > > diff --git a/drivers/gpu/drm/xe/Makefile b/drivers/gpu/drm/xe/Makefile
> > > > index b35021e5b9eb..319f208eb19a 100644
> > > > --- a/drivers/gpu/drm/xe/Makefile
> > > > +++ b/drivers/gpu/drm/xe/Makefile
> > > > @@ -58,7 +58,6 @@ xe-y += xe_bb.o \
> > > > 	xe_gt_freq.o \
> > > > 	xe_gt_idle.o \
> > > > 	xe_gt_mcr.o \
> > > > -	xe_gt_pagefault.o \
> > > > 	xe_gt_sysfs.o \
> > > > 	xe_gt_throttle.o \
> > > > 	xe_gt_topology.o \
> > > > @@ -73,6 +72,7 @@ xe-y += xe_bb.o \
> > > > 	xe_guc_id_mgr.o \
> > > > 	xe_guc_klv_helpers.o \
> > > > 	xe_guc_log.o \
> > > > +	xe_guc_pagefault.o \
> > > > 	xe_guc_pc.o \
> > > > 	xe_guc_submit.o \
> > > > 	xe_guc_tlb_inval.o \
> > > > diff --git a/drivers/gpu/drm/xe/xe_gt.c b/drivers/gpu/drm/xe/xe_gt.c
> > > > index e4852c4c90cd..44ed3202dfd1 100644
> > > > --- a/drivers/gpu/drm/xe/xe_gt.c
> > > > +++ b/drivers/gpu/drm/xe/xe_gt.c
> > > > @@ -32,7 +32,6 @@
> > > > #include "xe_gt_freq.h"
> > > > #include "xe_gt_idle.h"
> > > > #include "xe_gt_mcr.h"
> > > > -#include "xe_gt_pagefault.h"
> > > > #include "xe_gt_printk.h"
> > > > #include "xe_gt_sriov_pf.h"
> > > > #include "xe_gt_sriov_vf.h"
> > > > @@ -638,10 +637,6 @@ int xe_gt_init(struct xe_gt *gt)
> > > > 	if (err)
> > > > 		return err;
> > > >
> > > > -	err = xe_gt_pagefault_init(gt);
> > > > -	if (err)
> > > > -		return err;
> > > > -
> > > > 	err = xe_gt_idle_init(&gt->gtidle);
> > > > 	if (err)
> > > > 		return err;
> > > > @@ -851,7 +846,6 @@ static int gt_reset(struct xe_gt *gt)
> > > > 	xe_uc_gucrc_disable(&gt->uc);
> > > > 	xe_uc_stop_prepare(&gt->uc);
> > > > 	xe_pagefault_reset(gt_to_xe(gt), gt);
> > > > -	xe_gt_pagefault_reset(gt);
> > > >
> > > > 	xe_uc_stop(&gt->uc);
> > > >
> > > > diff --git a/drivers/gpu/drm/xe/xe_guc_ct.c b/drivers/gpu/drm/xe/xe_guc_ct.c
> > > > index e68953ef3a00..529d2b3bec21 100644
> > > > --- a/drivers/gpu/drm/xe/xe_guc_ct.c
> > > > +++ b/drivers/gpu/drm/xe/xe_guc_ct.c
> > > > @@ -21,12 +21,12 @@
> > > > #include "xe_devcoredump.h"
> > > > #include "xe_device.h"
> > > > #include "xe_gt.h"
> > > > -#include "xe_gt_pagefault.h"
> > > > #include "xe_gt_printk.h"
> > > > #include "xe_gt_sriov_pf_control.h"
> > > > #include "xe_gt_sriov_pf_monitor.h"
> > > > #include "xe_guc.h"
> > > > #include "xe_guc_log.h"
> > > > +#include "xe_guc_pagefault.h"
> > > > #include "xe_guc_relay.h"
> > > > #include "xe_guc_submit.h"
> > > > #include "xe_guc_tlb_inval.h"
> > > > @@ -1545,10 +1545,6 @@ static int process_g2h_msg(struct xe_guc_ct *ct, u32 *msg, u32 len)
> > > > 	case XE_GUC_ACTION_TLB_INVALIDATION_DONE:
> > > > 		ret = xe_guc_tlb_inval_done_handler(guc, payload, adj_len);
> > > > 		break;
> > > > -	case XE_GUC_ACTION_ACCESS_COUNTER_NOTIFY:
> > > > -		ret = xe_guc_access_counter_notify_handler(guc, payload,
> > > > -							   adj_len);
> > > > -		break;
> > > 
> > > this doesn't seem to belong here. Should that be in the last patch?
> > 
> > I think it is in this patch as xe_gt_pagefault.h was removed as include
> > and xe_guc_pagefault.h was added. So I think this is probably correct.
> 
> humn... ok, now I see why, but it still looks weird. The justification
> in the next patch for removal was "it's now dead code", but there's no
> mention why we made it dead code in the first place.
> 

Let me adjust the comment in the follow patch.

Matt

> > 
> > > Or a dedicated patch for acc removal?
> > > 
> > > I started to look into this because we need to add support for the
> > > prefetch bit for xe3p, and I think it makes sense to add it on top of
> > > your series rather than conflicting with it.
> > > 
> > 
> > Yes, adding it on top would be preferred from my point of view.
> 
> will do, thanks.
> 
> Lucas De Marchi
> 
> > 
> > Matt
> > 
> > > Lucas De Marchi