| Message ID | 1411052315-22979-2-git-send-email-mika.kuoppala@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Sep 18, 2014 at 05:58:30PM +0300, Mika Kuoppala wrote: > We don't know in what shape the default context was before reset. > The reset also dropped our changes that were done in > ring->init_context. > > Mark our default context as uninitialized for it to be properly > setup up on reset recovery . The fundamental problem here is that context_init is not correctly split into init/init_hw (or init/enable) pairs. That is we simply do not attempt to reset the default state after reset via i915_gem_init_hw(). I would rather we brought rationality to the gem init/init_hw sequence first. -Chris
On Thu, Sep 18, 2014 at 04:36:47PM +0100, Chris Wilson wrote: > On Thu, Sep 18, 2014 at 05:58:30PM +0300, Mika Kuoppala wrote: > > We don't know in what shape the default context was before reset. > > The reset also dropped our changes that were done in > > ring->init_context. > > > > Mark our default context as uninitialized for it to be properly > > setup up on reset recovery . > > The fundamental problem here is that context_init is not correctly split > into init/init_hw (or init/enable) pairs. That is we simply do not > attempt to reset the default state after reset via i915_gem_init_hw(). I > would rather we brought rationality to the gem init/init_hw sequence > first. Yeah I agree with Chris that context setup doesn't follow our established split with _init doing the software setup (and doesn't touch the hw at all) and _init_hw doing the hw setup. _init is run at driver load, while _init_hw is run at driver load, resume, after gpu reset and maybe even from runtime pm code. But I don't mind if we do this after or before this series, whatever is less fuzz is fine with me. -Daniel
[snip] On Thu, Sep 18, 2014 at 07:58:30AM -0700, Mika Kuoppala wrote: > @@ -577,7 +596,7 @@ static int do_switch(struct intel_engine_cs *ring, > vma->bind_vma(vma, to->legacy_hw_ctx.rcs_state->cache_level, GLOBAL_BIND); > } > > - if (!to->legacy_hw_ctx.initialized || i915_gem_context_is_default(to)) > + if (!to->initialized || i915_gem_context_is_default(to)) > hw_flags |= MI_RESTORE_INHIBIT; > > ret = mi_set_context(ring, to, hw_flags); > @@ -618,26 +637,19 @@ static int do_switch(struct intel_engine_cs *ring, > /* obj is kept alive until the next request by its active ref */ > i915_gem_object_ggtt_unpin(from->legacy_hw_ctx.rcs_state); > i915_gem_context_unreference(from); > - } > > - uninitialized = !to->legacy_hw_ctx.initialized && from == NULL; > - to->legacy_hw_ctx.initialized = true; > + /* We inherit the state from the previous context */ > + to->initialized = true; > + } Regarding these two hunks, I may have asked this question before but if so I've forgotten the answer. Why do we want to set MI_RESTORE_INHIBIT when switching to the default context? Why do we want to inherit state from the previous context? I assumed that when switching to an initialized context we would always want to restore its last state. Thanks, Brad
"Volkin, Bradley D" <bradley.d.volkin@intel.com> writes: Hi Bradley, > [snip] > > On Thu, Sep 18, 2014 at 07:58:30AM -0700, Mika Kuoppala wrote: >> @@ -577,7 +596,7 @@ static int do_switch(struct intel_engine_cs *ring, >> vma->bind_vma(vma, to->legacy_hw_ctx.rcs_state->cache_level, GLOBAL_BIND); >> } >> >> - if (!to->legacy_hw_ctx.initialized || i915_gem_context_is_default(to)) >> + if (!to->initialized || i915_gem_context_is_default(to)) >> hw_flags |= MI_RESTORE_INHIBIT; >> >> ret = mi_set_context(ring, to, hw_flags); >> @@ -618,26 +637,19 @@ static int do_switch(struct intel_engine_cs *ring, >> /* obj is kept alive until the next request by its active ref */ >> i915_gem_object_ggtt_unpin(from->legacy_hw_ctx.rcs_state); >> i915_gem_context_unreference(from); >> - } >> >> - uninitialized = !to->legacy_hw_ctx.initialized && from == NULL; >> - to->legacy_hw_ctx.initialized = true; >> + /* We inherit the state from the previous context */ >> + to->initialized = true; >> + } > > Regarding these two hunks, I may have asked this question before but if > so I've forgotten the answer. Why do we want to set MI_RESTORE_INHIBIT > when switching to the default context? Why do we want to inherit state > from the previous context? > > I assumed that when switching to an initialized context we would always > want to restore its last state. > I would have been pondering the same questions. Perhaps it has something to do with the fact that default context should not assume anything about its previous state and thus userspace needs to initialize everything fully. And also to avoid the context switch performance hit on restoring. Ben might know answer. My plan is to experiment with making master copy of the first fully initialized state and use it as a read-only bo to initialize all subsequent contexts from. Atleast with the code, we could measure the perf impact. -Mika
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c index 0ba5c71..a03361c 100644 --- a/drivers/gpu/drm/i915/i915_debugfs.c +++ b/drivers/gpu/drm/i915/i915_debugfs.c @@ -176,7 +176,7 @@ describe_obj(struct seq_file *m, struct drm_i915_gem_object *obj) static void describe_ctx(struct seq_file *m, struct intel_context *ctx) { - seq_putc(m, ctx->legacy_hw_ctx.initialized ? 'I' : 'i'); + seq_putc(m, ctx->initialized ? 'I' : 'i'); seq_putc(m, ctx->remap_slice ? 'R' : 'r'); seq_putc(m, ' '); } diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 07dafa2..49b45ec 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -637,16 +637,16 @@ struct intel_context { /* Legacy ring buffer submission */ struct { struct drm_i915_gem_object *rcs_state; - bool initialized; } legacy_hw_ctx; /* Execlists */ - bool rcs_initialized; struct { struct drm_i915_gem_object *state; struct intel_ringbuffer *ringbuf; } engine[I915_NUM_RINGS]; + bool initialized; + struct list_head link; }; diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c index a5221d8..b479840 100644 --- a/drivers/gpu/drm/i915/i915_gem_context.c +++ b/drivers/gpu/drm/i915/i915_gem_context.c @@ -306,6 +306,8 @@ void i915_gem_context_reset(struct drm_device *dev) i915_gem_context_unreference(lctx); ring->last_context = NULL; } + + ring->default_context->initialized = false; } } @@ -515,13 +517,30 @@ mi_set_context(struct intel_engine_cs *ring, return ret; } +static void context_state_init(struct intel_engine_cs *ring, + struct intel_context *to) +{ + int ret; + + if (ring->init_context) { + ret = ring->init_context(ring); + if (ret) + DRM_ERROR("ring init context: %d\n", ret); + } + + if (ring->id == RCS) { + ret = i915_gem_render_state_init(ring); + if (ret) + DRM_ERROR("init render state: %d\n", ret); + } +} + static int do_switch(struct intel_engine_cs *ring, struct intel_context *to) { struct drm_i915_private *dev_priv = ring->dev->dev_private; struct intel_context *from = ring->last_context; u32 hw_flags = 0; - bool uninitialized = false; int ret, i; if (from != NULL && ring == &dev_priv->ring[RCS]) { @@ -577,7 +596,7 @@ static int do_switch(struct intel_engine_cs *ring, vma->bind_vma(vma, to->legacy_hw_ctx.rcs_state->cache_level, GLOBAL_BIND); } - if (!to->legacy_hw_ctx.initialized || i915_gem_context_is_default(to)) + if (!to->initialized || i915_gem_context_is_default(to)) hw_flags |= MI_RESTORE_INHIBIT; ret = mi_set_context(ring, to, hw_flags); @@ -618,26 +637,19 @@ static int do_switch(struct intel_engine_cs *ring, /* obj is kept alive until the next request by its active ref */ i915_gem_object_ggtt_unpin(from->legacy_hw_ctx.rcs_state); i915_gem_context_unreference(from); - } - uninitialized = !to->legacy_hw_ctx.initialized && from == NULL; - to->legacy_hw_ctx.initialized = true; + /* We inherit the state from the previous context */ + to->initialized = true; + } done: i915_gem_context_reference(to); ring->last_context = to; - if (uninitialized) { - if (ring->init_context) { - ret = ring->init_context(ring); - if (ret) - DRM_ERROR("ring init context: %d\n", ret); - } + if (!to->initialized) + context_state_init(ring, to); - ret = i915_gem_render_state_init(ring); - if (ret) - DRM_ERROR("init render state: %d\n", ret); - } + to->initialized = true; return 0; diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c index 803fc38..4899a3c 100644 --- a/drivers/gpu/drm/i915/intel_lrc.c +++ b/drivers/gpu/drm/i915/intel_lrc.c @@ -1744,7 +1744,7 @@ int intel_lr_context_deferred_create(struct intel_context *ctx, ring->status_page.obj = ctx_obj; } - if (ring->id == RCS && !ctx->rcs_initialized) { + if (ring->id == RCS && !ctx->initialized) { ret = intel_lr_context_render_state_init(ring, ctx); if (ret) { DRM_ERROR("Init render state failed: %d\n", ret); @@ -1753,7 +1753,7 @@ int intel_lr_context_deferred_create(struct intel_context *ctx, intel_destroy_ringbuffer_obj(ringbuf); goto error; } - ctx->rcs_initialized = true; + ctx->initialized = true; } return 0;
We don't know in what shape the default context was before reset. The reset also dropped our changes that were done in ring->init_context. Mark our default context as uninitialized for it to be properly setup up on reset recovery . Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com> --- drivers/gpu/drm/i915/i915_debugfs.c | 2 +- drivers/gpu/drm/i915/i915_drv.h | 4 ++-- drivers/gpu/drm/i915/i915_gem_context.c | 42 +++++++++++++++++++++------------ drivers/gpu/drm/i915/intel_lrc.c | 4 ++-- 4 files changed, 32 insertions(+), 20 deletions(-)