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

[Ilia Mirkin]

On Thu, Jan 21, 2016 at 9:14 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:32 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 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.
>>
> That's good. I now have this on my github:
> https://github.com/MartaLo/mesa/tree/oes_geometry_shader_Ilia_enums

That part looks good. I noticed you also have


 +  ["MAX_FRAMEBUFFER_LAYERS",
"CONTEXT_INT(Const.MaxFramebufferLayers),
extra_ARB_framebuffer_no_attachments_and_geometry_shader"],

which is missing a space after [ and before ]. And I think in one of
your other patches, you were missing a space between if and ().

In all, I think this would benefit from having the actual patches
being resent to list, along with a note not unlike in your earlier
email which is that "these have been previously reviewed but want to
give people some time to provide more feedback, will push in X time
unless I hear otherwise".

Cheers,

  -ilia