From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ben Widawsky Subject: Re: [PATCH 1/4] drm/i915: fix module unload after context merge Date: Tue, 19 Jun 2012 10:32:47 -0700 Message-ID: <20120619103247.64614557@bwidawsk.net> References: <1340117552-3605-1-git-send-email-daniel.vetter@ffwll.ch> 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 4333A9F00B for ; Tue, 19 Jun 2012 10:32:55 -0700 (PDT) In-Reply-To: <1340117552-3605-1-git-send-email-daniel.vetter@ffwll.ch> 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: Intel Graphics Development List-Id: intel-gfx@lists.freedesktop.org On Tue, 19 Jun 2012 16:52:29 +0200 Daniel Vetter wrote: > commit 8e96d9c4d9843f00ebeb4a9b33596d96602ea101 > Author: Ben Widawsky > Date: Mon Jun 4 14:42:56 2012 -0700 > > drm/i915: reset the GPU on context fini > > broke module unload because it reset the gpu before we've stopped > touching it. Later on in the unload sequence the ringbuffer code > complained that the gpu would idle properly (because intel_gpu_reset > only resets the hw and not our sw state). > > Hence we need to reset the gpu hw as the very last thing before we > unmap the register mmio space. > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=51183 > Signed-Off-by: Daniel Vetter One thing which is both "wrong" with this patch, and the existing code is it does the reset after the default context has been destroyed. Ideally we want to reset before we make the object go away, as it's the true way of getting the GPU to stop referencing the object. It's *probably* fine that we do this in the "wrong" fashion as long as we can avoid doing a context switch... which I think we can only guarantee if we hold forcewake for the duration of unload, or don't have RC6 enabled. > --- > drivers/gpu/drm/i915/i915_dma.c | 7 +++++++ > drivers/gpu/drm/i915/i915_gem_context.c | 2 -- > 2 files changed, 7 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c > index f1544c5..a47ed44 100644 > --- a/drivers/gpu/drm/i915/i915_dma.c > +++ b/drivers/gpu/drm/i915/i915_dma.c > @@ -1731,6 +1731,13 @@ int i915_driver_unload(struct drm_device *dev) > i915_free_hws(dev); > } > > + > + /* The only known way to stop the gpu from accessing the hw context is > + * to reset it. Do this as the very last operation to avoid confusing > + * other code, leading to spurious errors. */ > + if (!dev_priv->hw_contexts_disabled) > + intel_gpu_reset(dev); > + > if (dev_priv->regs != NULL) > pci_iounmap(dev->pdev, dev_priv->regs); > > diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c > index 8fb8cd8..693718b 100644 > --- a/drivers/gpu/drm/i915/i915_gem_context.c > +++ b/drivers/gpu/drm/i915/i915_gem_context.c > @@ -280,8 +280,6 @@ void i915_gem_context_fini(struct drm_device *dev) > i915_gem_object_unpin(dev_priv->ring[RCS].default_context->obj); > > do_destroy(dev_priv->ring[RCS].default_context); > - > - intel_gpu_reset(dev); > } > > void i915_gem_context_open(struct drm_device *dev, struct drm_file *file) -- Ben Widawsky, Intel Open Source Technology Center