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

Work around an apparent driver bug that caused wrong corner radii. #1810

Merged
merged 1 commit into from Oct 5, 2017

Conversation

@mstange
Copy link
Contributor

mstange commented Oct 5, 2017

Fixes #1809.

Using the driver of the Intel GPU in my Macbook Pro (Intel HD Graphics 530
1536 MB, 0x191b, Skylake GT2) on macOS 10.12.6, the integer calculation that was
used in order to compute the lookup address gave wrong results, and the corner
radius for the wrong corner was looked up.
The only way to get correct results from it was to move the entire adjustment of
the address ivec2 variable into the caller. Now all values in the calculations
were integer literals.
I then replaced the multiplications with gradual "+= 2" adjustments of the
address variable in fetch_clip, because that looked better and might even be
easier to reason about than the original code.


This change is Reviewable

@mstange
Copy link
Contributor Author

mstange commented Oct 5, 2017

r? @glennw or @kvark

@glennw
Copy link
Member

glennw commented Oct 5, 2017

Fun! Could you add a comment in that shader code just outlining roughly what's in the commit message and then r=me. Thanks for digging into it!

@nical
Copy link
Collaborator

nical commented Oct 5, 2017

I added this to the driver issues page. Unfortunately my descriptions of the problem and resolution are super vague, if you have any idea how to better explain this for future reference don't hesitate to fill in the blanks.

@kvark
Copy link
Member

kvark commented Oct 5, 2017

omg, this is an aweful bug
thanks @mstange for exposing and fixing it!

@mstange
Copy link
Contributor Author

mstange commented Oct 5, 2017

Here's an alternative fix that also works:

diff --git a/webrender/res/cs_clip_rectangle.glsl b/webrender/res/cs_clip_rectangle.glsl
--- a/webrender/res/cs_clip_rectangle.glsl
+++ b/webrender/res/cs_clip_rectangle.glsl
@@ -27,8 +27,8 @@ struct ClipCorner {
     vec4 outer_inner_radius;
 };

-ClipCorner fetch_clip_corner(ivec2 address, int index) {
-    address += ivec2(2 + 2 * index, 0);
+ClipCorner fetch_clip_corner(ivec2 address, float index) {
+    address += ivec2(2 + 2 * int(index), 0);
     vec4 data[2] = fetch_from_resource_cache_2_direct(address);
     return ClipCorner(RectWithSize(data[0].xy, data[0].zw), data[1]);
 }

It looks like what's broken is: reading from integer variables (or integer arguments) when doing any kind of integer calculation, in the right circumstances. If I store the number in a float, everything works. If I do the calculation without making use of an integer variable as part of the inputs of that calculation, things work as well.

I'll stick with the original fix though, and I'll add a comment.

@leeoniya
Copy link

leeoniya commented Oct 5, 2017

those pesky integers are at it again? #1728 (comment)

Using the driver of the Intel GPU in my Macbook Pro (Intel HD Graphics 530
1536 MB, 0x191b, Skylake GT2) on macOS 10.12.6, the integer calculation that was
used in order to compute the lookup address gave wrong results, and the corner
radius for the wrong corner was looked up.
The only way to get correct results from it was to move the entire adjustment of
the address ivec2 variable into the caller. Now all values in the calculations
were integer literals.
I then replaced the multiplications with gradual "+= 2" adjustments of the
address variable in fetch_clip, because that looked better and might even be
easier to reason about than the original code.
@mstange mstange force-pushed the mstange:clip-corner branch from 1386014 to d8f31f4 Oct 5, 2017
@mstange
Copy link
Contributor Author

mstange commented Oct 5, 2017

They sure are. And there's still at least one more instance of this problem left to find (#1738).

@kvark
Copy link
Member

kvark commented Oct 5, 2017

@bors-servo
Copy link
Contributor

bors-servo commented Oct 5, 2017

📌 Commit d8f31f4 has been approved by kvark

@bors-servo
Copy link
Contributor

bors-servo commented Oct 5, 2017

Testing commit d8f31f4 with merge eb30eb7...

bors-servo added a commit that referenced this pull request Oct 5, 2017
Work around an apparent driver bug that caused wrong corner radii.

Fixes #1809.

Using the driver of the Intel GPU in my Macbook Pro (Intel HD Graphics 530
1536 MB, 0x191b, Skylake GT2) on macOS 10.12.6, the integer calculation that was
used in order to compute the lookup address gave wrong results, and the corner
radius for the wrong corner was looked up.
The only way to get correct results from it was to move the entire adjustment of
the address ivec2 variable into the caller. Now all values in the calculations
were integer literals.
I then replaced the multiplications with gradual "+= 2" adjustments of the
address variable in fetch_clip, because that looked better and might even be
easier to reason about than the original code.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/1810)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Oct 5, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: kvark
Pushing eb30eb7 to master...

@bors-servo bors-servo merged commit d8f31f4 into servo:master Oct 5, 2017
4 checks passed
4 checks passed
Taskcluster (pull_request) TaskGroup: success
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
bors-servo added a commit that referenced this pull request Oct 24, 2017
Use a different workaround that fixes both #1809 and #1738.

This reverts #1810 and applies the alternative workaround that I gave in #1810 (comment) , because this alternative workaround also happens to fix #1738.

I was hesitant to create this PR because I haven't debugged #1738 enough to understand what's going wrong. But there is probably no good reason to delay fixing this until somebody does the investigation.

r? @kvark

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/1915)
<!-- Reviewable:end -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.