From patchwork Fri Jul 8 19:43:47 2011 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Chris Wilson X-Patchwork-Id: 957252 Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) by demeter1.kernel.org (8.14.4/8.14.4) with ESMTP id p68JiWLv029458 for ; Fri, 8 Jul 2011 19:44:52 GMT Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 8D209A024A for ; Fri, 8 Jul 2011 12:44:31 -0700 (PDT) X-Original-To: intel-gfx@lists.freedesktop.org Delivered-To: intel-gfx@lists.freedesktop.org Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by gabe.freedesktop.org (Postfix) with ESMTP id AB8A19E89D for ; Fri, 8 Jul 2011 12:43:50 -0700 (PDT) Received: from orsmga001.jf.intel.com ([10.7.209.18]) by orsmga101.jf.intel.com with ESMTP; 08 Jul 2011 12:43:50 -0700 Message-Id: X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="4.65,500,1304319600"; d="scan'208";a="25048894" Received: from unknown (HELO cwilso3-mobl.ger.corp.intel.com) ([10.255.17.149]) by orsmga001.jf.intel.com with SMTP; 08 Jul 2011 12:43:48 -0700 Received: by cwilso3-mobl.ger.corp.intel.com (sSMTP sendmail emulation); Fri, 08 Jul 2011 20:43:47 +0100 Date: Fri, 08 Jul 2011 20:43:47 +0100 To: intel-gfx@lists.freedesktop.org References: <1310124163-12177-1-git-send-email-chris@chris-wilson.co.uk> From: Chris Wilson In-Reply-To: <1310124163-12177-1-git-send-email-chris@chris-wilson.co.uk> Subject: Re: [Intel-gfx] FBC patchset X-BeenThere: intel-gfx@lists.freedesktop.org X-Mailman-Version: 2.1.11 Precedence: list List-Id: Intel graphics driver community testing & development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , MIME-Version: 1.0 Sender: intel-gfx-bounces+patchwork-intel-gfx=patchwork.kernel.org@lists.freedesktop.org Errors-To: intel-gfx-bounces+patchwork-intel-gfx=patchwork.kernel.org@lists.freedesktop.org X-Greylist: IP, sender and recipient auto-whitelisted, not delayed by milter-greylist-4.2.6 (demeter1.kernel.org [140.211.167.41]); Fri, 08 Jul 2011 19:44:52 +0000 (UTC) Oh dear, it was all looking too good. Works fine with just one output. Runs into a nasty race if you add a second and start unplugging things... The issue is that when unplugging an output, userspace sees this and appropriately reconfigures from an extended desktop to just using, for the sake of argument, the LVDS. As the desktop is changing size this requires a new framebuffer and so we begin to teardown the two crtcs and replace with just the one with the new fb. The first thing we do is disable the crtc connected to the unplugged display. This triggers an intel_update_fbc() which spots that it can now enable FBC on the current single crtc + old fb and promptly dispatches a delayed task to do so. The mode reconfiguration continues apace and we disable the other crtc and start to replace the plane's FB. This involves a couple of wait-for-vblanks, and so is quite slow. During this time we avoid taking the struct_mutex. Meanwhile, the delayed task kicks off on a second CPU grabs the struct_mutex and reconfigures the FBC registers. Apparently enabling FBC on a dead plane is an undefined operation. Hilarity ensues, followed by a swift reboot. Bumping to 250ms sufficiently delays the task to miss the race, but we can not foretell just how long any given crtc modeset will take. So what we need is to take the mode_config.lock in order to prevent concurrent access to the FBC registers and also to prevent the fb from disappearing beneath us: --- drivers/gpu/drm/i915/intel_display.c | 8 +++++++- 1 files changed, 7 insertions(+), 1 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index e375500..bec9e1d 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -1603,6 +1603,11 @@ static void intel_fbc_work_fn(struct work_struct *__work) struct drm_device *dev = work->crtc->dev; struct drm_i915_private *dev_priv = dev->dev_private; + if (!mutex_trylock(&dev->mode_config.mutex)) { + schedule_delayed_work(&work->work, msecs_to_jiffies(100)); + return; + } + mutex_lock(&dev->struct_mutex); if (work == dev_priv->fbc_work) { /* Double check that we haven't switched fb without cancelling @@ -1620,6 +1625,7 @@ static void intel_fbc_work_fn(struct work_struct *__work) dev_priv->fbc_work = NULL; } mutex_unlock(&dev->struct_mutex); + mutex_unlock(&dev->mode_config.mutex); kfree(work); } @@ -1684,7 +1690,7 @@ static void intel_enable_fbc(struct drm_crtc *crtc, unsigned long interval) * and indeed performing the enable as a co-routine and not * waiting synchronously upon the vblank. */ - schedule_delayed_work(&work->work, msecs_to_jiffies(50)); + schedule_delayed_work(&work->work, msecs_to_jiffies(250)); } void intel_disable_fbc(struct drm_device *dev)