Questions about KMS flip

[Alex Deucher]

On Tue, Nov 16, 2021 at 3:09 AM Christian König
<ckoenig.leichtzumerken at 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?
>
> BTW: Nicholas are there any plans to get rid of all that stuff? It would
> be a really nice cleanup of rather flaky code I think.

We just need to add analog support to the DC code.  Darren was looking into it.

Alex


>
> Thanks,
> Christian.
>
> >
> > Regards,
> > Lang
> >
> >> Thanks,
> >> Christian.
> >>
> >>> Regards,
> >>> Lang
> >>>
>