[Mesa-dev] [PATCH v4] mesa: enable enums for OES_geometry_shader

[Ilia Mirkin]

On Thu, Jan 21, 2016 at 8:28 AM, Lofstedt, Marta
<marta.lofstedt at intel.com> wrote:
>
>
>> -----Original Message-----
>> From: ibmirkin at gmail.com [mailto:ibmirkin at gmail.com] On Behalf Of Ilia
>> Mirkin
>> Sent: Thursday, January 21, 2016 2:03 PM
>> To: Lofstedt, Marta
>> Cc: Marta Lofstedt; mesa-dev at lists.freedesktop.org
>> Subject: Re: [PATCH v4] mesa: enable enums for OES_geometry_shader
>>
>> On Thu, Jan 21, 2016 at 7:57 AM, Lofstedt, Marta <marta.lofstedt at intel.com>
>> wrote:
>> >> -----Original Message-----
>> >> From: ibmirkin at gmail.com [mailto:ibmirkin at gmail.com] On Behalf Of
>> >> Ilia Mirkin
>> >> Sent: Thursday, January 21, 2016 1:46 PM
>> >> To: Lofstedt, Marta
>> >> Cc: Marta Lofstedt; mesa-dev at lists.freedesktop.org
>> >> Subject: Re: [PATCH v4] mesa: enable enums for OES_geometry_shader
>> >>
>> >> On Thu, Jan 21, 2016 at 7:41 AM, Lofstedt, Marta
>> >> <marta.lofstedt at intel.com>
>> >> wrote:
>> >> >> -----Original Message-----
>> >> >> From: ibmirkin at gmail.com [mailto:ibmirkin at gmail.com] On Behalf Of
>> >> >> Ilia Mirkin
>> >> >> Sent: Thursday, January 21, 2016 1:25 PM
>> >> >> To: Marta Lofstedt
>> >> >> Cc: mesa-dev at lists.freedesktop.org; Lofstedt, Marta
>> >> >> Subject: Re: [PATCH v4] mesa: enable enums for
>> OES_geometry_shader
>> >> >>
>> >> >> On Thu, Dec 10, 2015 at 11:11 AM, Marta Lofstedt
>> >> >> <marta.lofstedt at linux.intel.com> wrote:
>> >> >> > +      case EXTRA_EXT_GPU5_GS:
>> >> >> > +         api_check = GL_TRUE;
>> >> >> > +         api_found = (ctx->Extensions.ARB_gpu_shader5 ||
>> >> >> > +                      _mesa_has_OES_geometry_shader(ctx));
>> >> >> > +         break;
>> >> >> > +      case EXTRA_EXT_VIEWPORT_GS:
>> >> >> > +         api_check = GL_TRUE;
>> >> >> > +         api_found = (ctx->Extensions.ARB_viewport_array ||
>> >> >> > +                      _mesa_has_OES_geometry_shader(ctx));
>> >> >> > +         break;
>> >> >>
>> >> >> You can do these without the special tokens. Or did you mean &&
>> here?
>> >> >
>> >> > I am pretty sure that our previous discussions on this topic ended
>> >> > up with
>> >> || to be preferable in these cases, but if you want && I will change.
>> >>
>> >> I actually don't want either. What I'm saying is that if you want ||,
>> >> then you don't have to add these EXTRA_EXT_GPU5_GS things -- using
>> >> the regular mechanism for composing tokens will get you ||. You only
>> >> need to use these special tokens if you want &&.
>> >>
>> > If by the "regular" mechanism mean:
>> > +static const int extra_ARB_viewport_array_or_geometry_shader[] = {
>> > +   EXT(ARB_viewport_array),
>> > +   EXT(OES_geometry_shader),
>> > +   EXTRA_END
>> > +};
>> > I had that in an earlier patch, where I interpreted your comment as a
>> rejection.
>>
>> Nope, that's precisely what I mean. The only distinction between the two is
>> the extra check for the ES context. If you want to preserve that, just create a
>> single EXTRA_EXT_ES_GS token which just does
>>
>> api_found = _mesa_has_OES_geometry_shader(ctx)
>>
>> and then you can use that instead of EXT(OES_geometry_shader). The
>> distinction is pretty minor, of course. Exposing one or two enums that you're
>> supposed to error on is probably not the end of the world.
>> Either way is good with me though.
>>
> So, to check if we understand each other. Would it be OK to you, if I replace:
> EXTRA_EXT_VIEWPORT_GS and EXTRA_EXT_GPU5_GS, both in value_extra and the check_extra(...) switch,
>
> with: EXTRA_EXT_ES_GS  in value_extra and according to your suggestion above in check_extra(...)
>
> And use EXTRA_EXT_ES_GS instead of EXT(OES_geometry_shader) in:
> extra_ARB_viewport_array_or_geometry_shader[] and similar to:
> extra_ARB_gpu_shader5_and_geometry_shader []

Yep, that's precisely my suggestion. I think we understand each other perfectly.

I hope you're not seeing this as random busy-work -- if you are,
please speak up -- but I think it's best to minimize the number of the
custom tokens to the ones that we *actually* need. Otherwise there's
too many ways to do the same thing, and the getter logic doesn't need
any more of those!

Cheers,

  -ilia