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 > > <timothy.arc...@collabora.com> 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=0x7ffff0f808a0 > > > > > > > > <mat_op_to_vec_predicate(ir_instruction*)>) at > > > > > > > > glsl/ir_expression_flattening.cpp:60 > > > > > > > > #1 0x00007ffff0f816b9 in do_mat_op_to_vec > > > > > > > > (instructions=0x0) > > > > > > > > at > > > > > > > > glsl/lower_mat_op_to_vec.cpp:96 > > > > > > > > #2 0x00007ffff10385bd 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 0x00007ffff0ec1f31 in _mesa_glsl_link_shader > > > > > > > > (ctx=0x7d2478, > > > > > > > > prog=0x9b74d8) at program/ir_to_mesa.cpp:3067 > > > > > > > > #5 0x00007ffff0ddc99b in _mesa_link_program > > > > > > > > (ctx=0x7d2478, > > > > > > > > shProg=0x9b74d8) at main/shaderapi.c:1098 > > > > > > > > #6 0x00007ffff7ac8d15 in stub_glLinkProgram > > > > > > > > (program=2) > > > > > > > > at > > > > > > > > /home/tpalli/source/fdo/piglit/tests/util/piglit- > > > > > > > > dispatch- > > > > > > > > gen.c:33005 > > > > > > > > #7 0x00000000004019ab 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. > > > > > > > > > > > > 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: > > > > > > > > > > 76cfb472077dc83c892b4cddf79333341deaa7b5 > > > > > > > > > > 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. > > > > > > Actually we still return early it just shouldn't throw an error. > > > > > > > > > > > > > > > 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. > > > > > > I've taken a closer look. We don't do anything about creating > > > fallback > > > shaders in GLSL IR so we shouldn't be hitting the code where the > > > error > > > happens. > > > > > > I believe the problem is in glDetachShader() we remove the > > > gl_shader > > > but we leave gl_linked_shader in place. > > > > AFAIK that's a valid sequence - glAttachShader(); glLinkProgram(); > > glDetachShader(); glDeleteShader(); - the program should still be > > usable. > > 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. Maybe we could put this out there as a beginner/easy task for someone to do. IMO what we should do is turn gl_linked_shader into a temporary structure that holds stuff we need during linking (IR, packed_varyings list, etc) and release it after linking. The result would be much more consistent code, currently we seem to just randomly grab/store things from/to gl_linked_shader or gl_program without much thought. The spec states: "If a program object that is active for any shader stage is re-linked unsuccessfully, the link status will be set to FALSE, but any existing executables and associated state will remain part of the current rendering state until a subsequent call to UseProgram, UseProgramStages, or BindProgramPipeline removes them from use." And since we destroy the previous gl_linked_shader when relinking we shouldn't be storing anything related to state in here anyway, since we will end up a broken state if linking fails. Freeing gl_linked_program will help us avoid this mistake in future. We would also probably want to move the pointer out to gl_program out of gl_linked_shader and instead have an array like _LinkedShader in gl_shader_program for gl_program. > > > > > > > -ilia > > _______________________________________________ > > 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