If we remove the amdgpu_sync_get_owner() helper and replace it with local operations tailored for each of its two call sites, we get two benefits.
First is that in amdgpu_sync_resv() it enables us to avoid converting the fence to scheduler fence twice, first time to get the owner, second to check if it is coming from the same device. While at it, we also refactor the mode check ladder a bit to allow checking for the same device only once. Second benefit of replacing the helper with local operations is that in amdgpu_sync_kfd() we can now check for the only piece of information the caller is interested in. If the readability of the code is kept while reducing the lines of source, while at the same time amdgpu_sync.o bloat-o-meter sees a reduction, all should be good: add/remove: 0/0 grow/shrink: 0/2 up/down: 0/-106 (-106) Function old new delta amdgpu_sync_kfd 259 228 -31 amdgpu_sync_resv 734 659 -75 Total: Before=3123, After=3017, chg -3.39% Signed-off-by: Tvrtko Ursulin <[email protected]> Cc: Christian König <[email protected]> --- Regardless of whether the patch will be considered as improving or reducing redability, another reason why I am sending it is becuase I noticed in amdgpu_sync_resv() it peeks into the dma-fence-chain but it is not passing it to amdgpu_sync_fence(). I was curious what is the reason for this? Would it not be better to keep the unwrapped latest fence in the hash? For the case if it finds that it wants to sync to the fence, but if it is may not see that it already has one from the same context in there. TBH I did not find that it is happening in practice but maybe I just did not find the right userspace to trigger it. I did try passing the unwrapped fence from amdgpu_sync_test_fence() to amdgpu_sync_fence() and nothing appeared to have broke. --- drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c | 88 ++++++++---------------- 1 file changed, 29 insertions(+), 59 deletions(-) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c index d6ae9974c952..3b113d4cdf75 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c @@ -58,49 +58,20 @@ void amdgpu_sync_create(struct amdgpu_sync *sync) * amdgpu_sync_same_dev - test if fence belong to us * * @adev: amdgpu device to use for the test - * @f: fence to test + * @s_fence: scheduler fence to test * * Test if the fence was issued by us. */ static bool amdgpu_sync_same_dev(struct amdgpu_device *adev, - struct dma_fence *f) + struct drm_sched_fence *s_fence) { - struct drm_sched_fence *s_fence = to_drm_sched_fence(f); + struct amdgpu_ring *ring; - if (s_fence) { - struct amdgpu_ring *ring; + if (!s_fence) + return false; - ring = container_of(s_fence->sched, struct amdgpu_ring, sched); - return ring->adev == adev; - } - - return false; -} - -/** - * amdgpu_sync_get_owner - extract the owner of a fence - * - * @f: fence get the owner from - * - * Extract who originally created the fence. - */ -static void *amdgpu_sync_get_owner(struct dma_fence *f) -{ - struct drm_sched_fence *s_fence; - struct amdgpu_amdkfd_fence *kfd_fence; - - if (!f) - return AMDGPU_FENCE_OWNER_UNDEFINED; - - s_fence = to_drm_sched_fence(f); - if (s_fence) - return s_fence->owner; - - kfd_fence = to_amdgpu_amdkfd_fence(f); - if (kfd_fence) - return AMDGPU_FENCE_OWNER_KFD; - - return AMDGPU_FENCE_OWNER_UNDEFINED; + ring = container_of(s_fence->sched, struct amdgpu_ring, sched); + return ring->adev == adev; } /** @@ -183,7 +154,18 @@ static bool amdgpu_sync_test_fence(struct amdgpu_device *adev, enum amdgpu_sync_mode mode, void *owner, struct dma_fence *f) { - void *fence_owner = amdgpu_sync_get_owner(f); + struct drm_sched_fence *s_fence; + void *fence_owner; + + f = dma_fence_chain_contained(f); + + s_fence = to_drm_sched_fence(f); + if (s_fence) + fence_owner = s_fence->owner; + else if (to_amdgpu_amdkfd_fence(f)) + fence_owner = AMDGPU_FENCE_OWNER_KFD; + else + fence_owner = AMDGPU_FENCE_OWNER_UNDEFINED; /* Always sync to moves, no matter what */ if (fence_owner == AMDGPU_FENCE_OWNER_UNDEFINED) @@ -203,24 +185,16 @@ static bool amdgpu_sync_test_fence(struct amdgpu_device *adev, return false; /* Ignore fences depending on the sync mode */ - switch (mode) { - case AMDGPU_SYNC_ALWAYS: + if (mode == AMDGPU_SYNC_ALWAYS) { return true; - - case AMDGPU_SYNC_NE_OWNER: - if (amdgpu_sync_same_dev(adev, f) && - fence_owner == owner) - return false; - break; - - case AMDGPU_SYNC_EQ_OWNER: - if (amdgpu_sync_same_dev(adev, f) && - fence_owner != owner) - return false; - break; - - case AMDGPU_SYNC_EXPLICIT: + } else if (mode == AMDGPU_SYNC_EXPLICIT) { return false; + } else if ((mode == AMDGPU_SYNC_NE_OWNER || + mode == AMDGPU_SYNC_EQ_OWNER) && + amdgpu_sync_same_dev(adev, s_fence)) { + if ((mode == AMDGPU_SYNC_NE_OWNER && fence_owner == owner) || + (mode == AMDGPU_SYNC_EQ_OWNER && fence_owner != owner)) + return false; } WARN(debug_evictions && fence_owner == AMDGPU_FENCE_OWNER_KFD, @@ -252,9 +226,7 @@ int amdgpu_sync_resv(struct amdgpu_device *adev, struct amdgpu_sync *sync, /* Implicitly sync only to KERNEL, WRITE and READ */ dma_resv_for_each_fence(&cursor, resv, DMA_RESV_USAGE_READ, f) { dma_fence_chain_for_each(f, f) { - struct dma_fence *tmp = dma_fence_chain_contained(f); - - if (amdgpu_sync_test_fence(adev, mode, owner, tmp)) { + if (amdgpu_sync_test_fence(adev, mode, owner, f)) { r = amdgpu_sync_fence(sync, f, GFP_KERNEL); dma_fence_put(f); if (r) @@ -282,9 +254,7 @@ int amdgpu_sync_kfd(struct amdgpu_sync *sync, struct dma_resv *resv) dma_resv_iter_begin(&cursor, resv, DMA_RESV_USAGE_BOOKKEEP); dma_resv_for_each_fence_unlocked(&cursor, f) { - void *fence_owner = amdgpu_sync_get_owner(f); - - if (fence_owner != AMDGPU_FENCE_OWNER_KFD) + if (!to_amdgpu_amdkfd_fence(f)) continue; r = amdgpu_sync_fence(sync, f, GFP_KERNEL); -- 2.48.0
