On Wed, Apr 13, 2016 at 03:16:30PM +0100, Chris Wilson wrote:
> Having the !RCS legacy context switch threaded through the RCS switching
> code makes it much harder to follow and understand. In the next patch, I
> want to fix a bug handling the incomplete switch, this is made much
> simpler if we segregate the two paths now.
>
> Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> Cc: Daniel Vetter <daniel@xxxxxxxx>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxxxxxxx>
> ---
> drivers/gpu/drm/i915/i915_gem_context.c | 74 +++++++++++++++++----------------
> 1 file changed, 38 insertions(+), 36 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index 453b655e86fc..c027240aacf3 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -609,9 +609,9 @@ mi_set_context(struct drm_i915_gem_request *req, u32 hw_flags)
> return ret;
> }
>
> -static inline bool should_skip_switch(struct intel_engine_cs *engine,
> - struct intel_context *from,
> - struct intel_context *to)
> +static inline bool skip_rcs_switch(struct intel_engine_cs *engine,
> + struct intel_context *from,
> + struct intel_context *to)
> {
> if (to->remap_slice)
> return false;
> @@ -626,15 +626,17 @@ static inline bool should_skip_switch(struct intel_engine_cs *engine,
> static bool
> needs_pd_load_pre(struct intel_engine_cs *engine, struct intel_context *to)
> {
> - struct drm_i915_private *dev_priv = engine->dev->dev_private;
> -
> if (!to->ppgtt)
> return false;
>
> - if (INTEL_INFO(engine->dev)->gen < 8)
> + if (engine->last_context == to &&
> + !(intel_engine_flag(engine) & to->ppgtt->pd_dirty_rings))
> + return false;
> +
> + if (engine->id != RCS)
> return true;
>
> - if (engine != &dev_priv->engine[RCS])
> + if (INTEL_INFO(engine->dev)->gen < 8)
> return true;
>
> return false;
> @@ -661,32 +663,24 @@ needs_pd_load_post(struct intel_engine_cs *engine, struct intel_context *to,
> return false;
> }
>
> -static int do_switch(struct drm_i915_gem_request *req)
> +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 drm_i915_private *dev_priv = req->i915;
> struct intel_context *from = engine->last_context;
> u32 hw_flags = 0;
> bool uninitialized = false;
> int ret, i;
>
> - if (from != NULL && engine == &dev_priv->engine[RCS]) {
> - BUG_ON(from->legacy_hw_ctx.rcs_state == NULL);
> - BUG_ON(!i915_gem_obj_is_pinned(from->legacy_hw_ctx.rcs_state));
> - }
> -
> - if (should_skip_switch(engine, from, to))
> + if (skip_rcs_switch(engine, from, to))
> return 0;
>
> /* Trying to pin first makes error handling easier. */
> - if (engine == &dev_priv->engine[RCS]) {
> - ret = i915_gem_obj_ggtt_pin(to->legacy_hw_ctx.rcs_state,
> - get_context_alignment(engine->dev),
> - 0);
> - if (ret)
> - return ret;
> - }
> + ret = i915_gem_obj_ggtt_pin(to->legacy_hw_ctx.rcs_state,
> + get_context_alignment(engine->dev),
> + 0);
> + if (ret)
> + return ret;
>
> /*
> * Pin can switch back to the default context if we end up calling into
> @@ -709,12 +703,6 @@ static int do_switch(struct drm_i915_gem_request *req)
> to->ppgtt->pd_dirty_rings &= ~intel_engine_flag(engine);
> }
>
> - if (engine != &dev_priv->engine[RCS]) {
> - if (from)
> - i915_gem_context_unreference(from);
> - goto done;
> - }
> -
> /*
> * 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
> @@ -802,7 +790,6 @@ static int do_switch(struct drm_i915_gem_request *req)
> uninitialized = !to->legacy_hw_ctx.initialized;
> to->legacy_hw_ctx.initialized = true;
>
> -done:
> i915_gem_context_reference(to);
> engine->last_context = to;
>
> @@ -817,8 +804,7 @@ done:
> return 0;
>
> unpin_out:
> - if (engine->id == RCS)
> - i915_gem_object_ggtt_unpin(to->legacy_hw_ctx.rcs_state);
> + i915_gem_object_ggtt_unpin(to->legacy_hw_ctx.rcs_state);
> return ret;
> }
>
> @@ -843,17 +829,33 @@ int i915_switch_context(struct drm_i915_gem_request *req)
> WARN_ON(i915.enable_execlists);
> WARN_ON(!mutex_is_locked(&dev_priv->dev->struct_mutex));
>
> - if (req->ctx->legacy_hw_ctx.rcs_state == NULL) { /* We have the fake context */
> - if (req->ctx != engine->last_context) {
> - i915_gem_context_reference(req->ctx);
> + if (engine->id != RCS ||
> + req->ctx->legacy_hw_ctx.rcs_state == NULL) {
> + struct intel_context *to = req->ctx;
> +
> + if (needs_pd_load_pre(engine, to)) {
Hm, I'd inline this condition now since it's a bit confusing if there's no
POST. Assuming I read code correctly it seems to boil down to to->ppgtt
(which reads a lot cleaner) for !RCS.
Either way (since this is just a bikeshed):
Reviewed-by: Daniel Vetter <daniel.vetter@xxxxxxxx>
> + int ret;
> +
> + trace_switch_mm(engine, to);
> + ret = to->ppgtt->switch_mm(to->ppgtt, req);
> + if (ret)
> + return ret;
> +
> + /* Doing a PD load always reloads the page dirs */
> + to->ppgtt->pd_dirty_rings &= ~intel_engine_flag(engine);
> + }
> +
> + if (to != engine->last_context) {
> + i915_gem_context_reference(to);
> if (engine->last_context)
> i915_gem_context_unreference(engine->last_context);
> - engine->last_context = req->ctx;
> + engine->last_context = to;
> }
> +
> return 0;
> }
>
> - return do_switch(req);
> + return do_rcs_switch(req);
> }
>
> static bool contexts_enabled(struct drm_device *dev)
> --
> 2.8.0.rc3
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
