From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dan Carpenter Subject: Re: [patch v2] i915: take struct_mutex lock in intel_setup_overlay() Date: Sat, 19 Jun 2010 15:39:01 +0200 Message-ID: <20100619133901.GC5467@bicker> References: <20100430222757.GJ29093@bicker> <20100503075832.GL29093@bicker> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail-fx0-f49.google.com (mail-fx0-f49.google.com [209.85.161.49]) by gabe.freedesktop.org (Postfix) with ESMTP id D1CFA9E73D for ; Sat, 19 Jun 2010 06:39:18 -0700 (PDT) Received: by fxm11 with SMTP id 11so1175697fxm.36 for ; Sat, 19 Jun 2010 06:39:17 -0700 (PDT) Content-Disposition: inline In-Reply-To: <20100503075832.GL29093@bicker> 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: David Airlie Cc: Luca Barbieri , dri-devel@lists.freedesktop.org, Daniel Vetter List-Id: dri-devel@lists.freedesktop.org This patch never made it into 2.6.35 but it's still valid. Can you pick it up for 2.6.36? It still applies cleanly to 2.6.35-rc3 with fuzz (8 lines offset). regards, dan carpenter On Mon, May 03, 2010 at 09:58:32AM +0200, Dan Carpenter wrote: > I changed the drm_gem_object_unreference() to > drm_gem_object_unreference_unlocked(). > > "reg_bo" is a local variable so there are no synchronization issues, but > the problem is that if struct_mutex isn't held, it triggers a BUG_ON() > in drm_gem_object_free(). > > I also took the "struct_mutex" when calling i915_gem_object_pin(). > The i915_gem_object_pin() function calls i915_gem_object_bind_to_gtt() > which unreferences stuff if we are low on gtt space. The truth is that > if we run out of gtt space on this code path, we're sunk already > regardless of the locking. But we should take the lock anyway for > consistency and to make the static checkers happy. > > That call tree is: > i915_gem_object_pin() > => i915_gem_object_bind_to_gtt() > => i915_gem_evict_something() > => i915_gem_retire_requests() > => i915_gem_retire_request() > => drm_gem_object_unreference() > > intel_setup_overlay() is always called with "struct_mutex" unlocked. > There is only one call tree and it is: > > i915_load_modeset_init() > => intel_modeset_init() > => intel_setup_overlay() > > Signed-off-by: Dan Carpenter > --- > V2: The code is the same, but the change log is updated. > No one has complained about hitting the BUG_ON() and I can't tested it > myself, so this patch is probably linux-next material. > > diff --git a/drivers/gpu/drm/i915/intel_overlay.c b/drivers/gpu/drm/i915/intel_overlay.c > index 6d524a1..a103582 100644 > --- a/drivers/gpu/drm/i915/intel_overlay.c > +++ b/drivers/gpu/drm/i915/intel_overlay.c > @@ -1347,7 +1347,9 @@ void intel_setup_overlay(struct drm_device *dev) > overlay->reg_bo = to_intel_bo(reg_bo); > > if (OVERLAY_NONPHYSICAL(dev)) { > + mutex_lock(&dev->struct_mutex); > ret = i915_gem_object_pin(reg_bo, PAGE_SIZE); > + mutex_unlock(&dev->struct_mutex); > if (ret) { > DRM_ERROR("failed to pin overlay register bo\n"); > goto out_free_bo; > @@ -1385,7 +1387,7 @@ void intel_setup_overlay(struct drm_device *dev) > return; > > out_free_bo: > - drm_gem_object_unreference(reg_bo); > + drm_gem_object_unreference_unlocked(reg_bo); > out_free: > kfree(overlay); > return;