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

["Christian König" <ckoenig.leichtzumerken@gmail.com>]

Am 07.01.20 um 02:09 schrieb Sierra Guiza, Alejandro (Alex):
> [AMD Official Use Only - Internal Distribution Only]
>
>
>
> -----Original Message-----
> From: Koenig, Christian <Christian.Koenig@amd.com>
> Sent: Monday, January 6, 2020 10:23 AM
> To: Kuehling, Felix <Felix.Kuehling@amd.com>; amd-gfx@lists.freedesktop.org; Sierra Guiza, Alejandro (Alex) <Alex.Sierra@amd.com>
> 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@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.
>> 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.

I would add a new amdgpu_kiq_funcs structure for that.

> 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.

Well yes and no, the amdgpu_virt_kiq_reg_write_reg_wait() was just an 
example of a similar case.

The function amdgpu_virt_kiq_reg_write_reg_wait() should probably be 
cleaned up and moved into the gfx_*.c files as well.

Regards,
Christian.

>
>>>
>>>> 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%2Flis
>>> ts.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&amp;data=02%7C01%7C
>>> felix.kuehling%40amd.com%7C0ebb82d62d1044ea57b608d791f5b021%7C3dd8961
>>> fe4884e608e11a82d994e183d%7C0%7C0%7C637138356784407460&amp;sdata=K8zh
>>> HT2YYFj8LXdD3YiihtNkbKNwa0ml6nQZ74zF828%3D&amp;reserved=0
>>>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx