On Wed, Dec 07, 2016 at 05:41:39PM +0000, Chris Wilson wrote:
> 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.
Hmm. Yeah I guess we could try to do this in a different place.
> -Chris
>
> --
> Chris Wilson, Intel Open Source Technology Centre
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
