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@xxxxxxx>
>>
>> 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@xxxxxxxxxxxxxxx> # v4.11+
>> Signed-off-by: Leo (Sunpeng) Li <sunpeng.li@xxxxxxx>
>> Acked-by: Harry Wentland <harry.wentland@xxxxxxx> #v1
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
>> ---
>> 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