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 <apinhe...@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
_______________________________________________
Piglit mailing list
Piglit@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/piglit
