[Intel-gfx] [PATCH 2/2] drm/i915: Reorganise legacy context switch to cope with late failure

[Chris Wilson]

On Wed, Apr 13, 2016 at 05:05:05PM +0200, Daniel Vetter wrote:
> >  static bool
> > -needs_pd_load_post(struct intel_engine_cs *engine, struct intel_context *to,
> > -		   u32 hw_flags)
> > +needs_pd_load_post(struct intel_context *to, u32 hw_flags)
> >  {
> > -	struct drm_i915_private *dev_priv = engine->dev->dev_private;
> > -
> >  	if (!to->ppgtt)
> >  		return false;
> >  
> > -	if (!IS_GEN8(engine->dev))
> > -		return false;
> > -
> > -	if (engine != &dev_priv->engine[RCS])
> > +	if (!IS_GEN8(to->i915))
> >  		return false;
> >  
> >  	if (hw_flags & MI_RESTORE_INHIBIT)
> 
> Above hunk might be better in the previous patch, but meh.

Previous patch changed needs_pd_load_pre() :-p

> > @@ -667,12 +662,11 @@ static int do_rcs_switch(struct drm_i915_gem_request *req)
> >  {
> >  	struct intel_context *to = req->ctx;
> >  	struct intel_engine_cs *engine = req->engine;
> > -	struct intel_context *from = engine->last_context;
> > -	u32 hw_flags = 0;
> > -	bool uninitialized = false;
> > +	struct intel_context *from;
> > +	u32 hw_flags;
> >  	int ret, i;
> >  
> > -	if (skip_rcs_switch(engine, from, to))
> > +	if (skip_rcs_switch(engine->last_context, to))
> >  		return 0;
> >  
> >  	/* Trying to pin first makes error handling easier. */
> > @@ -686,9 +680,23 @@ static int do_rcs_switch(struct drm_i915_gem_request *req)
> >  	 * Pin can switch back to the default context if we end up calling into
> >  	 * evict_everything - as a last ditch gtt defrag effort that also
> >  	 * switches to the default context. Hence we need to reload from here.
> > +	 *
> > +	 * XXX: Doing so is painfully broken!
> 
> Why the XXX addition here? I thought the point of this patch is to fix
> this ...

No, that requires my seqno->requests patches....

To fix this we have to move the context pinning into request allocation,
so that if we do have to emit a switch to the default context we do so
before we take ownership of the ring for ourselves.

  	/* Trying to pin first makes error handling easier. */

is not kidding.

> >  	 */
> >  	from = engine->last_context;
> >  
> > +	/*
> > +	 * Clear this page out of any CPU caches for coherent swap-in/out. Note
> > +	 * that thanks to write = false in this call and us not setting any gpu
> > +	 * write domains when putting a context object onto the active list
> > +	 * (when switching away from it), this won't block.
> > +	 *
> > +	 * XXX: We need a real interface to do this instead of trickery.
> > +	 */
> > +	ret = i915_gem_object_set_to_gtt_domain(to->legacy_hw_ctx.rcs_state, false);
> > +	if (ret)
> > +		goto unpin_out;
> > +
> >  	if (needs_pd_load_pre(engine, to)) {
> >  		/* Older GENs and non render rings still want the load first,
> >  		 * "PP_DCLV followed by PP_DIR_BASE register through Load
> > @@ -700,70 +708,36 @@ static int do_rcs_switch(struct drm_i915_gem_request *req)
> >  			goto unpin_out;
> >  
> >  		/* Doing a PD load always reloads the page dirs */
> > -		to->ppgtt->pd_dirty_rings &= ~intel_engine_flag(engine);
> > +		to->ppgtt->pd_dirty_rings &= ~(1 << RCS);
> 
> Superflous change for the imo worse.

* shrug. I was just trying to reinforce this was RCS, not any random
engine.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre