[Mesa-dev] [PATCH] i965/blorp: do not use unnecessary hw-blending support

[Pohjolainen, Topi]

On Tue, Jan 28, 2014 at 11:48:18AM +0200, Pohjolainen, Topi wrote:
> On Mon, Jan 27, 2014 at 06:00:54PM -0800, Kenneth Graunke wrote:
> > On 01/27/2014 11:06 AM, Pohjolainen, Topi wrote:
> > > On Mon, Jan 27, 2014 at 10:20:54AM -0800, Eric Anholt wrote:
> > >> Topi Pohjolainen <topi.pohjolainen at intel.com> writes:
> > >>
> > >>> This is really not needed as blorp blit programs already sample
> > >>> XRGB normally and get alpha channel set to 1.0 automatically by
> > >>> the sampler engine. This is simply copied directly to the payload
> > >>> of the render target write message and hence there is no need for
> > >>> any additional blending support from the pixel processing pipeline.
> > >>>
> > >>> Fixes recently modified piglit test gl-3.2-layered-rendering-blit
> > >>> on IVB. No regressions on IVB.
> > >>
> > >> What about when you have a RGB-but-not-alpha gl format that's been
> > >> promoted to an ARGB Mesa and BRW surface format?  I don't think blorp's
> > >> samplers have any overrides going on there.
> > > 
> > > I relied on the restriction that only blits from RGBX to RGBA and vice
> > > versa are allowed. Otherwise the formats must match.
> > 
> > I think Eric's right.  Technically, it works for BlitFramebuffer because
> > of what you say...but...we allow nearly arbitrary format conversions for
> > CopyTexSubImage today, so I think dropping this could break things
> > there.  Plus, we hope to add that for BlitFramebuffer too (it's trivial).
> > 
> > So, I think this code may need to stay after all...sorry for the confusion.
> 
> This is fine by me, I'd rather understand it fully before changing it anyway.
> Further observation I made is that if I left just the alpha channel blending
> in place and removed the color blending than the test case passes also:
> 
> diff --git a/src/mesa/drivers/dri/i965/gen6_blorp.cpp b/src/mesa/drivers/dri/i96
> index 90b9fbb..959ed05 100644
> --- a/src/mesa/drivers/dri/i965/gen6_blorp.cpp
> +++ b/src/mesa/drivers/dri/i965/gen6_blorp.cpp
> @@ -262,14 +262,10 @@ gen6_blorp_emit_blend_state(struct brw_context *brw,
>     if (params->src.mt &&
>         _mesa_get_format_bits(params->dst.mt->format, GL_ALPHA_BITS) > 0 &&
>         _mesa_get_format_bits(params->src.mt->format, GL_ALPHA_BITS) == 0) {
> -      blend->blend0.blend_enable = 1;
>        blend->blend0.ia_blend_enable = 1;
>  
> -      blend->blend0.blend_func = BRW_BLENDFUNCTION_ADD;
>        blend->blend0.ia_blend_func = BRW_BLENDFUNCTION_ADD;
>  
> -      blend->blend0.source_blend_factor = BRW_BLENDFACTOR_SRC_COLOR;
> -      blend->blend0.dest_blend_factor = BRW_BLENDFACTOR_ZERO;
>        blend->blend0.ia_source_blend_factor = BRW_BLENDFACTOR_ONE;
>        blend->blend0.ia_dest_blend_factor = BRW_BLENDFACTOR_ZERO;
>     }
> 
> 
> If I read this correctly the color blending should be no-op anyway 
> ((1.0 * src) + (0 * dst)) and should be therefore safe to drop anyway?
> Naturally it is still unclear why it behaves as (src * src) instead:
> 
> Expected: 0.500000 0.400000 0.300000
> Observed: 0.250980 0.160784 0.090196

Well, I think I now understand it. We are setting the _multiplier_ of the
source as the source itself instead of just one. This passes the test and
makes sense to me at least:

diff --git a/src/mesa/drivers/dri/i965/gen6_blorp.cpp b/src/mesa/drivers/dri/i96
index 90b9fbb..4ddfc74 100644
--- a/src/mesa/drivers/dri/i965/gen6_blorp.cpp
+++ b/src/mesa/drivers/dri/i965/gen6_blorp.cpp
@@ -268,7 +268,7 @@ gen6_blorp_emit_blend_state(struct brw_context *brw,
       blend->blend0.blend_func = BRW_BLENDFUNCTION_ADD;
       blend->blend0.ia_blend_func = BRW_BLENDFUNCTION_ADD;
 
-      blend->blend0.source_blend_factor = BRW_BLENDFACTOR_SRC_COLOR;
+      blend->blend0.source_blend_factor = BRW_BLENDFACTOR_ONE;
       blend->blend0.dest_blend_factor = BRW_BLENDFACTOR_ZERO;
       blend->blend0.ia_source_blend_factor = BRW_BLENDFACTOR_ONE;
       blend->blend0.ia_dest_blend_factor = BRW_BLENDFACTOR_ZERO;

> 
> There is a note in the PRM saying "Enabling LogicOp and ColorBufferBlending
> at the same time is UNDEFINED". I don't know exactly what is this referring
> to.
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev