[Mesa-dev] [PATCH] radeonsi: Fix blending using destination alpha factor but non-alpha destination

[Michel Dänzer]

On Die, 2013-02-19 at 10:33 +0100, Christian König wrote: 
> Am 18.02.2013 20:11, schrieb Roland Scheidegger:
> > Am 18.02.2013 19:14, schrieb Michel Dänzer:
> >> From: Michel Dänzer <michel.daenzer at amd.com>
> >>
> >> 11 more little piglits.
> >>
> >> NOTE: This is a candidate for the 9.1 branch.
> >>
> >> Signed-off-by: Michel Dänzer <michel.daenzer at amd.com>
> >> ---
> >>
> >> Any ideas why this seems necessary with radeonsi but not with r600g?
> > Maybe the hw uses an implicit 1 if the format has no alpha (though I'm
> > not sure if it can always know with bgrx formats and the like).
> > I'm wondering if there should be a helper for those fixups. Looks to me
> > like quite some drivers need it (though well so far I think just
> > non-gallium i965 does this plus llvmpipe, but for some of the others I'm
> > skeptical if not doing it is really correct...).
> 
> I agree alpha blending with a buffer format that doesn't have alpha is a 
> bit strange, that should be catched by the upper layers.

If it was that simple. :\

The problem is that AFAICT for formats such as R8G8B8X8, there's no
other way to tell the hardware to always use 1 for the destination
alpha. And I'm not sure we can just not support any such formats, I
certainly don't think that would be a good idea.


> >> @@ -84,7 +57,7 @@ static uint32_t si_translate_blend_function(int blend_func)
> >>   	return 0;
> >>   }
> >>   
> >> -static uint32_t si_translate_blend_factor(int blend_fact)
> >> +static uint32_t si_translate_blend_factor(int blend_fact, bool dst_alpha)
> >>   {
> >>   	switch (blend_fact) {
> >>   	case PIPE_BLENDFACTOR_ONE:
> >> @@ -94,7 +67,7 @@ static uint32_t si_translate_blend_factor(int blend_fact)
> >>   	case PIPE_BLENDFACTOR_SRC_ALPHA:
> >>   		return V_028780_BLEND_SRC_ALPHA;
> >>   	case PIPE_BLENDFACTOR_DST_ALPHA:
> >> -		return V_028780_BLEND_DST_ALPHA;
> >> +		return dst_alpha ? V_028780_BLEND_DST_ALPHA : V_028780_BLEND_ONE;
> >>   	case PIPE_BLENDFACTOR_DST_COLOR:
> >>   		return V_028780_BLEND_DST_COLOR;
> >>   	case PIPE_BLENDFACTOR_SRC_ALPHA_SATURATE:
> >> @@ -110,7 +83,7 @@ static uint32_t si_translate_blend_factor(int blend_fact)
> >>   	case PIPE_BLENDFACTOR_INV_SRC_ALPHA:
> >>   		return V_028780_BLEND_ONE_MINUS_SRC_ALPHA;
> >>   	case PIPE_BLENDFACTOR_INV_DST_ALPHA:
> >> -		return V_028780_BLEND_ONE_MINUS_DST_ALPHA;
> >> +		return dst_alpha ? V_028780_BLEND_ONE_MINUS_DST_ALPHA : V_028780_BLEND_ZERO;
> >>   	case PIPE_BLENDFACTOR_INV_DST_COLOR:
> >>   		return V_028780_BLEND_ONE_MINUS_DST_COLOR;
> >>   	case PIPE_BLENDFACTOR_INV_CONST_COLOR:
> >> @@ -133,30 +106,25 @@ static uint32_t si_translate_blend_factor(int blend_fact)
> >>   	return 0;
> >>   }
> > I think you might also need to patch up SRC_ALPHA_SATURATE (to zero).
> >
> > Can't comment on the hw stuff but at least llvmpipe does the same
> > otherwise :-).
> 
> Why should we do so? SRC_ALPHA_SATURATE should still work fine, even 
> when the destination buffer doesn't have an alpha component.

I think Roland is right. When the destination has no alpha, the
destination alpha value is supposed to be always 1, so
SRC_ALPHA_SATURATE is always 0. But with a format as described above,
the destination X8 channel may contain any value.


Really, what I don't understand is why r600g doesn't seem affected by
this... at least on my RS880 it's passing the piglit tests this change
fixes with radeonsi. So maybe I'm just missing some magic bit for
radeonsi.


-- 
Earthling Michel Dänzer           |                   http://www.amd.com
Libre software enthusiast         |          Debian, X and DRI developer