Message-ID: <CAOw6vbLxyPOCoF4GQSyZEaM1z376grWp=PMRkP+Mv6_n_Jxdkw@mail.gmail.com>
Date:   Mon, 12 Sep 2016 10:13:28 -0400
From:   Sean Paul <seanpaul@...gle.com>
To:     Gustavo Padovan <gustavo@...ovan.org>
Cc:     dri-devel <dri-devel@...ts.freedesktop.org>,
        Stéphane Marchesin <marcheu@...gle.com>,
        Daniel Stone <daniels@...labora.com>,
        Daniel Vetter <daniel.vetter@...ll.ch>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Laurent Pinchart <laurent.pinchart@...asonboard.com>,
        Gustavo Padovan <gustavo.padovan@...labora.co.uk>,
        John Harrison <John.C.Harrison@...el.com>, m.chehab@...sung.com
Subject: Re: [PATCH v3] drm/fence: allow fence waiting to be interrupted by userspace

On Thu, Aug 25, 2016 at 12:47 PM, Gustavo Padovan <gustavo@...ovan.org> wrote:
> From: Gustavo Padovan <gustavo.padovan@...labora.co.uk>
>
> If userspace is running an synchronously atomic commit and interrupts the
> atomic operation during fence_wait() it will hang until the timer expires,
> so here we change the wait to be interruptible so it stop immediately when
> userspace wants to quit.
>
> Also adds the necessary error checking for fence_wait().
>
> v2: Comment by Daniel Vetter
>         - Add error checking for fence_wait()
>
> v3: Rebase on top of new atomic noblocking support
>
> v4: Comment by Maarten Lankhorst
>         - remove 'swapped' bitfield as it was duplicating information
>
> v5: Comments by Maarten Lankhorst
>         - assign plane->state to plane_state if !intr
>         - squash previous patch into this one
>
> Signed-off-by: Gustavo Padovan <gustavo.padovan@...labora.co.uk>
> ---
>  drivers/gpu/drm/drm_atomic_helper.c | 42 +++++++++++++++++++++++++++++--------
>  drivers/gpu/drm/msm/msm_atomic.c    |  2 +-
>  include/drm/drm_atomic_helper.h     |  5 +++--
>  3 files changed, 37 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index e1f5de2..f752f9c 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -1007,29 +1007,47 @@ EXPORT_SYMBOL(drm_atomic_helper_commit_modeset_enables);
>   * drm_atomic_helper_wait_for_fences - wait for fences stashed in plane state
>   * @dev: DRM device
>   * @state: atomic state object with old state structures
> + * @intr: if true, do an interruptible wait
>   *
>   * For implicit sync, driver should fish the exclusive fence out from the
>   * incoming fb's and stash it in the drm_plane_state.  This is called after
>   * drm_atomic_helper_swap_state() so it uses the current plane state (and
>   * just uses the atomic state to find the changed planes)
> + *
> + * Returns zero if sucess or < 0 if fence_wait() fails.
>   */
> -void drm_atomic_helper_wait_for_fences(struct drm_device *dev,
> -                           struct drm_atomic_state *state)
> +int drm_atomic_helper_wait_for_fences(struct drm_device *dev,
> +                                      struct drm_atomic_state *state,
> +                                      bool intr)
>  {
>         struct drm_plane *plane;
>         struct drm_plane_state *plane_state;
> -       int i;
> +       int i, ret;
>
>         for_each_plane_in_state(state, plane, plane_state, i) {
> -               if (!plane->state->fence)
> +               /*
> +                * If the caller asks for an interruptible wait it means
> +                * that the state were not swapped yet and the operation
> +                * can still be interrupted by userspace, so we need
> +                * to look to plane_state instead.
> +                */
> +               if (!intr)
> +                       plane_state = plane->state;

The "intr" name is tripping me up here, since it doesn't _really_ imply that the
state hasn't been swapped yet (this is just an implementation detail
of the caller(s).

Perhaps we could change the name to "pre_swap", and remove the comment
above this conditonal. Below, above
fence_wait, add a new comment to the effect of:

/*
 * If waiting for fences pre-swap (ie: nonblock), userspace can still interrupt
 * the operation. Instead of blocking until the timer expires, make the wait
 * interruptible.
 */


Or perhaps I'm being too pedantic (or missing the point) :)

> +
> +               if (!plane_state->fence)
>                         continue;
>
> -               WARN_ON(!plane->state->fb);
> +               WARN_ON(!plane_state->fb);
> +
> +               ret = fence_wait(plane_state->fence, intr);
> +               if (ret)
> +                       return ret;
>
> -               fence_wait(plane->state->fence, false);
> -               fence_put(plane->state->fence);
> -               plane->state->fence = NULL;
> +               fence_put(plane_state->fence);
> +               plane_state->fence = NULL;
>         }
> +
> +       return 0;
>  }
>  EXPORT_SYMBOL(drm_atomic_helper_wait_for_fences);
>
> @@ -1176,7 +1194,7 @@ static void commit_tail(struct drm_atomic_state *state)
>
>         funcs = dev->mode_config.helper_private;
>
> -       drm_atomic_helper_wait_for_fences(dev, state);
> +       drm_atomic_helper_wait_for_fences(dev, state, false);
>
>         drm_atomic_helper_wait_for_dependencies(state);
>
> @@ -1235,6 +1253,12 @@ int drm_atomic_helper_commit(struct drm_device *dev,
>         if (ret)
>                 return ret;
>
> +       if (!nonblock) {
> +               ret = drm_atomic_helper_wait_for_fences(dev, state, true);
> +               if (ret)
> +                       return ret;
> +       }
> +
>         /*
>          * This is the point of no return - everything below never fails except
>          * when the hw goes bonghits. Which means we can commit the new state on
> diff --git a/drivers/gpu/drm/msm/msm_atomic.c b/drivers/gpu/drm/msm/msm_atomic.c
> index 4a8a6f1..9518e43 100644
> --- a/drivers/gpu/drm/msm/msm_atomic.c
> +++ b/drivers/gpu/drm/msm/msm_atomic.c
> @@ -112,7 +112,7 @@ static void complete_commit(struct msm_commit *c, bool async)
>         struct msm_drm_private *priv = dev->dev_private;
>         struct msm_kms *kms = priv->kms;
>
> -       drm_atomic_helper_wait_for_fences(dev, state);
> +       drm_atomic_helper_wait_for_fences(dev, state, false);
>
>         kms->funcs->prepare_commit(kms, state);
>
> diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
> index d86ae5d..a42c34b 100644
> --- a/include/drm/drm_atomic_helper.h
> +++ b/include/drm/drm_atomic_helper.h
> @@ -43,8 +43,9 @@ int drm_atomic_helper_commit(struct drm_device *dev,
>                              struct drm_atomic_state *state,
>                              bool nonblock);
>
> -void drm_atomic_helper_wait_for_fences(struct drm_device *dev,
> -                                       struct drm_atomic_state *state);
> +int drm_atomic_helper_wait_for_fences(struct drm_device *dev,
> +                                       struct drm_atomic_state *state,
> +                                       bool intr);
>  bool drm_atomic_helper_framebuffer_changed(struct drm_device *dev,
>                                            struct drm_atomic_state *old_state,
>                                            struct drm_crtc *crtc);
> --
> 2.5.5
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@...ts.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Powered by blists - more mailing lists