On Sat, Jan 2, 2021 at 3:05 PM Bas Nieuwenhuizen <bas@xxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxx> > > Cc: Alex Deucher <alexander.deucher@xxxxxxx> > > Signed-off-by: Mario Kleiner <mario.kleiner.de@xxxxxxxxx> > > --- > > 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 > > _______________________________________________ amd-gfx mailing list amd-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/amd-gfx
