Bug 108311 - Query buffer object support is broken on r600.
Summary: Query buffer object support is broken on r600.
Status: RESOLVED FIXED
Alias: None
Product: Mesa
Classification: Unclassified
Component: Drivers/Gallium/r600 (show other bugs)
Version: git
Hardware: x86-64 (AMD64) Linux (All)
: medium normal
Assignee: Default DRI bug account
QA Contact: Default DRI bug account
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-10-10 06:46 UTC by Andrew Wesie
Modified: 2018-11-28 23:10 UTC (History)
2 users (show)

See Also:
i915 platform:
i915 features:


Attachments
Patch (2.32 KB, patch)
2018-10-10 06:46 UTC, Andrew Wesie
Details | Splinter Review
set larger alignment for tmp buffer offset (876 bytes, patch)
2018-10-11 03:45 UTC, Dave Airlie
Details | Splinter Review

Description Andrew Wesie 2018-10-10 06:46:20 UTC
Created attachment 141970 [details] [review]
Patch

After implementing support for query buffer objects in Wine, I got a bug report about a regression on older AMD hardware (https://bugs.winehq.org/show_bug.cgi?id=45946). The root cause was a bug in the query buffer object support for r600 in Mesa.

As mentioned in the commit message for 1c9ea24a19a28e87f6038281c516287f25ad88b5, the r600 shader cannot address non-256 aligned ssbos. While that commit fixed most buffer accesses, it failed to fix ssbo[1], e.g. tmp_buffer. This led to random failures of the query buffer shader depending on whether we got lucky (e.g. tmp_buffer_offset was a multiple of 256) or not.

The fix is easy. Mask tmp_buffer_offset and add another constant for when we access the tmp_buffer in the shader. Patch is attached.
Comment 1 Dave Airlie 2018-10-11 03:45:25 UTC
Created attachment 141989 [details] [review]
set larger alignment for tmp buffer offset

Does this patch work as an alternate?
Comment 2 Andrew Wesie 2018-10-11 03:55:46 UTC
(In reply to Dave Airlie from comment #1)
> Created attachment 141989 [details] [review] [review]
> set larger alignment for tmp buffer offset
> 
> Does this patch work as an alternate?

It looks like it should work but I'll test it with real hw.

Any reason you prefer this patch? It seems like it would use more heap space without any notable benefits (e.g. should it have better performance characteristics?).
Comment 3 Andrew Wesie 2018-10-11 04:33:32 UTC
(In reply to Andrew Wesie from comment #2)
> (In reply to Dave Airlie from comment #1)
> > Created attachment 141989 [details] [review] [review] [review]
> > set larger alignment for tmp buffer offset
> > 
> > Does this patch work as an alternate?
> 
> It looks like it should work but I'll test it with real hw.
> 

I confirmed the new patch fixes the bug with my test gpu (HD 5700 series).
Comment 4 Dave Airlie 2018-11-28 23:10:45 UTC
I pushed my patch for simplicity sakes, I don't think we'd notice the difference in perf or mem usage.

Thanks for pointing out the problem and the first patch!


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.