| Summary: | [SNB+ Bisected]Ogles3conform ES3-CTS.shaders.struct.uniform.sampler_array_fragment fails | ||
|---|---|---|---|
| Product: | Mesa | Reporter: | lu hua <huax.lu> |
| Component: | Drivers/DRI/i965 | Assignee: | Tapani Pälli <lemody> |
| Status: | VERIFIED FIXED | QA Contact: | Intel 3D Bugs Mailing List <intel-3d-bugs> |
| Severity: | major | ||
| Priority: | highest | CC: | james.ausmus, nroberts |
| Version: | git | ||
| Hardware: | All | ||
| OS: | Linux (All) | ||
| Whiteboard: | |||
| i915 platform: | i915 features: | ||
| Attachments: |
Test case for shader_runner
Test case for shader_runner workaround for the issue more complex shader_runner test fix for the issue |
||
|
Description
lu hua
2015-04-20 09:11:10 UTC
Created attachment 115410 [details]
Test case for shader_runner
It looks like nir_lower_samplers calculates the wrong sampler number when the sampler uniform is in an array. Here's a simpler example for shader_runner which demonstrates the problem.
Created attachment 115411 [details]
Test case for shader_runner
Oops, here's a better version of that test.
Created attachment 115527 [details] [review] workaround for the issue Here's a workaround. I believe the calculation code expects that we have array of samplers and does not understand the situation where we have arrays of structures with samplers in them. I haven't quite understood from the code why this is so, will continue investigation. Attached workaround fixes this issue. So, is this a bug, or a test issue? Tested it on BDW and SNB with the latest nightly kernel(2ead07) and Mesa(1ac7db07b), this problem has gone away. So Verified. (In reply to ye.tian from comment #5) > Tested it on BDW and SNB with the latest nightly kernel(2ead07) and > Mesa(1ac7db07b), this problem has gone away. > So Verified. I find this hard to believe, could you still verify again? I can still reproduce this on HSW with current Mesa head (b5045e2). (In reply to Tapani Pälli from comment #6) > (In reply to ye.tian from comment #5) > > Tested it on BDW and SNB with the latest nightly kernel(2ead07) and > > Mesa(1ac7db07b), this problem has gone away. > > So Verified. > > I find this hard to believe, could you still verify again? I can still > reproduce this on HSW with current Mesa head (b5045e2). It still fails on the latest mesa master branch, Fixed by comment 3's patch. Reopen it until the patch merged. (In reply to Gavin Hindman from comment #4) > So, is this a bug, or a test issue? This is a bug in Mesa, I've received some comments on the patch and will try to improve it, it won't work for all the cases as is. Created attachment 115599 [details]
more complex shader_runner test
This test has additional cases with array of samplers and array of structures containing array of samplers.
Created attachment 115601 [details] [review] fix for the issue here's another try for the fix, for me this passes all of piglit and the attached 'complex' case (In reply to Tapani Pälli from comment #10) > Created attachment 115601 [details] [review] [review] > fix for the issue > > here's another try for the fix, for me this passes all of piglit and the > attached 'complex' case I'm still not convinced that's correct, but it made me look through the code again and I think I know what's going wrong. On line 81 in the direct array access case we increment sampler_index by deref->base_offset *and* add "[%u]" to the string description with deref->base_offset. Then sampler_index is incremented by the return value of get_sampler_index which should take the array offset into account *again*. Maybe we just need to remove the "instr->sampler_index += deref->base_offset" line to get rid of the double-counting. (In reply to Tapani Pälli from comment #10) > Created attachment 115601 [details] [review] [review] > fix for the issue > > here's another try for the fix, for me this passes all of piglit and the > attached 'complex' case Fixed by this patch. (In reply to Jason Ekstrand from comment #11) > (In reply to Tapani Pälli from comment #10) > > Created attachment 115601 [details] [review] [review] [review] > > fix for the issue > > > > here's another try for the fix, for me this passes all of piglit and the > > attached 'complex' case > > I'm still not convinced that's correct, but it made me look through the code > again and I think I know what's going wrong. On line 81 in the direct array > access case we increment sampler_index by deref->base_offset *and* add > "[%u]" to the string description with deref->base_offset. Then > sampler_index is incremented by the return value of get_sampler_index which > should take the array offset into account *again*. Maybe we just need to > remove the "instr->sampler_index += deref->base_offset" line to get rid of > the double-counting. Ah right, that's what the first (workaround patch) causes and the second patch too but by accident. It looks to me that deref_array->base_offset is required *only* in the case of having array of samplers (), otherwise it should be not taken to account. I will test this and set a patch. sending another try to mesa-dev fix pushed to master Verified.fixed. (In reply to Tapani Pälli from comment #15) > fix pushed to master commit 95774ca258d216d42877f9a8da7e1bb4212a6500 Author: Tapani Pälli <tapani.palli@intel.com> Date: Mon May 11 14:50:19 2015 +0300 nir: fix sampler lowering pass for arrays (please include this from now on) |
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.