Re: [Mesa-dev] [PATCH] i965: do not release GLSL IR for SSO programs

2016-11-01 Thread Timothy Arceri 


On 1 November 2016 6:00:46 pm AEDT, Timothy Arceri 
 wrote:
>On Tue, 2016-11-01 at 08:06 +0200, Tapani Pälli wrote:
>> 
>> On 10/29/2016 03:29 AM, Kenneth Graunke wrote:
>> > 
>> > On Friday, October 28, 2016 10:39:01 AM PDT Tapani Pälli wrote:
>> > > 
>> > > On 10/28/2016 05:15 AM, Timothy Arceri wrote:
>> > > > 
>> > > > On Thu, 2016-10-27 at 18:51 -0700, Kenneth Graunke wrote:
>> > > > > 
>> > > > > On Thursday, October 27, 2016 9:03:12 PM PDT Timothy Arceri
>> > > > > wrote:
>> > > > > > 
>> > > > > > On Thu, 2016-10-27 at 12:37 +1100, Timothy Arceri wrote:
>> > > > > > > 
>> > > > > > > Agreed but as far as I can tell we shouldn't even need
>> > > > > > > gl_linked_shader
>> > > > > > > after glLinkProgram.
>> > > > > > > 
>> > > > > > > We should probably just free it after linking. Everything
>> > > > > > > we want
>> > > > > > > *should* have been copied to gl_program, however in
>> > > > > > > practice it
>> > > > > > > seems
>> > > > > > > there are some things we still get from gl_linked_shader
>> > > > > > > like NumImages, but with the changes I landed recently we
>> > > > > > > could
>> > > > > > > just
>> > > > > > > be getting this from the new shader_info struct.
>> > > > > > I've started work on this:
>> > > > > > 
>> > > > > > https://patchwork.freedesktop.org/series/14471/
>> > > > > > 
>> > > > > > But I'm not sure I have is in me to do another big refactor
>> > > > > > right
>> > > > > > now.
>> > > > > Is there a reasonable short-term fix?  This regression is
>> > > > > present on
>> > > > > the 13.0 branch and we really ought to fix it before Emil
>> > > > > cuts the
>> > > > > 13.0
>> > > > > final release.  But it'd be nice if we could keep some
>> > > > > freeing of
>> > > > > IR...
>> > > > I was thinking about this yesterday and the only options I see
>> > > > are
>> > > > using this patch for the 13.0 branch or leaving it broken.
>> > > > 
>> > > > The scenario for this bug is highly unlikey you need to be in
>> > > > compat
>> > > > profile, and relinking a SSO program that has had its shaders
>> > > > detached.
>> > > 
>> > > Yeah, this is quite exotic, I'd vote for this patch now and we
>> > > can make
>> > > things better from there. I hope Serious Sam 3 (which is the only
>> > > program I know using SSO) does core profile. Eero, have we
>> > > recently run
>> > > this game if it still works?
>> > 
>> > Actually, lots of games use SSO...
>> > 
>> > - Bioshock Infinite
>> > - Chivalry: Medieval Warfare
>> > - DiRT Showdown
>> > - Dota 2
>> > - Saints Row: The Third
>> > - Serious Sam 3
>> > - The Talos Principle
>> > - Tomb Raider
>> > 
>> > and probably a bunch of others.  As far as I know they all use core
>> > profile, though.
>> > 
>> 
>> OK, good to know there's some more games.
>> 
>> So .. how about the proposal of using this patch?
>> 
>> IMHO I would prefer instead erroring out if there are no shaders but 
>> compatibility profile allows such empty programs to link. I'd like
>> to 
>> understand this case better. I don't understand what is expected
>> result 
>> when there is no VS or FS and then we use the shader to draw
>> something? 
>> What kind of output should be generated?
>
>Yeah I have a feeling what we are going to end up with after this patch
>is not really what we want. I'm not sure what is meant to happen
>exactly but I doubt using the IR that was left behind is it. With that
>in mind I don't know if the patch is worth it ... I guess it does stop
>a crash.
>
>I suspect we really need to not relink things in the backend. Maybe we
>could just check NumShaders like we do in the glsl linker in the i965
>link code and exit early (without error) if its compat (assert in
>core).

Actually deleting the linked shaders when we return early in the glsl pass is 
probably what we want to do.

 >But again I don't know if this acctually results in something
>that functions correctly.
>
>
>> 
>> // Tapani
>> ___
>> 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

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] i965: do not release GLSL IR for SSO programs

2016-11-01 Thread Timothy Arceri 
On Tue, 2016-11-01 at 08:06 +0200, Tapani Pälli wrote:
> 
> On 10/29/2016 03:29 AM, Kenneth Graunke wrote:
> > 
> > On Friday, October 28, 2016 10:39:01 AM PDT Tapani Pälli wrote:
> > > 
> > > On 10/28/2016 05:15 AM, Timothy Arceri wrote:
> > > > 
> > > > On Thu, 2016-10-27 at 18:51 -0700, Kenneth Graunke wrote:
> > > > > 
> > > > > On Thursday, October 27, 2016 9:03:12 PM PDT Timothy Arceri
> > > > > wrote:
> > > > > > 
> > > > > > On Thu, 2016-10-27 at 12:37 +1100, Timothy Arceri wrote:
> > > > > > > 
> > > > > > > Agreed but as far as I can tell we shouldn't even need
> > > > > > > gl_linked_shader
> > > > > > > after glLinkProgram.
> > > > > > > 
> > > > > > > We should probably just free it after linking. Everything
> > > > > > > we want
> > > > > > > *should* have been copied to gl_program, however in
> > > > > > > practice it
> > > > > > > seems
> > > > > > > there are some things we still get from gl_linked_shader
> > > > > > > like NumImages, but with the changes I landed recently we
> > > > > > > could
> > > > > > > just
> > > > > > > be getting this from the new shader_info struct.
> > > > > > I've started work on this:
> > > > > > 
> > > > > > https://patchwork.freedesktop.org/series/14471/
> > > > > > 
> > > > > > But I'm not sure I have is in me to do another big refactor
> > > > > > right
> > > > > > now.
> > > > > Is there a reasonable short-term fix?  This regression is
> > > > > present on
> > > > > the 13.0 branch and we really ought to fix it before Emil
> > > > > cuts the
> > > > > 13.0
> > > > > final release.  But it'd be nice if we could keep some
> > > > > freeing of
> > > > > IR...
> > > > I was thinking about this yesterday and the only options I see
> > > > are
> > > > using this patch for the 13.0 branch or leaving it broken.
> > > > 
> > > > The scenario for this bug is highly unlikey you need to be in
> > > > compat
> > > > profile, and relinking a SSO program that has had its shaders
> > > > detached.
> > > 
> > > Yeah, this is quite exotic, I'd vote for this patch now and we
> > > can make
> > > things better from there. I hope Serious Sam 3 (which is the only
> > > program I know using SSO) does core profile. Eero, have we
> > > recently run
> > > this game if it still works?
> > 
> > Actually, lots of games use SSO...
> > 
> > - Bioshock Infinite
> > - Chivalry: Medieval Warfare
> > - DiRT Showdown
> > - Dota 2
> > - Saints Row: The Third
> > - Serious Sam 3
> > - The Talos Principle
> > - Tomb Raider
> > 
> > and probably a bunch of others.  As far as I know they all use core
> > profile, though.
> > 
> 
> OK, good to know there's some more games.
> 
> So .. how about the proposal of using this patch?
> 
> IMHO I would prefer instead erroring out if there are no shaders but 
> compatibility profile allows such empty programs to link. I'd like
> to 
> understand this case better. I don't understand what is expected
> result 
> when there is no VS or FS and then we use the shader to draw
> something? 
> What kind of output should be generated?

Yeah I have a feeling what we are going to end up with after this patch
is not really what we want. I'm not sure what is meant to happen
exactly but I doubt using the IR that was left behind is it. With that
in mind I don't know if the patch is worth it ... I guess it does stop
a crash.

I suspect we really need to not relink things in the backend. Maybe we
could just check NumShaders like we do in the glsl linker in the i965
link code and exit early (without error) if its compat (assert in
core). But again I don't know if this acctually results in something
that functions correctly.


> 
> // Tapani
> ___
> 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

Re: [Mesa-dev] [PATCH] i965: do not release GLSL IR for SSO programs

2016-11-01 Thread Tapani Pälli 



On 10/29/2016 03:29 AM, Kenneth Graunke wrote:

On Friday, October 28, 2016 10:39:01 AM PDT Tapani Pälli wrote:

On 10/28/2016 05:15 AM, Timothy Arceri wrote:

On Thu, 2016-10-27 at 18:51 -0700, Kenneth Graunke wrote:

On Thursday, October 27, 2016 9:03:12 PM PDT Timothy Arceri wrote:

On Thu, 2016-10-27 at 12:37 +1100, Timothy Arceri wrote:

Agreed but as far as I can tell we shouldn't even need
gl_linked_shader
after glLinkProgram.

We should probably just free it after linking. Everything we want
*should* have been copied to gl_program, however in practice it
seems
there are some things we still get from gl_linked_shader
like NumImages, but with the changes I landed recently we could
just
be getting this from the new shader_info struct.

I've started work on this:

https://patchwork.freedesktop.org/series/14471/

But I'm not sure I have is in me to do another big refactor right
now.

Is there a reasonable short-term fix?  This regression is present on
the 13.0 branch and we really ought to fix it before Emil cuts the
13.0
final release.  But it'd be nice if we could keep some freeing of
IR...

I was thinking about this yesterday and the only options I see are
using this patch for the 13.0 branch or leaving it broken.

The scenario for this bug is highly unlikey you need to be in compat
profile, and relinking a SSO program that has had its shaders
detached.


Yeah, this is quite exotic, I'd vote for this patch now and we can make
things better from there. I hope Serious Sam 3 (which is the only
program I know using SSO) does core profile. Eero, have we recently run
this game if it still works?


Actually, lots of games use SSO...

- Bioshock Infinite
- Chivalry: Medieval Warfare
- DiRT Showdown
- Dota 2
- Saints Row: The Third
- Serious Sam 3
- The Talos Principle
- Tomb Raider

and probably a bunch of others.  As far as I know they all use core
profile, though.



OK, good to know there's some more games.

So .. how about the proposal of using this patch?

IMHO I would prefer instead erroring out if there are no shaders but 
compatibility profile allows such empty programs to link. I'd like to 
understand this case better. I don't understand what is expected result 
when there is no VS or FS and then we use the shader to draw something? 
What kind of output should be generated?


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

Re: [Mesa-dev] [PATCH] i965: do not release GLSL IR for SSO programs

2016-10-28 Thread Kenneth Graunke 
On Friday, October 28, 2016 10:39:01 AM PDT Tapani Pälli wrote:
> On 10/28/2016 05:15 AM, Timothy Arceri wrote:
> > On Thu, 2016-10-27 at 18:51 -0700, Kenneth Graunke wrote:
> >> On Thursday, October 27, 2016 9:03:12 PM PDT Timothy Arceri wrote:
> >>> On Thu, 2016-10-27 at 12:37 +1100, Timothy Arceri wrote:
>  Agreed but as far as I can tell we shouldn't even need
>  gl_linked_shader
>  after glLinkProgram.
> 
>  We should probably just free it after linking. Everything we want
>  *should* have been copied to gl_program, however in practice it
>  seems
>  there are some things we still get from gl_linked_shader
>  like NumImages, but with the changes I landed recently we could
>  just
>  be getting this from the new shader_info struct.
> >>> I've started work on this:
> >>>
> >>> https://patchwork.freedesktop.org/series/14471/
> >>>
> >>> But I'm not sure I have is in me to do another big refactor right
> >>> now.
> >> Is there a reasonable short-term fix?  This regression is present on
> >> the 13.0 branch and we really ought to fix it before Emil cuts the
> >> 13.0
> >> final release.  But it'd be nice if we could keep some freeing of
> >> IR...
> > I was thinking about this yesterday and the only options I see are
> > using this patch for the 13.0 branch or leaving it broken.
> >
> > The scenario for this bug is highly unlikey you need to be in compat
> > profile, and relinking a SSO program that has had its shaders
> > detached.
> 
> Yeah, this is quite exotic, I'd vote for this patch now and we can make 
> things better from there. I hope Serious Sam 3 (which is the only 
> program I know using SSO) does core profile. Eero, have we recently run 
> this game if it still works?

Actually, lots of games use SSO...

- Bioshock Infinite
- Chivalry: Medieval Warfare
- DiRT Showdown
- Dota 2
- Saints Row: The Third
- Serious Sam 3
- The Talos Principle
- Tomb Raider

and probably a bunch of others.  As far as I know they all use core
profile, though.


signature.asc
Description: This is a digitally signed message part.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] i965: do not release GLSL IR for SSO programs

2016-10-28 Thread Tapani Pälli 

On 10/28/2016 05:15 AM, Timothy Arceri wrote:

On Thu, 2016-10-27 at 18:51 -0700, Kenneth Graunke wrote:

On Thursday, October 27, 2016 9:03:12 PM PDT Timothy Arceri wrote:

On Thu, 2016-10-27 at 12:37 +1100, Timothy Arceri wrote:

Agreed but as far as I can tell we shouldn't even need
gl_linked_shader
after glLinkProgram.

We should probably just free it after linking. Everything we want
*should* have been copied to gl_program, however in practice it
seems
there are some things we still get from gl_linked_shader
like NumImages, but with the changes I landed recently we could
just
be getting this from the new shader_info struct.

I've started work on this:

https://patchwork.freedesktop.org/series/14471/

But I'm not sure I have is in me to do another big refactor right
now.

Is there a reasonable short-term fix?  This regression is present on
the 13.0 branch and we really ought to fix it before Emil cuts the
13.0
final release.  But it'd be nice if we could keep some freeing of
IR...

I was thinking about this yesterday and the only options I see are
using this patch for the 13.0 branch or leaving it broken.

The scenario for this bug is highly unlikey you need to be in compat
profile, and relinking a SSO program that has had its shaders
detached.


Yeah, this is quite exotic, I'd vote for this patch now and we can make 
things better from there. I hope Serious Sam 3 (which is the only 
program I know using SSO) does core profile. Eero, have we recently run 
this game if it still works?



Refactoring is probably a good idea in the longer term

Looking at things yesterday I think there are some bugs (at least in
i965) we would hit currently with atomics and images if we were to fail
relinking a program, and then the program was to recompile a shader
variant due to storing state in gl_linked_shader.


--Ken
___
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 mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] i965: do not release GLSL IR for SSO programs

2016-10-27 Thread Timothy Arceri 
On Thu, 2016-10-27 at 18:51 -0700, Kenneth Graunke wrote:
> On Thursday, October 27, 2016 9:03:12 PM PDT Timothy Arceri wrote:
> > 
> > On Thu, 2016-10-27 at 12:37 +1100, Timothy Arceri wrote:
> > > 
> > > Agreed but as far as I can tell we shouldn't even need
> > > gl_linked_shader
> > > after glLinkProgram.
> > > 
> > > We should probably just free it after linking. Everything we want
> > > *should* have been copied to gl_program, however in practice it
> > > seems
> > > there are some things we still get from gl_linked_shader
> > > like NumImages, but with the changes I landed recently we could
> > > just
> > > be getting this from the new shader_info struct.
> > 
> > I've started work on this:
> > 
> > https://patchwork.freedesktop.org/series/14471/
> > 
> > But I'm not sure I have is in me to do another big refactor right
> > now. 
> 
> Is there a reasonable short-term fix?  This regression is present on
> the 13.0 branch and we really ought to fix it before Emil cuts the
> 13.0
> final release.  But it'd be nice if we could keep some freeing of
> IR...

I was thinking about this yesterday and the only options I see are
using this patch for the 13.0 branch or leaving it broken.

The scenario for this bug is highly unlikey you need to be in compat
profile, and relinking a SSO program that has had its shaders
detached. 

> 
> Refactoring is probably a good idea in the longer term

Looking at things yesterday I think there are some bugs (at least in
i965) we would hit currently with atomics and images if we were to fail
relinking a program, and then the program was to recompile a shader
variant due to storing state in gl_linked_shader.

> 
> --Ken
> ___
> 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

Re: [Mesa-dev] [PATCH] i965: do not release GLSL IR for SSO programs

2016-10-27 Thread Kenneth Graunke 
On Thursday, October 27, 2016 9:03:12 PM PDT Timothy Arceri wrote:
> On Thu, 2016-10-27 at 12:37 +1100, Timothy Arceri wrote:
> > Agreed but as far as I can tell we shouldn't even need
> > gl_linked_shader
> > after glLinkProgram.
> > 
> > We should probably just free it after linking. Everything we want
> > *should* have been copied to gl_program, however in practice it seems
> > there are some things we still get from gl_linked_shader
> > like NumImages, but with the changes I landed recently we could just
> > be getting this from the new shader_info struct.
> 
> I've started work on this:
> 
> https://patchwork.freedesktop.org/series/14471/
> 
> But I'm not sure I have is in me to do another big refactor right now. 

Is there a reasonable short-term fix?  This regression is present on
the 13.0 branch and we really ought to fix it before Emil cuts the 13.0
final release.  But it'd be nice if we could keep some freeing of IR...

Refactoring is probably a good idea in the longer term

--Ken


signature.asc
Description: This is a digitally signed message part.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] i965: do not release GLSL IR for SSO programs

2016-10-27 Thread Timothy Arceri 
On Thu, 2016-10-27 at 12:37 +1100, Timothy Arceri wrote:
> On Wed, 2016-10-26 at 20:19 -0400, Ilia Mirkin wrote:
> > 
> > On Wed, Oct 26, 2016 at 8:08 PM, Timothy Arceri
> >  wrote:
> > > 
> > > 
> > > On Wed, 2016-10-26 at 22:51 +1100, Timothy Arceri wrote:
> > > > 
> > > > 
> > > > On Wed, 2016-10-26 at 13:13 +0300, Tapani Pälli wrote:
> > > > > 
> > > > > 
> > > > > 
> > > > > Hi;
> > > > > 
> > > > > On 10/26/2016 11:27 AM, Tapani Pälli wrote:
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > On 10/26/2016 11:21 AM, Timothy Arceri wrote:
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > On Wed, 2016-10-26 at 08:50 +0300, Tapani Pälli wrote:
> > > > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > > > On 10/26/2016 08:15 AM, Timothy Arceri wrote:
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > On Tue, 2016-10-25 at 09:39 +0300, Tapani Pälli
> > > > > > > > > wrote:
> > > > > > > > > > 
> > > > > > > > > > 
> > > > > > > > > > 
> > > > > > > > > > 
> > > > > > > > > > 
> > > > > > > > > > SSO shader programs can be later modified by
> > > > > > > > > > attaching/detaching
> > > > > > > > > > shaders and relinked, this requires IR.
> > > > > > > > > 
> > > > > > > > > Doesn't relinking recreate the IR? We can relink
> > > > > > > > > exiting
> > > > > > > > > shaders
> > > > > > > > > into
> > > > > > > > > new programs. The IR is cloned from gl_shader (the
> > > > > > > > > compiled
> > > > > > > > > IR)
> > > > > > > > > before
> > > > > > > > > this happens.
> > > > > > > > > 
> > > > > > > > > Where exactly are things falling over for SSO?
> > > > > > > > 
> > > > > > > > I went this way as I haven't seen this happening
> > > > > > > > elsewhere
> > > > > > > > but
> > > > > > > > when
> > > > > > > > relinking program created by glCreateShaderProgram. TBH
> > > > > > > > I'm
> > > > > > > > not
> > > > > > > > sure
> > > > > > > > if
> > > > > > > > this could happen with regular programs, I would assume
> > > > > > > > we
> > > > > > > > had
> > > > > > > > already
> > > > > > > > bugs if it would be so (?)
> > > > > > > > 
> > > > > > > > In practice things go bad in brw_link_shader where
> > > > > > > > process_glsl_ir()
> > > > > > > > gets called, like this:
> > > > > > > > 
> > > > > > > > --- 8< ---
> > > > > > > > #0  do_expression_flattening (instructions=instructions
> > > > > > > > @e
> > > > > > > > ntry
> > > > > > > > =0
> > > > > > > > x0,
> > > > > > > > predicate=predicate@entry=0x70f808a0
> > > > > > > > ) at
> > > > > > > > glsl/ir_expression_flattening.cpp:60
> > > > > > > > #1  0x70f816b9 in do_mat_op_to_vec
> > > > > > > > (instructions=0x0)
> > > > > > > > at
> > > > > > > > glsl/lower_mat_op_to_vec.cpp:96
> > > > > > > > #2  0x710385bd in process_glsl_ir
> > > > > > > > (shader_prog=0x9b74d8,
> > > > > > > > shader=0x9b2688, brw=0x7d2478) at brw_link.cpp:108
> > > > > > > > #3  brw_link_shader (ctx=0x7d2478, shProg=0x9b74d8) at
> > > > > > > > brw_link.cpp:234
> > > > > > > > #4  0x70ec1f31 in _mesa_glsl_link_shader
> > > > > > > > (ctx=0x7d2478,
> > > > > > > > prog=0x9b74d8) at program/ir_to_mesa.cpp:3067
> > > > > > > > #5  0x70ddc99b in _mesa_link_program
> > > > > > > > (ctx=0x7d2478,
> > > > > > > > shProg=0x9b74d8) at main/shaderapi.c:1098
> > > > > > > > #6  0x77ac8d15 in stub_glLinkProgram
> > > > > > > > (program=2)
> > > > > > > > at
> > > > > > > > /home/tpalli/source/fdo/piglit/tests/util/piglit-
> > > > > > > > dispatch-
> > > > > > > > gen.c:33005
> > > > > > > > #7  0x004019ab in
> > > > > > > > relink_program_created_by_glCreateShaderProgram () at
> > > > > > > > /home/tpalli/source/fdo/piglit/tests/spec/arb_separate_
> > > > > > > > sh
> > > > > > > > ader
> > > > > > > > _o
> > > > > > > > bjects
> > > > > > > > /api-errors.c:78
> > > > > > > 
> > > > > > > There is a comment above that line in the test.
> > > > > > > 
> > > > > > > /* Issue #14 of the GL_ARB_separate_shader_objects spec
> > > > > > > says:
> > > > > > > *
> > > > > > >  * "14. Should glLinkProgram work to re-link a shader
> > > > > > > created
> > > > > > > with
> > > > > > >  *  glCreateShaderProgram?
> > > > > > >  *
> > > > > > >  *  RESOLVED: NO because the shader created by
> > > > > > >  *  glCreateShaderProgram is detached and deleted
> > > > > > > as
> > > > > > > part
> > > > > > > of
> > > > > > >  *  the glCreateShaderProgram sequence.  This
> > > > > > > means
> > > > > > > if
> > > > > > > you
> > > > > > >  *  call glLinkProgram on a program returned from
> > > > > > >  *  glCreateShaderProgram, you'll find the re-
> > > > > > > link
> > > > > > > fails
> > > > > > >  *  because no shader object is attached.
> > > > > > >  *
> >

Re: [Mesa-dev] [PATCH] i965: do not release GLSL IR for SSO programs

2016-10-26 Thread Timothy Arceri 
On Wed, 2016-10-26 at 20:19 -0400, Ilia Mirkin wrote:
> On Wed, Oct 26, 2016 at 8:08 PM, Timothy Arceri
>  wrote:
> > 
> > On Wed, 2016-10-26 at 22:51 +1100, Timothy Arceri wrote:
> > > 
> > > On Wed, 2016-10-26 at 13:13 +0300, Tapani Pälli wrote:
> > > > 
> > > > 
> > > > Hi;
> > > > 
> > > > On 10/26/2016 11:27 AM, Tapani Pälli wrote:
> > > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > On 10/26/2016 11:21 AM, Timothy Arceri wrote:
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > On Wed, 2016-10-26 at 08:50 +0300, Tapani Pälli wrote:
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > On 10/26/2016 08:15 AM, Timothy Arceri wrote:
> > > > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > > > On Tue, 2016-10-25 at 09:39 +0300, Tapani Pälli wrote:
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > SSO shader programs can be later modified by
> > > > > > > > > attaching/detaching
> > > > > > > > > shaders and relinked, this requires IR.
> > > > > > > > 
> > > > > > > > Doesn't relinking recreate the IR? We can relink
> > > > > > > > exiting
> > > > > > > > shaders
> > > > > > > > into
> > > > > > > > new programs. The IR is cloned from gl_shader (the
> > > > > > > > compiled
> > > > > > > > IR)
> > > > > > > > before
> > > > > > > > this happens.
> > > > > > > > 
> > > > > > > > Where exactly are things falling over for SSO?
> > > > > > > 
> > > > > > > I went this way as I haven't seen this happening
> > > > > > > elsewhere
> > > > > > > but
> > > > > > > when
> > > > > > > relinking program created by glCreateShaderProgram. TBH
> > > > > > > I'm
> > > > > > > not
> > > > > > > sure
> > > > > > > if
> > > > > > > this could happen with regular programs, I would assume
> > > > > > > we
> > > > > > > had
> > > > > > > already
> > > > > > > bugs if it would be so (?)
> > > > > > > 
> > > > > > > In practice things go bad in brw_link_shader where
> > > > > > > process_glsl_ir()
> > > > > > > gets called, like this:
> > > > > > > 
> > > > > > > --- 8< ---
> > > > > > > #0  do_expression_flattening (instructions=instructions@e
> > > > > > > ntry
> > > > > > > =0
> > > > > > > x0,
> > > > > > > predicate=predicate@entry=0x70f808a0
> > > > > > > ) at
> > > > > > > glsl/ir_expression_flattening.cpp:60
> > > > > > > #1  0x70f816b9 in do_mat_op_to_vec
> > > > > > > (instructions=0x0)
> > > > > > > at
> > > > > > > glsl/lower_mat_op_to_vec.cpp:96
> > > > > > > #2  0x710385bd in process_glsl_ir
> > > > > > > (shader_prog=0x9b74d8,
> > > > > > > shader=0x9b2688, brw=0x7d2478) at brw_link.cpp:108
> > > > > > > #3  brw_link_shader (ctx=0x7d2478, shProg=0x9b74d8) at
> > > > > > > brw_link.cpp:234
> > > > > > > #4  0x70ec1f31 in _mesa_glsl_link_shader
> > > > > > > (ctx=0x7d2478,
> > > > > > > prog=0x9b74d8) at program/ir_to_mesa.cpp:3067
> > > > > > > #5  0x70ddc99b in _mesa_link_program
> > > > > > > (ctx=0x7d2478,
> > > > > > > shProg=0x9b74d8) at main/shaderapi.c:1098
> > > > > > > #6  0x77ac8d15 in stub_glLinkProgram (program=2)
> > > > > > > at
> > > > > > > /home/tpalli/source/fdo/piglit/tests/util/piglit-
> > > > > > > dispatch-
> > > > > > > gen.c:33005
> > > > > > > #7  0x004019ab in
> > > > > > > relink_program_created_by_glCreateShaderProgram () at
> > > > > > > /home/tpalli/source/fdo/piglit/tests/spec/arb_separate_sh
> > > > > > > ader
> > > > > > > _o
> > > > > > > bjects
> > > > > > > /api-errors.c:78
> > > > > > 
> > > > > > There is a comment above that line in the test.
> > > > > > 
> > > > > > /* Issue #14 of the GL_ARB_separate_shader_objects spec
> > > > > > says:
> > > > > > *
> > > > > >  * "14. Should glLinkProgram work to re-link a shader
> > > > > > created
> > > > > > with
> > > > > >  *  glCreateShaderProgram?
> > > > > >  *
> > > > > >  *  RESOLVED: NO because the shader created by
> > > > > >  *  glCreateShaderProgram is detached and deleted
> > > > > > as
> > > > > > part
> > > > > > of
> > > > > >  *  the glCreateShaderProgram sequence.  This means
> > > > > > if
> > > > > > you
> > > > > >  *  call glLinkProgram on a program returned from
> > > > > >  *  glCreateShaderProgram, you'll find the re-link
> > > > > > fails
> > > > > >  *  because no shader object is attached.
> > > > > >  *
> > > > > >  *  An application is free to attach one or more
> > > > > > new
> > > > > > shader
> > > > > >  *  objects to the program and then relink would
> > > > > > work.
> > > > > >  *
> > > > > >  *  This is fine because re-linking isn't
> > > > > > necessary/expected."
> > > > > >  */
> > > > > > 
> > > > > > Which means we shouldn't able to relink this shader. We
> > > > > > shouldn't
> > > > > > be
> > > > > > able to get as far along as we do when we get the error
> > > > > > message.
> > > > >

Re: [Mesa-dev] [PATCH] i965: do not release GLSL IR for SSO programs

2016-10-26 Thread Ilia Mirkin 
On Wed, Oct 26, 2016 at 8:08 PM, Timothy Arceri
 wrote:
> On Wed, 2016-10-26 at 22:51 +1100, Timothy Arceri wrote:
>> On Wed, 2016-10-26 at 13:13 +0300, Tapani Pälli wrote:
>> >
>> > Hi;
>> >
>> > On 10/26/2016 11:27 AM, Tapani Pälli wrote:
>> > >
>> > >
>> > >
>> > >
>> > > On 10/26/2016 11:21 AM, Timothy Arceri wrote:
>> > > >
>> > > >
>> > > > On Wed, 2016-10-26 at 08:50 +0300, Tapani Pälli wrote:
>> > > > >
>> > > > >
>> > > > >
>> > > > > On 10/26/2016 08:15 AM, Timothy Arceri wrote:
>> > > > > >
>> > > > > >
>> > > > > >
>> > > > > > On Tue, 2016-10-25 at 09:39 +0300, Tapani Pälli wrote:
>> > > > > > >
>> > > > > > >
>> > > > > > >
>> > > > > > > SSO shader programs can be later modified by
>> > > > > > > attaching/detaching
>> > > > > > > shaders and relinked, this requires IR.
>> > > > > >
>> > > > > > Doesn't relinking recreate the IR? We can relink exiting
>> > > > > > shaders
>> > > > > > into
>> > > > > > new programs. The IR is cloned from gl_shader (the compiled
>> > > > > > IR)
>> > > > > > before
>> > > > > > this happens.
>> > > > > >
>> > > > > > Where exactly are things falling over for SSO?
>> > > > >
>> > > > > I went this way as I haven't seen this happening elsewhere
>> > > > > but
>> > > > > when
>> > > > > relinking program created by glCreateShaderProgram. TBH I'm
>> > > > > not
>> > > > > sure
>> > > > > if
>> > > > > this could happen with regular programs, I would assume we
>> > > > > had
>> > > > > already
>> > > > > bugs if it would be so (?)
>> > > > >
>> > > > > In practice things go bad in brw_link_shader where
>> > > > > process_glsl_ir()
>> > > > > gets called, like this:
>> > > > >
>> > > > > --- 8< ---
>> > > > > #0  do_expression_flattening (instructions=instructions@entry
>> > > > > =0
>> > > > > x0,
>> > > > > predicate=predicate@entry=0x70f808a0
>> > > > > ) at
>> > > > > glsl/ir_expression_flattening.cpp:60
>> > > > > #1  0x70f816b9 in do_mat_op_to_vec (instructions=0x0)
>> > > > > at
>> > > > > glsl/lower_mat_op_to_vec.cpp:96
>> > > > > #2  0x710385bd in process_glsl_ir
>> > > > > (shader_prog=0x9b74d8,
>> > > > > shader=0x9b2688, brw=0x7d2478) at brw_link.cpp:108
>> > > > > #3  brw_link_shader (ctx=0x7d2478, shProg=0x9b74d8) at
>> > > > > brw_link.cpp:234
>> > > > > #4  0x70ec1f31 in _mesa_glsl_link_shader
>> > > > > (ctx=0x7d2478,
>> > > > > prog=0x9b74d8) at program/ir_to_mesa.cpp:3067
>> > > > > #5  0x70ddc99b in _mesa_link_program (ctx=0x7d2478,
>> > > > > shProg=0x9b74d8) at main/shaderapi.c:1098
>> > > > > #6  0x77ac8d15 in stub_glLinkProgram (program=2) at
>> > > > > /home/tpalli/source/fdo/piglit/tests/util/piglit-dispatch-
>> > > > > gen.c:33005
>> > > > > #7  0x004019ab in
>> > > > > relink_program_created_by_glCreateShaderProgram () at
>> > > > > /home/tpalli/source/fdo/piglit/tests/spec/arb_separate_shader
>> > > > > _o
>> > > > > bjects
>> > > > > /api-errors.c:78
>> > > >
>> > > > There is a comment above that line in the test.
>> > > >
>> > > > /* Issue #14 of the GL_ARB_separate_shader_objects spec says:
>> > > > *
>> > > >  * "14. Should glLinkProgram work to re-link a shader
>> > > > created
>> > > > with
>> > > >  *  glCreateShaderProgram?
>> > > >  *
>> > > >  *  RESOLVED: NO because the shader created by
>> > > >  *  glCreateShaderProgram is detached and deleted as
>> > > > part
>> > > > of
>> > > >  *  the glCreateShaderProgram sequence.  This means if
>> > > > you
>> > > >  *  call glLinkProgram on a program returned from
>> > > >  *  glCreateShaderProgram, you'll find the re-link
>> > > > fails
>> > > >  *  because no shader object is attached.
>> > > >  *
>> > > >  *  An application is free to attach one or more new
>> > > > shader
>> > > >  *  objects to the program and then relink would work.
>> > > >  *
>> > > >  *  This is fine because re-linking isn't
>> > > > necessary/expected."
>> > > >  */
>> > > >
>> > > > Which means we shouldn't able to relink this shader. We
>> > > > shouldn't
>> > > > be
>> > > > able to get as far along as we do when we get the error
>> > > > message.
>> > >
>> > > Ah right, so this should be detected somewhere within shaderapi,
>> > > will
>> > > take a look there. The reason I suspected we end here with SSO
>> > > was
>> > > that
>> > > this fails just on the old gen models so something definitely
>> > > goes
>> > > different ways with those. But I'll look at shaderapi first.
>> >
>> > Now I realized that I could bail in linker with following:
>> > if (shProg->SeparateShader && shProg->NumShaders == 0)
>> >
>> > BUT following commit states that having NumShaders == 0 is OK for
>> > compatibility profile:
>> >
>> > 76cfb472077dc83c892b4cddf7941deaa7b5
>> >
>> > Timothy, do you think I could still bailout with the condition
>> > above?
>> > I
>> > really do wonder the case mentioned in commit

Re: [Mesa-dev] [PATCH] i965: do not release GLSL IR for SSO programs

2016-10-26 Thread Timothy Arceri 
On Wed, 2016-10-26 at 22:51 +1100, Timothy Arceri wrote:
> On Wed, 2016-10-26 at 13:13 +0300, Tapani Pälli wrote:
> > 
> > Hi;
> > 
> > On 10/26/2016 11:27 AM, Tapani Pälli wrote:
> > > 
> > > 
> > > 
> > > 
> > > On 10/26/2016 11:21 AM, Timothy Arceri wrote:
> > > > 
> > > > 
> > > > On Wed, 2016-10-26 at 08:50 +0300, Tapani Pälli wrote:
> > > > > 
> > > > > 
> > > > > 
> > > > > On 10/26/2016 08:15 AM, Timothy Arceri wrote:
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > On Tue, 2016-10-25 at 09:39 +0300, Tapani Pälli wrote:
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > SSO shader programs can be later modified by
> > > > > > > attaching/detaching
> > > > > > > shaders and relinked, this requires IR.
> > > > > > 
> > > > > > Doesn't relinking recreate the IR? We can relink exiting
> > > > > > shaders
> > > > > > into
> > > > > > new programs. The IR is cloned from gl_shader (the compiled
> > > > > > IR)
> > > > > > before
> > > > > > this happens.
> > > > > > 
> > > > > > Where exactly are things falling over for SSO?
> > > > > 
> > > > > I went this way as I haven't seen this happening elsewhere
> > > > > but
> > > > > when
> > > > > relinking program created by glCreateShaderProgram. TBH I'm
> > > > > not
> > > > > sure
> > > > > if
> > > > > this could happen with regular programs, I would assume we
> > > > > had
> > > > > already
> > > > > bugs if it would be so (?)
> > > > > 
> > > > > In practice things go bad in brw_link_shader where
> > > > > process_glsl_ir()
> > > > > gets called, like this:
> > > > > 
> > > > > --- 8< ---
> > > > > #0  do_expression_flattening (instructions=instructions@entry
> > > > > =0
> > > > > x0,
> > > > > predicate=predicate@entry=0x70f808a0
> > > > > ) at
> > > > > glsl/ir_expression_flattening.cpp:60
> > > > > #1  0x70f816b9 in do_mat_op_to_vec (instructions=0x0)
> > > > > at
> > > > > glsl/lower_mat_op_to_vec.cpp:96
> > > > > #2  0x710385bd in process_glsl_ir
> > > > > (shader_prog=0x9b74d8,
> > > > > shader=0x9b2688, brw=0x7d2478) at brw_link.cpp:108
> > > > > #3  brw_link_shader (ctx=0x7d2478, shProg=0x9b74d8) at
> > > > > brw_link.cpp:234
> > > > > #4  0x70ec1f31 in _mesa_glsl_link_shader
> > > > > (ctx=0x7d2478,
> > > > > prog=0x9b74d8) at program/ir_to_mesa.cpp:3067
> > > > > #5  0x70ddc99b in _mesa_link_program (ctx=0x7d2478,
> > > > > shProg=0x9b74d8) at main/shaderapi.c:1098
> > > > > #6  0x77ac8d15 in stub_glLinkProgram (program=2) at
> > > > > /home/tpalli/source/fdo/piglit/tests/util/piglit-dispatch-
> > > > > gen.c:33005
> > > > > #7  0x004019ab in
> > > > > relink_program_created_by_glCreateShaderProgram () at
> > > > > /home/tpalli/source/fdo/piglit/tests/spec/arb_separate_shader
> > > > > _o
> > > > > bjects
> > > > > /api-errors.c:78
> > > > 
> > > > There is a comment above that line in the test.
> > > > 
> > > > /* Issue #14 of the GL_ARB_separate_shader_objects spec says:
> > > > *
> > > >  * "14. Should glLinkProgram work to re-link a shader
> > > > created
> > > > with
> > > >  *  glCreateShaderProgram?
> > > >  *
> > > >  *  RESOLVED: NO because the shader created by
> > > >  *  glCreateShaderProgram is detached and deleted as
> > > > part
> > > > of
> > > >  *  the glCreateShaderProgram sequence.  This means if
> > > > you
> > > >  *  call glLinkProgram on a program returned from
> > > >  *  glCreateShaderProgram, you'll find the re-link
> > > > fails
> > > >  *  because no shader object is attached.
> > > >  *
> > > >  *  An application is free to attach one or more new
> > > > shader
> > > >  *  objects to the program and then relink would work.
> > > >  *
> > > >  *  This is fine because re-linking isn't
> > > > necessary/expected."
> > > >  */
> > > > 
> > > > Which means we shouldn't able to relink this shader. We
> > > > shouldn't
> > > > be
> > > > able to get as far along as we do when we get the error
> > > > message.
> > > 
> > > Ah right, so this should be detected somewhere within shaderapi,
> > > will
> > > take a look there. The reason I suspected we end here with SSO
> > > was
> > > that
> > > this fails just on the old gen models so something definitely
> > > goes
> > > different ways with those. But I'll look at shaderapi first.
> > 
> > Now I realized that I could bail in linker with following:
> > if (shProg->SeparateShader && shProg->NumShaders == 0)
> > 
> > BUT following commit states that having NumShaders == 0 is OK for 
> > compatibility profile:
> > 
> > 76cfb472077dc83c892b4cddf7941deaa7b5
> > 
> > Timothy, do you think I could still bailout with the condition
> > above?
> > I 
> > really do wonder the case mentioned in commit where someone links
> > a 
> > shader without any stages, does this really make sense (what is the
> > use 
> > case) or is this allowed just because spec does not happen to
> > prohibit this?
> 
>

Re: [Mesa-dev] [PATCH] i965: do not release GLSL IR for SSO programs

2016-10-26 Thread Timothy Arceri 
On Wed, 2016-10-26 at 13:13 +0300, Tapani Pälli wrote:
> Hi;
> 
> On 10/26/2016 11:27 AM, Tapani Pälli wrote:
> > 
> > 
> > 
> > On 10/26/2016 11:21 AM, Timothy Arceri wrote:
> > > 
> > > On Wed, 2016-10-26 at 08:50 +0300, Tapani Pälli wrote:
> > > > 
> > > > 
> > > > On 10/26/2016 08:15 AM, Timothy Arceri wrote:
> > > > > 
> > > > > 
> > > > > On Tue, 2016-10-25 at 09:39 +0300, Tapani Pälli wrote:
> > > > > > 
> > > > > > 
> > > > > > SSO shader programs can be later modified by
> > > > > > attaching/detaching
> > > > > > shaders and relinked, this requires IR.
> > > > > 
> > > > > Doesn't relinking recreate the IR? We can relink exiting
> > > > > shaders
> > > > > into
> > > > > new programs. The IR is cloned from gl_shader (the compiled
> > > > > IR)
> > > > > before
> > > > > this happens.
> > > > > 
> > > > > Where exactly are things falling over for SSO?
> > > > 
> > > > I went this way as I haven't seen this happening elsewhere but
> > > > when
> > > > relinking program created by glCreateShaderProgram. TBH I'm not
> > > > sure
> > > > if
> > > > this could happen with regular programs, I would assume we had
> > > > already
> > > > bugs if it would be so (?)
> > > > 
> > > > In practice things go bad in brw_link_shader where
> > > > process_glsl_ir()
> > > > gets called, like this:
> > > > 
> > > > --- 8< ---
> > > > #0  do_expression_flattening (instructions=instructions@entry=0
> > > > x0,
> > > > predicate=predicate@entry=0x70f808a0
> > > > ) at
> > > > glsl/ir_expression_flattening.cpp:60
> > > > #1  0x70f816b9 in do_mat_op_to_vec (instructions=0x0)
> > > > at
> > > > glsl/lower_mat_op_to_vec.cpp:96
> > > > #2  0x710385bd in process_glsl_ir
> > > > (shader_prog=0x9b74d8,
> > > > shader=0x9b2688, brw=0x7d2478) at brw_link.cpp:108
> > > > #3  brw_link_shader (ctx=0x7d2478, shProg=0x9b74d8) at
> > > > brw_link.cpp:234
> > > > #4  0x70ec1f31 in _mesa_glsl_link_shader (ctx=0x7d2478,
> > > > prog=0x9b74d8) at program/ir_to_mesa.cpp:3067
> > > > #5  0x70ddc99b in _mesa_link_program (ctx=0x7d2478,
> > > > shProg=0x9b74d8) at main/shaderapi.c:1098
> > > > #6  0x77ac8d15 in stub_glLinkProgram (program=2) at
> > > > /home/tpalli/source/fdo/piglit/tests/util/piglit-dispatch-
> > > > gen.c:33005
> > > > #7  0x004019ab in
> > > > relink_program_created_by_glCreateShaderProgram () at
> > > > /home/tpalli/source/fdo/piglit/tests/spec/arb_separate_shader_o
> > > > bjects
> > > > /api-errors.c:78
> > > 
> > > There is a comment above that line in the test.
> > > 
> > > /* Issue #14 of the GL_ARB_separate_shader_objects spec says:
> > > *
> > >  * "14. Should glLinkProgram work to re-link a shader created
> > > with
> > >  *  glCreateShaderProgram?
> > >  *
> > >  *  RESOLVED: NO because the shader created by
> > >  *  glCreateShaderProgram is detached and deleted as part
> > > of
> > >  *  the glCreateShaderProgram sequence.  This means if
> > > you
> > >  *  call glLinkProgram on a program returned from
> > >  *  glCreateShaderProgram, you'll find the re-link fails
> > >  *  because no shader object is attached.
> > >  *
> > >  *  An application is free to attach one or more new
> > > shader
> > >  *  objects to the program and then relink would work.
> > >  *
> > >  *  This is fine because re-linking isn't
> > > necessary/expected."
> > >  */
> > > 
> > > Which means we shouldn't able to relink this shader. We shouldn't
> > > be
> > > able to get as far along as we do when we get the error message.
> > 
> > Ah right, so this should be detected somewhere within shaderapi,
> > will
> > take a look there. The reason I suspected we end here with SSO was
> > that
> > this fails just on the old gen models so something definitely goes
> > different ways with those. But I'll look at shaderapi first.
> 
> Now I realized that I could bail in linker with following:
> if (shProg->SeparateShader && shProg->NumShaders == 0)
> 
> BUT following commit states that having NumShaders == 0 is OK for 
> compatibility profile:
> 
> 76cfb472077dc83c892b4cddf7941deaa7b5
> 
> Timothy, do you think I could still bailout with the condition above?
> I 
> really do wonder the case mentioned in commit where someone links a 
> shader without any stages, does this really make sense (what is the
> use 
> case) or is this allowed just because spec does not happen to
> prohibit this?

ok, I see. The test says it *should* relink when in compat profile. In
which case it should fall back to fixed-function. I'm not entirely sure
what path things take from here but it seems something is not working
correctly when creating the fallback shader.


> 
> 
> > 
> > 
> > 
> > > 
> > > > 
> > > > 
> > > > 
> > > > > 
> > > > > 
> > > > > > 
> > > > > > 
> > > > > >  This patch fixes regression
> > > > > > caused by 4542c7ed5fc6d8cb2495d322b4f06d802d7292cc.
> > > > > > 
> > > > >

Re: [Mesa-dev] [PATCH] i965: do not release GLSL IR for SSO programs

2016-10-26 Thread Tapani Pälli 

Hi;

On 10/26/2016 11:27 AM, Tapani Pälli wrote:



On 10/26/2016 11:21 AM, Timothy Arceri wrote:

On Wed, 2016-10-26 at 08:50 +0300, Tapani Pälli wrote:


On 10/26/2016 08:15 AM, Timothy Arceri wrote:


On Tue, 2016-10-25 at 09:39 +0300, Tapani Pälli wrote:


SSO shader programs can be later modified by attaching/detaching
shaders and relinked, this requires IR.


Doesn't relinking recreate the IR? We can relink exiting shaders
into
new programs. The IR is cloned from gl_shader (the compiled IR)
before
this happens.

Where exactly are things falling over for SSO?


I went this way as I haven't seen this happening elsewhere but when
relinking program created by glCreateShaderProgram. TBH I'm not sure
if
this could happen with regular programs, I would assume we had
already
bugs if it would be so (?)

In practice things go bad in brw_link_shader where process_glsl_ir()
gets called, like this:

--- 8< ---
#0  do_expression_flattening (instructions=instructions@entry=0x0,
predicate=predicate@entry=0x70f808a0
) at
glsl/ir_expression_flattening.cpp:60
#1  0x70f816b9 in do_mat_op_to_vec (instructions=0x0) at
glsl/lower_mat_op_to_vec.cpp:96
#2  0x710385bd in process_glsl_ir (shader_prog=0x9b74d8,
shader=0x9b2688, brw=0x7d2478) at brw_link.cpp:108
#3  brw_link_shader (ctx=0x7d2478, shProg=0x9b74d8) at
brw_link.cpp:234
#4  0x70ec1f31 in _mesa_glsl_link_shader (ctx=0x7d2478,
prog=0x9b74d8) at program/ir_to_mesa.cpp:3067
#5  0x70ddc99b in _mesa_link_program (ctx=0x7d2478,
shProg=0x9b74d8) at main/shaderapi.c:1098
#6  0x77ac8d15 in stub_glLinkProgram (program=2) at
/home/tpalli/source/fdo/piglit/tests/util/piglit-dispatch-gen.c:33005
#7  0x004019ab in
relink_program_created_by_glCreateShaderProgram () at
/home/tpalli/source/fdo/piglit/tests/spec/arb_separate_shader_objects
/api-errors.c:78


There is a comment above that line in the test.

/* Issue #14 of the GL_ARB_separate_shader_objects spec says:
*
 * "14. Should glLinkProgram work to re-link a shader created with
 *  glCreateShaderProgram?
 *
 *  RESOLVED: NO because the shader created by
 *  glCreateShaderProgram is detached and deleted as part of
 *  the glCreateShaderProgram sequence.  This means if you
 *  call glLinkProgram on a program returned from
 *  glCreateShaderProgram, you'll find the re-link fails
 *  because no shader object is attached.
 *
 *  An application is free to attach one or more new shader
 *  objects to the program and then relink would work.
 *
 *  This is fine because re-linking isn't necessary/expected."
 */

Which means we shouldn't able to relink this shader. We shouldn't be
able to get as far along as we do when we get the error message.


Ah right, so this should be detected somewhere within shaderapi, will
take a look there. The reason I suspected we end here with SSO was that
this fails just on the old gen models so something definitely goes
different ways with those. But I'll look at shaderapi first.


Now I realized that I could bail in linker with following:
if (shProg->SeparateShader && shProg->NumShaders == 0)

BUT following commit states that having NumShaders == 0 is OK for 
compatibility profile:


76cfb472077dc83c892b4cddf7941deaa7b5

Timothy, do you think I could still bailout with the condition above? I 
really do wonder the case mentioned in commit where someone links a 
shader without any stages, does this really make sense (what is the use 
case) or is this allowed just because spec does not happen to prohibit this?













 This patch fixes regression
caused by 4542c7ed5fc6d8cb2495d322b4f06d802d7292cc.

Signed-off-by: Tapani Pälli 
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97715
Cc: "12.0 13.0" 
---
 src/mesa/drivers/dri/i965/brw_link.cpp | 17 ++---
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/src/mesa/drivers/dri/i965/brw_link.cpp
b/src/mesa/drivers/dri/i965/brw_link.cpp
index 5ea9773..ffb66a9 100644
--- a/src/mesa/drivers/dri/i965/brw_link.cpp
+++ b/src/mesa/drivers/dri/i965/brw_link.cpp
@@ -290,14 +290,17 @@ brw_link_shader(struct gl_context *ctx,
struct
gl_shader_program *shProg)

build_program_resource_list(ctx, shProg);

-   for (stage = 0; stage < ARRAY_SIZE(shProg->_LinkedShaders);
stage++) {
-  struct gl_linked_shader *shader = shProg-


_LinkedShaders[stage];

-  if (!shader)
- continue;
+   /* We can't free IR for SSO programs since those may need
relinking. */
+   if (!shProg->SeparateShader) {
+  for (stage = 0; stage < ARRAY_SIZE(shProg-

_LinkedShaders);

stage++) {
+ struct gl_linked_shader *shader = shProg-


_LinkedShaders[stage];

+ if (!shader)
+continue;

-  /* The GLSL IR won't be needed anymore. */
-  ralloc_free(shader->ir);
-  shader->ir =

Re: [Mesa-dev] [PATCH] i965: do not release GLSL IR for SSO programs

2016-10-26 Thread Tapani Pälli 



On 10/26/2016 11:21 AM, Timothy Arceri wrote:

On Wed, 2016-10-26 at 08:50 +0300, Tapani Pälli wrote:


On 10/26/2016 08:15 AM, Timothy Arceri wrote:


On Tue, 2016-10-25 at 09:39 +0300, Tapani Pälli wrote:


SSO shader programs can be later modified by attaching/detaching
shaders and relinked, this requires IR.


Doesn't relinking recreate the IR? We can relink exiting shaders
into
new programs. The IR is cloned from gl_shader (the compiled IR)
before
this happens.

Where exactly are things falling over for SSO?


I went this way as I haven't seen this happening elsewhere but when
relinking program created by glCreateShaderProgram. TBH I'm not sure
if
this could happen with regular programs, I would assume we had
already
bugs if it would be so (?)

In practice things go bad in brw_link_shader where process_glsl_ir()
gets called, like this:

--- 8< ---
#0  do_expression_flattening (instructions=instructions@entry=0x0,
predicate=predicate@entry=0x70f808a0
) at
glsl/ir_expression_flattening.cpp:60
#1  0x70f816b9 in do_mat_op_to_vec (instructions=0x0) at
glsl/lower_mat_op_to_vec.cpp:96
#2  0x710385bd in process_glsl_ir (shader_prog=0x9b74d8,
shader=0x9b2688, brw=0x7d2478) at brw_link.cpp:108
#3  brw_link_shader (ctx=0x7d2478, shProg=0x9b74d8) at
brw_link.cpp:234
#4  0x70ec1f31 in _mesa_glsl_link_shader (ctx=0x7d2478,
prog=0x9b74d8) at program/ir_to_mesa.cpp:3067
#5  0x70ddc99b in _mesa_link_program (ctx=0x7d2478,
shProg=0x9b74d8) at main/shaderapi.c:1098
#6  0x77ac8d15 in stub_glLinkProgram (program=2) at
/home/tpalli/source/fdo/piglit/tests/util/piglit-dispatch-gen.c:33005
#7  0x004019ab in
relink_program_created_by_glCreateShaderProgram () at
/home/tpalli/source/fdo/piglit/tests/spec/arb_separate_shader_objects
/api-errors.c:78


There is a comment above that line in the test.

/* Issue #14 of the GL_ARB_separate_shader_objects spec says:
*
 * "14. Should glLinkProgram work to re-link a shader created with
 *  glCreateShaderProgram?
 *
 *  RESOLVED: NO because the shader created by
 *  glCreateShaderProgram is detached and deleted as part of
 *  the glCreateShaderProgram sequence.  This means if you
 *  call glLinkProgram on a program returned from
 *  glCreateShaderProgram, you'll find the re-link fails
 *  because no shader object is attached.
 *
 *  An application is free to attach one or more new shader
 *  objects to the program and then relink would work.
 *
 *  This is fine because re-linking isn't necessary/expected."
 */

Which means we shouldn't able to relink this shader. We shouldn't be
able to get as far along as we do when we get the error message.


Ah right, so this should be detected somewhere within shaderapi, will 
take a look there. The reason I suspected we end here with SSO was that 
this fails just on the old gen models so something definitely goes 
different ways with those. But I'll look at shaderapi first.











 This patch fixes regression
caused by 4542c7ed5fc6d8cb2495d322b4f06d802d7292cc.

Signed-off-by: Tapani Pälli 
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97715
Cc: "12.0 13.0" 
---
 src/mesa/drivers/dri/i965/brw_link.cpp | 17 ++---
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/src/mesa/drivers/dri/i965/brw_link.cpp
b/src/mesa/drivers/dri/i965/brw_link.cpp
index 5ea9773..ffb66a9 100644
--- a/src/mesa/drivers/dri/i965/brw_link.cpp
+++ b/src/mesa/drivers/dri/i965/brw_link.cpp
@@ -290,14 +290,17 @@ brw_link_shader(struct gl_context *ctx,
struct
gl_shader_program *shProg)

build_program_resource_list(ctx, shProg);

-   for (stage = 0; stage < ARRAY_SIZE(shProg->_LinkedShaders);
stage++) {
-  struct gl_linked_shader *shader = shProg-


_LinkedShaders[stage];

-  if (!shader)
- continue;
+   /* We can't free IR for SSO programs since those may need
relinking. */
+   if (!shProg->SeparateShader) {
+  for (stage = 0; stage < ARRAY_SIZE(shProg-

_LinkedShaders);

stage++) {
+ struct gl_linked_shader *shader = shProg-


_LinkedShaders[stage];

+ if (!shader)
+continue;

-  /* The GLSL IR won't be needed anymore. */
-  ralloc_free(shader->ir);
-  shader->ir = NULL;
+ /* The GLSL IR won't be needed anymore. */
+ ralloc_free(shader->ir);
+ shader->ir = NULL;
+  }
}

return true;

___
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

Re: [Mesa-dev] [PATCH] i965: do not release GLSL IR for SSO programs

2016-10-26 Thread Timothy Arceri 
On Wed, 2016-10-26 at 08:50 +0300, Tapani Pälli wrote:
> 
> On 10/26/2016 08:15 AM, Timothy Arceri wrote:
> > 
> > On Tue, 2016-10-25 at 09:39 +0300, Tapani Pälli wrote:
> > > 
> > > SSO shader programs can be later modified by attaching/detaching
> > > shaders and relinked, this requires IR.
> > 
> > Doesn't relinking recreate the IR? We can relink exiting shaders
> > into
> > new programs. The IR is cloned from gl_shader (the compiled IR)
> > before
> > this happens.
> > 
> > Where exactly are things falling over for SSO?
> 
> I went this way as I haven't seen this happening elsewhere but when 
> relinking program created by glCreateShaderProgram. TBH I'm not sure
> if 
> this could happen with regular programs, I would assume we had
> already 
> bugs if it would be so (?)
> 
> In practice things go bad in brw_link_shader where process_glsl_ir() 
> gets called, like this:
> 
> --- 8< ---
> #0  do_expression_flattening (instructions=instructions@entry=0x0, 
> predicate=predicate@entry=0x70f808a0 
> ) at 
> glsl/ir_expression_flattening.cpp:60
> #1  0x70f816b9 in do_mat_op_to_vec (instructions=0x0) at 
> glsl/lower_mat_op_to_vec.cpp:96
> #2  0x710385bd in process_glsl_ir (shader_prog=0x9b74d8, 
> shader=0x9b2688, brw=0x7d2478) at brw_link.cpp:108
> #3  brw_link_shader (ctx=0x7d2478, shProg=0x9b74d8) at
> brw_link.cpp:234
> #4  0x70ec1f31 in _mesa_glsl_link_shader (ctx=0x7d2478, 
> prog=0x9b74d8) at program/ir_to_mesa.cpp:3067
> #5  0x70ddc99b in _mesa_link_program (ctx=0x7d2478, 
> shProg=0x9b74d8) at main/shaderapi.c:1098
> #6  0x77ac8d15 in stub_glLinkProgram (program=2) at 
> /home/tpalli/source/fdo/piglit/tests/util/piglit-dispatch-gen.c:33005
> #7  0x004019ab in 
> relink_program_created_by_glCreateShaderProgram () at 
> /home/tpalli/source/fdo/piglit/tests/spec/arb_separate_shader_objects
> /api-errors.c:78

There is a comment above that line in the test.

/* Issue #14 of the GL_ARB_separate_shader_objects spec says:
*
 * "14. Should glLinkProgram work to re-link a shader created with
 *  glCreateShaderProgram?
 *
 *  RESOLVED: NO because the shader created by
 *  glCreateShaderProgram is detached and deleted as part of
 *  the glCreateShaderProgram sequence.  This means if you
 *  call glLinkProgram on a program returned from
 *  glCreateShaderProgram, you'll find the re-link fails
 *  because no shader object is attached.
 *
 *  An application is free to attach one or more new shader
 *  objects to the program and then relink would work.
 *
 *  This is fine because re-linking isn't necessary/expected."
 */

Which means we shouldn't able to relink this shader. We shouldn't be
able to get as far along as we do when we get the error message.

> 
> 
> > 
> > > 
> > >  This patch fixes regression
> > > caused by 4542c7ed5fc6d8cb2495d322b4f06d802d7292cc.
> > > 
> > > Signed-off-by: Tapani Pälli 
> > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97715
> > > Cc: "12.0 13.0" 
> > > ---
> > >  src/mesa/drivers/dri/i965/brw_link.cpp | 17 ++---
> > >  1 file changed, 10 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/src/mesa/drivers/dri/i965/brw_link.cpp
> > > b/src/mesa/drivers/dri/i965/brw_link.cpp
> > > index 5ea9773..ffb66a9 100644
> > > --- a/src/mesa/drivers/dri/i965/brw_link.cpp
> > > +++ b/src/mesa/drivers/dri/i965/brw_link.cpp
> > > @@ -290,14 +290,17 @@ brw_link_shader(struct gl_context *ctx,
> > > struct
> > > gl_shader_program *shProg)
> > > 
> > > build_program_resource_list(ctx, shProg);
> > > 
> > > -   for (stage = 0; stage < ARRAY_SIZE(shProg->_LinkedShaders);
> > > stage++) {
> > > -  struct gl_linked_shader *shader = shProg-
> > > > 
> > > > _LinkedShaders[stage];
> > > -  if (!shader)
> > > - continue;
> > > +   /* We can't free IR for SSO programs since those may need
> > > relinking. */
> > > +   if (!shProg->SeparateShader) {
> > > +  for (stage = 0; stage < ARRAY_SIZE(shProg-
> > > >_LinkedShaders);
> > > stage++) {
> > > + struct gl_linked_shader *shader = shProg-
> > > > 
> > > > _LinkedShaders[stage];
> > > + if (!shader)
> > > +continue;
> > > 
> > > -  /* The GLSL IR won't be needed anymore. */
> > > -  ralloc_free(shader->ir);
> > > -  shader->ir = NULL;
> > > + /* The GLSL IR won't be needed anymore. */
> > > + ralloc_free(shader->ir);
> > > + shader->ir = NULL;
> > > +  }
> > > }
> > > 
> > > return true;
> ___
> 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

Re: [Mesa-dev] [PATCH] i965: do not release GLSL IR for SSO programs

2016-10-25 Thread Tapani Pälli 



On 10/26/2016 08:15 AM, Timothy Arceri wrote:

On Tue, 2016-10-25 at 09:39 +0300, Tapani Pälli wrote:

SSO shader programs can be later modified by attaching/detaching
shaders and relinked, this requires IR.


Doesn't relinking recreate the IR? We can relink exiting shaders into
new programs. The IR is cloned from gl_shader (the compiled IR) before
this happens.

Where exactly are things falling over for SSO?


I went this way as I haven't seen this happening elsewhere but when 
relinking program created by glCreateShaderProgram. TBH I'm not sure if 
this could happen with regular programs, I would assume we had already 
bugs if it would be so (?)


In practice things go bad in brw_link_shader where process_glsl_ir() 
gets called, like this:


--- 8< ---
#0  do_expression_flattening (instructions=instructions@entry=0x0, 
predicate=predicate@entry=0x70f808a0 
) at 
glsl/ir_expression_flattening.cpp:60
#1  0x70f816b9 in do_mat_op_to_vec (instructions=0x0) at 
glsl/lower_mat_op_to_vec.cpp:96
#2  0x710385bd in process_glsl_ir (shader_prog=0x9b74d8, 
shader=0x9b2688, brw=0x7d2478) at brw_link.cpp:108

#3  brw_link_shader (ctx=0x7d2478, shProg=0x9b74d8) at brw_link.cpp:234
#4  0x70ec1f31 in _mesa_glsl_link_shader (ctx=0x7d2478, 
prog=0x9b74d8) at program/ir_to_mesa.cpp:3067
#5  0x70ddc99b in _mesa_link_program (ctx=0x7d2478, 
shProg=0x9b74d8) at main/shaderapi.c:1098
#6  0x77ac8d15 in stub_glLinkProgram (program=2) at 
/home/tpalli/source/fdo/piglit/tests/util/piglit-dispatch-gen.c:33005
#7  0x004019ab in 
relink_program_created_by_glCreateShaderProgram () at 
/home/tpalli/source/fdo/piglit/tests/spec/arb_separate_shader_objects/api-errors.c:78




 This patch fixes regression
caused by 4542c7ed5fc6d8cb2495d322b4f06d802d7292cc.

Signed-off-by: Tapani Pälli 
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97715
Cc: "12.0 13.0" 
---
 src/mesa/drivers/dri/i965/brw_link.cpp | 17 ++---
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/src/mesa/drivers/dri/i965/brw_link.cpp
b/src/mesa/drivers/dri/i965/brw_link.cpp
index 5ea9773..ffb66a9 100644
--- a/src/mesa/drivers/dri/i965/brw_link.cpp
+++ b/src/mesa/drivers/dri/i965/brw_link.cpp
@@ -290,14 +290,17 @@ brw_link_shader(struct gl_context *ctx, struct
gl_shader_program *shProg)

build_program_resource_list(ctx, shProg);

-   for (stage = 0; stage < ARRAY_SIZE(shProg->_LinkedShaders);
stage++) {
-  struct gl_linked_shader *shader = shProg-

_LinkedShaders[stage];

-  if (!shader)
- continue;
+   /* We can't free IR for SSO programs since those may need
relinking. */
+   if (!shProg->SeparateShader) {
+  for (stage = 0; stage < ARRAY_SIZE(shProg->_LinkedShaders);
stage++) {
+ struct gl_linked_shader *shader = shProg-

_LinkedShaders[stage];

+ if (!shader)
+continue;

-  /* The GLSL IR won't be needed anymore. */
-  ralloc_free(shader->ir);
-  shader->ir = NULL;
+ /* The GLSL IR won't be needed anymore. */
+ ralloc_free(shader->ir);
+ shader->ir = NULL;
+  }
}

return true;

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

Re: [Mesa-dev] [PATCH] i965: do not release GLSL IR for SSO programs

2016-10-25 Thread Timothy Arceri 
On Tue, 2016-10-25 at 09:39 +0300, Tapani Pälli wrote:
> SSO shader programs can be later modified by attaching/detaching
> shaders and relinked, this requires IR.

Doesn't relinking recreate the IR? We can relink exiting shaders into
new programs. The IR is cloned from gl_shader (the compiled IR) before
this happens.

Where exactly are things falling over for SSO?

>  This patch fixes regression
> caused by 4542c7ed5fc6d8cb2495d322b4f06d802d7292cc.
> 
> Signed-off-by: Tapani Pälli 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97715
> Cc: "12.0 13.0" 
> ---
>  src/mesa/drivers/dri/i965/brw_link.cpp | 17 ++---
>  1 file changed, 10 insertions(+), 7 deletions(-)
> 
> diff --git a/src/mesa/drivers/dri/i965/brw_link.cpp
> b/src/mesa/drivers/dri/i965/brw_link.cpp
> index 5ea9773..ffb66a9 100644
> --- a/src/mesa/drivers/dri/i965/brw_link.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_link.cpp
> @@ -290,14 +290,17 @@ brw_link_shader(struct gl_context *ctx, struct
> gl_shader_program *shProg)
>  
> build_program_resource_list(ctx, shProg);
>  
> -   for (stage = 0; stage < ARRAY_SIZE(shProg->_LinkedShaders);
> stage++) {
> -  struct gl_linked_shader *shader = shProg-
> >_LinkedShaders[stage];
> -  if (!shader)
> - continue;
> +   /* We can't free IR for SSO programs since those may need
> relinking. */
> +   if (!shProg->SeparateShader) {
> +  for (stage = 0; stage < ARRAY_SIZE(shProg->_LinkedShaders);
> stage++) {
> + struct gl_linked_shader *shader = shProg-
> >_LinkedShaders[stage];
> + if (!shader)
> +continue;
>  
> -  /* The GLSL IR won't be needed anymore. */
> -  ralloc_free(shader->ir);
> -  shader->ir = NULL;
> + /* The GLSL IR won't be needed anymore. */
> + ralloc_free(shader->ir);
> + shader->ir = NULL;
> +  }
> }
>  
> return true;
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev