[Mesa-dev] [PATCH] st/xa: Use PIPE_FORMAT_R8_UNORM when available

[Jose Fonseca]

On 17/09/15 10:58, Thomas Hellstrom wrote:
> On 09/17/2015 11:53 AM, Thomas Hellstrom wrote:
>> On 09/17/2015 11:34 AM, Jose Fonseca wrote:
>>> On 16/09/15 14:04, Thomas Hellstrom wrote:
>>>> XA has been using L8_UNORM for a8 and yuv component surfaces.
>>>> This commit instead makes XA prefer R8_UNORM since it's assumed to
>>>> have a
>>>> higher availability.
>>>>
>>>> Also neither of these formats are suitable as destination formats using
>>>> destination alpha blending, so reject those operations.
>>>>
>>>> Signed-off-by: Thomas Hellstrom <thellstrom at vmware.com>
>>>> ---
>>>>    src/gallium/state_trackers/xa/xa_composite.c | 40
>>>> ++++++++++------------------
>>>>    src/gallium/state_trackers/xa/xa_tracker.c   | 28 +++++++++++++------
>>>>    2 files changed, 34 insertions(+), 34 deletions(-)
>>>>
>>>> diff --git a/src/gallium/state_trackers/xa/xa_composite.c
>>>> b/src/gallium/state_trackers/xa/xa_composite.c
>>>> index 7cfd1e1..e81eeba 100644
>>>> --- a/src/gallium/state_trackers/xa/xa_composite.c
>>>> +++ b/src/gallium/state_trackers/xa/xa_composite.c
>>>> @@ -78,26 +78,6 @@ static const struct xa_composite_blend xa_blends[]
>>>> = {
>>>>          0, 0, PIPE_BLENDFACTOR_ONE, PIPE_BLENDFACTOR_ONE},
>>>>    };
>>>>
>>>> -
>>>> -/*
>>>> - * The alpha value stored in a luminance texture is read by the
>>>> - * hardware as color.
>>>> - */
>>>> -static unsigned
>>>> -xa_convert_blend_for_luminance(unsigned factor)
>>>> -{
>>>> -    switch(factor) {
>>>> -    case PIPE_BLENDFACTOR_DST_ALPHA:
>>>> -    return PIPE_BLENDFACTOR_DST_COLOR;
>>>> -    case PIPE_BLENDFACTOR_INV_DST_ALPHA:
>>>> -    return PIPE_BLENDFACTOR_INV_DST_COLOR;
>>>> -    default:
>>>> -    break;
>>>> -    }
>>>> -    return factor;
>>>> -}
>>>> -
>>>> -
>>>>    static boolean
>>>>    blend_for_op(struct xa_composite_blend *blend,
>>>>             enum xa_composite_op op,
>>>> @@ -131,10 +111,16 @@ blend_for_op(struct xa_composite_blend *blend,
>>>>        if (!dst_pic->srf)
>>>>        return supported;
>>>>
>>>> -    if (dst_pic->srf->tex->format == PIPE_FORMAT_L8_UNORM) {
>>>> -    blend->rgb_src = xa_convert_blend_for_luminance(blend->rgb_src);
>>>> -    blend->rgb_dst = xa_convert_blend_for_luminance(blend->rgb_dst);
>>>> -    }
>>>> +    /*
>>>> +     * None of the hardware formats we might use for dst A8 are
>>>> +     * suitable for dst_alpha blending, since they present the
>>>> +     * alpha channel either in all color channels (L8_UNORM) or
>>>> +     * in the red channel only (R8_UNORM)
>>>> +     */
>>>> +    if ((dst_pic->srf->tex->format == PIPE_FORMAT_L8_UNORM ||
>>>> +         dst_pic->srf->tex->format == PIPE_FORMAT_R8_UNORM) &&
>>>> +        blend->alpha_dst)
>>>> +        return FALSE;
>>> I'm not very familiar with this code, but wasn't
>>> xa_convert_blend_for_luminance compensating for the A8 <-> L8
>>> mismatch?  It looks like it was doing the right thing.   And
>>> xa_convert_blend_for_luminance would work both for L8 and R8.
>> In these operations since the destination format is A8, we're only
>> interested in the resulting alpha channel. With
>> PIPE_BLENDFACTOR_DST_ALPHA, we would use the destination alpha channel
>> for blending, which would pick up 1 for L8 and R8 which is obviously
>> wrong. Now the removed function instead set PIPE_BLENDFACTOR_DST_COLOR,
>> which would, if I read the specs correctly blend component-wise and
>> yield the correct results for the R, G and B channels if L8 is used. and
>> for the R channel only if R8 is used. But a prerequisite here is that
>> the destination logical format is A8 and the alpha channel is never
>> correct AFAICT.
>>
> Hmm, or are you making the point that blending takes place when we've
> already swizzled the alpha value to the red channel? Yes, then you're
> obviously right, and I need to revert that "fix" ;).

Yeah, my understanding is that we're emulating A8 with L8/R8, hence 
logical alpha is in physical red.  Hence DST_ALPHA -> DST_RED, etc..

Jose