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.

Sign up for GitHub

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

Jump to bottom

Blacklist Sandy Bridge on mesa from using geometry shaders. #3518

Merged
merged 1 commit into from Jan 20, 2016
Merged

Blacklist Sandy Bridge on mesa from using geometry shaders. #3518

merged 1 commit into from Jan 20, 2016

Conversation

Sonicadvance1
Copy link
Contributor

Review on Reviewable

BhaaLseN
BhaaLseN reviewed Jan 15, 2016
View reviewed changes
Source/Core/VideoCommon/DriverDetails.h
// Started Version: -1
// Ended Version: -1
// Mesa inroduced geometry shader support for radeon devices and failed to test it with us.
// Mesa inroduced geometry shader support for radeon and sandy bridge devices and failed to test it with us.

This comment was marked as off-topic.

Sign in to view

@CrossVR CrossVR added this to the Dolphin Release 5.0 milestone Jan 15, 2016
@delroth
Copy link
Member

delroth commented Jan 16, 2016

Why?

@Sonicadvance1
Copy link
Contributor Author

For the same reason that radeon is blacklisted.

delroth
delroth reviewed Jan 16, 2016
View reviewed changes
Source/Core/VideoBackends/OGL/Render.cpp
driver = DriverDetails::DRIVER_I965;
if (srenderer.find("Sandybridge") != std::string::npos)
family = 2;

This comment was marked as off-topic.

Sign in to view

@sheepdestroyer
Copy link

Is the bug reported at mesa?

@Sonicadvance1
Copy link
Contributor Author

The bug is reported somewhere to mesa for both the radeon and Intel drivers.

Comments from the review on Reviewable.io

@BhaaLseN
Copy link
Member

Review status: 0 of 3 files reviewed at latest revision, 1 unresolved discussion.

Source/Core/VideoCommon/DriverDetails.h, line 55 [r2] (raw file):
enum class

Comments from the review on Reviewable.io

@Sonicadvance1
Copy link
Contributor Author

Source/Core/VideoCommon/DriverDetails.h, line 55 [r2] (raw file):
Won't work, it's being cast to a u32. Which can't be easily changed over to use the class of DriverDetails::Family because the OS X and Windows paths rely on it to be a u32.

Comments from the review on Reviewable.io

@BhaaLseN
Copy link
Member

Review status: 0 of 3 files reviewed at latest revision, 1 unresolved discussion.

Source/Core/VideoCommon/DriverDetails.h, line 55 [r2] (raw file):
enum class also supports specifying a type if you want to: enum class Family : std::uint32_t (which requires cstdint to be included)

Comments from the review on Reviewable.io

@Sonicadvance1
Copy link
Contributor Author

Source/Core/VideoCommon/DriverDetails.h, line 55 [r2] (raw file):
That still doesn't fix the issue and it won't compile.

Comments from the review on Reviewable.io

@delroth
Copy link
Member

delroth commented Jan 20, 2016

Reviewed 3 of 3 files at r2.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

Source/Core/VideoCommon/DriverDetails.h, line 55 [r2] (raw file):
Suggestion: Add UNKNOWN = -1 to family, replace our uses of -1 in the bugs array by UNKNOWN, change the type to the array to Family. That way it can be enum class :)

Comments from the review on Reviewable.io

@Sonicadvance1
Copy link
Contributor Author

Source/Core/VideoCommon/DriverDetails.h, line 55 [r2] (raw file):
That won't fix the OS X and Windows path requiring it as a u32 and I don't know how to query the family on those OSes to make sure I choose the proper one so the DriverDetail bug is still active.

Comments from the review on Reviewable.io

@Helios747
Copy link
Contributor

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

Source/Core/VideoCommon/DriverDetails.h, line 55 [r2] (raw file):
I think @lioncash has an OS X system. Maybe he would know the solution here?

Comments from the review on Reviewable.io

@sheepdestroyer
Copy link

Should specific driver bug reports be referenced with these workarounds, so that progress be tracked and the blacklist lifted once solved?

@BhaaLseN
Copy link
Member

Reviewed 2 of 3 files at r2.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

Comments from the review on Reviewable.io

@delroth
Copy link
Member

delroth commented Jan 20, 2016

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

Source/Core/VideoCommon/DriverDetails.h, line 55 [r2] (raw file):
OK. I'm fine with making them not enum classes then. It makes me very sad though :(

Please fix the bug with it causing diffs on Broadwell though.

Comments from the review on Reviewable.io

@Sonicadvance1
Copy link
Contributor Author

Review status: 0 of 3 files reviewed at latest revision, 1 unresolved discussion.

Source/Core/VideoCommon/DriverDetails.h, line 55 [r2] (raw file):
Fixed

Comments from the review on Reviewable.io

@BhaaLseN
Copy link
Member

Reviewed 1 of 3 files at r2.
Review status: 0 of 3 files reviewed at latest revision, 1 unresolved discussion.

Source/Core/VideoCommon/DriverDetails.h, line 55 [r2] (raw file):
^ I can live with that too.

Comments from the review on Reviewable.io

@delroth
Copy link
Member

delroth commented Jan 20, 2016

Reviewed 1 of 3 files at r2, 3 of 3 files at r3.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

Comments from the review on Reviewable.io

@BhaaLseN
Copy link
Member

Reviewed 3 of 3 files at r3.
Review status: :shipit: all files reviewed at latest revision, all discussions resolved, all commit checks successful.

Comments from the review on Reviewable.io

1 similar comment
@Sonicadvance1
Copy link
Contributor Author

Reviewed 3 of 3 files at r3.
Review status: :shipit: all files reviewed at latest revision, all discussions resolved, all commit checks successful.

Comments from the review on Reviewable.io

delroth added a commit that referenced this pull request Jan 20, 2016
@delroth
Merge pull request #3518 from Sonicadvance1/blacklist_sandy
be1a9e4 
Blacklist Sandy Bridge on mesa from using geometry shaders.
@delroth delroth merged commit be1a9e4 into dolphin-emu:master Jan 20, 2016
@Sonicadvance1 Sonicadvance1 deleted the blacklist_sandy branch January 20, 2016 18:35
@Sonicadvance1
Copy link
Contributor Author

A lot of the times we track the bug independently and I keep a list of bugs on a google drive document.
Could definitely be handling the tracking better
https://docs.google.com/spreadsheets/d/1ZbuYefVawKlzqiCtu7IU1XiXmgqO3z826-wiD_nw6lY/edit#gid=0

Comments from the review on Reviewable.io

@sheepdestroyer
Copy link

Yep, your spreadsheet lists the bug's reference at freedesktop. However it doesn't link to this github entry. So there is no cross referencing going both ways.
I could possibly imagine a bug getting resolved in the driver but the corresponding fix in dolphin not being reverted.

@sheepdestroyer
Copy link

The original dolphin bug entry is also not listed in your spreadsheet or in this PR
https://bugs.dolphin-emu.org/issues/9166

@sheepdestroyer
Copy link

The patch fixing the original bug has been committed to mesa master : http://cgit.freedesktop.org/mesa/mesa/commit/?id=9f2e22bf343b21d6b44e6a502f00a86d169f5ade
It has also been CCed to 11.0 and 11.1 stable trees
The next major version (should be called 11.2) and stable releases (that would be 11.1.2 and 11.0.10) should include it.

@sheepdestroyer
Copy link

On Intel SNB HD 3000, built Mesa-master with included fix as well as dolphin-master with blacklist removed.
I then tried Metroid Prime and can confirm the visual bugs are not present even when looking at minimap

@sheepdestroyer
Copy link

Mesa 11.2 has been branched 4 days ago and includes the aforementioned fix : https://cgit.freedesktop.org/mesa/mesa/log/?h=11.2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Milestone
Dolphin Release 5.0
6 participants
@Sonicadvance1 @delroth @sheepdestroyer @BhaaLseN @Helios747 @CrossVR