[PATCH 04/11] drm/amdgpu: fix and cleanup gmc_v7_0_flush_gpu_tlb_pasid

[Shashank Sharma]

On 06/09/2023 16:25, Shashank Sharma wrote:
>
> On 05/09/2023 08:04, Christian König wrote:
>> Testing for reset is pointless since the reset can start right after the
>> test. Grab the reset semaphore instead.
>>
>> The same PASID can be used by more than once VMID, build a mask of VMIDs
>> to reset instead of just restting the first one.
>>
>> Signed-off-by: Christian König <christian.koenig at amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c | 19 ++++++++++---------
>>   1 file changed, 10 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c 
>> b/drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c
>> index 6a6929ac2748..9e19a752f94b 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c
>> @@ -33,6 +33,7 @@
>>   #include "amdgpu_ucode.h"
>>   #include "amdgpu_amdkfd.h"
>>   #include "amdgpu_gem.h"
>> +#include "amdgpu_reset.h"
>>     #include "bif/bif_4_1_d.h"
>>   #include "bif/bif_4_1_sh_mask.h"
>> @@ -426,23 +427,23 @@ static int gmc_v7_0_flush_gpu_tlb_pasid(struct 
>> amdgpu_device *adev,
>>                       uint16_t pasid, uint32_t flush_type,
>>                       bool all_hub, uint32_t inst)
>>   {
>> +    u32 mask = 0x0;
>>       int vmid;
>> -    unsigned int tmp;
>>   -    if (amdgpu_in_reset(adev))
>> -        return -EIO;
>> +    if(!down_read_trylock(&adev->reset_domain->sem))
>> +        return 0;
>>         for (vmid = 1; vmid < 16; vmid++) {
>> +        u32 tmp = RREG32(mmATC_VMID0_PASID_MAPPING + vmid);
>>   -        tmp = RREG32(mmATC_VMID0_PASID_MAPPING + vmid);
>>           if ((tmp & ATC_VMID0_PASID_MAPPING__VALID_MASK) &&
>> -            (tmp & ATC_VMID0_PASID_MAPPING__PASID_MASK) == pasid) {
>> -            WREG32(mmVM_INVALIDATE_REQUEST, 1 << vmid);
>> -            RREG32(mmVM_INVALIDATE_RESPONSE);
>> -            break;
>> -        }
>> +            (tmp & ATC_VMID0_PASID_MAPPING__PASID_MASK) == pasid)
>> +            mask |= 1 << vmid;
>
> I am a bit concerned here about the change in code, in the previous 
> code we were writing the 'first match out of 16' of tmp and of mask 
> and programming the registers with (1 << vmid), whereas in new code 
> set we are writing the 'last match out of 16' of vmid. Is that 
> intentional or expected ?
>
With last, I mean all matching bits until last :)
> - Shashank
>
>>       }
>>   +    WREG32(mmVM_INVALIDATE_REQUEST, mask);
>> +    RREG32(mmVM_INVALIDATE_RESPONSE);
>> +    up_read(&adev->reset_domain->sem);
>>       return 0;
>>   }