Hi, i just sent out a (v2) of Daniels patch, with my review comments and reviewed-by for the code already applied to the code for convenience. Interspersed below in the patch the review comments for a few small bugs.
This and Daniels original patch is only compile tested. I still have that GeForce 7800 GTX, but unfortunately i don't have the original PC anymore for testing it. Today i tried to put the card as a 2nd non-boot card into a MacPro for testing, but the EFI based Mac apparently didn't like that old PC card that much, so testing was a no go. Bootup ended with some nouveau MMIO read and write faults and then lockup. Usually more recent NVidia PC cards do work in Macs under Linux with nouveau as non-boot gpus, but for some reason this one doesn't. Anyway, after digging through my old e-mail conversation with Ben from a year ago, i think Daniel's patch should work and solve the problem quite elegantly: iirc Ben explained to me that on pre-nv50, nouveau_flip_complete() (which calls nouveau_finish_page_flip()), is not triggered by an actual pageflip interrupt, but by a fifo software interrupt programmed to fire shortly before the vblank. On my test card it fired in the last scanline before vblank, probably at the end of active scanout. nouveau_flip_complete() would first call nouveau_finish_page_flip() to send the pageflip event, and then manually flip to the new framebuffer by calling nv_set_crtc_base(). I think/assume nv_set_crtc_base() is not itself synchronized to vblank, so we should get the correct behaviour: 1. Shortly before start of vblank: fifo sw interrupt -> nouveau_flip_complete() -> nouveau_finish_page_flip() queues pageflip event for later delivery by vblank irq handler -> nv_set_crtc_base() flips to the new fb. Return from irq. 2. A few scanlines later, vblank irq fires -> drm_handle_vblank() updates vblank count and timestamps -> drm_handle_vblank_events() dispatches queued pageflip completion event from 1), now tagged with proper vblank count and timestamp of flip completion. thanks, -mario On 11/06/2015 06:19 PM, Thierry Reding wrote: > Cc += Mario Kleiner, Mario, can you take a look whether this proposed > solution makes sense and fixes the issues you were seeing back when you > posted the patch in commit: > > commit af4870e406126b7ac0ae7c7ce5751f25ebe60f28 > Author: Mario Kleiner <mario.kleiner.de at gmail.com> > Date: Tue May 13 00:42:08 2014 +0200 > > drm/nouveau/kms/nv04-nv40: fix pageflip events via special case. > > Cards with nv04 display engine can't reliably use vblank > counts and timestamps computed via drm_handle_vblank(), as > the function gets invoked after sending the pageflip events. > > Fix this by defaulting to the old crtcid = -1 fallback path > on <= NV-50 cards, and only using the precise path on NV-50 > and later. > > Signed-off-by: Mario Kleiner <mario.kleiner.de at gmail.com> > Signed-off-by: Ben Skeggs <bskeggs at redhat.com> > Cc: <stable at vger.kernel.org> # 3.13+ > > Do you happen to still have the setup around where you saw this? > > Thierry > > On Fri, Oct 30, 2015 at 10:55:40PM +0100, Daniel Vetter wrote: >> Apparently pre-nv50 pageflip events happen before the actual vblank >> period. Therefore that functionality got semi-disabled in >> >> commit af4870e406126b7ac0ae7c7ce5751f25ebe60f28 >> Author: Mario Kleiner <mario.kleiner.de at gmail.com> >> Date: Tue May 13 00:42:08 2014 +0200 >> >> drm/nouveau/kms/nv04-nv40: fix pageflip events via special case. >> >> Unfortunately that hack got uprooted in >> >> commit cc1ef118fc099295ae6aabbacc8af94d8d8885eb >> Author: Thierry Reding <treding at nvidia.com> >> Date: Wed Aug 12 17:00:31 2015 +0200 >> >> drm/irq: Make pipe unsigned and name consistent >> >> Trigering a warning when trying to sample the vblank timestamp for a >> non-existing pipe. There's a few ways to fix this: >> >> - Open-code the old behaviour, which just enshrines this slight >> breakage of the userspace ABI. >> >> - Revert Mario's commit and again inflict broken timestamps, again not >> pretty. >> >> - Fix this for real by delaying the pageflip TS until the next vblank >> interrupt, thereby making it accurate. >> >> This patch implements the third option. Since having a page flip >> interrupt that happens when the pageflip gets armed and not when it >> completes in the next vblank seems to be fairly common (older i915 hw >> works very similarly) create a new helper to arm vblank events for >> such drivers. >> >> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=106431 >> Cc: Thierry Reding <treding at nvidia.com> >> Cc: Mario Kleiner <mario.kleiner.de at gmail.com> >> Cc: Ben Skeggs <bskeggs at redhat.com> >> Cc: Ilia Mirkin <imirkin at alum.mit.edu> >> Signed-off-by: Daniel Vetter <daniel.vetter at intel.com> >> --- >> >> Note that due to lack of hw this is completely untested. But I think >> it's the right way to fix this. >> -Daniel >> --- >> drivers/gpu/drm/drm_irq.c | 56 >> ++++++++++++++++++++++++++++++- >> drivers/gpu/drm/nouveau/nouveau_display.c | 16 ++++----- >> include/drm/drmP.h | 4 +++ >> 3 files changed, 66 insertions(+), 10 deletions(-) >> >> diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c >> index 46dbc34b81ba..b3e1f58666a6 100644 >> --- a/drivers/gpu/drm/drm_irq.c >> +++ b/drivers/gpu/drm/drm_irq.c >> @@ -972,7 +972,8 @@ static void send_vblank_event(struct drm_device *dev, >> struct drm_pending_vblank_event *e, >> unsigned long seq, struct timeval *now) >> { >> - WARN_ON_SMP(!spin_is_locked(&dev->event_lock)); >> + assert_spin_locked(&dev->event_lock); >> + >> e->event.sequence = seq; >> e->event.tv_sec = now->tv_sec; >> e->event.tv_usec = now->tv_usec; >> @@ -985,6 +986,59 @@ static void send_vblank_event(struct drm_device *dev, >> } >> >> /** >> + * drm_arm_vblank_event - arm vblanke event after pageflip Typo vblanke -> vblank >> + * @dev: DRM device >> + * @pipe: CRTC index >> + * @e: the event to prepare to send >> + * >> + * A lot of drivers need to generate vblank events for the very next vblank >> + * interrupt. For example when the page flip interrupt happens when the page >> + * flip gets armed, but not when it actually executes within the next vblank >> + * period. This helper function implements exactly the required vblank >> arming >> + * behaviour. >> + * >> + * Caller must hold event lock. Caller must also hold a vblank reference >> for the >> + * event @e, which will be dropped when the next vblank arrives. >> + * >> + * This is the legacy version of drm_crtc_arm_vblank_event(). >> + */ >> +void drm_arm_vblank_event(struct drm_device *dev, unsigned int pipe, >> + struct drm_pending_vblank_event *e) >> +{ >> + struct timeval now; >> + unsigned int seq; Dead code: now and seq are not used. >> + assert_spin_locked(&dev->event_lock); >> + >> + e->pipe = pipe; -> Add this missing init: + e->event.sequence = drm_vblank_count(dev, pipe); Otherwise target sequence number for dispatch of the pageflip event by drm_handle_vblank_events() will be zero and we get in trouble for a failing comparison for running vblank count > 2^23 and clients probably hang? >> + list_add_tail(&e->base.link, &dev->vblank_event_list); >> +} >> +EXPORT_SYMBOL(drm_arm_vblank_event); >> + >> +/** >> + * drm_arm_vblank_event - arm vblanke event after pageflip vblanke -> vblank >> + * @crtc: the source CRTC of the vblank event >> + * @e: the event to send >> + * >> + * A lot of drivers need to generate vblank events for the very next vblank >> + * interrupt. For example when the page flip interrupt happens when the page >> + * flip gets armed, but not when it actually executes within the next vblank >> + * period. This helper function implements exactly the required vblank >> arming >> + * behaviour. >> + * >> + * Caller must hold event lock. Caller must also hold a vblank reference >> for the >> + * event @e, which will be dropped when the next vblank arrives. >> + * >> + * This is the native KMS version of drm_send_vblank_event(). >> + */ >> +void drm_crtc_arm_vblank_event(struct drm_crtc *crtc, >> + struct drm_pending_vblank_event *e) >> +{ >> + drm_arm_vblank_event(crtc->dev, drm_crtc_index(crtc), e); >> +} >> +EXPORT_SYMBOL(drm_crtc_arm_vblank_event); >> + >> +/** >> * drm_send_vblank_event - helper to send vblank event after pageflip >> * @dev: DRM device >> * @pipe: CRTC index >> diff --git a/drivers/gpu/drm/nouveau/nouveau_display.c >> b/drivers/gpu/drm/nouveau/nouveau_display.c >> index 184445d4abbf..041e5f84538c 100644 >> --- a/drivers/gpu/drm/nouveau/nouveau_display.c >> +++ b/drivers/gpu/drm/nouveau/nouveau_display.c >> @@ -826,7 +826,6 @@ nouveau_finish_page_flip(struct nouveau_channel *chan, >> struct drm_device *dev = drm->dev; >> struct nouveau_page_flip_state *s; >> unsigned long flags; >> - int crtcid = -1; >> >> spin_lock_irqsave(&dev->event_lock, flags); >> >> @@ -838,16 +837,15 @@ nouveau_finish_page_flip(struct nouveau_channel *chan, >> >> s = list_first_entry(&fctx->flip, struct nouveau_page_flip_state, head); >> if (s->event) { >> - /* Vblank timestamps/counts are only correct on >= NV-50 */ >> - if (drm->device.info.family >= NV_DEVICE_INFO_V0_TESLA) >> - crtcid = s->crtc; >> - >> - drm_send_vblank_event(dev, crtcid, s->event); >> + if (drm->device.info.family < NV_DEVICE_INFO_V0_TESLA) { >> + drm_arm_vblank_event(dev, s->crtc, s->event); >> + } else { >> + drm_send_vblank_event(dev, s->crtc, s->event); >> + /* Give up ownership of vblank for page-flipped crtc */ >> + drm_vblank_put(dev, s->crtc); >> + } >> } Need a drm_vblank_put() here in a else branch, so refcounting is balanced in case of !s->event. >> >> - /* Give up ownership of vblank for page-flipped crtc */ >> - drm_vblank_put(dev, s->crtc); >> - >> list_del(&s->head); >> if (ps) >> *ps = *s; >> diff --git a/include/drm/drmP.h b/include/drm/drmP.h >> index eb513341b6ee..4c91ac419d5d 100644 >> --- a/include/drm/drmP.h >> +++ b/include/drm/drmP.h >> @@ -948,6 +948,10 @@ extern void drm_send_vblank_event(struct drm_device >> *dev, unsigned int pipe, >> struct drm_pending_vblank_event *e); >> extern void drm_crtc_send_vblank_event(struct drm_crtc *crtc, >> struct drm_pending_vblank_event *e); >> +void drm_send_vblank_event(struct drm_device *dev, unsigned int pipe, >> + struct drm_pending_vblank_event *e); >> +void drm_crtc_send_vblank_event(struct drm_crtc *crtc, >> + struct drm_pending_vblank_event *e); -> Copy & Paste error, wrong function prototype. >> extern bool drm_handle_vblank(struct drm_device *dev, unsigned int pipe); >> extern bool drm_crtc_handle_vblank(struct drm_crtc *crtc); >> extern int drm_vblank_get(struct drm_device *dev, unsigned int pipe); >> -- >> 2.5.1 >> >> _______________________________________________ >> dri-devel mailing list >> dri-devel at lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/dri-devel