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

[Michel Thierry <michel.thierry@intel.com>]

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>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx