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

Sat Jan 2 15:49:47 UTC 2021

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.

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