[PATCH 1/2] drm/i915: Align the hangcheck wakeup to the nearest second

[PATCH 1/2] drm/i915: Align the hangcheck wakeup to the nearest second

[Chris Wilson]

round_jiffies() aligns the wakeup time to the nearest second in order to
batch wakeups and reduce system load, which is useful for unimportant
coarse timers like our hangcheck.

Suggested-by: Arjan van de Ven <arjan@linux.intel.com>
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Arjan van de Ven <arjan@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h |    1 +
 drivers/gpu/drm/i915/i915_gem.c |    3 +--
 drivers/gpu/drm/i915/i915_irq.c |    5 ++---
 3 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index d8043af..f79c664 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -460,6 +460,7 @@ typedef struct drm_i915_private {
 
 	/* For hangcheck timer */
 #define DRM_I915_HANGCHECK_PERIOD 1500 /* in ms */
+#define DRM_I915_HANGCHECK_JIFFIES msecs_to_jiffies(DRM_I915_HANGCHECK_PERIOD)
 	struct timer_list hangcheck_timer;
 	int hangcheck_count;
 	uint32_t last_acthd[I915_NUM_RINGS];
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index c78f8e3..8e05d53 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2151,8 +2151,7 @@ i915_add_request(struct intel_ring_buffer *ring,
 	if (!dev_priv->mm.suspended) {
 		if (i915_enable_hangcheck) {
 			mod_timer(&dev_priv->hangcheck_timer,
-				  jiffies +
-				  msecs_to_jiffies(DRM_I915_HANGCHECK_PERIOD));
+				  round_jiffies_relative(DRM_I915_HANGCHECK_JIFFIES));
 		}
 		if (was_empty) {
 			queue_delayed_work(dev_priv->wq,
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index e18e56b..67dc487 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -353,8 +353,7 @@ static void notify_ring(struct drm_device *dev,
 	if (i915_enable_hangcheck) {
 		dev_priv->hangcheck_count = 0;
 		mod_timer(&dev_priv->hangcheck_timer,
-			  jiffies +
-			  msecs_to_jiffies(DRM_I915_HANGCHECK_PERIOD));
+			  round_jiffies_relative(DRM_I915_HANGCHECK_JIFFIES));
 	}
 }
 
@@ -1787,7 +1786,7 @@ void i915_hangcheck_elapsed(unsigned long data)
 repeat:
 	/* Reset timer case chip hangs without another request being added */
 	mod_timer(&dev_priv->hangcheck_timer,
-		  jiffies + msecs_to_jiffies(DRM_I915_HANGCHECK_PERIOD));
+		  round_jiffies_relative(DRM_I915_HANGCHECK_JIFFIES));
 }
 
 /* drm_dma.h hooks
-- 
1.7.10.4

[PATCH 2/2] drm/i915: Align the retire_requests worker to the nearest second

[Chris Wilson]

By using round_jiffies() we can align the wakeup of our worker to the
nearest second in order to batch wakeups and reduce system load, which
is useful for unimportant coarse tasks like our retire_requests.

Suggested-by: Arjan van de Ven <arjan@linux.intel.com>
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Arjan van de Ven <arjan@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c |   14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 8e05d53..706f481 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2084,6 +2084,11 @@ i915_gem_next_request_seqno(struct intel_ring_buffer *ring)
 	return ring->outstanding_lazy_request;
 }
 
+static unsigned long round_jiffies_delay(unsigned long delay)
+{
+	return round_jiffies_relative(delay) - jiffies;
+}
+
 int
 i915_add_request(struct intel_ring_buffer *ring,
 		 struct drm_file *file,
@@ -2155,7 +2160,8 @@ i915_add_request(struct intel_ring_buffer *ring,
 		}
 		if (was_empty) {
 			queue_delayed_work(dev_priv->wq,
-					   &dev_priv->mm.retire_work, HZ);
+					   &dev_priv->mm.retire_work,
+					   round_jiffies_delay(HZ));
 			intel_mark_busy(dev_priv->dev);
 		}
 	}
@@ -2346,7 +2352,8 @@ i915_gem_retire_work_handler(struct work_struct *work)
 
 	/* Come back later if the device is busy... */
 	if (!mutex_trylock(&dev->struct_mutex)) {
-		queue_delayed_work(dev_priv->wq, &dev_priv->mm.retire_work, HZ);
+		queue_delayed_work(dev_priv->wq, &dev_priv->mm.retire_work,
+				   round_jiffies_delay(HZ));
 		return;
 	}
 
@@ -2364,7 +2371,8 @@ i915_gem_retire_work_handler(struct work_struct *work)
 	}
 
 	if (!dev_priv->mm.suspended && !idle)
-		queue_delayed_work(dev_priv->wq, &dev_priv->mm.retire_work, HZ);
+		queue_delayed_work(dev_priv->wq, &dev_priv->mm.retire_work,
+				   round_jiffies_delay(HZ));
 	if (idle)
 		intel_mark_idle(dev);
 
-- 
1.7.10.4

Re: [PATCH 2/2] drm/i915: Align the retire_requests worker to the nearest second

[Arjan van de Ven]

On 10/5/2012 6:53 AM, Chris Wilson wrote:
> By using round_jiffies() we can align the wakeup of our worker to the
> nearest second in order to batch wakeups and reduce system load, which
> is useful for unimportant coarse tasks like our retire_requests.
> 
> Suggested-by: Arjan van de Ven <arjan@linux.intel.com>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Arjan van de Ven <arjan@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem.c |   14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 8e05d53..706f481 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2084,6 +2084,11 @@ i915_gem_next_request_seqno(struct intel_ring_buffer *ring)
>  	return ring->outstanding_lazy_request;
>  }
>  
> +static unsigned long round_jiffies_delay(unsigned long delay)
> +{
> +	return round_jiffies_relative(delay) - jiffies;
> +}

this is buggy


> +
>  int
>  i915_add_request(struct intel_ring_buffer *ring,
>  		 struct drm_file *file,
> @@ -2155,7 +2160,8 @@ i915_add_request(struct intel_ring_buffer *ring,
>  		}
>  		if (was_empty) {
>  			queue_delayed_work(dev_priv->wq,
> -					   &dev_priv->mm.retire_work, HZ);
> +					   &dev_priv->mm.retire_work,
> +					   round_jiffies_delay(HZ));

when used like this


round_jiffies() rounds absolute jiffies towards the next second

round_jiffies_relative() already subtracts jiffies from the result, like
the helper that you're trying to invent here does ;=)

doing that double up is a bad idea.

Re: [PATCH 1/2] drm/i915: Align the hangcheck wakeup to the nearest second

[Jani Nikula]

On Fri, 05 Oct 2012, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> round_jiffies() aligns the wakeup time to the nearest second in order to
> batch wakeups and reduce system load, which is useful for unimportant
> coarse timers like our hangcheck.
>
> Suggested-by: Arjan van de Ven <arjan@linux.intel.com>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Arjan van de Ven <arjan@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h |    1 +
>  drivers/gpu/drm/i915/i915_gem.c |    3 +--
>  drivers/gpu/drm/i915/i915_irq.c |    5 ++---
>  3 files changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index d8043af..f79c664 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -460,6 +460,7 @@ typedef struct drm_i915_private {
>  
>  	/* For hangcheck timer */
>  #define DRM_I915_HANGCHECK_PERIOD 1500 /* in ms */
> +#define DRM_I915_HANGCHECK_JIFFIES msecs_to_jiffies(DRM_I915_HANGCHECK_PERIOD)
>  	struct timer_list hangcheck_timer;
>  	int hangcheck_count;
>  	uint32_t last_acthd[I915_NUM_RINGS];
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index c78f8e3..8e05d53 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2151,8 +2151,7 @@ i915_add_request(struct intel_ring_buffer *ring,
>  	if (!dev_priv->mm.suspended) {
>  		if (i915_enable_hangcheck) {
>  			mod_timer(&dev_priv->hangcheck_timer,
> -				  jiffies +
> -				  msecs_to_jiffies(DRM_I915_HANGCHECK_PERIOD));
> +				  round_jiffies_relative(DRM_I915_HANGCHECK_JIFFIES));

What is DRM_I915_HANGCHECK_PERIOD based on; specifically is it a strict
minimum value? Should round_jiffies_*up*_relative() be used instead?

BR,
Jani.


>  		}
>  		if (was_empty) {
>  			queue_delayed_work(dev_priv->wq,
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index e18e56b..67dc487 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -353,8 +353,7 @@ static void notify_ring(struct drm_device *dev,
>  	if (i915_enable_hangcheck) {
>  		dev_priv->hangcheck_count = 0;
>  		mod_timer(&dev_priv->hangcheck_timer,
> -			  jiffies +
> -			  msecs_to_jiffies(DRM_I915_HANGCHECK_PERIOD));
> +			  round_jiffies_relative(DRM_I915_HANGCHECK_JIFFIES));
>  	}
>  }
>  
> @@ -1787,7 +1786,7 @@ void i915_hangcheck_elapsed(unsigned long data)
>  repeat:
>  	/* Reset timer case chip hangs without another request being added */
>  	mod_timer(&dev_priv->hangcheck_timer,
> -		  jiffies + msecs_to_jiffies(DRM_I915_HANGCHECK_PERIOD));
> +		  round_jiffies_relative(DRM_I915_HANGCHECK_JIFFIES));
>  }
>  
>  /* drm_dma.h hooks
> -- 
> 1.7.10.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm/i915: Align the hangcheck wakeup to the nearest second

[Chris Wilson]

On Fri, 05 Oct 2012 18:40:05 +0300, Jani Nikula <jani.nikula@linux.intel.com> wrote:
> On Fri, 05 Oct 2012, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > round_jiffies() aligns the wakeup time to the nearest second in order to
> > batch wakeups and reduce system load, which is useful for unimportant
> > coarse timers like our hangcheck.
> >
> > Suggested-by: Arjan van de Ven <arjan@linux.intel.com>
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Arjan van de Ven <arjan@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h |    1 +
> >  drivers/gpu/drm/i915/i915_gem.c |    3 +--
> >  drivers/gpu/drm/i915/i915_irq.c |    5 ++---
> >  3 files changed, 4 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index d8043af..f79c664 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -460,6 +460,7 @@ typedef struct drm_i915_private {
> >  
> >  	/* For hangcheck timer */
> >  #define DRM_I915_HANGCHECK_PERIOD 1500 /* in ms */
> > +#define DRM_I915_HANGCHECK_JIFFIES msecs_to_jiffies(DRM_I915_HANGCHECK_PERIOD)
> >  	struct timer_list hangcheck_timer;
> >  	int hangcheck_count;
> >  	uint32_t last_acthd[I915_NUM_RINGS];
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > index c78f8e3..8e05d53 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -2151,8 +2151,7 @@ i915_add_request(struct intel_ring_buffer *ring,
> >  	if (!dev_priv->mm.suspended) {
> >  		if (i915_enable_hangcheck) {
> >  			mod_timer(&dev_priv->hangcheck_timer,
> > -				  jiffies +
> > -				  msecs_to_jiffies(DRM_I915_HANGCHECK_PERIOD));
> > +				  round_jiffies_relative(DRM_I915_HANGCHECK_JIFFIES));
> 
> What is DRM_I915_HANGCHECK_PERIOD based on; specifically is it a strict
> minimum value? Should round_jiffies_*up*_relative() be used instead?

It's a random value plucked out of the air for being long enough that
any typical batch will have completed and short enough that the user
doesn't turn the machine off in digust. Typically we repeat the
hangcheck as well to confirm that GPU is dead before starting the error
recovery. Rounding up is slightly preferable.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 2/2] drm/i915: Align the retire_requests worker to the nearest second

[Chris Wilson]

On Fri, 05 Oct 2012 08:18:17 -0700, Arjan van de Ven <arjan@linux.intel.com> wrote:
> On 10/5/2012 6:53 AM, Chris Wilson wrote:
> > By using round_jiffies() we can align the wakeup of our worker to the
> > nearest second in order to batch wakeups and reduce system load, which
> > is useful for unimportant coarse tasks like our retire_requests.
> > 
> > Suggested-by: Arjan van de Ven <arjan@linux.intel.com>
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Arjan van de Ven <arjan@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_gem.c |   14 +++++++++++---
> >  1 file changed, 11 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > index 8e05d53..706f481 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -2084,6 +2084,11 @@ i915_gem_next_request_seqno(struct intel_ring_buffer *ring)
> >  	return ring->outstanding_lazy_request;
> >  }
> >  
> > +static unsigned long round_jiffies_delay(unsigned long delay)
> > +{
> > +	return round_jiffies_relative(delay) - jiffies;
> > +}
> 
> this is buggy
> 
> 
> > +
> >  int
> >  i915_add_request(struct intel_ring_buffer *ring,
> >  		 struct drm_file *file,
> > @@ -2155,7 +2160,8 @@ i915_add_request(struct intel_ring_buffer *ring,
> >  		}
> >  		if (was_empty) {
> >  			queue_delayed_work(dev_priv->wq,
> > -					   &dev_priv->mm.retire_work, HZ);
> > +					   &dev_priv->mm.retire_work,
> > +					   round_jiffies_delay(HZ));
> 
> when used like this
> 
> 
> round_jiffies() rounds absolute jiffies towards the next second
> 
> round_jiffies_relative() already subtracts jiffies from the result, like
> the helper that you're trying to invent here does ;=)
> 
> doing that double up is a bad idea.

For some reason the example I read convinced me that
round_jiffies_relative() returned the absolute jiffie for the relative
delay so that we could put it straight into mod_timer().

Again we can use round up here as well.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH 1/2] drm/i915: Align the hangcheck wakeup to the nearest second

[Chris Wilson]

round_jiffies() aligns the wakeup time to the nearest second in order to
batch wakeups and reduce system load, which is useful for unimportant
coarse timers like our hangcheck.

v2: round_jiffies_relative() returns the relative jiffie value, whereas
we need the absolute value for the timer.

Suggested-by: Arjan van de Ven <arjan@linux.intel.com>
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Arjan van de Ven <arjan@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h |    1 +
 drivers/gpu/drm/i915/i915_gem.c |    3 +--
 drivers/gpu/drm/i915/i915_irq.c |    5 ++---
 3 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index d8043af..f79c664 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -460,6 +460,7 @@ typedef struct drm_i915_private {
 
 	/* For hangcheck timer */
 #define DRM_I915_HANGCHECK_PERIOD 1500 /* in ms */
+#define DRM_I915_HANGCHECK_JIFFIES msecs_to_jiffies(DRM_I915_HANGCHECK_PERIOD)
 	struct timer_list hangcheck_timer;
 	int hangcheck_count;
 	uint32_t last_acthd[I915_NUM_RINGS];
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index c78f8e3..5639ac7 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2151,8 +2151,7 @@ i915_add_request(struct intel_ring_buffer *ring,
 	if (!dev_priv->mm.suspended) {
 		if (i915_enable_hangcheck) {
 			mod_timer(&dev_priv->hangcheck_timer,
-				  jiffies +
-				  msecs_to_jiffies(DRM_I915_HANGCHECK_PERIOD));
+				  round_jiffies_up(jiffies + DRM_I915_HANGCHECK_JIFFIES));
 		}
 		if (was_empty) {
 			queue_delayed_work(dev_priv->wq,
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index e18e56b..943e67b 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -353,8 +353,7 @@ static void notify_ring(struct drm_device *dev,
 	if (i915_enable_hangcheck) {
 		dev_priv->hangcheck_count = 0;
 		mod_timer(&dev_priv->hangcheck_timer,
-			  jiffies +
-			  msecs_to_jiffies(DRM_I915_HANGCHECK_PERIOD));
+			  round_jiffies_up(jiffies + DRM_I915_HANGCHECK_JIFFIES));
 	}
 }
 
@@ -1787,7 +1786,7 @@ void i915_hangcheck_elapsed(unsigned long data)
 repeat:
 	/* Reset timer case chip hangs without another request being added */
 	mod_timer(&dev_priv->hangcheck_timer,
-		  jiffies + msecs_to_jiffies(DRM_I915_HANGCHECK_PERIOD));
+		  round_jiffies_up(jiffies + DRM_I915_HANGCHECK_JIFFIES));
 }
 
 /* drm_dma.h hooks
-- 
1.7.10.4

[PATCH 2/2] drm/i915: Align the retire_requests worker to the nearest second

[Chris Wilson]

By using round_jiffies() we can align the wakeup of our worker to the
nearest second in order to batch wakeups and reduce system load, which
is useful for unimportant coarse tasks like our retire_requests.

v2: round_jiffies_relative() already returns the relative timeout value,
so no need to incorrectly perform the subtraction twice. The timer
interface still leaves the possibility for the value of jiffies to
change be we program the timer.

Suggested-by: Arjan van de Ven <arjan@linux.intel.com>
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Arjan van de Ven <arjan@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c |    9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 5639ac7..586c2a2 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2155,7 +2155,8 @@ i915_add_request(struct intel_ring_buffer *ring,
 		}
 		if (was_empty) {
 			queue_delayed_work(dev_priv->wq,
-					   &dev_priv->mm.retire_work, HZ);
+					   &dev_priv->mm.retire_work,
+					   round_jiffies_up_relative(HZ));
 			intel_mark_busy(dev_priv->dev);
 		}
 	}
@@ -2346,7 +2347,8 @@ i915_gem_retire_work_handler(struct work_struct *work)
 
 	/* Come back later if the device is busy... */
 	if (!mutex_trylock(&dev->struct_mutex)) {
-		queue_delayed_work(dev_priv->wq, &dev_priv->mm.retire_work, HZ);
+		queue_delayed_work(dev_priv->wq, &dev_priv->mm.retire_work,
+				   round_jiffies_up_relative(HZ));
 		return;
 	}
 
@@ -2364,7 +2366,8 @@ i915_gem_retire_work_handler(struct work_struct *work)
 	}
 
 	if (!dev_priv->mm.suspended && !idle)
-		queue_delayed_work(dev_priv->wq, &dev_priv->mm.retire_work, HZ);
+		queue_delayed_work(dev_priv->wq, &dev_priv->mm.retire_work,
+				   round_jiffies_up_relative(HZ));
 	if (idle)
 		intel_mark_idle(dev);
 
-- 
1.7.10.4

Re: [PATCH 2/2] drm/i915: Align the retire_requests worker to the nearest second

[Jani Nikula]

On Fri, 05 Oct 2012, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> By using round_jiffies() we can align the wakeup of our worker to the
> nearest second in order to batch wakeups and reduce system load, which
> is useful for unimportant coarse tasks like our retire_requests.

Is there a reason not to just use INIT_DELAYED_WORK_DEFERRABLE()? Come
to think of it, same with deferrable timer in patch 1/2.

> Suggested-by: Arjan van de Ven <arjan@linux.intel.com>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Arjan van de Ven <arjan@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem.c |   14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 8e05d53..706f481 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2084,6 +2084,11 @@ i915_gem_next_request_seqno(struct intel_ring_buffer *ring)
>  	return ring->outstanding_lazy_request;
>  }
>  
> +static unsigned long round_jiffies_delay(unsigned long delay)
> +{
> +	return round_jiffies_relative(delay) - jiffies;
> +}

Hmm, is it possible that would end up negative if someone reuses that
with a small delay?

An observation: there's a bunch of calls elsewhere in kernel to
queue_delayed_work() with the delay wrapped in round_jiffies() or
round_jiffies_relative(). The former at least gets queued within
expected tolerance (though likely not on full second), but how could the
code using the latter ever work?!

I guess a function like yours could be useful in generic code.

BR,
Jani.

> +
>  int
>  i915_add_request(struct intel_ring_buffer *ring,
>  		 struct drm_file *file,
> @@ -2155,7 +2160,8 @@ i915_add_request(struct intel_ring_buffer *ring,
>  		}
>  		if (was_empty) {
>  			queue_delayed_work(dev_priv->wq,
> -					   &dev_priv->mm.retire_work, HZ);
> +					   &dev_priv->mm.retire_work,
> +					   round_jiffies_delay(HZ));
>  			intel_mark_busy(dev_priv->dev);
>  		}
>  	}
> @@ -2346,7 +2352,8 @@ i915_gem_retire_work_handler(struct work_struct *work)
>  
>  	/* Come back later if the device is busy... */
>  	if (!mutex_trylock(&dev->struct_mutex)) {
> -		queue_delayed_work(dev_priv->wq, &dev_priv->mm.retire_work, HZ);
> +		queue_delayed_work(dev_priv->wq, &dev_priv->mm.retire_work,
> +				   round_jiffies_delay(HZ));
>  		return;
>  	}
>  
> @@ -2364,7 +2371,8 @@ i915_gem_retire_work_handler(struct work_struct *work)
>  	}
>  
>  	if (!dev_priv->mm.suspended && !idle)
> -		queue_delayed_work(dev_priv->wq, &dev_priv->mm.retire_work, HZ);
> +		queue_delayed_work(dev_priv->wq, &dev_priv->mm.retire_work,
> +				   round_jiffies_delay(HZ));
>  	if (idle)
>  		intel_mark_idle(dev);
>  
> -- 
> 1.7.10.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm/i915: Align the hangcheck wakeup to the nearest second

[Jani Nikula]

On Fri, 05 Oct 2012, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> round_jiffies() aligns the wakeup time to the nearest second in order to
> batch wakeups and reduce system load, which is useful for unimportant
> coarse timers like our hangcheck.
>
> v2: round_jiffies_relative() returns the relative jiffie value, whereas
> we need the absolute value for the timer.

On the series,

Reviewed-by: Jani Nikula <jani.nikula@intel.com>

> Suggested-by: Arjan van de Ven <arjan@linux.intel.com>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Arjan van de Ven <arjan@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h |    1 +
>  drivers/gpu/drm/i915/i915_gem.c |    3 +--
>  drivers/gpu/drm/i915/i915_irq.c |    5 ++---
>  3 files changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index d8043af..f79c664 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -460,6 +460,7 @@ typedef struct drm_i915_private {
>  
>  	/* For hangcheck timer */
>  #define DRM_I915_HANGCHECK_PERIOD 1500 /* in ms */
> +#define DRM_I915_HANGCHECK_JIFFIES msecs_to_jiffies(DRM_I915_HANGCHECK_PERIOD)
>  	struct timer_list hangcheck_timer;
>  	int hangcheck_count;
>  	uint32_t last_acthd[I915_NUM_RINGS];
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index c78f8e3..5639ac7 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2151,8 +2151,7 @@ i915_add_request(struct intel_ring_buffer *ring,
>  	if (!dev_priv->mm.suspended) {
>  		if (i915_enable_hangcheck) {
>  			mod_timer(&dev_priv->hangcheck_timer,
> -				  jiffies +
> -				  msecs_to_jiffies(DRM_I915_HANGCHECK_PERIOD));
> +				  round_jiffies_up(jiffies + DRM_I915_HANGCHECK_JIFFIES));
>  		}
>  		if (was_empty) {
>  			queue_delayed_work(dev_priv->wq,
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index e18e56b..943e67b 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -353,8 +353,7 @@ static void notify_ring(struct drm_device *dev,
>  	if (i915_enable_hangcheck) {
>  		dev_priv->hangcheck_count = 0;
>  		mod_timer(&dev_priv->hangcheck_timer,
> -			  jiffies +
> -			  msecs_to_jiffies(DRM_I915_HANGCHECK_PERIOD));
> +			  round_jiffies_up(jiffies + DRM_I915_HANGCHECK_JIFFIES));
>  	}
>  }
>  
> @@ -1787,7 +1786,7 @@ void i915_hangcheck_elapsed(unsigned long data)
>  repeat:
>  	/* Reset timer case chip hangs without another request being added */
>  	mod_timer(&dev_priv->hangcheck_timer,
> -		  jiffies + msecs_to_jiffies(DRM_I915_HANGCHECK_PERIOD));
> +		  round_jiffies_up(jiffies + DRM_I915_HANGCHECK_JIFFIES));
>  }
>  
>  /* drm_dma.h hooks
> -- 
> 1.7.10.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm/i915: Align the hangcheck wakeup to the nearest second

[Daniel Vetter]

On Mon, Oct 08, 2012 at 01:59:58PM +0300, Jani Nikula wrote:
> On Fri, 05 Oct 2012, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > round_jiffies() aligns the wakeup time to the nearest second in order to
> > batch wakeups and reduce system load, which is useful for unimportant
> > coarse timers like our hangcheck.
> >
> > v2: round_jiffies_relative() returns the relative jiffie value, whereas
> > we need the absolute value for the timer.
> 
> On the series,
> 
> Reviewed-by: Jani Nikula <jani.nikula@intel.com>

Both patches merged to dinq, thanks.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch