all.py: add run_concurrent=False where needed

Submitted by Arthur Huillet on Nov. 14, 2018, 12:41 p.m.

Details

Message ID 20181114124104.15687-1-arthur.huillet@free.fr
State New
Headers show
Series "all.py: add run_concurrent=False where needed" ( rev: 1 ) in Piglit

Not browsing as part of any series.

Commit Message

Arthur Huillet Nov. 14, 2018, 12:41 p.m.
From: Arthur Huillet <ahuillet@nvidia.com>

Commit 57537d45b75218438716506594e16b91dade968f removed run_concurrent=False
from a bunch of tests, saying there was no reason for them to be marked as
such. This is wrong for at least two tests, which require a displayed window
(cannot render to an FBO).  As such, when run concurrently they are liable to
write over each other's pixels if the windows overlap, which can happen on some
setups.

This change fixes the problem by marking clear-accum and masked-clear as
run_concurrent=False. It is likely to be the correct thing to do for all tests
that require a displayed window for the same reason, but these two are the only
one that were so far identified by NVIDIA.
---
 tests/all.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Patch hide | download patch | download mbox

diff --git a/tests/all.py b/tests/all.py
index 4cd911fab..e1b182f15 100644
--- a/tests/all.py
+++ b/tests/all.py
@@ -779,7 +779,7 @@  with profile.test_list.group_manager(
     g(['gl-1.1-read-pixels-after-display-list'])
     g(['gl-1.1-set-vertex-color-after-draw'])
     g(['array-stride'])
-    g(['clear-accum'])
+    g(['clear-accum'], run_concurrent=False)
     g(['clipflat'])
     g(['copypixels-draw-sync'])
     g(['copypixels-sync'])
@@ -813,7 +813,7 @@  with profile.test_list.group_manager(
     g(['lineloop', '-dlist'], 'lineloop-dlist')
     g(['linestipple'], run_concurrent=False)
     g(['longprim'])
-    g(['masked-clear'])
+    g(['masked-clear'], run_concurrent=False)
     g(['point-line-no-cull'])
     g(['polygon-mode'])
     g(['polygon-mode-facing'])

Comments

Ping. Pixel ownership test makes it impossible for Piglit to blindly 
assume that it can read back all the pixels of its backbuffer, so tests 
that don't use an FBO can't be run concurrently unless it can be ensured 
that the windows do not overlap.
One reason this seems to only show up on NVIDIA driver is that we have a 
unified backbuffer under some circumstances.

Thanks
--
Arthur

On 14.11.2018 13:41, arthur.huillet@free.fr wrote:
> From: Arthur Huillet <ahuillet@nvidia.com>
> 
> Commit 57537d45b75218438716506594e16b91dade968f removed 
> run_concurrent=False
> from a bunch of tests, saying there was no reason for them to be marked 
> as
> such. This is wrong for at least two tests, which require a displayed 
> window
> (cannot render to an FBO).  As such, when run concurrently they are 
> liable to
> write over each other's pixels if the windows overlap, which can happen 
> on some
> setups.
> 
> This change fixes the problem by marking clear-accum and masked-clear 
> as
> run_concurrent=False. It is likely to be the correct thing to do for 
> all tests
> that require a displayed window for the same reason, but these two are 
> the only
> one that were so far identified by NVIDIA.
> ---
>  tests/all.py | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/tests/all.py b/tests/all.py
> index 4cd911fab..e1b182f15 100644
> --- a/tests/all.py
> +++ b/tests/all.py
> @@ -779,7 +779,7 @@ with profile.test_list.group_manager(
>      g(['gl-1.1-read-pixels-after-display-list'])
>      g(['gl-1.1-set-vertex-color-after-draw'])
>      g(['array-stride'])
> -    g(['clear-accum'])
> +    g(['clear-accum'], run_concurrent=False)
>      g(['clipflat'])
>      g(['copypixels-draw-sync'])
>      g(['copypixels-sync'])
> @@ -813,7 +813,7 @@ with profile.test_list.group_manager(
>      g(['lineloop', '-dlist'], 'lineloop-dlist')
>      g(['linestipple'], run_concurrent=False)
>      g(['longprim'])
> -    g(['masked-clear'])
> +    g(['masked-clear'], run_concurrent=False)
>      g(['point-line-no-cull'])
>      g(['polygon-mode'])
>      g(['polygon-mode-facing'])
run_concurrent=False implies that the test will be run with "-fbo".
Looking at clear-accum.c, I don't see anything that will fail to work
when run with an fbo. If piglit_probe_rect_rgb doesn't work with -fbo,
then there are a LOT more tests that will have this problem.

  -ilia

On Tue, Jan 22, 2019 at 5:40 AM Arthur Huillet <arthur.huillet@free.fr> wrote:
>
> Ping. Pixel ownership test makes it impossible for Piglit to blindly
> assume that it can read back all the pixels of its backbuffer, so tests
> that don't use an FBO can't be run concurrently unless it can be ensured
> that the windows do not overlap.
> One reason this seems to only show up on NVIDIA driver is that we have a
> unified backbuffer under some circumstances.
>
> Thanks
> --
> Arthur
>
> On 14.11.2018 13:41, arthur.huillet@free.fr wrote:
> > From: Arthur Huillet <ahuillet@nvidia.com>
> >
> > Commit 57537d45b75218438716506594e16b91dade968f removed
> > run_concurrent=False
> > from a bunch of tests, saying there was no reason for them to be marked
> > as
> > such. This is wrong for at least two tests, which require a displayed
> > window
> > (cannot render to an FBO).  As such, when run concurrently they are
> > liable to
> > write over each other's pixels if the windows overlap, which can happen
> > on some
> > setups.
> >
> > This change fixes the problem by marking clear-accum and masked-clear
> > as
> > run_concurrent=False. It is likely to be the correct thing to do for
> > all tests
> > that require a displayed window for the same reason, but these two are
> > the only
> > one that were so far identified by NVIDIA.
> > ---
> >  tests/all.py | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/tests/all.py b/tests/all.py
> > index 4cd911fab..e1b182f15 100644
> > --- a/tests/all.py
> > +++ b/tests/all.py
> > @@ -779,7 +779,7 @@ with profile.test_list.group_manager(
> >      g(['gl-1.1-read-pixels-after-display-list'])
> >      g(['gl-1.1-set-vertex-color-after-draw'])
> >      g(['array-stride'])
> > -    g(['clear-accum'])
> > +    g(['clear-accum'], run_concurrent=False)
> >      g(['clipflat'])
> >      g(['copypixels-draw-sync'])
> >      g(['copypixels-sync'])
> > @@ -813,7 +813,7 @@ with profile.test_list.group_manager(
> >      g(['lineloop', '-dlist'], 'lineloop-dlist')
> >      g(['linestipple'], run_concurrent=False)
> >      g(['longprim'])
> > -    g(['masked-clear'])
> > +    g(['masked-clear'], run_concurrent=False)
> >      g(['point-line-no-cull'])
> >      g(['polygon-mode'])
> >      g(['polygon-mode-facing'])
>
> --
> A. Huillet
> _______________________________________________
> Piglit mailing list
> Piglit@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/piglit
If I remember correctly, these tests ignore -fbo when passed. At any 
rate, they render to a real window. If you have two of these windows at 
the same time because they are run concurrently, you run into the pixel 
ownership problem which is present everywhere and potentially extra 
painful on the NVIDIA driver.

On 22.01.2019 15:39, Ilia Mirkin wrote:
> run_concurrent=False implies that the test will be run with "-fbo".
> Looking at clear-accum.c, I don't see anything that will fail to work
> when run with an fbo. If piglit_probe_rect_rgb doesn't work with -fbo,
> then there are a LOT more tests that will have this problem.
> 
>   -ilia
> 
> On Tue, Jan 22, 2019 at 5:40 AM Arthur Huillet <arthur.huillet@free.fr> 
> wrote:
>> 
>> Ping. Pixel ownership test makes it impossible for Piglit to blindly
>> assume that it can read back all the pixels of its backbuffer, so 
>> tests
>> that don't use an FBO can't be run concurrently unless it can be 
>> ensured
>> that the windows do not overlap.
>> One reason this seems to only show up on NVIDIA driver is that we have 
>> a
>> unified backbuffer under some circumstances.
>> 
>> Thanks
>> --
>> Arthur
>> 
>> On 14.11.2018 13:41, arthur.huillet@free.fr wrote:
>> > From: Arthur Huillet <ahuillet@nvidia.com>
>> >
>> > Commit 57537d45b75218438716506594e16b91dade968f removed
>> > run_concurrent=False
>> > from a bunch of tests, saying there was no reason for them to be marked
>> > as
>> > such. This is wrong for at least two tests, which require a displayed
>> > window
>> > (cannot render to an FBO).  As such, when run concurrently they are
>> > liable to
>> > write over each other's pixels if the windows overlap, which can happen
>> > on some
>> > setups.
>> >
>> > This change fixes the problem by marking clear-accum and masked-clear
>> > as
>> > run_concurrent=False. It is likely to be the correct thing to do for
>> > all tests
>> > that require a displayed window for the same reason, but these two are
>> > the only
>> > one that were so far identified by NVIDIA.
>> > ---
>> >  tests/all.py | 4 ++--
>> >  1 file changed, 2 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/tests/all.py b/tests/all.py
>> > index 4cd911fab..e1b182f15 100644
>> > --- a/tests/all.py
>> > +++ b/tests/all.py
>> > @@ -779,7 +779,7 @@ with profile.test_list.group_manager(
>> >      g(['gl-1.1-read-pixels-after-display-list'])
>> >      g(['gl-1.1-set-vertex-color-after-draw'])
>> >      g(['array-stride'])
>> > -    g(['clear-accum'])
>> > +    g(['clear-accum'], run_concurrent=False)
>> >      g(['clipflat'])
>> >      g(['copypixels-draw-sync'])
>> >      g(['copypixels-sync'])
>> > @@ -813,7 +813,7 @@ with profile.test_list.group_manager(
>> >      g(['lineloop', '-dlist'], 'lineloop-dlist')
>> >      g(['linestipple'], run_concurrent=False)
>> >      g(['longprim'])
>> > -    g(['masked-clear'])
>> > +    g(['masked-clear'], run_concurrent=False)
>> >      g(['point-line-no-cull'])
>> >      g(['polygon-mode'])
>> >      g(['polygon-mode-facing'])
>> 
>> --
>> A. Huillet
>> _______________________________________________
>> Piglit mailing list
>> Piglit@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/piglit
Huh, interesting. There might be more going on than meets the eye.

        config.requires_displayed_window = true;

I don't know much about accumulators, so ... perhaps they don't work
with fbo the way one might think they do.

masked-clear appears to have the same
config.requires_displayed_window=true and does do front-buffer stuff.

So thinking about this a bit more, I don't think fbo's have a way of
having an accum buffer, so implicitly this would still use the winsys
visual (or plain not work with -fbo). So this is:

Reviewed-by: Ilia Mirkin <imirkin@alum.mit.edu>

On Tue, Jan 22, 2019 at 9:46 AM Arthur Huillet <arthur.huillet@free.fr> wrote:
>
> If I remember correctly, these tests ignore -fbo when passed. At any
> rate, they render to a real window. If you have two of these windows at
> the same time because they are run concurrently, you run into the pixel
> ownership problem which is present everywhere and potentially extra
> painful on the NVIDIA driver.
>
> On 22.01.2019 15:39, Ilia Mirkin wrote:
> > run_concurrent=False implies that the test will be run with "-fbo".
> > Looking at clear-accum.c, I don't see anything that will fail to work
> > when run with an fbo. If piglit_probe_rect_rgb doesn't work with -fbo,
> > then there are a LOT more tests that will have this problem.
> >
> >   -ilia
> >
> > On Tue, Jan 22, 2019 at 5:40 AM Arthur Huillet <arthur.huillet@free.fr>
> > wrote:
> >>
> >> Ping. Pixel ownership test makes it impossible for Piglit to blindly
> >> assume that it can read back all the pixels of its backbuffer, so
> >> tests
> >> that don't use an FBO can't be run concurrently unless it can be
> >> ensured
> >> that the windows do not overlap.
> >> One reason this seems to only show up on NVIDIA driver is that we have
> >> a
> >> unified backbuffer under some circumstances.
> >>
> >> Thanks
> >> --
> >> Arthur
> >>
> >> On 14.11.2018 13:41, arthur.huillet@free.fr wrote:
> >> > From: Arthur Huillet <ahuillet@nvidia.com>
> >> >
> >> > Commit 57537d45b75218438716506594e16b91dade968f removed
> >> > run_concurrent=False
> >> > from a bunch of tests, saying there was no reason for them to be marked
> >> > as
> >> > such. This is wrong for at least two tests, which require a displayed
> >> > window
> >> > (cannot render to an FBO).  As such, when run concurrently they are
> >> > liable to
> >> > write over each other's pixels if the windows overlap, which can happen
> >> > on some
> >> > setups.
> >> >
> >> > This change fixes the problem by marking clear-accum and masked-clear
> >> > as
> >> > run_concurrent=False. It is likely to be the correct thing to do for
> >> > all tests
> >> > that require a displayed window for the same reason, but these two are
> >> > the only
> >> > one that were so far identified by NVIDIA.
> >> > ---
> >> >  tests/all.py | 4 ++--
> >> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >> >
> >> > diff --git a/tests/all.py b/tests/all.py
> >> > index 4cd911fab..e1b182f15 100644
> >> > --- a/tests/all.py
> >> > +++ b/tests/all.py
> >> > @@ -779,7 +779,7 @@ with profile.test_list.group_manager(
> >> >      g(['gl-1.1-read-pixels-after-display-list'])
> >> >      g(['gl-1.1-set-vertex-color-after-draw'])
> >> >      g(['array-stride'])
> >> > -    g(['clear-accum'])
> >> > +    g(['clear-accum'], run_concurrent=False)
> >> >      g(['clipflat'])
> >> >      g(['copypixels-draw-sync'])
> >> >      g(['copypixels-sync'])
> >> > @@ -813,7 +813,7 @@ with profile.test_list.group_manager(
> >> >      g(['lineloop', '-dlist'], 'lineloop-dlist')
> >> >      g(['linestipple'], run_concurrent=False)
> >> >      g(['longprim'])
> >> > -    g(['masked-clear'])
> >> > +    g(['masked-clear'], run_concurrent=False)
> >> >      g(['point-line-no-cull'])
> >> >      g(['polygon-mode'])
> >> >      g(['polygon-mode-facing'])
> >>
> >> --
> >> A. Huillet
> >> _______________________________________________
> >> Piglit mailing list
> >> Piglit@lists.freedesktop.org
> >> https://lists.freedesktop.org/mailman/listinfo/piglit
>
> --
> A. Huillet