On Tue, Aug 18, 2015 at 5:21 PM, Daniel Vetter daniel@ffwll.ch wrote:
On Tue, Aug 18, 2015 at 11:54 AM, Rob Clark robdclark@gmail.com wrote:
The addition of ww_acquire_done() call to mark the point where all locks should already be held, added in:
commit 992cbf19b32900efa17850b9fa0031fd623edd4d Author: Daniel Vetter daniel.vetter@ffwll.ch AuthorDate: Thu Aug 6 15:06:40 2015 +0200
drm/atomic: Call ww_acquire_done after check phase is completemissed the case that legacy paths (which are already called between drm_modeset_lock_all()/unlock_all()) re-use the single acquire ctx, even if there are multiple atomic updates. For example, restore_fbdev_mode() clearing rotation properties individually on each plane.
Inki already sent a patch which completely removed the ww_acquire_done() call, but I think this is less ideal (since it is a nice debugging feature of ww_mutex to confirm that everyone is following the rules). So instead, just skip the call only in the special legacy-path case.
Reported-by: Inki Dae inki.dae@samsung.com Tested-by: Bjorn Andersson bjorn.andersson@sonymobile.com Signed-off-by: Rob Clark robdclark@gmail.com
Should we instead just do it in the atomic ioclt path? Your patch here is missing the legacy crtc-update case (where the acquire_ctx is stored in the drm_crtc). Also this won't work once we have a drm_modeset_lock_all_ctx.
Well, doing it here (vs in ioctl fxn, outside of drm_atomic_commit()) is that it will catch cases where drivers try to acquire locks in ->commit()..
Maybe we just need to pass a legacy_dont_acquire_done flag down? Or just take Inki's patch for now until we sort out a better plan..
BR, -R
-Daniel
drivers/gpu/drm/drm_atomic.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 1066e4b..b90068c 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -1230,8 +1230,15 @@ int drm_atomic_check_only(struct drm_atomic_state *state) } }
if (ret == 0)ww_acquire_done(&state->acquire_ctx->ww_ctx);
if (ret == 0) {/* in legacy paths, there can be multiple atomic updates,* such as drm_mode_plane_set_obj_prop(), which happen* under a single drm_modeset_lock_all(). In the legacy* paths, skip marking acquire-done lest we anger ww-mutex:*/if (state->acquire_ctx != dev->mode_config.acquire_ctx)ww_acquire_done(&state->acquire_ctx->ww_ctx);} return ret;}
2.4.3
-- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch