| Message ID | 1478106028-27602-1-git-send-email-tvrtko.ursulin@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Nov 02, 2016 at 05:00:28PM +0000, Tvrtko Ursulin wrote: > From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > Use of an un-allocated bit in flags is making me nervous so I > thought to use the bit zero of the private pointer instead. > > That should be safer against the core kernel changes and safe > since I can't imagine we can get a fence at the odd address. I'm not squeamish about using flags ;) > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > --- > drivers/gpu/drm/i915/i915_sw_fence.c | 24 ++++++++++++++++-------- > 1 file changed, 16 insertions(+), 8 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_sw_fence.c b/drivers/gpu/drm/i915/i915_sw_fence.c > index 95f2f12e0917..cd4d6b915848 100644 > --- a/drivers/gpu/drm/i915/i915_sw_fence.c > +++ b/drivers/gpu/drm/i915/i915_sw_fence.c > @@ -13,7 +13,8 @@ > > #include "i915_sw_fence.h" > > -#define I915_SW_FENCE_FLAG_ALLOC BIT(3) /* after WQ_FLAG_* for safety */ > +#define I915_SW_FENCE_FLAG_ALLOC (1) BIT(0) before Joonas notices. > +#define I915_SW_FENCE_PRIVATE_MASK ~(I915_SW_FENCE_FLAG_ALLOC) > > static DEFINE_SPINLOCK(i915_sw_fence_lock); > > @@ -132,12 +133,17 @@ void i915_sw_fence_commit(struct i915_sw_fence *fence) > i915_sw_fence_put(fence); > } > > +#define wq_to_i915_sw_fence(wq) (struct i915_sw_fence *) \ > + ((unsigned long)(wq)->private & I915_SW_FENCE_PRIVATE_MASK) I quite like: #define wq_to_i915_sw_fence(wq) ({ unsigned long __v = (unsigned long)(wq)->private; (struct i915_sw_fence *)(__v & I915_SW_FENCE_PRIVATE_MASK); )} or better static inline struct i915_sw_fence * wq_to_i915_sw_fence(const wait_queue_t *wq) { unsigned long __v = (unsigned long)wq->private; return (struct i915_sw_fence *)(__v & I915_SW_FENCE_PRIVATE_MASK); } static inline bool wq_is_alloc(const wait_queue_t *wq) { unsigned long __v = (unsigned long)wq->private; return __v & I915_SW_FENCE_FLAG_ALLOC; } > + > static int i915_sw_fence_wake(wait_queue_t *wq, unsigned mode, int flags, void *key) > { > + struct i915_sw_fence *fence = wq_to_i915_sw_fence(wq); > + > 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) > + __i915_sw_fence_complete(fence, key); > + i915_sw_fence_put(fence); > + if ((unsigned long)wq->private & I915_SW_FENCE_FLAG_ALLOC) > kfree(wq); > return 0; > } > @@ -157,7 +163,8 @@ static bool __i915_sw_fence_check_if_after(struct i915_sw_fence *fence, > if (wq->func != i915_sw_fence_wake) > continue; > > - if (__i915_sw_fence_check_if_after(wq->private, signaler)) > + if (__i915_sw_fence_check_if_after(wq_to_i915_sw_fence(wq), > + signaler)) > return true; > } > > @@ -175,7 +182,7 @@ static void __i915_sw_fence_clear_checked_bit(struct i915_sw_fence *fence) > if (wq->func != i915_sw_fence_wake) > continue; > > - __i915_sw_fence_clear_checked_bit(wq->private); > + __i915_sw_fence_clear_checked_bit(wq_to_i915_sw_fence(wq)); > } > } > > @@ -221,13 +228,14 @@ static int __i915_sw_fence_await_sw_fence(struct i915_sw_fence *fence, > return 0; > } > > - pending |= I915_SW_FENCE_FLAG_ALLOC; > + pending = I915_SW_FENCE_FLAG_ALLOC; > } > > INIT_LIST_HEAD(&wq->task_list); > - wq->flags = pending; We still need to set wq->flags to 0. It looks ok, but I just don't see the point. wq->flags is private to the wq->func callback. -Chris
On 02/11/2016 17:34, Chris Wilson wrote: > On Wed, Nov 02, 2016 at 05:00:28PM +0000, Tvrtko Ursulin wrote: >> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >> >> Use of an un-allocated bit in flags is making me nervous so I >> thought to use the bit zero of the private pointer instead. >> >> That should be safer against the core kernel changes and safe >> since I can't imagine we can get a fence at the odd address. > > I'm not squeamish about using flags ;) > >> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >> Cc: Chris Wilson <chris@chris-wilson.co.uk> >> --- >> drivers/gpu/drm/i915/i915_sw_fence.c | 24 ++++++++++++++++-------- >> 1 file changed, 16 insertions(+), 8 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_sw_fence.c b/drivers/gpu/drm/i915/i915_sw_fence.c >> index 95f2f12e0917..cd4d6b915848 100644 >> --- a/drivers/gpu/drm/i915/i915_sw_fence.c >> +++ b/drivers/gpu/drm/i915/i915_sw_fence.c >> @@ -13,7 +13,8 @@ >> >> #include "i915_sw_fence.h" >> >> -#define I915_SW_FENCE_FLAG_ALLOC BIT(3) /* after WQ_FLAG_* for safety */ >> +#define I915_SW_FENCE_FLAG_ALLOC (1) > > BIT(0) before Joonas notices. > >> +#define I915_SW_FENCE_PRIVATE_MASK ~(I915_SW_FENCE_FLAG_ALLOC) >> >> static DEFINE_SPINLOCK(i915_sw_fence_lock); >> >> @@ -132,12 +133,17 @@ void i915_sw_fence_commit(struct i915_sw_fence *fence) >> i915_sw_fence_put(fence); >> } >> >> +#define wq_to_i915_sw_fence(wq) (struct i915_sw_fence *) \ >> + ((unsigned long)(wq)->private & I915_SW_FENCE_PRIVATE_MASK) > > I quite like: > > #define wq_to_i915_sw_fence(wq) ({ > unsigned long __v = (unsigned long)(wq)->private; > (struct i915_sw_fence *)(__v & I915_SW_FENCE_PRIVATE_MASK); > )} > > or better > > static inline struct i915_sw_fence * > wq_to_i915_sw_fence(const wait_queue_t *wq) > { > unsigned long __v = (unsigned long)wq->private; > return (struct i915_sw_fence *)(__v & I915_SW_FENCE_PRIVATE_MASK); > } > > static inline bool > wq_is_alloc(const wait_queue_t *wq) > { > unsigned long __v = (unsigned long)wq->private; > return __v & I915_SW_FENCE_FLAG_ALLOC; > } > >> + >> static int i915_sw_fence_wake(wait_queue_t *wq, unsigned mode, int flags, void *key) >> { >> + struct i915_sw_fence *fence = wq_to_i915_sw_fence(wq); >> + >> 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) >> + __i915_sw_fence_complete(fence, key); >> + i915_sw_fence_put(fence); >> + if ((unsigned long)wq->private & I915_SW_FENCE_FLAG_ALLOC) >> kfree(wq); >> return 0; >> } >> @@ -157,7 +163,8 @@ static bool __i915_sw_fence_check_if_after(struct i915_sw_fence *fence, >> if (wq->func != i915_sw_fence_wake) >> continue; >> >> - if (__i915_sw_fence_check_if_after(wq->private, signaler)) >> + if (__i915_sw_fence_check_if_after(wq_to_i915_sw_fence(wq), >> + signaler)) >> return true; >> } >> >> @@ -175,7 +182,7 @@ static void __i915_sw_fence_clear_checked_bit(struct i915_sw_fence *fence) >> if (wq->func != i915_sw_fence_wake) >> continue; >> >> - __i915_sw_fence_clear_checked_bit(wq->private); >> + __i915_sw_fence_clear_checked_bit(wq_to_i915_sw_fence(wq)); >> } >> } >> >> @@ -221,13 +228,14 @@ static int __i915_sw_fence_await_sw_fence(struct i915_sw_fence *fence, >> return 0; >> } >> >> - pending |= I915_SW_FENCE_FLAG_ALLOC; >> + pending = I915_SW_FENCE_FLAG_ALLOC; >> } >> >> INIT_LIST_HEAD(&wq->task_list); >> - wq->flags = pending; > > We still need to set wq->flags to 0. Ooops. > It looks ok, but I just don't see the point. wq->flags is private to the > wq->func callback. A very superficial skim shows that wake_up_common at least looks at the flags. So I thought, as long as wake_up_something gets called form somewhere on these ones, it would be safer not to potentially silently collide with some core flag. Or are you saying no one ever touches them outside i915_sw_fence.c ? Hm, I have to admit I haven't figured it all out yet so I am not sure. Regards, Tvrtko
On Thu, Nov 03, 2016 at 08:33:58AM +0000, Tvrtko Ursulin wrote: > On 02/11/2016 17:34, Chris Wilson wrote: > >It looks ok, but I just don't see the point. wq->flags is private to the > >wq->func callback. > > A very superficial skim shows that wake_up_common at least looks at > the flags. So I thought, as long as wake_up_something gets called > form somewhere on these ones, it would be safer not to potentially > silently collide with some core flag. > > Or are you saying no one ever touches them outside i915_sw_fence.c ? That would break the encapsulation of the waitqueue being inside the fence. I was not intending for people to call wake_up_all(fence.wait), kfence_wake_up_all() [bad name, I was trying to borrow the concept from wake_up_all() but it is not the same, it is for the completion of a deferred signal notify] completes the fence in addition to signaling its waiters. -Chris
diff --git a/drivers/gpu/drm/i915/i915_sw_fence.c b/drivers/gpu/drm/i915/i915_sw_fence.c index 95f2f12e0917..cd4d6b915848 100644 --- a/drivers/gpu/drm/i915/i915_sw_fence.c +++ b/drivers/gpu/drm/i915/i915_sw_fence.c @@ -13,7 +13,8 @@ #include "i915_sw_fence.h" -#define I915_SW_FENCE_FLAG_ALLOC BIT(3) /* after WQ_FLAG_* for safety */ +#define I915_SW_FENCE_FLAG_ALLOC (1) +#define I915_SW_FENCE_PRIVATE_MASK ~(I915_SW_FENCE_FLAG_ALLOC) static DEFINE_SPINLOCK(i915_sw_fence_lock); @@ -132,12 +133,17 @@ void i915_sw_fence_commit(struct i915_sw_fence *fence) i915_sw_fence_put(fence); } +#define wq_to_i915_sw_fence(wq) (struct i915_sw_fence *) \ + ((unsigned long)(wq)->private & I915_SW_FENCE_PRIVATE_MASK) + static int i915_sw_fence_wake(wait_queue_t *wq, unsigned mode, int flags, void *key) { + struct i915_sw_fence *fence = wq_to_i915_sw_fence(wq); + 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) + __i915_sw_fence_complete(fence, key); + i915_sw_fence_put(fence); + if ((unsigned long)wq->private & I915_SW_FENCE_FLAG_ALLOC) kfree(wq); return 0; } @@ -157,7 +163,8 @@ static bool __i915_sw_fence_check_if_after(struct i915_sw_fence *fence, if (wq->func != i915_sw_fence_wake) continue; - if (__i915_sw_fence_check_if_after(wq->private, signaler)) + if (__i915_sw_fence_check_if_after(wq_to_i915_sw_fence(wq), + signaler)) return true; } @@ -175,7 +182,7 @@ static void __i915_sw_fence_clear_checked_bit(struct i915_sw_fence *fence) if (wq->func != i915_sw_fence_wake) continue; - __i915_sw_fence_clear_checked_bit(wq->private); + __i915_sw_fence_clear_checked_bit(wq_to_i915_sw_fence(wq)); } } @@ -221,13 +228,14 @@ static int __i915_sw_fence_await_sw_fence(struct i915_sw_fence *fence, return 0; } - pending |= I915_SW_FENCE_FLAG_ALLOC; + pending = I915_SW_FENCE_FLAG_ALLOC; } INIT_LIST_HEAD(&wq->task_list); - wq->flags = pending; wq->func = i915_sw_fence_wake; wq->private = i915_sw_fence_get(fence); + BUG_ON((unsigned long)wq->private & I915_SW_FENCE_FLAG_ALLOC); + wq->private = (void *)((unsigned long)wq->private | pending); i915_sw_fence_await(fence);