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

[Timothy Arceri]

On 1 November 2016 6:00:46 pm AEDT, Timothy Arceri <timothy.arceri at collabora.com> 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 at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>_______________________________________________
>mesa-dev mailing list
>mesa-dev at lists.freedesktop.org
>https://lists.freedesktop.org/mailman/listinfo/mesa-dev

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.