On Wed, Jan 17, 2018 at 10:39 AM, Maarten Lankhorst
<maarten.lankhorst@xxxxxxxxxxxxxxx> 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@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. :(
>
yeah, fair enough.
Reviewed-by: Sean Paul <seanpaul@xxxxxxxxxxxx>
> ~Maarten
>