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

Harvest Moon Graphic Issue on Adreno #10421

Closed
Lotv opened this issue Dec 19, 2017 · 33 comments
Closed

Harvest Moon Graphic Issue on Adreno #10421

Lotv opened this issue Dec 19, 2017 · 33 comments

Comments

@Lotv
Copy link

Lotv commented Dec 19, 2017

On Vulkan Renderer I habe graphical Issues (Black Pixel Boxes around the Clouds) with OpenGL everything is fine

Axon 7
Android 7.1.1
Adreno 530

@Lotv
Copy link
Author

Lotv commented Dec 19, 2017

screenshot_2017-12-18-23-22-52

@ppmeis
Copy link
Contributor

ppmeis commented Dec 20, 2017

Tested with latest build, Windows version default settings under Vulkan API, everything is fine:
image

@unknownbrackets
Copy link
Collaborator

Another device lying about dualSrcBlend support?

Maybe it'd be worth seeing if we can get the conformance tests improved...

-[Unknown]

@hrydgard
Copy link
Owner

That would surprise me...

http://vulkan.gpuinfo.org/listreports.php?feature=dualSrcBlend

No adrenos there.

@unknownbrackets
Copy link
Collaborator

Maybe a rounding issue with alpha testing, then? Like in Popolocrois with GL...

-[Unknown]

@hrydgard
Copy link
Owner

Or it's a depth issue..

@mrcmunir
Copy link
Contributor

mrcmunir commented Oct 6, 2018

screenshot_2018-10-06-18-38-03-336_org ppsspp ppsspp
screenshot_2018-10-06-18-49-26-278_org ppsspp ppsspp
screenshot_2018-10-06-18-37-58-168_org ppsspp ppsspp
screenshot_2018-10-06-18-49-55-582_org ppsspp ppsspp

I can confirm this only happen in vulkan backend in Xiaomi mi 8 (adreno 630) the driver are supported dualsrcblend but no helped sigh adreno specific bug

@unknownbrackets
Copy link
Collaborator

Could you try exporting a GE debugger dump on PC?

To do this, open the game and select Debug -> GE debugger..., then when it's displaying the scene, press Record in the top right. After a second or so, it'll finish and save a trace of the drawing activity.

After that, check the memstick/PSP/SYSTEM/DUMP folder and it'll have created a file named something like "ULES12345_0000.ppdmp". You can zip that and then drag and drop it into a reply here.

-[Unknown]

@mrcmunir
Copy link
Contributor

mrcmunir commented Oct 6, 2018

@unknownbrackets on PC however works well tested with an Intel GPU

Here the ppdmp files I made of several scenes on the mobile looks bad but on the PC it works fine.

DUMPULUS10458.zip

@hrydgard
Copy link
Owner

hrydgard commented Oct 6, 2018

Did an experimental commit disabling dual src blend on Adreno, so you'll get a build to try soon.

hrydgard added a commit that referenced this issue Oct 6, 2018
@hrydgard hrydgard added this to the v1.7.0 milestone Oct 6, 2018
@mrcmunir
Copy link
Contributor

mrcmunir commented Oct 6, 2018

@hrydgard Thanks I try and no detected correct blacklist srcblend always enabled.

@unknownbrackets
Copy link
Collaborator

I lost you a bit in that message (maybe autocorrect), but do you mean that even in the latest git build, you're still seeing the issue and dualSrcBlend is still marked enabled: 1?

-[Unknown]

@mrcmunir
Copy link
Contributor

mrcmunir commented Oct 6, 2018

@unknownbrackets Correct I said dualSrcBlend is still marked enabled:1 in the latest git build 1.6.3.548-geba6c00a8 👍

@unknownbrackets
Copy link
Collaborator

The change won't prevent it from being enabled, it just won't use it. Hmm, but if it still displays wrong maybe even enabling the feature causes problems with the driver...

-[Unknown]

@mrcmunir
Copy link
Contributor

mrcmunir commented Oct 7, 2018

@unknownbrackets Ah I see . I could see that the last change "solves" some crashes in some games where the game was previously closed, now you can go further with in (forgetting the graphic defects)

@hrydgard
Copy link
Owner

hrydgard commented Oct 7, 2018

Just for the sake of experiment, I committed a change where we don't enable the feature at all on Adreno.

@mrcmunir
Copy link
Contributor

mrcmunir commented Oct 7, 2018

6fd1c0e now detected correct DualsrcBlend:0 but not helped same glitch error.

So it's not related to DualsrcBlend issue?

Regards

@hrydgard
Copy link
Owner

hrydgard commented Oct 7, 2018

Seems not, fortunately or unfortunately, depending on point of view...

@unknownbrackets
Copy link
Collaborator

unknownbrackets commented Oct 7, 2018

I guess we should revert the dualSrcBlend change.

TL;DR: Driver bug involving depth testing.

This is actually happening on my Pixel (using dump 3.) I had to make a small hack because of an issue with us dumping render-to-texture.

The clouds are drawn using simple src.a + (1.0 - src.a), stencil test disabled, alpha test disabled. There is a depth test, src > dst. Depth write disabled. It's a pretty simple draw. It does reuse state from a previous draw.

Blend state ends up:

  • Color: SRC_ALPHA + ONE_MINUS_SRC_ALPHA
  • Alpha: ZERO + ONE

The texture is 128x128, and has 0/0/0/0 for transparent pixels. It's ultimately a 4-bit swizzled texture using a CLUT (8888.) Modulate, no doubling, pretty standard.

Things I've tried:

  • Disabling the early_fragment_tests/etc. change makes no impact.
  • My device DOES report dual source blending in Vulkan, in fact.
  • Disabling several other Vulkan features doesn't help.
  • Forcing alpha write (we mask in this scenario) doesn't help.
  • Forcing it to recalculate blending state doesn't help.
  • Forcing alpha blend to match color blend doesn't help.
  • Forcing the clouds to 100% 0/0/0/0 just causes black boxes.

But then... on a lark, I disabled depth testing. Great success, issue fixed. Clouds look fine. src > dst depth testing is just too hard for Adreno, it turns out.

  • Depth write force enabled doesn't help.
  • Depth test ALWAYS does help.
  • >= has the same problems as >.
  • != does not have problems.

Adreno trying to prove that our hopes, dreams, and wishes that Vulkan drivers couldn't be as bad/worse than GL drivers were silly...

-[Unknown]

@unknownbrackets unknownbrackets changed the title Harvest Moon Graphic Issue Harvest Moon Graphic Issue on Adreno Oct 7, 2018
@hrydgard
Copy link
Owner

hrydgard commented Oct 7, 2018

That's just... awful. And sad. darn.

@hrydgard
Copy link
Owner

Don't see any hope of working this around for 1.7.0 so bumping to 1.8.0, or maybe should close as driver bug...

@hrydgard hrydgard modified the milestones: v1.7.0, Future Oct 11, 2018
@unknownbrackets
Copy link
Collaborator

If you can create a new ppdmp with the latest version of PPSSPP, it would help. It will dump this correctly so people can reproduce without any workarounds (sorry, there was a bug in the version you initially created the dump with.)

It works for Android now too:
https://github.com/hrydgard/ppsspp/wiki/How-to-create-a-frame-dump

Anyway - as noted in #11620, it seems the full issue is that Adreno Vulkan drivers are simply unable to do depth testing properly (as outlined above) when not doing early fragment tests. Maybe something about the draw is causing it to skip early fragment tests (despite any shader hint)?

As noted above, this is apparently not affected by early_fragment_tests / force writing to depth to prevent the OpKill bug, and isn't using alpha or color testing. So #11620 is not likely to have helped.

-[Unknown]

@mrcmunir
Copy link
Contributor

@unknownbrackets Okay, see if that new ppdmp help you now : DUMPULUS10458

@unknownbrackets
Copy link
Collaborator

Turns out, this is ultimately caused by draw 96 from 0x088f1e60 specifically.

I think it's a driver bug with colorWriteMask @hrydgard. This works around it:

			if ((gstate.pmskc & 0x00FFFFFF) == 0x00FFFFFF) {
				key.colorWriteMask = VK_COLOR_COMPONENT_R_BIT | VK_COLOR_COMPONENT_G_BIT | VK_COLOR_COMPONENT_B_BIT | VK_COLOR_COMPONENT_A_BIT;
				key.blendEnable = true;
				key.blendOpColor = VK_BLEND_OP_ADD;
				key.blendOpAlpha = VK_BLEND_OP_ADD;
				key.srcColor = VK_BLEND_FACTOR_ZERO;
				key.srcAlpha = VK_BLEND_FACTOR_ZERO;
				key.destColor = VK_BLEND_FACTOR_ONE;
				key.destAlpha = VK_BLEND_FACTOR_ONE;
			}

It seems like when the depth test fails, it incorrectly ignores the color write mask. Not altogether different from the stencil driver bug... but I was mistaken on how depth testing was broken.

This draw is NOT the initial clouds draw, but a later one redrawing cloud depth (not sure why.) It is:

  • Depth test: src > dst.
  • IMPORTANT: rgb masked (FFFFFF.)
  • Depth test enabled, no other tests. Depth write enabled.
  • No blending, lighting, culling, or other complexity in flags.
  • Texture mapping disabled.
  • Verts without color: float tc, u8 norm, float pos.
  • Typical viewport/depth range.

The goal is presumably only to write depth. Stencil testing is off so stencil is not written either. It seems to draw the same positions as the clouds, but is a much later draw.

-[Unknown]

@hrydgard
Copy link
Owner

Wow, nasty. Thanks for investigating that. I guess I'll need to write another test...

@unknownbrackets
Copy link
Collaborator

Hm, proving difficult to create a test that replicates. Doing depth testing + color write mask alone isn't reproducing, but the draw is very simple...

-[Unknown]

@hrydgard
Copy link
Owner

Are you making sure that the depth test actually fails?

@unknownbrackets
Copy link
Collaborator

Yes, I turned on color write to verify that part was working. Tried making it the same depth, greater, and less to make sure it wasn't one of those states...

-[Unknown]

@unknownbrackets
Copy link
Collaborator

Also - confirmed not happening on Adreno 6xx / Android 8.0. Probably only 5xx...

-[Unknown]

@hrydgard
Copy link
Owner

Strange, wonder what other state could be involved - or, possibly, it needs to be a more complex shader, with something particular in it. Maybe has something to do with that early_test flag?

@unknownbrackets
Copy link
Collaborator

No, as noted above it happens regardless of that, even before the commit that added any of that.

-[Unknown]

unknownbrackets added a commit that referenced this issue Dec 23, 2018
…o-bug

WIP: Vulkan/adreno: Apply workaround for Harvest Moon issue #10421
@unknownbrackets
Copy link
Collaborator

I'm going to mark this as fixed since we applied the workaround in #11691. Hopefully the workaround prevents other similar bugs.

-[Unknown]

@unknownbrackets
Copy link
Collaborator

This driver bug still seems present on latest GPUs. I cut down a dump a lot to make a more minimal reproduction case, because it seems to require multiple draws to trigger it.

ULUS10458_#10421_harvest_moon_edit.zip

This is an edited dump. If the clouds look ugly, but you only see white and blue: no bug.
If the clouds have black boxes around them: bug.

It's not really minimal now, but I got it down to 5 draw commands and 15 KB of GE dump from n original 495 KB and 112 draw commands, so it's much reduced and still reproduces.

-[Unknown]

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

No branches or pull requests

5 participants