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

[Ilia Mirkin]

On Wed, Aug 26, 2015 at 5:03 AM, Tapani Pälli <tapani.palli at intel.com> wrote:
> 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.

Yeah, that somebody was me, if you recall. And I strongly argued
against the _es31 stuff when it was first coming in too. But then I
looked away, and this stuff got pushed. Same as with the ARB_dsa stuff
when I kept saying that the piglits had to be core and then eventually
gave up, and it caused a giant thing at mesa 10.6 release time.

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

I think from your next email you've come around to understanding what
all I've been saying, but just in case, let me restate:

Old check: ARB_texture_multisample || es31

es31 is set based on either a cmdline override (in which case I care
less about compliance) or based on the condition in
compute_version_es2. This condition will only be true if the various
extensions are there, so logically es31 is an && of the various exts.
[And also whether an actual ES31 context was selected.]

Therefore the || es31 does nothing. Literally nothing. You can't
*exclude* GLES2.0 by adding an || condition to the availability, and
in this case, ES31 *implies* that ARB_texture_multisample will have
been set, since no driver will have an ES31 context but not have
ARB_texture_multisample. The only thing it can do is *add*, and it
doesn't increase the number of states that will pass through the
condition for any practical scenario.

In case you're still unconvinced, come up with a scenario and show me
which line of code is executed with the _es31 thing but not without.
Outside of using cmdline overrides on a driver that has no business
exposing ES3.1 in the first place, I don't think that will ever
happen.

  -ilia