| Message ID | 20170719125502.25696-5-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Quoting Daniel Vetter (2017-07-19 13:54:57) > Blocking in a worker is ok, but needlessly inefficient, > that's what the unbound_wq is for. And it > unifies the paths between the blocking and nonblocking commit, giving > me just one path where I have to implement the deadlock avoidance > trickery in the next patch. For reference, I did that the other way by moving it all over to fences. -Chris
On Wed, Jul 19, 2017 at 02:04:25PM +0100, Chris Wilson wrote: > Quoting Daniel Vetter (2017-07-19 13:54:57) > > Blocking in a worker is ok, > > but needlessly inefficient, > > > that's what the unbound_wq is for. And it > > unifies the paths between the blocking and nonblocking commit, giving > > me just one path where I have to implement the deadlock avoidance > > trickery in the next patch. > > For reference, I did that the other way by moving it all over to fences. Yeah the commit message fails to explain this here: "I first tried to implement the following patch without this rework, but force-completing i915_sw_fence creates some serious challenges around properly cleaning things up. So wasn't a feasible short-term approach. Another approach would be to simple keep track of all pending atomic commit work items and manually queue them from the reset code. With the caveat that double-queue in case we race with the i915_sw_fence must be avoided. Given all that, taking the cost of a double schedule in atomic for the short-term fix is the best approach, but can be changed in the future of course." Thanks, Daniel
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 010a1f3e000c..5aa7ca1ab592 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -12397,6 +12397,8 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state) unsigned crtc_vblank_mask = 0; int i; + i915_sw_fence_wait(&intel_state->commit_ready); + drm_atomic_helper_wait_for_dependencies(state); if (intel_state->modeset) @@ -12564,10 +12566,7 @@ intel_atomic_commit_ready(struct i915_sw_fence *fence, switch (notify) { case FENCE_COMPLETE: - if (state->base.commit_work.func) - queue_work(system_unbound_wq, &state->base.commit_work); break; - case FENCE_FREE: { struct intel_atomic_helper *helper = @@ -12671,14 +12670,14 @@ static int intel_atomic_commit(struct drm_device *dev, } drm_atomic_state_get(state); - INIT_WORK(&state->commit_work, - nonblock ? intel_atomic_commit_work : NULL); + INIT_WORK(&state->commit_work, intel_atomic_commit_work); i915_sw_fence_commit(&intel_state->commit_ready); - if (!nonblock) { - i915_sw_fence_wait(&intel_state->commit_ready); + if (nonblock) + queue_work(system_unbound_wq, &state->commit_work); + else intel_atomic_commit_tail(state); - } + return 0; }
Blocking in a worker is ok, that's what the unbound_wq is for. And it unifies the paths between the blocking and nonblocking commit, giving me just one path where I have to implement the deadlock avoidance trickery in the next patch. Cc: Chris Wilson <chris@chris-wilson.co.uk> Cc: Mika Kuoppala <mika.kuoppala@intel.com> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> --- drivers/gpu/drm/i915/intel_display.c | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-)