Re: [PATCH 2/5] drm/amdgpu: export function to flush TLB via pasid

[Felix Kuehling <felix.kuehling@amd.com>]

On 2020-01-05 10:41 a.m., Christian König wrote:
> Am 20.12.19 um 07:24 schrieb Alex Sierra:
>> This can be used directly from amdgpu and amdkfd to invalidate
>> TLB through pasid.
>> It supports gmc v7, v8, v9 and v10.
>>
>> Change-Id: I6563a8eba2e42d1a67fa2547156c20da41d1e490
>> Signed-off-by: Alex Sierra <alex.sierra@amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h |  6 ++
>>   drivers/gpu/drm/amd/amdgpu/gmc_v10_0.c  | 81 ++++++++++++++++++++++++
>>   drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c   | 33 ++++++++++
>>   drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c   | 34 ++++++++++
>>   drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c   | 84 +++++++++++++++++++++++++
>>   5 files changed, 238 insertions(+)
[snip]
>> diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c 
>> b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
>> index fa025ceeea0f..eb1e64bd56ed 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
>> @@ -38,10 +38,12 @@
>>   #include "dce/dce_12_0_sh_mask.h"
>>   #include "vega10_enum.h"
>>   #include "mmhub/mmhub_1_0_offset.h"
>> +#include "athub/athub_1_0_sh_mask.h"
>>   #include "athub/athub_1_0_offset.h"
>>   #include "oss/osssys_4_0_offset.h"
>>     #include "soc15.h"
>> +#include "soc15d.h"
>>   #include "soc15_common.h"
>>   #include "umc/umc_6_0_sh_mask.h"
>>   @@ -434,6 +436,47 @@ static bool 
>> gmc_v9_0_use_invalidate_semaphore(struct amdgpu_device *adev,
>>              adev->pdev->device == 0x15d8)));
>>   }
>>   +static bool gmc_v9_0_get_atc_vmid_pasid_mapping_info(struct 
>> amdgpu_device *adev,
>> +                    uint8_t vmid, uint16_t *p_pasid)
>> +{
>> +    uint32_t value;
>> +
>> +    value = RREG32(SOC15_REG_OFFSET(ATHUB, 0, 
>> mmATC_VMID0_PASID_MAPPING)
>> +             + vmid);
>> +    *p_pasid = value & ATC_VMID0_PASID_MAPPING__PASID_MASK;
>> +
>> +    return !!(value & ATC_VMID0_PASID_MAPPING__VALID_MASK);
>> +}
>
> Probably better to expose just this function in the GMC interface.

This is called below in gmc_v9_0_flush_gpu_tlb_pasid in the case that 
the KIQ is not ready. It is not needed outside this file, so no need to 
expose it in the GMC interface.


>
>> +
>> +static int gmc_v9_0_invalidate_tlbs_with_kiq(struct amdgpu_device 
>> *adev,
>> +                uint16_t pasid, uint32_t flush_type,
>> +                bool all_hub)
>> +{
>> +    signed long r;
>> +    uint32_t seq;
>> +    struct amdgpu_ring *ring = &adev->gfx.kiq.ring;
>> +
>> +    spin_lock(&adev->gfx.kiq.ring_lock);
>> +    amdgpu_ring_alloc(ring, 12); /* fence + invalidate_tlbs package*/
>> +    amdgpu_ring_write(ring, PACKET3(PACKET3_INVALIDATE_TLBS, 0));
>> +    amdgpu_ring_write(ring,
>> +            PACKET3_INVALIDATE_TLBS_DST_SEL(1) |
>> +            PACKET3_INVALIDATE_TLBS_ALL_HUB(all_hub) |
>> +            PACKET3_INVALIDATE_TLBS_PASID(pasid) |
>> +            PACKET3_INVALIDATE_TLBS_FLUSH_TYPE(flush_type));
>
> That stuff is completely unrelated to the GMC and shouldn't be added 
> here.

Where else should it go? To me TLB flushing is very much something that 
belongs in this file.

Maybe you'd rather add "flush_tlbs_with_kiq" to amdgpu_ring_funcs and 
implement it in the gfx_v*.c GFX-IP code? I'm not sure that makes much 
sense either because it's only available on the KIQ ring.


>
> Christian.
>
>> +    amdgpu_fence_emit_polling(ring, &seq);
>> +    amdgpu_ring_commit(ring);
>> +    spin_unlock(&adev->gfx.kiq.ring_lock);
>> +
>> +    r = amdgpu_fence_wait_polling(ring, seq, adev->usec_timeout);
>> +    if (r < 1) {
>> +        DRM_ERROR("wait for kiq fence error: %ld.\n", r);
>> +        return -ETIME;
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>>   /*
>>    * GART
>>    * VMID 0 is the physical GPU addresses as used by the kernel.
>> @@ -532,6 +575,46 @@ static void gmc_v9_0_flush_gpu_tlb(struct 
>> amdgpu_device *adev, uint32_t vmid,
>>       DRM_ERROR("Timeout waiting for VM flush ACK!\n");
>>   }
>>   +/**
>> + * gmc_v9_0_flush_gpu_tlb_pasid - tlb flush via pasid
>> + *
>> + * @adev: amdgpu_device pointer
>> + * @pasid: pasid to be flush
>> + *
>> + * Flush the TLB for the requested pasid.
>> + */
>> +static int gmc_v9_0_flush_gpu_tlb_pasid(struct amdgpu_device *adev,
>> +                    uint16_t pasid, uint32_t flush_type,
>> +                    bool all_hub)

Christian, do you agree that this function belongs in the GMC interface? 
If not here, where do you suggest moving it?

Regards,
   Felix


>> +{
>> +    int vmid, i;
>> +    uint16_t queried_pasid;
>> +    bool ret;
>> +    struct amdgpu_ring *ring = &adev->gfx.kiq.ring;
>> +
>> +    if (adev->in_gpu_reset)
>> +        return -EIO;
>> +
>> +    if (ring->sched.ready)
>> +        return gmc_v9_0_invalidate_tlbs_with_kiq(adev,
>> +                        pasid, flush_type, all_hub);
>> +
>> +    for (vmid = 1; vmid < 16; vmid++) {
>> +
>> +        ret = gmc_v9_0_get_atc_vmid_pasid_mapping_info(adev, vmid,
>> +                &queried_pasid);
>> +        if (ret && queried_pasid == pasid) {
>> +            for (i = 0; i < adev->num_vmhubs; i++)
>> +                amdgpu_gmc_flush_gpu_tlb(adev, vmid,
>> +                            i, flush_type);
>> +            break;
>> +        }
>> +    }
>> +
>> +    return 0;
>> +
>> +}
>> +
>>   static uint64_t gmc_v9_0_emit_flush_gpu_tlb(struct amdgpu_ring *ring,
>>                           unsigned vmid, uint64_t pd_addr)
>>   {
>> @@ -693,6 +776,7 @@ static void gmc_v9_0_get_vm_pte(struct 
>> amdgpu_device *adev,
>>     static const struct amdgpu_gmc_funcs gmc_v9_0_gmc_funcs = {
>>       .flush_gpu_tlb = gmc_v9_0_flush_gpu_tlb,
>> +    .flush_gpu_tlb_pasid = gmc_v9_0_flush_gpu_tlb_pasid,
>>       .emit_flush_gpu_tlb = gmc_v9_0_emit_flush_gpu_tlb,
>>       .emit_pasid_mapping = gmc_v9_0_emit_pasid_mapping,
>>       .map_mtype = gmc_v9_0_map_mtype,
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&amp;data=02%7C01%7Cfelix.kuehling%40amd.com%7C0ebb82d62d1044ea57b608d791f5b021%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637138356784407460&amp;sdata=K8zhHT2YYFj8LXdD3YiihtNkbKNwa0ml6nQZ74zF828%3D&amp;reserved=0 
>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx