Re: [Mesa-dev] [PATCH 2/3] glsl: Fix incorrect hard-coded location of the gl_SecondaryFragColorEXT built-in.

2016-08-25 Thread Ilia Mirkin 
On Thu, Aug 25, 2016 at 12:57 AM, Francisco Jerez  wrote:
> Ilia Mirkin  writes:
>
>> On Thu, Aug 25, 2016 at 12:45 AM, Francisco Jerez  
>> wrote:
>>> Ilia Mirkin  writes:
>>>
 On Wed, Aug 24, 2016 at 4:30 PM, Francisco Jerez  
 wrote:
> Ilia Mirkin  writes:
>
>> I had trouble getting these to apply, perhaps they were meant to go on
>> top of something else. Anyways, should be fairly easy for you to test
>> out with llvmpipe.
>>
> It should apply cleanly on latest master now.

 Indeed it does. Running these 3 patches:

 glsl: Calculate bitset of secondary outputs written in 
 ir_set_program_inouts.
 st/glsl_to_tgsi: Translate fragment shader secondary outputs to TGSI 
 outputs.
 glsl: Fix incorrect hard-coded location of the
 gl_SecondaryFragColorEXT built-in.

>>>
>>> So you didn't have PATCH 1 applied to your tree?  I'd expect this change
>>> to behave badly without it and possibly end up creating too many
>>> outputs.  I guess I'll have to squash this change into the others...
>>
>> Isn't patch 1 the last patch in that list? (I applied them in the
>> wrong order, but I don't think it matters)
>
> Nope, I was referring to "[PATCH 1/3] glsl: Fix
> gl_program::OutputsWritten computation for dual-source blending.".

Ah yeah, I couldn't apply that one, I think due to the order of things
I did in. With all of those applied, the piglit in question passes
now, along with the other arb_blend_func_extended piglits. I haven't
actually looked at the contents of the patch.

  -ilia
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 2/3] glsl: Fix incorrect hard-coded location of the gl_SecondaryFragColorEXT built-in.

2016-08-24 Thread Francisco Jerez 
Ilia Mirkin  writes:

> On Thu, Aug 25, 2016 at 12:45 AM, Francisco Jerez  
> wrote:
>> Ilia Mirkin  writes:
>>
>>> On Wed, Aug 24, 2016 at 4:30 PM, Francisco Jerez  
>>> wrote:
 Ilia Mirkin  writes:

> I had trouble getting these to apply, perhaps they were meant to go on
> top of something else. Anyways, should be fairly easy for you to test
> out with llvmpipe.
>
 It should apply cleanly on latest master now.
>>>
>>> Indeed it does. Running these 3 patches:
>>>
>>> glsl: Calculate bitset of secondary outputs written in 
>>> ir_set_program_inouts.
>>> st/glsl_to_tgsi: Translate fragment shader secondary outputs to TGSI 
>>> outputs.
>>> glsl: Fix incorrect hard-coded location of the
>>> gl_SecondaryFragColorEXT built-in.
>>>
>>
>> So you didn't have PATCH 1 applied to your tree?  I'd expect this change
>> to behave badly without it and possibly end up creating too many
>> outputs.  I guess I'll have to squash this change into the others...
>
> Isn't patch 1 the last patch in that list? (I applied them in the
> wrong order, but I don't think it matters)

Nope, I was referring to "[PATCH 1/3] glsl: Fix
gl_program::OutputsWritten computation for dual-source blending.".


signature.asc
Description: PGP signature
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 2/3] glsl: Fix incorrect hard-coded location of the gl_SecondaryFragColorEXT built-in.

2016-08-24 Thread Ilia Mirkin 
On Thu, Aug 25, 2016 at 12:45 AM, Francisco Jerez  wrote:
> Ilia Mirkin  writes:
>
>> On Wed, Aug 24, 2016 at 4:30 PM, Francisco Jerez  
>> wrote:
>>> Ilia Mirkin  writes:
>>>
 I had trouble getting these to apply, perhaps they were meant to go on
 top of something else. Anyways, should be fairly easy for you to test
 out with llvmpipe.

>>> It should apply cleanly on latest master now.
>>
>> Indeed it does. Running these 3 patches:
>>
>> glsl: Calculate bitset of secondary outputs written in ir_set_program_inouts.
>> st/glsl_to_tgsi: Translate fragment shader secondary outputs to TGSI outputs.
>> glsl: Fix incorrect hard-coded location of the
>> gl_SecondaryFragColorEXT built-in.
>>
>
> So you didn't have PATCH 1 applied to your tree?  I'd expect this change
> to behave badly without it and possibly end up creating too many
> outputs.  I guess I'll have to squash this change into the others...

Isn't patch 1 the last patch in that list? (I applied them in the
wrong order, but I don't think it matters)
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 2/3] glsl: Fix incorrect hard-coded location of the gl_SecondaryFragColorEXT built-in.

2016-08-24 Thread Francisco Jerez 
Ilia Mirkin  writes:

> On Wed, Aug 24, 2016 at 4:30 PM, Francisco Jerez  
> wrote:
>> Ilia Mirkin  writes:
>>
>>> I had trouble getting these to apply, perhaps they were meant to go on
>>> top of something else. Anyways, should be fairly easy for you to test
>>> out with llvmpipe.
>>>
>> It should apply cleanly on latest master now.
>
> Indeed it does. Running these 3 patches:
>
> glsl: Calculate bitset of secondary outputs written in ir_set_program_inouts.
> st/glsl_to_tgsi: Translate fragment shader secondary outputs to TGSI outputs.
> glsl: Fix incorrect hard-coded location of the
> gl_SecondaryFragColorEXT built-in.
>

So you didn't have PATCH 1 applied to your tree?  I'd expect this change
to behave badly without it and possibly end up creating too many
outputs.  I guess I'll have to squash this change into the others...

> I get:
>
> state_tracker/st_program.c:824:st_translate_fragment_program:
> Assertion `0' failed.
> (gdb) l
> 819 switch (loc) {
> 820 case FRAG_RESULT_DEPTH:
> 821 case FRAG_RESULT_STENCIL:
> 822 case FRAG_RESULT_SAMPLE_MASK:
> 823/* handled above */
> 824assert(0);
> 825break;
> 826 case FRAG_RESULT_COLOR:
> 827write_all = GL_TRUE; /* fallthrough */
> 828 default:
> (gdb) i locals
> written = 8
> loc = 3
> numColors = 1
> outputsWritten = 4
> outputMapping = {7080032, 0, 1, 0, 14, 0, 7082880, 14, 4294951632, 32767,
>   4156877824, 3574963175, 4294951316, 32767, 7015088, 0, 4294951504, 32767,
>   4030068507, 32767, 3, 14, 0, 15}
> inputMapping = {0, 4294967295 }
> inputSlotToAttr = {0, 4294967295 }
> interpMode = {1, 0 }
> interpLocation = {0 , 4120177808, 32767, 0, 0, 0, 0,
>   7080656, 0, 27, 0, 0, 0, 1744, 0, 1744, 0, 4120177728, 32767, 37, 32767,
>   576, 0, 4030368463, 32767, 7074928, 0, 7078416, 0, 4294952288, 32767, 2,
>   32767, 16, 0, 7078416, 0, 0, 2090733823, 7062160, 0, 0, 0, 7078416, 3, 3,
>   2090733823, 6845752, 0, 4294952320, 2, 4294952320, 32767, 4029319846, 32767,
>   4040141664, 32767}
> attr = 15
> inputsRead = 1
> ureg = 0x4e
> write_all = 1 '\001'
>
> I don't really have the time to investigate this further ATM. However
> you should be able to reproduce yourself with llvmpipe.
>
Heh, I have a hunch what's going on, I'll take a look and resend as a
proper patch -- if the general approach seems reasonable to you?

>   -ilia


signature.asc
Description: PGP signature
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 2/3] glsl: Fix incorrect hard-coded location of the gl_SecondaryFragColorEXT built-in.

2016-08-24 Thread Ilia Mirkin 
On Wed, Aug 24, 2016 at 4:30 PM, Francisco Jerez  wrote:
> Ilia Mirkin  writes:
>
>> I had trouble getting these to apply, perhaps they were meant to go on
>> top of something else. Anyways, should be fairly easy for you to test
>> out with llvmpipe.
>>
> It should apply cleanly on latest master now.

Indeed it does. Running these 3 patches:

glsl: Calculate bitset of secondary outputs written in ir_set_program_inouts.
st/glsl_to_tgsi: Translate fragment shader secondary outputs to TGSI outputs.
glsl: Fix incorrect hard-coded location of the
gl_SecondaryFragColorEXT built-in.

I get:

state_tracker/st_program.c:824:st_translate_fragment_program:
Assertion `0' failed.
(gdb) l
819 switch (loc) {
820 case FRAG_RESULT_DEPTH:
821 case FRAG_RESULT_STENCIL:
822 case FRAG_RESULT_SAMPLE_MASK:
823/* handled above */
824assert(0);
825break;
826 case FRAG_RESULT_COLOR:
827write_all = GL_TRUE; /* fallthrough */
828 default:
(gdb) i locals
written = 8
loc = 3
numColors = 1
outputsWritten = 4
outputMapping = {7080032, 0, 1, 0, 14, 0, 7082880, 14, 4294951632, 32767,
  4156877824, 3574963175, 4294951316, 32767, 7015088, 0, 4294951504, 32767,
  4030068507, 32767, 3, 14, 0, 15}
inputMapping = {0, 4294967295 }
inputSlotToAttr = {0, 4294967295 }
interpMode = {1, 0 }
interpLocation = {0 , 4120177808, 32767, 0, 0, 0, 0,
  7080656, 0, 27, 0, 0, 0, 1744, 0, 1744, 0, 4120177728, 32767, 37, 32767,
  576, 0, 4030368463, 32767, 7074928, 0, 7078416, 0, 4294952288, 32767, 2,
  32767, 16, 0, 7078416, 0, 0, 2090733823, 7062160, 0, 0, 0, 7078416, 3, 3,
  2090733823, 6845752, 0, 4294952320, 2, 4294952320, 32767, 4029319846, 32767,
  4040141664, 32767}
attr = 15
inputsRead = 1
ureg = 0x4e
write_all = 1 '\001'

I don't really have the time to investigate this further ATM. However
you should be able to reproduce yourself with llvmpipe.

  -ilia
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 2/3] glsl: Fix incorrect hard-coded location of the gl_SecondaryFragColorEXT built-in.

2016-08-24 Thread Francisco Jerez 
Ilia Mirkin  writes:

> I had trouble getting these to apply, perhaps they were meant to go on
> top of something else. Anyways, should be fairly easy for you to test
> out with llvmpipe.
>
It should apply cleanly on latest master now.

>[...]


signature.asc
Description: PGP signature
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 2/3] glsl: Fix incorrect hard-coded location of the gl_SecondaryFragColorEXT built-in.

2016-08-24 Thread Ilia Mirkin 
I had trouble getting these to apply, perhaps they were meant to go on
top of something else. Anyways, should be fairly easy for you to test
out with llvmpipe.

On Tue, Aug 23, 2016 at 2:31 PM, Francisco Jerez  wrote:
> Francisco Jerez  writes:
>
>> Ilia Mirkin  writes:
>>
>>> On Tue, Aug 23, 2016 at 12:18 AM, Francisco Jerez  
>>> wrote:
 Ilia Mirkin  writes:

> On Tue, Aug 23, 2016 at 12:05 AM, Francisco Jerez  
> wrote:
>> Ilia Mirkin  writes:
>>
>>> On Mon, Aug 22, 2016 at 10:55 PM, Francisco Jerez 
>>>  wrote:
 Ilia Mirkin  writes:

> On Mon, Aug 22, 2016 at 9:59 PM, Francisco Jerez 
>  wrote:
>> gl_SecondaryFragColorEXT should have the same location as 
>> gl_FragColor
>> for the secondary fragment color to be replicated to all fragment
>> outputs.  The incorrect location of gl_SecondaryFragColorEXT would
>> cause the linker to mark both FRAG_RESULT_COLOR and FRAG_RESULT_DATA0
>> as being written to, which isn't allowed by the spec and would
>> ultimately lead to an assertion failure in
>> fs_visitor::emit_fb_writes() on my i965-fb-fetch branch.
>
> My recollection was that it didn't work with COLOR for "stupid"
> reasons. Can you confirm that
> bin/arb_blend_func_extended-fbo-extended-blend-pattern_gles2 -auto
> passes with this patch?
>
 Yes, it does, in fact
 arb_blend_func_extended-fbo-extended-blend-pattern_gles2 hits the i965
 assertion failure I mentioned above unless this patch is applied.
>>>
>>> This causes the test in question to fail on nouveau... the TGSI shader
>>> generated starts with
>>>
>>> FRAG
>>> PROPERTY FS_COLOR0_WRITES_ALL_CBUFS 1
>>> DCL IN[0], POSITION, LINEAR
>>> DCL OUT[0], SAMPLEMASK
>>
>> Heh, this smells a lot like the bug fixed in PATCH 1, but somewhere in
>> the mesa state tracker.  st_glsl_to_tgsi.cpp:2422 does:
>>
>> | entry = new(mem_ctx) variable_storage(var,
>> |   PROGRAM_OUTPUT,
>> |   var->data.location
>> |   + var->data.index);
>>
>> which is obviously bogus, e.g. for var->data.location ==
>> FRAG_RESULT_COLOR and var->data.index == 1 you get
>> FRAG_RESULT_SAMPLE_MASK which explains the sample mask declaration
>> above.
>
> Right, because having FRAG_RESULT_COLOR and index != 0 was never
> possible prior to this. That might be why Ryan stuck it into
> FRAG_RESULT_DATA0 [I may have been the one to suggest that].

 Heh, so I guess that's the "stupid" reason you were referring to,
 working around this mesa state tracker bug in the GLSL front-end.
>>>
>>> Right. Or another way of looking at it, FRAG_RESULT_COLOR + index != 0
>>> is illegal,
>>
>> That would be an acceptable limitation if it were taken into account
>> consistently (which would imply among other things binding gl_FragColor
>> to FRAG_RESULT_DATA0 if gl_SecondaryFragColor is written).  Otherwise
>> you will be telling the linker and the back-end that both
>> FRAG_RESULT_COLOR and FRAG_RESULT_DATA0 are being written, which is
>> illegal.  The i965 has been misbehaving since forever because of this,
>> but we just didn't notice until I added that assertion because the only
>> symptom is increased shader recompiles.
>>
>>> (as it would, among other things, imply multi-rt support for
>>> dual-source blending)
>>
>> FRAG_RESULT_COLOR + index != 0 doesn't imply multi-rt support, it's a
>> requirement for multi-rt support, which the GLSL spec makes room for and
>> some drivers in this tree could potentially support if the GLSL IR
>> representation of dual-source blending wasn't deliberately inconsistent.
>>
>>> and the former code was making the GLSL fe not emit the illegal
>>> combination.
>>>
>> I started digging into this and found out that that's not the only way
>> the GLSL-to-TGSI pass relies on a GLSL bug: The output semantic array
>> calculation in st_translate_fragment_program() relies on there being
>> multiple locations marked as written in the shader's OutputsWritten set
>> in the dual-source blending case (IOW you're relying on the bug fixed by
>> the previous patch too).  GLSL is not TGSI, in GLSL the secondary and
>> primary colors of a dual-source-blended output have the same location,
>> so you cannot expect there will be multiple elements present in a bitset
>> of locations.
>>
>>> Whichever way you look at it, breaking st/mesa isn't a great option.
>>
>> Then you may want to take a look at the attached patches in addition.
>> They are untested and I

Re: [Mesa-dev] [PATCH 2/3] glsl: Fix incorrect hard-coded location of the gl_SecondaryFragColorEXT built-in.

2016-08-23 Thread Francisco Jerez 
Francisco Jerez  writes:

> Ilia Mirkin  writes:
>
>> On Tue, Aug 23, 2016 at 12:18 AM, Francisco Jerez  
>> wrote:
>>> Ilia Mirkin  writes:
>>>
 On Tue, Aug 23, 2016 at 12:05 AM, Francisco Jerez  
 wrote:
> Ilia Mirkin  writes:
>
>> On Mon, Aug 22, 2016 at 10:55 PM, Francisco Jerez 
>>  wrote:
>>> Ilia Mirkin  writes:
>>>
 On Mon, Aug 22, 2016 at 9:59 PM, Francisco Jerez 
  wrote:
> gl_SecondaryFragColorEXT should have the same location as gl_FragColor
> for the secondary fragment color to be replicated to all fragment
> outputs.  The incorrect location of gl_SecondaryFragColorEXT would
> cause the linker to mark both FRAG_RESULT_COLOR and FRAG_RESULT_DATA0
> as being written to, which isn't allowed by the spec and would
> ultimately lead to an assertion failure in
> fs_visitor::emit_fb_writes() on my i965-fb-fetch branch.

 My recollection was that it didn't work with COLOR for "stupid"
 reasons. Can you confirm that
 bin/arb_blend_func_extended-fbo-extended-blend-pattern_gles2 -auto
 passes with this patch?

>>> Yes, it does, in fact
>>> arb_blend_func_extended-fbo-extended-blend-pattern_gles2 hits the i965
>>> assertion failure I mentioned above unless this patch is applied.
>>
>> This causes the test in question to fail on nouveau... the TGSI shader
>> generated starts with
>>
>> FRAG
>> PROPERTY FS_COLOR0_WRITES_ALL_CBUFS 1
>> DCL IN[0], POSITION, LINEAR
>> DCL OUT[0], SAMPLEMASK
>
> Heh, this smells a lot like the bug fixed in PATCH 1, but somewhere in
> the mesa state tracker.  st_glsl_to_tgsi.cpp:2422 does:
>
> | entry = new(mem_ctx) variable_storage(var,
> |   PROGRAM_OUTPUT,
> |   var->data.location
> |   + var->data.index);
>
> which is obviously bogus, e.g. for var->data.location ==
> FRAG_RESULT_COLOR and var->data.index == 1 you get
> FRAG_RESULT_SAMPLE_MASK which explains the sample mask declaration
> above.

 Right, because having FRAG_RESULT_COLOR and index != 0 was never
 possible prior to this. That might be why Ryan stuck it into
 FRAG_RESULT_DATA0 [I may have been the one to suggest that].
>>>
>>> Heh, so I guess that's the "stupid" reason you were referring to,
>>> working around this mesa state tracker bug in the GLSL front-end.
>>
>> Right. Or another way of looking at it, FRAG_RESULT_COLOR + index != 0
>> is illegal,
>
> That would be an acceptable limitation if it were taken into account
> consistently (which would imply among other things binding gl_FragColor
> to FRAG_RESULT_DATA0 if gl_SecondaryFragColor is written).  Otherwise
> you will be telling the linker and the back-end that both
> FRAG_RESULT_COLOR and FRAG_RESULT_DATA0 are being written, which is
> illegal.  The i965 has been misbehaving since forever because of this,
> but we just didn't notice until I added that assertion because the only
> symptom is increased shader recompiles.
>
>> (as it would, among other things, imply multi-rt support for
>> dual-source blending)
>
> FRAG_RESULT_COLOR + index != 0 doesn't imply multi-rt support, it's a
> requirement for multi-rt support, which the GLSL spec makes room for and
> some drivers in this tree could potentially support if the GLSL IR
> representation of dual-source blending wasn't deliberately inconsistent.
>
>> and the former code was making the GLSL fe not emit the illegal
>> combination.
>>
> I started digging into this and found out that that's not the only way
> the GLSL-to-TGSI pass relies on a GLSL bug: The output semantic array
> calculation in st_translate_fragment_program() relies on there being
> multiple locations marked as written in the shader's OutputsWritten set
> in the dual-source blending case (IOW you're relying on the bug fixed by
> the previous patch too).  GLSL is not TGSI, in GLSL the secondary and
> primary colors of a dual-source-blended output have the same location,
> so you cannot expect there will be multiple elements present in a bitset
> of locations.
>
>> Whichever way you look at it, breaking st/mesa isn't a great option.
>
> Then you may want to take a look at the attached patches in addition.
> They are untested and I have close to zero experience with the
> GLSL-to-TGSI pass, so they may break st/mesa after all.
>

Oops, I attached the wrong 0001-glsl.*.patch file, these are the right
patches.

>>   -ilia
>

From 7503e974d08636ae83d7248af8cc5991bd0034e5 Mon Sep 17 00:00:00 2001
From: Francisco Jerez 
Date: Tue, 23 Aug 2016 11:15:57 -0700

Re: [Mesa-dev] [PATCH 2/3] glsl: Fix incorrect hard-coded location of the gl_SecondaryFragColorEXT built-in.

2016-08-23 Thread Francisco Jerez 
Ilia Mirkin  writes:

> On Tue, Aug 23, 2016 at 12:18 AM, Francisco Jerez  
> wrote:
>> Ilia Mirkin  writes:
>>
>>> On Tue, Aug 23, 2016 at 12:05 AM, Francisco Jerez  
>>> wrote:
 Ilia Mirkin  writes:

> On Mon, Aug 22, 2016 at 10:55 PM, Francisco Jerez  
> wrote:
>> Ilia Mirkin  writes:
>>
>>> On Mon, Aug 22, 2016 at 9:59 PM, Francisco Jerez 
>>>  wrote:
 gl_SecondaryFragColorEXT should have the same location as gl_FragColor
 for the secondary fragment color to be replicated to all fragment
 outputs.  The incorrect location of gl_SecondaryFragColorEXT would
 cause the linker to mark both FRAG_RESULT_COLOR and FRAG_RESULT_DATA0
 as being written to, which isn't allowed by the spec and would
 ultimately lead to an assertion failure in
 fs_visitor::emit_fb_writes() on my i965-fb-fetch branch.
>>>
>>> My recollection was that it didn't work with COLOR for "stupid"
>>> reasons. Can you confirm that
>>> bin/arb_blend_func_extended-fbo-extended-blend-pattern_gles2 -auto
>>> passes with this patch?
>>>
>> Yes, it does, in fact
>> arb_blend_func_extended-fbo-extended-blend-pattern_gles2 hits the i965
>> assertion failure I mentioned above unless this patch is applied.
>
> This causes the test in question to fail on nouveau... the TGSI shader
> generated starts with
>
> FRAG
> PROPERTY FS_COLOR0_WRITES_ALL_CBUFS 1
> DCL IN[0], POSITION, LINEAR
> DCL OUT[0], SAMPLEMASK

 Heh, this smells a lot like the bug fixed in PATCH 1, but somewhere in
 the mesa state tracker.  st_glsl_to_tgsi.cpp:2422 does:

 | entry = new(mem_ctx) variable_storage(var,
 |   PROGRAM_OUTPUT,
 |   var->data.location
 |   + var->data.index);

 which is obviously bogus, e.g. for var->data.location ==
 FRAG_RESULT_COLOR and var->data.index == 1 you get
 FRAG_RESULT_SAMPLE_MASK which explains the sample mask declaration
 above.
>>>
>>> Right, because having FRAG_RESULT_COLOR and index != 0 was never
>>> possible prior to this. That might be why Ryan stuck it into
>>> FRAG_RESULT_DATA0 [I may have been the one to suggest that].
>>
>> Heh, so I guess that's the "stupid" reason you were referring to,
>> working around this mesa state tracker bug in the GLSL front-end.
>
> Right. Or another way of looking at it, FRAG_RESULT_COLOR + index != 0
> is illegal,

That would be an acceptable limitation if it were taken into account
consistently (which would imply among other things binding gl_FragColor
to FRAG_RESULT_DATA0 if gl_SecondaryFragColor is written).  Otherwise
you will be telling the linker and the back-end that both
FRAG_RESULT_COLOR and FRAG_RESULT_DATA0 are being written, which is
illegal.  The i965 has been misbehaving since forever because of this,
but we just didn't notice until I added that assertion because the only
symptom is increased shader recompiles.

> (as it would, among other things, imply multi-rt support for
> dual-source blending)

FRAG_RESULT_COLOR + index != 0 doesn't imply multi-rt support, it's a
requirement for multi-rt support, which the GLSL spec makes room for and
some drivers in this tree could potentially support if the GLSL IR
representation of dual-source blending wasn't deliberately inconsistent.

> and the former code was making the GLSL fe not emit the illegal
> combination.
>
I started digging into this and found out that that's not the only way
the GLSL-to-TGSI pass relies on a GLSL bug: The output semantic array
calculation in st_translate_fragment_program() relies on there being
multiple locations marked as written in the shader's OutputsWritten set
in the dual-source blending case (IOW you're relying on the bug fixed by
the previous patch too).  GLSL is not TGSI, in GLSL the secondary and
primary colors of a dual-source-blended output have the same location,
so you cannot expect there will be multiple elements present in a bitset
of locations.

> Whichever way you look at it, breaking st/mesa isn't a great option.

Then you may want to take a look at the attached patches in addition.
They are untested and I have close to zero experience with the
GLSL-to-TGSI pass, so they may break st/mesa after all.

>   -ilia

From ccd4f17436018688c77a23ea0728316a7f5141b3 Mon Sep 17 00:00:00 2001
From: Francisco Jerez 
Date: Sat, 20 Aug 2016 14:55:19 -0700
Subject: [PATCH] glsl: Fix gl_program::OutputsWritten computation for
 dual-source blending.

---
 src/compiler/glsl/ir_set_program_inouts.cpp | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/compiler/glsl/ir_set_program_inouts.cpp

Re: [Mesa-dev] [PATCH 2/3] glsl: Fix incorrect hard-coded location of the gl_SecondaryFragColorEXT built-in.

2016-08-23 Thread Ilia Mirkin 
On Tue, Aug 23, 2016 at 12:18 AM, Francisco Jerez  wrote:
> Ilia Mirkin  writes:
>
>> On Tue, Aug 23, 2016 at 12:05 AM, Francisco Jerez  
>> wrote:
>>> Ilia Mirkin  writes:
>>>
 On Mon, Aug 22, 2016 at 10:55 PM, Francisco Jerez  
 wrote:
> Ilia Mirkin  writes:
>
>> On Mon, Aug 22, 2016 at 9:59 PM, Francisco Jerez  
>> wrote:
>>> gl_SecondaryFragColorEXT should have the same location as gl_FragColor
>>> for the secondary fragment color to be replicated to all fragment
>>> outputs.  The incorrect location of gl_SecondaryFragColorEXT would
>>> cause the linker to mark both FRAG_RESULT_COLOR and FRAG_RESULT_DATA0
>>> as being written to, which isn't allowed by the spec and would
>>> ultimately lead to an assertion failure in
>>> fs_visitor::emit_fb_writes() on my i965-fb-fetch branch.
>>
>> My recollection was that it didn't work with COLOR for "stupid"
>> reasons. Can you confirm that
>> bin/arb_blend_func_extended-fbo-extended-blend-pattern_gles2 -auto
>> passes with this patch?
>>
> Yes, it does, in fact
> arb_blend_func_extended-fbo-extended-blend-pattern_gles2 hits the i965
> assertion failure I mentioned above unless this patch is applied.

 This causes the test in question to fail on nouveau... the TGSI shader
 generated starts with

 FRAG
 PROPERTY FS_COLOR0_WRITES_ALL_CBUFS 1
 DCL IN[0], POSITION, LINEAR
 DCL OUT[0], SAMPLEMASK
>>>
>>> Heh, this smells a lot like the bug fixed in PATCH 1, but somewhere in
>>> the mesa state tracker.  st_glsl_to_tgsi.cpp:2422 does:
>>>
>>> | entry = new(mem_ctx) variable_storage(var,
>>> |   PROGRAM_OUTPUT,
>>> |   var->data.location
>>> |   + var->data.index);
>>>
>>> which is obviously bogus, e.g. for var->data.location ==
>>> FRAG_RESULT_COLOR and var->data.index == 1 you get
>>> FRAG_RESULT_SAMPLE_MASK which explains the sample mask declaration
>>> above.
>>
>> Right, because having FRAG_RESULT_COLOR and index != 0 was never
>> possible prior to this. That might be why Ryan stuck it into
>> FRAG_RESULT_DATA0 [I may have been the one to suggest that].
>
> Heh, so I guess that's the "stupid" reason you were referring to,
> working around this mesa state tracker bug in the GLSL front-end.

Right. Or another way of looking at it, FRAG_RESULT_COLOR + index != 0
is illegal (as it would, among other things, imply multi-rt support
for dual-source blending), and the former code was making the GLSL fe
not emit the illegal combination. Whichever way you look at it,
breaking st/mesa isn't a great option.

  -ilia
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 2/3] glsl: Fix incorrect hard-coded location of the gl_SecondaryFragColorEXT built-in.

2016-08-23 Thread Francisco Jerez 
Ilia Mirkin  writes:

> On Mon, Aug 22, 2016 at 10:55 PM, Francisco Jerez  
> wrote:
>> Ilia Mirkin  writes:
>>
>>> On Mon, Aug 22, 2016 at 9:59 PM, Francisco Jerez  
>>> wrote:
 gl_SecondaryFragColorEXT should have the same location as gl_FragColor
 for the secondary fragment color to be replicated to all fragment
 outputs.  The incorrect location of gl_SecondaryFragColorEXT would
 cause the linker to mark both FRAG_RESULT_COLOR and FRAG_RESULT_DATA0
 as being written to, which isn't allowed by the spec and would
 ultimately lead to an assertion failure in
 fs_visitor::emit_fb_writes() on my i965-fb-fetch branch.
>>>
>>> My recollection was that it didn't work with COLOR for "stupid"
>>> reasons. Can you confirm that
>>> bin/arb_blend_func_extended-fbo-extended-blend-pattern_gles2 -auto
>>> passes with this patch?
>>>
>> Yes, it does, in fact
>> arb_blend_func_extended-fbo-extended-blend-pattern_gles2 hits the i965
>> assertion failure I mentioned above unless this patch is applied.
>
> This causes the test in question to fail on nouveau... the TGSI shader
> generated starts with
>
> FRAG
> PROPERTY FS_COLOR0_WRITES_ALL_CBUFS 1
> DCL IN[0], POSITION, LINEAR
> DCL OUT[0], SAMPLEMASK

Heh, this smells a lot like the bug fixed in PATCH 1, but somewhere in
the mesa state tracker.  st_glsl_to_tgsi.cpp:2422 does:

| entry = new(mem_ctx) variable_storage(var,
|   PROGRAM_OUTPUT,
|   var->data.location
|   + var->data.index);

which is obviously bogus, e.g. for var->data.location ==
FRAG_RESULT_COLOR and var->data.index == 1 you get
FRAG_RESULT_SAMPLE_MASK which explains the sample mask declaration
above.

> DCL OUT[1], COLOR
> DCL CONST[2..3]
> DCL CONST[0..1]
> DCL TEMP[0]
> DCL TEMP[1..2], LOCAL
>
> So clearly something gets confused. I think this needs a bit more
> investigation... without this patch, the header looks more like
>
> FRAG
> PROPERTY FS_COLOR0_WRITES_ALL_CBUFS 1
> DCL IN[0], POSITION, LINEAR
> DCL OUT[0], COLOR
> DCL OUT[1], COLOR[1]


signature.asc
Description: PGP signature
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 2/3] glsl: Fix incorrect hard-coded location of the gl_SecondaryFragColorEXT built-in.

2016-08-22 Thread Francisco Jerez 
Ilia Mirkin  writes:

> On Tue, Aug 23, 2016 at 12:05 AM, Francisco Jerez  
> wrote:
>> Ilia Mirkin  writes:
>>
>>> On Mon, Aug 22, 2016 at 10:55 PM, Francisco Jerez  
>>> wrote:
 Ilia Mirkin  writes:

> On Mon, Aug 22, 2016 at 9:59 PM, Francisco Jerez  
> wrote:
>> gl_SecondaryFragColorEXT should have the same location as gl_FragColor
>> for the secondary fragment color to be replicated to all fragment
>> outputs.  The incorrect location of gl_SecondaryFragColorEXT would
>> cause the linker to mark both FRAG_RESULT_COLOR and FRAG_RESULT_DATA0
>> as being written to, which isn't allowed by the spec and would
>> ultimately lead to an assertion failure in
>> fs_visitor::emit_fb_writes() on my i965-fb-fetch branch.
>
> My recollection was that it didn't work with COLOR for "stupid"
> reasons. Can you confirm that
> bin/arb_blend_func_extended-fbo-extended-blend-pattern_gles2 -auto
> passes with this patch?
>
 Yes, it does, in fact
 arb_blend_func_extended-fbo-extended-blend-pattern_gles2 hits the i965
 assertion failure I mentioned above unless this patch is applied.
>>>
>>> This causes the test in question to fail on nouveau... the TGSI shader
>>> generated starts with
>>>
>>> FRAG
>>> PROPERTY FS_COLOR0_WRITES_ALL_CBUFS 1
>>> DCL IN[0], POSITION, LINEAR
>>> DCL OUT[0], SAMPLEMASK
>>
>> Heh, this smells a lot like the bug fixed in PATCH 1, but somewhere in
>> the mesa state tracker.  st_glsl_to_tgsi.cpp:2422 does:
>>
>> | entry = new(mem_ctx) variable_storage(var,
>> |   PROGRAM_OUTPUT,
>> |   var->data.location
>> |   + var->data.index);
>>
>> which is obviously bogus, e.g. for var->data.location ==
>> FRAG_RESULT_COLOR and var->data.index == 1 you get
>> FRAG_RESULT_SAMPLE_MASK which explains the sample mask declaration
>> above.
>
> Right, because having FRAG_RESULT_COLOR and index != 0 was never
> possible prior to this. That might be why Ryan stuck it into
> FRAG_RESULT_DATA0 [I may have been the one to suggest that].

Heh, so I guess that's the "stupid" reason you were referring to,
working around this mesa state tracker bug in the GLSL front-end.


signature.asc
Description: PGP signature
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 2/3] glsl: Fix incorrect hard-coded location of the gl_SecondaryFragColorEXT built-in.

2016-08-22 Thread Ilia Mirkin 
On Tue, Aug 23, 2016 at 12:05 AM, Francisco Jerez  wrote:
> Ilia Mirkin  writes:
>
>> On Mon, Aug 22, 2016 at 10:55 PM, Francisco Jerez  
>> wrote:
>>> Ilia Mirkin  writes:
>>>
 On Mon, Aug 22, 2016 at 9:59 PM, Francisco Jerez  
 wrote:
> gl_SecondaryFragColorEXT should have the same location as gl_FragColor
> for the secondary fragment color to be replicated to all fragment
> outputs.  The incorrect location of gl_SecondaryFragColorEXT would
> cause the linker to mark both FRAG_RESULT_COLOR and FRAG_RESULT_DATA0
> as being written to, which isn't allowed by the spec and would
> ultimately lead to an assertion failure in
> fs_visitor::emit_fb_writes() on my i965-fb-fetch branch.

 My recollection was that it didn't work with COLOR for "stupid"
 reasons. Can you confirm that
 bin/arb_blend_func_extended-fbo-extended-blend-pattern_gles2 -auto
 passes with this patch?

>>> Yes, it does, in fact
>>> arb_blend_func_extended-fbo-extended-blend-pattern_gles2 hits the i965
>>> assertion failure I mentioned above unless this patch is applied.
>>
>> This causes the test in question to fail on nouveau... the TGSI shader
>> generated starts with
>>
>> FRAG
>> PROPERTY FS_COLOR0_WRITES_ALL_CBUFS 1
>> DCL IN[0], POSITION, LINEAR
>> DCL OUT[0], SAMPLEMASK
>
> Heh, this smells a lot like the bug fixed in PATCH 1, but somewhere in
> the mesa state tracker.  st_glsl_to_tgsi.cpp:2422 does:
>
> | entry = new(mem_ctx) variable_storage(var,
> |   PROGRAM_OUTPUT,
> |   var->data.location
> |   + var->data.index);
>
> which is obviously bogus, e.g. for var->data.location ==
> FRAG_RESULT_COLOR and var->data.index == 1 you get
> FRAG_RESULT_SAMPLE_MASK which explains the sample mask declaration
> above.

Right, because having FRAG_RESULT_COLOR and index != 0 was never
possible prior to this. That might be why Ryan stuck it into
FRAG_RESULT_DATA0 [I may have been the one to suggest that].
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 2/3] glsl: Fix incorrect hard-coded location of the gl_SecondaryFragColorEXT built-in.

2016-08-22 Thread Ilia Mirkin 
On Mon, Aug 22, 2016 at 10:55 PM, Francisco Jerez  wrote:
> Ilia Mirkin  writes:
>
>> On Mon, Aug 22, 2016 at 9:59 PM, Francisco Jerez  
>> wrote:
>>> gl_SecondaryFragColorEXT should have the same location as gl_FragColor
>>> for the secondary fragment color to be replicated to all fragment
>>> outputs.  The incorrect location of gl_SecondaryFragColorEXT would
>>> cause the linker to mark both FRAG_RESULT_COLOR and FRAG_RESULT_DATA0
>>> as being written to, which isn't allowed by the spec and would
>>> ultimately lead to an assertion failure in
>>> fs_visitor::emit_fb_writes() on my i965-fb-fetch branch.
>>
>> My recollection was that it didn't work with COLOR for "stupid"
>> reasons. Can you confirm that
>> bin/arb_blend_func_extended-fbo-extended-blend-pattern_gles2 -auto
>> passes with this patch?
>>
> Yes, it does, in fact
> arb_blend_func_extended-fbo-extended-blend-pattern_gles2 hits the i965
> assertion failure I mentioned above unless this patch is applied.

This causes the test in question to fail on nouveau... the TGSI shader
generated starts with

FRAG
PROPERTY FS_COLOR0_WRITES_ALL_CBUFS 1
DCL IN[0], POSITION, LINEAR
DCL OUT[0], SAMPLEMASK
DCL OUT[1], COLOR
DCL CONST[2..3]
DCL CONST[0..1]
DCL TEMP[0]
DCL TEMP[1..2], LOCAL

So clearly something gets confused. I think this needs a bit more
investigation... without this patch, the header looks more like

FRAG
PROPERTY FS_COLOR0_WRITES_ALL_CBUFS 1
DCL IN[0], POSITION, LINEAR
DCL OUT[0], COLOR
DCL OUT[1], COLOR[1]
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 2/3] glsl: Fix incorrect hard-coded location of the gl_SecondaryFragColorEXT built-in.

2016-08-22 Thread Francisco Jerez 
Ilia Mirkin  writes:

> On Mon, Aug 22, 2016 at 9:59 PM, Francisco Jerez  
> wrote:
>> gl_SecondaryFragColorEXT should have the same location as gl_FragColor
>> for the secondary fragment color to be replicated to all fragment
>> outputs.  The incorrect location of gl_SecondaryFragColorEXT would
>> cause the linker to mark both FRAG_RESULT_COLOR and FRAG_RESULT_DATA0
>> as being written to, which isn't allowed by the spec and would
>> ultimately lead to an assertion failure in
>> fs_visitor::emit_fb_writes() on my i965-fb-fetch branch.
>
> My recollection was that it didn't work with COLOR for "stupid"
> reasons. Can you confirm that
> bin/arb_blend_func_extended-fbo-extended-blend-pattern_gles2 -auto
> passes with this patch?
>
Yes, it does, in fact
arb_blend_func_extended-fbo-extended-blend-pattern_gles2 hits the i965
assertion failure I mentioned above unless this patch is applied.

>>
>> This should also fix the code below for multiple dual-source-blended
>> render targets, which no driver currently supports but we have plans
>> to enable eventually in the i965 driver (the comment saying that no
>> hardware will ever support it seems rather hilarious).
>> ---
>>  src/compiler/glsl/builtin_variables.cpp | 9 ++---
>>  1 file changed, 2 insertions(+), 7 deletions(-)
>>
>> diff --git a/src/compiler/glsl/builtin_variables.cpp 
>> b/src/compiler/glsl/builtin_variables.cpp
>> index cb5f730..b29f677 100644
>> --- a/src/compiler/glsl/builtin_variables.cpp
>> +++ b/src/compiler/glsl/builtin_variables.cpp
>> @@ -1145,13 +1145,8 @@ builtin_variable_generator::generate_fs_special_vars()
>> }
>>
>> if (state->es_shader && state->language_version == 100 && 
>> state->EXT_blend_func_extended_enable) {
>> -  /* We make an assumption here that there will only ever be one 
>> dual-source draw buffer
>> -   * In case this assumption is ever proven to be false, make sure to 
>> assert here
>> -   * since we don't handle this case.
>> -   * In practice, this issue will never arise since no hardware will 
>> support it.
>> -   */
>> -  assert(state->Const.MaxDualSourceDrawBuffers <= 1);
>> -  add_index_output(FRAG_RESULT_DATA0, 1, vec4_t, 
>> "gl_SecondaryFragColorEXT");
>> +  add_index_output(FRAG_RESULT_COLOR, 1, vec4_t,
>> +   "gl_SecondaryFragColorEXT");
>>add_index_output(FRAG_RESULT_DATA0, 1,
>> array(vec4_t, state->Const.MaxDualSourceDrawBuffers),
>> "gl_SecondaryFragDataEXT");
>> --
>> 2.9.0
>>
>> ___
>> mesa-dev mailing list
>> mesa-dev@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev


signature.asc
Description: PGP signature
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 2/3] glsl: Fix incorrect hard-coded location of the gl_SecondaryFragColorEXT built-in.

2016-08-22 Thread Ilia Mirkin 
On Mon, Aug 22, 2016 at 9:59 PM, Francisco Jerez  wrote:
> gl_SecondaryFragColorEXT should have the same location as gl_FragColor
> for the secondary fragment color to be replicated to all fragment
> outputs.  The incorrect location of gl_SecondaryFragColorEXT would
> cause the linker to mark both FRAG_RESULT_COLOR and FRAG_RESULT_DATA0
> as being written to, which isn't allowed by the spec and would
> ultimately lead to an assertion failure in
> fs_visitor::emit_fb_writes() on my i965-fb-fetch branch.

My recollection was that it didn't work with COLOR for "stupid"
reasons. Can you confirm that
bin/arb_blend_func_extended-fbo-extended-blend-pattern_gles2 -auto
passes with this patch?

>
> This should also fix the code below for multiple dual-source-blended
> render targets, which no driver currently supports but we have plans
> to enable eventually in the i965 driver (the comment saying that no
> hardware will ever support it seems rather hilarious).
> ---
>  src/compiler/glsl/builtin_variables.cpp | 9 ++---
>  1 file changed, 2 insertions(+), 7 deletions(-)
>
> diff --git a/src/compiler/glsl/builtin_variables.cpp 
> b/src/compiler/glsl/builtin_variables.cpp
> index cb5f730..b29f677 100644
> --- a/src/compiler/glsl/builtin_variables.cpp
> +++ b/src/compiler/glsl/builtin_variables.cpp
> @@ -1145,13 +1145,8 @@ builtin_variable_generator::generate_fs_special_vars()
> }
>
> if (state->es_shader && state->language_version == 100 && 
> state->EXT_blend_func_extended_enable) {
> -  /* We make an assumption here that there will only ever be one 
> dual-source draw buffer
> -   * In case this assumption is ever proven to be false, make sure to 
> assert here
> -   * since we don't handle this case.
> -   * In practice, this issue will never arise since no hardware will 
> support it.
> -   */
> -  assert(state->Const.MaxDualSourceDrawBuffers <= 1);
> -  add_index_output(FRAG_RESULT_DATA0, 1, vec4_t, 
> "gl_SecondaryFragColorEXT");
> +  add_index_output(FRAG_RESULT_COLOR, 1, vec4_t,
> +   "gl_SecondaryFragColorEXT");
>add_index_output(FRAG_RESULT_DATA0, 1,
> array(vec4_t, state->Const.MaxDualSourceDrawBuffers),
> "gl_SecondaryFragDataEXT");
> --
> 2.9.0
>
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH 2/3] glsl: Fix incorrect hard-coded location of the gl_SecondaryFragColorEXT built-in.

2016-08-22 Thread Francisco Jerez 
gl_SecondaryFragColorEXT should have the same location as gl_FragColor
for the secondary fragment color to be replicated to all fragment
outputs.  The incorrect location of gl_SecondaryFragColorEXT would
cause the linker to mark both FRAG_RESULT_COLOR and FRAG_RESULT_DATA0
as being written to, which isn't allowed by the spec and would
ultimately lead to an assertion failure in
fs_visitor::emit_fb_writes() on my i965-fb-fetch branch.

This should also fix the code below for multiple dual-source-blended
render targets, which no driver currently supports but we have plans
to enable eventually in the i965 driver (the comment saying that no
hardware will ever support it seems rather hilarious).
---
 src/compiler/glsl/builtin_variables.cpp | 9 ++---
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/src/compiler/glsl/builtin_variables.cpp 
b/src/compiler/glsl/builtin_variables.cpp
index cb5f730..b29f677 100644
--- a/src/compiler/glsl/builtin_variables.cpp
+++ b/src/compiler/glsl/builtin_variables.cpp
@@ -1145,13 +1145,8 @@ builtin_variable_generator::generate_fs_special_vars()
}
 
if (state->es_shader && state->language_version == 100 && 
state->EXT_blend_func_extended_enable) {
-  /* We make an assumption here that there will only ever be one 
dual-source draw buffer
-   * In case this assumption is ever proven to be false, make sure to 
assert here
-   * since we don't handle this case.
-   * In practice, this issue will never arise since no hardware will 
support it.
-   */
-  assert(state->Const.MaxDualSourceDrawBuffers <= 1);
-  add_index_output(FRAG_RESULT_DATA0, 1, vec4_t, 
"gl_SecondaryFragColorEXT");
+  add_index_output(FRAG_RESULT_COLOR, 1, vec4_t,
+   "gl_SecondaryFragColorEXT");
   add_index_output(FRAG_RESULT_DATA0, 1,
array(vec4_t, state->Const.MaxDualSourceDrawBuffers),
"gl_SecondaryFragDataEXT");
-- 
2.9.0

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev