[Intel-gfx] [PATCH] drm/i915/sw_fence: Replace private use of wq->flags with bit zero in wq->private

[Chris Wilson]

On Wed, Nov 02, 2016 at 05:00:28PM +0000, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin at 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 at intel.com>
> Cc: Chris Wilson <chris at 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

-- 
Chris Wilson, Intel Open Source Technology Centre