| Message ID | 150274380178.28425.15865885784220472106@mail.alporthouse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Aug 14, 2017 at 09:50:01PM +0100, Chris Wilson wrote: > Quoting Daniel Vetter (2017-08-14 21:32:00) > > The goal of these subtests was just to delay a kms operation a little > > bit. The goal wasn't to > > - spin until the operation was completed. That results in a gpu hang, > > and gpu hangs need igt_hang for safety. > > - complete it before the operation. That's just pointless. > > > > Fix it by using the timeout support. > > > > This was all broken in the initial conversation: > > > > commit 96ec8cb3b5ec1fc2927d6cff8e09930082301d7e > > Author: Abdiel Janulgue <abdiel.janulgue@linux.intel.com> > > Date: Sat Oct 29 01:01:05 2016 +0300 > > > > igt/kms_flip: Use new igt_spin_batch > > > > Note that part of the damage was removed already in > > Ah no, that patch removed the timeouts! I remember there being > set_timeout in there and was baffled to see them gone. > > Indeed, the breakage is > > > commit b0081ea9cb7d39234fd0fcc64dd56ed4f5d50b05 > > Author: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > Date: Wed Aug 9 11:11:52 2017 +0200 > > > > tests/kms_flip: Remove $engine-flip-vs-dpms/modeset > > > due to > > diff --git a/tests/kms_flip.c b/tests/kms_flip.c > index ede5fd2b..ea860e19 100644 > --- a/tests/kms_flip.c > +++ b/tests/kms_flip.c > @@ -755,13 +755,8 @@ static unsigned int run_test_step(struct test_output *o) > if (o->flags & TEST_MODESET) > igt_assert(set_mode(o, o->fb_ids[o->current_fb_id], 0, 0) == 0); > > - if (o->flags & TEST_DPMS) { > - if (spin_rcs) > - igt_spin_batch_set_timeout(spin_rcs, NSEC_PER_SEC); > - if (spin_bcs) > - igt_spin_batch_set_timeout(spin_bcs, NSEC_PER_SEC); > + if (o->flags & TEST_DPMS) This doesn't fix the case where TEST_DPMS is not set. I seen hangs everywhere, because we only stop the spinning _after_ the operation is completed. The only way for the operation to complete is by a gpu reset, and resetting a live spinner on snb blt is not a good idea (it kills the box eventually). And yes there's a a race with just setting a timeout, but we have other checks that make sure an atomic flip doesn't take one full second. So I don't think the race is relevant in practice for testing (because it would indicate a broken kernel, and we catch those bugs already). -Daniel
On Tue, Aug 15, 2017 at 10:05:42AM +0200, Daniel Vetter wrote: > On Mon, Aug 14, 2017 at 09:50:01PM +0100, Chris Wilson wrote: > > Quoting Daniel Vetter (2017-08-14 21:32:00) > > > The goal of these subtests was just to delay a kms operation a little > > > bit. The goal wasn't to > > > - spin until the operation was completed. That results in a gpu hang, > > > and gpu hangs need igt_hang for safety. > > > - complete it before the operation. That's just pointless. > > > > > > Fix it by using the timeout support. > > > > > > This was all broken in the initial conversation: > > > > > > commit 96ec8cb3b5ec1fc2927d6cff8e09930082301d7e > > > Author: Abdiel Janulgue <abdiel.janulgue@linux.intel.com> > > > Date: Sat Oct 29 01:01:05 2016 +0300 > > > > > > igt/kms_flip: Use new igt_spin_batch > > > > > > Note that part of the damage was removed already in > > > > Ah no, that patch removed the timeouts! I remember there being > > set_timeout in there and was baffled to see them gone. > > > > Indeed, the breakage is > > > > > commit b0081ea9cb7d39234fd0fcc64dd56ed4f5d50b05 > > > Author: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > > Date: Wed Aug 9 11:11:52 2017 +0200 > > > > > > tests/kms_flip: Remove $engine-flip-vs-dpms/modeset > > > > > > due to > > > > diff --git a/tests/kms_flip.c b/tests/kms_flip.c > > index ede5fd2b..ea860e19 100644 > > --- a/tests/kms_flip.c > > +++ b/tests/kms_flip.c > > @@ -755,13 +755,8 @@ static unsigned int run_test_step(struct test_output *o) > > if (o->flags & TEST_MODESET) > > igt_assert(set_mode(o, o->fb_ids[o->current_fb_id], 0, 0) == 0); > > > > - if (o->flags & TEST_DPMS) { > > - if (spin_rcs) > > - igt_spin_batch_set_timeout(spin_rcs, NSEC_PER_SEC); > > - if (spin_bcs) > > - igt_spin_batch_set_timeout(spin_bcs, NSEC_PER_SEC); > > + if (o->flags & TEST_DPMS) > > This doesn't fix the case where TEST_DPMS is not set. I seen hangs > everywhere, because we only stop the spinning _after_ the operation is > completed. The only way for the operation to complete is by a gpu reset, > and resetting a live spinner on snb blt is not a good idea (it kills the > box eventually). > > And yes there's a a race with just setting a timeout, but we have other > checks that make sure an atomic flip doesn't take one full second. So I > don't think the race is relevant in practice for testing (because it would > indicate a broken kernel, and we catch those bugs already). Correction: The patch from Maarten did indeed break stuff even more, because it removed code that wasn't dead yet (since it didn't remove all the DPMS vs spin tests). But the breakage I'm trying to fix is older, and goes back to Abdiel's patch. I'll augment the commit message to explain this better. -Daniel
Quoting Daniel Vetter (2017-08-15 09:08:01) > On Tue, Aug 15, 2017 at 10:05:42AM +0200, Daniel Vetter wrote: > > On Mon, Aug 14, 2017 at 09:50:01PM +0100, Chris Wilson wrote: > > > Quoting Daniel Vetter (2017-08-14 21:32:00) > > > > The goal of these subtests was just to delay a kms operation a little > > > > bit. The goal wasn't to > > > > - spin until the operation was completed. That results in a gpu hang, > > > > and gpu hangs need igt_hang for safety. > > > > - complete it before the operation. That's just pointless. > > > > > > > > Fix it by using the timeout support. > > > > > > > > This was all broken in the initial conversation: > > > > > > > > commit 96ec8cb3b5ec1fc2927d6cff8e09930082301d7e > > > > Author: Abdiel Janulgue <abdiel.janulgue@linux.intel.com> > > > > Date: Sat Oct 29 01:01:05 2016 +0300 > > > > > > > > igt/kms_flip: Use new igt_spin_batch > > > > > > > > Note that part of the damage was removed already in > > > > > > Ah no, that patch removed the timeouts! I remember there being > > > set_timeout in there and was baffled to see them gone. > > > > > > Indeed, the breakage is > > > > > > > commit b0081ea9cb7d39234fd0fcc64dd56ed4f5d50b05 > > > > Author: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > > > Date: Wed Aug 9 11:11:52 2017 +0200 > > > > > > > > tests/kms_flip: Remove $engine-flip-vs-dpms/modeset > > > > > > > > > due to > > > > > > diff --git a/tests/kms_flip.c b/tests/kms_flip.c > > > index ede5fd2b..ea860e19 100644 > > > --- a/tests/kms_flip.c > > > +++ b/tests/kms_flip.c > > > @@ -755,13 +755,8 @@ static unsigned int run_test_step(struct test_output *o) > > > if (o->flags & TEST_MODESET) > > > igt_assert(set_mode(o, o->fb_ids[o->current_fb_id], 0, 0) == 0); > > > > > > - if (o->flags & TEST_DPMS) { > > > - if (spin_rcs) > > > - igt_spin_batch_set_timeout(spin_rcs, NSEC_PER_SEC); > > > - if (spin_bcs) > > > - igt_spin_batch_set_timeout(spin_bcs, NSEC_PER_SEC); > > > + if (o->flags & TEST_DPMS) > > > > This doesn't fix the case where TEST_DPMS is not set. I seen hangs > > everywhere, because we only stop the spinning _after_ the operation is > > completed. The only way for the operation to complete is by a gpu reset, > > and resetting a live spinner on snb blt is not a good idea (it kills the > > box eventually). > > > > And yes there's a a race with just setting a timeout, but we have other > > checks that make sure an atomic flip doesn't take one full second. So I > > don't think the race is relevant in practice for testing (because it would > > indicate a broken kernel, and we catch those bugs already). > > Correction: The patch from Maarten did indeed break stuff even more, > because it removed code that wasn't dead yet (since it didn't remove all > the DPMS vs spin tests). But the breakage I'm trying to fix is older, and > goes back to Abdiel's patch. I'll augment the commit message to explain > this better. Nope, your commit message is still wrong. And you have not addressed the underlying issue. If you looked at the patch I sent for this last week... -Chris
diff --git a/tests/kms_flip.c b/tests/kms_flip.c index ede5fd2b..ea860e19 100644 --- a/tests/kms_flip.c +++ b/tests/kms_flip.c @@ -755,13 +755,8 @@ static unsigned int run_test_step(struct test_output *o) if (o->flags & TEST_MODESET) igt_assert(set_mode(o, o->fb_ids[o->current_fb_id], 0, 0) == 0); - if (o->flags & TEST_DPMS) { - if (spin_rcs) - igt_spin_batch_set_timeout(spin_rcs, NSEC_PER_SEC); - if (spin_bcs) - igt_spin_batch_set_timeout(spin_bcs, NSEC_PER_SEC); + if (o->flags & TEST_DPMS) -Chris _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx