Re: [Piglit] [PATCH 1/2] GLX_OML_sync_control: Add new tests.

[Eric Anholt]

Ian Romanick <i...@freedesktop.org> writes:

> On 10/19/2012 11:13 AM, Eric Anholt wrote:
>> I was going to go touch this code in Mesa, then I realised that we
>> didn't have a single test for it.
>>
>> v2: Rebase on Chad's BUILD_* sedjob, and explicitly test various swap 
>> intervals.
>
> A couple comments below...

>> diff --git a/tests/spec/glx_oml_sync_control/swapbuffersmsc-divisor-zero.c 
>> b/tests/spec/glx_oml_sync_control/swapbuffersmsc-divisor-zero.c
>> new file mode 100644
>> index 0000000..247c130
>> --- /dev/null
>> +++ b/tests/spec/glx_oml_sync_control/swapbuffersmsc-divisor-zero.c

>> +    glXGetSyncValuesOML(dpy, win, &start_ust, &start_msc, &start_sbc);
>> +    if (start_sbc != 0) {
>> +            fprintf(stderr,
>> +                    "Initial SBC for the window should be 0, was %lld\n",
>> +                    (long long)start_sbc);
>> +            piglit_report_result(PIGLIT_FAIL);
>> +    }
>
> Since this appears in every test, maybe make a 
> get_and_validate_initial_sync_values helper function?

In 2 out of the 4 tests, and refactoring would be at least as many LOC
and less code locality.  No thanks.

>> diff --git a/tests/spec/glx_oml_sync_control/swapbuffersmsc-return.c 
>> b/tests/spec/glx_oml_sync_control/swapbuffersmsc-return.c
>> new file mode 100644
>> index 0000000..92f34ea
>> --- /dev/null
>> +++ b/tests/spec/glx_oml_sync_control/swapbuffersmsc-return.c

>> +    do {
>> +            glXGetSyncValuesOML(dpy, win, &start_ust, &start_msc, 
>> &start_sbc);
>> +
>> +            /* Wait for the MSC to be at least equal to target,
>> +             * with no divisor trickery.
>> +             */
>> +            target_msc = start_msc + 5;
>> +            glXWaitForMscOML(dpy, win, target_msc, 0, 0,
>> +                             &wait_ust, &wait_msc, &wait_sbc);
>> +
>> +            if (i++ >= 5) {
>> +                    fprintf(stderr,
>> +                            "current_msc is returning bad values "
>> +                            "(%lld, start value was %lld\n",
>> +                            (long long)start_msc,
>> +                            (long long)wait_msc);
>> +                    piglit_report_result(PIGLIT_FAIL);
>> +            }
>> +    } while (wait_msc < start_msc);
>
> There's something about this loop that I don't grok.  Why try 5 times to 
> see if glXWaitForMscOML can wait 5 vblanks?  How can wait_msc ever be 
> less than start_msc?

It was just the wrapping test. I rewrote it to be more like
swapbuffersmsc-divisor-zero for clarity.

>> +    if (wait_msc < target_msc) {
>> +            fprintf(stderr,
>> +                    "glXWaitForMscOML() returned msc of %lld, "
>> +                    "expected >= %lld\n",
>> +                    (long long)wait_msc,
>> +                    (long long)target_msc);
>> +    }
>> +
>> +    glXGetSyncValuesOML(dpy, win,
>> +                        &current_ust, &current_msc, &current_sbc);
>> +
>> +    if (current_msc < target_msc) {
>
> Maybe 'if (current_msc < wait_msc)' instead?

I ended up moving this into the wrapping test block in the rewrite.

pgp5P5pvfMlYN.pgp
Description: PGP signature

_______________________________________________
Piglit mailing list
Piglit@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/piglit