Re: [PATCH v4 3/5] drm/xe: Introduce has_device_atomics_on_smem device info

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

Hi Brian,

On 4/29/2024 10:19 PM, Welty, Brian wrote:
>
>
> On 4/25/2024 3:23 PM, Nirmoy Das wrote:
>> Add has_device_atomics_on_smem to specify that a device
>> supports device atomics on system memory. Currently XE2
>> supports this so set this for XE2.
>>
>> v2: Set has_device_atomics_on_smem for all platform but
>>      PVC.
>>
>> Signed-off-by: Nirmoy Das <nirmoy.das@intel.com>
>> Reviewed-by: Oak Zeng <oak.zeng@intel.com>
>> Reviewed-by: José Roberto de Souza <jose.souza@intel.com>
>> ---
>>   drivers/gpu/drm/xe/xe_device_types.h | 2 ++
>>   drivers/gpu/drm/xe/xe_pci.c          | 2 ++
>>   2 files changed, 4 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_device_types.h 
>> b/drivers/gpu/drm/xe/xe_device_types.h
>> index 7cddb00f9c35..0af739981ebf 100644
>> --- a/drivers/gpu/drm/xe/xe_device_types.h
>> +++ b/drivers/gpu/drm/xe/xe_device_types.h
>> @@ -283,6 +283,8 @@ struct xe_device {
>>           u8 skip_guc_pc:1;
>>           /** @info.has_atomic_enable_pte_bit: Device has atomic 
>> enable PTE bit */
>>           u8 has_atomic_enable_pte_bit:1;
>> +        /** @info.has_device_atomics_on_smem: Supports device 
>> atomics on SMEM */
>> +        u8 has_device_atomics_on_smem:1;
>>     #if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
>>           struct {
>> diff --git a/drivers/gpu/drm/xe/xe_pci.c b/drivers/gpu/drm/xe/xe_pci.c
>> index 5618318e6f3a..c6e8a264348c 100644
>> --- a/drivers/gpu/drm/xe/xe_pci.c
>> +++ b/drivers/gpu/drm/xe/xe_pci.c
>> @@ -632,6 +632,8 @@ static int xe_info_init(struct xe_device *xe,
>>       xe->info.vm_max_level = graphics_desc->vm_max_level;
>>       xe->info.has_asid = graphics_desc->has_asid;
>>       xe->info.has_atomic_enable_pte_bit = 
>> graphics_desc->has_atomic_enable_pte_bit;
>> +    if (xe->info.platform != XE_PVC)
>> +        xe->info.has_device_atomics_on_smem = 1;
>
> Minor comment...
> I guess no harm with how this is used later.
>
> But seems best to not set unless has_atomic_enable_pte_bit is also set?
>
>     if (xe->info.platform != XE_PVC && 
> xe->info.has_atomic_enable_pte_bit)
>          xe->info.has_device_atomics_on_smem = 1;
>
> Or:
>     xe->info.has_device_atomics_on_smem = (xe->info.platform != XE_PVC 
> && xe->info.has_atomic_enable_pte_bit);

In earlier review for a query uAPI of this variable, it was noted that 
this gives an impression that only xe2+ supports atomics on SMEM which 
is not true.

I think there is no harm with this change and in case we want to expose 
this a query flag then it won't misled anyone.


Regards,

Nirmoy

>
>
> -Brian
>
>
>>       xe->info.has_flat_ccs = graphics_desc->has_flat_ccs;
>>       xe->info.has_range_tlb_invalidation = 
>> graphics_desc->has_range_tlb_invalidation;
>>       xe->info.has_usm = graphics_desc->has_usm;