On Sat, Jan 2, 2021 at 4:05 PM Mario Kleiner <mario.kleiner.de@xxxxxxxxx> wrote: > > 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. 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@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
