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

[Kazlauskas, Nicholas]

On 12/14/18 10:19 AM, Kazlauskas, Nicholas wrote:
> 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
> _______________________________________________
> amd-gfx mailing list
> amd-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
> 

A simpler solution that I would still be fine with is to just disallow 
framebuffer changes in the fast path until DRM behavior is clarified or 
fixed. This would work with DRM freeing either the new_plane_state or 
the old_plane_state - it's the same FB in either case.

This will still resolve most cursor stuttering problems while being 
significantly less hacky and easier to revert.

It unfortunately does affect performance quite a bit for cases where the 
the cursor is changing the framebuffer, but you don't see this in most 
desktop usage. Just in the cursor-vs-flip-toggle and 
cursor-vs-flip-varying-size igt tests.

Nicholas Kazlauskas