Re: [PATCH v3 7/7] drm/xe/uapi: Add a query flag for has_device_atomics_on_smem

[Nirmoy Das <nirmoy.das@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 3534 bytes --]

Hi Lionel,

On 4/19/2024 9:08 AM, Lionel Landwerlin wrote:
> On 15/04/2024 17:52, Nirmoy Das wrote:
>> Add a query flag for xe->info.has_device_atomics_on_smem
>> as this is platform dependent. This flag can be use to inform
>> whether DRM_XE_VM_BIND_FLAG_DEVICE_ATOMICS can be effectively
>> applied for a given platform.
>>
>> Signed-off-by: Nirmoy Das<nirmoy.das@intel.com>
>> ---
>>   drivers/gpu/drm/xe/xe_query.c | 3 +++
>>   include/uapi/drm/xe_drm.h     | 5 ++++-
>>   2 files changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_query.c b/drivers/gpu/drm/xe/xe_query.c
>> index df407d73e5f5..43272fdd5442 100644
>> --- a/drivers/gpu/drm/xe/xe_query.c
>> +++ b/drivers/gpu/drm/xe/xe_query.c
>> @@ -336,6 +336,9 @@ static int query_config(struct xe_device *xe, struct drm_xe_device_query *query)
>>   	if (xe_device_get_root_tile(xe)->mem.vram.usable_size)
>>   		config->info[DRM_XE_QUERY_CONFIG_FLAGS] =
>>   			DRM_XE_QUERY_CONFIG_FLAG_HAS_VRAM;
>> +	if (xe->info.has_device_atomics_on_smem)
>> +		config->info[DRM_XE_QUERY_CONFIG_FLAGS] |=
>> +			DRM_XE_QUERY_CONFIG_FLAG_HAS_DEV_ATOMIC_ON_SMEM;
>>   	config->info[DRM_XE_QUERY_CONFIG_MIN_ALIGNMENT] =
>>   		xe->info.vram_flags & XE_VRAM_FLAGS_NEED64K ? SZ_64K : SZ_4K;
>>   	config->info[DRM_XE_QUERY_CONFIG_VA_BITS] = xe->info.va_bits;
>> diff --git a/include/uapi/drm/xe_drm.h b/include/uapi/drm/xe_drm.h
>> index ca4447e10ac9..80fff6fe3199 100644
>> --- a/include/uapi/drm/xe_drm.h
>> +++ b/include/uapi/drm/xe_drm.h
>> @@ -389,6 +389,8 @@ struct drm_xe_query_mem_regions {
>>    *
>>    *    - %DRM_XE_QUERY_CONFIG_FLAG_HAS_VRAM - Flag is set if the device
>>    *      has usable VRAM
>> + *    - %DRM_XE_QUERY_CONFIG_FLAG_HAS_DEV_ATOMIC_ON_SMEM - Flag is set if the device
>> + *      supports device atomics on system memory
>>    *  - %DRM_XE_QUERY_CONFIG_MIN_ALIGNMENT - Minimal memory alignment
>>    *    required by this device, typically SZ_4K or SZ_64K
>>    *  - %DRM_XE_QUERY_CONFIG_VA_BITS - Maximum bits of a virtual address
>> @@ -404,7 +406,8 @@ struct drm_xe_query_config {
>>   
>>   #define DRM_XE_QUERY_CONFIG_REV_AND_DEVICE_ID	0
>>   #define DRM_XE_QUERY_CONFIG_FLAGS			1
>> -	#define DRM_XE_QUERY_CONFIG_FLAG_HAS_VRAM	(1 << 0)
>> +	#define DRM_XE_QUERY_CONFIG_FLAG_HAS_VRAM		(1 << 0)
>> +	#define DRM_XE_QUERY_CONFIG_FLAG_HAS_DEV_ATOMIC_ON_SMEM	(1 << 1)
>
>
> I find this flag a bit confusing, because it would imply that 
> platforms that do not report this flag don't suppoort atomics on smem.
>
> But for DG2/MTL/TGL (if officially supported by Xe), that's not the 
> case, yet they do support this.
>
This query should return true for DG2/MTL/TGL but I missed setting 
has_device_atomics_on_smem on those platform.

>
> Maybe renaming this 
> toDRM_XE_QUERY_CONFIG_FLAG_HAS_EXPLICIT_DEV_ATOMIC_ON_SMEM would make 
> more sense.
>
> Meaning the platform requires a specific vm_bind flag to get atomics 
> working on SMEM only BO.
>
We could do that but we would need another query flag to indicate when a 
platform doesn't support dev atomics SMEM only BO at all like PVC.

So I think let's keep it DRM_XE_QUERY_CONFIG_FLAG_HAS_DEV_ATOMIC_ON_SMEM 
which will return true for all platform but PVC at this moment.

On platforms that doesn't have the new PTE bit, this flag will have no 
effect. Let me know what do you think?

Regards,

Nirmoy

> -Lionel
>
>
>>   #define DRM_XE_QUERY_CONFIG_MIN_ALIGNMENT		2
>>   #define DRM_XE_QUERY_CONFIG_VA_BITS			3
>>   #define DRM_XE_QUERY_CONFIG_MAX_EXEC_QUEUE_PRIORITY	4
>
>

[-- Attachment #2: Type: text/html, Size: 5280 bytes --]