Hi,
On 27 August 2015 at 13:43, Maarten Lankhorst maarten.lankhorst@linux.intel.com wrote:
Op 27-08-15 om 14:19 schreef Daniel Stone:
On 4 August 2015 at 12:34, Maarten Lankhorst maarten.lankhorst@linux.intel.com 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.
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.
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@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);}}
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 daniels@collabora.com
Cheers, Daniel