[Piglit] [PATCH 1/4] arb_vertex_attrib_64bit: Adds double vertex input tests generator (v2)

Alejandro Piñeiro apinheiro at igalia.com
Fri May 20 09:37:17 UTC 2016


On 20/05/16 10:25, Andres Gomez wrote:
> On Fri, 2016-05-20 at 10:00 +0200, Alejandro Piñeiro wrote:
>> On 20/05/16 01:25, Andres Gomez wrote:
>> Really nitpicky nitpick: when v2 patches are sent to the list, "v2"
>> is
>> included on the prefix on blocks. So "[PATCH 1/4 v2]", not a "(v2)"
>> suffix at the end. Usually it is easier to just let use git-send do
>> that
>> work for you (git send-email -v2).
> Ah, right! Thanks for pointing this out! :)
>
> [skip]
>
>>> +# Hard limit so we don't generate useless tests that cannot be run
>>> in any existing HW.
>>> +MAX_VERTEX_ATTRIBS = 32
>> Any reason to use a hardcode value instead of just asking it before
>> generating it? I think that this value would be too big for older hw,
>> so
>> you would get those useless tests there. Unless the idea is having
>> tests
>> failing on older hw due, ans fully passing only on newer hw.
> This hard code value hints about the current maxim capacity of any
> existing GPU, in order to not create useless tests for any GPU.
>
> The shader runner is the one querying for the capacity of the GPU and
> will skip the test if it needs more VS inputs than what the HW can
> handle.
>
> In other words, the generation of the tests outputs always the same
> tests (as it should). In a cutting-edge GPU allowing 32 VS inputs, all
> the tests would be run. In an "older" GPU allowing less than 32 VS
> inputs, the tests that use more VS inputs than the ones the HW can
> handle will be skipped.

Ah ok, makes sense.

>
> [skip]
>
>>> +                # dvec* and dmat* didn't appear in GLSL until 4.20
>>> +                if (in_type.startswith('dvec') or
>>>>> in_type.startswith('dmat')) and ver == '410':
>>> +                    ver = '420'
>> I don't get this. So if we are using dvecs or dmats on 410, we just
>> "upgrade" the version to be 420? Why not just assume that 410 is
>> wrong
>> (see below for further comments on this)?
> [skip]
>
>>> +
>>> +        # We need additional directories for GLSL 420
>>> +        if not names_only:
>>> +            utils.safe_makedirs(TestTuple.get_dir_name('420'))
>>> +        for test_args in (list(RegularTestTuple.create_tests(
>>> +                ['GL_ARB_vertex_attrib_64bit', '410'],
>> What is the reason to call create_tests with 410. Shouldn't it be
>> 420?
>> As far as I understand this call, it is creating tests if the
>> extension
>> is supported for any GL version, or on the gl version that supports
>> it
>> on core. But this functionality is not part of the core until 420.
>> Why
>> not calling with 420 instead of 410? And after all, as Im saying on
>> my
>> previous comment, you are tweaking the version anyway.
> [skip]
>
>>>>> +        assert ver in ('GL_ARB_vertex_attrib_64bit', '410',
>>>>> '420')
>> Related with my previous comment, why not just
>> ('GL_ARB_vertex_attrib_64bit', '420')? What having 410 there
>> provides?
> doubles support appears in 4.10
> dvec and dmat support appears in 4.20
>
> If you check the output, you will see that tests are generated for
> both, not just for 4.20.

Ok, after our IRC chat, I understand my confusion. You were talking
about GLSL version and I was thinking on GL core version (even although
you explicitly put on code GLSL version). I assumed that with
GLSLversion 410 it would get all the functionality of gl core 4.10, and
not doubles but not dmats (/me shrugs).

So get an:
Acked-by: Alejandro Piñeiro <apinheiro at igalia.com>

Ack instead of reviewed basically due my lack of experience on python
and mako templates. But in general the tests seems reasonable to me.

BR




More information about the Piglit mailing list