[Mesa-dev] [PATCH] st/mesa: only enable MSAA coverage options when we have a MSAA buffer

[Brian Paul]

On 09/16/2016 08:07 AM, Marek Olšák wrote:
> On Thu, Sep 15, 2016 at 11:20 PM, Brian Paul <brianp at vmware.com> wrote:
>> Regardless of whether GL_MULTISAMPLE is enabled (it's enabled by default)
>> we should not set the alpha_to_coverage or alpha_to_one flags if the
>> current drawing buffer does not do MSAA.
>>
>> This fixes the new piglit gl-1.3-alpha_to_coverage_nop test.
>> ---
>>   src/mesa/state_tracker/st_atom_blend.c | 9 ++++++---
>>   src/mesa/state_tracker/st_context.c    | 3 ++-
>>   2 files changed, 8 insertions(+), 4 deletions(-)
>>
>> diff --git a/src/mesa/state_tracker/st_atom_blend.c b/src/mesa/state_tracker/st_atom_blend.c
>> index 65de67b..222267e 100644
>> --- a/src/mesa/state_tracker/st_atom_blend.c
>> +++ b/src/mesa/state_tracker/st_atom_blend.c
>> @@ -265,9 +265,12 @@ update_blend( struct st_context *st )
>>
>>      blend->dither = ctx->Color.DitherFlag;
>>
>> -   if (ctx->Multisample.Enabled) {
>> -      /* unlike in gallium/d3d10 these operations are only performed
>> -         if msaa is enabled */
>> +   if (ctx->Multisample.Enabled &&
>> +       ctx->DrawBuffer &&
>
> Is it possible for ctx->DrawBuffer to be NULL?

Probably not, but I'm not 100% sure.  I have some memory of an extension 
to allow MakeCurrent(ctx!=null, fb==null) but I can't find it now.  Or 
maybe MakeCurrent(ctx!=null, fb==null) is supposed to be generally 
supported now.  I don't remember and will have to look.

I think the use case for MakeCurrent(ctx!=null, fb==null) is to have a 
context just to compile shaders, etc.

Actually, looking again now, I found the IncompleteFramebuffer object 
and the _mesa_get_incomplete_framebuffer() function.  So it looks like 
that should be used to prevent the null pointer.

And we're not checking for DrawBuffer==NULL elsewhere.  I can remove the 
check.


>
>> +       ctx->DrawBuffer->Visual.sampleBuffers > 0) {
>> +      /* Unlike in gallium/d3d10 these operations are only performed
>> +       * if both msaa is enabled and we have a multisample buffer.
>> +       */
>>         blend->alpha_to_coverage = ctx->Multisample.SampleAlphaToCoverage;
>>         blend->alpha_to_one = ctx->Multisample.SampleAlphaToOne;
>>      }
>> diff --git a/src/mesa/state_tracker/st_context.c b/src/mesa/state_tracker/st_context.c
>> index ddc11a4..81b3387 100644
>> --- a/src/mesa/state_tracker/st_context.c
>> +++ b/src/mesa/state_tracker/st_context.c
>> @@ -166,7 +166,8 @@ void st_invalidate_state(struct gl_context * ctx, GLbitfield new_state)
>>      struct st_context *st = st_context(ctx);
>>
>>      if (new_state & _NEW_BUFFERS) {
>> -      st->dirty |= ST_NEW_DSA |
>> +      st->dirty |= ST_NEW_BLEND |
>> +                   ST_NEW_DSA |
>>                      ST_NEW_FB_STATE |
>>                      ST_NEW_SAMPLE_MASK |
>>                      ST_NEW_SAMPLE_SHADING |
>
> I guess it's OK to add a dependency on _NEW_BUFFERS, because that flag
> is set the least often.
>
> Reviewed-by: Marek Olšák <marek.olsak at amd.com>

Thanks!

PS: I'm also updating the check-in comment with remarks about how I 
stumbled across this in ETQW.

-Brian