[PATCH] drm/atomic: Fix memleak on ERESTARTSYS during non-blocking commits

[PATCH] drm/atomic: Fix memleak on ERESTARTSYS during non-blocking commits

[Maarten Lankhorst]

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.
+	 */
+	bool abort_completion;
 };
 
 struct __drm_planes_state {
-- 
2.15.1

✓ Fi.CI.BAT: success for drm/atomic: Fix memleak on ERESTARTSYS during non-blocking commits

[Patchwork]

== Series Details ==

Series: drm/atomic: Fix memleak on ERESTARTSYS during non-blocking commits
URL   : https://patchwork.freedesktop.org/series/36608/
State : success

== Summary ==

Series 36608v1 drm/atomic: Fix memleak on ERESTARTSYS during non-blocking commits
https://patchwork.freedesktop.org/api/1.0/series/36608/revisions/1/mbox/

Test debugfs_test:
        Subgroup read_all_entries:
                pass       -> DMESG-WARN (fi-bdw-gvtdvm) fdo#103938 +1

fdo#103938 https://bugs.freedesktop.org/show_bug.cgi?id=103938

fi-bdw-5557u     total:288  pass:267  dwarn:0   dfail:0   fail:0   skip:21  time:423s
fi-bdw-gvtdvm    total:288  pass:262  dwarn:2   dfail:0   fail:0   skip:24  time:421s
fi-blb-e6850     total:288  pass:223  dwarn:1   dfail:0   fail:0   skip:64  time:379s
fi-bsw-n3050     total:288  pass:242  dwarn:0   dfail:0   fail:0   skip:46  time:491s
fi-bwr-2160      total:288  pass:183  dwarn:0   dfail:0   fail:0   skip:105 time:282s
fi-bxt-dsi       total:288  pass:258  dwarn:0   dfail:0   fail:0   skip:30  time:483s
fi-bxt-j4205     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:491s
fi-byt-j1900     total:288  pass:253  dwarn:0   dfail:0   fail:0   skip:35  time:477s
fi-byt-n2820     total:288  pass:249  dwarn:0   dfail:0   fail:0   skip:39  time:456s
fi-elk-e7500     total:224  pass:168  dwarn:9   dfail:1   fail:0   skip:45 
fi-gdg-551       total:288  pass:179  dwarn:0   dfail:0   fail:1   skip:108 time:282s
fi-glk-1         total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:517s
fi-hsw-4770      total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:394s
fi-hsw-4770r     total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:401s
fi-ilk-650       total:288  pass:228  dwarn:0   dfail:0   fail:0   skip:60  time:414s
fi-ivb-3520m     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:460s
fi-ivb-3770      total:288  pass:255  dwarn:0   dfail:0   fail:0   skip:33  time:413s
fi-kbl-7500u     total:288  pass:263  dwarn:1   dfail:0   fail:0   skip:24  time:462s
fi-kbl-7560u     total:288  pass:269  dwarn:0   dfail:0   fail:0   skip:19  time:497s
fi-kbl-7567u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:458s
fi-kbl-r         total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:506s
fi-pnv-d510      total:288  pass:222  dwarn:1   dfail:0   fail:0   skip:65  time:580s
fi-skl-6260u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:430s
fi-skl-6600u     total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:510s
fi-skl-6700hq    total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:531s
fi-skl-6700k2    total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:488s
fi-skl-6770hq    total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:496s
fi-skl-gvtdvm    total:288  pass:265  dwarn:0   dfail:0   fail:0   skip:23  time:435s
fi-snb-2520m     total:288  pass:248  dwarn:0   dfail:0   fail:0   skip:40  time:531s
fi-snb-2600      total:288  pass:248  dwarn:0   dfail:0   fail:0   skip:40  time:401s
Blacklisted hosts:
fi-cfl-s2        total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:576s
fi-glk-dsi       total:288  pass:258  dwarn:0   dfail:0   fail:0   skip:30  time:472s

a5cf4f0319302df5036145fd6c96d6966cc677ae drm-tip: 2018y-01m-17d-09h-14m-44s UTC integration manifest
29987ceee72d drm/atomic: Fix memleak on ERESTARTSYS during non-blocking commits

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_7693/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.IGT: warning for drm/atomic: Fix memleak on ERESTARTSYS during non-blocking commits

[Patchwork]

== Series Details ==

Series: drm/atomic: Fix memleak on ERESTARTSYS during non-blocking commits
URL   : https://patchwork.freedesktop.org/series/36608/
State : warning

== Summary ==

Test kms_frontbuffer_tracking:
        Subgroup fbc-1p-primscrn-cur-indfb-draw-mmap-cpu:
                pass       -> SKIP       (shard-snb)
        Subgroup fbc-1p-offscren-pri-shrfb-draw-render:
                fail       -> PASS       (shard-snb) fdo#101623
        Subgroup fbc-1p-primscrn-pri-shrfb-draw-render:
                pass       -> INCOMPLETE (shard-hsw) fdo#103167
Test perf:
        Subgroup polling:
                pass       -> FAIL       (shard-hsw) fdo#102252
Test drv_suspend:
        Subgroup fence-restore-untiled:
                skip       -> PASS       (shard-snb) fdo#102365
Test kms_flip:
        Subgroup flip-vs-modeset-vs-hang:
                pass       -> INCOMPLETE (shard-snb) fdo#103821

fdo#101623 https://bugs.freedesktop.org/show_bug.cgi?id=101623
fdo#103167 https://bugs.freedesktop.org/show_bug.cgi?id=103167
fdo#102252 https://bugs.freedesktop.org/show_bug.cgi?id=102252
fdo#102365 https://bugs.freedesktop.org/show_bug.cgi?id=102365
fdo#103821 https://bugs.freedesktop.org/show_bug.cgi?id=103821

shard-hsw        total:2642 pass:1653 dwarn:1   dfail:0   fail:12  skip:974 time:13691s
shard-snb        total:2737 pass:1309 dwarn:1   dfail:0   fail:11  skip:1415 time:7708s
Blacklisted hosts:
shard-apl        total:2693 pass:1666 dwarn:1   dfail:0   fail:22  skip:1002 time:13047s
shard-kbl        total:2753 pass:1819 dwarn:21  dfail:1   fail:22  skip:890 time:10703s

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_7693/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for drm/atomic: Fix memleak on ERESTARTSYS during non-blocking commits

[Patchwork]

== Series Details ==

Series: drm/atomic: Fix memleak on ERESTARTSYS during non-blocking commits
URL   : https://patchwork.freedesktop.org/series/36608/
State : success

== Summary ==

Series 36608v1 drm/atomic: Fix memleak on ERESTARTSYS during non-blocking commits
https://patchwork.freedesktop.org/api/1.0/series/36608/revisions/1/mbox/

Test debugfs_test:
        Subgroup read_all_entries:
                dmesg-fail -> DMESG-WARN (fi-elk-e7500) fdo#103989
Test gem_mmap_gtt:
        Subgroup basic-small-bo-tiledx:
                fail       -> PASS       (fi-gdg-551) fdo#102575

fdo#103989 https://bugs.freedesktop.org/show_bug.cgi?id=103989
fdo#102575 https://bugs.freedesktop.org/show_bug.cgi?id=102575

fi-bdw-5557u     total:288  pass:267  dwarn:0   dfail:0   fail:0   skip:21  time:418s
fi-bdw-gvtdvm    total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:424s
fi-blb-e6850     total:288  pass:223  dwarn:1   dfail:0   fail:0   skip:64  time:372s
fi-bsw-n3050     total:288  pass:242  dwarn:0   dfail:0   fail:0   skip:46  time:488s
fi-bwr-2160      total:288  pass:183  dwarn:0   dfail:0   fail:0   skip:105 time:281s
fi-bxt-dsi       total:288  pass:258  dwarn:0   dfail:0   fail:0   skip:30  time:480s
fi-bxt-j4205     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:487s
fi-byt-j1900     total:288  pass:253  dwarn:0   dfail:0   fail:0   skip:35  time:467s
fi-byt-n2820     total:288  pass:249  dwarn:0   dfail:0   fail:0   skip:39  time:452s
fi-elk-e7500     total:224  pass:168  dwarn:10  dfail:0   fail:0   skip:45 
fi-gdg-551       total:288  pass:180  dwarn:0   dfail:0   fail:0   skip:108 time:278s
fi-glk-1         total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:513s
fi-hsw-4770      total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:390s
fi-hsw-4770r     total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:398s
fi-ilk-650       total:288  pass:228  dwarn:0   dfail:0   fail:0   skip:60  time:414s
fi-ivb-3520m     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:466s
fi-ivb-3770      total:288  pass:255  dwarn:0   dfail:0   fail:0   skip:33  time:414s
fi-kbl-7500u     total:288  pass:263  dwarn:1   dfail:0   fail:0   skip:24  time:464s
fi-kbl-7560u     total:288  pass:269  dwarn:0   dfail:0   fail:0   skip:19  time:500s
fi-kbl-7567u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:454s
fi-kbl-r         total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:503s
fi-pnv-d510      total:288  pass:222  dwarn:1   dfail:0   fail:0   skip:65  time:588s
fi-skl-6260u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:429s
fi-skl-6600u     total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:509s
fi-skl-6700hq    total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:531s
fi-skl-6700k2    total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:484s
fi-skl-6770hq    total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:481s
fi-skl-gvtdvm    total:288  pass:265  dwarn:0   dfail:0   fail:0   skip:23  time:435s
fi-snb-2520m     total:288  pass:248  dwarn:0   dfail:0   fail:0   skip:40  time:519s
fi-snb-2600      total:288  pass:248  dwarn:0   dfail:0   fail:0   skip:40  time:394s
Blacklisted hosts:
fi-cfl-s2        total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:570s
fi-glk-dsi       total:288  pass:258  dwarn:0   dfail:0   fail:0   skip:30  time:471s

a5cf4f0319302df5036145fd6c96d6966cc677ae drm-tip: 2018y-01m-17d-09h-14m-44s UTC integration manifest
26f0f1af3156 drm/atomic: Fix memleak on ERESTARTSYS during non-blocking commits

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_7698/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/atomic: Fix memleak on ERESTARTSYS during non-blocking commits

[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.

Sean

> +	bool abort_completion;
>  };
>  
>  struct __drm_planes_state {
> -- 
> 2.15.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Sean Paul, Software Engineer, Google / Chromium OS

Re: [PATCH] drm/atomic: Fix memleak on ERESTARTSYS during non-blocking commits

[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.

Sean

> +	bool abort_completion;
>  };
>  
>  struct __drm_planes_state {
> -- 
> 2.15.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/atomic: Fix memleak on ERESTARTSYS during non-blocking commits

[Maarten Lankhorst]

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

Re: [Intel-gfx] [PATCH] drm/atomic: Fix memleak on ERESTARTSYS during non-blocking commits

[Sean Paul]

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
>

✗ Fi.CI.IGT: warning for drm/atomic: Fix memleak on ERESTARTSYS during non-blocking commits

[Patchwork]

== Series Details ==

Series: drm/atomic: Fix memleak on ERESTARTSYS during non-blocking commits
URL   : https://patchwork.freedesktop.org/series/36608/
State : warning

== Summary ==

Test drv_suspend:
        Subgroup fence-restore-untiled:
                skip       -> PASS       (shard-snb) fdo#102365
Test kms_flip:
        Subgroup 2x-vblank-vs-modeset-suspend:
                pass       -> SKIP       (shard-hsw)
Test kms_frontbuffer_tracking:
        Subgroup fbc-1p-offscren-pri-shrfb-draw-blt:
                fail       -> PASS       (shard-snb) fdo#101623 +1
Test perf:
        Subgroup oa-exponents:
                pass       -> FAIL       (shard-apl) fdo#102254
Test gem_fence_thrash:
        Subgroup bo-write-verify-x:
                incomplete -> PASS       (shard-hsw) fdo#103540
Test kms_cursor_legacy:
        Subgroup pipe-b-forked-bo:
                notrun     -> INCOMPLETE (shard-hsw) fdo#104218
Test gem_eio:
        Subgroup in-flight-contexts:
                pass       -> DMESG-WARN (shard-snb) fdo#104058
Test drv_selftest:
        Subgroup live_gtt:
                incomplete -> PASS       (shard-apl) fdo#103927

fdo#102365 https://bugs.freedesktop.org/show_bug.cgi?id=102365
fdo#101623 https://bugs.freedesktop.org/show_bug.cgi?id=101623
fdo#102254 https://bugs.freedesktop.org/show_bug.cgi?id=102254
fdo#103540 https://bugs.freedesktop.org/show_bug.cgi?id=103540
fdo#104218 https://bugs.freedesktop.org/show_bug.cgi?id=104218
fdo#104058 https://bugs.freedesktop.org/show_bug.cgi?id=104058
fdo#103927 https://bugs.freedesktop.org/show_bug.cgi?id=103927

shard-apl        total:2753 pass:1715 dwarn:1   dfail:0   fail:22  skip:1015 time:13897s
shard-hsw        total:2724 pass:1702 dwarn:1   dfail:0   fail:11  skip:1008 time:14480s
shard-snb        total:2753 pass:1318 dwarn:2   dfail:0   fail:10  skip:1423 time:7891s
Blacklisted hosts:
shard-kbl        total:2753 pass:1838 dwarn:1   dfail:0   fail:23  skip:891 time:10920s

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_7698/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/atomic: Fix memleak on ERESTARTSYS during non-blocking commits

[Leo Li]

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
>>

Re: [Intel-gfx] [PATCH] drm/atomic: Fix memleak on ERESTARTSYS during non-blocking commits

[Maarten Lankhorst]

Op 29-01-18 om 16:41 schreef Leo Li:
> 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 for the reminder, pushed.

~Maarten

Re: [Intel-gfx] [PATCH] drm/atomic: Fix memleak on ERESTARTSYS during non-blocking commits

[Harry Wentland]

On 2018-01-30 05:28 AM, Maarten Lankhorst wrote:
> Op 29-01-18 om 16:41 schreef Leo Li:
>> 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 for the reminder, pushed.
> 

Thanks, Maarten. I see it in drm-misc-next.

Would someone be able to pull this into drm-misc-fixes as well, or can I just I apply this myself with the following dim commands?

dim checkout drm-misc-fixes
dim cherry-pick 1c6ceeee6ebb
dim push-branch

Harry


> ~Maarten
> 

Re: [PATCH] drm/atomic: Fix memleak on ERESTARTSYS during non-blocking commits

[Harry Wentland]

On 2018-01-30 05:28 AM, Maarten Lankhorst wrote:
> Op 29-01-18 om 16:41 schreef Leo Li:
>> 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 for the reminder, pushed.
> 

Thanks, Maarten. I see it in drm-misc-next.

Would someone be able to pull this into drm-misc-fixes as well, or can I just I apply this myself with the following dim commands?

dim checkout drm-misc-fixes
dim cherry-pick 1c6ceeee6ebb
dim push-branch

Harry


> ~Maarten
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/atomic: Fix memleak on ERESTARTSYS during non-blocking commits

[Maarten Lankhorst]

Op 31-01-18 om 20:57 schreef Harry Wentland:
> On 2018-01-30 05:28 AM, Maarten Lankhorst wrote:
>> Op 29-01-18 om 16:41 schreef Leo Li:
>>> 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 for the reminder, pushed.
>>
> Thanks, Maarten. I see it in drm-misc-next.
>
> Would someone be able to pull this into drm-misc-fixes as well, or can I just I apply this myself with the following dim commands?
>
> dim checkout drm-misc-fixes
> dim cherry-pick 1c6ceeee6ebb
> dim push-branch
My bad, pushed to the right branch. :)

Re: [Intel-gfx] [PATCH] drm/atomic: Fix memleak on ERESTARTSYS during non-blocking commits

[Maarten Lankhorst]

Op 31-01-18 om 20:57 schreef Harry Wentland:
> On 2018-01-30 05:28 AM, Maarten Lankhorst wrote:
>> Op 29-01-18 om 16:41 schreef Leo Li:
>>> 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 for the reminder, pushed.
>>
> Thanks, Maarten. I see it in drm-misc-next.
>
> Would someone be able to pull this into drm-misc-fixes as well, or can I just I apply this myself with the following dim commands?
>
> dim checkout drm-misc-fixes
> dim cherry-pick 1c6ceeee6ebb
> dim push-branch
My bad, pushed to the right branch. :)
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH] drm/atomic: Fix memleak on ERESTARTSYS during non-blocking commits

[Harry Wentland]

On 2018-02-01 05:30 AM, Maarten Lankhorst wrote:
> Op 31-01-18 om 20:57 schreef Harry Wentland:
>> On 2018-01-30 05:28 AM, Maarten Lankhorst wrote:
>>> Op 29-01-18 om 16:41 schreef Leo Li:
>>>> 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 for the reminder, pushed.
>>>
>> Thanks, Maarten. I see it in drm-misc-next.
>>
>> Would someone be able to pull this into drm-misc-fixes as well, or can I just I apply this myself with the following dim commands?
>>
>> dim checkout drm-misc-fixes
>> dim cherry-pick 1c6ceeee6ebb
>> dim push-branch
> My bad, pushed to the right branch. :)
> 

Thanks, Maarten.

Harry