Re: [Intel-xe] [PATCH 1/6] drm/xe: add xe_ttm_stolen_cpu_access_needs_ggtt()

[Matthew Auld <matthew.auld@intel.com>]

On 07/03/2023 09:53, Gwan-gyeong Mun wrote:
> Looks good to me.
> By the way, the commit message explained only the case of smallbar, is 
> there a way to access stolen memory in the case of dgpu resizable-bar?

It's just a case of whether the stolen portion fits entirely within the 
BAR size, or not. If it doesn't then we disable CPU access for stolen 
VRAM. Normally with resizable-bar enabled in the BIOS the BAR we are 
given is automatically configured to use the full-bar size. The 
resize-bar logic in the driver I think is normally never triggered 
(outside ofc driver testing with the special modparam). And when 
triggered most of the time it just fails anyway, since resizable-bar is 
likely disabled or not supported, at least from what I've seen.

> 
> Reviewed-by: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com>

Thanks.

> 
> On 3/1/23 4:48 PM, 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.
>>
>> Suggested-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> Signed-off-by: Matthew Auld <matthew.auld@intel.com>
>> ---
>>   drivers/gpu/drm/xe/xe_bo.c             |  2 +-
>>   drivers/gpu/drm/xe/xe_ttm_stolen_mgr.c | 14 ++++++++++++++
>>   drivers/gpu/drm/xe/xe_ttm_stolen_mgr.h |  1 +
>>   3 files changed, 16 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_bo.c b/drivers/gpu/drm/xe/xe_bo.c
>> index 2bfd3f6f2e9a..876f77669104 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 2e8d07ad42ae..4bf373a03d64 100644
>> --- a/drivers/gpu/drm/xe/xe_ttm_stolen_mgr.c
>> +++ b/drivers/gpu/drm/xe/xe_ttm_stolen_mgr.c
>> @@ -66,6 +66,20 @@ bool xe_ttm_stolen_cpu_inaccessible(struct 
>> xe_device *xe)
>>       return !mgr->io_base || GRAPHICS_VERx100(xe) < 1270;
>>   }
>> +/**
>> + * xe_ttm_stolen_needs_ggtt - If we can't directly CPU access stolen, 
>> can we
>> + * then fallback to mapping through the GGTT.
>> + * @xe: xe device
>> + *
>> + * 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_access_needs_ggtt(struct xe_device *xe)
>> +{
>> +    return xe_ttm_stolen_cpu_inaccessible(xe) && !IS_DGFX(xe);
>> +}
>> +
>>   static s64 detect_bar2_dgfx(struct xe_device *xe, struct 
>> xe_ttm_stolen_mgr *mgr)
>>   {
>>       struct pci_dev *pdev = to_pci_dev(xe->drm.dev);
>> diff --git a/drivers/gpu/drm/xe/xe_ttm_stolen_mgr.h 
>> b/drivers/gpu/drm/xe/xe_ttm_stolen_mgr.h
>> index 2fda97b97a05..e210dada636e 100644
>> --- a/drivers/gpu/drm/xe/xe_ttm_stolen_mgr.h
>> +++ b/drivers/gpu/drm/xe/xe_ttm_stolen_mgr.h
>> @@ -15,6 +15,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);