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

[Tvrtko Ursulin]

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 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.

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