Re: Questions about KMS flip

["Michel Dänzer" <michel@daenzer.net>]

On 2021-11-16 15:10, Alex Deucher wrote:
> On Tue, Nov 16, 2021 at 3:09 AM Christian König
> <ckoenig.leichtzumerken@gmail.com> wrote:
>>
>> Am 16.11.21 um 09:00 schrieb Lang Yu:
>>> On Tue, Nov 16, 2021 at 08:14:08AM +0100, Christian KKKnig wrote:
>>>> Am 16.11.21 um 04:27 schrieb Lang Yu:
>>>>> On Mon, Nov 15, 2021 at 01:04:15PM +0100, Michel DDDnzer wrote:
>>>>>> [SNIP]
>>>>>>> Though a single call to dce_v*_0_crtc_do_set_base() will
>>>>>>> only pin the BO, I found it will be unpinned in next call to
>>>>>>> dce_v*_0_crtc_do_set_base().
>>>>>> Yeah, that's the normal case when the new BO is different from the old one.
>>>>>>
>>>>>> To catch the case I described, try something like
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c b/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
>>>>>>
>>>>>> index 18a7b3bd633b..5726bd87a355 100644
>>>>>>
>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
>>>>>>
>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
>>>>>>
>>>>>> @@ -1926,6 +1926,7 @@ static int dce_v11_0_crtc_do_set_base(struct drm_crtc *crtc,
>>>>>>
>>>>>>                   return r;
>>>>>>
>>>>>>
>>>>>>
>>>>>>           if (!atomic) {
>>>>>>
>>>>>> +               WARN_ON_ONCE(target_fb == fb);
>>>>>>
>>>>>>                   r = amdgpu_bo_pin(abo, AMDGPU_GEM_DOMAIN_VRAM);
>>>>>>
>>>>>>                   if (unlikely(r != 0)) {
>>>>>>
>>>>>>                           amdgpu_bo_unreserve(abo);
>>>>>>
>>>>> I did some tests, the warning can be triggered.
>>>>>
>>>>> pin/unpin operations in *_crtc_do_set_base() and
>>>>> amdgpu_display_crtc_page_flip_target() are mixed.
>>>> Ok sounds like we narrowed down the root cause pretty well.
>>>>
>>>> Question is now how can we fix this? Just not pin the BO when target_fb ==
>>>> fb?
>>> That worked. I did a few simple tests and didn't observe ttm_bo_release warnings
>>> any more.
>>>
>>> The pin/unpin logic,
>>>
>>> 1, fist crtc_mode_set, dce_v*_0_crtc_do_set_base() pins crtc->primary->fb(new),
>>> old_fb is NULL.
>>>
>>> 2, second crtc_mode_set, dce_v*_0_crtc_do_set_base() pins crtc->primary->fb(new),
>>> unpins old fb.
>>>
>>> 3, amdgpu_display_crtc_page_flip_target() pin/unpin operations.
>>>
>>> 4, third crtc_mode_set, dce_v*_0_crtc_do_set_base() pins crtc->primary->fb(new),
>>> unpins old fb (it is pinned in last call to amdgpu_display_crtc_page_flip_target)
>>>
>>> 5, amdgpu_display_crtc_page_flip_target() pin/unpin operations.
>>>
>>> .....
>>>
>>> x, reboot, amdgpu_display_suspend_helper() is called, the last pinned fb was unpinned.
>>>
>>> And I didn't observe amdgpu_bo_unpin() in dce_v*_0_crtc_disable() is called.
>>>
>>> If the logic is wrong, please correct me.
>>
>> I can't fully judge because I'm not that deep with my head in the old
>> display code, but from a ten mile high view it sounds sane to me. Michel
>> what do you think?

Sounds good to me.


Would be nice to address the other issue identified in this thread as well:

The pin in amdgpu_display_crtc_page_flip_target is guarded by if (!adev->enable_virtual_display), but the corresponding unpins in 
amdgpu_display_unpin_work_func & dce_v*_0_crtc_disable aren't. This probably results in pin count underflows with virtual display.


-- 
Earthling Michel Dänzer            |                  https://redhat.com
Libre software enthusiast          |         Mesa and Xwayland developer