[PATCH 1/2] drm/amdgpu: Export function to flush TLB of specific vm hub

[Zeng, Oak]

Thanks. I realized I didn't read the code careful enough...The workaround is only for navi10 and navi12 - I didn't read this correctly and was thinking gfxhub tlb invalidation was done twice.

I understand the codes now. I think the HW SDMA bug has been fixed in navi14 so we don't need that WA for 14.

Regards,
Oak

-----Original Message-----
From: Christian König <ckoenig.leichtzumerken at gmail.com> 
Sent: Wednesday, August 7, 2019 4:51 AM
To: Zeng, Oak <Oak.Zeng at amd.com>; Kuehling, Felix <Felix.Kuehling at amd.com>; Koenig, Christian <Christian.Koenig at amd.com>; amd-gfx at lists.freedesktop.org; Zhang, Hawking <Hawking.Zhang at amd.com>; Deucher, Alexander <Alexander.Deucher at amd.com>
Cc: Zhou1, Tao <Tao.Zhou1 at amd.com>; Liu, Shaoyun <Shaoyun.Liu at amd.com>
Subject: Re: [PATCH 1/2] drm/amdgpu: Export function to flush TLB of specific vm hub

> Does the coded below invalidate TLB on both mm and gfx hub?
No, just the gfx hub. The VMHUBs on Navi are unfortunately really buggy, so we had to do a lot of workarounds to get them into a state where they actually did what was expected from them.

One major issue for example is that you can't do MMIO based VM invalidation when the engine is busy. To work around this we do the invalidation with the (IIRC) SDMA engine as soon as that one is working.

The is the code you are noting below.

Regards,
Christian.

Am 07.08.19 um 04:40 schrieb Zeng, Oak:
> Ok, will do it.
>
> BTW, does those codes below really needed, in function 
> gmc_v10_0_flush_gpu_tlb. I think if we have the bug, then before below 
> codes, when we flush TLB of gfxhub through mmio, it has already 
> triggered the bug. Also as we already invalidated tlb on both mm and 
> gfx hub (in the same function gmc_v10_0_flush_gpu_tlb), what is the 
> point of below codes? Does the coded below invalidate TLB on both mm 
> and gfx hub? Also @Zhang, Hawking at Deucher, Alexander
>
> 	/* The SDMA on Navi has a bug which can theoretically result in memory
> 	 * corruption if an invalidation happens at the same time as an VA
> 	 * translation. Avoid this by doing the invalidation from the SDMA
> 	 * itself.
> 	 */
> 	r = amdgpu_job_alloc_with_ib(adev, 16 * 4, &job);
> 	if (r)
> 		goto error_alloc;
>
> 	job->vm_pd_addr = amdgpu_gmc_pd_addr(adev->gart.bo);
> 	job->vm_needs_flush = true;
> 	amdgpu_ring_pad_ib(ring, &job->ibs[0]);
> 	r = amdgpu_job_submit(job, &adev->mman.entity,
> 			      AMDGPU_FENCE_OWNER_UNDEFINED, &fence);
>
> Regards,
> Oak
>
> -----Original Message-----
> From: Christian König <ckoenig.leichtzumerken at gmail.com>
> Sent: Monday, August 5, 2019 5:37 AM
> To: Zeng, Oak <Oak.Zeng at amd.com>; amd-gfx at lists.freedesktop.org
> Cc: Kuehling, Felix <Felix.Kuehling at amd.com>; Zhou1, Tao 
> <Tao.Zhou1 at amd.com>; Liu, Shaoyun <Shaoyun.Liu at amd.com>
> Subject: Re: [PATCH 1/2] drm/amdgpu: Export function to flush TLB of 
> specific vm hub
>
> Am 02.08.19 um 18:04 schrieb Zeng, Oak:
>> This is for kfd to reuse amdgpu TLB invalidation function. There is 
>> already a gmc function flush_gpu_tlb to flush TLB on all vm hub. On 
>> gfx10, kfd only needs to flush TLB on gfx hub but not on mm hub. So 
>> export a function for KFD flush TLB only on gfx hub.
> I would rather go ahead and add another parameter to flush_gpu_tlb to note which hub needs flushing.
>
> We can probably easily extend the few callers to flush all hubs needed.
>
> Christian.
>
>> Change-Id: I58ff00969f88438cfd3dc7e9deb7bff0c1bb4133
>> Signed-off-by: Oak Zeng <Oak.Zeng at amd.com>
>> ---
>>    drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h | 4 ++++
>>    drivers/gpu/drm/amd/amdgpu/gmc_v10_0.c  | 1 +
>>    2 files changed, 5 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h
>> index 071145a..0bd4a4f 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h
>> @@ -88,6 +88,9 @@ struct amdgpu_vmhub {
>>     * GPU MC structures, functions & helpers
>>     */
>>    struct amdgpu_gmc_funcs {
>> +	/* flush vm tlb of specific hub */
>> +	void (*flush_vm_hub)(struct amdgpu_device *adev, uint32_t vmid,
>> +				   unsigned int vmhub, uint32_t flush_type);
>>    	/* flush the vm tlb via mmio */
>>    	void (*flush_gpu_tlb)(struct amdgpu_device *adev,
>>    			      uint32_t vmid, uint32_t flush_type); @@ -180,6 +183,7 @@ 
>> struct amdgpu_gmc {
>>    	struct ras_common_if    *ras_if;
>>    };
>>    
>> +#define amdgpu_gmc_flush_vm_hub(adev, vmid, vmhub, type) 
>> +((adev)->gmc.gmc_funcs->flush_vm_hub((adev), (vmid), (vmhub),
>> +(type)))
>>    #define amdgpu_gmc_flush_gpu_tlb(adev, vmid, type) (adev)->gmc.gmc_funcs->flush_gpu_tlb((adev), (vmid), (type))
>>    #define amdgpu_gmc_emit_flush_gpu_tlb(r, vmid, addr) (r)->adev->gmc.gmc_funcs->emit_flush_gpu_tlb((r), (vmid), (addr))
>>    #define amdgpu_gmc_emit_pasid_mapping(r, vmid, pasid) 
>> (r)->adev->gmc.gmc_funcs->emit_pasid_mapping((r), (vmid), (pasid)) 
>> diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v10_0.c
>> b/drivers/gpu/drm/amd/amdgpu/gmc_v10_0.c
>> index 4e3ac10..247515d 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/gmc_v10_0.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v10_0.c
>> @@ -416,6 +416,7 @@ static void gmc_v10_0_get_vm_pde(struct amdgpu_device *adev, int level,
>>    }
>>    
>>    static const struct amdgpu_gmc_funcs gmc_v10_0_gmc_funcs = {
>> +	.flush_vm_hub = gmc_v10_0_flush_vm_hub,
>>    	.flush_gpu_tlb = gmc_v10_0_flush_gpu_tlb,
>>    	.emit_flush_gpu_tlb = gmc_v10_0_emit_flush_gpu_tlb,
>>    	.emit_pasid_mapping = gmc_v10_0_emit_pasid_mapping,
> _______________________________________________
> amd-gfx mailing list
> amd-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx