From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ben Widawsky Subject: Re: [PATCH v2 2/4] drm/i915: Ironlake mobile GPU with VT-d fix Date: Fri, 14 Oct 2011 19:08:25 -0700 Message-ID: <20111014190825.6f10070f@bwidawsk.net> References: <1316902987-24774-1-git-send-email-ben@bwidawsk.net> <1316902987-24774-3-git-send-email-ben@bwidawsk.net> <20110925094014.GA2852@phenom.ffwll.local> <20110925164257.69b55b70@bwidawsk.net> <20110926072317.GA2804@phenom.ffwll.local> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from cloud01.chad-versace.us (184-106-247-128.static.cloud-ips.com [184.106.247.128]) by gabe.freedesktop.org (Postfix) with ESMTP id 98F1A9E9B1 for ; Fri, 14 Oct 2011 19:08:36 -0700 (PDT) In-Reply-To: <20110926072317.GA2804@phenom.ffwll.local> 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: Daniel Vetter Cc: Dave Airlie , intel-gfx@lists.freedesktop.org, Woodhouse , David@freedesktop.org List-Id: intel-gfx@lists.freedesktop.org On Mon, 26 Sep 2011 09:23:17 +0200 Daniel Vetter wrote: > On Sun, Sep 25, 2011 at 04:42:57PM -0700, Ben Widawsky wrote: > > > > static int sandybridge_write_fence_reg(struct drm_i915_gem_object > > > > *obj, diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c > > > > b/drivers/gpu/drm/i915/i915_gem_gtt.c index 7a709cd..0c6226b 100644 > > > > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c > > > > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c > > > > @@ -49,10 +49,39 @@ static unsigned int > > > > cache_level_to_agp_type(struct drm_device *dev, } > > > > } > > > > > > > > +static bool do_idling(struct drm_i915_private *dev_priv) > > > > +{ > > > > + bool ret = dev_priv->mm.interruptible; > > > > + > > > > + if (dev_priv->mm.gtt->do_idle_maps) { > > > > + dev_priv->mm.interruptible = false; > > > > + if (i915_gpu_idle(dev_priv->dev)) > > > > + DRM_ERROR("couldn't idle GPU %d\n", ret); > > > > + } > > > > > > I don't like the uninterruptible sleep in core gem - this is only for > > > modesetting where backtracking is just not really feasible. I've > > > quickly checked the code and I haven't seen any problems in handing > > > back the -ERESTARTSYS to gem_object_unbind. > > > > > > > While I agree that we could probably return at idle time, we have to > > guarantee that no GPU activity occurs until the unmap has completed. I > > checked the code and don't really understand how it can occur, but I > > believe it *can* occur as I've seen it happen in the idle function. Any > > other proposal how to fix this, or any proof that the GPU can't wake up > > to do display stuff while doing the unmap? > > Hm, looks like some misunderstanding. I understand why the gpu_idle needs > to be here, my gripes are solely with the uninterruptible sleep that you > use. I think a normal interruptible sleep with passing back the return > value to gem_unbind_object should work: By the time we're calling > gtt_unbind_object in gem_unbind_object we have not yet committed to the > unbound state, so can just bail out if the mappings are still there. And > gtt_unbind_object idles the gpu before touching the mappings, so when we > have to bail out with -ERESTARTSYS the mapping is untouched and everything > safe. > -Daniel Okay. I agree your suggestion works. I've changed the code to do this. I will not listen to you for a while if someone comes in and just says make it non interruptible, because it seems to be happening a lot recently :P Ben