On Wed, Nov 02, 2016 at 05:00:28PM +0000, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
>
> 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@xxxxxxxxx>
> Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> ---
> 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
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
