From: Jesse B. <jb...@vi...> - 2009-01-29 04:55:14
|
Per fdo bz #18041, we timeout if a vblank event doesn't occur within a specified time period. However, this wait happens in libdrm, so old userspace can still end up hanging, waiting for a vblank event that may never occur. So add a more reasonable timeout value to drm_wait_vblank and make the wait uninterruptible, so that the X server's scheduling signals don't get in the way. Reported-by: Linus Torvalds <tor...@li...> Tested-by: Linus Torvalds <tor...@li...> Signed-off-by: Jesse Barnes <jb...@vi...> diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c index 69aa0ab..c5df063 100644 --- a/drivers/gpu/drm/drm_irq.c +++ b/drivers/gpu/drm/drm_irq.c @@ -539,9 +539,11 @@ out: int drm_wait_vblank(struct drm_device *dev, void *data, struct drm_file *file_priv) { + struct timeval now; union drm_wait_vblank *vblwait = data; int ret = 0; unsigned int flags, seq, crtc; + int timeout, max_wait = (DRM_HZ / 40) * 100; /* 100ms max */ if ((!dev->pdev->irq) || (!dev->irq_enabled)) return -EINVAL; @@ -588,25 +590,24 @@ int drm_wait_vblank(struct drm_device *dev, void *data, DRM_DEBUG("waiting on vblank count %d, crtc %d\n", vblwait->request.sequence, crtc); + + /* Wait at most 25ms * number of frames */ + timeout = (DRM_HZ / 40) * (vblwait->request.sequence - seq); + if (timeout > max_wait) + timeout = max_wait; + dev->last_vblank_wait[crtc] = vblwait->request.sequence; - DRM_WAIT_ON(ret, dev->vbl_queue[crtc], 3 * DRM_HZ, + DRM_WAIT_ON_UNINTERRUPTIBLE(ret, dev->vbl_queue[crtc], timeout, (((drm_vblank_count(dev, crtc) - vblwait->request.sequence) <= (1 << 23)) || !dev->irq_enabled)); - if (ret != -EINTR) { - struct timeval now; + do_gettimeofday(&now); - do_gettimeofday(&now); - - vblwait->reply.tval_sec = now.tv_sec; - vblwait->reply.tval_usec = now.tv_usec; - vblwait->reply.sequence = drm_vblank_count(dev, crtc); - DRM_DEBUG("returning %d to client\n", - vblwait->reply.sequence); - } else { - DRM_DEBUG("vblank wait interrupted by signal\n"); - } + vblwait->reply.tval_sec = now.tv_sec; + vblwait->reply.tval_usec = now.tv_usec; + vblwait->reply.sequence = drm_vblank_count(dev, crtc); + DRM_DEBUG("returning %d to client\n", vblwait->reply.sequence); done: drm_vblank_put(dev, crtc); diff --git a/include/drm/drm_os_linux.h b/include/drm/drm_os_linux.h index 8dbd257..67afee8 100644 --- a/include/drm/drm_os_linux.h +++ b/include/drm/drm_os_linux.h @@ -104,5 +104,25 @@ do { \ remove_wait_queue(&(queue), &entry); \ } while (0) +#define DRM_WAIT_ON_UNINTERRUPTIBLE( ret, queue, timeout, condition ) \ +do { \ + DECLARE_WAITQUEUE(entry, current); \ + unsigned long end = jiffies + (timeout); \ + add_wait_queue(&(queue), &entry); \ + \ + for (;;) { \ + __set_current_state(TASK_UNINTERRUPTIBLE); \ + if (condition) \ + break; \ + if (time_after_eq(jiffies, end)) { \ + ret = -EBUSY; \ + break; \ + } \ + schedule_timeout((HZ/100 > 1) ? HZ/100 : 1); \ + } \ + __set_current_state(TASK_RUNNING); \ + remove_wait_queue(&(queue), &entry); \ +} while (0) + #define DRM_WAKEUP( queue ) wake_up_interruptible( queue ) #define DRM_INIT_WAITQUEUE( queue ) init_waitqueue_head( queue ) |
From: Michel D. <mi...@da...> - 2009-01-29 08:09:00
|
On Wed, 2009-01-28 at 20:55 -0800, Jesse Barnes wrote: > Per fdo bz #18041, we timeout if a vblank event doesn't occur within a > specified time period. However, this wait happens in libdrm, so old > userspace can still end up hanging, waiting for a vblank event that may never > occur. So add a more reasonable timeout value to drm_wait_vblank and make > the wait uninterruptible, so that the X server's scheduling signals don't get > in the way. I'm afraid I don't like it. > Reported-by: Linus Torvalds <tor...@li...> > Tested-by: Linus Torvalds <tor...@li...> > Signed-off-by: Jesse Barnes <jb...@vi...> It would be nice if things like this could be discussed on this list in the first place... > diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c > index 69aa0ab..c5df063 100644 > --- a/drivers/gpu/drm/drm_irq.c > +++ b/drivers/gpu/drm/drm_irq.c > @@ -539,9 +539,11 @@ out: > int drm_wait_vblank(struct drm_device *dev, void *data, > struct drm_file *file_priv) > { > + struct timeval now; > union drm_wait_vblank *vblwait = data; > int ret = 0; > unsigned int flags, seq, crtc; > + int timeout, max_wait = (DRM_HZ / 40) * 100; /* 100ms max */ Is that really a more reasonable timeout? It means e.g. the GLX_SGI_swap_control extension can't be used for swap intervals longer than 100 ms. Now I wouldn't expect such long intervals to be needed often, but it doesn't seem completely useless either (e.g. it allows apps which only need to update their display a couple of times per second to have a trivial rendering loop). Also, blocking signals for up to 100 ms sounds like bad news for the X server scheduler and SIGIO driven mouse cursor updates. Maybe the kernel code could remember how long it waited before being interrupted by a signal and then decrease that from the timeout next time or something like that? -- Earthling Michel Dänzer | http://www.vmware.com Libre software enthusiast | Debian, X and DRI developer |
From: Jesse B. <jb...@vi...> - 2009-01-29 17:16:12
|
On Thursday, January 29, 2009 12:08 am Michel Dänzer wrote: > On Wed, 2009-01-28 at 20:55 -0800, Jesse Barnes wrote: > > Per fdo bz #18041, we timeout if a vblank event doesn't occur within a > > specified time period. However, this wait happens in libdrm, so old > > userspace can still end up hanging, waiting for a vblank event that may > > never occur. So add a more reasonable timeout value to drm_wait_vblank > > and make the wait uninterruptible, so that the X server's scheduling > > signals don't get in the way. > > I'm afraid I don't like it. > > > Reported-by: Linus Torvalds <tor...@li...> > > Tested-by: Linus Torvalds <tor...@li...> > > Signed-off-by: Jesse Barnes <jb...@vi...> > > It would be nice if things like this could be discussed on this list in > the first place... Well that's why I posted it here... > > diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c > > index 69aa0ab..c5df063 100644 > > --- a/drivers/gpu/drm/drm_irq.c > > +++ b/drivers/gpu/drm/drm_irq.c > > @@ -539,9 +539,11 @@ out: > > int drm_wait_vblank(struct drm_device *dev, void *data, > > struct drm_file *file_priv) > > { > > + struct timeval now; > > union drm_wait_vblank *vblwait = data; > > int ret = 0; > > unsigned int flags, seq, crtc; > > + int timeout, max_wait = (DRM_HZ / 40) * 100; /* 100ms max */ > > Is that really a more reasonable timeout? It means e.g. the > GLX_SGI_swap_control extension can't be used for swap intervals longer > than 100 ms. Now I wouldn't expect such long intervals to be needed > often, but it doesn't seem completely useless either (e.g. it allows > apps which only need to update their display a couple of times per > second to have a trivial rendering loop). > > Also, blocking signals for up to 100 ms sounds like bad news for the X > server scheduler and SIGIO driven mouse cursor updates. If the server does long waits, yeah it could affect scheduling. But really there are two problems here: one is the signal problem that causes an infinite loop of never completed vblank waits, and another is lots missed interrupts causing apps to hang for awhile. Maybe we could do an interruptible wait if the framecount was greater than say 10, but do an uninterruptible one with a small max timeout for less (numbers are arbitrarily picked here). > Maybe the kernel code could remember how long it waited before being > interrupted by a signal and then decrease that from the timeout next > time or something like that? Probably not without a new arg to the ioctl like Thomas said (or a huge new mess of code to track processes what what we think they want), which would be hard to do in a backward compatible way. And keep in mind that vblank waits like this are inherently more racy than a good swapbuffers implementation, since they depend on the app getting scheduled in time to actually avoid tearing. -- Jesse Barnes, Intel Open Source Technology Center |
From: Michel D. <mi...@da...> - 2009-01-30 12:47:13
|
On Thu, 2009-01-29 at 09:15 -0800, Jesse Barnes wrote: > On Thursday, January 29, 2009 12:08 am Michel Dänzer wrote: > > On Wed, 2009-01-28 at 20:55 -0800, Jesse Barnes wrote: > > > > > Reported-by: Linus Torvalds <tor...@li...> > > > Tested-by: Linus Torvalds <tor...@li...> > > > Signed-off-by: Jesse Barnes <jb...@vi...> > > > > It would be nice if things like this could be discussed on this list in > > the first place... > > Well that's why I posted it here... I appreciate that, but the above headers suggest there's some history we're missing. > > > diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c > > > index 69aa0ab..c5df063 100644 > > > --- a/drivers/gpu/drm/drm_irq.c > > > +++ b/drivers/gpu/drm/drm_irq.c > > > @@ -539,9 +539,11 @@ out: > > > int drm_wait_vblank(struct drm_device *dev, void *data, > > > struct drm_file *file_priv) > > > { > > > + struct timeval now; > > > union drm_wait_vblank *vblwait = data; > > > int ret = 0; > > > unsigned int flags, seq, crtc; > > > + int timeout, max_wait = (DRM_HZ / 40) * 100; /* 100ms max */ > > > > Is that really a more reasonable timeout? It means e.g. the > > GLX_SGI_swap_control extension can't be used for swap intervals longer > > than 100 ms. Now I wouldn't expect such long intervals to be needed > > often, but it doesn't seem completely useless either (e.g. it allows > > apps which only need to update their display a couple of times per > > second to have a trivial rendering loop). > > > > Also, blocking signals for up to 100 ms sounds like bad news for the X > > server scheduler and SIGIO driven mouse cursor updates. > > If the server does long waits, yeah it could affect scheduling. But really > there are two problems here: one is the signal problem that causes an > infinite loop of never completed vblank waits, and another is lots missed > interrupts causing apps to hang for awhile. > > Maybe we could do an interruptible wait if the framecount was greater than say > 10, but do an uninterruptible one with a small max timeout for less (numbers > are arbitrarily picked here). That wouldn't help for the X server hangs I used to get with radeon, where the counters were way off due to broken wraparound handling. Are you sure the problems you're dealing with aren't that but lost interrupts? > > Maybe the kernel code could remember how long it waited before being > > interrupted by a signal and then decrease that from the timeout next > > time or something like that? > > Probably not without a new arg to the ioctl like Thomas said (or a huge new > mess of code to track processes what what we think they want), [...] I'm not sure either is necessary. Just store the timeout in the file_priv, decrease it when the wait is interrupted and reset it to the maximum when the wait finishes. Or could there be several threads sharing the same file descriptor? > And keep in mind that vblank waits like this are inherently more racy than a > good swapbuffers implementation, since they depend on the app getting > scheduled in time to actually avoid tearing. Sure, but this is borderline crippling the core functionality of this ioctl. -- Earthling Michel Dänzer | http://www.vmware.com Libre software enthusiast | Debian, X and DRI developer |
From: Jesse B. <jb...@vi...> - 2009-01-30 17:26:10
|
On Friday, January 30, 2009 4:47 am Michel Dänzer wrote: > On Thu, 2009-01-29 at 09:15 -0800, Jesse Barnes wrote: > > On Thursday, January 29, 2009 12:08 am Michel Dänzer wrote: > > > On Wed, 2009-01-28 at 20:55 -0800, Jesse Barnes wrote: > > > > Reported-by: Linus Torvalds <tor...@li...> > > > > Tested-by: Linus Torvalds <tor...@li...> > > > > Signed-off-by: Jesse Barnes <jb...@vi...> > > > > > > It would be nice if things like this could be discussed on this list in > > > the first place... > > > > Well that's why I posted it here... > > I appreciate that, but the above headers suggest there's some history > we're missing. Yeah, Linus mailed me off list about a bug that looked at lot like 18041, but he wanted his current userspace to work (a reasonable position). > > > > + struct timeval now; > > > > union drm_wait_vblank *vblwait = data; > > > > int ret = 0; > > > > unsigned int flags, seq, crtc; > > > > + int timeout, max_wait = (DRM_HZ / 40) * 100; /* 100ms max */ > > > > > > Is that really a more reasonable timeout? It means e.g. the > > > GLX_SGI_swap_control extension can't be used for swap intervals longer > > > than 100 ms. Now I wouldn't expect such long intervals to be needed > > > often, but it doesn't seem completely useless either (e.g. it allows > > > apps which only need to update their display a couple of times per > > > second to have a trivial rendering loop). > > > > > > Also, blocking signals for up to 100 ms sounds like bad news for the X > > > server scheduler and SIGIO driven mouse cursor updates. > > > > If the server does long waits, yeah it could affect scheduling. But > > really there are two problems here: one is the signal problem that causes > > an infinite loop of never completed vblank waits, and another is lots > > missed interrupts causing apps to hang for awhile. > > > > Maybe we could do an interruptible wait if the framecount was greater > > than say 10, but do an uninterruptible one with a small max timeout for > > less (numbers are arbitrarily picked here). > > That wouldn't help for the X server hangs I used to get with radeon, > where the counters were way off due to broken wraparound handling. Are > you sure the problems you're dealing with aren't that but lost > interrupts? I don't think they're wraparound problems in general, no, I think they're interrupt enable/disable problems (waiting when interrupts on a given pipe are disabled). I'm not sure about Timo's problem though; this patch helps with a problem he was seeing too, but he sees it after running for awhile, not at VT switch. It's possible that his problem could be due to wraparound, if the jumps we see at interrupt off->on time are big enough, a few DPMS events could cause a wraparound related hang. Timo, do you have a reliable way of reproducing your problem? It sounds separate from 18041 so we may want to fix it differently. > > > Maybe the kernel code could remember how long it waited before being > > > interrupted by a signal and then decrease that from the timeout next > > > time or something like that? > > > > Probably not without a new arg to the ioctl like Thomas said (or a huge > > new mess of code to track processes what what we think they want), [...] > > I'm not sure either is necessary. Just store the timeout in the > file_priv, decrease it when the wait is interrupted and reset it to the > maximum when the wait finishes. Or could there be several threads > sharing the same file descriptor? Yeah, that's one concern. We'd also have to reset the timeout in the case where an interrupted wait ended up getting aborted by the client (i.e. client doesn't call back in after -EINTR), followed by a normal wait a bit later. That should be detectable though, since a new call would probably have a new sequence count, while a restarted call would have the same one. (Ok, so a *small* new mess of code to track this.) > > And keep in mind that vblank waits like this are inherently more racy > > than a good swapbuffers implementation, since they depend on the app > > getting scheduled in time to actually avoid tearing. > > Sure, but this is borderline crippling the core functionality of this > ioctl. Yeah, it's borderline, I guess I was hoping it would be on the right side of the border enough that I wouldn't have to rewrite it a dozen times. :) For a time, we just didn't disable vblank interrupts across VT switch (this is how we designed the reworked vblank code), but now we do, sigh. 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. -- Jesse Barnes, Intel Open Source Technology Center |
From: Jesse B. <jb...@vi...> - 2009-01-30 18:43:27
|
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, 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. -- 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..801cdc4 100644 --- a/drivers/gpu/drm/drm_irq.c +++ b/drivers/gpu/drm/drm_irq.c @@ -590,9 +590,8 @@ int drm_wait_vblank(struct drm_device *dev, void *data, vblwait->request.sequence, crtc); dev->last_vblank_wait[crtc] = vblwait->request.sequence; DRM_WAIT_ON(ret, dev->vbl_queue[crtc], 3 * DRM_HZ, - (((drm_vblank_count(dev, crtc) - - vblwait->request.sequence) <= (1 << 23)) || - !dev->irq_enabled)); + (drm_vblank_count(dev, crtc) >= + vblwait->request.sequence) || !dev->irq_enabled); if (ret != -EINTR) { struct timeval now; |
From: Timo A. <tja...@cc...> - 2009-01-30 19:17:11
|
On Fri, 30 Jan 2009, Jesse Barnes wrote: > I'm not sure about Timo's problem though; this patch helps with a problem he > was seeing too, but he sees it after running for awhile, not at VT switch. > It's possible that his problem could be due to wraparound, if the jumps we > see at interrupt off->on time are big enough, a few DPMS events could cause a > wraparound related hang. Timo, do you have a reliable way of reproducing > your problem? It sounds separate from 18041 so we may want to fix it > differently. I understood that 18041 was already fixed by the recent'ish commits to drm, and I haven't been able to reproduce that since they were added to the jaunty kernel (and vblank was again enabled in mesa). I can reproduce the momentary freezes after changing the VT a couple of times. That's what happens on a suspend/resume cycle as well, so it's probably what triggered it in the first place. And the patch you posted did fix this issue for me. t |
From: Jesse B. <jb...@vi...> - 2009-01-30 19:35:57
|
On Friday, January 30, 2009 11:16 am Timo Aaltonen wrote: > On Fri, 30 Jan 2009, Jesse Barnes wrote: > > I'm not sure about Timo's problem though; this patch helps with a problem > > he was seeing too, but he sees it after running for awhile, not at VT > > switch. It's possible that his problem could be due to wraparound, if the > > jumps we see at interrupt off->on time are big enough, a few DPMS events > > could cause a wraparound related hang. Timo, do you have a reliable way > > of reproducing your problem? It sounds separate from 18041 so we may > > want to fix it differently. > > I understood that 18041 was already fixed by the recent'ish commits to > drm, and I haven't been able to reproduce that since they were added to > the jaunty kernel (and vblank was again enabled in mesa). > > I can reproduce the momentary freezes after changing the VT a couple of > times. That's what happens on a suspend/resume cycle as well, so it's > probably what triggered it in the first place. And the patch you posted > did fix this issue for me. Ah ok, then it probably is the same issue. Sounds like you're just hitting the timeout in libdrm, which would cause apps to freeze for about a second after a VT switch. I think the sequence number the apps are waiting for is actually less than the current one, but our current code prevents that from being detected. Does this patch also fix things for you? -- 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; |
From: Timo A. <tja...@cc...> - 2009-01-30 22:57:59
|
On Fri, 30 Jan 2009, Jesse Barnes wrote: > On Friday, January 30, 2009 11:16 am Timo Aaltonen wrote: >> On Fri, 30 Jan 2009, Jesse Barnes wrote: >>> I'm not sure about Timo's problem though; this patch helps with a problem >>> he was seeing too, but he sees it after running for awhile, not at VT >>> switch. It's possible that his problem could be due to wraparound, if the >>> jumps we see at interrupt off->on time are big enough, a few DPMS events >>> could cause a wraparound related hang. Timo, do you have a reliable way >>> of reproducing your problem? It sounds separate from 18041 so we may >>> want to fix it differently. >> >> I understood that 18041 was already fixed by the recent'ish commits to >> drm, and I haven't been able to reproduce that since they were added to >> the jaunty kernel (and vblank was again enabled in mesa). >> >> I can reproduce the momentary freezes after changing the VT a couple of >> times. That's what happens on a suspend/resume cycle as well, so it's >> probably what triggered it in the first place. And the patch you posted >> did fix this issue for me. > > Ah ok, then it probably is the same issue. Sounds like you're just hitting > the timeout in libdrm, which would cause apps to freeze for about a second > after a VT switch. I think the sequence number the apps are waiting for is > actually less than the current one, but our current code prevents that from > being detected. Does this patch also fix things for you? Yes it did. t |
From: Michel D. <mi...@da...> - 2009-01-31 12:49:51
|
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. > 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. -- Earthling Michel Dänzer | http://www.vmware.com Libre software enthusiast | Debian, X and DRI developer |
From: Jesse B. <jb...@vi...> - 2009-01-31 16:42:10
|
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; |
From: Michel D. <mi...@da...> - 2009-02-02 16:43:33
|
On Sat, 2009-01-31 at 08:15 -0800, Jesse Barnes wrote: > 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? It should allow waiting for up to about one or two days (38.8 hours at 60 Hz to be exact :), anything else is considered a miss. From today's perspective, it might make sense to further restrict that to about a minute or so. (A minor complication being that the same logic exists in userspace code, not sure what would happen if they didn't match anymore) However, that isn't what your proposed change does, is it? Doesn't it even increase the maximum wait span to the positive range of an int, which equals more than a year at 60 Hz? > > > 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. There should be corresponding modeset ioctl calls to compensate for that? > 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. If this was fixed, would any of the other changes you proposed in this thread be necessary? Wouldn't they just be papering over the real issue, which is the jumping counter? -- Earthling Michel Dänzer | http://www.vmware.com Libre software enthusiast | Debian, X and DRI developer |
From: Jesse B. <jb...@vi...> - 2009-02-02 17:17:01
|
On Monday, February 2, 2009 8:43 am Michel Dänzer wrote: > On Sat, 2009-01-31 at 08:15 -0800, Jesse Barnes wrote: > > 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? > > It should allow waiting for up to about one or two days (38.8 hours at > 60 Hz to be exact :), anything else is considered a miss. From today's > perspective, it might make sense to further restrict that to about a > minute or so. (A minor complication being that the same logic exists in > userspace code, not sure what would happen if they didn't match anymore) > > However, that isn't what your proposed change does, is it? Doesn't it > even increase the maximum wait span to the positive range of an int, > which equals more than a year at 60 Hz? Yeah, you *could* pass in a value that expired way in the future, but it would either be caught by the builtin 3s timeout or by libdrm's 1s timeout, right? So capping the sequence space at the cost of not properly handling events with very distant (but still past!) sequence values doesn't seem like a good tradeoff. > > > > 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. > > There should be corresponding modeset ioctl calls to compensate for > that? There are modeset calls, but I'm saying they don't compensate for it, since last_vblank is only set by the expiration timer. pre-modeset just takes a vblank ref, and post-modeset releases it, which works for any driver that doesn't uninstall its IRQ handler at VT switch time. > > 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. > > If this was fixed, would any of the other changes you proposed in this > thread be necessary? Wouldn't they just be papering over the real issue, > which is the jumping counter? The real issue is that passing in a vblank sequence that's already passed doesn't return immediately like it should. The jumping counter exposed that problem and should probably be fixed too (just to keep the counter values more friendly) but the core problem should be fixed first, imo. -- Jesse Barnes, Intel Open Source Technology Center |
From: Michel D. <mi...@da...> - 2009-02-02 22:49:42
|
On Mon, 2009-02-02 at 09:16 -0800, Jesse Barnes wrote: > On Monday, February 2, 2009 8:43 am Michel Dänzer wrote: > > On Sat, 2009-01-31 at 08:15 -0800, Jesse Barnes wrote: > > > 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? > > > > It should allow waiting for up to about one or two days (38.8 hours at > > 60 Hz to be exact :), anything else is considered a miss. From today's > > perspective, it might make sense to further restrict that to about a > > minute or so. (A minor complication being that the same logic exists in > > userspace code, not sure what would happen if they didn't match anymore) > > > > However, that isn't what your proposed change does, is it? Doesn't it > > even increase the maximum wait span to the positive range of an int, > > which equals more than a year at 60 Hz? Actually, I'm afraid I was getting confused... Looking at the current code again, it does about the opposite of what I said above: it terminates the wait if the target sequence is up to a day behind the counter, so it would allow waiting for up to more than two years. However, if we are to change this test, I'd suggest going much further than your proposal (which btw is really just a fancy way of replacing the shift of 23 with 31 :): e.g. something like (vblwait->request.sequence - drm_vblank_count(dev, crtc) - 1) > (1 << 23) should do what I said above. Then if we reduce the right hand side to e.g. 3600, only waiting for up to 3600 frames (one minute at 60 Hz) is possible, anything else is considered a miss. (I hope I didn't make any stupid mistake in the math again, it's getting late... but I'm quite confident the principle is sound) > > > 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. > > > > If this was fixed, would any of the other changes you proposed in this > > thread be necessary? Wouldn't they just be papering over the real issue, > > which is the jumping counter? > > The real issue is that passing in a vblank sequence that's already passed > doesn't return immediately like it should. Well, the problem is exactly what 'already passed' means. > The jumping counter exposed that problem and should probably be fixed > too (just to keep the counter values more friendly) but the core > problem should be fixed first, imo. I still disagree there: If the counter doesn't jump, even the current code works just fine as long as userspace calls the ioctl at least once a day. We can try to contain the damage caused by a jumping counter, but we can't completely eliminate it in all cases. Even if we can usually handle it sanely in the kernel, userspace libraries / apps may not be so robust. From my POV the core problem is clearly the jumping counter, everything else discussed in this thread is just workarounds to paper over it. -- Earthling Michel Dänzer | http://www.vmware.com Libre software enthusiast | Debian, X and DRI developer |
From: Jesse B. <jb...@vi...> - 2009-02-03 02:15:31
|
On Monday, February 2, 2009 2:49 pm Michel Dänzer wrote: > Actually, I'm afraid I was getting confused... Looking at the current > code again, it does about the opposite of what I said above: it > terminates the wait if the target sequence is up to a day behind the > counter, so it would allow waiting for up to more than two years. > > However, if we are to change this test, I'd suggest going much further > than your proposal (which btw is really just a fancy way of replacing > the shift of 23 with 31 :): e.g. something like > > (vblwait->request.sequence - drm_vblank_count(dev, crtc) - 1) > (1 << 23) > > should do what I said above. Then if we reduce the right hand side to > e.g. 3600, only waiting for up to 3600 frames (one minute at 60 Hz) is > possible, anything else is considered a miss. > > (I hope I didn't make any stupid mistake in the math again, it's getting > late... but I'm quite confident the principle is sound) Ok let's go through a few cases with your above test (the 8 million count one), I think you got the math right with the corrected test above :): a) request greater than current sequence (the typical one hopefully) b) request less than current sequence c) request greater than current sequence, but due to wrapping (vblwait->request.sequence - drm_vblank_count(dev, crtc) - 1) > (1 << 23) So for case (a) with say request = 500 vs cur = 482, we'd have (500 - 482 - 1) > (1 << 23), which is false, so we'd stay in the loop, good. And for case (b), say request = 320 and cur = 698, we'd have (320 - 698 - 1) > (1 << 23), which is true, so we'd break out, good (this even works for large cur vs. request differences). And for case (c), say request = 8388611 and cur = 1, we'd have (8388611 - 1 - 1) > (1 << 23), which is true, so we'd break out. Again good. And yes, the patch I posted is a "fancy" but also typical way of doing this (just look around the kernel, it's a pretty common idiom to cast stuff to signed and compare against 0). As for the higher level question, I hope we both agree that we should fix this up, though I'm not sure why we need a frame count cap at all. We've already got the timeout, so shouldn't we just keep the code simple? Is debugging tearing easier than debugging a hang? I guess that's what we're really getting at... > > The jumping counter exposed that problem and should probably be fixed > > too (just to keep the counter values more friendly) but the core > > problem should be fixed first, imo. > > I still disagree there: If the counter doesn't jump, even the current > code works just fine as long as userspace calls the ioctl at least once > a day. We can try to contain the damage caused by a jumping counter, but > we can't completely eliminate it in all cases. Even if we can usually > handle it sanely in the kernel, userspace libraries / apps may not be so > robust. From my POV the core problem is clearly the jumping counter, > everything else discussed in this thread is just workarounds to paper > over it. I looked at this more; apparently the framecount reg on at least my machine is running way too fast, which triggered the behavior we talked about earlier. But given that we didn't use to track frame counts across VT switch (many drivers removed their IRQ handler, so the frame count wouldn't increase between LeaveVT and EnterVT) and things seemed ok, it's probably fine to just return 0 from the get_vblank_counter hook, and ignore frames we lost between interrupt/vblank off periods. Anyway I'll get that fixed one way or the other... So... if you feel strongly about the frame count cap, go ahead and push a change like you suggested above, otherwise you can ack the patch I posted earlier and everyone will be happy. :) -- Jesse Barnes, Intel Open Source Technology Center |
From: Michel D. <mi...@da...> - 2009-02-03 09:34:50
|
On Mon, 2009-02-02 at 18:15 -0800, Jesse Barnes wrote: > On Monday, February 2, 2009 2:49 pm Michel Dänzer wrote: > > As for the higher level question, I hope we both agree that we should fix this > up, I'm fine with it, as long as it's clear that it's just a workaround for bad counter behaviour. > though I'm not sure why we need a frame count cap at all. We've already > got the timeout, so shouldn't we just keep the code simple? Is debugging > tearing easier than debugging a hang? I guess that's what we're really > getting at... I'm afraid you lost me here, care to elaborate on what you mean by 'frame count cap' and what this has to do with debugging tearing vs. hangs? If by 'frame count cap' you mean the limit for the number of frames that can be waited for, that's implicitly there (even if we only considered a single delta as 'has passed', one couldn't wait for more than (2 ^ 32 - 1) frames) and your proposal changes it in the same direction, just only half the way. I'm suggesting to take it further if at all, as the motivation is to catch pathological cases rather than being able to wait as long as possible. > > > The jumping counter exposed that problem and should probably be fixed > > > too (just to keep the counter values more friendly) but the core > > > problem should be fixed first, imo. > > > > I still disagree there: If the counter doesn't jump, even the current > > code works just fine as long as userspace calls the ioctl at least once > > a day. We can try to contain the damage caused by a jumping counter, but > > we can't completely eliminate it in all cases. Even if we can usually > > handle it sanely in the kernel, userspace libraries / apps may not be so > > robust. From my POV the core problem is clearly the jumping counter, > > everything else discussed in this thread is just workarounds to paper > > over it. > > I looked at this more; apparently the framecount reg on at least my machine is > running way too fast, which triggered the behavior we talked about earlier. > But given that we didn't use to track frame counts across VT switch (many > drivers removed their IRQ handler, so the frame count wouldn't increase > between LeaveVT and EnterVT) and things seemed ok, Yeah, I think a counter that doesn't move is less problematic than one that moves erratically from the POV of users of this functionality. > it's probably fine to just return 0 from the get_vblank_counter hook, > and ignore frames we lost between interrupt/vblank off periods. Not sure I understand what you're proposing here, do you have a patch? -- Earthling Michel Dänzer | http://www.vmware.com Libre software enthusiast | Debian, X and DRI developer |
From: Jesse B. <jb...@vi...> - 2009-02-03 16:58:41
|
On Tuesday, February 3, 2009 1:34 am Michel Dänzer wrote: > On Mon, 2009-02-02 at 18:15 -0800, Jesse Barnes wrote: > > On Monday, February 2, 2009 2:49 pm Michel Dänzer wrote: > > > > As for the higher level question, I hope we both agree that we should fix > > this up, > > I'm fine with it, as long as it's clear that it's just a workaround for > bad counter behaviour. > > > though I'm not sure why we need a frame count cap at all. We've already > > got the timeout, so shouldn't we just keep the code simple? Is debugging > > tearing easier than debugging a hang? I guess that's what we're really > > getting at... > > I'm afraid you lost me here, care to elaborate on what you mean by > 'frame count cap' and what this has to do with debugging tearing vs. > hangs? > > If by 'frame count cap' you mean the limit for the number of frames that > can be waited for, that's implicitly there (even if we only considered a > single delta as 'has passed', one couldn't wait for more than (2 ^ 32 - > 1) frames) and your proposal changes it in the same direction, just only > half the way. I'm suggesting to take it further if at all, as the > motivation is to catch pathological cases rather than being able to wait > as long as possible. Yeah I mean a (small) limit to the number of frames we can wait for. Without an explicit limit we'll be capped at 2^31 frames. What I'm saying is that anything greater than a few hundred frames (up to the limit) will be caught by the timeout, and anything greater than the limit will return instantly. It sounds like you're suggesting we make the code return instantly if anything greater than a few thousand frames is passed in, which is fine (though I don't know if there are displays with khz or mhz refresh rates out there where that would cause problems). > > > > > The jumping counter exposed that problem and should probably be fixed > > > > too (just to keep the counter values more friendly) but the core > > > > problem should be fixed first, imo. > > > > > > I still disagree there: If the counter doesn't jump, even the current > > > code works just fine as long as userspace calls the ioctl at least once > > > a day. We can try to contain the damage caused by a jumping counter, > > > but we can't completely eliminate it in all cases. Even if we can > > > usually handle it sanely in the kernel, userspace libraries / apps may > > > not be so robust. From my POV the core problem is clearly the jumping > > > counter, everything else discussed in this thread is just workarounds > > > to paper over it. > > > > I looked at this more; apparently the framecount reg on at least my > > machine is running way too fast, which triggered the behavior we talked > > about earlier. But given that we didn't use to track frame counts across > > VT switch (many drivers removed their IRQ handler, so the frame count > > wouldn't increase between LeaveVT and EnterVT) and things seemed ok, > > Yeah, I think a counter that doesn't move is less problematic than one > that moves erratically from the POV of users of this functionality. > > > it's probably fine to just return 0 from the get_vblank_counter hook, > > and ignore frames we lost between interrupt/vblank off periods. > > Not sure I understand what you're proposing here, do you have a patch? No you got it; it would keep the counter from jumping at all between IRQ/vblank off->on transitions. The only downside is that apps that do their own calculations for vblank sequence numbers and then try to use them across such periods might be surprised, but the GL wrappers around the ioctl should handle that case. The patch below is all it takes. Remind me again why we bothered to keep an accurate count across off->on periods? :) -- Jesse Barnes, Intel Open Source Technology Center diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c index 6290219..879a5d3 100644 --- a/drivers/gpu/drm/i915/i915_irq.c +++ b/drivers/gpu/drm/i915/i915_irq.c @@ -137,41 +137,17 @@ i915_pipe_enabled(struct drm_device *dev, int pipe) return 0; } -/* Called from drm generic code, passed a 'crtc', which - * we use as a pipe index +/* + * Stubbing this out keeps us from tracking lost frames across + * vblank/IRQ off periods, but that should be fine, since it will + * stay accurate as long as there's a client waiting. And at + * VT switch any frames that occur between LeaveVT and EnterVT + * will be invisible to the client, which is arguably correct + * anyway. */ u32 i915_get_vblank_counter(struct drm_device *dev, int pipe) { - drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private; - unsigned long high_frame; - unsigned long low_frame; - u32 high1, high2, low, count; - - high_frame = pipe ? PIPEBFRAMEHIGH : PIPEAFRAMEHIGH; - low_frame = pipe ? PIPEBFRAMEPIXEL : PIPEAFRAMEPIXEL; - - if (!i915_pipe_enabled(dev, pipe)) { - DRM_ERROR("trying to get vblank count for disabled pipe %d\n", pipe); - return 0; - } - - /* - * High & low register fields aren't synchronized, so make sure - * we get a low value that's stable across two reads of the high - * register. - */ - do { - high1 = ((I915_READ(high_frame) & PIPE_FRAME_HIGH_MASK) >> - PIPE_FRAME_HIGH_SHIFT); - low = ((I915_READ(low_frame) & PIPE_FRAME_LOW_MASK) >> - PIPE_FRAME_LOW_SHIFT); - high2 = ((I915_READ(high_frame) & PIPE_FRAME_HIGH_MASK) >> - PIPE_FRAME_HIGH_SHIFT); - } while (high1 != high2); - - count = (high1 << 8) | low; - - return count; + return 0; } irqreturn_t i915_driver_irq_handler(DRM_IRQ_ARGS) |
From: Michel D. <mi...@da...> - 2009-02-03 17:26:18
|
On Tue, 2009-02-03 at 08:58 -0800, Jesse Barnes wrote: > On Tuesday, February 3, 2009 1:34 am Michel Dänzer wrote: > > On Mon, 2009-02-02 at 18:15 -0800, Jesse Barnes wrote: > > > On Monday, February 2, 2009 2:49 pm Michel Dänzer wrote: > > > > > > though I'm not sure why we need a frame count cap at all. We've already > > > got the timeout, so shouldn't we just keep the code simple? Is debugging > > > tearing easier than debugging a hang? I guess that's what we're really > > > getting at... > > > > I'm afraid you lost me here, care to elaborate on what you mean by > > 'frame count cap' and what this has to do with debugging tearing vs. > > hangs? > > > > If by 'frame count cap' you mean the limit for the number of frames that > > can be waited for, that's implicitly there (even if we only considered a > > single delta as 'has passed', one couldn't wait for more than (2 ^ 32 - > > 1) frames) and your proposal changes it in the same direction, just only > > half the way. I'm suggesting to take it further if at all, as the > > motivation is to catch pathological cases rather than being able to wait > > as long as possible. > > Yeah I mean a (small) limit to the number of frames we can wait for. Without > an explicit limit we'll be capped at 2^31 frames. You mean with your proposed explicit limit. :) > What I'm saying is that anything greater than a few hundred frames (up > to the limit) will be caught by the timeout, If it wasn't for the problems that triggered all these discussions... > and anything greater than the limit will return instantly. It sounds > like you're suggesting we make the code return instantly if anything > greater than a few thousand frames is passed in, which is fine (though > I don't know if there are displays with khz or mhz refresh rates out > there where that would cause problems). The most I've heard of is 240 Hz, which would be fine. I suspect anything with a significantly higher refresh would be different enough from displays as we know them that a lot more than this would need to be revisited anyway. :) > > > I looked at this more; apparently the framecount reg on at least my > > > machine is running way too fast, which triggered the behavior we talked > > > about earlier. But given that we didn't use to track frame counts across > > > VT switch (many drivers removed their IRQ handler, so the frame count > > > wouldn't increase between LeaveVT and EnterVT) and things seemed ok, > > > > Yeah, I think a counter that doesn't move is less problematic than one > > that moves erratically from the POV of users of this functionality. > > > > > it's probably fine to just return 0 from the get_vblank_counter hook, > > > and ignore frames we lost between interrupt/vblank off periods. > > > > Not sure I understand what you're proposing here, do you have a patch? > > No you got it; it would keep the counter from jumping at all between > IRQ/vblank off->on transitions. The only downside is that apps that do their > own calculations for vblank sequence numbers and then try to use them across > such periods might be surprised, but the GL wrappers around the ioctl should > handle that case. The patch below is all it takes. Remind me again why we > bothered to keep an accurate count across off->on periods? :) I don't think this is a good general solution; just because your driver can't maintain a consistent counter under all circumstances doesn't mean it should never even try. Also, it might be possible to disable vblank interrupts immediately instead of only after a delay, and avoid some wakeups with apps that only wait for every second or third refresh or something like that? -- Earthling Michel Dänzer | http://www.vmware.com Libre software enthusiast | Debian, X and DRI developer |
From: Jesse B. <jb...@vi...> - 2009-02-03 17:47:55
|
On Tuesday, February 3, 2009 9:26 am Michel Dänzer wrote: > On Tue, 2009-02-03 at 08:58 -0800, Jesse Barnes wrote: > > What I'm saying is that anything greater than a few hundred frames (up > > to the limit) will be caught by the timeout, > > If it wasn't for the problems that triggered all these discussions... No, remember the problem that triggered this was the current, broken test, that was returning false for sequence numbers less than a few million of the current sequence number. So with a large cap (2^31) or a small cap that problem will be fixed. > > No you got it; it would keep the counter from jumping at all between > > IRQ/vblank off->on transitions. The only downside is that apps that do > > their own calculations for vblank sequence numbers and then try to use > > them across such periods might be surprised, but the GL wrappers around > > the ioctl should handle that case. The patch below is all it takes. > > Remind me again why we bothered to keep an accurate count across off->on > > periods? :) > > I don't think this is a good general solution; just because your driver > can't maintain a consistent counter under all circumstances doesn't mean > it should never even try. It can; when interrupts are on. And when they're off (i.e. no clients are waiting and/or we're VT switched away), why do we care about keeping the frame count accurate? DPMS events will turn off the display too after all, and apps won't generally be aware of that, so stopping the counter arbitrarily needs to be safe anyway. > Also, it might be possible to disable vblank interrupts immediately > instead of only after a delay, and avoid some wakeups with apps that > only wait for every second or third refresh or something like that? Yeah, or you could reduce the delay. But if your app has a swap interval set to indicate it wants to wake up every 2 or 3 frames, it'll still take a vblank ref, so we won't disable (unless it's trying to time a single frame wait by itself of course, dunno how common that is). -- Jesse Barnes, Intel Open Source Technology Center |
From: Michel D. <mi...@da...> - 2009-02-03 18:01:05
|
On Tue, 2009-02-03 at 09:47 -0800, Jesse Barnes wrote: > On Tuesday, February 3, 2009 9:26 am Michel Dänzer wrote: > > On Tue, 2009-02-03 at 08:58 -0800, Jesse Barnes wrote: > > > What I'm saying is that anything greater than a few hundred frames (up > > > to the limit) will be caught by the timeout, > > > > If it wasn't for the problems that triggered all these discussions... > > No, remember the problem that triggered this was the current, broken test, > that was returning false for sequence numbers less than a few million of the > current sequence number. So with a large cap (2^31) or a small cap that > problem will be fixed. The existing test isn't broken, it has a 'cap' of (2^32 - 2^23). Your change just happens to help with your driver problem because it changes the 'cap' such that counter jumps by your dev->max_vblank_count result in reasonable wait requests to be considered as 'has passed'. Maybe we'll just have to agree to completely disagree on this. :} -- Earthling Michel Dänzer | http://www.vmware.com Libre software enthusiast | Debian, X and DRI developer |