[igt-dev] [PATCH i-g-t] prime_vgem: Downgrade the severity of a single missed vblank on flipping

[Chris Wilson]

Quoting Daniel Vetter (2019-04-04 09:17:06)
> On Thu, Apr 04, 2019 at 08:05:37AM +0100, Chris Wilson wrote:
> > Not displaying the flip on the next vblank is bad, but not the end of
> > the world -- so long as that is only a temporary glitch. Give the vblank
> > a few more frames to complete, and warn instead of failing if it takes
> > more than one vblank interval to flip.
> > 
> > Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> > ---
> >  tests/prime_vgem.c | 22 +++++++++++++++-------
> >  1 file changed, 15 insertions(+), 7 deletions(-)
> > 
> > diff --git a/tests/prime_vgem.c b/tests/prime_vgem.c
> > index 85a27b69f..8c0dd6c7a 100644
> > --- a/tests/prime_vgem.c
> > +++ b/tests/prime_vgem.c
> > @@ -713,15 +713,23 @@ static void flip_to_vgem(int i915, int vgem,
> >  
> >       /* And then the flip is completed as soon as it is ready */
> >       if (!hang) {
> > -             union drm_wait_vblank wait;
> > +             unsigned long miss;
> >  
> > -             memset(&wait, 0, sizeof(wait));
> > -             wait.request.type = DRM_VBLANK_RELATIVE | pipe_select(0);
> > -             wait.request.sequence = 10;
> > -             do_or_die(drmIoctl(i915, DRM_IOCTL_WAIT_VBLANK, &wait));
> > -
> > -             vgem_fence_signal(vgem, fence);
> > +             /* Signal fence at the start of then next vblank */
> >               get_vblank(i915, 0, DRM_VBLANK_NEXTONMISS);
> > +             vgem_fence_signal(vgem, fence);
> > +
> > +             miss = 0;
> > +             igt_until_timeout(5) {
> > +                     get_vblank(i915, 0, DRM_VBLANK_NEXTONMISS);
> > +                     if (poll(&pfd, 1, 0))
> > +                             break;
> > +                     miss++;
> > +             }
> > +             if (miss) {
> 
> I think miss > 1 would be correct here, since once we signal the kernel
> should be able to realize that, queue the flip and execute it within 1
> vblank. 16ms delay would be rather bad.
> 
> Otoh we can't expect that it'll always issue it right away, because
> there's some worker and code involved plus other delays, so no matter
> wether we sample the vblank counter before or after we signal the fence,
> we might miss the very next vblank. Warning about a race we expect to
> occasionally happen and can't prevent doesn't sound good.

Surely missing a vblank on an idle system with a full 16ms grace period
is worth a warning in CI? If we can miss under relaxed conditions, how
bad will it be under load? :)

> With the miss > 1:
> 
> Reviewed-by: Daniel Vetter <daniel.vetter at ffwll.ch>

Ok, because I believe we should have other tests (like kms_flip) that
are just as sensitive to missed flips, and try to compare the timestamps
on the vblank vs walltime etc.

> Also, is there some CI bugzilla for this? Would be good to reference that
> one.

We were previously failing at the nanosleep, I do not recall seeing a
warning about the igt_assert(poll()) before, and a quick scan through
bugzilla says it hasn't been filed yet.
-Chris