| Message ID | 1439924077-9569-1-git-send-email-robdclark@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 complete > > missed 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. -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 >
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 complete >> >> missed 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
On Tue, Aug 18, 2015 at 5:29 PM, Rob Clark <robdclark@gmail.com> wrote: > 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 complete >>> >>> missed 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.. actually, I don't think we'll hit it w/ legacy crtc-update paths.. but one possible approach is just stuff a no_acquire_fini flag in the state object.. BR, -R > 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
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; }
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 complete missed 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> --- drivers/gpu/drm/drm_atomic.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)