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

[Daniel Vetter]

On Thu, Apr 4, 2019 at 10:40 AM Chris Wilson <chris at chris-wilson.co.uk> wrote:
>
> 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? :)

Hm, I misread the logic, coffee didn't kick in yet I guess. We wait
for the vblank first, then signal, that should indeed be enough on an
idle box. Somehow I mixed those up.

> > 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.

Correcting to r-b: me as-is. Maybe add a comment that we wait for the
vblank to give the kernel almost a full frame to get things done,
which should be enough.
-Daniel

> > 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
> _______________________________________________
> igt-dev mailing list
> igt-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/igt-dev



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch