[Mesa-dev] [PATCH] i965: do not release GLSL IR for SSO programs
Tapani Pälli
tapani.palli at intel.com
Wed Oct 26 10:13:41 UTC 2016
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 at entry=0x0,
>>> predicate=predicate at 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_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:
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?
>
>
>>>
>>>
>>>>
>>>>>
>>>>> This patch fixes regression
>>>>> caused by 4542c7ed5fc6d8cb2495d322b4f06d802d7292cc.
>>>>>
>>>>> Signed-off-by: Tapani Pälli <tapani.palli at intel.com>
>>>>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97715
>>>>> Cc: "12.0 13.0" <mesa-stable at lists.freedesktop.org>
>>>>> ---
>>>>> 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 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
More information about the mesa-dev
mailing list