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

[Pohjolainen, Topi]

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

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.