[Intel-gfx] [PATCH 1/2] drm/i915: Unwind incomplete legacy context switches

[Chris Wilson]

Quoting Mika Kuoppala (2017-11-21 16:25:23)
> Chris Wilson <chris at chris-wilson.co.uk> writes:
> 
> > The legacy context switch for ringbuffer submission is multistaged,
> > where each of those stages may fail. However, we were updating global
> > state after some stages, and so we had to force the incomplete request
> > to be submitted because we could not unwind. Save the global state
> > before performing the switches, and so enable us to unwind back to the
> > previous global state should any phase fail. We then must cancel the
> > request instead of submitting it should the construction fail.
> >
> > Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> > Cc: Mika Kuoppala <mika.kuoppala at linux.intel.com>
> > ---
> > -static int do_rcs_switch(struct drm_i915_gem_request *req)
> > +/**
> > + * i915_switch_context() - perform a GPU context switch.
> > + * @rq: request for which we'll execute the context switch
> > + *
> > + * The context life cycle is simple. The context refcount is incremented and
> > + * decremented by 1 and create and destroy. If the context is in use by the GPU,
> 
> s/and/on
> 
> > + * it will have a refcount > 1. This allows us to destroy the context abstract
> > + * object while letting the normal object tracking destroy the backing BO.
> > + *
> > + * This function should not be used in execlists mode.  Instead the context is
> > + * switched by writing to the ELSP and requests keep a reference to their
> > + * context.
> > + */
> > +int i915_switch_context(struct drm_i915_gem_request *rq)
> >  {
> > -     struct i915_gem_context *to = req->ctx;
> > -     struct intel_engine_cs *engine = req->engine;
> > -     struct i915_hw_ppgtt *ppgtt = to->ppgtt ?: req->i915->mm.aliasing_ppgtt;
> > -     struct i915_gem_context *from = engine->legacy_active_context;
> > -     u32 hw_flags;
> > +     struct intel_engine_cs *engine = rq->engine;
> > +     struct i915_gem_context *to = rq->ctx;
> > +     struct i915_hw_ppgtt *ppgtt = to->ppgtt ?: rq->i915->mm.aliasing_ppgtt;
> > +     struct i915_gem_context *saved_ctx = engine->legacy_active_context;
> > +     struct i915_hw_ppgtt *saved_mm = engine->legacy_active_ppgtt;
> 
> saved as in context of this function or saved as in saved by hardware?
> Just trying to get behind the naming here.

saved on stack.

The naming we've used elsewhere (ye olde save and restore across
suspend, around setcrtc etc).

> 
> I am not in bottom of this patch yet but at this point,
> 
> new_[mm|ctx] and current[mm|ctx] feel more natural.

[snip]
 
> > -             engine->legacy_active_context = to;
> > -             return 0;
> > +             to->remap_slice = 0;
> 
> Because we have unwind we can just remap again everything if error
> happens?

Right. Now since we cancel the request, the changes up to this point do
not exist on error, so we revert back to the previous state.

(Note this still imposes an order that needs magic if we switch to
virtual ringbuffers and reordering...)
-Chris