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

[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 at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev