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

[Pohjolainen, Topi]

On Tue, Jan 28, 2014 at 02:20:47PM +0200, Pohjolainen, Topi wrote:
> 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;
> 

And I think we need additionally the following to take care of the cases
mentioned by Eric where the alpha is not set to 1.0 by the sampler. There
we need to get destination alpha fixed to 1.0 regardless of the value returned
by the sampler engine for the source. Does this make any sense?

diff --git a/src/mesa/drivers/dri/i965/gen6_blorp.cpp b/src/mesa/drivers/dri/i96
index 4ddfc74..ca903fd 100644
--- a/src/mesa/drivers/dri/i965/gen6_blorp.cpp
+++ b/src/mesa/drivers/dri/i965/gen6_blorp.cpp
@@ -270,7 +270,7 @@ gen6_blorp_emit_blend_state(struct brw_context *brw,
 
       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_source_blend_factor = BRW_BLENDFACTOR_CONST_ALPHA;
       blend->blend0.ia_dest_blend_factor = BRW_BLENDFACTOR_ZERO;
    }
 
@@ -291,6 +291,8 @@ gen6_blorp_emit_cc_state(struct brw_context *brw,
                       &cc_state_offset);
    memset(cc, 0, sizeof(*cc));
 
+   cc->constant_a = 1.0;
+
    return cc_state_offset;
 }

> > 
> > 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
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev