[PATCH] use uninterruptible sleep for vblank waits

[Tim Gardner]

Timo Aaltonen wrote:
>  	Hi
> 
>    Here's a patch from a thread on dri-devel. It fixes bug 320813, and 
> probably needs this commit to apply cleanly:
> 
> drm: Add a debug node for vblank state fede5c91c4a8a7701d205b2b84b9835ddc7d6f02
> 
> It's still debated whether this is the final patch, but I've confirmed 
> that it works. We could probably have it as a SAUCE patch before it's 
> upstream?
> 
> 
> ---------- Forwarded message ----------
> Date: Sat, 31 Jan 2009 08:15:21 -0800
> From: Jesse Barnes
> To: Michel Dänzer
> Cc: Dave Airlie, dri-devel@
> Subject: Re: [PATCH] use uninterruptible sleep for vblank waits
> 
> 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.
> 

Timo - I would prefer to wait until it appears in Linus' tree in order
to avoid future rebase conflicts. I've been assigned to the bug which is
also mile-stoned for Jaunty A6, so we ought not forget it.

rtg
-- 
Tim Gardner tim.gardner at canonical.com