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

[Marek Olšák]

On Wed, Jan 25, 2012 at 4:04 AM, Brian Paul <brian.e.paul at gmail.com> wrote:
> 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.

C90 and C99 do require all variables to be initalized to 0 if an
initializer is used. The GCC warning we get with scons seems to be
bogus and it gets printed because we enable it explicitly (maybe we
shouldn't do that). Besides, all the arrays are static...

I'll send a version 3 addressing the other remarks.

Marek