[Intel-gfx] [PATCH 35/39] drm/i915: Encode fence specific waitqueue behaviour into the wait.flags

[Thomas Hellström (Intel)]

Hi, Chris,

On 8/26/20 3:28 PM, Chris Wilson wrote:
> Use the wait_queue_entry.flags to denote the special fence behaviour
> (flattening continuations along fence chains, and for propagating
> errors) rather than trying to detect ordinary waiters by their
> functions.
>
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> ---
>   drivers/gpu/drm/i915/i915_sw_fence.c | 25 +++++++++++++++----------
>   1 file changed, 15 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_sw_fence.c b/drivers/gpu/drm/i915/i915_sw_fence.c
> index 4cd2038cbe35..4e557d1c4644 100644
> --- a/drivers/gpu/drm/i915/i915_sw_fence.c
> +++ b/drivers/gpu/drm/i915/i915_sw_fence.c
> @@ -18,10 +18,15 @@
>   #define I915_SW_FENCE_BUG_ON(expr) BUILD_BUG_ON_INVALID(expr)
>   #endif
>   
> -#define I915_SW_FENCE_FLAG_ALLOC BIT(3) /* after WQ_FLAG_* for safety */
> -
>   static DEFINE_SPINLOCK(i915_sw_fence_lock);
>   
> +#define WQ_FLAG_BITS \
> +	BITS_PER_TYPE(typeof_member(struct wait_queue_entry, flags))
> +
> +/* after WQ_FLAG_* for safety */
> +#define I915_SW_FENCE_FLAG_FENCE BIT(WQ_FLAG_BITS - 1)
> +#define I915_SW_FENCE_FLAG_ALLOC BIT(WQ_FLAG_BITS - 2)

Not sure if sharing the flags field with the wait.c implementation is 
fully OK either. Is the @key parameter to the wake function useable? I 
mean rather than passing just a list head could we pass something like

struct i915_sw_fence_key {
     bool no_recursion; /* Makes the wait function just put its entry on 
@continuation and return */
     int error;
     struct list_head continuation;
}

/Thomas





> +
>   enum {
>   	DEBUG_FENCE_IDLE = 0,
>   	DEBUG_FENCE_NOTIFY,
> @@ -154,10 +159,10 @@ static void __i915_sw_fence_wake_up_all(struct i915_sw_fence *fence,
>   	spin_lock_irqsave_nested(&x->lock, flags, 1 + !!continuation);
>   	if (continuation) {
>   		list_for_each_entry_safe(pos, next, &x->head, entry) {
> -			if (pos->func == autoremove_wake_function)
> -				pos->func(pos, TASK_NORMAL, 0, continuation);
> -			else
> +			if (pos->flags & I915_SW_FENCE_FLAG_FENCE)
>   				list_move_tail(&pos->entry, continuation);
> +			else
> +				pos->func(pos, TASK_NORMAL, 0, continuation);
>   		}
>   	} else {
>   		LIST_HEAD(extra);
> @@ -166,9 +171,9 @@ static void __i915_sw_fence_wake_up_all(struct i915_sw_fence *fence,
>   			list_for_each_entry_safe(pos, next, &x->head, entry) {
>   				int wake_flags;
>   
> -				wake_flags = fence->error;
> -				if (pos->func == autoremove_wake_function)
> -					wake_flags = 0;
> +				wake_flags = 0;
> +				if (pos->flags & I915_SW_FENCE_FLAG_FENCE)
> +					wake_flags = fence->error;
>   
>   				pos->func(pos, TASK_NORMAL, wake_flags, &extra);
>   			}
> @@ -332,8 +337,8 @@ static int __i915_sw_fence_await_sw_fence(struct i915_sw_fence *fence,
>   					  struct i915_sw_fence *signaler,
>   					  wait_queue_entry_t *wq, gfp_t gfp)
>   {
> +	unsigned int pending;
>   	unsigned long flags;
> -	int pending;
>   
>   	debug_fence_assert(fence);
>   	might_sleep_if(gfpflags_allow_blocking(gfp));
> @@ -349,7 +354,7 @@ static int __i915_sw_fence_await_sw_fence(struct i915_sw_fence *fence,
>   	if (unlikely(i915_sw_fence_check_if_after(fence, signaler)))
>   		return -EINVAL;
>   
> -	pending = 0;
> +	pending = I915_SW_FENCE_FLAG_FENCE;
>   	if (!wq) {
>   		wq = kmalloc(sizeof(*wq), gfp);
>   		if (!wq) {