From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Vetter Subject: Re: [PATCH v2 2/4] drm/i915: Ironlake mobile GPU with VT-d fix Date: Mon, 26 Sep 2011 09:23:17 +0200 Message-ID: <20110926072317.GA2804@phenom.ffwll.local> 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> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail-wy0-f177.google.com (mail-wy0-f177.google.com [74.125.82.177]) by gabe.freedesktop.org (Postfix) with ESMTP id E47F29E78C for ; Mon, 26 Sep 2011 00:22:48 -0700 (PDT) Received: by wyi11 with SMTP id 11so4928924wyi.36 for ; Mon, 26 Sep 2011 00:22:47 -0700 (PDT) Content-Disposition: inline In-Reply-To: <20110925164257.69b55b70@bwidawsk.net> 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: Ben Widawsky Cc: Dave Airlie , intel-gfx@lists.freedesktop.org, David Woodhouse List-Id: intel-gfx@lists.freedesktop.org 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 -- Daniel Vetter Mail: daniel@ffwll.ch Mobile: +41 (0)79 365 57 48