From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ben Widawsky Subject: Re: [PATCH] drm/i915: properly restore the ppgtt page directory on resume Date: Wed, 28 Mar 2012 10:06:23 -0700 Message-ID: <20120328100623.3a29b8ba@bwidawsk.net> References: <1332371683-3326-1-git-send-email-daniel.vetter@ffwll.ch> <20120327193011.1d79817d@bwidawsk.net> <20120328070552.GA4081@phenom.ffwll.local> <20120328100409.1325849e@bwidawsk.net> 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 D0933A0DBE for ; Wed, 28 Mar 2012 10:06:32 -0700 (PDT) In-Reply-To: <20120328100409.1325849e@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: Daniel Vetter , Intel Graphics Development List-Id: intel-gfx@lists.freedesktop.org NVM, Jesse has corrected me on IRC. On Wed, 28 Mar 2012 10:04:09 -0700 Ben Widawsky wrote: > On Wed, 28 Mar 2012 09:05:52 +0200 > Daniel Vetter wrote: > > > On Tue, Mar 27, 2012 at 07:30:11PM -0700, Ben Widawsky wrote: > > > On Thu, 22 Mar 2012 00:14:43 +0100 > > > Daniel Vetter wrote: > > > > > > > The ppgtt page directory lives in a snatched part of the gtt pte > > > > range. Which naturally gets cleared on hibernate when we pull > > > > the power. Suspend to ram (which is what I've tested) works > > > > because despite the fact that this is a mmio region, it is > > > > actually back by system ram. > > > > > > > > Fix this by moving the page directory setup code to the ppgtt > > > > init code (which gets called on resume). > > > > > > > > This fixes hibernate on my ivb and snb. > > > > > > > > Signed-Off-by: Daniel Vetter > > > > --- > > > > drivers/gpu/drm/i915/i915_gem.c | 22 > > > > +++++++++++++++++++++- drivers/gpu/drm/i915/i915_gem_gtt.c | > > > > 9 --------- 2 files changed, 21 insertions(+), 10 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/i915_gem.c > > > > b/drivers/gpu/drm/i915/i915_gem.c index 863e14a..6ab44be 100644 > > > > --- a/drivers/gpu/drm/i915/i915_gem.c > > > > +++ b/drivers/gpu/drm/i915/i915_gem.c > > > > @@ -3766,12 +3766,32 @@ void i915_gem_init_ppgtt(struct > > > > drm_device *dev) drm_i915_private_t *dev_priv = > > > > dev->dev_private; uint32_t pd_offset; > > > > struct intel_ring_buffer *ring; > > > > + struct i915_hw_ppgtt *ppgtt = > > > > dev_priv->mm.aliasing_ppgtt; > > > > + uint32_t __iomem *pd_addr; > > > > + uint32_t pd_entry; > > > > int i; > > > > > > > > if (!dev_priv->mm.aliasing_ppgtt) > > > > return; > > > > > > > > - pd_offset = dev_priv->mm.aliasing_ppgtt->pd_offset; > > > > + > > > > + pd_addr = dev_priv->mm.gtt->gtt + > > > > ppgtt->pd_offset/sizeof(uint32_t); > > > > + for (i = 0; i < ppgtt->num_pd_entries; i++) { > > > > + dma_addr_t pt_addr; > > > > + > > > > + if (dev_priv->mm.gtt->needs_dmar) > > > > + pt_addr = ppgtt->pt_dma_addr[i]; > > > > + else > > > > + pt_addr = > > > > page_to_phys(ppgtt->pt_pages[i]); + > > > > + pd_entry = GEN6_PDE_ADDR_ENCODE(pt_addr); > > > > + pd_entry |= GEN6_PDE_VALID; > > > > + > > > > + writel(pd_entry, pd_addr + i); > > > > + } > > > > + readl(pd_addr); > > > > > > I know I reviewed the other usage of this before, but remind me > > > why we need readl again (comment it, maybe?). > > > > The hw intercepts these mmio writes before it forwards them to > > memory to shoot down any system agent tlbs (i.e. for cpu access). > > Before we can clain to have set up global gtt entries we need to > > read back (so that the hw can stall us until it has finished tlb > > flushing). Standard procedure to ensure the hw sees the mmio > > ordering we want it to see, no comment needed imo. > > Since your patch is simply moving this logic around, I don't think > it's worth debating this too much further... > I didn't understand your explanation. Are you saying you are using > this as an I/O barrier? Any reason why wmb() is not sufficient? > > > > > > > + > > > > + pd_offset = ppgtt->pd_offset; > > > > pd_offset /= 64; /* in cachelines, */ > > > > pd_offset <<= 16; > > > > > > > > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c > > > > b/drivers/gpu/drm/i915/i915_gem_gtt.c index 72be806..99b27ff > > > > 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.c > > > > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c > > > > @@ -65,9 +65,7 @@ int i915_gem_init_aliasing_ppgtt(struct > > > > drm_device *dev) { > > > > struct drm_i915_private *dev_priv = dev->dev_private; > > > > struct i915_hw_ppgtt *ppgtt; > > > > - uint32_t pd_entry; > > > > unsigned first_pd_entry_in_global_pt; > > > > - uint32_t __iomem *pd_addr; > > > > int i; > > > > int ret = -ENOMEM; > > > > > > > > @@ -100,7 +98,6 @@ int i915_gem_init_aliasing_ppgtt(struct > > > > drm_device *dev) goto err_pt_alloc; > > > > } > > > > > > > > - pd_addr = dev_priv->mm.gtt->gtt + > > > > first_pd_entry_in_global_pt; for (i = 0; i < > > > > ppgtt->num_pd_entries; i++) { dma_addr_t pt_addr; > > > > if (dev_priv->mm.gtt->needs_dmar) { > > > > @@ -117,13 +114,7 @@ int i915_gem_init_aliasing_ppgtt(struct > > > > drm_device *dev) ppgtt->pt_dma_addr[i] = pt_addr; > > > > } else > > > > pt_addr = > > > > page_to_phys(ppgtt->pt_pages[i]); - > > > > - pd_entry = GEN6_PDE_ADDR_ENCODE(pt_addr); > > > > - pd_entry |= GEN6_PDE_VALID; > > > > - > > > > - writel(pd_entry, pd_addr + i); > > > > } > > > > - readl(pd_addr); > > > > > > > > ppgtt->scratch_page_dma_addr = > > > > dev_priv->mm.gtt->scratch_page_dma; > > > > > > It's kind of hard for me to tell that the GPU won't access > > > anything on resume that might kill things before hitting > > > init_ppgtt. Do you think it makes sense to disable ppgtt before > > > suspending? > > > > Afaik yanking the power does that for us - before I've re-enabled > > ppgtt on the resume path (the bit frobbing, not the page directory > > restoring) the hw just kept on using global gtt. > > Again since this is just moving code around, I don't think further > discussion here is required. > > > -Daniel > > Reviewed-by: Ben Widawsky > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx