| Message ID | 20190501114541.10077-3-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/14] drm/i915/hangcheck: Track context changes | expand |
On 01/05/2019 12:45, Chris Wilson wrote: > Tidy up the cleanup sequence by always ensure that the tasklet is > flushed on parking (before we cleanup). The parking provides a > convenient point to ensure that the backend is truly idle. > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > --- > drivers/gpu/drm/i915/gt/intel_lrc.c | 7 ++++++- > drivers/gpu/drm/i915/intel_guc_submission.c | 1 + > 2 files changed, 7 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c > index 851e62ddcb87..7be54b868d8e 100644 > --- a/drivers/gpu/drm/i915/gt/intel_lrc.c > +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c > @@ -2331,6 +2331,11 @@ static int gen8_init_rcs_context(struct i915_request *rq) > return i915_gem_render_state_emit(rq); > } > > +static void execlists_park(struct intel_engine_cs *engine) > +{ > + tasklet_kill(&engine->execlists.tasklet); Isn't it actually a problem if tasklet is scheduled and unstarted, or even in progress at the point of engine getting parked? Regards, Tvrtko > +} > + > void intel_execlists_set_default_submission(struct intel_engine_cs *engine) > { > engine->submit_request = execlists_submit_request; > @@ -2342,7 +2347,7 @@ void intel_execlists_set_default_submission(struct intel_engine_cs *engine) > engine->reset.reset = execlists_reset; > engine->reset.finish = execlists_reset_finish; > > - engine->park = NULL; > + engine->park = execlists_park; > engine->unpark = NULL; > > engine->flags |= I915_ENGINE_SUPPORTS_STATS; > diff --git a/drivers/gpu/drm/i915/intel_guc_submission.c b/drivers/gpu/drm/i915/intel_guc_submission.c > index 4c814344809c..ed94001028f2 100644 > --- a/drivers/gpu/drm/i915/intel_guc_submission.c > +++ b/drivers/gpu/drm/i915/intel_guc_submission.c > @@ -1363,6 +1363,7 @@ static void guc_interrupts_release(struct drm_i915_private *dev_priv) > > static void guc_submission_park(struct intel_engine_cs *engine) > { > + tasklet_kill(&engine->execlists.tasklet); > intel_engine_unpin_breadcrumbs_irq(engine); > engine->flags &= ~I915_ENGINE_NEEDS_BREADCRUMB_TASKLET; > } >
Quoting Tvrtko Ursulin (2019-05-02 14:48:18) > > On 01/05/2019 12:45, Chris Wilson wrote: > > Tidy up the cleanup sequence by always ensure that the tasklet is > > flushed on parking (before we cleanup). The parking provides a > > convenient point to ensure that the backend is truly idle. > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > --- > > drivers/gpu/drm/i915/gt/intel_lrc.c | 7 ++++++- > > drivers/gpu/drm/i915/intel_guc_submission.c | 1 + > > 2 files changed, 7 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c > > index 851e62ddcb87..7be54b868d8e 100644 > > --- a/drivers/gpu/drm/i915/gt/intel_lrc.c > > +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c > > @@ -2331,6 +2331,11 @@ static int gen8_init_rcs_context(struct i915_request *rq) > > return i915_gem_render_state_emit(rq); > > } > > > > +static void execlists_park(struct intel_engine_cs *engine) > > +{ > > + tasklet_kill(&engine->execlists.tasklet); > > Isn't it actually a problem if tasklet is scheduled and unstarted, or > even in progress at the point of engine getting parked? That would be a broken driver. :| We must be quite sure that engine isn't going to send an interrupt as we are just about to drop the wakeref we need to service that interrupt. tasklet_kill() GEM_BUG_ON(engine->execlists.active); -Chris
On 02/05/2019 14:53, Chris Wilson wrote: > Quoting Tvrtko Ursulin (2019-05-02 14:48:18) >> >> On 01/05/2019 12:45, Chris Wilson wrote: >>> Tidy up the cleanup sequence by always ensure that the tasklet is >>> flushed on parking (before we cleanup). The parking provides a >>> convenient point to ensure that the backend is truly idle. >>> >>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> >>> --- >>> drivers/gpu/drm/i915/gt/intel_lrc.c | 7 ++++++- >>> drivers/gpu/drm/i915/intel_guc_submission.c | 1 + >>> 2 files changed, 7 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c >>> index 851e62ddcb87..7be54b868d8e 100644 >>> --- a/drivers/gpu/drm/i915/gt/intel_lrc.c >>> +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c >>> @@ -2331,6 +2331,11 @@ static int gen8_init_rcs_context(struct i915_request *rq) >>> return i915_gem_render_state_emit(rq); >>> } >>> >>> +static void execlists_park(struct intel_engine_cs *engine) >>> +{ >>> + tasklet_kill(&engine->execlists.tasklet); >> >> Isn't it actually a problem if tasklet is scheduled and unstarted, or >> even in progress at the point of engine getting parked? > > That would be a broken driver. :| > > We must be quite sure that engine isn't going to send an interrupt as we > are just about to drop the wakeref we need to service that interrupt. > > tasklet_kill() > GEM_BUG_ON(engine->execlists.active); Or instead of both: /* Tasklet must not be running or scheduled at this point. */ GEM_BUG_ON(engine->execlists.tasklet.state); ? Regards, Tvrtko
Quoting Tvrtko Ursulin (2019-05-02 15:14:08) > > On 02/05/2019 14:53, Chris Wilson wrote: > > Quoting Tvrtko Ursulin (2019-05-02 14:48:18) > >> > >> On 01/05/2019 12:45, Chris Wilson wrote: > >>> Tidy up the cleanup sequence by always ensure that the tasklet is > >>> flushed on parking (before we cleanup). The parking provides a > >>> convenient point to ensure that the backend is truly idle. > >>> > >>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > >>> --- > >>> drivers/gpu/drm/i915/gt/intel_lrc.c | 7 ++++++- > >>> drivers/gpu/drm/i915/intel_guc_submission.c | 1 + > >>> 2 files changed, 7 insertions(+), 1 deletion(-) > >>> > >>> diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c > >>> index 851e62ddcb87..7be54b868d8e 100644 > >>> --- a/drivers/gpu/drm/i915/gt/intel_lrc.c > >>> +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c > >>> @@ -2331,6 +2331,11 @@ static int gen8_init_rcs_context(struct i915_request *rq) > >>> return i915_gem_render_state_emit(rq); > >>> } > >>> > >>> +static void execlists_park(struct intel_engine_cs *engine) > >>> +{ > >>> + tasklet_kill(&engine->execlists.tasklet); > >> > >> Isn't it actually a problem if tasklet is scheduled and unstarted, or > >> even in progress at the point of engine getting parked? > > > > That would be a broken driver. :| > > > > We must be quite sure that engine isn't going to send an interrupt as we > > are just about to drop the wakeref we need to service that interrupt. > > > > tasklet_kill() > > GEM_BUG_ON(engine->execlists.active); > > Or instead of both: > > /* Tasklet must not be running or scheduled at this point. */ > GEM_BUG_ON(engine->execlists.tasklet.state); There's the dilemma that we start parking based on retirement not final CS event. -Chris
On 02/05/2019 15:21, Chris Wilson wrote: > Quoting Tvrtko Ursulin (2019-05-02 15:14:08) >> >> On 02/05/2019 14:53, Chris Wilson wrote: >>> Quoting Tvrtko Ursulin (2019-05-02 14:48:18) >>>> >>>> On 01/05/2019 12:45, Chris Wilson wrote: >>>>> Tidy up the cleanup sequence by always ensure that the tasklet is >>>>> flushed on parking (before we cleanup). The parking provides a >>>>> convenient point to ensure that the backend is truly idle. >>>>> >>>>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> >>>>> --- >>>>> drivers/gpu/drm/i915/gt/intel_lrc.c | 7 ++++++- >>>>> drivers/gpu/drm/i915/intel_guc_submission.c | 1 + >>>>> 2 files changed, 7 insertions(+), 1 deletion(-) >>>>> >>>>> diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c >>>>> index 851e62ddcb87..7be54b868d8e 100644 >>>>> --- a/drivers/gpu/drm/i915/gt/intel_lrc.c >>>>> +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c >>>>> @@ -2331,6 +2331,11 @@ static int gen8_init_rcs_context(struct i915_request *rq) >>>>> return i915_gem_render_state_emit(rq); >>>>> } >>>>> >>>>> +static void execlists_park(struct intel_engine_cs *engine) >>>>> +{ >>>>> + tasklet_kill(&engine->execlists.tasklet); >>>> >>>> Isn't it actually a problem if tasklet is scheduled and unstarted, or >>>> even in progress at the point of engine getting parked? >>> >>> That would be a broken driver. :| >>> >>> We must be quite sure that engine isn't going to send an interrupt as we >>> are just about to drop the wakeref we need to service that interrupt. >>> >>> tasklet_kill() >>> GEM_BUG_ON(engine->execlists.active); >> >> Or instead of both: >> >> /* Tasklet must not be running or scheduled at this point. */ >> GEM_BUG_ON(engine->execlists.tasklet.state); > > There's the dilemma that we start parking based on retirement not > final CS event. But engine->park() is called once the last engine pm reference is dropped. Are we dropping the last reference with a CS event pending? Regards, Tvrtko
Quoting Tvrtko Ursulin (2019-05-02 15:24:16) > > On 02/05/2019 15:21, Chris Wilson wrote: > > Quoting Tvrtko Ursulin (2019-05-02 15:14:08) > >> > >> On 02/05/2019 14:53, Chris Wilson wrote: > >>> Quoting Tvrtko Ursulin (2019-05-02 14:48:18) > >>>> > >>>> On 01/05/2019 12:45, Chris Wilson wrote: > >>>>> Tidy up the cleanup sequence by always ensure that the tasklet is > >>>>> flushed on parking (before we cleanup). The parking provides a > >>>>> convenient point to ensure that the backend is truly idle. > >>>>> > >>>>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > >>>>> --- > >>>>> drivers/gpu/drm/i915/gt/intel_lrc.c | 7 ++++++- > >>>>> drivers/gpu/drm/i915/intel_guc_submission.c | 1 + > >>>>> 2 files changed, 7 insertions(+), 1 deletion(-) > >>>>> > >>>>> diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c > >>>>> index 851e62ddcb87..7be54b868d8e 100644 > >>>>> --- a/drivers/gpu/drm/i915/gt/intel_lrc.c > >>>>> +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c > >>>>> @@ -2331,6 +2331,11 @@ static int gen8_init_rcs_context(struct i915_request *rq) > >>>>> return i915_gem_render_state_emit(rq); > >>>>> } > >>>>> > >>>>> +static void execlists_park(struct intel_engine_cs *engine) > >>>>> +{ > >>>>> + tasklet_kill(&engine->execlists.tasklet); > >>>> > >>>> Isn't it actually a problem if tasklet is scheduled and unstarted, or > >>>> even in progress at the point of engine getting parked? > >>> > >>> That would be a broken driver. :| > >>> > >>> We must be quite sure that engine isn't going to send an interrupt as we > >>> are just about to drop the wakeref we need to service that interrupt. > >>> > >>> tasklet_kill() > >>> GEM_BUG_ON(engine->execlists.active); > >> > >> Or instead of both: > >> > >> /* Tasklet must not be running or scheduled at this point. */ > >> GEM_BUG_ON(engine->execlists.tasklet.state); > > > > There's the dilemma that we start parking based on retirement not > > final CS event. > > But engine->park() is called once the last engine pm reference is > dropped. Are we dropping the last reference with a CS event pending? Potentially we are. i915_request_retire() -> context->exit() -> engine->park() At no point along that chain do we actually check we have flushed the backend. The tasklet_kill() would flush if the interrupt had already been sent, but that's not very strict. Oh well, you've talked me into to re-adding the wait loop here. -Chris
diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c index 851e62ddcb87..7be54b868d8e 100644 --- a/drivers/gpu/drm/i915/gt/intel_lrc.c +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c @@ -2331,6 +2331,11 @@ static int gen8_init_rcs_context(struct i915_request *rq) return i915_gem_render_state_emit(rq); } +static void execlists_park(struct intel_engine_cs *engine) +{ + tasklet_kill(&engine->execlists.tasklet); +} + void intel_execlists_set_default_submission(struct intel_engine_cs *engine) { engine->submit_request = execlists_submit_request; @@ -2342,7 +2347,7 @@ void intel_execlists_set_default_submission(struct intel_engine_cs *engine) engine->reset.reset = execlists_reset; engine->reset.finish = execlists_reset_finish; - engine->park = NULL; + engine->park = execlists_park; engine->unpark = NULL; engine->flags |= I915_ENGINE_SUPPORTS_STATS; diff --git a/drivers/gpu/drm/i915/intel_guc_submission.c b/drivers/gpu/drm/i915/intel_guc_submission.c index 4c814344809c..ed94001028f2 100644 --- a/drivers/gpu/drm/i915/intel_guc_submission.c +++ b/drivers/gpu/drm/i915/intel_guc_submission.c @@ -1363,6 +1363,7 @@ static void guc_interrupts_release(struct drm_i915_private *dev_priv) static void guc_submission_park(struct intel_engine_cs *engine) { + tasklet_kill(&engine->execlists.tasklet); intel_engine_unpin_breadcrumbs_irq(engine); engine->flags &= ~I915_ENGINE_NEEDS_BREADCRUMB_TASKLET; }
Tidy up the cleanup sequence by always ensure that the tasklet is flushed on parking (before we cleanup). The parking provides a convenient point to ensure that the backend is truly idle. Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> --- drivers/gpu/drm/i915/gt/intel_lrc.c | 7 ++++++- drivers/gpu/drm/i915/intel_guc_submission.c | 1 + 2 files changed, 7 insertions(+), 1 deletion(-)