Op 27-08-15 om 14:19 schreef Daniel Stone:
> Hi,
>
> On 4 August 2015 at 12:34, Maarten Lankhorst
> <maarten.lankhorst at 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.
> 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.
>> @@ -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:
> 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 <maarten.lankhorst at linux.intel.com>
---
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index c2448f42480f..78ffb4965548 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -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);
