From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Vetter Subject: Re: [PATCH v3] drm: do not sleep on vblank while holding a mutex Date: Mon, 31 Oct 2011 22:09:44 +0100 Message-ID: <20111031210944.GD30108@phenom.ffwll.local> References: <1320081021-14970-1-git-send-email-ihadzic@research.bell-labs.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail-ww0-f43.google.com (mail-ww0-f43.google.com [74.125.82.43]) by gabe.freedesktop.org (Postfix) with ESMTP id AC7539E74E for ; Mon, 31 Oct 2011 14:08:48 -0700 (PDT) Received: by wwf4 with SMTP id 4so1811178wwf.12 for ; Mon, 31 Oct 2011 14:08:48 -0700 (PDT) Content-Disposition: inline In-Reply-To: <1320081021-14970-1-git-send-email-ihadzic@research.bell-labs.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: dri-devel-bounces+sf-dri-devel=m.gmane.org@lists.freedesktop.org Errors-To: dri-devel-bounces+sf-dri-devel=m.gmane.org@lists.freedesktop.org To: Ilija Hadzic Cc: dri-devel@lists.freedesktop.org List-Id: dri-devel@lists.freedesktop.org On Mon, Oct 31, 2011 at 01:10:21PM -0400, Ilija Hadzic wrote: > drm_wait_vblank must be DRM_UNLOCKED because otherwise it > will grab the drm_global_mutex and then go to sleep until the vblank > event it is waiting for. That can wreck havoc in the windowing system > because if one process issues this ioctl, it will block all other > processes for the duration of all vblanks between the current and the > one it is waiting for. In some cases it can block the entire windowing > system. > > v2: incorporate comments received from Daniel Vetter and > Michel Daenzer. > > v3: after a lengty discussion with Daniel Vetter, it was concluded > we should not worry about any locking, within drm_wait_vblank > function so this patch becomes a rather trivial removal > of drm_global_mutex from drm_wait_vblank That commit message is a bit garbage. What about ... "... it was concluded that the only think not yet protected with locks and atomic ops is the write to dev->last_vblank_wait. It's only used in a debug file in proc, and the current code already employs no correct locking: the proc file only takes dev->struct_mutex, whereas drm_wait_vblank implicitly took the drm_global_mutex. Given all this, it's not worth bothering to try to fix this at this time." I think it's important to correctly document the conclusion of this discussion, because we've worried quite a bit about correct locking. Yours, Daniel -- Daniel Vetter Mail: daniel@ffwll.ch Mobile: +41 (0)79 365 57 48