Re: [PATCH 03/11] drm/i915: Restore dev_priv->mm.interruptible for overlay

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > 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@chris-wilson.co.uk>
> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx