[igt-dev] [PATCH i-g-t v2] tests: Add variable refresh rate tests

Tue Apr 14 19:28:06 UTC 2020

On Tue, Apr 14, 2020 at 08:50:29PM +0200, Daniel Vetter wrote:
> On Tue, Apr 14, 2020 at 3:00 PM Kazlauskas, Nicholas
> <nicholas.kazlauskas at amd.com> wrote:
> >
> > On 2020-04-14 6:52 a.m., Daniel Vetter wrote:
> > >> +/* Performs an asynchronous non-blocking page-flip on a pipe. */
> > >> +static int
> > >> +do_flip(data_t *data, enum pipe pipe_id, igt_fb_t *fb)
> > >> +{
> > >> +       igt_pipe_t *pipe = &data->display.pipes[pipe_id];
> > >> +       int ret;
> > >> +
> > >> +       igt_set_timeout(1, "Scheduling page flip\n");
> > >> +
> > >> +       /*
> > >> +        * Only the legacy flip ioctl supports async flips.
> > >> +        * It's also non-blocking, but returns -EBUSY if flipping too fast.
> > >> +        * 2x monitor tests will need async flips in the atomic API.
> > >> +        */
> > >
> > > Uh, if this is also how your amdgpu userspace works we've just fucked
> > > up the uapi for good :-/
> > >
> > > FLIP_ASYNC = please tear
> > >
> > > VRR = please don't strictly obey the vrefresh, but very much dont tear
> > >
> > > Tying them together means we're deeply mixing things up.
> > >
> > > Also amdgpu is still using it's own flip implementation, which makes
> > > me wonder whether VRR would even work with atomic, or whether that's
> > > also butchered ...
> > >
> > > How I thought this stuff was supposed to work:
> > >
> > > - VRR_ENABLED controls whether we do VRR
> > > - since atomic is awesome you can change that on every frame
> > > - VRR has nothing to do with ASYNC
> > >
> > > So a) do I read this correctly b) how do we get out of this hole (and
> > > maybe c) amdgpu really needs to remove
> > > amdgpu_display_crtc_page_flip_target asap).
> > >
> > > Manasi pointed this out to me, so adding a few more people here.
> > > -Daniel
> >
> > Adaptive sync is an extended front porch that ends upon some signaling
> > from the driver is almost always tied to a page flip.
> >
> > FLIP_ASYNC is a mechanism to flip immediately without waiting for any
> > double buffering from the driver or hardware, potentially causing tearing.
> >
> > This test uses FLIP_ASYNC to precisely position when the flip occurs
> > from userspace within the VBLANK to end the frame at the midpoint
> > between the min and max range.
> >
> > But it doesn't really need to be FLIP_ASYNC to make this work, explicit
> > fencing is probably the better solution here for all drivers.
> 
> Ok, I freaked out for a bit there, and reading amdgpu DC code didn't
> really help with convincing me it's not needed - you're still using
> your own page_flip_target handler instead of the one in the helpers
> (written by amd people even, and for a few features before VRR), so
> biggest worry I had was that VRR doesn't work without FLIP_ASYNC set.
> I also looked at -amdgpu code, but I couldn't figure this out one way
> or another there either (since that's not using atomic either).
> 
> > This test only really worked by accident for testing amdgpu. We have
> > internal stalls in the driver to prevent an immediate flip from
> > occurring more than once per frame, so if you try to flip in the
> > extended front porch you end up hitting that stall and the returned
> > timestamp is a frame ahead.
> >
> > The other part of this test that's sort of an issue is that this is
> > effectively a measurement for how fast you can immediate flip - which is
> > good for catching performance regressions in the driver, and good for
> > consistent VRR behavior, but not exactly related to the test.
> 
> Hm yeah I think that's another reason for why this test should maybe
> use atomic with vgem dma_fences that we signal exactly when needed.
> Probably should be a 2nd subtest, so that we can keep the other one
> working.
> 
> Plus update the comment on why exactly FLIP_ASYNC is needed with your
> story above. Btw instead of FLIP_ASYNC could we use the target frame
> number, set to the next frame? That should also allow us to do an
> immediate flip while in the vblank.
> 
> Also I think we should have a normal flip which is somewhat late-ish
> in the vblank, and make sure VRR can still hit that frame. If that
> doesn't exist yet. I think with that set of tests we should be able to
> exercise all the timings we want for VRR?
>

Yes I think we can use the normal flip here.
And the way it really works on Intel HW is that, it has its own flip decision boundary
where if the flip request occurs in the Vactive of the previous frame then it waits
to flip unil the flip decision boundary hits which is vmin + the framestart_delay +
the pipeline fill delay and if the flip request occurs anywhere in between vmin and vmax
then the flip happens immediately thus terminating the vblank there.

Do we still need to use the vgem_dma_fences? Could you elaborate a little bit on
what signalling is expected to be achieved with the fences and how that can be used
in combination with the regular atomic commit and flip request and the page flip event captures?

Manasi

> > The way FLIP_ASYNC should probably work in amdgpu is:
> >
> > 1) No stalls
> > 2) Return -EBUSY if we're in the middle of a frame already, or even
> > allow the commit to happen in the same frame since the hardware supports it
> >
> > Not sure how userspace is equipped to handle that though.
> 
> I think aside from -amdgpu there's 0 userspace out there using
> FLIP_ASYNC. Plus it's not standardized for atomic drivers, so really
> the semantics are super wobbly. I'm not worried about FLIP_ASYNC
> itself (not the first piece of kms uapi that might be fun to unify
> across drivers), the only thing that worried me is tying VRR into
> FLIP_ASYNC. Looks I worried without reasons since it's just an
> igt/test hack that we can change.
> 
> Thanks for your explanations and all.
> 
> Cheers, Daniel
> 
> >
> > Regards,
> > Nicholas Kazlauskas
> >
> > >
> > >> +       do {
> > >> +               ret = drmModePageFlip(data->drm_fd, pipe->crtc_id,
> > >> +                                     fb->fb_id,
> > >> +                                     DRM_MODE_PAGE_FLIP_EVENT |
> > >> +                                     DRM_MODE_PAGE_FLIP_ASYNC,
> > >> +                                     data);
> > >> +       } while (ret == -EBUSY);
> > >> +
> > >> +       igt_assert_eq(ret, 0);
> > >> +       igt_reset_timeout();
> > >> +
> > >> +       return 0;
> > >> +}
> >
> 
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch