From mboxrd@z Thu Jan 1 00:00:00 1970 From: Chris Wilson Subject: Re: FBC patchset Date: Fri, 08 Jul 2011 20:43:47 +0100 Message-ID: References: <1310124163-12177-1-git-send-email-chris@chris-wilson.co.uk> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: 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) In-Reply-To: <1310124163-12177-1-git-send-email-chris@chris-wilson.co.uk> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org Errors-To: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org To: intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org 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) -- 1.7.6 -- Chris Wilson, Intel Open Source Technology Centre