[Mesa-dev] [PATCH 5/6] mesa: enable enums for OES_texture_storage_multisample_2d_array

[Ilia Mirkin]

On Wed, Aug 26, 2015 at 1:19 AM, Tapani Pälli <tapani.palli at intel.com> wrote:
> On 08/24/2015 04:18 PM, Ilia Mirkin wrote:
>>
>> On Fri, Aug 21, 2015 at 3:22 AM, Tapani Pälli <tapani.palli at intel.com>
>> wrote:
>>>
>>> Signed-off-by: Tapani Pälli <tapani.palli at intel.com>
>>> ---
>>>   src/mesa/main/get_hash_params.py | 6 +++---
>>>   src/mesa/main/texobj.c           | 6 ++++--
>>>   src/mesa/main/texparam.c         | 2 +-
>>>   3 files changed, 8 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/src/mesa/main/get_hash_params.py
>>> b/src/mesa/main/get_hash_params.py
>>> index 517c391..c06e9b1 100644
>>> --- a/src/mesa/main/get_hash_params.py
>>> +++ b/src/mesa/main/get_hash_params.py
>>> @@ -434,6 +434,9 @@ descriptor=[
>>>     [ "SAMPLE_MASK", "CONTEXT_BOOL(Multisample.SampleMask),
>>> extra_ARB_texture_multisample_es31" ],
>>>     [ "MAX_SAMPLE_MASK_WORDS", "CONST(1),
>>> extra_ARB_texture_multisample_es31" ],
>>>
>>> +# GL_ARB_texture_multisample / ES 3.1 with
>>> GL_OES_texture_storage_multisample_2d_array
>>> +  [ "TEXTURE_BINDING_2D_MULTISAMPLE_ARRAY", "LOC_CUSTOM, TYPE_INT,
>>> TEXTURE_2D_MULTISAMPLE_ARRAY_INDEX, extra_ARB_texture_multisample_es31" ],
>>
>> What does the _es31 add? Should be removed, I think. Should be done in
>> a cleanup patch later though. Looks like I wasn't looking carefully
>> enough at what was going on the list and this managed to sneak
>> through... same goes for most (but probably not all) of the _es31
>> things.
>
>
> I think using the _es31 is technically the best solution leaving no gaps or
> 'hidden features' where we enable some GLES functionality through desktop
> extension. I think we already went through this discussion.

Yeah, and I pointed this same thing out then too, and was ignored I believe.

> For extensions
> string enable, I think it's feasible to use desktop feature enables but
> anywhere else in the code it hides things from the reader.

I must be missing something. Can you describe to me a scenario under
which the _es31 thing makes *any* change to the code paths executed?
The only one I can imagine is with a driver that doesn't set
Extensions.ARB_texture_multisample, but a user has force-enabled ES3.1
via override flags. I don't think that this is a case worth worrying
about.

In fact I've sent a patch to remove all the _es31 things (except for
ARB_cs, where it's an actual scenario).

>
>>> +
>>>   # GL_ARB_texture_gather / GLES 3.1
>>>     [ "MIN_PROGRAM_TEXTURE_GATHER_OFFSET",
>>> "CONTEXT_INT(Const.MinProgramTextureGatherOffset),
>>> extra_ARB_texture_gather_es31"],
>>>     [ "MAX_PROGRAM_TEXTURE_GATHER_OFFSET",
>>> "CONTEXT_INT(Const.MaxProgramTextureGatherOffset),
>>> extra_ARB_texture_gather_es31"],
>>> @@ -740,9 +743,6 @@ descriptor=[
>>>     [ "TEXTURE_BUFFER_FORMAT_ARB", "LOC_CUSTOM, TYPE_INT, 0,
>>> extra_texture_buffer_object" ],
>>>     [ "TEXTURE_BUFFER_ARB", "LOC_CUSTOM, TYPE_INT, 0,
>>> extra_texture_buffer_object" ],
>>>
>>> -# GL_ARB_texture_multisample / GL 3.2
>>> -  [ "TEXTURE_BINDING_2D_MULTISAMPLE_ARRAY", "LOC_CUSTOM, TYPE_INT,
>>> TEXTURE_2D_MULTISAMPLE_ARRAY_INDEX, extra_ARB_texture_multisample" ],
>>> -
>>>   # GL 3.0
>>>     [ "CONTEXT_FLAGS", "CONTEXT_INT(Const.ContextFlags),
>>> extra_version_30" ],
>>>
>>> diff --git a/src/mesa/main/texobj.c b/src/mesa/main/texobj.c
>>> index 395e4d3..a3f44c9 100644
>>> --- a/src/mesa/main/texobj.c
>>> +++ b/src/mesa/main/texobj.c
>>> @@ -217,7 +217,8 @@ _mesa_get_current_tex_object(struct gl_context *ctx,
>>> GLenum target)
>>>            return ctx->Extensions.ARB_texture_multisample
>>>               ? ctx->Texture.ProxyTex[TEXTURE_2D_MULTISAMPLE_INDEX] :
>>> NULL;
>>>         case GL_TEXTURE_2D_MULTISAMPLE_ARRAY:
>>> -         return ctx->Extensions.ARB_texture_multisample
>>> +         return (ctx->Extensions.ARB_texture_multisample ||
>>> +
>>> ctx->Extensions.OES_texture_storage_multisample_2d_array)
>>
>> And then you don't have to touch this (if you don't add the extra enable)
>
>
> This is used in such a lot of places that I'll need to then add is_gles31()
> though. One should not be able to get this using bare gles 2.0.

errr... how does the context affect what's in Extensions.*? [hint, it
doesn't... if you need those checks now, you also needed them before]

>
>>>               ? texUnit->CurrentTex[TEXTURE_2D_MULTISAMPLE_ARRAY_INDEX] :
>>> NULL;
>>>         case GL_PROXY_TEXTURE_2D_MULTISAMPLE_ARRAY:
>>>            return ctx->Extensions.ARB_texture_multisample
>>> @@ -1612,7 +1613,8 @@ _mesa_tex_target_to_index(const struct gl_context
>>> *ctx, GLenum target)
>>>         return ((_mesa_is_desktop_gl(ctx) &&
>>> ctx->Extensions.ARB_texture_multisample) ||
>>>                 _mesa_is_gles31(ctx)) ? TEXTURE_2D_MULTISAMPLE_INDEX: -1;
>>>      case GL_TEXTURE_2D_MULTISAMPLE_ARRAY:
>>> -      return _mesa_is_desktop_gl(ctx) &&
>>> ctx->Extensions.ARB_texture_multisample
>>> +      return ((_mesa_is_desktop_gl(ctx) &&
>>> ctx->Extensions.ARB_texture_multisample) ||
>>> +              ctx->Extensions.OES_texture_storage_multisample_2d_array)
>>
>> or this
>
>
> Same here, is_gles31() check needed without separate extension enable.

Separate enable has got nothing to do with it. It's enabled
irrespective of the context.

  -ilia