Re: [PATCH v8 2/6] drm/xe/xe_gt_pagefault: Move pagefault struct to header

[Michal Wajdeczko <michal.wajdeczko@intel.com>]

On 13.03.2025 19:34, Jonathan Cavitt wrote:
> Move the pagefault struct from xe_gt_pagefault.c to the
> xe_gt_pagefault_types.h header file, along with the associated enum values.
> 
> 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)
> 
> Signed-off-by: Jonathan Cavitt <jonathan.cavitt@intel.com>
> Cc: Michal Wajdeczko <Michal.Wajdeczko@intel.com>
> ---

...

> diff --git a/drivers/gpu/drm/xe/xe_gt_pagefault.h b/drivers/gpu/drm/xe/xe_gt_pagefault.h
> index 839c065a5e4c..69b700c4915a 100644
> --- a/drivers/gpu/drm/xe/xe_gt_pagefault.h
> +++ b/drivers/gpu/drm/xe/xe_gt_pagefault.h
> @@ -8,6 +8,8 @@
>  
>  #include <linux/types.h>
>  
> +#include "xe_gt_pagefault_types.h"

it's not needed here, move it to .c

> +
>  struct xe_gt;
>  struct xe_guc;
>  
> diff --git a/drivers/gpu/drm/xe/xe_gt_pagefault_types.h b/drivers/gpu/drm/xe/xe_gt_pagefault_types.h
> new file mode 100644
> index 000000000000..90b7085d4b8e
> --- /dev/null
> +++ b/drivers/gpu/drm/xe/xe_gt_pagefault_types.h
> @@ -0,0 +1,67 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> + * Copyright © 2022-2025 Intel Corporation
> + */
> +
> +#ifndef _XE_GT_PAGEFAULT_TYPES_H_
> +#define _XE_GT_PAGEFAULT_TYPES_H_
> +

don't forget to

#include <linux/types.h>

> +/**
> + * struct xe_gt_pagefault - Structure of pagefaults returned by the
> + * pagefault handler
> + */
> +struct xe_gt_pagefault {
> +	/** @page_addr: faulted address of this pagefault */
> +	u64 page_addr;
> +	/** @asid: ASID of this pagefault */
> +	u32 asid;
> +	/** @pdata: PDATA of this pagefault */
> +	u16 pdata;
> +	/** @vfid: VFID of this pagefault */
> +	u8 vfid;

btw, IIRC the VFID from the descriptor will be zero'ed
does it make sense to keep it here?

> +	/**
> +	 * @access_type: access type of this pagefault, as a value
> +	 * from xe_gt_pagefault_access_type
> +	 */
> +	u8 access_type;
> +	/**
> +	 * @fault_type: fault type of this pagefault, as a value
> +	 * from xe_gt_pagefault_fault_type
> +	 */
> +	u8 fault_type;
> +	/** @fault_level: fault level of this pagefault */
> +	u8 fault_level;
> +	/** @engine_class: engine class this pagefault was reported on */
> +	u8 engine_class;
> +	/** @engine_instance: engine instance this pagefault was reported on */
> +	u8 engine_instance;
> +	/** @fault_unsuccessful: flag for if the pagefault recovered or not */
> +	u8 fault_unsuccessful;
> +	/** @prefetch: unused */
> +	bool prefetch;
> +	/** @trva_fault: is set if this is a TRTT fault */
> +	bool trva_fault;
> +};
> +
> +/**
> + * enum xe_gt_pagefault_access_type - Access type reported to the xe_gt_pagefault
> + * struct.  Saved to xe_gt_pagefault@access_type

this seems to be copied from G2H descriptor as-is.
so shouldn't this be part of the GuC ABI?
or based on HW ABI if GuC is just a proxy

> + */
> +enum xe_gt_pagefault_access_type {
> +	XE_GT_PAGEFAULT_ACCESS_TYPE_READ = 0,
> +	XE_GT_PAGEFAULT_ACCESS_TYPE_WRITE = 1,
> +	XE_GT_PAGEFAULT_ACCESS_TYPE_ATOMIC = 2,
> +	XE_GT_PAGEFAULT_ACCESS_TYPE_RESERVED = 3,
> +};
> +
> +/**
> + * enum xe_gt_pagefault_fault_type - Fault type reported to the xe_gt_pagefault
> + * struct.  Saved to xe_gt_pagefault@fault_type

ditto

> + */
> +enum xe_gt_pagefault_fault_type {
> +	XE_GT_PAGEFAULT_TYPE_NOT_PRESENT = 0,
> +	XE_GT_PAGEFAULT_TYPE_WRITE_ACCESS_VIOLATION = 1,
> +	XE_GT_PAGEFAULT_TYPE_ATOMIC_ACCESS_VIOLATION = 2,
> +};
> +
> +#endif