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

[Mario Kleiner]

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.

> (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.

Happy new year!
-mario

>
> On Thu, Dec 31, 2020 at 9:52 PM Mario Kleiner
> <mario.kleiner.de at gmail.com> wrote:
> >
> > Commit 816853f9dc4057b6c7ee3c45ca9bd5905 ("drm/amd/display: Set new
> > format info for converted metadata.") may fix the getfb2 ioctl, but
> > in exchange it completely breaks all pageflipping for classic user
> > space, e.g., XOrg, as tested with both amdgpu-ddx and modesetting-ddx.
> > This leads to massive tearing, broken visual timing/timestamping etc.
> >
> > Reason is that the classic pageflip ioctl doesn't allow a fb format
> > change during flip, and at least X uses classic pageflip ioctl and no
> > atomic modesetting api at all.
> >
> > As one attempted workaround, only set the new format info for converted
> > metadata if the calling client isn't X. Not sure if this is the best
> > way, or if a better check would not be "not all atomic clients" or
> > similar? In any case it works for XOrg X-Server. Checking the ddx
> > code of intel-ddx/modesetting-ddx/amdgpu-ddx as well as grepping over
> > Mesa doesn't show any users of the getfb2 ioctl(), so the need for this
> > format info assignment seems to be more the exception than the rule?
> >
> > Fixes: 816853f9dc40 ("drm/amd/display: Set new format info for converted metadata.")
> > Cc: Bas Nieuwenhuizen <bas at basnieuwenhuizen.nl>
> > Cc: Alex Deucher <alexander.deucher at amd.com>
> > Signed-off-by: Mario Kleiner <mario.kleiner.de at gmail.com>
> > ---
> >  drivers/gpu/drm/amd/amdgpu/amdgpu_display.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> > index f764803c53a4..cb414b3d327a 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> > @@ -828,7 +828,8 @@ static int convert_tiling_flags_to_modifier(struct amdgpu_framebuffer *afb)
> >                         if (!format_info)
> >                                 return -EINVAL;
> >
> > -                       afb->base.format = format_info;
> > +                       if (afb->base.comm[0] != 'X')
> > +                               afb->base.format = format_info;
> >                 }
> >         }
> >
> > --
> > 2.25.1
> >