[Mesa-dev] [PATCH] st/mesa: use table-driven approach to exposing extensions for formats (v2)

[Brian Paul]

On Tue, Jan 24, 2012 at 9:09 PM, Marek Olšák <maraeo at gmail.com> wrote:
> The check for ctx->API was unnecessary, because OES extensions are not exposed
> in desktop GL.
>
> Also require renderbuffer support for ARB_texture_rgb10_a2ui,
> as per the spec.
>
> Tested by comparing old and new glxinfo with softpipe and r600g.
>
> v2: fix bugs
> ---
>  src/mesa/state_tracker/st_extensions.c |  364 ++++++++++++++------------------
>  1 files changed, 157 insertions(+), 207 deletions(-)
>
> diff --git a/src/mesa/state_tracker/st_extensions.c b/src/mesa/state_tracker/st_extensions.c
> index 7fb4c8c..e4a1024 100644
> --- a/src/mesa/state_tracker/st_extensions.c
> +++ b/src/mesa/state_tracker/st_extensions.c
> @@ -258,6 +258,45 @@ struct st_extension_cap_mapping {
>    int cap;
>  };
>
> +struct st_extension_format_mapping {
> +   int extension_offset[2];
> +   enum pipe_format format[8];
> +   GLboolean need_only_one;

Maybe add a comment on need_only_one to describe what it's for.  Or,
if you think there might someday be a case where we might need to
check for two or more formats being supported, make it an unsigned
min_formats_required.


> +};
> +

Some comments on this function would be nice.


> +static void init_format_extensions(struct st_context *st,
> +                                   struct st_extension_format_mapping *mapping,

mapping can be const, right?

> +                                   unsigned num_elements,

If num_elements is the size of the mapping array, maybe call it num_mappings.


> +                                   enum pipe_texture_target target,
> +                                   unsigned bind_flags)
> +{
> +   struct pipe_screen *screen = st->pipe->screen;
> +   GLboolean *extensions = (GLboolean *) &st->ctx->Extensions;
> +   int i, j;
> +   int num_formats = Elements(mapping->format);
> +   int num_ext = Elements(mapping->extension_offset);
> +
> +   for (i = 0; i < num_elements; i++) {
> +      int num_supported = 0;
> +
> +      /* Examine each format in the list. */
> +      for (j = 0; j < num_formats && mapping[i].format[j]; j++) {
> +         if (screen->is_format_supported(screen, mapping[i].format[j],
> +                                         target, 0, bind_flags)) {
> +            num_supported++;
> +         }
> +      }
> +
> +      if (!num_supported ||
> +          (!mapping[i].need_only_one && num_supported != j)) {
> +         continue;
> +      }
> +
> +      /* Enable all extensions in the list. */
> +      for (j = 0; j < num_ext && mapping[i].extension_offset[j]; j++)
> +         extensions[mapping[i].extension_offset[j]] = GL_TRUE;
> +   }
> +}
>
>  /**
>  * Use pipe_screen::get_param() to query PIPE_CAP_ values to determine
> @@ -311,6 +350,108 @@ void st_init_extensions(struct st_context *st)
>       { o(MESA_texture_array),               PIPE_CAP_MAX_TEXTURE_ARRAY_LAYERS         },
>    };
>
> +   /* Required: render target and sampler support */
> +   static struct st_extension_format_mapping rendertarget_mapping[] = {

This whole object can be const, I think.


> +      { { o(ARB_texture_float) },
> +        { PIPE_FORMAT_R32G32B32A32_FLOAT,
> +          PIPE_FORMAT_R16G16B16A16_FLOAT } },

For entries like this where need_only_one is not specified, we're
assuming it'll get initialized to zero/false, right?  I have a feeling
that some compilers will complain about missing initializers.

The rest looks good.

-Brian