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

[Lucas De Marchi]

On Mon, Jul 24, 2023 at 08:16:18AM -0700, Matt Roper wrote:
>On Fri, Jul 21, 2023 at 01:54:02PM -0700, Lucas De Marchi wrote:
>> On Fri, Jul 21, 2023 at 10:55:58AM -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
>> >
>> > 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 828a9fea420b..d04538e65f11 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.
>> > +	 *
>> > +	 */
>> > +	if (bo->flags & XE_BO_SCANOUT_BIT ||
>> > +	    (xe->info.graphics_verx100 >= 1270 && bo->flags & XE_BO_PPGTT_BIT))
>> > +		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..fd103b64f446 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_PPGTT_BIT			BIT(12)
>>
>> we have XE_BO_CREATE_GGTT_BIT. To make name consistent and close to a
>> similar flag, this should probably be
>>
>> #define XE_BO_CREATE_PPGTT_BIT		BIT(6)
>
>We actually *don't* want consistent naming here since these are
>completely different things.  GGTT_BIT indicates that the object is
>intended to be bound into the GGTT's address space, whereas PPGTT_BIT
>indicates that the object *is* the PPGTT.  Maybe I should rename this
>one to XE_BO_PAGETABLE_BIT or something to make them less similar.

that sounds better to me

Lucas De Marchi

>
>
>Matt
>
>>
>> While at it, let's remove the _BIT that is agains our naming
>> convention.
>>
>> Although here I think what we actually need is the XE_BO_NEEDS_CPU_ACCESS
>> from Matt Auld's  series. Cc'ing him.
>>
>> 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 a458224a88ca..fe91b10f951e 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_PPGTT_BIT);
>> > 	if (IS_ERR(bo)) {
>> > 		err = PTR_ERR(bo);
>> > 		goto err_kfree;
>> > --
>> > 2.41.0
>> >
>
>-- 
>Matt Roper
>Graphics Software Engineer
>Linux GPU Platform Enablement
>Intel Corporation