Message ID | 20170511195922.15844-1-chris@chris-wilson.co.uk (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Chris Wilson <chris@chris-wilson.co.uk> writes: > My original intention was for i915_sw_fence to be the base class and > provide the reference count for the container. This was from starting > with a design to handle async_work. In practice, for i915 we embed > fences into structs which have their own independent reference counting, > making the i915_sw_fence.kref duplicitous. If we remove the kref, we > remove the i915_sw_fence's ability to free itself and its independence, > it can only exist within a container and must be supplied with a > callback to handle its release. > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > --- > drivers/gpu/drm/i915/i915_sw_fence.c | 55 ++++++++---------------------------- > drivers/gpu/drm/i915/i915_sw_fence.h | 1 - > 2 files changed, 11 insertions(+), 45 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_sw_fence.c b/drivers/gpu/drm/i915/i915_sw_fence.c > index a277f8eb7beb..a0a690d6627e 100644 > --- a/drivers/gpu/drm/i915/i915_sw_fence.c > +++ b/drivers/gpu/drm/i915/i915_sw_fence.c > @@ -120,34 +120,6 @@ void i915_sw_fence_fini(struct i915_sw_fence *fence) > } > #endif > > -static void i915_sw_fence_release(struct kref *kref) > -{ > - struct i915_sw_fence *fence = container_of(kref, typeof(*fence), kref); > - > - WARN_ON(atomic_read(&fence->pending) > 0); > - debug_fence_destroy(fence); > - > - if (fence->flags & I915_SW_FENCE_MASK) { > - __i915_sw_fence_notify(fence, FENCE_FREE); Is the current design so that the callback equals embeddding? -Mika > - } else { > - i915_sw_fence_fini(fence); > - kfree(fence); > - } > -} > - > -static void i915_sw_fence_put(struct i915_sw_fence *fence) > -{ > - debug_fence_assert(fence); > - kref_put(&fence->kref, i915_sw_fence_release); > -} > - > -static struct i915_sw_fence *i915_sw_fence_get(struct i915_sw_fence *fence) > -{ > - debug_fence_assert(fence); > - kref_get(&fence->kref); > - return fence; > -} > - > static void __i915_sw_fence_wake_up_all(struct i915_sw_fence *fence, > struct list_head *continuation) > { > @@ -202,13 +174,15 @@ static void __i915_sw_fence_complete(struct i915_sw_fence *fence, > > debug_fence_set_state(fence, DEBUG_FENCE_IDLE, DEBUG_FENCE_NOTIFY); > > - if (fence->flags & I915_SW_FENCE_MASK && > - __i915_sw_fence_notify(fence, FENCE_COMPLETE) != NOTIFY_DONE) > + if (__i915_sw_fence_notify(fence, FENCE_COMPLETE) != NOTIFY_DONE) > return; > > debug_fence_set_state(fence, DEBUG_FENCE_NOTIFY, DEBUG_FENCE_IDLE); > > __i915_sw_fence_wake_up_all(fence, continuation); > + > + debug_fence_destroy(fence); > + __i915_sw_fence_notify(fence, FENCE_FREE); > } > > static void i915_sw_fence_complete(struct i915_sw_fence *fence) > @@ -232,33 +206,26 @@ void __i915_sw_fence_init(struct i915_sw_fence *fence, > const char *name, > struct lock_class_key *key) > { > - BUG_ON((unsigned long)fn & ~I915_SW_FENCE_MASK); > + BUG_ON(!fn || (unsigned long)fn & ~I915_SW_FENCE_MASK); > > debug_fence_init(fence); > > __init_waitqueue_head(&fence->wait, name, key); > - kref_init(&fence->kref); > atomic_set(&fence->pending, 1); > fence->flags = (unsigned long)fn; > } > > -static void __i915_sw_fence_commit(struct i915_sw_fence *fence) > -{ > - i915_sw_fence_complete(fence); > - i915_sw_fence_put(fence); > -} > - > void i915_sw_fence_commit(struct i915_sw_fence *fence) > { > debug_fence_activate(fence); > - __i915_sw_fence_commit(fence); > + i915_sw_fence_complete(fence); > } > > static int i915_sw_fence_wake(wait_queue_t *wq, unsigned mode, int flags, void *key) > { > list_del(&wq->task_list); > __i915_sw_fence_complete(wq->private, key); > - i915_sw_fence_put(wq->private); > + > if (wq->flags & I915_SW_FENCE_FLAG_ALLOC) > kfree(wq); > return 0; > @@ -353,7 +320,7 @@ static int __i915_sw_fence_await_sw_fence(struct i915_sw_fence *fence, > INIT_LIST_HEAD(&wq->task_list); > wq->flags = pending; > wq->func = i915_sw_fence_wake; > - wq->private = i915_sw_fence_get(fence); > + wq->private = fence; > > i915_sw_fence_await(fence); > > @@ -402,7 +369,7 @@ static void timer_i915_sw_fence_wake(unsigned long data) > dma_fence_put(cb->dma); > cb->dma = NULL; > > - __i915_sw_fence_commit(cb->fence); > + i915_sw_fence_complete(cb->fence); > cb->timer.function = NULL; > } > > @@ -413,7 +380,7 @@ static void dma_i915_sw_fence_wake(struct dma_fence *dma, > > del_timer_sync(&cb->timer); > if (cb->timer.function) > - __i915_sw_fence_commit(cb->fence); > + i915_sw_fence_complete(cb->fence); > dma_fence_put(cb->dma); > > kfree(cb); > @@ -440,7 +407,7 @@ int i915_sw_fence_await_dma_fence(struct i915_sw_fence *fence, > return dma_fence_wait(dma, false); > } > > - cb->fence = i915_sw_fence_get(fence); > + cb->fence = fence; > i915_sw_fence_await(fence); > > cb->dma = NULL; > diff --git a/drivers/gpu/drm/i915/i915_sw_fence.h b/drivers/gpu/drm/i915/i915_sw_fence.h > index d31cefbbcc04..1d3b6051daaf 100644 > --- a/drivers/gpu/drm/i915/i915_sw_fence.h > +++ b/drivers/gpu/drm/i915/i915_sw_fence.h > @@ -23,7 +23,6 @@ struct reservation_object; > struct i915_sw_fence { > wait_queue_head_t wait; > unsigned long flags; > - struct kref kref; > atomic_t pending; > }; > > -- > 2.11.0 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Fri, May 12, 2017 at 10:43:31AM +0300, Mika Kuoppala wrote: > Chris Wilson <chris@chris-wilson.co.uk> writes: > > > My original intention was for i915_sw_fence to be the base class and > > provide the reference count for the container. This was from starting > > with a design to handle async_work. In practice, for i915 we embed > > fences into structs which have their own independent reference counting, > > making the i915_sw_fence.kref duplicitous. If we remove the kref, we > > remove the i915_sw_fence's ability to free itself and its independence, > > it can only exist within a container and must be supplied with a > > callback to handle its release. > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > --- > > drivers/gpu/drm/i915/i915_sw_fence.c | 55 ++++++++---------------------------- > > drivers/gpu/drm/i915/i915_sw_fence.h | 1 - > > 2 files changed, 11 insertions(+), 45 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_sw_fence.c b/drivers/gpu/drm/i915/i915_sw_fence.c > > index a277f8eb7beb..a0a690d6627e 100644 > > --- a/drivers/gpu/drm/i915/i915_sw_fence.c > > +++ b/drivers/gpu/drm/i915/i915_sw_fence.c > > @@ -120,34 +120,6 @@ void i915_sw_fence_fini(struct i915_sw_fence *fence) > > } > > #endif > > > > -static void i915_sw_fence_release(struct kref *kref) > > -{ > > - struct i915_sw_fence *fence = container_of(kref, typeof(*fence), kref); > > - > > - WARN_ON(atomic_read(&fence->pending) > 0); > > - debug_fence_destroy(fence); > > - > > - if (fence->flags & I915_SW_FENCE_MASK) { > > - __i915_sw_fence_notify(fence, FENCE_FREE); > > Is the current design so that the callback equals embeddding? To embed, in 99.999% of cases you really do have to employ the callback to resolve lifetime. Yes, it is possible by explicitly calling wait that you do know its state; and that is useful for on-stack fences. However, the current usage of debugobjects negects that (on-stack objects are banned), so we do a need constructor for i915_sw_fence_init_onstack(), so mandating a callback simplifies everyone's lives. -Chris
Chris Wilson <chris@chris-wilson.co.uk> writes: > My original intention was for i915_sw_fence to be the base class and > provide the reference count for the container. This was from starting > with a design to handle async_work. In practice, for i915 we embed > fences into structs which have their own independent reference counting, > making the i915_sw_fence.kref duplicitous. If we remove the kref, we > remove the i915_sw_fence's ability to free itself and its independence, > it can only exist within a container and must be supplied with a > callback to handle its release. > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Reviewed-by: Mika Kuoppala <mika.kuoppala@intel.com> > --- > drivers/gpu/drm/i915/i915_sw_fence.c | 55 ++++++++---------------------------- > drivers/gpu/drm/i915/i915_sw_fence.h | 1 - > 2 files changed, 11 insertions(+), 45 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_sw_fence.c b/drivers/gpu/drm/i915/i915_sw_fence.c > index a277f8eb7beb..a0a690d6627e 100644 > --- a/drivers/gpu/drm/i915/i915_sw_fence.c > +++ b/drivers/gpu/drm/i915/i915_sw_fence.c > @@ -120,34 +120,6 @@ void i915_sw_fence_fini(struct i915_sw_fence *fence) > } > #endif > > -static void i915_sw_fence_release(struct kref *kref) > -{ > - struct i915_sw_fence *fence = container_of(kref, typeof(*fence), kref); > - > - WARN_ON(atomic_read(&fence->pending) > 0); > - debug_fence_destroy(fence); > - > - if (fence->flags & I915_SW_FENCE_MASK) { > - __i915_sw_fence_notify(fence, FENCE_FREE); > - } else { > - i915_sw_fence_fini(fence); > - kfree(fence); > - } > -} > - > -static void i915_sw_fence_put(struct i915_sw_fence *fence) > -{ > - debug_fence_assert(fence); > - kref_put(&fence->kref, i915_sw_fence_release); > -} > - > -static struct i915_sw_fence *i915_sw_fence_get(struct i915_sw_fence *fence) > -{ > - debug_fence_assert(fence); > - kref_get(&fence->kref); > - return fence; > -} > - > static void __i915_sw_fence_wake_up_all(struct i915_sw_fence *fence, > struct list_head *continuation) > { > @@ -202,13 +174,15 @@ static void __i915_sw_fence_complete(struct i915_sw_fence *fence, > > debug_fence_set_state(fence, DEBUG_FENCE_IDLE, DEBUG_FENCE_NOTIFY); > > - if (fence->flags & I915_SW_FENCE_MASK && > - __i915_sw_fence_notify(fence, FENCE_COMPLETE) != NOTIFY_DONE) > + if (__i915_sw_fence_notify(fence, FENCE_COMPLETE) != NOTIFY_DONE) > return; > > debug_fence_set_state(fence, DEBUG_FENCE_NOTIFY, DEBUG_FENCE_IDLE); > > __i915_sw_fence_wake_up_all(fence, continuation); > + > + debug_fence_destroy(fence); > + __i915_sw_fence_notify(fence, FENCE_FREE); > } > > static void i915_sw_fence_complete(struct i915_sw_fence *fence) > @@ -232,33 +206,26 @@ void __i915_sw_fence_init(struct i915_sw_fence *fence, > const char *name, > struct lock_class_key *key) > { > - BUG_ON((unsigned long)fn & ~I915_SW_FENCE_MASK); > + BUG_ON(!fn || (unsigned long)fn & ~I915_SW_FENCE_MASK); > > debug_fence_init(fence); > > __init_waitqueue_head(&fence->wait, name, key); > - kref_init(&fence->kref); > atomic_set(&fence->pending, 1); > fence->flags = (unsigned long)fn; > } > > -static void __i915_sw_fence_commit(struct i915_sw_fence *fence) > -{ > - i915_sw_fence_complete(fence); > - i915_sw_fence_put(fence); > -} > - > void i915_sw_fence_commit(struct i915_sw_fence *fence) > { > debug_fence_activate(fence); > - __i915_sw_fence_commit(fence); > + i915_sw_fence_complete(fence); > } > > static int i915_sw_fence_wake(wait_queue_t *wq, unsigned mode, int flags, void *key) > { > list_del(&wq->task_list); > __i915_sw_fence_complete(wq->private, key); > - i915_sw_fence_put(wq->private); > + > if (wq->flags & I915_SW_FENCE_FLAG_ALLOC) > kfree(wq); > return 0; > @@ -353,7 +320,7 @@ static int __i915_sw_fence_await_sw_fence(struct i915_sw_fence *fence, > INIT_LIST_HEAD(&wq->task_list); > wq->flags = pending; > wq->func = i915_sw_fence_wake; > - wq->private = i915_sw_fence_get(fence); > + wq->private = fence; > > i915_sw_fence_await(fence); > > @@ -402,7 +369,7 @@ static void timer_i915_sw_fence_wake(unsigned long data) > dma_fence_put(cb->dma); > cb->dma = NULL; > > - __i915_sw_fence_commit(cb->fence); > + i915_sw_fence_complete(cb->fence); > cb->timer.function = NULL; > } > > @@ -413,7 +380,7 @@ static void dma_i915_sw_fence_wake(struct dma_fence *dma, > > del_timer_sync(&cb->timer); > if (cb->timer.function) > - __i915_sw_fence_commit(cb->fence); > + i915_sw_fence_complete(cb->fence); > dma_fence_put(cb->dma); > > kfree(cb); > @@ -440,7 +407,7 @@ int i915_sw_fence_await_dma_fence(struct i915_sw_fence *fence, > return dma_fence_wait(dma, false); > } > > - cb->fence = i915_sw_fence_get(fence); > + cb->fence = fence; > i915_sw_fence_await(fence); > > cb->dma = NULL; > diff --git a/drivers/gpu/drm/i915/i915_sw_fence.h b/drivers/gpu/drm/i915/i915_sw_fence.h > index d31cefbbcc04..1d3b6051daaf 100644 > --- a/drivers/gpu/drm/i915/i915_sw_fence.h > +++ b/drivers/gpu/drm/i915/i915_sw_fence.h > @@ -23,7 +23,6 @@ struct reservation_object; > struct i915_sw_fence { > wait_queue_head_t wait; > unsigned long flags; > - struct kref kref; > atomic_t pending; > }; > > -- > 2.11.0 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/i915/i915_sw_fence.c b/drivers/gpu/drm/i915/i915_sw_fence.c index a277f8eb7beb..a0a690d6627e 100644 --- a/drivers/gpu/drm/i915/i915_sw_fence.c +++ b/drivers/gpu/drm/i915/i915_sw_fence.c @@ -120,34 +120,6 @@ void i915_sw_fence_fini(struct i915_sw_fence *fence) } #endif -static void i915_sw_fence_release(struct kref *kref) -{ - struct i915_sw_fence *fence = container_of(kref, typeof(*fence), kref); - - WARN_ON(atomic_read(&fence->pending) > 0); - debug_fence_destroy(fence); - - if (fence->flags & I915_SW_FENCE_MASK) { - __i915_sw_fence_notify(fence, FENCE_FREE); - } else { - i915_sw_fence_fini(fence); - kfree(fence); - } -} - -static void i915_sw_fence_put(struct i915_sw_fence *fence) -{ - debug_fence_assert(fence); - kref_put(&fence->kref, i915_sw_fence_release); -} - -static struct i915_sw_fence *i915_sw_fence_get(struct i915_sw_fence *fence) -{ - debug_fence_assert(fence); - kref_get(&fence->kref); - return fence; -} - static void __i915_sw_fence_wake_up_all(struct i915_sw_fence *fence, struct list_head *continuation) { @@ -202,13 +174,15 @@ static void __i915_sw_fence_complete(struct i915_sw_fence *fence, debug_fence_set_state(fence, DEBUG_FENCE_IDLE, DEBUG_FENCE_NOTIFY); - if (fence->flags & I915_SW_FENCE_MASK && - __i915_sw_fence_notify(fence, FENCE_COMPLETE) != NOTIFY_DONE) + if (__i915_sw_fence_notify(fence, FENCE_COMPLETE) != NOTIFY_DONE) return; debug_fence_set_state(fence, DEBUG_FENCE_NOTIFY, DEBUG_FENCE_IDLE); __i915_sw_fence_wake_up_all(fence, continuation); + + debug_fence_destroy(fence); + __i915_sw_fence_notify(fence, FENCE_FREE); } static void i915_sw_fence_complete(struct i915_sw_fence *fence) @@ -232,33 +206,26 @@ void __i915_sw_fence_init(struct i915_sw_fence *fence, const char *name, struct lock_class_key *key) { - BUG_ON((unsigned long)fn & ~I915_SW_FENCE_MASK); + BUG_ON(!fn || (unsigned long)fn & ~I915_SW_FENCE_MASK); debug_fence_init(fence); __init_waitqueue_head(&fence->wait, name, key); - kref_init(&fence->kref); atomic_set(&fence->pending, 1); fence->flags = (unsigned long)fn; } -static void __i915_sw_fence_commit(struct i915_sw_fence *fence) -{ - i915_sw_fence_complete(fence); - i915_sw_fence_put(fence); -} - void i915_sw_fence_commit(struct i915_sw_fence *fence) { debug_fence_activate(fence); - __i915_sw_fence_commit(fence); + i915_sw_fence_complete(fence); } static int i915_sw_fence_wake(wait_queue_t *wq, unsigned mode, int flags, void *key) { list_del(&wq->task_list); __i915_sw_fence_complete(wq->private, key); - i915_sw_fence_put(wq->private); + if (wq->flags & I915_SW_FENCE_FLAG_ALLOC) kfree(wq); return 0; @@ -353,7 +320,7 @@ static int __i915_sw_fence_await_sw_fence(struct i915_sw_fence *fence, INIT_LIST_HEAD(&wq->task_list); wq->flags = pending; wq->func = i915_sw_fence_wake; - wq->private = i915_sw_fence_get(fence); + wq->private = fence; i915_sw_fence_await(fence); @@ -402,7 +369,7 @@ static void timer_i915_sw_fence_wake(unsigned long data) dma_fence_put(cb->dma); cb->dma = NULL; - __i915_sw_fence_commit(cb->fence); + i915_sw_fence_complete(cb->fence); cb->timer.function = NULL; } @@ -413,7 +380,7 @@ static void dma_i915_sw_fence_wake(struct dma_fence *dma, del_timer_sync(&cb->timer); if (cb->timer.function) - __i915_sw_fence_commit(cb->fence); + i915_sw_fence_complete(cb->fence); dma_fence_put(cb->dma); kfree(cb); @@ -440,7 +407,7 @@ int i915_sw_fence_await_dma_fence(struct i915_sw_fence *fence, return dma_fence_wait(dma, false); } - cb->fence = i915_sw_fence_get(fence); + cb->fence = fence; i915_sw_fence_await(fence); cb->dma = NULL; diff --git a/drivers/gpu/drm/i915/i915_sw_fence.h b/drivers/gpu/drm/i915/i915_sw_fence.h index d31cefbbcc04..1d3b6051daaf 100644 --- a/drivers/gpu/drm/i915/i915_sw_fence.h +++ b/drivers/gpu/drm/i915/i915_sw_fence.h @@ -23,7 +23,6 @@ struct reservation_object; struct i915_sw_fence { wait_queue_head_t wait; unsigned long flags; - struct kref kref; atomic_t pending; };
My original intention was for i915_sw_fence to be the base class and provide the reference count for the container. This was from starting with a design to handle async_work. In practice, for i915 we embed fences into structs which have their own independent reference counting, making the i915_sw_fence.kref duplicitous. If we remove the kref, we remove the i915_sw_fence's ability to free itself and its independence, it can only exist within a container and must be supplied with a callback to handle its release. Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> --- drivers/gpu/drm/i915/i915_sw_fence.c | 55 ++++++++---------------------------- drivers/gpu/drm/i915/i915_sw_fence.h | 1 - 2 files changed, 11 insertions(+), 45 deletions(-)