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!

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.

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);
  

• else
  

• 	RCU_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);
  

  void dma_resv_add_excl_fence(struct dma_resv *obj, struct dma_fence *fence); int dma_resv_get_fences(struct dma_resv *obj, struct dma_fence **pfence_excl, unsigned *pshared_count, struct dma_fence ***pshared);

-- 2.25.1

Am 22.12.21 um 22:21 schrieb Daniel Vetter:

On Tue, Dec 07, 2021 at 01:33:51PM +0100, Christian König wrote:

...

Add a function to simplify getting a single fence for all the fences in the dma_resv object.

v2: fix ref leak in error handling

Signed-off-by: Christian König christian.koenig@amd.com

drivers/dma-buf/dma-resv.c | 52 ++++++++++++++++++++++++++++++++++++++ include/linux/dma-resv.h | 2 ++ 2 files changed, 54 insertions(+)

diff --git a/drivers/dma-buf/dma-resv.c b/drivers/dma-buf/dma-resv.c index 480c305554a1..694716a3d66d 100644 --- a/drivers/dma-buf/dma-resv.c +++ b/drivers/dma-buf/dma-resv.c @@ -34,6 +34,7 @@ */ #include <linux/dma-resv.h> +#include <linux/dma-fence-array.h> #include <linux/export.h> #include <linux/mm.h> #include <linux/sched/mm.h> @@ -657,6 +658,57 @@ int dma_resv_get_fences(struct dma_resv *obj, bool write, } EXPORT_SYMBOL_GPL(dma_resv_get_fences); +/**

• • dma_resv_get_singleton - Get a single fence for all the fences
• • @obj: the reservation object
• • @write: true if we should return all fences
• • @fence: the resulting fence
• • Get a single fence representing all the fences inside the resv object.
• • Returns either 0 for success or -ENOMEM.
• • Warning: This can't be used like this when adding the fence back to the resv
• • object since that can lead to stack corruption when finalizing the
• • dma_fence_array.

Uh I don't get this one? I thought the only problem with nested fences is the signalling recursion, which we work around with the irq_work?

Nope, the main problem is finalizing the dma_fence_array.

E.g. imagine that you build up a chain of dma_fence_array objects like this: a<-b<-c<-d<-e<-f.....

With each one referencing the previous dma_fence_array and then you call dma_fence_put() on the last one. That in turn will cause calling dma_fence_put() on the previous one, which in turn will cause dma_fence_put() one the one before the previous one etc....

In other words you recurse because each dma_fence_array instance drops the last reference of it's predecessor.

What we could do is to delegate dropping the reference to the containing fences in a dma_fence_array as well, but that would require some changes to the irq_work_run_list() function to be halve way efficient.

Also if there's really an issue with dma_fence_array fences, then that warning should be on the dma_resv kerneldoc, not somewhere hidden like this. And finally I really don't see what can go wrong, sure we'll end up with the same fence once in the dma_resv_list and then once more in the fence array. But they're all refcounted, so really shouldn't matter.

The code itself looks correct, but me not understanding what even goes wrong here freaks me out a bit.

Yeah, IIRC we already discussed that with Jason in length as well.

Essentially what you can't do is to put a dma_fence_array into another dma_fence_array without causing issues.

So I think we should maybe just add a WARN_ON() into dma_fence_array_init() to make sure that this never happens.

Regards, Christian.

I guess something to figure out next year, I kinda hoped I could squeeze a review in before I disappear :-/ -Daniel

...

• */

+int dma_resv_get_singleton(struct dma_resv *obj, bool write,

• 	   struct dma_fence **fence)
  

+{

• struct dma_fence_array *array;
• struct dma_fence **fences;
• unsigned count;
• int r;
• r = dma_resv_get_fences(obj, write, &count, &fences);

•    if (r)
  

• return r;
  

• if (count == 0) {

• *fence = NULL;
  

• return 0;
  

• }
• if (count == 1) {

• *fence = fences[0];
  

• kfree(fences);
  

• return 0;
  

• }
• array = dma_fence_array_create(count, fences,

• 		       dma_fence_context_alloc(1),
  

• 		       1, false);
  

• if (!array) {

• while (count--)
  

• 	dma_fence_put(fences[count]);
  

• kfree(fences);
  

• return -ENOMEM;
  

• }
• *fence = &array->base;
• return 0;

+} +EXPORT_SYMBOL_GPL(dma_resv_get_singleton);

• /**
  • dma_resv_wait_timeout - Wait on reservation's objects
  • shared and/or exclusive fences.

diff --git a/include/linux/dma-resv.h b/include/linux/dma-resv.h index fa2002939b19..cdfbbda6f600 100644 --- a/include/linux/dma-resv.h +++ b/include/linux/dma-resv.h @@ -438,6 +438,8 @@ void dma_resv_replace_fences(struct dma_resv *obj, uint64_t context, void dma_resv_add_excl_fence(struct dma_resv *obj, struct dma_fence *fence); int dma_resv_get_fences(struct dma_resv *obj, bool write, unsigned int *num_fences, struct dma_fence ***fences); +int dma_resv_get_singleton(struct dma_resv *obj, bool write,

• 	   struct dma_fence **fence);
  

  int dma_resv_copy_fences(struct dma_resv *dst, struct dma_resv *src); long dma_resv_wait_timeout(struct dma_resv *obj, bool wait_all, bool intr, unsigned long timeout);

-- 2.25.1

Am 14.01.22 um 17:31 schrieb Daniel Vetter:

On Mon, Jan 03, 2022 at 12:13:41PM +0100, Christian König wrote:

...

Am 22.12.21 um 22:21 schrieb Daniel Vetter:

...

On Tue, Dec 07, 2021 at 01:33:51PM +0100, Christian König wrote:

...

Add a function to simplify getting a single fence for all the fences in the dma_resv object.

v2: fix ref leak in error handling

Signed-off-by: Christian König christian.koenig@amd.com

drivers/dma-buf/dma-resv.c | 52 ++++++++++++++++++++++++++++++++++++++ include/linux/dma-resv.h | 2 ++ 2 files changed, 54 insertions(+)

diff --git a/drivers/dma-buf/dma-resv.c b/drivers/dma-buf/dma-resv.c index 480c305554a1..694716a3d66d 100644 --- a/drivers/dma-buf/dma-resv.c +++ b/drivers/dma-buf/dma-resv.c @@ -34,6 +34,7 @@ */ #include <linux/dma-resv.h> +#include <linux/dma-fence-array.h> #include <linux/export.h> #include <linux/mm.h> #include <linux/sched/mm.h> @@ -657,6 +658,57 @@ int dma_resv_get_fences(struct dma_resv *obj, bool write, } EXPORT_SYMBOL_GPL(dma_resv_get_fences); +/**

• • dma_resv_get_singleton - Get a single fence for all the fences
• • @obj: the reservation object
• • @write: true if we should return all fences
• • @fence: the resulting fence
• • Get a single fence representing all the fences inside the resv object.
• • Returns either 0 for success or -ENOMEM.
• • Warning: This can't be used like this when adding the fence back to the resv
• • object since that can lead to stack corruption when finalizing the
• • dma_fence_array.

Uh I don't get this one? I thought the only problem with nested fences is the signalling recursion, which we work around with the irq_work?

Nope, the main problem is finalizing the dma_fence_array.

E.g. imagine that you build up a chain of dma_fence_array objects like this: a<-b<-c<-d<-e<-f.....

With each one referencing the previous dma_fence_array and then you call dma_fence_put() on the last one. That in turn will cause calling dma_fence_put() on the previous one, which in turn will cause dma_fence_put() one the one before the previous one etc....

In other words you recurse because each dma_fence_array instance drops the last reference of it's predecessor.

What we could do is to delegate dropping the reference to the containing fences in a dma_fence_array as well, but that would require some changes to the irq_work_run_list() function to be halve way efficient.o

...

Also if there's really an issue with dma_fence_array fences, then that warning should be on the dma_resv kerneldoc, not somewhere hidden like this. And finally I really don't see what can go wrong, sure we'll end up with the same fence once in the dma_resv_list and then once more in the fence array. But they're all refcounted, so really shouldn't matter.

The code itself looks correct, but me not understanding what even goes wrong here freaks me out a bit.

Yeah, IIRC we already discussed that with Jason in length as well.

Essentially what you can't do is to put a dma_fence_array into another dma_fence_array without causing issues.

So I think we should maybe just add a WARN_ON() into dma_fence_array_init() to make sure that this never happens.

Yeah I think this would be much clearer instead of sprinkling half the story as a scary&confusing warning over all kinds of users which internally use dma fence arrays.

And then if it goes boom I guess we could fix it internally in dma_fence_array_init by flattening fences down again. But only if actually needed.

Ok, going to do that first then.

What confused me is why dma_resv is special, and from your reply it sounds like it really isn't.

Well, it isn't special in any way. It's just something very obvious which could go wrong.

Regards, Christian.

-Daniel

...

Regards, Christian.

...

I guess something to figure out next year, I kinda hoped I could squeeze a review in before I disappear :-/ -Daniel

...

• */

+int dma_resv_get_singleton(struct dma_resv *obj, bool write,

• 	   struct dma_fence **fence)
  

+{

• struct dma_fence_array *array;
• struct dma_fence **fences;
• unsigned count;
• int r;
• r = dma_resv_get_fences(obj, write, &count, &fences);

•    if (r)
  

• return r;
  

• if (count == 0) {

• *fence = NULL;
  

• return 0;
  

• }
• if (count == 1) {

• *fence = fences[0];
  

• kfree(fences);
  

• return 0;
  

• }
• array = dma_fence_array_create(count, fences,

• 		       dma_fence_context_alloc(1),
  

• 		       1, false);
  

• if (!array) {

• while (count--)
  

• 	dma_fence_put(fences[count]);
  

• kfree(fences);
  

• return -ENOMEM;
  

• }
• *fence = &array->base;
• return 0;

+} +EXPORT_SYMBOL_GPL(dma_resv_get_singleton);

• /**
  • dma_resv_wait_timeout - Wait on reservation's objects
  • shared and/or exclusive fences.

diff --git a/include/linux/dma-resv.h b/include/linux/dma-resv.h index fa2002939b19..cdfbbda6f600 100644 --- a/include/linux/dma-resv.h +++ b/include/linux/dma-resv.h @@ -438,6 +438,8 @@ void dma_resv_replace_fences(struct dma_resv *obj, uint64_t context, void dma_resv_add_excl_fence(struct dma_resv *obj, struct dma_fence *fence); int dma_resv_get_fences(struct dma_resv *obj, bool write, unsigned int *num_fences, struct dma_fence ***fences); +int dma_resv_get_singleton(struct dma_resv *obj, bool write,

• 	   struct dma_fence **fence);
  

  int dma_resv_copy_fences(struct dma_resv *dst, struct dma_resv *src); long dma_resv_wait_timeout(struct dma_resv *obj, bool wait_all, bool intr, unsigned long timeout);

-- 2.25.1

Am 22.12.21 um 22:37 schrieb Daniel Vetter:

On Tue, Dec 07, 2021 at 01:33:58PM +0100, Christian König wrote:

...

Get the write fence using dma_resv_for_each_fence instead of accessing it manually.

Signed-off-by: Christian König christian.koenig@amd.com

drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c index 53e407ea4c89..7facd614e50a 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c @@ -1268,6 +1268,8 @@ static int amdgpu_cs_submit(struct amdgpu_cs_parser *p, amdgpu_bo_list_for_each_entry(e, p->bo_list) { struct dma_resv *resv = e->tv.bo->base.resv; struct dma_fence_chain *chain = e->chain;

• struct dma_resv_iter cursor;
  

• struct dma_fence *fence;
  

if (!chain) continue; @@ -1277,9 +1279,10 @@ static int amdgpu_cs_submit(struct amdgpu_cs_parser *p, * submission in a dma_fence_chain and add it as exclusive * fence. */

• dma_fence_chain_init(chain, dma_resv_excl_fence(resv),
  

• 		     dma_fence_get(p->fence), 1);
  

• dma_resv_for_each_fence(&cursor, resv, false, fence) {
  

• 	break;
  

• }
  

• dma_fence_chain_init(chain, fence, dma_fence_get(p->fence), 1);
  

Uh this needs a TODO. I'm assuming you'll fix this up later on when there's more than write fence, but in case of bisect or whatever this is a bit too clever. Like you just replace one "dig around in dma-resv implementation details" with one that's not even a documented interface :-)

Ah, yes. There is a rather big TODO just above this, but I should probably make that even more stronger.

With an adequately loud comment added interim:

Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch

Thanks, Christian.

...

rcu_assign_pointer(resv->fence_excl, &chain->base);
e->chain = NULL;

}

2.25.1

On Tue, Dec 07, 2021 at 01:33:59PM +0100, Christian König wrote:

Drivers should never touch this directly.

Signed-off-by: Christian König christian.koenig@amd.com

drivers/dma-buf/dma-resv.c | 17 +++++++++++++++++ include/linux/dma-resv.h | 17 ----------------- 2 files changed, 17 insertions(+), 17 deletions(-)

diff --git a/drivers/dma-buf/dma-resv.c b/drivers/dma-buf/dma-resv.c index 694716a3d66d..9acceabc9399 100644 --- a/drivers/dma-buf/dma-resv.c +++ b/drivers/dma-buf/dma-resv.c @@ -147,6 +147,23 @@ void dma_resv_fini(struct dma_resv *obj) } EXPORT_SYMBOL(dma_resv_fini); +/**

• • dma_resv_excl_fence - return the object's exclusive fence
• • @obj: the reservation object
• • Returns the exclusive fence (if any). Caller must either hold the objects
• • through dma_resv_lock() or the RCU read side lock through rcu_read_lock(),
• • or one of the variants of each
• • RETURNS
• • The exclusive fence or NULL
• */

Same thing with us not documenting internals, pls drop the comment outright it doesn't really explain anything. With that:

Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch

+static inline struct dma_fence * +dma_resv_excl_fence(struct dma_resv *obj) +{

• return rcu_dereference_check(obj->fence_excl, dma_resv_held(obj));

+}

/**

• dma_resv_shared_list - get the reservation object's shared fence list
• @obj: the reservation object

diff --git a/include/linux/dma-resv.h b/include/linux/dma-resv.h index cdfbbda6f600..40ac9d486f8f 100644 --- a/include/linux/dma-resv.h +++ b/include/linux/dma-resv.h @@ -412,23 +412,6 @@ static inline void dma_resv_unlock(struct dma_resv *obj) ww_mutex_unlock(&obj->lock); } -/**

• • dma_resv_excl_fence - return the object's exclusive fence
• • @obj: the reservation object
• • Returns the exclusive fence (if any). Caller must either hold the objects
• • through dma_resv_lock() or the RCU read side lock through rcu_read_lock(),
• • or one of the variants of each
• • RETURNS
• • The exclusive fence or NULL
• */

-static inline struct dma_fence * -dma_resv_excl_fence(struct dma_resv *obj) -{

• return rcu_dereference_check(obj->fence_excl, dma_resv_held(obj));

-}

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); -- 2.25.1

On Tue, Dec 07, 2021 at 01:34:00PM +0100, Christian König wrote:

So far we had the approach of using a directed acyclic graph with the dma_resv obj.

This turned out to have many downsides, especially it means that every single driver and user of this interface needs to be aware of this restriction when adding fences. If the rules for the DAG are not followed then we end up with potential hard to debug memory corruption, information leaks or even elephant big security holes because we allow userspace to access freed up memory.

Since we already took a step back from that by always looking at all fences we now go a step further and stop dropping the shared fences when a new exclusive one is added.

Signed-off-by: Christian König christian.koenig@amd.com

drivers/dma-buf/dma-resv.c | 13 ------------- 1 file changed, 13 deletions(-)

diff --git a/drivers/dma-buf/dma-resv.c b/drivers/dma-buf/dma-resv.c index 9acceabc9399..ecb2ff606bac 100644 --- a/drivers/dma-buf/dma-resv.c +++ b/drivers/dma-buf/dma-resv.c

No doc update at all!

I checked, we're not that shitty with docs, Minimally the DOC: section header and also the struct dma_resv kerneldoc. Also there's maybe more references and stuff I've missed on a quick look, please check for them (e.g. dma_buf.resv kerneldoc is rather important to keep correct too).

Code itself does what it says in the commit message, but we really need the most accurate docs we can get for this stuff, or the confusion will persist :-/

Cheers, Daniel

@@ -383,29 +383,16 @@ EXPORT_SYMBOL(dma_resv_replace_fences); void dma_resv_add_excl_fence(struct dma_resv *obj, struct dma_fence *fence) { struct dma_fence *old_fence = dma_resv_excl_fence(obj);

• struct dma_resv_list *old;
• u32 i = 0;

dma_resv_assert_held(obj);

• old = dma_resv_shared_list(obj);
• if (old)

• i = old->shared_count;
  

• dma_fence_get(fence);

write_seqcount_begin(&obj->seq); /* write_seqcount_begin provides the necessary memory barrier */ RCU_INIT_POINTER(obj->fence_excl, fence);

• if (old)

• old->shared_count = 0;
  

  write_seqcount_end(&obj->seq);

• /* inplace update, no shared fences */
• while (i--)

• dma_fence_put(rcu_dereference_protected(old->shared[i],
  

• 				dma_resv_held(obj)));
  

• dma_fence_put(old_fence);

} EXPORT_SYMBOL(dma_resv_add_excl_fence); -- 2.25.1

On Tue, Jan 04, 2022 at 04:08:20PM +0100, Christian König wrote:

Am 22.12.21 um 22:43 schrieb Daniel Vetter:

...

On Tue, Dec 07, 2021 at 01:34:00PM +0100, Christian König wrote:

...

So far we had the approach of using a directed acyclic graph with the dma_resv obj.

This turned out to have many downsides, especially it means that every single driver and user of this interface needs to be aware of this restriction when adding fences. If the rules for the DAG are not followed then we end up with potential hard to debug memory corruption, information leaks or even elephant big security holes because we allow userspace to access freed up memory.

Since we already took a step back from that by always looking at all fences we now go a step further and stop dropping the shared fences when a new exclusive one is added.

Signed-off-by: Christian König christian.koenig@amd.com

drivers/dma-buf/dma-resv.c | 13 ------------- 1 file changed, 13 deletions(-)

diff --git a/drivers/dma-buf/dma-resv.c b/drivers/dma-buf/dma-resv.c index 9acceabc9399..ecb2ff606bac 100644 --- a/drivers/dma-buf/dma-resv.c +++ b/drivers/dma-buf/dma-resv.c

No doc update at all!

Scratching my head I'm pretty sure I've updated at least the kerneldoc for dma_resv_add_excl_fence(). Must have gone lost in some rebase.

...

I checked, we're not that shitty with docs,

Well I wouldn't say shitty, but they are not perfect either.

This was sarcasm, I meant to say that despite the struggles the docs in-tree are pretty good nowadays. Email just sucks sometimes for communication.

...

Minimally the DOC: section header and also the struct dma_resv kerneldoc. Also there's maybe more references and stuff I've missed on a quick look, please check for them (e.g. dma_buf.resv kerneldoc is rather important to keep correct too).

Code itself does what it says in the commit message, but we really need the most accurate docs we can get for this stuff, or the confusion will persist :-/

Yeah completely agree, going to fix that.

Awesome!

Cheers, Daniel

Thanks, Christian.

...

Cheers, Daniel

...

@@ -383,29 +383,16 @@ EXPORT_SYMBOL(dma_resv_replace_fences); void dma_resv_add_excl_fence(struct dma_resv *obj, struct dma_fence *fence) { struct dma_fence *old_fence = dma_resv_excl_fence(obj);

• struct dma_resv_list *old;
• u32 i = 0; dma_resv_assert_held(obj);
• old = dma_resv_shared_list(obj);
• if (old)

• i = old->shared_count;
  

• dma_fence_get(fence); write_seqcount_begin(&obj->seq); /* write_seqcount_begin provides the necessary memory barrier */ RCU_INIT_POINTER(obj->fence_excl, fence);
• if (old)

• old->shared_count = 0;
  

  write_seqcount_end(&obj->seq);
• /* inplace update, no shared fences */
• while (i--)

• dma_fence_put(rcu_dereference_protected(old->shared[i],
  

• 				dma_resv_held(obj)));
  

• dma_fence_put(old_fence); } EXPORT_SYMBOL(dma_resv_add_excl_fence);

-- 2.25.1

On Tue, Dec 07, 2021 at 01:34:01PM +0100, Christian König wrote:

Audit all the users of dma_resv_add_excl_fence() and make sure they reserve a shared slot also when only trying to add an exclusive fence.

This is the next step towards handling the exclusive fence like a shared one.

v2: fix missed case in amdgpu

Signed-off-by: Christian König christian.koenig@amd.com

Needs all the driver cc and also at least some acks/testing.

---

drivers/dma-buf/st-dma-resv.c | 64 +++++++++---------- drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 8 +++ drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c | 8 +-- drivers/gpu/drm/i915/gem/i915_gem_clflush.c | 3 +- .../gpu/drm/i915/gem/i915_gem_execbuffer.c | 8 +-- .../drm/i915/gem/selftests/i915_gem_migrate.c | 5 +- drivers/gpu/drm/i915/i915_vma.c | 6 ++ .../drm/i915/selftests/intel_memory_region.c | 7 ++ drivers/gpu/drm/lima/lima_gem.c | 10 ++- drivers/gpu/drm/msm/msm_gem_submit.c | 18 +++--- drivers/gpu/drm/nouveau/nouveau_fence.c | 9 +-- drivers/gpu/drm/panfrost/panfrost_job.c | 4 ++ drivers/gpu/drm/ttm/ttm_bo_util.c | 12 +++- drivers/gpu/drm/ttm/ttm_execbuf_util.c | 11 ++--

vc4 seems missing?

Also I think I found one bug below in the conversions. -Daniel

drivers/gpu/drm/v3d/v3d_gem.c | 15 +++-- drivers/gpu/drm/vgem/vgem_fence.c | 12 ++-- drivers/gpu/drm/virtio/virtgpu_gem.c | 9 +++ drivers/gpu/drm/vmwgfx/vmwgfx_bo.c | 16 +++-- 18 files changed, 133 insertions(+), 92 deletions(-)

diff --git a/drivers/dma-buf/st-dma-resv.c b/drivers/dma-buf/st-dma-resv.c index cbe999c6e7a6..f33bafc78693 100644 --- a/drivers/dma-buf/st-dma-resv.c +++ b/drivers/dma-buf/st-dma-resv.c @@ -75,17 +75,16 @@ static int test_signaling(void *arg, bool shared) goto err_free; }

• if (shared) {

• r = dma_resv_reserve_shared(&resv, 1);
  

• if (r) {
  

• 	pr_err("Resv shared slot allocation failed\n");
  

• 	goto err_unlock;
  

• }
  

• r = dma_resv_reserve_shared(&resv, 1);
• if (r) {

• pr_err("Resv shared slot allocation failed\n");
  

• goto err_unlock;
  

• }

• if (shared) dma_resv_add_shared_fence(&resv, f);

• } else {

• else dma_resv_add_excl_fence(&resv, f);

• }

if (dma_resv_test_signaled(&resv, shared)) { pr_err("Resv unexpectedly signaled\n"); @@ -134,17 +133,16 @@ static int test_for_each(void *arg, bool shared) goto err_free; }

• if (shared) {

• r = dma_resv_reserve_shared(&resv, 1);
  

• if (r) {
  

• 	pr_err("Resv shared slot allocation failed\n");
  

• 	goto err_unlock;
  

• }
  

• r = dma_resv_reserve_shared(&resv, 1);
• if (r) {

• pr_err("Resv shared slot allocation failed\n");
  

• goto err_unlock;
  

• }

• if (shared) dma_resv_add_shared_fence(&resv, f);

• } else {

• else dma_resv_add_excl_fence(&resv, f);

• }

r = -ENOENT; dma_resv_for_each_fence(&cursor, &resv, shared, fence) { @@ -206,18 +204,17 @@ static int test_for_each_unlocked(void *arg, bool shared) goto err_free; }

• if (shared) {

• r = dma_resv_reserve_shared(&resv, 1);
  

• if (r) {
  

• 	pr_err("Resv shared slot allocation failed\n");
  

• 	dma_resv_unlock(&resv);
  

• 	goto err_free;
  

• }
  

• r = dma_resv_reserve_shared(&resv, 1);
• if (r) {

• pr_err("Resv shared slot allocation failed\n");
  

• dma_resv_unlock(&resv);
  

• goto err_free;
  

• }

• if (shared) dma_resv_add_shared_fence(&resv, f);

• } else {

• else dma_resv_add_excl_fence(&resv, f);

• } dma_resv_unlock(&resv);

r = -ENOENT; @@ -290,18 +287,17 @@ static int test_get_fences(void *arg, bool shared) goto err_resv; }

• if (shared) {

• r = dma_resv_reserve_shared(&resv, 1);
  

• if (r) {
  

• 	pr_err("Resv shared slot allocation failed\n");
  

• 	dma_resv_unlock(&resv);
  

• 	goto err_resv;
  

• }
  

• r = dma_resv_reserve_shared(&resv, 1);
• if (r) {

• pr_err("Resv shared slot allocation failed\n");
  

• dma_resv_unlock(&resv);
  

• goto err_resv;
  

• }

• if (shared) dma_resv_add_shared_fence(&resv, f);

• } else {

• else dma_resv_add_excl_fence(&resv, f);

• } dma_resv_unlock(&resv);

r = dma_resv_get_fences(&resv, shared, &i, &fences); diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c index 4fcfc2313b8c..24a6b88afcca 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c @@ -1367,6 +1367,14 @@ void amdgpu_bo_fence(struct amdgpu_bo *bo, struct dma_fence *fence, bool shared) { struct dma_resv *resv = bo->tbo.base.resv;

• int r;
• r = dma_resv_reserve_shared(resv, 1);
• if (r) {

• /* As last resort on OOM we block for the fence */
  

• dma_fence_wait(fence, false);
  

• return;
  

• }

if (shared) dma_resv_add_shared_fence(resv, fence); diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c index 4286dc93fdaa..d4a7073190ec 100644 --- a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c @@ -179,11 +179,9 @@ static int submit_fence_sync(struct etnaviv_gem_submit *submit) struct etnaviv_gem_submit_bo *bo = &submit->bos[i]; struct dma_resv *robj = bo->obj->base.resv;

• if (!(bo->flags & ETNA_SUBMIT_BO_WRITE)) {
  

• 	ret = dma_resv_reserve_shared(robj, 1);
  

• 	if (ret)
  

• 		return ret;
  

• }
  

• ret = dma_resv_reserve_shared(robj, 1);
  

• if (ret)
  

• 	return ret;
  

if (submit->flags & ETNA_SUBMIT_NO_IMPLICIT) continue; diff --git a/drivers/gpu/drm/i915/gem/i915_gem_clflush.c b/drivers/gpu/drm/i915/gem/i915_gem_clflush.c index f0435c6feb68..fc57ab914b60 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_clflush.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_clflush.c @@ -100,7 +100,8 @@ bool i915_gem_clflush_object(struct drm_i915_gem_object *obj, trace_i915_gem_object_clflush(obj); clflush = NULL;

• if (!(flags & I915_CLFLUSH_SYNC))

• if (!(flags & I915_CLFLUSH_SYNC) &&

•    dma_resv_reserve_shared(obj->base.resv, 1) == 0)
  

  clflush = clflush_work_create(obj); if (clflush) { i915_sw_fence_await_reservation(&clflush->base.chain,

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c index 4d7da07442f2..fc0e1625847c 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c @@ -989,11 +989,9 @@ static int eb_validate_vmas(struct i915_execbuffer *eb) } }

• if (!(ev->flags & EXEC_OBJECT_WRITE)) {
  

• 	err = dma_resv_reserve_shared(vma->resv, 1);
  

• 	if (err)
  

• 		return err;
  

• }
  

• err = dma_resv_reserve_shared(vma->resv, 1);
  

• if (err)
  

• 	return err;
  

GEM_BUG_ON(drm_mm_node_allocated(&vma->node) && eb_vma_misplaced(&eb->exec[i], vma, ev->flags)); diff --git a/drivers/gpu/drm/i915/gem/selftests/i915_gem_migrate.c b/drivers/gpu/drm/i915/gem/selftests/i915_gem_migrate.c index 28a700f08b49..2bf491fd5cdf 100644 --- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_migrate.c +++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_migrate.c @@ -179,7 +179,10 @@ static int igt_lmem_pages_migrate(void *arg) i915_gem_object_is_lmem(obj), 0xdeadbeaf, &rq); if (rq) {

• 	dma_resv_add_excl_fence(obj->base.resv, &rq->fence);
  

• 	err = dma_resv_reserve_shared(obj->base.resv, 1);
  

• 	if (!err)
  

• 		dma_resv_add_excl_fence(obj->base.resv,
  

• 					&rq->fence);
  i915_request_put(rq);
  

  } if (err)

diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c index bef795e265a6..5ec87de63963 100644 --- a/drivers/gpu/drm/i915/i915_vma.c +++ b/drivers/gpu/drm/i915/i915_vma.c @@ -1255,6 +1255,12 @@ int _i915_vma_move_to_active(struct i915_vma *vma, intel_frontbuffer_put(front); }

• if (!(flags & __EXEC_OBJECT_NO_RESERVE)) {
  

• 	err = dma_resv_reserve_shared(vma->resv, 1);
  

• 	if (unlikely(err))
  

• 		return err;
  

• }
  

• if (fence) { dma_resv_add_excl_fence(vma->resv, fence); obj->write_domain = I915_GEM_DOMAIN_RENDER;

diff --git a/drivers/gpu/drm/i915/selftests/intel_memory_region.c b/drivers/gpu/drm/i915/selftests/intel_memory_region.c index 418caae84759..b85af1672a7e 100644 --- a/drivers/gpu/drm/i915/selftests/intel_memory_region.c +++ b/drivers/gpu/drm/i915/selftests/intel_memory_region.c @@ -894,6 +894,13 @@ static int igt_lmem_write_cpu(void *arg) } i915_gem_object_lock(obj, NULL);

• err = dma_resv_reserve_shared(obj->base.resv, 1);
• if (err) {

• i915_gem_object_unlock(obj);
  

• goto out_put;
  

• }
• /* Put the pages into a known state -- from the gpu for added fun */ intel_engine_pm_get(engine); err = intel_context_migrate_clear(engine->gt->migrate.context, NULL,

diff --git a/drivers/gpu/drm/lima/lima_gem.c b/drivers/gpu/drm/lima/lima_gem.c index f9a9198ef198..b4846007463f 100644 --- a/drivers/gpu/drm/lima/lima_gem.c +++ b/drivers/gpu/drm/lima/lima_gem.c @@ -255,13 +255,11 @@ int lima_gem_get_info(struct drm_file *file, u32 handle, u32 *va, u64 *offset) static int lima_gem_sync_bo(struct lima_sched_task *task, struct lima_bo *bo, bool write, bool explicit) {

• int err = 0;

• int err;

• if (!write) {

• err = dma_resv_reserve_shared(lima_bo_resv(bo), 1);
  

• if (err)
  

• 	return err;
  

• }

• err = dma_resv_reserve_shared(lima_bo_resv(bo), 1);
• if (err)

• return err;
  

/* explicit sync use user passed dep fence */ if (explicit) diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c b/drivers/gpu/drm/msm/msm_gem_submit.c index 3cb029f10925..e874d09b74ef 100644 --- a/drivers/gpu/drm/msm/msm_gem_submit.c +++ b/drivers/gpu/drm/msm/msm_gem_submit.c @@ -320,16 +320,14 @@ static int submit_fence_sync(struct msm_gem_submit *submit, bool no_implicit) struct drm_gem_object *obj = &submit->bos[i].obj->base; bool write = submit->bos[i].flags & MSM_SUBMIT_BO_WRITE;

• if (!write) {
  

• 	/* NOTE: _reserve_shared() must happen before
  

• 	 * _add_shared_fence(), which makes this a slightly
  

• 	 * strange place to call it.  OTOH this is a
  

• 	 * convenient can-fail point to hook it in.
  

• 	 */
  

• 	ret = dma_resv_reserve_shared(obj->resv, 1);
  

• 	if (ret)
  

• 		return ret;
  

• }
  

• /* NOTE: _reserve_shared() must happen before
  

•  * _add_shared_fence(), which makes this a slightly
  

•  * strange place to call it.  OTOH this is a
  

•  * convenient can-fail point to hook it in.
  

•  */
  

• ret = dma_resv_reserve_shared(obj->resv, 1);
  

• if (ret)
  

• 	return ret;
  

/* exclusive fences must be ordered */ if (no_implicit && !write) diff --git a/drivers/gpu/drm/nouveau/nouveau_fence.c b/drivers/gpu/drm/nouveau/nouveau_fence.c index 26f9299df881..cd6715bd6d6b 100644 --- a/drivers/gpu/drm/nouveau/nouveau_fence.c +++ b/drivers/gpu/drm/nouveau/nouveau_fence.c @@ -349,12 +349,9 @@ nouveau_fence_sync(struct nouveau_bo *nvbo, struct nouveau_channel *chan, struct nouveau_fence *f; int ret;

• if (!exclusive) {

• ret = dma_resv_reserve_shared(resv, 1);
  

• if (ret)
  

• 	return ret;
  

• }

• ret = dma_resv_reserve_shared(resv, 1);
• if (ret)

• return ret;
  

dma_resv_for_each_fence(&cursor, resv, exclusive, fence) { struct nouveau_channel *prev = NULL; diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c index 908d79520853..89c3fe389476 100644 --- a/drivers/gpu/drm/panfrost/panfrost_job.c +++ b/drivers/gpu/drm/panfrost/panfrost_job.c @@ -247,6 +247,10 @@ static int panfrost_acquire_object_fences(struct drm_gem_object **bos, int i, ret; for (i = 0; i < bo_count; i++) {

• ret = dma_resv_reserve_shared(bos[i]->resv, 1);
  

• if (ret)
  

• 	return ret;
  

• /* panfrost always uses write mode in its current uapi */ ret = drm_sched_job_add_implicit_dependencies(job, bos[i], true);

diff --git a/drivers/gpu/drm/ttm/ttm_bo_util.c b/drivers/gpu/drm/ttm/ttm_bo_util.c index 72a94301bc95..ea9eabcc0a0c 100644 --- a/drivers/gpu/drm/ttm/ttm_bo_util.c +++ b/drivers/gpu/drm/ttm/ttm_bo_util.c @@ -221,9 +221,6 @@ static int ttm_buffer_object_transfer(struct ttm_buffer_object *bo, fbo->base = *bo;

• ttm_bo_get(bo);
• fbo->bo = bo;
• /**
  • Fix up members that we shouldn't copy directly:
  • TODO: Explicit member copy would probably be better here.

@@ -246,6 +243,15 @@ static int ttm_buffer_object_transfer(struct ttm_buffer_object *bo, ret = dma_resv_trylock(&fbo->base.base._resv); WARN_ON(!ret);

• ret = dma_resv_reserve_shared(&fbo->base.base._resv, 1);
• if (ret) {

• kfree(fbo);
  

• return ret;
  

• }
• ttm_bo_get(bo);
• fbo->bo = bo;
• ttm_bo_move_to_lru_tail_unlocked(&fbo->base);

*new_obj = &fbo->base; diff --git a/drivers/gpu/drm/ttm/ttm_execbuf_util.c b/drivers/gpu/drm/ttm/ttm_execbuf_util.c index 071c48d672c6..5da922639d54 100644 --- a/drivers/gpu/drm/ttm/ttm_execbuf_util.c +++ b/drivers/gpu/drm/ttm/ttm_execbuf_util.c @@ -90,6 +90,7 @@ int ttm_eu_reserve_buffers(struct ww_acquire_ctx *ticket, list_for_each_entry(entry, list, head) { struct ttm_buffer_object *bo = entry->bo;

• unsigned int num_fences;
  

ret = ttm_bo_reserve(bo, intr, (ticket == NULL), ticket); if (ret == -EALREADY && dups) { @@ -100,12 +101,10 @@ int ttm_eu_reserve_buffers(struct ww_acquire_ctx *ticket, continue; }

• num_fences = min(entry->num_shared, 1u);
  

  if (!ret) {

• 	if (!entry->num_shared)
  

• 		continue;
  

• ret = dma_resv_reserve_shared(bo->base.resv,
  

• 						entry->num_shared);
  

• 				      num_fences);
  

Needs to be at least one, otherwise you call this with 0 when we want to install a write fence ang go boom?

	if (!ret)
		continue;
}

@@ -120,9 +119,9 @@ int ttm_eu_reserve_buffers(struct ww_acquire_ctx *ticket, ret = ttm_bo_reserve_slowpath(bo, intr, ticket); }

• if (!ret && entry->num_shared)
  

• if (!ret)
  ret = dma_resv_reserve_shared(bo->base.resv,
  

• 						entry->num_shared);
  

• 				      num_fences);
  

if (unlikely(ret != 0)) { if (ticket) { diff --git a/drivers/gpu/drm/v3d/v3d_gem.c b/drivers/gpu/drm/v3d/v3d_gem.c index c7ed2e1cbab6..1bea90e40ce1 100644 --- a/drivers/gpu/drm/v3d/v3d_gem.c +++ b/drivers/gpu/drm/v3d/v3d_gem.c @@ -259,16 +259,21 @@ v3d_lock_bo_reservations(struct v3d_job *job, return ret; for (i = 0; i < job->bo_count; i++) {

• ret = dma_resv_reserve_shared(job->bo[i]->resv, 1);
  

• if (ret)
  

• 	goto fail;
  

• ret = drm_sched_job_add_implicit_dependencies(&job->base, job->bo[i], true);

• if (ret) {
  

• 	drm_gem_unlock_reservations(job->bo, job->bo_count,
  

• 				    acquire_ctx);
  

• 	return ret;
  

• }
  

• if (ret)
  

• 	goto fail;
  

  }

return 0;

+fail:

• drm_gem_unlock_reservations(job->bo, job->bo_count, acquire_ctx);
• return ret;

} /** diff --git a/drivers/gpu/drm/vgem/vgem_fence.c b/drivers/gpu/drm/vgem/vgem_fence.c index bd6f75285fd9..a4cb296d4fcd 100644 --- a/drivers/gpu/drm/vgem/vgem_fence.c +++ b/drivers/gpu/drm/vgem/vgem_fence.c @@ -157,12 +157,14 @@ int vgem_fence_attach_ioctl(struct drm_device *dev, } /* Expose the fence via the dma-buf */

• ret = 0; dma_resv_lock(resv, NULL);
• if (arg->flags & VGEM_FENCE_WRITE)

• dma_resv_add_excl_fence(resv, fence);
  

• else if ((ret = dma_resv_reserve_shared(resv, 1)) == 0)

• dma_resv_add_shared_fence(resv, fence);
  

• ret = dma_resv_reserve_shared(resv, 1);
• if (!ret) {

• if (arg->flags & VGEM_FENCE_WRITE)
  

• 	dma_resv_add_excl_fence(resv, fence);
  

• else
  

• 	dma_resv_add_shared_fence(resv, fence);
  

• } dma_resv_unlock(resv);

/* Record the fence in our idr for later signaling */ diff --git a/drivers/gpu/drm/virtio/virtgpu_gem.c b/drivers/gpu/drm/virtio/virtgpu_gem.c index 2de61b63ef91..aec105cdd64c 100644 --- a/drivers/gpu/drm/virtio/virtgpu_gem.c +++ b/drivers/gpu/drm/virtio/virtgpu_gem.c @@ -214,6 +214,7 @@ void virtio_gpu_array_add_obj(struct virtio_gpu_object_array *objs, int virtio_gpu_array_lock_resv(struct virtio_gpu_object_array *objs) {

• unsigned int i; int ret;

if (objs->nents == 1) { @@ -222,6 +223,14 @@ int virtio_gpu_array_lock_resv(struct virtio_gpu_object_array *objs) ret = drm_gem_lock_reservations(objs->objs, objs->nents, &objs->ticket); }

• if (ret)

• return ret;
  

• for (i = 0; i < objs->nents; ++i) {

• ret = dma_resv_reserve_shared(objs->objs[i]->resv, 1);
  

• if (ret)
  

• 	return ret;
  

• } return ret;

} diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c b/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c index c97a3d5e90ce..6d0abc2b0beb 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c @@ -1053,16 +1053,22 @@ void vmw_bo_fence_single(struct ttm_buffer_object *bo, struct vmw_fence_obj *fence) { struct ttm_device *bdev = bo->bdev;

• struct vmw_private *dev_priv = container_of(bdev, struct vmw_private, bdev);

• int ret;

• if (fence == NULL) {

• if (fence == NULL) vmw_execbuf_fence_commands(NULL, dev_priv, &fence, NULL);
• else

• dma_fence_get(&fence->base);
  

• ret = dma_resv_reserve_shared(bo->base.resv, 1);
• if (!ret) dma_resv_add_excl_fence(bo->base.resv, &fence->base);

• dma_fence_put(&fence->base);
  

• } else

• dma_resv_add_excl_fence(bo->base.resv, &fence->base);
  

• else

• /* Last resort fallback when we are OOM */
  

• dma_fence_wait(&fence->base, false);
  

• dma_fence_put(&fence->base);

} -- 2.25.1

On Tue, Dec 07, 2021 at 01:34:02PM +0100, Christian König wrote:

Use dma_resv_get_singleton() here to eventually get more than one write fence as single fence.

Signed-off-by: Christian König christian.koenig@amd.com

Patch title should be drm/atomic-helper: prefix, not just drm:

With that nit:

Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch

---

drivers/gpu/drm/drm_gem_atomic_helper.c | 18 +++++++----------- 1 file changed, 7 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/drm_gem_atomic_helper.c b/drivers/gpu/drm/drm_gem_atomic_helper.c index c3189afe10cb..9338ddb7edff 100644 --- a/drivers/gpu/drm/drm_gem_atomic_helper.c +++ b/drivers/gpu/drm/drm_gem_atomic_helper.c @@ -143,25 +143,21 @@ */ int drm_gem_plane_helper_prepare_fb(struct drm_plane *plane, struct drm_plane_state *state) {

• struct dma_resv_iter cursor; struct drm_gem_object *obj; struct dma_fence *fence;

• int ret;

if (!state->fb) return 0; obj = drm_gem_fb_get_obj(state->fb, 0);

• dma_resv_iter_begin(&cursor, obj->resv, false);
• dma_resv_for_each_fence_unlocked(&cursor, fence) {

• /* TODO: Currently there should be only one write fence, so this
  

•  * here works fine. But drm_atomic_set_fence_for_plane() should
  

•  * be changed to be able to handle more fences in general for
  

•  * multiple BOs per fb anyway. */
  

• dma_fence_get(fence);
  

• break;
  

• }
• dma_resv_iter_end(&cursor);

• ret = dma_resv_get_singleton(obj->resv, false, &fence);
• if (ret)

• return ret;
  

• /* TODO: drm_atomic_set_fence_for_plane() should be changed to be able

• * to handle more fences in general for multiple BOs per fb.
  

• */
  

  drm_atomic_set_fence_for_plane(state, fence); return 0;

}

2.25.1

On Tue, Dec 07, 2021 at 01:34:03PM +0100, Christian König wrote:

Use dma_resv_get_singleton() here to eventually get more than one write fence as single fence.

Signed-off-by: Christian König christian.koenig@amd.com

drivers/gpu/drm/nouveau/dispnv50/wndw.c | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/dispnv50/wndw.c b/drivers/gpu/drm/nouveau/dispnv50/wndw.c index 133c8736426a..b55a8a723581 100644 --- a/drivers/gpu/drm/nouveau/dispnv50/wndw.c +++ b/drivers/gpu/drm/nouveau/dispnv50/wndw.c @@ -536,8 +536,6 @@ nv50_wndw_prepare_fb(struct drm_plane *plane, struct drm_plane_state *state) struct nouveau_bo *nvbo; struct nv50_head_atom *asyh; struct nv50_wndw_ctxdma *ctxdma;

• struct dma_resv_iter cursor;
• struct dma_fence *fence; int ret;

NV_ATOMIC(drm, "%s prepare: %p\n", plane->name, fb); @@ -560,13 +558,11 @@ nv50_wndw_prepare_fb(struct drm_plane *plane, struct drm_plane_state *state) asyw->image.handle[0] = ctxdma->object.handle; }

• dma_resv_iter_begin(&cursor, nvbo->bo.base.resv, false);
• dma_resv_for_each_fence_unlocked(&cursor, fence) {

• /* TODO: We only use the first writer here */
  

• asyw->state.fence = dma_fence_get(fence);
  

• break;
  

• }
• dma_resv_iter_end(&cursor);

• ret = dma_resv_get_singleton(nvbo->bo.base.resv, false,

• 		     &asyw->state.fence);
  

Needs nouveau-ack, but otherwise lgtm.

Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch

• if (ret)

• return ret;
  

• asyw->image.offset[0] = nvbo->offset;

if (wndw->func->prepare) { -- 2.25.1

On Tue, Dec 07, 2021 at 01:34:04PM +0100, Christian König wrote:

Makes the code a bit more simpler.

Signed-off-by: Christian König christian.koenig@amd.com

Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch

---

drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c | 23 +++-------------------- 1 file changed, 3 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c index be48487e2ca7..888d97143177 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c @@ -107,36 +107,19 @@ static void amdgpu_pasid_free_cb(struct dma_fence *fence, void amdgpu_pasid_free_delayed(struct dma_resv *resv, u32 pasid) {

• struct dma_fence *fence, **fences; struct amdgpu_pasid_cb *cb;
• unsigned count;

• struct dma_fence *fence; int r;

• r = dma_resv_get_fences(resv, true, &count, &fences);

• r = dma_resv_get_singleton(resv, true, &fence); if (r) goto fallback;

• if (count == 0) {

• if (!fence) { amdgpu_pasid_free(pasid); return; }

• if (count == 1) {

• fence = fences[0];
  

• kfree(fences);
  

• } else {

• uint64_t context = dma_fence_context_alloc(1);
  

• struct dma_fence_array *array;
  

• array = dma_fence_array_create(count, fences, context,
  

• 			       1, false);
  

• if (!array) {
  

• 	kfree(fences);
  

• 	goto fallback;
  

• }
  

• fence = &array->base;
  

• }
• cb = kmalloc(sizeof(*cb), GFP_KERNEL); if (!cb) { /* Last resort when we are OOM */

-- 2.25.1

On Tue, Dec 07, 2021 at 01:34:07PM +0100, Christian König wrote:

Add an usage for kernel submissions. Waiting for those are mandatory for dynamic DMA-bufs.

Signed-off-by: Christian König christian.koenig@amd.com

Again just skipping to the doc bikeshedding, maybe with more cc others help with some code review too.

EXPORT_SYMBOL(ib_umem_dmabuf_map_pages); diff --git a/include/linux/dma-resv.h b/include/linux/dma-resv.h index 4f3a6abf43c4..29d799991496 100644 --- a/include/linux/dma-resv.h +++ b/include/linux/dma-resv.h @@ -54,8 +54,30 @@ struct dma_resv_list;

• This enum describes the different use cases for a dma_resv object and
• controls which fences are returned when queried.

• • An important fact is that there is the order KERNEL<WRITE<READ and
• • when the dma_resv object is asked for fences for one use case the fences
• • for the lower use case are returned as well.
• • For example when asking for WRITE fences then the KERNEL fences are returned
• • as well. Similar when asked for READ fences then both WRITE and KERNEL
• • fences are returned as well.
  */

enum dma_resv_usage {

• /**

• * @DMA_RESV_USAGE_KERNEL: For in kernel memory management only.
  

• *
  

• * This should only be used for things like copying or clearing memory
  

• * with a DMA hardware engine for the purpose of kernel memory
  

• * management.
  

• *
  

•     * Drivers *always* need to wait for those fences before accessing the
  

s/need to/must/ to stay with usual RFC wording. It's a hard requirement or there's a security bug somewhere.

• * resource protected by the dma_resv object. The only exception for
  

• * that is when the resource is known to be locked down in place by
  

• * pinning it previously.
  

Is this true? This sounds more confusing than helpful, because afaik in general our pin interfaces do not block for any kernel fences. dma_buf_pin doesn't do that for sure. And I don't think ttm does that either.

I think the only safe thing here is to state that it's safe if a) the resource is pinned down and b) the callers has previously waited for the kernel fences.

I also think we should put that wait for kernel fences into dma_buf_pin(), but that's maybe a later patch. -Daniel

• */
  

• DMA_RESV_USAGE_KERNEL,
• /**
  • @DMA_RESV_USAGE_WRITE: Implicit write synchronization.

-- 2.25.1

Am 02.03.22 um 19:11 schrieb Jason Ekstrand:

On Wed, Dec 22, 2021 at 4:05 PM Daniel Vetter daniel@ffwll.ch wrote:

On Tue, Dec 07, 2021 at 01:34:07PM +0100, Christian König wrote:
> Add an usage for kernel submissions. Waiting for those
> are mandatory for dynamic DMA-bufs.
>
> Signed-off-by: Christian König <christian.koenig@amd.com>

Again just skipping to the doc bikeshedding, maybe with more cc others
help with some code review too.

>  EXPORT_SYMBOL(ib_umem_dmabuf_map_pages);
> diff --git a/include/linux/dma-resv.h b/include/linux/dma-resv.h
> index 4f3a6abf43c4..29d799991496 100644
> --- a/include/linux/dma-resv.h
> +++ b/include/linux/dma-resv.h
> @@ -54,8 +54,30 @@ struct dma_resv_list;
>   *
>   * This enum describes the different use cases for a dma_resv
object and
>   * controls which fences are returned when queried.
> + *
> + * An important fact is that there is the order
KERNEL<WRITE<READ and
> + * when the dma_resv object is asked for fences for one use
case the fences
> + * for the lower use case are returned as well.
> + *
> + * For example when asking for WRITE fences then the KERNEL
fences are returned
> + * as well. Similar when asked for READ fences then both WRITE
and KERNEL
> + * fences are returned as well.
>   */
>  enum dma_resv_usage {
> +     /**
> +      * @DMA_RESV_USAGE_KERNEL: For in kernel memory management
only.
> +      *
> +      * This should only be used for things like copying or
clearing memory
> +      * with a DMA hardware engine for the purpose of kernel memory
> +      * management.
> +      *
> +         * Drivers *always* need to wait for those fences
before accessing the

super-nit: Your whitespace is wrong here.

Fixed, thanks.

s/need to/must/ to stay with usual RFC wording. It's a hard
requirement or
there's a security bug somewhere.

Yeah, probably.  I like *must* but that's because that's what we use in the VK spec.  Do whatever's usual for kernel docs.

I agree, must sounds better and is already fixed.

Not sure where to put this comment but I feel like the way things are framed is a bit the wrong way around. Specifically, I don't think we should be talking about what fences you must wait on so much as what fences you can safely skip.  In the previous model, the exclusive fence had to be waited on at all times and the shared fences could be skipped unless you were doing something that would result in a new exclusive fence.

Well exactly that's what we unfortunately didn't do, as Daniel explained some drivers just ignored the exclusive fence sometimes.

In this new world of "it's just a bucket of fences", we need to be very sure the waiting is happening on the right things.  It sounds (I could be wrong) like USAGE_KERNEL is the new exclusive fence.  If so, we need to make it virtually impossible to ignore.

Yes, exactly that's the goal here.

Sorry if that's a bit of a ramble.  I think what I'm saying is this:  In whatever helpers or iterators we have, be that get_singleton or iter_begin or whatever, we need to be sure we specify things in terms of exclusion and not inclusion.  "Give me everything except implicit sync read fences" rather than "give me implicit sync write fences".

Mhm, exactly that's what I tried to avoid. The basic idea here is that the driver and memory management components tells the framework what use case it has and the framework returns the appropriate fences for that.

So when the use case is mmap() the buffer on the CPU without any further sync (for example) you only get the kernel fences.

When the use case is you want to add a CS which is an implicit read you get all kernel fences plus all writers (see function dma_resv_usage_rw).

When the use case is you want to add a CS which is an implicit write you get all kernel fences, other writers as well as readers.

And last when you are the memory management which wants to move a buffer around you get everything.

If having a single, well-ordered enum is sufficient for that, great.  If we think we'll ever end up with something other than a strict ordering, we may need to re-think a bit.

I actually started with a matrix which gives you an indicator when to sync with what, but at least for now the well-ordered enum seems to get the job done as well and is far less complex.

Concerning well-ordering... I'm a bit surprised to only see three values here.  I expected 4:

- kernel exclusive, used for memory moves and the like  - kernel shared, used for "I'm using this right now, don't yank it out from under me" which may not have any implicit sync implications whatsoever  - implicit sync write  - implicit sync read

See the follow up patch which adds DMA_RESV_USAGE_BOOKKEEP. That's the 4th one you are missing.

If we had those four, I don't think the strict ordering works anymore.  From the POV of implicit sync, they would look at the implicit sync read/write fences and maybe not even kernel exclusive.  From the POV of some doing a BO move, they'd look at all of them.  From the POV of holding on to memory while Vulkan is using it, you want to set a kernel shared fence but it doesn't need to interact with implicit sync at all.  Am I missing something obvious here?

Yeah, sounds like you didn't looked at patch 21 :)

My thinking is more or less exactly the same. Only difference is that I've put the BOOKKEEP usage after the implicit read and write usages. This way you can keep the strict ordering since the implicit submissions won't ask for the BOOKKEEP usage.

The order is then KERNEL<WRITE<READ<BOOKKEEP. See the final documentation here as well:

* An important fact is that there is the order KERNEL<WRITE<READ<BOOKKEEP and  * when the dma_resv object is asked for fences for one use case the fences  * for the lower use case are returned as well.  *  * For example when asking for WRITE fences then the KERNEL fences are returned  * as well. Similar when asked for READ fences then both WRITE and KERNEL  * fences are returned as well.

Regards, Christian.

--Jason

> +      * resource protected by the dma_resv object. The only
exception for
> +      * that is when the resource is known to be locked down in
place by
> +      * pinning it previously.

Is this true? This sounds more confusing than helpful, because
afaik in
general our pin interfaces do not block for any kernel fences.
dma_buf_pin
doesn't do that for sure. And I don't think ttm does that either.

I think the only safe thing here is to state that it's safe if a) the
resource is pinned down and b) the callers has previously waited
for the
kernel fences.

I also think we should put that wait for kernel fences into
dma_buf_pin(),
but that's maybe a later patch.
-Daniel



> +      */
> +     DMA_RESV_USAGE_KERNEL,
> +
>       /**
>        * @DMA_RESV_USAGE_WRITE: Implicit write synchronization.
>        *
> --
> 2.25.1
>

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

On Tue, Dec 07, 2021 at 01:34:08PM +0100, Christian König wrote:

Add an usage for submissions independent of implicit sync but still interesting for memory management.

Signed-off-by: Christian König christian.koenig@amd.com

Focusing on the kerneldoc first to get semantics agreed.

diff --git a/include/linux/dma-resv.h b/include/linux/dma-resv.h index 29d799991496..07ae5b00c1fa 100644 --- a/include/linux/dma-resv.h +++ b/include/linux/dma-resv.h @@ -55,7 +55,7 @@ struct dma_resv_list;

• This enum describes the different use cases for a dma_resv object and
• controls which fences are returned when queried.

• • An important fact is that there is the order KERNEL<WRITE<READ and

• • An important fact is that there is the order KERNEL<WRITE<READ<BOOKKEEP and
  • when the dma_resv object is asked for fences for one use case the fences
  • for the lower use case are returned as well.

@@ -93,6 +93,22 @@ enum dma_resv_usage { * an implicit read dependency. */ DMA_RESV_USAGE_READ,

• /**

• * @DMA_RESV_USAGE_BOOKKEEP: No implicit sync.
  

• *
  

• * This should be used by submissions which don't want to participate in
  

• * implicit synchronization.
  

Uh we might still have a disagreement, because that isn't really what drivers which added opt-in implicit sync have done thus far. Minimally we need a note that some drivers also use _READ for this.

• *
  

• * The most common case are submissions with explicit synchronization,
  

• * but also things like preemption fences as well as page table updates
  

• * might use this.
  

• *
  

• * The kernel memory management *always* need to wait for those fences
  

• * before moving or freeing the resource protected by the dma_resv
  

• * object.
  

Yeah this is the comment I wanted to see for READ, and which now is in bookkeeping (where it's correct in the end). I think we still should have something in the READ comment (and here) explaining that there could very well be writes hiding behind this, and that the kernel cannot assume anything about what's going on in general (maybe some drivers enforce read/write through command parsers).

Also all the text in dma_buf.resv needs to be updated to use the right constants instead of words. -Daniel

• */
  

• DMA_RESV_USAGE_BOOKKEEP

}; /** -- 2.25.1

On Tue, Dec 07, 2021 at 01:34:09PM +0100, Christian König wrote:

We have previously done that in the individual drivers but it is more defensive to move that into the common code.

Dynamic attachments should wait for map operations to complete by themselves.

Signed-off-by: Christian König christian.koenig@amd.com

i915 should probably stop reinveinting so much stuff here and align more ...

I do wonder whether we want the same for dma_buf_pin(), or at least document that for dynamic attachments, you still need to sync even if it's pinned. Especially since your kerneldoc for the usage flags suggests that waiting isn't needed, but after this patch waiting _is_ needed even for dynamic importers.

So there is a gap here I think, and I deleted my r-b tag that I already typed again. Or do I miss something?

Minimally needs accurate docs, but I'm leaning towards an unconditional dma_resv_wait() in dma_buf_pin() for safety's sake.

---

drivers/dma-buf/dma-buf.c | 18 +++++++++++++++--- drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c | 14 +------------- drivers/gpu/drm/nouveau/nouveau_prime.c | 17 +---------------- drivers/gpu/drm/radeon/radeon_prime.c | 16 +++------------- 4 files changed, 20 insertions(+), 45 deletions(-)

diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c index 528983d3ba64..d3dd602c4753 100644 --- a/drivers/dma-buf/dma-buf.c +++ b/drivers/dma-buf/dma-buf.c @@ -660,12 +660,24 @@ static struct sg_table * __map_dma_buf(struct dma_buf_attachment *attach, enum dma_data_direction direction) { struct sg_table *sg_table;

• signed long ret;

sg_table = attach->dmabuf->ops->map_dma_buf(attach, direction);

• if (IS_ERR_OR_NULL(sg_table))

• return sg_table;
  

• if (!dma_buf_attachment_is_dynamic(attach)) {

• ret = dma_resv_wait_timeout(attach->dmabuf->resv,
  

Another place where this dma_resv_wait() wrapper would be good. I think we should have it :-)

Cheers, Daniel

• 			    DMA_RESV_USAGE_KERNEL, true,
  

• 			    MAX_SCHEDULE_TIMEOUT);
  

• if (ret < 0) {
  

• 	attach->dmabuf->ops->unmap_dma_buf(attach, sg_table,
  

• 					   direction);
  

• 	return ERR_PTR(ret);
  

• }
  

• }

• if (!IS_ERR_OR_NULL(sg_table))

• mangle_sg_table(sg_table);
  

• mangle_sg_table(sg_table); return sg_table;

} diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c index 4896c876ffec..33127bd56c64 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c @@ -102,21 +102,9 @@ static int amdgpu_dma_buf_pin(struct dma_buf_attachment *attach) { struct drm_gem_object *obj = attach->dmabuf->priv; struct amdgpu_bo *bo = gem_to_amdgpu_bo(obj);

• int r;

/* pin buffer into GTT */

• r = amdgpu_bo_pin(bo, AMDGPU_GEM_DOMAIN_GTT);
• if (r)

• return r;
  

• if (bo->tbo.moving) {

• r = dma_fence_wait(bo->tbo.moving, true);
  

• if (r) {
  

• 	amdgpu_bo_unpin(bo);
  

• 	return r;
  

• }
  

• }
• return 0;

• return amdgpu_bo_pin(bo, AMDGPU_GEM_DOMAIN_GTT);

} /** diff --git a/drivers/gpu/drm/nouveau/nouveau_prime.c b/drivers/gpu/drm/nouveau/nouveau_prime.c index 60019d0532fc..347488685f74 100644 --- a/drivers/gpu/drm/nouveau/nouveau_prime.c +++ b/drivers/gpu/drm/nouveau/nouveau_prime.c @@ -93,22 +93,7 @@ int nouveau_gem_prime_pin(struct drm_gem_object *obj) if (ret) return -EINVAL;

• ret = ttm_bo_reserve(&nvbo->bo, false, false, NULL);
• if (ret)

• goto error;
  

• if (nvbo->bo.moving)

• ret = dma_fence_wait(nvbo->bo.moving, true);
  

• ttm_bo_unreserve(&nvbo->bo);
• if (ret)

• goto error;
  

• return ret;

-error:

• nouveau_bo_unpin(nvbo);
• return ret;

• return 0;

} void nouveau_gem_prime_unpin(struct drm_gem_object *obj) diff --git a/drivers/gpu/drm/radeon/radeon_prime.c b/drivers/gpu/drm/radeon/radeon_prime.c index 4a90807351e7..42a87948e28c 100644 --- a/drivers/gpu/drm/radeon/radeon_prime.c +++ b/drivers/gpu/drm/radeon/radeon_prime.c @@ -77,19 +77,9 @@ int radeon_gem_prime_pin(struct drm_gem_object *obj) /* pin buffer into GTT */ ret = radeon_bo_pin(bo, RADEON_GEM_DOMAIN_GTT, NULL);

• if (unlikely(ret))

• goto error;
  

• if (bo->tbo.moving) {

• ret = dma_fence_wait(bo->tbo.moving, false);
  

• if (unlikely(ret)) {
  

• 	radeon_bo_unpin(bo);
  

• 	goto error;
  

• }
  

• }
• bo->prime_shared_count++;

-error:

• if (likely(ret == 0))

• bo->prime_shared_count++;
  

• radeon_bo_unreserve(bo); return ret;

}

2.25.1