[PATCH] drm/xe: Implement print vfunc for page fault info logging

[PATCH] drm/xe: Implement print vfunc for page fault info logging

[Brian Nguyen]

Add a print vfunc to xe_pagefault_ops so different producer backends can
log specific relevant debug info while keeping the consumer layer free
of potential backend-specific formatting logic.

Consumer-specific prints are handled separately outside of the vfunc.

Suggested-by: Stuart Summers <stuart.summers@intel.com>
Signed-off-by: Brian Nguyen <brian3.nguyen@intel.com>
---
 drivers/gpu/drm/xe/xe_guc_pagefault.c   | 35 +++++++++++++++++++++++++
 drivers/gpu/drm/xe/xe_pagefault.c       | 33 +++--------------------
 drivers/gpu/drm/xe/xe_pagefault_types.h |  8 ++++++
 3 files changed, 46 insertions(+), 30 deletions(-)

diff --git a/drivers/gpu/drm/xe/xe_guc_pagefault.c b/drivers/gpu/drm/xe/xe_guc_pagefault.c
index df237fd40551..7e4f0df1444b 100644
--- a/drivers/gpu/drm/xe/xe_guc_pagefault.c
+++ b/drivers/gpu/drm/xe/xe_guc_pagefault.c
@@ -4,9 +4,11 @@
  */
 
 #include "abi/guc_actions_abi.h"
+#include "xe_gt_printk.h"
 #include "xe_guc.h"
 #include "xe_guc_ct.h"
 #include "xe_guc_pagefault.h"
+#include "xe_hw_engine.h"
 #include "xe_pagefault.h"
 #include "xe_pagefault_types.h"
 
@@ -68,10 +70,43 @@ static void guc_ack_fault_end(void *private)
 	xe_guc_ct_unlock(&guc->ct);
 }
 
+static void xe_guc_pagefault_print(struct xe_pagefault *pf)
+{
+	u8 engine_class = FIELD_GET(XE_PAGEFAULT_ENGINE_CLASS_MASK,
+				    pf->consumer.engine_class_instance);
+	u64 addr = xe_pagefault_addr(pf);
+
+	xe_gt_info(pf->gt, "\n\tASID: %lu\n"
+		   "\tFaulted Address: 0x%08x%08x\n"
+		   "\tFaultType: %lu\n"
+		   "\tAccessType: %lu\n"
+		   "\tFaultLevel: %lu\n"
+		   "\tEngineClass: %d %s\n"
+		   "\tEngineInstance: %lu\n"
+		   "\tSRCID: 0x%02lx\n",
+		   FIELD_GET(XE_PAGEFAULT_ASID_MASK,
+			     pf->consumer.id),
+		   upper_32_bits(addr),
+		   lower_32_bits(addr),
+		   FIELD_GET(XE_PAGEFAULT_TYPE_MASK,
+			     pf->consumer.fault_type_level),
+		   FIELD_GET(XE_PAGEFAULT_ACCESS_TYPE_MASK,
+			     pf->consumer.access_type),
+		   FIELD_GET(XE_PAGEFAULT_LEVEL_MASK,
+			     pf->consumer.fault_type_level),
+		   engine_class,
+		   xe_hw_engine_class_to_str(engine_class),
+		   FIELD_GET(XE_PAGEFAULT_ENGINE_INSTANCE_MASK,
+			     pf->consumer.engine_class_instance),
+		   FIELD_GET(XE_PAGEFAULT_SRCID_MASK,
+			     pf->consumer.id));
+}
+
 static const struct xe_pagefault_ops guc_pagefault_ops = {
 	.ack_fault_begin = guc_ack_fault_begin,
 	.ack_fault = guc_ack_fault,
 	.ack_fault_end = guc_ack_fault_end,
+	.print = xe_guc_pagefault_print,
 };
 
 /**
diff --git a/drivers/gpu/drm/xe/xe_pagefault.c b/drivers/gpu/drm/xe/xe_pagefault.c
index f9e5bd41cfd1..298a67268c0d 100644
--- a/drivers/gpu/drm/xe/xe_pagefault.c
+++ b/drivers/gpu/drm/xe/xe_pagefault.c
@@ -13,7 +13,6 @@
 #include "xe_gt_printk.h"
 #include "xe_gt_types.h"
 #include "xe_gt_stats.h"
-#include "xe_hw_engine.h"
 #include "xe_log.h"
 #include "xe_pagefault.h"
 #include "xe_pagefault_types.h"
@@ -607,35 +606,9 @@ static const char *xe_pagefault_error_to_str(enum xe_pagefault_error error)
 
 static void xe_pagefault_print(struct xe_pagefault *pf)
 {
-	u8 engine_class = FIELD_GET(XE_PAGEFAULT_ENGINE_CLASS_MASK,
-				    pf->consumer.engine_class_instance);
-	u64 addr = xe_pagefault_addr(pf);
-
-	xe_gt_info(pf->gt, "\n\tASID: %lu\n"
-		   "\tFaulted Address: 0x%08x%08x\n"
-		   "\tFaultType: %lu\n"
-		   "\tAccessType: %lu\n"
-		   "\tFaultLevel: %lu\n"
-		   "\tEngineClass: %d %s\n"
-		   "\tEngineInstance: %lu\n"
-		   "\tSRCID: 0x%02lx\n"
-		   "\tError: %s\n",
-		   FIELD_GET(XE_PAGEFAULT_ASID_MASK,
-			     pf->consumer.id),
-		   upper_32_bits(addr),
-		   lower_32_bits(addr),
-		   FIELD_GET(XE_PAGEFAULT_TYPE_MASK,
-			     pf->consumer.fault_type_level),
-		   FIELD_GET(XE_PAGEFAULT_ACCESS_TYPE_MASK,
-			     pf->consumer.access_type),
-		   FIELD_GET(XE_PAGEFAULT_LEVEL_MASK,
-			     pf->consumer.fault_type_level),
-		   engine_class,
-		   xe_hw_engine_class_to_str(engine_class),
-		   FIELD_GET(XE_PAGEFAULT_ENGINE_INSTANCE_MASK,
-			     pf->consumer.engine_class_instance),
-		   FIELD_GET(XE_PAGEFAULT_SRCID_MASK,
-			     pf->consumer.id),
+	xe_gt_assert(pf->gt, pf->producer.ops->print);
+	pf->producer.ops->print(pf);
+	xe_gt_info(pf->gt, "\tError: %s\n",
 		   xe_pagefault_error_to_str(xe_pagefault_get_error(pf)));
 }
 
diff --git a/drivers/gpu/drm/xe/xe_pagefault_types.h b/drivers/gpu/drm/xe/xe_pagefault_types.h
index 8ae6b9848bdc..72568ab9686a 100644
--- a/drivers/gpu/drm/xe/xe_pagefault_types.h
+++ b/drivers/gpu/drm/xe/xe_pagefault_types.h
@@ -139,6 +139,14 @@ struct xe_pagefault_ops {
 	 * Page fault producer ends acknowledgment from the consumer.
 	 */
 	void (*ack_fault_end)(void *private);
+	/**
+	 * @print: Print pagefault info
+	 * @pf: Page fault
+	 *
+	 * Print pagefault identification fields for the fault.
+	 * Must be implemented by all producers.
+	 */
+	void (*print)(struct xe_pagefault *pf);
 };
 
 /**
-- 
2.43.0

Re: [PATCH] drm/xe: Implement print vfunc for page fault info logging

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] xe: log interleaving and redundant prefixes due to split print calls
--

commit 114cd7fd731f9ee67d8c36081ae1b0f44d9be562
Author: Brian Nguyen <brian3.nguyen@intel.com>

drm/xe: Implement print vfunc for page fault info logging

This commit adds a print vfunc to xe_pagefault_ops to let producer
backends log specific relevant debug info. This keeps the consumer
layer free of backend-specific formatting logic, with consumer-specific
prints handled separately.

> diff --git a/drivers/gpu/drm/xe/xe_pagefault.c b/drivers/gpu/drm/xe/xe_pagefault.c
> --- a/drivers/gpu/drm/xe/xe_pagefault.c
> +++ b/drivers/gpu/drm/xe/xe_pagefault.c
> @@ -607,35 +606,9 @@ static const char *xe_pagefault_error_to_str(enum xe_pagefault_error error)
>  
>  static void xe_pagefault_print(struct xe_pagefault *pf)
>  {

[ ... ]

> -		   FIELD_GET(XE_PAGEFAULT_ENGINE_INSTANCE_MASK,
> -			     pf->consumer.engine_class_instance),
> -		   FIELD_GET(XE_PAGEFAULT_SRCID_MASK,
> -			     pf->consumer.id),
> +	xe_gt_assert(pf->gt, pf->producer.ops->print);
> +	pf->producer.ops->print(pf);
> +	xe_gt_info(pf->gt, "\tError: %s\n",
>  		   xe_pagefault_error_to_str(xe_pagefault_get_error(pf)));
>  }

[Severity: Low]
Does this split print call cause log interleaving and redundant prefixes?

Because the error string is now logged with a separate call to xe_gt_info()
in xe_pagefault_print(), it will be printed as an isolated log entry with
its own prefix, rather than forming a continuous atomic block with the fault
details from the producer print vfunc.

This disrupts the formatting of the page fault debug info, introducing
redundant logging prefixes in the middle of what used to be a single
continuous multiline log block. It also creates a risk of concurrent kernel
messages interleaving with the output.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260925232757.3456172-2-brian3.nguyen@intel.com?part=1

Re: [PATCH] drm/xe: Implement print vfunc for page fault info logging

[Matthew Brost]

On Fri, Sep 25, 2026 at 11:38:52PM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Low] xe: log interleaving and redundant prefixes due to split print calls
> --
> 
> commit 114cd7fd731f9ee67d8c36081ae1b0f44d9be562
> Author: Brian Nguyen <brian3.nguyen@intel.com>
> 
> drm/xe: Implement print vfunc for page fault info logging
> 
> This commit adds a print vfunc to xe_pagefault_ops to let producer
> backends log specific relevant debug info. This keeps the consumer
> layer free of backend-specific formatting logic, with consumer-specific
> prints handled separately.
> 
> > diff --git a/drivers/gpu/drm/xe/xe_pagefault.c b/drivers/gpu/drm/xe/xe_pagefault.c
> > --- a/drivers/gpu/drm/xe/xe_pagefault.c
> > +++ b/drivers/gpu/drm/xe/xe_pagefault.c
> > @@ -607,35 +606,9 @@ static const char *xe_pagefault_error_to_str(enum xe_pagefault_error error)
> >  
> >  static void xe_pagefault_print(struct xe_pagefault *pf)
> >  {
> 
> [ ... ]
> 
> > -		   FIELD_GET(XE_PAGEFAULT_ENGINE_INSTANCE_MASK,
> > -			     pf->consumer.engine_class_instance),
> > -		   FIELD_GET(XE_PAGEFAULT_SRCID_MASK,
> > -			     pf->consumer.id),
> > +	xe_gt_assert(pf->gt, pf->producer.ops->print);
> > +	pf->producer.ops->print(pf);
> > +	xe_gt_info(pf->gt, "\tError: %s\n",
> >  		   xe_pagefault_error_to_str(xe_pagefault_get_error(pf)));
> >  }
> 
> [Severity: Low]
> Does this split print call cause log interleaving and redundant prefixes?
> 
> Because the error string is now logged with a separate call to xe_gt_info()
> in xe_pagefault_print(), it will be printed as an isolated log entry with
> its own prefix, rather than forming a continuous atomic block with the fault
> details from the producer print vfunc.
> 
> This disrupts the formatting of the page fault debug info, introducing
> redundant logging prefixes in the middle of what used to be a single
> continuous multiline log block. It also creates a risk of concurrent kernel
> messages interleaving with the output.
> 

This is valid too. So either make print accept the output of
xe_pagefault_error_to_str as an argument and print the entire thing in
one shot.

Matt

> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260925232757.3456172-2-brian3.nguyen@intel.com?part=1

RE: [PATCH] drm/xe: Implement print vfunc for page fault info logging

[Nguyen, Brian3]

On Friday, September 25, 2026 7:18 PM, Matthew Brost wrote:
> On Fri, Sep 25, 2026 at 11:38:52PM +0000, sashiko-bot@kernel.org wrote:
> > Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> > - [Low] xe: log interleaving and redundant prefixes due to split print calls
> > --
> >
> > commit 114cd7fd731f9ee67d8c36081ae1b0f44d9be562
> > Author: Brian Nguyen <brian3.nguyen@intel.com>
> >
> > drm/xe: Implement print vfunc for page fault info logging
> >
> > This commit adds a print vfunc to xe_pagefault_ops to let producer
> > backends log specific relevant debug info. This keeps the consumer
> > layer free of backend-specific formatting logic, with consumer-specific
> > prints handled separately.
> >
> > > diff --git a/drivers/gpu/drm/xe/xe_pagefault.c b/drivers/gpu/drm/xe/xe_pagefault.c
> > > --- a/drivers/gpu/drm/xe/xe_pagefault.c
> > > +++ b/drivers/gpu/drm/xe/xe_pagefault.c
> > > @@ -607,35 +606,9 @@ static const char *xe_pagefault_error_to_str(enum xe_pagefault_error error)
> > >
> > >  static void xe_pagefault_print(struct xe_pagefault *pf)
> > >  {
> >
> > [ ... ]
> >
> > > -		   FIELD_GET(XE_PAGEFAULT_ENGINE_INSTANCE_MASK,
> > > -			     pf->consumer.engine_class_instance),
> > > -		   FIELD_GET(XE_PAGEFAULT_SRCID_MASK,
> > > -			     pf->consumer.id),
> > > +	xe_gt_assert(pf->gt, pf->producer.ops->print);
> > > +	pf->producer.ops->print(pf);
> > > +	xe_gt_info(pf->gt, "\tError: %s\n",
> > >  		   xe_pagefault_error_to_str(xe_pagefault_get_error(pf)));
> > >  }
> >
> > [Severity: Low]
> > Does this split print call cause log interleaving and redundant prefixes?
> >
> > Because the error string is now logged with a separate call to xe_gt_info()
> > in xe_pagefault_print(), it will be printed as an isolated log entry with
> > its own prefix, rather than forming a continuous atomic block with the fault
> > details from the producer print vfunc.
> >
> > This disrupts the formatting of the page fault debug info, introducing
> > redundant logging prefixes in the middle of what used to be a single
> > continuous multiline log block. It also creates a risk of concurrent kernel
> > messages interleaving with the output.
> >
> 
> This is valid too. So either make print accept the output of
> xe_pagefault_error_to_str as an argument and print the entire thing in
> one shot.
> 

Will do. I'll take a string as an additional input for the vfunc:
	void (*print)(struct xe_pagefault *pf, const char *err_str);

Brian

> Matt
> 
> > --
> > Sashiko AI review · https://sashiko.dev/#/patchset/20260925232757.3456172-2-brian3.nguyen@intel.com?part=1

Re: [PATCH] drm/xe: Implement print vfunc for page fault info logging

[Matthew Brost]

On Fri, Sep 25, 2026 at 11:27:58PM +0000, Brian Nguyen wrote:
> Add a print vfunc to xe_pagefault_ops so different producer backends can
> log specific relevant debug info while keeping the consumer layer free
> of potential backend-specific formatting logic.
> 
> Consumer-specific prints are handled separately outside of the vfunc.
> 
> Suggested-by: Stuart Summers <stuart.summers@intel.com>
> Signed-off-by: Brian Nguyen <brian3.nguyen@intel.com>
> ---
>  drivers/gpu/drm/xe/xe_guc_pagefault.c   | 35 +++++++++++++++++++++++++
>  drivers/gpu/drm/xe/xe_pagefault.c       | 33 +++--------------------
>  drivers/gpu/drm/xe/xe_pagefault_types.h |  8 ++++++
>  3 files changed, 46 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_guc_pagefault.c b/drivers/gpu/drm/xe/xe_guc_pagefault.c
> index df237fd40551..7e4f0df1444b 100644
> --- a/drivers/gpu/drm/xe/xe_guc_pagefault.c
> +++ b/drivers/gpu/drm/xe/xe_guc_pagefault.c
> @@ -4,9 +4,11 @@
>   */
>  
>  #include "abi/guc_actions_abi.h"
> +#include "xe_gt_printk.h"
>  #include "xe_guc.h"
>  #include "xe_guc_ct.h"
>  #include "xe_guc_pagefault.h"
> +#include "xe_hw_engine.h"
>  #include "xe_pagefault.h"
>  #include "xe_pagefault_types.h"
>  
> @@ -68,10 +70,43 @@ static void guc_ack_fault_end(void *private)
>  	xe_guc_ct_unlock(&guc->ct);
>  }
>  
> +static void xe_guc_pagefault_print(struct xe_pagefault *pf)
> +{
> +	u8 engine_class = FIELD_GET(XE_PAGEFAULT_ENGINE_CLASS_MASK,
> +				    pf->consumer.engine_class_instance);
> +	u64 addr = xe_pagefault_addr(pf);
> +

In general if this vfunc is in the producer layer it should extract
everything from producer part of the page fault otherwise the vfunc is a
bit pointless. That said I do think a vfunc makes sense as it could have
more information than the consumer.

Matt

> +	xe_gt_info(pf->gt, "\n\tASID: %lu\n"
> +		   "\tFaulted Address: 0x%08x%08x\n"
> +		   "\tFaultType: %lu\n"
> +		   "\tAccessType: %lu\n"
> +		   "\tFaultLevel: %lu\n"
> +		   "\tEngineClass: %d %s\n"
> +		   "\tEngineInstance: %lu\n"
> +		   "\tSRCID: 0x%02lx\n",
> +		   FIELD_GET(XE_PAGEFAULT_ASID_MASK,
> +			     pf->consumer.id),
> +		   upper_32_bits(addr),
> +		   lower_32_bits(addr),
> +		   FIELD_GET(XE_PAGEFAULT_TYPE_MASK,
> +			     pf->consumer.fault_type_level),
> +		   FIELD_GET(XE_PAGEFAULT_ACCESS_TYPE_MASK,
> +			     pf->consumer.access_type),
> +		   FIELD_GET(XE_PAGEFAULT_LEVEL_MASK,
> +			     pf->consumer.fault_type_level),
> +		   engine_class,
> +		   xe_hw_engine_class_to_str(engine_class),
> +		   FIELD_GET(XE_PAGEFAULT_ENGINE_INSTANCE_MASK,
> +			     pf->consumer.engine_class_instance),
> +		   FIELD_GET(XE_PAGEFAULT_SRCID_MASK,
> +			     pf->consumer.id));
> +}
> +
>  static const struct xe_pagefault_ops guc_pagefault_ops = {
>  	.ack_fault_begin = guc_ack_fault_begin,
>  	.ack_fault = guc_ack_fault,
>  	.ack_fault_end = guc_ack_fault_end,
> +	.print = xe_guc_pagefault_print,
>  };
>  
>  /**
> diff --git a/drivers/gpu/drm/xe/xe_pagefault.c b/drivers/gpu/drm/xe/xe_pagefault.c
> index f9e5bd41cfd1..298a67268c0d 100644
> --- a/drivers/gpu/drm/xe/xe_pagefault.c
> +++ b/drivers/gpu/drm/xe/xe_pagefault.c
> @@ -13,7 +13,6 @@
>  #include "xe_gt_printk.h"
>  #include "xe_gt_types.h"
>  #include "xe_gt_stats.h"
> -#include "xe_hw_engine.h"
>  #include "xe_log.h"
>  #include "xe_pagefault.h"
>  #include "xe_pagefault_types.h"
> @@ -607,35 +606,9 @@ static const char *xe_pagefault_error_to_str(enum xe_pagefault_error error)
>  
>  static void xe_pagefault_print(struct xe_pagefault *pf)
>  {
> -	u8 engine_class = FIELD_GET(XE_PAGEFAULT_ENGINE_CLASS_MASK,
> -				    pf->consumer.engine_class_instance);
> -	u64 addr = xe_pagefault_addr(pf);
> -
> -	xe_gt_info(pf->gt, "\n\tASID: %lu\n"
> -		   "\tFaulted Address: 0x%08x%08x\n"
> -		   "\tFaultType: %lu\n"
> -		   "\tAccessType: %lu\n"
> -		   "\tFaultLevel: %lu\n"
> -		   "\tEngineClass: %d %s\n"
> -		   "\tEngineInstance: %lu\n"
> -		   "\tSRCID: 0x%02lx\n"
> -		   "\tError: %s\n",
> -		   FIELD_GET(XE_PAGEFAULT_ASID_MASK,
> -			     pf->consumer.id),
> -		   upper_32_bits(addr),
> -		   lower_32_bits(addr),
> -		   FIELD_GET(XE_PAGEFAULT_TYPE_MASK,
> -			     pf->consumer.fault_type_level),
> -		   FIELD_GET(XE_PAGEFAULT_ACCESS_TYPE_MASK,
> -			     pf->consumer.access_type),
> -		   FIELD_GET(XE_PAGEFAULT_LEVEL_MASK,
> -			     pf->consumer.fault_type_level),
> -		   engine_class,
> -		   xe_hw_engine_class_to_str(engine_class),
> -		   FIELD_GET(XE_PAGEFAULT_ENGINE_INSTANCE_MASK,
> -			     pf->consumer.engine_class_instance),
> -		   FIELD_GET(XE_PAGEFAULT_SRCID_MASK,
> -			     pf->consumer.id),
> +	xe_gt_assert(pf->gt, pf->producer.ops->print);
> +	pf->producer.ops->print(pf);
> +	xe_gt_info(pf->gt, "\tError: %s\n",
>  		   xe_pagefault_error_to_str(xe_pagefault_get_error(pf)));
>  }
>  
> diff --git a/drivers/gpu/drm/xe/xe_pagefault_types.h b/drivers/gpu/drm/xe/xe_pagefault_types.h
> index 8ae6b9848bdc..72568ab9686a 100644
> --- a/drivers/gpu/drm/xe/xe_pagefault_types.h
> +++ b/drivers/gpu/drm/xe/xe_pagefault_types.h
> @@ -139,6 +139,14 @@ struct xe_pagefault_ops {
>  	 * Page fault producer ends acknowledgment from the consumer.
>  	 */
>  	void (*ack_fault_end)(void *private);
> +	/**
> +	 * @print: Print pagefault info
> +	 * @pf: Page fault
> +	 *
> +	 * Print pagefault identification fields for the fault.
> +	 * Must be implemented by all producers.
> +	 */
> +	void (*print)(struct xe_pagefault *pf);
>  };
>  
>  /**
> -- 
> 2.43.0
>