[PATCH 1/3] drm/amdgpu: Optimize a function called by every IB sheduling
Christian König
deathsimple at vodafone.de
Wed May 31 13:07:06 UTC 2017
> 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.
>
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:
>
> 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
>
> ------------------------------------------------------------------------
> *From:* Christian König <deathsimple at vodafone.de>
> *Sent:* Wednesday, May 31, 2017 2:57 AM
> *To:* Xie, AlexBin; amd-gfx at lists.freedesktop.org
> *Subject:* Re: [PATCH 1/3] drm/amdgpu: Optimize a function called by
> every IB sheduling
> 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 <AlexBin.Xie at amd.com>
> > ---
> > 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;
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/amd-gfx/attachments/20170531/1efaedcf/attachment-0001.html>
More information about the amd-gfx
mailing list