[Mesa-dev] [PATCH] meta: remove call to _meta_in_progress(), fix multisample enable/disable

[Ian Romanick]

On 08/30/2012 08:14 AM, Paul Berry wrote:
> From: Brian Paul <brianp at vmware.com>
>
> This partially reverts d638da23d2ec2e9c52655b1ea138249e7f8bcccb.
>
> With gallium the meta code is not always built so the call to
> _meta_in_progress() was unresolved.  Simply special-case the
> GL_MULTISAMPLE case in the meta code.  There might be other special
> cases in the future given all the differences between legacy GL,
> core GL, GLES, etc.
>
> Fixes https://bugs.freedesktop.org/show_bug.cgi?id=54234
> and https://bugs.freedesktop.org/show_bug.cgi?id=54239
>
> v2 (Paul Berry <stereotype441 at gmail.com>): keep _meta_in_progress
> function, since it's needed by the i965 driver, but don't call it from
> core mesa.
> ---
>   src/mesa/drivers/common/meta.c |   33 +++++++++++++++++++++++++++++++--
>   src/mesa/main/enable.c         |    7 +------
>   2 files changed, 32 insertions(+), 8 deletions(-)
>
> diff --git a/src/mesa/drivers/common/meta.c b/src/mesa/drivers/common/meta.c
> index abb7d4e..e38759f 100644
> --- a/src/mesa/drivers/common/meta.c
> +++ b/src/mesa/drivers/common/meta.c
> @@ -438,6 +438,35 @@ _mesa_meta_free(struct gl_context *ctx)
>
>
>   /**
> + * This is an alternative to _mesa_set_enable() to handle some special cases.
> + * See comments inside.
> + */
> +static void
> +meta_set_enable(struct gl_context *ctx, GLenum cap, GLboolean state)
> +{
> +   switch (cap) {
> +   case GL_MULTISAMPLE:
> +      /* We need to enable/disable multisample when using GLES but this enum
> +       * is not supported there.
> +       */
> +      if (ctx->Multisample.Enabled == state)
> +         return;
> +      FLUSH_VERTICES(ctx, _NEW_MULTISAMPLE);
> +      ctx->Multisample.Enabled = state;
> +      break;
> +   default:
> +      _mesa_problem(ctx, "Unexpected cap in _meta_set_enable()");
> +      return;
> +   }
> +
> +   if (ctx->Driver.Enable) {
> +      ctx->Driver.Enable(ctx, cap, state);
> +   }
> +}

I'm not a fan of this ugly layering violation.  Duplicating this logic 
across multiple places is just asking for trouble.  I'd prefer:

1. Just allow GL_MULTISAMPLE to be modified regardless of API (what I 
had originally done).

2. Add a _mesa_set_multisample in enable.c that is used from meta and 
_mesa_set_enable.

> +
> +
> +
> +/**
>    * Enter meta state.  This is like a light-weight version of glPushAttrib
>    * but it also resets most GL state back to default values.
>    *
> @@ -755,7 +784,7 @@ _mesa_meta_begin(struct gl_context *ctx, GLbitfield state)
>      if (state & MESA_META_MULTISAMPLE) {
>         save->MultisampleEnabled = ctx->Multisample.Enabled;
>         if (ctx->Multisample.Enabled)
> -         _mesa_set_enable(ctx, GL_MULTISAMPLE, GL_FALSE);
> +         meta_set_enable(ctx, GL_MULTISAMPLE, GL_FALSE);
>      }
>
>      /* misc */
> @@ -1057,7 +1086,7 @@ _mesa_meta_end(struct gl_context *ctx)
>
>      if (state & MESA_META_MULTISAMPLE) {
>         if (ctx->Multisample.Enabled != save->MultisampleEnabled)
> -         _mesa_set_enable(ctx, GL_MULTISAMPLE, save->MultisampleEnabled);
> +         meta_set_enable(ctx, GL_MULTISAMPLE, save->MultisampleEnabled);
>      }
>
>      /* misc */
> diff --git a/src/mesa/main/enable.c b/src/mesa/main/enable.c
> index b713f5f..14eea53 100644
> --- a/src/mesa/main/enable.c
> +++ b/src/mesa/main/enable.c
> @@ -765,13 +765,8 @@ _mesa_set_enable(struct gl_context *ctx, GLenum cap, GLboolean state)
>
>         /* GL_ARB_multisample */
>         case GL_MULTISAMPLE_ARB:
> -         /* Technically speaking, this should not be allowed for OpenGL ES 2.0
> -          * or 3.0.  However, meta really needs it.
> -          */
> -         if (!_mesa_meta_in_progress(ctx) && !_mesa_is_desktop_gl(ctx)
> -             && ctx->API != API_OPENGLES)
> +         if (!_mesa_is_desktop_gl(ctx) && ctx->API != API_OPENGLES)
>               goto invalid_enum_error;
> -
>            if (ctx->Multisample.Enabled == state)
>               return;
>            FLUSH_VERTICES(ctx, _NEW_MULTISAMPLE);
>