| Message ID | 20191119100929.2628356-9-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/17] drm/i915/gem: Manually dump the debug trace on GEM_BUG_ON | expand |
Chris Wilson <chris@chris-wilson.co.uk> writes: > When waiting for idle, serialise with any ongoing callback so that it > will have completed before completing the wait. Might be come apparent and evident when reading the patch that introduce the intel_wakeref_unlock_wait(), but reader is yearning for a why part. The 'wait_for_idle' is kind of revaling of why the need for sync tho. -Mika > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > --- > drivers/gpu/drm/i915/intel_wakeref.c | 11 +++++++++-- > 1 file changed, 9 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_wakeref.c b/drivers/gpu/drm/i915/intel_wakeref.c > index 9b29176cc1ca..91feb53b2942 100644 > --- a/drivers/gpu/drm/i915/intel_wakeref.c > +++ b/drivers/gpu/drm/i915/intel_wakeref.c > @@ -109,8 +109,15 @@ void __intel_wakeref_init(struct intel_wakeref *wf, > > int intel_wakeref_wait_for_idle(struct intel_wakeref *wf) > { > - return wait_var_event_killable(&wf->wakeref, > - !intel_wakeref_is_active(wf)); > + int err; > + > + err = wait_var_event_killable(&wf->wakeref, > + !intel_wakeref_is_active(wf)); > + if (err) > + return err; > + > + intel_wakeref_unlock_wait(wf); > + return 0; > } > > static void wakeref_auto_timeout(struct timer_list *t) > -- > 2.24.0
Quoting Mika Kuoppala (2019-11-19 16:12:18) > Chris Wilson <chris@chris-wilson.co.uk> writes: > > > When waiting for idle, serialise with any ongoing callback so that it > > will have completed before completing the wait. > > Might be come apparent and evident when reading the patch > that introduce the intel_wakeref_unlock_wait(), > but reader is yearning for a why part. > > The 'wait_for_idle' is kind of revaling of > why the need for sync tho. I don't have a particular example where by not waiting for the callback to finish, we found ourselves inspecting stale state. But that's the worry -- that any caller proceeds assuming the wakeref mutexes are no longer in use, when in fact some CPU is still accessing that memory. Tends to only be a worry before free, which in this case entails module unload. I needed the "unlock_wait" for engine-pm in one of the selftests, as I was adding that, I saw no harm in dotting the 'i' and crossing the 't' here as well. -Chris
diff --git a/drivers/gpu/drm/i915/intel_wakeref.c b/drivers/gpu/drm/i915/intel_wakeref.c index 9b29176cc1ca..91feb53b2942 100644 --- a/drivers/gpu/drm/i915/intel_wakeref.c +++ b/drivers/gpu/drm/i915/intel_wakeref.c @@ -109,8 +109,15 @@ void __intel_wakeref_init(struct intel_wakeref *wf, int intel_wakeref_wait_for_idle(struct intel_wakeref *wf) { - return wait_var_event_killable(&wf->wakeref, - !intel_wakeref_is_active(wf)); + int err; + + err = wait_var_event_killable(&wf->wakeref, + !intel_wakeref_is_active(wf)); + if (err) + return err; + + intel_wakeref_unlock_wait(wf); + return 0; } static void wakeref_auto_timeout(struct timer_list *t)
When waiting for idle, serialise with any ongoing callback so that it will have completed before completing the wait. Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> --- drivers/gpu/drm/i915/intel_wakeref.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)