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

[Andres Gomez]

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.

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

---

Thanks for the review!

-- 
Br,

Andres