| Message ID | 1464176933-31067-3-git-send-email-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On ke, 2016-05-25 at 12:48 +0100, Chris Wilson wrote: > The kernel context exists simply as a placeholder and should never be > executed with a render context. It does not need the golden render > state, as that will always be applied to a user context. By skipping the > initialisation we can avoid issues in attempting to program the golden > render context when trying to make the hardware idle. > > Testcase: igt/drm_module_reload_basic #byt > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=95634 > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > --- > drivers/gpu/drm/i915/i915_gem_context.c | 45 +++++++++++++++++++++++---------- > 1 file changed, 32 insertions(+), 13 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c > index a3b11aac23a4..3e3acd054f05 100644 > --- a/drivers/gpu/drm/i915/i915_gem_context.c > +++ b/drivers/gpu/drm/i915/i915_gem_context.c > @@ -413,7 +413,8 @@ int i915_gem_context_init(struct drm_device *dev) > return PTR_ERR(ctx); > } > > - if (!i915.enable_execlists && ctx->engine[RCS].state) { > + if (!i915.enable_execlists) { > + struct intel_engine_cs *engine; > int ret; > > /* We may need to do things with the shrinker which > @@ -423,13 +424,28 @@ int i915_gem_context_init(struct drm_device *dev) > * be available. To avoid this we always pin the default > * context. > */ > - ret = i915_gem_obj_ggtt_pin(ctx->engine[RCS].state, > - get_context_alignment(dev_priv), 0); > - if (ret) { > - DRM_ERROR("Failed to pinned default global context (error %d)\n", > - ret); > - i915_gem_context_unreference(ctx); > - return ret; > + for_each_engine(engine, dev_priv) { > + struct intel_context *ce = &ctx->engine[engine->id]; > + > + ret = 0; > + if (ce->state) Ugh, rather have one level of indent and teardown separated, it'll be easier to read and extend yadda yadda... > + ret = i915_gem_obj_ggtt_pin(ce->state, > + get_context_alignment(dev_priv), 0); > + if (ret) { > + DRM_ERROR("Failed to pinned default global context (error %d)\n", s/pinned/pin/ > + ret); > + i915_gem_context_unreference(ctx); > + return ret; > + } > + > + /* The kernel context is only used as a placeholder > + * for flushing the active context. It is never used > + * for submitting rendering and as such never requires > + * the golden render context, and so we can skip > + * emitting it when we switch to the kernel context > + * (during eviction). > + */ > + ce->initialised = true; > } > } > > @@ -452,13 +468,16 @@ void i915_gem_context_lost(struct drm_i915_private *dev_priv) > i915_gem_context_unpin(engine->last_context, engine); > engine->last_context = NULL; > } > - > - /* Force the GPU state to be reinitialised on enabling */ > - dev_priv->kernel_context->engine[engine->id].initialised = > - engine->init_context == NULL; > } > > - /* Force the GPU state to be reinitialised on enabling */ > + /* Force the GPU state to be restore on enabling */ s/restore/restored/ > + for_each_engine(engine, dev_priv) { > + struct intel_context *ce = > + &dev_priv->kernel_context->engine[engine->id]; > + > + ce->initialised = > + !i915.enable_execlists || engine->init_context == NULL; > + } <NL> here? > dev_priv->kernel_context->remap_slice = ALL_L3_SLICES(dev_priv); > } >
On Wed, May 25, 2016 at 12:48:49PM +0100, Chris Wilson wrote: > The kernel context exists simply as a placeholder and should never be > executed with a render context. It does not need the golden render > state, as that will always be applied to a user context. By skipping the > initialisation we can avoid issues in attempting to program the golden > render context when trying to make the hardware idle. > > Testcase: igt/drm_module_reload_basic #byt > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=95634 > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > --- > drivers/gpu/drm/i915/i915_gem_context.c | 45 +++++++++++++++++++++++---------- > 1 file changed, 32 insertions(+), 13 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c > index a3b11aac23a4..3e3acd054f05 100644 > --- a/drivers/gpu/drm/i915/i915_gem_context.c > +++ b/drivers/gpu/drm/i915/i915_gem_context.c > @@ -413,7 +413,8 @@ int i915_gem_context_init(struct drm_device *dev) > return PTR_ERR(ctx); > } > > - if (!i915.enable_execlists && ctx->engine[RCS].state) { > + if (!i915.enable_execlists) { > + struct intel_engine_cs *engine; > int ret; > > /* We may need to do things with the shrinker which > @@ -423,13 +424,28 @@ int i915_gem_context_init(struct drm_device *dev) > * be available. To avoid this we always pin the default > * context. > */ > - ret = i915_gem_obj_ggtt_pin(ctx->engine[RCS].state, > - get_context_alignment(dev_priv), 0); > - if (ret) { > - DRM_ERROR("Failed to pinned default global context (error %d)\n", > - ret); > - i915_gem_context_unreference(ctx); > - return ret; > + for_each_engine(engine, dev_priv) { for_each_engine! Too early, we haven't yet initialised the engines. The problem of trying to make it look neat. -Chris
On Wed, May 25, 2016 at 03:38:44PM +0300, Joonas Lahtinen wrote: > On ke, 2016-05-25 at 12:48 +0100, Chris Wilson wrote: > > + for_each_engine(engine, dev_priv) { > > + struct intel_context *ce = > > + &dev_priv->kernel_context->engine[engine->id]; > > + > > + ce->initialised = > > + !i915.enable_execlists || engine->init_context == NULL; > > + } > > <NL> here? Not sure. For, keep it tight with the comment and similar to the code it replaced. Against, whitespace is good. It's moot as this line disappears in a couple of patches time. I preferred trying to show the semantic similarity in the patch. -Chris
Chris Wilson <chris@chris-wilson.co.uk> writes: > [ text/plain ] > The kernel context exists simply as a placeholder and should never be > executed with a render context. It does not need the golden render > state, as that will always be applied to a user context. By skipping the > initialisation we can avoid issues in attempting to program the golden > render context when trying to make the hardware idle. > > Testcase: igt/drm_module_reload_basic #byt > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=95634 > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > --- > drivers/gpu/drm/i915/i915_gem_context.c | 45 +++++++++++++++++++++++---------- > 1 file changed, 32 insertions(+), 13 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c > index a3b11aac23a4..3e3acd054f05 100644 > --- a/drivers/gpu/drm/i915/i915_gem_context.c > +++ b/drivers/gpu/drm/i915/i915_gem_context.c > @@ -413,7 +413,8 @@ int i915_gem_context_init(struct drm_device *dev) > return PTR_ERR(ctx); > } > > - if (!i915.enable_execlists && ctx->engine[RCS].state) { > + if (!i915.enable_execlists) { > + struct intel_engine_cs *engine; > int ret; > > /* We may need to do things with the shrinker which > @@ -423,13 +424,28 @@ int i915_gem_context_init(struct drm_device *dev) > * be available. To avoid this we always pin the default > * context. > */ > - ret = i915_gem_obj_ggtt_pin(ctx->engine[RCS].state, > - get_context_alignment(dev_priv), 0); > - if (ret) { > - DRM_ERROR("Failed to pinned default global context (error %d)\n", > - ret); > - i915_gem_context_unreference(ctx); > - return ret; > + for_each_engine(engine, dev_priv) { > + struct intel_context *ce = &ctx->engine[engine->id]; > + > + ret = 0; > + if (ce->state) > + ret = i915_gem_obj_ggtt_pin(ce->state, > + get_context_alignment(dev_priv), 0); > + if (ret) { > + DRM_ERROR("Failed to pinned default global context (error %d)\n", > + ret); > + i915_gem_context_unreference(ctx); > + return ret; > + } > + > + /* The kernel context is only used as a placeholder > + * for flushing the active context. It is never used > + * for submitting rendering and as such never requires > + * the golden render context, and so we can skip > + * emitting it when we switch to the kernel context > + * (during eviction). > + */ > + ce->initialised = true; My concern here is that the bdw had/has problem with entering and exiting from rc6 with a vanilla hw state. And if you have not submitted anything, you need a working context to sleep on. Thus golden context was created to be that working one to sleep and wakeup on. -Mika > } > } > > @@ -452,13 +468,16 @@ void i915_gem_context_lost(struct drm_i915_private *dev_priv) > i915_gem_context_unpin(engine->last_context, engine); > engine->last_context = NULL; > } > - > - /* Force the GPU state to be reinitialised on enabling */ > - dev_priv->kernel_context->engine[engine->id].initialised = > - engine->init_context == NULL; > } > > - /* Force the GPU state to be reinitialised on enabling */ > + /* Force the GPU state to be restore on enabling */ > + for_each_engine(engine, dev_priv) { > + struct intel_context *ce = > + &dev_priv->kernel_context->engine[engine->id]; > + > + ce->initialised = > + !i915.enable_execlists || engine->init_context == NULL; > + } > dev_priv->kernel_context->remap_slice = ALL_L3_SLICES(dev_priv); > } > > -- > 2.8.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Wed, May 25, 2016 at 05:16:56PM +0300, Mika Kuoppala wrote: > Chris Wilson <chris@chris-wilson.co.uk> writes: > > > [ text/plain ] > > The kernel context exists simply as a placeholder and should never be > > executed with a render context. It does not need the golden render > > state, as that will always be applied to a user context. By skipping the > > initialisation we can avoid issues in attempting to program the golden > > render context when trying to make the hardware idle. > > > > Testcase: igt/drm_module_reload_basic #byt > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=95634 > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > --- > > drivers/gpu/drm/i915/i915_gem_context.c | 45 +++++++++++++++++++++++---------- > > 1 file changed, 32 insertions(+), 13 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c > > index a3b11aac23a4..3e3acd054f05 100644 > > --- a/drivers/gpu/drm/i915/i915_gem_context.c > > +++ b/drivers/gpu/drm/i915/i915_gem_context.c > > @@ -413,7 +413,8 @@ int i915_gem_context_init(struct drm_device *dev) > > return PTR_ERR(ctx); > > } > > > > - if (!i915.enable_execlists && ctx->engine[RCS].state) { > > + if (!i915.enable_execlists) { > > + struct intel_engine_cs *engine; > > int ret; > > > > /* We may need to do things with the shrinker which > > @@ -423,13 +424,28 @@ int i915_gem_context_init(struct drm_device *dev) > > * be available. To avoid this we always pin the default > > * context. > > */ > > - ret = i915_gem_obj_ggtt_pin(ctx->engine[RCS].state, > > - get_context_alignment(dev_priv), 0); > > - if (ret) { > > - DRM_ERROR("Failed to pinned default global context (error %d)\n", > > - ret); > > - i915_gem_context_unreference(ctx); > > - return ret; > > + for_each_engine(engine, dev_priv) { > > + struct intel_context *ce = &ctx->engine[engine->id]; > > + > > + ret = 0; > > + if (ce->state) > > + ret = i915_gem_obj_ggtt_pin(ce->state, > > + get_context_alignment(dev_priv), 0); > > + if (ret) { > > + DRM_ERROR("Failed to pinned default global context (error %d)\n", > > + ret); > > + i915_gem_context_unreference(ctx); > > + return ret; > > + } > > + > > + /* The kernel context is only used as a placeholder > > + * for flushing the active context. It is never used > > + * for submitting rendering and as such never requires > > + * the golden render context, and so we can skip > > + * emitting it when we switch to the kernel context > > + * (during eviction). > > + */ > > + ce->initialised = true; > > My concern here is that the bdw had/has problem with entering and > exiting from rc6 with a vanilla hw state. And if you have not > submitted anything, you need a working context to sleep on. This patch doesn't alter the fact that no context is set prior to rc6 being enabled. Nor have we been setting one for bdw for almost 2 years. -Chris
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c index a3b11aac23a4..3e3acd054f05 100644 --- a/drivers/gpu/drm/i915/i915_gem_context.c +++ b/drivers/gpu/drm/i915/i915_gem_context.c @@ -413,7 +413,8 @@ int i915_gem_context_init(struct drm_device *dev) return PTR_ERR(ctx); } - if (!i915.enable_execlists && ctx->engine[RCS].state) { + if (!i915.enable_execlists) { + struct intel_engine_cs *engine; int ret; /* We may need to do things with the shrinker which @@ -423,13 +424,28 @@ int i915_gem_context_init(struct drm_device *dev) * be available. To avoid this we always pin the default * context. */ - ret = i915_gem_obj_ggtt_pin(ctx->engine[RCS].state, - get_context_alignment(dev_priv), 0); - if (ret) { - DRM_ERROR("Failed to pinned default global context (error %d)\n", - ret); - i915_gem_context_unreference(ctx); - return ret; + for_each_engine(engine, dev_priv) { + struct intel_context *ce = &ctx->engine[engine->id]; + + ret = 0; + if (ce->state) + ret = i915_gem_obj_ggtt_pin(ce->state, + get_context_alignment(dev_priv), 0); + if (ret) { + DRM_ERROR("Failed to pinned default global context (error %d)\n", + ret); + i915_gem_context_unreference(ctx); + return ret; + } + + /* The kernel context is only used as a placeholder + * for flushing the active context. It is never used + * for submitting rendering and as such never requires + * the golden render context, and so we can skip + * emitting it when we switch to the kernel context + * (during eviction). + */ + ce->initialised = true; } } @@ -452,13 +468,16 @@ void i915_gem_context_lost(struct drm_i915_private *dev_priv) i915_gem_context_unpin(engine->last_context, engine); engine->last_context = NULL; } - - /* Force the GPU state to be reinitialised on enabling */ - dev_priv->kernel_context->engine[engine->id].initialised = - engine->init_context == NULL; } - /* Force the GPU state to be reinitialised on enabling */ + /* Force the GPU state to be restore on enabling */ + for_each_engine(engine, dev_priv) { + struct intel_context *ce = + &dev_priv->kernel_context->engine[engine->id]; + + ce->initialised = + !i915.enable_execlists || engine->init_context == NULL; + } dev_priv->kernel_context->remap_slice = ALL_L3_SLICES(dev_priv); }
The kernel context exists simply as a placeholder and should never be executed with a render context. It does not need the golden render state, as that will always be applied to a user context. By skipping the initialisation we can avoid issues in attempting to program the golden render context when trying to make the hardware idle. Testcase: igt/drm_module_reload_basic #byt Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=95634 Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> --- drivers/gpu/drm/i915/i915_gem_context.c | 45 +++++++++++++++++++++++---------- 1 file changed, 32 insertions(+), 13 deletions(-)