Hi everybody, we recently found that a good bunch of the RCU accesses to the dma_resv object are actually not correctly protected. Those where fixed by either dropping the RCU approach and taking appropriate locks or using a central function to return the current fences as array and then work with that snapshot. This set now tries to prevent adding any new broken code by rolling out two new interfaces to access the fences in a dma_resv object: dma_resv_for_each_fence() - Iterator which should be used while holding the reservation lock. dma_resv_for_each_fence_unlocked() - Iterator based on RCU which can be used without holding the reservation lock and automatic restart on concurrent modification. While doing this we also move the decision which fences to use for write and read accesses into the dma_resv object which results in a quite nice code de-duplication and simplification. The only two remaining users of the RCU shared fence interface are removing shared fences in amdkfd and debugfs code in qxl which will both be addresses in the next patch set. Please review and/or comment, Christian.
Hi everybody, we recently found that a good bunch of the RCU accesses to the dma_resv object are actually not correctly protected. Those where fixed by either dropping the RCU approach and taking appropriate locks or using a central function to return the current fences as array and then work with that snapshot. This set now tries to prevent adding any new broken code by rolling out two new interfaces to access the fences in a dma_resv object: dma_resv_for_each_fence() - Iterator which should be used while holding the reservation lock. dma_resv_for_each_fence_unlocked() - Iterator based on RCU which can be used without holding the reservation lock and automatic restart on concurrent modification. While doing this we also move the decision which fences to use for write and read accesses into the dma_resv object which results in a quite nice code de-duplication and simplification. The only two remaining users of the RCU shared fence interface are removing shared fences in amdkfd and debugfs code in qxl which will both be addresses in the next patch set. Please review and/or comment, Christian.
Abstract the complexity of iterating over all the fences in a dma_resv object. The new loop handles the whole RCU and retry dance and returns only fences where we can be sure we grabbed the right one. Signed-off-by: Christian König <christian.koenig@amd.com> --- drivers/dma-buf/dma-resv.c | 63 ++++++++++++++++++++++++++++++++++++++ include/linux/dma-resv.h | 36 ++++++++++++++++++++++ 2 files changed, 99 insertions(+) diff --git a/drivers/dma-buf/dma-resv.c b/drivers/dma-buf/dma-resv.c index 84fbe60629e3..213a9b7251ca 100644 --- a/drivers/dma-buf/dma-resv.c +++ b/drivers/dma-buf/dma-resv.c @@ -323,6 +323,69 @@ void dma_resv_add_excl_fence(struct dma_resv *obj, struct dma_fence *fence) } EXPORT_SYMBOL(dma_resv_add_excl_fence); +/** + * dma_resv_walk_unlocked - walk over fences in a dma_resv obj + * @obj: the dma_resv object + * @cursor: cursor to record the current position + * @all_fences: true returns also the shared fences + * @first: if we should start over + * + * Return all the fences in the dma_resv object which are not yet signaled. + * The returned fence has an extra local reference so will stay alive. + * If a concurrent modify is detected the whole iterator is started over again. + */ +struct dma_fence *dma_resv_walk_unlocked(struct dma_resv *obj, + struct dma_resv_cursor *cursor, + bool all_fences, bool first) +{ + struct dma_fence *fence = NULL; + + do { + /* Drop the reference from the previous round */ + dma_fence_put(fence); + + cursor->is_first = first; + if (first) { + cursor->seq = read_seqcount_begin(&obj->seq); + cursor->index = -1; + cursor->fences = dma_resv_shared_list(obj); + cursor->is_exclusive = true; + + fence = dma_resv_excl_fence(obj); + if (fence && test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, + &fence->flags)) + fence = NULL; + } else { + fence = NULL; + } + + if (fence) { + fence = dma_fence_get_rcu(fence); + } else if (all_fences && cursor->fences) { + struct dma_resv_list *fences = cursor->fences; + + cursor->is_exclusive = false; + while (++cursor->index < fences->shared_count) { + fence = rcu_dereference(fences->shared[ + cursor->index]); + if (!test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, + &fence->flags)) + break; + } + if (cursor->index < fences->shared_count) + fence = dma_fence_get_rcu(fence); + else + fence = NULL; + } + + /* For the eventually next round */ + first = true; + } while (read_seqcount_retry(&obj->seq, cursor->seq)); + + return fence; +} +EXPORT_SYMBOL_GPL(dma_resv_walk_unlocked); + /** * dma_resv_copy_fences - Copy all fences from src to dst. * @dst: the destination reservation object diff --git a/include/linux/dma-resv.h b/include/linux/dma-resv.h index 9100dd3dc21f..f5b91c292ee0 100644 --- a/include/linux/dma-resv.h +++ b/include/linux/dma-resv.h @@ -149,6 +149,39 @@ struct dma_resv { struct dma_resv_list __rcu *fence; }; +/** + * struct dma_resv_cursor - current position into the dma_resv fences + * @seq: sequence number to check + * @index: index into the shared fences + * @shared: the shared fences + * @is_first: true if this is the first returned fence + * @is_exclusive: if the current fence is the exclusive one + */ +struct dma_resv_cursor { + unsigned int seq; + unsigned int index; + struct dma_resv_list *fences; + bool is_first; + bool is_exclusive; +}; + +/** + * dma_resv_for_each_fence_unlocked - fence iterator + * @obj: a dma_resv object pointer + * @cursor: a struct dma_resv_cursor pointer + * @all_fences: true if all fences should be returned + * @fence: the current fence + * + * Iterate over the fences in a struct dma_resv object without holding the + * dma_resv::lock. The RCU read side lock must be hold when using this, but can + * be dropped and re-taken as necessary inside the loop. @all_fences controls + * if the shared fences are returned as well. + */ +#define dma_resv_for_each_fence_unlocked(obj, cursor, all_fences, fence) \ + for (fence = dma_resv_walk_unlocked(obj, cursor, all_fences, true); \ + fence; dma_fence_put(fence), \ + fence = dma_resv_walk_unlocked(obj, cursor, all_fences, false)) + #define dma_resv_held(obj) lockdep_is_held(&(obj)->lock.base) #define dma_resv_assert_held(obj) lockdep_assert_held(&(obj)->lock.base) @@ -366,6 +399,9 @@ 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_add_excl_fence(struct dma_resv *obj, struct dma_fence *fence); +struct dma_fence *dma_resv_walk_unlocked(struct dma_resv *obj, + struct dma_resv_cursor *cursor, + bool first, bool all_fences); int dma_resv_get_fences(struct dma_resv *obj, struct dma_fence **pfence_excl, unsigned *pshared_count, struct dma_fence ***pshared); int dma_resv_copy_fences(struct dma_resv *dst, struct dma_resv *src); -- 2.25.1
Abstract the complexity of iterating over all the fences in a dma_resv object. The new loop handles the whole RCU and retry dance and returns only fences where we can be sure we grabbed the right one. Signed-off-by: Christian König <christian.koenig@amd.com> --- drivers/dma-buf/dma-resv.c | 63 ++++++++++++++++++++++++++++++++++++++ include/linux/dma-resv.h | 36 ++++++++++++++++++++++ 2 files changed, 99 insertions(+) diff --git a/drivers/dma-buf/dma-resv.c b/drivers/dma-buf/dma-resv.c index 84fbe60629e3..213a9b7251ca 100644 --- a/drivers/dma-buf/dma-resv.c +++ b/drivers/dma-buf/dma-resv.c @@ -323,6 +323,69 @@ void dma_resv_add_excl_fence(struct dma_resv *obj, struct dma_fence *fence) } EXPORT_SYMBOL(dma_resv_add_excl_fence); +/** + * dma_resv_walk_unlocked - walk over fences in a dma_resv obj + * @obj: the dma_resv object + * @cursor: cursor to record the current position + * @all_fences: true returns also the shared fences + * @first: if we should start over + * + * Return all the fences in the dma_resv object which are not yet signaled. + * The returned fence has an extra local reference so will stay alive. + * If a concurrent modify is detected the whole iterator is started over again. + */ +struct dma_fence *dma_resv_walk_unlocked(struct dma_resv *obj, + struct dma_resv_cursor *cursor, + bool all_fences, bool first) +{ + struct dma_fence *fence = NULL; + + do { + /* Drop the reference from the previous round */ + dma_fence_put(fence); + + cursor->is_first = first; + if (first) { + cursor->seq = read_seqcount_begin(&obj->seq); + cursor->index = -1; + cursor->fences = dma_resv_shared_list(obj); + cursor->is_exclusive = true; + + fence = dma_resv_excl_fence(obj); + if (fence && test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, + &fence->flags)) + fence = NULL; + } else { + fence = NULL; + } + + if (fence) { + fence = dma_fence_get_rcu(fence); + } else if (all_fences && cursor->fences) { + struct dma_resv_list *fences = cursor->fences; + + cursor->is_exclusive = false; + while (++cursor->index < fences->shared_count) { + fence = rcu_dereference(fences->shared[ + cursor->index]); + if (!test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, + &fence->flags)) + break; + } + if (cursor->index < fences->shared_count) + fence = dma_fence_get_rcu(fence); + else + fence = NULL; + } + + /* For the eventually next round */ + first = true; + } while (read_seqcount_retry(&obj->seq, cursor->seq)); + + return fence; +} +EXPORT_SYMBOL_GPL(dma_resv_walk_unlocked); + /** * dma_resv_copy_fences - Copy all fences from src to dst. * @dst: the destination reservation object diff --git a/include/linux/dma-resv.h b/include/linux/dma-resv.h index 9100dd3dc21f..f5b91c292ee0 100644 --- a/include/linux/dma-resv.h +++ b/include/linux/dma-resv.h @@ -149,6 +149,39 @@ struct dma_resv { struct dma_resv_list __rcu *fence; }; +/** + * struct dma_resv_cursor - current position into the dma_resv fences + * @seq: sequence number to check + * @index: index into the shared fences + * @shared: the shared fences + * @is_first: true if this is the first returned fence + * @is_exclusive: if the current fence is the exclusive one + */ +struct dma_resv_cursor { + unsigned int seq; + unsigned int index; + struct dma_resv_list *fences; + bool is_first; + bool is_exclusive; +}; + +/** + * dma_resv_for_each_fence_unlocked - fence iterator + * @obj: a dma_resv object pointer + * @cursor: a struct dma_resv_cursor pointer + * @all_fences: true if all fences should be returned + * @fence: the current fence + * + * Iterate over the fences in a struct dma_resv object without holding the + * dma_resv::lock. The RCU read side lock must be hold when using this, but can + * be dropped and re-taken as necessary inside the loop. @all_fences controls + * if the shared fences are returned as well. + */ +#define dma_resv_for_each_fence_unlocked(obj, cursor, all_fences, fence) \ + for (fence = dma_resv_walk_unlocked(obj, cursor, all_fences, true); \ + fence; dma_fence_put(fence), \ + fence = dma_resv_walk_unlocked(obj, cursor, all_fences, false)) + #define dma_resv_held(obj) lockdep_is_held(&(obj)->lock.base) #define dma_resv_assert_held(obj) lockdep_assert_held(&(obj)->lock.base) @@ -366,6 +399,9 @@ 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_add_excl_fence(struct dma_resv *obj, struct dma_fence *fence); +struct dma_fence *dma_resv_walk_unlocked(struct dma_resv *obj, + struct dma_resv_cursor *cursor, + bool first, bool all_fences); int dma_resv_get_fences(struct dma_resv *obj, struct dma_fence **pfence_excl, unsigned *pshared_count, struct dma_fence ***pshared); int dma_resv_copy_fences(struct dma_resv *dst, struct dma_resv *src); -- 2.25.1
A simpler version of the iterator to be used when the dma_resv object is locked. Signed-off-by: Christian König <christian.koenig@amd.com> --- drivers/dma-buf/dma-resv.c | 38 ++++++++++++++++++++++++++++++++++++++ include/linux/dma-resv.h | 18 ++++++++++++++++++ 2 files changed, 56 insertions(+) diff --git a/drivers/dma-buf/dma-resv.c b/drivers/dma-buf/dma-resv.c index 213a9b7251ca..8cbccaae169d 100644 --- a/drivers/dma-buf/dma-resv.c +++ b/drivers/dma-buf/dma-resv.c @@ -323,6 +323,44 @@ void dma_resv_add_excl_fence(struct dma_resv *obj, struct dma_fence *fence) } EXPORT_SYMBOL(dma_resv_add_excl_fence); +/** + * dma_resv_walk - walk over fences in a dma_resv obj + * @obj: the dma_resv object + * @cursor: cursor to record the current position + * @all_fences: true returns also the shared fences + * @first: if we should start over + * + * Return all the fences in the dma_resv object while holding the + * dma_resv::lock. + */ +struct dma_fence *dma_resv_walk(struct dma_resv *obj, + struct dma_resv_cursor *cursor, + bool all_fences, bool first) +{ + dma_resv_assert_held(obj); + + cursor->is_first = first; + if (first) { + struct dma_fence *fence; + + cursor->index = -1; + cursor->fences = dma_resv_shared_list(obj); + cursor->is_exclusive = true; + + fence = dma_resv_excl_fence(obj); + if (fence) + return fence; + } + + if (!all_fences || !cursor->fences || + ++cursor->index >= cursor->fences->shared_count) + return NULL; + + return rcu_dereference_protected(cursor->fences->shared[cursor->index], + dma_resv_held(obj)); +} +EXPORT_SYMBOL_GPL(dma_resv_walk); + /** * dma_resv_walk_unlocked - walk over fences in a dma_resv obj * @obj: the dma_resv object diff --git a/include/linux/dma-resv.h b/include/linux/dma-resv.h index f5b91c292ee0..6f9bb7e4c538 100644 --- a/include/linux/dma-resv.h +++ b/include/linux/dma-resv.h @@ -165,6 +165,21 @@ struct dma_resv_cursor { bool is_exclusive; }; +/** + * dma_resv_for_each_fence - fence iterator + * @obj: a dma_resv object pointer + * @cursor: a struct dma_resv_cursor pointer + * @all_fences: true if all fences should be returned + * @fence: the current fence + * + * Iterate over the fences in a struct dma_resv object while holding the + * dma_resv::lock. @all_fences controls if the shared fences are returned as + * well. + */ +#define dma_resv_for_each_fence(obj, cursor, all_fences, fence) \ + for (fence = dma_resv_walk(obj, cursor, all_fences, true); fence; \ + fence = dma_resv_walk(obj, cursor, all_fences, false)) + /** * dma_resv_for_each_fence_unlocked - fence iterator * @obj: a dma_resv object pointer @@ -399,6 +414,9 @@ 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_add_excl_fence(struct dma_resv *obj, struct dma_fence *fence); +struct dma_fence *dma_resv_walk(struct dma_resv *obj, + struct dma_resv_cursor *cursor, + bool first, bool all_fences); struct dma_fence *dma_resv_walk_unlocked(struct dma_resv *obj, struct dma_resv_cursor *cursor, bool first, bool all_fences); -- 2.25.1
A simpler version of the iterator to be used when the dma_resv object is locked. Signed-off-by: Christian König <christian.koenig@amd.com> --- drivers/dma-buf/dma-resv.c | 38 ++++++++++++++++++++++++++++++++++++++ include/linux/dma-resv.h | 18 ++++++++++++++++++ 2 files changed, 56 insertions(+) diff --git a/drivers/dma-buf/dma-resv.c b/drivers/dma-buf/dma-resv.c index 213a9b7251ca..8cbccaae169d 100644 --- a/drivers/dma-buf/dma-resv.c +++ b/drivers/dma-buf/dma-resv.c @@ -323,6 +323,44 @@ void dma_resv_add_excl_fence(struct dma_resv *obj, struct dma_fence *fence) } EXPORT_SYMBOL(dma_resv_add_excl_fence); +/** + * dma_resv_walk - walk over fences in a dma_resv obj + * @obj: the dma_resv object + * @cursor: cursor to record the current position + * @all_fences: true returns also the shared fences + * @first: if we should start over + * + * Return all the fences in the dma_resv object while holding the + * dma_resv::lock. + */ +struct dma_fence *dma_resv_walk(struct dma_resv *obj, + struct dma_resv_cursor *cursor, + bool all_fences, bool first) +{ + dma_resv_assert_held(obj); + + cursor->is_first = first; + if (first) { + struct dma_fence *fence; + + cursor->index = -1; + cursor->fences = dma_resv_shared_list(obj); + cursor->is_exclusive = true; + + fence = dma_resv_excl_fence(obj); + if (fence) + return fence; + } + + if (!all_fences || !cursor->fences || + ++cursor->index >= cursor->fences->shared_count) + return NULL; + + return rcu_dereference_protected(cursor->fences->shared[cursor->index], + dma_resv_held(obj)); +} +EXPORT_SYMBOL_GPL(dma_resv_walk); + /** * dma_resv_walk_unlocked - walk over fences in a dma_resv obj * @obj: the dma_resv object diff --git a/include/linux/dma-resv.h b/include/linux/dma-resv.h index f5b91c292ee0..6f9bb7e4c538 100644 --- a/include/linux/dma-resv.h +++ b/include/linux/dma-resv.h @@ -165,6 +165,21 @@ struct dma_resv_cursor { bool is_exclusive; }; +/** + * dma_resv_for_each_fence - fence iterator + * @obj: a dma_resv object pointer + * @cursor: a struct dma_resv_cursor pointer + * @all_fences: true if all fences should be returned + * @fence: the current fence + * + * Iterate over the fences in a struct dma_resv object while holding the + * dma_resv::lock. @all_fences controls if the shared fences are returned as + * well. + */ +#define dma_resv_for_each_fence(obj, cursor, all_fences, fence) \ + for (fence = dma_resv_walk(obj, cursor, all_fences, true); fence; \ + fence = dma_resv_walk(obj, cursor, all_fences, false)) + /** * dma_resv_for_each_fence_unlocked - fence iterator * @obj: a dma_resv object pointer @@ -399,6 +414,9 @@ 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_add_excl_fence(struct dma_resv *obj, struct dma_fence *fence); +struct dma_fence *dma_resv_walk(struct dma_resv *obj, + struct dma_resv_cursor *cursor, + bool first, bool all_fences); struct dma_fence *dma_resv_walk_unlocked(struct dma_resv *obj, struct dma_resv_cursor *cursor, bool first, bool all_fences); -- 2.25.1
This makes the function much simpler since the complex retry logic is now handled else where. Signed-off-by: Christian König <christian.koenig@amd.com> --- drivers/dma-buf/dma-resv.c | 81 +++++++++++++++----------------------- 1 file changed, 32 insertions(+), 49 deletions(-) diff --git a/drivers/dma-buf/dma-resv.c b/drivers/dma-buf/dma-resv.c index 8cbccaae169d..9a9c0bba772b 100644 --- a/drivers/dma-buf/dma-resv.c +++ b/drivers/dma-buf/dma-resv.c @@ -433,74 +433,57 @@ EXPORT_SYMBOL_GPL(dma_resv_walk_unlocked); */ int dma_resv_copy_fences(struct dma_resv *dst, struct dma_resv *src) { - struct dma_resv_list *src_list, *dst_list; - struct dma_fence *old, *new; - unsigned int i; + struct dma_resv_cursor cursor; + struct dma_resv_list *list; + struct dma_fence *f, *excl; dma_resv_assert_held(dst); - rcu_read_lock(); - src_list = dma_resv_shared_list(src); + list = NULL; + excl = NULL; -retry: - if (src_list) { - unsigned int shared_count = src_list->shared_count; + rcu_read_lock(); + dma_resv_for_each_fence_unlocked(dst, &cursor, true, f) { - rcu_read_unlock(); + if (cursor.is_first) { + dma_resv_list_free(list); + dma_fence_put(excl); - dst_list = dma_resv_list_alloc(shared_count); - if (!dst_list) - return -ENOMEM; + if (cursor.fences) { + unsigned int cnt = cursor.fences->shared_count; - rcu_read_lock(); - src_list = dma_resv_shared_list(src); - if (!src_list || src_list->shared_count > shared_count) { - kfree(dst_list); - goto retry; - } + rcu_read_unlock(); + list = dma_resv_list_alloc(cnt); + if (!list) + return -ENOMEM; - dst_list->shared_count = 0; - for (i = 0; i < src_list->shared_count; ++i) { - struct dma_fence __rcu **dst; - struct dma_fence *fence; + list->shared_count = 0; + rcu_read_lock(); - fence = rcu_dereference(src_list->shared[i]); - if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, - &fence->flags)) - continue; - - if (!dma_fence_get_rcu(fence)) { - dma_resv_list_free(dst_list); - src_list = dma_resv_shared_list(src); - goto retry; + } else { + list = NULL; } + excl = NULL; + } - if (dma_fence_is_signaled(fence)) { - dma_fence_put(fence); - continue; - } + if (cursor.is_exclusive) + excl = f; + else + RCU_INIT_POINTER(list->shared[list->shared_count++], f); - dst = &dst_list->shared[dst_list->shared_count++]; - rcu_assign_pointer(*dst, fence); - } - } else { - dst_list = NULL; + /* Don't drop the reference */ + f = NULL; } - new = dma_fence_get_rcu_safe(&src->fence_excl); rcu_read_unlock(); - src_list = dma_resv_shared_list(dst); - old = dma_resv_excl_fence(dst); - write_seqcount_begin(&dst->seq); - /* write_seqcount_begin provides the necessary memory barrier */ - RCU_INIT_POINTER(dst->fence_excl, new); - RCU_INIT_POINTER(dst->fence, dst_list); + excl = rcu_replace_pointer(dst->fence_excl, excl, dma_resv_held(dst)); + list = rcu_replace_pointer(dst->fence, list, dma_resv_held(dst)); write_seqcount_end(&dst->seq); - dma_resv_list_free(src_list); - dma_fence_put(old); + dma_resv_list_free(list); + dma_fence_put(excl); return 0; } -- 2.25.1
This makes the function much simpler since the complex retry logic is now handled else where. Signed-off-by: Christian König <christian.koenig@amd.com> --- drivers/dma-buf/dma-resv.c | 81 +++++++++++++++----------------------- 1 file changed, 32 insertions(+), 49 deletions(-) diff --git a/drivers/dma-buf/dma-resv.c b/drivers/dma-buf/dma-resv.c index 8cbccaae169d..9a9c0bba772b 100644 --- a/drivers/dma-buf/dma-resv.c +++ b/drivers/dma-buf/dma-resv.c @@ -433,74 +433,57 @@ EXPORT_SYMBOL_GPL(dma_resv_walk_unlocked); */ int dma_resv_copy_fences(struct dma_resv *dst, struct dma_resv *src) { - struct dma_resv_list *src_list, *dst_list; - struct dma_fence *old, *new; - unsigned int i; + struct dma_resv_cursor cursor; + struct dma_resv_list *list; + struct dma_fence *f, *excl; dma_resv_assert_held(dst); - rcu_read_lock(); - src_list = dma_resv_shared_list(src); + list = NULL; + excl = NULL; -retry: - if (src_list) { - unsigned int shared_count = src_list->shared_count; + rcu_read_lock(); + dma_resv_for_each_fence_unlocked(dst, &cursor, true, f) { - rcu_read_unlock(); + if (cursor.is_first) { + dma_resv_list_free(list); + dma_fence_put(excl); - dst_list = dma_resv_list_alloc(shared_count); - if (!dst_list) - return -ENOMEM; + if (cursor.fences) { + unsigned int cnt = cursor.fences->shared_count; - rcu_read_lock(); - src_list = dma_resv_shared_list(src); - if (!src_list || src_list->shared_count > shared_count) { - kfree(dst_list); - goto retry; - } + rcu_read_unlock(); + list = dma_resv_list_alloc(cnt); + if (!list) + return -ENOMEM; - dst_list->shared_count = 0; - for (i = 0; i < src_list->shared_count; ++i) { - struct dma_fence __rcu **dst; - struct dma_fence *fence; + list->shared_count = 0; + rcu_read_lock(); - fence = rcu_dereference(src_list->shared[i]); - if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, - &fence->flags)) - continue; - - if (!dma_fence_get_rcu(fence)) { - dma_resv_list_free(dst_list); - src_list = dma_resv_shared_list(src); - goto retry; + } else { + list = NULL; } + excl = NULL; + } - if (dma_fence_is_signaled(fence)) { - dma_fence_put(fence); - continue; - } + if (cursor.is_exclusive) + excl = f; + else + RCU_INIT_POINTER(list->shared[list->shared_count++], f); - dst = &dst_list->shared[dst_list->shared_count++]; - rcu_assign_pointer(*dst, fence); - } - } else { - dst_list = NULL; + /* Don't drop the reference */ + f = NULL; } - new = dma_fence_get_rcu_safe(&src->fence_excl); rcu_read_unlock(); - src_list = dma_resv_shared_list(dst); - old = dma_resv_excl_fence(dst); - write_seqcount_begin(&dst->seq); - /* write_seqcount_begin provides the necessary memory barrier */ - RCU_INIT_POINTER(dst->fence_excl, new); - RCU_INIT_POINTER(dst->fence, dst_list); + excl = rcu_replace_pointer(dst->fence_excl, excl, dma_resv_held(dst)); + list = rcu_replace_pointer(dst->fence, list, dma_resv_held(dst)); write_seqcount_end(&dst->seq); - dma_resv_list_free(src_list); - dma_fence_put(old); + dma_resv_list_free(list); + dma_fence_put(excl); return 0; } -- 2.25.1
This makes the function much simpler since the complex retry logic is now handled elsewhere. v2: use sizeof(void*) instead Signed-off-by: Christian König <christian.koenig@amd.com> --- drivers/dma-buf/dma-resv.c | 110 +++++++++++++------------------------ 1 file changed, 37 insertions(+), 73 deletions(-) diff --git a/drivers/dma-buf/dma-resv.c b/drivers/dma-buf/dma-resv.c index 9a9c0bba772b..0a0e3ee62648 100644 --- a/drivers/dma-buf/dma-resv.c +++ b/drivers/dma-buf/dma-resv.c @@ -493,99 +493,63 @@ EXPORT_SYMBOL(dma_resv_copy_fences); * dma_resv_get_fences - Get an object's shared and exclusive * fences without update side lock held * @obj: the reservation object - * @pfence_excl: the returned exclusive fence (or NULL) - * @pshared_count: the number of shared fences returned - * @pshared: the array of shared fence ptrs returned (array is krealloc'd to + * @fence_excl: the returned exclusive fence (or NULL) + * @shared_count: the number of shared fences returned + * @shared: the array of shared fence ptrs returned (array is krealloc'd to * the required size, and must be freed by caller) * * Retrieve all fences from the reservation object. If the pointer for the * exclusive fence is not specified the fence is put into the array of the * shared fences as well. Returns either zero or -ENOMEM. */ -int dma_resv_get_fences(struct dma_resv *obj, struct dma_fence **pfence_excl, - unsigned int *pshared_count, - struct dma_fence ***pshared) +int dma_resv_get_fences(struct dma_resv *obj, struct dma_fence **fence_excl, + unsigned int *shared_count, struct dma_fence ***shared) { - struct dma_fence **shared = NULL; - struct dma_fence *fence_excl; - unsigned int shared_count; - int ret = 1; - - do { - struct dma_resv_list *fobj; - unsigned int i, seq; - size_t sz = 0; - - shared_count = i = 0; - - rcu_read_lock(); - seq = read_seqcount_begin(&obj->seq); - - fence_excl = dma_resv_excl_fence(obj); - if (fence_excl && !dma_fence_get_rcu(fence_excl)) - goto unlock; + struct dma_resv_cursor cursor; + struct dma_fence *fence; - fobj = dma_resv_shared_list(obj); - if (fobj) - sz += sizeof(*shared) * fobj->shared_max; + *shared_count = 0; + *shared = NULL; - if (!pfence_excl && fence_excl) - sz += sizeof(*shared); + if (fence_excl) + *fence_excl = NULL; - if (sz) { - struct dma_fence **nshared; + rcu_read_lock(); + dma_resv_for_each_fence_unlocked(obj, &cursor, true, fence) { - nshared = krealloc(shared, sz, - GFP_NOWAIT | __GFP_NOWARN); - if (!nshared) { - rcu_read_unlock(); + if (cursor.is_first) { + unsigned int count; - dma_fence_put(fence_excl); - fence_excl = NULL; + while (*shared_count) + dma_fence_put((*shared)[--(*shared_count)]); - nshared = krealloc(shared, sz, GFP_KERNEL); - if (nshared) { - shared = nshared; - continue; - } + if (fence_excl) + dma_fence_put(*fence_excl); - ret = -ENOMEM; - break; - } - shared = nshared; - shared_count = fobj ? fobj->shared_count : 0; - for (i = 0; i < shared_count; ++i) { - shared[i] = rcu_dereference(fobj->shared[i]); - if (!dma_fence_get_rcu(shared[i])) - break; - } - } + count = cursor.fences ? cursor.fences->shared_count : 0; + count += fence_excl ? 0 : 1; + rcu_read_unlock(); - if (i != shared_count || read_seqcount_retry(&obj->seq, seq)) { - while (i--) - dma_fence_put(shared[i]); - dma_fence_put(fence_excl); - goto unlock; + /* Eventually re-allocate the array */ + *shared = krealloc_array(*shared, count, + sizeof(void*), + GFP_KERNEL); + if (count && !*shared) + return -ENOMEM; + rcu_read_lock(); } - ret = 0; -unlock: - rcu_read_unlock(); - } while (ret); - - if (pfence_excl) - *pfence_excl = fence_excl; - else if (fence_excl) - shared[shared_count++] = fence_excl; + if (cursor.is_exclusive && fence_excl) + *fence_excl = fence; + else + (*shared)[(*shared_count)++] = fence; - if (!shared_count) { - kfree(shared); - shared = NULL; + /* Don't drop the reference */ + fence = NULL; } + rcu_read_unlock(); - *pshared_count = shared_count; - *pshared = shared; - return ret; + return 0; } EXPORT_SYMBOL_GPL(dma_resv_get_fences); -- 2.25.1
This makes the function much simpler since the complex retry logic is now handled elsewhere. v2: use sizeof(void*) instead Signed-off-by: Christian König <christian.koenig@amd.com> --- drivers/dma-buf/dma-resv.c | 110 +++++++++++++------------------------ 1 file changed, 37 insertions(+), 73 deletions(-) diff --git a/drivers/dma-buf/dma-resv.c b/drivers/dma-buf/dma-resv.c index 9a9c0bba772b..0a0e3ee62648 100644 --- a/drivers/dma-buf/dma-resv.c +++ b/drivers/dma-buf/dma-resv.c @@ -493,99 +493,63 @@ EXPORT_SYMBOL(dma_resv_copy_fences); * dma_resv_get_fences - Get an object's shared and exclusive * fences without update side lock held * @obj: the reservation object - * @pfence_excl: the returned exclusive fence (or NULL) - * @pshared_count: the number of shared fences returned - * @pshared: the array of shared fence ptrs returned (array is krealloc'd to + * @fence_excl: the returned exclusive fence (or NULL) + * @shared_count: the number of shared fences returned + * @shared: the array of shared fence ptrs returned (array is krealloc'd to * the required size, and must be freed by caller) * * Retrieve all fences from the reservation object. If the pointer for the * exclusive fence is not specified the fence is put into the array of the * shared fences as well. Returns either zero or -ENOMEM. */ -int dma_resv_get_fences(struct dma_resv *obj, struct dma_fence **pfence_excl, - unsigned int *pshared_count, - struct dma_fence ***pshared) +int dma_resv_get_fences(struct dma_resv *obj, struct dma_fence **fence_excl, + unsigned int *shared_count, struct dma_fence ***shared) { - struct dma_fence **shared = NULL; - struct dma_fence *fence_excl; - unsigned int shared_count; - int ret = 1; - - do { - struct dma_resv_list *fobj; - unsigned int i, seq; - size_t sz = 0; - - shared_count = i = 0; - - rcu_read_lock(); - seq = read_seqcount_begin(&obj->seq); - - fence_excl = dma_resv_excl_fence(obj); - if (fence_excl && !dma_fence_get_rcu(fence_excl)) - goto unlock; + struct dma_resv_cursor cursor; + struct dma_fence *fence; - fobj = dma_resv_shared_list(obj); - if (fobj) - sz += sizeof(*shared) * fobj->shared_max; + *shared_count = 0; + *shared = NULL; - if (!pfence_excl && fence_excl) - sz += sizeof(*shared); + if (fence_excl) + *fence_excl = NULL; - if (sz) { - struct dma_fence **nshared; + rcu_read_lock(); + dma_resv_for_each_fence_unlocked(obj, &cursor, true, fence) { - nshared = krealloc(shared, sz, - GFP_NOWAIT | __GFP_NOWARN); - if (!nshared) { - rcu_read_unlock(); + if (cursor.is_first) { + unsigned int count; - dma_fence_put(fence_excl); - fence_excl = NULL; + while (*shared_count) + dma_fence_put((*shared)[--(*shared_count)]); - nshared = krealloc(shared, sz, GFP_KERNEL); - if (nshared) { - shared = nshared; - continue; - } + if (fence_excl) + dma_fence_put(*fence_excl); - ret = -ENOMEM; - break; - } - shared = nshared; - shared_count = fobj ? fobj->shared_count : 0; - for (i = 0; i < shared_count; ++i) { - shared[i] = rcu_dereference(fobj->shared[i]); - if (!dma_fence_get_rcu(shared[i])) - break; - } - } + count = cursor.fences ? cursor.fences->shared_count : 0; + count += fence_excl ? 0 : 1; + rcu_read_unlock(); - if (i != shared_count || read_seqcount_retry(&obj->seq, seq)) { - while (i--) - dma_fence_put(shared[i]); - dma_fence_put(fence_excl); - goto unlock; + /* Eventually re-allocate the array */ + *shared = krealloc_array(*shared, count, + sizeof(void*), + GFP_KERNEL); + if (count && !*shared) + return -ENOMEM; + rcu_read_lock(); } - ret = 0; -unlock: - rcu_read_unlock(); - } while (ret); - - if (pfence_excl) - *pfence_excl = fence_excl; - else if (fence_excl) - shared[shared_count++] = fence_excl; + if (cursor.is_exclusive && fence_excl) + *fence_excl = fence; + else + (*shared)[(*shared_count)++] = fence; - if (!shared_count) { - kfree(shared); - shared = NULL; + /* Don't drop the reference */ + fence = NULL; } + rcu_read_unlock(); - *pshared_count = shared_count; - *pshared = shared; - return ret; + return 0; } EXPORT_SYMBOL_GPL(dma_resv_get_fences); -- 2.25.1
This makes the function much simpler since the complex retry logic is now handled elsewhere. Signed-off-by: Christian König <christian.koenig@amd.com> --- drivers/dma-buf/dma-resv.c | 64 +++++--------------------------------- 1 file changed, 7 insertions(+), 57 deletions(-) diff --git a/drivers/dma-buf/dma-resv.c b/drivers/dma-buf/dma-resv.c index 0a0e3ee62648..3ce929b770ea 100644 --- a/drivers/dma-buf/dma-resv.c +++ b/drivers/dma-buf/dma-resv.c @@ -571,74 +571,24 @@ long dma_resv_wait_timeout(struct dma_resv *obj, bool wait_all, bool intr, unsigned long timeout) { long ret = timeout ? timeout : 1; - unsigned int seq, shared_count; + struct dma_resv_cursor cursor; struct dma_fence *fence; - int i; -retry: - shared_count = 0; - seq = read_seqcount_begin(&obj->seq); rcu_read_lock(); - i = -1; - - fence = dma_resv_excl_fence(obj); - if (fence && !test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags)) { - if (!dma_fence_get_rcu(fence)) - goto unlock_retry; + dma_resv_for_each_fence_unlocked(obj, &cursor, wait_all, fence) { + rcu_read_unlock(); - if (dma_fence_is_signaled(fence)) { + ret = dma_fence_wait_timeout(fence, intr, ret); + if (ret <= 0) { dma_fence_put(fence); - fence = NULL; + return ret; } - } else { - fence = NULL; - } - - if (wait_all) { - struct dma_resv_list *fobj = dma_resv_shared_list(obj); - - if (fobj) - shared_count = fobj->shared_count; - - for (i = 0; !fence && i < shared_count; ++i) { - struct dma_fence *lfence; - - lfence = rcu_dereference(fobj->shared[i]); - if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, - &lfence->flags)) - continue; - - if (!dma_fence_get_rcu(lfence)) - goto unlock_retry; - - if (dma_fence_is_signaled(lfence)) { - dma_fence_put(lfence); - continue; - } - - fence = lfence; - break; - } + rcu_read_lock(); } - rcu_read_unlock(); - if (fence) { - if (read_seqcount_retry(&obj->seq, seq)) { - dma_fence_put(fence); - goto retry; - } - ret = dma_fence_wait_timeout(fence, intr, ret); - dma_fence_put(fence); - if (ret > 0 && wait_all && (i + 1 < shared_count)) - goto retry; - } return ret; - -unlock_retry: - rcu_read_unlock(); - goto retry; } EXPORT_SYMBOL_GPL(dma_resv_wait_timeout); -- 2.25.1
This makes the function much simpler since the complex retry logic is now handled elsewhere. Signed-off-by: Christian König <christian.koenig@amd.com> --- drivers/dma-buf/dma-resv.c | 64 +++++--------------------------------- 1 file changed, 7 insertions(+), 57 deletions(-) diff --git a/drivers/dma-buf/dma-resv.c b/drivers/dma-buf/dma-resv.c index 0a0e3ee62648..3ce929b770ea 100644 --- a/drivers/dma-buf/dma-resv.c +++ b/drivers/dma-buf/dma-resv.c @@ -571,74 +571,24 @@ long dma_resv_wait_timeout(struct dma_resv *obj, bool wait_all, bool intr, unsigned long timeout) { long ret = timeout ? timeout : 1; - unsigned int seq, shared_count; + struct dma_resv_cursor cursor; struct dma_fence *fence; - int i; -retry: - shared_count = 0; - seq = read_seqcount_begin(&obj->seq); rcu_read_lock(); - i = -1; - - fence = dma_resv_excl_fence(obj); - if (fence && !test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags)) { - if (!dma_fence_get_rcu(fence)) - goto unlock_retry; + dma_resv_for_each_fence_unlocked(obj, &cursor, wait_all, fence) { + rcu_read_unlock(); - if (dma_fence_is_signaled(fence)) { + ret = dma_fence_wait_timeout(fence, intr, ret); + if (ret <= 0) { dma_fence_put(fence); - fence = NULL; + return ret; } - } else { - fence = NULL; - } - - if (wait_all) { - struct dma_resv_list *fobj = dma_resv_shared_list(obj); - - if (fobj) - shared_count = fobj->shared_count; - - for (i = 0; !fence && i < shared_count; ++i) { - struct dma_fence *lfence; - - lfence = rcu_dereference(fobj->shared[i]); - if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, - &lfence->flags)) - continue; - - if (!dma_fence_get_rcu(lfence)) - goto unlock_retry; - - if (dma_fence_is_signaled(lfence)) { - dma_fence_put(lfence); - continue; - } - - fence = lfence; - break; - } + rcu_read_lock(); } - rcu_read_unlock(); - if (fence) { - if (read_seqcount_retry(&obj->seq, seq)) { - dma_fence_put(fence); - goto retry; - } - ret = dma_fence_wait_timeout(fence, intr, ret); - dma_fence_put(fence); - if (ret > 0 && wait_all && (i + 1 < shared_count)) - goto retry; - } return ret; - -unlock_retry: - rcu_read_unlock(); - goto retry; } EXPORT_SYMBOL_GPL(dma_resv_wait_timeout); -- 2.25.1
This makes the function much simpler since the complex retry logic is now handled elsewhere. Signed-off-by: Christian König <christian.koenig@amd.com> --- drivers/dma-buf/dma-resv.c | 54 +++++--------------------------------- 1 file changed, 7 insertions(+), 47 deletions(-) diff --git a/drivers/dma-buf/dma-resv.c b/drivers/dma-buf/dma-resv.c index 3ce929b770ea..fefc7ec37029 100644 --- a/drivers/dma-buf/dma-resv.c +++ b/drivers/dma-buf/dma-resv.c @@ -593,22 +593,6 @@ long dma_resv_wait_timeout(struct dma_resv *obj, bool wait_all, bool intr, EXPORT_SYMBOL_GPL(dma_resv_wait_timeout); -static inline int dma_resv_test_signaled_single(struct dma_fence *passed_fence) -{ - struct dma_fence *fence, *lfence = passed_fence; - int ret = 1; - - if (!test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &lfence->flags)) { - fence = dma_fence_get_rcu(lfence); - if (!fence) - return -1; - - ret = !!dma_fence_is_signaled(fence); - dma_fence_put(fence); - } - return ret; -} - /** * dma_resv_test_signaled - Test if a reservation object's fences have been * signaled. @@ -625,43 +609,19 @@ static inline int dma_resv_test_signaled_single(struct dma_fence *passed_fence) */ bool dma_resv_test_signaled(struct dma_resv *obj, bool test_all) { + struct dma_resv_cursor cursor; struct dma_fence *fence; - unsigned int seq; - int ret; rcu_read_lock(); -retry: - ret = true; - seq = read_seqcount_begin(&obj->seq); - - if (test_all) { - struct dma_resv_list *fobj = dma_resv_shared_list(obj); - unsigned int i, shared_count; - - shared_count = fobj ? fobj->shared_count : 0; - for (i = 0; i < shared_count; ++i) { - fence = rcu_dereference(fobj->shared[i]); - ret = dma_resv_test_signaled_single(fence); - if (ret < 0) - goto retry; - else if (!ret) - break; + dma_resv_for_each_fence_unlocked(obj, &cursor, test_all, fence) { + if (!dma_fence_is_signaled(fence)) { + rcu_read_unlock(); + dma_fence_put(fence); + return false; } } - - fence = dma_resv_excl_fence(obj); - if (ret && fence) { - ret = dma_resv_test_signaled_single(fence); - if (ret < 0) - goto retry; - - } - - if (read_seqcount_retry(&obj->seq, seq)) - goto retry; - rcu_read_unlock(); - return ret; + return true; } EXPORT_SYMBOL_GPL(dma_resv_test_signaled); -- 2.25.1
This makes the function much simpler since the complex retry logic is now handled elsewhere. Signed-off-by: Christian König <christian.koenig@amd.com> --- drivers/dma-buf/dma-resv.c | 54 +++++--------------------------------- 1 file changed, 7 insertions(+), 47 deletions(-) diff --git a/drivers/dma-buf/dma-resv.c b/drivers/dma-buf/dma-resv.c index 3ce929b770ea..fefc7ec37029 100644 --- a/drivers/dma-buf/dma-resv.c +++ b/drivers/dma-buf/dma-resv.c @@ -593,22 +593,6 @@ long dma_resv_wait_timeout(struct dma_resv *obj, bool wait_all, bool intr, EXPORT_SYMBOL_GPL(dma_resv_wait_timeout); -static inline int dma_resv_test_signaled_single(struct dma_fence *passed_fence) -{ - struct dma_fence *fence, *lfence = passed_fence; - int ret = 1; - - if (!test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &lfence->flags)) { - fence = dma_fence_get_rcu(lfence); - if (!fence) - return -1; - - ret = !!dma_fence_is_signaled(fence); - dma_fence_put(fence); - } - return ret; -} - /** * dma_resv_test_signaled - Test if a reservation object's fences have been * signaled. @@ -625,43 +609,19 @@ static inline int dma_resv_test_signaled_single(struct dma_fence *passed_fence) */ bool dma_resv_test_signaled(struct dma_resv *obj, bool test_all) { + struct dma_resv_cursor cursor; struct dma_fence *fence; - unsigned int seq; - int ret; rcu_read_lock(); -retry: - ret = true; - seq = read_seqcount_begin(&obj->seq); - - if (test_all) { - struct dma_resv_list *fobj = dma_resv_shared_list(obj); - unsigned int i, shared_count; - - shared_count = fobj ? fobj->shared_count : 0; - for (i = 0; i < shared_count; ++i) { - fence = rcu_dereference(fobj->shared[i]); - ret = dma_resv_test_signaled_single(fence); - if (ret < 0) - goto retry; - else if (!ret) - break; + dma_resv_for_each_fence_unlocked(obj, &cursor, test_all, fence) { + if (!dma_fence_is_signaled(fence)) { + rcu_read_unlock(); + dma_fence_put(fence); + return false; } } - - fence = dma_resv_excl_fence(obj); - if (ret && fence) { - ret = dma_resv_test_signaled_single(fence); - if (ret < 0) - goto retry; - - } - - if (read_seqcount_retry(&obj->seq, seq)) - goto retry; - rcu_read_unlock(); - return ret; + return true; } EXPORT_SYMBOL_GPL(dma_resv_test_signaled); -- 2.25.1
This is probably a fix since we didn't even grabed a reference to the fences. Signed-off-by: Christian König <christian.koenig@amd.com> --- drivers/gpu/drm/ttm/ttm_bo.c | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-) diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c index 0a3127436f61..5dd0c3dfec3c 100644 --- a/drivers/gpu/drm/ttm/ttm_bo.c +++ b/drivers/gpu/drm/ttm/ttm_bo.c @@ -269,19 +269,11 @@ static int ttm_bo_individualize_resv(struct ttm_buffer_object *bo) static void ttm_bo_flush_all_fences(struct ttm_buffer_object *bo) { struct dma_resv *resv = &bo->base._resv; - struct dma_resv_list *fobj; + struct dma_resv_cursor cursor; struct dma_fence *fence; - int i; rcu_read_lock(); - fobj = dma_resv_shared_list(resv); - fence = dma_resv_excl_fence(resv); - if (fence && !fence->ops->signaled) - dma_fence_enable_sw_signaling(fence); - - for (i = 0; fobj && i < fobj->shared_count; ++i) { - fence = rcu_dereference(fobj->shared[i]); - + dma_resv_for_each_fence_unlocked(resv, &cursor, true, fence) { if (!fence->ops->signaled) dma_fence_enable_sw_signaling(fence); } -- 2.25.1
This is probably a fix since we didn't even grabed a reference to the fences. Signed-off-by: Christian König <christian.koenig@amd.com> --- drivers/gpu/drm/ttm/ttm_bo.c | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-) diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c index 0a3127436f61..5dd0c3dfec3c 100644 --- a/drivers/gpu/drm/ttm/ttm_bo.c +++ b/drivers/gpu/drm/ttm/ttm_bo.c @@ -269,19 +269,11 @@ static int ttm_bo_individualize_resv(struct ttm_buffer_object *bo) static void ttm_bo_flush_all_fences(struct ttm_buffer_object *bo) { struct dma_resv *resv = &bo->base._resv; - struct dma_resv_list *fobj; + struct dma_resv_cursor cursor; struct dma_fence *fence; - int i; rcu_read_lock(); - fobj = dma_resv_shared_list(resv); - fence = dma_resv_excl_fence(resv); - if (fence && !fence->ops->signaled) - dma_fence_enable_sw_signaling(fence); - - for (i = 0; fobj && i < fobj->shared_count; ++i) { - fence = rcu_dereference(fobj->shared[i]); - + dma_resv_for_each_fence_unlocked(resv, &cursor, true, fence) { if (!fence->ops->signaled) dma_fence_enable_sw_signaling(fence); } -- 2.25.1
Simplifying the code a bit. Signed-off-by: Christian König <christian.koenig@amd.com> --- drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c | 44 ++++++++---------------- 1 file changed, 14 insertions(+), 30 deletions(-) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c index 862eb3c1c4c5..031ba20debb9 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c @@ -252,41 +252,25 @@ int amdgpu_sync_resv(struct amdgpu_device *adev, struct amdgpu_sync *sync, struct dma_resv *resv, enum amdgpu_sync_mode mode, void *owner) { - struct dma_resv_list *flist; + struct dma_resv_cursor cursor; struct dma_fence *f; - unsigned i; - int r = 0; + int r; if (resv == NULL) return -EINVAL; - /* always sync to the exclusive fence */ - f = dma_resv_excl_fence(resv); - dma_fence_chain_for_each(f, f) { - struct dma_fence_chain *chain = to_dma_fence_chain(f); - - if (amdgpu_sync_test_fence(adev, mode, owner, chain ? - chain->fence : f)) { - r = amdgpu_sync_fence(sync, f); - dma_fence_put(f); - if (r) - return r; - break; - } - } - - flist = dma_resv_shared_list(resv); - if (!flist) - return 0; - - for (i = 0; i < flist->shared_count; ++i) { - f = rcu_dereference_protected(flist->shared[i], - dma_resv_held(resv)); - - if (amdgpu_sync_test_fence(adev, mode, owner, f)) { - r = amdgpu_sync_fence(sync, f); - if (r) - return r; + dma_resv_for_each_fence(resv, &cursor, true, f) { + dma_fence_chain_for_each(f, f) { + struct dma_fence_chain *chain = to_dma_fence_chain(f); + + if (amdgpu_sync_test_fence(adev, mode, owner, chain ? + chain->fence : f)) { + r = amdgpu_sync_fence(sync, f); + dma_fence_put(f); + if (r) + return r; + break; + } } } return 0; -- 2.25.1
Simplifying the code a bit. Signed-off-by: Christian König <christian.koenig@amd.com> --- drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c | 44 ++++++++---------------- 1 file changed, 14 insertions(+), 30 deletions(-) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c index 862eb3c1c4c5..031ba20debb9 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c @@ -252,41 +252,25 @@ int amdgpu_sync_resv(struct amdgpu_device *adev, struct amdgpu_sync *sync, struct dma_resv *resv, enum amdgpu_sync_mode mode, void *owner) { - struct dma_resv_list *flist; + struct dma_resv_cursor cursor; struct dma_fence *f; - unsigned i; - int r = 0; + int r; if (resv == NULL) return -EINVAL; - /* always sync to the exclusive fence */ - f = dma_resv_excl_fence(resv); - dma_fence_chain_for_each(f, f) { - struct dma_fence_chain *chain = to_dma_fence_chain(f); - - if (amdgpu_sync_test_fence(adev, mode, owner, chain ? - chain->fence : f)) { - r = amdgpu_sync_fence(sync, f); - dma_fence_put(f); - if (r) - return r; - break; - } - } - - flist = dma_resv_shared_list(resv); - if (!flist) - return 0; - - for (i = 0; i < flist->shared_count; ++i) { - f = rcu_dereference_protected(flist->shared[i], - dma_resv_held(resv)); - - if (amdgpu_sync_test_fence(adev, mode, owner, f)) { - r = amdgpu_sync_fence(sync, f); - if (r) - return r; + dma_resv_for_each_fence(resv, &cursor, true, f) { + dma_fence_chain_for_each(f, f) { + struct dma_fence_chain *chain = to_dma_fence_chain(f); + + if (amdgpu_sync_test_fence(adev, mode, owner, chain ? + chain->fence : f)) { + r = amdgpu_sync_fence(sync, f); + dma_fence_put(f); + if (r) + return r; + break; + } } } return 0; -- 2.25.1
Simplifying the code a bit. Signed-off-by: Christian König <christian.koenig@amd.com> --- drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c index 489e22190e29..0a927006ba9c 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c @@ -1332,10 +1332,9 @@ static bool amdgpu_ttm_bo_eviction_valuable(struct ttm_buffer_object *bo, const struct ttm_place *place) { unsigned long num_pages = bo->resource->num_pages; + struct dma_resv_cursor resv_cursor; struct amdgpu_res_cursor cursor; - struct dma_resv_list *flist; struct dma_fence *f; - int i; /* Swapout? */ if (bo->resource->mem_type == TTM_PL_SYSTEM) @@ -1349,14 +1348,9 @@ static bool amdgpu_ttm_bo_eviction_valuable(struct ttm_buffer_object *bo, * If true, then return false as any KFD process needs all its BOs to * be resident to run successfully */ - flist = dma_resv_shared_list(bo->base.resv); - if (flist) { - for (i = 0; i < flist->shared_count; ++i) { - f = rcu_dereference_protected(flist->shared[i], - dma_resv_held(bo->base.resv)); - if (amdkfd_fence_check_mm(f, current->mm)) - return false; - } + dma_resv_for_each_fence(bo->base.resv, &resv_cursor, true, f) { + if (amdkfd_fence_check_mm(f, current->mm)) + return false; } switch (bo->resource->mem_type) { -- 2.25.1
Simplifying the code a bit. Signed-off-by: Christian König <christian.koenig@amd.com> --- drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c index 489e22190e29..0a927006ba9c 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c @@ -1332,10 +1332,9 @@ static bool amdgpu_ttm_bo_eviction_valuable(struct ttm_buffer_object *bo, const struct ttm_place *place) { unsigned long num_pages = bo->resource->num_pages; + struct dma_resv_cursor resv_cursor; struct amdgpu_res_cursor cursor; - struct dma_resv_list *flist; struct dma_fence *f; - int i; /* Swapout? */ if (bo->resource->mem_type == TTM_PL_SYSTEM) @@ -1349,14 +1348,9 @@ static bool amdgpu_ttm_bo_eviction_valuable(struct ttm_buffer_object *bo, * If true, then return false as any KFD process needs all its BOs to * be resident to run successfully */ - flist = dma_resv_shared_list(bo->base.resv); - if (flist) { - for (i = 0; i < flist->shared_count; ++i) { - f = rcu_dereference_protected(flist->shared[i], - dma_resv_held(bo->base.resv)); - if (amdkfd_fence_check_mm(f, current->mm)) - return false; - } + dma_resv_for_each_fence(bo->base.resv, &resv_cursor, true, f) { + if (amdkfd_fence_check_mm(f, current->mm)) + return false; } switch (bo->resource->mem_type) { -- 2.25.1
Simplifying the code a bit. Also drop the RCU read side lock since the object is locked anyway. Untested since I can't get the driver to compile on !ARM. Signed-off-by: Christian König <christian.koenig@amd.com> --- drivers/gpu/drm/msm/msm_gem.c | 19 +++++-------------- 1 file changed, 5 insertions(+), 14 deletions(-) diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c index 5db07fc287ad..8ee4e8881b03 100644 --- a/drivers/gpu/drm/msm/msm_gem.c +++ b/drivers/gpu/drm/msm/msm_gem.c @@ -906,7 +906,7 @@ void msm_gem_describe(struct drm_gem_object *obj, struct seq_file *m, { struct msm_gem_object *msm_obj = to_msm_bo(obj); struct dma_resv *robj = obj->resv; - struct dma_resv_list *fobj; + struct dma_resv_cursor cursor; struct dma_fence *fence; struct msm_gem_vma *vma; uint64_t off = drm_vma_node_start(&obj->vma_node); @@ -981,22 +981,13 @@ void msm_gem_describe(struct drm_gem_object *obj, struct seq_file *m, seq_puts(m, "\n"); } - rcu_read_lock(); - fobj = dma_resv_shared_list(robj); - if (fobj) { - unsigned int i, shared_count = fobj->shared_count; - - for (i = 0; i < shared_count; i++) { - fence = rcu_dereference(fobj->shared[i]); + dma_resv_for_each_fence(robj, &cursor, true, fence) { + if (cursor.is_exclusive) + describe_fence(fence, "Exclusive", m); + else describe_fence(fence, "Shared", m); - } } - fence = dma_resv_excl_fence(robj); - if (fence) - describe_fence(fence, "Exclusive", m); - rcu_read_unlock(); - msm_gem_unlock(obj); } -- 2.25.1
Simplifying the code a bit. Also drop the RCU read side lock since the object is locked anyway. Untested since I can't get the driver to compile on !ARM. Signed-off-by: Christian König <christian.koenig@amd.com> --- drivers/gpu/drm/msm/msm_gem.c | 19 +++++-------------- 1 file changed, 5 insertions(+), 14 deletions(-) diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c index 5db07fc287ad..8ee4e8881b03 100644 --- a/drivers/gpu/drm/msm/msm_gem.c +++ b/drivers/gpu/drm/msm/msm_gem.c @@ -906,7 +906,7 @@ void msm_gem_describe(struct drm_gem_object *obj, struct seq_file *m, { struct msm_gem_object *msm_obj = to_msm_bo(obj); struct dma_resv *robj = obj->resv; - struct dma_resv_list *fobj; + struct dma_resv_cursor cursor; struct dma_fence *fence; struct msm_gem_vma *vma; uint64_t off = drm_vma_node_start(&obj->vma_node); @@ -981,22 +981,13 @@ void msm_gem_describe(struct drm_gem_object *obj, struct seq_file *m, seq_puts(m, "\n"); } - rcu_read_lock(); - fobj = dma_resv_shared_list(robj); - if (fobj) { - unsigned int i, shared_count = fobj->shared_count; - - for (i = 0; i < shared_count; i++) { - fence = rcu_dereference(fobj->shared[i]); + dma_resv_for_each_fence(robj, &cursor, true, fence) { + if (cursor.is_exclusive) + describe_fence(fence, "Exclusive", m); + else describe_fence(fence, "Shared", m); - } } - fence = dma_resv_excl_fence(robj); - if (fence) - describe_fence(fence, "Exclusive", m); - rcu_read_unlock(); - msm_gem_unlock(obj); } -- 2.25.1
Simplifying the code a bit. Signed-off-by: Christian König <christian.koenig@amd.com> --- drivers/gpu/drm/radeon/radeon_sync.c | 22 +++------------------- 1 file changed, 3 insertions(+), 19 deletions(-) diff --git a/drivers/gpu/drm/radeon/radeon_sync.c b/drivers/gpu/drm/radeon/radeon_sync.c index 9257b60144c4..14a4d8135bad 100644 --- a/drivers/gpu/drm/radeon/radeon_sync.c +++ b/drivers/gpu/drm/radeon/radeon_sync.c @@ -91,33 +91,17 @@ int radeon_sync_resv(struct radeon_device *rdev, struct dma_resv *resv, bool shared) { - struct dma_resv_list *flist; - struct dma_fence *f; + struct dma_resv_cursor cursor; struct radeon_fence *fence; - unsigned i; + struct dma_fence *f; int r = 0; - /* always sync to the exclusive fence */ - f = dma_resv_excl_fence(resv); - fence = f ? to_radeon_fence(f) : NULL; - if (fence && fence->rdev == rdev) - radeon_sync_fence(sync, fence); - else if (f) - r = dma_fence_wait(f, true); - - flist = dma_resv_shared_list(resv); - if (shared || !flist || r) - return r; - - for (i = 0; i < flist->shared_count; ++i) { - f = rcu_dereference_protected(flist->shared[i], - dma_resv_held(resv)); + dma_resv_for_each_fence(resv, &cursor, shared, f) { fence = to_radeon_fence(f); if (fence && fence->rdev == rdev) radeon_sync_fence(sync, fence); else r = dma_fence_wait(f, true); - if (r) break; } -- 2.25.1
Simplifying the code a bit. Signed-off-by: Christian König <christian.koenig@amd.com> --- drivers/gpu/drm/radeon/radeon_sync.c | 22 +++------------------- 1 file changed, 3 insertions(+), 19 deletions(-) diff --git a/drivers/gpu/drm/radeon/radeon_sync.c b/drivers/gpu/drm/radeon/radeon_sync.c index 9257b60144c4..14a4d8135bad 100644 --- a/drivers/gpu/drm/radeon/radeon_sync.c +++ b/drivers/gpu/drm/radeon/radeon_sync.c @@ -91,33 +91,17 @@ int radeon_sync_resv(struct radeon_device *rdev, struct dma_resv *resv, bool shared) { - struct dma_resv_list *flist; - struct dma_fence *f; + struct dma_resv_cursor cursor; struct radeon_fence *fence; - unsigned i; + struct dma_fence *f; int r = 0; - /* always sync to the exclusive fence */ - f = dma_resv_excl_fence(resv); - fence = f ? to_radeon_fence(f) : NULL; - if (fence && fence->rdev == rdev) - radeon_sync_fence(sync, fence); - else if (f) - r = dma_fence_wait(f, true); - - flist = dma_resv_shared_list(resv); - if (shared || !flist || r) - return r; - - for (i = 0; i < flist->shared_count; ++i) { - f = rcu_dereference_protected(flist->shared[i], - dma_resv_held(resv)); + dma_resv_for_each_fence(resv, &cursor, shared, f) { fence = to_radeon_fence(f); if (fence && fence->rdev == rdev) radeon_sync_fence(sync, fence); else r = dma_fence_wait(f, true); - if (r) break; } -- 2.25.1
Simplifying the code a bit. Signed-off-by: Christian König <christian.koenig@amd.com> --- drivers/gpu/drm/scheduler/sched_main.c | 26 +++++++------------------- 1 file changed, 7 insertions(+), 19 deletions(-) diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c index 6987d412a946..b64b673ed040 100644 --- a/drivers/gpu/drm/scheduler/sched_main.c +++ b/drivers/gpu/drm/scheduler/sched_main.c @@ -699,29 +699,17 @@ int drm_sched_job_add_implicit_dependencies(struct drm_sched_job *job, struct drm_gem_object *obj, bool write) { + struct dma_resv_cursor cursor; + struct dma_fence *fence; int ret; - struct dma_fence **fences; - unsigned int i, fence_count; - - if (!write) { - struct dma_fence *fence = dma_resv_get_excl_unlocked(obj->resv); - return drm_sched_job_add_dependency(job, fence); - } - - ret = dma_resv_get_fences(obj->resv, NULL, &fence_count, &fences); - if (ret || !fence_count) - return ret; - - for (i = 0; i < fence_count; i++) { - ret = drm_sched_job_add_dependency(job, fences[i]); - if (ret) + dma_resv_for_each_fence_unlocked(obj->resv, &cursor, write, fence) { + ret = drm_sched_job_add_dependency(job, fence); + if (ret) { + dma_fence_put(fence); break; + } } - - for (; i < fence_count; i++) - dma_fence_put(fences[i]); - kfree(fences); return ret; } EXPORT_SYMBOL(drm_sched_job_add_implicit_dependencies); -- 2.25.1
Simplifying the code a bit. Signed-off-by: Christian König <christian.koenig@amd.com> --- drivers/gpu/drm/scheduler/sched_main.c | 26 +++++++------------------- 1 file changed, 7 insertions(+), 19 deletions(-) diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c index 6987d412a946..b64b673ed040 100644 --- a/drivers/gpu/drm/scheduler/sched_main.c +++ b/drivers/gpu/drm/scheduler/sched_main.c @@ -699,29 +699,17 @@ int drm_sched_job_add_implicit_dependencies(struct drm_sched_job *job, struct drm_gem_object *obj, bool write) { + struct dma_resv_cursor cursor; + struct dma_fence *fence; int ret; - struct dma_fence **fences; - unsigned int i, fence_count; - - if (!write) { - struct dma_fence *fence = dma_resv_get_excl_unlocked(obj->resv); - return drm_sched_job_add_dependency(job, fence); - } - - ret = dma_resv_get_fences(obj->resv, NULL, &fence_count, &fences); - if (ret || !fence_count) - return ret; - - for (i = 0; i < fence_count; i++) { - ret = drm_sched_job_add_dependency(job, fences[i]); - if (ret) + dma_resv_for_each_fence_unlocked(obj->resv, &cursor, write, fence) { + ret = drm_sched_job_add_dependency(job, fence); + if (ret) { + dma_fence_put(fence); break; + } } - - for (; i < fence_count; i++) - dma_fence_put(fences[i]); - kfree(fences); return ret; } EXPORT_SYMBOL(drm_sched_job_add_implicit_dependencies); -- 2.25.1
This makes the function much simpler since the complex retry logic is now handled else where. Signed-off-by: Christian König <christian.koenig@amd.com> --- drivers/gpu/drm/i915/gem/i915_gem_busy.c | 30 +++++++----------------- 1 file changed, 9 insertions(+), 21 deletions(-) diff --git a/drivers/gpu/drm/i915/gem/i915_gem_busy.c b/drivers/gpu/drm/i915/gem/i915_gem_busy.c index 6234e17259c1..c6c6d747b33e 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_busy.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_busy.c @@ -82,8 +82,8 @@ i915_gem_busy_ioctl(struct drm_device *dev, void *data, { struct drm_i915_gem_busy *args = data; struct drm_i915_gem_object *obj; - struct dma_resv_list *list; - unsigned int seq; + struct dma_resv_cursor cursor; + struct dma_fence *fence; int err; err = -ENOENT; @@ -109,28 +109,16 @@ i915_gem_busy_ioctl(struct drm_device *dev, void *data, * to report the overall busyness. This is what the wait-ioctl does. * */ -retry: - seq = raw_read_seqcount(&obj->base.resv->seq); - - /* Translate the exclusive fence to the READ *and* WRITE engine */ - args->busy = busy_check_writer(dma_resv_excl_fence(obj->base.resv)); - - /* Translate shared fences to READ set of engines */ - list = dma_resv_shared_list(obj->base.resv); - if (list) { - unsigned int shared_count = list->shared_count, i; - - for (i = 0; i < shared_count; ++i) { - struct dma_fence *fence = - rcu_dereference(list->shared[i]); - + args->busy = false; + dma_resv_for_each_fence_unlocked(obj->base.resv, &cursor, true, fence) { + if (cursor.is_exclusive) + /* Translate the exclusive fence to the READ *and* WRITE engine */ + args->busy = busy_check_writer(fence); + else + /* Translate shared fences to READ set of engines */ args->busy |= busy_check_reader(fence); - } } - if (args->busy && read_seqcount_retry(&obj->base.resv->seq, seq)) - goto retry; - err = 0; out: rcu_read_unlock(); -- 2.25.1
This makes the function much simpler since the complex retry logic is now handled else where. Signed-off-by: Christian König <christian.koenig@amd.com> --- drivers/gpu/drm/i915/gem/i915_gem_busy.c | 30 +++++++----------------- 1 file changed, 9 insertions(+), 21 deletions(-) diff --git a/drivers/gpu/drm/i915/gem/i915_gem_busy.c b/drivers/gpu/drm/i915/gem/i915_gem_busy.c index 6234e17259c1..c6c6d747b33e 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_busy.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_busy.c @@ -82,8 +82,8 @@ i915_gem_busy_ioctl(struct drm_device *dev, void *data, { struct drm_i915_gem_busy *args = data; struct drm_i915_gem_object *obj; - struct dma_resv_list *list; - unsigned int seq; + struct dma_resv_cursor cursor; + struct dma_fence *fence; int err; err = -ENOENT; @@ -109,28 +109,16 @@ i915_gem_busy_ioctl(struct drm_device *dev, void *data, * to report the overall busyness. This is what the wait-ioctl does. * */ -retry: - seq = raw_read_seqcount(&obj->base.resv->seq); - - /* Translate the exclusive fence to the READ *and* WRITE engine */ - args->busy = busy_check_writer(dma_resv_excl_fence(obj->base.resv)); - - /* Translate shared fences to READ set of engines */ - list = dma_resv_shared_list(obj->base.resv); - if (list) { - unsigned int shared_count = list->shared_count, i; - - for (i = 0; i < shared_count; ++i) { - struct dma_fence *fence = - rcu_dereference(list->shared[i]); - + args->busy = false; + dma_resv_for_each_fence_unlocked(obj->base.resv, &cursor, true, fence) { + if (cursor.is_exclusive) + /* Translate the exclusive fence to the READ *and* WRITE engine */ + args->busy = busy_check_writer(fence); + else + /* Translate shared fences to READ set of engines */ args->busy |= busy_check_reader(fence); - } } - if (args->busy && read_seqcount_retry(&obj->base.resv->seq, seq)) - goto retry; - err = 0; out: rcu_read_unlock(); -- 2.25.1
Simplifying the code a bit. Signed-off-by: Christian König <christian.koenig@amd.com> --- drivers/gpu/drm/i915/i915_sw_fence.c | 52 ++++++---------------------- 1 file changed, 10 insertions(+), 42 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_sw_fence.c b/drivers/gpu/drm/i915/i915_sw_fence.c index c589a681da77..09d2c9f96c52 100644 --- a/drivers/gpu/drm/i915/i915_sw_fence.c +++ b/drivers/gpu/drm/i915/i915_sw_fence.c @@ -572,56 +572,24 @@ int i915_sw_fence_await_reservation(struct i915_sw_fence *fence, unsigned long timeout, gfp_t gfp) { - struct dma_fence *excl; + struct dma_resv_cursor cursor; + struct dma_fence *f; int ret = 0, pending; debug_fence_assert(fence); might_sleep_if(gfpflags_allow_blocking(gfp)); - if (write) { - struct dma_fence **shared; - unsigned int count, i; - - ret = dma_resv_get_fences(resv, &excl, &count, &shared); - if (ret) - return ret; - - for (i = 0; i < count; i++) { - if (shared[i]->ops == exclude) - continue; - - pending = i915_sw_fence_await_dma_fence(fence, - shared[i], - timeout, - gfp); - if (pending < 0) { - ret = pending; - break; - } - - ret |= pending; - } - - for (i = 0; i < count; i++) - dma_fence_put(shared[i]); - kfree(shared); - } else { - excl = dma_resv_get_excl_unlocked(resv); - } - - if (ret >= 0 && excl && excl->ops != exclude) { - pending = i915_sw_fence_await_dma_fence(fence, - excl, - timeout, + dma_resv_for_each_fence_unlocked(resv, &cursor, write, f) { + pending = i915_sw_fence_await_dma_fence(fence, f, timeout, gfp); - if (pending < 0) + if (pending < 0) { ret = pending; - else - ret |= pending; - } - - dma_fence_put(excl); + dma_fence_put(f); + break; + } + ret |= pending; + } return ret; } -- 2.25.1
Simplifying the code a bit. Signed-off-by: Christian König <christian.koenig@amd.com> --- drivers/gpu/drm/i915/i915_sw_fence.c | 52 ++++++---------------------- 1 file changed, 10 insertions(+), 42 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_sw_fence.c b/drivers/gpu/drm/i915/i915_sw_fence.c index c589a681da77..09d2c9f96c52 100644 --- a/drivers/gpu/drm/i915/i915_sw_fence.c +++ b/drivers/gpu/drm/i915/i915_sw_fence.c @@ -572,56 +572,24 @@ int i915_sw_fence_await_reservation(struct i915_sw_fence *fence, unsigned long timeout, gfp_t gfp) { - struct dma_fence *excl; + struct dma_resv_cursor cursor; + struct dma_fence *f; int ret = 0, pending; debug_fence_assert(fence); might_sleep_if(gfpflags_allow_blocking(gfp)); - if (write) { - struct dma_fence **shared; - unsigned int count, i; - - ret = dma_resv_get_fences(resv, &excl, &count, &shared); - if (ret) - return ret; - - for (i = 0; i < count; i++) { - if (shared[i]->ops == exclude) - continue; - - pending = i915_sw_fence_await_dma_fence(fence, - shared[i], - timeout, - gfp); - if (pending < 0) { - ret = pending; - break; - } - - ret |= pending; - } - - for (i = 0; i < count; i++) - dma_fence_put(shared[i]); - kfree(shared); - } else { - excl = dma_resv_get_excl_unlocked(resv); - } - - if (ret >= 0 && excl && excl->ops != exclude) { - pending = i915_sw_fence_await_dma_fence(fence, - excl, - timeout, + dma_resv_for_each_fence_unlocked(resv, &cursor, write, f) { + pending = i915_sw_fence_await_dma_fence(fence, f, timeout, gfp); - if (pending < 0) + if (pending < 0) { ret = pending; - else - ret |= pending; - } - - dma_fence_put(excl); + dma_fence_put(f); + break; + } + ret |= pending; + } return ret; } -- 2.25.1
Simplifying the code a bit. Signed-off-by: Christian König <christian.koenig@amd.com> --- drivers/gpu/drm/i915/i915_request.c | 36 ++++++----------------------- 1 file changed, 7 insertions(+), 29 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c index 37aef1308573..b81045ceb619 100644 --- a/drivers/gpu/drm/i915/i915_request.c +++ b/drivers/gpu/drm/i915/i915_request.c @@ -1583,38 +1583,16 @@ i915_request_await_object(struct i915_request *to, struct drm_i915_gem_object *obj, bool write) { - struct dma_fence *excl; + struct dma_resv_cursor cursor; + struct dma_fence *fence; int ret = 0; - if (write) { - struct dma_fence **shared; - unsigned int count, i; - - ret = dma_resv_get_fences(obj->base.resv, &excl, &count, - &shared); - if (ret) - return ret; - - for (i = 0; i < count; i++) { - ret = i915_request_await_dma_fence(to, shared[i]); - if (ret) - break; - - dma_fence_put(shared[i]); + dma_resv_for_each_fence_unlocked(obj->base.resv, &cursor, write, fence) { + ret = i915_request_await_dma_fence(to, fence); + if (ret) { + dma_fence_put(fence); + break; } - - for (; i < count; i++) - dma_fence_put(shared[i]); - kfree(shared); - } else { - excl = dma_resv_get_excl_unlocked(obj->base.resv); - } - - if (excl) { - if (ret == 0) - ret = i915_request_await_dma_fence(to, excl); - - dma_fence_put(excl); } return ret; -- 2.25.1
Simplifying the code a bit. Signed-off-by: Christian König <christian.koenig@amd.com> --- drivers/gpu/drm/i915/i915_request.c | 36 ++++++----------------------- 1 file changed, 7 insertions(+), 29 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c index 37aef1308573..b81045ceb619 100644 --- a/drivers/gpu/drm/i915/i915_request.c +++ b/drivers/gpu/drm/i915/i915_request.c @@ -1583,38 +1583,16 @@ i915_request_await_object(struct i915_request *to, struct drm_i915_gem_object *obj, bool write) { - struct dma_fence *excl; + struct dma_resv_cursor cursor; + struct dma_fence *fence; int ret = 0; - if (write) { - struct dma_fence **shared; - unsigned int count, i; - - ret = dma_resv_get_fences(obj->base.resv, &excl, &count, - &shared); - if (ret) - return ret; - - for (i = 0; i < count; i++) { - ret = i915_request_await_dma_fence(to, shared[i]); - if (ret) - break; - - dma_fence_put(shared[i]); + dma_resv_for_each_fence_unlocked(obj->base.resv, &cursor, write, fence) { + ret = i915_request_await_dma_fence(to, fence); + if (ret) { + dma_fence_put(fence); + break; } - - for (; i < count; i++) - dma_fence_put(shared[i]); - kfree(shared); - } else { - excl = dma_resv_get_excl_unlocked(obj->base.resv); - } - - if (excl) { - if (ret == 0) - ret = i915_request_await_dma_fence(to, excl); - - dma_fence_put(excl); } return ret; -- 2.25.1
Simplifying the code a bit. Signed-off-by: Christian König <christian.koenig@amd.com> --- drivers/gpu/drm/i915/gem/i915_gem_wait.c | 49 +++++------------------- 1 file changed, 9 insertions(+), 40 deletions(-) diff --git a/drivers/gpu/drm/i915/gem/i915_gem_wait.c b/drivers/gpu/drm/i915/gem/i915_gem_wait.c index 1e97520c62b2..13174541f6c8 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_wait.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_wait.c @@ -37,55 +37,24 @@ i915_gem_object_wait_reservation(struct dma_resv *resv, unsigned int flags, long timeout) { - struct dma_fence *excl; - bool prune_fences = false; + struct dma_resv_cursor cursor; + struct dma_fence *fence; - if (flags & I915_WAIT_ALL) { - struct dma_fence **shared; - unsigned int count, i; - int ret; + dma_resv_for_each_fence_unlocked(resv, &cursor, flags & I915_WAIT_ALL, + fence) { - ret = dma_resv_get_fences(resv, &excl, &count, &shared); - if (ret) - return ret; - - for (i = 0; i < count; i++) { - timeout = i915_gem_object_wait_fence(shared[i], - flags, timeout); - if (timeout < 0) - break; - - dma_fence_put(shared[i]); + timeout = i915_gem_object_wait_fence(fence, flags, timeout); + if (timeout < 0) { + dma_fence_put(fence); + break; } - - for (; i < count; i++) - dma_fence_put(shared[i]); - kfree(shared); - - /* - * If both shared fences and an exclusive fence exist, - * then by construction the shared fences must be later - * than the exclusive fence. If we successfully wait for - * all the shared fences, we know that the exclusive fence - * must all be signaled. If all the shared fences are - * signaled, we can prune the array and recover the - * floating references on the fences/requests. - */ - prune_fences = count && timeout >= 0; - } else { - excl = dma_resv_get_excl_unlocked(resv); } - if (excl && timeout >= 0) - timeout = i915_gem_object_wait_fence(excl, flags, timeout); - - dma_fence_put(excl); - /* * Opportunistically prune the fences iff we know they have *all* been * signaled. */ - if (prune_fences) + if (timeout > 0) dma_resv_prune(resv); return timeout; -- 2.25.1
Simplifying the code a bit. Signed-off-by: Christian König <christian.koenig@amd.com> --- drivers/gpu/drm/i915/gem/i915_gem_wait.c | 49 +++++------------------- 1 file changed, 9 insertions(+), 40 deletions(-) diff --git a/drivers/gpu/drm/i915/gem/i915_gem_wait.c b/drivers/gpu/drm/i915/gem/i915_gem_wait.c index 1e97520c62b2..13174541f6c8 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_wait.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_wait.c @@ -37,55 +37,24 @@ i915_gem_object_wait_reservation(struct dma_resv *resv, unsigned int flags, long timeout) { - struct dma_fence *excl; - bool prune_fences = false; + struct dma_resv_cursor cursor; + struct dma_fence *fence; - if (flags & I915_WAIT_ALL) { - struct dma_fence **shared; - unsigned int count, i; - int ret; + dma_resv_for_each_fence_unlocked(resv, &cursor, flags & I915_WAIT_ALL, + fence) { - ret = dma_resv_get_fences(resv, &excl, &count, &shared); - if (ret) - return ret; - - for (i = 0; i < count; i++) { - timeout = i915_gem_object_wait_fence(shared[i], - flags, timeout); - if (timeout < 0) - break; - - dma_fence_put(shared[i]); + timeout = i915_gem_object_wait_fence(fence, flags, timeout); + if (timeout < 0) { + dma_fence_put(fence); + break; } - - for (; i < count; i++) - dma_fence_put(shared[i]); - kfree(shared); - - /* - * If both shared fences and an exclusive fence exist, - * then by construction the shared fences must be later - * than the exclusive fence. If we successfully wait for - * all the shared fences, we know that the exclusive fence - * must all be signaled. If all the shared fences are - * signaled, we can prune the array and recover the - * floating references on the fences/requests. - */ - prune_fences = count && timeout >= 0; - } else { - excl = dma_resv_get_excl_unlocked(resv); } - if (excl && timeout >= 0) - timeout = i915_gem_object_wait_fence(excl, flags, timeout); - - dma_fence_put(excl); - /* * Opportunistically prune the fences iff we know they have *all* been * signaled. */ - if (prune_fences) + if (timeout > 0) dma_resv_prune(resv); return timeout; -- 2.25.1
Simplifying the code a bit. Signed-off-by: Christian König <christian.koenig@amd.com> --- drivers/gpu/drm/i915/gem/i915_gem_wait.c | 29 ++++-------------------- 1 file changed, 5 insertions(+), 24 deletions(-) diff --git a/drivers/gpu/drm/i915/gem/i915_gem_wait.c b/drivers/gpu/drm/i915/gem/i915_gem_wait.c index 13174541f6c8..e2173a55e527 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_wait.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_wait.c @@ -120,31 +120,12 @@ i915_gem_object_wait_priority(struct drm_i915_gem_object *obj, unsigned int flags, const struct i915_sched_attr *attr) { - struct dma_fence *excl; - - if (flags & I915_WAIT_ALL) { - struct dma_fence **shared; - unsigned int count, i; - int ret; - - ret = dma_resv_get_fences(obj->base.resv, &excl, &count, - &shared); - if (ret) - return ret; - - for (i = 0; i < count; i++) { - i915_gem_fence_wait_priority(shared[i], attr); - dma_fence_put(shared[i]); - } - - kfree(shared); - } else { - excl = dma_resv_get_excl_unlocked(obj->base.resv); - } + struct dma_resv_cursor cursor; + struct dma_fence *fence; - if (excl) { - i915_gem_fence_wait_priority(excl, attr); - dma_fence_put(excl); + dma_resv_for_each_fence_unlocked(obj->base.resv, &cursor, + flags & I915_WAIT_ALL, fence) { + i915_gem_fence_wait_priority(fence, attr); } return 0; } -- 2.25.1
Simplifying the code a bit. Signed-off-by: Christian König <christian.koenig@amd.com> --- drivers/gpu/drm/i915/gem/i915_gem_wait.c | 29 ++++-------------------- 1 file changed, 5 insertions(+), 24 deletions(-) diff --git a/drivers/gpu/drm/i915/gem/i915_gem_wait.c b/drivers/gpu/drm/i915/gem/i915_gem_wait.c index 13174541f6c8..e2173a55e527 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_wait.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_wait.c @@ -120,31 +120,12 @@ i915_gem_object_wait_priority(struct drm_i915_gem_object *obj, unsigned int flags, const struct i915_sched_attr *attr) { - struct dma_fence *excl; - - if (flags & I915_WAIT_ALL) { - struct dma_fence **shared; - unsigned int count, i; - int ret; - - ret = dma_resv_get_fences(obj->base.resv, &excl, &count, - &shared); - if (ret) - return ret; - - for (i = 0; i < count; i++) { - i915_gem_fence_wait_priority(shared[i], attr); - dma_fence_put(shared[i]); - } - - kfree(shared); - } else { - excl = dma_resv_get_excl_unlocked(obj->base.resv); - } + struct dma_resv_cursor cursor; + struct dma_fence *fence; - if (excl) { - i915_gem_fence_wait_priority(excl, attr); - dma_fence_put(excl); + dma_resv_for_each_fence_unlocked(obj->base.resv, &cursor, + flags & I915_WAIT_ALL, fence) { + i915_gem_fence_wait_priority(fence, attr); } return 0; } -- 2.25.1
This is maybe even a fix since the RCU usage here looks incorrect. Signed-off-by: Christian König <christian.koenig@amd.com> --- drivers/gpu/drm/i915/gem/i915_gem_object.h | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.h b/drivers/gpu/drm/i915/gem/i915_gem_object.h index e9eecebf5c9d..3343922af4d6 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_object.h +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.h @@ -500,16 +500,15 @@ static inline struct intel_engine_cs * i915_gem_object_last_write_engine(struct drm_i915_gem_object *obj) { struct intel_engine_cs *engine = NULL; + struct dma_resv_cursor cursor; struct dma_fence *fence; - rcu_read_lock(); - fence = dma_resv_get_excl_unlocked(obj->base.resv); - rcu_read_unlock(); - - if (fence && dma_fence_is_i915(fence) && !dma_fence_is_signaled(fence)) - engine = to_request(fence)->engine; - dma_fence_put(fence); - + dma_resv_for_each_fence_unlocked(obj->base.resv, &cursor, false, + fence) { + if (fence && dma_fence_is_i915(fence) && + !dma_fence_is_signaled(fence)) + engine = to_request(fence)->engine; + } return engine; } -- 2.25.1
This is maybe even a fix since the RCU usage here looks incorrect. Signed-off-by: Christian König <christian.koenig@amd.com> --- drivers/gpu/drm/i915/gem/i915_gem_object.h | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.h b/drivers/gpu/drm/i915/gem/i915_gem_object.h index e9eecebf5c9d..3343922af4d6 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_object.h +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.h @@ -500,16 +500,15 @@ static inline struct intel_engine_cs * i915_gem_object_last_write_engine(struct drm_i915_gem_object *obj) { struct intel_engine_cs *engine = NULL; + struct dma_resv_cursor cursor; struct dma_fence *fence; - rcu_read_lock(); - fence = dma_resv_get_excl_unlocked(obj->base.resv); - rcu_read_unlock(); - - if (fence && dma_fence_is_i915(fence) && !dma_fence_is_signaled(fence)) - engine = to_request(fence)->engine; - dma_fence_put(fence); - + dma_resv_for_each_fence_unlocked(obj->base.resv, &cursor, false, + fence) { + if (fence && dma_fence_is_i915(fence) && + !dma_fence_is_signaled(fence)) + engine = to_request(fence)->engine; + } return engine; } -- 2.25.1
Simplifying the code a bit. Signed-off-by: Christian König <christian.koenig@amd.com> --- drivers/gpu/drm/i915/display/intel_display.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c index eec6c9e9cda7..11c38e0f7fe0 100644 --- a/drivers/gpu/drm/i915/display/intel_display.c +++ b/drivers/gpu/drm/i915/display/intel_display.c @@ -11133,6 +11133,7 @@ intel_prepare_plane_fb(struct drm_plane *_plane, i915_gem_object_flush_frontbuffer(obj, ORIGIN_DIRTYFB); if (!new_plane_state->uapi.fence) { /* implicit fencing */ + struct dma_resv_cursor cursor; struct dma_fence *fence; ret = i915_sw_fence_await_reservation(&state->commit_ready, @@ -11143,11 +11144,10 @@ intel_prepare_plane_fb(struct drm_plane *_plane, if (ret < 0) goto unpin_fb; - fence = dma_resv_get_excl_unlocked(obj->base.resv); - if (fence) { + dma_resv_for_each_fence_unlocked(obj->base.resv, &cursor, false, + fence) { add_rps_boost_after_vblank(new_plane_state->hw.crtc, fence); - dma_fence_put(fence); } } else { add_rps_boost_after_vblank(new_plane_state->hw.crtc, -- 2.25.1
Simplifying the code a bit. Signed-off-by: Christian König <christian.koenig@amd.com> --- drivers/gpu/drm/i915/display/intel_display.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c index eec6c9e9cda7..11c38e0f7fe0 100644 --- a/drivers/gpu/drm/i915/display/intel_display.c +++ b/drivers/gpu/drm/i915/display/intel_display.c @@ -11133,6 +11133,7 @@ intel_prepare_plane_fb(struct drm_plane *_plane, i915_gem_object_flush_frontbuffer(obj, ORIGIN_DIRTYFB); if (!new_plane_state->uapi.fence) { /* implicit fencing */ + struct dma_resv_cursor cursor; struct dma_fence *fence; ret = i915_sw_fence_await_reservation(&state->commit_ready, @@ -11143,11 +11144,10 @@ intel_prepare_plane_fb(struct drm_plane *_plane, if (ret < 0) goto unpin_fb; - fence = dma_resv_get_excl_unlocked(obj->base.resv); - if (fence) { + dma_resv_for_each_fence_unlocked(obj->base.resv, &cursor, false, + fence) { add_rps_boost_after_vblank(new_plane_state->hw.crtc, fence); - dma_fence_put(fence); } } else { add_rps_boost_after_vblank(new_plane_state->hw.crtc, -- 2.25.1
Simplifying the code a bit. Signed-off-by: Christian König <christian.koenig@amd.com> --- drivers/gpu/drm/drm_gem.c | 30 ++++++++---------------------- 1 file changed, 8 insertions(+), 22 deletions(-) diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c index 09c820045859..6e3b8491be68 100644 --- a/drivers/gpu/drm/drm_gem.c +++ b/drivers/gpu/drm/drm_gem.c @@ -1340,31 +1340,17 @@ int drm_gem_fence_array_add_implicit(struct xarray *fence_array, struct drm_gem_object *obj, bool write) { - int ret; - struct dma_fence **fences; - unsigned int i, fence_count; - - if (!write) { - struct dma_fence *fence = - dma_resv_get_excl_unlocked(obj->resv); - - return drm_gem_fence_array_add(fence_array, fence); - } - - ret = dma_resv_get_fences(obj->resv, NULL, - &fence_count, &fences); - if (ret || !fence_count) - return ret; + struct dma_resv_cursor cursor; + struct dma_fence *fence; + int ret = 0; - for (i = 0; i < fence_count; i++) { - ret = drm_gem_fence_array_add(fence_array, fences[i]); - if (ret) + dma_resv_for_each_fence_unlocked(obj->resv, &cursor, write, fence) { + ret = drm_gem_fence_array_add(fence_array, fence); + if (ret) { + dma_fence_put(fence); break; + } } - - for (; i < fence_count; i++) - dma_fence_put(fences[i]); - kfree(fences); return ret; } EXPORT_SYMBOL(drm_gem_fence_array_add_implicit); -- 2.25.1
Simplifying the code a bit. Signed-off-by: Christian König <christian.koenig@amd.com> --- drivers/gpu/drm/drm_gem.c | 30 ++++++++---------------------- 1 file changed, 8 insertions(+), 22 deletions(-) diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c index 09c820045859..6e3b8491be68 100644 --- a/drivers/gpu/drm/drm_gem.c +++ b/drivers/gpu/drm/drm_gem.c @@ -1340,31 +1340,17 @@ int drm_gem_fence_array_add_implicit(struct xarray *fence_array, struct drm_gem_object *obj, bool write) { - int ret; - struct dma_fence **fences; - unsigned int i, fence_count; - - if (!write) { - struct dma_fence *fence = - dma_resv_get_excl_unlocked(obj->resv); - - return drm_gem_fence_array_add(fence_array, fence); - } - - ret = dma_resv_get_fences(obj->resv, NULL, - &fence_count, &fences); - if (ret || !fence_count) - return ret; + struct dma_resv_cursor cursor; + struct dma_fence *fence; + int ret = 0; - for (i = 0; i < fence_count; i++) { - ret = drm_gem_fence_array_add(fence_array, fences[i]); - if (ret) + dma_resv_for_each_fence_unlocked(obj->resv, &cursor, write, fence) { + ret = drm_gem_fence_array_add(fence_array, fence); + if (ret) { + dma_fence_put(fence); break; + } } - - for (; i < fence_count; i++) - dma_fence_put(fences[i]); - kfree(fences); return ret; } EXPORT_SYMBOL(drm_gem_fence_array_add_implicit); -- 2.25.1
Makes the handling a bit more complex, but avoids the use of dma_resv_get_excl_unlocked(). Signed-off-by: Christian König <christian.koenig@amd.com> --- drivers/gpu/drm/drm_gem_atomic_helper.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/drm_gem_atomic_helper.c b/drivers/gpu/drm/drm_gem_atomic_helper.c index e570398abd78..9f51fef82644 100644 --- a/drivers/gpu/drm/drm_gem_atomic_helper.c +++ b/drivers/gpu/drm/drm_gem_atomic_helper.c @@ -143,6 +143,7 @@ */ int drm_gem_plane_helper_prepare_fb(struct drm_plane *plane, struct drm_plane_state *state) { + struct dma_resv_cursor cursor; struct drm_gem_object *obj; struct dma_fence *fence; @@ -150,9 +151,13 @@ int drm_gem_plane_helper_prepare_fb(struct drm_plane *plane, struct drm_plane_st return 0; obj = drm_gem_fb_get_obj(state->fb, 0); - fence = dma_resv_get_excl_unlocked(obj->resv); - drm_atomic_set_fence_for_plane(state, fence); + dma_resv_for_each_fence_unlocked(obj->resv, &cursor, false, fence) { + /* TODO: We only use the first write fence here */ + drm_atomic_set_fence_for_plane(state, fence); + return 0; + } + drm_atomic_set_fence_for_plane(state, NULL); return 0; } EXPORT_SYMBOL_GPL(drm_gem_plane_helper_prepare_fb); -- 2.25.1
Makes the handling a bit more complex, but avoids the use of dma_resv_get_excl_unlocked(). Signed-off-by: Christian König <christian.koenig@amd.com> --- drivers/gpu/drm/drm_gem_atomic_helper.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/drm_gem_atomic_helper.c b/drivers/gpu/drm/drm_gem_atomic_helper.c index e570398abd78..9f51fef82644 100644 --- a/drivers/gpu/drm/drm_gem_atomic_helper.c +++ b/drivers/gpu/drm/drm_gem_atomic_helper.c @@ -143,6 +143,7 @@ */ int drm_gem_plane_helper_prepare_fb(struct drm_plane *plane, struct drm_plane_state *state) { + struct dma_resv_cursor cursor; struct drm_gem_object *obj; struct dma_fence *fence; @@ -150,9 +151,13 @@ int drm_gem_plane_helper_prepare_fb(struct drm_plane *plane, struct drm_plane_st return 0; obj = drm_gem_fb_get_obj(state->fb, 0); - fence = dma_resv_get_excl_unlocked(obj->resv); - drm_atomic_set_fence_for_plane(state, fence); + dma_resv_for_each_fence_unlocked(obj->resv, &cursor, false, fence) { + /* TODO: We only use the first write fence here */ + drm_atomic_set_fence_for_plane(state, fence); + return 0; + } + drm_atomic_set_fence_for_plane(state, NULL); return 0; } EXPORT_SYMBOL_GPL(drm_gem_plane_helper_prepare_fb); -- 2.25.1
Simplifying the code a bit. Signed-off-by: Christian König <christian.koenig@amd.com> --- drivers/gpu/drm/nouveau/nouveau_fence.c | 48 +++++++------------------ 1 file changed, 12 insertions(+), 36 deletions(-) diff --git a/drivers/gpu/drm/nouveau/nouveau_fence.c b/drivers/gpu/drm/nouveau/nouveau_fence.c index 05d0b3eb3690..dc8d7ca1e239 100644 --- a/drivers/gpu/drm/nouveau/nouveau_fence.c +++ b/drivers/gpu/drm/nouveau/nouveau_fence.c @@ -339,14 +339,15 @@ nouveau_fence_wait(struct nouveau_fence *fence, bool lazy, bool intr) } int -nouveau_fence_sync(struct nouveau_bo *nvbo, struct nouveau_channel *chan, bool exclusive, bool intr) +nouveau_fence_sync(struct nouveau_bo *nvbo, struct nouveau_channel *chan, + bool exclusive, bool intr) { struct nouveau_fence_chan *fctx = chan->fence; - struct dma_fence *fence; struct dma_resv *resv = nvbo->bo.base.resv; - struct dma_resv_list *fobj; + struct dma_resv_cursor cursor; + struct dma_fence *fence; struct nouveau_fence *f; - int ret = 0, i; + int ret; if (!exclusive) { ret = dma_resv_reserve_shared(resv, 1); @@ -355,10 +356,7 @@ nouveau_fence_sync(struct nouveau_bo *nvbo, struct nouveau_channel *chan, bool e return ret; } - fobj = dma_resv_shared_list(resv); - fence = dma_resv_excl_fence(resv); - - if (fence) { + dma_resv_for_each_fence(resv, &cursor, exclusive, fence) { struct nouveau_channel *prev = NULL; bool must_wait = true; @@ -366,41 +364,19 @@ nouveau_fence_sync(struct nouveau_bo *nvbo, struct nouveau_channel *chan, bool e if (f) { rcu_read_lock(); prev = rcu_dereference(f->channel); - if (prev && (prev == chan || fctx->sync(f, prev, chan) == 0)) + if (prev && (prev == chan || + fctx->sync(f, prev, chan) == 0)) must_wait = false; rcu_read_unlock(); } - if (must_wait) + if (must_wait) { ret = dma_fence_wait(fence, intr); - - return ret; - } - - if (!exclusive || !fobj) - return ret; - - for (i = 0; i < fobj->shared_count && !ret; ++i) { - struct nouveau_channel *prev = NULL; - bool must_wait = true; - - fence = rcu_dereference_protected(fobj->shared[i], - dma_resv_held(resv)); - - f = nouveau_local_fence(fence, chan->drm); - if (f) { - rcu_read_lock(); - prev = rcu_dereference(f->channel); - if (prev && (prev == chan || fctx->sync(f, prev, chan) == 0)) - must_wait = false; - rcu_read_unlock(); + if (ret) + return ret; } - - if (must_wait) - ret = dma_fence_wait(fence, intr); } - - return ret; + return 0; } void -- 2.25.1
Simplifying the code a bit. Signed-off-by: Christian König <christian.koenig@amd.com> --- drivers/gpu/drm/nouveau/nouveau_fence.c | 48 +++++++------------------ 1 file changed, 12 insertions(+), 36 deletions(-) diff --git a/drivers/gpu/drm/nouveau/nouveau_fence.c b/drivers/gpu/drm/nouveau/nouveau_fence.c index 05d0b3eb3690..dc8d7ca1e239 100644 --- a/drivers/gpu/drm/nouveau/nouveau_fence.c +++ b/drivers/gpu/drm/nouveau/nouveau_fence.c @@ -339,14 +339,15 @@ nouveau_fence_wait(struct nouveau_fence *fence, bool lazy, bool intr) } int -nouveau_fence_sync(struct nouveau_bo *nvbo, struct nouveau_channel *chan, bool exclusive, bool intr) +nouveau_fence_sync(struct nouveau_bo *nvbo, struct nouveau_channel *chan, + bool exclusive, bool intr) { struct nouveau_fence_chan *fctx = chan->fence; - struct dma_fence *fence; struct dma_resv *resv = nvbo->bo.base.resv; - struct dma_resv_list *fobj; + struct dma_resv_cursor cursor; + struct dma_fence *fence; struct nouveau_fence *f; - int ret = 0, i; + int ret; if (!exclusive) { ret = dma_resv_reserve_shared(resv, 1); @@ -355,10 +356,7 @@ nouveau_fence_sync(struct nouveau_bo *nvbo, struct nouveau_channel *chan, bool e return ret; } - fobj = dma_resv_shared_list(resv); - fence = dma_resv_excl_fence(resv); - - if (fence) { + dma_resv_for_each_fence(resv, &cursor, exclusive, fence) { struct nouveau_channel *prev = NULL; bool must_wait = true; @@ -366,41 +364,19 @@ nouveau_fence_sync(struct nouveau_bo *nvbo, struct nouveau_channel *chan, bool e if (f) { rcu_read_lock(); prev = rcu_dereference(f->channel); - if (prev && (prev == chan || fctx->sync(f, prev, chan) == 0)) + if (prev && (prev == chan || + fctx->sync(f, prev, chan) == 0)) must_wait = false; rcu_read_unlock(); } - if (must_wait) + if (must_wait) { ret = dma_fence_wait(fence, intr); - - return ret; - } - - if (!exclusive || !fobj) - return ret; - - for (i = 0; i < fobj->shared_count && !ret; ++i) { - struct nouveau_channel *prev = NULL; - bool must_wait = true; - - fence = rcu_dereference_protected(fobj->shared[i], - dma_resv_held(resv)); - - f = nouveau_local_fence(fence, chan->drm); - if (f) { - rcu_read_lock(); - prev = rcu_dereference(f->channel); - if (prev && (prev == chan || fctx->sync(f, prev, chan) == 0)) - must_wait = false; - rcu_read_unlock(); + if (ret) + return ret; } - - if (must_wait) - ret = dma_fence_wait(fence, intr); } - - return ret; + return 0; } void -- 2.25.1
Makes the handling a bit more complex, but avoids the use of dma_resv_get_excl_unlocked(). Signed-off-by: Christian König <christian.koenig@amd.com> --- drivers/gpu/drm/nouveau/dispnv50/wndw.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/nouveau/dispnv50/wndw.c b/drivers/gpu/drm/nouveau/dispnv50/wndw.c index 8d048bacd6f0..9a0c42d38a47 100644 --- a/drivers/gpu/drm/nouveau/dispnv50/wndw.c +++ b/drivers/gpu/drm/nouveau/dispnv50/wndw.c @@ -539,6 +539,8 @@ 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_cursor cursor; + struct dma_fence *fence; int ret; NV_ATOMIC(drm, "%s prepare: %p\n", plane->name, fb); @@ -561,7 +563,12 @@ nv50_wndw_prepare_fb(struct drm_plane *plane, struct drm_plane_state *state) asyw->image.handle[0] = ctxdma->object.handle; } - asyw->state.fence = dma_resv_get_excl_unlocked(nvbo->bo.base.resv); + dma_resv_for_each_fence_unlocked(nvbo->bo.base.resv, &cursor, false, + fence) { + /* TODO: We only use the first writer here */ + asyw->state.fence = fence; + break; + } asyw->image.offset[0] = nvbo->offset; if (wndw->func->prepare) { -- 2.25.1
Makes the handling a bit more complex, but avoids the use of dma_resv_get_excl_unlocked(). Signed-off-by: Christian König <christian.koenig@amd.com> --- drivers/gpu/drm/nouveau/dispnv50/wndw.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/nouveau/dispnv50/wndw.c b/drivers/gpu/drm/nouveau/dispnv50/wndw.c index 8d048bacd6f0..9a0c42d38a47 100644 --- a/drivers/gpu/drm/nouveau/dispnv50/wndw.c +++ b/drivers/gpu/drm/nouveau/dispnv50/wndw.c @@ -539,6 +539,8 @@ 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_cursor cursor; + struct dma_fence *fence; int ret; NV_ATOMIC(drm, "%s prepare: %p\n", plane->name, fb); @@ -561,7 +563,12 @@ nv50_wndw_prepare_fb(struct drm_plane *plane, struct drm_plane_state *state) asyw->image.handle[0] = ctxdma->object.handle; } - asyw->state.fence = dma_resv_get_excl_unlocked(nvbo->bo.base.resv); + dma_resv_for_each_fence_unlocked(nvbo->bo.base.resv, &cursor, false, + fence) { + /* TODO: We only use the first writer here */ + asyw->state.fence = fence; + break; + } asyw->image.offset[0] = nvbo->offset; if (wndw->func->prepare) { -- 2.25.1
Instead of hand rolling the logic. Signed-off-by: Christian König <christian.koenig@amd.com> --- drivers/gpu/drm/etnaviv/etnaviv_gem.c | 27 +++++++++------------------ 1 file changed, 9 insertions(+), 18 deletions(-) diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.c b/drivers/gpu/drm/etnaviv/etnaviv_gem.c index b8fa6ed3dd73..6808dbef5c79 100644 --- a/drivers/gpu/drm/etnaviv/etnaviv_gem.c +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.c @@ -437,19 +437,17 @@ int etnaviv_gem_wait_bo(struct etnaviv_gpu *gpu, struct drm_gem_object *obj, static void etnaviv_gem_describe_fence(struct dma_fence *fence, const char *type, struct seq_file *m) { - if (!test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags)) - seq_printf(m, "\t%9s: %s %s seq %llu\n", - type, - fence->ops->get_driver_name(fence), - fence->ops->get_timeline_name(fence), - fence->seqno); + seq_printf(m, "\t%9s: %s %s seq %llu\n", type, + fence->ops->get_driver_name(fence), + fence->ops->get_timeline_name(fence), + fence->seqno); } static void etnaviv_gem_describe(struct drm_gem_object *obj, struct seq_file *m) { struct etnaviv_gem_object *etnaviv_obj = to_etnaviv_bo(obj); struct dma_resv *robj = obj->resv; - struct dma_resv_list *fobj; + struct dma_resv_cursor cursor; struct dma_fence *fence; unsigned long off = drm_vma_node_start(&obj->vma_node); @@ -459,19 +457,12 @@ static void etnaviv_gem_describe(struct drm_gem_object *obj, struct seq_file *m) off, etnaviv_obj->vaddr, obj->size); rcu_read_lock(); - fobj = dma_resv_shared_list(robj); - if (fobj) { - unsigned int i, shared_count = fobj->shared_count; - - for (i = 0; i < shared_count; i++) { - fence = rcu_dereference(fobj->shared[i]); + dma_resv_for_each_fence_unlocked(robj, &cursor, true, fence) { + if (cursor.is_exclusive) + etnaviv_gem_describe_fence(fence, "Exclusive", m); + else etnaviv_gem_describe_fence(fence, "Shared", m); - } } - - fence = dma_resv_excl_fence(robj); - if (fence) - etnaviv_gem_describe_fence(fence, "Exclusive", m); rcu_read_unlock(); } -- 2.25.1
Instead of hand rolling the logic. Signed-off-by: Christian König <christian.koenig@amd.com> --- drivers/gpu/drm/etnaviv/etnaviv_gem.c | 27 +++++++++------------------ 1 file changed, 9 insertions(+), 18 deletions(-) diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.c b/drivers/gpu/drm/etnaviv/etnaviv_gem.c index b8fa6ed3dd73..6808dbef5c79 100644 --- a/drivers/gpu/drm/etnaviv/etnaviv_gem.c +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.c @@ -437,19 +437,17 @@ int etnaviv_gem_wait_bo(struct etnaviv_gpu *gpu, struct drm_gem_object *obj, static void etnaviv_gem_describe_fence(struct dma_fence *fence, const char *type, struct seq_file *m) { - if (!test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags)) - seq_printf(m, "\t%9s: %s %s seq %llu\n", - type, - fence->ops->get_driver_name(fence), - fence->ops->get_timeline_name(fence), - fence->seqno); + seq_printf(m, "\t%9s: %s %s seq %llu\n", type, + fence->ops->get_driver_name(fence), + fence->ops->get_timeline_name(fence), + fence->seqno); } static void etnaviv_gem_describe(struct drm_gem_object *obj, struct seq_file *m) { struct etnaviv_gem_object *etnaviv_obj = to_etnaviv_bo(obj); struct dma_resv *robj = obj->resv; - struct dma_resv_list *fobj; + struct dma_resv_cursor cursor; struct dma_fence *fence; unsigned long off = drm_vma_node_start(&obj->vma_node); @@ -459,19 +457,12 @@ static void etnaviv_gem_describe(struct drm_gem_object *obj, struct seq_file *m) off, etnaviv_obj->vaddr, obj->size); rcu_read_lock(); - fobj = dma_resv_shared_list(robj); - if (fobj) { - unsigned int i, shared_count = fobj->shared_count; - - for (i = 0; i < shared_count; i++) { - fence = rcu_dereference(fobj->shared[i]); + dma_resv_for_each_fence_unlocked(robj, &cursor, true, fence) { + if (cursor.is_exclusive) + etnaviv_gem_describe_fence(fence, "Exclusive", m); + else etnaviv_gem_describe_fence(fence, "Shared", m); - } } - - fence = dma_resv_excl_fence(robj); - if (fence) - etnaviv_gem_describe_fence(fence, "Exclusive", m); rcu_read_unlock(); } -- 2.25.1
We certainly hold the reservation lock here, no need for the RCU dance. Signed-off-by: Christian König <christian.koenig@amd.com> --- drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c index 4dd7d9d541c0..7e17bc2b5df1 100644 --- a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c @@ -195,7 +195,7 @@ static int submit_fence_sync(struct etnaviv_gem_submit *submit) if (ret) return ret; } else { - bo->excl = dma_resv_get_excl_unlocked(robj); + bo->excl = dma_fence_get(dma_resv_excl_fence(robj)); } } -- 2.25.1
We certainly hold the reservation lock here, no need for the RCU dance. Signed-off-by: Christian König <christian.koenig@amd.com> --- drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c index 4dd7d9d541c0..7e17bc2b5df1 100644 --- a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c @@ -195,7 +195,7 @@ static int submit_fence_sync(struct etnaviv_gem_submit *submit) if (ret) return ret; } else { - bo->excl = dma_resv_get_excl_unlocked(robj); + bo->excl = dma_fence_get(dma_resv_excl_fence(robj)); } } -- 2.25.1
Heureka, that's finally not used any more. Signed-off-by: Christian König <christian.koenig@amd.com> --- include/linux/dma-resv.h | 26 -------------------------- 1 file changed, 26 deletions(-) diff --git a/include/linux/dma-resv.h b/include/linux/dma-resv.h index 6f9bb7e4c538..90c15cbe7d92 100644 --- a/include/linux/dma-resv.h +++ b/include/linux/dma-resv.h @@ -370,32 +370,6 @@ dma_resv_excl_fence(struct dma_resv *obj) return rcu_dereference_check(obj->fence_excl, dma_resv_held(obj)); } -/** - * dma_resv_get_excl_unlocked - get the reservation object's - * exclusive fence, without lock held. - * @obj: the reservation object - * - * If there is an exclusive fence, this atomically increments it's - * reference count and returns it. - * - * RETURNS - * The exclusive fence or NULL if none - */ -static inline struct dma_fence * -dma_resv_get_excl_unlocked(struct dma_resv *obj) -{ - struct dma_fence *fence; - - if (!rcu_access_pointer(obj->fence_excl)) - return NULL; - - rcu_read_lock(); - fence = dma_fence_get_rcu_safe(&obj->fence_excl); - rcu_read_unlock(); - - return fence; -} - /** * dma_resv_shared_list - get the reservation object's shared fence list * @obj: the reservation object -- 2.25.1
Heureka, that's finally not used any more. Signed-off-by: Christian König <christian.koenig@amd.com> --- include/linux/dma-resv.h | 26 -------------------------- 1 file changed, 26 deletions(-) diff --git a/include/linux/dma-resv.h b/include/linux/dma-resv.h index 6f9bb7e4c538..90c15cbe7d92 100644 --- a/include/linux/dma-resv.h +++ b/include/linux/dma-resv.h @@ -370,32 +370,6 @@ dma_resv_excl_fence(struct dma_resv *obj) return rcu_dereference_check(obj->fence_excl, dma_resv_held(obj)); } -/** - * dma_resv_get_excl_unlocked - get the reservation object's - * exclusive fence, without lock held. - * @obj: the reservation object - * - * If there is an exclusive fence, this atomically increments it's - * reference count and returns it. - * - * RETURNS - * The exclusive fence or NULL if none - */ -static inline struct dma_fence * -dma_resv_get_excl_unlocked(struct dma_resv *obj) -{ - struct dma_fence *fence; - - if (!rcu_access_pointer(obj->fence_excl)) - return NULL; - - rcu_read_lock(); - fence = dma_fence_get_rcu_safe(&obj->fence_excl); - rcu_read_unlock(); - - return fence; -} - /** * dma_resv_shared_list - get the reservation object's shared fence list * @obj: the reservation object -- 2.25.1
== Series Details == Series: series starting with [01/26] dma-buf: add dma_resv_for_each_fence_unlocked URL : https://patchwork.freedesktop.org/series/94605/ State : warning == Summary == $ dim checkpatch origin/drm-tip a5a4809c51c3 dma-buf: add dma_resv_for_each_fence_unlocked -:69: CHECK:OPEN_ENDED_LINE: Lines should not end with a '[' #69: FILE: drivers/dma-buf/dma-resv.c:369: + fence = rcu_dereference(fences->shared[ -:128: CHECK:MACRO_ARG_REUSE: Macro argument reuse 'obj' - possible side-effects? #128: FILE: include/linux/dma-resv.h:180: +#define dma_resv_for_each_fence_unlocked(obj, cursor, all_fences, fence) \ + for (fence = dma_resv_walk_unlocked(obj, cursor, all_fences, true); \ + fence; dma_fence_put(fence), \ + fence = dma_resv_walk_unlocked(obj, cursor, all_fences, false)) -:128: CHECK:MACRO_ARG_REUSE: Macro argument reuse 'cursor' - possible side-effects? #128: FILE: include/linux/dma-resv.h:180: +#define dma_resv_for_each_fence_unlocked(obj, cursor, all_fences, fence) \ + for (fence = dma_resv_walk_unlocked(obj, cursor, all_fences, true); \ + fence; dma_fence_put(fence), \ + fence = dma_resv_walk_unlocked(obj, cursor, all_fences, false)) -:128: CHECK:MACRO_ARG_REUSE: Macro argument reuse 'all_fences' - possible side-effects? #128: FILE: include/linux/dma-resv.h:180: +#define dma_resv_for_each_fence_unlocked(obj, cursor, all_fences, fence) \ + for (fence = dma_resv_walk_unlocked(obj, cursor, all_fences, true); \ + fence; dma_fence_put(fence), \ + fence = dma_resv_walk_unlocked(obj, cursor, all_fences, false)) -:128: CHECK:MACRO_ARG_REUSE: Macro argument reuse 'fence' - possible side-effects? #128: FILE: include/linux/dma-resv.h:180: +#define dma_resv_for_each_fence_unlocked(obj, cursor, all_fences, fence) \ + for (fence = dma_resv_walk_unlocked(obj, cursor, all_fences, true); \ + fence; dma_fence_put(fence), \ + fence = dma_resv_walk_unlocked(obj, cursor, all_fences, false)) -:145: WARNING:FROM_SIGN_OFF_MISMATCH: From:/Signed-off-by: email address mismatch: 'From: "Christian König" <ckoenig.leichtzumerken@gmail.com>' != 'Signed-off-by: Christian König <christian.koenig@amd.com>' total: 0 errors, 1 warnings, 5 checks, 117 lines checked d86af6324ecd dma-buf: add dma_resv_for_each_fence -:82: CHECK:MACRO_ARG_REUSE: Macro argument reuse 'obj' - possible side-effects? #82: FILE: include/linux/dma-resv.h:179: +#define dma_resv_for_each_fence(obj, cursor, all_fences, fence) \ + for (fence = dma_resv_walk(obj, cursor, all_fences, true); fence; \ + fence = dma_resv_walk(obj, cursor, all_fences, false)) -:82: CHECK:MACRO_ARG_REUSE: Macro argument reuse 'cursor' - possible side-effects? #82: FILE: include/linux/dma-resv.h:179: +#define dma_resv_for_each_fence(obj, cursor, all_fences, fence) \ + for (fence = dma_resv_walk(obj, cursor, all_fences, true); fence; \ + fence = dma_resv_walk(obj, cursor, all_fences, false)) -:82: CHECK:MACRO_ARG_REUSE: Macro argument reuse 'all_fences' - possible side-effects? #82: FILE: include/linux/dma-resv.h:179: +#define dma_resv_for_each_fence(obj, cursor, all_fences, fence) \ + for (fence = dma_resv_walk(obj, cursor, all_fences, true); fence; \ + fence = dma_resv_walk(obj, cursor, all_fences, false)) -:82: CHECK:MACRO_ARG_REUSE: Macro argument reuse 'fence' - possible side-effects? #82: FILE: include/linux/dma-resv.h:179: +#define dma_resv_for_each_fence(obj, cursor, all_fences, fence) \ + for (fence = dma_resv_walk(obj, cursor, all_fences, true); fence; \ + fence = dma_resv_walk(obj, cursor, all_fences, false)) -:98: WARNING:FROM_SIGN_OFF_MISMATCH: From:/Signed-off-by: email address mismatch: 'From: "Christian König" <ckoenig.leichtzumerken@gmail.com>' != 'Signed-off-by: Christian König <christian.koenig@amd.com>' total: 0 errors, 1 warnings, 4 checks, 74 lines checked a09933d567e8 dma-buf: use new iterator in dma_resv_copy_fences -:124: WARNING:FROM_SIGN_OFF_MISMATCH: From:/Signed-off-by: email address mismatch: 'From: "Christian König" <ckoenig.leichtzumerken@gmail.com>' != 'Signed-off-by: Christian König <christian.koenig@amd.com>' total: 0 errors, 1 warnings, 0 checks, 106 lines checked 52d4d01515dd dma-buf: use new iterator in dma_resv_get_fences v2 -:121: ERROR:POINTER_LOCATION: "(foo*)" should be "(foo *)" #121: FILE: drivers/dma-buf/dma-resv.c:535: + sizeof(void*), -:156: WARNING:FROM_SIGN_OFF_MISMATCH: From:/Signed-off-by: email address mismatch: 'From: "Christian König" <ckoenig.leichtzumerken@gmail.com>' != 'Signed-off-by: Christian König <christian.koenig@amd.com>' total: 1 errors, 1 warnings, 0 checks, 136 lines checked 9ac03efbd4df dma-buf: use new iterator in dma_resv_wait_timeout -:99: WARNING:FROM_SIGN_OFF_MISMATCH: From:/Signed-off-by: email address mismatch: 'From: "Christian König" <ckoenig.leichtzumerken@gmail.com>' != 'Signed-off-by: Christian König <christian.koenig@amd.com>' total: 0 errors, 1 warnings, 0 checks, 81 lines checked b3c5b34567df dma-buf: use new iterator in dma_resv_test_signaled -:91: WARNING:FROM_SIGN_OFF_MISMATCH: From:/Signed-off-by: email address mismatch: 'From: "Christian König" <ckoenig.leichtzumerken@gmail.com>' != 'Signed-off-by: Christian König <christian.koenig@amd.com>' total: 0 errors, 1 warnings, 0 checks, 72 lines checked 73f737059793 drm/ttm: use the new iterator in ttm_bo_flush_all_fences -:39: WARNING:FROM_SIGN_OFF_MISMATCH: From:/Signed-off-by: email address mismatch: 'From: "Christian König" <ckoenig.leichtzumerken@gmail.com>' != 'Signed-off-by: Christian König <christian.koenig@amd.com>' total: 0 errors, 1 warnings, 0 checks, 21 lines checked dffe8854038d drm/amdgpu: use the new iterator in amdgpu_sync_resv -:72: WARNING:FROM_SIGN_OFF_MISMATCH: From:/Signed-off-by: email address mismatch: 'From: "Christian König" <ckoenig.leichtzumerken@gmail.com>' != 'Signed-off-by: Christian König <christian.koenig@amd.com>' total: 0 errors, 1 warnings, 0 checks, 55 lines checked bb6008db8ce0 drm/amdgpu: use new iterator in amdgpu_ttm_bo_eviction_valuable -:47: WARNING:FROM_SIGN_OFF_MISMATCH: From:/Signed-off-by: email address mismatch: 'From: "Christian König" <ckoenig.leichtzumerken@gmail.com>' != 'Signed-off-by: Christian König <christian.koenig@amd.com>' total: 0 errors, 1 warnings, 0 checks, 28 lines checked 5d1ec21b3b8f drm/msm: use new iterator in msm_gem_describe -:55: WARNING:FROM_SIGN_OFF_MISMATCH: From:/Signed-off-by: email address mismatch: 'From: "Christian König" <ckoenig.leichtzumerken@gmail.com>' != 'Signed-off-by: Christian König <christian.koenig@amd.com>' total: 0 errors, 1 warnings, 0 checks, 34 lines checked b2a3b96527a5 drm/radeon: use new iterator in radeon_sync_resv -:53: WARNING:FROM_SIGN_OFF_MISMATCH: From:/Signed-off-by: email address mismatch: 'From: "Christian König" <ckoenig.leichtzumerken@gmail.com>' != 'Signed-off-by: Christian König <christian.koenig@amd.com>' total: 0 errors, 1 warnings, 0 checks, 36 lines checked a88d8558e2ca drm/scheduler: use new iterator in drm_sched_job_add_implicit_dependencies -:54: WARNING:FROM_SIGN_OFF_MISMATCH: From:/Signed-off-by: email address mismatch: 'From: "Christian König" <ckoenig.leichtzumerken@gmail.com>' != 'Signed-off-by: Christian König <christian.koenig@amd.com>' total: 0 errors, 1 warnings, 0 checks, 36 lines checked f952954785f3 drm/i915: use the new iterator in i915_gem_busy_ioctl -:64: WARNING:FROM_SIGN_OFF_MISMATCH: From:/Signed-off-by: email address mismatch: 'From: "Christian König" <ckoenig.leichtzumerken@gmail.com>' != 'Signed-off-by: Christian König <christian.koenig@amd.com>' total: 0 errors, 1 warnings, 0 checks, 45 lines checked 542393a55f28 drm/i915: use the new iterator in i915_sw_fence_await_reservation -:84: WARNING:FROM_SIGN_OFF_MISMATCH: From:/Signed-off-by: email address mismatch: 'From: "Christian König" <ckoenig.leichtzumerken@gmail.com>' != 'Signed-off-by: Christian König <christian.koenig@amd.com>' total: 0 errors, 1 warnings, 0 checks, 66 lines checked ff87ab40927f drm/i915: use the new iterator in i915_request_await_object -:62: WARNING:FROM_SIGN_OFF_MISMATCH: From:/Signed-off-by: email address mismatch: 'From: "Christian König" <ckoenig.leichtzumerken@gmail.com>' != 'Signed-off-by: Christian König <christian.koenig@amd.com>' total: 0 errors, 1 warnings, 0 checks, 45 lines checked dc06dd9d1fa1 drm/i915: use new iterator in i915_gem_object_wait_reservation -:82: WARNING:FROM_SIGN_OFF_MISMATCH: From:/Signed-off-by: email address mismatch: 'From: "Christian König" <ckoenig.leichtzumerken@gmail.com>' != 'Signed-off-by: Christian König <christian.koenig@amd.com>' total: 0 errors, 1 warnings, 0 checks, 64 lines checked 201bbdd7fcc7 drm/i915: use new iterator in i915_gem_object_wait_priority -:53: WARNING:FROM_SIGN_OFF_MISMATCH: From:/Signed-off-by: email address mismatch: 'From: "Christian König" <ckoenig.leichtzumerken@gmail.com>' != 'Signed-off-by: Christian König <christian.koenig@amd.com>' total: 0 errors, 1 warnings, 0 checks, 36 lines checked 800e9dbca06f drm/i915: use new iterator in i915_gem_object_last_write_engine -:41: WARNING:FROM_SIGN_OFF_MISMATCH: From:/Signed-off-by: email address mismatch: 'From: "Christian König" <ckoenig.leichtzumerken@gmail.com>' != 'Signed-off-by: Christian König <christian.koenig@amd.com>' total: 0 errors, 1 warnings, 0 checks, 23 lines checked b2a96f247826 drm/i915: use new cursor in intel_prepare_plane_fb -:38: WARNING:FROM_SIGN_OFF_MISMATCH: From:/Signed-off-by: email address mismatch: 'From: "Christian König" <ckoenig.leichtzumerken@gmail.com>' != 'Signed-off-by: Christian König <christian.koenig@amd.com>' total: 0 errors, 1 warnings, 0 checks, 20 lines checked 9dafcbeff892 drm: use new iterator in drm_gem_fence_array_add_implicit -:56: WARNING:FROM_SIGN_OFF_MISMATCH: From:/Signed-off-by: email address mismatch: 'From: "Christian König" <ckoenig.leichtzumerken@gmail.com>' != 'Signed-off-by: Christian König <christian.koenig@amd.com>' total: 0 errors, 1 warnings, 0 checks, 39 lines checked 36f4828e60a7 drm: use new iterator in drm_gem_plane_helper_prepare_fb -:41: WARNING:FROM_SIGN_OFF_MISMATCH: From:/Signed-off-by: email address mismatch: 'From: "Christian König" <ckoenig.leichtzumerken@gmail.com>' != 'Signed-off-by: Christian König <christian.koenig@amd.com>' total: 0 errors, 1 warnings, 0 checks, 22 lines checked 145e068ae31e drm/nouveau: use the new iterator in nouveau_fence_sync -:96: WARNING:FROM_SIGN_OFF_MISMATCH: From:/Signed-off-by: email address mismatch: 'From: "Christian König" <ckoenig.leichtzumerken@gmail.com>' != 'Signed-off-by: Christian König <christian.koenig@amd.com>' total: 0 errors, 1 warnings, 0 checks, 77 lines checked d8cbd50515cf drm/nouveau: use the new interator in nv50_wndw_prepare_fb -:40: WARNING:FROM_SIGN_OFF_MISMATCH: From:/Signed-off-by: email address mismatch: 'From: "Christian König" <ckoenig.leichtzumerken@gmail.com>' != 'Signed-off-by: Christian König <christian.koenig@amd.com>' total: 0 errors, 1 warnings, 0 checks, 21 lines checked 22010ad4c08c drm/etnaviv: use new iterator in etnaviv_gem_describe -:65: WARNING:FROM_SIGN_OFF_MISMATCH: From:/Signed-off-by: email address mismatch: 'From: "Christian König" <ckoenig.leichtzumerken@gmail.com>' != 'Signed-off-by: Christian König <christian.koenig@amd.com>' total: 0 errors, 1 warnings, 0 checks, 47 lines checked 2bb60c4ef61e drm/etnaviv: replace dma_resv_get_excl_unlocked -:25: WARNING:FROM_SIGN_OFF_MISMATCH: From:/Signed-off-by: email address mismatch: 'From: "Christian König" <ckoenig.leichtzumerken@gmail.com>' != 'Signed-off-by: Christian König <christian.koenig@amd.com>' total: 0 errors, 1 warnings, 0 checks, 8 lines checked ca57e3cbd5d4 dma-buf: nuke dma_resv_get_excl_unlocked -:49: WARNING:FROM_SIGN_OFF_MISMATCH: From:/Signed-off-by: email address mismatch: 'From: "Christian König" <ckoenig.leichtzumerken@gmail.com>' != 'Signed-off-by: Christian König <christian.koenig@amd.com>' total: 0 errors, 1 warnings, 0 checks, 32 lines checked
[-- Attachment #1: Type: text/plain, Size: 17073 bytes --] == Series Details == Series: series starting with [01/26] dma-buf: add dma_resv_for_each_fence_unlocked URL : https://patchwork.freedesktop.org/series/94605/ State : failure == Summary == CI Bug Log - changes from CI_DRM_10574 -> Patchwork_21021 ==================================================== Summary ------- **FAILURE** Serious unknown changes coming with Patchwork_21021 absolutely need to be verified manually. If you think the reported changes have nothing to do with the changes introduced in Patchwork_21021, please notify your bug team to allow them to document this new failure mode, which will reduce false positives in CI. External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21021/index.html Possible new issues ------------------- Here are the unknown changes that may have been introduced in Patchwork_21021: ### IGT changes ### #### Possible regressions #### * igt@gem_busy@busy@all: - fi-bdw-gvtdvm: [PASS][1] -> [DMESG-WARN][2] +1 similar issue [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10574/fi-bdw-gvtdvm/igt@gem_busy@busy@all.html [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21021/fi-bdw-gvtdvm/igt@gem_busy@busy@all.html - fi-bsw-nick: [PASS][3] -> [DMESG-WARN][4] +1 similar issue [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10574/fi-bsw-nick/igt@gem_busy@busy@all.html [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21021/fi-bsw-nick/igt@gem_busy@busy@all.html - fi-kbl-guc: [PASS][5] -> [DMESG-WARN][6] +1 similar issue [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10574/fi-kbl-guc/igt@gem_busy@busy@all.html [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21021/fi-kbl-guc/igt@gem_busy@busy@all.html * igt@gem_ctx_create@basic-files: - fi-kbl-r: [PASS][7] -> [DMESG-WARN][8] [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10574/fi-kbl-r/igt@gem_ctx_create@basic-files.html [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21021/fi-kbl-r/igt@gem_ctx_create@basic-files.html - fi-kbl-8809g: [PASS][9] -> [DMESG-WARN][10] +1 similar issue [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10574/fi-kbl-8809g/igt@gem_ctx_create@basic-files.html [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21021/fi-kbl-8809g/igt@gem_ctx_create@basic-files.html - fi-cml-u2: [PASS][11] -> [DMESG-WARN][12] [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10574/fi-cml-u2/igt@gem_ctx_create@basic-files.html [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21021/fi-cml-u2/igt@gem_ctx_create@basic-files.html - fi-rkl-guc: [PASS][13] -> [DMESG-WARN][14] [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10574/fi-rkl-guc/igt@gem_ctx_create@basic-files.html [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21021/fi-rkl-guc/igt@gem_ctx_create@basic-files.html - fi-tgl-1115g4: [PASS][15] -> [DMESG-WARN][16] [15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10574/fi-tgl-1115g4/igt@gem_ctx_create@basic-files.html [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21021/fi-tgl-1115g4/igt@gem_ctx_create@basic-files.html - fi-bxt-dsi: [PASS][17] -> [DMESG-WARN][18] [17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10574/fi-bxt-dsi/igt@gem_ctx_create@basic-files.html [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21021/fi-bxt-dsi/igt@gem_ctx_create@basic-files.html - fi-hsw-4770: [PASS][19] -> [DMESG-WARN][20] [19]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10574/fi-hsw-4770/igt@gem_ctx_create@basic-files.html [20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21021/fi-hsw-4770/igt@gem_ctx_create@basic-files.html - fi-cfl-guc: [PASS][21] -> [DMESG-WARN][22] [21]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10574/fi-cfl-guc/igt@gem_ctx_create@basic-files.html [22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21021/fi-cfl-guc/igt@gem_ctx_create@basic-files.html - fi-snb-2600: [PASS][23] -> [DMESG-WARN][24] [23]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10574/fi-snb-2600/igt@gem_ctx_create@basic-files.html [24]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21021/fi-snb-2600/igt@gem_ctx_create@basic-files.html - fi-skl-guc: [PASS][25] -> [DMESG-WARN][26] [25]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10574/fi-skl-guc/igt@gem_ctx_create@basic-files.html [26]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21021/fi-skl-guc/igt@gem_ctx_create@basic-files.html - fi-kbl-7567u: [PASS][27] -> [DMESG-WARN][28] +1 similar issue [27]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10574/fi-kbl-7567u/igt@gem_ctx_create@basic-files.html [28]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21021/fi-kbl-7567u/igt@gem_ctx_create@basic-files.html - fi-rkl-11600: [PASS][29] -> [DMESG-WARN][30] [29]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10574/fi-rkl-11600/igt@gem_ctx_create@basic-files.html [30]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21021/fi-rkl-11600/igt@gem_ctx_create@basic-files.html - fi-ilk-650: [PASS][31] -> [DMESG-WARN][32] [31]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10574/fi-ilk-650/igt@gem_ctx_create@basic-files.html [32]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21021/fi-ilk-650/igt@gem_ctx_create@basic-files.html - fi-ivb-3770: [PASS][33] -> [DMESG-WARN][34] [33]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10574/fi-ivb-3770/igt@gem_ctx_create@basic-files.html [34]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21021/fi-ivb-3770/igt@gem_ctx_create@basic-files.html - fi-icl-y: [PASS][35] -> [DMESG-WARN][36] [35]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10574/fi-icl-y/igt@gem_ctx_create@basic-files.html [36]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21021/fi-icl-y/igt@gem_ctx_create@basic-files.html - fi-cfl-8700k: [PASS][37] -> [DMESG-WARN][38] [37]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10574/fi-cfl-8700k/igt@gem_ctx_create@basic-files.html [38]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21021/fi-cfl-8700k/igt@gem_ctx_create@basic-files.html - fi-icl-u2: [PASS][39] -> [DMESG-WARN][40] [39]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10574/fi-icl-u2/igt@gem_ctx_create@basic-files.html [40]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21021/fi-icl-u2/igt@gem_ctx_create@basic-files.html - fi-skl-6700k2: [PASS][41] -> [DMESG-WARN][42] [41]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10574/fi-skl-6700k2/igt@gem_ctx_create@basic-files.html [42]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21021/fi-skl-6700k2/igt@gem_ctx_create@basic-files.html - fi-elk-e7500: [PASS][43] -> [DMESG-WARN][44] [43]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10574/fi-elk-e7500/igt@gem_ctx_create@basic-files.html [44]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21021/fi-elk-e7500/igt@gem_ctx_create@basic-files.html - fi-glk-dsi: [PASS][45] -> [DMESG-WARN][46] [45]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10574/fi-glk-dsi/igt@gem_ctx_create@basic-files.html [46]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21021/fi-glk-dsi/igt@gem_ctx_create@basic-files.html - fi-apl-guc: [PASS][47] -> [DMESG-WARN][48] [47]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10574/fi-apl-guc/igt@gem_ctx_create@basic-files.html [48]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21021/fi-apl-guc/igt@gem_ctx_create@basic-files.html - fi-snb-2520m: [PASS][49] -> [DMESG-WARN][50] [49]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10574/fi-snb-2520m/igt@gem_ctx_create@basic-files.html [50]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21021/fi-snb-2520m/igt@gem_ctx_create@basic-files.html - fi-bsw-kefka: [PASS][51] -> [DMESG-WARN][52] [51]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10574/fi-bsw-kefka/igt@gem_ctx_create@basic-files.html [52]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21021/fi-bsw-kefka/igt@gem_ctx_create@basic-files.html - fi-tgl-y: [PASS][53] -> [DMESG-WARN][54] [53]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10574/fi-tgl-y/igt@gem_ctx_create@basic-files.html [54]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21021/fi-tgl-y/igt@gem_ctx_create@basic-files.html - fi-bwr-2160: [PASS][55] -> [DMESG-WARN][56] [55]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10574/fi-bwr-2160/igt@gem_ctx_create@basic-files.html [56]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21021/fi-bwr-2160/igt@gem_ctx_create@basic-files.html - fi-skl-6600u: [PASS][57] -> [DMESG-WARN][58] [57]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10574/fi-skl-6600u/igt@gem_ctx_create@basic-files.html [58]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21021/fi-skl-6600u/igt@gem_ctx_create@basic-files.html - fi-bdw-5557u: [PASS][59] -> [DMESG-WARN][60] [59]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10574/fi-bdw-5557u/igt@gem_ctx_create@basic-files.html [60]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21021/fi-bdw-5557u/igt@gem_ctx_create@basic-files.html - fi-kbl-soraka: [PASS][61] -> [DMESG-WARN][62] [61]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10574/fi-kbl-soraka/igt@gem_ctx_create@basic-files.html [62]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21021/fi-kbl-soraka/igt@gem_ctx_create@basic-files.html - fi-cfl-8109u: [PASS][63] -> [DMESG-WARN][64] [63]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10574/fi-cfl-8109u/igt@gem_ctx_create@basic-files.html [64]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21021/fi-cfl-8109u/igt@gem_ctx_create@basic-files.html * igt@gem_exec_basic@basic@rcs0-smem: - fi-pnv-d510: [PASS][65] -> [DMESG-WARN][66] [65]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10574/fi-pnv-d510/igt@gem_exec_basic@basic@rcs0-smem.html [66]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21021/fi-pnv-d510/igt@gem_exec_basic@basic@rcs0-smem.html #### Suppressed #### The following results come from untrusted machines, tests, or statuses. They do not affect the overall result. * igt@gem_busy@busy@all: - {fi-hsw-gt1}: [PASS][67] -> [DMESG-WARN][68] +1 similar issue [67]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10574/fi-hsw-gt1/igt@gem_busy@busy@all.html [68]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21021/fi-hsw-gt1/igt@gem_busy@busy@all.html * igt@gem_ctx_create@basic-files: - {fi-jsl-1}: [PASS][69] -> [DMESG-WARN][70] [69]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10574/fi-jsl-1/igt@gem_ctx_create@basic-files.html [70]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21021/fi-jsl-1/igt@gem_ctx_create@basic-files.html - {fi-ehl-2}: [PASS][71] -> [DMESG-WARN][72] [71]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10574/fi-ehl-2/igt@gem_ctx_create@basic-files.html [72]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21021/fi-ehl-2/igt@gem_ctx_create@basic-files.html - {fi-tgl-dsi}: [PASS][73] -> [DMESG-WARN][74] [73]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10574/fi-tgl-dsi/igt@gem_ctx_create@basic-files.html [74]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21021/fi-tgl-dsi/igt@gem_ctx_create@basic-files.html Known issues ------------ Here are the changes found in Patchwork_21021 that come from known issues: ### IGT changes ### #### Issues hit #### * igt@amdgpu/amd_basic@semaphore: - fi-bdw-5557u: NOTRUN -> [SKIP][75] ([fdo#109271]) +27 similar issues [75]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21021/fi-bdw-5557u/igt@amdgpu/amd_basic@semaphore.html * igt@core_hotunplug@unbind-rebind: - fi-bdw-5557u: NOTRUN -> [WARN][76] ([i915#3718]) [76]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21021/fi-bdw-5557u/igt@core_hotunplug@unbind-rebind.html * igt@i915_selftest@live@hangcheck: - fi-snb-2600: [PASS][77] -> [INCOMPLETE][78] ([i915#3921]) [77]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10574/fi-snb-2600/igt@i915_selftest@live@hangcheck.html [78]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21021/fi-snb-2600/igt@i915_selftest@live@hangcheck.html * igt@kms_chamelium@dp-crc-fast: - fi-bdw-5557u: NOTRUN -> [SKIP][79] ([fdo#109271] / [fdo#111827]) +8 similar issues [79]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21021/fi-bdw-5557u/igt@kms_chamelium@dp-crc-fast.html #### Possible fixes #### * igt@i915_module_load@reload: - fi-tgl-y: [DMESG-WARN][80] ([i915#1982]) -> [PASS][81] [80]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10574/fi-tgl-y/igt@i915_module_load@reload.html [81]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21021/fi-tgl-y/igt@i915_module_load@reload.html * igt@i915_selftest@live@hangcheck: - {fi-hsw-gt1}: [DMESG-WARN][82] ([i915#3303]) -> [PASS][83] [82]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10574/fi-hsw-gt1/igt@i915_selftest@live@hangcheck.html [83]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21021/fi-hsw-gt1/igt@i915_selftest@live@hangcheck.html * igt@kms_force_connector_basic@force-connector-state: - fi-rkl-guc: [FAIL][84] ([i915#3983]) -> [PASS][85] [84]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10574/fi-rkl-guc/igt@kms_force_connector_basic@force-connector-state.html [85]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21021/fi-rkl-guc/igt@kms_force_connector_basic@force-connector-state.html {name}: This element is suppressed. This means it is ignored when computing the status of the difference (SUCCESS, WARNING, or FAILURE). [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271 [fdo#111827]: https://bugs.freedesktop.org/show_bug.cgi?id=111827 [i915#1982]: https://gitlab.freedesktop.org/drm/intel/issues/1982 [i915#2867]: https://gitlab.freedesktop.org/drm/intel/issues/2867 [i915#3303]: https://gitlab.freedesktop.org/drm/intel/issues/3303 [i915#3718]: https://gitlab.freedesktop.org/drm/intel/issues/3718 [i915#3921]: https://gitlab.freedesktop.org/drm/intel/issues/3921 [i915#3983]: https://gitlab.freedesktop.org/drm/intel/issues/3983 Participating hosts (44 -> 38) ------------------------------ Missing (6): bat-dg1-6 bat-dg1-5 fi-bsw-cyan bat-adlp-4 fi-bdw-samus bat-jsl-1 Build changes ------------- * Linux: CI_DRM_10574 -> Patchwork_21021 CI-20190529: 20190529 CI_DRM_10574: 3ad0415f7f7a68058d5ff02d8a7ac51d8b542cf9 @ git://anongit.freedesktop.org/gfx-ci/linux IGT_6204: 1084c5eb74fd0daf8c9b8e83e85f5208c396579b @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git Patchwork_21021: ca57e3cbd5d45521e004de0c8956fc34828b8744 @ git://anongit.freedesktop.org/gfx-ci/linux == Linux commits == ca57e3cbd5d4 dma-buf: nuke dma_resv_get_excl_unlocked 2bb60c4ef61e drm/etnaviv: replace dma_resv_get_excl_unlocked 22010ad4c08c drm/etnaviv: use new iterator in etnaviv_gem_describe d8cbd50515cf drm/nouveau: use the new interator in nv50_wndw_prepare_fb 145e068ae31e drm/nouveau: use the new iterator in nouveau_fence_sync 36f4828e60a7 drm: use new iterator in drm_gem_plane_helper_prepare_fb 9dafcbeff892 drm: use new iterator in drm_gem_fence_array_add_implicit b2a96f247826 drm/i915: use new cursor in intel_prepare_plane_fb 800e9dbca06f drm/i915: use new iterator in i915_gem_object_last_write_engine 201bbdd7fcc7 drm/i915: use new iterator in i915_gem_object_wait_priority dc06dd9d1fa1 drm/i915: use new iterator in i915_gem_object_wait_reservation ff87ab40927f drm/i915: use the new iterator in i915_request_await_object 542393a55f28 drm/i915: use the new iterator in i915_sw_fence_await_reservation f952954785f3 drm/i915: use the new iterator in i915_gem_busy_ioctl a88d8558e2ca drm/scheduler: use new iterator in drm_sched_job_add_implicit_dependencies b2a3b96527a5 drm/radeon: use new iterator in radeon_sync_resv 5d1ec21b3b8f drm/msm: use new iterator in msm_gem_describe bb6008db8ce0 drm/amdgpu: use new iterator in amdgpu_ttm_bo_eviction_valuable dffe8854038d drm/amdgpu: use the new iterator in amdgpu_sync_resv 73f737059793 drm/ttm: use the new iterator in ttm_bo_flush_all_fences b3c5b34567df dma-buf: use new iterator in dma_resv_test_signaled 9ac03efbd4df dma-buf: use new iterator in dma_resv_wait_timeout 52d4d01515dd dma-buf: use new iterator in dma_resv_get_fences v2 a09933d567e8 dma-buf: use new iterator in dma_resv_copy_fences d86af6324ecd dma-buf: add dma_resv_for_each_fence a5a4809c51c3 dma-buf: add dma_resv_for_each_fence_unlocked == Logs == For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21021/index.html [-- Attachment #2: Type: text/html, Size: 18618 bytes --]
On 13/09/2021 14:16, Christian König wrote: > Simplifying the code a bit. > > Signed-off-by: Christian König <christian.koenig@amd.com> > --- > drivers/gpu/drm/i915/i915_request.c | 36 ++++++----------------------- > 1 file changed, 7 insertions(+), 29 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c > index 37aef1308573..b81045ceb619 100644 > --- a/drivers/gpu/drm/i915/i915_request.c > +++ b/drivers/gpu/drm/i915/i915_request.c > @@ -1583,38 +1583,16 @@ i915_request_await_object(struct i915_request *to, > struct drm_i915_gem_object *obj, > bool write) > { > - struct dma_fence *excl; > + struct dma_resv_cursor cursor; > + struct dma_fence *fence; > int ret = 0; > > - if (write) { > - struct dma_fence **shared; > - unsigned int count, i; > - > - ret = dma_resv_get_fences(obj->base.resv, &excl, &count, > - &shared); > - if (ret) > - return ret; > - > - for (i = 0; i < count; i++) { > - ret = i915_request_await_dma_fence(to, shared[i]); > - if (ret) > - break; > - > - dma_fence_put(shared[i]); > + dma_resv_for_each_fence_unlocked(obj->base.resv, &cursor, write, fence) { I think callers have the object locked for this one. At least if you haven't tried it it's worth asking CI (you have the assert already so it will tell you). But I think it's important to have an atomic snapshot here. Regards, Tvrtko > + ret = i915_request_await_dma_fence(to, fence); > + if (ret) { > + dma_fence_put(fence); > + break; > } > - > - for (; i < count; i++) > - dma_fence_put(shared[i]); > - kfree(shared); > - } else { > - excl = dma_resv_get_excl_unlocked(obj->base.resv); > - } > - > - if (excl) { > - if (ret == 0) > - ret = i915_request_await_dma_fence(to, excl); > - > - dma_fence_put(excl); > } > > return ret; >
Am 14.09.21 um 12:26 schrieb Tvrtko Ursulin: > > On 13/09/2021 14:16, Christian König wrote: >> Simplifying the code a bit. >> >> Signed-off-by: Christian König <christian.koenig@amd.com> >> --- >> drivers/gpu/drm/i915/i915_request.c | 36 ++++++----------------------- >> 1 file changed, 7 insertions(+), 29 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_request.c >> b/drivers/gpu/drm/i915/i915_request.c >> index 37aef1308573..b81045ceb619 100644 >> --- a/drivers/gpu/drm/i915/i915_request.c >> +++ b/drivers/gpu/drm/i915/i915_request.c >> @@ -1583,38 +1583,16 @@ i915_request_await_object(struct i915_request >> *to, >> struct drm_i915_gem_object *obj, >> bool write) >> { >> - struct dma_fence *excl; >> + struct dma_resv_cursor cursor; >> + struct dma_fence *fence; >> int ret = 0; >> - if (write) { >> - struct dma_fence **shared; >> - unsigned int count, i; >> - >> - ret = dma_resv_get_fences(obj->base.resv, &excl, &count, >> - &shared); >> - if (ret) >> - return ret; >> - >> - for (i = 0; i < count; i++) { >> - ret = i915_request_await_dma_fence(to, shared[i]); >> - if (ret) >> - break; >> - >> - dma_fence_put(shared[i]); >> + dma_resv_for_each_fence_unlocked(obj->base.resv, &cursor, write, >> fence) { > > I think callers have the object locked for this one. At least if you > haven't tried it it's worth asking CI (you have the assert already so > it will tell you). But I think it's important to have an atomic > snapshot here. Thanks for the info. In this case I'm just going to use the locked variant of the iterator here for the next round. Could you point me to the place where the lock is grabed/released for reference? Thanks, Christian. > > Regards, > > Tvrtko > >> + ret = i915_request_await_dma_fence(to, fence); >> + if (ret) { >> + dma_fence_put(fence); >> + break; >> } >> - >> - for (; i < count; i++) >> - dma_fence_put(shared[i]); >> - kfree(shared); >> - } else { >> - excl = dma_resv_get_excl_unlocked(obj->base.resv); >> - } >> - >> - if (excl) { >> - if (ret == 0) >> - ret = i915_request_await_dma_fence(to, excl); >> - >> - dma_fence_put(excl); >> } >> return ret; >>
On 13/09/2021 14:16, Christian König wrote: > Abstract the complexity of iterating over all the fences > in a dma_resv object. > > The new loop handles the whole RCU and retry dance and > returns only fences where we can be sure we grabbed the > right one. > > Signed-off-by: Christian König <christian.koenig@amd.com> > --- > drivers/dma-buf/dma-resv.c | 63 ++++++++++++++++++++++++++++++++++++++ > include/linux/dma-resv.h | 36 ++++++++++++++++++++++ > 2 files changed, 99 insertions(+) > > diff --git a/drivers/dma-buf/dma-resv.c b/drivers/dma-buf/dma-resv.c > index 84fbe60629e3..213a9b7251ca 100644 > --- a/drivers/dma-buf/dma-resv.c > +++ b/drivers/dma-buf/dma-resv.c > @@ -323,6 +323,69 @@ void dma_resv_add_excl_fence(struct dma_resv *obj, struct dma_fence *fence) > } > EXPORT_SYMBOL(dma_resv_add_excl_fence); > > +/** > + * dma_resv_walk_unlocked - walk over fences in a dma_resv obj > + * @obj: the dma_resv object > + * @cursor: cursor to record the current position > + * @all_fences: true returns also the shared fences > + * @first: if we should start over > + * > + * Return all the fences in the dma_resv object which are not yet signaled. > + * The returned fence has an extra local reference so will stay alive. > + * If a concurrent modify is detected the whole iterator is started over again. > + */ > +struct dma_fence *dma_resv_walk_unlocked(struct dma_resv *obj, > + struct dma_resv_cursor *cursor, > + bool all_fences, bool first) > +{ > + struct dma_fence *fence = NULL; > + > + do { > + /* Drop the reference from the previous round */ > + dma_fence_put(fence); > + > + cursor->is_first = first; > + if (first) { > + cursor->seq = read_seqcount_begin(&obj->seq); > + cursor->index = -1; > + cursor->fences = dma_resv_shared_list(obj); > + cursor->is_exclusive = true; > + > + fence = dma_resv_excl_fence(obj); > + if (fence && test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, > + &fence->flags)) > + fence = NULL; > + } else { > + fence = NULL; > + } > + > + if (fence) { > + fence = dma_fence_get_rcu(fence); > + } else if (all_fences && cursor->fences) { > + struct dma_resv_list *fences = cursor->fences; If rcu lock is allowed to be dropped while walking the list what guarantees list of fences hasn't been freed? Like: 1st call -> gets seqcount -> stores cursor->fences rcu lock dropped/re-acquired 2nd call -> dereferences into cursor->fences -> boom? > + > + cursor->is_exclusive = false; > + while (++cursor->index < fences->shared_count) { > + fence = rcu_dereference(fences->shared[ > + cursor->index]); > + if (!test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, > + &fence->flags)) > + break; > + } > + if (cursor->index < fences->shared_count) > + fence = dma_fence_get_rcu(fence); > + else > + fence = NULL; > + } > + > + /* For the eventually next round */ > + first = true; > + } while (read_seqcount_retry(&obj->seq, cursor->seq)); > + > + return fence; > +} > +EXPORT_SYMBOL_GPL(dma_resv_walk_unlocked); > + > /** > * dma_resv_copy_fences - Copy all fences from src to dst. > * @dst: the destination reservation object > diff --git a/include/linux/dma-resv.h b/include/linux/dma-resv.h > index 9100dd3dc21f..f5b91c292ee0 100644 > --- a/include/linux/dma-resv.h > +++ b/include/linux/dma-resv.h > @@ -149,6 +149,39 @@ struct dma_resv { > struct dma_resv_list __rcu *fence; > }; > > +/** > + * struct dma_resv_cursor - current position into the dma_resv fences > + * @seq: sequence number to check > + * @index: index into the shared fences > + * @shared: the shared fences > + * @is_first: true if this is the first returned fence > + * @is_exclusive: if the current fence is the exclusive one > + */ > +struct dma_resv_cursor { > + unsigned int seq; > + unsigned int index; > + struct dma_resv_list *fences; > + bool is_first; Is_first is useful to callers - like they are legitimately allowed to look inside this, what could otherwise be private object? What is the intended use case, given when true the returned fence can be either exclusive or first from a shared list? > + bool is_exclusive; Is_exclusive could be written as index == -1 in the code, right? If so then an opportunity to remove some redundancy. > +}; > + > +/** > + * dma_resv_for_each_fence_unlocked - fence iterator > + * @obj: a dma_resv object pointer > + * @cursor: a struct dma_resv_cursor pointer > + * @all_fences: true if all fences should be returned > + * @fence: the current fence > + * > + * Iterate over the fences in a struct dma_resv object without holding the > + * dma_resv::lock. The RCU read side lock must be hold when using this, but can > + * be dropped and re-taken as necessary inside the loop. @all_fences controls > + * if the shared fences are returned as well. > + */ > +#define dma_resv_for_each_fence_unlocked(obj, cursor, all_fences, fence) \ > + for (fence = dma_resv_walk_unlocked(obj, cursor, all_fences, true); \ > + fence; dma_fence_put(fence), \ > + fence = dma_resv_walk_unlocked(obj, cursor, all_fences, false)) Has the fact RCU lock can be dropped so there is potential to walk over completely different snapshots been discussed? At least if I followed the code correctly - it appears there is potential the walk restarts from the start (exclusive slot) at any point during the walk. Because theoretically I think you could take an atomic snapshot of everything (given you have a cursor object) and then release it on the end condition. Regards, Tvrtko > + > #define dma_resv_held(obj) lockdep_is_held(&(obj)->lock.base) > #define dma_resv_assert_held(obj) lockdep_assert_held(&(obj)->lock.base) > > @@ -366,6 +399,9 @@ 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_add_excl_fence(struct dma_resv *obj, struct dma_fence *fence); > +struct dma_fence *dma_resv_walk_unlocked(struct dma_resv *obj, > + struct dma_resv_cursor *cursor, > + bool first, bool all_fences); > int dma_resv_get_fences(struct dma_resv *obj, struct dma_fence **pfence_excl, > unsigned *pshared_count, struct dma_fence ***pshared); > int dma_resv_copy_fences(struct dma_resv *dst, struct dma_resv *src); >
On 14/09/2021 11:39, Christian König wrote: > Am 14.09.21 um 12:26 schrieb Tvrtko Ursulin: >> >> On 13/09/2021 14:16, Christian König wrote: >>> Simplifying the code a bit. >>> >>> Signed-off-by: Christian König <christian.koenig@amd.com> >>> --- >>> drivers/gpu/drm/i915/i915_request.c | 36 ++++++----------------------- >>> 1 file changed, 7 insertions(+), 29 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/i915/i915_request.c >>> b/drivers/gpu/drm/i915/i915_request.c >>> index 37aef1308573..b81045ceb619 100644 >>> --- a/drivers/gpu/drm/i915/i915_request.c >>> +++ b/drivers/gpu/drm/i915/i915_request.c >>> @@ -1583,38 +1583,16 @@ i915_request_await_object(struct i915_request >>> *to, >>> struct drm_i915_gem_object *obj, >>> bool write) >>> { >>> - struct dma_fence *excl; >>> + struct dma_resv_cursor cursor; >>> + struct dma_fence *fence; >>> int ret = 0; >>> - if (write) { >>> - struct dma_fence **shared; >>> - unsigned int count, i; >>> - >>> - ret = dma_resv_get_fences(obj->base.resv, &excl, &count, >>> - &shared); >>> - if (ret) >>> - return ret; >>> - >>> - for (i = 0; i < count; i++) { >>> - ret = i915_request_await_dma_fence(to, shared[i]); >>> - if (ret) >>> - break; >>> - >>> - dma_fence_put(shared[i]); >>> + dma_resv_for_each_fence_unlocked(obj->base.resv, &cursor, write, >>> fence) { >> >> I think callers have the object locked for this one. At least if you >> haven't tried it it's worth asking CI (you have the assert already so >> it will tell you). But I think it's important to have an atomic >> snapshot here. > > Thanks for the info. In this case I'm just going to use the locked > variant of the iterator here for the next round. > > Could you point me to the place where the lock is grabed/released for > reference? There is quite a few callers and I haven't audited all of them. But I think, given the function is used for setting up tracking of implicit dependencies, that it has to be true. In the case of execbuf for instance the flow is relatively complicated: i915_gem_do_execbuffer eb_relocate_parse eb_validate_vmas eb_lock_vmas i915_gem_object_lock eb_submit eb_move_to_gpu i915_request_await_object i915_gem_ww_ctx_fini i915_gem_ww_ctx_unlock_all i915_gem_object_unlock Other call sites have simpler flows but there is a lot of them so I think using CI is easiest. Regards, Tvrtko > Thanks, > Christian. > >> >> Regards, >> >> Tvrtko >> >>> + ret = i915_request_await_dma_fence(to, fence); >>> + if (ret) { >>> + dma_fence_put(fence); >>> + break; >>> } >>> - >>> - for (; i < count; i++) >>> - dma_fence_put(shared[i]); >>> - kfree(shared); >>> - } else { >>> - excl = dma_resv_get_excl_unlocked(obj->base.resv); >>> - } >>> - >>> - if (excl) { >>> - if (ret == 0) >>> - ret = i915_request_await_dma_fence(to, excl); >>> - >>> - dma_fence_put(excl); >>> } >>> return ret; >>> >
Am 14.09.21 um 12:53 schrieb Tvrtko Ursulin: > > On 13/09/2021 14:16, Christian König wrote: >> Abstract the complexity of iterating over all the fences >> in a dma_resv object. >> >> The new loop handles the whole RCU and retry dance and >> returns only fences where we can be sure we grabbed the >> right one. >> >> Signed-off-by: Christian König <christian.koenig@amd.com> >> --- >> drivers/dma-buf/dma-resv.c | 63 ++++++++++++++++++++++++++++++++++++++ >> include/linux/dma-resv.h | 36 ++++++++++++++++++++++ >> 2 files changed, 99 insertions(+) >> >> diff --git a/drivers/dma-buf/dma-resv.c b/drivers/dma-buf/dma-resv.c >> index 84fbe60629e3..213a9b7251ca 100644 >> --- a/drivers/dma-buf/dma-resv.c >> +++ b/drivers/dma-buf/dma-resv.c >> @@ -323,6 +323,69 @@ void dma_resv_add_excl_fence(struct dma_resv >> *obj, struct dma_fence *fence) >> } >> EXPORT_SYMBOL(dma_resv_add_excl_fence); >> +/** >> + * dma_resv_walk_unlocked - walk over fences in a dma_resv obj >> + * @obj: the dma_resv object >> + * @cursor: cursor to record the current position >> + * @all_fences: true returns also the shared fences >> + * @first: if we should start over >> + * >> + * Return all the fences in the dma_resv object which are not yet >> signaled. >> + * The returned fence has an extra local reference so will stay alive. >> + * If a concurrent modify is detected the whole iterator is started >> over again. >> + */ >> +struct dma_fence *dma_resv_walk_unlocked(struct dma_resv *obj, >> + struct dma_resv_cursor *cursor, >> + bool all_fences, bool first) >> +{ >> + struct dma_fence *fence = NULL; >> + >> + do { >> + /* Drop the reference from the previous round */ >> + dma_fence_put(fence); >> + >> + cursor->is_first = first; >> + if (first) { >> + cursor->seq = read_seqcount_begin(&obj->seq); >> + cursor->index = -1; >> + cursor->fences = dma_resv_shared_list(obj); >> + cursor->is_exclusive = true; >> + >> + fence = dma_resv_excl_fence(obj); >> + if (fence && test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, >> + &fence->flags)) >> + fence = NULL; >> + } else { >> + fence = NULL; >> + } >> + >> + if (fence) { >> + fence = dma_fence_get_rcu(fence); >> + } else if (all_fences && cursor->fences) { >> + struct dma_resv_list *fences = cursor->fences; > > If rcu lock is allowed to be dropped while walking the list what > guarantees list of fences hasn't been freed? Ah, good point! We need to test the sequence number when we enter the function as well. Going to fix that. > > Like: > > 1st call > -> gets seqcount > -> stores cursor->fences > > rcu lock dropped/re-acquired > > 2nd call > -> dereferences into cursor->fences -> boom? > >> + >> + cursor->is_exclusive = false; >> + while (++cursor->index < fences->shared_count) { >> + fence = rcu_dereference(fences->shared[ >> + cursor->index]); >> + if (!test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, >> + &fence->flags)) >> + break; >> + } >> + if (cursor->index < fences->shared_count) >> + fence = dma_fence_get_rcu(fence); >> + else >> + fence = NULL; >> + } >> + >> + /* For the eventually next round */ >> + first = true; >> + } while (read_seqcount_retry(&obj->seq, cursor->seq)); >> + >> + return fence; >> +} >> +EXPORT_SYMBOL_GPL(dma_resv_walk_unlocked); >> + >> /** >> * dma_resv_copy_fences - Copy all fences from src to dst. >> * @dst: the destination reservation object >> diff --git a/include/linux/dma-resv.h b/include/linux/dma-resv.h >> index 9100dd3dc21f..f5b91c292ee0 100644 >> --- a/include/linux/dma-resv.h >> +++ b/include/linux/dma-resv.h >> @@ -149,6 +149,39 @@ struct dma_resv { >> struct dma_resv_list __rcu *fence; >> }; >> +/** >> + * struct dma_resv_cursor - current position into the dma_resv fences >> + * @seq: sequence number to check >> + * @index: index into the shared fences >> + * @shared: the shared fences >> + * @is_first: true if this is the first returned fence >> + * @is_exclusive: if the current fence is the exclusive one >> + */ >> +struct dma_resv_cursor { >> + unsigned int seq; >> + unsigned int index; >> + struct dma_resv_list *fences; >> + bool is_first; > > Is_first is useful to callers - like they are legitimately allowed to > look inside this, what could otherwise be private object? Yes, I was pondering on the same question. Key point is that this is only used by other dma_resv functions which also use cursor.fences for example. So this is only supposed to be used by code working with other privates of the dma_resv object as well. Maybe adding that as a comment? > What is the intended use case, given when true the returned fence can > be either exclusive or first from a shared list? To reset counters for and know that the sequence has restarted. See how this is used in dma_resv_copy_fences() for example: rcu_read_lock(); dma_resv_for_each_fence_unlocked(dst, &cursor, true, f) { if (cursor.is_first) { dma_resv_list_free(list); dma_fence_put(excl); if (cursor.fences) { unsigned int cnt = cursor.fences->shared_count; rcu_read_unlock(); list = dma_resv_list_alloc(cnt); ... > >> + bool is_exclusive; > > Is_exclusive could be written as index == -1 in the code, right? If so > then an opportunity to remove some redundancy. Correct and good point. But I want to move away a bit from the exclusive/shared notation in the future and avoid that tests like index == -1 spread around in the code. What we could do is to add some helper like dma_resv_cursor_is_excludive(cursor) which tests that. > >> +}; >> + >> +/** >> + * dma_resv_for_each_fence_unlocked - fence iterator >> + * @obj: a dma_resv object pointer >> + * @cursor: a struct dma_resv_cursor pointer >> + * @all_fences: true if all fences should be returned >> + * @fence: the current fence >> + * >> + * Iterate over the fences in a struct dma_resv object without >> holding the >> + * dma_resv::lock. The RCU read side lock must be hold when using >> this, but can >> + * be dropped and re-taken as necessary inside the loop. @all_fences >> controls >> + * if the shared fences are returned as well. >> + */ >> +#define dma_resv_for_each_fence_unlocked(obj, cursor, all_fences, >> fence) \ >> + for (fence = dma_resv_walk_unlocked(obj, cursor, all_fences, >> true); \ >> + fence; dma_fence_put(fence), \ >> + fence = dma_resv_walk_unlocked(obj, cursor, all_fences, >> false)) > > Has the fact RCU lock can be dropped so there is potential to walk > over completely different snapshots been discussed? Well that's basically the heart of the functionality. Even without dropping the RCU lock there can be an restart at any time when the dma_resv object is modified. > At least if I followed the code correctly - it appears there is > potential the walk restarts from the start (exclusive slot) at any > point during the walk. Correct, yes. > Because theoretically I think you could take an atomic snapshot of > everything (given you have a cursor object) and then release it on the > end condition. That's what the dma_resv_get_fences() function is good for, yes. This one returns an array of fences. The key difference is that we need to allocate memory for that which is at least sometimes not feasible or desired. Thanks for the review, Christian. > > Regards, > > Tvrtko > >> + >> #define dma_resv_held(obj) lockdep_is_held(&(obj)->lock.base) >> #define dma_resv_assert_held(obj) >> lockdep_assert_held(&(obj)->lock.base) >> @@ -366,6 +399,9 @@ 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_add_excl_fence(struct dma_resv *obj, struct dma_fence >> *fence); >> +struct dma_fence *dma_resv_walk_unlocked(struct dma_resv *obj, >> + struct dma_resv_cursor *cursor, >> + bool first, bool all_fences); >> int dma_resv_get_fences(struct dma_resv *obj, struct dma_fence >> **pfence_excl, >> unsigned *pshared_count, struct dma_fence ***pshared); >> int dma_resv_copy_fences(struct dma_resv *dst, struct dma_resv *src); >>
On 13/09/2021 14:16, Christian König wrote: > This is maybe even a fix since the RCU usage here looks incorrect. What you think is incorrect? Pointless extra rcu locking? Also, FWIW, I submitted a patch to remove this function altogether since its IMO pretty useless, just failed in getting anyone to ack it so far. Regards, Tvrtko > Signed-off-by: Christian König <christian.koenig@amd.com> > --- > drivers/gpu/drm/i915/gem/i915_gem_object.h | 15 +++++++-------- > 1 file changed, 7 insertions(+), 8 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.h b/drivers/gpu/drm/i915/gem/i915_gem_object.h > index e9eecebf5c9d..3343922af4d6 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_object.h > +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.h > @@ -500,16 +500,15 @@ static inline struct intel_engine_cs * > i915_gem_object_last_write_engine(struct drm_i915_gem_object *obj) > { > struct intel_engine_cs *engine = NULL; > + struct dma_resv_cursor cursor; > struct dma_fence *fence; > > - rcu_read_lock(); > - fence = dma_resv_get_excl_unlocked(obj->base.resv); > - rcu_read_unlock(); > - > - if (fence && dma_fence_is_i915(fence) && !dma_fence_is_signaled(fence)) > - engine = to_request(fence)->engine; > - dma_fence_put(fence); > - > + dma_resv_for_each_fence_unlocked(obj->base.resv, &cursor, false, > + fence) { > + if (fence && dma_fence_is_i915(fence) && > + !dma_fence_is_signaled(fence)) > + engine = to_request(fence)->engine; > + } > return engine; > } > >
Am 14.09.21 um 14:27 schrieb Tvrtko Ursulin: > > On 13/09/2021 14:16, Christian König wrote: >> This is maybe even a fix since the RCU usage here looks incorrect. > > What you think is incorrect? Pointless extra rcu locking? Yeah, exactly that. I also wondered for a second if rcu_read_lock() can nest or not. But obviously it either works or lockdep hasn't complained yet. But I've made a mistake here and at a couple of other places to remove to many rcu_read_lock() calls. Thanks for pointing that out, going to fix it as well. > Also, FWIW, I submitted a patch to remove this function altogether > since its IMO pretty useless, just failed in getting anyone to ack it > so far. I was on the edge of suggesting that as well since it's only debugfs usage looked quite pointless to me. Feel free to CC me on the patch and you can have my acked-by. Thanks, Christian. > > Regards, > > Tvrtko > >> Signed-off-by: Christian König <christian.koenig@amd.com> >> --- >> drivers/gpu/drm/i915/gem/i915_gem_object.h | 15 +++++++-------- >> 1 file changed, 7 insertions(+), 8 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.h >> b/drivers/gpu/drm/i915/gem/i915_gem_object.h >> index e9eecebf5c9d..3343922af4d6 100644 >> --- a/drivers/gpu/drm/i915/gem/i915_gem_object.h >> +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.h >> @@ -500,16 +500,15 @@ static inline struct intel_engine_cs * >> i915_gem_object_last_write_engine(struct drm_i915_gem_object *obj) >> { >> struct intel_engine_cs *engine = NULL; >> + struct dma_resv_cursor cursor; >> struct dma_fence *fence; >> - rcu_read_lock(); >> - fence = dma_resv_get_excl_unlocked(obj->base.resv); >> - rcu_read_unlock(); >> - >> - if (fence && dma_fence_is_i915(fence) && >> !dma_fence_is_signaled(fence)) >> - engine = to_request(fence)->engine; >> - dma_fence_put(fence); >> - >> + dma_resv_for_each_fence_unlocked(obj->base.resv, &cursor, false, >> + fence) { >> + if (fence && dma_fence_is_i915(fence) && >> + !dma_fence_is_signaled(fence)) >> + engine = to_request(fence)->engine; >> + } >> return engine; >> } >>
On 13/09/2021 14:16, Christian König wrote: > Simplifying the code a bit. > > Signed-off-by: Christian König <christian.koenig@amd.com> > --- > drivers/gpu/drm/i915/gem/i915_gem_wait.c | 29 ++++-------------------- > 1 file changed, 5 insertions(+), 24 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_wait.c b/drivers/gpu/drm/i915/gem/i915_gem_wait.c > index 13174541f6c8..e2173a55e527 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_wait.c > +++ b/drivers/gpu/drm/i915/gem/i915_gem_wait.c > @@ -120,31 +120,12 @@ i915_gem_object_wait_priority(struct drm_i915_gem_object *obj, > unsigned int flags, > const struct i915_sched_attr *attr) > { > - struct dma_fence *excl; > - > - if (flags & I915_WAIT_ALL) { > - struct dma_fence **shared; > - unsigned int count, i; > - int ret; > - > - ret = dma_resv_get_fences(obj->base.resv, &excl, &count, > - &shared); > - if (ret) > - return ret; > - > - for (i = 0; i < count; i++) { > - i915_gem_fence_wait_priority(shared[i], attr); > - dma_fence_put(shared[i]); > - } > - > - kfree(shared); > - } else { > - excl = dma_resv_get_excl_unlocked(obj->base.resv); > - } > + struct dma_resv_cursor cursor; > + struct dma_fence *fence; > > - if (excl) { > - i915_gem_fence_wait_priority(excl, attr); > - dma_fence_put(excl); > + dma_resv_for_each_fence_unlocked(obj->base.resv, &cursor, > + flags & I915_WAIT_ALL, fence) { > + i915_gem_fence_wait_priority(fence, attr); Do you know if the RCU lock is actually held here? I tried following the KMS code paths but got lost in core helpers and driver vfuncs. Regards, Tvrtko > } > return 0; > } >
On 14/09/2021 13:32, Christian König wrote: > Am 14.09.21 um 14:27 schrieb Tvrtko Ursulin: >> >> On 13/09/2021 14:16, Christian König wrote: >>> This is maybe even a fix since the RCU usage here looks incorrect. >> >> What you think is incorrect? Pointless extra rcu locking? > > Yeah, exactly that. I also wondered for a second if rcu_read_lock() can > nest or not. But obviously it either works or lockdep hasn't complained > yet. > > But I've made a mistake here and at a couple of other places to remove > to many rcu_read_lock() calls. Thanks for pointing that out, going to > fix it as well. Ack. >> Also, FWIW, I submitted a patch to remove this function altogether >> since its IMO pretty useless, just failed in getting anyone to ack it >> so far. > > I was on the edge of suggesting that as well since it's only debugfs > usage looked quite pointless to me. > > Feel free to CC me on the patch and you can have my acked-by. Patch is here https://patchwork.freedesktop.org/patch/451864/?series=94202&rev=1, thanks! Regards, Tvrtko > Thanks, > Christian. > >> >> Regards, >> >> Tvrtko >> >>> Signed-off-by: Christian König <christian.koenig@amd.com> >>> --- >>> drivers/gpu/drm/i915/gem/i915_gem_object.h | 15 +++++++-------- >>> 1 file changed, 7 insertions(+), 8 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.h >>> b/drivers/gpu/drm/i915/gem/i915_gem_object.h >>> index e9eecebf5c9d..3343922af4d6 100644 >>> --- a/drivers/gpu/drm/i915/gem/i915_gem_object.h >>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.h >>> @@ -500,16 +500,15 @@ static inline struct intel_engine_cs * >>> i915_gem_object_last_write_engine(struct drm_i915_gem_object *obj) >>> { >>> struct intel_engine_cs *engine = NULL; >>> + struct dma_resv_cursor cursor; >>> struct dma_fence *fence; >>> - rcu_read_lock(); >>> - fence = dma_resv_get_excl_unlocked(obj->base.resv); >>> - rcu_read_unlock(); >>> - >>> - if (fence && dma_fence_is_i915(fence) && >>> !dma_fence_is_signaled(fence)) >>> - engine = to_request(fence)->engine; >>> - dma_fence_put(fence); >>> - >>> + dma_resv_for_each_fence_unlocked(obj->base.resv, &cursor, false, >>> + fence) { >>> + if (fence && dma_fence_is_i915(fence) && >>> + !dma_fence_is_signaled(fence)) >>> + engine = to_request(fence)->engine; >>> + } >>> return engine; >>> } >>> >
On 14/09/2021 12:25, Christian König wrote: > Am 14.09.21 um 12:53 schrieb Tvrtko Ursulin: >> >> On 13/09/2021 14:16, Christian König wrote: >>> Abstract the complexity of iterating over all the fences >>> in a dma_resv object. >>> >>> The new loop handles the whole RCU and retry dance and >>> returns only fences where we can be sure we grabbed the >>> right one. >>> >>> Signed-off-by: Christian König <christian.koenig@amd.com> >>> --- >>> drivers/dma-buf/dma-resv.c | 63 ++++++++++++++++++++++++++++++++++++++ >>> include/linux/dma-resv.h | 36 ++++++++++++++++++++++ >>> 2 files changed, 99 insertions(+) >>> >>> diff --git a/drivers/dma-buf/dma-resv.c b/drivers/dma-buf/dma-resv.c >>> index 84fbe60629e3..213a9b7251ca 100644 >>> --- a/drivers/dma-buf/dma-resv.c >>> +++ b/drivers/dma-buf/dma-resv.c >>> @@ -323,6 +323,69 @@ void dma_resv_add_excl_fence(struct dma_resv >>> *obj, struct dma_fence *fence) >>> } >>> EXPORT_SYMBOL(dma_resv_add_excl_fence); >>> +/** >>> + * dma_resv_walk_unlocked - walk over fences in a dma_resv obj >>> + * @obj: the dma_resv object >>> + * @cursor: cursor to record the current position >>> + * @all_fences: true returns also the shared fences >>> + * @first: if we should start over >>> + * >>> + * Return all the fences in the dma_resv object which are not yet >>> signaled. >>> + * The returned fence has an extra local reference so will stay alive. >>> + * If a concurrent modify is detected the whole iterator is started >>> over again. >>> + */ >>> +struct dma_fence *dma_resv_walk_unlocked(struct dma_resv *obj, >>> + struct dma_resv_cursor *cursor, >>> + bool all_fences, bool first) >>> +{ >>> + struct dma_fence *fence = NULL; >>> + >>> + do { >>> + /* Drop the reference from the previous round */ >>> + dma_fence_put(fence); >>> + >>> + cursor->is_first = first; >>> + if (first) { >>> + cursor->seq = read_seqcount_begin(&obj->seq); >>> + cursor->index = -1; >>> + cursor->fences = dma_resv_shared_list(obj); >>> + cursor->is_exclusive = true; >>> + >>> + fence = dma_resv_excl_fence(obj); >>> + if (fence && test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, >>> + &fence->flags)) >>> + fence = NULL; >>> + } else { >>> + fence = NULL; >>> + } >>> + >>> + if (fence) { >>> + fence = dma_fence_get_rcu(fence); >>> + } else if (all_fences && cursor->fences) { >>> + struct dma_resv_list *fences = cursor->fences; >> >> If rcu lock is allowed to be dropped while walking the list what >> guarantees list of fences hasn't been freed? > > Ah, good point! We need to test the sequence number when we enter the > function as well. Going to fix that. Right, but just to say, I am still on the fence a bit on the concept of the unlocked iterator. So for now I am looking only superficially at the implementation and i915 side of things. > >> >> Like: >> >> 1st call >> -> gets seqcount >> -> stores cursor->fences >> >> rcu lock dropped/re-acquired >> >> 2nd call >> -> dereferences into cursor->fences -> boom? >> >>> + >>> + cursor->is_exclusive = false; >>> + while (++cursor->index < fences->shared_count) { >>> + fence = rcu_dereference(fences->shared[ >>> + cursor->index]); >>> + if (!test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, >>> + &fence->flags)) >>> + break; >>> + } >>> + if (cursor->index < fences->shared_count) >>> + fence = dma_fence_get_rcu(fence); >>> + else >>> + fence = NULL; >>> + } >>> + >>> + /* For the eventually next round */ >>> + first = true; >>> + } while (read_seqcount_retry(&obj->seq, cursor->seq)); >>> + >>> + return fence; >>> +} >>> +EXPORT_SYMBOL_GPL(dma_resv_walk_unlocked); >>> + >>> /** >>> * dma_resv_copy_fences - Copy all fences from src to dst. >>> * @dst: the destination reservation object >>> diff --git a/include/linux/dma-resv.h b/include/linux/dma-resv.h >>> index 9100dd3dc21f..f5b91c292ee0 100644 >>> --- a/include/linux/dma-resv.h >>> +++ b/include/linux/dma-resv.h >>> @@ -149,6 +149,39 @@ struct dma_resv { >>> struct dma_resv_list __rcu *fence; >>> }; >>> +/** >>> + * struct dma_resv_cursor - current position into the dma_resv fences >>> + * @seq: sequence number to check >>> + * @index: index into the shared fences >>> + * @shared: the shared fences >>> + * @is_first: true if this is the first returned fence >>> + * @is_exclusive: if the current fence is the exclusive one >>> + */ >>> +struct dma_resv_cursor { >>> + unsigned int seq; >>> + unsigned int index; >>> + struct dma_resv_list *fences; >>> + bool is_first; >> >> Is_first is useful to callers - like they are legitimately allowed to >> look inside this, what could otherwise be private object? > > Yes, I was pondering on the same question. Key point is that this is > only used by other dma_resv functions which also use cursor.fences for > example. > > So this is only supposed to be used by code working with other privates > of the dma_resv object as well. Hmmm and you think external callers have no legitimate case of detecting restarts? Or to better say will not have the need of distinguishing between real restarts and just the first iteration? I need to read more of the series to get a more complete opinion here. > > Maybe adding that as a comment? > >> What is the intended use case, given when true the returned fence can >> be either exclusive or first from a shared list? > > To reset counters for and know that the sequence has restarted. See how > this is used in dma_resv_copy_fences() for example: > > rcu_read_lock(); > dma_resv_for_each_fence_unlocked(dst, &cursor, true, f) { > > if (cursor.is_first) { > dma_resv_list_free(list); > dma_fence_put(excl); > > if (cursor.fences) { > unsigned int cnt = > cursor.fences->shared_count; > > rcu_read_unlock(); > list = dma_resv_list_alloc(cnt); > ... > > >> >>> + bool is_exclusive; >> >> Is_exclusive could be written as index == -1 in the code, right? If so >> then an opportunity to remove some redundancy. > > Correct and good point. But I want to move away a bit from the > exclusive/shared notation in the future and avoid that tests like index > == -1 spread around in the code. > > What we could do is to add some helper like > dma_resv_cursor_is_excludive(cursor) which tests that. Helper is I think a good solution. > >> >>> +}; >>> + >>> +/** >>> + * dma_resv_for_each_fence_unlocked - fence iterator >>> + * @obj: a dma_resv object pointer >>> + * @cursor: a struct dma_resv_cursor pointer >>> + * @all_fences: true if all fences should be returned >>> + * @fence: the current fence >>> + * >>> + * Iterate over the fences in a struct dma_resv object without >>> holding the >>> + * dma_resv::lock. The RCU read side lock must be hold when using >>> this, but can >>> + * be dropped and re-taken as necessary inside the loop. @all_fences >>> controls >>> + * if the shared fences are returned as well. >>> + */ >>> +#define dma_resv_for_each_fence_unlocked(obj, cursor, all_fences, >>> fence) \ >>> + for (fence = dma_resv_walk_unlocked(obj, cursor, all_fences, >>> true); \ >>> + fence; dma_fence_put(fence), \ >>> + fence = dma_resv_walk_unlocked(obj, cursor, all_fences, >>> false)) >> >> Has the fact RCU lock can be dropped so there is potential to walk >> over completely different snapshots been discussed? > > Well that's basically the heart of the functionality. Even without > dropping the RCU lock there can be an restart at any time when the > dma_resv object is modified. Hm yes.. that's one of the thing which makes me undecided yet whether a generalised helper is desirable. For example i915_gem_busy_ioctl, as converted, is not completely like-for-like. Maybe it is irrelevant for that one, but then the question needs to be answered for all of the replacements. > >> At least if I followed the code correctly - it appears there is >> potential the walk restarts from the start (exclusive slot) at any >> point during the walk. > > Correct, yes. > >> Because theoretically I think you could take an atomic snapshot of >> everything (given you have a cursor object) and then release it on the >> end condition. > > That's what the dma_resv_get_fences() function is good for, yes. This > one returns an array of fences. > > The key difference is that we need to allocate memory for that which is > at least sometimes not feasible or desired. Ah true.. dma_resv_list is not reference counted to simply grab it during setup. > Thanks for the review, Np, it is intriguing to look at the option of code consolidation. Just need to read more of the series to form a better high level opinion. Regards, Tvrtko > Christian. > >> >> Regards, >> >> Tvrtko >> >>> + >>> #define dma_resv_held(obj) lockdep_is_held(&(obj)->lock.base) >>> #define dma_resv_assert_held(obj) >>> lockdep_assert_held(&(obj)->lock.base) >>> @@ -366,6 +399,9 @@ 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_add_excl_fence(struct dma_resv *obj, struct dma_fence >>> *fence); >>> +struct dma_fence *dma_resv_walk_unlocked(struct dma_resv *obj, >>> + struct dma_resv_cursor *cursor, >>> + bool first, bool all_fences); >>> int dma_resv_get_fences(struct dma_resv *obj, struct dma_fence >>> **pfence_excl, >>> unsigned *pshared_count, struct dma_fence ***pshared); >>> int dma_resv_copy_fences(struct dma_resv *dst, struct dma_resv *src); >>> >
On 13/09/2021 14:16, Christian König wrote: > This makes the function much simpler since the complex > retry logic is now handled else where. > > Signed-off-by: Christian König <christian.koenig@amd.com> > --- > drivers/gpu/drm/i915/gem/i915_gem_busy.c | 30 +++++++----------------- > 1 file changed, 9 insertions(+), 21 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_busy.c b/drivers/gpu/drm/i915/gem/i915_gem_busy.c > index 6234e17259c1..c6c6d747b33e 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_busy.c > +++ b/drivers/gpu/drm/i915/gem/i915_gem_busy.c > @@ -82,8 +82,8 @@ i915_gem_busy_ioctl(struct drm_device *dev, void *data, > { > struct drm_i915_gem_busy *args = data; > struct drm_i915_gem_object *obj; > - struct dma_resv_list *list; > - unsigned int seq; > + struct dma_resv_cursor cursor; > + struct dma_fence *fence; > int err; > > err = -ENOENT; > @@ -109,28 +109,16 @@ i915_gem_busy_ioctl(struct drm_device *dev, void *data, > * to report the overall busyness. This is what the wait-ioctl does. > * > */ > -retry: > - seq = raw_read_seqcount(&obj->base.resv->seq); > - > - /* Translate the exclusive fence to the READ *and* WRITE engine */ > - args->busy = busy_check_writer(dma_resv_excl_fence(obj->base.resv)); > - > - /* Translate shared fences to READ set of engines */ > - list = dma_resv_shared_list(obj->base.resv); > - if (list) { > - unsigned int shared_count = list->shared_count, i; > - > - for (i = 0; i < shared_count; ++i) { > - struct dma_fence *fence = > - rcu_dereference(list->shared[i]); > - > + args->busy = false; > + dma_resv_for_each_fence_unlocked(obj->base.resv, &cursor, true, fence) { To preserve strictly identical behaviour, I think add before the below if-else: if (cursor.is_first) args->busy = 0; As mentioned elsewhere in the thread I am not convinced it is hugely important, all that can happen without it is that stale read activity gets reported, and this ioctl is not really that sensitive to that. Regards, Tvrtko > + if (cursor.is_exclusive) > + /* Translate the exclusive fence to the READ *and* WRITE engine */ > + args->busy = busy_check_writer(fence); > + else > + /* Translate shared fences to READ set of engines */ > args->busy |= busy_check_reader(fence); > - } > } > > - if (args->busy && read_seqcount_retry(&obj->base.resv->seq, seq)) > - goto retry; > - > err = 0; > out: > rcu_read_unlock(); >
Am 14.09.21 um 15:07 schrieb Tvrtko Ursulin: > On 14/09/2021 12:25, Christian König wrote: >> Am 14.09.21 um 12:53 schrieb Tvrtko Ursulin: >>> >>> On 13/09/2021 14:16, Christian König wrote: >>>> [SNIP] >>>> + if (fence) { >>>> + fence = dma_fence_get_rcu(fence); >>>> + } else if (all_fences && cursor->fences) { >>>> + struct dma_resv_list *fences = cursor->fences; >>> >>> If rcu lock is allowed to be dropped while walking the list what >>> guarantees list of fences hasn't been freed? >> >> Ah, good point! We need to test the sequence number when we enter the >> function as well. Going to fix that. > > Right, but just to say, I am still on the fence a bit on the concept > of the unlocked iterator. So for now I am looking only superficially > at the implementation and i915 side of things. I'm really in favor of taking the lock as well and contain the unlocked operation into the dma_resv object code and I think Daniel is on absolutely the same side as well. But the use cases are as they are for now and I think containing the internal structure of the dma_resv object is the right next step. >>> [SNIP] >>>> +/** >>>> + * struct dma_resv_cursor - current position into the dma_resv fences >>>> + * @seq: sequence number to check >>>> + * @index: index into the shared fences >>>> + * @shared: the shared fences >>>> + * @is_first: true if this is the first returned fence >>>> + * @is_exclusive: if the current fence is the exclusive one >>>> + */ >>>> +struct dma_resv_cursor { >>>> + unsigned int seq; >>>> + unsigned int index; >>>> + struct dma_resv_list *fences; >>>> + bool is_first; >>> >>> Is_first is useful to callers - like they are legitimately allowed >>> to look inside this, what could otherwise be private object? >> >> Yes, I was pondering on the same question. Key point is that this is >> only used by other dma_resv functions which also use cursor.fences >> for example. >> >> So this is only supposed to be used by code working with other >> privates of the dma_resv object as well. > > Hmmm and you think external callers have no legitimate case of > detecting restarts? Yes, if somebody needs a snapshot of the current state and can't for some reason take the lock they should use dma_resv_get_fences() instead. On the other hand allocating memory in dma_resv_get_fences() has probably more overhead than just grabbing and releasing the lock. > Or to better say will not have the need of distinguishing between real > restarts and just the first iteration? I need to read more of the > series to get a more complete opinion here. Yeah, that's indeed a good point. Off hand I don't see any, but we should probably decide for each place individually if we should take the lock, allocate memory or use the lockless iterator. > [SNIP] >>> >>>> +}; >>>> + >>>> +/** >>>> + * dma_resv_for_each_fence_unlocked - fence iterator >>>> + * @obj: a dma_resv object pointer >>>> + * @cursor: a struct dma_resv_cursor pointer >>>> + * @all_fences: true if all fences should be returned >>>> + * @fence: the current fence >>>> + * >>>> + * Iterate over the fences in a struct dma_resv object without >>>> holding the >>>> + * dma_resv::lock. The RCU read side lock must be hold when using >>>> this, but can >>>> + * be dropped and re-taken as necessary inside the loop. >>>> @all_fences controls >>>> + * if the shared fences are returned as well. >>>> + */ >>>> +#define dma_resv_for_each_fence_unlocked(obj, cursor, all_fences, >>>> fence) \ >>>> + for (fence = dma_resv_walk_unlocked(obj, cursor, all_fences, >>>> true); \ >>>> + fence; dma_fence_put(fence), \ >>>> + fence = dma_resv_walk_unlocked(obj, cursor, all_fences, >>>> false)) >>> >>> Has the fact RCU lock can be dropped so there is potential to walk >>> over completely different snapshots been discussed? >> >> Well that's basically the heart of the functionality. Even without >> dropping the RCU lock there can be an restart at any time when the >> dma_resv object is modified. > > Hm yes.. that's one of the thing which makes me undecided yet whether > a generalised helper is desirable. For example i915_gem_busy_ioctl, as > converted, is not completely like-for-like. Maybe it is irrelevant for > that one, but then the question needs to be answered for all of the > replacements. > >> >>> At least if I followed the code correctly - it appears there is >>> potential the walk restarts from the start (exclusive slot) at any >>> point during the walk. >> >> Correct, yes. >> >>> Because theoretically I think you could take an atomic snapshot of >>> everything (given you have a cursor object) and then release it on >>> the end condition. >> >> That's what the dma_resv_get_fences() function is good for, yes. This >> one returns an array of fences. >> >> The key difference is that we need to allocate memory for that which >> is at least sometimes not feasible or desired. > > Ah true.. dma_resv_list is not reference counted to simply grab it > during setup. > >> Thanks for the review, > > Np, it is intriguing to look at the option of code consolidation. Just > need to read more of the series to form a better high level opinion. Really appreciated, thanks for looking into this. Regards, Christian. > > Regards, > > Tvrtko > >> Christian. >> >>> >>> Regards, >>> >>> Tvrtko >>> >>>> + >>>> #define dma_resv_held(obj) lockdep_is_held(&(obj)->lock.base) >>>> #define dma_resv_assert_held(obj) >>>> lockdep_assert_held(&(obj)->lock.base) >>>> @@ -366,6 +399,9 @@ 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_add_excl_fence(struct dma_resv *obj, struct >>>> dma_fence *fence); >>>> +struct dma_fence *dma_resv_walk_unlocked(struct dma_resv *obj, >>>> + struct dma_resv_cursor *cursor, >>>> + bool first, bool all_fences); >>>> int dma_resv_get_fences(struct dma_resv *obj, struct dma_fence >>>> **pfence_excl, >>>> unsigned *pshared_count, struct dma_fence ***pshared); >>>> int dma_resv_copy_fences(struct dma_resv *dst, struct dma_resv >>>> *src); >>>> >>
Am 14.09.21 um 14:47 schrieb Tvrtko Ursulin: > > On 14/09/2021 13:32, Christian König wrote: >> Am 14.09.21 um 14:27 schrieb Tvrtko Ursulin: >>> >>> On 13/09/2021 14:16, Christian König wrote: >>>> This is maybe even a fix since the RCU usage here looks incorrect. >>> >>> What you think is incorrect? Pointless extra rcu locking? >> >> Yeah, exactly that. I also wondered for a second if rcu_read_lock() >> can nest or not. But obviously it either works or lockdep hasn't >> complained yet. >> >> But I've made a mistake here and at a couple of other places to >> remove to many rcu_read_lock() calls. Thanks for pointing that out, >> going to fix it as well. > > Ack. > >>> Also, FWIW, I submitted a patch to remove this function altogether >>> since its IMO pretty useless, just failed in getting anyone to ack >>> it so far. >> >> I was on the edge of suggesting that as well since it's only debugfs >> usage looked quite pointless to me. >> >> Feel free to CC me on the patch and you can have my acked-by. > > Patch is here > https://patchwork.freedesktop.org/patch/451864/?series=94202&rev=1, > thanks! Feel free to add an Acked-by: Christian König <christian.koenig@amd.com> to that one. Regards, Christian. > > Regards, > > Tvrtko > >> Thanks, >> Christian. >> >>> >>> Regards, >>> >>> Tvrtko >>> >>>> Signed-off-by: Christian König <christian.koenig@amd.com> >>>> --- >>>> drivers/gpu/drm/i915/gem/i915_gem_object.h | 15 +++++++-------- >>>> 1 file changed, 7 insertions(+), 8 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.h >>>> b/drivers/gpu/drm/i915/gem/i915_gem_object.h >>>> index e9eecebf5c9d..3343922af4d6 100644 >>>> --- a/drivers/gpu/drm/i915/gem/i915_gem_object.h >>>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.h >>>> @@ -500,16 +500,15 @@ static inline struct intel_engine_cs * >>>> i915_gem_object_last_write_engine(struct drm_i915_gem_object *obj) >>>> { >>>> struct intel_engine_cs *engine = NULL; >>>> + struct dma_resv_cursor cursor; >>>> struct dma_fence *fence; >>>> - rcu_read_lock(); >>>> - fence = dma_resv_get_excl_unlocked(obj->base.resv); >>>> - rcu_read_unlock(); >>>> - >>>> - if (fence && dma_fence_is_i915(fence) && >>>> !dma_fence_is_signaled(fence)) >>>> - engine = to_request(fence)->engine; >>>> - dma_fence_put(fence); >>>> - >>>> + dma_resv_for_each_fence_unlocked(obj->base.resv, &cursor, false, >>>> + fence) { >>>> + if (fence && dma_fence_is_i915(fence) && >>>> + !dma_fence_is_signaled(fence)) >>>> + engine = to_request(fence)->engine; >>>> + } >>>> return engine; >>>> } >>>> >>