From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Vetter Subject: Re: [PATCH] drm/i915: properly restore the ppgtt page directory on resume Date: Wed, 28 Mar 2012 09:05:52 +0200 Message-ID: <20120328070552.GA4081@phenom.ffwll.local> References: <1332371683-3326-1-git-send-email-daniel.vetter@ffwll.ch> <20120327193011.1d79817d@bwidawsk.net> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail-wg0-f43.google.com (mail-wg0-f43.google.com [74.125.82.43]) by gabe.freedesktop.org (Postfix) with ESMTP id D07F59E956 for ; Wed, 28 Mar 2012 00:05:13 -0700 (PDT) Received: by wgbdr12 with SMTP id dr12so519598wgb.12 for ; Wed, 28 Mar 2012 00:05:12 -0700 (PDT) Content-Disposition: inline In-Reply-To: <20120327193011.1d79817d@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 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. > > + > > + 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. -Daniel -- Daniel Vetter Mail: daniel@ffwll.ch Mobile: +41 (0)79 365 57 48