Re: Questions about KMS flip

[Lang Yu <Lang.Yu@amd.com>]

On Mon, Nov 15, 2021 at 01:04:15PM +0100, Michel DDDnzer wrote:
> On 2021-11-15 12:31, Lang Yu wrote:
> > On Mon, Nov 15, 2021 at 10:49:39AM +0100, Michel DDDnzer wrote:
> >> On 2021-11-15 10:04, Lang Yu wrote:
> >>> On Mon, Nov 15, 2021 at 09:38:47AM +0100, Michel DDDnzer wrote:
> >>>> On 2021-11-15 07:41, Lang Yu wrote:
> >>>>> On Fri, Nov 12, 2021 at 05:10:27PM +0100, Michel DDDnzer wrote:
> >>>>>> On 2021-11-12 16:03, Christian König wrote:
> >>>>>>> Am 12.11.21 um 15:30 schrieb Michel Dänzer:
> >>>>>>>> On 2021-11-12 15:29, Michel Dänzer wrote:
> >>>>>>>>> On 2021-11-12 13:47, Christian König wrote:
> >>>>>>>>>> Anyway this unfortunately turned out to be work for Harray and Nicholas. In detail it's about this bug report here: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.kernel.org%2Fshow_bug.cgi%3Fid%3D214621&data=04%7C01%7CLang.Yu%40amd.com%7C07b2a4c13d2f4eba9ebb08d9a8300cd3%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637725746610767105%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=Qz2VKDE0%2BcIuXZMy0IL3NLZAjeXLimwl8PPZh4Cp4iE%3D&reserved=0
> >>>>>>>>>>
> >>>>>>>>>> Lang was able to reproduce the issue and narrow it down to the pin in amdgpu_display_crtc_page_flip_target().
> >>>>>>>>>>
> >>>>>>>>>> In other words we somehow have an unbalanced pinning of the scanout buffer in DC.
> >>>>>>>>> DC doesn't use amdgpu_display_crtc_page_flip_target AFAICT. The corresponding pin with DC would be in dm_plane_helper_prepare_fb, paired with the unpin in
> >>>>>>>>> dm_plane_helper_cleanup_fb.
> >>>>>>>>>
> >>>>>>>>>
> >>>>>>>>> With non-DC, the pin in amdgpu_display_crtc_page_flip_target is paired with the unpin in dm_plane_helper_cleanup_fb
> >>>>>>>> This should say amdgpu_display_unpin_work_func.
> >>>>>>>
> >>>>>>> Ah! So that is the classic (e.g. non atomic) path?
> >>>>>>
> >>>>>> Presumably.
> >>>>>>
> >>>>>>
> >>>>>>>>> & dce_v*_crtc_disable. One thing I notice is that the pin is guarded by if (!adev->enable_virtual_display), but the unpins seem unconditional. So could this be about virtual display, and the problem is actually trying to unpin a BO that was never pinned?
> >>>>>>>
> >>>>>>> Nope, my educated guess is rather that we free up the BO before amdgpu_display_unpin_work_func is called.
> >>>>>>>
> >>>>>>> E.g. not pin unbalance, but rather use after free.
> >>>>>>
> >>>>>> amdgpu_display_crtc_page_flip_target calls amdgpu_bo_ref(work->old_abo), and amdgpu_display_unpin_work_func calls amdgpu_bo_unref(&work->old_abo) only after amdgpu_bo_unpin. So what you describe could only happen if there's an imbalance elsewhere such that amdgpu_bo_unref is called more often than amdgpu_bo_ref, or maybe if amdgpu_bo_reserve fails in amdgpu_display_unpin_work_func (in which case the "failed to reserve buffer after flip" error message should appear in dmesg).
> >>>>>
> >>>>>
> >>>>> Actually, each call to amdgpu_display_crtc_page_flip_target() will
> >>>>>
> >>>>> 1, init a work(amdgpu_display_unpin_work_func) to unpin an old buffer
> >>>>>    (crtc->primary->fb), the work will be queued in dce_vX_0_pageflip_irq().
> >>>>>
> >>>>> 2, pin a new buffer, assign it to crtc->primary->fb. But how to unpin it?
> >>>>>    Next call.
> >>>>>
> >>>>> The problem is the pinned buffer of last call to 
> >>>>> amdgpu_display_crtc_page_flip_target() is not unpinned.
> >>>>
> >>>> It's unpinned in dce_v*_0_crtc_disable.
> >>>
> >>> I just found crtc->primary->fb is NULL when came in dce_v*_0_crtc_disable().
> >>> So it's not unpinned...
> >>
> >> __drm_helper_disable_unused_functions sets crtc->primary->fb = NULL only after calling crtc_funcs->disable. Maybe this path can get hit for a CRTC which was already disabled, in which case crtc->primary->fb == NULL in dce_v*_0_crtc_disable is harmless.
> >>
> >> Have you checked for the issue I described below? Should be pretty easy to catch.
> >>
> >>
> >>>> I think I've found the problem though: dce_v*_0_crtc_do_set_base pin the BO from target_fb unconditionally, but unpin the BO from the fb parameter only if it's different from the former. So if they're the same, the BO's pin count is incremented by 1.
> > 
> > Form my observations, amdgpu_bo_unpin() in dce_v*_0_crtc_disable() is
> > never called.
> 
> It would be expected to happen when the screen turns off, e.g. due to DPMS.
> 
> 
> > 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.

Regards,
Lang

> 
> -- 
> Earthling Michel Dänzer            |                  https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fredhat.com%2F&data=04%7C01%7CLang.Yu%40amd.com%7C07b2a4c13d2f4eba9ebb08d9a8300cd3%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637725746610767105%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=jct%2F2NG7LamVRzzpkSy9XjQIDZRBIutSeK3VQ2jh524%3D&reserved=0
> Libre software enthusiast          |         Mesa and Xwayland developer