[Piglit] [PATCH] winsys-framework Default to showing window

[Ilia Mirkin]

Most tests do indeed use an FBO, either directly or via the fbo
winsys, and all will be well. However some don't (glean, probably
others) because they were written in the stone ages. Those are the
ones that have problems.

Perhaps we should just suck up the slower-ness and use that as an
incentive to finally convert those tests over to The New Way (tm).

Unfortunately I'm ill-equipped to either agree or disagree with your
claim that "using the default framebuffer without mapping it is
undefined behavior". Hopefully someone "in the know" can comment?

  -ilia


On Fri, Aug 14, 2015 at 7:27 PM, Alexander Goins <agoins at nvidia.com> wrote:
> From NVIDIA's side, I don't think it would be that big of a problem to just keep running our tests with PIGLIT_FORCE_WINDOW=1 or -fbo. However, my reasoning is that Piglit should default to the 'correct' behavior according to the OpenGL spec. While the NVIDIA driver is the only one I know of that actually enforces pixel ownership, any driver theoretically could do so while still adhering to the spec.
>
> I agree that having a window pop up for every test is annoying, but using the default framebuffer without mapping it is undefined behavior. The 'correct' way to do offscreen rendering would be using FBOs which Piglit supports with the -fbo argument. Is there a reason why this can't/shouldn't be used for running Piglit offscreen? I originally removed the ability to render offscreen with the default framebuffer altogether, but I figured some users might still desire this behavior so instead decided to leave it in as a non-default option.
>
> Thanks,
> Alex
>
> -----Original Message-----
> From: ibmirkin at gmail.com [mailto:ibmirkin at gmail.com] On Behalf Of Ilia Mirkin
> Sent: Friday, August 14, 2015 8:07 AM
> To: Alexander Goins
> Cc: piglit at lists.freedesktop.org
> Subject: Re: [Piglit] [PATCH] winsys-framework Default to showing window
>
> Interesting. PIGLIT_FORCE_WINDOW was introduced in this change:
>
> commit b797406fb850c5e3c9fdaaebae36127b5e5f8954
> Author: Paul Berry <stereotype441 at gmail.com>
> Date:   Tue Jun 11 16:34:32 2013 -0700
>
>     Add PIGLIT_FORCE_WINDOW environment variable.
>
>     Setting PIGLIT_FORCE_WINDOW=1 causes all piglit windowsystem tests
>     (those that weren't invoked with the "-fbo" flag) to show the window
>     prior to calling piglit_display().  Setting PIGLIT_FORCE_WINDOW=0 (the
>     default) preserves piglit's existing behaviour, which is to only show
>     the window when the test is run in "manual" mode (with no "-auto"
>     argument supplied).
>
>     Setting PIGLIT_FORCE_WINDOW=1 slows down the piglit run, and produces
>     more annoying windows on the screen, but it has been observed to
>     produce more repeatable results when testing with the nVidia
>     proprietary driver for Linux.
>
>     Reviewed-by: Chad Versace <chad.versace at linux.intel.com>
>
> Which explicitly mentions the nvidia blob driver thing. Having more windows popping up during piglit runs is going to be annoying... and slowing things down will also be unwelcome. Is there anything else that can be done here?
>
>   -ilia
>
> On Thu, Aug 13, 2015 at 1:49 PM, agoins <agoins at nvidia.com> wrote:
>> winsys-framework incorrectly assumed that it will always have
>> ownership of the pixels in its own buffers. If using the default
>> framebuffer (i.e., when Piglit is not running in FBO mode,) ownership
>> will not be acquired unless the window is mapped.
>>
>> While this is not typically a problem because buffers are separate,
>> NVIDIA's Unified Back Buffer feature makes all windows share a back
>> buffer while still conforming to the OpenGL spec. If Piglit does not
>> establish pixel ownership, its output will be clobbered by other windows.
>>
>> While this problem could be easily fixed by specifying
>> PIGLIT_FORCE_WINDOW=1 or -fbo, the current default takes advantage of
>> undefined behavior in the OpenGL spec. A better solution would be to
>> replace PIGLIT_FORCE_WINDOW with PIGLIT_NO_WINDOW, a flag that allows
>> tests to be run without mapping a window or using an FBO. The default
>> behavior, the, would be to map a window. Really, though, if users want
>> to test with offscreen rendering, they should use FBOs with flag -fbo.
>>
>> More information on pixel ownership here:
>> https://www.opengl.org/documentation/specs/version1.1/glspec1.1/node94
>> .html
>> ---
>>  .../piglit-framework-gl/piglit_winsys_framework.c    | 20 ++++++++++----------
>>  1 file changed, 10 insertions(+), 10 deletions(-)
>>
>> diff --git a/tests/util/piglit-framework-gl/piglit_winsys_framework.c
>> b/tests/util/piglit-framework-gl/piglit_winsys_framework.c
>> index c80e972..fcba91e 100644
>> --- a/tests/util/piglit-framework-gl/piglit_winsys_framework.c
>> +++ b/tests/util/piglit-framework-gl/piglit_winsys_framework.c
>> @@ -53,18 +53,18 @@ run_test(struct piglit_gl_framework *gl_fw,
>>           int argc, char *argv[])
>>  {
>>         struct piglit_winsys_framework *winsys_fw = piglit_winsys_framework(gl_fw);
>> -       bool force_window = false;
>> -       const char *env_force_window = getenv("PIGLIT_FORCE_WINDOW");
>> +       bool no_window = false;
>> +       const char *env_no_window = getenv("PIGLIT_NO_WINDOW");
>>
>>
>> -       if (env_force_window != NULL) {
>> -               if (strcmp(env_force_window, "0") == 0) {
>> -                       force_window = false;
>> -               } else if (strcmp(env_force_window, "1") == 0) {
>> -                       force_window = true;
>> +       if (env_no_window != NULL) {
>> +               if (strcmp(env_no_window, "0") == 0) {
>> +                       no_window = true;
>> +               } else if (strcmp(env_no_window, "1") == 0) {
>> +                       no_window = true;
>>                 } else {
>> -                       fprintf(stderr, "PIGLIT_FORCE_WINDOW has invalid"
>> -                               " value: %s\n", env_force_window);
>> +                       fprintf(stderr, "PIGLIT_NO_WINDOW has invalid"
>> +                               " value: %s\n", env_no_window);
>>                         abort();
>>                 }
>>         }
>> @@ -73,7 +73,7 @@ run_test(struct piglit_gl_framework *gl_fw,
>>                 gl_fw->test_config->init(argc, argv);
>>
>>         if (!gl_fw->test_config->requires_displayed_window &&
>> -           piglit_automatic && !force_window) {
>> +           piglit_automatic && no_window) {
>>                 enum piglit_result result = PIGLIT_PASS;
>>                 if (gl_fw->test_config->display)
>>                         result = gl_fw->test_config->display();
>> --
>> 1.9.1
>> New to the project - please commit for me.
>>
>> _______________________________________________
>> Piglit mailing list
>> Piglit at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/piglit
>
> -----------------------------------------------------------------------------------
> This email message is for the sole use of the intended recipient(s) and may contain
> confidential information.  Any unauthorized review, use, disclosure or distribution
> is prohibited.  If you are not the intended recipient, please contact the sender by
> reply email and destroy all copies of the original message.
> -----------------------------------------------------------------------------------