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

Vulkan: Cleanup and implement XFB support #4374

Merged
merged 10 commits into from Nov 4, 2016

Conversation

stenzek
Copy link
Contributor

@stenzek stenzek commented Oct 22, 2016

The cleanups here are mostly just removing references to some of the singletons, and making the texture cache entry class re-usable for the XFB implementation (but also generally useful).

The XFB part is a single commit, so it can easily be reverted after texturecache-based-XFB is finished. I don't think an additional ~400 lines isn't too bad, for bringing Vulkan to feature parity with the other backends. RealXFB also seems to be significantly faster than GL for triangle.dol, probably helped by the fact that it's not doing byte order swapping when reading back.

Could anyone with realxfb-required games please confirm there's no obvious visual errors?


This change is Reviewable

@RisingFog
Copy link
Member

THPS3 seems to play fine, don't notice any issues with it,

@JMC47
Copy link
Contributor

JMC47 commented Oct 23, 2016

I can try some RealXFB limited games sometime and see; I haven't run into it in a while.

@lioncash
Copy link
Member

Review status: 0 of 21 files reviewed at latest revision, 7 unresolved discussions.


Source/Core/VideoBackends/Vulkan/FramebufferManager.cpp, line 1380 at r1 (raw file):

  config.rendertarget = true;
  TextureCacheBase::TCacheEntryBase* base_texture = g_texture_cache->CreateTexture(config);
  TextureCache::TCacheEntry* texture = static_cast<TextureCache::TCacheEntry*>(base_texture);

this and base_texture can just be auto*.


Source/Core/VideoBackends/Vulkan/FramebufferManager.h, line 172 at r1 (raw file):

// The XFB source class simply wraps a texture cache entry.
// All the required functionality is provided by TextureCache.
class XFBSource : public XFBSourceBase

class XFBSource final if it's not intended to be inherited from.


Source/Core/VideoBackends/Vulkan/FramebufferManager.h, line 175 at r1 (raw file):

{
public:
  XFBSource(std::unique_ptr<TextureCache::TCacheEntry> texture);

explicit XFBSource


Source/Core/VideoBackends/Vulkan/Renderer.cpp, line 558 at r1 (raw file):

  // No-XFB case.
  if (!g_ActiveConfig.bUseXFB)

The No-XFB and XFB-enabled cases should likely be different functions that are called through this function.


Source/Core/VideoBackends/Vulkan/StateTracker.h, line 195 at r1 (raw file):

};

extern std::unique_ptr<StateTracker> g_state_tracker;

Note that this makes it more annoying to get rid of global state that plagues the video code (which in particular, is something I'm working on).


Source/Core/VideoBackends/Vulkan/TextureCache.cpp, line 138 at r1 (raw file):

                      static_cast<u32>(scaled_src_rect.GetHeight())}};
  Texture2D* src_texture;
  if (src_format == PEControl::Z24)

This should likely be its own function.


Source/Core/VideoBackends/Vulkan/TextureCache.cpp, line 170 at r1 (raw file):

  // Fast path when not scaling the image.
  if (src_rect.GetWidth() == dst_rect.GetWidth() && src_rect.GetHeight() == dst_rect.GetHeight())

This fast path should likely be its own function, which is then called through this function.


Comments from Reviewable

@stenzek stenzek force-pushed the vulkan-xfb branch 3 times, most recently from 7b1b9fb to 5a4780a Compare October 23, 2016 12:50
@JMC47
Copy link
Contributor

JMC47 commented Oct 28, 2016

I didn't notice any issues in some quick tests yesterday. I couldn't find any RealXFB limited games though.

@Tinob
Copy link
Contributor

Tinob commented Oct 28, 2016

tested on nvidia gtx 750 ,amd r9 290, amd rx 480 and get only a black screen when using real xfb, with virtual xfb or no xfb it works perfectly.

@JMC47
Copy link
Contributor

JMC47 commented Oct 29, 2016

Can anyone else reproduce RealXFB not working on Vulkan? I can't reproduce it.

@ichttt
Copy link

ichttt commented Oct 30, 2016

RealXFB isn't working for me, too, I'm only getting black screen. Using AMD r9 390 with latest driver(16.10.3).

@stenzek
Copy link
Contributor Author

stenzek commented Oct 30, 2016

Seems to be working fine for me on my AMD box (16.9.2 driver).

@ichttt are you testing with this PR? If so, can you try enabling the validation layers (under Advanced), checking Host GPU and Video Backend in the log, and seeing it reports anything useful.

@ichttt
Copy link

ichttt commented Oct 30, 2016

Nevermind, the error was on my end. I forgot to restart Windows after installing the latest driver. It is working perfectly now. Sorry

@stenzek stenzek force-pushed the vulkan-xfb branch 2 times, most recently from 24ca3ab to 05ecbce Compare October 31, 2016 14:13
@degasus
Copy link
Member

degasus commented Nov 2, 2016

:lgtm:


Reviewed 7 of 21 files at r1, 12 of 14 files at r2, 1 of 2 files at r3, 3 of 3 files at r4.
Review status: all files reviewed at latest revision, 1 unresolved discussion.


Source/Core/VideoBackends/Vulkan/Renderer.cpp, line 494 at r4 (raw file):

  // Draw to the screenshot buffer if needed.
  if (IsFrameDumping() &&

Was it on purpose to dump the screenshot first, and render later on?


Comments from Reviewable

@stenzek
Copy link
Contributor Author

stenzek commented Nov 2, 2016

Review status: all files reviewed at latest revision, 1 unresolved discussion.


Source/Core/VideoBackends/Vulkan/Renderer.cpp, line 494 at r4 (raw file):

Previously, degasus (Markus Wick) wrote…

Was it on purpose to dump the screenshot first, and render later on?

Not really on purpose. There isn't any technical reason we couldn't submit the screen blit+present and the readback all in one command buffer though, sounds like a decent small optimization for when framedumping is on.

Comments from Reviewable

There's not a lot of point in passing these around or storing them
(texture cache/state tracker mainly) as there will only ever be a single
instance of the class.

Also adds downcast helpers such as Vulkan::Renderer::GetInstance().
This was causing issues when the stereo mode was changed at runtime.
Small optimization that should make things slightly more efficient when
frame dumping is enabled.
Name makes more sense given the methods it calls in the base class.
@degasus
Copy link
Member

degasus commented Nov 4, 2016

Looks very good


Reviewed 2 of 2 files at r6.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@stenzek stenzek merged commit ac2971b into dolphin-emu:master Nov 4, 2016
@psennermann
Copy link

psennermann commented Nov 4, 2016

I tried Batman Vengeance (wich requires Real XFB for movies), but with the Vulkan backend it doesn't work, it only shows a black screen.
Windows 10, Nvidia Gtx950, Intel e8600

@JMC47
Copy link
Contributor

JMC47 commented Nov 5, 2016

Works fine here, Vulkan backend, GTX 1070.

@JMC47
Copy link
Contributor

JMC47 commented Nov 5, 2016

It actually works better than OpenGL; if I turn on realXFB during the videos they start displaying immediately instead of being a black screen until gameplay starts.

@psennermann
Copy link

psennermann commented Nov 5, 2016

Also notice that if I enable Real XFB with the Vulkan backend, every game shows a black screen (but in the background the game is running, because you can hear the audio playing)
Gtx950 Driver 375.70 - Dolphin 5.0-1243

@psennermann
Copy link

I have the same problem of Tinob, cause virtual XFB works fine...

@stenzek
Copy link
Contributor Author

stenzek commented Nov 5, 2016

Given myself and @JMC47 aren't experiencing this issue I'm going to assume it's not widespread and perhaps specific to some particular hardware or config? @psennermann please open a bug on the issue tracker, and attach your config files if possible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
8 participants