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@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.
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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
