[Intel-xe] [PATCH v4 2/6] drm/xe: add xe_ttm_stolen_cpu_access_needs_ggtt()

[Matthew Auld]

On 14/03/2023 09:53, Thomas Hellström wrote:
> 
> On 3/14/23 09:58, Matthew Auld wrote:
>> xe_ttm_stolen_cpu_inaccessible() was originally meant to just cover the
>> case where stolen is not directly CPU accessible on some older
>> integrated platforms, and as such a GGTT mapping was also required for
>> CPU access (as per the check in xe_bo_create_pin_map_at()).
>>
>> However with small-bar systems on dgfx we have one more case where
>> stolen is also inaccessible, however here we don't have any fallback
>> GGTT mode for CPU access. Fix the check in xe_bo_create_pin_map_at() to
>> make this distinction clear. In such a case the later vmap() will fail
>> anyway.
>>
>> v2: fix kernel-doc warning
>> v3: Simplify further and remove cpu_inaccessible()
>>
>> Suggested-by: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
>> Signed-off-by: Matthew Auld <matthew.auld at intel.com>
>> Cc: Gwan-gyeong Mun <gwan-gyeong.mun at intel.com>
>> ---
>>   drivers/gpu/drm/xe/xe_bo.c             |  2 +-
>>   drivers/gpu/drm/xe/xe_ttm_stolen_mgr.c | 39 ++++++++------------------
>>   drivers/gpu/drm/xe/xe_ttm_stolen_mgr.h |  2 +-
>>   3 files changed, 14 insertions(+), 29 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_bo.c b/drivers/gpu/drm/xe/xe_bo.c
>> index 3e90ea42e075..73a7f2cd4ad8 100644
>> --- a/drivers/gpu/drm/xe/xe_bo.c
>> +++ b/drivers/gpu/drm/xe/xe_bo.c
>> @@ -1151,7 +1151,7 @@ struct xe_bo *xe_bo_create_pin_map_at(struct 
>> xe_device *xe, struct xe_gt *gt,
>>       u64 end = offset == ~0ull ? offset : start + size;
>>       if (flags & XE_BO_CREATE_STOLEN_BIT &&
>> -        xe_ttm_stolen_cpu_inaccessible(xe))
>> +        xe_ttm_stolen_cpu_access_needs_ggtt(xe))
>>           flags |= XE_BO_CREATE_GGTT_BIT;
>>       bo = xe_bo_create_locked_range(xe, gt, vm, size, start, end, 
>> type, flags);
>> diff --git a/drivers/gpu/drm/xe/xe_ttm_stolen_mgr.c 
>> b/drivers/gpu/drm/xe/xe_ttm_stolen_mgr.c
>> index 27cc31f022a5..8759c13acd9b 100644
>> --- a/drivers/gpu/drm/xe/xe_ttm_stolen_mgr.c
>> +++ b/drivers/gpu/drm/xe/xe_ttm_stolen_mgr.c
>> @@ -38,32 +38,17 @@ to_stolen_mgr(struct ttm_resource_manager *man)
>>   }
>>   /**
>> - * xe_ttm_stolen_cpu_inaccessible - Can we directly CPU access stolen 
>> memory for
>> - * this device.
>> + * xe_ttm_stolen_cpu_access_needs_ggtt - If we can't directly CPU 
>> access stolen,
> 
> Nit: Should be xe_ttm_stolen_cpu_access_needs_ggtt() ?

Do you just mean I need the brackets? Ctrl-F is not helping me...maybe 
I'm blind.

> 
> Otherwise LGTM.

Thanks.

> 
> /Thomas
> 
> 
>> + * can we then fallback to mapping through the GGTT.
>>    * @xe: xe device
>>    *
>> - * On some integrated platforms we can't directly access stolen via 
>> the CPU
>> - * (like some normal system memory).  Also on small-bar systems for 
>> discrete,
>> - * since stolen is always as the end of normal VRAM, and the BAR 
>> likely doesn't
>> - * stretch that far. However CPU access of stolen is generally rare, 
>> and at
>> - * least on discrete should not be needed.
>> - *
>> - * If this is indeed inaccessible then we fallback to using the GGTT 
>> mappable
>> - * aperture for CPU access. On discrete platforms we have no such 
>> thing, so when
>> - * later attempting to CPU map the memory an error is instead thrown.
>> + * Some older integrated platforms don't support reliable CPU access 
>> for stolen,
>> + * however on such hardware we can always use the mappable part of 
>> the GGTT for
>> + * CPU access. Check if that's the case for this device.
>>    */
>> -bool xe_ttm_stolen_cpu_inaccessible(struct xe_device *xe)
>> +bool xe_ttm_stolen_cpu_access_needs_ggtt(struct xe_device *xe)
>>   {
>> -    struct ttm_resource_manager *ttm_mgr =
>> -        ttm_manager_type(&xe->ttm, XE_PL_STOLEN);
>> -    struct xe_ttm_stolen_mgr *mgr;
>> -
>> -    if (!ttm_mgr)
>> -        return true;
>> -
>> -    mgr = to_stolen_mgr(ttm_mgr);
>> -
>> -    return !mgr->io_base || GRAPHICS_VERx100(xe) < 1270;
>> +    return GRAPHICS_VERx100(xe) < 1270 && !IS_DGFX(xe);
>>   }
>>   static s64 detect_bar2_dgfx(struct xe_device *xe, struct 
>> xe_ttm_stolen_mgr *mgr)
>> @@ -178,7 +163,7 @@ void xe_ttm_stolen_mgr_init(struct xe_device *xe)
>>       drm_dbg_kms(&xe->drm, "Initialized stolen memory support with 
>> %llu bytes\n",
>>               stolen_size);
>> -    if (!xe_ttm_stolen_cpu_inaccessible(xe))
>> +    if (mgr->io_base && !xe_ttm_stolen_cpu_access_needs_ggtt(xe))
>>           mgr->mapping = devm_ioremap_wc(&pdev->dev, mgr->io_base, 
>> stolen_size);
>>   }
>> @@ -191,7 +176,7 @@ u64 xe_ttm_stolen_io_offset(struct xe_bo *bo, u32 
>> offset)
>>       XE_BUG_ON(!mgr->io_base);
>> -    if (!IS_DGFX(xe) && xe_ttm_stolen_cpu_inaccessible(xe))
>> +    if (xe_ttm_stolen_cpu_access_needs_ggtt(xe))
>>           return mgr->io_base + xe_bo_ggtt_addr(bo) + offset;
>>       xe_res_first(bo->ttm.resource, offset, 4096, &cur);
>> @@ -257,10 +242,10 @@ int xe_ttm_stolen_io_mem_reserve(struct 
>> xe_device *xe, struct ttm_resource *mem)
>>       if (!mgr || !mgr->io_base)
>>           return -EIO;
>> -    if (!xe_ttm_stolen_cpu_inaccessible(xe))
>> -        return __xe_ttm_stolen_io_mem_reserve_bar2(xe, mgr, mem);
>> -    else
>> +    if (xe_ttm_stolen_cpu_access_needs_ggtt(xe))
>>           return __xe_ttm_stolen_io_mem_reserve_stolen(xe, mgr, mem);
>> +    else
>> +        return __xe_ttm_stolen_io_mem_reserve_bar2(xe, mgr, mem);
>>   }
>>   u64 xe_ttm_stolen_gpu_offset(struct xe_device *xe)
>> diff --git a/drivers/gpu/drm/xe/xe_ttm_stolen_mgr.h 
>> b/drivers/gpu/drm/xe/xe_ttm_stolen_mgr.h
>> index 2fda97b97a05..1777245ff810 100644
>> --- a/drivers/gpu/drm/xe/xe_ttm_stolen_mgr.h
>> +++ b/drivers/gpu/drm/xe/xe_ttm_stolen_mgr.h
>> @@ -14,7 +14,7 @@ struct xe_device;
>>   void xe_ttm_stolen_mgr_init(struct xe_device *xe);
>>   int xe_ttm_stolen_io_mem_reserve(struct xe_device *xe, struct 
>> ttm_resource *mem);
>> -bool xe_ttm_stolen_cpu_inaccessible(struct xe_device *xe);
>> +bool xe_ttm_stolen_cpu_access_needs_ggtt(struct xe_device *xe);
>>   u64 xe_ttm_stolen_io_offset(struct xe_bo *bo, u32 offset);
>>   u64 xe_ttm_stolen_gpu_offset(struct xe_device *xe);