<html> <head> <meta http-equiv="Content-Type" content="text/html; charset=us-ascii"> <style type="text/css" style="display:none;"></style> </head> <body dir="ltr"> <div id="divtagdefaultwrapper" style="font-size:12pt;color:#000000;font-family:Calibri,Helvetica,sans-serif;" dir="ltr"> Revised accordingly, and pushed. Thanks! Yong </div> <hr style="display:inline-block;width:98%" tabindex="-1"> <div id="divRplyFwdMsg" dir="ltr">From: Kuehling, Felix Sent: Tuesday, October 23, 2018 4:37:45 PM To: Zhao, Yong; brahma_sw_dev; amd-gfx@lists.freedesktop.org Subject: Re: [PATCH 1/2] drm/amdgpu: Reorganize *_flush_gpu_tlb() for kfd to use <div> </div> </div> <div class="BodyFragment"> <div class="PlainText">It occurred to me that the flush_type is a hardware-specific value, but you're using it in a hardware-abstracted interface. If the meaning of the flush type values changes in future HW-generations, we'll need to define an abstract enum that gets translated to the respective HW values in the HW-specific code. Anyway, this works for now. One more cosmetic comment inline ... Other than that the series is Reviewed-by: Felix Kuehling <Felix.Kuehling@amd.com> Regards, Felix On 2018-10-23 2:15 p.m., Zhao, Yong wrote: > Add a flush_type parameter to that series of functions. > > Change-Id: I3dcd71955297c53b181f82e7078981230c642c01 > Signed-off-by: Yong Zhao <Yong.Zhao@amd.com> > --- > drivers/gpu/drm/amd/amdgpu/amdgpu_gart.c | 4 ++-- > drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h | 4 ++-- > drivers/gpu/drm/amd/amdgpu/gmc_v6_0.c | 5 +++-- > drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c | 5 +++-- > drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c | 4 ++-- > drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c | 19 ++++++++++--------- > 6 files changed, 22 insertions(+), 19 deletions(-) > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gart.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gart.c > index 11fea28..9a212aa 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gart.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gart.c > @@ -248,7 +248,7 @@ int amdgpu_gart_unbind(struct amdgpu_device *adev, uint64_t offset, > } > mb(); > amdgpu_asic_flush_hdp(adev, NULL); > - amdgpu_gmc_flush_gpu_tlb(adev, 0); > + amdgpu_gmc_flush_gpu_tlb(adev, 0, 0); > return 0; > } > > @@ -331,7 +331,7 @@ int amdgpu_gart_bind(struct amdgpu_device *adev, uint64_t offset, > > mb(); > amdgpu_asic_flush_hdp(adev, NULL); > - amdgpu_gmc_flush_gpu_tlb(adev, 0); > + amdgpu_gmc_flush_gpu_tlb(adev, 0, 0); > return 0; > } > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h > index 6fa7ef4..4c5f18c 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h > @@ -64,7 +64,7 @@ struct amdgpu_vmhub { > struct amdgpu_gmc_funcs { > /* flush the vm tlb via mmio */ > void (*flush_gpu_tlb)(struct amdgpu_device *adev, > - uint32_t vmid); > + uint32_t vmid, uint32_t flush_type); > /* flush the vm tlb via ring */ > uint64_t (*emit_flush_gpu_tlb)(struct amdgpu_ring *ring, unsigned vmid, > uint64_t pd_addr); > @@ -151,7 +151,7 @@ struct amdgpu_gmc { > struct amdgpu_xgmi xgmi; > }; > > -#define amdgpu_gmc_flush_gpu_tlb(adev, vmid) (adev)->gmc.gmc_funcs->flush_gpu_tlb((adev), (vmid)) > +#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)) > #define amdgpu_gmc_set_pte_pde(adev, pt, idx, addr, flags) (adev)->gmc.gmc_funcs->set_pte_pde((adev), (pt), (idx), (addr), (flags)) > diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v6_0.c b/drivers/gpu/drm/amd/amdgpu/gmc_v6_0.c > index e1c2b4e..2821d1d 100644 > --- a/drivers/gpu/drm/amd/amdgpu/gmc_v6_0.c > +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v6_0.c > @@ -358,7 +358,8 @@ static int gmc_v6_0_mc_init(struct amdgpu_device *adev) > return 0; > } > > -static void gmc_v6_0_flush_gpu_tlb(struct amdgpu_device *adev, uint32_t vmid) > +static void gmc_v6_0_flush_gpu_tlb(struct amdgpu_device *adev, > + uint32_t vmid, uint32_t flush_type) > { > WREG32(mmVM_INVALIDATE_REQUEST, 1 << vmid); > } > @@ -580,7 +581,7 @@ static int gmc_v6_0_gart_enable(struct amdgpu_device *adev) > else > gmc_v6_0_set_fault_enable_default(adev, true); > > - gmc_v6_0_flush_gpu_tlb(adev, 0); > + gmc_v6_0_flush_gpu_tlb(adev, 0, 0); > dev_info(adev->dev, "PCIE GART of %uM enabled (table at 0x%016llX).\n", > (unsigned)(adev->gmc.gart_size >> 20), > (unsigned long long)table_addr); > diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c b/drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c > index 910c4ce..761dcfb 100644 > --- a/drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c > +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c > @@ -430,7 +430,8 @@ static int gmc_v7_0_mc_init(struct amdgpu_device *adev) > * > * Flush the TLB for the requested page table (CIK). > */ > -static void gmc_v7_0_flush_gpu_tlb(struct amdgpu_device *adev, uint32_t vmid) > +static void gmc_v7_0_flush_gpu_tlb(struct amdgpu_device *adev, > + uint32_t vmid, uint32_t flush_type) > { > /* bits 0-15 are the VM contexts0-15 */ > WREG32(mmVM_INVALIDATE_REQUEST, 1 << vmid); > @@ -698,7 +699,7 @@ static int gmc_v7_0_gart_enable(struct amdgpu_device *adev) > WREG32(mmCHUB_CONTROL, tmp); > } > > - gmc_v7_0_flush_gpu_tlb(adev, 0); > + gmc_v7_0_flush_gpu_tlb(adev, 0, 0); > DRM_INFO("PCIE GART of %uM enabled (table at 0x%016llX).\n", > (unsigned)(adev->gmc.gart_size >> 20), > (unsigned long long)table_addr); > diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c b/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c > index 1d3265c..531aaf3 100644 > --- a/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c > +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c > @@ -611,7 +611,7 @@ static int gmc_v8_0_mc_init(struct amdgpu_device *adev) > * Flush the TLB for the requested page table (CIK). > */ > static void gmc_v8_0_flush_gpu_tlb(struct amdgpu_device *adev, > - uint32_t vmid) > + uint32_t vmid, uint32_t flush_type) > { > /* bits 0-15 are the VM contexts0-15 */ > WREG32(mmVM_INVALIDATE_REQUEST, 1 << vmid); > @@ -920,7 +920,7 @@ static int gmc_v8_0_gart_enable(struct amdgpu_device *adev) > else > gmc_v8_0_set_fault_enable_default(adev, true); > > - gmc_v8_0_flush_gpu_tlb(adev, 0); > + gmc_v8_0_flush_gpu_tlb(adev, 0, 0); > DRM_INFO("PCIE GART of %uM enabled (table at 0x%016llX).\n", > (unsigned)(adev->gmc.gart_size >> 20), > (unsigned long long)table_addr); > diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c > index f35d7a5..79d8a84 100644 > --- a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c > +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c > @@ -293,14 +293,15 @@ static void gmc_v9_0_set_irq_funcs(struct amdgpu_device *adev) > adev->gmc.vm_fault.funcs = &gmc_v9_0_irq_funcs; > } > > -static uint32_t gmc_v9_0_get_invalidate_req(unsigned int vmid) > +static uint32_t gmc_v9_0_get_invalidate_req(unsigned int vmid, > + uint32_t flush_type) > { > u32 req = 0; > > /* invalidate using legacy mode on vmid*/ This comment is no longer accurate. > req = REG_SET_FIELD(req, VM_INVALIDATE_ENG0_REQ, > PER_VMID_INVALIDATE_REQ, 1 << vmid); > - req = REG_SET_FIELD(req, VM_INVALIDATE_ENG0_REQ, FLUSH_TYPE, 0); > + req = REG_SET_FIELD(req, VM_INVALIDATE_ENG0_REQ, FLUSH_TYPE, flush_type); > req = REG_SET_FIELD(req, VM_INVALIDATE_ENG0_REQ, INVALIDATE_L2_PTES, 1); > req = REG_SET_FIELD(req, VM_INVALIDATE_ENG0_REQ, INVALIDATE_L2_PDE0, 1); > req = REG_SET_FIELD(req, VM_INVALIDATE_ENG0_REQ, INVALIDATE_L2_PDE1, 1); > @@ -362,24 +363,24 @@ static signed long amdgpu_kiq_reg_write_reg_wait(struct amdgpu_device *adev, > */ > > /** > - * gmc_v9_0_flush_gpu_tlb - gart tlb flush callback > + * gmc_v9_0_flush_gpu_tlb - tlb flush with certain type > * > * @adev: amdgpu_device pointer > * @vmid: vm instance to flush > + * @flush_type: the flush type > * > - * Flush the TLB for the requested page table. > + * Flush the TLB for the requested page table using certain type. > */ > static void gmc_v9_0_flush_gpu_tlb(struct amdgpu_device *adev, > - uint32_t vmid) > + uint32_t vmid, uint32_t flush_type) > { > - /* Use register 17 for GART */ > const unsigned eng = 17; > unsigned i, j; > int r; > > for (i = 0; i < AMDGPU_MAX_VMHUBS; ++i) { > struct amdgpu_vmhub *hub = &adev->vmhub[i]; > - u32 tmp = gmc_v9_0_get_invalidate_req(vmid); > + u32 tmp = gmc_v9_0_get_invalidate_req(vmid, flush_type); > > if (adev->gfx.kiq.ring.ready && > (amdgpu_sriov_runtime(adev) || !amdgpu_sriov_vf(adev)) && > @@ -429,7 +430,7 @@ static uint64_t gmc_v9_0_emit_flush_gpu_tlb(struct amdgpu_ring *ring, > { > struct amdgpu_device *adev = ring->adev; > struct amdgpu_vmhub *hub = &adev->vmhub[ring->funcs->vmhub]; > - uint32_t req = gmc_v9_0_get_invalidate_req(vmid); > + uint32_t req = gmc_v9_0_get_invalidate_req(vmid, 0); > unsigned eng = ring->vm_inv_eng; > > amdgpu_ring_emit_wreg(ring, hub->ctx0_ptb_addr_lo32 + (2 * vmid), > @@ -1122,7 +1123,7 @@ static int gmc_v9_0_gart_enable(struct amdgpu_device *adev) > > gfxhub_v1_0_set_fault_enable_default(adev, value); > mmhub_v1_0_set_fault_enable_default(adev, value); > - gmc_v9_0_flush_gpu_tlb(adev, 0); > + gmc_v9_0_flush_gpu_tlb(adev, 0, 0); > > DRM_INFO("PCIE GART of %uM enabled (table at 0x%016llX).\n", > (unsigned)(adev->gmc.gart_size >> 20), </div> </div> </body> </html>