Re: [RFC PATCH] drm: Fix off-by-one races on vblank disable

From: "Michel Dänzer" <michel@daenzer.net>
To: Andy Lutomirski <luto@MIT.EDU>
Cc: Matthew Garrett <mjg59@srcf.ucam.org>,
	dri-devel@lists.freedesktop.org,
	Mario Kleiner <mario.kleiner@tuebingen.mpg.de>
Subject: Re: [RFC PATCH] drm: Fix off-by-one races on vblank disable
Date: Thu, 17 Nov 2011 09:30:21 +0100	[thread overview]
Message-ID: <1321518621.3794.83.camel@thor.local> (raw)
In-Reply-To: <4dfde72a33598c136eb3e4866325c523700dcc46.1321507426.git.luto@mit.edu>

[ Dropping intel-gfx list from CC, as it automatically rejects posts
from non-subscribers ]

On Mit, 2011-11-16 at 21:39 -0800, Andy Lutomirski wrote: 
> There are two possible races when disabling vblanks.  If the IRQ
> fired but the hardware didn't update its counter yet, then we store
> too low a hardware counter.  (Sensible hardware never does this.
> Apparently not all hardware is sensible.)

The thing is, 'the' IRQ and 'the' hardware counter aren't necessarily
about the same thing. We want an IRQ which triggers at the beginning of
vertical blank, but the Radeon hardware counters increment at the
beginning of scanout, i.e. at the end of vertical blank. Does that make
the hardware 'broken' or 'not sensible'?

> If, on the other hand, the counter updated but the IRQ didn't fire
> yet, we store too high a counter. 
> 
> We handled the former case with a heuristic based on timestamps and
> we did not handle the latter case.  By saving a little more state,
> we can handle both cases exactly: all we need to do is watch for
> changes in the difference between the hardware and software vblank
> counts.

I'm afraid that can't work:

Some (AFAIR also Intel) hardware resets the counter to 0 when the CRTC
is disabled / enabled (e.g. for DPMS, or a modeset). That's why we ended
up only counting interrupts while the IRQ is enabled, and only using the
hardware counter to fill in while the IRQ is disabled. The hardware
counter cannot be assumed to have any defined behaviour between enabling
and disabling the IRQ.

To compensate for this, the drivers call drm_vblank_pre_modeset (which
enables the IRQ, which also updates the virtual counter from the
hardware counter) before disabling the CRTC and drm_vblank_post_modeset
(which disables the IRQ, which also records the hardware counter) after
enabling the CRTC.

> This compiles but is not very well tested, because I don't know what
> tests to run.

Not sure there are any good tests yet. Mario, would it be possible to
extract something exercising the various corner cases from your toolkit?

-- 
Earthling Michel Dänzer           |                   http://www.amd.com
Libre software enthusiast         |          Debian, X and DRI developer
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel