[Mesa-dev] [PATCH 1/3] mesa: return 0 for GL_INTERNALFORMAT_SUPPORTED for unsupported TBO formats

[Alejandro Piñeiro]

On 24/10/17 12:18, Marek Olšák wrote:
> On Tue, Oct 24, 2017 at 12:09 PM, Alejandro Piñeiro
> <apinheiro at igalia.com> wrote:
>> On 24/10/17 11:53, Marek Olšák wrote:
>>> On Tue, Oct 24, 2017 at 11:47 AM, Alejandro Piñeiro
>>> <apinheiro at igalia.com> wrote:
>>>> On 21/10/17 14:54, Marek Olšák wrote:
>>>>> From: Marek Olšák <marek.olsak at amd.com>
>>>>>
>>>>> ---
>>>>>  src/mesa/main/formatquery.c | 5 +++++
>>>>>  1 file changed, 5 insertions(+)
>>>>>
>>>>> diff --git a/src/mesa/main/formatquery.c b/src/mesa/main/formatquery.c
>>>>> index 77c7faa..05b7810 100644
>>>>> --- a/src/mesa/main/formatquery.c
>>>>> +++ b/src/mesa/main/formatquery.c
>>>>> @@ -895,20 +895,25 @@ _mesa_GetInternalformativ(GLenum target, GLenum internalformat, GLenum pname,
>>>>>        if (pname == GL_NUM_SAMPLE_COUNTS && ctx->API == API_OPENGLES2 &&
>>>>>            ctx->Version == 30 && _mesa_is_enum_format_integer(internalformat)) {
>>>>>           goto end;
>>>>>        }
>>>>>
>>>>>        ctx->Driver.QueryInternalFormat(ctx, target, internalformat, pname,
>>>>>                                        buffer);
>>>>>        break;
>>>>>
>>>>>     case GL_INTERNALFORMAT_SUPPORTED:
>>>>> +      /* Reject invalid texture buffer formats. */
>>>>> +      if (target == GL_TEXTURE_BUFFER &&
>>>>> +          _mesa_validate_texbuffer_format(ctx, internalformat) == MESA_FORMAT_NONE)
>>>>> +         break;
>>>>> +
>>>> _mesa_validate_texbuffer_format with that internalformat if target is
>>>> GL_TEXTURE_BUFFER is already called on _is_resource_supported, on line
>>>> 864. Why it is needed a second call here?
>>>>
>>> See the beginning of _is_resource_supported.
>> Ah true. internalformat_query2, that delightful spec. I'm starting to
>> remember. So from the spec:
>>
>> "
>>
>>     - INTERNALFORMAT_SUPPORTED: If <internalformat> is an internal format
>>       that is supported by the implementation in at least some subset of
>>       possible operations, TRUE is written to <params>.  If <internalformat>
>>       if not a valid token for any internal format usage, FALSE is returned."
>>
>> This pname is asking <internalformat> is supported somehow, on at least
>> one operation. And note that it says "format", and not "resource" that
>> in the wording of this spec for the pair target/internalformat.
>>
>> Reading it literally, that means that this pname query is independent of
>> the target.
>>
>> So, under that interpretation of your spec, your patch is wrong, as you
>> are checking against a specific target, but it should return TRUE as far
>> as it is supported for one case.
> Are the piglit tests wrong then?

We took that into account when we wrote the piglit tests. So in general
they should be right. As usual, perhaps there are bugs there.

BR