drm/atomic: Fix bookkeeping with TEST_ONLY.

PATCH Superseded Maarten Lankhorst 4 participants 16 messages Aug 4, 2015 – Aug 31, 2015

Subsystems (2)

DRM drivers DRM misc

lore

Share & tools

lore

Maarten Lankhorst

author maintainer

2015-08-04 11:34 UTC #0 lore raw

PATCH 1/1 drm/atomic: Fix bookkeeping with TEST_ONLY.

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.

Signed-off-by Maarten Lankhorst <[email protected]>

---

drivers/gpu/drm/drm_atomic.c +4 -2

index d719ce0b10a0..03672a782e84 100644
@@ -1507,7 +1507,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;

@@ -1532,7 +1533,7 @@ retry:
 		ret = drm_atomic_check_only(state);
 		/* _check_only() does not free state, unlike _commit() */
 		if (!ret)
-			drm_atomic_state_free(state);
+			goto free;
 	} else if (arg->flags & DRM_MODE_ATOMIC_NONBLOCK) {
 		ret = drm_atomic_async_commit(state);
 	} else {

@@ -1566,6 +1567,7 @@ out:
 	}
 
 	if (ret) {
+free:
 		if (arg->flags & DRM_MODE_PAGE_FLIP_EVENT) {
 			for_each_crtc_in_state(state, crtc, crtc_state, i) {
 				if (!crtc_state->event)

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[email protected]

Tested-by on v1 1/1

2015-08-11 10:03 UTC #1 lore raw

Tested-by Intel Graphics QA PRTS (Patch Regression Test System Contact: [email protected])

Task id: 6996 -------------------------------------Summary-------------------------------------
Platform Delta drm-intel-nightly Series Applied
ILK -2 302/302 300/302

SNB                                  315/315              315/315
IVB                                  336/336              336/336
BYT                                  283/283              283/283
HSW                                  378/378              378/378

-------------------------------------Detailed-------------------------------------
Platform Test drm-intel-nightly Series Applied
*ILK igt@kms_flip@flip-vs-dpms-interruptible PASS(1) DMESG_WARN(1)
*ILK igt@kms_flip@wf_vblank-vs-modeset-interruptible PASS(1) DMESG_WARN(1)
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Daniel Stone

2015-08-27 12:19 UTC #2 lore raw

In reply to Maarten Lankhorst

Hi,

Quoted 5 lines · 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.

Bikeshedding a bit ...

An early test precludes TEST_ONLY | PAGE_FLIP_EVENT, so you don't need to mention this in the commit message; in this case, the main change is about plane->{,old_}fb.

Quoted 10 lines

drivers/gpu/drm/drm_atomic.c +1 -1

@@ -1532,7 +1533,7 @@ retry:
                ret = drm_atomic_check_only(state);
                /* _check_only() does not free state, unlike _commit() */
                if (!ret)
-                       drm_atomic_state_free(state);
+                       goto free;
        } else if (arg->flags & DRM_MODE_ATOMIC_NONBLOCK) {
                ret = drm_atomic_async_commit(state);
        } else {

drivers/gpu/drm/drm_atomic.c out: · line 1570

Quoted 4 lines

drivers/gpu/drm/drm_atomic.c +1 -0

@@ -1566,6 +1567,7 @@ out:
        }
 
        if (ret) {
+free:

This is a bit nasty. Can we please move the label above the conditional and change the condition to (ret || flags & TEST_ONLY)? Doing that, you could also move the label above the (ret == -EDEADLK) check, which would cover ->atomic_check needing to grab other states (global resources?) and failing.

Cheers,
Daniel
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Maarten Lankhorst

author maintainer

2015-08-27 12:43 UTC #3 lore raw

PATCH v2 1/1 drm/atomic: Fix bookkeeping with TEST_ONLY, v2.

Op 27-08-15 om 14:19 schreef Daniel Stone:

Quoted 9 lines

Hi,
Quoted 5 lines · 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.

Quoted 5 lines

Bikeshedding a bit ...
An early test precludes TEST_ONLY | PAGE_FLIP_EVENT, so you don't need to mention this in the commit message; in this case, the main change is about plane->{,old_}fb.

Even testing with PAGE_FLIP_EVENT would be useful because event && !crtc_state->active should not be allowed. In that case test could succeed but commit could fail.

Though I guess you're right and it's currently not allowed.

Quoted 12 lines

(quoted diff) +1 -1

@@ -1532,7 +1533,7 @@ retry:
                ret = drm_atomic_check_only(state);
                /* _check_only() does not free state, unlike _commit() */
                if (!ret)
-                       drm_atomic_state_free(state);
+                       goto free;
        } else if (arg->flags & DRM_MODE_ATOMIC_NONBLOCK) {
                ret = drm_atomic_async_commit(state);
        } else {

@@ -1566,6 +1567,7 @@ out:
        }

Quoted 7 lines

Review out: · line 1570
Quoted 2 lines
(quoted diff) +1 -0
        if (ret) {
+free:
This is a bit nasty. Can we please move the label above the conditional and change the condition to (ret || flags & TEST_ONLY)? Doing that, you could also move the label above the (ret == -EDEADLK) check, which would cover ->atomic_check needing to grab other states (global resources?) and failing.

If our bookkeeping is correct then it won't be harmful to fixup old_fb. Cleaning up more old_fb for more planes than initially had fb updates can always happen anyway, because a modeset will add all affected planes.

How about the below patch? Apply with git am --scissors

-------->8------
Commit ec9f932ed41622d120de52a5b525e4d77b9ef17e "drm/atomic: Cleanup on error properly in the atomic ioctl." cleaned up some error paths, but allowed -EDEADLK to leak vblank events.

Additionally check_only was updating plane->fb, which should not be done when checking a new configuration only.

Signed-off-by Maarten Lankhorst <[email protected]>

---

drivers/gpu/drm/drm_atomic.c +17 ~~-16~~

index c2448f42480f..78ffb4965548 100644
@@ -1542,7 +1542,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;

@@ -1564,10 +1565,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 {

@@ -1594,25 +1596,24 @@ out:
 		plane->old_fb = NULL;
 	}
 
+	if (ret && 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 == -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);

_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Ville Syrjälä

2015-08-27 12:48 UTC #4 lore raw

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

Quoted 10 lines · Maarten Lankhorst

Op 27-08-15 om 14:19 schreef Daniel Stone:
Quoted 9 lines
Hi,
Quoted 5 lines · 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.

Quoted 8 lines

Quoted 5 lines
Bikeshedding a bit ...
An early test precludes TEST_ONLY | PAGE_FLIP_EVENT, so you don't need to mention this in the commit message; in this case, the main change is about plane->{,old_}fb.
Even testing with PAGE_FLIP_EVENT would be useful because event && !crtc_state->active should not be allowed. In that case test could succeed but commit could fail.

Why would commit fail when the we're in DPMS off? I would suggest it should be allowed. The operation would just a be a nop from a HW point of view, all the calculation/checks would still be performed.

Quoted 109 lines

Though I guess you're right and it's currently not allowed.
Quoted 12 lines
Quoted 12 lines
drivers/gpu/drm/drm_atomic.c +1 -1
@@ -1532,7 +1533,7 @@ retry:
                ret = drm_atomic_check_only(state);
                /* _check_only() does not free state, unlike _commit() */
                if (!ret)
-                       drm_atomic_state_free(state);
+                       goto free;
        } else if (arg->flags & DRM_MODE_ATOMIC_NONBLOCK) {
                ret = drm_atomic_async_commit(state);
        } else {
@@ -1566,6 +1567,7 @@ out:
        }
 
Quoted 7 lines
Review out: · line 1570
Quoted 2 lines
(quoted diff) +1 -0
        if (ret) {
+free:
This is a bit nasty. Can we please move the label above the conditional and change the condition to (ret || flags & TEST_ONLY)? Doing that, you could also move the label above the (ret == -EDEADLK) check, which would cover ->atomic_check needing to grab other states (global resources?) and failing.
If our bookkeeping is correct then it won't be harmful to fixup old_fb. Cleaning up more old_fb for more planes than initially had fb updates can always happen anyway, because a modeset will add all affected planes.
How about the below patch? Apply with git am --scissors
-------->8------
Commit ec9f932ed41622d120de52a5b525e4d77b9ef17e "drm/atomic: Cleanup on error properly in the atomic ioctl." cleaned up some error paths, but allowed -EDEADLK to leak vblank events.
Additionally check_only was updating plane->fb, which should not be done when checking a new configuration only.
Signed-off-by Maarten Lankhorst <[email protected]>
---
drivers/gpu/drm/drm_atomic.c +17 ~~-16~~
index c2448f42480f..78ffb4965548 100644
@@ -1542,7 +1542,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;
@@ -1564,10 +1565,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 {
@@ -1594,25 +1596,24 @@ out:
 		plane->old_fb = NULL;
 	}
 
+	if (ret && 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 == -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);
_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Signature · 6 lines

Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Maarten Lankhorst

author maintainer

2015-08-27 12:50 UTC #5 lore raw

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

Op 27-08-15 om 14:48 schreef Ville Syrjälä:

Quoted 16 lines · Maarten Lankhorst

Quoted 15 lines
Op 27-08-15 om 14:19 schreef Daniel Stone:
Quoted 14 lines
Hi,
Quoted 5 lines · 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.
Bikeshedding a bit ...
An early test precludes TEST_ONLY | PAGE_FLIP_EVENT, so you don't need to mention this in the commit message; in this case, the main change is about plane->{,old_}fb.

Quoted 7 lines

Quoted 3 lines
Even testing with PAGE_FLIP_EVENT would be useful because event && !crtc_state->active should not be allowed. In that case test could succeed but commit could fail.
Why would commit fail when the we're in DPMS off? I would suggest it should be allowed. The operation would just a be a nop from a HW point of view, all the calculation/checks would still be performed.

You can commit, just not with PAGE_FLIP_EVENT set when crtc is inactive.

~Maarten
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Ville Syrjälä

2015-08-27 12:52 UTC #6 lore raw

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

Quoted 17 lines · Maarten Lankhorst

Op 27-08-15 om 14:48 schreef Ville Syrjälä:
Quoted 16 lines · Maarten Lankhorst
Quoted 15 lines
Op 27-08-15 om 14:19 schreef Daniel Stone:
Quoted 14 lines
Hi,
Quoted 5 lines · 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.
Bikeshedding a bit ...
An early test precludes TEST_ONLY | PAGE_FLIP_EVENT, so you don't need to mention this in the commit message; in this case, the main change is about plane->{,old_}fb.

Quoted 8 lines

Quoted 7 lines
Quoted 3 lines
Even testing with PAGE_FLIP_EVENT would be useful because event && !crtc_state->active should not be allowed. In that case test could succeed but commit could fail.
Why would commit fail when the we're in DPMS off? I would suggest it should be allowed. The operation would just a be a nop from a HW point of view, all the calculation/checks would still be performed.
You can commit, just not with PAGE_FLIP_EVENT set when crtc is inactive.

What's so special about the event here? Just send it out as soon as the state has been swapped.

Signature · 6 lines

Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Maarten Lankhorst

author maintainer

2015-08-27 13:05 UTC #7 lore raw

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

Op 27-08-15 om 14:52 schreef Ville Syrjälä:

Quoted 21 lines · Maarten Lankhorst

Quoted 20 lines
Op 27-08-15 om 14:48 schreef Ville Syrjälä:
Quoted 19 lines · Maarten Lankhorst
Quoted 18 lines
Op 27-08-15 om 14:19 schreef Daniel Stone:
Quoted 9 lines
Hi,
Quoted 5 lines · 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.
Quoted 5 lines
Bikeshedding a bit ...
An early test precludes TEST_ONLY | PAGE_FLIP_EVENT, so you don't need to mention this in the commit message; in this case, the main change is about plane->{,old_}fb.
Even testing with PAGE_FLIP_EVENT would be useful because event && !crtc_state->active should not be allowed. In that case test could succeed but commit could fail.

Quoted 7 lines

Quoted 5 lines
Quoted 4 lines
Why would commit fail when the we're in DPMS off? I would suggest it should be allowed. The operation would just a be a nop from a HW point of view, all the calculation/checks would still be performed.
You can commit, just not with PAGE_FLIP_EVENT set when crtc is inactive.
What's so special about the event here? Just send it out as soon as the state has been swapped.

Previously this has been disallowed for legacy page flips. I don't see why this should be relaxed. It just complicates things and you have nothing to stick in for the vblank counter.

~Maarten
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Daniel Stone

Reviewed-by on v2 1/1

2015-08-27 13:34 UTC #14 lore raw

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

In reply to Maarten Lankhorst

Hi,

Quoted 9 lines · Maarten Lankhorst

Op 27-08-15 om 14:19 schreef Daniel Stone:
Quoted 5 lines
On 4 August 2015 at 12:34, Maarten Lankhorst <[email protected]> wrote:
An early test precludes TEST_ONLY | PAGE_FLIP_EVENT, so you don't need to mention this in the commit message; in this case, the main change is about plane->{,old_}fb.
Even testing with PAGE_FLIP_EVENT would be useful because event && !crtc_state->active should not be allowed. In that case test could succeed but commit could fail.

Will reply to that in the (very old) thread.

Quoted 8 lines

Quoted 5 lines
This is a bit nasty. Can we please move the label above the conditional and change the condition to (ret || flags & TEST_ONLY)? Doing that, you could also move the label above the (ret == -EDEADLK) check, which would cover ->atomic_check needing to grab other states (global resources?) and failing.
If our bookkeeping is correct then it won't be harmful to fixup old_fb. Cleaning up more old_fb for more planes than initially had fb updates can always happen anyway, because a modeset will add all affected planes.

It won't happen: plane_mask will always be 0, because it's only set in the branch which is now !TEST_ONLY. So yeah, it's safe to just skip the label completely.

Quoted 45 lines

How about the below patch? Apply with git am --scissors

Additionally check_only was updating plane->fb, which should not be done when checking a new configuration only.

Signed-off-by Maarten Lankhorst <[email protected]>

---

drivers/gpu/drm/drm_atomic.c +6 -4

index c2448f42480f..78ffb4965548 100644
@@ -1542,7 +1542,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;

@@ -1564,10 +1565,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 {

@@ -1594,25 +1596,24 @@ out:
                plane->old_fb = NULL;
        }

drivers/gpu/drm/drm_atomic.c out: · line 1607

Quoted 10 lines

drivers/gpu/drm/drm_atomic.c +9 -0

 
+       if (ret && 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);
+               }
+       }

Yeah, moving this looks good for fixing the -EDEADLK leak. So, great. Can you please add a comment above though noting that we don't need to call this branch on (ret == 0 && flags & DRM_MODE_ATOMIC_TEST_ONLY), as we do for freeing the state, because TEST_ONLY and PAGE_FLIP_EVENT are mutually exclusive? Otherwise the next person to come along and have the same idea of allowing them is probably going to break this. :P

With that though, feel free to send it directly with:

Reviewed-by Daniel Stone <[email protected]>

Cheers,
Daniel
_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Ville Syrjälä

2015-08-27 13:50 UTC #8 lore raw

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

In reply to Maarten Lankhorst

Quoted 22 lines · Maarten Lankhorst

Op 27-08-15 om 14:52 schreef Ville Syrjälä:
Quoted 21 lines · Maarten Lankhorst
Quoted 20 lines
Op 27-08-15 om 14:48 schreef Ville Syrjälä:
Quoted 19 lines · Maarten Lankhorst
Quoted 18 lines
Op 27-08-15 om 14:19 schreef Daniel Stone:
Quoted 9 lines
Hi,
Quoted 5 lines · 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.
Quoted 5 lines
Bikeshedding a bit ...
An early test precludes TEST_ONLY | PAGE_FLIP_EVENT, so you don't need to mention this in the commit message; in this case, the main change is about plane->{,old_}fb.
Even testing with PAGE_FLIP_EVENT would be useful because event && !crtc_state->active should not be allowed. In that case test could succeed but commit could fail.

Quoted 8 lines

Quoted 7 lines
Quoted 5 lines
Quoted 4 lines
Why would commit fail when the we're in DPMS off? I would suggest it should be allowed. The operation would just a be a nop from a HW point of view, all the calculation/checks would still be performed.
You can commit, just not with PAGE_FLIP_EVENT set when crtc is inactive.
What's so special about the event here? Just send it out as soon as the state has been swapped.
Previously this has been disallowed for legacy page flips.

I don't think so. Speaking for i915, I think we've just rejected legacy page flips entirely with the pipe is off on account of drm_vblank_get() failing.

Quoted 1 line

I don't see why this should be relaxed. It just complicates things and you have nothing to stick in for the vblank counter.

We could stick the last vbl count/timestamp in there.

Not allowing means userspace is forced to consider the dpms state whenever it wants to call the atomic ioctl.

Signature · 6 lines

Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Maarten Lankhorst

author maintainer

2015-08-27 14:00 UTC #9 lore raw

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

Op 27-08-15 om 15:50 schreef Ville Syrjälä:

Quoted 27 lines · Maarten Lankhorst

Quoted 26 lines
Op 27-08-15 om 14:52 schreef Ville Syrjälä:
Quoted 25 lines · Maarten Lankhorst
Quoted 24 lines
Op 27-08-15 om 14:48 schreef Ville Syrjälä:
Quoted 23 lines · Maarten Lankhorst
Quoted 10 lines
Op 27-08-15 om 14:19 schreef Daniel Stone:
Quoted 9 lines
Hi,
Quoted 5 lines · 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.
Quoted 8 lines
Quoted 5 lines
Bikeshedding a bit ...
An early test precludes TEST_ONLY | PAGE_FLIP_EVENT, so you don't need to mention this in the commit message; in this case, the main change is about plane->{,old_}fb.
Even testing with PAGE_FLIP_EVENT would be useful because event && !crtc_state->active should not be allowed. In that case test could succeed but commit could fail.
Why would commit fail when the we're in DPMS off? I would suggest it should be allowed. The operation would just a be a nop from a HW point of view, all the calculation/checks would still be performed.

Quoted 6 lines

Quoted 4 lines
Quoted 3 lines
Quoted 1 line
You can commit, just not with PAGE_FLIP_EVENT set when crtc is inactive.
What's so special about the event here? Just send it out as soon as the state has been swapped.
Previously this has been disallowed for legacy page flips.
I don't think so. Speaking for i915, I think we've just rejected legacy page flips entirely with the pipe is off on account of drm_vblank_get() failing.

No atomic driver handles this case correctly. You can't get vblank events with the crtc off.

Quoted 5 lines

Quoted 1 line
I don't see why this should be relaxed. It just complicates things and you have nothing to stick in for the vblank counter.
We could stick the last vbl count/timestamp in there.
Not allowing means userspace is forced to consider the dpms state whenever it wants to call the atomic ioctl.

Userspace was the one turning off the crtc in the first place; it shouldn't continue flipping but preserve power. :-)

~Maarten
_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Ville Syrjälä

2015-08-27 14:09 UTC #10 lore raw

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

Quoted 28 lines · Maarten Lankhorst

Op 27-08-15 om 15:50 schreef Ville Syrjälä:
Quoted 27 lines · Maarten Lankhorst
Quoted 26 lines
Op 27-08-15 om 14:52 schreef Ville Syrjälä:
Quoted 25 lines · Maarten Lankhorst
Quoted 24 lines
Op 27-08-15 om 14:48 schreef Ville Syrjälä:
Quoted 23 lines · Maarten Lankhorst
Quoted 10 lines
Op 27-08-15 om 14:19 schreef Daniel Stone:
Quoted 9 lines
Hi,
Quoted 5 lines · 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.
Quoted 8 lines
Quoted 5 lines
Bikeshedding a bit ...
An early test precludes TEST_ONLY | PAGE_FLIP_EVENT, so you don't need to mention this in the commit message; in this case, the main change is about plane->{,old_}fb.
Even testing with PAGE_FLIP_EVENT would be useful because event && !crtc_state->active should not be allowed. In that case test could succeed but commit could fail.
Why would commit fail when the we're in DPMS off? I would suggest it should be allowed. The operation would just a be a nop from a HW point of view, all the calculation/checks would still be performed.

Quoted 7 lines

Quoted 6 lines
Quoted 4 lines
Quoted 3 lines
Quoted 1 line
You can commit, just not with PAGE_FLIP_EVENT set when crtc is inactive.
What's so special about the event here? Just send it out as soon as the state has been swapped.
Previously this has been disallowed for legacy page flips.
I don't think so. Speaking for i915, I think we've just rejected legacy page flips entirely with the pipe is off on account of drm_vblank_get() failing.
No atomic driver handles this case correctly. You can't get vblank events with the crtc off.

I don't understand what you're saying. Should there be a comma after "No" ? If not, then I'm not sure what they don't handle.

Quoted 6 lines

Quoted 5 lines
Quoted 1 line
I don't see why this should be relaxed. It just complicates things and you have nothing to stick in for the vblank counter.
We could stick the last vbl count/timestamp in there.
Not allowing means userspace is forced to consider the dpms state whenever it wants to call the atomic ioctl.
Userspace was the one turning off the crtc in the first place; it shouldn't continue flipping but preserve power. :-)

Meh. Much simpler to write code when you don't have to worry about such details.

In the kernel it should amount to
if (!pipe_active)
send_event

We anyway need something like that for the crtc getting disabled case, don't we?

Signature · 6 lines

Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Daniel Stone

2015-08-27 14:28 UTC #11 lore raw

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

Hi,

Quoted 8 lines · Ville Syrjälä

Quoted 4 lines · Maarten Lankhorst
Op 27-08-15 om 15:50 schreef Ville Syrjälä:
Quoted 2 lines
I don't think so. Speaking for i915, I think we've just rejected legacy page flips entirely with the pipe is off on account of drm_vblank_get() failing.
No atomic driver handles this case correctly. You can't get vblank events with the crtc off.
I don't understand what you're saying. Should there be a comma after "No" ? If not, then I'm not sure what they don't handle.

I think you're arguing over the definition of 'correctly' perhaps?

Quoted 5 lines

Quoted 5 lines
Quoted 5 lines
Quoted 1 line
I don't see why this should be relaxed. It just complicates things and you have nothing to stick in for the vblank counter.
We could stick the last vbl count/timestamp in there.
Not allowing means userspace is forced to consider the dpms state whenever it wants to call the atomic ioctl.

Quoted 8 lines

Quoted 1 line
Userspace was the one turning off the crtc in the first place; it shouldn't continue flipping but preserve power. :-)
Meh. Much simpler to write code when you don't have to worry about such details.
In the kernel it should amount to
if (!pipe_active)
send_event

No, thankyou. Asking for an event, having the request succeed, and never getting an event, is a deathtrap. PAGE_FLIP_EVENT should mean that either an event gets delivered for every CRTC in crtc_state, or the request getting rejected. Nothing else.

Quoted 2 lines

We anyway need something like that for the crtc getting disabled case, don't we?

Yes, which I was getting at previously. We know when the CRTC gets disabled - we have to, in order to sensibly unpin etc - so that's when the event gets sent.

Cheers,
Daniel
_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Ville Syrjälä

2015-08-27 14:34 UTC #12 lore raw

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

Quoted 24 lines · Daniel Stone

Hi,
Quoted 8 lines · Ville Syrjälä
Quoted 4 lines · Maarten Lankhorst
Op 27-08-15 om 15:50 schreef Ville Syrjälä:
Quoted 2 lines
I don't think so. Speaking for i915, I think we've just rejected legacy page flips entirely with the pipe is off on account of drm_vblank_get() failing.
No atomic driver handles this case correctly. You can't get vblank events with the crtc off.
I don't understand what you're saying. Should there be a comma after "No" ? If not, then I'm not sure what they don't handle.
I think you're arguing over the definition of 'correctly' perhaps?
Quoted 10 lines
Quoted 6 lines
Quoted 5 lines
Quoted 1 line
I don't see why this should be relaxed. It just complicates things and you have nothing to stick in for the vblank counter.
We could stick the last vbl count/timestamp in there.
Not allowing means userspace is forced to consider the dpms state whenever it wants to call the atomic ioctl.
Userspace was the one turning off the crtc in the first place; it shouldn't continue flipping but preserve power. :-)
Meh. Much simpler to write code when you don't have to worry about such details.

Quoted 6 lines

Quoted 3 lines
In the kernel it should amount to
if (!pipe_active)
send_event
No, thankyou. Asking for an event, having the request succeed, and never getting an event, is a deathtrap.

Obviously the event gets sent if the operation succeeds. I never suggested anything else.

Quoted 13 lines

PAGE_FLIP_EVENT should mean
that either an event gets delivered for every CRTC in crtc_state, or the request getting rejected. Nothing else.
Quoted 2 lines
We anyway need something like that for the crtc getting disabled case, don't we?
Yes, which I was getting at previously. We know when the CRTC gets disabled - we have to, in order to sensibly unpin etc - so that's when the event gets sent.
Cheers,
Daniel

Signature · 6 lines

Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Daniel Stone

2015-08-27 14:42 UTC #13 lore raw

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

Quoted 11 lines · Ville Syrjälä

Quoted 7 lines · Daniel Stone
Quoted 3 lines · Ville Syrjälä
In the kernel it should amount to
if (!pipe_active)
send_event
No, thankyou. Asking for an event, having the request succeed, and never getting an event, is a deathtrap.
Obviously the event gets sent if the operation succeeds. I never suggested anything else.

Er, yeah. Totally missed the ! in the condition. As you were.

Cheers,
Daniel
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[email protected]

Tested-by on v2 1/1

2015-08-31 13:56 UTC #15 lore raw

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

In reply to Maarten Lankhorst

Tested-by Intel Graphics QA PRTS (Patch Regression Test System Contact: [email protected])

Task id: 7274 -------------------------------------Summary-------------------------------------
Platform Delta drm-intel-nightly Series Applied
ILK -1 283/283 282/283

SNB                                  315/315              315/315
IVB                                  336/336              336/336
BYT                                  283/283              283/283
HSW                                  378/378              378/378

-------------------------------------Detailed-------------------------------------
Platform Test drm-intel-nightly Series Applied
*ILK igt@kms_flip@flip-vs-dpms-interruptible PASS(1) DMESG_WARN(1)
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx