[Intel-xe] [PATCH v2 1/2] drm/xe/mtl: Map PPGTT as CPU:WC

[Lucas De Marchi]

On Mon, Jul 24, 2023 at 05:34:34PM -0700, Matt Roper wrote:
>On MTL and beyond, the GPU performs non-coherent accesses to the PPGTT
>page tables.  These page tables should be mapped as CPU:WC.
>
>Removes CAT errors triggered by xe_exec_basic at once-basic on MTL:
>
>   xe 0000:00:02.0: [drm:__xe_pt_bind_vma [xe]] Preparing bind, with range [1a0000...1a0fff) engine 0000000000000000.
>   xe 0000:00:02.0: [drm:xe_vm_dbg_print_entries [xe]] 1 entries to update
>   xe 0000:00:02.0: [drm:xe_vm_dbg_print_entries [xe]]  0: Update level 3 at (0 + 1) [0...8000000000) f:0
>   xe 0000:00:02.0: [drm] Engine memory cat error: guc_id=2
>   xe 0000:00:02.0: [drm] Engine memory cat error: guc_id=2
>   xe 0000:00:02.0: [drm] Timedout job: seqno=4294967169, guc_id=2, flags=0x4
>
>v2:
> - Rename to XE_BO_PAGETABLE to make it more clear that this BO is the
>   pagetable itself, rather than just being bound in the PPGTT.  (Lucas)
>
>Cc: Lucas De Marchi <lucas.demarchi at intel.com>
>Signed-off-by: Matt Roper <matthew.d.roper at intel.com>
>---
> drivers/gpu/drm/xe/xe_bo.c | 17 +++++++++++++----
> drivers/gpu/drm/xe/xe_bo.h |  1 +
> drivers/gpu/drm/xe/xe_pt.c |  3 ++-
> 3 files changed, 16 insertions(+), 5 deletions(-)
>
>diff --git a/drivers/gpu/drm/xe/xe_bo.c b/drivers/gpu/drm/xe/xe_bo.c
>index 538501c46b8b..10858fcf4f62 100644
>--- a/drivers/gpu/drm/xe/xe_bo.c
>+++ b/drivers/gpu/drm/xe/xe_bo.c
>@@ -301,6 +301,7 @@ static struct ttm_tt *xe_ttm_tt_create(struct ttm_buffer_object *ttm_bo,
> 	struct xe_device *xe = xe_bo_device(bo);
> 	struct xe_ttm_tt *tt;
> 	unsigned long extra_pages;
>+	enum ttm_caching caching = ttm_cached;
> 	int err;
>
> 	tt = kzalloc(sizeof(*tt), GFP_KERNEL);
>@@ -314,10 +315,18 @@ static struct ttm_tt *xe_ttm_tt_create(struct ttm_buffer_object *ttm_bo,
> 		extra_pages = DIV_ROUND_UP(xe_device_ccs_bytes(xe, bo->size),
> 					   PAGE_SIZE);
>
>-	/* TODO: Select caching mode */
>-	err = ttm_tt_init(&tt->ttm, &bo->ttm, page_flags,
>-			  bo->flags & XE_BO_SCANOUT_BIT ? ttm_write_combined : ttm_cached,
>-			  extra_pages);
>+	/*
>+	 * Display scanout is always non-coherent with the CPU cache.
>+	 *
>+	 * For Xe_LPG and beyond, PPGTT PTE lookups are also non-coherent and
>+	 * require a CPU:WC mapping.
>+	 *

trailing newline here

>+	 */
>+	if (bo->flags & XE_BO_SCANOUT_BIT ||
>+	    (xe->info.graphics_verx100 >= 1270 && bo->flags & XE_BO_PAGETABLE))
>+		caching = ttm_write_combined;
>+
>+	err = ttm_tt_init(&tt->ttm, &bo->ttm, page_flags, caching, extra_pages);
> 	if (err) {
> 		kfree(tt);
> 		return NULL;
>diff --git a/drivers/gpu/drm/xe/xe_bo.h b/drivers/gpu/drm/xe/xe_bo.h
>index 72c68facd481..43ddfa998141 100644
>--- a/drivers/gpu/drm/xe/xe_bo.h
>+++ b/drivers/gpu/drm/xe/xe_bo.h
>@@ -31,6 +31,7 @@
> #define XE_BO_DEFER_BACKING		BIT(9)
> #define XE_BO_SCANOUT_BIT		BIT(10)
> #define XE_BO_FIXED_PLACEMENT_BIT	BIT(11)
>+#define XE_BO_PAGETABLE			BIT(12)

What do you think about a followup patch to bring the names back to
consistency?

Remove _BIT suffix and use XE_BO_CREATE as namespace.

#define XE_BO_CREATE_USER			BIT(0)
/* The bits below need to be contiguous, or things break */
#define XE_BO_CREATE_SYSTEM			BIT(1)
#define XE_BO_CREATE_VRAM0			BIT(2)
#define XE_BO_CREATE_VRAM1			BIT(3)
#define XE_BO_CREATE_VRAM_MASK          	(XE_BO_CREATE_VRAM0 | \
                                         	 XE_BO_CREATE_VRAM1)

#define XE_BO_CREATE_STOLEN			BIT(4)
#define XE_BO_CREATE_VRAM_IF_DGFX(tile) \
         (IS_DGFX(tile_to_xe(tile)) ? XE_BO_CREATE_VRAM0 << (tile)->id : \
          XE_BO_CREATE_SYSTEM)
#define XE_BO_CREATE_GGTT			BIT(5)
#define XE_BO_CREATE_IGNORE_MIN_PAGE_SIZE	BIT(6)
#define XE_BO_CREATE_PINNED			BIT(7)
#define XE_BO_CREATE_NO_RESV_EVICT		BIT(8)
#define XE_BO_CREATE_DEFER_BACKING		BIT(9)
#define XE_BO_CREATE_SCANOUT			BIT(10)
#define XE_BO_CREATE_FIXED_PLACEMENT		BIT(11)
#define XE_BO_CREATE_PAGETABLE			BIT(12)
/* this one is trigger internally only */
#define XE_BO_CREATE_INTERNAL_TEST		BIT(30)
#define XE_BO_CREATE_INTERNAL_64K		BIT(31)

Up to you if XE_BO_CREATE_PAGETABLE would already be named so in this
patch.

with the newline fix,

	
	Reviewed-by: Lucas De Marchi <lucas.demarchi at intel.com>


Lucas De Marchi

> /* this one is trigger internally only */
> #define XE_BO_INTERNAL_TEST		BIT(30)
> #define XE_BO_INTERNAL_64K		BIT(31)
>diff --git a/drivers/gpu/drm/xe/xe_pt.c b/drivers/gpu/drm/xe/xe_pt.c
>index bbc319fba9e8..d4660520ac2c 100644
>--- a/drivers/gpu/drm/xe/xe_pt.c
>+++ b/drivers/gpu/drm/xe/xe_pt.c
>@@ -221,7 +221,8 @@ struct xe_pt *xe_pt_create(struct xe_vm *vm, struct xe_tile *tile,
> 				  XE_BO_CREATE_VRAM_IF_DGFX(tile) |
> 				  XE_BO_CREATE_IGNORE_MIN_PAGE_SIZE_BIT |
> 				  XE_BO_CREATE_PINNED_BIT |
>-				  XE_BO_CREATE_NO_RESV_EVICT);
>+				  XE_BO_CREATE_NO_RESV_EVICT |
>+				  XE_BO_PAGETABLE);
> 	if (IS_ERR(bo)) {
> 		err = PTR_ERR(bo);
> 		goto err_kfree;
>-- 
>2.41.0
>