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

Arjan van de Ven
2012-10-05T15:18:19Z

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.