Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Bisected] Color corruption on intel graphics #356

Open
Samsagax opened this issue Dec 22, 2021 · 100 comments
Open

[Bisected] Color corruption on intel graphics #356

Samsagax opened this issue Dec 22, 2021 · 100 comments

Comments

@Samsagax
Copy link

Being using gamescope on my AMD machine for a while without issues. When trying out on my laptop Nvidia won't let me use it because of #151 but I managed to start gamescope on the intel GPU (iGPU) and then start games from it using the Nvidia GPU (dGPU) by setting some env variables (pretty cool, huh?).

Here is the issue: I started steam client within gamescope on my intel card by forcing VK_ICD_FILENAMES variable like so

$ VK_ICD_FILENAMES=/usr/share/vulkan/icd.d/intel_icd.x86_64.json gamescope -- steam

The client launches fine but then the colors are corrupted:
Screenshot_20211222_132212

But I managed to start a native game (selected Factorio, then Start) and then it all works inside the game (Launched with __NV_PRIME* variables for it to use the dGPU):
Screenshot_20211222_132503

Also when the game is launching the window flickers back and forth with the corrupted colors client and the correct colored game. After the game is started, it all "just works".

@l3s2d
Copy link

l3s2d commented Dec 29, 2021

Noticed this after upgrading to 3.10.3 on Arch Linux. Goes away after downgrading to 3.9.5.

@Samsagax
Copy link
Author

Noticed this after upgrading to 3.10.3 on Arch Linux. Goes away after downgrading to 3.9.5.

Great. Never noticed this before but with this I'll try to bisect it.

@Samsagax
Copy link
Author

Samsagax commented Jan 1, 2022

Ok I managed to bisect it. Here is the log.

git bisect start
# good: [26aeebf11d2d195fd2fa75f0d85d11dcd3ee5e69] steamcompmgr: publish input change count
git bisect good 26aeebf11d2d195fd2fa75f0d85d11dcd3ee5e69
# bad: [dd22f8db76aa9726b64d7a21095d669aec3b3732] steamcompmgr: Fix validContents nullptr deref
git bisect bad dd22f8db76aa9726b64d7a21095d669aec3b3732
# good: [6402b5cd40c78e0972348133ffe1a57cedbb807c] steamcompmgr: Fix child window cursor barrier
git bisect good 6402b5cd40c78e0972348133ffe1a57cedbb807c
# good: [7e93200e084f49dc8b68b8d0f85ad15afccf2e9e] Dedupe and memoize fb_id and vulkan texture per buffer.
git bisect good 7e93200e084f49dc8b68b8d0f85ad15afccf2e9e
# bad: [296a3d498d02c1277e4de7ed2e67ad537b1d3553] steamcompmgr: Add framework for held commits
git bisect bad 296a3d498d02c1277e4de7ed2e67ad537b1d3553
# bad: [93e4d94a78c1d371f1e2112c6386fe3dfd085bb9] steamcompmgr: Add --cursor-hotspot
git bisect bad 93e4d94a78c1d371f1e2112c6386fe3dfd085bb9
# bad: [4ee56a1e75a043c6f317ea2f71c96b2061e85cf5] rendervulkan: Do not premultiply color
git bisect bad 4ee56a1e75a043c6f317ea2f71c96b2061e85cf5
# bad: [23430084e2245e226f27dbe1882bd30633a1d50c] rendervulkan: Blend in linear space
git bisect bad 23430084e2245e226f27dbe1882bd30633a1d50c
# first bad commit: [23430084e2245e226f27dbe1882bd30633a1d50c] rendervulkan: Blend in linear space

And the first bad commit:

23430084e2245e226f27dbe1882bd30633a1d50c is the first bad commit
commit 23430084e2245e226f27dbe1882bd30633a1d50c
Author: Joshua Ashton <joshua@froggi.es>
Date:   Sat Nov 13 22:28:25 2021 +0000

    rendervulkan: Blend in linear space

    We must blend in linear space, not SRGB space or we get these horrible browns.

    Gets us closer to how this looks with planes.

 src/composite.comp   | 28 ++++++++++++++++++++++++++--
 src/rendervulkan.cpp | 27 ++++++++++++++-------------
 2 files changed, 40 insertions(+), 15 deletions(-)

@Samsagax Samsagax changed the title Color corruption on intel integrated graphics [Bisected] Color corruption on intel integrated graphics Jan 1, 2022
@Joshua-Ashton
Copy link
Collaborator

Once we fix validation stuff and add acquire barrier itd make sense to open an issue on Mesa if it isnt fixed.

@leonidx86
Copy link

Reporting my test on two different Intel iGPUs, maybe this will help to locate the issue.
For both tests the Linux distro and package versions are the same:

  • Kernel: 5.16.3.arch1-1
  • Mesa: 21.3.5-1
  • Gamescope: 3.11.0beta4-1

gamescope -W 320 -H 320 -m 1 glxgears

GPU: Intel(R) HD Graphics 4000 (IVB GT2)
test_4000

GPU: Intel(R) UHD Graphics 630 (CFL GT2)
test_630

@Samsagax
Copy link
Author

Might worth a try with mesa-git package to see if it's fixed upstream.

@leonidx86
Copy link

Might worth a try with mesa-git package to see if it's fixed upstream.

Tested now, same result with mesa-git (version 22.0.0_devel.149533.24fef8f33da.d41d8cd98f00b204e9800998ecf8427e-1).

@DadSchoorse
Copy link
Collaborator

This should be reported to mesa, as far as I know we do everything correctly now. All the validation errors have been fixed.

@vignedev
Copy link

vignedev commented Feb 2, 2022

Not sure how helpful this might be, but I've been experimenting/hacking a few changes to try to get it visually working on my setup.

There are two scenarios that I have tested, both involving changes in the s_DRMVKFormatTable structure and both resulting in a non-glitch state, however with different bugs:


  1. Forcing both vkFormat and vkFormatSrgb to be their sRGB variant
struct {
	uint32_t DRMFormat;
	VkFormat vkFormat;
	VkFormat vkFormatSrgb;
	bool bHasAlpha;
} s_DRMVKFormatTable[] = {
-       { DRM_FORMAT_ARGB8888, VK_FORMAT_B8G8R8A8_UNORM, VK_FORMAT_B8G8R8A8_SRGB, true },
-       { DRM_FORMAT_XRGB8888, VK_FORMAT_B8G8R8A8_UNORM, VK_FORMAT_B8G8R8A8_SRGB, false },
+       { DRM_FORMAT_ARGB8888, VK_FORMAT_B8G8R8A8_SRGB, VK_FORMAT_B8G8R8A8_SRGB, true },
+       { DRM_FORMAT_XRGB8888, VK_FORMAT_B8G8R8A8_SRGB, VK_FORMAT_B8G8R8A8_SRGB, false },
	{ DRM_FORMAT_NV12, VK_FORMAT_G8_B8R8_2PLANE_420_UNORM, VK_FORMAT_G8_B8R8_2PLANE_420_UNORM, false },
	{ DRM_FORMAT_INVALID, VK_FORMAT_UNDEFINED, VK_FORMAT_UNDEFINED, false },
};

This makes gamescope run with correct colors, however enabling FSR results in black screen.

Without FSR With FSR
srgb_nofsr srgb_fsr

  1. Forcing both vkFormat and vkFormatSrgb to be their UNNORM variant
struct {
	uint32_t DRMFormat;
	VkFormat vkFormat;
	VkFormat vkFormatSrgb;
	bool bHasAlpha;
} s_DRMVKFormatTable[] = {
-       { DRM_FORMAT_ARGB8888, VK_FORMAT_B8G8R8A8_UNORM, VK_FORMAT_B8G8R8A8_SRGB, true },
-       { DRM_FORMAT_XRGB8888, VK_FORMAT_B8G8R8A8_UNORM, VK_FORMAT_B8G8R8A8_SRGB, false },
+       { DRM_FORMAT_ARGB8888, VK_FORMAT_B8G8R8A8_UNORM, VK_FORMAT_B8G8R8A8_UNORM, true },
+       { DRM_FORMAT_XRGB8888, VK_FORMAT_B8G8R8A8_UNORM, VK_FORMAT_B8G8R8A8_UNORM, false },

	{ DRM_FORMAT_NV12, VK_FORMAT_G8_B8R8_2PLANE_420_UNORM, VK_FORMAT_G8_B8R8_2PLANE_420_UNORM, false },
	{ DRM_FORMAT_INVALID, VK_FORMAT_UNDEFINED, VK_FORMAT_UNDEFINED, false },
};

This makes gamescope run in incorrect washed-out colors (obviously due to color conversion), however when using FSR all colors return into their properness.
This however required the revertion of changes brought by eff99fd to get it working.

Without FSR With FSR
unorm_nofsr unorm_fsr

I'm not experienced at all in these regions and I found these changes purely by trial and error, so I am unsure of any possible side-effects nor the full purpose of such changes, however I hope it will help narrow the issue down somewhat.

@patrickaldis
Copy link

I can confirm I'm having near identical same issues on an 7700HQ under intel UHD 630

Created an issue here, but realising this is actually the same as this thread

#393

@DadSchoorse
Copy link
Collaborator

@vignedev All of that indicates that it's a driver issue, so something that should be reported to mesa.

@patrickaldis
Copy link

@DadSchoorse Would this really be a mesa issue if the game runs without gamescope on mesa?

@DadSchoorse
Copy link
Collaborator

@patrickaldis There's always the possibility that there's some subtle bug in our code, but given that gamescope works fine on radv I think it's an anv problem. The hacks by @vignedev show that there's is some issue with mutable images + dmabufs and with the import barriers. The game doesn't matter.

@leonidx86
Copy link

As an anecdotal evidence, I've tested this on several different Intel CPUs with integrated graphics. Only one of them (UHD 630) has this visual artifact (but works fine without Gamescope). Everything else works fine with or without Gamescope.

@Samsagax Which Intel CPU/iGPU have you tested on?
@l3s2d Says the issue is not present on v3.9.5, but I can't downgrade, since there is no package below v3.11.0 at https://archive.archlinux.org/packages/g/gamescope/.

@ccajas
Copy link

ccajas commented Feb 6, 2022

I'd like to add my two cents in, and say that these problems occur on most games I've tried, but there are at least a few games where Gamescope is mostly working fine with Intel UHD graphics. I'm running it on a Celeron N4120 with UHD 600 graphics.

I've tested out Portal and Lara Croft: Temple of Osiris and both these games are playable with Gamescope, at least in all the sections with real-time 3D graphics.

Portal is very playable, only seeing image artifacts in the Valve splash screen and the death sequences. Temple of Osiris starts with a setup window where artifacts show up, but once you get past that window it all looks fine.

@Samsagax
Copy link
Author

Samsagax commented Feb 6, 2022

As an anecdotal evidence, I've tested this on several different Intel CPUs with integrated graphics. Only one of them (UHD 630) has this visual artifact (but works fine without Gamescope). Everything else works fine with or without Gamescope.

@Samsagax Which Intel CPU/iGPU have you tested on? @l3s2d Says the issue is not present on v3.9.5, but I can't downgrade, since there is no package below v3.11.0 at https://archive.archlinux.org/packages/g/gamescope/.

I have an i5 6200U cpu, the iGPU is a 520 (SKL GT2).

@Samsagax
Copy link
Author

Samsagax commented Feb 7, 2022

For what is worth, seems to be a problem with the vulkan bit of the intel driver for those iGPUs. Games that use OpenGL will not have this issue.

Does anyone have an account on the mesa gitlab instance to report this? I think we colected valuable information and should be able to report it to be fixed easily.

@l3s2d Says the issue is not present on v3.9.5, but I can't downgrade, since there is no package below v3.11.0 at https://archive.archlinux.org/packages/g/gamescope/.

You would need to use git tags for this. Use the gamescope-git package as a reference.

@Oschowa
Copy link

Oschowa commented Feb 12, 2022

I can reproduce this issue on my Kaby Lake UHD 620 using Mesa 21.3.5. However, on gamescope master and latest Mesa main it's working fine. INTEL_DEBUG=norbc also makes it work fine on Mesa 21.3.5 (it will however likely reduce performance).
This Mesa commit that fixes the issue is https://gitlab.freedesktop.org/mesa/mesa/-/commit/ca791f5c5d88a6451e4934f3e03fa0e38a7759fa

@leonidx86
Copy link

@Oschowa You're right! With INTEL_DEBUG=norbc it works fine. However, with latest Mesa (22.1.0) and relatively latest Gamescope (3.11.0beta6-1) the issue is still there on UHD 630.

@Oschowa
Copy link

Oschowa commented Feb 12, 2022

Yes, it seems like latest Mesa only fixes it for vulkan apps running in gamescope, anything using OpenGL is still broken. I opened a Mesa issue about this here https://gitlab.freedesktop.org/mesa/mesa/-/issues/6029

@JordanViknar
Copy link

I can confirm I also encounter this issue running Minecraft Java Edition or glxgears on my Intel HD Graphics 500 iGPU.

@patrickaldis
Copy link

patrickaldis commented Mar 6, 2022

The artifacts in my case seem to be more extreme:
Screenshot from 2022-03-06 09-26-55

@adolfintel
Copy link

I also have this problem on Intel UHD 600 (Gemini Lake)

@Samsagax
Copy link
Author

Samsagax commented Mar 28, 2022

I'm currently testing some stuff and the issue seems to be fixed. Could anyone please confirm?
Relevant package versions:

  • linux-zen 5.16.16.zen1-1
  • mesa 21.3.7-2
  • vulkan-intel 21.3.7-2
  • vulkan-icd-loader 1.3.208-1

Screenshot_20220328_121030

@leonidx86
Copy link

@Samsagax Nope. Even installed the Zen Kernel. The only major difference with your config seems to be the GPU.

  • GPU Intel(R) UHD Graphics 630 (CFL GT2)
  • linux 5.16.16-arch1-1 and 5.16.16-zen1-1-zen
  • mesa 21.3.7-2
  • vulkan-intel 21.3.7-2
  • vulkan-icd-loader 1.3.208-1

@JordanViknar
Copy link

I'm currently testing some stuff and the issue seems to be fixed. Could anyone please confirm? Relevant package versions:

  • linux-zen 5.16.16.zen1-1
  • mesa 21.3.7-2
  • vulkan-intel 21.3.7-2
  • vulkan-icd-loader 1.3.208-1

Screenshot_20220328_121030

The issue still occurs for me, using the latest Arch Linux packages.

@mrozigor
Copy link

gamescope glxgears. I've tried also to downgrade to mesa-24.0.1 but with same result.

@djdeath
Copy link

djdeath commented Mar 10, 2024

Thanks, I don't have access to an Icelake machine, but I'll try to see with a colleague.

Though for me the tip of tree of gamescope is just crashing for a completely different reason : #1188

@mrozigor
Copy link

Let me know if I can help you in any way.

@CaiCarvalho
Copy link

The problem still occurs if I try to use any --filter besides linear or start it with vkBasalt enabled.

Currently on:

  • Arch Linux x86_64
  • linux-6.8.0-273-tkg-pds-llvm
  • Mesa 24.1.0-devel (git-4fc2ab43c0)
  • Intel® UHD Graphics (ICL GT1)

Problem also happens to Mesa stable releases.

@djdeath
Copy link

djdeath commented Mar 27, 2024

I'm still blocked by #1188 to investigate this.

@nchery-intel
Copy link

nchery-intel commented Apr 5, 2024

My guess for the ICL failure is that gamescope is performing excessive layout transitions from UNDEFINED:

.oldLayout = (state.discarded || state.needsImport) ? VK_IMAGE_LAYOUT_UNDEFINED : VK_IMAGE_LAYOUT_GENERAL,

result.first->second.needsImport = image->externalImage();

@djdeath
Copy link

djdeath commented Apr 9, 2024

My guess for the ICL failure is that gamescope is performing excessive layout transitions from UNDEFINED:

.oldLayout = (state.discarded || state.needsImport) ? VK_IMAGE_LAYOUT_UNDEFINED : VK_IMAGE_LAYOUT_GENERAL,

result.first->second.needsImport = image->externalImage();

Hmm makes sense.

The problem is that there is also this VU that forces the import operation to use UNDEFINED :

VUID-VkImageMemoryBarrier-oldLayout-01197
If srcQueueFamilyIndex and dstQueueFamilyIndex define a [queue family ownership transfer](https://registry.khronos.org/vulkan/specs/1.3-extensions/man/html/VkImageMemoryBarrier.html#synchronization-queue-transfers) or oldLayout and newLayout define an [image layout transition](https://registry.khronos.org/vulkan/specs/1.3-extensions/man/html/VkImageMemoryBarrier.html#synchronization-image-layout-transitions), oldLayout must be VK_IMAGE_LAYOUT_UNDEFINED or the current layout of the image subresources affected by the barrier

The spec also kind of contradicts itself by saying using the undefined layout might loose the data :

VK_IMAGE_LAYOUT_UNDEFINED specifies that the layout is unknown. Image memory cannot be transitioned into this layout. This layout can be used as the initialLayout member of [VkImageCreateInfo](https://registry.khronos.org/vulkan/specs/1.3-extensions/man/html/VkImageCreateInfo.html). This layout can be used in place of the current image layout in a layout transition, but doing so will cause the contents of the image’s memory to be undefined.

We might have to ignore the undefined layout in case of external queue transitions...

@nchery-intel
Copy link

nchery-intel commented Apr 9, 2024

My guess for the ICL failure is that gamescope is performing excessive layout transitions from UNDEFINED:

.oldLayout = (state.discarded || state.needsImport) ? VK_IMAGE_LAYOUT_UNDEFINED : VK_IMAGE_LAYOUT_GENERAL,

result.first->second.needsImport = image->externalImage();

Hmm makes sense.

The problem is that there is also this VU that forces the import operation to use UNDEFINED :

VUID-VkImageMemoryBarrier-oldLayout-01197
If srcQueueFamilyIndex and dstQueueFamilyIndex define a [queue family ownership transfer](https://registry.khronos.org/vulkan/specs/1.3-extensions/man/html/VkImageMemoryBarrier.html#synchronization-queue-transfers) or oldLayout and newLayout define an [image layout transition](https://registry.khronos.org/vulkan/specs/1.3-extensions/man/html/VkImageMemoryBarrier.html#synchronization-image-layout-transitions), oldLayout must be VK_IMAGE_LAYOUT_UNDEFINED or the current layout of the image subresources affected by the barrier

The spec also kind of contradicts itself by saying using the undefined layout might loose the data :

VK_IMAGE_LAYOUT_UNDEFINED specifies that the layout is unknown. Image memory cannot be transitioned into this layout. This layout can be used as the initialLayout member of [VkImageCreateInfo](https://registry.khronos.org/vulkan/specs/1.3-extensions/man/html/VkImageCreateInfo.html). This layout can be used in place of the current image layout in a layout transition, but doing so will cause the contents of the image’s memory to be undefined.

We might have to ignore the undefined layout in case of external queue transitions...

Apps should be able to account for these rules by doing any import operations before rendering, no? If the driver ignores the layout, apps depending on this behavior could get GPU hangs.

The reason I think excessive layout transitions is the issue is because gamescope seems to consider the dmabuf as not imported every time it's read from (follow the call stack from the second referenced line of code). That causes it to do a layout transition from UNDEFINED more than necessary. Assuming an app didn't perform import operations before rendering, I would expect a single import and layout transition from UNDEFINED to cause a single frame of corruption.

@Joshua-Ashton
Copy link
Collaborator

We've had this discussion a bunch before. FWIR, when transitioning from a foreign queue, drivers must ignore the srcLayout.

@sharkautarch
Copy link

We've had this discussion a bunch before. FWIR, when transitioning from a foreign queue, drivers must ignore the srcLayout.

Someone had commented on the mesa issue thread a year ago about this:
https://gitlab.freedesktop.org/mesa/mesa/-/issues/6029#note_1513967

Where they basically confirmed what you just said
Tho they also said for importing from 'EXTERNAL' queues, it was unclear from the vulkan spec what the correct behavior would be

@sharkautarch
Copy link

@djdeath @nchery-intel @mrozigor
I have no clue if this will actually help at all, but I did have one idea of removing the image format qualifiers from the glsl shader code (since apparently you don't need them w/ vulkan glsl)
https://github.com/sharkautarch/gamescope/tree/glsl_comp_w_o_image_format
if you're up to testing it out, just clone my fork, and do a git checkout glsl_comp_w_o_image_format

I don't really expect this to actually fix the issue, but I guess it's worth a shot?

@mrozigor
Copy link

mrozigor commented Apr 9, 2024

I've rebuild gamescope with those changes but still doesn't work correctly.

@djdeath
Copy link

djdeath commented Apr 9, 2024

We've had this discussion a bunch before. FWIR, when transitioning from a foreign queue, drivers must ignore the srcLayout.

Someone had commented on the mesa issue thread a year ago about this: https://gitlab.freedesktop.org/mesa/mesa/-/issues/6029#note_1513967

Where they basically confirmed what you just said Tho they also said for importing from 'EXTERNAL' queues, it was unclear from the vulkan spec what the correct behavior would be

Thanks, I asked on the Khronos gitlab and got forwarded to the discussion from a while ago : https://gitlab.khronos.org/vulkan/vulkan/-/issues/2692

So it looks like we need to start handling the case where oldLayout = UNDEFINED in Anv. We should probably limit that to images with modifiers having compression though, because we can have separated compression not living in the shared buffer and that one would still need to be put back to UNDEFINED.

@djdeath
Copy link

djdeath commented Apr 15, 2024

Is https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/28742 helping?

@mrozigor
Copy link

Yes, it helped! I've tried mesa-24.0.5 first, but with no change. Then after applying above patch gamescope works properly. Thanks!

@djdeath
Copy link

djdeath commented Apr 16, 2024

So I've been discussing this more in particular here : https://gitlab.khronos.org/vulkan/vulkan/-/issues/3846

And to me it looks like gamescope should try to avoid using the UNDEFINED layout.

I'm not sure how images are allocated & bound, but if an image is received from the application (glxgears), it should already be in GENERAL or PRESENT_SRC layout. It should never be put back into UNDEFINED as long as the content cannot be discarded.

Another alternative would be for us to implement VK_EXT_external_memory_acquire_unmodified and assume there is no need to reset the compressed data. But it looks like not using UNDEFINED all the time it a better solution tbh.

@sharkautarch
Copy link

sharkautarch commented Apr 16, 2024

Replying to #356 (comment)

Interesting
I will say that in my discussion in this now-closed PR request I made in the past about vulkan memory/execution barriers, @Joshua-Ashton said they wanted a rendergraph-based approach.
And I did find one rendergraph library -- Vuk -- that could potentially both:

  • automatically create the appropriate execution/memory barriers
  • should automatically track the layouts of the images, performing layout transitions where needed.

From what I can tell, it seems like Vuk has pretty good support for both renderpass stuff and compute work, and it doesn't force you to only present via graphics pipeline, because you can do an execute_submit to execute a rendergraph, retrieve the resultant swapchain image from that command, and manually present said swapchain image.
Though I can tell that there are some issues with trying to integrate Vuk into gamescope:

  • Gamescope uses a lot of timeline semaphores, whereas Vuk mostly uses binary semaphores
  • Not sure how importing/exporting memoryfds (which gamescope currently does) would work w/ Vuk (same goes for any possible usage of external fences/semaphores in the future)
  • More minor concern: Vuk has quite a number of dependencies. Tho some of them like Spriv-reflect are unavoidable (hard to automatically synchronize vulkan compute otherwise)

@sharkautarch
Copy link

UPDATE
@djdeath
It looks like there's ongoing work in the new-FE branch of Vuk, and it looks like Vuk may move towards using more timeline semaphores (don't quote me on the last part tho, I'm not entirely sure about it)...

@djdeath
Copy link

djdeath commented Apr 22, 2024

In my opinion this has nothing to do with semaphores.

@djdeath
Copy link

djdeath commented Apr 22, 2024

A change that would make sense to me would be :

diff --git a/src/rendervulkan.hpp b/src/rendervulkan.hpp
index 3aa9815..76b7229 100644
--- a/src/rendervulkan.hpp
+++ b/src/rendervulkan.hpp
@@ -1027,7 +1027,7 @@ public:
                                                | ( (isPresent && state.dirty) ? VK_ACCESS_SHADER_WRITE_BIT | VK_ACCESS_SHADER_READ_BIT : 0u),
 
                                .dstAccessMask = dst_read_bits | dst_write_bits,
-                               .oldLayout = ( (state.discarded || state.needsImport) ) ? VK_IMAGE_LAYOUT_UNDEFINED : VK_IMAGE_LAYOUT_GENERAL,
+                               .oldLayout = ( state.discarded ) ? VK_IMAGE_LAYOUT_UNDEFINED : VK_IMAGE_LAYOUT_GENERAL,
                                .newLayout =  isPresent ? GetBackend()->GetPresentLayout() : VK_IMAGE_LAYOUT_GENERAL,
                                .srcQueueFamilyIndex = isExport ? image->queueFamily : state.needsImport ? externalQueue : image->queueFamily,
                                .dstQueueFamilyIndex = isExport ? externalQueue : state.needsImport ? m_queueFamily : m_queueFamily,

But I'm not really sure this is correct. For Icelake it will depend on whether the image has storage usage.

@sharkautarch
Copy link

sharkautarch commented Apr 22, 2024

Replying to #356 (comment)

Is it well defined by the spec that imported images will be in the VK_IMAGE_LAYOUT_GENERAL layout?

@djdeath
Copy link

djdeath commented Apr 22, 2024

Replying to #356 (comment)

Is it well defined by the spec that imported images will be in the VK_IMAGE_LAYOUT_GENERAL layout?

Nope. It depends on the application you interact with I suppose.

SRC_PRESENT would maybe make more sense.

@sharkautarch
Copy link

sharkautarch commented Apr 22, 2024

Replying to #356 (comment)

Is it well defined by the spec that imported images will be in the VK_IMAGE_LAYOUT_GENERAL layout?

Nope. It depends on the application you interact with I suppose.

SRC_PRESENT would maybe make more sense.

ok, so uh, let’s say for example:

  • in the case where gamescope is running a vulkan game, and it’s using the gamescope WSI layer to grab frames from the game (instead of xwayland) (in this example, I’ll ignore the aspect of gamescope WSI’s canBypassXWayland function that is used to dynamically switch between using an internal XWayland or wayland vulkan surface)
  • So inside the gamescope WSI layer, the swapchain, surface, and instance creation functions are hooked so that the game’s wayland surface is just gamescope’s surface
  • Gamescope WSI layer hooks QueuePresent so that the swapchain usage pattern is:
  • Game prepares image -> game invokes QueuePresent() -> gamescope acquires said image -> gamescope invokes QueuePresent() and then said image is painted to screen

Honestly even for this specific situation, I’m not quite sure, but would imagine it would be in src_present layout???

For other situations, I think when gamescope WSI is not being used, gamescope might either use xwayland, or (especially w/ the new wayland backend for gamescope) use some wayland stuff for grabbing frames

@djdeath
Copy link

djdeath commented Apr 23, 2024

Replying to #356 (comment)

You might be able to add a layout value to communicate between the app and gamescope in that case. And you could use whatever layout you think is best. Just don't use UNDEFINED as for the driver that means reset some of the data.

@Joshua-Ashton
Copy link
Collaborator

Joshua-Ashton commented Apr 23, 2024

No. Undefined is fine as the src layout is ignored for foreign import.

If you wanted an undefined transition, you would just not do a queue transition.

@djdeath
Copy link

djdeath commented Apr 23, 2024

No. Undefined is fine as the src layout is ignored for foreign import.

Can you point to the spec bit that says this?

@djdeath
Copy link

djdeath commented Apr 23, 2024

I'm not against amending the spec, but afaict, this "ignore undefined layout when doing queue transitions" is not specified anywhere.

@Joshua-Ashton
Copy link
Collaborator

There was some discussion here https://gitlab.khronos.org/vulkan/vulkan/-/issues/3195

I agree it is underspecified but there is really no other option than ignoring oldLayout? Otherwise the entire ext is totally unusable.

This is what RADV and NV prop do and what Gamescope + other users would expect as its the only way to use the ext.

If you wanted an undefined transition, you wouldn't need or do a queue ownership transfer.

gnomesysadmins pushed a commit to GNOME/gtk that referenced this issue Apr 24, 2024
The VK_IMAGE_LAYOUT_UNDEFINED layout means that the data hold by the
texture can be discarded, and we don't want to discard it. Because the
Vulkan spec is unclear (see [1] for a discussion), err on the side of
caution and use VK_IMAGE_LAYOUT_GENERAL.

Fixes import failures with WebKit.

[1] ValveSoftware/gamescope#356
colinmarc added a commit to colinmarc/magic-mirror that referenced this issue Apr 25, 2024
This can apparently cause the driver to discard the dmabuf contents on some
systems. For more discussion, see the following gamescope thread:

ValveSoftware/gamescope#356
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests