[PATCH] drm/atomic: Fix bookkeeping with TEST_ONLY, v3.

[PATCH] drm/atomic: Fix bookkeeping with TEST_ONLY, v3.

[Maarten Lankhorst]

Commit ec9f932ed41622d120de52a5b525e4d77b9ef17e
"drm/atomic: Cleanup on error properly in the atomic ioctl."
cleaned up some error paths, but didn't fix the TEST_ONLY path.
In the check only case plane->fb shouldn't be updated, and
the vblank events should be cleared as on failure.

Changes since v1:
- Fix -EDEADLK handling of vblank events too.
- Free state last with CHECK_ONLY.
Changes since v2:
- Add comment about freeing crtc_state->event with TEST_ONLY.
  (Daniel Stone)

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Reviewed-by: Daniel Stone <daniels@collabora.com>
---
 drivers/gpu/drm/drm_atomic.c | 39 +++++++++++++++++++++++----------------
 1 file changed, 23 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index e515261d0a7f..940f80bbf192 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -1540,7 +1540,8 @@ retry:
 			copied_props++;
 		}
 
-		if (obj->type == DRM_MODE_OBJECT_PLANE && count_props) {
+		if (obj->type == DRM_MODE_OBJECT_PLANE && count_props &&
+		    !(arg->flags & DRM_MODE_ATOMIC_TEST_ONLY)) {
 			plane = obj_to_plane(obj);
 			plane_mask |= (1 << drm_plane_index(plane));
 			plane->old_fb = plane->fb;
@@ -1562,10 +1563,11 @@ retry:
 	}
 
 	if (arg->flags & DRM_MODE_ATOMIC_TEST_ONLY) {
+		/*
+		 * Unlike commit, check_only does not clean up state.
+		 * Below we call drm_atomic_state_free for it.
+		 */
 		ret = drm_atomic_check_only(state);
-		/* _check_only() does not free state, unlike _commit() */
-		if (!ret)
-			drm_atomic_state_free(state);
 	} else if (arg->flags & DRM_MODE_ATOMIC_NONBLOCK) {
 		ret = drm_atomic_async_commit(state);
 	} else {
@@ -1592,25 +1594,30 @@ out:
 		plane->old_fb = NULL;
 	}
 
+	if (ret && arg->flags & DRM_MODE_PAGE_FLIP_EVENT) {
+		/*
+		 * TEST_ONLY and PAGE_FLIP_EVENT are mutually exclusive,
+		 * if they weren't, this code should be called on success
+		 * for TEST_ONLY too.
+		 */
+
+		for_each_crtc_in_state(state, crtc, crtc_state, i) {
+			if (!crtc_state->event)
+				continue;
+
+			destroy_vblank_event(dev, file_priv,
+					     crtc_state->event);
+		}
+	}
+
 	if (ret == -EDEADLK) {
 		drm_atomic_state_clear(state);
 		drm_modeset_backoff(&ctx);
 		goto retry;
 	}
 
-	if (ret) {
-		if (arg->flags & DRM_MODE_PAGE_FLIP_EVENT) {
-			for_each_crtc_in_state(state, crtc, crtc_state, i) {
-				if (!crtc_state->event)
-					continue;
-
-				destroy_vblank_event(dev, file_priv,
-						     crtc_state->event);
-			}
-		}
-
+	if (ret || arg->flags & DRM_MODE_ATOMIC_TEST_ONLY)
 		drm_atomic_state_free(state);
-	}
 
 	drm_modeset_drop_locks(&ctx);
 	drm_modeset_acquire_fini(&ctx);
-- 
2.1.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/atomic: Fix bookkeeping with TEST_ONLY, v3.

[Jani Nikula]

On Mon, 31 Aug 2015, Maarten Lankhorst <maarten.lankhorst@linux.intel.com> wrote:
> Commit ec9f932ed41622d120de52a5b525e4d77b9ef17e
> "drm/atomic: Cleanup on error properly in the atomic ioctl."
> cleaned up some error paths, but didn't fix the TEST_ONLY path.
> In the check only case plane->fb shouldn't be updated, and
> the vblank events should be cleared as on failure.
>
> Changes since v1:
> - Fix -EDEADLK handling of vblank events too.
> - Free state last with CHECK_ONLY.
> Changes since v2:
> - Add comment about freeing crtc_state->event with TEST_ONLY.
>   (Daniel Stone)
>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Reviewed-by: Daniel Stone <daniels@collabora.com>

Pushed to our topic/drm-fixes branch, thanks for the patch and review.

BR,
Jani.


> ---
>  drivers/gpu/drm/drm_atomic.c | 39 +++++++++++++++++++++++----------------
>  1 file changed, 23 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index e515261d0a7f..940f80bbf192 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -1540,7 +1540,8 @@ retry:
>  			copied_props++;
>  		}
>  
> -		if (obj->type == DRM_MODE_OBJECT_PLANE && count_props) {
> +		if (obj->type == DRM_MODE_OBJECT_PLANE && count_props &&
> +		    !(arg->flags & DRM_MODE_ATOMIC_TEST_ONLY)) {
>  			plane = obj_to_plane(obj);
>  			plane_mask |= (1 << drm_plane_index(plane));
>  			plane->old_fb = plane->fb;
> @@ -1562,10 +1563,11 @@ retry:
>  	}
>  
>  	if (arg->flags & DRM_MODE_ATOMIC_TEST_ONLY) {
> +		/*
> +		 * Unlike commit, check_only does not clean up state.
> +		 * Below we call drm_atomic_state_free for it.
> +		 */
>  		ret = drm_atomic_check_only(state);
> -		/* _check_only() does not free state, unlike _commit() */
> -		if (!ret)
> -			drm_atomic_state_free(state);
>  	} else if (arg->flags & DRM_MODE_ATOMIC_NONBLOCK) {
>  		ret = drm_atomic_async_commit(state);
>  	} else {
> @@ -1592,25 +1594,30 @@ out:
>  		plane->old_fb = NULL;
>  	}
>  
> +	if (ret && arg->flags & DRM_MODE_PAGE_FLIP_EVENT) {
> +		/*
> +		 * TEST_ONLY and PAGE_FLIP_EVENT are mutually exclusive,
> +		 * if they weren't, this code should be called on success
> +		 * for TEST_ONLY too.
> +		 */
> +
> +		for_each_crtc_in_state(state, crtc, crtc_state, i) {
> +			if (!crtc_state->event)
> +				continue;
> +
> +			destroy_vblank_event(dev, file_priv,
> +					     crtc_state->event);
> +		}
> +	}
> +
>  	if (ret == -EDEADLK) {
>  		drm_atomic_state_clear(state);
>  		drm_modeset_backoff(&ctx);
>  		goto retry;
>  	}
>  
> -	if (ret) {
> -		if (arg->flags & DRM_MODE_PAGE_FLIP_EVENT) {
> -			for_each_crtc_in_state(state, crtc, crtc_state, i) {
> -				if (!crtc_state->event)
> -					continue;
> -
> -				destroy_vblank_event(dev, file_priv,
> -						     crtc_state->event);
> -			}
> -		}
> -
> +	if (ret || arg->flags & DRM_MODE_ATOMIC_TEST_ONLY)
>  		drm_atomic_state_free(state);
> -	}
>  
>  	drm_modeset_drop_locks(&ctx);
>  	drm_modeset_acquire_fini(&ctx);
> -- 
> 2.1.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/atomic: Fix bookkeeping with TEST_ONLY, v3.

[shuang.he]

Tested-By: Intel Graphics QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 7289
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
ILK                 -2              255/255              253/255
SNB                                  248/248              248/248
IVB                                  281/281              281/281
BYT                                  234/234              234/234
HSW                                  317/317              317/317
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
*ILK  igt@gem_fence_thrash@bo-write-verify-threaded-none      PASS(1)      NO_RESULT(1)
*ILK  igt@gem_persistent_relocs@forked-interruptible-faulting-reloc-thrashing      PASS(1)      DMESG_WARN(1)
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx