[Mesa-dev] [PATCH 1/4] mesa: properly return GetTexLevelParameter queries for buffer textures

[Ilia Mirkin]

On Mon, Mar 28, 2016 at 12:31 PM, Brian Paul <brianp at vmware.com> wrote:
> On 03/28/2016 10:20 AM, Ilia Mirkin wrote:
>>
>> Thanks! On the off chance you're in the mood for reviewing the rest of
>> the series but it's lost in your inbox, you can find it here:
>>
>>
>> https://urldefense.proofpoint.com/v2/url?u=https-3A__patchwork.freedesktop.org_series_3887_&d=BQIFaQ&c=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEs&r=T0t4QG7chq2ZwJo6wilkFznRSFy-8uDKartPGbomVj8&m=0xRgcNjzm9L4CxMt1DG9coPwumjYve81_bkYgcQA6-0&s=_fp9_BM7DpwjSOJAM3dQIDF02eQ7DCBbJxs8Yl8PsxE&e=
>>
>> The later patches actually pipe through GL_EXT/OES_texture_buffer
>> support so that the dEQP tests can run on mesa. (All the current dEQP
>> tests are around AEP, so they like the EXT variants.)
>
>
>
> Overall, the series looks OK to me.  But I have a question.
> GL_OES/EXT_texture_buffer is only for ES, right?  In the GLSL code, don't we
> have to check for that, in addition to the GLSL version?  I'm looking at
> your new texture_buffer() helper function in
> src/compiler/glsl/builtin_functions.cpp
>
> Reviewed-by: Brian Paul <brianp at vmware.com>
>

Presumably you're referring to

 static bool
+texture_buffer(const _mesa_glsl_parse_state *state)
+{
+   return state->is_version(140, 320) ||
+      state->EXT_texture_buffer_enable ||
+      state->OES_texture_buffer_enable;
+}

and the question is whether this shouldn't be more like

  state->es_shader && state->EXT_foo_enable?

If so, I don't think so. Those exts are marked as ES-only in the
_mesa_glsl_supported_extensions (glsl_parser_extras.cpp), which means
that they should only be enable-able from an ES shader. So if you're
in a non-ES shader, those two enables should always be false (or else
we have larger problems).

Does that address your concern?

I guess there's a question here of what happens if you write a ESSL
1.0 shader and enable those exts (which require ESSL 3.10)... I think
it's supposed to fail, but mesa will be none the wiser and expose the
relevant functionality. The ext definitions in glsl_parser_extras
should probably have minimum GL/GLES versions, but those don't exist
right now. IMHO this isn't such a big deal.

  -ilia