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@xxxxxxxxxxxxxxx>
> > > 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@xxxxxxxxxxxxxxx>
> > > 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
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
