[Mesa-dev] [PATCH] i965: Implement a drirc workaround for broken dual color blending.
Iago Toral
itoral at igalia.com
Thu Jan 21 01:03:18 PST 2016
On Wed, 2016-01-20 at 20:35 -0800, Kenneth Graunke wrote:
> OpenGL's dual color blending feature was specified so that an
> implementation could support both multiple render targets (MRT) and
> dual source blending. Fragment shader outputs specify both "location"
> (the render target number) and "index" (either color 0 or 1).
>
> I believe DirectX only has the notion of "location" - if using dual
> color blending, location 0 or 1 will specify the operands. If not,
> then location means the render target index. The two features can't
> be used together.
>
> As such, some applications mistakenly try to use <loc = 0, index = 0>
> and <loc = 1, index = 0> in a shader used for dual color blending with
> a single render target, rather than the correct <loc = 0, index = 0>
> and <loc = 0, index = 1>.
>
> In particular, Unigine Heaven 4.0 and Valley 1.0 suffer from this bug.
> Unigine is aware of the problem, and quickly developed a fix, but has
> not bothered to change the download link on their website to a working
> copy in over a year. People were still using the broken version and
> complaining. We tried working around this by disabling dual color
> blending, but that apparently hurts performance, and people were once
> again unhappy.
>
> On i965, dual source blending is achieved by using different framebuffer
> write messages than normal rendering. So, we have to compile different
> code for the two cases. We're not being pedantic: we actually have to
> know in order to function.
>
> Normally, dual source blending is detectable in the shader: if a shader
> has an output with index = 1, then it's meant for blending, not MRT.
> With the broken inputs, they're indistinguishable, so we can only tell
> by looking at the current GL state.
>
> This patch implements a new drirc workaround:
>
> export dual_color_blend_by_location=true
>
> which makes the i965 driver detect when OpenGL state is configured for
> dual source blending, and recompile the fragment shader to use the right
> messages. In that case, we allow either location = 1 or index = 1 to
> specify the second source for the blending equations.
>
> It also re-enables GL_ARB_blend_func_extended for Unigine.
That's a great description of the problem and the solution, thanks Ken!
You probably want to get confirmation from others before pushing this
since I imagine that re-enabling blend_func for Unigine affects other
drivers as well, but from my side at least this is:
Reviewed-by: Iago Toral Quiroga <itoral at igalia.com>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=92233
> Signed-off-by: Kenneth Graunke <kenneth at whitecape.org>
> Cc: Marek Olšák <marek.olsak at amd.com>
> Cc: Ilia Mirkin <imirkin at alum.mit.edu>
> ---
> src/mesa/drivers/dri/common/drirc | 16 ++++++++--------
> src/mesa/drivers/dri/common/xmlpool/t_options.h | 5 +++++
> src/mesa/drivers/dri/i965/brw_compiler.h | 1 +
> src/mesa/drivers/dri/i965/brw_context.c | 3 +++
> src/mesa/drivers/dri/i965/brw_context.h | 1 +
> src/mesa/drivers/dri/i965/brw_fs_nir.cpp | 6 +++++-
> src/mesa/drivers/dri/i965/brw_wm.c | 4 ++++
> src/mesa/drivers/dri/i965/intel_screen.c | 1 +
> 8 files changed, 28 insertions(+), 9 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/common/drirc b/src/mesa/drivers/dri/common/drirc
> index e1874c3..183a1dc 100644
> --- a/src/mesa/drivers/dri/common/drirc
> +++ b/src/mesa/drivers/dri/common/drirc
> @@ -37,26 +37,26 @@ TODO: document the other workarounds.
>
> <application name="Unigine Heaven (32-bit)" executable="heaven_x86">
> <option name="allow_glsl_extension_directive_midshader" value="true" />
> - <!-- remove disable_blend_func_extended if 4.1 ever comes out -->
> - <option name="disable_blend_func_extended" value="true" />
> + <!-- remove dual_color_blend_by_location if 4.1 ever comes out -->
> + <option name="dual_color_blend_by_location" value="true" />
> </application>
>
> <application name="Unigine Heaven (64-bit)" executable="heaven_x64">
> <option name="allow_glsl_extension_directive_midshader" value="true" />
> - <!-- remove disable_blend_func_extended if 4.1 ever comes out -->
> - <option name="disable_blend_func_extended" value="true" />
> + <!-- remove dual_color_blend_by_location if 4.1 ever comes out -->
> + <option name="dual_color_blend_by_location" value="true" />
> </application>
>
> <application name="Unigine Valley (32-bit)" executable="valley_x86">
> <option name="allow_glsl_extension_directive_midshader" value="true" />
> - <!-- remove disable_blend_func_extended if 1.1 ever comes out -->
> - <option name="disable_blend_func_extended" value="true" />
> + <!-- remove dual_color_blend_by_location if 1.1 ever comes out -->
> + <option name="dual_color_blend_by_location" value="true" />
> </application>
>
> <application name="Unigine Valley (64-bit)" executable="valley_x64">
> <option name="allow_glsl_extension_directive_midshader" value="true" />
> - <!-- remove disable_blend_func_extended if 1.1 ever comes out -->
> - <option name="disable_blend_func_extended" value="true" />
> + <!-- remove dual_color_blend_by_location if 1.1 ever comes out -->
> + <option name="dual_color_blend_by_location" value="true" />
> </application>
>
> <application name="Unigine OilRush (32-bit)" executable="OilRush_x86">
> diff --git a/src/mesa/drivers/dri/common/xmlpool/t_options.h b/src/mesa/drivers/dri/common/xmlpool/t_options.h
> index 4e5a721..55e926b 100644
> --- a/src/mesa/drivers/dri/common/xmlpool/t_options.h
> +++ b/src/mesa/drivers/dri/common/xmlpool/t_options.h
> @@ -90,6 +90,11 @@ DRI_CONF_OPT_BEGIN_B(disable_blend_func_extended, def) \
> DRI_CONF_DESC(en,gettext("Disable dual source blending")) \
> DRI_CONF_OPT_END
>
> +#define DRI_CONF_DUAL_COLOR_BLEND_BY_LOCATION(def) \
> +DRI_CONF_OPT_BEGIN_B(dual_color_blend_by_location, def) \
> + DRI_CONF_DESC(en,gettext("Identify dual color blending sources by location rather than index")) \
> +DRI_CONF_OPT_END
> +
> #define DRI_CONF_DISABLE_GLSL_LINE_CONTINUATIONS(def) \
> DRI_CONF_OPT_BEGIN_B(disable_glsl_line_continuations, def) \
> DRI_CONF_DESC(en,gettext("Disable backslash-based line continuations in GLSL source")) \
> diff --git a/src/mesa/drivers/dri/i965/brw_compiler.h b/src/mesa/drivers/dri/i965/brw_compiler.h
> index 224ddb1..748ffe5 100644
> --- a/src/mesa/drivers/dri/i965/brw_compiler.h
> +++ b/src/mesa/drivers/dri/i965/brw_compiler.h
> @@ -246,6 +246,7 @@ struct brw_wm_prog_key {
> bool compute_sample_id:1;
> unsigned line_aa:2;
> bool high_quality_derivatives:1;
> + bool force_dual_color_blend:1;
>
> uint16_t drawable_height;
> uint64_t input_slots_valid;
> diff --git a/src/mesa/drivers/dri/i965/brw_context.c b/src/mesa/drivers/dri/i965/brw_context.c
> index 9ba3339..1032e5a 100644
> --- a/src/mesa/drivers/dri/i965/brw_context.c
> +++ b/src/mesa/drivers/dri/i965/brw_context.c
> @@ -750,6 +750,9 @@ brw_process_driconf_options(struct brw_context *brw)
>
> ctx->Const.AllowGLSLExtensionDirectiveMidShader =
> driQueryOptionb(options, "allow_glsl_extension_directive_midshader");
> +
> + brw->dual_color_blend_by_location =
> + driQueryOptionb(options, "dual_color_blend_by_location");
> }
>
> GLboolean
> diff --git a/src/mesa/drivers/dri/i965/brw_context.h b/src/mesa/drivers/dri/i965/brw_context.h
> index 2a29dfe..55d6723 100644
> --- a/src/mesa/drivers/dri/i965/brw_context.h
> +++ b/src/mesa/drivers/dri/i965/brw_context.h
> @@ -836,6 +836,7 @@ struct brw_context
> bool always_flush_cache;
> bool disable_throttling;
> bool precompile;
> + bool dual_color_blend_by_location;
>
> driOptionCache optionCache;
> /** @} */
> diff --git a/src/mesa/drivers/dri/i965/brw_fs_nir.cpp b/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
> index d7bcc1c..f9df2a4 100644
> --- a/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
> @@ -130,7 +130,11 @@ fs_visitor::nir_setup_outputs()
> break;
> }
> case MESA_SHADER_FRAGMENT:
> - if (var->data.index > 0) {
> + if (key->force_dual_color_blend &&
> + var->data.location == FRAG_RESULT_DATA1) {
> + this->dual_src_output = reg;
> + this->do_dual_src = true;
> + } else if (var->data.index > 0) {
> assert(var->data.location == FRAG_RESULT_DATA0);
> assert(var->data.index == 1);
> this->dual_src_output = reg;
> diff --git a/src/mesa/drivers/dri/i965/brw_wm.c b/src/mesa/drivers/dri/i965/brw_wm.c
> index 39d644e..78846dc 100644
> --- a/src/mesa/drivers/dri/i965/brw_wm.c
> +++ b/src/mesa/drivers/dri/i965/brw_wm.c
> @@ -515,6 +515,10 @@ brw_wm_populate_key(struct brw_context *brw, struct brw_wm_prog_key *key)
> /* _NEW_BUFFERS */
> key->nr_color_regions = ctx->DrawBuffer->_NumColorDrawBuffers;
>
> + /* _NEW_COLOR */
> + key->force_dual_color_blend = brw->dual_color_blend_by_location &&
> + (ctx->Color.BlendEnabled & 1) && ctx->Color.Blend[0]._UsesDualSrc;
> +
> /* _NEW_MULTISAMPLE, _NEW_COLOR, _NEW_BUFFERS */
> key->replicate_alpha = ctx->DrawBuffer->_NumColorDrawBuffers > 1 &&
> (ctx->Multisample.SampleAlphaToCoverage || ctx->Color.AlphaEnabled);
> diff --git a/src/mesa/drivers/dri/i965/intel_screen.c b/src/mesa/drivers/dri/i965/intel_screen.c
> index e1e1e62..bca783a 100644
> --- a/src/mesa/drivers/dri/i965/intel_screen.c
> +++ b/src/mesa/drivers/dri/i965/intel_screen.c
> @@ -79,6 +79,7 @@ DRI_CONF_BEGIN
> DRI_CONF_FORCE_GLSL_EXTENSIONS_WARN("false")
> DRI_CONF_DISABLE_GLSL_LINE_CONTINUATIONS("false")
> DRI_CONF_DISABLE_BLEND_FUNC_EXTENDED("false")
> + DRI_CONF_DUAL_COLOR_BLEND_BY_LOCATION("false")
> DRI_CONF_ALLOW_GLSL_EXTENSION_DIRECTIVE_MIDSHADER("false")
>
> DRI_CONF_OPT_BEGIN_B(shader_precompile, "true")
More information about the mesa-dev
mailing list