[Intel-gfx] [PATCH] drm/i915: properly restore the ppgtt page directory on resume

[Daniel Vetter]

On Tue, Mar 27, 2012 at 07:30:11PM -0700, Ben Widawsky wrote:
> On Thu, 22 Mar 2012 00:14:43 +0100
> Daniel Vetter <daniel.vetter at ffwll.ch> 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 <daniel.vetter at ffwll.ch>
> > ---
> >  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 at ffwll.ch
Mobile: +41 (0)79 365 57 48