[Mesa-stable] [Mesa-dev] [PATCH V2 2/2] meta: Use gl_FragColor to output color values to all the draw buffers
Kenneth Graunke
kenneth at whitecape.org
Tue May 20 14:36:14 PDT 2014
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:
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
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.
--Ken
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: OpenPGP digital signature
URL: <http://lists.freedesktop.org/archives/mesa-stable/attachments/20140520/50a2b170/attachment.sig>
More information about the mesa-stable
mailing list