<html> <head> <meta http-equiv="Content-Type" content="text/html; charset=iso-8859-1"> <style type="text/css" style="display:none;"></style> </head> <body dir="ltr"> <div id="divtagdefaultwrapper" style="font-size: 12pt; color: rgb(0, 0, 0); font-family: Calibri,Arial,Helvetica,sans-serif,"EmojiFont","Apple Color Emoji","Segoe UI Emoji",NotoColorEmoji,"Segoe UI Symbol","Android Emoji",EmojiSymbols;" dir="ltr"> Hi Christian, I have asked for code review of a new patch. It turns out that the index operation is not needed. But there is one extra if statement in every IB submission, which will be a slight performance hit. Function amdgpu_vm_check_compute_bug is called in amdgpu_device_init. Function amdgpu_vm_check_compute_bug cannot be called in amdgpu_vm_manager_init, in which GFX is not initialized yet. MEC firmware version information is not available yet. Thanks, Alex Bin Xie <div style="color: rgb(0, 0, 0);"> <hr tabindex="-1" style="display:inline-block; width:98%"> <div id="divRplyFwdMsg" dir="ltr">From: Christian König <deathsimple@vodafone.de> Sent: Wednesday, May 31, 2017 9:07 AM To: Xie, AlexBin; amd-gfx@lists.freedesktop.org Subject: Re: [PATCH 1/3] drm/amdgpu: Optimize a function called by every IB sheduling <div> </div> </div> <div> <div class="moz-cite-prefix"> <blockquote type="cite"> I don't have strong opinion about where the code should be. But if we put this code in VM, there will be one extra array index operation because the VM bug is ring related. VM manager need to maintain an array to manage this information. </blockquote> Yeah, but array index suck when you access cache cold stuff as well. My concern is not so much where to put the field, but rather where to put the code to detect this condition. That is a bug very deeply related to how the CP manages VMs and VMIDs and actually not specific to the ring. Just send out a patch which uses the ring type again to check if that workaround applies or not. The heavy stuff was calling amdgpu_get_ip_block() on every command submission, one additional if shouldn't hurt us here. Regards, Christian. Am 31.05.2017 um 14:53 schrieb Xie, AlexBin: </div> <blockquote type="cite"> <div id="divtagdefaultwrapper" dir="ltr" style="font-size: 12pt; color: rgb(0, 0, 0); font-family: Calibri,Arial,Helvetica,sans-serif,"EmojiFont","Apple Color Emoji","Segoe UI Emoji",NotoColorEmoji,"Segoe UI Symbol","Android Emoji",EmojiSymbols;"> HI Christian, Too late. The code has been committed. I don't have strong opinion about where the code should be. But if we put this code in VM, there will be one extra array index operation because the VM bug is ring related. VM manager need to maintain an array to manage this information. In the amdgpu_ring structure, there is already information like vm_inv_eng and vmhub. Those are VM related information too. So this one extra information is not new. Thanks, Alex Bin <div style="color:rgb(0,0,0)"> <div> <hr tabindex="-1" style="display:inline-block; width:98%"> <div id="x_divRplyFwdMsg" dir="ltr">From: Christian König <a class="moz-txt-link-rfc2396E" href="mailto:deathsimple@vodafone.de"><deathsimple@vodafone.de></a> Sent: Wednesday, May 31, 2017 2:57 AM To: Xie, AlexBin; <a class="moz-txt-link-abbreviated" href="mailto:amd-gfx@lists.freedesktop.org"> amd-gfx@lists.freedesktop.org</a> Subject: Re: [PATCH 1/3] drm/amdgpu: Optimize a function called by every IB sheduling <div> </div> </div> </div> <div class="PlainText">Am 30.05.2017 um 23:47 schrieb Alex Xie: > Move several if statements and a loop statment from > run time to initialization time. Yeah, that's exactly what I've suggested before as well. Just keep the code inside amdgpu_vm.c (and the variable inside amdgpu_vm_manager), since this isn't related to ring management at all. Regards, Christian. > > Signed-off-by: Alex Xie <a class="moz-txt-link-rfc2396E" href="mailto:AlexBin.Xie@amd.com"> <AlexBin.Xie@amd.com></a> > --- > drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c | 33 ++++++++++++++++++++++++++++++++ > drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h | 6 ++++++ > drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 28 +-------------------------- > 3 files changed, 40 insertions(+), 27 deletions(-) > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c > index 6a85db0..7d95435 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c > @@ -153,6 +153,36 @@ void amdgpu_ring_undo(struct amdgpu_ring *ring) > } > > /** > + * amdgpu_ring_check_compute_vm_bug - check whether this ring has compute vm bug > + * > + * @adev: amdgpu_device pointer > + * @ring: amdgpu_ring structure holding ring information > + */ > +static void amdgpu_ring_check_compute_vm_bug(struct amdgpu_device *adev, > + struct amdgpu_ring *ring) > +{ > + const struct amdgpu_ip_block *ip_block; > + > + ring->has_compute_vm_bug = false; > + > + if (ring->funcs->type != AMDGPU_RING_TYPE_COMPUTE) > + /* only compute rings */ > + return; > + > + ip_block = amdgpu_get_ip_block(adev, AMD_IP_BLOCK_TYPE_GFX); > + if (!ip_block) > + return; > + > + /* Compute ring has a VM bug for GFX version < 7. > + And compute ring has a VM bug for GFX 8 MEC firmware version < 673.*/ > + if (ip_block->version->major <= 7) { > + ring->has_compute_vm_bug = true; > + } else if (ip_block->version->major == 8) > + if (adev->gfx.mec_fw_version < 673) > + ring->has_compute_vm_bug = true; > +} > + > +/** > * amdgpu_ring_init - init driver ring struct. > * > * @adev: amdgpu_device pointer > @@ -257,6 +287,9 @@ int amdgpu_ring_init(struct amdgpu_device *adev, struct amdgpu_ring *ring, > if (amdgpu_debugfs_ring_init(adev, ring)) { > DRM_ERROR("Failed to register debugfs file for rings !\n"); > } > + > + amdgpu_ring_check_compute_vm_bug(adev, ring); > + > return 0; > } > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h > index a9223a8..334307e 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h > @@ -185,6 +185,7 @@ struct amdgpu_ring { > u64 cond_exe_gpu_addr; > volatile u32 *cond_exe_cpu_addr; > unsigned vm_inv_eng; > + bool has_compute_vm_bug; > #if defined(CONFIG_DEBUG_FS) > struct dentry *ent; > #endif > @@ -207,4 +208,9 @@ static inline void amdgpu_ring_clear_ring(struct amdgpu_ring *ring) > > } > > +static inline bool amdgpu_ring_has_compute_vm_bug(struct amdgpu_ring *ring) > +{ > + return ring->has_compute_vm_bug; > +} > + > #endif > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c > index b2384b8..7a323f9 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c > @@ -656,32 +656,6 @@ static int amdgpu_vm_alloc_reserved_vmid(struct amdgpu_device *adev, > return r; > } > > -static bool amdgpu_vm_ring_has_compute_vm_bug(struct amdgpu_ring *ring) > -{ > - struct amdgpu_device *adev = ring->adev; > - const struct amdgpu_ip_block *ip_block; > - > - if (ring->funcs->type != AMDGPU_RING_TYPE_COMPUTE) > - /* only compute rings */ > - return false; > - > - ip_block = amdgpu_get_ip_block(adev, AMD_IP_BLOCK_TYPE_GFX); > - if (!ip_block) > - return false; > - > - if (ip_block->version->major <= 7) { > - /* gfx7 has no workaround */ > - return true; > - } else if (ip_block->version->major == 8) { > - if (adev->gfx.mec_fw_version >= 673) > - /* gfx8 is fixed in MEC firmware 673 */ > - return false; > - else > - return true; > - } > - return false; > -} > - > bool amdgpu_vm_need_pipeline_sync(struct amdgpu_ring *ring, > struct amdgpu_job *job) > { > @@ -691,7 +665,7 @@ bool amdgpu_vm_need_pipeline_sync(struct amdgpu_ring *ring, > struct amdgpu_vm_id *id; > bool gds_switch_needed; > bool vm_flush_needed = job->vm_needs_flush || > - amdgpu_vm_ring_has_compute_vm_bug(ring); > + amdgpu_ring_has_compute_vm_bug(ring); > > if (job->vm_id == 0) > return false; </div> </div> </div> </blockquote> </div> </div> </div> </body> </html>