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@xxxxxxxxxxxxxxx>
> > Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> > Cc: Mika Kuoppala <mika.kuoppala@xxxxxxxxx>
> > Cc: Joonas Lahtinen <joonas.lahtinen@xxxxxxxxxxxxxxx>
> > Signed-off-by: Daniel Vetter <daniel.vetter@xxxxxxxxx>
> > ---
> > 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@xxxxxxxxx>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
