[PATCH] drm/amd/display: Fix deadlock with display during hanged ring recovery.

[Christian König]

Am 14.02.19 um 20:03 schrieb Grodzovsky, Andrey:
> On 2/14/19 12:54 PM, Kazlauskas, Nicholas wrote:
>> On 2/14/19 12:47 PM, Grodzovsky, Andrey wrote:
>>> On 2/14/19 11:57 AM, Kazlauskas, Nicholas wrote:
>>>> On 2/14/19 11:47 AM, Grodzovsky, Andrey wrote:
>>>>> On 2/14/19 11:07 AM, Michel Dänzer wrote:
>>>>>> On 2019-02-14 4:54 p.m., Kazlauskas, Nicholas wrote:
>>>>>>> On 2/14/19 10:42 AM, Grodzovsky, Andrey wrote:
>>>>>>>> On 2/14/19 4:05 AM, Christian König wrote:
>>>>>>>>> Am 13.02.19 um 19:58 schrieb Andrey Grodzovsky:
>>>>>>>>>> When ring hang happens amdgpu_dm_commit_planes during flip is holding
>>>>>>>>>> the BO reserved and then stack waiting for fences to signal in
>>>>>>>>>> reservation_object_wait_timeout_rcu (which won't signal because there
>>>>>>>>>> was a hnag). Then when we try to shutdown display block during reset
>>>>>>>>>> recovery from drm_atomic_helper_suspend we also try to reserve the BO
>>>>>>>>>> from dm_plane_helper_cleanup_fb ending in deadlock.
>>>>>>>>>> Also remove useless WARN_ON
>>>>>>>>> Well it is good that you pointed this out, but there are more problems
>>>>>>>>> than just waiting wile the BO is reserved here.
>>>>>>>>>
>>>>>>>>>> Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky at amd.com>
>>>>>>>>>> ---
>>>>>>>>>>          drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 19
>>>>>>>>>> +++++++++++++------
>>>>>>>>>>          1 file changed, 13 insertions(+), 6 deletions(-)
>>>>>>>>>>
>>>>>>>>>> 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 acc4ff8..f8dec36 100644
>>>>>>>>>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>>>>>>>>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>>>>>>>>> @@ -4802,14 +4802,21 @@ static void amdgpu_dm_commit_planes(struct
>>>>>>>>>> drm_atomic_state *state,
>>>>>>>>>>                       */
>>>>>>>>>>                      abo = gem_to_amdgpu_bo(fb->obj[0]);
>>>>>>>>>>                      r = amdgpu_bo_reserve(abo, true);
>>>>>>>>> Well why do we reserve the BO in the first place? As the name
>>>>>>>>> indicates reservation_object_wait_timeout_rcu() just uses rcu to wait
>>>>>>>>> for the BO to be idle, no need to actually reserve it at all.
>>>>>>>> That a good point, I honestly can't remember any more why it's here...
>>>>>>>> It does look unnecessary given that we are not validating the BO here.
>>>>>>> I think we reserve it to grab the tiling flags to update the plane
>>>>>>> address (and some other bits of the plane state with a patch I'm
>>>>>>> currently working on). I'm guessing that we still need the reservation
>>>>>>> so nothing else tries to grab it after it becomes idle, but I'm not
>>>>>>> overly familiar with that bit.
>>>>>> The BO is already pinned at this point, isn't it? That's enough to
>>>>>> prevent it from moving, no reservation needed.
>>>>> Yes, agree.
>>>>>
>>>>> Andrey
>>>>>
>>>>>
>>>> I'm not worried about the buffer moving so much as I am about userspace
>>>> changing the tiling flags by calling the IOCTL. Can that happen when
>>>> it's pinned?
>>>>
>>>> Nicholas Kazlauskas
>>> In any case this has nothing to do with reserving the BO.
>>>
>>> Andrey
>>>
>>>
>> Technically it does? See the function:
>>
>> void amdgpu_bo_get_tiling_flags(struct amdgpu_bo *bo, u64 *tiling_flags)
>> {
>> 	lockdep_assert_held(&bo->tbo.resv->lock.base);
>>
>> 	if (tiling_flags)
>> 		*tiling_flags = bo->tiling_flags;
>> }
>>
>> Nicholas Kazlauskas
> What I meant that it has nothing to do with this BO reservation in the
> flip code while waiting for fences. Seems it's dafe to remove it.

Yeah, agree. That looks actually quite superfluous to me.

Following up on Michel's argument you would actually need to keep the BO 
reserved until the scanout is completed when you really want to prevent 
that the tilling flags don't change.

I would just completely drop that function and access bo->tilling_flags 
directly here.

Christian.

>
> Andrey
>
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx