[Intel-gfx] [PATCH 2/7] drm/i915: Push i915_sw_fence_wait into the nonblocking atomic commit

[Michel Thierry]

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 at linux.intel.com>
>>> Cc: Chris Wilson <chris at chris-wilson.co.uk>
>>> Cc: Mika Kuoppala <mika.kuoppala at intel.com>
>>> Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
>>> Signed-off-by: Daniel Vetter <daniel.vetter at 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 at intel.com>
>