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

Wed Aug 26 02:03:13 PDT 2015

On 08/26/2015 08:26 AM, Ilia Mirkin wrote:
> 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.

Not ignored, Ian stated that we have to see case by case and most of the 
cases I've seen require checks. We should not be able to query this enum 
using GLES 3.0. Doing the '_es31 way' we make sure that these exist only 
for 3.1. We changed to this model when somebody pointed out that in 
earlier model we exposed enums for GLES 2.0 apps.

>> 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).

OK. Maybe we've been too protective then if it works and I don't think 
we have any 'negative tests' to for example use ES 3.1 enums in a GLES 
2.0 context. Program would need to strange anyway to either include ES 
3.1 headers or use hardcoded values in the calls. I don't have a strong 
opinion, I'm somewhat disappointed that we are changing this once again. 
Remember, first we did this loosely by exposing the enums for anyone. 
Then we made more strict check by adding _es31 so that we would not 
expose enums in wrong contexts and now we are back again giving them for 
everyone?

>>>> +
>>>>    # 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]

Ah right, it affects only the extensions string. Then the context 
version check has probably existed in the functions calling this.

>>>>                ? 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.

Yeah it was broken, check is needed because of the is_desktop_gl(ctx) 
check. Now we want to enable it on GLES but only on >= 3.1.

>    -ilia

// Tapani