On Tue, May 20, 2014 at 2:36 PM, Kenneth Graunke <kenn...@whitecape.org> wrote:
> On 05/20/2014 10:49 AM, Pohjolainen, Topi wrote:
>> On Tue, May 20, 2014 at 10:31:05AM -0700, Anuj Phogat wrote:
>>> _mesa_meta_setup_blit_shader() currently generates a fragment shader
>>> which, irrespective of the number of draw buffers, writes the color
>>> to only one 'out' variable. Current shader rely on an undefined
>>> behavior and possibly works by chance.
>>>
>>> From OpenGL 4.0 spec, page 256:
>>> "If a fragment shader writes to gl_FragColor, DrawBuffers specifies a
>>> set of draw buffers into which the single fragment color defined by
>>> gl_FragColor is written. If a fragment shader writes to gl_FragData,
>>> or a user-defined varying out variable, DrawBuffers specifies a set
>>> of draw buffers into which each of the multiple output colors defined
>>> by these variables are separately written. If a fragment shader writes
>>> to none of gl_FragColor, gl_FragData, nor any user defined varying out
>>> variables, the values of the fragment colors following shader execution
>>> are undefined, and may differ for each fragment color."
>>>
>>> OpenGL 4.4 spec, page 463, added an additional line in this section:
>>> "If some, but not all user-defined output variables are written, the
>>> values of fragment colors corresponding to unwritten variables are
>>> similarly undefined."
>>>
>>> V2: Write color output to gl_FragColor instead of writing to multiple
>>> 'out' variables. This'll avoid recompiling the shader every time
>>> draw buffers count is updated.
>>>
>>> Cc: <mesa-sta...@lists.freedesktop.org>
>>> Signed-off-by: Anuj Phogat <anuj.pho...@gmail.com>
>>> Reviewed-by: Matt Turner <matts...@gmail.com> (V1)
>>
>> Fixes gles3 cts tests:
>>
>> framebuffer_blit_functionality_color_and_depth_blit.test
>> framebuffer_blit_functionality_nearest_filter_color_blit.test
>> framebuffer_blit_functionality_linear_filter_color_blit.test
>> framebuffer_blit_functionality_color_and_stencil_blit.test
>>
>>> ---
>>> src/mesa/drivers/common/meta.c | 15 ++++-----------
>>> 1 file changed, 4 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/src/mesa/drivers/common/meta.c b/src/mesa/drivers/common/meta.c
>>> index fab9106..337be1b 100644
>>> --- a/src/mesa/drivers/common/meta.c
>>> +++ b/src/mesa/drivers/common/meta.c
>>> @@ -246,7 +246,6 @@ _mesa_meta_setup_blit_shader(struct gl_context *ctx,
>>> void *const mem_ctx = ralloc_context(NULL);
>>> struct blit_shader *shader = choose_blit_shader(target, table);
>>> const char *vs_input, *vs_output, *fs_input, *vs_preprocess,
>>> *fs_preprocess;
>>> - const char *fs_output_var, *fs_output_var_decl;
>>>
>>> if (ctx->Const.GLSLVersion < 130) {
>>> vs_preprocess = "";
>>> @@ -254,16 +253,12 @@ _mesa_meta_setup_blit_shader(struct gl_context *ctx,
>>> vs_output = "varying";
>>> fs_preprocess = "#extension GL_EXT_texture_array : enable";
>>> fs_input = "varying";
>>> - fs_output_var_decl = "";
>>> - fs_output_var = "gl_FragColor";
>>> } else {
>>> vs_preprocess = "#version 130";
>>> vs_input = "in";
>>> vs_output = "out";
>>> fs_preprocess = "#version 130";
>>> fs_input = "in";
>>> - fs_output_var_decl = "out vec4 out_color;";
>>> - fs_output_var = "out_color";
>>> shader->func = "texture";
>>> }
>>>
>>> @@ -291,15 +286,13 @@ _mesa_meta_setup_blit_shader(struct gl_context *ctx,
>>> "#extension GL_ARB_texture_cube_map_array: enable\n"
>>> "uniform %s texSampler;\n"
>>> "%s vec4 texCoords;\n"
>>> - "%s\n"
>>> "void main()\n"
>>> "{\n"
>>> - " vec4 color = %s(texSampler, %s);\n"
>>> - " %s = color;\n"
>>> - " gl_FragDepth = color.x;\n"
>>> + " gl_FragColor = %s(texSampler, %s);\n"
>>> + " gl_FragDepth = gl_FragColor.x;\n"
>>> "}\n",
>>> - fs_preprocess, shader->type, fs_input, fs_output_decl,
>>> - shader->func, shader->texcoords, fs_output);
>>> + fs_preprocess, shader->type, fs_input,
>>> + shader->func, shader->texcoords);
>
> Assuming you fix this line (as Topi pointed out), both patches are:
Fixed.
> Reviewed-by: Kenneth Graunke <kenn...@whitecape.org>
>
> Thanks for fixing this, Anuj.
>
>>>
>>> _mesa_meta_compile_and_link_program(ctx, vs_source, fs_source,
>>> ralloc_asprintf(mem_ctx, "%s blit",
>
> Topi and I spent a while analyzing why we saw failures with this code on
> Broadwell, but not on earlier platforms.
>
> Using gl_FragColor causes OutputsWritten to include FRAG_RESULT_COLOR,
> while using "out vec4 out_color" causes it to include only
> FRAG_RESULT_DATA0 (and not 1/2/3 for the other targets).
>
> If a program binds color buffer 1, but not color buffer 0, this means
> that brw_color_buffer_write_enabled() will return FALSE. Buffer 0 is
> NULL, and buffer 1 supposedly isn't written by the program.
>
> On most platforms, this happens to work out due to several subtle
> interactions:
>
> 1. Since the shader also happens to write gl_FragDepth, the following
> code in gen7_wm_state.c will still enable pixel shader dispatch:
>
> if (brw_color_buffer_write_enabled(brw) || writes_depth ||
> dw1 & GEN7_WM_KILL_ENABLE) {
> dw1 |= GEN7_WM_DISPATCH_ENABLE;
>
> So, the only reason we get color buffer writes to work at all is that we
> happened to write gl_FragDepth. Otherwise, we'd get no PS threads
> dispatched.
>
> 2. The fragment shader happens to write the right colors to the RTs.
>
> Our FS backend writes render targets in order. Since out_color
> corresponds to RT 0, it happens first. We put the color in the MRFs,
> and issue our first FB write.
>
> The later targets aren't technically written, but we happen to issue FB
> writes for them anyway, for some reason. We don't assign any particular
I'm not sure if it's a bug to write to all the draw buffers even if fragment
shader writes to only one. Spec says color values of unwritten draw
buffers will be undefined. But, I think it's better to leave them unchanged.
NVIDIA's driver doesn't modify the color values of unwritten draw buffers.
> color to the MRFs, so they retain their existing value...which happens
> to be the color for RT 0...which happens to be what we wanted.
>
> Nasty stuff.
>
> On Broadwell, things break because we use brw_color_buffer_write_enabled
> in the 3DSTATE_PS_BLEND packet to set the GEN8_PS_BLEND_HAS_WRITEABLE_RT
> bit. Since it returns FALSE, we've told the hardware that no writable
> render target exists. Apparently, this makes the hardware not write
> things depending on $PHASE_OF_MOON.
>
> Changing it to gl_FragColor makes brw_color_buffer_write_enabled return
> true when RTs (!= 0) exist, which makes us set this bit correctly, and
> then things start working.
>
I had these questions in my mind why existing code ever worked. Thanks
for explaining it Ken. Now I can sleep peacefully :).
> --Ken
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
