[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