Re: [PATCH v3] drm/fence: allow fence waiting to be interrupted by userspace

[Gustavo Padovan]

Hi Sean,

2016-09-12 Sean Paul :

> On Thu, Aug 25, 2016 at 12:47 PM, Gustavo Padovan  wrote:
> > From: Gustavo Padovan 
> >
> > 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 
> > ---
> >  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.
>  */

Indeed, that is much better naming. In this case specifically intr has
the same meaning as pre_swap, but as you said it is an implementation
detail and we need to avoid people to get it wrong.

I'll update it and send a v4.

Gustavo

Re: [PATCH v3] drm/fence: allow fence waiting to be interrupted by userspace

[Gustavo Padovan]

Hi Sean,

2016-09-12 Sean Paul :

> On Thu, Aug 25, 2016 at 12:47 PM, Gustavo Padovan  wrote:
> > From: Gustavo Padovan 
> >
> > 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 
> > ---
> >  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.
>  */

Indeed, that is much better naming. In this case specifically intr has
the same meaning as pre_swap, but as you said it is an implementation
detail and we need to avoid people to get it wrong.

I'll update it and send a v4.

Gustavo

Re: [PATCH v3] drm/fence: allow fence waiting to be interrupted by userspace

[Sean Paul]

On Thu, Aug 25, 2016 at 12:47 PM, Gustavo Padovan  wrote:
> From: Gustavo Padovan 
>
> 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 
> ---
>  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 

Re: [PATCH v3] drm/fence: allow fence waiting to be interrupted by userspace

[Sean Paul]

On Thu, Aug 25, 2016 at 12:47 PM, Gustavo Padovan  wrote:
> From: Gustavo Padovan 
>
> 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 
> ---
>  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 
> 

Re: [PATCH v3] drm/fence: allow fence waiting to be interrupted by userspace

[Maarten Lankhorst]

Op 25-08-16 om 18:47 schreef Gustavo Padovan:
> From: Gustavo Padovan 
>
> 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 
Reviewed-by: Maarten Lankhorst 

Re: [PATCH v3] drm/fence: allow fence waiting to be interrupted by userspace

[Maarten Lankhorst]

Op 25-08-16 om 18:47 schreef Gustavo Padovan:
> From: Gustavo Padovan 
>
> 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 
Reviewed-by: Maarten Lankhorst