[Intel-gfx] [PATCH 01/15] drm/i915: Break busywaiting for requests on pending signals

[Tvrtko Ursulin]

On 29/11/15 08:47, Chris Wilson wrote:
> The busywait in __i915_spin_request() does not respect pending signals
> and so may consume the entire timeslice for the task instead of
> returning to userspace to handle the signal.
>
> In the worst case this could cause a delay in signal processing of 20ms,
> which would be a noticeable jitter in cursor tracking. If a higher
> resolution signal was being used, for example to provide fairness of a
> server timeslices between clients, we could expect to detect some
> unfairness between clients (i.e. some windows not updating as fast as
> others). This issue was noticed when inspecting a report of poor
> interactivity resulting from excessively high __i915_spin_request usage.
>
> Fixes regression from
> commit 2def4ad99befa25775dd2f714fdd4d92faec6e34 [v4.2]
> Author: Chris Wilson <chris at chris-wilson.co.uk>
> Date:   Tue Apr 7 16:20:41 2015 +0100
>
>       drm/i915: Optimistically spin for the request completion
>
> v2: Try to assess the impact of the bug
>
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Jens Axboe <axboe at kernel.dk>
> Cc; "Rogozhkin, Dmitry V" <dmitry.v.rogozhkin at intel.com>
> Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
> Cc: Tvrtko Ursulin <tvrtko.ursulin at linux.intel.com>
> Cc: Eero Tamminen <eero.t.tamminen at intel.com>
> Cc: "Rantala, Valtteri" <valtteri.rantala at intel.com>
> Cc: stable at kernel.vger.org
> ---
>   drivers/gpu/drm/i915/i915_gem.c | 13 ++++++++-----
>   1 file changed, 8 insertions(+), 5 deletions(-)

One more time,

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>

Regards,

Tvrtko

>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 33adc8f8ab20..87fc34f5899f 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -1146,7 +1146,7 @@ static bool missed_irq(struct drm_i915_private *dev_priv,
>   	return test_bit(ring->id, &dev_priv->gpu_error.missed_irq_rings);
>   }
>
> -static int __i915_spin_request(struct drm_i915_gem_request *req)
> +static int __i915_spin_request(struct drm_i915_gem_request *req, int state)
>   {
>   	unsigned long timeout;
>
> @@ -1158,6 +1158,9 @@ static int __i915_spin_request(struct drm_i915_gem_request *req)
>   		if (i915_gem_request_completed(req, true))
>   			return 0;
>
> +		if (signal_pending_state(state, current))
> +			break;
> +
>   		if (time_after_eq(jiffies, timeout))
>   			break;
>
> @@ -1197,6 +1200,7 @@ int __i915_wait_request(struct drm_i915_gem_request *req,
>   	struct drm_i915_private *dev_priv = dev->dev_private;
>   	const bool irq_test_in_progress =
>   		ACCESS_ONCE(dev_priv->gpu_error.test_irq_rings) & intel_ring_flag(ring);
> +	int state = interruptible ? TASK_INTERRUPTIBLE : TASK_UNINTERRUPTIBLE;
>   	DEFINE_WAIT(wait);
>   	unsigned long timeout_expire;
>   	s64 before, now;
> @@ -1221,7 +1225,7 @@ int __i915_wait_request(struct drm_i915_gem_request *req,
>   	before = ktime_get_raw_ns();
>
>   	/* Optimistic spin for the next jiffie before touching IRQs */
> -	ret = __i915_spin_request(req);
> +	ret = __i915_spin_request(req, state);
>   	if (ret == 0)
>   		goto out;
>
> @@ -1233,8 +1237,7 @@ int __i915_wait_request(struct drm_i915_gem_request *req,
>   	for (;;) {
>   		struct timer_list timer;
>
> -		prepare_to_wait(&ring->irq_queue, &wait,
> -				interruptible ? TASK_INTERRUPTIBLE : TASK_UNINTERRUPTIBLE);
> +		prepare_to_wait(&ring->irq_queue, &wait, state);
>
>   		/* We need to check whether any gpu reset happened in between
>   		 * the caller grabbing the seqno and now ... */
> @@ -1252,7 +1255,7 @@ int __i915_wait_request(struct drm_i915_gem_request *req,
>   			break;
>   		}
>
> -		if (interruptible && signal_pending(current)) {
> +		if (signal_pending_state(state, current)) {
>   			ret = -ERESTARTSYS;
>   			break;
>   		}
>