From patchwork Fri Feb 13 20:03:46 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Daniel Vetter X-Patchwork-Id: 5827261 Return-Path: X-Original-To: patchwork-dri-devel@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork1.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork1.web.kernel.org (Postfix) with ESMTP id 89FEF9F30C for ; Fri, 13 Feb 2015 20:25:20 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 0542520270 for ; Fri, 13 Feb 2015 20:25:19 +0000 (UTC) Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) by mail.kernel.org (Postfix) with ESMTP id 1A2292026F for ; Fri, 13 Feb 2015 20:25:18 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 53AC46E313; Fri, 13 Feb 2015 12:25:17 -0800 (PST) X-Original-To: dri-devel@lists.freedesktop.org Delivered-To: dri-devel@lists.freedesktop.org Received: from mail-we0-f176.google.com (mail-we0-f176.google.com [74.125.82.176]) by gabe.freedesktop.org (Postfix) with ESMTP id 0A5156E7F0 for ; Fri, 13 Feb 2015 12:25:16 -0800 (PST) Received: by mail-we0-f176.google.com with SMTP id x3so18784037wes.7 for ; Fri, 13 Feb 2015 12:25:15 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ffwll.ch; s=google; h=from:to:cc:subject:date:message-id:in-reply-to:references; bh=JV/DuP3+LoGwSX7u7L5fnXay4rffw0tkIyr+lXXcOMA=; b=g4YphTk7/Myej+bryPFvRFQcRjNb6b76YUVwEERWd4MKd74sZtEjvk90Wl9cX81L0k xaps50kA1adqgfiv2ZQyDtgXusVTIv4CM7SvuZcN1/rC72ga1bM2ETlQgcXmIaBpB59T I0+M2CVw5RM7foNMOkO4+wVxigw0N9Hp2Csxw= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:from:to:cc:subject:date:message-id:in-reply-to :references; bh=JV/DuP3+LoGwSX7u7L5fnXay4rffw0tkIyr+lXXcOMA=; b=M4WmU9UUh5EBXJOQqjCHBinbt0b15hTvY8XnbbRh/ljoFyHMawzSUupL9UsB3j7mkN BS/grceESBZ946cwhk6kMKTDrGP3X0AlJqpAb0a+9Zjd42rTjqg/rqWx0poO4uiTmVSG 9ov8dcKgZ0HYg/v7AmSvUgCDNFW8YENXckzDg73xQsmgtJhwMarAC7TViQx5VPiNJAUa 1BibMqecNYjx8eOMeZiWXY2qRwrhbMomv2qWVdiYrQ5v6yM6KYXsv3z3WbmiP0VlSYr4 VpJKKkFYlLeW9gredigPgS6V+6xgTAd/OpRv9YarlznDp1TgnGMqAB65CIr3rVot8vfW pnog== X-Gm-Message-State: ALoCoQmg6ZsKRLulfTlrzQtDtnBs/TNMGtperxtIOtyLbOU1QuRAfthGBggRcaLUxD6LBGEDGbT2 X-Received: by 10.180.212.113 with SMTP id nj17mr20191822wic.54.1423859114576; Fri, 13 Feb 2015 12:25:14 -0800 (PST) Received: from fliege.ffwll.local (212-51-149-109.fiber7.init7.net. [212.51.149.109]) by mx.google.com with ESMTPSA id vq9sm11465726wjc.6.2015.02.13.12.25.12 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Fri, 13 Feb 2015 12:25:13 -0800 (PST) From: Daniel Vetter To: Intel Graphics Development , DRI Development Subject: [PATCH 5/5] drm/irq: Don't call ->get_vblank_counter directly from irq_uninstall/cleanup Date: Fri, 13 Feb 2015 21:03:46 +0100 Message-Id: <1423857826-11048-5-git-send-email-daniel.vetter@ffwll.ch> X-Mailer: git-send-email 1.9.3 In-Reply-To: <1423857826-11048-1-git-send-email-daniel.vetter@ffwll.ch> References: <1423857826-11048-1-git-send-email-daniel.vetter@ffwll.ch> Cc: Daniel Vetter , Daniel Vetter X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , MIME-Version: 1.0 Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" X-Spam-Status: No, score=-4.1 required=5.0 tests=BAYES_00,DKIM_SIGNED, RCVD_IN_DNSWL_MED, T_DKIM_INVALID, T_RP_MATCHES_RCVD, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP The pipe might already have been shut down, and then it's not a good idea to call hw accessor functions. Instead use the same logic as drm_vblank_off which has all the necessary checks to avoid troubles or inconsistency. Noticed by Imre while reviewing my patches to remove some sanity checks from ->get_vblank_counter. v2: Try harder. disable_and_save can still access the vblank stuff when vblank->enabled isn't set. It has to, since vlbank irq could be disable but the pipe is still on when being called from drm_vblank_off. But we still want to use that code for more code sharing. So add a check for vblank->enabled on top - if that's not set we shouldn't have anyone waiting for the vblank. If we have that's a pretty serious bug. The other issue that Imre spotted is drm_vblank_cleanup. That code again calls disable_and_save and so suffers from the same issues. But really drm_irq_uninstall should have cleaned that all up, so replace the code with WARN_ON. Note that we can't delete the timer cleanup since drivers aren't required to use drm_irq_install/uninstall, but can do their own irq handling. v3: Make it clear that all that gunk in drm_irq_uninstall is really just bandaids for UMS races between the irq/vblank code. In UMS userspace is in control of enabling/disabling interrupts in general and vblanks specifically. Cc: Imre Deak Signed-off-by: Daniel Vetter --- drivers/gpu/drm/drm_irq.c | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c index 1e5fb1b994d7..885fb756fed5 100644 --- a/drivers/gpu/drm/drm_irq.c +++ b/drivers/gpu/drm/drm_irq.c @@ -269,7 +269,6 @@ static void vblank_disable_fn(unsigned long arg) void drm_vblank_cleanup(struct drm_device *dev) { int crtc; - unsigned long irqflags; /* Bail if the driver didn't call drm_vblank_init() */ if (dev->num_crtcs == 0) @@ -278,11 +277,9 @@ void drm_vblank_cleanup(struct drm_device *dev) for (crtc = 0; crtc < dev->num_crtcs; crtc++) { struct drm_vblank_crtc *vblank = &dev->vblank[crtc]; - del_timer_sync(&vblank->disable_timer); + WARN_ON(vblank->enabled); - spin_lock_irqsave(&dev->vbl_lock, irqflags); - vblank_disable_and_save(dev, crtc); - spin_unlock_irqrestore(&dev->vbl_lock, irqflags); + del_timer_sync(&vblank->disable_timer); } kfree(dev->vblank); @@ -468,17 +465,23 @@ int drm_irq_uninstall(struct drm_device *dev) dev->irq_enabled = false; /* - * Wake up any waiters so they don't hang. + * Wake up any waiters so they don't hang. This is just to paper over + * isssues for UMS drivers which aren't in full control of their + * vblank/irq handling. KMS drivers must ensure that vblanks are all + * disabled when uninstalling the irq handler. */ if (dev->num_crtcs) { spin_lock_irqsave(&dev->vbl_lock, irqflags); for (i = 0; i < dev->num_crtcs; i++) { struct drm_vblank_crtc *vblank = &dev->vblank[i]; + if (!vblank->enabled) + continue; + + WARN_ON(drm_core_check_feature(dev, DRIVER_MODESET)); + + vblank_disable_and_save(dev, i); wake_up(&vblank->queue); - vblank->enabled = false; - vblank->last = - dev->driver->get_vblank_counter(dev, i); } spin_unlock_irqrestore(&dev->vbl_lock, irqflags); }