[03/14] drm/i915/execlists: Flush the tasklet on parking
diff mbox series

Message ID 20190501114541.10077-3-chris@chris-wilson.co.uk
State New
Headers show
Series
  • [01/14] drm/i915/hangcheck: Track context changes
Related show

Commit Message

Chris Wilson May 1, 2019, 11:45 a.m. UTC
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(-)

Comments

Tvrtko Ursulin May 2, 2019, 1:48 p.m. UTC | #1
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;
>   }
>
Chris Wilson May 2, 2019, 1:53 p.m. UTC | #2
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
Tvrtko Ursulin May 2, 2019, 2:14 p.m. UTC | #3
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
Chris Wilson May 2, 2019, 2:21 p.m. UTC | #4
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
Tvrtko Ursulin May 2, 2019, 2:24 p.m. UTC | #5
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
Chris Wilson May 2, 2019, 2:33 p.m. UTC | #6
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

Patch
diff mbox series

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;
 }