Re: [PATCH] drm/atomic: swap_state should stall on cleanup_done

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