[4/9] drm/i915: Push i915_sw_fence_wait into the nonblocking atomic commit
diff mbox

Message ID 20170719125502.25696-5-daniel.vetter@ffwll.ch
State New
Headers show

Commit Message

Daniel Vetter July 19, 2017, 12:54 p.m. UTC
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(-)

Comments

Chris Wilson July 19, 2017, 1:04 p.m. UTC | #1
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
Daniel Vetter July 19, 2017, 1:14 p.m. UTC | #2
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

Patch
diff mbox

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;
 }