drm/i915: Don't need a timer to wake us up

Submitted by Tvrtko Ursulin on Nov. 26, 2015, 5:15 p.m.

Details

Message ID 1448558146-4673-1-git-send-email-tvrtko.ursulin@linux.intel.com
State New
Headers show
Series "drm/i915: Don't need a timer to wake us up" ( rev: 1 ) in Intel GFX

Not browsing as part of any series.

Commit Message

Tvrtko Ursulin Nov. 26, 2015, 5:15 p.m.
From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Looks like the sleeping loop in __i915_wait_request can be
simplified by using io_schedule_timeout instead of setting
up and destroying a timer.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_gem.c | 28 ++++++++--------------------
 1 file changed, 8 insertions(+), 20 deletions(-)

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 33adc8f8ab20..efb3eb8b4a5e 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1135,11 +1135,6 @@  i915_gem_check_wedge(struct i915_gpu_error *error,
 	return 0;
 }
 
-static void fake_irq(unsigned long data)
-{
-	wake_up_process((struct task_struct *)data);
-}
-
 static bool missed_irq(struct drm_i915_private *dev_priv,
 		       struct intel_engine_cs *ring)
 {
@@ -1231,7 +1226,7 @@  int __i915_wait_request(struct drm_i915_gem_request *req,
 	}
 
 	for (;;) {
-		struct timer_list timer;
+		long sched_timeout;
 
 		prepare_to_wait(&ring->irq_queue, &wait,
 				interruptible ? TASK_INTERRUPTIBLE : TASK_UNINTERRUPTIBLE);
@@ -1262,21 +1257,14 @@  int __i915_wait_request(struct drm_i915_gem_request *req,
 			break;
 		}
 
-		timer.function = NULL;
-		if (timeout || missed_irq(dev_priv, ring)) {
-			unsigned long expire;
-
-			setup_timer_on_stack(&timer, fake_irq, (unsigned long)current);
-			expire = missed_irq(dev_priv, ring) ? jiffies + 1 : timeout_expire;
-			mod_timer(&timer, expire);
-		}
-
-		io_schedule();
+		if (timeout)
+			sched_timeout = timeout_expire - jiffies;
+		else if (missed_irq(dev_priv, ring))
+			sched_timeout = 1;
+		else
+			sched_timeout = MAX_SCHEDULE_TIMEOUT;
 
-		if (timer.function) {
-			del_singleshot_timer_sync(&timer);
-			destroy_timer_on_stack(&timer);
-		}
+		io_schedule_timeout(sched_timeout);
 	}
 	if (!irq_test_in_progress)
 		ring->irq_put(ring);

Comments

On Thu, Nov 26, 2015 at 05:15:46PM +0000, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> Looks like the sleeping loop in __i915_wait_request can be
> simplified by using io_schedule_timeout instead of setting
> up and destroying a timer.

Simplified by duplicating code? I liked the explicit handling for its
obviousness and simplicity.
-Chris
On Thu, Nov 26, 2015 at 05:55:39PM +0000, Chris Wilson wrote:
> On Thu, Nov 26, 2015 at 05:15:46PM +0000, Tvrtko Ursulin wrote:
> > From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > 
> > Looks like the sleeping loop in __i915_wait_request can be
> > simplified by using io_schedule_timeout instead of setting
> > up and destroying a timer.
> 
> Simplified by duplicating code? I liked the explicit handling for its
> obviousness and simplicity.
To be slightl more gracious, after we eliminate the irq handling from
this function, the waiter does look like this, albeit I still think it
is cleaner to keep the timeout handling distinct from the indefinite
waits.
-Chris
On 26/11/15 18:24, Chris Wilson wrote:
> On Thu, Nov 26, 2015 at 05:55:39PM +0000, Chris Wilson wrote:
>> On Thu, Nov 26, 2015 at 05:15:46PM +0000, Tvrtko Ursulin wrote:
>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>
>>> Looks like the sleeping loop in __i915_wait_request can be
>>> simplified by using io_schedule_timeout instead of setting
>>> up and destroying a timer.
>>
>> Simplified by duplicating code? I liked the explicit handling for its
>> obviousness and simplicity.
> To be slightl more gracious, after we eliminate the irq handling from
> this function, the waiter does look like this, albeit I still think it
> is cleaner to keep the timeout handling distinct from the indefinite
> waits.

Duplicating code, fake_irq obviousness and simplicity?!?

To start with:

  drivers/gpu/drm/i915/i915_gem.c | 28 ++++++++--------------------
  1 file changed, 8 insertions(+), 20 deletions(-)

So less code, less binary text, less stack space, less kernel API usage 
so also fewer cycles to execute (not that it matters here but hey), and 
easier to understand logic.

So really I don't know what you don't like about it.

Regards,

Tvrtko
On Fri, Nov 27, 2015 at 09:35:16AM +0000, Tvrtko Ursulin wrote:
> 
> On 26/11/15 18:24, Chris Wilson wrote:
> >On Thu, Nov 26, 2015 at 05:55:39PM +0000, Chris Wilson wrote:
> >>On Thu, Nov 26, 2015 at 05:15:46PM +0000, Tvrtko Ursulin wrote:
> >>>From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >>>
> >>>Looks like the sleeping loop in __i915_wait_request can be
> >>>simplified by using io_schedule_timeout instead of setting
> >>>up and destroying a timer.
> >>
> >>Simplified by duplicating code? I liked the explicit handling for its
> >>obviousness and simplicity.
> >To be slightl more gracious, after we eliminate the irq handling from
> >this function, the waiter does look like this, albeit I still think it
> >is cleaner to keep the timeout handling distinct from the indefinite
> >waits.
> 
> Duplicating code, fake_irq obviousness and simplicity?!?

Bah, the code was duplicated in Feb 2015. fake_irq is obvious and a
precise description of what we are doing and why, that's why I like it.
It is much easier to convert to other wakeups (say hrtimer, or napi
style). This code needs to be changed to solve the thundering herd
problem, if you want to improve this code, keep reviewing the
prequisites.
-Chris