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

[Brian Paul]

On 03/28/2016 10:38 AM, Ilia Mirkin wrote:
> 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?

Yes, thanks.

-Brian

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