On Mon, Jan 03, 2022 at 11:48:25AM +0100, Christian König wrote:
Am 22.12.21 um 22:05 schrieb Daniel Vetter:
On Tue, Dec 07, 2021 at 01:33:48PM +0100, Christian König wrote:
This function allows to replace fences from the shared fence list when we can gurantee that the operation represented by the original fence has finished or no accesses to the resources protected by the dma_resv object any more when the new fence finishes.
Then use this function in the amdkfd code when BOs are unmapped from the process.
Signed-off-by: Christian König christian.koenig@amd.com
drivers/dma-buf/dma-resv.c | 43 ++++++++++++++++ .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c | 49 +++---------------- include/linux/dma-resv.h | 2 + 3 files changed, 52 insertions(+), 42 deletions(-)
diff --git a/drivers/dma-buf/dma-resv.c b/drivers/dma-buf/dma-resv.c index 4deea75c0b9c..a688dbded3d3 100644 --- a/drivers/dma-buf/dma-resv.c +++ b/drivers/dma-buf/dma-resv.c @@ -284,6 +284,49 @@ void dma_resv_add_shared_fence(struct dma_resv *obj, struct dma_fence *fence) } EXPORT_SYMBOL(dma_resv_add_shared_fence); +/**
- dma_resv_replace_fences - replace fences in the dma_resv obj
- @obj: the reservation object
- @context: the context of the fences to replace
- @replacement: the new fence to use instead
- Replace fences with a specified context with a new fence. Only valid if the
- operation represented by the original fences is completed or has no longer
- access to the resources protected by the dma_resv object when the new fence
- completes.
- */
+void dma_resv_replace_fences(struct dma_resv *obj, uint64_t context,
struct dma_fence *replacement)+{
- struct dma_resv_list *list;
- struct dma_fence *old;
- unsigned int i;
- dma_resv_assert_held(obj);
- write_seqcount_begin(&obj->seq);
- old = dma_resv_excl_fence(obj);
- if (old->context == context) {
RCU_INIT_POINTER(obj->fence_excl, dma_fence_get(replacement));dma_fence_put(old);- }
- list = dma_resv_shared_list(obj);
- for (i = 0; list && i < list->shared_count; ++i) {
old = rcu_dereference_protected(list->shared[i],dma_resv_held(obj));if (old->context != context)continue;rcu_assign_pointer(list->shared[i], dma_fence_get(replacement));dma_fence_put(old);Since the fences are all guaranteed to be from the same context, maybe we should have a WARN_ON(__dma_fence_is_later()); here just to be safe?
First of all happy new year!
Happy new year to you too!
Also I'm only still catching up.
Then to answer your question, no :)
This here is the case where we replace an preemption fence with a VM page table update fence. So both fences are not from the same context.
But since you ask that means that I somehow need to improve the documentation.
Hm yeah then I'm confused, since right above you have the context check. And I thought if the contexts are equal, then the fences must be ordered, and since you're adding a new one it must be a later fences.
But now you're saying this is to replace a fence with a totally different context one (which can totally make sense for the special fences compute mode contexts all need), but then I honestly don't get why you even check for the context.
Maybe more docs help explain what's going on, or maybe we should have the is_later check only if the new fences is from the same context. amdkfd might not benefit, but this is a new generic interface and other drivers might horrendously screw this up :-) Plus then a big comment that if it's a different fence timeline context the driver must guarantee that the new fence is guaranteed to signal after anything we're replacing here.
I think it might also be good to just include the specific amdkfd use case with a short intro to wth are preempt-ctx and page table fences, to explain when this function is actually useful.
It's definitely a very special case function, and I'm worried driver authors might come up with creative abuses for it that cause trouble. -Daniel
Regards, Christian.
With that added:
Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch
- }
- write_seqcount_end(&obj->seq);
+} +EXPORT_SYMBOL(dma_resv_replace_fences);
- /**
- dma_resv_add_excl_fence - Add an exclusive fence.
- @obj: the reservation object
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c index 71acd577803e..b558ef0f8c4a 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c @@ -236,53 +236,18 @@ void amdgpu_amdkfd_release_notify(struct amdgpu_bo *bo) static int amdgpu_amdkfd_remove_eviction_fence(struct amdgpu_bo *bo, struct amdgpu_amdkfd_fence *ef) {
- struct dma_resv *resv = bo->tbo.base.resv;
- struct dma_resv_list *old, *new;
- unsigned int i, j, k;
- struct dma_fence *replacement; if (!ef) return -EINVAL;
- old = dma_resv_shared_list(resv);
- if (!old)
return 0;- new = kmalloc(struct_size(new, shared, old->shared_max), GFP_KERNEL);
- if (!new)
return -ENOMEM;- /* Go through all the shared fences in the resevation object and sort
* the interesting ones to the end of the list.
- /* TODO: Instead of block before we should use the fence of the page
* table update and TLB flush here directly.
- for (i = 0, j = old->shared_count, k = 0; i < old->shared_count; ++i) {
struct dma_fence *f;f = rcu_dereference_protected(old->shared[i],dma_resv_held(resv));if (f->context == ef->base.context)RCU_INIT_POINTER(new->shared[--j], f);elseRCU_INIT_POINTER(new->shared[k++], f);- }
- new->shared_max = old->shared_max;
- new->shared_count = k;
- /* Install the new fence list, seqcount provides the barriers */
- write_seqcount_begin(&resv->seq);
- RCU_INIT_POINTER(resv->fence, new);
- write_seqcount_end(&resv->seq);
- /* Drop the references to the removed fences or move them to ef_list */
- for (i = j; i < old->shared_count; ++i) {
struct dma_fence *f;f = rcu_dereference_protected(new->shared[i],dma_resv_held(resv));dma_fence_put(f);- }
- kfree_rcu(old, rcu);
- replacement = dma_fence_get_stub();
- dma_resv_replace_fences(bo->tbo.base.resv, ef->base.context,
replacement);- dma_fence_put(replacement); return 0; }
diff --git a/include/linux/dma-resv.h b/include/linux/dma-resv.h index eebf04325b34..e0be34265eae 100644 --- a/include/linux/dma-resv.h +++ b/include/linux/dma-resv.h @@ -457,6 +457,8 @@ void dma_resv_init(struct dma_resv *obj); void dma_resv_fini(struct dma_resv *obj); int dma_resv_reserve_shared(struct dma_resv *obj, unsigned int num_fences); void dma_resv_add_shared_fence(struct dma_resv *obj, struct dma_fence *fence); +void dma_resv_replace_fences(struct dma_resv *obj, uint64_t context,
struct dma_fence *fence);-- 2.25.1