diff mbox

[07/42] drm/i915: Allow i915_sw_fence_await_sw_fence() to allocate

Message ID 20161007094635.28319-8-chris@chris-wilson.co.uk (mailing list archive)
State New, archived
Headers show

Commit Message

Chris Wilson Oct. 7, 2016, 9:46 a.m. UTC
In forthcoming patches, we want to be able to dynamically allocate the
wait_queue_t used whilst awaiting. This is more convenient if we extend
the i915_sw_fence_await_sw_fence() to perform the allocation for us if
we pass in a gfp mask as an alternative than a preallocated struct.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_sw_fence.c | 40 ++++++++++++++++++++++++++++++++----
 drivers/gpu/drm/i915/i915_sw_fence.h |  8 ++++++++
 2 files changed, 44 insertions(+), 4 deletions(-)

Comments

Tvrtko Ursulin Oct. 7, 2016, 4:10 p.m. UTC | #1
On 07/10/2016 10:46, Chris Wilson wrote:
> In forthcoming patches, we want to be able to dynamically allocate the
> wait_queue_t used whilst awaiting. This is more convenient if we extend
> the i915_sw_fence_await_sw_fence() to perform the allocation for us if
> we pass in a gfp mask as an alternative than a preallocated struct.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> ---
>   drivers/gpu/drm/i915/i915_sw_fence.c | 40 ++++++++++++++++++++++++++++++++----
>   drivers/gpu/drm/i915/i915_sw_fence.h |  8 ++++++++
>   2 files changed, 44 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_sw_fence.c b/drivers/gpu/drm/i915/i915_sw_fence.c
> index 1e5cbc585ca2..dfb59dced0ce 100644
> --- a/drivers/gpu/drm/i915/i915_sw_fence.c
> +++ b/drivers/gpu/drm/i915/i915_sw_fence.c
> @@ -13,6 +13,8 @@
>   
>   #include "i915_sw_fence.h"
>   
> +#define I915_SW_FENCE_FLAG_ALLOC BIT(0)
> +

You collide with WQ_FLAG_EXCLUSIVE here, so I assume i915 will have 
complete control of the wq in question?

>   static DEFINE_SPINLOCK(i915_sw_fence_lock);
>   
>   static int __i915_sw_fence_notify(struct i915_sw_fence *fence,
> @@ -135,6 +137,8 @@ static int i915_sw_fence_wake(wait_queue_t *wq, unsigned mode, int flags, void *
>   	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)
> +		kfree(wq);
>   	return 0;
>   }
>   
> @@ -192,9 +196,9 @@ static bool i915_sw_fence_check_if_after(struct i915_sw_fence *fence,
>   	return err;
>   }
>   
> -int i915_sw_fence_await_sw_fence(struct i915_sw_fence *fence,
> -				 struct i915_sw_fence *signaler,
> -				 wait_queue_t *wq)
> +static int __i915_sw_fence_await_sw_fence(struct i915_sw_fence *fence,
> +					  struct i915_sw_fence *signaler,
> +					  wait_queue_t *wq, gfp_t gfp)
>   {
>   	unsigned long flags;
>   	int pending;
> @@ -206,8 +210,22 @@ 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;
> +	if (!wq) {
> +		wq = kmalloc(sizeof(*wq), gfp);
> +		if (!wq) {
> +			if (!gfpflags_allow_blocking(gfp))
> +				return -ENOMEM;
> +
> +			i915_sw_fence_wait(signaler);

This looks like a strange API, unless I am missing something - it 
normally sets things up for waiting, unless the allocation fails when it 
actually does the wait?

> +			return 0;
> +		}
> +
> +		pending |= I915_SW_FENCE_FLAG_ALLOC;

Why is this stored in pending? Just because it is around as re-used?

> +	}
> +
>   	INIT_LIST_HEAD(&wq->task_list);
> -	wq->flags = 0;
> +	wq->flags = pending;
>   	wq->func = i915_sw_fence_wake;
>   	wq->private = i915_sw_fence_get(fence);
>   
> @@ -226,6 +244,20 @@ int i915_sw_fence_await_sw_fence(struct i915_sw_fence *fence,
>   	return pending;
>   }
>   
> +int i915_sw_fence_await_sw_fence(struct i915_sw_fence *fence,
> +				 struct i915_sw_fence *signaler,
> +				 wait_queue_t *wq)
> +{
> +	return __i915_sw_fence_await_sw_fence(fence, signaler, wq, 0);
> +}
> +
> +int i915_sw_fence_await_sw_fence_gfp(struct i915_sw_fence *fence,
> +				     struct i915_sw_fence *signaler,
> +				     gfp_t gfp)
> +{
> +	return __i915_sw_fence_await_sw_fence(fence, signaler, NULL, gfp);
> +}
> +
>   struct dma_fence_cb {
>   	struct fence_cb base;
>   	struct i915_sw_fence *fence;
> diff --git a/drivers/gpu/drm/i915/i915_sw_fence.h b/drivers/gpu/drm/i915/i915_sw_fence.h
> index 373141602ca4..5861c69f24d4 100644
> --- a/drivers/gpu/drm/i915/i915_sw_fence.h
> +++ b/drivers/gpu/drm/i915/i915_sw_fence.h
> @@ -46,6 +46,9 @@ void i915_sw_fence_commit(struct i915_sw_fence *fence);
>   int i915_sw_fence_await_sw_fence(struct i915_sw_fence *fence,
>   				 struct i915_sw_fence *after,
>   				 wait_queue_t *wq);
> +int i915_sw_fence_await_sw_fence_gfp(struct i915_sw_fence *fence,
> +				     struct i915_sw_fence *after,
> +				     gfp_t gfp);
>   int i915_sw_fence_await_dma_fence(struct i915_sw_fence *fence,
>   				  struct fence *dma,
>   				  unsigned long timeout,
> @@ -62,4 +65,9 @@ static inline bool i915_sw_fence_done(const struct i915_sw_fence *fence)
>   	return atomic_read(&fence->pending) < 0;
>   }
>   
> +static inline void i915_sw_fence_wait(struct i915_sw_fence *fence)
> +{
> +	wait_event(fence->wait, i915_sw_fence_done(fence));
> +}
> +
>   #endif /* _I915_SW_FENCE_H_ */

Regards,

Tvrtko
Chris Wilson Oct. 7, 2016, 4:22 p.m. UTC | #2
On Fri, Oct 07, 2016 at 05:10:04PM +0100, Tvrtko Ursulin wrote:
> 
> On 07/10/2016 10:46, Chris Wilson wrote:
> >In forthcoming patches, we want to be able to dynamically allocate the
> >wait_queue_t used whilst awaiting. This is more convenient if we extend
> >the i915_sw_fence_await_sw_fence() to perform the allocation for us if
> >we pass in a gfp mask as an alternative than a preallocated struct.
> >
> >Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> >Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> >---
> >  drivers/gpu/drm/i915/i915_sw_fence.c | 40 ++++++++++++++++++++++++++++++++----
> >  drivers/gpu/drm/i915/i915_sw_fence.h |  8 ++++++++
> >  2 files changed, 44 insertions(+), 4 deletions(-)
> >
> >diff --git a/drivers/gpu/drm/i915/i915_sw_fence.c b/drivers/gpu/drm/i915/i915_sw_fence.c
> >index 1e5cbc585ca2..dfb59dced0ce 100644
> >--- a/drivers/gpu/drm/i915/i915_sw_fence.c
> >+++ b/drivers/gpu/drm/i915/i915_sw_fence.c
> >@@ -13,6 +13,8 @@
> >  #include "i915_sw_fence.h"
> >+#define I915_SW_FENCE_FLAG_ALLOC BIT(0)
> >+
> 
> You collide with WQ_FLAG_EXCLUSIVE here, so I assume i915 will have
> complete control of the wq in question?

Hmm, we should move bit as well, but yes the flag is atm private to the
wake up function for which we use a custom function.

> >+		wq = kmalloc(sizeof(*wq), gfp);
> >+		if (!wq) {
> >+			if (!gfpflags_allow_blocking(gfp))
> >+				return -ENOMEM;
> >+
> >+			i915_sw_fence_wait(signaler);
> 
> This looks like a strange API, unless I am missing something - it
> normally sets things up for waiting, unless the allocation fails
> when it actually does the wait?

Yes. If we blocked on the malloc and failed, we block a little more for
the completion. iirc, the idea came from the async task.
> 
> >+			return 0;
> >+		}
> >+
> >+		pending |= I915_SW_FENCE_FLAG_ALLOC;
> 
> Why is this stored in pending? Just because it is around as re-used?

Urm, yes. s/pending/tmp/
-Chris
Tvrtko Ursulin Oct. 8, 2016, 8:21 a.m. UTC | #3
On 07/10/2016 17:22, Chris Wilson wrote:
> On Fri, Oct 07, 2016 at 05:10:04PM +0100, Tvrtko Ursulin wrote:
>> On 07/10/2016 10:46, Chris Wilson wrote:
>>> In forthcoming patches, we want to be able to dynamically allocate the
>>> wait_queue_t used whilst awaiting. This is more convenient if we extend
>>> the i915_sw_fence_await_sw_fence() to perform the allocation for us if
>>> we pass in a gfp mask as an alternative than a preallocated struct.
>>>
>>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>>> Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>>> ---
>>>   drivers/gpu/drm/i915/i915_sw_fence.c | 40 ++++++++++++++++++++++++++++++++----
>>>   drivers/gpu/drm/i915/i915_sw_fence.h |  8 ++++++++
>>>   2 files changed, 44 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_sw_fence.c b/drivers/gpu/drm/i915/i915_sw_fence.c
>>> index 1e5cbc585ca2..dfb59dced0ce 100644
>>> --- a/drivers/gpu/drm/i915/i915_sw_fence.c
>>> +++ b/drivers/gpu/drm/i915/i915_sw_fence.c
>>> @@ -13,6 +13,8 @@
>>>   #include "i915_sw_fence.h"
>>> +#define I915_SW_FENCE_FLAG_ALLOC BIT(0)
>>> +
>> You collide with WQ_FLAG_EXCLUSIVE here, so I assume i915 will have
>> complete control of the wq in question?
> Hmm, we should move bit as well, but yes the flag is atm private to the
> wake up function for which we use a custom function.
>
>>> +		wq = kmalloc(sizeof(*wq), gfp);
>>> +		if (!wq) {
>>> +			if (!gfpflags_allow_blocking(gfp))
>>> +				return -ENOMEM;
>>> +
>>> +			i915_sw_fence_wait(signaler);
>> This looks like a strange API, unless I am missing something - it
>> normally sets things up for waiting, unless the allocation fails
>> when it actually does the wait?
> Yes. If we blocked on the malloc and failed, we block a little more for
> the completion. iirc, the idea came from the async task.

Still sounds strange to me. I understand it is a unlikely corner case 
but sounds like that sort of decision should happen in the caller. I 
will have to revisit the i915_sw_fence implementation and usage in more 
detail it seems.

Regards,

Tvrtko
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/i915_sw_fence.c b/drivers/gpu/drm/i915/i915_sw_fence.c
index 1e5cbc585ca2..dfb59dced0ce 100644
--- a/drivers/gpu/drm/i915/i915_sw_fence.c
+++ b/drivers/gpu/drm/i915/i915_sw_fence.c
@@ -13,6 +13,8 @@ 
 
 #include "i915_sw_fence.h"
 
+#define I915_SW_FENCE_FLAG_ALLOC BIT(0)
+
 static DEFINE_SPINLOCK(i915_sw_fence_lock);
 
 static int __i915_sw_fence_notify(struct i915_sw_fence *fence,
@@ -135,6 +137,8 @@  static int i915_sw_fence_wake(wait_queue_t *wq, unsigned mode, int flags, void *
 	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)
+		kfree(wq);
 	return 0;
 }
 
@@ -192,9 +196,9 @@  static bool i915_sw_fence_check_if_after(struct i915_sw_fence *fence,
 	return err;
 }
 
-int i915_sw_fence_await_sw_fence(struct i915_sw_fence *fence,
-				 struct i915_sw_fence *signaler,
-				 wait_queue_t *wq)
+static int __i915_sw_fence_await_sw_fence(struct i915_sw_fence *fence,
+					  struct i915_sw_fence *signaler,
+					  wait_queue_t *wq, gfp_t gfp)
 {
 	unsigned long flags;
 	int pending;
@@ -206,8 +210,22 @@  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;
+	if (!wq) {
+		wq = kmalloc(sizeof(*wq), gfp);
+		if (!wq) {
+			if (!gfpflags_allow_blocking(gfp))
+				return -ENOMEM;
+
+			i915_sw_fence_wait(signaler);
+			return 0;
+		}
+
+		pending |= I915_SW_FENCE_FLAG_ALLOC;
+	}
+
 	INIT_LIST_HEAD(&wq->task_list);
-	wq->flags = 0;
+	wq->flags = pending;
 	wq->func = i915_sw_fence_wake;
 	wq->private = i915_sw_fence_get(fence);
 
@@ -226,6 +244,20 @@  int i915_sw_fence_await_sw_fence(struct i915_sw_fence *fence,
 	return pending;
 }
 
+int i915_sw_fence_await_sw_fence(struct i915_sw_fence *fence,
+				 struct i915_sw_fence *signaler,
+				 wait_queue_t *wq)
+{
+	return __i915_sw_fence_await_sw_fence(fence, signaler, wq, 0);
+}
+
+int i915_sw_fence_await_sw_fence_gfp(struct i915_sw_fence *fence,
+				     struct i915_sw_fence *signaler,
+				     gfp_t gfp)
+{
+	return __i915_sw_fence_await_sw_fence(fence, signaler, NULL, gfp);
+}
+
 struct dma_fence_cb {
 	struct fence_cb base;
 	struct i915_sw_fence *fence;
diff --git a/drivers/gpu/drm/i915/i915_sw_fence.h b/drivers/gpu/drm/i915/i915_sw_fence.h
index 373141602ca4..5861c69f24d4 100644
--- a/drivers/gpu/drm/i915/i915_sw_fence.h
+++ b/drivers/gpu/drm/i915/i915_sw_fence.h
@@ -46,6 +46,9 @@  void i915_sw_fence_commit(struct i915_sw_fence *fence);
 int i915_sw_fence_await_sw_fence(struct i915_sw_fence *fence,
 				 struct i915_sw_fence *after,
 				 wait_queue_t *wq);
+int i915_sw_fence_await_sw_fence_gfp(struct i915_sw_fence *fence,
+				     struct i915_sw_fence *after,
+				     gfp_t gfp);
 int i915_sw_fence_await_dma_fence(struct i915_sw_fence *fence,
 				  struct fence *dma,
 				  unsigned long timeout,
@@ -62,4 +65,9 @@  static inline bool i915_sw_fence_done(const struct i915_sw_fence *fence)
 	return atomic_read(&fence->pending) < 0;
 }
 
+static inline void i915_sw_fence_wait(struct i915_sw_fence *fence)
+{
+	wait_event(fence->wait, i915_sw_fence_done(fence));
+}
+
 #endif /* _I915_SW_FENCE_H_ */