| Message ID | 20191129093908.631527-1-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: Serialise i915_active_wait() with its retirement | expand |
On 29/11/2019 09:39, Chris Wilson wrote: > As the i915_active.retire() may be running on another CPU as we detect > that the i915_active is idle, we may not wait for the retirement itself. > Wait for the remote callback by waiting for the retirement worker. > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=112424 > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > --- > drivers/gpu/drm/i915/i915_active.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/gpu/drm/i915/i915_active.c b/drivers/gpu/drm/i915/i915_active.c > index 479195ecbc6c..e8630ee33336 100644 > --- a/drivers/gpu/drm/i915/i915_active.c > +++ b/drivers/gpu/drm/i915/i915_active.c > @@ -469,6 +469,7 @@ int i915_active_wait(struct i915_active *ref) > if (wait_var_event_interruptible(ref, i915_active_is_idle(ref))) > return -EINTR; > > + flush_work(&ref->work); > return 0; > } > > Hm, but wake_up_war is in the worker so how does wait_var_event wake the waiter up before it has been retired? Regards, Tvrtko
Quoting Tvrtko Ursulin (2019-11-29 11:15:46) > > On 29/11/2019 09:39, Chris Wilson wrote: > > As the i915_active.retire() may be running on another CPU as we detect > > that the i915_active is idle, we may not wait for the retirement itself. > > Wait for the remote callback by waiting for the retirement worker. > > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=112424 > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > --- > > drivers/gpu/drm/i915/i915_active.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/drivers/gpu/drm/i915/i915_active.c b/drivers/gpu/drm/i915/i915_active.c > > index 479195ecbc6c..e8630ee33336 100644 > > --- a/drivers/gpu/drm/i915/i915_active.c > > +++ b/drivers/gpu/drm/i915/i915_active.c > > @@ -469,6 +469,7 @@ int i915_active_wait(struct i915_active *ref) > > if (wait_var_event_interruptible(ref, i915_active_is_idle(ref))) > > return -EINTR; > > > > + flush_work(&ref->work); > > return 0; > > } > > > > > > Hm, but wake_up_war is in the worker so how does wait_var_event wake the > waiter up before it has been retired? Remember the wait_event pattern is to skip the wait if COND is already met. So since the first thing the retirement does is the atomic_dec_and_test(), we can see ref->count == 0 very early, long before ref->retire() is called. Our selftest is checking that if i915_active_wait() reports completion, the callback has also run and that the i915_active can then be destroyed. -Chris
On 29/11/2019 11:28, Chris Wilson wrote: > Quoting Tvrtko Ursulin (2019-11-29 11:15:46) >> >> On 29/11/2019 09:39, Chris Wilson wrote: >>> As the i915_active.retire() may be running on another CPU as we detect >>> that the i915_active is idle, we may not wait for the retirement itself. >>> Wait for the remote callback by waiting for the retirement worker. >>> >>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=112424 >>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> >>> --- >>> drivers/gpu/drm/i915/i915_active.c | 1 + >>> 1 file changed, 1 insertion(+) >>> >>> diff --git a/drivers/gpu/drm/i915/i915_active.c b/drivers/gpu/drm/i915/i915_active.c >>> index 479195ecbc6c..e8630ee33336 100644 >>> --- a/drivers/gpu/drm/i915/i915_active.c >>> +++ b/drivers/gpu/drm/i915/i915_active.c >>> @@ -469,6 +469,7 @@ int i915_active_wait(struct i915_active *ref) >>> if (wait_var_event_interruptible(ref, i915_active_is_idle(ref))) >>> return -EINTR; >>> >>> + flush_work(&ref->work); >>> return 0; >>> } >>> >>> >> >> Hm, but wake_up_war is in the worker so how does wait_var_event wake the >> waiter up before it has been retired? > > Remember the wait_event pattern is to skip the wait if COND is already > met. So since the first thing the retirement does is the > atomic_dec_and_test(), we can see ref->count == 0 very early, long > before ref->retire() is called. Our selftest is checking that if > i915_active_wait() reports completion, the callback has also run and > that the i915_active can then be destroyed. True! Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Regards, Tvrtko
diff --git a/drivers/gpu/drm/i915/i915_active.c b/drivers/gpu/drm/i915/i915_active.c index 479195ecbc6c..e8630ee33336 100644 --- a/drivers/gpu/drm/i915/i915_active.c +++ b/drivers/gpu/drm/i915/i915_active.c @@ -469,6 +469,7 @@ int i915_active_wait(struct i915_active *ref) if (wait_var_event_interruptible(ref, i915_active_is_idle(ref))) return -EINTR; + flush_work(&ref->work); return 0; }
As the i915_active.retire() may be running on another CPU as we detect that the i915_active is idle, we may not wait for the retirement itself. Wait for the remote callback by waiting for the retirement worker. Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=112424 Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> --- drivers/gpu/drm/i915/i915_active.c | 1 + 1 file changed, 1 insertion(+)