Message ID | 20170621091627.30837-1-daniel.vetter@ffwll.ch (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
On Wed, Jun 21, 2017 at 11:16:27AM +0200, Daniel Vetter wrote: > The crtc->commit_lock only protects commit_list and commit_entry. If > we chase the pointer from the drm_atomic_state update structure, then > we don't need any locks (since we hold a reference already). > > Simplify the locking accordingly. > > Noticed while reviewing a patch from Boris. > > Cc: Boris Brezillon <boris.brezillon@free-electrons.com> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> After reading this over a few times, I've convinced myself that it does the right thing. Reviewed-by: Sean Paul <seanpaul@chromium.org> > --- > drivers/gpu/drm/drm_atomic_helper.c | 7 +------ > 1 file changed, 1 insertion(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c > index 86d3093c6c9b..9bf86eb87fe1 100644 > --- a/drivers/gpu/drm/drm_atomic_helper.c > +++ b/drivers/gpu/drm/drm_atomic_helper.c > @@ -1680,9 +1680,7 @@ void drm_atomic_helper_commit_hw_done(struct drm_atomic_state *old_state) > > /* backend must have consumed any event by now */ > WARN_ON(new_crtc_state->event); > - spin_lock(&crtc->commit_lock); > complete_all(&commit->hw_done); > - spin_unlock(&crtc->commit_lock); > } > } > EXPORT_SYMBOL(drm_atomic_helper_commit_hw_done); > @@ -1711,7 +1709,6 @@ void drm_atomic_helper_commit_cleanup_done(struct drm_atomic_state *old_state) > if (WARN_ON(!commit)) > continue; > > - spin_lock(&crtc->commit_lock); > complete_all(&commit->cleanup_done); > WARN_ON(!try_wait_for_completion(&commit->hw_done)); > > @@ -1721,8 +1718,6 @@ void drm_atomic_helper_commit_cleanup_done(struct drm_atomic_state *old_state) > if (try_wait_for_completion(&commit->flip_done)) > goto del_commit; > > - spin_unlock(&crtc->commit_lock); > - > /* 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. */ > @@ -1732,8 +1727,8 @@ void drm_atomic_helper_commit_cleanup_done(struct drm_atomic_state *old_state) > DRM_ERROR("[CRTC:%d:%s] flip_done timed out\n", > crtc->base.id, crtc->name); > > - spin_lock(&crtc->commit_lock); > del_commit: > + spin_lock(&crtc->commit_lock); > list_del(&commit->commit_entry); > spin_unlock(&crtc->commit_lock); > } > -- > 2.11.0 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Wed, Jun 21, 2017 at 10:30:51AM -0400, Sean Paul wrote: > On Wed, Jun 21, 2017 at 11:16:27AM +0200, Daniel Vetter wrote: > > The crtc->commit_lock only protects commit_list and commit_entry. If > > we chase the pointer from the drm_atomic_state update structure, then > > we don't need any locks (since we hold a reference already). > > > > Simplify the locking accordingly. > > > > Noticed while reviewing a patch from Boris. > > > > Cc: Boris Brezillon <boris.brezillon@free-electrons.com> > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > > After reading this over a few times, I've convinced myself that it does > the right thing. > > Reviewed-by: Sean Paul <seanpaul@chromium.org> Pushed to -misc, thanks for your review. -Daniel > > > > --- > > drivers/gpu/drm/drm_atomic_helper.c | 7 +------ > > 1 file changed, 1 insertion(+), 6 deletions(-) > > > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c > > index 86d3093c6c9b..9bf86eb87fe1 100644 > > --- a/drivers/gpu/drm/drm_atomic_helper.c > > +++ b/drivers/gpu/drm/drm_atomic_helper.c > > @@ -1680,9 +1680,7 @@ void drm_atomic_helper_commit_hw_done(struct drm_atomic_state *old_state) > > > > /* backend must have consumed any event by now */ > > WARN_ON(new_crtc_state->event); > > - spin_lock(&crtc->commit_lock); > > complete_all(&commit->hw_done); > > - spin_unlock(&crtc->commit_lock); > > } > > } > > EXPORT_SYMBOL(drm_atomic_helper_commit_hw_done); > > @@ -1711,7 +1709,6 @@ void drm_atomic_helper_commit_cleanup_done(struct drm_atomic_state *old_state) > > if (WARN_ON(!commit)) > > continue; > > > > - spin_lock(&crtc->commit_lock); > > complete_all(&commit->cleanup_done); > > WARN_ON(!try_wait_for_completion(&commit->hw_done)); > > > > @@ -1721,8 +1718,6 @@ void drm_atomic_helper_commit_cleanup_done(struct drm_atomic_state *old_state) > > if (try_wait_for_completion(&commit->flip_done)) > > goto del_commit; > > > > - spin_unlock(&crtc->commit_lock); > > - > > /* 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. */ > > @@ -1732,8 +1727,8 @@ void drm_atomic_helper_commit_cleanup_done(struct drm_atomic_state *old_state) > > DRM_ERROR("[CRTC:%d:%s] flip_done timed out\n", > > crtc->base.id, crtc->name); > > > > - spin_lock(&crtc->commit_lock); > > del_commit: > > + spin_lock(&crtc->commit_lock); > > list_del(&commit->commit_entry); > > spin_unlock(&crtc->commit_lock); > > } > > -- > > 2.11.0 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Sean Paul, Software Engineer, Google / Chromium OS
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 86d3093c6c9b..9bf86eb87fe1 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -1680,9 +1680,7 @@ void drm_atomic_helper_commit_hw_done(struct drm_atomic_state *old_state) /* backend must have consumed any event by now */ WARN_ON(new_crtc_state->event); - spin_lock(&crtc->commit_lock); complete_all(&commit->hw_done); - spin_unlock(&crtc->commit_lock); } } EXPORT_SYMBOL(drm_atomic_helper_commit_hw_done); @@ -1711,7 +1709,6 @@ void drm_atomic_helper_commit_cleanup_done(struct drm_atomic_state *old_state) if (WARN_ON(!commit)) continue; - spin_lock(&crtc->commit_lock); complete_all(&commit->cleanup_done); WARN_ON(!try_wait_for_completion(&commit->hw_done)); @@ -1721,8 +1718,6 @@ void drm_atomic_helper_commit_cleanup_done(struct drm_atomic_state *old_state) if (try_wait_for_completion(&commit->flip_done)) goto del_commit; - spin_unlock(&crtc->commit_lock); - /* 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. */ @@ -1732,8 +1727,8 @@ void drm_atomic_helper_commit_cleanup_done(struct drm_atomic_state *old_state) DRM_ERROR("[CRTC:%d:%s] flip_done timed out\n", crtc->base.id, crtc->name); - spin_lock(&crtc->commit_lock); del_commit: + spin_lock(&crtc->commit_lock); list_del(&commit->commit_entry); spin_unlock(&crtc->commit_lock); }
The crtc->commit_lock only protects commit_list and commit_entry. If we chase the pointer from the drm_atomic_state update structure, then we don't need any locks (since we hold a reference already). Simplify the locking accordingly. Noticed while reviewing a patch from Boris. Cc: Boris Brezillon <boris.brezillon@free-electrons.com> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> --- drivers/gpu/drm/drm_atomic_helper.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-)