[PATCH] drm/amd/display: Skip fast cursor updates for fb changes

Grodzovsky, Andrey Andrey.Grodzovsky at amd.com
Fri Dec 14 19:06:16 UTC 2018


In general I agree with Michel that  DRM solution is required to 
properly address this but since now it's not really obvious what is the 
proper solution it seems to me OK to go with this fix until it's found.

Reviewed-by: Andrey Grodzovsky <andrey.grodzovsky at amd.com>

Andrey


On 12/14/2018 12:51 PM, Kazlauskas, Nicholas wrote:
> On 12/14/18 12:47 PM, Grodzovsky, Andrey wrote:
>>
>> On 12/14/2018 12:41 PM, Kazlauskas, Nicholas wrote:
>>> On 12/14/18 12:34 PM, Grodzovsky, Andrey wrote:
>>>> On 12/14/2018 12:26 PM, Nicholas Kazlauskas wrote:
>>>>> [Why]
>>>>> The behavior of drm_atomic_helper_cleanup_planes 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 is updated
>>>>> in place and has not been swapped the cleanup_fb call affects the new
>>>>> plane state.
>>>>>
>>>>> This results in a use after free for the given sequence:
>>>>>
>>>>> - Fast update, fb1 pin/ref, fb1 unpin/unref
>>>>> - Fast update, fb2 pin/ref, fb2 unpin/unref
>>>>> - Slow update, fb1 pin/ref, fb2 unpin/unref
>>>> Shouldn't you have use after free already at this point ?
>>>>
>>>> Andrey
>>> This assumes there was 1 reference on the bo. You would be getting use
>>> after free on every single update (be it fast or slow) if this wasn't
>>> the case.
>> Why would I get it on every single update if it's all balanced - first
>> pin/ref then unpin/unref ?
> It's balanced, but has a reference not from the atomic code path, ie.
> from creation.
>
> So this is actually:
>
> fb1 pin, refcount=2, b1 unpin refcount=1
>
>>> So in the case where there was 1 reference on fb2 it's actually freed at
>>> the end of slow update since the ref count is now 0. Then the use after
>>> free happens:
>> I still don't understand where exactly the 1 reference on fb2 during
>> slow update comes form
>> if all i see before that is 'Fast update, fb2 pin/ref, fb2 unpin/unref'
>> - can you clarify that ?
>>
>> Andrey
> There isn't any pin/ref on fb2 during the slow update. The pin/ref
> happens during a prepare_fb call - which is *always* called on
> new_plane_state. So the state looks like this in commit tail:
>
> old_plane_state->fb = fb2
> new_plane_state->fb = fb1
>
> Then at the end during cleanup planes, cleanup_fb is called on
> old_plane_state (fb2).
>
> Nicholas Kazlauskas
>
>>>>> - Fast update, fb2 pin/ref -> use after free. bug
>>> ...here.
>>>
>>> You can see this exact sequence happening in Michel's log.
>>>
>>> Nicholas Kazlauskas
>>>
>>>>> [How]
>>>>> Disallow framebuffer changes in the fast path. Since this includes
>>>>> a NULL framebuffer, this means that only framebuffers that have
>>>>> been previously pin+ref at least once will be used, preventing a
>>>>> use after free.
>>>>>
>>>>> This has a significant throughput reduction for cursor updates where
>>>>> the framebuffer changes. For most desktop usage this isn't a problem,
>>>>> but it does introduce performance regressions for two specific IGT
>>>>> tests:
>>>>>
>>>>> - cursor-vs-flip-toggle
>>>>> - cursor-vs-flip-varying-size
>>>>>
>>>>> Cc: Leo Li <sunpeng.li at amd.com>
>>>>> Cc: Harry Wentland <harry.wentland at amd.com>
>>>>> Cc: Michel Dänzer <michel at daenzer.net>
>>>>> Signed-off-by: Nicholas Kazlauskas <nicholas.kazlauskas at amd.com>
>>>>> ---
>>>>>       drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 10 ++++++++++
>>>>>       1 file changed, 10 insertions(+)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>>>> index d01315965af0..dc1eb9ec2c38 100644
>>>>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>>>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>>>> @@ -3628,10 +3628,20 @@ static int dm_plane_atomic_check(struct drm_plane *plane,
>>>>>       static int dm_plane_atomic_async_check(struct drm_plane *plane,
>>>>>       				       struct drm_plane_state *new_plane_state)
>>>>>       {
>>>>> +	struct drm_plane_state *old_plane_state =
>>>>> +		drm_atomic_get_old_plane_state(new_plane_state->state, plane);
>>>>> +
>>>>>       	/* Only support async updates on cursor planes. */
>>>>>       	if (plane->type != DRM_PLANE_TYPE_CURSOR)
>>>>>       		return -EINVAL;
>>>>>       
>>>>> +	/*
>>>>> +	 * DRM calls prepare_fb and cleanup_fb on new_plane_state for
>>>>> +	 * async commits so don't allow fb changes.
>>>>> +	 */
>>>>> +	if (old_plane_state->fb != new_plane_state->fb)
>>>>> +		return -EINVAL;
>>>>> +
>>>>>       	return 0;
>>>>>       }
>>>>>       



More information about the amd-gfx mailing list