Re: [Intel-gfx] [PATCH 05/12] drm/i915/xehp: Check for faults on primary GAM

[Balasubramani Vivekanandan <balasubramani.vivekanandan@intel.com>]

On 19.09.2022 15:32, Matt Roper wrote:
> On Xe_HP the fault registers are now in a multicast register range.
> However as part of the GAM these registers follow special rules and we
> need only read from the "primary" GAM's instance to get the information
> we need.  So a single intel_gt_mcr_read_any() (which will automatically
> steer to the primary GAM) is sufficient; we don't need to loop over each
> instance of the MCR register.
> 
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
>  drivers/gpu/drm/i915/gt/intel_gt.c      | 40 ++++++++++++++++++++++++-
>  drivers/gpu/drm/i915/gt/intel_gt_regs.h |  3 ++
>  2 files changed, 42 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c b/drivers/gpu/drm/i915/gt/intel_gt.c
> index 5ddae95d4886..1cb7dd40ec47 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt.c
> +++ b/drivers/gpu/drm/i915/gt/intel_gt.c
> @@ -304,6 +304,42 @@ static void gen6_check_faults(struct intel_gt *gt)
>  	}
>  }
>  
> +static void xehp_check_faults(struct intel_gt *gt)
> +{
> +	u32 fault;
> +
> +	/*
> +	 * Although the fault register now lives in an MCR register range,
> +	 * the GAM registers are special and we only truly need to read
> +	 * the "primary" GAM instance rather than handling each instance
> +	 * individually.  intel_gt_mcr_read_any() will automatically steer
> +	 * toward the primary instance.
> +	 */
> +	fault = intel_gt_mcr_read_any(gt, XEHP_RING_FAULT_REG);
> +	if (fault & RING_FAULT_VALID) {
> +		u32 fault_data0, fault_data1;
> +		u64 fault_addr;
> +
> +		fault_data0 = intel_gt_mcr_read_any(gt, XEHP_FAULT_TLB_DATA0);
> +		fault_data1 = intel_gt_mcr_read_any(gt, XEHP_FAULT_TLB_DATA1);
> +
> +		fault_addr = ((u64)(fault_data1 & FAULT_VA_HIGH_BITS) << 44) |
> +			     ((u64)fault_data0 << 12);
> +
> +		drm_dbg(&gt->i915->drm, "Unexpected fault\n"
> +			"\tAddr: 0x%08x_%08x\n"
> +			"\tAddress space: %s\n"
> +			"\tEngine ID: %d\n"
> +			"\tSource ID: %d\n"
> +			"\tType: %d\n",
> +			upper_32_bits(fault_addr), lower_32_bits(fault_addr),
> +			fault_data1 & FAULT_GTT_SEL ? "GGTT" : "PPGTT",
> +			GEN8_RING_FAULT_ENGINE_ID(fault),
> +			RING_FAULT_SRCID(fault),
> +			RING_FAULT_FAULT_TYPE(fault));
> +	}
> +}
> +
>  static void gen8_check_faults(struct intel_gt *gt)
>  {
>  	struct intel_uncore *uncore = gt->uncore;
> @@ -350,7 +386,9 @@ void intel_gt_check_and_clear_faults(struct intel_gt *gt)
>  	struct drm_i915_private *i915 = gt->i915;
>  
>  	/* From GEN8 onwards we only have one 'All Engine Fault Register' */
> -	if (GRAPHICS_VER(i915) >= 8)
> +	if (GRAPHICS_VER_FULL(i915) >= IP_VER(12, 50))
> +		xehp_check_faults(gt);
> +	else if (GRAPHICS_VER(i915) >= 8)
>  		gen8_check_faults(gt);
>  	else if (GRAPHICS_VER(i915) >= 6)
>  		gen6_check_faults(gt);
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_regs.h b/drivers/gpu/drm/i915/gt/intel_gt_regs.h
> index cf87a1b36a21..dff38b0c4430 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt_regs.h
> +++ b/drivers/gpu/drm/i915/gt/intel_gt_regs.h
> @@ -1024,11 +1024,14 @@
>  #define GEN9_BLT_MOCS(i)			_MMIO(__GEN9_BCS0_MOCS0 + (i) * 4)
>  
>  #define GEN12_FAULT_TLB_DATA0			_MMIO(0xceb8)
> +#define XEHP_FAULT_TLB_DATA0			_MMIO(0xceb8)
>  #define GEN12_FAULT_TLB_DATA1			_MMIO(0xcebc)
> +#define XEHP_FAULT_TLB_DATA1			_MMIO(0xcebc)
>  #define   FAULT_VA_HIGH_BITS			(0xf << 0)
>  #define   FAULT_GTT_SEL				(1 << 4)
>  
>  #define GEN12_RING_FAULT_REG			_MMIO(0xcec4)
> +#define XEHP_RING_FAULT_REG			_MMIO(0xcec4)

The fault registers GEN12_FAULT_TLB_DATA0, GEN12_FAULT_TLB_DATA1,
GEN12_RING_FAULT_REG are used in few other places in the driver for
platforms including Xe_HP. Don't we need to take care of them?

Regards,
Bala

>  #define   GEN8_RING_FAULT_ENGINE_ID(x)		(((x) >> 12) & 0x7)
>  #define   RING_FAULT_GTTSEL_MASK		(1 << 11)
>  #define   RING_FAULT_SRCID(x)			(((x) >> 3) & 0xff)
> -- 
> 2.37.3
>