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)


bug/show.html.tmpl processed on Mar 25, 2017 at 07:37:39.
(provided by the Example extension).