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

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

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
>