On Saturday, January 31, 2009 4:49 am Michel Dänzer wrote: > On Fri, 2009-01-30 at 10:43 -0800, Jesse Barnes wrote: > > On Friday, January 30, 2009 9:26 am Jesse Barnes wrote: > > > But maybe there's an even simpler way to handle this (the wait on > > > disabled pipe/irq problem). The DRM_WAIT_ON already checks if irqs are > > > disabled before waiting (or at least it's supposed to), and the > > > vblank_get call will check to make sure the pipe is enabled before > > > waiting... maybe one of those checks is failing in this case, or we're > > > not waking clients up at IRQ disable time like we should. I'll do some > > > more testing and see. > > > > Since in the VT switch case at least, userspace is calling in with a > > stale vblank value (one much smaller than the current count), another > > option would be something like the below. Needs to be a bit smarter to > > deal with counter wraparound though, > > I don't think an additional test should be needed; the > > (drm_vblank_count(dev, crtc) - vblwait->request.sequence) <= (1 << 23) > > test is supposed to catch insane conditions, if it isn't appropriate, > just fix it.
I don't want to add another test, just to replace the existing one. Below is what I'd like to push, since it matches other wraparound handling in the kernel. How did you settle on 1<<23 originally? > > but OTOH wraparound wouldn't happen as often if our > > drm_update_vblank_count() wasn't so eager to add ~max_frame_count to > > the counter at enable time. > > Huh? If the counter jumps, that's a bug (most likely due to the display > driver not calling the pre/post modesetting ioctl around the CRTC frame > counter resetting?) that needs to be fixed. I'm not seeing any such > issues with radeon anymore though; maybe you;d like to take a look at > how the X radeon driver calls the ioctl in Leave/EnterVT. Lost frame accounting is done in drm_update_vblank_count, which uses last_vblank[crtc], which is updated by the expiration timer function (should have renamed this stuff to "frames" when we moved back to the interrupt only scheme). If it gets called before the timer has expired, but after a frame counter reset (e.g. a quick VT switch), you'll see cur_vblank < last_vblank on that crtc, and add max_vblank_count. It's mostly harmless, we probably see it on Intel because we do an IRQ disable across VT switch, which would trigger the update_vblank_count call at the first post-VT switch drm_vblank_get. Should be easy to fix up though, we can just capture last_vblank_count at IRQ disable time too. -- Jesse Barnes, Intel Open Source Technology Center diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c index 69aa0ab..c41cba4 100644 --- a/drivers/gpu/drm/drm_irq.c +++ b/drivers/gpu/drm/drm_irq.c @@ -341,7 +341,7 @@ int drm_control(struct drm_device *dev, void *data, * vblank events since the system was booted, including lost events due to * modesetting activity. */ -u32 drm_vblank_count(struct drm_device *dev, int crtc) +unsigned int drm_vblank_count(struct drm_device *dev, int crtc) { return atomic_read(&dev->_vblank_count[crtc]); } @@ -522,6 +522,11 @@ out: return ret; } +#define frame_after_eq(a,b) \ + (typecheck(unsigned int, a) && \ + typecheck(unsigned int, b) && \ + ((int)(a) - (int)(b) >= 0)) + /** * Wait for VBLANK. * @@ -589,10 +594,12 @@ int drm_wait_vblank(struct drm_device *dev, void *data, DRM_DEBUG("waiting on vblank count %d, crtc %d\n", vblwait->request.sequence, crtc); dev->last_vblank_wait[crtc] = vblwait->request.sequence; + + /* Wait for the sequence number to pass or IRQs to get disabled */ DRM_WAIT_ON(ret, dev->vbl_queue[crtc], 3 * DRM_HZ, - (((drm_vblank_count(dev, crtc) - - vblwait->request.sequence) <= (1 << 23)) || - !dev->irq_enabled)); + frame_after_eq(drm_vblank_count(dev, crtc), + vblwait->request.sequence) || + !dev->irq_enabled); if (ret != -EINTR) { struct timeval now; ------------------------------------------------------------------------------ This SF.net email is sponsored by: SourcForge Community SourceForge wants to tell your story. http://p.sf.net/sfu/sf-spreadtheword -- _______________________________________________ Dri-devel mailing list Dri-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/dri-devel