From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Vetter Subject: Re: [PATCH] drm/i915: Guard pages being reaped by OOM whilst binding-to-GTT Date: Tue, 20 Nov 2012 10:48:54 +0100 Message-ID: <20121120094854.GE5854@phenom.ffwll.local> References: <20121108134909.345d14e3@bwidawsk.net> <1352451530-10767-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 mail-ee0-f49.google.com (mail-ee0-f49.google.com [74.125.83.49]) by gabe.freedesktop.org (Postfix) with ESMTP id 4AA7FE618D for ; Tue, 20 Nov 2012 01:47:34 -0800 (PST) Received: by mail-ee0-f49.google.com with SMTP id c4so1621644eek.36 for ; Tue, 20 Nov 2012 01:47:33 -0800 (PST) Content-Disposition: inline In-Reply-To: <1352451530-10767-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: Chris Wilson Cc: intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org On Fri, Nov 09, 2012 at 08:58:50AM +0000, Chris Wilson wrote: > In the circumstances that the shrinker is allowed to steal the mutex > in order to reap pages, we need to be careful to prevent it operating on > the current object and shooting ourselves in the foot. > > Signed-off-by: Chris Wilson Ok, I this is definitely the major one that I've thought of, but I think I've found another place that need a similar treatment: i915_gem_fault: In between bind_to_gtt and vm_insert_pfn we have a call to get_fence, which might potentially call kmalloc due to a pending request. I know, it's highly unlikely that this will ever matter, but I think if we braket this with a pin/unpin, it will nicely serve as a reminder where to reserve/unreserve and object, in case we ever switch to a per-object locking scheme. Otherwise I can't poke holes into this any more ... Cheers, Daniel > --- > drivers/gpu/drm/i915/i915_gem.c | 9 ++++++++- > 1 file changed, 8 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index 159dca5..f225583 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -3091,6 +3091,8 @@ i915_gem_object_bind_to_gtt(struct drm_i915_gem_object *obj, > if (ret) > return ret; > > + i915_gem_object_pin_pages(obj); > + > search_free: > if (map_and_fenceable) > free_space = > @@ -3121,14 +3123,17 @@ i915_gem_object_bind_to_gtt(struct drm_i915_gem_object *obj, > obj->cache_level, > map_and_fenceable, > nonblocking); > - if (ret) > + if (ret) { > + i915_gem_object_unpin_pages(obj); > return ret; > + } > > goto search_free; > } > if (WARN_ON(!i915_gem_valid_gtt_space(dev, > obj->gtt_space, > obj->cache_level))) { > + i915_gem_object_unpin_pages(obj); > drm_mm_put_block(obj->gtt_space); > obj->gtt_space = NULL; > return -EINVAL; > @@ -3137,6 +3142,7 @@ i915_gem_object_bind_to_gtt(struct drm_i915_gem_object *obj, > > ret = i915_gem_gtt_prepare_object(obj); > if (ret) { > + i915_gem_object_unpin_pages(obj); > drm_mm_put_block(obj->gtt_space); > obj->gtt_space = NULL; > return ret; > @@ -3159,6 +3165,7 @@ i915_gem_object_bind_to_gtt(struct drm_i915_gem_object *obj, > > obj->map_and_fenceable = mappable && fenceable; > > + i915_gem_object_unpin_pages(obj); > trace_i915_gem_object_bind(obj, map_and_fenceable); > i915_gem_verify_gtt(dev); > return 0; > -- > 1.7.10.4 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch