On Wed, Dec 07, 2016 at 07:28:05PM +0200, ville.syrjala@xxxxxxxxxxxxxxx wrote:
> From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
>
> We need an uninterruptible wait for the overlay off request during
> modeset. Restore the operation of dev_priv->mm.interruptible
> sufficiently for that.
>
> Toss in a WARN_ON() to make sure the request succeeds.
>
> Fixes: 7da844c5c6fc ("drm/i915: Move the special case wait-request handling to its one caller")
> Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> Cc: Joonas Lahtinen <joonas.lahtinen@xxxxxxxxxxxxxxx>
> Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> ---
> drivers/gpu/drm/i915/i915_gem_request.c | 36 +++++++++++++++++++++++++++++++++
> drivers/gpu/drm/i915/i915_gem_request.h | 35 ++------------------------------
> drivers/gpu/drm/i915/intel_display.c | 2 +-
> 3 files changed, 39 insertions(+), 34 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_request.c b/drivers/gpu/drm/i915/i915_gem_request.c
> index fcf22b0e2967..1a7b88166c51 100644
> --- a/drivers/gpu/drm/i915/i915_gem_request.c
> +++ b/drivers/gpu/drm/i915/i915_gem_request.c
> @@ -1199,3 +1199,39 @@ void i915_gem_retire_requests(struct drm_i915_private *dev_priv)
> for_each_engine(engine, dev_priv, id)
> engine_retire_requests(engine);
> }
> +
> +/**
> + * i915_gem_active_retire - waits until the request is retired
> + * @active - the active request on which to wait
> + *
> + * i915_gem_active_retire() waits until the request is completed,
> + * and then ensures that at least the retirement handler for this
> + * @active tracker is called before returning. If the @active
> + * tracker is idle, the function returns immediately.
> + */
> +int __must_check
> +i915_gem_active_retire(struct i915_gem_active *active,
> + struct mutex *mutex)
> +{
> + struct drm_i915_gem_request *request;
> + long ret;
> +
> + request = i915_gem_active_raw(active, mutex);
> + if (!request)
> + return 0;
> +
> + ret = i915_wait_request(request,
> + (request->i915->mm.interruptible ?
> + I915_WAIT_INTERRUPTIBLE : 0) |
> + I915_WAIT_LOCKED,
> + MAX_SCHEDULE_TIMEOUT);
At least pass in the flags.
> + if (ret < 0)
> + return ret;
> +
> + list_del_init(&active->link);
> + RCU_INIT_POINTER(active->request, NULL);
> +
> + active->retire(active, request);
> +
> + return 0;
> +}
> diff --git a/drivers/gpu/drm/i915/i915_gem_request.h b/drivers/gpu/drm/i915/i915_gem_request.h
> index e2b077df2da0..09add6b9cfc7 100644
> --- a/drivers/gpu/drm/i915/i915_gem_request.h
> +++ b/drivers/gpu/drm/i915/i915_gem_request.h
> @@ -657,39 +657,8 @@ i915_gem_active_wait(const struct i915_gem_active *active, unsigned int flags)
> return ret < 0 ? ret : 0;
> }
>
> -/**
> - * i915_gem_active_retire - waits until the request is retired
> - * @active - the active request on which to wait
> - *
> - * i915_gem_active_retire() waits until the request is completed,
> - * and then ensures that at least the retirement handler for this
> - * @active tracker is called before returning. If the @active
> - * tracker is idle, the function returns immediately.
> - */
> -static inline int __must_check
> -i915_gem_active_retire(struct i915_gem_active *active,
> - struct mutex *mutex)
> -{
> - struct drm_i915_gem_request *request;
> - long ret;
> -
> - request = i915_gem_active_raw(active, mutex);
> - if (!request)
> - return 0;
> -
> - ret = i915_wait_request(request,
> - I915_WAIT_INTERRUPTIBLE | I915_WAIT_LOCKED,
> - MAX_SCHEDULE_TIMEOUT);
> - if (ret < 0)
> - return ret;
> -
> - list_del_init(&active->link);
> - RCU_INIT_POINTER(active->request, NULL);
> -
> - active->retire(active, request);
> -
> - return 0;
> -}
> +int __must_check i915_gem_active_retire(struct i915_gem_active *active,
> + struct mutex *mutex);
>
> #define for_each_active(mask, idx) \
> for (; mask ? idx = ffs(mask) - 1, 1 : 0; mask &= ~BIT(idx))
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index a41082e2750e..5a74991854e3 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4923,7 +4923,7 @@ static void intel_crtc_dpms_overlay_disable(struct intel_crtc *intel_crtc)
>
> mutex_lock(&dev->struct_mutex);
> dev_priv->mm.interruptible = false;
> - (void) intel_overlay_switch_off(intel_crtc->overlay);
> + WARN_ON(intel_overlay_switch_off(intel_crtc->overlay) != 0);
> dev_priv->mm.interruptible = true;
> mutex_unlock(&dev->struct_mutex);
Or we can push this wait to where it can interrupt, such as prepare_planes_fb.
(For intel_atomic_commit_tail, intel_crtc_disable_noatomic() should always
be a no-op.) And then we are down to just the shrinker running
uninterruptibly, just one last place to fix.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
