diff mbox

[v2,2/2] drm/fence: allow fence waiting to be interrupted by userspace

Message ID 1471302341-12824-2-git-send-email-gustavo@padovan.org (mailing list archive)
State New, archived
Headers show

Commit Message

Gustavo Padovan Aug. 15, 2016, 11:05 p.m. UTC
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

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
---
 drivers/gpu/drm/drm_atomic_helper.c | 33 ++++++++++++++++++++++++++++-----
 1 file changed, 28 insertions(+), 5 deletions(-)

Comments

Maarten Lankhorst Aug. 18, 2016, 8:28 a.m. UTC | #1
Op 16-08-16 om 01:05 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
>
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
I think it would make sense to squash this patch with 1/2.
> ---
>  drivers/gpu/drm/drm_atomic_helper.c | 33 ++++++++++++++++++++++++++++-----
>  1 file changed, 28 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index bf1bc43..292d15b 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -1022,20 +1022,37 @@ int drm_atomic_helper_wait_for_fences(struct drm_device *dev,
>  {
>  	struct drm_plane *plane;
>  	struct drm_plane_state *plane_state;
> +	struct fence *fence;
>  	int i, ret;
>  
>  	for_each_plane_in_state(state, plane, plane_state, i) {
> -		if (!plane->state->fence)
> +		if (!plane->state->fence && !plane_state->fence)
>  			continue;
Move this check to below, or put if (!intr) plane_state = plane->state on top, and only use plane_state.

Daniel, I keep thinking more and more that a for_each_XXX_(old,new,both)_state would be useful,
could we add those perhaps?
> -		WARN_ON(!plane->state->fb);
> +		/*
> +		 * 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) {
> +			fence = plane_state->fence;
> +			WARN_ON(!plane_state->fb);
> +		} else {
> +			fence = plane->state->fence;
> +			WARN_ON(!plane->state->fb);
> +		}
^ if (!fence) continue;
> -		ret = fence_wait(plane->state->fence, intr);
> +		ret = fence_wait(fence, intr);
>  		if (ret)
>  			return ret;
>  
> -		fence_put(plane->state->fence);
> -		plane->state->fence = NULL;
> +		fence_put(fence);
> +
> +		if (intr)
> +			plane_state->fence = NULL;
> +		else
> +			plane->state->fence = NULL;
>  	}
>  
>  	return 0;
> @@ -1244,6 +1261,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 mbox

Patch

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index bf1bc43..292d15b 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -1022,20 +1022,37 @@  int drm_atomic_helper_wait_for_fences(struct drm_device *dev,
 {
 	struct drm_plane *plane;
 	struct drm_plane_state *plane_state;
+	struct fence *fence;
 	int i, ret;
 
 	for_each_plane_in_state(state, plane, plane_state, i) {
-		if (!plane->state->fence)
+		if (!plane->state->fence && !plane_state->fence)
 			continue;
 
-		WARN_ON(!plane->state->fb);
+		/*
+		 * 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) {
+			fence = plane_state->fence;
+			WARN_ON(!plane_state->fb);
+		} else {
+			fence = plane->state->fence;
+			WARN_ON(!plane->state->fb);
+		}
 
-		ret = fence_wait(plane->state->fence, intr);
+		ret = fence_wait(fence, intr);
 		if (ret)
 			return ret;
 
-		fence_put(plane->state->fence);
-		plane->state->fence = NULL;
+		fence_put(fence);
+
+		if (intr)
+			plane_state->fence = NULL;
+		else
+			plane->state->fence = NULL;
 	}
 
 	return 0;
@@ -1244,6 +1261,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