[Mesa-dev] [PATCH] i965: allocate at least 1 BLEND_STATE element

[Emil Velikov]

Hello gents,

On 2 July 2015 at 08:45, Kenneth Graunke <kenneth at whitecape.org> wrote:
> On Wednesday, July 01, 2015 10:16:28 AM Mike Stroyan wrote:
>> When there are no color buffer render targets, gen6 and gen7 still
>> use the first BLEND_STATE element to determine alpha test.
>> gen6_upload_blend_state was allocating zero elements when
>> ctx->Color.AlphaEnabled was false.
>> That left _3DSTATE_CC_STATE_POINTERS or _3DSTATE_BLEND_STATE_POINTERS
>> pointing to random data from some previous brw_state_batch().
>> That sometimes suppressed depth rendering when those bits
>> happened to mean COMPAREFUNC_NEVER.
>> This produced flickering shadows for dota2 reborn.
>> ---
>>  src/mesa/drivers/dri/i965/gen6_cc.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/src/mesa/drivers/dri/i965/gen6_cc.c b/src/mesa/drivers/dri/i965/gen6_cc.c
>> index 2bfa271..2b76e24 100644
>> --- a/src/mesa/drivers/dri/i965/gen6_cc.c
>> +++ b/src/mesa/drivers/dri/i965/gen6_cc.c
>> @@ -51,7 +51,7 @@ gen6_upload_blend_state(struct brw_context *brw)
>>      * with render target 0, which will reference BLEND_STATE[0] for
>>      * alpha test enable.
>>      */
>> -   if (nr_draw_buffers == 0 && ctx->Color.AlphaEnabled)
>> +   if (nr_draw_buffers == 0)
>>        nr_draw_buffers = 1;
>>
>>     size = sizeof(*blend) * nr_draw_buffers;
>>
>
> Great catch!
>
> Reviewed-by: Kenneth Graunke <kenneth at whitecape.org>
>
> And pushed:
>    9d408a4..fe2b748  master -> master
>
> I think we ought to change gen8_blend_state.c as well, but I'm not quite
> sure what change to make.  Either we should make the same change you did
> here, or delete the whole "We need at least 1 BLEND_STATE written"
> block.
>
> On Gen8+, it looks like the alpha test and other functions that might
> discard pixels are all in the shared/common DWord, and the per-color
> target DWord pairs look relatively harmless.  I suppose the null RT
> would still refer to BLEND_STATE[0]...so it might still be worth
> emitting one.  Any thoughts?
Should we get this into 10.6 or there are some not so obvious
dependencies that we're missing in the 10.6 branch ? Asking every user
to rebuild mesa just to play Dota2 seems unreasonable imho.

Cheers,
Emil