| Message ID | 20210917123513.1106-15-christian.koenig@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/26] dma-buf: add dma_resv_for_each_fence_unlocked v2 | expand |
On 17/09/2021 13:35, Christian König wrote: > Simplifying the code a bit. > > v2: use dma_resv_for_each_fence instead, according to Tvrtko the lock is > held here anyway. > v3: back to using dma_resv_for_each_fence_unlocked. It did not work out - what happened? Regards, Tvrtko > Signed-off-by: Christian König <christian.koenig@amd.com> > --- > drivers/gpu/drm/i915/i915_sw_fence.c | 57 ++++++++-------------------- > 1 file changed, 15 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..7635b0478ea5 100644 > --- a/drivers/gpu/drm/i915/i915_sw_fence.c > +++ b/drivers/gpu/drm/i915/i915_sw_fence.c > @@ -572,56 +572,29 @@ int i915_sw_fence_await_reservation(struct i915_sw_fence *fence, > unsigned long timeout, > gfp_t gfp) > { > - struct dma_fence *excl; > + struct dma_resv_iter 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, > + rcu_read_lock(); > + dma_resv_iter_begin(&cursor, resv, write); > + dma_resv_for_each_fence_unlocked(&cursor, f) { > + rcu_read_unlock(); > + pending = i915_sw_fence_await_dma_fence(fence, f, timeout, > gfp); > - if (pending < 0) > + rcu_read_lock(); > + if (pending < 0) { > ret = pending; > - else > - ret |= pending; > - } > - > - dma_fence_put(excl); > + break; > + } > > + ret |= pending; > + } > + dma_resv_iter_end(&cursor); > + rcu_read_unlock(); > return ret; > } > >
On 20/09/2021 09:45, Tvrtko Ursulin wrote: > > On 17/09/2021 13:35, Christian König wrote: >> Simplifying the code a bit. >> >> v2: use dma_resv_for_each_fence instead, according to Tvrtko the lock is >> held here anyway. >> v3: back to using dma_resv_for_each_fence_unlocked. > > It did not work out - what happened? Wait, my suggestion to try the locked iterator was against i915_request_await_object. I haven't looked at this one at the time or even now. Regards, Tvrtko > Regards, > > Tvrtko > >> Signed-off-by: Christian König <christian.koenig@amd.com> >> --- >> drivers/gpu/drm/i915/i915_sw_fence.c | 57 ++++++++-------------------- >> 1 file changed, 15 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..7635b0478ea5 100644 >> --- a/drivers/gpu/drm/i915/i915_sw_fence.c >> +++ b/drivers/gpu/drm/i915/i915_sw_fence.c >> @@ -572,56 +572,29 @@ int i915_sw_fence_await_reservation(struct >> i915_sw_fence *fence, >> unsigned long timeout, >> gfp_t gfp) >> { >> - struct dma_fence *excl; >> + struct dma_resv_iter 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, >> + rcu_read_lock(); >> + dma_resv_iter_begin(&cursor, resv, write); >> + dma_resv_for_each_fence_unlocked(&cursor, f) { >> + rcu_read_unlock(); >> + pending = i915_sw_fence_await_dma_fence(fence, f, timeout, >> gfp); >> - if (pending < 0) >> + rcu_read_lock(); >> + if (pending < 0) { >> ret = pending; >> - else >> - ret |= pending; >> - } >> - >> - dma_fence_put(excl); >> + break; >> + } >> + ret |= pending; >> + } >> + dma_resv_iter_end(&cursor); >> + rcu_read_unlock(); >> return ret; >> } >>
Am 20.09.21 um 10:47 schrieb Tvrtko Ursulin: > > On 20/09/2021 09:45, Tvrtko Ursulin wrote: >> >> On 17/09/2021 13:35, Christian König wrote: >>> Simplifying the code a bit. >>> >>> v2: use dma_resv_for_each_fence instead, according to Tvrtko the >>> lock is >>> held here anyway. >>> v3: back to using dma_resv_for_each_fence_unlocked. >> >> It did not work out - what happened? > Wait, my suggestion to try the locked iterator was against > i915_request_await_object. I haven't looked at this one at the time or > even now. Exactly! I've mixed the two up and this one here is really called without holding a lock. Regards, Christian. > > Regards, > > Tvrtko > > >> Regards, >> >> Tvrtko >> >>> Signed-off-by: Christian König <christian.koenig@amd.com> >>> --- >>> drivers/gpu/drm/i915/i915_sw_fence.c | 57 >>> ++++++++-------------------- >>> 1 file changed, 15 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..7635b0478ea5 100644 >>> --- a/drivers/gpu/drm/i915/i915_sw_fence.c >>> +++ b/drivers/gpu/drm/i915/i915_sw_fence.c >>> @@ -572,56 +572,29 @@ int i915_sw_fence_await_reservation(struct >>> i915_sw_fence *fence, >>> unsigned long timeout, >>> gfp_t gfp) >>> { >>> - struct dma_fence *excl; >>> + struct dma_resv_iter 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, >>> + rcu_read_lock(); >>> + dma_resv_iter_begin(&cursor, resv, write); >>> + dma_resv_for_each_fence_unlocked(&cursor, f) { >>> + rcu_read_unlock(); >>> + pending = i915_sw_fence_await_dma_fence(fence, f, timeout, >>> gfp); >>> - if (pending < 0) >>> + rcu_read_lock(); >>> + if (pending < 0) { >>> ret = pending; >>> - else >>> - ret |= pending; >>> - } >>> - >>> - dma_fence_put(excl); >>> + break; >>> + } >>> + ret |= pending; >>> + } >>> + dma_resv_iter_end(&cursor); >>> + rcu_read_unlock(); >>> return ret; >>> } >>>
diff --git a/drivers/gpu/drm/i915/i915_sw_fence.c b/drivers/gpu/drm/i915/i915_sw_fence.c index c589a681da77..7635b0478ea5 100644 --- a/drivers/gpu/drm/i915/i915_sw_fence.c +++ b/drivers/gpu/drm/i915/i915_sw_fence.c @@ -572,56 +572,29 @@ int i915_sw_fence_await_reservation(struct i915_sw_fence *fence, unsigned long timeout, gfp_t gfp) { - struct dma_fence *excl; + struct dma_resv_iter 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, + rcu_read_lock(); + dma_resv_iter_begin(&cursor, resv, write); + dma_resv_for_each_fence_unlocked(&cursor, f) { + rcu_read_unlock(); + pending = i915_sw_fence_await_dma_fence(fence, f, timeout, gfp); - if (pending < 0) + rcu_read_lock(); + if (pending < 0) { ret = pending; - else - ret |= pending; - } - - dma_fence_put(excl); + break; + } + ret |= pending; + } + dma_resv_iter_end(&cursor); + rcu_read_unlock(); return ret; }
Simplifying the code a bit. v2: use dma_resv_for_each_fence instead, according to Tvrtko the lock is held here anyway. v3: back to using dma_resv_for_each_fence_unlocked. Signed-off-by: Christian König <christian.koenig@amd.com> --- drivers/gpu/drm/i915/i915_sw_fence.c | 57 ++++++++-------------------- 1 file changed, 15 insertions(+), 42 deletions(-)