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

[Marek Olšák]

On Tue, Feb 19, 2013 at 11:02 AM, Michel Dänzer <michel at daenzer.net> wrote:
> 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.

RGB formats do fail fbo-blending-formats with r600g/redwood here.

However the alpha channel can sometimes contain 1 in memory even if
the format is RGBX. Off the top of my head, glClear, glTex[Sub]Image,
glCopyTex[Sub]Image always set alpha to 1. Blits do too if they use
RGBX as a source. One way to set alpha != 1 is to draw some geometry.

Marek