[Intel-gfx] [PATCH] drm: Aggressively disable vblanks

Tue Dec 21 20:36:13 PST 2010

> ----------------------------------------------------------------------
>
> Message: 1
> Date: Mon, 20 Dec 2010 19:23:40 -0800
> From: Keith Packard <keithp at keithp.com>
> Subject: Re: [Intel-gfx] [PATCH] drm: Aggressively disable vblanks
> To: Andy Lutomirski <luto at MIT.EDU>, Jesse Barnes
> 	<jbarnes at virtuousgeek.org>,	Chris Wilson <chris at chris-wilson.co.uk>,
> 	David Airlie <airlied at linux.ie>
> Cc: intel-gfx at lists.freedesktop.org, dri-devel at lists.freedesktop.org
> Message-ID: <yunfwtrrepv.fsf at aiko.keithp.com>
> Content-Type: text/plain; charset="us-ascii"
>
> On Mon, 20 Dec 2010 14:00:54 -0500, Andy Lutomirski <luto at MIT.EDU>  
> wrote:
>
>> Enabling and disabling the vblank interrupt (on devices that
>> support it) is cheap.  So disable it quickly after each
>> interrupt.
>
> So, the concern (and reason for the original design) was that you  
> might
> lose count of vblank interrupts as vblanks are counted differently  
> while
> off than while on. If vblank interrupts get enabled near the interrupt
> time, is it possible that you'll end up off by one somehow?
>
> Leaving them enabled for 'a while' was supposed to reduce the  
> impact of
> this potential error.
>
> -- 
> keith.packard at intel.com
> -------------- next part --------------
> A non-text attachment was scrubbed...
> Name: not available
> Type: application/pgp-signature
> Size: 189 bytes
> Desc: not available
> URL: <http://lists.freedesktop.org/archives/dri-devel/attachments/ 
> 20101220/105a9fb6/attachment-0001.pgp>
>
> ------------------------------
>
> Message: 2
> Date: Mon, 20 Dec 2010 22:34:12 -0500
> From: Andrew Lutomirski <luto at mit.edu>
> Subject: Re: [Intel-gfx] [PATCH] drm: Aggressively disable vblanks
> To: Keith Packard <keithp at keithp.com>
> Cc: intel-gfx at lists.freedesktop.org, dri-devel at lists.freedesktop.org
> Message-ID:
> 	<AANLkTik-1zni1DdS6i+1ARoenx6p=9jQAAiA6t5uGg_K at mail.gmail.com>
> Content-Type: text/plain; charset=ISO-8859-1
>
> On Mon, Dec 20, 2010 at 10:23 PM, Keith Packard <keithp at keithp.com>  
> wrote:
>> On Mon, 20 Dec 2010 14:00:54 -0500, Andy Lutomirski <luto at MIT.EDU>  
>> wrote:
>>
>>> Enabling and disabling the vblank interrupt (on devices that
>>> support it) is cheap. ?So disable it quickly after each
>>> interrupt.
>>
>> So, the concern (and reason for the original design) was that you  
>> might
>> lose count of vblank interrupts as vblanks are counted differently  
>> while
>> off than while on. If vblank interrupts get enabled near the  
>> interrupt
>> time, is it possible that you'll end up off by one somehow?
>
> So the race is:
>
> 1. Vblank happens.
> 2. vblank_get runs, reads hardware counter, and enables the interrupt
> (and maybe not quite in that order)
> 3. Interrupt fires and increments the counter.  Now the counter is  
> one too high.
>
> What if we read the hardware counter from the IRQ the first time that
> it fires after being enabled?  That way, if the hardware and software
> counters match (mod 2^23 or whatever the magic number is), we don't
> increment the counter.
>
>>
>> Leaving them enabled for 'a while' was supposed to reduce the  
>> impact of
>> this potential error.
>>
>
> Fair enough,
>
> But five seconds is a *long* time, and anything short enough that the
> interrupt actually gets turned off in normal use risks the same race.
>
> --Andy
>
>
> ------------------------------
>
> Message: 3
> Date: Mon, 20 Dec 2010 19:47:11 -0800
> From: Keith Packard <keithp at keithp.com>
> Subject: Re: [Intel-gfx] [PATCH] drm: Aggressively disable vblanks
> To: Andrew Lutomirski <luto at mit.edu>
> Cc: intel-gfx at lists.freedesktop.org, dri-devel at lists.freedesktop.org
> Message-ID: <yunbp4frdmo.fsf at aiko.keithp.com>
> Content-Type: text/plain; charset="us-ascii"
>
> On Mon, 20 Dec 2010 22:34:12 -0500, Andrew Lutomirski  
> <luto at mit.edu> wrote:
>
>> But five seconds is a *long* time, and anything short enough that the
>> interrupt actually gets turned off in normal use risks the same race.
>
> Right, so eliminating any race seems like the basic requirement. Can
> that be done?
>
> -- 
> keith.packard at intel.com
> -------------- next part --------------
> A non-text attachment was scrubbed...
> Name: not available
> Type: application/pgp-signature
> Size: 189 bytes
> Desc: not available
> URL: <http://lists.freedesktop.org/archives/dri-devel/attachments/ 
> 20101220/5ca3b674/attachment-0001.pgp>
>
> ------------------------------
>
> Message: 4
> Date: Mon, 20 Dec 2010 22:55:46 -0500
> From: Andrew Lutomirski <luto at mit.edu>
> Subject: Re: [Intel-gfx] [PATCH] drm: Aggressively disable vblanks
> To: Keith Packard <keithp at keithp.com>
> Cc: intel-gfx at lists.freedesktop.org, dri-devel at lists.freedesktop.org
> Message-ID:
> 	<AANLkTimk6RLkr8Lt76cR8ncLW_3kaX6Dqa+=id9_G-8C at mail.gmail.com>
> Content-Type: text/plain; charset=ISO-8859-1
>
> On Mon, Dec 20, 2010 at 10:47 PM, Keith Packard <keithp at keithp.com>  
> wrote:
>> On Mon, 20 Dec 2010 22:34:12 -0500, Andrew Lutomirski  
>> <luto at mit.edu> wrote:
>>
>>> But five seconds is a *long* time, and anything short enough that  
>>> the
>>> interrupt actually gets turned off in normal use risks the same  
>>> race.
>>
>> Right, so eliminating any race seems like the basic requirement. Can
>> that be done?
>
> I'll give it a shot.
>
> Do you know if there's an existing tool to call drm_wait_vblank from
> userspace for testing?  I know approximately nothing about libdrm or
> any userspace graphics stuff whatsoever.
>
> --Andy
>
>>
>> --
>> keith.packard at intel.com
>>
>
>
> ------------------------------
>
> Message: 5
> Date: Mon, 20 Dec 2010 20:03:53 -0800
> From: Jesse Barnes <jbarnes at virtuousgeek.org>
> Subject: Re: [Intel-gfx] [PATCH] drm: Aggressively disable vblanks
> To: Andrew Lutomirski <luto at mit.edu>
> Cc: intel-gfx at lists.freedesktop.org, dri-devel at lists.freedesktop.org
> Message-ID: <20101220200353.12479178 at jbarnes-desktop>
> Content-Type: text/plain; charset=US-ASCII
>
> On Mon, 20 Dec 2010 22:55:46 -0500
> Andrew Lutomirski <luto at mit.edu> wrote:
>
>> On Mon, Dec 20, 2010 at 10:47 PM, Keith Packard  
>> <keithp at keithp.com> wrote:
>>> On Mon, 20 Dec 2010 22:34:12 -0500, Andrew Lutomirski  
>>> <luto at mit.edu> wrote:
>>>
>>>> But five seconds is a *long* time, and anything short enough  
>>>> that the
>>>> interrupt actually gets turned off in normal use risks the same  
>>>> race.
>>>
>>> Right, so eliminating any race seems like the basic requirement. Can
>>> that be done?
>>
>> I'll give it a shot.
>>
>> Do you know if there's an existing tool to call drm_wait_vblank from
>> userspace for testing?  I know approximately nothing about libdrm or
>> any userspace graphics stuff whatsoever.
>
> Yeah, libdrm has a test program called vbltest; it should do what you
> need.
>

Not so fast please! After a lot of review by Jesse, Dave and Chris  
just merged a set of my patches into the drm-next (and the intel and  
radeon kms drivers) to implement precise timestamping of vblank's and  
pageflip completion and vblank counting for DRI2 and the  
OML_sync_control extension. It also fixes (hopefully) almost all race- 
conditions (that i could find or think of) related to vblank irq on/ 
off, a few of them surprising and due to "funny" behaviour of some  
gpu's when you enable/disable vblanks (e.g., radeon's spontaneously  
firing a vblank irq in the middle of a scanout cycle when vblank  
irq's get enabled, or firing the irq sometimes shortly before a  
vblank instead of in the vblank).

There's one tiny race left in the vblank off path, which i wanted to  
address during the next weeks. Also i need to implement support for  
nouveau. After that we could simply reduce the vblank off timeout to  
something small like 50 msecs. Or use Andrew's heuristic on top of this.

In any case, please check against the drm-next branch. I think your  
patches touch/conflict with most of the areas in drm that are  
modified in drm-next. At least my users need a very high level of  
precision and robustness in vblank counting and timestamping for  
neuro-science applications and similar stuff.

thanks,
-mario

*********************************************************************
Mario Kleiner
Max Planck Institute for Biological Cybernetics
Spemannstr. 38
72076 Tuebingen
Germany

e-mail: mario.kleiner at tuebingen.mpg.de
office: +49 (0)7071/601-1623
fax:    +49 (0)7071/601-616
www:    http://www.kyb.tuebingen.mpg.de/~kleinerm
*********************************************************************
"For a successful technology, reality must take precedence
over public relations, for Nature cannot be fooled."
(Richard Feynman)