[PATCH 04/11] drm/amdgpu: fix and cleanup gmc_v7_0_flush_gpu_tlb_pasid
Shashank Sharma
shashank.sharma at amd.com
Thu Sep 7 07:31:04 UTC 2023
On 07/09/2023 08:57, Christian König wrote:
> Am 06.09.23 um 16:35 schrieb 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 :)
>
> Take a closer look :)
>
> The bits are ORed together for each VMID which has the matching pasid.
Agree, I saw that the previous code was programming only the first
matching VMID (and then break the loop), but and then I reiterated the
commit message and realized that it was the bug and this patch is fixing
it :).
Please feel free to use: Reviewed-by: Shashank Sharma
<shashank.sharma at amd.com>
- Shashank
>
> Christian.
>
>>> - Shashank
>>>
>>>> }
>>>> + WREG32(mmVM_INVALIDATE_REQUEST, mask);
>>>> + RREG32(mmVM_INVALIDATE_RESPONSE);
>>>> + up_read(&adev->reset_domain->sem);
>>>> return 0;
>>>> }
>
More information about the amd-gfx
mailing list