On Wed, Feb 13, 2019 at 10:50 AM Daniel Vetter <daniel@xxxxxxxx> wrote:
>
> On Tue, Feb 12, 2019 at 10:32:31PM +0100, Mario Kleiner wrote:
> > On Mon, Feb 11, 2019 at 6:04 PM Daniel Vetter <daniel@xxxxxxxx> wrote:
> > >
> > > On Mon, Feb 11, 2019 at 4:01 PM Kazlauskas, Nicholas
> > > <Nicholas.Kazlauskas@xxxxxxx> wrote:
> > > >
> > > > On 2/11/19 3:35 AM, Daniel Vetter wrote:
> > > > > On Mon, Feb 11, 2019 at 04:22:24AM +0100, Mario Kleiner wrote:
> > > > >> The pageflip completion timestamps transmitted to userspace
...
> > > > >
> > > > > Uh, this looks like a pretty bad hack. Can't we fix amdgpu to only give us
> > > > > the right timestampe, once? With this I guess if you do a vblank query in
> > > > > between the wrong and the right vblank you'll get the bogus value. Not
> > > > > really great for userspace.
> > > > > -Daniel
> > > >
> > > > I think we calculate the timestamp and send the vblank event both within
> > > > the pageflip IRQ handler so calculating the right pageflip timestamp
> > > > once could probably be done. I'm not sure if it's easier than proposing
> > > > a later flip time with an API like this though.
> > > >
> > > > The actual scanout time should be known from the page-flip handler so
> > > > the semantics for VRR on/off remain the same. This is because the
> > > > page-flip triggers entering the back porch if we're in the extended
> > > > front porch.
> > > >
> > > > But scanout time from vblank events for something like
> > > > DRM_IOCTL_WAIT_VBLANK are going to be wrong in most cases and are only
> > > > treated as estimates. If we're in the regular front porch then the
> > > > timing to scanout is based on the fixed duration front porch for the
> > > > current mode. If we're in the extended back porch then it's technically
> > > > driver defined but the most reasonable guess is to assume that the front
> > > > porch is going to end at any moment, so just return the length of the
> > > > back porch for getting the scanout time.
> > > >
> > > > Proposing the late timestamp shouldn't affect vblank event in the
> > > > DRM_IOCTL_WAIT_VBLANK case and should only be used in the page-flip
> > > > event case. I'm not sure if that's what's guaranteed to happen with this
> > > > patch though. There doesn't seem to be any locking on either
> > > > dev->vblank_time_lock or the vblank->seqlock so while it's likely to get
> > > > the same vblank event back as the one just stored I don't think it's
> > > > guaranteed.
> > >
> > > That's the inconsistency I mean to highlight - the timestamp for the
> > > same frame as observed through flip complete and through the
> > > wait_vblank ioctl can differ. Which they really shouldn't.
> > >
> >
> > Ideally they shouldn't differ. The kernel docs for drm_crtc_state say
> > that vblank and pageflip timestamps should always match. But then the
> > kernel docs for "Variable refresh properties" in drm_connector.c for
> > vblank timestamps were changed for the VRR implementation in Linux
> > 5.0-rc to redefine them when in VRR mode. They are defined, but
> > probably rather useless for any practical purpose, like this:
> >
> > "The semantics for the vertical blank timestamp differ when
> > variable refresh rate is active. The vertical blank timestamp
> > is defined to be an estimate using the current mode's fixed
> > refresh rate timings. The semantics for the page-flip event
> > timestamp remain the same."
>
> Uh I missed that. That sounds like nonsense tbh.
>
> > So our docs contradict each other as of Linux 5.0-rc. Certainly having
> > useful vblank timetamps would be useful.
>
> Yup, imo vblank should still match page_flip. Otherwise I expect a lot of
> hilarity will ensue.
>
> > > Now added complication is that amdgpu sends out vblank events really
> > > early, which is used by userspace to do frontbuffer rendering in the
> > > vblank time. But I don't think anyone wants to do both VRR and
> >
> > I think all kms drivers try to call drm_crtc_handle_vblank() at start
> > of vblank to give Mesa the most time for frontbuffer rendering for
> > classic X. But vblank events are also used for scheduling bufferswaps
> > or other stuff for redirected windowed rendering, or via api's like
> > OML_sync_controls glXWaitForMscOML, so there might be other things
> > affected by a more delayed vblank handling.
>
> The frontbuffer rendering is very much X driver specific, and I think
> -amdgpu/radeon is the only one that requires this. No i915 driver ever
> used the vblank interrupt to schedule frontbuffer blits, we use some
> CS-side stalls.
>
> Wrt scheduling pageflips: The rule is that right after you've received the
> vblank for frame X, then an immediately schedule pageflip should hit X+1,
> but not X. amdgpu had this broken, but it's fixed since a while.
>
> > > frontbuffer rendering, hence I think it should be possible to create
> > > correct vblank timestamps for the VRR case, while leaving the current
> > > logic in place for everything else. But that means moving the entire
> > > vblank processing to where you know the next frame's start time for
> > > VRR, both for page flips and for all other vblank events.
> > > -Daniel
> > >
> >
> > I think for amdgpu it would be doable by calling drm_handle_vblank
> > from the pageflip irq handler in VRR mode, and then additionally from
> > the vblank interrupt handler like now. Vblank irq's are triggered via
> > vline interrupts with programmable vertical trigger line position, so
> > we could program the trigger line to be start of back-porch instead of
> > start of front-porch. In the pageflip case we do vblank handling +
> > timestamping + pflip completion from pflip irq, and have the regular
> > back-porch vblank handling for refresh cycles in which no
> > pageflip/pflip irq happened.
>
> Hm, can't we simply program the vblank to occur in the back porch region
> for VRR? The vblank timestamping should automatically correct the
> timestamp in that case. Would require a lot less code changes.
>
> Plus update the documentation ofc.
>
Thanks to an unexpected bus driver strike i got stuck in the lab
instead of my bed for the last 6 extra hours, so i had some time to
think about this and hack up a proof-of-concept. Yes, moving vblank
irq ~ drm_handle_vblank() to the start of back-porch seems to do the
trick in VRR mode pretty well, at least as far as 30 minutes of
testing with my own application Psychtoolbox show. This may work -
More sleep and more testing required :)
As a side-effect of the shifted point of drm_update_vblank_counter()
update in back-porch or flip-irq though, the flip throttling in VRR
needed some work, which makes it even more hacky/messy - Michel will
not be pleased ;). Might need a timestamp-based throttling in VRR mode
to make it less messy.
-mario
> > Not sure if/how that would mess with below-vrr-refresh-rate-range
> > support for stutter reduction at low fps though? Or with performance,
> > given vblank event dispatch could be delayed a lot in VRR compared to
> > normal mode? Or with all the vblank accounting during modesets or crtc
> > en/disable?
>
> Currently VRR and explicit frame scheduling don't mix. We've discussed
> this, and consens seems to be that we need a new property for page_flip,
> similar to the target frame, but instead of frames a timestamp. Then the
> driver can hit a specific time for the next frame.
>
> And since compositors use the vblank ioctl just to do frame scheduling, I
> don't think we have to worry hugely about interactions there. Much better
> to aim for clean semantics (i.e. vblank and flip complete timestamps
> better match).
>
> > Also, could other drivers like Intel easily do this delayed vblank
> > processing in the non-pageflip case?
>
> Intel's problem :-) But yeah we have page flip timestamp registers, so
> worst case all we need to do is shuffle code around a bit. And we have
> interrupts for both start of vblank and and of vblank. It might be a bit
> more work because we use the vblank interrupt to avoid races in atomic
> updates, but that's not too terrible to fix.
>
> > From my user perspective as a developer of a neuroscience research
> > application that has a couple of exciting/important use cases for VRR
> > mode, i absolutely do need trustworthy and precise pageflip event
> > timestamps that represent reality, otherwise VRR mode would be less
> > than useless for me. I can do without meaningful vblank timestamps in
> > VRR mode though, and expected to do without them, as any scheduling
> > has to happen time-based instead of based on vblank counts anyway,
> > given that vblank counts are quite meaningless if every frame can have
> > a different duration. I'd expect that situation to be similar for
> > other apps that would want to use timestamps in VRR mode like video
> > games, movie players or VR/AR applications.
>
> Yeah for that you want to schedule your frames with a target frame number.
> Would still be nice if vblank timestamps wouldn't be an outright lie (and
> the timestamp is still useful information, even if the frame counter
> isn't - many displays have min/max frame rates for VRR, which is
> information we could perhaps expose).
> -Daniel
>
> >
> > -mario
> >
> > > >
> > > > Nicholas Kazlauskas
> > > >
> > > > >
> > > > >> ---
> > > > >> drivers/gpu/drm/drm_vblank.c | 49 +++++++++++++++++++++++++++++++++++-
> > > > >> include/drm/drm_vblank.h | 8 ++++++
> > > > >> 2 files changed, 56 insertions(+), 1 deletion(-)
> > > > >>
> > > > >> diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
> > > > >> index 98e091175921..4b3a4c38fabe 100644
> > > > >> --- a/drivers/gpu/drm/drm_vblank.c
> > > > >> +++ b/drivers/gpu/drm/drm_vblank.c
> > > > >> @@ -814,10 +814,21 @@ static void send_vblank_event(struct drm_device *dev,
> > > > >> u64 seq, ktime_t now)
> > > > >> {
> > > > >> struct timespec64 tv;
> > > > >> + ktime_t alt_flip_time;
> > > > >>
> > > > >> switch (e->event.base.type) {
> > > > >> - case DRM_EVENT_VBLANK:
> > > > >> case DRM_EVENT_FLIP_COMPLETE:
> > > > >> + /*
> > > > >> + * For flip completion events, override "now" time
> > > > >> + * with alt_flip_time provided by the driver via
> > > > >> + * drm_crtc_set_vrr_pageflip_timestamp() in VRR mode
> > > > >> + * if that time is later than given "now" vblank time.
> > > > >> + */
> > > > >> + alt_flip_time = dev->vblank[e->pipe].alt_flip_time;
> > > > >> + if (alt_flip_time > now)
> > > > >> + now = alt_flip_time;
> > > > >> + /* Fallthrough */
> > > > >> + case DRM_EVENT_VBLANK:
> > > > >> tv = ktime_to_timespec64(now);
> > > > >> e->event.vbl.sequence = seq;
> > > > >> /*
> > > > >> @@ -916,11 +927,47 @@ void drm_crtc_send_vblank_event(struct drm_crtc *crtc,
> > > > >>
> > > > >> now = ktime_get();
> > > > >> }
> > > > >> +
> > > > >> e->pipe = pipe;
> > > > >> send_vblank_event(dev, e, seq, now);
> > > > >> }
> > > > >> EXPORT_SYMBOL(drm_crtc_send_vblank_event);
> > > > >>
> > > > >> +/**
> > > > >> + * drm_crtc_set_vrr_pageflip_timestamp - helper to set alternate pageflip time
> > > > >> + * @crtc: the source CRTC of the pageflip completion event
> > > > >> + * @flip_time: The alternate pageflip completion timestamp in VRR mode
> > > > >> + *
> > > > >> + * In variable refresh rate mode (VRR), a pageflip completion timestamp carried
> > > > >> + * by the pageflip event can never be earlier than the vblank timestamp of the
> > > > >> + * vblank of flip completion, as that vblank timestamp defines the end of the
> > > > >> + * shortest possible vblank duration. In case of a delayed flip completion
> > > > >> + * inside the extended VRR front porch however, the end of vblank can be much
> > > > >> + * later, so the driver must assign an estimated timestamp of that later end of
> > > > >> + * vblank. For a CRTC in VRR mode, the driver should use this helper function to
> > > > >> + * set an alternate flip completion timestamp in case of late flip completions
> > > > >> + * in extended vblank. In the most simple case, this @flip_time timestamp could
> > > > >> + * simply be a ktime_get() timestamp taken at the start of the pageflip
> > > > >> + * completion routine, with some constant duration of the back porch interval
> > > > >> + * added, although more precise estimates may be possible on some hardware if
> > > > >> + * the hardware provides some means of timestamping the true end of vblank.
> > > > >> + *
> > > > >> + * When sending out pageflip events, e.g., via drm_crtc_send_vblank_event(), it
> > > > >> + * will use either the standard vblank timestamp, calculated for a minimum
> > > > >> + * duration vblank, or the provided @flip_time if that time is later than the
> > > > >> + * vblank timestamp, to get the best possible estimate of start of display of
> > > > >> + * the new post-pageflip scanout buffer.
> > > > >> + */
> > > > >> +void drm_crtc_set_vrr_pageflip_timestamp(struct drm_crtc *crtc,
> > > > >> + ktime_t flip_time)
> > > > >> +{
> > > > >> + struct drm_device *dev = crtc->dev;
> > > > >> + struct drm_vblank_crtc *vblank = &dev->vblank[drm_crtc_index(crtc)];
> > > > >> +
> > > > >> + vblank->alt_flip_time = flip_time;
> > > > >> +}
> > > > >> +EXPORT_SYMBOL(drm_crtc_set_vrr_pageflip_timestamp);
> > > > >> +
> > > > >> static int __enable_vblank(struct drm_device *dev, unsigned int pipe)
> > > > >> {
> > > > >> if (drm_core_check_feature(dev, DRIVER_MODESET)) {
> > > > >> diff --git a/include/drm/drm_vblank.h b/include/drm/drm_vblank.h
> > > > >> index 6ad9630d4f48..aacf44694ab6 100644
> > > > >> --- a/include/drm/drm_vblank.h
> > > > >> +++ b/include/drm/drm_vblank.h
> > > > >> @@ -117,6 +117,12 @@ struct drm_vblank_crtc {
> > > > >> * @time: Vblank timestamp corresponding to @count.
> > > > >> */
> > > > >> ktime_t time;
> > > > >> + /**
> > > > >> + * @alt_flip_time: Vblank timestamp for end of extended vblank due to
> > > > >> + * a late pageflip completion in variable refresh rate mode. Pageflip
> > > > >> + * events will carry the later one of @time and @alt_flip_time.
> > > > >> + */
> > > > >> + ktime_t alt_flip_time;
> > > > >>
> > > > >> /**
> > > > >> * @refcount: Number of users/waiters of the vblank interrupt. Only when
> > > > >> @@ -179,6 +185,8 @@ int drm_vblank_init(struct drm_device *dev, unsigned int num_crtcs);
> > > > >> u64 drm_crtc_vblank_count(struct drm_crtc *crtc);
> > > > >> u64 drm_crtc_vblank_count_and_time(struct drm_crtc *crtc,
> > > > >> ktime_t *vblanktime);
> > > > >> +void drm_crtc_set_vrr_pageflip_timestamp(struct drm_crtc *crtc,
> > > > >> + ktime_t flip_time);
> > > > >> void drm_crtc_send_vblank_event(struct drm_crtc *crtc,
> > > > >> struct drm_pending_vblank_event *e);
> > > > >> void drm_crtc_arm_vblank_event(struct drm_crtc *crtc,
> > > > >> --
> > > > >> 2.17.1
> > > > >>
> > > > >> _______________________________________________
> > > > >> dri-devel mailing list
> > > > >> dri-devel@xxxxxxxxxxxxxxxxxxxxx
> > > > >> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > > > >
> > > >
> > >
> > >
> > > --
> > > Daniel Vetter
> > > Software Engineer, Intel Corporation
> > > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
_______________________________________________
amd-gfx mailing list
amd-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
