[Intel-gfx] [PATCH 1/8] drm/i915/execlists: Avoid sync calls during park

[Mika Kuoppala]

Chris Wilson <chris at chris-wilson.co.uk> writes:

> Quoting Mika Kuoppala (2019-08-12 10:27:16)
>> Chris Wilson <chris at chris-wilson.co.uk> writes:
>> 
>> > Since we allow ourselves to use non-process context during parking, we
>> > cannot allow ourselves to sleep and in particular cannot call
>> > del_timer_sync() -- but we can use a plain del_timer().
>> >
>> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=111375
>> > Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
>> > ---
>> >  drivers/gpu/drm/i915/gt/intel_lrc.c | 2 +-
>> >  1 file changed, 1 insertion(+), 1 deletion(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
>> > index bb74954889dd..b97047d58d3d 100644
>> > --- a/drivers/gpu/drm/i915/gt/intel_lrc.c
>> > +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
>> > @@ -2728,7 +2728,7 @@ static u32 *gen8_emit_fini_breadcrumb_rcs(struct i915_request *request, u32 *cs)
>> >  
>> >  static void execlists_park(struct intel_engine_cs *engine)
>> >  {
>> > -     del_timer_sync(&engine->execlists.timer);
>> > +     del_timer(&engine->execlists.timer);
>> 
>> There will be another sync point then somewhere else or not needed?
>
> Not required, as it means the timer if currently running and will just
> kick the tasklet (as it does today). The tasklet running after we park
> is not a huge issue as it doesn't touch HW -- it checks a CPU mapping
> and in the process drains the GT wakeref.
>  
>> Also are irq safe timers where we could do a sync deletion. 
>> 
>> So my question is why the need for a sync point disappeared?
>
> We didn't use it correctly to begin with :) To complete the sync, we
> should have put a tasklet_kill(&execlists->tasklet); afterwards.

Ok,
So no need for fancey irq safe timers either.

Reviewed-by: Mika Kuoppala <mika.kuoppala at linux.intel.com>