| Message ID | 20170830121752.31291-3-maarten.lankhorst@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Aug 30, 2017 at 02:17:49PM +0200, Maarten Lankhorst wrote: > When commit synchronization through drm_crtc_commit was first > introduced, we tried to solve the problem of the flip_done > needing a reference count by blocking in cleanup_done. > > This has been changed by commit 24835e442f28 ("drm: reference count > event->completion") which made the waits here no longer needed. This is wrong. Your next patch makes this no longer necessary. > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch> > --- > drivers/gpu/drm/drm_atomic_helper.c | 17 ----------------- > 1 file changed, 17 deletions(-) > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c > index 4e53aae9a1fb..11d0e94a2181 100644 > --- a/drivers/gpu/drm/drm_atomic_helper.c > +++ b/drivers/gpu/drm/drm_atomic_helper.c > @@ -1885,7 +1885,6 @@ void drm_atomic_helper_commit_cleanup_done(struct drm_atomic_state *old_state) > struct drm_crtc_state *new_crtc_state; > struct drm_crtc_commit *commit; > int i; > - long ret; > > for_each_new_crtc_in_state(old_state, crtc, new_crtc_state, i) { > commit = old_state->crtcs[i].commit; > @@ -1895,22 +1894,6 @@ void drm_atomic_helper_commit_cleanup_done(struct drm_atomic_state *old_state) > complete_all(&commit->cleanup_done); > WARN_ON(!try_wait_for_completion(&commit->hw_done)); > > - /* commit_list borrows our reference, need to remove before we > - * clean up our drm_atomic_state. But only after it actually > - * completed, otherwise subsequent commits won't stall properly. */ This comment here is the clue: "... otherwise subsequent commits wont stall properly". When we remove the drm_crtc_commit from commit_list, the next commit won't be able to properly stall for flip_done. Hence we must wait for flip_done before removing it. But with your new patch we wait for flip_done using old_crtc_state->commit, and this is indeed no longer necessary. With the commit message and patch ordering fixed up: Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > - if (try_wait_for_completion(&commit->flip_done)) > - goto del_commit; > - > - /* We must wait for the vblank event to signal our completion > - * before releasing our reference, since the vblank work does > - * not hold a reference of its own. */ > - ret = wait_for_completion_timeout(&commit->flip_done, > - 10*HZ); > - if (ret == 0) > - DRM_ERROR("[CRTC:%d:%s] flip_done timed out\n", > - crtc->base.id, crtc->name); > - > -del_commit: > spin_lock(&crtc->commit_lock); > list_del(&commit->commit_entry); > spin_unlock(&crtc->commit_lock); > -- > 2.11.0 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 4e53aae9a1fb..11d0e94a2181 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -1885,7 +1885,6 @@ void drm_atomic_helper_commit_cleanup_done(struct drm_atomic_state *old_state) struct drm_crtc_state *new_crtc_state; struct drm_crtc_commit *commit; int i; - long ret; for_each_new_crtc_in_state(old_state, crtc, new_crtc_state, i) { commit = old_state->crtcs[i].commit; @@ -1895,22 +1894,6 @@ void drm_atomic_helper_commit_cleanup_done(struct drm_atomic_state *old_state) complete_all(&commit->cleanup_done); WARN_ON(!try_wait_for_completion(&commit->hw_done)); - /* commit_list borrows our reference, need to remove before we - * clean up our drm_atomic_state. But only after it actually - * completed, otherwise subsequent commits won't stall properly. */ - if (try_wait_for_completion(&commit->flip_done)) - goto del_commit; - - /* We must wait for the vblank event to signal our completion - * before releasing our reference, since the vblank work does - * not hold a reference of its own. */ - ret = wait_for_completion_timeout(&commit->flip_done, - 10*HZ); - if (ret == 0) - DRM_ERROR("[CRTC:%d:%s] flip_done timed out\n", - crtc->base.id, crtc->name); - -del_commit: spin_lock(&crtc->commit_lock); list_del(&commit->commit_entry); spin_unlock(&crtc->commit_lock);
When commit synchronization through drm_crtc_commit was first introduced, we tried to solve the problem of the flip_done needing a reference count by blocking in cleanup_done. This has been changed by commit 24835e442f28 ("drm: reference count event->completion") which made the waits here no longer needed. Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch> --- drivers/gpu/drm/drm_atomic_helper.c | 17 ----------------- 1 file changed, 17 deletions(-)