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 ------------------------------------------------------------------------------ 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
