[Mesa-stable] [Mesa-dev] [PATCH V2 2/2] meta: Use gl_FragColor to output color values to all the draw buffers

Anuj Phogat anuj.phogat at gmail.com
Tue May 20 14:56:39 PDT 2014


On Tue, May 20, 2014 at 2:36 PM, Kenneth Graunke <kenneth at 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-stable at lists.freedesktop.org>
>>> Signed-off-by: Anuj Phogat <anuj.phogat at gmail.com>
>>> Reviewed-by: Matt Turner <mattst88 at 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 <kenneth at 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
>


More information about the mesa-stable mailing list