Op 17-01-18 om 19:29 schreef Sean Paul:

On Wed, Jan 17, 2018 at 12:51:08PM +0100, Maarten Lankhorst wrote:

...

From: "Leo (Sunpeng) Li" sunpeng.li@amd.com

During a non-blocking commit, it is possible to return before the commit_tail work is queued (-ERESTARTSYS, for example).

Since a reference on the crtc commit object is obtained for the pending vblank event when preparing the commit, the above situation will leave us with an extra reference.

Therefore, if the commit_tail worker has not consumed the event at the end of a commit, release it's reference.

Changes since v1:

• Also check for state->event->base.completion being set, to handle the case where stall_checks() fails in setup_crtc_commit().

Changes since v2:

• Add a flag to drm_crtc_commit, to prevent dereferencing a freed event. i915 may unreference the state in a worker.

Fixes: 24835e442f28 ("drm: reference count event->completion") Cc: stable@vger.kernel.org # v4.11+ Signed-off-by: Leo (Sunpeng) Li sunpeng.li@amd.com Acked-by: Harry Wentland harry.wentland@amd.com #v1 Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com

---

drivers/gpu/drm/drm_atomic_helper.c | 15 +++++++++++++++ include/drm/drm_atomic.h | 9 +++++++++ 2 files changed, 24 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index ab4032167094..ae3cbfe9e01c 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -1878,6 +1878,8 @@ int drm_atomic_helper_setup_commit(struct drm_atomic_state *state, new_crtc_state->event->base.completion = &commit->flip_done; new_crtc_state->event->base.completion_release = release_crtc_commit; drm_crtc_commit_get(commit);

• commit->abort_completion = true;
  

  }

for_each_oldnew_connector_in_state(state, conn, old_conn_state, new_conn_state, i) { @@ -3421,8 +3423,21 @@ EXPORT_SYMBOL(drm_atomic_helper_crtc_duplicate_state); void __drm_atomic_helper_crtc_destroy_state(struct drm_crtc_state *state) { if (state->commit) {

• /*
  

•  * In the event that a non-blocking commit returns
  

•  * -ERESTARTSYS before the commit_tail work is queued, we will
  

•  * have an extra reference to the commit object. Release it, if
  

•  * the event has not been consumed by the worker.
  

•  *
  

•  * state->event may be freed, so we can't directly look at
  

•  * state->event->base.completion.
  

•  */
  

• if (state->event && state->commit->abort_completion)
  

• 	drm_crtc_commit_put(state->commit);
  

• kfree(state->commit->event); state->commit->event = NULL;
• drm_crtc_commit_put(state->commit); }

diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h index 1c27526c499e..cf13842a6dbd 100644 --- a/include/drm/drm_atomic.h +++ b/include/drm/drm_atomic.h @@ -134,6 +134,15 @@ struct drm_crtc_commit { * &drm_pending_vblank_event pointer to clean up private events. */ struct drm_pending_vblank_event *event;

• /**

• * @abort_completion:
  

• *
  

• * A flag that's set after drm_atomic_helper_setup_commit takes a second
  

• * reference for the completion of $drm_crtc_state.event. It's used by
  

• * the free code to remove the second reference if commit fails.
  

• */
  

Perhaps it's just me, or I'm oversimplifying the problem. I think this would be easier to understand if we just dropped the additional reference at the point of failure (ie: in swap_state). That way we don't have to add Yet Another Piece Of State.

That's assuming nothing can fail between setup_commit() and swap_state() and also that the driver implementing atomci puts no calls in between. And also assumes that even setup_commit has proper rollback. I think it's overkill, and we have no choice but to do it like this. :(

~Maarten

Updated IGT results seem sane: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_7698/shards.html

Would someone be able to apply this patch?

Thanks, Leo

On 2018-01-17 03:18 PM, Sean Paul wrote:

On Wed, Jan 17, 2018 at 10:39 AM, Maarten Lankhorst maarten.lankhorst@linux.intel.com wrote:

...

Op 17-01-18 om 19:29 schreef Sean Paul:

...

On Wed, Jan 17, 2018 at 12:51:08PM +0100, Maarten Lankhorst wrote:

...

From: "Leo (Sunpeng) Li" sunpeng.li@amd.com

During a non-blocking commit, it is possible to return before the commit_tail work is queued (-ERESTARTSYS, for example).

Since a reference on the crtc commit object is obtained for the pending vblank event when preparing the commit, the above situation will leave us with an extra reference.

Therefore, if the commit_tail worker has not consumed the event at the end of a commit, release it's reference.

Changes since v1:

• Also check for state->event->base.completion being set, to handle the case where stall_checks() fails in setup_crtc_commit().

Changes since v2:

• Add a flag to drm_crtc_commit, to prevent dereferencing a freed event. i915 may unreference the state in a worker.

Fixes: 24835e442f28 ("drm: reference count event->completion") Cc: stable@vger.kernel.org # v4.11+ Signed-off-by: Leo (Sunpeng) Li sunpeng.li@amd.com Acked-by: Harry Wentland harry.wentland@amd.com #v1 Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com

---

drivers/gpu/drm/drm_atomic_helper.c | 15 +++++++++++++++ include/drm/drm_atomic.h | 9 +++++++++ 2 files changed, 24 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index ab4032167094..ae3cbfe9e01c 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -1878,6 +1878,8 @@ int drm_atomic_helper_setup_commit(struct drm_atomic_state *state, new_crtc_state->event->base.completion = &commit->flip_done; new_crtc_state->event->base.completion_release = release_crtc_commit; drm_crtc_commit_get(commit);

•        commit->abort_completion = true;
  }
  
  for_each_oldnew_connector_in_state(state, conn, old_conn_state, new_conn_state, i) {
  

@@ -3421,8 +3423,21 @@ EXPORT_SYMBOL(drm_atomic_helper_crtc_duplicate_state); void __drm_atomic_helper_crtc_destroy_state(struct drm_crtc_state *state) { if (state->commit) {

•        /*
  

•         * In the event that a non-blocking commit returns
  

•         * -ERESTARTSYS before the commit_tail work is queued, we will
  

•         * have an extra reference to the commit object. Release it, if
  

•         * the event has not been consumed by the worker.
  

•         *
  

•         * state->event may be freed, so we can't directly look at
  

•         * state->event->base.completion.
  

•         */
  

•        if (state->event && state->commit->abort_completion)
  

•                drm_crtc_commit_put(state->commit);
  

•         kfree(state->commit->event);
          state->commit->event = NULL;
  

•         drm_crtc_commit_put(state->commit);
  }
  
  

diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h index 1c27526c499e..cf13842a6dbd 100644 --- a/include/drm/drm_atomic.h +++ b/include/drm/drm_atomic.h @@ -134,6 +134,15 @@ struct drm_crtc_commit { * &drm_pending_vblank_event pointer to clean up private events. */ struct drm_pending_vblank_event *event;

• /**

• * @abort_completion:
  

• *
  

• * A flag that's set after drm_atomic_helper_setup_commit takes a second
  

• * reference for the completion of $drm_crtc_state.event. It's used by
  

• * the free code to remove the second reference if commit fails.
  

• */
  

Perhaps it's just me, or I'm oversimplifying the problem. I think this would be easier to understand if we just dropped the additional reference at the point of failure (ie: in swap_state). That way we don't have to add Yet Another Piece Of State.

That's assuming nothing can fail between setup_commit() and swap_state() and also that the driver implementing atomci puts no calls in between. And also assumes that even setup_commit has proper rollback. I think it's overkill, and we have no choice but to do it like this. :(

yeah, fair enough.

Reviewed-by: Sean Paul seanpaul@chromium.org

...

~Maarten