| Message ID | 20170720175754.30751-3-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 7/20/2017 10:57 AM, Daniel Vetter wrote: > 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. > > 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. > > v2: Amend commit message (Chris). > > Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > 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(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 995522e40ec1..f6bd6282d7f7 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -12394,6 +12394,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) > @@ -12561,10 +12563,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); I would add a small comment here, because later-on if someone has doubts (and use git-blame), it won't be visible that something changed (the case and break were added by the same commit). > break; > - > case FENCE_FREE: > { > struct intel_atomic_helper *helper = > @@ -12668,14 +12667,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; > } Reviewed-by: Michel Thierry <michel.thierry@intel.com>
On Thu, Aug 03, 2017 at 12:44:40PM -0700, Michel Thierry wrote: > On 7/20/2017 10:57 AM, Daniel Vetter wrote: > > 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. > > > > 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. > > > > v2: Amend commit message (Chris). > > > > Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > 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(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > index 995522e40ec1..f6bd6282d7f7 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -12394,6 +12394,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) > > @@ -12561,10 +12563,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); > > I would add a small comment here, because later-on if someone has doubts > (and use git-blame), it won't be visible that something changed (the case > and break were added by the same commit). Hm, not sure what comment I should put here? Suggestions? Only thing I could come up with was /* we do blocking waits in the worker, nothing to do here */ But not sure that adds the information you're looking for. -Daniel > > > break; > > - > > case FENCE_FREE: > > { > > struct intel_atomic_helper *helper = > > @@ -12668,14 +12667,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; > > } > > Reviewed-by: Michel Thierry <michel.thierry@intel.com>
On 8/7/2017 8:33 AM, Daniel Vetter wrote: > On Thu, Aug 03, 2017 at 12:44:40PM -0700, Michel Thierry wrote: >> On 7/20/2017 10:57 AM, Daniel Vetter wrote: >>> 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. >>> >>> 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. >>> >>> v2: Amend commit message (Chris). >>> >>> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >>> 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(-) >>> >>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c >>> index 995522e40ec1..f6bd6282d7f7 100644 >>> --- a/drivers/gpu/drm/i915/intel_display.c >>> +++ b/drivers/gpu/drm/i915/intel_display.c >>> @@ -12394,6 +12394,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) >>> @@ -12561,10 +12563,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); >> >> I would add a small comment here, because later-on if someone has doubts >> (and use git-blame), it won't be visible that something changed (the case >> and break were added by the same commit). > > Hm, not sure what comment I should put here? Suggestions? Only thing I > could come up with was > > /* we do blocking waits in the worker, nothing to do here */ > > But not sure that adds the information you're looking for. That sounds good to me, or maybe "any blocking waits already handled in the worker" But I think both are ok. -Michel > >> >>> break; >>> - >>> case FENCE_FREE: >>> { >>> struct intel_atomic_helper *helper = >>> @@ -12668,14 +12667,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; >>> } >> >> Reviewed-by: Michel Thierry <michel.thierry@intel.com> >
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 995522e40ec1..f6bd6282d7f7 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -12394,6 +12394,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) @@ -12561,10 +12563,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 = @@ -12668,14 +12667,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; }