All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] drm/i915/sw_fence: Replace private use of wq->flags with bit zero in wq->private
@ 2016-11-02 17:00 Tvrtko Ursulin
  2016-11-02 17:34 ` Chris Wilson
  2016-11-02 17:45 ` ✓ Fi.CI.BAT: success for " Patchwork
  0 siblings, 2 replies; 5+ messages in thread
From: Tvrtko Ursulin @ 2016-11-02 17:00 UTC (permalink / raw)
  To: Intel-gfx

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.

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)
+#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);
 
-- 
2.7.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply related	[flat|nested] 5+ messages in thread
* Re: [PATCH] drm/i915/sw_fence: Replace private use of wq->flags with bit zero in wq->private
  2016-11-02 17:00 [PATCH] drm/i915/sw_fence: Replace private use of wq->flags with bit zero in wq->private Tvrtko Ursulin
@ 2016-11-02 17:34 ` Chris Wilson
  2016-11-03  8:33   ` Tvrtko Ursulin
  2016-11-02 17:45 ` ✓ Fi.CI.BAT: success for " Patchwork
  1 sibling, 1 reply; 5+ messages in thread
From: Chris Wilson @ 2016-11-02 17:34 UTC (permalink / raw)
  To: Tvrtko Ursulin; +Cc: Intel-gfx

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

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 5+ messages in thread
* ✓ Fi.CI.BAT: success for drm/i915/sw_fence: Replace private use of wq->flags with bit zero in wq->private
  2016-11-02 17:00 [PATCH] drm/i915/sw_fence: Replace private use of wq->flags with bit zero in wq->private Tvrtko Ursulin
  2016-11-02 17:34 ` Chris Wilson
@ 2016-11-02 17:45 ` Patchwork
  1 sibling, 0 replies; 5+ messages in thread
From: Patchwork @ 2016-11-02 17:45 UTC (permalink / raw)
  To: Tvrtko Ursulin; +Cc: intel-gfx

== Series Details ==

Series: drm/i915/sw_fence: Replace private use of wq->flags with bit zero in wq->private
URL   : https://patchwork.freedesktop.org/series/14745/
State : success

== Summary ==

Series 14745v1 drm/i915/sw_fence: Replace private use of wq->flags with bit zero in wq->private
https://patchwork.freedesktop.org/api/1.0/series/14745/revisions/1/mbox/


fi-bdw-5557u     total:241  pass:226  dwarn:0   dfail:0   fail:0   skip:15 
fi-bsw-n3050     total:241  pass:201  dwarn:0   dfail:0   fail:0   skip:40 
fi-bxt-t5700     total:241  pass:213  dwarn:0   dfail:0   fail:0   skip:28 
fi-byt-j1900     total:241  pass:213  dwarn:0   dfail:0   fail:0   skip:28 
fi-byt-n2820     total:241  pass:209  dwarn:0   dfail:0   fail:0   skip:32 
fi-hsw-4770      total:241  pass:221  dwarn:0   dfail:0   fail:0   skip:20 
fi-hsw-4770r     total:241  pass:220  dwarn:0   dfail:0   fail:0   skip:21 
fi-ilk-650       total:241  pass:187  dwarn:0   dfail:0   fail:0   skip:54 
fi-ivb-3520m     total:241  pass:218  dwarn:0   dfail:0   fail:0   skip:23 
fi-ivb-3770      total:241  pass:218  dwarn:0   dfail:0   fail:0   skip:23 
fi-kbl-7200u     total:241  pass:219  dwarn:0   dfail:0   fail:0   skip:22 
fi-skl-6260u     total:241  pass:227  dwarn:0   dfail:0   fail:0   skip:14 
fi-skl-6700hq    total:241  pass:220  dwarn:0   dfail:0   fail:0   skip:21 
fi-skl-6700k     total:241  pass:219  dwarn:1   dfail:0   fail:0   skip:21 
fi-skl-6770hq    total:241  pass:227  dwarn:0   dfail:0   fail:0   skip:14 
fi-snb-2520m     total:241  pass:208  dwarn:0   dfail:0   fail:0   skip:33 
fi-snb-2600      total:241  pass:207  dwarn:0   dfail:0   fail:0   skip:34 

bf6b989af8b0fde56a352d9005c97b2d8e3bbbe3 drm-intel-nightly: 2016y-11m-02d-15h-44m-03s UTC integration manifest
9067391 drm/i915/sw_fence: Replace private use of wq->flags with bit zero in wq->private

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_2892/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 5+ messages in thread
* Re: [PATCH] drm/i915/sw_fence: Replace private use of wq->flags with bit zero in wq->private
  2016-11-02 17:34 ` Chris Wilson
@ 2016-11-03  8:33   ` Tvrtko Ursulin
  2016-11-03  9:04     ` Chris Wilson
  0 siblings, 1 reply; 5+ messages in thread
From: Tvrtko Ursulin @ 2016-11-03  8:33 UTC (permalink / raw)
  To: Chris Wilson, Tvrtko Ursulin, Intel-gfx, 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@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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 5+ messages in thread
* Re: [PATCH] drm/i915/sw_fence: Replace private use of wq->flags with bit zero in wq->private
  2016-11-03  8:33   ` Tvrtko Ursulin
@ 2016-11-03  9:04     ` Chris Wilson
  0 siblings, 0 replies; 5+ messages in thread
From: Chris Wilson @ 2016-11-03  9:04 UTC (permalink / raw)
  To: Tvrtko Ursulin; +Cc: Intel-gfx

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

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 5+ messages in thread
end of thread, other threads:[~2016-11-03  9:05 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-11-02 17:00 [PATCH] drm/i915/sw_fence: Replace private use of wq->flags with bit zero in wq->private Tvrtko Ursulin
2016-11-02 17:34 ` Chris Wilson
2016-11-03  8:33   ` Tvrtko Ursulin
2016-11-03  9:04     ` Chris Wilson
2016-11-02 17:45 ` ✓ Fi.CI.BAT: success for " Patchwork

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.