Re: [PATCH] use uninterruptible sleep for vblank waits

[Michel D. <mi...@da...>]

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