| Message ID | 20180409111413.6352-8-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Chris Wilson <chris@chris-wilson.co.uk> writes: > Ideally, we want to atomically flush and disable the tasklet before > resetting the GPU. At present, we rely on being the only part to touch > our tasklet and serialisation of the reset process to ensure that we can > suspend the tasklet from the mix of reset/wedge pathways. In this patch, > we move the tasklet abuse into its own function and tweak it such that > we only do a synchronous operation the first time it is disabled around > the reset. This allows us to avoid the sync inside a softirq context in > subsequent patches. > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com> > Cc: Michał Winiarski <michal.winiarski@intel.com> > CC: Michel Thierry <michel.thierry@intel.com> > Cc: Jeff McGee <jeff.mcgee@intel.com> > --- > drivers/gpu/drm/i915/intel_lrc.c | 14 +++++++++++--- > 1 file changed, 11 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c > index bbcc6439a2a1..d5640f3d5276 100644 > --- a/drivers/gpu/drm/i915/intel_lrc.c > +++ b/drivers/gpu/drm/i915/intel_lrc.c > @@ -1754,6 +1754,16 @@ static int gen9_init_render_ring(struct intel_engine_cs *engine) > return init_workarounds_ring(engine); > } > > +static void tasklet_kill_and_disable(struct tasklet_struct *t) > +{ > + if (!atomic_read(&t->count)) > + tasklet_kill(t); > + > + if (atomic_inc_return(&t->count) == 1) > + tasklet_unlock_wait(t); You would spin only on the first try regardless. Is this just to prevent one extra spinlock on reset path? -Mika > + smp_mb(); > +} > + > static struct i915_request * > execlists_reset_prepare(struct intel_engine_cs *engine) > { > @@ -1778,9 +1788,7 @@ execlists_reset_prepare(struct intel_engine_cs *engine) > * common case of recursively being called from set-wedged from inside > * i915_reset. > */ > - if (!atomic_read(&execlists->tasklet.count)) > - tasklet_kill(&execlists->tasklet); > - tasklet_disable(&execlists->tasklet); > + tasklet_kill_and_disable(&execlists->tasklet); > > /* > * We want to flush the pending context switches, having disabled > -- > 2.17.0
Quoting Mika Kuoppala (2018-04-24 13:26:11) > Chris Wilson <chris@chris-wilson.co.uk> writes: > > > Ideally, we want to atomically flush and disable the tasklet before > > resetting the GPU. At present, we rely on being the only part to touch > > our tasklet and serialisation of the reset process to ensure that we can > > suspend the tasklet from the mix of reset/wedge pathways. In this patch, > > we move the tasklet abuse into its own function and tweak it such that > > we only do a synchronous operation the first time it is disabled around > > the reset. This allows us to avoid the sync inside a softirq context in > > subsequent patches. > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com> > > Cc: Michał Winiarski <michal.winiarski@intel.com> > > CC: Michel Thierry <michel.thierry@intel.com> > > Cc: Jeff McGee <jeff.mcgee@intel.com> > > --- > > drivers/gpu/drm/i915/intel_lrc.c | 14 +++++++++++--- > > 1 file changed, 11 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c > > index bbcc6439a2a1..d5640f3d5276 100644 > > --- a/drivers/gpu/drm/i915/intel_lrc.c > > +++ b/drivers/gpu/drm/i915/intel_lrc.c > > @@ -1754,6 +1754,16 @@ static int gen9_init_render_ring(struct intel_engine_cs *engine) > > return init_workarounds_ring(engine); > > } > > > > +static void tasklet_kill_and_disable(struct tasklet_struct *t) > > +{ > > + if (!atomic_read(&t->count)) > > + tasklet_kill(t); > > + > > + if (atomic_inc_return(&t->count) == 1) > > + tasklet_unlock_wait(t); > > You would spin only on the first try regardless. Is this > just to prevent one extra spinlock on reset path? No, the end goal is to prevent a recursive lock. -Chris
Chris Wilson <chris@chris-wilson.co.uk> writes: > Quoting Mika Kuoppala (2018-04-24 13:26:11) >> Chris Wilson <chris@chris-wilson.co.uk> writes: >> >> > Ideally, we want to atomically flush and disable the tasklet before >> > resetting the GPU. At present, we rely on being the only part to touch >> > our tasklet and serialisation of the reset process to ensure that we can >> > suspend the tasklet from the mix of reset/wedge pathways. In this patch, >> > we move the tasklet abuse into its own function and tweak it such that >> > we only do a synchronous operation the first time it is disabled around >> > the reset. This allows us to avoid the sync inside a softirq context in >> > subsequent patches. >> > >> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> >> > Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com> >> > Cc: Michał Winiarski <michal.winiarski@intel.com> >> > CC: Michel Thierry <michel.thierry@intel.com> >> > Cc: Jeff McGee <jeff.mcgee@intel.com> >> > --- >> > drivers/gpu/drm/i915/intel_lrc.c | 14 +++++++++++--- >> > 1 file changed, 11 insertions(+), 3 deletions(-) >> > >> > diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c >> > index bbcc6439a2a1..d5640f3d5276 100644 >> > --- a/drivers/gpu/drm/i915/intel_lrc.c >> > +++ b/drivers/gpu/drm/i915/intel_lrc.c >> > @@ -1754,6 +1754,16 @@ static int gen9_init_render_ring(struct intel_engine_cs *engine) >> > return init_workarounds_ring(engine); >> > } >> > >> > +static void tasklet_kill_and_disable(struct tasklet_struct *t) >> > +{ >> > + if (!atomic_read(&t->count)) >> > + tasklet_kill(t); >> > + >> > + if (atomic_inc_return(&t->count) == 1) >> > + tasklet_unlock_wait(t); >> >> You would spin only on the first try regardless. Is this >> just to prevent one extra spinlock on reset path? > > No, the end goal is to prevent a recursive lock. Ok so you want to be able to call this from inside the tasklet itself. On the bigger picture, if the preempt has has already timeouted and we want to reset, does it matter between resetting from wq or from timer irq. In another words what do we gain for this much of added complexity? -Mika
Quoting Mika Kuoppala (2018-04-26 11:19:14) > Chris Wilson <chris@chris-wilson.co.uk> writes: > > > Quoting Mika Kuoppala (2018-04-24 13:26:11) > >> Chris Wilson <chris@chris-wilson.co.uk> writes: > >> > >> > Ideally, we want to atomically flush and disable the tasklet before > >> > resetting the GPU. At present, we rely on being the only part to touch > >> > our tasklet and serialisation of the reset process to ensure that we can > >> > suspend the tasklet from the mix of reset/wedge pathways. In this patch, > >> > we move the tasklet abuse into its own function and tweak it such that > >> > we only do a synchronous operation the first time it is disabled around > >> > the reset. This allows us to avoid the sync inside a softirq context in > >> > subsequent patches. > >> > > >> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > >> > Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com> > >> > Cc: Michał Winiarski <michal.winiarski@intel.com> > >> > CC: Michel Thierry <michel.thierry@intel.com> > >> > Cc: Jeff McGee <jeff.mcgee@intel.com> > >> > --- > >> > drivers/gpu/drm/i915/intel_lrc.c | 14 +++++++++++--- > >> > 1 file changed, 11 insertions(+), 3 deletions(-) > >> > > >> > diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c > >> > index bbcc6439a2a1..d5640f3d5276 100644 > >> > --- a/drivers/gpu/drm/i915/intel_lrc.c > >> > +++ b/drivers/gpu/drm/i915/intel_lrc.c > >> > @@ -1754,6 +1754,16 @@ static int gen9_init_render_ring(struct intel_engine_cs *engine) > >> > return init_workarounds_ring(engine); > >> > } > >> > > >> > +static void tasklet_kill_and_disable(struct tasklet_struct *t) > >> > +{ > >> > + if (!atomic_read(&t->count)) > >> > + tasklet_kill(t); > >> > + > >> > + if (atomic_inc_return(&t->count) == 1) > >> > + tasklet_unlock_wait(t); > >> > >> You would spin only on the first try regardless. Is this > >> just to prevent one extra spinlock on reset path? > > > > No, the end goal is to prevent a recursive lock. > > Ok so you want to be able to call this from inside the > tasklet itself. > > On the bigger picture, if the preempt has has already > timeouted and we want to reset, does it matter between > resetting from wq or from timer irq. In another > words what do we gain for this much of added complexity? Yes. A reset and recovery is microseconds. Scheduling the wq is orders of magnitude indeterminably worse latency. (Calling schedule() itself will take as long as the reset.) -Chris
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c index bbcc6439a2a1..d5640f3d5276 100644 --- a/drivers/gpu/drm/i915/intel_lrc.c +++ b/drivers/gpu/drm/i915/intel_lrc.c @@ -1754,6 +1754,16 @@ static int gen9_init_render_ring(struct intel_engine_cs *engine) return init_workarounds_ring(engine); } +static void tasklet_kill_and_disable(struct tasklet_struct *t) +{ + if (!atomic_read(&t->count)) + tasklet_kill(t); + + if (atomic_inc_return(&t->count) == 1) + tasklet_unlock_wait(t); + smp_mb(); +} + static struct i915_request * execlists_reset_prepare(struct intel_engine_cs *engine) { @@ -1778,9 +1788,7 @@ execlists_reset_prepare(struct intel_engine_cs *engine) * common case of recursively being called from set-wedged from inside * i915_reset. */ - if (!atomic_read(&execlists->tasklet.count)) - tasklet_kill(&execlists->tasklet); - tasklet_disable(&execlists->tasklet); + tasklet_kill_and_disable(&execlists->tasklet); /* * We want to flush the pending context switches, having disabled
Ideally, we want to atomically flush and disable the tasklet before resetting the GPU. At present, we rely on being the only part to touch our tasklet and serialisation of the reset process to ensure that we can suspend the tasklet from the mix of reset/wedge pathways. In this patch, we move the tasklet abuse into its own function and tweak it such that we only do a synchronous operation the first time it is disabled around the reset. This allows us to avoid the sync inside a softirq context in subsequent patches. Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com> Cc: Michał Winiarski <michal.winiarski@intel.com> CC: Michel Thierry <michel.thierry@intel.com> Cc: Jeff McGee <jeff.mcgee@intel.com> --- drivers/gpu/drm/i915/intel_lrc.c | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-)