[Mesa-dev] [PATCH 1/4] mesa: don't flag _NEW_COLOR for KHR adv.blend if prog constant doesn't change

[Marek Olšák]

On Wed, Jan 31, 2018 at 4:34 AM, Ian Romanick <idr at freedesktop.org> wrote:
> On 01/30/2018 07:48 AM, Marek Olšák wrote:
>> From: Marek Olšák <marek.olsak at amd.com>
>>
>> This only affects drivers that set DriverFlags.NewBlend.
>> ---
>>  src/mesa/main/blend.c             |  6 ++++--
>>  src/mesa/main/blend.h             | 41 +++++++++++++++++++++++++++++++--------
>>  src/mesa/main/enable.c            | 14 +++++++++----
>>  src/mesa/program/prog_statevars.c |  3 ++-
>>  4 files changed, 49 insertions(+), 15 deletions(-)
>>
>> diff --git a/src/mesa/main/blend.c b/src/mesa/main/blend.c
>> index 6b379f2..ec8e27e 100644
>> --- a/src/mesa/main/blend.c
>> +++ b/src/mesa/main/blend.c
>> @@ -528,21 +528,22 @@ _mesa_BlendEquation( GLenum mode )
>>
>>     if (!changed)
>>        return;
>>
>>
>>     if (!legal_simple_blend_equation(ctx, mode) && !advanced_mode) {
>>        _mesa_error(ctx, GL_INVALID_ENUM, "glBlendEquation");
>>        return;
>>     }
>>
>> -   _mesa_flush_vertices_for_blend_state(ctx);
>> +   _mesa_flush_vertices_for_blend_adv(ctx, ctx->Color.BlendEnabled,
>> +                                      advanced_mode);
>>
>>     for (buf = 0; buf < numBuffers; buf++) {
>>        ctx->Color.Blend[buf].EquationRGB = mode;
>>        ctx->Color.Blend[buf].EquationA = mode;
>>     }
>>     ctx->Color._BlendEquationPerBuffer = GL_FALSE;
>>     ctx->Color._AdvancedBlendMode = advanced_mode;
>>
>>     if (ctx->Driver.BlendEquationSeparate)
>>        ctx->Driver.BlendEquationSeparate(ctx, mode, mode);
>> @@ -553,21 +554,22 @@ _mesa_BlendEquation( GLenum mode )
>>   * Set blend equation for one color buffer/target.
>>   */
>>  static void
>>  blend_equationi(struct gl_context *ctx, GLuint buf, GLenum mode,
>>                  enum gl_advanced_blend_mode advanced_mode)
>>  {
>>     if (ctx->Color.Blend[buf].EquationRGB == mode &&
>>         ctx->Color.Blend[buf].EquationA == mode)
>>        return;  /* no change */
>>
>> -   _mesa_flush_vertices_for_blend_state(ctx);
>> +   _mesa_flush_vertices_for_blend_adv(ctx, ctx->Color.BlendEnabled,
>> +                                      advanced_mode);
>>     ctx->Color.Blend[buf].EquationRGB = mode;
>>     ctx->Color.Blend[buf].EquationA = mode;
>>     ctx->Color._BlendEquationPerBuffer = GL_TRUE;
>>
>>     if (buf == 0)
>>        ctx->Color._AdvancedBlendMode = advanced_mode;
>>  }
>>
>>
>>  void GLAPIENTRY
>> diff --git a/src/mesa/main/blend.h b/src/mesa/main/blend.h
>> index 2454e0c..cba5a98 100644
>> --- a/src/mesa/main/blend.h
>> +++ b/src/mesa/main/blend.h
>> @@ -147,28 +147,53 @@ extern void
>>  _mesa_update_clamp_vertex_color(struct gl_context *ctx,
>>                                  const struct gl_framebuffer *drawFb);
>>
>>  extern mesa_format
>>  _mesa_get_render_format(const struct gl_context *ctx, mesa_format format);
>>
>>  extern void
>>  _mesa_init_color( struct gl_context * ctx );
>>
>>
>> +static inline unsigned
>> +_mesa_get_advanded_blend_sh_constant(GLbitfield blend_enabled,
>> +                                     enum gl_advanced_blend_mode mode)
>> +{
>> +   return blend_enabled ? mode : 0;
>
> Should this be BLEND_NONE with a return type enum gl_advanced_blend_mode?
>
>> +}
>> +
>> +static inline bool
>> +_mesa_advanded_blend_sh_constant_changed(struct gl_context *ctx,
>> +                                         GLbitfield new_blend_enabled,
>> +                                         enum gl_advanced_blend_mode new_mode)
>> +{
>> +   return _mesa_get_advanded_blend_sh_constant(new_blend_enabled, new_mode) !=
>> +          _mesa_get_advanded_blend_sh_constant(ctx->Color.BlendEnabled,
>> +                                               ctx->Color._AdvancedBlendMode);
>> +}
>> +
>>  static inline void
>>  _mesa_flush_vertices_for_blend_state(struct gl_context *ctx)
>>  {
>> -   /* The advanced blend mode needs _NEW_COLOR to update the state constant,
>> -    * so we have to set it. This is inefficient.
>> -    * This should only be done for states that affect the state constant.
>> -    * It shouldn't be done for other blend states.
>> -    */
>> -   if (_mesa_has_KHR_blend_equation_advanced(ctx) ||
>> -       !ctx->DriverFlags.NewBlend) {
>> +   if (!ctx->DriverFlags.NewBlend) {
>>        FLUSH_VERTICES(ctx, _NEW_COLOR);
>>     } else {
>>        FLUSH_VERTICES(ctx, 0);
>> +      ctx->NewDriverState |= ctx->DriverFlags.NewBlend;
>> +   }
>> +}
>> +
>> +static inline void
>> +_mesa_flush_vertices_for_blend_adv(struct gl_context *ctx,
>> +                                   GLbitfield new_blend_enabled,
>> +                                   enum gl_advanced_blend_mode new_mode)
>> +{
>> +   /* The advanced blend mode needs _NEW_COLOR to update the state constant. */
>> +   if (_mesa_has_KHR_blend_equation_advanced(ctx) &&
>> +       _mesa_advanded_blend_sh_constant_changed(ctx, new_blend_enabled,
>> +                                                new_mode)) {
>> +      FLUSH_VERTICES(ctx, _NEW_COLOR);
>>     }
>> -   ctx->NewDriverState |= ctx->DriverFlags.NewBlend;
>> +   _mesa_flush_vertices_for_blend_state(ctx);
>
> This is going to cause FLUSH_VERTICES twice.  This is supposed to be an
> optimization, so it seems best to avoid that.

FLUSH_VERTICES is a no-op if it's called for the second time, because
this test will fail:
   if (ctx->Driver.NeedFlush & FLUSH_STORED_VERTICES)

The optimization consists of not flagging _NEW_COLOR in most cases,
which has a far greater impact.

Marek