Bug 90114 - [SNB+ Bisected]Ogles3conform ES3-CTS.shaders.struct.uniform.sampler_array_fragment fails
Summary: [SNB+ Bisected]Ogles3conform ES3-CTS.shaders.struct.uniform.sampler_array_fra...
Status: VERIFIED FIXED
Alias: None
Product: Mesa
Classification: Unclassified
Component: Drivers/DRI/i965 (show other bugs)
Version: git
Hardware: All Linux (All)
: highest major
Assignee: Tapani Pälli
QA Contact: Intel 3D Bugs Mailing List
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-04-20 09:11 UTC by lu hua
Modified: 2015-06-01 18:48 UTC (History)
2 users (show)

See Also:
i915 platform:
i915 features:


Attachments
Test case for shader_runner (346 bytes, text/plain)
2015-04-28 17:40 UTC, Neil Roberts
Details
Test case for shader_runner (364 bytes, text/plain)
2015-04-28 17:43 UTC, Neil Roberts
Details
workaround for the issue (1.26 KB, patch)
2015-05-04 08:06 UTC, Tapani Pälli
Details | Splinter Review
more complex shader_runner test (613 bytes, text/plain)
2015-05-06 16:52 UTC, Tapani Pälli
Details
fix for the issue (1.18 KB, patch)
2015-05-06 17:37 UTC, Tapani Pälli
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description lu hua 2015-04-20 09:11:10 UTC
System Environment:
--------------------------
Platform: SNB
Libdrm:		(master)libdrm-2.4.60-34-g0d78b37b1cac304ce5e84d1207f0a43abd29c000
Mesa:		(master)b2e871bd484db229978cfe4b7efa12dfd79067a8
Xserver:	(master)xorg-server-1.17.0-76-gb1029716e41e252f149b82124a149da180607c96
Xf86_video_intel:(master)2.99.917-275-g7eaf593640d4479f850227252fd793bcb55be8d3
Libva:		(master)062a63932c0f1439aa587aa986bbcfb758ff38f2
Libva_intel_driver:(master)890f538f62707ec07a6accdb65bafcaffc941bb1
Kernel:   (drm-intel-nightly)d600654ab94b325f253e267422dcf60302120ea0

Bug detailed description:
-----------------------------
It fails on SNB+ platforms with mesa master branch, works well on 10.5 branch.
It is a separate bug from bug 90000.

Bisect shows: d47405eb707b9921f70454049677a9d504ee3fa6 is the first bad commit.
commit d47405eb707b9921f70454049677a9d504ee3fa6
Author:     Jason Ekstrand <jason.ekstrand@intel.com>
AuthorDate: Fri Apr 10 16:24:11 2015 -0700
Commit:     Jason Ekstrand <jason.ekstrand@intel.com>
CommitDate: Fri Apr 10 17:19:54 2015 -0700

    i965: Use NIR by default for fragment shaders

    GLSL IR vs. NIR shader-db results on i965:

output:
dEQP Core GL-CTS-2.0 (0x0052484b) starting..
  target implementation = 'X11'

Test case 'ES3-CTS.shaders.struct.uniform.sampler_array_fragment'..
Vertex compile time = 3.502000 ms
Fragment compile time = 0.393000 ms
Link time = 1.111000 ms
  Fail (Fail)

DONE!

Test run totals:
  Passed:        0/1 (0.00%)
  Failed:        1/1 (100.00%)
  Not supported: 0/1 (0.00%)
  Warnings:      0/1 (0.00%)


Reproduce steps:
---------------------------- 
1. xinit
2. ./glcts --deqp-case=ES3-CTS.shaders.struct.uniform.sampler_array_fragment
Comment 1 Neil Roberts 2015-04-28 17:40:04 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.
Comment 2 Neil Roberts 2015-04-28 17:43:29 UTC
Created attachment 115411 [details]
Test case for shader_runner

Oops, here's a better version of that test.
Comment 3 Tapani Pälli 2015-05-04 08:06:19 UTC
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.
Comment 4 Gavin Hindman 2015-05-04 16:56:47 UTC
So, is this a bug, or a test issue?
Comment 5 ye.tian 2015-05-05 01:32:38 UTC
Tested it on BDW and SNB with the latest nightly kernel(2ead07) and Mesa(1ac7db07b), this problem has gone away.
So Verified.
Comment 6 Tapani Pälli 2015-05-05 05:18:28 UTC
(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).
Comment 7 lu hua 2015-05-05 05:46:32 UTC
(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.
Comment 8 Tapani Pälli 2015-05-06 15:38:42 UTC
(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.
Comment 9 Tapani Pälli 2015-05-06 16:52:49 UTC
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.
Comment 10 Tapani Pälli 2015-05-06 17:37:06 UTC
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
Comment 11 Jason Ekstrand 2015-05-06 17:49:39 UTC
(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.
Comment 12 lu hua 2015-05-08 05:21:21 UTC
(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.
Comment 13 Tapani Pälli 2015-05-08 06:15:42 UTC
(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.
Comment 14 Tapani Pälli 2015-05-11 11:50:15 UTC
sending another try to mesa-dev
Comment 15 Tapani Pälli 2015-05-12 11:32:56 UTC
fix pushed to master
Comment 16 lu hua 2015-05-13 08:08:11 UTC
Verified.fixed.
Comment 17 Matt Turner 2015-06-01 18:48:41 UTC
(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.