| Message ID | 20191101180713.5470-1-robdclark@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] drm/atomic: fix self-refresh helpers crtc state dereference | expand |
On Fri, Nov 1, 2019 at 2:09 PM Rob Clark <robdclark@gmail.com> wrote: > > From: Rob Clark <robdclark@chromium.org> > > drm_self_refresh_helper_update_avg_times() was incorrectly accessing the > new incoming state after drm_atomic_helper_commit_hw_done(). But this > state might have already been superceeded by an !nonblock atomic update > resulting in dereferencing an already free'd crtc_state. > > Fixes: d4da4e33341c ("drm: Measure Self Refresh Entry/Exit times to avoid thrashing") > Cc: Sean Paul <seanpaul@chromium.org> > Signed-off-by: Rob Clark <robdclark@chromium.org> > --- > TODO I *think* this will more or less do the right thing.. althought I'm > not 100% sure if, for example, we enter psr in a nonblock commit, and > then leave psr in a !nonblock commit that overtakes the completion of > the nonblock commit. Not sure if this sort of scenario can happen in > practice. But not crashing is better than crashing, so I guess we > should either take this patch or rever the self-refresh helpers until > Sean can figure out a better solution. > > drivers/gpu/drm/drm_atomic_helper.c | 2 ++ > drivers/gpu/drm/drm_self_refresh_helper.c | 11 ++++++----- > include/drm/drm_atomic.h | 8 ++++++++ > 3 files changed, 16 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c > index 3ef2ac52ce94..732bd0ce9241 100644 > --- a/drivers/gpu/drm/drm_atomic_helper.c > +++ b/drivers/gpu/drm/drm_atomic_helper.c > @@ -2240,6 +2240,8 @@ void drm_atomic_helper_commit_hw_done(struct drm_atomic_state *old_state) > int i; > > for_each_oldnew_crtc_in_state(old_state, crtc, old_crtc_state, new_crtc_state, i) { > + old_state->crtcs[i].new_self_refresh_active = new_crtc_state->self_refresh_active; > + > commit = new_crtc_state->commit; > if (!commit) > continue; > diff --git a/drivers/gpu/drm/drm_self_refresh_helper.c b/drivers/gpu/drm/drm_self_refresh_helper.c > index 68f4765a5896..77b9079fa578 100644 > --- a/drivers/gpu/drm/drm_self_refresh_helper.c > +++ b/drivers/gpu/drm/drm_self_refresh_helper.c > @@ -143,19 +143,20 @@ void drm_self_refresh_helper_update_avg_times(struct drm_atomic_state *state, > unsigned int commit_time_ms) > { > struct drm_crtc *crtc; > - struct drm_crtc_state *old_crtc_state, *new_crtc_state; > + struct drm_crtc_state *old_crtc_state; > int i; > > - for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, > - new_crtc_state, i) { > + for_each_old_crtc_in_state(state, crtc, old_crtc_state, i) { > + bool new_self_refresh_active = > + state->crtcs[i].new_self_refresh_active; > struct drm_self_refresh_data *sr_data = crtc->self_refresh_data; > struct ewma_psr_time *time; > > if (old_crtc_state->self_refresh_active == > - new_crtc_state->self_refresh_active) > + new_self_refresh_active) > continue; > > - if (new_crtc_state->self_refresh_active) > + if (new_self_refresh_active) > time = &sr_data->entry_avg_ms; > else > time = &sr_data->exit_avg_ms; > diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h > index 927e1205d7aa..86baf2b38bb3 100644 > --- a/include/drm/drm_atomic.h > +++ b/include/drm/drm_atomic.h > @@ -155,6 +155,14 @@ struct __drm_crtcs_state { > struct drm_crtc *ptr; > struct drm_crtc_state *state, *old_state, *new_state; > > + /** > + * @new_self_refresh_active: > + * > + * drm_atomic_helper_commit_hw_done() stashes new_crtc_state->self_refresh_active > + * so that it can be accessed late in drm_self_refresh_helper_update_avg_times(). > + */ > + bool new_self_refresh_active; > + Instead of stashing this in crtc, we could generate a bitmask local to commit_tail and pass that to calc_avg_times? That way we don't have to worry about someone using this when they shouldn't Sean > /** > * @commit: > * > -- > 2.21.0 >
Op 01-11-2019 om 21:06 schreef Sean Paul: > On Fri, Nov 1, 2019 at 2:09 PM Rob Clark <robdclark@gmail.com> wrote: >> From: Rob Clark <robdclark@chromium.org> >> >> drm_self_refresh_helper_update_avg_times() was incorrectly accessing the >> new incoming state after drm_atomic_helper_commit_hw_done(). But this >> state might have already been superceeded by an !nonblock atomic update >> resulting in dereferencing an already free'd crtc_state. >> >> Fixes: d4da4e33341c ("drm: Measure Self Refresh Entry/Exit times to avoid thrashing") >> Cc: Sean Paul <seanpaul@chromium.org> >> Signed-off-by: Rob Clark <robdclark@chromium.org> >> --- >> TODO I *think* this will more or less do the right thing.. althought I'm >> not 100% sure if, for example, we enter psr in a nonblock commit, and >> then leave psr in a !nonblock commit that overtakes the completion of >> the nonblock commit. Not sure if this sort of scenario can happen in >> practice. But not crashing is better than crashing, so I guess we >> should either take this patch or rever the self-refresh helpers until >> Sean can figure out a better solution. >> >> drivers/gpu/drm/drm_atomic_helper.c | 2 ++ >> drivers/gpu/drm/drm_self_refresh_helper.c | 11 ++++++----- >> include/drm/drm_atomic.h | 8 ++++++++ >> 3 files changed, 16 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c >> index 3ef2ac52ce94..732bd0ce9241 100644 >> --- a/drivers/gpu/drm/drm_atomic_helper.c >> +++ b/drivers/gpu/drm/drm_atomic_helper.c >> @@ -2240,6 +2240,8 @@ void drm_atomic_helper_commit_hw_done(struct drm_atomic_state *old_state) >> int i; >> >> for_each_oldnew_crtc_in_state(old_state, crtc, old_crtc_state, new_crtc_state, i) { >> + old_state->crtcs[i].new_self_refresh_active = new_crtc_state->self_refresh_active; >> + >> commit = new_crtc_state->commit; >> if (!commit) >> continue; >> diff --git a/drivers/gpu/drm/drm_self_refresh_helper.c b/drivers/gpu/drm/drm_self_refresh_helper.c >> index 68f4765a5896..77b9079fa578 100644 >> --- a/drivers/gpu/drm/drm_self_refresh_helper.c >> +++ b/drivers/gpu/drm/drm_self_refresh_helper.c >> @@ -143,19 +143,20 @@ void drm_self_refresh_helper_update_avg_times(struct drm_atomic_state *state, >> unsigned int commit_time_ms) >> { >> struct drm_crtc *crtc; >> - struct drm_crtc_state *old_crtc_state, *new_crtc_state; >> + struct drm_crtc_state *old_crtc_state; >> int i; >> >> - for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, >> - new_crtc_state, i) { >> + for_each_old_crtc_in_state(state, crtc, old_crtc_state, i) { >> + bool new_self_refresh_active = >> + state->crtcs[i].new_self_refresh_active; >> struct drm_self_refresh_data *sr_data = crtc->self_refresh_data; >> struct ewma_psr_time *time; >> >> if (old_crtc_state->self_refresh_active == >> - new_crtc_state->self_refresh_active) >> + new_self_refresh_active) >> continue; >> >> - if (new_crtc_state->self_refresh_active) >> + if (new_self_refresh_active) >> time = &sr_data->entry_avg_ms; >> else >> time = &sr_data->exit_avg_ms; >> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h >> index 927e1205d7aa..86baf2b38bb3 100644 >> --- a/include/drm/drm_atomic.h >> +++ b/include/drm/drm_atomic.h >> @@ -155,6 +155,14 @@ struct __drm_crtcs_state { >> struct drm_crtc *ptr; >> struct drm_crtc_state *state, *old_state, *new_state; >> >> + /** >> + * @new_self_refresh_active: >> + * >> + * drm_atomic_helper_commit_hw_done() stashes new_crtc_state->self_refresh_active >> + * so that it can be accessed late in drm_self_refresh_helper_update_avg_times(). >> + */ >> + bool new_self_refresh_active; >> + > Instead of stashing this in crtc, we could generate a bitmask local to > commit_tail and pass that to calc_avg_times? That way we don't have to > worry about someone using this when they shouldn't Yeah would make sense to have a bitmask, instead of making the property special. :) Current solution seems a bit ugly.
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 3ef2ac52ce94..732bd0ce9241 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -2240,6 +2240,8 @@ void drm_atomic_helper_commit_hw_done(struct drm_atomic_state *old_state) int i; for_each_oldnew_crtc_in_state(old_state, crtc, old_crtc_state, new_crtc_state, i) { + old_state->crtcs[i].new_self_refresh_active = new_crtc_state->self_refresh_active; + commit = new_crtc_state->commit; if (!commit) continue; diff --git a/drivers/gpu/drm/drm_self_refresh_helper.c b/drivers/gpu/drm/drm_self_refresh_helper.c index 68f4765a5896..77b9079fa578 100644 --- a/drivers/gpu/drm/drm_self_refresh_helper.c +++ b/drivers/gpu/drm/drm_self_refresh_helper.c @@ -143,19 +143,20 @@ void drm_self_refresh_helper_update_avg_times(struct drm_atomic_state *state, unsigned int commit_time_ms) { struct drm_crtc *crtc; - struct drm_crtc_state *old_crtc_state, *new_crtc_state; + struct drm_crtc_state *old_crtc_state; int i; - for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, - new_crtc_state, i) { + for_each_old_crtc_in_state(state, crtc, old_crtc_state, i) { + bool new_self_refresh_active = + state->crtcs[i].new_self_refresh_active; struct drm_self_refresh_data *sr_data = crtc->self_refresh_data; struct ewma_psr_time *time; if (old_crtc_state->self_refresh_active == - new_crtc_state->self_refresh_active) + new_self_refresh_active) continue; - if (new_crtc_state->self_refresh_active) + if (new_self_refresh_active) time = &sr_data->entry_avg_ms; else time = &sr_data->exit_avg_ms; diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h index 927e1205d7aa..86baf2b38bb3 100644 --- a/include/drm/drm_atomic.h +++ b/include/drm/drm_atomic.h @@ -155,6 +155,14 @@ struct __drm_crtcs_state { struct drm_crtc *ptr; struct drm_crtc_state *state, *old_state, *new_state; + /** + * @new_self_refresh_active: + * + * drm_atomic_helper_commit_hw_done() stashes new_crtc_state->self_refresh_active + * so that it can be accessed late in drm_self_refresh_helper_update_avg_times(). + */ + bool new_self_refresh_active; + /** * @commit: *