| Message ID | 20180517165642.1416-1-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 17 May 2018 18:56:41 +0200, Chris Wilson <chris@chris-wilson.co.uk> wrote: > We make a decision at module load whether to use the GuC backend or not, > but lose that setup across set-wedge. Currently, the guc doesn't > override the engine->set_default_submission hook letting execlists sneak > back in temporarily on unwedging leading to an unbalanced park/unpark. > > Testcase: igt/gem_eio > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Michał Winiarski <michal.winiarski@intel.com> > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> > --- > drivers/gpu/drm/i915/intel_guc_submission.c | 34 +++++++++++++++------ > drivers/gpu/drm/i915/intel_lrc.c | 2 +- > drivers/gpu/drm/i915/intel_lrc.h | 2 ++ > 3 files changed, 28 insertions(+), 10 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_guc_submission.c > b/drivers/gpu/drm/i915/intel_guc_submission.c > index 637e852888ec..cbd8caffd271 100644 > --- a/drivers/gpu/drm/i915/intel_guc_submission.c > +++ b/drivers/gpu/drm/i915/intel_guc_submission.c > @@ -1264,6 +1264,29 @@ static void guc_submission_unpark(struct > intel_engine_cs *engine) > intel_engine_pin_breadcrumbs_irq(engine); > } > +static void guc_set_default_submission(struct intel_engine_cs *engine) > +{ > + /* > + * We inherit a bunch of functions from execlists that we'd like > + * to keep using: > + * > + * engine->submit_request = execlists_submit_request; > + * engine->cancel_requests = execlists_cancel_requests; > + * engine->schedule = execlists_schedule; > + * > + * But we need to override the actual submission backend in order > + * to talk to the GuC. > + */ > + execlists_set_default_submission(engine); > + > + engine->execlists.tasklet.func = guc_submission_tasklet; > + > + engine->park = guc_submission_park; > + engine->unpark = guc_submission_unpark; > + > + engine->flags &= ~I915_ENGINE_SUPPORTS_STATS; > +} > + > int intel_guc_submission_enable(struct intel_guc *guc) > { > struct drm_i915_private *dev_priv = guc_to_i915(guc); > @@ -1302,17 +1325,10 @@ int intel_guc_submission_enable(struct intel_guc > *guc) > guc_interrupts_capture(dev_priv); > for_each_engine(engine, dev_priv, id) { > - struct intel_engine_execlists * const execlists = > - &engine->execlists; > - > - execlists->tasklet.func = guc_submission_tasklet; > - > engine->reset.prepare = guc_reset_prepare; > + engine->set_default_submission = guc_set_default_submission; > - engine->park = guc_submission_park; > - engine->unpark = guc_submission_unpark; > - > - engine->flags &= ~I915_ENGINE_SUPPORTS_STATS; > + engine->set_default_submission(engine); While this part looks ok (and maybe will help with my [1]) but what about intel_guc_submission_disable(), where we will call intel_engines_reset_default_submission() and 'revert' to GuC again... time for nop_submission() ? Michal [1] https://patchwork.freedesktop.org/series/41304/ > } > return 0; > diff --git a/drivers/gpu/drm/i915/intel_lrc.c > b/drivers/gpu/drm/i915/intel_lrc.c > index 646ecf267411..853fb0b5f73e 100644 > --- a/drivers/gpu/drm/i915/intel_lrc.c > +++ b/drivers/gpu/drm/i915/intel_lrc.c > @@ -2291,7 +2291,7 @@ void intel_logical_ring_cleanup(struct > intel_engine_cs *engine) > kfree(engine); > } > -static void execlists_set_default_submission(struct intel_engine_cs > *engine) > +void execlists_set_default_submission(struct intel_engine_cs *engine) > { > engine->submit_request = execlists_submit_request; > engine->cancel_requests = execlists_cancel_requests; > diff --git a/drivers/gpu/drm/i915/intel_lrc.h > b/drivers/gpu/drm/i915/intel_lrc.h > index 4ec7d8dd13c8..e64f47e612f4 100644 > --- a/drivers/gpu/drm/i915/intel_lrc.h > +++ b/drivers/gpu/drm/i915/intel_lrc.h > @@ -111,4 +111,6 @@ intel_lr_context_descriptor(struct i915_gem_context > *ctx, > return to_intel_context(ctx, engine)->lrc_desc; > } > +void execlists_set_default_submission(struct intel_engine_cs *engine); > + > #endif /* _INTEL_LRC_H_ */
Quoting Michal Wajdeczko (2018-05-17 18:23:23) > On Thu, 17 May 2018 18:56:41 +0200, Chris Wilson > <chris@chris-wilson.co.uk> wrote: > > > We make a decision at module load whether to use the GuC backend or not, > > but lose that setup across set-wedge. Currently, the guc doesn't > > override the engine->set_default_submission hook letting execlists sneak > > back in temporarily on unwedging leading to an unbalanced park/unpark. > > > > Testcase: igt/gem_eio > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > Cc: Michał Winiarski <michal.winiarski@intel.com> > > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> > > --- > > drivers/gpu/drm/i915/intel_guc_submission.c | 34 +++++++++++++++------ > > drivers/gpu/drm/i915/intel_lrc.c | 2 +- > > drivers/gpu/drm/i915/intel_lrc.h | 2 ++ > > 3 files changed, 28 insertions(+), 10 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_guc_submission.c > > b/drivers/gpu/drm/i915/intel_guc_submission.c > > index 637e852888ec..cbd8caffd271 100644 > > --- a/drivers/gpu/drm/i915/intel_guc_submission.c > > +++ b/drivers/gpu/drm/i915/intel_guc_submission.c > > @@ -1264,6 +1264,29 @@ static void guc_submission_unpark(struct > > intel_engine_cs *engine) > > intel_engine_pin_breadcrumbs_irq(engine); > > } > > +static void guc_set_default_submission(struct intel_engine_cs *engine) > > +{ > > + /* > > + * We inherit a bunch of functions from execlists that we'd like > > + * to keep using: > > + * > > + * engine->submit_request = execlists_submit_request; > > + * engine->cancel_requests = execlists_cancel_requests; > > + * engine->schedule = execlists_schedule; > > + * > > + * But we need to override the actual submission backend in order > > + * to talk to the GuC. > > + */ > > + execlists_set_default_submission(engine); > > + > > + engine->execlists.tasklet.func = guc_submission_tasklet; > > + > > + engine->park = guc_submission_park; > > + engine->unpark = guc_submission_unpark; > > + > > + engine->flags &= ~I915_ENGINE_SUPPORTS_STATS; > > +} > > + > > int intel_guc_submission_enable(struct intel_guc *guc) > > { > > struct drm_i915_private *dev_priv = guc_to_i915(guc); > > @@ -1302,17 +1325,10 @@ int intel_guc_submission_enable(struct intel_guc > > *guc) > > guc_interrupts_capture(dev_priv); > > for_each_engine(engine, dev_priv, id) { > > - struct intel_engine_execlists * const execlists = > > - &engine->execlists; > > - > > - execlists->tasklet.func = guc_submission_tasklet; > > - > > engine->reset.prepare = guc_reset_prepare; > > + engine->set_default_submission = guc_set_default_submission; > > - engine->park = guc_submission_park; > > - engine->unpark = guc_submission_unpark; > > - > > - engine->flags &= ~I915_ENGINE_SUPPORTS_STATS; > > + engine->set_default_submission(engine); > > While this part looks ok (and maybe will help with my [1]) > but what about intel_guc_submission_disable(), where we > will call intel_engines_reset_default_submission() and > 'revert' to GuC again... time for nop_submission() ? If we apply the "once you go GuC, you won't go back" rule, then we might as well just call i915_gem_set_wedged() (or leave it wedged) on disabling. So long as any re-enable path goes through a i915_reset, we will be fine. -Chris
diff --git a/drivers/gpu/drm/i915/intel_guc_submission.c b/drivers/gpu/drm/i915/intel_guc_submission.c index 637e852888ec..cbd8caffd271 100644 --- a/drivers/gpu/drm/i915/intel_guc_submission.c +++ b/drivers/gpu/drm/i915/intel_guc_submission.c @@ -1264,6 +1264,29 @@ static void guc_submission_unpark(struct intel_engine_cs *engine) intel_engine_pin_breadcrumbs_irq(engine); } +static void guc_set_default_submission(struct intel_engine_cs *engine) +{ + /* + * We inherit a bunch of functions from execlists that we'd like + * to keep using: + * + * engine->submit_request = execlists_submit_request; + * engine->cancel_requests = execlists_cancel_requests; + * engine->schedule = execlists_schedule; + * + * But we need to override the actual submission backend in order + * to talk to the GuC. + */ + execlists_set_default_submission(engine); + + engine->execlists.tasklet.func = guc_submission_tasklet; + + engine->park = guc_submission_park; + engine->unpark = guc_submission_unpark; + + engine->flags &= ~I915_ENGINE_SUPPORTS_STATS; +} + int intel_guc_submission_enable(struct intel_guc *guc) { struct drm_i915_private *dev_priv = guc_to_i915(guc); @@ -1302,17 +1325,10 @@ int intel_guc_submission_enable(struct intel_guc *guc) guc_interrupts_capture(dev_priv); for_each_engine(engine, dev_priv, id) { - struct intel_engine_execlists * const execlists = - &engine->execlists; - - execlists->tasklet.func = guc_submission_tasklet; - engine->reset.prepare = guc_reset_prepare; + engine->set_default_submission = guc_set_default_submission; - engine->park = guc_submission_park; - engine->unpark = guc_submission_unpark; - - engine->flags &= ~I915_ENGINE_SUPPORTS_STATS; + engine->set_default_submission(engine); } return 0; diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c index 646ecf267411..853fb0b5f73e 100644 --- a/drivers/gpu/drm/i915/intel_lrc.c +++ b/drivers/gpu/drm/i915/intel_lrc.c @@ -2291,7 +2291,7 @@ void intel_logical_ring_cleanup(struct intel_engine_cs *engine) kfree(engine); } -static void execlists_set_default_submission(struct intel_engine_cs *engine) +void execlists_set_default_submission(struct intel_engine_cs *engine) { engine->submit_request = execlists_submit_request; engine->cancel_requests = execlists_cancel_requests; diff --git a/drivers/gpu/drm/i915/intel_lrc.h b/drivers/gpu/drm/i915/intel_lrc.h index 4ec7d8dd13c8..e64f47e612f4 100644 --- a/drivers/gpu/drm/i915/intel_lrc.h +++ b/drivers/gpu/drm/i915/intel_lrc.h @@ -111,4 +111,6 @@ intel_lr_context_descriptor(struct i915_gem_context *ctx, return to_intel_context(ctx, engine)->lrc_desc; } +void execlists_set_default_submission(struct intel_engine_cs *engine); + #endif /* _INTEL_LRC_H_ */
We make a decision at module load whether to use the GuC backend or not, but lose that setup across set-wedge. Currently, the guc doesn't override the engine->set_default_submission hook letting execlists sneak back in temporarily on unwedging leading to an unbalanced park/unpark. Testcase: igt/gem_eio Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: Michał Winiarski <michal.winiarski@intel.com> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> --- drivers/gpu/drm/i915/intel_guc_submission.c | 34 +++++++++++++++------ drivers/gpu/drm/i915/intel_lrc.c | 2 +- drivers/gpu/drm/i915/intel_lrc.h | 2 ++ 3 files changed, 28 insertions(+), 10 deletions(-)