On Wed, Jan 17, 2018 at 12:51:08PM +0100, Maarten Lankhorst wrote:
> From: "Leo (Sunpeng) Li" <sunpeng...@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: <sta...@vger.kernel.org> # v4.11+
> Signed-off-by: Leo (Sunpeng) Li <sunpeng...@amd.com>
> Acked-by: Harry Wentland <harry.wentl...@amd.com> #v1
> Signed-off-by: Maarten Lankhorst <maarten.lankho...@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-...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
