[Intel-gfx] [PATCH i-g-t] tests/kms_flip: fix spin_batch conversion

[Daniel Vetter]

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 at 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 at 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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch