[Mesa-dev] [PATCH v4 3/6] mesa/es3.1: enable GL_ARB_texture_multisample for GLES 3.1

[Ilia Mirkin]

On Wed, Jul 22, 2015 at 2:55 PM, Ian Romanick <idr at freedesktop.org> wrote:
> On 06/26/2015 07:38 AM, Ilia Mirkin wrote:
>> On Fri, Jun 26, 2015 at 4:18 AM, Tapani Pälli <tapani.palli at intel.com> wrote:
>>>
>>>
>>> On 06/26/2015 01:06 AM, Ilia Mirkin wrote:
>>>>
>>>> On Thu, Jun 25, 2015 at 4:22 PM, Ilia Mirkin <imirkin at alum.mit.edu> wrote:
>>>>>
>>>>> On Thu, Jun 25, 2015 at 5:08 AM, Marta Lofstedt
>>>>> <marta.lofstedt at linux.intel.com> wrote:
>>>>>>
>>>>>> From: Marta Lofstedt <marta.lofstedt at intel.com>
>>>>>>
>>>>>> v4 : only expose GL_ARB_texture_multisample enums
>>>>>> for gles 3.1 and desktop GL.
>>>>>
>>>>>
>>>>> I was suspicious of this logic. Based on my reading of the code, what
>>>>> your ARB_texture_multisample_es31 thing does is expose those enums
>>>>> when *either* the driver enables ARB_texture_multisample *or* the
>>>>> current context is ES3.1.
>>>>>
>>>>> ARB_draw_indirect_es31 has the same problem, btw.
>>>
>>>
>>> I'm not sure I understand the issue. It should work fine with 'OR', if we
>>> have either of these then we expose the enum?
>>>
>>>>> I could have misread the get.c extra_ext() logic, but I don't think I
>>>>> have. As far as I can tell there's no way to (generically) AND these
>>>>> things.
>>>
>>>
>>> Why do we need AND? If you have 3.1 context then you *need* to support this
>>> enum. There is no such condition where you would have gles 3.1 context but
>>> not the enum, right?
>>>
>>>
>>>>> What you really need to do is create a whole new [GL, GL_CORE, ES31]
>>>>> section in get_hash_params and update get_hash_generator.py
>>>>> accordingly.
>>>>
>>>>
>>>> An alternative, BTW, is to just say "screw it" and expose the enums in
>>>> GL ES 3.0. In that case, just move the enums into a section that
>>>> includes GLES3. I'm not sure how big of a deal it is. But your current
>>>> patches are definitely wrong.
>>>
>>>
>>> We do this, but we have explicit version check against version 3.1.
>>
>> OK, well, at the end of the day, the things anyone says don't matter,
>> it just has to work, right? So here are the scenarios:
>>
>> Let's take it as a given that the driver sets
>> Ext.ARB_texture_multisample to true, we have the following situations:
>>
>> GL compat glGet(SAMPLE_MASK)
>> GL core glGet(SAMPLE_MASK)
>> GL ES3.0 glGet(SAMPLE_MASK)
>> GL ES3.1 glGet(SAMPLE_MASK)
>>
>> Where the ES3.0 one should fail, and all the others should succeed.
>> OK, so first off, this is in the GL/GL_CORE/GLES3 section, which means
>> that the logic will get hit for all 4, right? Then you do a check for
>
> Yes, I think you're right, and I think I led Marta and Tapani astray.  I
> think they originally had separate sections, and I told them to change
> at least some of them.  The whole separate section mechanism is
> confusing and leads to code churn (imagine if an ES3.0 extension comes
> along that enables this functionality).
>
> I think a good Python project for someone would be to reorganize that
> file so that all the enums are in a single list.  Each enum would have a
> list of minterms to describe its availability.  These would include
> profile and version.  Instead of having a flat array of integers, we'd
> have an array of structures:
>
> struct requirement {
>     gl_api  API;
>     unsigned Version;       /**< Minimum version required. */
>     unsigned extension[2];  /**< Offsets of required extensions.  Use
>                              * dummy_true if none.
>                              */
>     enum value_extra extra; /**< Other requirements. */
> };
>
> This is equivalent to the current system (which is also a maximum of
> minterms), but I think it's a lot easier to specify things.
>
> Either way, I think that can be follow-up work.

We currently have varying levels of correctness wrt exposing features
we're not supposed to expose. I haven't thought about your new
dispatch stuff, but would, e.g., glPolygonOffsetClampEXT be
retrievable in an ES context right now? If it would be, do we care?

I feel like a bit of a troll when I'm pointing out these
inconsistencies since they're fairly trivial, but I also see that some
thought and effort has gone into hiding various functionality where
it's not supposed to exist.

  -ilia