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. > > -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