| Message ID | 20210913131707.45639-19-christian.koenig@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/26] dma-buf: add dma_resv_for_each_fence_unlocked | expand |
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 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; >>> } >>> >
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; >>>> } >>>> >>
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; }
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(-)