Bug 108311

Summary:	Query buffer object support is broken on r600.
Product:	Mesa	Reporter:	Andrew Wesie <awesie>
Component:	Drivers/Gallium/r600	Assignee:	Default DRI bug account <dri-devel>
Status:	RESOLVED FIXED	QA Contact:	Default DRI bug account <dri-devel>
Severity:	normal
Priority:	medium	CC:	airlied, mesa-dev
Version:	git
Hardware:	x86-64 (AMD64)
OS:	Linux (All)
Whiteboard:
i915 platform:		i915 features:
Attachments:	Patch set larger alignment for tmp buffer offset

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.