[PATCH] drm/amd/display: Fix pageflipping for XOrg in Linux 5.11+

[Mario Kleiner]

On Sat, Jan 2, 2021 at 4:49 PM Bas Nieuwenhuizen
<bas at basnieuwenhuizen.nl> wrote:
>
> On Sat, Jan 2, 2021 at 4:05 PM Mario Kleiner <mario.kleiner.de at gmail.com> wrote:
> >
> > On Sat, Jan 2, 2021 at 3:05 PM Bas Nieuwenhuizen
> > <bas at basnieuwenhuizen.nl> wrote:
> > >
> > > I think the problem here is that application A can set the FB and then
> > > application B can use getfb2 (say ffmpeg).
> >
> >
> > Yes. That, and also the check for 'X' won't get us far, because if i
> > use my own software Psychtoolbox under Vulkan in direct display mode
> > (leased RandR outputs), e.g., under radv or amdvlk, then the ->comm
> > name will be "PTB mainthread" and 'P' != 'X'. But the Vulkan drivers
> > all use legacy pageflips as well in der WSI/drm, so if Vulkan gets
> > framebuffers with DCC modifiers, it would just fail the same way.
> >
> > Neither would it work to check for atomic client, as they sometimes
> > use atomic commit ioctl only for certain use cases, e.g., setting HDR
> > metadata, but still use the legacy pageflip ioctl for flipping.
> >
> > So that patch of mine is not the proper solution for anything non-X.
> >
> > >
> > > https://lists.freedesktop.org/archives/dri-devel/2021-January/292761.html
> > > would be my alternative patch.
> > >
> >
> > I also produced and tested hopefully better alternative to my original
> > one yesterday, but was too tired to send it. I just sent it out to
> > you:
> > https://lists.freedesktop.org/archives/dri-devel/2021-January/292763.html
> >
> > This one keeps the format_info check as is for non-atomic drivers, but
> > no longer rejects pageflip if the underlying kms driver is atomic. I
> > checked, and current atomic drivers use the drm_atomic... helper for
> > implementing legacy pageflips, and that helper just wraps the pageflip
> > into a "set new fb on plane" + atomic check + atomic commit.
> >
> > My understanding is that one can do these format changes safely under
> > atomic commit, so i hope this would be safe and future proof.
>
> So I think the difference between your patch and mine seem to boil
> down to whether we want any uabi extension, since AFAICT none of the
> pre-atomic drivers support modifiers.
>

That's a point: Although the uabi extension would only relax rules,
instead of tighten them, so current drm clients would be ok, i guess.

Afaict the current non-atomic modesetting drivers are:

gma500, shmobile, radeon, nouveau, amdgpu non-DC.

gma500, shmobile and radeon don't use modifiers, and probably won't
get any in the future?

Also amdgpu without DC? Atm. you only enable explicit modifiers for >=
FAMILY_AI, ie. Vega and later, and DC is a requirement for Vega and
later, so modifiers ==> DC ==> atomic.

But some of your code was moved from amdgpu_dm to amdgpu_display
specifically to allow compiling it without DC, and any client i tested
apart from Waylands weston (and that only for cursor planes) didn't
use addfb2 ioctl with modifiers at all, so all of X and Vulkan
currently hits the new convert_tiling_flags_to_modifier() fallback
code that converts old style tiling flags into modifiers. That
fallback path is the reason for triggering this issue in the first
place, as it converts some tiling flags to DCC/DCC-retile modifiers
and therefore changes the format_info.

Modifiers are only enabled if DC is on. So as long as nobody decides
to add modifiers in the legacy non-DC kms path, we'd be ok.

I'm less sure about nouveau. It uses modifiers, but has atomic support
only on nv50+ and that atomic support is off by default -- needs a
nouveau.nouveau_atomic=1 boot parameter to switch it on. It seems to
enable modifier support unconditionally regardless if atomic or not,
see:
https://elixir.bootlin.com/linux/v5.11-rc1/source/drivers/gpu/drm/nouveau/nouveau_display.c#L703

Atm. nouveau doesn't assign a new format_info though, so wouldn't
trigger this issue atm.

So i think the decision is about relaxing uabi a bit with my patch,
vs. less future-proofing with your patch?

Atm. both patches would solve the immediate problem, which is very
serious for my users' use cases, so I'd be ok with any of them. I just
don't want this issue to repeat in the future. Tracking it down killed
almost two full days for me, although I involuntarily learned more
about the current state of modifiers in kernel and user space than I
ever thought I wanted to know :/.

-mario



> >
> > > (I'm not good at detecting the effects of tearing  apparently but
> > > tested this avoids the pageflip failure by debug-prints)
> >
> >
> > XOrg log (e.g., ~/.local/share/xorg/XOrg0.log on current Ubuntu's) is
> > a good place on native XOrg, where the amdgpu-ddx was flooding the log
> > with present flip failures. Or drm.debug=4 for the kernel log.
> >
> > Piglit has the OML_sync_control tests for timing correctness, although
> > they are mostly pointless if not run in fullscreen mode, which they
> > are not by default.
> >
> > I can also highly recommend (sudo apt install octave-psychtoolbox-3)
> > on Debian/Ubutu based systems for X11. It is used for neuroscience and
> > medical research and critically depends on properly working pageflips
> > and timestamping on native X11/GLX under OpenGL and recently also
> > under Vulkan/WSI (radv,anv,amdvlk) in direct display mode. Working
> > FOSS AMD and Intel are especially critical for this research, so far
> > under X11+Mesa/OpenGL, but lately also under Vulkan direct display
> > mode. It has many built-in correctness tests and will shout angrily if
> > something software-detectable is broken wrt. pageflipping or timing.
> > E.g., octave-cli --eval PerceptualVBLSyncTest
> > PerceptualVBLSyncTest creates a flicker pattern that will tear like
> > crazy under Mesa if pageflipping isn't used. Also good to test
> > synchronization on dual-display setups, e.g., for udal-display stereo
> > presentation.
> >
> > I was actually surprised that this patch made it through the various
> > test suites and into drm-next. I thought page-flipping was covered
> > well enough somewhere.
>
> I don't think there are any tests using the AMDGPU implicit modifiers
> (not in IGT for anything except linear at least)
>
> >
> > Happy new year!
> > -mario
> >
> > >