[AMD Official Use Only - Internal Distribution Only]
-----Original Message-----
From: Koenig, Christian <Christian.Koenig@xxxxxxx>
Sent: Monday, January 6, 2020 10:23 AM
To: Kuehling, Felix <Felix.Kuehling@xxxxxxx>; amd-gfx@xxxxxxxxxxxxxxxxxxxxx; Sierra Guiza, Alejandro (Alex) <Alex.Sierra@xxxxxxx>
Subject: Re: [PATCH 2/5] drm/amdgpu: export function to flush TLB via pasid
Am 06.01.20 um 17:04 schrieb Felix Kuehling:
> 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@xxxxxxx>
>>> ---
>>> 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.
>Yes, something like this. We should probably add a amdgpu_kiq_funcs and expose the functions needed by the GMC code there.
>See the amdgpu_virt_kiq_reg_write_reg_wait() case for reference, it is very similar and should probably be added to that function table as well.
>Christian.
To summarize:
1.- The idea is to add a new function pointer to the amdgpu_ring_funcs to flush the tlbs with kiq.
2.- This function pointer should be pointed and implemented on each of the gfx_v*.c under the gfx_*_ring_funcs_kiq struct. If this is true, Im not seeing this struct on the gfx_v10.c file.
3.- Use the amdgpu_virt_kiq_reg_write_reg_wait as a reference for the implementation.
>>
>>
>>>
>>> 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@xxxxxxxxxxxxxxxxxxxxx
>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flis
>> ts.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&data=02%7C01%7C
>> felix.kuehling%40amd.com%7C0ebb82d62d1044ea57b608d791f5b021%7C3dd8961
>> fe4884e608e11a82d994e183d%7C0%7C0%7C637138356784407460&sdata=K8zh
>> HT2YYFj8LXdD3YiihtNkbKNwa0ml6nQZ74zF828%3D&reserved=0
>>
_______________________________________________
amd-gfx mailing list
amd-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
