| Message ID | 20191031223641.19208-1-robdclark@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/atomic: swap_state should stall on cleanup_done | expand |
Op 31-10-2019 om 23:36 schreef Rob Clark: > From: Rob Clark <robdclark@chromium.org> > > Stalling on cleanup_done ensures that any atomic state related to a > nonblock commit no longer has dangling references to per-object state > that can be freed. > > Otherwise, if a !nonblock commit completes after a nonblock commit has > swapped state (ie. the synchronous part of the nonblock commit comes > before the !nonblock commit), but before the asynchronous part of the > nonblock commit completes, what was the new per-object state in the > nonblock commit can be freed. > > This shows up with the new self-refresh helper, as _update_avg_times() > dereferences the original old and new 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> > --- > Other possibilities: > 1) maybe block later before freeing atomic state? > 2) refcount individual per-object state > > drivers/gpu/drm/drm_atomic_helper.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c > index 3ef2ac52ce94..a5d95429f91b 100644 > --- a/drivers/gpu/drm/drm_atomic_helper.c > +++ b/drivers/gpu/drm/drm_atomic_helper.c > @@ -2711,7 +2711,7 @@ int drm_atomic_helper_swap_state(struct drm_atomic_state *state, > if (!commit) > continue; > > - ret = wait_for_completion_interruptible(&commit->hw_done); > + ret = wait_for_completion_interruptible(&commit->cleanup_done); > if (ret) > return ret; > } > @@ -2722,7 +2722,7 @@ int drm_atomic_helper_swap_state(struct drm_atomic_state *state, > if (!commit) > continue; > > - ret = wait_for_completion_interruptible(&commit->hw_done); > + ret = wait_for_completion_interruptible(&commit->cleanup_done); > if (ret) > return ret; > } > @@ -2733,7 +2733,7 @@ int drm_atomic_helper_swap_state(struct drm_atomic_state *state, > if (!commit) > continue; > > - ret = wait_for_completion_interruptible(&commit->hw_done); > + ret = wait_for_completion_interruptible(&commit->cleanup_done); > if (ret) > return ret; > } Nack, hw_done means all new_crtc_state (from the old commit pov) dereferences are done. Self refresh helpers should be fixed. :)
Op 31-10-2019 om 23:36 schreef Rob Clark: > From: Rob Clark <robdclark@chromium.org> > > Stalling on cleanup_done ensures that any atomic state related to a > nonblock commit no longer has dangling references to per-object state > that can be freed. > > Otherwise, if a !nonblock commit completes after a nonblock commit has > swapped state (ie. the synchronous part of the nonblock commit comes > before the !nonblock commit), but before the asynchronous part of the > nonblock commit completes, what was the new per-object state in the > nonblock commit can be freed. > > This shows up with the new self-refresh helper, as _update_avg_times() > dereferences the original old and new 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> > --- > Other possibilities: > 1) maybe block later before freeing atomic state? > 2) refcount individual per-object state > > drivers/gpu/drm/drm_atomic_helper.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c > index 3ef2ac52ce94..a5d95429f91b 100644 > --- a/drivers/gpu/drm/drm_atomic_helper.c > +++ b/drivers/gpu/drm/drm_atomic_helper.c > @@ -2711,7 +2711,7 @@ int drm_atomic_helper_swap_state(struct drm_atomic_state *state, > if (!commit) > continue; > > - ret = wait_for_completion_interruptible(&commit->hw_done); > + ret = wait_for_completion_interruptible(&commit->cleanup_done); > if (ret) > return ret; > } > @@ -2722,7 +2722,7 @@ int drm_atomic_helper_swap_state(struct drm_atomic_state *state, > if (!commit) > continue; > > - ret = wait_for_completion_interruptible(&commit->hw_done); > + ret = wait_for_completion_interruptible(&commit->cleanup_done); > if (ret) > return ret; > } > @@ -2733,7 +2733,7 @@ int drm_atomic_helper_swap_state(struct drm_atomic_state *state, > if (!commit) > continue; > > - ret = wait_for_completion_interruptible(&commit->hw_done); > + ret = wait_for_completion_interruptible(&commit->cleanup_done); > if (ret) > return ret; > } From setup_commit(): * Completion of the hardware commit step must be signalled using * drm_atomic_helper_commit_hw_done(). After this step the driver is not allowed * to read or change any permanent software or hardware modeset state. The only * exception is state protected by other means than &drm_modeset_lock locks. * Only the free standing @state with pointers to the old state structures can * be inspected, e.g. to clean up old buffers using * drm_atomic_helper_cleanup_planes(). And the hw_done function says pretty much the same thing. :)
On Fri, Nov 1, 2019 at 7:47 AM Maarten Lankhorst <maarten.lankhorst@linux.intel.com> wrote: > > Op 31-10-2019 om 23:36 schreef Rob Clark: > > From: Rob Clark <robdclark@chromium.org> > > > > Stalling on cleanup_done ensures that any atomic state related to a > > nonblock commit no longer has dangling references to per-object state > > that can be freed. > > > > Otherwise, if a !nonblock commit completes after a nonblock commit has > > swapped state (ie. the synchronous part of the nonblock commit comes > > before the !nonblock commit), but before the asynchronous part of the > > nonblock commit completes, what was the new per-object state in the > > nonblock commit can be freed. > > > > This shows up with the new self-refresh helper, as _update_avg_times() > > dereferences the original old and new 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> > > --- > > Other possibilities: > > 1) maybe block later before freeing atomic state? > > 2) refcount individual per-object state > > > > drivers/gpu/drm/drm_atomic_helper.c | 6 +++--- > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c > > index 3ef2ac52ce94..a5d95429f91b 100644 > > --- a/drivers/gpu/drm/drm_atomic_helper.c > > +++ b/drivers/gpu/drm/drm_atomic_helper.c > > @@ -2711,7 +2711,7 @@ int drm_atomic_helper_swap_state(struct drm_atomic_state *state, > > if (!commit) > > continue; > > > > - ret = wait_for_completion_interruptible(&commit->hw_done); > > + ret = wait_for_completion_interruptible(&commit->cleanup_done); > > if (ret) > > return ret; > > } > > @@ -2722,7 +2722,7 @@ int drm_atomic_helper_swap_state(struct drm_atomic_state *state, > > if (!commit) > > continue; > > > > - ret = wait_for_completion_interruptible(&commit->hw_done); > > + ret = wait_for_completion_interruptible(&commit->cleanup_done); > > if (ret) > > return ret; > > } > > @@ -2733,7 +2733,7 @@ int drm_atomic_helper_swap_state(struct drm_atomic_state *state, > > if (!commit) > > continue; > > > > - ret = wait_for_completion_interruptible(&commit->hw_done); > > + ret = wait_for_completion_interruptible(&commit->cleanup_done); > > if (ret) > > return ret; > > } > > Nack, hw_done means all new_crtc_state (from the old commit pov) dereferences are done. > hmm, it would be nice if the for_each_blah_in_state() iterators would splat on incorrect usage, then.. it tool a while to track down what was going wrong. And Sean claimed the self refresh helpers worked for him on rockchip/i915 (although I'm starting to suspect maybe he just didn't have enough debug options enabled to poison freed memory?) > Self refresh helpers should be fixed. :) Looks like what they need out of crtc_state is pretty minimal, maybe they could extract out crtc_state->self_refresh_active earlier.. BR, -R
Op 01-11-2019 om 15:59 schreef Rob Clark: > On Fri, Nov 1, 2019 at 7:47 AM Maarten Lankhorst > <maarten.lankhorst@linux.intel.com> wrote: >> Op 31-10-2019 om 23:36 schreef Rob Clark: >>> From: Rob Clark <robdclark@chromium.org> >>> >>> Stalling on cleanup_done ensures that any atomic state related to a >>> nonblock commit no longer has dangling references to per-object state >>> that can be freed. >>> >>> Otherwise, if a !nonblock commit completes after a nonblock commit has >>> swapped state (ie. the synchronous part of the nonblock commit comes >>> before the !nonblock commit), but before the asynchronous part of the >>> nonblock commit completes, what was the new per-object state in the >>> nonblock commit can be freed. >>> >>> This shows up with the new self-refresh helper, as _update_avg_times() >>> dereferences the original old and new 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> >>> --- >>> Other possibilities: >>> 1) maybe block later before freeing atomic state? >>> 2) refcount individual per-object state >>> >>> drivers/gpu/drm/drm_atomic_helper.c | 6 +++--- >>> 1 file changed, 3 insertions(+), 3 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c >>> index 3ef2ac52ce94..a5d95429f91b 100644 >>> --- a/drivers/gpu/drm/drm_atomic_helper.c >>> +++ b/drivers/gpu/drm/drm_atomic_helper.c >>> @@ -2711,7 +2711,7 @@ int drm_atomic_helper_swap_state(struct drm_atomic_state *state, >>> if (!commit) >>> continue; >>> >>> - ret = wait_for_completion_interruptible(&commit->hw_done); >>> + ret = wait_for_completion_interruptible(&commit->cleanup_done); >>> if (ret) >>> return ret; >>> } >>> @@ -2722,7 +2722,7 @@ int drm_atomic_helper_swap_state(struct drm_atomic_state *state, >>> if (!commit) >>> continue; >>> >>> - ret = wait_for_completion_interruptible(&commit->hw_done); >>> + ret = wait_for_completion_interruptible(&commit->cleanup_done); >>> if (ret) >>> return ret; >>> } >>> @@ -2733,7 +2733,7 @@ int drm_atomic_helper_swap_state(struct drm_atomic_state *state, >>> if (!commit) >>> continue; >>> >>> - ret = wait_for_completion_interruptible(&commit->hw_done); >>> + ret = wait_for_completion_interruptible(&commit->cleanup_done); >>> if (ret) >>> return ret; >>> } >> Nack, hw_done means all new_crtc_state (from the old commit pov) dereferences are done. >> > hmm, it would be nice if the for_each_blah_in_state() iterators would > splat on incorrect usage, then.. it tool a while to track down what > was going wrong. And Sean claimed the self refresh helpers worked for > him on rockchip/i915 (although I'm starting to suspect maybe he just > didn't have enough debug options enabled to poison freed memory?) Could do a memset on the new arrays after hw_done? >> Self refresh helpers should be fixed. :) > Looks like what they need out of crtc_state is pretty minimal, maybe > they could extract out crtc_state->self_refresh_active earlier.. Yeah, something like that would work. :)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 3ef2ac52ce94..a5d95429f91b 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -2711,7 +2711,7 @@ int drm_atomic_helper_swap_state(struct drm_atomic_state *state, if (!commit) continue; - ret = wait_for_completion_interruptible(&commit->hw_done); + ret = wait_for_completion_interruptible(&commit->cleanup_done); if (ret) return ret; } @@ -2722,7 +2722,7 @@ int drm_atomic_helper_swap_state(struct drm_atomic_state *state, if (!commit) continue; - ret = wait_for_completion_interruptible(&commit->hw_done); + ret = wait_for_completion_interruptible(&commit->cleanup_done); if (ret) return ret; } @@ -2733,7 +2733,7 @@ int drm_atomic_helper_swap_state(struct drm_atomic_state *state, if (!commit) continue; - ret = wait_for_completion_interruptible(&commit->hw_done); + ret = wait_for_completion_interruptible(&commit->cleanup_done); if (ret) return ret; }