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.pohjolai...@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;
> }
Reading the PRM more carefully I now get the impression that the independent
alpha channel blending is just a _special_ case of the color buffer blending
and cannot be used alone - this is why you had that no-op color component
blending there in the first place, I guess. I effectively did the same thing
as the original patch of mine - disabled the blending altogether.
>
>
> 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.
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
