From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Subject: Re: [Intel-gfx] [PATCH 5/5] drm/i915: Allow vblank interrupts during modeset and eliminate some vblank races Date: Wed, 28 May 2014 14:19:01 +0300 Message-ID: <20140528111900.GS27580@intel.com> References: <1393009415-27651-1-git-send-email-ville.syrjala@linux.intel.com> <1393009415-27651-6-git-send-email-ville.syrjala@linux.intel.com> <1393213735.27769.8.camel@thor.local> <20140224121101.GN3852@intel.com> <1393297106.27769.48.camel@thor.local> <20140304091343.GP17001@phenom.ffwll.local> <5385A896.9070600@daenzer.net> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Content-Disposition: inline In-Reply-To: <5385A896.9070600@daenzer.net> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Michel =?iso-8859-1?Q?D=E4nzer?= Cc: intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org On Wed, May 28, 2014 at 06:12:54PM +0900, Michel D=E4nzer wrote: > = > Digging out an ooold post of Daniel's... > = > On 04.03.2014 18:13, Daniel Vetter wrote: > > On Tue, Feb 25, 2014 at 11:58:26AM +0900, Michel D=E4nzer wrote: > >> = > >> When the pre/post-modeset hooks were originally added, it worked like > >> this: the pre-modeset hook enabled the vblank interrupt, which updated > >> the DRM vblank counter from the driver/HW counter. The post-modeset ho= ok > >> disabled the vblank interrupt again, which recorded the post-modeset > >> driver/HW counter value. > >> > >> But the vblank code has changed a lot since then, not sure it still > >> works like that. > > = > > It still works like that, but there's two fundamental issues with this > > trick: > > - There's a race where the vblank state is fubar right between the > > completion of the modeset and before the first vblank happened. > = > Can you provide more details about that? You mentioned on IRC that > sometimes 'bogus' DRM vblank counter values are returned to userspace. > The most likely cause of that would be drm_vblank_pre_modeset() being > called too late, i.e. after the hardware counter was reset. (Or if > you're reducing / eliminating the vblank disable timer, possibly the > vblank interrupt getting disabled too early, i.e. before the hardware > counter was reset) The hardware counter reset is a problem: 1. vblank_disable_and_save() updates .last 2. modeset/dpms/suspend (hw counter is reset) 3. drm_vblank_get() -> cur_vblank-.last =3D=3D garbage The lack of drm_vblank_on() is a problem: 1. drm_vblank_get() 2. drm_vblank_off() 3. modeset/dpms/suspend 4. drm_vblank_get() -> -EINVAL Another issue: 1. drm_vblank_get() 2. drm_vblank_put() 3. disable timer expires which updates .last ... 4. drm_vblank_off() updates .last again 5. modeset/dpms/suspend 6. drm_vblank_get() -> sequence number doesn't account for the time between 3. and 4. I suppose this isn't a big issue, but I don't like leaking implementation details (the timer delay) into the sequence number. Now this last one should actually work with the current drm_vblank_pre_modeset() since it does a drm_vblank_get() which will apply the cur_vblank-.last diff, but it also enables the vblank interrupt which is entirely pointless, and also wrong on Intel hardware (well, if we didn't have drm_vblank_off()). Our docs say that we shouldn't have the vblank interrupt enabled+unmasked while the pipe is off. Anyway it's not a very obvious way to do things. Ie. you're doing the drm_vblank_get() not because you actually want vblank interrupts, but because you want the side effects. I usually prefer straightforward code to magic. > Speaking of reducing or disabling the vblank disable timer, that should > be possible with drm_vblank_pre/post_modeset() as well. I get the impression that you're a bit hung up on the names :) We could rename the off/on to pre/post_modeset if you like, but then someone gets to audit all the other drivers. That someone isn't going to be me. > > - It doesn't work across suspend/resume since no one re-enables the vbl= ank > > interrupt. > = > That sounds like a driver bug to me. The driver should re-enable the > hardware interrupt on resume if the vblank interrupt is currently > enabled by the DRM core. The interrupt is not enabled due to drm_vblank_off(). There's nothing to undo that which is why I added drm_vblank_on(). -- = Ville Syrj=E4l=E4 Intel OTC