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; }
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:
- maybe block later before freeing atomic state?
- 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. :)
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:
- maybe block later before freeing atomic state?
- 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:
- maybe block later before freeing atomic state?
- 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. :)
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:
- maybe block later before freeing atomic state?
- 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. :)
dri-devel@lists.freedesktop.org
-
Maarten Lankhorst -
Rob Clark