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

[Jani Nikula <jani.nikula@linux.intel.com>]

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