[PATCH] drm/amd/display: Don't cleanup new cursor fb in fast cursor update

[Kazlauskas, Nicholas]

On 12/14/18 10:01 AM, Michel Dänzer wrote:
> On 2018-12-14 3:48 p.m., Nicholas Kazlauskas wrote:
>> [Why]
>> The behavior of dm_plane_helper_cleanup_fb differs depending on
>> whether the commit was asynchronous or not. When it's called from
>> amdgpu_dm_atomic_commit_tail during a typical atomic commit the
>> plane state has been swapped so it calls cleanup_fb on the old plane
>> state.
>>
>> However, in the asynchronous commit codepath the call to
>> drm_atomic_helper_commit also calls dm_plane_helper_cleanup_fb after
>> atomic_async_update has been called. Since the plane state has not
>> been swapped, the cleanup_fb call affects the new plane state with the
>> active fb.
>>
>> The slow, full path is only ever used for the cursor update when the
>> cursor CRTC changes. If there was previously a fb in the state before
>> that had gone through the fast asynchronous path then cleanup_fb will
>> be called a second time on an fb that was previously unpinned/unref'd.
>>
>> This results in a use after free on the next cursor update.
>>
>> [How]
>> Regardless of whether this was intentional behavior in DRM or not,
>> the fix is to manually call cleanup_fb on the old plane state's fb.
>>
>> Since the pointer to the old_plane_state is actually the current
>> plane->state in this function, a backup is needed. This has a minor
>> change to behavior for handle_cursor_update since it now correctly uses
>> the old state.
>>
>> The next step is to prevent the cleanup_fb call from DRM from unpinning
>> the active new_state->fb. Setting new_state->fb to NULL would be
>> enough to handle this, but DRM has a warning in the
>> drm_atomic_helper_async_commit helper if
>> plane->state->fb != new_state->fb.
>>
>> A new field was added (cursor_update) to dm_plane_state that's only
>> ever set to true during the fast path. If it's true, then cleanup_fb
>> doesn't unpin and unref the fb.
>>
>> Cc: Leo Li <sunpeng.li at amd.com>
>> Cc: Harry Wentland <harry.wentland at amd.com>
>> Cc: Michel Dänzer <michel.daenzer at amd.com>
>> Signed-off-by: Nicholas Kazlauskas <nicholas.kazlauskas at amd.com>
>>
>> [...]
>>   
>> @@ -3653,7 +3654,15 @@ static void dm_plane_atomic_async_update(struct drm_plane *plane,
>>   	plane->state->crtc_w = new_state->crtc_w;
>>   	plane->state->crtc_h = new_state->crtc_h;
>>   
>> -	handle_cursor_update(plane, old_state);
>> +	handle_cursor_update(plane, &old_state);
>> +
>> +	/*
>> +	 * While DRM already takes care of drm_atomic_helper_cleanup_planes,
>> +	 * it does it on the wrong state (new_state instead of old_state).
>> +	 * This is a workaround for that behavior.
>> +	 */
>> +	dm_plane_helper_cleanup_fb(plane, &old_state);
>> +	dm_plane_state->cursor_update = true;
> 
> We shouldn't work around DRM core code in driver code. Please work on a
> solution with the core code developers on the dri-devel list, and revert
> the cursor fast path for now.
> 
> 

I'd rather not revert the patch given how much it improves the user 
experience. Some compositors (and wine games) are unusable or unplayable 
without it.

In general I agree with not doing core workarounds, but I'm not even 
sure if this is a bug in core DRM to begin with. Since the plane state 
modification is in place if you free the old_state then you're freeing 
what's currently active on plane->state. This works for us since we only 
work on the ->fb during prepare/cleanup, but this isn't true in general.

The only other way to do this is not using 
async_commit_check/async_commit_update and reimplementing atomic commit 
behavior for the fast path. Intel does this with their legacy cursor 
update fast path - it's a lot more code and it involves using 
"deprecated" elements of the atomic API.

Nicholas Kazlauskas