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

[Matthew Auld]

On 21/07/2023 21:54, 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)
> 
> 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.

Do you mean make it conditional on XE_BO_NEEDS_CPU_ACCESS && 
XE_BO_CREATE_PPGTT_BIT, or what did you have in mind here?

> 
> 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
>>