FAQ Search Today's Posts Mark Forums Read
» Video Reviews

» Linux Archive

Linux-archive is a website aiming to archive linux email lists and to make them easily accessible for linux users/developers.


» Sponsor

» Partners

» Sponsor

Go Back   Linux Archive > Ubuntu > Ubuntu Kernel Team

 
 
LinkBack Thread Tools
 
Old 02-02-2009, 08:10 AM
Timo Aaltonen
 
Default use uninterruptible sleep for vblank waits

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.

--
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
",
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;

--
kernel-team mailing list
kernel-team@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/kernel-team
 
Old 02-03-2009, 12:37 PM
Tim Gardner
 
Default use uninterruptible sleep for vblank waits

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@canonical.com

--
kernel-team mailing list
kernel-team@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/kernel-team
 

Thread Tools




All times are GMT. The time now is 11:19 AM.

VBulletin, Copyright ©2000 - 2014, Jelsoft Enterprises Ltd.
Content Relevant URLs by vBSEO ©2007, Crawlability, Inc.
Copyright ©2007 - 2008, www.linux-archive.org