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

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

[Gustavo Padovan]

From: Gustavo Padovan <gustavo.padovan@collabora.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@collabora.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;
+
+		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

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

[Gustavo Padovan]

From: Gustavo Padovan <gustavo.padovan@collabora.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@collabora.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;
+
+		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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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 <gustavo.padovan@collabora.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@collabora.co.uk>
Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

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 <gustavo@padovan.org> wrote:
> From: Gustavo Padovan <gustavo.padovan@collabora.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@collabora.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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Gustavo Padovan]

Hi Sean,

2016-09-12 Sean Paul <seanpaul@google.com>:

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

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 <seanpaul@google.com>:

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

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